Re: [PATCH v2 3/5] arm/pci: Allocate and assign memory and io space resources

[Andrew Jones <drjones@redhat.com>]

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@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  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