Hi Ron, Ard,
On Sun, Jun 25, 2023 at 11:57 PM ron minnich <[email protected]> wrote:
>
> Hey Ard, thanks for the discussion, sounds like we are able to move forward now!
>
> On Sun, Jun 25, 2023, 6:13 AM Ard Biesheuvel <[email protected]> wrote:
>>
>> If this is only used on RISC-V, and implemented under arch/riscv, I
>> have no objections.
Thank you for your suggestions that made us reach an agreement, let's
continue to review this patch.
The current logic is to implement the common interface under
drivers/firmware/, if we need this function, we can call
fdt_fwtbl_init() to complete it in arch/xxx/kernel/setup.c.
For enabling on RISC-V, we can complete it in a subsequent patch to
setup_arch-->fdt_fwtbl_init() in arch/riscv/kernel/setup.c.
What do you think?
Thanks,
Yunhui
On Mon, 26 Jun 2023 at 04:35, 运辉崔 <[email protected]> wrote:
>
> Hi Ron, Ard,
>
> On Sun, Jun 25, 2023 at 11:57 PM ron minnich <[email protected]> wrote:
> >
> > Hey Ard, thanks for the discussion, sounds like we are able to move forward now!
>
> >
> > On Sun, Jun 25, 2023, 6:13 AM Ard Biesheuvel <[email protected]> wrote:
> >>
> >> If this is only used on RISC-V, and implemented under arch/riscv, I
> >> have no objections.
>
> Thank you for your suggestions that made us reach an agreement, let's
> continue to review this patch.
>
> The current logic is to implement the common interface under
> drivers/firmware/, if we need this function, we can call
> fdt_fwtbl_init() to complete it in arch/xxx/kernel/setup.c.
>
> For enabling on RISC-V, we can complete it in a subsequent patch to
> setup_arch-->fdt_fwtbl_init() in arch/riscv/kernel/setup.c.
>
> What do you think?
>
I think all of this belongs under arch/riscv
Hi Ard,
On Mon, Jun 26, 2023 at 2:43 PM Ard Biesheuvel <[email protected]> wrote:
> I think all of this belongs under arch/riscv
Could you look at the content of the patch again? As we discussed
before, we need to connect to the ACPI and the SMBIOS entry
At least this part of the code has to be placed in the corresponding place:
drivers/acpi/osl.c: acpi_os_get_root_pointer()
drivers/firmware/dmi_scan.c:dmi_scan_machine()
Because obtaining firmware information through DTS belongs to the
content of the driver firmware, it is appropriate to put this piece of
code in drivers/firmware/ffi.c.
So I insist on the current revision, what do you think?
Thanks,
Yunhui
On Mon, 26 Jun 2023 at 10:05, 运辉崔 <[email protected]> wrote:
>
> Hi Ard,
>
> On Mon, Jun 26, 2023 at 2:43 PM Ard Biesheuvel <[email protected]> wrote:
>
> > I think all of this belongs under arch/riscv
>
> Could you look at the content of the patch again? As we discussed
> before, we need to connect to the ACPI and the SMBIOS entry
> At least this part of the code has to be placed in the corresponding place:
> drivers/acpi/osl.c: acpi_os_get_root_pointer()
> drivers/firmware/dmi_scan.c:dmi_scan_machine()
>
> Because obtaining firmware information through DTS belongs to the
> content of the driver firmware, it is appropriate to put this piece of
> code in drivers/firmware/ffi.c.
>
> So I insist on the current revision, what do you think?
>
DT support for SMBIOS can live in generic code, but the binding has to
be sane. As I suggested before, it probably makes sense to supplant
the entrypoint rather than just carry its address - this means a 'reg'
property with base and size to describe the physical region, and at
least major/minor/docrev fields to describe the version.
In any case, there is really no point in supporting both entrypoints
(this applies to the ACPI root pointer as well).
For the ACPI side, you should just implement
acpi_arch_get_root_pointer() under arch/riscv, and wire it up in
whichever way you want. But please check with the RISC-V maintainers
if they are up for this, and whether they want to see this mechanism
contributed to one of the pertinent specifications.
So NAK on the current revision, in case this was unclear.
Hi Ard, Mark,
On Mon, Jun 26, 2023 at 4:23 PM Ard Biesheuvel <[email protected]> wrote:
> DT support for SMBIOS can live in generic code, but the binding has to
> be sane. As I suggested before, it probably makes sense to supplant
> the entrypoint rather than just carry its address - this means a 'reg'
> property with base and size to describe the physical region, and at
> least major/minor/docrev fields to describe the version.
Regarding dts node binding, our current definition is as follows:
/dts
{
...
cfgtables {
acpi_phy_ptr = 0000000000000000; //u64
smbios_phy_ptr = 0000000000000000; //u64
...
}
...
}
x86 only gave a root_pointer entry address
u64 x86_default_get_root_pointer(void)
{
return boot_params.acpi_rsdp_addr;
}
Regarding the naming of the binding above, Mark, do you have any suggestions?
> For the ACPI side, you should just implement
> acpi_arch_get_root_pointer() under arch/riscv, and wire it up in
> whichever way you want. But please check with the RISC-V maintainers
> if they are up for this, and whether they want to see this mechanism
> contributed to one of the pertinent specifications.
You suggest putting SMBIOS in general code instead of ACPI, why?
From the perspective of firmware information passing, they are a class.
SMBIOS and ACPI are not related to ARCH, nor is DTS to obtain firmware
information,
Why do you have to put part of the ACPI code under arch/risc-v/?
The scope of the previous discussion was limited to RISC-V because of
historical reasons such as the binding with EFI on ARM64. We will only
enable this function on RISC-V in subsequent patches.
The realization of the FFI scheme itself is irrelevant to the arch.
Thanks，
Yunhui
Hi Ard,
1. Regarding the definition of DTS FFI nodes, according to your
suggestion, we plan to make the following modifications：
/ {
...
ffi_cfg {
acpi_tbl {
root_pinter = ; //u64
...
};
smbios_tbl {
root_pinter = ; //u64
...
};
};
...
};
2. Let's move on to the discussion: should we put code under arch/risc-v/?
On Mon, Jun 26, 2023 at 6:19 PM 运辉崔 <[email protected]> wrote:
>
> Hi Ard, Mark,
>
> On Mon, Jun 26, 2023 at 4:23 PM Ard Biesheuvel <[email protected]> wrote:
>
> > DT support for SMBIOS can live in generic code, but the binding has to
> > be sane. As I suggested before, it probably makes sense to supplant
> > the entrypoint rather than just carry its address - this means a 'reg'
> > property with base and size to describe the physical region, and at
> > least major/minor/docrev fields to describe the version.
>
> Regarding dts node binding, our current definition is as follows:
> /dts
> {
> ...
> cfgtables {
> acpi_phy_ptr = 0000000000000000; //u64
> smbios_phy_ptr = 0000000000000000; //u64
> ...
> }
> ...
> }
>
> x86 only gave a root_pointer entry address
> u64 x86_default_get_root_pointer(void)
> {
> return boot_params.acpi_rsdp_addr;
> }
>
> Regarding the naming of the binding above, Mark, do you have any suggestions?
>
>
> > For the ACPI side, you should just implement
> > acpi_arch_get_root_pointer() under arch/riscv, and wire it up in
> > whichever way you want. But please check with the RISC-V maintainers
> > if they are up for this, and whether they want to see this mechanism
> > contributed to one of the pertinent specifications.
>
> You suggest putting SMBIOS in general code instead of ACPI, why?
> From the perspective of firmware information passing, they are a class.
>
> SMBIOS and ACPI are not related to ARCH, nor is DTS to obtain firmware
> information,
>
> Why do you have to put part of the ACPI code under arch/risc-v/?
> The scope of the previous discussion was limited to RISC-V because of
> historical reasons such as the binding with EFI on ARM64. We will only
> enable this function on RISC-V in subsequent patches.
>
> The realization of the FFI scheme itself is irrelevant to the arch.
>
> Thanks，
> Yunhui
(cc RISC-V maintainers and mailing list)
On Mon, 26 Jun 2023 at 12:20, 运辉崔 <[email protected]> wrote:
>
> Hi Ard, Mark,
>
> On Mon, Jun 26, 2023 at 4:23 PM Ard Biesheuvel <[email protected]> wrote:
>
> > DT support for SMBIOS can live in generic code, but the binding has to
> > be sane. As I suggested before, it probably makes sense to supplant
> > the entrypoint rather than just carry its address - this means a 'reg'
> > property with base and size to describe the physical region, and at
> > least major/minor/docrev fields to describe the version.
>
> Regarding dts node binding, our current definition is as follows:
> /dts
> {
> ...
> cfgtables {
> acpi_phy_ptr = 0000000000000000; //u64
> smbios_phy_ptr = 0000000000000000; //u64
> ...
> }
> ...
> }
>
> x86 only gave a root_pointer entry address
> u64 x86_default_get_root_pointer(void)
> {
> return boot_params.acpi_rsdp_addr;
> }
>
> Regarding the naming of the binding above, Mark, do you have any suggestions?
>
I will defer to Mark or other DT experts to help decide on the naming
and general shape of these.
However, as I have indicated twice now, it would be better to describe
the SMBIOS structured data directly, instead of passing the physical
address of one of the existing entry points. This avoids the need for
mapping and reserving additional pages that don't carry any relevant
information.
So the node in question should have at least (base, size) and the
major, minor and docrev version fields.
>
> > For the ACPI side, you should just implement
> > acpi_arch_get_root_pointer() under arch/riscv, and wire it up in
> > whichever way you want. But please check with the RISC-V maintainers
> > if they are up for this, and whether they want to see this mechanism
> > contributed to one of the pertinent specifications.
>
> You suggest putting SMBIOS in general code instead of ACPI, why?
SMBIOS is a separate set of firmware tables that have little
significance to the kernel itself, and describing it via DT makes
sense.
ACPI serves a similar purpose as DT, and so having both at the same
time results in a maintenance burden, where the arch code is forced to
reason about whether they are consistent with each other, and if not,
which description has precedence.
> From the perspective of firmware information passing, they are a class.
>
> SMBIOS and ACPI are not related to ARCH, nor is DTS to obtain firmware
> information,
>
> Why do you have to put part of the ACPI code under arch/risc-v/?
Yes. And I don't think it should be using this FFI scheme either.
If the firmware uses DT as a conduit to deliver the ACPI system
description to the OS, it is probably better to pass this via the
/chosen node as a special boot argument.
> The scope of the previous discussion was limited to RISC-V because of
> historical reasons such as the binding with EFI on ARM64. We will only
> enable this function on RISC-V in subsequent patches.
>
> The realization of the FFI scheme itself is irrelevant to the arch.
>
I don't think we need a FFI scheme or framework for this.
Hi Ard,
>
> I will defer to Mark or other DT experts to help decide on the naming
> and general shape of these.
Okay, thanks.
> However, as I have indicated twice now, it would be better to describe
> the SMBIOS structured data directly, instead of passing the physical
> address of one of the existing entry points. This avoids the need for
> mapping and reserving additional pages that don't carry any relevant
> information.
>
> So the node in question should have at least (base, size) and the
> major, minor and docrev version fields.
Other platforms also need related memory to store this table, don't they?
Coreboot also completes the construction of this table according to
its existing code, rather than creating a new description method.
Furthermore, As we discussed before, SMBIOS-related code should be
placed in the general code, and an entry address is required to
connect to dmi_scan_machine().
according to what you said (base, size, region) how can it be
connected to dmi_scan with an entry address?
So, For SMBIOS, only keep the smbios part in drivers/firmware/ffi.c in
this patch? If yes in terms of code structure, I will update it in v2.
> SMBIOS is a separate set of firmware tables that have little
> significance to the kernel itself, and describing it via DT makes
> sense.
>
> ACPI serves a similar purpose as DT, and so having both at the same
> time results in a maintenance burden, where the arch code is forced to
> reason about whether they are consistent with each other, and if not,
> which description has precedence.
>
Well... I don’t want to discuss too much, according to your
suggestion, To implement acpi_arch_get_root_pointer() under
arch/riscv.
I'll update it on v2.
> If the firmware uses DT as a conduit to deliver the ACPI system
> description to the OS, it is probably better to pass this via the
> /chosen node as a special boot argument.
>
This is not the focus of our discussion this time, and we will discuss
it when we discuss node naming with DTS experts.
Thanks,
Yunhui