Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-09-07 Thread Lorenzo Pieralisi 
On Tue, Sep 05, 2017 at 12:07:59PM +0100, John Garry wrote:
> >>
> >>Hi Will, Lorenzo, Robin,
> >>
> >>I have created the patch to add DT support for this erratum.
> >>However, currently I have only added support for pci-based devices.
> >>I'm a bit stumped on how to add platform device support, or if we
> >>should also add support at all. And I would rather ask before
> >>sending the patches.
> >>
> >>The specific issue is that I know of no platform device with an ITS
> >>msi-parent which we would want reserve, i.e. do not translate msi.
> >>And, if there were, how to do it.
> >>
> >>The only platform devices I know off with msi ITS parents are SMMUv3
> >>and mbigen. For mbigen, the msi are not translated. Actually, as I
> >>see under IORT solution, for mbigen we don't reserve the hw msi
> >>region as the SMMUv3 does not have an ID mapping. And we have no
> >>equivalent ID mapping property for DT SMMUv3, so cannot create an
> >>equivalent check.
> >>
> >>So here is how the DT iommu get reserved region function with only
> >>pci device support looks:
> >>
> >>/**
> >> * of_iommu_its_get_resv_regions - Reserved region driver helper
> >> * @dev: Device from iommu_get_resv_regions()
> >> * @list: Reserved region list from iommu_get_resv_regions()
> >> *
> >> * Returns: Number of reserved regions on success(0 if no associated ITS),
> >> *  appropriate error value otherwise.
> >> */
> >>int of_iommu_its_get_resv_regions(struct device *dev, struct
> >>list_head *head)
> >>{
> >>struct device_node *of_node = NULL;
> >>struct device *parent_dev;
> >>const __be32 *msi_map;
> >>int num_mappings, i, err, len, resv = 0;
> >>
> >>/* walk up to find the parent with a msi-map property */
> >>for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
> >>if (!parent_dev->of_node)
> >>continue;
> >>
> >>/*
> >> * Current logic to reserve ITS regions for only PCI root
> >> * complex.
> >> */
> >>msi_map = of_get_property(parent_dev->of_node, "msi-map", );
> >>if (msi_map) {
> >>of_node = parent_dev->of_node;
> >>break;
> >>}
> >>}
> >>
> >>if (!of_node)
> >>return 0;
> >>
> >>num_mappings = of_count_phandle_with_args(of_node, "msi-map",
> >>NULL) / 4;
> >>
> >>for (i = 0; i < num_mappings; i++) {
> >>int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> >>struct iommu_resv_region *region;
> >>struct device_node *msi_node;
> >>struct resource resource;
> >>u32 phandle;
> >>
> >>phandle = be32_to_cpup(msi_map + 1);
> >>msi_node = of_find_node_by_phandle(phandle);
> >>if (!msi_node)
> >>return -ENODEV;
> >>
> >>/*
> >> * There may be different types of msi-controller, so check
> >> * for ITS.
> >> */
> >>if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) {
> >>of_node_put(msi_node);
> >>continue;
> >>}
> >>
> >>err = of_address_to_resource(msi_node, 0, );
> >>
> >>of_node_put(msi_node);
> >>if (err)
> >>return err;
> >>
> >>region = iommu_alloc_resv_region(resource.start, SZ_128K, prot,
> >> IOMMU_RESV_MSI);
> >>if (region) {
> >>list_add_tail(>list, head);
> >>resv++;
> >>}
> >>}
> >>
> >>return (resv == num_mappings) ? resv : -ENODEV;
> >>}
> >>
> >>Any feedback is appreciated.
> >
> >Hi John,
> >
> >I appreciate it is not trivial to make the code generic, part of the
> >snippet above can be shared between ACPI/IORT and OF, the only sticking
> >point is probably the "compatible" string that has to be parameterized
> >since having this code in generic IOMMU layer is a bit horrible to
> >have it ITS specific (and it is a problem that was existing already
> >in the original patch with the hardcoded ITS node type or function
> >name).
> 
> Hi Lorenzo,
> 
> Agreed, checking the ITS compatible string in IOMMU code is not
> nice. However the function is just trying to replicate our ACPI
> version, which calls IORT code directly, and this is ITS specific.
> Anyway, we can make it generic.
> 
> >
> >To sum it up the hook:
> >
> >- has to be called from SMMU driver in a firmware agnostic way
> 
> ok
> 
> >- somehow it has to ascertain which interrupt controller "compatible"
> >  (which in IORT is a node type) to match against so the hook has to
> >  take an id of sorts
> 
> OK
> 
> I will mention 2 more points after discussion on OF solution with Shameer:
> - for platform device, we can add suppport by checking for the
> devices msi-parent property
> - for both pci and platform device, we should check device rid for
> msi-controller also, like IORT solution
> 
> BTW, even though merge window is open, we would like to send some
> solution to the lists this week, so any outstanding topics can
> potentially

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-09-05 Thread John Garry 


Hi Will, Lorenzo, Robin,

I have created the patch to add DT support for this erratum.
However, currently I have only added support for pci-based devices.
I'm a bit stumped on how to add platform device support, or if we
should also add support at all. And I would rather ask before
sending the patches.

The specific issue is that I know of no platform device with an ITS
msi-parent which we would want reserve, i.e. do not translate msi.
And, if there were, how to do it.

The only platform devices I know off with msi ITS parents are SMMUv3
and mbigen. For mbigen, the msi are not translated. Actually, as I
see under IORT solution, for mbigen we don't reserve the hw msi
region as the SMMUv3 does not have an ID mapping. And we have no
equivalent ID mapping property for DT SMMUv3, so cannot create an
equivalent check.

So here is how the DT iommu get reserved region function with only
pci device support looks:

/**
 * of_iommu_its_get_resv_regions - Reserved region driver helper
 * @dev: Device from iommu_get_resv_regions()
 * @list: Reserved region list from iommu_get_resv_regions()
 *
 * Returns: Number of reserved regions on success(0 if no associated ITS),
 *  appropriate error value otherwise.
 */
int of_iommu_its_get_resv_regions(struct device *dev, struct
list_head *head)
{
struct device_node *of_node = NULL;
struct device *parent_dev;
const __be32 *msi_map;
int num_mappings, i, err, len, resv = 0;

/* walk up to find the parent with a msi-map property */
for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
if (!parent_dev->of_node)
continue;

/*
 * Current logic to reserve ITS regions for only PCI root
 * complex.
 */
msi_map = of_get_property(parent_dev->of_node, "msi-map", );
if (msi_map) {
of_node = parent_dev->of_node;
break;
}
}

if (!of_node)
return 0;

num_mappings = of_count_phandle_with_args(of_node, "msi-map",
NULL) / 4;

for (i = 0; i < num_mappings; i++) {
int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
struct iommu_resv_region *region;
struct device_node *msi_node;
struct resource resource;
u32 phandle;

phandle = be32_to_cpup(msi_map + 1);
msi_node = of_find_node_by_phandle(phandle);
if (!msi_node)
return -ENODEV;

/*
 * There may be different types of msi-controller, so check
 * for ITS.
 */
if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) {
of_node_put(msi_node);
continue;
}

err = of_address_to_resource(msi_node, 0, );

of_node_put(msi_node);
if (err)
return err;

region = iommu_alloc_resv_region(resource.start, SZ_128K, prot,
 IOMMU_RESV_MSI);
if (region) {
list_add_tail(>list, head);
resv++;
}
}

return (resv == num_mappings) ? resv : -ENODEV;
}

Any feedback is appreciated.


Hi John,

I appreciate it is not trivial to make the code generic, part of the
snippet above can be shared between ACPI/IORT and OF, the only sticking
point is probably the "compatible" string that has to be parameterized
since having this code in generic IOMMU layer is a bit horrible to
have it ITS specific (and it is a problem that was existing already
in the original patch with the hardcoded ITS node type or function
name).


Hi Lorenzo,

Agreed, checking the ITS compatible string in IOMMU code is not nice. 
However the function is just trying to replicate our ACPI version, which 
calls IORT code directly, and this is ITS specific. Anyway, we can make 
it generic.




To sum it up the hook:

- has to be called from SMMU driver in a firmware agnostic way


ok


- somehow it has to ascertain which interrupt controller "compatible"
  (which in IORT is a node type) to match against so the hook has to
  take an id of sorts


OK

I will mention 2 more points after discussion on OF solution with Shameer:
- for platform device, we can add suppport by checking for the devices 
msi-parent property
- for both pci and platform device, we should check device rid for 
msi-controller also, like IORT solution


BTW, even though merge window is open, we would like to send some 
solution to the lists this week, so any outstanding topics can 
potentially be discussed at LPC next week. Let me know if you're unhappy 
about this.


All the best,
John




I need to go back and have a look at the original patch to see how
we can accommodate both OF/ACPI - certainly the region reservations
code can and should be shared.

Lorenzo

.




___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-09-01 Thread John Garry 

On 10/08/2017 18:27, Will Deacon wrote:

On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum 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 Kolothum 
---
 drivers/iommu/arm-smmu-v3.c | 27 ++-
 1 file changed, 22 insertions(+), 5 deletions(-)


Please can you also add a devicetree binding with corresponding
documentation to enable this workaround on non-ACPI based systems too? It
should be straightforward if you update the arm_smmu_options table.



Hi Will, Lorenzo, Robin,

I have created the patch to add DT support for this erratum. However, 
currently I have only added support for pci-based devices. I'm a bit 
stumped on how to add platform device support, or if we should also add 
support at all. And I would rather ask before sending the patches.


The specific issue is that I know of no platform device with an ITS 
msi-parent which we would want reserve, i.e. do not translate msi. And, 
if there were, how to do it.


The only platform devices I know off with msi ITS parents are SMMUv3 and 
mbigen. For mbigen, the msi are not translated. Actually, as I see under 
IORT solution, for mbigen we don't reserve the hw msi region as the 
SMMUv3 does not have an ID mapping. And we have no equivalent ID mapping 
property for DT SMMUv3, so cannot create an equivalent check.


So here is how the DT iommu get reserved region function with only pci 
device support looks:


/**
 * of_iommu_its_get_resv_regions - Reserved region driver helper
 * @dev: Device from iommu_get_resv_regions()
 * @list: Reserved region list from iommu_get_resv_regions()
 *
 * Returns: Number of reserved regions on success(0 if no associated ITS),
 *  appropriate error value otherwise.
 */
int of_iommu_its_get_resv_regions(struct device *dev, struct list_head 
*head)

{
struct device_node *of_node = NULL;
struct device *parent_dev;
const __be32 *msi_map;
int num_mappings, i, err, len, resv = 0;

/* walk up to find the parent with a msi-map property */
for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
if (!parent_dev->of_node)
continue;

/*
 * Current logic to reserve ITS regions for only PCI root
 * complex.
 */
msi_map = of_get_property(parent_dev->of_node, "msi-map", );
if (msi_map) {
of_node = parent_dev->of_node;
break;
}
}

if (!of_node)
return 0;

num_mappings = of_count_phandle_with_args(of_node, "msi-map", NULL) 
/ 4;


for (i = 0; i < num_mappings; i++) {
int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
struct iommu_resv_region *region;
struct device_node *msi_node;
struct resource resource;
u32 phandle;

phandle = be32_to_cpup(msi_map + 1);
msi_node = of_find_node_by_phandle(phandle);
if (!msi_node)
return -ENODEV;

/*
 * There may be different types of msi-controller, so check
 * for ITS.
 */
if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) {
of_node_put(msi_node);
continue;
}

err = of_address_to_resource(msi_node, 0, );

of_node_put(msi_node);
if (err)
return err;

region = iommu_alloc_resv_region(resource.start, SZ_128K, prot,
 IOMMU_RESV_MSI);
if (region) {
list_add_tail(>list, head);
resv++;
}
}

return (resv == num_mappings) ? resv : -ENODEV;
}

Any feedback is appreciated.

Thanks,
John



Thanks,

Will

.




___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-08-24 Thread John Garry 

On 24/08/2017 15:35, Will Deacon wrote:

> >>OK, seems reasonable.
> >>
> >>We would consider blacklisting the device, where/how to do is the question.
> >>
> >>So the errata is in the GICv3 ITS/PCI host controller, and we just use the
> >>in-between SMMU (driver) to provide the workaround. So my initial impression
> >>is that the PCI host controller would have to be blacklisted IFF behind an
> >>IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
> >>sound?

> >
> >If that avoids us running into the erratum, then fine by me. I'd obviously
> >prefer we work-around it since we know how to, but that's up to you.

>
> I'm surpsised that you may want more errata workaround code to maintain.
>
> Anyway we'll check both approaches and show you how they look and go from
> there.

Don't get me wrong, I don't dream about adding errata workarounds to the
code, but our job as an operating system is to abstract the hardware from
the user, which means dealing with its quirks whether we like it or not.



Fine, it's ok.

For our next platform, hip08, we will provide no DT FW support, so this 
whole DT grey area should not be an issue.


And hopefully no errata also.

Much appreciated,
John


Thanks,

Will



