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 support for retrieving boot cpu id and mpidr from proxy #369

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dberlin
Copy link
Contributor

@dberlin dberlin commented Dec 3, 2023

Uses it to add basic support for non-0,0,0 boot cpu in python hypervisor code

Signed-off-by: Daniel Berlin [email protected]

Support non-0,0,0 boot cpu in python hypervisor code

Signed-off-by: Daniel Berlin <[email protected]>
boot_cpu_name = "cpu" + str(self.p.get_boot_cpu_idx())
boot_cpu_adt = self.u.adt["/cpus/" + boot_cpu_name]
boot_cpu_mpidr = self.p.get_boot_cpu_mpidr()
self.started_cpus[0] = (boot_cpu_adt.die_id, boot_cpu_adt.cluster_id, boot_cpu_mpidr & 0xf)
Copy link
Member

Choose a reason for hiding this comment

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

t8122 doesn't have die-id, please use getattr(boot_cpu_adt, "die_id", 0) instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

for cpu in self.adt["cpus"][1:]:
boot_cpu_name = "cpu" + str(self.p.get_boot_cpu_idx())
for cpu in self.adt["cpus"]:
if cpu.name == boot_cpu_name:
Copy link
Member

Choose a reason for hiding this comment

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

since we already have the CPU core's ADT node wouldn't it make more sense to test cpu.state == "running" directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, fair enough, i'l fix

@@ -54,6 +54,12 @@ int proxy_process(ProxyRequest *request, ProxyReply *reply)
case P_GET_BOOTARGS:
reply->retval = boot_args_addr;
break;
case P_GET_BOOT_CPU_IDX:
reply->retval = boot_cpu_idx;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a good idea to retrieve something which is only valid after smp_start_secondaries is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts on how to approach this?
This was all sort of a hack to get it working.
I could make smp_start_secondaries return this value as an out arg or something?
Or some totally different approach?

Copy link
Member

@jannau jannau Jan 28, 2024

Choose a reason for hiding this comment

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

I would use the state property from the ADT. In two cases it is iterating over all CPUs and skipping the boot CPU. The remaining use can be replaced with [cpu for cpu in self.u.adt["cpus"] if cpu.state == "running"][0].

reply->retval = boot_cpu_idx;
break;
case P_GET_BOOT_CPU_MPIDR:
reply->retval = boot_cpu_mpidr;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think anyone has plans to use the non boot CPU for proxy commands so this is equivalent to u.mrs(MPIDR_EL1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, i'l remove

for cpu in list(self.adt["cpus"]):
if cpu.name != "cpu0":
if cpu.name != boot_cpu_name:
Copy link
Member

Choose a reason for hiding this comment

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

simpler change without additional proxy commands would use the CPU's ADT node "state" property

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep.

@marcan marcan force-pushed the main branch 3 times, most recently from 4f95305 to 95d67cf Compare April 9, 2024 03:58
@marcan
Copy link
Member

marcan commented Oct 30, 2024

Review comments never got addressed for this one, right? @dberlin do you want me to fix it up myself if you're busy?

@jannau
Copy link
Member

jannau commented Oct 30, 2024

I have the alternate implementation in https://github.com/jannau/m1n1/commits/t8122/ and will create a PR tonight

@mildsunrise
Copy link

this can be closed now, right?

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.

4 participants