Re: [RFC PATCH 4/4] hw/arm/smmuv3-accel: Read and propagate host vIOMMU events

[Jonathan Cameron via <qemu-arm@nongnu.org>]

On Wed, 5 Nov 2025 15:46:52 +0000
Shameer Kolothum <skolothumtho@nvidia.com> wrote:

> Install an event handler on the vEVENTQ fd to read and propagate host
> generated vIOMMU 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: Shameer Kolothum <skolothumtho@nvidia.com>
A few minor suggestions inline.  Otherwise set looks good to me, though
I'm very far from an expert of this stuff!

Jonathan

> ---
>  hw/arm/smmuv3-accel.c | 62 +++++++++++++++++++++++++++++++++++++++++++
>  hw/arm/smmuv3-accel.h |  2 ++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
> index 210e7ebf36..e6c81c4786 100644
> --- a/hw/arm/smmuv3-accel.c
> +++ b/hw/arm/smmuv3-accel.c
> @@ -383,6 +383,62 @@ static SMMUv3AccelDevice *smmuv3_accel_get_dev(SMMUState *bs, SMMUPciBus *sbus,
>      return accel_dev;
>  }
>  
> +static void smmuv3_accel_event_read(void *opaque)
> +{
> +    SMMUv3State *s = opaque;
> +    SMMUv3AccelState *s_accel = s->s_accel;
> +    SMMUViommu *vsmmu = s_accel->vsmmu;
> +    struct iommu_vevent_arm_smmuv3 *vevent;
> +    struct iommufd_vevent_header *hdr;
> +    ssize_t readsz = sizeof(*hdr) + sizeof(*vevent);
> +    uint8_t buf[sizeof(*hdr) + sizeof(*vevent)];
Could you wrap this up in a structure to make it a tiny
bit more obvious what is going on?

    struct {
        struct iommufd_vevent_header hdr;
        struct iommufd_vevent_arm_smmuv3 vevent;
    } buf;

Should allow sizeof(buf);
and accessing elements directly without casts.

> +    uint32_t last_seq = vsmmu->last_event_seq;
> +    ssize_t bytes;
> +    Evt evt = {};
Given you copy into this based on sizeof(evt) I can't see why you need
to initialize.

> +
> +    bytes = read(vsmmu->veventq->veventq_fd, buf, readsz);
> +    if (bytes <= 0) {
> +        if (errno == EAGAIN || errno == EINTR) {
> +            return;
> +        }
> +        error_report("vEVENTQ: read failed (%s)", strerror(errno));
> +        return;
> +    }
> +
> +    if (bytes < readsz) {
> +        error_report("vEVENTQ: incomplete read (%zd/%zd bytes)", bytes, readsz);
> +        return;
> +    }
> +
> +    hdr = (struct iommufd_vevent_header *)buf;
> +    if (hdr->flags & IOMMU_VEVENTQ_FLAG_LOST_EVENTS) {
> +        error_report("vEVENTQ has lost events");
> +        return;
> +    }
> +
> +    vevent = (struct iommu_vevent_arm_smmuv3 *)(buf + sizeof(*hdr));
> +    /* Check sequence in hdr for lost events if any */
> +    if (vsmmu->event_start) {
> +        uint32_t expected = (last_seq == INT_MAX) ? 0 : last_seq + 1;
> +
> +        if (hdr->sequence != expected) {
> +            uint32_t delta;
> +
> +            if (hdr->sequence >= last_seq) {
> +                delta = hdr->sequence - last_seq;
> +            } else {
> +                /* Handle wraparound from INT_MAX */
> +                delta = (INT_MAX - last_seq) + hdr->sequence + 1;
> +            }
> +            error_report("vEVENTQ: detected lost %u event(s)", delta - 1);
> +        }
> +    }
> +    vsmmu->last_event_seq = hdr->sequence;
> +    vsmmu->event_start = true;
> +    memcpy(&evt, vevent, sizeof(evt));
> +    smmuv3_propagate_event(s, &evt);
Why is the copy needed?  Can't you just use the vevent in place?
> +}