Re: [Xen-devel] [PATCH v10 07/11] vpci/bars: add handlers to map the BARs

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

On Mon, Mar 19, 2018 at 10:18:34AM -0600, Jan Beulich wrote:
> >>> On 16.03.18 at 14:30, <roger.pau@xxxxxxxxxx> wrote:
> > +static int modify_bars(const struct pci_dev *pdev, bool map, bool rom_only)
> > +{
> > +    struct vpci_header *header = &pdev->vpci->header;
> > +    struct rangeset *mem = rangeset_new(NULL, NULL, 0);
> > +    struct pci_dev *tmp, *dev = NULL;
> > +    unsigned int i;
> > +    int rc;
> > +
> > +    if ( !mem )
> > +        return -ENOMEM;
> > +
> > +    /*
> > +     * Create a rangeset that represents the current device BARs memory 
> > region
> > +     * and compare it against all the currently active BAR memory regions. 
> > If
> > +     * an overlap is found, subtract it from the region to be 
> > mapped/unmapped.
> > +     *
> > +     * First fill the rangeset with all the BARs of this device or with 
> > the ROM
> > +     * BAR only, depending on whether the guest is toggling the memory 
> > decode
> > +     * bit of the command register, or the enable bit of the ROM BAR 
> > register.
> > +     */
> > +    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
> > +    {
> > +        const struct vpci_bar *bar = &header->bars[i];
> > +
> > +        if ( !MAPPABLE_BAR(bar) ||
> > +             (rom_only ? bar->type != VPCI_BAR_ROM
> > +                       : (bar->type == VPCI_BAR_ROM && 
> > !header->rom_enabled)) )
> > +            continue;
> > +
> > +        rc = rangeset_add_range(mem, PFN_DOWN(bar->addr),
> > +                                PFN_DOWN(bar->addr + bar->size - 1));
> > +        if ( rc )
> > +        {
> > +            printk(XENLOG_G_WARNING
> > +                   "Failed to add [%" PRI_gfn ", %" PRI_gfn "]: %d\n",
> > +                   PFN_DOWN(bar->addr), PFN_DOWN(bar->addr + bar->size - 
> > 1),
> > +                   rc);
> > +            rangeset_destroy(mem);
> > +            return rc;
> > +        }
> > +    }
> > +
> > +    /*
> > +     * Check for overlaps with other BARs. Note that only BARs that are
> > +     * currently mapped (enabled) are checked for overlaps.
> > +     */
> > +    list_for_each_entry(tmp, &pdev->domain->arch.pdev_list, domain_list)
> > +    {
> > +        if ( tmp == pdev )
> > +        {
> > +            /*
> > +             * Need to store the device so it's not constified and
> > +             * maybe_defer_map can modify it in case of error.
> > +             */
> > +            dev = tmp;
> 
> There's no maybe_defer_map anymore.
> 
> And then I'm having a problem with this comment on const-ness:
> apply_map() only wants to hand the device to modify_decoding(),
> whose respective argument is const. defer_map() stores the
> pointer, but afaics again only for vpci_process_pending() to hand
> it on to modify_decoding(); the lock the function takes is in a
> structure pointed to from the device, so unaffected by the const.

vpci_process_pending calls vpci_remove_device which needs a
non-constified pdev, since it sets pdev->vpci = NULL.

> > +             * memory has been added or removed from the p2m (because the
> > +             * actual p2m changes are deferred in maybe_defer_map) and the 
> > ROM
> > +             * enable bit has not been changed, so leave everything as-is,
> > +             * hoping the guest will realize and try again.
> > +             */
> > +            return;
> > +    }
> > +    else
> > +    {
> > +        header->rom_enabled = new_enabled;
> > +        pci_conf_write32(pdev->seg, pdev->bus, slot, func, reg, val);
> > +    }
> > +
> > +    if ( !new_enabled )
> > +        rom->addr = val & PCI_ROM_ADDRESS_MASK;
> 
> I'm struggling to understand this update, not the least seeing the
> other update further up.

I've added some comments now, but the point is that when mapping the
ROM BAR we should update the addr field first, so that the correct p2m
mappings are established

In the unmap case however (!new_enabled) the addr needs to be updated
after calling modify_bars, or else the wrong address might be
unmapped.

Does this address your concern?

Thanks, Roger.

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