We've got a perfectly good vendor abstraction already for microcode. No need
for a second ad-hoc one in microcode_scan_module().
This is in preparation to use ucode_ops.cpio_path in multiple places.
These paths are only used during __init, so take the opportunity to move them
into __initconst.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>
---
xen/arch/x86/cpu/microcode/amd.c | 4 ++++
xen/arch/x86/cpu/microcode/core.c | 14 +++-----------
xen/arch/x86/cpu/microcode/intel.c | 4 ++++
xen/arch/x86/cpu/microcode/private.h | 7 +++++++
4 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 0fe869eff119..c7a779c1d885 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -443,11 +443,15 @@ static struct microcode_patch *cf_check cpu_request_microcode(
return patch;
}
+static const char __initconst amd_cpio_path[] =
+ "kernel/x86/microcode/AuthenticAMD.bin";
+
static const struct microcode_ops __initconst_cf_clobber amd_ucode_ops = {
.cpu_request_microcode = cpu_request_microcode,
.collect_cpu_info = collect_cpu_info,
.apply_microcode = apply_microcode,
.compare_patch = compare_patch,
+ .cpio_path = amd_cpio_path,
};
void __init ucode_probe_amd(struct microcode_ops *ops)
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index fc31ab35c3c8..6a87390ab770 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -170,25 +170,19 @@ static int __init cf_check parse_ucode(const char *s)
}
custom_param("ucode", parse_ucode);
+static struct microcode_ops __ro_after_init ucode_ops;
+
static void __init microcode_scan_module(struct boot_info *bi)
{
uint64_t *_blob_start;
unsigned long _blob_size;
struct cpio_data cd;
- const char *p = NULL;
int i;
ucode_blob.size = 0;
if ( !opt_scan )
return;
- if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
- p = "kernel/x86/microcode/AuthenticAMD.bin";
- else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
- p = "kernel/x86/microcode/GenuineIntel.bin";
- else
- return;
-
/*
* Try all modules and see whichever could be the microcode blob.
*/
@@ -207,7 +201,7 @@ static void __init microcode_scan_module(struct boot_info *bi)
}
cd.data = NULL;
cd.size = 0;
- cd = find_cpio_data(p, _blob_start, _blob_size);
+ cd = find_cpio_data(ucode_ops.cpio_path, _blob_start, _blob_size);
if ( cd.data )
{
ucode_blob.size = cd.size;
@@ -218,8 +212,6 @@ static void __init microcode_scan_module(struct boot_info *bi)
}
}
-static struct microcode_ops __ro_after_init ucode_ops;
-
static DEFINE_SPINLOCK(microcode_mutex);
DEFINE_PER_CPU(struct cpu_signature, cpu_sig);
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index bad51f64724a..aad6a693e800 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -405,11 +405,15 @@ static bool __init can_load_microcode(void)
return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);
}
+static const char __initconst intel_cpio_path[] =
+ "kernel/x86/microcode/GenuineIntel.bin";
+
static const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
.cpu_request_microcode = cpu_request_microcode,
.collect_cpu_info = collect_cpu_info,
.apply_microcode = apply_microcode,
.compare_patch = compare_patch,
+ .cpio_path = intel_cpio_path,
};
void __init ucode_probe_intel(struct microcode_ops *ops)
diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
index c72f060ac394..c9dd8ba066f9 100644
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -59,6 +59,13 @@ struct microcode_ops {
*/
enum microcode_match_result (*compare_patch)(
const struct microcode_patch *new, const struct microcode_patch *old);
+
+ /*
+ * For Linux inird microcode compatibliity.
+ *
+ * The path where this vendor's microcode can be found in CPIO.
+ */
+ const char *cpio_path;
};
/*
--
2.39.5
On 10/28/24 05:18, Andrew Cooper wrote: > We've got a perfectly good vendor abstraction already for microcode. No need > for a second ad-hoc one in microcode_scan_module(). > > This is in preparation to use ucode_ops.cpio_path in multiple places. > > These paths are only used during __init, so take the opportunity to move them > into __initconst. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Daniel P. Smith <dpsmith@apertussolutions.com> > --- Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
On 28.10.2024 10:18, Andrew Cooper wrote:
> We've got a perfectly good vendor abstraction already for microcode. No need
> for a second ad-hoc one in microcode_scan_module().
>
> This is in preparation to use ucode_ops.cpio_path in multiple places.
>
> These paths are only used during __init, so take the opportunity to move them
> into __initconst.
As an alternative to this, how about ...
> --- a/xen/arch/x86/cpu/microcode/private.h
> +++ b/xen/arch/x86/cpu/microcode/private.h
> @@ -59,6 +59,13 @@ struct microcode_ops {
> */
> enum microcode_match_result (*compare_patch)(
> const struct microcode_patch *new, const struct microcode_patch *old);
> +
> + /*
> + * For Linux inird microcode compatibliity.
> + *
> + * The path where this vendor's microcode can be found in CPIO.
> + */
> + const char *cpio_path;
const char cpio_path[];
inheriting the __initconst from the struct instances?
Acked-by: Jan Beulich <jbeulich@suse.com>
with a slight preference to the form without the extra pointer. Except that:
gcc14 looks to be buggy when it comes to the copying of such a struct. The
example below yields an internal compiler error. And the direct structure
assignment also doesn't quite do what I would expect it to do (visible when
commenting out the "else" branch. Bottom line - leave the code as is.
Jan
struct s {
unsigned long ul;
int i;
char ac[];
};
const struct s gs = { 3, -4, "abcdef" };
void copy_s(struct s*d, const struct s*s) {
*d = *s;
}
unsigned test(struct s*ps, _Bool direct) {
if(direct)
*ps = gs;
else
copy_s(ps, &gs);
return sizeof(*ps);
}
unsigned size(void) {
return sizeof(gs);
}
On 28/10/2024 2:25 pm, Jan Beulich wrote:
> On 28.10.2024 10:18, Andrew Cooper wrote:
>> We've got a perfectly good vendor abstraction already for microcode. No need
>> for a second ad-hoc one in microcode_scan_module().
>>
>> This is in preparation to use ucode_ops.cpio_path in multiple places.
>>
>> These paths are only used during __init, so take the opportunity to move them
>> into __initconst.
> As an alternative to this, how about ...
>
>> --- a/xen/arch/x86/cpu/microcode/private.h
>> +++ b/xen/arch/x86/cpu/microcode/private.h
>> @@ -59,6 +59,13 @@ struct microcode_ops {
>> */
>> enum microcode_match_result (*compare_patch)(
>> const struct microcode_patch *new, const struct microcode_patch *old);
>> +
>> + /*
>> + * For Linux inird microcode compatibliity.
>> + *
>> + * The path where this vendor's microcode can be found in CPIO.
>> + */
>> + const char *cpio_path;
> const char cpio_path[];
>
> inheriting the __initconst from the struct instances?
> Acked-by: Jan Beulich <jbeulich@suse.com>
> with a slight preference to the form without the extra pointer.
I'm slightly surprised at this request, given that the form with the
pointer results in less data held at runtime.
> Except that:
> gcc14 looks to be buggy when it comes to the copying of such a struct. The
> example below yields an internal compiler error. And the direct structure
> assignment also doesn't quite do what I would expect it to do (visible when
> commenting out the "else" branch. Bottom line - leave the code as is.
It's unfortunate to hit an ICE, but the copy cannot possibly work in the
first place.
ucode_ops is in a separate translation unit and has no space allocated
after the flexible member. Any copy into it is memory corruption of
whatever object happens to be sequentially after ucode_ops.
The only way it would work is having `const char cpio_path[40];` which
is long enough for anything we'd expect to find.
But again, that involves holding init-only data post init.
~Andrew
On 28.10.2024 15:38, Andrew Cooper wrote:
> On 28/10/2024 2:25 pm, Jan Beulich wrote:
>> On 28.10.2024 10:18, Andrew Cooper wrote:
>>> We've got a perfectly good vendor abstraction already for microcode. No need
>>> for a second ad-hoc one in microcode_scan_module().
>>>
>>> This is in preparation to use ucode_ops.cpio_path in multiple places.
>>>
>>> These paths are only used during __init, so take the opportunity to move them
>>> into __initconst.
>> As an alternative to this, how about ...
>>
>>> --- a/xen/arch/x86/cpu/microcode/private.h
>>> +++ b/xen/arch/x86/cpu/microcode/private.h
>>> @@ -59,6 +59,13 @@ struct microcode_ops {
>>> */
>>> enum microcode_match_result (*compare_patch)(
>>> const struct microcode_patch *new, const struct microcode_patch *old);
>>> +
>>> + /*
>>> + * For Linux inird microcode compatibliity.
>>> + *
>>> + * The path where this vendor's microcode can be found in CPIO.
>>> + */
>>> + const char *cpio_path;
>> const char cpio_path[];
>>
>> inheriting the __initconst from the struct instances?
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>> with a slight preference to the form without the extra pointer.
>
> I'm slightly surprised at this request, given that the form with the
> pointer results in less data held at runtime.
No, it doesn't. Yet I only now realize that ...
>> Except that:
>> gcc14 looks to be buggy when it comes to the copying of such a struct. The
>> example below yields an internal compiler error. And the direct structure
>> assignment also doesn't quite do what I would expect it to do (visible when
>> commenting out the "else" branch. Bottom line - leave the code as is.
>
> It's unfortunate to hit an ICE, but the copy cannot possibly work in the
> first place.
>
> ucode_ops is in a separate translation unit and has no space allocated
> after the flexible member. Any copy into it is memory corruption of
> whatever object happens to be sequentially after ucode_ops.
... my expectation of how the copy ought to work (and how the C standard,
at least in close enough an example, specifies it) would specifically _not_
suit our needs. The copy ought to only cover sizeof(struct ...), i.e. not
the string. Yet we'd need that string to be copied to be usable for our
purposes.
> The only way it would work is having `const char cpio_path[40];` which
> is long enough for anything we'd expect to find.
>
> But again, that involves holding init-only data post init.
This, indeed, would increase post-init size. Yet with the compiler issue
no question arises anyway as to how this needs doing.
Jan
© 2016 - 2024 Red Hat, Inc.