Re: [Qemu-devel] [RFC PATCH 07/10] spapr_pci: Allow PCI host bridge DMA window to be configured

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3774 bytes --]

On Wed, Sep 23, 2015 at 01:08:34PM +0200, Thomas Huth wrote:
> On 17/09/15 15:09, David Gibson wrote:
> > At present the PCI host bridge (PHB) for the pseries machine type has a
> > fixed DMA window from 0..1GB (in PCI address space) which is mapped to real
> > memory via the PAPR paravirtualized IOMMU.
> > 
> > For better support of VFIO devices, we're going to want to allow for
> > different configurations of the DMA window.
> > 
> > Eventually we'll want to allow the guest itself to reconfigure the window
> > via the PAPR dynamic DMA window interface, but as a preliminary this patch
> > allows the user to reconfigure the window with new properties on the PHB
> > device.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_pci.c          | 7 +++++--
> >  include/hw/pci-host/spapr.h | 3 +--
> >  2 files changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index b088491..622c4ac 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1394,7 +1394,7 @@ static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
> >      sPAPRTCETable *tcet;
> >      uint32_t nb_table;
> >  
> > -    nb_table = SPAPR_PCI_DMA32_SIZE >> SPAPR_TCE_PAGE_SHIFT;
> > +    nb_table = sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT;
> >      tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn,
> >                                 0, SPAPR_TCE_PAGE_SHIFT, nb_table, false);
> >      if (!tcet) {
> > @@ -1404,7 +1404,7 @@ static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
> >      }
> >  
> >      /* Register default 32bit DMA window */
> > -    memory_region_add_subregion(&sphb->iommu_root, 0,
> > +    memory_region_add_subregion(&sphb->iommu_root, sphb->dma_win_addr,
> >                                  spapr_tce_get_iommu(tcet));
> >  }
> >  
> > @@ -1437,6 +1437,9 @@ static Property spapr_phb_properties[] = {
> >                         SPAPR_PCI_IO_WIN_SIZE),
> >      DEFINE_PROP_BOOL("dynamic-reconfiguration", sPAPRPHBState, dr_enabled,
> >                       true),
> > +    /* Default DMA window is 0..1GB */
> > +    DEFINE_PROP_UINT64("dma_win_addr", sPAPRPHBState, dma_win_addr, 0),
> > +    DEFINE_PROP_UINT64("dma_win_size", sPAPRPHBState, dma_win_size, 0x40000000),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> > index 5322b56..7de5e02 100644
> > --- a/include/hw/pci-host/spapr.h
> > +++ b/include/hw/pci-host/spapr.h
> > @@ -78,6 +78,7 @@ struct sPAPRPHBState {
> >      MemoryRegion memwindow, iowindow, msiwindow;
> >  
> >      uint32_t dma_liobn;
> > +    hwaddr dma_win_addr, dma_win_size;
> 
> Maybe use dma_addr_t for dma_win_addr? And dma_win_size isn't an
> address, so I'd maybe use uint64_t here instead.

So dma_win_addr is being passed to memory_region_add_subregion() which
takes a hwaddr, so I think hwaddr is correct.

> >      AddressSpace iommu_as;
> >      MemoryRegion iommu_root;
> >  
> > @@ -115,8 +116,6 @@ struct sPAPRPHBVFIOState {
> >  
> >  #define SPAPR_PCI_MSI_WINDOW         0x40000000000ULL
> >  
> > -#define SPAPR_PCI_DMA32_SIZE         0x40000000
> > -
> >  static inline qemu_irq spapr_phb_lsi_qirq(struct sPAPRPHBState *phb, int pin)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> 
> Anyway, patch looks fine to me, so:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]