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

[Alexey Kardashevskiy]

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.

btw is_1_0 would make more sense here. Or even virtio_version. Don't know 
about virtio folks plans though...


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