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

[Jan Beulich <jbeulich@xxxxxxxx>]

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