Re: [Xen-devel] [PATCH v4 6/9] xen/vpci: add handlers to map the BARs

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

On Wed, Aug 09, 2017 at 02:17:57AM -0600, Jan Beulich wrote:
> >>> On 08.08.17 at 14:35, <roger.pau@xxxxxxxxxx> wrote:
> > On Sat, Jul 29, 2017 at 10:44:02AM -0600, Jan Beulich wrote:
> >> >>> Roger Pau Monne <roger.pau@xxxxxxxxxx> 07/24/17 4:58 PM >>>
> >> >On Fri, Jul 14, 2017 at 09:11:29AM -0600, Jan Beulich wrote:
> >> >> >>> On 30.06.17 at 17:01, <roger.pau@xxxxxxxxxx> wrote:
> >> >> > +        /* Size the BAR and map it. */
> >> >> > +        rc = pci_size_mem_bar(seg, bus, slot, func, reg, i == 
> >> >> > num_bars - 1,
> >> >> > +                              &addr, &size);
> >> >> > +        if ( rc < 0 )
> >> >> > +            return rc;
> >> >> > +
> >> >> > +        if ( size == 0 )
> >> >> > +        {
> >> >> > +            bars[i].type = VPCI_BAR_EMPTY;
> >> >> > +            continue;
> >> >> > +        }
> >> >> > +
> >> >> > +        bars[i].addr = (cmd & PCI_COMMAND_MEMORY) ? addr : 
> >> >> > INVALID_PADDR;
> >> >> 
> >> >> This doesn't match up with logic further up: When the memory decode
> >> >> bit gets cleared, you don't zap the addresses, so I think you'd better
> >> >> store it here too. Use INVALID_PADDR only when the value read has
> >> >> all address bits set (same caveat as pointed out earlier).
> >> >
> >> >OK, note that .addr can only possibly be INVALID_PADDR at
> >> >initialization time, once the user has written something to the BAR
> >> >.addr will be different than INVALID_PADDR.
> >> 
> >> Which is part of what worries me - it would be better if the field wouldn't
> >> ever hold a special init-time-only value.
> > 
> > Right, but that matches the behavior of the hardware itself. On boot
> > the address of the BAR is not valid, but there's no way AFAIK to
> > restore the BAR to this state once an address has been written (except
> > by doing a reset of the device itself).
> 
> True, but the BARs still hold _some_ value. And hence they can
> equally well be made hold a value consistent with normal runtime.

I've changed it to remove the usage of INVALID_PADDR and instead made
the BAR hold the value that Xen finds in the underlying hardware,
without Xen trying to figure out if it's initialized or not.

Roger.

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