[PATCH v2] hvf: arm: Ignore cache operations on MMIO

[PATCH v2] hvf: arm: Ignore cache operations on MMIO

[Alexander Graf]

Apple's Hypervisor.Framework forwards cache operations as MMIO traps
into user space. For MMIO however, these have no meaning: There is no
cache attached to them.

So let's just treat cache data exits as nops.

This fixes OpenBSD booting as guest.

Signed-off-by: Alexander Graf <agraf@csgraf.de>
Reported-by: AJ Barris <AwlsomeAlex@github.com>
Reference: https://github.com/utmapp/UTM/issues/3197
---
 target/arm/hvf/hvf.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index bff3e0cde7..0dc96560d3 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -1150,12 +1150,19 @@ int hvf_vcpu_exec(CPUState *cpu)
         uint32_t sas = (syndrome >> 22) & 3;
         uint32_t len = 1 << sas;
         uint32_t srt = (syndrome >> 16) & 0x1f;
+        uint32_t cm = (syndrome >> 8) & 0x1;
         uint64_t val = 0;
 
         trace_hvf_data_abort(env->pc, hvf_exit->exception.virtual_address,
                              hvf_exit->exception.physical_address, isv,
                              iswrite, s1ptw, len, srt);
 
+        if (cm) {
+            /* We don't cache MMIO regions */
+            advance_pc = true;
+            break;
+        }
+
         assert(isv);
 
         if (iswrite) {
-- 
2.30.1 (Apple Git-130)

Re: [PATCH v2] hvf: arm: Ignore cache operations on MMIO

[Philippe Mathieu-Daudé]

On 10/26/21 09:12, Alexander Graf wrote:
> Apple's Hypervisor.Framework forwards cache operations as MMIO traps
> into user space. For MMIO however, these have no meaning: There is no
> cache attached to them.
> 
> So let's just treat cache data exits as nops.
> 
> This fixes OpenBSD booting as guest.
> 
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> Reported-by: AJ Barris <AwlsomeAlex@github.com>
> Reference: https://github.com/utmapp/UTM/issues/3197
> ---
>  target/arm/hvf/hvf.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index bff3e0cde7..0dc96560d3 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -1150,12 +1150,19 @@ int hvf_vcpu_exec(CPUState *cpu)
>          uint32_t sas = (syndrome >> 22) & 3;
>          uint32_t len = 1 << sas;
>          uint32_t srt = (syndrome >> 16) & 0x1f;
> +        uint32_t cm = (syndrome >> 8) & 0x1;
>          uint64_t val = 0;
>  
>          trace_hvf_data_abort(env->pc, hvf_exit->exception.virtual_address,
>                               hvf_exit->exception.physical_address, isv,
>                               iswrite, s1ptw, len, srt);
>  
> +        if (cm) {
> +            /* We don't cache MMIO regions */
> +            advance_pc = true;
> +            break;
> +        }
> +
>          assert(isv);

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2] hvf: arm: Ignore cache operations on MMIO

[Mark Kettenis]

> From: Alexander Graf <agraf@csgraf.de>
> Date: Tue, 26 Oct 2021 09:12:41 +0200
> 
> Apple's Hypervisor.Framework forwards cache operations as MMIO traps
> into user space. For MMIO however, these have no meaning: There is no
> cache attached to them.
> 
> So let's just treat cache data exits as nops.
> 
> This fixes OpenBSD booting as guest.
> 
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> Reported-by: AJ Barris <AwlsomeAlex@github.com>
> Reference: https://github.com/utmapp/UTM/issues/3197
> ---
>  target/arm/hvf/hvf.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index bff3e0cde7..0dc96560d3 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -1150,12 +1150,19 @@ int hvf_vcpu_exec(CPUState *cpu)
>          uint32_t sas = (syndrome >> 22) & 3;
>          uint32_t len = 1 << sas;
>          uint32_t srt = (syndrome >> 16) & 0x1f;
> +        uint32_t cm = (syndrome >> 8) & 0x1;
>          uint64_t val = 0;
>  
>          trace_hvf_data_abort(env->pc, hvf_exit->exception.virtual_address,
>                               hvf_exit->exception.physical_address, isv,
>                               iswrite, s1ptw, len, srt);
>  
> +        if (cm) {
> +            /* We don't cache MMIO regions */
> +            advance_pc = true;
> +            break;
> +        }
> +
>          assert(isv);
>  
>          if (iswrite) {
> -- 
> 2.30.1 (Apple Git-130)

Now that I see this diff, I think this is pretty much how I fixed it
when first trying your hvf patchset back in december/january.  Before
I lost the change because I accidentally wiped the disk...

Reviewed-by: Mark Kettenis <kettenis@openbsd.org>

Re: [PATCH v2] hvf: arm: Ignore cache operations on MMIO

[Richard Henderson]

On 10/26/21 12:12 AM, Alexander Graf wrote:
> +        if (cm) {
> +            /* We don't cache MMIO regions */
> +            advance_pc = true;
> +            break;
> +        }
> +
>           assert(isv);

The assert should come first.  If the "iss valid" bit is not set, then nothing else in the 
word is defined.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2] hvf: arm: Ignore cache operations on MMIO

[Alexander Graf]

> Am 26.10.2021 um 18:10 schrieb Richard Henderson <richard.henderson@linaro.org>:
> 
> On 10/26/21 12:12 AM, Alexander Graf wrote:
>> +        if (cm) {
>> +            /* We don't cache MMIO regions */
>> +            advance_pc = true;
>> +            break;
>> +        }
>> +
>>          assert(isv);
> 
> The assert should come first.  If the "iss valid" bit is not set, then nothing else in the word is defined.
> 
> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Yes, but isv=0 for cm=1. And even in other isv=0 situations most other fields are valid (post-idx provides the correct va/pa too for example).

Does cm=1 really give you isv=1 on other hardware?

Alex

Re: [PATCH v2] hvf: arm: Ignore cache operations on MMIO

[Richard Henderson]

On 10/26/21 9:38 AM, Alexander Graf wrote:
>> On 10/26/21 12:12 AM, Alexander Graf wrote:
>>> +        if (cm) {
>>> +            /* We don't cache MMIO regions */
>>> +            advance_pc = true;
>>> +            break;
>>> +        }
>>> +
>>>           assert(isv);
>>
>> The assert should come first.  If the "iss valid" bit is not set, then nothing else in the word is defined.
>>
>> Otherwise,
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> Yes, but isv=0 for cm=1. And even in other isv=0 situations most other fields are valid (post-idx provides the correct va/pa too for example).
> 
> Does cm=1 really give you isv=1 on other hardware?

Ah hah.  From 0487G.a, page D13-3191:

# For other faults reported in ESR_EL2, ISV is 0 except
# for the following stage 2 aborts...

(which incidentally sounds like documenting around a historic chip bug, since both EL1 and 
EL3 do get ISV set).  And of course HVF would be passing along the EL2 version, being the 
hypervisor.

I guess the assert can stand where it is, because everything below should be one of those 
"following stage 2 aborts".   The only thing that could be improved is a bit of commentary 
there at the assert.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2] hvf: arm: Ignore cache operations on MMIO

[Peter Maydell]

On Tue, 26 Oct 2021 at 18:46, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Ah hah.  From 0487G.a, page D13-3191:
>
> # For other faults reported in ESR_EL2, ISV is 0 except
> # for the following stage 2 aborts...
>
> (which incidentally sounds like documenting around a historic chip bug, since both EL1 and
> EL3 do get ISV set).

Nope, you have that the wrong way around: EL1 and EL3 get ISV=0 for
almost all cases -- only the FEAT_LS64 ST64BV/ST64BV0/ST64B/LD64B insns
cause a fault with ISV=1. For EL2, in addition to the FEAT_LS64 stuff,
you also get ISV=1 for the loads and stores which are emulatable
without having to load and decode the instruction word by hand,
because all the information you need to emulate them is in the ISS
fields. So you don't get ISV=1 for load/store pair because the ISS
doesn't have fields for more than one transfer register, and you
don't get ISV=1 for instructions doing register writeback because
that's not something the ISS gives you enough information to do, and so on.
And the reason that you only get this extra ISV=1 information for
these faults at EL2 is that the assumption is that only a hypervisor
needs to be doing this kind of emulate-and-continue of a data abort,
so the architecture absolves non-EL2 implementations of the need to
do all this work to track and report the information relating to the
insn that provoked the fault.

-- PMM

Re: [PATCH v2] hvf: arm: Ignore cache operations on MMIO

[Peter Maydell]

On Tue, 26 Oct 2021 at 17:22, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 10/26/21 12:12 AM, Alexander Graf wrote:
> > +        if (cm) {
> > +            /* We don't cache MMIO regions */
> > +            advance_pc = true;
> > +            break;
> > +        }
> > +
> >           assert(isv);
>
> The assert should come first.  If the "iss valid" bit is not set, then nothing else in the
> word is defined.

No, ISV only indicates that ISS[23:14] is valid; ISS[13:0] (including CM)
are valid regardless. (The distinction is that the bits which might or
might not be valid are the ones which encode information about the insn
necessary to possibly emulate it, like the data access size and the
source/destination register; the always-present ones are the ones that
have always been reported for data aborts -- AArch32 DFSR has a CM bit,
for instance.)

-- PMM

Re: [PATCH v2] hvf: arm: Ignore cache operations on MMIO

[Richard Henderson]

On 11/1/21 1:55 PM, Peter Maydell wrote:
> On Tue, 26 Oct 2021 at 17:22, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 10/26/21 12:12 AM, Alexander Graf wrote:
>>> +        if (cm) {
>>> +            /* We don't cache MMIO regions */
>>> +            advance_pc = true;
>>> +            break;
>>> +        }
>>> +
>>>            assert(isv);
>>
>> The assert should come first.  If the "iss valid" bit is not set, then nothing else in the
>> word is defined.
> 
> No, ISV only indicates that ISS[23:14] is valid; ISS[13:0] (including CM)
> are valid regardless. (The distinction is that the bits which might or
> might not be valid are the ones which encode information about the insn
> necessary to possibly emulate it, like the data access size and the
> source/destination register; the always-present ones are the ones that
> have always been reported for data aborts -- AArch32 DFSR has a CM bit,
> for instance.)

Thanks, that clears up some of my confusion here and a bit further downthread.


r~

Re: [PATCH v2] hvf: arm: Ignore cache operations on MMIO

[Richard Henderson]

On 10/26/21 3:12 AM, Alexander Graf wrote:
> Apple's Hypervisor.Framework forwards cache operations as MMIO traps
> into user space. For MMIO however, these have no meaning: There is no
> cache attached to them.
> 
> So let's just treat cache data exits as nops.
> 
> This fixes OpenBSD booting as guest.
> 
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> Reported-by: AJ Barris <AwlsomeAlex@github.com>
> Reference: https://github.com/utmapp/UTM/issues/3197
> ---
>   target/arm/hvf/hvf.c | 7 +++++++
>   1 file changed, 7 insertions(+)

Thanks, applied to target-arm.next


r~

Re: [PATCH v2] hvf: arm: Ignore cache operations on MMIO

[Peter Maydell]

On Mon, 1 Nov 2021 at 19:28, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 10/26/21 3:12 AM, Alexander Graf wrote:
> > Apple's Hypervisor.Framework forwards cache operations as MMIO traps
> > into user space. For MMIO however, these have no meaning: There is no
> > cache attached to them.
> >
> > So let's just treat cache data exits as nops.
> >
> > This fixes OpenBSD booting as guest.
> >
> > Signed-off-by: Alexander Graf <agraf@csgraf.de>
> > Reported-by: AJ Barris <AwlsomeAlex@github.com>
> > Reference: https://github.com/utmapp/UTM/issues/3197
> > ---
> >   target/arm/hvf/hvf.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
>
> Thanks, applied to target-arm.next

...did you see my email saying I think we also need
to test S1PTW ?

-- PMM

Re: [PATCH v2] hvf: arm: Ignore cache operations on MMIO

[Richard Henderson]

