Re: [Xen-devel] [PATCH v7 for-next 09/12] vpci/bars: add handlers to map the BARs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.01.18 at 16:47, <roger.pau@xxxxxxxxxx> wrote:
> On Fri, Dec 15, 2017 at 04:43:11AM -0700, Jan Beulich wrote:
>> >>> On 18.10.17 at 13:40, <roger.pau@xxxxxxxxxx> wrote:
>> > +static void modify_decoding(const struct pci_dev *pdev, bool map, bool 
>> > rom)
>> > +{
>> > +    struct vpci_header *header = &pdev->vpci->header;
>> > +    uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
>> > +    unsigned int i;
>> > +
>> > +    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
>> > +    {
>> > +        if ( rom && header->bars[i].type == VPCI_BAR_ROM )
>> > +        {
>> > +            unsigned int rom_pos = (i == 6) ? PCI_ROM_ADDRESS
>> > +                                            : PCI_ROM_ADDRESS1;
>> > +            uint32_t val = pci_conf_read32(pdev->seg, pdev->bus, slot, 
>> > func,
>> > +                                           rom_pos);
>> > +
>> > +            header->bars[i].enabled = header->bars[i].rom_enabled = map;
>> > +
>> > +            val &= ~PCI_ROM_ADDRESS_ENABLE;
>> > +            val |= map ? PCI_ROM_ADDRESS_ENABLE : 0;
>> > +            pci_conf_write32(pdev->seg, pdev->bus, slot, func, rom_pos, 
>> > val);
>> > +            break;
>> > +        }
>> > +        if ( !rom && (header->bars[i].type != VPCI_BAR_ROM ||
>> > +                      header->bars[i].rom_enabled) )
>> > +            header->bars[i].enabled = map;
>> > +    }
>> 
>> Looking at all of this, it would clearly be more logical for
>> rom_enabled to be a per-header instead of a per-BAR flag.
> 
> Hm, I'm not sure just moving the rom_enable would be such a win,

I didn't say it would be a win, but that it would be more logical.

> we
> would still need to iterate over the array of BARs in order to find
> the position of the ROM BAR and thus which register has to be used
> (PCI_ROM_ADDRESS or PCI_ROM_ADDRESS1).
> 
> I could solve that but it would mean adding a bool plus an unsigned
> int field to store the position of the ROM BAR. Since this is not
> going to change the behavior I would rather leave this for future
> improvements, likely when SR-IOV is implemented and we have a better
> picture of what needs to be stored in the 'header' struct.

Hmm, okay, I certainly donn't want you to add yet another field.
Could you perhaps attach a brief comment to the field declaration
indicating why it's in the BAR structure rather than in the header
one?

>> >  struct vpci {
>> >      /* List of vPCI handlers for a device. */
>> >      struct list_head handlers;
>> >      spinlock_t lock;
>> > +
>> > +#ifdef __XEN__
>> > +    /* Hide the rest of the vpci struct from the user-space test harness. 
>> > */
>> > +    struct vpci_header {
>> > +        /* Information about the PCI BARs of this device. */
>> > +        struct vpci_bar {
>> > +            uint64_t addr;
>> > +            uint64_t size;
>> > +            enum {
>> > +                VPCI_BAR_EMPTY,
>> > +                VPCI_BAR_IO,
>> > +                VPCI_BAR_MEM32,
>> > +                VPCI_BAR_MEM64_LO,
>> > +                VPCI_BAR_MEM64_HI,
>> > +                VPCI_BAR_ROM,
>> > +            } type;
>> > +            bool prefetchable : 1;
>> > +            /* Store whether the BAR is mapped into guest p2m. */
>> > +            bool enabled      : 1;
>> > +            /*
>> > +             * Store whether the ROM enable bit is set (doesn't imply ROM 
>> > BAR
>> > +             * is mapped into guest p2m). Only used for type VPCI_BAR_ROM.
>> > +             */
>> > +            bool rom_enabled  : 1;
>> > +        } bars[7]; /* At most 6 BARS + 1 expansion ROM BAR. */
>> > +        /* FIXME: currently there's no support for SR-IOV. */
>> > +    } header;
>> > +#endif
>> >  };
>> >  
>> > +#ifdef __XEN__
>> > +struct vpci_vcpu {
>> > +    struct rangeset *mem;
>> > +    const struct pci_dev *pdev;
>> > +    bool map : 1;
>> > +    bool rom : 1;
>> > +};
>> > +#endif
>> 
>> This structure could do with a comment briefly noting it purpose.
>> Also - if the #ifdef really needed here?
> 
> I prefer to add the ifdef rather than adding a struct rangeset forward
> declaration to tests/vpci/emul.h.

Why would you need a forward declaration? This isn't function
declaration, but a structure one.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel