Re: [Xen-devel] [PATCH v2 08/25] arm/altp2m: Add HVMOP_altp2m_set_domain_state.

[Julien Grall <julien.grall@xxxxxxx>]

Hello Sergej,

On 06/08/2016 11:36, Sergej Proskurin wrote:
> > > +
> > > +    /* Initialize the new altp2m view. */
> > > +    rc = p2m_init_one(d, p2m);
> > > +    if ( rc )
> > > +        goto err;
> > > +
> > > +    /* Allocate a root table for the altp2m view. */
> > > +    rc = p2m_alloc_table(p2m);
> > > +    if ( rc )
> > > +        goto err;
> > > +
> > > +    p2m->p2m_class = p2m_alternate;
> > > +    p2m->access_required = 1;
> > Please use true here. Although, I am not sure why you want to enable
> > the access by default.
> Will do.
>
> p2m->access_required is true by default in the x86 implementation. Also,
> there is currently no way to manually set access_required on altp2m.
> Besides, I do not see a scenario, where it makes sense to run altp2m
> without access_required set to true.
Please add a comment in the code to explain it.

[...]

> > > +
> > > +            /*
> > > +             * The altp2m_active state has been deactivated. It is
> > > now safe to
> > > +             * flush all altp2m views -- including altp2m[0].
> > > +             */
> > > +            if ( ostate )
> > > +                altp2m_flush(d);
> > The function altp2m_flush is defined afterwards (in patch #9). Please
> > make sure that all the patches compile one by one.
> The patches compile one by one. Please note that there is an
> altp2m_flush stub inside of this patch.
>
> +/* Flush all the alternate p2m's for a domain */
> +static inline void altp2m_flush(struct domain *d)
> +{
> +    /* Not yet implemented. */
> +}
I don't want to see stubs that are been replaced later on within the 
same series. The patch #9 does not seem to depend on patch #8, so I 
don't see any reason why you can't swap the 2 patches.
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel