[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zdza1xJNlvhl8OZb@google.com>
Date: Mon, 26 Feb 2024 10:39:19 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Isaku Yamahata <isaku.yamahata@...ux.intel.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, isaku.yamahata@...el.com, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org, isaku.yamahata@...il.com, erdemaktas@...gle.com,
Sagi Shahar <sagis@...gle.com>, Kai Huang <kai.huang@...el.com>, chen.bo@...el.com,
hang.yuan@...el.com, tina.zhang@...el.com, Chao Gao <chao.gao@...el.com>
Subject: Re: [PATCH v18 044/121] KVM: x86/mmu: Assume guest MMIOs are shared
On Mon, Feb 26, 2024, Isaku Yamahata wrote:
> On Mon, Feb 12, 2024 at 11:29:51AM +0100,
> Paolo Bonzini <pbonzini@...hat.com> wrote:
>
> > On Tue, Jan 23, 2024 at 12:55 AM <isaku.yamahata@...el.com> wrote:
> > >
> > > From: Chao Gao <chao.gao@...el.com>
> > >
> > > 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@intel.com
[2] https://lore.kernel.org/all/8b7380f1b02f8e3995f18bebb085e43165d5d682.1708933498.git.isaku.yamahata@intel.com
> - 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@google.com
> + 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@...ux.intel.com>
Powered by blists - more mailing lists