Re: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of
> Julien Grall
> Sent: 04 February 2020 13:06
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: julien@xxxxxxx; Wei Liu <wl@xxxxxxx>; George Dunlap
> <george.dunlap@xxxxxxxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>;
> Grall, Julien <jgrall@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Roger
> Pau Monné <roger.pau@xxxxxxxxxx>
> Subject: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden
> hap_enable()
> 
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> Unlike shadow_enable(), hap_enable() can only be called once during
> domain creation and with the mode equal to mode equal to
> PG_external | PG_translate | PG_refcounts.
> 
> If it were called twice, then we might have something interesting
> problem as the p2m tables would be re-allocated (and therefore all the
> mappings would be lost).

There are two tests in p2m_alloc_tabl2: whether the domain has memory 
allocated, and whether the domain already has a p2m. Can these now be dropped?

  Paul

> 
> Add code to sanity check the mode and that the function is only called
> once. Take the opportunity to an if checking that PG_translate is set.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> 
> ---
> 
> It is not entirely clear when PG_translate was enforced.
> 
> I keep the check != 0 because this is consistent with the rest of the
> file. If we want to omit comparison against 0, then this should be in a
> separate patches converting the file.
> 
>     Changes in v2:
>         - Fix typoes in the commit message
>         - Use -EEXIST instead of -EINVAL
> ---
>  xen/arch/x86/mm/hap/hap.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index 31362a31b6..4974bd13d4 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -445,6 +445,13 @@ int hap_enable(struct domain *d, u32 mode)
>      unsigned int i;
>      int rv = 0;
> 
> +    if ( mode != (PG_external | PG_translate | PG_refcounts) )
> +        return -EINVAL;
> +
> +    /* The function can only be called once */
> +    if ( d->arch.paging.mode != 0 )
> +        return -EEXIST;
> +
>      domain_pause(d);
> 
>      old_pages = d->arch.paging.hap.total_pages;
> @@ -465,13 +472,10 @@ int hap_enable(struct domain *d, u32 mode)
>      d->arch.paging.alloc_page = hap_alloc_p2m_page;
>      d->arch.paging.free_page = hap_free_p2m_page;
> 
> -    /* allocate P2m table */
> -    if ( mode & PG_translate )
> -    {
> -        rv = p2m_alloc_table(p2m_get_hostp2m(d));
> -        if ( rv != 0 )
> -            goto out;
> -    }
> +    /* allocate P2M table */
> +    rv = p2m_alloc_table(p2m_get_hostp2m(d));
> +    if ( rv != 0 )
> +        goto out;
> 
>      for ( i = 0; i < MAX_NESTEDP2M; i++ )
>      {
> --
> 2.17.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel