Re: [Xen-devel] Re: Paravirtualizing bits of acpi access

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Saturday 28 March 2009, Jeremy Fitzhardinge wrote:
> Len Brown wrote:
> >>> Jeremy, I'm not excited about a proposed change to acpixf.h --
> >>> this is the API to ACPICA...
> >>>   
> >>>       
> >> Do you have an issue with the mechanism (using weak function, etc), or just
> >> the placement of the prototypes in that header?  Would there be a better
> >> header to put them in?  Or would you prefer some other mechanism?
> >>
> >> It certainly seems like Xen and tboot should be able to share the same hook,
> >> given that they're doing similar things for similar reasons.
> >>
> >> (I don't really understand the structure of all the acpi stuff; I'm just
> >> wading in and making a mess of things until I can close the lid of laptop
> >> successfully.)
> >>     
> >
> > Everything in acpi/acpica/ is source code that we share with BSD
> > via the ACPICA project http://acpica.org/
> >
> > ACPICA also supplies a couple of the headers outside that directory,
> > eg. acpixf.h, which is the API between the kernel and ACPICA.
> >
> > We can, and do, change that API when it makes sense.
> > However, we want to think carefully before changing it,
> > for we either cause Linux to diverge, or we have to sell
> > the same change to several other operating systems.
> > So ideally we wouuld need to make no Linux-specific, or Xen-specific
> > changes in that directory.
> >
> > One possibility is to have this called via
> > function pointer from ASM and scribble over the default
> > function pointer for the Xen case.  In that case, the Xen
> > version of the routine would live someplace other than acpi/acpica/ -
> > someplace with the word xen in the path.
> 
> Yes, that would be easy enough to do; we could overwrite it only when 
> actually running under Xen.
> 
> However, we don't need to replace the whole of acpi_enter_sleep_state(); 
> there are two options: we could duplicate the early part of the function 
> in the Xen version of it, or break just the differing part out via 
> function pointer.  The former has the disadvantage of duplicating code, 
> but it does allow the same function pointer to be used by the tboot version.
> 
> >   If using _weak can effectively
> > do that at link time, then fine, if we can do it w/o changing the API --
> > a _weak annotation is an easy ACPICA/Linux divergencen to manage.
> >   
> 
> The weak approach is what my proposed patch does:
> 
>     * the default native-hardware version of the sleep-entry register
>       writes is broken out into default_acpi_enter_sleep_state()
>     * it introduces a weak arch_acpi_enter_sleep_state() which just
>       calls the default case, leaving the normal function unchanged
>     * in arch/x86/kernel/acpi/sleep.c, it adds an override
>       arch_acpi_enter_sleep_state(), which checks to see if we're
>       running under Xen; if not, then it simply calls
>       default_acpi_enter_sleep_state() as usual; if it does, it calls
>       xen_acpi_enter_sleep_state()
>     * xen_acpi_enter_sleep-state() is defined in arch/x86/xen/acpi.c
> 
> (Actually it didn't break the Xen version out separately, but it easily 
> could.)
> 
> On the whole, using a function pointer would be a bit cleaner, as it 
> removes the need for the weak glue functions, at the cost of some 
> (possible) code duplication.
> 
> > I don't know if Xen and TXT are exclusive, or if we'd ever need
> > to handle both cases at the same time.  I guess that will influence
> > if the TXT and Xen use the same approach or something different.
> >   
> 
> As Kevin said, they're exclusive, so they could share the same function 
> pointer.

FWIW, if you only care about suspen to RAM (S3). I'm still thinking that
do_suspend_lowlevel() is the place to work on.  After all
acpi_enter_sleep_state() is called from there.

Thanks,
Rafael