diff mbox series

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

Message ID 20210416125256.2039734-1-thuth@redhat.com (mailing list archive)
State New
Headers show
Series hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine | expand

Commit Message

Thomas Huth April 16, 2021, 12:52 p.m. UTC
QEMU currently crashes when the user tries to do something like:

 qemu-system-x86_64 -M x-remote -device piix3-ide

This happens because the "isabus" variable is not initialized with
the x-remote machine yet. Add a proper check for this condition
and propagate the error to the caller, so we can fail there gracefully.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/ide/ioport.c           | 16 ++++++++++------
 hw/ide/piix.c             | 22 +++++++++++++++++-----
 hw/isa/isa-bus.c          | 14 ++++++++++----
 include/hw/ide/internal.h |  2 +-
 include/hw/isa/isa.h      | 13 ++++++++-----
 5 files changed, 46 insertions(+), 21 deletions(-)

Comments

Philippe Mathieu-Daudé April 27, 2021, 1:27 p.m. UTC | #1
Hi Thomas,

On 4/16/21 2:52 PM, Thomas Huth wrote:
> QEMU currently crashes when the user tries to do something like:
> 
>  qemu-system-x86_64 -M x-remote -device piix3-ide
> 
> This happens because the "isabus" variable is not initialized with
> the x-remote machine yet.

The qdev contract is everything must be ready at realize() time,
so your safe check in pci_piix_ide_realize() seems alright.

But something doesn't sound right... If no ISA bus is present,
we shouldn't have executed so many code, rather have bailed out
earlier.

How does it work with the x-remote machine? The bus will become
available later upon remote connection?

piix3-ide is a PCI function device. Personally I consider it part
of the PIIX3 chipset, but we prefer to instantiate it separately.
So per Kconfig, piix3-ide is user-creatable by any machine providing
a PCI bus.

See hw/ide/Kconfig:

config IDE_CORE
    bool

config IDE_PCI
    bool
    depends on PCI
    select IDE_CORE

config IDE_ISA
    bool
    depends on ISA_BUS
    select IDE_QDEV

config IDE_PIIX
    bool
    select IDE_PCI
    select IDE_QDEV

and x-remote machine:

config MULTIPROCESS
    bool
    depends on PCI && PCI_EXPRESS && KVM
    select REMOTE_PCIHOST

1/ This KVM check is dubious, and isn't enforced at runtime
   in the machine.

2/ There is no Kconfig dependency IDE_PIIX -> ISA_BUS, apparently
   we need it.


Anyhow I think it would be easier to manage the ISA code if the bus
were created explicitly, not under our feet. I have a WIP series
doing that, but it isn't easy (as with all very old QEMU code/devices).

> Add a proper check for this condition
> and propagate the error to the caller, so we can fail there gracefully.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/ide/ioport.c           | 16 ++++++++++------
>  hw/ide/piix.c             | 22 +++++++++++++++++-----
>  hw/isa/isa-bus.c          | 14 ++++++++++----
>  include/hw/ide/internal.h |  2 +-
>  include/hw/isa/isa.h      | 13 ++++++++-----
>  5 files changed, 46 insertions(+), 21 deletions(-)

>  static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>  {
>      PCIIDEState *d = PCI_IDE(dev);
>      uint8_t *pci_conf = dev->config;
> +    int rc;
>  
>      pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>  
> @@ -158,7 +166,11 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>  
>      vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>  
> -    pci_piix_init_ports(d);
> +    rc = pci_piix_init_ports(d);
> +    if (rc) {
> +        error_setg_errno(errp, -rc, "Failed to realize %s",
> +                         object_get_typename(OBJECT(dev)));
> +    }
>  }
Stefan Hajnoczi April 27, 2021, 1:54 p.m. UTC | #2
On Tue, Apr 27, 2021 at 03:27:40PM +0200, Philippe Mathieu-Daudé wrote:
> 2/ There is no Kconfig dependency IDE_PIIX -> ISA_BUS, apparently
>    we need it.

