Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 11 Dec 2023 16:08:11 +0800 | Subject | Re: [PATCH 3/3] vfio: Report PASID capability via VFIO_DEVICE_FEATURE ioctl | From | Yi Liu <> |
| |
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
| ![\](/images/icornerr.gif) |