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

[Michael Roth]

Quoting David Gibson (2017-03-05 22:16:58)
> 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: address@hidden
> > Cc: Nikunj A Dadhania <address@hidden>
> > Cc: David Gibson <address@hidden>
> > Cc: Alexey Kardashevskiy <address@hidden>
> > Signed-off-by: Michael Roth <address@hidden>
> > ---
> > 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)?

Is it okay for SLOF to assume 64K is sufficient? Since this is a
restriction/quirk on the QEMU/platform side it seems like it should be
controlled via the device tree.

That said, I can't think of any situation where we'd want something other
than 64K...

But even if we bake the 64k alignment into SLOF, shouldn't we still have a
way to switch it off for older machine types so assignments don't change
from one boot to the next? That could be a boolean option/property, but if
that at least is deemed necessary it might as well provide the requested
alignment as well to reduce assumptions on the SLOF side.

If neither of these is an issue it can be fixed within SLOF; just want
to make sure that's okay from a QEMU perspective. In theory guests
shouldn't care how BAR assignments are done, but they also aren't
supposed to care about things like enumeration order either...

> 
> > ---
> >  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("address@hidden" 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