Re: [PATCH v2] x86/AMD: make HT range dynamic for Fam17 and up

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.10.2021 11:41, Roger Pau Monné wrote:
> On Mon, Jun 28, 2021 at 01:48:53PM +0200, Jan Beulich wrote:
>> At the time of d838ac2539cf ("x86: don't allow Dom0 access to the HT
>> address range") documentation correctly stated that the range was
>> completely fixed. For Fam17 and newer, it lives at the top of physical
>> address space, though.
>>
>> To correctly determine the top of physical address space, we need to
>> account for their physical address reduction, hence the calculation of
>> paddr_bits also gets adjusted.
>>
>> While for paddr_bits < 40 the HT range is completely hidden, there's no
>> need to suppress the range insertion in that case: It'll just have no
>> real meaning.
>>
>> Reported-by: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, but before applying this I'd prefer to resolve your concern
voiced below.

>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -349,16 +349,23 @@ void __init early_cpu_init(void)
>>  
>>      eax = cpuid_eax(0x80000000);
>>      if ((eax >> 16) == 0x8000 && eax >= 0x80000008) {
>> +            ebx = eax >= 0x8000001f ? cpuid_ebx(0x8000001f) : 0;
>>              eax = cpuid_eax(0x80000008);
>> +
>>              paddr_bits = eax & 0xff;
>>              if (paddr_bits > PADDR_BITS)
>>                      paddr_bits = PADDR_BITS;
>> +
>>              vaddr_bits = (eax >> 8) & 0xff;
>>              if (vaddr_bits > VADDR_BITS)
>>                      vaddr_bits = VADDR_BITS;
>> +
>>              hap_paddr_bits = ((eax >> 16) & 0xff) ?: paddr_bits;
>>              if (hap_paddr_bits > PADDR_BITS)
>>                      hap_paddr_bits = PADDR_BITS;
>> +
>> +            /* Account for SME's physical address space reduction. */
>> +            paddr_bits -= (ebx >> 6) & 0x3f;
> 
> Does it make sense to check for 0x8000001f[eax] bit 0 in order to
> assert that there's support for SME, or assuming that the reduction is
> != 0 in the other cpuid leaf is enough.

Documentation doesn't really tie them together afaics, so I thought
I wouldn't either. I was reading into this lack of an explicit
connection the possibility of address space reduction to also,
hypothetically at this point, apply to other features.

> It's possible for firmware vendors to disable advertising the SME
> support bit and leave the physical address space reduction one in
> place?

I don't know if it's possible (I'm unaware of e.g. MSR-level control
allowing to modify these independently), but if it is I'd consider
it inconsistent if one but not the other was zapped. I'm unconvinced
that we really would need to deal with such inconsistencies, the
more that it's not really clear what the inconsistent setting would
really mean for the placement of the HT range.

While writing this, there was one more thing I came to think of:
Should we perhaps suppress the iomem_deny_access() altogether when
running virtualized ourselves?

Jan