Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <0bdae2ca-a200-4db1-a016-059730d1545e@intel.com>
Date:   Mon, 11 Dec 2023 16:08:11 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 3/3] vfio: Report PASID capability via VFIO_DEVICE_FEATURE
 ioctl
Content-Language: en-US
To:     "Tian, Kevin" <kevin.tian@intel.com>,
        "joro@8bytes.org" <joro@8bytes.org>,
        "alex.williamson@redhat.com" <alex.williamson@redhat.com>,
        "jgg@nvidia.com" <jgg@nvidia.com>,
        "robin.murphy@arm.com" <robin.murphy@arm.com>,
        "baolu.lu@linux.intel.com" <baolu.lu@linux.intel.com>
CC:     "cohuck@redhat.com" <cohuck@redhat.com>,
        "eric.auger@redhat.com" <eric.auger@redhat.com>,
        "nicolinc@nvidia.com" <nicolinc@nvidia.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "mjrosato@linux.ibm.com" <mjrosato@linux.ibm.com>,
        "chao.p.peng@linux.intel.com" <chao.p.peng@linux.intel.com>,
        "yi.y.sun@linux.intel.com" <yi.y.sun@linux.intel.com>,
        "peterx@redhat.com" <peterx@redhat.com>,
        "jasowang@redhat.com" <jasowang@redhat.com>,
        "shameerali.kolothum.thodi@huawei.com" 
        <shameerali.kolothum.thodi@huawei.com>,
        "lulu@redhat.com" <lulu@redhat.com>,
        "suravee.suthikulpanit@amd.com" <suravee.suthikulpanit@amd.com>,
        "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-kselftest@vger.kernel.org" <linux-kselftest@vger.kernel.org>,
        "Duan, Zhenzhong" <zhenzhong.duan@intel.com>,
        "joao.m.martins@oracle.com" <joao.m.martins@oracle.com>,
        "Zeng, Xin" <xin.zeng@intel.com>,
        "Zhao, Yan Y" <yan.y.zhao@intel.com>
References: <20231127063909.129153-1-yi.l.liu@intel.com>
 <20231127063909.129153-4-yi.l.liu@intel.com>
 <BN9PR11MB527639DBE4C433542F351F6D8C8BA@BN9PR11MB5276.namprd11.prod.outlook.com>
