From: "Tian, Kevin" <kevin.tian@intel.com>
To: Auger Eric <eric.auger@redhat.com>,
"Liu, Yi L" <yi.l.liu@intel.com>,
"Alex Williamson" <alex.williamson@redhat.com>
Cc: "jacob.jun.pan@linux.intel.com" <jacob.jun.pan@linux.intel.com>,
"joro@8bytes.org" <joro@8bytes.org>,
"Raj, Ashok" <ashok.raj@intel.com>,
"Tian, Jun J" <jun.j.tian@intel.com>,
"Sun, Yi Y" <yi.y.sun@intel.com>,
"jean-philippe@linaro.org" <jean-philippe@linaro.org>,
"peterx@redhat.com" <peterx@redhat.com>,
"iommu@lists.linux-foundation.org"
<iommu@lists.linux-foundation.org>,
"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"Wu, Hao" <hao.wu@intel.com>
Subject: RE: [PATCH v1 7/8] vfio/type1: Add VFIO_IOMMU_CACHE_INVALIDATE
Date: Thu, 16 Apr 2020 13:28:41 +0000 [thread overview]
Message-ID: <AADFC41AFE54684AB9EE6CBC0274A5D19D823543@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <7d13bdbb-e972-c301-0970-90f63ecf69fc@redhat.com>
> From: Auger Eric <eric.auger@redhat.com>
> Sent: Thursday, April 16, 2020 8:43 PM
>
> Hi Kevin,
> On 4/16/20 2:09 PM, Tian, Kevin wrote:
> >> From: Liu, Yi L <yi.l.liu@intel.com>
> >> Sent: Thursday, April 16, 2020 6:40 PM
> >>
> >> Hi Alex,
> >> Still have a direction question with you. Better get agreement with you
> >> before heading forward.
> >>
> >>> From: Alex Williamson <alex.williamson@redhat.com>
> >>> Sent: Friday, April 3, 2020 11:35 PM
> >> [...]
> >>>>>> + *
> >>>>>> + * returns: 0 on success, -errno on failure.
> >>>>>> + */
> >>>>>> +struct vfio_iommu_type1_cache_invalidate {
> >>>>>> + __u32 argsz;
> >>>>>> + __u32 flags;
> >>>>>> + struct iommu_cache_invalidate_info cache_info;
> >>>>>> +};
> >>>>>> +#define VFIO_IOMMU_CACHE_INVALIDATE _IO(VFIO_TYPE,
> >>> VFIO_BASE
> >>>>> + 24)
> >>>>>
> >>>>> The future extension capabilities of this ioctl worry me, I wonder if
> >>>>> we should do another data[] with flag defining that data as
> >> CACHE_INFO.
> >>>>
> >>>> Can you elaborate? Does it mean with this way we don't rely on iommu
> >>>> driver to provide version_to_size conversion and instead we just pass
> >>>> data[] to iommu driver for further audit?
> >>>
> >>> No, my concern is that this ioctl has a single function, strictly tied
> >>> to the iommu uapi. If we replace cache_info with data[] then we can
> >>> define a flag to specify that data[] is struct
> >>> iommu_cache_invalidate_info, and if we need to, a different flag to
> >>> identify data[] as something else. For example if we get stuck
> >>> expanding cache_info to meet new demands and develop a new uapi to
> >>> solve that, how would we expand this ioctl to support it rather than
> >>> also create a new ioctl? There's also a trade-off in making the ioctl
> >>> usage more difficult for the user. I'd still expect the vfio layer to
> >>> check the flag and interpret data[] as indicated by the flag rather
> >>> than just passing a blob of opaque data to the iommu layer though.
> >>> Thanks,
> >>
> >> Based on your comments about defining a single ioctl and a unified
> >> vfio structure (with a @data[] field) for pasid_alloc/free, bind/
> >> unbind_gpasid, cache_inv. After some offline trying, I think it would
> >> be good for bind/unbind_gpasid and cache_inv as both of them use the
> >> iommu uapi definition. While the pasid alloc/free operation doesn't.
> >> It would be weird to put all of them together. So pasid alloc/free
> >> may have a separate ioctl. It would look as below. Does this direction
> >> look good per your opinion?
> >>
> >> ioctl #22: VFIO_IOMMU_PASID_REQUEST
> >> /**
> >> * @pasid: used to return the pasid alloc result when flags ==
> ALLOC_PASID
> >> * specify a pasid to be freed when flags == FREE_PASID
> >> * @range: specify the allocation range when flags == ALLOC_PASID
> >> */
> >> struct vfio_iommu_pasid_request {
> >> __u32 argsz;
> >> #define VFIO_IOMMU_ALLOC_PASID (1 << 0)
> >> #define VFIO_IOMMU_FREE_PASID (1 << 1)
> >> __u32 flags;
> >> __u32 pasid;
> >> struct {
> >> __u32 min;
> >> __u32 max;
> >> } range;
> >> };
> >>
> >> ioctl #23: VFIO_IOMMU_NESTING_OP
> >> struct vfio_iommu_type1_nesting_op {
> >> __u32 argsz;
> >> __u32 flags;
> >> __u32 op;
> >> __u8 data[];
> >> };
> >>
> >> /* Nesting Ops */
> >> #define VFIO_IOMMU_NESTING_OP_BIND_PGTBL 0
> >> #define VFIO_IOMMU_NESTING_OP_UNBIND_PGTBL 1
> >> #define VFIO_IOMMU_NESTING_OP_CACHE_INVLD 2
> >>
> >
> > Then why cannot we just put PASID into the header since the
> > majority of nested usage is associated with a pasid?
> >
> > ioctl #23: VFIO_IOMMU_NESTING_OP
> > struct vfio_iommu_type1_nesting_op {
> > __u32 argsz;
> > __u32 flags;
> > __u32 op;
> > __u32 pasid;
> > __u8 data[];
> > };
> >
> > In case of SMMUv2 which supports nested w/o PASID, this field can
> > be ignored for that specific case.
> On my side I would prefer keeping the pasid in the data[]. This is not
> always used.
>
> For instance, in iommu_cache_invalidate_info/iommu_inv_pasid_info we
> devised flags to tell whether the PASID is used.
>
But don't we include a PASID in both invalidate structures already?
struct iommu_inv_addr_info {
#define IOMMU_INV_ADDR_FLAGS_PASID (1 << 0)
#define IOMMU_INV_ADDR_FLAGS_ARCHID (1 << 1)
#define IOMMU_INV_ADDR_FLAGS_LEAF (1 << 2)
__u32 flags;
__u32 archid;
__u64 pasid;
__u64 addr;
__u64 granule_size;
__u64 nb_granules;
};
struct iommu_inv_pasid_info {
#define IOMMU_INV_PASID_FLAGS_PASID (1 << 0)
#define IOMMU_INV_PASID_FLAGS_ARCHID (1 << 1)
__u32 flags;
__u32 archid;
__u64 pasid;
};
then consolidating the pasid field into generic header doesn't
hurt. the specific handler still rely on flags to tell whether it
is used?
Thanks
Kevin
next prev parent reply other threads:[~2020-04-16 15:47 UTC|newest]
Thread overview: 110+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-22 12:31 [PATCH v1 0/8] vfio: expose virtual Shared Virtual Addressing to VMs Liu, Yi L
2020-03-22 12:31 ` [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free) Liu, Yi L
2020-03-22 16:21 ` kbuild test robot
2020-03-30 8:32 ` Tian, Kevin
2020-03-30 14:36 ` Liu, Yi L
2020-03-31 5:40 ` Tian, Kevin
2020-03-31 13:22 ` Liu, Yi L
2020-04-01 5:43 ` Tian, Kevin
2020-04-01 5:48 ` Liu, Yi L
2020-03-31 7:53 ` Christoph Hellwig
2020-03-31 8:17 ` Liu, Yi L
2020-03-31 8:32 ` Liu, Yi L
2020-03-31 8:36 ` Liu, Yi L
2020-03-31 9:15 ` Christoph Hellwig
2020-04-02 13:52 ` Jean-Philippe Brucker
2020-04-03 11:56 ` Liu, Yi L
2020-04-03 12:39 ` Jean-Philippe Brucker
2020-04-03 12:44 ` Liu, Yi L
2020-04-02 17:50 ` Alex Williamson
2020-04-03 5:58 ` Tian, Kevin
2020-04-03 15:14 ` Alex Williamson
2020-04-07 4:42 ` Tian, Kevin
2020-04-07 15:14 ` Alex Williamson
2020-04-03 13:12 ` Liu, Yi L
2020-04-03 17:50 ` Alex Williamson
2020-04-07 4:52 ` Tian, Kevin
2020-04-08 0:52 ` Liu, Yi L
2020-03-22 12:31 ` [PATCH v1 2/8] vfio/type1: Add vfio_iommu_type1 parameter for quota tuning Liu, Yi L
2020-03-22 17:20 ` kbuild test robot
2020-03-30 8:40 ` Tian, Kevin
2020-03-30 8:52 ` Liu, Yi L
2020-03-30 9:19 ` Tian, Kevin
2020-03-30 9:26 ` Liu, Yi L
2020-03-30 11:44 ` Tian, Kevin
2020-04-02 17:58 ` Alex Williamson
2020-04-03 8:15 ` Liu, Yi L
2020-03-22 12:32 ` [PATCH v1 3/8] vfio/type1: Report PASID alloc/free support to userspace Liu, Yi L
2020-03-30 9:43 ` Tian, Kevin
2020-04-01 7:46 ` Liu, Yi L
2020-04-01 9:41 ` Auger Eric
2020-04-01 13:13 ` Liu, Yi L
2020-04-02 18:01 ` Alex Williamson
2020-04-03 8:17 ` Liu, Yi L
2020-04-03 17:28 ` Alex Williamson
2020-04-04 11:36 ` Liu, Yi L
2020-03-22 12:32 ` [PATCH v1 4/8] vfio: Check nesting iommu uAPI version Liu, Yi L
2020-03-22 18:30 ` kbuild test robot
2020-03-22 12:32 ` [PATCH v1 5/8] vfio/type1: Report 1st-level/stage-1 format to userspace Liu, Yi L
2020-03-22 16:44 ` kbuild test robot
2020-03-30 11:48 ` Tian, Kevin
2020-04-01 7:38 ` Liu, Yi L
2020-04-01 7:56 ` Tian, Kevin
2020-04-01 8:06 ` Liu, Yi L
2020-04-01 8:08 ` Tian, Kevin
2020-04-01 8:09 ` Liu, Yi L
2020-04-01 8:51 ` Auger Eric
2020-04-01 12:51 ` Liu, Yi L
2020-04-01 13:01 ` Auger Eric
2020-04-03 8:23 ` Jean-Philippe Brucker
2020-04-07 9:43 ` Liu, Yi L
2020-04-08 1:02 ` Liu, Yi L
2020-04-08 10:27 ` Auger Eric
2020-04-09 8:14 ` Jean-Philippe Brucker
2020-04-09 9:01 ` Auger Eric
2020-04-09 12:47 ` Liu, Yi L
2020-04-10 3:28 ` Auger Eric
2020-04-10 3:48 ` Liu, Yi L
2020-04-10 12:30 ` Liu, Yi L
2020-04-02 19:20 ` Alex Williamson
2020-04-03 11:59 ` Liu, Yi L
2020-03-22 12:32 ` [PATCH v1 6/8] vfio/type1: Bind guest page tables to host Liu, Yi L
2020-03-22 18:10 ` kbuild test robot
2020-03-30 12:46 ` Tian, Kevin
2020-04-01 9:13 ` Liu, Yi L
2020-04-02 2:12 ` Tian, Kevin
2020-04-02 8:05 ` Liu, Yi L
2020-04-03 8:34 ` Jean-Philippe Brucker
2020-04-07 10:33 ` Liu, Yi L
2020-04-09 8:28 ` Jean-Philippe Brucker
2020-04-09 9:15 ` Liu, Yi L
2020-04-09 9:38 ` Jean-Philippe Brucker
2020-04-02 19:57 ` Alex Williamson
2020-04-03 13:30 ` Liu, Yi L
2020-04-03 18:11 ` Alex Williamson
2020-04-04 10:28 ` Liu, Yi L
2020-04-11 5:52 ` Liu, Yi L
2020-03-22 12:32 ` [PATCH v1 7/8] vfio/type1: Add VFIO_IOMMU_CACHE_INVALIDATE Liu, Yi L
2020-03-30 12:58 ` Tian, Kevin
2020-04-01 7:49 ` Liu, Yi L
2020-03-31 7:56 ` Christoph Hellwig
2020-03-31 10:48 ` Liu, Yi L
2020-04-02 20:24 ` Alex Williamson
2020-04-03 6:39 ` Tian, Kevin
2020-04-03 15:31 ` Jacob Pan
2020-04-03 15:34 ` Alex Williamson
2020-04-08 2:28 ` Liu, Yi L
2020-04-16 10:40 ` Liu, Yi L
2020-04-16 12:09 ` Tian, Kevin
2020-04-16 12:42 ` Auger Eric
2020-04-16 13:28 ` Tian, Kevin [this message]
2020-04-16 15:12 ` Auger Eric
2020-04-16 14:40 ` Alex Williamson
2020-04-16 14:48 ` Alex Williamson
2020-04-17 6:03 ` Liu, Yi L
2020-03-22 12:32 ` [PATCH v1 8/8] vfio/type1: Add vSVA support for IOMMU-backed mdevs Liu, Yi L
2020-03-30 13:18 ` Tian, Kevin
2020-04-01 7:51 ` Liu, Yi L
2020-04-02 20:33 ` Alex Williamson
2020-04-03 13:39 ` Liu, Yi L
2020-03-26 12:56 ` [PATCH v1 0/8] vfio: expose virtual Shared Virtual Addressing to VMs 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=AADFC41AFE54684AB9EE6CBC0274A5D19D823543@SHSMSX104.ccr.corp.intel.com \
--to=kevin.tian@intel.com \
--cc=alex.williamson@redhat.com \
--cc=ashok.raj@intel.com \
--cc=eric.auger@redhat.com \
--cc=hao.wu@intel.com \
--cc=iommu@lists.linux-foundation.org \
--cc=jacob.jun.pan@linux.intel.com \
--cc=jean-philippe@linaro.org \
--cc=joro@8bytes.org \
--cc=jun.j.tian@intel.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=peterx@redhat.com \
--cc=yi.l.liu@intel.com \
--cc=yi.y.sun@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).