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

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

On 05/07/16 15:48, Sergej Proskurin wrote:
> On 07/04/2016 10:32 PM, Julien Grall wrote:
>> On 04/07/2016 12:45, Sergej Proskurin wrote:
>>> +        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.

You seem to be really confused how TLB flush instructions work on ARM.
The function flush_tlb() will flush TLBs on all the processor for the 
current VMID. If the VMID is shared, then calling N-times the flush with 
the same VMID will just be a waste of processor cycle.

Now, if the VMID is not shared (and based on your current series):
flush_tlb_domain is called in two places:
    - p2m_alloc_table
    - apply_p2m_changes

For the former, it allocates root table for a specific p2m. For the 
latter, the changes is only done for a specific p2m, and those changes 
are currently not replicated in all the p2ms. So flushing the TLBs for 
each altp2m view do not seem useful here too.

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

All the functions, you mentioned can be called after the domain has been 
created. If you grep guest_physmap_remove_page in the code source, you 
will find that it is used to unmap grant entry from the memory (see 
replace_grant_host_mapping) or when decreasing the memory reservation 
(see do_memory_op).

Note that, from my understanding, x86 has the same problem.

Regards,

-- 
Julien Grall

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