On Sun, Dec 30, 2018 at 06:42:29PM +0530, Cherry G.Mathew wrote:
> Christoph Badura <b...@bsd.de> 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
