On Mon, 2023-04-17 at 09:31 -0700, Sean Christopherson wrote:
> On Mon, Apr 17, 2023, Metin Kaya wrote:
> > HVMOP_flush_tlbs suboperation of hvm_op hypercall allows a guest to
> > flush all vCPU TLBs. There is no way for the VMM to flush TLBs from
> > userspace.
>
> Ah, took me a minute to connect the dots. Monday morning is definitely partly
> to blame, but it would be helpful to expand this sentence to be more explicit as
> to why userspace's inability to efficiently flush TLBs.
>
> And strictly speaking, userspace _can_ flush TLBs, just not in a precise, efficient
> way.
Hm, how? We should probably implement that in userspace as a fallback,
however much it sucks.
> > arch/x86/kvm/xen.c | 31 ++++++++++++++++++++++++++++++
> > include/xen/interface/hvm/hvm_op.h | 3 +++
>
> Modifications to uapi headers is conspicuously missing. I.e. there likely needs
> to be a capability so that userspace can query support.
Nah, nobody cares. If the kernel "accelerates" this hypercall, so be
it. Userspace will just never get the KVM_EXIT_XEN for that hypercall
because it'll be magically handled, like the others.
>
> > + u64 arg, u64 *r)
> > +{
> > + if (arg) {
> > + *r = -EINVAL;
> > + return;
> > + }
> > +
> > + kvm_make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH_GUEST);
>
> This doesn't even compile. I'll give you one guess as to how much confidence I
> have that this was properly tested.
>
> Aha! And QEMU appears to have Xen emulation support.
As of 8.0, yes :)
> That means KVM-Unit-Tests
> is an option. Specifically, extend the "access" test to use this hypercall instead
> of INVLPG. That'll verify that the flush is actually being performed as expteced.
>
> > + int cmd, u64 arg, u64 *r)
> > +{
> > + switch (cmd) {
> > + case HVMOP_flush_tlbs:
> > + kvm_xen_hvmop_flush_tlbs(vcpu, longmode, arg, r);
>
> This can simply be:
>
> if (arg) {
> *r = -EINVAL;
I don't even know that we care about in-kernel acceleration for the
-EINVAL case. We could just return false for that, and let userspace
(report and) handle it.
It should never happen; Xen has never accepted anything but NULL as the
first argument. For reasons unclear.
case HVMOP_flush_tlbs:
rc = guest_handle_is_null(arg) ? hvmop_flush_tlb_all() : -EINVAL;
I don't think we'll be adding any more HVMOPs to that switch statement
so we might as well make it:
if (cmd == HVMOP_flush_tlbs && !arg)
> } else {
> kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_TLB_FLUSH_GUEST);
> *r = 0;
> }
> return true;
...
> > +/* Flushes all VCPU TLBs:
>
> s/VCPU/vCPU
>
> And "all vCPU TLBs" is ambiguous. It could mean "all TLBs for the target vCPU".
> Adding "guest" in there would also be helpful, e.g. to make it clear that this
> doesn't flush TDP mappings.
>
> > @arg must be NULL. */
>
> Is the "NULL" part verbatim from Xen? Because "0" seems like a better description
> than "NULL" for a u64.
Yes, that's all verbatim from Xen. The first hypercall argument is
typically a pointer.
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
