On Tue, Jun 20, 2017 at 03:39:30PM +0000, Shameerali Kolothum Thodi wrote: > > > > -----Original Message----- > > From: Robin Murphy [mailto:robin.murphy@xxxxxxx] > > Sent: Tuesday, June 20, 2017 4:16 PM > > To: Shameerali Kolothum Thodi; Lorenzo Pieralisi > > Cc: marc.zyngier@xxxxxxx; sudeep.holla@xxxxxxx; will.deacon@xxxxxxx; > > hanjun.guo@xxxxxxxxxx; Gabriele Paoloni; John Garry; iommu@lists.linux- > > foundation.org; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux- > > acpi@xxxxxxxxxxxxxxx; devel@xxxxxxxxxx; Linuxarm; Wangzhou (B); > > Guohanjun (Hanjun Guo) > > Subject: Re: [PATCH v2 2/2] iommu/arm-smmu-v3:Enable ACPI based > > HiSilicon erratum 161010801 > > > > On 20/06/17 15:07, Shameerali Kolothum Thodi wrote: > > > > > > > > >> -----Original Message----- > > >> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@xxxxxxx] > > >> Sent: Tuesday, June 20, 2017 11:29 AM > > >> To: Shameerali Kolothum Thodi > > >> Cc: marc.zyngier@xxxxxxx; sudeep.holla@xxxxxxx; > > will.deacon@xxxxxxx; > > >> robin.murphy@xxxxxxx; hanjun.guo@xxxxxxxxxx; Gabriele Paoloni; John > > >> Garry; iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-arm- > > >> kernel@xxxxxxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; > > devel@xxxxxxxxxx; > > >> Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo) > > >> Subject: Re: [PATCH v2 2/2] iommu/arm-smmu-v3:Enable ACPI based > > >> HiSilicon erratum 161010801 > > >> > > >> Hi Shameer, > > >> > > >> On Mon, Jun 19, 2017 at 04:45:00PM +0100, shameer wrote: > > >>> The HiSilicon erratum 161010801 describes the limitation of HiSilicon > > >>> platforms Hip06/Hip07 to support the SMMU mappings for MSI > > >> transactions. > > >>> > > >>> On these platforms GICv3 ITS translator is presented with the deviceID > > >>> by extending the MSI payload data to 64 bits to include the deviceID. > > >>> Hence, the PCIe controller on this platforms has to differentiate the > > >>> MSI payload against other DMA payload and has to modify the MSI > > >> payload. > > >>> This basically makes it difficult for this platforms to have a SMMU > > >>> translation for MSI. > > >>> > > >>> This patch implements a ACPI table based quirk to reserve the hw msi > > >>> regions in the smmu-v3 driver which means these address regions will > > >>> not be translated and will be excluded from iova allocations. > > >>> > > >>> Signed-off-by: shameer <shameerali.kolothum.thodi@xxxxxxxxxx> > > >>> --- > > >>> drivers/iommu/arm-smmu-v3.c | 29 ++++++++++++++++++++++++----- > > >>> 1 file changed, 24 insertions(+), 5 deletions(-) > > >>> > > >>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm- > > smmu- > > >> v3.c > > >>> index abe4b88..f03c63b 100644 > > >>> --- a/drivers/iommu/arm-smmu-v3.c > > >>> +++ b/drivers/iommu/arm-smmu-v3.c > > >>> @@ -597,6 +597,7 @@ struct arm_smmu_device { > > >>> u32 features; > > >>> > > >>> #define ARM_SMMU_OPT_SKIP_PREFETCH (1 << 0) > > >>> +#define ARM_SMMU_OPT_RESV_HW_MSI (1 << 1) > > >>> u32 options; > > >>> > > >>> struct arm_smmu_cmdq cmdq; > > >>> @@ -1904,14 +1905,31 @@ static void > > arm_smmu_get_resv_regions(struct > > >> device *dev, > > >>> struct list_head *head) > > >>> { > > >>> struct iommu_resv_region *region; > > >>> + struct arm_smmu_device *smmu; > > >>> + struct iommu_fwspec *fwspec = dev->iommu_fwspec; > > >>> int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO; > > >>> > > >>> - region = iommu_alloc_resv_region(MSI_IOVA_BASE, > > >> MSI_IOVA_LENGTH, > > >>> - prot, IOMMU_RESV_SW_MSI); > > >>> - if (!region) > > >>> - return; > > >>> + smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode); > > >>> + > > >>> + if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI) > > >> && > > >>> + dev_is_pci(dev)) { > > >> > > >> IORT changes are fine to me, I am still no big fan of this supposedly > > >> generic option that is _really_ platform specific (in particular as I > > >> said before the quirk depends on the PCI host bridge but in this > > >> patchset I see no such dependency. In short - the quirk is hooked off > > >> the SMMUv3 model which implicitly implies a PCI host bridge > > >> configuration IIUC). It is Will and Robin decision though, I am not sure > > >> you can make it any tidier (given that on ACPI you may not even have > > >> a PCI host bridge specific _HID to base your check above on). > > > > > > Thanks Lorenzo. I understand your point. As far as our platform is > > > concerned, I think It is ok to remove the dev_is_pci() check and make > > > it generic, if that helps. I don't think it will harm us other than couple of > > > "HW MSI region resv failed: " logs for our platform devices. > > > > I think the answer there is that iort_iommu_its_get_resv_regions() > > really should distinguish between "this device just doesn't have an ITS > > mapping" and "something actually went wrong", such that you don't treat > > the former as an error. That's almost certainly cleaner than e.g. trying > > to check if dev has an associated MSI domain here before calling it. > > If I understood that correctly, the suggestion is to treat cases where device > doesn’t have any ITS node associated with it as "success". > > +int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head) > +{ > [....] > + if (!its_node) > + return -ENODEV; > > ie, return success above from patch 1/2. > > Lorenzo, > > Please let me know if that’s a correct thing to do as I am not sure > about all the error scenarios here. Yes it is, you need to add specific return values that you can then handle in the SMMU callback accordingly (reverting to SW_MSI in case the device does not map to an ITS). Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
