Re: [Xen-devel] [PATCH v3 4/6] tools: load IPXE from standalone file

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Wei Liu writes ("[Xen-devel] [PATCH v3 4/6] tools: load IPXE from standalone 
file"):
> Do not embed IPXE into Rombios anymore. Instead, it is loaded by the
> toolstack from a file as a separate module.
...
> -    void (*bios_load)(const struct bios_config *config, void *addr, uint32_t 
> size);
> +    void (*bios_load)(const struct bios_config *config, void *addr,
> +                      uint32_t size, void *extra_addr);
...
>  #ifdef ENABLE_ROMBIOS
>      else if ( bios == &rombios_config )
>      {
> -        bios->bios_load(bios, NULL, 0);
> +        const struct hvm_modlist_entry *ipxe;
> +        uint32_t paddr = 0;
> +
> +        ipxe = get_module_entry(hvm_start_info, "ipxe");
> +        if ( ipxe )
> +            paddr = ipxe->paddr;
> +        bios->bios_load(bios, NULL, 0, (void *)paddr);
...
>  static void ovmf_load(const struct bios_config *config,
> -                      void *bios_addr, uint32_t bios_length)
> +                      void *bios_addr, uint32_t bios_length,
> +                      void *unused_addr)
...
> +static void *ipxe_module_addr;
...
> +    if ( ipxe_module_addr )
> +    {
> +        etherboot_sz = scan_etherboot_nic(OPTIONROM_PHYSICAL_END,
> +                                          etherboot_phys_addr,
> +                                          ipxe_module_addr);
> +
> +        option_rom_phys_addr = etherboot_phys_addr + etherboot_sz;
> +        option_rom_sz = pci_load_option_roms(OPTIONROM_PHYSICAL_END,
> +                                             option_rom_phys_addr);
> +    }
...
>  static void rombios_load(const struct bios_config *config,
> -                         void *unused_addr, uint32_t unused_size)
> +                         void *unused_addr, uint32_t unused_size,
> +                         void *ipxe_addr)
>  {
>      uint32_t bioshigh;
>      struct rombios_info *info;
> @@ -133,6 +140,9 @@ static void rombios_load(const struct bios_config *config,
>  
>      info = (struct rombios_info *)BIOS_INFO_PHYSICAL_ADDRESS;
>      info->bios32_entry = bioshigh;
> +
> +    /* Stash ipxe address */
> +    ipxe_module_addr = ipxe_addr;

This seems to me to be a layering violation, and quite an ugly one at
that.  I'm afraid that at the very least, IMO this needs to be better
documented both in the code and the commit message.

Is ipxe rombios-specific ?  Forgive my ignorance.

Thanks,
Ian.

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