Re: [Xen-devel] [PATCH 5/5] hvmloader: Use iPXE ROM loaded from a standalone file

2018-03-19 Thread Jan Beulich 
>>> On 19.03.18 at 15:24,  wrote:
> On 16/03/18 11:26, Jan Beulich wrote:
>>>
>>> +/* Physical address of iPXE ROM, loaded by domain builder
>>> + * when using ROMBIOS
>>> + */
>>> +unsigned int *ipxe_rom_addresss;
>> Comment style. And can the pointer be to const?
> 
> I will fixup the comment style and but making ipxe_rom_address a pointer 
> to const will require codes changes in hvmloader/rombios.c, 
> hvmloader/optionroms.c to make all function that use ipxe_rom_address 
> const as well. Are you fine with changing these files/functions.

In a separate patch, yes. If you don't want to create a separate
patch, I'm also fine with your explanation of why it can't be const
for now.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 5/5] hvmloader: Use iPXE ROM loaded from a standalone file

2018-03-19 Thread Anoob Soman 

On 16/03/18 11:26, Jan Beulich wrote:


+/* Physical address of iPXE ROM, loaded by domain builder
+ * when using ROMBIOS
+ */
+unsigned int *ipxe_rom_addresss;

Comment style. And can the pointer be to const?


I will fixup the comment style and but making ipxe_rom_address a pointer 
to const will require codes changes in hvmloader/rombios.c, 
hvmloader/optionroms.c to make all function that use ipxe_rom_address 
const as well. Are you fine with changing these files/functions.


-Anoob

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 5/5] hvmloader: Use iPXE ROM loaded from a standalone file

