Re: [Xen-devel] [PATCH v2] x86: ignore guest microcode loading attempts

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/03/18 11:09, Jan Beulich wrote:
> The respective MSRs are write-only, and hence attempts by guests to
> write to these are - as of 1f1d183d49 ("x86/HVM: don't give the wrong
> impression of WRMSR succeeding") no longer ignored. Restore original
> behavior for the two affected MSRs.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v2: Add comments.
> ---
> While what is being logged for the current osstest failures on the 4.7
> and 4.9 branches doesn't fully prove this to be the problem, RCX holding
> 0x79 and there being a recorded hypervisor level #GP recovery
> immediately before the guest triple fault is sufficient indication imo.
> What I'm unsure about is whether we want to ignore such writes also for
> PV guests. If not, at least the WRMSR change would need to move into
> hvm/hvm.c.

Sorry - I've raced with v2 of your patch.  The PV side currently always
yields #GP, because we have a read_safe() check on the wrmsr side.  With
that behaviour retained (i.e. with !is_pv_domain() checks in the hunks
below), Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

I specifically want to avoid any further additions to the legacy MSR
paths.  They should dwindle to nothing as we move functionality over.

>
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -147,6 +147,8 @@ int guest_rdmsr(const struct vcpu *v, ui
>  
>      switch ( msr )
>      {
> +    case MSR_AMD_PATCHLOADER:
> +    case MSR_IA32_UCODE_WRITE:
>      case MSR_PRED_CMD:
>          /* Write-only */
>          goto gp_fault;
> @@ -200,6 +202,26 @@ int guest_wrmsr(struct vcpu *v, uint32_t
>          /* Read-only */
>          goto gp_fault;
>  
> +    case MSR_AMD_PATCHLOADER:
> +        /*
> +         * See note on MSR_IA32_UCODE_WRITE below, which may or may not apply
> +         * to AMD CPUs as well (at least the architectural/CPUID part does).
> +         */
> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_AMD )
> +            goto gp_fault;
> +        break;
> +
> +    case MSR_IA32_UCODE_WRITE:
> +        /*
> +         * Some versions of Windows at least on certain hardware try to load
> +         * microcode before setting up an IDT. Therefore we must not inject 
> #GP
> +         * for such attempts. Also the MSR is architectural and not qualified
> +         * by any CPUID bit.
> +         */
> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL )
> +            goto gp_fault;
> +        break;
> +
>      case MSR_SPEC_CTRL:
>          if ( !cp->feat.ibrsb )
>              goto gp_fault; /* MSR available? */
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel