[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
  • Date: Wed, 29 Jul 2020 07:08:56 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=U2cKBtYay6SUBHhKq0BxG/6jpeaOSxBEP3HVwLHv64Y=; b=ERkNFSrG26/sM5yF1HHzgABmqF2zOuMm3hEkti8gJsAxoccpYVb1M2oSDsK7+B7ayJVP+p6ah9IEf+CHsr++uYDsqphh00+oGgN0K1I0kg+egdy7S5URNSZ8on3Z1EFAovNavrvG2D+h50c/przriEEFgh5KfDGX2L1Uu6KLgqBfQWBeBYWlK+qDg2LN+DVMLJg6gli/OwalhyBHtjLPMjuCO3L9WERKjDQRTHcauSNgOgVeAc15eTdEtbgTwy0XuJEQ8scKH7lwOLn7cKjRFnNVOiqzafsIMC8NUmAc35V9ZSE06Zbzhby2t+HeZInHowb+943//2XgpiU47LMxYw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=TS69m3nXzlJyyKMa1Bgz2VZGb79s0/tshaohVikmEoL6doyij5byz3b9tEZYbFfQgheYcwDJnZsVOwg/r0kMqgsF/NcRfhI+5UkGqPisXrgMybtK3gHjpaVvqWMherqk9X2+StjL2zR05lTDYGvZ2S1uVSLEQ9B2YFDP1cKJoU0c8yw3V8w4nEyl5WSXWgwymS3fsGriiY/QFbL6yNtaxnniW8tPiVdFETE69bzj9AoMAJqaw3HI2NwpC5vrq4aTzBhKzPhybvH6zHkH/a65I+X0gYbFTSij9nWCQsCoIreSC+v9KTrcBQMTsf8itmnl/Ii1lmLpWI16gIMfHsiayQ==
  • Authentication-results-original: suse.com; dkim=none (message not signed) header.d=none;suse.com; dmarc=none action=none header.from=arm.com;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Wei Liu <wl@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Ian Jackson <ian.jackson@xxxxxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, nd <nd@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Wed, 29 Jul 2020 07:09:18 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: suse.com; dkim=none (message not signed) header.d=none;suse.com; dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHWZPcpcJdIAbXhcEqLxJ7JubPXUKkdZ+eAgAC8aQA=
  • Thread-topic: [PATCH v2] xen/arm: Convert runstate address during hypcall

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

if cond

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.



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.