[PATCH for-4.9 0/2] x86/pv: Misc fixes

[PATCH for-4.9 0/2] x86/pv: Misc fixes

[Andrew Cooper]

Two fixes for bugs which I have found while trying to raise
{compat_,}create_bounce_frame() up into C.  They should both be fixed in all
releases of Xen.

Andrew Cooper (2):
  x86/pv: Fix bugs with the handling of int80_bounce
  x86/pv: Align %rsp before pushing the failsafe stack frame

 xen/arch/x86/domain.c              | 5 ++---
 xen/arch/x86/x86_64/compat/traps.c | 1 +
 xen/arch/x86/x86_64/traps.c        | 3 ++-
 3 files changed, 5 insertions(+), 4 deletions(-)

-- 
2.1.4


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

[PATCH 1/2] x86/pv: Fix bugs with the handling of int80_bounce

[Andrew Cooper]

Testing has revealed two issues:

 1) Passing a NULL handle to set_trap_table() is intended to flush the entire
    table.  The 64bit guest case (and 32bit guest on 32bit Xen, when it
    existed) called init_int80_direct_trap() to reset int80_bounce, but c/s
    cda335c279 which introduced the 32bit guest on 64bit Xen support omitted
    this step.  Previously therefore, it was impossible for a 32bit guest to
    reset its registered int80_bounce details.

 2) init_int80_direct_trap() doesn't honour the guests request to have
    interrupts disabled on entry.  PVops Linux requests that interrupts are
    disabled, but Xen currently leaves them enabled when following the int80
    fastpath.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Julien Grall <julien.grall@arm.com>

This change should be backported to all releases, and included in Xen 4.9
---
 xen/arch/x86/x86_64/compat/traps.c | 1 +
 xen/arch/x86/x86_64/traps.c        | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/x86_64/compat/traps.c b/xen/arch/x86/x86_64/compat/traps.c
index 8e9a11c..1751ec6 100644
--- a/xen/arch/x86/x86_64/compat/traps.c
+++ b/xen/arch/x86/x86_64/compat/traps.c
@@ -339,6 +339,7 @@ int compat_set_trap_table(XEN_GUEST_HANDLE(trap_info_compat_t) traps)
     if ( guest_handle_is_null(traps) )
     {
         memset(dst, 0, NR_VECTORS * sizeof(*dst));
+        init_int80_direct_trap(current);
         return 0;
     }
 
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index ad4d6c1..78f4105 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -427,12 +427,13 @@ void init_int80_direct_trap(struct vcpu *v)
     struct trap_info *ti = &v->arch.pv_vcpu.trap_ctxt[0x80];
     struct trap_bounce *tb = &v->arch.pv_vcpu.int80_bounce;
 
-    tb->flags = TBF_EXCEPTION;
     tb->cs    = ti->cs;
     tb->eip   = ti->address;
 
     if ( null_trap_bounce(v, tb) )
         tb->flags = 0;
+    else
+        tb->flags = TBF_EXCEPTION | (TI_GET_IF(ti) ? TBF_INTERRUPT : 0);
 }
 
 static long register_guest_callback(struct callback_register *reg)
-- 
2.1.4


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

Re: [PATCH 1/2] x86/pv: Fix bugs with the handling of int80_bounce

[Jan Beulich]

>>> On 08.05.17 at 12:04, <andrew.cooper3@citrix.com> wrote:
> Testing has revealed two issues:
> 
>  1) Passing a NULL handle to set_trap_table() is intended to flush the entire
>     table.  The 64bit guest case (and 32bit guest on 32bit Xen, when it
>     existed) called init_int80_direct_trap() to reset int80_bounce, but c/s
>     cda335c279 which introduced the 32bit guest on 64bit Xen support omitted
>     this step.  Previously therefore, it was impossible for a 32bit guest to
>     reset its registered int80_bounce details.
> 
>  2) init_int80_direct_trap() doesn't honour the guests request to have
>     interrupts disabled on entry.  PVops Linux requests that interrupts are
>     disabled, but Xen currently leaves them enabled when following the int80
>     fastpath.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with a remark:

> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -427,12 +427,13 @@ void init_int80_direct_trap(struct vcpu *v)
>      struct trap_info *ti = &v->arch.pv_vcpu.trap_ctxt[0x80];
>      struct trap_bounce *tb = &v->arch.pv_vcpu.int80_bounce;
>  
> -    tb->flags = TBF_EXCEPTION;
>      tb->cs    = ti->cs;
>      tb->eip   = ti->address;
>  
>      if ( null_trap_bounce(v, tb) )
>          tb->flags = 0;
> +    else
> +        tb->flags = TBF_EXCEPTION | (TI_GET_IF(ti) ? TBF_INTERRUPT : 0);
>  }

