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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.03.18 at 14:30, <roger.pau@xxxxxxxxxx> wrote:
> +static int map_range(unsigned long s, unsigned long e, void *data,
> +                     unsigned long *c)
> +{
> +    const struct map_data *map = data;
> +    int rc;
> +
> +    for ( ; ; )
> +    {
> +        unsigned long size = e - s + 1;
> +
> +        /*
> +         * ARM TODOs:
> +         * - On ARM whether the memory is prefetchable or not should be 
> passed
> +         *   to map_mmio_regions in order to decide which memory attributes
> +         *   should be used.
> +         *
> +         * - {un}map_mmio_regions doesn't support preemption.
> +         */
> +
> +        rc = (map->map ? map_mmio_regions : unmap_mmio_regions)
> +             (map->d, _gfn(s), size, _mfn(s));

This, btw, is one of those instances where a pair of direct calls
may end up being translated to an indirect one by the compiler.
Despite growing redundancy in source if you address that, I
think I agree with Andrew that we'd prefer to avoid the indirect
call here.

> +bool vpci_process_pending(struct vcpu *v)
> +{
> +    if ( v->vpci.mem )
> +    {
> +        struct map_data data = {
> +            .d = v->domain,
> +            .map = v->vpci.map,
> +        };
> +        int rc = rangeset_consume_ranges(v->vpci.mem, map_range, &data);
> +
> +        if ( rc == -ERESTART )
> +            return true;
> +
> +        spin_lock(&v->vpci.pdev->vpci->lock);
> +        /* Disable memory decoding unconditionally on failure. */
> +        modify_decoding(v->vpci.pdev, rc ? false : v->vpci.map,
> +                        rc ? false : v->vpci.rom_only);

These two ternary expressions could be simplified to "!rc && ..."
afaict.

> +static int __init apply_map(struct domain *d, struct pci_dev *pdev,
> +                            struct rangeset *mem)
> +{
> +    struct map_data data = { .d = d, .map = true };
> +    int rc;
> +
> +    while ( (rc = rangeset_consume_ranges(mem, map_range, &data)) == 
> -ERESTART )
> +        process_pending_softirqs();
> +    rangeset_destroy(mem);
> +    if ( rc )
> +        return rc;
> +    modify_decoding(pdev, true, false);
> +
> +    return rc;
> +}

    if ( !rc )
        modify_decoding(pdev, true, false);

    return rc;

would make this function have just a single return point without
adversely affecting readability.

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

> +            if ( !rom_only )
> +                /*
> +                 * If memory decoding is toggled avoid checking against the
> +                 * same device, or else all regions will be removed from the
> +                 * memory map in the unmap case.
> +                 */
> +                continue;
> +        }
> +
> +        for ( i = 0; i < ARRAY_SIZE(tmp->vpci->header.bars); i++ )
> +        {
> +            const struct vpci_bar *bar = &tmp->vpci->header.bars[i];
> +            unsigned long start = PFN_DOWN(bar->addr);
> +            unsigned long end = PFN_DOWN(bar->addr + bar->size - 1);
> +
> +            if ( !bar->enabled || !rangeset_overlaps_range(mem, start, end) 
> ||
> +                 /*
> +                  * If only the ROM enable bit is toggled check against other
> +                  * BARs in the same device for overlaps, but not against the
> +                  * same ROM BAR.
> +                  */
> +                 (rom_only && tmp == pdev && bar->type == VPCI_BAR_ROM) )
> +                continue;
> +
> +            rc = rangeset_remove_range(mem, start, end);
> +            if ( rc )
> +            {
> +                printk(XENLOG_G_WARNING
> +                       "Failed to remove [%" PRI_gfn ", %" PRI_gfn "]: %d\n",
> +                       start, end, rc);
> +                rangeset_destroy(mem);
> +                return rc;
> +            }
> +        }
> +    }
> +
> +    ASSERT(dev);
> +
> +    if ( system_state < SYS_STATE_active )
> +    {
> +        /*
> +         * Mappings might be created when building Dom0 if the memory 
> decoding
> +         * bit of PCI devices is enabled. In that case it's not possible to
> +         * defer the operation, so call apply_map in order to create the
> +         * mappings right away. Note that at build time this function will 
> only
> +         * be called iff the memory decoding bit is enabled, thus the 
> operation
> +         * will always be to establish mappings and process all the BARs.
> +         */
> +        ASSERT(map && !rom_only);
> +        return apply_map(pdev->domain, dev, mem);
> +    }
> +
> +    defer_map(pdev->domain, dev, mem, map, rom_only);

In both calls, in case the dual pdev/dev needs to be retained for
some reason, please use only/consistently one of the two here,
to make visible at the first glance that the domain passed is the
owner of the device passed.

> +static void rom_write(const struct pci_dev *pdev, unsigned int reg,
> +                      uint32_t val, void *data)
> +{
> +    struct vpci_header *header = &pdev->vpci->header;
> +    struct vpci_bar *rom = data;
> +    uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
> +    uint16_t cmd = pci_conf_read16(pdev->seg, pdev->bus, slot, func,
> +                                   PCI_COMMAND);
> +    bool new_enabled = val & PCI_ROM_ADDRESS_ENABLE;
> +
> +    if ( (cmd & PCI_COMMAND_MEMORY) && header->rom_enabled && new_enabled )
> +    {
> +        gprintk(XENLOG_WARNING,
> +                "%04x:%02x:%02x.%u: ignored ROM BAR write with memory 
> decoding enabled\n",
> +                pdev->seg, pdev->bus, slot, func);
> +        return;
> +    }
> +
> +    if ( !header->rom_enabled )
> +        rom->addr = val & PCI_ROM_ADDRESS_MASK;
> +
> +    /* Check if ROM BAR should be mapped/unmapped. */
> +    if ( (cmd & PCI_COMMAND_MEMORY) && header->rom_enabled != new_enabled )
> +    {
> +        if ( modify_bars(pdev, new_enabled, true) )
> +            /*
> +             * Return on error in order to avoid updating the 'addr' field. 
> No

Note that if you inverted the outer if() condition, you would get
away with a level less of indentation of this (now inner) if() body.

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

Jan

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