Re: [PATCH v4 07/25] ppc/spapr: Implement the XiveFabric interface

[Cédric Le Goater]

On 03/10/2019 03:58, David Gibson wrote:
> On Wed, Sep 18, 2019 at 06:06:27PM +0200, Cédric Le Goater wrote:
>> The CAM line matching sequence in the pseries machine does not change
>> much apart from the use of the new QOM interfaces. There is an extra
>> indirection because of the sPAPR IRQ backend of the machine. Only the
>> XIVE backend implements the new 'match_nvt' handler.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  include/hw/ppc/spapr_irq.h |  6 ++++++
>>  hw/ppc/spapr.c             | 34 ++++++++++++++++++++++++++++++++++
>>  hw/ppc/spapr_irq.c         | 25 +++++++++++++++++++++++++
>>  3 files changed, 65 insertions(+)
>>
>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>> index 5db305165ce2..859780efaf95 100644
>> --- a/include/hw/ppc/spapr_irq.h
>> +++ b/include/hw/ppc/spapr_irq.h
>> @@ -31,6 +31,8 @@ int spapr_irq_msi_alloc(SpaprMachineState *spapr, uint32_t 
>> num, bool align,
>>                          Error **errp);
>>  void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num);
>>  
>> +struct XiveTCTXMatch;
>> +
>>  typedef struct SpaprIrq {
>>      uint32_t    nr_irqs;
>>      uint32_t    nr_msis;
>> @@ -50,6 +52,10 @@ typedef struct SpaprIrq {
>>      void (*set_irq)(void *opaque, int srcno, int val);
>>      const char *(*get_nodename)(SpaprMachineState *spapr);
>>      void (*init_kvm)(SpaprMachineState *spapr, Error **errp);
>> +    int (*match_nvt)(SpaprMachineState *spapr, uint8_t format,
>> +                     uint8_t nvt_blk, uint32_t nvt_idx,
>> +                     bool cam_ignore, uint8_t priority,
>> +                     uint32_t logic_serv, struct XiveTCTXMatch *match);
> 
> Obviously this will need some rework against my stuff.
> 
> But more importantly, I don't see the point of indirecting via here,
> when the method is only relevant for the xive case.  Why not just
> assert that XIVE is in use in the XiveFabric hook, and go directly  to
> the XIVE matching code.

Yes you might be right. I had to introduce this helper to cover dual 
and xive mode for some reason. I will rebase when your code is merged and 
see how things fit together. I am not worried.

C.


> 
>>  } SpaprIrq;
>>  
>>  extern SpaprIrq spapr_irq_xics;
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 2725b139a7f0..90f6f5fb9536 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -4360,6 +4360,37 @@ static void 
>> spapr_pic_print_info(InterruptStatsProvider *obj,
>>                     kvm_irqchip_in_kernel() ? "in-kernel" : "emulated");
>>  }
>>  
>> +static int spapr_xive_match_nvt(XiveFabric *xfb, uint8_t format,
>> +                                uint8_t nvt_blk, uint32_t nvt_idx,
>> +                                bool cam_ignore, uint8_t priority,
>> +                                uint32_t logic_serv, XiveTCTXMatch *match)
>> +{
>> +    SpaprMachineState *spapr = SPAPR_MACHINE(xfb);
>> +    int count;
>> +
>> +    count = spapr->irq->match_nvt(spapr, format, nvt_blk, nvt_idx, 
>> cam_ignore,
>> +                                  priority, logic_serv, match);
>> +    if (count < 0) {
>> +        return count;
>> +    }
>> +
>> +    /*
>> +     * When we implement the save and restore of the thread interrupt
>> +     * contexts in the enter/exit CPU handlers of the machine and the
>> +     * escalations in QEMU, we should be able to handle non dispatched
>> +     * vCPUs.
>> +     *
>> +     * Until this is done, the sPAPR machine should find at least one
>> +     * matching context always.
>> +     */
>> +    if (count == 0) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: NVT %x/%x is not 
>> dispatched\n",
>> +                      nvt_blk, nvt_idx);
>> +    }
>> +
>> +    return count;
>> +}
>> +
>>  int spapr_get_vcpu_id(PowerPCCPU *cpu)
>>  {
>>      return cpu->vcpu_id;
>> @@ -4456,6 +4487,7 @@ static void spapr_machine_class_init(ObjectClass *oc, 
>> void *data)
>>      PPCVirtualHypervisorClass *vhc = PPC_VIRTUAL_HYPERVISOR_CLASS(oc);
>>      XICSFabricClass *xic = XICS_FABRIC_CLASS(oc);
>>      InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
>> +    XiveFabricClass *xfc = XIVE_FABRIC_CLASS(oc);
>>  
>>      mc->desc = "pSeries Logical Partition (PAPR compliant)";
>>      mc->ignore_boot_device_suffixes = true;
>> @@ -4514,6 +4546,7 @@ static void spapr_machine_class_init(ObjectClass *oc, 
>> void *data)
>>       */
>>      mc->numa_mem_align_shift = 28;
>>      mc->numa_mem_supported = true;
>> +    xfc->match_nvt = spapr_xive_match_nvt;
>>  
>>      smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
>>      smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON;
>> @@ -4547,6 +4580,7 @@ static const TypeInfo spapr_machine_info = {
>>          { TYPE_PPC_VIRTUAL_HYPERVISOR },
>>          { TYPE_XICS_FABRIC },
>>          { TYPE_INTERRUPT_STATS_PROVIDER },
>> +        { TYPE_XIVE_FABRIC },
>>          { }
>>      },
>>  };
>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index d8f46b6797f8..8a6d79a59af2 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -257,6 +257,7 @@ SpaprIrq spapr_irq_xics = {
>>      .set_irq     = spapr_irq_set_irq_xics,
>>      .get_nodename = spapr_irq_get_nodename_xics,
>>      .init_kvm    = spapr_irq_init_kvm_xics,
>> +    .match_nvt   = NULL, /* should not be used */
>>  };
>>  
>>  /*
>> @@ -406,6 +407,18 @@ static void spapr_irq_init_kvm_xive(SpaprMachineState 
>> *spapr, Error **errp)
>>      }
>>  }
>>  
>> +static int spapr_irq_match_nvt_xive(SpaprMachineState *spapr, uint8_t 
>> format,
>> +                                    uint8_t nvt_blk, uint32_t nvt_idx,
>> +                                    bool cam_ignore, uint8_t priority,
>> +                                    uint32_t logic_serv, XiveTCTXMatch 
>> *match)
>> +{
>> +    XivePresenter *xptr = XIVE_PRESENTER(spapr->xive);
>> +    XivePresenterClass *xpc = XIVE_PRESENTER_GET_CLASS(xptr);
>> +
>> +    return xpc->match_nvt(xptr, format, nvt_blk, nvt_idx, cam_ignore,
>> +                          priority, logic_serv, match);
>> +}
>> +
>>  /*
>>   * XIVE uses the full IRQ number space. Set it to 8K to be compatible
>>   * with XICS.
>> @@ -431,6 +444,7 @@ SpaprIrq spapr_irq_xive = {
>>      .set_irq     = spapr_irq_set_irq_xive,
>>      .get_nodename = spapr_irq_get_nodename_xive,
>>      .init_kvm    = spapr_irq_init_kvm_xive,
>> +    .match_nvt   = spapr_irq_match_nvt_xive,
>>  };
>>  
>>  /*
>> @@ -585,6 +599,15 @@ static const char 
>> *spapr_irq_get_nodename_dual(SpaprMachineState *spapr)
>>      return spapr_irq_current(spapr)->get_nodename(spapr);
>>  }
>>  
>> +static int spapr_irq_match_nvt_dual(SpaprMachineState *spapr, uint8_t 
>> format,
>> +                                    uint8_t nvt_blk, uint32_t nvt_idx,
>> +                                    bool cam_ignore, uint8_t priority,
>> +                                    uint32_t logic_serv, XiveTCTXMatch 
>> *match)
>> +{
>> +    return spapr_irq_current(spapr)->match_nvt(spapr, format, nvt_blk, 
>> nvt_idx,
>> +                                     cam_ignore, priority, logic_serv, 
>> match);
>> +}
>> +
>>  /*
>>   * Define values in sync with the XIVE and XICS backend
>>   */
>> @@ -608,6 +631,7 @@ SpaprIrq spapr_irq_dual = {
>>      .set_irq     = spapr_irq_set_irq_dual,
>>      .get_nodename = spapr_irq_get_nodename_dual,
>>      .init_kvm    = NULL, /* should not be used */
>> +    .match_nvt   = spapr_irq_match_nvt_dual,
>>  };
>>  
>>  
>> @@ -825,4 +849,5 @@ SpaprIrq spapr_irq_xics_legacy = {
>>      .set_irq     = spapr_irq_set_irq_xics,
>>      .get_nodename = spapr_irq_get_nodename_xics,
>>      .init_kvm    = spapr_irq_init_kvm_xics,
>> +    .match_nvt   = NULL, /* should not be used */
>>  };
>