Skip to content

Conversation

@mark-honman
Copy link

Each of the 6 commits in the PR corresponds to one of the issues that I've recently opened to make the ACLIC spec more readable.

#615 ACLIC is introduced as a 3rd AIA external interrupt controller option, with a hart local interface like an IMSIC but implementing APLIC like source configuration and interrupt prioritization.

#617 An IMSIC like eidelivery register can be used without changes - since eidelivery = 1 means delivery from the interrupt files inside a hart-local interrupt controller. Once the ACLIC is presented as an aliternative to IMSIC, rather than a heavily modified APLIC, the lengthly description of mapping between APLIC and IMSIC type registers is no longer required.

#618 Indirect source and priority configuration is described as a way to merge the wired-interrupt configuration capabilities of APLIC, into a hart-local interrupt controller as an extension to the set of ei-prefixed indirect CSRs in an interrupt file. Register naming and addressing are changed to align with existing ei-regs BUT I have not checked that the suggested part of CSRind space is in fact available. Since ACLIC contains interrupt files rather than APLIC like domains, the more limited sourcecfg use model is explained in detail.

#618 State enables - attempted to update this to match the renaming in the previous commit; however I'm not familiar with state enables and have probably got it wrong.

#619 "APLIC configured for ACLIC operation" section is deleted, because it seems to be based on an early iteration of the spec in which ACLIC was more like a specialized APLIC. I found this section especially confusing because it was not clear whether the referenced APLIC was an actual APLIC, or a set of APLIC like registers inside ACLIC that are not described elsewhere.

#622 Smaiae is removed as an extension in its own right; we found that AIA provides guidance on a minimal implementation of S-mode mvip and mvien that does not have an excessive area penalty. The AIA recommendations are noted in the Smaclic/Ssaclic extension description.

Expanded introduction describes ACLIC as a 3rd type of AIA external interrupt controller; since it is closely coupled to a hart the existing AIA CSR interface for close coupled interrupt controllers is used.
Describes eidelivery etc. with reference to their description in the AIA IMSIC section. there are no interactions with APLIC registers, so that part of the text is removed.
Text rewritten for clarity.
Registers renamed to have an ei prefix because they are associated with interrupt files.
Delegation described with reference to AIA.
Address layout changed IN THE HOPE that CSRind addresses 0x100 - 0x1FF are available.
…iscv#618

Updated the state enable to use the ei prefixed names for source and priority configuration. NEEDS REVIEW.
Section seems to presume an APLIC within or alongside the ACLIC. However the rest of the spec does not describe how they would integrate, or what the benefit might be - because the key APLIC source and priority configuration has already been covered, and the APLIC features described in this section are only relevant to a shared platform-level APLIC.
The AIA spec describes a minimal implementation of mvip and mvien, which does not carry a significant area penalty. The Smaiae section is removed, and a Smaclic/Ssaclic section "Area impact of AIA features" explains how area can be reduced when needed.
Note:
* A hart cannot have more than one closely coupled external interrupt controller;
* in particular a hart cannot have both an ACLIC and an IMSIC.
* A system can contain an APLIC as well as a closely coupled ACLIC; in this case {eidelivery} is
Copy link
Collaborator

Choose a reason for hiding this comment

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

how does this selection work? You removed the eidelivery changes.

Copy link
Author

Choose a reason for hiding this comment

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

eidelivery is a register in the interrupt file of a hart-connected interrupt controller. It determines whether the result of the interrupt file's filter tree goes to the external interrupt input of the hart's major interrupt controller corresponding to the mode of the interrupt file. AIA provides a "special case" where the external interrupt input is connected directly to the output of an APLIC domain. That's all still supported.
My understanding is that we don't need an extra eidelivery bit because there is no possibility for IMSIC and ACLIC to coexist, because they occupy the same CSRind space.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But then you would still have to change the meaning of the IMSIC e delivery value to be IMSIC or ACLIC

Copy link
Author

Choose a reason for hiding this comment

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

IMO we should keep the original AIA eidelivery definition until we have a specification of how ACLIC and IMSIC could possibly be attached to the same hart. In the meantime I will draft a Smaclicmsi extension...

Copy link
Collaborator

Choose a reason for hiding this comment

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

but the current text is missing the notion that IMSIC MSI delivery is repurposed for ACLIC mode, right?

Copy link
Author

Choose a reason for hiding this comment

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

In yesterday's long discussion with Alex, he came up with a potentially useful way to distinguish ACLIC and IMSIC. i.e. that IMSIC has a "front end" that has an MMIO and receives MSIs; and a "back end" that contains the interrupt files and interfaces to the hart. So, conceptually what ACLIC is doing is

  1. replacing the front-end with a wire-triggered interface (with sourcecfg based configuration).
  2. adding SW configurable priorities to the front-end.

