| Message ID | 1254918996-26050-6-git-send-email-kraxel@redhat.com |
|---|---|
| State | RFC |
| Headers | show |
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
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
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
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
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
>> 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
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
>>>> 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
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];
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(-)