From: "Liu, Yi L" <yi.l.liu@intel.com>
To: Peter Xu <peterx@redhat.com>
Cc: "jean-philippe@linaro.org" <jean-philippe@linaro.org>,
"Tian, Kevin" <kevin.tian@intel.com>,
Jacob Pan <jacob.jun.pan@linux.intel.com>,
Yi Sun <yi.y.sun@linux.intel.com>,
Eduardo Habkost <ehabkost@redhat.com>,
"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
"mst@redhat.com" <mst@redhat.com>,
"Tian, Jun J" <jun.j.tian@intel.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"eric.auger@redhat.com" <eric.auger@redhat.com>,
"alex.williamson@redhat.com" <alex.williamson@redhat.com>,
"pbonzini@redhat.com" <pbonzini@redhat.com>,
"Wu, Hao" <hao.wu@intel.com>, "Sun, Yi Y" <yi.y.sun@intel.com>,
Richard Henderson <rth@twiddle.net>,
"david@gibson.dropbear.id.au" <david@gibson.dropbear.id.au>
Subject: RE: [PATCH v2 13/22] intel_iommu: add PASID cache management infrastructure
Date: Fri, 3 Apr 2020 15:05:57 +0000 [thread overview]
Message-ID: <A2975661238FB949B60364EF0F2C25743A220DE7@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <20200402134436.GI7174@xz-x1>
> From: Peter Xu <peterx@redhat.com>
> Sent: Thursday, April 2, 2020 9:45 PM
> To: Liu, Yi L <yi.l.liu@intel.com>
> Subject: Re: [PATCH v2 13/22] intel_iommu: add PASID cache management
> infrastructure
>
> On Thu, Apr 02, 2020 at 06:46:11AM +0000, Liu, Yi L wrote:
>
> [...]
>
> > > > +/**
> > > > + * This function replay the guest pasid bindings to hots by
> > > > + * walking the guest PASID table. This ensures host will have
> > > > + * latest guest pasid bindings. Caller should hold iommu_lock.
> > > > + */
> > > > +static void vtd_replay_guest_pasid_bindings(IntelIOMMUState *s,
> > > > + VTDPASIDCacheInfo
> > > > +*pc_info) {
> > > > + VTDHostIOMMUContext *vtd_dev_icx;
> > > > + int start = 0, end = VTD_HPASID_MAX;
> > > > + vtd_pasid_table_walk_info walk_info = {.flags = 0};
> > >
> > > So vtd_pasid_table_walk_info is still used. I thought we had
> > > reached a consensus that this can be dropped?
> >
> > yeah, I did have considered your suggestion and plan to do it. But
> > when I started coding, it looks a little bit weird to me:
> > For one, there is an input VTDPASIDCacheInfo in this function. It may
> > be nature to think about passing the parameter to further calling
> > (vtd_replay_pasid_bind_for_dev()). But, we can't do that. The
> > vtd_bus/devfn fields should be filled when looping the assigned
> > devices, not the one passed by vtd_replay_guest_pasid_bindings() caller.
>
> Hacky way is we can directly modify VTDPASIDCacheInfo* with bus/devfn for the
> loop. Otherwise we can duplicate the object when looping, so that we can avoid
> introducing a new struct which seems to contain mostly the same information.
I see. Please see below reply.
> > For two, reusing the VTDPASIDCacheInfo for passing walk info may
> > require the final user do the same thing as what the
> > vtd_replay_guest_pasid_bindings() has done here.
>
> I don't see it happen, could you explain?
my concern is around flags field in VTDPASIDCacheInfo. The flags not
only indicates the invalidation granularity, but also indicates the
field presence. e.g. VTD_PASID_CACHE_DEVSI indicates the vtd_bus/devfn
fields are valid. If reuse it to pass walk info to vtd_sm_pasid_table_walk_one,
it would be meaningless as vtd_bus/devfn fields are always valid. But
I'm fine to reuse it's more prefered. Instead of modifying the vtd_bus/devn
in VTDPASIDCacheInfo*, I'd rather to define another VTDPASIDCacheInfo variable
and pass it to vtd_sm_pasid_table_walk_one. This may not affect the future
caller of vtd_replay_guest_pasid_bindings() as vtd_bus/devfn field are not
designed to bring something back to caller.
struct VTDPASIDCacheInfo {
#define VTD_PASID_CACHE_FORCE_RESET (1ULL << 0)
#define VTD_PASID_CACHE_GLOBAL (1ULL << 1)
#define VTD_PASID_CACHE_DOMSI (1ULL << 2)
#define VTD_PASID_CACHE_PASIDSI (1ULL << 3)
#define VTD_PASID_CACHE_DEVSI (1ULL << 4)
uint32_t flags;
uint16_t domain_id;
uint32_t pasid;
VTDBus *vtd_bus;
uint16_t devfn;
};
> >
> > So kept the vtd_pasid_table_walk_info.
>
> [...]
>
> > > > +/**
> > > > + * This function syncs the pasid bindings between guest and host.
> > > > + * It includes updating the pasid cache in vIOMMU and updating
> > > > +the
> > > > + * pasid bindings per guest's latest pasid entry presence.
> > > > + */
> > > > +static void vtd_pasid_cache_sync(IntelIOMMUState *s,
> > > > + VTDPASIDCacheInfo *pc_info) {
> > > > + /*
> > > > + * Regards to a pasid cache invalidation, e.g. a PSI.
> > > > + * it could be either cases of below:
> > > > + * a) a present pasid entry moved to non-present
> > > > + * b) a present pasid entry to be a present entry
> > > > + * c) a non-present pasid entry moved to present
> > > > + *
> > > > + * Different invalidation granularity may affect different device
> > > > + * scope and pasid scope. But for each invalidation granularity,
> > > > + * it needs to do two steps to sync host and guest pasid binding.
> > > > + *
> > > > + * Here is the handling of a PSI:
> > > > + * 1) loop all the existing vtd_pasid_as instances to update them
> > > > + * according to the latest guest pasid entry in pasid table.
> > > > + * this will make sure affected existing vtd_pasid_as instances
> > > > + * cached the latest pasid entries. Also, during the loop, the
> > > > + * host should be notified if needed. e.g. pasid unbind or pasid
> > > > + * update. Should be able to cover case a) and case b).
> > > > + *
> > > > + * 2) loop all devices to cover case c)
> > > > + * - For devices which have HostIOMMUContext instances,
> > > > + * we loop them and check if guest pasid entry exists. If yes,
> > > > + * it is case c), we update the pasid cache and also notify
> > > > + * host.
> > > > + * - For devices which have no HostIOMMUContext, it is not
> > > > + * necessary to create pasid cache at this phase since it
> > > > + * could be created when vIOMMU does DMA address translation.
> > > > + * This is not yet implemented since there is no emulated
> > > > + * pasid-capable devices today. If we have such devices in
> > > > + * future, the pasid cache shall be created there.
> > > > + * Other granularity follow the same steps, just with different scope
> > > > + *
> > > > + */
> > > > +
> > > > + vtd_iommu_lock(s);
> > > > + /* Step 1: loop all the exisitng vtd_pasid_as instances */
> > > > + g_hash_table_foreach_remove(s->vtd_pasid_as,
> > > > + vtd_flush_pasid, pc_info);
> > >
> > > OK the series is evolving along with our discussions, and /me too on
> > > understanding your series... Now I'm not very sure whether this operation is still
> useful...
> > >
> > > The major point is you'll need to do pasid table walk for all the
> > > registered devices below. So IIUC vtd_replay_guest_pasid_bindings()
> > > will be able to also detect addition, removal or modification of
> > > pasid address spaces. Am I right?
> >
> > It's true if there is only assigned pasid-capable devices. If there is
> > emualted pasid-capable device, it would be a problem as emualted
> > devices won't register HostIOMMUContext. Somehow, the pasid cahce
> > invalidation for emualted device would be missed. So I chose to make
> > the step 1 cover the "real" cache invalidation(a.k.a. removal), while
> > step 2 to cover addition and modification.
>
> OK. Btw, I think modification should still belongs to step 1 then (I think you're doing
> that, though).
Oh, yes, modification is done in step 1... step 2 is only for addition.
Regards,
Yi Liu
next prev parent reply other threads:[~2020-04-03 15:08 UTC|newest]
Thread overview: 80+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-30 4:24 [PATCH v2 00/22] intel_iommu: expose Shared Virtual Addressing to VMs Liu Yi L
2020-03-30 4:24 ` [PATCH v2 01/22] scripts/update-linux-headers: Import iommu.h Liu Yi L
2020-03-30 4:24 ` [PATCH v2 02/22] header file update VFIO/IOMMU vSVA APIs Liu Yi L
2020-03-30 4:24 ` [PATCH v2 03/22] vfio: check VFIO_TYPE1_NESTING_IOMMU support Liu Yi L
2020-03-30 9:36 ` Auger Eric
2020-03-31 6:08 ` Liu, Yi L
2020-03-30 4:24 ` [PATCH v2 04/22] hw/iommu: introduce HostIOMMUContext Liu Yi L
2020-03-30 17:22 ` Auger Eric
2020-03-31 4:10 ` Liu, Yi L
2020-03-31 7:47 ` Auger Eric
2020-03-31 12:43 ` Liu, Yi L
2020-04-06 8:04 ` Liu, Yi L
2020-04-06 10:30 ` Auger Eric
2020-03-30 4:24 ` [PATCH v2 05/22] hw/pci: modify pci_setup_iommu() to set PCIIOMMUOps Liu Yi L
2020-03-30 11:02 ` Auger Eric
2020-04-02 8:52 ` Liu, Yi L
2020-04-02 12:41 ` Auger Eric
2020-04-02 13:37 ` Liu, Yi L
2020-04-02 13:49 ` Auger Eric
2020-04-06 6:27 ` Liu, Yi L
2020-04-06 10:04 ` Auger Eric
2020-03-30 4:24 ` [PATCH v2 06/22] hw/pci: introduce pci_device_set/unset_iommu_context() Liu Yi L
2020-03-30 17:30 ` Auger Eric
2020-03-31 12:14 ` Liu, Yi L
2020-03-30 4:24 ` [PATCH v2 07/22] intel_iommu: add set/unset_iommu_context callback Liu Yi L
2020-03-30 20:23 ` Auger Eric
2020-03-31 12:25 ` Liu, Yi L
2020-03-31 12:57 ` Auger Eric
2020-03-30 4:24 ` [PATCH v2 08/22] vfio/common: provide PASID alloc/free hooks Liu Yi L
2020-03-31 10:47 ` Auger Eric
2020-03-31 10:59 ` Liu, Yi L
2020-03-31 11:15 ` Auger Eric
2020-03-31 12:54 ` Liu, Yi L
2020-03-30 4:24 ` [PATCH v2 09/22] vfio/common: init HostIOMMUContext per-container Liu Yi L
2020-04-01 7:50 ` Auger Eric
2020-04-06 7:12 ` Liu, Yi L
2020-04-06 10:20 ` Auger Eric
2020-04-07 11:59 ` Liu, Yi L
2020-03-30 4:24 ` [PATCH v2 10/22] vfio/pci: set host iommu context to vIOMMU Liu Yi L
2020-03-31 14:30 ` Auger Eric
2020-04-01 3:20 ` Liu, Yi L
2020-03-30 4:24 ` [PATCH v2 11/22] intel_iommu: add virtual command capability support Liu Yi L
2020-03-30 4:24 ` [PATCH v2 12/22] intel_iommu: process PASID cache invalidation Liu Yi L
2020-03-30 4:24 ` [PATCH v2 13/22] intel_iommu: add PASID cache management infrastructure Liu Yi L
2020-04-02 0:02 ` Peter Xu
2020-04-02 6:46 ` Liu, Yi L
2020-04-02 13:44 ` Peter Xu
2020-04-03 15:05 ` Liu, Yi L [this message]
2020-04-03 16:19 ` Peter Xu
2020-04-04 11:39 ` Liu, Yi L
2020-03-30 4:24 ` [PATCH v2 14/22] vfio: add bind stage-1 page table support Liu Yi L
2020-03-30 4:24 ` [PATCH v2 15/22] intel_iommu: bind/unbind guest page table to host Liu Yi L
2020-04-02 18:09 ` Peter Xu
2020-04-03 14:29 ` Liu, Yi L
2020-03-30 4:24 ` [PATCH v2 16/22] intel_iommu: replay pasid binds after context cache invalidation Liu Yi L
2020-04-03 14:45 ` Peter Xu
2020-04-03 15:21 ` Liu, Yi L
2020-04-03 16:11 ` Peter Xu
2020-04-04 12:00 ` Liu, Yi L
2020-04-06 19:48 ` Peter Xu
2020-03-30 4:24 ` [PATCH v2 17/22] intel_iommu: do not pass down pasid bind for PASID #0 Liu Yi L
2020-03-30 4:24 ` [PATCH v2 18/22] vfio: add support for flush iommu stage-1 cache Liu Yi L
2020-03-30 4:24 ` [PATCH v2 19/22] intel_iommu: process PASID-based iotlb invalidation Liu Yi L
2020-04-03 14:47 ` Peter Xu
2020-04-03 15:21 ` Liu, Yi L
2020-03-30 4:24 ` [PATCH v2 20/22] intel_iommu: propagate PASID-based iotlb invalidation to host Liu Yi L
2020-03-30 4:25 ` [PATCH v2 21/22] intel_iommu: process PASID-based Device-TLB invalidation Liu Yi L
2020-03-30 4:25 ` [PATCH v2 22/22] intel_iommu: modify x-scalable-mode to be string option Liu Yi L
2020-04-03 14:49 ` Peter Xu
2020-04-03 15:22 ` Liu, Yi L
2020-03-30 5:40 ` [PATCH v2 00/22] intel_iommu: expose Shared Virtual Addressing to VMs no-reply
2020-03-30 10:36 ` Auger Eric
2020-03-30 14:46 ` Peter Xu
2020-03-31 6:53 ` Liu, Yi L
2020-04-02 8:33 ` Jason Wang
2020-04-02 13:46 ` Peter Xu
2020-04-03 1:38 ` Jason Wang
2020-04-03 14:20 ` Liu, Yi L
2020-04-02 18:12 ` Peter Xu
2020-04-03 14:32 ` Liu, Yi L
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=A2975661238FB949B60364EF0F2C25743A220DE7@SHSMSX104.ccr.corp.intel.com \
--to=yi.l.liu@intel.com \
--cc=alex.williamson@redhat.com \
--cc=david@gibson.dropbear.id.au \
--cc=ehabkost@redhat.com \
--cc=eric.auger@redhat.com \
--cc=hao.wu@intel.com \
--cc=jacob.jun.pan@linux.intel.com \
--cc=jean-philippe@linaro.org \
--cc=jun.j.tian@intel.com \
--cc=kevin.tian@intel.com \
--cc=kvm@vger.kernel.org \
--cc=mst@redhat.com \
--cc=pbonzini@redhat.com \
--cc=peterx@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=rth@twiddle.net \
--cc=yi.y.sun@intel.com \
--cc=yi.y.sun@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).