On Mon, Jul 15, 2013 at 8:00 PM, Parag Warudkar <[email protected]> wrote:
> On Mon, Jul 15, 2013 at 7:56 PM, Parag Warudkar <[email protected]> wrote:
>> On Mon, Jul 15, 2013 at 7:04 PM, Josh Triplett <[email protected]> wrote:
>>
>>> We do need to handle the case of a valid pointer into memory that e820
>>> calls system RAM, as well as the case of a valid pointer into memory
>>> reserved for the BIOS or similar, but not the case of an invalid
>>> pointer.
>>
>> Would that be as simple as
>>
>> page_is_ram(
>
> Damn shortcuts -
> virt_addr = phys_to_virt(image->base_address);
> if(page_is_ram(virt_to_page(virt_addr))) {
> //direct read from virt addr
> }
>
> Would that suffice for the System RAM case or some other MM trickery
> is involved?
Ok, so I played around with it a bit and the following patch works
fine on my system. (I.E. image size is reasonable, cat
/sys/firmware/acpi/bgrt/image > img.bmp generates a valid,
non-distorted bitmap, which it did before too, btw as despite of the
ioremap WARN_ON the ioremap seems to succeed if !(is_ram &&
pfn_valid(pfn) && !PageReserved.)
The patch also includes a check for the status bit - if it's not 1,
the boot image cannot be assumed to be valid per the spec, so we just
ignore it in that case.
This also shouldn't impact other machines unless the page_is_ram check
is wrong - but that's copied straight out of ioremap.c!
Signed-off-by: Parag Warudkar <[email protected]>
diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
index 7145ec6..c894047 100644
--- a/arch/x86/platform/efi/efi-bgrt.c
+++ b/arch/x86/platform/efi/efi-bgrt.c
@@ -16,6 +16,8 @@
#include <linux/efi.h>
#include <linux/efi-bgrt.h>
+extern int page_is_ram(unsigned long pfn);
+
struct acpi_table_bgrt *bgrt_tab;
void *__initdata bgrt_image;
size_t __initdata bgrt_image_size;
@@ -31,6 +33,7 @@ void __init efi_bgrt_init(void)
void __iomem *image;
bool ioremapped = false;
struct bmp_header bmp_header;
+ int img_addr_in_ram;
if (acpi_disabled)
return;
@@ -42,25 +45,36 @@ void __init efi_bgrt_init(void)
if (bgrt_tab->header.length < sizeof(*bgrt_tab))
return;
- if (bgrt_tab->version != 1)
+ if (!bgrt_tab->status || bgrt_tab->version != 1)
return;
if (bgrt_tab->image_type != 0 || !bgrt_tab->image_address)
return;
-
- image = efi_lookup_mapped_addr(bgrt_tab->image_address);
- if (!image) {
- image = ioremap(bgrt_tab->image_address, sizeof(bmp_header));
- ioremapped = true;
- if (!image)
- return;
+ /* Before ioremap check if image address falls in System RAM */
+ img_addr_in_ram = page_is_ram(bgrt_tab->image_address >> PAGE_SHIFT);
+ if (img_addr_in_ram) {
+ pr_info("BGRT: Image Address falls in System RAM");
+ image = phys_to_virt(bgrt_tab->image_address);
+ } else {
+ image = efi_lookup_mapped_addr(bgrt_tab->image_address);
+ if (!image) {
+ image = ioremap(bgrt_tab->image_address,
+ sizeof(bmp_header));
+ ioremapped = true;
+ }
}
- memcpy_fromio(&bmp_header, image, sizeof(bmp_header));
+ if (!image)
+ return;
+ if (img_addr_in_ram)
+ memcpy(&bmp_header, image, sizeof(bmp_header));
+ else
+ memcpy_fromio(&bmp_header, image, sizeof(bmp_header));
if (ioremapped)
iounmap(image);
- bgrt_image_size = bmp_header.size;
+ bgrt_image_size = bmp_header.size;
bgrt_image = kmalloc(bgrt_image_size, GFP_KERNEL);
+
if (!bgrt_image)
return;
@@ -72,8 +86,10 @@ void __init efi_bgrt_init(void)
return;
}
}
-
- memcpy_fromio(bgrt_image, image, bgrt_image_size);
+ if (img_addr_in_ram)
+ memcpy(bgrt_image, image, bgrt_image_size);
+ else
+ memcpy_fromio(bgrt_image, image, bgrt_image_size);
if (ioremapped)
iounmap(image);
}
On Tue, Jul 16, 2013 at 3:15 PM, Parag Warudkar <[email protected]> wrote:
> On Mon, Jul 15, 2013 at 8:00 PM, Parag Warudkar <[email protected]> wrote:
>> On Mon, Jul 15, 2013 at 7:56 PM, Parag Warudkar <[email protected]> wrote:
>>> On Mon, Jul 15, 2013 at 7:04 PM, Josh Triplett <[email protected]> wrote:
>>>
>>>> We do need to handle the case of a valid pointer into memory that e820
>>>> calls system RAM, as well as the case of a valid pointer into memory
>>>> reserved for the BIOS or similar, but not the case of an invalid
>>>> pointer.
>>>
>>> Would that be as simple as
>>>
>>> page_is_ram(
>>
>> Damn shortcuts -
>> virt_addr = phys_to_virt(image->base_address);
>> if(page_is_ram(virt_to_page(virt_addr))) {
>> //direct read from virt addr
>> }
>>
>> Would that suffice for the System RAM case or some other MM trickery
>> is involved?
>
> Ok, so I played around with it a bit and the following patch works
> fine on my system. (I.E. image size is reasonable, cat
> /sys/firmware/acpi/bgrt/image > img.bmp generates a valid,
> non-distorted bitmap, which it did before too, btw as despite of the
> ioremap WARN_ON the ioremap seems to succeed if !(is_ram &&
> pfn_valid(pfn) && !PageReserved.)
>
How reliable is this? That is, is there any guarantee that nothing
will have overwritten the image in memory before this code runs?
--Andy
On Tue, Jul 16, 2013 at 4:23 PM, Parag Warudkar <[email protected]> wrote:
>
> On Jul 16, 2013 6:55 PM, "Andy Lutomirski" <[email protected]> wrote:
>
>> > Ok, so I played around with it a bit and the following patch works
>> > fine on my system. (I.E. image size is reasonable, cat
>> > /sys/firmware/acpi/bgrt/image > img.bmp generates a valid,
>> > non-distorted bitmap, which it did before too, btw as despite of the
>> > ioremap WARN_ON the ioremap seems to succeed if !(is_ram &&
>> > pfn_valid(pfn) && !PageReserved.)
>> >
>>
>> How reliable is this? That is, is there any guarantee that nothing
>> will have overwritten the image in memory before this code runs?
>>
> From the little digging I did, this code runs fairly early in the boot
> process, right after ACPI acquires all tables. If I am not mistaken it runs
> as part of efi_late_init which should be before efi_free_boot_services() is
> called.
>
> Image address on my system is 00000000B2E1B018. At boot EFI prints the
> following -
>
> [ 0.000000] efi: mem23: type=4, attr=0xf,
> range=[0x00000000b2c34000-0x00000000b2e5d0
>
> Type=4, again if I am not mistaken is EFI_BOOT_SERVICES_DATA. So all put
> together I think it should be reliable to read off of that address when
> efi-bgrt-init runs, which is before the boot services code and data are
> discarded.
Fair enough. I leave it to the experts to comment on whether there
should be some explicit check of whether this is
EFI_BOOT_SERVICES_DATA.
FWIW, if my board does indeed have a DWORD-swapped address, it's in
plain old RAM (type=7).
--Andy
On Tue, Jul 16, 2013 at 7:32 PM, Andy Lutomirski <[email protected]> wrote:
> Fair enough. I leave it to the experts to comment on whether there
> should be some explicit check of whether this is
> EFI_BOOT_SERVICES_DATA.
>
I am reading the code again and something doesn't sound right. The
original code calls efi_lookup_mapped_address.
It doesn't find the image address there - which is strange given that
on my system the address lies in mem type 4 as printed during boot.
Hmm. But then phys_to_virt on the same address succeeds and we can
read the image off of that.
I might need to do some debugging of efi_lookup_mapped_address to see
why it isn't finding it there. If it did find it there, none of the
ioremap issues would arise.
> FWIW, if my board does indeed have a DWORD-swapped address, it's in
> plain old RAM (type=7).
Yeah, for the status invalid case I think we would be better off just
ignoring it. Too much heuristic trickery required and it might not be
worth it after all that if the firmware didn't bother putting a proper
image in there.
Parag