Re: [Xen-devel] [PATCH v3 for 4.13] x86/e820: fix 640k - 1M region reservation logic

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 31/10/2019 11:07, Jan Beulich wrote:
> On 31.10.2019 11:56, Sergey Dyasli wrote:
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -274,6 +274,15 @@ static inline u32 phys_pkg_id(u32 cpuid_apic, int 
>> index_msb)
>>      return _phys_pkg_id(get_apic_id(), index_msb);
>>  }
>>  
>> +/*
>> + * Sometimes it's too early to use cpu_has_hypervisor which is available 
>> only
>> + * after early_cpu_init().
>> + */
>> +bool __init early_cpu_has_hypervisor(void)
>> +{
>> +    return cpuid_ecx(1) & cpufeat_mask(X86_FEATURE_HYPERVISOR);
>> +}

Static inline alongside the other cpuid helpers please.  This assembles
to smaller than the call required to get to a separate translation unit.

> OOI, did you consider introducing a more general early_cpu_has(),
> with X86_FEATURE_* passed as an argument?

This is interim code.  Personally, I'm still go with what I said at the
very beginning, of opencoding it just in the hunk below.

>
>> @@ -318,9 +319,10 @@ static int __init copy_e820_map(struct e820entry * 
>> biosmap, unsigned int nr_map)
>>  
>>          /*
>>           * Some BIOSes claim RAM in the 640k - 1M region.
>> -         * Not right. Fix it up.
>> +         * Not right. Fix it up, but only when running on bare metal.
>>           */
>> -        if (type == E820_RAM) {
>> +        if ( !early_cpu_has_hypervisor() && type == E820_RAM )
>> +        {
>>              if (start < 0x100000ULL && end > 0xA0000ULL) {
>>                  if (start < 0xA0000ULL)
>>                      add_memory_region(start, 0xA0000ULL-start, type);
> Seeing original line and lower context - aren't you corrupting
> previously reasonably consistent (Linux) style here? (This could
> be easily taken care of while committing, but I'd first like the
> point below be clarified.)
>
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -5943,7 +5943,7 @@ const struct platform_bad_page *__init 
>> get_platform_badpages(unsigned int *array
>>      case 0x000806e0: /* erratum KBL??? */
>>      case 0x000906e0: /* errata KBL??? / KBW114 / CFW103 */
>>          *array_size = (cpuid_eax(0) >= 7 &&
>> -                       !(cpuid_ecx(1) & 
>> cpufeat_mask(X86_FEATURE_HYPERVISOR)) &&
>> +                       !early_cpu_has_hypervisor() &&
> Strictly speaking this makes clear that in early_cpu_has_hypervisor()
> you should also check cpuid_eax(0) >= 1. We don't currently seem to
> object to running on a system with only basic leaf 0 available (we
> do object to there not being extended leaf 1). Andrew, do you have
> any clear opinion one way or the other?

By the time we are running in C, we know the CPU actually has long mode
and therefore max_leaf > 4(?) and max_extd_leaf >= 0x80000008

I don't see any value in not relying on this property, and a cost (extra
branches) to not relying on it.

~Andrew

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