[SLOF] [PATCH v1 17/27] virtio: make virtio_{get, read}_config 1.0 aware

[Nikunj A Dadhania]

Alexey Kardashevskiy <aik at ozlabs.ru> writes:

> On 01/18/2016 03:52 PM, Nikunj A Dadhania wrote:
>> Alexey Kardashevskiy <aik at ozlabs.ru> writes:
>>>>>
>>>>>> +		if (dev->is_modern)
>>>>>> +			val = (uint64_t)le32_to_cpu(hi) << 32 | le32_to_cpu(lo);
>>>>>> +		else
>>>>>> +			val = (uint64_t)hi << 32 | lo;
>>>>>
>>>>>
>>>>> Instead of changing the entite "case 8", you could do:
>>>>>
>>>>> 	if (dev->is_modern)
>>>>> 		val = le64_to_cpu(val);
>>>>>
>>>>>
>>>>> and that's it, is not it?
>>>>
>>>> ditto.
>>>
>>> Ditto what? I am suggesting keeping original couple of ci_read_32's (i.e.
>>> not adding lo/hi) and only do byteswap when is_modern.
>>
>> This is the complete diff:
>>
>>   +	case 8:
>>       /* We don't support 8 bytes PIO accesses
>>        * in qemu and this is all PIO
>>        */
>>   -		val = ci_read_32(confbase+offset);
>>   -		val <<= 32;
>>   -		val |= ci_read_32(confbase+offset+4);
>>
>> The legacy too has hi/lo 32bits.
>>
>>
>>   +		lo = ci_read_32(confbase+offset);
>>   +		hi = ci_read_32(confbase+offset+4);
>>   +		if (dev->is_modern)
>>   +			val = (uint64_t)le32_to_cpu(hi) << 32 | le32_to_cpu(lo);
>>   +		else
>>   +			val = (uint64_t)hi << 32 | lo;
>>
>> This is correct change.
>
>
> This change is bigger than the one I suggested and I cannot see how it is 
> functionally different, just may be unmeasurable slower. Or I am missing 
> something again?

Yes. 64-bit are represented as two different registers: lo and hi, in
case of legacy, i dont need byte-swapping. In case of 1.0, both the regs
are le, needs to be byteswapped before combining them.

In the below code, you assume that its a 64-bit value and doing a
le64_to_cpu.

      >>>> 	if (dev->is_modern)
      >>>> 		val = le64_to_cpu(val);

So (le32_to_cpu(hi) << 32 | le32_to_cpu(lo)) is not the same as
le64_to_cpu(val)

Regards
Nikunj