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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> +            /* 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.

> +        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"?

Jan

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