[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 02/27] x86/cpuid: Introduce guest_cpuid() and struct cpuid_leaf
On 04/01/17 14:01, Jan Beulich wrote: > >> @@ -17,6 +18,8 @@ uint32_t __read_mostly raw_featureset[FSCAPINTS]; >> uint32_t __read_mostly pv_featureset[FSCAPINTS]; >> uint32_t __read_mostly hvm_featureset[FSCAPINTS]; >> >> +#define EMPTY_LEAF (struct cpuid_leaf){} > Perhaps another pair of parentheses around the entire thing? Can do. > >> @@ -215,6 +218,36 @@ const uint32_t * __init lookup_deep_deps(uint32_t >> feature) >> return NULL; >> } >> >> +void guest_cpuid(const struct vcpu *v, unsigned int leaf, >> + unsigned int subleaf, struct cpuid_leaf *res) >> +{ >> + *res = EMPTY_LEAF; > Why? There's no valid path leaving the structure uninitialized. Paths are introduced later in the series. > >> + /* {pv,hvm}_cpuid() have this expectation. */ >> + ASSERT(v == current); >> + >> + if ( is_pv_vcpu(v) || is_pvh_vcpu(v) ) >> + { >> + struct cpu_user_regs regs = *guest_cpu_user_regs(); > I assume this is only a transient thing, in which case I'm fine with > this relatively big item getting placed on the stack. Yes. Removed in patch 27. > >> + regs.rax = leaf; >> + regs.rcx = subleaf; > DYM _eax/_ecx respectively? The upper halves are of no interest. Mainly for my peace of mind. This, like the above, is only transient. > >> @@ -3246,10 +3252,10 @@ static int priv_op_wbinvd(struct x86_emulate_ctxt >> *ctxt) >> return X86EMUL_OKAY; >> } >> >> -int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx, >> - unsigned int *edx, struct x86_emulate_ctxt *ctxt) >> +int pv_emul_cpuid(unsigned int leaf, unsigned int subleaf, >> + struct cpuid_leaf *res, struct x86_emulate_ctxt *ctxt) >> { >> - struct cpu_user_regs regs = *ctxt->regs; >> + struct vcpu *curr = current; >> >> /* >> * x86_emulate uses this function to query CPU features for its own >> @@ -3258,7 +3264,6 @@ int pv_emul_cpuid(unsigned int *eax, unsigned int >> *ebx, unsigned int *ecx, >> */ >> if ( ctxt->opcode == X86EMUL_OPC(0x0f, 0xa2) ) >> { >> - const struct vcpu *curr = current; > There was a "const" here - did you really mean to get rid of it? No. Will fix. > >> @@ -3266,16 +3271,7 @@ int pv_emul_cpuid(unsigned int *eax, unsigned int >> *ebx, unsigned int *ecx, >> return X86EMUL_EXCEPTION; >> } >> >> - regs._eax = *eax; >> - regs._ecx = *ecx; >> - >> - pv_cpuid(®s); >> - >> - *eax = regs._eax; >> - *ebx = regs._ebx; >> - *ecx = regs._ecx; >> - *edx = regs._edx; >> - >> + guest_cpuid(curr, leaf, subleaf, res); >> return X86EMUL_OKAY; >> } > Please retain the blank line before the return statement. Ok. > >> @@ -4502,15 +4502,15 @@ x86_emulate( >> >> case 0xfc: /* clzero */ >> { >> - unsigned int eax = 1, ebx = 0, dummy = 0; >> + struct cpuid_leaf res; > Please put a single instance of this at the top of the body of the giant > switch() statement (likely calling for it to be named other than "res"). struct cpuid_leaf cpuid_leaf? I can't think of anything clearer. > >> --- a/xen/arch/x86/x86_emulate/x86_emulate.h >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h >> @@ -164,6 +164,11 @@ enum x86_emulate_fpu_type { >> X86EMUL_FPU_ymm /* AVX/XOP instruction set (%ymm0-%ymm7/15) */ >> }; >> >> +struct cpuid_leaf >> +{ >> + uint32_t a, b, c, d; > Could you please consistently use uint32_t or unsigned int between > here and ... > >> @@ -415,10 +420,9 @@ struct x86_emulate_ops >> * #GP[0]. Used to implement CPUID faulting. >> */ >> int (*cpuid)( >> - unsigned int *eax, >> - unsigned int *ebx, >> - unsigned int *ecx, >> - unsigned int *edx, >> + unsigned int leaf, >> + unsigned int subleaf, >> + struct cpuid_leaf *res, > ... here? I have no particular preference which of the two to use. Will use uint32_t. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |