Re: [PATCH 19/20] spapr: Use less cryptic representation of which irq backends are supported

[Greg Kurz <groug@kaod.org>]

On Wed, 25 Sep 2019 16:45:33 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> SpaprIrq::ov5 stores the value for a particular byte in PAPR option vector
> 5 which indicates whether XICS, XIVE or both interrupt controllers are
> available.  As usual for PAPR, the encoding is kind of overly complicated
> and confusing (though to be fair there are some backwards compat things it
> has to handle).
> 
> But to make our internal code clearer, have SpaprIrq encode more directly
> which backends are available as two booleans, and derive the OV5 value from
> that at the point we need it.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c             | 15 ++++++++++++---
>  hw/ppc/spapr_hcall.c       |  6 +++---
>  hw/ppc/spapr_irq.c         | 12 ++++++++----
>  include/hw/ppc/spapr_irq.h |  3 ++-
>  4 files changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3742a8cf06..795f6ab505 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1136,19 +1136,28 @@ static void spapr_dt_ov5_platform_support(SpaprMachineState *spapr, void *fdt,
>      PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
>  
>      char val[2 * 4] = {
> -        23, spapr->irq->ov5, /* Xive mode. */
> +        23, 0x00, /* XICS / XIVE mode */
>          24, 0x00, /* Hash/Radix, filled in below. */
>          25, 0x00, /* Hash options: Segment Tables == no, GTSE == no. */
>          26, 0x40, /* Radix options: GTSE == yes. */
>      };
>  
> +    if (spapr->irq->xics && spapr->irq->xive) {
> +        val[1] = SPAPR_OV5_XIVE_BOTH;
> +    } else if (spapr->irq->xive) {
> +        val[1] = SPAPR_OV5_XIVE_EXPLOIT;
> +    } else {
> +        assert(spapr->irq->xics);
> +        val[1] = SPAPR_OV5_XIVE_LEGACY;
> +    }
> +
>      if (!ppc_check_compat(first_ppc_cpu, CPU_POWERPC_LOGICAL_3_00, 0,
>                            first_ppc_cpu->compat_pvr)) {
>          /*
>           * If we're in a pre POWER9 compat mode then the guest should
>           * do hash and use the legacy interrupt mode
>           */
> -        val[1] = 0x00; /* XICS */
> +        val[1] = SPAPR_OV5_XIVE_LEGACY; /* XICS */
>          val[3] = 0x00; /* Hash */
>      } else if (kvm_enabled()) {
>          if (kvmppc_has_cap_mmu_radix() && kvmppc_has_cap_mmu_hash_v3()) {
> @@ -2837,7 +2846,7 @@ static void spapr_machine_init(MachineState *machine)
>      spapr_ovec_set(spapr->ov5, OV5_DRMEM_V2);
>  
>      /* advertise XIVE on POWER9 machines */
> -    if (spapr->irq->ov5 & (SPAPR_OV5_XIVE_EXPLOIT | SPAPR_OV5_XIVE_BOTH)) {
> +    if (spapr->irq->xive) {
>          spapr_ovec_set(spapr->ov5, OV5_XIVE_EXPLOIT);
>      }
>  
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 3d3a67149a..140f05c1c6 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1784,13 +1784,13 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>       * terminate the boot.
>       */
>      if (guest_xive) {
> -        if (spapr->irq->ov5 == SPAPR_OV5_XIVE_LEGACY) {
> +        if (!spapr->irq->xive) {
>              error_report(
>  "Guest requested unavailable interrupt mode (XIVE), try the ic-mode=xive or ic-mode=dual machine property");
>              exit(EXIT_FAILURE);
>          }
>      } else {
> -        if (spapr->irq->ov5 == SPAPR_OV5_XIVE_EXPLOIT) {
> +        if (!spapr->irq->xics) {
>              error_report(
>  "Guest requested unavailable interrupt mode (XICS), either don't set the ic-mode machine property or try ic-mode=xics or ic-mode=dual");
>              exit(EXIT_FAILURE);
> @@ -1804,7 +1804,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>       */
>      if (!spapr->cas_reboot) {
>          spapr->cas_reboot = spapr_ovec_test(ov5_updates, OV5_XIVE_EXPLOIT)
> -            && spapr->irq->ov5 & SPAPR_OV5_XIVE_BOTH;
> +            && spapr->irq->xics && spapr->irq->xive;
>      }
>  
>      spapr_ovec_cleanup(ov5_updates);
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index f53544e45e..073f375ba2 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -209,7 +209,8 @@ static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
>  SpaprIrq spapr_irq_xics = {
>      .nr_xirqs    = SPAPR_NR_XIRQS,
>      .nr_msis     = SPAPR_NR_MSIS,
> -    .ov5         = SPAPR_OV5_XIVE_LEGACY,
> +    .xics        = true,
> +    .xive        = false,
>  
>      .init        = spapr_irq_init_xics,
>      .claim       = spapr_irq_claim_xics,
> @@ -357,7 +358,8 @@ static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp)
>  SpaprIrq spapr_irq_xive = {
>      .nr_xirqs    = SPAPR_NR_XIRQS,
>      .nr_msis     = SPAPR_NR_MSIS,
> -    .ov5         = SPAPR_OV5_XIVE_EXPLOIT,
> +    .xics        = false,
> +    .xive        = true,
>  
>      .init        = spapr_irq_init_xive,
>      .claim       = spapr_irq_claim_xive,
> @@ -515,7 +517,8 @@ static void spapr_irq_set_irq_dual(void *opaque, int irq, int val)
>  SpaprIrq spapr_irq_dual = {
>      .nr_xirqs    = SPAPR_NR_XIRQS,
>      .nr_msis     = SPAPR_NR_MSIS,
> -    .ov5         = SPAPR_OV5_XIVE_BOTH,
> +    .xics        = true,
> +    .xive        = true,
>  
>      .init        = spapr_irq_init_dual,
>      .claim       = spapr_irq_claim_dual,
> @@ -751,7 +754,8 @@ int spapr_irq_find(SpaprMachineState *spapr, int num, bool align, Error **errp)
>  SpaprIrq spapr_irq_xics_legacy = {
>      .nr_xirqs    = SPAPR_IRQ_XICS_LEGACY_NR_XIRQS,
>      .nr_msis     = SPAPR_IRQ_XICS_LEGACY_NR_XIRQS,
> -    .ov5         = SPAPR_OV5_XIVE_LEGACY,
> +    .xics        = true,
> +    .xive        = false,
>  
>      .init        = spapr_irq_init_xics,
>      .claim       = spapr_irq_claim_xics,
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 75279ca137..6816cb0500 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -39,7 +39,8 @@ void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num);
>  typedef struct SpaprIrq {
>      uint32_t    nr_xirqs;
>      uint32_t    nr_msis;
> -    uint8_t     ov5;
> +    bool        xics;
> +    bool        xive;
>  
>      void (*init)(SpaprMachineState *spapr, Error **errp);
>      void (*claim)(SpaprMachineState *spapr, int irq, bool lsi, Error **errp);