On 07/04/2017 04:37 PM, Cornelia Huck wrote:
> On Tue, 4 Jul 2017 16:07:56 +0200
> Christian Borntraeger <borntrae...@de.ibm.com> wrote:
>
>> From: Halil Pasic <pa...@linux.vnet.ibm.com>
>>
>> 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 <pa...@linux.vnet.ibm.com>
>> Fixes: f6f4ce4211 ("s390x: add property adapter_routes_max_batch")
>> Reviewed-by: Dong Jia Shi <bjsdj...@linux.vnet.ibm.com>
>> Reviewed-by: Yi Min Zhao <zyi...@linux.vnet.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntrae...@de.ibm.com>
>> ---
>> 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,
>> };
>>
>
>
