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