Re: [PATCH v3] x86/domain: revisit logging in arch_domain_create()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.04.2025 23:44, dmkhn@xxxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
> 
> 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@xxxxxxxx>

A few nits:

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -791,13 +791,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);

If this is touched, I think it would better become

            printk(XENLOG_G_ERR
                   "%pd: Xen does not allow DomU creation on this CPU for 
security reasons\n"
                   d);

to follow our "don't wrap format strings" principle.

>              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);

Personally I don't think the word "domain" needs adding here.

> @@ -807,16 +806,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",

Please switch to using a lower case 'i' on "invalid".

> +               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",

Here wrapping of the format string would also be nice to avoid. That
may call for an attempt to shorten the text a little. Maybe "with
this set of emulators:" would already help some.

Jan