| Message ID | 20250404232145.1252544-1-dmukhin@ford.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] x86/domain: revisit logging in arch_domain_create() | expand |
On 05.04.2025 01:21, dmkhn@proton.me wrote: > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -791,13 +791,14 @@ 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: cannot create domain on this CPU due to security reasons\n", > + d); > return -EPERM; I was about to give an ack, but here and ... > @@ -807,16 +808,20 @@ 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 " > - "with the current selection of emulators: %#x\n", > - d->domain_id, is_hvm_domain(d) ? "HVM" : "PV", emflags); > + printk(XENLOG_G_ERR > + "%pd: cannot create %s %sdomain with emulators: %#x\n", > + d, > + is_hvm_domain(d) ? "HVM" : "PV", > + is_hardware_domain(d) ? "hardware " : "", > + emflags); > return -EOPNOTSUPP; > } ... here I question the re-wording: Xen could very well create domains in these cases. It merely refuses to for one reason or another. In the latter case the re-wording may be kind of okay, because code elsewhere may need updating. In the former case, however, the situation is a pretty clear cut. It doesn't need to be the original wording, but minimally in what you suggest it needs to be "s/cannot/will not/" or some such. Plus a nit: In the revision log you say "shortened message text where possible", yet then you swapped in "due to" for the prior "for" in the former of the two cases discussed here. That's clearly longer, without (imo) gaining us anything. Similarly it's unclear why you replaced "DomU". Jan
On Monday, April 7th, 2025 at 7:21 AM, Jan Beulich <jbeulich@suse.com> wrote: > > > On 05.04.2025 01:21, dmkhn@proton.me wrote: > > > --- a/xen/arch/x86/domain.c > > +++ b/xen/arch/x86/domain.c > > @@ -791,13 +791,14 @@ 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: cannot create domain on this CPU due to security reasons\n", > > + d); > > return -EPERM; > > > I was about to give an ack, but here and ... > > > @@ -807,16 +808,20 @@ 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 " > > - "with the current selection of emulators: %#x\n", > > - d->domain_id, is_hvm_domain(d) ? "HVM" : "PV", emflags); > > + printk(XENLOG_G_ERR > > + "%pd: cannot create %s %sdomain with emulators: %#x\n", > > + d, > > + is_hvm_domain(d) ? "HVM" : "PV", > > + is_hardware_domain(d) ? "hardware " : "", > > + emflags); > > return -EOPNOTSUPP; > > } > > > ... here I question the re-wording: Xen could very well create domains in > these cases. It merely refuses to for one reason or another. In the > latter case the re-wording may be kind of okay, because code elsewhere may > need updating. In the former case, however, the situation is a pretty > clear cut. It doesn't need to be the original wording, but minimally in > what you suggest it needs to be "s/cannot/will not/" or some such. I see, that makes sense. Fixed. > > Plus a nit: In the revision log you say "shortened message text where > possible", yet then you swapped in "due to" for the prior "for" in the > former of the two cases discussed here. That's clearly longer, without > (imo) gaining us anything. Similarly it's unclear why you replaced "DomU". re: message rewording: kept "for". re: domU: I removed it because my understanding is that w/ eventual introduction of a split between control domain and hardware domain there might be another change here. Kept "domU". > > Jan
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 4989600627..0db0567877 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -791,13 +791,14 @@ 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: cannot create domain on this CPU due to security reasons\n", + d); return -EPERM; } printk(XENLOG_G_WARNING - "Dom%d may compromise security on this CPU.\n", - d->domain_id); + "%pd: may compromise security on this CPU\n", + d); } emflags = config->arch.emulation_flags; @@ -807,16 +808,20 @@ 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 " - "with the current selection of emulators: %#x\n", - d->domain_id, is_hvm_domain(d) ? "HVM" : "PV", emflags); + printk(XENLOG_G_ERR + "%pd: cannot create %s %sdomain with emulators: %#x\n", + d, + is_hvm_domain(d) ? "HVM" : "PV", + is_hardware_domain(d) ? "hardware " : "", + emflags); return -EOPNOTSUPP; } d->arch.emulation_flags = emflags;