Re: [PATCH] hvf: Handle EC_INSNABORT

[Mark Burton]

This patch came from a discussion on the KVM call the other day.
It may well be the case there is a better/different implementation - so the 
patch is more by way of asking the question.

Re-phrasing your question - I think it boils down to “should HVF (and KVM) 
support executing instructions from IO space?”.

In that case, this is a ‘partial’ answer to providing such support for HVF - 
partial in that it relies upon a memory region being created “dynamically” for 
the IO space that has been accessed as a side-effect of a normal access. In 
principle I think this is at least a reasonable approach to dynamically create 
memory regions in this way, potentially a cache could be constructed etc… of 
course the MR could be subsequently removed too…. I’m less happy about it being 
a side-effect of a memory operation, but I don’t see a better path?

Perhaps there is a better way of handling this.

Cheers
Mark.




> On 1 Jun 2023, at 17:34, Antonio Caggiano <quic_acaggian@quicinc.com> wrote:
> 
> Hi Peter,
> 
> On 01/06/2023 16:58, Peter Maydell wrote:
>> On Thu, 1 Jun 2023 at 15:33, Antonio Caggiano <quic_acaggian@quicinc.com> 
>> wrote:
>>> 
>>> Instead of aborting immediately, try reading the physical address where
>>> the instruction should be fetched by calling address_space_read. This
>>> would give any memory regions ops callback a chance to allocate and/or
>>> register an RAM/Alias memory region needed for resolving that physical
>>> address. Then, if the memory transaction is OK, retry HVF execution at
>>> the same PC.
>> What are the circumstances where this happens?
>> Do we try to support this on KVM ?
> 
> We use qemu as a library and manage memory regions externally, allocating 
> them on demand when there is a read or a write (through memory region ops 
> callbacks).
> 
> When enabling HVF, we hit an instruction abort on the very first instruction 
> as there is no memory region alias for it yet in system memory.
> 
>>> Signed-off-by: Antonio Caggiano <quic_acaggian@quicinc.com>
>>> Co-authored-by: Mark Burton <quic_mburton@quicinc.com>
>>> ---
>>>  target/arm/hvf/hvf.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>> 
>>> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
>>> index ad65603445..6e527254b1 100644
>>> --- a/target/arm/hvf/hvf.c
>>> +++ b/target/arm/hvf/hvf.c
>>> @@ -1446,6 +1446,18 @@ int hvf_vcpu_exec(CPUState *cpu)
>>>              hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized());
>>>          }
>>>          break;
>>> +    case EC_INSNABORT: {
>>> +        uint32_t sas = (syndrome >> 22) & 3;
>>> +        uint32_t len = 1 << sas;
>>> +        uint64_t val = 0;
>>> +
>>> +        MemTxResult res = address_space_read(
>>> +            &address_space_memory, hvf_exit->exception.physical_address,
>>> +            MEMTXATTRS_UNSPECIFIED, &val, len);
>>> +        assert(res == MEMTX_OK);
>> You can't assert() this, it might not be true, especially if
>> we're here because hvf couldn't read from this address.
> 
> The idea is to try reading so that memory region ops can create the Alias MR 
> required, in which case it would return MEMTX_OK. In case of error, maybe we 
> can just exit and report the error like the default case.
> 
>>> +        flush_cpu_state(cpu);
>>> +        break;
>>> +    }
>>>      default:
>>>          cpu_synchronize_state(cpu);
>>>          trace_hvf_exit(syndrome, ec, env->pc);
>> thanks
>> -- PMM
> 
> Cheers,
> Antonio