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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.10.17 at 11:20, <roger.pau@xxxxxxxxxx> wrote:
> On Wed, Oct 04, 2017 at 08:33:33AM +0000, Jan Beulich wrote:
>> >>> On 19.09.17 at 17:29, <roger.pau@xxxxxxxxxx> wrote:
>> > +static int vpci_check_bar_overlap(const struct pci_dev *pdev,
>> > +                                  const struct vpci_bar *rom,
>> > +                                  struct rangeset *mem)
>> > +{
>> > +    const struct pci_dev *cmp;
>> > +
>> > +    /* Check for overlaps with other device's BARs. */
>> > +    list_for_each_entry(cmp, &pdev->domain->arch.pdev_list, domain_list)
>> > +    {
>> > +        unsigned int i;
>> > +
>> > +        if ( rom == NULL && pdev == cmp )
>> > +            continue;
>> 
>> This check looks rather unmotivated (or even bogus) without a comment.
>> The other special casing of ROM BARs further down also isn't all that
>> obvious (and right now I can't even convince myself it's correct).
> 
> I've added the following comment before this check, which I think
> explains the logic for this check, and the one below:
> 
> Since ROM BARs can be enabled independently of the memory decoding
> bit we need to check for overlapping in slightly different
> ways depending on the case.
> 
> If !rom it means the memory decoding bit has been toggled, and all
> BARs belonging to the device will be {un}mapped,

That's not precise: When mapping, you may still skip the ROM one
if its enable bit is clear. Whether the difference matters for
unmapping when the ROM is already unmapped I can't tell right
away. Nevertheless I think ...

> hence the rangeset
> will contain the mappings for the whole device. In this case there's
> no need to check for overlaps with BARs that belong to the same
> device because the rangeset is able to deal with overlapping areas.

... the conclusion is correct, as I would expect the ROM range to
simply not be part of the rangeset then.

>> > +        }
>> > +    }
>> > +
>> > +    /* Check for overlaps with other device's BARs. */
>> > +    rc = vpci_check_bar_overlap(pdev, NULL, mem);
>> 
>> Why is this not symmetrical with vpci_modify_rom() (which also checks
>> overlaps inside the current device)?
> 
> I think the comment above should answer the question here, the
> difference is because in this case Xen is mapping a whole device, so
> vpci_check_bar_overlap should not check for overlap with BARs that
> belong to the same device. OTOH, when mapping a ROM BAR Xen should
> check for such overlap, because the regular BARs will already be
> mapped.

Right. Part of my confusion results from the naming of these
two functions (pretty similar despite their different call sites)
as well as their placement (modify_bars() sitting ahead of
the CMD write is fine, as it's a helper of that function, but
modify_rom() would better be moved down to make clear
whose helper it is; it's questionable whether this being a
separate helper function is actually useful).

>> > +    }
>> > +
>> > +    rc = vpci_maybe_defer_map(pdev->domain, mem, map);
>> > +    if ( !rc )
>> > +        for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
>> > +            if ( header->bars[i].type != VPCI_BAR_ROM ||
>> > +                 header->bars[i].rom_enabled )
>> > +            header->bars[i].enabled = map;
>> 
>> Hmm, you're updating state here regardless of possible failure in the
>> deferred operation (see the discarded error code in
>> vpci_check_pending()).
> 
> Yes, I've fixed the code above to try to map/unmap as much as
> possible, even when a failure happens.
> 
> I agree that enabling/disabling here with the operation being deferred
> is not ideal, but I also think we would end up doing the same
> regardless of the outcome of the deferred operation. If some
> mapping/unmapping of BARs failed, the memory decoding should be
> enabled anyway. I can add a comment along this lines if you think
> that's OK.

Yes, at least explaining why things are the (not fully correct) way
they are would help (also to tell anyone wanting to improve this
what it actually is that would need changing). Of course even
better would be if maintained state would match the state
hardware is in.

>> > --- a/xen/include/xen/vpci.h
>> > +++ b/xen/include/xen/vpci.h
>> > @@ -35,11 +35,52 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, 
>> > unsigned int size);
>> >  void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
>> >                  uint32_t data);
>> >  
>> > +/*
>> > + * Check for pending vPCI operations on this vcpu. Returns true if the 
>> > vcpu
>> > + * should not run.
>> > + */
>> > +bool vpci_check_pending(struct vcpu *v);
>> > +
>> >  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 {
>> > +            paddr_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;
>> > +            /* Store whether the BAR is mapped into guest p2m. */
>> > +            bool enabled;
>> > +            /*
>> > +             * 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;
>> 
>> Especially with the error handling issue in mind that I've mentioned
>> earlier, I wonder whether this field shouldn't be dropped, along the
>> lines of you also no longer caching the memory decode enable bit in the
>> command register.
> 
> Removing rom_enabled would imply doing a register read in
> vpci_modify_bars in order to know whether the ROM BAR is enabled or
> not, which is not trivial because depending on the header type the
> position of the ROM BAR is different.

As said - I wouldn't mind the field if it was always in sync with the
hardware one. And it was for a reason that I mentioned the
memory decode bit, which you no longer cache. I think both
should be treated the same.

> Another option would be to store the prefetch/enable bits inside of
> the addr field, but that would also require more masking/unmasking of
> the fields when the values are used or updated.

I didn't ask for these two to be eliminated.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel