Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ACPI: Add requirement to have PLIC/APLIC namespace devices #143

Merged
merged 2 commits into from
Apr 15, 2024

Conversation

vlsunil
Copy link
Collaborator

@vlsunil vlsunil commented Apr 11, 2024

PLIC and APLIC need to be namespace devices as well so that devices can add dependencies using _DEP. This is required for the OS to ensure drivers are probed in proper order.

So, this commit

  1. Creates a new section under ACPI to list of ACPI IDs maintained for RVI standard devices.
  2. Adds new requirement to mandate namespace devices for PLIC and APLIC. 3) Mandates _GSB required to map the namespace device to MADT. 4) Mandates _DEP to indicate dependency between devices and interrupt controller for GSI interrupts.

This is discussed in PRS meeting on 04/11/2024. This should close #127

PoC available at:
https://github.com/vlsunil/qemu/tree/riscv_acpi_dep_v1
https://github.com/vlsunil/linux/tree/acpi_b2_v4_dep_v1

@vlsunil
Copy link
Collaborator Author

vlsunil commented Apr 11, 2024

@andreiw @atishp04 @xypron : Please help reviewing this.

acpi.adoc Outdated
@@ -64,9 +69,16 @@ objects.
| [[acpi-tad]] AML_070 | Systems with a Real-Time Clock on an OS-managed bus (e.g. I2C, subject to arbitration issues due to access to the bus by the OS) MUST implement the Time and Alarm Device (TAD).
2+| _Also see <<uefi-rtc, URT_020>>_.
| AML_080 | Systems implementing a TAD must be functional without additional system-specific OS drivers.
| [[acpi-irq-gsb]] AML_090 | PLIC and APLIC devices must support
**_GSB** method. <<acpi-ids, See RVI ACPI IDs>>.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest this as being _GSB (Global System Interrupt Base), with no need for bolding as we haven't done that elsewhere

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better would be something like Global System Interrupt Base (_GSB, cite:[ACPI] Section 6.2.7).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if we move away from MADT entries, then these devices would also return _MAT, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_MAT is not really mandatory. Even IOAPIC namespace device don't need _MAT. OS will look at the MADT entry directly using _GSB.

acpi.adoc Show resolved Hide resolved
Copy link
Collaborator

@andreiw andreiw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all thanks for the PR. I've left comments on the the actual adoc contribution, which are trivial in nature (please build and make sure it looks ok).

While not a blocker for the contribution proper, I'd like to understand why on earth ACPI on RISC-V has to be different from ACPI on anything else, and whether we have ASWG consensus any of this being a good idea. Linux kernel maintainers are not ASWG and it's not clear the right level of ACPI expertise is being applied.

acpi.adoc Outdated
**_GSB** method. <<acpi-ids, See RVI ACPI IDs>>.
| [[acpi-irq-dep]] AML_100 | Devices which use Global System
Interrupts (GSI) must indicate the dependency on corresponding
interrupt controller using **_DEP** method.
Copy link
Collaborator

@andreiw andreiw Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a comment on your text but... this is still just so weird. Have you taken this to ASWG? This is ACPI, not Device Tree. We can't just make up stuff just because someone in Linux thinks it makes sense. There's no reason ACPI on RISC-V has to be done in (what looks like) an entirely crazy fashion. This is just so....alien to ACPI on every architecture out there. Why can't the OS take an implicit dependency, then look up the IC controller based on GSI range in the MADT entry?

Copy link
Collaborator

@andreiw andreiw Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmcneill-vmware your feedback on this from a NetBSD perspective?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if there's an AML-centric mechanism adopted for interrupt controller discovery, do we still need the relevant MADT entries? What value would those provide if they are not used by Linux?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Andrei. Good questions.

