Re: [Xen-devel] [PATCH v3 4/9] xen/pci: split code to size BARs from pci_add_device

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.04.17 at 16:35, <roger.pau@xxxxxxxxxx> wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -588,6 +588,51 @@ static void pci_enable_acs(struct pci_dev *pdev)
>      pci_conf_write16(seg, bus, dev, func, pos + PCI_ACS_CTRL, ctrl);
>  }
>  
> +int pci_size_bar(unsigned int seg, unsigned int bus, unsigned int slot,
> +                 unsigned int func, unsigned int base, unsigned int max_bars,
> +                 unsigned int *index, uint64_t *addr, uint64_t *size)
> +{
> +    unsigned int idx = base + *index * 4;

The parameter and variable naming looks confusing to me. Generally
we call what we pass to pci_conf_read*() "pos". I think it would be
better to have the caller pass pos and a boolean indicator whether
another BAR is following, reducing the (base,max_bars,index)
triplet to a pair, and the function returning a negative error or the
(positive) number of BARs to increment by (the more that you leave
half of the incrementing to the caller anyway).

> +    u32 bar = pci_conf_read32(seg, bus, slot, func, idx);
> +    u32 hi = 0;
> +
> +    *addr = *size = 0;

With addr not needed by the current only caller, please allow passing
NULL there. I'm also unconvinced these initializations are actually
needed.

> +    ASSERT((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY);

With this, the function name should be more like pci_size_mem_bar().

> +    pci_conf_write32(seg, bus, slot, func, idx, ~0);
> +    if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> +         PCI_BASE_ADDRESS_MEM_TYPE_64 )
> +    {
> +        if ( *index >= max_bars )
> +        {
> +            dprintk(XENLOG_WARNING,
> +                    "device %04x:%02x:%02x.%u with 64-bit BAR in last 
> slot\n",
> +                    seg, bus, slot, func);

This was a normal printk() originally.

> @@ -663,38 +708,13 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>                             seg, bus, slot, func, i);
>                      continue;
>                  }
> -                pci_conf_write32(seg, bus, slot, func, idx, ~0);
> -                if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> -                     PCI_BASE_ADDRESS_MEM_TYPE_64 )
> -                {
> -                    if ( i >= PCI_SRIOV_NUM_BARS )
> -                    {
> -                        printk(XENLOG_WARNING
> -                               "SR-IOV device %04x:%02x:%02x.%u with 64-bit"
> -                               " vf BAR in last slot\n",
> -                               seg, bus, slot, func);
> -                        break;
> -                    }
> -                    hi = pci_conf_read32(seg, bus, slot, func, idx + 4);
> -                    pci_conf_write32(seg, bus, slot, func, idx + 4, ~0);
> -                }
> -                pdev->vf_rlen[i] = pci_conf_read32(seg, bus, slot, func, 
> idx) &
> -                                   PCI_BASE_ADDRESS_MEM_MASK;
> -                if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> -                     PCI_BASE_ADDRESS_MEM_TYPE_64 )
> -                {
> -                    pdev->vf_rlen[i] |= (u64)pci_conf_read32(seg, bus,
> -                                                             slot, func,
> -                                                             idx + 4) << 32;
> -                    pci_conf_write32(seg, bus, slot, func, idx + 4, hi);
> -                }
> -                else if ( pdev->vf_rlen[i] )
> -                    pdev->vf_rlen[i] |= (u64)~0 << 32;
> -                pci_conf_write32(seg, bus, slot, func, idx, bar);
> -                pdev->vf_rlen[i] = -pdev->vf_rlen[i];
> -                if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> -                     PCI_BASE_ADDRESS_MEM_TYPE_64 )
> -                    ++i;
> +                ret = pci_size_bar(seg, bus, slot, func, pos + PCI_SRIOV_BAR,
> +                                   PCI_SRIOV_NUM_BARS, &i, &addr,
> +                                   &pdev->vf_rlen[i]);
> +                if ( ret )
> +                    dprintk(XENLOG_WARNING,
> +                            "%04x:%02x:%02x.%u: failed to size SR-IOV 
> BAR%u\n",
> +                            seg, bus, slot, func, i);

You shouldn't log two messages for the same problem (the called
function already logs one).

A final more general remark: With you intending to call this function
from other than pci_add_device() context, some further care may /
will be needed. For example, are all to be added callers such that
you playing with config space won't interfere with what Dom0 does?
Are you sure you can get away without disabling memory decode
while fiddling with the BARs?

Jan

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