Re: CVS commit: src/sys/arch

[Christoph Badura <bad%bsd.de@localhost>]

On Sun, Dec 30, 2018 at 06:42:29PM +0530, Cherry G.Mathew wrote:
> Christoph Badura <bad%bsd.de@localhost> writes:
> > On Tue, Dec 25, 2018 at 11:45:42AM +0530, Cherry G.Mathew wrote:
> >> 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).

I know, and I know that the don't work in your scenario because you need
both versions of the functions.  I was hoping that implicit about
acknowleding your plan to do runtime selection between several versions.

> > 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.

config(8) runs before the kernel is compiled.  And there you can select
what sources get compiled.

> > 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.

I did look.  I think it is pretty straight forward to read.  I had to
look at much more complicated code today.

The only real sore is the "if (!mpacpi_active) {" bit.  I would move
that bit out of the #ifdef and stop worrying.  It is not time critical.
And we waste way more space with aprint_naive/aprint_normal duplication.

It does have the benefit that I have all the conditionals in one place,
so I can look at them and reason about them without having lots of
editor windows open or working out call graphs on paper.

Here we're having a fairly moderately abstract conversation about whether,
essentially,

#if defined(XEN_PHVM)
void *
intr_establish_xname(...)
{
	if (need_native_interrupt)
		return x86_intr_establish_xname(...);
	else
		return xen_intr_establish_xname(...);
}
#endif
intr_establish_xname(...);	/* WTH does intr_establish_xname come
				/* from in the non-PVHWM case? */

or


void *
intr_establish_xname(...)
#if !defined(XEN_PV) && !defined(XEN_PVHVM) 
	if (need_native_interrupt)
		return x86_intr_establish_xname(...);
	else
#else
		return xen_intr_establish_xname(...);
#endif
}

is better.  We're not critically reducing the #ifdef impact either.

However, we are already losing track where intr_establish_xname is
coming from.  IMHO, that does not bode well for this approach.  I believe
Martin is trying to make the same point.

NB, my argument is not about disentangling the code.  Only about the
maintainability aspect of using weak aliases.

--chris