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

[John Snow <jsnow@redhat.com>]

On 9/9/19 1:18 PM, andychiu via Qemu-devel 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
> 

Oof. That can't have been fun to discover.

> 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.>

This looks good to me from an AHCI perspective, but I'm not as clear on
the implications of toggling the MemoryRegion, so I have some doubts.


MST, can you chime in and clear my confusion?

I suppose when the PCI_COMMAND_MASTER bit is turned off, we disable the
memory region, as a guest would be unable to establish a new mapping in
this time, so it makes sense that the attempt to map it fails.

What's less clear to me is what happens to existing mappings when a
region is disabled. Are they invalidated? If so, does it make sense that
we are trying to establish a mapping here at all? Maybe it's absolutely
correct that this fails.

(I suppose, though, that the simple toggling of the region won't be a
guest-visible event, so it's probably safe to do. Right?)

What I find weird for AHCI is this: We try to engage the CLB mapping
before the FIS mapping, but we fail at the FIS mapping. So why is
PORT_CMD_FIS_RX set while PORT_CMD_START is unset?

It kind of looks like we only half-heartedly stopped the AHCI device.
Maybe that's just what Windows does, but I wonder if there's a bug where
we're erroneously leaving PORT_CMD_FIS_RX set when we've been disabled.
It seems like the guest would need to re-set the mappings anyway, so
maybe trying to restore a stale mapping is not the right thing to do.



Andy, if you have traces left over: What AHCI registers does Windows
touch when it disables the AHCI device? What registers does it touch
when it re-engages it?

I just want to make sure I'm not leaving something dangling by accident.

--js

> Signed-off-by: andychiu <andychiu@synology.com>
> ---
>  hw/ide/ahci.c | 53 ++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 36 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index d45393c..83f8c30 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1649,33 +1649,52 @@ static const VMStateDescription vmstate_ahci_device = {
>      },
>  };
>  
> +static int ahci_state_load_engines(AHCIState *s, AHCIDevice *ad)
> +{
> +    AHCIPortRegs *pr = &ad->port_regs;
> +    DeviceState *dev_state = s->container;
> +    PCIDevice *pci_dev = (PCIDevice *) object_dynamic_cast(OBJECT(dev_state),
> +                                                           TYPE_PCI_DEVICE);
> +    bool pci_bus_master_enabled = pci_dev->bus_master_enable_region.enabled;
> +
> +    if (!(pr->cmd & PORT_CMD_START) && (pr->cmd & PORT_CMD_LIST_ON)) {
> +        error_report("AHCI: DMA engine should be off, but status bit "
> +                     "indicates it is still running.");
> +        return -1;
> +    }
> +    if (!(pr->cmd & PORT_CMD_FIS_RX) && (pr->cmd & PORT_CMD_FIS_ON)) {
> +        error_report("AHCI: FIS RX engine should be off, but status bit "
> +                     "indicates it is still running.");
> +        return -1;
> +    }
> +
> +    memory_region_set_enabled(&pci_dev->bus_master_enable_region, true);
> +
> +    /*
> +     * After a migrate, the DMA/FIS engines are "off" and
> +     * need to be conditionally restarted
> +     */
> +    pr->cmd &= ~(PORT_CMD_LIST_ON | PORT_CMD_FIS_ON);
> +    if (ahci_cond_start_engines(ad) != 0) {
> +        return -1;
> +    }
> +    memory_region_set_enabled(&pci_dev->bus_master_enable_region,
> +                              pci_bus_master_enabled);
> +
> +    return 0;
> +}
> +
>  static int ahci_state_post_load(void *opaque, int version_id)
>  {
>      int i, j;
>      struct AHCIDevice *ad;
>      NCQTransferState *ncq_tfs;
> -    AHCIPortRegs *pr;
>      AHCIState *s = opaque;
>  
>      for (i = 0; i < s->ports; i++) {
>          ad = &s->dev[i];
> -        pr = &ad->port_regs;
> -
> -        if (!(pr->cmd & PORT_CMD_START) && (pr->cmd & PORT_CMD_LIST_ON)) {
> -            error_report("AHCI: DMA engine should be off, but status bit "
> -                         "indicates it is still running.");
> -            return -1;
> -        }
> -        if (!(pr->cmd & PORT_CMD_FIS_RX) && (pr->cmd & PORT_CMD_FIS_ON)) {
> -            error_report("AHCI: FIS RX engine should be off, but status bit "
> -                         "indicates it is still running.");
> -            return -1;
> -        }
>  
> -        /* After a migrate, the DMA/FIS engines are "off" and
> -         * need to be conditionally restarted */
> -        pr->cmd &= ~(PORT_CMD_LIST_ON | PORT_CMD_FIS_ON);
> -        if (ahci_cond_start_engines(ad) != 0) {
> +        if (ahci_state_load_engines(s, ad)) {
>              return -1;
>          }
>  
>