___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-08-24 Thread Will Deacon 
On Wed, Aug 23, 2017 at 05:55:52PM +0100, John Garry wrote:
> On 23/08/2017 17:43, Will Deacon wrote:
> >On Wed, Aug 23, 2017 at 03:29:46PM +0100, John Garry wrote:
> >>On 23/08/2017 14:24, Will Deacon wrote:
> >>>On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
> >>>Signed-off-by: Shameer Kolothum
> >>
> >>>---
> >>>drivers/iommu/arm-smmu-v3.c | 27 ++-
> >>>1 file changed, 22 insertions(+), 5 deletions(-)
> >>
> >>Please can you also add a devicetree binding with corresponding
> >>documentation to enable this workaround on non-ACPI based systems too?
> >>It should be straightforward if you update the arm_smmu_options table.
> >
> >As I mentioned before, devicetree was a lower priority and we would 
> >definitely
> >submit patch to support that. Even if we update the arm_smmu_options 
> >table
> >with DT binding, the generic function to retrieve the MSI address 
> >regions only
> >works on ACPI/IORT case now.
> >
> 
> Hi Will,
> 
> Can you confirm your stance on supporting this workaround for DT as well 
> as
> ACPI?
> 
> For us, we now only "officially" support ACPI FW, and DT support at this
> point is patchy/limited. To me, adding DT support is just more errata
> workaround code to maintain with little useful gain.
> >>>
> >>>I basically don't like the idea of a driver that only works for one of
> >>>ACPI or DT yet claims to support both. I'm less fussed about functionality
> >>>differences (feature X is only available with firmware Y), but not working
> >>>around a hardware erratum that we know about is just lazy.
> >>>
> >>>So I'd prefer that we handle this in both cases, or blacklist affected
> >>>devices when booting with DT. Continuing as though there isn't an erratum
> >>>is the worst thing we can do.
> >>
> >>OK, seems reasonable.
> >>
> >>We would consider blacklisting the device, where/how to do is the question.
> >>
> >>So the errata is in the GICv3 ITS/PCI host controller, and we just use the
> >>in-between SMMU (driver) to provide the workaround. So my initial impression
> >>is that the PCI host controller would have to be blacklisted IFF behind an
> >>IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
> >>sound?
> >
> >If that avoids us running into the erratum, then fine by me. I'd obviously
> >prefer we work-around it since we know how to, but that's up to you.
> 
> I'm surpsised that you may want more errata workaround code to maintain.
> 
> Anyway we'll check both approaches and show you how they look and go from
> there.

Don't get me wrong, I don't dream about adding errata workarounds to the
code, but our job as an operating system is to abstract the hardware from
the user, which means dealing with its quirks whether we like it or not.

Thanks,

Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-08-23 Thread John Garry 

On 23/08/2017 17:43, Will Deacon wrote:

On Wed, Aug 23, 2017 at 03:29:46PM +0100, John Garry wrote:

On 23/08/2017 14:24, Will Deacon wrote:

On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:

Signed-off-by: Shameer Kolothum



---
drivers/iommu/arm-smmu-v3.c | 27 ++-
1 file changed, 22 insertions(+), 5 deletions(-)


Please can you also add a devicetree binding with corresponding
documentation to enable this workaround on non-ACPI based systems too?
It should be straightforward if you update the arm_smmu_options table.


As I mentioned before, devicetree was a lower priority and we would definitely
submit patch to support that. Even if we update the arm_smmu_options table
with DT binding, the generic function to retrieve the MSI address regions only
works on ACPI/IORT case now.



Hi Will,

Can you confirm your stance on supporting this workaround for DT as well as
ACPI?

For us, we now only "officially" support ACPI FW, and DT support at this
point is patchy/limited. To me, adding DT support is just more errata
workaround code to maintain with little useful gain.


I basically don't like the idea of a driver that only works for one of
ACPI or DT yet claims to support both. I'm less fussed about functionality
differences (feature X is only available with firmware Y), but not working
around a hardware erratum that we know about is just lazy.

So I'd prefer that we handle this in both cases, or blacklist affected
devices when booting with DT. Continuing as though there isn't an erratum
is the worst thing we can do.


OK, seems reasonable.

We would consider blacklisting the device, where/how to do is the question.

So the errata is in the GICv3 ITS/PCI host controller, and we just use the
in-between SMMU (driver) to provide the workaround. So my initial impression
is that the PCI host controller would have to be blacklisted IFF behind an
IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
sound?


If that avoids us running into the erratum, then fine by me. I'd obviously
prefer we work-around it since we know how to, but that's up to you.


