Stefano Stabellini <stefano.stabell...@eu.citrix.com> writes: > On Fri, 30 Jan 2015, Markus Armbruster wrote: >> Stefano Stabellini <stefano.stabell...@eu.citrix.com> writes: >> >> > On Thu, 29 Jan 2015, Markus Armbruster wrote: >> >> Stefano Stabellini <stefano.stabell...@eu.citrix.com> writes: >> >> >> >> > On Thu, 29 Jan 2015, Markus Armbruster wrote: >> >> >> Reproducer: qemu -nodefaults -S -display none -device xen-platform >> >> >> >> >> >> Yes, xen-platform makes no sense without Xen, but it shouldn't crash. >> >> > >> >> > Is it just a matter of doing the following? >> >> > >> >> > diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c >> >> > index 28b324a..40ae1f3 100644 >> >> > --- a/hw/i386/xen/xen_platform.c >> >> > +++ b/hw/i386/xen/xen_platform.c >> >> > @@ -177,6 +177,10 @@ static void platform_fixed_ioport_writeb(void >> >> > *opaque, uint32_t addr, uint32_t v >> >> > { >> >> > PCIXenPlatformState *s = opaque; >> >> > >> >> > + if (!xen_enabled()) { >> >> > + return; >> >> > + } >> >> > + >> >> > switch (addr) { >> >> > case 0: /* Platform flags */ { >> >> > hvmmem_type_t mem_type = (val & PFFLAG_ROM_LOCK) ? >> >> >> >> Fixes the crash for me. >> >> >> >> Should Xen-only devices fail to realize when !xen_enabled()? >> > >> > Accelerators are configured after device registration unfortunately. >> >> Realization happens even later, doesn't it? > > Yes, you are right. > > >> With this patch: >> >> @@ -389,6 +393,11 @@ static int xen_platform_initfn(PCIDevice *dev) >> PCIXenPlatformState *d = XEN_PLATFORM(dev); >> uint8_t *pci_conf; >> >> + if (!xen_enabled()) { >> + error_report("Nope"); >> + return -1; >> + } >> + >> pci_conf = dev->config; >> >> pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | >> PCI_COMMAND_MEMORY); >> >> I get: >> >> $ upstream-qemu -nodefaults -S -display none -device xen-platform >> upstream-qemu: -device xen-platform: Nope >> upstream-qemu: -device xen-platform: Device initialization failed. >> upstream-qemu: -device xen-platform: Device 'xen-platform' could not be >> initialized >> [Exit 1 ] >> >> Note that error_report() is a bit problematic in init methods. The best >> solution is to convert the device to realize (requires my "pci: Partial >> conversion to realize" series) and use error_setg(). > > I agree that this is much better than breaking at runtime for no clear > reason. Are you going to submit a proper patch?
Can do. Best for all the xen-only PCI devices, not just this one. I'd prefer to do it on top of a conversion to realize. Makes the task depend on [PATCH 00/10] pci: Partial conversion to realize.