Received-SPF: pass (google.com: domain of linux-kernel+bounces-150180-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <4ae8893f-b3dc-4768-887a-b6a680654479@linux.intel.com>
Date: Thu, 18 Apr 2024 21:36:35 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 111/130] KVM: TDX: Implement callbacks for MSR
 operations for TDX
To: Isaku Yamahata <isaku.yamahata@intel.com>,
 Sean Christopherson <seanjc@google.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
 isaku.yamahata@gmail.com, Paolo Bonzini <pbonzini@redhat.com>,
 erdemaktas@google.com, Sagi Shahar <sagis@google.com>,
 Kai Huang <kai.huang@intel.com>, chen.bo@intel.com, hang.yuan@intel.com,
 tina.zhang@intel.com, isaku.yamahata@linux.intel.com
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <62f8890cb90e49a3e0b0d5946318c0267b80c540.1708933498.git.isaku.yamahata@intel.com>
 <Zg1yPIV6cVJrwGxX@google.com>
 <20240404234238.GW2444378@ls.amr.corp.intel.com>
From: Binbin Wu <binbin.wu@linux.intel.com>
In-Reply-To: <20240404234238.GW2444378@ls.amr.corp.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 4/5/2024 7:42 AM, Isaku Yamahata wrote:
> 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;

Shoud return true here, right?
>>
>> 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;
>>> +	}
>>> +}
>>> +
>>>