Re: [Xen-devel] [PATCH 0/2 for-4.12] Introduce runstate area registration with phys address

[Julien Grall <julien.grall@xxxxxxx>]

On 07/03/2019 13:01, Andrii Anisov wrote:
> Dear Julien,
Hello,

> On 05.03.19 15:56, Julien Grall wrote:
> > I had some concern about this solution in [1] that have not been addressed nor 
> > even mentioned in the series.
> You missed the link.
I was referring to your [1].

> So I assume you say about you preferences to not have runstate area mapped 
> because of consuming vmap space for arm64. Also, along that thread you mentioned 
> you that guest might change gva mapping, what is irrelevant to registration with 
> physical address.
> My reasons to have that runstate mapped are following:
>   - Introducing the new interface we are not burden with legacy, so in charge to 
> impose requirements. In this case - to have runstate area not crossing a page 
> boundary
>   - Global mapping used here does not consume vmap space on arm64. It seems to 
> me x86 guys are ok with mapping as well, at least Roger suggested it from the 
> beginning. So it should be ok for them as well.
You left arm32 out of your equations here...

>   - In case domain is mapping runstate with physical address, it can not change 
> the mapping.
This is not entirely correct. The domain can not change the mapping under our 
feet, but it can still change via the hypercall. There are nothing preventing 
that with current hypercall and the one your propose.
AFAICT, the vCPU operation can happen from a different vCPU. So now you need to 
add some kind of synchronization to avoid the page disappearing under your feet 
if the mapping is replaced from another vCPU. The risk is to write to a page the 
has been allocated for another purpose and corrupt it.
I am not entirely sure we want to deny the call been done twice as this may have 
an impact on the way the OS is written. So the only way would be to introduce 
either reference counting or lock (if the former is not possible).
This is not an issue with the current code because we only store the guest 
virtual address. The worst thing that can happen is we are writing to the wrong 
guest virtual address, but at that point this is a guest problem. Although we 
may want to use ACCESS_ONCE() in both load and store to ensure atomicity.
>   - I suppose, normally, runstate area would be registered once per vcpu. While 
> it is updated each context switch, I do estimate it more optimal to have it 
> mapped once on registration. This reduces two mappings per context switch in the 
> worst case (one for prev vcpu, one for the next).
Well the number you showed in the other thread didn't show any improvement at 
all... So please explain why we should call map_domain_page_global() here and 
using more vmap on arm32.
>   - IMHO, this implementation is simpler and cleaner than what I have for 
> runstate mapping on access.
Did you implement it using access_guest_memory_by_ipa?

But I don't think the implementation you suggest will be that simpler once you 
deal with the problem above.
> So, taking in account all above, I really do not see, how runstate area 
> registered with physical address differs from vcpu_info and does not deserve to 
> be permanently mapped.
As I said to the other thread, we should switch all the hypercalls to use 
physical address. However, I am not convinced that keep the runstate mapped in 
Xen is a good idea.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel