Re: [Xen-devel] [PATCH v6 05/11] pci: split code to size BARs from pci_add_device

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.09.17 at 17:29, <roger.pau@xxxxxxxxxx> wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -603,6 +603,56 @@ static int iommu_add_device(struct pci_dev *pdev);
>  static int iommu_enable_device(struct pci_dev *pdev);
>  static int iommu_remove_device(struct pci_dev *pdev);
>  
> +int pci_size_mem_bar(pci_sbdf_t sbdf, unsigned int pos, bool last,
> +                     uint64_t *paddr, uint64_t *psize, unsigned int flags)
> +{
> +    uint32_t hi = 0, bar = pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev,
> +                                           sbdf.func, pos);
> +    uint64_t addr, size;
> +    bool vf = flags & PCI_BAR_VF;

Honestly I'm not convinced of the utility of this variable; same for the
"rom" one in the next patch.

> +    ASSERT((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY);
> +    pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos, ~0);
> +    if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> +         PCI_BASE_ADDRESS_MEM_TYPE_64 )
> +    {
> +        if ( last )
> +        {
> +            printk(XENLOG_WARNING
> +                   "%sdevice %04x:%02x:%02x.%u with 64-bit %sBAR in last 
> slot\n",
> +                   vf ? "SR-IOV " : "", sbdf.seg, sbdf.bus, sbdf.dev, 
> sbdf.func,
> +                   vf ? "vf " : "");
> +            return -EINVAL;
> +        }
> +        hi = pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos + 
> 4);
> +        pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos + 4, 
> ~0);
> +    }
> +    size = pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos) &
> +           PCI_BASE_ADDRESS_MEM_MASK;
> +    if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> +         PCI_BASE_ADDRESS_MEM_TYPE_64 )
> +    {
> +        size |= (uint64_t)pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev,
> +                                          sbdf.func, pos + 4) << 32;
> +        pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos + 4, 
> hi);
> +    }
> +    else if ( size )
> +        size |= (uint64_t)~0 << 32;
> +    pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos, bar);
> +    size = -size;
> +    addr = (bar & PCI_BASE_ADDRESS_MEM_MASK) | ((uint64_t)hi << 32);
> +
> +    if ( paddr )
> +        *paddr = addr;

You need addr only inside the if() - no need for the local variable,
and no need to calculate it unconditionally.

> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -189,6 +189,10 @@ const char *parse_pci(const char *, unsigned int *seg, 
> unsigned int *bus,
>  const char *parse_pci_seg(const char *, unsigned int *seg, unsigned int *bus,
>                            unsigned int *dev, unsigned int *func, bool 
> *def_seg);
>  
> +#define _PCI_BAR_VF     0
> +#define PCI_BAR_VF      (1u << _PCI_BAR_VF)

Do you really need both? I know we have quite a few cases where flags
are being defined this way, but that's usually when bit operations
(test_bit() and alike) are intended on the flags fields.

Jan


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