[PATCH v2 2/2] EFI/BGRT: use efi_mem_type()

[Jan Beulich]

Avoid effectively open-coding the function.

Signed-off-by: Jan Beulich <jbeul...@suse.com>
Reviewed-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
---
 drivers/firmware/efi/efi-bgrt.c |   22 +-
 1 file changed, 1 insertion(+), 21 deletions(-)

--- 4.13-rc6-EFI.orig/drivers/firmware/efi/efi-bgrt.c
+++ 4.13-rc6-EFI/drivers/firmware/efi/efi-bgrt.c
@@ -27,26 +27,6 @@ struct bmp_header {
u32 size;
 } __packed;
 
-static bool efi_bgrt_addr_valid(u64 addr)
-{
-   efi_memory_desc_t *md;
-
-   for_each_efi_memory_desc(md) {
-   u64 size;
-   u64 end;
-
-   if (md->type != EFI_BOOT_SERVICES_DATA)
-   continue;
-
-   size = md->num_pages << EFI_PAGE_SHIFT;
-   end = md->phys_addr + size;
-   if (addr >= md->phys_addr && addr < end)
-   return true;
-   }
-
-   return false;
-}
-
 void __init efi_bgrt_init(struct acpi_table_header *table)
 {
void *image;
@@ -85,7 +65,7 @@ void __init efi_bgrt_init(struct acpi_ta
goto out;
}
 
-   if (!efi_bgrt_addr_valid(bgrt->image_address)) {
+   if (efi_mem_type(bgrt->image_address) != EFI_BOOT_SERVICES_DATA) {
pr_notice("Ignoring BGRT: invalid image address\n");
goto out;
}



--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] EFI: move efi_mem_type() to common code

[Jan Beulich]

This follows efi_mem_attributes(), as it's similarly generic. Drop
__weak from that one though (and don't introduce it for efi_mem_type()
in the first place) to make clear that other overrides to these
functions are really not intended.

Signed-off-by: Jan Beulich <jbeul...@suse.com>
---
v2: Drop __weak as requested.
---
 arch/x86/platform/efi/efi.c |   19 ---
 drivers/firmware/efi/efi.c  |   29 +
 2 files changed, 29 insertions(+), 19 deletions(-)

--- 4.13-rc6-EFI.orig/arch/x86/platform/efi/efi.c   2017-08-23 
08:25:56.360254268 +0200
+++ 4.13-rc6-EFI/arch/x86/platform/efi/efi.c2017-08-24 10:46:34.580986480 
+0200
@@ -1032,25 +1032,6 @@ void __init efi_enter_virtual_mode(void)
efi_dump_pagetable();
 }
 
