[PATCH] hvf: Handle EC_INSNABORT

[PATCH] hvf: Handle EC_INSNABORT

[Antonio Caggiano]

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.

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);
+        flush_cpu_state(cpu);
+        break;
+    }
     default:
         cpu_synchronize_state(cpu);
         trace_hvf_exit(syndrome, ec, env->pc);
-- 
2.40.0

Re: [PATCH] hvf: Handle EC_INSNABORT

[Peter Maydell]

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 ?


> 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.

> +        flush_cpu_state(cpu);
> +        break;
> +    }
>      default:
>          cpu_synchronize_state(cpu);
>          trace_hvf_exit(syndrome, ec, env->pc);

thanks
-- PMM

Re: [PATCH] hvf: Handle EC_INSNABORT

[Antonio Caggiano]

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

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

Re: [PATCH] hvf: Handle EC_INSNABORT

[Peter Maydell]

On Thu, 1 Jun 2023 at 17:00, Mark Burton <quic_mburton@quicinc.com> wrote:
> 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?”.

I think this falls into "might theoretically be nice but is
probably too painful to actually implement". In practice
well-behaved guests don't try to execute out of MMIO devices.

> 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.

But nothing in (upstream) QEMU magically creates MemoryRegions
just because the guest tries to access them. Either there's
nothing there in the AddressSpace at all (definitely can't
execute from it) or there's already RAM (happy case) or there's
already a device there. If there's already a device there
then something would need to do a "put a bit of RAM in
temporarily, fill in the single instruction by doing an
address_space_read() to find the data value and writing it
to the scratch RAM, tell KVM/HVF to do a single-step, undo
everything again".

-- PMM

Re: [PATCH] hvf: Handle EC_INSNABORT

[Mark Burton]

> On 1 Jun 2023, at 18:45, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
> 
> On Thu, 1 Jun 2023 at 17:00, Mark Burton <quic_mburton@quicinc.com> wrote:
>> 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?”.
> 
> I think this falls into "might theoretically be nice but is
> probably too painful to actually implement". In practice
> well-behaved guests don't try to execute out of MMIO devices.
> 

>> 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.
> 
> But nothing in (upstream) QEMU magically creates MemoryRegions
> just because the guest tries to access them. Either there's
> nothing there in the AddressSpace at all (definitely can't
> execute from it) or there's already RAM (happy case) or there's
> already a device there. If there's already a device there
> then something would need to do a "put a bit of RAM in
> temporarily, fill in the single instruction by doing an
> address_space_read() to find the data value and writing it
> to the scratch RAM, tell KVM/HVF to do a single-step, undo
> everything again".
> 

Indeed, that’s basically what we’re implementing. In TCG mode you ’see’ the access, we’re just making it so that in HVF you equally ‘see’ such accesses to the ‘device’ (so you can put the bit of RAM in, out, shake it all about). A cleaner implementation may be some sort of “pre-i-side-access-op I’m about to access this device/address please register a ‘memory region’ I can use (temporarily)”. I’d have thought that could be useful any time you execute from e.g. a temporary ram of any sort (whatever the accelerator).

Cheers
Mark.


> -- PMM

Re: [PATCH] hvf: Handle EC_INSNABORT

[Peter Maydell]

On Thu, 1 Jun 2023 at 20:21, Mark Burton <quic_mburton@quicinc.com> wrote:
>
>
>
> > On 1 Jun 2023, at 18:45, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
> >
> > On Thu, 1 Jun 2023 at 17:00, Mark Burton <quic_mburton@quicinc.com> wrote:
> >> 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?”.
> >
> > I think this falls into "might theoretically be nice but is
> > probably too painful to actually implement". In practice
> > well-behaved guests don't try to execute out of MMIO devices.
> >
>
> >> 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.
> >
> > But nothing in (upstream) QEMU magically creates MemoryRegions
> > just because the guest tries to access them. Either there's
> > nothing there in the AddressSpace at all (definitely can't
> > execute from it) or there's already RAM (happy case) or there's
> > already a device there. If there's already a device there
> > then something would need to do a "put a bit of RAM in
> > temporarily, fill in the single instruction by doing an
> > address_space_read() to find the data value and writing it
> > to the scratch RAM, tell KVM/HVF to do a single-step, undo
> > everything again".

> Indeed, that’s basically what we’re implementing. In TCG mode you ’see’ the access, we’re just making it so that in HVF you equally ‘see’ such accesses to the ‘device’ (so you can put the bit of RAM in, out, shake it all about). A cleaner implementation may be some sort of “pre-i-side-access-op I’m about to access this device/address please register a ‘memory region’ I can use (temporarily)”. I’d have thought that could be useful any time you execute from e.g. a temporary ram of any sort (whatever the accelerator).

This patch doesn't do any of the "set up the RAM, single
step, tear it down again" work, though, which is the complicated
bit. It just retries an access that ought to have worked directly
when HVF did it; which isn't really what you would want to
do if you were trying to handle HVF or KVM exec-from-device.
In that scenario the "read from the underlying device" would
be in the middle of a large amount of other complicated code.
And without all that other complicated code (which I tend
to feel is not worthwhile as a feature) this change is
completely unmotivated by anything we have upstream...

-- PMM

Re: [PATCH] hvf: Handle EC_INSNABORT

[Mark Burton]

> On 2 Jun 2023, at 11:07, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
> 
> On Thu, 1 Jun 2023 at 20:21, Mark Burton <quic_mburton@quicinc.com> wrote:
>> 
>> 
>> 
>>> On 1 Jun 2023, at 18:45, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> 
>>> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
>>> 
>>> On Thu, 1 Jun 2023 at 17:00, Mark Burton <quic_mburton@quicinc.com> wrote:
>>>> 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?”.
>>> 
>>> I think this falls into "might theoretically be nice but is
>>> probably too painful to actually implement". In practice
>>> well-behaved guests don't try to execute out of MMIO devices.
>>> 
>> 
>>>> 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.
>>> 
>>> But nothing in (upstream) QEMU magically creates MemoryRegions
>>> just because the guest tries to access them. Either there's
>>> nothing there in the AddressSpace at all (definitely can't
>>> execute from it) or there's already RAM (happy case) or there's
>>> already a device there. If there's already a device there
>>> then something would need to do a "put a bit of RAM in
>>> temporarily, fill in the single instruction by doing an
>>> address_space_read() to find the data value and writing it
>>> to the scratch RAM, tell KVM/HVF to do a single-step, undo
>>> everything again".
> 
>> Indeed, that’s basically what we’re implementing. In TCG mode you ’see’ the access, we’re just making it so that in HVF you equally ‘see’ such accesses to the ‘device’ (so you can put the bit of RAM in, out, shake it all about). A cleaner implementation may be some sort of “pre-i-side-access-op I’m about to access this device/address please register a ‘memory region’ I can use (temporarily)”. I’d have thought that could be useful any time you execute from e.g. a temporary ram of any sort (whatever the accelerator).
> 
> This patch doesn't do any of the "set up the RAM, single
> step, tear it down again" work, though, which is the complicated

(That would need to be done in the device I believe).

> bit. It just retries an access that ought to have worked directly
> when HVF did it; which isn't really what you would want to
> do if you were trying to handle HVF or KVM exec-from-device.
> In that scenario the "read from the underlying device" would
> be in the middle of a large amount of other complicated code.

(Maybe not _so_ complicated given a suitable API, but this patch doesn’t provide any such API).

> And without all that other complicated code (which I tend
> to feel is not worthwhile as a feature) this change is
> completely unmotivated by anything we have upstream...
> 

I can’t help but agree on that :-)

Cheers
Mark.


> -- PMM