Re: [kvm-unit-tests PATCH v3 7/8] s390x: Add diag318 intercept test

[Thomas Huth <thuth@redhat.com>]

On 17/12/2020 16.31, Janosch Frank wrote:
> On 12/17/20 3:31 PM, Janosch Frank wrote:
>> On 12/17/20 11:34 AM, Thomas Huth wrote:
>>> On 17/12/2020 10.59, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 17.12.20 10:53, Thomas Huth wrote:
>>>>> On 11/12/2020 11.00, Janosch Frank wrote:
>>>>>> Not much to test except for the privilege and specification
>>>>>> exceptions.
>>>>>>
>>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>>>> ---
>>>>>>  lib/s390x/sclp.c  |  2 ++
>>>>>>  lib/s390x/sclp.h  |  6 +++++-
>>>>>>  s390x/intercept.c | 19 +++++++++++++++++++
>>>>>>  3 files changed, 26 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
>>>>>> index cf6ea7c..0001993 100644
>>>>>> --- a/lib/s390x/sclp.c
>>>>>> +++ b/lib/s390x/sclp.c
>>>>>> @@ -138,6 +138,8 @@ void sclp_facilities_setup(void)
>>>>>>  
>>>>>>  	assert(read_info);
>>>>>>  
>>>>>> +	sclp_facilities.has_diag318 = read_info->byte_134_diag318;
>>>>>> +
>>>>>>  	cpu = (void *)read_info + read_info->offset_cpu;
>>>>>>  	for (i = 0; i < read_info->entries_cpu; i++, cpu++) {
>>>>>>  		if (cpu->address == cpu0_addr) {
>>>>>> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
>>>>>> index 6c86037..58f8e54 100644
>>>>>> --- a/lib/s390x/sclp.h
>>>>>> +++ b/lib/s390x/sclp.h
>>>>>> @@ -105,7 +105,8 @@ extern struct sclp_facilities sclp_facilities;
>>>>>>  
>>>>>>  struct sclp_facilities {
>>>>>>  	uint64_t has_sief2 : 1;
>>>>>> -	uint64_t : 63;
>>>>>> +	uint64_t has_diag318 : 1;
>>>>>> +	uint64_t : 62;
>>>>>>  };
>>>>>>  
>>>>>>  typedef struct ReadInfo {
>>>>>> @@ -130,6 +131,9 @@ typedef struct ReadInfo {
>>>>>>      uint16_t highest_cpu;
>>>>>>      uint8_t  _reserved5[124 - 122];     /* 122-123 */
>>>>>>      uint32_t hmfai;
>>>>>> +    uint8_t reserved7[134 - 128];
>>>>>> +    uint8_t byte_134_diag318 : 1;
>>>>>> +    uint8_t : 7;
>>>>>>      struct CPUEntry entries[0];
>>>>>
>>>>> ... the entries[] array can be moved around here without any further ado?
>>>>> Looks confusing to me. Should there be a CPUEntry array here at all, or only
>>>>> in ReadCpuInfo?
>>>>
>>>> there is offset_cpu for the cpu entries at the beginning of the structure.
>>>
>>> Ah, thanks, right, this was used earlier in the patch series, now I
>>> remember. But I think the "struct CPUEntry entries[0]" here is rather
>>> confusing, since there is no guarantee that the entries are really at this
>>> location ... I think this line should rather be replaced by a comment saying
>>> that offset_cpu should be used instead.
>>
>> Sure, as long as it's clear that there's something at the end, I'm fine
>> with it.
> 
> I would add that to the "fix style issues" patch or into an own patch.
> Any preferences?

I think a separate patch is cleaner.

> -       struct CPUEntry entries[0];
> +       /*
> +        * The cpu entries follow, they start at the offset specified
> +        * in offset_cpu.
> +        */

Sounds good, thanks!

 Thomas