Re: [PATCH] ide: clean up ahci_populate_sglist

[Philippe Mathieu-Daudé]

On 10/5/20 2:55 PM, Paolo Bonzini wrote:
> Alex reported an uninitialized variable warning in ahci_populate_sglist.
> Even though the warning is bogus and happens only because of -Og, the
> code in the function leaves something to be desired; the condition that
> triggers the warning is easily shown to be entirely redundant.
> 
> In particular, the loop's "if" condition can be rewritten from
> "offset < sum + tbl_entry_size" to "offset - sum < tbl_entry_size";
> this is safe since the LHS cannot underflow.  Because off_pos is
> exactly "offset - sum" it is clear that it can never be less than
> zero or greater than tbl_entry_size.  We can therefore keep the off_idx
> check only and, for documentation purposes, reduce off_pos to an unsigned
> 32-bit integer.
> 
> The tracepoint also is not particularly useful at this point, since
> we know that (if it ever triggers) off_idx will be -1 and off_pos
> uninitialized.  Instead, include the requested offset and the total PRDT
> length, which will be smaller than the offset.
> 

Reported-by: Alex Bennée <alex.bennee@linaro.org>
so we know which 'Alex', and:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ide/ahci.c       | 12 +++++-------
>  hw/ide/trace-events |  2 +-
>  2 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 680304a24c..997b67a6fc 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -924,8 +924,7 @@ static int ahci_populate_sglist(AHCIDevice *ad, 
> QEMUSGList *sglist,
>      int r = 0;
>      uint64_t sum = 0;
>      int off_idx = -1;
> -    int64_t off_pos = -1;
> -    int tbl_entry_size;
> +    uint32_t off_pos = 0;
>      IDEBus *bus = &ad->port;
>      BusState *qbus = BUS(bus);
>  
> @@ -952,19 +951,18 @@ static int ahci_populate_sglist(AHCIDevice *ad, 
> QEMUSGList *sglist,
>      /* Get entries in the PRDT, init a qemu sglist accordingly */
>      if (prdtl > 0) {
>          AHCI_SG *tbl = (AHCI_SG *)prdt;
> -        sum = 0;
>          for (i = 0; i < prdtl; i++) {
> -            tbl_entry_size = prdt_tbl_entry_size(&tbl[i]);
> -            if (offset < (sum + tbl_entry_size)) {
> +            uint32_t tbl_entry_size = prdt_tbl_entry_size(&tbl[i]);
> +            if (offset - sum < tbl_entry_size) {
>                  off_idx = i;
>                  off_pos = offset - sum;
>                  break;
>              }
>              sum += tbl_entry_size;
>          }
> -        if ((off_idx == -1) || (off_pos < 0) || (off_pos > tbl_entry_size)) {
> +        if (off_idx == -1) {
>              trace_ahci_populate_sglist_bad_offset(ad->hba, ad->port_no,
> -                                                  off_idx, off_pos);
> +                                                  sum, offset);
>              r = -1;
>              goto out;
>          }
> diff --git a/hw/ide/trace-events b/hw/ide/trace-events
> index 6e357685f9..81706efe80 100644
> --- a/hw/ide/trace-events
> +++ b/hw/ide/trace-events
> @@ -88,7 +88,7 @@ ahci_populate_sglist(void *s, int port) "ahci(%p)[%d]"
>  ahci_populate_sglist_no_prdtl(void *s, int port, uint16_t opts) 
> "ahci(%p)[%d]: no sg list given by guest: 0x%04x"
>  ahci_populate_sglist_no_map(void *s, int port) "ahci(%p)[%d]: DMA mapping 
> failed"
>  ahci_populate_sglist_short_map(void *s, int port) "ahci(%p)[%d]: mapped less 
> than expected"
> -ahci_populate_sglist_bad_offset(void *s, int port, int off_idx, int64_t 
> off_pos) "ahci(%p)[%d]: Incorrect offset! off_idx: %d, off_pos: %"PRId64
> +ahci_populate_sglist_bad_offset(void *s, int port, uint64_t sum, uint64_t 
> offset) "ahci(%p)[%d]: Incorrect offset! total PRDT length %"PRIu64", offset: 
> %"PRIu64
>  ncq_finish(void *s, int port, uint8_t tag) "ahci(%p)[%d][tag:%d]: NCQ 
> transfer finished"
>  execute_ncq_command_read(void *s, int port, uint8_t tag, int count, int64_t 
> lba) "ahci(%p)[%d][tag:%d]: NCQ reading %d sectors from LBA %"PRId64
>  execute_ncq_command_unsup(void *s, int port, uint8_t tag, uint8_t cmd) 
> "ahci(%p)[%d][tag:%d]: error: unsupported NCQ command (0x%02x) received"
>