Patchwork [05/20] eepro100: Add all supported devices to pci.c

login
register
mail settings
Submitter Stefan Weil
Date Feb. 14, 2010, 4:16 p.m.
Message ID <1266164189-21062-5-git-send-email-weil@mail.berlios.de>
Download mbox | patch
Permalink /patch/45329/
State New
Headers show

Comments

Stefan Weil - Feb. 14, 2010, 4:16 p.m.
All eepro100 devices work with drivers which
only use basic features.

They were tested with gpxe boot.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/pci.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)
Michael S. Tsirkin - Feb. 21, 2010, 5 p.m.
On Sun, Feb 14, 2010 at 05:16:14PM +0100, Stefan Weil wrote:
> All eepro100 devices work with drivers which
> only use basic features.
> 
> They were tested with gpxe boot.
> 
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  hw/pci.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index eb2043e..1ba3f92 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1436,9 +1436,18 @@ void do_pci_info(Monitor *mon, QObject **ret_data)
>  
>  static const char * const pci_nic_models[] = {
>      "ne2k_pci",
> +    "i82550",
>      "i82551",
> +    "i82557a",
>      "i82557b",
> +    "i82557c",
> +    "i82558a",
> +    "i82558b",
> +    "i82559a",
> +    "i82559b",
> +    "i82559c",
>      "i82559er",
> +    "i82562",
>      "rtl8139",
>      "e1000",
>      "pcnet",
> @@ -1448,9 +1457,18 @@ static const char * const pci_nic_models[] = {
>  
>  static const char * const pci_nic_names[] = {
>      "ne2k_pci",
> +    "i82550",
>      "i82551",
> +    "i82557a",
>      "i82557b",
> +    "i82557c",
> +    "i82558a",
> +    "i82558b",
> +    "i82559a",
> +    "i82559b",
> +    "i82559c",
>      "i82559er",
> +    "i82562",
>      "rtl8139",
>      "e1000",
>      "pcnet",

One wonders: would it be cleaner to have a single eepro100 device
with specific model as qdev option?
Stefan Weil - Feb. 21, 2010, 9:08 p.m.
Michael S. Tsirkin schrieb:
> On Sun, Feb 14, 2010 at 05:16:14PM +0100, Stefan Weil wrote:
>   
>> All eepro100 devices work with drivers which
>> only use basic features.
>>
>> They were tested with gpxe boot.
>>
>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>> ---
>>  hw/pci.c |   18 ++++++++++++++++++
>>  1 files changed, 18 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/pci.c b/hw/pci.c
>> index eb2043e..1ba3f92 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -1436,9 +1436,18 @@ void do_pci_info(Monitor *mon, QObject **ret_data)
>>  
>>  static const char * const pci_nic_models[] = {
>>      "ne2k_pci",
>> +    "i82550",
>>      "i82551",
>> +    "i82557a",
>>      "i82557b",
>> +    "i82557c",
>> +    "i82558a",
>> +    "i82558b",
>> +    "i82559a",
>> +    "i82559b",
>> +    "i82559c",
>>      "i82559er",
>> +    "i82562",
>>      "rtl8139",
>>      "e1000",
>>      "pcnet",
>> @@ -1448,9 +1457,18 @@ static const char * const pci_nic_models[] = {
>>  
>>  static const char * const pci_nic_names[] = {
>>      "ne2k_pci",
>> +    "i82550",
>>      "i82551",
>> +    "i82557a",
>>      "i82557b",
>> +    "i82557c",
>> +    "i82558a",
>> +    "i82558b",
>> +    "i82559a",
>> +    "i82559b",
>> +    "i82559c",
>>      "i82559er",
>> +    "i82562",
>>      "rtl8139",
>>      "e1000",
>>      "pcnet",
>>     
>
> One wonders: would it be cleaner to have a single eepro100 device
> with specific model as qdev option?

Technically that would be possible, too.
You could even have a single pci ethernet device and
specify vendor and device id as qdev options.

I prefer the "official" device names which are also
used in the Intel documentation. eepro100 or e100
are marketing names (more of them exist).

Please note that this patch was marked as optional.
The arrays pci_nic_names and pci_nic_models are
not really needed, and as soon as the table of available
nics is automatically derived from the device information,
all variants of i825xx are supported automatically.

Regards
Stefan Weil
Michael S. Tsirkin - March 3, 2010, 11:51 a.m.
On Sun, Feb 21, 2010 at 10:08:49PM +0100, Stefan Weil wrote:
> Michael S. Tsirkin schrieb:
> > On Sun, Feb 14, 2010 at 05:16:14PM +0100, Stefan Weil wrote:
> >   
> >> All eepro100 devices work with drivers which
> >> only use basic features.
> >>
> >> They were tested with gpxe boot.
> >>
> >> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> >> ---
> >>  hw/pci.c |   18 ++++++++++++++++++
> >>  1 files changed, 18 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/hw/pci.c b/hw/pci.c
> >> index eb2043e..1ba3f92 100644
> >> --- a/hw/pci.c
> >> +++ b/hw/pci.c
> >> @@ -1436,9 +1436,18 @@ void do_pci_info(Monitor *mon, QObject **ret_data)
> >>  
> >>  static const char * const pci_nic_models[] = {
> >>      "ne2k_pci",
> >> +    "i82550",
> >>      "i82551",
> >> +    "i82557a",
> >>      "i82557b",
> >> +    "i82557c",
> >> +    "i82558a",
> >> +    "i82558b",
> >> +    "i82559a",
> >> +    "i82559b",
> >> +    "i82559c",
> >>      "i82559er",
> >> +    "i82562",
> >>      "rtl8139",
> >>      "e1000",
> >>      "pcnet",
> >> @@ -1448,9 +1457,18 @@ static const char * const pci_nic_models[] = {
> >>  
> >>  static const char * const pci_nic_names[] = {
> >>      "ne2k_pci",
> >> +    "i82550",
> >>      "i82551",
> >> +    "i82557a",
> >>      "i82557b",
> >> +    "i82557c",
> >> +    "i82558a",
> >> +    "i82558b",
> >> +    "i82559a",
> >> +    "i82559b",
> >> +    "i82559c",
> >>      "i82559er",
> >> +    "i82562",
> >>      "rtl8139",
> >>      "e1000",
> >>      "pcnet",
> >>     
> >
> > One wonders: would it be cleaner to have a single eepro100 device
> > with specific model as qdev option?
> 
> Technically that would be possible, too.
> You could even have a single pci ethernet device and
> specify vendor and device id as qdev options.
> 
> I prefer the "official" device names which are also
> used in the Intel documentation. eepro100 or e100
> are marketing names (more of them exist).
> 
> Please note that this patch was marked as optional.
> The arrays pci_nic_names and pci_nic_models are
> not really needed, and as soon as the table of available
> nics is automatically derived from the device information,
> all variants of i825xx are supported automatically.
> 
> Regards
> Stefan Weil


Gerd, any input on this patch?
Gerd Hoffmann - March 3, 2010, 1:31 p.m.
On 03/03/10 12:51, Michael S. Tsirkin wrote:
> On Sun, Feb 21, 2010 at 10:08:49PM +0100, Stefan Weil wrote:
>>>>   static const char * const pci_nic_models[] = {
>>>>       "ne2k_pci",
>>>> +    "i82550",
>>>>       "i82551",
>>>> +    "i82557a",
>>>>       "i82557b",
>>>> +    "i82557c",
>>>> +    "i82558a",
>>>> +    "i82558b",
>>>> +    "i82559a",
>>>> +    "i82559b",
>>>> +    "i82559c",
>>>>       "i82559er",
>>>> +    "i82562",
>>>>       "rtl8139",
>>>>       "e1000",
>>>>       "pcnet",

>>> One wonders: would it be cleaner to have a single eepro100 device
>>> with specific model as qdev option?

No.  I think it would be cleaner to use qdev even more.  For that 
specific driver it would make sense to create a private Info struct, 
i.e. something like this:

E100PCIDeviceInfo {
    PCIDeviceInfo pci;
    [ model specific stuff such as has_extended_tcb_support here ]
};

Then go

static E100PCIDeviceInfo eepro100_info[] = {
     {
         .pci.qdev.name = "i82550",
         [ usual qdev stuff here ]
         has_extended_tcb_support = 0 | 1;
         [ more e100 device description bits here ]
     },
     [ ... ]
};

Then you can simply zap all the one-liner wrapper functions around 
nic_init().  nic_init() can get a pointer using something like this:

E100PCIDeviceInfo *einfo = DO_UPCAST(E100PCIDeviceInfo,
          pci.qdev, pci_dev->qdev.info);

and setup the device accordingly.

>> I prefer the "official" device names which are also
>> used in the Intel documentation. eepro100 or e100
>> are marketing names (more of them exist).

qdev has aliases.  You can add .qdev.alias = "eepro100" to one of the 
variants, then the eepro100 name will work as well.

Adding the marketing name to the .desc test is probably a good idea too 
because alot of people know the marketing name only.

>> Please note that this patch was marked as optional.
>> The arrays pci_nic_names and pci_nic_models are
>> not really needed, and as soon as the table of available
>> nics is automatically derived from the device information,
>> all variants of i825xx are supported automatically.

> Gerd, any input on this patch?

Looks fine to me.  When we switch over to walking the list of registered 
devices instead of having a hard-coded list of pci nic drivers all the 
eepro100 variants will show up in the list anyway.

cheers,
   Gerd
Michael S. Tsirkin - March 3, 2010, 1:33 p.m.
On Wed, Mar 03, 2010 at 02:31:53PM +0100, Gerd Hoffmann wrote:
> On 03/03/10 12:51, Michael S. Tsirkin wrote:
>> On Sun, Feb 21, 2010 at 10:08:49PM +0100, Stefan Weil wrote:
>>>>>   static const char * const pci_nic_models[] = {
>>>>>       "ne2k_pci",
>>>>> +    "i82550",
>>>>>       "i82551",
>>>>> +    "i82557a",
>>>>>       "i82557b",
>>>>> +    "i82557c",
>>>>> +    "i82558a",
>>>>> +    "i82558b",
>>>>> +    "i82559a",
>>>>> +    "i82559b",
>>>>> +    "i82559c",
>>>>>       "i82559er",
>>>>> +    "i82562",
>>>>>       "rtl8139",
>>>>>       "e1000",
>>>>>       "pcnet",
>
>>>> One wonders: would it be cleaner to have a single eepro100 device
>>>> with specific model as qdev option?
>
> No.  I think it would be cleaner to use qdev even more.  For that  
> specific driver it would make sense to create a private Info struct,  
> i.e. something like this:
>
> E100PCIDeviceInfo {
>    PCIDeviceInfo pci;
>    [ model specific stuff such as has_extended_tcb_support here ]
> };
>
> Then go
>
> static E100PCIDeviceInfo eepro100_info[] = {
>     {
>         .pci.qdev.name = "i82550",
>         [ usual qdev stuff here ]
>         has_extended_tcb_support = 0 | 1;
>         [ more e100 device description bits here ]
>     },
>     [ ... ]
> };
>
> Then you can simply zap all the one-liner wrapper functions around  
> nic_init().  nic_init() can get a pointer using something like this:
>
> E100PCIDeviceInfo *einfo = DO_UPCAST(E100PCIDeviceInfo,
>          pci.qdev, pci_dev->qdev.info);
>
> and setup the device accordingly.
>
>>> I prefer the "official" device names which are also
>>> used in the Intel documentation. eepro100 or e100
>>> are marketing names (more of them exist).
>
> qdev has aliases.  You can add .qdev.alias = "eepro100" to one of the  
> variants, then the eepro100 name will work as well.
>
> Adding the marketing name to the .desc test is probably a good idea too  
> because alot of people know the marketing name only.

That's my thinking as well.

>>> Please note that this patch was marked as optional.
>>> The arrays pci_nic_names and pci_nic_models are
>>> not really needed, and as soon as the table of available
>>> nics is automatically derived from the device information,
>>> all variants of i825xx are supported automatically.
>
>> Gerd, any input on this patch?
>
> Looks fine to me.  When we switch over to walking the list of registered  
> devices instead of having a hard-coded list of pci nic drivers all the  
> eepro100 variants will show up in the list anyway.
>
> cheers,
>   Gerd
Stefan Weil - March 3, 2010, 6:53 p.m.
Michael S. Tsirkin schrieb:
> On Wed, Mar 03, 2010 at 02:31:53PM +0100, Gerd Hoffmann wrote:
>> On 03/03/10 12:51, Michael S. Tsirkin wrote:
>>> On Sun, Feb 21, 2010 at 10:08:49PM +0100, Stefan Weil wrote:
>>>>>> static const char * const pci_nic_models[] = {
>>>>>> "ne2k_pci",
>>>>>> + "i82550",
>>>>>> "i82551",
>>>>>> + "i82557a",
>>>>>> "i82557b",
>>>>>> + "i82557c",
>>>>>> + "i82558a",
>>>>>> + "i82558b",
>>>>>> + "i82559a",
>>>>>> + "i82559b",
>>>>>> + "i82559c",
>>>>>> "i82559er",
>>>>>> + "i82562",
>>>>>> "rtl8139",
>>>>>> "e1000",
>>>>>> "pcnet",
>>>>> One wonders: would it be cleaner to have a single eepro100 device
>>>>> with specific model as qdev option?
>> No. I think it would be cleaner to use qdev even more. For that
>> specific driver it would make sense to create a private Info struct,
>> i.e. something like this:
>>
>> E100PCIDeviceInfo {
>> PCIDeviceInfo pci;
>> [ model specific stuff such as has_extended_tcb_support here ]
>> };
>>
>> Then go
>>
>> static E100PCIDeviceInfo eepro100_info[] = {
>> {
>> .pci.qdev.name = "i82550",
>> [ usual qdev stuff here ]
>> has_extended_tcb_support = 0 | 1;
>> [ more e100 device description bits here ]
>> },
>> [ ... ]
>> };
>>
>> Then you can simply zap all the one-liner wrapper functions around
>> nic_init(). nic_init() can get a pointer using something like this:
>>
>> E100PCIDeviceInfo *einfo = DO_UPCAST(E100PCIDeviceInfo,
>> pci.qdev, pci_dev->qdev.info);
>>
>> and setup the device accordingly.

This would indeed simplify the code.

Although I don't like tricky programming like DO_UPCAST
(because I saw too much code where tricky programming
was faulty programming), I'll consider to modify the code
as Gerd proposed.

>>>> I prefer the "official" device names which are also
>>>> used in the Intel documentation. eepro100 or e100
>>>> are marketing names (more of them exist).
>> qdev has aliases. You can add .qdev.alias = "eepro100" to one of the
>> variants, then the eepro100 name will work as well.

Good idea. To keep the number of aliases small, I plan to
add only one or two aliases: e100 (eepro100 only if a second
alias is possible).

Why? e100 is the current linux driver name.
eepro100 was the former linux driver name.

>>
>> Adding the marketing name to the .desc test is probably a good idea too
>> because alot of people know the marketing name only.
>
> That's my thinking as well.

Neither eepro100 nor e100 are marketing names
(I was wrong when I wrote that in an earlier mail).

The marketing name was Intel EtherExpress Pro 100.
I agree that it would help people to see this name,
so I suggest adding it to qemu-doc.texi.

As far as I know, only insiders (and the old linux driver)
used the abbreviations eepro100 and e100.

Look at your favorite search machine:
nobody sells / sold eepro100 cards.

If you want a less technical name than i82559c, e100 is the
better choice - like e1000 for the successor.

>
>>>> Please note that this patch was marked as optional.
>>>> The arrays pci_nic_names and pci_nic_models are
>>>> not really needed, and as soon as the table of available
>>>> nics is automatically derived from the device information,
>>>> all variants of i825xx are supported automatically.
>>> Gerd, any input on this patch?
>> Looks fine to me. When we switch over to walking the list of registered
>> devices instead of having a hard-coded list of pci nic drivers all the
>> eepro100 variants will show up in the list anyway.
>>
>> cheers,
>> Gerd
Gerd Hoffmann - March 4, 2010, 8:56 a.m.
On 03/03/10 19:53, Stefan Weil wrote:
> This would indeed simplify the code.
>
> Although I don't like tricky programming like DO_UPCAST
> (because I saw too much code where tricky programming
> was faulty programming),

Yea, when coding C++ in C you need little tricks now and then ;)

You need that in one place only though, nic_init() can store a pointer 
in EEPRO100State and everybody can take the direct route then ...

>>> qdev has aliases. You can add .qdev.alias = "eepro100" to one of the
>>> variants, then the eepro100 name will work as well.
>
> Good idea. To keep the number of aliases small, I plan to
> add only one or two aliases: e100 (eepro100 only if a second
> alias is possible).

You can have one alias only.

> Why? e100 is the current linux driver name.

Reasonable choice, lines up with e1000.

> eepro100 was the former linux driver name.

And leaked into a bunch of places (like qemu source code) because of 
that.  Probably only developers recognize the name though.

> The marketing name was Intel EtherExpress Pro 100.
> I agree that it would help people to see this name,
> so I suggest adding it to qemu-doc.texi.

Maybe .desc too ?

cheers,
   Gerd

Patch

diff --git a/hw/pci.c b/hw/pci.c
index eb2043e..1ba3f92 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1436,9 +1436,18 @@  void do_pci_info(Monitor *mon, QObject **ret_data)
 
 static const char * const pci_nic_models[] = {
     "ne2k_pci",
+    "i82550",
     "i82551",
+    "i82557a",
     "i82557b",
+    "i82557c",
+    "i82558a",
+    "i82558b",
+    "i82559a",
+    "i82559b",
+    "i82559c",
     "i82559er",
+    "i82562",
     "rtl8139",
     "e1000",
     "pcnet",
@@ -1448,9 +1457,18 @@  static const char * const pci_nic_models[] = {
 
 static const char * const pci_nic_names[] = {
     "ne2k_pci",
+    "i82550",
     "i82551",
+    "i82557a",
     "i82557b",
+    "i82557c",
+    "i82558a",
+    "i82558b",
+    "i82559a",
+    "i82559b",
+    "i82559c",
     "i82559er",
+    "i82562",
     "rtl8139",
     "e1000",
     "pcnet",