Re: [Xen-devel] [PATCH 15/22] xen/arm: Don't call p2m_alloc_table from arch_domain_create

[Julien Grall <julien.grall@xxxxxxx>]

On 22/07/16 12:05, Sergej Proskurin wrote:
> On 07/22/2016 12:38 PM, Julien Grall wrote:
> > On 22/07/16 11:39, Sergej Proskurin wrote:
> > > On 07/22/2016 12:26 PM, Julien Grall wrote:
> > > > On 22/07/16 11:16, Sergej Proskurin wrote:
> > > > > Hi Julien,
> > > > Hello,
> > > >
> > > > > On 07/22/2016 11:18 AM, Julien Grall wrote:
> > > > > > On 22/07/16 09:32, Sergej Proskurin wrote:
> > > > > > > Hi Julien,
> > > > > > Hello Sergej,
> > > > > >
> > > > > > > > -int p2m_alloc_table(struct domain *d)
> > > > > > > > +static int p2m_alloc_table(struct domain *d)
> > > > > > > While moving parts of the altp2m code out of ./xen/arch/arm/p2m.c,
> > > > > > > the
> > > > > > > function p2m_alloc_table needs to be called from
> > > > > > > ./xen/arch/arm/altp2m.c
> > > > > > > to allocate the individual altp2m views. Hence it should not be
> > > > > > > static.
> > > > > > No, this function should not be called outside p2m.c as it will not
> > > > > > fully initialize the p2m. You need to need to provide a function to
> > > > > > initialize a p2m (such as p2m_init).
> > > > > The last time we have discussed reusing existing code, among others,
> > > > > for
> > > > > individual struct p2m_domain initialization routines. Also, we have
> > > > > agreed to move altp2m-related parts out of p2m.c into altp2m.c, which
> > > > > makes it hard not to access parts required for initialization/teardown
> > > > > (that are equal for both p2m and altp2m).
> > > > I remember this discussion. However, the p2m initialize/teardown should
> > > > exactly be the same for the hostp2m and altp2m (except for the type of
> > > > the p2m). So, a function should be provided to initialize a full p2m to
> > > > avoid code duplication.
> > > This is exactly what has been done. Nevertheless, altp2m views are
> > > somewhat more dynamic than hostp2m and hence require frequent
> > > initialization/teardown of individual views. That is, by moving altp2m
> > > parts out of p2m.c we simply need to access this shared code multiple
> > > times at runtime from different altp2m-related functions. This applies
> > > to more functions that just to p2m_alloc_table.
> > I am not convinced that you need to reallocate the root page every time
> > rather than clearing them. Anyway, I will need to see the code to
> > understand what is done.
> >
> > Regards,
> In the following, you will find an excerpt of both the p2m.c and
> altp2m.c concerning the (alt)p2m initialization. Please note that
> altp2m_init_helper is called from different initialization routines in
> altp2m.c. Also, please consider that this code has not yet been ported
> to your recent patches. Please let me know if you need more information.
> static int altp2m_init_helper(struct domain *d, unsigned int idx)
> {
>     int rc;
>     struct p2m_domain *p2m = d->arch.altp2m_p2m[idx];
>
>     if ( p2m == NULL )
>     {
>         /* Allocate a new altp2m view. */
>         p2m = xzalloc(struct p2m_domain);
>         if ( p2m == NULL)
>         {
>             rc = -ENOMEM;
>             goto err;
>         }
>         memset(p2m, 0, sizeof(struct p2m_domain));
>     }
>
>     /* 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);
This patch moved p2m_alloc_table call into p2m_init (i.e your 
p2m_init_one). You complained that the function was not exported 
anymore, but you did not look how it was called in this patch.
>     if ( rc )
>         goto err;
>
>     p2m->p2m_class = p2m_alternate;
>     p2m->access_required = 1;
>     _atomic_set(&p2m->active_vcpus, 0);
>
>     d->arch.altp2m_p2m[idx] = p2m;
>     d->arch.altp2m_vttbr[idx] = p2m->vttbr.vttbr;
Regards,

--
Julien Grall

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