Re: [PATCH v18 044/121] KVM: x86/mmu: Assume guest MMIOs are shared

[Sean Christopherson]

On Mon, Feb 26, 2024, Isaku Yamahata wrote:
> On Mon, Feb 12, 2024 at 11:29:51AM +0100,
> Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
> 
> > On Tue, Jan 23, 2024 at 12:55 AM <isaku.yamahata@xxxxxxxxx> wrote:
> > >
> > > From: Chao Gao <chao.gao@xxxxxxxxx>
> > >
> > > Guest TD doesn't necessarily invoke MAP_GPA to convert the virtual MMIO
> > > range to shared before accessing it.  When TD tries to access the virtual
> > > device's MMIO as shared, an EPT violation is raised first.
> > > kvm_mem_is_private() checks whether the GFN is shared or private.  If
> > > MAP_GPA is not called for the GPA, KVM thinks the GPA is private and
> > > refuses shared access, and doesn't set up shared EPT entry.  The guest
> > > can't make progress.
> > >
> > > Instead of requiring the guest to invoke MAP_GPA for regions of virtual
> > > MMIOs assume regions of virtual MMIOs are shared in KVM as well (i.e., GPAs
> > > either have no kvm_memory_slot or are backed by host MMIOs). So that guests
> > > can access those MMIO regions.
> > 
> > I'm not sure how the patch below deals with host MMIOs?
> 
> It falls back to shared case to hit KVM_PFN_NOSLOT. It will be handled as
> MMIO.
> 
> Anyway I found it breaks SW_PROTECTED case.  So I came up with the following.
> I think we'd like to handle as
>   - SW_PROTECTED => KVM_EXIT_MEMORY_FAULT
>   - SNP, TDX => MMIO.
>   

FFS.  Stop lobbing patch bombs and start having actual conversations.

Seriously, the whole point of using mailing lists is to have *discussions* and
to coordinate development.  Throwing patches at kvm@ and then walking away DOES
NOT WORK.

Putting a "TODO: Drop this patch once the common patch is merged." in the
changelog[1] is not helpful.

Dropping a proposed common uAPI[2] into a 121 patch series without even *acknowledging*
that you received the message DOES NOT WORK.  You didn't even add a Suggested-by
or Cc: the people who expressed interest.  I can't read minds, and AFAIK no one
else working on KVM is a telepath either.

I do not know to make it any clearer: for TDX support to go anywhere, there needs
to be a _lot_ more communication.

[1] https://lore.kernel.org/all/b2e5c92fd66a0113b472dd602220346d3d435732.1708933498.git.isaku.yamahata@xxxxxxxxx
[2] https://lore.kernel.org/all/8b7380f1b02f8e3995f18bebb085e43165d5d682.1708933498.git.isaku.yamahata@xxxxxxxxx

> -       if (fault->is_private != kvm_mem_is_private(vcpu->kvm, fault->gfn)) {
> +       /*
> +        * !fault->slot means MMIO for SNP and TDX.  Don't require explicit GPA
> +        * conversion for MMIO because MMIO is assigned at the boot time.  Fall
> +        * to !is_private case to get pfn = KVM_PFN_NOSLOT.
> +        */
> +       force_mmio = !slot &&

NAK, this already got shot down.

https://lore.kernel.org/all/ZcUO5sFEAIH68JIA@xxxxxxxxxx

> +               vcpu->kvm->arch.vm_type != KVM_X86_DEFAULT_VM &&
> +               vcpu->kvm->arch.vm_type != KVM_X86_SW_PROTECTED_VM;
> +       if (!force_mmio &&
> +           fault->is_private != kvm_mem_is_private(vcpu->kvm, fault->gfn)) {
>                 kvm_mmu_prepare_memory_fault_exit(vcpu, fault);
>                 return -EFAULT;
>         }
>  
> -       if (fault->is_private)
> +       if (!force_mmio && fault->is_private)
>                 return kvm_faultin_pfn_private(vcpu, fault);
> 
> -- 
> Isaku Yamahata <isaku.yamahata@xxxxxxxxxxxxxxx>