| Message ID | 20200123170256.12386-1-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/s390x: Add a more verbose comment about get_machine_class() and the wrappers | expand |
On 23/01/2020 18.42, Cornelia Huck wrote: > On Thu, 23 Jan 2020 18:02:56 +0100 > Thomas Huth <thuth@redhat.com> wrote: > >> While working on the "Enable adapter interruption suppression again" >> recently, I had to discover that the meaning of get_machine_class() >> and the related *_allowed() wrappers is not very obvious. Add a more >> verbose comment here to clarify how these should be used. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 15 ++++++++++++--- >> 1 file changed, 12 insertions(+), 3 deletions(-) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index e0e28139a2..7fb389f0e5 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -505,6 +505,18 @@ static inline void machine_set_dea_key_wrap(Object *obj, bool value, >> >> static S390CcwMachineClass *current_mc; >> >> +/* >> + * Get the class of the s390-ccw-virtio machine that is currently in use. >> + * Note: libvirt is using the "none" machine to probe for the features of the >> + * host CPU, so in case this is called with the "none" machine, the function >> + * returns the TYPE_S390_CCW_MACHINE base class. In this base class, all the >> + * various "*_allowed" variables are enabled, so that the *_allowed() wrappers >> + * below return the correct default value for the "none" machine. > > Maybe add a blank line here for readability? (Can do so while applying.) Sure, fine for me! >> + * Attention! Do *not* add additional new wrappers for CPU features (e.g. like >> + * the ri_allowed() wrapper) via this mechanism anymore. CPU features should >> + * be handled via the CPU models, i.e. checking with cpu_model_allowed() during >> + * CPU initialization and s390_has_feat() later should be sufficient. >> + */ >> static S390CcwMachineClass *get_machine_class(void) >> { >> if (unlikely(!current_mc)) { >> @@ -521,19 +533,16 @@ static S390CcwMachineClass *get_machine_class(void) >> >> bool ri_allowed(void) >> { >> - /* for "none" machine this results in true */ >> return get_machine_class()->ri_allowed; >> } >> >> bool cpu_model_allowed(void) >> { >> - /* for "none" machine this results in true */ >> return get_machine_class()->cpu_model_allowed; >> } >> >> bool hpage_1m_allowed(void) >> { >> - /* for "none" machine this results in true */ >> return get_machine_class()->hpage_1m_allowed; >> } >> > > Looks good to me, but will wait for a review or two. > Thanks, Thomas
On 23.01.20 18:02, Thomas Huth wrote: > While working on the "Enable adapter interruption suppression again" > recently, I had to discover that the meaning of get_machine_class() > and the related *_allowed() wrappers is not very obvious. Add a more > verbose comment here to clarify how these should be used. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/s390x/s390-virtio-ccw.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index e0e28139a2..7fb389f0e5 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -505,6 +505,18 @@ static inline void machine_set_dea_key_wrap(Object *obj, bool value, > > static S390CcwMachineClass *current_mc; > > +/* > + * Get the class of the s390-ccw-virtio machine that is currently in use. > + * Note: libvirt is using the "none" machine to probe for the features of the > + * host CPU, so in case this is called with the "none" machine, the function > + * returns the TYPE_S390_CCW_MACHINE base class. In this base class, all the > + * various "*_allowed" variables are enabled, so that the *_allowed() wrappers > + * below return the correct default value for the "none" machine. > + * Attention! Do *not* add additional new wrappers for CPU features (e.g. like > + * the ri_allowed() wrapper) via this mechanism anymore. CPU features should > + * be handled via the CPU models, i.e. checking with cpu_model_allowed() during > + * CPU initialization and s390_has_feat() later should be sufficient. > + */ > static S390CcwMachineClass *get_machine_class(void) > { > if (unlikely(!current_mc)) { > @@ -521,19 +533,16 @@ static S390CcwMachineClass *get_machine_class(void) > > bool ri_allowed(void) > { > - /* for "none" machine this results in true */ > return get_machine_class()->ri_allowed; > } > > bool cpu_model_allowed(void) > { > - /* for "none" machine this results in true */ > return get_machine_class()->cpu_model_allowed; > } > > bool hpage_1m_allowed(void) > { > - /* for "none" machine this results in true */ > return get_machine_class()->hpage_1m_allowed; > } > > Very helpful, thanks! Reviewed-by: David Hildenbrand <david@redhat.com>
On Thu, 23 Jan 2020 18:02:56 +0100 Thomas Huth <thuth@redhat.com> wrote: > While working on the "Enable adapter interruption suppression again" > recently, I had to discover that the meaning of get_machine_class() > and the related *_allowed() wrappers is not very obvious. Add a more > verbose comment here to clarify how these should be used. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/s390x/s390-virtio-ccw.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) Thanks, applied.
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index e0e28139a2..7fb389f0e5 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -505,6 +505,18 @@ static inline void machine_set_dea_key_wrap(Object *obj, bool value, static S390CcwMachineClass *current_mc; +/* + * Get the class of the s390-ccw-virtio machine that is currently in use. + * Note: libvirt is using the "none" machine to probe for the features of the + * host CPU, so in case this is called with the "none" machine, the function + * returns the TYPE_S390_CCW_MACHINE base class. In this base class, all the + * various "*_allowed" variables are enabled, so that the *_allowed() wrappers + * below return the correct default value for the "none" machine. + * Attention! Do *not* add additional new wrappers for CPU features (e.g. like + * the ri_allowed() wrapper) via this mechanism anymore. CPU features should + * be handled via the CPU models, i.e. checking with cpu_model_allowed() during + * CPU initialization and s390_has_feat() later should be sufficient. + */ static S390CcwMachineClass *get_machine_class(void) { if (unlikely(!current_mc)) { @@ -521,19 +533,16 @@ static S390CcwMachineClass *get_machine_class(void) bool ri_allowed(void) { - /* for "none" machine this results in true */ return get_machine_class()->ri_allowed; } bool cpu_model_allowed(void) { - /* for "none" machine this results in true */ return get_machine_class()->cpu_model_allowed; } bool hpage_1m_allowed(void) { - /* for "none" machine this results in true */ return get_machine_class()->hpage_1m_allowed; }
While working on the "Enable adapter interruption suppression again" recently, I had to discover that the meaning of get_machine_class() and the related *_allowed() wrappers is not very obvious. Add a more verbose comment here to clarify how these should be used. Signed-off-by: Thomas Huth <thuth@redhat.com> --- hw/s390x/s390-virtio-ccw.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)