[Skiboot] [EXTERNAL] [PATCH v6 13/29] xive: make endian-clean

[Nicholas Piggin]

Cédric Le Goater's on November 7, 2019 6:25 pm:
> On 07/11/2019 07:26, Nicholas Piggin wrote:
>> Cédric Le Goater's on November 7, 2019 1:38 am:
>>> On 06/11/2019 13:10, Nicholas Piggin wrote:
>>>> Convert xive opal calls, dt construction, and in-memory hardware tables
>>>> to use explicit endian conversions.
>>> Thanks for reworking the patch with helper routines. This is a huge 
>>> work. You are setting the standards pretty high for xive2 !
>> Well once I took your nice helpers then it's only mindless conversions.
> 
> May be we could generalize the use to other parts of your patchset ?

I had a bit of a look, nothing seems to use fields within words to the
extent that xive does.

> 
>> I thought I'd better do it rather than pull you away from useful work :)
> 
> xive2 does not have LE support currently ... I will do the conversion
> after some tests has been done first.
>  
>>> Did you run a KVM guest ? 
>> I just pulled the latest qemu and tried. Had some bugs in the
>> conversion, so here is an updated patch. I'll not re-send the series
>> until you get a chance to ack it, at least.
> 
> OK. 
> 
> Here are some remarks below. Nothing really important requiring a resend.

Thanks.

>> @@ -791,8 +791,8 @@ static void xive_init_default_vp(struct xive_vp *vp,
>>  	memset(vp, 0, sizeof(struct xive_vp));
>>  
>>  	/* Stash the EQ base in the pressure relief interrupt field */
>> -	vp->w1 = (eq_blk << 28) | eq_idx;
>> -	vp->w0 = VP_W0_VALID;
>> +	vp->w1 = cpu_to_be32((eq_blk << 28) | eq_idx);
> 
> This field is not architected in xive1 and we should had an helper like 
> in xive2 : xive_vp_set_end_base(). It can come later.
>  
>> +	vp->w0 = xive_set_field32(VP_W0_VALID, 0, 1);
>>  }
>>  
>>  static void xive_init_emu_eq(uint32_t vp_blk, uint32_t vp_idx,
>> @@ -801,18 +801,20 @@ static void xive_init_emu_eq(uint32_t vp_blk, uint32_t vp_idx,
>>  {
>>  	memset(eq, 0, sizeof(struct xive_eq));
>>  
>> -	eq->w1 = EQ_W1_GENERATION;
>> -	eq->w3 = ((uint64_t)backing_page) & 0xffffffff;
>> -	eq->w2 = (((uint64_t)backing_page)) >> 32 & 0x0fffffff;
>> -	eq->w6 = SETFIELD(EQ_W6_NVT_BLOCK, 0ul, vp_blk) |
>> -		SETFIELD(EQ_W6_NVT_INDEX, 0ul, vp_idx);
>> -	eq->w7 = SETFIELD(EQ_W7_F0_PRIORITY, 0ul, prio);
>> -	eq->w0 = EQ_W0_VALID | EQ_W0_ENQUEUE |
>> -		SETFIELD(EQ_W0_QSIZE, 0ul, EQ_QSIZE_64K) |
>> -		EQ_W0_FIRMWARE;
>> +	eq->w1 = xive_set_field32(EQ_W1_GENERATION, 0, 1);
>> +	eq->w3 = cpu_to_be32(((uint64_t)backing_page) & 0xffffffff);
>> +	eq->w2 = cpu_to_be32(((((uint64_t)backing_page)) >> 32) & 0x0fffffff);
> 
> I think we can remove one parenthesis and we should use the field 
> definitions like in opal_xive_set_queue_info() :
> 
> 	eq.w3 = ((uint64_t)qpage) & EQ_W3_OP_DESC_LO;
> 	eq.w2 = (((uint64_t)qpage) >> 32) & EQ_W2_OP_DESC_HI;
> 
> I should done that in my cleanup series but I missed the emulation layer. 
> My inconscient refuses to do it I think.
> 
> I can address that in a prereq patch if you prefer ? 

I don't mind before or after. If this is a missing bit from your
cleanup series sure send a fix for upstream and I can rebase.

>> +	eq->w6 = xive_set_field32(EQ_W6_NVT_BLOCK, 0, vp_blk) |
>> +		 xive_set_field32(EQ_W6_NVT_INDEX, 0, vp_idx);
>> +	eq->w7 = xive_set_field32(EQ_W7_F0_PRIORITY, 0, prio);
>> +	eq->w0 = xive_set_field32(EQ_W0_VALID, 0, 1) |
>> +		 xive_set_field32(EQ_W0_ENQUEUE, 0, 1) |
>> +		 xive_set_field32(EQ_W0_FIRMWARE, 0, 1) |
>> +		 xive_set_field32(EQ_W0_QSIZE, 0, EQ_QSIZE_64K) |
>>  #ifdef EQ_ALWAYS_NOTIFY
>> -	eq->w0 |= EQ_W0_UCOND_NOTIFY;
>> +		 xive_set_field32(EQ_W0_UCOND_NOTIFY, 0, 1) |
>>  #endif
>> +		 0 ;
>>  }
>>  
>>  static uint32_t *xive_get_eq_buf(uint32_t eq_blk, uint32_t eq_idx)
>> @@ -824,8 +826,8 @@ static uint32_t *xive_get_eq_buf(uint32_t eq_blk, uint32_t eq_idx)
>>  	assert(x);
>>  	eq = xive_get_eq(x, eq_idx);
>>  	assert(eq);
>> -	assert(eq->w0 & EQ_W0_VALID);
>> -	addr = (((uint64_t)eq->w2) & 0x0fffffff) << 32 | eq->w3;
>> +	assert(xive_get_field32(EQ_W0_VALID, eq->w0));
>> +	addr = ((((uint64_t)be32_to_cpu(eq->w2)) & 0x0fffffff) << 32) | be32_to_cpu(eq->w3);
> 
> This deserves a helper like in QEMU : 
> 
> static inline uint64_t xive_end_qaddr(XiveEND *end)
> {
>     return ((uint64_t) be32_to_cpu(end->w2) & 0x0fffffff) << 32 |
>         be32_to_cpu(end->w3);
> }

Sure. That can be done before or after as well.

>> -	eq_blk = vp->w1 >> 28;
>> -	eq_idx = vp->w1 & 0x0fffffff;
>> +	eq_blk = be32_to_cpu(vp->w1) >> 28;
>> +	eq_idx = be32_to_cpu(vp->w1) & 0x0fffffff;
> 
> 
> These are not architected fields but we should add defines. It can come later.

>> +		if (xive_get_field32(EQ_W0_ENQUEUE, eq->w0))
>> +			*out_qpage = cpu_to_be64(((uint64_t)xive_get_field32(EQ_W2_OP_DESC_HI, eq->w2) << 32) | be32_to_cpu(eq->w3));
> 
> we need a helper.

>>  		*out_qeoi_page =
>> -			(uint64_t)x->eq_mmio + idx * 0x20000;
>> +			cpu_to_be64((uint64_t)x->eq_mmio + idx * 0x20000);
> 
> There are plenty of 0x10000 values in the code and it would be clearer to use 
> XIVE_PAGE_SIZE or OPAL_PAGE_SIZE. Something for later.

>> -		eq_blk = vp->w1 >> 28;
>> -		eq_idx = vp->w1 & 0x0fffffff;
>> +		eq_blk = be32_to_cpu(vp->w1) >> 28;
>> +		eq_idx = be32_to_cpu(vp->w1) & 0x0fffffff;
> 
> we need a helper for the END base. Later.

So there's a bunch of other small tidy-ups to do, sounds like most
you are happy to go afterward?

Thanks,
Nick