From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Huth Subject: Re: [PATCH RFC 08/15] pci: Rework pci_bar_addr() Date: Wed, 13 Apr 2016 15:28:34 +0200 Message-ID: <570E4982.70501@redhat.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: Andrew Jones To: Alexander Gordeev , kvm@vger.kernel.org Return-path: Received: from mx1.redhat.com ([209.132.183.28]:44849 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758203AbcDMN2j (ORCPT ); Wed, 13 Apr 2016 09:28:39 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D3CB37F097 for ; Wed, 13 Apr 2016 13:28:37 +0000 (UTC) In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On 11.04.2016 13:04, Alexander Gordeev wrote: > This update makes pci_bar_addr() interface 64 bit BARs aware > and introduces a concept of PCI address translation. In cases > PCI bus and CPU bus physical addresses are not indentical the > architecutre should override pci_xslate_addr() interface to > provide necessary mapping between the two. > > Cc: Thomas Huth > Cc: Andrew Jones > Signed-off-by: Alexander Gordeev > --- > lib/asm-generic/pci.h | 8 +++++ > lib/pci.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++----- > lib/pci.h | 4 ++- > 3 files changed, 87 insertions(+), 9 deletions(-) > > diff --git a/lib/asm-generic/pci.h b/lib/asm-generic/pci.h > index 175b0497ed82..15f23079f27e 100644 > --- a/lib/asm-generic/pci.h > +++ b/lib/asm-generic/pci.h > @@ -14,4 +14,12 @@ static inline void pci_shutdown(void) > } > #endif > > +#ifndef pci_xslate_addr > +static inline > +phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) > +{ > + return addr; > +} > +#endif > + > #endif > diff --git a/lib/pci.c b/lib/pci.c > index 88c7d2d06fc2..43e9c0c38434 100644 > --- a/lib/pci.c > +++ b/lib/pci.c > @@ -9,23 +9,78 @@ > /* Scan bus look for a specific device. Only bus 0 scanned for now. */ > pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id) > { > - unsigned dev; > - for (dev = 0; dev < 256; ++dev) { > - uint32_t id = pci_config_readl(dev, 0); > - if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id) { > + pcidevaddr_t dev; > + > + for (dev = 0; dev < 256; dev++) { > + if (pci_config_readw(dev, PCI_VENDOR_ID) == vendor_id && > + pci_config_readw(dev, PCI_DEVICE_ID) == device_id) { > return dev; > } > } > + > return PCIDEVADDR_INVALID; > } > > -unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num) > +static phys_addr_t pci_bar_mask(uint32_t bar) > +{ > + return (bar & PCI_BASE_ADDRESS_SPACE_IO) ? > + PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK; > +} > + > +phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num) > +{ > + int off; > + uint32_t bar; > + uint64_t addr, mask; > + > + off = PCI_BASE_ADDRESS_0 + bar_num * 4; Looks like you also added now support for 64-bit bars below ... but is the above calculation still valid if there is a 64-bit bar inbetween? E.g. what is this function supposed to do if the first bar is a 64-bit bar? Is the caller supposed to use bar_num = 2 for the second bar, or bar_num = 1? ... maybe a comment in front of the function would be good here. > + bar = pci_config_readl(dev, off); > + mask = pci_bar_mask(bar); > + > + if (pci_bar_is64(dev, bar_num)) { > + uint32_t addr_high = pci_config_readl(dev, off + 4); > + addr = (((uint64_t)addr_high << 32) | bar) & mask; > + } else { > + addr = bar & mask; > + } > + > + return pci_xlate_addr(dev, addr); > +} > + > +/* > + * To determine the amount of address space needed by a PCI device, > + * one must save the original value of the BAR, write a value of > + * all 1's to the register, then read it back. The amount of memory > + * can then be then determined by masking the information bits, > + * performing a bitwise NOT and incrementing the value by 1. > + * > + * The following pci_bar_size32() and pci_bar_size() functions do > + * the described algorithm. > + */ > +static uint32_t pci_bar_size32(pcidevaddr_t dev, int bar_num) > +{ > + int off = PCI_BASE_ADDRESS_0 + (bar_num * 4); > + uint32_t bar, size; > + > + bar = pci_config_readl(dev, off); > + pci_config_writel(dev, off, ~0u); > + size = pci_config_readl(dev, off); > + pci_config_writel(dev, off, bar); > + > + return size; > +} > + > +phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num) > { > uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4); > - if (bar & PCI_BASE_ADDRESS_SPACE_IO) { > - return bar & PCI_BASE_ADDRESS_IO_MASK; > + uint32_t size = pci_bar_size32(dev, bar_num); > + phys_addr_t mask = pci_bar_mask(bar); > + > + if (pci_bar_is64(dev, bar_num)) { > + uint32_t size_high = pci_bar_size32(dev, bar_num + 1); > + return (~((((phys_addr_t)size_high << 32) | size) & mask)) + 1; > } else { > - return bar & PCI_BASE_ADDRESS_MEM_MASK; > + return (~((phys_addr_t)size & mask)) + 1; > } > } I think you could slightly simplify that function, something like: phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num) { uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4); phys_addr_t size = pci_bar_size32(dev, bar_num); phys_addr_t mask = pci_bar_mask(bar); if (pci_bar_is64(dev, bar_num)) { size |= ((phys_addr_t)pci_bar_size32(dev, bar_num + 1) << 32; } return (~((phys_addr_t)size & mask)) + 1; } > @@ -40,3 +95,16 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num) > uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4); > return bar; > } > + > +bool pci_bar_is64(pcidevaddr_t dev, int bar_num) > +{ > + uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4); > + > + if (bar & PCI_BASE_ADDRESS_SPACE_IO) > + return false; > + else if ((bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == > + PCI_BASE_ADDRESS_MEM_TYPE_64) > + return true; > + else > + return false; More simple: else return ((bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == PCI_BASE_ADDRESS_MEM_TYPE_64); ? > +} > diff --git a/lib/pci.h b/lib/pci.h > index 90bf62bf3b6d..09b500ea19f0 100644 > --- a/lib/pci.h > +++ b/lib/pci.h > @@ -15,7 +15,9 @@ enum { > PCIDEVADDR_INVALID = 0xffff, > }; > pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id); > -unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num); > +phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num); > +phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num); > +bool pci_bar_is64(pcidevaddr_t dev, int bar_num); > bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num); > bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num); Thomas