[PATCH] target/arm: build smbios 19 table

[PATCH] target/arm: build smbios 19 table

[Mihai Carabas]

Use the base_memmap to build the SMBIOS 19 table which provides the address
mapping for a Physical Memory Array (from spec [1] chapter 7.20).

This was present on i386 from commit c97294ec1b9e36887e119589d456557d72ab37b5
("SMBIOS: Build aggregate smbios tables and entry point").

[1] https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.5.0.pdf

Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
---
 hw/arm/virt.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index cda9defe8f09..855b6982f363 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1607,9 +1607,11 @@ static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
 static void virt_build_smbios(VirtMachineState *vms)
 {
     MachineClass *mc = MACHINE_GET_CLASS(vms);
+    MachineState *ms = MACHINE(vms);
     VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
     uint8_t *smbios_tables, *smbios_anchor;
     size_t smbios_tables_len, smbios_anchor_len;
+    struct smbios_phys_mem_area mem_array;
     const char *product = "QEMU Virtual Machine";
 
     if (kvm_enabled()) {
@@ -1620,7 +1622,11 @@ static void virt_build_smbios(VirtMachineState *vms)
                         vmc->smbios_old_sys_ver ? "1.0" : mc->name, false,
                         true, SMBIOS_ENTRY_POINT_TYPE_64);
 
-    smbios_get_tables(MACHINE(vms), NULL, 0,
+    /* build the array of physical mem area from base_memmap */
+    mem_array.address = vms->memmap[VIRT_MEM].base;
+    mem_array.length = ms->ram_size;
+
+    smbios_get_tables(ms, &mem_array, 1,
                       &smbios_tables, &smbios_tables_len,
                       &smbios_anchor, &smbios_anchor_len,
                       &error_fatal);
-- 
1.8.3.1

Re: [PATCH] target/arm: build smbios 19 table

[Peter Maydell]

On Fri, 18 Nov 2022 at 17:37, Mihai Carabas <mihai.carabas@oracle.com> wrote:
>
> Use the base_memmap to build the SMBIOS 19 table which provides the address
> mapping for a Physical Memory Array (from spec [1] chapter 7.20).
>
> This was present on i386 from commit c97294ec1b9e36887e119589d456557d72ab37b5
> ("SMBIOS: Build aggregate smbios tables and entry point").
>
> [1] https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.5.0.pdf
>
> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>

Is this a bug fix, or a new feature? What are the consequences
of it being missing? Is this important enough to go into the 7.2
release? (My default position would be "no", given this has been
like this on the virt board for a very long time.)

> ---
>  hw/arm/virt.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index cda9defe8f09..855b6982f363 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1607,9 +1607,11 @@ static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
>  static void virt_build_smbios(VirtMachineState *vms)
>  {
>      MachineClass *mc = MACHINE_GET_CLASS(vms);
> +    MachineState *ms = MACHINE(vms);
>      VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>      uint8_t *smbios_tables, *smbios_anchor;
>      size_t smbios_tables_len, smbios_anchor_len;
> +    struct smbios_phys_mem_area mem_array;
>      const char *product = "QEMU Virtual Machine";
>
>      if (kvm_enabled()) {
> @@ -1620,7 +1622,11 @@ static void virt_build_smbios(VirtMachineState *vms)
>                          vmc->smbios_old_sys_ver ? "1.0" : mc->name, false,
>                          true, SMBIOS_ENTRY_POINT_TYPE_64);
>
> -    smbios_get_tables(MACHINE(vms), NULL, 0,
> +    /* build the array of physical mem area from base_memmap */
> +    mem_array.address = vms->memmap[VIRT_MEM].base;
> +    mem_array.length = ms->ram_size;
> +
> +    smbios_get_tables(ms, &mem_array, 1,
>                        &smbios_tables, &smbios_tables_len,
>                        &smbios_anchor, &smbios_anchor_len,
>                        &error_fatal);

Does this show up as a difference in the ACPI tables? If so then
the bios-tables-tests will need updating (and this would probably
show up as "make check" failing).

Do we need to care here about pluggable memory devices?
(We seem to do something with them in the ACPI tables
via build_srat_memory(), so maybe not?)

thanks
-- PMM

Re: [PATCH] target/arm: build smbios 19 table

[Mihai Carabas]

La 18.11.2022 21:11, Peter Maydell a scris:
> On Fri, 18 Nov 2022 at 17:37, Mihai Carabas <mihai.carabas@oracle.com> wrote:
>> Use the base_memmap to build the SMBIOS 19 table which provides the address
>> mapping for a Physical Memory Array (from spec [1] chapter 7.20).
>>
>> This was present on i386 from commit c97294ec1b9e36887e119589d456557d72ab37b5
>> ("SMBIOS: Build aggregate smbios tables and entry point").
>>
>> [1] https://urldefense.com/v3/__https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.5.0.pdf__;!!ACWV5N9M2RV99hQ!KF2xmQw9nxPvqvNCgDleyVHv4MoZseoZFHmR1veww7O2BmRxSH1spOCNWX-c-FvzcaR_o8PunXSWWH2ECvFqlR4E7vw$
>>
>> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
> Is this a bug fix, or a new feature? What are the consequences
> of it being missing? Is this important enough to go into the 7.2
> release? (My default position would be "no", given this has been
> like this on the virt board for a very long time.)


This is required by ARM SystemReady Virtual Environment [1]. As 
described in the Arm SystemReady Requirements Specification v2.0
  [2] page 9, 2.5.1 SystemReady Virtual Environment (VE) v1.0 
requirements,: "FirmwareTestSuite (FWTS) must still be used" -> fwts 
checks for the presence of SMBIOS type 19 table and fails the test in 
this case.


[1] 
https://www.arm.com/architecture/system-architectures/systemready-certification-program/ve

[2] https://developer.arm.com/documentation/den0109/latest

>
>> ---
>>   hw/arm/virt.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index cda9defe8f09..855b6982f363 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1607,9 +1607,11 @@ static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
>>   static void virt_build_smbios(VirtMachineState *vms)
>>   {
>>       MachineClass *mc = MACHINE_GET_CLASS(vms);
>> +    MachineState *ms = MACHINE(vms);
>>       VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>>       uint8_t *smbios_tables, *smbios_anchor;
>>       size_t smbios_tables_len, smbios_anchor_len;
>> +    struct smbios_phys_mem_area mem_array;
>>       const char *product = "QEMU Virtual Machine";
>>
>>       if (kvm_enabled()) {
>> @@ -1620,7 +1622,11 @@ static void virt_build_smbios(VirtMachineState *vms)
>>                           vmc->smbios_old_sys_ver ? "1.0" : mc->name, false,
>>                           true, SMBIOS_ENTRY_POINT_TYPE_64);
>>
>> -    smbios_get_tables(MACHINE(vms), NULL, 0,
>> +    /* build the array of physical mem area from base_memmap */
>> +    mem_array.address = vms->memmap[VIRT_MEM].base;
>> +    mem_array.length = ms->ram_size;
>> +
>> +    smbios_get_tables(ms, &mem_array, 1,
>>                         &smbios_tables, &smbios_tables_len,
>>                         &smbios_anchor, &smbios_anchor_len,
>>                         &error_fatal);
> Does this show up as a difference in the ACPI tables? If so then
> the bios-tables-tests will need updating (and this would probably
> show up as "make check" failing).
In ./tests/qtest/bios-tables-test.c we have test_smbios_structs which is 
checking for all structs present. Also it is looking for mandatory types 
and 19 is one of them (base_required_struct_types -> 19). I think we 
cover everything. I will re-run make check to see.
>
> Do we need to care here about pluggable memory devices?
> (We seem to do something with them in the ACPI tables
> via build_srat_memory(), so maybe not?)
Here you are refering to the fact that when we hot plug a memory dim, to 
automatically update smbios type 19 entry/entries?
>
> thanks
> -- PMM

Re: [PATCH] target/arm: build smbios 19 table

[Peter Maydell]

On Sun, 20 Nov 2022 at 17:53, Mihai Carabas <mihai.carabas@oracle.com> wrote:
>
> La 18.11.2022 21:11, Peter Maydell a scris:
> > On Fri, 18 Nov 2022 at 17:37, Mihai Carabas <mihai.carabas@oracle.com> wrote:
> >> Use the base_memmap to build the SMBIOS 19 table which provides the address
> >> mapping for a Physical Memory Array (from spec [1] chapter 7.20).
> >>
> >> This was present on i386 from commit c97294ec1b9e36887e119589d456557d72ab37b5
> >> ("SMBIOS: Build aggregate smbios tables and entry point").
> >>
> >> [1] https://urldefense.com/v3/__https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.5.0.pdf__;!!ACWV5N9M2RV99hQ!KF2xmQw9nxPvqvNCgDleyVHv4MoZseoZFHmR1veww7O2BmRxSH1spOCNWX-c-FvzcaR_o8PunXSWWH2ECvFqlR4E7vw$
> >>
> >> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
> > Is this a bug fix, or a new feature? What are the consequences
> > of it being missing? Is this important enough to go into the 7.2
> > release? (My default position would be "no", given this has been
> > like this on the virt board for a very long time.)
>
>
> This is required by ARM SystemReady Virtual Environment [1]. As
> described in the Arm SystemReady Requirements Specification v2.0
>   [2] page 9, 2.5.1 SystemReady Virtual Environment (VE) v1.0
> requirements,: "FirmwareTestSuite (FWTS) must still be used" -> fwts
> checks for the presence of SMBIOS type 19 table and fails the test in
> this case.

OK, so it's a spec requirement. Are there any actual realworld
guests that don't work because we get this wrong ?

> > Do we need to care here about pluggable memory devices?
> > (We seem to do something with them in the ACPI tables
> > via build_srat_memory(), so maybe not?)

> Here you are refering to the fact that when we hot plug a memory dim, to
> automatically update smbios type 19 entry/entries?

I don't know anything at all really about any of these ACPI/SMBIOS/etc
data structures. I do know that the virt board has two ways to
have RAM in it:
 * the 'standard RAM" that you get with -m 1024M etc
 * the pluggable DIMMs

So I'm just asking if this bit of code needs to account for
the second kind, or if this SMBIOS table type only cares about
the first kind. If you don't know the answer then we can
check with the person who added the pluggable DIMM support.

thanks
-- PMM

Re: [PATCH] target/arm: build smbios 19 table

[Mihai Carabas]

La 21.11.2022 13:02, Peter Maydell a scris:
> On Sun, 20 Nov 2022 at 17:53, Mihai Carabas <mihai.carabas@oracle.com> wrote:
>> La 18.11.2022 21:11, Peter Maydell a scris:
>>> On Fri, 18 Nov 2022 at 17:37, Mihai Carabas <mihai.carabas@oracle.com> wrote:
>>>> Use the base_memmap to build the SMBIOS 19 table which provides the address
>>>> mapping for a Physical Memory Array (from spec [1] chapter 7.20).
>>>>
>>>> This was present on i386 from commit c97294ec1b9e36887e119589d456557d72ab37b5
>>>> ("SMBIOS: Build aggregate smbios tables and entry point").
>>>>
>>>> [1] https://urldefense.com/v3/__https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.5.0.pdf__;!!ACWV5N9M2RV99hQ!KF2xmQw9nxPvqvNCgDleyVHv4MoZseoZFHmR1veww7O2BmRxSH1spOCNWX-c-FvzcaR_o8PunXSWWH2ECvFqlR4E7vw$
>>>>
>>>> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
>>> Is this a bug fix, or a new feature? What are the consequences
>>> of it being missing? Is this important enough to go into the 7.2
>>> release? (My default position would be "no", given this has been
>>> like this on the virt board for a very long time.)
>>
>> This is required by ARM SystemReady Virtual Environment [1]. As
>> described in the Arm SystemReady Requirements Specification v2.0
>>    [2] page 9, 2.5.1 SystemReady Virtual Environment (VE) v1.0
>> requirements,: "FirmwareTestSuite (FWTS) must still be used" -> fwts
>> checks for the presence of SMBIOS type 19 table and fails the test in
>> this case.
> OK, so it's a spec requirement. Are there any actual realworld
> guests that don't work because we get this wrong ?

We do not have a clear example. The thing we hit was the ARM SystemReady 
certification based on fwts.

Re: [PATCH] target/arm: build smbios 19 table

[Peter Maydell]

On Mon, 21 Nov 2022 at 11:24, Mihai Carabas <mihai.carabas@oracle.com> wrote:
>
> La 21.11.2022 13:02, Peter Maydell a scris:
> > On Sun, 20 Nov 2022 at 17:53, Mihai Carabas <mihai.carabas@oracle.com> wrote:
> >> La 18.11.2022 21:11, Peter Maydell a scris:
> >>> On Fri, 18 Nov 2022 at 17:37, Mihai Carabas <mihai.carabas@oracle.com> wrote:
> >>>> Use the base_memmap to build the SMBIOS 19 table which provides the address
> >>>> mapping for a Physical Memory Array (from spec [1] chapter 7.20).
> >>>>
> >>>> This was present on i386 from commit c97294ec1b9e36887e119589d456557d72ab37b5
> >>>> ("SMBIOS: Build aggregate smbios tables and entry point").
> >>>>
> >>>> [1] https://urldefense.com/v3/__https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.5.0.pdf__;!!ACWV5N9M2RV99hQ!KF2xmQw9nxPvqvNCgDleyVHv4MoZseoZFHmR1veww7O2BmRxSH1spOCNWX-c-FvzcaR_o8PunXSWWH2ECvFqlR4E7vw$
> >>>>
> >>>> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
> >>> Is this a bug fix, or a new feature? What are the consequences
> >>> of it being missing? Is this important enough to go into the 7.2
> >>> release? (My default position would be "no", given this has been
> >>> like this on the virt board for a very long time.)
> >>
> >> This is required by ARM SystemReady Virtual Environment [1]. As
> >> described in the Arm SystemReady Requirements Specification v2.0
> >>    [2] page 9, 2.5.1 SystemReady Virtual Environment (VE) v1.0
> >> requirements,: "FirmwareTestSuite (FWTS) must still be used" -> fwts
> >> checks for the presence of SMBIOS type 19 table and fails the test in
> >> this case.
> > OK, so it's a spec requirement. Are there any actual realworld
> > guests that don't work because we get this wrong ?
>
> We do not have a clear example. The thing we hit was the ARM SystemReady
> certification based on fwts.

Thanks for clarifying; in that case given we're quite far along
in the 7.2 release cycle I think we shouldn't try to get this
patch in to that release but instead put it in for 8.0.

thanks
-- PMM

Re: [PATCH] target/arm: build smbios 19 table

[Peter Maydell]

On Mon, 21 Nov 2022 at 11:43, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 21 Nov 2022 at 11:24, Mihai Carabas <mihai.carabas@oracle.com> wrote:
> >
> > La 21.11.2022 13:02, Peter Maydell a scris:
> > > On Sun, 20 Nov 2022 at 17:53, Mihai Carabas <mihai.carabas@oracle.com> wrote:
> > >> La 18.11.2022 21:11, Peter Maydell a scris:
> > >>> On Fri, 18 Nov 2022 at 17:37, Mihai Carabas <mihai.carabas@oracle.com> wrote:
> > >>>> Use the base_memmap to build the SMBIOS 19 table which provides the address
> > >>>> mapping for a Physical Memory Array (from spec [1] chapter 7.20).
> > >>>>
> > >>>> This was present on i386 from commit c97294ec1b9e36887e119589d456557d72ab37b5
> > >>>> ("SMBIOS: Build aggregate smbios tables and entry point").
> > >>>>
> > >>>> [1] https://urldefense.com/v3/__https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.5.0.pdf__;!!ACWV5N9M2RV99hQ!KF2xmQw9nxPvqvNCgDleyVHv4MoZseoZFHmR1veww7O2BmRxSH1spOCNWX-c-FvzcaR_o8PunXSWWH2ECvFqlR4E7vw$
> > >>>>
> > >>>> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
> > >>> Is this a bug fix, or a new feature? What are the consequences
> > >>> of it being missing? Is this important enough to go into the 7.2
> > >>> release? (My default position would be "no", given this has been
> > >>> like this on the virt board for a very long time.)
> > >>
> > >> This is required by ARM SystemReady Virtual Environment [1]. As
> > >> described in the Arm SystemReady Requirements Specification v2.0
> > >>    [2] page 9, 2.5.1 SystemReady Virtual Environment (VE) v1.0
> > >> requirements,: "FirmwareTestSuite (FWTS) must still be used" -> fwts
> > >> checks for the presence of SMBIOS type 19 table and fails the test in
> > >> this case.
> > > OK, so it's a spec requirement. Are there any actual realworld
> > > guests that don't work because we get this wrong ?
> >
> > We do not have a clear example. The thing we hit was the ARM SystemReady
> > certification based on fwts.
>
> Thanks for clarifying; in that case given we're quite far along
> in the 7.2 release cycle I think we shouldn't try to get this
> patch in to that release but instead put it in for 8.0.

I checked how x86 sets up this smbios table, and it doesn't
put any pluggable DIMMs in it, only the initial fixed RAM,
so I'm happy that virt doesn't need to do anything special
there. I've applied this patch to a target-arm-for-8.0 branch
that will eventually turn into target-arm when 7.2 is released
in a few weeks time.

I've tweaked the commit message to make it clear that this
is fixing a spec issue which doesn't to our knowledge
cause any guest OS problems.

thanks
-- PMM

Re: [PATCH] target/arm: build smbios 19 table

[Mihai Carabas]

La 22.11.2022 15:40, Peter Maydell a scris:
> On Mon, 21 Nov 2022 at 11:43, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On Mon, 21 Nov 2022 at 11:24, Mihai Carabas <mihai.carabas@oracle.com> wrote:
>>> La 21.11.2022 13:02, Peter Maydell a scris:
>>>> On Sun, 20 Nov 2022 at 17:53, Mihai Carabas <mihai.carabas@oracle.com> wrote:
>>>>> La 18.11.2022 21:11, Peter Maydell a scris:
>>>>>> On Fri, 18 Nov 2022 at 17:37, Mihai Carabas <mihai.carabas@oracle.com> wrote:
>>>>>>> Use the base_memmap to build the SMBIOS 19 table which provides the address
>>>>>>> mapping for a Physical Memory Array (from spec [1] chapter 7.20).
>>>>>>>
>>>>>>> This was present on i386 from commit c97294ec1b9e36887e119589d456557d72ab37b5
>>>>>>> ("SMBIOS: Build aggregate smbios tables and entry point").
>>>>>>>
>>>>>>> [1] https://urldefense.com/v3/__https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.5.0.pdf__;!!ACWV5N9M2RV99hQ!KF2xmQw9nxPvqvNCgDleyVHv4MoZseoZFHmR1veww7O2BmRxSH1spOCNWX-c-FvzcaR_o8PunXSWWH2ECvFqlR4E7vw$
>>>>>>>
>>>>>>> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
>>>>>> Is this a bug fix, or a new feature? What are the consequences
>>>>>> of it being missing? Is this important enough to go into the 7.2
>>>>>> release? (My default position would be "no", given this has been
>>>>>> like this on the virt board for a very long time.)
>>>>> This is required by ARM SystemReady Virtual Environment [1]. As
>>>>> described in the Arm SystemReady Requirements Specification v2.0
>>>>>     [2] page 9, 2.5.1 SystemReady Virtual Environment (VE) v1.0
>>>>> requirements,: "FirmwareTestSuite (FWTS) must still be used" -> fwts
>>>>> checks for the presence of SMBIOS type 19 table and fails the test in
>>>>> this case.
>>>> OK, so it's a spec requirement. Are there any actual realworld
>>>> guests that don't work because we get this wrong ?
>>> We do not have a clear example. The thing we hit was the ARM SystemReady
>>> certification based on fwts.
>> Thanks for clarifying; in that case given we're quite far along
>> in the 7.2 release cycle I think we shouldn't try to get this
>> patch in to that release but instead put it in for 8.0.
> I checked how x86 sets up this smbios table, and it doesn't
> put any pluggable DIMMs in it, only the initial fixed RAM,
> so I'm happy that virt doesn't need to do anything special
> there. I've applied this patch to a target-arm-for-8.0 branch
> that will eventually turn into target-arm when 7.2 is released
> in a few weeks time.
>
> I've tweaked the commit message to make it clear that this
> is fixing a spec issue which doesn't to our knowledge
> cause any guest OS problems.
Thank you! Also talked with my collegues and confirmed that SMBIOS is a 
static area that is not updated during OS running.
>
> thanks
> -- PMM