[XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Ayan Kumar Halder]

Refer to Armv8 ARM DDI 0487G.b, Page D13-3219 "ISS encoding for an exception
from a Data Abort" :-
ISV - ISS[23:14] holds a valid instruction syndrome

When the ISV is 0, the instruction could not be decoded by the hardware (ie ISS
is invalid). One should immediately return an error to the caller with an
appropriate error message. There is no use of the MMIO handler. This is the
reason why one should check for ISV before attempting to find a MMIO handler.

Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
---

Suggested by Julien Grall in https://lists.xenproject.org/archives/html/xen-devel/2022-01/msg01245.html

 xen/arch/arm/io.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 729287e37c..14d39222f2 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -109,6 +109,13 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs,
 
     ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
 
+    /* All the instructions used on emulated MMIO region should be valid */
+    if ( !dabt.valid )
+    {
+        gprintk(XENLOG_DEBUG, "No valid instruction syndrome for data abort\n");
+        return IO_ABORT;
+    }
+
     handler = find_mmio_handler(v->domain, info.gpa);
     if ( !handler )
     {
@@ -121,10 +128,6 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs,
         return rc;
     }
 
-    /* All the instructions used on emulated MMIO region should be valid */
-    if ( !dabt.valid )
-        return IO_ABORT;
-
     /*
      * Erratum 766422: Thumb store translation fault to Hypervisor may
      * not have correct HSR Rt value.
-- 
2.17.1

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

Hi,

On Wed, 26 Jan 2022, 16:58 Ayan Kumar Halder, <ayan.kumar.halder@xilinx.com>
wrote:

> Refer to Armv8 ARM DDI 0487G.b, Page D13-3219 "ISS encoding for an
> exception
> from a Data Abort" :-
> ISV - ISS[23:14] holds a valid instruction syndrome
>
> When the ISV is 0, the instruction could not be decoded by the hardware
> (ie ISS
> is invalid). One should immediately return an error to the caller with an
> appropriate error message.


That's going to be very spammy because we have use-case where it could trap
without a valid ISV (e.g. when break-before-make happens). So please don't
had a message.

That makes me think that the same will be valid for your other patch.

There is no use of the MMIO handler. This is the
> reason why one should check for ISV before attempting to find a MMIO
> handler.
>
> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
> ---
>
> Suggested by Julien Grall in
> https://lists.xenproject.org/archives/html/xen-devel/2022-01/msg01245.html
>
>  xen/arch/arm/io.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index 729287e37c..14d39222f2 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -109,6 +109,13 @@ enum io_state try_handle_mmio(struct cpu_user_regs
> *regs,
>
>      ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
>
> +    /* All the instructions used on emulated MMIO region should be valid
> */
> +    if ( !dabt.valid )
> +    {
> +        gprintk(XENLOG_DEBUG, "No valid instruction syndrome for data
> abort\n");
> +        return IO_ABORT;
> +    }
> +
>      handler = find_mmio_handler(v->domain, info.gpa);
>      if ( !handler )
>      {
> @@ -121,10 +128,6 @@ enum io_state try_handle_mmio(struct cpu_user_regs
> *regs,
>          return rc;
>      }
>
> -    /* All the instructions used on emulated MMIO region should be valid
> */
> -    if ( !dabt.valid )
> -        return IO_ABORT;
> -
>      /*
>       * Erratum 766422: Thumb store translation fault to Hypervisor may
>       * not have correct HSR Rt value.
> --
> 2.17
>

[-- Attachment #2: Type: text/html, Size: 3161 bytes --]

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Ayan Kumar Halder]

[-- Attachment #1: Type: text/plain, Size: 3060 bytes --]

Hi Julien,

On 26/01/2022 17:22, Julien Grall wrote:
> Hi,
>
> On Wed, 26 Jan 2022, 16:58 Ayan Kumar Halder, 
> <ayan.kumar.halder@xilinx.com> wrote:
>
>     Refer to Armv8 ARM DDI 0487G.b, Page D13-3219 "ISS encoding for an
>     exception
>     from a Data Abort" :-
>     ISV - ISS[23:14] holds a valid instruction syndrome
>
>     When the ISV is 0, the instruction could not be decoded by the
>     hardware (ie ISS
>     is invalid). One should immediately return an error to the caller
>     with an
>     appropriate error message. 
>
>
> That's going to be very spammy because we have use-case where it could 
> trap without a valid ISV (e.g. when break-before-make happens). So 
> please don't had a message.

There is already a logging statement in traps.c :-

inject_abt:
     gdprintk(XENLOG_DEBUG,
              "HSR=%#"PRIregister" pc=%#"PRIregister" gva=%#"PRIvaddr" 
gpa=%#"PRIpaddr"\n",
              hsr.bits, regs->pc, gva, gpa);

The reason for the error is to give the user some hint that an IO abort 
is triggered by Xen. Can we keep the error message ?

Also, I think this is a duplicate check 
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/ioreq.c;h=308650b40051825fdea78bb33bfbcc87ef5deaff;hb=HEAD#l79 
, I will remove this in v2 if it makes sense.

- Ayan

>
> That makes me think that the same will be valid for your other patch.
>
>     There is no use of the MMIO handler. This is the
>     reason why one should check for ISV before attempting to find a
>     MMIO handler.
>
>     Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
>     ---
>
>     Suggested by Julien Grall in
>     https://lists.xenproject.org/archives/html/xen-devel/2022-01/msg01245.html
>
>      xen/arch/arm/io.c | 11 +++++++----
>      1 file changed, 7 insertions(+), 4 deletions(-)
>
>     diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>     index 729287e37c..14d39222f2 100644
>     --- a/xen/arch/arm/io.c
>     +++ b/xen/arch/arm/io.c
>     @@ -109,6 +109,13 @@ enum io_state try_handle_mmio(struct
>     cpu_user_regs *regs,
>
>          ASSERT(hsr.ec <http://hsr.ec> == HSR_EC_DATA_ABORT_LOWER_EL);
>
>     +    /* All the instructions used on emulated MMIO region should
>     be valid */
>     +    if ( !dabt.valid )
>     +    {
>     +        gprintk(XENLOG_DEBUG, "No valid instruction syndrome for
>     data abort\n");
>     +        return IO_ABORT;
>     +    }
>     +
>          handler = find_mmio_handler(v->domain, info.gpa);
>          if ( !handler )
>          {
>     @@ -121,10 +128,6 @@ enum io_state try_handle_mmio(struct
>     cpu_user_regs *regs,
>              return rc;
>          }
>
>     -    /* All the instructions used on emulated MMIO region should
>     be valid */
>     -    if ( !dabt.valid )
>     -        return IO_ABORT;
>     -
>          /*
>           * Erratum 766422: Thumb store translation fault to
>     Hypervisor may
>           * not have correct HSR Rt value.
>     -- 
>     2.17
>

[-- Attachment #2: Type: text/html, Size: 6063 bytes --]

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 3514 bytes --]

Hi,

On Wed, 26 Jan 2022, 17:56 Ayan Kumar Halder, <ayan.kumar.halder@xilinx.com>
wrote:

> Hi Julien,
> On 26/01/2022 17:22, Julien Grall wrote:
>
> Hi,
>
> On Wed, 26 Jan 2022, 16:58 Ayan Kumar Halder, <
> ayan.kumar.halder@xilinx.com> wrote:
>
>> Refer to Armv8 ARM DDI 0487G.b, Page D13-3219 "ISS encoding for an
>> exception
>> from a Data Abort" :-
>> ISV - ISS[23:14] holds a valid instruction syndrome
>>
>> When the ISV is 0, the instruction could not be decoded by the hardware
>> (ie ISS
>> is invalid). One should immediately return an error to the caller with an
>> appropriate error message.
>
>
> That's going to be very spammy because we have use-case where it could
> trap without a valid ISV (e.g. when break-before-make happens). So please
> don't had a message.
>
> There is already a logging statement in traps.c :-
>
> inject_abt:
>     gdprintk(XENLOG_DEBUG,
>              "HSR=%#"PRIregister" pc=%#"PRIregister" gva=%#"PRIvaddr"
> gpa=%#"PRIpaddr"\n",
>              hsr.bits, regs->pc, gva, gpa);
>
> The reason for the error is to give the user some hint that an IO abort is
> triggered by Xen.
>
The main difference here is inject_dabt should only be reached when we
exhausted all the possibility in I/O handling.

In your case, if we can't handle as an MMIO then we should fallthrough the
other method (see do_trap_stage2_abort_guest()).

In fact, moving the check early and doing the decoding before find_mmio()
or try_fwd_io() is actually wrong. Sorry I should realized that earlier.

So we want to provide an helper that will do the dabt.vid check and do the
decoding. The helper will be called in 2 places.

With that in place, then I agree the gprintk could be kept in place.

Can we keep the error message ?
>
> Also, I think this is a duplicate check
> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/ioreq.c;h=308650b40051825fdea78bb33bfbcc87ef5deaff;hb=HEAD#l79
> , I will remove this in v2 if it makes sense.
>
> - Ayan
>
>
> That makes me think that the same will be valid for your other patch.
>
> There is no use of the MMIO handler. This is the
>> reason why one should check for ISV before attempting to find a MMIO
>> handler.
>>
>> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
>> ---
>>
>> Suggested by Julien Grall in
>> https://lists.xenproject.org/archives/html/xen-devel/2022-01/msg01245.html
>>
>>  xen/arch/arm/io.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>> index 729287e37c..14d39222f2 100644
>> --- a/xen/arch/arm/io.c
>> +++ b/xen/arch/arm/io.c
>> @@ -109,6 +109,13 @@ enum io_state try_handle_mmio(struct cpu_user_regs
>> *regs,
>>
>>      ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
>>
>> +    /* All the instructions used on emulated MMIO region should be valid
>> */
>> +    if ( !dabt.valid )
>> +    {
>> +        gprintk(XENLOG_DEBUG, "No valid instruction syndrome for data
>> abort\n");
>> +        return IO_ABORT;
>> +    }
>> +
>>      handler = find_mmio_handler(v->domain, info.gpa);
>>      if ( !handler )
>>      {
>> @@ -121,10 +128,6 @@ enum io_state try_handle_mmio(struct cpu_user_regs
>> *regs,
>>          return rc;
>>      }
>>
>> -    /* All the instructions used on emulated MMIO region should be valid
>> */
>> -    if ( !dabt.valid )
>> -        return IO_ABORT;
>> -
>>      /*
>>       * Erratum 766422: Thumb store translation fault to Hypervisor may
>>       * not have correct HSR Rt value.
>> --
>> 2.17
>>
>

[-- Attachment #2: Type: text/html, Size: 7051 bytes --]

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Ayan Kumar Halder]

Hi,

Asking here as well (might not have been clear on irc).

On 27/01/2022 00:10, Julien Grall wrote:
> Hi,
>
> On Wed, 26 Jan 2022, 17:56 Ayan Kumar Halder, 
> <ayan.kumar.halder@xilinx.com> wrote:
>
>     Hi Julien,
>
>     On 26/01/2022 17:22, Julien Grall wrote:
>>     Hi,
>>
>>     On Wed, 26 Jan 2022, 16:58 Ayan Kumar Halder,
>>     <ayan.kumar.halder@xilinx.com> wrote:
>>
>>         Refer to Armv8 ARM DDI 0487G.b, Page D13-3219 "ISS encoding
>>         for an exception
>>         from a Data Abort" :-
>>         ISV - ISS[23:14] holds a valid instruction syndrome
>>
>>         When the ISV is 0, the instruction could not be decoded by
>>         the hardware (ie ISS
>>         is invalid). One should immediately return an error to the
>>         caller with an
>>         appropriate error message. 
>>
>>
>>     That's going to be very spammy because we have use-case where it
>>     could trap without a valid ISV (e.g. when break-before-make
>>     happens). So please don't had a message.
>
>     There is already a logging statement in traps.c :-
>
>     inject_abt:
>         gdprintk(XENLOG_DEBUG,
>                  "HSR=%#"PRIregister" pc=%#"PRIregister"
>     gva=%#"PRIvaddr" gpa=%#"PRIpaddr"\n",
>                  hsr.bits, regs->pc, gva, gpa);
>
>     The reason for the error is to give the user some hint that an IO
>     abort is triggered by Xen.
>
> The main difference here is inject_dabt should only be reached when we 
> exhausted all the possibility in I/O handling.
>
> In your case, if we can't handle as an MMIO then we should fallthrough 
> the other method (see do_trap_stage2_abort_guest()).
>
> In fact, moving the check early and doing the decoding before 
> find_mmio() or try_fwd_io() is actually wrong. Sorry I should realized 
> that earlier.

Why should we care about MMIO when ISS is invalid ? Should we not check 
first if the ISS is valid or not as it will trigger IO_abort regardless 
of the MMIO.

I am assuming that once Xen resolves the MMIO 
(p2m_resolve_translation_fault()), the guest will again try to run the 
same instruction on MMIO region. This will be trapped in Xen which will 
return IO abort as the post-indexing instruction could not be decoded.

In this scenario, translation fault resolved by Xen was of no use.

Please help to clear my doubts.

- Ayan

>
> So we want to provide an helper that will do the dabt.vid check and do 
> the decoding. The helper will be called in 2 places.
>
> With that in place, then I agree the gprintk could be kept in place.
>
>     Can we keep the error message ?
>
>     Also, I think this is a duplicate check
>     https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/ioreq.c;h=308650b40051825fdea78bb33bfbcc87ef5deaff;hb=HEAD#l79
>     , I will remove this in v2 if it makes sense.
>
>     - Ayan
>
>>
>>     That makes me think that the same will be valid for your other patch.
>>
>>         There is no use of the MMIO handler. This is the
>>         reason why one should check for ISV before attempting to find
>>         a MMIO handler.
>>
>>         Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
>>         ---
>>
>>         Suggested by Julien Grall in
>>         https://lists.xenproject.org/archives/html/xen-devel/2022-01/msg01245.html
>>
>>          xen/arch/arm/io.c | 11 +++++++----
>>          1 file changed, 7 insertions(+), 4 deletions(-)
>>
>>         diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>>         index 729287e37c..14d39222f2 100644
>>         --- a/xen/arch/arm/io.c
>>         +++ b/xen/arch/arm/io.c
>>         @@ -109,6 +109,13 @@ enum io_state try_handle_mmio(struct
>>         cpu_user_regs *regs,
>>
>>              ASSERT(hsr.ec <http://hsr.ec> ==
>>         HSR_EC_DATA_ABORT_LOWER_EL);
>>
>>         +    /* All the instructions used on emulated MMIO region
>>         should be valid */
>>         +    if ( !dabt.valid )
>>         +    {
>>         +        gprintk(XENLOG_DEBUG, "No valid instruction syndrome
>>         for data abort\n");
>>         +        return IO_ABORT;
>>         +    }
>>         +
>>              handler = find_mmio_handler(v->domain, info.gpa);
>>              if ( !handler )
>>              {
>>         @@ -121,10 +128,6 @@ enum io_state try_handle_mmio(struct
>>         cpu_user_regs *regs,
>>                  return rc;
>>              }
>>
>>         -    /* All the instructions used on emulated MMIO region
>>         should be valid */
>>         -    if ( !dabt.valid )
>>         -        return IO_ABORT;
>>         -
>>              /*
>>               * Erratum 766422: Thumb store translation fault to
>>         Hypervisor may
>>               * not have correct HSR Rt value.
>>         -- 
>>         2.17
>>

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Julien Grall]

On 27/01/2022 13:20, Ayan Kumar Halder wrote:
> Hi,
> 
> Asking here as well (might not have been clear on irc).
> 
> On 27/01/2022 00:10, Julien Grall wrote:
>> Hi,
>>
>> On Wed, 26 Jan 2022, 17:56 Ayan Kumar Halder, 
>> <ayan.kumar.halder@xilinx.com> wrote:
>>
>>     Hi Julien,
>>
>>     On 26/01/2022 17:22, Julien Grall wrote:
>>>     Hi,
>>>
>>>     On Wed, 26 Jan 2022, 16:58 Ayan Kumar Halder,
>>>     <ayan.kumar.halder@xilinx.com> wrote:
>>>
>>>         Refer to Armv8 ARM DDI 0487G.b, Page D13-3219 "ISS encoding
>>>         for an exception
>>>         from a Data Abort" :-
>>>         ISV - ISS[23:14] holds a valid instruction syndrome
>>>
>>>         When the ISV is 0, the instruction could not be decoded by
>>>         the hardware (ie ISS
>>>         is invalid). One should immediately return an error to the
>>>         caller with an
>>>         appropriate error message.
>>>
>>>     That's going to be very spammy because we have use-case where it
>>>     could trap without a valid ISV (e.g. when break-before-make
>>>     happens). So please don't had a message.
>>
>>     There is already a logging statement in traps.c :-
>>
>>     inject_abt:
>>         gdprintk(XENLOG_DEBUG,
>>                  "HSR=%#"PRIregister" pc=%#"PRIregister"
>>     gva=%#"PRIvaddr" gpa=%#"PRIpaddr"\n",
>>                  hsr.bits, regs->pc, gva, gpa);
>>
>>     The reason for the error is to give the user some hint that an IO
>>     abort is triggered by Xen.
>>
>> The main difference here is inject_dabt should only be reached when we 
>> exhausted all the possibility in I/O handling.
>>
>> In your case, if we can't handle as an MMIO then we should fallthrough 
>> the other method (see do_trap_stage2_abort_guest()).
>>
>> In fact, moving the check early and doing the decoding before 
>> find_mmio() or try_fwd_io() is actually wrong. Sorry I should realized 
>> that earlier.
> 
> Why should we care about MMIO when ISS is invalid ?

Because a translation fault doesn't mean this is emulated MMIO. This may 
be actual RAM but with the stage-2 entry marked as invalid for tracking 
purpose (or else).

> Should we not check 
> first if the ISS is valid or not as it will trigger IO_abort regardless 
> of the MMIO.

No. Imagine the guest decides to use a store exclusive on a RAM region 
that was temporally marked as invalid in the stage-2 page-table.

This will generate a data abort in Xen with ISV=0. We want to try to 
resolve the fault first and retry the instruction.

> 
> I am assuming that once Xen resolves the MMIO 
> (p2m_resolve_translation_fault()), the guest will again try to run the 
> same instruction on MMIO region. This will be trapped in Xen which will 
> return IO abort as the post-indexing instruction could not be decoded.

The access will not trap again in Xen if the fault was resolved.

Cheers,

-- 
Julien Grall

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Ayan Kumar Halder]

Hi Julien,

Thanks a lot for your clarification. Appreciate your help. I have a 
concern as below:-

On 27/01/2022 13:57, Julien Grall wrote:
>
>
> On 27/01/2022 13:20, Ayan Kumar Halder wrote:
>> Hi,
>>
>> Asking here as well (might not have been clear on irc).
>>
>> On 27/01/2022 00:10, Julien Grall wrote:
>>> Hi,
>>>
>>> On Wed, 26 Jan 2022, 17:56 Ayan Kumar Halder, 
>>> <ayan.kumar.halder@xilinx.com> wrote:
>>>
>>>     Hi Julien,
>>>
>>>     On 26/01/2022 17:22, Julien Grall wrote:
>>>>     Hi,
>>>>
>>>>     On Wed, 26 Jan 2022, 16:58 Ayan Kumar Halder,
>>>>     <ayan.kumar.halder@xilinx.com> wrote:
>>>>
>>>>         Refer to Armv8 ARM DDI 0487G.b, Page D13-3219 "ISS encoding
>>>>         for an exception
>>>>         from a Data Abort" :-
>>>>         ISV - ISS[23:14] holds a valid instruction syndrome
>>>>
>>>>         When the ISV is 0, the instruction could not be decoded by
>>>>         the hardware (ie ISS
>>>>         is invalid). One should immediately return an error to the
>>>>         caller with an
>>>>         appropriate error message.
>>>>
>>>>     That's going to be very spammy because we have use-case where it
>>>>     could trap without a valid ISV (e.g. when break-before-make
>>>>     happens). So please don't had a message.
>>>
>>>     There is already a logging statement in traps.c :-
>>>
>>>     inject_abt:
>>>         gdprintk(XENLOG_DEBUG,
>>>                  "HSR=%#"PRIregister" pc=%#"PRIregister"
>>>     gva=%#"PRIvaddr" gpa=%#"PRIpaddr"\n",
>>>                  hsr.bits, regs->pc, gva, gpa);
>>>
>>>     The reason for the error is to give the user some hint that an IO
>>>     abort is triggered by Xen.
>>>
>>> The main difference here is inject_dabt should only be reached when 
>>> we exhausted all the possibility in I/O handling.
>>>
>>> In your case, if we can't handle as an MMIO then we should 
>>> fallthrough the other method (see do_trap_stage2_abort_guest()).
>>>
>>> In fact, moving the check early and doing the decoding before 
>>> find_mmio() or try_fwd_io() is actually wrong. Sorry I should 
>>> realized that earlier.
>>
>> Why should we care about MMIO when ISS is invalid ?
>
> Because a translation fault doesn't mean this is emulated MMIO. This 
> may be actual RAM but with the stage-2 entry marked as invalid for 
> tracking purpose (or else).
>
>> Should we not check first if the ISS is valid or not as it will 
>> trigger IO_abort regardless of the MMIO.
>
> No. Imagine the guest decides to use a store exclusive on a RAM region 
> that was temporally marked as invalid in the stage-2 page-table.
>
> This will generate a data abort in Xen with ISV=0. We want to try to 
> resolve the fault first and retry the instruction.
>
>>
>> I am assuming that once Xen resolves the MMIO 
>> (p2m_resolve_translation_fault()), the guest will again try to run 
>> the same instruction on MMIO region. This will be trapped in Xen 
>> which will return IO abort as the post-indexing instruction could not 
>> be decoded.
>
> The access will not trap again in Xen if the fault was resolved.

I think your words makes sense.

However, I am still concerned that we might not be doing the correct 
thing in try_fwd_ioserv().

See this :-

     ioreq_t p = {
         .type = IOREQ_TYPE_COPY,
         .addr = info->gpa,
         .size = 1 << info->dabt.size,
         .count = 1,
         .dir = !info->dabt.write,
         /*
          * On x86, df is used by 'rep' instruction to tell the direction
          * to iterate (forward or backward).
          * On Arm, all the accesses to MMIO region will do a single
          * memory access. So for now, we can safely always set to 0.
          */
         .df = 0,
         .data = get_user_reg(regs, info->dabt.reg),
         .state = STATE_IOREQ_READY,
     };
If info->dabt.valid = 0, then '.size', '.dir' and '.data' fields are 
invalid.

'.size' gets used in ioreq_server_select() to obtain the end address. It 
seems incorrect to me.

I suppose "if ( !info->dabt.valid )" needs to be checked before "s = 
ioreq_server_select(v->domain, &p);".

What do you think ?

- Ayan

>
> Cheers,
>

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Julien Grall]

On 27/01/2022 15:48, Ayan Kumar Halder wrote:
> Hi Julien,

Hi,

> On 27/01/2022 13:57, Julien Grall wrote:
>>
>>
>> On 27/01/2022 13:20, Ayan Kumar Halder wrote:
>>> Hi,
>>>
>>> Asking here as well (might not have been clear on irc).
>>>
>>> On 27/01/2022 00:10, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On Wed, 26 Jan 2022, 17:56 Ayan Kumar Halder, 
>>>> <ayan.kumar.halder@xilinx.com> wrote:
>>>>
>>>>     Hi Julien,
>>>>
>>>>     On 26/01/2022 17:22, Julien Grall wrote:
>>>>>     Hi,
>>>>>
>>>>>     On Wed, 26 Jan 2022, 16:58 Ayan Kumar Halder,
>>>>>     <ayan.kumar.halder@xilinx.com> wrote:
>>>>>
>>>>>         Refer to Armv8 ARM DDI 0487G.b, Page D13-3219 "ISS encoding
>>>>>         for an exception
>>>>>         from a Data Abort" :-
>>>>>         ISV - ISS[23:14] holds a valid instruction syndrome
>>>>>
>>>>>         When the ISV is 0, the instruction could not be decoded by
>>>>>         the hardware (ie ISS
>>>>>         is invalid). One should immediately return an error to the
>>>>>         caller with an
>>>>>         appropriate error message.
>>>>>
>>>>>     That's going to be very spammy because we have use-case where it
>>>>>     could trap without a valid ISV (e.g. when break-before-make
>>>>>     happens). So please don't had a message.
>>>>
>>>>     There is already a logging statement in traps.c :-
>>>>
>>>>     inject_abt:
>>>>         gdprintk(XENLOG_DEBUG,
>>>>                  "HSR=%#"PRIregister" pc=%#"PRIregister"
>>>>     gva=%#"PRIvaddr" gpa=%#"PRIpaddr"\n",
>>>>                  hsr.bits, regs->pc, gva, gpa);
>>>>
>>>>     The reason for the error is to give the user some hint that an IO
>>>>     abort is triggered by Xen.
>>>>
>>>> The main difference here is inject_dabt should only be reached when 
>>>> we exhausted all the possibility in I/O handling.
>>>>
>>>> In your case, if we can't handle as an MMIO then we should 
>>>> fallthrough the other method (see do_trap_stage2_abort_guest()).
>>>>
>>>> In fact, moving the check early and doing the decoding before 
>>>> find_mmio() or try_fwd_io() is actually wrong. Sorry I should 
>>>> realized that earlier.
>>>
>>> Why should we care about MMIO when ISS is invalid ?
>>
>> Because a translation fault doesn't mean this is emulated MMIO. This 
>> may be actual RAM but with the stage-2 entry marked as invalid for 
>> tracking purpose (or else).
>>
>>> Should we not check first if the ISS is valid or not as it will 
>>> trigger IO_abort regardless of the MMIO.
>>
>> No. Imagine the guest decides to use a store exclusive on a RAM region 
>> that was temporally marked as invalid in the stage-2 page-table.
>>
>> This will generate a data abort in Xen with ISV=0. We want to try to 
>> resolve the fault first and retry the instruction.
>>
>>>
>>> I am assuming that once Xen resolves the MMIO 
>>> (p2m_resolve_translation_fault()), the guest will again try to run 
>>> the same instruction on MMIO region. This will be trapped in Xen 
>>> which will return IO abort as the post-indexing instruction could not 
>>> be decoded.
>>
>> The access will not trap again in Xen if the fault was resolved.
> 
> I think your words makes sense.
> 
> However, I am still concerned that we might not be doing the correct 
> thing in try_fwd_ioserv().
> 
> See this :-
> 
>      ioreq_t p = {
>          .type = IOREQ_TYPE_COPY,
>          .addr = info->gpa,
>          .size = 1 << info->dabt.size,
>          .count = 1,
>          .dir = !info->dabt.write,
>          /*
>           * On x86, df is used by 'rep' instruction to tell the direction
>           * to iterate (forward or backward).
>           * On Arm, all the accesses to MMIO region will do a single
>           * memory access. So for now, we can safely always set to 0.
>           */
>          .df = 0,
>          .data = get_user_reg(regs, info->dabt.reg),
>          .state = STATE_IOREQ_READY,
>      };
> If info->dabt.valid = 0, then '.size', '.dir' and '.data' fields are 
> invalid.

Sort of. ISV=0 means that bits [23:14] are RES0. So this doesn't cover 
the field WnR. The validity of WnR will depend on DFSC. In our case, we 
will always reach this code with a translation fault. So WnR should 
always be valid.

That said, the rest will not be valid.

> 
> '.size' gets used in ioreq_server_select() to obtain the end address. It 
> seems incorrect to me.
> 
> I suppose "if ( !info->dabt.valid )" needs to be checked before "s = 
> ioreq_server_select(v->domain, &p);".

The trouble is we would need to return IO_UNHANDLED (so the rest of the 
code can pick it up) which I think is incorrect here.

The approach I can think of is to call ioreq_server_select() with size = 
0 (i.e. byte access). Then decode the size (if needed) and then check 
the access is correct.

This is not very nice. But that at the same time, I would like to avoid 
dealing emulated I/O in Xen or outside differently. Stefano?

Cheers,

-- 
Julien Grall

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 7403 bytes --]

On Thu, 27 Jan 2022, Julien Grall wrote:
> On 27/01/2022 15:48, Ayan Kumar Halder wrote:
> > On 27/01/2022 13:57, Julien Grall wrote:
> > > 
> > > 
> > > On 27/01/2022 13:20, Ayan Kumar Halder wrote:
> > > > Hi,
> > > > 
> > > > Asking here as well (might not have been clear on irc).
> > > > 
> > > > On 27/01/2022 00:10, Julien Grall wrote:
> > > > > Hi,
> > > > > 
> > > > > On Wed, 26 Jan 2022, 17:56 Ayan Kumar Halder,
> > > > > <ayan.kumar.halder@xilinx.com> wrote:
> > > > > 
> > > > >     Hi Julien,
> > > > > 
> > > > >     On 26/01/2022 17:22, Julien Grall wrote:
> > > > > >     Hi,
> > > > > > 
> > > > > >     On Wed, 26 Jan 2022, 16:58 Ayan Kumar Halder,
> > > > > >     <ayan.kumar.halder@xilinx.com> wrote:
> > > > > > 
> > > > > >         Refer to Armv8 ARM DDI 0487G.b, Page D13-3219 "ISS encoding
> > > > > >         for an exception
> > > > > >         from a Data Abort" :-
> > > > > >         ISV - ISS[23:14] holds a valid instruction syndrome
> > > > > > 
> > > > > >         When the ISV is 0, the instruction could not be decoded by
> > > > > >         the hardware (ie ISS
> > > > > >         is invalid). One should immediately return an error to the
> > > > > >         caller with an
> > > > > >         appropriate error message.
> > > > > > 
> > > > > >     That's going to be very spammy because we have use-case where it
> > > > > >     could trap without a valid ISV (e.g. when break-before-make
> > > > > >     happens). So please don't had a message.
> > > > > 
> > > > >     There is already a logging statement in traps.c :-
> > > > > 
> > > > >     inject_abt:
> > > > >         gdprintk(XENLOG_DEBUG,
> > > > >                  "HSR=%#"PRIregister" pc=%#"PRIregister"
> > > > >     gva=%#"PRIvaddr" gpa=%#"PRIpaddr"\n",
> > > > >                  hsr.bits, regs->pc, gva, gpa);
> > > > > 
> > > > >     The reason for the error is to give the user some hint that an IO
> > > > >     abort is triggered by Xen.
> > > > > 
> > > > > The main difference here is inject_dabt should only be reached when we
> > > > > exhausted all the possibility in I/O handling.
> > > > > 
> > > > > In your case, if we can't handle as an MMIO then we should fallthrough
> > > > > the other method (see do_trap_stage2_abort_guest()).
> > > > > 
> > > > > In fact, moving the check early and doing the decoding before
> > > > > find_mmio() or try_fwd_io() is actually wrong. Sorry I should realized
> > > > > that earlier.
> > > > 
> > > > Why should we care about MMIO when ISS is invalid ?
> > > 
> > > Because a translation fault doesn't mean this is emulated MMIO. This may
> > > be actual RAM but with the stage-2 entry marked as invalid for tracking
> > > purpose (or else).
> > > 
> > > > Should we not check first if the ISS is valid or not as it will trigger
> > > > IO_abort regardless of the MMIO.
> > > 
> > > No. Imagine the guest decides to use a store exclusive on a RAM region
> > > that was temporally marked as invalid in the stage-2 page-table.
> > > 
> > > This will generate a data abort in Xen with ISV=0. We want to try to
> > > resolve the fault first and retry the instruction.
> > > 
> > > > 
> > > > I am assuming that once Xen resolves the MMIO
> > > > (p2m_resolve_translation_fault()), the guest will again try to run the
> > > > same instruction on MMIO region. This will be trapped in Xen which will
> > > > return IO abort as the post-indexing instruction could not be decoded.
> > > 
> > > The access will not trap again in Xen if the fault was resolved.
> > 
> > I think your words makes sense.
> > 
> > However, I am still concerned that we might not be doing the correct thing
> > in try_fwd_ioserv().
> > 
> > See this :-
> > 
> >      ioreq_t p = {
> >          .type = IOREQ_TYPE_COPY,
> >          .addr = info->gpa,
> >          .size = 1 << info->dabt.size,
> >          .count = 1,
> >          .dir = !info->dabt.write,
> >          /*
> >           * On x86, df is used by 'rep' instruction to tell the direction
> >           * to iterate (forward or backward).
> >           * On Arm, all the accesses to MMIO region will do a single
> >           * memory access. So for now, we can safely always set to 0.
> >           */
> >          .df = 0,
> >          .data = get_user_reg(regs, info->dabt.reg),
> >          .state = STATE_IOREQ_READY,
> >      };
> > If info->dabt.valid = 0, then '.size', '.dir' and '.data' fields are
> > invalid.
> 
> Sort of. ISV=0 means that bits [23:14] are RES0. So this doesn't cover the
> field WnR. The validity of WnR will depend on DFSC. In our case, we will
> always reach this code with a translation fault. So WnR should always be
> valid.
> 
> That said, the rest will not be valid.
> 
> > 
> > '.size' gets used in ioreq_server_select() to obtain the end address. It
> > seems incorrect to me.
> > 
> > I suppose "if ( !info->dabt.valid )" needs to be checked before "s =
> > ioreq_server_select(v->domain, &p);".
> 
> The trouble is we would need to return IO_UNHANDLED (so the rest of the code
> can pick it up) which I think is incorrect here.
> 
> The approach I can think of is to call ioreq_server_select() with size = 0
> (i.e. byte access). Then decode the size (if needed) and then check the access
> is correct.
> 
> This is not very nice. But that at the same time, I would like to avoid
> dealing emulated I/O in Xen or outside differently. Stefano?

I am with you on both points.

One thing I noticed is that the code today is not able to deal with
IO_UNHANDLED for MMIO regions handled by IOREQ servers or Xen MMIO
emulator handlers. p2m_resolve_translation_fault and try_map_mmio are
called after try_handle_mmio returns IO_UNHANDLED but try_handle_mmio is
not called a second time (or am I mistaken?)

Another thing I noticed is that currently find_mmio_handler and
try_fwd_ioserv expect dabt to be already populated and valid so it would
be better if we could get there only when dabt.valid.

With these two things in mind, I think maybe the best thing to do is to
change the code in do_trap_stage2_abort_guest slightly so that
p2m_resolve_translation_fault and try_map_mmio are called first when
!dabt.valid.

Patch below only for explaning; it doesn't build as is and I am not sure
it is 100% correct. For instance, if try_map_mmio succeeds, should we
return or goto again?


diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 9339d12f58..d09f901a50 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1965,7 +1965,8 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
          *
          * Note that emulated region cannot be executed
          */
-        if ( is_data )
+again:
+        if ( is_data && hsr.dabt.valid )
         {
             enum io_state state = try_handle_mmio(regs, hsr, gpa);
 
@@ -1996,6 +1997,12 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
         if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
             return;
 
+        if ( !hsr.dabt.valid )
+        {
+            if ( !decode_instruction(regs, &hsr.dabt) )
+                goto again;
+        }
+
         break;
     default:
         gprintk(XENLOG_WARNING,

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Julien Grall]

On Thu, 27 Jan 2022 at 22:40, Stefano Stabellini <sstabellini@kernel.org> wrote:
> I am with you on both points.
>
> One thing I noticed is that the code today is not able to deal with
> IO_UNHANDLED for MMIO regions handled by IOREQ servers or Xen MMIO
> emulator handlers. p2m_resolve_translation_fault and try_map_mmio are
> called after try_handle_mmio returns IO_UNHANDLED but try_handle_mmio is
> not called a second time (or am I mistaken?)

Why would you need it? If try_mmio_fault() doesn't work the first time, then
it will not work the second it.

>
> Another thing I noticed is that currently find_mmio_handler and
> try_fwd_ioserv expect dabt to be already populated and valid so it would
> be better if we could get there only when dabt.valid.
>
> With these two things in mind, I think maybe the best thing to do is to
> change the code in do_trap_stage2_abort_guest slightly so that
> p2m_resolve_translation_fault and try_map_mmio are called first when
> !dabt.valid.

An abort will mostly likely happen because of emulated I/O. If we call
p2m_resolve_translation_fault() and try_map_mmio() first, then it means
the processing will take longer than necessary for the common case.

So I think we want to keep the order as it is. I.e first trying the MMIO
and then falling back to the less likely reason for a trap.

>
> Patch below only for explaning; it doesn't build as is and I am not sure
> it is 100% correct. For instance, if try_map_mmio succeeds, should we
> return or goto again?

If try_map_mmio() succeeds then it means we create a valid entry
in the stage-2. Therefore, we want to return to the guest and
try to execute the instruction.

The same is valid for p2m_resolve_translation_fault(). If it works
then it means we successfully set the valid bit in an entry. So
we want to re-try directly.

Cheers,

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Julien Grall]

On Thu, 27 Jan 2022 at 23:05, Julien Grall <julien.grall.oss@gmail.com> wrote:
>
> On Thu, 27 Jan 2022 at 22:40, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > I am with you on both points.
> >
> > One thing I noticed is that the code today is not able to deal with
> > IO_UNHANDLED for MMIO regions handled by IOREQ servers or Xen MMIO
> > emulator handlers. p2m_resolve_translation_fault and try_map_mmio are
> > called after try_handle_mmio returns IO_UNHANDLED but try_handle_mmio is
> > not called a second time (or am I mistaken?)
>
> Why would you need it? If try_mmio_fault() doesn't work the first time, then

Sorry I meant try_handle_mmio().

> it will not work the second it.
>
> >
> > Another thing I noticed is that currently find_mmio_handler and
> > try_fwd_ioserv expect dabt to be already populated and valid so it would
> > be better if we could get there only when dabt.valid.
> >
> > With these two things in mind, I think maybe the best thing to do is to
> > change the code in do_trap_stage2_abort_guest slightly so that
> > p2m_resolve_translation_fault and try_map_mmio are called first when
> > !dabt.valid.
>
> An abort will mostly likely happen because of emulated I/O. If we call
> p2m_resolve_translation_fault() and try_map_mmio() first, then it means
> the processing will take longer than necessary for the common case.
>
> So I think we want to keep the order as it is. I.e first trying the MMIO
> and then falling back to the less likely reason for a trap.
>
> >
> > Patch below only for explaning; it doesn't build as is and I am not sure
> > it is 100% correct. For instance, if try_map_mmio succeeds, should we
> > return or goto again?
>
> If try_map_mmio() succeeds then it means we create a valid entry
> in the stage-2. Therefore, we want to return to the guest and
> try to execute the instruction.
>
> The same is valid for p2m_resolve_translation_fault(). If it works
> then it means we successfully set the valid bit in an entry. So
> we want to re-try directly.
>
> Cheers,

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Stefano Stabellini]

On Thu, 27 Jan 2022, Julien Grall wrote:
> On Thu, 27 Jan 2022 at 23:05, Julien Grall <julien.grall.oss@gmail.com> wrote:
> >
> > On Thu, 27 Jan 2022 at 22:40, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > > I am with you on both points.
> > >
> > > One thing I noticed is that the code today is not able to deal with
> > > IO_UNHANDLED for MMIO regions handled by IOREQ servers or Xen MMIO
> > > emulator handlers. p2m_resolve_translation_fault and try_map_mmio are
> > > called after try_handle_mmio returns IO_UNHANDLED but try_handle_mmio is
> > > not called a second time (or am I mistaken?)
> >
> > Why would you need it? If try_mmio_fault() doesn't work the first time, then
> 
> Sorry I meant try_handle_mmio().
> 
> > it will not work the second it.

I think I explained myself badly, I'll try again below.


> > > Another thing I noticed is that currently find_mmio_handler and
> > > try_fwd_ioserv expect dabt to be already populated and valid so it would
> > > be better if we could get there only when dabt.valid.
> > >
> > > With these two things in mind, I think maybe the best thing to do is to
> > > change the code in do_trap_stage2_abort_guest slightly so that
> > > p2m_resolve_translation_fault and try_map_mmio are called first when
> > > !dabt.valid.
> >
> > An abort will mostly likely happen because of emulated I/O. If we call
> > p2m_resolve_translation_fault() and try_map_mmio() first, then it means
> > the processing will take longer than necessary for the common case.
> >
> > So I think we want to keep the order as it is. I.e first trying the MMIO
> > and then falling back to the less likely reason for a trap.

Yeah I thought about it as well. The idea would be that if dabt.valid is
set then we leave things as they are (we call try_handle_mmio first) but
if dabt.valid is not set (it is not valid) then we skip the
try_handle_mmio() call because it wouldn't succeed anyway and go
directly to p2m_resolve_translation_fault() and try_map_mmio().

If either of them work (also reading what you wrote about it) then we
return immediately.

If not, then we call decode_instruction from do_trap_stage2_abort_guest
and try again. The second time dabt.valid is set so we end up calling
try_handle_mmio() as usual.

Just for clarity let me copy/paste the relevant code, apologies if it
was already obvious to you -- I got the impression my suggestion wasn't
very clear.



+again:
+        if ( is_data && hsr.dabt.valid )
        {
            enum io_state state = try_handle_mmio(regs, hsr, gpa);

            switch ( state )
            {
            case IO_ABORT:
                goto inject_abt;
            case IO_HANDLED:
                advance_pc(regs, hsr);
                return;
            case IO_RETRY:
                /* finish later */
                return;
            case IO_UNHANDLED:
                /* IO unhandled, try another way to handle it. */
                break;
            }
        }

        /*
         * First check if the translation fault can be resolved by the
         * P2M subsystem. If that's the case nothing else to do.
         */
        if ( p2m_resolve_translation_fault(current->domain,
                                           gaddr_to_gfn(gpa)) )
            return;

        if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
            return;

+        if ( !hsr.dabt.valid )
+        {
+            if ( !decode_instruction(regs, &hsr.dabt) )
+                goto again;
+        }

        break;
    default:
        gprintk(XENLOG_WARNING,
                "Unsupported FSC: HSR=%#"PRIregister" DFSC=%#x\n",
                hsr.bits, xabt.fsc);
    }

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Julien Grall]

On 28/01/2022 01:20, Stefano Stabellini wrote:
> On Thu, 27 Jan 2022, Julien Grall wrote:
>> On Thu, 27 Jan 2022 at 23:05, Julien Grall <julien.grall.oss@gmail.com> wrote:
>>>
>>> On Thu, 27 Jan 2022 at 22:40, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>> I am with you on both points.
>>>>
>>>> One thing I noticed is that the code today is not able to deal with
>>>> IO_UNHANDLED for MMIO regions handled by IOREQ servers or Xen MMIO
>>>> emulator handlers. p2m_resolve_translation_fault and try_map_mmio are
>>>> called after try_handle_mmio returns IO_UNHANDLED but try_handle_mmio is
>>>> not called a second time (or am I mistaken?)
>>>
>>> Why would you need it? If try_mmio_fault() doesn't work the first time, then
>>
>> Sorry I meant try_handle_mmio().
>>
>>> it will not work the second it.
> 
> I think I explained myself badly, I'll try again below.
> 
> 
>>>> Another thing I noticed is that currently find_mmio_handler and
>>>> try_fwd_ioserv expect dabt to be already populated and valid so it would
>>>> be better if we could get there only when dabt.valid.
>>>>
>>>> With these two things in mind, I think maybe the best thing to do is to
>>>> change the code in do_trap_stage2_abort_guest slightly so that
>>>> p2m_resolve_translation_fault and try_map_mmio are called first when
>>>> !dabt.valid.
>>>
>>> An abort will mostly likely happen because of emulated I/O. If we call
>>> p2m_resolve_translation_fault() and try_map_mmio() first, then it means
>>> the processing will take longer than necessary for the common case.
>>>
>>> So I think we want to keep the order as it is. I.e first trying the MMIO
>>> and then falling back to the less likely reason for a trap.
> 
> Yeah I thought about it as well. The idea would be that if dabt.valid is
> set then we leave things as they are (we call try_handle_mmio first) but
> if dabt.valid is not set (it is not valid) then we skip the
> try_handle_mmio() call because it wouldn't succeed anyway and go
> directly to p2m_resolve_translation_fault() and try_map_mmio().
> 
> If either of them work (also reading what you wrote about it) then we
> return immediately.

Ok. So the assumption is data abort with invalid syndrome would mostly 
likely be because of a fault handled by p2m_resolve_translation_fault().

I think this makes sense. However, I am not convinced we can currently 
safely call try_map_mmio() before try_handle_mmio(). This is because the 
logic in try_map_mmio() is quite fragile and we may mistakenly map an 
emulated region.

Similarly, we can't call try_map_mmio() before 
p2m_resolve_translation_fault() because a transient fault may be
misinterpreted.

I think we may be able to harden try_map_mmio() by checking if the I/O 
region is emulated. But this will need to be fully thought through first.

> 
> If not, then we call decode_instruction from do_trap_stage2_abort_guest
> and try again. The second time dabt.valid is set so we end up calling
> try_handle_mmio() as usual.

With the approach below, you will also end up to call 
p2m_resolve_translation_fault() and try_map_mmio() a second time if 
try_handle_mmio() fails.

> 
> Just for clarity let me copy/paste the relevant code, apologies if it
> was already obvious to you -- I got the impression my suggestion wasn't
> very clear.
> 
> 
> 
> +again:
> +        if ( is_data && hsr.dabt.valid )
>          {
>              enum io_state state = try_handle_mmio(regs, hsr, gpa);
> 
>              switch ( state )
>              {
>              case IO_ABORT:
>                  goto inject_abt;
>              case IO_HANDLED:
>                  advance_pc(regs, hsr);
>                  return;
>              case IO_RETRY:
>                  /* finish later */
>                  return;
>              case IO_UNHANDLED:
>                  /* IO unhandled, try another way to handle it. */
>                  break;
>              }
>          }
> 
>          /*
>           * First check if the translation fault can be resolved by the
>           * P2M subsystem. If that's the case nothing else to do.
>           */
>          if ( p2m_resolve_translation_fault(current->domain,
>                                             gaddr_to_gfn(gpa)) )
>              return;
> 
>          if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
>              return;
> 
> +        if ( !hsr.dabt.valid )
One more thing I noticed, a stage 2 fault can also happen on an access 
made for a stage 1 translation walk. In this case, I think we don't want 
to decode the instruction.

So this would need to be !hsr.dabt.valid && !hsr.dabt.s1ptw. Depending 
on which patch we go with, this would also need to be adjusted in the 
other one as well.

Cheers,

-- 
Julien Grall

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Ayan Kumar Halder]

Hi Julien/Stefano,

Good discussion to learn about Xen (from a newbie's perspective). :)

I am trying to clarify my understanding. Some queries as below :-

On 28/01/2022 09:46, Julien Grall wrote:
>
>
> On 28/01/2022 01:20, Stefano Stabellini wrote:
>> On Thu, 27 Jan 2022, Julien Grall wrote:
>>> On Thu, 27 Jan 2022 at 23:05, Julien Grall 
>>> <julien.grall.oss@gmail.com> wrote:
>>>>
>>>> On Thu, 27 Jan 2022 at 22:40, Stefano Stabellini 
>>>> <sstabellini@kernel.org> wrote:
>>>>> I am with you on both points.
>>>>>
>>>>> One thing I noticed is that the code today is not able to deal with
>>>>> IO_UNHANDLED for MMIO regions handled by IOREQ servers or Xen MMIO
>>>>> emulator handlers. p2m_resolve_translation_fault and try_map_mmio are
>>>>> called after try_handle_mmio returns IO_UNHANDLED but 
>>>>> try_handle_mmio is
>>>>> not called a second time (or am I mistaken?)
>>>>
>>>> Why would you need it? If try_mmio_fault() doesn't work the first 
>>>> time, then
>>>
>>> Sorry I meant try_handle_mmio().
>>>
>>>> it will not work the second it.
>>
>> I think I explained myself badly, I'll try again below.
>>
>>
>>>>> Another thing I noticed is that currently find_mmio_handler and
>>>>> try_fwd_ioserv expect dabt to be already populated and valid so it 
>>>>> would
>>>>> be better if we could get there only when dabt.valid.
>>>>>
>>>>> With these two things in mind, I think maybe the best thing to do 
>>>>> is to
>>>>> change the code in do_trap_stage2_abort_guest slightly so that
>>>>> p2m_resolve_translation_fault and try_map_mmio are called first when
>>>>> !dabt.valid.
>>>>
>>>> An abort will mostly likely happen because of emulated I/O. If we call
>>>> p2m_resolve_translation_fault() and try_map_mmio() first, then it 
>>>> means
>>>> the processing will take longer than necessary for the common case.
>>>>
>>>> So I think we want to keep the order as it is. I.e first trying the 
>>>> MMIO
>>>> and then falling back to the less likely reason for a trap.
>>
>> Yeah I thought about it as well. The idea would be that if dabt.valid is
>> set then we leave things as they are (we call try_handle_mmio first) but
>> if dabt.valid is not set (it is not valid) then we skip the
>> try_handle_mmio() call because it wouldn't succeed anyway and go
>> directly to p2m_resolve_translation_fault() and try_map_mmio().
>>
>> If either of them work (also reading what you wrote about it) then we
>> return immediately.
>
> Ok. So the assumption is data abort with invalid syndrome would mostly 
> likely be because of a fault handled by p2m_resolve_translation_fault().
>
> I think this makes sense. However, I am not convinced we can currently 
> safely call try_map_mmio() before try_handle_mmio(). This is because 
> the logic in try_map_mmio() is quite fragile and we may mistakenly map 
> an emulated region.

By emulated region, you mean vgic.dbase (Refer 
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/vgic-v2.c;h=589c033eda8f5e11af33c868eae2c159f985eac9;hb=0bdc43c8dec993258e930b34855853c22b917519#l702, 
which has not been mapped to the guest) and thus requires an MMIO handler.

Is my understanding correcr ?

If so, can Xen mantain a table of such emulated regions ? I am guessing 
that all emulated regions will have a mmio_handler. Then, before 
invoking try_map_mmio(), it can check the table.

>
> Similarly, we can't call try_map_mmio() before 
> p2m_resolve_translation_fault() because a transient fault may be
> misinterpreted.
>
> I think we may be able to harden try_map_mmio() by checking if the I/O 
> region is emulated. But this will need to be fully thought through first.
>
>>
>> If not, then we call decode_instruction from do_trap_stage2_abort_guest
>> and try again. The second time dabt.valid is set so we end up calling
>> try_handle_mmio() as usual.
>
> With the approach below, you will also end up to call 
> p2m_resolve_translation_fault() and try_map_mmio() a second time if 
> try_handle_mmio() fails.
>
>>
>> Just for clarity let me copy/paste the relevant code, apologies if it
>> was already obvious to you -- I got the impression my suggestion wasn't
>> very clear.
>>
>>
>>
>> +again:
>> +        if ( is_data && hsr.dabt.valid )
>>          {
>>              enum io_state state = try_handle_mmio(regs, hsr, gpa);
>>
>>              switch ( state )
>>              {
>>              case IO_ABORT:
>>                  goto inject_abt;
>>              case IO_HANDLED:
>>                  advance_pc(regs, hsr);
>>                  return;
>>              case IO_RETRY:
>>                  /* finish later */
>>                  return;
>>              case IO_UNHANDLED:
>>                  /* IO unhandled, try another way to handle it. */
>>                  break;
>>              }
>>          }
>>
>>          /*
>>           * First check if the translation fault can be resolved by the
>>           * P2M subsystem. If that's the case nothing else to do.
>>           */
>>          if ( p2m_resolve_translation_fault(current->domain,
>>                                             gaddr_to_gfn(gpa)) )
>>              return;
>>
>>          if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
>>              return;
>>
>> +        if ( !hsr.dabt.valid )
> One more thing I noticed, a stage 2 fault can also happen on an access 
> made for a stage 1 translation walk. In this case, I think we don't 
> want to decode the instruction.
>
> So this would need to be !hsr.dabt.valid && !hsr.dabt.s1ptw. Depending 
> on which patch we go with, this would also need to be adjusted in the 
> other one as well.

This triggered me to check for the remaining bits as well. Refer DDI 
0487G.b Armv8 Arm, "ISS encoding for an exception from a Data Abort", 
Page D13-3219

I guess we need to check the following :-

1. !hsr.dabt.valid

2. !hsr.dabt.s1ptw - Abort may be due to stage 1 translation table walk

3. !hsr.dabt.cache - Abort is due to cache maintenance or address 
translation instructions. We do not decode these instructions.

4. !hsr.dabt.eat - Abort is external


There is no need to check the following due to the reasons mentioned :-

1. hsr.dabt.dfsc - no need as we have already determined that it is a 
translation fault from EL0/EL1.

2. hsr.dabt.write - no need as the fault can be caused due to both read 
or write

3. hsr.dabt.fnv - no use for this in instruction decoding

4. hsr.dabt.sbzp0 - Bits[12:11] - We know that DFSC cannot be 0b010000 
(FEAT_RAS), We may not check for FEAT_LS64 as from the instruction 
opcode, we can make out that it is not ST64BV, LD64B, ST64B or ST64BV0

                          Bit[13] - VCNR - The instruction opcode will 
tell us that it is not MRS/MSR instruction.

Please let me know if my understanding is correct. I can send out a v5 
patch "Re: [XEN v4] xen/arm64: io: Decode ldr/str post-indexing 
instructions" will additional checks in place.

- Ayan

>
> Cheers,
>

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 8330 bytes --]

On Fri, 28 Jan 2022, Ayan Kumar Halder wrote:
> Hi Julien/Stefano,
> 
> Good discussion to learn about Xen (from a newbie's perspective). :)
> 
> I am trying to clarify my understanding. Some queries as below :-
> 
> On 28/01/2022 09:46, Julien Grall wrote:
> > 
> > 
> > On 28/01/2022 01:20, Stefano Stabellini wrote:
> > > On Thu, 27 Jan 2022, Julien Grall wrote:
> > > > On Thu, 27 Jan 2022 at 23:05, Julien Grall <julien.grall.oss@gmail.com>
> > > > wrote:
> > > > > 
> > > > > On Thu, 27 Jan 2022 at 22:40, Stefano Stabellini
> > > > > <sstabellini@kernel.org> wrote:
> > > > > > I am with you on both points.
> > > > > > 
> > > > > > One thing I noticed is that the code today is not able to deal with
> > > > > > IO_UNHANDLED for MMIO regions handled by IOREQ servers or Xen MMIO
> > > > > > emulator handlers. p2m_resolve_translation_fault and try_map_mmio
> > > > > > are
> > > > > > called after try_handle_mmio returns IO_UNHANDLED but
> > > > > > try_handle_mmio is
> > > > > > not called a second time (or am I mistaken?)
> > > > > 
> > > > > Why would you need it? If try_mmio_fault() doesn't work the first
> > > > > time, then
> > > > 
> > > > Sorry I meant try_handle_mmio().
> > > > 
> > > > > it will not work the second it.
> > > 
> > > I think I explained myself badly, I'll try again below.
> > > 
> > > 
> > > > > > Another thing I noticed is that currently find_mmio_handler and
> > > > > > try_fwd_ioserv expect dabt to be already populated and valid so it
> > > > > > would
> > > > > > be better if we could get there only when dabt.valid.
> > > > > > 
> > > > > > With these two things in mind, I think maybe the best thing to do is
> > > > > > to
> > > > > > change the code in do_trap_stage2_abort_guest slightly so that
> > > > > > p2m_resolve_translation_fault and try_map_mmio are called first when
> > > > > > !dabt.valid.
> > > > > 
> > > > > An abort will mostly likely happen because of emulated I/O. If we call
> > > > > p2m_resolve_translation_fault() and try_map_mmio() first, then it
> > > > > means
> > > > > the processing will take longer than necessary for the common case.
> > > > > 
> > > > > So I think we want to keep the order as it is. I.e first trying the
> > > > > MMIO
> > > > > and then falling back to the less likely reason for a trap.
> > > 
> > > Yeah I thought about it as well. The idea would be that if dabt.valid is
> > > set then we leave things as they are (we call try_handle_mmio first) but
> > > if dabt.valid is not set (it is not valid) then we skip the
> > > try_handle_mmio() call because it wouldn't succeed anyway and go
> > > directly to p2m_resolve_translation_fault() and try_map_mmio().
> > > 
> > > If either of them work (also reading what you wrote about it) then we
> > > return immediately.
> > 
> > Ok. So the assumption is data abort with invalid syndrome would mostly
> > likely be because of a fault handled by p2m_resolve_translation_fault().
> > 
> > I think this makes sense. However, I am not convinced we can currently
> > safely call try_map_mmio() before try_handle_mmio(). This is because the
> > logic in try_map_mmio() is quite fragile and we may mistakenly map an
> > emulated region.
> 
> By emulated region, you mean vgic.dbase (Refer
> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/vgic-v2.c;h=589c033eda8f5e11af33c868eae2c159f985eac9;hb=0bdc43c8dec993258e930b34855853c22b917519#l702,
> which has not been mapped to the guest) and thus requires an MMIO handler.
> 
> Is my understanding correcr ?
 
I'll try to answer for Julien but yes.


> If so, can Xen mantain a table of such emulated regions ? I am guessing that
> all emulated regions will have a mmio_handler. Then, before invoking
> try_map_mmio(), it can check the table.

Today we keep those as a list, see find_mmio_handler (for regions
emulated in Xen) and also ioreq_server_select (for regions emulated by
QEMU or other external emulators.)

But I think there might be a simpler way: if you look at try_map_mmio,
you'll notice that there is iomem_access_permitted check. I don't think
that check can succeed for an emulated region. (I'd love for Julien and
others to confirm this observation though.)

 
> > Similarly, we can't call try_map_mmio() before
> > p2m_resolve_translation_fault() because a transient fault may be
> > misinterpreted.
> > 
> > I think we may be able to harden try_map_mmio() by checking if the I/O
> > region is emulated. But this will need to be fully thought through first.
> > 
> > > 
> > > If not, then we call decode_instruction from do_trap_stage2_abort_guest
> > > and try again. The second time dabt.valid is set so we end up calling
> > > try_handle_mmio() as usual.
> > 
> > With the approach below, you will also end up to call
> > p2m_resolve_translation_fault() and try_map_mmio() a second time if
> > try_handle_mmio() fails.
> > 
> > > 
> > > Just for clarity let me copy/paste the relevant code, apologies if it
> > > was already obvious to you -- I got the impression my suggestion wasn't
> > > very clear.
> > > 
> > > 
> > > 
> > > +again:
> > > +        if ( is_data && hsr.dabt.valid )
> > >          {
> > >              enum io_state state = try_handle_mmio(regs, hsr, gpa);
> > > 
> > >              switch ( state )
> > >              {
> > >              case IO_ABORT:
> > >                  goto inject_abt;
> > >              case IO_HANDLED:
> > >                  advance_pc(regs, hsr);
> > >                  return;
> > >              case IO_RETRY:
> > >                  /* finish later */
> > >                  return;
> > >              case IO_UNHANDLED:
> > >                  /* IO unhandled, try another way to handle it. */
> > >                  break;
> > >              }
> > >          }
> > > 
> > >          /*
> > >           * First check if the translation fault can be resolved by the
> > >           * P2M subsystem. If that's the case nothing else to do.
> > >           */
> > >          if ( p2m_resolve_translation_fault(current->domain,
> > >                                             gaddr_to_gfn(gpa)) )
> > >              return;
> > > 
> > >          if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
> > >              return;
> > > 
> > > +        if ( !hsr.dabt.valid )
> > One more thing I noticed, a stage 2 fault can also happen on an access made
> > for a stage 1 translation walk. In this case, I think we don't want to
> > decode the instruction.
> > 
> > So this would need to be !hsr.dabt.valid && !hsr.dabt.s1ptw. Depending on
> > which patch we go with, this would also need to be adjusted in the other one
> > as well.
> 
> This triggered me to check for the remaining bits as well. Refer DDI 0487G.b
> Armv8 Arm, "ISS encoding for an exception from a Data Abort", Page D13-3219
> 
> I guess we need to check the following :-
> 
> 1. !hsr.dabt.valid
> 
> 2. !hsr.dabt.s1ptw - Abort may be due to stage 1 translation table walk
> 
> 3. !hsr.dabt.cache - Abort is due to cache maintenance or address translation
> instructions. We do not decode these instructions.
> 
> 4. !hsr.dabt.eat - Abort is external

Yes, makes sense to me

 
> There is no need to check the following due to the reasons mentioned :-
> 
> 1. hsr.dabt.dfsc - no need as we have already determined that it is a
> translation fault from EL0/EL1.
> 
> 2. hsr.dabt.write - no need as the fault can be caused due to both read or
> write
> 
> 3. hsr.dabt.fnv - no use for this in instruction decoding

These also makes sense to me


> 4. hsr.dabt.sbzp0 - Bits[12:11] - We know that DFSC cannot be 0b010000
> (FEAT_RAS), We may not check for FEAT_LS64 as from the instruction opcode, we
> can make out that it is not ST64BV, LD64B, ST64B or ST64BV0
> 
>                          Bit[13] - VCNR - The instruction opcode will tell us
> that it is not MRS/MSR instruction.

Yeah this check could be useful in the future but it would be redundant
at the moment. I am fine either way, I'll let other comment.

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Julien Grall]

Hi,

Replying to Ayan's e-mail at the same time.

On 28/01/2022 20:30, Stefano Stabellini wrote:
> On Fri, 28 Jan 2022, Ayan Kumar Halder wrote:
>> Hi Julien/Stefano,
>>
>> Good discussion to learn about Xen (from a newbie's perspective). :)
>>
>> I am trying to clarify my understanding. Some queries as below :-
>>
>> On 28/01/2022 09:46, Julien Grall wrote:
>>>
>>>
>>> On 28/01/2022 01:20, Stefano Stabellini wrote:
>>>> On Thu, 27 Jan 2022, Julien Grall wrote:
>>>>> On Thu, 27 Jan 2022 at 23:05, Julien Grall <julien.grall.oss@gmail.com>
>>>>> wrote:
>>>>>>
>>>>>> On Thu, 27 Jan 2022 at 22:40, Stefano Stabellini
>>>>>> <sstabellini@kernel.org> wrote:
>>>>>>> I am with you on both points.
>>>>>>>
>>>>>>> One thing I noticed is that the code today is not able to deal with
>>>>>>> IO_UNHANDLED for MMIO regions handled by IOREQ servers or Xen MMIO
>>>>>>> emulator handlers. p2m_resolve_translation_fault and try_map_mmio
>>>>>>> are
>>>>>>> called after try_handle_mmio returns IO_UNHANDLED but
>>>>>>> try_handle_mmio is
>>>>>>> not called a second time (or am I mistaken?)
>>>>>>
>>>>>> Why would you need it? If try_mmio_fault() doesn't work the first
>>>>>> time, then
>>>>>
>>>>> Sorry I meant try_handle_mmio().
>>>>>
>>>>>> it will not work the second it.
>>>>
>>>> I think I explained myself badly, I'll try again below.
>>>>
>>>>
>>>>>>> Another thing I noticed is that currently find_mmio_handler and
>>>>>>> try_fwd_ioserv expect dabt to be already populated and valid so it
>>>>>>> would
>>>>>>> be better if we could get there only when dabt.valid.
>>>>>>>
>>>>>>> With these two things in mind, I think maybe the best thing to do is
>>>>>>> to
>>>>>>> change the code in do_trap_stage2_abort_guest slightly so that
>>>>>>> p2m_resolve_translation_fault and try_map_mmio are called first when
>>>>>>> !dabt.valid.
>>>>>>
>>>>>> An abort will mostly likely happen because of emulated I/O. If we call
>>>>>> p2m_resolve_translation_fault() and try_map_mmio() first, then it
>>>>>> means
>>>>>> the processing will take longer than necessary for the common case.
>>>>>>
>>>>>> So I think we want to keep the order as it is. I.e first trying the
>>>>>> MMIO
>>>>>> and then falling back to the less likely reason for a trap.
>>>>
>>>> Yeah I thought about it as well. The idea would be that if dabt.valid is
>>>> set then we leave things as they are (we call try_handle_mmio first) but
>>>> if dabt.valid is not set (it is not valid) then we skip the
>>>> try_handle_mmio() call because it wouldn't succeed anyway and go
>>>> directly to p2m_resolve_translation_fault() and try_map_mmio().
>>>>
>>>> If either of them work (also reading what you wrote about it) then we
>>>> return immediately.
>>>
>>> Ok. So the assumption is data abort with invalid syndrome would mostly
>>> likely be because of a fault handled by p2m_resolve_translation_fault().
>>>
>>> I think this makes sense. However, I am not convinced we can currently
>>> safely call try_map_mmio() before try_handle_mmio(). This is because the
>>> logic in try_map_mmio() is quite fragile and we may mistakenly map an
>>> emulated region.
>>
>> By emulated region, you mean vgic.dbase (Refer
>> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/vgic-v2.c;h=589c033eda8f5e11af33c868eae2c159f985eac9;hb=0bdc43c8dec993258e930b34855853c22b917519#l702,
>> which has not been mapped to the guest) and thus requires an MMIO handler.
>>
>> Is my understanding correcr ?
>   
> I'll try to answer for Julien but yes.
> 
> 
>> If so, can Xen mantain a table of such emulated regions ? I am guessing that
>> all emulated regions will have a mmio_handler. Then, before invoking
>> try_map_mmio(), it can check the table.
> 
> Today we keep those as a list, see find_mmio_handler (for regions
> emulated in Xen) and also ioreq_server_select (for regions emulated by
> QEMU or other external emulators.)
> 
> But I think there might be a simpler way: if you look at try_map_mmio,
> you'll notice that there is iomem_access_permitted check. I don't think
> that check can succeed for an emulated region. 

It can. iomem_access_permitted() is telling which host physical frame is 
accessible by the domain. This is different to which guest physical 
address is emulated.

It happens that most (all?) of them are the same today for the hardware 
domain. But that's not something we should rely on.

So I think we want to check that the region will be used for emulated I/O.

You could use find_mmio() but I think ioreq_server_select() is not 
directly suitable to us because we want to check that the full page is 
not emulated (You could technically only emulate part of it).

>>> Similarly, we can't call try_map_mmio() before
>>> p2m_resolve_translation_fault() because a transient fault may be
>>> misinterpreted.
>>>
>>> I think we may be able to harden try_map_mmio() by checking if the I/O
>>> region is emulated. But this will need to be fully thought through first.
>>>
>>>>
>>>> If not, then we call decode_instruction from do_trap_stage2_abort_guest
>>>> and try again. The second time dabt.valid is set so we end up calling
>>>> try_handle_mmio() as usual.
>>>
>>> With the approach below, you will also end up to call
>>> p2m_resolve_translation_fault() and try_map_mmio() a second time if
>>> try_handle_mmio() fails.
>>>
>>>>
>>>> Just for clarity let me copy/paste the relevant code, apologies if it
>>>> was already obvious to you -- I got the impression my suggestion wasn't
>>>> very clear.
>>>>
>>>>
>>>>
>>>> +again:
>>>> +        if ( is_data && hsr.dabt.valid )
>>>>           {
>>>>               enum io_state state = try_handle_mmio(regs, hsr, gpa);
>>>>
>>>>               switch ( state )
>>>>               {
>>>>               case IO_ABORT:
>>>>                   goto inject_abt;
>>>>               case IO_HANDLED:
>>>>                   advance_pc(regs, hsr);
>>>>                   return;
>>>>               case IO_RETRY:
>>>>                   /* finish later */
>>>>                   return;
>>>>               case IO_UNHANDLED:
>>>>                   /* IO unhandled, try another way to handle it. */
>>>>                   break;
>>>>               }
>>>>           }
>>>>
>>>>           /*
>>>>            * First check if the translation fault can be resolved by the
>>>>            * P2M subsystem. If that's the case nothing else to do.
>>>>            */
>>>>           if ( p2m_resolve_translation_fault(current->domain,
>>>>                                              gaddr_to_gfn(gpa)) )
>>>>               return;
>>>>
>>>>           if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
>>>>               return;
>>>>
>>>> +        if ( !hsr.dabt.valid )
>>> One more thing I noticed, a stage 2 fault can also happen on an access made
>>> for a stage 1 translation walk. In this case, I think we don't want to
>>> decode the instruction.
>>>
>>> So this would need to be !hsr.dabt.valid && !hsr.dabt.s1ptw. Depending on
>>> which patch we go with, this would also need to be adjusted in the other one
>>> as well.
>>
>> This triggered me to check for the remaining bits as well. Refer DDI 0487G.b
>> Armv8 Arm, "ISS encoding for an exception from a Data Abort", Page D13-3219
>>
>> I guess we need to check the following :-
>>
>> 1. !hsr.dabt.valid
>>
>> 2. !hsr.dabt.s1ptw - Abort may be due to stage 1 translation table walk
>>
>> 3. !hsr.dabt.cache - Abort is due to cache maintenance or address translation
>> instructions. We do not decode these instructions.

I agree that we want to check hsr.dabt.cache. But they need to be 
ignored rather than sending a data abort to the guest (That's 
technically already an issue today).

>>
>> 4. !hsr.dabt.eat - Abort is external

Reading the description, this bit doesn't tell whether this is an 
external abort. Instead, it seems to provide an implementation defined 
way to categorize an external abort.

In any case, I don't think it is useful to check it because that bit is 
guaranteed to be 0 for non-external abort fault. The DFSC already tells 
you that.

> 
> Yes, makes sense to me
> 
>   
>> There is no need to check the following due to the reasons mentioned :-
>>
>> 1. hsr.dabt.dfsc - no need as we have already determined that it is a
>> translation fault from EL0/EL1.
>>
>> 2. hsr.dabt.write - no need as the fault can be caused due to both read or
>> write
>>
>> 3. hsr.dabt.fnv - no use for this in instruction decoding
> 
> These also makes sense to me
> 
> 
>> 4. hsr.dabt.sbzp0 - Bits[12:11] - We know that DFSC cannot be 0b010000
>> (FEAT_RAS), We may not check for FEAT_LS64 as from the instruction opcode, we
>> can make out that it is not ST64BV, LD64B, ST64B or ST64BV0
>>
>>                           Bit[13] - VCNR - The instruction opcode will tell us
>> that it is not MRS/MSR instruction.

The key point of bit[13] is we don't support nested virt on Xen on Arm.

> 
> Yeah this check could be useful in the future but it would be redundant
> at the moment. I am fine either way, I'll let other comment.

Cheers,

-- 
Julien Grall

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Stefano Stabellini]

On Fri, 28 Jan 2022, Julien Grall wrote:
> On 28/01/2022 01:20, Stefano Stabellini wrote:
> > On Thu, 27 Jan 2022, Julien Grall wrote:
> > > On Thu, 27 Jan 2022 at 23:05, Julien Grall <julien.grall.oss@gmail.com>
> > > wrote:
> > > > 
> > > > On Thu, 27 Jan 2022 at 22:40, Stefano Stabellini
> > > > <sstabellini@kernel.org> wrote:
> > > > > I am with you on both points.
> > > > > 
> > > > > One thing I noticed is that the code today is not able to deal with
> > > > > IO_UNHANDLED for MMIO regions handled by IOREQ servers or Xen MMIO
> > > > > emulator handlers. p2m_resolve_translation_fault and try_map_mmio are
> > > > > called after try_handle_mmio returns IO_UNHANDLED but try_handle_mmio
> > > > > is
> > > > > not called a second time (or am I mistaken?)
> > > > 
> > > > Why would you need it? If try_mmio_fault() doesn't work the first time,
> > > > then
> > > 
> > > Sorry I meant try_handle_mmio().
> > > 
> > > > it will not work the second it.
> > 
> > I think I explained myself badly, I'll try again below.
> > 
> > 
> > > > > Another thing I noticed is that currently find_mmio_handler and
> > > > > try_fwd_ioserv expect dabt to be already populated and valid so it
> > > > > would
> > > > > be better if we could get there only when dabt.valid.
> > > > > 
> > > > > With these two things in mind, I think maybe the best thing to do is
> > > > > to
> > > > > change the code in do_trap_stage2_abort_guest slightly so that
> > > > > p2m_resolve_translation_fault and try_map_mmio are called first when
> > > > > !dabt.valid.
> > > > 
> > > > An abort will mostly likely happen because of emulated I/O. If we call
> > > > p2m_resolve_translation_fault() and try_map_mmio() first, then it means
> > > > the processing will take longer than necessary for the common case.
> > > > 
> > > > So I think we want to keep the order as it is. I.e first trying the MMIO
> > > > and then falling back to the less likely reason for a trap.
> > 
> > Yeah I thought about it as well. The idea would be that if dabt.valid is
> > set then we leave things as they are (we call try_handle_mmio first) but
> > if dabt.valid is not set (it is not valid) then we skip the
> > try_handle_mmio() call because it wouldn't succeed anyway and go
> > directly to p2m_resolve_translation_fault() and try_map_mmio().
> > 
> > If either of them work (also reading what you wrote about it) then we
> > return immediately.
> 
> Ok. So the assumption is data abort with invalid syndrome would mostly likely
> be because of a fault handled by p2m_resolve_translation_fault().
> 
> I think this makes sense. However, I am not convinced we can currently safely
> call try_map_mmio() before try_handle_mmio(). This is because the logic in
> try_map_mmio() is quite fragile and we may mistakenly map an emulated region.
> 
> Similarly, we can't call try_map_mmio() before p2m_resolve_translation_fault()
> because a transient fault may be
> misinterpreted.
> 
> I think we may be able to harden try_map_mmio() by checking if the I/O region
> is emulated. But this will need to be fully thought through first.

That's a good point. I wonder if it could be as simple as making sure
that iomem_access_permitted returns false for all emulated regions?
Looking at the code, it looks like it is already the case today. Is that
right?


> > If not, then we call decode_instruction from do_trap_stage2_abort_guest
> > and try again. The second time dabt.valid is set so we end up calling
> > try_handle_mmio() as usual.
> 
> With the approach below, you will also end up to call
> p2m_resolve_translation_fault() and try_map_mmio() a second time if
> try_handle_mmio() fails.
 
Yeah... we can find a way to avoid it.

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Julien Grall]

