tools/init-dom0less: Fix cpus > 1 and xenstore entries

[PATCH] tools/init-dom0less: Fix cpus > 1 and xenstore entries

Posted by Jason Andryuk 3 days, 17 hours ago

The trailing / in the path is incorrect and generates an error when
writing to xenstore:
Checking domid: 1
Init dom0less domain: 1
init-dom0less: writing to xenstore: No error information

init-dom0less exits without finishing initialization.

vcpu_max_id is an inclusive value, so it should be included in the
loop's range to include all vcpus.  Without this, no xenstore entries
are created for a 1 vcpu domain.

Finally, use vcpu_online, the count of online vcpus, to determine online
vs. offline.  info->cpupool is a cpupool id and not a bitmask.

Fixes: ec53e0c4ea ("tools: add example application to initialize dom0less PV drivers")
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
 tools/helpers/init-dom0less.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
index 17579fe2e8..c569a890a0 100644
--- a/tools/helpers/init-dom0less.c
+++ b/tools/helpers/init-dom0less.c
@@ -182,13 +182,13 @@ retry_transaction:
     if (!do_xs_write_dom(xsh, t, domid, "vm", vm_val_str)) goto err;
     if (!do_xs_write_dom(xsh, t, domid, "name", dom_name_str)) goto err;
     if (!do_xs_write_dom(xsh, t, domid, "cpu", "")) goto err;
-    for (i = 0; i < info->vcpu_max_id; i++) {
-        rc = snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%u/availability/", i);
+    for (i = 0; i <= info->vcpu_max_id; i++) {
+        rc = snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%u/availability", i);
         if (rc < 0 || rc >= STR_MAX_LENGTH)
             goto err;
         rc = -EIO;
         if (!do_xs_write_dom(xsh, t, domid, cpu_str,
-                             (info->cpupool & (1 << i)) ? "online" : "offline"))
+                             i < info->vcpu_online ? "online" : "offline"))
             goto err;
     }
 
-- 
2.48.1

Re: [PATCH] tools/init-dom0less: Fix cpus > 1 and xenstore entries

Posted by Orzel, Michal 3 days, 4 hours ago


On 05/03/2025 22:58, Jason Andryuk wrote:
> 
> 
> The trailing / in the path is incorrect and generates an error when
> writing to xenstore:
> Checking domid: 1
> Init dom0less domain: 1
> init-dom0less: writing to xenstore: No error information
> 
> init-dom0less exits without finishing initialization.
> 
> vcpu_max_id is an inclusive value, so it should be included in the
> loop's range to include all vcpus.  Without this, no xenstore entries
> are created for a 1 vcpu domain.
> 
> Finally, use vcpu_online, the count of online vcpus, to determine online
> vs. offline.  info->cpupool is a cpupool id and not a bitmask.
> 
> Fixes: ec53e0c4ea ("tools: add example application to initialize dom0less PV drivers")
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> ---
>  tools/helpers/init-dom0less.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
> index 17579fe2e8..c569a890a0 100644
> --- a/tools/helpers/init-dom0less.c
> +++ b/tools/helpers/init-dom0less.c
> @@ -182,13 +182,13 @@ retry_transaction:
>      if (!do_xs_write_dom(xsh, t, domid, "vm", vm_val_str)) goto err;
>      if (!do_xs_write_dom(xsh, t, domid, "name", dom_name_str)) goto err;
>      if (!do_xs_write_dom(xsh, t, domid, "cpu", "")) goto err;
> -    for (i = 0; i < info->vcpu_max_id; i++) {
> -        rc = snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%u/availability/", i);
> +    for (i = 0; i <= info->vcpu_max_id; i++) {
> +        rc = snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%u/availability", i);
Up until this point:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

That said...

>          if (rc < 0 || rc >= STR_MAX_LENGTH)
>              goto err;
>          rc = -EIO;
>          if (!do_xs_write_dom(xsh, t, domid, cpu_str,
> -                             (info->cpupool & (1 << i)) ? "online" : "offline"))
> +                             i < info->vcpu_online ? "online" : "offline"))
I struggle with this one. Let's say that a dom0less domU starts with 4 vCPUs and
later on (before executing init-dom0less from dom0), decides to kill it's 2nd
vCPU. So domU is running vCPU{0,2,3}. With your patch, after executing the
script, the 4th vCPU will have its availability set to offline and domU will get
notified to kill its 4th vCPU. That does not sound right...

~Michal

Re: [PATCH] tools/init-dom0less: Fix cpus > 1 and xenstore entries

Posted by Jason Andryuk 3 days ago

On 2025-03-06 05:29, Orzel, Michal wrote:
> 
> 
> On 05/03/2025 22:58, Jason Andryuk wrote:
>>
>>
>> The trailing / in the path is incorrect and generates an error when
>> writing to xenstore:
>> Checking domid: 1
>> Init dom0less domain: 1
>> init-dom0less: writing to xenstore: No error information
>>
>> init-dom0less exits without finishing initialization.
>>
>> vcpu_max_id is an inclusive value, so it should be included in the
>> loop's range to include all vcpus.  Without this, no xenstore entries
>> are created for a 1 vcpu domain.
>>
>> Finally, use vcpu_online, the count of online vcpus, to determine online
>> vs. offline.  info->cpupool is a cpupool id and not a bitmask.
>>
>> Fixes: ec53e0c4ea ("tools: add example application to initialize dom0less PV drivers")
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>> ---
>>   tools/helpers/init-dom0less.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
>> index 17579fe2e8..c569a890a0 100644
>> --- a/tools/helpers/init-dom0less.c
>> +++ b/tools/helpers/init-dom0less.c
>> @@ -182,13 +182,13 @@ retry_transaction:
>>       if (!do_xs_write_dom(xsh, t, domid, "vm", vm_val_str)) goto err;
>>       if (!do_xs_write_dom(xsh, t, domid, "name", dom_name_str)) goto err;
>>       if (!do_xs_write_dom(xsh, t, domid, "cpu", "")) goto err;
>> -    for (i = 0; i < info->vcpu_max_id; i++) {
>> -        rc = snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%u/availability/", i);
>> +    for (i = 0; i <= info->vcpu_max_id; i++) {
>> +        rc = snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%u/availability", i);
> Up until this point:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> 
> That said...
> 
>>           if (rc < 0 || rc >= STR_MAX_LENGTH)
>>               goto err;
>>           rc = -EIO;
>>           if (!do_xs_write_dom(xsh, t, domid, cpu_str,
>> -                             (info->cpupool & (1 << i)) ? "online" : "offline"))
>> +                             i < info->vcpu_online ? "online" : "offline"))
> I struggle with this one. Let's say that a dom0less domU starts with 4 vCPUs and
> later on (before executing init-dom0less from dom0), decides to kill it's 2nd
> vCPU. So domU is running vCPU{0,2,3}. With your patch, after executing the
> script, the 4th vCPU will have its availability set to offline and domU will get
> notified to kill its 4th vCPU. That does not sound right...

You are correct.

With xl create, cpus 0..vcpus-1 are marked online and vcpus...maxvcpus-1 
are marked offline.  I was trying to match that.  To online or offline 
cpus, xl vcpu-set keeps them densely packed, 0..$m.

I think libxl_list_vcpu() should give the needed information to set 
these correctly.  I'll try that.

Thanks for taking a look.

-Jason