qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 1/1] 390x/cpumodel: document S390FeatDef.bit not applicable
@ 2018-02-20 15:07 Halil Pasic
  2018-02-20 15:53 ` [Qemu-devel] [qemu-s390x] " Cornelia Huck
  0 siblings, 1 reply; 10+ messages in thread
From: Halil Pasic @ 2018-02-20 15:07 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Alexander Graf, Richard Henderson, Christian Borntraeger,
	qemu-devel, qemu-s390x, Halil Pasic

The 'bit' field of the 'S390FeatDef' structure is not applicable to all
it's instances. Currently a this field is not applicable, and remains
unused, iff the feature is of type S390_FEAT_TYPE_MISC. Having the value 0
specified for multiple such feature definition  was a little confusing,
as it's a perfectly legit bit value, and as usually the value of the bit
field is ought to be unique for each feature.

Let's document this, and hopefully reduce the potential for confusion.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---

Hi!

This may be an overkill. A comment where the misc features
are defined would do to, but I think this is nicer. So
I decided to try it with this approach first.

---
 target/s390x/cpu_features.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
index a5619f2893..34fddfe78b 100644
--- a/target/s390x/cpu_features.c
+++ b/target/s390x/cpu_features.c
@@ -23,6 +23,13 @@
         .desc = _desc,                               \
     }
 
+/*
+ * For some feature types (e.g. S390_FEAT_TYPE_MISC) S390FeatDef.bit
+ * is not applicable, as there is no corresponding feature block. See
+ * s390_fill_feat_block() and it's usages.
+ */
+#define FEAT_BIT_NA -1
+
 /* indexed by feature number for easy lookup */
 static const S390FeatDef s390_features[] = {
     FEAT_INIT("esan3", S390_FEAT_TYPE_STFL, 0, "Instructions marked as n3"),
@@ -123,8 +130,8 @@ static const S390FeatDef s390_features[] = {
     FEAT_INIT("ib", S390_FEAT_TYPE_SCLP_CPU, 42, "SIE: Intervention bypass facility"),
     FEAT_INIT("cei", S390_FEAT_TYPE_SCLP_CPU, 43, "SIE: Conditional-external-interception facility"),
 
-    FEAT_INIT("dateh2", S390_FEAT_TYPE_MISC, 0, "DAT-enhancement facility 2"),
-    FEAT_INIT("cmm", S390_FEAT_TYPE_MISC, 0, "Collaborative-memory-management facility"),
+    FEAT_INIT("dateh2", S390_FEAT_TYPE_MISC, FEAT_BIT_NA, "DAT-enhancement facility 2"),
+    FEAT_INIT("cmm", S390_FEAT_TYPE_MISC, FEAT_BIT_NA, "Collaborative-memory-management facility"),
 
     FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32 bit in general registers)"),
     FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64 bit in parameter list)"),
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [qemu-s390x] [PATCH 1/1] 390x/cpumodel: document S390FeatDef.bit not applicable
  2018-02-20 15:07 [Qemu-devel] [PATCH 1/1] 390x/cpumodel: document S390FeatDef.bit not applicable Halil Pasic
@ 2018-02-20 15:53 ` Cornelia Huck
  2018-02-20 15:55   ` David Hildenbrand
  2018-02-20 16:19   ` Halil Pasic
  0 siblings, 2 replies; 10+ messages in thread
From: Cornelia Huck @ 2018-02-20 15:53 UTC (permalink / raw)
  To: Halil Pasic
  Cc: David Hildenbrand, qemu-devel, Alexander Graf,
	Christian Borntraeger, qemu-s390x, Richard Henderson

On Tue, 20 Feb 2018 16:07:13 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> The 'bit' field of the 'S390FeatDef' structure is not applicable to all
> it's instances. Currently a this field is not applicable, and remains

s/it's/its/

s/a this/this/

> unused, iff the feature is of type S390_FEAT_TYPE_MISC. Having the value 0
> specified for multiple such feature definition  was a little confusing,
> as it's a perfectly legit bit value, and as usually the value of the bit
> field is ought to be unique for each feature.
> 
> Let's document this, and hopefully reduce the potential for confusion.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> 
> Hi!
> 
> This may be an overkill. A comment where the misc features
> are defined would do to, but I think this is nicer. So
> I decided to try it with this approach first.

Is there likely to be anything else than FEAT_MISC _not_ using .bit? If
not, would it be better to at a comment to the FEAT_MISC definition?

> 
> ---
>  target/s390x/cpu_features.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index a5619f2893..34fddfe78b 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -23,6 +23,13 @@
>          .desc = _desc,                               \
>      }
>  
> +/*
> + * For some feature types (e.g. S390_FEAT_TYPE_MISC) S390FeatDef.bit
> + * is not applicable, as there is no corresponding feature block. See
> + * s390_fill_feat_block() and it's usages.
> + */
> +#define FEAT_BIT_NA -1
> +
>  /* indexed by feature number for easy lookup */
>  static const S390FeatDef s390_features[] = {
>      FEAT_INIT("esan3", S390_FEAT_TYPE_STFL, 0, "Instructions marked as n3"),
> @@ -123,8 +130,8 @@ static const S390FeatDef s390_features[] = {
>      FEAT_INIT("ib", S390_FEAT_TYPE_SCLP_CPU, 42, "SIE: Intervention bypass facility"),
>      FEAT_INIT("cei", S390_FEAT_TYPE_SCLP_CPU, 43, "SIE: Conditional-external-interception facility"),
>  
> -    FEAT_INIT("dateh2", S390_FEAT_TYPE_MISC, 0, "DAT-enhancement facility 2"),
> -    FEAT_INIT("cmm", S390_FEAT_TYPE_MISC, 0, "Collaborative-memory-management facility"),
> +    FEAT_INIT("dateh2", S390_FEAT_TYPE_MISC, FEAT_BIT_NA, "DAT-enhancement facility 2"),
> +    FEAT_INIT("cmm", S390_FEAT_TYPE_MISC, FEAT_BIT_NA, "Collaborative-memory-management facility"),
>  
>      FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32 bit in general registers)"),
>      FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64 bit in parameter list)"),

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [qemu-s390x] [PATCH 1/1] 390x/cpumodel: document S390FeatDef.bit not applicable
  2018-02-20 15:53 ` [Qemu-devel] [qemu-s390x] " Cornelia Huck
@ 2018-02-20 15:55   ` David Hildenbrand
  2018-02-20 16:04     ` Christian Borntraeger
  2018-02-20 16:19   ` Halil Pasic
  1 sibling, 1 reply; 10+ messages in thread
From: David Hildenbrand @ 2018-02-20 15:55 UTC (permalink / raw)
  To: Cornelia Huck, Halil Pasic
  Cc: qemu-devel, Alexander Graf, Christian Borntraeger, qemu-s390x,
	Richard Henderson

On 20.02.2018 16:53, Cornelia Huck wrote:
> On Tue, 20 Feb 2018 16:07:13 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> The 'bit' field of the 'S390FeatDef' structure is not applicable to all
>> it's instances. Currently a this field is not applicable, and remains
> 
> s/it's/its/
> 
> s/a this/this/
> 
>> unused, iff the feature is of type S390_FEAT_TYPE_MISC. Having the value 0
>> specified for multiple such feature definition  was a little confusing,
>> as it's a perfectly legit bit value, and as usually the value of the bit
>> field is ought to be unique for each feature.
>>
>> Let's document this, and hopefully reduce the potential for confusion.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>
>> Hi!
>>
>> This may be an overkill. A comment where the misc features
>> are defined would do to, but I think this is nicer. So
>> I decided to try it with this approach first.
> 
> Is there likely to be anything else than FEAT_MISC _not_ using .bit? If
> not, would it be better to at a comment to the FEAT_MISC definition?

Doubt it right now. I would sign the "overkill" part :)

-- 

Thanks,

David / dhildenb

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [qemu-s390x] [PATCH 1/1] 390x/cpumodel: document S390FeatDef.bit not applicable
  2018-02-20 15:55   ` David Hildenbrand
@ 2018-02-20 16:04     ` Christian Borntraeger
  2018-02-20 16:07       ` Cornelia Huck
  0 siblings, 1 reply; 10+ messages in thread
From: Christian Borntraeger @ 2018-02-20 16:04 UTC (permalink / raw)
  To: David Hildenbrand, Cornelia Huck, Halil Pasic
  Cc: qemu-devel, Alexander Graf, qemu-s390x, Richard Henderson



On 02/20/2018 04:55 PM, David Hildenbrand wrote:
> On 20.02.2018 16:53, Cornelia Huck wrote:
>> On Tue, 20 Feb 2018 16:07:13 +0100
>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>
>>> The 'bit' field of the 'S390FeatDef' structure is not applicable to all
>>> it's instances. Currently a this field is not applicable, and remains
>>
>> s/it's/its/
>>
>> s/a this/this/
>>
>>> unused, iff the feature is of type S390_FEAT_TYPE_MISC. Having the value 0
>>> specified for multiple such feature definition  was a little confusing,
>>> as it's a perfectly legit bit value, and as usually the value of the bit
>>> field is ought to be unique for each feature.
>>>
>>> Let's document this, and hopefully reduce the potential for confusion.
>>>
>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>> ---
>>>
>>> Hi!
>>>
>>> This may be an overkill. A comment where the misc features
>>> are defined would do to, but I think this is nicer. So
>>> I decided to try it with this approach first.
>>
>> Is there likely to be anything else than FEAT_MISC _not_ using .bit? If
>> not, would it be better to at a comment to the FEAT_MISC definition?
> 
> Doubt it right now. I would sign the "overkill" part :)

I can cconfirm that this code caused some questions and it took me some
minutes to remember why 0 and 0 was ok. So I certainly want to have a comment
of some form.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [qemu-s390x] [PATCH 1/1] 390x/cpumodel: document S390FeatDef.bit not applicable
  2018-02-20 16:04     ` Christian Borntraeger
@ 2018-02-20 16:07       ` Cornelia Huck
  2018-02-20 16:08         ` David Hildenbrand
  0 siblings, 1 reply; 10+ messages in thread
From: Cornelia Huck @ 2018-02-20 16:07 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: David Hildenbrand, Halil Pasic, qemu-devel, Alexander Graf,
	qemu-s390x, Richard Henderson

On Tue, 20 Feb 2018 17:04:19 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 02/20/2018 04:55 PM, David Hildenbrand wrote:
> > On 20.02.2018 16:53, Cornelia Huck wrote:  
> >> On Tue, 20 Feb 2018 16:07:13 +0100
> >> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >>  
> >>> The 'bit' field of the 'S390FeatDef' structure is not applicable to all
> >>> it's instances. Currently a this field is not applicable, and remains  
> >>
> >> s/it's/its/
> >>
> >> s/a this/this/
> >>  
> >>> unused, iff the feature is of type S390_FEAT_TYPE_MISC. Having the value 0
> >>> specified for multiple such feature definition  was a little confusing,
> >>> as it's a perfectly legit bit value, and as usually the value of the bit
> >>> field is ought to be unique for each feature.
> >>>
> >>> Let's document this, and hopefully reduce the potential for confusion.
> >>>
> >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >>> ---
> >>>
> >>> Hi!
> >>>
> >>> This may be an overkill. A comment where the misc features
> >>> are defined would do to, but I think this is nicer. So
> >>> I decided to try it with this approach first.  
> >>
> >> Is there likely to be anything else than FEAT_MISC _not_ using .bit? If
> >> not, would it be better to at a comment to the FEAT_MISC definition?  
> > 
> > Doubt it right now. I would sign the "overkill" part :)  
> 
> I can cconfirm that this code caused some questions and it took me some
> minutes to remember why 0 and 0 was ok. So I certainly want to have a comment
> of some form.
> 

