[Qemu-devel] [PATCH] Do not try loading option ROM for hotplug PCI device in pc-0.11 compat mode

[Qemu-devel] [PATCH] Do not try loading option ROM for hotplug PCI device in pc-0.11 compat mode

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

pc-0.11 and older uses fw_cfg to provide option ROMs. As fw_cfg is setup
at init time, it is not possible to load an option ROM for a hotplug
device when running in compat mode.

v2: Alex Williamson pointed out that one can get to qdev directly from
pci_dev, so no need to pass it down.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 hw/pci.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index a98d6f3..2d38643 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1810,11 +1810,15 @@ static int pci_add_option_rom(PCIDevice *pdev)
         return 0;
 
     if (!pdev->rom_bar) {
+        int class;
         /*
          * Load rom via fw_cfg instead of creating a rom bar,
-         * for 0.11 compatibility.
+         * for 0.11 compatibility. fw_cfg is initialized at boot, so
+         * we cannot do hotplug load of option roms.
          */
-        int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
+        if (pdev->qdev.hotplugged)
+            return 0;
+        class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
         if (class == 0x0300) {
             rom_add_vga(pdev->romfile);
         } else {
-- 
1.7.1.1

Re: [Qemu-devel] [PATCH] Do not try loading option ROM for hotplug PCI device in pc-0.11 compat mode

[Aurelien Jarno]

On Fri, Jul 23, 2010 at 05:56:38PM +0200, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> pc-0.11 and older uses fw_cfg to provide option ROMs. As fw_cfg is setup
> at init time, it is not possible to load an option ROM for a hotplug
> device when running in compat mode.
> 
> v2: Alex Williamson pointed out that one can get to qdev directly from
> pci_dev, so no need to pass it down.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  hw/pci.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index a98d6f3..2d38643 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1810,11 +1810,15 @@ static int pci_add_option_rom(PCIDevice *pdev)
>          return 0;
>  
>      if (!pdev->rom_bar) {
> +        int class;
>          /*
>           * Load rom via fw_cfg instead of creating a rom bar,
> -         * for 0.11 compatibility.
> +         * for 0.11 compatibility. fw_cfg is initialized at boot, so
> +         * we cannot do hotplug load of option roms.
>           */
> -        int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> +        if (pdev->qdev.hotplugged)
> +            return 0;

Missing braces around the return 0 line. 

> +        class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
>          if (class == 0x0300) {
>              rom_add_vga(pdev->romfile);
>          } else {
> -- 
> 1.7.1.1

Otherwise looks good.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] Do not try loading option ROM for hotplug PCI device in pc-0.11 compat mode

[Jes Sorensen]

On 07/30/10 23:08, Aurelien Jarno wrote:
> On Fri, Jul 23, 2010 at 05:56:38PM +0200, Jes.Sorensen@redhat.com wrote:
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> pc-0.11 and older uses fw_cfg to provide option ROMs. As fw_cfg is setup
>> at init time, it is not possible to load an option ROM for a hotplug
>> device when running in compat mode.
>>
>> v2: Alex Williamson pointed out that one can get to qdev directly from
>> pci_dev, so no need to pass it down.
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>>  hw/pci.c |    8 ++++++--
>>  1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pci.c b/hw/pci.c
>> index a98d6f3..2d38643 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -1810,11 +1810,15 @@ static int pci_add_option_rom(PCIDevice *pdev)
>>          return 0;
>>  
>>      if (!pdev->rom_bar) {
>> +        int class;
>>          /*
>>           * Load rom via fw_cfg instead of creating a rom bar,
>> -         * for 0.11 compatibility.
>> +         * for 0.11 compatibility. fw_cfg is initialized at boot, so
>> +         * we cannot do hotplug load of option roms.
>>           */
>> -        int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
>> +        if (pdev->qdev.hotplugged)
>> +            return 0;
> 
> Missing braces around the return 0 line. 

Half the QEMU code base doesn't have braces around single line if
statements, including in hw/pci.c. Adding braces here would be inconsistent.

Jes

Re: [Qemu-devel] [PATCH] Do not try loading option ROM for hotplug PCI device in pc-0.11 compat mode

[Aurelien Jarno]

On Sat, Jul 31, 2010 at 11:16:45AM +0200, Jes Sorensen wrote:
> On 07/30/10 23:08, Aurelien Jarno wrote:
> > On Fri, Jul 23, 2010 at 05:56:38PM +0200, Jes.Sorensen@redhat.com wrote:
> >> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> >>
> >> pc-0.11 and older uses fw_cfg to provide option ROMs. As fw_cfg is setup
> >> at init time, it is not possible to load an option ROM for a hotplug
> >> device when running in compat mode.
> >>
> >> v2: Alex Williamson pointed out that one can get to qdev directly from
> >> pci_dev, so no need to pass it down.
> >>
> >> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> >> ---
> >>  hw/pci.c |    8 ++++++--
> >>  1 files changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/pci.c b/hw/pci.c
> >> index a98d6f3..2d38643 100644
> >> --- a/hw/pci.c
> >> +++ b/hw/pci.c
> >> @@ -1810,11 +1810,15 @@ static int pci_add_option_rom(PCIDevice *pdev)
> >>          return 0;
> >>  
> >>      if (!pdev->rom_bar) {
> >> +        int class;
> >>          /*
> >>           * Load rom via fw_cfg instead of creating a rom bar,
> >> -         * for 0.11 compatibility.
> >> +         * for 0.11 compatibility. fw_cfg is initialized at boot, so
> >> +         * we cannot do hotplug load of option roms.
> >>           */
> >> -        int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> >> +        if (pdev->qdev.hotplugged)
> >> +            return 0;
> > 
> > Missing braces around the return 0 line. 
> 
> Half the QEMU code base doesn't have braces around single line if
> statements, including in hw/pci.c. Adding braces here would be inconsistent.
> 

If we follow the coding style in new patches, it will eventually become
consistent.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] Do not try loading option ROM for hotplug PCI device in pc-0.11 compat mode

[Jes Sorensen]

On 07/31/10 16:19, Aurelien Jarno wrote:
> On Sat, Jul 31, 2010 at 11:16:45AM +0200, Jes Sorensen wrote:
>> On 07/30/10 23:08, Aurelien Jarno wrote:
>>> Missing braces around the return 0 line. 
>>
>> Half the QEMU code base doesn't have braces around single line if
>> statements, including in hw/pci.c. Adding braces here would be inconsistent.
>>
> 
> If we follow the coding style in new patches, it will eventually become
> consistent.
> 

If you want that, please do it in a separate patch for the entire file,
otherwise it will never become consistent. However it doesn't change the
issue either that putting braces around a single line like this is bad
coding style.

Jes

Re: [Qemu-devel] [PATCH] Do not try loading option ROM for hotplug PCI device in pc-0.11 compat mode

[Aurelien Jarno]

On Sat, Jul 31, 2010 at 04:40:05PM +0200, Jes Sorensen wrote:
> On 07/31/10 16:19, Aurelien Jarno wrote:
> > On Sat, Jul 31, 2010 at 11:16:45AM +0200, Jes Sorensen wrote:
> >> On 07/30/10 23:08, Aurelien Jarno wrote:
> >>> Missing braces around the return 0 line. 
> >>
> >> Half the QEMU code base doesn't have braces around single line if
> >> statements, including in hw/pci.c. Adding braces here would be inconsistent.
> >>
> > 
> > If we follow the coding style in new patches, it will eventually become
> > consistent.
> > 
> 
> If you want that, please do it in a separate patch for the entire file,
> otherwise it will never become consistent. However it doesn't change the
> issue either that putting braces around a single line like this is bad
> coding style.

We got this discussions numerous times, and I am not going to start it
again. If you want to see the patch applied, just follow the rules.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] Do not try loading option ROM for hotplug PCI device in pc-0.11 compat mode

[Jes Sorensen]

On 07/31/10 17:13, Aurelien Jarno wrote:
> On Sat, Jul 31, 2010 at 04:40:05PM +0200, Jes Sorensen wrote:
>> If you want that, please do it in a separate patch for the entire file,
>> otherwise it will never become consistent. However it doesn't change the
>> issue either that putting braces around a single line like this is bad
>> coding style.
> 
> We got this discussions numerous times, and I am not going to start it
> again. If you want to see the patch applied, just follow the rules.
> 

Then  I would like to know why you do not apply this silly rule to every
patch submitted. There's patches going in almost every day that doesn't
use this hopeless formatting.

Jes

Re: [Qemu-devel] [PATCH] Do not try loading option ROM for hotplug PCI device in pc-0.11 compat mode

[Aurelien Jarno]

On Sat, Jul 31, 2010 at 05:21:34PM +0200, Jes Sorensen wrote:
> On 07/31/10 17:13, Aurelien Jarno wrote:
> > On Sat, Jul 31, 2010 at 04:40:05PM +0200, Jes Sorensen wrote:
> >> If you want that, please do it in a separate patch for the entire file,
> >> otherwise it will never become consistent. However it doesn't change the
> >> issue either that putting braces around a single line like this is bad
> >> coding style.
> > 
> > We got this discussions numerous times, and I am not going to start it
> > again. If you want to see the patch applied, just follow the rules.
> > 
> 
> Then  I would like to know why you do not apply this silly rule to every
> patch submitted. There's patches going in almost every day that doesn't
> use this hopeless formatting.
> 

Because not everyone try to enforce the rules.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] [PATCH] Do not try loading option ROM for hotplug PCI device in pc-0.11 compat mode

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

pc-0.11 and older uses fw_cfg to provide option ROMs. As fw_cfg is setup
at init time, it is not possible to load an option ROM for a hotplug
device when running in compat mode.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 hw/pci.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index a98d6f3..12bd4aa 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -77,7 +77,7 @@ static struct BusInfo pci_bus_info = {
 
 static void pci_update_mappings(PCIDevice *d);
 static void pci_set_irq(void *opaque, int irq_num, int level);
-static int pci_add_option_rom(PCIDevice *pdev);
+static int pci_add_option_rom(PCIDevice *pdev, int hotplugged);
 static void pci_del_option_rom(PCIDevice *pdev);
 
 static uint16_t pci_default_sub_vendor_id = PCI_SUBVENDOR_ID_REDHAT_QUMRANET;
@@ -1693,7 +1693,7 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
     /* rom loading */
     if (pci_dev->romfile == NULL && info->romfile != NULL)
         pci_dev->romfile = qemu_strdup(info->romfile);
-    pci_add_option_rom(pci_dev);
+    pci_add_option_rom(pci_dev, qdev->hotplugged);
 
     if (qdev->hotplugged) {
         rc = bus->hotplug(bus->hotplug_qdev, pci_dev, 1);
@@ -1797,7 +1797,7 @@ static void pci_map_option_rom(PCIDevice *pdev, int region_num, pcibus_t addr, p
 }
 
 /* Add an option rom for the device */
-static int pci_add_option_rom(PCIDevice *pdev)
+static int pci_add_option_rom(PCIDevice *pdev, int hotplugged)
 {
     int size;
     char *path;
@@ -1810,11 +1810,15 @@ static int pci_add_option_rom(PCIDevice *pdev)
         return 0;
 
     if (!pdev->rom_bar) {
+        int class;
         /*
          * Load rom via fw_cfg instead of creating a rom bar,
-         * for 0.11 compatibility.
+         * for 0.11 compatibility. fw_cfg is initialized at boot, so
+         * we cannot do hotplug load of option roms.
          */
-        int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
+        if (hotplugged)
+            return 0;
+        class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
         if (class == 0x0300) {
             rom_add_vga(pdev->romfile);
         } else {
-- 
1.7.1.1

Re: [Qemu-devel] [PATCH] Do not try loading option ROM for hotplug PCI device in pc-0.11 compat mode

[Markus Armbruster]

Jes.Sorensen@redhat.com writes:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> pc-0.11 and older uses fw_cfg to provide option ROMs. As fw_cfg is setup
> at init time, it is not possible to load an option ROM for a hotplug
> device when running in compat mode.

Example:

    $ qemu -M pc-0.11 -S -monitor stdio
    QEMU 0.12.50 monitor - type 'help' for more information
    (qemu) device_add e1000
    qemu: hardware error: ROM images must be loaded at startup
[...]
    Aborted (core dumped)

The fix silently omits the option ROM when we can't load it.  Works for
me.