[SLOF] [PATCH v3 18/22] virtio: add and enable 1.0 capability parsing

[Nikunj A Dadhania]

Thomas Huth <thuth at redhat.com> writes:

> On 22.01.2016 11:55, Nikunj A Dadhania wrote:
>> Introduce 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>

>> +	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) {
>
> Maybe add a #define for "4", too, just as you did with PCI_BASE_ADDR_SPACE_IO?

Sure, I had to do that, alexey had mentioned in prev series.

>> +		addr |= SLOF_pci_config_read32(PCI_BASE_ADDR_REG_0 + 4 * (cap->bar + 1)) << 32;
>> +		addr = addr & PCI_BASE_ADDR_MEM_MASK;
>> +		addr = (uint64_t)SLOF_translate_my_address((void *)addr);
>
> No "cap->is_io = 0;" in this case here?

Either this or memset will help.

>> +	} else {
>> +		cap->is_io = 0;
>> +		addr = addr & PCI_BASE_ADDR_MEM_MASK;
>> +		addr = (uint64_t)SLOF_translate_my_address((void *)addr);
>> +	}
>
> Maybe you could simplify the above code as follows:
>
> 	if (addr & PCI_BASE_ADDR_SPACE_IO) {
> 		addr = addr & PCI_BASE_ADDR_IO_MASK;
> 		cap->is_io = 1;
> 	} else {
> 		if (addr & PCI_BASE_ADDR_SPACE_64BIT)
> 			addr |= SLOF_pci_config_read32(PCI_BASE_ADDR_REG_0 + 4 * (cap->bar + 1)) << 32;
> 		addr = addr & PCI_BASE_ADDR_MEM_MASK;
> 		addr = (uint64_t)SLOF_translate_my_address((void *)addr);
> 		cap->is_io = 0;
> 	}
>

Sure

>> +	cap->addr = (void *)addr + offset;
>> +	cap->cap_id = cfg_type;
>> +	return;
>
> Superfluous return statement.
>
>> +}
>> +
>> +/**
>> + * Reads the virtio device capabilities, gets called from SLOF routines The
>> + * function determines legacy or modern device and sets up driver registers
>> + */
>> +void virtio_parse_capabilities(struct virtio_device *dev)
>> +{
>> +	uint8_t cap_ptr, cap_vndr;
>> +	uint64_t addr = 0;
>> +
>> +	cap_ptr = SLOF_pci_config_read8(PCI_CONFIG_CAP_REG);
>> +	while (cap_ptr != 0) {
>> +		cap_vndr = SLOF_pci_config_read8(cap_ptr + VIRTIO_PCI_CAP_VNDR);
>> +		if (cap_vndr == PCI_CAP_ID_VNDR)
>> +			virtio_process_cap(dev, cap_ptr);
>> +		cap_ptr = SLOF_pci_config_read8(cap_ptr+VIRTIO_PCI_CAP_NEXT);
>> +	}
>> +
>> +	if (dev->common.cap_id && dev->notify.cap_id &&
>> +	    dev->isr.cap_id && dev->device.cap_id) {
>> +		dev->is_modern = 1;
>> +	} else {
>> +		dev->is_modern = 0;
>> +		dev->legacy.cap_id = 0;
>> +		dev->legacy.bar = 0;
>> +		addr = SLOF_pci_config_read32(PCI_BASE_ADDR_REG_0);
>> +		if (addr & PCI_BASE_ADDR_SPACE_IO) {
>> +			dev->legacy.is_io = 1;
>> +			addr = addr & PCI_BASE_ADDR_IO_MASK;
>> +		} else {
>> +			dev->legacy.is_io = 0;
>> +			addr = addr & PCI_BASE_ADDR_MEM_MASK;
>> +			addr = (uint64_t)SLOF_translate_my_address((void *)addr);
>
> No support for 64 bit bars in this case?

I did not change the legacy case, maybe can add it. Let me see if i can
combine this into a common routine.

>> +		}
>> +		dev->legacy.addr = (void *)addr;
>> +	}
>> +	return;
>
> Superfluous return statement.
>
>> +}
>> +
>

Regards
Nikunj