| Message ID | 1266164189-21062-5-git-send-email-weil@mail.berlios.de |
|---|---|
| State | New |
| Headers | show |
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?
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
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?
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
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
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
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
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",
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(-)