> -----Original Message----- > From: Eric Auger <[email protected]> > Sent: 22 October 2025 17:25 > To: Nicolin Chen <[email protected]> > Cc: Shameer Kolothum <[email protected]>; qemu- > [email protected]; [email protected]; [email protected]; > Jason Gunthorpe <[email protected]>; [email protected]; > [email protected]; Nathan Chen <[email protected]>; Matt Ochs > <[email protected]>; [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected] > Subject: Re: [PATCH v4 11/27] hw/pci/pci: Introduce optional > get_msi_address_space() callback > > External email: Use caution opening links or attachments > > > Hi Nicolin, > > On 10/21/25 8:56 PM, Nicolin Chen wrote: > > On Tue, Oct 21, 2025 at 06:26:39PM +0200, Eric Auger wrote: > >> Hi Nicolin, > >> > >> On 10/20/25 8:00 PM, Nicolin Chen wrote: > >>> On Mon, Oct 20, 2025 at 06:14:33PM +0200, Eric Auger wrote: > >>>>>> This will cause the device to be configured with wrong MSI doorbell > >>>>>> address if it return the system address space. > >>>>> I think it'd be nicer to elaborate why a wrong address will be returned: > >>>>> > >>>>> -------------------------------------------------------------------------- > >>>>> On ARM, a device behind an IOMMU requires translation for its MSI > doorbell > >>>>> address. When HW nested translation is enabled, the translation will > also > >>>>> happen in two stages: gIOVA => gPA => ITS page. > >>>>> > >>>>> In the accelerated SMMUv3 mode, both stages are translated by the > HW. So, > >>>>> get_address_space() returns the system address space for stage-2 > mappings, > >>>>> as the smmuv3-accel model doesn't involve in either stage. > >>>> I don't understand "doesn't involve in either stage". This is still not > >>>> obious to me that for an HW accelerated nested IOMMU > get_address_space() > >>>> shall return the system address space. I think this deserves to be > >>>> explained and maybe documented along with the callback. > >>> get_address_space() is used by pci_device_iommu_address_space(), > >>> which is for attach or translation. > >>> > >>> In QEMU, we have an "iommu" type of memory region, to represent > >>> the address space providing the stage-1 translation. > >>> > >>> In accel case excluding MSI, there is no need of "emulated iommu > >>> translation" since HW/host SMMU takes care of both stages. Thus, > >>> the system address is returned for get_address_space(), to avoid > >>> stage-1 translation and to also allow VFIO devices to attach to > >>> the system address space that the VFIO core will monitor to take > >>> care of stage-2 mappings. > >> but in general if you set as output 'as' the system_address_memory it > >> rather means you have no translation in place. This is what I am not > >> convinced about. > > You mean you are not convinced about "no translation"? > I am not convinced about the choice of using address_space_memory. > > > >> you say it aims at > >> - avoiding stage-1 translation - allow VFIO devices to attach to the > >> system address space that the VFIO core will monitor to take care of > >> stage-2 mappings. Can you achieve the same goals with a proper address > >> space? > > Would you please define "proper"? > an address space different from address_space_memory > > > > The disagreement is seemingly about using system address space or > > even address_space_memory, IIUIC. > Yes my doubt is about: > > smmuv3_accel_find_add_as() > * We are using the global &address_space_memory here, as this will > ensure > * same system address space pointer for all devices behind the > accelerated > * SMMUv3s in a VM. That way VFIO/iommufd can reuse a single IOAS ID in > * iommufd_cdev_attach(), allowing the Stage-2 page tables to be shared > * within the VM instead of duplicating them for every SMMUv3 instance. > */ > if (vfio_pci) { > return &address_space_memory; > > I think it would be cleaner to a have an AddressSpace allocated on > purpose to support the VFIO accel use case, if possible. > To me returning address_space_memory pretends we are not doing any > translation. I understand it is "easy" to reuse that one but I wonder it > is the spirit of the get_address_space callback. > > I would rather allocate a dedicated (shared) AddressSpace to support the > VFIO accel case. That's my suggestion.
Ok. I will give it a go with the "global variable in smmu-accel.c" route for a separate shared address space that you suggested earlier in patch #6 thread. Thanks, Shameer
