Re: [kvm-unit-tests PATCH v2 5/6] s390x: Use interrupts in SCLP

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 05.12.18 20:27, David Hildenbrand wrote:
> On 05.12.18 16:39, Janosch Frank wrote:
>> We need to properly implement interrupt handling for SCLP, because on
>> z/VM and LPAR SCLP calls are not synchronous!
>>
>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
>> ---
[]
>> -
>>  static void sclp_set_write_mask(void)
>>  {
>>  	WriteEventMask *sccb = (void *)_sccb;
>>  
>> +	while (sclp_busy)
>> +		/* Wait for SCLP request to complete */;
> 
> should we use barrier(); here to make this clearer?

I could factor that out in sclp_wait_busy(), to make it one place to
change. I thought about using spinlocks, but whatever we do, we have to
be careful to not print when holding the lock.

Maybe we need a sclp spinlock/busy bust error path for exit with > 0,
like we do in the kernel on a panic.

> 
>> +	sclp_busy = true;
>>  	sccb->h.length = sizeof(WriteEventMask);
>>  	sccb->mask_length = sizeof(unsigned int);
>>  	sccb->receive_mask = SCLP_EVENT_MASK_MSG_ASCII;
>> @@ -57,6 +40,9 @@ void sclp_print(const char *str)
>>  	int len = strlen(str);
>>  	WriteEventData *sccb = (void *)_sccb;
>>  
>> +	while (sclp_busy)
>> +		/* Wait for SCLP request to complete */;
> 
> dito
> 
> (I guess do not we need barriers between setting sclp_busy and testing
> it, because we're using volatile and we don't have multi-threading
> messing with the value)
> 
>> +	sclp_busy = true;
>>  	sccb->h.length = sizeof(WriteEventData) + len;
>>  	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
>>  	sccb->ebh.length = sizeof(EventBufferHeader) + len;
>> @@ -65,4 +51,7 @@ void sclp_print(const char *str)
>>  	memcpy(sccb->data, str, len);
>>  
>>  	sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb);
>> +	while (sclp_busy)
>> +		/* Wait for SCLP request to complete */;
> 
> dito
> 
>> +
>>  }
>> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
>> index 1d4a010..cd0e5e5 100644
>> --- a/lib/s390x/sclp.c
>> +++ b/lib/s390x/sclp.c
>> @@ -23,6 +23,9 @@ static uint64_t storage_increment_size;
>>  static uint64_t max_ram_size;
>>  static uint64_t ram_size;
>>  
>> +char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));
>> +volatile bool sclp_busy = false;
> 
> false should be the default value already.

Yes, that's a leftover from the bss clearing problem, where it sometimes
was > 0...

> 
>> +
>>  static void mem_init(phys_addr_t mem_end)
>>  {
>>  	phys_addr_t freemem_start = (phys_addr_t)&stacktop;
>> @@ -30,6 +33,42 @@ static void mem_init(phys_addr_t mem_end)
>>  	phys_alloc_init(freemem_start, mem_end - freemem_start);
>>  }
>>  
>> +static void sclp_setup_int(void)
>> +{
>> +	uint64_t mask;
>> +
>> +	ctl_set_bit(0, 9);
>> +
>> +	mask = extract_psw_mask();
>> +	mask |= PSW_MASK_EXT;
>> +	load_psw_mask(mask);
>> +}
>> +
>> +void sclp_handle_ext(void)
>> +{
> 
> Error out if !sclp_busy but we get an interrupt?

Yes, but then I need to reset sclp_busy in the progress and we need to
hope that we get sclp output.

> 
>> +	ctl_clear_bit(0, 9);
>> +	sclp_busy = false;
>> +}
>> +
>> +/* Perform service call. Return 0 on success, non-zero otherwise. */
>> +int sclp_service_call(unsigned int command, void *sccb)
>> +{
>> +        int cc;
>> +
>> +	sclp_setup_int();
>> +        asm volatile(
>> +                "       .insn   rre,0xb2200000,%1,%2\n"  /* servc %1,%2 */
>> +                "       ipm     %0\n"
>> +                "       srl     %0,28"
>> +                : "=&d" (cc) : "d" (command), "a" (__pa(sccb))
>> +                : "cc", "memory");
>> +        if (cc == 3)
>> +                return -1;
>> +        if (cc == 2)
>> +                return -1;
>> +        return 0;
>> +}
> 
> Guess moving that (including _sccb) could also be a separate patch ;)

Will do

> 
>> +
>>  void sclp_memory_setup(void)
>>  {
>>  	ReadInfo *ri = (void *)_sccb;
>> @@ -37,7 +76,10 @@ void sclp_memory_setup(void)
>>  	int cc;
>>  
>>  	ri->h.length = SCCB_SIZE;
>> +	sclp_busy = true;
>>  	sclp_service_call(SCLP_CMDW_READ_SCP_INFO_FORCED, ri);
>> +	while (sclp_busy)
>> +		/* Wait for SCLP request to complete */;
> 
> Should we factor that out into something like

As written above, I'm all for the second one, but I dislike the start
one liner.

> 
> sclp_start_request()
> {
> 	sclp_busy = true;
> }
> 
> sclp_wait_response()
> {
> 	while (sclp_busy)
> 		barrier();
> }
> 
>>  
>>  	/* calculate the storage increment size */
>>  	rnsize = ri->rnsize;
>> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
>> index e008932..a91aad1 100644
>> --- a/lib/s390x/sclp.h
>> +++ b/lib/s390x/sclp.h
>> @@ -207,9 +207,11 @@ typedef struct ReadEventData {
>>      uint32_t mask;
>>  } __attribute__((packed)) ReadEventData;
>>  
>> +extern char _sccb[];
>> +volatile bool sclp_busy;
>> +void sclp_handle_ext(void);
>>  void sclp_console_setup(void);
>>  void sclp_print(const char *str);
>> -extern char _sccb[];
>>  int sclp_service_call(unsigned int command, void *sccb);
>>  void sclp_memory_setup(void);
>>  
>>
> 
> 


Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux