Re: [kvm-unit-tests PATCH v4 1/6] s390x: css: Store CSS Characteristics

[Pierre Morel <pmorel@linux.ibm.com>]

On 3/1/21 3:45 PM, Janosch Frank wrote:
> On 3/1/21 12:47 PM, Pierre Morel wrote:
>> CSS characteristics exposes the features of the Channel SubSystem.
>> Let's use Store Channel Subsystem Characteristics to retrieve
>> the features of the CSS.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> 
> Small nits below
> 
>> ---
>>   lib/s390x/css.h     | 67 ++++++++++++++++++++++++++++++++
>>   lib/s390x/css_lib.c | 93 ++++++++++++++++++++++++++++++++++++++++++++-
>>   s390x/css.c         |  8 ++++
>>   3 files changed, 167 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> index 3e57445..4210472 100644
>> --- a/lib/s390x/css.h
>> +++ b/lib/s390x/css.h
>> @@ -288,4 +288,71 @@ int css_residual_count(unsigned int schid);
>>   void enable_io_isc(uint8_t isc);
>>   int wait_and_check_io_completion(int schid);
>>   
>> +/*
>> + * CHSC definitions
>> + */
>> +struct chsc_header {
>> +	uint16_t len;
>> +	uint16_t code;
>> +};
>> +
>> +/* Store Channel Subsystem Characteristics */
>> +struct chsc_scsc {
>> +	struct chsc_header req;
>> +	uint16_t req_fmt;
>> +	uint8_t cssid;
>> +	uint8_t res_03;
>> +	uint32_t res_04[2];
> 
> I find the naming a bit weird and it could be one uint8_t field.

OK

> 
>> +	struct chsc_header res;
>> +	uint32_t res_fmt;
>> +	uint64_t general_char[255];
>> +	uint64_t chsc_char[254];
>> +};

... snip

>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index 3c24480..f46e871 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -15,11 +15,102 @@
>>   #include <asm/arch_def.h>
>>   #include <asm/time.h>
>>   #include <asm/arch_def.h>
>> -
>> +#include <alloc_page.h>
> 
> Did you intend to remove the newline here?

Yes I do not see why we should have a new line here.
But I can keep it if you want.

> 
... snip...

>>   #include <asm/arch_def.h>
>> +#include <alloc_page.h>
>>   
>>   #include <malloc_io.h>
>>   #include <css.h>
>> @@ -140,10 +141,17 @@ error_senseid:
>>   	unregister_io_int_func(css_irq_io);
>>   }
>>   
>> +static void css_init(void)
>> +{
>> +	report(!!get_chsc_scsc(), "Store Channel Characteristics");
> 
> get_chsc_scsc() returns a bool, so you shouldn't need the !! here, no?

Yes, forgotten when changed get_chsc_scsc(), remove the !!


Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen