xen/arch/x86/hvm/viridian/synic.c | 2 +- xen/arch/x86/hvm/viridian/viridian.c | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-)
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
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
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); > } >
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
© 2016 - 2024 Red Hat, Inc.