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