| Message ID | 20240722160745.67904-7-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | util/fifo8: Rework fifo8_pop_buf() | expand |
On 7/22/24 09:07, Philippe Mathieu-Daudé wrote: > Extract fifo8_pop_buf() from hw/scsi/esp.c and expose > it as part of the <qemu/fifo8.h> API. This function takes > care of non-contiguous (wrapped) FIFO buffer (which is an > implementation detail). > > Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > include/qemu/fifo8.h | 14 ++++++++++++++ > hw/scsi/esp.c | 36 +++--------------------------------- > util/fifo8.c | 29 +++++++++++++++++++++++++++++ > 3 files changed, 46 insertions(+), 33 deletions(-) > > diff --git a/include/qemu/fifo8.h b/include/qemu/fifo8.h > index 686918a3a4..21c7a22937 100644 > --- a/include/qemu/fifo8.h > +++ b/include/qemu/fifo8.h > @@ -62,6 +62,20 @@ void fifo8_push_all(Fifo8 *fifo, const uint8_t *data, uint32_t num); > */ > uint8_t fifo8_pop(Fifo8 *fifo); > > +/** > + * fifo8_pop_buf: > + * @fifo: FIFO to pop from > + * @dest: the buffer to write the data into (can be NULL) > + * @destlen: size of @dest and maximum number of bytes to pop > + * > + * Pop a number of elements from the FIFO up to a maximum of @destlen. > + * The popped data is copied into the @dest buffer. > + * Care is taken when the data wraps around in the ring buffer. > + * > + * Returns: number of bytes popped. > + */ > +uint32_t fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, uint32_t destlen); > + > /** > * fifo8_pop_constbuf: > * @fifo: FIFO to pop from > diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c > index 64384f9b0e..cec847b54a 100644 > --- a/hw/scsi/esp.c > +++ b/hw/scsi/esp.c > @@ -197,39 +197,9 @@ static uint8_t esp_fifo_pop(ESPState *s) > return val; > } > > -static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen) > -{ > - const uint8_t *buf; > - uint32_t n, n2; > - int len; > - > - if (maxlen == 0) { > - return 0; > - } > - > - len = maxlen; > - buf = fifo8_pop_constbuf(fifo, len, &n); > - if (dest) { > - memcpy(dest, buf, n); > - } > - > - /* Add FIFO wraparound if needed */ > - len -= n; > - len = MIN(len, fifo8_num_used(fifo)); > - if (len) { > - buf = fifo8_pop_constbuf(fifo, len, &n2); > - if (dest) { > - memcpy(&dest[n], buf, n2); > - } > - n += n2; > - } > - > - return n; > -} > - > static uint32_t esp_fifo_pop_buf(ESPState *s, uint8_t *dest, int maxlen) > { > - uint32_t len = esp_fifo8_pop_buf(&s->fifo, dest, maxlen); > + uint32_t len = fifo8_pop_buf(&s->fifo, dest, maxlen); > > esp_update_drq(s); > return len; > @@ -335,7 +305,7 @@ static void do_command_phase(ESPState *s) > if (!cmdlen || !s->current_dev) { > return; > } > - esp_fifo8_pop_buf(&s->cmdfifo, buf, cmdlen); > + fifo8_pop_buf(&s->cmdfifo, buf, cmdlen); > > current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, s->lun); > if (!current_lun) { > @@ -381,7 +351,7 @@ static void do_message_phase(ESPState *s) > /* Ignore extended messages for now */ > if (s->cmdfifo_cdb_offset) { > int len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo)); > - esp_fifo8_pop_buf(&s->cmdfifo, NULL, len); > + fifo8_pop_buf(&s->cmdfifo, NULL, len); > s->cmdfifo_cdb_offset = 0; > } > } > diff --git a/util/fifo8.c b/util/fifo8.c > index 31f0d34c0c..6610b79182 100644 > --- a/util/fifo8.c > +++ b/util/fifo8.c > @@ -102,6 +102,35 @@ const uint8_t *fifo8_pop_constbuf(Fifo8 *fifo, uint32_t max, uint32_t *numptr) > return fifo8_peekpop_buf(fifo, max, numptr, true); > } > > +uint32_t fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, uint32_t destlen) > +{ > + const uint8_t *buf; > + uint32_t n1, n2 = 0; > + uint32_t len; > + > + if (destlen == 0) { > + return 0; > + } > + > + len = destlen; > + buf = fifo8_pop_constbuf(fifo, len, &n1); > + if (dest) { > + memcpy(dest, buf, n1); > + } > + > + /* Add FIFO wraparound if needed */ > + len -= n1; > + len = MIN(len, fifo8_num_used(fifo)); > + if (len) { > + buf = fifo8_pop_constbuf(fifo, len, &n2); > + if (dest) { > + memcpy(&dest[n1], buf, n2); > + } > + } > + > + return n1 + n2; > +} > + > bool fifo8_is_empty(Fifo8 *fifo) > { > return (fifo->num == 0); Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
On 22/07/2024 17:07, Philippe Mathieu-Daudé wrote: > Extract fifo8_pop_buf() from hw/scsi/esp.c and expose > it as part of the <qemu/fifo8.h> API. This function takes > care of non-contiguous (wrapped) FIFO buffer (which is an > implementation detail). I wonder if it is also worth updating the comment for fifo8_pop_bufptr() indicating that it returns a pointer to the internal buffer without checking for overflow, and that in general fifo8_pop_buf() is recommended instead? Otherwise: Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> ATB, Mark. > Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > include/qemu/fifo8.h | 14 ++++++++++++++ > hw/scsi/esp.c | 36 +++--------------------------------- > util/fifo8.c | 29 +++++++++++++++++++++++++++++ > 3 files changed, 46 insertions(+), 33 deletions(-) > > diff --git a/include/qemu/fifo8.h b/include/qemu/fifo8.h > index 686918a3a4..21c7a22937 100644 > --- a/include/qemu/fifo8.h > +++ b/include/qemu/fifo8.h > @@ -62,6 +62,20 @@ void fifo8_push_all(Fifo8 *fifo, const uint8_t *data, uint32_t num); > */ > uint8_t fifo8_pop(Fifo8 *fifo); > > +/** > + * fifo8_pop_buf: > + * @fifo: FIFO to pop from > + * @dest: the buffer to write the data into (can be NULL) > + * @destlen: size of @dest and maximum number of bytes to pop > + * > + * Pop a number of elements from the FIFO up to a maximum of @destlen. > + * The popped data is copied into the @dest buffer. > + * Care is taken when the data wraps around in the ring buffer. > + * > + * Returns: number of bytes popped. > + */ > +uint32_t fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, uint32_t destlen); > + > /** > * fifo8_pop_constbuf: > * @fifo: FIFO to pop from > diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c > index 64384f9b0e..cec847b54a 100644 > --- a/hw/scsi/esp.c > +++ b/hw/scsi/esp.c > @@ -197,39 +197,9 @@ static uint8_t esp_fifo_pop(ESPState *s) > return val; > } > > -static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen) > -{ > - const uint8_t *buf; > - uint32_t n, n2; > - int len; > - > - if (maxlen == 0) { > - return 0; > - } > - > - len = maxlen; > - buf = fifo8_pop_constbuf(fifo, len, &n); > - if (dest) { > - memcpy(dest, buf, n); > - } > - > - /* Add FIFO wraparound if needed */ > - len -= n; > - len = MIN(len, fifo8_num_used(fifo)); > - if (len) { > - buf = fifo8_pop_constbuf(fifo, len, &n2); > - if (dest) { > - memcpy(&dest[n], buf, n2); > - } > - n += n2; > - } > - > - return n; > -} > - > static uint32_t esp_fifo_pop_buf(ESPState *s, uint8_t *dest, int maxlen) > { > - uint32_t len = esp_fifo8_pop_buf(&s->fifo, dest, maxlen); > + uint32_t len = fifo8_pop_buf(&s->fifo, dest, maxlen); > > esp_update_drq(s); > return len; > @@ -335,7 +305,7 @@ static void do_command_phase(ESPState *s) > if (!cmdlen || !s->current_dev) { > return; > } > - esp_fifo8_pop_buf(&s->cmdfifo, buf, cmdlen); > + fifo8_pop_buf(&s->cmdfifo, buf, cmdlen); > > current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, s->lun); > if (!current_lun) { > @@ -381,7 +351,7 @@ static void do_message_phase(ESPState *s) > /* Ignore extended messages for now */ > if (s->cmdfifo_cdb_offset) { > int len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo)); > - esp_fifo8_pop_buf(&s->cmdfifo, NULL, len); > + fifo8_pop_buf(&s->cmdfifo, NULL, len); > s->cmdfifo_cdb_offset = 0; > } > } > diff --git a/util/fifo8.c b/util/fifo8.c > index 31f0d34c0c..6610b79182 100644 > --- a/util/fifo8.c > +++ b/util/fifo8.c > @@ -102,6 +102,35 @@ const uint8_t *fifo8_pop_constbuf(Fifo8 *fifo, uint32_t max, uint32_t *numptr) > return fifo8_peekpop_buf(fifo, max, numptr, true); > } > > +uint32_t fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, uint32_t destlen) > +{ > + const uint8_t *buf; > + uint32_t n1, n2 = 0; > + uint32_t len; > + > + if (destlen == 0) { > + return 0; > + } > + > + len = destlen; > + buf = fifo8_pop_constbuf(fifo, len, &n1); > + if (dest) { > + memcpy(dest, buf, n1); > + } > + > + /* Add FIFO wraparound if needed */ > + len -= n1; > + len = MIN(len, fifo8_num_used(fifo)); > + if (len) { > + buf = fifo8_pop_constbuf(fifo, len, &n2); > + if (dest) { > + memcpy(&dest[n1], buf, n2); > + } > + } > + > + return n1 + n2; > +} > + > bool fifo8_is_empty(Fifo8 *fifo) > { > return (fifo->num == 0);
Hi Mark, On 22/7/24 23:26, Mark Cave-Ayland wrote: > On 22/07/2024 17:07, Philippe Mathieu-Daudé wrote: > >> Extract fifo8_pop_buf() from hw/scsi/esp.c and expose >> it as part of the <qemu/fifo8.h> API. This function takes >> care of non-contiguous (wrapped) FIFO buffer (which is an >> implementation detail). > > I wonder if it is also worth updating the comment for fifo8_pop_bufptr() > indicating that it returns a pointer to the internal buffer without > checking for overflow, We document: * The function may return fewer bytes than requested when the data wraps * around in the ring buffer; in this case only a contiguous part of the data * is returned. but I'll try to reword a bit. > and that in general fifo8_pop_buf() is > recommended instead? Yes, this was my first motivation but then I forgot to write it :) BTW I now see fifo8_pop/peek_bufptr() as dangerous API and am thinking of deprecating them (after release). AFAICT the difference is a pair of memcpy(), when I expect to not be that important performance wise. > Otherwise: > > Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Thanks! BTW I'll respin this series including the fifo8_peek_buf() patch that I forgot and is the one I need in PL011. Preview: +uint32_t fifo8_peek_buf(Fifo8 *fifo, uint8_t *dest, uint32_t destlen) +{ + uint32_t tail_count, head_count = 0; + + if (destlen == 0) { + return 0; + } + + destlen = MIN(destlen, fifo->num); + tail_count = MIN(fifo->capacity - fifo->head, destlen); + + if (dest) { + memcpy(dest, &fifo->data[fifo->head], tail_count); + } + + /* Add FIFO wraparound if needed */ + destlen -= tail_count; + head_count = MIN(destlen, fifo->head); + if (head_count && dest) { + memcpy(&dest[tail_count], &fifo->data[0], head_count); + } + + return tail_count + head_count; +} ---
On 22/07/2024 22:39, Philippe Mathieu-Daudé wrote: > Hi Mark, > > On 22/7/24 23:26, Mark Cave-Ayland wrote: >> On 22/07/2024 17:07, Philippe Mathieu-Daudé wrote: >> >>> Extract fifo8_pop_buf() from hw/scsi/esp.c and expose >>> it as part of the <qemu/fifo8.h> API. This function takes >>> care of non-contiguous (wrapped) FIFO buffer (which is an >>> implementation detail). >> >> I wonder if it is also worth updating the comment for fifo8_pop_bufptr() indicating >> that it returns a pointer to the internal buffer without checking for overflow, > > We document: > > * The function may return fewer bytes than requested when the data wraps > * around in the ring buffer; in this case only a contiguous part of the data > * is returned. > > but I'll try to reword a bit. > >> and that in general fifo8_pop_buf() is recommended instead? > > Yes, this was my first motivation but then I forgot to write it :) > > BTW I now see fifo8_pop/peek_bufptr() as dangerous API and am thinking > of deprecating them (after release). AFAICT the difference is a pair of > memcpy(), when I expect to not be that important performance wise. Funny - I had exactly the same thought ;) >> Otherwise: >> >> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > Thanks! > > BTW I'll respin this series including the fifo8_peek_buf() patch that > I forgot and is the one I need in PL011. Preview: > > +uint32_t fifo8_peek_buf(Fifo8 *fifo, uint8_t *dest, uint32_t destlen) > +{ > + uint32_t tail_count, head_count = 0; > + > + if (destlen == 0) { > + return 0; > + } > + > + destlen = MIN(destlen, fifo->num); > + tail_count = MIN(fifo->capacity - fifo->head, destlen); > + > + if (dest) { > + memcpy(dest, &fifo->data[fifo->head], tail_count); > + } > + > + /* Add FIFO wraparound if needed */ > + destlen -= tail_count; > + head_count = MIN(destlen, fifo->head); > + if (head_count && dest) { > + memcpy(&dest[tail_count], &fifo->data[0], head_count); > + } > + > + return tail_count + head_count; > +} Looks good at first glance, although it's getting late here now. If you're looking at making a few more changes before a respin, is it worth considering to add a new file with qtests for the updated Fifo8 implementation? ATB, Mark.
diff --git a/include/qemu/fifo8.h b/include/qemu/fifo8.h index 686918a3a4..21c7a22937 100644 --- a/include/qemu/fifo8.h +++ b/include/qemu/fifo8.h @@ -62,6 +62,20 @@ void fifo8_push_all(Fifo8 *fifo, const uint8_t *data, uint32_t num); */ uint8_t fifo8_pop(Fifo8 *fifo); +/** + * fifo8_pop_buf: + * @fifo: FIFO to pop from + * @dest: the buffer to write the data into (can be NULL) + * @destlen: size of @dest and maximum number of bytes to pop + * + * Pop a number of elements from the FIFO up to a maximum of @destlen. + * The popped data is copied into the @dest buffer. + * Care is taken when the data wraps around in the ring buffer. + * + * Returns: number of bytes popped. + */ +uint32_t fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, uint32_t destlen); + /** * fifo8_pop_constbuf: * @fifo: FIFO to pop from diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 64384f9b0e..cec847b54a 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -197,39 +197,9 @@ static uint8_t esp_fifo_pop(ESPState *s) return val; } -static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen) -{ - const uint8_t *buf; - uint32_t n, n2; - int len; - - if (maxlen == 0) { - return 0; - } - - len = maxlen; - buf = fifo8_pop_constbuf(fifo, len, &n); - if (dest) { - memcpy(dest, buf, n); - } - - /* Add FIFO wraparound if needed */ - len -= n; - len = MIN(len, fifo8_num_used(fifo)); - if (len) { - buf = fifo8_pop_constbuf(fifo, len, &n2); - if (dest) { - memcpy(&dest[n], buf, n2); - } - n += n2; - } - - return n; -} - static uint32_t esp_fifo_pop_buf(ESPState *s, uint8_t *dest, int maxlen) { - uint32_t len = esp_fifo8_pop_buf(&s->fifo, dest, maxlen); + uint32_t len = fifo8_pop_buf(&s->fifo, dest, maxlen); esp_update_drq(s); return len; @@ -335,7 +305,7 @@ static void do_command_phase(ESPState *s) if (!cmdlen || !s->current_dev) { return; } - esp_fifo8_pop_buf(&s->cmdfifo, buf, cmdlen); + fifo8_pop_buf(&s->cmdfifo, buf, cmdlen); current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, s->lun); if (!current_lun) { @@ -381,7 +351,7 @@ static void do_message_phase(ESPState *s) /* Ignore extended messages for now */ if (s->cmdfifo_cdb_offset) { int len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo)); - esp_fifo8_pop_buf(&s->cmdfifo, NULL, len); + fifo8_pop_buf(&s->cmdfifo, NULL, len); s->cmdfifo_cdb_offset = 0; } } diff --git a/util/fifo8.c b/util/fifo8.c index 31f0d34c0c..6610b79182 100644 --- a/util/fifo8.c +++ b/util/fifo8.c @@ -102,6 +102,35 @@ const uint8_t *fifo8_pop_constbuf(Fifo8 *fifo, uint32_t max, uint32_t *numptr) return fifo8_peekpop_buf(fifo, max, numptr, true); } +uint32_t fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, uint32_t destlen) +{ + const uint8_t *buf; + uint32_t n1, n2 = 0; + uint32_t len; + + if (destlen == 0) { + return 0; + } + + len = destlen; + buf = fifo8_pop_constbuf(fifo, len, &n1); + if (dest) { + memcpy(dest, buf, n1); + } + + /* Add FIFO wraparound if needed */ + len -= n1; + len = MIN(len, fifo8_num_used(fifo)); + if (len) { + buf = fifo8_pop_constbuf(fifo, len, &n2); + if (dest) { + memcpy(&dest[n1], buf, n2); + } + } + + return n1 + n2; +} + bool fifo8_is_empty(Fifo8 *fifo) { return (fifo->num == 0);
Extract fifo8_pop_buf() from hw/scsi/esp.c and expose it as part of the <qemu/fifo8.h> API. This function takes care of non-contiguous (wrapped) FIFO buffer (which is an implementation detail). Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/qemu/fifo8.h | 14 ++++++++++++++ hw/scsi/esp.c | 36 +++--------------------------------- util/fifo8.c | 29 +++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 33 deletions(-)