I'd prefer a comment about FEAT_MISC usage rather than a magic value.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [qemu-s390x] [PATCH 1/1] 390x/cpumodel: document S390FeatDef.bit not applicable
  2018-02-20 16:07       ` Cornelia Huck
@ 2018-02-20 16:08         ` David Hildenbrand
  2018-02-20 16:25           ` Cornelia Huck
  2018-02-20 16:32           ` Halil Pasic
  0 siblings, 2 replies; 10+ messages in thread
From: David Hildenbrand @ 2018-02-20 16:08 UTC (permalink / raw)
  To: Cornelia Huck, Christian Borntraeger
  Cc: Halil Pasic, qemu-devel, Alexander Graf, qemu-s390x,
	Richard Henderson

On 20.02.2018 17:07, Cornelia Huck wrote:
> On Tue, 20 Feb 2018 17:04:19 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 02/20/2018 04:55 PM, David Hildenbrand wrote:
>>> On 20.02.2018 16:53, Cornelia Huck wrote:  
>>>> On Tue, 20 Feb 2018 16:07:13 +0100
>>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>>  
>>>>> The 'bit' field of the 'S390FeatDef' structure is not applicable to all
>>>>> it's instances. Currently a this field is not applicable, and remains  
>>>>
>>>> s/it's/its/
>>>>
>>>> s/a this/this/
>>>>  
>>>>> unused, iff the feature is of type S390_FEAT_TYPE_MISC. Having the value 0
>>>>> specified for multiple such feature definition  was a little confusing,
>>>>> as it's a perfectly legit bit value, and as usually the value of the bit
>>>>> field is ought to be unique for each feature.
>>>>>
>>>>> Let's document this, and hopefully reduce the potential for confusion.
>>>>>
>>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>>> ---
>>>>>
>>>>> Hi!
>>>>>
>>>>> This may be an overkill. A comment where the misc features
>>>>> are defined would do to, but I think this is nicer. So
>>>>> I decided to try it with this approach first.  
>>>>
>>>> Is there likely to be anything else than FEAT_MISC _not_ using .bit? If
>>>> not, would it be better to at a comment to the FEAT_MISC definition?  
>>>
>>> Doubt it right now. I would sign the "overkill" part :)  
>>
>> I can cconfirm that this code caused some questions and it took me some
>> minutes to remember why 0 and 0 was ok. So I certainly want to have a comment
>> of some form.
>>
> 
> I'd prefer a comment about FEAT_MISC usage rather than a magic value.
> 

We can also add FEAT_INIT_MISC. And add a comment in the initializer.

-- 

Thanks,

David / dhildenb

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [qemu-s390x] [PATCH 1/1] 390x/cpumodel: document S390FeatDef.bit not applicable
  2018-02-20 15:53 ` [Qemu-devel] [qemu-s390x] " Cornelia Huck
  2018-02-20 15:55   ` David Hildenbrand
@ 2018-02-20 16:19   ` Halil Pasic
  1 sibling, 0 replies; 10+ messages in thread
From: Halil Pasic @ 2018-02-20 16:19 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: David Hildenbrand, qemu-devel, Alexander Graf,
	Christian Borntraeger, qemu-s390x, Richard Henderson



On 02/20/2018 04:53 PM, Cornelia Huck wrote:
> On Tue, 20 Feb 2018 16:07:13 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> The 'bit' field of the 'S390FeatDef' structure is not applicable to all
>> it's instances. Currently a this field is not applicable, and remains
> 
> s/it's/its/
> 
> s/a this/this/

Will do.

> 
>> unused, iff the feature is of type S390_FEAT_TYPE_MISC. Having the value 0
>> specified for multiple such feature definition  was a little confusing,
>> as it's a perfectly legit bit value, and as usually the value of the bit
>> field is ought to be unique for each feature.
>>
>> Let's document this, and hopefully reduce the potential for confusion.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>
>> Hi!
>>
>> This may be an overkill. A comment where the misc features
>> are defined would do to, but I think this is nicer. So
>> I decided to try it with this approach first.
> 
> Is there likely to be anything else than FEAT_MISC _not_ using .bit? If
> not, would it be better to at a comment to the FEAT_MISC definition?
> 

I think, that anything not using .bit is likely to get modeled as
FEAT_MISC that is why I think a comment to where the FEAT_MISC features
are defined is an option. I don't think commenting the definition
of S390_FEAT_TYPE_MISC (that is the definition of enum S390FeatType) would help,
as what looks weird is the definition of the features (that
currently have all the .bit value 0, but that does not matter).

The point is we need to supply 'bit' to  FEAT_INIT() because of
formal reasons, even if it makes no sense to specify 'bit' for the
given feature.

I've done what I've done because it seemed the most idiomatic solution
(we used to call such 'special values' extremal elements in school).
But I'm fine with commenting the definitions (look for [HERE] below).

>>
>> ---
>>  target/s390x/cpu_features.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
>> index a5619f2893..34fddfe78b 100644
>> --- a/target/s390x/cpu_features.c
>> +++ b/target/s390x/cpu_features.c
>> @@ -23,6 +23,13 @@
>>          .desc = _desc,                               \
>>      }
>>  
>> +/*
>> + * For some feature types (e.g. S390_FEAT_TYPE_MISC) S390FeatDef.bit
>> + * is not applicable, as there is no corresponding feature block. See
>> + * s390_fill_feat_block() and it's usages.
>> + */
>> +#define FEAT_BIT_NA -1
>> +
>>  /* indexed by feature number for easy lookup */
>>  static const S390FeatDef s390_features[] = {
>>      FEAT_INIT("esan3", S390_FEAT_TYPE_STFL, 0, "Instructions marked as n3"),
>> @@ -123,8 +130,8 @@ static const S390FeatDef s390_features[] = {
>>      FEAT_INIT("ib", S390_FEAT_TYPE_SCLP_CPU, 42, "SIE: Intervention bypass facility"),
>>      FEAT_INIT("cei", S390_FEAT_TYPE_SCLP_CPU, 43, "SIE: Conditional-external-interception facility"),
>>  
[HERE]
>> -    FEAT_INIT("dateh2", S390_FEAT_TYPE_MISC, 0, "DAT-enhancement facility 2"),
>> -    FEAT_INIT("cmm", S390_FEAT_TYPE_MISC, 0, "Collaborative-memory-management facility"),
>> +    FEAT_INIT("dateh2", S390_FEAT_TYPE_MISC, FEAT_BIT_NA, "DAT-enhancement facility 2"),
>> +    FEAT_INIT("cmm", S390_FEAT_TYPE_MISC, FEAT_BIT_NA, "Collaborative-memory-management facility"),
>>  
>>      FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32 bit in general registers)"),
>>      FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64 bit in parameter list)"),
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [qemu-s390x] [PATCH 1/1] 390x/cpumodel: document S390FeatDef.bit not applicable
  2018-02-20 16:08         ` David Hildenbrand
