Re: [Qemu-devel] [PATCH v5 00/12] KVM Support for MIPS32 Processors
On 17 June 2014 23:10, James Hogan wrote: > The patchset depends on v4 of "target-mips: implement UserLocal > Register". I'm aiming for QEMU 2.1, hopefully it isn't too late to get > some final review. > > Thanks to everybody who has already taken part in review. > > This patchset implements KVM support for MIPS32 processors, using Trap & > Emulation. I was looking at what happens for MMIO accesses to nonexistent memory when we're running under KVM, and interestingly this patchset means MIPS is now the only CPU which both (a) supports KVM and (b) has an implementation of the do_unassigned_access() CPU hook. Does the current code support a guest attempt to access unassigned physical addresses? I had a look at the code and it seems like mips_cpu_unassigned_access() will end up calling cpu_restore_state() and cpu_loop_exit(), which I think will probably crash if you call them when using KVM rather than TCG... More generally, there doesn't really seem to be provision in the KVM KVM_EXIT_MMIO API for returning "this access failed". I guess in theory userspace could do all the "figure out how to adjust CPU state to do exception entry and then run VCPU", but that seems like quite a lot of work which the kernel already knows how to do; is there some way to provide a simpler API that lets userspace just inform the kernel that it needs to fault the access? thanks -- PMM -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH v5 00/12] KVM Support for MIPS32 Processors
Il 10/07/2014 14:17, Peter Maydell ha scritto: More generally, there doesn't really seem to be provision in the KVM KVM_EXIT_MMIO API for returning "this access failed". I guess in theory userspace could do all the "figure out how to adjust CPU state to do exception entry and then run VCPU", but that seems like quite a lot of work which the kernel already knows how to do; is there some way to provide a simpler API that lets userspace just inform the kernel that it needs to fault the access? There are 3 free padding bytes in struct kvm_run's mmio field. It's possible to add a per-VM capability and have the kernel check one of these bytes when the capability is set. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH v5 00/12] KVM Support for MIPS32 Processors
Hi Peter, On 10/07/14 13:17, Peter Maydell wrote: > On 17 June 2014 23:10, James Hogan wrote: >> The patchset depends on v4 of "target-mips: implement UserLocal >> Register". I'm aiming for QEMU 2.1, hopefully it isn't too late to get >> some final review. >> >> Thanks to everybody who has already taken part in review. >> >> This patchset implements KVM support for MIPS32 processors, using Trap & >> Emulation. > > I was looking at what happens for MMIO accesses to nonexistent > memory when we're running under KVM, and interestingly this > patchset means MIPS is now the only CPU which both (a) supports > KVM and (b) has an implementation of the do_unassigned_access() > CPU hook. Does the current code support a guest attempt to > access unassigned physical addresses? I had a look at the code > and it seems like mips_cpu_unassigned_access() will end up > calling cpu_restore_state() and cpu_loop_exit(), which I think > will probably crash if you call them when using KVM rather than > TCG... Yes, I have observed this in the past when experimentally writing to the Malta reset region from the guest (see the patch "mips/malta: allow volatile writes to reset flash" which didn't get applied but worked around the specific issue). That was because read only memory regions were treated as unassigned from the point of view of writes (which tbh seems wrong), but it could happen with any unassigned access > 0x8000 from the guest AFAICT. So yeh, until mips_cpu_unassigned_access does something more portable for KVM, conditionally setting do_unassigned_access only if !kvm_enabled() looks sensible. I'll see if I can reproduce it and submit a patch. > More generally, there doesn't really seem to be provision in the > KVM KVM_EXIT_MMIO API for returning "this access failed". > I guess in theory userspace could do all the "figure out how > to adjust CPU state to do exception entry and then run VCPU", > but that seems like quite a lot of work which the kernel already > knows how to do; is there some way to provide a simpler API > that lets userspace just inform the kernel that it needs to > fault the access? Indeed. Paolo's idea would work well I think. A data bus error exception would likely be the only sensible error response required other than ignoring writes or returning a garbage value for a read (which the current KVM MMIO API already allows). Cheers James -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH v5 00/12] KVM Support for MIPS32 Processors
On 14 July 2014 14:33, James Hogan wrote:
> On 10/07/14 13:17, Peter Maydell wrote:
>> More generally, there doesn't really seem to be provision in the
>> KVM KVM_EXIT_MMIO API for returning "this access failed".
>> I guess in theory userspace could do all the "figure out how
>> to adjust CPU state to do exception entry and then run VCPU",
>> but that seems like quite a lot of work which the kernel already
>> knows how to do; is there some way to provide a simpler API
>> that lets userspace just inform the kernel that it needs to
>> fault the access?
>
> Indeed. Paolo's idea would work well I think. A data bus error exception
> would likely be the only sensible error response required other than
> ignoring writes or returning a garbage value for a read (which the
> current KVM MMIO API already allows).
I think we should make the API at least permit returning an
(architecture-specific) error code to the kernel, rather than just
a single boolean "failed" response. For instance on ARM the
fault status registers include a single ExT bit for classifying
the type of an external abort. (The meaning of the bit is
IMPDEF; on the Cortex-A15 it can be used to distinguish
AXI bus DECERR ("decode error", ie the interconnect doesn't
have anything attached at that address) and SLVERR ("slave
error", ie there was a device at that address but it chose to
reject the transaction for some reason, eg unsupported
transfer size, timeout, write to read-only location, FIFO
overrun or just because the device was in a bad mood.)
thanks
-- PMM
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH v5 00/12] KVM Support for MIPS32 Processors
On 14/07/14 15:35, Peter Maydell wrote:
> On 14 July 2014 14:33, James Hogan wrote:
>> On 10/07/14 13:17, Peter Maydell wrote:
>>> More generally, there doesn't really seem to be provision in the
>>> KVM KVM_EXIT_MMIO API for returning "this access failed".
>>> I guess in theory userspace could do all the "figure out how
>>> to adjust CPU state to do exception entry and then run VCPU",
>>> but that seems like quite a lot of work which the kernel already
>>> knows how to do; is there some way to provide a simpler API
>>> that lets userspace just inform the kernel that it needs to
>>> fault the access?
>>
>> Indeed. Paolo's idea would work well I think. A data bus error exception
>> would likely be the only sensible error response required other than
>> ignoring writes or returning a garbage value for a read (which the
>> current KVM MMIO API already allows).
>
> I think we should make the API at least permit returning an
> (architecture-specific) error code to the kernel, rather than just
> a single boolean "failed" response. For instance on ARM the
> fault status registers include a single ExT bit for classifying
> the type of an external abort. (The meaning of the bit is
> IMPDEF; on the Cortex-A15 it can be used to distinguish
> AXI bus DECERR ("decode error", ie the interconnect doesn't
> have anything attached at that address) and SLVERR ("slave
> error", ie there was a device at that address but it chose to
> reject the transaction for some reason, eg unsupported
> transfer size, timeout, write to read-only location, FIFO
> overrun or just because the device was in a bad mood.)
Agreed (I wasn't suggesting a bool, just thinking out loud about how
mips would use that arch specific value :-) ).
Cheers
James
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
