| Message ID | 20210401074933.9923-4-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | esp: fix asserts/segfaults discovered by fuzzer | expand |
On 4/1/21 9:49 AM, Mark Cave-Ayland wrote: > Each FIFO currently has its own push functions with the only difference being > the capacity check. The original reason for this was that the fifo8 > implementation doesn't have a formal API for retrieving the FIFO capacity, > however there are multiple examples within QEMU where the capacity field is > accessed directly. So the Fifo8 API is not complete / practical. > Change esp_fifo_push() to access the FIFO capacity directly and then consolidate > esp_cmdfifo_push() into esp_fifo_push(). > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/scsi/esp.c | 27 ++++++++------------------- > 1 file changed, 8 insertions(+), 19 deletions(-) > > diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c > index 26fe1dcb9d..16aaf8be93 100644 > --- a/hw/scsi/esp.c > +++ b/hw/scsi/esp.c > @@ -98,16 +98,15 @@ void esp_request_cancelled(SCSIRequest *req) > } > } > > -static void esp_fifo_push(ESPState *s, uint8_t val) > +static void esp_fifo_push(Fifo8 *fifo, uint8_t val) > { > - if (fifo8_num_used(&s->fifo) == ESP_FIFO_SZ) { > + if (fifo8_num_used(fifo) == fifo->capacity) { > trace_esp_error_fifo_overrun(); > return; > } > > - fifo8_push(&s->fifo, val); > + fifo8_push(fifo, val); > } > - Spurious line removal? > static uint8_t esp_fifo_pop(ESPState *s) > { > if (fifo8_is_empty(&s->fifo)) { > @@ -117,16 +116,6 @@ static uint8_t esp_fifo_pop(ESPState *s) > return fifo8_pop(&s->fifo); > } Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On 01/04/2021 09:15, Philippe Mathieu-Daudé wrote: > On 4/1/21 9:49 AM, Mark Cave-Ayland wrote: >> Each FIFO currently has its own push functions with the only difference being >> the capacity check. The original reason for this was that the fifo8 >> implementation doesn't have a formal API for retrieving the FIFO capacity, >> however there are multiple examples within QEMU where the capacity field is >> accessed directly. > > So the Fifo8 API is not complete / practical. I guess it depends what you consider to be public and private to Fifo8: what is arguably missing is something like: int fifo8_capacity(Fifo8 *fifo) { return fifo->capacity; } But given that most other users access fifo->capacity directly then I might as well do the same and save myself requiring 2 separate implementations of esp_fifo_pop_buf() :) >> Change esp_fifo_push() to access the FIFO capacity directly and then consolidate >> esp_cmdfifo_push() into esp_fifo_push(). >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/scsi/esp.c | 27 ++++++++------------------- >> 1 file changed, 8 insertions(+), 19 deletions(-) >> >> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c >> index 26fe1dcb9d..16aaf8be93 100644 >> --- a/hw/scsi/esp.c >> +++ b/hw/scsi/esp.c >> @@ -98,16 +98,15 @@ void esp_request_cancelled(SCSIRequest *req) >> } >> } >> >> -static void esp_fifo_push(ESPState *s, uint8_t val) >> +static void esp_fifo_push(Fifo8 *fifo, uint8_t val) >> { >> - if (fifo8_num_used(&s->fifo) == ESP_FIFO_SZ) { >> + if (fifo8_num_used(fifo) == fifo->capacity) { >> trace_esp_error_fifo_overrun(); >> return; >> } >> >> - fifo8_push(&s->fifo, val); >> + fifo8_push(fifo, val); >> } >> - > > Spurious line removal? Ooops yes. I'm not too worried about this, but if Paolo has any other changes then I can roll this into a v4. >> static uint8_t esp_fifo_pop(ESPState *s) >> { >> if (fifo8_is_empty(&s->fifo)) { >> @@ -117,16 +116,6 @@ static uint8_t esp_fifo_pop(ESPState *s) >> return fifo8_pop(&s->fifo); >> } > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> ATB, Mark.
On 4/1/21 10:50 AM, Mark Cave-Ayland wrote: > On 01/04/2021 09:15, Philippe Mathieu-Daudé wrote: > >> On 4/1/21 9:49 AM, Mark Cave-Ayland wrote: >>> Each FIFO currently has its own push functions with the only >>> difference being >>> the capacity check. The original reason for this was that the fifo8 >>> implementation doesn't have a formal API for retrieving the FIFO >>> capacity, >>> however there are multiple examples within QEMU where the capacity >>> field is >>> accessed directly. >> >> So the Fifo8 API is not complete / practical. > > I guess it depends what you consider to be public and private to Fifo8: > what is arguably missing is something like: > > int fifo8_capacity(Fifo8 *fifo) > { > return fifo->capacity; > } > > But given that most other users access fifo->capacity directly then I > might as well do the same and save myself requiring 2 separate > implementations of esp_fifo_pop_buf() :) Sorry, I should have been more explicit by precising this was not a comment directed to your patch, but I was thinking loudly about the Fifo8 API; and as you mentioned the other models do that so your kludge is acceptable. >>> Change esp_fifo_push() to access the FIFO capacity directly and then >>> consolidate >>> esp_cmdfifo_push() into esp_fifo_push(). >>> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> --- >>> hw/scsi/esp.c | 27 ++++++++------------------- >>> 1 file changed, 8 insertions(+), 19 deletions(-) >>> >>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c >>> index 26fe1dcb9d..16aaf8be93 100644 >>> --- a/hw/scsi/esp.c >>> +++ b/hw/scsi/esp.c >>> @@ -98,16 +98,15 @@ void esp_request_cancelled(SCSIRequest *req) >>> } >>> } >>> -static void esp_fifo_push(ESPState *s, uint8_t val) >>> +static void esp_fifo_push(Fifo8 *fifo, uint8_t val) >>> { >>> - if (fifo8_num_used(&s->fifo) == ESP_FIFO_SZ) { >>> + if (fifo8_num_used(fifo) == fifo->capacity) { >>> trace_esp_error_fifo_overrun(); >>> return; >>> } >>> - fifo8_push(&s->fifo, val); >>> + fifo8_push(fifo, val); >>> } >>> - >> >> Spurious line removal? > > Ooops yes. I'm not too worried about this, but if Paolo has any other > changes then I can roll this into a v4. Actually it reappears in patch 05/11 ;)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 26fe1dcb9d..16aaf8be93 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -98,16 +98,15 @@ void esp_request_cancelled(SCSIRequest *req) } } -static void esp_fifo_push(ESPState *s, uint8_t val) +static void esp_fifo_push(Fifo8 *fifo, uint8_t val) { - if (fifo8_num_used(&s->fifo) == ESP_FIFO_SZ) { + if (fifo8_num_used(fifo) == fifo->capacity) { trace_esp_error_fifo_overrun(); return; } - fifo8_push(&s->fifo, val); + fifo8_push(fifo, val); } - static uint8_t esp_fifo_pop(ESPState *s) { if (fifo8_is_empty(&s->fifo)) { @@ -117,16 +116,6 @@ static uint8_t esp_fifo_pop(ESPState *s) return fifo8_pop(&s->fifo); } -static void esp_cmdfifo_push(ESPState *s, uint8_t val) -{ - if (fifo8_num_used(&s->cmdfifo) == ESP_CMDFIFO_SZ) { - trace_esp_error_fifo_overrun(); - return; - } - - fifo8_push(&s->cmdfifo, val); -} - static uint8_t esp_cmdfifo_pop(ESPState *s) { if (fifo8_is_empty(&s->cmdfifo)) { @@ -187,9 +176,9 @@ static void esp_pdma_write(ESPState *s, uint8_t val) } if (s->do_cmd) { - esp_cmdfifo_push(s, val); + esp_fifo_push(&s->cmdfifo, val); } else { - esp_fifo_push(s, val); + esp_fifo_push(&s->fifo, val); } dmalen--; @@ -645,7 +634,7 @@ static void esp_do_dma(ESPState *s) */ if (len < esp_get_tc(s) && esp_get_tc(s) <= ESP_FIFO_SZ) { while (fifo8_num_used(&s->fifo) < ESP_FIFO_SZ) { - esp_fifo_push(s, 0); + esp_fifo_push(&s->fifo, 0); len++; } } @@ -947,9 +936,9 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val) break; case ESP_FIFO: if (s->do_cmd) { - esp_cmdfifo_push(s, val); + esp_fifo_push(&s->cmdfifo, val); } else { - esp_fifo_push(s, val); + esp_fifo_push(&s->fifo, val); } /* Non-DMA transfers raise an interrupt after every byte */
Each FIFO currently has its own push functions with the only difference being the capacity check. The original reason for this was that the fifo8 implementation doesn't have a formal API for retrieving the FIFO capacity, however there are multiple examples within QEMU where the capacity field is accessed directly. Change esp_fifo_push() to access the FIFO capacity directly and then consolidate esp_cmdfifo_push() into esp_fifo_push(). Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-)