Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Xen-devel] [PATCH 0/4] x86/ucode: Cleanup - Part 2/n
@ 2020-03-20 21:24 Andrew Cooper
  2020-03-20 21:24 ` [Xen-devel] [PATCH 1/4] x86/ucode/amd: Fix assertion in compare_patch() Andrew Cooper
                   ` (4 more replies)
  0 siblings, 5 replies; 18+ messages in thread
From: Andrew Cooper @ 2020-03-20 21:24 UTC (permalink / raw)
  To: Xen-devel
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
	Jan Beulich, Volodymyr Babchuk, Roger Pau Monné

Two minor bugfixes, and two minor cleanups with minor benefits to other code
as well.

There is no dependency on the remaining pieces of the Part 1 series.

Andrew Cooper (4):
  x86/ucode/amd: Fix assertion in compare_patch()
  x86/ucode: Fix error paths in apply_microcode()
  xen: Drop raw_smp_processor_id()
  xen: Introduce a xmemdup_bytes() helper

 xen/arch/x86/cpu/microcode/amd.c   | 27 +++++++++++----------------
 xen/arch/x86/cpu/microcode/intel.c | 29 +++++++++++++----------------
 xen/include/asm-arm/smp.h          |  2 +-
 xen/include/asm-x86/smp.h          |  2 +-
 xen/include/xen/smp.h              |  2 --
 xen/include/xen/xmalloc.h          | 11 +++++++++++
 6 files changed, 37 insertions(+), 36 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Xen-devel] [PATCH 1/4] x86/ucode/amd: Fix assertion in compare_patch()
  2020-03-20 21:24 [Xen-devel] [PATCH 0/4] x86/ucode: Cleanup - Part 2/n Andrew Cooper
@ 2020-03-20 21:24 ` Andrew Cooper
  2020-03-21 16:45   ` Wei Liu
  2020-03-20 21:24 ` [Xen-devel] [PATCH 2/4] x86/ucode: Fix error paths in apply_microcode() Andrew Cooper
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2020-03-20 21:24 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné

This is clearly a typo.

Fixes: 9da23943ccd "microcode: introduce a global cache of ucode patch"
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/amd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 99e2449eee..d4b2874de6 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -222,7 +222,7 @@ static enum microcode_match_result compare_patch(
 
     /* Both patches to compare are supposed to be applicable to local CPU. */
     ASSERT(microcode_fits(new->mc_amd) != MIS_UCODE);
-    ASSERT(microcode_fits(new->mc_amd) != MIS_UCODE);
+    ASSERT(microcode_fits(old->mc_amd) != MIS_UCODE);
 
     return compare_header(new_header, old_header);
 }
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Xen-devel] [PATCH 2/4] x86/ucode: Fix error paths in apply_microcode()
  2020-03-20 21:24 [Xen-devel] [PATCH 0/4] x86/ucode: Cleanup - Part 2/n Andrew Cooper
  2020-03-20 21:24 ` [Xen-devel] [PATCH 1/4] x86/ucode/amd: Fix assertion in compare_patch() Andrew Cooper
@ 2020-03-20 21:24 ` Andrew Cooper
  2020-03-21 16:49   ` Wei Liu
  2020-03-23  8:32   ` Jan Beulich
  2020-03-20 21:24 ` [Xen-devel] [PATCH 3/4] xen: Drop raw_smp_processor_id() Andrew Cooper
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 18+ messages in thread
From: Andrew Cooper @ 2020-03-20 21:24 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné

In the unlikley case that patch application completes, but the resutling
revision isn't expected, sig->rev doesn't get updated to match reality.

It will get adjusted the next time collect_cpu_info() gets called, but in the
meantime Xen might operate on a state value.  Nothing good will come of this.

Rewrite the logic to always update the stashed revision, before worring about
whether the attempt was a success or failure.

