>>> On 20.03.18 at 12:12, <roger....@citrix.com> wrote:
> On Mon, Mar 19, 2018 at 10:18:34AM -0600, Jan Beulich wrote:
>> >>> On 16.03.18 at 14:30, <roger....@citrix.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
