Re: [PATCH qemu v20] spapr: Implement Open Firmware client interface

[BALATON Zoltan <balaton@eik.bme.hu>]

On Tue, 25 May 2021, David Gibson wrote:
> On Mon, May 24, 2021 at 02:42:30PM +0200, BALATON Zoltan wrote:
>> On Mon, 24 May 2021, David Gibson wrote:
>>> On Sun, May 23, 2021 at 07:09:26PM +0200, BALATON Zoltan wrote:
>>>> On Sun, 23 May 2021, BALATON Zoltan wrote:
>>>>> On Sun, 23 May 2021, Alexey Kardashevskiy wrote:
>>>>>> One thing to note about PCI is that normally I think the client
>>>>>> expects the firmware to do PCI probing and SLOF does it. But VOF
>>>>>> does not and Linux scans PCI bus(es) itself. Might be a problem for
>>>>>> you kernel.
>>>>>
>>>>> I'm not sure what info does MorphOS get from the device tree and what it
>>>>> probes itself but I think it may at least need device ids and info about
>>>>> the PCI bus to be able to access the config regs, after that it should
>>>>> set the devices up hopefully. I could add these from the board code to
>>>>> device tree so VOF does not need to do anything about it. However I'm
>>>>> not getting to that point yet because it crashes on something that it's
>>>>> missing and couldn't yet find out what is that.
>>>>>
>>>>> I'd like to get Linux working now as that would be enough to test this
>>>>> and then if for MorphOS we still need a ROM it's not a problem if at
>>>>> least we can boot Linux without the original firmware. But I can't make
>>>>> Linux open a serial console and I don't know what it needs for that. Do
>>>>> you happen to know? I've looked at the sources in Linux/arch/powerpc but
>>>>> not sure how it would find and open a serial port on pegasos2. It seems
>>>>> to work with the board firmware and now I can get it to boot with VOF
>>>>> but then it does not open serial so it probably needs something in the
>>>>> device tree or expects the firmware to set something up that we should
>>>>> add in pegasos2.c when using VOF.
>>>>
>>>> I've now found that Linux uses rtas methods read-pci-config and
>>>> write-pci-config for PCI access on pegasos2 so this means that we'll
>>>> probably need rtas too (I hoped we could get away without it if it were only
>>>> used for shutdown/reboot or so but seems Linux needs it for PCI as well and
>>>> does not scan the bus and won't find some devices without it).
>>>
>>> Yes, definitely sounds like you'll need an RTAS implementation.
>>>
>>>> While VOF can do rtas, this causes a problem with the hypercall method using
>>>> sc 1 that goes through vhyp but trips the assert in ppc_store_sdr1() so
>>>> cannot work after guest is past quiesce.
>>>
>>>> So the question is why is that
>>>> assert there
>>>
>>> Ah.. right.  So, vhyp was designed for the PAPR use case, where we
>>> want to model the CPU when it's in supervisor and user mode, but not
>>> when it's in hypervisor mode.  We want qemu to mimic the behaviour of
>>> the hypervisor, rather than attempting to actually execute hypervisor
>>> code in the virtual CPU.
>>>
>>> On systems that have a hypervisor mode, SDR1 is hypervisor privileged,
>>> so it makes no sense for the guest to attempt to set it.  That should
>>> be caught by the general SPR code and turned into a 0x700, hence the
>>> assert() if we somehow reach ppc_store_sdr1().
>>
>> This seems to work to avoid my problem so I can leave vhyp enabled after
>> qiuesce for now:
>>
>> diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
>> index d957d1a687..13b87b9b36 100644
>> --- a/target/ppc/cpu.c
>> +++ b/target/ppc/cpu.c
>> @@ -70,7 +70,7 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
>>  {
>>      PowerPCCPU *cpu = env_archcpu(env);
>>      qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value);
>> -    assert(!cpu->vhyp);
>> +    assert(!cpu->env.has_hv_mode || !cpu->vhyp);
>>  #if defined(TARGET_PPC64)
>>      if (mmu_is_64bit(env->mmu_model)) {
>>          target_ulong sdr_mask = SDR_64_HTABORG | SDR_64_HTABSIZE;
>>
>> But I wonder if the assert should also be moved within the TARGET_PPC64
>> block and if we may need to generate some exception here instead. Not sure
>> what a real CPU would do in this case but if accessing sdr1 is privileged in
>> HV mode then there should be an exception or if that's catched
>> elsewhere
>
> It should be caught elsehwere.  Specifically, when the SDR1 SPR is
> registered, on CPUs with a hypervisor mode it should be registered as
> hypervisor privileged, so the general mtspr dispatch logic should
> generate the exception if it's called from !HV code.  The assert here
> is just to sanity check that it has done so before we enter the actual
> softmmu code.

So what's the decision then? Remove this assert or modify it like above 
and move it to the TARGET_PPC64 block (as no 32 bit CPU should have an HV 
bit anyway).

Regards,
BALATON Zoltan