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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.03.18 at 12:12, <roger.pau@xxxxxxxxxx> wrote:
> 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.

Oh, I see. Still means that apply_map() could have its parameter
constified.

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

Yes. It was largely the asymmetry with bar_write() which did
confuse me, but I can see now why that one doesn't have such
ordering constraints.

Jan

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