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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Thu, Aug 15, 2013 at 04:24:20PM -0400, Eric Shelton wrote:
> 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.

Ha! I am just hand-waving right now :-)

What I had in mind was that this:

         efi_phys.systab = (efi_system_table_t 
*)boot_params.efi_info.efi_systab;

Is done, and we could implement some stub functions in the efi_systab
that would convert the Microsoft stack passing to normal Linux
and then do the hypercalls.

It would be a bit of this:

virt_efi_get_time -> calls efi_call_virt2 (efi_stub_32|64.S) ->
assembler code Linux to EFI stacks, and calls in
        efi.systab->runtime->get_time

The get_time would be our own little blob of code that alters the
parameters from EFI stack to Linux and makes the hypercall (so probably
in C). Then when the hypercall is done, it changes the stack back to EFI
and returns.

In other words we would implement an EFI runtime code that actually
would do hypercalls.

But from your analysis that does not solve the whole problem. Those
efi_init* variants in some cases are unneccessary. Which brings another
question - if we do barell throught them, what is the worst that will
happen? Can the values returned be the same?

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

Ok, so you are hoisting this a bit higher. That would work too - and
it would be less messier. I think getting Matthew's thoughts on this
is neccessary - as at the end of this it is his call.
> 
> 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.

If we prepapred the efi structures (hand-waving here) with the data that
the hypervisor has already setup and give that to this code - would that
work? Basically it would (I hope) just walk throught the same steps
that the hypervisor did, but all of this efi calls would be stubbed out.
For example the E820 - we already do some of that in setup.c so I would
think this could just prepare an memmap that has no entries.

Or get the entries from the XENMEM_machine_memory_map like in
arch/x86/xen/setup.c and just prepare the memmap.. Thought there is a
bunch of goodness happening in xen_memory_setup() that should still
be invoked.
> 
> 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

The original ones were much larger and the maintainer (Matthew) was
not too thrilled about them.

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

Yeah, it comes back to how Matthew would like it. And I need to do some
more homework and look more dilligiently at the efi.c code. Next week
should be able to do that.
> 
> >  - 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.

Yeeey!
> 
> - Eric

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