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@kernel.org>]

On Thu, Sep 29, 2016 at 11:48:43AM +0100, Wei Liu wrote:
> 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.

OK. I will try my best to get it done before then. But if I fail we can
always look at this for the next release. (Along with other patches
that I need to redo).

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