[SeaBIOS] [PATCH 5/6] bios_version: Remove misinterpreted version string

Sam Eiderman posted 6 patches 18 weeks ago

[SeaBIOS] [PATCH 5/6] bios_version: Remove misinterpreted version string

Posted by Sam Eiderman 18 weeks ago
From: Liran Alon <liran.alon@oracle.com>

This is a workaround for the Windows kernel wrongly extracting
SystemBiosVersion.

Windows kernel extracts various BIOS information at boot-time.
The method it use to extract SystemBiosVersion is very hueristic.
It is done by nt!CmpGetBiosVersion().

nt!CmpGetBiosVersion() works by scanning all BIOS memory
from 0xF0000 to 0xFFFFF in search for a string of the form x.y
where x & y are digits. When it finds such a string, it goes
a bunch of characters backwards until an unknown character is reached,
checks whether the string contains any of "v 0", "v 1", "Rev ", etc...
if it does - a match was found. It then continues to find the next
matches.

In our case, this lead to a debug-print string
"Intel IGD BDSM enabled at 0x%08x, size %lldMB, dev 00:02.0"
to be treated as BIOS version (Because of "2.0" at the end, and the
"v 0" contained in it).

This can be seen by:
    * Typing "wmic bios get biosversion" in CMD
    * Reading "HKLM\HARDWARE\DESCRIPTION\System" "SystemBiosVersion"

Therefore, this commit solves the issue by just modifying "00:02.0"
to "00:02:00".

For reference implementation of nt!CmpGetBiosVersion(), see ReactOS:
https://doxygen.reactos.org/d5/dd2/i386_2cmhardwr_8c.html

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Arbel Moshe <arbel.moshe@oracle.com>
Signed-off-by: Sam Eiderman <shmuel.eiderman@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 src/fw/pciinit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index c0634bcb..4ab9b724 100644
--- a/src/fw/pciinit.c
+++ b/src/fw/pciinit.c
@@ -328,7 +328,7 @@ static void intel_igd_setup(struct pci_device *dev, void *arg)
         pci_config_writel(bdf, 0x5C, cpu_to_le32((u32)addr));
 
         dprintf(1, "Intel IGD BDSM enabled at 0x%08x, size %lldMB, dev "
-                "00:02.0\n", (u32)addr, bdsm_size >> 20);
+                "00:02:00\n", (u32)addr, bdsm_size >> 20);
     }
 }
 
-- 
2.13.3
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 5/6] bios_version: Remove misinterpreted version string

Posted by Carl-Daniel Hailfinger 18 weeks ago
Am 17.05.19 um 22:57 schrieb Sam Eiderman:
> From: Liran Alon <liran.alon@oracle.com>
>
> This is a workaround for the Windows kernel wrongly extracting
> SystemBiosVersion.
>
> Windows kernel extracts various BIOS information at boot-time.
> The method it use to extract SystemBiosVersion is very hueristic.
> It is done by nt!CmpGetBiosVersion().
>
> nt!CmpGetBiosVersion() works by scanning all BIOS memory
> from 0xF0000 to 0xFFFFF in search for a string of the form x.y
> where x & y are digits. When it finds such a string, it goes
> a bunch of characters backwards until an unknown character is reached,
> checks whether the string contains any of "v 0", "v 1", "Rev ", etc...
> if it does - a match was found. It then continues to find the next
> matches.
>
> In our case, this lead to a debug-print string
> "Intel IGD BDSM enabled at 0x%08x, size %lldMB, dev 00:02.0"
> to be treated as BIOS version (Because of "2.0" at the end, and the
> "v 0" contained in it).
>
> This can be seen by:
>      * Typing "wmic bios get biosversion" in CMD
>      * Reading "HKLM\HARDWARE\DESCRIPTION\System" "SystemBiosVersion"
>
> Therefore, this commit solves the issue by just modifying "00:02.0"
> to "00:02:00".
>
> For reference implementation of nt!CmpGetBiosVersion(), see ReactOS:
> https://doxygen.reactos.org/d5/dd2/i386_2cmhardwr_8c.html
>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Arbel Moshe <arbel.moshe@oracle.com>
> Signed-off-by: Sam Eiderman <shmuel.eiderman@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>   src/fw/pciinit.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index c0634bcb..4ab9b724 100644
> --- a/src/fw/pciinit.c
> +++ b/src/fw/pciinit.c
> @@ -328,7 +328,7 @@ static void intel_igd_setup(struct pci_device *dev, void *arg)
>           pci_config_writel(bdf, 0x5C, cpu_to_le32((u32)addr));
>
>           dprintf(1, "Intel IGD BDSM enabled at 0x%08x, size %lldMB, dev "
> -                "00:02.0\n", (u32)addr, bdsm_size >> 20);
> +                "00:02:00\n", (u32)addr, bdsm_size >> 20);
>       }
>   }
>

