Re: [Xen-devel] [RFC 01/22] xen/arm: do_trap_instr_abort_guest: Move the IPA computation out of the switch

[Julien Grall]

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 
---
 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, , 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 


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,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [RFC 01/22] xen/arm: do_trap_instr_abort_guest: Move the IPA computation out of the switch

[Stefano Stabellini]

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 
> > > ---
> > >  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, , 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 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [RFC 01/22] xen/arm: do_trap_instr_abort_guest: Move the IPA computation out of the switch

[Julien Grall]

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 
---
 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, , 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,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [RFC 01/22] xen/arm: do_trap_instr_abort_guest: Move the IPA computation out of the switch

[Julien Grall]

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 
---
 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, , 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,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [RFC 01/22] xen/arm: do_trap_instr_abort_guest: Move the IPA computation out of the switch

[Stefano Stabellini]

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 
> ---
>  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, , 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.

I suggest having two switches or falling through from the first case to
the second.


> +}
>  
>  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, , 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 */
> -- 
> 1.9.1
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [RFC 01/22] xen/arm: do_trap_instr_abort_guest: Move the IPA computation out of the switch

[Julien Grall]

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 
---
 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, , 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, , 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 */
-- 
1.9.1


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel