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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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