On 11/1/21 3:35 PM, Peter Maydell wrote:
> On Mon, 1 Nov 2021 at 19:28, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 10/26/21 3:12 AM, Alexander Graf wrote:
>>> Apple's Hypervisor.Framework forwards cache operations as MMIO traps
>>> into user space. For MMIO however, these have no meaning: There is no
>>> cache attached to them.
>>>
>>> So let's just treat cache data exits as nops.
>>>
>>> This fixes OpenBSD booting as guest.
>>>
>>> Signed-off-by: Alexander Graf <agraf@csgraf.de>
>>> Reported-by: AJ Barris <AwlsomeAlex@github.com>
>>> Reference: https://github.com/utmapp/UTM/issues/3197
>>> ---
>>>    target/arm/hvf/hvf.c | 7 +++++++
>>>    1 file changed, 7 insertions(+)
>>
>> Thanks, applied to target-arm.next
> 
> ...did you see my email saying I think we also need
> to test S1PTW ?

That arrived afterward.

r~

Re: [PATCH v2] hvf: arm: Ignore cache operations on MMIO

[Peter Maydell]

On Tue, 2 Nov 2021 at 10:01, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 11/1/21 3:35 PM, Peter Maydell wrote:
> > On Mon, 1 Nov 2021 at 19:28, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> On 10/26/21 3:12 AM, Alexander Graf wrote:
> >>> Apple's Hypervisor.Framework forwards cache operations as MMIO traps
> >>> into user space. For MMIO however, these have no meaning: There is no
> >>> cache attached to them.
> >>>
> >>> So let's just treat cache data exits as nops.
> >>>
> >>> This fixes OpenBSD booting as guest.
> >>>
> >>> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> >>> Reported-by: AJ Barris <AwlsomeAlex@github.com>
> >>> Reference: https://github.com/utmapp/UTM/issues/3197
> >>> ---
> >>>    target/arm/hvf/hvf.c | 7 +++++++
> >>>    1 file changed, 7 insertions(+)
> >>
> >> Thanks, applied to target-arm.next
> >
> > ...did you see my email saying I think we also need
> > to test S1PTW ?
>
> That arrived afterward.

Thinking it over later, I wouldn't be opposed to taking this
patch now and adding the S1PTW second -- I think we're
currently going to do the wrong thing for the "page tables
not in RAM case anyway", so you could regard it as a
separate bug fix.

-- PMM

Re: [PATCH v2] hvf: arm: Ignore cache operations on MMIO

[Richard Henderson]

On 11/2/21 6:42 AM, Peter Maydell wrote:
> On Tue, 2 Nov 2021 at 10:01, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 11/1/21 3:35 PM, Peter Maydell wrote:
>>> On Mon, 1 Nov 2021 at 19:28, Richard Henderson
>>> <richard.henderson@linaro.org> wrote:
>>>>
>>>> On 10/26/21 3:12 AM, Alexander Graf wrote:
>>>>> Apple's Hypervisor.Framework forwards cache operations as MMIO traps
>>>>> into user space. For MMIO however, these have no meaning: There is no
>>>>> cache attached to them.
>>>>>
>>>>> So let's just treat cache data exits as nops.
>>>>>
>>>>> This fixes OpenBSD booting as guest.
>>>>>
>>>>> Signed-off-by: Alexander Graf <agraf@csgraf.de>
>>>>> Reported-by: AJ Barris <AwlsomeAlex@github.com>
>>>>> Reference: https://github.com/utmapp/UTM/issues/3197
>>>>> ---
>>>>>     target/arm/hvf/hvf.c | 7 +++++++
>>>>>     1 file changed, 7 insertions(+)
>>>>
>>>> Thanks, applied to target-arm.next
>>>
>>> ...did you see my email saying I think we also need
>>> to test S1PTW ?
>>
>> That arrived afterward.
> 
> Thinking it over later, I wouldn't be opposed to taking this
> patch now and adding the S1PTW second -- I think we're
> currently going to do the wrong thing for the "page tables
> not in RAM case anyway", so you could regard it as a
> separate bug fix.

Heh.  I was thinking the something similar, after I dequeued the patch.  That this fixes a 
problem emulating a well-behaved guest, and that the s1ptw hole is "merely" a problem vs 
an ill-behaved guest hitting the following assert.

