Re: [PATCH v2 3/3] x86/xen: supply rsdp address in boot params for pvh guests

[Juergen Gross]

On 08/12/17 08:22, Ingo Molnar wrote:
> 
> * Juergen Gross <jgross@xxxxxxxx> wrote:
> 
>> When booted via the special PVH entry save the RSDP address set in the
>> boot information block in struct boot_params. This will enable Xen to
>> locate the RSDP at an arbitrary address.
>>
>> Set the boot loader version to 2.14 (0x020e) replacing the wrong 0x0212
>> which should have been 0x020c.
>>
>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>> ---
>> V2: set bootloader version to 2.14 (Roger Pau MonnÃ)
>> ---
>>  arch/x86/xen/enlighten_pvh.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
>> index 436c4f003e17..036e3a5f284a 100644
>> --- a/arch/x86/xen/enlighten_pvh.c
>> +++ b/arch/x86/xen/enlighten_pvh.c
>> @@ -68,9 +68,12 @@ static void __init init_pvh_bootparams(void)
>>  	 *
>>  	 * Version 2.12 supports Xen entry point but we will use default x86/PC
>>  	 * environment (i.e. hardware_subarch 0).
>> +	 * The RSDP address is available from version 2.14 on.
>>  	 */
>> -	pvh_bootparams.hdr.version = 0x212;
>> +	pvh_bootparams.hdr.version = 0x20e;
> 
> While 0x212 was "obvious" to read but totally wrong, it would be less fragile and 
> more readable if the version was generated as something like:
> 
> 	pvh_bootparams.hdr.version = (2 << 8) | 14;

Sure, I can make that change.

> 
> similar to how it's written in other cases:
> 
>>  	pvh_bootparams.hdr.type_of_loader = (9 << 4) | 0; /* Xen loader */
> 
> Also, shouldn't the 0x212 fix be a separate patch, Cc: stable? The bug appears to 
> have been introduced at around v4.12.

While not really being very important, this seems to be cleaner, yes.
After all this value is visible in sysfs, so it should be correct.


Juergen