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

[Alexander Graf <agraf@csgraf.de>]

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