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

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

[Marcel Apfelbaum]

ROM images must be loaded at startup. Usage of rombar=0 after that
is not allowed, but should not crash QEMU.

Check that the device is not hotplugged before trying to
insert the rom file.

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

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 6ce75aa..3907c90 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;
 }
@@ -1940,6 +1945,10 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
         if (class == 0x0300) {
             rom_add_vga(pdev->romfile);
         } else {
+            if (DEVICE(pdev)->hotplugged) {
+                error_report("PCI: rombar can't be 0 for hotplugged devices!");
+                return -1;
+            }
             rom_add_option(pdev->romfile, -1);
         }
         return 0;
-- 
1.8.3.1

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

[Paolo Bonzini]

On 10/21/2014 02:37 PM, Marcel Apfelbaum wrote:
> ROM images must be loaded at startup. Usage of rombar=0 after that
> is not allowed, but should not crash QEMU.
> 
> Check that the device is not hotplugged before trying to
> insert the rom file.

I think it could also make sense to just ignore the option ROM and allow
the hotplug.

Sooner or later we should drop the oldest compat machine types...
everything until 0.12 probably could go.

Paolo

> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  hw/pci/pci.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 6ce75aa..3907c90 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;
>  }
> @@ -1940,6 +1945,10 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
>          if (class == 0x0300) {
>              rom_add_vga(pdev->romfile);
>          } else {
> +            if (DEVICE(pdev)->hotplugged) {
> +                error_report("PCI: rombar can't be 0 for hotplugged devices!");
> +                return -1;
> +            }
>              rom_add_option(pdev->romfile, -1);
>          }
>          return 0;
> 

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

[Michael S. Tsirkin]

On Wed, Oct 22, 2014 at 12:06:18AM +0200, Paolo Bonzini wrote:
> 
> 
> On 10/21/2014 02:37 PM, Marcel Apfelbaum wrote:
> > ROM images must be loaded at startup. Usage of rombar=0 after that
> > is not allowed, but should not crash QEMU.
> > 
> > Check that the device is not hotplugged before trying to
> > insert the rom file.
> 
> I think it could also make sense to just ignore the option ROM and allow
> the hotplug.
> 
> Sooner or later we should drop the oldest compat machine types...
> everything until 0.12 probably could go.
> 
> Paolo

Unfortunately, we made the mistake of exposing the rombar
attribute to management.

See rom element at
http://libvirt.org/formatdomain.html#elementsHostDevSubsys

and in theory some users might have configured rombar=off
but set file to some value.

I'd be OK with removing support for this combination,
if we want the legacy fw cfg thing, we should have a
separate attribute.


But I'd like someone from libvirt to OK this ...
Eric?


> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  hw/pci/pci.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 6ce75aa..3907c90 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;
> >  }
> > @@ -1940,6 +1945,10 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> >          if (class == 0x0300) {
> >              rom_add_vga(pdev->romfile);
> >          } else {
> > +            if (DEVICE(pdev)->hotplugged) {
> > +                error_report("PCI: rombar can't be 0 for hotplugged devices!");
> > +                return -1;
> > +            }
> >              rom_add_option(pdev->romfile, -1);
> >          }
> >          return 0;
> > 

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

[Marcel Apfelbaum]

On Wed, 2014-10-22 at 00:06 +0200, Paolo Bonzini wrote:
> 
> On 10/21/2014 02:37 PM, Marcel Apfelbaum wrote:
> > ROM images must be loaded at startup. Usage of rombar=0 after that
> > is not allowed, but should not crash QEMU.
> > 
> > Check that the device is not hotplugged before trying to
> > insert the rom file.
> 
> I think it could also make sense to just ignore the option ROM and allow
> the hotplug.
We need a way to inform the user we did that, he *specifically* asked
for a ROM he might need it.

Thanks,
Marcel
 
> 
> Sooner or later we should drop the oldest compat machine types...
> everything until 0.12 probably could go.
> 
> Paolo
> 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  hw/pci/pci.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 6ce75aa..3907c90 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;
> >  }
> > @@ -1940,6 +1945,10 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> >          if (class == 0x0300) {
> >              rom_add_vga(pdev->romfile);
> >          } else {
> > +            if (DEVICE(pdev)->hotplugged) {
> > +                error_report("PCI: rombar can't be 0 for hotplugged devices!");
> > +                return -1;
> > +            }
> >              rom_add_option(pdev->romfile, -1);
> >          }
> >          return 0;
> > 

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

