Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine

[Stefan Hajnoczi]

On Wed, Apr 28, 2021 at 04:18:17PM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > On Tue, Apr 27, 2021 at 02:02:27PM -0400, John Snow wrote:
> >> On 4/27/21 1:54 PM, Philippe Mathieu-Daudé wrote:
> >> > On 4/27/21 7:16 PM, John Snow wrote:
> >> > > On 4/27/21 9:54 AM, Stefan Hajnoczi wrote:
> >> > > > I suggest fixing this at the qdev level. Make piix3-ide have a
> >> > > > sub-device that inherits from ISA_DEVICE so it can only be 
> >> > > > instantiated
> >> > > > when there's an ISA bus.
> >> > > > 
> >> > > > Stefan
> >> > > 
> >> > > My qdev knowledge is shaky. Does this imply that you agree with the
> >> > > direction of Thomas's patch, or do you just mean to disagree with Phil
> >> > > on his preferred course of action?
> >> > 
> >> > My understanding is a disagreement to both, with a 3rd direction :)
> >> > 
> >> > I agree with Stefan direction but I'm not sure (yet) that a sub-device
> >> > is the best (long-term) solution. I guess there is a design issue with
> >> > this device, and would like to understanding it first.
> >> > 
> >> > IIUC Stefan says the piix3-ide is both a PCI and IDE device, but QOM
> >> > only allow a single parent. Multiple QOM inheritance is resolved as
> >> > interfaces, but PCI/IDE qdev aren't interfaces, rather abstract objects.
> >> > So he suggests to embed an IDE device within the PCI piix3-ide device.
> >> > 
> >> > My view is the PIIX is a chipset that share stuffs between components,
> >> > and the IDE bus belongs to the chipset PCI root (or eventually the
> >> > PCI-ISA bridge, function #0). The IDE function would use the IDE bus
> >> > from its root parent as a linked property.
> >> > My problem is currently this device is user-creatable as a Frankenstein
> >> > single PCI function, out of its chipset. I'm not sure yet this is a
> >> > dead end or I could work something out.
> >> > 
> >> > Regards,
> >> > 
> >> > Phil.
> >> > 
> >> 
> >> It sounds complicated. In the meantime, I think I am favor of taking
> >> Thomas's patch because it merely adds some error routing to allow us to
> >> avoid a crash. The core organizational issues of the IDE device(s) will
> >> remain and can be solved later as needed.
> >
> > The approach in this patch is okay but we should keep in mind it only
> > solves piix3-ide. ISA provides a non-qdev backdoor API and there may be
> > more instances of this type of bug.
> >
> > A qdev fix would address the root cause and make it possible to drop the
> > backdoor API, but that's probably too much work for little benefit.
> 
> What do you mean by backdoor API?  Global @isabus?

Yes. It's also strange that isa_get_irq(ISADevice *dev, unsigned isairq)
accepts dev = NULL as a valid argument.

Stefan

signature.asc
Description: PGP signature