I'm surpsised that you may want more errata workaround code to maintain.

Anyway we'll check both approaches and show you how they look and go 
from there.





Please also note that in our SoC we have other devices behind the same SMMU,
like storage controller, which are not affected or related to the Errata.

BTW, ignoring DT support, are you happy with this patchset?


Yes, but I won't ack it without the above taken care of.


Fair enough.



Will


Thanks,
John



.




___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-08-23 Thread Will Deacon 
On Wed, Aug 23, 2017 at 03:29:46PM +0100, John Garry wrote:
> On 23/08/2017 14:24, Will Deacon wrote:
> >On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
> >Signed-off-by: Shameer Kolothum
> 
> >---
> >drivers/iommu/arm-smmu-v3.c | 27 ++-
> >1 file changed, 22 insertions(+), 5 deletions(-)
> 
> Please can you also add a devicetree binding with corresponding
> documentation to enable this workaround on non-ACPI based systems too?
> It should be straightforward if you update the arm_smmu_options table.
> >>>
> >>>As I mentioned before, devicetree was a lower priority and we would 
> >>>definitely
> >>>submit patch to support that. Even if we update the arm_smmu_options table
> >>>with DT binding, the generic function to retrieve the MSI address regions 
> >>>only
> >>>works on ACPI/IORT case now.
> >>>
> >>
> >>Hi Will,
> >>
> >>Can you confirm your stance on supporting this workaround for DT as well as
> >>ACPI?
> >>
> >>For us, we now only "officially" support ACPI FW, and DT support at this
> >>point is patchy/limited. To me, adding DT support is just more errata
> >>workaround code to maintain with little useful gain.
> >
> >I basically don't like the idea of a driver that only works for one of
> >ACPI or DT yet claims to support both. I'm less fussed about functionality
> >differences (feature X is only available with firmware Y), but not working
> >around a hardware erratum that we know about is just lazy.
> >
> >So I'd prefer that we handle this in both cases, or blacklist affected
> >devices when booting with DT. Continuing as though there isn't an erratum
> >is the worst thing we can do.
> 
> OK, seems reasonable.
> 
> We would consider blacklisting the device, where/how to do is the question.
> 
> So the errata is in the GICv3 ITS/PCI host controller, and we just use the
> in-between SMMU (driver) to provide the workaround. So my initial impression
> is that the PCI host controller would have to be blacklisted IFF behind an
> IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
> sound?

If that avoids us running into the erratum, then fine by me. I'd obviously
prefer we work-around it since we know how to, but that's up to you.

> Please also note that in our SoC we have other devices behind the same SMMU,
> like storage controller, which are not affected or related to the Errata.
> 
> BTW, ignoring DT support, are you happy with this patchset?

Yes, but I won't ack it without the above taken care of.

Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-08-23 Thread John Garry 

On 23/08/2017 14:24, Will Deacon wrote:

On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:

Signed-off-by: Shameer Kolothum



---
drivers/iommu/arm-smmu-v3.c | 27 ++-
1 file changed, 22 insertions(+), 5 deletions(-)


Please can you also add a devicetree binding with corresponding
documentation to enable this workaround on non-ACPI based systems too?
It should be straightforward if you update the arm_smmu_options table.


As I mentioned before, devicetree was a lower priority and we would definitely
submit patch to support that. Even if we update the arm_smmu_options table
with DT binding, the generic function to retrieve the MSI address regions only
works on ACPI/IORT case now.



Hi Will,

Can you confirm your stance on supporting this workaround for DT as well as
ACPI?

For us, we now only "officially" support ACPI FW, and DT support at this
point is patchy/limited. To me, adding DT support is just more errata
workaround code to maintain with little useful gain.


I basically don't like the idea of a driver that only works for one of
ACPI or DT yet claims to support both. I'm less fussed about functionality
differences (feature X is only available with firmware Y), but not working
around a hardware erratum that we know about is just lazy.

So I'd prefer that we handle this in both cases, or blacklist affected
devices when booting with DT. Continuing as though there isn't an erratum
is the worst thing we can do.


OK, seems reasonable.

We would consider blacklisting the device, where/how to do is the question.

So the errata is in the GICv3 ITS/PCI host controller, and we just use 
the in-between SMMU (driver) to provide the workaround. So my initial 
impression is that the PCI host controller would have to be blacklisted 
IFF behind an IOMMU for DT firmware in pcie-hisi.c or pci quirks 
framework. How does it sound?


Please also note that in our SoC we have other devices behind the same 
SMMU, like storage controller, which are not affected or related to the 
Errata.


BTW, ignoring DT support, are you happy with this patchset?

Regards,
John



Will
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

.




___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-08-23 Thread Will Deacon 
On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
> >>>Signed-off-by: Shameer Kolothum
> >>
> >>>---
> >>> drivers/iommu/arm-smmu-v3.c | 27 ++-
> >>> 1 file changed, 22 insertions(+), 5 deletions(-)
> >>
> >>Please can you also add a devicetree binding with corresponding
> >>documentation to enable this workaround on non-ACPI based systems too?
> >>It should be straightforward if you update the arm_smmu_options table.
> >
> >As I mentioned before, devicetree was a lower priority and we would 
> >definitely
> >submit patch to support that. Even if we update the arm_smmu_options table
> >with DT binding, the generic function to retrieve the MSI address regions 
> >only
> >works on ACPI/IORT case now.
> >
> 
> Hi Will,
> 
> Can you confirm your stance on supporting this workaround for DT as well as
> ACPI?
> 
> For us, we now only "officially" support ACPI FW, and DT support at this
> point is patchy/limited. To me, adding DT support is just more errata
> workaround code to maintain with little useful gain.

I basically don't like the idea of a driver that only works for one of
ACPI or DT yet claims to support both. I'm less fussed about functionality
differences (feature X is only available with firmware Y), but not working
around a hardware erratum that we know about is just lazy.

So I'd prefer that we handle this in both cases, or blacklist affected
devices when booting with DT. Continuing as though there isn't an erratum
is the worst thing we can do.

Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-08-23 Thread John Garry 

Signed-off-by: Shameer Kolothum



---
 drivers/iommu/arm-smmu-v3.c | 27 ++-
 1 file changed, 22 insertions(+), 5 deletions(-)


Please can you also add a devicetree binding with corresponding
documentation to enable this workaround on non-ACPI based systems too?
It should be straightforward if you update the arm_smmu_options table.


As I mentioned before, devicetree was a lower priority and we would definitely
submit patch to support that. Even if we update the arm_smmu_options table
with DT binding, the generic function to retrieve the MSI address regions only
works on ACPI/IORT case now.



Hi Will,

Can you confirm your stance on supporting this workaround for DT as well 
as ACPI?


For us, we now only "officially" support ACPI FW, and DT support at this 
point is patchy/limited. To me, adding DT support is just more errata 
workaround code to maintain with little useful gain.


Thanks very much,
John


Moreover I am on holidays starting tomorrow, and really appreciate if this 
series
can go through now.

Please let me know.

Thanks,
Shameer

.




___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-08-10 Thread Shameerali Kolothum Thodi 
Hi Will,

> -Original Message-
> From: Will Deacon [mailto:will.dea...@arm.com]
> Sent: Thursday, August 10, 2017 6:27 PM
> To: Shameerali Kolothum Thodi
> Cc: lorenzo.pieral...@arm.com; marc.zyng...@arm.com;
> sudeep.ho...@arm.com; robin.mur...@arm.com; hanjun@linaro.org;
> Gabriele Paoloni; John Garry; iommu@lists.linux-foundation.org; linux-arm-
> ker...@lists.infradead.org; linux-a...@vger.kernel.org; de...@acpica.org;
> Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> Subject: Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based
> HiSilicon erratum 161010801
> 
> On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum 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 Kolothum
> <shameerali.kolothum.th...@huawei.com>
> > ---
> >  drivers/iommu/arm-smmu-v3.c | 27 ++-
> >  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> Please can you also add a devicetree binding with corresponding
> documentation to enable this workaround on non-ACPI based systems too?
> It should be straightforward if you update the arm_smmu_options table.

As I mentioned before, devicetree was a lower priority and we would definitely
submit patch to support that. Even if we update the arm_smmu_options table
with DT binding, the generic function to retrieve the MSI address regions only
works on ACPI/IORT case now.

Moreover I am on holidays starting tomorrow, and really appreciate if this 
series
can go through now.

Please let me know.

Thanks,
Shameer
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-08-10 Thread Will Deacon 
On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum 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 Kolothum 
> ---
>  drivers/iommu/arm-smmu-v3.c | 27 ++-
>  1 file changed, 22 insertions(+), 5 deletions(-)

Please can you also add a devicetree binding with corresponding
documentation to enable this workaround on non-ACPI based systems too? It
should be straightforward if you update the arm_smmu_options table.

Thanks,

Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu