lkml.org 
[lkml]   [2009]   [Apr]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] Add MCE support to KVM
    Huang Ying wrote:
    > On Thu, 2009-04-09 at 23:50 +0800, Avi Kivity wrote:
    >
    >> Huang Ying wrote:
    >>
    >>> +int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
    >>> +{
    >>> + switch (msr) {
    >>> + case MSR_EFER:
    >>> + set_efer(vcpu, data);
    >>> break;
    >>> case MSR_IA32_DEBUGCTLMSR:
    >>> if (!data) {
    >>> @@ -807,6 +828,8 @@ int kvm_set_msr_common(struct kvm_vcpu *
    >>> break;
    >>> }
    >>> default:
    >>> + if (!set_msr_mce(vcpu, msr, data))
    >>> + break;
    >>> pr_unimpl(vcpu, "unhandled wrmsr: 0x%x data %llx\n", msr, data);
    >>> return 1;
    >>> }
    >>>
    >>>
    >> Is there any reason you split kvm_set_msr_common() into two functions?
    >>
    >
    > I want to group MCE related MSR together. And most MCE MSR read/write
    > need to access vcpu->arch.mcg_xxx or vcpu->arch_mce_banks, So I think
    > use a MCE specific function would be cleaner.
    >
    > But It seems that something as follow would be better.
    >
    > kvm_set_msr_comm()
    > {
    > switch (msr) {
    > case MSR_IA32_P5_MC_ADDR:
    > case MSR_IA32_P5_MC_TYPE:
    > case MSR_IA32_MCG_CAP:
    > case MSR_IA32_MCG_CTL:
    > case MSR_IA32_MCG_STATUS:
    > case MSR_IA32_MC0_CTL ... MSR_IA32_MC0_MISC + 4 * KVM_MCE_MAX_BANK:
    > set_msr_mce();
    > break;
    > ...
    > }
    > ...
    > }
    >
    >

    Yes. Just make sure KVM_MCE_MAX_BANK (better change to KVM_MCE_NR_BANK,
    with MAX you never know if it's the index of the last bank or the number
    of banks) doesn't conflict with any other MSRs.

    >>> +
    >>> +int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
    >>> +{
    >>> + u64 data;
    >>> +
    >>> + switch (msr) {
    >>> + case 0xc0010010: /* SYSCFG */
    >>> + case 0xc0010015: /* HWCR */
    >>>
    >>>
    >> Please use MSR_ constants (add them if they don't exist yet).
    >>
    >
    > In fact, this is not added by me. But I can change this by the way.
    >

    Oh okay. So don't change them in this patch.

    >> Why not always allocate it on vcpu setup?
    >>
    >
    > Because the MCE bank number is not fixed, it is based on mcg_cap from
    > user space.
    >

    Right, but we can allocate the maximum number, no? it's a fairly small
    amount of memory.

    >
    >>> +static int kvm_vcpu_ioctl_x86_set_mce(struct kvm_vcpu *vcpu,
    >>> + struct kvm_x86_mce *mce)
    >>> +{
    >>> + u64 mcg_cap = vcpu->arch.mcg_cap;
    >>> + unsigned bank_num = mcg_cap & 0xff;
    >>> + u64 *banks = vcpu->arch.mce_banks;
    >>> +
    >>> + if (mce->bank >= bank_num || !(mce->status & MCI_STATUS_VAL))
    >>> + return -EINVAL;
    >>> + /*
    >>> + * if IA32_MCG_CTL is not all 1s, the uncorrected error
    >>> + * reporting is disabled
    >>> + */
    >>> + if ((mce->status & MCI_STATUS_UC) && (mcg_cap & MCG_CTL_P) &&
    >>> + vcpu->arch.mcg_ctl != ~(u64)0)
    >>> + return 0;
    >>> + banks += 4 * mce->bank;
    >>> + /*
    >>> + * if IA32_MCi_CTL is not all 1s, the uncorrected error
    >>> + * reporting is disabled for the bank
    >>> + */
    >>> + if ((mce->status & MCI_STATUS_UC) && banks[0] != ~(u64)0)
    >>> + return 0;
    >>> + if (mce->status & MCI_STATUS_UC) {
    >>> + u64 status = mce->status;
    >>> + if ((vcpu->arch.mcg_status & MCG_STATUS_MCIP) ||
    >>> + !(vcpu->arch.cr4 & X86_CR4_MCE)) {
    >>> + printk(KERN_DEBUG "kvm: set_mce: "
    >>> + "injects mce exception while "
    >>> + "previous one is in progress!\n");
    >>> + set_bit(KVM_REQ_TRIPLE_FAULT, &vcpu->requests);
    >>> + return 0;
    >>> + }
    >>> + if (banks[1] & MCI_STATUS_VAL)
    >>> + status |= MCI_STATUS_OVER;
    >>> + banks[1] = mce->status;
    >>> + banks[2] = mce->addr;
    >>> + banks[3] = mce->misc;
    >>> + vcpu->arch.mcg_status = mce->mcg_status;
    >>> + kvm_queue_exception(vcpu, MC_VECTOR);
    >>> + } else if (!(banks[1] & MCI_STATUS_VAL) ||
    >>> + (!(banks[1] & MCI_STATUS_UC) &&
    >>> + !((mcg_cap & MCG_TES_P) && ((banks[1]>>53) & 0x3) < 2))) {
    >>> + u64 status = mce->status;
    >>> + if (banks[1] & MCI_STATUS_VAL)
    >>> + status |= MCI_STATUS_OVER;
    >>> + banks[1] = mce->status;
    >>> + banks[2] = mce->addr;
    >>> + banks[3] = mce->misc;
    >>> + } else
    >>> + banks[1] |= MCI_STATUS_OVER;
    >>> + return 0;
    >>> +}
    >>>
    >>>
    >> Can userspace just use KVM_SET_MSR for this?
    >>
    >
    > In addition to assigning MSR, we have some other logic for MCE, such as
    > BANK reporting disabling, overwriting rules, triple fault for UC MCE
    > during MCIP. So I think we need some dedicate interface.
    >

    Yes, you're right.

    >
    >>> + case KVM_X86_SETUP_MCE: {
    >>> + u64 mcg_cap;
    >>> +
    >>> + r = -EFAULT;
    >>> + if (copy_from_user(&mcg_cap, argp, sizeof mcg_cap))
    >>> + goto out;
    >>> + /*
    >>> + * extended machine-check state registers and CMCI are
    >>> + * not supported.
    >>> + */
    >>> + mcg_cap &= ~(MCG_EXT_P|MCG_CMCI_P);
    >>>
    >>>
    >> Instead of silently dropping, should return an error.
    >>
    >>
    >>> + if (copy_to_user(argp, &mcg_cap, sizeof mcg_cap))
    >>> + goto out;
    >>>
    >>>
    >> And not copy.
    >>
    >
    > This is designed as some kind of feature negotiating. Use space request
    > MCE features via mcg_cap, kernel space remove un-supported features and
    > return the resulting mcg_cap.
    >

    kvm does feature negotiation (really, feature advertising) using
    KVM_CAP_... and KVM_CHECK_EXTENSION. So there's no need for this.


    --
    I have a truly marvellous patch that fixes the bug which this
    signature is too narrow to contain.



    \
     
     \ /
      Last update: 2009-04-11 14:07    [W:2.131 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site