diff mbox

-device xen-platform crashes

Message ID alpine.DEB.2.02.1501291521000.9702@kaball.uk.xensource.com
State New
Headers show

Commit Message

Stefano Stabellini Jan. 29, 2015, 3:23 p.m. UTC 
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?


> Backtrace:
> 
> #0  0x00007fffef5a677c in hypercall_buffer_cache_lock ()
>    from /lib64/libxenctrl.so.4.3
> #1  0x00007fffef5a68f1 in xc.hypercall_buffer_alloc_pages ()
>    from /lib64/libxenctrl.so.4.3
> #2  0x00007fffef5a6a61 in xc.hypercall_buffer_alloc ()
>    from /lib64/libxenctrl.so.4.3
> #3  0x00007fffef59da54 in xc_hvm_set_mem_type () from /lib64/libxenctrl.so.4.3
> #4  0x00005555556d5bcd in platform_fixed_ioport_writeb (opaque=0x5555564713b0, 
>     addr=0, val=0) at /work/armbru/qemu/hw/i386/xen/xen_platform.c:184
> #5  0x00005555556d5ca4 in platform_fixed_ioport_reset (opaque=0x5555564713b0)
>     at /work/armbru/qemu/hw/i386/xen/xen_platform.c:238
> #6  0x00005555556d608c in platform_reset (dev=0x5555564713b0)
>     at /work/armbru/qemu/hw/i386/xen/xen_platform.c:417
> #7  0x00005555557e915b in device_reset (dev=0x5555564713b0)
>     at /work/armbru/qemu/hw/core/qdev.c:1269
> #8  0x00005555557e6a7b in qdev_reset_one (dev=0x5555564713b0, opaque=0x0)
>     at /work/armbru/qemu/hw/core/qdev.c:329
> #9  0x00005555557e7608 in qdev_walk_children (dev=0x5555564713b0, 
>     pre_devfn=0x0, pre_busfn=0x0, post_devfn=0x5555557e6a5f <qdev_reset_one>, 
>     post_busfn=0x5555557e6a82 <qbus_reset_one>, opaque=0x0)
>     at /work/armbru/qemu/hw/core/qdev.c:643
> #10 0x00005555557e74fb in qbus_walk_children (bus=0x5555563b1f00, 
>     pre_devfn=0x0, pre_busfn=0x0, post_devfn=0x5555557e6a5f <qdev_reset_one>, 
>     post_busfn=0x5555557e6a82 <qbus_reset_one>, opaque=0x0)
>     at /work/armbru/qemu/hw/core/qdev.c:601
> #11 0x00005555557e75cc in qdev_walk_children (dev=0x5555563d67d0, 
>     pre_devfn=0x0, pre_busfn=0x0, post_devfn=0x5555557e6a5f <qdev_reset_one>, 
>     post_busfn=0x5555557e6a82 <qbus_reset_one>, opaque=0x0)
>     at /work/armbru/qemu/hw/core/qdev.c:635
> #12 0x00005555557e74fb in qbus_walk_children (bus=0x5555563bd3c0, 
>     pre_devfn=0x0, pre_busfn=0x0, post_devfn=0x5555557e6a5f <qdev_reset_one>, 
>     post_busfn=0x5555557e6a82 <qbus_reset_one>, opaque=0x0)
>     at /work/armbru/qemu/hw/core/qdev.c:601
> #13 0x00005555557e6b57 in qbus_reset_all (bus=0x5555563bd3c0)
>     at /work/armbru/qemu/hw/core/qdev.c:350
> #14 0x00005555557e6b79 in qbus_reset_all_fn (opaque=0x5555563bd3c0)
>     at /work/armbru/qemu/hw/core/qdev.c:356
> #15 0x00005555557587b9 in qemu_devices_reset () at /work/armbru/qemu/vl.c:1614
> #16 0x0000555555758845 in qemu_system_reset (report=false)
>     at /work/armbru/qemu/vl.c:1627
> #17 0x0000555555760739 in main (argc=7, argv=0x7fffffffe0f8, 
>     envp=0x7fffffffe138) at /work/armbru/qemu/vl.c:4315
> 
> Sorry, no libxenctrl symbols installed.
>

Comments

Markus Armbruster Jan. 29, 2015, 3:33 p.m. UTC | #1 
Stefano Stabellini <stefano.stabellini@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()?
Stefano Stabellini Jan. 29, 2015, 5:42 p.m. UTC | #2 
On Thu, 29 Jan 2015, Markus Armbruster wrote:
> Stefano Stabellini <stefano.stabellini@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.
Markus Armbruster Jan. 30, 2015, 8:06 a.m. UTC | #3 
Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:

> On Thu, 29 Jan 2015, Markus Armbruster wrote:
>> Stefano Stabellini <stefano.stabellini@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?

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().
Stefano Stabellini Jan. 30, 2015, 12:45 p.m. UTC | #4 
On Fri, 30 Jan 2015, Markus Armbruster wrote:
> Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
> 
> > On Thu, 29 Jan 2015, Markus Armbruster wrote:
> >> Stefano Stabellini <stefano.stabellini@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?
Markus Armbruster Feb. 2, 2015, 9:24 a.m. UTC | #5 
Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:

> On Fri, 30 Jan 2015, Markus Armbruster wrote:
>> Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
>> 
>> > On Thu, 29 Jan 2015, Markus Armbruster wrote:
>> >> Stefano Stabellini <stefano.stabellini@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.
Stefano Stabellini Feb. 4, 2015, 3:37 p.m. UTC | #6 
On Mon, 2 Feb 2015, Markus Armbruster wrote:
> Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
> 
> > On Fri, 30 Jan 2015, Markus Armbruster wrote:
> >> Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
> >> 
> >> > On Thu, 29 Jan 2015, Markus Armbruster wrote:
> >> >> Stefano Stabellini <stefano.stabellini@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.

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

Patch

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) ?