[4/7] s390x: fix realize inheritance for kvm-flic

Commit Message

Christian Borntraeger July 4, 2017, 2:07 p.m. UTC

From: Halil Pasic <[e-mail]>

Commit f6f4ce4211 ("s390x: add property adapter_routes_max_batch",
2016-12-09) introduces a common realize (intended to be common for all
the subclasses) for flic, but fails to make sure the kvm-flic which had
it's own is actually calling this common realize.

This omission fortunately does not result in a grave problem. The common
realize was only supposed to catch a possible programming mistake by
validating a value of a property set via the compat machine macros. Since
there was no programming mistake we don't need this fixed for stable.

Let's fix this problem by making sure kvm flic honors the realize of its
parent class.

Let us also improve on the error message we would hypothetically emit
when the validation fails.

Signed-off-by: Halil Pasic <[e-mail]>
Fixes: f6f4ce4211 ("s390x: add property adapter_routes_max_batch")
Reviewed-by: Dong Jia Shi <[e-mail]>
Reviewed-by: Yi Min Zhao <[e-mail]>
Signed-off-by: Christian Borntraeger <[e-mail]>
---
 hw/intc/s390_flic.c     |  4 ++--
 hw/intc/s390_flic_kvm.c | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

Comments

Cornelia Huck July 4, 2017, 2:37 p.m. UTC | #1

On Tue,  4 Jul 2017 16:07:56 +0200
Christian Borntraeger <[e-mail]> wrote:

> From: Halil Pasic <[e-mail]>
> 
> Commit f6f4ce4211 ("s390x: add property adapter_routes_max_batch",
> 2016-12-09) introduces a common realize (intended to be common for all
> the subclasses) for flic, but fails to make sure the kvm-flic which had
> it's own is actually calling this common realize.

s/it's/its/

