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

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

On 07/22/2016 12:57 PM, Julien Grall wrote:
> 
> 
> 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.
> 

This true. However, p2m_propagate_change is invoked (from
apply_p2m_changes) only if the p2m that was currently modified is the
hostp2m.

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

Well yes. However, it is not used for the hostp2m, thus it makes it
automatically altp2m specific - but I know what you mean. Yet, I believe
it is cleaner to separate the entire altp2m code and maintain it in
altp2m.c. Nevertheless, I will need to pull back parts of altp2m code
into p2m.c, if we will not share some of the initialization/teardown
functions between both files.

Regards,
~Sergej


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