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

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

On Thu, Oct 05, 2017 at 10:01:46AM +0000, Jan Beulich wrote:
> >>> 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:
> >> > +    }
> >> > +
> >> > +    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.

I see the current code in this version is slightly confusing regarding
the usage of the 'enabled' field. I've fixed the code so that the
'enabled' field matches the following conditions:

 - For non-ROM BARs: the 'enabled' field matches the value of the
   memory decoding bit, but it doesn't guarantee that the range is
   fully mapped/unmapped in the guest p2m.
 - For ROM BARs: the 'enabled' bit matches the value of the memory
   decoding bit & the ROM enable bit, but again it doesn't guarantee
   that the memory is fully mapped/unmapped in the guest p2m.

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

I think I'm missing something, rom_enabled matches exactly the state
of the ROM enable bit. There's no way rom_enabled will get updated
without the BAR ROM also being updated in vpci_rom_write.

In line with my comments above regarding the 'enabled' field, what
about adding:

 - rom_enabled matches the state of the ROM BAR enable bit. It doesn't
   take into account the state of the memory decoding bit. As such, it
   cannot be used to detect if the ROM BAR memory is active or not,
   the 'enabled' bit should be used in that case.

Roger.

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