What I'm not sure of is how ACLIC priorities interact with major interrupt priorities in Smnip, i.e. whether it is a single prioritization space, or 2 separate levels. That will affect the way we describe the relationship between ACLIC and IMSIC.

But as I think about it, the more I'm convinced that there is no value in design having both ACLIC and IMSIC (wasted area, as only one can be used); rather we provide an extension that adds an IMSIC front-end to ACLIC so that both wired interrupts and MSIs can be handled (I think it's possible to do this without area getting out of hand).

In this case the {eip} and {eie} bits and source and priority configuration for the unimplemented
identities are WARL 0.

For XLEN=32, {eipk} and {eiek} access {setip}[_k_]/{in_clrip}[_k_] and {setie}[_k_]/{clrie}[_k_], respectively.
Copy link
Collaborator

Choose a reason for hiding this comment

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

As this mapping was removed, I believe we are missing the functional specification for how the pending bits get asserted etc.

Copy link
Author

Choose a reason for hiding this comment

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

Noted... I think we should specify that directly or perhaps by analogy with APLIC.

Copy link
Author

Choose a reason for hiding this comment

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

AIA section 4.7 Precise effects on interrupt-pending bits describes pending bit behaviour for all of the APLIC source configurations. This can be used as the basis of an interrupt-pending behaviour description for ALIC.

Copy link
Collaborator

@christian-herber-nxp christian-herber-nxp left a comment

Choose a reason for hiding this comment

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

I am in principle fine with these changes.
I do think eidelivery should stay as it was.
While technically this allows dynamic selection between ACLIC and IMSIC,

  1. I do not see a technical problem with that
  2. The register is WARL, so ACLIC systems will likely have ACLIC as the only legal option.

My other concern is that there might now be gaps in the specification, as you pulled in register definitions from APLIC, but it is not clear to me that the behavior of APLIC is also required.

Co-authored-by: Christian Herber <[email protected]>
Signed-off-by: mark-honman <[email protected]>
@mark-honman
Copy link
Author

I am in principle fine with these changes. I do think eidelivery should stay as it was. While technically this allows dynamic selection between ACLIC and IMSIC,

It seems to me that dynamic selection between ACLIC and IMSIC is not possible due to their interrupt files occupying overlapping CSRind space. The solution is probably an extension that adds MSI-receiving capability to ACLIC (initial suggestion is in issue #616).

My other concern is that there might now be gaps in the specification, as you pulled in register definitions from APLIC, but it is not clear to me that the behavior of APLIC is also required.

I'll have a look into how AIA describes eip and eie and the APLIC pending and enables, try to find some common ground.

@mark-honman
Copy link
Author

I'll add an explicit description of pending bit behaviour based on AIA section 4.7; it is mostly a case of changing register names and removing APLIC delivery modes that are not relevant.

there is one big question left to resolve: ACLIC + IMSIC configurations.

What's clear is that due to AIA CSRind space being a fixed window, if ACLIC and IMSIC were both present they would need to share the AIA-defined ei-prefixed registers.

But the way I read the AIA spec, the interrupt files are described as internal to the IMSIC and I guess this is how people will implement it.

IMO we should keep the original AIA eidelivery definition until we have a specification of how ACLIC and IMSIC could possibly be attached to the same hart.
Explain how xireg4-xireg6 accesses are handled in the ACLIC CSRind address space.
The number of interrupt identities supported by an ACLIC interrupt file is one less than a multiple of 64.
An ACLIC implementation can implement fewer than 63 interrupt identities.
In this case the {eip} and {eie} bits and source and priority configuration for the unimplemented
identities are WARL 0.
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a difference between WARL 0 and roz?

Copy link
Author

Choose a reason for hiding this comment

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

To be honest, I don't know... if roz does not cause an exception when csrw attempts to modify those bits, then they would be the same thing and roz is better way to express the behaviour.

Copy link
Collaborator

@christian-herber-nxp christian-herber-nxp left a comment

Choose a reason for hiding this comment

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

What is written looks fine to me.
I would like to give it a deeper review front to back to make sure nothing is missing.

Copy link
Collaborator

@christian-herber-nxp christian-herber-nxp left a comment

Choose a reason for hiding this comment

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

Overall, there are plenty of formatting issues, mostly around the notes.
You need to review the pdf and fix those.

Content-wise, this seems incomplete. The Smaclic chapter is now mostly a chapter describing the existence of some registers and its access, but not what they do and how interrupts work.
It would be a lot of work to add all of this. I am not sure this will be an upgrade over the current state of the specification.

Finally, separate the PR where possible. I believe the removal of Smaiae is independent of the rest of these changes and should be its own PR (which we want to merge asap).

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.

2 participants