Re: [PATCH] target: ppc: Correctly initialize HILE in HID-0 for book3s processors

[Narayana Murty N <nnmlinux@linux.vnet.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 3640 bytes --]

On 4/28/23 20:00, Fabiano Rosas wrote:
> Vaibhav Jain<vaibhav@linux.ibm.com>  writes:
>
>> Hi Fabiano,
>>
>> Thanks for looking into this patch and apologies for the delayed reponse.
>> Fabiano Rosas<farosas@suse.de>  writes:
>>
>>> Narayana Murty N<nnmlinux@linux.ibm.com>  writes:
>>>
>>>> On PPC64 the HILE(Hypervisor Interrupt Little Endian) bit in HID-0
>>>> register needs to be initialized as per isa 3.0b[1] section
>>>> 2.10. This bit gets copied to the MSR_LE when handling interrupts that
>>>> are handled in HV mode to establish the Endianess mode of the interrupt
>>>> handler.
>>>>
>>>> Qemu's ppc_interrupts_little_endian() depends on HILE to determine Host
>>>> endianness which is then used to determine the endianess of the guest dump.
>>>>
>>> Not quite. We use the interrupt endianness as a proxy to guest
>>> endianness to avoid reading MSR_LE at an inopportune moment when the
>>> guest is switching endianness.
>> Agreed

Agreed

>>
>>> This is not dependent on host
>>> endianness. The HILE check is used when taking a memory dump of a
>>> HV-capable machine such as the emulated powernv.
>> I think one concern which the patch tries to address is the guest memorydump file
>> generated of a BigEndian(BE) guest on a LittleEndian(LE) host is not readable on
>> the same LE host since 'crash' doesnt support cross endianess
>> dumps. Also even for a LE guest on LE host the memory dumps are marked as BE
>> making it not possible to analyze any guest memory dumps on the host.
>>
>  From QEMU's perspective there's no "host" in this equation. We'll
> generate a BE dump for a BE guest and a LE dump for a LE guest. Anything
> different is a bug in QEMU (as the one this patch addresses).
>
>> However setting the HILE based on host endianess of qemu might not be
>> the right way to fix this problem. Based on an off mailing list discussion
>> with Narayana, he is working on another patch which doesnt set HILE
>> based on host endianess. However the problem seems to be stemming from
>> fact that qemu on KVM is using the HILE to set up the endianess of
>> memory-dump elf and since its not setup correctly the memory dumps are
>> in wrong endianess.
>>
>>> I think the actual issue might be that we're calling
>>> ppc_interrupts_little_endian with hv=true for the dump.
>>>
>> Yes, that is currently the case with cpu_get_dump_info(). Excerpt from
>> that function below that sets the endianess of the dump:
>>
>>      if (ppc_interrupts_little_endian(cpu, cpu->env.has_hv_mode)) {
> This should probably be looking at cpu->vhyp or MSR_HVB since
> has_hv_mode will not change after we init the cpu.

yes, I agree. New version patch is under testing once done,
will post the patch.

>>          info->d_endian = ELFDATA2LSB;
>>      } else {
>>          info->d_endian = ELFDATA2MSB;
>>      }
>>
>> for pseries kvm guest cpu->env.has_hv_mode is already set hence
>> ppc_interrupts_little_endian() assumes its running in 'hv' mode. The new
>> patch from Narayana will be addressing this.
>>
>>>> Currently the HILE bit is never set in the HID0 register even if the
>>>> qemu is running in Little-Endian mode. This causes the guest dumps to be
>>>> always taken in Big-Endian byte ordering. A guest memory dump of a
>>>> Little-Endian guest running on Little-Endian qemu guest fails with the
>>>> crash tool as illustrated below:
>>>>
>>> Could you describe in more detail what is your setup? Specifically
>>> whether both guests are running TCG or KVM (info kvm) and the state of
>>> the nested-hv capability in QEMU command line.
>> Currently the issue is seen with any pseries KVM guest running on a PowerNV host.

[-- Attachment #2: Type: text/html, Size: 5746 bytes --]