From: Vivek Kumar Gautam <vivek.gautam@arm.com>
To: Jean-Philippe Brucker <jean-philippe@linaro.org>
Cc: linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
iommu@lists.linux-foundation.org,
virtualization@lists.linux-foundation.org, joro@8bytes.org,
will.deacon@arm.com, mst@redhat.com, robin.murphy@arm.com,
eric.auger@redhat.com, kevin.tian@intel.com,
jacob.jun.pan@linux.intel.com, yi.l.liu@intel.com,
Lorenzo.Pieralisi@arm.com, shameerali.kolothum.thodi@huawei.com,
Alex Williamson <alex.williamson@redhat.com>
Subject: Re: [PATCH RFC v1 01/11] uapi/virtio-iommu: Add page request grp-id and flags information
Date: Thu, 30 Sep 2021 10:26:35 +0530 [thread overview]
Message-ID: <3b490967-58b5-7c4a-2275-250e26d24aeb@arm.com> (raw)
In-Reply-To: <YUoBHA6NZaz8wlkA@myrica>
Hi Jean,
On 9/21/21 9:28 PM, Jean-Philippe Brucker wrote:
> Hi Vivek,
>
> Thanks a lot for your work on this
Thanks a lot for taking a look at it. I hope that most of the changes in
this series and the nested page table series [1] are independent of the
presently on-going /dev/iommu proposal, and can be separately reviewed.
Please find my comments inline below.
>
> On Fri, Apr 23, 2021 at 03:21:37PM +0530, Vivek Gautam wrote:
>> Add fault information for group-id and necessary flags for page
>> request faults that can be handled by page fault handler in
>> virtio-iommu driver.
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@arm.com>
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Robin Murphy <robin.murphy@arm.com>
>> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
>> Cc: Eric Auger <eric.auger@redhat.com>
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Cc: Kevin Tian <kevin.tian@intel.com>
>> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
>> Cc: Liu Yi L <yi.l.liu@intel.com>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
>> ---
>> include/uapi/linux/virtio_iommu.h | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
>> index f8bf927a0689..accc3318ce46 100644
>> --- a/include/uapi/linux/virtio_iommu.h
>> +++ b/include/uapi/linux/virtio_iommu.h
>> @@ -307,14 +307,27 @@ struct virtio_iommu_req_invalidate {
>> #define VIRTIO_IOMMU_FAULT_F_DMA_UNRECOV 1
>> #define VIRTIO_IOMMU_FAULT_F_PAGE_REQ 2
>>
>> +#define VIRTIO_IOMMU_FAULT_PRQ_F_PASID_VALID (1 << 0)
>> +#define VIRTIO_IOMMU_FAULT_PRQ_F_LAST_PAGE (1 << 1)
>> +#define VIRTIO_IOMMU_FAULT_PRQ_F_PRIV_DATA (1 << 2)
>> +#define VIRTIO_IOMMU_FAULT_PRQ_F_NEEDS_PASID (1 << 3)
>
> I don't think this one is necessary here. The NEEDS_PASID flags added by
> commit 970471914c67 ("iommu: Allow page responses without PASID") mainly
> helps Linux keep track of things internally. It does tell the fault
> handler whether to reply with PASID or not, but we don't need that here.
> The virtio-iommu driver knows whether a PASID is required by looking at
> the "PRG Response PASID Required" bit in the PCIe capability. For non-PCIe
> faults (e.g. SMMU stall), I'm guessing we'll need a PROBE property to
> declare that the endpoint supports recoverable faults anyway, so "PASID
> required in response" can go through there as well.
Sure, I will remove this flag, and rather read the PCIe cap to find out
if PASID is required or not. After this series, I will follow up with
the non-PCIe fault handling.
>
>> +
>> +#define VIRTIO_IOMMU_FAULT_UNREC_F_PASID_VALID (1 << 0)
>> +#define VIRTIO_IOMMU_FAULT_UNREC_F_ADDR_VALID (1 << 1)
>> +#define VIRTIO_IOMMU_FAULT_UNREC_F_FETCH_ADDR_VALID (1 << 2)
>> +
>> struct virtio_iommu_fault {
>> __u8 reason;
>> __u8 reserved[3];
>> __le16 flt_type;
>> __u8 reserved2[2];
>> + /* flags is actually permission flags */
>
> It's also used for declaring validity of fields.
> VIRTIO_IOMMU_FAULT_F_ADDRESS already tells whether the address field is
> valid, so all the other flags introduced by this patch can go in here.
Sure, will remove pr_evt_flags field, and move all the flags to 'flags'.
>
>> __le32 flags;
>> + /* flags for PASID and Page request handling info */
>> + __le32 pr_evt_flags;
>> __le32 endpoint;
>> __le32 pasid;
>> + __le32 grpid;
>
> I'm not sure why we made it 32-bit in Linux UAPI, it's a little wasteful.
> PCIe PRGI is 9-bits and SMMU STAG is 16-bits. Since the scope of the grpid
> is the endpoint, 16-bit means 64k in-flight faults per endpoint, which
> seems more than enough.
Right, I will update this to 16-bits field. It won't be okay to update
the iommu uAPI now, right?
>
> New fields must be appended at the end of the struct, because old drivers
> will expect to find the 'endpoint' field at this offset. You could remove
> 'reserved3' while adding 'grpid', to keep the struct layout.
Sure, will update this.
>
>> __u8 reserved3[4];
>> __le64 address;
>> __u8 reserved4[8];
>
>
> So the base structure, currently in the spec, looks like this:
>
> struct virtio_iommu_fault {
> u8 reason;
> u8 reserved[3];
> le32 flags;
> le32 endpoint;
> le32 reserved1;
> le64 address;
> };
>
> #define VIRTIO_IOMMU_FAULT_F_READ (1 << 0)
> #define VIRTIO_IOMMU_FAULT_F_WRITE (1 << 1)
> #define VIRTIO_IOMMU_FAULT_F_ADDRESS (1 << 8)
>
> The extended struct could be:
>
> struct virtio_iommu_fault {
> u8 reason;
> u8 reserved[3];
> le32 flags;
> le32 endpoint;
> le32 pasid;
> le64 address;
> /* Page request group ID */
> le16 group_id;
> u8 reserved1[6];
> /* For VT-d private data */
> le64 private_data[2];
> };
>
> #define VIRTIO_IOMMU_FAULT_F_READ (1 << 0)
> #define VIRTIO_IOMMU_FAULT_F_WRITE (1 << 1)
> #define VIRTIO_IOMMU_FAULT_F_EXEC (1 << 2)
> #define VIRTIO_IOMMU_FAULT_F_PRIVILEGED (1 << 3)
> /* Last fault in group */
> #define VIRTIO_IOMMU_FAULT_F_LAST (1 << 4)
> /* Fault is a recoverable page request and requires a response */
> #define VIRTIO_IOMMU_FAULT_F_PAGE_REQ (1 << 5)
>
> /* address field is valid */
> #define VIRTIO_IOMMU_FAULT_F_ADDRESS (1 << 8)
> /* pasid field is valid */
> #define VIRTIO_IOMMU_FAULT_F_PASID (1 << 9)
> /* group_id field is valid */
> #define VIRTIO_IOMMU_FAULT_F_GROUP_ID (1 << 10)
> /* private data field is valid */
> #define VIRTIO_IOMMU_FAULT_F_PRIV_DATA (1 << 11)
Thanks Jean for summarizing it here. I will update the patch.
Best regards
Vivek
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-09-30 4:59 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-23 9:51 [PATCH RFC v1 00/11] iommu/virtio: vSVA support with Arm Vivek Gautam
2021-04-23 9:51 ` [PATCH RFC v1 01/11] uapi/virtio-iommu: Add page request grp-id and flags information Vivek Gautam
2021-09-21 15:58 ` Jean-Philippe Brucker
2021-09-30 4:56 ` Vivek Kumar Gautam [this message]
2021-09-30 8:49 ` Jean-Philippe Brucker
2021-09-30 10:57 ` Vivek Kumar Gautam
2021-04-23 9:51 ` [PATCH RFC v1 02/11] iommu/virtio: Maintain a list of endpoints served by viommu_dev Vivek Gautam
2021-09-21 15:59 ` Jean-Philippe Brucker
2021-09-30 9:17 ` Vivek Kumar Gautam
2021-04-23 9:51 ` [PATCH RFC v1 03/11] iommu/virtio: Handle incoming page faults Vivek Gautam
2021-09-21 16:03 ` Jean-Philippe Brucker
2021-10-11 8:11 ` Vivek Gautam
2021-10-11 9:16 ` Jean-Philippe Brucker
2021-10-11 9:20 ` Vivek Kumar Gautam
2021-04-23 9:51 ` [PATCH RFC v1 04/11] iommu/virtio: Add a io page fault queue Vivek Gautam
2021-04-23 9:51 ` [PATCH RFC v1 05/11] iommu/virtio: Add SVA feature and related enable/disable callbacks Vivek Gautam
2021-09-21 16:04 ` Jean-Philippe Brucker
2021-04-23 9:51 ` [PATCH RFC v1 06/11] iommu/pasid-table: Add pasid table ops for shared context management Vivek Gautam
2021-04-23 9:51 ` [PATCH RFC v1 07/11] iommu/arm-smmu-v3: Move shared context descriptor code to cd-lib Vivek Gautam
2021-04-23 9:51 ` [PATCH RFC v1 08/11] iommu/arm-smmu-v3: Implement shared context alloc and free ops Vivek Gautam
2021-09-21 16:07 ` Jean-Philippe Brucker
2021-09-30 9:50 ` Vivek Kumar Gautam
2021-04-23 9:51 ` [PATCH RFC v1 09/11] iommu/virtio: Implement sva bind/unbind calls Vivek Gautam
2021-09-21 16:09 ` Jean-Philippe Brucker
2021-04-23 9:51 ` [PATCH RFC v1 10/11] uapi/virtio-iommu: Add a new request type to send page response Vivek Gautam
2021-09-21 16:16 ` Jean-Philippe Brucker
2021-09-30 9:24 ` Vivek Kumar Gautam
2021-10-06 9:55 ` Jean-Philippe Brucker
2021-04-23 9:51 ` [PATCH RFC v1 11/11] iommu/virtio: Add support " Vivek Gautam
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=3b490967-58b5-7c4a-2275-250e26d24aeb@arm.com \
--to=vivek.gautam@arm.com \
--cc=Lorenzo.Pieralisi@arm.com \
--cc=alex.williamson@redhat.com \
--cc=eric.auger@redhat.com \
--cc=iommu@lists.linux-foundation.org \
--cc=jacob.jun.pan@linux.intel.com \
--cc=jean-philippe@linaro.org \
--cc=joro@8bytes.org \
--cc=kevin.tian@intel.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mst@redhat.com \
--cc=robin.murphy@arm.com \
--cc=shameerali.kolothum.thodi@huawei.com \
--cc=virtualization@lists.linux-foundation.org \
--cc=will.deacon@arm.com \
--cc=yi.l.liu@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).