diff mbox

[7/9] xen/arm: traps: MMIO should only be emulated for fault translation

Message ID 1466601669-25398-8-git-send-email-julien.grall@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Julien Grall June 22, 2016, 1:21 p.m. UTC
The function do_trap_data_abort_guest assumes that a stage-2 data abort
can only be taken for a translation fault or permission fault today.

Whilst this is true today, it might not be in the future. Rather than
emulating the MMIO for any fault other than the permission one, print
a warning message when the fault is not handled by Xen.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/traps.c | 51 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

Comments

Stefano Stabellini July 14, 2016, 3:06 p.m. UTC | #1
On Wed, 22 Jun 2016, Julien Grall wrote:
> The function do_trap_data_abort_guest assumes that a stage-2 data abort
> can only be taken for a translation fault or permission fault today.
> 
> Whilst this is true today, it might not be in the future. Rather than
> emulating the MMIO for any fault other than the permission one, print
> a warning message when the fault is not handled by Xen.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/traps.c | 51 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 28 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 785e3e9..591de3c 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2468,35 +2468,40 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>          /* Trap was triggered by mem_access, work here is done */
>          if ( !rc )
>              return;
> +        break;
>      }
> -    break;
> -    }
> -
> -    if ( dabt.s1ptw )
> -        goto bad_data_abort;
> +    case FSC_FLT_TRANS:
> +        if ( dabt.s1ptw )
> +            goto bad_data_abort;
>  
> -    /* XXX: Decode the instruction if ISS is not valid */
> -    if ( !dabt.valid )
> -        goto bad_data_abort;
> +        /* XXX: Decode the instruction if ISS is not valid */
> +        if ( !dabt.valid )
> +            goto bad_data_abort;
>  
> -    /*
> -     * Erratum 766422: Thumb store translation fault to Hypervisor may
> -     * not have correct HSR Rt value.
> -     */
> -    if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) && dabt.write )
> -    {
> -        rc = decode_instruction(regs, &info.dabt);
> -        if ( rc )
> +        /*
> +         * Erratum 766422: Thumb store translation fault to Hypervisor may
> +         * not have correct HSR Rt value.
> +         */
> +        if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
> +             dabt.write )
>          {
> -            gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> -            goto bad_data_abort;
> +            rc = decode_instruction(regs, &info.dabt);
> +            if ( rc )
> +            {
> +                gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> +                goto bad_data_abort;
> +            }
>          }
> -    }
>  
> -    if (handle_mmio(&info))
> -    {
> -        advance_pc(regs, hsr);
> -        return;
> +        if ( handle_mmio(&info) )
> +        {
> +            advance_pc(regs, hsr);
> +            return;
> +        }
> +        break;
> +    default:
> +        gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n",
> +                hsr.bits, dabt.dfsc);

Given that bad_data_abort, which is right after, will print HSR again, I
would remove it from this message as it's redundant.


>      }
>  
>  bad_data_abort:
> -- 
> 1.9.1
>
Julien Grall July 14, 2016, 3:23 p.m. UTC | #2
Hi Stefano,

On 14/07/16 16:06, Stefano Stabellini wrote:
> On Wed, 22 Jun 2016, Julien Grall wrote:
>> -    if (handle_mmio(&info))
>> -    {
>> -        advance_pc(regs, hsr);
>> -        return;
>> +        if ( handle_mmio(&info) )
>> +        {
>> +            advance_pc(regs, hsr);
>> +            return;
>> +        }
>> +        break;
>> +    default:
>> +        gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n",
>> +                hsr.bits, dabt.dfsc);
>
> Given that bad_data_abort, which is right after, will print HSR again, I
> would remove it from this message as it's redundant.

I agree this is redundant, however the message below will only be 
printed in debug build because a guest could trigger it easily. The 
message above is here to catch any possible misconfiguration of stage-2 
page table or hardware issue (ECC error, address size, TLB conflict...) 
and could be seen in non-debug build.

