On Tue, Sep 21, 2010 at 10:16:29AM -0500, Anthony Liguori wrote:
> 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<thomas.ost...@nsn.com>
> >>Signed-off-by: Bernhard Kohl<bernhard.k...@nsn.com>
> >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 ...
Cool, but needs to be documented.
> 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[,vl...@var{n}][,macad...@var{mac}][,mod...@var{type}]
> >>[,na...@var{name}][,ad...@var{addr}][,vecto...@var{v}]
> >>+...@item -net nic[,vl...@var{n}][,macad...@var{mac}][,mod...@var{type}]
> >>[,na...@var{name}][,ad...@var{addr}][,vecto...@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
> >>
