| Message ID | 1520413694-1271-1-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/ppc/prep: Fix implicit creation of "-drive if=scsi" devices | expand |
On Wed, Mar 07, 2018 at 10:08:14AM +0100, Thomas Huth wrote: > The global hack for creating SCSI devices has recently been removed, > but this apparently broke SCSI devices on some boards that were not > ready for this change yet. For the 40p machine you now get: > > $ ppc64-softmmu/qemu-system-ppc64 -M 40p -cdrom x.iso > qemu-system-ppc64: -cdrom x.iso: machine type does not support if=scsi,bus=0,unit=2 > > Fix it by providing a lsi53c810_create() function that takes care > of calling scsi_bus_legacy_handle_cmdline() after creating the > corresponding SCSI controller. > > Fixes: 1454509726719e0933c800fad00d6999752688ea > Signed-off-by: Thomas Huth <thuth@redhat.com> Applied, thanks. > --- > hw/ppc/prep.c | 2 +- > hw/scsi/lsi53c895a.c | 7 +++++++ > include/hw/pci/pci.h | 1 + > 3 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c > index 096d4d4..3361509 100644 > --- a/hw/ppc/prep.c > +++ b/hw/ppc/prep.c > @@ -788,7 +788,7 @@ static void ibm_40p_init(MachineState *machine) > qdev_prop_set_uint32(dev, "equipment", 0xc0); > qdev_init_nofail(dev); > > - pci_create_simple(pci_bus, PCI_DEVFN(1, 0), "lsi53c810"); > + lsi53c810_create(pci_bus, PCI_DEVFN(1, 0)); > > /* XXX: s3-trio at PCI_DEVFN(2, 0) */ > pci_vga_init(pci_bus); > diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c > index f3d4c4d..160657f 100644 > --- a/hw/scsi/lsi53c895a.c > +++ b/hw/scsi/lsi53c895a.c > @@ -2279,3 +2279,10 @@ void lsi53c895a_create(PCIBus *bus) > > scsi_bus_legacy_handle_cmdline(&s->bus); > } > + > +void lsi53c810_create(PCIBus *bus, int devfn) > +{ > + LSIState *s = LSI53C895A(pci_create_simple(bus, devfn, "lsi53c810")); > + > + scsi_bus_legacy_handle_cmdline(&s->bus); > +} > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index d8c18c7..e255941 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -708,6 +708,7 @@ PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name); > PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name); > > void lsi53c895a_create(PCIBus *bus); > +void lsi53c810_create(PCIBus *bus, int devfn); > > qemu_irq pci_allocate_irq(PCIDevice *pci_dev); > void pci_set_irq(PCIDevice *pci_dev, int level);
Le 07/03/2018 à 10:08, Thomas Huth a écrit : > The global hack for creating SCSI devices has recently been removed, > but this apparently broke SCSI devices on some boards that were not > ready for this change yet. For the 40p machine you now get: > > $ ppc64-softmmu/qemu-system-ppc64 -M 40p -cdrom x.iso > qemu-system-ppc64: -cdrom x.iso: machine type does not support if=scsi,bus=0,unit=2 > > Fix it by providing a lsi53c810_create() function that takes care > of calling scsi_bus_legacy_handle_cmdline() after creating the > corresponding SCSI controller. > > Fixes: 1454509726719e0933c800fad00d6999752688ea > Signed-off-by: Thomas Huth <thuth@redhat.com> Why is it required? - because SCSI adapter is not up to date to QEMU standards (QOM, ...)? - because board is not up to date to QEMU standards (QOM, ...)? - because board is using SCSI devices by default? (mc->block_default_type = IF_SCSI) ? In 2 first cases, what is missing? In third case, maybe it may be better to put it in generic code? You just fixed 40p and MIPS Jazz machines, but sparc/SS-10 (and other) also have the same problem... Hervé > --- > hw/ppc/prep.c | 2 +- > hw/scsi/lsi53c895a.c | 7 +++++++ > include/hw/pci/pci.h | 1 + > 3 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c > index 096d4d4..3361509 100644 > --- a/hw/ppc/prep.c > +++ b/hw/ppc/prep.c > @@ -788,7 +788,7 @@ static void ibm_40p_init(MachineState *machine) > qdev_prop_set_uint32(dev, "equipment", 0xc0); > qdev_init_nofail(dev); > > - pci_create_simple(pci_bus, PCI_DEVFN(1, 0), "lsi53c810"); > + lsi53c810_create(pci_bus, PCI_DEVFN(1, 0)); > > /* XXX: s3-trio at PCI_DEVFN(2, 0) */ > pci_vga_init(pci_bus); > diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c > index f3d4c4d..160657f 100644 > --- a/hw/scsi/lsi53c895a.c > +++ b/hw/scsi/lsi53c895a.c > @@ -2279,3 +2279,10 @@ void lsi53c895a_create(PCIBus *bus) > > scsi_bus_legacy_handle_cmdline(&s->bus); > } > + > +void lsi53c810_create(PCIBus *bus, int devfn) > +{ > + LSIState *s = LSI53C895A(pci_create_simple(bus, devfn, "lsi53c810")); > + > + scsi_bus_legacy_handle_cmdline(&s->bus); > +} > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index d8c18c7..e255941 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -708,6 +708,7 @@ PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name); > PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name); > > void lsi53c895a_create(PCIBus *bus); > +void lsi53c810_create(PCIBus *bus, int devfn); > > qemu_irq pci_allocate_irq(PCIDevice *pci_dev); > void pci_set_irq(PCIDevice *pci_dev, int level); >
On 08.03.2018 07:58, Hervé Poussineau wrote: > Le 07/03/2018 à 10:08, Thomas Huth a écrit : >> The global hack for creating SCSI devices has recently been removed, >> but this apparently broke SCSI devices on some boards that were not >> ready for this change yet. For the 40p machine you now get: >> >> $ ppc64-softmmu/qemu-system-ppc64 -M 40p -cdrom x.iso >> qemu-system-ppc64: -cdrom x.iso: machine type does not support >> if=scsi,bus=0,unit=2 >> >> Fix it by providing a lsi53c810_create() function that takes care >> of calling scsi_bus_legacy_handle_cmdline() after creating the >> corresponding SCSI controller. >> >> Fixes: 1454509726719e0933c800fad00d6999752688ea >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > Why is it required? > - because SCSI adapter is not up to date to QEMU standards (QOM, ...)? > - because board is not up to date to QEMU standards (QOM, ...)? > - because board is using SCSI devices by default? > (mc->block_default_type = IF_SCSI) ? > > In 2 first cases, what is missing? > In third case, maybe it may be better to put it in generic code? It's the third case. The "generic" code was just removed with commit 1454509726719e0933 since it was considered as a big hack. The generic code should not have to guess to which SCSI adapter a SCSI drive should be attached to. That's the job of the board init code, and this is what this patch is doing now for the 40p machine. Other boards like the "pseries" machine were doing this since a long time already (see the spapr_vscsi_create() function in hw/scsi/spapr_vscsi.c for example). > You just fixed 40p and MIPS Jazz machines, but sparc/SS-10 (and other) > also have the same problem... I also posted a patch for the Sparc machines, you can find it here: https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg01830.html Thomas
Le 07/03/2018 à 10:08, Thomas Huth a écrit : > The global hack for creating SCSI devices has recently been removed, > but this apparently broke SCSI devices on some boards that were not > ready for this change yet. For the 40p machine you now get: > > $ ppc64-softmmu/qemu-system-ppc64 -M 40p -cdrom x.iso > qemu-system-ppc64: -cdrom x.iso: machine type does not support if=scsi,bus=0,unit=2 > > Fix it by providing a lsi53c810_create() function that takes care > of calling scsi_bus_legacy_handle_cmdline() after creating the > corresponding SCSI controller. > > Fixes: 1454509726719e0933c800fad00d6999752688ea > Signed-off-by: Thomas Huth <thuth@redhat.com> Reviewed-by: Hervé Poussineau <hpoussin@reactos.org> > --- > hw/ppc/prep.c | 2 +- > hw/scsi/lsi53c895a.c | 7 +++++++ > include/hw/pci/pci.h | 1 + > 3 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c > index 096d4d4..3361509 100644 > --- a/hw/ppc/prep.c > +++ b/hw/ppc/prep.c > @@ -788,7 +788,7 @@ static void ibm_40p_init(MachineState *machine) > qdev_prop_set_uint32(dev, "equipment", 0xc0); > qdev_init_nofail(dev); > > - pci_create_simple(pci_bus, PCI_DEVFN(1, 0), "lsi53c810"); > + lsi53c810_create(pci_bus, PCI_DEVFN(1, 0)); > > /* XXX: s3-trio at PCI_DEVFN(2, 0) */ > pci_vga_init(pci_bus); > diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c > index f3d4c4d..160657f 100644 > --- a/hw/scsi/lsi53c895a.c > +++ b/hw/scsi/lsi53c895a.c > @@ -2279,3 +2279,10 @@ void lsi53c895a_create(PCIBus *bus) > > scsi_bus_legacy_handle_cmdline(&s->bus); > } > + > +void lsi53c810_create(PCIBus *bus, int devfn) > +{ > + LSIState *s = LSI53C895A(pci_create_simple(bus, devfn, "lsi53c810")); > + > + scsi_bus_legacy_handle_cmdline(&s->bus); > +} > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index d8c18c7..e255941 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -708,6 +708,7 @@ PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name); > PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name); > > void lsi53c895a_create(PCIBus *bus); > +void lsi53c810_create(PCIBus *bus, int devfn); > > qemu_irq pci_allocate_irq(PCIDevice *pci_dev); > void pci_set_irq(PCIDevice *pci_dev, int level); >
On 8 March 2018 at 07:09, Thomas Huth <thuth@redhat.com> wrote: > It's the third case. The "generic" code was just removed with commit > 1454509726719e0933 since it was considered as a big hack. The generic > code should not have to guess to which SCSI adapter a SCSI drive should > be attached to. That's the job of the board init code, and this is what > this patch is doing now for the 40p machine. > > Other boards like the "pseries" machine were doing this since a long > time already (see the spapr_vscsi_create() function in > hw/scsi/spapr_vscsi.c for example). > >> You just fixed 40p and MIPS Jazz machines, but sparc/SS-10 (and other) >> also have the same problem... > > I also posted a patch for the Sparc machines, you can find it here: > > https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg01830.html Are we sure this is the entire list of machines that use this? Can we in general try to avoid removing generic code features until we've checked and fixed everything that relies on them? x86 pc is not the only system we support... thanks -- PMM
On 08.03.2018 11:07, Peter Maydell wrote: > On 8 March 2018 at 07:09, Thomas Huth <thuth@redhat.com> wrote: >> It's the third case. The "generic" code was just removed with commit >> 1454509726719e0933 since it was considered as a big hack. The generic >> code should not have to guess to which SCSI adapter a SCSI drive should >> be attached to. That's the job of the board init code, and this is what >> this patch is doing now for the 40p machine. >> >> Other boards like the "pseries" machine were doing this since a long >> time already (see the spapr_vscsi_create() function in >> hw/scsi/spapr_vscsi.c for example). >> >>> You just fixed 40p and MIPS Jazz machines, but sparc/SS-10 (and other) >>> also have the same problem... >> >> I also posted a patch for the Sparc machines, you can find it here: >> >> https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg01830.html > > Are we sure this is the entire list of machines that use this? Yes. The problem only occurs for boards that are using block_default_type = IF_SCSI. $ grep -rl IF_SCSI hw/ hw/arm/versatilepb.c hw/arm/realview.c hw/mips/mips_jazz.c hw/ppc/prep.c hw/ppc/spapr.c hw/scsi/scsi-bus.c hw/sparc/sun4m.c hw/hppa/machine.c versatilepb, realview and the hppa machine were already using lsi53c895a_create() that takes care of this already. spapr is properly using scsi_bus_legacy_handle_cmdline() in spapr_vscsi_create(). And for jazz, prep and sun4m, I sent the patches to the list yesterday. > Can we in general try to avoid removing generic code features > until we've checked and fixed everything that relies on them? x86 pc > is not the only system we support... Sure. You know, I'm a ppc / s390x guy, so I am very aware of other platforms, too. In this case it was just a bad misunderstanding - I did not expect that the code would still be used on other platforms, too. Sorry for that. The problem is rather that we still need more automatic regression tests for stuff like this. I already have got something in mind for a new qtest: If "mkisofs" or "genisoimage" is available, we could create a boot CD-ROM automatically using the boot code from tests/boot-sector.c. Then the test could try to boot from that CD-ROM to check whether "--cdrom" is still working as expected. What do you think, does that sound reasonable? Thomas
On 8 March 2018 at 10:26, Thomas Huth <thuth@redhat.com> wrote: > On 08.03.2018 11:07, Peter Maydell wrote: >> Are we sure this is the entire list of machines that use this? > > Yes. The problem only occurs for boards that are using > block_default_type = IF_SCSI. > > $ grep -rl IF_SCSI hw/ > hw/arm/versatilepb.c > hw/arm/realview.c > hw/mips/mips_jazz.c > hw/ppc/prep.c > hw/ppc/spapr.c > hw/scsi/scsi-bus.c > hw/sparc/sun4m.c > hw/hppa/machine.c > > versatilepb, realview and the hppa machine were already using > lsi53c895a_create() that takes care of this already. spapr is properly > using scsi_bus_legacy_handle_cmdline() in spapr_vscsi_create(). And for > jazz, prep and sun4m, I sent the patches to the list yesterday. Cool. >> Can we in general try to avoid removing generic code features >> until we've checked and fixed everything that relies on them? x86 pc >> is not the only system we support... > > Sure. You know, I'm a ppc / s390x guy, so I am very aware of other > platforms, too. In this case it was just a bad misunderstanding - I did > not expect that the code would still be used on other platforms, too. > Sorry for that. Sorry for being grumpy -- I should have skipped sending that mail til I'd had some more coffee. > The problem is rather that we still need more automatic regression tests > for stuff like this. I already have got something in mind for a new > qtest: If "mkisofs" or "genisoimage" is available, we could create a > boot CD-ROM automatically using the boot code from tests/boot-sector.c. > Then the test could try to boot from that CD-ROM to check whether > "--cdrom" is still working as expected. What do you think, does that > sound reasonable? That's probably a good plan, though it wouldn't be able to cover boards like versatile/realview that can't boot from a cdrom. thanks -- PMM
On 08/03/2018 11:45, Peter Maydell wrote: > On 8 March 2018 at 10:26, Thomas Huth <thuth@redhat.com> wrote: >> The problem is rather that we still need more automatic regression tests >> for stuff like this. I already have got something in mind for a new >> qtest: If "mkisofs" or "genisoimage" is available, we could create a >> boot CD-ROM automatically using the boot code from tests/boot-sector.c. >> Then the test could try to boot from that CD-ROM to check whether >> "--cdrom" is still working as expected. What do you think, does that >> sound reasonable? > > That's probably a good plan, though it wouldn't be able to cover > boards like versatile/realview that can't boot from a cdrom. We don't need to boot, we "only" need to have a qtest that sends an INQUIRY command. The code to bring up the LSI HBA can be copied from SeaBIOS, what's missing is libqos ports for the various PCI bridges... Paolo
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c index 096d4d4..3361509 100644 --- a/hw/ppc/prep.c +++ b/hw/ppc/prep.c @@ -788,7 +788,7 @@ static void ibm_40p_init(MachineState *machine) qdev_prop_set_uint32(dev, "equipment", 0xc0); qdev_init_nofail(dev); - pci_create_simple(pci_bus, PCI_DEVFN(1, 0), "lsi53c810"); + lsi53c810_create(pci_bus, PCI_DEVFN(1, 0)); /* XXX: s3-trio at PCI_DEVFN(2, 0) */ pci_vga_init(pci_bus); diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c index f3d4c4d..160657f 100644 --- a/hw/scsi/lsi53c895a.c +++ b/hw/scsi/lsi53c895a.c @@ -2279,3 +2279,10 @@ void lsi53c895a_create(PCIBus *bus) scsi_bus_legacy_handle_cmdline(&s->bus); } + +void lsi53c810_create(PCIBus *bus, int devfn) +{ + LSIState *s = LSI53C895A(pci_create_simple(bus, devfn, "lsi53c810")); + + scsi_bus_legacy_handle_cmdline(&s->bus); +} diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index d8c18c7..e255941 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -708,6 +708,7 @@ PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name); PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name); void lsi53c895a_create(PCIBus *bus); +void lsi53c810_create(PCIBus *bus, int devfn); qemu_irq pci_allocate_irq(PCIDevice *pci_dev); void pci_set_irq(PCIDevice *pci_dev, int level);
The global hack for creating SCSI devices has recently been removed, but this apparently broke SCSI devices on some boards that were not ready for this change yet. For the 40p machine you now get: $ ppc64-softmmu/qemu-system-ppc64 -M 40p -cdrom x.iso qemu-system-ppc64: -cdrom x.iso: machine type does not support if=scsi,bus=0,unit=2 Fix it by providing a lsi53c810_create() function that takes care of calling scsi_bus_legacy_handle_cmdline() after creating the corresponding SCSI controller. Fixes: 1454509726719e0933c800fad00d6999752688ea Signed-off-by: Thomas Huth <thuth@redhat.com> --- hw/ppc/prep.c | 2 +- hw/scsi/lsi53c895a.c | 7 +++++++ include/hw/pci/pci.h | 1 + 3 files changed, 9 insertions(+), 1 deletion(-)