| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <49E08762.1010206@redhat.com>
Date: Sat, 11 Apr 2009 15:04:50 +0300
From: Avi Kivity <avi@...hat.com>
To: Huang Ying <ying.huang@...el.com>
CC: "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Andi Kleen <andi@...stfloor.org>
Subject: Re: [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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists