[PATCH] x86/viridian: Clarify some viridian logging strings

[PATCH] x86/viridian: Clarify some viridian logging strings

[Alejandro Vallejo]

It's sadically misleading to show an error without letters and expect
the dmesg reader to understand it's in hex. The patch adds a 0x prefix
to all hex numbers that don't already have it.

On the one instance in which a boolean is printed as an integer, print
it as a decimal integer instead so it's 0/1 in the common case and not
misleading if it's ever not just that due to a bug.

While at it, rename VIRIDIAN CRASH to VIRIDIAN GUEST_CRASH. Every member
of a support team that looks at the message systematically believes
"viridian" crashed, which is absolutely not what goes on. It's the guest
asking the hypervisor for a sudden shutdown because it crashed, and
stating why.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
Still going through its Gitlab pipeline

---
 xen/arch/x86/hvm/viridian/synic.c    | 2 +-
 xen/arch/x86/hvm/viridian/viridian.c | 9 +++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/viridian/synic.c b/xen/arch/x86/hvm/viridian/synic.c
index 3375e55e95ca..c3dc573b003d 100644
--- a/xen/arch/x86/hvm/viridian/synic.c
+++ b/xen/arch/x86/hvm/viridian/synic.c
@@ -172,7 +172,7 @@ int viridian_synic_wrmsr(struct vcpu *v, uint32_t idx, uint64_t val)
         vector = new.vector;
         vv->vector_to_sintx[vector] = sintx;
 
-        printk(XENLOG_G_INFO "%pv: VIRIDIAN SINT%u: vector: %x\n", v, sintx,
+        printk(XENLOG_G_INFO "%pv: VIRIDIAN SINT%u: vector: %#x\n", v, sintx,
                vector);
 
         *vs = new;
diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 0496c52ed5a2..21480d9ee700 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -253,7 +253,7 @@ static void dump_guest_os_id(const struct domain *d)
     goi = &d->arch.hvm.viridian->guest_os_id;
 
     printk(XENLOG_G_INFO
-           "d%d: VIRIDIAN GUEST_OS_ID: vendor: %x os: %x major: %x minor: %x sp: %x build: %x\n",
+           "d%d: VIRIDIAN GUEST_OS_ID: vendor: %#x os: %#x major: %#x minor: %#x sp: %#x build: %#x\n",
            d->domain_id, goi->vendor, goi->os, goi->major, goi->minor,
            goi->service_pack, goi->build_number);
 }
@@ -264,7 +264,7 @@ static void dump_hypercall(const struct domain *d)
 
     hg = &d->arch.hvm.viridian->hypercall_gpa;
 
-    printk(XENLOG_G_INFO "d%d: VIRIDIAN HYPERCALL: enabled: %x pfn: %lx\n",
+    printk(XENLOG_G_INFO "d%d: VIRIDIAN HYPERCALL: enabled: %u pfn: %#lx\n",
            d->domain_id,
            hg->enabled, (unsigned long)hg->pfn);
 }
@@ -372,7 +372,8 @@ int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
         d->shutdown_code = SHUTDOWN_crash;
         spin_unlock(&d->shutdown_lock);
 
-        gprintk(XENLOG_WARNING, "VIRIDIAN CRASH: %lx %lx %lx %lx %lx\n",
+        gprintk(XENLOG_WARNING,
+                "VIRIDIAN GUEST_CRASH: %#lx %#lx %#lx %#lx %#lx\n",
                 vv->crash_param[0], vv->crash_param[1], vv->crash_param[2],
                 vv->crash_param[3], vv->crash_param[4]);
         break;
@@ -1056,7 +1057,7 @@ void viridian_dump_guest_page(const struct vcpu *v, const char *name,
     if ( !vp->msr.enabled )
         return;
 
-    printk(XENLOG_G_INFO "%pv: VIRIDIAN %s: pfn: %lx\n",
+    printk(XENLOG_G_INFO "%pv: VIRIDIAN %s: pfn: %#lx\n",
            v, name, (unsigned long)vp->msr.pfn);
 }
 
-- 
2.45.2

Re: [PATCH] x86/viridian: Clarify some viridian logging strings

[Paul Durrant]

On 26/07/2024 15:52, Alejandro Vallejo wrote:
> It's sadically misleading to show an error without letters and expect
> the dmesg reader to understand it's in hex.

That depends on who's doing the reading.

> The patch adds a 0x prefix
> to all hex numbers that don't already have it.
> 
> On the one instance in which a boolean is printed as an integer, print
> it as a decimal integer instead so it's 0/1 in the common case and not
> misleading if it's ever not just that due to a bug.
> 
> While at it, rename VIRIDIAN CRASH to VIRIDIAN GUEST_CRASH. Every member
> of a support team that looks at the message systematically believes
> "viridian" crashed,

... which suggests they need educating as to what 'viridian' is (or was).

> which is absolutely not what goes on. It's the guest
> asking the hypervisor for a sudden shutdown because it crashed, and
> stating why.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
> Still going through its Gitlab pipeline
> 
> ---
>   xen/arch/x86/hvm/viridian/synic.c    | 2 +-
>   xen/arch/x86/hvm/viridian/viridian.c | 9 +++++----
>   2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/viridian/synic.c b/xen/arch/x86/hvm/viridian/synic.c
> index 3375e55e95ca..c3dc573b003d 100644
> --- a/xen/arch/x86/hvm/viridian/synic.c
> +++ b/xen/arch/x86/hvm/viridian/synic.c
> @@ -172,7 +172,7 @@ int viridian_synic_wrmsr(struct vcpu *v, uint32_t idx, uint64_t val)
>           vector = new.vector;
>           vv->vector_to_sintx[vector] = sintx;
>   
> -        printk(XENLOG_G_INFO "%pv: VIRIDIAN SINT%u: vector: %x\n", v, sintx,
> +        printk(XENLOG_G_INFO "%pv: VIRIDIAN SINT%u: vector: %#x\n", v, sintx,
>                  vector);
>   
>           *vs = new;
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
> index 0496c52ed5a2..21480d9ee700 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -253,7 +253,7 @@ static void dump_guest_os_id(const struct domain *d)
>       goi = &d->arch.hvm.viridian->guest_os_id;
>   
>       printk(XENLOG_G_INFO
> -           "d%d: VIRIDIAN GUEST_OS_ID: vendor: %x os: %x major: %x minor: %x sp: %x build: %x\n",
> +           "d%d: VIRIDIAN GUEST_OS_ID: vendor: %#x os: %#x major: %#x minor: %#x sp: %#x build: %#x\n",
>              d->domain_id, goi->vendor, goi->os, goi->major, goi->minor,
>              goi->service_pack, goi->build_number);
>   }
> @@ -264,7 +264,7 @@ static void dump_hypercall(const struct domain *d)
>   
>       hg = &d->arch.hvm.viridian->hypercall_gpa;
>   
> -    printk(XENLOG_G_INFO "d%d: VIRIDIAN HYPERCALL: enabled: %x pfn: %lx\n",
> +    printk(XENLOG_G_INFO "d%d: VIRIDIAN HYPERCALL: enabled: %u pfn: %#lx\n",
>              d->domain_id,
>              hg->enabled, (unsigned long)hg->pfn);
>   }
> @@ -372,7 +372,8 @@ int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
>           d->shutdown_code = SHUTDOWN_crash;
>           spin_unlock(&d->shutdown_lock);
>   
> -        gprintk(XENLOG_WARNING, "VIRIDIAN CRASH: %lx %lx %lx %lx %lx\n",
> +        gprintk(XENLOG_WARNING,
> +                "VIRIDIAN GUEST_CRASH: %#lx %#lx %#lx %#lx %#lx\n",

Honestly this change should be unnecessary, but since this is all 
cosmetic...

Reviewed-by: Paul Durrant <paul@xen.org>

>                   vv->crash_param[0], vv->crash_param[1], vv->crash_param[2],
>                   vv->crash_param[3], vv->crash_param[4]);
>           break;
> @@ -1056,7 +1057,7 @@ void viridian_dump_guest_page(const struct vcpu *v, const char *name,
>       if ( !vp->msr.enabled )
>           return;
>   
> -    printk(XENLOG_G_INFO "%pv: VIRIDIAN %s: pfn: %lx\n",
> +    printk(XENLOG_G_INFO "%pv: VIRIDIAN %s: pfn: %#lx\n",
>              v, name, (unsigned long)vp->msr.pfn);
>   }
>   

Re: [PATCH] x86/viridian: Clarify some viridian logging strings

[Alejandro Vallejo]

On Fri Jul 26, 2024 at 4:11 PM BST, Paul Durrant wrote:
> On 26/07/2024 15:52, Alejandro Vallejo wrote:
> > It's sadically misleading to show an error without letters and expect
> > the dmesg reader to understand it's in hex.
>
> That depends on who's doing the reading.
>
> > The patch adds a 0x prefix
> > to all hex numbers that don't already have it.
> > 
> > On the one instance in which a boolean is printed as an integer, print
> > it as a decimal integer instead so it's 0/1 in the common case and not
> > misleading if it's ever not just that due to a bug.
> > 
> > While at it, rename VIRIDIAN CRASH to VIRIDIAN GUEST_CRASH. Every member
> > of a support team that looks at the message systematically believes
> > "viridian" crashed,
>
> ... which suggests they need educating as to what 'viridian' is (or was).
>

Can't argue with you there. But if a minor cosmetic tweak to a dmesg string
clarifies a matter without further explanation it's imo a net positive change.

> > which is absolutely not what goes on. It's the guest
> > asking the hypervisor for a sudden shutdown because it crashed, and
> > stating why.
> > 
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> > ---
> > Still going through its Gitlab pipeline
> > 
> > ---
> >   xen/arch/x86/hvm/viridian/synic.c    | 2 +-
> >   xen/arch/x86/hvm/viridian/viridian.c | 9 +++++----
> >   2 files changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/viridian/synic.c b/xen/arch/x86/hvm/viridian/synic.c
> > index 3375e55e95ca..c3dc573b003d 100644
> > --- a/xen/arch/x86/hvm/viridian/synic.c
> > +++ b/xen/arch/x86/hvm/viridian/synic.c
> > @@ -172,7 +172,7 @@ int viridian_synic_wrmsr(struct vcpu *v, uint32_t idx, uint64_t val)
> >           vector = new.vector;
> >           vv->vector_to_sintx[vector] = sintx;
> >   
> > -        printk(XENLOG_G_INFO "%pv: VIRIDIAN SINT%u: vector: %x\n", v, sintx,
> > +        printk(XENLOG_G_INFO "%pv: VIRIDIAN SINT%u: vector: %#x\n", v, sintx,
> >                  vector);
> >   
> >           *vs = new;
> > diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
> > index 0496c52ed5a2..21480d9ee700 100644
> > --- a/xen/arch/x86/hvm/viridian/viridian.c
> > +++ b/xen/arch/x86/hvm/viridian/viridian.c
> > @@ -253,7 +253,7 @@ static void dump_guest_os_id(const struct domain *d)
> >       goi = &d->arch.hvm.viridian->guest_os_id;
> >   
> >       printk(XENLOG_G_INFO
> > -           "d%d: VIRIDIAN GUEST_OS_ID: vendor: %x os: %x major: %x minor: %x sp: %x build: %x\n",
> > +           "d%d: VIRIDIAN GUEST_OS_ID: vendor: %#x os: %#x major: %#x minor: %#x sp: %#x build: %#x\n",
> >              d->domain_id, goi->vendor, goi->os, goi->major, goi->minor,
> >              goi->service_pack, goi->build_number);
> >   }
> > @@ -264,7 +264,7 @@ static void dump_hypercall(const struct domain *d)
> >   
> >       hg = &d->arch.hvm.viridian->hypercall_gpa;
> >   
> > -    printk(XENLOG_G_INFO "d%d: VIRIDIAN HYPERCALL: enabled: %x pfn: %lx\n",
> > +    printk(XENLOG_G_INFO "d%d: VIRIDIAN HYPERCALL: enabled: %u pfn: %#lx\n",
> >              d->domain_id,
> >              hg->enabled, (unsigned long)hg->pfn);
> >   }
> > @@ -372,7 +372,8 @@ int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
> >           d->shutdown_code = SHUTDOWN_crash;
> >           spin_unlock(&d->shutdown_lock);
> >   
> > -        gprintk(XENLOG_WARNING, "VIRIDIAN CRASH: %lx %lx %lx %lx %lx\n",
> > +        gprintk(XENLOG_WARNING,
> > +                "VIRIDIAN GUEST_CRASH: %#lx %#lx %#lx %#lx %#lx\n",
>
> Honestly this change should be unnecessary, but since this is all 
> cosmetic...
>
> Reviewed-by: Paul Durrant <paul@xen.org>
>

Thanks

> >                   vv->crash_param[0], vv->crash_param[1], vv->crash_param[2],
> >                   vv->crash_param[3], vv->crash_param[4]);
> >           break;
> > @@ -1056,7 +1057,7 @@ void viridian_dump_guest_page(const struct vcpu *v, const char *name,
> >       if ( !vp->msr.enabled )
> >           return;
> >   
> > -    printk(XENLOG_G_INFO "%pv: VIRIDIAN %s: pfn: %lx\n",
> > +    printk(XENLOG_G_INFO "%pv: VIRIDIAN %s: pfn: %#lx\n",
> >              v, name, (unsigned long)vp->msr.pfn);
> >   }
> >   

Cheers,
Alejandro

Re: [PATCH] x86/viridian: Clarify some viridian logging strings

[Jan Beulich]

On 26.07.2024 16:52, Alejandro Vallejo wrote:
> It's sadically misleading to show an error without letters and expect
> the dmesg reader to understand it's in hex. The patch adds a 0x prefix
> to all hex numbers that don't already have it.

Just a few remarks: First I agree with Paul here. Imo ...

> --- a/xen/arch/x86/hvm/viridian/synic.c
> +++ b/xen/arch/x86/hvm/viridian/synic.c
> @@ -172,7 +172,7 @@ int viridian_synic_wrmsr(struct vcpu *v, uint32_t idx, uint64_t val)
>          vector = new.vector;
>          vv->vector_to_sintx[vector] = sintx;
>  
> -        printk(XENLOG_G_INFO "%pv: VIRIDIAN SINT%u: vector: %x\n", v, sintx,
> +        printk(XENLOG_G_INFO "%pv: VIRIDIAN SINT%u: vector: %#x\n", v, sintx,
>                 vector);

... vectors should simply never be logged decimal, and then there's no
need for any disambiguating 0x.

> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -253,7 +253,7 @@ static void dump_guest_os_id(const struct domain *d)
>      goi = &d->arch.hvm.viridian->guest_os_id;
>  
>      printk(XENLOG_G_INFO
> -           "d%d: VIRIDIAN GUEST_OS_ID: vendor: %x os: %x major: %x minor: %x sp: %x build: %x\n",
> +           "d%d: VIRIDIAN GUEST_OS_ID: vendor: %#x os: %#x major: %#x minor: %#x sp: %#x build: %#x\n",
>             d->domain_id, goi->vendor, goi->os, goi->major, goi->minor,
>             goi->service_pack, goi->build_number);

Here otoh I agree, to some degree at least: Generally I'd expect version
numbers to be logged decimal.

What would have been nice both here and ...

> @@ -264,7 +264,7 @@ static void dump_hypercall(const struct domain *d)
>  
>      hg = &d->arch.hvm.viridian->hypercall_gpa;
>  
> -    printk(XENLOG_G_INFO "d%d: VIRIDIAN HYPERCALL: enabled: %x pfn: %lx\n",
> +    printk(XENLOG_G_INFO "d%d: VIRIDIAN HYPERCALL: enabled: %u pfn: %#lx\n",
>             d->domain_id,
>             hg->enabled, (unsigned long)hg->pfn);

... here would be to take the opportunity and switch to %pd.

> @@ -372,7 +372,8 @@ int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
>          d->shutdown_code = SHUTDOWN_crash;
>          spin_unlock(&d->shutdown_lock);
>  
> -        gprintk(XENLOG_WARNING, "VIRIDIAN CRASH: %lx %lx %lx %lx %lx\n",
> +        gprintk(XENLOG_WARNING,
> +                "VIRIDIAN GUEST_CRASH: %#lx %#lx %#lx %#lx %#lx\n",
>                  vv->crash_param[0], vv->crash_param[1], vv->crash_param[2],
>                  vv->crash_param[3], vv->crash_param[4]);

Here I wonder what good the underscore does that you add.

Jan