Re: [Qemu-devel] [PATCH 3/3] q35: change default NIC to e1000e

[Thomas Huth]

On 06.03.2018 20:46, Paolo Bonzini wrote:
> The e1000 NIC is getting old and is not a very good default for a
> PCIe machine type.  Change it to e1000e, which should be supported
> by a good number of guests.
> 
> In particular, drivers for 82574 were added first to Linux 2.6.27 (2008)
> and Windows 2008 R2.  This does mean that Windows 2008 will not work
> anymore with Q35 machine types and a default "-net nic -net xxx" network
> configuration; it did work before because it does have an AHCI driver.
> However, Windows 2008 has been declared out of main stream support
> in 2015.  It will get out of extended support in 2020.  Windows 2008
> R2 has the same end of support dates and, since the two are basically
> Vista vs. Windows 7, R2 probably is more popular.
> 
> Cc: Jason Wang <address@hidden>
> Cc: Thomas Huth <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/i386/pc.c         | 7 ++++---
>  hw/i386/pc_piix.c    | 6 +++++-
>  hw/i386/pc_q35.c     | 8 +++++++-
>  hw/pci/pci.c         | 2 +-
>  include/hw/i386/pc.h | 3 ++-
>  5 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 35fcb6efdf..d3e1d50b59 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1619,18 +1619,19 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq 
> *gsi,
>      }
>  }
>  
> -void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus)
> +void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
>  {
>      int i;
>  
>      rom_set_order_override(FW_CFG_ORDER_OVERRIDE_NIC);
>      for (i = 0; i < nb_nics; i++) {
>          NICInfo *nd = &nd_table[i];
> +        const char *model = nd->model ? nd->model : pcmc->default_nic_model;
>  
> -        if (!pci_bus || (nd->model && strcmp(nd->model, "ne2k_isa") == 0)) {
> +        if (!strcmp(model, "ne2k_isa")) {

<bikeshedpainting>
I'd prefer g_str_equal() these days ...
</bikeshedpainting>

>              pc_init_ne2k_isa(isa_bus, nd);
>          } else {
> -            pci_nic_init_nofail(nd, pci_bus, "e1000", NULL);
> +            pci_nic_init_nofail(nd, pci_bus, model, NULL);
>          }
>      }
>      rom_reset_order_override();
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 8658bcba63..0f1966d547 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -240,7 +240,7 @@ static void pc_init1(MachineState *machine,
>      pc_basic_device_init(isa_bus, pcms->gsi, &rtc_state, true,
>                           (pcms->vmport != ON_OFF_AUTO_ON), pcms->pit, 0x4);
>  
> -    pc_nic_init(isa_bus, pci_bus);
> +    pc_nic_init(pcmc, isa_bus, pci_bus);
>  
>      ide_drive_get(hd, ARRAY_SIZE(hd));
>      if (pcmc->pci_enabled) {
> @@ -417,6 +417,9 @@ static void pc_xen_hvm_init(MachineState *machine)
>  
>  static void pc_i440fx_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> +    pcmc->default_nic_model = "e1000";
> +
>      m->family = "pc_piix";
>      m->desc = "Standard PC (i440FX + PIIX, 1996)";
>      m->default_machine_opts = "firmware=bios-256k.bin";
> @@ -1114,6 +1117,7 @@ static void isapc_machine_options(MachineClass *m)
>      pcmc->gigabyte_align = false;
>      pcmc->smbios_legacy_mode = true;
>      pcmc->has_reserved_memory = false;
> +    pcmc->default_nic_model = "ne2k_isa";
>      m->default_cpu_type = X86_CPU_TYPE_NAME("486");
>  }
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 0c0bc48137..9ae916327e 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -272,7 +272,7 @@ static void pc_q35_init(MachineState *machine)
>  
>      /* the rest devices to which pci devfn is automatically assigned */
>      pc_vga_init(isa_bus, host_bus);
> -    pc_nic_init(isa_bus, host_bus);
> +    pc_nic_init(pcmc, isa_bus, host_bus);
>  
>      if (pcms->acpi_nvdimm_state.is_enabled) {
>          nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
> @@ -294,6 +294,9 @@ static void pc_q35_init(MachineState *machine)
>  
>  static void pc_q35_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> +    pcmc->default_nic_model = "e1000e";
> +
>      m->family = "pc_q35";
>      m->desc = "Standard PC (Q35 + ICH9, 2009)";
>      m->units_per_default_bus = 1;
> @@ -316,7 +319,10 @@ DEFINE_Q35_MACHINE(v2_12, "pc-q35-2.12", NULL,
>  
>  static void pc_q35_2_11_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> +
>      pc_q35_2_12_machine_options(m);
> +    pcmc->default_nic_model = "e1000";
>      m->alias = NULL;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_11);
>  }
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index aa24a26680..3b0df51983 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1829,7 +1829,7 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus 
> *rootbus,
>      int devfn;
>      int i;
>  
> -    if (!strcmp(nd->model, "virtio")) {
> +    if (nd->model && !strcmp(nd->model, "virtio")) {

Please squash that into the previous patch instead.

>          g_free(nd->model);
>          nd->model = g_strdup("virtio-net-pci");
>      }
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index bb49165fe0..e81654eb7f 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -114,6 +114,7 @@ struct PCMachineClass {
>      /* Device configuration: */
>      bool pci_enabled;
>      bool kvmclock_enabled;
> +    const char *default_nic_model;
>  
>      /* Compat options: */
>  
> @@ -248,7 +249,7 @@ void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd);
>  void pc_cmos_init(PCMachineState *pcms,
>                    BusState *ide0, BusState *ide1,
>                    ISADevice *s);
> -void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus);
> +void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus);
>  void pc_pci_device_init(PCIBus *pci_bus);
>  
>  typedef void (*cpu_set_smm_t)(int smm, void *arg);
> 

Apart from the nits, this looks fine to me.

 Thomas