[PATCH] ide: clean up ahci_populate_sglist

[PATCH] ide: clean up ahci_populate_sglist

[Paolo Bonzini]

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.

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"
-- 
2.26.2

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"
>