diff mbox

Make NIC model fallback to default when specified model is not supported

Message ID 4C972DCF.1050003@redhat.com
State New
Headers show

Commit Message

Michal Novotny Sept. 20, 2010, 9:47 a.m. UTC
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<minovotn@redhat.com>

Comments

Paolo Bonzini Sept. 20, 2010, 10:34 a.m. UTC | #1
On 09/20/2010 11:47 AM, 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?  If it's not supported, it shouldn't run.

Paolo
Michal Novotny Sept. 20, 2010, 10:48 a.m. UTC | #2
On 09/20/2010 12:34 PM, Paolo Bonzini wrote:
> On 09/20/2010 11:47 AM, 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?  If it's not supported, it shouldn't run.
>
> Paolo

I don't think so. It makes sense it shouldn't run for case of pure qemu 
but since there's newly added support for xen (and also there's support 
for other virtualization platforms to be used with the qemu device 
model) it should fallback with just a warning since otherwise those 
platforms, like e.g. mentioned Xen, will leave defunct device models 
there and the guests won't run be running at all ending up with no 
state. If there's a warning with information it's falling back to 
default the user can notice if he wants to but it won't leave the 
defunct device models anymore which can be pretty hard to determine 
what's going on there for standard user that doesn't have much 
experience with e.g. Xen yet.

Michal
Daniel P. Berrangé Sept. 20, 2010, 10:53 a.m. UTC | #3
On Mon, Sep 20, 2010 at 12:48:50PM +0200, Michal Novotny wrote:
> On 09/20/2010 12:34 PM, Paolo Bonzini wrote:
> >On 09/20/2010 11:47 AM, 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?  If it's not supported, it shouldn't run.
> >
> >Paolo
> 
> I don't think so. It makes sense it shouldn't run for case of pure qemu 
> but since there's newly added support for xen (and also there's support 
> for other virtualization platforms to be used with the qemu device 
> model) it should fallback with just a warning since otherwise those 
> platforms, like e.g. mentioned Xen, will leave defunct device models 
> there and the guests won't run be running at all ending up with no 
> state. If there's a warning with information it's falling back to 
> default the user can notice if he wants to but it won't leave the 
> defunct device models anymore which can be pretty hard to determine 
> what's going on there for standard user that doesn't have much 
> experience with e.g. Xen yet.

IMHO this is just a bug in the xen mgmt layer. If the QEMU device model
dies/quits, then XenD should teardown the guest, since you can't do any
useful work once the device model has crashed. Silently switching to a
different NIC model than the one requested is definitely a wrong approach.

Daniel
Michal Novotny Sept. 20, 2010, 11:05 a.m. UTC | #4
On 09/20/2010 12:53 PM, Daniel P. Berrange wrote:
> On Mon, Sep 20, 2010 at 12:48:50PM +0200, Michal Novotny wrote:
>    
>> On 09/20/2010 12:34 PM, Paolo Bonzini wrote:
>>      
>>> On 09/20/2010 11:47 AM, 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?  If it's not supported, it shouldn't run.
>>>
>>> Paolo
>>>        
>> I don't think so. It makes sense it shouldn't run for case of pure qemu
>> but since there's newly added support for xen (and also there's support
>> for other virtualization platforms to be used with the qemu device
>> model) it should fallback with just a warning since otherwise those
>> platforms, like e.g. mentioned Xen, will leave defunct device models
>> there and the guests won't run be running at all ending up with no
>> state. If there's a warning with information it's falling back to
>> default the user can notice if he wants to but it won't leave the
>> defunct device models anymore which can be pretty hard to determine
>> what's going on there for standard user that doesn't have much
>> experience with e.g. Xen yet.
>>      
> IMHO this is just a bug in the xen mgmt layer. If the QEMU device model
> dies/quits, then XenD should teardown the guest, since you can't do any
> useful work once the device model has crashed. Silently switching to a
> different NIC model than the one requested is definitely a wrong approach.
>
> Daniel
>    
When the qemu-dm has crashed we can't do anything with the guest, that's 
correct. Nevertheless do you think that we should bail with error and 
just fix the layer of xen management to check whether there's a device 
model still running or not? It's being spawned by XenD itself so we 
would need to check whether this process is not a zombie using the 
/proc/$PID/stat or use some better way to get the state. Unfortunately 
using /proc/$PID/stat would kill the portability of the code.

The other way is to implement the thread that will be (periodically or 
"on change") checking the device model state and that will be 
terminating the domain when device model dies/quits. I'm not saying this 
is the bad approach but we've been talking with Mirek about at least 
RHEL-5 version and he told me that he recommends to implement a fallback 
to the default NIC.

Daniel, if you consider RHEL-5 version, what do you prefer to do with 
this one? Fix it somehow in the XenD or is altering the device model OK 
for this version? Also, the patch has been already sent upstream Xen for 
consideration about an hour ago.

Michal
Daniel P. Berrangé Sept. 20, 2010, 11:07 a.m. UTC | #5
On Mon, Sep 20, 2010 at 01:05:33PM +0200, Michal Novotny wrote:
> On 09/20/2010 12:53 PM, Daniel P. Berrange wrote:
> >On Mon, Sep 20, 2010 at 12:48:50PM +0200, Michal Novotny wrote:
> >   
> >>On 09/20/2010 12:34 PM, Paolo Bonzini wrote:
> >>     
> >>>On 09/20/2010 11:47 AM, 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?  If it's not supported, it shouldn't run.
> >>>
> >>>Paolo
> >>>       
> >>I don't think so. It makes sense it shouldn't run for case of pure qemu
> >>but since there's newly added support for xen (and also there's support
> >>for other virtualization platforms to be used with the qemu device
> >>model) it should fallback with just a warning since otherwise those
> >>platforms, like e.g. mentioned Xen, will leave defunct device models
> >>there and the guests won't run be running at all ending up with no
> >>state. If there's a warning with information it's falling back to
> >>default the user can notice if he wants to but it won't leave the
> >>defunct device models anymore which can be pretty hard to determine
> >>what's going on there for standard user that doesn't have much
> >>experience with e.g. Xen yet.
> >>     
> >IMHO this is just a bug in the xen mgmt layer. If the QEMU device model
> >dies/quits, then XenD should teardown the guest, since you can't do any
> >useful work once the device model has crashed. Silently switching to a
> >different NIC model than the one requested is definitely a wrong approach.
> >
> >Daniel
> >   
> When the qemu-dm has crashed we can't do anything with the guest, that's 
> correct. Nevertheless do you think that we should bail with error and 
> just fix the layer of xen management to check whether there's a device 
> model still running or not? It's being spawned by XenD itself so we 
> would need to check whether this process is not a zombie using the 
> /proc/$PID/stat or use some better way to get the state. Unfortunately 
> using /proc/$PID/stat would kill the portability of the code.
> 
> The other way is to implement the thread that will be (periodically or 
> "on change") checking the device model state and that will be 
> terminating the domain when device model dies/quits. I'm not saying this 
> is the bad approach but we've been talking with Mirek about at least 
> RHEL-5 version and he told me that he recommends to implement a fallback 
> to the default NIC.

If XenD holds open a connection to the QEMU monitor socket, then it
can easily receive a POLLHUP when QEMU dies. This is the approach 
most mgmt tools use for detecting QEMU death.

> Daniel, if you consider RHEL-5 version, what do you prefer to do with 
> this one? Fix it somehow in the XenD or is altering the device model OK 
> for this version? Also, the patch has been already sent upstream Xen for 
> consideration about an hour ago.

RHEL5 Xen maintenance isn't a concern of qemu-devel really, so this
is not the place to decide that.

Daniel
Michal Novotny Sept. 20, 2010, 11:15 a.m. UTC | #6
On 09/20/2010 01:07 PM, Daniel P. Berrange wrote:
> On Mon, Sep 20, 2010 at 01:05:33PM +0200, Michal Novotny wrote:
>    
>> On 09/20/2010 12:53 PM, Daniel P. Berrange wrote:
>>      
>>> On Mon, Sep 20, 2010 at 12:48:50PM +0200, Michal Novotny wrote:
>>>
>>>        
>>>> On 09/20/2010 12:34 PM, Paolo Bonzini wrote:
>>>>
>>>>          
>>>>> On 09/20/2010 11:47 AM, 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?  If it's not supported, it shouldn't run.
>>>>>
>>>>> Paolo
>>>>>
>>>>>            
>>>> I don't think so. It makes sense it shouldn't run for case of pure qemu
>>>> but since there's newly added support for xen (and also there's support
>>>> for other virtualization platforms to be used with the qemu device
>>>> model) it should fallback with just a warning since otherwise those
>>>> platforms, like e.g. mentioned Xen, will leave defunct device models
>>>> there and the guests won't run be running at all ending up with no
>>>> state. If there's a warning with information it's falling back to
>>>> default the user can notice if he wants to but it won't leave the
>>>> defunct device models anymore which can be pretty hard to determine
>>>> what's going on there for standard user that doesn't have much
>>>> experience with e.g. Xen yet.
>>>>
>>>>          
>>> IMHO this is just a bug in the xen mgmt layer. If the QEMU device model
>>> dies/quits, then XenD should teardown the guest, since you can't do any
>>> useful work once the device model has crashed. Silently switching to a
>>> different NIC model than the one requested is definitely a wrong approach.
>>>
>>> Daniel
>>>
>>>        
>> When the qemu-dm has crashed we can't do anything with the guest, that's
>> correct. Nevertheless do you think that we should bail with error and
>> just fix the layer of xen management to check whether there's a device
>> model still running or not? It's being spawned by XenD itself so we
>> would need to check whether this process is not a zombie using the
>> /proc/$PID/stat or use some better way to get the state. Unfortunately
>> using /proc/$PID/stat would kill the portability of the code.
>>
>> The other way is to implement the thread that will be (periodically or
>> "on change") checking the device model state and that will be
>> terminating the domain when device model dies/quits. I'm not saying this
>> is the bad approach but we've been talking with Mirek about at least
>> RHEL-5 version and he told me that he recommends to implement a fallback
>> to the default NIC.
>>      
> If XenD holds open a connection to the QEMU monitor socket, then it
> can easily receive a POLLHUP when QEMU dies. This is the approach
> most mgmt tools use for detecting QEMU death.
>    

