Re: [kvm-unit-tests PATCH v2 6/8] s390x: css: testing ssch error response

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

On 3/25/21 5:02 PM, Claudio Imbrenda wrote:
> On Thu, 25 Mar 2021 10:39:05 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> Checking error response on various eroneous SSCH instructions:
>> - ORB alignment
>> - ORB above 2G
>> - CCW above 2G
>> - bad ORB flags
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     |   4 ++
>>   lib/s390x/css_lib.c |   5 +--
>>   s390x/css.c         | 105
>> ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 111
>> insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> index 1603781..e1e9264 100644
>> --- a/lib/s390x/css.h
>> +++ b/lib/s390x/css.h
>> @@ -90,6 +90,9 @@ struct scsw {
>>   #define SCSW_ESW_FORMAT		0x04000000
>>   #define SCSW_SUSPEND_CTRL	0x08000000
>>   #define SCSW_KEY		0xf0000000
>> +#define SCSW_SSCH_COMPLETED	(SCSW_CCW_FORMAT | SCSW_FC_START
>> | \
>> +				 SCSW_SC_PENDING | SCSW_SC_SECONDARY
>> | \
>> +				 SCSW_SC_PRIMARY)
>>   	uint32_t ctrl;
>>   	uint32_t ccw_addr;
>>   #define SCSW_DEVS_DEV_END	0x04
>> @@ -138,6 +141,7 @@ struct irb {
>>   	uint32_t ecw[8];
>>   	uint32_t emw[8];
>>   } __attribute__ ((aligned(4)));
>> +extern struct irb irb;
>>   
>>   #define CCW_CMD_SENSE_ID	0xe4
>>   #define CSS_SENSEID_COMMON_LEN	8
>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index 55e70e6..7c93e94 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -21,6 +21,7 @@
>>   
>>   struct schib schib;
>>   struct chsc_scsc *chsc_scsc;
>> +struct irb irb;
>>   
>>   static const char * const chsc_rsp_description[] = {
>>   	"CHSC unknown error",
>> @@ -415,8 +416,6 @@ bool css_disable_mb(int schid)
>>   	return retry_count > 0;
>>   }
>>   
>> -static struct irb irb;
>> -
>>   void css_irq_io(void)
>>   {
>>   	int ret = 0;
>> @@ -512,7 +511,7 @@ int check_io_completion(int schid, uint32_t ctrl)
>>   
>>   	report_prefix_push("check I/O completion");
>>   
>> -	if (lowcore_ptr->io_int_param != schid) {
>> +	if (!ctrl && lowcore_ptr->io_int_param != schid) {
>>   		report(0, "interrupt parameter: expected %08x got
>> %08x", schid, lowcore_ptr->io_int_param);
>>   		ret = -1;
>> diff --git a/s390x/css.c b/s390x/css.c
>> index 57dc340..f6890f2 100644
>> --- a/s390x/css.c
>> +++ b/s390x/css.c
>> @@ -15,6 +15,7 @@
>>   #include <interrupt.h>
>>   #include <asm/arch_def.h>
>>   #include <alloc_page.h>
>> +#include <sclp.h>
>>   
>>   #include <malloc_io.h>
>>   #include <css.h>
>> @@ -55,6 +56,109 @@ static void test_enable(void)
>>   	report(cc == 0, "Enable subchannel %08x", test_device_sid);
>>   }
>>   
>> +static void test_ssch(void)
>> +{
>> +	struct orb orb = {
>> +		.intparm = test_device_sid,
>> +		.ctrl = ORB_CTRL_ISIC | ORB_CTRL_FMT | ORB_LPM_DFLT,
>> +	};
>> +	int i;
>> +	phys_addr_t top;
>> +
>> +	NODEV_SKIP(test_device_sid);
>> +
>> +	assert(css_enable(test_device_sid, 0) == 0);
>> +
>> +	/* 1- ORB address should be aligned on 32 bits */
>> +	report_prefix_push("ORB alignment");
>> +	expect_pgm_int();
>> +	ssch(test_device_sid, (void *)0x110002);
> 
> I don't like using random hardcoded addresses. can you use a valid
> address for it? either allocate it or use a static buffer.

OK, I can make it differently.

> 
>> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +	report_prefix_pop();
>> +
>> +	/* 2- ORB address should be lower than 2G */
>> +	report_prefix_push("ORB Address above 2G");
>> +	expect_pgm_int();
>> +	ssch(test_device_sid, (void *)0x80000000);
> 
> another hardcoded address... you should try allocating memory over 2G,
> and try to use it. put a check if there is enough memory, and skip if
> you do not have enough memory, like you did below

OK, you are right, here I can not know if the memory is available at 
this address.

> 
>> +	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
>> +	report_prefix_pop();
>> +
>> +	/* 3- ORB address should be available we check 1G*/
>> +	top = get_ram_size();
>> +	report_prefix_push("ORB Address must be available");
>> +	if (top < 0x40000000) {
>> +		expect_pgm_int();
>> +		ssch(test_device_sid, (void *)0x40000000);
>> +		check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
>> +	} else {
>> +		report_skip("guest started with more than 1G
>> memory");
> 
> this is what I meant above. you will need to run this test both with 1G
> and with 3G of ram (look at the SCLP test, it has the same issue)

OK thanks, will take example from it.

> 
>> +	}
>> +	report_prefix_pop();
>> +
>> +	/* 3- ORB address should not be equal or above 2G */
> 
> the comment seems the same as the one above, maybe clarify that here
> you mean the address inside the ORB, instead of the address of the ORB
> itself

cut and past and forgot to adapt: here I check the CCW address not the 
ORB address

> 
>> +	report_prefix_push("CCW address above 2G");
>> +	orb.cpa = 0x80000000;
> 
> and again, please no hardcoded values; put a check that enough memory
> is available, and allocate from >2GB

and again.. yes

> 
>> +	expect_pgm_int();
>> +	ssch(test_device_sid, &orb);
>> +	check_pgm_int_code(PGM_INT_CODE_OPERAND);
>> +	report_prefix_pop();
>> +
>> +	senseid = alloc_io_mem(sizeof(*senseid), 0);
>> +	assert(senseid);
>> +	orb.cpa = (uint64_t)ccw_alloc(CCW_CMD_SENSE_ID, senseid,
>> +				      sizeof(*senseid), CCW_F_SLI);
>> +	assert(orb.cpa);
>> +
>> +	/* 4- Start on a disabled subchannel */
>> +	report_prefix_push("Disabled subchannel");
>> +	assert(css_disable(test_device_sid) == 0);
>> +	report(ssch(test_device_sid, &orb) == 3, "CC = 3");
>> +	report_prefix_pop();
>> +
>> +	/* 5- MIDAW is not supported by the firmware */
>> +	report_prefix_push("ORB MIDAW unsupported");
>> +	assert(css_enable(test_device_sid, 0) == 0);
>> +	orb.ctrl |= ORB_CTRL_MIDAW;
>> +	expect_pgm_int();
>> +	ssch(test_device_sid, &orb);
>> +	check_pgm_int_code(PGM_INT_CODE_OPERAND);
>> +	report_prefix_pop();
>> +	orb.ctrl = 0;
>> +
>> +	/* 6-12- Check the reserved bits of the ORB CTRL field */
>> +	for (i = 0; i < 5; i++) {
>> +		char buffer[30];
>> +
>> +		orb.ctrl = (0x02 << i);
>> +		snprintf(buffer, 30, "ORB reserved ctrl flags %02x",
>> orb.ctrl);
>> +		report_prefix_push(buffer);
>> +		expect_pgm_int();
>> +		ssch(test_device_sid, &orb);
>> +		check_pgm_int_code(PGM_INT_CODE_OPERAND);
>> +		report_prefix_pop();
>> +	}
>> +
>> +	/* 13- check the reserved bits of the ORB flags */
>> +	report_prefix_push("ORB wrong ctrl flags");
>> +	orb.ctrl |= 0x040000;
> 
> do you need the magic constant, or can you define a name for it? (or is
> even a name already defined?)

I can define this

...snip...

Thanks for the comments,

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen