[XEN v3 0/3] Pre-requisite patches for supporting 32 bit physical address

[XEN v3 0/3] Pre-requisite patches for supporting 32 bit physical address

[Ayan Kumar Halder]

Hi All,

These series include some patches and fixes identified during the review of
"[XEN v2 00/11] Add support for 32 bit physical address".

Patch 1/3 : The previous version causes CI to fail. This patch attempts to fix
this.

Patch 2/3 : This was pointed by Jan during the review of
"[XEN v2 05/11] xen/arm: Use paddr_t instead of u64 for address/size".
Similar to Patch 1/3, this can also be considered as a pre-req for supporting
32 bit physical address.

Patch 3/3 : This was also pointed by Jan during the review of
"[XEN v2 05/11] xen/arm: Use paddr_t instead of u64 for address/size".

Ayan Kumar Halder (3):
  xen/arm: Use the correct format specifier
  xen/drivers: ns16550: Fix the use of simple_strtoul() for extracting
    u64
  xen/drivers: ns16550: Fix an incorrect assignment to uart->io_size

 xen/arch/arm/domain_build.c | 45 +++++++++++++++++--------------------
 xen/arch/arm/gic-v2.c       |  6 ++---
 xen/arch/arm/mm.c           |  2 +-
 xen/drivers/char/ns16550.c  |  6 ++---
 4 files changed, 28 insertions(+), 31 deletions(-)

-- 
2.17.1

[XEN v3 1/3] xen/arm: Use the correct format specifier

[Ayan Kumar Halder]

1. One should use 'PRIpaddr' to display 'paddr_t' variables. However,
while creating nodes in fdt, the address (if present in the node name)
should be represented using 'PRIx64'. This is to be in conformance
with the following rule present in https://elinux.org/Device_Tree_Linux

. node names
"unit-address does not have leading zeros"

As 'PRIpaddr' introduces leading zeros, we cannot use it.

So, we have introduced a wrapper ie domain_fdt_begin_node() which will
represent physical address using 'PRIx64'.

2. One should use 'PRIx64' to display 'u64' in hex format. The current
use of 'PRIpaddr' for printing PTE is buggy as this is not a physical
address.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---

Changes from -

v1 - 1. Moved the patch earlier.
2. Moved a part of change from "[XEN v1 8/9] xen/arm: Other adaptations required to support 32bit paddr"
into this patch.

v2 - 1. Use PRIx64 for appending addresses to fdt node names. This fixes the CI failure.

 xen/arch/arm/domain_build.c | 45 +++++++++++++++++--------------------
 xen/arch/arm/gic-v2.c       |  6 ++---
 xen/arch/arm/mm.c           |  2 +-
 3 files changed, 25 insertions(+), 28 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index f35f4d2456..97c2395f9a 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1288,6 +1288,20 @@ static int __init fdt_property_interrupts(const struct kernel_info *kinfo,
     return res;
 }
 
