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

Nikunj A Dadhania nikunj at linux.vnet.ibm.com
Fri Jan 29 21:19:37 AEDT 2016


Thomas Huth <thuth at redhat.com> writes:

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

I am thinking of squashing the above two patches to this [20/23].

Alongwith enabling virtio 1.0 parsing, will fix legacy and get rid of
virtio.fs. I would not add the vd>cap structure in 15/23. Just the
structures until we move the allocation to C in this patch.

Regards
Nikunj





More information about the SLOF mailing list