On Wed, Apr 13, 2016 at 07:46:04PM +0200, Alexander Gordeev wrote:
> On Wed, Apr 13, 2016 at 03:28:34PM +0200, Thomas Huth wrote:
> > 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 <thuth@xxxxxxxxxx>
> > > Cc: Andrew Jones <drjones@xxxxxxxxxx>
> > > Signed-off-by: Alexander Gordeev <agordeev@xxxxxxxxxx>
> > > ---
> > > 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.
>
> That is a good point. I tend not to complicate this function which would
> have report a failure in case of "inbetween" access and nailing "bar_num"
> to 32 bits. I do not like this idea much, but struggling with an alternate
> idea. Otherwise it should be something like this:
>
> bool pci_bar_addr(pcidevaddr_t dev, int bar_num, phys_addr_t *addr);
>
> But in early versions of this series Andrew did not like it ;) The code
> iterated through all 6 BARs decoding each one-by-one.
>
>
> > > + 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;
> > }
>
> Yep, looks better.
>
> > > @@ -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);
> >
> > ?
>
> I personally find it less readable, but can not explain it ;)
I prefer Thomas' return the result of the condition. You can also drop
the 'else' :-)
>
> > > +}
> > > 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
> >
> >
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
