Re: [Xen-devel] [PATCH 11/18] arm/altp2m: Make flush_tlb_domain ready for altp2m.

[Julien Grall <julien.grall@xxxxxxx>]

Hello Sergej,

On 04/07/2016 12:45, Sergej Proskurin wrote:
> This commit makes sure that the TLB of a domain considers flushing all
> of the associated altp2m views. Therefore, in case a different domain
> (not the currently active domain) shall flush its TLBs, the current
> implementation loops over all VTTBRs of the different altp2m mappings
> per vCPU and flushes the TLBs. This way, a change of one of the altp2m
> mapping is considered.  At this point, it must be considered that the
> domain --whose TLBs are to be flushed-- is not locked.
>
> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
> ---
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/arm/p2m.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 63 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 7e721f9..019f10e 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -15,6 +15,8 @@
>  #include <asm/hardirq.h>
>  #include <asm/page.h>
>
> +#include <asm/altp2m.h>
> +
>  #ifdef CONFIG_ARM_64
>  static unsigned int __read_mostly p2m_root_order;
>  static unsigned int __read_mostly p2m_root_level;
> @@ -79,12 +81,41 @@ void dump_p2m_lookup(struct domain *d, paddr_t addr)
>                   P2M_ROOT_LEVEL, P2M_ROOT_PAGES);
>  }
>
> +static uint64_t p2m_get_altp2m_vttbr(struct vcpu *v)
> +{
> +    struct domain *d = v->domain;
> +    uint16_t index = vcpu_altp2m(v).p2midx;
> +
> +    if ( index == INVALID_ALTP2M )
> +        return INVALID_MFN;
> +
> +    BUG_ON(index >= MAX_ALTP2M);
> +
> +    return d->arch.altp2m_vttbr[index];
> +}
> +
> +static void p2m_load_altp2m_VTTBR(struct vcpu *v)
Please try to share code when it is possible. For instance, a big part 
of this helper is similar to p2m_load_VTTBR. Assuming you get the p2m 
rather than the VTTBR directly.
> +{
> +    struct domain *d = v->domain;
> +    uint64_t vttbr = p2m_get_altp2m_vttbr(v);
> +
> +    if ( is_idle_domain(d) )
> +        return;
> +
> +    BUG_ON(vttbr == INVALID_MFN);
> +    WRITE_SYSREG64(vttbr, VTTBR_EL2);
> +
> +    isb(); /* Ensure update is visible */
> +}
> +
>  static void p2m_load_VTTBR(struct domain *d)
>  {
>      if ( is_idle_domain(d) )
>          return;
> +
>      BUG_ON(!d->arch.vttbr);
>      WRITE_SYSREG64(d->arch.vttbr, VTTBR_EL2);
> +
Spurious changes.

>      isb(); /* Ensure update is visible */
>  }
>
> @@ -101,7 +132,11 @@ void p2m_restore_state(struct vcpu *n)
>      WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2);
>      isb();
>
> -    p2m_load_VTTBR(n->domain);
> +    if ( altp2m_active(n->domain) )
This would benefit of an unlikely (maybe within altp2m_active).

> +        p2m_load_altp2m_VTTBR(n);
> +    else
> +        p2m_load_VTTBR(n->domain);
> +
>      isb();
>
>      if ( is_32bit_domain(n->domain) )
> @@ -119,22 +154,42 @@ void p2m_restore_state(struct vcpu *n)
>  void flush_tlb_domain(struct domain *d)
>  {
>      unsigned long flags = 0;
> +    struct vcpu *v = NULL;
>
> -    /* Update the VTTBR if necessary with the domain d. In this case,
> -     * it's only necessary to flush TLBs on every CPUs with the current VMID
> -     * (our domain).
> +    /*
> +     * Update the VTTBR if necessary with the domain d. In this case, it is 
> only
> +     * necessary to flush TLBs on every CPUs with the current VMID (our
> +     * domain).
>       */
>      if ( d != current->domain )
>      {
>          local_irq_save(flags);
> -        p2m_load_VTTBR(d);
> -    }
>
> -    flush_tlb();
> +        /* If altp2m is active, update VTTBR and flush TLBs of every VCPU */
> +        if ( altp2m_active(d) )
> +        {
> +            for_each_vcpu( d, v )
> +            {
> +                p2m_load_altp2m_VTTBR(v);
> +                flush_tlb();
> +            }
> +        }
> +        else
> +        {
> +            p2m_load_VTTBR(d);
> +            flush_tlb();
> +        }
Why do you need to do a such things? If the VMID is the same, a single 
call to flush_tlb() will nuke all the entries for the given TLBs.
If the VMID is not shared, then I am not even sure why you would need to 
flush the TLBs for all the altp2ms.
I have looked to Xen with this series applied and noticed that when you 
remove a page from the hostp2m, the mapping in the altp2m is not 
removed. So the guest may use a page that would have been freed 
previously. Did I miss something?
Regards,

--
Julien Grall

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