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 <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.
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, >>> }; >>> >> >>