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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.04.2025 01:21, dmkhn@xxxxxxxxx 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