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

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

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?

>
> 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;
>


Regards,

-- 
Julien Grall

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