Hi Stefano,

On 28/01/2022 20:23, Stefano Stabellini wrote:
> On Fri, 28 Jan 2022, Julien Grall wrote:
>> On 28/01/2022 01:20, Stefano Stabellini wrote:
>>> On Thu, 27 Jan 2022, Julien Grall wrote:
>>>> On Thu, 27 Jan 2022 at 23:05, Julien Grall <julien.grall.oss@gmail.com>
>>>> wrote:
>>>>>
>>>>> On Thu, 27 Jan 2022 at 22:40, Stefano Stabellini
>>>>> <sstabellini@kernel.org> wrote:
>>>>>> I am with you on both points.
>>>>>>
>>>>>> One thing I noticed is that the code today is not able to deal with
>>>>>> IO_UNHANDLED for MMIO regions handled by IOREQ servers or Xen MMIO
>>>>>> emulator handlers. p2m_resolve_translation_fault and try_map_mmio are
>>>>>> called after try_handle_mmio returns IO_UNHANDLED but try_handle_mmio
>>>>>> is
>>>>>> not called a second time (or am I mistaken?)
>>>>>
>>>>> Why would you need it? If try_mmio_fault() doesn't work the first time,
>>>>> then
>>>>
>>>> Sorry I meant try_handle_mmio().
>>>>
>>>>> it will not work the second it.
>>>
>>> I think I explained myself badly, I'll try again below.
>>>
>>>
>>>>>> Another thing I noticed is that currently find_mmio_handler and
>>>>>> try_fwd_ioserv expect dabt to be already populated and valid so it
>>>>>> would
>>>>>> be better if we could get there only when dabt.valid.
>>>>>>
>>>>>> With these two things in mind, I think maybe the best thing to do is
>>>>>> to
>>>>>> change the code in do_trap_stage2_abort_guest slightly so that
>>>>>> p2m_resolve_translation_fault and try_map_mmio are called first when
>>>>>> !dabt.valid.
>>>>>
>>>>> An abort will mostly likely happen because of emulated I/O. If we call
>>>>> p2m_resolve_translation_fault() and try_map_mmio() first, then it means
>>>>> the processing will take longer than necessary for the common case.
>>>>>
>>>>> So I think we want to keep the order as it is. I.e first trying the MMIO
>>>>> and then falling back to the less likely reason for a trap.
>>>
>>> Yeah I thought about it as well. The idea would be that if dabt.valid is
>>> set then we leave things as they are (we call try_handle_mmio first) but
>>> if dabt.valid is not set (it is not valid) then we skip the
>>> try_handle_mmio() call because it wouldn't succeed anyway and go
>>> directly to p2m_resolve_translation_fault() and try_map_mmio().
>>>
>>> If either of them work (also reading what you wrote about it) then we
>>> return immediately.
>>
>> Ok. So the assumption is data abort with invalid syndrome would mostly likely
>> be because of a fault handled by p2m_resolve_translation_fault().
>>
>> I think this makes sense. However, I am not convinced we can currently safely
>> call try_map_mmio() before try_handle_mmio(). This is because the logic in
>> try_map_mmio() is quite fragile and we may mistakenly map an emulated region.
>>
>> Similarly, we can't call try_map_mmio() before p2m_resolve_translation_fault()
>> because a transient fault may be
>> misinterpreted.
>>
>> I think we may be able to harden try_map_mmio() by checking if the I/O region
>> is emulated. But this will need to be fully thought through first.
> 
> That's a good point. I wonder if it could be as simple as making sure
> that iomem_access_permitted returns false for all emulated regions?

I have replied to that in the other thread. The short answer is no and...

> Looking at the code, it looks like it is already the case today. Is that
> right?

not 100%. The thing is iomem_access_permitted() is telling you which 
*host* physical address is accessible. Not which *guest* physical 
address is emulated.

We could possibly take some short cut at the risk of bitting back in the 
future if we end up to emulate non-existing region in the host physical 
address.

Cheers,

-- 
Julien Grall

Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Ayan Kumar Halder]

Hi Stefano/Julien,

On 29/01/2022 17:40, Julien Grall wrote:
> Hi Stefano,
>
> On 28/01/2022 20:23, Stefano Stabellini wrote:
>> On Fri, 28 Jan 2022, Julien Grall wrote:
>>> On 28/01/2022 01:20, Stefano Stabellini wrote:
>>>> On Thu, 27 Jan 2022, Julien Grall wrote:
>>>>> On Thu, 27 Jan 2022 at 23:05, Julien Grall 
>>>>> <julien.grall.oss@gmail.com>
>>>>> wrote:
>>>>>>
>>>>>> On Thu, 27 Jan 2022 at 22:40, Stefano Stabellini
>>>>>> <sstabellini@kernel.org> wrote:
>>>>>>> I am with you on both points.
>>>>>>>
>>>>>>> One thing I noticed is that the code today is not able to deal with
>>>>>>> IO_UNHANDLED for MMIO regions handled by IOREQ servers or Xen MMIO
>>>>>>> emulator handlers. p2m_resolve_translation_fault and 
>>>>>>> try_map_mmio are
>>>>>>> called after try_handle_mmio returns IO_UNHANDLED but 
>>>>>>> try_handle_mmio
>>>>>>> is
>>>>>>> not called a second time (or am I mistaken?)
>>>>>>
>>>>>> Why would you need it? If try_mmio_fault() doesn't work the first 
>>>>>> time,
>>>>>> then
>>>>>
>>>>> Sorry I meant try_handle_mmio().
>>>>>
>>>>>> it will not work the second it.
>>>>
>>>> I think I explained myself badly, I'll try again below.
>>>>
>>>>
>>>>>>> Another thing I noticed is that currently find_mmio_handler and
>>>>>>> try_fwd_ioserv expect dabt to be already populated and valid so it
>>>>>>> would
>>>>>>> be better if we could get there only when dabt.valid.
>>>>>>>
>>>>>>> With these two things in mind, I think maybe the best thing to 
>>>>>>> do is
>>>>>>> to
>>>>>>> change the code in do_trap_stage2_abort_guest slightly so that
>>>>>>> p2m_resolve_translation_fault and try_map_mmio are called first 
>>>>>>> when
>>>>>>> !dabt.valid.
>>>>>>
>>>>>> An abort will mostly likely happen because of emulated I/O. If we 
>>>>>> call
>>>>>> p2m_resolve_translation_fault() and try_map_mmio() first, then it 
>>>>>> means
>>>>>> the processing will take longer than necessary for the common case.
>>>>>>
>>>>>> So I think we want to keep the order as it is. I.e first trying 
>>>>>> the MMIO
>>>>>> and then falling back to the less likely reason for a trap.
>>>>
>>>> Yeah I thought about it as well. The idea would be that if 
>>>> dabt.valid is
>>>> set then we leave things as they are (we call try_handle_mmio 
>>>> first) but
>>>> if dabt.valid is not set (it is not valid) then we skip the
>>>> try_handle_mmio() call because it wouldn't succeed anyway and go
>>>> directly to p2m_resolve_translation_fault() and try_map_mmio().
>>>>
>>>> If either of them work (also reading what you wrote about it) then we
>>>> return immediately.
>>>
>>> Ok. So the assumption is data abort with invalid syndrome would 
>>> mostly likely
>>> be because of a fault handled by p2m_resolve_translation_fault().
>>>
>>> I think this makes sense. However, I am not convinced we can 
>>> currently safely
>>> call try_map_mmio() before try_handle_mmio(). This is because the 
>>> logic in
>>> try_map_mmio() is quite fragile and we may mistakenly map an 
>>> emulated region.
>>>
>>> Similarly, we can't call try_map_mmio() before 
>>> p2m_resolve_translation_fault()
>>> because a transient fault may be
>>> misinterpreted.
>>>
>>> I think we may be able to harden try_map_mmio() by checking if the 
>>> I/O region
>>> is emulated. But this will need to be fully thought through first.
>>
>> That's a good point. I wonder if it could be as simple as making sure
>> that iomem_access_permitted returns false for all emulated regions?
>
> I have replied to that in the other thread. The short answer is no and...
>
>> Looking at the code, it looks like it is already the case today. Is that
>> right?
>
> not 100%. The thing is iomem_access_permitted() is telling you which 
> *host* physical address is accessible. Not which *guest* physical 
> address is emulated.
>
> We could possibly take some short cut at the risk of bitting back in 
> the future if we end up to emulate non-existing region in the host 
> physical address.

I have sent out a "[XEN v5] xen/arm64: io: Decode ldr/str post-indexing 
instructions" addressing some of the comments based on the discussion. 
As for this patch, we agreed that this is incorrect. Thus, there will be 
no v2 patch for this.

- Ayan

>
> Cheers,
>