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... > - > - if (bytes == sizeof(buf.hdr) && > - (buf.hdr.flags & IOMMU_VEVENTQ_FLAG_LOST_EVENTS)) { > - error_report_once("vEVENTQ(type %u id %u): overflowed", type, id); > - veventq->event_start = false; > - return; > - } > - if (bytes < sizeof(buf)) { > - error_report_once("vEVENTQ(type %u id %u): short read(%zd/%zd > bytes)", > - type, id, bytes, sizeof(buf)); > - return; > - } > - > - /* Check sequence in hdr for lost events if any */ > - if (veventq->event_start && (buf.hdr.sequence - last_seq != 1)) { > - error_report_once("vEVENTQ(type %u id %u): lost %u event(s)", > - type, id, buf.hdr.sequence - last_seq - 1); > - } > - veventq->last_event_seq = buf.hdr.sequence; > - veventq->event_start = true; > smmuv3_propagate_event(s, (Evt *)&buf.vevent); > } thanks -- PMM
