[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.10 6/8] ppc/pnv: Add cut down PSI bridge m
|
From: |
David Gibson |
|
Subject: |
Re: [Qemu-devel] [PATCH for-2.10 6/8] ppc/pnv: Add cut down PSI bridge model and hookup external interrupt |
|
Date: |
Fri, 17 Mar 2017 13:00:35 +1100 |
|
User-agent: |
Mutt/1.8.0 (2017-02-23) |
On Thu, Mar 16, 2017 at 02:52:17PM +0100, Cédric Le Goater wrote:
> On 03/15/2017 07:16 AM, David Gibson wrote:
> > On Wed, Mar 08, 2017 at 11:52:49AM +0100, Cédric Le Goater wrote:
> >> From: Benjamin Herrenschmidt <address@hidden>
> >>
> >> The PSI (Processor Service Interface) Controller 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.9
> >> - changed the XSCOM interface to fit new model
> >> - QOMified the model
> >> - reworked set_xive and worked around a skiboot bug
> >> - removed the 'psi_mmio_to_xscom' mapping array ]
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >> hw/ppc/Makefile.objs | 2 +-
> >> hw/ppc/pnv.c | 35 ++-
> >> hw/ppc/pnv_psi.c | 583
> >> +++++++++++++++++++++++++++++++++++++++++++++
> >> include/hw/ppc/pnv.h | 8 +
> >> include/hw/ppc/pnv_psi.h | 61 +++++
> >> include/hw/ppc/pnv_xscom.h | 3 +
> >> 6 files changed, 685 insertions(+), 7 deletions(-)
> >> create mode 100644 hw/ppc/pnv_psi.c
> >> create mode 100644 include/hw/ppc/pnv_psi.h
> >>
> >> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> >> index 001293423c8d..dc19ee17fa57 100644
> >> --- a/hw/ppc/Makefile.objs
> >> +++ b/hw/ppc/Makefile.objs
> >> @@ -6,7 +6,7 @@ obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o
> >> spapr_rtas.o
> >> obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> >> obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o
> >> # IBM PowerNV
> >> -obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o
> >> +obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o
> >> ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> >> obj-y += spapr_pci_vfio.o
> >> endif
> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> >> index 0ae11cc3a2ca..85b00bf235c6 100644
> >> --- a/hw/ppc/pnv.c
> >> +++ b/hw/ppc/pnv.c
> >> @@ -356,15 +356,22 @@ static void ppc_powernv_reset(void)
> >> * have a CPLD that will collect the SerIRQ and shoot them as a
> >> * single level interrupt to the P8 chip. So let's setup a hook
> >> * for doing just that.
> >> - *
> >> - * Note: The actual interrupt input isn't emulated yet, this will
> >> - * come with the PSI bridge model.
> >> */
> >> static void pnv_lpc_isa_irq_handler_cpld(void *opaque, int n, int level)
> >> {
> >> - /* We don't yet emulate the PSI bridge which provides the external
> >> - * interrupt, so just drop interrupts on the floor
> >> - */
> >> + PnvMachineState *pnv = POWERNV_MACHINE(qdev_get_machine());
> >> + uint32_t old_state = pnv->cpld_irqstate;
> >> + PnvChip *chip = opaque;
> >> +
> >> + if (level) {
> >> + pnv->cpld_irqstate |= 1u << n;
> >> + } else {
> >> + pnv->cpld_irqstate &= ~(1u << n);
> >> + }
> >> + if (pnv->cpld_irqstate != old_state) {
> >> + pnv_psi_irq_set(&chip->psi, PSIHB_IRQ_EXTERNAL,
> >> + pnv->cpld_irqstate != 0);
> >> + }
> >> }
> >>
> >> static void pnv_lpc_isa_irq_handler(void *opaque, int n, int level)
> >> @@ -702,6 +709,11 @@ static void pnv_chip_init(Object *obj)
> >>
> >> object_initialize(&chip->lpc, sizeof(chip->lpc), TYPE_PNV_LPC);
> >> object_property_add_child(obj, "lpc", OBJECT(&chip->lpc), NULL);
> >> +
> >> + object_initialize(&chip->psi, sizeof(chip->psi), TYPE_PNV_PSI);
> >> + object_property_add_child(obj, "psi", OBJECT(&chip->psi), NULL);
> >> + object_property_add_const_link(OBJECT(&chip->psi), "xics",
> >> + OBJECT(qdev_get_machine()),
> >> &error_abort);
> >> }
> >>
> >> static void pnv_chip_icp_realize(PnvChip *chip, Error **errp)
> >> @@ -722,6 +734,8 @@ static void pnv_chip_realize(DeviceState *dev, Error
> >> **errp)
> >> char *typename = pnv_core_typename(pcc->cpu_model);
> >> size_t typesize = object_type_get_instance_size(typename);
> >> int i, core_hwid;
> >> + MachineState *machine = MACHINE(qdev_get_machine());
> >> + PnvMachineState *pnv = POWERNV_MACHINE(machine);
> >>
> >> if (!object_class_by_name(typename)) {
> >> error_setg(errp, "Unable to find PowerNV CPU Core '%s'",
> >> typename);
> >> @@ -797,6 +811,15 @@ static void pnv_chip_realize(DeviceState *dev, Error
> >> **errp)
> >> }
> >> g_free(typename);
> >>
> >> +
> >> + /* Processor Service Interface (PSI) Host Bridge */
> >> + object_property_set_bool(OBJECT(&chip->psi), true, "realized",
> >> + &error_fatal);
> >> + pnv_xscom_add_subregion(chip, PNV_XSCOM_PSI_BASE,
> >> &chip->psi.xscom_regs);
> >> +
> >> + /* link in the PSI ICS */
> >> + QLIST_INSERT_HEAD(&pnv->ics, &chip->psi.ics, list);
> >> +
> >> /* Create LPC controller */
> >> object_property_set_bool(OBJECT(&chip->lpc), true, "realized",
> >> &error_fatal);
> >> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> >> new file mode 100644
> >> index 000000000000..6ba688aac075
> >> --- /dev/null
> >> +++ b/hw/ppc/pnv_psi.c
> >> @@ -0,0 +1,583 @@
> >> +/*
> >> + * QEMU PowerNV PowerPC PSI interface
> >> + *
> >> + * Copyright (c) 2016, IBM Corporation
> >> + *
> >> + * This library is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU Lesser General Public
> >> + * License as published by the Free Software Foundation; either
> >> + * version 2 of the License, or (at your option) any later version.
> >> + *
> >> + * This library is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> >> + * Lesser General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU Lesser General Public
> >> + * License along with this library; if not, see
> >> <http://www.gnu.org/licenses/>.
> >> + */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "hw/hw.h"
> >> +#include "target/ppc/cpu.h"
> >> +#include "qemu/log.h"
> >> +#include "qapi/error.h"
> >> +
> >> +#include "exec/address-spaces.h"
> >> +
> >> +#include "hw/ppc/fdt.h"
> >> +#include "hw/ppc/pnv.h"
> >> +#include "hw/ppc/pnv_xscom.h"
> >> +#include "hw/ppc/pnv_psi.h"
> >> +
> >> +#include <libfdt.h>
> >> +
> >> +#define PSIHB_XSCOM_FIR_RW 0x00
> >> +#define PSIHB_XSCOM_FIR_AND 0x01
> >> +#define PSIHB_XSCOM_FIR_OR 0x02
> >> +#define PSIHB_XSCOM_FIRMASK_RW 0x03
> >> +#define PSIHB_XSCOM_FIRMASK_AND 0x04
> >> +#define PSIHB_XSCOM_FIRMASK_OR 0x05
> >> +#define PSIHB_XSCOM_FIRACT0 0x06
> >> +#define PSIHB_XSCOM_FIRACT1 0x07
> >> +#define PSIHB_XSCOM_BAR 0x0a
> >> +#define PSIHB_BAR_EN 0x0000000000000001ull
> >> +#define PSIHB_XSCOM_FSPBAR 0x0b
> >> +#define PSIHB_XSCOM_CR 0x0e
> >> +#define PSIHB_CR_FSP_CMD_ENABLE 0x8000000000000000ull
> >> +#define PSIHB_CR_FSP_MMIO_ENABLE 0x4000000000000000ull
> >> +#define PSIHB_CR_FSP_IRQ_ENABLE 0x1000000000000000ull
> >> +#define PSIHB_CR_FSP_ERR_RSP_ENABLE 0x0800000000000000ull
> >> +#define PSIHB_CR_PSI_LINK_ENABLE 0x0400000000000000ull
> >> +#define PSIHB_CR_FSP_RESET 0x0200000000000000ull
> >> +#define PSIHB_CR_PSIHB_RESET 0x0100000000000000ull
> >> +#define PSIHB_CR_PSI_IRQ 0x0000800000000000ull
> >> +#define PSIHB_CR_FSP_IRQ 0x0000400000000000ull
> >> +#define PSIHB_CR_FSP_LINK_ACTIVE 0x0000200000000000ull
> >> + /* and more ... */
> >> +#define PSIHB_XSCOM_SEMR 0x0f
> >> +#define PSIHB_XSCOM_XIVR_PSI 0x10
> >> +#define PSIHB_XIVR_SERVER_SH 40
> >> +#define PSIHB_XIVR_SERVER_MSK (0xffffull << PSIHB_XIVR_SERVER_SH)
> >> +#define PSIHB_XIVR_PRIO_SH 32
> >> +#define PSIHB_XIVR_PRIO_MSK (0xffull << PSIHB_XIVR_PRIO_SH)
> >> +#define PSIHB_XIVR_SRC_SH 29
> >> +#define PSIHB_XIVR_SRC_MSK (0x7ull << PSIHB_XIVR_SRC_SH)
> >> +#define PSIHB_XIVR_PENDING 0x01000000ull
> >> +#define PSIHB_XSCOM_SCR 0x12
> >> +#define PSIHB_XSCOM_CCR 0x13
> >> +#define PSIHB_XSCOM_DMA_UPADD 0x14
> >> +#define PSIHB_XSCOM_IRQ_STAT 0x15
> >> +#define PSIHB_IRQ_STAT_OCC 0x0000001000000000ull
> >> +#define PSIHB_IRQ_STAT_FSI 0x0000000800000000ull
> >> +#define PSIHB_IRQ_STAT_LPCI2C 0x0000000400000000ull
> >> +#define PSIHB_IRQ_STAT_LOCERR 0x0000000200000000ull
> >> +#define PSIHB_IRQ_STAT_EXT 0x0000000100000000ull
> >> +#define PSIHB_XSCOM_XIVR_OCC 0x16
> >> +#define PSIHB_XSCOM_XIVR_FSI 0x17
> >> +#define PSIHB_XSCOM_XIVR_LPCI2C 0x18
> >> +#define PSIHB_XSCOM_XIVR_LOCERR 0x19
> >> +#define PSIHB_XSCOM_XIVR_EXT 0x1a
> >> +#define PSIHB_XSCOM_IRSN 0x1b
> >> +#define PSIHB_IRSN_COMP_SH 45
> >> +#define PSIHB_IRSN_COMP_MSK (0x7ffffull << PSIHB_IRSN_COMP_SH)
> >> +#define PSIHB_IRSN_IRQ_MUX 0x0000000800000000ull
> >> +#define PSIHB_IRSN_IRQ_RESET 0x0000000400000000ull
> >> +#define PSIHB_IRSN_DOWNSTREAM_EN 0x0000000200000000ull
> >> +#define PSIHB_IRSN_UPSTREAM_EN 0x0000000100000000ull
> >> +#define PSIHB_IRSN_COMPMASK_SH 13
> >> +#define PSIHB_IRSN_COMPMASK_MSK (0x7ffffull <<
> >> PSIHB_IRSN_COMPMASK_SH)
> >> +
> >> +/*
> >> + * These are the values of the registers when accessed through the
> >> + * MMIO region. The relation is xscom = (mmio + 0x50) >> 3
> >> + */
> >> +#define PSIHB_MMIO_BAR 0x00
> >> +#define PSIHB_MMIO_FSPBAR 0x08
> >> +#define PSIHB_MMIO_CR 0x20
> >> +#define PSIHB_MMIO_SEMR 0x28
> >> +#define PSIHB_MMIO_XIVR_PSI 0x30
> >> +#define PSIHB_MMIO_SCR 0x40
> >> +#define PSIHB_MMIO_CCR 0x48
> >> +#define PSIHB_MMIO_DMA_UPADD 0x50
> >> +#define PSIHB_MMIO_IRQ_STAT 0x58
> >> +#define PSIHB_MMIO_XIVR_OCC 0x60
> >> +#define PSIHB_MMIO_XIVR_FSI 0x68
> >> +#define PSIHB_MMIO_XIVR_LPCI2C 0x70
> >> +#define PSIHB_MMIO_XIVR_LOCERR 0x78
> >> +#define PSIHB_MMIO_XIVR_EXT 0x80
> >> +#define PSIHB_MMIO_IRSN 0x88
> >> +#define PSIHB_MMIO_MAX 0x100
> >> +
> >> +static void pnv_psi_set_bar(PnvPsi *psi, uint64_t bar)
> >> +{
> >> + MemoryRegion *sysmem = get_system_memory();
> >> + uint64_t old = psi->regs[PSIHB_XSCOM_BAR];
> >> +
> >> + psi->regs[PSIHB_XSCOM_BAR] = bar & 0x0003fffffff00001;
> >> +
> >> + /* Update MR, always remove it first */
> >> + if (old & PSIHB_BAR_EN) {
> >> + memory_region_del_subregion(sysmem, &psi->regs_mr);
> >> + }
> >> + /* Then add it back if needed */
> >> + if (bar & PSIHB_BAR_EN) {
> >> + uint64_t addr = bar & 0x0003fffffff00000;
> >> + memory_region_add_subregion(sysmem, addr, &psi->regs_mr);
> >> + }
> >> +}
> >> +
> >> +static void pnv_psi_update_fsp_mr(PnvPsi *psi)
> >> +{
> >> + /* XXX Update FSP MR if/when we support FSP BAR */
> >> +}
> >> +
> >> +static void pnv_psi_set_cr(PnvPsi *psi, uint64_t cr)
> >> +{
> >> + uint64_t old = psi->regs[PSIHB_XSCOM_CR];
> >> +
> >> + psi->regs[PSIHB_XSCOM_CR] = cr & 0x0003ffff00000000;
> >> +
> >> + /* Check some bit changes */
> >> + if ((old ^ psi->regs[PSIHB_XSCOM_CR]) & PSIHB_CR_FSP_MMIO_ENABLE) {
> >> + pnv_psi_update_fsp_mr(psi);
> >> + }
> >> +}
> >> +
> >> +static void pnv_psi_set_irsn(PnvPsi *psi, uint64_t val)
> >> +{
> >> + uint32_t offset;
> >> + ICSState *ics = &psi->ics;
> >> +
> >> + /* In this model we ignore the up/down enable bits for now
> >> + * as SW doesn't use them (other than setting them at boot).
> >> + * We ignore IRQ_MUX, its meaning isn't clear and we don't use
> >> + * it and finally we ignore reset (XXX fix that ?)
> >> + */
> >> + psi->regs[PSIHB_XSCOM_IRSN] = val & (PSIHB_IRSN_COMP_MSK |
> >> + PSIHB_IRSN_IRQ_MUX |
> >> + PSIHB_IRSN_DOWNSTREAM_EN |
> >> + PSIHB_IRSN_DOWNSTREAM_EN |
> >> + PSIHB_IRSN_DOWNSTREAM_EN);
> >> +
> >> + /* We ignore the compare mask as well, our ICS emulation is too
> >> + * simplistic to make any use if it, and we extract the offset
> >> + * from the compare value
> >> + */
> >> + offset = (val & PSIHB_IRSN_COMP_MSK) >> PSIHB_IRSN_COMP_SH;
> >> + ics->offset = offset;
> >> +}
> >> +
> >> +static bool pnv_psi_irq_bits(PnvPsi *psi, PnvPsiIrq irq,
> >> + uint32_t *out_xivr_reg,
> >> + uint32_t *out_stat_reg,
> >> + uint64_t *out_stat_bit)
> >
> > Your PnvPsiIrq values are arbitrary and contiguous AFACT. Why not
> > just have a lookup table for this info, instead of a giant switch
> > statement?
>
> Well, I agree but at the same time, we are not gaining much in terms
> of lines by using an array,
Hmm.. seems like an ~ x5 line reduction to me...
And, IMO, easier to read.
> and we have to check for boundaries which
> the switch provide. The question would be different if we had more
> parameters. So let's keep it that way.
>
> >> +{
> >> + switch (irq) {
> >> + case PSIHB_IRQ_PSI:
> >> + *out_xivr_reg = PSIHB_XSCOM_XIVR_PSI;
> >> + *out_stat_reg = PSIHB_XSCOM_CR;
> >> + *out_stat_bit = PSIHB_CR_PSI_IRQ;
> >> + break;
> >> + case PSIHB_IRQ_FSP:
> >> + *out_xivr_reg = PSIHB_XSCOM_XIVR_PSI;
> >> + *out_stat_reg = PSIHB_XSCOM_CR;
> >> + *out_stat_bit = PSIHB_CR_FSP_IRQ;
> >> + break;
> >> + case PSIHB_IRQ_OCC:
> >> + *out_xivr_reg = PSIHB_XSCOM_XIVR_OCC;
> >> + *out_stat_reg = PSIHB_XSCOM_IRQ_STAT;
> >> + *out_stat_bit = PSIHB_IRQ_STAT_OCC;
> >> + break;
> >> + case PSIHB_IRQ_FSI:
> >> + *out_xivr_reg = PSIHB_XSCOM_XIVR_FSI;
> >> + *out_stat_reg = PSIHB_XSCOM_IRQ_STAT;
> >> + *out_stat_bit = PSIHB_IRQ_STAT_FSI;
> >> + break;
> >> + case PSIHB_IRQ_LPC_I2C:
> >> + *out_xivr_reg = PSIHB_XSCOM_XIVR_LPCI2C;
> >> + *out_stat_reg = PSIHB_XSCOM_IRQ_STAT;
> >> + *out_stat_bit = PSIHB_IRQ_STAT_LPCI2C;
> >> + break;
> >> + case PSIHB_IRQ_LOCAL_ERR:
> >> + *out_xivr_reg = PSIHB_XSCOM_XIVR_LOCERR;
> >> + *out_stat_reg = PSIHB_XSCOM_IRQ_STAT;
> >> + *out_stat_bit = PSIHB_IRQ_STAT_LOCERR;
> >> + break;
> >> + case PSIHB_IRQ_EXTERNAL:
> >> + *out_xivr_reg = PSIHB_XSCOM_XIVR_EXT;
> >> + *out_stat_reg = PSIHB_XSCOM_IRQ_STAT;
> >> + *out_stat_bit = PSIHB_IRQ_STAT_EXT;
> >> + break;
> >> + default:
> >> + return false;
> >> + }
> >> + return true;
> >> +}
> >> +
> >> +void pnv_psi_irq_set(PnvPsi *psi, PnvPsiIrq irq, bool state)
> >> +{
> >> + ICSState *ics = &psi->ics;
> >> + uint32_t xivr_reg;
> >> + uint32_t stat_reg;
> >> + uint64_t stat_bit;
> >> + uint32_t src;
> >> + bool masked;
> >> +
> >> + if (!pnv_psi_irq_bits(psi, irq, &xivr_reg, &stat_reg, &stat_bit)) {
> >> + qemu_log_mask(LOG_GUEST_ERROR, "PSI: Unsupported irq %d\n", irq);
> >> + return;
> >> + }
> >> +
> >> + src = (psi->regs[xivr_reg] & PSIHB_XIVR_SRC_MSK) >> PSIHB_XIVR_SRC_SH;
> >> + masked = (psi->regs[xivr_reg] & PSIHB_XIVR_PRIO_MSK) ==
> >> PSIHB_XIVR_PRIO_MSK;
> >> + if (state) {
> >> + psi->regs[stat_reg] |= stat_bit;
> >> + /* XXX optimization: check mask here. That means re-evaluating
> >> + * when unmasking, thus TODO
> >> + */
> >> + qemu_irq_raise(ics->qirqs[src]);
> >> + } else {
> >> + psi->regs[stat_reg] &= ~stat_bit;
> >> +
> >> + /* FSP and PSI are muxed so don't lower if either still set */
> >> + if (stat_reg != PSIHB_XSCOM_CR ||
> >> + !(psi->regs[stat_reg] & (PSIHB_CR_PSI_IRQ |
> >> PSIHB_CR_FSP_IRQ))) {
> >> + qemu_irq_lower(ics->qirqs[src]);
> >> + } else {
> >> + state = true;
> >> + }
> >> + }
> >
> > It might be cleaner to just revaluate the irq level from scratch here,
> > and set the level, rather than doing this complicated dance to work
> > out if it has changed.
>
> OK. I need to take a closer look at that.
>
> >> +
> >> + /* XXX Note about the emulation of the pending bit: This isn't
> >> + * entirely correct. The pending bit should be cleared when the
> >> + * EOI has been received. However, we don't have callbacks on
> >> + * EOI (especially not under KVM) so no way to emulate that
> >> + * properly, so instead we just set that bit as the logical
> >> + * "output" of the XIVR (ie pending & !masked)
> >> + * XXX TODO: Also update it on set_xivr
> >> + */
> >> + if (state && !masked) {
> >> + psi->regs[xivr_reg] |= PSIHB_XIVR_PENDING;
> >> + } else {
> >> + psi->regs[xivr_reg] &= ~PSIHB_XIVR_PENDING;
> >> + }
> >> +}
> >> +
> >> +static void pnv_psi_set_xivr(PnvPsi *psi, uint32_t reg, uint64_t val)
> >> +{
> >> + ICSState *ics = &psi->ics;
> >> + uint16_t server;
> >> + uint8_t prio;
> >> + uint8_t src;
> >> + int icp_index;
> >> +
> >> + psi->regs[reg] = (psi->regs[reg] & PSIHB_XIVR_PENDING) |
> >> + (val & (PSIHB_XIVR_SERVER_MSK |
> >> + PSIHB_XIVR_PRIO_MSK |
> >> + PSIHB_XIVR_SRC_MSK));
> >> + val = psi->regs[reg];
> >> + server = (val & PSIHB_XIVR_SERVER_MSK) >> PSIHB_XIVR_SERVER_SH;
> >> + prio = (val & PSIHB_XIVR_PRIO_MSK) >> PSIHB_XIVR_PRIO_SH;
> >> + src = (val & PSIHB_XIVR_SRC_MSK) >> PSIHB_XIVR_SRC_SH;
> >> + if (src > PSIHB_IRQ_EXTERNAL) {
> >> + /* XXX Generate error ? */
> >> + return;
> >> + }
> >> +
> >> + /*
> >> + * Linux fills the irq xivr with the hw processor id plus the
> >> + * link bits. shift back to get something valid.
> >> + */
> >> + server >>= 2;
> >> +
> >> + /*
> >> + * When skiboot initializes PSIHB, it fills the xives with
> >> + * server=0, prio=0xff, but we don't have a CPU with a pir=0. So
> >> + * skip that case.
> >> + */
> >> + if (prio != 0xff) {
> >> + icp_index = xics_get_cpu_index_by_pir(server);
> >> + assert(icp_index != -1);
> >> + } else {
> >> + if (server) {
> >> + qemu_log_mask(LOG_GUEST_ERROR, "PSI: bogus server %d for IRQ
> >> %d\n",
> >> + server, src);
> >> + }
> >> + icp_index = server;
> >> + }
> >
> > This logic doesn't seem like it belongs here. You've received a
> > server number, seems like you should just pass it on to the xics code,
> > and if there can be an error, have that detect it.
>
> I have removed all of that in a private patch. This is tracking
> an issue in skiboot which did not clear correctly the PSI xive.
> It is partially resolved in recent version but I still see some
> warnings whe the guest reboots.
Ok. Workarounds for firmware bugs are fine, but they need a detailed
comment so other people have a hope of understanding why they're
there. Including stating outright that it is a bug workaround, rather
than expected firmware behaviour.
>
> >> +
> >> + /* Now because of source remapping, weird things can happen
> >> + * if you change the source number dynamically, our simple ICS
> >> + * doesn't deal with remapping. So we just poke a different
> >> + * ICS entry based on what source number was written. This will
> >> + * do for now but a more accurate implementation would instead
> >> + * use a fixed server/prio and a remapper of the generated irq.
> >> + */
> >> + ics_simple_write_xive(ics, src, icp_index, prio, prio);
> >> +}
> >> +
> >> +static uint64_t pnv_psi_reg_read(PnvPsi *psi, uint32_t offset, bool mmio)
> >> +{
> >> + uint64_t val = 0xffffffffffffffffull;
> >> +
> >> + switch (offset) {
> >> + case PSIHB_XSCOM_FIR_RW:
> >> + case PSIHB_XSCOM_FIRACT0:
> >> + case PSIHB_XSCOM_FIRACT1:
> >> + case PSIHB_XSCOM_BAR:
> >> + case PSIHB_XSCOM_FSPBAR:
> >> + case PSIHB_XSCOM_CR:
> >> + case PSIHB_XSCOM_XIVR_PSI:
> >> + case PSIHB_XSCOM_XIVR_OCC:
> >> + case PSIHB_XSCOM_XIVR_FSI:
> >> + case PSIHB_XSCOM_XIVR_LPCI2C:
> >> + case PSIHB_XSCOM_XIVR_LOCERR:
> >> + case PSIHB_XSCOM_XIVR_EXT:
> >> + case PSIHB_XSCOM_IRQ_STAT:
> >> + case PSIHB_XSCOM_SEMR:
> >> + case PSIHB_XSCOM_DMA_UPADD:
> >> + case PSIHB_XSCOM_IRSN:
> >> + val = psi->regs[offset];
> >> + break;
> >> + default:
> >> + qemu_log_mask(LOG_UNIMP, "PSI Unimplemented register: Ox%" PRIx32
> >> "\n",
> >> + offset);
> >
> > As noted elsewhere, tracepoints are more usual than qemu_log() these
> > days. But either way, this really should have a distinguishable
> > message from the one in the write path.
>
> OK. Will add a read/write statement.
>
> >> + }
> >> + return val;
> >> +}
> >> +
> >> +static void pnv_psi_reg_write(PnvPsi *psi, uint32_t offset, uint64_t val,
> >> + bool mmio)
> >> +{
> >> + switch (offset) {
> >> + case PSIHB_XSCOM_FIR_RW:
> >> + case PSIHB_XSCOM_FIRACT0:
> >> + case PSIHB_XSCOM_FIRACT1:
> >> + case PSIHB_XSCOM_SEMR:
> >> + case PSIHB_XSCOM_DMA_UPADD:
> >> + psi->regs[offset] = val;
> >> + break;
> >> + case PSIHB_XSCOM_FIR_OR:
> >> + psi->regs[PSIHB_XSCOM_FIR_RW] |= val;
> >> + break;
> >> + case PSIHB_XSCOM_FIR_AND:
> >> + psi->regs[PSIHB_XSCOM_FIR_RW] &= val;
> >> + break;
> >> + case PSIHB_XSCOM_BAR:
> >> + /* Only XSCOM can write this one */
> >> + if (!mmio) {
> >> + pnv_psi_set_bar(psi, val);
> >> + }
> >> + break;
> >> + case PSIHB_XSCOM_FSPBAR:
> >> + psi->regs[PSIHB_XSCOM_BAR] = val & 0x0003ffff00000000;
> >
> > Should that be PSIHB_XSCOM_FSPBAR?
>
> yes ...
>
> >> + pnv_psi_update_fsp_mr(psi);
> >> + break;
> >> + case PSIHB_XSCOM_CR:
> >> + pnv_psi_set_cr(psi, val);
> >> + break;
> >> + case PSIHB_XSCOM_SCR:
> >> + pnv_psi_set_cr(psi, psi->regs[PSIHB_XSCOM_CR] | val);
> >> + break;
> >> + case PSIHB_XSCOM_CCR:
> >> + pnv_psi_set_cr(psi, psi->regs[PSIHB_XSCOM_CR] & ~val);
> >> + break;
> >> + case PSIHB_XSCOM_XIVR_PSI:
> >> + case PSIHB_XSCOM_XIVR_OCC:
> >> + case PSIHB_XSCOM_XIVR_FSI:
> >> + case PSIHB_XSCOM_XIVR_LPCI2C:
> >> + case PSIHB_XSCOM_XIVR_LOCERR:
> >> + case PSIHB_XSCOM_XIVR_EXT:
> >> + pnv_psi_set_xivr(psi, offset, val);
> >> + break;
> >> + case PSIHB_XSCOM_IRQ_STAT:
> >> + /* Read only, should we generate an error ? */
> >> + break;
> >> + case PSIHB_XSCOM_IRSN:
> >> + pnv_psi_set_irsn(psi, val);
> >> + break;
> >> + default:
> >> + qemu_log_mask(LOG_UNIMP, "PSI Unimplemented register: Ox%" PRIx32
> >> "\n",
> >> + offset);
> >> + }
> >> +}
> >> +
> >> +static inline uint32_t psi_mmio_to_xscom(hwaddr addr)
> >> +{
> >> + return (addr >> 3) + PSIHB_XSCOM_BAR;
> >> +}
> >> +
> >> +static uint64_t pnv_psi_mmio_read(void *opaque, hwaddr addr, unsigned
> >> size)
> >> +{
> >> + return pnv_psi_reg_read(opaque, psi_mmio_to_xscom(addr), true);
> >> +}
> >> +
> >> +static void pnv_psi_mmio_write(void *opaque, hwaddr addr,
> >> + uint64_t val, unsigned size)
> >> +{
> >> + pnv_psi_reg_write(opaque, psi_mmio_to_xscom(addr), val, true);
> >> +}
> >> +
> >> +static const MemoryRegionOps psi_mmio_ops = {
> >> + .read = pnv_psi_mmio_read,
> >> + .write = pnv_psi_mmio_write,
> >> + .endianness = DEVICE_BIG_ENDIAN,
> >> + .valid = {
> >> + .min_access_size = 8,
> >> + .max_access_size = 8,
> >> + },
> >> + .impl = {
> >> + .min_access_size = 8,
> >> + .max_access_size = 8,
> >> + },
> >> +};
> >> +
> >> +static uint64_t pnv_psi_xscom_read(void *opaque, hwaddr addr, unsigned
> >> size)
> >> +{
> >> + PnvPsi *psi = PNV_PSI(opaque);
> >> + uint32_t offset = addr >> 3;
> >> +
> >> + return pnv_psi_reg_read(psi, offset, false);
> >> +}
> >> +
> >> +static void pnv_psi_xscom_write(void *opaque, hwaddr addr,
> >> + uint64_t val, unsigned size)
> >> +{
> >> + PnvPsi *psi = PNV_PSI(opaque);
> >> + uint32_t offset = addr >> 3;
> >> +
> >> + pnv_psi_reg_write(psi, offset, val, false);
> >> +}
> >> +
> >> +static const MemoryRegionOps pnv_psi_xscom_ops = {
> >> + .read = pnv_psi_xscom_read,
> >> + .write = pnv_psi_xscom_write,
> >> + .valid.min_access_size = 8,
> >> + .valid.max_access_size = 8,
> >> + .impl.min_access_size = 8,
> >> + .impl.max_access_size = 8,
> >
> > Consistent nesting format of the two MemoryRegionOps would be good.
>
> OK.
>
> >> + .endianness = DEVICE_BIG_ENDIAN,
> >> +};
> >> +
> >> +static void pnv_psi_init(Object *obj)
> >> +{
> >> + PnvPsi *psi = PNV_PSI(obj);
> >> +
> >> + object_initialize(&psi->ics, sizeof(psi->ics), TYPE_ICS_SIMPLE);
> >> + qdev_set_parent_bus(DEVICE(&psi->ics), sysbus_get_default());
> >> + object_property_add_child(obj, "ics-psi", OBJECT(&psi->ics), NULL);
> >> +}
> >> +
> >> +static void pnv_psi_realize(DeviceState *dev, Error **errp)
> >> +{
> >> + PnvPsi *psi = PNV_PSI(dev);
> >> + ICSState *ics = &psi->ics;
> >> + Object *obj;
> >> + Error *err = NULL, *local_err = NULL;
> >> + unsigned int i;
> >> +
> >> + /* Initialize MMIO region */
> >> + memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
> >> + "psihb", PNV_PSIHB_BAR_SIZE);
> >> +
> >> + /* Default BAR. Use object properties ? */
> >> + pnv_psi_set_bar(psi, PNV_PSIHB_BAR | PSIHB_BAR_EN);
> >> +
> >> + /* Default sources in XIVR */
> >> + psi->regs[PSIHB_XSCOM_XIVR_PSI] = PSIHB_XIVR_PRIO_MSK |
> >> + (0ull << PSIHB_XIVR_SRC_SH);
> >> + psi->regs[PSIHB_XSCOM_XIVR_OCC] = PSIHB_XIVR_PRIO_MSK |
> >> + (1ull << PSIHB_XIVR_SRC_SH);
> >> + psi->regs[PSIHB_XSCOM_XIVR_FSI] = PSIHB_XIVR_PRIO_MSK |
> >> + (2ull << PSIHB_XIVR_SRC_SH);
> >> + psi->regs[PSIHB_XSCOM_XIVR_LPCI2C] = PSIHB_XIVR_PRIO_MSK |
> >> + (3ull << PSIHB_XIVR_SRC_SH);
> >> + psi->regs[PSIHB_XSCOM_XIVR_LOCERR] = PSIHB_XIVR_PRIO_MSK |
> >> + (4ull << PSIHB_XIVR_SRC_SH);
> >> + psi->regs[PSIHB_XSCOM_XIVR_EXT] = PSIHB_XIVR_PRIO_MSK |
> >> + (5ull << PSIHB_XIVR_SRC_SH);
> >> +
> >> + /* get XICSFabric from chip */
> >> + obj = object_property_get_link(OBJECT(dev), "xics", &err);
> >> + if (!obj) {
> >> + error_setg(errp, "%s: required link 'xics' not found: %s",
> >> + __func__, error_get_pretty(err));
> >> + return;
> >> + }
> >> +
> >> + /*
> >> + * PSI interrupt control source
> >> + */
> >> + object_property_set_int(OBJECT(ics), PSI_NUM_INTERRUPTS, "nr-irqs",
> >> &err);
> >> + object_property_add_const_link(OBJECT(ics), "xics", obj, &err);
> >> + object_property_set_bool(OBJECT(ics), true, "realized", &local_err);
> >> + error_propagate(&err, local_err);
> >> + if (err) {
> >> + error_propagate(errp, err);
> >> + return;
> >> + }
> >> +
> >> + for (i = 0; i < ics->nr_irqs; i++) {
> >> + ics_set_irq_type(ics, i, true);
> >> + }
> >> +
> >> + /* XScom region for PSI registers */
> >> + pnv_xscom_region_init(&psi->xscom_regs, OBJECT(dev),
> >> &pnv_psi_xscom_ops,
> >> + psi, "xscom-psi", PNV_XSCOM_PSI_SIZE);
> >> +}
> >> +
> >> +static int pnv_psi_populate(PnvXScomInterface *dev, void *fdt, int
> >> xscom_offset)
> >> +{
> >> + const char compat[] = "ibm,power8-psihb-x\0ibm,psihb-x";
> >> + char *name;
> >> + int offset;
> >> + uint32_t lpc_pcba = PNV_XSCOM_PSI_BASE;
> >> + uint32_t reg[] = {
> >> + cpu_to_be32(lpc_pcba),
> >> + cpu_to_be32(PNV_XSCOM_PSI_SIZE)
> >> + };
> >> +
> >> + name = g_strdup_printf("address@hidden", lpc_pcba);
> >> + offset = fdt_add_subnode(fdt, xscom_offset, name);
> >> + _FDT(offset);
> >> + g_free(name);
> >> +
> >> + _FDT((fdt_setprop(fdt, offset, "reg", reg, sizeof(reg))));
> >> +
> >> + _FDT((fdt_setprop_cell(fdt, offset, "#address-cells", 2)));
> >> + _FDT((fdt_setprop_cell(fdt, offset, "#size-cells", 1)));
> >> + _FDT((fdt_setprop(fdt, offset, "compatible", compat,
> >> + sizeof(compat))));
> >> + return 0;
> >> +}
> >> +
> >> +
> >> +static void pnv_psi_class_init(ObjectClass *klass, void *data)
> >> +{
> >> + DeviceClass *dc = DEVICE_CLASS(klass);
> >> + PnvXScomInterfaceClass *xdc = PNV_XSCOM_INTERFACE_CLASS(klass);
> >> +
> >> + xdc->populate = pnv_psi_populate;
> >> +
> >> + dc->realize = pnv_psi_realize;
> >> +}
> >> +
> >> +static const TypeInfo pnv_psi_info = {
> >> + .name = TYPE_PNV_PSI,
> >> + .parent = TYPE_DEVICE,
> >
> > Since the PSI has an MMIO presence, it probably should be a
> > SysBusDevice, rather than a raw descendent of TYPE_DEVICE.
>
> Yes indeed.
>
> So I will resend the patchset with just the XICS part. I want to
> take a look at pnv_psi_irq_set() first.
>
> Thanks,
>
> C.
>
> >> + .instance_size = sizeof(PnvPsi),
> >> + .instance_init = pnv_psi_init,
> >> + .class_init = pnv_psi_class_init,
> >> + .interfaces = (InterfaceInfo[]) {
> >> + { TYPE_PNV_XSCOM_INTERFACE },
> >> + { }
> >> + }
> >> +};
> >> +
> >> +static void pnv_psi_register_types(void)
> >> +{
> >> + type_register_static(&pnv_psi_info);
> >> +}
> >> +
> >> +type_init(pnv_psi_register_types)
> >> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> >> index f11215ea31f2..f93ec32603b7 100644
> >> --- a/include/hw/ppc/pnv.h
> >> +++ b/include/hw/ppc/pnv.h
> >> @@ -23,6 +23,7 @@
> >> #include "hw/sysbus.h"
> >> #include "hw/ppc/pnv_lpc.h"
> >> #include "hw/ppc/xics.h"
> >> +#include "hw/ppc/pnv_psi.h"
> >>
> >> #define TYPE_PNV_CHIP "powernv-chip"
> >> #define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP)
> >> @@ -58,6 +59,7 @@ typedef struct PnvChip {
> >> MemoryRegion icp_mmio;
> >>
> >> PnvLpcController lpc;
> >> + PnvPsi psi;
> >> } PnvChip;
> >>
> >> typedef struct PnvChipClass {
> >> @@ -119,6 +121,8 @@ typedef struct PnvMachineState {
> >> ICPState *icps;
> >> uint32_t nr_servers;
> >> QLIST_HEAD(, ICSState) ics;
> >> +
> >> + uint32_t cpld_irqstate;
> >> } PnvMachineState;
> >>
> >> #define PNV_FDT_ADDR 0x01000000
> >> @@ -150,4 +154,8 @@ typedef struct PnvMachineState {
> >> #define PNV_ICP_BASE(chip) 0x0003ffff80000000ull
> >> #define PNV_ICP_SIZE 0x0000000000100000ull
> >>
> >> +#define PNV_PSIHB_BAR 0x0003fffe80000000ull
> >> +#define PNV_PSIHB_BAR_SIZE 0x0000000000100000ull
> >> +
> >> +
> >> #endif /* _PPC_PNV_H */
> >> diff --git a/include/hw/ppc/pnv_psi.h b/include/hw/ppc/pnv_psi.h
> >> new file mode 100644
> >> index 000000000000..ac3c5f8362e3
> >> --- /dev/null
> >> +++ b/include/hw/ppc/pnv_psi.h
> >> @@ -0,0 +1,61 @@
> >> +/*
> >> + * QEMU PowerPC PowerNV PSI controller
> >> + *
> >> + * Copyright (c) 2016, IBM Corporation.
> >> + *
> >> + * This library is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU Lesser General Public
> >> + * License as published by the Free Software Foundation; either
> >> + * version 2 of the License, or (at your option) any later version.
> >> + *
> >> + * This library is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> >> + * Lesser General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU Lesser General Public
> >> + * License along with this library; if not, see
> >> <http://www.gnu.org/licenses/>.
> >> + */
> >> +#ifndef _PPC_PNV_PSI_H
> >> +#define _PPC_PNV_PSI_H
> >> +
> >> +#define TYPE_PNV_PSI "pnv-psi"
> >> +#define PNV_PSI(obj) \
> >> + OBJECT_CHECK(PnvPsi, (obj), TYPE_PNV_PSI)
> >> +
> >> +#define PSIHB_XSCOM_MAX 0x20
> >> +
> >> +typedef struct XICSState XICSState;
> >> +
> >> +typedef struct PnvPsi {
> >> + DeviceState parent;
> >> +
> >> + MemoryRegion regs_mr;
> >> +
> >> + /* FSP region not supported */
> >> + /* MemoryRegion fsp_mr; */
> >> +
> >> + /* Interrupt generation */
> >> + ICSState ics;
> >> +
> >> + /* Registers */
> >> + uint64_t regs[PSIHB_XSCOM_MAX];
> >> +
> >> + MemoryRegion xscom_regs;
> >> +} PnvPsi;
> >> +
> >> +typedef enum PnvPsiIrq {
> >> + PSIHB_IRQ_PSI, /* internal use only */
> >> + PSIHB_IRQ_FSP, /* internal use only */
> >> + PSIHB_IRQ_OCC,
> >> + PSIHB_IRQ_FSI,
> >> + PSIHB_IRQ_LPC_I2C,
> >> + PSIHB_IRQ_LOCAL_ERR,
> >> + PSIHB_IRQ_EXTERNAL,
> >> +} PnvPsiIrq;
> >> +
> >> +#define PSI_NUM_INTERRUPTS 6
> >> +
> >> +extern void pnv_psi_irq_set(PnvPsi *psi, PnvPsiIrq irq, bool state);
> >> +
> >> +#endif /* _PPC_PNV_PSI_H */
> >> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
> >> index 0faa1847bf13..2938abd74955 100644
> >> --- a/include/hw/ppc/pnv_xscom.h
> >> +++ b/include/hw/ppc/pnv_xscom.h
> >> @@ -60,6 +60,9 @@ typedef struct PnvXScomInterfaceClass {
> >> #define PNV_XSCOM_LPC_BASE 0xb0020
> >> #define PNV_XSCOM_LPC_SIZE 0x4
> >>
> >> +#define PNV_XSCOM_PSI_BASE 0x2010900
> >> +#define PNV_XSCOM_PSI_SIZE 0x20
> >> +
> >> extern void pnv_xscom_realize(PnvChip *chip, Error **errp);
> >> extern int pnv_xscom_populate(PnvChip *chip, void *fdt, int offset);
> >>
> >
>
--
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
- [Qemu-devel] [PATCH for-2.10 3/8] ppc/pnv: create the ICP and ICS objects under the machine, (continued)
[Qemu-devel] [PATCH for-2.10 7/8] ppc/pnv: Add OCC model stub with interrupt support, Cédric Le Goater, 2017/03/08
[Qemu-devel] [PATCH for-2.10 8/8] ppc/pnv: Add support for POWER8+ LPC Controller, Cédric Le Goater, 2017/03/08