[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] ide: clean up ahci_populate_sglist
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH] ide: clean up ahci_populate_sglist |
Date: |
Mon, 5 Oct 2020 16:07:23 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 |
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"
>