Re: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
On Fri, May 23, 2025 at 01:51:25AM +, Tian, Kevin wrote: > > If page is unmapped then stage 2 (Host page table) gets updated. IOMMU > > will not > > be able to find page and logs fault. > > As long as the fault is contained only for the relevant queue, yes > we don't need another pinning from the driver. Yeah, that's my expectation too. If the guest uses a bad address then the fault should be logged in HW and forwarded to the guest. No need to validate the guest IOVA for queues. Jason
RE: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
> From: Vasant Hegde
> Sent: Tuesday, May 20, 2025 4:39 PM
>
> Hi Nicolin,
>
>
> On 5/19/2025 11:44 PM, Nicolin Chen wrote:
> > On Mon, May 19, 2025 at 10:59:49PM +0530, Vasant Hegde wrote:
> >> Jason, Nicolin, Kevin,
> >>
> >>
> >> On 5/15/2025 9:36 PM, Jason Gunthorpe wrote:
> >>> On Thu, May 08, 2025 at 08:02:32PM -0700, Nicolin Chen wrote:
> +/**
> + * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
> + * @size: sizeof(struct iommu_hw_queue_alloc)
> + * @flags: Must be 0
> + * @viommu_id: Virtual IOMMU ID to associate the HW queue with
> + * @type: One of enum iommu_hw_queue_type
> + * @index: The logical index to the HW queue per virtual IOMMU for a
> multi-queue
> + * model
> + * @out_hw_queue_id: The ID of the new HW queue
> + * @base_addr: Base address of the queue memory in guest physical
> address space
> + * @length: Length of the queue memory in the guest physical address
> space
> + *
> + * Allocate a HW queue object for a vIOMMU-specific HW-accelerated
> queue, which
> + * allows HW to access a guest queue memory described by
> @base_addr and @length.
> + * Upon success, the underlying physical pages of the guest queue
> memory will be
> + * pinned to prevent VMM from unmapping them in the IOAS until the
> HW queue gets
> + * destroyed.
> >>>
> >>> Do we have way to make the pinning optional?
> >>>
> >>> As I understand AMD's system the iommu HW itself translates the
> >>> base_addr through the S2 page table automatically, so it doesn't need
> >>> pinned memory and physical addresses but just the IOVA.
> >>
> >> Correct. HW will translate GPA -> SPA automatically using below
> information.
> >>
> >> AMD IOMMU need special device ID to setup with GPA -> SPA mapping
> per VM.
> >> and its programmed in VF Control BAR (VFCntlMMIO Offset
> {16’b[GuestID],
> >> 6’b01_} Guest Miscellaneous Control Register). IOMMU HW will use
> this
> >> address for GPA to SPA translation for buffers like command buffer.
> >>
> >> So HW will use Base address (GPA), head/tail pointer to get the offset
> from
> >> Base. Then it will use GPA -> SPA translation.
> >>
> >>
> >>>
> >>> Perhaps for this reason the pinning should be done with a function
> >>> call from the driver?
> >>
> >> We still need to make sure memory allocated for page is present in
> memory so
> >> that IOMMU HW can access it.
> >>
> >> Pinning at the time of guest boot is enough here -OR- do we need to
> increase
> >> reference in queue_alloc() path ?
> >
> > For NVIDIA's vCMDQ that reads host PA directly, pages should be
> > pinned once when stage 2 mappings are created for the guest RAM,
> > and iommu_hw_queue_alloc() should pin the pages again to prevent
> > the gPA from being unmapped in the stage 2 page table. Otherwise
> > it will be a security hole, as HW continues to read the unmapped
> > memory through physical address space.
> >
> > I understand that AMD Command Buffer also needs the S2 mappings
> > to be present in order to work correctly. But what happens if a
> > queue memory that isn't pinned (or even gets unmapped)? Will it
> > raise a translation fault v.s. HW reading the unmapped memory?
>
> If page is unmapped then stage 2 (Host page table) gets updated. IOMMU
> will not
> be able to find page and logs fault.
>
As long as the fault is contained only for the relevant queue, yes
we don't need another pinning from the driver.
Re: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
Hi Nicolin,
On 5/19/2025 11:44 PM, Nicolin Chen wrote:
> On Mon, May 19, 2025 at 10:59:49PM +0530, Vasant Hegde wrote:
>> Jason, Nicolin, Kevin,
>>
>>
>> On 5/15/2025 9:36 PM, Jason Gunthorpe wrote:
>>> On Thu, May 08, 2025 at 08:02:32PM -0700, Nicolin Chen wrote:
+/**
+ * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
+ * @size: sizeof(struct iommu_hw_queue_alloc)
+ * @flags: Must be 0
+ * @viommu_id: Virtual IOMMU ID to associate the HW queue with
+ * @type: One of enum iommu_hw_queue_type
+ * @index: The logical index to the HW queue per virtual IOMMU for a
multi-queue
+ * model
+ * @out_hw_queue_id: The ID of the new HW queue
+ * @base_addr: Base address of the queue memory in guest physical address
space
+ * @length: Length of the queue memory in the guest physical address space
+ *
+ * Allocate a HW queue object for a vIOMMU-specific HW-accelerated queue,
which
+ * allows HW to access a guest queue memory described by @base_addr and
@length.
+ * Upon success, the underlying physical pages of the guest queue memory
will be
+ * pinned to prevent VMM from unmapping them in the IOAS until the HW
queue gets
+ * destroyed.
>>>
>>> Do we have way to make the pinning optional?
>>>
>>> As I understand AMD's system the iommu HW itself translates the
>>> base_addr through the S2 page table automatically, so it doesn't need
>>> pinned memory and physical addresses but just the IOVA.
>>
>> Correct. HW will translate GPA -> SPA automatically using below information.
>>
>> AMD IOMMU need special device ID to setup with GPA -> SPA mapping per VM.
>> and its programmed in VF Control BAR (VFCntlMMIO Offset {16’b[GuestID],
>> 6’b01_} Guest Miscellaneous Control Register). IOMMU HW will use this
>> address for GPA to SPA translation for buffers like command buffer.
>>
>> So HW will use Base address (GPA), head/tail pointer to get the offset from
>> Base. Then it will use GPA -> SPA translation.
>>
>>
>>>
>>> Perhaps for this reason the pinning should be done with a function
>>> call from the driver?
>>
>> We still need to make sure memory allocated for page is present in memory so
>> that IOMMU HW can access it.
>>
>> Pinning at the time of guest boot is enough here -OR- do we need to increase
>> reference in queue_alloc() path ?
>
> For NVIDIA's vCMDQ that reads host PA directly, pages should be
> pinned once when stage 2 mappings are created for the guest RAM,
> and iommu_hw_queue_alloc() should pin the pages again to prevent
> the gPA from being unmapped in the stage 2 page table. Otherwise
> it will be a security hole, as HW continues to read the unmapped
> memory through physical address space.
>
> I understand that AMD Command Buffer also needs the S2 mappings
> to be present in order to work correctly. But what happens if a
> queue memory that isn't pinned (or even gets unmapped)? Will it
> raise a translation fault v.s. HW reading the unmapped memory?
If page is unmapped then stage 2 (Host page table) gets updated. IOMMU will not
be able to find page and logs fault.
>
> If so, I think this is Jason's point: there would be unlikely a
> security hole, i.e. for AMD, iommu_hw_queue_alloc() pinning the
> physical pages is likely optional.
I think so.
-Vasant
Re: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
On Mon, May 19, 2025 at 10:59:49PM +0530, Vasant Hegde wrote:
> Jason, Nicolin, Kevin,
>
>
> On 5/15/2025 9:36 PM, Jason Gunthorpe wrote:
> > On Thu, May 08, 2025 at 08:02:32PM -0700, Nicolin Chen wrote:
> >> +/**
> >> + * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
> >> + * @size: sizeof(struct iommu_hw_queue_alloc)
> >> + * @flags: Must be 0
> >> + * @viommu_id: Virtual IOMMU ID to associate the HW queue with
> >> + * @type: One of enum iommu_hw_queue_type
> >> + * @index: The logical index to the HW queue per virtual IOMMU for a
> >> multi-queue
> >> + * model
> >> + * @out_hw_queue_id: The ID of the new HW queue
> >> + * @base_addr: Base address of the queue memory in guest physical address
> >> space
> >> + * @length: Length of the queue memory in the guest physical address space
> >> + *
> >> + * Allocate a HW queue object for a vIOMMU-specific HW-accelerated queue,
> >> which
> >> + * allows HW to access a guest queue memory described by @base_addr and
> >> @length.
> >> + * Upon success, the underlying physical pages of the guest queue memory
> >> will be
> >> + * pinned to prevent VMM from unmapping them in the IOAS until the HW
> >> queue gets
> >> + * destroyed.
> >
> > Do we have way to make the pinning optional?
> >
> > As I understand AMD's system the iommu HW itself translates the
> > base_addr through the S2 page table automatically, so it doesn't need
> > pinned memory and physical addresses but just the IOVA.
>
> Correct. HW will translate GPA -> SPA automatically using below information.
>
> AMD IOMMU need special device ID to setup with GPA -> SPA mapping per VM.
> and its programmed in VF Control BAR (VFCntlMMIO Offset {16’b[GuestID],
> 6’b01_} Guest Miscellaneous Control Register). IOMMU HW will use this
> address for GPA to SPA translation for buffers like command buffer.
>
> So HW will use Base address (GPA), head/tail pointer to get the offset from
> Base. Then it will use GPA -> SPA translation.
>
>
> >
> > Perhaps for this reason the pinning should be done with a function
> > call from the driver?
>
> We still need to make sure memory allocated for page is present in memory so
> that IOMMU HW can access it.
>
> Pinning at the time of guest boot is enough here -OR- do we need to increase
> reference in queue_alloc() path ?
For NVIDIA's vCMDQ that reads host PA directly, pages should be
pinned once when stage 2 mappings are created for the guest RAM,
and iommu_hw_queue_alloc() should pin the pages again to prevent
the gPA from being unmapped in the stage 2 page table. Otherwise
it will be a security hole, as HW continues to read the unmapped
memory through physical address space.
I understand that AMD Command Buffer also needs the S2 mappings
to be present in order to work correctly. But what happens if a
queue memory that isn't pinned (or even gets unmapped)? Will it
raise a translation fault v.s. HW reading the unmapped memory?
If so, I think this is Jason's point: there would be unlikely a
security hole, i.e. for AMD, iommu_hw_queue_alloc() pinning the
physical pages is likely optional.
Thanks
Nicolin
Re: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
Jason, Nicolin, Kevin,
On 5/15/2025 9:36 PM, Jason Gunthorpe wrote:
> On Thu, May 08, 2025 at 08:02:32PM -0700, Nicolin Chen wrote:
>> +/**
>> + * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
>> + * @size: sizeof(struct iommu_hw_queue_alloc)
>> + * @flags: Must be 0
>> + * @viommu_id: Virtual IOMMU ID to associate the HW queue with
>> + * @type: One of enum iommu_hw_queue_type
>> + * @index: The logical index to the HW queue per virtual IOMMU for a
>> multi-queue
>> + * model
>> + * @out_hw_queue_id: The ID of the new HW queue
>> + * @base_addr: Base address of the queue memory in guest physical address
>> space
>> + * @length: Length of the queue memory in the guest physical address space
>> + *
>> + * Allocate a HW queue object for a vIOMMU-specific HW-accelerated queue,
>> which
>> + * allows HW to access a guest queue memory described by @base_addr and
>> @length.
>> + * Upon success, the underlying physical pages of the guest queue memory
>> will be
>> + * pinned to prevent VMM from unmapping them in the IOAS until the HW queue
>> gets
>> + * destroyed.
>
> Do we have way to make the pinning optional?
>
> As I understand AMD's system the iommu HW itself translates the
> base_addr through the S2 page table automatically, so it doesn't need
> pinned memory and physical addresses but just the IOVA.
Correct. HW will translate GPA -> SPA automatically using below information.
AMD IOMMU need special device ID to setup with GPA -> SPA mapping per VM.
and its programmed in VF Control BAR (VFCntlMMIO Offset {16’b[GuestID],
6’b01_} Guest Miscellaneous Control Register). IOMMU HW will use this
address for GPA to SPA translation for buffers like command buffer.
So HW will use Base address (GPA), head/tail pointer to get the offset from
Base. Then it will use GPA -> SPA translation.
>
> Perhaps for this reason the pinning should be done with a function
> call from the driver?
We still need to make sure memory allocated for page is present in memory so
that IOMMU HW can access it.
Pinning at the time of guest boot is enough here -OR- do we need to increase
reference in queue_alloc() path ?
-Vasant
Re: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
Hi Kevin, On Thu, May 15, 2025 at 09:05:11PM -0700, Nicolin Chen wrote: > On Fri, May 16, 2025 at 03:52:16AM +, Tian, Kevin wrote: > > But hey, we are already adding various restrictions to the uAPI > > about dependency, contiguity, etc. which the VMM should conform > > to. What hurts if we further say that the VMM should allocate > > virtual index in an ascending order along with hw queue allocation? > > You mean adding another flag to manage the dependency in the core, > right? > > I talked with Jason offline when adding that depend API. He didn't > want it to be in the core, saying that is a driver thing. > > But that was before we added pin and contiguity, which he doesn't > really enjoy being in the core either. > > So, yea, I think you have a point here.. It seems Jason is out of office. And in the last sync w.r.t this, he thinks that this ascending order stuff is too unique/weird to make sense as a feature in the core, that there would be unlikely a second HW wanting this.. I think that's a valid point too. The pin/contiguity requirement at least serves for HW that reads in physical address space, and it could result in a slightly faster memory access since it does not need a translation, which though gives software some trouble yet still makes sense IMHO. So, in v5, I kept the dependency APIs rather than moving to the core. I think we can move to the core later if we see another HW doing the same thing. Thanks Nicolin
Re: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
On Thu, May 15, 2025 at 01:32:48PM -0700, Nicolin Chen wrote:
> On Thu, May 15, 2025 at 03:59:38PM -0300, Jason Gunthorpe wrote:
> > On Thu, May 15, 2025 at 11:16:45AM -0700, Nicolin Chen wrote:
> > > > I don't think this actually works like this without an unmap
> > > > callback. unmap will break:
> > > >
> > > > iommufd_access_notify_unmap(iopt, area_first,
> > > > length);
> > > > /* Something is not responding to unmap
> > > > requests. */
> > > > tries++;
> > > > if (WARN_ON(tries > 100))
> > > > return -EDEADLOCK;
> > > >
> > > > If it can't shoot down the pinning.
> > >
> > > Hmm, I thought we want the unmap to fail until VMM releases the HW
> > > QUEUE first? In what case, does VMM wants to unmap while holding
> > > the queue pages?
> >
> > Well, if that is what we want to do then this needs to be revised
> > somehow..
>
> Yea, unless we have a strong reason to allow unmap while holding
> the HW queue.
>
> I think we could set a new flag:
>
> enum {
> IOMMUFD_ACCESS_RW_READ = 0,
> IOMMUFD_ACCESS_RW_WRITE = 1 << 0,
> /* Set if the caller is in a kthread then rw will use kthread_use_mm()
> */
> IOMMUFD_ACCESS_RW_KTHREAD = 1 << 1,
> + IOMMUFD_ACCESS_NO_UNMAP = 1 << 3,
>
> /* Only for use by selftest */
> __IOMMUFD_ACCESS_RW_SLOW_PATH = 1 << 2,
> };
>
> and reject iopt_unmap_iova_range().
Okay, it would need a patch for this too. I think we wanted to limit
this no_unmap behavior though. Linking it to deliberate action that
the user took to create a vqueue with a user provided address seems
reasonable
I would probably put the flag out of the public header though, just to
prevent abuse from mdev drivers.
Jason
Re: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
On Fri, May 16, 2025 at 02:42:32AM +, Tian, Kevin wrote: > > From: Jason Gunthorpe > > Sent: Friday, May 16, 2025 12:06 AM > > > > Do we have way to make the pinning optional? > > > > As I understand AMD's system the iommu HW itself translates the > > base_addr through the S2 page table automatically, so it doesn't need > > pinned memory and physical addresses but just the IOVA. > > > > Though using IOVA could eliminate pinning conceptually, implementation > wise an IOMMU may not tolerate translation errors in its access to guest > queues with assumption that S2 is pinned. Yes, but the entire S2 is pinned today. This isn't about transient unmap.. If the VMM decides to unmap the memory, eg with hotunplug or something, then I'd fully expect the IOMMU to take a fault and forward the error to the guest. Guest make a mistake to put the queue in memory that was hot-unplugged. Jason
Re: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
On Fri, May 16, 2025 at 03:52:16AM +, Tian, Kevin wrote: > > From: Nicolin Chen > > Sent: Friday, May 16, 2025 11:17 AM > > > > On Fri, May 16, 2025 at 02:49:44AM +, Tian, Kevin wrote: > > > > From: Nicolin Chen > > > > Sent: Friday, May 16, 2025 2:45 AM > > > > > > > > On Thu, May 15, 2025 at 06:30:27AM +, Tian, Kevin wrote: > > > > > > From: Nicolin Chen > > > > > > Sent: Friday, May 9, 2025 11:03 AM > > > > > > > > > > > > + > > > > > > +/** > > > > > > + * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC) > > > > > > + * @size: sizeof(struct iommu_hw_queue_alloc) > > > > > > + * @flags: Must be 0 > > > > > > + * @viommu_id: Virtual IOMMU ID to associate the HW queue with > > > > > > + * @type: One of enum iommu_hw_queue_type > > > > > > + * @index: The logical index to the HW queue per virtual IOMMU for > > a > > > > > > multi-queue > > > > > > + * model > > > > > > > > > > I'm thinking of an alternative way w/o having the user to assign index > > > > > and allowing the driver to poke object dependency (next patch). > > > > > > > > > > Let's say the index is internally assigned by the driver. so this cmd > > > > > is > > > > > just for allowing a hw queue and it's the driver to decide the > > > > > allocation > > > > > policy, e.g. in ascending order. > > > > > > > > > > Introduce a new flag in viommu_ops to indicate to core that the > > > > > new hw queue should hold a reference to the previous hw queue. > > > > > > > > > > core maintains a last_queue field in viommu. Upon success return > > > > > from @hw_queue_alloc() the core increments the users refcnt of > > > > > last_queue, records the dependency in iommufd_hw_queue struct, > > > > > and update viommu->last_queue. > > > > > > > > > > Then the destroy order is naturally guaranteed. > > > > > > > > I have thought about that too. It's nice that the core can easily > > > > maintain the dependency for the driver. > > > > > > > > But there would still need an out_index to mark each dynamically > > > > allocated queue. So VMM would know where it should map the queue. > > > > > > > > For example, if VMM wants to allocate a queue at its own index=1 > > > > without allocating index=0 first, kernel cannot fail that as VMM > > > > doesn't provide the index. The only way left for kernel would be > > > > to output the allocated queue with index=0 and then wish VMM can > > > > validate it, which doesn't sound safe.. > > > > > > > > > > VMM's index is virtual which could be mapped to whatever queue > > > object created at its own disposal. > > > > > > the uAPI just requires VMM to remember a sequential list of allocated > > > queue objects and destroy them in reverse order of allocation, instead > > > of in the reverse order of virtual indexes. > > > > But that's not going to work for VCMDQ. > > > > VINTF mmaps only a single page that controls multiple queues. And > > all queues have to be mapped correctly between HW and VM indexes. > > Otherwise, it won't work if VMM maps: > > > > HW-level VINTF1 LVCMDQ0 <==> VM-level VINTF0 LVCMDQ1 > > HW-level VINTF1 LVCMDQ1 <==> VM-level VINTF0 LVCMDQ0 > > > > So, one way or another, kernel has to ensure the static mappings > > of the indexes. And I think it's safer in the way that VMM tells > > what index to allocate.. > > > > Okay, that's a valid point. > > But hey, we are already adding various restrictions to the uAPI > about dependency, contiguity, etc. which the VMM should conform > to. What hurts if we further say that the VMM should allocate > virtual index in an ascending order along with hw queue allocation? You mean adding another flag to manage the dependency in the core, right? I talked with Jason offline when adding that depend API. He didn't want it to be in the core, saying that is a driver thing. But that was before we added pin and contiguity, which he doesn't really enjoy being in the core either. So, yea, I think you have a point here.. @Jason? Thanks Nicolin
RE: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
> From: Nicolin Chen > Sent: Friday, May 16, 2025 11:17 AM > > On Fri, May 16, 2025 at 02:49:44AM +, Tian, Kevin wrote: > > > From: Nicolin Chen > > > Sent: Friday, May 16, 2025 2:45 AM > > > > > > On Thu, May 15, 2025 at 06:30:27AM +, Tian, Kevin wrote: > > > > > From: Nicolin Chen > > > > > Sent: Friday, May 9, 2025 11:03 AM > > > > > > > > > > + > > > > > +/** > > > > > + * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC) > > > > > + * @size: sizeof(struct iommu_hw_queue_alloc) > > > > > + * @flags: Must be 0 > > > > > + * @viommu_id: Virtual IOMMU ID to associate the HW queue with > > > > > + * @type: One of enum iommu_hw_queue_type > > > > > + * @index: The logical index to the HW queue per virtual IOMMU for > a > > > > > multi-queue > > > > > + * model > > > > > > > > I'm thinking of an alternative way w/o having the user to assign index > > > > and allowing the driver to poke object dependency (next patch). > > > > > > > > Let's say the index is internally assigned by the driver. so this cmd is > > > > just for allowing a hw queue and it's the driver to decide the > > > > allocation > > > > policy, e.g. in ascending order. > > > > > > > > Introduce a new flag in viommu_ops to indicate to core that the > > > > new hw queue should hold a reference to the previous hw queue. > > > > > > > > core maintains a last_queue field in viommu. Upon success return > > > > from @hw_queue_alloc() the core increments the users refcnt of > > > > last_queue, records the dependency in iommufd_hw_queue struct, > > > > and update viommu->last_queue. > > > > > > > > Then the destroy order is naturally guaranteed. > > > > > > I have thought about that too. It's nice that the core can easily > > > maintain the dependency for the driver. > > > > > > But there would still need an out_index to mark each dynamically > > > allocated queue. So VMM would know where it should map the queue. > > > > > > For example, if VMM wants to allocate a queue at its own index=1 > > > without allocating index=0 first, kernel cannot fail that as VMM > > > doesn't provide the index. The only way left for kernel would be > > > to output the allocated queue with index=0 and then wish VMM can > > > validate it, which doesn't sound safe.. > > > > > > > VMM's index is virtual which could be mapped to whatever queue > > object created at its own disposal. > > > > the uAPI just requires VMM to remember a sequential list of allocated > > queue objects and destroy them in reverse order of allocation, instead > > of in the reverse order of virtual indexes. > > But that's not going to work for VCMDQ. > > VINTF mmaps only a single page that controls multiple queues. And > all queues have to be mapped correctly between HW and VM indexes. > Otherwise, it won't work if VMM maps: > > HW-level VINTF1 LVCMDQ0 <==> VM-level VINTF0 LVCMDQ1 > HW-level VINTF1 LVCMDQ1 <==> VM-level VINTF0 LVCMDQ0 > > So, one way or another, kernel has to ensure the static mappings > of the indexes. And I think it's safer in the way that VMM tells > what index to allocate.. > Okay, that's a valid point. But hey, we are already adding various restrictions to the uAPI about dependency, contiguity, etc. which the VMM should conform to. What hurts if we further say that the VMM should allocate virtual index in an ascending order along with hw queue allocation?
Re: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
On Fri, May 16, 2025 at 02:49:44AM +, Tian, Kevin wrote: > > From: Nicolin Chen > > Sent: Friday, May 16, 2025 2:45 AM > > > > On Thu, May 15, 2025 at 06:30:27AM +, Tian, Kevin wrote: > > > > From: Nicolin Chen > > > > Sent: Friday, May 9, 2025 11:03 AM > > > > > > > > + > > > > +/** > > > > + * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC) > > > > + * @size: sizeof(struct iommu_hw_queue_alloc) > > > > + * @flags: Must be 0 > > > > + * @viommu_id: Virtual IOMMU ID to associate the HW queue with > > > > + * @type: One of enum iommu_hw_queue_type > > > > + * @index: The logical index to the HW queue per virtual IOMMU for a > > > > multi-queue > > > > + * model > > > > > > I'm thinking of an alternative way w/o having the user to assign index > > > and allowing the driver to poke object dependency (next patch). > > > > > > Let's say the index is internally assigned by the driver. so this cmd is > > > just for allowing a hw queue and it's the driver to decide the allocation > > > policy, e.g. in ascending order. > > > > > > Introduce a new flag in viommu_ops to indicate to core that the > > > new hw queue should hold a reference to the previous hw queue. > > > > > > core maintains a last_queue field in viommu. Upon success return > > > from @hw_queue_alloc() the core increments the users refcnt of > > > last_queue, records the dependency in iommufd_hw_queue struct, > > > and update viommu->last_queue. > > > > > > Then the destroy order is naturally guaranteed. > > > > I have thought about that too. It's nice that the core can easily > > maintain the dependency for the driver. > > > > But there would still need an out_index to mark each dynamically > > allocated queue. So VMM would know where it should map the queue. > > > > For example, if VMM wants to allocate a queue at its own index=1 > > without allocating index=0 first, kernel cannot fail that as VMM > > doesn't provide the index. The only way left for kernel would be > > to output the allocated queue with index=0 and then wish VMM can > > validate it, which doesn't sound safe.. > > > > VMM's index is virtual which could be mapped to whatever queue > object created at its own disposal. > > the uAPI just requires VMM to remember a sequential list of allocated > queue objects and destroy them in reverse order of allocation, instead > of in the reverse order of virtual indexes. But that's not going to work for VCMDQ. VINTF mmaps only a single page that controls multiple queues. And all queues have to be mapped correctly between HW and VM indexes. Otherwise, it won't work if VMM maps: HW-level VINTF1 LVCMDQ0 <==> VM-level VINTF0 LVCMDQ1 HW-level VINTF1 LVCMDQ1 <==> VM-level VINTF0 LVCMDQ0 So, one way or another, kernel has to ensure the static mappings of the indexes. And I think it's safer in the way that VMM tells what index to allocate.. Thanks Nicolin
RE: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
> From: Nicolin Chen > Sent: Friday, May 16, 2025 2:45 AM > > On Thu, May 15, 2025 at 06:30:27AM +, Tian, Kevin wrote: > > > From: Nicolin Chen > > > Sent: Friday, May 9, 2025 11:03 AM > > > > > > + > > > +/** > > > + * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC) > > > + * @size: sizeof(struct iommu_hw_queue_alloc) > > > + * @flags: Must be 0 > > > + * @viommu_id: Virtual IOMMU ID to associate the HW queue with > > > + * @type: One of enum iommu_hw_queue_type > > > + * @index: The logical index to the HW queue per virtual IOMMU for a > > > multi-queue > > > + * model > > > > I'm thinking of an alternative way w/o having the user to assign index > > and allowing the driver to poke object dependency (next patch). > > > > Let's say the index is internally assigned by the driver. so this cmd is > > just for allowing a hw queue and it's the driver to decide the allocation > > policy, e.g. in ascending order. > > > > Introduce a new flag in viommu_ops to indicate to core that the > > new hw queue should hold a reference to the previous hw queue. > > > > core maintains a last_queue field in viommu. Upon success return > > from @hw_queue_alloc() the core increments the users refcnt of > > last_queue, records the dependency in iommufd_hw_queue struct, > > and update viommu->last_queue. > > > > Then the destroy order is naturally guaranteed. > > I have thought about that too. It's nice that the core can easily > maintain the dependency for the driver. > > But there would still need an out_index to mark each dynamically > allocated queue. So VMM would know where it should map the queue. > > For example, if VMM wants to allocate a queue at its own index=1 > without allocating index=0 first, kernel cannot fail that as VMM > doesn't provide the index. The only way left for kernel would be > to output the allocated queue with index=0 and then wish VMM can > validate it, which doesn't sound safe.. > VMM's index is virtual which could be mapped to whatever queue object created at its own disposal. the uAPI just requires VMM to remember a sequential list of allocated queue objects and destroy them in reverse order of allocation, instead of in the reverse order of virtual indexes.
RE: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
> From: Jason Gunthorpe > Sent: Friday, May 16, 2025 12:06 AM > > Do we have way to make the pinning optional? > > As I understand AMD's system the iommu HW itself translates the > base_addr through the S2 page table automatically, so it doesn't need > pinned memory and physical addresses but just the IOVA. > Though using IOVA could eliminate pinning conceptually, implementation wise an IOMMU may not tolerate translation errors in its access to guest queues with assumption that S2 is pinned. @Vasant, can you help confirm?
Re: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
On Thu, May 15, 2025 at 03:59:38PM -0300, Jason Gunthorpe wrote:
> On Thu, May 15, 2025 at 11:16:45AM -0700, Nicolin Chen wrote:
> > > I don't think this actually works like this without an unmap
> > > callback. unmap will break:
> > >
> > > iommufd_access_notify_unmap(iopt, area_first, length);
> > > /* Something is not responding to unmap requests. */
> > > tries++;
> > > if (WARN_ON(tries > 100))
> > > return -EDEADLOCK;
> > >
> > > If it can't shoot down the pinning.
> >
> > Hmm, I thought we want the unmap to fail until VMM releases the HW
> > QUEUE first? In what case, does VMM wants to unmap while holding
> > the queue pages?
>
> Well, if that is what we want to do then this needs to be revised
> somehow..
Yea, unless we have a strong reason to allow unmap while holding
the HW queue.
I think we could set a new flag:
enum {
IOMMUFD_ACCESS_RW_READ = 0,
IOMMUFD_ACCESS_RW_WRITE = 1 << 0,
/* Set if the caller is in a kthread then rw will use kthread_use_mm()
*/
IOMMUFD_ACCESS_RW_KTHREAD = 1 << 1,
+ IOMMUFD_ACCESS_NO_UNMAP = 1 << 3,
/* Only for use by selftest */
__IOMMUFD_ACCESS_RW_SLOW_PATH = 1 << 2,
};
and reject iopt_unmap_iova_range().
Thanks
Nicolin
Re: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
On Thu, May 15, 2025 at 11:16:45AM -0700, Nicolin Chen wrote: > > As I understand AMD's system the iommu HW itself translates the > > base_addr through the S2 page table automatically, so it doesn't need > > pinned memory and physical addresses but just the IOVA. > > Right. That's why we invented a flag, and it should be probably > extended to cover the pin step as well. Yes, no pin > > Perhaps for this reason the pinning should be done with a function > > call from the driver? > > But the whole point of doing in the core was to avoid the entire > iopt related structure/function from being exposed to the driver, > which would otherwise hugely increase the size of the driver.o? Ugh, yes, but also, maybe we need to figure something else out for this. Pass down a function pointers struct to the driver or something like that? > > I don't think this actually works like this without an unmap > > callback. unmap will break: > > > > iommufd_access_notify_unmap(iopt, area_first, length); > > /* Something is not responding to unmap requests. */ > > tries++; > > if (WARN_ON(tries > 100)) > > return -EDEADLOCK; > > > > If it can't shoot down the pinning. > > Hmm, I thought we want the unmap to fail until VMM releases the HW > QUEUE first? In what case, does VMM wants to unmap while holding > the queue pages? Well, if that is what we want to do then this needs to be revised somehow.. > > This is more reason to put the pin/access in the driver so it can > > provide an unmap callback that can fix it up. > > As there are two types of "access" here.. you mean iommufd_access, > i.e. vcmdq driver should hold an iommufd_access like an emulated > vfio device driver? Yes. Jason
Re: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
On Thu, May 15, 2025 at 06:30:27AM +, Tian, Kevin wrote:
> > From: Nicolin Chen
> > Sent: Friday, May 9, 2025 11:03 AM
> >
> > +
> > +/**
> > + * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
> > + * @size: sizeof(struct iommu_hw_queue_alloc)
> > + * @flags: Must be 0
> > + * @viommu_id: Virtual IOMMU ID to associate the HW queue with
> > + * @type: One of enum iommu_hw_queue_type
> > + * @index: The logical index to the HW queue per virtual IOMMU for a
> > multi-queue
> > + * model
>
> I'm thinking of an alternative way w/o having the user to assign index
> and allowing the driver to poke object dependency (next patch).
>
> Let's say the index is internally assigned by the driver. so this cmd is
> just for allowing a hw queue and it's the driver to decide the allocation
> policy, e.g. in ascending order.
>
> Introduce a new flag in viommu_ops to indicate to core that the
> new hw queue should hold a reference to the previous hw queue.
>
> core maintains a last_queue field in viommu. Upon success return
> from @hw_queue_alloc() the core increments the users refcnt of
> last_queue, records the dependency in iommufd_hw_queue struct,
> and update viommu->last_queue.
>
> Then the destroy order is naturally guaranteed.
I have thought about that too. It's nice that the core can easily
maintain the dependency for the driver.
But there would still need an out_index to mark each dynamically
allocated queue. So VMM would know where it should map the queue.
For example, if VMM wants to allocate a queue at its own index=1
without allocating index=0 first, kernel cannot fail that as VMM
doesn't provide the index. The only way left for kernel would be
to output the allocated queue with index=0 and then wish VMM can
validate it, which doesn't sound safe..
> > + * @out_hw_queue_id: The ID of the new HW queue
> > + * @base_addr: Base address of the queue memory in guest physical
> > address space
> > + * @length: Length of the queue memory in the guest physical address
> > space
>
> length is agnostic to address space.
Ack.
* @length: Length of the queue memory
> > +int iommufd_hw_queue_alloc_ioctl(struct iommufd_ucmd *ucmd)
> > +{
> > + struct iommu_hw_queue_alloc *cmd = ucmd->cmd;
> > + struct iommufd_hw_queue *hw_queue;
> > + struct iommufd_hwpt_paging *hwpt;
> > + struct iommufd_viommu *viommu;
> > + struct page **pages;
> > + int max_npages, i;
> > + u64 end;
> > + int rc;
> > +
> > + if (cmd->flags || cmd->type == IOMMU_HW_QUEUE_TYPE_DEFAULT)
> > + return -EOPNOTSUPP;
> > + if (!cmd->base_addr || !cmd->length)
> > + return -EINVAL;
> > + if (check_add_overflow(cmd->base_addr, cmd->length - 1, &end))
> > + return -EOVERFLOW;
> > +
> > + max_npages = DIV_ROUND_UP(cmd->length, PAGE_SIZE);
>
> what about [base_addr, base_addr+length) spanning two pages but
> 'length' is smaller than the size of one page?
Ah, right! Probably should be something like:
offset = cmd->base_addr - PAGE_ALIGN(cmd->base_addr);
max_npages = DIV_ROUND_UP(offset + cmd->length, PAGE_SIZE);
> > + pages = kcalloc(max_npages, sizeof(*pages), GFP_KERNEL);
> > + if (!pages)
> > + return -ENOMEM;
>
> this could be moved to right before iopt_pin_pages().
Ack.
> > +
> > + viommu = iommufd_get_viommu(ucmd, cmd->viommu_id);
> > + if (IS_ERR(viommu)) {
> > + rc = PTR_ERR(viommu);
> > + goto out_free;
> > + }
> > + hwpt = viommu->hwpt;
> > +
> > + if (!viommu->ops || !viommu->ops->hw_queue_alloc) {
> > + rc = -EOPNOTSUPP;
> > + goto out_put_viommu;
> > + }
> > +
> > + /* Quick test on the base address */
> > + if (!iommu_iova_to_phys(hwpt->common.domain, cmd->base_addr))
> > {
> > + rc = -ENXIO;
> > + goto out_put_viommu;
> > + }
>
> this check is redundant. Actually it's not future proof, assuming that
> S2 must be pinned before the user attempts to call this cmd. But what
> if one day iommufd supports unpinned S2 (if a device is 100% PRI faultable)
> then this path will be broken.
OK. Let's drop it.
> > + hw_queue = viommu->ops->hw_queue_alloc(viommu, cmd->type,
> > cmd->index,
> > + cmd->base_addr, cmd->length);
> > + if (IS_ERR(hw_queue)) {
> > + rc = PTR_ERR(hw_queue);
> > + goto out_unpin;
> > + }
> > +
> > + hw_queue->viommu = viommu;
> > + refcount_inc(&viommu->obj.users);
> > + hw_queue->ictx = ucmd->ictx;
>
> viommu/ictx are already set by iommufd_hw_queue_alloc().
OK. We'd need to be careful if someday there is a core-allocated
hw_queue that doesn't call iommufd_hw_queue_alloc(). Maybe I can
put a note here.
Thanks
Nicolin
Re: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
On Thu, May 15, 2025 at 01:06:20PM -0300, Jason Gunthorpe wrote:
> On Thu, May 08, 2025 at 08:02:32PM -0700, Nicolin Chen wrote:
> > +/**
> > + * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
> > + * @size: sizeof(struct iommu_hw_queue_alloc)
> > + * @flags: Must be 0
> > + * @viommu_id: Virtual IOMMU ID to associate the HW queue with
> > + * @type: One of enum iommu_hw_queue_type
> > + * @index: The logical index to the HW queue per virtual IOMMU for a
> > multi-queue
> > + * model
> > + * @out_hw_queue_id: The ID of the new HW queue
> > + * @base_addr: Base address of the queue memory in guest physical address
> > space
> > + * @length: Length of the queue memory in the guest physical address space
> > + *
> > + * Allocate a HW queue object for a vIOMMU-specific HW-accelerated queue,
> > which
> > + * allows HW to access a guest queue memory described by @base_addr and
> > @length.
> > + * Upon success, the underlying physical pages of the guest queue memory
> > will be
> > + * pinned to prevent VMM from unmapping them in the IOAS until the HW
> > queue gets
> > + * destroyed.
>
> Do we have way to make the pinning optional?
>
> As I understand AMD's system the iommu HW itself translates the
> base_addr through the S2 page table automatically, so it doesn't need
> pinned memory and physical addresses but just the IOVA.
Right. That's why we invented a flag, and it should be probably
extended to cover the pin step as well.
> Perhaps for this reason the pinning should be done with a function
> call from the driver?
But the whole point of doing in the core was to avoid the entire
iopt related structure/function from being exposed to the driver,
which would otherwise hugely increase the size of the driver.o?
> > +struct iommu_hw_queue_alloc {
> > + __u32 size;
> > + __u32 flags;
> > + __u32 viommu_id;
> > + __u32 type;
> > + __u32 index;
> > + __u32 out_hw_queue_id;
> > + __aligned_u64 base_addr;
>
> base addr should probably be called nesting_parent_iova to match how
> we described the viommu hwpt:
>
> * @hwpt_id: ID of a nesting parent HWPT to associate to
Ack.
> > + /*
> > +* The underlying physical pages must be pinned to prevent them from
> > +* being unmapped (via IOMMUFD_CMD_IOAS_UNMAP) during the life cycle
> > +* of the HW QUEUE object.
> > +*/
> > + rc = iopt_pin_pages(&hwpt->ioas->iopt, cmd->base_addr, cmd->length,
> > + pages, 0);
>
> I don't think this actually works like this without an unmap
> callback. unmap will break:
>
> iommufd_access_notify_unmap(iopt, area_first, length);
> /* Something is not responding to unmap requests. */
> tries++;
> if (WARN_ON(tries > 100))
> return -EDEADLOCK;
>
> If it can't shoot down the pinning.
Hmm, I thought we want the unmap to fail until VMM releases the HW
QUEUE first? In what case, does VMM wants to unmap while holding
the queue pages?
> Why did this need to change away from just a normal access? That ops
> and unmap callback are not optional things.
>
> What vcmdq would do in the unmap callback is question I'm not sure
> of..
>
> This is more reason to put the pin/access in the driver so it can
> provide an unmap callback that can fix it up.
As there are two types of "access" here.. you mean iommufd_access,
i.e. vcmdq driver should hold an iommufd_access like an emulated
vfio device driver?
> I think this should have
> been done just by using the normal access mechanism, maybe with a
> simplifying wrapper for in-driver use. ie no need for patch #9
Still, the driver.o file will be very large, unlike VFIO that just
depends on CONFIG_IOMMUFD?
Thanks
Nicolin
Re: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
On Thu, May 08, 2025 at 08:02:32PM -0700, Nicolin Chen wrote:
> +/**
> + * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
> + * @size: sizeof(struct iommu_hw_queue_alloc)
> + * @flags: Must be 0
> + * @viommu_id: Virtual IOMMU ID to associate the HW queue with
> + * @type: One of enum iommu_hw_queue_type
> + * @index: The logical index to the HW queue per virtual IOMMU for a
> multi-queue
> + * model
> + * @out_hw_queue_id: The ID of the new HW queue
> + * @base_addr: Base address of the queue memory in guest physical address
> space
> + * @length: Length of the queue memory in the guest physical address space
> + *
> + * Allocate a HW queue object for a vIOMMU-specific HW-accelerated queue,
> which
> + * allows HW to access a guest queue memory described by @base_addr and
> @length.
> + * Upon success, the underlying physical pages of the guest queue memory
> will be
> + * pinned to prevent VMM from unmapping them in the IOAS until the HW queue
> gets
> + * destroyed.
Do we have way to make the pinning optional?
As I understand AMD's system the iommu HW itself translates the
base_addr through the S2 page table automatically, so it doesn't need
pinned memory and physical addresses but just the IOVA.
Perhaps for this reason the pinning should be done with a function
call from the driver?
> +struct iommu_hw_queue_alloc {
> + __u32 size;
> + __u32 flags;
> + __u32 viommu_id;
> + __u32 type;
> + __u32 index;
> + __u32 out_hw_queue_id;
> + __aligned_u64 base_addr;
base addr should probably be called nesting_parent_iova to match how
we described the viommu hwpt:
* @hwpt_id: ID of a nesting parent HWPT to associate to
> + /*
> + * The underlying physical pages must be pinned to prevent them from
> + * being unmapped (via IOMMUFD_CMD_IOAS_UNMAP) during the life cycle
> + * of the HW QUEUE object.
> + */
> + rc = iopt_pin_pages(&hwpt->ioas->iopt, cmd->base_addr, cmd->length,
> + pages, 0);
I don't think this actually works like this without an unmap
callback. unmap will break:
iommufd_access_notify_unmap(iopt, area_first, length);
/* Something is not responding to unmap requests. */
tries++;
if (WARN_ON(tries > 100))
return -EDEADLOCK;
If it can't shoot down the pinning.
Why did this need to change away from just a normal access? That ops
and unmap callback are not optional things.
What vcmdq would do in the unmap callback is question I'm not sure
of..
This is more reason to put the pin/access in the driver so it can
provide an unmap callback that can fix it up. I think this should have
been done just by using the normal access mechanism, maybe with a
simplifying wrapper for in-driver use. ie no need for patch #9
Jason
RE: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
> From: Nicolin Chen
> Sent: Friday, May 9, 2025 11:03 AM
>
> +
> +/**
> + * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
> + * @size: sizeof(struct iommu_hw_queue_alloc)
> + * @flags: Must be 0
> + * @viommu_id: Virtual IOMMU ID to associate the HW queue with
> + * @type: One of enum iommu_hw_queue_type
> + * @index: The logical index to the HW queue per virtual IOMMU for a
> multi-queue
> + * model
I'm thinking of an alternative way w/o having the user to assign index
and allowing the driver to poke object dependency (next patch).
Let's say the index is internally assigned by the driver. so this cmd is
just for allowing a hw queue and it's the driver to decide the allocation
policy, e.g. in ascending order.
Introduce a new flag in viommu_ops to indicate to core that the
new hw queue should hold a reference to the previous hw queue.
core maintains a last_queue field in viommu. Upon success return
from @hw_queue_alloc() the core increments the users refcnt of
last_queue, records the dependency in iommufd_hw_queue struct,
and update viommu->last_queue.
Then the destroy order is naturally guaranteed.
> + * @out_hw_queue_id: The ID of the new HW queue
> + * @base_addr: Base address of the queue memory in guest physical
> address space
> + * @length: Length of the queue memory in the guest physical address
> space
length is agnostic to address space.
> +int iommufd_hw_queue_alloc_ioctl(struct iommufd_ucmd *ucmd)
> +{
> + struct iommu_hw_queue_alloc *cmd = ucmd->cmd;
> + struct iommufd_hw_queue *hw_queue;
> + struct iommufd_hwpt_paging *hwpt;
> + struct iommufd_viommu *viommu;
> + struct page **pages;
> + int max_npages, i;
> + u64 end;
> + int rc;
> +
> + if (cmd->flags || cmd->type == IOMMU_HW_QUEUE_TYPE_DEFAULT)
> + return -EOPNOTSUPP;
> + if (!cmd->base_addr || !cmd->length)
> + return -EINVAL;
> + if (check_add_overflow(cmd->base_addr, cmd->length - 1, &end))
> + return -EOVERFLOW;
> +
> + max_npages = DIV_ROUND_UP(cmd->length, PAGE_SIZE);
what about [base_addr, base_addr+length) spanning two pages but
'length' is smaller than the size of one page?
> + pages = kcalloc(max_npages, sizeof(*pages), GFP_KERNEL);
> + if (!pages)
> + return -ENOMEM;
this could be moved to right before iopt_pin_pages().
> +
> + viommu = iommufd_get_viommu(ucmd, cmd->viommu_id);
> + if (IS_ERR(viommu)) {
> + rc = PTR_ERR(viommu);
> + goto out_free;
> + }
> + hwpt = viommu->hwpt;
> +
> + if (!viommu->ops || !viommu->ops->hw_queue_alloc) {
> + rc = -EOPNOTSUPP;
> + goto out_put_viommu;
> + }
> +
> + /* Quick test on the base address */
> + if (!iommu_iova_to_phys(hwpt->common.domain, cmd->base_addr))
> {
> + rc = -ENXIO;
> + goto out_put_viommu;
> + }
this check is redundant. Actually it's not future proof, assuming that
S2 must be pinned before the user attempts to call this cmd. But what
if one day iommufd supports unpinned S2 (if a device is 100% PRI faultable)
then this path will be broken.
> +
> + /*
> + * The underlying physical pages must be pinned to prevent them
> from
> + * being unmapped (via IOMMUFD_CMD_IOAS_UNMAP) during the
> life cycle
> + * of the HW QUEUE object.
> + */
> + rc = iopt_pin_pages(&hwpt->ioas->iopt, cmd->base_addr, cmd-
> >length,
> + pages, 0);
> + if (rc)
> + goto out_put_viommu;
> +
> + if (viommu->ops->flags &
> IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA) {
> + /* Validate if the underlying physical pages are contiguous */
> + for (i = 1; i < max_npages && pages[i]; i++) {
> + if (page_to_pfn(pages[i]) ==
> + page_to_pfn(pages[i - 1]) + 1)
> + continue;
> + rc = -EFAULT;
> + goto out_unpin;
> + }
> + }
> +
> + hw_queue = viommu->ops->hw_queue_alloc(viommu, cmd->type,
> cmd->index,
> +cmd->base_addr, cmd->length);
> + if (IS_ERR(hw_queue)) {
> + rc = PTR_ERR(hw_queue);
> + goto out_unpin;
> + }
> +
> + hw_queue->viommu = viommu;
> + refcount_inc(&viommu->obj.users);
> + hw_queue->ictx = ucmd->ictx;
viommu/ictx are already set by iommufd_hw_queue_alloc().
> + hw_queue->length = cmd->length;
> + hw_queue->base_addr = cmd->base_addr;
> + cmd->out_hw_queue_id = hw_queue->obj.id;
> + rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
> + if (rc)
> + iommufd_object_abort_and_destroy(ucmd->ictx,
> &hw_queue->obj);
> + else
> + iommufd_object_finalize(ucmd->ictx, &hw_queue->obj);
> + goto out_put_viommu;
> +
> +out_unpin:
> + iopt_unpin_pages(&hwpt->ioas->iopt, cmd->base_addr, cmd-
> >length);
> +out_put_
