No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v2->v3:
  * No change

v1->v2:
  * s/ret/bndcfgu
---
 xen/arch/x86/xstate.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 3d249518a1b7..2003ba664594 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -1024,9 +1024,10 @@ int handle_xsetbv(u32 index, u64 new_bv)
 
 uint64_t read_bndcfgu(void)
 {
+    uint64_t bndcfgu = 0;
     unsigned long cr0 = read_cr0();
-    struct xsave_struct *xstate
-        = idle_vcpu[smp_processor_id()]->arch.xsave_area;
+    struct vcpu *v = idle_vcpu[smp_processor_id()];
+    struct xsave_struct *xstate = VCPU_MAP_XSAVE_AREA(v);
     const struct xstate_bndcsr *bndcsr;
 
     ASSERT(cpu_has_mpx);
@@ -1052,7 +1053,12 @@ uint64_t read_bndcfgu(void)
     if ( cr0 & X86_CR0_TS )
         write_cr0(cr0);
 
-    return xstate->xsave_hdr.xstate_bv & X86_XCR0_BNDCSR ? bndcsr->bndcfgu : 0;
+    if ( xstate->xsave_hdr.xstate_bv & X86_XCR0_BNDCSR )
+        bndcfgu = bndcsr->bndcfgu;
+
+    VCPU_UNMAP_XSAVE_AREA(v, xstate);
+
+    return bndcfgu;
 }
 
 void xstate_set_init(uint64_t mask)
-- 
2.47.1

On 10.01.2025 14:28, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -1024,9 +1024,10 @@ int handle_xsetbv(u32 index, u64 new_bv)
>  
>  uint64_t read_bndcfgu(void)
>  {
> +    uint64_t bndcfgu = 0;
>      unsigned long cr0 = read_cr0();
> -    struct xsave_struct *xstate
> -        = idle_vcpu[smp_processor_id()]->arch.xsave_area;
> +    struct vcpu *v = idle_vcpu[smp_processor_id()];

Question on this one remains: Can it be pointer-to-const (in the longer
run; certainly in can be right now)?

> +    struct xsave_struct *xstate = VCPU_MAP_XSAVE_AREA(v);

I realize my similar remark on this one was actually wrong; the asm()s
clearly modify what is being pointed top.

Jan

Hi,

Thanks for reviewing this and the other patches.

On Mon Jan 27, 2025 at 10:57 AM GMT, Jan Beulich wrote:
> On 10.01.2025 14:28, Alejandro Vallejo wrote:
> > --- a/xen/arch/x86/xstate.c
> > +++ b/xen/arch/x86/xstate.c
> > @@ -1024,9 +1024,10 @@ int handle_xsetbv(u32 index, u64 new_bv)
> >  
> >  uint64_t read_bndcfgu(void)
> >  {
> > +    uint64_t bndcfgu = 0;
> >      unsigned long cr0 = read_cr0();
> > -    struct xsave_struct *xstate
> > -        = idle_vcpu[smp_processor_id()]->arch.xsave_area;
> > +    struct vcpu *v = idle_vcpu[smp_processor_id()];
>
> Question on this one remains: Can it be pointer-to-const (in the longer
> run; certainly in can be right now)?

I have no idea where I got the idea the C constness was transitive (quite
likely from Rust, as its illegal to grab a &mut from a &). Const being
non-transitive means I can constify the vcpu as you suggest/ask. The rationale
was that getting a pointer to non-const from a pointer to const seemed wrong.

But C is bizarre and its finer details have a way of biting. Oh well.

FWIW, this ought to hold even in the long run. There won't be anything special
in the map/unmap macros besides some indirection, so it'll work in a very
similar fashion as it does now.

I'll adjust it as you propose.

>
> > +    struct xsave_struct *xstate = VCPU_MAP_XSAVE_AREA(v);
>
> I realize my similar remark on this one was actually wrong; the asm()s
> clearly modify what is being pointed top.

Indeed, xstate can definitely not be const.

> Jan

Cheers,
Alejandro