Re: [PATCH v2] x86/pv: Fix assertions in svm_load_segs()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.09.2020 17: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
> 
> This is because we pass a non-NUL selector into svm_load_segs(), which is
> something we must not do, as this path does not load the attributes/limit from
> the GDT/LDT.
> 
> Drop the {gs,gs}_sel parameters from svm_load_segs() and use 0 instead.  This

Nit: {fs,gs}

> is fine even for non-zero NUL segments, as it is how the IRET instruction
> behaves in all CPUs.

To be honest, I'd not call it "fine", but "acceptable". (And I don't
consider IRET's behavior "fine" either.)

> Only use the svm_load_segs() path when FS and GS are NUL, which is the common
> case when scheduling a 64bit vcpu with 64bit userspace in context.
> 
> Fixes: ad0fd291c5 ("x86/pv: Rewrite segment context switching from scratch")
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> --- a/xen/include/asm-x86/hvm/svm/svm.h
> +++ b/xen/include/asm-x86/hvm/svm/svm.h
> @@ -52,10 +52,12 @@ void svm_update_guest_cr(struct vcpu *, unsigned int cr, 
> unsigned int flags);
>  /*
>   * PV context switch helper. Calls with zero ldt_base request a prefetch of
>   * the VMCB area to be loaded from, instead of an actual load of state.
> + *
> + * Must only be used for NUL FS/GS, as the segment attributes/limits are not
> + * read from the GDT/LDT.
>   */

Ah, right - this is the part I was missing when you said you'd redo
the patch.

Jan