Cheers,
Stefano Stabellini July 14, 2016, 3:28 p.m. UTC | #3
On Thu, 14 Jul 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 14/07/16 16:06, Stefano Stabellini wrote:
> > On Wed, 22 Jun 2016, Julien Grall wrote:
> > > -    if (handle_mmio(&info))
> > > -    {
> > > -        advance_pc(regs, hsr);
> > > -        return;
> > > +        if ( handle_mmio(&info) )
> > > +        {
> > > +            advance_pc(regs, hsr);
> > > +            return;
> > > +        }
> > > +        break;
> > > +    default:
> > > +        gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n",
> > > +                hsr.bits, dabt.dfsc);
> > 
> > Given that bad_data_abort, which is right after, will print HSR again, I
> > would remove it from this message as it's redundant.
> 
> I agree this is redundant, however the message below will only be printed in
> debug build because a guest could trigger it easily. The message above is here
> to catch any possible misconfiguration of stage-2 page table or hardware issue
> (ECC error, address size, TLB conflict...) and could be seen in non-debug
> build.

Fair enough, it will just look a bit weird in the source code.

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Julien Grall July 14, 2016, 3:29 p.m. UTC | #4
On 14/07/16 16:28, Stefano Stabellini wrote:
> On Thu, 14 Jul 2016, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 14/07/16 16:06, Stefano Stabellini wrote:
>>> On Wed, 22 Jun 2016, Julien Grall wrote:
>>>> -    if (handle_mmio(&info))
>>>> -    {
>>>> -        advance_pc(regs, hsr);
>>>> -        return;
>>>> +        if ( handle_mmio(&info) )
>>>> +        {
>>>> +            advance_pc(regs, hsr);
>>>> +            return;
>>>> +        }
>>>> +        break;
>>>> +    default:
>>>> +        gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n",
>>>> +                hsr.bits, dabt.dfsc);
>>>
>>> Given that bad_data_abort, which is right after, will print HSR again, I
>>> would remove it from this message as it's redundant.
>>
>> I agree this is redundant, however the message below will only be printed in
>> debug build because a guest could trigger it easily. The message above is here
>> to catch any possible misconfiguration of stage-2 page table or hardware issue
>> (ECC error, address size, TLB conflict...) and could be seen in non-debug
>> build.
>
> Fair enough, it will just look a bit weird in the source code.

Would a comment in the source code make it less weird?

>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>
diff mbox

Patch

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 785e3e9..591de3c 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2468,35 +2468,40 @@  static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
         /* Trap was triggered by mem_access, work here is done */
         if ( !rc )
             return;
+        break;
     }
-    break;
-    }
-
-    if ( dabt.s1ptw )
-        goto bad_data_abort;
+    case FSC_FLT_TRANS:
+        if ( dabt.s1ptw )
+            goto bad_data_abort;
 
-    /* XXX: Decode the instruction if ISS is not valid */
-    if ( !dabt.valid )
-        goto bad_data_abort;
+        /* XXX: Decode the instruction if ISS is not valid */
+        if ( !dabt.valid )
+            goto bad_data_abort;
 
-    /*
-     * Erratum 766422: Thumb store translation fault to Hypervisor may
-     * not have correct HSR Rt value.
-     */
-    if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) && dabt.write )
-    {
-        rc = decode_instruction(regs, &info.dabt);
-        if ( rc )
+        /*
+         * Erratum 766422: Thumb store translation fault to Hypervisor may
+         * not have correct HSR Rt value.
+         */
+        if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
+             dabt.write )
         {
-            gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
-            goto bad_data_abort;
+            rc = decode_instruction(regs, &info.dabt);
+            if ( rc )
+            {
+                gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
+                goto bad_data_abort;
+            }
         }
-    }
 
-    if (handle_mmio(&info))
-    {
-        advance_pc(regs, hsr);
-        return;
+        if ( handle_mmio(&info) )
+        {
+            advance_pc(regs, hsr);
+            return;
+        }
+        break;
+    default:
+        gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n",
+                hsr.bits, dabt.dfsc);
     }
 
 bad_data_abort: