Re: [Xen-devel] Is: Linux-EFI code to call Xen's EFI hypercalls [RFC PATCH comments] Was:Re: Xen 4.4 development update

[Eric Shelton <eshelton@xxxxxxxxx>]

On Thu, Aug 15, 2013 at 10:12 AM, Konrad Rzeszutek Wilk
<konrad.wilk@xxxxxxxxxx> wrote:
> On Thu, Aug 15, 2013 at 01:32:12AM -0400, Eric Shelton wrote:
>
> First of, thank you for taking a stab at it and redoing it.

Sure.  The first releases of the patch were posted during a hectic
time in the release cycle.  Three months later, I wanted to make sure
this had not gotten lost in the noise, and instead got wrapped up
while it was still fairly fresh for me.

> Some general comments:
>  - the #ifdef CONFIG_XEN is generaly frowend upon. If you need them they 
> should
>    be in header files. The exception is the CONFIG_X86 and
>    CONFIG_X86_64. Now said that there are other instances of code where
>    it is sprinkled with #ifdef CONFIG_ACPI .. #ifdef CONFIG_PCI, and so
>    on. It would have been nice if all of that got moved to a header file
>    but in reality that could make it just more confusing. Enough about
>    that - what I want to say is that doing #idef CONFIG_XEN is something
>    we have been trying the utmost to not put in generic code. The
>    reasoning is that instead of using the API (so for example if we have
>    an generic layer and then there are platform related drivers - we
>    want to implement the stuff in the platform drivers - not add
>    side-channels for a specific platform).

OK.  Hopefully the reorganization I suggest below will clear out most
or all of this.

>  - Is there any way to move the bulk of the hypercalls in the
>    efi_runtime services. Meaning alter the efi_runtime services
>    to do hypercalls instead of EFI calls? That way I would think most
>    of the EFI general logic would be untouched? This is going back to
>    the first comment - you would want to leave as much generic code
>    untouched as possible. And implement the bulk of the code in the
>    platform specific code.

This sounds similar to Matthew Garrett's suggestion "to do this by
replacing efi_call_*  I'm not quite sure how I would "alter the
efi_runtime services" to accomplish this - or at least not in some way
that is not worse than what is being done for things like
*_set_variable().  If you can more concretely show me how this might
be coded for one of the runtime service functions, I would be happy to
replicate that across the patch.

Rather than doing that,I would suggest, as least as far as the runtime
services are concerned, that the architecture/implementation specific
code has been narrowly encapsulated - in the virt_efi_* functions in
efi.c, and in the corresponding xen_efi_* functions in xen.c.  These
are registered in the function table included in the 'efi' struct and
called through there, and are essentially setter/getter functions
without any general logic.  There are higher level functions, such as
efi_set_rtc_mmss(), and they use the function table to get/set EFI
info as needed, and have not been duplicated.  The runtime services
look pretty cleanly implemented in this way, and introduce essentially
no changes since this general mechanism was already in place.  The
only improvement I might suggest is to break out the runtime services
function pointers into a separate struct, perhaps named efi_runtime,
as it would make it a bit clearer they are runtime services accessor
methods.

The remaining functions are for initialization, and these are where
the "if efi_enabled(EFI_XEN)" and "#ifdef CONFIG_XEN" stuff crept in.
Many of the differences arise from the fact that much of what the
kernel originally did for EFI init has been moved into the hypervisor,
and the information is accessed via hypercalls.  As a result, xen.c
has much less to do than efi.c, which has to concern itself with
memory mapping of the system and runtime services tables.  Where dom0
is booting under Xen, various init functions were changed to return
immediately, as this extra work does not need to be done.

It sounds like I should return to what the original patches were
doing, and make use of a function table for the init functions, much
as done for the runtime services.  However, as function pointers were
not in the original code, this looked a bit uglier in the first
patches, because each original efi_*_init function ended up with a
counterpart efi_*_init_generic function, which did "if (func_ptr ==
NULL) { return; } else { func_ptr(args); }", with most values being
NULL in xen.c (probably because many table entries were only called
internally in efi.c).  I can make this a little less ugly by (1)
minimizing the functions in the table, and (2) making table.func_ptr()
calls instead of *_generic() calls.

Additionally it might be sensible to split up efi.c into efi_runtime.c
and efi_init.c to separately bundle the functions used during these
two phases of operation.  The maintainer for the linux kernel would
have to rule on that idea...

>  - When approaching Matthew I would suggest you label it as RFC - to get
>    an idea of how he would like it done - as I don't think he will
>    take the patch as is. He might suggest the thing I just mentioned
>    above. Or perhaps make most of the 'efi_*' calls go through some form
>    of function table that by default is set to baremetal_efi_calls
>    (see x86_init.c)

See above.  This appears to already be happening for the runtime
services calls.  The problems appear to be with init code, which I
will try to resolve with a similar function table-based approach.

>  - There are some code paths that just do:
>         if (xen)
>                 return;
>    But it is not clear why that is so (missing comments). This is needed
>    b/c if somebody wants to redo the code in the future they would need to
>    know where to place that 'if (xen) do something different'

The course laid out above will remove this code, in favor of the
additional function table.

>  - You want to credit Jan Beulich in the commit description saying
>    that the patch was derieved from the work that he did for SuSE Linux.
>    Liang Tang took some of that - but he forgot to mention that in the
>    initial post.

Definitely.  If the hypercalls and original patches had not been
around, I likely would not have gone very far with this, and instead
stuck with the weird legacy BIOS boot mechanisms provided by Apple
(mainly for Bootcamp).

>  - It is unclear to me why the machine_ops assigment has been changed?

Sorry, that was a result of work with a C++ codebase that generally
disfavored object-to-object assignments.  The original
struct-to-struct assignment was legitimate, and will be restored.

> Thank you again for taking a stab at it! I know that getting push back
> on patches is not a thing that anybody likes - but the end goal is make
> it be fantastic and sometimes that takes a couple of rounds (or about a
> dozen as my first patches for Linux took that many revisions)

No problem on the comments.  This is mainly about a cleaner and
clearer presentation on the code.  I appreciate having an express
indication of the coding philosophies and design patterns for both Xen
and the Linux kernel, especially if it smooths the path through LKML.

- Eric

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel