| Message ID | 1476247497-6976-5-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On 12/10/2016 06:44, David Gibson wrote: > The 'spapr-pci-host-bridge' represents the virtual PCI host bridge (PHB) > for a PAPR guest. Unlike on x86, it's routine on Power (both bare metal > and PAPR guests) to have numerous independent PHBs, each controlling a > separate PCI domain. > > There are two ways of configuring the spapr-pci-host-bridge device: first > it can be done fully manually, specifying the locations and sizes of all > the IO windows. This gives the most control, but is very awkward with 6 > mandatory parameters. Alternatively just an "index" can be specified > which essentially selects from an array of predefined PHB locations. > The PHB at index 0 is automatically created as the default PHB. > > The current set of default locations causes some problems for guests with > large RAM (> 1 TiB) or PCI devices with very large BARs (e.g. big nVidia > GPGPU cards via VFIO). Obviously, for migration we can only change the > locations on a new machine type, however. > > This is awkward, because the placement is currently decided within the > spapr-pci-host-bridge code, so it breaks abstraction to look inside the > machine type version. > > So, this patch delegates the "default mode" PHB placement from the > spapr-pci-host-bridge device back to the machine type via a public method > in sPAPRMachineClass. It's still a bit ugly, but it's about the best we > can do. > > For now, this just changes where the calculation is done. It doesn't > change the actual location of the host bridges, or any other behaviour. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr.c | 34 ++++++++++++++++++++++++++++++++++ > hw/ppc/spapr_pci.c | 22 ++++++++-------------- > include/hw/pci-host/spapr.h | 11 +---------- > include/hw/ppc/spapr.h | 4 ++++ > 4 files changed, 47 insertions(+), 24 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 03e3803..f6e9c2a 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2370,6 +2370,39 @@ static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine) > return head; > } > > +static void spapr_phb_placement(sPAPRMachineState *spapr, uint32_t index, > + uint64_t *buid, hwaddr *pio, hwaddr *pio_size, > + hwaddr *mmio, hwaddr *mmio_size, > + unsigned n_dma, uint32_t *liobns, Error **errp) > +{ > + const uint64_t base_buid = 0x800000020000000ULL; > + const hwaddr phb0_base = 0x10000000000ULL; /* 1 TiB */ > + const hwaddr phb_spacing = 0x1000000000ULL; /* 64 GiB */ > + const hwaddr mmio_offset = 0xa0000000; /* 2 GiB + 512 MiB */ > + const hwaddr pio_offset = 0x80000000; /* 2 GiB */ > + const uint32_t max_index = 255; > + > + hwaddr phb_base; > + int i; > + > + if (index > max_index) { > + error_setg(errp, "\"index\" for PAPR PHB is too large (max %u)", > + max_index); > + return; > + } > + > + *buid = base_buid + index; > + for (i = 0; i < n_dma; ++i) { > + liobns[i] = SPAPR_PCI_LIOBN(index, i); > + } > + > + phb_base = phb0_base + index * phb_spacing; > + *pio = phb_base + pio_offset; > + *pio_size = SPAPR_PCI_IO_WIN_SIZE; > + *mmio = phb_base + mmio_offset; > + *mmio_size = SPAPR_PCI_MMIO_WIN_SIZE; sphb->io_win_size (*pio_size) and sphb->mem_win_size (*mmio_size) were previously initialized from spapr_phb_properties[], you overwrite these values now. Is this what you want? Laurent
On Wed, Oct 12, 2016 at 11:26:05AM +0200, Laurent Vivier wrote: > > > On 12/10/2016 06:44, David Gibson wrote: > > The 'spapr-pci-host-bridge' represents the virtual PCI host bridge (PHB) > > for a PAPR guest. Unlike on x86, it's routine on Power (both bare metal > > and PAPR guests) to have numerous independent PHBs, each controlling a > > separate PCI domain. > > > > There are two ways of configuring the spapr-pci-host-bridge device: first > > it can be done fully manually, specifying the locations and sizes of all > > the IO windows. This gives the most control, but is very awkward with 6 > > mandatory parameters. Alternatively just an "index" can be specified > > which essentially selects from an array of predefined PHB locations. > > The PHB at index 0 is automatically created as the default PHB. > > > > The current set of default locations causes some problems for guests with > > large RAM (> 1 TiB) or PCI devices with very large BARs (e.g. big nVidia > > GPGPU cards via VFIO). Obviously, for migration we can only change the > > locations on a new machine type, however. > > > > This is awkward, because the placement is currently decided within the > > spapr-pci-host-bridge code, so it breaks abstraction to look inside the > > machine type version. > > > > So, this patch delegates the "default mode" PHB placement from the > > spapr-pci-host-bridge device back to the machine type via a public method > > in sPAPRMachineClass. It's still a bit ugly, but it's about the best we > > can do. > > > > For now, this just changes where the calculation is done. It doesn't > > change the actual location of the host bridges, or any other behaviour. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > hw/ppc/spapr.c | 34 ++++++++++++++++++++++++++++++++++ > > hw/ppc/spapr_pci.c | 22 ++++++++-------------- > > include/hw/pci-host/spapr.h | 11 +---------- > > include/hw/ppc/spapr.h | 4 ++++ > > 4 files changed, 47 insertions(+), 24 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 03e3803..f6e9c2a 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -2370,6 +2370,39 @@ static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine) > > return head; > > } > > > > +static void spapr_phb_placement(sPAPRMachineState *spapr, uint32_t index, > > + uint64_t *buid, hwaddr *pio, hwaddr *pio_size, > > + hwaddr *mmio, hwaddr *mmio_size, > > + unsigned n_dma, uint32_t *liobns, Error **errp) > > +{ > > + const uint64_t base_buid = 0x800000020000000ULL; > > + const hwaddr phb0_base = 0x10000000000ULL; /* 1 TiB */ > > + const hwaddr phb_spacing = 0x1000000000ULL; /* 64 GiB */ > > + const hwaddr mmio_offset = 0xa0000000; /* 2 GiB + 512 MiB */ > > + const hwaddr pio_offset = 0x80000000; /* 2 GiB */ > > + const uint32_t max_index = 255; > > + > > + hwaddr phb_base; > > + int i; > > + > > + if (index > max_index) { > > + error_setg(errp, "\"index\" for PAPR PHB is too large (max %u)", > > + max_index); > > + return; > > + } > > + > > + *buid = base_buid + index; > > + for (i = 0; i < n_dma; ++i) { > > + liobns[i] = SPAPR_PCI_LIOBN(index, i); > > + } > > + > > + phb_base = phb0_base + index * phb_spacing; > > + *pio = phb_base + pio_offset; > > + *pio_size = SPAPR_PCI_IO_WIN_SIZE; > > + *mmio = phb_base + mmio_offset; > > + *mmio_size = SPAPR_PCI_MMIO_WIN_SIZE; > > sphb->io_win_size (*pio_size) and sphb->mem_win_size (*mmio_size) were > previously initialized from spapr_phb_properties[], you overwrite these > values now. Is this what you want? Uuuhhhh... I guess not. I put the sizes in here because I wanted to have all the configuration for the common case in one place, rather than split between phb_placement() and the default properties. But you're right, it does introduce a real, if small, behaviour change, which this patch isn't supposed to do. I'll change it.
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 03e3803..f6e9c2a 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2370,6 +2370,39 @@ static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine) return head; } +static void spapr_phb_placement(sPAPRMachineState *spapr, uint32_t index, + uint64_t *buid, hwaddr *pio, hwaddr *pio_size, + hwaddr *mmio, hwaddr *mmio_size, + unsigned n_dma, uint32_t *liobns, Error **errp) +{ + const uint64_t base_buid = 0x800000020000000ULL; + const hwaddr phb0_base = 0x10000000000ULL; /* 1 TiB */ + const hwaddr phb_spacing = 0x1000000000ULL; /* 64 GiB */ + const hwaddr mmio_offset = 0xa0000000; /* 2 GiB + 512 MiB */ + const hwaddr pio_offset = 0x80000000; /* 2 GiB */ + const uint32_t max_index = 255; + + hwaddr phb_base; + int i; + + if (index > max_index) { + error_setg(errp, "\"index\" for PAPR PHB is too large (max %u)", + max_index); + return; + } + + *buid = base_buid + index; + for (i = 0; i < n_dma; ++i) { + liobns[i] = SPAPR_PCI_LIOBN(index, i); + } + + phb_base = phb0_base + index * phb_spacing; + *pio = phb_base + pio_offset; + *pio_size = SPAPR_PCI_IO_WIN_SIZE; + *mmio = phb_base + mmio_offset; + *mmio_size = SPAPR_PCI_MMIO_WIN_SIZE; +} + static void spapr_machine_class_init(ObjectClass *oc, void *data) { MachineClass *mc = MACHINE_CLASS(oc); @@ -2406,6 +2439,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) mc->query_hotpluggable_cpus = spapr_query_hotpluggable_cpus; fwc->get_dev_path = spapr_get_fw_dev_path; nc->nmi_monitor_handler = spapr_nmi; + smc->phb_placement = spapr_phb_placement; } static const TypeInfo spapr_machine_info = { diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 4f00865..c0fc964 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -1311,7 +1311,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1; if (sphb->index != (uint32_t)-1) { - hwaddr windows_base; + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr); + Error *local_err = NULL; if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn[0] != (uint32_t)-1) || (sphb->dma_liobn[1] != (uint32_t)-1 && windows_supported == 2) @@ -1322,21 +1323,14 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) return; } - if (sphb->index > SPAPR_PCI_MAX_INDEX) { - error_setg(errp, "\"index\" for PAPR PHB is too large (max %u)", - SPAPR_PCI_MAX_INDEX); + smc->phb_placement(spapr, sphb->index, + &sphb->buid, &sphb->io_win_addr, &sphb->io_win_size, + &sphb->mem_win_addr, &sphb->mem_win_size, + windows_supported, sphb->dma_liobn, &local_err); + if (local_err) { + error_propagate(errp, local_err); return; } - - sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index; - for (i = 0; i < windows_supported; ++i) { - sphb->dma_liobn[i] = SPAPR_PCI_LIOBN(sphb->index, i); - } - - windows_base = SPAPR_PCI_WINDOW_BASE - + sphb->index * SPAPR_PCI_WINDOW_SPACING; - sphb->mem_win_addr = windows_base + SPAPR_PCI_MMIO_WIN_OFF; - sphb->io_win_addr = windows_base + SPAPR_PCI_IO_WIN_OFF; } if (sphb->buid == (uint64_t)-1) { diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h index 30dbd46..8c9ebfd 100644 --- a/include/hw/pci-host/spapr.h +++ b/include/hw/pci-host/spapr.h @@ -79,18 +79,9 @@ struct sPAPRPHBState { uint32_t numa_node; }; -#define SPAPR_PCI_MAX_INDEX 255 - -#define SPAPR_PCI_BASE_BUID 0x800000020000000ULL - #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL -#define SPAPR_PCI_WINDOW_BASE 0x10000000000ULL -#define SPAPR_PCI_WINDOW_SPACING 0x1000000000ULL -#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_MMIO_WIN_SIZE 0xf80000000 #define SPAPR_PCI_IO_WIN_SIZE 0x10000 #define SPAPR_PCI_MSI_WINDOW 0x40000000000ULL diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index 39dadaa..9e1c5a5 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -40,6 +40,10 @@ struct sPAPRMachineClass { bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */ bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */ const char *tcg_default_cpu; /* which (TCG) CPU to simulate by default */ + void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index, + uint64_t *buid, hwaddr *pio, hwaddr *pio_size, + hwaddr *mmio, hwaddr *mmio_size, + unsigned n_dma, uint32_t *liobns, Error **errp); }; /**
The 'spapr-pci-host-bridge' represents the virtual PCI host bridge (PHB) for a PAPR guest. Unlike on x86, it's routine on Power (both bare metal and PAPR guests) to have numerous independent PHBs, each controlling a separate PCI domain. There are two ways of configuring the spapr-pci-host-bridge device: first it can be done fully manually, specifying the locations and sizes of all the IO windows. This gives the most control, but is very awkward with 6 mandatory parameters. Alternatively just an "index" can be specified which essentially selects from an array of predefined PHB locations. The PHB at index 0 is automatically created as the default PHB. The current set of default locations causes some problems for guests with large RAM (> 1 TiB) or PCI devices with very large BARs (e.g. big nVidia GPGPU cards via VFIO). Obviously, for migration we can only change the locations on a new machine type, however. This is awkward, because the placement is currently decided within the spapr-pci-host-bridge code, so it breaks abstraction to look inside the machine type version. So, this patch delegates the "default mode" PHB placement from the spapr-pci-host-bridge device back to the machine type via a public method in sPAPRMachineClass. It's still a bit ugly, but it's about the best we can do. For now, this just changes where the calculation is done. It doesn't change the actual location of the host bridges, or any other behaviour. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr.c | 34 ++++++++++++++++++++++++++++++++++ hw/ppc/spapr_pci.c | 22 ++++++++-------------- include/hw/pci-host/spapr.h | 11 +---------- include/hw/ppc/spapr.h | 4 ++++ 4 files changed, 47 insertions(+), 24 deletions(-)