Re: [Qemu-devel] [PATCH v3 8/9] vmstate: Add preallocation for migrating arrays (VMS_ALLOC flag)

Re: [Qemu-devel] [PATCH v3 8/9] vmstate: Add preallocation for migrating arrays (VMS_ALLOC flag)

[Alexey Kardashevskiy]

On 05/30/2014 07:34 PM, Alexey Kardashevskiy wrote:
> There are few helpers already to support array migration. However they all
> require the destination side to preallocate arrays before migration which
> is not always possible due to unknown array size as it might be some
> sort of dynamic state. One of the examples is an array of MSIX-enabled
> devices in SPAPR PHB - this array may vary from 0 to 65536 entries and
> its size depends on guest's ability to enable MSIX or do PCI hotplug.
> 
> This adds new VMSTATE_VARRAY_STRUCT_ALLOC macro which is pretty similar to
> VMSTATE_STRUCT_VARRAY_POINTER_INT32 but it can alloc memory for migratign
> array on the destination side.
> 
> This defines VMS_ALLOC flag for a field.
> 
> This changes vmstate_base_addr() to do the allocation when receiving
> migration.



Juan, Peter? No hurry, just pinging in order not to forget :) Thanks!


> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  include/migration/vmstate.h | 11 +++++++++++
>  vmstate.c                   | 13 ++++++++++---
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 7e45048..e794694 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -101,6 +101,7 @@ enum VMStateFlags {
>      VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
>      VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
>      VMS_MUST_EXIST       = 0x1000, /* Field must exist in input */
> +    VMS_ALLOC            = 0x2000, /* Alloc a buffer on the destination */
>  };
>  
>  typedef struct {
> @@ -429,6 +430,16 @@ extern const VMStateInfo vmstate_info_bitmap;
>      .offset     = offsetof(_state, _field),                          \
>  }
>  
> +#define VMSTATE_STRUCT_VARRAY_ALLOC(_field, _state, _field_num, _version, _vmsd, _type) {\
> +    .name       = (stringify(_field)),                               \
> +    .version_id = (_version),                                        \
> +    .vmsd       = &(_vmsd),                                          \
> +    .num_offset = vmstate_offset_value(_state, _field_num, int32_t), \
> +    .size       = sizeof(_type),                                     \
> +    .flags      = VMS_STRUCT|VMS_VARRAY_INT32|VMS_ALLOC|VMS_POINTER, \
> +    .offset     = vmstate_offset_pointer(_state, _field, _type),     \
> +}
> +
>  #define VMSTATE_STATIC_BUFFER(_field, _state, _version, _test, _start, _size) { \
>      .name         = (stringify(_field)),                             \
>      .version_id   = (_version),                                      \
> diff --git a/vmstate.c b/vmstate.c
> index b5882fa..fb95e39 100644
> --- a/vmstate.c
> +++ b/vmstate.c
> @@ -43,11 +43,18 @@ static int vmstate_size(void *opaque, VMStateField *field)
>      return size;
>  }
>  
> -static void *vmstate_base_addr(void *opaque, VMStateField *field)
> +static void *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc)
>  {
>      void *base_addr = opaque + field->offset;
>  
>      if (field->flags & VMS_POINTER) {
> +        if (alloc && (field->flags & VMS_ALLOC)) {
> +            int n_elems = vmstate_n_elems(opaque, field);
> +            if (n_elems) {
> +                *((void **)base_addr + field->start) = g_malloc_n(n_elems,
> +                                                                  field->size);
> +            }
> +        }
>          base_addr = *(void **)base_addr + field->start;
>      }
>  
> @@ -81,7 +88,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>               field->field_exists(opaque, version_id)) ||
>              (!field->field_exists &&
>               field->version_id <= version_id)) {
> -            void *base_addr = vmstate_base_addr(opaque, field);
> +            void *base_addr = vmstate_base_addr(opaque, field, true);
>              int i, n_elems = vmstate_n_elems(opaque, field);
>              int size = vmstate_size(opaque, field);
>  
> @@ -131,7 +138,7 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>      while (field->name) {
>          if (!field->field_exists ||
>              field->field_exists(opaque, vmsd->version_id)) {
> -            void *base_addr = vmstate_base_addr(opaque, field);
> +            void *base_addr = vmstate_base_addr(opaque, field, false);
>              int i, n_elems = vmstate_n_elems(opaque, field);
>              int size = vmstate_size(opaque, field);
>  
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 8/9] vmstate: Add preallocation for migrating arrays (VMS_ALLOC flag)

[Alexey Kardashevskiy]

On 06/03/2014 11:19 PM, Alexey Kardashevskiy wrote:
> On 05/30/2014 07:34 PM, Alexey Kardashevskiy wrote:
>> There are few helpers already to support array migration. However they all
>> require the destination side to preallocate arrays before migration which
>> is not always possible due to unknown array size as it might be some
>> sort of dynamic state. One of the examples is an array of MSIX-enabled
>> devices in SPAPR PHB - this array may vary from 0 to 65536 entries and
>> its size depends on guest's ability to enable MSIX or do PCI hotplug.
>>
>> This adds new VMSTATE_VARRAY_STRUCT_ALLOC macro which is pretty similar to
>> VMSTATE_STRUCT_VARRAY_POINTER_INT32 but it can alloc memory for migratign
>> array on the destination side.
>>
>> This defines VMS_ALLOC flag for a field.
>>
>> This changes vmstate_base_addr() to do the allocation when receiving
>> migration.
> 
> 
> 
> Juan, Peter? No hurry, just pinging in order not to forget :) Thanks!


Hi, anyone? :)



> 
> 
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  include/migration/vmstate.h | 11 +++++++++++
>>  vmstate.c                   | 13 ++++++++++---
>>  2 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index 7e45048..e794694 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -101,6 +101,7 @@ enum VMStateFlags {
>>      VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
>>      VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
>>      VMS_MUST_EXIST       = 0x1000, /* Field must exist in input */
>> +    VMS_ALLOC            = 0x2000, /* Alloc a buffer on the destination */
>>  };
>>  
>>  typedef struct {
>> @@ -429,6 +430,16 @@ extern const VMStateInfo vmstate_info_bitmap;
>>      .offset     = offsetof(_state, _field),                          \
>>  }
>>  
>> +#define VMSTATE_STRUCT_VARRAY_ALLOC(_field, _state, _field_num, _version, _vmsd, _type) {\
>> +    .name       = (stringify(_field)),                               \
>> +    .version_id = (_version),                                        \
>> +    .vmsd       = &(_vmsd),                                          \
>> +    .num_offset = vmstate_offset_value(_state, _field_num, int32_t), \
>> +    .size       = sizeof(_type),                                     \
>> +    .flags      = VMS_STRUCT|VMS_VARRAY_INT32|VMS_ALLOC|VMS_POINTER, \
>> +    .offset     = vmstate_offset_pointer(_state, _field, _type),     \
>> +}
>> +
>>  #define VMSTATE_STATIC_BUFFER(_field, _state, _version, _test, _start, _size) { \
>>      .name         = (stringify(_field)),                             \
>>      .version_id   = (_version),                                      \
>> diff --git a/vmstate.c b/vmstate.c
>> index b5882fa..fb95e39 100644
>> --- a/vmstate.c
>> +++ b/vmstate.c
>> @@ -43,11 +43,18 @@ static int vmstate_size(void *opaque, VMStateField *field)
>>      return size;
>>  }
>>  
>> -static void *vmstate_base_addr(void *opaque, VMStateField *field)
>> +static void *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc)
>>  {
>>      void *base_addr = opaque + field->offset;
>>  
>>      if (field->flags & VMS_POINTER) {
>> +        if (alloc && (field->flags & VMS_ALLOC)) {
>> +            int n_elems = vmstate_n_elems(opaque, field);
>> +            if (n_elems) {
>> +                *((void **)base_addr + field->start) = g_malloc_n(n_elems,
>> +                                                                  field->size);
>> +            }
>> +        }
>>          base_addr = *(void **)base_addr + field->start;
>>      }
>>  
>> @@ -81,7 +88,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>               field->field_exists(opaque, version_id)) ||
>>              (!field->field_exists &&
>>               field->version_id <= version_id)) {
>> -            void *base_addr = vmstate_base_addr(opaque, field);
>> +            void *base_addr = vmstate_base_addr(opaque, field, true);
>>              int i, n_elems = vmstate_n_elems(opaque, field);
>>              int size = vmstate_size(opaque, field);
>>  
>> @@ -131,7 +138,7 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>>      while (field->name) {
>>          if (!field->field_exists ||
>>              field->field_exists(opaque, vmsd->version_id)) {
>> -            void *base_addr = vmstate_base_addr(opaque, field);
>> +            void *base_addr = vmstate_base_addr(opaque, field, false);
>>              int i, n_elems = vmstate_n_elems(opaque, field);
>>              int size = vmstate_size(opaque, field);
>>  
>>
> 
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 8/9] vmstate: Add preallocation for migrating arrays (VMS_ALLOC flag)

[Alexey Kardashevskiy]

On 06/08/2014 09:59 AM, Alexey Kardashevskiy wrote:
> On 06/03/2014 11:19 PM, Alexey Kardashevskiy wrote:
>> On 05/30/2014 07:34 PM, Alexey Kardashevskiy wrote:
>>> There are few helpers already to support array migration. However they all
>>> require the destination side to preallocate arrays before migration which
>>> is not always possible due to unknown array size as it might be some
>>> sort of dynamic state. One of the examples is an array of MSIX-enabled
>>> devices in SPAPR PHB - this array may vary from 0 to 65536 entries and
>>> its size depends on guest's ability to enable MSIX or do PCI hotplug.
>>>
>>> This adds new VMSTATE_VARRAY_STRUCT_ALLOC macro which is pretty similar to
>>> VMSTATE_STRUCT_VARRAY_POINTER_INT32 but it can alloc memory for migratign
>>> array on the destination side.
>>>
>>> This defines VMS_ALLOC flag for a field.
>>>
>>> This changes vmstate_base_addr() to do the allocation when receiving
>>> migration.
>>
>>
>>
>> Juan, Peter? No hurry, just pinging in order not to forget :) Thanks!
> 
> 
> Hi, anyone? :)


