lkml.org 
[lkml]   [2019]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC v3 17/21] iommu/smmuv3: Report non recoverable faults
    From
    Date
    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@redhat.com>
    > ---
    > 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?

    > + iommu_report_device_fault(master->dev, &event);

    We should return here if the fault is successfully injected

    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
    >

    \
     
     \ /
      Last update: 2019-01-11 18:47    [W:4.322 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site