| Message ID | 20240304161041.3465897-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/nospec: Improvements | expand |
Hi Andrew, On 04/03/2024 16:10, Andrew Cooper wrote: > It is daft to require all architectures to provide empty implementations of > this functionality. Oleksii recenlty sent a similar patch [1]. This was pushed back because from naming, it sounds like the helpers ought to be non-empty on every architecture. It would be best if asm-generic provides a safe version of the helpers. So my preference is to not have this patch. This can of course change if I see an explanation why it is empty on Arm (I believe it should contain csdb) and other arch would want the same. Cheers, [1] 5889d7a5fa81722472f95cc1448af0be8f359a7d.1707146506.git.oleksii.kurochko@gmail.com
On 04.03.2024 17:31, Julien Grall wrote: > On 04/03/2024 16:10, Andrew Cooper wrote: >> It is daft to require all architectures to provide empty implementations of >> this functionality. > > Oleksii recenlty sent a similar patch [1]. This was pushed back because > from naming, it sounds like the helpers ought to be non-empty on every > architecture. > > It would be best if asm-generic provides a safe version of the helpers. > So my preference is to not have this patch. This can of course change if > I see an explanation why it is empty on Arm (I believe it should contain > csdb) and other arch would want the same. Except that there's no new asm-generic/ header here (as opposed to how Oleksii had it). Imo avoiding the need for empty stubs is okay this way, when introducing an asm-generic/ header would not have been. Of course if Arm wants to put something there rather sooner than later, then perhaps the functions better wouldn't be removed from there, just to then be put back pretty soon. Jan
On 04.03.2024 17:10, Andrew Cooper wrote: > --- a/xen/arch/x86/include/asm/nospec.h > +++ b/xen/arch/x86/include/asm/nospec.h > @@ -23,20 +23,20 @@ static always_inline bool barrier_nospec_false(void) > return false; > } > > -/* Allow to protect evaluation of conditionals with respect to speculation */ > -static always_inline bool evaluate_nospec(bool condition) > +static always_inline bool arch_evaluate_nospec(bool condition) > { > if ( condition ) > return barrier_nospec_true(); > else > return barrier_nospec_false(); > } > +#define arch_evaluate_nospec arch_evaluate_nospec > > -/* Allow to block speculative execution in generic code */ > -static always_inline void block_speculation(void) > +static always_inline void arch_block_speculation(void) > { > barrier_nospec_true(); > } > +#define arch_block_speculation arch_block_speculation I'm having some difficulty seeing the need for the renaming (adding or the arch_ prefixes): Why can't the original names be kept, and ... > --- a/xen/include/xen/nospec.h > +++ b/xen/include/xen/nospec.h > @@ -9,6 +9,29 @@ > > #include <asm/nospec.h> > > +/* > + * Protect a conditional branch from bad speculation. Architectures *must* > + * provide arch_evaluate_nospec() for this to be effective. > + */ > +static always_inline bool evaluate_nospec(bool cond) > +{ > +#ifndef arch_evaluate_nospec > +#define arch_evaluate_nospec(cond) cond > +#endif > + return arch_evaluate_nospec(cond); > +} > + > +/* > + * Halt speculation unconditonally. Architectures *must* provide > + * arch_block_speculation() for this to be effective. > + */ > +static always_inline void block_speculation(void) > +{ > +#ifdef arch_block_speculation > + arch_block_speculation(); > +#endif > +} ... stubs be introduced here if the original names aren't #define-d? IOW what does the extra layer gain us? Jan
On 04/03/2024 4:45 pm, Jan Beulich wrote: > On 04.03.2024 17:10, Andrew Cooper wrote: >> --- a/xen/arch/x86/include/asm/nospec.h >> +++ b/xen/arch/x86/include/asm/nospec.h >> @@ -23,20 +23,20 @@ static always_inline bool barrier_nospec_false(void) >> return false; >> } >> >> -/* Allow to protect evaluation of conditionals with respect to speculation */ >> -static always_inline bool evaluate_nospec(bool condition) >> +static always_inline bool arch_evaluate_nospec(bool condition) >> { >> if ( condition ) >> return barrier_nospec_true(); >> else >> return barrier_nospec_false(); >> } >> +#define arch_evaluate_nospec arch_evaluate_nospec >> >> -/* Allow to block speculative execution in generic code */ >> -static always_inline void block_speculation(void) >> +static always_inline void arch_block_speculation(void) >> { >> barrier_nospec_true(); >> } >> +#define arch_block_speculation arch_block_speculation > I'm having some difficulty seeing the need for the renaming (adding > or the arch_ prefixes): Why can't the original names be kept, and > ... > >> --- a/xen/include/xen/nospec.h >> +++ b/xen/include/xen/nospec.h >> @@ -9,6 +9,29 @@ >> >> #include <asm/nospec.h> >> >> +/* >> + * Protect a conditional branch from bad speculation. Architectures *must* >> + * provide arch_evaluate_nospec() for this to be effective. >> + */ >> +static always_inline bool evaluate_nospec(bool cond) >> +{ >> +#ifndef arch_evaluate_nospec >> +#define arch_evaluate_nospec(cond) cond >> +#endif >> + return arch_evaluate_nospec(cond); >> +} >> + >> +/* >> + * Halt speculation unconditonally. Architectures *must* provide >> + * arch_block_speculation() for this to be effective. >> + */ >> +static always_inline void block_speculation(void) >> +{ >> +#ifdef arch_block_speculation >> + arch_block_speculation(); >> +#endif >> +} > ... stubs be introduced here if the original names aren't #define-d? > IOW what does the extra layer gain us? See the next patch. ~Andrew
Hi Jan, On 04/03/2024 16:41, Jan Beulich wrote: > On 04.03.2024 17:31, Julien Grall wrote: >> On 04/03/2024 16:10, Andrew Cooper wrote: >>> It is daft to require all architectures to provide empty implementations of >>> this functionality. >> >> Oleksii recenlty sent a similar patch [1]. This was pushed back because >> from naming, it sounds like the helpers ought to be non-empty on every >> architecture. >> >> It would be best if asm-generic provides a safe version of the helpers. >> So my preference is to not have this patch. This can of course change if >> I see an explanation why it is empty on Arm (I believe it should contain >> csdb) and other arch would want the same. > > Except that there's no new asm-generic/ header here (as opposed to how > Oleksii had it). Imo avoiding the need for empty stubs is okay this way, > when introducing an asm-generic/ header would not have been. Of course > if Arm wants to put something there rather sooner than later, then > perhaps the functions better wouldn't be removed from there, just to then > be put back pretty soon. I am confused. I agree the patch is slightly different, but I thought the fundamental problem was the block_speculation() implementation may not be safe everywhere. And it was best to let each architecture decide how they want to implement (vs Xen decide for us the default). Reading the original thread, I thought you had agreed with that statement. Did I misinterpret? Cheers,
On 04/03/2024 4:41 pm, Jan Beulich wrote: > On 04.03.2024 17:31, Julien Grall wrote: >> On 04/03/2024 16:10, Andrew Cooper wrote: >>> It is daft to require all architectures to provide empty implementations of >>> this functionality. >> Oleksii recenlty sent a similar patch [1]. This was pushed back because >> from naming, it sounds like the helpers ought to be non-empty on every >> architecture. >> >> It would be best if asm-generic provides a safe version of the helpers. >> So my preference is to not have this patch. This can of course change if >> I see an explanation why it is empty on Arm (I believe it should contain >> csdb) and other arch would want the same. > Except that there's no new asm-generic/ header here (as opposed to how > Oleksii had it). Imo avoiding the need for empty stubs is okay this way, > when introducing an asm-generic/ header would not have been. Of course > if Arm wants to put something there rather sooner than later, then > perhaps the functions better wouldn't be removed from there, just to then > be put back pretty soon. If the ARM maintainers want Spectre-v1 safety then they can do the work themselves, after this patch has gone in. ~Andrew
On 04.03.2024 17:46, Julien Grall wrote: > On 04/03/2024 16:41, Jan Beulich wrote: >> On 04.03.2024 17:31, Julien Grall wrote: >>> On 04/03/2024 16:10, Andrew Cooper wrote: >>>> It is daft to require all architectures to provide empty implementations of >>>> this functionality. >>> >>> Oleksii recenlty sent a similar patch [1]. This was pushed back because >>> from naming, it sounds like the helpers ought to be non-empty on every >>> architecture. >>> >>> It would be best if asm-generic provides a safe version of the helpers. >>> So my preference is to not have this patch. This can of course change if >>> I see an explanation why it is empty on Arm (I believe it should contain >>> csdb) and other arch would want the same. >> >> Except that there's no new asm-generic/ header here (as opposed to how >> Oleksii had it). Imo avoiding the need for empty stubs is okay this way, >> when introducing an asm-generic/ header would not have been. Of course >> if Arm wants to put something there rather sooner than later, then >> perhaps the functions better wouldn't be removed from there, just to then >> be put back pretty soon. > > I am confused. I agree the patch is slightly different, but I thought > the fundamental problem was the block_speculation() implementation may > not be safe everywhere. And it was best to let each architecture decide > how they want to implement (vs Xen decide for us the default). > > Reading the original thread, I thought you had agreed with that > statement. Did I misinterpret? Yes and no. Whatever is put in asm-generic/ ought to be correct and safe by default, imo. The same doesn't apply to fallbacks put in place in headers in xen/: If an arch doesn't provide its own implementation, it indicates that the default (fallback) is good enough. Still I can easily see that other views are possible here ... Jan
On 04/03/2024 4:55 pm, Jan Beulich wrote: > On 04.03.2024 17:46, Julien Grall wrote: >> On 04/03/2024 16:41, Jan Beulich wrote: >>> On 04.03.2024 17:31, Julien Grall wrote: >>>> On 04/03/2024 16:10, Andrew Cooper wrote: >>>>> It is daft to require all architectures to provide empty implementations of >>>>> this functionality. >>>> Oleksii recenlty sent a similar patch [1]. This was pushed back because >>>> from naming, it sounds like the helpers ought to be non-empty on every >>>> architecture. >>>> >>>> It would be best if asm-generic provides a safe version of the helpers. >>>> So my preference is to not have this patch. This can of course change if >>>> I see an explanation why it is empty on Arm (I believe it should contain >>>> csdb) and other arch would want the same. >>> Except that there's no new asm-generic/ header here (as opposed to how >>> Oleksii had it). Imo avoiding the need for empty stubs is okay this way, >>> when introducing an asm-generic/ header would not have been. Of course >>> if Arm wants to put something there rather sooner than later, then >>> perhaps the functions better wouldn't be removed from there, just to then >>> be put back pretty soon. >> I am confused. I agree the patch is slightly different, but I thought >> the fundamental problem was the block_speculation() implementation may >> not be safe everywhere. And it was best to let each architecture decide >> how they want to implement (vs Xen decide for us the default). >> >> Reading the original thread, I thought you had agreed with that >> statement. Did I misinterpret? > Yes and no. Whatever is put in asm-generic/ ought to be correct and safe > by default, imo. The same doesn't apply to fallbacks put in place in > headers in xen/: If an arch doesn't provide its own implementation, it > indicates that the default (fallback) is good enough. Still I can easily > see that other views are possible here ... With speculation, there's absolutely nothing we can possibly do in any common code which will be safe generally. But we can make it less invasive until an architecture wants to implement the primitives. ~Andrew
Hi Andrew, On 04/03/2024 17:07, Andrew Cooper wrote: > On 04/03/2024 4:55 pm, Jan Beulich wrote: >> On 04.03.2024 17:46, Julien Grall wrote: >>> On 04/03/2024 16:41, Jan Beulich wrote: >>>> On 04.03.2024 17:31, Julien Grall wrote: >>>>> On 04/03/2024 16:10, Andrew Cooper wrote: >>>>>> It is daft to require all architectures to provide empty implementations of >>>>>> this functionality. >>>>> Oleksii recenlty sent a similar patch [1]. This was pushed back because >>>>> from naming, it sounds like the helpers ought to be non-empty on every >>>>> architecture. >>>>> >>>>> It would be best if asm-generic provides a safe version of the helpers. >>>>> So my preference is to not have this patch. This can of course change if >>>>> I see an explanation why it is empty on Arm (I believe it should contain >>>>> csdb) and other arch would want the same. >>>> Except that there's no new asm-generic/ header here (as opposed to how >>>> Oleksii had it). Imo avoiding the need for empty stubs is okay this way, >>>> when introducing an asm-generic/ header would not have been. Of course >>>> if Arm wants to put something there rather sooner than later, then >>>> perhaps the functions better wouldn't be removed from there, just to then >>>> be put back pretty soon. >>> I am confused. I agree the patch is slightly different, but I thought >>> the fundamental problem was the block_speculation() implementation may >>> not be safe everywhere. And it was best to let each architecture decide >>> how they want to implement (vs Xen decide for us the default). >>> >>> Reading the original thread, I thought you had agreed with that >>> statement. Did I misinterpret? >> Yes and no. Whatever is put in asm-generic/ ought to be correct and safe >> by default, imo. The same doesn't apply to fallbacks put in place in >> headers in xen/: If an arch doesn't provide its own implementation, it >> indicates that the default (fallback) is good enough. Still I can easily >> see that other views are possible here ... > > With speculation, there's absolutely nothing we can possibly do in any > common code which will be safe generally. > > But we can make it less invasive until an architecture wants to > implement the primitives. I understand the goal. However, I am unsure it is a good idea to provide unsafe just to reduce the arch specific header by a few lines. My concern is new ports may not realize that block_speculation() needs to be implemented. This could end to a preventable XSA in the future. I guess the risk could be reduced if we had some documentation explaining how to port Xen to a new architecture (I am not asking you to write the doc). Anyway, I understand this is a matter of perspective. I will not oppose this change if you can get others to agree with the risk. Cheers,
On 04/03/2024 5:40 pm, Julien Grall wrote: > Hi Andrew, > > On 04/03/2024 17:07, Andrew Cooper wrote: >> On 04/03/2024 4:55 pm, Jan Beulich wrote: >>> On 04.03.2024 17:46, Julien Grall wrote: >>>> On 04/03/2024 16:41, Jan Beulich wrote: >>>>> On 04.03.2024 17:31, Julien Grall wrote: >>>>>> On 04/03/2024 16:10, Andrew Cooper wrote: >>>>>>> It is daft to require all architectures to provide empty >>>>>>> implementations of >>>>>>> this functionality. >>>>>> Oleksii recenlty sent a similar patch [1]. This was pushed back >>>>>> because >>>>>> from naming, it sounds like the helpers ought to be non-empty on >>>>>> every >>>>>> architecture. >>>>>> >>>>>> It would be best if asm-generic provides a safe version of the >>>>>> helpers. >>>>>> So my preference is to not have this patch. This can of course >>>>>> change if >>>>>> I see an explanation why it is empty on Arm (I believe it should >>>>>> contain >>>>>> csdb) and other arch would want the same. >>>>> Except that there's no new asm-generic/ header here (as opposed to >>>>> how >>>>> Oleksii had it). Imo avoiding the need for empty stubs is okay >>>>> this way, >>>>> when introducing an asm-generic/ header would not have been. Of >>>>> course >>>>> if Arm wants to put something there rather sooner than later, then >>>>> perhaps the functions better wouldn't be removed from there, just >>>>> to then >>>>> be put back pretty soon. >>>> I am confused. I agree the patch is slightly different, but I thought >>>> the fundamental problem was the block_speculation() implementation may >>>> not be safe everywhere. And it was best to let each architecture >>>> decide >>>> how they want to implement (vs Xen decide for us the default). >>>> >>>> Reading the original thread, I thought you had agreed with that >>>> statement. Did I misinterpret? >>> Yes and no. Whatever is put in asm-generic/ ought to be correct and >>> safe >>> by default, imo. The same doesn't apply to fallbacks put in place in >>> headers in xen/: If an arch doesn't provide its own implementation, it >>> indicates that the default (fallback) is good enough. Still I can >>> easily >>> see that other views are possible here ... >> >> With speculation, there's absolutely nothing we can possibly do in any >> common code which will be safe generally. >> >> But we can make it less invasive until an architecture wants to >> implement the primitives. > > I understand the goal. However, I am unsure it is a good idea to > provide unsafe just to reduce the arch specific header by a few lines. It doesn't matter if it's unsafe in the arch header or unsafe in the common code. It's still unsafe. There is no change in risk here. There's simply less blind copy/pasting of the unsafe form into new architectures in order to get Xen to compile. ~Andrew
On 04.03.2024 18:40, Julien Grall wrote: > Hi Andrew, > > On 04/03/2024 17:07, Andrew Cooper wrote: >> On 04/03/2024 4:55 pm, Jan Beulich wrote: >>> On 04.03.2024 17:46, Julien Grall wrote: >>>> On 04/03/2024 16:41, Jan Beulich wrote: >>>>> On 04.03.2024 17:31, Julien Grall wrote: >>>>>> On 04/03/2024 16:10, Andrew Cooper wrote: >>>>>>> It is daft to require all architectures to provide empty implementations of >>>>>>> this functionality. >>>>>> Oleksii recenlty sent a similar patch [1]. This was pushed back because >>>>>> from naming, it sounds like the helpers ought to be non-empty on every >>>>>> architecture. >>>>>> >>>>>> It would be best if asm-generic provides a safe version of the helpers. >>>>>> So my preference is to not have this patch. This can of course change if >>>>>> I see an explanation why it is empty on Arm (I believe it should contain >>>>>> csdb) and other arch would want the same. >>>>> Except that there's no new asm-generic/ header here (as opposed to how >>>>> Oleksii had it). Imo avoiding the need for empty stubs is okay this way, >>>>> when introducing an asm-generic/ header would not have been. Of course >>>>> if Arm wants to put something there rather sooner than later, then >>>>> perhaps the functions better wouldn't be removed from there, just to then >>>>> be put back pretty soon. >>>> I am confused. I agree the patch is slightly different, but I thought >>>> the fundamental problem was the block_speculation() implementation may >>>> not be safe everywhere. And it was best to let each architecture decide >>>> how they want to implement (vs Xen decide for us the default). >>>> >>>> Reading the original thread, I thought you had agreed with that >>>> statement. Did I misinterpret? >>> Yes and no. Whatever is put in asm-generic/ ought to be correct and safe >>> by default, imo. The same doesn't apply to fallbacks put in place in >>> headers in xen/: If an arch doesn't provide its own implementation, it >>> indicates that the default (fallback) is good enough. Still I can easily >>> see that other views are possible here ... >> >> With speculation, there's absolutely nothing we can possibly do in any >> common code which will be safe generally. >> >> But we can make it less invasive until an architecture wants to >> implement the primitives. > > I understand the goal. However, I am unsure it is a good idea to provide > unsafe just to reduce the arch specific header by a few lines. My > concern is new ports may not realize that block_speculation() needs to > be implemented. This could end to a preventable XSA in the future. > > I guess the risk could be reduced if we had some documentation > explaining how to port Xen to a new architecture (I am not asking you to > write the doc). But that's precisely the difference I'm trying to point out between having a stub header in asm-generic/ vs having the fallback in xen/nospec.h: This way an arch still has to supply asm/nospec.h, and hence they can be expected to consider what needs putting there and what can be left to the fallbacks (whether just "for the time being" is a separate question). Whereas allowing to simply point at the asm-generic/ header is (imo) far more likely to have only little thought applied ("oh, there is that generic header, let's just use it"). Yet as said, the line between the two can certainly be viewed as blurred. Jan
On 04.03.2024 17:10, Andrew Cooper wrote: > It is daft to require all architectures to provide empty implementations of > this functionality. > > Provide evaluate_nospec() and block_speculation() unconditionally in > xen/nospec.h with architectures able to opt in by providing suitable arch > variants. > > Rename x86's implementation to the arch_*() variants. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Upon further thinking and with Julien's recent reply in mind: Acked-by: Jan Beulich <jbeulich@suse.com> Still I'd prefer if the arch_* were left out. They look to me to go this half step too far (despite now having looked at patch 2 as well; I'll reply there separately). And it is this which was why I decided that going the other half step (moving these handful lines of code) wasn't really worth it, when considering whether to make such a patch myself. Jan
On 04.03.2024 17:10, Andrew Cooper wrote: > --- a/xen/include/xen/nospec.h > +++ b/xen/include/xen/nospec.h > @@ -9,6 +9,29 @@ > > #include <asm/nospec.h> > > +/* > + * Protect a conditional branch from bad speculation. Architectures *must* > + * provide arch_evaluate_nospec() for this to be effective. > + */ > +static always_inline bool evaluate_nospec(bool cond) > +{ > +#ifndef arch_evaluate_nospec > +#define arch_evaluate_nospec(cond) cond Hmm, noticed only while replying to patch 2: If the #define is to be kept (see my reply there) it needs to be one of #define arch_evaluate_nospec(cond) (cond) or #define arch_evaluate_nospec Or it ought to be #undef-ed after use (thus preventing use in a context where "cond" may expand to other than "cond"). Jan > +#endif > + return arch_evaluate_nospec(cond); > +} > + > +/* > + * Halt speculation unconditonally. Architectures *must* provide > + * arch_block_speculation() for this to be effective. > + */ > +static always_inline void block_speculation(void) > +{ > +#ifdef arch_block_speculation > + arch_block_speculation(); > +#endif > +} > + > /** > * array_index_mask_nospec() - generate a ~0 mask when index < size, 0 otherwise > * @index: array element index
On Mon, 2024-03-04 at 17:50 +0000, Andrew Cooper wrote: > On 04/03/2024 5:40 pm, Julien Grall wrote: > > Hi Andrew, > > > > On 04/03/2024 17:07, Andrew Cooper wrote: > > > On 04/03/2024 4:55 pm, Jan Beulich wrote: > > > > On 04.03.2024 17:46, Julien Grall wrote: > > > > > On 04/03/2024 16:41, Jan Beulich wrote: > > > > > > On 04.03.2024 17:31, Julien Grall wrote: > > > > > > > On 04/03/2024 16:10, Andrew Cooper wrote: > > > > > > > > It is daft to require all architectures to provide > > > > > > > > empty > > > > > > > > implementations of > > > > > > > > this functionality. > > > > > > > Oleksii recenlty sent a similar patch [1]. This was > > > > > > > pushed back > > > > > > > because > > > > > > > from naming, it sounds like the helpers ought to be non- > > > > > > > empty on > > > > > > > every > > > > > > > architecture. > > > > > > > > > > > > > > It would be best if asm-generic provides a safe version > > > > > > > of the > > > > > > > helpers. > > > > > > > So my preference is to not have this patch. This can of > > > > > > > course > > > > > > > change if > > > > > > > I see an explanation why it is empty on Arm (I believe it > > > > > > > should > > > > > > > contain > > > > > > > csdb) and other arch would want the same. > > > > > > Except that there's no new asm-generic/ header here (as > > > > > > opposed to > > > > > > how > > > > > > Oleksii had it). Imo avoiding the need for empty stubs is > > > > > > okay > > > > > > this way, > > > > > > when introducing an asm-generic/ header would not have > > > > > > been. Of > > > > > > course > > > > > > if Arm wants to put something there rather sooner than > > > > > > later, then > > > > > > perhaps the functions better wouldn't be removed from > > > > > > there, just > > > > > > to then > > > > > > be put back pretty soon. > > > > > I am confused. I agree the patch is slightly different, but I > > > > > thought > > > > > the fundamental problem was the block_speculation() > > > > > implementation may > > > > > not be safe everywhere. And it was best to let each > > > > > architecture > > > > > decide > > > > > how they want to implement (vs Xen decide for us the > > > > > default). > > > > > > > > > > Reading the original thread, I thought you had agreed with > > > > > that > > > > > statement. Did I misinterpret? > > > > Yes and no. Whatever is put in asm-generic/ ought to be correct > > > > and > > > > safe > > > > by default, imo. The same doesn't apply to fallbacks put in > > > > place in > > > > headers in xen/: If an arch doesn't provide its own > > > > implementation, it > > > > indicates that the default (fallback) is good enough. Still I > > > > can > > > > easily > > > > see that other views are possible here ... > > > > > > With speculation, there's absolutely nothing we can possibly do > > > in any > > > common code which will be safe generally. > > > > > > But we can make it less invasive until an architecture wants to > > > implement the primitives. > > > > I understand the goal. However, I am unsure it is a good idea to > > provide unsafe just to reduce the arch specific header by a few > > lines. > > It doesn't matter if it's unsafe in the arch header or unsafe in the > common code. It's still unsafe. > > There is no change in risk here. There's simply less blind > copy/pasting > of the unsafe form into new architectures in order to get Xen to > compile. So, we're revisiting this topic again. I agree that upon examining the current state of the code around these functions, it appears safe to provide stubs. However, the reason my patch was rejected is that it may not be entirely safe, as Julien pointed out that even with Arm, some functions shouldn't be empty. What I would like to propose is that it might be beneficial, at least in CONFIG_DEBUG=y, to have a warning message. Does that make sense? ~ Oleksii
On 05.03.2024 16:15, Oleksii wrote: > I agree that upon examining the current state of the code around these > functions, it appears safe to provide stubs. However, the reason my > patch was rejected is that it may not be entirely safe, as Julien > pointed out that even with Arm, some functions shouldn't be empty. > > What I would like to propose is that it might be beneficial, at least > in CONFIG_DEBUG=y, to have a warning message. Does that make sense? A warning message to what effect? And are you thinking of a build-time warning, or a runtime one? Plus wouldn't different aspects quickly lead to proliferation of warnings? Jan
On 05/03/2024 3:43 pm, Jan Beulich wrote: > On 05.03.2024 16:15, Oleksii wrote: >> I agree that upon examining the current state of the code around these >> functions, it appears safe to provide stubs. However, the reason my >> patch was rejected is that it may not be entirely safe, as Julien >> pointed out that even with Arm, some functions shouldn't be empty. >> >> What I would like to propose is that it might be beneficial, at least >> in CONFIG_DEBUG=y, to have a warning message. Does that make sense? > A warning message to what effect? And are you thinking of a build-time > warning, or a runtime one? Plus wouldn't different aspects quickly lead > to proliferation of warnings? Putting in a warning for something like this is specifically a bad idea. All it will do is cause people to ignore warnings, and that's the very worst thing Xen could do. ~Andrew
On Tue, 2024-03-05 at 16:43 +0100, Jan Beulich wrote: > On 05.03.2024 16:15, Oleksii wrote: > > I agree that upon examining the current state of the code around > > these > > functions, it appears safe to provide stubs. However, the reason my > > patch was rejected is that it may not be entirely safe, as Julien > > pointed out that even with Arm, some functions shouldn't be empty. > > > > What I would like to propose is that it might be beneficial, at > > least > > in CONFIG_DEBUG=y, to have a warning message. Does that make sense? > > A warning message to what effect? And are you thinking of a build- > time > warning, or a runtime one? Plus wouldn't different aspects quickly > lead > to proliferation of warnings? A warning message about that an empty definition is provided for evaluate_nospec/block_speculation functions, so a developer will know that it can be an issue. Personally, I am OK with having this function empty by default as it is done in the patch with opportunity to being redefined in arch specific code if it is necessary, but I remembered that I had the similar questions in my patch series which probably should be covered in this patch series. Runtime message can also be an option, but I thought about build-time, but I agree that it can lead to proliferation of warning, so not the best one idea. ~ Oleksii
diff --git a/xen/arch/arm/include/asm/nospec.h b/xen/arch/arm/include/asm/nospec.h index efac51fc03be..05df096faab0 100644 --- a/xen/arch/arm/include/asm/nospec.h +++ b/xen/arch/arm/include/asm/nospec.h @@ -12,15 +12,6 @@ # error "unknown ARM variant" #endif -static inline bool evaluate_nospec(bool condition) -{ - return condition; -} - -static inline void block_speculation(void) -{ -} - #endif /* _ASM_ARM_NOSPEC_H */ /* diff --git a/xen/arch/ppc/include/asm/nospec.h b/xen/arch/ppc/include/asm/nospec.h index b97322e48d32..9b57a7e4b24d 100644 --- a/xen/arch/ppc/include/asm/nospec.h +++ b/xen/arch/ppc/include/asm/nospec.h @@ -3,13 +3,4 @@ #ifndef __ASM_PPC_NOSPEC_H__ #define __ASM_PPC_NOSPEC_H__ -static inline bool evaluate_nospec(bool condition) -{ - return condition; -} - -static inline void block_speculation(void) -{ -} - #endif /* __ASM_PPC_NOSPEC_H__ */ diff --git a/xen/arch/riscv/include/asm/nospec.h b/xen/arch/riscv/include/asm/nospec.h index e30f0a781b68..b227fc61ed8b 100644 --- a/xen/arch/riscv/include/asm/nospec.h +++ b/xen/arch/riscv/include/asm/nospec.h @@ -4,15 +4,6 @@ #ifndef _ASM_RISCV_NOSPEC_H #define _ASM_RISCV_NOSPEC_H -static inline bool evaluate_nospec(bool condition) -{ - return condition; -} - -static inline void block_speculation(void) -{ -} - #endif /* _ASM_RISCV_NOSPEC_H */ /* diff --git a/xen/arch/x86/include/asm/nospec.h b/xen/arch/x86/include/asm/nospec.h index 07606834c4c9..defc97707f03 100644 --- a/xen/arch/x86/include/asm/nospec.h +++ b/xen/arch/x86/include/asm/nospec.h @@ -23,20 +23,20 @@ static always_inline bool barrier_nospec_false(void) return false; } -/* Allow to protect evaluation of conditionals with respect to speculation */ -static always_inline bool evaluate_nospec(bool condition) +static always_inline bool arch_evaluate_nospec(bool condition) { if ( condition ) return barrier_nospec_true(); else return barrier_nospec_false(); } +#define arch_evaluate_nospec arch_evaluate_nospec -/* Allow to block speculative execution in generic code */ -static always_inline void block_speculation(void) +static always_inline void arch_block_speculation(void) { barrier_nospec_true(); } +#define arch_block_speculation arch_block_speculation /** * array_index_mask_nospec() - generate a mask that is ~0UL when the diff --git a/xen/include/xen/nospec.h b/xen/include/xen/nospec.h index 4c250ebbd663..a4155af08770 100644 --- a/xen/include/xen/nospec.h +++ b/xen/include/xen/nospec.h @@ -9,6 +9,29 @@ #include <asm/nospec.h> +/* + * Protect a conditional branch from bad speculation. Architectures *must* + * provide arch_evaluate_nospec() for this to be effective. + */ +static always_inline bool evaluate_nospec(bool cond) +{ +#ifndef arch_evaluate_nospec +#define arch_evaluate_nospec(cond) cond +#endif + return arch_evaluate_nospec(cond); +} + +/* + * Halt speculation unconditonally. Architectures *must* provide + * arch_block_speculation() for this to be effective. + */ +static always_inline void block_speculation(void) +{ +#ifdef arch_block_speculation + arch_block_speculation(); +#endif +} + /** * array_index_mask_nospec() - generate a ~0 mask when index < size, 0 otherwise * @index: array element index
It is daft to require all architectures to provide empty implementations of this functionality. Provide evaluate_nospec() and block_speculation() unconditionally in xen/nospec.h with architectures able to opt in by providing suitable arch variants. Rename x86's implementation to the arch_*() variants. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Michal Orzel <michal.orzel@amd.com> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com> CC: Shawn Anastasio <sanastasio@raptorengineering.com> --- xen/arch/arm/include/asm/nospec.h | 9 --------- xen/arch/ppc/include/asm/nospec.h | 9 --------- xen/arch/riscv/include/asm/nospec.h | 9 --------- xen/arch/x86/include/asm/nospec.h | 8 ++++---- xen/include/xen/nospec.h | 23 +++++++++++++++++++++++ 5 files changed, 27 insertions(+), 31 deletions(-)