Ping?



> 
> 
> 
>>
>>
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>  include/migration/vmstate.h | 11 +++++++++++
>>>  vmstate.c                   | 13 ++++++++++---
>>>  2 files changed, 21 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>>> index 7e45048..e794694 100644
>>> --- a/include/migration/vmstate.h
>>> +++ b/include/migration/vmstate.h
>>> @@ -101,6 +101,7 @@ enum VMStateFlags {
>>>      VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
>>>      VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
>>>      VMS_MUST_EXIST       = 0x1000, /* Field must exist in input */
>>> +    VMS_ALLOC            = 0x2000, /* Alloc a buffer on the destination */
>>>  };
>>>  
>>>  typedef struct {
>>> @@ -429,6 +430,16 @@ extern const VMStateInfo vmstate_info_bitmap;
>>>      .offset     = offsetof(_state, _field),                          \
>>>  }
>>>  
>>> +#define VMSTATE_STRUCT_VARRAY_ALLOC(_field, _state, _field_num, _version, _vmsd, _type) {\
>>> +    .name       = (stringify(_field)),                               \
>>> +    .version_id = (_version),                                        \
>>> +    .vmsd       = &(_vmsd),                                          \
>>> +    .num_offset = vmstate_offset_value(_state, _field_num, int32_t), \
>>> +    .size       = sizeof(_type),                                     \
>>> +    .flags      = VMS_STRUCT|VMS_VARRAY_INT32|VMS_ALLOC|VMS_POINTER, \
>>> +    .offset     = vmstate_offset_pointer(_state, _field, _type),     \
>>> +}
>>> +
>>>  #define VMSTATE_STATIC_BUFFER(_field, _state, _version, _test, _start, _size) { \
>>>      .name         = (stringify(_field)),                             \
>>>      .version_id   = (_version),                                      \
>>> diff --git a/vmstate.c b/vmstate.c
>>> index b5882fa..fb95e39 100644
>>> --- a/vmstate.c
>>> +++ b/vmstate.c
>>> @@ -43,11 +43,18 @@ static int vmstate_size(void *opaque, VMStateField *field)
>>>      return size;
>>>  }
>>>  
>>> -static void *vmstate_base_addr(void *opaque, VMStateField *field)
>>> +static void *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc)
>>>  {
>>>      void *base_addr = opaque + field->offset;
>>>  
>>>      if (field->flags & VMS_POINTER) {
>>> +        if (alloc && (field->flags & VMS_ALLOC)) {
>>> +            int n_elems = vmstate_n_elems(opaque, field);
>>> +            if (n_elems) {
>>> +                *((void **)base_addr + field->start) = g_malloc_n(n_elems,
>>> +                                                                  field->size);
>>> +            }
>>> +        }
>>>          base_addr = *(void **)base_addr + field->start;
>>>      }
>>>  
>>> @@ -81,7 +88,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>               field->field_exists(opaque, version_id)) ||
>>>              (!field->field_exists &&
>>>               field->version_id <= version_id)) {
>>> -            void *base_addr = vmstate_base_addr(opaque, field);
>>> +            void *base_addr = vmstate_base_addr(opaque, field, true);
>>>              int i, n_elems = vmstate_n_elems(opaque, field);
>>>              int size = vmstate_size(opaque, field);
>>>  
>>> @@ -131,7 +138,7 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>      while (field->name) {
>>>          if (!field->field_exists ||
>>>              field->field_exists(opaque, vmsd->version_id)) {
>>> -            void *base_addr = vmstate_base_addr(opaque, field);
>>> +            void *base_addr = vmstate_base_addr(opaque, field, false);
>>>              int i, n_elems = vmstate_n_elems(opaque, field);
>>>              int size = vmstate_size(opaque, field);
>>>  
>>>
>>
>>
> 
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 8/9] vmstate: Add preallocation for migrating arrays (VMS_ALLOC flag)

[Alexander Graf]

On 06/12/2014 05:02 PM, Alexey Kardashevskiy wrote:
> On 06/08/2014 09:59 AM, Alexey Kardashevskiy wrote:
>> On 06/03/2014 11:19 PM, Alexey Kardashevskiy wrote:
>>> On 05/30/2014 07:34 PM, Alexey Kardashevskiy wrote:
>>>> There are few helpers already to support array migration. However they all
>>>> require the destination side to preallocate arrays before migration which
>>>> is not always possible due to unknown array size as it might be some
>>>> sort of dynamic state. One of the examples is an array of MSIX-enabled
>>>> devices in SPAPR PHB - this array may vary from 0 to 65536 entries and
>>>> its size depends on guest's ability to enable MSIX or do PCI hotplug.
>>>>
>>>> This adds new VMSTATE_VARRAY_STRUCT_ALLOC macro which is pretty similar to
>>>> VMSTATE_STRUCT_VARRAY_POINTER_INT32 but it can alloc memory for migratign
>>>> array on the destination side.
>>>>
>>>> This defines VMS_ALLOC flag for a field.
>>>>
>>>> This changes vmstate_base_addr() to do the allocation when receiving
>>>> migration.
>>>
>>>
>>> Juan, Peter? No hurry, just pinging in order not to forget :) Thanks!
>>
>> Hi, anyone? :)
>
> Ping?

Acked-by: Alexander Graf <agraf@suse.de>


Alex

Re: [Qemu-devel] [PATCH v3 8/9] vmstate: Add preallocation for migrating arrays (VMS_ALLOC flag)

[Alexey Kardashevskiy]

On 06/13/2014 02:57 AM, Alexander Graf wrote:
> On 06/12/2014 05:02 PM, Alexey Kardashevskiy wrote:
>> On 06/08/2014 09:59 AM, Alexey Kardashevskiy wrote:
>>> On 06/03/2014 11:19 PM, Alexey Kardashevskiy wrote:
>>>> On 05/30/2014 07:34 PM, Alexey Kardashevskiy wrote:
>>>>> There are few helpers already to support array migration. However they
>>>>> all
>>>>> require the destination side to preallocate arrays before migration which
>>>>> is not always possible due to unknown array size as it might be some
>>>>> sort of dynamic state. One of the examples is an array of MSIX-enabled
>>>>> devices in SPAPR PHB - this array may vary from 0 to 65536 entries and
>>>>> its size depends on guest's ability to enable MSIX or do PCI hotplug.
>>>>>
>>>>> This adds new VMSTATE_VARRAY_STRUCT_ALLOC macro which is pretty
>>>>> similar to
>>>>> VMSTATE_STRUCT_VARRAY_POINTER_INT32 but it can alloc memory for migratign
>>>>> array on the destination side.
>>>>>
>>>>> This defines VMS_ALLOC flag for a field.
>>>>>
>>>>> This changes vmstate_base_addr() to do the allocation when receiving
>>>>> migration.
>>>>
>>>>
>>>> Juan, Peter? No hurry, just pinging in order not to forget :) Thanks!
>>>
>>> Hi, anyone? :)
>>
>> Ping?
> 
> Acked-by: Alexander Graf <agraf@suse.de>


