Re: [Xen-devel] [PATCH v2] x86/dom0: improve PVH initrd and metadata placement

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Mar 03, 2020 at 04:40:36PM +0100, Jan Beulich wrote:
> On 03.03.2020 12:52, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/dom0_build.c
> > +++ b/xen/arch/x86/hvm/dom0_build.c
> > @@ -490,6 +490,45 @@ static int __init pvh_populate_p2m(struct domain *d)
> >  #undef MB1_PAGES
> >  }
> >  
> > +static paddr_t find_memory(const struct domain *d, const struct elf_binary 
> > *elf,
> > +                           size_t size)
> > +{
> > +    paddr_t kernel_start = (paddr_t)elf->dest_base;
> > +    paddr_t kernel_end = (paddr_t)(elf->dest_base + elf->dest_size);
> > +    unsigned int i;
> > +
> > +    for ( i = 0; i < d->arch.nr_e820; i++ )
> > +    {
> > +        paddr_t start, end = d->arch.e820[i].addr + d->arch.e820[i].size;
> > +
> > +        /* Don't use memory below 1MB, as it could overwrite the BDA/EBDA. 
> > */
> > +        if ( end <= MB(1) || d->arch.e820[i].type != E820_RAM )
> > +            continue;
> > +
> > +        start = MAX(ROUNDUP(d->arch.e820[i].addr, PAGE_SIZE), MB(1));
> > +
> > +        if ( end <= kernel_start || start >= kernel_end )
> > +            ; /* No overlap, nothing to do. */
> > +        /* Deal with the kernel already being loaded in the region. */
> > +        else if ( kernel_start <= start && kernel_end > start )
> 
> Since, according to your reply on v1, [kernel_start,kernel_end) is
> a subset of [start,end), I understand that the <= could equally
> well be == - do you agree? From this then ...
> 
> > +            /* Truncate the start of the region. */
> > +            start = ROUNDUP(kernel_end, PAGE_SIZE);
> > +        else if ( kernel_start <= end && kernel_end > end )
> 
> ... it follows that you now have two off-by-1s here, as you changed
> the right side of the && instead of the left one (the right side
> could, as per above, use == again). Using == in both places would,
> in lieu of a comment, imo make more visible to the reader that
> there is this sub-range relationship between both ranges.

Right, I agree to both the above and have adjusted the conditions.

> > +            /* Truncate the end of the region. */
> > +            end = kernel_start;
> > +        /* Pick the biggest of the split regions. */
> 
> Then again - wouldn't this part suffice? if start == kernel_start
> or end == kernel_end, one side of the "split" region would simply
> be empty.

That's why it's using an else if construct, so that we only get
here if the kernel is loaded in the middle of the region, and there
are two regions left as part of the split.

> 
> > +        else if ( kernel_start - start > end - kernel_end )
> > +            end = kernel_start;
> > +        else
> > +            start = ROUNDUP(kernel_end, PAGE_SIZE);
> > +
> > +        if ( end - start >= size )
> > +            return start;
> > +    }
> > +
> > +    return INVALID_PADDR;
> > +}
> > +
> >  static int __init pvh_load_kernel(struct domain *d, const module_t *image,
> >                                    unsigned long image_headroom,
> >                                    module_t *initrd, void *image_base,
> > @@ -546,7 +585,24 @@ static int __init pvh_load_kernel(struct domain *d, 
> > const module_t *image,
> >          return rc;
> >      }
> >  
> > -    last_addr = ROUNDUP(parms.virt_kend - parms.virt_base, PAGE_SIZE);
> > +    /*
> > +     * Find a RAM region big enough (and that doesn't overlap with the 
> > loaded
> > +     * kernel) in order to load the initrd and the metadata. Note it could 
> > be
> > +     * split into smaller allocations, done it as a single region in order 
> > to
> > +     * simplify it.
> 
> I guess either "done" without "it" or "doing it"?

Fixed, thanks.

Roger.

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