Take the opportunity to make the printk() messages as consistent as possible.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
-CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/amd.c   | 14 +++++++-------
 xen/arch/x86/cpu/microcode/intel.c | 22 +++++++++++-----------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index d4b2874de6..a053e43923 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -229,11 +229,11 @@ static enum microcode_match_result compare_patch(
 
 static int apply_microcode(const struct microcode_patch *patch)
 {
-    uint32_t rev;
     int hw_err;
     unsigned int cpu = smp_processor_id();
     struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
     const struct microcode_header_amd *hdr;
+    uint32_t rev, old_rev = sig->rev;
 
     if ( !patch )
         return -ENOENT;
@@ -249,6 +249,7 @@ static int apply_microcode(const struct microcode_patch *patch)
 
     /* get patch id after patching */
     rdmsrl(MSR_AMD_PATCHLEVEL, rev);
+    sig->rev = rev;
 
     /*
      * Some processors leave the ucode blob mapping as UC after the update.
@@ -259,15 +260,14 @@ static int apply_microcode(const struct microcode_patch *patch)
     /* check current patch id and patch's id for match */
     if ( hw_err || (rev != hdr->patch_id) )
     {
-        printk(KERN_ERR "microcode: CPU%d update from revision "
-               "%#x to %#x failed\n", cpu, rev, hdr->patch_id);
+        printk(XENLOG_ERR
+               "microcode: CPU%u update rev %#x to %#x failed, result %#x\n",
+               cpu, old_rev, hdr->patch_id, rev);
         return -EIO;
     }
 
-    printk(KERN_WARNING "microcode: CPU%d updated from revision %#x to %#x\n",
-           cpu, sig->rev, hdr->patch_id);
-
-    sig->rev = rev;
+    printk(XENLOG_WARNING "microcode: CPU%u updated from revision %#x to %#x\n",
+           cpu, old_rev, hdr->patch_id);
 
     return 0;
 }
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 5e9c2a9c7f..6ac5f98694 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -278,10 +278,10 @@ static enum microcode_match_result compare_patch(
 static int apply_microcode(const struct microcode_patch *patch)
 {
     uint64_t msr_content;
-    unsigned int val[2];
-    unsigned int cpu_num = raw_smp_processor_id();
+    unsigned int cpu = smp_processor_id();
     struct cpu_signature *sig = &this_cpu(cpu_sig);
     const struct microcode_intel *mc_intel;
+    uint32_t rev, old_rev = sig->rev;
 
     if ( !patch )
         return -ENOENT;
@@ -302,20 +302,20 @@ static int apply_microcode(const struct microcode_patch *patch)
 
     /* get the current revision from MSR 0x8B */
     rdmsrl(MSR_IA32_UCODE_REV, msr_content);
-    val[1] = (uint32_t)(msr_content >> 32);
+    sig->rev = rev = msr_content >> 32;
 
-    if ( val[1] != mc_intel->hdr.rev )
+    if ( rev != mc_intel->hdr.rev )
     {
-        printk(KERN_ERR "microcode: CPU%d update from revision "
-               "%#x to %#x failed. Resulting revision is %#x.\n", cpu_num,
-               sig->rev, mc_intel->hdr.rev, val[1]);
+        printk(XENLOG_ERR
+               "microcode: CPU%u update rev %#x to %#x failed, result %#x\n",
+               cpu, old_rev, mc_intel->hdr.rev, rev);
         return -EIO;
     }
-    printk(KERN_INFO "microcode: CPU%d updated from revision "
-           "%#x to %#x, date = %04x-%02x-%02x\n",
-           cpu_num, sig->rev, val[1], mc_intel->hdr.year,
+
+    printk(XENLOG_WARNING
+           "microcode: CPU%u updated from revision %#x to %#x, date = %04x-%02x-%02x\n",
+           cpu, old_rev, rev, mc_intel->hdr.year,
            mc_intel->hdr.month, mc_intel->hdr.day);
-    sig->rev = val[1];
 
     return 0;
 }
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Xen-devel] [PATCH 3/4] xen: Drop raw_smp_processor_id()
  2020-03-20 21:24 [Xen-devel] [PATCH 0/4] x86/ucode: Cleanup - Part 2/n Andrew Cooper
  2020-03-20 21:24 ` [Xen-devel] [PATCH 1/4] x86/ucode/amd: Fix assertion in compare_patch() Andrew Cooper
  2020-03-20 21:24 ` [Xen-devel] [PATCH 2/4] x86/ucode: Fix error paths in apply_microcode() Andrew Cooper
@ 2020-03-20 21:24 ` Andrew Cooper
  2020-03-21 10:14   ` Julien Grall
  2020-03-21 16:50   ` Wei Liu
  2020-03-20 21:24 ` [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper Andrew Cooper
  2020-03-23  8:41 ` [Xen-devel] [PATCH 0/4] x86/ucode: Cleanup - Part 2/n Jan Beulich
  4 siblings, 2 replies; 18+ messages in thread
From: Andrew Cooper @ 2020-03-20 21:24 UTC (permalink / raw)
  To: Xen-devel
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
	Jan Beulich, Volodymyr Babchuk, Roger Pau Monné

There is only a single user of raw_smp_processor_id() left in the tree (and it
is unconditionally compiled out).  Drop the alias from all architectures.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
---
 xen/arch/x86/cpu/microcode/amd.c | 2 +-
 xen/include/asm-arm/smp.h        | 2 +-
 xen/include/asm-x86/smp.h        | 2 +-
 xen/include/xen/smp.h            | 2 --
 4 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index a053e43923..0998a36b5c 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -306,7 +306,7 @@ static int get_ucode_from_buffer_amd(
     memcpy(mc_amd->mpb, mpbuf->data, mpbuf->len);
 
     pr_debug("microcode: CPU%d size %zu, block size %u offset %zu equivID %#x rev %#x\n",
-             raw_smp_processor_id(), bufsize, mpbuf->len, *offset,
+             smp_processor_id(), bufsize, mpbuf->len, *offset,
              ((struct microcode_header_amd *)mc_amd->mpb)->processor_rev_id,
              ((struct microcode_header_amd *)mc_amd->mpb)->patch_id);
 
diff --git a/xen/include/asm-arm/smp.h b/xen/include/asm-arm/smp.h
index fdbcefa241..af5a2fe652 100644
--- a/xen/include/asm-arm/smp.h
+++ b/xen/include/asm-arm/smp.h
@@ -12,7 +12,7 @@ DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask);
 
 #define cpu_is_offline(cpu) unlikely(!cpu_online(cpu))
 
-#define raw_smp_processor_id() (get_processor_id())
+#define smp_processor_id() get_processor_id()
 
 /*
  * Do we, for platform reasons, need to actually keep CPUs online when we
diff --git a/xen/include/asm-x86/smp.h b/xen/include/asm-x86/smp.h
index 6150363655..f7485f602e 100644
--- a/xen/include/asm-x86/smp.h
+++ b/xen/include/asm-x86/smp.h
@@ -53,7 +53,7 @@ int cpu_add(uint32_t apic_id, uint32_t acpi_id, uint32_t pxm);
  * from the initial startup. We map APIC_BASE very early in page_setup(),
  * so this is correct in the x86 case.
  */
-#define raw_smp_processor_id() (get_processor_id())
+#define smp_processor_id() get_processor_id()
 
 void __stop_this_cpu(void);
 
diff --git a/xen/include/xen/smp.h b/xen/include/xen/smp.h
index a64c9b3882..d5a3644611 100644
--- a/xen/include/xen/smp.h
+++ b/xen/include/xen/smp.h
@@ -65,8 +65,6 @@ void smp_call_function_interrupt(void);
 
 void smp_send_call_function_mask(const cpumask_t *mask);
 
-#define smp_processor_id() raw_smp_processor_id()
-
 int alloc_cpu_id(void);
 
 extern void *stack_base[NR_CPUS];
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper
  2020-03-20 21:24 [Xen-devel] [PATCH 0/4] x86/ucode: Cleanup - Part 2/n Andrew Cooper
                   ` (2 preceding siblings ...)
  2020-03-20 21:24 ` [Xen-devel] [PATCH 3/4] xen: Drop raw_smp_processor_id() Andrew Cooper
@ 2020-03-20 21:24 ` Andrew Cooper
  2020-03-21 16:51   ` Wei Liu
  2020-03-21 22:19   ` Julien Grall
  2020-03-23  8:41 ` [Xen-devel] [PATCH 0/4] x86/ucode: Cleanup - Part 2/n Jan Beulich
  4 siblings, 2 replies; 18+ messages in thread
From: Andrew Cooper @ 2020-03-20 21:24 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné

Use it to simplify the x86 microcode logic, taking the opportunity to drop the
-ENOMEM printks.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/amd.c   |  9 ++-------
 xen/arch/x86/cpu/microcode/intel.c |  7 ++-----
 xen/include/xen/xmalloc.h          | 11 +++++++++++
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 0998a36b5c..12a3b6b32c 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -299,11 +299,10 @@ static int get_ucode_from_buffer_amd(
         return -EINVAL;
     }
 
-    mc_amd->mpb = xmalloc_bytes(mpbuf->len);
+    mc_amd->mpb = xmemdup_bytes(mpbuf->data, mpbuf->len);
     if ( !mc_amd->mpb )
         return -ENOMEM;
     mc_amd->mpb_size = mpbuf->len;
-    memcpy(mc_amd->mpb, mpbuf->data, mpbuf->len);
 
     pr_debug("microcode: CPU%d size %zu, block size %u offset %zu equivID %#x rev %#x\n",
              smp_processor_id(), bufsize, mpbuf->len, *offset,
@@ -336,14 +335,10 @@ static int install_equiv_cpu_table(
         return -EINVAL;
     }
 
-    mc_amd->equiv_cpu_table = xmalloc_bytes(mpbuf->len);
+    mc_amd->equiv_cpu_table = xmemdup_bytes(mpbuf->data, mpbuf->len);
     if ( !mc_amd->equiv_cpu_table )
-    {
-        printk(KERN_ERR "microcode: Cannot allocate memory for equivalent cpu table\n");
         return -ENOMEM;
-    }
 
-    memcpy(mc_amd->equiv_cpu_table, mpbuf->data, mpbuf->len);
     mc_amd->equiv_cpu_table_size = mpbuf->len;
 
     return 0;
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 6ac5f98694..f26511da98 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -339,13 +339,10 @@ static long get_next_ucode_from_buffer(struct microcode_intel **mc,
         return -EINVAL;
     }
 
-    *mc = xmalloc_bytes(total_size);
+    *mc = xmemdup_bytes(mc_header, total_size);
     if ( *mc == NULL )
-    {
-        printk(KERN_ERR "microcode: error! Can not allocate memory\n");
         return -ENOMEM;
-    }
-    memcpy(*mc, (const void *)(buf + offset), total_size);
+
     return offset + total_size;
 }
 
diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h
index f515ceee2a..16979a117c 100644
--- a/xen/include/xen/xmalloc.h
+++ b/xen/include/xen/xmalloc.h
@@ -51,6 +51,17 @@
 #define xmalloc_bytes(_bytes) _xmalloc(_bytes, SMP_CACHE_BYTES)
 #define xzalloc_bytes(_bytes) _xzalloc(_bytes, SMP_CACHE_BYTES)
 
+/* Allocate untyped storage and copying an existing instance. */
+#define xmemdup_bytes(_src, _nr)                \
+    ({                                          \
+        unsigned long nr_ = (_nr);              \
+        void *dst_ = xmalloc_bytes(nr_);        \
+                                                \
+        if ( dst_ )                             \
+            memcpy(dst_, _src, nr_);            \
+        dst_;                                   \
+    })
+
 /* Free any of the above. */
 extern void xfree(void *);
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Xen-devel] [PATCH 3/4] xen: Drop raw_smp_processor_id()
  2020-03-20 21:24 ` [Xen-devel] [PATCH 3/4] xen: Drop raw_smp_processor_id() Andrew Cooper
@ 2020-03-21 10:14   ` Julien Grall
  2020-03-21 16:50   ` Wei Liu
  1 sibling, 0 replies; 18+ messages in thread
From: Julien Grall @ 2020-03-21 10:14 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: Stefano Stabellini, Volodymyr Babchuk, Wei Liu, Jan Beulich,
	Roger Pau Monné



On 20/03/2020 21:24, Andrew Cooper wrote:
> There is only a single user of raw_smp_processor_id() left in the tree (and it
> is unconditionally compiled out).  Drop the alias from all architectures.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Cheers,

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> ---
>   xen/arch/x86/cpu/microcode/amd.c | 2 +-
>   xen/include/asm-arm/smp.h        | 2 +-
>   xen/include/asm-x86/smp.h        | 2 +-
>   xen/include/xen/smp.h            | 2 --
>   4 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
> index a053e43923..0998a36b5c 100644
> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -306,7 +306,7 @@ static int get_ucode_from_buffer_amd(
>       memcpy(mc_amd->mpb, mpbuf->data, mpbuf->len);
>   
>       pr_debug("microcode: CPU%d size %zu, block size %u offset %zu equivID %#x rev %#x\n",
> -             raw_smp_processor_id(), bufsize, mpbuf->len, *offset,
> +             smp_processor_id(), bufsize, mpbuf->len, *offset,
>                ((struct microcode_header_amd *)mc_amd->mpb)->processor_rev_id,
>                ((struct microcode_header_amd *)mc_amd->mpb)->patch_id);
>   
> diff --git a/xen/include/asm-arm/smp.h b/xen/include/asm-arm/smp.h
> index fdbcefa241..af5a2fe652 100644
> --- a/xen/include/asm-arm/smp.h
> +++ b/xen/include/asm-arm/smp.h
> @@ -12,7 +12,7 @@ DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask);
>   
>   #define cpu_is_offline(cpu) unlikely(!cpu_online(cpu))
>   
> -#define raw_smp_processor_id() (get_processor_id())
> +#define smp_processor_id() get_processor_id()
>   
>   /*
>    * Do we, for platform reasons, need to actually keep CPUs online when we
> diff --git a/xen/include/asm-x86/smp.h b/xen/include/asm-x86/smp.h
> index 6150363655..f7485f602e 100644
> --- a/xen/include/asm-x86/smp.h
> +++ b/xen/include/asm-x86/smp.h
> @@ -53,7 +53,7 @@ int cpu_add(uint32_t apic_id, uint32_t acpi_id, uint32_t pxm);
>    * from the initial startup. We map APIC_BASE very early in page_setup(),
>    * so this is correct in the x86 case.
>    */
> -#define raw_smp_processor_id() (get_processor_id())
> +#define smp_processor_id() get_processor_id()
>   
>   void __stop_this_cpu(void);
>   
> diff --git a/xen/include/xen/smp.h b/xen/include/xen/smp.h
> index a64c9b3882..d5a3644611 100644
> --- a/xen/include/xen/smp.h
> +++ b/xen/include/xen/smp.h
> @@ -65,8 +65,6 @@ void smp_call_function_interrupt(void);
>   
>   void smp_send_call_function_mask(const cpumask_t *mask);
>   
> -#define smp_processor_id() raw_smp_processor_id()
> -
>   int alloc_cpu_id(void);
>   
>   extern void *stack_base[NR_CPUS];
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Xen-devel] [PATCH 1/4] x86/ucode/amd: Fix assertion in compare_patch()
  2020-03-20 21:24 ` [Xen-devel] [PATCH 1/4] x86/ucode/amd: Fix assertion in compare_patch() Andrew Cooper
@ 2020-03-21 16:45   ` Wei Liu
  0 siblings, 0 replies; 18+ messages in thread
From: Wei Liu @ 2020-03-21 16:45 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Jan Beulich, Roger Pau Monné

On Fri, Mar 20, 2020 at 09:24:49PM +0000, Andrew Cooper wrote:
> This is clearly a typo.
> 
> Fixes: 9da23943ccd "microcode: introduce a global cache of ucode patch"
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wl@xen.org>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Xen-devel] [PATCH 2/4] x86/ucode: Fix error paths in apply_microcode()
  2020-03-20 21:24 ` [Xen-devel] [PATCH 2/4] x86/ucode: Fix error paths in apply_microcode() Andrew Cooper
@ 2020-03-21 16:49   ` Wei Liu
  2020-03-23  8:32   ` Jan Beulich
  1 sibling, 0 replies; 18+ messages in thread
From: Wei Liu @ 2020-03-21 16:49 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Jan Beulich, Wei Liu, Roger Pau Monné

Actually CC Jan

On Fri, Mar 20, 2020 at 09:24:50PM +0000, Andrew Cooper wrote:
> In the unlikley case that patch application completes, but the resutling
> revision isn't expected, sig->rev doesn't get updated to match reality.
> 
> It will get adjusted the next time collect_cpu_info() gets called, but in the
> meantime Xen might operate on a state value.  Nothing good will come of this.

state -> stale

> 
> Rewrite the logic to always update the stashed revision, before worring about

worring -> worrying

> whether the attempt was a success or failure.
> 
> Take the opportunity to make the printk() messages as consistent as possible.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wl@xen.org>

> ---
> -CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/cpu/microcode/amd.c   | 14 +++++++-------
>  xen/arch/x86/cpu/microcode/intel.c | 22 +++++++++++-----------
>  2 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
> index d4b2874de6..a053e43923 100644
> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -229,11 +229,11 @@ static enum microcode_match_result compare_patch(
>  
>  static int apply_microcode(const struct microcode_patch *patch)
>  {
> -    uint32_t rev;
>      int hw_err;
>      unsigned int cpu = smp_processor_id();
>      struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
>      const struct microcode_header_amd *hdr;
> +    uint32_t rev, old_rev = sig->rev;
>  
>      if ( !patch )
>          return -ENOENT;
> @@ -249,6 +249,7 @@ static int apply_microcode(const struct microcode_patch *patch)
>  
>      /* get patch id after patching */
>      rdmsrl(MSR_AMD_PATCHLEVEL, rev);
> +    sig->rev = rev;
>  
>      /*
>       * Some processors leave the ucode blob mapping as UC after the update.
> @@ -259,15 +260,14 @@ static int apply_microcode(const struct microcode_patch *patch)
>      /* check current patch id and patch's id for match */
>      if ( hw_err || (rev != hdr->patch_id) )
>      {
> -        printk(KERN_ERR "microcode: CPU%d update from revision "
> -               "%#x to %#x failed\n", cpu, rev, hdr->patch_id);
> +        printk(XENLOG_ERR
> +               "microcode: CPU%u update rev %#x to %#x failed, result %#x\n",
> +               cpu, old_rev, hdr->patch_id, rev);
>          return -EIO;
>      }
>  
> -    printk(KERN_WARNING "microcode: CPU%d updated from revision %#x to %#x\n",
> -           cpu, sig->rev, hdr->patch_id);
> -
> -    sig->rev = rev;
> +    printk(XENLOG_WARNING "microcode: CPU%u updated from revision %#x to %#x\n",
> +           cpu, old_rev, hdr->patch_id);
>  
>      return 0;
>  }
> diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
> index 5e9c2a9c7f..6ac5f98694 100644
> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -278,10 +278,10 @@ static enum microcode_match_result compare_patch(
>  static int apply_microcode(const struct microcode_patch *patch)
>  {
>      uint64_t msr_content;
> -    unsigned int val[2];
> -    unsigned int cpu_num = raw_smp_processor_id();
> +    unsigned int cpu = smp_processor_id();
>      struct cpu_signature *sig = &this_cpu(cpu_sig);
>      const struct microcode_intel *mc_intel;
> +    uint32_t rev, old_rev = sig->rev;
>  
>      if ( !patch )
>          return -ENOENT;
> @@ -302,20 +302,20 @@ static int apply_microcode(const struct microcode_patch *patch)
>  
>      /* get the current revision from MSR 0x8B */
>      rdmsrl(MSR_IA32_UCODE_REV, msr_content);
> -    val[1] = (uint32_t)(msr_content >> 32);
> +    sig->rev = rev = msr_content >> 32;
>  
> -    if ( val[1] != mc_intel->hdr.rev )
> +    if ( rev != mc_intel->hdr.rev )
>      {
> -        printk(KERN_ERR "microcode: CPU%d update from revision "
> -               "%#x to %#x failed. Resulting revision is %#x.\n", cpu_num,
> -               sig->rev, mc_intel->hdr.rev, val[1]);
> +        printk(XENLOG_ERR
> +               "microcode: CPU%u update rev %#x to %#x failed, result %#x\n",
> +               cpu, old_rev, mc_intel->hdr.rev, rev);
>          return -EIO;
>      }
> -    printk(KERN_INFO "microcode: CPU%d updated from revision "
> -           "%#x to %#x, date = %04x-%02x-%02x\n",
> -           cpu_num, sig->rev, val[1], mc_intel->hdr.year,
> +
> +    printk(XENLOG_WARNING
> +           "microcode: CPU%u updated from revision %#x to %#x, date = %04x-%02x-%02x\n",
> +           cpu, old_rev, rev, mc_intel->hdr.year,
>             mc_intel->hdr.month, mc_intel->hdr.day);
> -    sig->rev = val[1];
>  
>      return 0;
>  }
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Xen-devel] [PATCH 3/4] xen: Drop raw_smp_processor_id()
  2020-03-20 21:24 ` [Xen-devel] [PATCH 3/4] xen: Drop raw_smp_processor_id() Andrew Cooper
  2020-03-21 10:14   ` Julien Grall
@ 2020-03-21 16:50   ` Wei Liu
  1 sibling, 0 replies; 18+ messages in thread
From: Wei Liu @ 2020-03-21 16:50 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Jan Beulich,
	Xen-devel, Volodymyr Babchuk, Roger Pau Monné

On Fri, Mar 20, 2020 at 09:24:51PM +0000, Andrew Cooper wrote:
> There is only a single user of raw_smp_processor_id() left in the tree (and it
> is unconditionally compiled out).  Drop the alias from all architectures.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wl@xen.org>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper
  2020-03-20 21:24 ` [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper Andrew Cooper
@ 2020-03-21 16:51   ` Wei Liu
  2020-03-21 22:19   ` Julien Grall
  1 sibling, 0 replies; 18+ messages in thread
From: Wei Liu @ 2020-03-21 16:51 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Jan Beulich, Roger Pau Monné

On Fri, Mar 20, 2020 at 09:24:52PM +0000, Andrew Cooper wrote:
> Use it to simplify the x86 microcode logic, taking the opportunity to drop the
> -ENOMEM printks.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wl@xen.org>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper
  2020-03-20 21:24 ` [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper Andrew Cooper
  2020-03-21 16:51   ` Wei Liu
@ 2020-03-21 22:19   ` Julien Grall
  2020-03-23  8:38     ` Jan Beulich
  2020-03-26 14:53     ` Andrew Cooper
  1 sibling, 2 replies; 18+ messages in thread
From: Julien Grall @ 2020-03-21 22:19 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Jan Beulich, Wei Liu, Roger Pau Monné

Hi Andrew,

On Fri, 20 Mar 2020 at 21:26, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> Use it to simplify the x86 microcode logic, taking the opportunity to drop the
> -ENOMEM printks.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/cpu/microcode/amd.c   |  9 ++-------
>  xen/arch/x86/cpu/microcode/intel.c |  7 ++-----
>  xen/include/xen/xmalloc.h          | 11 +++++++++++

I did notice a few times in the past months where only the x86 folks
where CCed even when there are changes in common code.
Even if I am mostly likely going to be happy with the changes, you
should at least give the other maintainers an opportunity to
object/comment.

May I ask to CC all the relevant maintainers in the future?
scripts/add_maintainers.pl should do the job for you.

>  3 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
> index 0998a36b5c..12a3b6b32c 100644
> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -299,11 +299,10 @@ static int get_ucode_from_buffer_amd(
>          return -EINVAL;
>      }
>
> -    mc_amd->mpb = xmalloc_bytes(mpbuf->len);
> +    mc_amd->mpb = xmemdup_bytes(mpbuf->data, mpbuf->len);
>      if ( !mc_amd->mpb )
>          return -ENOMEM;
>      mc_amd->mpb_size = mpbuf->len;
> -    memcpy(mc_amd->mpb, mpbuf->data, mpbuf->len);
>
>      pr_debug("microcode: CPU%d size %zu, block size %u offset %zu equivID %#x rev %#x\n",
>               smp_processor_id(), bufsize, mpbuf->len, *offset,
> @@ -336,14 +335,10 @@ static int install_equiv_cpu_table(
>          return -EINVAL;
>      }
>
> -    mc_amd->equiv_cpu_table = xmalloc_bytes(mpbuf->len);
> +    mc_amd->equiv_cpu_table = xmemdup_bytes(mpbuf->data, mpbuf->len);
>      if ( !mc_amd->equiv_cpu_table )
> -    {
> -        printk(KERN_ERR "microcode: Cannot allocate memory for equivalent cpu table\n");
>          return -ENOMEM;
> -    }
>
> -    memcpy(mc_amd->equiv_cpu_table, mpbuf->data, mpbuf->len);
>      mc_amd->equiv_cpu_table_size = mpbuf->len;
>
>      return 0;
> diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
> index 6ac5f98694..f26511da98 100644
> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -339,13 +339,10 @@ static long get_next_ucode_from_buffer(struct microcode_intel **mc,
>          return -EINVAL;
>      }
>
> -    *mc = xmalloc_bytes(total_size);
> +    *mc = xmemdup_bytes(mc_header, total_size);
>      if ( *mc == NULL )
> -    {
> -        printk(KERN_ERR "microcode: error! Can not allocate memory\n");
>          return -ENOMEM;
> -    }
> -    memcpy(*mc, (const void *)(buf + offset), total_size);
> +
>      return offset + total_size;
>  }
>
> diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h
> index f515ceee2a..16979a117c 100644
> --- a/xen/include/xen/xmalloc.h
> +++ b/xen/include/xen/xmalloc.h
> @@ -51,6 +51,17 @@
>  #define xmalloc_bytes(_bytes) _xmalloc(_bytes, SMP_CACHE_BYTES)
>  #define xzalloc_bytes(_bytes) _xzalloc(_bytes, SMP_CACHE_BYTES)
>
> +/* Allocate untyped storage and copying an existing instance. */
> +#define xmemdup_bytes(_src, _nr)                \
> +    ({                                          \
> +        unsigned long nr_ = (_nr);              \
> +        void *dst_ = xmalloc_bytes(nr_);        \

The nr_ vs _nr is really confusing to read. Could you re-implement the
function as a static inline?

Cheers,

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Xen-devel] [PATCH 2/4] x86/ucode: Fix error paths in apply_microcode()
  2020-03-20 21:24 ` [Xen-devel] [PATCH 2/4] x86/ucode: Fix error paths in apply_microcode() Andrew Cooper
  2020-03-21 16:49   ` Wei Liu
@ 2020-03-23  8:32   ` Jan Beulich
  1 sibling, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2020-03-23  8:32 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné

On 20.03.2020 22:24, Andrew Cooper wrote:
> @@ -259,15 +260,14 @@ static int apply_microcode(const struct microcode_patch *patch)
>      /* check current patch id and patch's id for match */
>      if ( hw_err || (rev != hdr->patch_id) )
>      {
> -        printk(KERN_ERR "microcode: CPU%d update from revision "
> -               "%#x to %#x failed\n", cpu, rev, hdr->patch_id);
> +        printk(XENLOG_ERR
> +               "microcode: CPU%u update rev %#x to %#x failed, result %#x\n",
> +               cpu, old_rev, hdr->patch_id, rev);
>          return -EIO;
>      }
>  
> -    printk(KERN_WARNING "microcode: CPU%d updated from revision %#x to %#x\n",
> -           cpu, sig->rev, hdr->patch_id);
> -
> -    sig->rev = rev;
> +    printk(XENLOG_WARNING "microcode: CPU%u updated from revision %#x to %#x\n",
> +           cpu, old_rev, hdr->patch_id);

Prefer the local variable here over hdr->patch_id, just like you do
on the Intel side?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper
  2020-03-21 22:19   ` Julien Grall
@ 2020-03-23  8:38     ` Jan Beulich
  2020-03-26 15:38       ` Andrew Cooper
  2020-03-26 14:53     ` Andrew Cooper
  1 sibling, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2020-03-23  8:38 UTC (permalink / raw)
  To: Julien Grall, Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné

On 21.03.2020 23:19, Julien Grall wrote:
> On Fri, 20 Mar 2020 at 21:26, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/include/xen/xmalloc.h
>> +++ b/xen/include/xen/xmalloc.h
>> @@ -51,6 +51,17 @@
>>  #define xmalloc_bytes(_bytes) _xmalloc(_bytes, SMP_CACHE_BYTES)
>>  #define xzalloc_bytes(_bytes) _xzalloc(_bytes, SMP_CACHE_BYTES)
>>
>> +/* Allocate untyped storage and copying an existing instance. */
>> +#define xmemdup_bytes(_src, _nr)                \
>> +    ({                                          \
>> +        unsigned long nr_ = (_nr);              \
>> +        void *dst_ = xmalloc_bytes(nr_);        \
> 
> The nr_ vs _nr is really confusing to read. Could you re-implement the
> function as a static inline?

And even if that wouldn't work out - what's the point of having
macro argument names with leading underscores? This isn't any
better standard-wise (afaict) than other uses of leading
underscores for identifiers which aren't CU-scope.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Xen-devel] [PATCH 0/4] x86/ucode: Cleanup - Part 2/n
  2020-03-20 21:24 [Xen-devel] [PATCH 0/4] x86/ucode: Cleanup - Part 2/n Andrew Cooper
                   ` (3 preceding siblings ...)
  2020-03-20 21:24 ` [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper Andrew Cooper
@ 2020-03-23  8:41 ` Jan Beulich
  4 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2020-03-23  8:41 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Xen-devel,
	Volodymyr Babchuk, Roger Pau Monné

On 20.03.2020 22:24, Andrew Cooper wrote:
> Two minor bugfixes, and two minor cleanups with minor benefits to other code
> as well.
> 
> There is no dependency on the remaining pieces of the Part 1 series.
> 
> Andrew Cooper (4):
>   x86/ucode/amd: Fix assertion in compare_patch()
>   x86/ucode: Fix error paths in apply_microcode()
>   xen: Drop raw_smp_processor_id()

FAOD feel free to throw in with Wei's R-b, ideally with the small
adjustment suggested for patch 2.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper
  2020-03-21 22:19   ` Julien Grall
  2020-03-23  8:38     ` Jan Beulich
@ 2020-03-26 14:53     ` Andrew Cooper
  2020-03-26 19:05       ` Julien Grall
  1 sibling, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2020-03-26 14:53 UTC (permalink / raw)
  To: Julien Grall; +Cc: Xen-devel, Jan Beulich, Wei Liu, Roger Pau Monné

On 21/03/2020 22:19, Julien Grall wrote:
>> diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h
>> index f515ceee2a..16979a117c 100644
>> --- a/xen/include/xen/xmalloc.h
>> +++ b/xen/include/xen/xmalloc.h
>> @@ -51,6 +51,17 @@
>>  #define xmalloc_bytes(_bytes) _xmalloc(_bytes, SMP_CACHE_BYTES)
>>  #define xzalloc_bytes(_bytes) _xzalloc(_bytes, SMP_CACHE_BYTES)
>>
>> +/* Allocate untyped storage and copying an existing instance. */
>> +#define xmemdup_bytes(_src, _nr)                \
>> +    ({                                          \
>> +        unsigned long nr_ = (_nr);              \
>> +        void *dst_ = xmalloc_bytes(nr_);        \
> The nr_ vs _nr is really confusing to read. Could you re-implement the
> function as a static inline?

I'd really prefer to, but sadly not.

That requires untangling headers sufficiently so we can include
string.h, to be able to use memcpy.  I don't have time at the moment to
sort that out.

~Andrew


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper
  2020-03-23  8:38     ` Jan Beulich
@ 2020-03-26 15:38       ` Andrew Cooper
  2020-03-26 15:47         ` Jan Beulich
  0 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2020-03-26 15:38 UTC (permalink / raw)
  To: Jan Beulich, Julien Grall; +Cc: Xen-devel, Wei Liu, Roger Pau Monné

On 23/03/2020 08:38, Jan Beulich wrote:
> On 21.03.2020 23:19, Julien Grall wrote:
>> On Fri, 20 Mar 2020 at 21:26, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/include/xen/xmalloc.h
>>> +++ b/xen/include/xen/xmalloc.h
>>> @@ -51,6 +51,17 @@
>>>  #define xmalloc_bytes(_bytes) _xmalloc(_bytes, SMP_CACHE_BYTES)
>>>  #define xzalloc_bytes(_bytes) _xzalloc(_bytes, SMP_CACHE_BYTES)
>>>
>>> +/* Allocate untyped storage and copying an existing instance. */
>>> +#define xmemdup_bytes(_src, _nr)                \
>>> +    ({                                          \
>>> +        unsigned long nr_ = (_nr);              \
>>> +        void *dst_ = xmalloc_bytes(nr_);        \
>> The nr_ vs _nr is really confusing to read. Could you re-implement the
>> function as a static inline?
> And even if that wouldn't work out - what's the point of having
> macro argument names with leading underscores?

Consistency with all the other code in this file.

>  This isn't any
> better standard-wise (afaict) than other uses of leading
> underscores for identifiers which aren't CU-scope.

It is a parameter describing textural replacement within the body. 
There is 0 interaction with external namespacing standards.

~Andrew


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper
  2020-03-26 15:38       ` Andrew Cooper
@ 2020-03-26 15:47         ` Jan Beulich
  0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2020-03-26 15:47 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Roger Pau Monné, Wei Liu, Julien Grall

On 26.03.2020 16:38, Andrew Cooper wrote:
> On 23/03/2020 08:38, Jan Beulich wrote:
>> On 21.03.2020 23:19, Julien Grall wrote:
>>> On Fri, 20 Mar 2020 at 21:26, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/include/xen/xmalloc.h
>>>> +++ b/xen/include/xen/xmalloc.h
>>>> @@ -51,6 +51,17 @@
>>>>  #define xmalloc_bytes(_bytes) _xmalloc(_bytes, SMP_CACHE_BYTES)
>>>>  #define xzalloc_bytes(_bytes) _xzalloc(_bytes, SMP_CACHE_BYTES)
>>>>
>>>> +/* Allocate untyped storage and copying an existing instance. */
>>>> +#define xmemdup_bytes(_src, _nr)                \
>>>> +    ({                                          \
>>>> +        unsigned long nr_ = (_nr);              \
>>>> +        void *dst_ = xmalloc_bytes(nr_);        \
>>> The nr_ vs _nr is really confusing to read. Could you re-implement the
>>> function as a static inline?
>> And even if that wouldn't work out - what's the point of having
>> macro argument names with leading underscores?
> 
> Consistency with all the other code in this file.

I value consistency quite high, but then please consistent with
something that properly follows other rules.

>>  This isn't any
>> better standard-wise (afaict) than other uses of leading
>> underscores for identifiers which aren't CU-scope.
> 
> It is a parameter describing textural replacement within the body. 
> There is 0 interaction with external namespacing standards.

Please forgive me saying so, but a typical reply I might get back
from you would be: And?

I'm not going to insist nor nak the patch, but I don't welcome
widening existing issues we have.

Jan


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper
  2020-03-26 14:53     ` Andrew Cooper
@ 2020-03-26 19:05       ` Julien Grall
  0 siblings, 0 replies; 18+ messages in thread
From: Julien Grall @ 2020-03-26 19:05 UTC (permalink / raw)
  To: Andrew Cooper, Julien Grall
  Cc: Xen-devel, Jan Beulich, Wei Liu, Roger Pau Monné

Hi Andrew,

On 26/03/2020 14:53, Andrew Cooper wrote:
> On 21/03/2020 22:19, Julien Grall wrote:
>>> diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h
>>> index f515ceee2a..16979a117c 100644
>>> --- a/xen/include/xen/xmalloc.h
>>> +++ b/xen/include/xen/xmalloc.h
>>> @@ -51,6 +51,17 @@
>>>   #define xmalloc_bytes(_bytes) _xmalloc(_bytes, SMP_CACHE_BYTES)
>>>   #define xzalloc_bytes(_bytes) _xzalloc(_bytes, SMP_CACHE_BYTES)
>>>
>>> +/* Allocate untyped storage and copying an existing instance. */
>>> +#define xmemdup_bytes(_src, _nr)                \
>>> +    ({                                          \
>>> +        unsigned long nr_ = (_nr);              \
>>> +        void *dst_ = xmalloc_bytes(nr_);        \
>> The nr_ vs _nr is really confusing to read. Could you re-implement the
>> function as a static inline?
> 
> I'd really prefer to, but sadly not.
> 
> That requires untangling headers sufficiently so we can include
> string.h, to be able to use memcpy.  I don't have time at the moment to
> sort that out.

Ok :(. We will have to live with the macro for the time being then.

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

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, back to index

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-20 21:24 [Xen-devel] [PATCH 0/4] x86/ucode: Cleanup - Part 2/n Andrew Cooper
2020-03-20 21:24 ` [Xen-devel] [PATCH 1/4] x86/ucode/amd: Fix assertion in compare_patch() Andrew Cooper
2020-03-21 16:45   ` Wei Liu
2020-03-20 21:24 ` [Xen-devel] [PATCH 2/4] x86/ucode: Fix error paths in apply_microcode() Andrew Cooper
2020-03-21 16:49   ` Wei Liu
2020-03-23  8:32   ` Jan Beulich
2020-03-20 21:24 ` [Xen-devel] [PATCH 3/4] xen: Drop raw_smp_processor_id() Andrew Cooper
2020-03-21 10:14   ` Julien Grall
2020-03-21 16:50   ` Wei Liu
2020-03-20 21:24 ` [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper Andrew Cooper
2020-03-21 16:51   ` Wei Liu
2020-03-21 22:19   ` Julien Grall
2020-03-23  8:38     ` Jan Beulich
2020-03-26 15:38       ` Andrew Cooper
2020-03-26 15:47         ` Jan Beulich
2020-03-26 14:53     ` Andrew Cooper
2020-03-26 19:05       ` Julien Grall
2020-03-23  8:41 ` [Xen-devel] [PATCH 0/4] x86/ucode: Cleanup - Part 2/n Jan Beulich

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git