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

[BALATON Zoltan]

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