From: Bertrand Marquis <Bertrand.Marquis@arm.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
"Julien Grall" <julien@xen.org>, "Wei Liu" <wl@xen.org>,
"Andrew Cooper" <andrew.cooper3@citrix.com>,
"Ian Jackson" <ian.jackson@eu.citrix.com>,
"George Dunlap" <george.dunlap@citrix.com>,
Xen-devel <xen-devel@lists.xenproject.org>, nd <nd@arm.com>,
"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH v2] xen/arm: Convert runstate address during hypcall
Date: Wed, 29 Jul 2020 07:08:56 +0000 [thread overview]
Message-ID: <FCAB700B-4617-4323-BE1E-B80DDA1806C1@arm.com> (raw)
In-Reply-To: <8d2d7f03-450c-d50c-630b-8608c6d42bb9@suse.com>
> On 28 Jul 2020, at 21:54, Jan Beulich <jbeulich@suse.com> 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
next prev parent reply other threads:[~2020-07-29 7:09 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-28 15:52 [PATCH v2] xen/arm: Convert runstate address during hypcall Bertrand Marquis
2020-07-28 19:04 ` Stefano Stabellini
2020-07-29 6:47 ` Bertrand Marquis
2020-07-28 19:54 ` Jan Beulich
2020-07-29 7:08 ` Bertrand Marquis [this message]
2020-07-29 18:41 ` Jan Beulich
2020-07-30 1:30 ` Stefano Stabellini
2020-07-31 6:39 ` Jan Beulich
2020-07-31 10:12 ` Julien Grall
2020-07-31 10:18 ` Jan Beulich
2020-07-31 14:36 ` Bertrand Marquis
2020-07-31 23:03 ` Stefano Stabellini
2020-08-14 9:25 ` Bertrand Marquis
2020-08-20 10:23 ` Julien Grall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=FCAB700B-4617-4323-BE1E-B80DDA1806C1@arm.com \
--to=bertrand.marquis@arm.com \
--cc=Volodymyr_Babchuk@epam.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=ian.jackson@eu.citrix.com \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=nd@arm.com \
--cc=roger.pau@citrix.com \
--cc=sstabellini@kernel.org \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).