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

[Julien Grall <julien@xxxxxxx>]

On 03/03/2020 13:25, Jan Beulich wrote:
> On 04.02.2020 14:06, Julien Grall wrote:
> > 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).
> >
> > 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>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> preferably with the duplicate words on the second line of the description
> dropped.
I will remove it on commit.

> > 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.
> I disagree, but not enough to make the ack dependent upon the adjustment.
>
> > --- 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;
> I think if such a comment gets added, it should be unambiguous. The
> function can be called once per domain, not just once in total.
I will replace with "The function can only be called one per domain".

Cheers,

--
Julien Grall

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