Re: [PATCH 16/24] iommu/amd: Introduce IOMMUFD vDevice support for AMD

> [ ... 56 lines skipped ... ]
> +/*
> + * Called from drivers/iommu/iommufd/viommu.c: iommufd_vdevice_alloc_ioctl()
> + */
> +static int _amd_viommu_vdevice_init(struct iommufd_vdevice *vdev)
> +{
> +	struct iommu_dev_data *dev_data;
> +	struct pci_dev *pdev = to_pci_dev(vdev->idev->dev);
> +	struct iommufd_viommu *viommu = vdev->viommu;
> +	struct amd_iommu_viommu *aviommu = container_of(viommu, struct amd_iommu_viommu, core);
> +
> +	if (!pdev) {
> +		pr_err("%s: not a PCI device\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	dev_data = dev_iommu_priv_get(&pdev->dev);
> +	if (!dev_data) {
> +		pr_err("%s: Device not found (devid=%#x)\n",
> +		       __func__, pci_dev_id(pdev));
> +		return -EINVAL;
> +	}
> +
> +	pr_debug("%s: gid=%#x, hdev_id=%#x, gdev_id=%#llx\n", __func__,
> +		 aviommu->gid, pci_dev_id(pdev), vdev->virt_id);

What is the point of this function? It doesn't do anything, and you
cannot have a vdevice that isn't already bound to the iommu. Please
don't add random !dev_data checks where it is impossible

> [ ... 66 lines skipped ... ]
> +
> +	/* vImuEn */
> +	new->data[3] |= 1ULL << DTE_VIOMMU_EN_SHIFT;
> +
> +	/* GDeviceID */
> +	new->data[3] |= FIELD_PREP(DTE_VIOMMU_GDEVICEID_MASK, (u16)gDevId);

No need for a type cast?

> +
> +	/* GuestID */
> +	new->data[3] |= FIELD_PREP(DTE_VIOMMU_GUESTID_MASK, gid);

The comments are a little redundant with the field names :)

But the dte construction flow looks OK now

-- 
Jason