@ 2018-02-20 16:25           ` Cornelia Huck
  2018-02-21 12:11             ` Halil Pasic
  2018-02-20 16:32           ` Halil Pasic
  1 sibling, 1 reply; 10+ messages in thread
From: Cornelia Huck @ 2018-02-20 16:25 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Christian Borntraeger, Halil Pasic, qemu-devel, Alexander Graf,
	qemu-s390x, Richard Henderson

On Tue, 20 Feb 2018 17:08:52 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 20.02.2018 17:07, Cornelia Huck wrote:
> > On Tue, 20 Feb 2018 17:04:19 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> On 02/20/2018 04:55 PM, David Hildenbrand wrote:  
> >>> On 20.02.2018 16:53, Cornelia Huck wrote:    
> >>>> On Tue, 20 Feb 2018 16:07:13 +0100
> >>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >>>>    
> >>>>> The 'bit' field of the 'S390FeatDef' structure is not applicable to all
> >>>>> it's instances. Currently a this field is not applicable, and remains    
> >>>>
> >>>> s/it's/its/
> >>>>
> >>>> s/a this/this/
> >>>>    
> >>>>> unused, iff the feature is of type S390_FEAT_TYPE_MISC. Having the value 0
> >>>>> specified for multiple such feature definition  was a little confusing,
> >>>>> as it's a perfectly legit bit value, and as usually the value of the bit
> >>>>> field is ought to be unique for each feature.
> >>>>>
> >>>>> Let's document this, and hopefully reduce the potential for confusion.
> >>>>>
> >>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >>>>> ---
> >>>>>
> >>>>> Hi!
> >>>>>
> >>>>> This may be an overkill. A comment where the misc features
> >>>>> are defined would do to, but I think this is nicer. So
> >>>>> I decided to try it with this approach first.    
> >>>>
> >>>> Is there likely to be anything else than FEAT_MISC _not_ using .bit? If
> >>>> not, would it be better to at a comment to the FEAT_MISC definition?    
> >>>
> >>> Doubt it right now. I would sign the "overkill" part :)    
> >>
> >> I can cconfirm that this code caused some questions and it took me some
> >> minutes to remember why 0 and 0 was ok. So I certainly want to have a comment
> >> of some form.
> >>  
> > 
> > I'd prefer a comment about FEAT_MISC usage rather than a magic value.
> >   
> 
> We can also add FEAT_INIT_MISC. And add a comment in the initializer.
> 

That's what I like best.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [qemu-s390x] [PATCH 1/1] 390x/cpumodel: document S390FeatDef.bit not applicable
  2018-02-20 16:08         ` David Hildenbrand
  2018-02-20 16:25           ` Cornelia Huck
@ 2018-02-20 16:32           ` Halil Pasic
  1 sibling, 0 replies; 10+ messages in thread
From: Halil Pasic @ 2018-02-20 16:32 UTC (permalink / raw)
  To: qemu-devel



On 02/20/2018 05:08 PM, David Hildenbrand wrote:
> On 20.02.2018 17:07, Cornelia Huck wrote:
>> On Tue, 20 Feb 2018 17:04:19 +0100
>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>
>>> On 02/20/2018 04:55 PM, David Hildenbrand wrote:
>>>> On 20.02.2018 16:53, Cornelia Huck wrote:  
>>>>> On Tue, 20 Feb 2018 16:07:13 +0100
>>>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>>>  
>>>>>> The 'bit' field of the 'S390FeatDef' structure is not applicable to all
>>>>>> it's instances. Currently a this field is not applicable, and remains  
>>>>>
>>>>> s/it's/its/
>>>>>
>>>>> s/a this/this/
>>>>>  
>>>>>> unused, iff the feature is of type S390_FEAT_TYPE_MISC. Having the value 0
>>>>>> specified for multiple such feature definition  was a little confusing,
>>>>>> as it's a perfectly legit bit value, and as usually the value of the bit
>>>>>> field is ought to be unique for each feature.
>>>>>>
>>>>>> Let's document this, and hopefully reduce the potential for confusion.
>>>>>>
>>>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>
>>>>>> Hi!
>>>>>>
>>>>>> This may be an overkill. A comment where the misc features
>>>>>> are defined would do to, but I think this is nicer. So
>>>>>> I decided to try it with this approach first.  
>>>>>
>>>>> Is there likely to be anything else than FEAT_MISC _not_ using .bit? If
>>>>> not, would it be better to at a comment to the FEAT_MISC definition?  
>>>>
>>>> Doubt it right now. I would sign the "overkill" part :)  
>>>
>>> I can cconfirm that this code caused some questions and it took me some
>>> minutes to remember why 0 and 0 was ok. So I certainly want to have a comment
>>> of some form.
>>>
>>
>> I'd prefer a comment about FEAT_MISC usage rather than a magic value.

As I said elsewhere such stuff used to be called 'extremal element' at
the university. We used the term 'magic value' for hard coded constants.
But the jargon can be different elsewhere.