+static int __init domain_fdt_begin_node(void *fdt, const char *name,
+                                        uint64_t unit)
+{
+    /*
+     * The size of the buffer to hold the longest possible string ie
+     * interrupt-controller@ + a 64-bit number + \0
+     */
+    char buf[38];
+
+    /* ePAPR 3.4 */
+    snprintf(buf, sizeof(buf), "%s@%"PRIx64, name, unit);
+    return fdt_begin_node(fdt, buf);
+}
+
 static int __init make_memory_node(const struct domain *d,
                                    void *fdt,
                                    int addrcells, int sizecells,
@@ -1296,8 +1310,6 @@ static int __init make_memory_node(const struct domain *d,
     unsigned int i;
     int res, reg_size = addrcells + sizecells;
     int nr_cells = 0;
-    /* Placeholder for memory@ + a 64-bit number + \0 */
-    char buf[24];
     __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];
     __be32 *cells;
 
@@ -1314,9 +1326,7 @@ static int __init make_memory_node(const struct domain *d,
 
     dt_dprintk("Create memory node\n");
 
-    /* ePAPR 3.4 */
-    snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[i].start);
-    res = fdt_begin_node(fdt, buf);
+    res = domain_fdt_begin_node(fdt, "memory", mem->bank[i].start);
     if ( res )
         return res;
 
@@ -1375,16 +1385,13 @@ static int __init make_shm_memory_node(const struct domain *d,
     {
         uint64_t start = mem->bank[i].start;
         uint64_t size = mem->bank[i].size;
-        /* Placeholder for xen-shmem@ + a 64-bit number + \0 */
-        char buf[27];
         const char compat[] = "xen,shared-memory-v1";
         /* Worst case addrcells + sizecells */
         __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
         __be32 *cells;
         unsigned int len = (addrcells + sizecells) * sizeof(__be32);
 
-        snprintf(buf, sizeof(buf), "xen-shmem@%"PRIx64, mem->bank[i].start);
-        res = fdt_begin_node(fdt, buf);
+        res = domain_fdt_begin_node(fdt, "xen-shmem", mem->bank[i].start);
         if ( res )
             return res;
 
@@ -2716,12 +2723,9 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
     __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
     __be32 *cells;
     const struct domain *d = kinfo->d;
-    /* Placeholder for interrupt-controller@ + a 64-bit number + \0 */
-    char buf[38];
 
-    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
-             vgic_dist_base(&d->arch.vgic));
-    res = fdt_begin_node(fdt, buf);
+    res = domain_fdt_begin_node(fdt, "interrupt-controller",
+                                vgic_dist_base(&d->arch.vgic));
     if ( res )
         return res;
 
@@ -2771,14 +2775,10 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
     int res = 0;
     __be32 *reg, *cells;
     const struct domain *d = kinfo->d;
-    /* Placeholder for interrupt-controller@ + a 64-bit number + \0 */
-    char buf[38];
     unsigned int i, len = 0;
 
-    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
-             vgic_dist_base(&d->arch.vgic));
-
-    res = fdt_begin_node(fdt, buf);
+    res = domain_fdt_begin_node(fdt, "interrupt-controller",
+                                vgic_dist_base(&d->arch.vgic));
     if ( res )
         return res;
 
@@ -2858,11 +2858,8 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
     __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
     __be32 *cells;
     struct domain *d = kinfo->d;
-    /* Placeholder for sbsa-uart@ + a 64-bit number + \0 */
-    char buf[27];
 
-    snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d->arch.vpl011.base_addr);
-    res = fdt_begin_node(fdt, buf);
+    res = domain_fdt_begin_node(fdt, "sbsa-uart", d->arch.vpl011.base_addr);
     if ( res )
         return res;
 
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 61802839cb..5d4d298b86 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -1049,7 +1049,7 @@ static void __init gicv2_dt_init(void)
     if ( csize < SZ_8K )
     {
         printk(XENLOG_WARNING "GICv2: WARNING: "
-               "The GICC size is too small: %#"PRIx64" expected %#x\n",
+               "The GICC size is too small: %#"PRIpaddr" expected %#x\n",
                csize, SZ_8K);
         if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
         {
@@ -1280,11 +1280,11 @@ static int __init gicv2_init(void)
         gicv2.map_cbase += aliased_offset;
 
         printk(XENLOG_WARNING
-               "GICv2: Adjusting CPU interface base to %#"PRIx64"\n",
+               "GICv2: Adjusting CPU interface base to %#"PRIpaddr"\n",
                cbase + aliased_offset);
     } else if ( csize == SZ_128K )
         printk(XENLOG_WARNING
-               "GICv2: GICC size=%#"PRIx64" but not aliased\n",
+               "GICv2: GICC size=%#"PRIpaddr" but not aliased\n",
                csize);
 
     gicv2.map_hbase = ioremap_nocache(hbase, PAGE_SIZE);
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 0fc6f2992d..fab54618ab 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -249,7 +249,7 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
 
         pte = mapping[offsets[level]];
 
-        printk("%s[0x%03x] = 0x%"PRIpaddr"\n",
+        printk("%s[0x%03x] = 0x%"PRIx64"\n",
                level_strs[level], offsets[level], pte.bits);
 
         if ( level == 3 || !pte.walk.valid || !pte.walk.table )
-- 
2.17.1

Re: [XEN v3 1/3] xen/arm: Use the correct format specifier

[Stefano Stabellini]

On Mon, 23 Jan 2023, Ayan Kumar Halder wrote:
> 1. One should use 'PRIpaddr' to display 'paddr_t' variables. However,
> while creating nodes in fdt, the address (if present in the node name)
> should be represented using 'PRIx64'. This is to be in conformance
> with the following rule present in https://elinux.org/Device_Tree_Linux
> 
> . node names
> "unit-address does not have leading zeros"
> 
> As 'PRIpaddr' introduces leading zeros, we cannot use it.
> 
> So, we have introduced a wrapper ie domain_fdt_begin_node() which will
> represent physical address using 'PRIx64'.
> 
> 2. One should use 'PRIx64' to display 'u64' in hex format. The current
> use of 'PRIpaddr' for printing PTE is buggy as this is not a physical
> address.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> 
> Changes from -
> 
> v1 - 1. Moved the patch earlier.
> 2. Moved a part of change from "[XEN v1 8/9] xen/arm: Other adaptations required to support 32bit paddr"
> into this patch.
> 
> v2 - 1. Use PRIx64 for appending addresses to fdt node names. This fixes the CI failure.
> 
>  xen/arch/arm/domain_build.c | 45 +++++++++++++++++--------------------
>  xen/arch/arm/gic-v2.c       |  6 ++---
>  xen/arch/arm/mm.c           |  2 +-

The changes to mm.c and gic-v2.c look OK and I'd ack them already. One
question on the changes to domain_build.c below.


>  3 files changed, 25 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index f35f4d2456..97c2395f9a 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1288,6 +1288,20 @@ static int __init fdt_property_interrupts(const struct kernel_info *kinfo,
>      return res;
>  }
>  
> +static int __init domain_fdt_begin_node(void *fdt, const char *name,
> +                                        uint64_t unit)
> +{
> +    /*
> +     * The size of the buffer to hold the longest possible string ie
> +     * interrupt-controller@ + a 64-bit number + \0
> +     */
> +    char buf[38];
> +
> +    /* ePAPR 3.4 */
> +    snprintf(buf, sizeof(buf), "%s@%"PRIx64, name, unit);
> +    return fdt_begin_node(fdt, buf);
> +}
> +
>  static int __init make_memory_node(const struct domain *d,
>                                     void *fdt,
>                                     int addrcells, int sizecells,
> @@ -1296,8 +1310,6 @@ static int __init make_memory_node(const struct domain *d,
>      unsigned int i;
>      int res, reg_size = addrcells + sizecells;
>      int nr_cells = 0;
> -    /* Placeholder for memory@ + a 64-bit number + \0 */
> -    char buf[24];
>      __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];
>      __be32 *cells;
>  
> @@ -1314,9 +1326,7 @@ static int __init make_memory_node(const struct domain *d,
>  
>      dt_dprintk("Create memory node\n");
>  
> -    /* ePAPR 3.4 */
> -    snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[i].start);
> -    res = fdt_begin_node(fdt, buf);
> +    res = domain_fdt_begin_node(fdt, "memory", mem->bank[i].start);

Basically this "hides" the paddr_t->uint64_t cast because it happens
implicitly when passing mem->bank[i].start as an argument to
domain_fdt_begin_node.

To be honest, I don't know if it is necessary. Also a normal cast would
be fine:

    snprintf(buf, sizeof(buf), "memory@%"PRIx64, (uint64_t)mem->bank[i].start);
    res = fdt_begin_node(fdt, buf);

Julien, what do you prefer?

Re: [XEN v3 1/3] xen/arm: Use the correct format specifier

[Julien Grall]

Hi Stefano,

On 23/01/2023 21:19, Stefano Stabellini wrote:
> On Mon, 23 Jan 2023, Ayan Kumar Halder wrote:
>> 1. One should use 'PRIpaddr' to display 'paddr_t' variables. However,
>> while creating nodes in fdt, the address (if present in the node name)
>> should be represented using 'PRIx64'. This is to be in conformance
>> with the following rule present in https://elinux.org/Device_Tree_Linux
>>
>> . node names
>> "unit-address does not have leading zeros"
>>
>> As 'PRIpaddr' introduces leading zeros, we cannot use it.
>>
>> So, we have introduced a wrapper ie domain_fdt_begin_node() which will
>> represent physical address using 'PRIx64'.
>>
>> 2. One should use 'PRIx64' to display 'u64' in hex format. The current
>> use of 'PRIpaddr' for printing PTE is buggy as this is not a physical
>> address.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>>
>> Changes from -
>>
>> v1 - 1. Moved the patch earlier.
>> 2. Moved a part of change from "[XEN v1 8/9] xen/arm: Other adaptations required to support 32bit paddr"
>> into this patch.
>>
>> v2 - 1. Use PRIx64 for appending addresses to fdt node names. This fixes the CI failure.
>>
>>   xen/arch/arm/domain_build.c | 45 +++++++++++++++++--------------------
>>   xen/arch/arm/gic-v2.c       |  6 ++---
>>   xen/arch/arm/mm.c           |  2 +-
> 
> The changes to mm.c and gic-v2.c look OK and I'd ack them already. One
> question on the changes to domain_build.c below.
> 
> 
>>   3 files changed, 25 insertions(+), 28 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index f35f4d2456..97c2395f9a 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1288,6 +1288,20 @@ static int __init fdt_property_interrupts(const struct kernel_info *kinfo,
>>       return res;
>>   }
>>   
>> +static int __init domain_fdt_begin_node(void *fdt, const char *name,
>> +                                        uint64_t unit)
>> +{
>> +    /*
>> +     * The size of the buffer to hold the longest possible string ie
>> +     * interrupt-controller@ + a 64-bit number + \0
>> +     */
>> +    char buf[38];
>> +
>> +    /* ePAPR 3.4 */
>> +    snprintf(buf, sizeof(buf), "%s@%"PRIx64, name, unit);

The return wants to be checked.

>> +    return fdt_begin_node(fdt, buf);
>> +}
>> +
>>   static int __init make_memory_node(const struct domain *d,
>>                                      void *fdt,
>>                                      int addrcells, int sizecells,
>> @@ -1296,8 +1310,6 @@ static int __init make_memory_node(const struct domain *d,
>>       unsigned int i;
>>       int res, reg_size = addrcells + sizecells;
>>       int nr_cells = 0;
>> -    /* Placeholder for memory@ + a 64-bit number + \0 */
>> -    char buf[24];
>>       __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];
>>       __be32 *cells;
>>   
>> @@ -1314,9 +1326,7 @@ static int __init make_memory_node(const struct domain *d,
>>   
>>       dt_dprintk("Create memory node\n");
>>   
>> -    /* ePAPR 3.4 */
>> -    snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[i].start);
>> -    res = fdt_begin_node(fdt, buf);
>> +    res = domain_fdt_begin_node(fdt, "memory", mem->bank[i].start);
> 
> Basically this "hides" the paddr_t->uint64_t cast because it happens
> implicitly when passing mem->bank[i].start as an argument to
> domain_fdt_begin_node.
> 
> To be honest, I don't know if it is necessary. Also a normal cast would
> be fine:
> 
>      snprintf(buf, sizeof(buf), "memory@%"PRIx64, (uint64_t)mem->bank[i].start);
>      res = fdt_begin_node(fdt, buf);
The problem with the open-coding version is you would need to explain 
the cast everywhere (I disliked unexplained one).

I don't particular mind 'hidden cast' but I think we need to explain on 
top of domain_fdt_begin_node() why it is necessary.

> 
> Julien, what do you prefer?

Definitely the function because that's what I suggested (see the 
rationale above).

Cheers,

-- 
Julien Grall

Re: [XEN v3 1/3] xen/arm: Use the correct format specifier

[Stefano Stabellini]

On Mon, 23 Jan 2023, Julien Grall wrote:
> Hi Stefano,
> 
> On 23/01/2023 21:19, Stefano Stabellini wrote:
> > On Mon, 23 Jan 2023, Ayan Kumar Halder wrote:
> > > 1. One should use 'PRIpaddr' to display 'paddr_t' variables. However,
> > > while creating nodes in fdt, the address (if present in the node name)
> > > should be represented using 'PRIx64'. This is to be in conformance
> > > with the following rule present in https://elinux.org/Device_Tree_Linux
> > > 
> > > . node names
> > > "unit-address does not have leading zeros"
> > > 
> > > As 'PRIpaddr' introduces leading zeros, we cannot use it.
> > > 
> > > So, we have introduced a wrapper ie domain_fdt_begin_node() which will
> > > represent physical address using 'PRIx64'.
> > > 
> > > 2. One should use 'PRIx64' to display 'u64' in hex format. The current
> > > use of 'PRIpaddr' for printing PTE is buggy as this is not a physical
> > > address.
> > > 
> > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> > > ---
> > > 
> > > Changes from -
> > > 
> > > v1 - 1. Moved the patch earlier.
> > > 2. Moved a part of change from "[XEN v1 8/9] xen/arm: Other adaptations
> > > required to support 32bit paddr"
> > > into this patch.
> > > 
> > > v2 - 1. Use PRIx64 for appending addresses to fdt node names. This fixes
> > > the CI failure.
> > > 
> > >   xen/arch/arm/domain_build.c | 45 +++++++++++++++++--------------------
> > >   xen/arch/arm/gic-v2.c       |  6 ++---
> > >   xen/arch/arm/mm.c           |  2 +-
> > 
> > The changes to mm.c and gic-v2.c look OK and I'd ack them already. One
> > question on the changes to domain_build.c below.
> > 
> > 
> > >   3 files changed, 25 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index f35f4d2456..97c2395f9a 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -1288,6 +1288,20 @@ static int __init fdt_property_interrupts(const
> > > struct kernel_info *kinfo,
> > >       return res;
> > >   }
> > >   +static int __init domain_fdt_begin_node(void *fdt, const char *name,
> > > +                                        uint64_t unit)
> > > +{
> > > +    /*
> > > +     * The size of the buffer to hold the longest possible string ie
> > > +     * interrupt-controller@ + a 64-bit number + \0
> > > +     */
> > > +    char buf[38];
> > > +
> > > +    /* ePAPR 3.4 */
> > > +    snprintf(buf, sizeof(buf), "%s@%"PRIx64, name, unit);
> 
> The return wants to be checked.
> 
> > > +    return fdt_begin_node(fdt, buf);
> > > +}
> > > +
> > >   static int __init make_memory_node(const struct domain *d,
> > >                                      void *fdt,
> > >                                      int addrcells, int sizecells,
> > > @@ -1296,8 +1310,6 @@ static int __init make_memory_node(const struct
> > > domain *d,
> > >       unsigned int i;
> > >       int res, reg_size = addrcells + sizecells;
> > >       int nr_cells = 0;
> > > -    /* Placeholder for memory@ + a 64-bit number + \0 */
> > > -    char buf[24];
> > >       __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];
> > >       __be32 *cells;
> > >   @@ -1314,9 +1326,7 @@ static int __init make_memory_node(const struct
> > > domain *d,
> > >         dt_dprintk("Create memory node\n");
> > >   -    /* ePAPR 3.4 */
> > > -    snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[i].start);
> > > -    res = fdt_begin_node(fdt, buf);
> > > +    res = domain_fdt_begin_node(fdt, "memory", mem->bank[i].start);
> > 
> > Basically this "hides" the paddr_t->uint64_t cast because it happens
> > implicitly when passing mem->bank[i].start as an argument to
> > domain_fdt_begin_node.
> > 
> > To be honest, I don't know if it is necessary. Also a normal cast would
> > be fine:
> > 
> >      snprintf(buf, sizeof(buf), "memory@%"PRIx64,
> > (uint64_t)mem->bank[i].start);
> >      res = fdt_begin_node(fdt, buf);
> The problem with the open-coding version is you would need to explain the cast
> everywhere (I disliked unexplained one).
> 
> I don't particular mind 'hidden cast' but I think we need to explain on top of
> domain_fdt_begin_node() why it is necessary.
> 
> > 
> > Julien, what do you prefer?
> 
> Definitely the function because that's what I suggested (see the rationale
> above).

OK, no worries

[XEN v3 2/3] xen/drivers: ns16550: Fix the use of simple_strtoul() for extracting u64

[Ayan Kumar Halder]

One should be using simple_strtoull() ( instead of simple_strtoul() )
to assign value to 'u64' variable. The reason being u64 can be
represented by 'unsigned long long' on all the platforms (ie Arm32,
Arm64 and x86).

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---

Changes from -

v1,v2 - NA (This patch is introduced in v3).

 xen/drivers/char/ns16550.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 58d0ccd889..43e1f971ab 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1532,7 +1532,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
         else
 #endif
         {
-            uart->io_base = simple_strtoul(conf, &conf, 0);
+            uart->io_base = simple_strtoull(conf, &conf, 0);
         }
     }
 
@@ -1603,7 +1603,7 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
                        "Can't use io_base with dev=pci or dev=amt options\n");
                 break;
             }
-            uart->io_base = simple_strtoul(param_value, NULL, 0);
+            uart->io_base = simple_strtoull(param_value, NULL, 0);
             break;
 
         case irq:
-- 
2.17.1

Re: [XEN v3 2/3] xen/drivers: ns16550: Fix the use of simple_strtoul() for extracting u64

[Jan Beulich]

On 23.01.2023 14:44, Ayan Kumar Halder wrote:
> One should be using simple_strtoull() ( instead of simple_strtoul() )
> to assign value to 'u64' variable. The reason being u64 can be
> represented by 'unsigned long long' on all the platforms (ie Arm32,
> Arm64 and x86).

Suggested-by: Jan Beulich <jbeulich@suse.com>
(or Reported-by or Requested-by, to your liking)

> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

[XEN v3 3/3] xen/drivers: ns16550: Fix an incorrect assignment to uart->io_size

[Ayan Kumar Halder]

uart->io_size represents the size in bytes. Thus, when serial_port.bit_width
is assigned to it, it should be converted to size in bytes.

Fixes: 17b516196c55 ("ns16550: add ACPI support for ARM only")
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---

Changes from -

v1, v2 - NA (New patch introduced in v3).

 xen/drivers/char/ns16550.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 43e1f971ab..092f6b9c4b 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1870,7 +1870,7 @@ static int __init ns16550_acpi_uart_init(const void *data)
     uart->parity = spcr->parity;
     uart->stop_bits = spcr->stop_bits;
     uart->io_base = spcr->serial_port.address;
