|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] x86/AMD: Add support for AMD's OSVW feature in guests
>>> On 16.01.12 at 22:11, Boris Ostrovsky <boris.ostrovsky@xxxxxxx> wrote:
> --- a/tools/libxc/xc_cpuid_x86.c Mon Jan 09 16:01:44 2012 +0100
> +++ b/tools/libxc/xc_cpuid_x86.c Mon Jan 16 22:08:09 2012 +0100
> @@ -108,6 +108,7 @@ static void amd_xc_cpuid_policy(
> bitmaskof(X86_FEATURE_SSE4A) |
> bitmaskof(X86_FEATURE_MISALIGNSSE) |
> bitmaskof(X86_FEATURE_3DNOWPREFETCH) |
> + bitmaskof(X86_FEATURE_OSVW) |
Indentation.
Also, is this really meaningful to PV guests? And valid for pre-Fam10?
> bitmaskof(X86_FEATURE_XOP) |
> bitmaskof(X86_FEATURE_FMA4) |
> bitmaskof(X86_FEATURE_TBM) |
> --- a/xen/arch/x86/cpu/amd.c Mon Jan 09 16:01:44 2012 +0100
> +++ b/xen/arch/x86/cpu/amd.c Mon Jan 16 22:08:09 2012 +0100
> @@ -32,6 +32,13 @@
> static char opt_famrev[14];
> string_param("cpuid_mask_cpu", opt_famrev);
>
> +/*
> + * Set osvw_len to higher number when updated Revision Guides
> + * are published and we know what the new status bits are
> + */
This is ugly, as it requires permanent attention. The value to start
with should really be 64 (beyond which other code adjustments are
going to be necessary anyway).
> +static uint64_t osvw_length = 4, osvw_status;
> +static DEFINE_SPINLOCK(amd_lock);
> +
> static inline void wrmsr_amd(unsigned int index, unsigned int lo,
> unsigned int hi)
> {
> @@ -182,6 +189,35 @@ static void __devinit set_cpuidmask(cons
> }
> }
>
> +static void amd_guest_osvw_init(struct vcpu *vcpu)
> +{
> + if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> + return;
Shouldn't we bail here for pre-Fam10 CPUs too?
Further, as asked above already - is all this really meaningful to PV
guests? Otherwise the code would better go somewhere under
xen/arch/x86/hvm/svm/ ...
Also, throughout this file indentation needs to be changed to Linux
style (tabs instead of spaces), unless you want to contribute a patch
to convert the whole file to Xen style (in which case you'd still need
to make adjustments here).
> +
> + /*
> + * Guests should see errata 400 and 415 as fixed (assuming that
> + * HLT and IO instructions are intercepted).
> + */
> + vcpu->arch.amd.osvw.length = (osvw_length >= 3) ? (osvw_length) : 3;
An expression consisting of just an identifier for sure doesn't need
parentheses.
> + vcpu->arch.amd.osvw.status = osvw_status & ~(6ULL);
> +
> + /*
> + * By increasing VCPU's osvw.length to 3 we are telling the guest that
> + * all osvw.status bits inside that length, including bit 0 (which is
> + * reserved for erratum 298), are valid. However, if host processor's
> + * osvw_len is 0 then osvw_status[0] carries no information. We need to
> + * be conservative here and therefore we tell the guest that erratum 298
> + * is present (because we really don't know).
> + */
> + if (osvw_length == 0 && boot_cpu_data.x86 == 0x10)
Why do you check the family here? If osvw_length can only ever be
zero on Fam10, then the first check is sufficient. If osvw_length can
be zero on other than Fam10 (no matter whether we bail early older
CPUs), then the check is actually wrong.
> + vcpu->arch.amd.osvw.status |= 1;
> +}
> +
> +void amd_vcpu_initialise(struct vcpu *vcpu)
> +{
> + amd_guest_osvw_init(vcpu);
> +}
> +
> /*
> * Check for the presence of an AMD erratum. Arguments are defined in amd.h
>
> * for each known erratum. Return 1 if erratum is found.
> @@ -512,6 +548,30 @@ static void __devinit init_amd(struct cp
> set_cpuidmask(c);
>
> check_syscfg_dram_mod_en();
> +
> + /*
> + * Get OSVW bits. If bits are not the same on different processors then
> + * choose the worst case (i.e. if erratum is present on one processor and
> + * not on another assume that the erratum is present everywhere).
> + */
> + if (test_bit(X86_FEATURE_OSVW, &boot_cpu_data.x86_capability)) {
> + uint64_t len, status;
> +
> + if (rdmsr_safe(MSR_AMD_OSVW_ID_LENGTH, len) ||
> + rdmsr_safe(MSR_AMD_OSVW_STATUS, status))
> + len = status = 0;
> +
> + spin_lock(&amd_lock);
What is the locking here supposed to protect against? The AP call tree
is smp_callin() -> identify_cpu() -> init_amd(), which cannot race with
anything (as the initiating processor is waiting for cpu_state to change
to CPU_STATE_CALLIN).
> +
> + if (len < osvw_length)
> + osvw_length = len;
> +
> + osvw_status |= status;
> + osvw_status &= (1ULL << osvw_length) - 1;
> +
> + spin_unlock(&amd_lock);
> + } else
> + osvw_length = osvw_status = 0;
> }
>
> static struct cpu_dev amd_cpu_dev __cpuinitdata = {
> --- a/xen/arch/x86/domain.c Mon Jan 09 16:01:44 2012 +0100
> +++ b/xen/arch/x86/domain.c Mon Jan 16 22:08:09 2012 +0100
> @@ -422,6 +422,10 @@ int vcpu_initialise(struct vcpu *v)
> if ( (rc = vcpu_init_fpu(v)) != 0 )
> return rc;
>
> + /* Vendor-specific initialization */
> + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
If you check the vendor here, you don't need to anywhere in the
descendant functions.
> + amd_vcpu_initialise(v);
> +
> if ( is_hvm_domain(d) )
> {
> rc = hvm_vcpu_initialise(v);
> --- a/xen/arch/x86/hvm/svm/svm.c Mon Jan 09 16:01:44 2012 +0100
> +++ b/xen/arch/x86/hvm/svm/svm.c Mon Jan 16 22:08:09 2012 +0100
> @@ -1044,6 +1044,27 @@ static void svm_init_erratum_383(struct
> }
> }
>
> +static int svm_handle_osvw(struct vcpu *v, uint32_t msr, uint64_t *val, uint
> read)
> +{
> + uint eax, ebx, ecx, edx;
> +
> + /* Guest OSVW support */
> + hvm_cpuid(0x80000001, &eax, &ebx, &ecx, &edx);
> + if (!test_bit((X86_FEATURE_OSVW & 31), &ecx))
Formatting of changes to this file should be changed to Xen style.
> + return -1;
> +
> + if (read) {
> + if (msr == MSR_AMD_OSVW_ID_LENGTH)
> + *val = v->arch.amd.osvw.length;
> + else
> + *val = v->arch.amd.osvw.status;
> + }
> + /* Writes are ignored */
> +
> + return 0;
> +}
> +
> +
> static int svm_cpu_up(void)
> {
> uint64_t msr_content;
> --- a/xen/arch/x86/traps.c Mon Jan 09 16:01:44 2012 +0100
> +++ b/xen/arch/x86/traps.c Mon Jan 16 22:08:09 2012 +0100
> @@ -2542,6 +2542,15 @@ static int emulate_privileged_op(struct
> if ( wrmsr_safe(regs->ecx, msr_content) != 0 )
> goto fail;
> break;
> + case MSR_AMD_OSVW_ID_LENGTH:
> + case MSR_AMD_OSVW_STATUS:
> + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) {
> + if (!boot_cpu_has(X86_FEATURE_OSVW))
> + goto fail;
> + else
Bogus "else" after a "goto". And I wonder, provided there is some
point in doing all this for PV guests in the first place, whether this
shouldn't be kept getting handled by the default case.
> + break; /* Writes are ignored */
> + }
> + /* Fall through to default case */
> default:
> if ( wrmsr_hypervisor_regs(regs->ecx, msr_content) )
> break;
> @@ -2573,6 +2582,7 @@ static int emulate_privileged_op(struct
> break;
>
> case 0x32: /* RDMSR */
> +
Stray addition of a newline.
> switch ( (u32)regs->ecx )
> {
> #ifdef CONFIG_X86_64
> @@ -2632,6 +2642,23 @@ static int emulate_privileged_op(struct
> regs->eax = (uint32_t)msr_content;
> regs->edx = (uint32_t)(msr_content >> 32);
> break;
> + case MSR_AMD_OSVW_ID_LENGTH:
> + case MSR_AMD_OSVW_STATUS:
> + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
> + if (!boot_cpu_has(X86_FEATURE_OSVW))
> + goto fail;
> + else {
Bogus "else" after a "goto".
Jan
> + if ((u32)regs->ecx == MSR_AMD_OSVW_ID_LENGTH)
> + msr_content = v->arch.amd.osvw.length;
> + else
> + msr_content = v->arch.amd.osvw.status;
> +
> + regs->eax = (uint32_t)msr_content;
> + regs->edx = (uint32_t)(msr_content >> 32);
> + }
> + } else
> + goto rdmsr_normal;
> + break;
> default:
> if ( rdmsr_hypervisor_regs(regs->ecx, &val) )
> {
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |