[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/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.

>
> 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.

>
>
>>
>>>
>>>
>>>>    		break;
>>>>    	}
>>>> -
>>>>    	return val;
>>>>    }
>>>>
>>>> @@ -522,13 +530,14 @@ int __virtio_read_config(struct virtio_device *dev, void *dst,
>>>>    	unsigned char *buf = dst;
>>>>    	int i;
>>>>
>>>> -	switch (dev->type) {
>>>> -	 case VIRTIO_TYPE_PCI:
>>>> -		confbase = dev->base+VIRTIOHDR_DEVICE_CONFIG;
>>>> -		break;
>>>> -	 default:
>>>> +	if (dev->type != VIRTIO_TYPE_PCI)
>>>>    		return 0;
>>>> -	}
>>>> +
>>>> +	if (dev->is_modern)
>>>> +		confbase = dev->device.addr;
>>>> +	else
>>>> +		confbase = dev->base+VIRTIOHDR_DEVICE_CONFIG;
>>>> +
>>>>    	for (i = 0; i < len; i++)
>>>>    		buf[i] = ci_read_8(confbase + offset + i);
>>>>    	return len;
>>>>
>>>
>>>
>>> --
>>> Alexey
>>
>
>
> -- 
> Alexey