This changed notation for a PCI device looks weird, and AFAICS it is
also inconsistent with other places printing PCI device locations.
Replacing "dev " with "device " would fix the issue as well, but people
grepping for "dev " would have a problem.

Regards,
Carl-Daniel
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 5/6] bios_version: Remove misinterpreted version string

Posted by Shmuel Eiderman 18 weeks ago
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 5/6] bios_version: Remove misinterpreted version string

Posted by Shmuel Eiderman 18 weeks ago
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 5/6] bios_version: Remove misinterpreted version string

Posted by Carl-Daniel Hailfinger 18 weeks ago
What about this?

diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index c0634bcb..4ab9b724 100644
--- a/src/fw/pciinit.c
+++ b/src/fw/pciinit.c
@@ -328,7 +328,7 @@ static void intel_igd_setup(struct pci_device *dev,
void *arg)
           pci_config_writel(bdf, 0x5C, cpu_to_le32((u32)addr));

           dprintf(1, "Intel IGD BDSM enabled at 0x%08x, size %lldMB, dev "
-                "00:02.0\n", (u32)addr, bdsm_size >> 20);
+                "%s\n", (u32)addr, bdsm_size >> 20, "00:02.0");
       }
   }


Regards,
Carl-Daniel

Am 18.05.19 um 13:26 schrieb Shmuel Eiderman:
> Or better yet, replacing the numbers with format string specifiers,
> not changing the string at all.
>
> Sam
>
> On May 18, 2019 14:20, Shmuel Eiderman <shmuel.eiderman@oracle.com> wrote:
>
>     True, "device" is a good option.
>     What about "size: , dev:" (with colon - grepping for "dev " will
>     still not work) or "dev  " (two spaces - grepping will work but
>     debug string looks a bit weird).
>
>     Sam
>
>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 5/6] bios_version: Remove misinterpreted version string

Posted by Sam Eiderman 18 weeks ago
I wanted it to look more consistent with a dprintf just a few lines above:

dprintf(1, "Intel IGD OpRegion enabled at 0x%08x, size %dKB, dev "
        "%02x:%02x.%x\n", (u32)addr, opregion->size >> 10,
        pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf));

What do you think?

Sam



> On 18 May 2019, at 21:28, Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:
> 
> What about this?
> 
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index c0634bcb..4ab9b724 100644
> --- a/src/fw/pciinit.c
> +++ b/src/fw/pciinit.c
> @@ -328,7 +328,7 @@ static void intel_igd_setup(struct pci_device *dev,
> void *arg)
>           pci_config_writel(bdf, 0x5C, cpu_to_le32((u32)addr));
> 
>           dprintf(1, "Intel IGD BDSM enabled at 0x%08x, size %lldMB, dev "
> -                "00:02.0\n", (u32)addr, bdsm_size >> 20);
> +                "%s\n", (u32)addr, bdsm_size >> 20, "00:02.0");
>       }
>   }
> 
> 
> Regards,
> Carl-Daniel
> 
> Am 18.05.19 um 13:26 schrieb Shmuel Eiderman:
>> Or better yet, replacing the numbers with format string specifiers,
>> not changing the string at all.
>> 
>> Sam
>> 
>> On May 18, 2019 14:20, Shmuel Eiderman <shmuel.eiderman@oracle.com> wrote:
>> 
>>    True, "device" is a good option.
>>    What about "size: , dev:" (with colon - grepping for "dev " will
>>    still not work) or "dev  " (two spaces - grepping will work but
>>    debug string looks a bit weird).
>> 
>>    Sam
>> 
>> 
> 
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 5/6] bios_version: Remove misinterpreted version string

