Patchwork [RFC,v2,5/5] ne2k_isa: add property for option rom loading.

login
register
mail settings
Submitter Gerd Hoffmann
Date Oct. 7, 2009, 12:36 p.m.
Message ID <1254918996-26050-6-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/35268/
State RFC
Headers show

Comments

Gerd Hoffmann - Oct. 7, 2009, 12:36 p.m.
Moving option rom loading from machine init to the (nic) drivers.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/ne2000-isa.c |    9 +++++++++
 hw/ne2000.h     |    1 +
 2 files changed, 10 insertions(+), 0 deletions(-)
Anthony Liguori - Oct. 7, 2009, 1:08 p.m.
Gerd Hoffmann wrote:
> Moving option rom loading from machine init to the (nic) drivers.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/ne2000-isa.c |    9 +++++++++
>  hw/ne2000.h     |    1 +
>  2 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/hw/ne2000-isa.c b/hw/ne2000-isa.c
> index 7a24088..1d296a0 100644
> --- a/hw/ne2000-isa.c
> +++ b/hw/ne2000-isa.c
> @@ -27,6 +27,7 @@
>  #include "qdev.h"
>  #include "net.h"
>  #include "ne2000.h"
> +#include "loader.h"
>  
>  typedef struct ISANE2000State {
>      ISADevice dev;
> @@ -78,6 +79,13 @@ static int isa_ne2000_initfn(ISADevice *dev)
>          qemu_error("warning: no vlan specfied, ne2k_isa is unconnected\n");
>      }
>  
> +    if (s->pxe) {
> +        if (rom_add_option("pxe-ne2k_isa.bin") != 0) {
> +            qemu_error("warning: loading rom pxe-ne2k_isa.bin failed\n");
> +            s->pxe = 0;
> +        }
> +    }
> +
>   

Maybe we should make the filename a property instead of adding a pxe option?

Regards,

Anthony Liguori
Gerd Hoffmann - Oct. 7, 2009, 1:21 p.m.
On 10/07/09 15:08, Anthony Liguori wrote:
> Gerd Hoffmann wrote:
>> + if (s->pxe) {
>> + if (rom_add_option("pxe-ne2k_isa.bin") != 0) {
>> + qemu_error("warning: loading rom pxe-ne2k_isa.bin failed\n");
>> + s->pxe = 0;
>> + }
>> + }
>> +
>
> Maybe we should make the filename a property instead of adding a pxe
> option?

No.  The user should not need to know the file name of the option rom 
just to enable pxe booting for the nic.

When making the filename configurable it should be a separate property 
like "rom-name" or simliar.  I would suggest to NOT implement it unless 
users actually ask for it ;)

cheers,
   Gerd
Anthony Liguori - Oct. 7, 2009, 1:28 p.m.
Gerd Hoffmann wrote:
> On 10/07/09 15:08, Anthony Liguori wrote:
>> Gerd Hoffmann wrote:
>>> + if (s->pxe) {
>>> + if (rom_add_option("pxe-ne2k_isa.bin") != 0) {
>>> + qemu_error("warning: loading rom pxe-ne2k_isa.bin failed\n");
>>> + s->pxe = 0;
>>> + }
>>> + }
>>> +
>>
>> Maybe we should make the filename a property instead of adding a pxe
>> option?
>
> No.  The user should not need to know the file name of the option rom 
> just to enable pxe booting for the nic.

Having a pxe flag is somewhat odd.  Real network devices always have 
roms and they always get loaded.  They register themselves as BEV 
devices and the normal boot selection is used to determine whether a 
particular NIC gets network booted or not.

Our roms do expose themselves as BEV roms so there's really no harm in 
loading an option rom while booting from disk.

Any PCI device can have a rom and it probably should be a generic 
property of any PCI device.  There's really nothing specific about 
network adapters.

> When making the filename configurable it should be a separate property 
> like "rom-name" or simliar.  I would suggest to NOT implement it 
> unless users actually ask for it ;)

Quite a few users today replace the standard etherboot roms with gPXE roms.

