Re: [PATCH 12/18] arm/altp2m: Cosmetic fixes - function prototypes.

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>,
	Tamas K Lengyel <tamas@tklengyel.com>
Subject: Re: [PATCH 12/18] arm/altp2m: Cosmetic fixes - function prototypes.
Date: Sat, 16 Jul 2016 17:18:47 +0200	[thread overview]
Message-ID: <9bfbe2a1-464e-991b-db5d-ce84709ba860@sec.in.tum.de> (raw)
In-Reply-To: <5788E8F4.4030504@arm.com>

Hi Julien,

On 07/15/2016 03:45 PM, Julien Grall wrote:
> Hi Sergej,
> 
> On 04/07/16 12:45, Sergej Proskurin wrote:
>> This commit changes the prototype of the following functions:
>> - apply_p2m_changes
>> - apply_one_level
>> - p2m_shatter_page
>> - p2m_create_table
>> - __p2m_lookup
>> - __p2m_get_mem_access
>>
>> These changes are required as our implementation reuses most of the
>> existing ARM p2m implementation to set page table attributes of the
>> individual altp2m views. Therefore, exiting function prototypes have
>> been extended to hold another argument (of type struct p2m_domain *).
>> This allows to specify the p2m/altp2m domain that should be processed by
>> the individual function -- instead of accessing the host's default p2m
>> domain.
> 
> I am actually reworking the whole p2m code to be complain with the ARM
> architecture (such as break-before-make) and make easier to implement
> new features such as altp2m.
> 
> Would it be possible to send this patch separately with nothing altp2m
> related in it?
> 

I will look into that, thank you for asking. The current implementation
already provides more cosmetic fixes, therefore I need to shortly assess
which parts might be submitted independently of altp2m. I will respond
as quickly as possible.

