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

[Jan Beulich <jbeulich@xxxxxxxx>]

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);
> +}

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

> @@ -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?

Jan

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