> -----Original Message----- > From: Peter Maydell <[email protected]> > Sent: 22 June 2026 16:28 > To: [email protected] > Cc: Shameer Kolothum Thodi <[email protected]>; Nicolin Chen > <[email protected]>; Eric Auger <[email protected]> > Subject: Re: [PULL 32/61] hw/arm/smmuv3-accel: Introduce common helper > for veventq read > > External email: Use caution opening links or attachments > > > On Tue, 16 Jun 2026 at 20:07, Peter Maydell <[email protected]> > wrote: > > > > From: Shameer Kolothum <[email protected]> > > > > Move the vEVENTQ read and validation logic into a common helper > > smmuv3_accel_event_read_validate(). The helper performs the read(), > > checks for overflow and short reads, validates the sequence number, > > and updates the sequence state. > > > > This helper can be reused for Tegra241 CMDQV vEVENTQ support in a > > subsequent patch. > > > > Error handling is slightly adjusted: instead of reporting errors > > directly in the read handler, the helper now returns errors via > > Error **. Sequence gaps are reported as warnings. > > Hi; Coverity spots what looks like an error here (CID 1660057); > could you have a look and send a fix, please? > > > Reviewed-by: Nicolin Chen <[email protected]> > > > +bool 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; > > + struct iommufd_vevent_header *hdr; > > + ssize_t bytes; > > + > > + bytes = read(veventq->veventq_fd, buf, size); > > + if (bytes <= 0) { > > + if (errno == EAGAIN || errno == EINTR) { > > + return true; > > + } > > + error_setg(errp, "vEVENTQ(type %u id %u): read failed (%m)", type, > > id); > > + return false; > > + } > > + 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; > > + } > > + if (bytes < size) { > > + error_setg(errp, "vEVENTQ(type %u id %u): short read(%zd/%zd > bytes)", > > + type, id, bytes, size); > > + return false; > > + } > > + /* Check sequence in hdr for lost events if any */ > > + if (veventq->event_start && (hdr->sequence - last_seq != 1)) { > > + warn_report("vEVENTQ(type %u id %u): lost %u event(s)", > > + type, id, hdr->sequence - last_seq - 1); > > + } > > + veventq->last_event_seq = hdr->sequence; > > + veventq->event_start = true; > > + return true; > > This function takes a buf and size that it will read into. It returns > falso on various error cases, and true on success. In particular, > "didn't read exactly the requested number of bytes" is an error case. > However, it also returns true for EAGAIN and EINTR, having not read > any data into the buffer. The API provides no way for the caller > to tell the difference between "successfully read the data" and > "you need to try again later". > > > +} > > + > > static void smmuv3_accel_event_read(void *opaque) > > { > > SMMUv3State *s = opaque; > > @@ -448,39 +486,14 @@ static void smmuv3_accel_event_read(void > *opaque) > > struct iommufd_vevent_header hdr; > > struct iommu_vevent_arm_smmuv3 vevent; > > } buf; > > - enum iommu_veventq_type type = > IOMMU_VEVENTQ_TYPE_ARM_SMMUV3; > > - uint32_t id = veventq->veventq_id; > > - uint32_t last_seq = veventq->last_event_seq; > > - ssize_t bytes; > > + Error *local_err = NULL; > > > > - bytes = read(veventq->veventq_fd, &buf, sizeof(buf)); > > - if (bytes <= 0) { > > - if (errno == EAGAIN || errno == EINTR) { > > - return; > > - } > > - error_report_once("vEVENTQ(type %u id %u): read failed (%m)", type, > id); > > Here at the callsite, we used to return from smmuv3_accel_event_read() > without doing anything for the EAGAIN/EINTR case, report the error > in other error cases, and continue to smmuv3_propagate_event() only > for the success case. > > > > + if (!smmuv3_accel_event_read_validate(veventq, > > + IOMMU_VEVENTQ_TYPE_ARM_SMMUV3, > > &buf, > > + sizeof(buf), &local_err)) { > > + warn_report_err_once(local_err); > > return; > > } > > In the new code, we report the failures (good), proceed for the > success case (also good), but we now continue to call > smmuv3_propagate_event() with an uninitialized buf struct if > the read call returned EAGAIN/EINTR...
Yes, this is a valid bug. I will send a fix. Thanks, Shameer