Regards,

Anthony Liguori
Gerd Hoffmann - Oct. 7, 2009, 2 p.m.
On 10/07/09 15:28, Anthony Liguori wrote:
> Having a pxe flag is somewhat odd. Real network devices always have roms
> and they always get loaded. They register themselves as BEV devices and
> the normal boot selection is used to determine whether a particular NIC
> gets network booted or not.
>
> Our roms do expose themselves as BEV roms so there's really no harm in
> loading an option rom while booting from disk.

Wrong.  Loading a pxe rom makes qemu trying to boot from it, even with 
-boot c (using the roms shipped in pc-bios/).

Maybe SeaBIOS has better BEV support and handles things differently, so 
we could load them unconditionally once we made the switch.

> Any PCI device can have a rom and it probably should be a generic
> property of any PCI device. There's really nothing specific about
> network adapters.

It's pc-specific though, so when we go the route of loading roms 
unconditionally we need to wrap that into a machine-specific helper 
function so it happes on TARGET_I386 only.

>> When making the filename configurable it should be a separate property
>> like "rom-name" or simliar. I would suggest to NOT implement it unless
>> users actually ask for it ;)
>
> Quite a few users today replace the standard etherboot roms with gPXE roms.

Usually with the same file names though, so a simple '-boot n' continues 
to work.

cheers,
   Gerd
Anthony Liguori - Oct. 7, 2009, 2:17 p.m.
Gerd Hoffmann wrote:
> On 10/07/09 15:28, Anthony Liguori wrote:
>> Having a pxe flag is somewhat odd. Real network devices always have roms
>> and they always get loaded. They register themselves as BEV devices and
>> the normal boot selection is used to determine whether a particular NIC
>> gets network booted or not.
>>
>> Our roms do expose themselves as BEV roms so there's really no harm in
>> loading an option rom while booting from disk.
>
> Wrong.  Loading a pxe rom makes qemu trying to boot from it, even with 
> -boot c (using the roms shipped in pc-bios/).

Only with the e1000 because the rom is misconfig.  Try the ne2k or the 
rtl8139.

>> Any PCI device can have a rom and it probably should be a generic
>> property of any PCI device. There's really nothing specific about
>> network adapters.
>
> It's pc-specific though, so when we go the route of loading roms 
> unconditionally we need to wrap that into a machine-specific helper 
> function so it happes on TARGET_I386 only.

No, it's not pc-specific.  An e1000 card on a PPC still has an x86 
option rom.  Whether it gets loaded and how it gets loaded depends on 
the target, but not the existence of the rom on the device.

Regards,

Anthony Liguori
Gerd Hoffmann - Oct. 7, 2009, 2:27 p.m.
>> Wrong. Loading a pxe rom makes qemu trying to boot from it, even with
>> -boot c (using the roms shipped in pc-bios/).
>
> Only with the e1000 because the rom is misconfig. Try the ne2k or the
> rtl8139.

rtl8139 works indeed (and shows up in the F12 menu as it should).

>> It's pc-specific though, so when we go the route of loading roms
>> unconditionally we need to wrap that into a machine-specific helper
>> function so it happes on TARGET_I386 only.
>
> No, it's not pc-specific. An e1000 card on a PPC still has an x86 option
> rom. Whether it gets loaded and how it gets loaded depends on the
> target, but not the existence of the rom on the device.

Yep, the *loading* is what I was referring to (see $subject) ...

cheers,
   Gerd
Anthony Liguori - Oct. 7, 2009, 6:34 p.m.
Gerd Hoffmann wrote:
>>> Wrong. Loading a pxe rom makes qemu trying to boot from it, even with
>>> -boot c (using the roms shipped in pc-bios/).
>>
>> Only with the e1000 because the rom is misconfig. Try the ne2k or the
>> rtl8139.
>
> rtl8139 works indeed (and shows up in the F12 menu as it should).
>
>>> It's pc-specific though, so when we go the route of loading roms
>>> unconditionally we need to wrap that into a machine-specific helper
>>> function so it happes on TARGET_I386 only.
>>
>> No, it's not pc-specific. An e1000 card on a PPC still has an x86 option
>> rom. Whether it gets loaded and how it gets loaded depends on the
>> target, but not the existence of the rom on the device.
>
> Yep, the *loading* is what I was referring to (see $subject) ...

Well, I guess I'm confused about where we stand.

Are you suggesting that we drop the pxe property and load the roms 
unconditionally?  The tricky thing here is that we only want to load a 
particular rom once.  There's no need to load the rtl8139 multiple times 
for multiple nics.

Regards,

Anthony Liguori
Gerd Hoffmann - Oct. 12, 2009, 10:13 a.m.
>>>> It's pc-specific though, so when we go the route of loading roms
>>>> unconditionally we need to wrap that into a machine-specific helper
>>>> function so it happes on TARGET_I386 only.
>>>
>>> No, it's not pc-specific. An e1000 card on a PPC still has an x86 option
>>> rom. Whether it gets loaded and how it gets loaded depends on the
>>> target, but not the existence of the rom on the device.
>>
>> Yep, the *loading* is what I was referring to (see $subject) ...
>
> Well, I guess I'm confused about where we stand.

loading the rom is x86 specific ...

> Are you suggesting that we drop the pxe property and load the roms
> unconditionally?

Yes, I think we should do that (on x86), given BEV works nicely.
The e1000 rom needs fixing first of course ;)

> The tricky thing here is that we only want to load a
> particular rom once. There's no need to load the rtl8139 multiple times
> for multiple nics.

As hw/loader.c keeps track of the roms this should be easy to do.

The rom_add_option() macro in hw/loader.h should become a function which 
loads the rom on x86 and does nothing on other archs.  Then the nic 
drivers can simply call rom_add_option("pxe-${driver}.bin") unconditionally.

cheers,
   Gerd

Patch

diff --git a/hw/ne2000-isa.c b/hw/ne2000-isa.c
index 7a24088..1d296a0 100644
--- a/hw/ne2000-isa.c
+++ b/hw/ne2000-isa.c
@@ -27,6 +27,7 @@ 
 #include "qdev.h"
 #include "net.h"
 #include "ne2000.h"
+#include "loader.h"
 
 typedef struct ISANE2000State {
     ISADevice dev;
@@ -78,6 +79,13 @@  static int isa_ne2000_initfn(ISADevice *dev)
         qemu_error("warning: no vlan specfied, ne2k_isa is unconnected\n");
     }
 
+    if (s->pxe) {
+        if (rom_add_option("pxe-ne2k_isa.bin") != 0) {
+            qemu_error("warning: loading rom pxe-ne2k_isa.bin failed\n");
+            s->pxe = 0;
+        }
+    }
+
     register_savevm("ne2000", -1, 2, ne2000_save, ne2000_load, s);
     return 0;
 }
@@ -104,6 +112,7 @@  static ISADeviceInfo ne2000_isa_info = {
     .qdev.props = (Property[]) {
         DEFINE_PROP_HEX32("iobase", ISANE2000State, iobase,       0x300),
         DEFINE_PROP_UINT32("irq",   ISANE2000State, isairq,       9),
+        DEFINE_PROP_UINT32("pxe",   ISANE2000State, ne2000.pxe,   0),
         DEFINE_PROP_MACADDR("mac",  ISANE2000State, ne2000.macaddr),
         DEFINE_PROP_INT32("vlan",   ISANE2000State, ne2000.vlan,  -1),
         DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/ne2000.h b/hw/ne2000.h
index 7936fb2..febaf6a 100644
--- a/hw/ne2000.h
+++ b/hw/ne2000.h
@@ -23,6 +23,7 @@  typedef struct NE2000State {
     uint8_t mult[8]; /* multicast mask array */
     qemu_irq irq;
     int32_t vlan;
+    uint32_t pxe;
     VLANClientState *vc;
     macaddr_t macaddr;
     uint8_t mem[NE2000_MEM_SIZE];