| Message ID | 20240405121128.260493-3-fouad.hilly@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/xen-ucode: Introduce --force option | expand |
On 05.04.2024 14:11, Fouad Hilly wrote: > @@ -708,11 +712,13 @@ static long cf_check microcode_update_helper(void *data) > return ret; > } > > -int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len) > +int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len, unsigned int flags) > { > int ret; > struct ucode_buf *buffer; > > + ucode_force_flag = (flags == XENPF_UCODE_FLAG_FORCE_SET)? 1: 0; No need for ?: when the lhs has type bool. But - do we really need to resort to parameter passing via static variables here? If it's unavoidable, its setting needs to move inside a locked region (with that region covering everything up to all consumption of the value). Further, to avoid the same issue again when another flag wants adding, you want to check that all other bits in the flags field are clear. > --- a/xen/arch/x86/include/asm/microcode.h > +++ b/xen/arch/x86/include/asm/microcode.h > @@ -22,7 +22,7 @@ struct cpu_signature { > DECLARE_PER_CPU(struct cpu_signature, cpu_sig); > > void microcode_set_module(unsigned int idx); > -int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len); > +int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len, unsigned int flags); Nit: Too long line. > --- a/xen/include/public/platform.h > +++ b/xen/include/public/platform.h > @@ -99,6 +99,10 @@ struct xenpf_microcode_update { > /* IN variables. */ > XEN_GUEST_HANDLE(const_void) data;/* Pointer to microcode data */ > uint32_t length; /* Length of microcode data. */ > + uint32_t flags; /* Flags to be passed with ucode. */ > +/* Force to skip microcode version check when set */ > +#define XENPF_UCODE_FLAG_FORCE_NOT_SET 0 > +#define XENPF_UCODE_FLAG_FORCE_SET 1 > }; The safety of this growing of an existing stable ABI struct wants at least briefly mentioning in the description. > @@ -624,6 +628,10 @@ struct xenpf_ucode_revision { > typedef struct xenpf_ucode_revision xenpf_ucode_revision_t; > DEFINE_XEN_GUEST_HANDLE(xenpf_ucode_revision_t); > > +/* Hypercall to microcode_update with flags */ > +#define XENPF_microcode_update2 66 > + > + No double blank lines please. Jan
On Mon, Apr 8, 2024 at 10:16 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 05.04.2024 14:11, Fouad Hilly wrote: > > @@ -708,11 +712,13 @@ static long cf_check microcode_update_helper(void *data) > > return ret; > > } > > > > -int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len) > > +int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len, unsigned int flags) > > { > > int ret; > > struct ucode_buf *buffer; > > > > + ucode_force_flag = (flags == XENPF_UCODE_FLAG_FORCE_SET)? 1: 0; > > No need for ?: when the lhs has type bool. > > But - do we really need to resort to parameter passing via static variables > here? If it's unavoidable, its setting needs to move inside a locked region > (with that region covering everything up to all consumption of the value). There are many function calls and checks of the firmware between microcode_update() and the actual update, which makes static variable the viable option. In V2 I broke it down between the actual update_flags (static) and force_flag (local to firmware update function), I understand that might not be enough, I will look into further improvement for microcode_update flags in V3. > > Further, to avoid the same issue again when another flag wants adding, you > want to check that all other bits in the flags field are clear. The above check is checking all bits in the flags field. Are you referring to flag per bit where multiple flags can be set simultaneously? > > > --- a/xen/arch/x86/include/asm/microcode.h > > +++ b/xen/arch/x86/include/asm/microcode.h > > @@ -22,7 +22,7 @@ struct cpu_signature { > > DECLARE_PER_CPU(struct cpu_signature, cpu_sig); > > > > void microcode_set_module(unsigned int idx); > > -int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len); > > +int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len, unsigned int flags); > > Nit: Too long line. > > > --- a/xen/include/public/platform.h > > +++ b/xen/include/public/platform.h > > @@ -99,6 +99,10 @@ struct xenpf_microcode_update { > > /* IN variables. */ > > XEN_GUEST_HANDLE(const_void) data;/* Pointer to microcode data */ > > uint32_t length; /* Length of microcode data. */ > > + uint32_t flags; /* Flags to be passed with ucode. */ > > +/* Force to skip microcode version check when set */ > > +#define XENPF_UCODE_FLAG_FORCE_NOT_SET 0 > > +#define XENPF_UCODE_FLAG_FORCE_SET 1 > > }; > > The safety of this growing of an existing stable ABI struct wants at least > briefly mentioning in the description. > > > @@ -624,6 +628,10 @@ struct xenpf_ucode_revision { > > typedef struct xenpf_ucode_revision xenpf_ucode_revision_t; > > DEFINE_XEN_GUEST_HANDLE(xenpf_ucode_revision_t); > > > > +/* Hypercall to microcode_update with flags */ > > +#define XENPF_microcode_update2 66 > > + > > + > > No double blank lines please. > > Jan Thanks, Fouad
On 23.04.2024 16:53, Fouad Hilly wrote: > On Mon, Apr 8, 2024 at 10:16 AM Jan Beulich <jbeulich@suse.com> wrote: >> On 05.04.2024 14:11, Fouad Hilly wrote: >>> @@ -708,11 +712,13 @@ static long cf_check microcode_update_helper(void *data) >>> return ret; >>> } >>> >>> -int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len) >>> +int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len, unsigned int flags) >>> { >>> int ret; >>> struct ucode_buf *buffer; >>> >>> + ucode_force_flag = (flags == XENPF_UCODE_FLAG_FORCE_SET)? 1: 0; >> >> No need for ?: when the lhs has type bool. >> >> But - do we really need to resort to parameter passing via static variables >> here? If it's unavoidable, its setting needs to move inside a locked region >> (with that region covering everything up to all consumption of the value). > There are many function calls and checks of the firmware between > microcode_update() and the actual update, which makes static variable > the viable option. > In V2 I broke it down between the actual update_flags (static) and > force_flag (local to firmware update function), I understand that > might not be enough, I will look into further improvement for > microcode_update flags in V3. >> >> Further, to avoid the same issue again when another flag wants adding, you >> want to check that all other bits in the flags field are clear. > The above check is checking all bits in the flags field. Are you > referring to flag per bit where multiple flags can be set > simultaneously? No. What you do is treat a flags value of, say, 2 the same as a flags value of 0. That's wrong when considering that the value 2 may gain a meaning going forward. At this point you want to refuse flags values other than 0 or 1. Jan
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index 1c9f66ea8a0f..f987b2bd632d 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -40,6 +40,8 @@ #include <asm/processor.h> #include <asm/setup.h> +#include <public/platform.h> + #include "private.h" /* @@ -100,6 +102,8 @@ static bool ucode_in_nmi = true; bool __read_mostly opt_ucode_allow_same; +bool ucode_force_flag; + /* Protected by microcode_mutex */ static struct microcode_patch *microcode_cache; @@ -633,12 +637,12 @@ static long cf_check microcode_update_helper(void *data) microcode_cache); if ( result != NEW_UCODE && - !(opt_ucode_allow_same && result == SAME_UCODE) ) + !((opt_ucode_allow_same || ucode_force_flag) && result == SAME_UCODE) ) { spin_unlock(µcode_mutex); printk(XENLOG_WARNING "microcode: couldn't find any newer%s revision in the provided blob!\n", - opt_ucode_allow_same ? " (or the same)" : ""); + (opt_ucode_allow_same || ucode_force_flag) ? " (or the same)" : ""); microcode_free_patch(patch); ret = -ENOENT; @@ -708,11 +712,13 @@ static long cf_check microcode_update_helper(void *data) return ret; } -int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len) +int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len, unsigned int flags) { int ret; struct ucode_buf *buffer; + ucode_force_flag = (flags == XENPF_UCODE_FLAG_FORCE_SET)? 1: 0; + if ( len != (uint32_t)len ) return -E2BIG; diff --git a/xen/arch/x86/include/asm/microcode.h b/xen/arch/x86/include/asm/microcode.h index 8f59b20b0289..bcba356b4134 100644 --- a/xen/arch/x86/include/asm/microcode.h +++ b/xen/arch/x86/include/asm/microcode.h @@ -22,7 +22,7 @@ struct cpu_signature { DECLARE_PER_CPU(struct cpu_signature, cpu_sig); void microcode_set_module(unsigned int idx); -int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len); +int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len, unsigned int flags); int early_microcode_init(unsigned long *module_map, const struct multiboot_info *mbi); int microcode_init_cache(unsigned long *module_map, diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c index 95467b88ab64..3b29ede8b316 100644 --- a/xen/arch/x86/platform_hypercall.c +++ b/xen/arch/x86/platform_hypercall.c @@ -311,7 +311,17 @@ ret_t do_platform_op( guest_from_compat_handle(data, op->u.microcode.data); - ret = microcode_update(data, op->u.microcode.length); + ret = microcode_update(data, op->u.microcode.length, 0); + break; + } + + case XENPF_microcode_update2: + { + XEN_GUEST_HANDLE(const_void) data; + + guest_from_compat_handle(data, op->u.microcode.data); + + ret = microcode_update(data, op->u.microcode.length, op->u.microcode.flags); break; } diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h index 15777b541690..31e59ba3d07b 100644 --- a/xen/include/public/platform.h +++ b/xen/include/public/platform.h @@ -99,6 +99,10 @@ struct xenpf_microcode_update { /* IN variables. */ XEN_GUEST_HANDLE(const_void) data;/* Pointer to microcode data */ uint32_t length; /* Length of microcode data. */ + uint32_t flags; /* Flags to be passed with ucode. */ +/* Force to skip microcode version check when set */ +#define XENPF_UCODE_FLAG_FORCE_NOT_SET 0 +#define XENPF_UCODE_FLAG_FORCE_SET 1 }; typedef struct xenpf_microcode_update xenpf_microcode_update_t; DEFINE_XEN_GUEST_HANDLE(xenpf_microcode_update_t); @@ -624,6 +628,10 @@ struct xenpf_ucode_revision { typedef struct xenpf_ucode_revision xenpf_ucode_revision_t; DEFINE_XEN_GUEST_HANDLE(xenpf_ucode_revision_t); +/* Hypercall to microcode_update with flags */ +#define XENPF_microcode_update2 66 + + /* * ` enum neg_errnoval * ` HYPERVISOR_platform_op(const struct xen_platform_op*);
Refactor microcode_update() hypercall by adding flags field. Introduce XENPF_microcode_update2 hypercall to handle flags field. Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com> --- xen/arch/x86/cpu/microcode/core.c | 12 +++++++++--- xen/arch/x86/include/asm/microcode.h | 2 +- xen/arch/x86/platform_hypercall.c | 12 +++++++++++- xen/include/public/platform.h | 8 ++++++++ 4 files changed, 29 insertions(+), 5 deletions(-)