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

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Fri, Jul 14, 2017 at 04:33:20AM -0600, Jan Beulich wrote:
> >>> On 30.06.17 at 17:01, <roger.pau@xxxxxxxxxx> wrote:
> > So that it can be called from outside in order to get the size of regular 
> > PCI
> > BARs. This will be required in order to map the BARs from PCI devices into 
> > PVH
> > Dom0 p2m.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > 
> > --- a/xen/drivers/passthrough/pci.c
> > +++ b/xen/drivers/passthrough/pci.c
> > @@ -588,6 +588,54 @@ static void pci_enable_acs(struct pci_dev *pdev)
> >      pci_conf_write16(seg, bus, dev, func, pos + PCI_ACS_CTRL, ctrl);
> >  }
> >  
> > +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)
> > +{
> > +    uint32_t hi = 0, bar = pci_conf_read32(seg, bus, slot, func, pos);
> > +    uint64_t addr, size;
> > +
> > +    ASSERT((bar & PCI_BASE_ADDRESS_SPACE) == 
> > PCI_BASE_ADDRESS_SPACE_MEMORY);
> > +    pci_conf_write32(seg, bus, slot, func, pos, ~0);
> > +    if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> > +         PCI_BASE_ADDRESS_MEM_TYPE_64 )
> > +    {
> > +        if ( last )
> > +        {
> > +            printk(XENLOG_WARNING
> > +                    "device %04x:%02x:%02x.%u with 64-bit BAR in last 
> > slot\n",
> 
> This message needs to tell what kind of slot is being processed (just
> like the original did).

The original message is:

"SR-IOV device %04x:%02x:%02x.%u with 64-bit vf BAR in last slot"

I guess you would like to have the "vf" again, in which case I will
add a bool vf parameter to the function that's only going to be used
here. IMHO I'm not really sure it's worth it because I don't find it
that informative. I though that just knowing the device sbdf is
enough.

> > +                    seg, bus, slot, func);
> > +            return -EINVAL;
> > +        }
> > +        hi = pci_conf_read32(seg, bus, slot, func, pos + 4);
> > +        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 )
> > +    {
> > +        size |= (u64)pci_conf_read32(seg, bus, slot, func, pos + 4) << 32;
> 
> uint64_t
> 
> > +        pci_conf_write32(seg, bus, slot, func, pos + 4, hi);
> > +    }
> > +    else if ( size )
> > +        size |= (u64)~0 << 32;
> 
> Again (and more below).

Yes, I think I've fixed all of them.

> > +    pci_conf_write32(seg, bus, slot, func, pos, bar);
> > +    size = -(size);
> 
> Stray parentheses.
> 
> > +    addr = (bar & PCI_BASE_ADDRESS_MEM_MASK) | ((u64)hi << 32);
> > +
> > +    if ( paddr )
> > +        *paddr = addr;
> > +    if ( psize )
> > +        *psize = size;
> 
> Is it reasonable to expect the caller to not care about the size?

Not at the moment, so I guess ASSERT(psize) would be better.

> > @@ -663,38 +710,12 @@ 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_mem_bar(seg, bus, slot, func, idx,
> > +                                       i == PCI_SRIOV_NUM_BARS - 1, NULL,
> > +                                       &pdev->vf_rlen[i]);
> > +                if ( ret < 0 )
> > +                    break;
> 
> ASSERT(ret) ?

Really? This is different from the previous behavior, that would just
break out of the loop in this situation. And on non-debug builds we
would end up decreasing i, which is not good.

Thanks for the review, Roger.

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