qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hvf: arm: Ignore cache operations on MMIO
@ 2021-10-25 19:13 Alexander Graf
  2021-10-25 20:57 ` Philippe Mathieu-Daudé
  2021-10-26  0:14 ` Richard Henderson
  0 siblings, 2 replies; 5+ messages in thread
From: Alexander Graf @ 2021-10-25 19:13 UTC (permalink / raw)
  To: Cameron Esfahani
  Cc: kettenis, qemu-devel, AJ Barris, Roman Bolshakov, Paolo Bonzini, osy

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 filter SYS instructions for DATA exits out and treat them as nops.

This fixes OpenBSD booting as guest.

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

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index bff3e0cde7..46ff4892a7 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -1098,6 +1098,33 @@ static void hvf_sync_vtimer(CPUState *cpu)
     }
 }
 
+static bool hvf_emulate_insn(CPUState *cpu)
+{
+    ARMCPU *arm_cpu = ARM_CPU(cpu);
+    CPUARMState *env = &arm_cpu->env;
+    uint32_t insn;
+
+    /*
+     * We ran into an instruction that traps for data, but is not
+     * hardware predecoded. This should not ever happen for well
+     * behaved guests. Let's try to see if we can somehow rescue
+     * the situation.
+     */
+
+    cpu_synchronize_state(cpu);
+    if (cpu_memory_rw_debug(cpu, env->pc, &insn, 4, 0)) {
+        /* Could not read the instruction */
+        return false;
+    }
+
+    if ((insn & 0xffc00000) == 0xd5000000) {
+        /* MSR/MRS/SYS/SYSL - happens for cache ops which are nops on data */
+        return true;
+    }
+
+    return false;
+}
+
 int hvf_vcpu_exec(CPUState *cpu)
 {
     ARMCPU *arm_cpu = ARM_CPU(cpu);
@@ -1156,6 +1183,11 @@ int hvf_vcpu_exec(CPUState *cpu)
                              hvf_exit->exception.physical_address, isv,
                              iswrite, s1ptw, len, srt);
 
+        if (!isv) {
+            g_assert(hvf_emulate_insn(cpu));
+            advance_pc = true;
+            break;
+        }
         assert(isv);
 
         if (iswrite) {
-- 
2.30.1 (Apple Git-130)



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] hvf: arm: Ignore cache operations on MMIO
  2021-10-25 19:13 [PATCH] hvf: arm: Ignore cache operations on MMIO Alexander Graf
@ 2021-10-25 20:57 ` Philippe Mathieu-Daudé
  2021-10-26  0:14 ` Richard Henderson
  1 sibling, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-10-25 20:57 UTC (permalink / raw)
  To: Alexander Graf, Cameron Esfahani
  Cc: kettenis, qemu-devel, AJ Barris, Roman Bolshakov, Paolo Bonzini, osy

On 10/25/21 21:13, 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 filter SYS instructions for DATA exits out and treat them as nops.
> 
> This fixes OpenBSD booting as guest.
> 
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> Reported-by: AJ Barris <AwlsomeAlex@github.com>
> ---
>  target/arm/hvf/hvf.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index bff3e0cde7..46ff4892a7 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -1098,6 +1098,33 @@ static void hvf_sync_vtimer(CPUState *cpu)
>      }
>  }
>  
> +static bool hvf_emulate_insn(CPUState *cpu)
> +{
> +    ARMCPU *arm_cpu = ARM_CPU(cpu);
> +    CPUARMState *env = &arm_cpu->env;
> +    uint32_t insn;
> +
> +    /*
> +     * We ran into an instruction that traps for data, but is not
> +     * hardware predecoded. This should not ever happen for well
> +     * behaved guests. Let's try to see if we can somehow rescue
> +     * the situation.
> +     */
> +
> +    cpu_synchronize_state(cpu);
> +    if (cpu_memory_rw_debug(cpu, env->pc, &insn, 4, 0)) {

What about using cpu_ldl_data()?

> +        /* Could not read the instruction */
> +        return false;
> +    }
> +
> +    if ((insn & 0xffc00000) == 0xd5000000) {

Could there be an endianess issue here?

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

> +        /* MSR/MRS/SYS/SYSL - happens for cache ops which are nops on data */
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
>  int hvf_vcpu_exec(CPUState *cpu)
>  {
>      ARMCPU *arm_cpu = ARM_CPU(cpu);
> @@ -1156,6 +1183,11 @@ int hvf_vcpu_exec(CPUState *cpu)
>                               hvf_exit->exception.physical_address, isv,
>                               iswrite, s1ptw, len, srt);
>  
> +        if (!isv) {
> +            g_assert(hvf_emulate_insn(cpu));
> +            advance_pc = true;
> +            break;
> +        }
>          assert(isv);
>  
>          if (iswrite) {
> 



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] hvf: arm: Ignore cache operations on MMIO
  2021-10-25 19:13 [PATCH] hvf: arm: Ignore cache operations on MMIO Alexander Graf
  2021-10-25 20:57 ` Philippe Mathieu-Daudé
@ 2021-10-26  0:14 ` Richard Henderson
  2021-10-26  7:09   ` Alexander Graf
  1 sibling, 1 reply; 5+ messages in thread
From: Richard Henderson @ 2021-10-26  0:14 UTC (permalink / raw)
  To: Alexander Graf, Cameron Esfahani
  Cc: kettenis, qemu-devel, AJ Barris, Roman Bolshakov, Paolo Bonzini, osy

On 10/25/21 12:13 PM, Alexander Graf wrote:
> +    /*
> +     * We ran into an instruction that traps for data, but is not
> +     * hardware predecoded. This should not ever happen for well
> +     * behaved guests. Let's try to see if we can somehow rescue
> +     * the situation.
> +     */
> +
> +    cpu_synchronize_state(cpu);
> +    if (cpu_memory_rw_debug(cpu, env->pc, &insn, 4, 0)) {

This isn't correct, since this would be a physical address access, and env->pc is virtual.

Phil's idea of cpu_ldl_data may be correct, and cpu_ldl_code may be slightly more so, 
because we got EC_DATAABORT not EC_INSNABORT, which means that the virtual address at 
env->pc is mapped and executable.

However, in the event that there's some sort of race condition in between this data abort 
and hvf stopping all threads for the vm exit, by which the page tables could have been 
modified between here and there, then cpu_ldl_code *could* produce another exception.

In which case the interface that gdbstub uses, cc->memory_rw_debug, will be most correct.


> @@ -1156,6 +1183,11 @@ int hvf_vcpu_exec(CPUState *cpu)
>                                hvf_exit->exception.physical_address, isv,
>                                iswrite, s1ptw, len, srt);
>   
> +        if (!isv) {
> +            g_assert(hvf_emulate_insn(cpu));
> +            advance_pc = true;
> +            break;
> +        }
>           assert(isv);

Ouch.  HVF really passes along an invalid syndrome?  I was expecting that you'd be able to 
avoid all of the instruction parsing and check syndrome.cm (bit 8) for a cache management 
instruction.


r~


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] hvf: arm: Ignore cache operations on MMIO
  2021-10-26  0:14 ` Richard Henderson
@ 2021-10-26  7:09   ` Alexander Graf
  2021-10-26  8:56     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 5+ messages in thread
From: Alexander Graf @ 2021-10-26  7:09 UTC (permalink / raw)
  To: Richard Henderson, Cameron Esfahani
  Cc: Paolo Bonzini, kettenis, Roman Bolshakov, qemu-devel


On 26.10.21 02:14, Richard Henderson wrote:
> On 10/25/21 12:13 PM, Alexander Graf wrote:
>> +    /*
>> +     * We ran into an instruction that traps for data, but is not
>> +     * hardware predecoded. This should not ever happen for well
>> +     * behaved guests. Let's try to see if we can somehow rescue
>> +     * the situation.
>> +     */
>> +
>> +    cpu_synchronize_state(cpu);
>> +    if (cpu_memory_rw_debug(cpu, env->pc, &insn, 4, 0)) {
>
> This isn't correct, since this would be a physical address access, and 
> env->pc is virtual.


Yes, hence cpu_memory_rw_debug which accesses virtual memory:

https://git.qemu.org/?p=qemu.git;a=blob;f=softmmu/physmem.c#l3418


>
> Phil's idea of cpu_ldl_data may be correct, and cpu_ldl_code may be 
> slightly more so, because we got EC_DATAABORT not EC_INSNABORT, which 
> means that the virtual address at env->pc is mapped and executable.
>
> However, in the event that there's some sort of race condition in 
> between this data abort and hvf stopping all threads for the vm exit, 
> by which the page tables could have been modified between here and 
> there, then cpu_ldl_code *could* produce another exception.
>
> In which case the interface that gdbstub uses, cc->memory_rw_debug, 
> will be most correct.


I don't believe that one is implemented for arm, correct?


>
>
>> @@ -1156,6 +1183,11 @@ int hvf_vcpu_exec(CPUState *cpu)
>> hvf_exit->exception.physical_address, isv,
>>                                iswrite, s1ptw, len, srt);
>>   +        if (!isv) {
>> +            g_assert(hvf_emulate_insn(cpu));
>> +            advance_pc = true;
>> +            break;
>> +        }
>>           assert(isv);
>
> Ouch.  HVF really passes along an invalid syndrome?  I was expecting 
> that you'd be able to avoid all of the instruction parsing and check 
> syndrome.cm (bit 8) for a cache management instruction.


That's a very subtle way of telling me I'm stupid :). Thanks for the 
catch! Using the CM bit is obviously way better. Let me build v2.


Alex




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] hvf: arm: Ignore cache operations on MMIO
  2021-10-26  7:09   ` Alexander Graf
@ 2021-10-26  8:56     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-10-26  8:56 UTC (permalink / raw)
  To: Alexander Graf, Richard Henderson, Cameron Esfahani
  Cc: Paolo Bonzini, kettenis, Roman Bolshakov, qemu-devel

On 10/26/21 09:09, Alexander Graf wrote:
> On 26.10.21 02:14, Richard Henderson wrote:
>> On 10/25/21 12:13 PM, Alexander Graf wrote:

>>> @@ -1156,6 +1183,11 @@ int hvf_vcpu_exec(CPUState *cpu)
>>> hvf_exit->exception.physical_address, isv,
>>>                                iswrite, s1ptw, len, srt);
>>>   +        if (!isv) {
>>> +            g_assert(hvf_emulate_insn(cpu));
>>> +            advance_pc = true;
>>> +            break;
>>> +        }
>>>           assert(isv);
>>
>> Ouch.  HVF really passes along an invalid syndrome?  I was expecting
>> that you'd be able to avoid all of the instruction parsing and check
>> syndrome.cm (bit 8) for a cache management instruction.
> 
> 
> That's a very subtle way of telling me I'm stupid :). Thanks for the
> catch! Using the CM bit is obviously way better. Let me build v2.

Having given my R-b I take half of the blame.


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-10-26  8:58 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-25 19:13 [PATCH] hvf: arm: Ignore cache operations on MMIO Alexander Graf
2021-10-25 20:57 ` Philippe Mathieu-Daudé
2021-10-26  0:14 ` Richard Henderson
2021-10-26  7:09   ` Alexander Graf
2021-10-26  8:56     ` Philippe Mathieu-Daudé

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).