diff mbox

[qemu] spapr-pci: Make MMIO spacing a machine property and increase it

Message ID 1456969373-6741-1-git-send-email-aik@ozlabs.ru (mailing list archive)
State New, archived
Headers show

Commit Message

Alexey Kardashevskiy March 3, 2016, 1:42 a.m. UTC
The pseries machine supports multiple PHBs. Each PHB's MMIO/IO space is
mapped to the CPU address space starting at SPAPR_PCI_WINDOW_BASE plus
some offset which is calculated from PHB's index and
SPAPR_PCI_WINDOW_SPACING which is defined now as 64GB.

Since the default 32bit DMA window is using first 2GB of MMIO space,
the amount of MMIO which the PCI devices can actually use is reduced
to 62GB. This is a problem if the user wants to use devices with
huge BARs.

For example, 2 PCI functions of a NVIDIA K80 adapter being passed through
will exceed this limit as they have 16M + 16G + 32M BARs which
(when aligned) will need 64GB.

This converts SPAPR_PCI_WINDOW_BASE and SPAPR_PCI_WINDOW_SPACING to
sPAPRMachineState properties. This uses old values for pseries machine
before 2.6 and increases the spacing to 128GB so MMIO space becomes 126GB.

This changes the default value of sPAPRPHBState::mem_win_size to -1 for
pseries-2.6 and adds setup to spapr_phb_realize.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/ppc/spapr.c              | 43 ++++++++++++++++++++++++++++++++++++++++++-
 hw/ppc/spapr_pci.c          | 14 ++++++++++----
 include/hw/pci-host/spapr.h |  4 +---
 include/hw/ppc/spapr.h      |  1 +
 4 files changed, 54 insertions(+), 8 deletions(-)

Comments

David Gibson March 4, 2016, 3:39 a.m. UTC | #1
On Thu, Mar 03, 2016 at 12:42:53PM +1100, Alexey Kardashevskiy wrote:
> The pseries machine supports multiple PHBs. Each PHB's MMIO/IO space is
> mapped to the CPU address space starting at SPAPR_PCI_WINDOW_BASE plus
> some offset which is calculated from PHB's index and
> SPAPR_PCI_WINDOW_SPACING which is defined now as 64GB.
> 
> Since the default 32bit DMA window is using first 2GB of MMIO space,
> the amount of MMIO which the PCI devices can actually use is reduced
> to 62GB. This is a problem if the user wants to use devices with
> huge BARs.
> 
> For example, 2 PCI functions of a NVIDIA K80 adapter being passed through
> will exceed this limit as they have 16M + 16G + 32M BARs which
> (when aligned) will need 64GB.
> 
> This converts SPAPR_PCI_WINDOW_BASE and SPAPR_PCI_WINDOW_SPACING to
> sPAPRMachineState properties. This uses old values for pseries machine
> before 2.6 and increases the spacing to 128GB so MMIO space becomes 126GB.
> 
> This changes the default value of sPAPRPHBState::mem_win_size to -1 for
> pseries-2.6 and adds setup to spapr_phb_realize.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

So, in theory I dislike the spapr_pci device reaching into the machine
type to get the spacing configuration.  But.. I don't know of a better
way to achieve the desired outcome.

A couple of other details concern me a little more.

> ---
>  hw/ppc/spapr.c              | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  hw/ppc/spapr_pci.c          | 14 ++++++++++----
>  include/hw/pci-host/spapr.h |  4 +---
>  include/hw/ppc/spapr.h      |  1 +
>  4 files changed, 54 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e9d4abf..d21ad8a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -40,6 +40,7 @@
>  #include "migration/migration.h"
>  #include "mmu-hash64.h"
>  #include "qom/cpu.h"
> +#include "qapi/visitor.h"
>  
>  #include "hw/boards.h"
>  #include "hw/ppc/ppc.h"
> @@ -2100,6 +2101,29 @@ static void spapr_set_kvm_type(Object *obj, const char *value, Error **errp)
>      spapr->kvm_type = g_strdup(value);
>  }
>  
> +static void spapr_prop_get_uint64(Object *obj, Visitor *v, const char *name,
> +                                  void *opaque, Error **errp)
> +{
> +    uint64_t value = *(uint64_t *)opaque;
> +    visit_type_uint64(v, name, &value, errp);
> +}
> +
> +static void spapr_prop_set_uint64(Object *obj, Visitor *v, const char *name,
> +                                  void *opaque, Error **errp)
> +{
> +    uint64_t value = -1;
> +    visit_type_uint64(v, name, &value, errp);
> +    *(uint64_t *)opaque = value;
> +}

Pity there aren't standard helpers for this.

> +static void spapr_prop_add_uint64(Object *obj, const char *name,
> +                                  uint64_t *pval, const char *desc)
> +{
> +    object_property_add(obj, name, "uint64", spapr_prop_get_uint64,
> +                        spapr_prop_set_uint64, NULL, pval, NULL);
> +    object_property_set_description(obj, name, desc, NULL);
> +}
> +
>  static void spapr_machine_initfn(Object *obj)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> @@ -2110,6 +2134,10 @@ static void spapr_machine_initfn(Object *obj)
>      object_property_set_description(obj, "kvm-type",
>                                      "Specifies the KVM virtualization mode (HV, PR)",
>                                      NULL);
> +    spapr_prop_add_uint64(obj, "phb-mmio-base", &spapr->phb_mmio_base,
> +                          "Base address for PCI host bridge MMIO");
> +    spapr_prop_add_uint64(obj, "phb-mmio-spacing", &spapr->phb_mmio_spacing,
> +                          "Amount of MMIO space per PCI host bridge");

Hmm.. what happens if someone tries to change these propertis at
runtime with qom-set?  That sounds bad.

>  }
>  
>  static void spapr_machine_finalizefn(Object *obj)
> @@ -2357,6 +2385,10 @@ static const TypeInfo spapr_machine_info = {
>   */
>  static void spapr_machine_2_6_instance_options(MachineState *machine)
>  {
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> +
> +    spapr->phb_mmio_base = SPAPR_PCI_WINDOW_BASE;
> +    spapr->phb_mmio_spacing = SPAPR_PCI_WINDOW_SPACING;
>  }
>  
>  static void spapr_machine_2_6_class_options(MachineClass *mc)
> @@ -2370,10 +2402,19 @@ DEFINE_SPAPR_MACHINE(2_6, "2.6", true);
>   * pseries-2.5
>   */
>  #define SPAPR_COMPAT_2_5 \
> -        HW_COMPAT_2_5
> +        HW_COMPAT_2_5 \
> +        {\
> +            .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
> +            .property = "mem_win_size",\
> +            .value    = "0x1000000000",\
> +        },
>  
>  static void spapr_machine_2_5_instance_options(MachineState *machine)
>  {
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> +
> +    spapr->phb_mmio_base = 0x10000000000ULL;
> +    spapr->phb_mmio_spacing = 0x1000000000ULL;
>  }
>  
>  static void spapr_machine_2_5_class_options(MachineClass *mc)
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index e8edad3..bae01dd 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1260,9 +1260,11 @@ 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);
>  
> -        windows_base = SPAPR_PCI_WINDOW_BASE
> -            + sphb->index * SPAPR_PCI_WINDOW_SPACING;
> +        windows_base = spapr->phb_mmio_base
> +            + sphb->index * spapr->phb_mmio_spacing;
>          sphb->mem_win_addr = windows_base + SPAPR_PCI_MMIO_WIN_OFF;
> +        sphb->mem_win_size = spapr->phb_mmio_spacing -
> +            SPAPR_PCI_MEM_WIN_BUS_OFFSET;
>          sphb->io_win_addr = windows_base + SPAPR_PCI_IO_WIN_OFF;
>      }
>  
> @@ -1281,6 +1283,11 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    if (sphb->mem_win_size == (hwaddr)-1) {
> +        error_setg(errp, "Memory window size not specified for PHB");
> +        return;
> +    }
> +
>      if (sphb->io_win_addr == (hwaddr)-1) {
>          error_setg(errp, "IO window address not specified for PHB");
>          return;
> @@ -1441,8 +1448,7 @@ static Property spapr_phb_properties[] = {
>      DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1),
>      DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn, -1),
>      DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
> -    DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size,
> -                       SPAPR_PCI_MMIO_WIN_SIZE),
> +    DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size, -1),
>      DEFINE_PROP_UINT64("io_win_addr", sPAPRPHBState, io_win_addr, -1),
>      DEFINE_PROP_UINT64("io_win_size", sPAPRPHBState, io_win_size,
>                         SPAPR_PCI_IO_WIN_SIZE),
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 7de5e02..b828c31 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -107,10 +107,8 @@ struct sPAPRPHBVFIOState {
>  #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
>  
>  #define SPAPR_PCI_WINDOW_BASE        0x10000000000ULL
> -#define SPAPR_PCI_WINDOW_SPACING     0x1000000000ULL
> +#define SPAPR_PCI_WINDOW_SPACING     0x2000000000ULL
>  #define SPAPR_PCI_MMIO_WIN_OFF       0xA0000000
> -#define SPAPR_PCI_MMIO_WIN_SIZE      (SPAPR_PCI_WINDOW_SPACING - \
> -                                     SPAPR_PCI_MEM_WIN_BUS_OFFSET)
>  #define SPAPR_PCI_IO_WIN_OFF         0x80000000
>  #define SPAPR_PCI_IO_WIN_SIZE        0x10000
>  
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 098d85d..8b1369e 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -48,6 +48,7 @@ struct sPAPRMachineState {
>  
>      struct VIOsPAPRBus *vio_bus;
>      QLIST_HEAD(, sPAPRPHBState) phbs;
> +    uint64_t phb_mmio_base, phb_mmio_spacing;
>      struct sPAPRNVRAM *nvram;
>      XICSState *icp;
>      DeviceState *rtc;
Alexey Kardashevskiy March 4, 2016, 4:13 a.m. UTC | #2
On 03/04/2016 02:39 PM, David Gibson wrote:
> On Thu, Mar 03, 2016 at 12:42:53PM +1100, Alexey Kardashevskiy wrote:
>> The pseries machine supports multiple PHBs. Each PHB's MMIO/IO space is
>> mapped to the CPU address space starting at SPAPR_PCI_WINDOW_BASE plus
>> some offset which is calculated from PHB's index and
>> SPAPR_PCI_WINDOW_SPACING which is defined now as 64GB.
>>
>> Since the default 32bit DMA window is using first 2GB of MMIO space,
>> the amount of MMIO which the PCI devices can actually use is reduced
>> to 62GB. This is a problem if the user wants to use devices with
>> huge BARs.
>>
>> For example, 2 PCI functions of a NVIDIA K80 adapter being passed through
>> will exceed this limit as they have 16M + 16G + 32M BARs which
>> (when aligned) will need 64GB.
>>
>> This converts SPAPR_PCI_WINDOW_BASE and SPAPR_PCI_WINDOW_SPACING to
>> sPAPRMachineState properties. This uses old values for pseries machine
>> before 2.6 and increases the spacing to 128GB so MMIO space becomes 126GB.
>>
>> This changes the default value of sPAPRPHBState::mem_win_size to -1 for
>> pseries-2.6 and adds setup to spapr_phb_realize.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> So, in theory I dislike the spapr_pci device reaching into the machine
> type to get the spacing configuration.  But.. I don't know of a better
> way to achieve the desired outcome.


We could drop @index and spacing; and request the user to specify the MMIO 
window start (at least) for every additional PHB.


>
> A couple of other details concern me a little more.
>
>> ---
>>   hw/ppc/spapr.c              | 43 ++++++++++++++++++++++++++++++++++++++++++-
>>   hw/ppc/spapr_pci.c          | 14 ++++++++++----
>>   include/hw/pci-host/spapr.h |  4 +---
>>   include/hw/ppc/spapr.h      |  1 +
>>   4 files changed, 54 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index e9d4abf..d21ad8a 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -40,6 +40,7 @@
>>   #include "migration/migration.h"
>>   #include "mmu-hash64.h"
>>   #include "qom/cpu.h"
>> +#include "qapi/visitor.h"
>>
>>   #include "hw/boards.h"
>>   #include "hw/ppc/ppc.h"
>> @@ -2100,6 +2101,29 @@ static void spapr_set_kvm_type(Object *obj, const char *value, Error **errp)
>>       spapr->kvm_type = g_strdup(value);
>>   }
>>
>> +static void spapr_prop_get_uint64(Object *obj, Visitor *v, const char *name,
>> +                                  void *opaque, Error **errp)
>> +{
>> +    uint64_t value = *(uint64_t *)opaque;
>> +    visit_type_uint64(v, name, &value, errp);
>> +}
>> +
>> +static void spapr_prop_set_uint64(Object *obj, Visitor *v, const char *name,
>> +                                  void *opaque, Error **errp)
>> +{
>> +    uint64_t value = -1;
>> +    visit_type_uint64(v, name, &value, errp);
>> +    *(uint64_t *)opaque = value;
>> +}
>
> Pity there aren't standard helpers for this.
>
>> +static void spapr_prop_add_uint64(Object *obj, const char *name,
>> +                                  uint64_t *pval, const char *desc)
>> +{
>> +    object_property_add(obj, name, "uint64", spapr_prop_get_uint64,
>> +                        spapr_prop_set_uint64, NULL, pval, NULL);
>> +    object_property_set_description(obj, name, desc, NULL);
>> +}
>> +
>>   static void spapr_machine_initfn(Object *obj)
>>   {
>>       sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
>> @@ -2110,6 +2134,10 @@ static void spapr_machine_initfn(Object *obj)
>>       object_property_set_description(obj, "kvm-type",
>>                                       "Specifies the KVM virtualization mode (HV, PR)",
>>                                       NULL);
>> +    spapr_prop_add_uint64(obj, "phb-mmio-base", &spapr->phb_mmio_base,
>> +                          "Base address for PCI host bridge MMIO");
>> +    spapr_prop_add_uint64(obj, "phb-mmio-spacing", &spapr->phb_mmio_spacing,
>> +                          "Amount of MMIO space per PCI host bridge");
>
> Hmm.. what happens if someone tries to change these propertis at
> runtime with qom-set?  That sounds bad.


What is the problem here exactly? These are the properties for new PHBs, 
if/when we add an ability to hotplug PHBs, changes to these properties will 
reflect in new PHB properties.

Likewise writing to "kvm-type" does not switch from HV to PR and vice versa.


>
>>   }
>>
>>   static void spapr_machine_finalizefn(Object *obj)
>> @@ -2357,6 +2385,10 @@ static const TypeInfo spapr_machine_info = {
>>    */
>>   static void spapr_machine_2_6_instance_options(MachineState *machine)
>>   {
>> +    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
>> +
>> +    spapr->phb_mmio_base = SPAPR_PCI_WINDOW_BASE;
>> +    spapr->phb_mmio_spacing = SPAPR_PCI_WINDOW_SPACING;
>>   }
>>
>>   static void spapr_machine_2_6_class_options(MachineClass *mc)
>> @@ -2370,10 +2402,19 @@ DEFINE_SPAPR_MACHINE(2_6, "2.6", true);
>>    * pseries-2.5
>>    */
>>   #define SPAPR_COMPAT_2_5 \
>> -        HW_COMPAT_2_5
>> +        HW_COMPAT_2_5 \
>> +        {\
>> +            .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
>> +            .property = "mem_win_size",\
>> +            .value    = "0x1000000000",\
>> +        },
>>
>>   static void spapr_machine_2_5_instance_options(MachineState *machine)
>>   {
>> +    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
>> +
>> +    spapr->phb_mmio_base = 0x10000000000ULL;
>> +    spapr->phb_mmio_spacing = 0x1000000000ULL;
>>   }
>>
>>   static void spapr_machine_2_5_class_options(MachineClass *mc)
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index e8edad3..bae01dd 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -1260,9 +1260,11 @@ 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);
>>
>> -        windows_base = SPAPR_PCI_WINDOW_BASE
>> -            + sphb->index * SPAPR_PCI_WINDOW_SPACING;
>> +        windows_base = spapr->phb_mmio_base
>> +            + sphb->index * spapr->phb_mmio_spacing;
>>           sphb->mem_win_addr = windows_base + SPAPR_PCI_MMIO_WIN_OFF;
>> +        sphb->mem_win_size = spapr->phb_mmio_spacing -
>> +            SPAPR_PCI_MEM_WIN_BUS_OFFSET;
>>           sphb->io_win_addr = windows_base + SPAPR_PCI_IO_WIN_OFF;
>>       }
>>
>> @@ -1281,6 +1283,11 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>           return;
>>       }
>>
>> +    if (sphb->mem_win_size == (hwaddr)-1) {
>> +        error_setg(errp, "Memory window size not specified for PHB");
>> +        return;
>> +    }
>> +
>>       if (sphb->io_win_addr == (hwaddr)-1) {
>>           error_setg(errp, "IO window address not specified for PHB");
>>           return;
>> @@ -1441,8 +1448,7 @@ static Property spapr_phb_properties[] = {
>>       DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1),
>>       DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn, -1),
>>       DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
>> -    DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size,
>> -                       SPAPR_PCI_MMIO_WIN_SIZE),
>> +    DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size, -1),
>>       DEFINE_PROP_UINT64("io_win_addr", sPAPRPHBState, io_win_addr, -1),
>>       DEFINE_PROP_UINT64("io_win_size", sPAPRPHBState, io_win_size,
>>                          SPAPR_PCI_IO_WIN_SIZE),
>> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
>> index 7de5e02..b828c31 100644
>> --- a/include/hw/pci-host/spapr.h
>> +++ b/include/hw/pci-host/spapr.h
>> @@ -107,10 +107,8 @@ struct sPAPRPHBVFIOState {
>>   #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
>>
>>   #define SPAPR_PCI_WINDOW_BASE        0x10000000000ULL
>> -#define SPAPR_PCI_WINDOW_SPACING     0x1000000000ULL
>> +#define SPAPR_PCI_WINDOW_SPACING     0x2000000000ULL
>>   #define SPAPR_PCI_MMIO_WIN_OFF       0xA0000000
>> -#define SPAPR_PCI_MMIO_WIN_SIZE      (SPAPR_PCI_WINDOW_SPACING - \
>> -                                     SPAPR_PCI_MEM_WIN_BUS_OFFSET)
>>   #define SPAPR_PCI_IO_WIN_OFF         0x80000000
>>   #define SPAPR_PCI_IO_WIN_SIZE        0x10000
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 098d85d..8b1369e 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -48,6 +48,7 @@ struct sPAPRMachineState {
>>
>>       struct VIOsPAPRBus *vio_bus;
>>       QLIST_HEAD(, sPAPRPHBState) phbs;
>> +    uint64_t phb_mmio_base, phb_mmio_spacing;
>>       struct sPAPRNVRAM *nvram;
>>       XICSState *icp;
>>       DeviceState *rtc;
>
Alexey Kardashevskiy March 7, 2016, 11:50 p.m. UTC | #3
On 03/04/2016 03:13 PM, Alexey Kardashevskiy wrote:
> On 03/04/2016 02:39 PM, David Gibson wrote:
>> On Thu, Mar 03, 2016 at 12:42:53PM +1100, Alexey Kardashevskiy wrote:
>>> The pseries machine supports multiple PHBs. Each PHB's MMIO/IO space is
>>> mapped to the CPU address space starting at SPAPR_PCI_WINDOW_BASE plus
>>> some offset which is calculated from PHB's index and
>>> SPAPR_PCI_WINDOW_SPACING which is defined now as 64GB.
>>>
>>> Since the default 32bit DMA window is using first 2GB of MMIO space,
>>> the amount of MMIO which the PCI devices can actually use is reduced
>>> to 62GB. This is a problem if the user wants to use devices with
>>> huge BARs.
>>>
>>> For example, 2 PCI functions of a NVIDIA K80 adapter being passed through
>>> will exceed this limit as they have 16M + 16G + 32M BARs which
>>> (when aligned) will need 64GB.
>>>
>>> This converts SPAPR_PCI_WINDOW_BASE and SPAPR_PCI_WINDOW_SPACING to
>>> sPAPRMachineState properties. This uses old values for pseries machine
>>> before 2.6 and increases the spacing to 128GB so MMIO space becomes 126GB.
>>>
>>> This changes the default value of sPAPRPHBState::mem_win_size to -1 for
>>> pseries-2.6 and adds setup to spapr_phb_realize.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>
>> So, in theory I dislike the spapr_pci device reaching into the machine
>> type to get the spacing configuration.  But.. I don't know of a better
>> way to achieve the desired outcome.
>
>
> We could drop @index and spacing; and request the user to specify the MMIO
> window start (at least) for every additional PHB.

So what is the decision? :)


>
>
>>
>> A couple of other details concern me a little more.
>>
>>> ---
>>>   hw/ppc/spapr.c              | 43
>>> ++++++++++++++++++++++++++++++++++++++++++-
>>>   hw/ppc/spapr_pci.c          | 14 ++++++++++----
>>>   include/hw/pci-host/spapr.h |  4 +---
>>>   include/hw/ppc/spapr.h      |  1 +
>>>   4 files changed, 54 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index e9d4abf..d21ad8a 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -40,6 +40,7 @@
>>>   #include "migration/migration.h"
>>>   #include "mmu-hash64.h"
>>>   #include "qom/cpu.h"
>>> +#include "qapi/visitor.h"
>>>
>>>   #include "hw/boards.h"
>>>   #include "hw/ppc/ppc.h"
>>> @@ -2100,6 +2101,29 @@ static void spapr_set_kvm_type(Object *obj, const
>>> char *value, Error **errp)
>>>       spapr->kvm_type = g_strdup(value);
>>>   }
>>>
>>> +static void spapr_prop_get_uint64(Object *obj, Visitor *v, const char
>>> *name,
>>> +                                  void *opaque, Error **errp)
>>> +{
>>> +    uint64_t value = *(uint64_t *)opaque;
>>> +    visit_type_uint64(v, name, &value, errp);
>>> +}
>>> +
>>> +static void spapr_prop_set_uint64(Object *obj, Visitor *v, const char
>>> *name,
>>> +                                  void *opaque, Error **errp)
>>> +{
>>> +    uint64_t value = -1;
>>> +    visit_type_uint64(v, name, &value, errp);
>>> +    *(uint64_t *)opaque = value;
>>> +}
>>
>> Pity there aren't standard helpers for this.
>>
>>> +static void spapr_prop_add_uint64(Object *obj, const char *name,
>>> +                                  uint64_t *pval, const char *desc)
>>> +{
>>> +    object_property_add(obj, name, "uint64", spapr_prop_get_uint64,
>>> +                        spapr_prop_set_uint64, NULL, pval, NULL);
>>> +    object_property_set_description(obj, name, desc, NULL);
>>> +}
>>> +
>>>   static void spapr_machine_initfn(Object *obj)
>>>   {
>>>       sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
>>> @@ -2110,6 +2134,10 @@ static void spapr_machine_initfn(Object *obj)
>>>       object_property_set_description(obj, "kvm-type",
>>>                                       "Specifies the KVM virtualization
>>> mode (HV, PR)",
>>>                                       NULL);
>>> +    spapr_prop_add_uint64(obj, "phb-mmio-base", &spapr->phb_mmio_base,
>>> +                          "Base address for PCI host bridge MMIO");
>>> +    spapr_prop_add_uint64(obj, "phb-mmio-spacing",
>>> &spapr->phb_mmio_spacing,
>>> +                          "Amount of MMIO space per PCI host bridge");
>>
>> Hmm.. what happens if someone tries to change these propertis at
>> runtime with qom-set?  That sounds bad.
>
>
> What is the problem here exactly? These are the properties for new PHBs,
> if/when we add an ability to hotplug PHBs, changes to these properties will
> reflect in new PHB properties.
>
> Likewise writing to "kvm-type" does not switch from HV to PR and vice versa.
>
>
>>
>>>   }
>>>
>>>   static void spapr_machine_finalizefn(Object *obj)
>>> @@ -2357,6 +2385,10 @@ static const TypeInfo spapr_machine_info = {
>>>    */
>>>   static void spapr_machine_2_6_instance_options(MachineState *machine)
>>>   {
>>> +    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
>>> +
>>> +    spapr->phb_mmio_base = SPAPR_PCI_WINDOW_BASE;
>>> +    spapr->phb_mmio_spacing = SPAPR_PCI_WINDOW_SPACING;
>>>   }
>>>
>>>   static void spapr_machine_2_6_class_options(MachineClass *mc)
>>> @@ -2370,10 +2402,19 @@ DEFINE_SPAPR_MACHINE(2_6, "2.6", true);
>>>    * pseries-2.5
>>>    */
>>>   #define SPAPR_COMPAT_2_5 \
>>> -        HW_COMPAT_2_5
>>> +        HW_COMPAT_2_5 \
>>> +        {\
>>> +            .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
>>> +            .property = "mem_win_size",\
>>> +            .value    = "0x1000000000",\
>>> +        },
>>>
>>>   static void spapr_machine_2_5_instance_options(MachineState *machine)
>>>   {
>>> +    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
>>> +
>>> +    spapr->phb_mmio_base = 0x10000000000ULL;
>>> +    spapr->phb_mmio_spacing = 0x1000000000ULL;
>>>   }
>>>
>>>   static void spapr_machine_2_5_class_options(MachineClass *mc)
>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>> index e8edad3..bae01dd 100644
>>> --- a/hw/ppc/spapr_pci.c
>>> +++ b/hw/ppc/spapr_pci.c
>>> @@ -1260,9 +1260,11 @@ 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);
>>>
>>> -        windows_base = SPAPR_PCI_WINDOW_BASE
>>> -            + sphb->index * SPAPR_PCI_WINDOW_SPACING;
>>> +        windows_base = spapr->phb_mmio_base
>>> +            + sphb->index * spapr->phb_mmio_spacing;
>>>           sphb->mem_win_addr = windows_base + SPAPR_PCI_MMIO_WIN_OFF;
>>> +        sphb->mem_win_size = spapr->phb_mmio_spacing -
>>> +            SPAPR_PCI_MEM_WIN_BUS_OFFSET;
>>>           sphb->io_win_addr = windows_base + SPAPR_PCI_IO_WIN_OFF;
>>>       }
>>>
>>> @@ -1281,6 +1283,11 @@ static void spapr_phb_realize(DeviceState *dev,
>>> Error **errp)
>>>           return;
>>>       }
>>>
>>> +    if (sphb->mem_win_size == (hwaddr)-1) {
>>> +        error_setg(errp, "Memory window size not specified for PHB");
>>> +        return;
>>> +    }
>>> +
>>>       if (sphb->io_win_addr == (hwaddr)-1) {
>>>           error_setg(errp, "IO window address not specified for PHB");
>>>           return;
>>> @@ -1441,8 +1448,7 @@ static Property spapr_phb_properties[] = {
>>>       DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1),
>>>       DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn, -1),
>>>       DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
>>> -    DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size,
>>> -                       SPAPR_PCI_MMIO_WIN_SIZE),
>>> +    DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size, -1),
>>>       DEFINE_PROP_UINT64("io_win_addr", sPAPRPHBState, io_win_addr, -1),
>>>       DEFINE_PROP_UINT64("io_win_size", sPAPRPHBState, io_win_size,
>>>                          SPAPR_PCI_IO_WIN_SIZE),
>>> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
>>> index 7de5e02..b828c31 100644
>>> --- a/include/hw/pci-host/spapr.h
>>> +++ b/include/hw/pci-host/spapr.h
>>> @@ -107,10 +107,8 @@ struct sPAPRPHBVFIOState {
>>>   #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
>>>
>>>   #define SPAPR_PCI_WINDOW_BASE        0x10000000000ULL
>>> -#define SPAPR_PCI_WINDOW_SPACING     0x1000000000ULL
>>> +#define SPAPR_PCI_WINDOW_SPACING     0x2000000000ULL
>>>   #define SPAPR_PCI_MMIO_WIN_OFF       0xA0000000
>>> -#define SPAPR_PCI_MMIO_WIN_SIZE      (SPAPR_PCI_WINDOW_SPACING - \
>>> -                                     SPAPR_PCI_MEM_WIN_BUS_OFFSET)
>>>   #define SPAPR_PCI_IO_WIN_OFF         0x80000000
>>>   #define SPAPR_PCI_IO_WIN_SIZE        0x10000
>>>
>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>> index 098d85d..8b1369e 100644
>>> --- a/include/hw/ppc/spapr.h
>>> +++ b/include/hw/ppc/spapr.h
>>> @@ -48,6 +48,7 @@ struct sPAPRMachineState {
>>>
>>>       struct VIOsPAPRBus *vio_bus;
>>>       QLIST_HEAD(, sPAPRPHBState) phbs;
>>> +    uint64_t phb_mmio_base, phb_mmio_spacing;
>>>       struct sPAPRNVRAM *nvram;
>>>       XICSState *icp;
>>>       DeviceState *rtc;
>>
>
>
David Gibson March 9, 2016, 1:04 a.m. UTC | #4
On Tue, Mar 08, 2016 at 10:50:51AM +1100, Alexey Kardashevskiy wrote:
> On 03/04/2016 03:13 PM, Alexey Kardashevskiy wrote:
> >On 03/04/2016 02:39 PM, David Gibson wrote:
> >>On Thu, Mar 03, 2016 at 12:42:53PM +1100, Alexey Kardashevskiy wrote:
> >>>The pseries machine supports multiple PHBs. Each PHB's MMIO/IO space is
> >>>mapped to the CPU address space starting at SPAPR_PCI_WINDOW_BASE plus
> >>>some offset which is calculated from PHB's index and
> >>>SPAPR_PCI_WINDOW_SPACING which is defined now as 64GB.
> >>>
> >>>Since the default 32bit DMA window is using first 2GB of MMIO space,
> >>>the amount of MMIO which the PCI devices can actually use is reduced
> >>>to 62GB. This is a problem if the user wants to use devices with
> >>>huge BARs.
> >>>
> >>>For example, 2 PCI functions of a NVIDIA K80 adapter being passed through
> >>>will exceed this limit as they have 16M + 16G + 32M BARs which
> >>>(when aligned) will need 64GB.
> >>>
> >>>This converts SPAPR_PCI_WINDOW_BASE and SPAPR_PCI_WINDOW_SPACING to
> >>>sPAPRMachineState properties. This uses old values for pseries machine
> >>>before 2.6 and increases the spacing to 128GB so MMIO space becomes 126GB.
> >>>
> >>>This changes the default value of sPAPRPHBState::mem_win_size to -1 for
> >>>pseries-2.6 and adds setup to spapr_phb_realize.
> >>>
> >>>Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>
> >>So, in theory I dislike the spapr_pci device reaching into the machine
> >>type to get the spacing configuration.  But.. I don't know of a better
> >>way to achieve the desired outcome.
> >
> >
> >We could drop @index and spacing; and request the user to specify the MMIO
> >window start (at least) for every additional PHB.
> 
> So what is the decision? :)

