linux-kernel - Re: [PATCH v2] KVM: SVM: Add support for AMD's OSVW feature in guests

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20120106104909.GB23932@amt.cnet>
Date:	Fri, 6 Jan 2012 08:49:09 -0200
From:	Marcelo Tosatti <mtosatti@...hat.com>
To:	Boris Ostrovsky <boris.ostrovsky@....com>
Cc:	Boris Ostrovsky <ostr@...64.org>, avi@...hat.com,
	joerg.roedel@....com, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] KVM: SVM: Add support for AMD's OSVW feature in guests

On Thu, Jan 05, 2012 at 01:37:36PM -0500, Boris Ostrovsky wrote:
> On 01/05/12 06:20, Marcelo Tosatti wrote:
> >On Tue, Jan 03, 2012 at 11:38:13PM -0500, Boris Ostrovsky wrote:
> >>
> >>diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> >>index e32243e..b19769d 100644
> >>--- a/arch/x86/kvm/svm.c
> >>+++ b/arch/x86/kvm/svm.c
> >>@@ -110,6 +110,13 @@ struct nested_state {
> >>  #define MSRPM_OFFSETS	16
> >>  static u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
> >>
> >>+/*
> >>+ * Set osvw_len to higher value when updated Revision Guides
> >>+ * are published and we know what the new status bits are
> >>+ */
> >>+static uint64_t osvw_len = 4, osvw_status;
> >>+static DEFINE_SPINLOCK(svm_lock);
> >
> >No need for this lock, operation already serialized by kvm_lock.
> 
> Will remove the lock.
> 
> >
> >>  struct vcpu_svm {
> >>  	struct kvm_vcpu vcpu;
> >>  	struct vmcb *vmcb;
> >>@@ -556,6 +563,20 @@ static void svm_init_erratum_383(void)
> >>  	erratum_383_found = true;
> >>  }
> >>
> >>+static void svm_init_osvw(struct kvm_vcpu *vcpu)
> >>+{
> >>+	/*
> >>+	 * Guests should see errata 400 and 415 as fixed (assuming that
> >>+	 * HLT and IO instructions are intercepted).
> >>+	 */
> >>+	vcpu->arch.osvw.length = (osvw_len>= 3) ? (osvw_len) : 3;
> >>+	vcpu->arch.osvw.status = osvw_status&  ~(6ULL);
> >
> >Do you really want to expose the hosts osvw_status and osvw_len? If
> >only exposure of 400 and 415 as fixed is necessary, then dealing with
> >migration is simpler (that is, you control what workarounds are applied
> >in the guest instead of making it dependent on particular hosts).
> 
> I do think we should (selectively) expose osvw information to the
> host. As of today we have 4 errata described by these bits. Two of
> them (400 and 415) don't need to be seen by the guest and the patch
> would mask them off. As for the other two (errata 383 and 298) ---
> the guest should be aware of them and the patch passes them through.
> 
> Since osvw_len is initialized to 4 and cannot become larger no other
> bits will be passed to guests until we update the initial value (by
> a future patch).

OK.

> If we are concerned about migration we can always ovewrite
> vcpu->arch.osvw registers from userspace when creating a guest.
> 
> >
> >>+	/* Mark erratum 298 as present */
> >>+	if (osvw_len == 0&&  boot_cpu_data.x86 == 0x10)
> >>+		vcpu->arch.osvw.status |= 1;
> >>+}
> >
> >Why is it necessary to explicitly do this? Please add documentation.
> 
> That's because we may have bumped vcpu->arch.osvw.length to 3 in
> order to signal the guest that 400 and 415 are fixed. By doing this
> we are also telling the guest that it can rely on state of bit0.
> 
> If we leave bit0 as 0 the guest will assume that 298 is fixed.
> However, if host's osvw_length is 0 it means that the physical HW
> *may* still be affected. So we take conservative approach and tell
> the guest that it should work around 298.
> 
> I'll add a comment to that effect.

OK thanks.

> >
> >>+	case MSR_AMD64_OSVW_ID_LENGTH:
> >>+		if (!guest_cpuid_has_osvw(vcpu))
> >>+			return 1;
> >>+		vcpu->arch.osvw.length = data;
> >>+		break;
> >>+	case MSR_AMD64_OSVW_STATUS:
> >>+		if (!guest_cpuid_has_osvw(vcpu))
> >>+			return 1;
> >>+		vcpu->arch.osvw.status = data;
> >>+		break;
> >
> >If writes are allowed, it is necessary to migrate this.
> 
> Not sure I understand what you mean here. Isn't vcpu->arch state
> migrated with the guest?

Since the MSR value is setup in the kernel, there is no need to migrate
it, actually.
--
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/