Re: [PATCH 3/3] x86/msr: Fix Solaris and turbostat following XSA-351

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.03.2021 17:18, Andrew Cooper wrote:
> @@ -284,6 +283,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t 
> *val)
>              goto gp_fault;
>          break;
>  
> +    case MSR_RAPL_POWER_UNIT:
> +        /*
> +         * This MSR is non-architectural.  However, some versions of Solaris
> +         * blindly reads it without a #GP guard, and some versions of
> +         * turbostat crash after expecting a read of /proc/cpu/0/msr not to
> +         * fail.  Read as zero on Intel hardware.
> +         */
> +        if ( !(cp->x86_vendor & X86_VENDOR_INTEL) )
> +            goto gp_fault;
> +        *val = 0;
> +        break;
> +
>          /*
>           * These MSRs are not enumerated in CPUID.  They have been around
>           * since the Pentium 4, and implemented by other vendors.
> 

I find all of this confusing, at best: I'd expect any entity reading
this MSR to - when successful - go on and read further MSRs. I have a
hard time seeing why those subsequent reads would be any less prone
to being unguarded. In fact I went and looked at the turbostat
sources. From its introduction the read of this MSR was done with -
afaict - appropriate error handling. There are anomalies (do_rapl
getting set prior to the probing of this MSR), but they look to not
matter stability-wise as the respective MSR reads are similarly
guarded. Were the observed crashes perhaps just in some private
versions of the tool? If so, I guess saying so in the comment (or
description) would be appropriate, but this still wouldn't invalidate
the general aspect of my remark.

On the good side the value of zero isn't entirely invalid, at least
as far as defined bitfields of the MSR go.

While looking around I also came across MSR_PLATFORM_ENERGY_COUNTER.
This one has specific precautions in the SDM: "This MSR is valid only
if both platform vendor hardware implementation and BIOS enablement
support it. This MSR will read 0 if not valid." Isn't this a fairly
strong suggestion that instead of raising #GP for it, we'd better
return zero as well? Because of the remark, unlike for certain other
MSRs, consumers have to expect zero possibly coming back.

Jan