Re: [Qemu-devel] [PATCH v3] spapr-pci: Enable huge BARs

[Alexey Kardashevskiy <aik@ozlabs.ru>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/14/2015 03:51 PM, David Gibson wrote:
> On Fri, Jan 09, 2015 at 12:11:14PM +1100, Alexey Kardashevskiy wrote:
>> At the moment sPAPR only supports 512MB window for MMIO BARs.
>> However modern devices might want bigger 64bit BARs.
>> 
>> This extends MMIO window from 512MB to 62GB (aligned to 
>> SPAPR_PCI_WINDOW_SPACING) and advertises it in 2 records in the PHB
>> "ranges" property. 32bit gets the space from 
>> SPAPR_PCI_MEM_WIN_BUS_OFFSET till the end of 4GB, 64bit gets the
>> rest of the space.
>> 
>> The approach changes the device tree which is a guest visible
>> change, however it won't break migration as: 1. we do not support
>> migration to older QEMU versions 2. migration to newer QEMU will
>> migrate the device tree as well and since the new layout only
>> extends the old one and does not change address mappigns, no
>> breakage is expected here too.
>> 
>> SLOF change is required to utilize this extension.
>> 
>> Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> 
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- Changes: 
>> v3: * used SPAPR_PCI_WINDOW_SPACING in SPAPR_PCI_MMIO_WIN_SIZE *
>> extended commit log
>> 
>> v2: * do not change existing memory layout * do not create another
>> mmio window --- hw/ppc/spapr_pci.c          | 8 +++++++- 
>> include/hw/pci-host/spapr.h | 7 ++++--- 2 files changed, 11
>> insertions(+), 4 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index
>> c0d8c1c..7b73106 100644 --- a/hw/ppc/spapr_pci.c +++
>> b/hw/ppc/spapr_pci.c @@ -862,6 +862,7 @@ int
>> spapr_populate_pci_dt(sPAPRPHBState *phb, int bus_off, i, j; char
>> nodename[256]; uint32_t bus_range[] = { cpu_to_be32(0),
>> cpu_to_be32(0xff) }; +    const uint64_t win32size = (1ULL << 32) -
>> SPAPR_PCI_MEM_WIN_BUS_OFFSET; struct { uint32_t hi; uint64_t child; 
>> @@ -876,7 +877,12 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb, 
>> { cpu_to_be32(b_ss(2)), cpu_to_be64(SPAPR_PCI_MEM_WIN_BUS_OFFSET), 
>> cpu_to_be64(phb->mem_win_addr), -
>> cpu_to_be64(memory_region_size(&phb->memwindow)), +
>> cpu_to_be64(win32size), +        }, +        { +
>> cpu_to_be32(b_ss(3)), cpu_to_be64(1ULL << 32), +
>> cpu_to_be64(phb->mem_win_addr + win32size), +
>> cpu_to_be64(memory_region_size(&phb->memwindow) - win32size) },
> 
> I think this needs to be a little fancier.  It will work in the case 
> of the normal PHB configuration.  But the user can override the size 
> of the memory window size, in which case you may need to omit the 
> second range, or even limit the size of the 32-bit range.


Ah, right, the "memory_region_size(&phb->memwindow) > win32size" check is
missing here. Will fix, thanks.


>> }; uint64_t bus_reg[] = { cpu_to_be64(phb->buid), 0 }; diff --git
>> a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h index
>> 4ea2a0d..92695b6 100644 --- a/include/hw/pci-host/spapr.h +++
>> b/include/hw/pci-host/spapr.h @@ -96,17 +96,18 @@ struct
>> sPAPRPHBVFIOState {
>> 
>> #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      0x20000000 +#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
>> 
>> #define SPAPR_PCI_MSI_WINDOW         0x40000000000ULL
>> 
>> -#define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL - static inline
>> qemu_irq spapr_phb_lsi_qirq(struct sPAPRPHBState *phb, int pin) { 
>> return xics_get_qirq(spapr->icp, phb->lsi_table[pin].irq);
> 
> I think you also should disable the extended window for older machine 
> revisions.  It would probably work anyway, but I think it's going to 
> be safer for backwards compat if you keep the windows the same for a 
> given machine revision.

Hos is it safer? What can possibly break it? I understand the intention
but in this particular case I cannot see any possible problems, can you?


- -- 
Alexey
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJUuF5hAAoJEIYTPdgrwSC5pPIP/iBfFOzM7NKC09Jir1TxeODg
dEAI4tBEWeF6yuoE5u6sN74SoT9kX3/FaUQoWAIT+6W4HVj7Sbd65J7O4yK2XTEa
XGsU3UN4qqp7rYP5OClHqXAiltN5saHe4MmPNLW5zqTl7bLkW1ExIZ18VdTrsZDI
5DNXqkhNdawTtQtu9URe1x5YmiHiaD1tKEn8v9yAXZwGMbjb+HWhkWXRuGER1WdO
V/+AuNTOsNGh+3PJE8WScOw/I1bHkCX4EQmevXVXcNQ1MLa5uI3h0cpCChDhppvV
Qj3CLk+PA8Ik3sq2uGEBnVI20jhRFb2o9LzO504EtEJMuqTbNbJrVX80WrgnzyWD
tqsE2tx6Pj/znhNRioHKBXqBYDcYaZ67Ut42PXbLOGm0Bha/1BrZEq7N0U+17xcT
diMsGcgMTEfBSfR+T3hCFOZVEBNYzvxeMfdTTIu2cysf6XYV3qMK7EopM4xMF1s6
dAUY6/TzTceOfBC6+6nfd3VWmxvC0CCd/upuV9x/a/0It6WOiYzwkMM4Tnu9AME+
gOEnY6GOwe1qw8/EQmUZcDqY0zC41KgTeuUyKk65juhmJjJuBUhn9p1YWfQ4xniD
Ra2CxoOkuO2BS3tAPxIdU0zPjm+NHOrwAOV5qC0MEmDzS2iixGEoG5ytbE1qNcg1
pwdxlDx2jOMbEfAny72s
=juwo
-----END PGP SIGNATURE-----