Re: [PATCH v5 01/14] libxc: Rework extra module initialisation

[Anthony PERARD <anthony.perard@citrix.com>]

On Fri, Jul 08, 2016 at 12:29:36PM +0100, Wei Liu wrote:
> On Fri, Jul 08, 2016 at 11:52:08AM +0100, Anthony PERARD wrote:
> > On Thu, Jul 07, 2016 at 03:55:23PM +0100, Wei Liu wrote:
> > > On Wed, Jun 22, 2016 at 06:15:32PM +0100, Anthony PERARD wrote:
> > > > diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
> > > > index 330d5e8..da8b995 100644
> > > > --- a/tools/libxc/xc_dom_hvmloader.c
> > > > +++ b/tools/libxc/xc_dom_hvmloader.c
> > > > @@ -129,98 +129,52 @@ static elf_errorstatus xc_dom_parse_hvm_kernel(struct xc_dom_image *dom)
> > > >      return rc;
> > > >  }
> > > >  
> > > > -static int modules_init(struct xc_dom_image *dom,
> > > > -                        uint64_t vend, struct elf_binary *elf,
> > > > -                        uint64_t *mstart_out, uint64_t *mend_out)
> > > > +static int module_init_one(struct xc_dom_image *dom,
> > > > +                           struct xc_hvm_firmware_module *module,
> > > > +                           char *name)
> > > >  {
> > > > -#define MODULE_ALIGN 1UL << 7
> > > > -#define MB_ALIGN     1UL << 20
> > > > -#define MKALIGN(x, a) (((uint64_t)(x) + (a) - 1) & ~(uint64_t)((a) - 1))
> > > > -    uint64_t total_len = 0, offset1 = 0;
> > > > +    struct xc_dom_seg seg;
> > > > +    void *dest;
> > > >  
> > > > -    if ( dom->acpi_module.length == 0 && dom->smbios_module.length == 0 )
> > > > -        return 0;
> > > > -
> > > > -    /* Find the total length for the firmware modules with a reasonable large
> > > > -     * alignment size to align each the modules.
> > > > -     */
> > > > -    total_len = MKALIGN(dom->acpi_module.length, MODULE_ALIGN);
> > > > -    offset1 = total_len;
> > > > -    total_len += MKALIGN(dom->smbios_module.length, MODULE_ALIGN);
> > > > -
> > > > -    /* Want to place the modules 1Mb+change behind the loader image. */
> > > > -    *mstart_out = MKALIGN(elf->pend, MB_ALIGN) + (MB_ALIGN);
> > > > -    *mend_out = *mstart_out + total_len;
> > > > -
> > > > -    if ( *mend_out > vend )
> > > > -        return -1;
> > > > -
> > > > -    if ( dom->acpi_module.length != 0 )
> > > > -        dom->acpi_module.guest_addr_out = *mstart_out;
> > > > -    if ( dom->smbios_module.length != 0 )
> > > > -        dom->smbios_module.guest_addr_out = *mstart_out + offset1;
> > > > +    if ( module->length )
> > > > +    {
> > > > +        if ( xc_dom_alloc_segment(dom, &seg, name, 0, module->length) )
> > > > +            goto err;
> > > > +        dest = xc_dom_seg_to_ptr(dom, &seg);
> > > > +        if ( dest == NULL )
> > > > +        {
> > > > +            DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &seg) => NULL",
> > > > +                      __FUNCTION__);
> > > > +            goto err;
> > > > +        }
> > > > +        memcpy(dest, module->data, module->length);
> > > > +        module->guest_addr_out = seg.vstart;
> > > > +        if ( module->guest_addr_out > UINT32_MAX ||
> > > > +             module->guest_addr_out + module->length > UINT32_MAX )
> > > > +        {
> > > > +            DOMPRINTF("%s: Module %s would be loaded abrove 4GB",
> > > > +                      __FUNCTION__, name);
> > > > +            goto err;
> > > > +        }
> > > 
> > > One question:
> > > 
> > > Can this check also account for MMIO hole below 4G? Maybe use
> > > dom->mmio_size?
> > 
> > Yes, I guess I can check against dom->mmio_start. Should I also check
> > that mmio_start have reasonable value? (<4G, and not 0x0) Or is
> > mmio_start is already supposed to have a good value?
> > 
> 
> mmio_start should already have a sane value here -- or at least I hope
> so. The sanity of mmio_start should be checked where it is assigned.

Ok, I'll use dom->mmio_start instead of UINT32_MAX, and probably add an
assert() on the values expected in mmio_start.

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel