diff mbox series

[v3,8/8] iommu/arm-smmu-v3: Make default domain type of HiSilicon PTT device to identity

Message ID 20220124131118.17887-9-yangyicong@hisilicon.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series Add support for HiSilicon PCIe Tune and Trace device | expand

Commit Message

Yicong Yang Jan. 24, 2022, 1:11 p.m. UTC
The DMA of HiSilicon PTT device can only work with identical
mapping. So add a quirk for the device to force the domain
passthrough.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

John Garry Feb. 8, 2022, 8:05 a.m. UTC | #1
On 24/01/2022 13:11, Yicong Yang wrote:
> The DMA of HiSilicon PTT device can only work with identical
> mapping. So add a quirk for the device to force the domain
> passthrough.

This patch should be earlier in the series, before the PTT driver, and 
the comment on hisi_ptt_check_iommu_mapping() should mention what is 
going on here.

> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 6dc6d8b6b368..6f67a2b1dd27 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2838,6 +2838,21 @@ static int arm_smmu_dev_disable_feature(struct device *dev,
>   	}
>   }
>   
> +#define IS_HISI_PTT_DEVICE(pdev)	((pdev)->vendor == PCI_VENDOR_ID_HUAWEI && \
> +					 (pdev)->device == 0xa12e)

I assume that not all revisions will require this check, right?

> +
> +static int arm_smmu_def_domain_type(struct device *dev)
> +{
> +	if (dev_is_pci(dev)) {
> +		struct pci_dev *pdev = to_pci_dev(dev);
> +
> +		if (IS_HISI_PTT_DEVICE(pdev))
> +			return IOMMU_DOMAIN_IDENTITY;
> +	}
> +
> +	return 0;
> +}
> +
>   static struct iommu_ops arm_smmu_ops = {
>   	.capable		= arm_smmu_capable,
>   	.domain_alloc		= arm_smmu_domain_alloc,
> @@ -2863,6 +2878,7 @@ static struct iommu_ops arm_smmu_ops = {
>   	.sva_unbind		= arm_smmu_sva_unbind,
>   	.sva_get_pasid		= arm_smmu_sva_get_pasid,
>   	.page_response		= arm_smmu_page_response,
> +	.def_domain_type	= arm_smmu_def_domain_type,
>   	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
>   	.owner			= THIS_MODULE,
>   };
Yicong Yang Feb. 8, 2022, 11:21 a.m. UTC | #2
On 2022/2/8 16:05, John Garry wrote:
> On 24/01/2022 13:11, Yicong Yang wrote:
>> The DMA of HiSilicon PTT device can only work with identical
>> mapping. So add a quirk for the device to force the domain
>> passthrough.
> 
> This patch should be earlier in the series, before the PTT driver, and the comment on hisi_ptt_check_iommu_mapping() should mention what is going on here.
> 

ok I'll reorder the serives and modify the comments of hisi_ptt_check_iommu_mapping() like:

/*
 * The DMA of PTT trace can only use direct mapping, due to some
 * hardware restriction. Check whether there is an iommu or the
 * policy of the iommu domain is passthrough, otherwise the trace
 * cannot work.
 *
 * The PTT device is supposed to behind the arm smmu v3, which
 * should have passthrough the device by a quirk. Otherwise user
 * should manually set the iommu domain type to identity through
 * sysfs.
 */

>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> index 6dc6d8b6b368..6f67a2b1dd27 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -2838,6 +2838,21 @@ static int arm_smmu_dev_disable_feature(struct device *dev,
>>       }
>>   }
>>   +#define IS_HISI_PTT_DEVICE(pdev)    ((pdev)->vendor == PCI_VENDOR_ID_HUAWEI && \
>> +                     (pdev)->device == 0xa12e)
> 
> I assume that not all revisions will require this check, right?
> 

For current revisions it's necessary.

>> +
>> +static int arm_smmu_def_domain_type(struct device *dev)
>> +{
>> +    if (dev_is_pci(dev)) {
>> +        struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> +        if (IS_HISI_PTT_DEVICE(pdev))
>> +            return IOMMU_DOMAIN_IDENTITY;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static struct iommu_ops arm_smmu_ops = {
>>       .capable        = arm_smmu_capable,
>>       .domain_alloc        = arm_smmu_domain_alloc,
>> @@ -2863,6 +2878,7 @@ static struct iommu_ops arm_smmu_ops = {
>>       .sva_unbind        = arm_smmu_sva_unbind,
>>       .sva_get_pasid        = arm_smmu_sva_get_pasid,
>>       .page_response        = arm_smmu_page_response,
>> +    .def_domain_type    = arm_smmu_def_domain_type,
>>       .pgsize_bitmap        = -1UL, /* Restricted during device attach */
>>       .owner            = THIS_MODULE,
>>   };
> 
> .
John Garry Feb. 8, 2022, 11:56 a.m. UTC | #3
On 08/02/2022 11:21, Yicong Yang wrote:
>> This patch should be earlier in the series, before the PTT driver, and the comment on hisi_ptt_check_iommu_mapping() should mention what is going on here.
>>
> ok I'll reorder the serives and modify the comments of hisi_ptt_check_iommu_mapping() like:
> 
> /*
>   * The DMA of PTT trace can only use direct mapping, due to some
>   * hardware restriction. Check whether there is an iommu or the
>   * policy of the iommu domain is passthrough, otherwise the trace
>   * cannot work.

IOMMU, capitalize acronyms

>   *
>   * The PTT device is supposed to behind the arm smmu v3, which
>   * should have passthrough the device by a quirk. Otherwise user
>   * should manually set the iommu domain type to identity through
>   * sysfs.

Sorry, but I don't really understand your meaning here.

I did not think that if we have a default domain then we can change via 
sysfs to anything else.

>   */
> 
>>> Signed-off-by: Yicong Yang<yangyicong@hisilicon.com>
>>> ---
>>>    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++++
>>>    1 file changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> index 6dc6d8b6b368..6f67a2b1dd27 100644
>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> @@ -2838,6 +2838,21 @@ static int arm_smmu_dev_disable_feature(struct device *dev,
>>>        }
>>>    }
>>>    +#define IS_HISI_PTT_DEVICE(pdev)    ((pdev)->vendor == PCI_VENDOR_ID_HUAWEI && \
>>> +                     (pdev)->device == 0xa12e)
>> I assume that not all revisions will require this check, right?

So if you are very confident that the next revision will be fixed then I 
would add a check for this current broken revision.

>>
> For current revisions it's necessary.
>
Yicong Yang Feb. 8, 2022, 12:20 p.m. UTC | #4
On 2022/2/8 19:56, John Garry wrote:
> On 08/02/2022 11:21, Yicong Yang wrote:
>>> This patch should be earlier in the series, before the PTT driver, and the comment on hisi_ptt_check_iommu_mapping() should mention what is going on here.
>>>
>> ok I'll reorder the serives and modify the comments of hisi_ptt_check_iommu_mapping() like:
>>
>> /*
>>   * The DMA of PTT trace can only use direct mapping, due to some
>>   * hardware restriction. Check whether there is an iommu or the
>>   * policy of the iommu domain is passthrough, otherwise the trace
>>   * cannot work.
> 
> IOMMU, capitalize acronyms
> 

ok.

>>   *
>>   * The PTT device is supposed to behind the arm smmu v3, which
>>   * should have passthrough the device by a quirk. Otherwise user
>>   * should manually set the iommu domain type to identity through
>>   * sysfs.
> 
> Sorry, but I don't really understand your meaning here.
> 
> I did not think that if we have a default domain then we can change via sysfs to anything else.
> 

ok I think the last sentence maybe misleading and better drop it.

>>   */
>>
>>>> Signed-off-by: Yicong Yang<yangyicong@hisilicon.com>
>>>> ---
>>>>    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++++
>>>>    1 file changed, 16 insertions(+)
>>>>
>>>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>> index 6dc6d8b6b368..6f67a2b1dd27 100644
>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>> @@ -2838,6 +2838,21 @@ static int arm_smmu_dev_disable_feature(struct device *dev,
>>>>        }
>>>>    }
>>>>    +#define IS_HISI_PTT_DEVICE(pdev)    ((pdev)->vendor == PCI_VENDOR_ID_HUAWEI && \
>>>> +                     (pdev)->device == 0xa12e)
>>> I assume that not all revisions will require this check, right?
> 
> So if you are very confident that the next revision will be fixed then I would add a check for this current broken revision.
> 
>>>
>> For current revisions it's necessary.
>>
> 
> .
Yicong Yang Feb. 14, 2022, 12:55 p.m. UTC | #5
Hi Robin,

Is this quirk ok with the SMMU v3 driver? Just want to confirm that I'm on the
right way to dealing with the issue of our device.

Thanks.

On 2022/1/24 21:11, Yicong Yang wrote:
> The DMA of HiSilicon PTT device can only work with identical
> mapping. So add a quirk for the device to force the domain
> passthrough.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 6dc6d8b6b368..6f67a2b1dd27 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2838,6 +2838,21 @@ static int arm_smmu_dev_disable_feature(struct device *dev,
>  	}
>  }
>  
> +#define IS_HISI_PTT_DEVICE(pdev)	((pdev)->vendor == PCI_VENDOR_ID_HUAWEI && \
> +					 (pdev)->device == 0xa12e)
> +
> +static int arm_smmu_def_domain_type(struct device *dev)
> +{
> +	if (dev_is_pci(dev)) {
> +		struct pci_dev *pdev = to_pci_dev(dev);
> +
> +		if (IS_HISI_PTT_DEVICE(pdev))
> +			return IOMMU_DOMAIN_IDENTITY;
> +	}
> +
> +	return 0;
> +}
> +
>  static struct iommu_ops arm_smmu_ops = {
>  	.capable		= arm_smmu_capable,
>  	.domain_alloc		= arm_smmu_domain_alloc,
> @@ -2863,6 +2878,7 @@ static struct iommu_ops arm_smmu_ops = {
>  	.sva_unbind		= arm_smmu_sva_unbind,
>  	.sva_get_pasid		= arm_smmu_sva_get_pasid,
>  	.page_response		= arm_smmu_page_response,
> +	.def_domain_type	= arm_smmu_def_domain_type,
>  	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
>  	.owner			= THIS_MODULE,
>  };
>
Will Deacon Feb. 15, 2022, 1 p.m. UTC | #6
On Mon, Feb 14, 2022 at 08:55:20PM +0800, Yicong Yang wrote:
> On 2022/1/24 21:11, Yicong Yang wrote:
> > The DMA of HiSilicon PTT device can only work with identical
> > mapping. So add a quirk for the device to force the domain
> > passthrough.
> > 
> > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> > ---
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index 6dc6d8b6b368..6f67a2b1dd27 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -2838,6 +2838,21 @@ static int arm_smmu_dev_disable_feature(struct device *dev,
> >  	}
> >  }
> >  
> > +#define IS_HISI_PTT_DEVICE(pdev)	((pdev)->vendor == PCI_VENDOR_ID_HUAWEI && \
> > +					 (pdev)->device == 0xa12e)
> > +
> > +static int arm_smmu_def_domain_type(struct device *dev)
> > +{
> > +	if (dev_is_pci(dev)) {
> > +		struct pci_dev *pdev = to_pci_dev(dev);
> > +
> > +		if (IS_HISI_PTT_DEVICE(pdev))
> > +			return IOMMU_DOMAIN_IDENTITY;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static struct iommu_ops arm_smmu_ops = {
> >  	.capable		= arm_smmu_capable,
> >  	.domain_alloc		= arm_smmu_domain_alloc,
> > @@ -2863,6 +2878,7 @@ static struct iommu_ops arm_smmu_ops = {
> >  	.sva_unbind		= arm_smmu_sva_unbind,
> >  	.sva_get_pasid		= arm_smmu_sva_get_pasid,
> >  	.page_response		= arm_smmu_page_response,
> > +	.def_domain_type	= arm_smmu_def_domain_type,
> >  	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
> >  	.owner			= THIS_MODULE,
> >  };
> > 
>
> Is this quirk ok with the SMMU v3 driver? Just want to confirm that I'm on the
> right way to dealing with the issue of our device.

I don't think the quirk should be in the SMMUv3 driver. Assumedly, you would
have the exact same problem if you stuck the PTT device behind a different
type of IOMMU, and so the quirk should be handled by a higher level of the
stack.

Will
Robin Murphy Feb. 15, 2022, 1:30 p.m. UTC | #7
On 2022-02-15 13:00, Will Deacon wrote:
> On Mon, Feb 14, 2022 at 08:55:20PM +0800, Yicong Yang wrote:
>> On 2022/1/24 21:11, Yicong Yang wrote:
>>> The DMA of HiSilicon PTT device can only work with identical
>>> mapping. So add a quirk for the device to force the domain
>>> passthrough.
>>>
>>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>>> ---
>>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++++
>>>   1 file changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> index 6dc6d8b6b368..6f67a2b1dd27 100644
>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> @@ -2838,6 +2838,21 @@ static int arm_smmu_dev_disable_feature(struct device *dev,
>>>   	}
>>>   }
>>>   
>>> +#define IS_HISI_PTT_DEVICE(pdev)	((pdev)->vendor == PCI_VENDOR_ID_HUAWEI && \
>>> +					 (pdev)->device == 0xa12e)
>>> +
>>> +static int arm_smmu_def_domain_type(struct device *dev)
>>> +{
>>> +	if (dev_is_pci(dev)) {
>>> +		struct pci_dev *pdev = to_pci_dev(dev);
>>> +
>>> +		if (IS_HISI_PTT_DEVICE(pdev))
>>> +			return IOMMU_DOMAIN_IDENTITY;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   static struct iommu_ops arm_smmu_ops = {
>>>   	.capable		= arm_smmu_capable,
>>>   	.domain_alloc		= arm_smmu_domain_alloc,
>>> @@ -2863,6 +2878,7 @@ static struct iommu_ops arm_smmu_ops = {
>>>   	.sva_unbind		= arm_smmu_sva_unbind,
>>>   	.sva_get_pasid		= arm_smmu_sva_get_pasid,
>>>   	.page_response		= arm_smmu_page_response,
>>> +	.def_domain_type	= arm_smmu_def_domain_type,
>>>   	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
>>>   	.owner			= THIS_MODULE,
>>>   };
>>>
>>
>> Is this quirk ok with the SMMU v3 driver? Just want to confirm that I'm on the
>> right way to dealing with the issue of our device.
> 
> I don't think the quirk should be in the SMMUv3 driver. Assumedly, you would
> have the exact same problem if you stuck the PTT device behind a different
> type of IOMMU, and so the quirk should be handled by a higher level of the
> stack.

Conceptually, yes, but I'm inclined to be pragmatic here. Default domain 
quirks could only move out as far as the other end of the call from 
iommu_get_def_domain_type() - it's not like we could rely on some flag 
in a driver which may not even be loaded yet, let alone matched to the 
device. And even then there's an equal and opposite argument for why the 
core code should have to maintain a list of platform-specific quirks 
rather than code specific to the relevant platforms. The fact is that a 
HiSilicon RCiEP is not going to end up behind anything other than a 
HiSilicon IOMMU, and if those ever stop being SMMUv3 *and* such a quirk 
still exists we can worry about it then.

Ugly as it is, this is the status quo. I don't recall anyone ever 
arguing that the equivalent quirks for Intel integrated graphics should 
be made generic ;)

Cheers,
Robin.
Will Deacon Feb. 15, 2022, 1:42 p.m. UTC | #8
On Tue, Feb 15, 2022 at 01:30:26PM +0000, Robin Murphy wrote:
> On 2022-02-15 13:00, Will Deacon wrote:
> > On Mon, Feb 14, 2022 at 08:55:20PM +0800, Yicong Yang wrote:
> > > On 2022/1/24 21:11, Yicong Yang wrote:
> > > > The DMA of HiSilicon PTT device can only work with identical
> > > > mapping. So add a quirk for the device to force the domain
> > > > passthrough.
> > > > 
> > > > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> > > > ---
> > > >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++++
> > > >   1 file changed, 16 insertions(+)
> > > > 
> > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > > index 6dc6d8b6b368..6f67a2b1dd27 100644
> > > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > > @@ -2838,6 +2838,21 @@ static int arm_smmu_dev_disable_feature(struct device *dev,
> > > >   	}
> > > >   }
> > > > +#define IS_HISI_PTT_DEVICE(pdev)	((pdev)->vendor == PCI_VENDOR_ID_HUAWEI && \
> > > > +					 (pdev)->device == 0xa12e)
> > > > +
> > > > +static int arm_smmu_def_domain_type(struct device *dev)
> > > > +{
> > > > +	if (dev_is_pci(dev)) {
> > > > +		struct pci_dev *pdev = to_pci_dev(dev);
> > > > +
> > > > +		if (IS_HISI_PTT_DEVICE(pdev))
> > > > +			return IOMMU_DOMAIN_IDENTITY;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >   static struct iommu_ops arm_smmu_ops = {
> > > >   	.capable		= arm_smmu_capable,
> > > >   	.domain_alloc		= arm_smmu_domain_alloc,
> > > > @@ -2863,6 +2878,7 @@ static struct iommu_ops arm_smmu_ops = {
> > > >   	.sva_unbind		= arm_smmu_sva_unbind,
> > > >   	.sva_get_pasid		= arm_smmu_sva_get_pasid,
> > > >   	.page_response		= arm_smmu_page_response,
> > > > +	.def_domain_type	= arm_smmu_def_domain_type,
> > > >   	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
> > > >   	.owner			= THIS_MODULE,
> > > >   };
> > > > 
> > > 
> > > Is this quirk ok with the SMMU v3 driver? Just want to confirm that I'm on the
> > > right way to dealing with the issue of our device.
> > 
> > I don't think the quirk should be in the SMMUv3 driver. Assumedly, you would
> > have the exact same problem if you stuck the PTT device behind a different
> > type of IOMMU, and so the quirk should be handled by a higher level of the
> > stack.
> 
> Conceptually, yes, but I'm inclined to be pragmatic here. Default domain
> quirks could only move out as far as the other end of the call from
> iommu_get_def_domain_type() - it's not like we could rely on some flag in a
> driver which may not even be loaded yet, let alone matched to the device.
> And even then there's an equal and opposite argument for why the core code
> should have to maintain a list of platform-specific quirks rather than code
> specific to the relevant platforms. The fact is that a HiSilicon RCiEP is
> not going to end up behind anything other than a HiSilicon IOMMU, and if
> those ever stop being SMMUv3 *and* such a quirk still exists we can worry
> about it then.

Perhaps, but you know that by adding this hook it's only a matter of time
before we get random compatible string matches in there, so I'd rather keep
the flood gates closed as long as we can.

Given that this is a PCI device, why can't we have a PCI quirk for devices
which require an identity mapping and then handle that in the IOMMU core?

> Ugly as it is, this is the status quo. I don't recall anyone ever arguing
> that the equivalent quirks for Intel integrated graphics should be made
> generic ;)

I don't know anything about Intel integrated graphics. Have they solved this
problem in a better way, or could they equally make use of a generic quirk?

Will
Robin Murphy Feb. 15, 2022, 2:29 p.m. UTC | #9
On 2022-02-15 13:42, Will Deacon wrote:
> On Tue, Feb 15, 2022 at 01:30:26PM +0000, Robin Murphy wrote:
>> On 2022-02-15 13:00, Will Deacon wrote:
>>> On Mon, Feb 14, 2022 at 08:55:20PM +0800, Yicong Yang wrote:
>>>> On 2022/1/24 21:11, Yicong Yang wrote:
>>>>> The DMA of HiSilicon PTT device can only work with identical
>>>>> mapping. So add a quirk for the device to force the domain
>>>>> passthrough.
>>>>>
>>>>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>>>>> ---
>>>>>    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++++
>>>>>    1 file changed, 16 insertions(+)
>>>>>
>>>>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>>> index 6dc6d8b6b368..6f67a2b1dd27 100644
>>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>>> @@ -2838,6 +2838,21 @@ static int arm_smmu_dev_disable_feature(struct device *dev,
>>>>>    	}
>>>>>    }
>>>>> +#define IS_HISI_PTT_DEVICE(pdev)	((pdev)->vendor == PCI_VENDOR_ID_HUAWEI && \
>>>>> +					 (pdev)->device == 0xa12e)
>>>>> +
>>>>> +static int arm_smmu_def_domain_type(struct device *dev)
>>>>> +{
>>>>> +	if (dev_is_pci(dev)) {
>>>>> +		struct pci_dev *pdev = to_pci_dev(dev);
>>>>> +
>>>>> +		if (IS_HISI_PTT_DEVICE(pdev))
>>>>> +			return IOMMU_DOMAIN_IDENTITY;
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>>    static struct iommu_ops arm_smmu_ops = {
>>>>>    	.capable		= arm_smmu_capable,
>>>>>    	.domain_alloc		= arm_smmu_domain_alloc,
>>>>> @@ -2863,6 +2878,7 @@ static struct iommu_ops arm_smmu_ops = {
>>>>>    	.sva_unbind		= arm_smmu_sva_unbind,
>>>>>    	.sva_get_pasid		= arm_smmu_sva_get_pasid,
>>>>>    	.page_response		= arm_smmu_page_response,
>>>>> +	.def_domain_type	= arm_smmu_def_domain_type,
>>>>>    	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
>>>>>    	.owner			= THIS_MODULE,
>>>>>    };
>>>>>
>>>>
>>>> Is this quirk ok with the SMMU v3 driver? Just want to confirm that I'm on the
>>>> right way to dealing with the issue of our device.
>>>
>>> I don't think the quirk should be in the SMMUv3 driver. Assumedly, you would
>>> have the exact same problem if you stuck the PTT device behind a different
>>> type of IOMMU, and so the quirk should be handled by a higher level of the
>>> stack.
>>
>> Conceptually, yes, but I'm inclined to be pragmatic here. Default domain
>> quirks could only move out as far as the other end of the call from
>> iommu_get_def_domain_type() - it's not like we could rely on some flag in a
>> driver which may not even be loaded yet, let alone matched to the device.
>> And even then there's an equal and opposite argument for why the core code
>> should have to maintain a list of platform-specific quirks rather than code
>> specific to the relevant platforms. The fact is that a HiSilicon RCiEP is
>> not going to end up behind anything other than a HiSilicon IOMMU, and if
>> those ever stop being SMMUv3 *and* such a quirk still exists we can worry
>> about it then.
> 
> Perhaps, but you know that by adding this hook it's only a matter of time
> before we get random compatible string matches in there, so I'd rather keep
> the flood gates closed as long as we can.
> 
> Given that this is a PCI device, why can't we have a PCI quirk for devices
> which require an identity mapping and then handle that in the IOMMU core?

Oh, don't think I *like* having quirks in the driver, it just seems like 
the least-worst choice from a bad bunch. All of the default domain 
quirks so far (including this one) exist for integrated devices and/or 
dodgy firmware setups such that they are platform-specific, so there is 
no technical reason for trying to split *some* of them off into a 
generic mechanism when the driver-based platform-specific mechanism 
still needs to exist anyway (some of them do depend on driver state as 
well).

Feel free to test the waters with a patch punting 
qcom_smmu_def_domain_type() to core code, but I think you'll struggle to 
find a reason to give in the commit message other than "I don't like it".

>> Ugly as it is, this is the status quo. I don't recall anyone ever arguing
>> that the equivalent quirks for Intel integrated graphics should be made
>> generic ;)
> 
> I don't know anything about Intel integrated graphics. Have they solved this
> problem in a better way, or could they equally make use of a generic quirk?

See intel-iommu's device_def_domain_type() implementation. The shape of 
it may seem quite familiar...

Robin.
Yicong Yang Feb. 16, 2022, 9:35 a.m. UTC | #10
On 2022/2/15 22:29, Robin Murphy wrote:
> On 2022-02-15 13:42, Will Deacon wrote:
>> On Tue, Feb 15, 2022 at 01:30:26PM +0000, Robin Murphy wrote:
>>> On 2022-02-15 13:00, Will Deacon wrote:
>>>> On Mon, Feb 14, 2022 at 08:55:20PM +0800, Yicong Yang wrote:
>>>>> On 2022/1/24 21:11, Yicong Yang wrote:
>>>>>> The DMA of HiSilicon PTT device can only work with identical
>>>>>> mapping. So add a quirk for the device to force the domain
>>>>>> passthrough.
>>>>>>
>>>>>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>>>>>> ---
>>>>>>    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++++
>>>>>>    1 file changed, 16 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>>>> index 6dc6d8b6b368..6f67a2b1dd27 100644
>>>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>>>> @@ -2838,6 +2838,21 @@ static int arm_smmu_dev_disable_feature(struct device *dev,
>>>>>>        }
>>>>>>    }
>>>>>> +#define IS_HISI_PTT_DEVICE(pdev)    ((pdev)->vendor == PCI_VENDOR_ID_HUAWEI && \
>>>>>> +                     (pdev)->device == 0xa12e)
>>>>>> +
>>>>>> +static int arm_smmu_def_domain_type(struct device *dev)
>>>>>> +{
>>>>>> +    if (dev_is_pci(dev)) {
>>>>>> +        struct pci_dev *pdev = to_pci_dev(dev);
>>>>>> +
>>>>>> +        if (IS_HISI_PTT_DEVICE(pdev))
>>>>>> +            return IOMMU_DOMAIN_IDENTITY;
>>>>>> +    }
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>>    static struct iommu_ops arm_smmu_ops = {
>>>>>>        .capable        = arm_smmu_capable,
>>>>>>        .domain_alloc        = arm_smmu_domain_alloc,
>>>>>> @@ -2863,6 +2878,7 @@ static struct iommu_ops arm_smmu_ops = {
>>>>>>        .sva_unbind        = arm_smmu_sva_unbind,
>>>>>>        .sva_get_pasid        = arm_smmu_sva_get_pasid,
>>>>>>        .page_response        = arm_smmu_page_response,
>>>>>> +    .def_domain_type    = arm_smmu_def_domain_type,
>>>>>>        .pgsize_bitmap        = -1UL, /* Restricted during device attach */
>>>>>>        .owner            = THIS_MODULE,
>>>>>>    };
>>>>>>
>>>>>
>>>>> Is this quirk ok with the SMMU v3 driver? Just want to confirm that I'm on the
>>>>> right way to dealing with the issue of our device.
>>>>
>>>> I don't think the quirk should be in the SMMUv3 driver. Assumedly, you would
>>>> have the exact same problem if you stuck the PTT device behind a different
>>>> type of IOMMU, and so the quirk should be handled by a higher level of the
>>>> stack.
>>>
>>> Conceptually, yes, but I'm inclined to be pragmatic here. Default domain
>>> quirks could only move out as far as the other end of the call from
>>> iommu_get_def_domain_type() - it's not like we could rely on some flag in a
>>> driver which may not even be loaded yet, let alone matched to the device.
>>> And even then there's an equal and opposite argument for why the core code
>>> should have to maintain a list of platform-specific quirks rather than code
>>> specific to the relevant platforms. The fact is that a HiSilicon RCiEP is
>>> not going to end up behind anything other than a HiSilicon IOMMU, and if
>>> those ever stop being SMMUv3 *and* such a quirk still exists we can worry
>>> about it then.
>>

That's true that this RCiEP only appears behind the HiSilicon's IOMMU which using
SMMU v3 driver.

>> Perhaps, but you know that by adding this hook it's only a matter of time
>> before we get random compatible string matches in there, so I'd rather keep
>> the flood gates closed as long as we can.
>>
>> Given that this is a PCI device, why can't we have a PCI quirk for devices
>> which require an identity mapping and then handle that in the IOMMU core?
> 

As Robin mentioned below, not only PCI devices but some platform devices also want
to passthrough the IOMMU. I noticed there're already some fields describe the device's
DMA information in struct device, so follow your point can it go there if we're going
to make it more generic?

Anyway if we're going to make all these quirks in a more generic place, I'll willing
to add this device there and have a test.

> Oh, don't think I *like* having quirks in the driver, it just seems like the least-worst choice from a bad bunch. All of the default domain quirks so far (including this one) exist for integrated devices and/or dodgy firmware setups such that they are platform-specific, so there is no technical reason for trying to split *some* of them off into a generic mechanism when the driver-based platform-specific mechanism still needs to exist anyway (some of them do depend on driver state as well).
> 
> Feel free to test the waters with a patch punting qcom_smmu_def_domain_type() to core code, but I think you'll struggle to find a reason to give in the commit message other than "I don't like it".
> 
>>> Ugly as it is, this is the status quo. I don't recall anyone ever arguing
>>> that the equivalent quirks for Intel integrated graphics should be made
>>> generic ;)
>>
>> I don't know anything about Intel integrated graphics. Have they solved this
>> problem in a better way, or could they equally make use of a generic quirk?
> 
> See intel-iommu's device_def_domain_type() implementation. The shape of it may seem quite familiar...
> 

Yes Intel's IOMMU passthrough some PCI devices in this way and this patch imitates that.
https://github.com/torvalds/linux/blob/master/drivers/iommu/intel/iommu.c#L2959

btw Will, much appreciated if you could have a look at the perf and PMU part of this driver.:)

Thanks,
Yicong
diff mbox series

Patch

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 6dc6d8b6b368..6f67a2b1dd27 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2838,6 +2838,21 @@  static int arm_smmu_dev_disable_feature(struct device *dev,
 	}
 }
 
+#define IS_HISI_PTT_DEVICE(pdev)	((pdev)->vendor == PCI_VENDOR_ID_HUAWEI && \
+					 (pdev)->device == 0xa12e)
+
+static int arm_smmu_def_domain_type(struct device *dev)
+{
+	if (dev_is_pci(dev)) {
+		struct pci_dev *pdev = to_pci_dev(dev);
+
+		if (IS_HISI_PTT_DEVICE(pdev))
+			return IOMMU_DOMAIN_IDENTITY;
+	}
+
+	return 0;
+}
+
 static struct iommu_ops arm_smmu_ops = {
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,
@@ -2863,6 +2878,7 @@  static struct iommu_ops arm_smmu_ops = {
 	.sva_unbind		= arm_smmu_sva_unbind,
 	.sva_get_pasid		= arm_smmu_sva_get_pasid,
 	.page_response		= arm_smmu_page_response,
+	.def_domain_type	= arm_smmu_def_domain_type,
 	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
 	.owner			= THIS_MODULE,
 };