Re: Reset problem vs. MMIO emulation, hypercalls, etc...

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Thu, 2012-08-02 at 12:46 +0200, Alexander Graf wrote:
> Avi, Marcelo, Jan? Any thoughts on this?

Had a chat with Anthony and it looks like we can do it using the MP
state stuff which we don't currently use on powerpc, at least it seems
that's how x86 breaks IO emulation on reset.

Cheers,
Ben.

> Alex
> 
> On 01.08.2012, at 05:17, Benjamin Herrenschmidt wrote:
> 
> > Hi Avi !
> > 
> > We identified a problem on powerpc which seems to actually be a generic
> > issue, and Alex suggested we propose a generic fix. I want to make sure
> > we are on the right track first before proposing an actual patch as we
> > would like the patch to go in ASAP (ie not waiting the next merge
> > window) as it will fix an actual nasty bug with reset in KVM.
> > 
> > So the basic issue has to do with doing a machine reset as a result of a
> > hypervisor call, but the same problem should happen with MMIO/PIO
> > emulation.
> > 
> > After we do an exit as a result of such an operation, at the next
> > KVM_RUN, KVM will fetch the "results" of the operation (in the hypercall
> > case that's a bunch of register values, in the MMIO read emulation case
> > it's a single register value usually, x86 might have more subtle cases)
> > and we update the VCPU state (ie. registers) with that data.
> > 
> > However, what happens is that if a reset happens in between, we end up
> > clobbering the reset state.
> > 
> > IE. What happens in qemu is roughtly:
> > 
> > - The hcall or MMIO that triggers the reset happens, goes to qemu,
> > which eventually calls qemu_system_reset_request()
> > 
> > - This sets the global reset pending flag and wakes up the main loop.
> > It also does a stop of the current vcpu, so we do not return to the
> > kernel at this stage.
> > 
> > - The main loop gets the flag, starts the reset process, which begins
> > with stopping all the VCPUs.
> > 
> > - The reset handlers are called, which includes resetting the CPU
> > state, which in our case (powerpc) results in a SET_REGS ioctl to
> > establish a new fresh state for booting.
> > 
> > - The generic code then restarts all VCPUs, which then return into
> > VCPU_RUN.
> > 
> > - The VCPU(s) that did an exit as a result of MMIO emulation,
> > hypercall, or similiar (typically the one that triggered the reset but
> > possibly others) then gets some of their register state "updated" by the
> > result of the operation (in the hcall case, it's a field in the mmap'ed
> > run structure that clobbers GPR3 among others).
> > 
> > Now this is generally not a big issue as -usually- machines don't care
> > much about the state of registers on reset.
> > 
> > However it is for us, at least in a couple of cases because we build a
> > device-tree in qemu and we pass a pointer to it in GPR3 before we start
> > the firmware or the guest kernel.
> > 
> > This pointer is set in env->gpr[3] which then gets written by a reset
> > handler to KVM with SET_REGS, but that ends up being clobbered
> > eventually by the return from the hypercall.
> > 
> > My original idea to fix that was a ppc specific "trick" where we would
> > make SET_REGS clear the flags indicating that we had a pending hypercall
> > (or similar) and thus have SET_REGS have the side-effect of aborting a
> > current operation.
> > 
> > However, Alex reckons we should instead have a generic ioctl
> > (VCPU_ABORT_OPERATION) which qemu could use when there's a reset, that
> > we call to generically abort all such operations (MMIO emu, hcalls,
> > etc...) on all archs when qemu decides to reset the processors.
> > 
> > Alex and I have looked at various possible ways to try to fix that from
> > inside qemu but they are all extremely horrible & gross and might not
> > even work in the general case.
> > 
> > Avi, before I actually do a patch (that adds the generic definition,
> > doc, and powerpc implementation), I'd like to confirm with you that this
> > is the right approach and that this can benefit from expedited
> > upstreaming :-)
> > 
> > Cheers,
> > Ben.
> > 
> >