Patchwork network: Added option to disable NIC option roms

login
register
mail settings
Submitter Gerhard Wiesinger
Date Jan. 8, 2012, 11:55 a.m.
Message ID <alpine.LFD.2.02.1201081253310.14379@bbs.intern>
Download mbox | patch
Permalink /patch/134899/
State New
Headers show

Comments

Gerhard Wiesinger - Jan. 8, 2012, 11:55 a.m.
Option ROM for network interface cards (NICs) can now explicitly disabled
with romfile=disabled parameter. With hotplugable NICs (currently NE2000, PCNET)
romfile=(empty) didn't work. This patch disables Option ROMs for iPXE for alls
supported NICs (hotplugable and non hotplugable).

Examples with 2 NICs with disabled Option ROM (separated on different lines for readability):
-device rtl8139,mac=1a:46:0b:ca:bc:7c,vlan=0,romfile=disabled
-net tap,ifname=tap0,script=no,downscript=no,vlan=0
-device pcnet,mac=1a:46:0b:ca:bc:7e,vlan=1,romfile=disabled
-net tap,ifname=tap1,script=no,downscript=no,vlan=1

Signed-off-by: Gerhard Wiesinger <lists@wiesinger.com>
---
  hw/ne2000.c    |    2 +-
  hw/pci.c       |   28 +++++++++++++++++++++++++---
  hw/pci.h       |    5 +++++
  hw/pcnet-pci.c |    2 +-
  4 files changed, 32 insertions(+), 5 deletions(-)
Stefan Hajnoczi - Jan. 8, 2012, 4:40 p.m.
On Sun, Jan 8, 2012 at 11:55 AM, Gerhard Wiesinger <lists@wiesinger.com> wrote:
> Option ROM for network interface cards (NICs) can now explicitly disabled
> with romfile=disabled parameter. With hotplugable NICs (currently NE2000,
> PCNET)
> romfile=(empty) didn't work. This patch disables Option ROMs for iPXE for
> alls
> supported NICs (hotplugable and non hotplugable).
>
> Examples with 2 NICs with disabled Option ROM (separated on different lines
> for readability):
> -device rtl8139,mac=1a:46:0b:ca:bc:7c,vlan=0,romfile=disabled
> -net tap,ifname=tap0,script=no,downscript=no,vlan=0
> -device pcnet,mac=1a:46:0b:ca:bc:7e,vlan=1,romfile=disabled
> -net tap,ifname=tap1,script=no,downscript=no,vlan=1

Did you consider "no" or "none"?  Those are already used by -net
script=no and -vga none.  I'm afraid we don't have much consistency in
the command-line and adding more variants of basically the same
concept should be avoided when possible.

Stefan
Gerhard Wiesinger - Jan. 8, 2012, 8:56 p.m.
On Sun, 8 Jan 2012, Stefan Hajnoczi wrote:

> On Sun, Jan 8, 2012 at 11:55 AM, Gerhard Wiesinger <lists@wiesinger.com> wrote:
>> Option ROM for network interface cards (NICs) can now explicitly disabled
>> with romfile=disabled parameter. With hotplugable NICs (currently NE2000,
>> PCNET)
>> romfile=(empty) didn't work. This patch disables Option ROMs for iPXE for
>> alls
>> supported NICs (hotplugable and non hotplugable).
>>
>> Examples with 2 NICs with disabled Option ROM (separated on different lines
>> for readability):
>> -device rtl8139,mac=1a:46:0b:ca:bc:7c,vlan=0,romfile=disabled
>> -net tap,ifname=tap0,script=no,downscript=no,vlan=0
>> -device pcnet,mac=1a:46:0b:ca:bc:7e,vlan=1,romfile=disabled
>> -net tap,ifname=tap1,script=no,downscript=no,vlan=1
>
> Did you consider "no" or "none"?  Those are already used by -net
> script=no and -vga none.  I'm afraid we don't have much consistency in
> the command-line and adding more variants of basically the same
> concept should be avoided when possible.

Whether the option is spelled "disabled", "no", "none" or all of them 
above: I don't care about that, just the group of maintainers should 
decide (I would prefer all of them ...).

The only thing: just help me with git and doing another commit and 
preparing V2 patch then ...

# I did:
git stash save mypatch
git branch -b mybranch lastcommit
# can be optimized with -b option ...
git checkout mybranch
git stash pop
git merge --squash master
git commit -a -F - <<EOF
commit message
EOF
# Signoff must be added
git format-patch -s master
git checkout master
# Send mail

Ciao,
Gerhard

--
http://www.wiesinger.com/
Stefan Hajnoczi - Jan. 9, 2012, 8:06 a.m.
On Sun, Jan 08, 2012 at 09:56:20PM +0100, Gerhard Wiesinger wrote:
> On Sun, 8 Jan 2012, Stefan Hajnoczi wrote:
> 
> >On Sun, Jan 8, 2012 at 11:55 AM, Gerhard Wiesinger <lists@wiesinger.com> wrote:
> >>Option ROM for network interface cards (NICs) can now explicitly disabled
> >>with romfile=disabled parameter. With hotplugable NICs (currently NE2000,
> >>PCNET)
> >>romfile=(empty) didn't work. This patch disables Option ROMs for iPXE for
> >>alls
> >>supported NICs (hotplugable and non hotplugable).
> >>
> >>Examples with 2 NICs with disabled Option ROM (separated on different lines
> >>for readability):
> >>-device rtl8139,mac=1a:46:0b:ca:bc:7c,vlan=0,romfile=disabled
> >>-net tap,ifname=tap0,script=no,downscript=no,vlan=0
> >>-device pcnet,mac=1a:46:0b:ca:bc:7e,vlan=1,romfile=disabled
> >>-net tap,ifname=tap1,script=no,downscript=no,vlan=1
> >
> >Did you consider "no" or "none"?  Those are already used by -net
> >script=no and -vga none.  I'm afraid we don't have much consistency in
> >the command-line and adding more variants of basically the same
> >concept should be avoided when possible.
> 
> Whether the option is spelled "disabled", "no", "none" or all of
> them above: I don't care about that, just the group of maintainers
> should decide (I would prefer all of them ...).
> 
> The only thing: just help me with git and doing another commit and
> preparing V2 patch then ...
> 
> # I did:
> git stash save mypatch
> git branch -b mybranch lastcommit
> # can be optimized with -b option ...
> git checkout mybranch
> git stash pop
> git merge --squash master
> git commit -a -F - <<EOF
> commit message
> EOF
> # Signoff must be added
> git format-patch -s master
> git checkout master
> # Send mail

Okay, you commited your patch onto the 'mypatch' branch.

$ git checkout mypatch  # go back to your branch
$ $EDITOR               # make changes to the code
$ git commit --amend -a # modify the last commit
$ git format-patch -s HEAD^ # generate new patch

Stefan
Gerd Hoffmann - Jan. 9, 2012, 9:15 a.m.
Hi,