Well, I've sent off the PR without this included, so I guess this should be the first cab 
off the rank for the next set.

I'm quite happy to hand the target-arm.next baton back to you.  ;-)

r~

Re: [PATCH v2] hvf: arm: Ignore cache operations on MMIO

[Richard Henderson]

On 11/2/21 7:15 AM, Richard Henderson wrote:
> On 11/2/21 6:42 AM, Peter Maydell wrote:
>> On Tue, 2 Nov 2021 at 10:01, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>
>>> On 11/1/21 3:35 PM, Peter Maydell wrote:
>>>> On Mon, 1 Nov 2021 at 19:28, Richard Henderson
>>>> <richard.henderson@linaro.org> wrote:
>>>>>
>>>>> On 10/26/21 3:12 AM, Alexander Graf wrote:
>>>>>> Apple's Hypervisor.Framework forwards cache operations as MMIO traps
>>>>>> into user space. For MMIO however, these have no meaning: There is no
>>>>>> cache attached to them.
>>>>>>
>>>>>> So let's just treat cache data exits as nops.
>>>>>>
>>>>>> This fixes OpenBSD booting as guest.
>>>>>>
>>>>>> Signed-off-by: Alexander Graf <agraf@csgraf.de>
>>>>>> Reported-by: AJ Barris <AwlsomeAlex@github.com>
>>>>>> Reference: https://github.com/utmapp/UTM/issues/3197
>>>>>> ---
>>>>>>     target/arm/hvf/hvf.c | 7 +++++++
>>>>>>     1 file changed, 7 insertions(+)
>>>>>
>>>>> Thanks, applied to target-arm.next
>>>>
>>>> ...did you see my email saying I think we also need
>>>> to test S1PTW ?
>>>
>>> That arrived afterward.
>>
>> Thinking it over later, I wouldn't be opposed to taking this
>> patch now and adding the S1PTW second -- I think we're
>> currently going to do the wrong thing for the "page tables
>> not in RAM case anyway", so you could regard it as a
>> separate bug fix.
> 
> Heh.  I was thinking the something similar, after I dequeued the patch.  That this fixes a 
> problem emulating a well-behaved guest, and that the s1ptw hole is "merely" a problem vs 
> an ill-behaved guest hitting the following assert.
> 
> Well, I've sent off the PR without this included, so I guess this should be the first cab 
> off the rank for the next set.
> 
> I'm quite happy to hand the target-arm.next baton back to you.  ;-)

Since I need to re-spin the target-arm.next PR anyway, I've re-queued this.


r~

Re: [PATCH v2] hvf: arm: Ignore cache operations on MMIO

[Peter Maydell]

On Tue, 26 Oct 2021 at 09:09, Alexander Graf <agraf@csgraf.de> wrote:
>
> Apple's Hypervisor.Framework forwards cache operations as MMIO traps
> into user space. For MMIO however, these have no meaning: There is no
> cache attached to them.
>
> So let's just treat cache data exits as nops.
>
> This fixes OpenBSD booting as guest.

I agree that "ignore cache maintenance ops" is the right thing
(among other things it's what KVM does in kvm_handle_guest_abort()).

But CM=1 isn't only cache maintenance, it is also set for faults
for address translation instructions. I think (but have not tested
or completely thought through) that before this you also want
   if (S1PTW is set) {
       /*
        * Guest has put its page tables not into RAM. We
        * can't do anything to retrieve this, so re-inject
        * the abort back into the guest.
        */
       inject a data abort with suitable fault info;
   }

Compare the sequence in the KVM code:
https://elixir.bootlin.com/linux/latest/source/arch/arm64/kvm/mmu.c#L1233
where we check S1PTW, then CM, then go for "let userspace do
MMIO emulation".

It's possible that Hypervisor.Framework handles the S1PTW
case for you; you could test with a stunt guest that sets up
the page tables so that the 2nd level page table for a
particular VA range is mapped to an IPA that's not in RAM,
and then try just using that VA and/or passing that VA to
one of the AT instructions, to see whether you get handed
the fault or not. (My bet would be that hvf does not handle
this for you, because in general it seems to prefer to punt
everything.)

-- PMM