This is cool and thanks :) But we still need to attract attention of others...


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 8/9] vmstate: Add preallocation for migrating arrays (VMS_ALLOC flag)

[Juan Quintela]

Alexander Graf <agraf@suse.de> wrote:
> On 06/12/2014 05:02 PM, Alexey Kardashevskiy wrote:
>> On 06/08/2014 09:59 AM, Alexey Kardashevskiy wrote:
>>> On 06/03/2014 11:19 PM, Alexey Kardashevskiy wrote:
>>>> On 05/30/2014 07:34 PM, Alexey Kardashevskiy wrote:
>>>>> There are few helpers already to support array migration. However they all
>>>>> require the destination side to preallocate arrays before migration which
>>>>> is not always possible due to unknown array size as it might be some
>>>>> sort of dynamic state. One of the examples is an array of MSIX-enabled
>>>>> devices in SPAPR PHB - this array may vary from 0 to 65536 entries and
>>>>> its size depends on guest's ability to enable MSIX or do PCI hotplug.
>>>>>
>>>>> This adds new VMSTATE_VARRAY_STRUCT_ALLOC macro which is pretty similar to
>>>>> VMSTATE_STRUCT_VARRAY_POINTER_INT32 but it can alloc memory for migratign
>>>>> array on the destination side.
>>>>>
>>>>> This defines VMS_ALLOC flag for a field.
>>>>>
>>>>> This changes vmstate_base_addr() to do the allocation when receiving
>>>>> migration.
>>>>
>>>>
>>>> Juan, Peter? No hurry, just pinging in order not to forget :) Thanks!
>>>
>>> Hi, anyone? :)
>>
>> Ping?
>
> Acked-by: Alexander Graf <agraf@suse.de>

Reviewed-by: Juan Quintela <quintela@redhat.com>

BTW, should I include it, or will it got include through this series?

I am ok with both.

Later, Juan.

Re: [Qemu-devel] [PATCH v3 8/9] vmstate: Add preallocation for migrating arrays (VMS_ALLOC flag)

[Alexander Graf]

On 25.06.14 13:41, Juan Quintela wrote:
> Alexander Graf <agraf@suse.de> wrote:
>> On 06/12/2014 05:02 PM, Alexey Kardashevskiy wrote:
>>> On 06/08/2014 09:59 AM, Alexey Kardashevskiy wrote:
>>>> On 06/03/2014 11:19 PM, Alexey Kardashevskiy wrote:
>>>>> On 05/30/2014 07:34 PM, Alexey Kardashevskiy wrote:
>>>>>> There are few helpers already to support array migration. However they all
>>>>>> require the destination side to preallocate arrays before migration which
>>>>>> is not always possible due to unknown array size as it might be some
>>>>>> sort of dynamic state. One of the examples is an array of MSIX-enabled
>>>>>> devices in SPAPR PHB - this array may vary from 0 to 65536 entries and
>>>>>> its size depends on guest's ability to enable MSIX or do PCI hotplug.
>>>>>>
>>>>>> This adds new VMSTATE_VARRAY_STRUCT_ALLOC macro which is pretty similar to
>>>>>> VMSTATE_STRUCT_VARRAY_POINTER_INT32 but it can alloc memory for migratign
>>>>>> array on the destination side.
>>>>>>
>>>>>> This defines VMS_ALLOC flag for a field.
>>>>>>
>>>>>> This changes vmstate_base_addr() to do the allocation when receiving
>>>>>> migration.
>>>>>
>>>>> Juan, Peter? No hurry, just pinging in order not to forget :) Thanks!
>>>> Hi, anyone? :)
>>> Ping?
>> Acked-by: Alexander Graf <agraf@suse.de>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
>
> BTW, should I include it, or will it got include through this series?

Thanks a lot for the review :). I'll pick it up - the next patch depends 
on it and we're getting very close to hard freeze.


Alex

Re: [Qemu-devel] [PATCH v3 9/9] spapr_pci: Use XICS interrupt allocator and do not cache interrupts in PHB

[Alexey Kardashevskiy]

