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

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

[dmkhn]

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

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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

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

[Roger Pau Monné]

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.