Re: [PATCH] hw/s390x: Add a more verbose comment about get_machine_class() and the wrappers

[Thomas Huth]

On 23/01/2020 18.42, Cornelia Huck wrote:
> On Thu, 23 Jan 2020 18:02:56 +0100
> Thomas Huth <address@hidden> 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 <address@hidden>
>> ---
>>  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