[PATCH v2 0/3] x86/svm : Misc changes for few vmcb bits

[PATCH v2 0/3] x86/svm : Misc changes for few vmcb bits

[Vaishali Thakkar]

Hi,

In this patchset, first & second patch removes the unnecessary 
suffix from a bunch of vmcb bits. Third patch is about printing
the status of sev and sev-es bits while dumping VMCB.

Changes since v1:
  - Address comments from Andrew and Jan
  - Add extrapatch to drop the suffix _guest as per Andrew's
    suggestion in one of the reviews
  - Address Andrew's comment with respect to pretty printing

Vaishali Thakkar (3):
  x86/svm: Drop the _enabled suffix from vmcb bits
  x86/svm: Drop the suffix _guest from vmcb bit
  x86/svmdebug: Print sev and sev_es vmcb bits

 xen/arch/x86/hvm/svm/asid.c                  |  6 ++---
 xen/arch/x86/hvm/svm/nestedsvm.c             | 22 +++++++++---------
 xen/arch/x86/hvm/svm/svm.c                   |  2 +-
 xen/arch/x86/hvm/svm/svmdebug.c              |  7 ++++--
 xen/arch/x86/hvm/svm/vmcb.c                  |  2 +-
 xen/arch/x86/include/asm/hvm/svm/nestedsvm.h |  2 +-
 xen/arch/x86/include/asm/hvm/svm/vmcb.h      | 24 ++++++++++----------
 7 files changed, 34 insertions(+), 31 deletions(-)

-- 
2.44.0

[PATCH v2 1/3] x86/svm: Drop the _enabled suffix from vmcb bits

[Vaishali Thakkar]

The suffix is redundant for np/sev/sev-es bits. Drop it
to avoid adding extra code volume. While we're here, drop
the double negations in one of the instances of _np bit
and replace 0/1 with false/true in the use cases of _np.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Vaishali Thakkar <vaishali.thakkar@vates.tech>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v1:
        - Address Andrew and Jan's reviews related to dropping
          double negation and replacing 0/1 with false/true
        - Fix the typo around signed-off-by
---
 xen/arch/x86/hvm/svm/nestedsvm.c        | 14 +++++++-------
 xen/arch/x86/hvm/svm/svm.c              |  2 +-
 xen/arch/x86/hvm/svm/vmcb.c             |  2 +-
 xen/arch/x86/include/asm/hvm/svm/vmcb.h | 18 +++++++++---------
 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index e4e01add8c..37548cf05c 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -571,7 +571,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
     if ( nestedhvm_paging_mode_hap(v) )
     {
         /* host nested paging + guest nested paging. */
-        n2vmcb->_np_enable = 1;
+        n2vmcb->_np = true;
 
         nestedsvm_vmcb_set_nestedp2m(v, ns_vmcb, n2vmcb);
 
@@ -585,7 +585,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
     else if ( paging_mode_hap(v->domain) )
     {
         /* host nested paging + guest shadow paging. */
-        n2vmcb->_np_enable = 1;
+        n2vmcb->_np = true;
         /* Keep h_cr3 as it is. */
         n2vmcb->_h_cr3 = n1vmcb->_h_cr3;
         /* When l1 guest does shadow paging
@@ -601,7 +601,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
     else
     {
         /* host shadow paging + guest shadow paging. */
-        n2vmcb->_np_enable = 0;
+        n2vmcb->_np = false;
         n2vmcb->_h_cr3 = 0x0;
 
         /* TODO: Once shadow-shadow paging is in place come back to here
@@ -706,7 +706,7 @@ nsvm_vcpu_vmentry(struct vcpu *v, struct cpu_user_regs *regs,
     }
 
     /* nested paging for the guest */
-    svm->ns_hap_enabled = !!ns_vmcb->_np_enable;
+    svm->ns_hap_enabled = ns_vmcb->_np;
 
     /* Remember the V_INTR_MASK in hostflags */
     svm->ns_hostflags.fields.vintrmask = !!ns_vmcb->_vintr.fields.intr_masking;
@@ -1084,7 +1084,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
     if ( nestedhvm_paging_mode_hap(v) )
     {
         /* host nested paging + guest nested paging. */
-        ns_vmcb->_np_enable = n2vmcb->_np_enable;
+        ns_vmcb->_np = n2vmcb->_np;
         ns_vmcb->_cr3 = n2vmcb->_cr3;
         /* The vmcb->h_cr3 is the shadowed h_cr3. The original
          * unshadowed guest h_cr3 is kept in ns_vmcb->h_cr3,
@@ -1093,7 +1093,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
     else if ( paging_mode_hap(v->domain) )
     {
         /* host nested paging + guest shadow paging. */
-        ns_vmcb->_np_enable = 0;
+        ns_vmcb->_np = false;
         /* Throw h_cr3 away. Guest is not allowed to set it or
          * it can break out, otherwise (security hole!) */
         ns_vmcb->_h_cr3 = 0x0;
@@ -1104,7 +1104,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
     else
     {
         /* host shadow paging + guest shadow paging. */
-        ns_vmcb->_np_enable = 0;
+        ns_vmcb->_np = false;
         ns_vmcb->_h_cr3 = 0x0;
         /* The vmcb->_cr3 is the shadowed cr3. The original
          * unshadowed guest cr3 is kept in ns_vmcb->_cr3,
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index b551eac807..1b38f6a494 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -473,7 +473,7 @@ static int svm_vmcb_restore(struct vcpu *v, struct hvm_hw_cpu *c)
 
     if ( paging_mode_hap(v->domain) )
     {
-        vmcb_set_np_enable(vmcb, 1);
+        vmcb_set_np(vmcb, 1);
         vmcb_set_g_pat(vmcb, MSR_IA32_CR_PAT_RESET /* guest PAT */);
         vmcb_set_h_cr3(vmcb, pagetable_get_paddr(p2m_get_pagetable(p2m)));
     }
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 282fe7cdbe..770a0228d4 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -133,7 +133,7 @@ static int construct_vmcb(struct vcpu *v)
 
     if ( paging_mode_hap(v->domain) )
     {
-        vmcb->_np_enable = 1; /* enable nested paging */
+        vmcb->_np = true; /* enable nested paging */
         vmcb->_g_pat = MSR_IA32_CR_PAT_RESET; /* guest PAT */
         vmcb->_h_cr3 = pagetable_get_paddr(
             p2m_get_pagetable(p2m_get_hostp2m(v->domain)));
diff --git a/xen/arch/x86/include/asm/hvm/svm/vmcb.h b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
index 91221ff4e2..76507576ba 100644
--- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h
+++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
@@ -473,12 +473,12 @@ struct vmcb_struct {
     intinfo_t exit_int_info;    /* offset 0x88 */
     union {                     /* offset 0x90 - cleanbit 4 */
         struct {
-            bool _np_enable     :1;
-            bool _sev_enable    :1;
-            bool _sev_es_enable :1;
-            bool _gmet          :1;
-            bool _np_sss        :1;
-            bool _vte           :1;
+            bool _np        :1;
+            bool _sev       :1;
+            bool _sev_es    :1;
+            bool _gmet      :1;
+            bool _np_sss    :1;
+            bool _vte       :1;
         };
         uint64_t _np_ctrl;
     };
@@ -645,9 +645,9 @@ VMCB_ACCESSORS(msrpm_base_pa, iopm)
 VMCB_ACCESSORS(guest_asid, asid)
 VMCB_ACCESSORS(vintr, tpr)
 VMCB_ACCESSORS(np_ctrl, np)
-VMCB_ACCESSORS_(np_enable, bool, np)
-VMCB_ACCESSORS_(sev_enable, bool, np)
-VMCB_ACCESSORS_(sev_es_enable, bool, np)
+VMCB_ACCESSORS_(np, bool, np)
+VMCB_ACCESSORS_(sev, bool, np)
+VMCB_ACCESSORS_(sev_es, bool, np)
 VMCB_ACCESSORS_(gmet, bool, np)
 VMCB_ACCESSORS_(vte, bool, np)
 VMCB_ACCESSORS(h_cr3, np)
-- 
2.44.0

[PATCH v2 2/3] x86/svm: Drop the suffix _guest from vmcb bit

[Vaishali Thakkar]

The suffix _guest is redundant for asid bit. Drop it
to avoid adding extra code volume.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Vaishali Thakkar <vaishali.thakkar@vates.tech>
---
Changes since v1:
        - This patch wasn't part of v1. It's been added to
          address Andrew's suggestion.
---
 xen/arch/x86/hvm/svm/asid.c                  | 6 +++---
 xen/arch/x86/hvm/svm/nestedsvm.c             | 8 ++++----
 xen/arch/x86/hvm/svm/svmdebug.c              | 4 ++--
 xen/arch/x86/include/asm/hvm/svm/nestedsvm.h | 2 +-
 xen/arch/x86/include/asm/hvm/svm/vmcb.h      | 6 +++---
 5 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/asid.c b/xen/arch/x86/hvm/svm/asid.c
index 28e0dbc176..d5f70f8848 100644
--- a/xen/arch/x86/hvm/svm/asid.c
+++ b/xen/arch/x86/hvm/svm/asid.c
@@ -37,14 +37,14 @@ void svm_asid_handle_vmrun(void)
     /* ASID 0 indicates that ASIDs are disabled. */
     if ( p_asid->asid == 0 )
     {
-        vmcb_set_guest_asid(vmcb, 1);
+        vmcb_set_asid(vmcb, 1);
         vmcb->tlb_control =
             cpu_has_svm_flushbyasid ? TLB_CTRL_FLUSH_ASID : TLB_CTRL_FLUSH_ALL;
         return;
     }
 
-    if ( vmcb_get_guest_asid(vmcb) != p_asid->asid )
-        vmcb_set_guest_asid(vmcb, p_asid->asid);
+    if ( vmcb_get_asid(vmcb) != p_asid->asid )
+        vmcb_set_asid(vmcb, p_asid->asid);
 
     vmcb->tlb_control =
         !need_flush ? TLB_CTRL_NO_FLUSH :
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 37548cf05c..adbd49b5a2 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -157,7 +157,7 @@ int cf_check nsvm_vcpu_reset(struct vcpu *v)
     svm->ns_hap_enabled = 0;
     svm->ns_vmcb_guestcr3 = 0;
     svm->ns_vmcb_hostcr3 = 0;
-    svm->ns_guest_asid = 0;
+    svm->ns_asid = 0;
     svm->ns_hostflags.bytes = 0;
     svm->ns_vmexit.exitinfo1 = 0;
     svm->ns_vmexit.exitinfo2 = 0;
@@ -698,11 +698,11 @@ nsvm_vcpu_vmentry(struct vcpu *v, struct cpu_user_regs *regs,
     /* Convert explicitely to boolean. Deals with l1 guests
      * that use flush-by-asid w/o checking the cpuid bits */
     nv->nv_flushp2m = !!ns_vmcb->tlb_control;
-    if ( svm->ns_guest_asid != ns_vmcb->_guest_asid )
+    if ( svm->ns_asid != ns_vmcb->_asid )
     {
         nv->nv_flushp2m = 1;
         hvm_asid_flush_vcpu_asid(&vcpu_nestedhvm(v).nv_n2asid);
-        svm->ns_guest_asid = ns_vmcb->_guest_asid;
+        svm->ns_asid = ns_vmcb->_asid;
     }
 
     /* nested paging for the guest */
@@ -1046,7 +1046,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
     /* Keep it. It's maintainted by the l1 guest. */
 
     /* ASID */
-    /* ns_vmcb->_guest_asid = n2vmcb->_guest_asid; */
+    /* ns_vmcb->_asid = n2vmcb->_asid; */
 
     /* TLB control */
     ns_vmcb->tlb_control = 0;
diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c
index 24358c6eea..0d714c728c 100644
--- a/xen/arch/x86/hvm/svm/svmdebug.c
+++ b/xen/arch/x86/hvm/svm/svmdebug.c
@@ -51,8 +51,8 @@ void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb)
            vmcb->exitcode, vmcb->exit_int_info.raw);
     printk("exitinfo1 = %#"PRIx64" exitinfo2 = %#"PRIx64"\n",
            vmcb->exitinfo1, vmcb->exitinfo2);
-    printk("np_ctrl = %#"PRIx64" guest_asid = %#x\n",
-           vmcb_get_np_ctrl(vmcb), vmcb_get_guest_asid(vmcb));
+    printk("np_ctrl = %#"PRIx64" asid = %#x\n",
+           vmcb_get_np_ctrl(vmcb), vmcb_get_asid(vmcb));
     printk("virtual vmload/vmsave = %d, virt_ext = %#"PRIx64"\n",
            vmcb->virt_ext.fields.vloadsave_enable, vmcb->virt_ext.bytes);
     printk("cpl = %d efer = %#"PRIx64" star = %#"PRIx64" lstar = %#"PRIx64"\n",
diff --git a/xen/arch/x86/include/asm/hvm/svm/nestedsvm.h b/xen/arch/x86/include/asm/hvm/svm/nestedsvm.h
index 406fc082b1..7767cd6080 100644
--- a/xen/arch/x86/include/asm/hvm/svm/nestedsvm.h
+++ b/xen/arch/x86/include/asm/hvm/svm/nestedsvm.h
@@ -51,7 +51,7 @@ struct nestedsvm {
      *     the l1 guest nested page table
      */
     uint64_t ns_vmcb_guestcr3, ns_vmcb_hostcr3;
-    uint32_t ns_guest_asid;
+    uint32_t ns_asid;
 
     bool ns_hap_enabled;
 
diff --git a/xen/arch/x86/include/asm/hvm/svm/vmcb.h b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
index 76507576ba..5d539fcdcf 100644
--- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h
+++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
@@ -383,7 +383,7 @@ typedef union
         bool intercepts:1; /* 0:  cr/dr/exception/general intercepts,
                             *     pause_filter_count, tsc_offset */
         bool iopm:1;       /* 1:  iopm_base_pa, msrpm_base_pa */
-        bool asid:1;       /* 2:  guest_asid */
+        bool asid:1;       /* 2:  asid */
         bool tpr:1;        /* 3:  vintr */
         bool np:1;         /* 4:  np_enable, h_cr3, g_pat */
         bool cr:1;         /* 5:  cr0, cr3, cr4, efer */
@@ -413,7 +413,7 @@ struct vmcb_struct {
     u64 _iopm_base_pa;          /* offset 0x40 - cleanbit 1 */
     u64 _msrpm_base_pa;         /* offset 0x48 - cleanbit 1 */
     u64 _tsc_offset;            /* offset 0x50 - cleanbit 0 */
-    u32 _guest_asid;            /* offset 0x58 - cleanbit 2 */
+    u32 _asid;                  /* offset 0x58 - cleanbit 2 */
     u8  tlb_control;            /* offset 0x5C - TLB_CTRL_* */
     u8  res07[3];
     vintr_t _vintr;             /* offset 0x60 - cleanbit 3 */
@@ -642,7 +642,7 @@ VMCB_ACCESSORS(pause_filter_thresh, intercepts)
 VMCB_ACCESSORS(tsc_offset, intercepts)
 VMCB_ACCESSORS(iopm_base_pa, iopm)
 VMCB_ACCESSORS(msrpm_base_pa, iopm)
-VMCB_ACCESSORS(guest_asid, asid)
+VMCB_ACCESSORS(asid, asid)
 VMCB_ACCESSORS(vintr, tpr)
 VMCB_ACCESSORS(np_ctrl, np)
 VMCB_ACCESSORS_(np, bool, np)
-- 
2.44.0

[PATCH v2 3/3] x86/svmdebug: Print sev and sev_es vmcb bits

[Vaishali Thakkar]

While sev and sev_es bits are not yet enabled in xen,
including their status in the VMCB dump could be
informational.Therefore, print it via svmdebug.

Signed-off-by: Vaishali Thakkar <vaishali.thakkar@vates.tech>
---
Changes since v1:
        - Pretty printing
---
 xen/arch/x86/hvm/svm/svmdebug.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c
index 0d714c728c..ba5fa40071 100644
--- a/xen/arch/x86/hvm/svm/svmdebug.c
+++ b/xen/arch/x86/hvm/svm/svmdebug.c
@@ -51,8 +51,11 @@ void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb)
            vmcb->exitcode, vmcb->exit_int_info.raw);
     printk("exitinfo1 = %#"PRIx64" exitinfo2 = %#"PRIx64"\n",
            vmcb->exitinfo1, vmcb->exitinfo2);
-    printk("np_ctrl = %#"PRIx64" asid = %#x\n",
-           vmcb_get_np_ctrl(vmcb), vmcb_get_asid(vmcb));
+    printk("asid = %#x np_ctrl = %#"PRIx64" - %s%s%s\n",
+           vmcb_get_asid(vmcb), vmcb_get_np_ctrl(vmcb),
+           vmcb_get_np(vmcb)     ? "NP"     : "",
+           vmcb_get_sev(vmcb)    ? "SEV"    : "",
+           vmcb_get_sev_es(vmcb) ? "SEV_ES" : "");
     printk("virtual vmload/vmsave = %d, virt_ext = %#"PRIx64"\n",
            vmcb->virt_ext.fields.vloadsave_enable, vmcb->virt_ext.bytes);
     printk("cpl = %d efer = %#"PRIx64" star = %#"PRIx64" lstar = %#"PRIx64"\n",
-- 
2.44.0

Re: [PATCH v2 1/3] x86/svm: Drop the _enabled suffix from vmcb bits

[Jan Beulich]

On 11.03.2024 13:40, Vaishali Thakkar wrote:
> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -571,7 +571,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
>      if ( nestedhvm_paging_mode_hap(v) )
>      {
>          /* host nested paging + guest nested paging. */
> -        n2vmcb->_np_enable = 1;
> +        n2vmcb->_np = true;
>  
>          nestedsvm_vmcb_set_nestedp2m(v, ns_vmcb, n2vmcb);
>  
> @@ -585,7 +585,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
>      else if ( paging_mode_hap(v->domain) )
>      {
>          /* host nested paging + guest shadow paging. */
> -        n2vmcb->_np_enable = 1;
> +        n2vmcb->_np = true;
>          /* Keep h_cr3 as it is. */
>          n2vmcb->_h_cr3 = n1vmcb->_h_cr3;
>          /* When l1 guest does shadow paging
> @@ -601,7 +601,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
>      else
>      {
>          /* host shadow paging + guest shadow paging. */
> -        n2vmcb->_np_enable = 0;
> +        n2vmcb->_np = false;
>          n2vmcb->_h_cr3 = 0x0;
>  
>          /* TODO: Once shadow-shadow paging is in place come back to here
> @@ -706,7 +706,7 @@ nsvm_vcpu_vmentry(struct vcpu *v, struct cpu_user_regs *regs,
>      }
>  
>      /* nested paging for the guest */
> -    svm->ns_hap_enabled = !!ns_vmcb->_np_enable;
> +    svm->ns_hap_enabled = ns_vmcb->_np;
>  
>      /* Remember the V_INTR_MASK in hostflags */
>      svm->ns_hostflags.fields.vintrmask = !!ns_vmcb->_vintr.fields.intr_masking;
> @@ -1084,7 +1084,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>      if ( nestedhvm_paging_mode_hap(v) )
>      {
>          /* host nested paging + guest nested paging. */
> -        ns_vmcb->_np_enable = n2vmcb->_np_enable;
> +        ns_vmcb->_np = n2vmcb->_np;
>          ns_vmcb->_cr3 = n2vmcb->_cr3;
>          /* The vmcb->h_cr3 is the shadowed h_cr3. The original
>           * unshadowed guest h_cr3 is kept in ns_vmcb->h_cr3,
> @@ -1093,7 +1093,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>      else if ( paging_mode_hap(v->domain) )
>      {
>          /* host nested paging + guest shadow paging. */
> -        ns_vmcb->_np_enable = 0;
> +        ns_vmcb->_np = false;
>          /* Throw h_cr3 away. Guest is not allowed to set it or
>           * it can break out, otherwise (security hole!) */
>          ns_vmcb->_h_cr3 = 0x0;
> @@ -1104,7 +1104,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>      else
>      {
>          /* host shadow paging + guest shadow paging. */
> -        ns_vmcb->_np_enable = 0;
> +        ns_vmcb->_np = false;
>          ns_vmcb->_h_cr3 = 0x0;
>          /* The vmcb->_cr3 is the shadowed cr3. The original
>           * unshadowed guest cr3 is kept in ns_vmcb->_cr3,

While spotting the small issue below it occurred to me: Why is it that
vmcb_set_...() is open-coded everywhere here? I think this would be
pretty nice to avoid at the same time (for lines touched anyway, or in
a separate prereq patch, or alternatively [and only ideally] for all
other instances in a follow-on patch). Thoughts?

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -473,7 +473,7 @@ static int svm_vmcb_restore(struct vcpu *v, struct hvm_hw_cpu *c)
>  
>      if ( paging_mode_hap(v->domain) )
>      {
> -        vmcb_set_np_enable(vmcb, 1);
> +        vmcb_set_np(vmcb, 1);

No switching to "true" here? (If the answer to the other question is
"No" for whatever reason, I'd nevertheless like to see this on adjusted,
which could then be done while committing.)

Jan

Re: [PATCH v2 2/3] x86/svm: Drop the suffix _guest from vmcb bit

[Jan Beulich]

On 11.03.2024 13:40, Vaishali Thakkar wrote:
> @@ -698,11 +698,11 @@ nsvm_vcpu_vmentry(struct vcpu *v, struct cpu_user_regs *regs,
>      /* Convert explicitely to boolean. Deals with l1 guests
>       * that use flush-by-asid w/o checking the cpuid bits */
>      nv->nv_flushp2m = !!ns_vmcb->tlb_control;
> -    if ( svm->ns_guest_asid != ns_vmcb->_guest_asid )
> +    if ( svm->ns_asid != ns_vmcb->_asid )
>      {
>          nv->nv_flushp2m = 1;
>          hvm_asid_flush_vcpu_asid(&vcpu_nestedhvm(v).nv_n2asid);
> -        svm->ns_guest_asid = ns_vmcb->_guest_asid;
> +        svm->ns_asid = ns_vmcb->_asid;
>      }
>  
>      /* nested paging for the guest */
> @@ -1046,7 +1046,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>      /* Keep it. It's maintainted by the l1 guest. */
>  
>      /* ASID */
> -    /* ns_vmcb->_guest_asid = n2vmcb->_guest_asid; */
> +    /* ns_vmcb->_asid = n2vmcb->_asid; */

Unlike in the earlier patch, where I could accept the request to switch
to using accessor functions as scope-creep-ish, here I'm pretty firm
with my request to stop their open-coding at the same time. Unless of
course there's a technical reason the accessors cannot be used here.

Jan

Re: [PATCH v2 3/3] x86/svmdebug: Print sev and sev_es vmcb bits

[Jan Beulich]

On 11.03.2024 13:40, Vaishali Thakkar wrote:
> While sev and sev_es bits are not yet enabled in xen,
> including their status in the VMCB dump could be
> informational.Therefore, print it via svmdebug.

Yet there are more bits there. I'm okay with leaving off printing of
them here, but it wants clarifying why some are printed and some are
not.

> --- a/xen/arch/x86/hvm/svm/svmdebug.c
> +++ b/xen/arch/x86/hvm/svm/svmdebug.c
> @@ -51,8 +51,11 @@ void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb)
>             vmcb->exitcode, vmcb->exit_int_info.raw);
>      printk("exitinfo1 = %#"PRIx64" exitinfo2 = %#"PRIx64"\n",
>             vmcb->exitinfo1, vmcb->exitinfo2);
> -    printk("np_ctrl = %#"PRIx64" asid = %#x\n",
> -           vmcb_get_np_ctrl(vmcb), vmcb_get_asid(vmcb));
> +    printk("asid = %#x np_ctrl = %#"PRIx64" - %s%s%s\n",
> +           vmcb_get_asid(vmcb), vmcb_get_np_ctrl(vmcb),
> +           vmcb_get_np(vmcb)     ? "NP"     : "",
> +           vmcb_get_sev(vmcb)    ? "SEV"    : "",
> +           vmcb_get_sev_es(vmcb) ? "SEV_ES" : "");

Each of these three string literals needs a leading blank as separator.
In exchange the one in the format string immediately after '-' then
will want dropping. That'll still lead to slightly odd output if none
of the bits is set; imo it would be slightly less odd if you used

    printk("asid = %#x np_ctrl = %#"PRIx64":%s%s%s\n",

instead.

Jan

Re: [PATCH v2 1/3] x86/svm: Drop the _enabled suffix from vmcb bits

[Vaishali Thakkar]

On 3/12/24 08:54, Jan Beulich wrote:
> On 11.03.2024 13:40, Vaishali Thakkar wrote:
>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>> @@ -571,7 +571,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
>>       if ( nestedhvm_paging_mode_hap(v) )
>>       {
>>           /* host nested paging + guest nested paging. */
>> -        n2vmcb->_np_enable = 1;
>> +        n2vmcb->_np = true;
>>   
>>           nestedsvm_vmcb_set_nestedp2m(v, ns_vmcb, n2vmcb);
>>   
>> @@ -585,7 +585,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
>>       else if ( paging_mode_hap(v->domain) )
>>       {
>>           /* host nested paging + guest shadow paging. */
>> -        n2vmcb->_np_enable = 1;
>> +        n2vmcb->_np = true;
>>           /* Keep h_cr3 as it is. */
>>           n2vmcb->_h_cr3 = n1vmcb->_h_cr3;
>>           /* When l1 guest does shadow paging
>> @@ -601,7 +601,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
>>       else
>>       {
>>           /* host shadow paging + guest shadow paging. */
>> -        n2vmcb->_np_enable = 0;
>> +        n2vmcb->_np = false;
>>           n2vmcb->_h_cr3 = 0x0;
>>   
>>           /* TODO: Once shadow-shadow paging is in place come back to here
>> @@ -706,7 +706,7 @@ nsvm_vcpu_vmentry(struct vcpu *v, struct cpu_user_regs *regs,
>>       }
>>   
>>       /* nested paging for the guest */
>> -    svm->ns_hap_enabled = !!ns_vmcb->_np_enable;
>> +    svm->ns_hap_enabled = ns_vmcb->_np;
>>   
>>       /* Remember the V_INTR_MASK in hostflags */
>>       svm->ns_hostflags.fields.vintrmask = !!ns_vmcb->_vintr.fields.intr_masking;
>> @@ -1084,7 +1084,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>       if ( nestedhvm_paging_mode_hap(v) )
>>       {
>>           /* host nested paging + guest nested paging. */
>> -        ns_vmcb->_np_enable = n2vmcb->_np_enable;
>> +        ns_vmcb->_np = n2vmcb->_np;
>>           ns_vmcb->_cr3 = n2vmcb->_cr3;
>>           /* The vmcb->h_cr3 is the shadowed h_cr3. The original
>>            * unshadowed guest h_cr3 is kept in ns_vmcb->h_cr3,
>> @@ -1093,7 +1093,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>       else if ( paging_mode_hap(v->domain) )
>>       {
>>           /* host nested paging + guest shadow paging. */
>> -        ns_vmcb->_np_enable = 0;
>> +        ns_vmcb->_np = false;
>>           /* Throw h_cr3 away. Guest is not allowed to set it or
>>            * it can break out, otherwise (security hole!) */
>>           ns_vmcb->_h_cr3 = 0x0;
>> @@ -1104,7 +1104,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>       else
>>       {
>>           /* host shadow paging + guest shadow paging. */
>> -        ns_vmcb->_np_enable = 0;
>> +        ns_vmcb->_np = false;
>>           ns_vmcb->_h_cr3 = 0x0;
>>           /* The vmcb->_cr3 is the shadowed cr3. The original
>>            * unshadowed guest cr3 is kept in ns_vmcb->_cr3,
> 
> While spotting the small issue below it occurred to me: Why is it that
> vmcb_set_...() is open-coded everywhere here? I think this would be
> pretty nice to avoid at the same time (for lines touched anyway, or in
> a separate prereq patch, or alternatively [and only ideally] for all
> other instances in a follow-on patch). Thoughts?

Yes, I noticed this too. My plan was to send a followup patch for
fixing all the instances where vmcb_set/get_...() can be used.
There are bunch of other vmcb bits (apart from the ones being
handled in this patchset) in this file and in svm.c which can
benefit from using VMCB accessors.

>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -473,7 +473,7 @@ static int svm_vmcb_restore(struct vcpu *v, struct hvm_hw_cpu *c)
>>   
>>       if ( paging_mode_hap(v->domain) )
>>       {
>> -        vmcb_set_np_enable(vmcb, 1);
>> +        vmcb_set_np(vmcb, 1);
> 
> No switching to "true" here? (If the answer to the other question is
> "No" for whatever reason, I'd nevertheless like to see this on adjusted,
> which could then be done while committing.)

Sorry, I missed this instance. I'll fix it if I'll need to send another
revised patchset for some other fixes (based on review comments), else
feel free to fix it while committing. Thanks.

> Jan

Re: [PATCH v2 2/3] x86/svm: Drop the suffix _guest from vmcb bit

[Vaishali Thakkar]

On 3/12/24 08:59, Jan Beulich wrote:
> On 11.03.2024 13:40, Vaishali Thakkar wrote:
>> @@ -698,11 +698,11 @@ nsvm_vcpu_vmentry(struct vcpu *v, struct cpu_user_regs *regs,
>>       /* Convert explicitely to boolean. Deals with l1 guests
>>        * that use flush-by-asid w/o checking the cpuid bits */
>>       nv->nv_flushp2m = !!ns_vmcb->tlb_control;
>> -    if ( svm->ns_guest_asid != ns_vmcb->_guest_asid )
>> +    if ( svm->ns_asid != ns_vmcb->_asid )
>>       {
>>           nv->nv_flushp2m = 1;
>>           hvm_asid_flush_vcpu_asid(&vcpu_nestedhvm(v).nv_n2asid);
>> -        svm->ns_guest_asid = ns_vmcb->_guest_asid;
>> +        svm->ns_asid = ns_vmcb->_asid;
>>       }
>>   
>>       /* nested paging for the guest */
>> @@ -1046,7 +1046,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>       /* Keep it. It's maintainted by the l1 guest. */
>>   
>>       /* ASID */
>> -    /* ns_vmcb->_guest_asid = n2vmcb->_guest_asid; */
>> +    /* ns_vmcb->_asid = n2vmcb->_asid; */
> 
> Unlike in the earlier patch, where I could accept the request to switch
> to using accessor functions as scope-creep-ish, here I'm pretty firm
> with my request to stop their open-coding at the same time. Unless of
> course there's a technical reason the accessors cannot be used here.

Yes, so as mentioned in the other patch's reply, I plan to tackle this 
instance too in the followup patchset along with others. So, if you're
fine with it, I'll leave this one here for now. Unless you prefer otherwise.

> Jan

Re: [PATCH v2 1/3] x86/svm: Drop the _enabled suffix from vmcb bits

[Jan Beulich]

On 12.03.2024 11:00, Vaishali Thakkar wrote:
> On 3/12/24 08:54, Jan Beulich wrote:
>> On 11.03.2024 13:40, Vaishali Thakkar wrote:
>>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>>> @@ -571,7 +571,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
>>>       if ( nestedhvm_paging_mode_hap(v) )
>>>       {
>>>           /* host nested paging + guest nested paging. */
>>> -        n2vmcb->_np_enable = 1;
>>> +        n2vmcb->_np = true;
>>>   
>>>           nestedsvm_vmcb_set_nestedp2m(v, ns_vmcb, n2vmcb);
>>>   
>>> @@ -585,7 +585,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
>>>       else if ( paging_mode_hap(v->domain) )
>>>       {
>>>           /* host nested paging + guest shadow paging. */
>>> -        n2vmcb->_np_enable = 1;
>>> +        n2vmcb->_np = true;
>>>           /* Keep h_cr3 as it is. */
>>>           n2vmcb->_h_cr3 = n1vmcb->_h_cr3;
>>>           /* When l1 guest does shadow paging
>>> @@ -601,7 +601,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
>>>       else
>>>       {
>>>           /* host shadow paging + guest shadow paging. */
>>> -        n2vmcb->_np_enable = 0;
>>> +        n2vmcb->_np = false;
>>>           n2vmcb->_h_cr3 = 0x0;
>>>   
>>>           /* TODO: Once shadow-shadow paging is in place come back to here
>>> @@ -706,7 +706,7 @@ nsvm_vcpu_vmentry(struct vcpu *v, struct cpu_user_regs *regs,
>>>       }
>>>   
>>>       /* nested paging for the guest */
>>> -    svm->ns_hap_enabled = !!ns_vmcb->_np_enable;
>>> +    svm->ns_hap_enabled = ns_vmcb->_np;
>>>   
>>>       /* Remember the V_INTR_MASK in hostflags */
>>>       svm->ns_hostflags.fields.vintrmask = !!ns_vmcb->_vintr.fields.intr_masking;
>>> @@ -1084,7 +1084,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>>       if ( nestedhvm_paging_mode_hap(v) )
>>>       {
>>>           /* host nested paging + guest nested paging. */
>>> -        ns_vmcb->_np_enable = n2vmcb->_np_enable;
>>> +        ns_vmcb->_np = n2vmcb->_np;
>>>           ns_vmcb->_cr3 = n2vmcb->_cr3;
>>>           /* The vmcb->h_cr3 is the shadowed h_cr3. The original
>>>            * unshadowed guest h_cr3 is kept in ns_vmcb->h_cr3,
>>> @@ -1093,7 +1093,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>>       else if ( paging_mode_hap(v->domain) )
>>>       {
>>>           /* host nested paging + guest shadow paging. */
>>> -        ns_vmcb->_np_enable = 0;
>>> +        ns_vmcb->_np = false;
>>>           /* Throw h_cr3 away. Guest is not allowed to set it or
>>>            * it can break out, otherwise (security hole!) */
>>>           ns_vmcb->_h_cr3 = 0x0;
>>> @@ -1104,7 +1104,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>>       else
>>>       {
>>>           /* host shadow paging + guest shadow paging. */
>>> -        ns_vmcb->_np_enable = 0;
>>> +        ns_vmcb->_np = false;
>>>           ns_vmcb->_h_cr3 = 0x0;
>>>           /* The vmcb->_cr3 is the shadowed cr3. The original
>>>            * unshadowed guest cr3 is kept in ns_vmcb->_cr3,
>>
>> While spotting the small issue below it occurred to me: Why is it that
>> vmcb_set_...() is open-coded everywhere here? I think this would be
>> pretty nice to avoid at the same time (for lines touched anyway, or in
>> a separate prereq patch, or alternatively [and only ideally] for all
>> other instances in a follow-on patch). Thoughts?
> 
> Yes, I noticed this too. My plan was to send a followup patch for
> fixing all the instances where vmcb_set/get_...() can be used.
> There are bunch of other vmcb bits (apart from the ones being
> handled in this patchset) in this file and in svm.c which can
> benefit from using VMCB accessors.

To keep churn as well as effort to find commits touching individual lines
low, doing the conversion when touching lines anyway is imo preferable. A
follow-on patch can then convert what's left.

Jan

Re: [PATCH v2 2/3] x86/svm: Drop the suffix _guest from vmcb bit

[Jan Beulich]

On 12.03.2024 11:08, Vaishali Thakkar wrote:
> On 3/12/24 08:59, Jan Beulich wrote:
>> On 11.03.2024 13:40, Vaishali Thakkar wrote:
>>> @@ -698,11 +698,11 @@ nsvm_vcpu_vmentry(struct vcpu *v, struct cpu_user_regs *regs,
>>>       /* Convert explicitely to boolean. Deals with l1 guests
>>>        * that use flush-by-asid w/o checking the cpuid bits */
>>>       nv->nv_flushp2m = !!ns_vmcb->tlb_control;
>>> -    if ( svm->ns_guest_asid != ns_vmcb->_guest_asid )
>>> +    if ( svm->ns_asid != ns_vmcb->_asid )
>>>       {
>>>           nv->nv_flushp2m = 1;
>>>           hvm_asid_flush_vcpu_asid(&vcpu_nestedhvm(v).nv_n2asid);
>>> -        svm->ns_guest_asid = ns_vmcb->_guest_asid;
>>> +        svm->ns_asid = ns_vmcb->_asid;
>>>       }
>>>   
>>>       /* nested paging for the guest */
>>> @@ -1046,7 +1046,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>>       /* Keep it. It's maintainted by the l1 guest. */
>>>   
>>>       /* ASID */
>>> -    /* ns_vmcb->_guest_asid = n2vmcb->_guest_asid; */
>>> +    /* ns_vmcb->_asid = n2vmcb->_asid; */
>>
>> Unlike in the earlier patch, where I could accept the request to switch
>> to using accessor functions as scope-creep-ish, here I'm pretty firm
>> with my request to stop their open-coding at the same time. Unless of
>> course there's a technical reason the accessors cannot be used here.
> 
> Yes, so as mentioned in the other patch's reply, I plan to tackle this 
> instance too in the followup patchset along with others. So, if you're
> fine with it, I'll leave this one here for now. Unless you prefer otherwise.

I thought I said pretty clearly that here I'm stronger with my request
than on the other patch.

Jan

Re: [PATCH v2 1/3] x86/svm: Drop the _enabled suffix from vmcb bits

[Vaishali Thakkar]

On 3/12/24 11:49, Jan Beulich wrote:
> On 12.03.2024 11:00, Vaishali Thakkar wrote:
>> On 3/12/24 08:54, Jan Beulich wrote:
>>> On 11.03.2024 13:40, Vaishali Thakkar wrote:
>>>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>>>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>>>> @@ -571,7 +571,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
>>>>        if ( nestedhvm_paging_mode_hap(v) )
>>>>        {
>>>>            /* host nested paging + guest nested paging. */
>>>> -        n2vmcb->_np_enable = 1;
>>>> +        n2vmcb->_np = true;
>>>>    
>>>>            nestedsvm_vmcb_set_nestedp2m(v, ns_vmcb, n2vmcb);
>>>>    
>>>> @@ -585,7 +585,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
>>>>        else if ( paging_mode_hap(v->domain) )
>>>>        {
>>>>            /* host nested paging + guest shadow paging. */
>>>> -        n2vmcb->_np_enable = 1;
>>>> +        n2vmcb->_np = true;
>>>>            /* Keep h_cr3 as it is. */
>>>>            n2vmcb->_h_cr3 = n1vmcb->_h_cr3;
>>>>            /* When l1 guest does shadow paging
>>>> @@ -601,7 +601,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
>>>>        else
>>>>        {
>>>>            /* host shadow paging + guest shadow paging. */
>>>> -        n2vmcb->_np_enable = 0;
>>>> +        n2vmcb->_np = false;
>>>>            n2vmcb->_h_cr3 = 0x0;
>>>>    
>>>>            /* TODO: Once shadow-shadow paging is in place come back to here
>>>> @@ -706,7 +706,7 @@ nsvm_vcpu_vmentry(struct vcpu *v, struct cpu_user_regs *regs,
>>>>        }
>>>>    
>>>>        /* nested paging for the guest */
>>>> -    svm->ns_hap_enabled = !!ns_vmcb->_np_enable;
>>>> +    svm->ns_hap_enabled = ns_vmcb->_np;
>>>>    
>>>>        /* Remember the V_INTR_MASK in hostflags */
>>>>        svm->ns_hostflags.fields.vintrmask = !!ns_vmcb->_vintr.fields.intr_masking;
>>>> @@ -1084,7 +1084,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>>>        if ( nestedhvm_paging_mode_hap(v) )
>>>>        {
>>>>            /* host nested paging + guest nested paging. */
>>>> -        ns_vmcb->_np_enable = n2vmcb->_np_enable;
>>>> +        ns_vmcb->_np = n2vmcb->_np;
>>>>            ns_vmcb->_cr3 = n2vmcb->_cr3;
>>>>            /* The vmcb->h_cr3 is the shadowed h_cr3. The original
>>>>             * unshadowed guest h_cr3 is kept in ns_vmcb->h_cr3,
>>>> @@ -1093,7 +1093,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>>>        else if ( paging_mode_hap(v->domain) )
>>>>        {
>>>>            /* host nested paging + guest shadow paging. */
>>>> -        ns_vmcb->_np_enable = 0;
>>>> +        ns_vmcb->_np = false;
>>>>            /* Throw h_cr3 away. Guest is not allowed to set it or
>>>>             * it can break out, otherwise (security hole!) */
>>>>            ns_vmcb->_h_cr3 = 0x0;
>>>> @@ -1104,7 +1104,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>>>        else
>>>>        {
>>>>            /* host shadow paging + guest shadow paging. */
>>>> -        ns_vmcb->_np_enable = 0;
>>>> +        ns_vmcb->_np = false;
>>>>            ns_vmcb->_h_cr3 = 0x0;
>>>>            /* The vmcb->_cr3 is the shadowed cr3. The original
>>>>             * unshadowed guest cr3 is kept in ns_vmcb->_cr3,
>>>
>>> While spotting the small issue below it occurred to me: Why is it that
>>> vmcb_set_...() is open-coded everywhere here? I think this would be
>>> pretty nice to avoid at the same time (for lines touched anyway, or in
>>> a separate prereq patch, or alternatively [and only ideally] for all
>>> other instances in a follow-on patch). Thoughts?
>>
>> Yes, I noticed this too. My plan was to send a followup patch for
>> fixing all the instances where vmcb_set/get_...() can be used.
>> There are bunch of other vmcb bits (apart from the ones being
>> handled in this patchset) in this file and in svm.c which can
>> benefit from using VMCB accessors.
> 
> To keep churn as well as effort to find commits touching individual lines
> low, doing the conversion when touching lines anyway is imo preferable. A
> follow-on patch can then convert what's left.

Alright, I'll replace open coding with VMCB accessors for the lines which 
are touched by this patchset. And others in a followup patchset.

> Jan

Re: [PATCH v2 3/3] x86/svmdebug: Print sev and sev_es vmcb bits

[Vaishali Thakkar]

On 3/12/24 09:05, Jan Beulich wrote:
> On 11.03.2024 13:40, Vaishali Thakkar wrote:
>> While sev and sev_es bits are not yet enabled in xen,
>> including their status in the VMCB dump could be
>> informational.Therefore, print it via svmdebug.
> 
> Yet there are more bits there. I'm okay with leaving off printing of
> them here, but it wants clarifying why some are printed and some are
> not.

Well, the idea is to print the bits that are either enabled or has WIP
to enable them. (e.g. sev and sev_es) I didn't include other bits as
I'm not sure if there is any WIP to enable them. Particularly including
sev and sev_es is useful for us while working on the enablement of it.

Does a commit log like the following makes it clear for you?

" Currently only raw _np_ctrl is being printed. It can
   be informational to know about which particular bits
   are enabled. So, this commit adds the bit-by-bit decode
   for np, sev and sev_es bits.

   Note that while only np is enabled in certain scenarios
   at the moment, work for enabling sev and sev_es is in
   progress. And it's useful to have this information as
   part of svmdebug. "

I'm also fine with including other bits here if that's preferred.

>> --- a/xen/arch/x86/hvm/svm/svmdebug.c
>> +++ b/xen/arch/x86/hvm/svm/svmdebug.c
>> @@ -51,8 +51,11 @@ void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb)
>>              vmcb->exitcode, vmcb->exit_int_info.raw);
>>       printk("exitinfo1 = %#"PRIx64" exitinfo2 = %#"PRIx64"\n",
>>              vmcb->exitinfo1, vmcb->exitinfo2);
>> -    printk("np_ctrl = %#"PRIx64" asid = %#x\n",
>> -           vmcb_get_np_ctrl(vmcb), vmcb_get_asid(vmcb));
>> +    printk("asid = %#x np_ctrl = %#"PRIx64" - %s%s%s\n",
>> +           vmcb_get_asid(vmcb), vmcb_get_np_ctrl(vmcb),
>> +           vmcb_get_np(vmcb)     ? "NP"     : "",
>> +           vmcb_get_sev(vmcb)    ? "SEV"    : "",
>> +           vmcb_get_sev_es(vmcb) ? "SEV_ES" : "");
> 
> Each of these three string literals needs a leading blank as separator.
> In exchange the one in the format string immediately after '-' then
> will want dropping. That'll still lead to slightly odd output if none
> of the bits is set; imo it would be slightly less odd if you used
> 
>      printk("asid = %#x np_ctrl = %#"PRIx64":%s%s%s\n",
> 
> instead.
> 
> Jan

Re: [PATCH v2 3/3] x86/svmdebug: Print sev and sev_es vmcb bits

[Jan Beulich]

On 13.03.2024 14:54, Vaishali Thakkar wrote:
> On 3/12/24 09:05, Jan Beulich wrote:
>> On 11.03.2024 13:40, Vaishali Thakkar wrote:
>>> While sev and sev_es bits are not yet enabled in xen,
>>> including their status in the VMCB dump could be
>>> informational.Therefore, print it via svmdebug.
>>
>> Yet there are more bits there. I'm okay with leaving off printing of
>> them here, but it wants clarifying why some are printed and some are
>> not.
> 
> Well, the idea is to print the bits that are either enabled or has WIP
> to enable them. (e.g. sev and sev_es) I didn't include other bits as
> I'm not sure if there is any WIP to enable them. Particularly including
> sev and sev_es is useful for us while working on the enablement of it.
> 
> Does a commit log like the following makes it clear for you?
> 
> " Currently only raw _np_ctrl is being printed. It can
>    be informational to know about which particular bits
>    are enabled. So, this commit adds the bit-by-bit decode
>    for np, sev and sev_es bits.
> 
>    Note that while only np is enabled in certain scenarios
>    at the moment, work for enabling sev and sev_es is in
>    progress. And it's useful to have this information as
>    part of svmdebug. "

I think that's sufficient, yes.

Jan