On Tue, Aug 19 2014 at 05:48:07 AM, Will Deacon <will.deacon@xxxxxxx> wrote:
> On Wed, Aug 13, 2014 at 01:51:39AM +0100, Mitchel Humpherys wrote:
>> Under certain conditions coherent hardware translation table walks can
>> result in degraded performance. Add a new domain attribute to
>> disable/enable this feature in generic code along with the domain
>> attribute setter and getter to handle it in the ARM SMMU driver.
>
> Again, it would be nice to have some information about these cases and the
> performance issues that you are seeing.
Basically, the data I'm basing these statements on is: that's what the
HW folks tell me :). I believe it's specific to our hardware, not ARM
IP. Unfortunately, I don't think I could share the specifics even if I
had them, but I can try to press the issue if you want me to.
>
>> @@ -1908,11 +1917,15 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
>> enum iommu_attr attr, void *data)
>> {
>> struct arm_smmu_domain *smmu_domain = domain->priv;
>> + struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>>
>> switch (attr) {
>> case DOMAIN_ATTR_NESTING:
>> *(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
>> return 0;
>> + case DOMAIN_ATTR_COHERENT_HTW_DISABLE:
>> + *((bool *)data) = cfg->htw_disable;
>> + return 0;
>
> I think I'd be more comfortable using int instead of bool for this, as it
> could well end up in the user ABI if vfio decides to make use of it. While
> we're here, let's also add an attributes bitmap to the arm_smmu_domain
> instead of having a bool in the arm_smmu_cfg.
Sounds good. I'll make these changes in v2.
>
>> default:
>> return -ENODEV;
>> }
>> @@ -1922,6 +1935,7 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
>> enum iommu_attr attr, void *data)
>> {
>> struct arm_smmu_domain *smmu_domain = domain->priv;
>> + struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>>
>> switch (attr) {
>> case DOMAIN_ATTR_NESTING:
>> @@ -1933,6 +1947,9 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
>> smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
>>
>> return 0;
>> + case DOMAIN_ATTR_COHERENT_HTW_DISABLE:
>> + cfg->htw_disable = *((bool *)data);
>> + return 0;
>> default:
>> return -ENODEV;
>> }
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 0550286df4..8a6449857a 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -81,6 +81,7 @@ enum iommu_attr {
>> DOMAIN_ATTR_FSL_PAMU_ENABLE,
>> DOMAIN_ATTR_FSL_PAMUV1,
>> DOMAIN_ATTR_NESTING, /* two stages of translation */
>> + DOMAIN_ATTR_COHERENT_HTW_DISABLE,
>
> I wonder whether we should make this ARM-specific. Can you take a quick look
> to see if any of the other IOMMUs can potentially benefit from this?
Yeah looks like amd_iommu.c and intel-iommu.c are using
IOMMU_CAP_CACHE_COHERENCY which seems to be the same thing (at least
that's how we're treating it in arm-smmu.c). AMD's doesn't look
configurable but Intel's does, so perhaps they would benefit from this.
-Mitch
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
