[Qemu-devel] Re: [PATCH] Make NIC model fallback to default when specified model is not supported

[Michael S. Tsirkin]

On Mon, Sep 20, 2010 at 11:47:59AM +0200, Michal Novotny wrote:
> Hi,
> 
> this is the patch to introduce a NIC model fallback to default when model
> specified is not supported. It's been tested on i386-softmmu target on
> i386 host using the Windows XP x86 virtual machine and by trying to setup
> the invalid (unsupported) model of NIC device. Also, the new constant in
> the net.h called the DEFAULT_NIC_MODEL has been introduced to be able to
> change the default NIC model easily. This variable is being used to set
> the default NIC model when necessary.

Why is this a good idea? This will create problems for anyone
doing migration, etc.

> Also, some bits per mips_jazz were added but usage of some constant for
> MIPS is not necessary since there is only one NIC model supported there.
> 
> Michal
> 
> Signed-off-by: Michal Novotny<address@hidden>

I think adding NIC_DEFAULT_MODEL macro in net.h is problematic exactly
because each platform has its own.
It belongs in per-platform .c file I think.

> -- 
> Michal Novotny<address@hidden>, RHCE
> Virtualization Team (xen userspace), Red Hat
> 
> 
> 

> >From bccd19d357045c20db332d185e93f8cf54caa340 Mon Sep 17 00:00:00 2001
> From: Michal Novotny <address@hidden>
> Date: Mon, 20 Sep 2010 11:37:54 +0200
> Subject: [PATCH] Make NIC model fallback to default when specified model is 
> not supported
> 
> Hi,
> this is the patch to introduce a NIC model fallback to default when model
> specified is not supported. It's been tested on i386-softmmu target on
> i386 host using the Windows XP x86 virtual machine and by trying to setup
> the invalid (unsupported) model of NIC device. Also, the new constant in
> the net.h called the DEFAULT_NIC_MODEL has been introduced to be able to
> change the default NIC model easily. This variable is being used to set
> the default NIC model when necessary.
> 
> Also, some bits per mips_jazz were added but usage of some constant for
> MIPS is not necessary since there is only one NIC model supported there.
> 
> Michal
> 
> Signed-off-by: Michal Novotny <address@hidden>
> ---
>  hw/mips_jazz.c     |    4 ++--
>  hw/pc_piix.c       |    2 +-
>  hw/pci.c           |    8 ++++++--
>  hw/ppc440_bamboo.c |    2 +-
>  net.c              |   12 +++++++++---
>  net.h              |    3 +++
>  6 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
> index 66397c0..0b66e65 100644
> --- a/hw/mips_jazz.c
> +++ b/hw/mips_jazz.c
> @@ -237,8 +237,8 @@ void mips_jazz_init (ram_addr_t ram_size,
>              fprintf(stderr, "qemu: Supported NICs: dp83932\n");
>              exit(1);
>          } else {
> -            fprintf(stderr, "qemu: Unsupported NIC: %s\n", nd->model);
> -            exit(1);
> +            fprintf(stderr, "qemu: Unsupported NIC: %s, using default: 
> dp83932\n", nd->model);
> +            nd->model = qemu_strdup("dp83932");
>          }
>      }
>  
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 12359a7..0f95910 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -122,7 +122,7 @@ static void pc_init1(ram_addr_t ram_size,
>          if (!pci_enabled || (nd->model && strcmp(nd->model, "ne2k_isa") == 
> 0))
>              pc_init_ne2k_isa(nd);
>          else
> -            pci_nic_init_nofail(nd, "e1000", NULL);
> +            pci_nic_init_nofail(nd, NIC_DEFAULT_MODEL, NULL);
>      }
>  
>      if (drive_get_max_bus(IF_IDE) >= MAX_IDE_BUS) {
> diff --git a/hw/pci.c b/hw/pci.c
> index 6d0934d..a2afa3a 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1502,8 +1502,12 @@ PCIDevice *pci_nic_init(NICInfo *nd, const char 
> *default_model,
>      int i;
>  
>      i = qemu_find_nic_model(nd, pci_nic_models, default_model);
> -    if (i < 0)
> -        return NULL;
> +    if (i < 0) {
> +        nd->model = qemu_strdup(NIC_DEFAULT_MODEL);
> +        i = qemu_find_nic_model(nd, pci_nic_models, NIC_DEFAULT_MODEL);
> +        if (i < 0)
> +            return NULL;
> +    }
>  
>      bus = pci_get_bus_devfn(&devfn, devaddr);
>      if (!bus) {
> diff --git a/hw/ppc440_bamboo.c b/hw/ppc440_bamboo.c
> index 34ddf45..0d41428 100644
> --- a/hw/ppc440_bamboo.c
> +++ b/hw/ppc440_bamboo.c
> @@ -115,7 +115,7 @@ static void bamboo_init(ram_addr_t ram_size,
>          for (i = 0; i < nb_nics; i++) {
>              /* There are no PCI NICs on the Bamboo board, but there are
>               * PCI slots, so we can pick whatever default model we want. */
> -            pci_nic_init_nofail(&nd_table[i], "e1000", NULL);
> +            pci_nic_init_nofail(&nd_table[i], NIC_DEFAULT_MODEL, NULL);
>          }
>      }
>  
> diff --git a/net.c b/net.c
> index 3d0fde7..ffd471e 100644
> --- a/net.c
> +++ b/net.c
> @@ -716,8 +716,14 @@ void qemu_check_nic_model(NICInfo *nd, const char *model)
>  
>      if (qemu_show_nic_models(nd->model, models))
>          exit(0);
> -    if (qemu_find_nic_model(nd, models, model) < 0)
> -        exit(1);
> +    if (qemu_find_nic_model(nd, models, model) < 0) {
> +        /* Fallback to default model if unsupported */
> +        models[0] = NIC_DEFAULT_MODEL;
> +        nd->model = qemu_strdup(NIC_DEFAULT_MODEL);
> +
> +        if (qemu_find_nic_model(nd, models, model) < 0)
> +            exit(1);
> +    }
>  }
>  
>  int qemu_find_nic_model(NICInfo *nd, const char * const *models,
> @@ -733,7 +739,7 @@ int qemu_find_nic_model(NICInfo *nd, const char * const 
> *models,
>              return i;
>      }
>  
> -    error_report("qemu: Unsupported NIC model: %s", nd->model);
> +    error_report("qemu: Unsupported NIC model: %s, using default: %s", 
> nd->model, NIC_DEFAULT_MODEL);
>      return -1;
>  }
>  
> diff --git a/net.h b/net.h
> index 518cf9c..fc8e04c 100644
> --- a/net.h
> +++ b/net.h
> @@ -24,6 +24,9 @@ typedef struct NICConf {
>      DEFINE_PROP_VLAN("vlan",     _state, _conf.vlan),                   \
>      DEFINE_PROP_NETDEV("netdev", _state, _conf.peer)
>  
> +/* Default NIC model to be used - also used as a fallback model when model 
> specified doesn't exist */
> +#define NIC_DEFAULT_MODEL "e1000"
> +
>  /* VLANs support */
>  
>  typedef enum {
> -- 
> 1.7.2.3
> 
>