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 11:55:39AM +0000, Jan Beulich wrote:
> >>> On 05.10.17 at 13:09, <roger.pau@xxxxxxxxxx> wrote:
> > 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:
> >> >> > --- 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.
> 
> Oh, I'm sorry for not being precise here: I think the hardware
> bit should only be set once the mapping is complete. That's
> not how the code currently behaves, so yes, right now the
> cached bit apparently properly reflects the actual one. With
> the possibly deferred mapping, that wouldn't be the case.

I could add some tail code to vpci_process_pending that sets the
memory decoding or ROM BAR enable bit together with the rom_enable and
enabled fields in the header struct. Would you agree to this?

Thanks, Roger.

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