| Message ID | 20190918160645.25126-12-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc/pnv: add XIVE support for KVM guests | expand |
On Wed, Sep 18, 2019 at 06:06:31PM +0200, Cédric Le Goater wrote: > This also removes the need of the get_tctx() XiveRouter handler in the > core XIVE framework. In general these commit messages could really do with more context. What exactly is the "controller model"? Where were the TIMA operations before now. Why is having them in the controller model better? I could probably answer those with some investigation, but the point is that the commit message is supposed to make it easy to find that information. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > include/hw/ppc/xive.h | 1 - > hw/intc/pnv_xive.c | 35 ++++++++++++++++++++++++++++++++++- > hw/intc/spapr_xive.c | 33 +++++++++++++++++++++++++++++++-- > hw/intc/xive.c | 29 ----------------------------- > 4 files changed, 65 insertions(+), 33 deletions(-) > > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h > index 536deea8c622..9d9cd88dd17e 100644 > --- a/include/hw/ppc/xive.h > +++ b/include/hw/ppc/xive.h > @@ -462,7 +462,6 @@ typedef struct XiveENDSource { > #define XIVE_TM_OS_PAGE 0x2 > #define XIVE_TM_USER_PAGE 0x3 > > -extern const MemoryRegionOps xive_tm_ops; > void xive_tctx_tm_write(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset, > uint64_t value, unsigned size); > uint64_t xive_tctx_tm_read(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset, > diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c > index 3d6fcf9ac139..40e18fb44811 100644 > --- a/hw/intc/pnv_xive.c > +++ b/hw/intc/pnv_xive.c > @@ -1475,6 +1475,39 @@ static const MemoryRegionOps xive_tm_indirect_ops = { > }, > }; > > +static void pnv_xive_tm_write(void *opaque, hwaddr offset, > + uint64_t value, unsigned size) > +{ > + PowerPCCPU *cpu = POWERPC_CPU(current_cpu); > + PnvXive *xive = pnv_xive_tm_get_xive(cpu); > + XiveTCTX *tctx = XIVE_TCTX(pnv_cpu_state(cpu)->intc); > + > + xive_tctx_tm_write(XIVE_PRESENTER(xive), tctx, offset, value, size); > +} > + > +static uint64_t pnv_xive_tm_read(void *opaque, hwaddr offset, unsigned size) > +{ > + PowerPCCPU *cpu = POWERPC_CPU(current_cpu); > + PnvXive *xive = pnv_xive_tm_get_xive(cpu); > + XiveTCTX *tctx = XIVE_TCTX(pnv_cpu_state(cpu)->intc); > + > + return xive_tctx_tm_read(XIVE_PRESENTER(xive), tctx, offset, size); You're not using the opaque pointer in either of these cases. So couldn't you make it point to the presenter for pnv as well, then... > +} > + > +const MemoryRegionOps pnv_xive_tm_ops = { > + .read = pnv_xive_tm_read, > + .write = pnv_xive_tm_write, > + .endianness = DEVICE_BIG_ENDIAN, > + .valid = { > + .min_access_size = 1, > + .max_access_size = 8, > + }, > + .impl = { > + .min_access_size = 1, > + .max_access_size = 8, > + }, > +}; > + > /* > * Interrupt controller XSCOM region. > */ > @@ -1832,7 +1865,7 @@ static void pnv_xive_realize(DeviceState *dev, Error **errp) > "xive-pc", PNV9_XIVE_PC_SIZE); > > /* Thread Interrupt Management Area (Direct) */ > - memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops, > + memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &pnv_xive_tm_ops, > xive, "xive-tima", PNV9_XIVE_TM_SIZE); > > qemu_register_reset(pnv_xive_reset, dev); > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > index eefc0d4c36b9..e00a9bdd901b 100644 > --- a/hw/intc/spapr_xive.c > +++ b/hw/intc/spapr_xive.c > @@ -222,6 +222,35 @@ void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx) > memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &nvt_cam, 4); > } > > +static void spapr_xive_tm_write(void *opaque, hwaddr offset, > + uint64_t value, unsigned size) > +{ > + XiveTCTX *tctx = spapr_cpu_state(POWERPC_CPU(current_cpu))->tctx; > + > + xive_tctx_tm_write(XIVE_PRESENTER(opaque), tctx, offset, value, > size); ... there would be no difference from the spapr versions, AFAICT. > +} > + > +static uint64_t spapr_xive_tm_read(void *opaque, hwaddr offset, unsigned size) > +{ > + XiveTCTX *tctx = spapr_cpu_state(POWERPC_CPU(current_cpu))->tctx; > + > + return xive_tctx_tm_read(XIVE_PRESENTER(opaque), tctx, offset, size); > +} > + > +const MemoryRegionOps spapr_xive_tm_ops = { > + .read = spapr_xive_tm_read, > + .write = spapr_xive_tm_write, > + .endianness = DEVICE_BIG_ENDIAN, > + .valid = { > + .min_access_size = 1, > + .max_access_size = 8, > + }, > + .impl = { > + .min_access_size = 1, > + .max_access_size = 8, > + }, > +}; > + > static void spapr_xive_end_reset(XiveEND *end) > { > memset(end, 0, sizeof(*end)); > @@ -331,8 +360,8 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) > qemu_register_reset(spapr_xive_reset, dev); > > /* TIMA initialization */ > - memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops, xive, > - "xive.tima", 4ull << TM_SHIFT); > + memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &spapr_xive_tm_ops, > + xive, "xive.tima", 4ull << TM_SHIFT); > sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xive->tm_mmio); > > /* > diff --git a/hw/intc/xive.c b/hw/intc/xive.c > index 9bb09ed6ee7b..11432f04f5c3 100644 > --- a/hw/intc/xive.c > +++ b/hw/intc/xive.c > @@ -483,35 +483,6 @@ uint64_t xive_tctx_tm_read(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset, > return xive_tm_raw_read(tctx, offset, size); > } > > -static void xive_tm_write(void *opaque, hwaddr offset, > - uint64_t value, unsigned size) > -{ > - XiveTCTX *tctx = xive_router_get_tctx(XIVE_ROUTER(opaque), current_cpu); > - > - xive_tctx_tm_write(XIVE_PRESENTER(opaque), tctx, offset, value, size); > -} > - > -static uint64_t xive_tm_read(void *opaque, hwaddr offset, unsigned size) > -{ > - XiveTCTX *tctx = xive_router_get_tctx(XIVE_ROUTER(opaque), current_cpu); > - > - return xive_tctx_tm_read(XIVE_PRESENTER(opaque), tctx, offset, size); > -} > - > -const MemoryRegionOps xive_tm_ops = { > - .read = xive_tm_read, > - .write = xive_tm_write, > - .endianness = DEVICE_BIG_ENDIAN, > - .valid = { > - .min_access_size = 1, > - .max_access_size = 8, > - }, > - .impl = { > - .min_access_size = 1, > - .max_access_size = 8, > - }, > -}; > - > static char *xive_tctx_ring_print(uint8_t *ring) > { > uint32_t w2 = xive_tctx_word2(ring);
On 03/10/2019 04:08, David Gibson wrote: > On Wed, Sep 18, 2019 at 06:06:31PM +0200, Cédric Le Goater wrote: >> This also removes the need of the get_tctx() XiveRouter handler in the >> core XIVE framework. > > In general these commit messages could really do with more context. > What exactly is the "controller model"? Where were the TIMA > operations before now. Why is having them in the controller model > better? > > I could probably answer those with some investigation, but the point > is that the commit message is supposed to make it easy to find that > information. > >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> include/hw/ppc/xive.h | 1 - >> hw/intc/pnv_xive.c | 35 ++++++++++++++++++++++++++++++++++- >> hw/intc/spapr_xive.c | 33 +++++++++++++++++++++++++++++++-- >> hw/intc/xive.c | 29 ----------------------------- >> 4 files changed, 65 insertions(+), 33 deletions(-) >> >> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h >> index 536deea8c622..9d9cd88dd17e 100644 >> --- a/include/hw/ppc/xive.h >> +++ b/include/hw/ppc/xive.h >> @@ -462,7 +462,6 @@ typedef struct XiveENDSource { >> #define XIVE_TM_OS_PAGE 0x2 >> #define XIVE_TM_USER_PAGE 0x3 >> >> -extern const MemoryRegionOps xive_tm_ops; >> void xive_tctx_tm_write(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset, >> uint64_t value, unsigned size); >> uint64_t xive_tctx_tm_read(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset, >> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c >> index 3d6fcf9ac139..40e18fb44811 100644 >> --- a/hw/intc/pnv_xive.c >> +++ b/hw/intc/pnv_xive.c >> @@ -1475,6 +1475,39 @@ static const MemoryRegionOps xive_tm_indirect_ops = { >> }, >> }; >> >> +static void pnv_xive_tm_write(void *opaque, hwaddr offset, >> + uint64_t value, unsigned size) >> +{ >> + PowerPCCPU *cpu = POWERPC_CPU(current_cpu); >> + PnvXive *xive = pnv_xive_tm_get_xive(cpu); >> + XiveTCTX *tctx = XIVE_TCTX(pnv_cpu_state(cpu)->intc); >> + >> + xive_tctx_tm_write(XIVE_PRESENTER(xive), tctx, offset, value, size); >> +} >> + >> +static uint64_t pnv_xive_tm_read(void *opaque, hwaddr offset, unsigned size) >> +{ >> + PowerPCCPU *cpu = POWERPC_CPU(current_cpu); >> + PnvXive *xive = pnv_xive_tm_get_xive(cpu); >> + XiveTCTX *tctx = XIVE_TCTX(pnv_cpu_state(cpu)->intc); >> + >> + return xive_tctx_tm_read(XIVE_PRESENTER(xive), tctx, offset, size); > > You're not using the opaque pointer in either of these cases. So > couldn't you make it point to the presenter for pnv as well, then... On PowerNV, it's the cpu doing the TIMA load/store which determines the chip and the XiveTCTX is deduced from the pnv_cpu_state(). The TIMA is only mapped on chip0 in our model. See CQ_TM1_BAR reg. > >> +} >> + >> +const MemoryRegionOps pnv_xive_tm_ops = { >> + .read = pnv_xive_tm_read, >> + .write = pnv_xive_tm_write, >> + .endianness = DEVICE_BIG_ENDIAN, >> + .valid = { >> + .min_access_size = 1, >> + .max_access_size = 8, >> + }, >> + .impl = { >> + .min_access_size = 1, >> + .max_access_size = 8, >> + }, >> +}; >> + >> /* >> * Interrupt controller XSCOM region. >> */ >> @@ -1832,7 +1865,7 @@ static void pnv_xive_realize(DeviceState *dev, Error **errp) >> "xive-pc", PNV9_XIVE_PC_SIZE); >> >> /* Thread Interrupt Management Area (Direct) */ >> - memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops, >> + memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &pnv_xive_tm_ops, >> xive, "xive-tima", PNV9_XIVE_TM_SIZE); >> >> qemu_register_reset(pnv_xive_reset, dev); >> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c >> index eefc0d4c36b9..e00a9bdd901b 100644 >> --- a/hw/intc/spapr_xive.c >> +++ b/hw/intc/spapr_xive.c >> @@ -222,6 +222,35 @@ void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx) >> memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &nvt_cam, 4); >> } >> >> +static void spapr_xive_tm_write(void *opaque, hwaddr offset, >> + uint64_t value, unsigned size) >> +{ >> + XiveTCTX *tctx = spapr_cpu_state(POWERPC_CPU(current_cpu))->tctx; >> + >> + xive_tctx_tm_write(XIVE_PRESENTER(opaque), tctx, offset, value, >> size); > > ... there would be no difference from the spapr versions, AFAICT. the XiveTCTX is deduced from the spapr_cpu_state(). C. > >> +} >> + >> +static uint64_t spapr_xive_tm_read(void *opaque, hwaddr offset, unsigned size) >> +{ >> + XiveTCTX *tctx = spapr_cpu_state(POWERPC_CPU(current_cpu))->tctx; >> + >> + return xive_tctx_tm_read(XIVE_PRESENTER(opaque), tctx, offset, size); >> +} >> + >> +const MemoryRegionOps spapr_xive_tm_ops = { >> + .read = spapr_xive_tm_read, >> + .write = spapr_xive_tm_write, >> + .endianness = DEVICE_BIG_ENDIAN, >> + .valid = { >> + .min_access_size = 1, >> + .max_access_size = 8, >> + }, >> + .impl = { >> + .min_access_size = 1, >> + .max_access_size = 8, >> + }, >> +}; >> + >> static void spapr_xive_end_reset(XiveEND *end) >> { >> memset(end, 0, sizeof(*end)); >> @@ -331,8 +360,8 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) >> qemu_register_reset(spapr_xive_reset, dev); >> >> /* TIMA initialization */ >> - memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops, xive, >> - "xive.tima", 4ull << TM_SHIFT); >> + memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &spapr_xive_tm_ops, >> + xive, "xive.tima", 4ull << TM_SHIFT); >> sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xive->tm_mmio); >> >> /* >> diff --git a/hw/intc/xive.c b/hw/intc/xive.c >> index 9bb09ed6ee7b..11432f04f5c3 100644 >> --- a/hw/intc/xive.c >> +++ b/hw/intc/xive.c >> @@ -483,35 +483,6 @@ uint64_t xive_tctx_tm_read(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset, >> return xive_tm_raw_read(tctx, offset, size); >> } >> >> -static void xive_tm_write(void *opaque, hwaddr offset, >> - uint64_t value, unsigned size) >> -{ >> - XiveTCTX *tctx = xive_router_get_tctx(XIVE_ROUTER(opaque), current_cpu); >> - >> - xive_tctx_tm_write(XIVE_PRESENTER(opaque), tctx, offset, value, size); >> -} >> - >> -static uint64_t xive_tm_read(void *opaque, hwaddr offset, unsigned size) >> -{ >> - XiveTCTX *tctx = xive_router_get_tctx(XIVE_ROUTER(opaque), current_cpu); >> - >> - return xive_tctx_tm_read(XIVE_PRESENTER(opaque), tctx, offset, size); >> -} >> - >> -const MemoryRegionOps xive_tm_ops = { >> - .read = xive_tm_read, >> - .write = xive_tm_write, >> - .endianness = DEVICE_BIG_ENDIAN, >> - .valid = { >> - .min_access_size = 1, >> - .max_access_size = 8, >> - }, >> - .impl = { >> - .min_access_size = 1, >> - .max_access_size = 8, >> - }, >> -}; >> - >> static char *xive_tctx_ring_print(uint8_t *ring) >> { >> uint32_t w2 = xive_tctx_word2(ring); >
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h index 536deea8c622..9d9cd88dd17e 100644 --- a/include/hw/ppc/xive.h +++ b/include/hw/ppc/xive.h @@ -462,7 +462,6 @@ typedef struct XiveENDSource { #define XIVE_TM_OS_PAGE 0x2 #define XIVE_TM_USER_PAGE 0x3 -extern const MemoryRegionOps xive_tm_ops; void xive_tctx_tm_write(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset, uint64_t value, unsigned size); uint64_t xive_tctx_tm_read(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset, diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c index 3d6fcf9ac139..40e18fb44811 100644 --- a/hw/intc/pnv_xive.c +++ b/hw/intc/pnv_xive.c @@ -1475,6 +1475,39 @@ static const MemoryRegionOps xive_tm_indirect_ops = { }, }; +static void pnv_xive_tm_write(void *opaque, hwaddr offset, + uint64_t value, unsigned size) +{ + PowerPCCPU *cpu = POWERPC_CPU(current_cpu); + PnvXive *xive = pnv_xive_tm_get_xive(cpu); + XiveTCTX *tctx = XIVE_TCTX(pnv_cpu_state(cpu)->intc); + + xive_tctx_tm_write(XIVE_PRESENTER(xive), tctx, offset, value, size); +} + +static uint64_t pnv_xive_tm_read(void *opaque, hwaddr offset, unsigned size) +{ + PowerPCCPU *cpu = POWERPC_CPU(current_cpu); + PnvXive *xive = pnv_xive_tm_get_xive(cpu); + XiveTCTX *tctx = XIVE_TCTX(pnv_cpu_state(cpu)->intc); + + return xive_tctx_tm_read(XIVE_PRESENTER(xive), tctx, offset, size); +} + +const MemoryRegionOps pnv_xive_tm_ops = { + .read = pnv_xive_tm_read, + .write = pnv_xive_tm_write, + .endianness = DEVICE_BIG_ENDIAN, + .valid = { + .min_access_size = 1, + .max_access_size = 8, + }, + .impl = { + .min_access_size = 1, + .max_access_size = 8, + }, +}; + /* * Interrupt controller XSCOM region. */ @@ -1832,7 +1865,7 @@ static void pnv_xive_realize(DeviceState *dev, Error **errp) "xive-pc", PNV9_XIVE_PC_SIZE); /* Thread Interrupt Management Area (Direct) */ - memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops, + memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &pnv_xive_tm_ops, xive, "xive-tima", PNV9_XIVE_TM_SIZE); qemu_register_reset(pnv_xive_reset, dev); diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c index eefc0d4c36b9..e00a9bdd901b 100644 --- a/hw/intc/spapr_xive.c +++ b/hw/intc/spapr_xive.c @@ -222,6 +222,35 @@ void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx) memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &nvt_cam, 4); } +static void spapr_xive_tm_write(void *opaque, hwaddr offset, + uint64_t value, unsigned size) +{ + XiveTCTX *tctx = spapr_cpu_state(POWERPC_CPU(current_cpu))->tctx; + + xive_tctx_tm_write(XIVE_PRESENTER(opaque), tctx, offset, value, size); +} + +static uint64_t spapr_xive_tm_read(void *opaque, hwaddr offset, unsigned size) +{ + XiveTCTX *tctx = spapr_cpu_state(POWERPC_CPU(current_cpu))->tctx; + + return xive_tctx_tm_read(XIVE_PRESENTER(opaque), tctx, offset, size); +} + +const MemoryRegionOps spapr_xive_tm_ops = { + .read = spapr_xive_tm_read, + .write = spapr_xive_tm_write, + .endianness = DEVICE_BIG_ENDIAN, + .valid = { + .min_access_size = 1, + .max_access_size = 8, + }, + .impl = { + .min_access_size = 1, + .max_access_size = 8, + }, +}; + static void spapr_xive_end_reset(XiveEND *end) { memset(end, 0, sizeof(*end)); @@ -331,8 +360,8 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) qemu_register_reset(spapr_xive_reset, dev); /* TIMA initialization */ - memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops, xive, - "xive.tima", 4ull << TM_SHIFT); + memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &spapr_xive_tm_ops, + xive, "xive.tima", 4ull << TM_SHIFT); sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xive->tm_mmio); /* diff --git a/hw/intc/xive.c b/hw/intc/xive.c index 9bb09ed6ee7b..11432f04f5c3 100644 --- a/hw/intc/xive.c +++ b/hw/intc/xive.c @@ -483,35 +483,6 @@ uint64_t xive_tctx_tm_read(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset, return xive_tm_raw_read(tctx, offset, size); } -static void xive_tm_write(void *opaque, hwaddr offset, - uint64_t value, unsigned size) -{ - XiveTCTX *tctx = xive_router_get_tctx(XIVE_ROUTER(opaque), current_cpu); - - xive_tctx_tm_write(XIVE_PRESENTER(opaque), tctx, offset, value, size); -} - -static uint64_t xive_tm_read(void *opaque, hwaddr offset, unsigned size) -{ - XiveTCTX *tctx = xive_router_get_tctx(XIVE_ROUTER(opaque), current_cpu); - - return xive_tctx_tm_read(XIVE_PRESENTER(opaque), tctx, offset, size); -} - -const MemoryRegionOps xive_tm_ops = { - .read = xive_tm_read, - .write = xive_tm_write, - .endianness = DEVICE_BIG_ENDIAN, - .valid = { - .min_access_size = 1, - .max_access_size = 8, - }, - .impl = { - .min_access_size = 1, - .max_access_size = 8, - }, -}; - static char *xive_tctx_ring_print(uint8_t *ring) { uint32_t w2 = xive_tctx_word2(ring);
This also removes the need of the get_tctx() XiveRouter handler in the core XIVE framework. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- include/hw/ppc/xive.h | 1 - hw/intc/pnv_xive.c | 35 ++++++++++++++++++++++++++++++++++- hw/intc/spapr_xive.c | 33 +++++++++++++++++++++++++++++++-- hw/intc/xive.c | 29 ----------------------------- 4 files changed, 65 insertions(+), 33 deletions(-)