lkml.org 
[lkml]   [2024]   [Apr]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v19 111/130] KVM: TDX: Implement callbacks for MSR operations for TDX
    On Wed, Apr 03, 2024 at 08:14:04AM -0700,
    Sean Christopherson <seanjc@google.com> wrote:

    > On Mon, Feb 26, 2024, isaku.yamahata@intel.com wrote:
    > > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
    > > index 389bb95d2af0..c8f991b69720 100644
    > > --- a/arch/x86/kvm/vmx/tdx.c
    > > +++ b/arch/x86/kvm/vmx/tdx.c
    > > @@ -1877,6 +1877,76 @@ void tdx_get_exit_info(struct kvm_vcpu *vcpu, u32 *reason,
    > > *error_code = 0;
    > > }
    > >
    > > +static bool tdx_is_emulated_kvm_msr(u32 index, bool write)
    > > +{
    > > + switch (index) {
    > > + case MSR_KVM_POLL_CONTROL:
    > > + return true;
    > > + default:
    > > + return false;
    > > + }
    > > +}
    > > +
    > > +bool tdx_has_emulated_msr(u32 index, bool write)
    > > +{
    > > + switch (index) {
    > > + case MSR_IA32_UCODE_REV:
    > > + case MSR_IA32_ARCH_CAPABILITIES:
    > > + case MSR_IA32_POWER_CTL:
    > > + case MSR_IA32_CR_PAT:
    > > + case MSR_IA32_TSC_DEADLINE:
    > > + case MSR_IA32_MISC_ENABLE:
    > > + case MSR_PLATFORM_INFO:
    > > + case MSR_MISC_FEATURES_ENABLES:
    > > + case MSR_IA32_MCG_CAP:
    > > + case MSR_IA32_MCG_STATUS:
    > > + case MSR_IA32_MCG_CTL:
    > > + case MSR_IA32_MCG_EXT_CTL:
    > > + case MSR_IA32_MC0_CTL ... MSR_IA32_MCx_CTL(KVM_MAX_MCE_BANKS) - 1:
    > > + case MSR_IA32_MC0_CTL2 ... MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) - 1:
    > > + /* MSR_IA32_MCx_{CTL, STATUS, ADDR, MISC, CTL2} */
    > > + return true;
    > > + case APIC_BASE_MSR ... APIC_BASE_MSR + 0xff:
    > > + /*
    > > + * x2APIC registers that are virtualized by the CPU can't be
    > > + * emulated, KVM doesn't have access to the virtual APIC page.
    > > + */
    > > + switch (index) {
    > > + case X2APIC_MSR(APIC_TASKPRI):
    > > + case X2APIC_MSR(APIC_PROCPRI):
    > > + case X2APIC_MSR(APIC_EOI):
    > > + case X2APIC_MSR(APIC_ISR) ... X2APIC_MSR(APIC_ISR + APIC_ISR_NR):
    > > + case X2APIC_MSR(APIC_TMR) ... X2APIC_MSR(APIC_TMR + APIC_ISR_NR):
    > > + case X2APIC_MSR(APIC_IRR) ... X2APIC_MSR(APIC_IRR + APIC_ISR_NR):
    > > + return false;
    > > + default:
    > > + return true;
    > > + }
    > > + case MSR_IA32_APICBASE:
    > > + case MSR_EFER:
    > > + return !write;
    >
    > Meh, for literally two MSRs, just open code them in tdx_set_msr() and drop the
    > @write param. Or alternatively add:
    >
    > static bool tdx_is_read_only_msr(u32 msr){
    > {
    > return msr == MSR_IA32_APICBASE || msr == MSR_EFER;
    > }

    Sure will add.

    >
    > > + case 0x4b564d00 ... 0x4b564dff:
    >
    > This is silly, just do
    >
    > case MSR_KVM_POLL_CONTROL:
    > return false;
    >
    > and let everything else go through the default statement, no?

    Now tdx_is_emulated_kvm_msr() is trivial, will open code it.


    > > + /* KVM custom MSRs */
    > > + return tdx_is_emulated_kvm_msr(index, write);
    > > + default:
    > > + return false;
    > > + }
    > > +}
    > > +
    > > +int tdx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
    > > +{
    > > + if (tdx_has_emulated_msr(msr->index, false))
    > > + return kvm_get_msr_common(vcpu, msr);
    > > + return 1;
    >
    > Please invert these and make the happy path the not-taken path, i.e.
    >
    > if (!tdx_has_emulated_msr(msr->index))
    > return 1;
    >
    > return kvm_get_msr_common(vcpu, msr);
    >
    > The standard kernel pattern is
    >
    > if (error)
    > return <error thingie>
    >
    > return <happy thingie>
    >
    > > +}
    > > +
    > > +int tdx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
    > > +{
    > > + if (tdx_has_emulated_msr(msr->index, true))
    >
    > As above:
    >
    > if (tdx_is_read_only_msr(msr->index))
    > return 1;
    >
    > if (!tdx_has_emulated_msr(msr->index))
    > return 1;
    >
    > return kvm_set_msr_common(vcpu, msr);

    Sure, will update them.


    > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
    > > index d5b18cad9dcd..0e1d3853eeb4 100644
    > > --- a/arch/x86/kvm/x86.c
    > > +++ b/arch/x86/kvm/x86.c
    > > @@ -90,7 +90,6 @@
    > > #include "trace.h"
    > >
    > > #define MAX_IO_MSRS 256
    > > -#define KVM_MAX_MCE_BANKS 32
    > >
    > > struct kvm_caps kvm_caps __read_mostly = {
    > > .supported_mce_cap = MCG_CTL_P | MCG_SER_P,
    > > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
    > > index 4e40c23d66ed..c87b7a777b67 100644
    > > --- a/arch/x86/kvm/x86.h
    > > +++ b/arch/x86/kvm/x86.h
    > > @@ -9,6 +9,8 @@
    > > #include "kvm_cache_regs.h"
    > > #include "kvm_emulate.h"
    > >
    > > +#define KVM_MAX_MCE_BANKS 32
    >
    > Split this to a separate. Yes, it's trivial, but that's _exactly_ why it should
    > be in a separate patch. The more trivial refactoring you split out, the more we
    > can apply _now_ and take off your hands.

    Will split it.
    --
    Isaku Yamahata <isaku.yamahata@intel.com>

    \
     
     \ /
      Last update: 2024-05-27 16:24    [W:4.209 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site