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

[Cédric Le Goater]

On 07/11/2019 11:29, Nicholas Piggin wrote:
> 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?

yes. This is not problematic. 

Reviewed-by: Cédric Le Goater <clg at kaod.org>

C. 


> 
> Thanks,
> Nick
>