[Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices

[Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices

[Marcel Apfelbaum]

Combining rombar=0 with romfile=<path> is an user error,
silently dropping the romfile is a reasonable thing to do.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
v1 -> v2:
 After a discussion with Michael, Paolo and Alex, this
 patch silent drops the romfile instead of not allowing
 the hotplug.
 
 An OK from libvirt will be nice.

 hw/pci/pci.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 6ce75aa..cd7a403 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1776,7 +1776,12 @@ static int pci_qdev_init(DeviceState *qdev)
         pci_dev->romfile = g_strdup(pc->romfile);
         is_default_rom = true;
     }
-    pci_add_option_rom(pci_dev, is_default_rom);
+
+    rc = pci_add_option_rom(pci_dev, is_default_rom);
+    if (rc != 0) {
+        pci_unregister_device(DEVICE(pci_dev));
+        return rc;
+    }
 
     return 0;
 }
@@ -1937,6 +1942,15 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
          * for 0.11 compatibility.
          */
         int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
+
+        /*
+         * For hot-plugged devices silently ignore the option ROM
+         * if the rom bar is disabled.
+         */
+        if (DEVICE(pdev)->hotplugged) {
+            return 0;
+        }
+
         if (class == 0x0300) {
             rom_add_vga(pdev->romfile);
         } else {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices

[Marcel Apfelbaum]

On Mon, 2014-10-27 at 15:44 +0200, Marcel Apfelbaum wrote:
> Combining rombar=0 with romfile=<path> is an user error,
> silently dropping the romfile is a reasonable thing to do.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
> v1 -> v2:
>  After a discussion with Michael, Paolo and Alex, this
>  patch silent drops the romfile instead of not allowing
>  the hotplug.
>  
>  An OK from libvirt will be nice.
CC: Erik Blake <eblake@redhat.com>

Thanks,
Marcel

> 
>  hw/pci/pci.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 6ce75aa..cd7a403 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1776,7 +1776,12 @@ static int pci_qdev_init(DeviceState *qdev)
>          pci_dev->romfile = g_strdup(pc->romfile);
>          is_default_rom = true;
>      }
> -    pci_add_option_rom(pci_dev, is_default_rom);
> +
> +    rc = pci_add_option_rom(pci_dev, is_default_rom);
> +    if (rc != 0) {
> +        pci_unregister_device(DEVICE(pci_dev));
> +        return rc;
> +    }
>  
>      return 0;
>  }
> @@ -1937,6 +1942,15 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
>           * for 0.11 compatibility.
>           */
>          int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> +
> +        /*
> +         * For hot-plugged devices silently ignore the option ROM
> +         * if the rom bar is disabled.
> +         */
> +        if (DEVICE(pdev)->hotplugged) {
> +            return 0;
> +        }
> +
>          if (class == 0x0300) {
>              rom_add_vga(pdev->romfile);
>          } else {

Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices

[Michael S. Tsirkin]

On Mon, Oct 27, 2014 at 03:44:04PM +0200, Marcel Apfelbaum wrote:
> Combining rombar=0 with romfile=<path> is an user error,
> silently dropping the romfile is a reasonable thing to do.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>

How about failing adding the device instead?
Return error from pci_add_option_rom, and check at
calling sites?

> ---
> v1 -> v2:
>  After a discussion with Michael, Paolo and Alex, this
>  patch silent drops the romfile instead of not allowing
>  the hotplug.
>  
>  An OK from libvirt will be nice.
> 
>  hw/pci/pci.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 6ce75aa..cd7a403 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1776,7 +1776,12 @@ static int pci_qdev_init(DeviceState *qdev)
>          pci_dev->romfile = g_strdup(pc->romfile);
>          is_default_rom = true;
>      }
> -    pci_add_option_rom(pci_dev, is_default_rom);
> +
> +    rc = pci_add_option_rom(pci_dev, is_default_rom);
> +    if (rc != 0) {
> +        pci_unregister_device(DEVICE(pci_dev));
> +        return rc;
> +    }
>  
>      return 0;
>  }
> @@ -1937,6 +1942,15 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
>           * for 0.11 compatibility.
>           */
>          int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> +
> +        /*
> +         * For hot-plugged devices silently ignore the option ROM
> +         * if the rom bar is disabled.
> +         */
> +        if (DEVICE(pdev)->hotplugged) {
> +            return 0;
> +        }
> +
>          if (class == 0x0300) {
>              rom_add_vga(pdev->romfile);
>          } else {
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices

[Marcel Apfelbaum]

On Mon, 2014-10-27 at 16:52 +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 27, 2014 at 03:44:04PM +0200, Marcel Apfelbaum wrote:
> > Combining rombar=0 with romfile=<path> is an user error,
> > silently dropping the romfile is a reasonable thing to do.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> 
> How about failing adding the device instead?
> Return error from pci_add_option_rom, and check at
> calling sites?

This was was the prev version of this patch has done,
we have only one calling site:  pci_qdev_init.
I could tweak the prev version to return error on both
rom_add_vga/rom_add_option, but I was under the impression
that silently drop the romfile was discussion's decision.

I am fine both ways, as it is a user error and hopefully
used correctly by libvirt. I only want to avoid the crash.

Thanks,
Marcel

> 
> > ---
> > v1 -> v2:
> >  After a discussion with Michael, Paolo and Alex, this
> >  patch silent drops the romfile instead of not allowing
> >  the hotplug.
> >  
> >  An OK from libvirt will be nice.
> > 
> >  hw/pci/pci.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 6ce75aa..cd7a403 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1776,7 +1776,12 @@ static int pci_qdev_init(DeviceState *qdev)
> >          pci_dev->romfile = g_strdup(pc->romfile);
> >          is_default_rom = true;
> >      }
> > -    pci_add_option_rom(pci_dev, is_default_rom);
> > +
> > +    rc = pci_add_option_rom(pci_dev, is_default_rom);
> > +    if (rc != 0) {
> > +        pci_unregister_device(DEVICE(pci_dev));
> > +        return rc;
> > +    }
> >  
> >      return 0;
> >  }
> > @@ -1937,6 +1942,15 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> >           * for 0.11 compatibility.
> >           */
> >          int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> > +
> > +        /*
> > +         * For hot-plugged devices silently ignore the option ROM
> > +         * if the rom bar is disabled.
> > +         */
> > +        if (DEVICE(pdev)->hotplugged) {
> > +            return 0;
> > +        }
> > +
> >          if (class == 0x0300) {
> >              rom_add_vga(pdev->romfile);
> >          } else {
> > -- 
> > 1.8.3.1

Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices

[Michael S. Tsirkin]

On Mon, Oct 27, 2014 at 05:04:03PM +0200, Marcel Apfelbaum wrote:
> On Mon, 2014-10-27 at 16:52 +0200, Michael S. Tsirkin wrote:
> > On Mon, Oct 27, 2014 at 03:44:04PM +0200, Marcel Apfelbaum wrote:
> > > Combining rombar=0 with romfile=<path> is an user error,
> > > silently dropping the romfile is a reasonable thing to do.
> > > 
> > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > 
> > How about failing adding the device instead?
> > Return error from pci_add_option_rom, and check at
> > calling sites?
> 
> This was was the prev version of this patch has done,
> we have only one calling site:  pci_qdev_init.
> I could tweak the prev version to return error on both
> rom_add_vga/rom_add_option, but I was under the impression
> that silently drop the romfile was discussion's decision.
> 
> I am fine both ways, as it is a user error and hopefully
> used correctly by libvirt. I only want to avoid the crash.
> 
> Thanks,
> Marcel

Let's try the non silent error.
If someone is unhappy we can make it silent again, but
if it succeeds we'll have to keep it working forever.


> > > ---
> > > v1 -> v2:
> > >  After a discussion with Michael, Paolo and Alex, this
> > >  patch silent drops the romfile instead of not allowing
> > >  the hotplug.
> > >  
> > >  An OK from libvirt will be nice.
> > > 
> > >  hw/pci/pci.c | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 6ce75aa..cd7a403 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -1776,7 +1776,12 @@ static int pci_qdev_init(DeviceState *qdev)
> > >          pci_dev->romfile = g_strdup(pc->romfile);
> > >          is_default_rom = true;
> > >      }
> > > -    pci_add_option_rom(pci_dev, is_default_rom);
> > > +
> > > +    rc = pci_add_option_rom(pci_dev, is_default_rom);
> > > +    if (rc != 0) {
> > > +        pci_unregister_device(DEVICE(pci_dev));
> > > +        return rc;
> > > +    }
> > >  
> > >      return 0;
> > >  }
> > > @@ -1937,6 +1942,15 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> > >           * for 0.11 compatibility.
> > >           */
> > >          int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> > > +
> > > +        /*
> > > +         * For hot-plugged devices silently ignore the option ROM
> > > +         * if the rom bar is disabled.
> > > +         */
> > > +        if (DEVICE(pdev)->hotplugged) {
> > > +            return 0;
> > > +        }
> > > +
> > >          if (class == 0x0300) {
> > >              rom_add_vga(pdev->romfile);
> > >          } else {
> > > -- 
> > > 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1652 bytes --]

