On Wed, 22 Sep 2021 16:41:20 +0200
Cédric Le Goater <c...@kaod.org> wrote:
> When QEMU switches to the XIVE interrupt mode, it creates all possible
> guest interrupts at the level of the KVM device. These interrupts are
> backed by real HW interrupts from the IPI interrupt pool of the XIVE
> controller.
>
> Currently, this is done from the QEMU main thread, which results in
> allocating all interrupts from the chip on which QEMU is running. IPIs
> are not distributed across the system and the load is not well
> balanced across the interrupt controllers.
>
> To improve distribution on the system, we should try to allocate the
> underlying HW IPI from the vCPU context. This also benefits to
> configurations using CPU pinning. In this case, the HW IPI is
> allocated on the local chip, rerouting between interrupt controllers
> is reduced and performance improved.
>
> This moves the initialization of the vCPU IPIs from reset time to the
> H_INT_SET_SOURCE_CONFIG hcall which is called from the vCPU context.
> But this needs some extra checks in the sequences getting and setting
> the source states to make sure a valid HW IPI is backing the guest
> interrupt. For that, we check if a target was configured in the END in
> case of a vCPU IPI.
>
> Signed-off-by: Cédric Le Goater <c...@kaod.org>
> ---
>
> I have tested different SMT configurations, kernel_irqchip=off/on,
> did some migrations, CPU hotplug, etc. It's not enough and I would
> like more testing but, at least, it is not making anymore the bad
> assumption vCPU id = IPI number.
>
Yeah, the IPI number is provided by the guest, so h_int_set_source_config()
is really the only place where we can know the IPI number of a given vCPU.
> Comments ?
>
LGTM but I didn't check if more users of xive_end_is_valid() should
be converted to using xive_source_is_initialized().
Any chance you have some perf numbers to share ?
> hw/intc/spapr_xive.c | 17 +++++++++++++++++
> hw/intc/spapr_xive_kvm.c | 36 +++++++++++++++++++++++++++++++-----
> 2 files changed, 48 insertions(+), 5 deletions(-)
>
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 6f31ce74f198..2dc594a720b1 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -1089,6 +1089,23 @@ static target_ulong h_int_set_source_config(PowerPCCPU
> *cpu,
> if (spapr_xive_in_kernel(xive)) {
> Error *local_err = NULL;
>
> + /*
> + * Initialize the vCPU IPIs from the vCPU context to allocate
> + * the backing HW IPI on the local chip. This improves
> + * distribution of the IPIs in the system and when the vCPUs
> + * are pinned, it reduces rerouting between interrupt
> + * controllers for better performance.
> + */
> + if (lisn < SPAPR_XIRQ_BASE) {
> + XiveSource *xsrc = &xive->source;
> +
> + kvmppc_xive_source_reset_one(xsrc, lisn, &local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + return H_HARDWARE;
> + }
> + }
> +
> kvmppc_xive_set_source_config(xive, lisn, &new_eas, &local_err);
> if (local_err) {
> error_report_err(local_err);
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 53731d158625..1607a59e9483 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -254,7 +254,12 @@ static int kvmppc_xive_source_reset(XiveSource *xsrc,
> Error **errp)
> SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
> int i;
>
> - for (i = 0; i < xsrc->nr_irqs; i++) {
> + /*
> + * vCPU IPIs are initialized at the KVM level when configured by
> + * H_INT_SET_SOURCE_CONFIG.
> + */
> +
> + for (i = SPAPR_XIRQ_BASE; i < xsrc->nr_irqs; i++) {
> int ret;
>
> if (!xive_eas_is_valid(&xive->eat[i])) {
> @@ -342,6 +347,27 @@ uint64_t kvmppc_xive_esb_rw(XiveSource *xsrc, int srcno,
> uint32_t offset,
> }
> }
>
> +static bool xive_source_is_initialized(SpaprXive *xive, int lisn)
> +{
> + assert(lisn < xive->nr_irqs);
> +
> + if (!xive_eas_is_valid(&xive->eat[lisn])) {
> + return false;
> + }
> +
> + /*
> + * vCPU IPIs are initialized at the KVM level when configured by
> + * H_INT_SET_SOURCE_CONFIG, in which case, we should have a valid
> + * target (server, priority) in the END.
> + */
> + if (lisn < SPAPR_XIRQ_BASE) {
> + return !!xive_get_field64(EAS_END_INDEX, xive->eat[lisn].w);
> + }
> +
> + /* Device sources */
> + return true;
> +}
> +
> static void kvmppc_xive_source_get_state(XiveSource *xsrc)
> {
> SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
> @@ -350,7 +376,7 @@ static void kvmppc_xive_source_get_state(XiveSource *xsrc)
> for (i = 0; i < xsrc->nr_irqs; i++) {
> uint8_t pq;
>
> - if (!xive_eas_is_valid(&xive->eat[i])) {
> + if (!xive_source_is_initialized(xive, i)) {
> continue;
> }
>
> @@ -533,7 +559,7 @@ static void kvmppc_xive_change_state_handler(void
> *opaque, bool running,
> uint8_t pq;
> uint8_t old_pq;
>
> - if (!xive_eas_is_valid(&xive->eat[i])) {
> + if (!xive_source_is_initialized(xive, i)) {
> continue;
> }
>
> @@ -561,7 +587,7 @@ static void kvmppc_xive_change_state_handler(void
> *opaque, bool running,
> for (i = 0; i < xsrc->nr_irqs; i++) {
> uint8_t pq;
>
> - if (!xive_eas_is_valid(&xive->eat[i])) {
> + if (!xive_source_is_initialized(xive, i)) {
> continue;
> }
>
> @@ -666,7 +692,7 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id)
>
> /* Restore the EAT */
> for (i = 0; i < xive->nr_irqs; i++) {
> - if (!xive_eas_is_valid(&xive->eat[i])) {
> + if (!xive_source_is_initialized(xive, i)) {
> continue;
> }
>
