RE: [RFC PATCH v2 09/20] hw/arm/smmuv3-accel: Add set/unset_iommu_device callback

[Shameerali Kolothum Thodi via <qemu-arm@nongnu.org>]

Hi Nicolin,

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Tuesday, March 11, 2025 9:08 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
> ddutile@redhat.com; berrange@redhat.com; nathanc@nvidia.com;
> mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@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