From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55695) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZILGm-0005QG-76 for qemu-devel@nongnu.org; Thu, 23 Jul 2015 14:31:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZILGi-0005Rn-1m for qemu-devel@nongnu.org; Thu, 23 Jul 2015 14:31:00 -0400 Date: Thu, 23 Jul 2015 21:30:51 +0300 From: "Michael S. Tsirkin" Message-ID: <20150723212731-mutt-send-email-mst@redhat.com> References: <1437566099-10004-1-git-send-email-lvivier@redhat.com> <1437675858-14070-1-git-send-email-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1437675858-14070-1-git-send-email-lvivier@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier Cc: Michael Roth , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Alexander Graf , David Gibson On Thu, Jul 23, 2015 at 08:24:18PM +0200, Laurent Vivier wrote: > From: Michael Roth I think at this point it's your patches, I'd write "based on patch by Michael Roth" instead. > Some kernels program a 0 address for io regions. PCI 3.0 spec > section 6.2.5.1 doesn't seem to disallow this. > > Signed-off-by: Michael Roth > [lvivier: add pci_allow_0_addr in MachineClass to conditionally > allow addr 0 for pseries, as this can break other architectures] Please spell the detailed explanation out in the commit log, not here. > Signed-off-by: Laurent Vivier I don't know whether this is the right thing to do for spapr, for pci.c I'm fine with this as a temporary hack: Acked-by: Michael S. Tsirkin but please look (in 2.5 timeframe) at fixing priorities for more machine types, esp the PC. > --- > v2: move flag from PCIBus to MachineClass, rename it pci_allow_0_address > 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; > -- > 2.1.0