Re: [Qemu-devel] [PATCH] ahci: enable pci bus master MemoryRegion before loading ahci engines

[Michael S. Tsirkin]

On Tue, Sep 10, 2019 at 09:50:41AM -0400, John Snow wrote:
> 
> 
> On 9/10/19 3:04 AM, Michael S. Tsirkin wrote:
> > On Tue, Sep 10, 2019 at 01:18:37AM +0800, andychiu wrote:
> >> If Windows 10 guests have enabled 'turn off hard disk after idle'
> >> option in power settings, and the guest has a SATA disk plugged in,
> >> the SATA disk will be turned off after a specified idle time.
> >> If the guest is live migrated or saved/loaded with its SATA disk
> >> turned off, the following error will occur:
> >>
> >> qemu-system-x86_64: AHCI: Failed to start FIS receive engine: bad FIS 
> >> receive buffer address
> >> qemu-system-x86_64: Failed to load ich9_ahci:ahci
> >> qemu-system-x86_64: error while loading state for instance 0x0 of device 
> >> '0000:00:1a.0/ich9_ahci'
> >> qemu-system-x86_64: load of migration failed: Operation not permitted
> >>
> >> Observation from trace logs shows that a while after Windows 10 turns off
> >> a SATA disk (IDE disks don't have the following behavior),
> >> it will disable the PCI_COMMAND_MASTER flag of the pci device containing
> >> the ahci device. When the the disk is turning back on,
> >> the PCI_COMMAND_MASTER flag will be restored first.
> >> But if the guest is migrated or saved/loaded while the disk is off,
> >> the post_load callback of ahci device, ahci_state_post_load(), will fail
> >> at ahci_cond_start_engines() if the MemoryRegion
> >> pci_dev->bus_master_enable_region is not enabled, with pci_dev pointing
> >> to the PCIDevice struct containing the ahci device.
> >>
> >> This patch enables pci_dev->bus_master_enable_region before calling
> >> ahci_cond_start_engines() in ahci_state_post_load(), and restore the
> >> MemoryRegion to its original state afterwards.
> >>
> >> Signed-off-by: andychiu <address@hidden>
> > 
> > Poking at PCI device internals like this seems fragile.  And force
> > enabling bus master can lead to unpleasantness like corrupting guest
> > memory, unhandled interrupts, etc.  E.g. it's quite reasonable,
> > spec-wise, for the guest to move thing in memory around while bus
> > mastering is off.
> > 
> > Can you teach ahci that region being disabled
> > during migration is ok, and recover from it?
> 
> That's what I'm wondering.
> 
> I could try to just disable the FIS RX engine if the mapping fails, but
> that will require a change to guest visible state.
> 
> My hunch, though, is that when windows re-enables the device it will
> need to re-program the address registers anyway, so it might cope well
> with the FIS RX bit getting switched off.
> 
> (I'm wondering if it isn't a mistake that QEMU is trying to re-map this
> address in the first place. Is it legal that the PCI device has pci bus
> master disabled but we've held on to a mapping?

If you are poking at guest memory when bus master is off, then most likely yes.

> Should there be some
> callback where AHCI knows to invalidate mappings at that point...?)

ATM the callback is the config write, you check
proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER
and if disabled invalidate the mapping.

virtio at least has code that pokes at
proxy->pci_dev.config[PCI_COMMAND] too, I'm quite
open to a function along the lines of
pci_is_bus_master_enabled()
that will do this.

-- 
MST