Received-SPF: pass (google.com: domain of linux-kernel+bounces-88019-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <c4f1b284-5b86-4cd9-9629-56bdd7244cd1@intel.com>
Date: Fri, 1 Mar 2024 15:44:42 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 06/21] KVM: x86/mmu: Track shadow MMIO value on a per-VM
 basis
Content-Language: en-US
To: Paolo Bonzini <pbonzini@redhat.com>, linux-kernel@vger.kernel.org,
 kvm@vger.kernel.org
Cc: seanjc@google.com, michael.roth@amd.com, isaku.yamahata@intel.com,
 thomas.lendacky@amd.com
References: <20240227232100.478238-1-pbonzini@redhat.com>
 <20240227232100.478238-7-pbonzini@redhat.com>
From: Xiaoyao Li <xiaoyao.li@intel.com>
In-Reply-To: <20240227232100.478238-7-pbonzini@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 2/28/2024 7:20 AM, Paolo Bonzini wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> TDX will use a different shadow PTE entry value for MMIO from VMX.  Add
> members to kvm_arch and track value for MMIO per-VM instead of global
> variables.  By using the per-VM EPT entry value for MMIO, the existing VMX
> logic is kept working.  Introduce a separate setter function so that guest
> TD can override later.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> Message-Id: <229a18434e5d83f45b1fcd7bf1544d79db1becb6.1705965635.git.isaku.yamahata@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   arch/x86/include/asm/kvm_host.h |  2 ++
>   arch/x86/kvm/mmu.h              |  1 +
>   arch/x86/kvm/mmu/mmu.c          |  8 +++++---
>   arch/x86/kvm/mmu/spte.c         | 10 ++++++++--
>   arch/x86/kvm/mmu/spte.h         |  4 ++--
>   arch/x86/kvm/mmu/tdp_mmu.c      |  6 +++---
>   6 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 85dc0f7d09e3..a4514c2ef0ec 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1313,6 +1313,8 @@ struct kvm_arch {
>   	 */
>   	spinlock_t mmu_unsync_pages_lock;
>   
> +	u64 shadow_mmio_value;
> +
>   	struct iommu_domain *iommu_domain;
>   	bool iommu_noncoherent;
>   #define __KVM_HAVE_ARCH_NONCOHERENT_DMA
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index 60f21bb4c27b..2c54ba5b0a28 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -101,6 +101,7 @@ static inline u8 kvm_get_shadow_phys_bits(void)
>   }
>   
>   void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask);
> +void kvm_mmu_set_mmio_spte_value(struct kvm *kvm, u64 mmio_value);

Now, the new added function along with above function mislead people on 
their names. It's easily to think

  - kvm_mmu_set_mmio_spte_mask() is to set shadow_mmio_mask, while
  - kvm_mmu_set_mmio_spte_value() is to set shadow_mmio_value

we'd better come up with a better name.

Other than it, the patch looks good to me.

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>