Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
On Tue, Jul 06, 2021 at 10:37:03AM +0200, Philippe Mathieu-Daudé wrote: > Stefan, IIRC the multi-process conclusion was we have to reject > PCI devices briding another (non-PCI) bus, such ISA / I2C / USB > / SD / ... because QEMU register the bus type globally and the > command line machinery resolves it to plug user-creatable devices, > so we can not share such buses. Is that correct? I'm not sure I understand, but I'll try: You can implement an out-of-process USB host controller (a PCI device), but QEMU will not be aware of devices on this out-of-process USB bus. If you're referring to a PCI IDE controller that is also exposed on the ISA bus, then that's hard to do. Maybe there would need to be a separate ISA-to-PCI bridge device so there's a clean separation between the PCI device and the ISA portion. The current multi-process QEMU protocol (and the upcoming vfio-user protocol) support PCI devices but not ISA devices. Stefan signature.asc Description: PGP signature
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
On 7/6/21 10:24 AM, Thomas Huth wrote: > On 16/04/2021 14.52, Thomas Huth wrote: >> QEMU currently crashes when the user tries to do something like: >> >> qemu-system-x86_64 -M x-remote -device piix3-ide > > It's now several months later already, and this crash has still not been > fixed yet. The next softfreeze is around the corner and the > "device-crash-test" still stumbles accross this problem. > If the other solutions are not mergable yet (what's the status here?), See this big thread about ISA vs PCI IDE modelling / design: https://www.mail-archive.com/qemu-devel@nongnu.org/msg809678.html TL;DR: short term we are screwed. long term, not worth it. Stefan, IIRC the multi-process conclusion was we have to reject PCI devices briding another (non-PCI) bus, such ISA / I2C / USB / SD / ... because QEMU register the bus type globally and the command line machinery resolves it to plug user-creatable devices, so we can not share such buses. Is that correct? > could we please include my patch for QEMU v6.1 instead, so that we get > this silenced in the device-crash-test script? Yes please. Regards, Phil. >> 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 >> --- >> 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(-)
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
On 16/04/2021 14.52, Thomas Huth wrote:
QEMU currently crashes when the user tries to do something like:
qemu-system-x86_64 -M x-remote -device piix3-ide
It's now several months later already, and this crash has still not been
fixed yet. The next softfreeze is around the corner and the
"device-crash-test" still stumbles accross this problem.
If the other solutions are not mergable yet (what's the status here?), could
we please include my patch for QEMU v6.1 instead, so that we get this
silenced in the device-crash-test script?
Thanks,
Thomas
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
---
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;
}
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, uint1
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
On 4/28/21 5: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?
Thomas
Hi Thomas. Looks like there's some more discussion, and Philippe has a
counter-proposal he wants to send.
I guess I'll de-queue this for now, and I'll see you on the review for
Philippe's series?
--js
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
On 4/28/21 5: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?
Thomas
All of the rest of this thread looks like longer term fixes. I've kept
this patch staged and will send a PR soonish, after I collate all of the
other hanging IDE/FDC fixes that are awaiting my attention.
--js
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
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 writes: > >> On Wed, Apr 28, 2021 at 04:18:17PM +0200, Markus Armbruster wrote: >>> Stefan Hajnoczi 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. >
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
Stefan Hajnoczi writes: > On Wed, Apr 28, 2021 at 04:18:17PM +0200, Markus Armbruster wrote: >> Stefan Hajnoczi 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.
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
On Wed, Apr 28, 2021 at 04:18:17PM +0200, Markus Armbruster wrote: > Stefan Hajnoczi 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
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
Stefan Hajnoczi 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.
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
Stefan Hajnoczi 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?
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
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"?
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
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
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
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
signature.asc
Description: PGP signature
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
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 signature.asc Description: PGP signature
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
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 signature.asc Description: PGP signature
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
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
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
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:-)
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
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
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
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.
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
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
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
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 signature.asc Description: PGP signature
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
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
> ---
> 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)));
> +}
> }
[PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
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
---
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;
}
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(piol
