Re: [PATCH v2 1/2] x86/amd: Add guest support for AMD TCE

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/04/2025 10:49 am, Teddy Astie wrote:
> AMD Translation Cache Extension is a flag that can be enabled in the EFER MSR 
> to optimize
> some TLB flushes. Expose this flag to guest if supported by hardware.
>
> AMD Architecture Developer Manual describe this feature as follow
>> Setting this bit to 1 changes how the INVLPG, INVLPGB, and INVPCID 
>> instructions operate
>> on TLB entries. When this bit is 0, these instructions remove the target PTE 
>> from the
>> TLB as well as all upper-level table entries that are cached in the TLB, 
>> whether or not
>> they are associated with the target PTE. When this bit is set, these 
>> instructions will
>> remove the target PTE and only those upper-level entries that lead to the 
>> target PTE in
>> the page table hierarchy, leaving unrelated upper-level entries intact. This 
>> may provide
>> a performance benefit.
> Signed-off-by: Teddy Astie <teddy.astie@xxxxxxxxxx>

You probably want to note that Linux 6.14(?) now uses this feature.

You definitely need to discuss that it's only in HAP VMs.  i.e. no PV,
and no Shadow.

Personally, I'd drop the quote from the APM.  You've already said "to
optimise some TLB flushes".  Anyone wanting to know details about the
feature needs to read the APM anyway.


> ---
>  CHANGELOG.md                                | 1 +
>  xen/arch/x86/hvm/hvm.c                      | 3 +++
>  xen/arch/x86/include/asm/msr-index.h        | 3 ++-
>  xen/include/public/arch-x86/cpufeatureset.h | 1 +
>  4 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index 8f6afa5c85..dbfecefbd4 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -18,6 +18,7 @@ The format is based on [Keep a 
> Changelog](https://keepachangelog.com/en/1.0.0/)
>     - Support PCI passthrough for HVM domUs when dom0 is PVH (note SR-IOV
>       capability usage is not yet supported on PVH dom0).
>     - Smoke tests for the FreeBSD Xen builds in Cirrus CI.
> +   - Guest support for AMD Translation Cache Extension feature.
>  
>  ### Removed
>  
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 5950f3160f..184357b042 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -959,6 +959,9 @@ const char *hvm_efer_valid(const struct vcpu *v, uint64_t 
> value,
>      if ( (value & EFER_FFXSE) && !p->extd.ffxsr )
>          return "FFXSE without feature";
>  
> +    if ( (value & EFER_TCE) && !p->extd.tce )
> +        return "TCE without feature";
> +
>      if ( (value & EFER_AIBRSE) && !p->extd.auto_ibrs )
>          return "AutoIBRS without feature";
>  

Not for this patch, but it's probably time we made hvm_efer_valid() work
more like hvm_cr4_guest_valid_bits().

The only interesting bit is the specific diagnostic for LMA/LME/CR0
mismatch, and I don't think the complexity is worth it for just that.

> diff --git a/xen/arch/x86/include/asm/msr-index.h 
> b/xen/arch/x86/include/asm/msr-index.h
> index 22d9e76e55..d8576aec1c 100644
> --- a/xen/arch/x86/include/asm/msr-index.h
> +++ b/xen/arch/x86/include/asm/msr-index.h
> @@ -200,11 +200,12 @@
>  #define  EFER_NXE                           (_AC(1, ULL) << 11) /* No 
> Execute Enable */
>  #define  EFER_SVME                          (_AC(1, ULL) << 12) /* Secure 
> Virtual Machine Enable */
>  #define  EFER_FFXSE                         (_AC(1, ULL) << 14) /* Fast 
> FXSAVE/FXRSTOR */
> +#define  EFER_TCE                           (_AC(1, ULL) << 15) /* 
> Translation Cache Extensions */
>  #define  EFER_AIBRSE                        (_AC(1, ULL) << 21) /* Automatic 
> IBRS Enable */
>  
>  #define EFER_KNOWN_MASK \
>      (EFER_SCE | EFER_LME | EFER_LMA | EFER_NXE | EFER_SVME | EFER_FFXSE | \
> -     EFER_AIBRSE)
> +     EFER_TCE | EFER_AIBRSE)

Sorry, missed one aspect.  You need to filter TSE out in guest_efer()
for PV guests.

>  
>  #define MSR_STAR                            _AC(0xc0000081, U) /* legacy 
> mode SYSCALL target */
>  #define MSR_LSTAR                           _AC(0xc0000082, U) /* long mode 
> SYSCALL target */
> diff --git a/xen/include/public/arch-x86/cpufeatureset.h 
> b/xen/include/public/arch-x86/cpufeatureset.h
> index cc6e984a88..8182d2dbed 100644
> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -170,6 +170,7 @@ XEN_CPUFEATURE(SKINIT,        3*32+12) /*   SKINIT/STGI 
> instructions */
>  XEN_CPUFEATURE(WDT,           3*32+13) /*   Watchdog timer */
>  XEN_CPUFEATURE(LWP,           3*32+15) /*   Light Weight Profiling */
>  XEN_CPUFEATURE(FMA4,          3*32+16) /*A  4 operands MAC instructions */
> +XEN_CPUFEATURE(TCE,           3*32+17) /*H  Translation Cache Extension 
> support */

I'd suggest dropping the "support".  It doesn't add anything here.

~Andrew