| Message ID | 20250331213406.422725-1-dmukhin@ford.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] x86/domain: revisit logging in arch_domain_create() | expand |
On Mon, Mar 31, 2025 at 09:34:24PM +0000, dmkhn@proton.me wrote: > From: Denis Mukhin <dmukhin@ford.com> > > Use %pd in all logs issued from arch_domain_create(). > > Also, expand error message in arch_domain_create() under > !emulation_flags_ok() case to help debugging. > > Signed-off-by: Denis Mukhin <dmukhin@ford.com> > --- > The origin of the patch is: > https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-6-c5d36b31d66c@ford.com/ > --- > xen/arch/x86/domain.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > index a42fa54805..15c5e2a652 100644 > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -798,13 +798,12 @@ int arch_domain_create(struct domain *d, > { > if ( !opt_allow_unsafe ) > { > - printk(XENLOG_G_ERR "Xen does not allow DomU creation on this CPU" > - " for security reasons.\n"); > + printk(XENLOG_G_ERR "%pd: Xen does not allow DomU creation on this CPU" > + " for security reasons.\n", d); Since you are already touching this, I would switch to gprintk, and avoid splitting the lines: gprintk(XENLOG_ERR, "%pd: Xen does not allow DomU creation on this CPU for security reasons.\n", d); Same for the other messages below. > return -EPERM; > } > printk(XENLOG_G_WARNING > - "Dom%d may compromise security on this CPU.\n", > - d->domain_id); > + "%pd: domain may compromise security on this CPU.\n", d); > } > > emflags = config->arch.emulation_flags; > @@ -814,16 +813,19 @@ int arch_domain_create(struct domain *d, > > if ( emflags & ~XEN_X86_EMU_ALL ) > { > - printk(XENLOG_G_ERR "d%d: Invalid emulation bitmap: %#x\n", > - d->domain_id, emflags); > + printk(XENLOG_G_ERR "%pd: Invalid emulation bitmap: %#x\n", > + d, emflags); > return -EINVAL; > } > > if ( !emulation_flags_ok(d, emflags) ) > { > - printk(XENLOG_G_ERR "d%d: Xen does not allow %s domain creation " > + printk(XENLOG_G_ERR "%pd: 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, > + is_hvm_domain(d) ? "HVM" : "PV", > + is_hardware_domain(d) ? "(hardware) " : "", I wouldn't use parentheses around hardware, but that's just my taste. Thanks, Roger.
On 01.04.2025 08:59, Roger Pau Monné wrote: > On Mon, Mar 31, 2025 at 09:34:24PM +0000, dmkhn@proton.me wrote: >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -798,13 +798,12 @@ int arch_domain_create(struct domain *d, >> { >> if ( !opt_allow_unsafe ) >> { >> - printk(XENLOG_G_ERR "Xen does not allow DomU creation on this CPU" >> - " for security reasons.\n"); >> + printk(XENLOG_G_ERR "%pd: Xen does not allow DomU creation on this CPU" >> + " for security reasons.\n", d); > > Since you are already touching this, I would switch to gprintk, and > avoid splitting the lines: > > gprintk(XENLOG_ERR, > "%pd: Xen does not allow DomU creation on this CPU for security reasons.\n", > d); > > Same for the other messages below. IOW you see value in also logging current->domain? What I'd like to ask for when touching these log messages is to get rid of the full stops. >> return -EPERM; >> } >> printk(XENLOG_G_WARNING >> - "Dom%d may compromise security on this CPU.\n", >> - d->domain_id); >> + "%pd: domain may compromise security on this CPU.\n", d); >> } >> >> emflags = config->arch.emulation_flags; >> @@ -814,16 +813,19 @@ int arch_domain_create(struct domain *d, >> >> if ( emflags & ~XEN_X86_EMU_ALL ) >> { >> - printk(XENLOG_G_ERR "d%d: Invalid emulation bitmap: %#x\n", >> - d->domain_id, emflags); >> + printk(XENLOG_G_ERR "%pd: Invalid emulation bitmap: %#x\n", >> + d, emflags); >> return -EINVAL; >> } >> >> if ( !emulation_flags_ok(d, emflags) ) >> { >> - printk(XENLOG_G_ERR "d%d: Xen does not allow %s domain creation " >> + printk(XENLOG_G_ERR "%pd: 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, >> + is_hvm_domain(d) ? "HVM" : "PV", >> + is_hardware_domain(d) ? "(hardware) " : "", > > I wouldn't use parentheses around hardware, but that's just my taste. +1 (provided we really need this extra property logged here) Jan
On Tue, Apr 01, 2025 at 09:51:53AM +0200, Jan Beulich wrote: > On 01.04.2025 08:59, Roger Pau Monné wrote: > > On Mon, Mar 31, 2025 at 09:34:24PM +0000, dmkhn@proton.me wrote: > >> --- a/xen/arch/x86/domain.c > >> +++ b/xen/arch/x86/domain.c > >> @@ -798,13 +798,12 @@ int arch_domain_create(struct domain *d, > >> { > >> if ( !opt_allow_unsafe ) > >> { > >> - printk(XENLOG_G_ERR "Xen does not allow DomU creation on this CPU" > >> - " for security reasons.\n"); > >> + printk(XENLOG_G_ERR "%pd: Xen does not allow DomU creation on this CPU" > >> + " for security reasons.\n", d); > > > > Since you are already touching this, I would switch to gprintk, and > > avoid splitting the lines: > > > > gprintk(XENLOG_ERR, > > "%pd: Xen does not allow DomU creation on this CPU for security reasons.\n", > > d); > > > > Same for the other messages below. > > IOW you see value in also logging current->domain? I always forget that gprintk also logs current->domain, my suggestion was so that XENLOG_ERR instead of XENLOG_G_ERR was used, as it's more compact. I think I withdraw my suggestion, there's likely very little help from printing current->domain in this context. It's either the IDLE domain for initial domain build, or the control domain otherwise. Thanks, Roger.
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index a42fa54805..15c5e2a652 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -798,13 +798,12 @@ int arch_domain_create(struct domain *d, { if ( !opt_allow_unsafe ) { - printk(XENLOG_G_ERR "Xen does not allow DomU creation on this CPU" - " for security reasons.\n"); + printk(XENLOG_G_ERR "%pd: Xen does not allow DomU creation on this CPU" + " for security reasons.\n", d); return -EPERM; } printk(XENLOG_G_WARNING - "Dom%d may compromise security on this CPU.\n", - d->domain_id); + "%pd: domain may compromise security on this CPU.\n", d); } emflags = config->arch.emulation_flags; @@ -814,16 +813,19 @@ int arch_domain_create(struct domain *d, if ( emflags & ~XEN_X86_EMU_ALL ) { - printk(XENLOG_G_ERR "d%d: Invalid emulation bitmap: %#x\n", - d->domain_id, emflags); + printk(XENLOG_G_ERR "%pd: Invalid emulation bitmap: %#x\n", + d, emflags); return -EINVAL; } if ( !emulation_flags_ok(d, emflags) ) { - printk(XENLOG_G_ERR "d%d: Xen does not allow %s domain creation " + printk(XENLOG_G_ERR "%pd: 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, + is_hvm_domain(d) ? "HVM" : "PV", + is_hardware_domain(d) ? "(hardware) " : "", + emflags); return -EOPNOTSUPP; } d->arch.emulation_flags = emflags;