Re: [Xen-devel] [PATCH] xen: arm: Support <32MB frametables

["Chris (Christopher) Brand" <chris.brand@xxxxxxxxxxxx>]

Hi Julien,

Thanks for the review.

> OOI, you win 30MB of RAM but how does this affect the performance?

Fair question. :-) All I can say is that I don't see any noticeable difference 
on my
system. Are there performance tests that you suggest I run ? Also, note that
the new code is only executed if you specify a previously-invalid value for
xenheap_megabytes on the command-line, so it won't affect any existing
systems. Is it worth adding a sentence mentioning performance to the
documentation, do you think ?

> Can you rework create_32mb_mappings to take the size of the mappings in 
> parameters?

Yeah, I have a version like that here somewhere, but it wasn't as clean as I'd 
hoped.
I'll re-visit it and send as v2.

>> -    /* Round up to 32M boundary */
>> -    frametable_size = (frametable_size + 0x1ffffff) & ~0x1ffffff;
>> +    /* Round up to 2M or 32M boundary, as appropriate. */
>> +    if ( frametable_size < MB(32) )
>> +        mask = MB(2) - 1;
>> +    else
>> +        mask = MB(32) - 1;
>> +    frametable_size = (frametable_size + mask) & ~mask;
>
>You can use ROUNDUP(frametable_size, size) to avoid open-coding the mask.

Will do.

> Also, this code is common with ARM64. If we happen to have a board with a
> frametable smaller than 32MB, you will round up to 2MB and crash later in
> create_32mb_mappings because you don't support 2MB mapping for ARM64.
> 
> It might be good to support 2MB for ARM64 too.

Whoops! Yes, I've messed the ARM64 path up at some point while cleaning
up the code. Will fix.

Thanks,

Chris


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