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