Re: [RFC PATCH 1/1] xen: Use a global mapping for runstate

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

> On 29 May 2020, at 08:19, Roger Pau Monné <roger@xxxxxxx> wrote:
> 
> On Thu, May 28, 2020 at 07:54:35PM +0100, Julien Grall wrote:
>> Hi Bertrand,
>> 
>> Thank you for the patch.
>> 
>> On 28/05/2020 16:25, Bertrand Marquis wrote:
>>> +static int map_runstate_area(struct vcpu *v,
>>> +                    struct vcpu_register_runstate_memory_area *area)
>>> +{
>>> +    unsigned long offset = area->addr.p & ~PAGE_MASK;
>>> +    void *mapping;
>>> +    struct page_info *page;
>>> +    size_t size = sizeof(struct vcpu_runstate_info);
>>> +
>>> +    ASSERT(runstate_guest(v) == NULL);
>>> +
>>> +    /* do not allow an area crossing 2 pages */
>>> +    if ( offset > (PAGE_SIZE - size) )
>>> +        return -EINVAL;
>> 
>> This is a change in behavior for the guest. If we are going forward with
>> this, this will want a separate patch with its own explanation why this is
>> done.
> 
> I don't think we can go this route without supporting crossing a page
> boundary.
> 
> Linux will BUG if VCPUOP_register_runstate_memory_area fails, and
> AFAICT there's no check in Linux to assure the runstate area doesn't
> cross a page boundary. If we want to go this route we must support the
> area crossing a page boundary, or else we will break existing
> guests.

Agree, I will add cross page boundary support.

> 
>>> +
>>> +#ifdef CONFIG_ARM
>>> +    page = get_page_from_gva(v, area->addr.p, GV2M_WRITE);
>> 
>> A guest is allowed to setup the runstate for a different vCPU than the
>> current one. This will lead to get_page_from_gva() to fail as the function
>> cannot yet work with a vCPU other than current.
>> 
>> AFAICT, there is no restriction on when the runstate hypercall can be
>> called. So this can even be called before the vCPU is brought up.
>> 
>> I was going to suggest to use the current vCPU for translating the address.
>> However, it would be reasonable for an OS to use the same virtual address
>> for all the vCPUs assuming the page-tables are different per vCPU.
> 
> Hm, it's a tricky question. Using the current vCPU page tables would
> seem like a good compromise, but it needs to get added to the header
> as a note, and this should ideally be merged at the start of a
> development cycle to get people time to test and report issues.

I agree and as this will not go in 4.14 we could got this route to have this in 
4.15 ?

Bertrand

> 
>> Recent Linux are using a per-cpu area, so the virtual address should be
>> different for each vCPU. But I don't know how the other OSes works. Roger
>> should be able to help for FreeBSD at least.
> 
> FreeBSD doesn't use VCPUOP_register_runstate_memory_area at all, so we
> are safe in that regard.
> 
> I never got around to implementing the required scheduler changes in
> order to support stolen time accounting.  Note sure this has changed
> since I last checked, the bhyve and KVM guys also had interest in
> properly accounting for stolen time on FreeBSD IIRC.
> 
> Roger.