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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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