Add a domid field to struct boot_domain to hold the assigned domain id for the
domain. During initialization, ensure all instances of struct boot_domain have
the invalid domid to ensure that the domid must be set either by convention or
configuration.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
 xen/arch/x86/include/asm/bootdomain.h |  2 ++
 xen/arch/x86/setup.c                  | 12 +++++++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index 12c19ab37bd8..3873f916f854 100644
--- a/xen/arch/x86/include/asm/bootdomain.h
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -12,6 +12,8 @@ struct boot_module;
 struct domain;
 
 struct boot_domain {
+    domid_t domid;
+
     struct boot_module *kernel;
     struct boot_module *ramdisk;
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 2ccaa7dc965b..533a1e2bbe05 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -339,6 +339,9 @@ static struct boot_info *__init multiboot_fill_boot_info(
     /* Variable 'i' should be one entry past the last module. */
     bi->mods[i].type = BOOTMOD_XEN;
 
+    for ( i = 0; i < MAX_NR_BOOTDOMS; i++ )
+        bi->domains[i].domid = DOMID_INVALID;
+
     return bi;
 }
 
@@ -977,7 +980,6 @@ static struct domain *__init create_dom0(struct boot_info *bi)
     };
     struct boot_domain *bd = &bi->domains[0];
     struct domain *d;
-    domid_t domid;
 
     if ( opt_dom0_pvh )
     {
@@ -993,15 +995,15 @@ static struct domain *__init create_dom0(struct boot_info *bi)
         dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
 
     /* Create initial domain.  Not d0 for pvshim. */
-    domid = get_initial_domain_id();
-    d = domain_create(domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
+    bd->domid = get_initial_domain_id();
+    d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
     if ( IS_ERR(d) )
-        panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
+        panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
 
     init_dom0_cpuid_policy(d);
 
     if ( alloc_dom0_vcpu0(d) == NULL )
-        panic("Error creating d%uv0\n", domid);
+        panic("Error creating d%uv0\n", bd->domid);
 
     /* Grab the DOM0 command line. */
     if ( bd->kernel->cmdline_pa || bi->kextra )
-- 
2.30.2

On 15.11.2024 14:12, Daniel P. Smith wrote:
> Add a domid field to struct boot_domain to hold the assigned domain id for the
> domain. During initialization, ensure all instances of struct boot_domain have
> the invalid domid to ensure that the domid must be set either by convention or
> configuration.

I'm missing the "why" part here - after all ...

> --- a/xen/arch/x86/include/asm/bootdomain.h
> +++ b/xen/arch/x86/include/asm/bootdomain.h
> @@ -12,6 +12,8 @@ struct boot_module;
>  struct domain;
>  
>  struct boot_domain {
> +    domid_t domid;
> +
>      struct boot_module *kernel;
>      struct boot_module *ramdisk;
>  

... just out of context here there is struct domain *. I can only guess that
the domain ID is needed for the time until the domain pointer was actually
filled.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -339,6 +339,9 @@ static struct boot_info *__init multiboot_fill_boot_info(
>      /* Variable 'i' should be one entry past the last module. */
>      bi->mods[i].type = BOOTMOD_XEN;
>  
> +    for ( i = 0; i < MAX_NR_BOOTDOMS; i++ )
> +        bi->domains[i].domid = DOMID_INVALID;

Generally I think ARRAY_SIZE() is better to use for loop boundaries. Yet
then - why don't you statically initialize the array in xen_boot_info?

> @@ -977,7 +980,6 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>      };
>      struct boot_domain *bd = &bi->domains[0];
>      struct domain *d;
> -    domid_t domid;
>  
>      if ( opt_dom0_pvh )
>      {
> @@ -993,15 +995,15 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>          dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>  
>      /* Create initial domain.  Not d0 for pvshim. */
> -    domid = get_initial_domain_id();
> -    d = domain_create(domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
> +    bd->domid = get_initial_domain_id();
> +    d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
>      if ( IS_ERR(d) )
> -        panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
> +        panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));

As to the comment at the top - this change alone certainly doesn't clarify
the "why".

>      init_dom0_cpuid_policy(d);
>  
>      if ( alloc_dom0_vcpu0(d) == NULL )
> -        panic("Error creating d%uv0\n", domid);
> +        panic("Error creating d%uv0\n", bd->domid);

Imo this would better use d->domain_id. And while touching it, %u would also
want swapping for %d.

Jan

On 11/27/24 05:32, Jan Beulich wrote:
> On 15.11.2024 14:12, Daniel P. Smith wrote:
>> Add a domid field to struct boot_domain to hold the assigned domain id for the
>> domain. During initialization, ensure all instances of struct boot_domain have
>> the invalid domid to ensure that the domid must be set either by convention or
>> configuration.
> 
> I'm missing the "why" part here - after all ...

Which is part of why I rolled these over into the dom0 device tree 
series, as it will provide more context to its purpose. This field is 
used to store the value parsed in from the device tree. In dom0 device 
tree series, this commit could be merged with the commit that introduces 
the device tree parsing for domid to provide better context for the 
introduction of the structure element.

>> --- a/xen/arch/x86/include/asm/bootdomain.h
>> +++ b/xen/arch/x86/include/asm/bootdomain.h
>> @@ -12,6 +12,8 @@ struct boot_module;
>>   struct domain;
>>   
>>   struct boot_domain {
>> +    domid_t domid;
>> +
>>       struct boot_module *kernel;
>>       struct boot_module *ramdisk;
>>   
> 
> ... just out of context here there is struct domain *. I can only guess that
> the domain ID is needed for the time until the domain pointer was actually
> filled.

Correct, thus why it makes more sense to merge with the domid device 
tree parsing.

>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -339,6 +339,9 @@ static struct boot_info *__init multiboot_fill_boot_info(
>>       /* Variable 'i' should be one entry past the last module. */
>>       bi->mods[i].type = BOOTMOD_XEN;
>>   
>> +    for ( i = 0; i < MAX_NR_BOOTDOMS; i++ )
>> +        bi->domains[i].domid = DOMID_INVALID;
> 
> Generally I think ARRAY_SIZE() is better to use for loop boundaries. Yet
> then - why don't you statically initialize the array in xen_boot_info?

I indifferent with ARRAY_SIZE(), I will try and keep that in mind for 
future parts of the series. As for static init'ing, good point since we 
are already doing it with other fields, I can add this to it.

>> @@ -977,7 +980,6 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>>       };
>>       struct boot_domain *bd = &bi->domains[0];
>>       struct domain *d;
>> -    domid_t domid;
>>   
>>       if ( opt_dom0_pvh )
>>       {
>> @@ -993,15 +995,15 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>>           dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>>   
>>       /* Create initial domain.  Not d0 for pvshim. */
>> -    domid = get_initial_domain_id();
>> -    d = domain_create(domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
>> +    bd->domid = get_initial_domain_id();
>> +    d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
>>       if ( IS_ERR(d) )
>> -        panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
>> +        panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
> 
> As to the comment at the top - this change alone certainly doesn't clarify
> the "why".

Agreed.

>>       init_dom0_cpuid_policy(d);
>>   
>>       if ( alloc_dom0_vcpu0(d) == NULL )
>> -        panic("Error creating d%uv0\n", domid);
>> +        panic("Error creating d%uv0\n", bd->domid);
> 
> Imo this would better use d->domain_id. And while touching it, %u would also
> want swapping for %d.

hmm, I was actually considering s/d%u/%pd/ and just pass in d, but was 
certain if there was an explicit reason it wasn't used before. If I am 
going to change it, would %pd not be more desired here?

v/r,
dps

On 04.12.2024 17:45, Daniel P. Smith wrote:
> On 11/27/24 05:32, Jan Beulich wrote:
>> On 15.11.2024 14:12, Daniel P. Smith wrote:
>>>       init_dom0_cpuid_policy(d);
>>>   
>>>       if ( alloc_dom0_vcpu0(d) == NULL )
>>> -        panic("Error creating d%uv0\n", domid);
>>> +        panic("Error creating d%uv0\n", bd->domid);
>>
>> Imo this would better use d->domain_id. And while touching it, %u would also
>> want swapping for %d.
> 
> hmm, I was actually considering s/d%u/%pd/ and just pass in d, but was 
> certain if there was an explicit reason it wasn't used before. If I am 
> going to change it, would %pd not be more desired here?

When writing my original reply, I certainly considered this. The anomaly
here is that you really mean to log a vCPU ID, which would require a
struct vcpu * and use of %pv. Yet you don't have that here, precisely
because the creation of the vCPU failed. That said, since
vsprintf.c:print_vcpu() calls print_domain(), using %pd is certainly an
option here (inconsistencies would arise if %pv and %pd presented domain
IDs in [perhaps just slightly] different ways).

Jan

On 12/9/24 03:55, Jan Beulich wrote:
> On 04.12.2024 17:45, Daniel P. Smith wrote:
>> On 11/27/24 05:32, Jan Beulich wrote:
>>> On 15.11.2024 14:12, Daniel P. Smith wrote:
>>>>        init_dom0_cpuid_policy(d);
>>>>    
>>>>        if ( alloc_dom0_vcpu0(d) == NULL )
>>>> -        panic("Error creating d%uv0\n", domid);
>>>> +        panic("Error creating d%uv0\n", bd->domid);
>>>
>>> Imo this would better use d->domain_id. And while touching it, %u would also
>>> want swapping for %d.
>>
>> hmm, I was actually considering s/d%u/%pd/ and just pass in d, but was
>> certain if there was an explicit reason it wasn't used before. If I am
>> going to change it, would %pd not be more desired here?
> 
> When writing my original reply, I certainly considered this. The anomaly
> here is that you really mean to log a vCPU ID, which would require a
> struct vcpu * and use of %pv. Yet you don't have that here, precisely
> because the creation of the vCPU failed. That said, since
> vsprintf.c:print_vcpu() calls print_domain(), using %pd is certainly an
> option here (inconsistencies would arise if %pv and %pd presented domain
> IDs in [perhaps just slightly] different ways).

Will do, thanks!

v/r,
dps

On 11/15/24 08:12, Daniel P. Smith wrote:
> Add a domid field to struct boot_domain to hold the assigned domain id for the
> domain. During initialization, ensure all instances of struct boot_domain have
> the invalid domid to ensure that the domid must be set either by convention or
> configuration.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> ---
>   xen/arch/x86/include/asm/bootdomain.h |  2 ++
>   xen/arch/x86/setup.c                  | 12 +++++++-----
>   2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
> index 12c19ab37bd8..3873f916f854 100644
> --- a/xen/arch/x86/include/asm/bootdomain.h
> +++ b/xen/arch/x86/include/asm/bootdomain.h
> @@ -12,6 +12,8 @@ struct boot_module;
>   struct domain;
>   
>   struct boot_domain {
> +    domid_t domid;

There is no definition for domid_t in this file, the only reason it has 
yet to fail, is that everywhere it is included has xen.h included before it.

v/r,
dps