* [PATCH] hw/s390x: Add a more verbose comment about get_machine_class() and the wrappers
@ 2020-01-23 17:02 Thomas Huth
2020-01-23 17:42 ` Cornelia Huck
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Thomas Huth @ 2020-01-23 17:02 UTC (permalink / raw)
To: Cornelia Huck, David Hildenbrand, qemu-devel
Cc: Halil Pasic, Christian Borntraeger, qemu-s390x, Matthew Rosato
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;
}
--
2.18.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/s390x: Add a more verbose comment about get_machine_class() and the wrappers
2020-01-23 17:02 [PATCH] hw/s390x: Add a more verbose comment about get_machine_class() and the wrappers Thomas Huth
@ 2020-01-23 17:42 ` Cornelia Huck
2020-01-24 8:42 ` Thomas Huth
2020-01-24 14:30 ` David Hildenbrand
2020-01-24 14:38 ` Cornelia Huck
2 siblings, 1 reply; 5+ messages in thread
From: Cornelia Huck @ 2020-01-23 17:42 UTC (permalink / raw)
To: Thomas Huth
Cc: Matthew Rosato, David Hildenbrand, qemu-devel, Halil Pasic,
Christian Borntraeger, qemu-s390x
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.)
> + * 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/s390x: Add a more verbose comment about get_machine_class() and the wrappers
2020-01-23 17:42 ` Cornelia Huck
@ 2020-01-24 8:42 ` Thomas Huth
0 siblings, 0 replies; 5+ messages in thread
From: Thomas Huth @ 2020-01-24 8:42 UTC (permalink / raw)
To: Cornelia Huck
Cc: Matthew Rosato, David Hildenbrand, qemu-devel, Halil Pasic,
Christian Borntraeger, qemu-s390x
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/s390x: Add a more verbose comment about get_machine_class() and the wrappers
2020-01-23 17:02 [PATCH] hw/s390x: Add a more verbose comment about get_machine_class() and the wrappers Thomas Huth
2020-01-23 17:42 ` Cornelia Huck
@ 2020-01-24 14:30 ` David Hildenbrand
2020-01-24 14:38 ` Cornelia Huck
2 siblings, 0 replies; 5+ messages in thread
From: David Hildenbrand @ 2020-01-24 14:30 UTC (permalink / raw)
To: Thomas Huth, Cornelia Huck, qemu-devel
Cc: Halil Pasic, Christian Borntraeger, qemu-s390x, Matthew Rosato
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>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/s390x: Add a more verbose comment about get_machine_class() and the wrappers
2020-01-23 17:02 [PATCH] hw/s390x: Add a more verbose comment about get_machine_class() and the wrappers Thomas Huth
2020-01-23 17:42 ` Cornelia Huck
2020-01-24 14:30 ` David Hildenbrand
@ 2020-01-24 14:38 ` Cornelia Huck
2 siblings, 0 replies; 5+ messages in thread
From: Cornelia Huck @ 2020-01-24 14:38 UTC (permalink / raw)
To: Thomas Huth
Cc: Matthew Rosato, David Hildenbrand, qemu-devel, Halil Pasic,
Christian Borntraeger, qemu-s390x
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-01-24 14:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-23 17:02 [PATCH] hw/s390x: Add a more verbose comment about get_machine_class() and the wrappers Thomas Huth
2020-01-23 17:42 ` Cornelia Huck
2020-01-24 8:42 ` Thomas Huth
2020-01-24 14:30 ` David Hildenbrand
2020-01-24 14:38 ` Cornelia Huck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.