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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 19 Oct 2022, at 23:33, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Andrew,
> 
> 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.

At this stage, I still do not get the NULL pointer case as from my point of 
view the list_empty done at the beginning is making that case impossible.
We have a currently blocked status where any GICv2 based board is not booting 
and we all know that Henry’s solution will need to be reworked post 4.17.

So I will give my reviewed-by on Henry’s patch so that we have a short term 
solution giving us more time to discuss and find a proper solution.

Cheers
Bertrand


> 
> Cheers,
> 
> -- 
> Julien Grall