I don't know how this is being implemented in the new qemu code that's 
implementing Xen support but from what I saw the upstream Xen-4.1 is not 
using it yet so implementing this into the new xen management layer 
could be a good idea for somebody working on the xen layer for qemu 
because I'm not familiar with this yet.

>    
>> Daniel, if you consider RHEL-5 version, what do you prefer to do with
>> this one? Fix it somehow in the XenD or is altering the device model OK
>> for this version? Also, the patch has been already sent upstream Xen for
>> consideration about an hour ago.
>>      
> RHEL5 Xen maintenance isn't a concern of qemu-devel really, so this
> is not the place to decide that.
>
> Daniel
>    
Well, this way I guess we should have 2, maybe 3 different approaches - 
different for qemu itself and for upstream version Xen using the older 
version of qemu-dm and RHEL-5 version.

Therefore I think we should drop the patch for qemu (the one sent to 
this list) and decide about the others using their own lists/discussions.

Michal
Anthony Liguori Sept. 20, 2010, 2:35 p.m. UTC | #7
On 09/20/2010 06:15 AM, Michal Novotny wrote:
> Well, this way I guess we should have 2, maybe 3 different approaches 
> - different for qemu itself and for upstream version Xen using the 
> older version of qemu-dm and RHEL-5 version.
>
> Therefore I think we should drop the patch for qemu (the one sent to 
> this list) and decide about the others using their own lists/discussions.

BTW, the simple way to fix this in Xen is to launch qemu-dm -net 
nic,model=?, parse the output to determine if the requested nic is 
valid, then if it's not valid, print your warning to the user and then 
select a "default" nic.

No need to implement this logic in qemu-dm itself.

Regards,

Anthony Liguori

> Michal
>
Michael S. Tsirkin Sept. 20, 2010, 5:58 p.m. UTC | #8
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<minovotn@redhat.com>

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<minovotn@redhat.com>, RHCE
> Virtualization Team (xen userspace), Red Hat
> 
> 
> 

> >From bccd19d357045c20db332d185e93f8cf54caa340 Mon Sep 17 00:00:00 2001
> From: Michal Novotny <minovotn@redhat.com>
> 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 <minovotn@redhat.com>
> ---
>  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
> 
>
Michal Novotny Sept. 21, 2010, 7:10 a.m. UTC | #9
On 09/20/2010 07:58 PM, Michael S. Tsirkin wrote:
> 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.
>    

Why do you think it would introduce issues when doing migrations? 
Imagine one version (v1) of qemu supporting NIC model called e.g. 
"model" but the newer version (v2) no longer  supporting the model and 
you migrate a guest from v1 to v2 that's using "model" NIC. What does it 
do when you do this now? Will it fail and will the guest be working fine 
on v1 but not migrated to v2? What would my patch do concerning the 
migrations? Would it pass with changing NIC type to default?

>    
>> 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<minovotn@redhat.com>
>>      
> 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.
>    

That's right. Implementing this into the per-platform.c could be a 
better idea nevertheless I saw message of "Unsupported NIC" just in 
mips_jazz.c and net.c so maybe I got confused. The NIC_DEFAULT_MODEL 
definition could help to change default NIC model in the future without 
digging into the code so much so I was thinking it could be useful.

Michal
diff mbox

Patch

From bccd19d357045c20db332d185e93f8cf54caa340 Mon Sep 17 00:00:00 2001
From: Michal Novotny <minovotn@redhat.com>
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 <minovotn@redhat.com>
---
 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