Re: [PATCH v2] piix: fix regression during unplug in Xen HVM domUs

[John Snow <jsnow@redhat.com>]

On 3/17/21 3:00 AM, Olaf Hering wrote:
> Commit ee358e919e385fdc79d59d0d47b4a81e349cd5c9 causes a regression in
> Xen HVM domUs which run xenlinux based kernels.
> 
> If the domU has an USB device assigned, for example with
> "usbdevice=['tablet']" in domU.cfg, the late unplug of devices will
> kill the emulated USB host. As a result the khubd thread hangs, and as
> a result the entire boot process.
> 
> For some reason this does not affect pvops based kernels. This is
> most likely caused by the fact that unplugging happens very early
> during boot.
> 

I'm not entirely sure of how the commit message relates to the patch, 
actually. (Sorry, I am not well familiar with XEN.)

> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>   hw/ide/piix.c        | 5 +++++
>   include/hw/ide/pci.h | 1 +
>   2 files changed, 6 insertions(+)
> 
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index b9860e35a5..7f1998bf04 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -109,6 +109,9 @@ static void piix_ide_reset(DeviceState *dev)
>       uint8_t *pci_conf = pd->config;
>       int i;
>   
> +    if (d->xen_unplug_done == true) {
> +        return;
> +    }

My understanding is that XEN has some extra disks that it unplugs when 
it later figures out it doesn't need them. How exactly this works is 
something I've not looked into too closely.

So if these IDE devices have been "unplugged" already, we avoid 
resetting them here. What about this reset causes the bug you describe 
in the commit message?

Does this reset now happen earlier/later as compared to what it did 
prior to ee358e91 ?

>       for (i = 0; i < 2; i++) {
>           ide_bus_reset(&d->bus[i]);
>       }
> @@ -151,6 +154,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>       PCIIDEState *d = PCI_IDE(dev);
>       uint8_t *pci_conf = dev->config;
>   
> +    d->xen_unplug_done = false;
>       pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>   
>       bmdma_setup_bar(d);
> @@ -170,6 +174,7 @@ int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
>       BlockBackend *blk;
>   
>       pci_ide = PCI_IDE(dev);
> +    pci_ide->xen_unplug_done = true;
>   
>       for (i = aux ? 1 : 0; i < 4; i++) {
>           idebus = &pci_ide->bus[i / 2];
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index d8384e1c42..9e71cfec3b 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -50,6 +50,7 @@ struct PCIIDEState {
>       IDEBus bus[2];
>       BMDMAState bmdma[2];
>       uint32_t secondary; /* used only for cmd646 */
> +    bool xen_unplug_done;

I am hesitant to put a new XEN-specific boolean here, but don't know 
enough about the problem to outright say "no".

This looks like a band-aid that's out of place, but I don't understand 
the problem well enough yet to suggest a better place.

>       MemoryRegion bmdma_bar;
>       MemoryRegion cmd_bar[2];
>       MemoryRegion data_bar[2];
> 

(If anyone else with more experience with XEN wants to take over the 
review of this patch, let me know. I only really care about the IDE bits.)