Re: [Qemu-devel] [PATCH qemu v17 11/12] spapr_pci/spapr_pci_vfio: Support Dynamic DMA Windows (DDW)

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

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

On Wed, Jun 08, 2016 at 04:09:57PM +1000, Alexey Kardashevskiy wrote:
> On 08/06/16 15:57, David Gibson wrote:
> > On Mon, Jun 06, 2016 at 06:12:58PM +1000, Alexey Kardashevskiy wrote:
> >> On 06/06/16 15:57, David Gibson wrote:
> >>> On Wed, Jun 01, 2016 at 06:57:42PM +1000, Alexey Kardashevskiy wrote:
> >>>> This adds support for Dynamic DMA Windows (DDW) option defined by
> >>>> the SPAPR specification which allows to have additional DMA window(s)
> >>>>
> >>>> The "ddw" property is enabled by default on a PHB but for compatibility
> >>>> the pseries-2.5 machine (TODO: update version) and older disable it.
> >>>
> >>> Looks like your todo is now todone, but you need to update the commit
> >>> message.
> >>>
> >>>> This also creates a single DMA window for the older machines to
> >>>> maintain backward migration.
> >>>>
> >>>> This implements DDW for PHB with emulated and VFIO devices. The host
> >>>> kernel support is required. The advertised IOMMU page sizes are 4K and
> >>>> 64K; 16M pages are supported but not advertised by default, in order to
> >>>> enable them, the user has to specify "pgsz" property for PHB and
> >>>> enable huge pages for RAM.
> >>>>
> >>>> The existing linux guests try creating one additional huge DMA window
> >>>> with 64K or 16MB pages and map the entire guest RAM to. If succeeded,
> >>>> the guest switches to dma_direct_ops and never calls TCE hypercalls
> >>>> (H_PUT_TCE,...) again. This enables VFIO devices to use the entire RAM
> >>>> and not waste time on map/unmap later. This adds a "dma64_win_addr"
> >>>> property which is a bus address for the 64bit window and by default
> >>>> set to 0x800.0000.0000.0000 as this is what the modern POWER8 hardware
> >>>> uses and this allows having emulated and VFIO devices on the same bus.
> >>>>
> >>>> This adds 4 RTAS handlers:
> >>>> * ibm,query-pe-dma-window
> >>>> * ibm,create-pe-dma-window
> >>>> * ibm,remove-pe-dma-window
> >>>> * ibm,reset-pe-dma-window
> >>>> These are registered from type_init() callback.
> >>>>
> >>>> These RTAS handlers are implemented in a separate file to avoid polluting
> >>>> spapr_iommu.c with PCI.
> >>>>
> >>>> This changes sPAPRPHBState::dma_liobn to an array to allow 2 LIOBNs.
> >>>>
> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>
> >>> Looks pretty close to ready.
> >>>
> >>> There are a handful of nits and one real error noted below.
> >>>
> >>>> ---
> >>>> Changes:
> >>>> v17:
> >>>> * fixed: "query" did return non-page-shifted value when memory hotplug is enabled
> >>>>
> >>>> v16:
> >>>> * s/dma_liobn/dma_liobn[SPAPR_PCI_DMA_MAX_WINDOWS]/
> >>>> * s/SPAPR_PCI_LIOBN()/dma_liobn[]/
> >>>>
> >>>> v15:
> >>>> * moved page mask filtering to PHB realize(), use "-mempath" to know
> >>>> if there are huge pages
> >>>> * fixed error reporting in RTAS handlers
> >>>> * max window size accounts now hotpluggable memory boundaries
> >>>> ---
> >>>>  hw/ppc/Makefile.objs        |   1 +
> >>>>  hw/ppc/spapr.c              |   5 +
> >>>>  hw/ppc/spapr_pci.c          |  77 +++++++++---
> >>>>  hw/ppc/spapr_rtas_ddw.c     | 293 ++++++++++++++++++++++++++++++++++++++++++++
> >>>>  include/hw/pci-host/spapr.h |   8 +-
> >>>>  include/hw/ppc/spapr.h      |  16 ++-
> >>>>  trace-events                |   4 +
> >>>>  7 files changed, 383 insertions(+), 21 deletions(-)
> >>>>  create mode 100644 hw/ppc/spapr_rtas_ddw.c
> >>>>
> >>>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> >>>> index c1ffc77..986b36f 100644
> >>>> --- a/hw/ppc/Makefile.objs
> >>>> +++ b/hw/ppc/Makefile.objs
> >>>> @@ -7,6 +7,7 @@ obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> >>>>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> >>>>  obj-y += spapr_pci_vfio.o
> >>>>  endif
> >>>> +obj-$(CONFIG_PSERIES) += spapr_rtas_ddw.o
> >>>>  # PowerPC 4xx boards
> >>>>  obj-y += ppc405_boards.o ppc4xx_devs.o ppc405_uc.o ppc440_bamboo.o
> >>>>  obj-y += ppc4xx_pci.o
> >>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>> index 44e401a..6ddcda9 100644
> >>>> --- a/hw/ppc/spapr.c
> >>>> +++ b/hw/ppc/spapr.c
> >>>> @@ -2366,6 +2366,11 @@ DEFINE_SPAPR_MACHINE(2_6, "2.6", true);
> >>>>          .driver   = "spapr-vlan", \
> >>>>          .property = "use-rx-buffer-pools", \
> >>>>          .value    = "off", \
> >>>> +    }, \
> >>>> +    {\
> >>>> +        .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
> >>>> +        .property = "ddw",\
> >>>> +        .value    = stringify(off),\
> >>>>      },
> >>>>  
> >>>>  static void spapr_machine_2_5_instance_options(MachineState *machine)
> >>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> >>>> index 68de523..bcf0360 100644
> >>>> --- a/hw/ppc/spapr_pci.c
> >>>> +++ b/hw/ppc/spapr_pci.c
> >>>> @@ -35,6 +35,7 @@
> >>>>  #include "hw/ppc/spapr.h"
> >>>>  #include "hw/pci-host/spapr.h"
> >>>>  #include "exec/address-spaces.h"
> >>>> +#include "exec/ram_addr.h"
> >>>>  #include <libfdt.h>
> >>>>  #include "trace.h"
> >>>>  #include "qemu/error-report.h"
> >>>> @@ -45,6 +46,7 @@
> >>>>  #include "hw/ppc/spapr_drc.h"
> >>>>  #include "sysemu/device_tree.h"
> >>>>  #include "sysemu/kvm.h"
> >>>> +#include "sysemu/hostmem.h"
> >>>>  
> >>>>  #include "hw/vfio/vfio.h"
> >>>>  
> >>>> @@ -1088,7 +1090,7 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
> >>>>      int fdt_start_offset = 0, fdt_size;
> >>>>  
> >>>>      if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
> >>>> -        sPAPRTCETable *tcet = spapr_tce_find_by_liobn(phb->dma_liobn);
> >>>> +        sPAPRTCETable *tcet = spapr_tce_find_by_liobn(phb->dma_liobn[0]);
> >>>>  
> >>>>          spapr_tce_set_need_vfio(tcet, true);
> >>>>      }
> >>>
> >>> Hang on.. I thought you'd got rid of the need for this explicit
> >>> set_need_vfio() stuff.
> >>
> >>
> >> It is in 12/12 (which I'll split in 2 halves when I respin this), I moved
> >> it to the end as it is not essential for DDW itself.
> > 
> > Yes, sorry, I saw that shortly after writing this.
> > 
> >>>> @@ -1310,11 +1312,14 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> >>>>      PCIBus *bus;
> >>>>      uint64_t msi_window_size = 4096;
> >>>>      sPAPRTCETable *tcet;
> >>>> +    const unsigned windows_supported =
> >>>> +        sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
> >>>>  
> >>>>      if (sphb->index != (uint32_t)-1) {
> >>>>          hwaddr windows_base;
> >>>>  
> >>>> -        if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn != (uint32_t)-1)
> >>>> +        if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn[0] != (uint32_t)-1)
> >>>> +            || ((sphb->dma_liobn[1] != (uint32_t)-1) && (windows_supported > 1))
> >>>>              || (sphb->mem_win_addr != (hwaddr)-1)
> >>>>              || (sphb->io_win_addr != (hwaddr)-1)) {
> >>>>              error_setg(errp, "Either \"index\" or other parameters must"
> >>>> @@ -1329,7 +1334,9 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> >>>>          }
> >>>>  
> >>>>          sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index;
> >>>> -        sphb->dma_liobn = SPAPR_PCI_LIOBN(sphb->index, 0);
> >>>> +        for (i = 0; i < windows_supported; ++i) {
> >>>> +            sphb->dma_liobn[i] = SPAPR_PCI_LIOBN(sphb->index, i);
> >>>> +        }
> >>>>  
> >>>>          windows_base = SPAPR_PCI_WINDOW_BASE
> >>>>              + sphb->index * SPAPR_PCI_WINDOW_SPACING;
> >>>> @@ -1342,8 +1349,9 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> >>>>          return;
> >>>>      }
> >>>>  
> >>>> -    if (sphb->dma_liobn == (uint32_t)-1) {
> >>>> -        error_setg(errp, "LIOBN not specified for PHB");
> >>>> +    if ((sphb->dma_liobn[0] == (uint32_t)-1) ||
> >>>> +        ((sphb->dma_liobn[1] == (uint32_t)-1) && (windows_supported > 1))) {
> >>>> +        error_setg(errp, "LIOBN(s) not specified for PHB");
> >>>>          return;
> >>>>      }
> >>>
> >>> Hrm.. there's a bit of false generality here, since this would break
> >>> if windows_supported > 2, and dma_liobn[2] was not specified.  Not
> >>> urgent for the initial commit though.
> >>
> >>
> >> Is s/windows_supported > 1/windows_supported == 2/ any better here?
> > 
> > Not really.  Unless you also have a windows_supported > 2 case (which
> > could just error / abort / whatever).
> 
> 
> It cannot be >2 as SPAPR_PCI_DMA_MAX_WINDOWS is defined as "2" now and it
> is quite unlikely to change in the future.

Yes, I know, but the value of MAX_WINDOWS isn't obvious if you're just
looking at this code.  My point is that a casual look at this code
suggests it will handle arbitrary windows_supported values, but that's
not actually the case.  This is generally bad practice.

> What do I do right now? :)

Well, as I said it's not urgent for the first commit.  Just leave it
as it is and we'll deal with the consequences later.

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]