Re: [Xen-devel] [PATCH 20/22] xen/arm: Don't export flush_tlb_domain

[Julien Grall <julien.grall@xxxxxxx>]

On 22/07/16 11:46, Sergej Proskurin wrote:
> On 07/22/2016 12:34 PM, Julien Grall wrote:
> > On 22/07/16 11:25, Sergej Proskurin wrote:
> > > Hi Julien,
> > >
> > > On 07/22/2016 11:30 AM, Julien Grall wrote:
> > > > On 22/07/16 09:54, Sergej Proskurin wrote:
> > > > > Hi Julien,
> > > > Hello Sergej,
> > > >
> > > > > On 07/20/2016 06:11 PM, Julien Grall wrote:
> > > > > > The function flush_tlb_domain is not used outside of the file where it
> > > > > > has been declared.
> > > > > As for patch #15, the same applies here too:
> > > > > For altp2m, flush_tlb_domain/p2m_flush_tlb should be made available to
> > > > > ./xen/arch/arm/altp2m.c.
> > > > Based on your previous version, I don't see any reason to flush call
> > > > flush_tlb_domain/p2m_flush_tlb in altp2m.
> > > >
> > > > Please justify why you would need it.
> > > The new version considers changes that are made to the hostp2m and
> > > propagates them to all affected altp2m views by either changing
> > > individual altp2m entries or even flushing (but not destroying) the
> > > entire altp2m-tables. This idea has been borrowed from the x86 altp2m
> > > implementation.
> > >
> > > To prevent access to old/invalid GPAs, the current implementation
> > > flushes the TLBs associated with the affected altp2m view after such
> > > propagation.
> > There is already a flush in apply_p2m_changes and removing all the
> > mapping in a p2m could be implemented in p2m.c. So I still don't see why
> > you need the flush outside.
> Yes, the flush you are referring to flushes the hostp2m - not the
> individual altp2m views.
apply_p2m_changes is *not* hostp2m specific. It should work on any p2m 
regardless the type.
The ARM P2M interface is not set in stone, so if it does not fit it will 
need to be changed. We should avoid to hack the code in order to add a 
new feature.
It might be time to mention that I am reworking the whole p2m code it 
does not respect the ARM spec (such as break-before-make semantics) and 
I believe it does not fit the altp2m model. It is very difficult to 
implement the former with the current implementation and without have a 
big performance impact.
Rather than having a function which implement all the operations, I am 
planning to have a simple set of functions that can be used to 
re-implement the operations:
        - p2m_set_entry: Set an entry in the P2M
        - p2m_get_entry: Retrieve the informations of an entry

This is very similar to x86 and make more straight forward to implement 
new operations and co-op with the ARM spec.
I have already a prototype and I am hoping to send it soon.

> > I looked at the x86 version of the propagation and I was not able to
> > spot any explicit flush. Maybe you can provide some code to show what
> > you mean.
> Sure thing:
>
> ...
>
> static void p2m_reset_altp2m(struct p2m_domain *p2m)
> {
>     p2m_flush_table(p2m);
>     /* Uninit and reinit ept to force TLB shootdown */
>     ept_p2m_uninit(p2m);
>     ept_p2m_init(p2m);
>     p2m->min_remapped_gfn = INVALID_GFN;
>     p2m->max_remapped_gfn = 0;
> }
>
> ...
>
> On x86, the uninit- and re-initialization of the EPTs force the TLBs
> associated with the configured VMID of the EPTs to flush.
As mentioned in my previous mail, p2m_reset can be implemented in p2m.c 
as this is not altp2m.c specific.
When I asked to move altp2m specific code from p2m.c to altp2m.c it was 
for avoiding to have p2m.c too big. However, if the function is not 
altp2m specific, there is little reason to move outside.
Regards,

--
Julien Grall

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