[PATCH 0/3] small fixes

[PATCH 0/3] small fixes

[Michal Orzel]

Small independent fixes.

Michal Orzel (3):
  xen/arm: traps: Fix reference to invalid erratum ID
  xen/arm: gicv2: Rename gicv2_map_hwdown_extra_mappings
  xen/console: Fix incorrect format tags for struct tm members

 xen/arch/arm/gic-v2.c      | 4 ++--
 xen/arch/arm/traps.c       | 2 +-
 xen/drivers/char/console.c | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH 1/3] xen/arm: traps: Fix reference to invalid erratum ID

[Michal Orzel]

The correct erratum ID should be 834220.

Fixes: 0a7ba2936457 ("xen/arm: arm64: Add Cortex-A57 erratum 834220 workaround")
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 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 43f30747cf..e989e742fd 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1856,7 +1856,7 @@ static inline bool hpfar_is_valid(bool s1ptw, uint8_t fsc)
      *  1. the stage 2 fault happen during a stage 1 page table walk
      *  (the bit ESR_EL2.S1PTW is set)
      *  2. the fault was due to a translation fault and the processor
-     *  does not carry erratum #8342220
+     *  does not carry erratum #834220
      *
      * Note that technically HPFAR is valid for other cases, but they
      * are currently not supported by Xen.
-- 
2.25.1

Re: [PATCH 1/3] xen/arm: traps: Fix reference to invalid erratum ID

[Julien Grall]

Hi Michal,

On 10/06/2022 09:33, Michal Orzel wrote:
> The correct erratum ID should be 834220.
> 
> Fixes: 0a7ba2936457 ("xen/arm: arm64: Add Cortex-A57 erratum 834220 workaround")

Despite my answer in #2, this is one of the exception where I would keep 
the fixes tag because we should try to keep the documentation correct 
even in stable trees (this is more important than typo in names).

> Signed-off-by: Michal Orzel <michal.orzel@arm.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

[PATCH 2/3] xen/arm: gicv2: Rename gicv2_map_hwdown_extra_mappings

[Michal Orzel]

... to gicv2_map_hwdom_extra_mappings as the former clearly contains
a typo.

Fixes: 86b93e00c0b6 ("xen/arm: gicv2: Export GICv2m register frames to Dom0 by device tree")
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/arch/arm/gic-v2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 2cc2f6bc18..bd773bcc67 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -679,7 +679,7 @@ static void gicv2_irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_m
     spin_unlock(&gicv2.lock);
 }
 
