[PATCH] xen/arm: traps: Add missing 0x in bad_trap

[PATCH] xen/arm: traps: Add missing 0x in bad_trap

[Julien Grall]

The syndrome value is printed in hexadecimal. Prefix it by 0x for less
confusion.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/traps.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 9eeed92..858abe5 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1616,7 +1616,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
         break;
     default:
  bad_trap:
-        printk("Hypervisor Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=%"PRIx32"\n",
+        printk("Hypervisor Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
                hsr.bits, hsr.ec, hsr.len, hsr.iss);
         do_unexpected_trap("Hypervisor", regs);
     }
-- 
1.7.10.4

Re: [PATCH] xen/arm: traps: Add missing 0x in bad_trap

[Andrew Cooper]

On 10/04/14 12:44, Julien Grall wrote:
> The syndrome value is printed in hexadecimal. Prefix it by 0x for less
> confusion.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Xen is trying to converge on '%#' rather than an explicit 0x.

Also, it looks as if you want to do the same for IL

~Andrew

> ---
>  xen/arch/arm/traps.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 9eeed92..858abe5 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1616,7 +1616,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>          break;
>      default:
>   bad_trap:
> -        printk("Hypervisor Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=%"PRIx32"\n",
> +        printk("Hypervisor Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
>                 hsr.bits, hsr.ec, hsr.len, hsr.iss);
>          do_unexpected_trap("Hypervisor", regs);
>      }

Re: [PATCH] xen/arm: traps: Add missing 0x in bad_trap

[Julien Grall]

On 04/10/2014 12:53 PM, Andrew Cooper wrote:
> On 10/04/14 12:44, Julien Grall wrote:
>> The syndrome value is printed in hexadecimal. Prefix it by 0x for less
>> confusion.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Xen is trying to converge on '%#' rather than an explicit 0x.

I use to prefer explicit 0x :). Anyway, I will change to use %#.

> 
> Also, it looks as if you want to do the same for IL

Oh right, I forgot this one. I will send a new version of the patch.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: traps: Add missing 0x in bad_trap

[Ian Campbell]

On Thu, 2014-04-10 at 12:53 +0100, Andrew Cooper wrote:
> On 10/04/14 12:44, Julien Grall wrote:
> > The syndrome value is printed in hexadecimal. Prefix it by 0x for less
> > confusion.
> >
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Xen is trying to converge on '%#' rather than an explicit 0x.

Really? I've been moving in the opposite direction because e.g. %010#lx
does the wrong thing with zero (0000000000 instead of 0x00000000).

Not sure about plain %#x though...

> Also, it looks as if you want to do the same for IL

IL must be 2 or 4, for clarity it would be better as %d rather than 0x%x
I think.

> 
> ~Andrew
> 
> > ---
> >  xen/arch/arm/traps.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 9eeed92..858abe5 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1616,7 +1616,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
> >          break;
> >      default:
> >   bad_trap:
> > -        printk("Hypervisor Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=%"PRIx32"\n",
> > +        printk("Hypervisor Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
> >                 hsr.bits, hsr.ec, hsr.len, hsr.iss);
> >          do_unexpected_trap("Hypervisor", regs);
> >      }
> 

Re: [PATCH] xen/arm: traps: Add missing 0x in bad_trap

[Andrew Cooper]

On 10/04/14 13:01, Ian Campbell wrote:
> On Thu, 2014-04-10 at 12:53 +0100, Andrew Cooper wrote:
>> On 10/04/14 12:44, Julien Grall wrote:
>>> The syndrome value is printed in hexadecimal. Prefix it by 0x for less
>>> confusion.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> Xen is trying to converge on '%#' rather than an explicit 0x.
> Really? I've been moving in the opposite direction because e.g. %010#lx
> does the wrong thing with zero (0000000000 instead of 0x00000000).
>
> Not sure about plain %#x though...

%#x DoesTheRightThing, and is a character shorter than its alternative. 
Jan appears to have replaced most of the x86 examples.

I also get slightly frustrated with Format strings with an explicit
width do the specified thing (limit the length of the entire formatted
entry), rather than the useful thing (sticking '0x' before a number with
an explicit width).

By the time you have got a zero-extended explicitly-width'd format
string, the 0x is hardly the end of the world

>
>> Also, it looks as if you want to do the same for IL
> IL must be 2 or 4, for clarity it would be better as %d rather than 0x%x
> I think.

Agreed

~Andrew

Re: [PATCH] xen/arm: traps: Add missing 0x in bad_trap

[Ian Campbell]

On Thu, 2014-04-10 at 12:44 +0100, Julien Grall wrote:
> The syndrome value is printed in hexadecimal. Prefix it by 0x for less
> confusion.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

I've acked + applied this. Irrespective of any apparent move to prefer %
#x this is an improvement.

> ---
>  xen/arch/arm/traps.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 9eeed92..858abe5 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1616,7 +1616,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>          break;
>      default:
>   bad_trap:
> -        printk("Hypervisor Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=%"PRIx32"\n",
> +        printk("Hypervisor Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
>                 hsr.bits, hsr.ec, hsr.len, hsr.iss);
>          do_unexpected_trap("Hypervisor", regs);
>      }

Re: [PATCH] xen/arm: traps: Add missing 0x in bad_trap

[Julien Grall]

On 05/02/2014 01:56 PM, Ian Campbell wrote:
> On Thu, 2014-04-10 at 12:44 +0100, Julien Grall wrote:
>> The syndrome value is printed in hexadecimal. Prefix it by 0x for less
>> confusion.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> I've acked + applied this. Irrespective of any apparent move to prefer %
> #x this is an improvement.

Thanks. So what was the conclusion about % vs #x. Which one shall we use?

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: traps: Add missing 0x in bad_trap

[Ian Campbell]

On Fri, 2014-05-02 at 14:19 +0100, Julien Grall wrote:
> On 05/02/2014 01:56 PM, Ian Campbell wrote:
> > On Thu, 2014-04-10 at 12:44 +0100, Julien Grall wrote:
> >> The syndrome value is printed in hexadecimal. Prefix it by 0x for less
> >> confusion.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > 
> > I've acked + applied this. Irrespective of any apparent move to prefer %
> > #x this is an improvement.
> 
> Thanks. So what was the conclusion about % vs #x. Which one shall we use?

I'm perfectly fine with 0x%x for ARM code. I think Andy's comment about
"Xen moving towards" was overstating things somewhat.

Ian.

Re: [PATCH] xen/arm: traps: Add missing 0x in bad_trap

[Julien Grall]

On 05/02/2014 02:21 PM, Ian Campbell wrote:
> On Fri, 2014-05-02 at 14:19 +0100, Julien Grall wrote:
>> On 05/02/2014 01:56 PM, Ian Campbell wrote:
>>> On Thu, 2014-04-10 at 12:44 +0100, Julien Grall wrote:
>>>> The syndrome value is printed in hexadecimal. Prefix it by 0x for less
>>>> confusion.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>
>>> I've acked + applied this. Irrespective of any apparent move to prefer %
>>> #x this is an improvement.
>>
>> Thanks. So what was the conclusion about % vs #x. Which one shall we use?
> 
> I'm perfectly fine with 0x%x for ARM code. I think Andy's comment about
> "Xen moving towards" was overstating things somewhat.

Ok. FIY, Jan Beulich asked me to change some 0x%x into #x in common code
when I sent patch a couple of month ago.

-- 
Julien Grall

Re: [PATCH] xen/arm: traps: Add missing 0x in bad_trap

[Jan Beulich]

>>> On 02.05.14 at 15:25, <julien.grall@linaro.org> wrote:
> On 05/02/2014 02:21 PM, Ian Campbell wrote:
>> On Fri, 2014-05-02 at 14:19 +0100, Julien Grall wrote:
>>> On 05/02/2014 01:56 PM, Ian Campbell wrote:
>>>> On Thu, 2014-04-10 at 12:44 +0100, Julien Grall wrote:
>>>>> The syndrome value is printed in hexadecimal. Prefix it by 0x for less
>>>>> confusion.
>>>>>
>>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>>
>>>> I've acked + applied this. Irrespective of any apparent move to prefer %
>>>> #x this is an improvement.
>>>
>>> Thanks. So what was the conclusion about % vs #x. Which one shall we use?
>> 
>> I'm perfectly fine with 0x%x for ARM code. I think Andy's comment about
>> "Xen moving towards" was overstating things somewhat.
> 
> Ok. FIY, Jan Beulich asked me to change some 0x%x into #x in common code
> when I sent patch a couple of month ago.

I can only repeat that: The format string is one byte shorter with
the %#x form, and since this accumulates and we shouldn't be
making the hypervisor image bigger than it needs to be, I think we
should prefer that form. But of course - as everywhere - the
maintainer(s) of a particular piece of code has/have the final say.

Jan

Re: [PATCH] xen/arm: traps: Add missing 0x in bad_trap

[Ian Campbell]

On Fri, 2014-05-02 at 15:08 +0100, Jan Beulich wrote:
> >>> On 02.05.14 at 15:25, <julien.grall@linaro.org> wrote:
> > On 05/02/2014 02:21 PM, Ian Campbell wrote:
> >> On Fri, 2014-05-02 at 14:19 +0100, Julien Grall wrote:
> >>> On 05/02/2014 01:56 PM, Ian Campbell wrote:
> >>>> On Thu, 2014-04-10 at 12:44 +0100, Julien Grall wrote:
> >>>>> The syndrome value is printed in hexadecimal. Prefix it by 0x for less
> >>>>> confusion.
> >>>>>
> >>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >>>>
> >>>> I've acked + applied this. Irrespective of any apparent move to prefer %
> >>>> #x this is an improvement.
> >>>
> >>> Thanks. So what was the conclusion about % vs #x. Which one shall we use?
> >> 
> >> I'm perfectly fine with 0x%x for ARM code. I think Andy's comment about
> >> "Xen moving towards" was overstating things somewhat.
> > 
> > Ok. FIY, Jan Beulich asked me to change some 0x%x into #x in common code
> > when I sent patch a couple of month ago.
> 
> I can only repeat that: The format string is one byte shorter with
> the %#x form, and since this accumulates and we shouldn't be
> making the hypervisor image bigger than it needs to be, I think we
> should prefer that form.

We'd need > 2000 of these before there was an even chance of pushing us
into the next page. Is this really the lowest hanging fruit in terms of
hypervisor size?

>  But of course - as everywhere - the
> maintainer(s) of a particular piece of code has/have the final say.

TBH it's purely cosmetic, for values which one expects to see in hex I
prefer to see 0x0 to 0, especially in e.g. a list of 0 and non-0 items.

Ian.