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
