| Message ID | 20211026071241.74889-1-agraf@csgraf.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] hvf: arm: Ignore cache operations on MMIO | expand |
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>
> 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>
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~
> 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
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~
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
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~
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
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~
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
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
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~
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
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~
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~
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) {
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(+)