Re: [PATCH] Xen/vMCE: remove is_vmce_ready check

[Christoph Egger <chegger@amazon.de>]

On 13.05.13 18:02, Liu, Jinsong wrote:
> From 50fbb875fcf567c75fdbc16c64491aa8e72746b9 Mon Sep 17 00:00:00 2001
> From: Liu Jinsong <jinsong.liu@intel.com>
> Date: Sat, 27 Apr 2013 22:37:35 +0800
> Subject: [PATCH] Xen/vMCE: remove is_vmce_ready check
> 
> Remove is_vmce_ready() check since
> 1. it's problematic and overkilled: it checks if virq bind to dom0 mcelog
> driver. That's not correct, since mcelog is just a dom0 driver used to log
> error info, irrelated to dom0 vmce injection. It's also overkilled, defaulty
> dom0 disabled mcelog driver, under such case this checking would resulting
> in system crash:
> (XEN) MCE: This error page is ownded by DOM 0
> (XEN) DOM0 not ready for vMCE
> (XEN) domain_crash called from mcaction.c:133
> (XEN) Domain 0 reported crashed by domain 32767 on cpu#31:
> (XEN) Domain 0 crashed: rebooting machine in 5 seconds.
> (XEN) Resetting with ACPI MEMORY or I/O RESET_REG.
> 
> 2. it's redundant: hypervisor in fact has checked
> 1). whether dom0 vmce ready or not (at inject_vmce()), via checking
>     vmce trap callback, to make sure vmce injection OK;
> 2). whether dom0 mcelog driver ready or not (at mce_softirq()), via
>     virq binding, to make sure error log works;
> 
> 3. it's deprecated: for hvm, it checks whether guest vcpu has different
> virtual family/model with that of host pcpu --> that's deprecated, since
> vMCE has changed a lot, not bound to host MCE any more.
> 
> Signed-off-by: Liu Jinsong <jinsong.liu@intel.com>

Acked-by: Christoph Egger <chegger@amazon.de>

P.S.: I like to see this backported to Xen 4.2. However,
the Xen 4.2 version may need an adaption as point 3. does not
count for Xen 4.2.

Christoph

> ---
>  xen/arch/x86/cpu/mcheck/mcaction.c |    6 ---
>  xen/arch/x86/cpu/mcheck/vmce.c     |   68 ------------------------------------
>  xen/arch/x86/cpu/mcheck/vmce.h     |    1 -
>  3 files changed, 0 insertions(+), 75 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/mcheck/mcaction.c b/xen/arch/x86/cpu/mcheck/mcaction.c
> index 0ac5b45..adf2ded 100644
> --- a/xen/arch/x86/cpu/mcheck/mcaction.c
> +++ b/xen/arch/x86/cpu/mcheck/mcaction.c
> @@ -83,12 +83,6 @@ mc_memerr_dhandler(struct mca_binfo *binfo,
>                  ASSERT(d);
>                  gfn = get_gpfn_from_mfn((bank->mc_addr) >> PAGE_SHIFT);
>  
> -                if ( !is_vmce_ready(bank, d) )
> -                {
> -                    printk("DOM%d not ready for vMCE\n", d->domain_id);
> -                    goto vmce_failed;
> -                }
> -
>                  if ( unmmap_broken_page(d, _mfn(mfn), gfn) )
>                  {
>                      printk("Unmap broken memory %lx for DOM%d failed\n",
> diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c
> index 7d3fac7..af3b491 100644
> --- a/xen/arch/x86/cpu/mcheck/vmce.c
> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
> @@ -413,74 +413,6 @@ int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d,
>      return 0;
>  }
>  
> -static int is_hvm_vmce_ready(struct mcinfo_bank *bank, struct domain *d)
> -{
> -    struct vcpu *v;
> -    int no_vmce = 0, i;
> -
> -    if (!is_hvm_domain(d))
> -        return 0;
> -
> -    /* kill guest if not enabled vMCE */
> -    for_each_vcpu(d, v)
> -    {
> -        if (!(v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_MCE))
> -        {
> -            no_vmce = 1;
> -            break;
> -        }
> -
> -        if (!mce_broadcast)
> -            break;
> -    }
> -
> -    if (no_vmce)
> -        return 0;
> -
> -    /* Guest has virtualized family/model information */
> -    for ( i = 0; i < MAX_CPUID_INPUT; i++ )
> -    {
> -        if (d->arch.cpuids[i].input[0] == 0x1)
> -        {
> -            uint32_t veax = d->arch.cpuids[i].eax, vfam, vmod;
> -
> -			vfam = (veax >> 8) & 15;
> -			vmod = (veax >> 4) & 15;
> -
> -            if (vfam == 0x6 || vfam == 0xf)
> -                vmod += ((veax >> 16) & 0xF) << 4;
> -			if (vfam == 0xf)
> -				vfam += (veax >> 20) & 0xff;
> -
> -            if ( ( vfam != boot_cpu_data.x86 ) ||
> -                 (vmod != boot_cpu_data.x86_model) )
> -            {
> -                dprintk(XENLOG_WARNING,
> -                    "No vmce for different virtual family/model cpuid\n");
> -                no_vmce = 1;
> -            }
> -            break;
> -        }
> -    }
> -
> -    if (no_vmce)
> -        return 0;
> -
> -    return 1;
> -}
> -
> -int is_vmce_ready(struct mcinfo_bank *bank, struct domain *d)
> -{
> -    if ( d == dom0)
> -        return dom0_vmce_enabled();
> -
> -    /* No vMCE to HVM guest now */
> -    if ( is_hvm_domain(d) )
> -        return is_hvm_vmce_ready(bank, d);
> -
> -    return 0;
> -}
> -
>  /* It's said some ram is setup as mmio_direct for UC cache attribute */
>  #define P2M_UNMAP_TYPES (p2m_to_mask(p2m_ram_rw) \
>                                  | p2m_to_mask(p2m_ram_logdirty) \
> diff --git a/xen/arch/x86/cpu/mcheck/vmce.h b/xen/arch/x86/cpu/mcheck/vmce.h
> index 7263deb..6b2c95a 100644
> --- a/xen/arch/x86/cpu/mcheck/vmce.h
> +++ b/xen/arch/x86/cpu/mcheck/vmce.h
> @@ -8,7 +8,6 @@ int vmce_init(struct cpuinfo_x86 *c);
>  #define dom0_vmce_enabled() (dom0 && dom0->max_vcpus && dom0->vcpu[0] \
>          && guest_enabled_event(dom0->vcpu[0], VIRQ_MCA))
>  
> -int is_vmce_ready(struct mcinfo_bank *bank, struct domain *d);
>  int unmmap_broken_page(struct domain *d, mfn_t mfn, unsigned long gfn);
>  
>  int vmce_intel_rdmsr(const struct vcpu *, uint32_t msr, uint64_t *val);
>