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

[Vaishali Thakkar <vaishali.thakkar@xxxxxxxxxx>]

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