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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.01.2020 13:06, Hongyan Xia wrote:
> @@ -4992,22 +4993,55 @@ int mmcfg_intercept_write(
>  }
>  
>  void *alloc_xen_pagetable(void)
> +{
> +    mfn_t mfn = alloc_xen_pagetable_new();
> +
> +    return mfn_eq(mfn, INVALID_MFN) ? NULL : mfn_to_virt(mfn_x(mfn));
> +}

Isn't it more dangerous to do the mapping this way round than the
opposite (new calls old)? Done the opposite way the new functions
could be switched to their new implementations ahead of the
removal of the old ones, and - if suitably isolated - perhaps
some of their users. Anyway, perhaps more a theoretical remark.

> +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));
}

> +/* v can point to an entry within a table or be NULL */
> +void unmap_xen_pagetable(const void *v)

Why "entry" in the comment?

Jan

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