[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH] ahci: enable pci bus master Memory
From: |
John Snow |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH] ahci: enable pci bus master MemoryRegion before loading ahci engines |
Date: |
Mon, 9 Sep 2019 14:13:02 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 |
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 <address@hidden>
> ---
> 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;
> }
>
>