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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

Regards, Roger.