There isn't one.  I really don't know how to handle this, trying to
talk to some people for ideas.
Alexey Kardashevskiy March 21, 2016, 2:15 a.m. UTC | #5
On 03/09/2016 12:04 PM, David Gibson wrote:
> On Tue, Mar 08, 2016 at 10:50:51AM +1100, Alexey Kardashevskiy wrote:
>> On 03/04/2016 03:13 PM, Alexey Kardashevskiy wrote:
>>> On 03/04/2016 02:39 PM, David Gibson wrote:
>>>> On Thu, Mar 03, 2016 at 12:42:53PM +1100, Alexey Kardashevskiy wrote:
>>>>> The pseries machine supports multiple PHBs. Each PHB's MMIO/IO space is
>>>>> mapped to the CPU address space starting at SPAPR_PCI_WINDOW_BASE plus
>>>>> some offset which is calculated from PHB's index and
>>>>> SPAPR_PCI_WINDOW_SPACING which is defined now as 64GB.
>>>>>
>>>>> Since the default 32bit DMA window is using first 2GB of MMIO space,
>>>>> the amount of MMIO which the PCI devices can actually use is reduced
>>>>> to 62GB. This is a problem if the user wants to use devices with
>>>>> huge BARs.
>>>>>
>>>>> For example, 2 PCI functions of a NVIDIA K80 adapter being passed through
>>>>> will exceed this limit as they have 16M + 16G + 32M BARs which
>>>>> (when aligned) will need 64GB.
>>>>>
>>>>> This converts SPAPR_PCI_WINDOW_BASE and SPAPR_PCI_WINDOW_SPACING to
>>>>> sPAPRMachineState properties. This uses old values for pseries machine
>>>>> before 2.6 and increases the spacing to 128GB so MMIO space becomes 126GB.
>>>>>
>>>>> This changes the default value of sPAPRPHBState::mem_win_size to -1 for
>>>>> pseries-2.6 and adds setup to spapr_phb_realize.
>>>>>
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>
>>>> So, in theory I dislike the spapr_pci device reaching into the machine
>>>> type to get the spacing configuration.  But.. I don't know of a better
>>>> way to achieve the desired outcome.
>>>
>>>
>>> We could drop @index and spacing; and request the user to specify the MMIO
>>> window start (at least) for every additional PHB.
>>
>> So what is the decision? :)
>
> There isn't one.  I really don't know how to handle this, trying to
> talk to some people for ideas.

