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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 09.09.21 13:46, Jan Beulich wrote:
> 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.

The proposed change makes the same code work for both Dom0 and DomU.

So, instead of having the common code as it proposed do you suggest to invent

something special for DomU (making the same job as we already do for Dom0)

and then see if we can then combine the both to have the code common

again? I am saying that the code is already common even if you think that

for DomU it can be simpler (I can't still see in which way as p2m and other

things are not directly touched by the vPCI code, e.g. both Dom0 and DomU

use {map|unmap}_mmio_regions and the only difference is that for Dom0

we have MFN == GFN and for DomU it's not).

So, even if ranges sets are not good for DomUs (I can't see why), but if they 
help

have the code common I think it is worth having them.

>
>>>    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.
It is already known, GFN <-> MFN non-identity mappings
> Then see how much of this actually fits the existing code (handling Dom0).
It is already in the patch: we have all code common for both Dom0 and DomU
> Then decide whether altering Dom0 handling is actually worth it,
> compared to handling DomU separately.
It leads to the same functionality implemented twice
>   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.

You can see from the patch if we have "if ( hwdom )" spread over the

implementation. I guess you won't find that (besides guest register

handlers which is expected).

>
>>> 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.

Hm, I thought that we only map/unmap with memory decoding disabled.

For my education: what happens if you unmap with decoding enabled and

domain accesses the MMIOs?

>   What I was putting under
> question is whether DomU handling code also needs to.
>
> Jan
>
Thank you,

Oleksandr