-/*
- * Convenience functions to obtain memory types and attributes
- */
-u32 efi_mem_type(unsigned long phys_addr)
-{
-   efi_memory_desc_t *md;
-
-   if (!efi_enabled(EFI_MEMMAP))
-   return 0;
-
-   for_each_efi_memory_desc(md) {
-   if ((md->phys_addr <= phys_addr) &&
-   (phys_addr < (md->phys_addr +
- (md->num_pages << EFI_PAGE_SHIFT
-   return md->type;
-   }
-   return 0;
-}
-
 static int __init arch_parse_efi_cmdline(char *str)
 {
if (!str) {
--- 4.13-rc6-EFI.orig/drivers/firmware/efi/efi.c2017-08-23 
08:25:59.792278686 +0200
+++ 4.13-rc6-EFI/drivers/firmware/efi/efi.c 2017-08-25 10:10:34.950714797 
+0200
@@ -791,19 +791,19 @@ char * __init efi_md_typeattr_format(cha
 }
 
 /*
+ * IA64 has a funky EFI memory map that doesn't work the same way as
+ * other architectures.
+ */
+#ifndef CONFIG_IA64
+/*
  * efi_mem_attributes - lookup memmap attributes for physical address
  * @phys_addr: the physical address to lookup
  *
  * Search in the EFI memory map for the region covering
  * @phys_addr. Returns the EFI memory attributes if the region
  * was found in the memory map, 0 otherwise.
- *
- * Despite being marked __weak, most architectures should *not*
- * override this function. It is __weak solely for the benefit
- * of ia64 which has a funky EFI memory map that doesn't work
- * the same way as other architectures.
  */
-u64 __weak efi_mem_attributes(unsigned long phys_addr)
+u64 efi_mem_attributes(unsigned long phys_addr)
 {
efi_memory_desc_t *md;
 
@@ -819,6 +819,31 @@ u64 __weak efi_mem_attributes(unsigned l
return 0;
 }
 
+/*
+ * efi_mem_type - lookup memmap type for physical address
+ * @phys_addr: the physical address to lookup
+ *
+ * Search in the EFI memory map for the region covering @phys_addr.
+ * Returns the EFI memory type if the region was found in the memory
+ * map, EFI_RESERVED_TYPE (zero) otherwise.
+ */
+u32 efi_mem_type(unsigned long phys_addr)
+{
+   const efi_memory_desc_t *md;
+
+   if (!efi_enabled(EFI_MEMMAP))
+   return 0;
+
+   for_each_efi_memory_desc(md) {
+   if ((md->phys_addr <= phys_addr) &&
+   (phys_addr < (md->phys_addr +
+ (md->num_pages << EFI_PAGE_SHIFT
+   return md->type;
+   }
+   return EFI_RESERVED_TYPE;
+}
+#endif
+
 int efi_status_to_err(efi_status_t status)
 {
int err;



--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] EFI: move efi_mem_type() to common code

[Jan Beulich]

>>> On 24.08.17 at 12:19, <ard.biesheu...@linaro.org> wrote:
> On 24 August 2017 at 11:11, Jan Beulich <jbeul...@suse.com> wrote:
>>>>> On 24.08.17 at 11:52, <ard.biesheu...@linaro.org> wrote:
>>> If it already has its own version, I'd prefer we just add #ifndef
>>> CONFIG_IA64 around it instead.
>>
>> Which would then preclude other environments to override it.
> 
> ... which is what the comment says in the first place.
> 
>> I have such a case in an out-of-tree patch set (which is why
>> I've stumbled over this and the open-coding of it in the BGRT
>> code in the first place). Plus to me it seems preferable to have
>> both functions consistent (of course you could then as for
>> the __weak to be dropped from the other one).
>>
> 
> Making symbols __weak in mainline to accommodate unidentified
> out-of-tree users is something I'd prefer to avoid.

Would you then mind extending the #ifndef to the other function
as well, or would I need to make this a 3rd patch?

Jan

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] EFI: move efi_mem_type() to common code

[Jan Beulich]

>>> On 24.08.17 at 11:52, <ard.biesheu...@linaro.org> wrote:
> On 24 August 2017 at 10:48, Jan Beulich <jbeul...@suse.com> wrote:
>>>>> On 24.08.17 at 11:18, <ard.biesheu...@linaro.org> wrote:
>>> On 24 August 2017 at 10:11, Jan Beulich <jbeul...@suse.com> wrote:
>>>> --- 4.13-rc6-EFI.orig/drivers/firmware/efi/efi.c
>>>> +++ 4.13-rc6-EFI/drivers/firmware/efi/efi.c
>>>> @@ -819,6 +819,35 @@ u64 __weak efi_mem_attributes(unsigned l
>>>> return 0;
>>>>  }
>>>>
>>>> +/*
>>>> + * efi_mem_type - lookup memmap type for physical address
>>>> + * @phys_addr: the physical address to lookup
>>>> + *
>>>> + * Search in the EFI memory map for the region covering @phys_addr.
>>>> + * Returns the EFI memory type if the region was found in the memory
>>>> + * map, EFI_RESERVED_TYPE (zero) otherwise.
>>>> + *
>>>> + * Despite being marked __weak, most architectures should *not*
>>>> + * override this function. It is __weak solely for the benefit
>>>> + * of ia64 which has a funky EFI memory map that doesn't work
>>>> + * the same way as other architectures.
>>>> + */
>>>> +u32 __weak efi_mem_type(unsigned long phys_addr)
>>>
>>> Why is this __weak now?
>>
>> As the comment (matching the one ahead of efi_mem_attributes())
>> says, in particular for ia64 to be able to override it.
>>
> 
> Apologies. I don't know how I could miss that ...
> 
> If it already has its own version, I'd prefer we just add #ifndef
> CONFIG_IA64 around it instead.

Which would then preclude other environments to override it.
I have such a case in an out-of-tree patch set (which is why
I've stumbled over this and the open-coding of it in the BGRT
code in the first place). Plus to me it seems preferable to have
both functions consistent (of course you could then as for
the __weak to be dropped from the other one).

Jan

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] EFI: move efi_mem_type() to common code

[Jan Beulich]

>>> On 24.08.17 at 11:18, <ard.biesheu...@linaro.org> wrote:
> On 24 August 2017 at 10:11, Jan Beulich <jbeul...@suse.com> wrote:
>> --- 4.13-rc6-EFI.orig/drivers/firmware/efi/efi.c
>> +++ 4.13-rc6-EFI/drivers/firmware/efi/efi.c
>> @@ -819,6 +819,35 @@ u64 __weak efi_mem_attributes(unsigned l
>> return 0;
>>  }
>>
>> +/*
>> + * efi_mem_type - lookup memmap type for physical address
>> + * @phys_addr: the physical address to lookup
>> + *
>> + * Search in the EFI memory map for the region covering @phys_addr.
>> + * Returns the EFI memory type if the region was found in the memory
>> + * map, EFI_RESERVED_TYPE (zero) otherwise.
>> + *
>> + * Despite being marked __weak, most architectures should *not*
>> + * override this function. It is __weak solely for the benefit
>> + * of ia64 which has a funky EFI memory map that doesn't work
>> + * the same way as other architectures.
>> + */
>> +u32 __weak efi_mem_type(unsigned long phys_addr)
> 
> Why is this __weak now?

As the comment (matching the one ahead of efi_mem_attributes())
says, in particular for ia64 to be able to override it.

Jan

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] EFI/BGRT: use efi_mem_type()

[Jan Beulich]

Avoid effectively open-coding the function.

Signed-off-by: Jan Beulich <jbeul...@suse.com>
---
 drivers/firmware/efi/efi-bgrt.c |   22 +-
 1 file changed, 1 insertion(+), 21 deletions(-)

--- 4.13-rc6-EFI.orig/drivers/firmware/efi/efi-bgrt.c
+++ 4.13-rc6-EFI/drivers/firmware/efi/efi-bgrt.c
@@ -27,26 +27,6 @@ struct bmp_header {
u32 size;
 } __packed;
 
-static bool efi_bgrt_addr_valid(u64 addr)
-{
-   efi_memory_desc_t *md;
-
-   for_each_efi_memory_desc(md) {
-   u64 size;
-   u64 end;
-
-   if (md->type != EFI_BOOT_SERVICES_DATA)
-   continue;
-
-   size = md->num_pages << EFI_PAGE_SHIFT;
-   end = md->phys_addr + size;
-   if (addr >= md->phys_addr && addr < end)
-   return true;
-   }
-
-   return false;
-}
-
 void __init efi_bgrt_init(struct acpi_table_header *table)
 {
void *image;
@@ -85,7 +65,7 @@ void __init efi_bgrt_init(struct acpi_ta
goto out;
}
 
-   if (!efi_bgrt_addr_valid(bgrt->image_address)) {
+   if (efi_mem_type(bgrt->image_address) != EFI_BOOT_SERVICES_DATA) {
pr_notice("Ignoring BGRT: invalid image address\n");
goto out;
}



--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] EFI: move efi_mem_type() to common code

[Jan Beulich]

This follows efi_mem_attributes(), as it's similarly generic.

Signed-off-by: Jan Beulich <jbeul...@suse.com>
---
 arch/x86/platform/efi/efi.c |   19 ---
 drivers/firmware/efi/efi.c  |   29 +
 2 files changed, 29 insertions(+), 19 deletions(-)

--- 4.13-rc6-EFI.orig/arch/x86/platform/efi/efi.c
+++ 4.13-rc6-EFI/arch/x86/platform/efi/efi.c
@@ -1032,25 +1032,6 @@ void __init efi_enter_virtual_mode(void)
efi_dump_pagetable();
 }
 
-/*
- * Convenience functions to obtain memory types and attributes
- */
-u32 efi_mem_type(unsigned long phys_addr)
-{
-   efi_memory_desc_t *md;
-
-   if (!efi_enabled(EFI_MEMMAP))
-   return 0;
-
-   for_each_efi_memory_desc(md) {
-   if ((md->phys_addr <= phys_addr) &&
-   (phys_addr < (md->phys_addr +
- (md->num_pages << EFI_PAGE_SHIFT
-   return md->type;
-   }
-   return 0;
-}
-
 static int __init arch_parse_efi_cmdline(char *str)
 {
if (!str) {
--- 4.13-rc6-EFI.orig/drivers/firmware/efi/efi.c
+++ 4.13-rc6-EFI/drivers/firmware/efi/efi.c
@@ -819,6 +819,35 @@ u64 __weak efi_mem_attributes(unsigned l
return 0;
 }
 
+/*
+ * efi_mem_type - lookup memmap type for physical address
+ * @phys_addr: the physical address to lookup
+ *
+ * Search in the EFI memory map for the region covering @phys_addr.
+ * Returns the EFI memory type if the region was found in the memory
+ * map, EFI_RESERVED_TYPE (zero) otherwise.
+ *
+ * Despite being marked __weak, most architectures should *not*
+ * override this function. It is __weak solely for the benefit
+ * of ia64 which has a funky EFI memory map that doesn't work
+ * the same way as other architectures.
+ */
+u32 __weak efi_mem_type(unsigned long phys_addr)
+{
+   const efi_memory_desc_t *md;
+
+   if (!efi_enabled(EFI_MEMMAP))
+   return 0;
+
+   for_each_efi_memory_desc(md) {
+   if ((md->phys_addr <= phys_addr) &&
+   (phys_addr < (md->phys_addr +
+ (md->num_pages << EFI_PAGE_SHIFT
+   return md->type;
+   }
+   return EFI_RESERVED_TYPE;
+}
+
 int efi_status_to_err(efi_status_t status)
 {
int err;



--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH] efi/libstub/fdt: Standardize the names of EFI stub parameters

[Jan Beulich]

>>> On 14.09.15 at 11:36,  wrote:
> On 14 September 2015 at 11:31, Shannon Zhao  wrote:
>> My understanding is that if there are no EFI_MEMORY_RUNTIME regions, it
>> means we can't use runtime services and should not set the bit
>> EFI_RUNTIME_SERVICES of efi.flags. But if efi_virtmap_init() return
>> true, the bit will be set.
>>
> 
> As I said, if you don't want the EFI_RUNTIME_SERVICES bit to be set
> for other reasons, don't rig efi_virtmap_init() to return false when
> it shouldn't.
> 
>>> The absence of such regions is allowed by the spec, so
>>> efi_virtmap_init() is correct imo to return success.
>>>
>> Sorry, not well know about the spec. Could you point out where the spec
>> says this?
>>
> 
> Well, I think it doesn't work that way. You are claiming that a memory
> map without at least one EFI_MEMORY_RUNTIME constitutes an error
> condition, so the burden is on you to provide a reference to the spec
> that says you must have at least one such region.

Sure, from a spec pov you're right. But where would runtime
services code/data live when there's not a single region marked
as needing a runtime mapping. IOW while the spec doesn't say
so, assuming no runtime services when there's not at least one
executable region with the runtime flag set could serve as a stop
gap measure against flawed firmware.

Jan

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH] efi/libstub/fdt: Standardize the names of EFI stub parameters

[Jan Beulich]

>>> On 14.09.15 at 13:16,  wrote:
> (I still think not using SetVirtualAddressMap() at all
> would be the best approach for arm64, but unfortunately, most of my
> colleagues disagree with me)

Any reasons they have? I'm curious because we run x86 Xen without
using this function ...

Jan

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH] efi/libstub/fdt: Standardize the names of EFI stub parameters

[Jan Beulich]

>>> On 10.09.15 at 14:58,  wrote:
> On Thu, 2015-09-10 at 13:15 +0100, Mark Rutland wrote:
> 
>> > In any case this should be separate from the shim ABI discussion.
>> 
>> I disagree; I think this is very much relevant to the ABI discussion.
>> That's not to say that I insist on a particular approach, but I think
>> that they need to be considered together.
> 
> Taking a step back, the reason for using the EFI stub parameters is only
> (AFAIK) in order to be able to locate the ACPI RDSP (the root table
> pointer), which as it happens is normally passed via one of the EFI
> firmware tables.
> 
> If there was a way to achieve that goal (i.e. another way to find the RSDP)
> without opening the can of UEFI worms then we could consider that opiton
> too.
> 
> a way != the legacy x86 thing of scanning low memory of the signature, of
> course.

But even x86 doesn't do that (other than as a fallback) on EFI. The
configuration table is available to Dom0 (via XENPF_firmware_info:
XEN_FW_EFI_INFO:XEN_FW_EFI_CONFIG_TABLE).

Jan

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] efi/libstub/fdt: Standardize the names of EFI stub parameters

[Jan Beulich]

>>> On 10.09.15 at 13:37,  wrote:
> On Thu, 10 Sep 2015, Mark Rutland wrote:
>> Why can't Xen give a virtual EFI interface to Dom0 / guests? e.g.
>> create pages of RuntimeServicesCode that are trivial assembly shims
>> doing hypercalls, and plumb these into the virtual EFI memory map and
>> tables?
>> 
>> That would keep things sane for any guest, allow for easy addition of
>> EFI features, and you could even enter the usual EFI entry point,
>> simulate ExitBootServices(), SetVirtualAddressMap(), and allow the guest
>> to make things sane for itself...
> 
> That's the way it was done on x86 and now we have common code both in
> Linux (drivers/xen/efi.c) and Xen (xen/common/efi) which implement this
> scheme.  Switching to a different solution for ARM, would mean diverging
> with x86, which is not nice, or reimplementing the x86 solution too,
> which is expensive.
> 
> BTW I think that the idea you proposed was actually considered at the
> time and deemed hard to implement, if I recall correctly.

Considering that the EFI support is just for Dom0, and Dom0 (at
the time) had to be PV anyway, it was the more natural solution to
expose the interface via hypercalls, the more that this allows better
control over what is and primarily what is not being exposed to
Dom0. With the wrapper approach we'd be back to the same
problem (discussed elsewhere) of which EFI version to surface: The
host one would impose potentially missing extensions, while the
most recent hypervisor known one might imply hiding valuable
information from Dom0. Plus there are incompatible changes like
the altered meaning of EFI_MEMORY_WP in 2.5.

Jan

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] efi/libstub/fdt: Standardize the names of EFI stub parameters

[Jan Beulich]

>>> On 10.09.15 at 16:53, <mark.rutl...@arm.com> wrote:
> On Thu, Sep 10, 2015 at 01:55:25PM +0100, Jan Beulich wrote:
>> >>> On 10.09.15 at 13:37, <stefano.stabell...@eu.citrix.com> wrote:
>> > On Thu, 10 Sep 2015, Mark Rutland wrote:
>> >> Why can't Xen give a virtual EFI interface to Dom0 / guests? e.g.
>> >> create pages of RuntimeServicesCode that are trivial assembly shims
>> >> doing hypercalls, and plumb these into the virtual EFI memory map and
>> >> tables?
>> >> 
>> >> That would keep things sane for any guest, allow for easy addition of
>> >> EFI features, and you could even enter the usual EFI entry point,
>> >> simulate ExitBootServices(), SetVirtualAddressMap(), and allow the guest
>> >> to make things sane for itself...
>> > 
>> > That's the way it was done on x86 and now we have common code both in
>> > Linux (drivers/xen/efi.c) and Xen (xen/common/efi) which implement this
>> > scheme.  Switching to a different solution for ARM, would mean diverging
>> > with x86, which is not nice, or reimplementing the x86 solution too,
>> > which is expensive.
>> > 
>> > BTW I think that the idea you proposed was actually considered at the
>> > time and deemed hard to implement, if I recall correctly.
>> 
>> Considering that the EFI support is just for Dom0, and Dom0 (at
>> the time) had to be PV anyway, it was the more natural solution to
>> expose the interface via hypercalls, the more that this allows better
>> control over what is and primarily what is not being exposed to
>> Dom0. With the wrapper approach we'd be back to the same
>> problem (discussed elsewhere) of which EFI version to surface: The
>> host one would impose potentially missing extensions, while the
>> most recent hypervisor known one might imply hiding valuable
>> information from Dom0. Plus there are incompatible changes like
>> the altered meaning of EFI_MEMORY_WP in 2.5.
> 
> I'm not sure I follow how hypercalls solve any impedance mismatch here;
> you're still expecting Dom0 to call up to Xen in order to perform calls,
> and all I suggested was a different location for those hypercalls.
> 
> If Xen is happy to make such calls blindly, why does it matter if the
> hypercall was in the kernel binary or an external shim?

Because there could be new entries in SystemTable->RuntimeServices
(expected and blindly but validly called by the kernel). Even worse
(because likely harder to deal with) would be new fields in other
structures.

> Incompatible changes are a spec problem regardless of how this is
> handled.

Not necessarily - we don't expose the memory map (we'd have to
if we were to mimic EFI for Dom0), and hence the mentioned issue
doesn't exist in our model.

Jan

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/9] efi: Use early_mem*() instead of early_io*()

[Jan Beulich]

 On 20.06.14 at 23:29, daniel.ki...@oracle.com wrote:
 --- a/drivers/firmware/efi/efi.c
 +++ b/drivers/firmware/efi/efi.c
 @@ -298,7 +298,7 @@ int __init efi_config_init(efi_config_table_type_t 
 *arch_tables)
   if (table64  32) {
   pr_cont(\n);
   pr_err(Table located above 4GB, disabling 
 EFI.\n);
 - early_iounmap(config_tables,
 + early_memunmap(config_tables,
  efi.systab-nr_tables * sz);
   return -EINVAL;
   }
 @@ -314,7 +314,7 @@ int __init efi_config_init(efi_config_table_type_t 
 *arch_tables)
   tablep += sz;
   }
   pr_cont(\n);
 - early_iounmap(config_tables, efi.systab-nr_tables * sz);
 + early_memunmap(config_tables, efi.systab-nr_tables * sz);
  
   set_bit(EFI_CONFIG_TABLES, efi.flags);
  

If these two changes are really deemed necessary (there's the
implied assumption currently in place that early_iounmap() can
undo early_memremap() mappings), then ia64 will need a
definition added for early_memunmap() or its build will break.

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/9] arch/x86: Do not access EFI memory map if it is not available

[Jan Beulich]

 On 23.06.14 at 11:53, david.vra...@citrix.com wrote:
 On 20/06/14 22:29, Daniel Kiper wrote:
 Do not access EFI memory map if it is not available. At least
 Xen dom0 EFI implementation does not have an access to it.
 
 Could it make one based on the XENMEM_memory_map or
 XENMEM_machine_memory_map hypercall?

No, the correct operation to implement this and efi_mem_type()
similar function is XEN_FW_EFI_INFO, index XEN_FW_EFI_MEM_INFO.

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/7] efi: Introduce EFI_NO_DIRECT flag

[Jan Beulich]

 On 18.06.14 at 15:52, m...@console-pimps.org wrote:
 EFI_PARAVIRT will be usable by architectures other than x86, correct? If
 your intention is for it only ever to be used by x86, then it should
 probably be EFI_ARCH_2.

I would expect ARM, once it gets UEFI support on the Xen side, to
be able to handle most of this identically to x86. Which raises the
question whether most of the new Xen-specific code (in one of the
other patches) wouldn't better live under drivers/xen/.

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag

[Jan Beulich]

 On 19.05.14 at 22:46, daniel.ki...@oracle.com wrote:
 On Mon, May 19, 2014 at 02:30:45PM +0100, Jan Beulich wrote:
  On 16.05.14 at 22:41, daniel.ki...@oracle.com wrote:
  @@ -457,6 +460,21 @@ void __init efi_free_boot_services(void)
 efi_unmap_memmap();
   }
 
  +static void __init __iomem *efi_early_ioremap(resource_size_t phys_addr,
  +  unsigned long size)
  +{
  +  if (efi_enabled(EFI_DIRECT))
  +  return early_ioremap(phys_addr, size);
  +
  +  return (__force void __iomem *)phys_addr;

 Now that surely needs some explanation: I can't see how this can
 ever be correct, Xen or not being completely irrelevant.
 
 I hope that efi_enabled(EFI_DIRECT) is obvious. However, in case of
 !efi_enabled(EFI_DIRECT) some structures are created artificially
 and they live in virtual address space. So that is why they should
 not be mapped. If you wish I could add relevant comment here.

That would be the very minimum I suppose. But I wonder whether
you wouldn't be better off storing their physical addresses in the
first place (and then decide whether you can stay with early_ioremap()
or want/need to use early_memremap() if !EFI_DIRECT).

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/5] xen: Put EFI machinery in place

[Jan Beulich]

 On 20.05.14 at 13:29, daniel.ki...@oracle.com wrote:
 On Tue, May 20, 2014 at 10:47:00AM +0100, David Vrabel wrote:
 On 16/05/14 21:41, Daniel Kiper wrote:
  @@ -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?

On the basis that (for Dom0 only) this is the equivalent of (and
actually also falls back to) mach_get_cmos_time(). If on Dom0
.get_wallclock doesn't get set to mach_get_cmos_time() on
pv-ops, then that line above should also be dropped.

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/5] xen: Put EFI machinery in place

[Jan Beulich]

 On 16.05.14 at 22:41, daniel.ki...@oracle.com wrote:
 --- a/drivers/xen/Kconfig
 +++ b/drivers/xen/Kconfig
 @@ -240,4 +240,7 @@ config XEN_MCE_LOG
  config XEN_HAVE_PVMMU
 bool
  
 +config XEN_EFI
 + def_bool X86_64  EFI

Constructs like this are bogus - they needlessly add a line to .config
when the expression evaluates to false.

config XEN_EFI
def_bool y
depends on X86_64  EFI

is what avoids this.

 +static efi_status_t xen_efi_get_variable(efi_char16_t *name,
 +  efi_guid_t *vendor,
 +  u32 *attr,
 +  unsigned long *data_size,
 +  void *data)
 +{
 + int err;
 + DECLARE_CALL(get_variable);
 +
 + set_xen_guest_handle(call.u.get_variable.name, name);
 + BUILD_BUG_ON(sizeof(*vendor) !=
 +  sizeof(call.u.get_variable.vendor_guid));
 + memcpy(call.u.get_variable.vendor_guid, vendor, sizeof(*vendor));
 + call.u.get_variable.size = *data_size;
 + set_xen_guest_handle(call.u.get_variable.data, data);
 + err = HYPERVISOR_dom0_op(op);
 + if (err)
 + return EFI_UNSUPPORTED;
 +
 + *data_size = call.u.get_variable.size;
 + *attr = call.misc; /* misc in struction is U32 variable*/

Iirc attr is an optional output, i.e. can be NULL, which hence needs
to be checked for here. I remember that this was broken in the
original EFI patch in our trees, so perhaps it would be good for you
to re-check against recent sources of ours for eventual other bug
fixes.

 +static efi_status_t xen_efi_query_variable_info(u32 attr,
 + u64 *storage_space,
 + u64 *remaining_space,
 + u64 *max_variable_size)
 +{
 + int err;
 + DECLARE_CALL(query_variable_info);
 +
 + if (efi.runtime_version  EFI_2_00_SYSTEM_TABLE_REVISION)
 + return EFI_UNSUPPORTED;
 +

Here's a similar case - there is

call.u.query_variable_info.attr = attr;

missing.

 +static efi_char16_t vendor[100] __initdata;
 +static const efi_char16_t unknown[] __initconst =
 + {'U', 'N', 'K', 'N', 'O', 'W', 'N', '\0'};

If you enforced -fshort-wchar for the file's compilation, this could be
written in a more legible manner (and probably you wouldn't need a
named variable at all).

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH v4 3/5] xen: Put EFI machinery in place

[Jan Beulich]

 On 19.05.14 at 15:39, andrew.coop...@citrix.com wrote:
 On 16/05/14 21:41, Daniel Kiper wrote:
 @@ -0,0 +1,374 @@
 +/*
 + * EFI support for Xen.
 + *
 + * Copyright (C) 1999 VA Linux Systems
 + * Copyright (C) 1999 Walt Drummond drumm...@valinux.com
 + * Copyright (C) 1999-2002 Hewlett-Packard Co.
 + *  David Mosberger-Tang dav...@hpl.hp.com
 + *  Stephane Eranian eran...@hpl.hp.com
 + * Copyright (C) 2005-2008 Intel Co.
 + *  Fenghua Yu fenghua...@intel.com
 + *  Bibo Mao bibo@intel.com
 + *  Chandramouli Narayanan mo...@linux.intel.com
 + *  Huang Ying ying.hu...@intel.com
 + * Copyright (C) 2011 Novell Co.
 + *  Jan Beulich jbeul...@suse.com
 + * Copyright (C) 2011-2012 Oracle Co.
 + *  Liang Tang liang.t...@oracle.com
 + * Copyright (c) 2014 Daniel Kiper, Oracle Corporation
 + */
 +
 +#include linux/bug.h
 +#include linux/efi.h
 +#include linux/init.h
 +#include linux/string.h
 +
 +#include xen/interface/xen.h
 +#include xen/interface/platform.h
 +
 +#include asm/xen/hypercall.h
 +
 +#define call (op.u.efi_runtime_call)
 
 I think not.

??? (Read: What's wrong with putting this shortcut in place?)

 +#define DECLARE_CALL(what) \
 +struct xen_platform_op op; \
 +op.cmd = XENPF_efi_runtime_call; \
 +call.function = XEN_EFI_##what; \
 +call.misc = 0
 
 Macros like this which explicitly create local variable with implicit
 names are evil when reading code.
 
 If you want to do initialisation like this, then at least do something like:
 
 #define INIT_EFI_CALL(what) \
 { .cmd = XENPF_efi_runtime_call, \
.u.efi_runtime_call.function = XEN_EFI_##what, \
.u.efi_runtime_call.misc = 0 }
 
 And use it as:
 
 struct xen_platform_op op = INIT_EFI_CALL(foo);

That's minimal redundancy as a goal versus (to you, but not to me)
legibility.

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag

[Jan Beulich]

 On 16.05.14 at 22:41, daniel.ki...@oracle.com wrote:
 @@ -457,6 +460,21 @@ void __init efi_free_boot_services(void)
   efi_unmap_memmap();
  }
  
 +static void __init __iomem *efi_early_ioremap(resource_size_t phys_addr,
 + unsigned long size)
 +{
 + if (efi_enabled(EFI_DIRECT))
 + return early_ioremap(phys_addr, size);
 +
 + return (__force void __iomem *)phys_addr;

Now that surely needs some explanation: I can't see how this can
ever be correct, Xen or not being completely irrelevant.

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

[Jan Beulich]

 On 26.03.14 at 14:00, m...@console-pimps.org wrote:
 On Tue, 25 Mar, at 09:57:54PM, Daniel Kiper wrote:
 Call efi_memblock_x86_reserve_range() on native EFI platform only.
 This is not needed and even it should not be called on platforms
 which wraps EFI infrastructure like Xen.
 
 Signed-off-by: Daniel Kiper daniel.ki...@oracle.com
 ---
  arch/x86/kernel/setup.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
 index ce72964..992b67a 100644
 --- a/arch/x86/kernel/setup.c
 +++ b/arch/x86/kernel/setup.c
 @@ -933,7 +933,7 @@ void __init setup_arch(char **cmdline_p)
  set_bit(EFI_64BIT, x86_efi_facility);
  }
  
 -if (efi_enabled(EFI_BOOT))
 +if (!strncmp((char *)boot_params.efi_info.efi_loader_signature, EL, 
 2))
  efi_memblock_x86_reserve_range();
  #endif
 
 This could do with a little bit more explanation. Why is it not
 necessary to mark the EFI memory map that was passed to the kernel as
 reserved in memblock?

Because that's in memory Dom0 doesn't even see: The EFI memory
map is visible to the hypervisor only.

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

[Jan Beulich]

 On 26.03.14 at 14:31, m...@console-pimps.org wrote:
 On Wed, 26 Mar, at 01:22:49PM, Jan Beulich wrote:
  On 26.03.14 at 14:00, m...@console-pimps.org wrote:
  
  This could do with a little bit more explanation. Why is it not
  necessary to mark the EFI memory map that was passed to the kernel as
  reserved in memblock?
 
 Because that's in memory Dom0 doesn't even see: The EFI memory
 map is visible to the hypervisor only.
 
 So where does boot_params.efi_info.efi_memmap point?
 
 If nowhere (i.e. it's NULL) that's no problem because memblock_reserve()
 handles zero size regions just fine.

That's a question to Daniel - in our implementation (with a separate
Xen kernel that can't run on bare hardware) boot_params as a whole
simply doesn't exist.

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/5] efi: Export arch_tables variable

[Jan Beulich]

 On 26.03.14 at 15:08, daniel.ki...@oracle.com wrote:
 On Wed, Mar 26, 2014 at 01:21:19PM +, Jan Beulich wrote:
  On 25.03.14 at 21:57, daniel.ki...@oracle.com wrote:
  Export arch_tables variable. Xen init function calls efi_config_init()
  which takes it as an argument.
 
  Additionally, put __initdata in place suggested by include/linux/init.h.

 Which isn't necessarily the most appropriate place.
 
 Why? If comments in include/linux/init.h are not valid they should be 
 changed.

Because they can't go there uniformly: While on function declarations
you can put them there, on function definitions they need to come
before the function name. And placing attributes between type and
name does - iirc - work consistently for everything.

  --- a/include/linux/efi.h
  +++ b/include/linux/efi.h
  @@ -583,6 +583,8 @@ extern struct efi {
 struct efi_memory_map *memmap;
   } efi;
 
  +extern efi_config_table_type_t arch_tables[] __initdata;

 And section placement annotations are bogus on declarations.
 
 Hmmm... I am not sure which approach is better. I saw that
 in many places declarations have annotations. Could you
 point me some docs which states (and explains) that this
 is wrong idea.

Just use common sense: Attributes that are of concern to the
caller should go on the declaration. Attributes that only affect
code generation for the function/object in question should go on
the definition.

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Jan Beulich]

 On 10.03.14 at 08:37, Ingo Molnar mi...@kernel.org wrote:

 * Jan Beulich jbeul...@suse.com wrote:
 
  On 09.03.14 at 19:50, Matt Fleming m...@console-pimps.org wrote:
  On Sun, 09 Mar, at 04:31:41PM, Matthew Garrett wrote:
  On Sun, Mar 09, 2014 at 04:20:20PM +, Matt Fleming wrote:
  
   We have tried to use the time functions before, with little success
   because of various bugs in the runtime implementations, e.g. see commit
   bacef661acdb (x86-64/efi: Use EFI to deal with platform wall clock)
   and commit bd52276fa1d4 (x86-64/efi: Use EFI to deal with platform wall
   clock (again)).
  
  I'd naively expected that these would be more reliable after the 
  1:1 mapping patches, so it might actually be time to give them 
  another go.
  
  Is there any value in that? Do machines exist where we absolutely 
  must have access to the EFI time services? Either because there's 
  no other method or no other working one?
 
 Is it such a bad thing to be prepared for this sort of machine to 
 arrive even if likely there are none so far?
 
 Be prepared for a not yet existing machine != time to give them 
 another go on existing machines, right?

That heavily depends on the perspective you take, and I know
we tend to have disagreeing perspectives here: I think things
should be done the intended way on all systems where this is
possible. Remember the far from insignificant number of issues
with the ACPI interrupt source overrides for particularly the
timer interrupt years ago? Which wasn't a reason to stop trying
to use ACPI wherever possible, even on affected systems (by
introducing command line overrides).

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Jan Beulich]

 On 10.03.14 at 11:43, Matt Fleming m...@console-pimps.org wrote:
 On Mon, 10 Mar, at 08:22:47AM, Jan Beulich wrote:
 Just to give an example from the Xen side: Xen uses the time
 interface in UEFI, as you would expect not without problems. Apart
 from issues with memory areas needed by those runtime calls not
 being properly marked for runtime use (which the respective
 vendors accepted they need to fix), we are also facing problems
 with runtime calls using XMM registers.
 
 Presumably you've got some kind of quirk mechanism to get things
 working?

Not yet, since so far we haven't been shown issues with the code
on other than non-production systems.

 That doesn't exist in the kernel so the time functions as-is
 are useless, right?

Not sure why you call them useless. If wired up properly, they
would be useful on systems where they work.

 What's the benefit of using the EFI time services for Xen?

As said before: Don't suffer from there not being any wallclock
if there's no CMOS clock. With there even being a FADT flag to
indicate its absence, there clearly must be plans to have such
machines.

 Does Xen use the current kernel functions directly?

Which kernel functions? Perhaps just a misunderstanding - I
gave that example with the actual hypervisor in mind, not the
kernel side code sitting on top. The kernel, after all, can't
directly call EFI runtime code when running on top of Xen.

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] x86, efi: retry ExitBootServices() on failure

[Jan Beulich]

 Zachary Bobroff zacha...@ami.com 06/18/13 2:25 AM 
 We only need to retry once because of what is in the UEFI specification 2.3.1 
 Errata
 C.  The exact verbiage is as follows:

 The ExitBootServices() function is called by the currently executing EFI OS 
 loader
 image to terminate all boot services. On success, the loader becomes 
 responsible for
 the continued operation of the system. All events of type 
 EVT_SIGNAL_EXIT_BOOT_SERVICES
 must be signaled before ExitBootServices() returns EFI_SUCCESS. The events are
 only signaled once even if ExitBootServices() is called multiple times.

 Since the firmware will only signal the ExitBootServices event once, the code 
 that
 has the potential to change the memory map will only be signaled on the first 
 call to
 exit boot services.

Okay, I'm fine with that aspect then. Let's hope everyone plays by that rule.

 Here is the logic for why we need space for the two additional Efi Memory Map
 Descriptors is as follows:
 First, we should think of the size that is returned from the GetMemoryMap as 
 being
 the number of bytes to contain the current memory map.  Once we have the size 
 of
 the current memory map, we, the kernel, have to perform an allocation of 
 memory
 so that we can store the current memory map into some memory that will not be
 overwritten. That allocation has the possibility of increasing the current 
 memory
 map count by one efi_memory_desc_t structure.

Why by one? Splitting some 'free memory' block may result in an increase by more
then one afaict. Assuming the increment can only be one is implying you having
knowledge of the allocator implementation and behavior, which shouldn't be made
use of in kernel code.

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] x86, efi: retry ExitBootServices() on failure

[Jan Beulich]

 On 17.06.13 at 11:21, Matt Fleming m...@console-pimps.org wrote:
 On Fri, 14 Jun, at 12:00:33AM, joeyli wrote:
  From: Zach Bobroff zacha...@ami.com
  --- a/arch/x86/boot/compressed/eboot.c
  +++ b/arch/x86/boot/compressed/eboot.c
  @@ -1037,18 +1037,20 @@ static efi_status_t exit_boot(struct boot_params 
  *boot_params,
 efi_memory_desc_t *mem_map;
 efi_status_t status;
 __u32 desc_version;
  +  bool called_exit = false;
 u8 nr_entries;
 int i;
   
 size = sizeof(*mem_map) * 32;
   
   again:
  -  size += sizeof(*mem_map);
  +  size += sizeof(*mem_map) * 2;
 _size = size;
 status = low_alloc(size, 1, (unsigned long *)mem_map);
 
 Can we know why increased the size of double *mem_map is big enough?
 Does there have any real guarantee to be sufficient?
  
 It's not guaranteed to be sufficient, it's simply a best guess. If we
 haven't allocated enough memory to store the memory map we should loop
 around to the 'again' label anyway. It's just an optimisation, right
 Zach?

If that were the case, the code change wouldn't be necessary at
all, i.e. the increment could remain to be a single array element
steps.

 And, why would doubling the increment be necessary here, but not in
 __get_map()?
  
 Again, we'll grow the map if it isn't large enough.

But in single element steps only. The question though was why
doubling the step was necessary (and sufficient) there, but isn't
also necessary here.

To me, all this looks like it is being done on phenomenological basis,
taking a particular build of a particular firmware implementation as
the reference. Imo we shouldn't change the code in this way. This
also applies to the fact that the step is being doubled rather than
e.g. tripled: With it ending up a again anyway (see below), what's
the point of avoiding one more of the iterations?

Generic considerations would result in the increment being at least
3 * element size; twice the element size assumes that the allocator
would behave in certain ways (like returning the head or tail part of
a larger piece of memory).

 if (status != EFI_SUCCESS)
 return status;
   
  +get_map:
 
 The get_map label is being placed such that the size doesn't
 get adjusted anymore, yet it is supposed to deal with an allocation
 having happened during ExitBootServices(), which could have
 grown the map.
 
 Why doesn't direct goto 'again' label?
  
 It makes more sense to query GetMemoryMap() directly first to get the
 required size of the memory map. Then we jump to 'again' and allocate
 the correct size.

Ah, okay, I didn't pay attention to this indirectly ending up at
again.

 status = efi_call_phys5(sys_table-boottime-get_memory_map, size,
 mem_map, key, desc_size, desc_version);
 if (status == EFI_BUFFER_TOO_SMALL) {
  @@ -1074,8 +1076,20 @@ again:
 /* Might as well exit boot services now */
 status = efi_call_phys2(sys_table-boottime-exit_boot_services,
 handle, key);
  -  if (status != EFI_SUCCESS)
  -  goto free_mem_map;
  +  if (status != EFI_SUCCESS) {
  +  /*
  +   * ExitBootServices() will fail if any of the event
  +   * handlers change the memory map. In which case, we
  +   * must be prepared to retry, but only once so that
  +   * we're guaranteed to exit on repeated failures instead
  +   * of spinning forever.
  +   */
  +  if (called_exit)
  +  goto free_mem_map;
  +
  +  called_exit = true;
 
 Why a single retry is having reasonable guarantees to work when the
 original one failed (nothing prevents an event handler to do another
 allocation the next time through).
 
 Why not retry 3, 4, 5 or even unlimited times?
  
 There has to be an upper limit on the number of retries. It seems fair
 to retry once but any more than that and it's more likely that there's a
 serious problem and we should bail.

I agree that there ought to be an upper limit. But a single retry here
again looks like a tailored solution to a particular observed (mis-)
behavior, rather than something resulting from general considerations.

Jan
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] x86, efi: retry ExitBootServices() on failure

[Jan Beulich]

 On 17.06.13 at 12:17, Matt Fleming m...@console-pimps.org wrote:
 On Mon, 17 Jun, at 10:46:28AM, Jan Beulich wrote:
 I agree that there ought to be an upper limit. But a single retry here
 again looks like a tailored solution to a particular observed (mis-)
 behavior, rather than something resulting from general considerations.
 
 What value would you suggest for the retry?

I'd be considering something like 5...10, but this to some degree
depends on what odd kinds of behavior this in fact needs to
cover.

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] fix setup_efi_pci()

[Jan Beulich]

 On 24.01.13 at 19:20, Matt Fleming matt.flem...@intel.com wrote:
 On Fri, 2013-01-18 at 12:35 +, Jan Beulich wrote:
 This fixes two issues:
 - wrong memory type used for allocation intended to persist post-boot
 
 [...]
 
 @@ -311,7 +311,7 @@ static efi_status_t setup_efi_pci(struct
  size = pci-romsize + sizeof(*rom);
  
  status = efi_call_phys3(sys_table-boottime-allocate_pool,
 -EFI_LOADER_DATA, size, rom);
 +EFI_RUNTIME_SERVICES_DATA, size, rom);
  
  if (status != EFI_SUCCESS)
  continue;
 
 I'm curious why you made this change. No one should be stealing that
 region of memory because that's all handled in parse_e820_ext() - it's
 marked as off limits wrt memory for the kernel's use. And the firmware
 certainly shouldn't start touching it.

According to my reading of do_add_efi_memmap(), EFI_LOADER_DATA
gets treated the same as EFI_CONVENTIONAL_MEMORY, i.e. gets
marked as available for allocation. That's also how I'd expect things
to be. I don't think parse_e820_ext() matters here at all.

 Have you witnessed some case where things explode without your change?

No, I didn't. I merely spotted this when evaluating the code for
porting over to Xen.

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] fix setup_efi_pci()

[Jan Beulich]

This fixes two issues:
- wrong memory type used for allocation intended to persist post-boot
- four similar build warnings on 32-bit (casts between different size
  pointers and integers)

Signed-off-by: Jan Beulich jbeul...@suse.com
---
v2: Drop the change that was already applied separately (commit
886d751a2ea99a160f2d0a472231566d9cb0cf58 x86, efi: correct
precedence of operators in setup_efi_pci)

---
 arch/x86/boot/compressed/eboot.c |   10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

--- 3.8-rc4/arch/x86/boot/compressed/eboot.c
+++ 3.8-rc4-x86-EFI-PCI-ROMs/arch/x86/boot/compressed/eboot.c
@@ -256,10 +256,10 @@ static efi_status_t setup_efi_pci(struct
int i;
struct setup_data *data;
 
-   data = (struct setup_data *)params-hdr.setup_data;
+   data = (struct setup_data *)(unsigned long)params-hdr.setup_data;
 
while (data  data-next)
-   data = (struct setup_data *)data-next;
+   data = (struct setup_data *)(unsigned long)data-next;
 
status = efi_call_phys5(sys_table-boottime-locate_handle,
EFI_LOCATE_BY_PROTOCOL, pci_proto,
@@ -311,7 +311,7 @@ static efi_status_t setup_efi_pci(struct
size = pci-romsize + sizeof(*rom);
 
status = efi_call_phys3(sys_table-boottime-allocate_pool,
-   EFI_LOADER_DATA, size, rom);
+   EFI_RUNTIME_SERVICES_DATA, size, rom);
 
if (status != EFI_SUCCESS)
continue;
@@ -345,9 +345,9 @@ static efi_status_t setup_efi_pci(struct
memcpy(rom-romdata, pci-romimage, pci-romsize);
 
if (data)
-   data-next = (uint64_t)rom;
+   data-next = (unsigned long)rom;
else
-   params-hdr.setup_data = (uint64_t)rom;
+   params-hdr.setup_data = (unsigned long)rom;
 
data = (struct setup_data *)rom;
 



--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix setup_efi_pci()

[Jan Beulich]

 On 17.01.13 at 13:29, Matt Fleming m...@console-pimps.org wrote:
 On Mon, 2013-01-14 at 08:59 +, Jan Beulich wrote:
 This fixes three issues:
 - missing parentheses around a flag test
 - wrong memory type used for allocation intended to persist post-boot
 - four similar build warnings on 32-bit (casts between different size
   pointers and integers)
 
 Signed-off-by: Jan Beulich jbeul...@suse.com
 
 ---
  arch/x86/boot/compressed/eboot.c |   12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)
 
 Jan, could you resubmit items 2 and 3 as separate patches please?

Sure, will do (probably only after rc4 though).

 Item 1 on your list has already been fixed in Linus' tree,

Yet I know I checked the tree right before sending...

Jan

 commit 886d751a2ea99a160f2d0a472231566d9cb0cf58
 Author: Sasha Levin sasha.le...@oracle.com
 Date:   Thu Dec 20 14:11:33 2012 -0500
 
 x86, efi: correct precedence of operators in setup_efi_pci
 
 With the current code, the condition in the if() doesn't make much sense 
 due
 precedence of operators.
 
 Signed-off-by: Sasha Levin sasha.le...@oracle.com
 Link: 
 http://lkml.kernel.org/r/1356030701-16284-25-git-send-email-sasha.levi 
 Cc: Matt Fleming matt.flem...@intel.com
 Cc: Matthew Garrett m...@redhat.com
 Cc: Bjorn Helgaas bhelg...@google.com
 Cc: Seth Forshee seth.fors...@canonical.com
 Signed-off-by: H. Peter Anvin h...@linux.intel.com
 
 -- 
 Matt Fleming, Intel Open Source Technology Center



--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] x86/EFI: check table header length in efi_bgrt_init()

[Jan Beulich]

 On 13.11.12 at 21:08, Matt Fleming m...@console-pimps.org wrote:
 On Wed, 2012-11-07 at 16:46 +, Jan Beulich wrote:
 Header length should be validated for all ACPI tables before accessing
 any non-header field.
 
 Signed-off-by: Jan Beulich jbeul...@suse.com
 
 ---
  arch/x86/platform/efi/efi-bgrt.c |2 ++
  1 file changed, 2 insertions(+)
 
 --- 3.7-rc4/arch/x86/platform/efi/efi-bgrt.c
 +++ 3.7-rc4-x86-EFI-BGRT-checks/arch/x86/platform/efi/efi-bgrt.c
 @@ -39,6 +39,8 @@ void efi_bgrt_init(void)
  if (ACPI_FAILURE(status))
  return;
  
 +if (bgrt_tab-header.length  sizeof(*bgrt_tab))
 +return;
  if (bgrt_tab-version != 1)
  return;
  if (bgrt_tab-image_type != 0 || !bgrt_tab-image_address)
 
 Guys, do you want me to take this into the efi tree? Jan, have you see
 machines that actually trip up without this check? I'm trying to gauge
 the urgency of this patch.

No, I haven't. I just spotted that omission in the context of the
apparent lack of checking of the valid flag (which meanwhile was
explained to me as being intentional).

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] x86/EFI: additional checks in efi_bgrt_init()

[Jan Beulich]

 On 06.11.12 at 09:57, Jan Beulich wrote:
 On 05.11.12 at 20:00, Josh Triplett j...@joshtriplett.org wrote:
  On Mon, Nov 05, 2012 at 06:43:46PM +, Matthew Garrett wrote:
  On Mon, Nov 05, 2012 at 10:37:52AM -0800, Josh Triplett wrote:
   On Mon, Nov 05, 2012 at 03:26:41PM +, Jan Beulich wrote:
Header length should be validated for all ACPI tables before accessing
any non-header field.

The valid flags should also be check, as with it clear there's no point
in trying to go through the rest of the code (and there's no guarantee
that the other table contents are valid/consistent in that case).

Signed-off-by: Jan Beulich jbeul...@suse.com
   
   The length check seems reasonable.  However, Matthew Garrett (already
   CCed) previously suggested to me that this code should not check the
   valid bit, and should instead present the information to userspace if
   otherwise valid (such as having image_address != 0).  Matthew?
  
  Yeah, my interpretation of the spec is that valid indicates whether or 
  not the contents represent what's currently on the screen, not whether 
  or not the contents can be interpreted for other reasons.
 
 That would be nice to get clarified.
 
  Given that, in the absence of a real BIOS that interprets the spec
  differently than that, it sounds like we should drop the check for the
  valid bit.
  
  Jan, have you seen a real BIOS which disagrees with the above
  interpretation?
 
 I would first have to check the rest of the data when the valid
 bit is clear on the two system I have (out of which one may not
 support BGRT at all).

On my system, the table points to EfiReservedMemoryType
and appears to have valid data despite the valid flag being
clear. But before encoding the invalidation into Xen in a form
other than clearing the valid bit, I'd really like to get
confirmation that e.g. clearing the image address is indeed a
proper thing to do. If no-one among you can officially tell
what the intentions here are, would you know who can?

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] x86/EFI: check table header length in efi_bgrt_init()

[Jan Beulich]

Header length should be validated for all ACPI tables before accessing
any non-header field.

Signed-off-by: Jan Beulich jbeul...@suse.com

---
 arch/x86/platform/efi/efi-bgrt.c |2 ++
 1 file changed, 2 insertions(+)

--- 3.7-rc4/arch/x86/platform/efi/efi-bgrt.c
+++ 3.7-rc4-x86-EFI-BGRT-checks/arch/x86/platform/efi/efi-bgrt.c
@@ -39,6 +39,8 @@ void efi_bgrt_init(void)
if (ACPI_FAILURE(status))
return;
 
+   if (bgrt_tab-header.length  sizeof(*bgrt_tab))
+   return;
if (bgrt_tab-version != 1)
return;
if (bgrt_tab-image_type != 0 || !bgrt_tab-image_address)



--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] x86/EFI: additional checks in efi_bgrt_init()

[Jan Beulich]

 On 06.11.12 at 13:55, Josh Triplett j...@joshtriplett.org wrote:
 On Tue, Nov 06, 2012 at 08:57:06AM +, Jan Beulich wrote:
 The question here is how to properly invalidate that data then:
 So far I was assuming that clearing the valid bit would be the way
 to go, as the specification says nothing on e.g. the image address
 being zero having any specific meaning. I need to do that in Xen,
 at least for the time being, as I'm not inclined to postpone
 indefinitely the use of boot services memory for normal memory
 allocations (which we would have to do if we wanted Dom0 to
 be able to access the image pointed to here).
 
 This doesn't postpone the use of boot services memory indefinitely; the
 BGRT driver copies the image out, and the kernel then frees boot
 services memory.  That does introduce a delay in the use of boot
 services memory, but not an indefinite one.

If and when Dom0 tells the hypervisor that it's done using that
data is unknown from the hypervisor perspective, so for Xen
this _is_ indefinitely.

 I was quite bad a design decision to allow (and even suggest)
 the image to live in boot services memory - one can't generally
 expect the ACPI parser to become available in an OS before
 setting up memory allocation. To Linux this already has turned
 out to be a problem, on Xen dealing with this would turn out
 even more cumbersome.
 
 What problems has this caused?

None so far - I'm trying to prevent any from occurring.

Or if you're referring to my use of the term problems above - I
was referring to the fact that for this very reason the treatment
of boot services memory had to be changed in Linux.

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] x86/EFI: additional checks in efi_bgrt_init()

[Jan Beulich]

Header length should be validated for all ACPI tables before accessing
any non-header field.

The valid flags should also be check, as with it clear there's no point
in trying to go through the rest of the code (and there's no guarantee
that the other table contents are valid/consistent in that case).

Signed-off-by: Jan Beulich jbeul...@suse.com

---
 arch/x86/platform/efi/efi-bgrt.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- 3.7-rc4/arch/x86/platform/efi/efi-bgrt.c
+++ 3.7-rc4-x86-EFI-BGRT-checks/arch/x86/platform/efi/efi-bgrt.c
@@ -39,7 +39,9 @@ void efi_bgrt_init(void)
if (ACPI_FAILURE(status))
return;
 
-   if (bgrt_tab-version != 1)
+   if (bgrt_tab-header.length  sizeof(*bgrt_tab))
+   return;
+   if (bgrt_tab-version != 1 || !(bgrt_tab-status  1))
return;
if (bgrt_tab-image_type != 0 || !bgrt_tab-image_address)
return;



--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html