RE: [PATCH v1] hw/usb/hcd-ehci: Handle get_dwords() failures in async writeback

Hi Peter,

> > diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index
> > 451a918e9f..d43951975a 100644
> > --- a/hw/usb/hcd-ehci.c
> > +++ b/hw/usb/hcd-ehci.c
> > @@ -533,11 +533,11 @@ static void
> ehci_writeback_async_complete_packet(EHCIPacket *p)
> >      /* Verify the qh + qtd, like we do when going through fetchqh &
> fetchqtd */
> >      memset(&qh, 0, sizeof(qh));
> >      memset(&qtd, 0, sizeof(qtd));
> > -    get_dwords(q->ehci, NLPTR_GET(q->qhaddr),
> > -               (uint32_t *) &qh, ehci_qh_dwords(q->ehci));
> > -    get_dwords(q->ehci, NLPTR_GET(q->qtdaddr),
> > -               (uint32_t *) &qtd, ehci_qtd_dwords(q->ehci));
> > -    if (!ehci_verify_qh(q, &qh) || !ehci_verify_qtd(p, &qtd)) {
> > +    if (get_dwords(q->ehci, NLPTR_GET(q->qhaddr),
> > +                   (uint32_t *) &qh, ehci_qh_dwords(q->ehci)) < 0 ||
> > +        get_dwords(q->ehci, NLPTR_GET(q->qtdaddr),
> > +                   (uint32_t *) &qtd, ehci_qtd_dwords(q->ehci)) < 0 ||
> > +        !ehci_verify_qh(q, &qh) || !ehci_verify_qtd(p, &qtd)) {
> >          p->async = EHCI_ASYNC_INITIALIZED;
> >          ehci_free_packet(p);
> >          return;
> 
> Looking at the coverity report, my question is whether echi->as can ever
> actually be NULL. This is the address space we use to do DMA, so it feels like
> every EHCI device must set that up somehow. ehci_sysbus_init() does. So does
> usb_ehci_pci_realize().
> usb_ehci_pci_write_config() can change it, but never to NULL.
> 
> If echi->as is always non-NULL then we could change get_dwords() and
> put_dwords() to return "void".
> 
> Alternatively, maybe get_dwords() and put_dwords() should be checking the
> return value from dma_memory_write() and
> dma_memory_read() so that they fail if the DMA fails...
> 

Thanks for the review and the suggestion.

I have sent a v2 that drops the dead NULL check, checks the MemTxResult
of dma_memory_read()/dma_memory_write() instead, and makes put_dwords()
return void since no caller can act on a failed writeback.

https://patchwork.kernel.org/project/qemu-devel/list/?series=1146899 

Thanks,
Jamin

> -- PMM