Posted by Kevin O'Connor 17 weeks ago
On Sat, May 18, 2019 at 09:58:59PM +0300, Sam Eiderman wrote:
> I wanted it to look more consistent with a dprintf just a few lines above:
> 
> dprintf(1, "Intel IGD OpRegion enabled at 0x%08x, size %dKB, dev "
>         "%02x:%02x.%x\n", (u32)addr, opregion->size >> 10,
>         pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf));
> 
> What do you think?

I'd use the dprintf(1, "%pP", dev) form instead.

-Kevin
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 5/6] bios_version: Remove misinterpreted version string

Posted by Sam Eiderman 17 weeks ago
See the v2 implementation of this patch.

I replaced:

---
@@ -328,7 +328,8 @@ static void intel_igd_setup(struct pci_device *dev, void *arg)
        pci_config_writel(bdf, 0x5C, cpu_to_le32((u32)addr));

        dprintf(1, "Intel IGD BDSM enabled at 0x%08x, size %lldMB, dev "
-                "00:02.0\n", (u32)addr, bdsm_size >> 20);
+                "%02x:%02x.%x\n", (u32)addr, bdsm_size >> 20,
+                0, 2, 0);
    }
}
---

So it will look like the format string a few lines above:

dprintf(1, "Intel IGD OpRegion enabled at 0x%08x, size %dKB, dev “
        "%02x:%02x.%x\n", (u32)addr, opregion->size >> 10,
        pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf));

This solves the version problem

Sam


> On 20 May 2019, at 5:10, Kevin O'Connor <kevin@koconnor.net> wrote:
> 
> On Sat, May 18, 2019 at 09:58:59PM +0300, Sam Eiderman wrote:
>> I wanted it to look more consistent with a dprintf just a few lines above:
>> 
>> dprintf(1, "Intel IGD OpRegion enabled at 0x%08x, size %dKB, dev "
>>        "%02x:%02x.%x\n", (u32)addr, opregion->size >> 10,
>>        pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf));
>> 
>> What do you think?
> 
> I'd use the dprintf(1, "%pP", dev) form instead.
> 
> -Kevin

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 5/6] bios_version: Remove misinterpreted version string

Posted by Carl-Daniel Hailfinger 18 weeks ago
Am 18.05.19 um 20:58 schrieb Sam Eiderman:
> I wanted it to look more consistent with a dprintf just a few lines above:
>
> dprintf(1, "Intel IGD OpRegion enabled at 0x%08x, size %dKB, dev "
>          "%02x:%02x.%x\n", (u32)addr, opregion->size >> 10,
>          pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf));
>
> What do you think?

Even better.

> Sam

Regards,
Carl-Daniel

>> On 18 May 2019, at 21:28, Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:
>>
>> What about this?
>>
>> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
>> index c0634bcb..4ab9b724 100644
>> --- a/src/fw/pciinit.c
>> +++ b/src/fw/pciinit.c
>> @@ -328,7 +328,7 @@ static void intel_igd_setup(struct pci_device *dev,
>> void *arg)
>>            pci_config_writel(bdf, 0x5C, cpu_to_le32((u32)addr));
>>
>>            dprintf(1, "Intel IGD BDSM enabled at 0x%08x, size %lldMB, dev "
>> -                "00:02.0\n", (u32)addr, bdsm_size >> 20);
>> +                "%s\n", (u32)addr, bdsm_size >> 20, "00:02.0");
>>        }
>>    }
>>
>>
>> Regards,
>> Carl-Daniel
>>
>> Am 18.05.19 um 13:26 schrieb Shmuel Eiderman:
>>> Or better yet, replacing the numbers with format string specifiers,
>>> not changing the string at all.
>>>
>>> Sam
>>>
>>> On May 18, 2019 14:20, Shmuel Eiderman <shmuel.eiderman@oracle.com> wrote:
>>>
>>>     True, "device" is a good option.
>>>     What about "size: , dev:" (with colon - grepping for "dev " will
>>>     still not work) or "dev  " (two spaces - grepping will work but
>>>     debug string looks a bit weird).
>>>
>>>     Sam
>>>
>>>
>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org