qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2] spapr_pci: allow control of BAR alignment through SLOF
@ 2017-03-03 23:32 Michael Roth
  2017-03-06  4:16 ` David Gibson
  0 siblings, 1 reply; 5+ messages in thread
From: Michael Roth @ 2017-03-03 23:32 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-ppc, Nikunj A Dadhania, David Gibson, Alexey Kardashevskiy

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)
---
 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;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH v2] spapr_pci: allow control of BAR alignment through SLOF
  2017-03-03 23:32 [Qemu-devel] [PATCH v2] spapr_pci: allow control of BAR alignment through SLOF Michael Roth
@ 2017-03-06  4:16 ` David Gibson
  2017-03-06  5:44   ` Michael Roth
  0 siblings, 1 reply; 5+ messages in thread
From: David Gibson @ 2017-03-06  4:16 UTC (permalink / raw)
  To: Michael Roth
  Cc: qemu-devel, qemu-ppc, Nikunj A Dadhania, Alexey Kardashevskiy

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH v2] spapr_pci: allow control of BAR alignment through SLOF
  2017-03-06  4:16 ` David Gibson
@ 2017-03-06  5:44   ` Michael Roth
  2017-03-06  9:43     ` David Gibson
  0 siblings, 1 reply; 5+ messages in thread
From: Michael Roth @ 2017-03-06  5:44 UTC (permalink / raw)
  To: David Gibson
  Cc: qemu-devel, qemu-ppc, Nikunj A Dadhania, Alexey Kardashevskiy

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

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH v2] spapr_pci: allow control of BAR alignment through SLOF
  2017-03-06  5:44   ` Michael Roth
@ 2017-03-06  9:43     ` David Gibson
  2017-03-07 14:32       ` Michael Roth
  0 siblings, 1 reply; 5+ messages in thread
From: David Gibson @ 2017-03-06  9:43 UTC (permalink / raw)
  To: Michael Roth
  Cc: qemu-devel, qemu-ppc, Nikunj A Dadhania, Alexey Kardashevskiy

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

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: 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)?
> 
> 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("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 --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH v2] spapr_pci: allow control of BAR alignment through SLOF
  2017-03-06  9:43     ` David Gibson
@ 2017-03-07 14:32       ` Michael Roth
  0 siblings, 0 replies; 5+ messages in thread
From: Michael Roth @ 2017-03-07 14:32 UTC (permalink / raw)
  To: David Gibson
  Cc: qemu-devel, qemu-ppc, Nikunj A Dadhania, Alexey Kardashevskiy

Quoting David Gibson (2017-03-06 03:43:49)
> 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: 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)?
> > 
> > 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.

I had a follow-up planned for 2.10 that moves BAR assignment to QEMU entirely,
which would also let us fix the alignment for hotplugged devices. So at this
point it probably makes more sense to focus on that for 2.10. Will get those
posted soon.

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-03-08  7:38 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-03 23:32 [Qemu-devel] [PATCH v2] spapr_pci: allow control of BAR alignment through SLOF Michael Roth
2017-03-06  4:16 ` David Gibson
2017-03-06  5:44   ` Michael Roth
2017-03-06  9:43     ` David Gibson
2017-03-07 14:32       ` Michael Roth

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).