RE: [PATCH v4 1/7] usb/hcd-xhci: Make dma read/writes hooks pci free

[Sai Pavan Boddu]

Hi Philippe,

> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> Sent: Tuesday, September 1, 2020 10:05 PM
> To: Sai Pavan Boddu <saipava@xilinx.com>; Peter Maydell
> <peter.maydell@linaro.org>; Markus Armbruster <armbru@redhat.com>;
> 'Marc-André Lureau' <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>; Gerd Hoffmann <kraxel@redhat.com>; Edgar
> Iglesias <edgari@xilinx.com>; Francisco Eduardo Iglesias
> <figlesia@xilinx.com>
> Cc: qemu-devel@nongnu.org; Alistair Francis <alistair.francis@wdc.com>;
> Eduardo Habkost <ehabkost@redhat.com>; Ying Fang
> <fangying1@huawei.com>; Vikram Garhwal <fnuv@xilinx.com>; Paul
> Zimmerman <pauldzim@gmail.com>
> Subject: Re: [PATCH v4 1/7] usb/hcd-xhci: Make dma read/writes hooks pci
> free
> 
> On 8/28/20 9:19 PM, Sai Pavan Boddu wrote:
> > This patch starts making the hcd-xhci.c pci free, as part of this
> > restructuring dma read/writes are handled without passing pci object.
> >
> > Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> > ---
> >  hw/usb/hcd-xhci.c | 24 +++++++++++-------------  hw/usb/hcd-xhci.h |
> > 3 +++
> >  2 files changed, 14 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index
> > 67a18fe..2590f7a 100644
> > --- a/hw/usb/hcd-xhci.c
> > +++ b/hw/usb/hcd-xhci.c
> > @@ -495,7 +495,7 @@ static inline void xhci_dma_read_u32s(XHCIState
> > *xhci, dma_addr_t addr,
> >
> >      assert((len % sizeof(uint32_t)) == 0);
> >
> > -    pci_dma_read(PCI_DEVICE(xhci), addr, buf, len);
> > +    dma_memory_read(xhci->as, addr, buf, len);
> >
> >      for (i = 0; i < (len / sizeof(uint32_t)); i++) {
> >          buf[i] = le32_to_cpu(buf[i]); @@ -515,7 +515,7 @@ static
> > inline void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr,
> >      for (i = 0; i < n; i++) {
> >          tmp[i] = cpu_to_le32(buf[i]);
> >      }
> > -    pci_dma_write(PCI_DEVICE(xhci), addr, tmp, len);
> > +    dma_memory_write(xhci->as, addr, tmp, len);
> >  }
> >
> >  static XHCIPort *xhci_lookup_port(XHCIState *xhci, struct USBPort
> > *uport) @@ -644,7 +644,6 @@ static void xhci_die(XHCIState *xhci)
> >
> >  static void xhci_write_event(XHCIState *xhci, XHCIEvent *event, int
> > v)  {
> > -    PCIDevice *pci_dev = PCI_DEVICE(xhci);
> >      XHCIInterrupter *intr = &xhci->intr[v];
> >      XHCITRB ev_trb;
> >      dma_addr_t addr;
> > @@ -663,7 +662,7 @@ static void xhci_write_event(XHCIState *xhci,
> XHCIEvent *event, int v)
> >                                 ev_trb.status, ev_trb.control);
> >
> >      addr = intr->er_start + TRB_SIZE*intr->er_ep_idx;
> > -    pci_dma_write(pci_dev, addr, &ev_trb, TRB_SIZE);
> > +    dma_memory_write(xhci->as, addr, &ev_trb, TRB_SIZE);
> >
> >      intr->er_ep_idx++;
> >      if (intr->er_ep_idx >= intr->er_size) { @@ -720,12 +719,11 @@
> > static void xhci_ring_init(XHCIState *xhci, XHCIRing *ring,  static
> > TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing *ring, XHCITRB *trb,
> >                                 dma_addr_t *addr)  {
> > -    PCIDevice *pci_dev = PCI_DEVICE(xhci);
> >      uint32_t link_cnt = 0;
> >
> >      while (1) {
> >          TRBType type;
> > -        pci_dma_read(pci_dev, ring->dequeue, trb, TRB_SIZE);
> > +        dma_memory_read(xhci->as, ring->dequeue, trb, TRB_SIZE);
> >          trb->addr = ring->dequeue;
> >          trb->ccs = ring->ccs;
> >          le64_to_cpus(&trb->parameter); @@ -762,7 +760,6 @@ static
> > TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing *ring, XHCITRB *trb,
> >
> >  static int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing
> > *ring)  {
> > -    PCIDevice *pci_dev = PCI_DEVICE(xhci);
> >      XHCITRB trb;
> >      int length = 0;
> >      dma_addr_t dequeue = ring->dequeue; @@ -773,7 +770,7 @@ static
> > int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing *ring)
> >
> >      while (1) {
> >          TRBType type;
> > -        pci_dma_read(pci_dev, dequeue, &trb, TRB_SIZE);
> > +        dma_memory_read(xhci->as, dequeue, &trb, TRB_SIZE);
> >          le64_to_cpus(&trb.parameter);
> >          le32_to_cpus(&trb.status);
> >          le32_to_cpus(&trb.control);
> > @@ -828,7 +825,7 @@ static void xhci_er_reset(XHCIState *xhci, int v)
> >          xhci_die(xhci);
> >          return;
> >      }
> > -    pci_dma_read(PCI_DEVICE(xhci), erstba, &seg, sizeof(seg));
> > +    dma_memory_read(xhci->as, erstba, &seg, sizeof(seg));
> >      le32_to_cpus(&seg.addr_low);
> >      le32_to_cpus(&seg.addr_high);
> >      le32_to_cpus(&seg.size);
> > @@ -1440,7 +1437,7 @@ static int xhci_xfer_create_sgl(XHCITransfer
> *xfer, int in_xfer)
> >      int i;
> >
> >      xfer->int_req = false;
> > -    pci_dma_sglist_init(&xfer->sgl, PCI_DEVICE(xhci), xfer->trb_count);
> > +    qemu_sglist_init(&xfer->sgl, DEVICE(xhci), xfer->trb_count,
> > + xhci->as);
> >      for (i = 0; i < xfer->trb_count; i++) {
> >          XHCITRB *trb = &xfer->trbs[i];
> >          dma_addr_t addr;
> > @@ -2101,7 +2098,7 @@ static TRBCCode xhci_address_slot(XHCIState
> *xhci, unsigned int slotid,
> >      assert(slotid >= 1 && slotid <= xhci->numslots);
> >
> >      dcbaap = xhci_addr64(xhci->dcbaap_low, xhci->dcbaap_high);
> > -    poctx = ldq_le_pci_dma(PCI_DEVICE(xhci), dcbaap + 8 * slotid);
> > +    poctx = ldq_le_dma(xhci->as, dcbaap + 8 * slotid);
> >      ictx = xhci_mask64(pictx);
> >      octx = xhci_mask64(poctx);
> >
> > @@ -2439,7 +2436,7 @@ static TRBCCode
> xhci_get_port_bandwidth(XHCIState *xhci, uint64_t pctx)
> >      /* TODO: actually implement real values here */
> >      bw_ctx[0] = 0;
> >      memset(&bw_ctx[1], 80, xhci->numports); /* 80% */
> > -    pci_dma_write(PCI_DEVICE(xhci), ctx, bw_ctx, sizeof(bw_ctx));
> > +    dma_memory_write(xhci->as, ctx, bw_ctx, sizeof(bw_ctx));
> >
> >      return CC_SUCCESS;
> >  }
> > @@ -3431,6 +3428,7 @@ static void usb_xhci_realize(struct PCIDevice
> *dev, Error **errp)
> >      }
> >
> >      usb_xhci_init(xhci);
> > +    xhci->as = pci_get_address_space(dev);
> >      xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > xhci_mfwrap_timer, xhci);
> >
> >      memory_region_init(&xhci->mem, OBJECT(xhci), "xhci", LEN_REGS);
> > @@ -3531,7 +3529,7 @@ static int usb_xhci_post_load(void *opaque, int
> version_id)
> >              continue;
> >          }
> >          slot->ctx =
> > -            xhci_mask64(ldq_le_pci_dma(pci_dev, dcbaap + 8 * slotid));
> > +            xhci_mask64(ldq_le_dma(xhci->as, dcbaap + 8 * slotid));
> >          xhci_dma_read_u32s(xhci, slot->ctx, slot_ctx, sizeof(slot_ctx));
> >          slot->uport = xhci_lookup_uport(xhci, slot_ctx);
> >          if (!slot->uport) {
> > diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h index
> > 946af51..edbd926 100644
> > --- a/hw/usb/hcd-xhci.h
> > +++ b/hw/usb/hcd-xhci.h
> > @@ -22,6 +22,8 @@
> >  #ifndef HW_USB_HCD_XHCI_H
> >  #define HW_USB_HCD_XHCI_H
> >
> > +#include "sysemu/dma.h"
> 
> AddressSpace is forward-declared in "qemu/typedefs.h", so no need to
> include it here (yes in the sources including hcd-xhci.h).
[Sai Pavan Boddu] Yes you are right!, but without this " dma_addr_t " is 
undefined. 
At this point of the patch, hcd-xhci.h is compiled along with pci.h which would 
provide 'dma_addr_t', but when we strip the pci wrapper around hcd-xhci we 
would miss it. Let me know, if its good to add later in the patch series when 
the split happens.

Regards,
Sai Pavan
> 
> With that fixed:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> > +
> >  #define TYPE_XHCI "base-xhci"
> >  #define TYPE_NEC_XHCI "nec-usb-xhci"
> >  #define TYPE_QEMU_XHCI "qemu-xhci"
> > @@ -189,6 +191,7 @@ struct XHCIState {
> >
> >      USBBus bus;
> >      MemoryRegion mem;
> > +    AddressSpace *as;
> >      MemoryRegion mem_cap;
> >      MemoryRegion mem_oper;
> >      MemoryRegion mem_runtime;
> >