Got any new idea?
diff mbox

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e9d4abf..d21ad8a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -40,6 +40,7 @@ 
 #include "migration/migration.h"
 #include "mmu-hash64.h"
 #include "qom/cpu.h"
+#include "qapi/visitor.h"
 
 #include "hw/boards.h"
 #include "hw/ppc/ppc.h"
@@ -2100,6 +2101,29 @@  static void spapr_set_kvm_type(Object *obj, const char *value, Error **errp)
     spapr->kvm_type = g_strdup(value);
 }
 
+static void spapr_prop_get_uint64(Object *obj, Visitor *v, const char *name,
+                                  void *opaque, Error **errp)
+{
+    uint64_t value = *(uint64_t *)opaque;
+    visit_type_uint64(v, name, &value, errp);
+}
+
+static void spapr_prop_set_uint64(Object *obj, Visitor *v, const char *name,
+                                  void *opaque, Error **errp)
+{
+    uint64_t value = -1;
+    visit_type_uint64(v, name, &value, errp);
+    *(uint64_t *)opaque = value;
+}
+
+static void spapr_prop_add_uint64(Object *obj, const char *name,
+                                  uint64_t *pval, const char *desc)
+{
+    object_property_add(obj, name, "uint64", spapr_prop_get_uint64,
+                        spapr_prop_set_uint64, NULL, pval, NULL);
+    object_property_set_description(obj, name, desc, NULL);
+}
+
 static void spapr_machine_initfn(Object *obj)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
@@ -2110,6 +2134,10 @@  static void spapr_machine_initfn(Object *obj)
     object_property_set_description(obj, "kvm-type",
                                     "Specifies the KVM virtualization mode (HV, PR)",
                                     NULL);
+    spapr_prop_add_uint64(obj, "phb-mmio-base", &spapr->phb_mmio_base,
+                          "Base address for PCI host bridge MMIO");
+    spapr_prop_add_uint64(obj, "phb-mmio-spacing", &spapr->phb_mmio_spacing,
+                          "Amount of MMIO space per PCI host bridge");
 }
 
 static void spapr_machine_finalizefn(Object *obj)
@@ -2357,6 +2385,10 @@  static const TypeInfo spapr_machine_info = {
  */
 static void spapr_machine_2_6_instance_options(MachineState *machine)
 {
+    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
+
+    spapr->phb_mmio_base = SPAPR_PCI_WINDOW_BASE;
+    spapr->phb_mmio_spacing = SPAPR_PCI_WINDOW_SPACING;
 }
 
 static void spapr_machine_2_6_class_options(MachineClass *mc)
@@ -2370,10 +2402,19 @@  DEFINE_SPAPR_MACHINE(2_6, "2.6", true);
  * pseries-2.5
  */
 #define SPAPR_COMPAT_2_5 \
-        HW_COMPAT_2_5
+        HW_COMPAT_2_5 \
+        {\
+            .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
+            .property = "mem_win_size",\
+            .value    = "0x1000000000",\
+        },
 
 static void spapr_machine_2_5_instance_options(MachineState *machine)
 {
+    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
+
+    spapr->phb_mmio_base = 0x10000000000ULL;
+    spapr->phb_mmio_spacing = 0x1000000000ULL;
 }
 
 static void spapr_machine_2_5_class_options(MachineClass *mc)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index e8edad3..bae01dd 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1260,9 +1260,11 @@  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);
 
-        windows_base = SPAPR_PCI_WINDOW_BASE
-            + sphb->index * SPAPR_PCI_WINDOW_SPACING;
+        windows_base = spapr->phb_mmio_base
+            + sphb->index * spapr->phb_mmio_spacing;
         sphb->mem_win_addr = windows_base + SPAPR_PCI_MMIO_WIN_OFF;
+        sphb->mem_win_size = spapr->phb_mmio_spacing -
+            SPAPR_PCI_MEM_WIN_BUS_OFFSET;
         sphb->io_win_addr = windows_base + SPAPR_PCI_IO_WIN_OFF;
     }
 
@@ -1281,6 +1283,11 @@  static void spapr_phb_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (sphb->mem_win_size == (hwaddr)-1) {
+        error_setg(errp, "Memory window size not specified for PHB");
+        return;
+    }
+
     if (sphb->io_win_addr == (hwaddr)-1) {
         error_setg(errp, "IO window address not specified for PHB");
         return;
@@ -1441,8 +1448,7 @@  static Property spapr_phb_properties[] = {
     DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1),
     DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn, -1),
     DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
-    DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size,
-                       SPAPR_PCI_MMIO_WIN_SIZE),
+    DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size, -1),
     DEFINE_PROP_UINT64("io_win_addr", sPAPRPHBState, io_win_addr, -1),
     DEFINE_PROP_UINT64("io_win_size", sPAPRPHBState, io_win_size,
                        SPAPR_PCI_IO_WIN_SIZE),
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 7de5e02..b828c31 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -107,10 +107,8 @@  struct sPAPRPHBVFIOState {
 #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
 
 #define SPAPR_PCI_WINDOW_BASE        0x10000000000ULL
-#define SPAPR_PCI_WINDOW_SPACING     0x1000000000ULL
+#define SPAPR_PCI_WINDOW_SPACING     0x2000000000ULL
 #define SPAPR_PCI_MMIO_WIN_OFF       0xA0000000
-#define SPAPR_PCI_MMIO_WIN_SIZE      (SPAPR_PCI_WINDOW_SPACING - \
-                                     SPAPR_PCI_MEM_WIN_BUS_OFFSET)
 #define SPAPR_PCI_IO_WIN_OFF         0x80000000
 #define SPAPR_PCI_IO_WIN_SIZE        0x10000
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 098d85d..8b1369e 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -48,6 +48,7 @@  struct sPAPRMachineState {
 
     struct VIOsPAPRBus *vio_bus;
     QLIST_HEAD(, sPAPRPHBState) phbs;
+    uint64_t phb_mmio_base, phb_mmio_spacing;
     struct sPAPRNVRAM *nvram;
     XICSState *icp;
     DeviceState *rtc;