> 
> This omission fortunately does not result in a grave problem. The common
> realize was only supposed to catch a possible programming mistake by
> validating a value of a property set via the compat machine macros. Since
> there was no programming mistake we don't need this fixed for stable.
> 
> Let's fix this problem by making sure kvm flic honors the realize of its
> parent class.
> 
> Let us also improve on the error message we would hypothetically emit
> when the validation fails.
> 
> Signed-off-by: Halil Pasic <[e-mail]>
> Fixes: f6f4ce4211 ("s390x: add property adapter_routes_max_batch")
> Reviewed-by: Dong Jia Shi <[e-mail]>
> Reviewed-by: Yi Min Zhao <[e-mail]>
> Signed-off-by: Christian Borntraeger <[e-mail]>
> ---
>  hw/intc/s390_flic.c     |  4 ++--
>  hw/intc/s390_flic_kvm.c | 17 +++++++++++++++++
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> index a99a350..837158b 100644
> --- a/hw/intc/s390_flic.c
> +++ b/hw/intc/s390_flic.c
> @@ -101,8 +101,8 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
>      uint32_t max_batch = S390_FLIC_COMMON(dev)->adapter_routes_max_batch;
>  
>      if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
> -        error_setg(errp, "flic adapter_routes_max_batch too big"
> -                   "%d (%d allowed)", max_batch, ADAPTER_ROUTES_MAX_GSI);
> +        error_setg(errp, "flic property adapter_routes_max_batch too big"
> +                   " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);

Unrelated message change?

>      }
>  }
>  
> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
> index bea3997..535d99d 100644
> --- a/hw/intc/s390_flic_kvm.c
> +++ b/hw/intc/s390_flic_kvm.c
> @@ -392,6 +392,17 @@ static const VMStateDescription kvm_s390_flic_vmstate = {
>      }
>  };
>  
> +typedef struct KVMS390FLICStateClass {
> +    S390FLICStateClass parent_class;
> +    DeviceRealize parent_realize;
> +} KVMS390FLICStateClass;
> +
> +#define KVM_S390_FLIC_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(KVMS390FLICStateClass, (obj), TYPE_KVM_S390_FLIC)
> +
> +#define KVM_S390_FLIC_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(KVMS390FLICStateClass, (klass), TYPE_KVM_S390_FLIC)
> +
>  static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
>  {
>      KVMS390FLICState *flic_state = KVM_S390_FLIC(dev);
> @@ -400,6 +411,10 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
>      int ret;
>      Error *errp_local = NULL;
>  
> +    KVM_S390_FLIC_GET_CLASS(dev)->parent_realize(dev, &errp_local);

I usually prefer to introduce a local variable for that, but don't care
too much.

> +    if (errp_local) {
> +        goto fail;
> +    }
>      flic_state->fd = -1;
>      if (!kvm_check_extension(kvm_state, KVM_CAP_DEVICE_CTRL)) {
>          error_setg_errno(&errp_local, errno, "KVM is missing capability"
> @@ -454,6 +469,7 @@ static void kvm_s390_flic_class_init(ObjectClass *oc, void *data)
>      DeviceClass *dc = DEVICE_CLASS(oc);
>      S390FLICStateClass *fsc = S390_FLIC_COMMON_CLASS(oc);
>  
> +    KVM_S390_FLIC_CLASS(oc)->parent_realize = dc->realize;

dito

>      dc->realize = kvm_s390_flic_realize;
>      dc->vmsd = &kvm_s390_flic_vmstate;
>      dc->reset = kvm_s390_flic_reset;
> @@ -468,6 +484,7 @@ static const TypeInfo kvm_s390_flic_info = {
>      .name          = TYPE_KVM_S390_FLIC,
>      .parent        = TYPE_S390_FLIC_COMMON,
>      .instance_size = sizeof(KVMS390FLICState),
> +    .class_size    = sizeof(KVMS390FLICStateClass),
>      .class_init    = kvm_s390_flic_class_init,
>  };
>

Halil Pasic July 4, 2017, 2:51 p.m. UTC | #2

On 07/04/2017 04:37 PM, Cornelia Huck wrote:
> On Tue,  4 Jul 2017 16:07:56 +0200
> Christian Borntraeger <[e-mail]> wrote:
> 
>> From: Halil Pasic <[e-mail]>
>>
>> Commit f6f4ce4211 ("s390x: add property adapter_routes_max_batch",
>> 2016-12-09) introduces a common realize (intended to be common for all
>> the subclasses) for flic, but fails to make sure the kvm-flic which had
>> it's own is actually calling this common realize.
> 
> s/it's/its/
> 

Valid. Sorry.

>>
>> This omission fortunately does not result in a grave problem. The common
>> realize was only supposed to catch a possible programming mistake by
>> validating a value of a property set via the compat machine macros. Since
>> there was no programming mistake we don't need this fixed for stable.
>>
>> Let's fix this problem by making sure kvm flic honors the realize of its
>> parent class.
>>
>> Let us also improve on the error message we would hypothetically emit
>> when the validation fails.
>>
>> Signed-off-by: Halil Pasic <[e-mail]>
>> Fixes: f6f4ce4211 ("s390x: add property adapter_routes_max_batch")
>> Reviewed-by: Dong Jia Shi <[e-mail]>
>> Reviewed-by: Yi Min Zhao <[e-mail]>
>> Signed-off-by: Christian Borntraeger <[e-mail]>
>> ---
>>  hw/intc/s390_flic.c     |  4 ++--
>>  hw/intc/s390_flic_kvm.c | 17 +++++++++++++++++
>>  2 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
>> index a99a350..837158b 100644
>> --- a/hw/intc/s390_flic.c
>> +++ b/hw/intc/s390_flic.c
>> @@ -101,8 +101,8 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
>>      uint32_t max_batch = S390_FLIC_COMMON(dev)->adapter_routes_max_batch;
>>  
>>      if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
>> -        error_setg(errp, "flic adapter_routes_max_batch too big"
>> -                   "%d (%d allowed)", max_batch, ADAPTER_ROUTES_MAX_GSI);
>> +        error_setg(errp, "flic property adapter_routes_max_batch too big"
>> +                   " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
> 
> Unrelated message change?
> 

I've mentioned it in the commit message. It was also introduced by the
patch I'm fixing. But yes strictly it's two different problems.


>>      }
>>  }
>>  
>> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
>> index bea3997..535d99d 100644
>> --- a/hw/intc/s390_flic_kvm.c
>> +++ b/hw/intc/s390_flic_kvm.c
>> @@ -392,6 +392,17 @@ static const VMStateDescription kvm_s390_flic_vmstate = {
>>      }
>>  };
>>  
>> +typedef struct KVMS390FLICStateClass {
>> +    S390FLICStateClass parent_class;
>> +    DeviceRealize parent_realize;
>> +} KVMS390FLICStateClass;
>> +
>> +#define KVM_S390_FLIC_GET_CLASS(obj) \
>> +    OBJECT_GET_CLASS(KVMS390FLICStateClass, (obj), TYPE_KVM_S390_FLIC)
>> +
>> +#define KVM_S390_FLIC_CLASS(klass) \
>> +    OBJECT_CLASS_CHECK(KVMS390FLICStateClass, (klass), TYPE_KVM_S390_FLIC)
>> +
>>  static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
>>  {
>>      KVMS390FLICState *flic_state = KVM_S390_FLIC(dev);
>> @@ -400,6 +411,10 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
>>      int ret;
>>      Error *errp_local = NULL;
>>  
>> +    KVM_S390_FLIC_GET_CLASS(dev)->parent_realize(dev, &errp_local);
> 
> I usually prefer to introduce a local variable for that, but don't care
> too much.
> 
>> +    if (errp_local) {
>> +        goto fail;
>> +    }
>>      flic_state->fd = -1;
>>      if (!kvm_check_extension(kvm_state, KVM_CAP_DEVICE_CTRL)) {
>>          error_setg_errno(&errp_local, errno, "KVM is missing capability"
>> @@ -454,6 +469,7 @@ static void kvm_s390_flic_class_init(ObjectClass *oc, void *data)
>>      DeviceClass *dc = DEVICE_CLASS(oc);
>>      S390FLICStateClass *fsc = S390_FLIC_COMMON_CLASS(oc);
>>  
>> +    KVM_S390_FLIC_CLASS(oc)->parent_realize = dc->realize;
> 
> dito
> 
>>      dc->realize = kvm_s390_flic_realize;
>>      dc->vmsd = &kvm_s390_flic_vmstate;
>>      dc->reset = kvm_s390_flic_reset;
>> @@ -468,6 +484,7 @@ static const TypeInfo kvm_s390_flic_info = {
>>      .name          = TYPE_KVM_S390_FLIC,
>>      .parent        = TYPE_S390_FLIC_COMMON,
>>      .instance_size = sizeof(KVMS390FLICState),
>> +    .class_size    = sizeof(KVMS390FLICStateClass),
>>      .class_init    = kvm_s390_flic_class_init,
>>  };
>>  
> 
>

Christian Borntraeger July 5, 2017, 10:20 a.m. UTC | #3

On 07/04/2017 04:51 PM, Halil Pasic wrote:
> 
> 
> On 07/04/2017 04:37 PM, Cornelia Huck wrote:
>> On Tue,  4 Jul 2017 16:07:56 +0200
>> Christian Borntraeger <[e-mail]> wrote:
>>
>>> From: Halil Pasic <[e-mail]>
>>>
>>> Commit f6f4ce4211 ("s390x: add property adapter_routes_max_batch",
>>> 2016-12-09) introduces a common realize (intended to be common for all
>>> the subclasses) for flic, but fails to make sure the kvm-flic which had
>>> it's own is actually calling this common realize.
>>
>> s/it's/its/
>>
> 
> Valid. Sorry.
> 
>>>
>>> This omission fortunately does not result in a grave problem. The common
>>> realize was only supposed to catch a possible programming mistake by
>>> validating a value of a property set via the compat machine macros. Since
>>> there was no programming mistake we don't need this fixed for stable.
>>>
>>> Let's fix this problem by making sure kvm flic honors the realize of its
>>> parent class.
>>>
>>> Let us also improve on the error message we would hypothetically emit
>>> when the validation fails.
>>>
>>> Signed-off-by: Halil Pasic <[e-mail]>
>>> Fixes: f6f4ce4211 ("s390x: add property adapter_routes_max_batch")
>>> Reviewed-by: Dong Jia Shi <[e-mail]>
>>> Reviewed-by: Yi Min Zhao <[e-mail]>
>>> Signed-off-by: Christian Borntraeger <[e-mail]>
>>> ---
>>>  hw/intc/s390_flic.c     |  4 ++--
>>>  hw/intc/s390_flic_kvm.c | 17 +++++++++++++++++
>>>  2 files changed, 19 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
>>> index a99a350..837158b 100644
>>> --- a/hw/intc/s390_flic.c
>>> +++ b/hw/intc/s390_flic.c
>>> @@ -101,8 +101,8 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
>>>      uint32_t max_batch = S390_FLIC_COMMON(dev)->adapter_routes_max_batch;
>>>  
>>>      if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
>>> -        error_setg(errp, "flic adapter_routes_max_batch too big"
>>> -                   "%d (%d allowed)", max_batch, ADAPTER_ROUTES_MAX_GSI);
>>> +        error_setg(errp, "flic property adapter_routes_max_batch too big"
>>> +                   " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
>>
>> Unrelated message change?
>>
> 
> I've mentioned it in the commit message. It was also introduced by the
> patch I'm fixing. But yes strictly it's two different problems.

I will only fix the patch description ( s/it's/its/) and keep the other things
unchanged. Is that fine with you?
> 
> 
>>>      }
>>>  }
>>>  
>>> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
>>> index bea3997..535d99d 100644
>>> --- a/hw/intc/s390_flic_kvm.c
>>> +++ b/hw/intc/s390_flic_kvm.c
>>> @@ -392,6 +392,17 @@ static const VMStateDescription kvm_s390_flic_vmstate = {
>>>      }
>>>  };
>>>  
>>> +typedef struct KVMS390FLICStateClass {
>>> +    S390FLICStateClass parent_class;
>>> +    DeviceRealize parent_realize;
>>> +} KVMS390FLICStateClass;
>>> +
>>> +#define KVM_S390_FLIC_GET_CLASS(obj) \
>>> +    OBJECT_GET_CLASS(KVMS390FLICStateClass, (obj), TYPE_KVM_S390_FLIC)
>>> +
>>> +#define KVM_S390_FLIC_CLASS(klass) \
>>> +    OBJECT_CLASS_CHECK(KVMS390FLICStateClass, (klass), TYPE_KVM_S390_FLIC)
>>> +
>>>  static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
>>>  {
>>>      KVMS390FLICState *flic_state = KVM_S390_FLIC(dev);
>>> @@ -400,6 +411,10 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
>>>      int ret;
>>>      Error *errp_local = NULL;
>>>  
>>> +    KVM_S390_FLIC_GET_CLASS(dev)->parent_realize(dev, &errp_local);
>>
>> I usually prefer to introduce a local variable for that, but don't care
>> too much.
>>
>>> +    if (errp_local) {
>>> +        goto fail;
>>> +    }
>>>      flic_state->fd = -1;
>>>      if (!kvm_check_extension(kvm_state, KVM_CAP_DEVICE_CTRL)) {
>>>          error_setg_errno(&errp_local, errno, "KVM is missing capability"
>>> @@ -454,6 +469,7 @@ static void kvm_s390_flic_class_init(ObjectClass *oc, void *data)
>>>      DeviceClass *dc = DEVICE_CLASS(oc);
>>>      S390FLICStateClass *fsc = S390_FLIC_COMMON_CLASS(oc);
>>>  
>>> +    KVM_S390_FLIC_CLASS(oc)->parent_realize = dc->realize;
>>
>> dito
>>
>>>      dc->realize = kvm_s390_flic_realize;
>>>      dc->vmsd = &kvm_s390_flic_vmstate;
>>>      dc->reset = kvm_s390_flic_reset;
>>> @@ -468,6 +484,7 @@ static const TypeInfo kvm_s390_flic_info = {
>>>      .name          = TYPE_KVM_S390_FLIC,
>>>      .parent        = TYPE_S390_FLIC_COMMON,
>>>      .instance_size = sizeof(KVMS390FLICState),
>>> +    .class_size    = sizeof(KVMS390FLICStateClass),
>>>      .class_init    = kvm_s390_flic_class_init,
>>>  };
>>>  
>>
>>

Halil Pasic July 5, 2017, 10:28 a.m. UTC | #4

On 07/05/2017 12:20 PM, Christian Borntraeger wrote:
> On 07/04/2017 04:51 PM, Halil Pasic wrote:
>>
>>
>> On 07/04/2017 04:37 PM, Cornelia Huck wrote:
>>> On Tue,  4 Jul 2017 16:07:56 +0200
>>> Christian Borntraeger <[e-mail]> wrote:
>>>
>>>> From: Halil Pasic <[e-mail]>
>>>>
>>>> Commit f6f4ce4211 ("s390x: add property adapter_routes_max_batch",
>>>> 2016-12-09) introduces a common realize (intended to be common for all
>>>> the subclasses) for flic, but fails to make sure the kvm-flic which had
>>>> it's own is actually calling this common realize.
>>>
>>> s/it's/its/
>>>
>>
>> Valid. Sorry.
>>
>>>>
>>>> This omission fortunately does not result in a grave problem. The common
>>>> realize was only supposed to catch a possible programming mistake by
>>>> validating a value of a property set via the compat machine macros. Since
>>>> there was no programming mistake we don't need this fixed for stable.
>>>>
>>>> Let's fix this problem by making sure kvm flic honors the realize of its
>>>> parent class.
>>>>
>>>> Let us also improve on the error message we would hypothetically emit
>>>> when the validation fails.
>>>>
>>>> Signed-off-by: Halil Pasic <[e-mail]>
>>>> Fixes: f6f4ce4211 ("s390x: add property adapter_routes_max_batch")
>>>> Reviewed-by: Dong Jia Shi <[e-mail]>
>>>> Reviewed-by: Yi Min Zhao <[e-mail]>
>>>> Signed-off-by: Christian Borntraeger <[e-mail]>
>>>> ---
>>>>  hw/intc/s390_flic.c     |  4 ++--
>>>>  hw/intc/s390_flic_kvm.c | 17 +++++++++++++++++
>>>>  2 files changed, 19 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
>>>> index a99a350..837158b 100644
>>>> --- a/hw/intc/s390_flic.c
>>>> +++ b/hw/intc/s390_flic.c
>>>> @@ -101,8 +101,8 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
>>>>      uint32_t max_batch = S390_FLIC_COMMON(dev)->adapter_routes_max_batch;
>>>>  
>>>>      if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
>>>> -        error_setg(errp, "flic adapter_routes_max_batch too big"
>>>> -                   "%d (%d allowed)", max_batch, ADAPTER_ROUTES_MAX_GSI);
>>>> +        error_setg(errp, "flic property adapter_routes_max_batch too big"
>>>> +                   " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
>>>
>>> Unrelated message change?
>>>
>>
>> I've mentioned it in the commit message. It was also introduced by the
>> patch I'm fixing. But yes strictly it's two different problems.
> 
> I will only fix the patch description ( s/it's/its/) and keep the other things
> unchanged. Is that fine with you?

It's fine with me, but I guess you probably asked Connie. Thanks!

Cornelia Huck July 5, 2017, 11:11 a.m. UTC | #5

On Wed, 5 Jul 2017 12:20:42 +0200
Christian Borntraeger <[e-mail]> wrote:

> On 07/04/2017 04:51 PM, Halil Pasic wrote:
> > 
> > 
> > On 07/04/2017 04:37 PM, Cornelia Huck wrote:  
> >> On Tue,  4 Jul 2017 16:07:56 +0200
> >> Christian Borntraeger <[e-mail]> wrote:
> >>  
> >>> From: Halil Pasic <[e-mail]>
> >>>
> >>> Commit f6f4ce4211 ("s390x: add property adapter_routes_max_batch",
> >>> 2016-12-09) introduces a common realize (intended to be common for all
> >>> the subclasses) for flic, but fails to make sure the kvm-flic which had
> >>> it's own is actually calling this common realize.  
> >>
> >> s/it's/its/
> >>  
> > 
> > Valid. Sorry.
> >   
> >>>
> >>> This omission fortunately does not result in a grave problem. The common
> >>> realize was only supposed to catch a possible programming mistake by
> >>> validating a value of a property set via the compat machine macros. Since
> >>> there was no programming mistake we don't need this fixed for stable.
> >>>
> >>> Let's fix this problem by making sure kvm flic honors the realize of its
> >>> parent class.
> >>>
> >>> Let us also improve on the error message we would hypothetically emit
> >>> when the validation fails.
> >>>
> >>> Signed-off-by: Halil Pasic <[e-mail]>
> >>> Fixes: f6f4ce4211 ("s390x: add property adapter_routes_max_batch")
> >>> Reviewed-by: Dong Jia Shi <[e-mail]>
> >>> Reviewed-by: Yi Min Zhao <[e-mail]>
> >>> Signed-off-by: Christian Borntraeger <[e-mail]>
> >>> ---
> >>>  hw/intc/s390_flic.c     |  4 ++--
> >>>  hw/intc/s390_flic_kvm.c | 17 +++++++++++++++++
> >>>  2 files changed, 19 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> >>> index a99a350..837158b 100644
> >>> --- a/hw/intc/s390_flic.c
> >>> +++ b/hw/intc/s390_flic.c
> >>> @@ -101,8 +101,8 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
> >>>      uint32_t max_batch = S390_FLIC_COMMON(dev)->adapter_routes_max_batch;
> >>>  
> >>>      if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
> >>> -        error_setg(errp, "flic adapter_routes_max_batch too big"
> >>> -                   "%d (%d allowed)", max_batch, ADAPTER_ROUTES_MAX_GSI);
> >>> +        error_setg(errp, "flic property adapter_routes_max_batch too big"
> >>> +                   " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);  
> >>
> >> Unrelated message change?
> >>  
> > 
> > I've mentioned it in the commit message. It was also introduced by the
> > patch I'm fixing. But yes strictly it's two different problems.  
> 
> I will only fix the patch description ( s/it's/its/) and keep the other things
> unchanged. Is that fine with you?

Yup. Let's get this done.

With the changes,

Reviewed-by: Cornelia Huck <[e-mail]>

Cornelia Huck July 5, 2017, 11:16 a.m. UTC | #6

On Wed, 5 Jul 2017 13:11:59 +0200
Cornelia Huck <[e-mail]> wrote:

> On Wed, 5 Jul 2017 12:20:42 +0200
> Christian Borntraeger <[e-mail]> wrote:
> 
> > On 07/04/2017 04:51 PM, Halil Pasic wrote:  
> > > 
> > > 
> > > On 07/04/2017 04:37 PM, Cornelia Huck wrote:    
> > >> On Tue,  4 Jul 2017 16:07:56 +0200
> > >> Christian Borntraeger <[e-mail]> wrote:
> > >>    
> > >>> From: Halil Pasic <[e-mail]>
> > >>>
> > >>> Commit f6f4ce4211 ("s390x: add property adapter_routes_max_batch",
> > >>> 2016-12-09) introduces a common realize (intended to be common for all
> > >>> the subclasses) for flic, but fails to make sure the kvm-flic which had
> > >>> it's own is actually calling this common realize.    
> > >>
> > >> s/it's/its/
> > >>    
> > > 
> > > Valid. Sorry.
> > >     
> > >>>
> > >>> This omission fortunately does not result in a grave problem. The common
> > >>> realize was only supposed to catch a possible programming mistake by
> > >>> validating a value of a property set via the compat machine macros. Since
> > >>> there was no programming mistake we don't need this fixed for stable.
> > >>>
> > >>> Let's fix this problem by making sure kvm flic honors the realize of its
> > >>> parent class.
> > >>>
> > >>> Let us also improve on the error message we would hypothetically emit
> > >>> when the validation fails.
> > >>>
> > >>> Signed-off-by: Halil Pasic <[e-mail]>
> > >>> Fixes: f6f4ce4211 ("s390x: add property adapter_routes_max_batch")
> > >>> Reviewed-by: Dong Jia Shi <[e-mail]>
> > >>> Reviewed-by: Yi Min Zhao <[e-mail]>
> > >>> Signed-off-by: Christian Borntraeger <[e-mail]>
> > >>> ---
> > >>>  hw/intc/s390_flic.c     |  4 ++--
> > >>>  hw/intc/s390_flic_kvm.c | 17 +++++++++++++++++
> > >>>  2 files changed, 19 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> > >>> index a99a350..837158b 100644
> > >>> --- a/hw/intc/s390_flic.c
> > >>> +++ b/hw/intc/s390_flic.c
> > >>> @@ -101,8 +101,8 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
> > >>>      uint32_t max_batch = S390_FLIC_COMMON(dev)->adapter_routes_max_batch;
> > >>>  
> > >>>      if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
> > >>> -        error_setg(errp, "flic adapter_routes_max_batch too big"
> > >>> -                   "%d (%d allowed)", max_batch, ADAPTER_ROUTES_MAX_GSI);
> > >>> +        error_setg(errp, "flic property adapter_routes_max_batch too big"
> > >>> +                   " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);    
> > >>
> > >> Unrelated message change?
> > >>    
> > > 
> > > I've mentioned it in the commit message. It was also introduced by the
> > > patch I'm fixing. But yes strictly it's two different problems.    
> > 
> > I will only fix the patch description ( s/it's/its/) and keep the other things
> > unchanged. Is that fine with you?  
> 
> Yup. Let's get this done.
> 
> With the changes,

s/changes/change/

One coffee is obviously not enough...

> 
> Reviewed-by: Cornelia Huck <[e-mail]>

Patch

diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
index a99a350..837158b 100644
--- a/hw/intc/s390_flic.c
+++ b/hw/intc/s390_flic.c
@@ -101,8 +101,8 @@  static void s390_flic_common_realize(DeviceState *dev, Error **errp)
     uint32_t max_batch = S390_FLIC_COMMON(dev)->adapter_routes_max_batch;
 
     if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
-        error_setg(errp, "flic adapter_routes_max_batch too big"
-                   "%d (%d allowed)", max_batch, ADAPTER_ROUTES_MAX_GSI);
+        error_setg(errp, "flic property adapter_routes_max_batch too big"
+                   " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
     }
 }
 
diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index bea3997..535d99d 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -392,6 +392,17 @@  static const VMStateDescription kvm_s390_flic_vmstate = {
     }
 };
 
+typedef struct KVMS390FLICStateClass {
+    S390FLICStateClass parent_class;
+    DeviceRealize parent_realize;
+} KVMS390FLICStateClass;
+
+#define KVM_S390_FLIC_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(KVMS390FLICStateClass, (obj), TYPE_KVM_S390_FLIC)
+
+#define KVM_S390_FLIC_CLASS(klass) \
+    OBJECT_CLASS_CHECK(KVMS390FLICStateClass, (klass), TYPE_KVM_S390_FLIC)
+
 static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
 {
     KVMS390FLICState *flic_state = KVM_S390_FLIC(dev);
@@ -400,6 +411,10 @@  static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
     int ret;
     Error *errp_local = NULL;
 
+    KVM_S390_FLIC_GET_CLASS(dev)->parent_realize(dev, &errp_local);
+    if (errp_local) {
+        goto fail;
+    }
     flic_state->fd = -1;
     if (!kvm_check_extension(kvm_state, KVM_CAP_DEVICE_CTRL)) {
         error_setg_errno(&errp_local, errno, "KVM is missing capability"
@@ -454,6 +469,7 @@  static void kvm_s390_flic_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
     S390FLICStateClass *fsc = S390_FLIC_COMMON_CLASS(oc);
 
+    KVM_S390_FLIC_CLASS(oc)->parent_realize = dc->realize;
     dc->realize = kvm_s390_flic_realize;
     dc->vmsd = &kvm_s390_flic_vmstate;
     dc->reset = kvm_s390_flic_reset;
@@ -468,6 +484,7 @@  static const TypeInfo kvm_s390_flic_info = {
     .name          = TYPE_KVM_S390_FLIC,
     .parent        = TYPE_S390_FLIC_COMMON,
     .instance_size = sizeof(KVMS390FLICState),
+    .class_size    = sizeof(KVMS390FLICStateClass),
     .class_init    = kvm_s390_flic_class_init,
 };