Re: CVS commit: src/sys/arch

[Cherry G.Mathew <cherry%zyx.in@localhost>]

Christoph Badura <bad%bsd.de@localhost> writes:

> On Tue, Dec 25, 2018 at 11:45:42AM +0530, Cherry G.Mathew wrote:
>> Joerg Sonnenberger <joerg%bec.de@localhost> writes:
>> > On Sat, Dec 22, 2018 at 09:27:22PM +0000, Cherry G. Mathew wrote:
>> >> Modified Files:
>> >> 	src/sys/arch/amd64/amd64: cpufunc.S
>> >> 	src/sys/arch/i386/i386: cpufunc.S i386func.S
>> >> 	src/sys/arch/xen/x86: xenfunc.c
>> >> Log Message:
>> >> Introduce a weak alias method of exporting different implementations
>> >> of the same API.
>> > Why? As in: what's the advantage of making them weak.
>> I'd posted separately before this commit explaining the rationale.
>
> It took me a while to check the most obvious places and figure out which
> message it might have been.  A more precise reference would have helped.
>
>> Here's the scenario above:
>> 
>> let's take lcr3(). On native this is a ring3 operation, implemented in
>> assembler. On xen, this is a PV call. Currently there's no need to have
>> both implementations in a single binary, since PV and native binaries
>> are distinct. With PVHVM, we have a situation where at boot time, the
>> native version is required, and after XEN is detected, we can use the
>> XEN version of the call. I've taken the approach of naming the
>> individual functions distinctly, eg: i386_lcr3(), or xen_lcr3(), while
>> and exporting them weakly as the consumed version, ie; lcr3().
>> 
>> What happens is that when the individual binary is compiled, the
>> appropriate weakly exported symbol takes over, and things work as
>> expected. When  the combined binary for PVHVM is required, we write a
>> strongly exported "switch" function, called lcr3() (I haven't committed
>> this yet) which overrides both the weak symbols. It can then do the
>> runtime calls by calling into the appropriate i386_lcr3() or xen_lcr3(),
>> depending on the mode in which it is running.
>
> I don't find this argument for weak aliases convincing.  You plan to
> write the function that makes a runtime decision between the
> implemenation anyway.  You might as well write that function now and avoid
> another bit of hidden magic.
>

The other options have been suggested earlier (function pointers and
hotpatching).

Function pointers require reprototyping every function required (I don't
have an exhaustive list yet). Hotpatching isn't useful in that it will
overwrite the default version, so we'll have to hotpatch twice. Once to
override the default, and then to make a copy so that the default is
available. It's also ugly, so for me this is the least preferable
method.

> You can have multiple versions of that "switch" function and select the
> appropriate one with config(8) logic.  

It's too late for that. Things like lcr3() need to work way before
config(8) is a twinkle in boot(8)s eyes.

> Or you can select with #ifdef.  Somewhere you have to place the logic
> for conditional compilation/linking.  Having that logic concentrated
> in a single place instead of N seems preferable to me.

Yes, it's pretty intense - see x86/mainbus.c:mainbus_attach() for a
sample of what is to come.

-- 
~cherry