Re: [PATCH v2 6/6] spapr/xive: Introduce a new CAS value for the StoreEOI capability

[Cédric Le Goater]

On 10/6/20 7:39 PM, Greg Kurz wrote:
> On Mon, 5 Oct 2020 18:51:47 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> When the StoreEOI capability is set to "cas", let CAS decide when
>> StoreEOI should be advertised. StoreEOI is safe to use with a P10
>> compat machine because the OS enforces load-after-store ordering but
>> not with P9 compat.
>>
>> The question now is : should we make "cas" the default at the machine
>> level ?
>>
> 
> Hmm, spapr capabilities are knobs for the end user to provide a specific
> and consistent behavior to the guest... so the "let CAS decide depending
> on what the guest asked for" thing looks awkward... 

The guest doesn't ask for StoreEOI. The hypervisor will return StoreEOI
if the kernel is P10 compat and will not if it's P9 to make sure that
it can be migrated to a P9 host.  

> I mean that making
> "cas" the default at the machine level looks like you don't really want
> this to be a capability at all.

It means that you are not forcing a behaviour "off" or "on". It depends
on the guest support, P10 or P9, which is what we want as a default 
behavior to be in sync with pHyp.

> If the user asked for StoreEOI but the guest chose a compat mode that
> can't support it, QEMU should simply exit IMHO
It's not like XICS. A P8 compat can not run on XIVE. A P9 compat can use 
StoreEOI. So it should be considered more like a mitigation. But anyhow, 
the P9 systems currenly shipped never activate StoreEOI. So you will get 
the error.
 
> Or alternatively I guess you can just forget about the spapr capability
> and do all the checks (XIVE interrupt controller, KVM support, guest
> asked for P10) at CAS.

That's how I did in the fisrt patches but then you loose the ability to 
switch it off on P10 and switch on on P9. Which is good to have for tests 
and performance and for possible issues if we ever have to handle a source 
which has different charateristics. In that case, we would switch it off. 

Thanks,

C.


 
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/ppc/spapr.h      |  1 +
>>  include/hw/ppc/spapr_xive.h |  1 +
>>  hw/intc/spapr_xive.c        |  9 +++++++++
>>  hw/ppc/spapr_caps.c         | 21 +++++++++++++++++----
>>  hw/ppc/spapr_hcall.c        |  7 +++++++
>>  5 files changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index b701c14b4e09..17e7d873e8dc 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -87,6 +87,7 @@ typedef enum {
>>  #define SPAPR_CAP_ON                    0x01
>>  
>>  /* Custom Caps */
>> +#define SPAPR_CAP_CAS                   0x02
>>  
>>  /* Generic */
>>  #define SPAPR_CAP_BROKEN                0x00
>> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
>> index 26c8d90d7196..8b8aa586e44f 100644
>> --- a/include/hw/ppc/spapr_xive.h
>> +++ b/include/hw/ppc/spapr_xive.h
>> @@ -75,6 +75,7 @@ void spapr_xive_map_mmio(SpaprXive *xive);
>>  
>>  int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx,
>>                               uint32_t *out_server, uint8_t *out_prio);
>> +void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable);
>>  
>>  /*
>>   * KVM XIVE device helpers
>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>> index 41f2719ff93a..f57a2681dd91 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -1802,3 +1802,12 @@ void spapr_xive_hcall_init(SpaprMachineState *spapr)
>>      spapr_register_hypercall(H_INT_SYNC, h_int_sync);
>>      spapr_register_hypercall(H_INT_RESET, h_int_reset);
>>  }
>> +
>> +void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable)
>> +{
>> +    if (enable) {
>> +        xive->source.esb_flags |= XIVE_SRC_STORE_EOI;
>> +    } else {
>> +        xive->source.esb_flags &= ~XIVE_SRC_STORE_EOI;
>> +    }
>> +}
>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>> index 9251badbdc27..c55e1fccb9bc 100644
>> --- a/hw/ppc/spapr_caps.c
>> +++ b/hw/ppc/spapr_caps.c
>> @@ -524,6 +524,13 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, 
>> uint8_t val,
>>      }
>>  }
>>  
>> +SpaprCapPossible cap_storeeoi_possible = {
>> +    .num = 3,
>> +    .vals = { "off", "on", "cas" },
>> +    .help = "off - no StoreEOI, on - StoreEOI, "
>> +            "cas - negotiated at CAS (POWER10 compat only)",
>> +};
>> +
>>  static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
>>                                 Error **errp)
>>  {
>> @@ -550,6 +557,11 @@ static void cap_storeeoi_apply(SpaprMachineState 
>> *spapr, uint8_t val,
>>              return;
>>          }
>>  
>> +        /* CAS will decide to advertise StoreEOI (P10 compat kernels only) 
>> */
>> +        if (val == SPAPR_CAP_CAS) {
>> +            return;
>> +        }
>> +
>>          /*
>>           * load-after-store ordering is not enforced on POWER9 CPUs
>>           * and StoreEOI can be racy.
>> @@ -671,11 +683,12 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>>      },
>>      [SPAPR_CAP_STOREEOI] = {
>>          .name = "storeeoi",
>> -        .description = "Implements XIVE StoreEOI feature",
>> +        .description = "Implements XIVE StoreEOI feature (off, on, cas)",
>>          .index = SPAPR_CAP_STOREEOI,
>> -        .get = spapr_cap_get_bool,
>> -        .set = spapr_cap_set_bool,
>> -        .type = "bool",
>> +        .get = spapr_cap_get_string,
>> +        .set = spapr_cap_set_string,
>> +        .type = "string",
>> +        .possible = &cap_storeeoi_possible,
>>          .apply = cap_storeeoi_apply,
>>      },
>>  };
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 607740150fa2..158b122b9192 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1804,6 +1804,13 @@ target_ulong 
>> do_client_architecture_support(PowerPCCPU *cpu,
>>  "Guest requested unavailable interrupt mode (XIVE), try the ic-mode=xive or 
>> ic-mode=dual machine property");
>>              exit(EXIT_FAILURE);
>>          }
>> +
>> +        /* Advertise StoreEOI for a P10 compat guest. */
>> +        if (spapr_get_cap(spapr, SPAPR_CAP_STOREEOI) == SPAPR_CAP_CAS) {
>> +            bool enable = ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_10, 0,
>> +                                           cpu->compat_pvr);
>> +            spapr_xive_enable_store_eoi(spapr->xive, enable);
>> +        }
>>      } else {
>>          if (!spapr->irq->xics) {
>>              error_report(
>