Re: [Xen-devel] [PATCH 04/18] arm/altp2m: Add altp2m init/teardown routines.

[Julien Grall <julien.grall@xxxxxxx>]

On 04/07/16 17:40, Sergej Proskurin wrote:
> On 07/04/2016 05:17 PM, Julien Grall wrote:
> > On 04/07/16 12:45, Sergej Proskurin wrote:
[...]

> > > +static struct p2m_domain *p2m_init_one(struct domain *d)
> > > +{
> > > +    struct p2m_domain *p2m = xzalloc(struct p2m_domain);
> > > +
> > > +    if ( !p2m )
> > > +        return NULL;
> > > +
> > > +    if ( p2m_initialise(d, p2m) )
> > > +        goto free_p2m;
> > > +
> > > +    return p2m;
> > > +
> > > +free_p2m:
> > > +    xfree(p2m);
> > > +    return NULL;
> > > +}
> > > +
> > > +static void p2m_teardown_hostp2m(struct domain *d)
> > Why does p2m_teardown_hostp2m not use p2m_teardown_one to teardown the
> > p2m? Assuming xfree(p2m) is move out of the function.
> I believe you mean p2m_free_one: The p2m_teardown_hostp2m might use the
> same function but would require the call p2m_free_vmid(d) to be called
> outside of p2m_free_one as well. This would require another acquisition
> of the p2m->lock. Just to be sure, I did not want to split the teardown
> process into two atomic executions. If you believe that it is safe to
> do, I will gladly change the code and re-use the code from p2m_free_one
> in p2m_teardown_hostp2m.
Looking at the caller of p2m_teardown, I don't think the lock is 
necessary because nobody can use the P2M anymore when this function is 
called.
[...]

> > > +        {
> > > +            mfn = _mfn(page_to_mfn(pg));
> > > +            clear_domain_page(mfn);
> > > +            free_domheap_page(pg);
> > > +        }
> > >
> > > +    for ( i = 0; i < P2M_ROOT_PAGES; i++ )
> > > +    {
> > > +        mfn = _mfn(page_to_mfn(p2m->root) + i);
> > > +        clear_domain_page(mfn);
> > > +    }
> > > +    free_domheap_pages(p2m->root, P2M_ROOT_ORDER);
> > >        p2m->root = NULL;
> > >
> > >        p2m_free_vmid(d);
> > > @@ -1422,7 +1506,7 @@ void p2m_teardown(struct domain *d)
> > >        spin_unlock(&p2m->lock);
> > >    }
> > >
> > > -int p2m_init(struct domain *d)
> > > +static int p2m_init_hostp2m(struct domain *d)
> > Why does p2m_init_hostp2m not use p2m_init_one to initialize the p2m?
> We dynamically allocate altp2ms. Also, the initialization of both the
> altp2ms and hostp2m slightly differs (see VMID allocation). I could
> rewrite the initialization function to be used for both, the hostp2m and
> altp2m structs. Especially, if you say that we do not associate domains
> with VMIDs (see your upper question).
I always prefer to see the code rewritten rather than duplicating code. 
The latter makes harder to fix bug when it is spread in multiple place.
[...]

> > > +    uint64_t altp2m_vttbr[MAX_ALTP2M];
> > >    }  __cacheline_aligned;
> > >
> > >    struct arch_vcpu
> > > diff --git a/xen/include/asm-arm/hvm/hvm.h
> > > b/xen/include/asm-arm/hvm/hvm.h
> > > index 96c455c..28d5298 100644
> > > --- a/xen/include/asm-arm/hvm/hvm.h
> > > +++ b/xen/include/asm-arm/hvm/hvm.h
> > > @@ -19,6 +19,18 @@
> > >    #ifndef __ASM_ARM_HVM_HVM_H__
> > >    #define __ASM_ARM_HVM_HVM_H__
> > >
> > > +struct vttbr_data {
> > This structure should not be part of hvm.h but processor.h. Also, I
> > would rename it to simply vttbr.
> Ok, I will move it. The struct was named this way to be the counterpart
> to struct ept_data. Do you still think, we should introduce naming
> differences for basically the same register at this point?
Yes, we are talking about two distinct architectures. If you look at 
ept_data, it stores more than the page table register. Hence the name.
[...]

> > > +
> > >    #define paddr_bits PADDR_BITS
> > >
> > >    /* Holds the bit size of IPAs in p2m tables.  */
> > > @@ -17,6 +20,11 @@ struct domain;
> > >
> > >    extern void memory_type_changed(struct domain *);
> > >
> > > +typedef enum {
> > > +    p2m_host,
> > > +    p2m_alternate,
> > > +} p2m_class_t;
> > > +
> > >    /* Per-p2m-table state */
> > >    struct p2m_domain {
> > >        /* Lock that protects updates to the p2m */
> > > @@ -66,6 +74,18 @@ struct p2m_domain {
> > >        /* Radix tree to store the p2m_access_t settings as the pte's
> > > don't have
> > >         * enough available bits to store this information. */
> > >        struct radix_tree_root mem_access_settings;
> > > +
> > > +    /* Alternate p2m: count of vcpu's currently using this p2m. */
> > > +    atomic_t active_vcpus;
> > > +
> > > +    /* Choose between: host/alternate */
> > > +    p2m_class_t p2m_class;
> > Is there any reason to have this field? It is set but never used.
> Actually it is used by p2m_is_altp2m and p2m_is_hostp2m (e.g. see assert
> in p2m_flush_table).
Right. Sorry, I didn't spot this call.

Regards,

--
Julien Grall

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