1. Patchew
  2. Seabios
  3. View series

[SeaBIOS] [PATCH] config: Add function to check if fw_cfg exists

petr.berky@email.cz posted 1 patch 8 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/seabios tags/patchew/zF.4larT.Jl6aGl1O9o.1Oo5En@seznam.cz
There is a newer version of this series
src/fw/paravirt.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
[SeaBIOS] [PATCH] config: Add function to check if fw_cfg exists
Posted by petr.berky@email.cz 8 years, 1 month ago 
From 405de6e571a2bf332452a17ae98f7b3a0613365e Mon Sep 17 00:00:00 2001
From: Petr Berky <petr.berky@email.cz>
Date: Tue, 14 Mar 2017 20:30:52 +0100
Subject: [PATCH] config: Add function to check if fw_cfg exists

It was found qemu_get_present_cpus_count may return impossible
number of cpus because of not checking if fw_cfg exists before
using it. That  may lead to undefined behavior of emulator,
in particular Bochs that freezes.

Signed-off-by: Petr Berky <petr.berky@email.cz>
---
 src/fw/paravirt.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
index 707502d..b2cfc23 100644
--- a/src/fw/paravirt.c
+++ b/src/fw/paravirt.c
@@ -220,6 +220,21 @@ qemu_cfg_select(u16 f)
     outw(f, PORT_QEMU_CFG_CTL);
 }
 
