Re: [Xen-devel] [PATCH v2 14/25] arm/altp2m: Make get_page_from_gva ready for altp2m.

[Julien Grall <julien.grall@xxxxxxx>]

Hello Sergej,

On 01/08/16 18:10, Sergej Proskurin wrote:
> The function get_page_from_gva uses ARM's hardware support to translate
> gva's to machine addresses. This function is used, among others, for
> memory regulation purposes, e.g, within the context of memory ballooning.
> To ensure correct behavior while altp2m is in use, we use the host's p2m
> table for the associated gva to ma translation. This is required at this
> point, as altp2m lazily copies pages from the host's p2m and even might
> be flushed because of changes to the host's p2m (as it is done within
> the context of memory ballooning).
I was expecting to see some change in p2m_mem_access_check_and_get_page. 
Is there any reason to not fix it?

> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
> ---
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/arm/p2m.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index bcad51f..784f8da 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1614,7 +1614,7 @@ struct page_info *get_page_from_gva(struct vcpu *v, 
> vaddr_t va,
>                                      unsigned long flags)
>  {
>      struct domain *d = v->domain;
> -    struct p2m_domain *p2m = &d->arch.p2m;
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
This is more a clean-up than necessary. I would prefer to see a patch 
modifying all "&d->arch.p2m" by p2m_get_hostp2m in one go.
>      struct page_info *page = NULL;
>      paddr_t maddr = 0;
>      int rc;
> @@ -1628,7 +1628,34 @@ struct page_info *get_page_from_gva(struct vcpu *v, 
> vaddr_t va,
>
>      p2m_read_lock(p2m);
>
> -    rc = gvirt_to_maddr(va, &maddr, flags);
> +    /*
> +     * If altp2m is active, we still read the gva from the hostp2m, as it
s/still/need to/

> +     * contains all valid mappings while the currently active altp2m view might
> +     * not have the required gva mapping yet.
> +     */
> +    if ( unlikely(altp2m_active(d)) )
> +    {
> +        unsigned long irq_flags = 0;
> +        uint64_t ovttbr = READ_SYSREG64(VTTBR_EL2);
> +
> +        if ( ovttbr != p2m->vttbr.vttbr )
> +        {
> +            local_irq_save(irq_flags);
> +            WRITE_SYSREG64(p2m->vttbr.vttbr, VTTBR_EL2);
> +            isb();
> +        }
> +
> +        rc = gvirt_to_maddr(va, &maddr, flags);
> +
> +        if ( ovttbr != p2m->vttbr.vttbr )
> +        {
> +            WRITE_SYSREG64(ovttbr, VTTBR_EL2);
> +            isb();
> +            local_irq_restore(irq_flags);
> +        }
The pattern is very similar to what p2m_flush_tlb does. Can we get 
macro/helpers to avoid duplicate code?
> +    }
> +    else
> +        rc = gvirt_to_maddr(va, &maddr, flags);
>
>      if ( rc )
>          goto err;
Regards,

--
Julien Grall

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