-    uart->io_size = spcr->serial_port.bit_width;
+    uart->io_size = DIV_ROUND_UP(spcr->serial_port.bit_width, BITS_PER_BYTE);
     uart->reg_shift = spcr->serial_port.bit_offset;
     uart->reg_width = spcr->serial_port.access_width;
 
-- 
2.17.1

Re: [XEN v3 3/3] xen/drivers: ns16550: Fix an incorrect assignment to uart->io_size

[Stefano Stabellini]

On Mon, 23 Jan 2023, Ayan Kumar Halder wrote:
> uart->io_size represents the size in bytes. Thus, when serial_port.bit_width
> is assigned to it, it should be converted to size in bytes.
> 
> Fixes: 17b516196c55 ("ns16550: add ACPI support for ARM only")
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>

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


> ---
> 
> Changes from -
> 
> v1, v2 - NA (New patch introduced in v3).
> 
>  xen/drivers/char/ns16550.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 43e1f971ab..092f6b9c4b 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1870,7 +1870,7 @@ static int __init ns16550_acpi_uart_init(const void *data)
>      uart->parity = spcr->parity;
>      uart->stop_bits = spcr->stop_bits;
>      uart->io_base = spcr->serial_port.address;
> -    uart->io_size = spcr->serial_port.bit_width;
> +    uart->io_size = DIV_ROUND_UP(spcr->serial_port.bit_width, BITS_PER_BYTE);
>      uart->reg_shift = spcr->serial_port.bit_offset;
>      uart->reg_width = spcr->serial_port.access_width;
>  
> -- 
> 2.17.1
> 
>