| Message ID | 1437726913-4534-1-git-send-email-lvivier@redhat.com |
|---|---|
| State | New |
| Headers | show |
Alex, could you ACK this patch ? It's not perfect and it will be removed later, but for the moment it allows to hotplug PCI card in pseries. Laurent On 24/07/2015 10:35, Laurent Vivier wrote: > Some kernels program a 0 address for io regions. PCI 3.0 spec > section 6.2.5.1 doesn't seem to disallow this. > > based on patch by Michael Roth <mdroth@linux.vnet.ibm.com> > > Add pci_allow_0_addr in MachineClass to conditionally > allow addr 0 for pseries, as this can break other architectures. > > This patch allows to hotplug PCI card in pseries machine, as the first > added card BAR0 is always set to 0 address. > > This as a temporary hack, waiting to fix PCI memory priorities for more > machine types... > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > v2: move flag from PCIBus to MachineClass, rename it pci_allow_0_address > v3: change author, update commit message. > hw/pci/pci.c | 12 +++++++++--- > hw/ppc/spapr.c | 1 + > include/hw/boards.h | 3 ++- > 3 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index a017614..9f57aea 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -38,6 +38,7 @@ > #include "hw/pci/msix.h" > #include "exec/address-spaces.h" > #include "hw/hotplug.h" > +#include "hw/boards.h" > > //#define DEBUG_PCI > #ifdef DEBUG_PCI > @@ -1065,6 +1066,10 @@ static pcibus_t pci_bar_address(PCIDevice *d, > pcibus_t new_addr, last_addr; > int bar = pci_bar(d, reg); > uint16_t cmd = pci_get_word(d->config + PCI_COMMAND); > + Object *machine = qdev_get_machine(); > + ObjectClass *oc = object_get_class(machine); > + MachineClass *mc = MACHINE_CLASS(oc); > + bool allow_0_address = mc->pci_allow_0_address; > > if (type & PCI_BASE_ADDRESS_SPACE_IO) { > if (!(cmd & PCI_COMMAND_IO)) { > @@ -1075,7 +1080,8 @@ static pcibus_t pci_bar_address(PCIDevice *d, > /* Check if 32 bit BAR wraps around explicitly. > * TODO: make priorities correct and remove this work around. > */ > - if (last_addr <= new_addr || new_addr == 0 || last_addr >= UINT32_MAX) { > + if (last_addr <= new_addr || last_addr >= UINT32_MAX || > + (!allow_0_address && new_addr == 0)) { > return PCI_BAR_UNMAPPED; > } > return new_addr; > @@ -1099,8 +1105,8 @@ static pcibus_t pci_bar_address(PCIDevice *d, > /* XXX: as we cannot support really dynamic > mappings, we handle specific values as invalid > mappings. */ > - if (last_addr <= new_addr || new_addr == 0 || > - last_addr == PCI_BAR_UNMAPPED) { > + if (last_addr <= new_addr || last_addr == PCI_BAR_UNMAPPED || > + (!allow_0_address && new_addr == 0)) { > return PCI_BAR_UNMAPPED; > } > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index a6f1947..bf0c64f 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1835,6 +1835,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) > mc->default_ram_size = 512 * M_BYTE; > mc->kvm_type = spapr_kvm_type; > mc->has_dynamic_sysbus = true; > + mc->pci_allow_0_address = true; > > fwc->get_dev_path = spapr_get_fw_dev_path; > nc->nmi_monitor_handler = spapr_nmi; > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 2aec9cb..3f84afd 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -100,7 +100,8 @@ struct MachineClass { > no_cdrom:1, > no_sdcard:1, > has_dynamic_sysbus:1, > - no_tco:1; > + no_tco:1, > + pci_allow_0_address:1; > int is_default; > const char *default_machine_opts; > const char *default_boot_order; >
Ping ? On 27/07/2015 10:19, Laurent Vivier wrote: > Alex, > > could you ACK this patch ? > > It's not perfect and it will be removed later, but for the moment it > allows to hotplug PCI card in pseries. > > Laurent > > On 24/07/2015 10:35, Laurent Vivier wrote: >> Some kernels program a 0 address for io regions. PCI 3.0 spec >> section 6.2.5.1 doesn't seem to disallow this. >> >> based on patch by Michael Roth <mdroth@linux.vnet.ibm.com> >> >> Add pci_allow_0_addr in MachineClass to conditionally >> allow addr 0 for pseries, as this can break other architectures. >> >> This patch allows to hotplug PCI card in pseries machine, as the first >> added card BAR0 is always set to 0 address. >> >> This as a temporary hack, waiting to fix PCI memory priorities for more >> machine types... >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >> --- >> v2: move flag from PCIBus to MachineClass, rename it pci_allow_0_address >> v3: change author, update commit message. >> hw/pci/pci.c | 12 +++++++++--- >> hw/ppc/spapr.c | 1 + >> include/hw/boards.h | 3 ++- >> 3 files changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index a017614..9f57aea 100644 >> --- a/hw/pci/pci.c >> +++ b/hw/pci/pci.c >> @@ -38,6 +38,7 @@ >> #include "hw/pci/msix.h" >> #include "exec/address-spaces.h" >> #include "hw/hotplug.h" >> +#include "hw/boards.h" >> >> //#define DEBUG_PCI >> #ifdef DEBUG_PCI >> @@ -1065,6 +1066,10 @@ static pcibus_t pci_bar_address(PCIDevice *d, >> pcibus_t new_addr, last_addr; >> int bar = pci_bar(d, reg); >> uint16_t cmd = pci_get_word(d->config + PCI_COMMAND); >> + Object *machine = qdev_get_machine(); >> + ObjectClass *oc = object_get_class(machine); >> + MachineClass *mc = MACHINE_CLASS(oc); >> + bool allow_0_address = mc->pci_allow_0_address; >> >> if (type & PCI_BASE_ADDRESS_SPACE_IO) { >> if (!(cmd & PCI_COMMAND_IO)) { >> @@ -1075,7 +1080,8 @@ static pcibus_t pci_bar_address(PCIDevice *d, >> /* Check if 32 bit BAR wraps around explicitly. >> * TODO: make priorities correct and remove this work around. >> */ >> - if (last_addr <= new_addr || new_addr == 0 || last_addr >= UINT32_MAX) { >> + if (last_addr <= new_addr || last_addr >= UINT32_MAX || >> + (!allow_0_address && new_addr == 0)) { >> return PCI_BAR_UNMAPPED; >> } >> return new_addr; >> @@ -1099,8 +1105,8 @@ static pcibus_t pci_bar_address(PCIDevice *d, >> /* XXX: as we cannot support really dynamic >> mappings, we handle specific values as invalid >> mappings. */ >> - if (last_addr <= new_addr || new_addr == 0 || >> - last_addr == PCI_BAR_UNMAPPED) { >> + if (last_addr <= new_addr || last_addr == PCI_BAR_UNMAPPED || >> + (!allow_0_address && new_addr == 0)) { >> return PCI_BAR_UNMAPPED; >> } >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index a6f1947..bf0c64f 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -1835,6 +1835,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) >> mc->default_ram_size = 512 * M_BYTE; >> mc->kvm_type = spapr_kvm_type; >> mc->has_dynamic_sysbus = true; >> + mc->pci_allow_0_address = true; >> >> fwc->get_dev_path = spapr_get_fw_dev_path; >> nc->nmi_monitor_handler = spapr_nmi; >> diff --git a/include/hw/boards.h b/include/hw/boards.h >> index 2aec9cb..3f84afd 100644 >> --- a/include/hw/boards.h >> +++ b/include/hw/boards.h >> @@ -100,7 +100,8 @@ struct MachineClass { >> no_cdrom:1, >> no_sdcard:1, >> has_dynamic_sysbus:1, >> - no_tco:1; >> + no_tco:1, >> + pci_allow_0_address:1; >> int is_default; >> const char *default_machine_opts; >> const char *default_boot_order; >> >
On 27.07.15 10:19, Laurent Vivier wrote: > Alex, > > could you ACK this patch ? > > It's not perfect and it will be removed later, but for the moment it > allows to hotplug PCI card in pseries. PCIe definitely allows for BARs to be allocated to 0, so this is more or less "the right thing". I really just wanted to make everyone aware that I've run into issues with BARs actually located at address 0 in generic PCI code in Linux ;). So I don't think you really need my ack, as you definitely don't have a nack from my side. Alex
On 11/08/2015 10:50, Alexander Graf wrote: > > > On 27.07.15 10:19, Laurent Vivier wrote: >> Alex, >> >> could you ACK this patch ? >> >> It's not perfect and it will be removed later, but for the moment it >> allows to hotplug PCI card in pseries. > > PCIe definitely allows for BARs to be allocated to 0, so this is more or > less "the right thing". I really just wanted to make everyone aware that > I've run into issues with BARs actually located at address 0 in generic > PCI code in Linux ;). > > So I don't think you really need my ack, as you definitely don't have a > nack from my side. Thank you Alex ! Michael S. T, could you take the patch ? Thanks, Laurent
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index a017614..9f57aea 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -38,6 +38,7 @@ #include "hw/pci/msix.h" #include "exec/address-spaces.h" #include "hw/hotplug.h" +#include "hw/boards.h" //#define DEBUG_PCI #ifdef DEBUG_PCI @@ -1065,6 +1066,10 @@ static pcibus_t pci_bar_address(PCIDevice *d, pcibus_t new_addr, last_addr; int bar = pci_bar(d, reg); uint16_t cmd = pci_get_word(d->config + PCI_COMMAND); + Object *machine = qdev_get_machine(); + ObjectClass *oc = object_get_class(machine); + MachineClass *mc = MACHINE_CLASS(oc); + bool allow_0_address = mc->pci_allow_0_address; if (type & PCI_BASE_ADDRESS_SPACE_IO) { if (!(cmd & PCI_COMMAND_IO)) { @@ -1075,7 +1080,8 @@ static pcibus_t pci_bar_address(PCIDevice *d, /* Check if 32 bit BAR wraps around explicitly. * TODO: make priorities correct and remove this work around. */ - if (last_addr <= new_addr || new_addr == 0 || last_addr >= UINT32_MAX) { + if (last_addr <= new_addr || last_addr >= UINT32_MAX || + (!allow_0_address && new_addr == 0)) { return PCI_BAR_UNMAPPED; } return new_addr; @@ -1099,8 +1105,8 @@ static pcibus_t pci_bar_address(PCIDevice *d, /* XXX: as we cannot support really dynamic mappings, we handle specific values as invalid mappings. */ - if (last_addr <= new_addr || new_addr == 0 || - last_addr == PCI_BAR_UNMAPPED) { + if (last_addr <= new_addr || last_addr == PCI_BAR_UNMAPPED || + (!allow_0_address && new_addr == 0)) { return PCI_BAR_UNMAPPED; } diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index a6f1947..bf0c64f 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1835,6 +1835,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) mc->default_ram_size = 512 * M_BYTE; mc->kvm_type = spapr_kvm_type; mc->has_dynamic_sysbus = true; + mc->pci_allow_0_address = true; fwc->get_dev_path = spapr_get_fw_dev_path; nc->nmi_monitor_handler = spapr_nmi; diff --git a/include/hw/boards.h b/include/hw/boards.h index 2aec9cb..3f84afd 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -100,7 +100,8 @@ struct MachineClass { no_cdrom:1, no_sdcard:1, has_dynamic_sysbus:1, - no_tco:1; + no_tco:1, + pci_allow_0_address:1; int is_default; const char *default_machine_opts; const char *default_boot_order;
Some kernels program a 0 address for io regions. PCI 3.0 spec section 6.2.5.1 doesn't seem to disallow this. based on patch by Michael Roth <mdroth@linux.vnet.ibm.com> Add pci_allow_0_addr in MachineClass to conditionally allow addr 0 for pseries, as this can break other architectures. This patch allows to hotplug PCI card in pseries machine, as the first added card BAR0 is always set to 0 address. This as a temporary hack, waiting to fix PCI memory priorities for more machine types... Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- v2: move flag from PCIBus to MachineClass, rename it pci_allow_0_address v3: change author, update commit message. hw/pci/pci.c | 12 +++++++++--- hw/ppc/spapr.c | 1 + include/hw/boards.h | 3 ++- 3 files changed, 12 insertions(+), 4 deletions(-)