Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 15 Aug 2023 16:11:36 -0700
In-Reply-To: <20230719024558.8539-5-guang.zeng@intel.com>
Mime-Version: 1.0
References: <20230719024558.8539-1-guang.zeng@intel.com> <20230719024558.8539-5-guang.zeng@intel.com>
Message-ID: <ZNwGKPnTY7hRRy+S@google.com>
Subject: Re: [PATCH v2 4/8] KVM: x86: Add X86EMUL_F_INVTLB and pass it in em_invlpg()
From:   Sean Christopherson <seanjc@google.com>
To:     Zeng Guang <guang.zeng@intel.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        H Peter Anvin <hpa@zytor.com>, kvm@vger.kernel.org,
        x86@kernel.org, linux-kernel@vger.kernel.org,
        Binbin Wu <binbin.wu@linux.intel.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Wed, Jul 19, 2023, Zeng Guang wrote:
> From: Binbin Wu <binbin.wu@linux.intel.com>
> 
> Add an emulation flag X86EMUL_F_INVTLB, which is used to identify an
> instruction that does TLB invalidation without true memory access.
> 
> Only invlpg & invlpga implemented in emulator belong to this kind.
> invlpga doesn't need additional information for emulation. Just pass
> the flag to em_invlpg().

Please add a paragraph explaining *why* this flag is being added.  Ideally, the
previous patch would also explain the need for an IMPLICIT flag, but that one
doesn't bug me all that much because implicit accesses are known to be special
snowflakes, i.e. it's easy to imagine that KVM would need to identify such
accesses.  But for INVLPG, without already knowing the details of LASS (or LAM),
it's harder to think of why it needs to exist.

> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
> Signed-off-by: Zeng Guang <guang.zeng@intel.com>
> ---
>  arch/x86/kvm/emulate.c     | 4 +++-
>  arch/x86/kvm/kvm_emulate.h | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 8e706d19ae45..9b4b3ce6d52a 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -3443,8 +3443,10 @@ static int em_invlpg(struct x86_emulate_ctxt *ctxt)
>  {
>  	int rc;
>  	ulong linear;
> +	unsigned max_size;

	unsigned int

> -	rc = linearize(ctxt, ctxt->src.addr.mem, 1, false, &linear);
> +	rc = __linearize(ctxt, ctxt->src.addr.mem, &max_size, 1, ctxt->mode,
> +		&linear, X86EMUL_F_INVTLB);

Align indentation:

	rc = __linearize(ctxt, ctxt->src.addr.mem, &max_size, 1, ctxt->mode,
			 &linear, X86EMUL_F_INVTLB);

>  	if (rc == X86EMUL_CONTINUE)
>  		ctxt->ops->invlpg(ctxt, linear);
>  	/* Disable writeback. */
> diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
> index c0e48f4fa7c4..c944055091e1 100644
> --- a/arch/x86/kvm/kvm_emulate.h
> +++ b/arch/x86/kvm/kvm_emulate.h
> @@ -93,6 +93,7 @@ struct x86_instruction_info {
>  #define X86EMUL_F_FETCH			BIT(1)
>  #define X86EMUL_F_BRANCH		BIT(2)
>  #define X86EMUL_F_IMPLICIT		BIT(3)
> +#define X86EMUL_F_INVTLB		BIT(4)

Why F_INVTLB instead of X86EMUL_F_INVLPG?  Ah, because LAM is ignored for the
linear address in the INVPCID and INVVPID descriptors.  Hrm.

I think my vote is to call this X86EMUL_F_INVLPG even though *in theory* it's not
strictly limited to INVLPG.  Odds are good KVM's emulator will never support
INVPCID or INVVPID, and IMO even though F_INVLPG would be somewhat of a misnomer,
it's much more intuitive even for INVPCID and INVVPID descriptors.  F_INVTLB makes
me think more of the actual act of invalidating the TLB.

I'm not dead set against INVTLB if someone really likes it, but I did scratch my
head for a second when I saw it.