On 8/16/2023 7:11 AM, Sean Christopherson wrote:
On Wed, Jul 19, 2023, Zeng Guang wrote:
From: Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx>
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.
OK, will add the reason for this case and for IMPLICIT as well.
Thanks.
Signed-off-by: Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx>
Signed-off-by: Zeng Guang <guang.zeng@xxxxxxxxx>
---
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
Let me think why I use 'unsigned'...
It's because the exist code uses 'unsigned'.
I suppose it is considered bad practice?
I will cleanup the exist code as well. Is it OK to cleanup it
opportunistically inside this patch?
- 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:
Will update it.
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,
One case is kvm_handle_invpcid() is in the common kvm x86 code.
LAM doesn't apply to the address in descriptor of invpcid though, but I
am not sure if there will be the need for SVM in the future.
But for now, F_INVLPG is OK if you think F_INVTLB brings confusion.
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.
