Re: [Xen-devel] [PATCH v5 07/11] pci: add support to size ROM BARs to pci_size_mem_bar

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.08.17 at 16:28, <roger.pau@xxxxxxxxxx> wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -594,15 +594,18 @@ static int iommu_remove_device(struct pci_dev *pdev);
>  
>  int pci_size_mem_bar(unsigned int seg, unsigned int bus, unsigned int slot,
>                       unsigned int func, unsigned int pos, bool last,
> -                     uint64_t *paddr, uint64_t *psize, bool vf)
> +                     uint64_t *paddr, uint64_t *psize, bool vf, bool rom)
>  {
>      uint32_t hi = 0, bar = pci_conf_read32(seg, bus, slot, func, pos);
>      uint64_t addr, size;
> +    bool is64bits = !rom && (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> +                    PCI_BASE_ADDRESS_MEM_TYPE_64;
>  
> -    ASSERT((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY);
> +    ASSERT(!(rom && vf));

Things like this as well as there now being three bools among the
function parameters is imo a good indication that you want an
"unsigned int flags" parameter instead. That'll also help seeing
what the true-s and false-s are representing at the call sites. And
perhaps that would then better already be done in the patch
adding "vf".

> @@ -616,9 +619,8 @@ int pci_size_mem_bar(unsigned int seg, unsigned int bus, 
> unsigned int slot,
>          pci_conf_write32(seg, bus, slot, func, pos + 4, ~0);
>      }
>      size = pci_conf_read32(seg, bus, slot, func, pos) &
> -           PCI_BASE_ADDRESS_MEM_MASK;
> -    if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> -         PCI_BASE_ADDRESS_MEM_TYPE_64 )
> +           (rom ? PCI_ROM_ADDRESS_MASK : PCI_BASE_ADDRESS_MEM_MASK);

To aid readability and because it repeats ...

> @@ -627,14 +629,14 @@ int pci_size_mem_bar(unsigned int seg, unsigned int 
> bus, unsigned int slot,
>          size |= (uint64_t)~0 << 32;
>      pci_conf_write32(seg, bus, slot, func, pos, bar);
>      size = -size;
> -    addr = (bar & PCI_BASE_ADDRESS_MEM_MASK) | ((uint64_t)hi << 32);
> +    addr = (bar & (rom ? PCI_ROM_ADDRESS_MASK : PCI_BASE_ADDRESS_MEM_MASK)) |

... here, perhaps worth using a local variable just like you did e.g.
with is64bits?

> +    if ( is64bits )
>          return 2;
>  
>      return 1;

Mind folding these into a single return statement now that the
result is going to be reasonably short?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel