Re: [PATCH 1/1] tools/libacpi: clear ASL warning about PCI0

["Ariel Otilibili-Anieli" <Ariel.Otilibili-Anieli@xxxxxxxxxx>]

On Monday, December 16, 2024 13:39 CET, Jan Beulich <jbeulich@xxxxxxxx> wrote:

> On 16.12.2024 13:19, Ariel Otilibili-Anieli wrote:
> > On Monday, December 16, 2024 12:38 CET, Jan Beulich <jbeulich@xxxxxxxx> 
> > wrote:
> > 
> >> On 16.12.2024 12:31, Ariel Otilibili-Anieli wrote:
> >>> On Monday, December 16, 2024 12:01 CET, Jan Beulich <jbeulich@xxxxxxxx> 
> >>> wrote:
> >>>
> >>>> On 16.12.2024 11:36, Ariel Otilibili-Anieli wrote:
> >>>>> On Monday, December 16, 2024 10:53 CET, Jan Beulich <jbeulich@xxxxxxxx> 
> >>>>> wrote:
> >>>>>
> >>>>>> On 15.12.2024 16:40, Ariel Otilibili wrote:
> >>>>>>> * iasl complains _HID and _ADR cannot be used at the same time
> >>>>>>>
> >>>>>>> ```
> >>>>>>> /usr/bin/iasl -vs -p tools/firmware/hvmloader/dsdt_anycpu.tmp -tc 
> >>>>>>> tools/firmware/hvmloader/dsdt_anycpu.asl 2>&1 | grep -B10 HID
> >>>>>>> tools/firmware/hvmloader/dsdt_anycpu.asl     40:        Device (PCI0)
> >>>>>>> Warning  3073 -                                    Multiple types ^  
> >>>>>>> (Device object requires either a _HID or _ADR, but not both)
> >>>>>>> ```
> >>>>>>>
> >>>>>>> * generally _HID devices are enumerated and have their drivers loaded 
> >>>>>>> by ACPI
> >>>>>>> * this is from "ASL 2.0 Introduction and Overview" (page 4).
> >>>>>>> * removing _ADR, the warning is cleared out.
> >>>>>>
> >>>>>> Okay, that's the positive aspect. Yet what about the potential fallout 
> >>>>>> thereof?
> >>>>>> Can you confirm that there's no risk of regressions with older guest 
> >>>>>> OSes, for
> >>>>>> example?
> >>>>>
> >>>>> OSes that were released after ACPI 2.0 should work [1]; including 
> >>>>> WinXP: 
> >>>>> The 2.0 specs says either _HID or _ADR should be included [2], not both 
> >>>>> (Section 6.1, page 146).
> >>>>
> >>>> We must be looking at two different variants of the spec then. My copy 
> >>>> says
> >>>> "device object must contain either an _HID object or an _ADR object, but 
> >>>> can
> >>>> contain both." Also still in 2.0c. I agree that in e.g. 6.5 the wording 
> >>>> has
> >>>> changed. I also agree that the use of "either" doesn't help clarity.
> >>>
> >>> I looked up 2.0 (July 2000); indeed, it said "can contain both". My bad.
> >>>>
> >>>>> I chose WinXP because, on another patch, it came up in the discussion 
> >>>>> [3].
> >>>
> >>> The change should work down to WinXP: the name _HID is kept.
> >>>
> >>> ```
> >>> $ git grep -B2 -A2 -n PNP0A03
> >>> tools/libacpi/dsdt.asl-40-       Device (PCI0)
> >>> tools/libacpi/dsdt.asl-41-       {
> >>> tools/libacpi/dsdt.asl:42:           Name (_HID, EisaId ("PNP0A03"))
> >>> tools/libacpi/dsdt.asl-43-           Name (_UID, 0x00)
> >>> tools/libacpi/dsdt.asl-44-           Name (_ADR, 0x00)
> >>> ```
> >>>
> >>> Its EISA ID is "PNP0A03"; the namespace is reserved for Microsoft. 
> >>> Microsoft identifies "PNP0A03" as PCI devices [1].
> >>
> >> You again say "should" without explaining what you derive this from. Is it
> >> written down somewhere that no OS we (remotely) care about ever evaluated
> >> _ADR when _HID was there? As before, along side mentioning the benefits of
> >> the change, I'd like to also see a discussion of risks.
> >>
> > 
> > I derive this knowledge only from the APCI specs. Indeed, I've not 
> > researched how every OS interprets _HID and _ADR.
> > 
> > From your answer, I see you would like to be sure the change will introduce 
> > no regression. I do understand you point of view; keeping the backward 
> > compatibility. 
> > 
> > The benefit is that the warning will be cleared. We agree on that.
> > 
> > The risk is that, if ever any OS relies on _ADR, and cannot read _HID; it 
> > would break.
> > 
> > After thinking about it, the other way is less risky: this _HID name is 
> > only recognized by Windows. Every OS should (I did say it again, should) 
> > understand _ADR.
> > 
> > If you think the change makes sense, I can remove _HID instead of _ADR.
> 
> But that would remove relevant information, the the PNP ID serves a purpose.

You are right, I dumped the DSDT tables of my Linux, and the PCI object does 
use it:

```
# cat /sys/firmware/acpi/tables/DSDT > dsdt.dat 
# iasl -e dsdt.dat -d dsdt.dsl
# grep PNP0A03 -B3 dsdt.dsl 
        Device (PC00)
        {
            Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */)  // _HID: 
Hardware ID
            Name (_CID, EisaId ("PNP0A03") /* PCI Bus */)  // _CID: Compatible 
ID
```
> 
> > Otherwise, I think we should end the discussion.
> 
> Well, you may decide to withdraw / abandon the patch, or you may decide to
> re-submit with an extended description, clarifying why the removal is
> expected to be safe. Even if - obviously - you can't inspect e.g. Windows
> sources.

Thanks for the feedback, Jan; I am pushing another patch series. Documenting 
all the findings.

I'll keep you posted,
Ariel
> 
> Jan