On 10/27/2014 10:03 AM, Michael S. Tsirkin wrote:
> On Mon, Oct 27, 2014 at 05:04:03PM +0200, Marcel Apfelbaum wrote:
>> On Mon, 2014-10-27 at 16:52 +0200, Michael S. Tsirkin wrote:
>>> On Mon, Oct 27, 2014 at 03:44:04PM +0200, Marcel Apfelbaum wrote:
>>>> Combining rombar=0 with romfile=<path> is an user error,
>>>> silently dropping the romfile is a reasonable thing to do.
>>>>
>>>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
>>>
>>> How about failing adding the device instead?
>>> Return error from pci_add_option_rom, and check at
>>> calling sites?
>>
>> This was was the prev version of this patch has done,
>> we have only one calling site:  pci_qdev_init.
>> I could tweak the prev version to return error on both
>> rom_add_vga/rom_add_option, but I was under the impression
>> that silently drop the romfile was discussion's decision.
>>
>> I am fine both ways, as it is a user error and hopefully
>> used correctly by libvirt. I only want to avoid the crash.
>>
>> Thanks,
>> Marcel
> 
> Let's try the non silent error.
> If someone is unhappy we can make it silent again, but
> if it succeeds we'll have to keep it working forever.

I agree - from the libvirt perspective, it is better to be conservative
and have a noisy failure on an incompatible combination, to prove no one
is using it (or give us reason to relax it later), than to accidentally
silently accept it and change from what was requested, and then find out
people were relying on the silent change in semantics.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices

[Markus Armbruster]

Marcel Apfelbaum <marcel.a@redhat.com> writes:

> Combining rombar=0 with romfile=<path> is an user error,
> silently dropping the romfile is a reasonable thing to do.
>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
> v1 -> v2:
>  After a discussion with Michael, Paolo and Alex, this
>  patch silent drops the romfile instead of not allowing
>  the hotplug.
>  
>  An OK from libvirt will be nice.
>
>  hw/pci/pci.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 6ce75aa..cd7a403 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1776,7 +1776,12 @@ static int pci_qdev_init(DeviceState *qdev)
>          pci_dev->romfile = g_strdup(pc->romfile);
>          is_default_rom = true;
>      }
> -    pci_add_option_rom(pci_dev, is_default_rom);
> +
> +    rc = pci_add_option_rom(pci_dev, is_default_rom);
> +    if (rc != 0) {
> +        pci_unregister_device(DEVICE(pci_dev));
> +        return rc;
> +    }
>  
>      return 0;
>  }

Looks like this part isn't covered by the commit message.

Before: errors in pci_add_option_rom() are reported, but the function
succeeds anyway.  After: it fails.  Is this a silent bug fix?

> @@ -1937,6 +1942,15 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
>           * for 0.11 compatibility.
>           */
>          int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> +
> +        /*
> +         * For hot-plugged devices silently ignore the option ROM
> +         * if the rom bar is disabled.
> +         */
> +        if (DEVICE(pdev)->hotplugged) {
> +            return 0;
> +        }
> +
>          if (class == 0x0300) {
>              rom_add_vga(pdev->romfile);
>          } else {

This part is covered by the commit message.

Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices

[Marcel Apfelbaum]

On Mon, 2014-10-27 at 15:59 +0100, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> 
> > Combining rombar=0 with romfile=<path> is an user error,
> > silently dropping the romfile is a reasonable thing to do.
> >
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> > v1 -> v2:
> >  After a discussion with Michael, Paolo and Alex, this
> >  patch silent drops the romfile instead of not allowing
> >  the hotplug.
> >  
> >  An OK from libvirt will be nice.
> >
> >  hw/pci/pci.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 6ce75aa..cd7a403 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1776,7 +1776,12 @@ static int pci_qdev_init(DeviceState *qdev)
> >          pci_dev->romfile = g_strdup(pc->romfile);
> >          is_default_rom = true;
> >      }
> > -    pci_add_option_rom(pci_dev, is_default_rom);
> > +
> > +    rc = pci_add_option_rom(pci_dev, is_default_rom);
> > +    if (rc != 0) {
> > +        pci_unregister_device(DEVICE(pci_dev));
> > +        return rc;
> > +    }
> >  
> >      return 0;
> >  }
> 
> Looks like this part isn't covered by the commit message.
> 
> Before: errors in pci_add_option_rom() are reported, but the function
> succeeds anyway.  After: it fails.  Is this a silent bug fix?
Yes, it made sense for V1, less for V2, but I wanted to keep it.
Should I split in 2 patches?

> 
> > @@ -1937,6 +1942,15 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> >           * for 0.11 compatibility.
> >           */
> >          int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> > +
> > +        /*
> > +         * For hot-plugged devices silently ignore the option ROM
> > +         * if the rom bar is disabled.
> > +         */
> > +        if (DEVICE(pdev)->hotplugged) {
> > +            return 0;
> > +        }
> > +
> >          if (class == 0x0300) {
> >              rom_add_vga(pdev->romfile);
> >          } else {
> 
> This part is covered by the commit message.

Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices

[Markus Armbruster]

Marcel Apfelbaum <marcel.a@redhat.com> writes:

> On Mon, 2014-10-27 at 15:59 +0100, Markus Armbruster wrote:
>> Marcel Apfelbaum <marcel.a@redhat.com> writes:
>> 
>> > Combining rombar=0 with romfile=<path> is an user error,
>> > silently dropping the romfile is a reasonable thing to do.
>> >
>> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
>> > ---
>> > v1 -> v2:
>> >  After a discussion with Michael, Paolo and Alex, this
>> >  patch silent drops the romfile instead of not allowing
>> >  the hotplug.
>> >  
>> >  An OK from libvirt will be nice.
>> >
>> >  hw/pci/pci.c | 16 +++++++++++++++-
>> >  1 file changed, 15 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> > index 6ce75aa..cd7a403 100644
>> > --- a/hw/pci/pci.c
>> > +++ b/hw/pci/pci.c
>> > @@ -1776,7 +1776,12 @@ static int pci_qdev_init(DeviceState *qdev)
>> >          pci_dev->romfile = g_strdup(pc->romfile);
>> >          is_default_rom = true;
>> >      }
>> > -    pci_add_option_rom(pci_dev, is_default_rom);
>> > +
>> > +    rc = pci_add_option_rom(pci_dev, is_default_rom);
>> > +    if (rc != 0) {
>> > +        pci_unregister_device(DEVICE(pci_dev));
>> > +        return rc;
>> > +    }
>> >  
>> >      return 0;
>> >  }
>> 
>> Looks like this part isn't covered by the commit message.
>> 
>> Before: errors in pci_add_option_rom() are reported, but the function
>> succeeds anyway.  After: it fails.  Is this a silent bug fix?
> Yes, it made sense for V1, less for V2, but I wanted to keep it.
> Should I split in 2 patches?

Yes, please.

[...]