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

[Wei Liu <wei.liu2@citrix.com>]

On Thu, Sep 29, 2016 at 03:36:00AM -0600, Jan Beulich wrote:
> >>> On 29.09.16 at 11:23, <konrad.wilk@oracle.com> wrote:
> > 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)?
> 
> No, it's because you subtract more than just mmio_total from GB(2).
> 
> >> 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?)
> 
> Well, I would view this as a bug fix, so it might still be allowed in.
> Ask Wei if in doubt.
> 

Before RC1, sure. After we cut RCs, anything that changes memory layout
of the guests need to be considered carefully.

Wei.

> Jan
> 

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