[Michael S. Tsirkin]

On Wed, Oct 22, 2014 at 10:41:05AM +0300, Marcel Apfelbaum wrote:
> On Wed, 2014-10-22 at 00:06 +0200, Paolo Bonzini wrote:
> > 
> > On 10/21/2014 02:37 PM, Marcel Apfelbaum wrote:
> > > ROM images must be loaded at startup. Usage of rombar=0 after that
> > > is not allowed, but should not crash QEMU.
> > > 
> > > Check that the device is not hotplugged before trying to
> > > insert the rom file.
> > 
> > I think it could also make sense to just ignore the option ROM and allow
> > the hotplug.
> We need a way to inform the user we did that, he *specifically* asked
> for a ROM he might need it.
> 
> Thanks,
> Marcel

But he also asked to disable BAR.

I don't see valid reasons for this configuration
expcept compatibility.

But I have a vague memory Alex thought differently.
Alex?


> > 
> > Sooner or later we should drop the oldest compat machine types...
> > everything until 0.12 probably could go.
> > 
> > Paolo
> > 
> > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > ---
> > >  hw/pci/pci.c | 11 ++++++++++-
> > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 6ce75aa..3907c90 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;
> > >  }
> > > @@ -1940,6 +1945,10 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> > >          if (class == 0x0300) {
> > >              rom_add_vga(pdev->romfile);
> > >          } else {
> > > +            if (DEVICE(pdev)->hotplugged) {
> > > +                error_report("PCI: rombar can't be 0 for hotplugged devices!");
> > > +                return -1;
> > > +            }
> > >              rom_add_option(pdev->romfile, -1);
> > >          }
> > >          return 0;
> > > 
> 
> 

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

[Alex Williamson]

On Wed, 2014-10-22 at 11:02 +0300, Michael S. Tsirkin wrote:
> On Wed, Oct 22, 2014 at 10:41:05AM +0300, Marcel Apfelbaum wrote:
> > On Wed, 2014-10-22 at 00:06 +0200, Paolo Bonzini wrote:
> > > 
> > > On 10/21/2014 02:37 PM, Marcel Apfelbaum wrote:
> > > > ROM images must be loaded at startup. Usage of rombar=0 after that
> > > > is not allowed, but should not crash QEMU.
> > > > 
> > > > Check that the device is not hotplugged before trying to
> > > > insert the rom file.
> > > 
> > > I think it could also make sense to just ignore the option ROM and allow
> > > the hotplug.
> > We need a way to inform the user we did that, he *specifically* asked
> > for a ROM he might need it.
> > 
> > Thanks,
> > Marcel
> 
> But he also asked to disable BAR.
> 
> I don't see valid reasons for this configuration
> expcept compatibility.
> 
> But I have a vague memory Alex thought differently.
> Alex?

The comment in the original patch is really confusing for device
assignment where rombar=0 is perfectly valid for any case, hotplug or
not, but only in combination with romfile= do we end up trying to use
fw_cfg, which I think is what we're trying to prevent here.  Emulated
devices are the ones that will still try to use fw_cfg because they have
an implicit romfile.

I can't think of any use cases for requiring fw_cfg for an assigned
device, it usually ends up being a user error to specify both rombar=0
with romfile=$FILE.  Doing that for any device, emulated or assigned,
disassociates the ROM from the device which breaks things like bootindex
as well.

If a user specifies rombar=0,romfile=$FILE we should probably error and
reject the device for hotplug.  Emulated devices with implicit romfiles
are a bit harder to know what will break.  Silently dropping the
implicit romfile seems like a reasonable thing, but then we have
different behavior between cold- and hot-plugged devices.  I think
that's reproducible for migration using romfile="", but I don't expect
libvirt handles that properly.  It's a can of worms...

Thanks,
Alex

> > > 
> > > Sooner or later we should drop the oldest compat machine types...
> > > everything until 0.12 probably could go.
> > > 
> > > Paolo
> > > 
> > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > > ---
> > > >  hw/pci/pci.c | 11 ++++++++++-
> > > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > index 6ce75aa..3907c90 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;
> > > >  }
> > > > @@ -1940,6 +1945,10 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> > > >          if (class == 0x0300) {
> > > >              rom_add_vga(pdev->romfile);
> > > >          } else {
> > > > +            if (DEVICE(pdev)->hotplugged) {
> > > > +                error_report("PCI: rombar can't be 0 for hotplugged devices!");
> > > > +                return -1;
> > > > +            }
> > > >              rom_add_option(pdev->romfile, -1);
> > > >          }
> > > >          return 0;
> > > > 
> > 
> > 
> 

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

[Marcel Apfelbaum]

On Wed, 2014-10-22 at 10:26 -0600, Alex Williamson wrote:
> On Wed, 2014-10-22 at 11:02 +0300, Michael S. Tsirkin wrote:
> > On Wed, Oct 22, 2014 at 10:41:05AM +0300, Marcel Apfelbaum wrote:
> > > On Wed, 2014-10-22 at 00:06 +0200, Paolo Bonzini wrote:
> > > > 
> > > > On 10/21/2014 02:37 PM, Marcel Apfelbaum wrote:
> > > > > ROM images must be loaded at startup. Usage of rombar=0 after that
> > > > > is not allowed, but should not crash QEMU.
> > > > > 
> > > > > Check that the device is not hotplugged before trying to
> > > > > insert the rom file.
> > > > 
> > > > I think it could also make sense to just ignore the option ROM and allow
> > > > the hotplug.
> > > We need a way to inform the user we did that, he *specifically* asked
> > > for a ROM he might need it.
> > > 
> > > Thanks,
> > > Marcel
> > 
> > But he also asked to disable BAR.
> > 
> > I don't see valid reasons for this configuration
> > expcept compatibility.
> > 
> > But I have a vague memory Alex thought differently.
> > Alex?
> 
> The comment in the original patch is really confusing for device
> assignment where rombar=0 is perfectly valid for any case, hotplug or
> not,
Yes, I should have made it more clear:
ROM images must be loaded at startup. Assigning ROM images
to devices and disabling the BAR at hotplug
is not allowed, but should not crash QEMU.


>  but only in combination with romfile= do we end up trying to use
> fw_cfg, which I think is what we're trying to prevent here.  Emulated
> devices are the ones that will still try to use fw_cfg because they have
> an implicit romfile.
> 
> I can't think of any use cases for requiring fw_cfg for an assigned
> device, it usually ends up being a user error to specify both rombar=0
> with romfile=$FILE.  Doing that for any device, emulated or assigned,
> disassociates the ROM from the device which breaks things like bootindex
> as well.
> 
> If a user specifies rombar=0,romfile=$FILE we should probably error and
> reject the device for hotplug.
>   Emulated devices with implicit romfiles
> are a bit harder to know what will break.  Silently dropping the
> implicit romfile seems like a reasonable thing,
It seems that this is the general agreement, I am going to post
a v2 for this and see what libvirt guys have to say about it.
I'll cc Eric.

Thanks,
Marcel

>  but then we have
> different behavior between cold- and hot-plugged devices.  I think
> that's reproducible for migration using romfile="", but I don't expect
> libvirt handles that properly.  It's a can of worms...
> 
> Thanks,
> Alex
> 
> > > > 
> > > > Sooner or later we should drop the oldest compat machine types...
> > > > everything until 0.12 probably could go.
> > > > 
> > > > Paolo
> > > > 
> > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > > > ---
> > > > >  hw/pci/pci.c | 11 ++++++++++-
> > > > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > > index 6ce75aa..3907c90 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;
> > > > >  }
> > > > > @@ -1940,6 +1945,10 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> > > > >          if (class == 0x0300) {
> > > > >              rom_add_vga(pdev->romfile);
> > > > >          } else {
> > > > > +            if (DEVICE(pdev)->hotplugged) {
> > > > > +                error_report("PCI: rombar can't be 0 for hotplugged devices!");
> > > > > +                return -1;
> > > > > +            }
> > > > >              rom_add_option(pdev->romfile, -1);
> > > > >          }
> > > > >          return 0;
> > > > > 
> > > 
> > > 
> > 
> 
> 
> 

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

[Michael S. Tsirkin]

On Tue, Oct 21, 2014 at 03:37:12PM +0300, Marcel Apfelbaum wrote:
> ROM images must be loaded at startup. Usage of rombar=0 after that
> is not allowed, but should not crash QEMU.
> 
> Check that the device is not hotplugged before trying to
> insert the rom file.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  hw/pci/pci.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 6ce75aa..3907c90 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;
>  }

Fair enough for this chunk.

