[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 09/19] x86/vmce: fill MSR_IA32_MCG_STATUS on all vcpus in broadcast case
>>> On 17.02.17 at 07:39, <haozhong.zhang@xxxxxxxxx> wrote: > --- a/xen/arch/x86/cpu/mcheck/mcaction.c > +++ b/xen/arch/x86/cpu/mcheck/mcaction.c > @@ -88,21 +88,21 @@ mc_memerr_dhandler(struct mca_binfo *binfo, > goto vmce_failed; > } > > + if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) > + vmce_vcpuid = VMCE_INJECT_BROADCAST; > + else > + vmce_vcpuid = global->mc_vcpuid; > + > bank->mc_addr = gfn << PAGE_SHIFT | > (bank->mc_addr & (PAGE_SIZE -1 )); > - if ( fill_vmsr_data(bank, d, > - global->mc_gstatus) == -1 ) > + if ( fill_vmsr_data(bank, d, global->mc_gstatus, > + vmce_vcpuid == VMCE_INJECT_BROADCAST) == > -1 ) You're changing the return values of function to be proper -E... values, so you can't validly compare against -1 here anymore. > int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d, > - uint64_t gstatus) > + uint64_t gstatus, bool broadcast) > { > struct vcpu *v = d->vcpu[0]; > + int ret; > > - if ( mc_bank->mc_domid != (uint16_t)~0 ) > - { > - if ( v->arch.vmce.mcg_status & MCG_STATUS_MCIP ) > - { > - mce_printk(MCE_QUIET, "MCE: guest has not handled previous" > - " vMCE yet!\n"); > - return -1; > - } > - > - spin_lock(&v->arch.vmce.lock); > + if ( mc_bank->mc_domid == (uint16_t)~0 ) > + return -EINVAL; Returning -EINVAL here is a behavioral change which, if intended, needs justification in the commit message. > - v->arch.vmce.mcg_status = gstatus; > - /* > - * 1. Skip bank 0 to avoid 'bank 0 quirk' of old processors > - * 2. Filter MCi_STATUS MSCOD model specific error code to guest > - */ > - v->arch.vmce.bank[1].mci_status = mc_bank->mc_status & > - MCi_STATUS_MSCOD_MASK; > - v->arch.vmce.bank[1].mci_addr = mc_bank->mc_addr; > - v->arch.vmce.bank[1].mci_misc = mc_bank->mc_misc; > + ret = vcpu_fill_mc_msrs(v, gstatus, mc_bank->mc_status, > + mc_bank->mc_addr, mc_bank->mc_misc); > + if ( ret || !broadcast ) > + goto out; > > - spin_unlock(&v->arch.vmce.lock); > + for_each_vcpu ( d, v ) > + { > + if ( v == d->vcpu[0] ) Comparing v->vcpu_id with zero would be a cheaper check as it seems. > + continue; > + ret = vcpu_fill_mc_msrs(v, MCG_STATUS_MCIP | MCG_STATUS_RIPV, > + 0, 0, 0); What guarantees RIP to be valid, and why all the zeros? Perhaps I'm lacking some understanding of how real hardware handles the broadcasting, but it would help if you left an explaining comment here. > + if ( ret ) > + break; Wouldn't you better, on a best effort basis, try to update the remaining vCPU-s anyway (making sure you don't lose the error indicator to be handed back to your caller). > } > > - return 0; > + out: > + return ret; If at the destination of a goto all you do is return, please avoid using goto. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |