Re: [Xen-devel] [PATCH] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.02.14 at 01:32, Aravind Gopalakrishnan 
>>> <aravind.gopalakrishnan@xxxxxxx> wrote:
> +/* check for AMD MC4 extended MISC register presence */
> +static inline int amd_thresholding_reg_present(uint32_t msr)
> +{
> +    uint64_t val;
> +    rdmsr_safe(msr, val);

You ought to check the result of this operation, even if at present
it clear "val" on error.

I also wonder what good it does to repeatedly trigger #GP here
if we already once learned that there's no such register. IOW,
please store the fact that the register is absent in a static
variable (and no, this shouldn't be a per-CPU one - if the register
is missing on any pCPU, we must not try to access it anywhere, as
vCPU-s could end up running once here and once there; in the end
we assume consistency across the CPUs in a system anyway).

> +    if ( val & (AMD_MC4_MISC_VAL_MASK | AMD_MC4_MISC_CNTP_MASK) )
> +        return 1;
> +
> +    return 0;
> +}
> +
>  /* amd specific MCA MSR */
>  int vmce_amd_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>  {
> -     switch (msr) {
> -     case MSR_F10_MC4_MISC1: /* DRAM error type */
> -             v->arch.vmce.bank[1].mci_misc = val; 
> -             mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
> -             break;
> -     case MSR_F10_MC4_MISC2: /* Link error type */
> -     case MSR_F10_MC4_MISC3: /* L3 cache error type */
> -             /* ignore write: we do not emulate link and l3 cache errors
> -              * to the guest.
> -              */
> -             mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
> -             break;
> -     default:
> -             return 0;
> -     }
> +    /* If not present, #GP fault, else do nothing as we don't emulate */
> +    if ( !amd_thresholding_reg_present(msr) )
> +        return -1;

The one thing I'm concerned about making this #GP in the guest is
migration: With it being _newer_ CPUs implementing fewer of these
MSRs, it would be impossible to migrate a guest from an older system
to a newer one - a direction that (as long as the newer system
provides all the hardware capabilities the older one has) is generally
assumed to work. Bottom line - we're probably better off always
dropping writes, and always returning zero for reads. Which will
eliminate the need for amd_thresholding_reg_present().

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel