Re: [kvm-unit-tests PATCH v4 05/12] pci: Factor out pci_bar_get()

[Andrew Jones <drjones@redhat.com>]

On Mon, Jun 06, 2016 at 02:46:34PM +0200, Alexander Gordeev wrote:
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index 07c911820e20..d092e22b8804 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -27,14 +27,18 @@ static uint32_t pci_bar_mask(uint32_t bar)
>  		PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
>  }
>  
> +static uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
> +{
> +	return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> +}
> +
>  phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
>  {
> -	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
> -	phys_addr_t bar = pci_config_readl(dev, off);
> +	phys_addr_t bar = pci_bar_get(dev, bar_num);
>  	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
>  
>  	if (pci_bar_is64(dev, bar_num))
> -		bar |= (phys_addr_t)pci_config_readl(dev, off + 4) << 32;
> +		bar |= (phys_addr_t)pci_bar_get(dev, bar_num + 1) << 32;
>  
>  	return pci_translate_addr(dev, bar & mask);
>  }
> @@ -64,7 +68,7 @@ static uint32_t pci_bar_size32(pcidevaddr_t dev, int bar_num)
>  
>  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);
> +	uint32_t bar = pci_bar_get(dev, bar_num);
>  	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
>  	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
>  
> @@ -78,19 +82,19 @@ phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
>  
>  bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
>  {
> -	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> +	uint32_t bar = pci_bar_get(dev, bar_num);
>  	return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
>  }
>  
>  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);
> +	uint32_t bar = pci_bar_get(dev, bar_num);
>  	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);
> +	uint32_t bar = pci_bar_get(dev, (bar_num));

Useless () around bar_num here

>  
>  	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
>  		return false;
> -- 
> 1.8.3.1
>

You probably could have introduced this function earlier in the
series, allowing you to immediately use it on new functions, but
it doesn't really matter. 

Besides the nit above,

Reviewed-by: Andrew Jones <drjones@redhat.com>