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

[David Gibson <david@gibson.dropbear.id.au>]

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

On Tue, May 25, 2021 at 11:55:43AM +0200, BALATON Zoltan wrote:
> 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).

Uh, I guess modify it with the if-hv-available thing.  Don't move it
under the ifdef, it still makes logical sense for 32-bit systems, even
though the HV available side should never trip.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]