>>
> 
> We can also add FEAT_INIT_MISC. And add a comment in the initializer.

I don't care that much about aesthetics. FEAT_INT_MISC would have the
benefit of making the definitions shorter. But whatever you decide on. 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [qemu-s390x] [PATCH 1/1] 390x/cpumodel: document S390FeatDef.bit not applicable
  2018-02-20 16:25           ` Cornelia Huck
@ 2018-02-21 12:11             ` Halil Pasic
  0 siblings, 0 replies; 10+ messages in thread
From: Halil Pasic @ 2018-02-21 12:11 UTC (permalink / raw)
  To: Cornelia Huck, David Hildenbrand
  Cc: qemu-devel, Alexander Graf, Christian Borntraeger, qemu-s390x,
	Richard Henderson



On 02/20/2018 05:25 PM, Cornelia Huck wrote:
> On Tue, 20 Feb 2018 17:08:52 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 20.02.2018 17:07, Cornelia Huck wrote:
>>> On Tue, 20 Feb 2018 17:04:19 +0100
>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>   
>>>> On 02/20/2018 04:55 PM, David Hildenbrand wrote:  
>>>>> On 20.02.2018 16:53, Cornelia Huck wrote:    
>>>>>> On Tue, 20 Feb 2018 16:07:13 +0100
>>>>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>>>>    
>>>>>>> The 'bit' field of the 'S390FeatDef' structure is not applicable to all
>>>>>>> it's instances. Currently a this field is not applicable, and remains    
>>>>>>
>>>>>> s/it's/its/
>>>>>>
>>>>>> s/a this/this/
>>>>>>    
>>>>>>> unused, iff the feature is of type S390_FEAT_TYPE_MISC. Having the value 0
>>>>>>> specified for multiple such feature definition  was a little confusing,
>>>>>>> as it's a perfectly legit bit value, and as usually the value of the bit
>>>>>>> field is ought to be unique for each feature.
>>>>>>>
>>>>>>> Let's document this, and hopefully reduce the potential for confusion.
>>>>>>>
>>>>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>>>>> ---
>>>>>>>
>>>>>>> Hi!
>>>>>>>
>>>>>>> This may be an overkill. A comment where the misc features
>>>>>>> are defined would do to, but I think this is nicer. So
>>>>>>> I decided to try it with this approach first.    
>>>>>>
>>>>>> Is there likely to be anything else than FEAT_MISC _not_ using .bit? If
>>>>>> not, would it be better to at a comment to the FEAT_MISC definition?    
>>>>>
>>>>> Doubt it right now. I would sign the "overkill" part :)    
>>>>
>>>> I can cconfirm that this code caused some questions and it took me some
>>>> minutes to remember why 0 and 0 was ok. So I certainly want to have a comment
>>>> of some form.
>>>>  
>>>
>>> I'd prefer a comment about FEAT_MISC usage rather than a magic value.
>>>   
>>
>> We can also add FEAT_INIT_MISC. And add a comment in the initializer.
>>
> 
> That's what I like best.
> 

OK, seems we have a winner: I will redo this with
#define FEAT_INIT_MISC(_name, _desc)                 \                          
            FEAT_INIT(_name, S390_FEAT_TYPE_MISC, 0, _desc)

Everybody thanks for the comments.

Regards,
Halil

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-02-21 12:11 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-20 15:07 [Qemu-devel] [PATCH 1/1] 390x/cpumodel: document S390FeatDef.bit not applicable Halil Pasic
2018-02-20 15:53 ` [Qemu-devel] [qemu-s390x] " Cornelia Huck
2018-02-20 15:55   ` David Hildenbrand
2018-02-20 16:04     ` Christian Borntraeger
2018-02-20 16:07       ` Cornelia Huck
2018-02-20 16:08         ` David Hildenbrand
2018-02-20 16:25           ` Cornelia Huck
2018-02-21 12:11             ` Halil Pasic
2018-02-20 16:32           ` Halil Pasic
2018-02-20 16:19   ` Halil Pasic

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).