| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 20 May 2014 13:29:33 +0200
From: Daniel Kiper <daniel.kiper@...cle.com>
To: David Vrabel <david.vrabel@...rix.com>
Cc: linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
x86@...nel.org, xen-devel@...ts.xenproject.org,
boris.ostrovsky@...cle.com, eshelton@...ox.com, hpa@...or.com,
ian.campbell@...rix.com, jbeulich@...e.com, jeremy@...p.org,
konrad.wilk@...cle.com, matt.fleming@...el.com, mingo@...hat.com,
mjg59@...f.ucam.org, stefano.stabellini@...citrix.com,
tglx@...utronix.de
Subject: Re: [PATCH v4 3/5] xen: Put EFI machinery in place
On Tue, May 20, 2014 at 10:47:00AM +0100, David Vrabel wrote:
> On 16/05/14 21:41, Daniel Kiper wrote:
> > Put EFI machinery for Xen in place.
>
> Put what machinery to do what?
>
> > @@ -1714,6 +1725,21 @@ asmlinkage __visible void __init xen_start_kernel(void)
> >
> > xen_setup_runstate_info(0);
> >
> > + efi_systab_xen = xen_efi_probe();
> > +
> > + if (efi_systab_xen) {
> > + strncpy((char *)&boot_params.efi_info.efi_loader_signature, "Xen",
> > + sizeof(boot_params.efi_info.efi_loader_signature));
> > + boot_params.efi_info.efi_systab = (__u32)((__u64)efi_systab_xen);
> > + boot_params.efi_info.efi_systab_hi = (__u32)((__u64)efi_systab_xen >> 32);
> > +
> > + x86_platform.get_wallclock = efi_get_time;
>
> x86_platform.get_wallclock should always be xen_get_wallclock().
Hmmm... Make sens... Jan, why did you replace x86_platform.get_wallclock
with efi_get_time() in your implementation?
> > + x86_platform.set_wallclock = efi_set_rtc_mmss;
Now I am not sure even about that. As I can see Linux Kernel
running on bare metal EFI platform directly sets RTC omitting
efi_set_rtc_mmss(). Am I missing something? IIRC, there was
discussion about that once. But where and when?
Anyway, now I think that this initialization should be done
in arch/x86/xen/time.c if needed.
> > +
> > + set_bit(EFI_BOOT, &efi.flags);
> > + set_bit(EFI_64BIT, &efi.flags);
> > + }
> > +
> > /* Start the world */
> > #ifdef CONFIG_X86_32
> > i386_start_kernel();
> > --- /dev/null
> > +++ b/drivers/xen/efi.c
> > @@ -0,0 +1,374 @@
> > + * Copyright (c) 2014 Daniel Kiper, Oracle Corporation
>
> Is this really copyright by you personally and not Oracle?
As I know it is correct but I will double check it.
>
>
> > +#define call (op.u.efi_runtime_call)
> > +#define DECLARE_CALL(what) \
> > + struct xen_platform_op op; \
> > + op.cmd = XENPF_efi_runtime_call; \
> > + call.function = XEN_EFI_##what; \
> > + call.misc = 0
>
> Macros that declare local variables are awful.
>
> Use what Andrew suggested and something like
>
> struct xen_blah *call = &op.u.efi_runtime_call;
>
>
> > +static const struct efi efi_xen __initconst = {
> > + .systab = NULL, /* Initialized later. */
> > + .runtime_version = 0, /* Initialized later. */
> > + .mps = EFI_INVALID_TABLE_ADDR,
> > + .acpi = EFI_INVALID_TABLE_ADDR,
> > + .acpi20 = EFI_INVALID_TABLE_ADDR,
> > + .smbios = EFI_INVALID_TABLE_ADDR,
> > + .sal_systab = EFI_INVALID_TABLE_ADDR,
> > + .boot_info = EFI_INVALID_TABLE_ADDR,
> > + .hcdp = EFI_INVALID_TABLE_ADDR,
> > + .uga = EFI_INVALID_TABLE_ADDR,
> > + .uv_systab = EFI_INVALID_TABLE_ADDR,
> > + .fw_vendor = EFI_INVALID_TABLE_ADDR,
> > + .runtime = EFI_INVALID_TABLE_ADDR,
> > + .config_table = EFI_INVALID_TABLE_ADDR,
> > + .get_time = xen_efi_get_time,
> > + .set_time = xen_efi_set_time,
> > + .get_wakeup_time = xen_efi_get_wakeup_time,
> > + .set_wakeup_time = xen_efi_set_wakeup_time,
> > + .get_variable = xen_efi_get_variable,
> > + .get_next_variable = xen_efi_get_next_variable,
> > + .set_variable = xen_efi_set_variable,
> > + .query_variable_info = xen_efi_query_variable_info,
> > + .update_capsule = xen_efi_update_capsule,
> > + .query_capsule_caps = xen_efi_query_capsule_caps,
> > + .get_next_high_mono_count = xen_efi_get_next_high_mono_count,
> > + .reset_system = NULL, /* Functionality provided by Xen. */
>
> Xen provides functionality to reset (just maybe not via an EFI call).
> Should an implementation be provided that does this?
I do not think so. efi.reset_system() would not be called on Xen because
all reset related stuff is replaced by Xen reset specific functions.
Please check arch/x86/xen/enlighten.c. Additionally, I think that
situation is a bit similar to time issue. If we are running on Xen
then we should ask Xen to do reset because Xen controls platform
and it knows how to do that.
> > + .set_virtual_address_map = NULL, /* Not used under Xen. */
> > + .memmap = NULL, /* Not used under Xen. */
> > + .flags = 0 /* Initialized later. */
> > +};
> > +
> > +efi_system_table_t __init *xen_efi_probe(void)
> > +{
> > + struct xen_platform_op op = {
> > + .cmd = XENPF_firmware_info,
> > + .u.firmware_info = {
> > + .type = XEN_FW_EFI_INFO,
> > + .index = XEN_FW_EFI_CONFIG_TABLE
> > + }
> > + };
> > + union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> > +
> > + if (!xen_initial_domain() || HYPERVISOR_dom0_op(&op))
> > + return NULL;
>
> if (!xen_initial_domain())
> return NULL;
>
> if (HYPERVISOR_dom0_op(&op) < 0)
> return NULL;
What is wrong with my if?
> > +
> > + /* Here we know that Xen runs on EFI platform. */
> > +
> > + efi = efi_xen;
> > +
> > + op.cmd = XENPF_firmware_info;
> > + op.u.firmware_info.type = XEN_FW_EFI_INFO;
> > + op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
> > + info->vendor.bufsz = sizeof(vendor);
> > + set_xen_guest_handle(info->vendor.name, vendor);
> > +
> > + if (!HYPERVISOR_dom0_op(&op)) {
>
> if (HYPERVISOR_dom0_op(&op) == 0)
Ditto?
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists