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 vCLIC support #55

Open
wants to merge 2 commits into
base: pulp-v2
Choose a base branch
from
Open

Add vCLIC support #55

wants to merge 2 commits into from

Conversation

ezelioli
Copy link

@ezelioli ezelioli commented Jul 3, 2024

This PR adds support for the custom vCLIC extension.

It is a replacement of the previous PR, with only changes strictly relevant to vCLIC. It also makes the extension parametrized on an additional RVVCLIC parameter, which depends on RVH and RVSCLIC parameters and can be used to enable / disable the virtualization extension support.

The main changes are:

  1. CSRs: Implemented HGEIE write logic and functionality. Added vCLIC-specific VSTVT and VSINTTHRESH registers.
  2. CLIC interface: Adapted core interface and CLIC controller logic to vCLIC.

@ezelioli ezelioli requested a review from niwis as a code owner July 3, 2024 08:27
@niwis niwis added the pulp-v2 label Nov 8, 2024
@niwis niwis changed the base branch from pulp-v1 to pulp-v2 February 24, 2025 03:49
@niwis
Copy link
Collaborator

niwis commented Feb 24, 2025

@ezelioli can you rebase onto pulp-v2?

@ezelioli ezelioli force-pushed the ez/vclic branch 2 times, most recently from 5e37e89 to ff6255a Compare March 19, 2025 13:08
@ezelioli
Copy link
Author

@niwis currently the cheshire-integration check fails because the configuration of Cheshire pointed by the CI does not enable the CLIC but uses a CVA6 configuration which enables RVSCLIC and RVVCLIC (only RVSCLIC is overwritten by Cheshire leading to an invalid config with RVVCLIC enabled when CLIC is disabled).

Possible workarounds that I see are:

  1. Re-point the CI job to a version of Cheshire which sets both the RVSCLIC and RVVCLIC parameters (probably the right way to do it). In this case, should I have the CLIC / vCLIC enabled or disabled for this CI check ?
  2. Change the defaul config package used by Cheshire to disable vCLIC feature.

Any hints on how to proceed?

@ezelioli
Copy link
Author

@niwis currently the cheshire-integration check fails because the configuration of Cheshire pointed by the CI does not enable the CLIC but uses a CVA6 configuration which enables RVSCLIC and RVVCLIC (only RVSCLIC is overwritten by Cheshire leading to an invalid config with RVVCLIC enabled when CLIC is disabled).

Possible workarounds that I see are:

1. Re-point the CI job to a version of Cheshire which sets both the RVSCLIC and RVVCLIC parameters (probably the right way to do it). In this case, should I have the CLIC / vCLIC enabled or disabled for this CI check ?

2. Change the defaul config package used by Cheshire to disable vCLIC feature.

Any hints on how to proceed?

assertion_error
vclic_cheshire_regression

@ezelioli
Copy link
Author

@niwis currently the cheshire-integration check fails because the configuration of Cheshire pointed by the CI does not enable the CLIC but uses a CVA6 configuration which enables RVSCLIC and RVVCLIC (only RVSCLIC is overwritten by Cheshire leading to an invalid config with RVVCLIC enabled when CLIC is disabled).

Possible workarounds that I see are:

  1. Re-point the CI job to a version of Cheshire which sets both the RVSCLIC and RVVCLIC parameters (probably the right way to do it). In this case, should I have the CLIC / vCLIC enabled or disabled for this CI check ?
  2. Change the defaul config package used by Cheshire to disable vCLIC feature.

Any hints on how to proceed?

I followed approach 1. and the cheshire-integration for this branch (push) is now passing (the CLIC/vCLIC parameters are disabled on the cva6/vclic branch of Cheshire).
The pull_request cheshire-integration job seems to fail due to Bender not finding the CVA6 hash. Do you have any hints on what I could have misconfigured here?

@niwis
Copy link
Collaborator

niwis commented Mar 24, 2025

Thank you @ezelioli! I agree with approach 1. Let's stick to the same approach as for the CLIC: disable it per default and only enable it for the tests that actually need it (which is currently none?)

Don't worry about the failing pull_request run; it's not your fault. This was fixed in pulp-platform/bender#181 and should be part of the next bender release.

One more point: my understanding is that upstream Cheshire tries to only reference released versions of dependencies. Should vCLIC be integrated into CLIC before updating Cheshire? (to be confirmed with the Cheshire/CLIC maintainers)

I'll do a full review pass asap.

@ezelioli
Copy link
Author

Thank you @ezelioli! I agree with approach 1. Let's stick to the same approach as for the CLIC: disable it per default and only enable it for the tests that actually need it (which is currently none?)

Don't worry about the failing pull_request run; it's not your fault. This was fixed in pulp-platform/bender#181 and should be part of the next bender release.

One more point: my understanding is that upstream Cheshire tries to only reference released versions of dependencies. Should vCLIC be integrated into CLIC before updating Cheshire? (to be confirmed with the Cheshire/CLIC maintainers)

I'll do a full review pass asap.

Thanks for the updates!
I will double check with the maintainers about the versioning in the meantime.

@ezelioli
Copy link
Author

Thank you @ezelioli! I agree with approach 1. Let's stick to the same approach as for the CLIC: disable it per default and only enable it for the tests that actually need it (which is currently none?)

Don't worry about the failing pull_request run; it's not your fault. This was fixed in pulp-platform/bender#181 and should be part of the next bender release.

One more point: my understanding is that upstream Cheshire tries to only reference released versions of dependencies. Should vCLIC be integrated into CLIC before updating Cheshire? (to be confirmed with the Cheshire/CLIC maintainers)

I'll do a full review pass asap.

Yes there is no test currently using vCLIC. I can add a vCLIC test and respective config to Cheshire and update this PR to point there.

@niwis
Copy link
Collaborator

niwis commented Mar 24, 2025

Yes there is no test currently using vCLIC. I can add a vCLIC test and respective config to Cheshire and update this PR to point there.

I think that would be useful to avoid regressions with vCLIC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants