Re: XSA-351 causing Solaris-11 systems to panic during boot.

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.12.2020 02:51, boris.ostrovsky@xxxxxxxxxx wrote:
> 
> On 11/17/20 3:12 AM, Jan Beulich wrote:
>> On 16.11.2020 22:57, Cheyenne Wills wrote:
>>> Running Xen with XSA-351 is causing Solaris 11 systems to panic during
>>> boot.  The panic screen is showing the failure to be coming from
>>> "unix:rdmsr".  The panic occurs with existing guests (booting off a disk)
>>> and the  booting from an install ISO image.
>>>
>>> I discussed the problem with "andyhhp__" in the "#xen" IRC channel and he
>>> requested that I report it here.
>> Thanks. What we need though is information on the specific MSR(s) that
>> will need to have workarounds added: We surely would want to avoid
>> blindly doing this for all that the XSA change disallowed access to.
>> Reproducing the panic screen here might already help; proper full logs
>> would be even better.
> 
> 
> We hit this issue today so I poked a bit around Solaris code.
> 
> 
> It definitely reads MSR_RAPL_POWER_UNIT unguarded during boot.
> 
> 
> In addition, it may read MSR_*_ENERGY_STATUS when running kstat. I haven't 
> been able to trigger those reads (I didn't have access to the system myself 
> and with neither me nor the tester remembering much about Solaris we only 
> tried some basic commands).
> 
> 
> The patch below lets Solaris guest boot on OVM. Our codebase is somewhat 
> different from stable branches but if this is an acceptable workaround I will 
> send proper patch for stable. I won't be able to test it though.

I think this is acceptable as a workaround, albeit we may want to
consider further restricting this (at least on staging), like e.g.
requiring a guest config setting to enable the workaround. But
maybe this will need to be part of the MSR policy for the domain
instead, down the road. We'll definitely want Andrew's view here.

Speaking of staging - before applying anything to the stable
branches, I think we want to have this addressed on the main
branch. I can't see how Solaris would work there.

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -131,6 +131,18 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>          *val &= ~(ARCH_CAPS_TSX_CTRL);
>          break;
>  
> +        /* Solaris reads these MSRs unguarded so let's return 0 */
> +    case MSR_RAPL_POWER_UNIT:
> +    case MSR_PKG_ENERGY_STATUS:
> +    case MSR_DRAM_ENERGY_STATUS:
> +    case MSR_PP0_ENERGY_STATUS:
> +    case MSR_PP1_ENERGY_STATUS:
> +        if ( boot_cpu_data.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.
> @@ -151,11 +163,16 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>              break;
>  
>          /*fallthrough*/
> -    case MSR_RAPL_POWER_UNIT:
> -    case MSR_PKG_POWER_LIMIT  ... MSR_PKG_POWER_INFO:
> -    case MSR_DRAM_POWER_LIMIT ... MSR_DRAM_POWER_INFO:
> -    case MSR_PP0_POWER_LIMIT  ... MSR_PP0_POLICY:
> -    case MSR_PP1_POWER_LIMIT  ... MSR_PP1_POLICY:
> +    case MSR_PKG_POWER_LIMIT:
> +    case MSR_PKG_PERF_STATUS:
> +    case MSR_PKG_POWER_INFO:
> +    case MSR_DRAM_POWER_LIMIT:
> +    case MSR_DRAM_PERF_STATUS:
> +    case MSR_DRAM_POWER_INFO:
> +    case MSR_PP0_POWER_LIMIT:
> +    case MSR_PP0_POLICY:
> +    case MSR_PP1_POWER_LIMIT:
> +    case MSR_PP1_POLICY:
>      case MSR_PLATFORM_ENERGY_COUNTER:
>      case MSR_PLATFORM_POWER_LIMIT:
>      case MSR_F15H_CU_POWER ... MSR_F15H_CU_MAX_POWER:

Note how you no longer handle MSRs previously included (one each
in the first two groups) in the range expressions. I think I'd
prefer the alternative of filtering just the STATUS ones here:

    case MSR_PKG_POWER_LIMIT  ... MSR_PKG_POWER_INFO:
    case MSR_DRAM_POWER_LIMIT ... MSR_DRAM_POWER_INFO:
    case MSR_PP0_POWER_LIMIT  ... MSR_PP0_POLICY:
    case MSR_PP1_POWER_LIMIT  ... MSR_PP1_POLICY:
        if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
             (msr & 0xf) != 1 /* MSR_*_POWER_STATUS */ )
            goto gp_fault;

        *val = 0;
        break;

Or, folding in MSR_RAPL_POWER_UNIT,

    case MSR_PKG_POWER_LIMIT  ... MSR_PKG_POWER_INFO:
    case MSR_DRAM_POWER_LIMIT ... MSR_DRAM_POWER_INFO:
    case MSR_PP0_POWER_LIMIT  ... MSR_PP0_POLICY:
    case MSR_PP1_POWER_LIMIT  ... MSR_PP1_POLICY:
        if ( (msr & 0xf) != 1 /* MSR_*_POWER_STATUS */ )
            goto gp_fault;
        /* fallthrough */
    case MSR_RAPL_POWER_UNIT:
        if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL )
            goto gp_fault;

        *val = 0;
        break;

Jan