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

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

On 3/8/21 3:01 PM, Pierre Morel wrote:
> 
> 
> 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.

Some people like to separate asm includes from other includes.

> But I can keep it if you want.

I just wanted to know if you removed it by mistake, if you want it that
way it's ok for me.

> 
>>
> ... 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
>