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

[Alexey Kardashevskiy]

On 01/18/2016 03:52 PM, Nikunj A Dadhania wrote:
> Alexey Kardashevskiy <aik at ozlabs.ru> writes:
>
>> On 01/14/2016 10:07 PM, Nikunj A Dadhania wrote:
>>> Alexey Kardashevskiy <aik at ozlabs.ru> writes:
>>>
>>>> On 01/13/2016 10:17 PM, Nikunj A Dadhania wrote:
>>>>> Signed-off-by: Nikunj A Dadhania <nikunj at linux.vnet.ibm.com>
>>>>> ---
>>>>>     lib/libvirtio/virtio.c | 49 +++++++++++++++++++++++++++++--------------------
>>>>>     1 file changed, 29 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/lib/libvirtio/virtio.c b/lib/libvirtio/virtio.c
>>>>> index 866ffb8..3bd8c67 100644
>>>>> --- a/lib/libvirtio/virtio.c
>>>>> +++ b/lib/libvirtio/virtio.c
>>>>> @@ -480,35 +480,43 @@ void virtio_get_host_features_long(struct virtio_device *dev, uint64_t *features
>>>>>     uint64_t virtio_get_config(struct virtio_device *dev, int offset, int size)
>>>>>     {
>>>>>     	uint64_t val = ~0ULL;
>>>>> +	uint32_t hi, lo;
>>>>>     	void *confbase;
>>>>>
>>>>> -	switch (dev->type) {
>>>>> -	 case VIRTIO_TYPE_PCI:
>>>>> +	if (dev->type != VIRTIO_TYPE_PCI)
>>>>> +		return val;
>>>>> +
>>>>> +	if (dev->is_modern)
>>>>> +		confbase = dev->device.addr;
>>>>> +	else
>>>>>     		confbase = dev->base+VIRTIOHDR_DEVICE_CONFIG;
>>>>> -		break;
>>>>> -	 default:
>>>>> -		return ~0ULL;
>>>>> -	}
>>>>> +
>>>>>     	switch (size) {
>>>>> -	 case 1:
>>>>> +	case 1:
>>>>
>>>> Too many unrelated changes :-/
>>>
>>> This is autoindent, as this was totally broken :(
>>
>>
>> That is fine if you fix it, just do this in a separate patch.
>>
>>>
>>>>
>>>>
>>>>>     		val = ci_read_8(confbase+offset);
>>>>>     		break;
>>>>> -	 case 2:
>>>>> +	case 2:
>>>>>     		val = ci_read_16(confbase+offset);
>>>>> +		if (dev->is_modern)
>>>>> +			val = le16_to_cpu(val);
>>>>>     		break;
>>>>> -	 case 4:
>>>>> +	case 4:
>>>>>     		val = ci_read_32(confbase+offset);
>>>>> +		if (dev->is_modern)
>>>>> +			val = le32_to_cpu(val);
>>>>>     		break;
>>>>> -	 case 8:
>>>>> +	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);
>>>>> +		lo = ci_read_32(confbase+offset);
>>>>> +		hi = ci_read_32(confbase+offset+4);
>>>>
>>>>
>>>> Does it have to be 2 separate 32bit reads or ci_read_64 can be used?
>>>
>>> No, as explained earlier, virtio has this as two separate regs high and
>>> low.
>>
>> Ok.
>>
>>
>>>>
>>>>
>>>>> +		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?


>
>>
>> btw is_1_0 would make more sense here. Or even virtio_version. Don't know
>> about virtio folks plans though...
>
> Qemu used is_modern, so continued that.


ok.




-- 
Alexey