>>
>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>> ---
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/p2m.c | 80
>> +++++++++++++++++++++++++++++-------------------------
>>   1 file changed, 43 insertions(+), 37 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 019f10e..9c8fefd 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -200,9 +200,8 @@ void flush_tlb_domain(struct domain *d)
>>    * There are no processor functions to do a stage 2 only lookup
>> therefore we
>>    * do a a software walk.
>>    */
>> -static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr,
>> p2m_type_t *t)
>> +static paddr_t __p2m_lookup(struct p2m_domain *p2m, paddr_t paddr,
>> p2m_type_t *t)
>>   {
>> -    struct p2m_domain *p2m = &d->arch.p2m;
>>       const unsigned int offsets[4] = {
>>           zeroeth_table_offset(paddr),
>>           first_table_offset(paddr),
>> @@ -282,10 +281,11 @@ err:
>>   paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
>>   {
>>       paddr_t ret;
>> -    struct p2m_domain *p2m = &d->arch.p2m;
>> +    struct vcpu *v = current;
>> +    struct p2m_domain *p2m = altp2m_active(d) ? p2m_get_altp2m(v) :
>> p2m_get_hostp2m(d);
> 
> This change is wrong, this function is called in hypercalls to translate
> an IPA for another domain to an MFN. So current->domain != d.
> 
>>
>>       spin_lock(&p2m->lock);
>> -    ret = __p2m_lookup(d, paddr, t);
>> +    ret = __p2m_lookup(p2m, paddr, t);
>>       spin_unlock(&p2m->lock);
>>
>>       return ret;
>> @@ -441,10 +441,12 @@ static inline void p2m_remove_pte(lpae_t *p,
>> bool_t flush_cache)
>>    *
>>    * level_shift is the number of bits at the level we want to create.
>>    */
>> -static int p2m_create_table(struct domain *d, lpae_t *entry,
>> -                            int level_shift, bool_t flush_cache)
>> +static int p2m_create_table(struct domain *d,
> 
> Please drop "struct domain *d", it was only here to get the p2m.
> 
>> +                            struct p2m_domain *p2m,
>> +                            lpae_t *entry,
>> +                            int level_shift,
>> +                            bool_t flush_cache)
>>   {
>> -    struct p2m_domain *p2m = &d->arch.p2m;
>>       struct page_info *page;
>>       lpae_t *p;
>>       lpae_t pte;
>> @@ -502,10 +504,9 @@ static int p2m_create_table(struct domain *d,
>> lpae_t *entry,
>>       return 0;
>>   }
>>
>> -static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
>> +static int __p2m_get_mem_access(struct p2m_domain *p2m, gfn_t gfn,
>>                                   xenmem_access_t *access)
>>   {
>> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>       void *i;
>>       unsigned int index;
>>
>> @@ -548,7 +549,7 @@ static int __p2m_get_mem_access(struct domain *d,
>> gfn_t gfn,
>>            * No setting was found in the Radix tree. Check if the
>>            * entry exists in the page-tables.
>>            */
>> -        paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
>> +        paddr_t maddr = __p2m_lookup(p2m, gfn_x(gfn) << PAGE_SHIFT,
>> NULL);
>>           if ( INVALID_PADDR == maddr )
>>               return -ESRCH;
>>
>> @@ -677,17 +678,17 @@ static const paddr_t level_shifts[] =
>>       { ZEROETH_SHIFT, FIRST_SHIFT, SECOND_SHIFT, THIRD_SHIFT };
>>
>>   static int p2m_shatter_page(struct domain *d,
> 
> Ditto.
> 
>> +                            struct p2m_domain *p2m,
>>                               lpae_t *entry,
>>                               unsigned int level,
>>                               bool_t flush_cache)
>>   {
>>       const paddr_t level_shift = level_shifts[level];
>> -    int rc = p2m_create_table(d, entry,
>> +    int rc = p2m_create_table(d, p2m, entry,
>>                                 level_shift - PAGE_SHIFT, flush_cache);
>>
>>       if ( !rc )
>>       {
>> -        struct p2m_domain *p2m = &d->arch.p2m;
>>           p2m->stats.shattered[level]++;
>>           p2m->stats.mappings[level]--;
>>           p2m->stats.mappings[level+1] += LPAE_ENTRIES;
>> @@ -704,6 +705,7 @@ static int p2m_shatter_page(struct domain *d,
>>    * -ve == (-Exxx) error.
>>    */
>>   static int apply_one_level(struct domain *d,
> 
> Ditto.
> 
>> +                           struct p2m_domain *p2m,
>>                              lpae_t *entry,
>>                              unsigned int level,
>>                              bool_t flush_cache,
>> @@ -721,7 +723,6 @@ static int apply_one_level(struct domain *d,
>>       const paddr_t level_mask = level_masks[level];
>>       const paddr_t level_shift = level_shifts[level];
>>
>> -    struct p2m_domain *p2m = &d->arch.p2m;
>>       lpae_t pte;
>>       const lpae_t orig_pte = *entry;
>>       int rc;
> 
> [...]
> 
>> @@ -1011,6 +1012,7 @@ static void update_reference_mapping(struct
>> page_info *page,
>>   }
>>
>>   static int apply_p2m_changes(struct domain *d,
> 
> I would like to see "struct domain *d" dropped completely. If we really
> need it, we could introduce a back pointer to domain.
> 
>> +                     struct p2m_domain *p2m,
>>                        enum p2m_operation op,
>>                        paddr_t start_gpaddr,
>>                        paddr_t end_gpaddr,
> 
> [...]
> 
>> @@ -1743,6 +1745,9 @@ p2m_mem_access_check_and_get_page(vaddr_t gva,
>> unsigned long flag)
>>       xenmem_access_t xma;
>>       p2m_type_t t;
>>       struct page_info *page = NULL;
>> +    struct vcpu *v = current;
>> +    struct domain *d = v->domain;
>> +    struct p2m_domain *p2m = altp2m_active(d) ? p2m_get_altp2m(v) :
>> p2m_get_hostp2m(d);
> 
> This patch is supposed to be "comestic fixes", so this change does not
> belong to this patch.
> 
> However, the changes within this function look wrong to me because
> p2m_mem_access_check_and_get_page is used by Xen to get the page when
> copying data to/from the guest. If the entry is not yet replicated in
> the altp2m, you will fail to copy the data.
> 
> You may also want to consider how get_page_from_gva would work if an
> altp2m is used.
> 
>>
>>       rc = gva_to_ipa(gva, &ipa, flag);
> 
> This is not related to this patch, but I think gva_to_ipa can fail to
> translate a VA to an IPA if the stage-1 page table reside in memory
> which was restricted by memaccess.
> 
>>       if ( rc < 0 )
>> @@ -1752,7 +1757,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva,
>> unsigned long flag)
>>        * We do this first as this is faster in the default case when no
>>        * permission is set on the page.
>>        */
>> -    rc = __p2m_get_mem_access(current->domain,
>> _gfn(paddr_to_pfn(ipa)), &xma);
>> +    rc = __p2m_get_mem_access(p2m, _gfn(paddr_to_pfn(ipa)), &xma);
>>       if ( rc < 0 )
>>           goto err;
>>
>> @@ -1801,7 +1806,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva,
>> unsigned long flag)
>>        * We had a mem_access permission limiting the access, but the
>> page type
>>        * could also be limiting, so we need to check that as well.
>>        */
>> -    maddr = __p2m_lookup(current->domain, ipa, &t);
>> +    maddr = __p2m_lookup(p2m, ipa, &t);
>>       if ( maddr == INVALID_PADDR )
>>           goto err;
>>
>> @@ -2125,7 +2130,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t
>> gfn, uint32_t nr,
>>           return 0;
>>       }
>>
>> -    rc = apply_p2m_changes(d, MEMACCESS,
>> +    rc = apply_p2m_changes(d, p2m, MEMACCESS,
>>                              pfn_to_paddr(gfn_x(gfn) + start),
>>                              pfn_to_paddr(gfn_x(gfn) + nr),
>>                              0, MATTR_MEM, mask, 0, a);
>> @@ -2141,10 +2146,11 @@ int p2m_get_mem_access(struct domain *d, gfn_t
>> gfn,
>>                          xenmem_access_t *access)
>>   {
>>       int ret;
>> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> +    struct vcpu *v = current;
>> +    struct p2m_domain *p2m = altp2m_active(d) ? p2m_get_altp2m(v) :
>> p2m_get_hostp2m(d);
> 
> Same remark as above, this patch is only "comestic" and functional
> changes does not belong to this patch.
> 
>>
>>       spin_lock(&p2m->lock);
>> -    ret = __p2m_get_mem_access(d, gfn, access);
>> +    ret = __p2m_get_mem_access(p2m, gfn, access);
>>       spin_unlock(&p2m->lock);
>>
>>       return ret;
>>

Best regards,
~Sergej

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