>-----Original Message-----
>From: Clément MATHIEU--DRIF <[email protected]>
>Subject: Re: [PATCH v4 4/6] intel_iommu_accel: Add PRQ injection for
>passthrough
>device
>
>
>On Tue, 2026-08-25 at 16:18 +0800, Zhenzhong Duan wrote:
>> Caution: External email. Do not open attachments or click links, unless this
>> email
>comes from a known sender and you know the content is safe.
>>
>>
>> When the guest enables the PRQ in vIOMMU, allocate a FAULTQ object so that
>> host-side recoverable fault events can be received and propagated back to
>> the guest.
>>
>> Install an event handler on the FAULTQ fd to read and propagate host
>> generated recoverable fault events to the guest.
>>
>> The handler runs in QEMU's main loop, using a non-blocking fd registered
>> via qemu_set_fd_handler().
>>
>> Signed-off-by: Zhenzhong Duan
><[[email protected]](mailto:[email protected])>
>> Tested-by: Xudong Hao
><[[email protected]](mailto:[email protected])>
>> ---
>> hw/i386/intel_iommu_accel.h | 2 +
>> hw/i386/intel_iommu_internal.h | 3 +
>> hw/i386/intel_iommu.c | 7 +-
>> hw/i386/intel_iommu_accel.c | 168 +++++++++++++++++++++++++++++++--
>> hw/i386/trace-events | 1 +
>> 5 files changed, 171 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu_accel.h b/hw/i386/intel_iommu_accel.h
>> index 46c1a29409..e0319749a5 100644
>> --- a/hw/i386/intel_iommu_accel.h
>> +++ b/hw/i386/intel_iommu_accel.h
>> @@ -18,6 +18,8 @@ typedef struct VTDAccelPASIDCacheEntry {
>> VTDPASIDEntry pasid_entry;
>> uint32_t pasid;
>> uint32_t fs_hwpt_id;
>> + uint32_t fault_id;
>> + int fault_fd;
>> QLIST_ENTRY(VTDAccelPASIDCacheEntry) next;
>> } VTDAccelPASIDCacheEntry;
>>
>> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
>> index 924e91cb8a..5ebc204e9a 100644
>> --- a/hw/i386/intel_iommu_internal.h
>> +++ b/hw/i386/intel_iommu_internal.h
>> @@ -786,4 +786,7 @@ int vtd_dev_to_context_entry(IntelIOMMUState *s,
>uint8_t bus_num,
>> VTDAddressSpace *vtd_get_as_by_sid(IntelIOMMUState *s, uint16_t sid);
>> int vtd_dev_get_pe_from_pasid(IntelIOMMUState *s, PCIBus *bus, uint8_t
>devfn,
>> uint32_t pasid, VTDPASIDEntry *pe);
>> +int vtd_pri_request_page(PCIBus *bus, void *opaque, int devfn, uint32_t
>> pasid,
>> + bool priv_req, bool exec_req, hwaddr addr, bool
>> lpig,
>> + uint16_t prgi, bool is_read, bool is_write);
>> #endif
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 82c3c3b2c3..cc8c9205df 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -5370,10 +5370,9 @@ static int
>vtd_pri_perform_implicit_invalidation(VTDAddressSpace *vtd_as,
>> }
>>
>> /* Page Request Descriptor : 7.4.1.1 */
>> -static int vtd_pri_request_page(PCIBus *bus, void *opaque, int devfn,
>> - uint32_t pasid, bool priv_req, bool
>> exec_req,
>> - hwaddr addr, bool lpig, uint16_t prgi,
>> - bool is_read, bool is_write)
>> +int vtd_pri_request_page(PCIBus *bus, void *opaque, int devfn, uint32_t
>> pasid,
>> + bool priv_req, bool exec_req, hwaddr addr, bool
>> lpig,
>> + uint16_t prgi, bool is_read, bool is_write)
>> {
>> IntelIOMMUState *s = opaque;
>> VTDAddressSpace *vtd_as;
>> diff --git a/hw/i386/intel_iommu_accel.c b/hw/i386/intel_iommu_accel.c
>> index d2f41f18f1..da262b7863 100644
>> --- a/hw/i386/intel_iommu_accel.c
>> +++ b/hw/i386/intel_iommu_accel.c
>> @@ -9,6 +9,7 @@
>> */
>>
>> #include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>> #include "system/iommufd.h"
>> #include "intel_iommu_internal.h"
>> #include "intel_iommu_accel.h"
>> @@ -77,14 +78,158 @@ VTDHostIOMMUDevice
>*vtd_find_hiod_iommufd(VTDAddressSpace *as)
>> return NULL;
>> }
>>
>> -static bool vtd_create_fs_hwpt(VTDHostIOMMUDevice *vtd_hiod,
>> - VTDPASIDEntry *pe, uint32_t *fs_hwpt_id,
>> - Error **errp)
>> +/**
>> + * vtd_prq_report_fault - Report page requests / page faults to the guest
>> + * @vtd_pce: Pointer to the VTD accelerator PASID cache entry
>> + * @fault: Pointer to the hardware page fault structure
>> + * @cnt: Number of faults to report
>> + *
>> + * Note: vtd_pri_request_page() accepts the PCI PASID as input.
>
>I meant the docstring of vtd_pri_request_page, sorry for the unclear comment :/
Got it, thanks for clarifying. I will adjust the docstring for
vtd_pri_request_page instead.
>
>> + */
>> +static void vtd_prq_report_fault(VTDAccelPASIDCacheEntry *vtd_pce,
>> + struct iommu_hwpt_pgfault *fault, int cnt)
>> +{
>
>The qemu coding style mentions this about variables that count things:
>
>"If you’re using “int” or “long”, odds are good that there’s a better type. If
>a
>variable is counting something, it should be declared with an unsigned type."
>
>Maybe "int cnt" could be "unsigned cnt" instead?
>
>with:
>
>```
>for (; cnt-- > 0; fault++) {
> ...
>}
>```
Sure, will do.
>
>> + VTDHostIOMMUDevice *vtd_hiod = vtd_pce->vtd_hiod;
>> + uint32_t pasid =
>> + vtd_pce->pasid == IOMMU_NO_PASID ? PCI_NO_PASID : vtd_pce->pasid;
>> +
>> + for (; cnt--; fault++) {
>> + bool last_page = fault->flags & IOMMU_PGFAULT_FLAGS_LAST_PAGE;
>> +
>> + vtd_pri_request_page(vtd_hiod->bus, vtd_hiod->iommu_state,
>> + vtd_hiod->devfn, pasid,
>> + fault->perm & IOMMU_PGFAULT_PERM_PRIV,
>> + fault->perm & IOMMU_PGFAULT_PERM_EXEC,
>> + fault->addr, last_page, fault->grpid,
>> + fault->perm & IOMMU_PGFAULT_PERM_READ,
>> + fault->perm & IOMMU_PGFAULT_PERM_WRITE);
>
>Is it safe to just drop the return code?
>
>Let's assume the host sends a PRI request that is not forwarded to the guest.
>This means that no response will go back all the way down to the hw IOMMU,
>which
>is likely to lead to a prgi shortage and application deadlock right?
>
>The easiest way to trigger such a situation is to overflow the submission
>queue.
>
>```
> if (old_pr_status & VTD_PR_STATUS_PRO) {
> /*
> * No action is taken by hardware to report a fault
> * or generate an event
> */
> return -ENOSPC;
> }
>```
>
>This is acceptable in vSVM as the device checks the return code but might be a
>bit
>brutal here.
>
>Maybe vtd_prq_report_fault should generate a proper response in such a
>situation.
>
>If I remeber correctly, hw vtd generates a "success" response with pasid prefix
>when the queue is full
Thanks for pointing out. Indeed, in that case prgi is not freed in host side,
will fix.
>
>> + }
>> +}
>> +
>> +/* Batch size per read(); remaining faults trigger another callback */
>> +#define FAULTQ_BUF_SIZE 100
>> +
>> +static void vtd_prq_read_fault(void *opaque)
>> +{
>> + VTDAccelPASIDCacheEntry *vtd_pce = opaque;
>> + struct iommu_hwpt_pgfault fault[FAULTQ_BUF_SIZE];
>> + uint32_t id = vtd_pce->fault_id;
>> + int fd = vtd_pce->fault_fd;
>> + ssize_t bytes, last_bytes;
>> +
>> + bytes = read(fd, fault, sizeof(fault));
>> + trace_vtd_prq_read_fault(id, fd, bytes);
>> + if (bytes < 0) {
>> + if (errno != EAGAIN && errno != EINTR) {
>> + error_report_once("FAULTQ(id %u): read failed (%m)", id);
>> + }
>> + return;
>> + } else if (!bytes) {
>> + error_report_once("FAULTQ(id %u): fault group too big", id);
>> + return;
>> + }
>> +
>> + last_bytes = bytes % sizeof(fault[0]);
>> + if (last_bytes) {
>> + error_report_once("FAULTQ(id %u): discard partial fault data:
>> %zd/%zu",
>> + id, last_bytes, sizeof(fault));
>> + }
>> +
>> + vtd_prq_report_fault(vtd_pce, fault, bytes / sizeof(fault[0]));
>> +}
>> +
>> +static void vtd_destroy_fs_faultq(VTDHostIOMMUDevice *vtd_hiod,
>> + uint32_t fault_id, int fault_fd)
>> +{
>> + HostIOMMUDeviceIOMMUFD *hiodi =
>HOST_IOMMU_DEVICE_IOMMUFD(vtd_hiod->hiod);
>> +
>> + if (fault_fd < 0) {
>> + return;
>> + }
>> +
>> + close(fault_fd);
>> + iommufd_backend_free_id(hiodi->iommufd, fault_id);
>> +}
>> +
>> +static bool vtd_create_fs_faultq(VTDHostIOMMUDevice *vtd_hiod,
>> + uint32_t *fault_id_p, int *fault_fd_p,
>> + Error **errp)
>> +{
>> + HostIOMMUDeviceIOMMUFD *hiodi =
>HOST_IOMMU_DEVICE_IOMMUFD(vtd_hiod->hiod);
>> + IntelIOMMUState *s = vtd_hiod->iommu_state;
>> + uint8_t bus_n = pci_bus_num(vtd_hiod->bus);
>> + uint32_t fault_id, fault_fd;
>> + VTDContextEntry ce;
>> + int flags;
>> +
>> + if (!s->svm ||
>> + vtd_dev_to_context_entry(s, bus_n, vtd_hiod->devfn, &ce) ||
>> + !VTD_CE_GET_PRE(&ce)) {
>> + *fault_id_p = 0;
>> + *fault_fd_p = -1;
>> + return true;
>> + }
>> +
>> + if (!iommufd_backend_alloc_faultq(hiodi->iommufd, &fault_id, &fault_fd,
>> + errp)) {
>> + return false;
>> + }
>> +
>> + flags = fcntl(fault_fd, F_GETFL);
>> + if (flags < 0) {
>> + error_setg_errno(errp, errno, "Failed to get flags for FAULTQ fd");
>> + goto free_faultq;
>> + }
>> +
>> + if (fcntl(fault_fd, F_SETFL, flags | O_NONBLOCK) < 0) {
>> + error_setg_errno(errp, errno, "Failed to set O_NONBLOCK on FAULTQ
>> fd");
>> + goto free_faultq;
>> + }
>> +
>> + *fault_id_p = fault_id;
>> + *fault_fd_p = fault_fd;
>> + return true;
>> +
>> +free_faultq:
>> + vtd_destroy_fs_faultq(vtd_hiod, fault_id, fault_fd);
>> + return false;
>> +}
>> +
>> +static void vtd_destroy_old_fs_faultq(VTDAccelPASIDCacheEntry *vtd_pce)
>> +{
>> + if (vtd_pce->fault_fd < 0) {
>> + return;
>> + }
>> +
>> + qemu_set_fd_handler(vtd_pce->fault_fd, NULL, NULL, NULL);
>> + vtd_destroy_fs_faultq(vtd_pce->vtd_hiod, vtd_pce->fault_id,
>> + vtd_pce->fault_fd);
>> + vtd_pce->fault_id = 0;
>> + vtd_pce->fault_fd = -1;
>
>Maybe silly question, but have you tried to detach the queue while faults
>are still being processed by the guest kernel?
Good point, normally guest kernel should drain PRQ before detaching.
But we still need to survive in qemu if issue happens in guest kernel.
With current code, all entries in vtd_pce->vtd_prq_list are leaked, though
we don't need to send responses explicitly, faultq object destroy will take
over that work. So fix will be like(pseudo):
--- a/hw/i386/intel_iommu_accel.c
+++ b/hw/i386/intel_iommu_accel.c
@@ -253,6 +253,21 @@ free_faultq:
return false;
}
+void vtd_free_prq_list(VTDAccelPASIDCacheEntry *vtd_pce)
+{
+ VTDPRQEntry *prqe, *next;
+
+ if (!vtd_pce) {
+ return;
+ }
+
+ QLIST_FOREACH_SAFE(prqe, &vtd_pce->vtd_prq_list, next, next) {
+ QLIST_REMOVE(prqe, next);
+ g_free(prqe);
+ }
+}
+
typedef struct IOMMUFaultQueue {
IOMMUFDBackend *iommufd;
uint32_t id;
@@ -263,6 +278,8 @@ static void faultq_teardown_bh(void *opaque)
{
IOMMUFaultQueue *fq = opaque;
+ vtd_free_prq_list(vtd_pce);
+
qemu_set_fd_handler(fq->fd, NULL, NULL, NULL);
close(fq->fd);
iommufd_backend_free_id(fq->iommufd, fq->id);
Thanks
Zhenzhong
