diff mbox series

x86/pv: Drop assertions from svm_load_segs()

Message ID 20200908100803.8533-1-andrew.cooper3@citrix.com
State New
Headers show
Series x86/pv: Drop assertions from svm_load_segs() | expand

Commit Message

Andrew Cooper Sept. 8, 2020, 10:08 a.m. UTC
OSSTest has shown an assertion failure:
http://logs.test-lab.xenproject.org/osstest/logs/153906/test-xtf-amd64-amd64-1/serial-rimava1.log

These assertions were never appropriate, as they rule out legal (and, as it
turns out, sensible perf-wise) inputs based on an expectation of how the sole
caller would behave.

Fixes: ad0fd291c5 ("x86/pv: Rewrite segment context switching from scratch")
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/hvm/svm/svm.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Jan Beulich Sept. 8, 2020, 10:36 a.m. UTC | #1
On 08.09.2020 12:08, Andrew Cooper wrote:
> OSSTest has shown an assertion failure:
> http://logs.test-lab.xenproject.org/osstest/logs/153906/test-xtf-amd64-amd64-1/serial-rimava1.log
> 
> These assertions were never appropriate, as they rule out legal (and, as it
> turns out, sensible perf-wise) inputs based on an expectation of how the sole
> caller would behave.

I wouldn't put it this way - not coming here was a requirement in
the old logic, when we expected non-null selectors to remain in
sync with the GDT/LDT. But yes, they needs to be dropped now.

> Fixes: ad0fd291c5 ("x86/pv: Rewrite segment context switching from scratch")

With your S-o-b added
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
Andrew Cooper Sept. 8, 2020, 10:58 a.m. UTC | #2
On 08/09/2020 11:36, Jan Beulich wrote:
> On 08.09.2020 12:08, Andrew Cooper wrote:
>> OSSTest has shown an assertion failure:
>> http://logs.test-lab.xenproject.org/osstest/logs/153906/test-xtf-amd64-amd64-1/serial-rimava1.log
>>
>> These assertions were never appropriate, as they rule out legal (and, as it
>> turns out, sensible perf-wise) inputs based on an expectation of how the sole
>> caller would behave.
> I wouldn't put it this way - not coming here was a requirement in
> the old logic, when we expected non-null selectors to remain in
> sync with the GDT/LDT. But yes, they needs to be dropped now.

Urgh.  That is a can of worms, as this path doesn't preserve the GDT
attributes/limits.

>
>> Fixes: ad0fd291c5 ("x86/pv: Rewrite segment context switching from scratch")
> With your S-o-b added
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Oops.  Thanks, but no - I retract this patch.  Will submit a different
one shortly.

~Andrew
diff mbox series

Patch

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index af584ff5d1..eaacbcb668 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1559,13 +1559,11 @@  bool svm_load_segs(unsigned int ldt_ents, unsigned long ldt_base,
         vmcb->ldtr.base = ldt_base;
     }
 
-    ASSERT(!(fs_sel & ~3));
     vmcb->fs.sel = fs_sel;
     vmcb->fs.attr = 0;
     vmcb->fs.limit = 0;
     vmcb->fs.base = fs_base;
 
-    ASSERT(!(gs_sel & ~3));
     vmcb->gs.sel = gs_sel;
     vmcb->gs.attr = 0;
     vmcb->gs.limit = 0;