Re: [Qemu-devel] -device xen-platform crashes

[Stefano Stabellini]

On Mon, 2 Feb 2015, Markus Armbruster wrote:
> Stefano Stabellini <address@hidden> writes:
> 
> > On Fri, 30 Jan 2015, Markus Armbruster wrote:
> >> Stefano Stabellini <address@hidden> writes:
> >> 
> >> > On Thu, 29 Jan 2015, Markus Armbruster wrote:
> >> >> Stefano Stabellini <address@hidden> 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.

Uhm.. what are the other Xen-only PCI devices?
All the other Xen "devices" are actually PV backends.


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