Re: [PATCH v2 25/26] iommu/amd: Handle set/get command for AMD vIOMMU

[Suthikulpanit, Suravee]

On 5/30/2026 6:20 AM, Nicolin Chen wrote:
> On Thu, May 28, 2026 at 05:17:37AM +0000, Suravee Suthikulpanit wrote:
> > Guest kernel programs guest Command Buffer, Event Log, and PPR Log
> > (a.k.a hardware queue) settings via guest control MMIO register
> > (guest MMIO offset 0x18). Accesses to the register is trapped by VMM (QEMU)
> > and information is passed as IOMMU_VIOMMU_OPTION to host IOMMU driver via
> > struct iommufd_viommu_ops set_command() and get_command().
> >
> > Provides AMD IOMMU driver hooks to handle set/get operations for the
> > guest control MMIO register, which uses key parameter as AMD IOMMU MMIO
> > offset. The value parameter contains the value of the corresponding guest
> > MMIO register, which is converted to the format of AMD vIOMMU VF Control
> > MMIO registers then programed onto the hardware.
>
> So the whole set_command/get_command thing is to MMIO a control
> register? I doubt this is a right ioctl(s) as it can be abused.

Correct. As part of the AMD vIOMMU architecture, MMIO registers in the 
1st 4K range are control registers, which can be grouped into Command 
buffer, Event Log buffer, PPR Log buffer. These are trapped by QEMU and 
communicate to AMD IOMMU driver to configure the vIOMMU hardware.

> Looking at the details:
>
> > +	switch (offset) {
> > +	case MMIO_CONTROL_OFFSET:
> > +	{
> > +		/* VFCTRL offset 20h */
> > +		val = readq(vfctrl + 0x20);
> > +		val &= ~(0x3ULL << 8);
> > +		tmp = GET_CTRL_BITS(ctrl, CONTROL_CMDBUF_EN, 1); // [12]
> > +		val |= (tmp << 8);
>
> This is to enable a hw_queue. So it could be marked when a hw_queue
> is allocated; IMHO, VMM should only allocate the hw_queue, when the
> guest really uses (enables) the buffer.

There are more than one registers to control each buffer. Some of them 
we can assume that it will be set last (e.g. the CONTROL_CMDBUF_EN, 
CONTROL_EVT_LOG_EN, CONTROL_PPRLOG_EN bits), and use them to trigger 
hw_queue allocation.

My original thought is to be explicit and separate the programming into:
  * Guest setup the buffer base/size -> IOMMUFD allocate the hw_queue
  * Guest enable/disable the buffer -> IOMMUFD enable/disable the 
control bits of each buffer.

Combine these steps into just hw_queue init/destroy might be okay.

> > +		tmp = GET_CTRL_BITS(ctrl, CONTROL_COMWAIT_EN, 1); // [4]
> > +		val |= (tmp << 9);
> > +		writeq(val, vfctrl + 0x20);
>
> The spec suggests to set this with CmdBufEn, so it may be a vendor
> (or hw_queue type) specific flag in structu iommu_hw_queue_alloc?
>
> And similar comments to the event log and pri log buffers as well.

Lemme see if we can integrate this for all hw_queue.

> Overall, VMM should control the timings of allocating these viommu
> hw_queue objects; then kernel would just enable them accordingly.
>
> Nicolin

Thanks,
Suravee