Re: [PATCH v3] target: ppc: Use MSR_HVB bit to get the target endianness for memory dump

["Nicholas Piggin" <npiggin@gmail.com>]

On Tue May 30, 2023 at 12:05 AM AEST, Fabiano Rosas wrote:
> "Nicholas Piggin" <npiggin@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-nnmlinux@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