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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Jan 19, 2018 at 09:16:54AM -0700, Jan Beulich wrote:
> >>> 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?

I've moved it from the BAR to the header struct.

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

Never mind, since this is not used in vpci it doesn't matter. I will
remove the guards.

Thanks, Roger.

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