diff mbox series

[03/19] tests: smbios: add test for legacy mode CLI options

Message ID 20240227154749.1818189-4-imammedo@redhat.com
State New
Headers show
Series Workaround Windows failing to find 64bit SMBIOS entry point with SeaBIOS | expand

Commit Message

Igor Mammedov Feb. 27, 2024, 3:47 p.m. UTC 
Unfortunately having 2.0 machine type deprecated is not enough
to get rid of legacy SMBIOS handling since 'isapc' also uses
that and it's staying around.

Hence add test for CLI options handling to be sure that it
ain't broken during SMBIOS code refactoring.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/data/smbios/type11_blob.legacy | Bin 0 -> 10 bytes
 tests/qtest/bios-tables-test.c       |  17 +++++++++++++++++
 2 files changed, 17 insertions(+)
 create mode 100644 tests/data/smbios/type11_blob.legacy

Comments

Ani Sinha Feb. 28, 2024, 11:11 a.m. UTC | #1 
> On 27-Feb-2024, at 21:17, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> Unfortunately having 2.0 machine type deprecated is not enough
> to get rid of legacy SMBIOS handling since 'isapc' also uses
> that and it's staying around.
> 
> Hence add test for CLI options handling to be sure that it
> ain't broken during SMBIOS code refactoring.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> tests/data/smbios/type11_blob.legacy | Bin 0 -> 10 bytes
> tests/qtest/bios-tables-test.c       |  17 +++++++++++++++++
> 2 files changed, 17 insertions(+)
> create mode 100644 tests/data/smbios/type11_blob.legacy
> 
> diff --git a/tests/data/smbios/type11_blob.legacy b/tests/data/smbios/type11_blob.legacy
> new file mode 100644
> index 0000000000000000000000000000000000000000..aef463aab903405958b0a85f85c5980671c08bee
> GIT binary patch
> literal 10
> Rcmd;PW!S(N;u;*n000Tp0s;U4
> 
> literal 0
> HcmV?d00001
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index a116f88e1d..d1ff4db7a2 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -2106,6 +2106,21 @@ static void test_acpi_pc_smbios_blob(void)
>     free_test_data(&data);
> }
> 
> +static void test_acpi_isapc_smbios_legacy(void)
> +{
> +    uint8_t req_type11[] = { 1, 11 };

Ok so looking at the code, it won’t let you specify smbios type other than 1 …

See the following in smbios_set_defaults()

  if (smbios_legacy) {
        g_free(smbios_tables);
	/* in legacy mode, also complain if fields were given for types >1 */
        if (find_next_bit(have_fields_bitmap,
	                  SMBIOS_MAX_TYPE+1, 2) < SMBIOS_MAX_TYPE+1) {
            error_report("can't process fields for smbios "
                         "types > 1 on machine versions < 2.1!");
            exit(1);
        }
    } else {

BUT you lets you load a blob of type 11? Is that a bug in QEMU? Should we also add a similar check for have_binfile_bitmap? 

> +    test_data data = {
> +        .machine = "isapc",
> +        .variant = ".pc_smbios_legacy",
> +        .required_struct_types = req_type11,
> +        .required_struct_types_len = ARRAY_SIZE(req_type11),
> +    };
> +
> +    test_smbios("-smbios file=tests/data/smbios/type11_blob.legacy "
> +                "-smbios type=1,family=TEST", &data);
> +    free_test_data(&data);
> +}
> +
> static void test_oem_fields(test_data *data)
> {
>     int i;
> @@ -2261,6 +2276,8 @@ int main(int argc, char *argv[])
>                            test_acpi_pc_smbios_options);
>             qtest_add_func("acpi/piix4/smbios-blob",
>                            test_acpi_pc_smbios_blob);
> +            qtest_add_func("acpi/piix4/smbios-legacy",
> +                           test_acpi_isapc_smbios_legacy);
>         }
>         if (qtest_has_machine(MACHINE_Q35)) {
>             qtest_add_func("acpi/q35", test_acpi_q35_tcg);
> -- 
> 2.39.3
>
Igor Mammedov Feb. 28, 2024, 1:59 p.m. UTC | #2 
On Wed, 28 Feb 2024 16:41:22 +0530
Ani Sinha <anisinha@redhat.com> wrote:

> > On 27-Feb-2024, at 21:17, Igor Mammedov <imammedo@redhat.com> wrote:
> > 
> > Unfortunately having 2.0 machine type deprecated is not enough
> > to get rid of legacy SMBIOS handling since 'isapc' also uses
> > that and it's staying around.
> > 
> > Hence add test for CLI options handling to be sure that it
> > ain't broken during SMBIOS code refactoring.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > tests/data/smbios/type11_blob.legacy | Bin 0 -> 10 bytes
> > tests/qtest/bios-tables-test.c       |  17 +++++++++++++++++
> > 2 files changed, 17 insertions(+)
> > create mode 100644 tests/data/smbios/type11_blob.legacy
> > 
> > diff --git a/tests/data/smbios/type11_blob.legacy b/tests/data/smbios/type11_blob.legacy
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..aef463aab903405958b0a85f85c5980671c08bee
> > GIT binary patch
> > literal 10
> > Rcmd;PW!S(N;u;*n000Tp0s;U4
> > 
> > literal 0
> > HcmV?d00001
> > 
> > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> > index a116f88e1d..d1ff4db7a2 100644
> > --- a/tests/qtest/bios-tables-test.c
> > +++ b/tests/qtest/bios-tables-test.c
> > @@ -2106,6 +2106,21 @@ static void test_acpi_pc_smbios_blob(void)
> >     free_test_data(&data);
> > }
> > 
> > +static void test_acpi_isapc_smbios_legacy(void)
> > +{
> > +    uint8_t req_type11[] = { 1, 11 };  
> 
> Ok so looking at the code, it won’t let you specify smbios type other than 1 …
> 
> See the following in smbios_set_defaults()
> 
>   if (smbios_legacy) {
>         g_free(smbios_tables);
> 	/* in legacy mode, also complain if fields were given for types >1 */
>         if (find_next_bit(have_fields_bitmap,
> 	                  SMBIOS_MAX_TYPE+1, 2) < SMBIOS_MAX_TYPE+1) {
>             error_report("can't process fields for smbios "
>                          "types > 1 on machine versions < 2.1!");
>             exit(1);
>         }
>     } else {
> 
> BUT you lets you load a blob of type 11? Is that a bug in QEMU? Should we also add a similar check for have_binfile_bitmap? 

I'd say bug (oversight) in QEMU, actually some of patches later in series
mention this issue in commit messages.
And I'm adding check only for type4 which I have to touch
as for other types it might work (type11) or not.
But I have not tested every possible type (only type 11 and 4)
the former lets test bloated SMBIOS blob and the later
needs refactoring to make this series work..

Everything else was ignored for this series and
can be done later on top.
As mentioned later in series I'd rather deprecate and
drop all legacy code instead of fixing it.

These tests are mostly for me to be sure that existing
CLI handling and blob building aren't broken during
refactoring.

> > +    test_data data = {
> > +        .machine = "isapc",
> > +        .variant = ".pc_smbios_legacy",
> > +        .required_struct_types = req_type11,
> > +        .required_struct_types_len = ARRAY_SIZE(req_type11),
> > +    };
> > +
> > +    test_smbios("-smbios file=tests/data/smbios/type11_blob.legacy "
> > +                "-smbios type=1,family=TEST", &data);
> > +    free_test_data(&data);
> > +}
> > +
> > static void test_oem_fields(test_data *data)
> > {
> >     int i;
> > @@ -2261,6 +2276,8 @@ int main(int argc, char *argv[])
> >                            test_acpi_pc_smbios_options);
> >             qtest_add_func("acpi/piix4/smbios-blob",
> >                            test_acpi_pc_smbios_blob);
> > +            qtest_add_func("acpi/piix4/smbios-legacy",
> > +                           test_acpi_isapc_smbios_legacy);
> >         }
> >         if (qtest_has_machine(MACHINE_Q35)) {
> >             qtest_add_func("acpi/q35", test_acpi_q35_tcg);
> > -- 
> > 2.39.3
> >   
>
Ani Sinha Feb. 28, 2024, 2:18 p.m. UTC | #3 
> On 27-Feb-2024, at 21:17, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> Unfortunately having 2.0 machine type deprecated is not enough
> to get rid of legacy SMBIOS handling since 'isapc' also uses
> that and it's staying around.
> 
> Hence add test for CLI options handling to be sure that it
> ain't broken during SMBIOS code refactoring.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Ani Sinha <anisinha@redhat.com>

> ---
> tests/data/smbios/type11_blob.legacy | Bin 0 -> 10 bytes
> tests/qtest/bios-tables-test.c       |  17 +++++++++++++++++
> 2 files changed, 17 insertions(+)
> create mode 100644 tests/data/smbios/type11_blob.legacy
> 
> diff --git a/tests/data/smbios/type11_blob.legacy b/tests/data/smbios/type11_blob.legacy
> new file mode 100644
> index 0000000000000000000000000000000000000000..aef463aab903405958b0a85f85c5980671c08bee
> GIT binary patch
> literal 10
> Rcmd;PW!S(N;u;*n000Tp0s;U4
> 
> literal 0
> HcmV?d00001
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index a116f88e1d..d1ff4db7a2 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -2106,6 +2106,21 @@ static void test_acpi_pc_smbios_blob(void)
>     free_test_data(&data);
> }
> 
> +static void test_acpi_isapc_smbios_legacy(void)
> +{
> +    uint8_t req_type11[] = { 1, 11 };
> +    test_data data = {
> +        .machine = "isapc",
> +        .variant = ".pc_smbios_legacy",
> +        .required_struct_types = req_type11,
> +        .required_struct_types_len = ARRAY_SIZE(req_type11),
> +    };
> +
> +    test_smbios("-smbios file=tests/data/smbios/type11_blob.legacy "
> +                "-smbios type=1,family=TEST", &data);
> +    free_test_data(&data);
> +}
> +
> static void test_oem_fields(test_data *data)
> {
>     int i;
> @@ -2261,6 +2276,8 @@ int main(int argc, char *argv[])
>                            test_acpi_pc_smbios_options);
>             qtest_add_func("acpi/piix4/smbios-blob",
>                            test_acpi_pc_smbios_blob);
> +            qtest_add_func("acpi/piix4/smbios-legacy",
> +                           test_acpi_isapc_smbios_legacy);
>         }
>         if (qtest_has_machine(MACHINE_Q35)) {
>             qtest_add_func("acpi/q35", test_acpi_q35_tcg);
> -- 
> 2.39.3
>
diff mbox series

Patch

diff --git a/tests/data/smbios/type11_blob.legacy b/tests/data/smbios/type11_blob.legacy
new file mode 100644
index 0000000000000000000000000000000000000000..aef463aab903405958b0a85f85c5980671c08bee
GIT binary patch
literal 10
Rcmd;PW!S(N;u;*n000Tp0s;U4

literal 0
HcmV?d00001

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index a116f88e1d..d1ff4db7a2 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -2106,6 +2106,21 @@  static void test_acpi_pc_smbios_blob(void)
     free_test_data(&data);
 }
 
+static void test_acpi_isapc_smbios_legacy(void)
+{
+    uint8_t req_type11[] = { 1, 11 };
+    test_data data = {
+        .machine = "isapc",
+        .variant = ".pc_smbios_legacy",
+        .required_struct_types = req_type11,
+        .required_struct_types_len = ARRAY_SIZE(req_type11),
+    };
+
+    test_smbios("-smbios file=tests/data/smbios/type11_blob.legacy "
+                "-smbios type=1,family=TEST", &data);
+    free_test_data(&data);
+}
+
 static void test_oem_fields(test_data *data)
 {
     int i;
@@ -2261,6 +2276,8 @@  int main(int argc, char *argv[])
                            test_acpi_pc_smbios_options);
             qtest_add_func("acpi/piix4/smbios-blob",
                            test_acpi_pc_smbios_blob);
+            qtest_add_func("acpi/piix4/smbios-legacy",
+                           test_acpi_isapc_smbios_legacy);
         }
         if (qtest_has_machine(MACHINE_Q35)) {
             qtest_add_func("acpi/q35", test_acpi_q35_tcg);