Re: [PATCH 4] MCA physical address check when calculate domain

["Jan Beulich" <JBeulich@novell.com>]

>>> On 07.05.11 at 22:29, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
> MCA physical address check when calculate domain
> 
> Bank addr maybe invalid, or 
> Bank addr maybe physical/memory/linear address or segment offset.
> This patch add mca MCi_STATUS_MISCV/MCi_STATUS_ADDRV check, and add physical 
> address verify,
> so that it work safe when calculate domain.
> 
> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
> 
> diff -r e4e1efda200f xen/arch/x86/cpu/mcheck/mce.c
> --- a/xen/arch/x86/cpu/mcheck/mce.c	Thu May 05 13:54:29 2011 +0800
> +++ b/xen/arch/x86/cpu/mcheck/mce.c	Fri May 06 13:56:21 2011 +0800
> @@ -151,7 +151,6 @@ static struct mcinfo_bank *mca_init_bank
>                                           struct mc_info *mi, int bank)
>  {
>      struct mcinfo_bank *mib;
> -    uint64_t addr=0, misc = 0;
>  
>      if (!mi)
>          return NULL;
> @@ -170,22 +169,23 @@ static struct mcinfo_bank *mca_init_bank
>      mib->common.size = sizeof (struct mcinfo_bank);
>      mib->mc_bank = bank;
>  
> -    addr = misc = 0;
>      if (mib->mc_status & MCi_STATUS_MISCV)
>          mib->mc_misc = mca_rdmsr(MSR_IA32_MCx_MISC(bank));
>  
>      if (mib->mc_status & MCi_STATUS_ADDRV)
> -    {
>          mib->mc_addr = mca_rdmsr(MSR_IA32_MCx_ADDR(bank));
>  
> -        if (mfn_valid(paddr_to_pfn(mib->mc_addr))) {
> -            struct domain *d;
> +    if ((mib->mc_status & MCi_STATUS_MISCV) &&
> +        (mib->mc_status & MCi_STATUS_ADDRV) &&
> +        ((mib->mc_misc & MCi_MISC_ADDRMOD_MASK) == MCi_MISC_PHYSMOD) && 
> +          mfn_valid(paddr_to_pfn(mib->mc_addr)))
> +    {
> +        struct domain *d;
>  
> -            d = maddr_get_owner(mib->mc_addr);
> -            if (d != NULL && (who == MCA_POLLER ||
> -                              who == MCA_CMCI_HANDLER))
> -                mib->mc_domid = d->domain_id;
> -        }
> +        d = maddr_get_owner(mib->mc_addr);
> +        if (d != NULL && (who == MCA_POLLER ||
> +                          who == MCA_CMCI_HANDLER))
> +            mib->mc_domid = d->domain_id;

This wasn't very logical before, and would probably be good if it
got changed while you re-structure it anyway: Rather than
calling maddr_get_owner() and *then* checking "who", that
check should be added to the other surrounding ones, thus
avoiding the possibly pointless call.

Further I wonder whether there aren't more cases for which the
domain ID could be set, as well as whether the !MISCV case
shouldn't also assume the address to be physical. Wouldn't that
be the case e.g. on older CPUs?

Jan

>      }
>  
>      if (who == MCA_CMCI_HANDLER) {