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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.10.2019 19:16, Hongyan Xia wrote:
> @@ -4847,22 +4848,50 @@ int mmcfg_intercept_write(
>  }
>  
>  void *alloc_xen_pagetable(void)
> +{
> +    mfn_t mfn;
> +
> +    mfn = alloc_xen_pagetable_new();
> +    ASSERT(!mfn_eq(mfn, INVALID_MFN));
> +
> +    return map_xen_pagetable_new(mfn);
> +}
> +
> +void free_xen_pagetable(void *v)
> +{
> +    if ( system_state != SYS_STATE_early_boot )
> +        free_xen_pagetable_new(virt_to_mfn(v));
> +}
> +
> +mfn_t alloc_xen_pagetable_new(void)
>  {
>      if ( system_state != SYS_STATE_early_boot )
>      {
>          void *ptr = alloc_xenheap_page();
>  
>          BUG_ON(!hardware_domain && !ptr);
> -        return ptr;
> +        return virt_to_mfn(ptr);
>      }
>  
> -    return mfn_to_virt(mfn_x(alloc_boot_pages(1, 1)));
> +    return alloc_boot_pages(1, 1);
>  }
>  
> -void free_xen_pagetable(void *v)
> +void *map_xen_pagetable_new(mfn_t mfn)

There's no need for the map/unmap functions to have a _new
suffix, is there?

>  {
> -    if ( system_state != SYS_STATE_early_boot )
> -        free_xenheap_page(v);
> +    return mfn_to_virt(mfn_x(mfn));
> +}
> +
> +/* v can point to an entry within a table or be NULL */
> +void unmap_xen_pagetable_new(void *v)

Can this please take const void *, such that callers needing
mappings just for read purposes can have their pointer const-
qualified as well?

> +{
> +    /* XXX still using xenheap page, no need to do anything.  */

I wonder if it wouldn't be a good idea to at least have some
leak detection here temporarily, such that we have a chance of
noticing paths not properly doing the necessary unmapping.

The again a question is why you introduce such a map/unmap pair
in the first place. This is going to be a thin wrapper around
{,un}map_domain_page() in the end anyway, and hence callers
could as well be switched to calling those function directly,
as they're no-ops on Xen heap pages.

> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -633,6 +633,17 @@ int arch_acquire_resource(struct domain *d, unsigned int 
> type,
>  /* Allocator functions for Xen pagetables. */
>  void *alloc_xen_pagetable(void);
>  void free_xen_pagetable(void *v);
> +mfn_t alloc_xen_pagetable_new(void);
> +void *map_xen_pagetable_new(mfn_t mfn);
> +void unmap_xen_pagetable_new(void *v);
> +void free_xen_pagetable_new(mfn_t mfn);
> +
> +#define UNMAP_XEN_PAGETABLE_NEW(ptr)    \
> +    do {                                \
> +        unmap_xen_pagetable_new((ptr)); \

Stray double parentheses.

Jan

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