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

From: Paolo Bonzini <pbonzini@redhat.com>
To: Olaf Hering <olaf@aepfle.de>, John Snow <jsnow@redhat.com>
Cc: xen-devel@lists.xenproject.org,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	qemu-devel@nongnu.org, qemu-block@nongnu.org,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>
Subject: Re: [PATCH v2] piix: fix regression during unplug in Xen HVM domUs
Date: Thu, 25 Mar 2021 17:49:49 +0100	[thread overview]
Message-ID: <dae251e1-f808-708e-902c-05cfcbbea9cf@redhat.com> (raw)
In-Reply-To: <20210325121219.7b5daf76.olaf@aepfle.de>

On 25/03/21 12:12, Olaf Hering wrote:
> Am Mon, 22 Mar 2021 18:09:17 -0400
> schrieb John Snow <jsnow@redhat.com>:
> 
>> 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.
> 
> It has no extra disks, why would it?
> 
> I assume each virtualization variant has some sort of unplug if it has to support guests that lack PV/virtio/enlightened/whatever drivers.

No, it's Xen only and really should be legacy.  Ideally one would just 
have devices supported at all levels from firmware to kernel.

>> 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 ?
> 
> Prior this commit, piix_ide_reset was only called when the entire
> emulated machine was reset. Like: never. With this commit,
> piix_ide_reset will be called from pci_piix3_xen_ide_unplug. For some
> reason it confuses the emulated USB hardware. Why it does confused
> it, no idea.

> I wonder what the purpose of the qdev_reset_all() call really is. It
> is 10 years old. It might be stale.

piix_ide_reset is only calling ide_bus_reset, and from there ide_reset 
and bmdma_reset.  All of these functions do just two things: reset 
internal registers and ensure pending I/O is completed or canceled.  The 
latter is indeed unnecessary; drain/flush/detach is already done before 
the call to qdev_reset_all.

But the fact that it breaks USB is weird.  That's the part that needs to 
be debugged, because changing IDE to unbreak USB needs an explanation 
even if it's the right thing to do.

If you don't want to debug it, removing the qdev_reset_all call might do 
the job; you'll have to see what the Xen maintainers think of it.  But 
if you don't debug the USB issue now, it will come back later almost surely.

Paolo