[SLOF] [PATCH v4 20/23] virtio: add and enable 1.0 capability parsing

[Thomas Huth]

On 29.01.2016 10:24, Nikunj A Dadhania wrote:
> Nikunj A Dadhania <nikunj at linux.vnet.ibm.com> writes:
> 
>> Thomas Huth <thuth at redhat.com> writes:
>>
>>> On 28.01.2016 11:24, 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.
>>>
>>> s/Disable all/Disable it in all/
>>>
>>>> Signed-off-by: Nikunj A Dadhania <nikunj at linux.vnet.ibm.com>
>>>> ---
>>>>  board-qemu/slof/virtio.fs   |   1 +
>>>>  lib/libvirtio/virtio-9p.c   |   3 ++
>>>>  lib/libvirtio/virtio-blk.c  |   3 ++
>>>>  lib/libvirtio/virtio-net.c  |   3 ++
>>>>  lib/libvirtio/virtio-scsi.c |   3 ++
>>>>  lib/libvirtio/virtio.c      | 109 ++++++++++++++++++++++++++++++++++++++++++++
>>>>  lib/libvirtio/virtio.code   |   6 +++
>>>>  lib/libvirtio/virtio.h      |   1 +
>>>>  lib/libvirtio/virtio.in     |   2 +
>>>>  9 files changed, 131 insertions(+)
>>> ...
>>>> +/**
>>>> + * 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;
>>>> +
>>>> +	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;
>>>> +		virtio_cap_set_base_addr(&dev->legacy, 0);
>>>> +	}
>>>> +}
>>>
>>> Do you ever use dev->legacy in the code later? struct virtio_device
>>> already contains the "addr" field, which is the pointer to the legacy
>>> BAR address, right? So you might want to remove dev->addr in the end
>>> and use dev->legacy.bar instead, I guess?
>>
>> I think let me not maintain a separate addr, instead us the
>> legacy->addr. Will add a new patch for converting this, as there will be
>> changes in all the accessor routines.
>>
>>> Or remove the dev->legacy
>>> structure and set the dev->addr field here instead? Either way, you
>>> could finally get rid of the Forth code that reads the legacy BAR in
>>> the virtio-setup-vd function.
>>>
>>> Anyway, you could also do this as a separate clean-up patch later
>>> (this patch here basically also looks fine to me as it is right now).
>>
>> I will add one patch after this to take care of this clean-up
> 
> With this patch, I thought of further optimizing this and getting rid of
> virtio.fs completely. Here is what I have currently, this can be squashed
> to different patches. But for completeness, I have all the diff here.
> Alexey, too was suggesting in the first revision not to have the SLOF
> structures. I can include this in v5. 

You're right, virto.fs seems really to be redundant now, and this looks
like a very good clean-up patch to me, too, so I'd also vote for
including this at the right spots in your patch series.

 Thomas