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

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

[Vaishali Thakkar]

Hi,

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

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

 xen/arch/x86/hvm/svm/nestedsvm.c        | 14 +++++++-------
 xen/arch/x86/hvm/svm/svm.c              |  2 +-
 xen/arch/x86/hvm/svm/svmdebug.c         |  2 ++
 xen/arch/x86/hvm/svm/vmcb.c             |  2 +-
 xen/arch/x86/include/asm/hvm/svm/vmcb.h | 18 +++++++++---------
 5 files changed, 20 insertions(+), 18 deletions(-)

-- 
2.44.0

[PATCH 1/2] 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.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Vaishali Thakkar <vaishali.thakkar@vates.tech>i
---
 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..7e285cf85a 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 = 1;
 
         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 = 1;
         /* 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 = 0;
         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 = 0;
         /* 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 = 0;
         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..a8dd87ca36 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 = 1; /* 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 2/2] 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>
---
JFYI, we'll send the follow-up patches with the enablement
of sev and ASP driver.
---
 xen/arch/x86/hvm/svm/svmdebug.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c
index 24358c6eea..f54b426fb3 100644
--- a/xen/arch/x86/hvm/svm/svmdebug.c
+++ b/xen/arch/x86/hvm/svm/svmdebug.c
@@ -53,6 +53,8 @@ void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb)
            vmcb->exitinfo1, vmcb->exitinfo2);
     printk("np_ctrl = %#"PRIx64" guest_asid = %#x\n",
            vmcb_get_np_ctrl(vmcb), vmcb_get_guest_asid(vmcb));
+    printk("sev = %d sev_es = %d\n",
+           vmcb_get_sev(vmcb), vmcb_get_sev_es(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",
-- 
2.44.0

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

[Andrew Cooper]

On 07/03/2024 9:40 pm, Vaishali Thakkar wrote:
> The suffix is redundant for np/sev/sev-es bits. 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>i

Typo on the end of your email address?

> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
> index e4e01add8c..7e285cf85a 100644
> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -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;

Because the type of is bool, the !! can be dropped too while changing
this line.

It seems that this was missing cleanup from f57ae00635 "SVM: split
_np_enable VMCB field".

Anyway, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> and I'm
happy to fix on commit.

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

[Andrew Cooper]

On 07/03/2024 9:40 pm, Vaishali Thakkar wrote:
> diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c
> index 24358c6eea..f54b426fb3 100644
> --- a/xen/arch/x86/hvm/svm/svmdebug.c
> +++ b/xen/arch/x86/hvm/svm/svmdebug.c
> @@ -53,6 +53,8 @@ void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb)
>             vmcb->exitinfo1, vmcb->exitinfo2);
>      printk("np_ctrl = %#"PRIx64" guest_asid = %#x\n",
>             vmcb_get_np_ctrl(vmcb), vmcb_get_guest_asid(vmcb));
> +    printk("sev = %d sev_es = %d\n",
> +           vmcb_get_sev(vmcb), vmcb_get_sev_es(vmcb));

Hmm.  These are covered by the previous line printing all of np_ctrl. 
What about rearranging the previous line to be something like:

printk("asid: %#x, np_ctrl: %#"PRIx64" -%s%s%s\n",
    vmcb->_asid, vmcb->_np_ctrl,
    vmcb->_np ? " NP" : "",
    vmcb->_sev ? " SEV" : "",
    ...);

This is more compact (things like "guest" in "guest asid" is entirely
redundant), and provides both the raw _np_ctrl field and a bit-by-bit
decode on the same line, rather than having different parts of the info
on different lines and bools written out in longhand?

See xen/arch/x86/spec_ctrl.c: print_details() for a rather more complete
example of this style of printk() rendering for bits, including how to
tabulate it for better readability.

~Andrew

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

[Vaishali Thakkar]

On 3/8/24 00:22, Andrew Cooper wrote:
> On 07/03/2024 9:40 pm, Vaishali Thakkar wrote:
>> The suffix is redundant for np/sev/sev-es bits. 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>i
> 
> Typo on the end of your email address?

Oops, thanks for catching it.

>> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
>> index e4e01add8c..7e285cf85a 100644
>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>> @@ -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;
> 
> Because the type of is bool, the !! can be dropped too while changing
> this line.

Thanks for the review. As I'm sending the revised patchset anyway,
will fix both things in this patch too.

> It seems that this was missing cleanup from f57ae00635 "SVM: split
> _np_enable VMCB field".
> 
> Anyway, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> and I'm
> happy to fix on commit.

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

[Vaishali Thakkar]

On 3/8/24 00:34, Andrew Cooper wrote:
> On 07/03/2024 9:40 pm, Vaishali Thakkar wrote:
>> diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c
>> index 24358c6eea..f54b426fb3 100644
>> --- a/xen/arch/x86/hvm/svm/svmdebug.c
>> +++ b/xen/arch/x86/hvm/svm/svmdebug.c
>> @@ -53,6 +53,8 @@ void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb)
>>              vmcb->exitinfo1, vmcb->exitinfo2);
>>       printk("np_ctrl = %#"PRIx64" guest_asid = %#x\n",
>>              vmcb_get_np_ctrl(vmcb), vmcb_get_guest_asid(vmcb));
>> +    printk("sev = %d sev_es = %d\n",
>> +           vmcb_get_sev(vmcb), vmcb_get_sev_es(vmcb));
>
> Hmm.  These are covered by the previous line printing all of np_ctrl.
> What about rearranging the previous line to be something like:
>
> printk("asid: %#x, np_ctrl: %#"PRIx64" -%s%s%s\n",
>      vmcb->_asid, vmcb->_np_ctrl,
>      vmcb->_np ? " NP" : "",
>      vmcb->_sev ? " SEV" : "",
>      ...);
>
> This is more compact (things like "guest" in "guest asid" is entirely
> redundant), and provides both the raw _np_ctrl field and a bit-by-bit
> decode on the same line, rather than having different parts of the info
> on different lines and bools written out in longhand?

Good point. Will change it in the revised v2.

> See xen/arch/x86/spec_ctrl.c: print_details() for a rather more complete
> example of this style of printk() rendering for bits, including how to
> tabulate it for better readability.

Thanks for pointing to the reference.

> ~Andrew

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

[Jan Beulich]

On 07.03.2024 22: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 = 1;

Given the field is bool, "true" here and elsewhere as well as ...

> @@ -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 = 0;

... "false" here (and once again further down)?

Jan