> @@ -1940,6 +1945,10 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
>          if (class == 0x0300) {
>              rom_add_vga(pdev->romfile);
>          } else {
> +            if (DEVICE(pdev)->hotplugged) {
> +                error_report("PCI: rombar can't be 0 for hotplugged devices!");
> +                return -1;
> +            }
>              rom_add_option(pdev->romfile, -1);
>          }
>          return 0;


The message is confusing. rombar=0 is ok if you
don't also try to force romfile.
Generally why are you adding this logic in pci?
And what about e.g. vga?
I think the right thing to do is to propagate return codes correctly,
and report the error where it occurs.

> -- 
> 1.8.3.1

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

[Marcel Apfelbaum]

On Wed, 2014-10-22 at 09:18 +0300, Michael S. Tsirkin wrote:
> On Tue, Oct 21, 2014 at 03:37:12PM +0300, Marcel Apfelbaum wrote:
> > ROM images must be loaded at startup. Usage of rombar=0 after that
> > is not allowed, but should not crash QEMU.
> > 
> > Check that the device is not hotplugged before trying to
> > insert the rom file.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  hw/pci/pci.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 6ce75aa..3907c90 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;
> >  }
> 
> Fair enough for this chunk.
> 
> > @@ -1940,6 +1945,10 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> >          if (class == 0x0300) {
> >              rom_add_vga(pdev->romfile);
> >          } else {
> > +            if (DEVICE(pdev)->hotplugged) {
> > +                error_report("PCI: rombar can't be 0 for hotplugged devices!");
> > +                return -1;
> > +            }
> >              rom_add_option(pdev->romfile, -1);
> >          }
> >          return 0;
> 
> 
> The message is confusing. rombar=0 is ok if you
> don't also try to force romfile.
> Generally why are you adding this logic in pci?
Because rom_add_option will call eventually rom_insert 
that will crash QEMU with the call to hw_error.

> And what about e.g. vga?
This logic would apply also to rom_add_vga, I was not
aware we can hotplug vga devices. Can we?
I can add it also to vga, of course.

> I think the right thing to do is to propagate return codes correctly,
> and report the error where it occurs.
I can remove the error_report, but this gives an extra hint to user.
I didn't see any other way to propagate the error message. 
Should I drop it?

Thanks,
Marcel  
> 
> > -- 
> > 1.8.3.1

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

[Michael S. Tsirkin]

On Wed, Oct 22, 2014 at 10:34:21AM +0300, Marcel Apfelbaum wrote:
> On Wed, 2014-10-22 at 09:18 +0300, Michael S. Tsirkin wrote:
> > On Tue, Oct 21, 2014 at 03:37:12PM +0300, Marcel Apfelbaum wrote:
> > > ROM images must be loaded at startup. Usage of rombar=0 after that
> > > is not allowed, but should not crash QEMU.
> > > 
> > > Check that the device is not hotplugged before trying to
> > > insert the rom file.
> > > 
> > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > ---
> > >  hw/pci/pci.c | 11 ++++++++++-
> > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 6ce75aa..3907c90 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;
> > >  }
> > 
> > Fair enough for this chunk.
> > 
> > > @@ -1940,6 +1945,10 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> > >          if (class == 0x0300) {
> > >              rom_add_vga(pdev->romfile);
> > >          } else {
> > > +            if (DEVICE(pdev)->hotplugged) {
> > > +                error_report("PCI: rombar can't be 0 for hotplugged devices!");
> > > +                return -1;
> > > +            }
> > >              rom_add_option(pdev->romfile, -1);
> > >          }
> > >          return 0;
> > 
> > 
> > The message is confusing. rombar=0 is ok if you
> > don't also try to force romfile.
> > Generally why are you adding this logic in pci?
> Because rom_add_option will call eventually rom_insert 
> that will crash QEMU with the call to hw_error.

So fix rom_insert to return an error code instead?

> > And what about e.g. vga?
> This logic would apply also to rom_add_vga, I was not
> aware we can hotplug vga devices. Can we?
> I can add it also to vga, of course.
> 
> > I think the right thing to do is to propagate return codes correctly,
> > and report the error where it occurs.
> I can remove the error_report, but this gives an extra hint to user.

Move it to rom_insert, instead of hw_error.

> I didn't see any other way to propagate the error message. 
> Should I drop it?
> 
> Thanks,
> Marcel  
> > 
> > > -- 
> > > 1.8.3.1
> 
> 

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

[Marcel Apfelbaum]

On Wed, 2014-10-22 at 10:58 +0300, Michael S. Tsirkin wrote:
> On Wed, Oct 22, 2014 at 10:34:21AM +0300, Marcel Apfelbaum wrote:
> > On Wed, 2014-10-22 at 09:18 +0300, Michael S. Tsirkin wrote:
> > > On Tue, Oct 21, 2014 at 03:37:12PM +0300, Marcel Apfelbaum wrote:
> > > > ROM images must be loaded at startup. Usage of rombar=0 after that
> > > > is not allowed, but should not crash QEMU.
> > > > 
> > > > Check that the device is not hotplugged before trying to
> > > > insert the rom file.
> > > > 
> > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > > ---
> > > >  hw/pci/pci.c | 11 ++++++++++-
> > > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > index 6ce75aa..3907c90 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;
> > > >  }
> > > 
> > > Fair enough for this chunk.
> > > 
> > > > @@ -1940,6 +1945,10 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> > > >          if (class == 0x0300) {
> > > >              rom_add_vga(pdev->romfile);
> > > >          } else {
> > > > +            if (DEVICE(pdev)->hotplugged) {
> > > > +                error_report("PCI: rombar can't be 0 for hotplugged devices!");
> > > > +                return -1;
> > > > +            }
> > > >              rom_add_option(pdev->romfile, -1);
> > > >          }
> > > >          return 0;
> > > 
> > > 
> > > The message is confusing. rombar=0 is ok if you
> > > don't also try to force romfile.
> > > Generally why are you adding this logic in pci?
> > Because rom_add_option will call eventually rom_insert 
> > that will crash QEMU with the call to hw_error.
> 
> So fix rom_insert to return an error code instead?
OK, thanks

> 
> > > And what about e.g. vga?
> > This logic would apply also to rom_add_vga, I was not
> > aware we can hotplug vga devices. Can we?
> > I can add it also to vga, of course.
> > 
> > > I think the right thing to do is to propagate return codes correctly,
> > > and report the error where it occurs.
> > I can remove the error_report, but this gives an extra hint to user.
> 
> Move it to rom_insert, instead of hw_error.
Sure, I was a little "afraid" to change the "crash" policy of rom_insert.

Thanks,
Marcel
> 
> > I didn't see any other way to propagate the error message. 
> > Should I drop it?
> > 
> > Thanks,
> > Marcel  
> > > 
> > > > -- 
> > > > 1.8.3.1
> > 
> > 

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

[Michael S. Tsirkin]

On Wed, Oct 22, 2014 at 11:16:05AM +0300, Marcel Apfelbaum wrote:
> On Wed, 2014-10-22 at 10:58 +0300, Michael S. Tsirkin wrote:
> > On Wed, Oct 22, 2014 at 10:34:21AM +0300, Marcel Apfelbaum wrote:
> > > On Wed, 2014-10-22 at 09:18 +0300, Michael S. Tsirkin wrote:
> > > > On Tue, Oct 21, 2014 at 03:37:12PM +0300, Marcel Apfelbaum wrote:
> > > > > ROM images must be loaded at startup. Usage of rombar=0 after that
> > > > > is not allowed, but should not crash QEMU.
> > > > > 
> > > > > Check that the device is not hotplugged before trying to
> > > > > insert the rom file.
> > > > > 
> > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > > > ---
> > > > >  hw/pci/pci.c | 11 ++++++++++-
> > > > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > > index 6ce75aa..3907c90 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;
> > > > >  }
> > > > 
> > > > Fair enough for this chunk.
> > > > 
> > > > > @@ -1940,6 +1945,10 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> > > > >          if (class == 0x0300) {
> > > > >              rom_add_vga(pdev->romfile);
> > > > >          } else {
> > > > > +            if (DEVICE(pdev)->hotplugged) {
> > > > > +                error_report("PCI: rombar can't be 0 for hotplugged devices!");
> > > > > +                return -1;
> > > > > +            }
> > > > >              rom_add_option(pdev->romfile, -1);
> > > > >          }
> > > > >          return 0;
> > > > 
> > > > 
> > > > The message is confusing. rombar=0 is ok if you
> > > > don't also try to force romfile.
> > > > Generally why are you adding this logic in pci?
> > > Because rom_add_option will call eventually rom_insert 
> > > that will crash QEMU with the call to hw_error.
> > 
> > So fix rom_insert to return an error code instead?
> OK, thanks
> 
> > 
> > > > And what about e.g. vga?
> > > This logic would apply also to rom_add_vga, I was not
> > > aware we can hotplug vga devices. Can we?
> > > I can add it also to vga, of course.
> > > 
> > > > I think the right thing to do is to propagate return codes correctly,
> > > > and report the error where it occurs.
> > > I can remove the error_report, but this gives an extra hint to user.
> > 
> > Move it to rom_insert, instead of hw_error.
> Sure, I was a little "afraid" to change the "crash" policy of rom_insert.
> 
> Thanks,
> Marcel

You will need to audit all users, and make sure they
check the error and handle it.
So it's a lot of work ...

> > 
> > > I didn't see any other way to propagate the error message. 
> > > Should I drop it?
> > > 
> > > Thanks,
> > > Marcel  
> > > > 
> > > > > -- 
> > > > > 1.8.3.1
> > > 
> > > 
> 
> 

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

[Marcel Apfelbaum]

On Wed, 2014-10-22 at 11:31 +0300, Michael S. Tsirkin wrote:
> On Wed, Oct 22, 2014 at 11:16:05AM +0300, Marcel Apfelbaum wrote:
> > On Wed, 2014-10-22 at 10:58 +0300, Michael S. Tsirkin wrote:
> > > On Wed, Oct 22, 2014 at 10:34:21AM +0300, Marcel Apfelbaum wrote:
> > > > On Wed, 2014-10-22 at 09:18 +0300, Michael S. Tsirkin wrote:
> > > > > On Tue, Oct 21, 2014 at 03:37:12PM +0300, Marcel Apfelbaum wrote:
> > > > > > ROM images must be loaded at startup. Usage of rombar=0 after that
> > > > > > is not allowed, but should not crash QEMU.
> > > > > > 
> > > > > > Check that the device is not hotplugged before trying to
> > > > > > insert the rom file.
> > > > > > 
> > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > > > > ---
> > > > > >  hw/pci/pci.c | 11 ++++++++++-
> > > > > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > > > index 6ce75aa..3907c90 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;
> > > > > >  }
> > > > > 
> > > > > Fair enough for this chunk.
> > > > > 
> > > > > > @@ -1940,6 +1945,10 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> > > > > >          if (class == 0x0300) {
> > > > > >              rom_add_vga(pdev->romfile);
> > > > > >          } else {
> > > > > > +            if (DEVICE(pdev)->hotplugged) {
> > > > > > +                error_report("PCI: rombar can't be 0 for hotplugged devices!");
> > > > > > +                return -1;
> > > > > > +            }
> > > > > >              rom_add_option(pdev->romfile, -1);
> > > > > >          }
> > > > > >          return 0;
> > > > > 
> > > > > 
> > > > > The message is confusing. rombar=0 is ok if you
> > > > > don't also try to force romfile.
> > > > > Generally why are you adding this logic in pci?
> > > > Because rom_add_option will call eventually rom_insert 
> > > > that will crash QEMU with the call to hw_error.
> > > 
> > > So fix rom_insert to return an error code instead?
> > OK, thanks
> > 
> > > 
> > > > > And what about e.g. vga?
> > > > This logic would apply also to rom_add_vga, I was not
> > > > aware we can hotplug vga devices. Can we?
> > > > I can add it also to vga, of course.
> > > > 
> > > > > I think the right thing to do is to propagate return codes correctly,
> > > > > and report the error where it occurs.
> > > > I can remove the error_report, but this gives an extra hint to user.
> > > 
> > > Move it to rom_insert, instead of hw_error.
> > Sure, I was a little "afraid" to change the "crash" policy of rom_insert.
> > 
> > Thanks,
> > Marcel
> 
> You will need to audit all users, and make sure they
> check the error and handle it.
> So it's a lot of work ...
The truth is, while I don't mind getting into it,
I was interested in solving the crash issue rather than
re-factoring hw_error.
I'll prefer to find a solution for the crash and deffer
hw_error re-factoring to another series...

Checking "hotplugged" at pci_add_option_rom for both
rom_ad_option and rom_add_vga can be a PCI specific
solution since it connects hotplug -> no rombar=0.

Propagating rom_insert error seems indeed difficult since the 
callers are mostly returning void.

Thanks,
Marcel