2018-03-17 Thread Doug Goldstein 
On 3/16/18 6:44 AM, Andrew Cooper wrote:
> On 16/03/18 11:26, Jan Beulich wrote:
> On 15.03.18 at 18:31,  wrote:
>>> --- a/tools/firmware/hvmloader/config.h
>>> +++ b/tools/firmware/hvmloader/config.h
>>> @@ -33,6 +33,11 @@ struct bios_config {
>>>  void (*create_mp_tables)(void);
>>>  void (*create_smbios_tables)(void);
>>>  void (*create_pir_tables)(void);
>>> +
>>> +/* Physical address of iPXE ROM, loaded by domain builder
>>> + * when using ROMBIOS
>>> + */
>>> +unsigned int *ipxe_rom_addresss;
>> Comment style. And can the pointer be to const?
>>
>>> @@ -368,7 +370,12 @@ int main(void)
>>>  #ifdef ENABLE_ROMBIOS
>>>  else if ( bios == _config )
>>>  {
>>> -bios->bios_load(bios, NULL, 0);
>>> +ipxe_module = get_module_entry(hvm_start_info, "ipxe");
>>> +
>>> +if ( ipxe_module )
>>> +ipxe_rom_addresss = ipxe_module->paddr;
>>> +
>>> +bios->bios_load(bios, (void *)ipxe_rom_addresss, 0);
>>>  }
>>>  #endif
>> Considering the #ifdef around here - is this whole series an
>> enhancement for qemu-trad only? I don't think we mean to make
>> any such enhancements anymore.
> 
> It is for several important reasons, including allowing distros to
> remove their qemu-trad packages completely.
> 
> 
> The main purpose is to have iPXE as a ROM not embedded in hvmloader, and
> finishes some incomplete earlier work by Anthony to split the SeaBIOS
> and OVMF roms out of HVMLoader (and then later some work which Boris
> offered to do, but time hasn't materialised yet).
> 
> This means that distros don't need to rebuild and redeploy their Xen
> packages every time they want to change their iPXE package and/or don't
> need to maintain multiple iPXE packages.>
> It also removes the final non-stubdom bit of the build which downloads a
> random tarball from the web, which is something downstreams have been
> crying out for for years.

I was originally asking if this was the case on the cover letter. I'll
lend a +1 to the overall series here. As someone that packages Xen in
two source based distros whose package managers run the build in a
separate phase from the fetch stage this is a welcome change. I've been
hacking around this and am glad to drop that.

-- 
Doug Goldstein

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 5/5] hvmloader: Use iPXE ROM loaded from a standalone file

2018-03-16 Thread Andrew Cooper 
On 16/03/18 11:26, Jan Beulich wrote:
 On 15.03.18 at 18:31,  wrote:
>> --- a/tools/firmware/hvmloader/config.h
>> +++ b/tools/firmware/hvmloader/config.h
>> @@ -33,6 +33,11 @@ struct bios_config {
>>  void (*create_mp_tables)(void);
>>  void (*create_smbios_tables)(void);
>>  void (*create_pir_tables)(void);
>> +
>> +/* Physical address of iPXE ROM, loaded by domain builder
>> + * when using ROMBIOS
>> + */
>> +unsigned int *ipxe_rom_addresss;
> Comment style. And can the pointer be to const?
>
>> @@ -368,7 +370,12 @@ int main(void)
>>  #ifdef ENABLE_ROMBIOS
>>  else if ( bios == _config )
>>  {
>> -bios->bios_load(bios, NULL, 0);
>> +ipxe_module = get_module_entry(hvm_start_info, "ipxe");
>> +
>> +if ( ipxe_module )
>> +ipxe_rom_addresss = ipxe_module->paddr;
>> +
>> +bios->bios_load(bios, (void *)ipxe_rom_addresss, 0);
>>  }
>>  #endif
> Considering the #ifdef around here - is this whole series an
> enhancement for qemu-trad only? I don't think we mean to make
> any such enhancements anymore.

It is for several important reasons, including allowing distros to
remove their qemu-trad packages completely.


The main purpose is to have iPXE as a ROM not embedded in hvmloader, and
finishes some incomplete earlier work by Anthony to split the SeaBIOS
and OVMF roms out of HVMLoader (and then later some work which Boris
offered to do, but time hasn't materialised yet).

This means that distros don't need to rebuild and redeploy their Xen
packages every time they want to change their iPXE package and/or don't
need to maintain multiple iPXE packages.

It also removes the final non-stubdom bit of the build which downloads a
random tarball from the web, which is something downstreams have been
crying out for for years.

As for the Qemu side of things, RomBIOS functions perfectly well with
qemu-upstream, and is the only viable transition mechanism available for
downstreams wishing to purge themselves of qemu-trad entirely, while
keeping older VMs working.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 5/5] hvmloader: Use iPXE ROM loaded from a standalone file

2018-03-16 Thread Jan Beulich 
>>> On 15.03.18 at 18:31,  wrote:
> --- a/tools/firmware/hvmloader/config.h
> +++ b/tools/firmware/hvmloader/config.h
> @@ -33,6 +33,11 @@ struct bios_config {
>  void (*create_mp_tables)(void);
>  void (*create_smbios_tables)(void);
>  void (*create_pir_tables)(void);
> +
> +/* Physical address of iPXE ROM, loaded by domain builder
> + * when using ROMBIOS
> + */
> +unsigned int *ipxe_rom_addresss;

Comment style. And can the pointer be to const?

> @@ -368,7 +370,12 @@ int main(void)
>  #ifdef ENABLE_ROMBIOS
>  else if ( bios == _config )
>  {
> -bios->bios_load(bios, NULL, 0);
> +ipxe_module = get_module_entry(hvm_start_info, "ipxe");
> +
> +if ( ipxe_module )
> +ipxe_rom_addresss = ipxe_module->paddr;
> +
> +bios->bios_load(bios, (void *)ipxe_rom_addresss, 0);
>  }
>  #endif

Considering the #ifdef around here - is this whole series an
enhancement for qemu-trad only? I don't think we mean to make
any such enhancements anymore.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 5/5] hvmloader: Use iPXE ROM loaded from a standalone file

2018-03-15 Thread Anoob Soman 
splatering of mkhex-ed etherboot inside hvmloader/rombios is removed,
instead hvmloader/rombios now relies on iPXE ROM to be added,loaded as
a module.

Signed-off-by: Anoob Soman 
---
 tools/firmware/hvmloader/Makefile|  7 +--
 tools/firmware/hvmloader/config.h|  5 +
 tools/firmware/hvmloader/hvmloader.c |  9 -
 tools/firmware/hvmloader/rombios.c   | 13 +
 4 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/tools/firmware/hvmloader/Makefile 
b/tools/firmware/hvmloader/Makefile
index fa1bf30..f347cb4 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -64,7 +64,7 @@ ifeq ($(CONFIG_ROMBIOS),y)
 OBJS += optionroms.o 32bitbios_support.o rombios.o
 CFLAGS += -DENABLE_ROMBIOS
 ROMBIOS_ROM := $(ROMBIOS_DIR)/BIOS-bochs-latest
-ROMS += $(ROMBIOS_ROM) $(STDVGA_ROM) $(CIRRUSVGA_ROM) $(ETHERBOOT_ROMS)
+ROMS += $(ROMBIOS_ROM) $(STDVGA_ROM) $(CIRRUSVGA_ROM)
 endif
 
 .PHONY: all
@@ -109,11 +109,6 @@ ifneq ($(CIRRUSVGA_ROM),)
sh ../../misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
echo "#endif" >> $@.new
 endif
-ifneq ($(ETHERBOOT_ROMS),)
-   echo "#ifdef ROM_INCLUDE_ETHERBOOT" >> $@.new
-   sh ../../misc/mkhex etherboot $(ETHERBOOT_ROMS) >> $@.new
-   echo "#endif" >> $@.new
-endif
 
mv $@.new $@
 
diff --git a/tools/firmware/hvmloader/config.h 
b/tools/firmware/hvmloader/config.h
index 6e00413..8f5d1040 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -33,6 +33,11 @@ struct bios_config {
 void (*create_mp_tables)(void);
 void (*create_smbios_tables)(void);
 void (*create_pir_tables)(void);
+
+/* Physical address of iPXE ROM, loaded by domain builder
+ * when using ROMBIOS
+ */
+unsigned int *ipxe_rom_addresss;
 };
 
 extern struct bios_config rombios_config;
diff --git a/tools/firmware/hvmloader/hvmloader.c 
b/tools/firmware/hvmloader/hvmloader.c
index f603f68..731cd81 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -323,6 +323,8 @@ int main(void)
 const struct bios_config *bios;
 int acpi_enabled;
 const struct hvm_modlist_entry *bios_module;
+const struct hvm_modlist_entry *ipxe_module = NULL;
+unsigned int ipxe_rom_addresss = 0;
 
 /* Initialise hypercall stubs with RET, rendering them no-ops. */
 memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE);
@@ -368,7 +370,12 @@ int main(void)
 #ifdef ENABLE_ROMBIOS
 else if ( bios == _config )
 {
-bios->bios_load(bios, NULL, 0);
+ipxe_module = get_module_entry(hvm_start_info, "ipxe");
+
+if ( ipxe_module )
+ipxe_rom_addresss = ipxe_module->paddr;
+
+bios->bios_load(bios, (void *)ipxe_rom_addresss, 0);
 }
 #endif
 else
diff --git a/tools/firmware/hvmloader/rombios.c 
b/tools/firmware/hvmloader/rombios.c
index c736fd9..c0e04cb 100644
--- a/tools/firmware/hvmloader/rombios.c
+++ b/tools/firmware/hvmloader/rombios.c
@@ -95,9 +95,12 @@ static void rombios_load_roms(void)
 etherboot_phys_addr = VGABIOS_PHYSICAL_ADDRESS + vgabios_sz;
 if ( etherboot_phys_addr < OPTIONROM_PHYSICAL_ADDRESS )
 etherboot_phys_addr = OPTIONROM_PHYSICAL_ADDRESS;
-etherboot_sz = scan_etherboot_nic(OPTIONROM_PHYSICAL_END,
-  etherboot_phys_addr,
-  etherboot);
+
+if ( rombios_config.ipxe_rom_addresss )
+etherboot_sz = scan_etherboot_nic(OPTIONROM_PHYSICAL_END,
+  etherboot_phys_addr,
+  rombios_config.ipxe_rom_addresss);
+
 
 option_rom_phys_addr = etherboot_phys_addr + etherboot_sz;
 option_rom_sz = pci_load_option_roms(OPTIONROM_PHYSICAL_END,
@@ -119,7 +122,7 @@ static void rombios_load_roms(void)
 }
 
 static void rombios_load(const struct bios_config *config,
- void *unused_addr, uint32_t unused_size)
+ void *ipxe_rom_addr, uint32_t unused_size)
 {
 uint32_t bioshigh;
 struct rombios_info *info;
@@ -133,6 +136,8 @@ static void rombios_load(const struct bios_config *config,
 
 info = (struct rombios_info *)BIOS_INFO_PHYSICAL_ADDRESS;
 info->bios32_entry = bioshigh;
+
+rombios_config.ipxe_rom_addresss = ipxe_rom_addr;
 }
 
 /*
-- 
1.8.3.1


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel