[PATCH 15/16] tests: bios-tables-test: Add test for smbios type4 thread count2

Zhao Liu posted 16 patches 2 years, 3 months ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <anisinha@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>
There is a newer version of this series
[PATCH 15/16] tests: bios-tables-test: Add test for smbios type4 thread count2
Posted by Zhao Liu 2 years, 3 months ago
From: Zhao Liu <zhao1.liu@intel.com>

This tests the commit 7298fd7de5551 ("hw/smbios: Fix thread count in
type4").

Add this test to cover 2 cases:
1. Test thread count2 field with multiple sockets and multiple dies to
   confirm this field could correctly calculate threads per sockets.

2. Confirm that field calculation could correctly recognize the
   difference between "-smp maxcpus" and "-smp cpus".

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 tests/qtest/bios-tables-test.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 26474d376633..1b0c27e95d26 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -96,6 +96,7 @@ typedef struct {
     uint8_t smbios_core_count;
     uint16_t smbios_core_count2;
     uint8_t smbios_thread_count;
+    uint16_t smbios_thread_count2;
     uint8_t *required_struct_types;
     int required_struct_types_len;
     int type4_count;
@@ -644,6 +645,7 @@ static void smbios_cpu_test(test_data *data, uint32_t addr,
     uint8_t thread_count, expected_thread_count = data->smbios_thread_count;
     uint16_t speed, expected_speed[2];
     uint16_t core_count2, expected_core_count2 = data->smbios_core_count2;
+    uint16_t thread_count2, expected_thread_count2 = data->smbios_thread_count2;
     int offset[2];
     int i;
 
@@ -673,6 +675,8 @@ static void smbios_cpu_test(test_data *data, uint32_t addr,
     }
 
     if (ep_type == SMBIOS_ENTRY_POINT_TYPE_64) {
+        uint64_t thread_count2_addr;
+
         core_count2 = qtest_readw(data->qts,
                           addr + offsetof(struct smbios_type_4, core_count2));
 
@@ -680,6 +684,15 @@ static void smbios_cpu_test(test_data *data, uint32_t addr,
         if (expected_core_count == 0xFF && expected_core_count2) {
             g_assert_cmpuint(core_count2, ==, expected_core_count2);
         }
+
+        thread_count2_addr = addr +
+                             offsetof(struct smbios_type_4, thread_count2);
+        thread_count2 = qtest_readw(data->qts, thread_count2_addr);
+
+        /* Thread Count has reached its limit, checking Thread Count 2 */
+        if (expected_thread_count == 0xFF && expected_thread_count2) {
+            g_assert_cmpuint(thread_count2, ==, expected_thread_count2);
+        }
     }
 }
 
@@ -1050,6 +1063,7 @@ static void test_acpi_q35_tcg_thread_count(void)
         .required_struct_types = base_required_struct_types,
         .required_struct_types_len = ARRAY_SIZE(base_required_struct_types),
         .smbios_thread_count = 27,
+        .smbios_thread_count2 = 27,
     };
 
     test_acpi_one("-machine smbios-entry-point-type=64 "
@@ -1058,6 +1072,23 @@ static void test_acpi_q35_tcg_thread_count(void)
     free_test_data(&data);
 }
 
+static void test_acpi_q35_tcg_thread_count2(void)
+{
+    test_data data = {
+        .machine = MACHINE_Q35,
+        .variant = ".thread-count2",
+        .required_struct_types = base_required_struct_types,
+        .required_struct_types_len = ARRAY_SIZE(base_required_struct_types),
+        .smbios_thread_count = 0xFF,
+        .smbios_thread_count2 = 260,
+    };
+
+    test_acpi_one("-machine smbios-entry-point-type=64 "
+                  "-smp cpus=210,maxcpus=520,sockets=2,dies=2,cores=65,threads=2",
+                  &data);
+    free_test_data(&data);
+}
+
 static void test_acpi_q35_tcg_bridge(void)
 {
     test_data data = {};
@@ -2216,6 +2247,8 @@ int main(int argc, char *argv[])
                                test_acpi_q35_tcg_core_count2);
                 qtest_add_func("acpi/q35/thread-count",
                                test_acpi_q35_tcg_thread_count);
+                qtest_add_func("acpi/q35/thread-count2",
+                               test_acpi_q35_tcg_thread_count2);
             }
             qtest_add_func("acpi/q35/viot", test_acpi_q35_viot);
 #ifdef CONFIG_POSIX
-- 
2.34.1
Re: [PATCH 15/16] tests: bios-tables-test: Add test for smbios type4 thread count2
Posted by Igor Mammedov 2 years, 3 months ago
On Fri, 25 Aug 2023 11:36:18 +0800
Zhao Liu <zhao1.liu@linux.intel.com> wrote:

> From: Zhao Liu <zhao1.liu@intel.com>
> 
> This tests the commit 7298fd7de5551 ("hw/smbios: Fix thread count in
> type4").
> 
> Add this test to cover 2 cases:
> 1. Test thread count2 field with multiple sockets and multiple dies to
>    confirm this field could correctly calculate threads per sockets.
> 
> 2. Confirm that field calculation could correctly recognize the
>    difference between "-smp maxcpus" and "-smp cpus".
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  tests/qtest/bios-tables-test.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 26474d376633..1b0c27e95d26 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -96,6 +96,7 @@ typedef struct {
>      uint8_t smbios_core_count;
>      uint16_t smbios_core_count2;
>      uint8_t smbios_thread_count;
> +    uint16_t smbios_thread_count2;
>      uint8_t *required_struct_types;
>      int required_struct_types_len;
>      int type4_count;
> @@ -644,6 +645,7 @@ static void smbios_cpu_test(test_data *data, uint32_t addr,
>      uint8_t thread_count, expected_thread_count = data->smbios_thread_count;
>      uint16_t speed, expected_speed[2];
>      uint16_t core_count2, expected_core_count2 = data->smbios_core_count2;
> +    uint16_t thread_count2, expected_thread_count2 = data->smbios_thread_count2;
>      int offset[2];
>      int i;
>  
> @@ -673,6 +675,8 @@ static void smbios_cpu_test(test_data *data, uint32_t addr,
>      }
>  
>      if (ep_type == SMBIOS_ENTRY_POINT_TYPE_64) {
> +        uint64_t thread_count2_addr;
> +
>          core_count2 = qtest_readw(data->qts,
>                            addr + offsetof(struct smbios_type_4, core_count2));
>  
> @@ -680,6 +684,15 @@ static void smbios_cpu_test(test_data *data, uint32_t addr,
>          if (expected_core_count == 0xFF && expected_core_count2) {
>              g_assert_cmpuint(core_count2, ==, expected_core_count2);
>          }
> +
> +        thread_count2_addr = addr +
> +                             offsetof(struct smbios_type_4, thread_count2);
> +        thread_count2 = qtest_readw(data->qts, thread_count2_addr);

I'd mimic the same code style as used for core_count2 and avoid introducing an extra variable

> +
> +        /* Thread Count has reached its limit, checking Thread Count 2 */
> +        if (expected_thread_count == 0xFF && expected_thread_count2) {
> +            g_assert_cmpuint(thread_count2, ==, expected_thread_count2);
> +        }
>      }
>  }
>  
> @@ -1050,6 +1063,7 @@ static void test_acpi_q35_tcg_thread_count(void)
>          .required_struct_types = base_required_struct_types,
>          .required_struct_types_len = ARRAY_SIZE(base_required_struct_types),
>          .smbios_thread_count = 27,
> +        .smbios_thread_count2 = 27,
>      };
>  
>      test_acpi_one("-machine smbios-entry-point-type=64 "
> @@ -1058,6 +1072,23 @@ static void test_acpi_q35_tcg_thread_count(void)
>      free_test_data(&data);
>  }
>  
> +static void test_acpi_q35_tcg_thread_count2(void)
> +{
> +    test_data data = {
> +        .machine = MACHINE_Q35,
> +        .variant = ".thread-count2",
> +        .required_struct_types = base_required_struct_types,
> +        .required_struct_types_len = ARRAY_SIZE(base_required_struct_types),
> +        .smbios_thread_count = 0xFF,
> +        .smbios_thread_count2 = 260,
> +    };
> +
> +    test_acpi_one("-machine smbios-entry-point-type=64 "
> +                  "-smp cpus=210,maxcpus=520,sockets=2,dies=2,cores=65,threads=2",
> +                  &data);

explain in commit message why abive -smp == 
  > +        .smbios_thread_count = 0xFF,
  > +        .smbios_thread_count2 = 260,


> +    free_test_data(&data);
> +}
> +
>  static void test_acpi_q35_tcg_bridge(void)
>  {
>      test_data data = {};
> @@ -2216,6 +2247,8 @@ int main(int argc, char *argv[])
>                                 test_acpi_q35_tcg_core_count2);
>                  qtest_add_func("acpi/q35/thread-count",
>                                 test_acpi_q35_tcg_thread_count);
> +                qtest_add_func("acpi/q35/thread-count2",
> +                               test_acpi_q35_tcg_thread_count2);
>              }
>              qtest_add_func("acpi/q35/viot", test_acpi_q35_viot);
>  #ifdef CONFIG_POSIX
Re: [PATCH 15/16] tests: bios-tables-test: Add test for smbios type4 thread count2
Posted by Zhao Liu 2 years, 2 months ago
Hi Igor,

On Fri, Sep 15, 2023 at 03:29:07PM +0200, Igor Mammedov wrote:
> Date: Fri, 15 Sep 2023 15:29:07 +0200
> From: Igor Mammedov <imammedo@redhat.com>
> Subject: Re: [PATCH 15/16] tests: bios-tables-test: Add test for smbios
>  type4 thread count2
> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-redhat-linux-gnu)
> 
> On Fri, 25 Aug 2023 11:36:18 +0800
> Zhao Liu <zhao1.liu@linux.intel.com> wrote:
> 
> > From: Zhao Liu <zhao1.liu@intel.com>
> > 
> > This tests the commit 7298fd7de5551 ("hw/smbios: Fix thread count in
> > type4").
> > 
> > Add this test to cover 2 cases:
> > 1. Test thread count2 field with multiple sockets and multiple dies to
> >    confirm this field could correctly calculate threads per sockets.
> > 
> > 2. Confirm that field calculation could correctly recognize the
> >    difference between "-smp maxcpus" and "-smp cpus".
> > 
> > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >  tests/qtest/bios-tables-test.c | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> > index 26474d376633..1b0c27e95d26 100644
> > --- a/tests/qtest/bios-tables-test.c
> > +++ b/tests/qtest/bios-tables-test.c
> > @@ -96,6 +96,7 @@ typedef struct {
> >      uint8_t smbios_core_count;
> >      uint16_t smbios_core_count2;
> >      uint8_t smbios_thread_count;
> > +    uint16_t smbios_thread_count2;
> >      uint8_t *required_struct_types;
> >      int required_struct_types_len;
> >      int type4_count;
> > @@ -644,6 +645,7 @@ static void smbios_cpu_test(test_data *data, uint32_t addr,
> >      uint8_t thread_count, expected_thread_count = data->smbios_thread_count;
> >      uint16_t speed, expected_speed[2];
> >      uint16_t core_count2, expected_core_count2 = data->smbios_core_count2;
> > +    uint16_t thread_count2, expected_thread_count2 = data->smbios_thread_count2;
> >      int offset[2];
> >      int i;
> >  
> > @@ -673,6 +675,8 @@ static void smbios_cpu_test(test_data *data, uint32_t addr,
> >      }
> >  
> >      if (ep_type == SMBIOS_ENTRY_POINT_TYPE_64) {
> > +        uint64_t thread_count2_addr;
> > +
> >          core_count2 = qtest_readw(data->qts,
> >                            addr + offsetof(struct smbios_type_4, core_count2));
> >  
> > @@ -680,6 +684,15 @@ static void smbios_cpu_test(test_data *data, uint32_t addr,
> >          if (expected_core_count == 0xFF && expected_core_count2) {
> >              g_assert_cmpuint(core_count2, ==, expected_core_count2);
> >          }
> > +
> > +        thread_count2_addr = addr +
> > +                             offsetof(struct smbios_type_4, thread_count2);
> > +        thread_count2 = qtest_readw(data->qts, thread_count2_addr);
> 
> I'd mimic the same code style as used for core_count2 and avoid introducing an extra variable

I'm not sure about the style of this case, since the code line is still
too long, so which style should I pick? ;-)

thread_count2 = qtest_readw(data->qts,
                    addr + offsetof(struct smbios_type_4,
		                    thread_count2));

or,

thread_count2 = qtest_readw(data->qts,
                    addr + offsetof(struct smbios_type_4,
                    thread_count2));


> 
> > +
> > +        /* Thread Count has reached its limit, checking Thread Count 2 */
> > +        if (expected_thread_count == 0xFF && expected_thread_count2) {
> > +            g_assert_cmpuint(thread_count2, ==, expected_thread_count2);
> > +        }
> >      }
> >  }
> >  
> > @@ -1050,6 +1063,7 @@ static void test_acpi_q35_tcg_thread_count(void)
> >          .required_struct_types = base_required_struct_types,
> >          .required_struct_types_len = ARRAY_SIZE(base_required_struct_types),
> >          .smbios_thread_count = 27,
> > +        .smbios_thread_count2 = 27,
> >      };
> >  
> >      test_acpi_one("-machine smbios-entry-point-type=64 "
> > @@ -1058,6 +1072,23 @@ static void test_acpi_q35_tcg_thread_count(void)
> >      free_test_data(&data);
> >  }
> >  
> > +static void test_acpi_q35_tcg_thread_count2(void)
> > +{
> > +    test_data data = {
> > +        .machine = MACHINE_Q35,
> > +        .variant = ".thread-count2",
> > +        .required_struct_types = base_required_struct_types,
> > +        .required_struct_types_len = ARRAY_SIZE(base_required_struct_types),
> > +        .smbios_thread_count = 0xFF,
> > +        .smbios_thread_count2 = 260,
> > +    };
> > +
> > +    test_acpi_one("-machine smbios-entry-point-type=64 "
> > +                  "-smp cpus=210,maxcpus=520,sockets=2,dies=2,cores=65,threads=2",
> > +                  &data);
> 
> explain in commit message why abive -smp == 

Ok, this is used to test if we could correctly distinguish smp.cpus and smp.maxcpus.

Thanks,
Zhao

>   > +        .smbios_thread_count = 0xFF,
>   > +        .smbios_thread_count2 = 260,
> 
> 
> > +    free_test_data(&data);
> > +}
> > +
> >  static void test_acpi_q35_tcg_bridge(void)
> >  {
> >      test_data data = {};
> > @@ -2216,6 +2247,8 @@ int main(int argc, char *argv[])
> >                                 test_acpi_q35_tcg_core_count2);
> >                  qtest_add_func("acpi/q35/thread-count",
> >                                 test_acpi_q35_tcg_thread_count);
> > +                qtest_add_func("acpi/q35/thread-count2",
> > +                               test_acpi_q35_tcg_thread_count2);
> >              }
> >              qtest_add_func("acpi/q35/viot", test_acpi_q35_viot);
> >  #ifdef CONFIG_POSIX
>
Re: [PATCH 15/16] tests: bios-tables-test: Add test for smbios type4 thread count2
Posted by Igor Mammedov 2 years, 2 months ago
On Tue, 19 Sep 2023 15:12:13 +0800
Zhao Liu <zhao1.liu@linux.intel.com> wrote:

> Hi Igor,
> 
> On Fri, Sep 15, 2023 at 03:29:07PM +0200, Igor Mammedov wrote:
> > Date: Fri, 15 Sep 2023 15:29:07 +0200
> > From: Igor Mammedov <imammedo@redhat.com>
> > Subject: Re: [PATCH 15/16] tests: bios-tables-test: Add test for smbios
> >  type4 thread count2
> > X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-redhat-linux-gnu)
> > 
> > On Fri, 25 Aug 2023 11:36:18 +0800
> > Zhao Liu <zhao1.liu@linux.intel.com> wrote:
> >   
> > > From: Zhao Liu <zhao1.liu@intel.com>
> > > 
> > > This tests the commit 7298fd7de5551 ("hw/smbios: Fix thread count in
> > > type4").
> > > 
> > > Add this test to cover 2 cases:
> > > 1. Test thread count2 field with multiple sockets and multiple dies to
> > >    confirm this field could correctly calculate threads per sockets.
> > > 
> > > 2. Confirm that field calculation could correctly recognize the
> > >    difference between "-smp maxcpus" and "-smp cpus".
> > > 
> > > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > > ---
> > >  tests/qtest/bios-tables-test.c | 33 +++++++++++++++++++++++++++++++++
> > >  1 file changed, 33 insertions(+)
> > > 
> > > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> > > index 26474d376633..1b0c27e95d26 100644
> > > --- a/tests/qtest/bios-tables-test.c
> > > +++ b/tests/qtest/bios-tables-test.c
> > > @@ -96,6 +96,7 @@ typedef struct {
> > >      uint8_t smbios_core_count;
> > >      uint16_t smbios_core_count2;
> > >      uint8_t smbios_thread_count;
> > > +    uint16_t smbios_thread_count2;
> > >      uint8_t *required_struct_types;
> > >      int required_struct_types_len;
> > >      int type4_count;
> > > @@ -644,6 +645,7 @@ static void smbios_cpu_test(test_data *data, uint32_t addr,
> > >      uint8_t thread_count, expected_thread_count = data->smbios_thread_count;
> > >      uint16_t speed, expected_speed[2];
> > >      uint16_t core_count2, expected_core_count2 = data->smbios_core_count2;
> > > +    uint16_t thread_count2, expected_thread_count2 = data->smbios_thread_count2;
> > >      int offset[2];
> > >      int i;
> > >  
> > > @@ -673,6 +675,8 @@ static void smbios_cpu_test(test_data *data, uint32_t addr,
> > >      }
> > >  
> > >      if (ep_type == SMBIOS_ENTRY_POINT_TYPE_64) {
> > > +        uint64_t thread_count2_addr;
> > > +
> > >          core_count2 = qtest_readw(data->qts,
> > >                            addr + offsetof(struct smbios_type_4, core_count2));
> > >  
> > > @@ -680,6 +684,15 @@ static void smbios_cpu_test(test_data *data, uint32_t addr,
> > >          if (expected_core_count == 0xFF && expected_core_count2) {
> > >              g_assert_cmpuint(core_count2, ==, expected_core_count2);
> > >          }
> > > +
> > > +        thread_count2_addr = addr +
> > > +                             offsetof(struct smbios_type_4, thread_count2);
> > > +        thread_count2 = qtest_readw(data->qts, thread_count2_addr);  
> > 
> > I'd mimic the same code style as used for core_count2 and avoid introducing an extra variable  
> 
> I'm not sure about the style of this case, since the code line is still
> too long, so which style should I pick? ;-)
> 
> thread_count2 = qtest_readw(data->qts,
>                     addr + offsetof(struct smbios_type_4,
> 		                    thread_count2));
> 
> or,


> thread_count2 = qtest_readw(data->qts,
>                     addr + offsetof(struct smbios_type_4,
>                     thread_count2));

this one

> 
> 
> >   
> > > +
> > > +        /* Thread Count has reached its limit, checking Thread Count 2 */
> > > +        if (expected_thread_count == 0xFF && expected_thread_count2) {
> > > +            g_assert_cmpuint(thread_count2, ==, expected_thread_count2);
> > > +        }
> > >      }
> > >  }
> > >  
> > > @@ -1050,6 +1063,7 @@ static void test_acpi_q35_tcg_thread_count(void)
> > >          .required_struct_types = base_required_struct_types,
> > >          .required_struct_types_len = ARRAY_SIZE(base_required_struct_types),
> > >          .smbios_thread_count = 27,
> > > +        .smbios_thread_count2 = 27,
> > >      };
> > >  
> > >      test_acpi_one("-machine smbios-entry-point-type=64 "
> > > @@ -1058,6 +1072,23 @@ static void test_acpi_q35_tcg_thread_count(void)
> > >      free_test_data(&data);
> > >  }
> > >  
> > > +static void test_acpi_q35_tcg_thread_count2(void)
> > > +{
> > > +    test_data data = {
> > > +        .machine = MACHINE_Q35,
> > > +        .variant = ".thread-count2",
> > > +        .required_struct_types = base_required_struct_types,
> > > +        .required_struct_types_len = ARRAY_SIZE(base_required_struct_types),
> > > +        .smbios_thread_count = 0xFF,
> > > +        .smbios_thread_count2 = 260,
> > > +    };
> > > +
> > > +    test_acpi_one("-machine smbios-entry-point-type=64 "
> > > +                  "-smp cpus=210,maxcpus=520,sockets=2,dies=2,cores=65,threads=2",
> > > +                  &data);  
> > 
> > explain in commit message why abive -smp ==   
> 
> Ok, this is used to test if we could correctly distinguish smp.cpus and smp.maxcpus.
> 
> Thanks,
> Zhao
> 
> >   > +        .smbios_thread_count = 0xFF,
> >   > +        .smbios_thread_count2 = 260,  
> > 
> >   
> > > +    free_test_data(&data);
> > > +}
> > > +
> > >  static void test_acpi_q35_tcg_bridge(void)
> > >  {
> > >      test_data data = {};
> > > @@ -2216,6 +2247,8 @@ int main(int argc, char *argv[])
> > >                                 test_acpi_q35_tcg_core_count2);
> > >                  qtest_add_func("acpi/q35/thread-count",
> > >                                 test_acpi_q35_tcg_thread_count);
> > > +                qtest_add_func("acpi/q35/thread-count2",
> > > +                               test_acpi_q35_tcg_thread_count2);
> > >              }
> > >              qtest_add_func("acpi/q35/viot", test_acpi_q35_viot);
> > >  #ifdef CONFIG_POSIX  
> >   
>