> -----Original Message-----
> From: Peter Maydell <[email protected]>
> Sent: 23 June 2026 09:47
> To: [email protected]
> Cc: Shameer Kolothum Thodi <[email protected]>; qemu-
> [email protected]; [email protected]; Nicolin Chen
> <[email protected]>; Nathan Chen <[email protected]>; Matt Ochs
> <[email protected]>; Jiandi An <[email protected]>
> Subject: Re: [PATCH] hw/arm/smmuv3-accel: Fix veventq read returning true
> on EAGAIN/EINTR
>
> On Tue, 23 Jun 2026 at 09:32, Eric Auger <[email protected]> wrote:
)
> > > if (!smmuv3_accel_event_read_validate(veventq,
> > >
> > > IOMMU_VEVENTQ_TYPE_TEGRA241_CMDQV,
> > > &buf, sizeof(buf),
> > > &local_err)) {
> > > - warn_report_err_once(local_err);
> > > + if (local_err) {
> > This is not aligned with the general policy. If a function taking an
> > error handle fails (and returns false), it should set errp.
> > Don't you have a way to initialize buf in smmuv3_accel_event_read()
> > and only call smmuv3_propagate_event() if the hdr of vevnt is checked
> valid?
>
> I think fundamentally the function has three return states:
> - success
> - failure
> - try again later
>
> Probably the simplest thing is to not use a 'bool' for the return type but
> instead something that has 3 values. Then you can set errp on the "failure"
> case, and the caller can tell which of the three cases it is dealing with.
Ok. How about this one below. I used int and documented ret types.
Could switch to enum if that’s preferred. Please let me know.
Thanks,
Shameer
diff --git a/hw/arm/smmuv3-accel.h b/hw/arm/smmuv3-accel.h
index dd755c394d..ea11d513cc 100644
--- a/hw/arm/smmuv3-accel.h
+++ b/hw/arm/smmuv3-accel.h
@@ -97,8 +97,8 @@ bool smmuv3_accel_issue_inv_cmd(SMMUv3State *s, void *cmd,
SMMUDevice *sdev,
Error **errp);
void smmuv3_accel_idr_override(SMMUv3State *s);
bool smmuv3_accel_alloc_veventq(SMMUv3State *s, Error **errp);
-bool smmuv3_accel_event_read_validate(IOMMUFDVeventq *veventq, uint32_t type,
- void *buf, size_t size, Error **errp);
+int smmuv3_accel_event_read_validate(IOMMUFDVeventq *veventq, uint32_t type,
+ void *buf, size_t size, Error **errp);
void smmuv3_accel_reset(SMMUv3State *s);
SMMUv3AccelCmdqvType smmuv3_accel_cmdqv_type(Object *obj);
diff --git a/hw/arm/smmuv3-accel-stubs.c b/hw/arm/smmuv3-accel-stubs.c
index 147ae06163..b8dd7e7b89 100644
--- a/hw/arm/smmuv3-accel-stubs.c
+++ b/hw/arm/smmuv3-accel-stubs.c
@@ -47,10 +47,10 @@ bool smmuv3_accel_alloc_veventq(SMMUv3State *s, Error
**errp)
return true;
}
-bool smmuv3_accel_event_read_validate(IOMMUFDVeventq *veventq, uint32_t type,
- void *buf, size_t size, Error **errp)
+int smmuv3_accel_event_read_validate(IOMMUFDVeventq *veventq, uint32_t type,
+ void *buf, size_t size, Error **errp)
{
- return true;
+ return 0;
}
diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
index 80900c2521..6df19f0e0e 100644
--- a/hw/arm/smmuv3-accel.c
+++ b/hw/arm/smmuv3-accel.c
@@ -440,8 +440,13 @@ bool smmuv3_accel_issue_inv_cmd(SMMUv3State *bs, void
*cmd, SMMUDevice *sdev,
sizeof(Cmd), &entry_num, cmd, errp);
}
-bool smmuv3_accel_event_read_validate(IOMMUFDVeventq *veventq, uint32_t type,
- void *buf, size_t size, Error **errp)
+/*
+ * Returns 0 on success (buf is populated and valid).
+ * Returns 1 if the read should be retried (EAGAIN/EINTR).
+ * Returns -1 on error with @errp set.
+ */
+int smmuv3_accel_event_read_validate(IOMMUFDVeventq *veventq, uint32_t type,
+ void *buf, size_t size, Error **errp)
{
uint32_t last_seq = veventq->last_event_seq;
uint32_t id = veventq->veventq_id;
@@ -451,22 +456,22 @@ bool smmuv3_accel_event_read_validate(IOMMUFDVeventq
*veventq, uint32_t type,
bytes = read(veventq->veventq_fd, buf, size);
if (bytes <= 0) {
if (errno == EAGAIN || errno == EINTR) {
- return true;
+ return 1;
}
error_setg(errp, "vEVENTQ(type %u id %u): read failed (%m)", type, id);
- return false;
+ return -1;
}
hdr = (struct iommufd_vevent_header *)buf;
if (bytes == sizeof(*hdr) &&
(hdr->flags & IOMMU_VEVENTQ_FLAG_LOST_EVENTS)) {
error_setg(errp, "vEVENTQ(type %u id %u): overflowed", type, id);
veventq->event_start = false;
- return false;
+ return -1;
}
if (bytes < size) {
error_setg(errp, "vEVENTQ(type %u id %u): short read(%zd/%zd bytes)",
type, id, bytes, size);
- return false;
+ return -1;
}
/* Check sequence in hdr for lost events if any */
if (veventq->event_start && (hdr->sequence - last_seq != 1)) {
@@ -475,7 +480,7 @@ bool smmuv3_accel_event_read_validate(IOMMUFDVeventq
*veventq, uint32_t type,
}
veventq->last_event_seq = hdr->sequence;
veventq->event_start = true;
- return true;
+ return 0;
}
static void smmuv3_accel_event_read(void *opaque)
@@ -487,13 +492,18 @@ static void smmuv3_accel_event_read(void *opaque)
struct iommu_vevent_arm_smmuv3 vevent;
} buf;
Error *local_err = NULL;
+ int ret;
- if (!smmuv3_accel_event_read_validate(veventq,
- IOMMU_VEVENTQ_TYPE_ARM_SMMUV3, &buf,
- sizeof(buf), &local_err)) {
+ ret = smmuv3_accel_event_read_validate(veventq,
+ IOMMU_VEVENTQ_TYPE_ARM_SMMUV3, &buf,
+ sizeof(buf), &local_err);
+ if (ret < 0) {
warn_report_err_once(local_err);
return;
}
+ if (ret > 0) {
+ return; /* EAGAIN/EINTR */
+ }
smmuv3_propagate_event(s, (Evt *)&buf.vevent);
}
diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c
index 29c488e0e4..7223aa9d1d 100644
--- a/hw/arm/tegra241-cmdqv.c
+++ b/hw/arm/tegra241-cmdqv.c
@@ -841,13 +841,18 @@ static void tegra241_cmdqv_event_read(void *opaque)
struct iommu_vevent_tegra241_cmdqv vevent;
} buf;
Error *local_err = NULL;
+ int ret;
- if (!smmuv3_accel_event_read_validate(veventq,
- IOMMU_VEVENTQ_TYPE_TEGRA241_CMDQV,
- &buf, sizeof(buf), &local_err)) {
+ ret = smmuv3_accel_event_read_validate(veventq,
+ IOMMU_VEVENTQ_TYPE_TEGRA241_CMDQV,
+ &buf, sizeof(buf), &local_err);
+ if (ret < 0) {
warn_report_err_once(local_err);
return;
}
+ if (ret > 0) {
+ return; /* EAGAIN/EINTR */
+ }
if (buf.vevent.lvcmdq_err_map[0] || buf.vevent.lvcmdq_err_map[1]) {
cmdqv->vintf_cmdq_err_map[0] =
--
