Re: [Xen-devel] [PATCH v5 2/7] x86: introduce a new set of APIs to manage Xen page tables

[Jan Beulich <jbeulich@suse.com>]

On 27.01.2020 15:33, Xia, Hongyan wrote:
> On Thu, 2020-01-16 at 13:04 +0100, Jan Beulich wrote:
>> ...
>>
>>> +void free_xen_pagetable(void *v)
>>> +{
>>> +    mfn_t mfn = v ? virt_to_mfn(v) : INVALID_MFN;
>>> +
>>> +    if ( system_state != SYS_STATE_early_boot )
>>> +        free_xen_pagetable_new(mfn);
>>
>> The condition is (partly) redundant with what
>> free_xen_pagetable_new() does. Therefore I'd like to ask that
>> either the if() be dropped here, or be completed by also
>> checking v to be non-NULL, at which point this would likely
>> become just
>>
>> void free_xen_pagetable(void *v)
>> {
>>     if ( v && system_state != SYS_STATE_early_boot )
>>         free_xen_pagetable_new(virt_to_mfn(v));
>> }
> 
> You are right. Will change in the next revision.
> 
>>> +/* v can point to an entry within a table or be NULL */
>>> +void unmap_xen_pagetable(const void *v)
>>
>> Why "entry" in the comment?
> 
> I think the comment originally meant pointing to the entry in its
> parent table, which then meant pointing to this table. It's a bit
> confusing. Will reword.
> 
> Reflecting back to your comment in v3 about whether the new Xen page
> table mapping APIs (map/unmap_xen_pagetable) are really necessary, I
> agree in the end they will just do the same thing as
> map/unmap_domain_page, although one thing is that the latter suggests
> it is trying to map a `domain' page, whose definition probably does not
> include Xen page tables, so the name could be a bit confusing (well, we
> could argue that Xen page tables are just idle `domain' pages so the
> name still holds). If we are happy with using map_domain_page on Xen
> PTE tables then I am okay with dropping the new mapping APIs and only
> include the new alloc APIs.

Anyone on the To: list - opinions?

Thanks, Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel