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

[Julien Grall <julien.grall@arm.com>]

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@lists.xen.org
https://lists.xen.org/xen-devel