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

Add RPMI MPXY Integration chapter #49

Merged
merged 7 commits into from
Sep 7, 2024

Conversation

pathakraul
Copy link
Collaborator

No description provided.

src/intro.adoc Outdated

[#img-transport-topologies]
.Transport for M-Mode and S-Mode
image::transport-topologies.png[width=800,height=800, align="center"]

RPMI is designed to work with a single or multi-tenant topology as shown
above.
==== RPMI Instance
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to "RPMI client"?
some chapter uses "client".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, client is always meant as a driver or SW which sends the RPMI message or data.
RPMI client uses an RPMI instance to do that.

If somewhere client is misinterpreted then we need to correct that

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, i saw some use 'client' in other chapters.

@@ -197,10 +200,10 @@ the `MAJOR` and `MINOR` numbers.
! Description

! [31:16]
! `MAJOR` number
! `MAJOR` Number
Copy link
Collaborator

Choose a reason for hiding this comment

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

need capital letter for ‘N'?

! [31:2]
! _Reserved_ and must be `0`.
! [31:3]
! _Reserved, must be initialized to_ `0`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

the whole sentence needs to be italic xxx font?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thats how i have done for all groups which i have updated so far

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But now i see its not same everywhere

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 will make only RESERVED as italic so we have less work to do

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, Joshua previous patch make them all to "Reserved and must be 0."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

:(

Copy link
Collaborator Author

@pathakraul pathakraul Sep 6, 2024

Choose a reason for hiding this comment

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

Thats why i was wondering why suddenly this changed since i did the older way
I will let you and Joshua do these kind of formatting now :) Will save everyones time

@@ -502,17 +524,17 @@ processor will take the wired interrupt.
| 0
| MSI_ADDRESS_LOW
| uint32
| Lower 32-bit of the MSI address.
| Lower 32-bit of the MSI address
Copy link
Collaborator

Choose a reason for hiding this comment

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

your patch revert changes from Joshua?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, sorry, i had to merge manually while rebasing and missed couple of changes

! _Reserved, must be initialized to_ `0`.

! [2]
! The RISC-V privilege level to which the RPMI instance belong.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Then Firmware needs to know this and return the correct mode?
why we need this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now combining this information with the MPXY chapter requirement. The M-Mode RPMI instance will know exactly which service groups will belong to MPXY and which will not

Copy link
Collaborator

Choose a reason for hiding this comment

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

but this information need retrieve from platform microcontroller?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because based on the mailbox we will create platform microcontroller will know which mailbox is meant for M-Mode and which one for S-Mode. Thats how a transport itself establishes as Secure or non secure

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PuC and DT details for mailbox have to be in sync

Copy link
Collaborator

Choose a reason for hiding this comment

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

but this also means needs to be fixed, can't have one PuC FW to support different use cases. Need to update PuC FW.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will be fixed only, its really a static information since you will be fixing the mailbox(shared memory) and assigning it for an RPMI instance and then providing it to DT. PuC firmware already aware of this and its a static detail. This could have been a part of DT node also but you need additional or new DT properties to encode this

:rootpath: ./../
endif::rootpath[]

== RPMI Integration with SBI MPXY extension.
Copy link
Collaborator

@lftan lftan Sep 6, 2024

Choose a reason for hiding this comment

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

remove full stop in header.

extension change to "Extension"

exposed to the S-mode over SBI MPXY interface and RPMI message protocol specific
attributes for MPXY channel.

The service groups that have dedicated SBI extension cannot be
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the previous meeting, there were people commenting about this, right? One example is CPPC, people might want to use S-mode RPMI with fast-channel?

Page 10, Recommendation for "Exclusive Interface Choice with MPXY or S-mode RPMI for a Specific RPMI Service Group"
https://docs.google.com/presentation/d/1EsrcgUZiXf_mqt4Wcd0oiRyLw4ZIGaVMa6Ew0GZ7hoQ/edit#slide=id.g2cd11e3ec8b_9_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.

I have already mentioned that this case is when RPMI instance is in M-Mode. S Mode RPMI instance can implement CPPC at their wish.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have already mentioned that this case is when RPMI instance is in M-Mode. S Mode RPMI instance can implement CPPC at their wish.

got this part?


The following table lists the service group
In each service group the `SERVICE_ID = 0x00` is dedicated to a notification type
service used to send events notification and a `SERVICE_ID = 0x1` dedicated for
Copy link
Collaborator

Choose a reason for hiding this comment

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

change to 0x01

@@ -27,7 +27,8 @@ necessary to categorize it as a graceful or forceful shutdown. In the case of
a shutdown request, it is implicit for the platform microcontroller that the
application processor has prepared itself for a successful shutdown.

Below table lists the services in this group:
Below table lists the services in the SYSTEM_RESET service group:
Copy link
Collaborator

Choose a reason for hiding this comment

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

"The following table lists the services in the SYSTEM_RESET service group:"

All service groups can follow the same.

@@ -18,7 +18,8 @@ platform microcontroller that all the application processors except the one
requesting are in `STOPPED` state and necessary state saving in the RAM has
been complete.

Below if the list of services in this group:
Below if the list of services in the SYSTEM_SUSPEND service group:
Copy link
Collaborator

Choose a reason for hiding this comment

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

grammar issue

@pathakraul pathakraul force-pushed the rpathak_mpxychapter branch 4 times, most recently from 323f707 to d13b8a5 Compare September 6, 2024 07:35

| 0x0001
|
| BASE
| M-mode, S-mode
Copy link
Collaborator

@lftan lftan Sep 7, 2024

Choose a reason for hiding this comment

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

In the get privilege level service, it only has M-mode and Non M-mode.
Should we change S-mode here to S-Mode and U-Mode? Or no limit to any privilege level

Copy link
Collaborator Author

@pathakraul pathakraul Sep 7, 2024

Choose a reason for hiding this comment

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

No, We are fixing the Base service group attribute to convey whats possible, and by Non-M-Mode we can encompass what may be possible in future.

This table is explicit about whats "currently" possible. And this table is also in constant update if anything changes in future

image::mpxy-rpmi.png[350,350, align="center"]

RPMI service groups that are M-mode only, including BASE, which is an exception
must not be accessible via SBI MPXY interface. A MPXY channel
Copy link
Collaborator

Choose a reason for hiding this comment

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

This mean BASE service group can't use SBI MPXY interface?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we want allow BASE via MPXY after that

Copy link
Collaborator

Choose a reason for hiding this comment

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

S-mode software can get some basic information of RPMI, such as RPMI version etc..

Copy link
Collaborator Author

@pathakraul pathakraul Sep 7, 2024

Choose a reason for hiding this comment

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

Few things to consider -

RPMI Base was never meant for the MPXY. RPMI base group only and only manages a RPMI context. MPXY channel is not a RPMI context. Its not even a RPMI transport. But you are right that these details are required and thats why we have attributes in the MPXY channel.

From your comment i just added Service group version as an attribute also in the MPXY chapter. So, all these things like RPMI
version and service group version whatever is required by a MPXY client using RPMI, has to be via channel attributes, either Standard or Message Proto specific attributes.

@lftan lftan merged commit 12627a0 into riscv-non-isa:main Sep 7, 2024
1 check passed
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