On Fri, 19 Sept 2025 at 08:38, Shameer Kolothum <skolothum...@nvidia.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Nicolin Chen <nicol...@nvidia.com>
> > Sent: 18 September 2025 23:00
> > To: Shameer Kolothum <skolothum...@nvidia.com>
> > Cc: Shameer Kolothum <shameerkolot...@gmail.com>; qemu-
> > a...@nongnu.org; qemu-devel@nongnu.org; eric.au...@redhat.com;
> > peter.mayd...@linaro.org; Jason Gunthorpe <j...@nvidia.com>;
> > ddut...@redhat.com; berra...@redhat.com; Nathan Chen
> > <nath...@nvidia.com>; Matt Ochs <mo...@nvidia.com>;
> > smost...@google.com; linux...@huawei.com; wangzh...@hisilicon.com;
> > jiangkun...@huawei.com; jonathan.came...@huawei.com;
> > zhangfei....@linaro.org; zhenzhong.d...@intel.com
> > Subject: Re: [RFC PATCH v3 06/15] hw/arm/smmuv3-accel: Restrict
> > accelerated SMMUv3 to vfio-pci endpoints with iommufd
> >
> > On Thu, Sep 18, 2025 at 06:31:43AM -0700, Shameer Kolothum wrote:
> > > > > > @@ -37,7 +37,6 @@ typedef struct SMMUS1Hwpt {
> > > > > >
> > > > > > typedef struct SMMUv3AccelDevice {
> > > > > > SMMUDevice sdev;
> > > > > > - AddressSpace as_sysmem;
> > > > > > HostIOMMUDeviceIOMMUFD *idev;
> > > > > > SMMUS1Hwpt *s1_hwpt;
> > > > > > SMMUViommu *viommu;
> > > > > > @@ -48,6 +47,7 @@ typedef struct SMMUv3AccelDevice { typedef
> > struct
> > > > > > SMMUv3AccelState {
> > > > > > MemoryRegion root;
> > > > > > MemoryRegion sysmem;
> > > > > > + AddressSpace as_sysmem;
> > > > > > SMMUViommu *viommu;
> > > > > > struct iommu_hw_info_arm_smmuv3 info; } SMMUv3AccelState;
> > > > >
> > > > > That's changing from an ioas_id per VFIO device to an ioas_id per
> > > > > vSMMU instance, right? I think it's still not enough.
> > > > >
> > > > > All vSMMU instances could share the same ioas_id. That is why I put in
> > > > > the SMMUBaseClass as it's shared structure across vSMMUs.
> > > >
> > > > Ah..you mean it is basically per VM then. Got it.
> > >
> > > Regarding using SMMUBaseClass for this, it looks like ObjectClass normally
> > holds
> > > function pointers. Eric has made a similar observation elsewhere in this
> > series.
> > >
> > > @Eric, any suggestions?
> > >
> > > Is use of &address_space_memory directly in smmuv3_accel_find_add_as()
> > a complete
> > > no-go, given we are talking about having the system address space for all
> > > the
> > SMMUv3
> > > accelerated devices here?
> >
> > smmuv3_accel_find_add_as() is per instance. So, it wouldn't share.
>
> My suggestion was...
>
> static AddressSpace *smmuv3_accel_find_add_as(..)
> {
> ...
> if (vfio_pci) {
> return &address_space_memory;
> } else {
> return &sdev->as;
> }
> }
>
> ie, use the global to system memory address space instead of creating an
> alias to the system memory and a different address space. This will provide
> the same pointer to VFIO/iommufd and it can then reuse the ioas_id.
> I can see that QEMU uses "&address_space_memory" directly in many places
> (pci_device_iommu_address_space(), etc). I think the idea behind a separate
> address space is to have private ownership and lifetime management probably.
> Not sure there are any other concerns here. Please let me know if there are
> any.
I don't know about vfio, but why is it special here? Generally
directly doing stuff with address_space_memory is not a good
idea : you should be dealing with whatever address space the
board model handed you as what you do transactions to. We
have a lot of legacy code that assumes it can directly work
with address_space_memory, but we should usually try to avoid
adding new code that does that.
thanks
-- PMM
