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

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

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

OTOH, if rom is set if means a single ROM BAR is being {un}mapped,
and hence the check for overlaps should be performed against all
the possible BARs, even the ones that belong to the device being
modified.

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

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

> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -20,6 +20,9 @@
> >  #include <xen/smp.h>
> >  #include <xen/perfc.h>
> >  #include <asm/atomic.h>
> > +#ifdef CONFIG_HAS_PCI
> > +#include <xen/vpci.h>
> > +#endif
> 
> Perhaps the conditional would better live in that header.
> 
> > @@ -264,6 +267,11 @@ struct vcpu
> >  
> >      struct evtchn_fifo_vcpu *evtchn_fifo;
> >  
> > +#ifdef CONFIG_HAS_PCI
> > +    /* vPCI per-vCPU area, used to store data for long running operations. 
> > */
> > +    struct vpci_vcpu vpci;
> > +#endif
> 
> And perhaps the header would better provide an empty structure for the
> "else" case. Another option would be to include the fields ...
> 
> >      struct arch_vcpu arch;
> 
> ... in this structure.

I've thought about placing the vpci data inside of arch vpcu, but it
felt a little bit weird because the vpci code should be arch-agnostic,
so placing some of it's data inside of an arch specific structure
seemed wrong. I will do as you suggest and provide an empty
vpci_vpcu structure in the !CONFIG_HAS_PCI case, together with hiding
the CONFIG_HAS_PCI macros inside of the header itself.

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

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.

Roger.

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