Hi Nicolin,
> -----Original Message-----
> From: Nicolin Chen <nicol...@nvidia.com>
> Sent: Tuesday, March 11, 2025 9:08 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com>
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org;
> eric.au...@redhat.com; peter.mayd...@linaro.org; j...@nvidia.com;
> ddut...@redhat.com; berra...@redhat.com; nath...@nvidia.com;
> mo...@nvidia.com; smost...@google.com; Linuxarm
> <linux...@huawei.com>; Wangzhou (B) <wangzh...@hisilicon.com>;
> jiangkunkun <jiangkun...@huawei.com>; Jonathan Cameron
> <jonathan.came...@huawei.com>; zhangfei....@linaro.org
> Subject: Re: [RFC PATCH v2 09/20] hw/arm/smmuv3-accel: Add
> set/unset_iommu_device callback
>
> On Tue, Mar 11, 2025 at 02:10:34PM +0000, Shameer Kolothum wrote:
> > @@ -30,6 +32,185 @@ static SMMUv3AccelDevice
> *smmuv3_accel_get_dev(SMMUState *s, SMMUPciBus *sbus,
> > return accel_dev;
> > }
> >
> > +static bool
> > +smmuv3_accel_dev_attach_viommu(SMMUv3AccelDevice *accel_dev,
> > + HostIOMMUDeviceIOMMUFD *idev, Error **errp)
>
> With vEVENTQ v9, vDEVICE (vSID) is required to attach a device
> to a proxy NESTED hwpt (applicable to bypass/abort HWPTs too).
> So, host_iommu_device_iommufd_attach_hwpt() would fail in this
> function because vSID isn't ready at this stage. So all those
> calls should be moved out of the function, then this should be
> likely "smmuv3_accel_dev_alloc_viommu"?
>
> That being said, I don't know when QEMU actually prepare a BDF
> number for a vfio-pci device. The only place that I see it is
> ready is at guest-level SMMU installing the Stream Table, i.e.
> in smmuv3_accel_install_nested_ste().
>
> > +{
> > + struct iommu_hwpt_arm_smmuv3 bypass_data = {
> > + .ste = { 0x9ULL, 0x0ULL },
> > + };
> > + struct iommu_hwpt_arm_smmuv3 abort_data = {
> > + .ste = { 0x1ULL, 0x0ULL },
> > + };
> > + SMMUDevice *sdev = &accel_dev->sdev;
> > + SMMUState *s = sdev->smmu;
> > + SMMUv3AccelState *s_accel = ARM_SMMUV3_ACCEL(s);
> > + SMMUS2Hwpt *s2_hwpt;
> > + SMMUViommu *viommu;
> > + uint32_t s2_hwpt_id;
> > + uint32_t viommu_id;
> > +
> > + if (s_accel->viommu) {
> > + accel_dev->viommu = s_accel->viommu;
> > + return host_iommu_device_iommufd_attach_hwpt(
> > + idev, s_accel->viommu->s2_hwpt->hwpt_id, errp);
>
> Yea, here is my bad. We shouldn't attach a device to s2_hwpt,
> since eventually s2_hwpt would be a shared hwpt across SMMUs.
>
> > + /* Attach to S2 for MSI cookie */
> > + if (!host_iommu_device_iommufd_attach_hwpt(idev, s2_hwpt_id,
> errp)) {
> > + goto free_s2_hwpt;
> > + }
>
> With the merged sw_msi series, we don't need this anymore.
>
> > + /*
> > + * Attach the bypass STE which means S1 bypass and S2 translate.
> > + * This is to make sure that the vIOMMU object is now associated
> > + * with the device and has this STE installed in the host SMMUV3.
> > + */
> > + if (!host_iommu_device_iommufd_attach_hwpt(
> > + idev, viommu->bypass_hwpt_id, errp)) {
> > + error_report("failed to attach the bypass pagetable");
> > + goto free_bypass_hwpt;
> > + }
>
> Ditto. We have to postpone this until vdevice is allocated.
Ok. I will take a look based on the vEVENTQ v9 series.
I guess this Qemu branch of yours is a more representative of the changes
described
above?
https://github.com/nicolinc/qemu/tree/wip/for_iommufd_veventq-v9
Right?
Thanks,
Shameer
> > +static void smmuv3_accel_unset_iommu_device(PCIBus *bus, void
> *opaque,
> > + int devfn)
> > +{
> > + SMMUDevice *sdev;
> > + SMMUv3AccelDevice *accel_dev;
> > + SMMUViommu *viommu;
> > + SMMUState *s = opaque;
> > + SMMUv3AccelState *s_accel = ARM_SMMUV3_ACCEL(s);
> > + SMMUPciBus *sbus = g_hash_table_lookup(s-
> >smmu_pcibus_by_busptr, bus);
> > +
> > + if (!sbus) {
> > + return;
> > + }
> > +
> > + sdev = sbus->pbdev[devfn];
> > + if (!sdev) {
> > + return;
> > + }
> > +
> > + accel_dev = container_of(sdev, SMMUv3AccelDevice, sdev);
> > + if (!host_iommu_device_iommufd_attach_hwpt(accel_dev->idev,
> > + accel_dev->idev->ioas_id,
> > + NULL)) {
> > + error_report("Unable to attach dev to the default HW pagetable");
> > + }
> > +
> > +
>
> Could drop the extra line.
>
> Thanks
> Nicolin
