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

[Sergej Proskurin <proskurin@sec.in.tum.de>]

Hi Julien,


On 07/04/2016 10:32 PM, Julien Grall wrote:
> 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@sec.in.tum.de>
>> ---
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> ---
>>  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.
>

I will combine these two functions, thank you.

>> +{
>> +    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.
>

I will revert these, thank you.

>>      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).
>

Fair enough. I will insert an unlikely macro into 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.
>

If the VMID is shared between multiple views and we would change one
entry in one specific view, we might reuse an entry that is not part of
the currently active altp2m view of the domain. And even if we assign
unique VMIDs to the individual altp2m views (as discussed in patch #04),
as far as I understand, we will still need to flush the mappings of all
altp2ms at this point because (AFAIK) changes in individual altp2m views
will still need to be propagated to the TLBs. Please correct me, if I am
wrong at this point.

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

When altp2m is active, the hostp2m is not used. All changes are applied
directly to the individual altp2m views. As far as I know, the only
situations, where a mapping is removed from a specific p2m view is in
the functions unmap_regions_rw_cache, unmap_mmio_regions, and
guest_physmap_remove_page. Each one of these functions provide, however
the hostp2m to the function apply_p2m_changes, where the mapping is
eventually removed. So, we definitely remove mappings from the hostp2m.
However, these would not be removed from the associated altp2m views.

Can you state a scenario, when and why pages might need to be removed at
run time from the hostp2m? In that case, maybe it would make sense to
extend the three functions (unmap_regions_rw_cache, unmap_mmio_regions,
and guest_physmap_remove_page) to additionally remove the mappings from
all available altp2m views?

Cheers,
~Sergej


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