Re: [Qemu-devel] [PATCH v5 15/17] ppc/pnv: Add cut down PSI bridge model and hookup external interrupt

[David Gibson]

On Tue, Oct 25, 2016 at 09:58:10AM +0200, Cédric Le Goater wrote:
> On 10/25/2016 07:30 AM, David Gibson wrote:
> > On Sat, Oct 22, 2016 at 11:46:48AM +0200, Cédric Le Goater wrote:
> >> From: Benjamin Herrenschmidt <address@hidden>
> >>
> >> The PSI (Processor Service Interface) is one of the engines of the
> >> "Bridge" unit which connects the different interfaces to the Power
> >> Processor.
> >>
> >> This adds just enough of the PSI bridge to handle various on-chip and
> >> the one external interrupt. The rest of PSI has to do with the link to
> >> the IBM FSP service processor which we don't plan to emulate (not used
> >> on OpenPower machines).
> >>
> >> Signed-off-by: Benjamin Herrenschmidt <address@hidden>
> >> [clg: - updated for qemu-2.7
> >>       - changed the XSCOM interface to fit new model
> >>       - QOMified the model
> >>       - reworked set_xive ]
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>
> >>  When skiboot initializes PSIHB, it fills the xives with server=0,
> >>  prio=0xff, which is fine, but for some reason the last two xive
> >>  settings reach the qemu MMIO region with a bogus value :
> >>  
> >>    pnv_psi_mmio_write: MMIO write 0x30 val 0x000000ff00000000
> >>    pnv_psi_mmio_write: MMIO write 0x60 val 0x000000ff20000000
> >>    pnv_psi_mmio_write: MMIO write 0x68 val 0x000000ff40000000
> >>    pnv_psi_mmio_write: MMIO write 0x70 val 0x000000ff60000000
> >>    pnv_psi_mmio_write: MMIO write 0x78 val 0xffffffff80000000
> >>    pnv_psi_mmio_write: MMIO write 0x80 val 0xffffffffa0000000
> >>
> >>  It looks like a badly initialized temp variable in the call
> >>  stack. The memory regions look fine, maybe in stdcix ? For the
> >>  moment, I have added a logging error to catch non zero values as the
> >>  guest should not do that in any case.
> > 
> > Just to clarify, I think you're saying that you believe this to be a
> > skiboot (guest side) bug rather than a qemu bug.  Is that right?
> 
> Yes. I just found why. The P8_IRQ_PSI_* values in skiboot need to
> be unsigned because they are shifted left of 29 bits :
> 
>       ...
>       #define P8_IRQ_PSI_LOCAL_ERR            4
>       #define P8_IRQ_PSI_EXTERNAL             5       /* Used for UART */
>       ...
>       out_be64(psi->regs + PSIHB_XIVR_LOCAL_ERR,
>                (0xffull << 32) | (P8_IRQ_PSI_LOCAL_ERR << 29));
>       out_be64(psi->regs + PSIHB_XIVR_HOST_ERR,
>                (0xffull << 32) | (P8_IRQ_PSI_EXTERNAL << 29));
> 
> 
> I will send a skiboot patch but we need to keep the code as it is. 

Sure.  Seems like the hardware ignores those high bits, so we probably
should too.

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

signature.asc
Description: PGP signature