Re: [RFC v3 17/21] iommu/smmuv3: Report non recoverable faults

From: Auger Eric
Date: Tue Jan 15 2019 - 16:06:40 EST

Next message: Rob Herring: "Re: [PATCH v2 2/5] dt-binding: pcf8523: add xtal load capacitance"
Previous message: Stephen Rothwell: "linux-next: Fixes tag needs work in the xen-tip tree"
In reply to: Jean-Philippe Brucker: "Re: [RFC v3 17/21] iommu/smmuv3: Report non recoverable faults"
Next in thread: Eric Auger: "[RFC v3 18/21] vfio-pci: Add a new VFIO_REGION_TYPE_NESTED region type"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Jean,

On 1/11/19 6:46 PM, Jean-Philippe Brucker wrote:
> On 08/01/2019 10:26, Eric Auger wrote:
>> When a stage 1 related fault event is read from the event queue,
>> let's propagate it to potential external fault listeners, ie. users
>> who registered a fault handler.
>>
>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>> ---
>> drivers/iommu/arm-smmu-v3.c | 124 ++++++++++++++++++++++++++++++++----
>> 1 file changed, 113 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 999ee470a2ae..6a711cbbb228 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -168,6 +168,26 @@
>> #define ARM_SMMU_PRIQ_IRQ_CFG1 0xd8
>> #define ARM_SMMU_PRIQ_IRQ_CFG2 0xdc
>>
>> +/* Events */
>> +#define ARM_SMMU_EVT_F_UUT 0x01
>> +#define ARM_SMMU_EVT_C_BAD_STREAMID 0x02
>> +#define ARM_SMMU_EVT_F_STE_FETCH 0x03
>> +#define ARM_SMMU_EVT_C_BAD_STE 0x04
>> +#define ARM_SMMU_EVT_F_BAD_ATS_TREQ 0x05
>> +#define ARM_SMMU_EVT_F_STREAM_DISABLED 0x06
>> +#define ARM_SMMU_EVT_F_TRANSL_FORBIDDEN 0x07
>> +#define ARM_SMMU_EVT_C_BAD_SUBSTREAMID 0x08
>> +#define ARM_SMMU_EVT_F_CD_FETCH 0x09
>> +#define ARM_SMMU_EVT_C_BAD_CD 0x0a
>> +#define ARM_SMMU_EVT_F_WALK_EABT 0x0b
>> +#define ARM_SMMU_EVT_F_TRANSLATION 0x10
>> +#define ARM_SMMU_EVT_F_ADDR_SIZE 0x11
>> +#define ARM_SMMU_EVT_F_ACCESS 0x12
>> +#define ARM_SMMU_EVT_F_PERMISSION 0x13
>> +#define ARM_SMMU_EVT_F_TLB_CONFLICT 0x20
>> +#define ARM_SMMU_EVT_F_CFG_CONFLICT 0x21
>> +#define ARM_SMMU_EVT_E_PAGE_REQUEST 0x24
>> +
>> /* Common MSI config fields */
>> #define MSI_CFG0_ADDR_MASK GENMASK_ULL(51, 2)
>> #define MSI_CFG2_SH GENMASK(5, 4)
>> @@ -333,6 +353,11 @@
>> #define EVTQ_MAX_SZ_SHIFT 7
>>
>> #define EVTQ_0_ID GENMASK_ULL(7, 0)
>> +#define EVTQ_0_SUBSTREAMID GENMASK_ULL(31, 12)
>> +#define EVTQ_0_STREAMID GENMASK_ULL(63, 32)
>> +#define EVTQ_1_S2 GENMASK_ULL(39, 39)
>> +#define EVTQ_1_CLASS GENMASK_ULL(40, 41)
>> +#define EVTQ_3_FETCH_ADDR GENMASK_ULL(51, 3)
>>
>> /* PRI queue */
>> #define PRIQ_ENT_DWORDS 2
>> @@ -1270,7 +1295,6 @@ static int arm_smmu_init_l2_strtab(struct arm_smmu_device *smmu, u32 sid)
>> return 0;
>> }
>>
>> -__maybe_unused
>> static struct arm_smmu_master_data *
>> arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
>> {
>> @@ -1296,24 +1320,102 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
>> return master;
>> }
>>
>> +static void arm_smmu_report_event(struct arm_smmu_device *smmu, u64 *evt)
>> +{
>> + u64 fetch_addr = FIELD_GET(EVTQ_3_FETCH_ADDR, evt[3]);
>> + u32 sid = FIELD_GET(EVTQ_0_STREAMID, evt[0]);
>> + bool s1 = !FIELD_GET(EVTQ_1_S2, evt[1]);
>> + u8 type = FIELD_GET(EVTQ_0_ID, evt[0]);
>> + struct arm_smmu_master_data *master;
>> + struct iommu_fault_event event;
>> + bool propagate = true;
>> + u64 addr = evt[2];
>> + int i;
>> +
>> + master = arm_smmu_find_master(smmu, sid);
>> + if (WARN_ON(!master))
>> + return;
>> +
>> + event.fault.type = IOMMU_FAULT_DMA_UNRECOV;
>> +
>> + switch (type) {
>> + case ARM_SMMU_EVT_C_BAD_STREAMID:
>> + event.fault.reason = IOMMU_FAULT_REASON_SOURCEID_INVALID;
>> + break;
>> + case ARM_SMMU_EVT_F_STREAM_DISABLED:
>> + case ARM_SMMU_EVT_C_BAD_SUBSTREAMID:
>> + event.fault.reason = IOMMU_FAULT_REASON_PASID_INVALID;
>> + break;
>> + case ARM_SMMU_EVT_F_CD_FETCH:
>> + event.fault.reason = IOMMU_FAULT_REASON_PASID_FETCH;
>> + break;
>> + case ARM_SMMU_EVT_F_WALK_EABT:
>> + event.fault.reason = IOMMU_FAULT_REASON_WALK_EABT;
>> + event.fault.addr = addr;
>> + event.fault.fetch_addr = fetch_addr;
>> + propagate = s1;
>> + break;
>> + case ARM_SMMU_EVT_F_TRANSLATION:
>> + event.fault.reason = IOMMU_FAULT_REASON_PTE_FETCH;
>> + event.fault.addr = addr;
>> + event.fault.fetch_addr = fetch_addr;
>> + propagate = s1;
>> + break;
>> + case ARM_SMMU_EVT_F_PERMISSION:
>> + event.fault.reason = IOMMU_FAULT_REASON_PERMISSION;
>> + event.fault.addr = addr;
>> + propagate = s1;
>> + break;
>> + case ARM_SMMU_EVT_F_ACCESS:
>> + event.fault.reason = IOMMU_FAULT_REASON_ACCESS;
>> + event.fault.addr = addr;
>> + propagate = s1;
>> + break;
>> + case ARM_SMMU_EVT_C_BAD_STE:
>> + event.fault.reason = IOMMU_FAULT_REASON_BAD_DEVICE_CONTEXT_ENTRY;
>> + break;
>> + case ARM_SMMU_EVT_C_BAD_CD:
>> + event.fault.reason = IOMMU_FAULT_REASON_BAD_PASID_ENTRY;
>> + break;
>> + case ARM_SMMU_EVT_F_ADDR_SIZE:
>> + event.fault.reason = IOMMU_FAULT_REASON_OOR_ADDRESS;
>> + propagate = s1;
>> + break;
>> + case ARM_SMMU_EVT_F_STE_FETCH:
>> + event.fault.reason = IOMMU_FAULT_REASON_DEVICE_CONTEXT_FETCH;
>> + event.fault.fetch_addr = fetch_addr;
>> + break;
>> + /* End of addition */
>> + case ARM_SMMU_EVT_E_PAGE_REQUEST:
>> + case ARM_SMMU_EVT_F_TLB_CONFLICT:
>> + case ARM_SMMU_EVT_F_CFG_CONFLICT:
>> + case ARM_SMMU_EVT_F_BAD_ATS_TREQ:
>> + case ARM_SMMU_EVT_F_TRANSL_FORBIDDEN:
>> + case ARM_SMMU_EVT_F_UUT:
>> + default:
>> + event.fault.reason = IOMMU_FAULT_REASON_UNKNOWN;
>> + }
>> + /* only propagate the error if it relates to stage 1 */
>> + if (s1)
>
> if (propagate)
>
> But I don't quite understand how we're deciding what to propagate: a
> C_BAD_STE is most likely a bug in the SMMU driver, but is reported to
> userspace. On the other hand a stage-2 F_TRANSLATION is likely an error
> from the VMM (didn't setup stage-2 mappings properly), but we're not
> reporting it. Maybe we should add a bit to event.fault that tells
> whether the fault was stage 1 or 2, and let the VMM deal with it?
Yes I mixed this up. propagate should be false by default. In case the
event has an S2 field and S2 == 0 we can safely propagate to the guest.
Otherwise it is case by case and I need to do another review. If it
relates to structures owned by the guest propagate.
>
>> + iommu_report_device_fault(master->dev, &event);
>
> We should return here if the fault is successfully injected

Even if the fault gets injected in the guest can't it be still useful to
get the message below on host side?

Thanks

Eric
>
> Thanks,
> Jean
>
>> +
>> + dev_info(smmu->dev, "event 0x%02x received:\n", type);
>> + for (i = 0; i < EVTQ_ENT_DWORDS; ++i) {
>> + dev_info(smmu->dev, "\t0x%016llx\n",
>> + (unsigned long long)evt[i]);
>> + }
>> +}
>> +
>> /* IRQ and event handlers */
>> static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
>> {
>> - int i;
>> struct arm_smmu_device *smmu = dev;
>> struct arm_smmu_queue *q = &smmu->evtq.q;
>> u64 evt[EVTQ_ENT_DWORDS];
>>
>> do {
>> - while (!queue_remove_raw(q, evt)) {
>> - u8 id = FIELD_GET(EVTQ_0_ID, evt[0]);
>> -
>> - dev_info(smmu->dev, "event 0x%02x received:\n", id);
>> - for (i = 0; i < ARRAY_SIZE(evt); ++i)
>> - dev_info(smmu->dev, "\t0x%016llx\n",
>> - (unsigned long long)evt[i]);
>> -
>> - }
>> + while (!queue_remove_raw(q, evt))
>> + arm_smmu_report_event(smmu, evt);
>>
>> /*
>> * Not much we can do on overflow, so scream and pretend we're
>>
>

Next message: Rob Herring: "Re: [PATCH v2 2/5] dt-binding: pcf8523: add xtal load capacitance"
Previous message: Stephen Rothwell: "linux-next: Fixes tag needs work in the xen-tip tree"
In reply to: Jean-Philippe Brucker: "Re: [RFC v3 17/21] iommu/smmuv3: Report non recoverable faults"
Next in thread: Eric Auger: "[RFC v3 18/21] vfio-pci: Add a new VFIO_REGION_TYPE_NESTED region type"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]