Re: [PATCH 2/2] xen/arm: p2m: Populate pages for GICv2 mapping in arch_domain_create()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.10.2022 00:33, Julien Grall wrote:
> On 19/10/2022 22:30, Andrew Cooper wrote:
>> On 18/10/2022 00:01, Julien Grall wrote:
>>>>> ... you are hardcoding both p2m_teardown() and p2m_set_allocation().
>>>>> IMO this is not an improvement at all. It is just making the code more
>>>>> complex than necessary and lack all the explanation on the assumptions.
>>>>>
>>>>> So while I am fine with your patch #1 (already reviewed it), there is
>>>>> a better patch from Henry on the ML. So we should take his (rebased)
>>>>> instead of yours.
>>>>
>>>> If by better, you mean something that still has errors, then sure.
>>>>
>>>> There's a really good reason why you cannot safely repurpose
>>>> p2m_teardown().  It's written expecting a fully constructed domain -
>>>> which is fine because that's how it is used.  It doesn't cope safely
>>>> with an partially constructed domain.
>>>
>>> It is not 100% clear what is the issue you are referring to as the
>>> VMID is valid at this point. So what part would be wrong?
>>
>> Falling over a bad root pointer from an early construction exit.
> 
> You have been mentioning that several time now but I can't see how this
> can happen. If you look at Henry's second patch, p2m_teardown() starts
> with the following check:
> if ( page_list_empty(&p2m->pages) )
>     return;
> 
> Per the logic in p2m_init(), the root pages have to be allocated (note 
> they are *not* allocated from the P2M pool) and the VMID as well before 
> any pages could be added in the list.
> 
>>
>>> But if there are part of p2m_teardown() that are not safe for
>>> partially constructed domain, then we should split the code. This
>>> would be much better that the duplication you are proposing.
>>
>> You have two totally different contexts with different safety
>> requirements.  c/s 55914f7fc9 is a reasonably good and clean separation
>> between preemptible and non-preemptible cleanup[1].
> 
> The part you mention in [1] was decided to be delayed post 4.17 for 
> development cycle reasons.
> 
>>
>> You've agreed that the introduction of the non-preemptible path to the
>> preemptible path is a hack and layering violation, and will need undoing
>> later.  Others have raised this concern too.
> 
> [...]
> 
>>
>> Also realise that you've now split the helper between regular hypercall
>> context, and RCU context, and recall what happened when we finally
>> started asserting that memory couldn't be allocated in stop-machine context.
>>
>> How certain are you that the safety is the same on earlier versions of
>> Xen?
> I am pretty confident because the P2M code has not changed a lot.
> 
>> What is the likelihood that all of these actions will remain safe
>> given future development?
> Code always evolve and neither you (nor I) can claim that any work will 
> stay safe forever. In your patch proposal, then the risk is a bug could 
> be duplicated.
> 
>>
>>
>> Despite what is being claimed, the attempt to share cleanup logic is
>> introducing fragility and risk, not removing it.
> 
> I find interesting you are saying that... If we were going to move 
> p2m_teardown() in domain_teardown() then we would end up to share the code.
> 
> To me, this is not very different here because in one context it would 
> be preemptible while the other it won't. At which point...
> 
>>   This is a bugfix for
>> to a security fix issue which is totally dead on arrival; net safety,
>> especially in older versions of the Xen, is *the highest priority*.
>>
>> These two different contexts don't share any common properties of how to
>> clean up the pool, save freeing the frames back to the memory
>> allocator.  In a proper design, this is the hint that they shouldn't
>> share logic either
> ... why is your design better than what Henry's proposed?
> 
>>
>> Given that you do expect someone to spend yet-more time&effort to undo
>> the short term hack currently being proposed, how do you envisage the
>> end result looking?
> 
> The end result will be p2m_teardown() & co to be called from 
> domain_teardown().
> 
> Anyway, this discussion doesn't make us closer to come to an agreement 
> on the correct approach. We have both very diverging opinion and so far 
> I haven't seen any strong reasons that is showing yours is better.
> 
> So unless Bertrand or Stefano agree with you, then I will go ahead and 
> merge Henry's patch tomorrow after a final review.

While Andrew makes several points worth considering, I agree here that
staging needs unbreaking rather sooner than later. I'm inclined to say
the patches want committing now, unless an actual bug was still known to
be present there (which I can't deduce from Andrew's reply). In the
absence of actual bugs it really should be the maintainers to have the
final say when there are multiple ways of carrying out certain
functionality.

Jan