[Qemu-devel] [PATCH v4 0/2] hw/pci: fixed hotplug crash when using rombar=0 with devices having romfile

[Qemu-devel] [PATCH v4 0/2] hw/pci: fixed hotplug crash when using rombar=0 with devices having romfile

[Marcel Apfelbaum]

Hot-plugging a device that has a romfile (either supplied by user
or built-in) using rombar=0 option is a user error,
do not allow the device to be hot-plugged.

v3 -> v4:
 - Fixed a typo in the commit (Eric Blake)

v2 -> v3:
 - Reverse the decision to forbid the hotplug on user error (Michael S. Tsirkin)
 - Split the patch in two, first part being a separate fix (Markus Armbruster)  

v1 -> v2:
 After a discussion with Michael, Paolo and Alex, this
 patch silent drops the romfile instead of not allowing
 the hotplug.

Marcel Apfelbaum (2):
  hw/pci: fixed error flow in pci_qdev_init
  hw/pci: fixed hotplug crash when using rombar=0 with devices having
    romfile

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

-- 
1.8.3.1

[Qemu-devel] [PATCH v4 1/2] hw/pci: fixed error flow in pci_qdev_init

[Marcel Apfelbaum]

Verify return code for pci_add_option_rom.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/pci/pci.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 6ce75aa..36226eb 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;
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4 1/2] hw/pci: fixed error flow in pci_qdev_init

[Markus Armbruster]

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

> Verify return code for pci_add_option_rom.
>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  hw/pci/pci.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 6ce75aa..36226eb 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;
>  }

Safe, because pci_add_option_rom() sets pci_dev->has_rom only on
success, and pci_unregister_device() cleans up the ROM only when
pci_has_rom is set.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v4 1/2] hw/pci: fixed error flow in pci_qdev_init

[Marcel Apfelbaum]

On Thu, 2014-10-30 at 14:33 +0100, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> 
> > Verify return code for pci_add_option_rom.
> >
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  hw/pci/pci.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 6ce75aa..36226eb 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;
> >  }
> 
> Safe, because pci_add_option_rom() sets pci_dev->has_rom only on
> success, and pci_unregister_device() cleans up the ROM only when
> pci_has_rom is set.
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Hi Michael,
The series has been reviewed, can you please add it to
your next pull request?

Thanks,
Marcel

[Qemu-devel] [PATCH v4 2/2] hw/pci: fixed hotplug crash when using rombar=0 with devices having romfile

[Marcel Apfelbaum]

Hot-plugging a device that has a romfile (either supplied by user
or built-in) using rombar=0 option is a user error,
do not allow the device to be hot-plugged.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/pci/pci.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 36226eb..371699c 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1942,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);
+
+        /*
+         * Hot-plugged devices can't use the option ROM
+         * if the rom bar is disabled.
+         */
+        if (DEVICE(pdev)->hotplugged) {
+            return -1;
+        }
+
         if (class == 0x0300) {
             rom_add_vga(pdev->romfile);
         } else {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4 2/2] hw/pci: fixed hotplug crash when using rombar=0 with devices having romfile

[Markus Armbruster]

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