From:   Yi Liu <yi.l.liu@intel.com>
In-Reply-To: <BN9PR11MB527639DBE4C433542F351F6D8C8BA@BN9PR11MB5276.namprd11.prod.outlook.com>
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?VXpOWk43cjVpa0UxV25qQndGdGphNHlVN3R6RUxCNkJUTmxDcHplelAzR0xF?=
 =?utf-8?B?b2Q2Q0M3a291dmg0YzVQT0l0N2dUQ0VMU1ZJSnJ6cHR1dmo5cDJVQXBwVHVQ?=
 =?utf-8?B?cTJOZUZ1cjVidG1oeFZZTjhzbTJwaXR5aE1ySkw2MnBBb1JDODVucjExN0Ry?=
 =?utf-8?B?Zzk1WlFyR1FXQXhBaHdFdFk5R29pMU9GWncxT2E4Z21SUG0xRTlSWTJNMlhG?=
 =?utf-8?B?SmhvZU9WSzFzK2FtVHZCcWo4Q1lUMmg2d1RJYjhkTytDb2VBaXozdzZBREdy?=
 =?utf-8?B?d0haOVFSNU5VLzlod3VTQXV0bnBqbkNRQVdkV2tOK2Q5RE00TENuMDJCbmg5?=
 =?utf-8?B?bTNKRHQ0ZVJZVm5wQVd5Vm9DVzhuY1hDa0wvaU5IS1JvR1NCeWJhLzlOZG5q?=
 =?utf-8?B?bGxnMzBFb21veW45cVpjRExmQ2xSMDFQZFJobUFqaHVWNGM5WWtHSm1HTGRi?=
 =?utf-8?B?T01WQzM5WTRtRWY4NnhUS3YxR2NRZDBjQmN2aGVaRkoraUJrM2t6ZmoyTHFl?=
 =?utf-8?B?SHlsOUJsTkx0VVBLSW9mSkV0UFZSbFZkOC9lQkdsanJuK0RENzhyN0ZpMldG?=
 =?utf-8?B?S0g4MndZVDI0K3FLakVBQzRucGo5Vm5MMDUzZCt2ZzJaVlBXaEZLenM3QXVH?=
 =?utf-8?B?cUtwN3Y1NEZhNU9lWlZGelZCWDBNRWhVSHUwVEp0WkV0YXE4czFQUi92MW5L?=
 =?utf-8?B?SExPcXh6MHJPdEZvRm1CWXBvb0hiSUZmWDZZT3NaSjJQb0xjY3NyNSs2SzBs?=
 =?utf-8?B?bStUbEFUbDVaM0hWTFlheENYUmZESHp4OGk4WTlWdk8vWFp1TGdZZHB4b3pU?=
 =?utf-8?B?MHNORjVJb3ZZbWlSOWgwSEo3ZDU4T3l5SnVENUNocnJvdHE5QVJvdHZpWVBq?=
 =?utf-8?B?eHFuY3FReVlxbnRMcWI2eXRuakh3MHdFaWdXeXFTdDFPUHVGbCtLSjBZbWxT?=
 =?utf-8?B?ZWhHNEpUbW9MZUFIdmgvWURJTVd1TC8xcmJrSWkrazVncTBUekM2TTMvbFRu?=
 =?utf-8?B?cjZLRjdIUkVyYk5hK2U5Y1YzYy9WbDFOTEFBUUdmZm02NHZsNStiUk5SY1Ju?=
 =?utf-8?B?YzV5Y2cxWHZOMmxHbXRNa01yQmJQc2F1T084Ui9xOHdHNTZUL0pDYTg4K2My?=
 =?utf-8?B?K2pyL3hQdzdtZ2VjSFFBZmNPbFBnYnFHTG9lcTZKenAzbTJtcWlkRXoreFM5?=
 =?utf-8?B?cHVFOFJ3ck45bjdiMHFOSStDc0lwdUdPbWk4dzVQVzRmTEswQ1grYmRPZVp2?=
 =?utf-8?B?M1JuWkFsdVExUHplR0V3d1JRUzRaRGxSVnVDRFY0T0xEM2FpdE52UFBTNTBn?=
 =?utf-8?B?UVFsVjJvZFAvM3VCZTI0QmoyTXROM01kNnJHYXVvbXJsOXliMzBJWXVTbytw?=
 =?utf-8?B?LzFGbzVCV3htUmxMNHJIVk1aQmI5MGtYanF5UE0wUnlGRWxPK24yOUQ5ZUR2?=
 =?utf-8?B?ZjBvRzhxRW1JbmN6YmhKZ0NZbWdLc2VUcFRvWFhabGRuTHRZYUh0VHBSV0Fs?=
 =?utf-8?B?ekc2TFp6VzRReUNrd3VRNG5VMGNvUHdXbERJWVRjLzRsRU1KcituNzVYYmlj?=
 =?utf-8?B?TzdTRklqcTNXK0tLTi9ZdmdYZlFaK3Rhbnd5ZDR3S1hjT2dKTUZZL3BaaGwv?=
 =?utf-8?B?eFBreXJ5ajJSODZNYnY0dEJVbUJPNUtVaStsVG12KzBXdU1hdVdPOUh2Nnpa?=
 =?utf-8?B?ZlVtcEZhR0JDRzlsaURIekc4dTg5elBxTlRLYlNlbXFob0IxTDFvR1BNdUcz?=
 =?utf-8?B?S1p1SGY3WnA2dFV5d3FUN0tqMW9ZM2VHdVN1aEltR3hLS3dhOTA0ZUpzQi91?=
 =?utf-8?B?ZTlGSmZGMnNVckoxSzFTTmx6QnlSV2JCSHhWKzFUM1cwOE5mY2JUY3RGV2dO?=
 =?utf-8?B?bzhtVUVQSTFwTUtXREZzMHJpeVBoSFZSWkZjcjJhMyt1Q29aVWl3ZnR1RnY2?=
 =?utf-8?B?b0J2RjlTdllONWM1elJWMFk4UnRGbDRKZWo4QnJjZ0VvamdtRi9JOXV3d2xC?=
 =?utf-8?B?RDJiSnRyRjRGb3pHM3Zha3Q1N3B4RS9kR2ZBQVpFQTdpMXpKWlZNRmdLWGgx?=
 =?utf-8?B?Vi9uSnd2NWQ1VGV0Q01XVVpUcWpRK3R2cmVaQVU3NGlSOEhMV242emhqY05w?=
 =?utf-8?Q?0ZW1MoEjOkDCCbiLszj7tjjJA?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1993d188-5b0a-46d9-c3e6-08dbfa1ff615
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB7529.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Dec 2023 08:05:38.5693
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: mUXijNr7TdyX/zVkiGfBD54T2C1oPGmD4/eISDQI0FJKjF8NGFMzOrohHet3md6PQlkpxYBOjsxVT7h2UkD1sw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7933
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Mon, 11 Dec 2023 00:05:58 -0800 (PST)