The issue is primarily with APLIC. Wired-to-MSI bridges are kind of new class of devices. Other architectures support either MSI or wired. The need for this type of device came in ARM fairly recently (compared to the existence of the CPU architectures) with MBIGEN device from HiSilicon and framework support was added to support such class of interrupt controllers. (REF: https://lwn.net/Articles/660893/).

The question of probing order came up with MBIGEN as well. (REF: https://lore.kernel.org/linux-arm-kernel/563B22F1.901@arm.com/). Obviously, using EPROBE_DEFER in all possible drivers will get profound NAK. So, people came up with fw_devlink framework to resolve these issues and used in DT. You can see that ACPI support for MBIGEN was also added later (https://yhbt.net/lore/all/58D974D2.7060809@linaro.org/) which is also a namespace device (HISI0152). The probing order issue in ACPI was resolved using ResourceSource since it doesn't support fw_devlink.

As I can see, MBIGEN is used only with limited set of devices for a platform. So, using ResourceSource will work. Other devices can still continue to use the wired interrupt space in GIC. But APLIC is more generic which needs to be supported for any device using Wired interrupts including PCI INTx pins. There are many devices/drivers (ex: PNP, GED etc) which are probed differently in ACPI assuming interrupt controllers are probed early. If such devices are required to be supported with MBIGEN, I am sure they will have the same issue as APLIC. So, unless probing of such devices itself is delayed, it is going to cause issue one way or other. Hence, _DEP is recommended which is simple to use.

Using _DEP for this type of ensuring probing order is not new. For ex: https://patchwork.kernel.org/project/linux-media/patch/20211203102857.44539-2-hdegoede@redhat.com/.

Regarding the question of MADT and namespace device: Namespace devices need more details. It can either support _MAT or lookup MADT directly. MBIGEN since it is not a standard architecture interrupt controller, it used _DSD which IMO we should avoid. The reason I kept MADT is, some other OS may be able to use MADT and ignore namespace devices.
So, should we make that either _MAT or MADT ?

Copy link
Collaborator

@andreiw andreiw Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the in-depth explanation!

Can we avoid the _DEP for PLIC systems, then, since these are wire interrupts and not using wire-to-msi? That way it's obvious the _DEP is for a reason, not a blanket policy just for RV.

Using _DEP is not necessarily new, but taking a mandatory _DEP in every Device (...) with an interrupt resource most surely is. And you have to know which one you have to _DEP on, which is going to be a trainwreck IMHO. Can't the OS synthesize an implicit "GSI interrupt" device, which every other ACPI device can depend on, that will handle the lazy probing and initialization of the APLIC (based on MADT, based on Device()s with a certain HID and the _GSB, w/e)?

Can the _DEP target a "container" device that contains all the other devices? Can the _DEP be targeted towards a /single/ Device() object, which can contain the actual APLIC children with their _GSBs? How do we make this not seem like a bunch of crutches or be terrible to implement/maintain for IFVs?

( The MBIGEN approach doesn't raise any questions in my mind. Those aren't GSIs. It's using the correct mechanism, ResourceSource, to deal with these not-GSI interrupts. The ResourceSource handling in a kernel may mean lazy binding. )

Like I said, not a gate for this PR at this point (it's exactly the kind of short term solution that makes sense to put into the BRS), but at the end of the day this is a Linux-specific implementation issue leaking into a spec. Do we have buy-in from the ACPI spec body - the ASWG?

My other worry is if interrupt controller initialization is moved entirely to depend on AML nodes, then the "de facto" OS (Linux) is not going to use the MADT entries. This will mean this to become an untested path, and likely filled with garbage that other OSes will just trip on. Keeping the same info in both places is not a good idea.

Copy link
Collaborator

@atishp04 atishp04 Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per our discussion in the PRS TG, this has not discussed in the ASWG yet. I think it would be good to just discuss the problem space and propose the solutions to see if there is a better way to do things. I understand Linux ACPI/irq maintainers doesn't attend ASWG but if there is a better way proposed in ASWG, we may be able to convince the ACPI maintainers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the in-depth explanation!

Can we avoid the _DEP for PLIC systems, then, since these are wire interrupts and not using wire-to-msi? That way it's obvious the _DEP is for a reason, not a blanket policy just for RV.

I thought it will be more confusing for people when to use _DEP if we make it required only for APLIC because APLIC also has direct mode.

Using _DEP is not necessarily new, but taking a mandatory _DEP in every Device (...) with an interrupt resource most surely is. And you have to know which one you have to _DEP on, which is going to be a trainwreck IMHO.

I share the same concern, but this situation needs a new solution IMO. It can be either in OS or FW. I think these discussions would eventually lead to some good solution.

Can't the OS synthesize an implicit "GSI interrupt" device, which every other ACPI device can depend on, that will handle the lazy probing and initialization of the APLIC (based on MADT, based on Device()s with a certain HID and the _GSB, w/e)?

That is fw_devlink approach I tried which is not getting traction from ACPI maintainer. Also, _PRT is another way dependency exists which makes it tricky to use this approach. _DEP has advantage that it ensures scan itself is deferred if dependencies are not met.

Can the _DEP target a "container" device that contains all the other devices? Can the _DEP be targeted towards a /single/ Device() object, which can contain the actual APLIC children with their _GSBs? How do we make this not seem like a bunch of crutches or be terrible to implement/maintain for IFVs?

Hmm, I will have to think about it. IIUC, you are trying to avoid mapping exact interrupt controller for a device. But that kind of mapping he has to do any way with GSIs no?

( The MBIGEN approach doesn't raise any questions in my mind. Those aren't GSIs. It's using the correct mechanism, ResourceSource, to deal with these not-GSI interrupts. The ResourceSource handling in a kernel may mean lazy binding. )

Like I said, not a gate for this PR at this point (it's exactly the kind of short term solution that makes sense to put into the BRS), but at the end of the day this is a Linux-specific implementation issue leaking into a spec. Do we have buy-in from the ACPI spec body - the ASWG?

I submitted this PR because of BRS deadline of ARC review. How much time do we have or can we keep this open item and close as part of ratification?
Let me start a mail thread with ASWG and copy Rafael.

My other worry is if interrupt controller initialization is moved entirely to depend on AML nodes, then the "de facto" OS (Linux) is not going to use the MADT entries. This will mean this to become an untested path, and likely filled with garbage that other OSes will just trip on. Keeping the same info in both places is not a good idea.
Not really. Like I said, OS will look at MADT entries itself and _MAT is not required. IOAPIC works in same way in linux. Namespace device is required only to ensure probe ordering.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can take this PR into the BRS. This follows our policy of taking in the things we need to make progress instead of waiting on the upstream to fix itself. All I'm saying is that it's super important to seek alignment with ASWG instead of ending up defining our own ACPI flavor.

It should be safe to add the _DEP requirement here. Worst case (or best case, I suppose, depending on how you look at it), the requirement gets deprecated in the future but has no ill effect on implementations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Let me try adding dependencies automatically without needing explicit _DEP. If the maintainer agrees, we can remove this _DEP requirement, just keep the namespace device + _GSB. I hope to conclude before freeze deadline.

[%header, cols="5,25"]
|===
| ACPI ID ^| Device
| RSCV0001 | RISC-V Platform-Level Interrupt Controller (PLIC)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a section to the non-normative ACPI guidance on examples of Device (...) definitions for both of these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an example for PLIC. I didn't see need to repeat the same thing for just HID change. Let me know if I should.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good.

acpi.adoc Outdated
**_GSB** method. <<acpi-ids, See RVI ACPI IDs>>.
| [[acpi-irq-dep]] AML_100 | Devices which use Global System
Interrupts (GSI) must indicate the dependency on corresponding
interrupt controller using **_DEP** method.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this in PRS TG. This should be optional. Other OS may not have the same issue. Firmware doesn't need to specify _GSB and _DEP in that case. Isn't it ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but FW will be common for different OSs. So I think even if one OS needs it, it will become mandatory for firmware. I was initially thinking keeping it bit open saying "Devices should indicate the dependency appropriately" without mentioning any ways like _DEP. But I guess that might cause confusion as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that wouldn't be really actionable, because "indicating appropriately" sounds like being able to enumerate every crutch needed by every OS kernel out there :-).

vlsunil added 2 commits April 15, 2024 17:30
…evices

Add an example which shows how PLIC/APLIC namespace devices can be added
and how devices which use GSIs can indicate the dependency.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
PLIC and APLIC need to be namespace devices as well so that devices can
add dependencies using _DEP. This is required for the OS to ensure
drivers are probed in proper order.

So, this commit
1) Creates a new section under ACPI to list of ACPI IDs maintained for
RVI standard devices.
2) Adds new requirement to mandate namespace devices for PLIC and APLIC.
3) Mandates _GSB required to map the namespace device to MADT.
4) Mandates _DEP to indicate dependency between devices and interrupt
controller for GSI interrupts.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
@andreiw andreiw merged commit 241575b into riscv-non-isa:main Apr 15, 2024
2 checks passed
@vlsunil vlsunil deleted the acpi_namespace branch May 21, 2024 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ACPI Namespace devices for APLIC and PLIC
3 participants