This certainly is a correct change to make, but it's not without risk:
If some guest relies on previous buggy behavior (wrongly setting
the flag but expecting interrupts to be on), ugly misbehavior in the
guest could result. Initially I was afraid XenoLinux might be
affected, but I've checked and it isn't.

Jan


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

Re: [PATCH 1/2] x86/pv: Fix bugs with the handling of int80_bounce

[Andrew Cooper]

On 08/05/17 11:52, Jan Beulich wrote:
>>>> On 08.05.17 at 12:04, <andrew.cooper3@citrix.com> wrote:
>> Testing has revealed two issues:
>>
>>  1) Passing a NULL handle to set_trap_table() is intended to flush the entire
>>     table.  The 64bit guest case (and 32bit guest on 32bit Xen, when it
>>     existed) called init_int80_direct_trap() to reset int80_bounce, but c/s
>>     cda335c279 which introduced the 32bit guest on 64bit Xen support omitted
>>     this step.  Previously therefore, it was impossible for a 32bit guest to
>>     reset its registered int80_bounce details.
>>
>>  2) init_int80_direct_trap() doesn't honour the guests request to have
>>     interrupts disabled on entry.  PVops Linux requests that interrupts are
>>     disabled, but Xen currently leaves them enabled when following the int80
>>     fastpath.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with a remark:
>
>> --- a/xen/arch/x86/x86_64/traps.c
>> +++ b/xen/arch/x86/x86_64/traps.c
>> @@ -427,12 +427,13 @@ void init_int80_direct_trap(struct vcpu *v)
>>      struct trap_info *ti = &v->arch.pv_vcpu.trap_ctxt[0x80];
>>      struct trap_bounce *tb = &v->arch.pv_vcpu.int80_bounce;
>>  
>> -    tb->flags = TBF_EXCEPTION;
>>      tb->cs    = ti->cs;
>>      tb->eip   = ti->address;
>>  
>>      if ( null_trap_bounce(v, tb) )
>>          tb->flags = 0;
>> +    else
>> +        tb->flags = TBF_EXCEPTION | (TI_GET_IF(ti) ? TBF_INTERRUPT : 0);
>>  }
> This certainly is a correct change to make, but it's not without risk:
> If some guest relies on previous buggy behavior (wrongly setting
> the flag but expecting interrupts to be on), ugly misbehavior in the
> guest could result. Initially I was afraid XenoLinux might be
> affected, but I've checked and it isn't.

So c/s bfad55585 (which introduces int80_bounce) is a very interesting read.

(Aside from the point here, I didn't realise that we ever had a copy of
the FreeBSD kernel in tree, or that the reason we have a separate IDT
per pcpu was for the predecessor to int80_bouce.)

At the time, there was generic set_fast_trap() which rewrote the IDT to
move straight from ring3 to ring1.  It had a few restrictions such as
only tolerating a vector of 0x80, and rejecting the setup if interrupts
were requested to be disabled (as there was no way of clearing the
evtchn_upcall_mask with this mechanism).

That patch introduced init_int80_direct_trap(), along with a comment
explaining why interrupt gates were rejected, although it was restricted
to 32bit hypervisors at that point.

The direct-trap path was never available in a 64bit build of Xen (owing
to the inability to have non long mode code segments in the IDT), and
c/s 3e1b9525de introduced the int80_direct_trap() path (which looks
remarkably unchanged in the 10 years its been in the codebase).

At this point, the 32bit version of int80_direct_trap() explained why it
couldn't tolerate interrupt gates, but the newly-introduced 64bit
version omitted any comment/code on this point, and would have worked
correctly for interrupt gates if the adjustment in this patch had been
considered.

Overall, I expect that Xenolinux purposefully never asked for an
interrupt gate (as Xen would have rejected that in the 32bit case), and
this point was never considered at all when PVOps was developed, which
followed Linux's normal expectation that int80 was an interrupt gate.

~Andrew

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

Re: [PATCH 1/2] x86/pv: Fix bugs with the handling of int80_bounce

[Jan Beulich]

>>> On 08.05.17 at 14:23, <andrew.cooper3@citrix.com> wrote:
> Overall, I expect that Xenolinux purposefully never asked for an
> interrupt gate (as Xen would have rejected that in the 32bit case), and
> this point was never considered at all when PVOps was developed, which
> followed Linux's normal expectation that int80 was an interrupt gate.

Yet I have a hard time seeing why 32-bit Linux needs it to be that
way.

Jan


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

Re: [PATCH 1/2] x86/pv: Fix bugs with the handling of int80_bounce

[Andrew Cooper]

On 08/05/17 13:39, Jan Beulich wrote:
>>>> On 08.05.17 at 14:23, <andrew.cooper3@citrix.com> wrote:
>> Overall, I expect that Xenolinux purposefully never asked for an
>> interrupt gate (as Xen would have rejected that in the 32bit case), and
>> this point was never considered at all when PVOps was developed, which
>> followed Linux's normal expectation that int80 was an interrupt gate.
> Yet I have a hard time seeing why 32-bit Linux needs it to be that
> way.

I don't understand what you mean by this.

Is it a counterpoint to my statement, or just an observation about the
way Linux chooses to set things up?

~Andrew

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

Re: [PATCH 1/2] x86/pv: Fix bugs with the handling of int80_bounce

[Jan Beulich]

>>> On 08.05.17 at 14:54, <andrew.cooper3@citrix.com> wrote:
> On 08/05/17 13:39, Jan Beulich wrote:
>>>>> On 08.05.17 at 14:23, <andrew.cooper3@citrix.com> wrote:
>>> Overall, I expect that Xenolinux purposefully never asked for an
>>> interrupt gate (as Xen would have rejected that in the 32bit case), and
>>> this point was never considered at all when PVOps was developed, which
>>> followed Linux's normal expectation that int80 was an interrupt gate.
>> Yet I have a hard time seeing why 32-bit Linux needs it to be that
>> way.
> 
> I don't understand what you mean by this.
> 
> Is it a counterpoint to my statement, or just an observation about the
> way Linux chooses to set things up?

Just an observation, including the implication that at least the 32-bit
Linux case has no issue (and quite likely never had one) with us not
masking events despite this having been requested. Otoh it's a little
surprising that 64-bit Linux never had this reported as an issue, as
there's a non-zero chance for its entry code to be hit before the GS
switch.

Jan


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

[PATCH 2/2] x86/pv: Align %rsp before pushing the failsafe stack frame

[Andrew Cooper]

Architecturally, all 64bit stacks are aligned on a 16 byte boundary before an
exception frame is pushed.  The failsafe frame is not special in this regard.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Julien Grall <julien.grall@arm.com>

This change should be backported to all releases, and included in Xen 4.9
---
 xen/arch/x86/domain.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 6e1474b..2ef1c9f 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1280,9 +1280,8 @@ static void load_segments(struct vcpu *n)
         struct pv_vcpu *pv = &n->arch.pv_vcpu;
         struct cpu_user_regs *regs = guest_cpu_user_regs();
         unsigned long *rsp =
-            (n->arch.flags & TF_kernel_mode) ?
-            (unsigned long *)regs->rsp :
-            (unsigned long *)pv->kernel_sp;
+            (unsigned long *)(((n->arch.flags & TF_kernel_mode)
+                               ? regs->rsp : pv->kernel_sp) & ~0xf);
         unsigned long cs_and_mask, rflags;
 
         /* Fold upcall mask and architectural IOPL into RFLAGS.IF. */
-- 
2.1.4


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

Re: [PATCH 2/2] x86/pv: Align %rsp before pushing the failsafe stack frame

[Jan Beulich]

>>> On 08.05.17 at 12:04, <andrew.cooper3@citrix.com> wrote:
> Architecturally, all 64bit stacks are aligned on a 16 byte boundary before an
> exception frame is pushed.  The failsafe frame is not special in this regard.

I'd prefer "should not be" as effectively is has been so far, but other
than that ...

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH 2/2] x86/pv: Align %rsp before pushing the failsafe stack frame

[Andrew Cooper]

On 08/05/17 11:56, Jan Beulich wrote:
>>>> On 08.05.17 at 12:04, <andrew.cooper3@citrix.com> wrote:
>> Architecturally, all 64bit stacks are aligned on a 16 byte boundary before an
>> exception frame is pushed.  The failsafe frame is not special in this regard.
> I'd prefer "should not be" as effectively is has been so far, but other
> than that ...

Certainly.

>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

~Andrew

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

Re: [PATCH for-4.9 0/2] x86/pv: Misc fixes

[Julien Grall]

Hi Andrew,

On 08/05/17 11:04, Andrew Cooper wrote:
> Two fixes for bugs which I have found while trying to raise
> {compat_,}create_bounce_frame() up into C.  They should both be fixed in all
> releases of Xen.
>
> Andrew Cooper (2):
>   x86/pv: Fix bugs with the handling of int80_bounce
>   x86/pv: Align %rsp before pushing the failsafe stack frame

For the 2 patches:

Release-acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

>
>  xen/arch/x86/domain.c              | 5 ++---
>  xen/arch/x86/x86_64/compat/traps.c | 1 +
>  xen/arch/x86/x86_64/traps.c        | 3 ++-
>  3 files changed, 5 insertions(+), 4 deletions(-)
>

-- 
Julien Grall

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