-static int gicv2_map_hwdown_extra_mappings(struct domain *d)
+static int gicv2_map_hwdom_extra_mappings(struct domain *d)
 {
     const struct v2m_data *v2m_data;
 
@@ -1352,7 +1352,7 @@ const static struct gic_hw_operations gicv2_ops = {
     .make_hwdom_madt     = gicv2_make_hwdom_madt,
     .get_hwdom_extra_madt_size = gicv2_get_hwdom_extra_madt_size,
 #endif
-    .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings,
+    .map_hwdom_extra_mappings = gicv2_map_hwdom_extra_mappings,
     .iomem_deny_access   = gicv2_iomem_deny_access,
     .do_LPI              = gicv2_do_LPI,
 };
-- 
2.25.1

Re: [PATCH 2/3] xen/arm: gicv2: Rename gicv2_map_hwdown_extra_mappings

[Julien Grall]

Hi,

On 10/06/2022 09:33, Michal Orzel wrote:
> ... to gicv2_map_hwdom_extra_mappings as the former clearly contains
> a typo.
> 
> Fixes: 86b93e00c0b6 ("xen/arm: gicv2: Export GICv2m register frames to Dom0 by device tree"

NIT: In general, fixes tag are used for bug (i.e. Xen would not function 
properly without it and likely need backports). Even if the name is 
incorrect, there is no bug here. So my preference is to drop this tag.

Other than that:

Acked-by; Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH 2/3] xen/arm: gicv2: Rename gicv2_map_hwdown_extra_mappings

[Michal Orzel]

Hi Julien,

On 10.06.2022 11:07, Julien Grall wrote:
> Hi,
> 
> On 10/06/2022 09:33, Michal Orzel wrote:
>> ... to gicv2_map_hwdom_extra_mappings as the former clearly contains
>> a typo.
>>
>> Fixes: 86b93e00c0b6 ("xen/arm: gicv2: Export GICv2m register frames to Dom0 by device tree"
> 
> NIT: In general, fixes tag are used for bug (i.e. Xen would not function properly without it and likely need backports). Even if the name is incorrect, there is no bug here. So my preference is to drop this tag.
> 
Thanks, I will keep it in mind.

> Other than that:
> 
> Acked-by; Julien Grall <jgrall@amazon.com>
NIT: s/;/:

> 
> Cheers,
> 

Cheers,
Michal

[PATCH 3/3] xen/console: Fix incorrect format tags for struct tm members

[Michal Orzel]

All the members of struct tm are defined as integers but the format tags
used in console driver for snprintf wrongly expect unsigned values. Fix
the tags to expect integers.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/drivers/char/console.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index f9937c5134..beb44fe06f 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -844,7 +844,7 @@ static void printk_start_of_line(const char *prefix)
             /* nothing */;
         else if ( mode == TSM_DATE )
         {
-            snprintf(tstr, sizeof(tstr), "[%04u-%02u-%02u %02u:%02u:%02u] ",
+            snprintf(tstr, sizeof(tstr), "[%04d-%02d-%02d %02d:%02d:%02d] ",
                      1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday,
                      tm.tm_hour, tm.tm_min, tm.tm_sec);
             break;
@@ -852,7 +852,7 @@ static void printk_start_of_line(const char *prefix)
         else
         {
             snprintf(tstr, sizeof(tstr),
-                     "[%04u-%02u-%02u %02u:%02u:%02u.%03"PRIu64"] ",
+                     "[%04d-%02d-%02d %02d:%02d:%02d.%03"PRIu64"] ",
                      1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday,
                      tm.tm_hour, tm.tm_min, tm.tm_sec, nsec / 1000000);
             break;
-- 
2.25.1

Re: [PATCH 3/3] xen/console: Fix incorrect format tags for struct tm members

[Jan Beulich]

On 10.06.2022 10:33, Michal Orzel wrote:
> All the members of struct tm are defined as integers but the format tags
> used in console driver for snprintf wrongly expect unsigned values. Fix
> the tags to expect integers.

Perhaps do things the other way around - convert field types to unsigned
unless negative values can be stored there? This would match our general
aim of using unsigned types when only non-negative values can be held in
variables / parameters / fields.

Jan

> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -844,7 +844,7 @@ static void printk_start_of_line(const char *prefix)
>              /* nothing */;
>          else if ( mode == TSM_DATE )
>          {
> -            snprintf(tstr, sizeof(tstr), "[%04u-%02u-%02u %02u:%02u:%02u] ",
> +            snprintf(tstr, sizeof(tstr), "[%04d-%02d-%02d %02d:%02d:%02d] ",
>                       1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday,
>                       tm.tm_hour, tm.tm_min, tm.tm_sec);
>              break;
> @@ -852,7 +852,7 @@ static void printk_start_of_line(const char *prefix)
>          else
>          {
>              snprintf(tstr, sizeof(tstr),
> -                     "[%04u-%02u-%02u %02u:%02u:%02u.%03"PRIu64"] ",
> +                     "[%04d-%02d-%02d %02d:%02d:%02d.%03"PRIu64"] ",
>                       1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday,
>                       tm.tm_hour, tm.tm_min, tm.tm_sec, nsec / 1000000);
>              break;

Re: [PATCH 3/3] xen/console: Fix incorrect format tags for struct tm members

[Michal Orzel]

Hi Jan,

On 10.06.2022 11:44, Jan Beulich wrote:
> On 10.06.2022 10:33, Michal Orzel wrote:
>> All the members of struct tm are defined as integers but the format tags
>> used in console driver for snprintf wrongly expect unsigned values. Fix
>> the tags to expect integers.
> 
> Perhaps do things the other way around - convert field types to unsigned
> unless negative values can be stored there? This would match our general
> aim of using unsigned types when only non-negative values can be held in
> variables / parameters / fields.
> 

The reason why I did not do this is to be coherent with Linux kernel (include/linux/time.h).
I believe the time.h code in Xen comes from Linux.

So if the Linux time.h defines these fields as integers and uses proper %d format specifiers (unlike Xen),
I think my solution is better.

> Jan
Cheers,
Michal

Re: [PATCH 3/3] xen/console: Fix incorrect format tags for struct tm members

[Jan Beulich]

On 10.06.2022 11:50, Michal Orzel wrote:
> On 10.06.2022 11:44, Jan Beulich wrote:
>> On 10.06.2022 10:33, Michal Orzel wrote:
>>> All the members of struct tm are defined as integers but the format tags
>>> used in console driver for snprintf wrongly expect unsigned values. Fix
>>> the tags to expect integers.
>>
>> Perhaps do things the other way around - convert field types to unsigned
>> unless negative values can be stored there? This would match our general
>> aim of using unsigned types when only non-negative values can be held in
>> variables / parameters / fields.
>>
> 
> The reason why I did not do this is to be coherent with Linux kernel (include/linux/time.h).
> I believe the time.h code in Xen comes from Linux.
> 
> So if the Linux time.h defines these fields as integers and uses proper %d format specifiers (unlike Xen),
> I think my solution is better.

One can view it that way, sure. I don't. But I also won't nak this change.

Jan

Re: [PATCH 3/3] xen/console: Fix incorrect format tags for struct tm members

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 648 bytes --]

On 10.06.22 11:44, Jan Beulich wrote:
> On 10.06.2022 10:33, Michal Orzel wrote:
>> All the members of struct tm are defined as integers but the format tags
>> used in console driver for snprintf wrongly expect unsigned values. Fix
>> the tags to expect integers.
> 
> Perhaps do things the other way around - convert field types to unsigned
> unless negative values can be stored there? This would match our general
> aim of using unsigned types when only non-negative values can be held in
> variables / parameters / fields.

Don't you think keeping struct tm in sync with the Posix definition should
be preferred here?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 3/3] xen/console: Fix incorrect format tags for struct tm members

[Jan Beulich]

On 10.06.2022 11:51, Juergen Gross wrote:
> On 10.06.22 11:44, Jan Beulich wrote:
>> On 10.06.2022 10:33, Michal Orzel wrote:
>>> All the members of struct tm are defined as integers but the format tags
>>> used in console driver for snprintf wrongly expect unsigned values. Fix
>>> the tags to expect integers.
>>
>> Perhaps do things the other way around - convert field types to unsigned
>> unless negative values can be stored there? This would match our general
>> aim of using unsigned types when only non-negative values can be held in
>> variables / parameters / fields.
> 
> Don't you think keeping struct tm in sync with the Posix definition should
> be preferred here?

Not necessarily, no. It's not just POSIX which has a (imo bad) habit of
using plain "int" even for values which can never go negative.

Jan

Re: [PATCH 3/3] xen/console: Fix incorrect format tags for struct tm members

[Stefano Stabellini]

On Fri, 10 Jun 2022, Jan Beulich wrote:
> On 10.06.2022 11:51, Juergen Gross wrote:
> > On 10.06.22 11:44, Jan Beulich wrote:
> >> On 10.06.2022 10:33, Michal Orzel wrote:
> >>> All the members of struct tm are defined as integers but the format tags
> >>> used in console driver for snprintf wrongly expect unsigned values. Fix
> >>> the tags to expect integers.
> >>
> >> Perhaps do things the other way around - convert field types to unsigned
> >> unless negative values can be stored there? This would match our general
> >> aim of using unsigned types when only non-negative values can be held in
> >> variables / parameters / fields.
> > 
> > Don't you think keeping struct tm in sync with the Posix definition should
> > be preferred here?
> 
> Not necessarily, no. It's not just POSIX which has a (imo bad) habit of
> using plain "int" even for values which can never go negative.

I committed the other two patches in the series because already acked
and straightforward.

In this case, I think the straightforward/mechanical fix is the one
Michal suggested in this patch: fixing %u to be %d. We could of course
consider changing the definition of tm, and there are valid reasons to
do that as Jan pointed out, but I think this patch is valid as-in
anyway.

So I am happy to give my reviewed-by for this version of the patch, and
we can still consider changing tm to use unsigned if someone feels like
proposing a patch for that.

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Cheers,

Stefano

Re: [PATCH 3/3] xen/console: Fix incorrect format tags for struct tm members

[Michal Orzel]

Hi Stefano,

On 11.06.2022 01:35, Stefano Stabellini wrote:
> On Fri, 10 Jun 2022, Jan Beulich wrote:
>> On 10.06.2022 11:51, Juergen Gross wrote:
>>> On 10.06.22 11:44, Jan Beulich wrote:
>>>> On 10.06.2022 10:33, Michal Orzel wrote:
>>>>> All the members of struct tm are defined as integers but the format tags
>>>>> used in console driver for snprintf wrongly expect unsigned values. Fix
>>>>> the tags to expect integers.
>>>>
>>>> Perhaps do things the other way around - convert field types to unsigned
>>>> unless negative values can be stored there? This would match our general
>>>> aim of using unsigned types when only non-negative values can be held in
>>>> variables / parameters / fields.
>>>
>>> Don't you think keeping struct tm in sync with the Posix definition should
>>> be preferred here?
>>
>> Not necessarily, no. It's not just POSIX which has a (imo bad) habit of
>> using plain "int" even for values which can never go negative.
> 
> I committed the other two patches in the series because already acked
> and straightforward.
> 
> In this case, I think the straightforward/mechanical fix is the one
> Michal suggested in this patch: fixing %u to be %d. We could of course
> consider changing the definition of tm, and there are valid reasons to
> do that as Jan pointed out, but I think this patch is valid as-in
> anyway.
> 
> So I am happy to give my reviewed-by for this version of the patch, and
> we can still consider changing tm to use unsigned if someone feels like
> proposing a patch for that.

It is not true that the members of struct tm cannot go negative.
Examples:
1) tm_year is used to store a number of years elapsed since 1900.
To express years before 1900, this value must be negative.
2) tm_isdst is used to inform whether DST is in effect. Negative value (-1)
means that no information is available.

The above rules are taken into account in a gmtime definition (common/time.c).

The signed/unsigned debate also applies to a calendar time defintion.
The concept of negative value is used to express the time before epoch.

Xen is using signed s_time_t for system time all over the codebase without
any valid reason other than to be coherent with POSIX definition of time_t.

> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> Cheers,
> 
> Stefano

Cheers,
Michal