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

[Jan Beulich <jbeulich@suse.com>]

On 09.09.2021 12:03, Oleksandr Andrushchenko wrote:
> On 09.09.21 12:39, Jan Beulich wrote:
>> On 09.09.2021 11:12, Oleksandr Andrushchenko wrote:
>>> Anyways, I am open to any decision on what would be the right approach here:
>>>
>>> 1. Use range sets per BAR as in the patch
>>>
>>> 2. Remove range sets completely and have a per-vCPU list with mapping
>>>
>>> data as I described above
>>>
>>> 3. Anything else?
>> A decision first requires a proposal.
> 
> I already have 2: one in the patch with the range set per BAR and one described
> earlier in the thread with a single range set and a list for GFN <-> MFN.
> If you can tell your opinion I am all ears. But, please be specific as common words
> don't change anything to me.
> At the same time I do understand that the current code is not set in stone,
> but we should have a good reason for major changes to it, IMO.

And I view your change, as proposed, as a major one. You turn the logic all
over imo.

> I mean that before
> DomU's we were fine with the range sets etc, and now we are not:
> so what has changed so much?

Nothing has changed. I'm not advocating for removal of the rangeset use in
handling Dom0's needs. I'm suggesting that their use might not be a good
fit for DomU.

>>   I think 3 is the way to investigate
>> first: Rather than starting from the code we currently have, start from
>> what you need for DomU-s to work. If there's enough overlap with how we
>> handle Dom0, code can be shared.
> 
> You can see that in my patch the same code is used by both hwdom and
> guest. What else needs to be proven? The patch shows that all the code
> besides guest register handlers (which is expected) is all common.

The complexity of dealing with Dom0 has increased. I've outlined the
process that I think should be followed: First determine what DomU needs.
Then see how much of this actually fits the existing code (handling Dom0).
Then decide whether altering Dom0 handling is actually worth it,
compared to handling DomU separately. In fact handling it separately
first may have its own benefits, like easing review and reducing the risk
of breaking Dom0 handling. If then there are enough similarities, in a
2nd step both may want folding.

>> All of this applies only with memory decoding enabled, I expect.
>> Disabling memory decoding on a device ought to be a simple "unmap all
>> BARs", while enabling is "map all BARs". Which again is, due to the
>> assumed lack of sharing of pages, much simpler than on Dom0: You only
>> need to subtract the MSI-X table range(s) (if any, and perhaps not
>> necessary when unmapping, as there's nothing wrong to unmap a P2M slot
>> which wasn't mapped); this may not even require any rangeset at all to
>> represent.
>>
>> And in fact I wonder whether for DomU-s you want to support BAR changes
>> in the first place while memory decoding is enabled.
> 
> No, why? I want to keep the existing logic, e.g. with memory decoding
> disabled as it is now.

Afaict existing code deals with both cases. What I was putting under
question is whether DomU handling code also needs to.

Jan