From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=55496 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Oy4aZ-0004MZ-EN for qemu-devel@nongnu.org; Tue, 21 Sep 2010 11:17:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Oy4aT-0004xB-MB for qemu-devel@nongnu.org; Tue, 21 Sep 2010 11:16:59 -0400 Received: from mail-px0-f173.google.com ([209.85.212.173]:42184) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Oy4aT-0004uX-AY for qemu-devel@nongnu.org; Tue, 21 Sep 2010 11:16:53 -0400 Received: by pxi12 with SMTP id 12so1636460pxi.4 for ; Tue, 21 Sep 2010 08:16:50 -0700 (PDT) Message-ID: <4C98CC4D.1040907@codemonkey.ws> Date: Tue, 21 Sep 2010 10:16:29 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [PATCH] new parameter boot=on|off for "-net nic" and "-device" NIC devices References: <1284479215-24679-1-git-send-email-bernhard.kohl@nsn.com> <20100919160759.GB10730@redhat.com> In-Reply-To: <20100919160759.GB10730@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Bernhard Kohl , Thomas Ostler , qemu-devel@nongnu.org On 09/19/2010 11:07 AM, Michael S. Tsirkin wrote: > On Tue, Sep 14, 2010 at 05:46:55PM +0200, Bernhard Kohl wrote: > >> This patch was motivated by the following use case: In our system >> the VMs usually have 4 NICs, any combination of virtio-net-pci and >> pci-assign NIC devices. The VMs boot via gPXE preferably over the >> pci-assign devices. >> >> There is no way to make this working with a combination of the >> current options -net -pcidevice -device -optionrom -boot. >> >> With the parameter boot=off it is possible to avoid loading >> and using gPXE option ROMs either for old style "-net nic" or >> for "-device" NIC devices. So we can select which NIC is used >> for booting. >> >> A side effect of the boot=off parameter is that unneeded ROMs >> which might waste memory are not longer loaded. E.g. if you have >> 2 virtio-net-pci and 2 pci-assign NICs in sum 4 option ROMs are >> loaded and the virtio ROMs take precedence over the pci-assign >> ROMs. The BIOS uses the first gPXE ROM which it finds and only >> needs one of them even if there are more NICs of the same type. >> >> Without using the boot=on|off parameter the current behaviour >> does not change. >> >> Signed-off-by: Thomas Ostler >> Signed-off-by: Bernhard Kohl >> > I think this is useful, however: > > - We have bit properties which handle parsing on/off > and other formats automatically. Please don't use string. > This is unneeded. Just do romfile= with -device and it will suppress the option rom loading. IOW: -device virtio-net-pci,romfile= -pcidevice ... But BTW, you should be able to select the pci device by doing: -boot cdn,menu=on And then hitting F12. We need to come up with a better way to let particular BEV or BCV devices to be chosen from the command line. Regards, Anthony Liguori > - boot is not a great property name for PCI: what > you actually do is disable option rom. > So maybe call it 'rom' or something like that? > - given you have added a property, it can now > be changed with -device. and visible in -device ? > This also has an advantage of only applying to pci devices > (-net option would appear to apply to non-pci but have no effect). > Please do not add more flag parsing in qdemu-options, net and vl.c > > To summarize, just add a qdev bit option and check > the bit. > > >> --- >> hw/pci.c | 8 +++++++- >> hw/pci.h | 1 + >> hw/qdev.c | 6 ++++++ >> hw/qdev.h | 1 + >> net.c | 8 ++++++++ >> net.h | 1 + >> qemu-options.hx | 8 ++++++-- >> vl.c | 27 +++++++++++++++++++++++++++ >> 8 files changed, 57 insertions(+), 3 deletions(-) >> >> diff --git a/hw/pci.c b/hw/pci.c >> index a98d6f3..055a2be 100644 >> --- a/hw/pci.c >> +++ b/hw/pci.c >> @@ -71,6 +71,7 @@ static struct BusInfo pci_bus_info = { >> DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1), >> DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present, >> QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false), >> + DEFINE_PROP_STRING("boot", PCIDevice, boot), >> DEFINE_PROP_END_OF_LIST() >> } >> }; >> @@ -1513,6 +1514,10 @@ PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model, >> >> pci_dev = pci_create(bus, devfn, pci_nic_names[i]); >> dev =&pci_dev->qdev; >> + if (nd->name) >> + dev->id = qemu_strdup(nd->name); >> + if (nd->no_boot) >> + dev->no_boot = 1; >> qdev_set_nic_properties(dev, nd); >> if (qdev_init(dev)< 0) >> return NULL; >> @@ -1693,7 +1698,8 @@ 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); >> + if (!qdev->no_boot) >> + pci_add_option_rom(pci_dev); >> >> if (qdev->hotplugged) { >> rc = bus->hotplug(bus->hotplug_qdev, pci_dev, 1); >> diff --git a/hw/pci.h b/hw/pci.h >> index 1eab7e7..20aa038 100644 >> --- a/hw/pci.h >> +++ b/hw/pci.h >> @@ -172,6 +172,7 @@ struct PCIDevice { >> char *romfile; >> ram_addr_t rom_offset; >> uint32_t rom_bar; >> + char *boot; >> }; >> >> PCIDevice *pci_register_device(PCIBus *bus, const char *name, >> diff --git a/hw/qdev.c b/hw/qdev.c >> index 35858cb..8445bc9 100644 >> --- a/hw/qdev.c >> +++ b/hw/qdev.c >> @@ -249,6 +249,10 @@ DeviceState *qdev_device_add(QemuOpts *opts) >> qdev_free(qdev); >> return NULL; >> } >> + if (qemu_opt_get(opts, "boot")) { >> + if (!strcmp("off", qemu_strdup(qemu_opt_get(opts, "boot")))) >> + qdev->no_boot = 1; >> + } >> if (qdev_init(qdev)< 0) { >> qerror_report(QERR_DEVICE_INIT_FAILED, driver); >> return NULL; >> @@ -421,6 +425,8 @@ void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd) >> qdev_prop_exists(dev, "vectors")) { >> qdev_prop_set_uint32(dev, "vectors", nd->nvectors); >> } >> + if (nd->no_boot) >> + qdev_prop_parse(dev, "boot", "off"); >> } >> >> static int next_block_unit[IF_COUNT]; >> diff --git a/hw/qdev.h b/hw/qdev.h >> index 579328a..e7df371 100644 >> --- a/hw/qdev.h >> +++ b/hw/qdev.h >> @@ -45,6 +45,7 @@ struct DeviceState { >> QLIST_ENTRY(DeviceState) sibling; >> int instance_id_alias; >> int alias_required_for_version; >> + int no_boot; >> }; >> >> typedef void (*bus_dev_printfn)(Monitor *mon, DeviceState *dev, int indent); >> diff --git a/net.c b/net.c >> index 3d0fde7..2370aca 100644 >> --- a/net.c >> +++ b/net.c >> @@ -792,6 +792,10 @@ static int net_init_nic(QemuOpts *opts, >> if (qemu_opt_get(opts, "addr")) { >> nd->devaddr = qemu_strdup(qemu_opt_get(opts, "addr")); >> } >> + if (qemu_opt_get(opts, "boot")) { >> + if (!strcmp("off", qemu_strdup(qemu_opt_get(opts, "boot")))) >> + nd->no_boot = 1; >> + } >> >> nd->macaddr[0] = 0x52; >> nd->macaddr[1] = 0x54; >> @@ -877,6 +881,10 @@ static const struct { >> .type = QEMU_OPT_STRING, >> .help = "PCI device address", >> }, { >> + .name = "boot", >> + .type = QEMU_OPT_STRING, >> + .help = "gPXE boot (on (default), off)", >> + }, { >> .name = "vectors", >> .type = QEMU_OPT_NUMBER, >> .help = "number of MSI-x vectors, 0 to disable MSI-X", >> diff --git a/net.h b/net.h >> index 518cf9c..288059b 100644 >> --- a/net.h >> +++ b/net.h >> @@ -132,6 +132,7 @@ struct NICInfo { >> VLANClientState *netdev; >> int used; >> int nvectors; >> + int no_boot; >> }; >> >> extern int nb_nics; >> diff --git a/qemu-options.hx b/qemu-options.hx >> index a0b5ae9..6084aa9 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -959,8 +959,10 @@ DEF("smb", HAS_ARG, QEMU_OPTION_smb, "", QEMU_ARCH_ALL) >> #endif >> >> DEF("net", HAS_ARG, QEMU_OPTION_net, >> - "-net nic[,vlan=n][,macaddr=mac][,model=type][,name=str][,addr=str][,vectors=v]\n" >> + "-net nic[,vlan=n][,macaddr=mac][,model=type][,name=str][,addr=str][,vectors=v][,boot=on|off]\n" >> " create a new Network Interface Card and connect it to VLAN 'n'\n" >> + " use 'boot=on|off' to enable/disable loading of an option rom;\n" >> + " loading enabled is the default\n" >> #ifdef CONFIG_SLIRP >> "-net user[,vlan=n][,name=str][,net=addr[/mask]][,host=addr][,restrict=y|n]\n" >> " [,hostname=host][,dhcpstart=addr][,dns=addr][,tftp=dir][,bootfile=f]\n" >> @@ -1014,13 +1016,15 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev, >> #endif >> "socket],id=str[,option][,option][,...]\n", QEMU_ARCH_ALL) >> STEXI >> -@item -net nic[,vlan=@var{n}][,macaddr=@var{mac}][,model=@var{type}] [,name=@var{name}][,addr=@var{addr}][,vectors=@var{v}] >> +@item -net nic[,vlan=@var{n}][,macaddr=@var{mac}][,model=@var{type}] [,name=@var{name}][,addr=@var{addr}][,vectors=@var{v}][,boot=on|off] >> @findex -net >> Create a new Network Interface Card and connect it to VLAN @var{n} (@var{n} >> = 0 is the default). The NIC is an e1000 by default on the PC >> target. Optionally, the MAC address can be changed to @var{mac}, the >> device address set to @var{addr} (PCI cards only), >> and a @var{name} can be assigned for use in monitor commands. >> +Optionally, with @option{boot=on|off}, you can enable/disable the loading of an option >> +rom; by default, loading is enabled. >> Optionally, for PCI cards, you can specify the number @var{v} of MSI-X vectors >> that the card should have; this option currently only affects virtio cards; set >> @var{v} = 0 to disable MSI-X. If no @option{-net} option is specified, a single >> diff --git a/vl.c b/vl.c >> index 3f45aa9..2aad6b1 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -2459,6 +2459,33 @@ int main(int argc, char **argv, char **envp) >> if (!qemu_opts_parse(qemu_find_opts("device"), optarg, 1)) { >> exit(1); >> } >> + >> + /* check whether option "boot" is present in the cmd string */ >> + /* for this a modified string is created that does not */ >> + /* contain the driver */ >> + /* if "boot" is present and set to "on", the relevant */ >> + /* variables are set in a way that net boot is possible and */ >> + /* that a present "romfile" is loaded for the given device */ >> + /* note that "default_net" is set to zero in order to avoid */ >> + /* creation of a default device if option "-net" is not */ >> + /* present in the complete command line */ >> + { >> + char mod_optarg[128]; >> + char *mod_optarg_p; >> + >> + if ((mod_optarg_p = strchr(optarg, ','))) >> + strcpy(mod_optarg, ++mod_optarg_p); >> + else >> + strcpy(mod_optarg, optarg); >> + >> + if (get_param_value(mod_optarg, 128, "boot", mod_optarg) != 0) { >> + if (!strcmp("on", mod_optarg)) { >> + char buf[8]="n"; >> + pstrcpy(boot_devices, sizeof(boot_devices), buf); >> + default_net = 0; >> + } >> + } >> + } >> break; >> case QEMU_OPTION_smp: >> smp_parse(optarg); >> -- >> 1.7.2.2 >> >> >