+static int
+qemu_cfg_check_signature(void)
+{
+    int i;
+    char *sig = "QEMU";
+
+    qemu_cfg_select(QEMU_CFG_SIGNATURE);
+    for (i = 0; i < 4; i++) {
+        if (inb(PORT_QEMU_CFG_DATA) != sig[i]) {
+            return -1;
+        }
+    }
+    return 0;
+}
+
 static void
 qemu_cfg_dma_transfer(void *address, u32 length, u32 control)
 {
@@ -392,7 +407,9 @@ u16
 qemu_get_present_cpus_count(void)
 {
     u16 smp_count = 0;
-    qemu_cfg_read_entry(&smp_count, QEMU_CFG_NB_CPUS, sizeof(smp_count));
+    if (qemu_cfg_check_signature() == 0) {
+        qemu_cfg_read_entry(&smp_count, QEMU_CFG_NB_CPUS, sizeof(smp_count));
+    }
     u16 cmos_cpu_count = rtc_read(CMOS_BIOS_SMP_COUNT) + 1;
     if (smp_count < cmos_cpu_count) {
         smp_count = cmos_cpu_count;
@@ -563,12 +580,9 @@ void qemu_cfg_init(void)
         return;
 
     // Detect fw_cfg interface.
-    qemu_cfg_select(QEMU_CFG_SIGNATURE);
-    char *sig = "QEMU";
-    int i;
-    for (i = 0; i < 4; i++)
-        if (inb(PORT_QEMU_CFG_DATA) != sig[i])
-            return;
+    if (qemu_cfg_check_signature() != 0) {
+        return;
+    }
 
     dprintf(1, "Found QEMU fw_cfg\n");
 
-- 
2.11.0


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] config: Add function to check if fw_cfg exists
Posted by Laszlo Ersek 8 years, 1 month ago 
On 03/14/17 21:33, petr.berky@email.cz wrote:
> From 405de6e571a2bf332452a17ae98f7b3a0613365e Mon Sep 17 00:00:00 2001
> From: Petr Berky <petr.berky@email.cz>
> Date: Tue, 14 Mar 2017 20:30:52 +0100
> Subject: [PATCH] config: Add function to check if fw_cfg exists
> 
> It was found qemu_get_present_cpus_count may return impossible
> number of cpus because of not checking if fw_cfg exists before
> using it. That  may lead to undefined behavior of emulator,
> in particular Bochs that freezes.
> 
> Signed-off-by: Petr Berky <petr.berky@email.cz>
> ---
>  src/fw/paravirt.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> index 707502d..b2cfc23 100644
> --- a/src/fw/paravirt.c
> +++ b/src/fw/paravirt.c
> @@ -220,6 +220,21 @@ qemu_cfg_select(u16 f)
>      outw(f, PORT_QEMU_CFG_CTL);
>  }
>  
> +static int
> +qemu_cfg_check_signature(void)
> +{
> +    int i;
> +    char *sig = "QEMU";
> +
> +    qemu_cfg_select(QEMU_CFG_SIGNATURE);
> +    for (i = 0; i < 4; i++) {
> +        if (inb(PORT_QEMU_CFG_DATA) != sig[i]) {
> +            return -1;
> +        }
> +    }
> +    return 0;
> +}
> +
>  static void
>  qemu_cfg_dma_transfer(void *address, u32 length, u32 control)
>  {
> @@ -392,7 +407,9 @@ u16
>  qemu_get_present_cpus_count(void)
>  {
>      u16 smp_count = 0;
> -    qemu_cfg_read_entry(&smp_count, QEMU_CFG_NB_CPUS, sizeof(smp_count));
> +    if (qemu_cfg_check_signature() == 0) {
> +        qemu_cfg_read_entry(&smp_count, QEMU_CFG_NB_CPUS, sizeof(smp_count));
> +    }
>      u16 cmos_cpu_count = rtc_read(CMOS_BIOS_SMP_COUNT) + 1;
>      if (smp_count < cmos_cpu_count) {
>          smp_count = cmos_cpu_count;
> @@ -563,12 +580,9 @@ void qemu_cfg_init(void)
>          return;
>  
>      // Detect fw_cfg interface.
> -    qemu_cfg_select(QEMU_CFG_SIGNATURE);
> -    char *sig = "QEMU";
> -    int i;
> -    for (i = 0; i < 4; i++)
> -        if (inb(PORT_QEMU_CFG_DATA) != sig[i])
> -            return;
> +    if (qemu_cfg_check_signature() != 0) {
> +        return;
> +    }
>  
>      dprintf(1, "Found QEMU fw_cfg\n");
>  
> 

"src/fw/paravirt.c" already has an extern function called
qemu_cfg_dma_enabled(), whic is based on the static global variable
"cfg_dma_enabled", which is set in qemu_cfg_init().

The above is about the DMA interface for fw_cfg. I think it would be a
"natural" extension to add a similar global variable and helper function
(called qemu_cfg_enabled()) for the basic fw_cfg presence as well.

Then qemu_cfg_check_signature() would not be necessary in this patch.

Just an idea of course.

Laszlo

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] config: Add function to check if fw_cfg exists
Posted by Petr Berky 8 years, 1 month ago 
On 03/14/2017 09:46 PM, Laszlo Ersek wrote:
> On 03/14/17 21:33, petr.berky@email.cz wrote:
>> From 405de6e571a2bf332452a17ae98f7b3a0613365e Mon Sep 17 00:00:00 2001
>> From: Petr Berky <petr.berky@email.cz>
>> Date: Tue, 14 Mar 2017 20:30:52 +0100
>> Subject: [PATCH] config: Add function to check if fw_cfg exists
>>
>> It was found qemu_get_present_cpus_count may return impossible
>> number of cpus because of not checking if fw_cfg exists before
>> using it. That  may lead to undefined behavior of emulator,
>> in particular Bochs that freezes.
>>
>> Signed-off-by: Petr Berky <petr.berky@email.cz>
>> ---
>>  src/fw/paravirt.c | 28 +++++++++++++++++++++-------
>>  1 file changed, 21 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
>> index 707502d..b2cfc23 100644
>> --- a/src/fw/paravirt.c
>> +++ b/src/fw/paravirt.c
>> @@ -220,6 +220,21 @@ qemu_cfg_select(u16 f)
>>      outw(f, PORT_QEMU_CFG_CTL);
>>  }
>>
>> +static int
>> +qemu_cfg_check_signature(void)
>> +{
>> +    int i;
>> +    char *sig = "QEMU";
>> +
>> +    qemu_cfg_select(QEMU_CFG_SIGNATURE);
>> +    for (i = 0; i < 4; i++) {
>> +        if (inb(PORT_QEMU_CFG_DATA) != sig[i]) {
>> +            return -1;
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>>  static void
>>  qemu_cfg_dma_transfer(void *address, u32 length, u32 control)
>>  {
>> @@ -392,7 +407,9 @@ u16
>>  qemu_get_present_cpus_count(void)
>>  {
>>      u16 smp_count = 0;
>> -    qemu_cfg_read_entry(&smp_count, QEMU_CFG_NB_CPUS, sizeof(smp_count));
>> +    if (qemu_cfg_check_signature() == 0) {
>> +        qemu_cfg_read_entry(&smp_count, QEMU_CFG_NB_CPUS, sizeof(smp_count));
>> +    }
>>      u16 cmos_cpu_count = rtc_read(CMOS_BIOS_SMP_COUNT) + 1;
>>      if (smp_count < cmos_cpu_count) {
>>          smp_count = cmos_cpu_count;
>> @@ -563,12 +580,9 @@ void qemu_cfg_init(void)
>>          return;
>>
>>      // Detect fw_cfg interface.
>> -    qemu_cfg_select(QEMU_CFG_SIGNATURE);
>> -    char *sig = "QEMU";
>> -    int i;
>> -    for (i = 0; i < 4; i++)
>> -        if (inb(PORT_QEMU_CFG_DATA) != sig[i])
>> -            return;
>> +    if (qemu_cfg_check_signature() != 0) {
>> +        return;
>> +    }
>>
>>      dprintf(1, "Found QEMU fw_cfg\n");
>>
>>
>
> "src/fw/paravirt.c" already has an extern function called
> qemu_cfg_dma_enabled(), whic is based on the static global variable
> "cfg_dma_enabled", which is set in qemu_cfg_init().
>
> The above is about the DMA interface for fw_cfg. I think it would be a
> "natural" extension to add a similar global variable and helper function
> (called qemu_cfg_enabled()) for the basic fw_cfg presence as well.
>
> Then qemu_cfg_check_signature() would not be necessary in this patch.
>
> Just an idea of course.
>
> Laszlo
>

I think you are right. I will prepare a second patch


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH v2] config: Add function to check if fw_cfg exists
Posted by Petr Berky 8 years, 1 month ago 
 From b06589c683a7defb4853a3b810bd7e6a12abe2d6 Mon Sep 17 00:00:00 2001
From: Petr Berky <petr.berky@email.cz>
Date: Tue, 14 Mar 2017 23:32:15 +0100
Subject: [PATCH v2] config: Add function to check if fw_cfg exists

It was found qemu_get_present_cpus_count may return impossible
number of cpus because of not checking if fw_cfg exists before
using it. That  may lead to undefined behavior of emulator,
in particular Bochs that freezes.

Signed-off-by: Petr Berky <petr.berky@email.cz>
---
  src/fw/paravirt.c | 12 +++++++++++-
  src/fw/paravirt.h |  1 +
  2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
index 707502d..dfc69d4 100644
--- a/src/fw/paravirt.c
+++ b/src/fw/paravirt.c
@@ -32,9 +32,16 @@ u32 RamSize;
  u64 RamSizeOver4G;
  // Type of emulator platform.
  int PlatformRunningOn VARFSEG;
+// cfg enabled
+int cfg_enabled = 0;
  // cfg_dma enabled
  int cfg_dma_enabled = 0;

+inline int qemu_cfg_enabled(void)
+{
+    return cfg_enabled;
+}
+
  inline int qemu_cfg_dma_enabled(void)
  {
      return cfg_dma_enabled;
@@ -392,7 +399,9 @@ u16
  qemu_get_present_cpus_count(void)
  {
      u16 smp_count = 0;
-    qemu_cfg_read_entry(&smp_count, QEMU_CFG_NB_CPUS, sizeof(smp_count));
+    if (qemu_cfg_enabled()) {
+        qemu_cfg_read_entry(&smp_count, QEMU_CFG_NB_CPUS, 
sizeof(smp_count));
+    }
      u16 cmos_cpu_count = rtc_read(CMOS_BIOS_SMP_COUNT) + 1;
      if (smp_count < cmos_cpu_count) {
          smp_count = cmos_cpu_count;
@@ -570,6 +579,7 @@ void qemu_cfg_init(void)
          if (inb(PORT_QEMU_CFG_DATA) != sig[i])
              return;

+    cfg_enabled = 1;
      dprintf(1, "Found QEMU fw_cfg\n");

      // Detect DMA interface.
diff --git a/src/fw/paravirt.h b/src/fw/paravirt.h
index 16f3d9a..a14d83e 100644
--- a/src/fw/paravirt.h
+++ b/src/fw/paravirt.h
@@ -49,6 +49,7 @@ static inline int runningOnKVM(void) {
  // QEMU_CFG_DMA ID bit
  #define QEMU_CFG_VERSION_DMA    2

+int qemu_cfg_enabled(void);
  int qemu_cfg_dma_enabled(void);
  void qemu_preinit(void);
  void qemu_platform_setup(void);
-- 
2.11.0



_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH v2] config: Add function to check if fw_cfg exists
Posted by Kevin O'Connor 8 years, 1 month ago 
On Wed, Mar 15, 2017 at 12:09:11AM +0100, Petr Berky wrote:
> From b06589c683a7defb4853a3b810bd7e6a12abe2d6 Mon Sep 17 00:00:00 2001
> From: Petr Berky <petr.berky@email.cz>
> Date: Tue, 14 Mar 2017 23:32:15 +0100
> Subject: [PATCH v2] config: Add function to check if fw_cfg exists
> 
> It was found qemu_get_present_cpus_count may return impossible
> number of cpus because of not checking if fw_cfg exists before
> using it. That  may lead to undefined behavior of emulator,
> in particular Bochs that freezes.

Thanks.  The patch looks fine to me.  However, it looks like it got
whitespace corrupted.  Can you retry with "git send-email", or if
that's not applicable, try attaching the patch.

-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH v2] config: Add function to check if fw_cfg exists
Posted by Petr Berky 8 years, 1 month ago 
On 03/22/2017 04:34 PM, Kevin O'Connor wrote:
> On Wed, Mar 15, 2017 at 12:09:11AM +0100, Petr Berky wrote:
>> From b06589c683a7defb4853a3b810bd7e6a12abe2d6 Mon Sep 17 00:00:00 2001
>> From: Petr Berky <petr.berky@email.cz>
>> Date: Tue, 14 Mar 2017 23:32:15 +0100
>> Subject: [PATCH v2] config: Add function to check if fw_cfg exists
>>
>> It was found qemu_get_present_cpus_count may return impossible
>> number of cpus because of not checking if fw_cfg exists before
>> using it. That  may lead to undefined behavior of emulator,
>> in particular Bochs that freezes.
>
> Thanks.  The patch looks fine to me.  However, it looks like it got
> whitespace corrupted.  Can you retry with "git send-email", or if
> that's not applicable, try attaching the patch.
>
> -Kevin
>

I just send a patch v3 with this tiny change mentioned by Laszlo.
I used "git send-email" as you advised and I hope you will receive it in 
the correct form this time.

-Petr


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH v2] config: Add function to check if fw_cfg exists
Posted by Laszlo Ersek 8 years, 1 month ago 
On 03/15/17 00:09, Petr Berky wrote:
> From b06589c683a7defb4853a3b810bd7e6a12abe2d6 Mon Sep 17 00:00:00 2001
> From: Petr Berky <petr.berky@email.cz>
> Date: Tue, 14 Mar 2017 23:32:15 +0100
> Subject: [PATCH v2] config: Add function to check if fw_cfg exists
> 
> It was found qemu_get_present_cpus_count may return impossible
> number of cpus because of not checking if fw_cfg exists before
> using it. That  may lead to undefined behavior of emulator,
> in particular Bochs that freezes.
> 
> Signed-off-by: Petr Berky <petr.berky@email.cz>
> ---
>  src/fw/paravirt.c | 12 +++++++++++-
>  src/fw/paravirt.h |  1 +
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> index 707502d..dfc69d4 100644
> --- a/src/fw/paravirt.c
> +++ b/src/fw/paravirt.c
> @@ -32,9 +32,16 @@ u32 RamSize;
>  u64 RamSizeOver4G;
>  // Type of emulator platform.
>  int PlatformRunningOn VARFSEG;
> +// cfg enabled
> +int cfg_enabled = 0;
>  // cfg_dma enabled
>  int cfg_dma_enabled = 0;
> 
> +inline int qemu_cfg_enabled(void)
> +{
> +    return cfg_enabled;
> +}
> +
>  inline int qemu_cfg_dma_enabled(void)
>  {
>      return cfg_dma_enabled;
> @@ -392,7 +399,9 @@ u16
>  qemu_get_present_cpus_count(void)
>  {
>      u16 smp_count = 0;
> -    qemu_cfg_read_entry(&smp_count, QEMU_CFG_NB_CPUS, sizeof(smp_count));
> +    if (qemu_cfg_enabled()) {
> +        qemu_cfg_read_entry(&smp_count, QEMU_CFG_NB_CPUS,
> sizeof(smp_count));
> +    }
>      u16 cmos_cpu_count = rtc_read(CMOS_BIOS_SMP_COUNT) + 1;
>      if (smp_count < cmos_cpu_count) {
>          smp_count = cmos_cpu_count;
> @@ -570,6 +579,7 @@ void qemu_cfg_init(void)
>          if (inb(PORT_QEMU_CFG_DATA) != sig[i])
>              return;
> 
> +    cfg_enabled = 1;
>      dprintf(1, "Found QEMU fw_cfg\n");
> 
>      // Detect DMA interface.

If we wanted to parallel the DMA check 100%, we'd set the variable under
the debug message, not above it, but even I am not that pedantic. :)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Igor, can you check if this is safe for S3 resume too? I think it is,
but I had better ask you.

Thanks
Laszlo


> diff --git a/src/fw/paravirt.h b/src/fw/paravirt.h
> index 16f3d9a..a14d83e 100644
> --- a/src/fw/paravirt.h
> +++ b/src/fw/paravirt.h
> @@ -49,6 +49,7 @@ static inline int runningOnKVM(void) {
>  // QEMU_CFG_DMA ID bit
>  #define QEMU_CFG_VERSION_DMA    2
> 
> +int qemu_cfg_enabled(void);
>  int qemu_cfg_dma_enabled(void);
>  void qemu_preinit(void);
>  void qemu_platform_setup(void);


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.