| Message ID | 1413895032-10116-1-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
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; >
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
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; > >
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
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; > >
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 > >
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; > > > > >
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 > > > >
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 > > > > > > > >
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 > > > > > > > > > > > >
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 > > > > > > > > > > > > > > > > > >
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; > > > > > > > > >
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; > > > > > > > > > > > > > > > >
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;
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(-)