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

[Janosch Frank <frankja@linux.ibm.com>]

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.

> 
>  Thomas
>