| Message ID | 068a32f917937baca179d7ff4c483ec1584defb4.1576630344.git.elnikety@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/microcode: Support builtin CPU microcode | expand |
On 18.12.2019 02:32, Eslam Elnikety wrote: > Decouple the microcode referencing mechanism when using GRUB to that > when using EFI. This allows us to avoid the "unspecified effect" of > using `<integer> | scan` along xen.efi. I guess "unspecified effect" in the doc was pretty pointless - such options have been ignored before; in fact ... > With that, Xen can explicitly > ignore those named options when using EFI. ... I don't see things becoming any more explicit (not even parsing the options was quite explicit to me). > As an added benefit, > we get a straightfoward parsing of the ucode parameter. It's a single if() you eliminate - for me this doesn't make it meaningfully more straightforward. > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -2128,7 +2128,13 @@ logic applies: > ### ucode (x86) > > `= List of [ <integer> | scan=<bool>, nmi=<bool> ]` > > -Specify how and where to find CPU microcode update blob. > + Applicability: x86 > + Default: `nmi` > + > +Controls for CPU microcode loading. For early loading, this parameter can > +specify how and where to find the microcode update blob. For late loading, > +this parameter specifies if the update happens within a NMI handler or in > +a stop_machine context. It's always stop_machine context, isn't it? I also don't think this implementation detail belongs here. > --- a/xen/arch/x86/microcode.c > +++ b/xen/arch/x86/microcode.c > @@ -60,7 +60,7 @@ > > static module_t __initdata ucode_mod; > static signed int __initdata ucode_mod_idx; > -static bool_t __initdata ucode_mod_forced; > +static signed int __initdata ucode_mod_efi_idx; I don't see anything negative be put into here - should be unsigned int then. > @@ -105,16 +105,10 @@ static struct microcode_patch *microcode_cache; > > void __init microcode_set_module(unsigned int idx) > { > - ucode_mod_idx = idx; > - ucode_mod_forced = 1; > + ucode_mod_efi_idx = idx; Is it guaranteed (now and forever) that the index passed in is non-zero? You changes to microcode_grab_module() imply so, but just looking at the call site of the function I can't convince myself this is the case. _If_ it is (thought to be) guaranteed, then I think you at least want to ASSERT() here, perhaps with a comment. > } > > -/* > - * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. Both options are > - * optional. If the EFI has forced which of the multiboot payloads is to be > - * used, only nmi=<bool> is parsed. > - */ > -static int __init parse_ucode(const char *s) > +static int __init parse_ucode_param(const char *s) Any particular reason for the renaming? The function name was quite fine imo. Jan
On 18.12.19 12:49, Jan Beulich wrote: > On 18.12.2019 02:32, Eslam Elnikety wrote: >> Decouple the microcode referencing mechanism when using GRUB to that >> when using EFI. This allows us to avoid the "unspecified effect" of >> using `<integer> | scan` along xen.efi. > > I guess "unspecified effect" in the doc was pretty pointless - such > options have been ignored before; in fact ... > >> With that, Xen can explicitly >> ignore those named options when using EFI. > > ... I don't see things becoming any more explicit (not even parsing > the options was quite explicit to me). > I agree that those options have been ignored so far in the case of EFI. The documentation contradicts that however. The documentation: * says <integer> has unspecified effect. * does not mention anything about scan being ignored. With this patch, it is explicit in code and in documentation that both options are ignored in case of EFI. >> As an added benefit, >> we get a straightfoward parsing of the ucode parameter. > > It's a single if() you eliminate - for me this doesn't make it > meaningfully more straightforward. > As we decouple ucode_mod_idx and ucode_mod_efi_idx, the parsing of the ucode= just follows the pattern "[ <integer> | scan=<bool>, nmi=<bool> ]" and it does not have to cater for corner cases. In either case, if you strongly disagree with the wording, I can drop that sentence. >> --- a/docs/misc/xen-command-line.pandoc >> +++ b/docs/misc/xen-command-line.pandoc >> @@ -2128,7 +2128,13 @@ logic applies: >> ### ucode (x86) >> > `= List of [ <integer> | scan=<bool>, nmi=<bool> ]` >> >> -Specify how and where to find CPU microcode update blob. >> + Applicability: x86 >> + Default: `nmi` >> + >> +Controls for CPU microcode loading. For early loading, this parameter can >> +specify how and where to find the microcode update blob. For late loading, >> +this parameter specifies if the update happens within a NMI handler or in >> +a stop_machine context. > > It's always stop_machine context, isn't it? I also don't think this > implementation detail belongs here. > Needs a better wording indeed. Let me know if you have particular suggestions, and I will incorporate in v3. >> --- a/xen/arch/x86/microcode.c >> +++ b/xen/arch/x86/microcode.c >> @@ -60,7 +60,7 @@ >> >> static module_t __initdata ucode_mod; >> static signed int __initdata ucode_mod_idx; >> -static bool_t __initdata ucode_mod_forced; >> +static signed int __initdata ucode_mod_efi_idx; > > I don't see anything negative be put into here - should be unsigned > int then. > Correct! The type just carried over from the coupling with ucode_mod_idx. Will adjust in v3. >> @@ -105,16 +105,10 @@ static struct microcode_patch *microcode_cache; >> >> void __init microcode_set_module(unsigned int idx) >> { >> - ucode_mod_idx = idx; >> - ucode_mod_forced = 1; >> + ucode_mod_efi_idx = idx; > > Is it guaranteed (now and forever) that the index passed in is > non-zero? You changes to microcode_grab_module() imply so, but > just looking at the call site of the function I can't convince > myself this is the case. _If_ it is (thought to be) guaranteed, > then I think you at least want to ASSERT() here, perhaps with > a comment. > For x86, it seems we have that guarantee (given that we must have a dom0). Right? >> } >> >> -/* >> - * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. Both options are >> - * optional. If the EFI has forced which of the multiboot payloads is to be >> - * used, only nmi=<bool> is parsed. >> - */ >> -static int __init parse_ucode(const char *s) >> +static int __init parse_ucode_param(const char *s) > > Any particular reason for the renaming? The function name was quite > fine imo. > To me, "parse_ucode" is a misnomer. Thanks for your comments, Jan. -- Eslam
On 19.12.2019 22:08, Eslam Elnikety wrote: > On 18.12.19 12:49, Jan Beulich wrote: >> On 18.12.2019 02:32, Eslam Elnikety wrote: >>> Decouple the microcode referencing mechanism when using GRUB to that >>> when using EFI. This allows us to avoid the "unspecified effect" of >>> using `<integer> | scan` along xen.efi. >> >> I guess "unspecified effect" in the doc was pretty pointless - such >> options have been ignored before; in fact ... >> >>> With that, Xen can explicitly >>> ignore those named options when using EFI. >> >> ... I don't see things becoming any more explicit (not even parsing >> the options was quite explicit to me). >> > > I agree that those options have been ignored so far in the case of EFI. > The documentation contradicts that however. The documentation: > * says <integer> has unspecified effect. > * does not mention anything about scan being ignored. > > With this patch, it is explicit in code and in documentation that both > options are ignored in case of EFI. But isn't it rather that ucode=scan could (and hence perhaps should) also have its value on EFI? >>> As an added benefit, >>> we get a straightfoward parsing of the ucode parameter. >> >> It's a single if() you eliminate - for me this doesn't make it >> meaningfully more straightforward. >> > > As we decouple ucode_mod_idx and ucode_mod_efi_idx, the parsing of the > ucode= just follows the pattern "[ <integer> | scan=<bool>, nmi=<bool> > ]" and it does not have to cater for corner cases. In either case, if > you strongly disagree with the wording, I can drop that sentence. Well, what I don't like is giving the impression that things have been worse than they actually are. >>> --- a/docs/misc/xen-command-line.pandoc >>> +++ b/docs/misc/xen-command-line.pandoc >>> @@ -2128,7 +2128,13 @@ logic applies: >>> ### ucode (x86) >>> > `= List of [ <integer> | scan=<bool>, nmi=<bool> ]` >>> >>> -Specify how and where to find CPU microcode update blob. >>> + Applicability: x86 >>> + Default: `nmi` >>> + >>> +Controls for CPU microcode loading. For early loading, this parameter can >>> +specify how and where to find the microcode update blob. For late loading, >>> +this parameter specifies if the update happens within a NMI handler or in >>> +a stop_machine context. >> >> It's always stop_machine context, isn't it? I also don't think this >> implementation detail belongs here. >> > > Needs a better wording indeed. Let me know if you have particular > suggestions, and I will incorporate in v3. Just drop everything from "or" onwards? >>> @@ -105,16 +105,10 @@ static struct microcode_patch *microcode_cache; >>> >>> void __init microcode_set_module(unsigned int idx) >>> { >>> - ucode_mod_idx = idx; >>> - ucode_mod_forced = 1; >>> + ucode_mod_efi_idx = idx; >> >> Is it guaranteed (now and forever) that the index passed in is >> non-zero? You changes to microcode_grab_module() imply so, but >> just looking at the call site of the function I can't convince >> myself this is the case. _If_ it is (thought to be) guaranteed, >> then I think you at least want to ASSERT() here, perhaps with >> a comment. >> > > For x86, it seems we have that guarantee (given that we must have a > dom0). Right? For fully bringing up the system - yes. But there's no reason to have a Dom0 if all you're after is testing early Xen boot. There'll be a panic() in the case, but there shouldn't be anything breaking proper behavior prior to this point. >>> } >>> >>> -/* >>> - * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. Both options are >>> - * optional. If the EFI has forced which of the multiboot payloads is to be >>> - * used, only nmi=<bool> is parsed. >>> - */ >>> -static int __init parse_ucode(const char *s) >>> +static int __init parse_ucode_param(const char *s) >> >> Any particular reason for the renaming? The function name was quite >> fine imo. >> > > To me, "parse_ucode" is a misnomer. Well, parse_"ucode= isn't a valid identifier. parse_ucode is what results when stripping everything that makes it invalid. I can see the ambiguity with parsing actual ucode, but the context here makes it pretty clear what the function is about. Personally I'd prefer such unnecessary renames to be avoided. Jan
Picking this up again after the break. Apologies for the delay. On 20.12.19 10:53, Jan Beulich wrote: > On 19.12.2019 22:08, Eslam Elnikety wrote: >> On 18.12.19 12:49, Jan Beulich wrote: >>> On 18.12.2019 02:32, Eslam Elnikety wrote: >>>> Decouple the microcode referencing mechanism when using GRUB to that >>>> when using EFI. This allows us to avoid the "unspecified effect" of >>>> using `<integer> | scan` along xen.efi. >>> >>> I guess "unspecified effect" in the doc was pretty pointless - such >>> options have been ignored before; in fact ... >>> >>>> With that, Xen can explicitly >>>> ignore those named options when using EFI. >>> >>> ... I don't see things becoming any more explicit (not even parsing >>> the options was quite explicit to me). >>> >> >> I agree that those options have been ignored so far in the case of EFI. >> The documentation contradicts that however. The documentation: >> * says <integer> has unspecified effect. >> * does not mention anything about scan being ignored. >> >> With this patch, it is explicit in code and in documentation that both >> options are ignored in case of EFI. > > But isn't it rather that ucode=scan could (and hence perhaps should) > also have its value on EFI? > I do not see "ucode=scan" applicable in anyway in the case of EFI. In EFI, there are not "modules" to scan through, but rather the efi config points exactly to the microcode blob. >>>> --- a/docs/misc/xen-command-line.pandoc >>>> +++ b/docs/misc/xen-command-line.pandoc >>>> @@ -2128,7 +2128,13 @@ logic applies: >>>> ### ucode (x86) >>>> > `= List of [ <integer> | scan=<bool>, nmi=<bool> ]` >>>> >>>> -Specify how and where to find CPU microcode update blob. >>>> + Applicability: x86 >>>> + Default: `nmi` >>>> + >>>> +Controls for CPU microcode loading. For early loading, this parameter can >>>> +specify how and where to find the microcode update blob. For late loading, >>>> +this parameter specifies if the update happens within a NMI handler or in >>>> +a stop_machine context. >>> >>> It's always stop_machine context, isn't it? I also don't think this >>> implementation detail belongs here. >>> >> >> Needs a better wording indeed. Let me know if you have particular >> suggestions, and I will incorporate in v3. > > Just drop everything from "or" onwards? > Ack >>>> @@ -105,16 +105,10 @@ static struct microcode_patch *microcode_cache; >>>> >>>> void __init microcode_set_module(unsigned int idx) >>>> { >>>> - ucode_mod_idx = idx; >>>> - ucode_mod_forced = 1; >>>> + ucode_mod_efi_idx = idx; >>> >>> Is it guaranteed (now and forever) that the index passed in is >>> non-zero? You changes to microcode_grab_module() imply so, but >>> just looking at the call site of the function I can't convince >>> myself this is the case. _If_ it is (thought to be) guaranteed, >>> then I think you at least want to ASSERT() here, perhaps with >>> a comment. >>> >> >> For x86, it seems we have that guarantee (given that we must have a >> dom0). Right? > > For fully bringing up the system - yes. But there's no reason to > have a Dom0 if all you're after is testing early Xen boot. There'll > be a panic() in the case, but there shouldn't be anything breaking > proper behavior prior to this point. > That's a valid point indeed. v3 will handle index being zero. >>>> } >>>> >>>> -/* >>>> - * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. Both options are >>>> - * optional. If the EFI has forced which of the multiboot payloads is to be >>>> - * used, only nmi=<bool> is parsed. >>>> - */ >>>> -static int __init parse_ucode(const char *s) >>>> +static int __init parse_ucode_param(const char *s) >>> >>> Any particular reason for the renaming? The function name was quite >>> fine imo. >>> >> >> To me, "parse_ucode" is a misnomer. > > Well, parse_"ucode= isn't a valid identifier. parse_ucode is what > results when stripping everything that makes it invalid. I can > see the ambiguity with parsing actual ucode, but the context here > makes it pretty clear what the function is about. Personally I'd > prefer such unnecessary renames to be avoided. Ack -- Eslam
On 17.01.2020 20:06, Eslam Elnikety wrote: > On 20.12.19 10:53, Jan Beulich wrote: >> On 19.12.2019 22:08, Eslam Elnikety wrote: >>> On 18.12.19 12:49, Jan Beulich wrote: >>>> On 18.12.2019 02:32, Eslam Elnikety wrote: >>>>> Decouple the microcode referencing mechanism when using GRUB to that >>>>> when using EFI. This allows us to avoid the "unspecified effect" of >>>>> using `<integer> | scan` along xen.efi. >>>> >>>> I guess "unspecified effect" in the doc was pretty pointless - such >>>> options have been ignored before; in fact ... >>>> >>>>> With that, Xen can explicitly >>>>> ignore those named options when using EFI. >>>> >>>> ... I don't see things becoming any more explicit (not even parsing >>>> the options was quite explicit to me). >>>> >>> >>> I agree that those options have been ignored so far in the case of EFI. >>> The documentation contradicts that however. The documentation: >>> * says <integer> has unspecified effect. >>> * does not mention anything about scan being ignored. >>> >>> With this patch, it is explicit in code and in documentation that both >>> options are ignored in case of EFI. >> >> But isn't it rather that ucode=scan could (and hence perhaps should) >> also have its value on EFI? >> > > I do not see "ucode=scan" applicable in anyway in the case of EFI. In > EFI, there are not "modules" to scan through, but rather the efi config > points exactly to the microcode blob. What would be wrong with the EFI code to also inspect whatever has been specified with ramdisk= if there was no ucode= ? Jan
On 20.01.20 09:42, Jan Beulich wrote: > On 17.01.2020 20:06, Eslam Elnikety wrote: >> On 20.12.19 10:53, Jan Beulich wrote: >>> On 19.12.2019 22:08, Eslam Elnikety wrote: >>>> On 18.12.19 12:49, Jan Beulich wrote: >>>>> On 18.12.2019 02:32, Eslam Elnikety wrote: >>>>>> Decouple the microcode referencing mechanism when using GRUB to that >>>>>> when using EFI. This allows us to avoid the "unspecified effect" of >>>>>> using `<integer> | scan` along xen.efi. >>>>> >>>>> I guess "unspecified effect" in the doc was pretty pointless - such >>>>> options have been ignored before; in fact ... >>>>> >>>>>> With that, Xen can explicitly >>>>>> ignore those named options when using EFI. >>>>> >>>>> ... I don't see things becoming any more explicit (not even parsing >>>>> the options was quite explicit to me). >>>>> >>>> >>>> I agree that those options have been ignored so far in the case of EFI. >>>> The documentation contradicts that however. The documentation: >>>> * says <integer> has unspecified effect. >>>> * does not mention anything about scan being ignored. >>>> >>>> With this patch, it is explicit in code and in documentation that both >>>> options are ignored in case of EFI. >>> >>> But isn't it rather that ucode=scan could (and hence perhaps should) >>> also have its value on EFI? >>> >> >> I do not see "ucode=scan" applicable in anyway in the case of EFI. In >> EFI, there are not "modules" to scan through, but rather the efi config >> points exactly to the microcode blob. > > What would be wrong with the EFI code to also inspect whatever has been > specified with ramdisk= if there was no ucode= ? > > Jan > I see, interesting. This sounds like a legitimate use case indeed. I wonder, would I be breaking anything if I simply allow the existing code that iterates over modules to kick in when ucode=scan irrespective of EFI or legacy boot? Also, it seems to me that the ucode= specified by efi.cfg would take precedence over the ucode=scan. Do not you think? Eslam
On 21.01.2020 00:50, Eslam Elnikety wrote: > On 20.01.20 09:42, Jan Beulich wrote: >> On 17.01.2020 20:06, Eslam Elnikety wrote: >>> On 20.12.19 10:53, Jan Beulich wrote: >>>> On 19.12.2019 22:08, Eslam Elnikety wrote: >>>>> On 18.12.19 12:49, Jan Beulich wrote: >>>>>> On 18.12.2019 02:32, Eslam Elnikety wrote: >>>>>>> Decouple the microcode referencing mechanism when using GRUB to that >>>>>>> when using EFI. This allows us to avoid the "unspecified effect" of >>>>>>> using `<integer> | scan` along xen.efi. >>>>>> >>>>>> I guess "unspecified effect" in the doc was pretty pointless - such >>>>>> options have been ignored before; in fact ... >>>>>> >>>>>>> With that, Xen can explicitly >>>>>>> ignore those named options when using EFI. >>>>>> >>>>>> ... I don't see things becoming any more explicit (not even parsing >>>>>> the options was quite explicit to me). >>>>>> >>>>> >>>>> I agree that those options have been ignored so far in the case of EFI. >>>>> The documentation contradicts that however. The documentation: >>>>> * says <integer> has unspecified effect. >>>>> * does not mention anything about scan being ignored. >>>>> >>>>> With this patch, it is explicit in code and in documentation that both >>>>> options are ignored in case of EFI. >>>> >>>> But isn't it rather that ucode=scan could (and hence perhaps should) >>>> also have its value on EFI? >>>> >>> >>> I do not see "ucode=scan" applicable in anyway in the case of EFI. In >>> EFI, there are not "modules" to scan through, but rather the efi config >>> points exactly to the microcode blob. >> >> What would be wrong with the EFI code to also inspect whatever has been >> specified with ramdisk= if there was no ucode= ? > > I see, interesting. This sounds like a legitimate use case indeed. I > wonder, would I be breaking anything if I simply allow the existing code > that iterates over modules to kick in when ucode=scan irrespective of > EFI or legacy boot? I don't think so, no, but it would need double checking (and mentioning in the description and/or documentation). > Also, it seems to me that the ucode= specified by > efi.cfg would take precedence over the ucode=scan. Do not you think? I guess we need to settle on what we want to take precedence and then make sure code also behaves this way. But yes, I think ucode= from the .cfg should supersede ucode=scan on the command line. A possibly useful adjustment to this might be to distinguish whether the ucode=scan was in a specific .cfg section while the ucode= was in [global] (i.e. sort of a default), in which case maybe the ucode=scan should win. Thoughts? Jan
On 21.01.20 10:27, Jan Beulich wrote: > On 21.01.2020 00:50, Eslam Elnikety wrote: >> On 20.01.20 09:42, Jan Beulich wrote: >>> On 17.01.2020 20:06, Eslam Elnikety wrote: >>>> On 20.12.19 10:53, Jan Beulich wrote: >>>>> On 19.12.2019 22:08, Eslam Elnikety wrote: >>>>>> On 18.12.19 12:49, Jan Beulich wrote: >>>>>>> On 18.12.2019 02:32, Eslam Elnikety wrote: >>>>>>>> Decouple the microcode referencing mechanism when using GRUB to that >>>>>>>> when using EFI. This allows us to avoid the "unspecified effect" of >>>>>>>> using `<integer> | scan` along xen.efi. >>>>>>> >>>>>>> I guess "unspecified effect" in the doc was pretty pointless - such >>>>>>> options have been ignored before; in fact ... >>>>>>> >>>>>>>> With that, Xen can explicitly >>>>>>>> ignore those named options when using EFI. >>>>>>> >>>>>>> ... I don't see things becoming any more explicit (not even parsing >>>>>>> the options was quite explicit to me). >>>>>>> >>>>>> >>>>>> I agree that those options have been ignored so far in the case of EFI. >>>>>> The documentation contradicts that however. The documentation: >>>>>> * says <integer> has unspecified effect. >>>>>> * does not mention anything about scan being ignored. >>>>>> >>>>>> With this patch, it is explicit in code and in documentation that both >>>>>> options are ignored in case of EFI. >>>>> >>>>> But isn't it rather that ucode=scan could (and hence perhaps should) >>>>> also have its value on EFI? >>>>> >>>> >>>> I do not see "ucode=scan" applicable in anyway in the case of EFI. In >>>> EFI, there are not "modules" to scan through, but rather the efi config >>>> points exactly to the microcode blob. >>> >>> What would be wrong with the EFI code to also inspect whatever has been >>> specified with ramdisk= if there was no ucode= ? >> >> I see, interesting. This sounds like a legitimate use case indeed. I >> wonder, would I be breaking anything if I simply allow the existing code >> that iterates over modules to kick in when ucode=scan irrespective of >> EFI or legacy boot? > > I don't think so, no, but it would need double checking (and > mentioning in the description and/or documentation). > >> Also, it seems to me that the ucode= specified by >> efi.cfg would take precedence over the ucode=scan. Do not you think? > > I guess we need to settle on what we want to take precedence and > then make sure code also behaves this way. But yes, I think ucode= > from the .cfg should supersede ucode=scan on the command line. A > possibly useful adjustment to this might be to distinguish whether > the ucode=scan was in a specific .cfg section while the ucode= was > in [global] (i.e. sort of a default), in which case maybe the > ucode=scan should win. Thoughts? > > Jan > I think any ucode= in the EFI .cfg ought to supersede the ucode=scan. The semantics are simpler in this case, rather than having to worry about where exactly the ucode= was specified in the EFI .cfg. With that, an administrator would default to using ucode=scan on the commandline to load the ramdisk microcode, and a ucode= in .cfg would be an explicit signal to use different microcode. Eslam
On Tue, Jan 21, 2020 at 10:27:47AM +0100, Jan Beulich wrote: > On 21.01.2020 00:50, Eslam Elnikety wrote: > > On 20.01.20 09:42, Jan Beulich wrote: > >> On 17.01.2020 20:06, Eslam Elnikety wrote: > >>> On 20.12.19 10:53, Jan Beulich wrote: > >>>> On 19.12.2019 22:08, Eslam Elnikety wrote: > >>>>> On 18.12.19 12:49, Jan Beulich wrote: > >>>>>> On 18.12.2019 02:32, Eslam Elnikety wrote: > >>>>>>> Decouple the microcode referencing mechanism when using GRUB to that > >>>>>>> when using EFI. This allows us to avoid the "unspecified effect" of > >>>>>>> using `<integer> | scan` along xen.efi. > >>>>>> > >>>>>> I guess "unspecified effect" in the doc was pretty pointless - such > >>>>>> options have been ignored before; in fact ... > >>>>>> > >>>>>>> With that, Xen can explicitly > >>>>>>> ignore those named options when using EFI. > >>>>>> > >>>>>> ... I don't see things becoming any more explicit (not even parsing > >>>>>> the options was quite explicit to me). > >>>>>> > >>>>> > >>>>> I agree that those options have been ignored so far in the case of EFI. > >>>>> The documentation contradicts that however. The documentation: > >>>>> * says <integer> has unspecified effect. > >>>>> * does not mention anything about scan being ignored. > >>>>> > >>>>> With this patch, it is explicit in code and in documentation that both > >>>>> options are ignored in case of EFI. > >>>> > >>>> But isn't it rather that ucode=scan could (and hence perhaps should) > >>>> also have its value on EFI? > >>>> > >>> > >>> I do not see "ucode=scan" applicable in anyway in the case of EFI. In > >>> EFI, there are not "modules" to scan through, but rather the efi config > >>> points exactly to the microcode blob. > >> > >> What would be wrong with the EFI code to also inspect whatever has been > >> specified with ramdisk= if there was no ucode= ? > > > > I see, interesting. This sounds like a legitimate use case indeed. I > > wonder, would I be breaking anything if I simply allow the existing code > > that iterates over modules to kick in when ucode=scan irrespective of > > EFI or legacy boot? > > I don't think so, no, but it would need double checking (and > mentioning in the description and/or documentation). Originally I wanted to have 'ucode=scan' be the default choice. That would have been the easiest choice. > > > Also, it seems to me that the ucode= specified by > > efi.cfg would take precedence over the ucode=scan. Do not you think? > > I guess we need to settle on what we want to take precedence and > then make sure code also behaves this way. But yes, I think ucode= > from the .cfg should supersede ucode=scan on the command line. A > possibly useful adjustment to this might be to distinguish whether > the ucode=scan was in a specific .cfg section while the ucode= was > in [global] (i.e. sort of a default), in which case maybe the > ucode=scan should win. Thoughts? > > Jan
On 21.01.20 21:51, Eslam Elnikety wrote: > On 21.01.20 10:27, Jan Beulich wrote: >> On 21.01.2020 00:50, Eslam Elnikety wrote: >>> On 20.01.20 09:42, Jan Beulich wrote: >>>> On 17.01.2020 20:06, Eslam Elnikety wrote: >>>>> On 20.12.19 10:53, Jan Beulich wrote: >>>>>> On 19.12.2019 22:08, Eslam Elnikety wrote: >>>>>>> On 18.12.19 12:49, Jan Beulich wrote: >>>>>>>> On 18.12.2019 02:32, Eslam Elnikety wrote: >>>>>>>>> Decouple the microcode referencing mechanism when using GRUB to >>>>>>>>> that >>>>>>>>> when using EFI. This allows us to avoid the "unspecified >>>>>>>>> effect" of >>>>>>>>> using `<integer> | scan` along xen.efi. >>>>>>>> >>>>>>>> I guess "unspecified effect" in the doc was pretty pointless - such >>>>>>>> options have been ignored before; in fact ... >>>>>>>> >>>>>>>>> With that, Xen can explicitly >>>>>>>>> ignore those named options when using EFI. >>>>>>>> >>>>>>>> ... I don't see things becoming any more explicit (not even parsing >>>>>>>> the options was quite explicit to me). >>>>>>>> >>>>>>> >>>>>>> I agree that those options have been ignored so far in the case >>>>>>> of EFI. >>>>>>> The documentation contradicts that however. The documentation: >>>>>>> * says <integer> has unspecified effect. >>>>>>> * does not mention anything about scan being ignored. >>>>>>> >>>>>>> With this patch, it is explicit in code and in documentation that >>>>>>> both >>>>>>> options are ignored in case of EFI. >>>>>> >>>>>> But isn't it rather that ucode=scan could (and hence perhaps should) >>>>>> also have its value on EFI? >>>>>> >>>>> >>>>> I do not see "ucode=scan" applicable in anyway in the case of EFI. In >>>>> EFI, there are not "modules" to scan through, but rather the efi >>>>> config >>>>> points exactly to the microcode blob. >>>> >>>> What would be wrong with the EFI code to also inspect whatever has been >>>> specified with ramdisk= if there was no ucode= ? >>> >>> I see, interesting. This sounds like a legitimate use case indeed. I >>> wonder, would I be breaking anything if I simply allow the existing code >>> that iterates over modules to kick in when ucode=scan irrespective of >>> EFI or legacy boot? >> >> I don't think so, no, but it would need double checking (and >> mentioning in the description and/or documentation). >> >>> Also, it seems to me that the ucode= specified by >>> efi.cfg would take precedence over the ucode=scan. Do not you think? >> >> I guess we need to settle on what we want to take precedence and >> then make sure code also behaves this way. But yes, I think ucode= >> from the .cfg should supersede ucode=scan on the command line. A >> possibly useful adjustment to this might be to distinguish whether >> the ucode=scan was in a specific .cfg section while the ucode= was >> in [global] (i.e. sort of a default), in which case maybe the >> ucode=scan should win. Thoughts? >> >> Jan >> > > I think any ucode= in the EFI .cfg ought to supersede the ucode=scan. > The semantics are simpler in this case, rather than having to worry > about where exactly the ucode= was specified in the EFI .cfg. With that, > an administrator would default to using ucode=scan on the commandline to > load the ramdisk microcode, and a ucode= in .cfg would be an explicit > signal to use different microcode. > > Eslam > So that happens to be the existing behaviour already :) I was under the impression that ucode=scan was simply ignored under EFI. That's not the case. It is only ignored if ucode=<filename> is specified in the EFI config. In other words, what we had just discussed above is already the case. This clearly needs spelling out in the documentation, which is the first patch in the "x86/microcode: Improve documentation and code" series I have sent just now. Cheers, Eslam
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 7a1be84ca9..40faf3bc3a 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -2128,7 +2128,13 @@ logic applies: ### ucode (x86) > `= List of [ <integer> | scan=<bool>, nmi=<bool> ]` -Specify how and where to find CPU microcode update blob. + Applicability: x86 + Default: `nmi` + +Controls for CPU microcode loading. For early loading, this parameter can +specify how and where to find the microcode update blob. For late loading, +this parameter specifies if the update happens within a NMI handler or in +a stop_machine context. 'integer' specifies the CPU microcode update blob module index. When positive, this specifies the n-th module (in the GrUB entry, zero based) to be used @@ -2136,10 +2142,7 @@ for updating CPU micrcode. When negative, counting starts at the end of the modules in the GrUB entry (so with the blob commonly being last, one could specify `ucode=-1`). Note that the value of zero is not valid here (entry zero, i.e. the first module, is always the Dom0 kernel -image). Note further that use of this option has an unspecified effect -when used with xen.efi (there the concept of modules doesn't exist, and -the blob gets specified via the `ucode=<filename>` config file/section -entry; see [EFI configuration file description](efi.html)). +image). 'scan' instructs the hypervisor to scan the multiboot images for an cpio image that contains microcode. Depending on the platform the blob with the @@ -2151,6 +2154,11 @@ microcode in the cpio name space must be: stop_machine context. In NMI handler, even NMIs are blocked, which is considered safer. The default value is `true`. +Note: When booting via EFI, both options 'integer' and 'scan' are ignored. +Here, the concept of modules does not exist. The microcode update blob for +early loading gets specified via the `ucode=<filename>` config file/section +entry; see [EFI configuration file description](efi.html)). + ### unrestricted_guest (Intel) > `= <boolean>` diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c index 6ced293d88..8b4d87782c 100644 --- a/xen/arch/x86/microcode.c +++ b/xen/arch/x86/microcode.c @@ -60,7 +60,7 @@ static module_t __initdata ucode_mod; static signed int __initdata ucode_mod_idx; -static bool_t __initdata ucode_mod_forced; +static signed int __initdata ucode_mod_efi_idx; static unsigned int nr_cores; /* @@ -105,16 +105,10 @@ static struct microcode_patch *microcode_cache; void __init microcode_set_module(unsigned int idx) { - ucode_mod_idx = idx; - ucode_mod_forced = 1; + ucode_mod_efi_idx = idx; } -/* - * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. Both options are - * optional. If the EFI has forced which of the multiboot payloads is to be - * used, only nmi=<bool> is parsed. - */ -static int __init parse_ucode(const char *s) +static int __init parse_ucode_param(const char *s) { const char *ss; int val, rc = 0; @@ -126,18 +120,15 @@ static int __init parse_ucode(const char *s) if ( (val = parse_boolean("nmi", s, ss)) >= 0 ) ucode_in_nmi = val; - else if ( !ucode_mod_forced ) /* Not forced by EFI */ + else if ( (val = parse_boolean("scan", s, ss)) >= 0 ) + ucode_scan = val; + else { - if ( (val = parse_boolean("scan", s, ss)) >= 0 ) - ucode_scan = val; - else - { - const char *q; - - ucode_mod_idx = simple_strtol(s, &q, 0); - if ( q != ss ) - rc = -EINVAL; - } + const char *q; + + ucode_mod_idx = simple_strtol(s, &q, 0); + if ( q != ss ) + rc = -EINVAL; } s = ss + 1; @@ -145,7 +136,7 @@ static int __init parse_ucode(const char *s) return rc; } -custom_param("ucode", parse_ucode); +custom_param("ucode", parse_ucode_param); /* * 8MB ought to be enough. @@ -228,14 +219,18 @@ void __init microcode_grab_module( { module_t *mod = (module_t *)__va(mbi->mods_addr); - if ( ucode_mod_idx < 0 ) + if ( ucode_mod_efi_idx ) /* Microcode specified by EFI */ + { + ucode_mod = mod[ucode_mod_efi_idx]; + return; + } + + if ( ucode_mod_idx < 0 ) /* Count from the end? */ ucode_mod_idx += mbi->mods_count; - if ( ucode_mod_idx <= 0 || ucode_mod_idx >= mbi->mods_count || - !__test_and_clear_bit(ucode_mod_idx, module_map) ) - goto scan; - ucode_mod = mod[ucode_mod_idx]; -scan: - if ( ucode_scan ) + if ( ucode_mod_idx > 0 && ucode_mod_idx < mbi->mods_count && + __test_and_clear_bit(ucode_mod_idx, module_map) ) + ucode_mod = mod[ucode_mod_idx]; + else if ( ucode_scan ) microcode_scan_module(module_map, mbi); }
Decouple the microcode referencing mechanism when using GRUB to that when using EFI. This allows us to avoid the "unspecified effect" of using `<integer> | scan` along xen.efi. With that, Xen can explicitly ignore those named options when using EFI. As an added benefit, we get a straightfoward parsing of the ucode parameter. While at it, simplify the logic in microcode_grab_module(). Update the command line documentation for consistency. Also, drop the leading comment for parse_ucode_param. (No practical use for it given this commit). Signed-off-by: Eslam Elnikety <elnikety@amazon.com> --- docs/misc/xen-command-line.pandoc | 18 ++++++++--- xen/arch/x86/microcode.c | 51 ++++++++++++++----------------- 2 files changed, 36 insertions(+), 33 deletions(-)