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

[David Gibson]

On Sun, Mar 05, 2017 at 11:44:54PM -0600, Michael Roth wrote:
> 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...

That was.. also my thinking.

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

Ah.. hmm.. good question.  I mean technically speaking you're not
guaranteed stable BAR allocation from one boot to the next anyway.
But as you mention below that doesn't necessarily mean it won't
confuse guests.

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

Not sure what to do with this.  Well, we've missed qemu 2.9 anyway, so
I guess we've got time to think about it.

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

signature.asc
Description: PGP signature