[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.