Re: [Qemu-devel] [PATCH v2] spapr_pci: allow control of BAR alignment through SLOF

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

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

On Fri, Mar 03, 2017 at 05:32:57PM -0600, Michael Roth wrote:
> In certain cases, such as PCI-passthrough with VFIO, we cannot offload
> MMIO accesses to KVM unless the BAR alignment matches the host. This
> patch, in conjunction with a separately submitted patch for SLOF
> which allows for control of this via the device-tree, allows us to
> set this alignment via QEMU.
> 
> Cc: qemu-ppc@nongnu.org
> Cc: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> v2:
>   * Keep natural alignment as the default in SLOF, only set DT prop if
>     different alignment is specified by QEMU (David)

Unfortunately, this has missed the cutoff for qemu-2.9.  

I'm afraid I'm still not entirely clear on the rationale here.

Why do we need a variable parameter based on the host page size?
Couldn't we just have SLOF always align to something "big enough"
(64kiB, IIUC)?

> ---
>  hw/ppc/spapr.c              |  7 ++++++-
>  hw/ppc/spapr_pci.c          | 10 ++++++++++
>  include/hw/pci-host/spapr.h |  1 +
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 14192ac..ef8df35 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3166,7 +3166,12 @@ DEFINE_SPAPR_MACHINE(2_9, "2.9", true);
>   * pseries-2.8
>   */
>  #define SPAPR_COMPAT_2_8                            \
> -    HW_COMPAT_2_8
> +    HW_COMPAT_2_8                                   \
> +    {                                               \
> +        .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,     \
> +        .property = "mem_bar_min_align",            \
> +        .value    = "0",                            \
> +    },                                              \
>  
>  static void spapr_machine_2_8_instance_options(MachineState *machine)
>  {
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 919d3c2..485d32d 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1664,6 +1664,10 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    if (sphb->mem_bar_min_align == (uint64_t)-1) {
> +        sphb->mem_bar_min_align = qemu_real_host_page_size;
> +    }
> +
>      sphb->dtbusname = g_strdup_printf("pci@%" PRIx64, sphb->buid);
>  
>      namebuf = alloca(strlen(sphb->dtbusname) + 32);
> @@ -1858,6 +1862,8 @@ static Property spapr_phb_properties[] = {
>      DEFINE_PROP_UINT32("numa_node", sPAPRPHBState, numa_node, -1),
>      DEFINE_PROP_BOOL("pre-2.8-migration", sPAPRPHBState,
>                       pre_2_8_migration, false),
> +    DEFINE_PROP_UINT64("mem_bar_min_align", sPAPRPHBState, mem_bar_min_align,
> +                       -1),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -2228,6 +2234,10 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>      if (ret) {
>          return ret;
>      }
> +    if (phb->mem_bar_min_align) {
> +        _FDT(fdt_setprop_cell(fdt, bus_off, "qemu,mem-bar-min-align",
> +                              phb->mem_bar_min_align));
> +    }
>  
>      return 0;
>  }
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index dfa7614..fa33346 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -79,6 +79,7 @@ struct sPAPRPHBState {
>      uint64_t dma64_win_addr;
>  
>      uint32_t numa_node;
> +    uint64_t mem_bar_min_align;
>  
>      /* Fields for migration compatibility hacks */
>      bool pre_2_8_migration;

-- 
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 --]