Re: [v7][PATCH 06/16] hvmloader/pci: skip reserved ranges

[George Dunlap <george.dunlap@eu.citrix.com>]

On 07/15/2015 12:24 PM, Jan Beulich wrote:
>>>> On 15.07.15 at 13:05, <George.Dunlap@eu.citrix.com> wrote:
>> On Wed, Jul 15, 2015 at 10:27 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 15.07.15 at 10:59, <tiejun.chen@intel.com> wrote:
>>>> What about this?
>>>>
>>>> @@ -301,6 +301,19 @@ void pci_setup(void)
>>>>               pci_mem_start <<= 1;
>>>>       }
>>>>
>>>> +    for ( i = 0; i < memory_map.nr_map ; i++ )
>>>> +    {
>>>> +        uint64_t reserved_start, reserved_size;
>>>> +        reserved_start = memory_map.map[i].addr;
>>>> +        reserved_size = memory_map.map[i].size;
>>>> +        if ( check_overlap(pci_mem_start, pci_mem_end - pci_mem_start,
>>>> +                           reserved_start, reserved_size) )
>>>> +        {
>>>> +            printf("Reserved device memory conflicts current PCI memory.\n");
>>>> +            BUG();
>>>> +        }
>>>> +    }
>>>
>>> So what would the cure be if someone ran into this BUG() (other
>>> than removing the device associated with the conflicting RMRR)?
>>> Afaics such a guest would remain permanently unbootable, which
>>> of course is not an option.
>>
>> Is not booting worse than what we have now -- which is, booting
>> successfully but (probably) having issues due to MMIO ranges
>> overlapping RMRRs?
> 
> Again a matter of perspective: For devices (USB!) where the RMRR
> exists solely for boot time (or outdated OS) use, this would be a
> plain regression. For the graphics device Tiejun needs this for, it of
> course would make little difference, I agree.

So it's the case that, for some devices, not only is it functionally OK
to have RAM in the RMRR with no issues, it's also functionally OK to
have PCI BARs in the RMRR with no issues?

If so, then yes, I agree having a device fail to work with no ability to
work around it is an unacceptable regression.

If we're not targeting 4.6 for this, then the situation changes
somewhat.  One thing worth considering (which perhaps may be what tiejun
is looking at) is the cost of keeping a large number of
working-and-already-acked patches out of tree -- both the psychological
cost, the cost of rebasing, and the cost of re-reviewing rebases.  Given
how independent the hvmloader changes are to the rest of the series,
it's probably worth trying to see if we can check in the other patches
as soon as we branch.

If we can check in those patches with hvmloader still ignoring RMRRs,
then there's no problem.  But if we need the issue addressed *somehow*
in hvmloader before checking the rest of the series in, then I think it
makes sense to consider a minimal change that would make the series
somewhat functional, before moving on to overhauling the hvmloader MMIO
placement code.

 -George