|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 12/18] arm/altp2m: Cosmetic fixes - function prototypes.
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@xxxxxxxxxxxxx>
>> ---
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |