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 8, 2020 5:57 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>; David Gibson <david@gibson.dropbear.id.au>
> 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 9/8/20 1:35 PM, Sai Pavan Boddu wrote:
> > Hi Philippe,
> >
> >> -----Original Message-----
> >> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> Sent: Tuesday, September 8, 2020 4: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>; David Gibson <david@gibson.dropbear.id.au>
> >> 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 9/8/20 12:30 PM, Philippe Mathieu-Daudé wrote:
> >>>>> 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(-)
> >>>>>>
> >>> [...]
> >>>>>> --- 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.
> >>>
> >>> OK :( I'd prefer to only include "sysemu/dma.h" in hw/usb/hcd-xhci.c.
> >>>
> >>> Cc'ing David who added dma_addr_t in commit e5332e6334f
> >>> ("iommu: Introduce IOMMU emulation infrastructure"), it might be
> >>> simpler to move its declaration to "qemu/typedefs.h"
> >>> too.
> >>
> >> Btw this type should be poisoned for user-mode.
> > [Sai Pavan Boddu] Thanks, I would try to define them in typedefs.h.
> > BTW I'm not clear about poisoning then in user-mode. Do you mean hide
> these defines for user-mode ?
> 
> This doesn't "hide" but poison them: if you try to use them the compiler (cpp
> actually) will raise an error.
> 
> This was not a direct comment for you, more something to discuss out of the
> scope of your series ;) No need to try to declare it poisoned now.
[Sai Pavan Boddu] One more thing, which came up during implementation is 
dma_memory_read/ dma_memory_write functions which used in this patch would 
require that header.

Regards,
Sai Pavan
> 
> >
> > Regards
> > Sai Pavan
> >>
> >>>
> >>> So no change needed for this patch.
> >>>
> >>>>
> >>>> 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;
> >>>>>>
> >>>>
> >>>
> >