| Message ID | 1668789029-5432-1-git-send-email-mihai.carabas@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: build smbios 19 table | expand |
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
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
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
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.
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
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
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
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);
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(-)