> Hot-plugging a device that has a romfile (either supplied by user
> or built-in) using rombar=0 option is a user error,
> do not allow the device to be hot-plugged.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  hw/pci/pci.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 36226eb..371699c 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1942,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);
> +
> +        /*
> +         * Hot-plugged devices can't use the option ROM
> +         * if the rom bar is disabled.
> +         */
> +        if (DEVICE(pdev)->hotplugged) {
> +            return -1;
> +        }
> +
>          if (class == 0x0300) {
>              rom_add_vga(pdev->romfile);
>          } else {

Unlike the function's other unsuccessful returns, this one is silent.
Intentional?

Re: [Qemu-devel] [PATCH v4 2/2] hw/pci: fixed hotplug crash when using rombar=0 with devices having romfile

[Michael S. Tsirkin]

On Mon, Nov 03, 2014 at 01:03:30PM +0100, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> 
> > Hot-plugging a device that has a romfile (either supplied by user
> > or built-in) using rombar=0 option is a user error, > > do not allow the device to be hot-plugged.
> >
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  hw/pci/pci.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 36226eb..371699c 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1942,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);
> > +
> > +        /*
> > +         * Hot-plugged devices can't use the option ROM
> > +         * if the rom bar is disabled.
> > +         */
> > +        if (DEVICE(pdev)->hotplugged) {
> > +            return -1;
> > +        }
> > +
> >          if (class == 0x0300) {
> >              rom_add_vga(pdev->romfile);
> >          } else {
> 
> Unlike the function's other unsuccessful returns, this one is silent.
> Intentional?

Marcel, Markus, as I applied the patch already, if you want to
fix things up pls send a new patch on top.


Thanks!

Re: [Qemu-devel] [PATCH v4 2/2] hw/pci: fixed hotplug crash when using rombar=0 with devices having romfile

[Marcel Apfelbaum]

On Mon, 2014-11-03 at 13:03 +0100, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> 
> > Hot-plugging a device that has a romfile (either supplied by user
> > or built-in) using rombar=0 option is a user error,
> > do not allow the device to be hot-plugged.
> >
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  hw/pci/pci.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 36226eb..371699c 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1942,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);
> > +
> > +        /*
> > +         * Hot-plugged devices can't use the option ROM
> > +         * if the rom bar is disabled.
> > +         */
> > +        if (DEVICE(pdev)->hotplugged) {
> > +            return -1;
> > +        }
> > +
> >          if (class == 0x0300) {
> >              rom_add_vga(pdev->romfile);
> >          } else {
> 
> Unlike the function's other unsuccessful returns, this one is silent.
> Intentional?
Yes, the first version included an error message, but was not accepted
as the reviewers preferred "silent drop" instead.
The main reason was that a proper error propagation mechanism
should be used.
At the time of the patch there was not such an option, but now there is.
I can add it on top of your series, preferably after is merged.

Thanks,
Marcel 

Re: [Qemu-devel] [PATCH v4 2/2] hw/pci: fixed hotplug crash when using rombar=0 with devices having romfile

[Markus Armbruster]

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

> On Mon, 2014-11-03 at 13:03 +0100, Markus Armbruster wrote:
>> Marcel Apfelbaum <marcel.a@redhat.com> writes:
>> 
>> > Hot-plugging a device that has a romfile (either supplied by user
>> > or built-in) using rombar=0 option is a user error,
>> > do not allow the device to be hot-plugged.
>> >
>> > Reviewed-by: Eric Blake <eblake@redhat.com>
>> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
>> > ---
>> >  hw/pci/pci.c | 9 +++++++++
>> >  1 file changed, 9 insertions(+)
>> >
>> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> > index 36226eb..371699c 100644
>> > --- a/hw/pci/pci.c
>> > +++ b/hw/pci/pci.c
>> > @@ -1942,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);
>> > +
>> > +        /*
>> > +         * Hot-plugged devices can't use the option ROM
>> > +         * if the rom bar is disabled.
>> > +         */
>> > +        if (DEVICE(pdev)->hotplugged) {
>> > +            return -1;
>> > +        }
>> > +
>> >          if (class == 0x0300) {
>> >              rom_add_vga(pdev->romfile);
>> >          } else {
>> 
>> Unlike the function's other unsuccessful returns, this one is silent.
>> Intentional?
> Yes, the first version included an error message, but was not accepted
> as the reviewers preferred "silent drop" instead.
> The main reason was that a proper error propagation mechanism
> should be used.
> At the time of the patch there was not such an option, but now there is.
> I can add it on top of your series, preferably after is merged.

My rebased "pci: Convert core to realize" has this hunk:

    @@ -1948,7 +1955,9 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
              * if the rom bar is disabled.
              */
             if (DEVICE(pdev)->hotplugged) {
    -            return -1;
    +            error_setg(errp, "Hot-plugged device without ROM bar"
    +                       " can't have an option ROM");
    +            return;
             }

             if (class == 0x0300) {

Bad, because the patch does two separate things: fix a failure not to be
silent, and convert to realize.  Needs to be split.  Begs the question
how to order the parts.  I'd prefer to put the fix first, and get it
into 2.2.  What do you think?

Re: [Qemu-devel] [PATCH v4 2/2] hw/pci: fixed hotplug crash when using rombar=0 with devices having romfile

[Marcel Apfelbaum]

On Mon, 2014-11-03 at 14:40 +0100, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> 
> > On Mon, 2014-11-03 at 13:03 +0100, Markus Armbruster wrote:
> >> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> >> 
> >> > Hot-plugging a device that has a romfile (either supplied by user
> >> > or built-in) using rombar=0 option is a user error,
> >> > do not allow the device to be hot-plugged.
> >> >
> >> > Reviewed-by: Eric Blake <eblake@redhat.com>
> >> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> >> > ---
> >> >  hw/pci/pci.c | 9 +++++++++
> >> >  1 file changed, 9 insertions(+)
> >> >
> >> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >> > index 36226eb..371699c 100644
> >> > --- a/hw/pci/pci.c
> >> > +++ b/hw/pci/pci.c
> >> > @@ -1942,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);
> >> > +
> >> > +        /*
> >> > +         * Hot-plugged devices can't use the option ROM
> >> > +         * if the rom bar is disabled.
> >> > +         */
> >> > +        if (DEVICE(pdev)->hotplugged) {
> >> > +            return -1;
> >> > +        }
> >> > +
> >> >          if (class == 0x0300) {
> >> >              rom_add_vga(pdev->romfile);
> >> >          } else {
> >> 
> >> Unlike the function's other unsuccessful returns, this one is silent.
> >> Intentional?
> > Yes, the first version included an error message, but was not accepted
> > as the reviewers preferred "silent drop" instead.
> > The main reason was that a proper error propagation mechanism
> > should be used.
> > At the time of the patch there was not such an option, but now there is.
> > I can add it on top of your series, preferably after is merged.
> 
> My rebased "pci: Convert core to realize" has this hunk:
> 
>     @@ -1948,7 +1955,9 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
>               * if the rom bar is disabled.
>               */
>              if (DEVICE(pdev)->hotplugged) {
>     -            return -1;
>     +            error_setg(errp, "Hot-plugged device without ROM bar"
>     +                       " can't have an option ROM");
>     +            return;
>              }
> 
>              if (class == 0x0300) {
> 
> Bad, because the patch does two separate things: fix a failure not to be
> silent, and convert to realize.  Needs to be split.  Begs the question
> how to order the parts.  I'd prefer to put the fix first, and get it
> into 2.2.  What do you think?

If I understand your question correctly:
I would first convert to realize, then add the fix.
The reason for it is pretty simple: Conversion to realize
gives us the error flow propagation we need.

Thanks,
Marcel

Re: [Qemu-devel] [PATCH v4 2/2] hw/pci: fixed hotplug crash when using rombar=0 with devices having romfile

[Markus Armbruster]

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

> On Mon, 2014-11-03 at 14:40 +0100, Markus Armbruster wrote:
>> Marcel Apfelbaum <marcel.a@redhat.com> writes:
>> 
>> > On Mon, 2014-11-03 at 13:03 +0100, Markus Armbruster wrote:
>> >> Marcel Apfelbaum <marcel.a@redhat.com> writes:
>> >> 
>> >> > Hot-plugging a device that has a romfile (either supplied by user
>> >> > or built-in) using rombar=0 option is a user error,
>> >> > do not allow the device to be hot-plugged.
>> >> >
>> >> > Reviewed-by: Eric Blake <eblake@redhat.com>
>> >> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
>> >> > ---
>> >> >  hw/pci/pci.c | 9 +++++++++
>> >> >  1 file changed, 9 insertions(+)
>> >> >
>> >> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> >> > index 36226eb..371699c 100644
>> >> > --- a/hw/pci/pci.c
>> >> > +++ b/hw/pci/pci.c
>> >> > @@ -1942,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);
>> >> > +
>> >> > +        /*
>> >> > +         * Hot-plugged devices can't use the option ROM
>> >> > +         * if the rom bar is disabled.
>> >> > +         */
>> >> > +        if (DEVICE(pdev)->hotplugged) {
>> >> > +            return -1;
>> >> > +        }
>> >> > +
>> >> >          if (class == 0x0300) {
>> >> >              rom_add_vga(pdev->romfile);
>> >> >          } else {
>> >> 
>> >> Unlike the function's other unsuccessful returns, this one is silent.
>> >> Intentional?
>> > Yes, the first version included an error message, but was not accepted
>> > as the reviewers preferred "silent drop" instead.
>> > The main reason was that a proper error propagation mechanism
>> > should be used.
>> > At the time of the patch there was not such an option, but now there is.
>> > I can add it on top of your series, preferably after is merged.
>> 
>> My rebased "pci: Convert core to realize" has this hunk:
>> 
>>     @@ -1948,7 +1955,9 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
>>               * if the rom bar is disabled.
>>               */
>>              if (DEVICE(pdev)->hotplugged) {
>>     -            return -1;
>>     +            error_setg(errp, "Hot-plugged device without ROM bar"
>>     +                       " can't have an option ROM");
>>     +            return;
>>              }
>> 
>>              if (class == 0x0300) {
>> 
>> Bad, because the patch does two separate things: fix a failure not to be
>> silent, and convert to realize.  Needs to be split.  Begs the question
>> how to order the parts.  I'd prefer to put the fix first, and get it
>> into 2.2.  What do you think?
>
> If I understand your question correctly:
> I would first convert to realize, then add the fix.
> The reason for it is pretty simple: Conversion to realize
> gives us the error flow propagation we need.

I'd do it the other way round, because

1. Before your series, pci_add_option_rom() can already fail.  It always
reports an error when it fails.  Good, except the caller ignores
failure.

2. Your PATCH 1/2 fixes the caller.  Good.

3. Your PATCH 2/2 adds a failure that doesn't report an error.  Bad,
because it leaves the user guessing what went wrong.  I view that as a
bug.

I'd like this bug to be fixed for 2.2.  Since Michael wants to delay my
"pci: Partial conversion to 2.3, that means fixing it before conversion
to realize.

Re: [Qemu-devel] [PATCH v4 2/2] hw/pci: fixed hotplug crash when using rombar=0 with devices having romfile

[Marcel Apfelbaum]

On Mon, 2014-11-03 at 16:54 +0100, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> 
> > On Mon, 2014-11-03 at 14:40 +0100, Markus Armbruster wrote:
> >> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> >> 
> >> > On Mon, 2014-11-03 at 13:03 +0100, Markus Armbruster wrote:
> >> >> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> >> >> 
> >> >> > Hot-plugging a device that has a romfile (either supplied by user
> >> >> > or built-in) using rombar=0 option is a user error,
> >> >> > do not allow the device to be hot-plugged.
> >> >> >
> >> >> > Reviewed-by: Eric Blake <eblake@redhat.com>
> >> >> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> >> >> > ---
> >> >> >  hw/pci/pci.c | 9 +++++++++
> >> >> >  1 file changed, 9 insertions(+)
> >> >> >
> >> >> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >> >> > index 36226eb..371699c 100644
> >> >> > --- a/hw/pci/pci.c
> >> >> > +++ b/hw/pci/pci.c
> >> >> > @@ -1942,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);
> >> >> > +
> >> >> > +        /*
> >> >> > +         * Hot-plugged devices can't use the option ROM
> >> >> > +         * if the rom bar is disabled.
> >> >> > +         */
> >> >> > +        if (DEVICE(pdev)->hotplugged) {
> >> >> > +            return -1;
> >> >> > +        }
> >> >> > +
> >> >> >          if (class == 0x0300) {
> >> >> >              rom_add_vga(pdev->romfile);
> >> >> >          } else {
> >> >> 
> >> >> Unlike the function's other unsuccessful returns, this one is silent.
> >> >> Intentional?
> >> > Yes, the first version included an error message, but was not accepted
> >> > as the reviewers preferred "silent drop" instead.
> >> > The main reason was that a proper error propagation mechanism
> >> > should be used.
> >> > At the time of the patch there was not such an option, but now there is.
> >> > I can add it on top of your series, preferably after is merged.
> >> 
> >> My rebased "pci: Convert core to realize" has this hunk:
> >> 
> >>     @@ -1948,7 +1955,9 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> >>               * if the rom bar is disabled.
> >>               */
> >>              if (DEVICE(pdev)->hotplugged) {
> >>     -            return -1;
> >>     +            error_setg(errp, "Hot-plugged device without ROM bar"
> >>     +                       " can't have an option ROM");
> >>     +            return;
> >>              }
> >> 
> >>              if (class == 0x0300) {
> >> 
> >> Bad, because the patch does two separate things: fix a failure not to be
> >> silent, and convert to realize.  Needs to be split.  Begs the question
> >> how to order the parts.  I'd prefer to put the fix first, and get it
> >> into 2.2.  What do you think?
> >
> > If I understand your question correctly:
> > I would first convert to realize, then add the fix.
> > The reason for it is pretty simple: Conversion to realize
> > gives us the error flow propagation we need.
> 
> I'd do it the other way round, because
> 
> 1. Before your series, pci_add_option_rom() can already fail.  It always
> reports an error when it fails.  Good, except the caller ignores
> failure.
> 
> 2. Your PATCH 1/2 fixes the caller.  Good.
> 
> 3. Your PATCH 2/2 adds a failure that doesn't report an error.  Bad,
> because it leaves the user guessing what went wrong.  I view that as a
> bug.
> 
> I'd like this bug to be fixed for 2.2.  Since Michael wants to delay my
> "pci: Partial conversion to 2.3, that means fixing it before conversion
> to realize.
I thought your patches will be part of 2.2.
I have nothing against it.

Thanks,
Marcel

Re: [Qemu-devel] [PATCH v4 2/2] hw/pci: fixed hotplug crash when using rombar=0 with devices having romfile

[Michael S. Tsirkin]

On Mon, Nov 03, 2014 at 04:54:25PM +0100, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> 
> > On Mon, 2014-11-03 at 14:40 +0100, Markus Armbruster wrote:
> >> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> >> 
> >> > On Mon, 2014-11-03 at 13:03 +0100, Markus Armbruster wrote:
> >> >> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> >> >> 
> >> >> > Hot-plugging a device that has a romfile (either supplied by user
> >> >> > or built-in) using rombar=0 option is a user error,
> >> >> > do not allow the device to be hot-plugged.
> >> >> >
> >> >> > Reviewed-by: Eric Blake <eblake@redhat.com>
> >> >> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> >> >> > ---
> >> >> >  hw/pci/pci.c | 9 +++++++++
> >> >> >  1 file changed, 9 insertions(+)
> >> >> >
> >> >> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >> >> > index 36226eb..371699c 100644
> >> >> > --- a/hw/pci/pci.c
> >> >> > +++ b/hw/pci/pci.c
> >> >> > @@ -1942,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);
> >> >> > +
> >> >> > +        /*
> >> >> > +         * Hot-plugged devices can't use the option ROM
> >> >> > +         * if the rom bar is disabled.
> >> >> > +         */
> >> >> > +        if (DEVICE(pdev)->hotplugged) {
> >> >> > +            return -1;
> >> >> > +        }
> >> >> > +
> >> >> >          if (class == 0x0300) {
> >> >> >              rom_add_vga(pdev->romfile);
> >> >> >          } else {
> >> >> 
> >> >> Unlike the function's other unsuccessful returns, this one is silent.
> >> >> Intentional?
> >> > Yes, the first version included an error message, but was not accepted
> >> > as the reviewers preferred "silent drop" instead.
> >> > The main reason was that a proper error propagation mechanism
> >> > should be used.
> >> > At the time of the patch there was not such an option, but now there is.
> >> > I can add it on top of your series, preferably after is merged.
> >> 
> >> My rebased "pci: Convert core to realize" has this hunk:
> >> 
> >>     @@ -1948,7 +1955,9 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> >>               * if the rom bar is disabled.
> >>               */
> >>              if (DEVICE(pdev)->hotplugged) {
> >>     -            return -1;
> >>     +            error_setg(errp, "Hot-plugged device without ROM bar"
> >>     +                       " can't have an option ROM");
> >>     +            return;
> >>              }
> >> 
> >>              if (class == 0x0300) {
> >> 
> >> Bad, because the patch does two separate things: fix a failure not to be
> >> silent, and convert to realize.  Needs to be split.  Begs the question
> >> how to order the parts.  I'd prefer to put the fix first, and get it
> >> into 2.2.  What do you think?
> >
> > If I understand your question correctly:
> > I would first convert to realize, then add the fix.
> > The reason for it is pretty simple: Conversion to realize
> > gives us the error flow propagation we need.
> 
> I'd do it the other way round, because
> 
> 1. Before your series, pci_add_option_rom() can already fail.  It always
> reports an error when it fails.  Good, except the caller ignores
> failure.
> 
> 2. Your PATCH 1/2 fixes the caller.  Good.
> 
> 3. Your PATCH 2/2 adds a failure that doesn't report an error.  Bad,
> because it leaves the user guessing what went wrong.  I view that as a
> bug.
> 
> I'd like this bug to be fixed for 2.2.  Since Michael wants to delay my
> "pci: Partial conversion to 2.3, that means fixing it before conversion
> to realize.

I didn't look at this deeply yes, so I don't know how
hard would it be to fix without pulling in realize.
If easy, go for it.
If hard, we can pull in realize for 2.2.

Re: [Qemu-devel] [PATCH v4 0/2] hw/pci: fixed hotplug crash when using rombar=0 with devices having romfile

[Marcel Apfelbaum]

On Mon, 2014-10-27 at 19:34 +0200, Marcel Apfelbaum wrote:
> Hot-plugging a device that has a romfile (either supplied by user
> or built-in) using rombar=0 option is a user error,
> do not allow the device to be hot-plugged.
Hi Michael,

The series has been reviewed, can you please add it to
your next PULL request?

Thanks,
Marcel

> 
> v3 -> v4:
>  - Fixed a typo in the commit (Eric Blake)
> 
> v2 -> v3:
>  - Reverse the decision to forbid the hotplug on user error (Michael S. Tsirkin)
>  - Split the patch in two, first part being a separate fix (Markus Armbruster)  
> 
> v1 -> v2:
>  After a discussion with Michael, Paolo and Alex, this
>  patch silent drops the romfile instead of not allowing
>  the hotplug.
> 
> Marcel Apfelbaum (2):
>   hw/pci: fixed error flow in pci_qdev_init
>   hw/pci: fixed hotplug crash when using rombar=0 with devices having
>     romfile
> 
>  hw/pci/pci.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>