Re: [Qemu-devel] Re: [PATCH] new parameter boot=on|off for "-net nic" and "-device" NIC devices

[Anthony Liguori <anthony@codemonkey.ws>]

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.ostler@nsn.com>
>> Signed-off-by: Bernhard Kohl<bernhard.kohl@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 ...

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
>>
>>      
>