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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.03.2020 11:29, Roger Pau Monné wrote:
> On Tue, Mar 03, 2020 at 10:14:50AM +0100, Jan Beulich wrote:
>> On 02.03.2020 16:55, Roger Pau Monne wrote:
>>> +        start = MAX(ROUNDUP(d->arch.e820[i].addr, PAGE_SIZE), MB(1));
>>> +        end = d->arch.e820[i].addr + d->arch.e820[i].size;
>>
>> ... calculate "end" earlier and use it in the if() above?
> 
> Right.
> 
>>
>> As to the aligning to a 1Mb boundary - why are you doing this?
> 
> I'm not sure placing the initrd and metadata below 1MB is sensible,
> even if a region big enough was found. In pvh_populate_p2m we copy the
> data on the memory regions < 1MB, I'm not sure BDA/EBDA is marked as
> reserved in the memory map always.

I realize now that I misread the code - you don't align to a 1Mb
boundary, but you cap the range at the lower end. That's fine of
course.

>> I guess whatever the reason warrants a comment, the more that
>> further down you only align to page boundaries.
>>
>>> +        /* Deal with the kernel being loaded in the region. */
>>> +        if ( kernel_start <= start && kernel_end >= start )
>>
>> While it doesn't matter much, I think it would look better to
>> use > on the right side of the && here ...
>>
>>> +            /* Truncate the start of the region */
>>> +            start = ROUNDUP(kernel_end, PAGE_SIZE);
>>> +        else if ( kernel_start <= end && kernel_end >= end )
>>
>> and < on the left side of the && here. Furthermore - can this
>> really be "else if()" here, i.e. doesn't it need to be plain
>> "if()"?
> 
> I don't think so, as the region where the kernel has been loaded must
> always be a single RAM region. Ie: [kernel_start, kernel_end) is
> always going to be a subset of a RAM region.

I this true even with the page size alignment getting done?
IOW are all E820 ranges we produce exact multiples of 4k in
size and aligned to 4k boundaries?

>> Also, do both regions need to be adjacent? If not, wouldn't it be
>> better to find slots for them one by one?
> 
> That's going to be much more complicated, as you would have to account
> for previous regions while searching for empty spaces. If we want to
> go that route we would have to use a rangeset or some such in order to
> keep track of used areas.

I accept this being more complicated, and hence not really
wanting doing now and here. But perhaps you could leave a
comment to the effect that the choice of using a single
region is for simplicity reasons?

Jan

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