>      if (!pci_dev->qdev.hotplugged) {
>          static int loaded = 0;
> -        if (!loaded) {
> +        if (!loaded &&
> pci_has_not_explicitly_disabled_option_romfile(pci_dev)) {
>              rom_add_option("pxe-ne2k_pci.rom", -1);
>              loaded = 1;
>          }

I think you can just remove this altogether and add a .romfile = "..."
entry instead like it is done for the other nics.

cheers,
  Gerd
Gerhard Wiesinger - Jan. 12, 2012, 6:45 a.m.
On Mon, 9 Jan 2012, Gerd Hoffmann wrote:

>  Hi,
>
>>      if (!pci_dev->qdev.hotplugged) {
>>          static int loaded = 0;
>> -        if (!loaded) {
>> +        if (!loaded &&
>> pci_has_not_explicitly_disabled_option_romfile(pci_dev)) {
>>              rom_add_option("pxe-ne2k_pci.rom", -1);
>>              loaded = 1;
>>          }
>
> I think you can just remove this altogether and add a .romfile = "..."
> entry instead like it is done for the other nics.

I'm not sure about the consequences (hotplugging feature, etc.) when 
changing it to romfile as in other PCI devices. Also the patch is more 
generic and supports static and dynamic devices (hotplugable and possible 
future devices). Patch supports without hotplugging both options to 
disable the romfile:
,romfile=
,romfile=disabled

And the patch has already been tested ...

Therefore I suggest to commit it and maybe make a refactoring without 
hotplugging later on.

Ciao,
Gerhard

--
http://www.wiesinger.com/
Gerd Hoffmann - Jan. 12, 2012, 7:58 a.m.
Hi,

> I'm not sure about the consequences (hotplugging feature, etc.) when
> changing it to romfile as in other PCI devices.

There should be no noticable difference.

> Also the patch is more
> generic and supports static and dynamic devices (hotplugable and
> possible future devices).

Hotplug works just fine for the other pci devices.

> Patch supports without hotplugging both
> options to disable the romfile:
> ,romfile=
> ,romfile=disabled

That should be a separate patch.  And it should update
pci_add_option_rom() so it works equally for all pci devices.

> And the patch has already been tested ...

Well, it adds a bunch of code which would not be needed in the first
place if you would simply make use of the romfile support of the pci layer.

cheers,
  Gerd
Gerhard Wiesinger - Jan. 25, 2012, 9:08 p.m.
On Thu, 12 Jan 2012, Gerd Hoffmann wrote:

>  Hi,
>
>> I'm not sure about the consequences (hotplugging feature, etc.) when
>> changing it to romfile as in other PCI devices.
>
> There should be no noticable difference.

I don't know the consequences there so I think it is better to let that 
code and just fix the romfile issues.

>
>> Also the patch is more
>> generic and supports static and dynamic devices (hotplugable and
>> possible future devices).
>
> Hotplug works just fine for the other pci devices.
>
>> Patch supports without hotplugging both
>> options to disable the romfile:
>> ,romfile=
>> ,romfile=disabled
>
> That should be a separate patch.  And it should update
> pci_add_option_rom() so it works equally for all pci devices.

pci_add_option_rom() has been updated, so it is generic for all pci 
devices. Please look at the patch ...

>> And the patch has already been tested ...
>
> Well, it adds a bunch of code which would not be needed in the first
> place if you would simply make use of the romfile support of the pci layer.

As discussed it modifies romfile support on the pci layer ...

I reworked code for "disabled", "no" and "none" options, all these options 
work now.

Submitted new patch.

Ciao,
Gerhard

--
http://www.wiesinger.com/

Patch

diff --git a/hw/ne2000.c b/hw/ne2000.c
index 62e082f..67bf458 100644
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -765,7 +765,7 @@  static int pci_ne2000_init(PCIDevice *pci_dev)

      if (!pci_dev->qdev.hotplugged) {
          static int loaded = 0;
-        if (!loaded) {
+        if (!loaded && pci_has_not_explicitly_disabled_option_romfile(pci_dev)) {
              rom_add_option("pxe-ne2k_pci.rom", -1);
              loaded = 1;
          }
diff --git a/hw/pci.c b/hw/pci.c
index c3082bc..fbce1a7 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -126,6 +126,30 @@  static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change)
      bus->set_irq(bus->irq_opaque, irq_num, bus->irq_count[irq_num] != 0);
  }

+int pci_has_enabled_option_romfile(PCIDevice *pdev)
+{
+    PCI_DPRINTF("pci_has_enabled_option_romfile: device=%s, romfile=%s\n", pdev->name, pdev->romfile);
+    if (pdev->romfile == NULL)
+        return 0;
+    if (strlen(pdev->romfile) == 0)
+        return 0;
+    if (strcmp(pdev->romfile, PCI_DEVICE_DISABLED_OPTION_ROMFILE) == 0)
+        return 0;
+    return 1;
+}
+
+int pci_has_not_explicitly_disabled_option_romfile(PCIDevice *pdev)
+{
+    PCI_DPRINTF("pci_has_not_explicitly_disabled_option_romfile: device=%s, romfile=%s\n", pdev->name, pdev->romfile);
+
+    /* No romfile is present for hotplugged devices, therefore dynamic codes decides */
+    if (pdev->romfile == NULL)
+        return 1;
+    if (strcmp(pdev->romfile, PCI_DEVICE_DISABLED_OPTION_ROMFILE) == 0)
+        return 0;
+    return 1;
+}
+
  int pci_bus_get_irq_level(PCIBus *bus, int irq_num)
  {
      assert(irq_num >= 0);
@@ -1725,9 +1749,7 @@  static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
      void *ptr;
      char name[32];

-    if (!pdev->romfile)
-        return 0;
-    if (strlen(pdev->romfile) == 0)
+    if (!pci_has_enabled_option_romfile(pdev))
          return 0;

      if (!pdev->rom_bar) {
diff --git a/hw/pci.h b/hw/pci.h
index 625e717..5146bba 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -78,6 +78,8 @@ 

  #define FMT_PCIBUS                      PRIx64

+#define PCI_DEVICE_DISABLED_OPTION_ROMFILE "disabled"
+
  typedef void PCIConfigWriteFunc(PCIDevice *pci_dev,
                                  uint32_t address, uint32_t data, int len);
  typedef uint32_t PCIConfigReadFunc(PCIDevice *pci_dev,
@@ -275,6 +277,9 @@  int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp,

  void pci_device_deassert_intx(PCIDevice *dev);

+int pci_has_enabled_option_romfile(PCIDevice *pdev);
+int pci_has_not_explicitly_disabled_option_romfile(PCIDevice *pdev);
+
  static inline void
  pci_set_byte(uint8_t *config, uint8_t val)
  {
diff --git a/hw/pcnet-pci.c b/hw/pcnet-pci.c
index 4e164da..e65745f 100644
--- a/hw/pcnet-pci.c
+++ b/hw/pcnet-pci.c
@@ -332,7 +332,7 @@  static int pci_pcnet_init(PCIDevice *pci_dev)

      if (!pci_dev->qdev.hotplugged) {
          static int loaded = 0;
-        if (!loaded) {
+        if (!loaded && pci_has_not_explicitly_disabled_option_romfile(pci_dev)) {
              rom_add_option("pxe-pcnet.rom", -1);
              loaded = 1;
          }