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

[Julien Grall <julien.grall@arm.com>]

Hi Roger,

On 07/03/2019 17:15, Roger Pau Monné wrote:
> On Thu, Mar 07, 2019 at 04:36:59PM +0000, Julien Grall wrote:
>> Hi Roger,
>>
>> Thank you for the answer.
>>
>> On 07/03/2019 16:16, Roger Pau Monné wrote:
>>> On Thu, Mar 07, 2019 at 03:17:54PM +0000, Julien Grall wrote:
>>>> Hi Andrii,
>>>>
>>>> On 07/03/2019 14:34, Andrii Anisov wrote:
>>>>> On 07.03.19 16:02, Julien Grall wrote:
>>>>>>>     - 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?
>>>>> Not exactly, access_guest_memory_by_ipa() has no implementation for x86.
>>>>> But it is made around that code.
>>>>
>>>> For the HVM, the equivalent function is hvm_copy_to_guest_phys. I don't know
>>>> what would be the interface for PV. Roger, any idea?
>>>
>>> For PV I think you will have to use get_page_from_gfn, check the
>>> permissions, map it, write and unmap it. The same flow would also work
>>> for HVM, so I'm not sure if there's much point in using
>>> hvm_copy_to_guest_phys. Or you can implement a generic
>>> copy_to_guest_phys helper that works for both PV and HVM.
>>>
>>> Note that for translated guests you will have to walk the HAP page
>>> tables for each vCPU for each context switch, which I think will be
>>> expensive in terms of performance (I might be wrong however, since I
>>> have no proof of this).
>>
>> AFAICT, we already walk the page-table with the current implementation. So
>> this should be no different here, except we will not need to walk the
>> guest-PT here. No?
> 
> Yes, current implementation is even worse because it walks both the
> guest page tables and the HAP page tables in the HVM case. It would be
> interesting IMO if we could avoid walking any of those page tables.
> 
> I see you have concerns about permanently mapping the runstate area,
> so I'm not going to oppose, albeit even with only 1G of VA space you
> can map plenty of runstate areas, and taking into account this is
> 32bit hardware I'm not sure you will ever have that many vCPUs that
> you will run out of VA space to map runstate areas.

Actually the vmap is only 768MB. The vmap is at the moment used for mapping:
	- MMIO devices (through ioremap)
         - event channel pages

As the runstate is far smaller than a page, this sounds like a waste of memory 
for a benefits that haven't not yet been shown. Indeed, number provided by 
Andrii either show worst performance or similar one.

But TBH, I am not expecting that a really clear performance improvement on Arm 
as there are a lot to do in the context switch.

> 
> That being said, if the implementation turns out to be more
> complicated because of this permanent mapping, walking the guest HAP
> page tables is certainly no worse than what's done ATM.

To be honest I am not fully against always mapping the runstate in Xen. But I 
need data to show this is worth it. So far, the performance promised are not 
there and the implementation is not foolproof yet.

If we want to keep the runstate mapped permanently, then we need to add either a 
lock or a refcounting. So the page does not disappear during context switch if 
we happen to update the runstate concurrently (via the hypercall).

This may increase the complexity of the implementation (not sure by how much 
thought).

Another solution is to prevent the runstate to be updated. But I think we will 
just add a bit more burden in the guest OS.

Cheers,

-- 
Julien Grall

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