Re: [PATCH v19 110/130] KVM: TDX: Handle TDX PV MMIO hypercall

[Isaku Yamahata]

On Thu, Apr 18, 2024 at 07:04:11PM +0800,
Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx> wrote:

> 
> 
> On 4/18/2024 5:29 PM, Binbin Wu wrote:
> > 
> > > +
> > > +static int tdx_emulate_mmio(struct kvm_vcpu *vcpu)
> > > +{
> > > +    struct kvm_memory_slot *slot;
> > > +    int size, write, r;
> > > +    unsigned long val;
> > > +    gpa_t gpa;
> > > +
> > > +    KVM_BUG_ON(vcpu->mmio_needed, vcpu->kvm);
> > > +
> > > +    size = tdvmcall_a0_read(vcpu);
> > > +    write = tdvmcall_a1_read(vcpu);
> > > +    gpa = tdvmcall_a2_read(vcpu);
> > > +    val = write ? tdvmcall_a3_read(vcpu) : 0;
> > > +
> > > +    if (size != 1 && size != 2 && size != 4 && size != 8)
> > > +        goto error;
> > > +    if (write != 0 && write != 1)
> > > +        goto error;
> > > +
> > > +    /* Strip the shared bit, allow MMIO with and without it set. */
> > Based on the discussion
> > https://lore.kernel.org/all/ZcUO5sFEAIH68JIA@xxxxxxxxxx/
> > Do we still allow the MMIO without shared bit?

That's independent.  The part is how to work around guest accesses the
MMIO region with private GPA.  This part is,  the guest issues
TDG.VP.VMCALL<MMMIO> and KVM masks out the shared bit to make it friendly
to the user space VMM.



> > > +    gpa = gpa & ~gfn_to_gpa(kvm_gfn_shared_mask(vcpu->kvm));
> > > +
> > > +    if (size > 8u || ((gpa + size - 1) ^ gpa) & PAGE_MASK)
> > "size > 8u" can be removed, since based on the check of size above, it
> > can't be greater than 8.

Yes, will remove the check.


> > > +        goto error;
> > > +
> > > +    slot = kvm_vcpu_gfn_to_memslot(vcpu, gpa_to_gfn(gpa));
> > > +    if (slot && !(slot->flags & KVM_MEMSLOT_INVALID))
> > > +        goto error;
> > > +
> > > +    if (!kvm_io_bus_write(vcpu, KVM_FAST_MMIO_BUS, gpa, 0, NULL)) {
> > Should this be checked for write first?
> > 
> > I check the handle_ept_misconfig() in VMX, it doesn't check write first
> > neither.
> > 
> > Functionally, it should be OK since guest will not read the address
> > range of fast mmio.
> > So the read case will be filtered out by ioeventfd_write().
> > But it has take a long way to get to ioeventfd_write().
> > Isn't it more efficient to check write first?
> 
> I got the reason why in handle_ept_misconfig(), it tries to do fast mmio
> write without checking.
> It was intended to make fast mmio faster.
> And for ept misconfig case, it's not easy to get the info of read/write.
> 
> But in this patch, we have already have read/write info, so maybe we can add
> the check for write before fast mmio?

Yes, let's add it.
-- 
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>