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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Roger Pau Monne <roger.pau@xxxxxxxxxx> 07/20/17 4:00 PM >>>
>On Fri, Jul 14, 2017 at 04:33:20AM -0600, Jan Beulich wrote:
> >>> On 30.06.17 at 17:01, <roger.pau@xxxxxxxxxx> wrote:
>> > --- 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.

Note also the "SR-IOV" at the beginning. But either part would be sufficient.

> 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.

It allows deducing the situation in which this function is being called.

>> > +    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.

I don't even see a need for such an ASSERT().

Jan


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