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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.09.16 at 11:23, <konrad.wilk@xxxxxxxxxx> wrote:
> On Thu, Sep 29, 2016 at 01:03:02AM -0600, Jan Beulich wrote:
>> >>> On 29.09.16 at 01:48, <konrad.wilk@xxxxxxxxxx> 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.

Jan


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