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

["Roger Pau Monné" <roger.pau@citrix.com>]

Thanks for the review, and sorry for the very late reply.

On Fri, Dec 15, 2017 at 04:43:11AM -0700, Jan Beulich wrote:
> >>> On 18.10.17 at 13:40, <roger.pau@citrix.com> 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, 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.

> > +    if ( !rom )
> > +    {
> > +        uint16_t cmd = pci_conf_read16(pdev->seg, pdev->bus, slot,
> > +                                       func, PCI_COMMAND);
> > +
> > +        cmd &= ~PCI_COMMAND_MEMORY;
> > +        cmd |= map ? PCI_COMMAND_MEMORY : 0;
> > +        pci_conf_write16(pdev->seg, pdev->bus, slot, func, PCI_COMMAND,
> > +                         cmd);
> > +    }
> 
> For both writes, wouldn't it be worthwhile to avoid them when the
> flag is already in the intended state?

If the flag is in the intended state modify_decoding would not be
called at all, because cmd_write and rom_write already filter those
cases.

> > +            if ( v->vpci.map )
> > +            {
> > +                spin_lock(&v->vpci.pdev->vpci->lock);
> > +                modify_decoding(v->vpci.pdev, v->vpci.map, v->vpci.rom);
> > +                spin_unlock(&v->vpci.pdev->vpci->lock);
> > +            }
> > +            /* fallthrough. */
> > +        case -ENOMEM:
> > +            /*
> > +             * Other errors are ignored, hopping that at least some regions
> 
> hoping
> 
> I also don't really understand your intentions here: If other errors
> are being ignored, wouldn't that lead to the rangeset being leaked?
> Or is "other" here meant to include -ENOMEM, in which case you
> really mean "default:" above?

Yes, s/case 0/default/ above, or else this is simply not true, and
leaks the partially consumed rangeset.

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

I've added the following comment:

/* Per-vcpu structure to store state while {un}mapping of PCI BARs. */

Thanks, Roger.

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