[SLOF] [PATCH v2 16/19] virtio: add and enable 1.0 capability parsing

[Nikunj A Dadhania]

Thomas Huth <thuth at redhat.com> writes:

> On 20.01.2016 13:10, Nikunj A Dadhania wrote:
>> Introcduce parsing routines for virtio capabilities. This would also
>> determine whether we need to function in legacy mode or virtio 1.0.
>> 
>> Drivers need to negotiate the 1.0 feature capability before starting to
>> use 1.0.
>> 
>> Disable all the drivers until 1.0 is enabled.
>> 
>> Signed-off-by: Nikunj A Dadhania <nikunj at linux.vnet.ibm.com>
>> ---

>> +	case VIRTIO_PCI_CAP_DEVICE_CFG:
>> +		cap = &dev->device;
>> +		break;
>> +	default:
>> +		cap = NULL;
>
> Simply return here ...

Yes

>> +	}
>> +
>> +	if (cap && cfg_type) {
>
> ... then you can get rid of this if-statement and the code below is less indented.

Indeed.

>
>> +		cap->bar = bar;
>> +		addr = SLOF_pci_config_read32(PCI_BASE_ADDR_REG_0 + 4 * cap->bar);
>> +		if (addr & PCI_BASE_ADDR_SPACE_IO) {
>> +			cap->is_io = 1;
>> +			addr = addr & PCI_BASE_ADDR_IO_MASK;
>> +		} else if (addr & 4) {
>> +			addr = SLOF_pci_config_read32(PCI_BASE_ADDR_REG_0 + 4 * cap->bar) |
>> +				(SLOF_pci_config_read32(PCI_BASE_ADDR_REG_0 + 4 * (cap->bar + 1)) << 32);
>
> I think you could simplify the above two lines to:
>
> 	addr |= SLOF_pci_config_read32(PCI_BASE_ADDR_REG_0 + 4 * (cap->bar + 1)) << 32
>
> since the first 32 bit have already been read.

Right.

>
>> +			addr = (uint64_t)SLOF_translate_my_address((void *)addr);
>> +			addr = addr & PCI_BASE_ADDR_MEM_MASK;
>
> I'd maybe swap the above two lines since the mask applies to the PCI
> address, not the memory address. (shouldn't make a difference in real
> life, but IMHO it's clearer the other way round).

Sure.

>
>> +		} else {
>> +			cap->is_io = 0;
>> +			addr = addr & PCI_BASE_ADDR_MEM_MASK;
>
> Isn't a SLOF_translate_my_address() needed for 32-bit addresses, too?

It is, not sure why didnt get an error though !

>
>> +		}
>> +		cap->addr = (void *)addr + offset;
>> +		cap->cap_id = cfg_type;
>> +	}
>> +	return;
>
> Superfluous return statement, please remove.

Sure

>> +	}
>> +	return;
>
> Superfluous return statement again.

Sure

>
>> +}
>> +
>>  /**
>>   * Calculate ring size according to queue size number
>>   */
>> diff --git a/lib/libvirtio/virtio.code b/lib/libvirtio/virtio.code
>> index 258b9bb..c3cb4de 100644
>> --- a/lib/libvirtio/virtio.code
>> +++ b/lib/libvirtio/virtio.code
>> @@ -18,6 +18,12 @@
>>  
>>  /******** core virtio ********/
>>  
>> +// : virtio-parse-capabilities  ( dev --  )
>> +PRIM(virtio_X2d_parse_X2d_capabilities)
>> +  void *dev = TOS.a; POP;
>
> Wrong indentation in above line?

Yes, got converted to spaces.

Regards
Nikunj