Message-ID: <3877b6fe-2b7e-3e0c-8b69-65e84c09cdd2@os.amperecomputing.com>
Date:   Thu, 16 Feb 2023 17:06:49 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.2
Subject: Re: [PATCH] PCI/ATS: Allow to enable ATS on VFs even if it is not
 enabled on PF
To:     Jean-Philippe Brucker <jean-philippe@linaro.org>,
        Bjorn Helgaas <helgaas@kernel.org>
Cc:     Leon Romanovsky <leon@kernel.org>, linux-kernel@vger.kernel.org,
        linux-pci@vger.kernel.org, bhelgaas@google.com,
        darren@os.amperecomputing.com, scott@os.amperecomputing.com,
        Will Deacon <will@kernel.org>,
        Robin Murphy <robin.murphy@arm.com>,
        Joerg Roedel <joro@8bytes.org>,
        linux-arm-kernel@lists.infradead.org, iommu@lists.linux.dev
References: <Y+ksmNWJdWNkGAU9@unreal> <20230215205726.GA3213227@bhelgaas>
 <Y+4PmJb2rBGMhS1y@myrica>
Content-Language: en-US
From:   Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
In-Reply-To: <Y+4PmJb2rBGMhS1y@myrica>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QnVNLzZlc1d1UGRldXMvcWVTTVA0c3QyMTc3Q09yUk5kWlBnN0llb1VWL3JK?=
 =?utf-8?B?bUlSWW10TkdJalFOYm0vTkh0cjFlbk95TmJEbmNMMHlJTDZGSXhnOU5iVHY5?=
 =?utf-8?B?U3JOZmZoTitDT0k1bXEwUjNoQVNrOE9nWVBPVU85YmJrWWtZdlQ2THd0dnBU?=
 =?utf-8?B?QUxrRklQWkxROWE3ckRGbENGUy9pSEZMamlpeWRYK09uWHBydGRmNDdnQ2R4?=
 =?utf-8?B?c1ZHK3hYeUFicUpDTHlkSHBoWTRRRk5pWUhiTFhIUC9MY0EyTlJmekp0d2JM?=
 =?utf-8?B?MWsxTVBYQnUxMkVLWU9JOWczYkUyYkVtNyt3blJ5QmZrMW5kcVhWWDN2U0tN?=
 =?utf-8?B?Z0E0ZCt3S09pbkJzSlhWbS9HYWp6WG9LZElXUGROY0VUR2V4cXh4TFVtOWxq?=
 =?utf-8?B?UlpSczltb09XeitKVmg2aTV4dm9iUlFNRW5xTklRT2ZZVHo3T0tsYy9iYW1q?=
 =?utf-8?B?U1V2aXRCeFZaK0p3Y29yV0ViQ2ptU0E3SEZiREZ5K2pVWU9vdkdXdU5uY3RT?=
 =?utf-8?B?UDlVaFo2SDZEdzREdWdCUy9wQTUreTlqUzVvVjV6MEVEbkVVWW9pWGNmL0gw?=
 =?utf-8?B?cjYwT3hqbEVlaktSbDI3RmR5NlhpWDMrM3pZUmJxNHdEbzFMWHpTTitOQ2hr?=
 =?utf-8?B?VjFVcXFXNFpiNG5EZ0RsN21KWE5iN3duZG9SUm03emJUdzkycENVU0orek81?=
 =?utf-8?B?U2RhelJDOStia2tKTEVCQW1BK081YmhGT25EdW5mY055TnBoQ3RxRTMyNy8z?=
 =?utf-8?B?SmlZYU9laThSLytUMDJCUjdOQ1NOQkhFdnhvZjg1OXpzaDl5QVhwZkV5RTNP?=
 =?utf-8?B?Q2U2WlQ5SlNENlJmR1dBQUZkbytqa2R5SmpFNVRIQlBxenN5L1lXQmlVc01Z?=
 =?utf-8?B?RGpad0lBVUk0Y1Q1R1VScjlZMnkvQ3RnTXpQWHFaQndEcDIyNE5LZy9rZXFo?=
 =?utf-8?B?Zmw3LzB2by95Q0pVYXF6elJsUFhiZXV0QVZCU1lXN3M1Z3p1a3JGRTQ5Snh0?=
 =?utf-8?B?M0txS1pGTnBZYnA4OEhEZ2FJZnFoQkVWb2lCT1RISkh1UmtZUTBaelVtNGRL?=
 =?utf-8?B?U3RieDBhdDQxa1BKWXZjaWdQYXpVZnRXQzVHT1hiMU1jZTNNVmRZamVVK0U5?=
 =?utf-8?B?c045dGY1WWVZQTB3L0xnTmNMdUxCOEFVaEZpbjBZemd6Z2U5SU5uL3VOOElF?=
 =?utf-8?B?TW01b2Y0b3l6L2JHRXJBdXptZUNpNnpKeEM4ckhIMlhncUZ1U3U3eWR2dDVq?=
 =?utf-8?B?MExhWFZJeFRLSDJOK0RVVEhjQmRtZjExUGdURVdjMmUzTm1NcVFXZ3FFRW93?=
 =?utf-8?B?bjliNWl0TkFjUVBDMDU5M2kxK2xTZ3pHTkdsTTZyVC8wTnhrNWJuM1JuU2hY?=
 =?utf-8?B?czkxdmtxSFZCSzJxTjlaSUd4Sk9nZG9WN2VWcVlSeklUcmtNTlJ3WVpjdWNS?=
 =?utf-8?B?VlplN3lGL3JnOXBaT1Q3NVc0d2taRHMraFJxTXR1UEpKMHFqNUZhY1FzUm1W?=
 =?utf-8?B?ak4ybm4wSElUa0drSHdBMDFCNTQvbWoxWUpGdlhvQzA2QTZRalc4QWdWVldM?=
 =?utf-8?B?WEhxQnZqUXZleThYcmlvRTN3WVFnVVJ5T1haZDRVK21hM0tVczdoMGloa3li?=
 =?utf-8?B?QWtnZlc0U2xRYzVTNk84T3A2SjV6WWF1YVhwN1RSKzMxTWthS1EzdlpIZFhC?=
 =?utf-8?B?UGJqb05XK3Uwa1VoTmhPOUR3WXlnNWpPQ28zSDVpajl5MmM2aWhWUnhJbFNl?=
 =?utf-8?B?bGl3SW10V1B4K1UzZmdHOFh5WFR3QnBwc1ZZSGw0b0svR0ZxUVBCdmNYQkFJ?=
 =?utf-8?B?RHNaVmRqTGRuUFFrTGREUWNVRUxid0N1dVRBYXpQczdMazdhdk40QTJqRy9p?=
 =?utf-8?B?MTJQeXV6WlpBVHc0cmxxS2Q4YmpqLzQ1dnBIcU9vVG4yZXpwWG96c2FVbTAw?=
 =?utf-8?B?NGUwZ2hIV0NxalZkSTArY0d0ZnJSYXQ3SmRpWjk3b01OVVNpSHdraGVMbExt?=
 =?utf-8?B?NzdiNGd3VUM0MjYwVnRRQ00ydU5tZVhzVVVrRko0VlhlVWZML3pNR3BzTFpz?=
 =?utf-8?B?S21JUVZrS3NseUN2QTVZM0RRR0JkeEgyZFNLbXpKRTN1QklPOU4rdWY0YWRQ?=
 =?utf-8?B?N2Y4L0ZFbXVyTUpFdVVQcDlTbWk2WGp1emVmR3MwZFdIcjBKTS83YysxMkhU?=
 =?utf-8?Q?SzsMj/P8kjBFTH2zi434c9+Ycz2EcMCKCnl+9oBhIoHZ?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fe293ba1-dd45-4c2b-925e-08db10121bd8
X-MS-Exchange-CrossTenant-AuthSource: DM8PR01MB6824.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Feb 2023 11:36:56.6628
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: W4X2cCmsV5Jvm1TZmWqoqGsJrPhTHbmmyQva+/PtaFRrIlF8AVkE+6mKeRaAHTA6K3GRgEYN2IDkXOKsK0yvfDjB2+U1y542M2xpj8bW9j27A7j9zxnh9GdPqxgHV3Eg
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR01MB4029
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 16-02-2023 04:42 pm, Jean-Philippe Brucker wrote:
> On Wed, Feb 15, 2023 at 02:57:26PM -0600, Bjorn Helgaas wrote:
>> [+cc Will, Robin, Joerg for arm-smmu-v3 page size question]
>>
>> On Sun, Feb 12, 2023 at 08:14:48PM +0200, Leon Romanovsky wrote:
>>> On Wed, Feb 08, 2023 at 10:43:21AM -0800, Ganapatrao Kulkarni wrote:
>>>> As per PCIe specification(section 10.5), If a VF implements an
>>>> ATS capability, its associated PF must implement an ATS capability.
>>>> The ATS Capabilities in VFs and their associated PFs are permitted to
>>>> be enabled independently.
>>>> Also, it states that the Smallest Translation Unit (STU) for VFs must be
>>>> hardwired to Zero and the associated PF's value applies to VFs STU.
>>>>
>>>> The current code allows to enable ATS on VFs only if it is already
>>>> enabled on associated PF, which is not necessary as per the specification.
>>>>
>>>> It is only required to have valid STU programmed on PF to enable
>>>> ATS on VFs. Adding code to write the first VFs STU to a PF's STU
>>>> when PFs ATS is not enabled.
>>>
>>> Can you please add here quotes from the spec and its version? I don't see
>>> anything like this in my version of PCIe specification.
>>
>> See PCIe r6.0, sec 10.5.1.
>>
>>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>>>> ---
>>>>   drivers/pci/ats.c | 15 +++++++++++----
>>>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>>>> index f9cc2e10b676..a97ec67201d1 100644
>>>> --- a/drivers/pci/ats.c
>>>> +++ b/drivers/pci/ats.c
>>>> @@ -67,13 +67,20 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
>>>>   	if (ps < PCI_ATS_MIN_STU)
>>>>   		return -EINVAL;
>>>>   
>>>> -	/*
>>>> -	 * Note that enabling ATS on a VF fails unless it's already enabled
>>>> -	 * with the same STU on the PF.
>>>> -	 */
>>>>   	ctrl = PCI_ATS_CTRL_ENABLE;
>>>>   	if (dev->is_virtfn) {
>>>>   		pdev = pci_physfn(dev);
>>>> +
>>>> +		if (!pdev->ats_enabled &&
>>>> +				(pdev->ats_stu < PCI_ATS_MIN_STU)) {
>>>> +			u16 ctrl2;
>>>> +
>>>> +			/* Associated PF's STU value applies to VFs. */
>>>> +			pdev->ats_stu = ps;
>>>> +			ctrl2 = PCI_ATS_CTRL_STU(pdev->ats_stu - PCI_ATS_MIN_STU);
>>>> +			pci_write_config_word(pdev, pdev->ats_cap + PCI_ATS_CTRL, ctrl2);
>>>> +		}
>>
>> For reference, it is this way because of edc90fee916b ("PCI: Allocate
>> ATS struct during enumeration").  The rationale was that since the PF
>> STU applies to all VFs, we should require that the PF STU be
>> programmed before enabling ATS on any of the VFs.
>>
>> This patch relaxes that so the PF STU would be set either by (a)
>> enabling ATS on the PF or (b) enabling ATS on the first VF.
>>
>> This looks racy because theoretically drivers for VF A and VF B could
>> independently call pci_enable_ats() with different IOMMU page sizes,
>> and we don't know which will get there first.
>>
>> Most callers supply a compile-time constant (PAGE_SHIFT or
>> VTD_PAGE_SHIFT), so it won't matter.  arm_smmu_enable_ats() is
>> fancier, but I *assume* it would still supply the same IOMMU page size
>> for all VFs of a given PF.
>>
>> But it's still kind of ugly to call pci_enable_ats(dev_A) and have it
>> muck with the configuration of dev_B.  Maybe we should configure the
>> PF STU (without enabling ATS) at enumeration-time in pci_ats_init()?
>> Is there some way to get the IOMMU page size at that time?
> 
> Not really, on Arm the supported page sizes are discovered when probing
> the SMMU registers, which may happen later than enumeration, during module
> loading.
> 
> What this patch is trying to solve is:
> * want the PF to bypass SMMU translation, and the VF to undergo SMMU
>    translation (in order to assign it to a VM)
> * SMMU forbids enabling ATS for a configuration that bypasses translation.
>    So the PF ATS capability must be left disabled.
> 
> For this situation I wonder if we could do: the SMMU driver, seeing that
> the PF is configured to bypass, calls a new function "pci_configure_ats()"

IMO, This seems to be feasible solution for this situation, which 
addresses SMMU-ATS expectation in bypass and we could avoid PCI VFs 
race. pci_configure_ats() can be called to program/configure STU of a PF 
in smmu-bypass mode.

> instead of pci_enable_ats(), which would only set the STU but leave the
> cap disabled. Then when setting up translation for the VF, the SMMU driver
> calls pci_enable_ats() as usual, which sees the PF's STU set appropriately
> and succeeds.
> 
> Thanks,
> Jean

Thanks,
Ganapat