Re: [PATCH 6/9] vpci/header: Handle p2m range sets per BAR

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.09.2021 16:31, Oleksandr Andrushchenko wrote:
> 
> On 06.09.21 17:47, Jan Beulich wrote:
>> On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>>
>>> Instead of handling a single range set, that contains all the memory
>>> regions of all the BARs and ROM, have them per BAR.
>> Without looking at how you carry out this change - this look wrong (as
>> in: wasteful) to me. Despite ...
>>
>>> This is in preparation of making non-identity mappings in p2m for the
>>> MMIOs/ROM.
>> ... the need for this, every individual BAR is still contiguous in both
>> host and guest address spaces, so can be represented as a single
>> (start,end) tuple (or a pair thereof, to account for both host and guest
>> values). No need to use a rangeset for this.
> 
> First of all this change is in preparation for non-identity mappings,

I'm afraid I continue to not see how this matters in the discussion at
hand. I'm fully aware that this is the goal.

> e.g. currently we collect all the memory ranges which require mappings
> into a single range set, then we cut off MSI-X regions and then use range set
> functionality to call a callback for every memory range left after MSI-X.
> This works perfectly fine for 1:1 mappings, e.g. what we have as the range
> set's starting address is what we want to be mapped/unmapped.
> Why range sets? Because they allow partial mappings, e.g. you can map part of
> the range and return back and continue from where you stopped. And if I
> understand that correctly that was the initial intention of introducing range 
> sets here.
> 
> For non-identity mappings this becomes not that easy. Each individual BAR may 
> be
> mapped differently according to what guest OS has programmed as 
> bar->guest_addr
> (guest view of the BAR start).

I don't see how the rangeset helps here. You have a guest and a host pair
of values for every BAR. Pages with e.g. the MSI-X table may not be mapped
to their host counterpart address, yes, but you need to special cases
these anyway: Accesses to them need to be handled. Hence I'm having a hard
time seeing how a per-BAR rangeset (which will cover at most three distinct
ranges afaict, which is way too little for this kind of data organization
imo) can gain you all this much.

Overall the 6 BARs of a device will cover up to 8 non-adjacent ranges. IOW
the majority (4 or more) of the rangesets will indeed merely represent a
plain (start,end) pair (or be entirely empty).

> Thus we need to collect all those non-identity mappings
> per BAR now (so we have a mapping "guest view" : "physical BAR" and again cut 
> off
> MSI-X regions as before.  So, yes, it may be a bit wasteful to use many range 
> sets,
> but makes vPCI life much-much easier.

Which I'm yet to be convinced of. Then again I'm not the maintainer of
this code, so if you can convince Roger you'll be all good.

> Thus, I think that even per-BAR range sets are
> good to go as they have more pros than cons. IMO
> Even if we go with "can be represented as a single (start,end) tuple" it 
> doesn't answer
> the question what needs to be done if a range gets partially mapped/unmapped.

This question also isn't answered when you use rangesets.

Jan