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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.10.2021 15:10, Roger Pau Monné wrote:
> On Mon, Oct 18, 2021 at 12:18:16PM +0200, Jan Beulich wrote:
>> 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.
> 
> Thanks, I think your proposed solution is fine.
> 
>> 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?
> 
> Hm, hard to tell TBH. HT being part of the platform specification
> for AMD it would feel wrong for hypervisors to attempt to populate
> this range.

Okay, I'll leave it as is then. If we decide otherwise, we always
can make an incremental change. Thanks for the feedback!

Jan