LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
To: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: "Tian, Kevin" <kevin.tian@intel.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	"iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Joerg Roedel <joro@8bytes.org>,
	David Woodhouse <dwmw2@infradead.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,
	"Liu, Yi L" <yi.l.liu@intel.com>,
	"Raj, Ashok" <ashok.raj@intel.com>,
	Christoph Hellwig <hch@infradead.org>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	Yi L <yi.l.liu@linux.intel.com>,
	Auger Eric <eric.auger@redhat.com>
Subject: Re: [PATCH v4 04/22] iommu/vt-d: add bind_pasid_table function
Date: Thu, 7 Jun 2018 14:21:13 +0100	[thread overview]
Message-ID: <d10d7011-08c0-2945-8587-3b4c35c84f2c@arm.com> (raw)
In-Reply-To: <20180606142204.529adec5@jacob-builder>

On 06/06/18 22:22, Jacob Pan wrote:
> On Wed, 6 Jun 2018 12:20:51 +0100
> Jean-Philippe Brucker <jean-philippe.brucker@arm.com> wrote:
> 
>> On 05/06/18 18:32, Jacob Pan wrote:
>>>> "bytes" could be passed by VFIO as argument to bind_pasid_table,
>>>> since it can deduce it from argsz
>>>>  
>>> Are you suggesting we wrap this struct in a vfio struct with argsz?
>>> or we directly use this struct?
>>>
>>> I need to clarify how vfio will use this.  
>>
>> Right, I think we've diverged a bit since the last discussion :)
>>
>>> - User program:
>>> struct pasid_table_config ptc = { .bytes = sizeof(ptc) };
>>> ptc.version = 1;
>>> ioctl(device, VFIO_DEVICE_BIND_PASID_TABLE, &ptc);  
>>
>> Any reason to do the ioctl on device instead of container? As we're
>> binding address spaces we probably want a consistent view for the
>> whole container, like the MAP/UNMAP ioctls do.
>>
> I was thinking the pasid table storage is per device, it would be
> more secure if the pasid table is contained within the device. We
> should have one device per container in most cases.
> in case of two or more devices in the same container shares the same
> pasid table, isolation may not be good in that the second device can
> dma with pasids it does not own but in the shared pasid table.

The situation seems similar to map/unmap interface: if two devices are
in the same container, they are not isolated from each others, they
access the same address space. One device can access mappings that were
created for the other, and it's a feature rather than a security issue.
In a non-SVA configuration, if user wants to isolate two devices (the
usual case), they will use different containers. With SVA I think they
should keep doing that. But that's probably a matter of taste more than
a technical problem.

My issue with doing the ioctl on device, though, is that we tell users
that we can isolate PASIDs at device granularity, which isn't
necessarily the case. If two PCI devices are in the same group because
they aren't isolated by ACS (they can do p2p), then a BIND_PASID_TABLE
call on one device might allow the other device to see the same address
spaces, even if that other device doesn't have a pasid table.

In my host-sva patches I don't allow bind if there's more than one
device in the group, but that's only to keep the series simple, and I
don't think we should prevent SVA support for multi-device groups from
being added later (some people might actually want p2p + PASID). So if
not on containers, the ioctl should at least be on groups. Otherwise
we'll make false promises to users and might run into trouble later.

>> As I remember it the userspace interface would use a VFIO header and
>> the BIND ioctl. I can't find the email in my archive though, so I
>> might be imagining it. This is what I remember, on the user side:
>>
>> struct {
>> 	struct vfio_iommu_type1_bind	hdr;
>> 	struct pasid_table_config	cfg;
>> } bind = {
>> 	.hdr.argsz	= sizeof(bind),
>> 	.hdr.flags	= VFIO_IOMMU_BIND_PASID_TABLE,
>> 	/* cfg data here */
>> };
>>
>> ioctl(container, VFIO_DEVICE_BIND, &bind);
>>
> or maybe just use your VFIO_IOMMU_BIND command and vfio_iommu_type1_bind
> with a new flag and PTC as the data. there can be future extensions,
> bind pasid table can be too narrow. And i agree below using argsz and
> flags are more flexible.
> 
> i.e.
> /* takes pasid_table_config as data for flag VFIO_IOMMU_BIND_PASIDTBL */
> struct vfio_iommu_type1_bind {
> 	__u32	argsz;
> 	__u32	flags;
> #define VFIO_IOMMU_BIND_PROCESS		(1 << 0)
> #define VFIO_IOMMU_BIND_PASIDTBL	(1 << 1)
> 	__u8	data[];
> };
> 
> pseudo code in kernel:
> 	switch (bind.flags) {
> 	case VFIO_IOMMU_BIND_PROCESS:
> 		return vfio_iommu_type1_bind_process(iommu, (void *)arg,
> 						     &bind);
> 	case VFIO_IOMMU_BIND_PASIDTBL:
> 		return vfio_iommu_type1_bind_pasid_tbl(iommu, &bind);
> }
> 
> vfio_iommu_type1_bind_pasid_tbl(iommu, bind)
> {
> 	/* loop through domain list, group, device */
> 	struct pasid_table_cfg *ptc = bind->data;
> 	iommu_bind_pasid_table(domain, device, ptc);
> }

Seems sensible

Thanks,
Jean

  reply	other threads:[~2018-06-07 13:21 UTC|newest]

Thread overview: 69+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-16 21:48 [PATCH v4 00/22] IOMMU and VT-d driver support for Shared Virtual Address (SVA) Jacob Pan
2018-04-16 21:48 ` [PATCH v4 01/22] iommu: introduce bind_pasid_table API function Jacob Pan
2018-04-16 21:48 ` [PATCH v4 02/22] iommu/vt-d: move device_domain_info to header Jacob Pan
2018-04-16 21:48 ` [PATCH v4 03/22] iommu/vt-d: add a flag for pasid table bound status Jacob Pan
2018-04-16 21:48 ` [PATCH v4 04/22] iommu/vt-d: add bind_pasid_table function Jacob Pan
2018-04-17 19:10   ` Alex Williamson
2018-04-20 18:25     ` Jean-Philippe Brucker
2018-04-20 23:42       ` Jacob Pan
2018-05-29 20:09         ` Alex Williamson
2018-05-30  1:41           ` Tian, Kevin
2018-05-30  3:17             ` Alex Williamson
2018-05-30  3:45               ` Tian, Kevin
2018-05-30 11:53                 ` Jean-Philippe Brucker
2018-05-30 19:52                   ` Jacob Pan
2018-05-31  9:09                     ` Jean-Philippe Brucker
2018-06-05 17:32                       ` Jacob Pan
2018-06-06 11:20                         ` Jean-Philippe Brucker
2018-06-06 21:22                           ` Jacob Pan
2018-06-07 13:21                             ` Jean-Philippe Brucker [this message]
2018-04-20 23:22     ` Jacob Pan
2018-04-16 21:48 ` [PATCH v4 05/22] iommu: introduce iommu invalidate API function Jacob Pan
2018-04-20 18:19   ` Jean-Philippe Brucker
2018-04-23 20:43     ` Jacob Pan
2018-04-27 18:07       ` Jean-Philippe Brucker
2018-04-28  2:41         ` Tian, Kevin
2018-05-01 22:58         ` Jacob Pan
2018-05-02  9:31           ` Jean-Philippe Brucker
2018-05-04  4:46             ` Jacob Pan
2018-05-04 18:07               ` Jacob Pan
2018-05-08 10:35                 ` Jean-Philippe Brucker
2018-05-09 12:55                   ` Jacob Pan
2018-05-05 22:19   ` Jerry Snitselaar
2018-05-07 15:41     ` Jacob Pan
2018-04-16 21:48 ` [PATCH v4 06/22] iommu/vt-d: add definitions for PFSID Jacob Pan
2018-04-16 21:48 ` [PATCH v4 07/22] iommu/vt-d: fix dev iotlb pfsid use Jacob Pan
2018-04-16 21:48 ` [PATCH v4 08/22] iommu/vt-d: support flushing more translation cache types Jacob Pan
2018-04-16 21:48 ` [PATCH v4 09/22] iommu/vt-d: add svm/sva invalidate function Jacob Pan
2018-04-17 19:10   ` Alex Williamson
2018-04-20 22:36     ` Jacob Pan
2018-04-16 21:48 ` [PATCH v4 10/22] iommu: introduce device fault data Jacob Pan
2018-04-23 10:11   ` Jean-Philippe Brucker
2018-04-23 11:54     ` Jacob Pan
2018-05-20  8:17   ` Liu, Yi L
2018-05-21 23:16     ` Jacob Pan
2018-04-16 21:49 ` [PATCH v4 11/22] driver core: add per device iommu param Jacob Pan
2018-04-23 10:26   ` Greg Kroah-Hartman
2018-04-16 21:49 ` [PATCH v4 12/22] iommu: introduce device fault report API Jacob Pan
2018-04-23 11:30   ` Jean-Philippe Brucker
2018-04-24 18:29     ` Jacob Pan
2018-04-30 16:53   ` Jean-Philippe Brucker
2018-04-30 18:54     ` Jacob Pan
2018-04-16 21:49 ` [PATCH v4 13/22] iommu: introduce page response function Jacob Pan
2018-04-23 11:47   ` Jean-Philippe Brucker
2018-04-23 12:16     ` Jacob Pan
2018-04-23 15:50       ` Jean-Philippe Brucker
2018-04-16 21:49 ` [PATCH v4 14/22] iommu: handle page response timeout Jacob Pan
2018-04-23 15:36   ` Jean-Philippe Brucker
2018-04-25 15:37     ` Jacob Pan
2018-04-30 10:58       ` Jean-Philippe Brucker
2018-04-30 17:54         ` Jacob Pan
2018-04-16 21:49 ` [PATCH v4 15/22] iommu/config: add build dependency for dmar Jacob Pan
2018-04-16 21:49 ` [PATCH v4 16/22] iommu/vt-d: report non-recoverable faults to device Jacob Pan
2018-04-16 21:49 ` [PATCH v4 17/22] iommu/intel-svm: report device page request Jacob Pan
2018-04-16 21:49 ` [PATCH v4 18/22] iommu/intel-svm: replace dev ops with fault report API Jacob Pan
2018-04-16 21:49 ` [PATCH v4 19/22] iommu/intel-svm: do not flush iotlb for viommu Jacob Pan
2018-04-16 21:49 ` [PATCH v4 20/22] iommu/vt-d: add intel iommu page response function Jacob Pan
2018-04-16 21:49 ` [PATCH v4 21/22] trace/iommu: add sva trace events Jacob Pan
2018-04-16 21:49 ` [PATCH v4 22/22] iommu: use sva invalidate and device fault trace event Jacob Pan
  -- strict thread matches above, loose matches on Subject: below --
2018-03-23  3:11 [PATCH v4 00/22] IOMMU and VT-d driver support for Shared Virtual Address (SVA) Jacob Pan
2018-03-23  3:11 ` [PATCH v4 04/22] iommu/vt-d: add bind_pasid_table function Jacob Pan

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=d10d7011-08c0-2945-8587-3b4c35c84f2c@arm.com \
    --to=jean-philippe.brucker@arm.com \
    --cc=alex.williamson@redhat.com \
    --cc=ashok.raj@intel.com \
    --cc=baolu.lu@linux.intel.com \
    --cc=dwmw2@infradead.org \
    --cc=eric.auger@redhat.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@infradead.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jacob.jun.pan@linux.intel.com \
    --cc=joro@8bytes.org \
    --cc=kevin.tian@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rafael.j.wysocki@intel.com \
    --cc=yi.l.liu@intel.com \
    --cc=yi.l.liu@linux.intel.com \
    --subject='Re: [PATCH v4 04/22] iommu/vt-d: add bind_pasid_table function' \
    /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

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).