Re: [XEN PATCH] intel/msr: Fix handling of MSR_RAPL_POWER_UNIT

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

On Mon, Jan 13, 2025 at 06:42:44PM +0000, Teddy Astie wrote:
> Solaris 11.4 tries to access this MSR on some Intel platforms without properly
> setting up a proper #GP handler, which leads to a immediate crash.
> 
> Emulate the access of this MSR by giving it a legal value (all values set to
> default, as defined by Intel SDM "RAPL Interfaces").
> 
> Fixes: 84e848fd7a1 ('x86/hvm: disallow access to unknown MSRs')

Hm, 

> Signed-off-by: Teddy Astie <teddy.astie@xxxxxxxxxx>
> ---
> Does it have a risk of negatively affecting other operating systems expecting
> this MSR read to fail ?
> ---
>  xen/arch/x86/include/asm/msr-index.h |  2 ++
>  xen/arch/x86/msr.c                   | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/xen/arch/x86/include/asm/msr-index.h 
> b/xen/arch/x86/include/asm/msr-index.h
> index 9cdb5b2625..2adcdf344f 100644
> --- a/xen/arch/x86/include/asm/msr-index.h
> +++ b/xen/arch/x86/include/asm/msr-index.h
> @@ -144,6 +144,8 @@
>  #define MSR_RTIT_ADDR_A(n)                 (0x00000580 + (n) * 2)
>  #define MSR_RTIT_ADDR_B(n)                 (0x00000581 + (n) * 2)
>  
> +#define MSR_RAPL_POWER_UNIT                 0x00000606
> +
>  #define MSR_U_CET                           0x000006a0
>  #define MSR_S_CET                           0x000006a2
>  #define  CET_SHSTK_EN                       (_AC(1, ULL) <<  0)
> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index 289cf10b78..b14d42dacf 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -169,6 +169,22 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t 
> *val)
>          if ( likely(!is_cpufreq_controller(d)) || rdmsr_safe(msr, *val) == 0 
> )
>              break;
>          goto gp_fault;
> +    

Trailing spaces in the added newline.

> +        /*
> +         * Solaris 11.4 DomU tries to use read this MSR without setting up a
> +         * proper #GP handler leading to a crash. Emulate this MSR by giving 
> a
> +         * legal value.
> +         */

The comment should be after (inside) the case statement IMO (but not
strong opinion.  Could you also raise a bug with Solaris and put a
link to the bug report here, so that we have a reference to it?

> +    case MSR_RAPL_POWER_UNIT:
> +        if ( !(cp->x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR)) )

Has Centaur ever released a CPU with RAPL?

> +            goto gp_fault;
> +
> +        /*
> +         * Return a legal register content with all default values defined in
> +         * Intel Architecture Software Developer Manual 16.10.1 RAPL 
> Interfaces
> +         */
> +        *val = 0x0000A1003;

The SPR Specification defines the default as 000A0E03h:

* SDM:

Energy Status Units (bits 12:8): Energy related information (in
Joules) is based on the multiplier, 1/2^ESU; where ESU is an unsigned
integer represented by bits 12:8. Default value is 10000b, indicating
energy status unit is in 15.3 micro-Joules increment.

* SPR:

Energy Units (ENERGY_UNIT):
Energy Units used for power control registers.
The actual unit value is calculated by 1 J / Power(2,ENERGY_UNIT).
The default value of 14 corresponds to Ux.14 number.

Note that KVM just returns all 0s [0], so we might consider doing the
same, as otherwise that could lead OSes to poke at further RAPL
related MSRs if the returned value from MSR_RAPL_POWER_UNIT looks
plausible.

[0] https://elixir.bootlin.com/linux/v6.12.6/source/arch/x86/kvm/x86.c#L4236

Thanks.