On 18/10/2019 10:07, Greg Kurz wrote:
> On Thu, 17 Oct 2019 16:42:41 +0200
> Cédric Le Goater <c...@kaod.org> wrote:
>
>> When a Virtual Processor is scheduled to run on a HW thread, the
>> hypervisor pushes its identifier in the OS CAM line. When running in
>> TCG or kernel_irqchip=off, QEMU needs to emulate the same behavior.
>>
>
> This is only related to kernel_irqchip=off, which is always the case
> when running in TCG actually. Maybe rephrase to "When not running with
> an in-kernel irqchip, QEMU needs..." ?
yes.
>> Introduce a 'os-cam' property which will be used to set the OS CAM
>> line at reset and remove the spapr_xive_set_tctx_os_cam() calls which
>> are done when the XIVE interrupt controller are activated.
>>
>
> Since OS CAM is constant, I guess it is ok to make it a property.
> Alternatively, you could pass it as an extra parameter to
> xive_tctx_reset().
indeed. We have all we need to do that. I will wait for some feedback.
>> This change also has the benefit to remove the use of CPU_FOREACH()
>> which can be unsafe.
>>
>
> Nice !
>
>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>> ---
>> include/hw/ppc/spapr_xive.h | 1 -
>> include/hw/ppc/xive.h | 4 +++-
>> hw/intc/spapr_xive.c | 31 +++++--------------------------
>> hw/intc/xive.c | 22 +++++++++++++++++++++-
>> hw/ppc/pnv.c | 3 ++-
>> 5 files changed, 31 insertions(+), 30 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
>> index d84bd5c229f0..742b7e834f2a 100644
>> --- a/include/hw/ppc/spapr_xive.h
>> +++ b/include/hw/ppc/spapr_xive.h
>> @@ -57,7 +57,6 @@ typedef struct SpaprXive {
>> void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon);
>>
>> void spapr_xive_hcall_init(SpaprMachineState *spapr);
>> -void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx);
>> void spapr_xive_mmio_set_enabled(SpaprXive *xive, bool enable);
>> void spapr_xive_map_mmio(SpaprXive *xive);
>>
>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index 99381639f50c..e273069c25a9 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -319,6 +319,7 @@ typedef struct XiveTCTX {
>> qemu_irq os_output;
>>
>> uint8_t regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE];
>> + uint32_t os_cam;
>> } XiveTCTX;
>>
>> /*
>> @@ -414,7 +415,8 @@ void xive_tctx_tm_write(XiveTCTX *tctx, hwaddr offset,
>> uint64_t value,
>> uint64_t xive_tctx_tm_read(XiveTCTX *tctx, hwaddr offset, unsigned size);
>>
>> void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
>> -Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp);
>> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
>> + Error **errp);
>> void xive_tctx_reset(XiveTCTX *tctx);
>>
>> static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>> index 0c3acf1a4192..71f138512a1c 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -205,21 +205,13 @@ void spapr_xive_mmio_set_enabled(SpaprXive *xive, bool
>> enable)
>> memory_region_set_enabled(&xive->end_source.esb_mmio, false);
>> }
>>
>> -/*
>> - * When a Virtual Processor is scheduled to run on a HW thread, the
>> - * hypervisor pushes its identifier in the OS CAM line. Emulate the
>> - * same behavior under QEMU.
>> - */
>> -void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx)
>> +static uint32_t spapr_xive_get_os_cam(PowerPCCPU *cpu)
>> {
>> uint8_t nvt_blk;
>> uint32_t nvt_idx;
>> - uint32_t nvt_cam;
>> -
>> - spapr_xive_cpu_to_nvt(POWERPC_CPU(tctx->cs), &nvt_blk, &nvt_idx);
>>
>> - nvt_cam = cpu_to_be32(TM_QW1W2_VO | xive_nvt_cam_line(nvt_blk,
>> nvt_idx));
>> - memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &nvt_cam, 4);
>> + spapr_xive_cpu_to_nvt(cpu, &nvt_blk, &nvt_idx);
>> + return xive_nvt_cam_line(nvt_blk, nvt_idx);
>> }
>>
>> static void spapr_xive_end_reset(XiveEND *end)
>> @@ -537,19 +529,14 @@ static int
>> spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>> SpaprXive *xive = SPAPR_XIVE(intc);
>> Object *obj;
>> SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>> + uint32_t os_cam = spapr_xive_get_os_cam(cpu);
>>
>> - obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), errp);
>> + obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), os_cam, errp);
>> if (!obj) {
>> return -1;
>> }
>>
>> spapr_cpu->tctx = XIVE_TCTX(obj);
>> -
>> - /*
>> - * (TCG) Early setting the OS CAM line for hotplugged CPUs as they
>> - * don't beneficiate from the reset of the XIVE IRQ backend
>> - */
>> - spapr_xive_set_tctx_os_cam(spapr_cpu->tctx);
>> return 0;
>> }
>>
>> @@ -650,14 +637,6 @@ static void spapr_xive_dt(SpaprInterruptController
>> *intc, uint32_t nr_servers,
>> static int spapr_xive_activate(SpaprInterruptController *intc, Error **errp)
>> {
>> SpaprXive *xive = SPAPR_XIVE(intc);
>> - CPUState *cs;
>> -
>> - CPU_FOREACH(cs) {
>> - PowerPCCPU *cpu = POWERPC_CPU(cs);
>> -
>> - /* (TCG) Set the OS CAM line of the thread interrupt context. */
>> - spapr_xive_set_tctx_os_cam(spapr_cpu_state(cpu)->tctx);
>> - }
>>
>> if (kvm_enabled()) {
>> int rc = spapr_irq_init_kvm(kvmppc_xive_connect, intc, errp);
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index 0ae3f9b1efe4..be4f2c974178 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -566,6 +566,18 @@ static void xive_tctx_reset_handler(void *dev)
>> ipb_to_pipr(tctx->regs[TM_QW1_OS + TM_IPB]);
>> tctx->regs[TM_QW3_HV_PHYS + TM_PIPR] =
>> ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
>> +
>> + /*
>> + * (TCG) Set the OS CAM line of the thread interrupt context.
>
> As per my remark above, this shouldn't mention TCG but rather
> kernel-irqchip=off.
ok.
Thanks,
C.
>
>> + *
>> + * When a Virtual Processor is scheduled to run on a HW thread,
>> + * the hypervisor pushes its identifier in the OS CAM line.
>> + * Emulate the same behavior under QEMU.
>> + */
>> + if (tctx->os_cam) {
>> + uint32_t qw1w2 = cpu_to_be32(TM_QW1W2_VO | tctx->os_cam);
>> + memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &qw1w2, 4);
>> + }
>> }
>>
>> void xive_tctx_reset(XiveTCTX *tctx)
>> @@ -667,11 +679,17 @@ static const VMStateDescription vmstate_xive_tctx = {
>> },
>> };
>>
>> +static Property xive_tctx_properties[] = {
>> + DEFINE_PROP_UINT32("os-cam", XiveTCTX, os_cam, 0),
>> + DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> static void xive_tctx_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>>
>> dc->desc = "XIVE Interrupt Thread Context";
>> + dc->props = xive_tctx_properties;
>> dc->realize = xive_tctx_realize;
>> dc->unrealize = xive_tctx_unrealize;
>> dc->vmsd = &vmstate_xive_tctx;
>> @@ -689,7 +707,8 @@ static const TypeInfo xive_tctx_info = {
>> .class_init = xive_tctx_class_init,
>> };
>>
>> -Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
>> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
>> + Error **errp)
>> {
>> Error *local_err = NULL;
>> Object *obj;
>> @@ -698,6 +717,7 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr,
>> Error **errp)
>> object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
>> object_unref(obj);
>> object_property_add_const_link(obj, "cpu", cpu, &error_abort);
>> + object_property_set_int(obj, os_cam, "os-cam", &local_err);
>> object_property_set_bool(obj, true, "realized", &local_err);
>> if (local_err) {
>> goto error;
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 7cf64b6d2533..99c06842573e 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -806,7 +806,8 @@ static void pnv_chip_power9_intc_create(PnvChip *chip,
>> PowerPCCPU *cpu,
>> * controller object is initialized afterwards. Hopefully, it's
>> * only used at runtime.
>> */
>> - obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive),
>> &local_err);
>> + obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), 0,
>> + &local_err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> return;
>
