This is used in an assertion only, which is somewhat dubious to begin with and
won't surivive the x86-S work (where TR will become be a NUL selector).

Delete it now.  This avoids many cases where as a global symbol, it shadows
local string variables.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Roberto Bagnara <roberto.bagnara@bugseng.com>
CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/hvm/svm/svm.c      | 2 --
 xen/arch/x86/include/asm/desc.h | 9 ---------
 2 files changed, 11 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 56cb2f61bb75..4d29ad3bc36a 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1614,8 +1614,6 @@ static int _svm_cpu_up(bool bsp)
     /* Initialize OSVW bits to be used by guests */
     svm_host_osvw_init();
 
-    /* Minimal checking that enough CPU setup was done by now. */
-    ASSERT(str() == TSS_SELECTOR);
     svm_vmsave_pa(per_cpu(host_vmcb, cpu));
 
     return 0;
diff --git a/xen/arch/x86/include/asm/desc.h b/xen/arch/x86/include/asm/desc.h
index 225a864c483e..a1e0807d97ed 100644
--- a/xen/arch/x86/include/asm/desc.h
+++ b/xen/arch/x86/include/asm/desc.h
@@ -238,15 +238,6 @@ static inline void ltr(unsigned int sel)
     __asm__ __volatile__ ( "ltr %w0" :: "rm" (sel) : "memory" );
 }
 
-static inline unsigned int str(void)
-{
-    unsigned int sel;
-
-    __asm__ ( "str %0" : "=r" (sel) );
-
-    return sel;
-}
-
 #endif /* !__ASSEMBLY__ */
 
 #endif /* __ARCH_DESC_H */
-- 
2.30.2

On 28/07/2023 21:43, Andrew Cooper wrote:
> This is used in an assertion only, which is somewhat dubious to begin 
> with and
> won't surivive the x86-S work (where TR will become be a NUL selector).
> 
> Delete it now.  This avoids many cases where as a global symbol, it 
> shadows
> local string variables.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roberto Bagnara <roberto.bagnara@bugseng.com>
> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/arch/x86/hvm/svm/svm.c      | 2 --
>  xen/arch/x86/include/asm/desc.h | 9 ---------
>  2 files changed, 11 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 56cb2f61bb75..4d29ad3bc36a 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1614,8 +1614,6 @@ static int _svm_cpu_up(bool bsp)
>      /* Initialize OSVW bits to be used by guests */
>      svm_host_osvw_init();
> 
> -    /* Minimal checking that enough CPU setup was done by now. */
> -    ASSERT(str() == TSS_SELECTOR);
>      svm_vmsave_pa(per_cpu(host_vmcb, cpu));
> 
>      return 0;
> diff --git a/xen/arch/x86/include/asm/desc.h 
> b/xen/arch/x86/include/asm/desc.h
> index 225a864c483e..a1e0807d97ed 100644
> --- a/xen/arch/x86/include/asm/desc.h
> +++ b/xen/arch/x86/include/asm/desc.h
> @@ -238,15 +238,6 @@ static inline void ltr(unsigned int sel)
>      __asm__ __volatile__ ( "ltr %w0" :: "rm" (sel) : "memory" );
>  }
> 
> -static inline unsigned int str(void)
> -{
> -    unsigned int sel;
> -
> -    __asm__ ( "str %0" : "=r" (sel) );
> -
> -    return sel;
> -}
> -
>  #endif /* !__ASSEMBLY__ */
> 
>  #endif /* __ARCH_DESC_H */

With respect to shadowing (Rule 5.3)
Tested-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

On 28.07.2023 21:43, Andrew Cooper wrote:
> This is used in an assertion only, which is somewhat dubious to begin with and
> won't surivive the x86-S work (where TR will become be a NUL selector).

I'm kind of okay with the removal, but I can't read anything like the
above out of the doc. Can you point me at where this is said?

Jan

> Delete it now.  This avoids many cases where as a global symbol, it shadows
> local string variables.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roberto Bagnara <roberto.bagnara@bugseng.com>
> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/arch/x86/hvm/svm/svm.c      | 2 --
>  xen/arch/x86/include/asm/desc.h | 9 ---------
>  2 files changed, 11 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 56cb2f61bb75..4d29ad3bc36a 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1614,8 +1614,6 @@ static int _svm_cpu_up(bool bsp)
>      /* Initialize OSVW bits to be used by guests */
>      svm_host_osvw_init();
>  
> -    /* Minimal checking that enough CPU setup was done by now. */
> -    ASSERT(str() == TSS_SELECTOR);
>      svm_vmsave_pa(per_cpu(host_vmcb, cpu));
>  
>      return 0;
> diff --git a/xen/arch/x86/include/asm/desc.h b/xen/arch/x86/include/asm/desc.h
> index 225a864c483e..a1e0807d97ed 100644
> --- a/xen/arch/x86/include/asm/desc.h
> +++ b/xen/arch/x86/include/asm/desc.h
> @@ -238,15 +238,6 @@ static inline void ltr(unsigned int sel)
>      __asm__ __volatile__ ( "ltr %w0" :: "rm" (sel) : "memory" );
>  }
>  
> -static inline unsigned int str(void)
> -{
> -    unsigned int sel;
> -
> -    __asm__ ( "str %0" : "=r" (sel) );
> -
> -    return sel;
> -}
> -
>  #endif /* !__ASSEMBLY__ */
>  
>  #endif /* __ARCH_DESC_H */

On 31/07/2023 9:25 am, Jan Beulich wrote:
> On 28.07.2023 21:43, Andrew Cooper wrote:
>> This is used in an assertion only, which is somewhat dubious to begin with and
>> won't surivive the x86-S work (where TR will become be a NUL selector).
> I'm kind of okay with the removal, but I can't read anything like the
> above out of the doc. Can you point me at where this is said?

A future draft of the spec.

FRED removes the IDT completely, most of the TSS, and can let you get
away with GDT/LDT limits of 0.

x86-S removes the final aspects of the TSS (the IO perm bitmap, and PVI).

Intel have agreed that being able to (effectively) `ltr $0x0000` to set
the TSS invalid (like NULL selectors do for all other segments) would be
useful, as it means you don't need to have a transiently non-empty GDT
just to load an empty TR.

~Andrew