| Message ID | 20230103102519.26224-1-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] xen/arm: Add 0x prefix when printing memory size in construct_domU | expand |
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"); >
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 >
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,
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 >
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,
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
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
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,
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");
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(-)