Re: [PATCH v2 4/4] xen/arm: Clean-up in p2m_init() and p2m_final_teardown()

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Henry,

On 22/03/2023 03:21, Henry Wang wrote:
> 
> 
> Hi Julien,
> 
>> -----Original Message-----
>> From: Julien Grall <julien@xxxxxxx>
>> Subject: Re: [PATCH v2 4/4] xen/arm: Clean-up in p2m_init() and
>> p2m_final_teardown()
>>
>> Hi Henry,
>>
>>> ---
>>> I am not entirely sure if I should also drop the "TODO" on top of
>>> the p2m_set_entry(). Because although we are sure there is no
>>> p2m pages populated in domain_create() stage now, but we are not
>>> sure if anyone will add more in the future...Any comments?
>>
>> I would keep it.
> 
> Sure.
> 
>>
>>> @@ -200,10 +200,6 @@ int p2m_init(struct domain *d);
>>>    *  - p2m_final_teardown() will be called when domain struct is been
>>>    *    freed. This *cannot* be preempted and therefore one small
>>
>> NIT: As you are touching the comment, would you mind to fix the typo
>> s/one/only/.
> 
> I would be more than happy to fix it. Thanks for noticing this :)
> 
>>
>>> -    BUG_ON(p2m_teardown(d, false));
>>
>> With this change, I think we can also drop the second parameter of
>> p2m_teardown(). Preferably with this change in the patch:
> 
> Yes indeed, I was also thinking of this when writing this patch but in
> the end decided to do minimal change..
> 
>>
>> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
> 
> Thanks! I am not 100% comfortable to take this tag because I made
> some extra code change, below is the diff to drop the second param
> of p2m_teardown():
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> @@ -1030,7 +1030,7 @@ int domain_relinquish_resources(struct domain *d)
>          p2m_clear_root_pages(&d->arch.p2m);
> 
>      PROGRESS(p2m):
> -        ret = p2m_teardown(d, true);
> +        ret = p2m_teardown(d);
>          if ( ret )
>              return ret;
> 
> diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
> @@ -201,7 +201,7 @@ int p2m_init(struct domain *d);
>   *    freed. This *cannot* be preempted and therefore only small
>   *    resources should be freed here.
>   */
> -int p2m_teardown(struct domain *d, bool allow_preemption);
> +int p2m_teardown(struct domain *d);
>  void p2m_final_teardown(struct domain *d);
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> -int p2m_teardown(struct domain *d, bool allow_preemption)
> +int p2m_teardown(struct domain *d)
>  {
> [...]
>          /* Arbitrarily preempt every 512 iterations */
> -        if ( allow_preemption && !(count % 512) && hypercall_preempt_check() 
> )
> +        if ( !(count % 512) && hypercall_preempt_check() )
> 
> If you are happy, I will take this acked-by. Same question to Michal for his
> Reviewed-by.
The diff looks good to me and surely you can keep my tag.
However, we might want to modify the comment on top of p2m_teardown() prototype 
as
it assumes presence of preemptive/non-preemptive p2m_teardown() call (at least 
this
is how I understand this).

~Michal