Re: [Xen-devel] [PATCH v2 04/25] arm/altp2m: Move hostp2m init/teardown to individual functions.

[Julien Grall <julien.grall@xxxxxxx>]

On 05/08/16 08:26, Sergej Proskurin wrote:
> Hi Julien,
Hello Sergej,

> On 08/03/2016 07:40 PM, Julien Grall wrote:
[...]

> > > +{
> > > +    p2m_flush_table(p2m);
> > > +
> > > +    /* Free VMID and reset VTTBR */
> > > +    p2m_free_vmid(p2m->domain);
> > Why do you move the call to p2m_free_vmid?
> When flushing a table, I did not want to free the associated VMID, as it
> would need to be allocated right afterwards (see altp2m_propagate_change
> and altp2m_reset). Since this would need to be done also in functions
> like p2m_altp2m_reset, I moved this call to p2m_free_one. I believe
> there is no need to free the VMIDs if the associated p2m is not freed as
> well.
That does not answer my question. You moved p2m_free_vmid within 
p2m_free_one. It used to be at the end of the function and now it is at 
the beginning. See...
> > > +    p2m->vttbr.vttbr = INVALID_VTTBR;
[...]

> > >      if ( p2m->root )
> > >          free_domheap_pages(p2m->root, P2M_ROOT_ORDER);
> > >
> > >      p2m->root = NULL;
> > >
> > > -    p2m_free_vmid(d);
> > > -
here. So please don't move code unless there is a good reason. This 
series is already quite difficult to read.
> > >      radix_tree_destroy(&p2m->mem_access_settings, NULL);
> > >  }
> > >
> > > -int p2m_init(struct domain *d)
> > > +int p2m_init_one(struct domain *d, struct p2m_domain *p2m)
> > Any function exported should have its prototype in an header within
> > the same patch.
> I will change that, thank you.
>
> > >  {
> > > -    struct p2m_domain *p2m = &d->arch.p2m;
> > >      int rc = 0;
> > >
> > >      rwlock_init(&p2m->lock);
> > >      INIT_PAGE_LIST_HEAD(&p2m->pages);
> > >
> > > -    p2m->vmid = INVALID_VMID;
> > > -
> > Why this is dropped?
> This will be shown in patch #07. We reuse altp2m views and check whether
> a p2m was flushed by checking for a valid VMID.
Which is wrong. A flushed altp2m should not need to be reinitialized 
(i.e reseting the lock...).
A flushed altp2m only need to have max_mapped_gfn and lowest_mapped_gfn 
reset.
[...]

> > > +    p2m->vttbr.vttbr = INVALID_VTTBR;
> > >      radix_tree_init(&p2m->mem_access_settings);
> > >
> > > -    rc = p2m_alloc_table(d);
> > > -
> > The function p2m_init_one should fully initialize the p2m (i.e
> > allocate the table).
> The function p2m_init_one is currently called also for initializing the
> hostp2m, which is not dynamically allocated. Since we are sharing code
> between the hostp2m and altp2m initialization, I solved it that way. We
> could always allocate the hostp2m dynamically, that would solve it quite
> easily without additional checks. The other solution can simply check,
> whether the p2m is NULL and perform additional p2m allocation. What do
> you think?
I don't understand your point here. It does not matter whether the 
hostp2m is allocated dynamically or not.
p2m_init_one should fully initialize a p2m and this does not depends on 
dynamic allocation.
[...]

> > > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> > > index 5c7cd1a..1f9c370 100644
> > > --- a/xen/include/asm-arm/p2m.h
> > > +++ b/xen/include/asm-arm/p2m.h
> > > @@ -18,6 +18,11 @@ struct domain;
> > >
> > >  extern void memory_type_changed(struct domain *);
> > >
> > > +typedef enum {
> > > +    p2m_host,
> > > +    p2m_alternate,
> > > +} p2m_class_t;
> > > +
> > This addition should really be in a separate patch.
> The function p2m_init_hostp2m uses p2m_host for initialization. I can
> introduce a patch before this one, however to make it easier for the
> reviewer.
You did not get my point here. A patch should do one thing: moving code 
or adding new code. Not both at the same.
Adding p2m_class_t and setting p2m_host should really be done in a 
separate patch. This will ease the review this patch series.
Regards,

--
Julien Grall

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