Re: [PATCH 3/3] Documentation/arm64: Update ACPI tables from BBR

[Hanjun Guo <guohanjun@huawei.com>]

On 2023/5/22 18:55, Jose Marinho wrote:
> Hi Hanjun, Robin,
> 
>> On 2023-05-19 08:10, Hanjun Guo wrote:
>>> On 2023/5/18 21:40, Robin Murphy wrote:
>>>> On 2023-05-18 13:07, Hanjun Guo wrote:
>>>>> Hi Jose,
>>>>>
>>>>> On 2023/5/18 18:52, Jose Marinho wrote:
>>>>>> The BBR specification requires (or conditionally requires) a set of
>>>>>> ACPI tables for a proper working system.
>>>>>> This commit updates:
>>>>>> - the list of ACPI tables to reflect the contents of BBR version
>>>>>> 2.0 (seehttps://developer.arm.com/documentation/den0044/g).
>>>>>> - the list of ACPI tables in acpi_object_usage. This last update
>>>>>> ensures that both files remain coherent.
>>>>> Thanks for the update, some comments inline.
>>>>>
>>>>>> Signed-off-by: Jose Marinho<jose.marinho@arm.com>
>>>>>> Reviewed-by: Samer El-Haj-Mahmoud <Samer.El-Haj-
>> Mahmoud@arm.com>
>>>>>> ---
>>>>>>    Documentation/arm64/acpi_object_usage.rst | 81
>>>>>> +++++++++++++++++++++--
>>>>>>    Documentation/arm64/arm-acpi.rst          | 71
>>>>>> +++++++++++++++++---
>>>>>>    2 files changed, 139 insertions(+), 13 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/arm64/acpi_object_usage.rst
>>>>>> b/Documentation/arm64/acpi_object_usage.rst
>>>>>> index 484ef9676653..1da22200fdf8 100644
>>>>>> --- a/Documentation/arm64/acpi_object_usage.rst
>>>>>> +++ b/Documentation/arm64/acpi_object_usage.rst
>>>>>> @@ -17,16 +17,37 @@ For ACPI on arm64, tables also fall into the
>>>>>> following categories:
>>>>>>           -  Recommended: BERT, EINJ, ERST, HEST, PCCT, SSDT
>>>>>> -       -  Optional: BGRT, CPEP, CSRT, DBG2, DRTM, ECDT, FACS,
>>>>>> FPDT, IBFT,
>>>>>> -          IORT, MCHI, MPST, MSCT, NFIT, PMTT, RASF, SBST, SLIT,
>>>>>> SPMI, SRAT,
>>>>>> -          STAO, TCPA, TPM2, UEFI, XENV
>>>>>> +       -  Optional: AGDI, BGRT, CEDT, CPEP, CSRT, DBG2, DRTM,
>>>>>> +ECDT,
>>>>>> FACS, FPDT,
>>>>>> +          HMAT, IBFT, IORT, MCHI, MPAM, MPST, MSCT, NFIT, PMTT,
>>>>>> PPTT, RASF, SBST,
>>>>>> +          SDEI, SLIT, SPMI, SRAT, STAO, TCPA, TPM2, UEFI, XENV
>>>>>> -       -  Not supported: BOOT, DBGP, DMAR, ETDT, HPET, IVRS, LPIT,
>>>>>> MSDM, OEMx,
>>>>>> -          PSDT, RSDT, SLIC, WAET, WDAT, WDRT, WPBT
>>>>>> +       -  Not supported: AEST, APMT, BOOT, DBGP, DMAR, ETDT, HPET,
>>>>>> IVRS, LPIT,
>>>>> AEST is ARM Error Source Table, and it can be used for ARM
>>>>> platforms, so I thinsk AEST is not belong to "Not supportted", "Optional"
>> instead.
>>>> Can you point to the code in Linux which does anything with AEST,
>>>> optionally or otherwise?;)
>>>>> and APMT is the same.
>>>>>
>>>>>> +          MSDM, OEMx, PDTT, PSDT, RAS2, RSDT, SLIC, WAET, WDAT,
>>>>>> WDRT, WPBT
>>>>> PDTT and RAS2 are now used for ARM too, please move it to Optional
>>>>> :)
> The 6.3 kernel does not yet have code consuming either table.
> 
> Perhaps the categories {"Required", "Recommended", "Optional", "Not supported"}
> listed in Documentation/arm64/acpi_object_usage.rst should be defined.
> 
> My opinion (which may be unaligned with the original intent behind the categories) is, If a table
> is consumed by kernel code, then it is supported, i.e. in {"Required", "Recommended", "Optional"}.
> Otherwise, the table is "Not supported".
> 
>>>> Ditto; as stated in arm-acpi.rst this is Linux documentation covering
>>>> the interaction between Linux and ACPI. It is not some kind of
>>>> generic
>>> Hmm, let me see...
>>>
>>> OK, I checked the arm-acpi.rst, it is saying:
>>>
>>> "Detailed expectations for ACPI tables and object are listed in the
>>> file Documentation/arm64/acpi_object_usage.rst."
>>>
>>> So if I remember correctly, it is the guidance of ACPI tables and
>>> methods usage on arm64, to align with the BBR.
>> "The purpose of this document is to describe the interaction between ACPI
>> and Linux only, on an ARMv8 system -- that is, what Linux expects of ACPI
>> and what ACPI can expect of Linux."
>>
>> I don't see how it could get much clearer than that. Yes, phrasing like "ACPI
>> on arm64" is used elsewhere, but remember that in context "arm64"
>> means "AArch64 Linux".
>>
>>>> ACPI-on-Arm guidance whitepaper. If and when Linux actually supports
>>>> these tables in the sense of meaningfully consuming them, that is
>>>> when we can document such support.
>>> If this is the case, we don't need categories of "Required",
>>> "Recommmened" and etc.
>> Certainly the distinction between required and optional is significant and
>> useful, since Linux may fail to boot at all if a required table is missing. I'd
>> agree I can't really make sense of the "recommended"
>> category though - it's not like firmware could make up RAS support if the
>> hardware doesn't have it, and whether SSDTs are appropriate or not seems
>> to depend on the fundamental design of the system, rather than being
>> something an OS should dictate :/
>>
> I agree with Robin that it isn’t clear what "Recommended" versus "Optional" signifies.
> Maybe the distinction should be better discussed, and we can make those clarifications in a separate change?

Looks good to me.

Thanks
Hanjun