Re: [PATCH v2] xen/arm: Convert runstate address during hypcall

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

> On 28 Jul 2020, at 21:54, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 28.07.2020 17:52, Bertrand Marquis wrote:
>> At the moment on Arm, a Linux guest running with KTPI enabled will
>> cause the following error when a context switch happens in user mode:
>> (XEN) p2m.c:1890: d1v0: Failed to walk page-table va 0xffffff837ebe0cd0
>> The error is caused by the virtual address for the runstate area
>> registered by the guest only being accessible when the guest is running
>> in kernel space when KPTI is enabled.
>> To solve this issue, this patch is doing the translation from virtual
>> address to physical address during the hypercall and mapping the
>> required pages using vmap. This is removing the conversion from virtual
>> to physical address during the context switch which is solving the
>> problem with KPTI.
>> This is done only on arm architecture, the behaviour on x86 is not
>> modified by this patch and the address conversion is done as before
>> during each context switch.
>> This is introducing several limitations in comparison to the previous
>> behaviour (on arm only):
>> - if the guest is remapping the area at a different physical address Xen
>> will continue to update the area at the previous physical address. As
>> the area is in kernel space and usually defined as a global variable this
>> is something which is believed not to happen. If this is required by a
>> guest, it will have to call the hypercall with the new area (even if it
>> is at the same virtual address).
>> - the area needs to be mapped during the hypercall. For the same reasons
>> as for the previous case, even if the area is registered for a different
>> vcpu. It is believed that registering an area using a virtual address
>> unmapped is not something done.
> 
> Beside me thinking that an in-use and stable ABI can't be changed like
> this, no matter what is "believed" kernel code may or may not do, I
> also don't think having arch-es diverge in behavior here is a good
> idea. Use of commonly available interfaces shouldn't lead to head
> aches or surprises when porting code from one arch to another. I'm
> pretty sure it was suggested before: Why don't you simply introduce
> a physical address based hypercall (and then also on x86 at the same
> time, keeping functional parity)? I even seem to recall giving a
> suggestion how to fit this into a future "physical addresses only"
> model, as long as we can settle on the basic principles of that
> conversion path that we want to go sooner or later anyway (as I
> understand).

I fully agree with the “physical address only” model and i think it must be
done. Introducing a new hypercall taking a physical address as parameter
is the long term solution (and I would even volunteer to do it in a new 
patchset).
But this would not solve the issue here unless linux is modified.
So I do see this patch as a “bug fix”.

> 
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1642,6 +1642,30 @@ void paravirt_ctxt_switch_to(struct vcpu *v)
>>          wrmsr_tsc_aux(v->arch.msrs->tsc_aux);
>>  }
>>  +int arch_vcpu_setup_runstate(struct vcpu *v,
>> +                             struct vcpu_register_runstate_memory_area area)
>> +{
>> +    struct vcpu_runstate_info runstate;
>> +
>> +    runstate_guest(v) = area.addr.h;
>> +
>> +    if ( v == current )
>> +    {
>> +        __copy_to_guest(runstate_guest(v), &v->runstate, 1);
>> +    }
> 
> Pointless braces (and I think there are more instances).

So:
if cond
   instruction
else
{
   xxx
}

is something that should be done in Xen ?

Sorry if i do those kind of mistakes in the future as i am more used to a model
where no braces is an absolute no-go. I will try to remember this.

> 
>> +    else
>> +    {
>> +        vcpu_runstate_get(v, &runstate);
>> +        __copy_to_guest(runstate_guest(v), &runstate, 1);
>> +    }
>> +    return 0;
> 
> Missing blank line before main "return".

I will fix it.
Bertrand