On 05/30/2014 11:45 PM, Alexander Graf wrote:
> 
> On 30.05.14 15:36, Alexey Kardashevskiy wrote:
>> On 05/30/2014 08:08 PM, Alexander Graf wrote:
>>> On 30.05.14 11:34, Alexey Kardashevskiy wrote:
>>>> Currently SPAPR PHB keeps track of all allocated MSI (here and below
>>>> MSI stands for both MSI and MSIX) interrupt because
>>>> XICS used to be unable to reuse interrupts. This is a problem for
>>>> dynamic MSI reconfiguration which happens when guest reloads a driver
>>>> or performs PCI hotplug. Another problem is that the existing
>>>> implementation can enable MSI on 32 devices maximum
>>>> (SPAPR_MSIX_MAX_DEVS=32) and there is no good reason for that.
>>>>
>>>> This makes use of new XICS ability to reuse interrupts.
>>>>
>>>> This reorganizes MSI information storage in sPAPRPHBState. Instead of
>>>> static array of 32 descriptors (one per a PCI function), this patch adds
>>>> a GHashTable when @config_addr is a key and (first_irq, num) pair is
>>>> a value. GHashTable can dynamically grow and shrink so the initial limit
>>>> of 32 devices is gone.
>>>>
>>>> This changes migration stream as @msi_table was a static array while new
>>>> @msi_devs is a dynamic hash table. This adds temporary array which is
>>>> used for migration, it is populated in "spapr_pci"::pre_save() callback
>>>> and expanded into the hash table in post_load() callback. Since
>>>> the destination side does not know the number of MSI-enabled devices
>>>> in advance and cannot pre-allocate the temporary array to receive
>>>> migration state, this makes use of new VMSTATE_STRUCT_VARRAY_ALLOC macro
>>>> which allocates the array automatically.
>>>>
>>>> This resets the MSI configuration space when interrupts are released by
>>>> the ibm,change-msi RTAS call.
>>>>
>>>> This fixed traces to be more informative.
>>>>
>>>> This changes vmstate_spapr_pci_msi name from "...lsi" to "...msi" which
>>>> was incorrect by accident. As the internal representation changed,
>>>> thus bumps migration version number.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>> Changes:
>>>> v3:
>>>> * used GHashTable
>>>> * implemented temporary MSI state array for migration
>>>> ---
>>>>    hw/ppc/spapr_pci.c          | 195
>>>> +++++++++++++++++++++++++-------------------
>>>>    include/hw/pci-host/spapr.h |  21 +++--
>>>>    trace-events                |   5 +-
>>>>    3 files changed, 129 insertions(+), 92 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>>> index a2f9677..48c9aad 100644
>>>> --- a/hw/ppc/spapr_pci.c
>>>> +++ b/hw/ppc/spapr_pci.c
>>>> @@ -220,36 +220,12 @@ static void rtas_write_pci_config(PowerPCCPU *cpu,
>>>> sPAPREnvironment *spapr,
>>>>    }
>>>>      /*
>>>> - * Find an entry with config_addr or returns the empty one if not
>>>> found AND
>>>> - * alloc_new is set.
>>>> - * At the moment the msi_table entries are never released so there is
>>>> - * no point to look till the end of the list if we need to find the free
>>>> entry.
>>>> - */
>>>> -static int spapr_msicfg_find(sPAPRPHBState *phb, uint32_t config_addr,
>>>> -                             bool alloc_new)
>>>> -{
>>>> -    int i;
>>>> -
>>>> -    for (i = 0; i < SPAPR_MSIX_MAX_DEVS; ++i) {
>>>> -        if (!phb->msi_table[i].nvec) {
>>>> -            break;
>>>> -        }
>>>> -        if (phb->msi_table[i].config_addr == config_addr) {
>>>> -            return i;
>>>> -        }
>>>> -    }
>>>> -    if ((i < SPAPR_MSIX_MAX_DEVS) && alloc_new) {
>>>> -        trace_spapr_pci_msi("Allocating new MSI config", i, config_addr);
>>>> -        return i;
>>>> -    }
>>>> -
>>>> -    return -1;
>>>> -}
>>>> -
>>>> -/*
>>>>     * Set MSI/MSIX message data.
>>>>     * This is required for msi_notify()/msix_notify() which
>>>>     * will write at the addresses via spapr_msi_write().
>>>> + *
>>>> + * If hwaddr == 0, all entries will have .data == first_irq i.e.
>>>> + * table will be reset.
>>>>     */
>>>>    static void spapr_msi_setmsg(PCIDevice *pdev, hwaddr addr, bool msix,
>>>>                                 unsigned first_irq, unsigned req_num)
>>>> @@ -263,9 +239,12 @@ static void spapr_msi_setmsg(PCIDevice *pdev, hwaddr
>>>> addr, bool msix,
>>>>            return;
>>>>        }
>>>>    -    for (i = 0; i < req_num; ++i, ++msg.data) {
>>>> +    for (i = 0; i < req_num; ++i) {
>>>>            msix_set_message(pdev, i, msg);
>>>>            trace_spapr_pci_msi_setup(pdev->name, i, msg.address);
>>>> +        if (addr) {
>>>> +            ++msg.data;
>>>> +        }
>>>>        }
>>>>    }
>>>>    @@ -280,9 +259,12 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu,
>>>> sPAPREnvironment *spapr,
>>>>        unsigned int req_num = rtas_ld(args, 4); /* 0 == remove all */
>>>>        unsigned int seq_num = rtas_ld(args, 5);
>>>>        unsigned int ret_intr_type;
>>>> -    int ndev, irq, max_irqs = 0;
>>>> +    unsigned int irq, max_irqs = 0, num = 0;
>>>>        sPAPRPHBState *phb = NULL;
>>>>        PCIDevice *pdev = NULL;
>>>> +    bool msix = false;
>>>> +    spapr_pci_msi *msi;
>>>> +    int *config_addr_key;
>>>>          switch (func) {
>>>>        case RTAS_CHANGE_MSI_FN:
>>>> @@ -310,13 +292,18 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu,
>>>> sPAPREnvironment *spapr,
>>>>          /* Releasing MSIs */
>>>>        if (!req_num) {
>>>> -        ndev = spapr_msicfg_find(phb, config_addr, false);
>>>> -        if (ndev < 0) {
>>>> -            trace_spapr_pci_msi("MSI has not been enabled", -1,
>>>> config_addr);
>>>> +        msi = (spapr_pci_msi *) g_hash_table_lookup(phb->msi,
>>>> &config_addr);
>>>> +        if (!msi) {
>>>> +            trace_spapr_pci_msi("Releasing wrong config", config_addr);
>>>>                rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
>>>>                return;
>>>>            }
>>>> -        trace_spapr_pci_msi("Released MSIs", ndev, config_addr);
>>>> +
>>>> +        xics_free(spapr->icp, msi->first_irq, msi->num);
>>>> +        spapr_msi_setmsg(pdev, 0, msix, 0, num);
>>>> +        g_hash_table_remove(phb->msi, &config_addr);
>>> Are you sure this doesn't have to be the exact same element? That pointer
>>> is to the stack, not to the element.
>>
>> I was not sure so I tested and it deletes element even if the key for
>> g_hash_table_remove() is on stack. I looked at glibc and it should just
>> work as it is now while I am providing a key_equal_func() callback to the
>> map:
>> http://sourcecodebrowser.com/glib2.0/2.12.4/ghash_8c.html#acff7e5fffc2572456a379d3d62d3e6ed
>>
> 
> True, works for me.
> 
>>
>>
>>
>>>> +
>>>> +        trace_spapr_pci_msi("Released MSIs", config_addr);
>>>>            rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>>>>            rtas_st(rets, 1, 0);
>>>>            return;
>>>> @@ -324,15 +311,6 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu,
>>>> sPAPREnvironment *spapr,
>>>>          /* Enabling MSI */
>>>>    -    /* Find a device number in the map to add or reuse the existing
>>>> one */
>>>> -    ndev = spapr_msicfg_find(phb, config_addr, true);
>>>> -    if (ndev >= SPAPR_MSIX_MAX_DEVS || ndev < 0) {
>>>> -        error_report("No free entry for a new MSI device");
>>>> -        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
>>>> -        return;
>>>> -    }
>>>> -    trace_spapr_pci_msi("Configuring MSI", ndev, config_addr);
>>>> -
>>>>        /* Check if the device supports as many IRQs as requested */
>>>>        if (ret_intr_type == RTAS_TYPE_MSI) {
>>>>            max_irqs = msi_nr_vectors_allocated(pdev);
>>>> @@ -340,48 +318,47 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu,
>>>> sPAPREnvironment *spapr,
>>>>            max_irqs = pdev->msix_entries_nr;
>>>>        }
>>>>        if (!max_irqs) {
>>>> -        error_report("Requested interrupt type %d is not enabled for
>>>> device#%d",
>>>> -                     ret_intr_type, ndev);
>>>> +        error_report("Requested interrupt type %d is not enabled for
>>>> device %x",
>>>> +                     ret_intr_type, config_addr);
>>>>            rtas_st(rets, 0, -1); /* Hardware error */
>>>>            return;
>>>>        }
>>>>        /* Correct the number if the guest asked for too many */
>>>>        if (req_num > max_irqs) {
>>>> +        trace_spapr_pci_msi_retry(config_addr, req_num, max_irqs);
>>>>            req_num = max_irqs;
>>>> +        irq = 0; /* to avoid misleading trace */
>>>> +        goto out;
>>>>        }
>>>>    -    /* Check if there is an old config and MSI number has not
>>>> changed */
>>>> -    if (phb->msi_table[ndev].nvec && (req_num !=
>>>> phb->msi_table[ndev].nvec)) {
>>>> -        /* Unexpected behaviour */
>>>> -        error_report("Cannot reuse MSI config for device#%d", ndev);
>>>> +    /* Allocate MSIs */
>>>> +    irq = xics_alloc_block(spapr->icp, 0, req_num, false,
>>>> +                           ret_intr_type == RTAS_TYPE_MSI);
>>>> +    if (!irq) {
>>>> +        error_report("Cannot allocate MSIs for device %x", config_addr);
>>>>            rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
>>>>            return;
>>>>        }
>>>>    -    /* There is no cached config, allocate MSIs */
>>>> -    if (!phb->msi_table[ndev].nvec) {
>>>> -        irq = xics_alloc_block(spapr->icp, 0, req_num, false,
>>>> -                               ret_intr_type == RTAS_TYPE_MSI);
>>>> -        if (irq < 0) {
>>>> -            error_report("Cannot allocate MSIs for device#%d", ndev);
>>>> -            rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
>>>> -            return;
>>>> -        }
>>>> -        phb->msi_table[ndev].irq = irq;
>>>> -        phb->msi_table[ndev].nvec = req_num;
>>>> -        phb->msi_table[ndev].config_addr = config_addr;
>>>> -    }
>>>> -
>>>>        /* Setup MSI/MSIX vectors in the device (via cfgspace or MSIX
>>>> BAR) */
>>>>        spapr_msi_setmsg(pdev, spapr->msi_win_addr, ret_intr_type ==
>>>> RTAS_TYPE_MSIX,
>>>> -                     phb->msi_table[ndev].irq, req_num);
>>>> +                     irq, req_num);
>>>>    +    /* Add MSI device to cache */
>>>> +    msi = g_new(spapr_pci_msi, 1);
>>>> +    msi->first_irq = irq;
>>>> +    msi->num = req_num;
>>>> +    config_addr_key = g_new(int, 1);
>>> gint?
>> Same thing but yeah, better to stay solid.
>>
>> Is that it or you will have more comments after more careful review? :)
>> Thanks!
> 
> I think the patch set is ok as is, but I want to have Juan's / Peter's ack
> on the vmstate patch.

Since Juan is ignoring this, can you please pull 1..7 of this and leave 8-9
for later as they not exactly about XICS anyway? Thanks!


> 
> 
> Alex
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 0/9] Move interrupts from spapr to xics

[Alexander Graf]

On 30.05.14 11:34, Alexey Kardashevskiy wrote:
> This moves interrupts allocation business from SPAPR to XICS
> and makes use of it.
>
> Changes:
> v3:
> * replaces static array of descriptors in SPAPR PHB with GHashTable
> * implements migration of hash table via temporary array
>
> v2:
> * s/server/source/
> * fixed typos, code style, added an assert
> * added patch for spapr_pci for better IRQ reuse for MSI/MSIX
>
> There is just one source at the moment. We might create one
> per PHB and one per VIO device or VIO bus but I do not see any
> immediate profit from it.
>
>
>
> Juan, Peter, you are cc-ed because 8/9 introduces new VMSTATE_STRUCT_VARRAY_ALLOC
> and 9/9 makes use of it and checkpatch.pl thinks you might be right people
> to get opinion about this from.
>
>
> Please comment. Thanks!

Thanks, applied to ppc-next.


Alex