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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.04.2025 08:59, Roger Pau Monné wrote:
> On Mon, Mar 31, 2025 at 09:34:24PM +0000, dmkhn@xxxxxxxxx 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