Re: [Qemu-devel] [RFC PATCH v2 04/21] ppc/xive: provide a link to the sPAPR ICS object under XIVE

[Cédric Le Goater]

On 09/19/2017 04:44 AM, David Gibson wrote:
> On Mon, Sep 11, 2017 at 07:12:18PM +0200, Cédric Le Goater wrote:
>> The sPAPR machine first starts with a XICS interrupt model and
>> depending on the guest capabilities, the XIVE exploitation mode is
>> negotiated during CAS. A reset should then be performed to rebuild the
>> device tree but the same IRQ numbers which were allocated by the
>> devices prior to reset, when the XICS model was operating, are still
>> in use.
>>
>> For this purpose, we need a common IRQ number allocator for both the
>> interrupt models: XICS legacy or XIVE exploitation. This is what the
>> ICSIRQState array of the XICS interrupt source is used for. It also
>> contains the LSI/MSI flag of an interrupt which will we need later on.
>>
>> So, let's provide a link to the sPAPR ICS object under XIVE to make
>> use of it.
> 
> Blech, please don't.  The XIVE code absolutely shouldn't be
> referencing XICS objects, it's a recipe for trouble down the line.

Trouble I don't know. But it is a bit ugly and this is why this 
patchset is still an RFC.  

> If we have to have some sort of abstract "spapr interrupt source"
> object that could map to either an ICS irq, or a XIVE source then we
> can do that, but don't directly link XIVE and XICS.  *Especially* not
> new-in-terms-of-old like this, rather than old-in-terms-of-new.

I agree with what you are saying on a common interrupt source but 
I just haven't found a way to do so yet. So I am trying to corner 
the ugliness in obvious shortcuts. The purpose is to identify what
we need to support migration, hotplug, cas reset, etc.

So the current solution is practical to support CAS reset and more 
important it does not break migration. We can't move an object or 
parts of an object around without breaking the migration as my 
recent changes on the xics icp have shown.  

>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  hw/intc/spapr_xive.c        | 12 ++++++++++++
>>  include/hw/ppc/spapr_xive.h |  4 ++++
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>> index 6d98528fae68..1681affb0848 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -56,6 +56,8 @@ void spapr_xive_reset(void *dev)
>>  static void spapr_xive_realize(DeviceState *dev, Error **errp)
>>  {
>>      sPAPRXive *xive = SPAPR_XIVE(dev);
>> +    Object *obj;
>> +    Error *err = NULL;
>>  
>>      if (!xive->nr_targets) {
>>          error_setg(errp, "Number of interrupt targets needs to be greater 
>> 0");
>> @@ -68,6 +70,16 @@ static void spapr_xive_realize(DeviceState *dev, Error 
>> **errp)
>>          return;
>>      }
>>  
>> +    /* Retrieve SPAPR ICS source to share the IRQ number allocator */
> 
> This really suggests we need to move the irq number allocator out of
> XICS and into the general spapr code.  Or get rid of it entirely
> (using a more static irq mapping) if possible.

I have some out of tree changes introducing a bitmap at the spapr
machine level. It is a nice cleanup of the spapr_ics_free() and 
spapr_ics_alloc*() routines. But it is not migration friendly and 
we still need to keep the ICSIRQState array, which also stores the 
IRQ state LSI/MSI. I need to check if such a change would bring
some benefits for XIVE.

C.

>> +    obj = object_property_get_link(OBJECT(dev), "ics", &err);
>> +    if (!obj) {
>> +        error_setg(errp, "%s: required link 'ics' not found: %s",
>> +                   __func__, error_get_pretty(err));
>> +        return;
>> +    }
>> +
>> +    xive->ics = ICS_BASE(obj);
>> +
>>      /* Allocate SBEs (State Bit Entry). 2 bits, so 4 entries per byte */
>>      xive->sbe_size = DIV_ROUND_UP(xive->nr_irqs, 4);
>>      xive->sbe = g_malloc0(xive->sbe_size);
>> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
>> index b17dd4f17b0b..29112589b37f 100644
>> --- a/include/hw/ppc/spapr_xive.h
>> +++ b/include/hw/ppc/spapr_xive.h
>> @@ -24,6 +24,7 @@
>>  typedef struct sPAPRXive sPAPRXive;
>>  typedef struct XiveIVE XiveIVE;
>>  typedef struct XiveEQ XiveEQ;
>> +typedef struct ICSState ICSState;
>>  
>>  #define TYPE_SPAPR_XIVE "spapr-xive"
>>  #define SPAPR_XIVE(obj) OBJECT_CHECK(sPAPRXive, (obj), TYPE_SPAPR_XIVE)
>> @@ -35,6 +36,9 @@ struct sPAPRXive {
>>      uint32_t     nr_targets;
>>      uint32_t     nr_irqs;
>>  
>> +    /* IRQ */
>> +    ICSState     *ics;  /* XICS source inherited from the SPAPR machine */
>> +
>>      /* XIVE internal tables */
>>      uint8_t      *sbe;
>>      uint32_t     sbe_size;
>