Again. arm has nothing to do with this patch, it shouldn't be in
the summary.
On Thu, Feb 11, 2016 at 09:25:03AM +0100, Alexander Gordeev wrote:
> Unlike x86, ARM and PPC kvm-unit-tests do not have a luxury
> of PCI bus initialized by a BIOS and ready to use at start.
> Thus, we need allocate and assign resources to all devices.
>
> There is no any sort of resource management for memory and
> io spaces, since only one-per-BAR allocations are expected
> between calls to pci_probe() and pci_shutdown(), and no
> deallocations at all.
>
> Cc: Thomas Huth <thuth@xxxxxxxxxx>
> Cc: Andrew Jones <drjones@xxxxxxxxxx>
> Signed-off-by: Alexander Gordeev <agordeev@xxxxxxxxxx>
> ---
> lib/pci-host-generic.c | 171 ++++++++++++++++++++++++++++++++++++++++++++++++-
> lib/pci-host-generic.h | 2 +
> 2 files changed, 172 insertions(+), 1 deletion(-)
>
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> index fd3bb34..167d0db 100644
> --- a/lib/pci-host-generic.c
> +++ b/lib/pci-host-generic.c
> @@ -211,6 +211,138 @@ static struct gen_pci *gen_pci_probe(void)
> return pci;
> }
>
> +static void pci_get_bar_addr_size(void *conf, int bar, u32 *addr, u32 *size)
> +{
> + int off = PCI_BASE_ADDRESS_0 + (bar * 4);
> +
> + *addr = pci_config_readl(conf, off);
> + pci_config_writel(~0, conf, off);
> + *size = pci_config_readl(conf, off);
> + pci_config_writel(*addr, conf, off);
> +}
> +
> +static bool pci_get_bar(void *conf, int bar, pci_res_type_t *type,
> + u64 *addr, u64 *size, bool *is64)
> +{
> + u32 addr_low, size_low;
> + u64 mask;
> +
> + /*
> + * 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.
> + * Use pci_get_bar_addr_size() helper to facilitate that algorithm.
> + */
I think this comment would be better up where the function is defined.
> + pci_get_bar_addr_size(conf, bar, &addr_low, &size_low);
> +
> + if (addr_low & PCI_BASE_ADDRESS_SPACE_IO) {
> + mask = PCI_BASE_ADDRESS_IO_MASK;
> + *type = PCI_RES_TYPE_IO;
> + *is64 = false;
> + } else {
> + mask = PCI_BASE_ADDRESS_MEM_MASK;
> + if ((addr_low & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> + PCI_BASE_ADDRESS_MEM_TYPE_64) {
> + if (addr_low & PCI_BASE_ADDRESS_MEM_PREFETCH)
> + *type = PCI_RES_TYPE_PREFMEM64;
> + else
> + *type = PCI_RES_TYPE_MEM64;
> + *is64 = true;
> + } else {
> + if (addr_low & PCI_BASE_ADDRESS_MEM_PREFETCH)
> + *type = PCI_RES_TYPE_PREFMEM32;
> + else
> + *type = PCI_RES_TYPE_MEM32;
> + *is64 = false;
> + }
> + }
> +
> + if (*is64) {
> + u32 addr_high, size_high;
> + u64 size64;
> +
> + assert(bar < 5);
> + pci_get_bar_addr_size(conf, bar + 1, &addr_high, &size_high);
> +
> + size64 = (~((((u64)size_high << 32) | size_low) & mask)) + 1;
> + if (!size64)
> + return false;
> +
> + if (size)
> + *size = size64;
> + if (addr)
> + *addr = (((u64)addr_high << 32) | addr_low) & mask;
> + } else {
> + u32 size32;
> +
> + size32 = (~(size_low & (u32)mask)) + 1;
> + if (!size32)
> + return false;
> +
> + if (size)
> + *size = size32;
> + if (addr)
> + *addr = addr_low & mask;
> + }
> +
> + return true;
I think this function could use some refactoring - lots of if-elses. Not
conflating the prefetch flag and the memtype would probably help.
> +}
> +
> +static void pci_set_bar(void *conf, int bar, phys_addr_t addr, bool is64)
> +{
> + int off = PCI_BASE_ADDRESS_0 + (bar * 4);
> +
> + pci_config_writel(addr, conf, off);
> + if (is64)
> + pci_config_writel(addr >> 32, conf, off + 4);
> +}
I'd rather we don't introduce small helper functions like this one for
just one use.
> +
> +static struct pci_addr_space *pci_find_res(struct gen_pci *pci,
> + pci_res_type_t type)
> +{
> + struct pci_addr_space *as = &pci->addr_space[0];
> + int i;
> +
> + for (i = 0; i < pci->nr_addr_spaces; i++, as++) {
> + if (flags_to_type(as->of_flags) == type)
> + return as;
> + }
> +
> + return NULL;
> +}
> +
> +static phys_addr_t pci_align_res_size(phys_addr_t size, pci_res_type_t type)
> +{
> + phys_addr_t mask;
> +
> + if (type == PCI_RES_TYPE_IO)
> + mask = PCI_BASE_ADDRESS_IO_MASK;
> + else
> + mask = PCI_BASE_ADDRESS_MEM_MASK;
> +
> + return (size + ~mask) & mask;
> +}
Another unnecessary (IMO) helper function.
> +
> +static phys_addr_t pci_alloc_res(struct gen_pci *pci,
> + pci_res_type_t type, u64 size)
> +{
> + struct pci_addr_space *res;
> + phys_addr_t addr;
> +
> + res = pci_find_res(pci, type);
> + assert(res != NULL);
> +
> + size = pci_align_res_size(size, type);
> + assert(res->alloc + size <= res->pci_range.size);
> +
> + addr = res->pci_range.start + res->alloc;
> + res->alloc += size;
I don't really like the name 'alloc'. It's also the free addresses
base. So how about 'free', or 'base'?
> +
> + return addr;
> +}
> +
> static void pci_dev_print(void *conf)
> {
> u16 vendor_id = pci_config_readw(conf, PCI_VENDOR_ID);
> @@ -226,9 +358,29 @@ static void pci_dev_print(void *conf)
> progif, class, subcl);
> }
>
> +static void pci_dev_bar_print(int bar, pci_res_type_t type,
> + phys_addr_t addr, u64 size, bool is64)
> +{
> + const char *desc = addr_space_desc[type];
> +
> + if (is64) {
> + printf("\tBAR#%d,%d [%-7s %02x-%02x]\n",
> + bar, bar + 1, desc, addr, addr + size - 1);
> + } else {
> + printf("\tBAR#%d [%-7s %02x-%02x]\n",
> + bar, desc, addr, addr + size - 1);
> + }
> +}
Another useful print routine that should be made public and put in
lib/pci.c
> +
> static int pci_bus_scan(struct gen_pci *pci)
> {
> int dev;
> + phys_addr_t addr;
> + u64 size;
> + pci_res_type_t type;
> + u32 cmd = PCI_COMMAND_SERR;
> + bool is64;
> + int bar;
> int nr_dev = 0;
>
> if (!pci)
> @@ -243,7 +395,24 @@ static int pci_bus_scan(struct gen_pci *pci)
> PCI_HEADER_TYPE_NORMAL)
> continue;
>
> - pci_config_writel(PCI_COMMAND_SERR, conf, PCI_COMMAND);
> + for (bar = 0; bar < PCI_HEADER_TYPE_NORMAL_NUM_BARS; bar++) {
Do we really need a define (with a long name) for normal-num-bars?
> + if (!pci_get_bar(conf, bar, &type, NULL, &size, &is64))
> + break;
> +
> + addr = pci_alloc_res(pci, type, size);
> + pci_set_bar(conf, bar, addr, is64);
> + pci_dev_bar_print(bar, type, addr, size, is64);
Don't print on each unittest boot.
> +
> + if (is64)
> + bar++;
> +
> + if (type == PCI_RES_TYPE_IO)
> + cmd |= PCI_COMMAND_IO;
> + else
> + cmd |= PCI_COMMAND_MEMORY;
Hmm... you're setting cmd here, but then you loop again before you use
it, so it may get changed. I think you should have the writel's directly
in here.
> + }
> +
> + pci_config_writel(cmd, conf, PCI_COMMAND);
I'm not sure what you want to do here. Do we really need the
PCI_COMMAND_SERR at all? You weren't doing it for non-normal,
and now it's not happening for normal either.
> nr_dev++;
> }
>
> diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
> index 1d86b43..0f69d71 100644
> --- a/lib/pci-host-generic.h
> +++ b/lib/pci-host-generic.h
> @@ -17,6 +17,7 @@ struct pci_addr_range {
> struct pci_addr_space {
> struct pci_addr_range cpu_range;
> struct pci_addr_range pci_range;
> + phys_addr_t alloc;
> u32 of_flags;
> };
>
> @@ -71,6 +72,7 @@ typedef enum pci_res_type {
> #define PCI_ECAM_BUS_SIZE (1 << 20)
> #define PCI_NUM_DEVICES (PCI_ECAM_BUS_SIZE / (1 << 15))
> #define PCI_ECAM_CONFIG_SIZE (1 << 12)
> +#define PCI_HEADER_TYPE_NORMAL_NUM_BARS 6 /* # of BARs in PCI function */
>
> #define for_each_pci_dev(pci, dev) \
> for (dev = find_next_dev(pci, -1); \
> --
> 1.8.3.1
Thanks,
drew
--
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
