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

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel