| Message ID | 1469717505-8026-2-git-send-email-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Stefano, On 16/08/2016 01:21, Stefano Stabellini wrote: > On Thu, 28 Jul 2016, Julien Grall wrote: >> A follow-up patch will add more case to the switch that will require the >> IPA. So move the computation out of the switch. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> --- >> xen/arch/arm/traps.c | 36 ++++++++++++++++++------------------ >> 1 file changed, 18 insertions(+), 18 deletions(-) >> >> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >> index 683bcb2..46e0663 100644 >> --- a/xen/arch/arm/traps.c >> +++ b/xen/arch/arm/traps.c >> @@ -2403,35 +2403,35 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs, >> int rc; >> register_t gva = READ_SYSREG(FAR_EL2); >> uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK; >> + paddr_t gpa; >> + >> + if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) ) >> + gpa = get_faulting_ipa(gva); >> + else >> + { >> + /* >> + * Flush the TLB to make sure the DTLB is clear before >> + * doing GVA->IPA translation. If we got here because of >> + * an entry only present in the ITLB, this translation may >> + * still be inaccurate. >> + */ >> + flush_tlb_local(); >> + >> + rc = gva_to_ipa(gva, &gpa, GV2M_READ); >> + if ( rc == -EFAULT ) >> + return; /* Try again */ > > The issue with this is that now for any cases that don't require a gpa > if gva_to_ipa fails we wrongly return -EFAULT. Well, stage-1 fault is prioritized over stage-2 fault (see B3.12.3 in ARM DDI 0406C.b), so gva_to_ipa should never fail unless someone is playing with the stage-1 page table at the same time or because of an erratum (see 834220). In both case, we should replay the instruction to let the processor injecting the correct fault. FWIW, this is already what we do for the data abort handler. > > I suggest having two switches or falling through from the first case to > the second. I am not sure to understand your suggestion. Could you detail it? Regards,
Hi Stefano, On 16/08/16 17:20, Julien Grall wrote: > On 16/08/2016 01:21, Stefano Stabellini wrote: >> On Thu, 28 Jul 2016, Julien Grall wrote: >>> A follow-up patch will add more case to the switch that will require the >>> IPA. So move the computation out of the switch. >>> >>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>> --- >>> xen/arch/arm/traps.c | 36 ++++++++++++++++++------------------ >>> 1 file changed, 18 insertions(+), 18 deletions(-) >>> >>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >>> index 683bcb2..46e0663 100644 >>> --- a/xen/arch/arm/traps.c >>> +++ b/xen/arch/arm/traps.c >>> @@ -2403,35 +2403,35 @@ static void do_trap_instr_abort_guest(struct >>> cpu_user_regs *regs, >>> int rc; >>> register_t gva = READ_SYSREG(FAR_EL2); >>> uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK; >>> + paddr_t gpa; >>> + >>> + if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) ) >>> + gpa = get_faulting_ipa(gva); >>> + else >>> + { >>> + /* >>> + * Flush the TLB to make sure the DTLB is clear before >>> + * doing GVA->IPA translation. If we got here because of >>> + * an entry only present in the ITLB, this translation may >>> + * still be inaccurate. >>> + */ >>> + flush_tlb_local(); >>> + >>> + rc = gva_to_ipa(gva, &gpa, GV2M_READ); >>> + if ( rc == -EFAULT ) >>> + return; /* Try again */ >> >> The issue with this is that now for any cases that don't require a gpa >> if gva_to_ipa fails we wrongly return -EFAULT. > > Well, stage-1 fault is prioritized over stage-2 fault (see B3.12.3 in > ARM DDI 0406C.b), so gva_to_ipa should never fail unless someone is > playing with the stage-1 page table at the same time or because of an > erratum (see 834220). In both case, we should replay the instruction to > let the processor injecting the correct fault. > > FWIW, this is already what we do for the data abort handler. > >> >> I suggest having two switches or falling through from the first case to >> the second. > > I am not sure to understand your suggestion. Could you detail it? Do you have any thoughts here or are you fine with the current approach? Cheers,
On 31/08/16 20:43, Stefano Stabellini wrote: > On Tue, 16 Aug 2016, Julien Grall wrote: >> Hi Stefano, >> >> On 16/08/2016 01:21, Stefano Stabellini wrote: >>> On Thu, 28 Jul 2016, Julien Grall wrote: >>>> A follow-up patch will add more case to the switch that will require the >>>> IPA. So move the computation out of the switch. >>>> >>>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>>> --- >>>> xen/arch/arm/traps.c | 36 ++++++++++++++++++------------------ >>>> 1 file changed, 18 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >>>> index 683bcb2..46e0663 100644 >>>> --- a/xen/arch/arm/traps.c >>>> +++ b/xen/arch/arm/traps.c >>>> @@ -2403,35 +2403,35 @@ static void do_trap_instr_abort_guest(struct >>>> cpu_user_regs *regs, >>>> int rc; >>>> register_t gva = READ_SYSREG(FAR_EL2); >>>> uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK; >>>> + paddr_t gpa; >>>> + >>>> + if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) ) >>>> + gpa = get_faulting_ipa(gva); >>>> + else >>>> + { >>>> + /* >>>> + * Flush the TLB to make sure the DTLB is clear before >>>> + * doing GVA->IPA translation. If we got here because of >>>> + * an entry only present in the ITLB, this translation may >>>> + * still be inaccurate. >>>> + */ >>>> + flush_tlb_local(); >>>> + >>>> + rc = gva_to_ipa(gva, &gpa, GV2M_READ); >>>> + if ( rc == -EFAULT ) >>>> + return; /* Try again */ >>> >>> The issue with this is that now for any cases that don't require a gpa >>> if gva_to_ipa fails we wrongly return -EFAULT. >> >> Well, stage-1 fault is prioritized over stage-2 fault (see B3.12.3 in ARM DDI >> 0406C.b), so gva_to_ipa should never fail unless someone is playing with the >> stage-1 page table at the same time or because of an erratum (see 834220). In >> both case, we should replay the instruction to let the processor injecting the >> correct fault. >> >> FWIW, this is already what we do for the data abort handler. >> >>> >>> I suggest having two switches or falling through from the first case to >>> the second. >> >> I am not sure to understand your suggestion. Could you detail it? > > I was merely suggesting to add another switch like: > > + switch ( fsc ) > + { > + case FSC_FLT_PERM: > + case BLA: > + find gpa; > + default: > + } > > switch ( fsc ) > { > > but given that we are already relying on the gva_to_ipa translation in > the data abort handler, your patch is fine too. > > Acked-by: Stefano Stabellini <sstabellini@kernel.org> Thank you! FWIW, I am planning to merge the instruction and data abort paths after Xen 4.8. I will avoid to diverge between the two implementations. Cheers,
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 683bcb2..46e0663 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -2403,35 +2403,35 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs, int rc; register_t gva = READ_SYSREG(FAR_EL2); uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK; + paddr_t gpa; + + if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) ) + gpa = get_faulting_ipa(gva); + else + { + /* + * Flush the TLB to make sure the DTLB is clear before + * doing GVA->IPA translation. If we got here because of + * an entry only present in the ITLB, this translation may + * still be inaccurate. + */ + flush_tlb_local(); + + rc = gva_to_ipa(gva, &gpa, GV2M_READ); + if ( rc == -EFAULT ) + return; /* Try again */ + } switch ( fsc ) { case FSC_FLT_PERM: { - paddr_t gpa; const struct npfec npfec = { .insn_fetch = 1, .gla_valid = 1, .kind = hsr.iabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla }; - if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) ) - gpa = get_faulting_ipa(gva); - else - { - /* - * Flush the TLB to make sure the DTLB is clear before - * doing GVA->IPA translation. If we got here because of - * an entry only present in the ITLB, this translation may - * still be inaccurate. - */ - flush_tlb_local(); - - rc = gva_to_ipa(gva, &gpa, GV2M_READ); - if ( rc == -EFAULT ) - return; /* Try again */ - } - rc = p2m_mem_access_check(gpa, gva, npfec); /* Trap was triggered by mem_access, work here is done */
A follow-up patch will add more case to the switch that will require the IPA. So move the computation out of the switch. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/traps.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-)