Bhyve treats an empty cpu set as an error which triggers MPASS panic in the guest. As per the SBI spec, an empty cpu mask returns SBI_SUCCESS, and a hart mask base of -1UL broadcasts to all harts.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 78656 Build 75539: arc lint + arc unit
Event Timeline
Comment Actions
Looks good and works great, only a minor issue
| sys/riscv/vmm/vmm_sbi.c | ||
|---|---|---|
| 99 | could we use ffs() here and below? | |
Comment Actions
another nit
| sys/riscv/vmm/vmm_sbi.c | ||
|---|---|---|
| 179 | need to make this line shorter for 1 character. I usually remove one whitespace-character after comma, but it is up to you | |
Comment Actions
This resolves a bug in the SBI IPI emulation when booting a guest OS in bhyve. The guest could request hart_mask == 0, in that case we indeed should return SBI_SUCCESS and treat the call as a no-op, as long as hart_mask_base != -1.
Thanks!
Comment Actions
Few optimizations
- use of ffsl() instead of a for each bit loop
- in case when hart_mask = -1, use CPU_COPY to copy active_cpus instead of copying bit by bit
- 80 column fix
Comment Actions
Looks good and tested with bhyve on SpacemiT k3 (using generic guest image).
SBI IPI works fine. Thanks!
| sys/riscv/vmm/vmm_sbi.c | ||
|---|---|---|
| 165 | from style.9: | |
| sys/riscv/riscv/sbi_ipi.c | ||
|---|---|---|
| 77 | Please don't conflate supervisor and hypervisor changes | |
| sys/riscv/vmm/vmm_riscv.c | ||
| 551 | Surely this can't happen? The caller ensures only active CPUs are allowed, and I think we should maintain the internal contract that calling riscv_send_ipi with garbage in cpus, including mentioning ones that don't exist for the given struct hyp, is illegal and not something to guard against. | |
Comment Actions
Took care of Jesse's comments:
- Remove hypervisor context's null check
- Removed the mask check in ipi send to separate host code from vmm code
Comment Actions
Can you please (a) avoid unnecessary churn (b) separate out and properly justify each logical change? The a1 addition in vmm_sbi_ecall seems entirely unrelated, but I also don't understand why it's needed, so if you believe it is, put it in its own revision with a proper commit message.
Also, the message for this patch is now outdated.
Finally, that's not, never has been, and never will be, my name.
Comment Actions
(b) is required as per specification. SBI call returns sbiret.{error, value}. Error is returned in a0 and value in a1. As per SBI specification, if a function has no return value, a1 must be 0. This path is being SBI spec compliant. Does it hurt here? or is this change big enough to stand alone in a separate patch?
For (a) could you be more specific? Which part is unnecessary churn?
Also, the message for this patch is now outdated.
I don't know what it is.
Finally, that's not, never has been, and never will be, my name.
I apologise for calling your name out wrong. Had no intention to hurt you.
Comment Actions
"separate out and properly justify". If you think it's needed, open a new review for it explaining why. But please don't conflate multiple independent changes in a single review.
For (a) could you be more specific? Which part is unnecessary churn?
https://reviews.freebsd.org/D61126#1401860
Also, the message for this patch is now outdated.
I don't know what it is.
The revision summary at the top of the page still talks about the NULL check and the guest change.
| sys/riscv/vmm/vmm_sbi.c | ||
|---|---|---|
| 108 | ||
| sys/riscv/vmm/vmm_sbi.c | ||
|---|---|---|
| 106 | This change will also be required. | |
| sys/riscv/vmm/vmm_sbi.c | ||
|---|---|---|
| 86 | I would prefer for the *bug fix* to be as close to the original code as possible. You can also follow up with a performance optimisation if you want to, which can adopt the smarter ffs approach, but that should be a distinct non-functional change commit, rather than mixing everything up and making it harder to understand (and harder to deal with if there are regressions). | |
| sys/riscv/vmm/vmm_sbi.c | ||
|---|---|---|
| 86 | Noted. Good call. | |
Comment Actions
bhyve/riscv: correctly handle empty masks and broadcast in SBI IPI/RFNC
Bhyve treats an empty cpu set as an error which triggers MPASS panic in
the guest. As per the SBI spec, an empty cpu mask returns SBI_SUCCESS,
and a hart mask base of -1UL broadcasts to all harts.