Adding an ISA_BUS Kconfig dependency won't solve the problem since the
qemu-system-x86_64 binary is built with many machine types. Most of then
include ISA_BUS so the IDE_PIIX device will be linked in and available
when -M x-remote is used. The crash will still occur.

> Anyhow I think it would be easier to manage the ISA code if the bus
> were created explicitly, not under our feet. I have a WIP series
> doing that, but it isn't easy (as with all very old QEMU code/devices).

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
John Snow April 27, 2021, 5:16 p.m. UTC | #3
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?

--js
Philippe Mathieu-Daudé April 27, 2021, 5:54 p.m. UTC | #4
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.
John Snow April 27, 2021, 6:02 p.m. UTC | #5
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.

Do you agree?

--js
John Snow April 27, 2021, 6:06 p.m. UTC | #6
On 4/16/21 8:52 AM, Thomas Huth wrote:
> QEMU currently crashes when the user tries to do something like:
> 
>   qemu-system-x86_64 -M x-remote -device piix3-ide
> 
> This happens because the "isabus" variable is not initialized with
> the x-remote machine yet. Add a proper check for this condition
> and propagate the error to the caller, so we can fail there gracefully.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Seems OK to me for now. I will trust that this won't get in the way of 
any deeper refactors Phil has planned, since this just adds error 
pathways to avoid something already broken, and doesn't change anything 
else.

I'm OK with that.

Reviewed-by: John Snow <jsnow@redhat.com>

Tentatively staged, pending further discussion.

> ---
>   hw/ide/ioport.c           | 16 ++++++++++------
>   hw/ide/piix.c             | 22 +++++++++++++++++-----
>   hw/isa/isa-bus.c          | 14 ++++++++++----
>   include/hw/ide/internal.h |  2 +-
>   include/hw/isa/isa.h      | 13 ++++++++-----
>   5 files changed, 46 insertions(+), 21 deletions(-)
> 


> diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
> index b613ff3bba..e6caa537fa 100644
> --- a/hw/ide/ioport.c
> +++ b/hw/ide/ioport.c
> @@ -50,15 +50,19 @@ static const MemoryRegionPortio ide_portio2_list[] = {
>       PORTIO_END_OF_LIST(),
>   };
>   
> -void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
> +int ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
>   {
> +    int ret;
> +
>       /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
>          bridge has been setup properly to always register with ISA.  */
> -    isa_register_portio_list(dev, &bus->portio_list,
> -                             iobase, ide_portio_list, bus, "ide");
> +    ret = isa_register_portio_list(dev, &bus->portio_list,
> +                                   iobase, ide_portio_list, bus, "ide");
>   
> -    if (iobase2) {
> -        isa_register_portio_list(dev, &bus->portio2_list,
> -                                 iobase2, ide_portio2_list, bus, "ide");
> +    if (ret == 0 && iobase2) {
> +        ret = isa_register_portio_list(dev, &bus->portio2_list,
> +                                       iobase2, ide_portio2_list, bus, "ide");
>       }
> +
> +    return ret;
>   }


Little funky instead of just checking and returning after the first 
portio list registration, you could leave a little more git blame intact 
by not doing this, but...

...Then again, I just acked a ton of stuff Phil moved around, so, 
whatever O:-)
Stefan Hajnoczi April 28, 2021, 9:15 a.m. UTC | #7
On Tue, Apr 27, 2021 at 07:54:21PM +0200, 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.

Kevin and Paolo previously pointed out that piix3-ide is sometimes used
with the Q35 machine type. The user-creatable piix3-ide device needs to
be deprecated before it can be dropped. That's a long process that
cannot fix the current crash any time soon.

I do support deprecating the user-creatable piix3-ide device in favor of
a proper Q35 Legacy IDE implementation. The main problem is this
involves a bunch of work and I'm not sure who would do it (the payoff is
not very high).

Stefan
Stefan Hajnoczi April 28, 2021, 9:22 a.m. UTC | #8
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.

Stefan
Stefan Hajnoczi April 28, 2021, 9:24 a.m. UTC | #9
On Fri, Apr 16, 2021 at 02:52:56PM +0200, Thomas Huth wrote:
> @@ -158,7 +166,11 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>  
>      vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>  
> -    pci_piix_init_ports(d);
> +    rc = pci_piix_init_ports(d);
> +    if (rc) {
> +        error_setg_errno(errp, -rc, "Failed to realize %s",
> +                         object_get_typename(OBJECT(dev)));
> +    }

Is there an error message explaining the reason for the failure
somewhere. I can't see one in the patch and imagine users will be
puzzled/frustrated by a generic "Failed to realize" error message. Can
you make it more meaningful?

Thanks,
Stefan
Thomas Huth April 28, 2021, 9:32 a.m. UTC | #10
On 28/04/2021 11.24, Stefan Hajnoczi wrote:
> On Fri, Apr 16, 2021 at 02:52:56PM +0200, Thomas Huth wrote:
>> @@ -158,7 +166,11 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>>   
>>       vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>>   
>> -    pci_piix_init_ports(d);
>> +    rc = pci_piix_init_ports(d);
>> +    if (rc) {
>> +        error_setg_errno(errp, -rc, "Failed to realize %s",
>> +                         object_get_typename(OBJECT(dev)));
>> +    }
> 
> Is there an error message explaining the reason for the failure
> somewhere. I can't see one in the patch and imagine users will be
> puzzled/frustrated by a generic "Failed to realize" error message. Can
> you make it more meaningful?

Do you have a suggestion for a better message?

  Thomas
Philippe Mathieu-Daudé April 28, 2021, 10:53 a.m. UTC | #11
On 4/28/21 11:32 AM, Thomas Huth wrote:
> On 28/04/2021 11.24, Stefan Hajnoczi wrote:
>> On Fri, Apr 16, 2021 at 02:52:56PM +0200, Thomas Huth wrote:
>>> @@ -158,7 +166,11 @@ static void pci_piix_ide_realize(PCIDevice *dev,
>>> Error **errp)
>>>         vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>>>   -    pci_piix_init_ports(d);
>>> +    rc = pci_piix_init_ports(d);
>>> +    if (rc) {
>>> +        error_setg_errno(errp, -rc, "Failed to realize %s",
>>> +                         object_get_typename(OBJECT(dev)));
>>> +    }
>>
>> Is there an error message explaining the reason for the failure
>> somewhere. I can't see one in the patch and imagine users will be
>> puzzled/frustrated by a generic "Failed to realize" error message. Can
>> you make it more meaningful?
> 
> Do you have a suggestion for a better message?

At this point it is hard to do better... Else we'd need to propagate
a meaningful Error* from ide_init_ioport(), and emit something like
"Failed to initialize the ISA bus"?
Markus Armbruster April 28, 2021, 2:18 p.m. UTC | #12
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?
Markus Armbruster April 28, 2021, 2:21 p.m. UTC | #13
Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Tue, Apr 27, 2021 at 07:54:21PM +0200, 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.
>
> Kevin and Paolo previously pointed out that piix3-ide is sometimes used
> with the Q35 machine type. The user-creatable piix3-ide device needs to
> be deprecated before it can be dropped. That's a long process that
> cannot fix the current crash any time soon.
>
> I do support deprecating the user-creatable piix3-ide device in favor of
> a proper Q35 Legacy IDE implementation. The main problem is this
> involves a bunch of work and I'm not sure who would do it (the payoff is
> not very high).

In my opinion, letting users plug device models for PCI *functions* as
if they were *devices* was a mistake.  Compounding the mistake of not
modelling the difference between PCI function and PCI device.

The more of them we can deprecate, the better.
Stefan Hajnoczi April 28, 2021, 6:43 p.m. UTC | #14
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
Markus Armbruster April 29, 2021, 6:08 a.m. UTC | #15
Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Wed, Apr 28, 2021 at 04:18:17PM +0200, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> writes:

[...]

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

@isabus is static in hw/isa/isa-bus.c.  Uses:

* Limit isa_bus_new() to one ISA bus.  Arbitrary restriction; multiple
  ISA buses could work with suitable memory mapping and IRQ wiring.
  "Single ISA bus" assumptions could of course hide elsewhere in the
  code.

* Implied argument to isa_get_irq(), isa_register_ioport(),
  isa_register_portio_list(), isa_address_space(),
  isa_address_space_io().

  isa_get_irq() asserts that a non-null @dev is a child of @isabus.
  This means we don't actually need @isabus, except when @dev is null.
  I suspect two separate functions would be cleaner: one taking an
  ISABus * argument, and a wrapper taking an ISADevice * argument.

  isa_address_space() and isa_address_space_io() work the same, less the
  assertion.

  isa_register_ioport() and isa_register_portio_list() take a non-null
  @dev argument.  They don't actually need @isabus.

To eliminate global @isabus, we need to fix up the callers passing null
@dev.  Clean solution: plumb the ISABus returned by isa_bus_new() to the
call sites.  Where that's impractical, we can also get it from QOM, like
build_dsdt_microvm() does.
Philippe Mathieu-Daudé May 3, 2021, 5:53 p.m. UTC | #16
Hi Elena,

+Mark

You asked to use the next KVM-external call slot to talk about
the ISA bus issues. I haven't scheduled the call because it seems
this thread helped to figure the problems and Markus's analysis
resumed them all. From here it should be clearer to see what has
to be done to go where we want to be from where we are :)

What do you think (in particular, from Markus list)?

On 4/29/21 8:08 AM, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
>> On Wed, Apr 28, 2021 at 04:18:17PM +0200, Markus Armbruster wrote:
>>> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> [...]
> 
>>>> 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.
> 
> @isabus is static in hw/isa/isa-bus.c.  Uses:
> 
> * Limit isa_bus_new() to one ISA bus.  Arbitrary restriction; multiple
>   ISA buses could work with suitable memory mapping and IRQ wiring.
>   "Single ISA bus" assumptions could of course hide elsewhere in the
>   code.
> 
> * Implied argument to isa_get_irq(), isa_register_ioport(),
>   isa_register_portio_list(), isa_address_space(),
>   isa_address_space_io().
> 
>   isa_get_irq() asserts that a non-null @dev is a child of @isabus.
>   This means we don't actually need @isabus, except when @dev is null.
>   I suspect two separate functions would be cleaner: one taking an
>   ISABus * argument, and a wrapper taking an ISADevice * argument.
> 
>   isa_address_space() and isa_address_space_io() work the same, less the
>   assertion.
> 
>   isa_register_ioport() and isa_register_portio_list() take a non-null
>   @dev argument.  They don't actually need @isabus.
> 
> To eliminate global @isabus, we need to fix up the callers passing null
> @dev.  Clean solution: plumb the ISABus returned by isa_bus_new() to the
> call sites.  Where that's impractical, we can also get it from QOM, like
> build_dsdt_microvm() does.
>
diff mbox series

Patch

diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index b613ff3bba..e6caa537fa 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -50,15 +50,19 @@  static const MemoryRegionPortio ide_portio2_list[] = {
     PORTIO_END_OF_LIST(),
 };
 
-void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
+int ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
 {
+    int ret;
+
     /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
        bridge has been setup properly to always register with ISA.  */
-    isa_register_portio_list(dev, &bus->portio_list,
-                             iobase, ide_portio_list, bus, "ide");
+    ret = isa_register_portio_list(dev, &bus->portio_list,
+                                   iobase, ide_portio_list, bus, "ide");
 
-    if (iobase2) {
-        isa_register_portio_list(dev, &bus->portio2_list,
-                                 iobase2, ide_portio2_list, bus, "ide");
+    if (ret == 0 && iobase2) {
+        ret = isa_register_portio_list(dev, &bus->portio2_list,
+                                       iobase2, ide_portio2_list, bus, "ide");
     }
+
+    return ret;
 }
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index b9860e35a5..d3e738320b 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -26,6 +26,7 @@ 
 #include "qemu/osdep.h"
 #include "hw/pci/pci.h"
 #include "migration/vmstate.h"
+#include "qapi/error.h"
 #include "qemu/module.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
@@ -123,7 +124,8 @@  static void piix_ide_reset(DeviceState *dev)
     pci_conf[0x20] = 0x01; /* BMIBA: 20-23h */
 }
 
-static void pci_piix_init_ports(PCIIDEState *d) {
+static int pci_piix_init_ports(PCIIDEState *d)
+{
     static const struct {
         int iobase;
         int iobase2;
@@ -132,24 +134,30 @@  static void pci_piix_init_ports(PCIIDEState *d) {
         {0x1f0, 0x3f6, 14},
         {0x170, 0x376, 15},
     };
-    int i;
+    int i, ret;
 
     for (i = 0; i < 2; i++) {
         ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-        ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
-                        port_info[i].iobase2);
+        ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
+                              port_info[i].iobase2);
+        if (ret) {
+            return ret;
+        }
         ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
 
         bmdma_init(&d->bus[i], &d->bmdma[i], d);
         d->bmdma[i].bus = &d->bus[i];
         ide_register_restart_cb(&d->bus[i]);
     }
+
+    return 0;
 }
 
 static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
 {
     PCIIDEState *d = PCI_IDE(dev);
     uint8_t *pci_conf = dev->config;
+    int rc;
 
     pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
 
@@ -158,7 +166,11 @@  static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
 
     vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
 
-    pci_piix_init_ports(d);
+    rc = pci_piix_init_ports(d);
+    if (rc) {
+        error_setg_errno(errp, -rc, "Failed to realize %s",
+                         object_get_typename(OBJECT(dev)));
+    }
 }
 
 int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 7820068e6e..cffaa35e9c 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -131,13 +131,17 @@  void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
     isa_init_ioport(dev, start);
 }
 
-void isa_register_portio_list(ISADevice *dev,
-                              PortioList *piolist, uint16_t start,
-                              const MemoryRegionPortio *pio_start,
-                              void *opaque, const char *name)
+int isa_register_portio_list(ISADevice *dev,
+                             PortioList *piolist, uint16_t start,
+                             const MemoryRegionPortio *pio_start,
+                             void *opaque, const char *name)
 {
     assert(piolist && !piolist->owner);
 
+    if (!isabus) {
+        return -ENODEV;
+    }
+
     /* START is how we should treat DEV, regardless of the actual
        contents of the portio array.  This is how the old code
        actually handled e.g. the FDC device.  */
@@ -145,6 +149,8 @@  void isa_register_portio_list(ISADevice *dev,
 
     portio_list_init(piolist, OBJECT(dev), pio_start, opaque, name);
     portio_list_add(piolist, isabus->address_space_io, start);
+
+    return 0;
 }
 
 static void isa_device_init(Object *obj)
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 2d09162eeb..79172217cc 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -624,7 +624,7 @@  int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
                    int chs_trans, Error **errp);
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_exit(IDEState *s);
-void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
+int ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
 void ide_register_restart_cb(IDEBus *bus);
 
 void ide_exec_cmd(IDEBus *bus, uint32_t val);
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index ddaae89a85..d4417b34b6 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -132,12 +132,15 @@  void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start);
  * @portio: the ports, sorted by offset.
  * @opaque: passed into the portio callbacks.
  * @name: passed into memory_region_init_io.
+ *
+ * Returns: 0 on success, negative error code otherwise (e.g. if the
+ *          ISA bus is not available)
  */
-void isa_register_portio_list(ISADevice *dev,
-                              PortioList *piolist,
-                              uint16_t start,
-                              const MemoryRegionPortio *portio,
-                              void *opaque, const char *name);
+int isa_register_portio_list(ISADevice *dev,
+                             PortioList *piolist,
+                             uint16_t start,
+                             const MemoryRegionPortio *portio,
+                             void *opaque, const char *name);
 
 static inline ISABus *isa_bus_from_device(ISADevice *d)
 {