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

Hi Philippe

> Subject: 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?
> 
Will resend v3.

> 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.
> 
Will add new patch in v3 and change get_dwords return type to a boolean.
Thanks,
Jamin

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