> 
> > > 
> > > > I didn't see any other way to propagate the error message. 
> > > > Should I drop it?
> > > > 
> > > > Thanks,
> > > > Marcel  
> > > > > 
> > > > > > -- 
> > > > > > 1.8.3.1
> > > > 
> > > > 
> > 
> > 

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

[Michael S. Tsirkin]

On Wed, Oct 22, 2014 at 06:28:15PM +0300, Marcel Apfelbaum wrote:
> On Wed, 2014-10-22 at 11:31 +0300, Michael S. Tsirkin wrote:
> > On Wed, Oct 22, 2014 at 11:16:05AM +0300, Marcel Apfelbaum wrote:
> > > On Wed, 2014-10-22 at 10:58 +0300, Michael S. Tsirkin wrote:
> > > > On Wed, Oct 22, 2014 at 10:34:21AM +0300, Marcel Apfelbaum wrote:
> > > > > On Wed, 2014-10-22 at 09:18 +0300, Michael S. Tsirkin wrote:
> > > > > > On Tue, Oct 21, 2014 at 03:37:12PM +0300, Marcel Apfelbaum wrote:
> > > > > > > ROM images must be loaded at startup. Usage of rombar=0 after that
> > > > > > > is not allowed, but should not crash QEMU.
> > > > > > > 
> > > > > > > Check that the device is not hotplugged before trying to
> > > > > > > insert the rom file.
> > > > > > > 
> > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > > > > > ---
> > > > > > >  hw/pci/pci.c | 11 ++++++++++-
> > > > > > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > > > > index 6ce75aa..3907c90 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;
> > > > > > >  }
> > > > > > 
> > > > > > Fair enough for this chunk.
> > > > > > 
> > > > > > > @@ -1940,6 +1945,10 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> > > > > > >          if (class == 0x0300) {
> > > > > > >              rom_add_vga(pdev->romfile);
> > > > > > >          } else {
> > > > > > > +            if (DEVICE(pdev)->hotplugged) {
> > > > > > > +                error_report("PCI: rombar can't be 0 for hotplugged devices!");
> > > > > > > +                return -1;
> > > > > > > +            }
> > > > > > >              rom_add_option(pdev->romfile, -1);
> > > > > > >          }
> > > > > > >          return 0;
> > > > > > 
> > > > > > 
> > > > > > The message is confusing. rombar=0 is ok if you
> > > > > > don't also try to force romfile.
> > > > > > Generally why are you adding this logic in pci?
> > > > > Because rom_add_option will call eventually rom_insert 
> > > > > that will crash QEMU with the call to hw_error.
> > > > 
> > > > So fix rom_insert to return an error code instead?
> > > OK, thanks
> > > 
> > > > 
> > > > > > And what about e.g. vga?
> > > > > This logic would apply also to rom_add_vga, I was not
> > > > > aware we can hotplug vga devices. Can we?
> > > > > I can add it also to vga, of course.
> > > > > 
> > > > > > I think the right thing to do is to propagate return codes correctly,
> > > > > > and report the error where it occurs.
> > > > > I can remove the error_report, but this gives an extra hint to user.
> > > > 
> > > > Move it to rom_insert, instead of hw_error.
> > > Sure, I was a little "afraid" to change the "crash" policy of rom_insert.
> > > 
> > > Thanks,
> > > Marcel
> > 
> > You will need to audit all users, and make sure they
> > check the error and handle it.
> > So it's a lot of work ...
> The truth is, while I don't mind getting into it,
> I was interested in solving the crash issue rather than
> re-factoring hw_error.
> I'll prefer to find a solution for the crash and deffer
> hw_error re-factoring to another series...
> 
> Checking "hotplugged" at pci_add_option_rom for both
> rom_ad_option and rom_add_vga can be a PCI specific
> solution since it connects hotplug -> no rombar=0.
> 
> Propagating rom_insert error seems indeed difficult since the 
> callers are mostly returning void.
> 
> Thanks,
> Marcel

But it's ugly, I'm not sure the crash as result of user error
is important enough to justify this.

> > 
> > > > 
> > > > > I didn't see any other way to propagate the error message. 
> > > > > Should I drop it?
> > > > > 
> > > > > Thanks,
> > > > > Marcel  
> > > > > > 
> > > > > > > -- 
> > > > > > > 1.8.3.1
> > > > > 
> > > > > 
> > > 
> > > 
> 
>