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

[Fabiano Rosas <farosas@suse.de>]

"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:

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.

> But even ignoring all of that, let's say you have all the same endian
> host and guest kernels and dump format... If you dump host memory then
> you need host kernel and structures to debug guest kernel/image
> (assuming crash or the person debugging it is smart enough to make sense
> of it). So I don't see how you can sanely use the crash dump of host
> memory with the guest kernel. I must still be missing something.
>

You're right, the QEMU instance that receives the qmp_dump_guest_memory
command should generate a memory dump of whatever OS is running in it at
a direct level.  So the endianness reported in the dump's ELF should
match the guest OS image ELF (roughly, there's -bios which doesn't
require an ELF).