On Tue May 30, 2023 at 12:05 AM AEST, Fabiano Rosas wrote: > "Nicholas Piggin" <npig...@gmail.com> writes: > > > On Tue May 23, 2023 at 2:02 AM AEST, Narayana Murty N wrote: > >> Changes since V2: > >> commit message modified as per feedbak from Nicholas Piggin. > >> Changes since V1: > >> https://lore.kernel.org/qemu-devel/20230420145055.10196-1-nnmli...@linux.ibm.com/ > >> The approach to solve the issue was changed based on feedback from > >> Fabiano Rosas on patch V1. > >> --- > >> target/ppc/arch_dump.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/target/ppc/arch_dump.c b/target/ppc/arch_dump.c > >> index f58e6359d5..a8315659d9 100644 > >> --- a/target/ppc/arch_dump.c > >> +++ b/target/ppc/arch_dump.c > >> @@ -237,7 +237,7 @@ int cpu_get_dump_info(ArchDumpInfo *info, > >> info->d_machine = PPC_ELF_MACHINE; > >> info->d_class = ELFCLASS; > >> > >> - if (ppc_interrupts_little_endian(cpu, cpu->env.has_hv_mode)) { > >> + if (ppc_interrupts_little_endian(cpu, !!(cpu->env.msr_mask & > >> MSR_HVB))) { > >> info->d_endian = ELFDATA2LSB; > >> } else { > >> info->d_endian = ELFDATA2MSB; > > > > Oh, and now I see it cpu_get_dump_info just picks the first CPU to test > > this! So a test that can change at runtime is surely not the right one. > > If you use MSR[HV] then if you have a SMP machine that is doing a bunch > > of things and you want to dump to debug the system, this will just > > randomly give you a wrong-endian dump if CPU0 just happened to be > > running some KVM guest. > > > > Not sure if you are just thinking out loud about MSR_HV or if you > mistook MSR_HVB for MSR_HV. But just in case:
Oh, yes I did confuse the MSR from the mask. Apologies, that makes much of my ranting invalid. > The env->msr_mask is what tells us what MSR bits are supported for this > CPU, i.e. what features it contains. So MSR_HVB is not equivalent to > MSR[HV], but merely informs that this CPU knows about MSR_HV. We then > store that information at has_hv_mode. The MSR_HVB bit changes only > once (at cpu_ppc_set_vhyp), after we decide whether to use vhyp. So: > > env->has_hv_mode == cpu supports HV mode; > > MSR_HVB=1 == cpu supports HV mode AND we allow the OS to run with MSR_HV=1; > > MSR_HVB=0 == cpu doesn't support HV mode OR > cpu supports HV mode, but we don't allow the OS to run with > MSR_HV=1 because QEMU is the HV (i.e. vhyp); > > For the arch_dump code, passing (msr_mask & MSR_HVB) ends up meaning: > "can this OS ever run with MSR_HV=1?", which for emulated powernv would > be Yes and for pseries (incl. nested) would be No. So for emulated > powernv we always look at the emulated HILE and for pseries we always > look at LPCR_ILE. Well then I agree with that, I think the talk of the KVM guest confused me. So in that case I agree with the patch. It does seem like there would be still a problem with a nested KVM guest on pseries though, since it hijacks LPCR among other SPRs, and may modify ILE. It seems like you would need a way to ask vhyp for the host's interrupt endian mode (and would get that from SpaprCpuState's nested_host_state regs. But that could be fixed later. Thanks, Nick