Hi Vaibhav, Thanks for taking a look at this patch. My response is inline:
On 2026/06/03 12:27 PM, Vaibhav Jain wrote: > Hi Amit, > > Thanks for the patch. My review comments inline below: > > Amit Machhiwal <[email protected]> writes: > > > During H_CLIENT_ARCHITECTURE_SUPPORT, QEMU attempts to set the requested > > CPU compatibility mode via ppc_set_compat_all(). If this fails, the > > existing code may fall back to raw mode when supported by the guest. > > > > The current logic treats all failures uniformly and tried to fall back > > to raw mode whenever possible. As a result, errors from KVM (e.g. > > kvmppc_set_compat() returning -EINVAL) are silently masked, making > > debugging difficult and allowing the guest to proceed with an > > unsupported or incompatible configuration. > > > > When running with KVM, such failures indicate that the requested > > compatibility level is not supported by the host. Do not fallback in > > this case; instead, report the error and fail the CAS negotiation. > > > > This makes the failure visible to the user and ensures that invalid > > compatibility requests are rejected early rather than being hidden by > > fallback behavior. For example, the following error is now reported: > > > > qemu-system-ppc64: Unable to set CPU compatibility mode in KVM: Invalid > > argument > > > > Fixes: cc7b35b169e9 ("spapr: fallback to raw mode if best compat mode > > cannot be set during CAS") > > Signed-off-by: Amit Machhiwal <[email protected]> > > Tested-by: Anushree Mathur <[email protected]> > > --- > > hw/ppc/spapr_hcall.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > > index 032805a8d0db..de2ea6f5e579 100644 > > --- a/hw/ppc/spapr_hcall.c > > +++ b/hw/ppc/spapr_hcall.c > > @@ -1204,6 +1204,15 @@ target_ulong > > do_client_architecture_support(PowerPCCPU *cpu, > > Error *local_err = NULL; > > > > if (ppc_set_compat_all(cas_pvr, &local_err) < 0) { > > + /* > > + * If KVM rejected the compat mode, do not fallback. This > > indicates > > + * the host cannot support the requested level. > > + */ > > + if (kvm_enabled()) { > > + error_report_err(local_err); > > + return H_HARDWARE; > > + } > > + > > NAK on this change. > > As per PAPR whether the guest can run in RAW mode needs to be negotiated > between Guest and VMM. The existing code path via cas_check_pvr() and > 'raw_mode_supported' already handles this case. > > So I dont see a reason for this KVM specific changes needed. Can you > please elaborate a bit more on the issue that gets fixed by this change. I have mentioned the reason in the commit message. I'm quoting the part of it below: The current logic treats all failures uniformly and tried to fall back to raw mode whenever possible. As a result, errors from KVM (e.g. kvmppc_set_compat() returning -EINVAL) are silently masked, making debugging difficult and allowing the guest to proceed with an unsupported or incompatible configuration. The currently call chain is as below: do_client_architecture_support() -> ppc_set_compat_all() -> run_on_cpu() -> do_run_on_cpu() -> do_set_compat() -> ppc_set_compat() -> kvmppc_set_compat() // returns error Now when an error is returned via kvmppc_set_comapt(), its just ignored in the current situation silently in do_client_architecture_support(), while the caller is supposed to handle the error returned in an errp object. This would make the guest try to continue with a raw mode when the host was actually booted in a compatibility mode, while it shouldn't be doing that. Please let me know your further views on this. Thanks, Amit > > If this change is related to your Linux kernel patch series titled > "KVM: PPC: Handle CPU compatibility mode for nested guests" then the > ideal place to put compat check would be in ppc_check_compat() and not in > do_client_architecture_support() > > > /* We fail to set compat mode (likely because running with KVM > > PR), > > * but maybe we can fallback to raw mode if the guest supports > > it. > > */ > > -- > > 2.50.1 (Apple Git-155) > > > > -- > Cheers > ~ Vaibhav >
