| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d8a81406-12c6-a5e1-7297-49c1a0a800ab@arm.com>
Date: Fri, 12 Mar 2021 18:39:05 +0530
From: Vivek Kumar Gautam <vivek.gautam@....com>
To: Jean-Philippe Brucker <jean-philippe@...aro.org>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
iommu@...ts.linux-foundation.org,
virtualization@...ts.linux-foundation.org, joro@...tes.org,
will.deacon@....com, mst@...hat.com, robin.murphy@....com,
eric.auger@...hat.com, alex.williamson@...hat.com,
kevin.tian@...el.com, jacob.jun.pan@...ux.intel.com,
yi.l.liu@...el.com, lorenzo.pieralisi@....com,
shameerali.kolothum.thodi@...wei.com
Subject: Re: [PATCH RFC v1 15/15] iommu/virtio: Update fault type and reason
info for viommu fault
On 3/3/21 10:55 PM, Jean-Philippe Brucker wrote:
> On Fri, Jan 15, 2021 at 05:43:42PM +0530, Vivek Gautam wrote:
>> Fault type information can tell about a page request fault or
>> an unreceoverable fault, and further additions to fault reasons
>> and the related PASID information can help in handling faults
>> efficiently.
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@....com>
>> Cc: Joerg Roedel <joro@...tes.org>
>> Cc: Will Deacon <will.deacon@....com>
>> Cc: Michael S. Tsirkin <mst@...hat.com>
>> Cc: Robin Murphy <robin.murphy@....com>
>> Cc: Jean-Philippe Brucker <jean-philippe@...aro.org>
>> Cc: Eric Auger <eric.auger@...hat.com>
>> Cc: Alex Williamson <alex.williamson@...hat.com>
>> Cc: Kevin Tian <kevin.tian@...el.com>
>> Cc: Jacob Pan <jacob.jun.pan@...ux.intel.com>
>> Cc: Liu Yi L <yi.l.liu@...el.com>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
>> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@...wei.com>
>> ---
>> drivers/iommu/virtio-iommu.c | 27 +++++++++++++++++++++++++--
>> include/uapi/linux/virtio_iommu.h | 13 ++++++++++++-
>> 2 files changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
>> index 9cc3d35125e9..10ef9e98214a 100644
>> --- a/drivers/iommu/virtio-iommu.c
>> +++ b/drivers/iommu/virtio-iommu.c
>> @@ -652,9 +652,16 @@ static int viommu_fault_handler(struct viommu_dev *viommu,
>> char *reason_str;
>>
>> u8 reason = fault->reason;
>> + u16 type = fault->flt_type;
>> u32 flags = le32_to_cpu(fault->flags);
>> u32 endpoint = le32_to_cpu(fault->endpoint);
>> u64 address = le64_to_cpu(fault->address);
>> + u32 pasid = le32_to_cpu(fault->pasid);
>> +
>> + if (type == VIRTIO_IOMMU_FAULT_F_PAGE_REQ) {
>> + dev_info(viommu->dev, "Page request fault - unhandled\n");
>> + return 0;
>> + }
>>
>> switch (reason) {
>> case VIRTIO_IOMMU_FAULT_R_DOMAIN:
>> @@ -663,6 +670,21 @@ static int viommu_fault_handler(struct viommu_dev *viommu,
>> case VIRTIO_IOMMU_FAULT_R_MAPPING:
>> reason_str = "page";
>> break;
>> + case VIRTIO_IOMMU_FAULT_R_WALK_EABT:
>> + reason_str = "page walk external abort";
>> + break;
>> + case VIRTIO_IOMMU_FAULT_R_PTE_FETCH:
>> + reason_str = "pte fetch";
>> + break;
>> + case VIRTIO_IOMMU_FAULT_R_PERMISSION:
>> + reason_str = "permission";
>> + break;
>> + case VIRTIO_IOMMU_FAULT_R_ACCESS:
>> + reason_str = "access";
>> + break;
>> + case VIRTIO_IOMMU_FAULT_R_OOR_ADDRESS:
>> + reason_str = "output address";
>> + break;
>> case VIRTIO_IOMMU_FAULT_R_UNKNOWN:
>> default:
>> reason_str = "unknown";
>> @@ -671,8 +693,9 @@ static int viommu_fault_handler(struct viommu_dev *viommu,
>>
>> /* TODO: find EP by ID and report_iommu_fault */
>> if (flags & VIRTIO_IOMMU_FAULT_F_ADDRESS)
>> - dev_err_ratelimited(viommu->dev, "%s fault from EP %u at %#llx [%s%s%s]\n",
>> - reason_str, endpoint, address,
>> + dev_err_ratelimited(viommu->dev,
>> + "%s fault from EP %u PASID %u at %#llx [%s%s%s]\n",
>> + reason_str, endpoint, pasid, address,
>> flags & VIRTIO_IOMMU_FAULT_F_READ ? "R" : "",
>> flags & VIRTIO_IOMMU_FAULT_F_WRITE ? "W" : "",
>> flags & VIRTIO_IOMMU_FAULT_F_EXEC ? "X" : "");
>> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
>> index 608c8d642e1f..a537d82777f7 100644
>> --- a/include/uapi/linux/virtio_iommu.h
>> +++ b/include/uapi/linux/virtio_iommu.h
>> @@ -290,19 +290,30 @@ struct virtio_iommu_req_invalidate {
>> #define VIRTIO_IOMMU_FAULT_R_UNKNOWN 0
>> #define VIRTIO_IOMMU_FAULT_R_DOMAIN 1
>> #define VIRTIO_IOMMU_FAULT_R_MAPPING 2
>> +#define VIRTIO_IOMMU_FAULT_R_WALK_EABT 3
>> +#define VIRTIO_IOMMU_FAULT_R_PTE_FETCH 4
>> +#define VIRTIO_IOMMU_FAULT_R_PERMISSION 5
>> +#define VIRTIO_IOMMU_FAULT_R_ACCESS 6
>> +#define VIRTIO_IOMMU_FAULT_R_OOR_ADDRESS 7
>>
>> #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_ADDRESS (1 << 8)
>>
>> +#define VIRTIO_IOMMU_FAULT_F_DMA_UNRECOV 1
>> +#define VIRTIO_IOMMU_FAULT_F_PAGE_REQ 2
>
> Currently all reported faults are unrecoverable, so to be consistent
> DMA_UNRECOV should be 0. But I'd prefer having just a new "page request"
> flag in the flags field, instead of the flt_type field.
Yea, looking at what I am currently trying as well - handle page-request
and leave all other faults as unrecoverable - I will add the page
request flag in the structure.
>
> For page requests we'll also need a 16-bit fault ID field to store the PRI
> "page request group index" or the stall "stag". "last" and "privileged"
> flags as well, to match the PRI page request. And a new command to
> complete a page fault.
Right, will add the fields as suggested.
To complete the page request we would also need to send the response
back to the host from virtio backend when handling page request. So the
virtio command should also be accompanied with a vfio api to send the
page request response back to the host. Isn't it?
This is where the host smmuv3 can send PRI_RESP command to the device to
complete the page fault.
>
>> +
>> struct virtio_iommu_fault {
>> __u8 reason;
>> - __u8 reserved[3];
>> + __le16 flt_type;
>> + __u8 reserved;
>> __le32 flags;
>> __le32 endpoint;
>> __u8 reserved2[4];
>
> Why not replace reserved2 with the pasid? It fits perfectly :)
Sure, will do it.
Thanks & regards
Vivek
>
> Thanks,
> Jean
>
>> __le64 address;
>> + __le32 pasid;
>> + __u8 reserved3[4];
>> };
>>
>> #endif
>> --
>> 2.17.1
>>
Powered by blists - more mailing lists