[v2,6/7] xen/ppc: mm-radix: Replace debug printing code with printk

Commit Message

Shawn Anastasio Dec. 15, 2023, 2:44 a.m. UTC

Now that we have common code building, there's no need to keep the old
itoa64+debug print function in mm-radix.c

Signed-off-by: Shawn Anastasio <[e-mail]>
---
 xen/arch/ppc/mm-radix.c | 58 +++++++++--------------------------------
 1 file changed, 12 insertions(+), 46 deletions(-)

Comments

Jan Beulich Dec. 20, 2023, 11:48 a.m. UTC | #1

On 15.12.2023 03:44, Shawn Anastasio wrote:
> Now that we have common code building, there's no need to keep the old
> itoa64+debug print function in mm-radix.c
> 
> Signed-off-by: Shawn Anastasio <[e-mail]>

Looks okay, just one question:

> --- a/xen/arch/ppc/mm-radix.c
> +++ b/xen/arch/ppc/mm-radix.c
> @@ -15,6 +15,12 @@
>  
>  void enable_mmu(void);
>  
> +#ifdef NDEBUG
> +#define radix_dprintk(...)
> +#else
> +#define radix_dprintk(msg, ...) printk(XENLOG_DEBUG msg, ## __VA_ARGS__)
> +#endif

Do you really mean NDEBUG here, and not CONFIG_DEBUG? NDEBUG is generally
supposed to only control ASSERT() behavior.

Jan

Shawn Anastasio Jan. 18, 2024, 1:37 a.m. UTC | #2

Hi Jan,

On 12/20/23 5:48 AM, Jan Beulich wrote:
> On 15.12.2023 03:44, Shawn Anastasio wrote:
>> Now that we have common code building, there's no need to keep the old
>> itoa64+debug print function in mm-radix.c
>>
>> Signed-off-by: Shawn Anastasio <[e-mail]>
> 
> Looks okay, just one question:
> 
>> --- a/xen/arch/ppc/mm-radix.c
>> +++ b/xen/arch/ppc/mm-radix.c
>> @@ -15,6 +15,12 @@
>>  
>>  void enable_mmu(void);
>>  
>> +#ifdef NDEBUG
>> +#define radix_dprintk(...)
>> +#else
>> +#define radix_dprintk(msg, ...) printk(XENLOG_DEBUG msg, ## __VA_ARGS__)
>> +#endif
> 
> Do you really mean NDEBUG here, and not CONFIG_DEBUG? NDEBUG is generally
> supposed to only control ASSERT() behavior.
>

Thanks for pointing this out. CONFIG_DEBUG does indeed seem to be a
better fit here. Will change in v3.

> Jan

Thanks,
Shawn

Patch

diff --git a/xen/arch/ppc/mm-radix.c b/xen/arch/ppc/mm-radix.c
index daa411a6fa..de181cf6f1 100644
--- a/xen/arch/ppc/mm-radix.c
+++ b/xen/arch/ppc/mm-radix.c
@@ -15,6 +15,12 @@ 
 
 void enable_mmu(void);
 
+#ifdef NDEBUG
+#define radix_dprintk(...)
+#else
+#define radix_dprintk(msg, ...) printk(XENLOG_DEBUG msg, ## __VA_ARGS__)
+#endif
+
 #define INITIAL_LVL1_PD_COUNT      1
 #define INITIAL_LVL2_LVL3_PD_COUNT 2
 #define INITIAL_LVL4_PT_COUNT      256
@@ -80,45 +86,6 @@  static __init struct lvl4_pt *lvl4_pt_pool_alloc(void)
     return &initial_lvl4_pt_pool[initial_lvl4_pt_pool_used++];
 }
 
-#ifndef NDEBUG
-/* TODO: Remove once we get common/ building */
-static char *__init itoa64_hex(uint64_t val, char *out_buf, size_t buf_len)
-{
-    uint64_t cur;
-    size_t i = buf_len - 1;
-
-    /* Null terminate buffer */
-    out_buf[i] = '\0';
-
-    /* Add digits in reverse */
-    cur = val;
-    while ( cur && i > 0 )
-    {
-        out_buf[--i] = "0123456789ABCDEF"[cur % 16];
-        cur /= 16;
-    }
-
-    /* Pad to 16 digits */
-    while ( i > 0 )
-        out_buf[--i] = '0';
-
-    return out_buf + i;
-}
-#endif
-
-static void __init radix_dprint(uint64_t addr, const char *msg)
-{
-#ifndef NDEBUG
-    char buf[sizeof("DEADBEEFCAFEBABA")];
-    char *addr_s = itoa64_hex(addr, buf, sizeof(buf));
-
-    early_printk("(0x");
-    early_printk(addr_s);
-    early_printk(") ");
-    early_printk(msg);
-#endif
-}
-
 static void __init setup_initial_mapping(struct lvl1_pd *lvl1,
                                          vaddr_t map_start,
                                          vaddr_t map_end,
@@ -186,27 +153,26 @@  static void __init setup_initial_mapping(struct lvl1_pd *lvl1,
             unsigned long paddr = (page_addr - map_start) + phys_base;
             unsigned long flags;
 
-            radix_dprint(paddr, "being mapped to ");
-            radix_dprint(page_addr, "!\n");
+            radix_dprintk("%016lx being mapped to %016lx\n", paddr, page_addr);
             if ( is_kernel_text(page_addr) || is_kernel_inittext(page_addr) )
             {
-                radix_dprint(page_addr, "being marked as TEXT (RX)\n");
+                radix_dprintk("%016lx being marked as TEXT (RX)\n", page_addr);
                 flags = PTE_XEN_RX;
             }
             else if ( is_kernel_rodata(page_addr) )
             {
-                radix_dprint(page_addr, "being marked as RODATA (RO)\n");
+                radix_dprintk("%016lx being marked as RODATA (RO)\n", page_addr);
                 flags = PTE_XEN_RO;
             }
             else
             {
-                radix_dprint(page_addr, "being marked as DEFAULT (RW)\n");
+                radix_dprintk("%016lx being marked as DEFAULT (RW)\n", page_addr);
                 flags = PTE_XEN_RW;
             }
 
             *pte = paddr_to_pte(paddr, flags);
-            radix_dprint(paddr_to_pte(paddr, flags).pte,
-                             "is result of PTE map!\n");
+            radix_dprintk("%016lx is the result of PTE map\n",
+                paddr_to_pte(paddr, flags).pte);
         }
         else
         {