On 2023/12/7 16:47, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Monday, November 27, 2023 2:39 PM
>>
>> +static int vfio_pci_core_feature_pasid(struct vfio_device *device, u32 flags,
>> +				       struct vfio_device_feature_pasid __user
>> *arg,
>> +				       size_t argsz)
>> +{
>> +	struct vfio_pci_core_device *vdev =
>> +		container_of(device, struct vfio_pci_core_device, vdev);
>> +	struct vfio_device_feature_pasid pasid = { 0 };
>> +	struct pci_dev *pdev = vdev->pdev;
>> +	u32 capabilities = 0;
>> +	int ret;
>> +
>> +	/* We do not support SET of the PASID capability */
> 
> this line alone is meaningless. Please explain the reason e.g. due to
> no PASID capability per VF...

sure. I think the major reason is we don't allow userspace to change the
PASID configuration. is it?

> 
>> +	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_GET,
>> +				 sizeof(pasid));
>> +	if (ret != 1)
>> +		return ret;
>> +
>> +	/*
>> +	 * Needs go to PF if the device is VF as VF shares its PF's
>> +	 * PASID Capability.
>> +	 */
> 
> /* VF shares the PASID capability of its PF */

got it.

>> +	if (pdev->is_virtfn)
>> +		pdev = pci_physfn(pdev);
>> +
>> +	if (!pdev->pasid_enabled)
>> +		goto out;
>> +
>> +#ifdef CONFIG_PCI_PASID
>> +	pci_read_config_dword(pdev, pdev->pasid_cap + PCI_PASID_CAP,
>> +			      &capabilities);
>> +#endif
> 
> #ifdef is unnecessary. If CONFIG_PCI_PASID is false pdev->pasid_enabled
> won't be set anyway.

it's sad that the pdev->pasid_cap is defined under #if CONFIG_PCI_PASID.
Perhaps we can have a wrapper for it.

> and it should read from PCI_PASID_CTRL which indicates whether a
> capability is actually enabled.

yes, for the EXEC and PRIV capability, needs to check if it's enabled or
not before reporting.

> 
>> +/**
>> + * Upon VFIO_DEVICE_FEATURE_GET, return the PASID capability for the
>> device.
>> + * Zero width means no support for PASID.
> 
> also mention the encoding of this field according to PCIe spec.

yes.

> or turn it to a plain number field.

It is not exact the same as the spec since bit0 is reserved. But
here bit0 is used as well.

>> + */
>> +struct vfio_device_feature_pasid {
>> +	__u16 capabilities;
>> +#define VFIO_DEVICE_PASID_CAP_EXEC	(1 << 0)
>> +#define VFIO_DEVICE_PASID_CAP_PRIV	(1 << 1)
>> +	__u8 width;
>> +	__u8 __reserved;
>> +};
>> +#define VFIO_DEVICE_FEATURE_PASID 11
>> +
>>   /* -------- API for Type1 VFIO IOMMU -------- */
>>
>>   /**
>> --
>> 2.34.1
> 

-- 
Regards,
Yi Liu