| Message ID | 20210722115245.16084-2-joro@8bytes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: SVM: Add initial GHCB protocol version 2 support | expand |
On Thu, Jul 22, 2021, Joerg Roedel wrote: > From: Joerg Roedel <jroedel@suse.de> > > Replace the get function with macros and the set function with > hypercall specific setters. This will avoid preserving any previous > bits in the GHCB-MSR and improved code readability. > > Suggested-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Joerg Roedel <jroedel@suse.de> > --- > arch/x86/include/asm/sev-common.h | 9 +++++++ > arch/x86/kvm/svm/sev.c | 41 +++++++++++-------------------- > 2 files changed, 24 insertions(+), 26 deletions(-) > > diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h > index 2cef6c5a52c2..8540972cad04 100644 > --- a/arch/x86/include/asm/sev-common.h > +++ b/arch/x86/include/asm/sev-common.h > @@ -50,6 +50,10 @@ > (GHCB_MSR_CPUID_REQ | \ > (((unsigned long)reg & GHCB_MSR_CPUID_REG_MASK) << GHCB_MSR_CPUID_REG_POS) | \ > (((unsigned long)fn) << GHCB_MSR_CPUID_FUNC_POS)) > +#define GHCB_MSR_CPUID_FN(msr) \ > + (((msr) >> GHCB_MSR_CPUID_FUNC_POS) & GHCB_MSR_CPUID_FUNC_MASK) > +#define GHCB_MSR_CPUID_REG(msr) \ > + (((msr) >> GHCB_MSR_CPUID_REG_POS) & GHCB_MSR_CPUID_REG_MASK) > > /* AP Reset Hold */ > #define GHCB_MSR_AP_RESET_HOLD_REQ 0x006 > @@ -67,6 +71,11 @@ > #define GHCB_SEV_TERM_REASON(reason_set, reason_val) \ > (((((u64)reason_set) & GHCB_MSR_TERM_REASON_SET_MASK) << GHCB_MSR_TERM_REASON_SET_POS) | \ > ((((u64)reason_val) & GHCB_MSR_TERM_REASON_MASK) << GHCB_MSR_TERM_REASON_POS)) > +#define GHCB_MSR_TERM_REASON_SET(msr) \ > + (((msr) >> GHCB_MSR_TERM_REASON_SET_POS) & GHCB_MSR_TERM_REASON_SET_MASK) > +#define GHCB_MSR_TERM_REASON(msr) \ > + (((msr) >> GHCB_MSR_TERM_REASON_POS) & GHCB_MSR_TERM_REASON_MASK) > + > > #define GHCB_SEV_ES_REASON_GENERAL_REQUEST 0 > #define GHCB_SEV_ES_REASON_PROTOCOL_UNSUPPORTED 1 > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 6710d9ee2e4b..d7b3557b8dbb 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -2342,16 +2342,15 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) > return true; > } > > -static void set_ghcb_msr_bits(struct vcpu_svm *svm, u64 value, u64 mask, > - unsigned int pos) > +static void set_ghcb_msr_cpuid_resp(struct vcpu_svm *svm, u64 reg, u64 value) > { > - svm->vmcb->control.ghcb_gpa &= ~(mask << pos); > - svm->vmcb->control.ghcb_gpa |= (value & mask) << pos; > -} > + u64 msr; > > -static u64 get_ghcb_msr_bits(struct vcpu_svm *svm, u64 mask, unsigned int pos) > -{ > - return (svm->vmcb->control.ghcb_gpa >> pos) & mask; > + msr = GHCB_MSR_CPUID_RESP; > + msr |= (reg & GHCB_MSR_CPUID_REG_MASK) << GHCB_MSR_CPUID_REG_POS; > + msr |= (value & GHCB_MSR_CPUID_VALUE_MASK) << GHCB_MSR_CPUID_VALUE_POS; > + > + svm->vmcb->control.ghcb_gpa = msr; I would rather have the get/set pairs be roughly symmetric, i.e. both functions or both macros, and both work on svm->vmcb->control.ghcb_gpa or both be purely functional (that may not be the correct word). I don't have a strong preference on function vs. macro. But for the second one, my preference would be to have the helper generate the value as opposed to taken and filling a pointer, e.g. to yield something like: cpuid_reg = GHCB_MSR_CPUID_REG(control->ghcb_gpa); if (cpuid_reg == 0) cpuid_value = vcpu->arch.regs[VCPU_REGS_RAX]; else if (cpuid_reg == 1) cpuid_value = vcpu->arch.regs[VCPU_REGS_RBX]; else if (cpuid_reg == 2) cpuid_value = vcpu->arch.regs[VCPU_REGS_RCX]; else cpuid_value = vcpu->arch.regs[VCPU_REGS_RDX]; control->ghcb_gpa = MAKE_GHCB_MSR_RESP(cpuid_reg, cpuid_value); The advantage is that it's obvious from the code that control->ghcb_gpa is being read _and_ written. > case GHCB_MSR_TERM_REQ: { > u64 reason_set, reason_code; > > - reason_set = get_ghcb_msr_bits(svm, > - GHCB_MSR_TERM_REASON_SET_MASK, > - GHCB_MSR_TERM_REASON_SET_POS); > - reason_code = get_ghcb_msr_bits(svm, > - GHCB_MSR_TERM_REASON_MASK, > - GHCB_MSR_TERM_REASON_POS); > + reason_set = GHCB_MSR_TERM_REASON_SET(control->ghcb_gpa); > + reason_code = GHCB_MSR_TERM_REASON(control->ghcb_gpa); > + > pr_info("SEV-ES guest requested termination: %#llx:%#llx\n", > reason_set, reason_code); > + > fallthrough; Not related to this patch, but why use fallthrough and more importantly, why is this an -EINVAL return? Why wouldn't KVM forward the request to userspace instead of returning an opaque -EINVAL? > } > default: > -- > 2.31.1 >
On Wed, Sep 01, 2021, Sean Christopherson wrote: > > -static u64 get_ghcb_msr_bits(struct vcpu_svm *svm, u64 mask, unsigned int pos) > > -{ > > - return (svm->vmcb->control.ghcb_gpa >> pos) & mask; > > + msr = GHCB_MSR_CPUID_RESP; > > + msr |= (reg & GHCB_MSR_CPUID_REG_MASK) << GHCB_MSR_CPUID_REG_POS; > > + msr |= (value & GHCB_MSR_CPUID_VALUE_MASK) << GHCB_MSR_CPUID_VALUE_POS; > > + > > + svm->vmcb->control.ghcb_gpa = msr; > > I would rather have the get/set pairs be roughly symmetric, i.e. both functions > or both macros, and both work on svm->vmcb->control.ghcb_gpa or both be purely > functional (that may not be the correct word). > > I don't have a strong preference on function vs. macro. But for the second one, > my preference would be to have the helper generate the value as opposed to taken > and filling a pointer, e.g. to yield something like: > > cpuid_reg = GHCB_MSR_CPUID_REG(control->ghcb_gpa); > > if (cpuid_reg == 0) > cpuid_value = vcpu->arch.regs[VCPU_REGS_RAX]; > else if (cpuid_reg == 1) > cpuid_value = vcpu->arch.regs[VCPU_REGS_RBX]; > else if (cpuid_reg == 2) > cpuid_value = vcpu->arch.regs[VCPU_REGS_RCX]; > else > cpuid_value = vcpu->arch.regs[VCPU_REGS_RDX]; > > control->ghcb_gpa = MAKE_GHCB_MSR_RESP(cpuid_reg, cpuid_value); > > > The advantage is that it's obvious from the code that control->ghcb_gpa is being > read _and_ written. Ah, but in the next path I see there's the existing ghcb_set_sw_exit_info_2(). Hrm. I think I still prefer open coding "control->ghcb_gpa = ..." with the right hand side being a macro. That would gel with the INFO_REQ, e.g. case GHCB_MSR_SEV_INFO_REQ: control->ghcb_gpa = GHCB_MSR_SEV_INFO(GHCB_VERSION_MAX, GHCB_VERSION_MIN, sev_enc_bit)); break; and drop set_ghcb_msr() altogether. Side topic, what about renaming control->ghcb_gpa => control->ghcb_msr so that the code for the MSR protocol is a bit more self-documenting? The APM defines the field as "Guest physical address of GHCB", so it's not exactly prescribing a specific name.
Hi Sean, On Wed, Sep 01, 2021 at 09:31:52PM +0000, Sean Christopherson wrote: > On Wed, Sep 01, 2021, Sean Christopherson wrote: > > control->ghcb_gpa = MAKE_GHCB_MSR_RESP(cpuid_reg, cpuid_value); Made that change, but kept the set_ghcb_msr_cpuid_resp() and renamed it to ghcb_msr_cpuid_resp(). It now returns the MSR value for the CPUID response. I like the keep the more complicated response setters as functions and not macros for readability. > case GHCB_MSR_SEV_INFO_REQ: > control->ghcb_gpa = GHCB_MSR_SEV_INFO(GHCB_VERSION_MAX, > GHCB_VERSION_MIN, > sev_enc_bit)); > break; > > and drop set_ghcb_msr() altogether. Makes sense, I replaced the set_ghcb_msr() calls with the above. > Side topic, what about renaming control->ghcb_gpa => control->ghcb_msr so that > the code for the MSR protocol is a bit more self-documenting? The APM defines > the field as "Guest physical address of GHCB", so it's not exactly prescribing a > specific name. No strong opinion here, I let this up to the AMD engineers to decide. If we change the name I can add a separate patch for this. Regards, Joerg
On Wed, Sep 01, 2021 at 09:12:10PM +0000, Sean Christopherson wrote: > On Thu, Jul 22, 2021, Joerg Roedel wrote: > > case GHCB_MSR_TERM_REQ: { > > u64 reason_set, reason_code; > > > > - reason_set = get_ghcb_msr_bits(svm, > > - GHCB_MSR_TERM_REASON_SET_MASK, > > - GHCB_MSR_TERM_REASON_SET_POS); > > - reason_code = get_ghcb_msr_bits(svm, > > - GHCB_MSR_TERM_REASON_MASK, > > - GHCB_MSR_TERM_REASON_POS); > > + reason_set = GHCB_MSR_TERM_REASON_SET(control->ghcb_gpa); > > + reason_code = GHCB_MSR_TERM_REASON(control->ghcb_gpa); > > + > > pr_info("SEV-ES guest requested termination: %#llx:%#llx\n", > > reason_set, reason_code); > > + > > fallthrough; > > Not related to this patch, but why use fallthrough and more importantly, why is > this an -EINVAL return? Why wouldn't KVM forward the request to userspace instead > of returning an opaque -EINVAL? I guess it is to signal an error condition up the call-chain to get the guest terminated, like requested. Regards, Joerg
On Thu, Sep 09, 2021, Joerg Roedel wrote: > On Wed, Sep 01, 2021 at 09:12:10PM +0000, Sean Christopherson wrote: > > On Thu, Jul 22, 2021, Joerg Roedel wrote: > > > case GHCB_MSR_TERM_REQ: { > > > u64 reason_set, reason_code; > > > > > > - reason_set = get_ghcb_msr_bits(svm, > > > - GHCB_MSR_TERM_REASON_SET_MASK, > > > - GHCB_MSR_TERM_REASON_SET_POS); > > > - reason_code = get_ghcb_msr_bits(svm, > > > - GHCB_MSR_TERM_REASON_MASK, > > > - GHCB_MSR_TERM_REASON_POS); > > > + reason_set = GHCB_MSR_TERM_REASON_SET(control->ghcb_gpa); > > > + reason_code = GHCB_MSR_TERM_REASON(control->ghcb_gpa); > > > + > > > pr_info("SEV-ES guest requested termination: %#llx:%#llx\n", > > > reason_set, reason_code); > > > + > > > fallthrough; > > > > Not related to this patch, but why use fallthrough and more importantly, why is > > this an -EINVAL return? Why wouldn't KVM forward the request to userspace instead > > of returning an opaque -EINVAL? > > I guess it is to signal an error condition up the call-chain to get the > guest terminated, like requested. Yes, but it's odd bizarre/unfortunate that KVM doesn't take this opportunity to forward the termination info to the VMM. The above pr_info() should not exist. If that information is relevant then it should be handed to the VMM directly, not dumped to dmesg.
diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h index 2cef6c5a52c2..8540972cad04 100644 --- a/arch/x86/include/asm/sev-common.h +++ b/arch/x86/include/asm/sev-common.h @@ -50,6 +50,10 @@ (GHCB_MSR_CPUID_REQ | \ (((unsigned long)reg & GHCB_MSR_CPUID_REG_MASK) << GHCB_MSR_CPUID_REG_POS) | \ (((unsigned long)fn) << GHCB_MSR_CPUID_FUNC_POS)) +#define GHCB_MSR_CPUID_FN(msr) \ + (((msr) >> GHCB_MSR_CPUID_FUNC_POS) & GHCB_MSR_CPUID_FUNC_MASK) +#define GHCB_MSR_CPUID_REG(msr) \ + (((msr) >> GHCB_MSR_CPUID_REG_POS) & GHCB_MSR_CPUID_REG_MASK) /* AP Reset Hold */ #define GHCB_MSR_AP_RESET_HOLD_REQ 0x006 @@ -67,6 +71,11 @@ #define GHCB_SEV_TERM_REASON(reason_set, reason_val) \ (((((u64)reason_set) & GHCB_MSR_TERM_REASON_SET_MASK) << GHCB_MSR_TERM_REASON_SET_POS) | \ ((((u64)reason_val) & GHCB_MSR_TERM_REASON_MASK) << GHCB_MSR_TERM_REASON_POS)) +#define GHCB_MSR_TERM_REASON_SET(msr) \ + (((msr) >> GHCB_MSR_TERM_REASON_SET_POS) & GHCB_MSR_TERM_REASON_SET_MASK) +#define GHCB_MSR_TERM_REASON(msr) \ + (((msr) >> GHCB_MSR_TERM_REASON_POS) & GHCB_MSR_TERM_REASON_MASK) + #define GHCB_SEV_ES_REASON_GENERAL_REQUEST 0 #define GHCB_SEV_ES_REASON_PROTOCOL_UNSUPPORTED 1 diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 6710d9ee2e4b..d7b3557b8dbb 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -2342,16 +2342,15 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) return true; } -static void set_ghcb_msr_bits(struct vcpu_svm *svm, u64 value, u64 mask, - unsigned int pos) +static void set_ghcb_msr_cpuid_resp(struct vcpu_svm *svm, u64 reg, u64 value) { - svm->vmcb->control.ghcb_gpa &= ~(mask << pos); - svm->vmcb->control.ghcb_gpa |= (value & mask) << pos; -} + u64 msr; -static u64 get_ghcb_msr_bits(struct vcpu_svm *svm, u64 mask, unsigned int pos) -{ - return (svm->vmcb->control.ghcb_gpa >> pos) & mask; + msr = GHCB_MSR_CPUID_RESP; + msr |= (reg & GHCB_MSR_CPUID_REG_MASK) << GHCB_MSR_CPUID_REG_POS; + msr |= (value & GHCB_MSR_CPUID_VALUE_MASK) << GHCB_MSR_CPUID_VALUE_POS; + + svm->vmcb->control.ghcb_gpa = msr; } static void set_ghcb_msr(struct vcpu_svm *svm, u64 value) @@ -2380,9 +2379,7 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm) case GHCB_MSR_CPUID_REQ: { u64 cpuid_fn, cpuid_reg, cpuid_value; - cpuid_fn = get_ghcb_msr_bits(svm, - GHCB_MSR_CPUID_FUNC_MASK, - GHCB_MSR_CPUID_FUNC_POS); + cpuid_fn = GHCB_MSR_CPUID_FN(control->ghcb_gpa); /* Initialize the registers needed by the CPUID intercept */ vcpu->arch.regs[VCPU_REGS_RAX] = cpuid_fn; @@ -2394,9 +2391,8 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm) break; } - cpuid_reg = get_ghcb_msr_bits(svm, - GHCB_MSR_CPUID_REG_MASK, - GHCB_MSR_CPUID_REG_POS); + cpuid_reg = GHCB_MSR_CPUID_REG(control->ghcb_gpa); + if (cpuid_reg == 0) cpuid_value = vcpu->arch.regs[VCPU_REGS_RAX]; else if (cpuid_reg == 1) @@ -2406,26 +2402,19 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm) else cpuid_value = vcpu->arch.regs[VCPU_REGS_RDX]; - set_ghcb_msr_bits(svm, cpuid_value, - GHCB_MSR_CPUID_VALUE_MASK, - GHCB_MSR_CPUID_VALUE_POS); + set_ghcb_msr_cpuid_resp(svm, cpuid_reg, cpuid_value); - set_ghcb_msr_bits(svm, GHCB_MSR_CPUID_RESP, - GHCB_MSR_INFO_MASK, - GHCB_MSR_INFO_POS); break; } case GHCB_MSR_TERM_REQ: { u64 reason_set, reason_code; - reason_set = get_ghcb_msr_bits(svm, - GHCB_MSR_TERM_REASON_SET_MASK, - GHCB_MSR_TERM_REASON_SET_POS); - reason_code = get_ghcb_msr_bits(svm, - GHCB_MSR_TERM_REASON_MASK, - GHCB_MSR_TERM_REASON_POS); + reason_set = GHCB_MSR_TERM_REASON_SET(control->ghcb_gpa); + reason_code = GHCB_MSR_TERM_REASON(control->ghcb_gpa); + pr_info("SEV-ES guest requested termination: %#llx:%#llx\n", reason_set, reason_code); + fallthrough; } default: