Re: [PATCH v2 2/2] hw/usb/hcd-ehci: Check for DMA errors in get_dwords()/put_dwords()

On 17/8/26 04:36, Jamin Lin wrote:
> get_dwords() and put_dwords() return -1 when ehci->as is NULL, but that
> can no longer happen. The sysbus variants set it in instance_init
> (ehci_sysbus_init()), the PCI variant sets it in realize
> (usb_ehci_pci_realize()), and usb_ehci_pci_write_config() only switches
> between the bus master address space and address_space_memory.
> 
> Meanwhile the results of dma_memory_read() and dma_memory_write() are
> ignored, so a failed guest memory access is silently treated as success.
> 
> Drop the dead NULL test and check the MemTxResult of each access
> instead, so that the existing -1 error path reports real DMA errors:
> raise USBSTS_HSE, clear USBCMD_RUNSTOP and stop processing the
> descriptor, as the NULL path used to do. All 11 get_dwords() callers
> already check the return value.
> 
> put_dwords() now returns void: none of its four callers (ehci_flush_qh(),
> ehci_state_fetchitd() and twice in ehci_state_writeback()) looks at the
> status.
> 

Suggested-by: Peter Maydell <[email protected]>

> Signed-off-by: Jamin Lin <[email protected]>
> ---
>   hw/usb/hcd-ehci.c | 38 ++++++++++++++++----------------------
>   1 file changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index d43951975a..daab5783bc 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -421,16 +421,14 @@ static inline int get_dwords(EHCIState *ehci, uint64_t addr,
>   {
>       int i;
>   
> -    if (!ehci->as) {
> -        ehci_raise_irq(ehci, USBSTS_HSE);
> -        ehci->usbcmd &= ~USBCMD_RUNSTOP;
> -        trace_usb_ehci_dma_error();
> -        return -1;
> -    }
> -
>       for (i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
> -        dma_memory_read(ehci->as, addr, buf, sizeof(*buf),
> -                        MEMTXATTRS_UNSPECIFIED);
> +        if (dma_memory_read(ehci->as, addr, buf, sizeof(*buf),
> +                            MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
> +            ehci_raise_irq(ehci, USBSTS_HSE);
> +            ehci->usbcmd &= ~USBCMD_RUNSTOP;
> +            trace_usb_ehci_dma_error();
> +            return -1;
> +        }
>           *buf = le32_to_cpu(*buf);
>       }

Maybe reorder before previous patch?

Note, for EHCI get_dwords() is only used as returning a boolean,
which would match with your conversion of put_dwords to void. While
the change involves some code churn, the result would be clearer
IMHO. Anyway can be done later.

Reviewed-by: Philippe Mathieu-Daudé <[email protected]>