Re: [Xen-devel] [PATCH v2 1/5] x86/hvm: Handle viridian MSRs via the new guest_{rd, wr}msr() infrastructure
>>> On 13.03.18 at 16:47, wrote: > On 13/03/18 15:20, Jan Beulich wrote: > On 07.03.18 at 19:58, wrote: >>> @@ -175,11 +177,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, > uint64_t *val) >>> _MSR_MISC_FEATURES_CPUID_FAULTING; >>> break; >>> >>> +case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS >>> - > >>> 1: >>> +if ( is_viridian_domain(d) ) >>> +{ >>> +ret = guest_rdmsr_viridian(v, msr, val); >>> +goto out; >>> +} >>> + >>> +/* Fallthrough. */ >>> default: >>> return X86EMUL_UNHANDLEABLE; >>> } >>> >>> -return X86EMUL_OKAY; >>> + out: >> I've noticed this only in the context of patch 4, but why is this label >> and yet another unnecessary "goto" here? That "goto" could simply >> be "break" afaics. > > Ah - that is for changes which I haven't posted yet. > > When we get onto MSRs which might be in the load/save lists, or may be > stashed in the VMCB/VMCS rather than in real hardware, we need to call > back into arch specific code when an update is completed. But I don't think this requires a goto here, does it? If that future code structure really can't get away without, so be it (then). But please let's evaluate that at the time you have that code ready. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/5] x86/hvm: Handle viridian MSRs via the new guest_{rd, wr}msr() infrastructure
On 13/03/18 15:20, Jan Beulich wrote: On 07.03.18 at 19:58, wrote: >> @@ -175,11 +177,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, >> uint64_t *val) >> _MSR_MISC_FEATURES_CPUID_FAULTING; >> break; >> >> +case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - >> 1: >> +if ( is_viridian_domain(d) ) >> +{ >> +ret = guest_rdmsr_viridian(v, msr, val); >> +goto out; >> +} >> + >> +/* Fallthrough. */ >> default: >> return X86EMUL_UNHANDLEABLE; >> } >> >> -return X86EMUL_OKAY; >> + out: > I've noticed this only in the context of patch 4, but why is this label > and yet another unnecessary "goto" here? That "goto" could simply > be "break" afaics. Ah - that is for changes which I haven't posted yet. When we get onto MSRs which might be in the load/save lists, or may be stashed in the VMCB/VMCS rather than in real hardware, we need to call back into arch specific code when an update is completed. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/5] x86/hvm: Handle viridian MSRs via the new guest_{rd, wr}msr() infrastructure
>>> On 07.03.18 at 19:58, wrote: > @@ -175,11 +177,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, > uint64_t *val) > _MSR_MISC_FEATURES_CPUID_FAULTING; > break; > > +case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - > 1: > +if ( is_viridian_domain(d) ) > +{ > +ret = guest_rdmsr_viridian(v, msr, val); > +goto out; > +} > + > +/* Fallthrough. */ > default: > return X86EMUL_UNHANDLEABLE; > } > > -return X86EMUL_OKAY; > + out: I've noticed this only in the context of patch 4, but why is this label and yet another unnecessary "goto" here? That "goto" could simply be "break" afaics. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/5] x86/hvm: Handle viridian MSRs via the new guest_{rd, wr}msr() infrastructure
>>> On 07.03.18 at 19:58, wrote: > @@ -554,13 +551,11 @@ static void update_reference_tsc(struct domain *d, > bool_t initialize) > put_page_and_type(page); > } > > -int wrmsr_viridian_regs(uint32_t idx, uint64_t val) > +int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val) > { > -struct vcpu *v = current; > struct domain *d = v->domain; > > -if ( !is_viridian_domain(d) ) > -return 0; > +ASSERT(is_viridian_domain(d)); > > switch ( idx ) > { > @@ -615,7 +610,7 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val) > > case HV_X64_MSR_REFERENCE_TSC: > if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) ) > -return 0; > +goto gp_fault; > > perfc_incr(mshv_wrmsr_tsc_msr); > d->arch.hvm_domain.viridian.reference_tsc.raw = val; > @@ -655,14 +650,15 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val) > } > > default: > -if ( idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX ) > -gprintk(XENLOG_WARNING, "write to unimplemented MSR %#x\n", > -idx); > - > -return 0; > +gdprintk(XENLOG_WARNING, > + "Write %016"PRIx64" to unimplemented MSR %#x\n", val, idx); > +goto gp_fault; > } > > -return 1; > +return X86EMUL_OKAY; > + > + gp_fault: > +return X86EMUL_EXCEPTION; > } Still these ugly goto-s to just a single return statement. But well, Paul is happy with them ... > --- a/xen/arch/x86/msr.c > +++ b/xen/arch/x86/msr.c > @@ -141,9 +141,11 @@ int init_vcpu_msr_policy(struct vcpu *v) > > int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val) > { > -const struct cpuid_policy *cp = v->domain->arch.cpuid; > -const struct msr_domain_policy *dp = v->domain->arch.msr; > +const struct domain *d = v->domain; > +const struct cpuid_policy *cp = d->arch.cpuid; > +const struct msr_domain_policy *dp = d->arch.msr; > const struct msr_vcpu_policy *vp = v->arch.msr; > +int ret = X86EMUL_OKAY; > > switch ( msr ) > { > @@ -175,11 +177,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, > uint64_t *val) > _MSR_MISC_FEATURES_CPUID_FAULTING; > break; > > +case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - > 1: The "HYPERVISOR" in here starts to make sense in the next patch, but its combination with "VIRIDIAN" is still suspicious. I'll comment on this further for the next patch. > --- a/xen/include/asm-x86/msr-index.h > +++ b/xen/include/asm-x86/msr-index.h > @@ -540,4 +540,8 @@ > #define MSR_PKGC9_IRTL 0x0634 > #define MSR_PKGC10_IRTL 0x0635 > > +/* Hypervisor leaves in the 0x4xxx range. */ > +#define MSR_HYPERVISOR_START0x4000 > +#define NR_VIRIDIAN_MSRS0x0200 Is "leaves" really an appropriate term for MSRs? Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 1/5] x86/hvm: Handle viridian MSRs via the new guest_{rd, wr}msr() infrastructure
Dispatch from the guest_{rd,wr}msr() functions, after confirming that the domain is configured to use viridian. This allows for simplifiction of the viridian helpers as they don't need to cope with the "not a viridian MSR" case. It also means that viridian MSRs which are unimplemented, or excluded because of features, don't fall back into default handling path. Rename {rd,wr}msr_viridian_regs() to guest_{rd,wr}msr_viridian() for consistency, and because the _regs suffix isn't very appropriate. Update them to take a vcpu pointer rather than presuming that they act on current, which is safe for all implemented operations. Also update them to use X86EMUL_* return values. Signed-off-by: Andrew Cooper Reviewed-by: Boris Ostrovsky Reviewed-by: Kevin Tian Reviewed-by: Paul Durrant Reviewed-by: Roger Pau Monné --- CC: Jan Beulich CC: Wei Liu CC: Sergey Dyasli v2: * Introduce some constants for the hypervisor range. However, I do not think the current code is an improvement over bare numbers. --- xen/arch/x86/hvm/svm/svm.c | 6 + xen/arch/x86/hvm/viridian.c| 52 +- xen/arch/x86/hvm/vmx/vmx.c | 6 + xen/arch/x86/msr.c | 41 +++--- xen/include/asm-x86/hvm/viridian.h | 11 ++-- xen/include/asm-x86/msr-index.h| 4 +++ 6 files changed, 68 insertions(+), 52 deletions(-) diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index c34f5b5..e8ac2d8 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -1966,8 +1966,7 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) else if ( ret ) break; -if ( rdmsr_viridian_regs(msr, msr_content) || - rdmsr_hypervisor_regs(msr, msr_content) ) +if ( rdmsr_hypervisor_regs(msr, msr_content) ) break; if ( rdmsr_safe(msr, *msr_content) == 0 ) @@ -2122,9 +2121,6 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content) else if ( ret ) break; -if ( wrmsr_viridian_regs(msr, msr_content) ) -break; - switch ( wrmsr_hypervisor_regs(msr, msr_content) ) { case -ERESTART: diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c index 70aab52..074d920 100644 --- a/xen/arch/x86/hvm/viridian.c +++ b/xen/arch/x86/hvm/viridian.c @@ -88,9 +88,6 @@ #define HV_X64_MSR_CRASH_P4 0x4104 #define HV_X64_MSR_CRASH_CTL 0x4105 -#define VIRIDIAN_MSR_MIN HV_X64_MSR_GUEST_OS_ID -#define VIRIDIAN_MSR_MAX HV_X64_MSR_CRASH_CTL - /* Viridian Hypercall Status Codes. */ #define HV_STATUS_SUCCESS 0x #define HV_STATUS_INVALID_HYPERCALL_CODE0x0002 @@ -554,13 +551,11 @@ static void update_reference_tsc(struct domain *d, bool_t initialize) put_page_and_type(page); } -int wrmsr_viridian_regs(uint32_t idx, uint64_t val) +int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val) { -struct vcpu *v = current; struct domain *d = v->domain; -if ( !is_viridian_domain(d) ) -return 0; +ASSERT(is_viridian_domain(d)); switch ( idx ) { @@ -615,7 +610,7 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val) case HV_X64_MSR_REFERENCE_TSC: if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) ) -return 0; +goto gp_fault; perfc_incr(mshv_wrmsr_tsc_msr); d->arch.hvm_domain.viridian.reference_tsc.raw = val; @@ -655,14 +650,15 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val) } default: -if ( idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX ) -gprintk(XENLOG_WARNING, "write to unimplemented MSR %#x\n", -idx); - -return 0; +gdprintk(XENLOG_WARNING, + "Write %016"PRIx64" to unimplemented MSR %#x\n", val, idx); +goto gp_fault; } -return 1; +return X86EMUL_OKAY; + + gp_fault: +return X86EMUL_EXCEPTION; } static int64_t raw_trc_val(struct domain *d) @@ -698,13 +694,11 @@ void viridian_time_ref_count_thaw(struct domain *d) trc->off = (int64_t)trc->val - raw_trc_val(d); } -int rdmsr_viridian_regs(uint32_t idx, uint64_t *val) +int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val) { -struct vcpu *v = current; struct domain *d = v->domain; - -if ( !is_viridian_domain(d) ) -return 0; + +ASSERT(is_viridian_domain(d)); switch ( idx ) { @@ -725,7 +719,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val) case HV_X64_MSR_TSC_FREQUENCY: if ( viridian_feature_mask(d) & HVMPV_no_freq ) -return 0; +goto gp_fault; perfc_incr(mshv_rdmsr_tsc_frequency); *val = (uint64_t)d->arch.tsc_khz * 1000ull; @@ -733,7 +727,7 @@ int rdmsr_v