[PATCH RFC] hvmloader: Make ROM dependencies optional

[PATCH RFC] hvmloader: Make ROM dependencies optional

[Julian Pidancet]

When booting HVMs with SeaBIOS, the BIOS itself takes care of extracting
option ROMs from the PCI devices. These ROMs are usually provided with by
the device model (qemu).
Thus, hvmloader should not require any longer the rombios, stdvga,
cirrusvga or etherboot ROMs to be present.

Also, the 32bitbios_support.c file is specific to rombios and should not
be built when building hvmloader with seabios.

Signed-off-by: Julian Pidancet <julian.pidancet@gmail.com>
---
 tools/firmware/hvmloader/Makefile    |   37 ++++++++++++++++++++++++---------
 tools/firmware/hvmloader/hvmloader.c |   13 ++++++++++-
 2 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index 41a4369..a8e0f97 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -29,7 +29,7 @@ LOADADDR = 0x100000
 CFLAGS += $(CFLAGS_xeninclude)
 
 OBJS  = hvmloader.o mp_tables.o util.o smbios.o 
-OBJS += 32bitbios_support.o smp.o cacheattr.o xenbus.o
+OBJS += smp.o cacheattr.o xenbus.o
 OBJS += e820.o pci.o pir.o ctype.o
 ifeq ($(debug),y)
 OBJS += tests.o
@@ -37,25 +37,41 @@ endif
 
 CIRRUSVGA_DEBUG ?= n
 
-ROMBIOS_DIR := ../rombios
+ROMBIOS_DIR ?= ../rombios
 ifneq ($(ROMBIOS_DIR),)
-OBJS += rombios.o
+OBJS += rombios.o 32bitbios_support.o 
 CFLAGS += -DENABLE_ROMBIOS
 ROMBIOS_ROM := $(ROMBIOS_DIR)/BIOS-bochs-latest
 endif
 
-SEABIOS_DIR := ../seabios-dir
+SEABIOS_DIR ?= ../seabios-dir
 ifneq ($(SEABIOS_DIR),)
 OBJS += seabios.o
 CFLAGS += -DENABLE_SEABIOS
 SEABIOS_ROM := $(SEABIOS_DIR)/out/bios.bin
 endif
 
-STDVGA_ROM    := ../vgabios/VGABIOS-lgpl-latest.bin
+STDVGA_DIR ?= ../vgabios/
+ifneq ($(STDVGA_DIR),)
+STDVGA_ROM    := $(STDVGA_DIR)/VGABIOS-lgpl-latest.bin
+CFLAGS += -DENABLE_STDVGA
+endif
+
+CIRRUSVGA_DIR ?= ../vgabios/
+ifneq ($(CIRRUSVGA_DIR),)
 ifeq ($(CIRRUSVGA_DEBUG),y)
-CIRRUSVGA_ROM := ../vgabios/VGABIOS-lgpl-latest.cirrus.debug.bin
+CIRRUSVGA_ROM := $(CIRRUSVGA_DIR)/VGABIOS-lgpl-latest.cirrus.debug.bin
 else
-CIRRUSVGA_ROM := ../vgabios/VGABIOS-lgpl-latest.cirrus.bin
+CIRRUSVGA_ROM := $(CIRRUSVGA_DIR)/VGABIOS-lgpl-latest.cirrus.bin
+endif
+CFLAGS += -DENABLE_CIRRUSVGA
+endif
+
+ETHERBOOT_DIR ?= ../etherboot/ipxe
+ETHERBOOT_NIC := rtl8139
+ifneq ($(ETHERBOOT_DIR),)
+CFLAGS += -DENABLE_ETHERBOOT
+ETHERBOOT_ROM := $(ETHERBOOT_DIR)/src/bin/$(ETHERBOOT_NIC).rom
 endif
 
 .PHONY: all
@@ -70,7 +86,7 @@ hvmloader: $(OBJS) acpi/acpi.a
 	$(OBJCOPY) hvmloader.tmp hvmloader
 	rm -f hvmloader.tmp
 
-roms.inc: $(ROMBIOS_ROM) $(SEABIOS_ROM) $(STDVGA_ROM) $(CIRRUSVGA_ROM) ../etherboot/eb-roms.h
+roms.inc: $(ROMBIOS_ROM) $(SEABIOS_ROM) $(STDVGA_ROM) $(CIRRUSVGA_ROM) $(ETHERBOOT_ROM)
 	echo "/* Autogenerated file. DO NOT EDIT */" > $@.new
 
 ifneq ($(ROMBIOS_ROM),)
@@ -95,10 +111,11 @@ ifneq ($(CIRRUSVGA_ROM),)
 	sh ./mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
 	echo "#endif" >> $@.new
 endif
-
+ifneq ($(ETHERBOOT_ROM),)
 	echo "#ifdef ROM_INCLUDE_ETHERBOOT" >> $@.new
-	cat ../etherboot/eb-roms.h >> $@.new
+	sh ./mkhex etherboot $(ETHERBOOT_ROM) >> $@.new
 	echo "#endif" >> $@.new
+endif
 
 	mv $@.new $@
 
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index f120ffe..659a382 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -236,6 +236,7 @@ static int scan_option_rom(
     return round_option_rom(rom->rom_size * 512 + 1);
 }
 
+#ifdef ENABLE_ETHERBOOT
 /*
  * Scan the PCI bus for the first NIC supported by etherboot, and copy
  * the corresponding rom data to *copy_rom_dest. Returns the length of the
@@ -264,6 +265,7 @@ static int scan_etherboot_nic(unsigned int option_rom_end,
 
     return rom_size;
 }
+#endif
 
 /*
  * Scan the PCI bus for the devices that have an option ROM, and copy
@@ -475,16 +477,20 @@ int main(void)
         switch ( virtual_vga )
         {
         case VGA_cirrus:
+#ifdef ENABLE_CIRRUSVGA
             printf("Loading Cirrus VGABIOS ...\n");
             memcpy((void *)VGABIOS_PHYSICAL_ADDRESS,
                    vgabios_cirrusvga, sizeof(vgabios_cirrusvga));
             vgabios_sz = round_option_rom(sizeof(vgabios_cirrusvga));
+#endif
             break;
         case VGA_std:
+#ifdef ENABLE_STDVGA
             printf("Loading Standard VGABIOS ...\n");
             memcpy((void *)VGABIOS_PHYSICAL_ADDRESS,
                    vgabios_stdvga, sizeof(vgabios_stdvga));
             vgabios_sz = round_option_rom(sizeof(vgabios_stdvga));
+#endif
             break;
         case VGA_pt:
             printf("Loading VGABIOS of passthroughed gfx ...\n");
@@ -496,13 +502,16 @@ int main(void)
             break;
         }
 
-        etherboot_phys_addr = VGABIOS_PHYSICAL_ADDRESS + vgabios_sz;
+        option_rom_phys_addr = VGABIOS_PHYSICAL_ADDRESS + vgabios_sz;
+#ifdef ENABLE_ETHERBOOT
+        etherboot_phys_addr = option_rom_phys_addr;
         if ( etherboot_phys_addr < bios->optionrom_start )
             etherboot_phys_addr = bios->optionrom_start;
         etherboot_sz = scan_etherboot_nic(bios->optionrom_end,
                                           etherboot_phys_addr);
 
-        option_rom_phys_addr = etherboot_phys_addr + etherboot_sz;
+        option_rom_phys_addr += etherboot_sz;
+#endif
         option_rom_sz = pci_load_option_roms(bios->optionrom_end,
                                              option_rom_phys_addr);
     }
-- 
Julian Pidancet

Re: [PATCH RFC] hvmloader: Make ROM dependencies optional

[Ian Campbell]

On Sun, 2012-02-05 at 16:18 +0000, Julian Pidancet wrote:
> When booting HVMs with SeaBIOS, the BIOS itself takes care of extracting
> option ROMs from the PCI devices. These ROMs are usually provided with by
> the device model (qemu).
> Thus, hvmloader should not require any longer the rombios, stdvga,
> cirrusvga or etherboot ROMs to be present.
> 
> Also, the 32bitbios_support.c file is specific to rombios and should not
> be built when building hvmloader with seabios.

> [...]
> @@ -475,16 +477,20 @@ int main(void)
>         switch ( virtual_vga )
>          {
>          case VGA_cirrus:
> +#ifdef ENABLE_CIRRUSVGA

Just outside the context here is an "if ( bios->load_roms )" which
prevents the ROMs from being loaded if we are using SeaBIOS -- is that
not sufficient for your needs?

Currently hvmloader builds with support for both ROMBIOS and SeaBIOS and
selects the one to use based on a configuration key in xenstore (pushed
down by the toolstack depending on which qemu you are running). Have you
modified something elsewhere in order to build a SeaBIOS-only hvmloader?

Perhaps it would make sense for the ROMs to be keyed off whether or not
ROMBIOS is enabled rather than each keying off their own DIR var?

You seem to have mixed some other changes, specifically s/:=/?=/ in a
few places and also some sort of change wrt how eb-roms.h is generated
(including dropping the 8086100e ROM from the image, which must be
discussed).

These other changes should be submitted and rationalised separately.

Ian.


>              printf("Loading Cirrus VGABIOS ...\n");
>              memcpy((void *)VGABIOS_PHYSICAL_ADDRESS,
>                     vgabios_cirrusvga, sizeof(vgabios_cirrusvga));
>              vgabios_sz = round_option_rom(sizeof(vgabios_cirrusvga));
> +#endif
>              break;
>          case VGA_std:
> +#ifdef ENABLE_STDVGA
>              printf("Loading Standard VGABIOS ...\n");
>              memcpy((void *)VGABIOS_PHYSICAL_ADDRESS,
>                     vgabios_stdvga, sizeof(vgabios_stdvga));
>              vgabios_sz = round_option_rom(sizeof(vgabios_stdvga));
> +#endif
>              break;
>          case VGA_pt:
>              printf("Loading VGABIOS of passthroughed gfx ...\n");
> @@ -496,13 +502,16 @@ int main(void)
>              break;
>          }
>  
> -        etherboot_phys_addr = VGABIOS_PHYSICAL_ADDRESS + vgabios_sz;
> +        option_rom_phys_addr = VGABIOS_PHYSICAL_ADDRESS + vgabios_sz;
> +#ifdef ENABLE_ETHERBOOT
> +        etherboot_phys_addr = option_rom_phys_addr;
>          if ( etherboot_phys_addr < bios->optionrom_start )
>              etherboot_phys_addr = bios->optionrom_start;
>          etherboot_sz = scan_etherboot_nic(bios->optionrom_end,
>                                            etherboot_phys_addr);
>  
> -        option_rom_phys_addr = etherboot_phys_addr + etherboot_sz;
> +        option_rom_phys_addr += etherboot_sz;
> +#endif
>          option_rom_sz = pci_load_option_roms(bios->optionrom_end,
>                                               option_rom_phys_addr);
>      }

Re: [PATCH RFC] hvmloader: Make ROM dependencies optional

[Julian Pidancet]

On Wed, Feb 8, 2012 at 1:28 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Sun, 2012-02-05 at 16:18 +0000, Julian Pidancet wrote:
>> When booting HVMs with SeaBIOS, the BIOS itself takes care of extracting
>> option ROMs from the PCI devices. These ROMs are usually provided with by
>> the device model (qemu).
>> Thus, hvmloader should not require any longer the rombios, stdvga,
>> cirrusvga or etherboot ROMs to be present.
>>
>> Also, the 32bitbios_support.c file is specific to rombios and should not
>> be built when building hvmloader with seabios.
>
>> [...]
>> @@ -475,16 +477,20 @@ int main(void)
>>         switch ( virtual_vga )
>>          {
>>          case VGA_cirrus:
>> +#ifdef ENABLE_CIRRUSVGA
>
> Just outside the context here is an "if ( bios->load_roms )" which
> prevents the ROMs from being loaded if we are using SeaBIOS -- is that
> not sufficient for your needs?
>
> Currently hvmloader builds with support for both ROMBIOS and SeaBIOS and
> selects the one to use based on a configuration key in xenstore (pushed
> down by the toolstack depending on which qemu you are running). Have you
> modified something elsewhere in order to build a SeaBIOS-only hvmloader?
>
> Perhaps it would make sense for the ROMs to be keyed off whether or not
> ROMBIOS is enabled rather than each keying off their own DIR var?
>
> You seem to have mixed some other changes, specifically s/:=/?=/ in a
> few places and also some sort of change wrt how eb-roms.h is generated
> (including dropping the 8086100e ROM from the image, which must be
> discussed).
>
> These other changes should be submitted and rationalised separately.
>

The purpose of this change was initially to allow the user to build
hvmloader with SeaBIOS only without having to depend on rombios,
vgabios or ipxe. I've only tested my changes by calling make from the
hvmloader/ directory directly. The Xen build system could also not
have to depend on bcc for example.

In addition to the patch I submitted, I suppose it would make sense to
introduce a global configuration option for the Makefile in the
firmware/ directory to decide wether it should recurse in the rombios,
vgabios, and etherboot directories.

I changed some variable assignations := by ?= because I wanted the
user of the build system to be able to specify a custom build
directory for these ROMs.

Same thing for the etherboot ROM, I made the Makefile not use eb-rom.h
and replace it with a call to mkhex on the binary file directly, so
the user can choose which ROM to inject into hvmloader, and I found it
to be more consistent with the rest of the Makefile.

-- 
Julian

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH RFC] hvmloader: Make ROM dependencies optional

[Ian Campbell]

On Wed, 2012-02-08 at 14:53 +0000, Julian Pidancet wrote:
> On Wed, Feb 8, 2012 at 1:28 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Sun, 2012-02-05 at 16:18 +0000, Julian Pidancet wrote:
> >> When booting HVMs with SeaBIOS, the BIOS itself takes care of extracting
> >> option ROMs from the PCI devices. These ROMs are usually provided with by
> >> the device model (qemu).
> >> Thus, hvmloader should not require any longer the rombios, stdvga,
> >> cirrusvga or etherboot ROMs to be present.
> >>
> >> Also, the 32bitbios_support.c file is specific to rombios and should not
> >> be built when building hvmloader with seabios.
> >
> >> [...]
> >> @@ -475,16 +477,20 @@ int main(void)
> >>         switch ( virtual_vga )
> >>          {
> >>          case VGA_cirrus:
> >> +#ifdef ENABLE_CIRRUSVGA
> >
> > Just outside the context here is an "if ( bios->load_roms )" which
> > prevents the ROMs from being loaded if we are using SeaBIOS -- is that
> > not sufficient for your needs?
> >
> > Currently hvmloader builds with support for both ROMBIOS and SeaBIOS and
> > selects the one to use based on a configuration key in xenstore (pushed
> > down by the toolstack depending on which qemu you are running). Have you
> > modified something elsewhere in order to build a SeaBIOS-only hvmloader?
> >
> > Perhaps it would make sense for the ROMs to be keyed off whether or not
> > ROMBIOS is enabled rather than each keying off their own DIR var?
> >
> > You seem to have mixed some other changes, specifically s/:=/?=/ in a
> > few places and also some sort of change wrt how eb-roms.h is generated
> > (including dropping the 8086100e ROM from the image, which must be
> > discussed).
> >
> > These other changes should be submitted and rationalised separately.
> >
> 
> The purpose of this change was initially to allow the user to build
> hvmloader with SeaBIOS only without having to depend on rombios,
> vgabios or ipxe. I've only tested my changes by calling make from the
> hvmloader/ directory directly. The Xen build system could also not
> have to depend on bcc for example.
> 
> In addition to the patch I submitted, I suppose it would make sense to
> introduce a global configuration option for the Makefile in the
> firmware/ directory to decide wether it should recurse in the rombios,
> vgabios, and etherboot directories.
> 
> I changed some variable assignations := by ?= because I wanted the
> user of the build system to be able to specify a custom build
> directory for these ROMs.

Seems fair enough, although I would still like to see separate issues
solved in separate patches.

> Same thing for the etherboot ROM, I made the Makefile not use eb-rom.h
> and replace it with a call to mkhex on the binary file directly, so
> the user can choose which ROM to inject into hvmloader, and I found it
> to be more consistent with the rest of the Makefile.

I don't think you have replaced like with like. Specifically you seem to
have omitted the 8086100e ROM -- look how eb-roms.h is built in
tools/firmware/etherboot using NICS = rtl8139 8086100e from the file
"Config" compared with your code which only uses ETHERBOOT_NIC :=
rtl8139.

Ian.

Re: [PATCH RFC] hvmloader: Make ROM dependencies optional

[Julian Pidancet]

On Wed, Feb 8, 2012 at 5:26 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2012-02-08 at 14:53 +0000, Julian Pidancet wrote:
>> On Wed, Feb 8, 2012 at 1:28 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Sun, 2012-02-05 at 16:18 +0000, Julian Pidancet wrote:
>> >> When booting HVMs with SeaBIOS, the BIOS itself takes care of extracting
>> >> option ROMs from the PCI devices. These ROMs are usually provided with by
>> >> the device model (qemu).
>> >> Thus, hvmloader should not require any longer the rombios, stdvga,
>> >> cirrusvga or etherboot ROMs to be present.
>> >>
>> >> Also, the 32bitbios_support.c file is specific to rombios and should not
>> >> be built when building hvmloader with seabios.
>> >
>> >> [...]
>> >> @@ -475,16 +477,20 @@ int main(void)
>> >>         switch ( virtual_vga )
>> >>          {
>> >>          case VGA_cirrus:
>> >> +#ifdef ENABLE_CIRRUSVGA
>> >
>> > Just outside the context here is an "if ( bios->load_roms )" which
>> > prevents the ROMs from being loaded if we are using SeaBIOS -- is that
>> > not sufficient for your needs?
>> >
>> > Currently hvmloader builds with support for both ROMBIOS and SeaBIOS and
>> > selects the one to use based on a configuration key in xenstore (pushed
>> > down by the toolstack depending on which qemu you are running). Have you
>> > modified something elsewhere in order to build a SeaBIOS-only hvmloader?
>> >
>> > Perhaps it would make sense for the ROMs to be keyed off whether or not
>> > ROMBIOS is enabled rather than each keying off their own DIR var?
>> >
>> > You seem to have mixed some other changes, specifically s/:=/?=/ in a
>> > few places and also some sort of change wrt how eb-roms.h is generated
>> > (including dropping the 8086100e ROM from the image, which must be
>> > discussed).
>> >
>> > These other changes should be submitted and rationalised separately.
>> >
>>
>> The purpose of this change was initially to allow the user to build
>> hvmloader with SeaBIOS only without having to depend on rombios,
>> vgabios or ipxe. I've only tested my changes by calling make from the
>> hvmloader/ directory directly. The Xen build system could also not
>> have to depend on bcc for example.
>>
>> In addition to the patch I submitted, I suppose it would make sense to
>> introduce a global configuration option for the Makefile in the
>> firmware/ directory to decide wether it should recurse in the rombios,
>> vgabios, and etherboot directories.
>>
>> I changed some variable assignations := by ?= because I wanted the
>> user of the build system to be able to specify a custom build
>> directory for these ROMs.
>
> Seems fair enough, although I would still like to see separate issues
> solved in separate patches.
>
>> Same thing for the etherboot ROM, I made the Makefile not use eb-rom.h
>> and replace it with a call to mkhex on the binary file directly, so
>> the user can choose which ROM to inject into hvmloader, and I found it
>> to be more consistent with the rest of the Makefile.
>
> I don't think you have replaced like with like. Specifically you seem to
> have omitted the 8086100e ROM -- look how eb-roms.h is built in
> tools/firmware/etherboot using NICS = rtl8139 8086100e from the file
> "Config" compared with your code which only uses ETHERBOOT_NIC :=
> rtl8139.
>

You are right, I wrongly assumed that eb-roms.h only contained one ROM.

-- 
Julian

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel