| Message ID | alpine.LFD.2.02.1201081253310.14379@bbs.intern |
|---|---|
| State | New |
| Headers | show |
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
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/
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
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
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/
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
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/
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; }
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(-)