On Tue, Nov 29, 2016 at 03:48:52PM +0100, Alexander Gordeev wrote:
> Cc: Thomas Huth <thuth@xxxxxxxxxx>
> Cc: Andrew Jones <drjones@xxxxxxxxxx>
> Signed-off-by: Alexander Gordeev <agordeev@xxxxxxxxxx>
> ---
> lib/pci.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/lib/pci.c b/lib/pci.c
> index fdd88296f0ae..953810d14334 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -35,6 +35,7 @@ uint32_t pci_bar_mask(uint32_t bar)
>
> uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
> {
> + assert(bar_num >= 0 && bar_num < 6);
> return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> }
>
> @@ -56,11 +57,12 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
>
> void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)
> {
> + bool is64 = pci_bar_is64(dev, bar_num);
> int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
>
> pci_config_writel(dev, off, (uint32_t)addr);
>
> - if (pci_bar_is64(dev, bar_num))
> + if (is64)
> pci_config_writel(dev, off + 4, (uint32_t)(addr >> 32));
> }
>
> @@ -76,9 +78,12 @@ void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)
> */
> static uint32_t pci_bar_size_helper(pcidevaddr_t dev, int bar_num)
> {
> - int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
> + int off;
> uint32_t bar, val;
>
> + assert(bar_num >= 0 && bar_num < 6);
> +
> + off = PCI_BASE_ADDRESS_0 + bar_num * 4;
No need to move the assignment of off down.
> bar = pci_config_readl(dev, off);
> pci_config_writel(dev, off, ~0u);
> val = pci_config_readl(dev, off);
> @@ -127,8 +132,13 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num)
> if (bar & PCI_BASE_ADDRESS_SPACE_IO)
> return false;
>
> - return (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> - PCI_BASE_ADDRESS_MEM_TYPE_64;
> + if ((bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> + PCI_BASE_ADDRESS_MEM_TYPE_64) {
> + assert(bar_num < 5);
> + return true;
> + }
> +
> + return false;
> }
>
> void pci_bar_print(pcidevaddr_t dev, int bar_num)
> --
> 1.8.3.1
>
I think I'd rather leave pci_bar_is64() alone and instead teach
pci_bar_is_valid() more about validating bars. Then scatter more
calls to that around. pci_bar_is_valid doesn't look right to me
anyway. It currently just says non-zero valid, zero not valid. What
if you're querying the second bar of a 64-bit bar? Or what if the
PCI base address is supposed to be zero, like it is for the PIO
region?
How about the following instead?
bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
{
assert(bar_num >= 0 && bar_num < 6);
assert(!(bar_num == 5 && pci_bar_is64(dev, bar_num)));
if (bar_num > 0 && pci_bar_is64(dev, bar_num - 1)) {
assert(!pci_bar_is64(dev, bar_num));
return pci_bar_is_valid(dev, bar_num - 1);
}
return pci_bar_size(dev, bar_num);
}
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
