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

From: Sergej Proskurin <proskurin@sec.in.tum.de>
To: Julien Grall <julien.grall@arm.com>, xen-devel@lists.xenproject.org
Cc: Stefano Stabellini <sstabellini@kernel.org>
Subject: Re: [PATCH 04/18] arm/altp2m: Add altp2m init/teardown routines.
Date: Mon, 4 Jul 2016 23:56:10 +0200	[thread overview]
Message-ID: <22b5527e-782a-594a-a6ab-aeaa58a2dbe3@sec.in.tum.de> (raw)
In-Reply-To: <577AAB4D.3090509@arm.com>

On 07/04/2016 08:30 PM, Julien Grall wrote:
> 
> 
> 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.
> 
> [...]
> 

Ok. I will adapt the code as discussed. Thank you.

>>>> +        {
>>>> +            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.
> 
> [...]
> 

Makes sense. Especially that we have now discussed the fact that we
really need to allocate a new VMID per altp2m view. I will rewrite the
functionality and remove the duplicated code. Thank you.

>>>> +    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.
> 
> [...]
> 

Ok.

>>>> +
>>>>    #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.
> 

It's all good: Thank you for the great review :)

> Regards,
> 

Best regards,
Sergej

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