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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Jul 05, 2018 at 11:39:46AM +0100, Ian Jackson wrote:
> 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.
> 

There isn't a better way to do this at the moment. Plus that ...

> Is ipxe rombios-specific ?  Forgive my ignorance.

... ipxe is rombios-specific at the moment, so Jan is fine with changes
like this.

Wei.

> 
> Thanks,
> Ian.

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