[PATCH v2] xen/arm: Add 0x prefix when printing memory size in construct_domU

Michal Orzel posted 1 patch 1 year, 2 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20230103102519.26224-1-michal.orzel@amd.com
xen/arch/arm/domain_build.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[PATCH v2] xen/arm: Add 0x prefix when printing memory size in construct_domU
Posted by Michal Orzel 1 year, 2 months ago
Printing memory size in hex without 0x prefix can be misleading, so
add it. Also, take the opportunity to adhere to 80 chars line length
limit by moving the printk arguments to the next line.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
Changes in v2:
 - was: "Print memory size in decimal in construct_domU"
 - stick to hex but add a 0x prefix
 - adhere to 80 chars line length limit
---
 xen/arch/arm/domain_build.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 829cea8de84f..f35f4d24569c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3774,7 +3774,8 @@ static int __init construct_domU(struct domain *d,
     if ( rc != 0 )
         return rc;
 
-    printk("*** LOADING DOMU cpus=%u memory=%"PRIx64"KB ***\n", d->max_vcpus, mem);
+    printk("*** LOADING DOMU cpus=%u memory=%#"PRIx64"KB ***\n",
+           d->max_vcpus, mem);
 
     kinfo.vpl011 = dt_property_read_bool(node, "vpl011");
 
-- 
2.25.1
Re: [PATCH v2] xen/arm: Add 0x prefix when printing memory size in construct_domU
Posted by Stefano Stabellini 1 year, 2 months ago
On Tue, 3 Jan 2023, Michal Orzel wrote:
> Printing memory size in hex without 0x prefix can be misleading, so
> add it. Also, take the opportunity to adhere to 80 chars line length
> limit by moving the printk arguments to the next line.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
> Changes in v2:
>  - was: "Print memory size in decimal in construct_domU"
>  - stick to hex but add a 0x prefix
>  - adhere to 80 chars line length limit

Honestly I prefer decimal but also hex is fine. I'll let Julien pick the
version of this patch that he prefers.


> ---
>  xen/arch/arm/domain_build.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 829cea8de84f..f35f4d24569c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -3774,7 +3774,8 @@ static int __init construct_domU(struct domain *d,
>      if ( rc != 0 )
>          return rc;
>  
> -    printk("*** LOADING DOMU cpus=%u memory=%"PRIx64"KB ***\n", d->max_vcpus, mem);
> +    printk("*** LOADING DOMU cpus=%u memory=%#"PRIx64"KB ***\n",
> +           d->max_vcpus, mem);
>  
>      kinfo.vpl011 = dt_property_read_bool(node, "vpl011");
>  
> -- 
> 2.25.1
>
Re: [PATCH v2] xen/arm: Add 0x prefix when printing memory size in construct_domU
Posted by Julien Grall 1 year, 2 months ago
Hi Stefano,

On 04/01/2023 23:47, Stefano Stabellini wrote:
> On Tue, 3 Jan 2023, Michal Orzel wrote:
>> Printing memory size in hex without 0x prefix can be misleading, so
>> add it. Also, take the opportunity to adhere to 80 chars line length
>> limit by moving the printk arguments to the next line.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>> Changes in v2:
>>   - was: "Print memory size in decimal in construct_domU"
>>   - stick to hex but add a 0x prefix
>>   - adhere to 80 chars line length limit
> 
> Honestly I prefer decimal but also hex is fine. 

decimal is perfect for very small values, but as we print the amount in 
KB it will become a big mess. Here some examples (decimal first, then 
hexadecimal):

   512MB: 524288 vs 0x80000
   555MB: 568320 vs 0x8ac00
   1GB: 1048576 vs 0x100000
   512GB: 536870912 vs 0x20000000
   1TB: 1073741824 vs 0x40000000

For power of two values, you might be able to find your way with 
decimal. It is more difficult for non power of two unless you have a 
calculator in hand.

The other option I suggested in v1 is to print the amount in KB/GB/MB. 
Would that be better?

That said, to be honest, I am not entirely sure why we are actually 
printing in KB. It would seems strange that someone would create a guest 
with memory not aligned to 1MB.

So I would consider to check the size is 1MB-aligned and then print the 
value in KB. This will remove one order of magnitude and make the value 
more readable in decimal.

Cheers,

-- 
Julien Grall
Re: [PATCH v2] xen/arm: Add 0x prefix when printing memory size in construct_domU
Posted by Ayan Kumar Halder 1 year, 2 months ago
Hi Julien,

I have a clarification.

On 05/01/2023 09:26, Julien Grall wrote:
> CAUTION: This message has originated from an External Source. Please 
> use proper judgment and caution when opening attachments, clicking 
> links, or responding to this email.
>
>
> Hi Stefano,
>
> On 04/01/2023 23:47, Stefano Stabellini wrote:
>> On Tue, 3 Jan 2023, Michal Orzel wrote:
>>> Printing memory size in hex without 0x prefix can be misleading, so
>>> add it. Also, take the opportunity to adhere to 80 chars line length
>>> limit by moving the printk arguments to the next line.
>>>
>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>> ---
>>> Changes in v2:
>>>   - was: "Print memory size in decimal in construct_domU"
>>>   - stick to hex but add a 0x prefix
>>>   - adhere to 80 chars line length limit
>>
>> Honestly I prefer decimal but also hex is fine.
>
> decimal is perfect for very small values, but as we print the amount in
> KB it will become a big mess. Here some examples (decimal first, then
> hexadecimal):
>
>   512MB: 524288 vs 0x80000
>   555MB: 568320 vs 0x8ac00
>   1GB: 1048576 vs 0x100000
>   512GB: 536870912 vs 0x20000000
>   1TB: 1073741824 vs 0x40000000
>
> For power of two values, you might be able to find your way with
> decimal. It is more difficult for non power of two unless you have a
> calculator in hand.
>
> The other option I suggested in v1 is to print the amount in KB/GB/MB.
> Would that be better?
>
> That said, to be honest, I am not entirely sure why we are actually
> printing in KB. It would seems strange that someone would create a guest
> with memory not aligned to 1MB.

For RTOS (Zephyr and FreeRTOS), it should be possible for guests to have 
memory less than 1 MB, isn't it ?

- Ayan

>
> So I would consider to check the size is 1MB-aligned and then print the
> value in KB. This will remove one order of magnitude and make the value
> more readable in decimal.
>
> Cheers,
>
> -- 
> Julien Grall
>

Re: [PATCH v2] xen/arm: Add 0x prefix when printing memory size in construct_domU
Posted by Julien Grall 1 year, 2 months ago
On 05/01/2023 09:59, Ayan Kumar Halder wrote:
> Hi Julien,

Hi,

> I have a clarification.
> 
> On 05/01/2023 09:26, Julien Grall wrote:
>> CAUTION: This message has originated from an External Source. Please 
>> use proper judgment and caution when opening attachments, clicking 
>> links, or responding to this email.
>>
>>
>> Hi Stefano,
>>
>> On 04/01/2023 23:47, Stefano Stabellini wrote:
>>> On Tue, 3 Jan 2023, Michal Orzel wrote:
>>>> Printing memory size in hex without 0x prefix can be misleading, so
>>>> add it. Also, take the opportunity to adhere to 80 chars line length
>>>> limit by moving the printk arguments to the next line.
>>>>
>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>> ---
>>>> Changes in v2:
>>>>   - was: "Print memory size in decimal in construct_domU"
>>>>   - stick to hex but add a 0x prefix
>>>>   - adhere to 80 chars line length limit
>>>
>>> Honestly I prefer decimal but also hex is fine.
>>
>> decimal is perfect for very small values, but as we print the amount in
>> KB it will become a big mess. Here some examples (decimal first, then
>> hexadecimal):
>>
>>   512MB: 524288 vs 0x80000
>>   555MB: 568320 vs 0x8ac00
>>   1GB: 1048576 vs 0x100000
>>   512GB: 536870912 vs 0x20000000
>>   1TB: 1073741824 vs 0x40000000
>>
>> For power of two values, you might be able to find your way with
>> decimal. It is more difficult for non power of two unless you have a
>> calculator in hand.
>>
>> The other option I suggested in v1 is to print the amount in KB/GB/MB.
>> Would that be better?
>>
>> That said, to be honest, I am not entirely sure why we are actually
>> printing in KB. It would seems strange that someone would create a guest
>> with memory not aligned to 1MB.
> 
> For RTOS (Zephyr and FreeRTOS), it should be possible for guests to have 
> memory less than 1 MB, isn't it ?

Yes. So does XTF. But most of the users are likely going allocate at 
least 1MB (or even 2MB to reduce the TLB pressure).

So it would be better to print the value in a way that is more 
meaningful for the majority of the users.

>> So I would consider to check the size is 1MB-aligned and then print the

I will retract my suggestion to check the size. There are technically no 
restriction to run a guest with a size not aligned to 1MB. Although, it 
would still seem strange.

Cheers,

-- 
Julien Grall

Re: [PATCH v2] xen/arm: Add 0x prefix when printing memory size in construct_domU
Posted by Andrew Cooper 1 year, 2 months ago
On 05/01/2023 11:19 am, Julien Grall wrote:
> On 05/01/2023 09:59, Ayan Kumar Halder wrote:
>> Hi Julien,
>
> Hi,
>
>> I have a clarification.
>>
>> On 05/01/2023 09:26, Julien Grall wrote:
>>> CAUTION: This message has originated from an External Source. Please
>>> use proper judgment and caution when opening attachments, clicking
>>> links, or responding to this email.
>>>
>>>
>>> Hi Stefano,
>>>
>>> On 04/01/2023 23:47, Stefano Stabellini wrote:
>>>> On Tue, 3 Jan 2023, Michal Orzel wrote:
>>>>> Printing memory size in hex without 0x prefix can be misleading, so
>>>>> add it. Also, take the opportunity to adhere to 80 chars line length
>>>>> limit by moving the printk arguments to the next line.
>>>>>
>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>>> ---
>>>>> Changes in v2:
>>>>>   - was: "Print memory size in decimal in construct_domU"
>>>>>   - stick to hex but add a 0x prefix
>>>>>   - adhere to 80 chars line length limit
>>>>
>>>> Honestly I prefer decimal but also hex is fine.
>>>
>>> decimal is perfect for very small values, but as we print the amount in
>>> KB it will become a big mess. Here some examples (decimal first, then
>>> hexadecimal):
>>>
>>>   512MB: 524288 vs 0x80000
>>>   555MB: 568320 vs 0x8ac00
>>>   1GB: 1048576 vs 0x100000
>>>   512GB: 536870912 vs 0x20000000
>>>   1TB: 1073741824 vs 0x40000000
>>>
>>> For power of two values, you might be able to find your way with
>>> decimal. It is more difficult for non power of two unless you have a
>>> calculator in hand.
>>>
>>> The other option I suggested in v1 is to print the amount in KB/GB/MB.
>>> Would that be better?
>>>
>>> That said, to be honest, I am not entirely sure why we are actually
>>> printing in KB. It would seems strange that someone would create a
>>> guest
>>> with memory not aligned to 1MB.
>>
>> For RTOS (Zephyr and FreeRTOS), it should be possible for guests to
>> have memory less than 1 MB, isn't it ?
>
> Yes. So does XTF. But most of the users are likely going allocate at
> least 1MB (or even 2MB to reduce the TLB pressure).
>
> So it would be better to print the value in a way that is more
> meaningful for the majority of the users.
>
>>> So I would consider to check the size is 1MB-aligned and then print the
>
> I will retract my suggestion to check the size. There are technically
> no restriction to run a guest with a size not aligned to 1MB.
> Although, it would still seem strange.

I have a need to extend tools/tests/tsx with a VM that is a single 4k
page.  Something which can execute CPUID in the context of a VM and
cross-check the results with what the "toolstack" (test) tried to configure.

Xen is buggy if it cannot operate a VM which looks like that, and a
bonus of explicitly testing like this is that it helps to remove
inappropriate checks.

~Andrew
Re: [PATCH v2] xen/arm: Add 0x prefix when printing memory size in construct_domU
Posted by Michal Orzel 1 year, 2 months ago

On 05/01/2023 13:15, Andrew Cooper wrote:
> 
> 
> On 05/01/2023 11:19 am, Julien Grall wrote:
>> On 05/01/2023 09:59, Ayan Kumar Halder wrote:
>>> Hi Julien,
>>
>> Hi,
>>
>>> I have a clarification.
>>>
>>> On 05/01/2023 09:26, Julien Grall wrote:
>>>> CAUTION: This message has originated from an External Source. Please
>>>> use proper judgment and caution when opening attachments, clicking
>>>> links, or responding to this email.
>>>>
>>>>
>>>> Hi Stefano,
>>>>
>>>> On 04/01/2023 23:47, Stefano Stabellini wrote:
>>>>> On Tue, 3 Jan 2023, Michal Orzel wrote:
>>>>>> Printing memory size in hex without 0x prefix can be misleading, so
>>>>>> add it. Also, take the opportunity to adhere to 80 chars line length
>>>>>> limit by moving the printk arguments to the next line.
>>>>>>
>>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>>>> ---
>>>>>> Changes in v2:
>>>>>>   - was: "Print memory size in decimal in construct_domU"
>>>>>>   - stick to hex but add a 0x prefix
>>>>>>   - adhere to 80 chars line length limit
>>>>>
>>>>> Honestly I prefer decimal but also hex is fine.
>>>>
>>>> decimal is perfect for very small values, but as we print the amount in
>>>> KB it will become a big mess. Here some examples (decimal first, then
>>>> hexadecimal):
>>>>
>>>>   512MB: 524288 vs 0x80000
>>>>   555MB: 568320 vs 0x8ac00
>>>>   1GB: 1048576 vs 0x100000
>>>>   512GB: 536870912 vs 0x20000000
>>>>   1TB: 1073741824 vs 0x40000000
>>>>
>>>> For power of two values, you might be able to find your way with
>>>> decimal. It is more difficult for non power of two unless you have a
>>>> calculator in hand.
>>>>
>>>> The other option I suggested in v1 is to print the amount in KB/GB/MB.
>>>> Would that be better?
>>>>
>>>> That said, to be honest, I am not entirely sure why we are actually
>>>> printing in KB. It would seems strange that someone would create a
>>>> guest
>>>> with memory not aligned to 1MB.
>>>
>>> For RTOS (Zephyr and FreeRTOS), it should be possible for guests to
>>> have memory less than 1 MB, isn't it ?
>>
>> Yes. So does XTF. But most of the users are likely going allocate at
>> least 1MB (or even 2MB to reduce the TLB pressure).
>>
>> So it would be better to print the value in a way that is more
>> meaningful for the majority of the users.
>>
>>>> So I would consider to check the size is 1MB-aligned and then print the
>>
>> I will retract my suggestion to check the size. There are technically
>> no restriction to run a guest with a size not aligned to 1MB.
>> Although, it would still seem strange.
> 
> I have a need to extend tools/tests/tsx with a VM that is a single 4k
> page.  Something which can execute CPUID in the context of a VM and
> cross-check the results with what the "toolstack" (test) tried to configure.
> 
> Xen is buggy if it cannot operate a VM which looks like that, and a
> bonus of explicitly testing like this is that it helps to remove
> inappropriate checks.
I can see we are all settled that it is fully ok to boot guests with memory size less than 1MB.
The 'memory' dt parameter for dom0less domUs requires being specified in KB and this is the
smallest common multiple so it makes the process of cross-checking easier. Stefano is ok with
either decimal or hex, Julien wanted hex (hence my v2), I'm more towards printing in hex as well.
Let's not forget that this patch aims to fix a misleading print that was missing 0x and can cause
someone to take it as a decimal value.

> 
> ~Andrew

~Michal
Re: [PATCH v2] xen/arm: Add 0x prefix when printing memory size in construct_domU
Posted by Julien Grall 1 year, 2 months ago
Hi Michal,

On 06/01/2023 10:41, Michal Orzel wrote:
> 
> 
> On 05/01/2023 13:15, Andrew Cooper wrote:
>>
>>
>> On 05/01/2023 11:19 am, Julien Grall wrote:
>>> On 05/01/2023 09:59, Ayan Kumar Halder wrote:
>>>> Hi Julien,
>>>
>>> Hi,
>>>
>>>> I have a clarification.
>>>>
>>>> On 05/01/2023 09:26, Julien Grall wrote:
>>>>> CAUTION: This message has originated from an External Source. Please
>>>>> use proper judgment and caution when opening attachments, clicking
>>>>> links, or responding to this email.
>>>>>
>>>>>
>>>>> Hi Stefano,
>>>>>
>>>>> On 04/01/2023 23:47, Stefano Stabellini wrote:
>>>>>> On Tue, 3 Jan 2023, Michal Orzel wrote:
>>>>>>> Printing memory size in hex without 0x prefix can be misleading, so
>>>>>>> add it. Also, take the opportunity to adhere to 80 chars line length
>>>>>>> limit by moving the printk arguments to the next line.
>>>>>>>
>>>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>>>>> ---
>>>>>>> Changes in v2:
>>>>>>>    - was: "Print memory size in decimal in construct_domU"
>>>>>>>    - stick to hex but add a 0x prefix
>>>>>>>    - adhere to 80 chars line length limit
>>>>>>
>>>>>> Honestly I prefer decimal but also hex is fine.
>>>>>
>>>>> decimal is perfect for very small values, but as we print the amount in
>>>>> KB it will become a big mess. Here some examples (decimal first, then
>>>>> hexadecimal):
>>>>>
>>>>>    512MB: 524288 vs 0x80000
>>>>>    555MB: 568320 vs 0x8ac00
>>>>>    1GB: 1048576 vs 0x100000
>>>>>    512GB: 536870912 vs 0x20000000
>>>>>    1TB: 1073741824 vs 0x40000000
>>>>>
>>>>> For power of two values, you might be able to find your way with
>>>>> decimal. It is more difficult for non power of two unless you have a
>>>>> calculator in hand.
>>>>>
>>>>> The other option I suggested in v1 is to print the amount in KB/GB/MB.
>>>>> Would that be better?
>>>>>
>>>>> That said, to be honest, I am not entirely sure why we are actually
>>>>> printing in KB. It would seems strange that someone would create a
>>>>> guest
>>>>> with memory not aligned to 1MB.
>>>>
>>>> For RTOS (Zephyr and FreeRTOS), it should be possible for guests to
>>>> have memory less than 1 MB, isn't it ?
>>>
>>> Yes. So does XTF. But most of the users are likely going allocate at
>>> least 1MB (or even 2MB to reduce the TLB pressure).
>>>
>>> So it would be better to print the value in a way that is more
>>> meaningful for the majority of the users.
>>>
>>>>> So I would consider to check the size is 1MB-aligned and then print the
>>>
>>> I will retract my suggestion to check the size. There are technically
>>> no restriction to run a guest with a size not aligned to 1MB.
>>> Although, it would still seem strange.
>>
>> I have a need to extend tools/tests/tsx with a VM that is a single 4k
>> page.  Something which can execute CPUID in the context of a VM and
>> cross-check the results with what the "toolstack" (test) tried to configure.
>>
>> Xen is buggy if it cannot operate a VM which looks like that, and a
>> bonus of explicitly testing like this is that it helps to remove
>> inappropriate checks.
> I can see we are all settled that it is fully ok to boot guests with memory size less than 1MB.
> The 'memory' dt parameter for dom0less domUs requires being specified in KB and this is the
> smallest common multiple so it makes the process of cross-checking easier. Stefano is ok with
> either decimal or hex, Julien wanted hex (hence my v2), I'm more towards printing in hex as well.
> Let's not forget that this patch aims to fix a misleading print that was missing 0x and can cause
> someone to take it as a decimal value.

I have committed it with my acked-by.

Cheers,

-- 
Julien Grall
Re: [PATCH v2] xen/arm: Add 0x prefix when printing memory size in construct_domU
Posted by Ayan Kumar Halder 1 year, 2 months ago
On 03/01/2023 10:25, Michal Orzel wrote:
> Printing memory size in hex without 0x prefix can be misleading, so
> add it. Also, take the opportunity to adhere to 80 chars line length
> limit by moving the printk arguments to the next line.
>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> Changes in v2:
>   - was: "Print memory size in decimal in construct_domU"
>   - stick to hex but add a 0x prefix
>   - adhere to 80 chars line length limit
> ---
>   xen/arch/arm/domain_build.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 829cea8de84f..f35f4d24569c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -3774,7 +3774,8 @@ static int __init construct_domU(struct domain *d,
>       if ( rc != 0 )
>           return rc;
>   
> -    printk("*** LOADING DOMU cpus=%u memory=%"PRIx64"KB ***\n", d->max_vcpus, mem);
> +    printk("*** LOADING DOMU cpus=%u memory=%#"PRIx64"KB ***\n",
> +           d->max_vcpus, mem);
>   
>       kinfo.vpl011 = dt_property_read_bool(node, "vpl011");
>