Re: [PATCH v2 2/2] hvmloader, pci: Don't try to relocate memory if 64-bit BAR is bigger than ~2GB

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Sep 29, 2016 at 01:03:02AM -0600, Jan Beulich wrote:
> >>> On 29.09.16 at 01:48, <konrad.wilk@oracle.com> wrote:
> > @@ -265,11 +266,30 @@ void pci_setup(void)
> >              bars[i].devfn   = devfn;
> >              bars[i].bar_reg = bar_reg;
> >              bars[i].bar_sz  = bar_sz;
> > +            bars[i].above_4gb = false;
> >  
> >              if ( ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
> >                    PCI_BASE_ADDRESS_SPACE_MEMORY) ||
> >                   (bar_reg == PCI_ROM_ADDRESS) )
> > -                mmio_total += bar_sz;
> > +            {
> > +                /*
> > +                 * If bigger than 2GB minus emulated devices BAR space and
> > +                 * APIC space, then don't try to put under 4GB.
> > +                 */
> > +                if ( is_64bar && (mmio_total >= GB(2) || bar_sz >=
> > +                     (GB(2) - HVM_BELOW_4G_MMIO_LENGTH - mmio_total)) )
> 
> As mentioned in the reply to your earlier mail already, the
> subtraction of mmio_total here is risking wrap through zero (the
> >= GB(2) check doesn't fully guard against that).

I am still waking up so bear with me, but is the reason the mmio_total
>= GB(2) check does not guard is because the compiler may choose
to execute _both_ parts of the '||' conditional (or swap them and
execute the 'mmio_total >= GB(2)' second)?

[Not that I had seen it looking at the assembler output, but that maybe
because I had only seen -O2 output and not -O1?]
> 
> Furthermore you're now making behavior dependent on the order
> devices appear on the bus: The same device appearing early may
> get its BAR placed below 4Gb whereas when it appears late, it'll
> get placed high. IOW I think this needs further refinement: We
> should in a first pass place only 32-bit BARs. In a second pass we
> can then see which 64-bit BARs still fit (and I think we then ought
> to prefer small ones). Which means we should presumably account
> 32- and 64-bit BARs here independent of any other considerations,
> deferring the decision which 64-bit ones to place low until after this
> first pass.

Ok, that is going to require some surgery and movement of code to add
some functions in that giant piece of code. Expect more patches next
week (or would it be easier if I just sent them out for the next release
considering the amount of patches that are floating this week that need
review?)

> 
> Jan
> 

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