| Message ID | 20241205-vuart-ns8250-v1-9-e9aa923127eb@ford.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce NS8250 UART emulator | expand |
On 06.12.2024 05:41, Denis Mukhin via B4 Relay wrote: > Print d->arch.emulation_flags on the console for better traceability while > debugging in-hypervisor hardware emulators. Personally I disagree with such extra printing. And that would in this case even apply if you used dprintk() or gdprintk(). However, if others support the idea, I don't mean to stand in the way. Just that ... > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -818,11 +818,15 @@ int arch_domain_create(struct domain *d, > > if ( !emulation_flags_ok(d, emflags) ) > { > - printk(XENLOG_G_ERR "d%d: Xen does not allow %s domain creation " > + printk(XENLOG_G_ERR "d%d: Xen does not allow %s %sdomain creation " > "with the current selection of emulators: %#x\n", > - d->domain_id, is_hvm_domain(d) ? "HVM" : "PV", emflags); > + d->domain_id, ... if already you touch this, please switch to %pd and also ... > + is_hvm_domain(d) ? "HVM" : "PV", > + is_hardware_domain(d) ? "(hardware) " : "", > + emflags); > return -EOPNOTSUPP; > } > + printk(XENLOG_G_INFO "d%d: emulation_flags %#x\n", d->domain_id, emflags); .. use that here. Jan
On Thu, Dec 05, 2024 at 08:41:39PM -0800, Denis Mukhin via B4 Relay wrote: > From: Denis Mukhin <dmukhin@ford.com> > > Print d->arch.emulation_flags on the console for better traceability while > debugging in-hypervisor hardware emulators. > > Signed-off-by: Denis Mukhin <dmukhin@ford.com> > --- > xen/arch/x86/domain.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > index 78a13e6812c9120901d0a70fb3bc1bd6a8b6917d..c88d422a64544531c1e1058fa484364bb4277d1e 100644 > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -818,11 +818,15 @@ int arch_domain_create(struct domain *d, > > if ( !emulation_flags_ok(d, emflags) ) > { > - printk(XENLOG_G_ERR "d%d: Xen does not allow %s domain creation " > + printk(XENLOG_G_ERR "d%d: Xen does not allow %s %sdomain creation " gprintk(XENLOG_ERR, "... Might be more natural now that we have the macro (together with Jan's suggestion to use %pd (same below). > "with the current selection of emulators: %#x\n", > - d->domain_id, is_hvm_domain(d) ? "HVM" : "PV", emflags); > + d->domain_id, > + is_hvm_domain(d) ? "HVM" : "PV", > + is_hardware_domain(d) ? "(hardware) " : "", > + emflags); > return -EOPNOTSUPP; > } > + printk(XENLOG_G_INFO "d%d: emulation_flags %#x\n", d->domain_id, emflags); This would need to be a dprintk at least, and the log level should be XENLOG_DEBUG. Maybe it would be better if you could print this information as part of some debug key, for not having to print it for every guest creation. Maybe as part of the 'q' debug key? Thanks, Roger.
On 11.12.2024 16:19, Roger Pau Monné wrote: > On Thu, Dec 05, 2024 at 08:41:39PM -0800, Denis Mukhin via B4 Relay wrote: >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -818,11 +818,15 @@ int arch_domain_create(struct domain *d, >> >> if ( !emulation_flags_ok(d, emflags) ) >> { >> - printk(XENLOG_G_ERR "d%d: Xen does not allow %s domain creation " >> + printk(XENLOG_G_ERR "d%d: Xen does not allow %s %sdomain creation " > > gprintk(XENLOG_ERR, "... > > Might be more natural now that we have the macro (together with Jan's > suggestion to use %pd (same below). Yet why would we want to log current here, as gprintk() does? Jan
On Thu, Dec 12, 2024 at 12:53:45PM +0100, Jan Beulich wrote: > On 11.12.2024 16:19, Roger Pau Monné wrote: > > On Thu, Dec 05, 2024 at 08:41:39PM -0800, Denis Mukhin via B4 Relay wrote: > >> --- a/xen/arch/x86/domain.c > >> +++ b/xen/arch/x86/domain.c > >> @@ -818,11 +818,15 @@ int arch_domain_create(struct domain *d, > >> > >> if ( !emulation_flags_ok(d, emflags) ) > >> { > >> - printk(XENLOG_G_ERR "d%d: Xen does not allow %s domain creation " > >> + printk(XENLOG_G_ERR "d%d: Xen does not allow %s %sdomain creation " > > > > gprintk(XENLOG_ERR, "... > > > > Might be more natural now that we have the macro (together with Jan's > > suggestion to use %pd (same below). > > Yet why would we want to log current here, as gprintk() does? Right - I've forgotten that gprintk already prepends %pd. Thanks, Roger.
On 12.12.2024 13:11, Roger Pau Monné wrote: > On Thu, Dec 12, 2024 at 12:53:45PM +0100, Jan Beulich wrote: >> On 11.12.2024 16:19, Roger Pau Monné wrote: >>> On Thu, Dec 05, 2024 at 08:41:39PM -0800, Denis Mukhin via B4 Relay wrote: >>>> --- a/xen/arch/x86/domain.c >>>> +++ b/xen/arch/x86/domain.c >>>> @@ -818,11 +818,15 @@ int arch_domain_create(struct domain *d, >>>> >>>> if ( !emulation_flags_ok(d, emflags) ) >>>> { >>>> - printk(XENLOG_G_ERR "d%d: Xen does not allow %s domain creation " >>>> + printk(XENLOG_G_ERR "d%d: Xen does not allow %s %sdomain creation " >>> >>> gprintk(XENLOG_ERR, "... >>> >>> Might be more natural now that we have the macro (together with Jan's >>> suggestion to use %pd (same below). >> >> Yet why would we want to log current here, as gprintk() does? > > Right - I've forgotten that gprintk already prepends %pd. FTAOD: It's %pv and logging current, which isn't what is being logged here (an incoming struct domain *). Jan
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 78a13e6812c9120901d0a70fb3bc1bd6a8b6917d..c88d422a64544531c1e1058fa484364bb4277d1e 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -818,11 +818,15 @@ int arch_domain_create(struct domain *d, if ( !emulation_flags_ok(d, emflags) ) { - printk(XENLOG_G_ERR "d%d: Xen does not allow %s domain creation " + printk(XENLOG_G_ERR "d%d: Xen does not allow %s %sdomain creation " "with the current selection of emulators: %#x\n", - d->domain_id, is_hvm_domain(d) ? "HVM" : "PV", emflags); + d->domain_id, + is_hvm_domain(d) ? "HVM" : "PV", + is_hardware_domain(d) ? "(hardware) " : "", + emflags); return -EOPNOTSUPP; } + printk(XENLOG_G_INFO "d%d: emulation_flags %#x\n", d->domain_id, emflags); d->arch.emulation_flags = emflags; #ifdef CONFIG_PV32