Re: [PATCH V4 4/6] mdev: introduce virtio device and its device ops

Jason Wang
2019-10-21T05:37:31Z

On 2019/10/18 下午9:30, Cornelia Huck wrote:
> On Fri, 18 Oct 2019 18:55:02 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> On 2019/10/18 下午5:46, Cornelia Huck wrote:
>>> On Thu, 17 Oct 2019 18:48:34 +0800
>>> Jason Wang <jasowang@redhat.com> wrote:
>>>> + * @get_vendor_id: Get virtio vendor id
>>>> + * @mdev: mediated device
>>>> + * Returns u32: virtio vendor id
>>> How is the vendor id defined? As for normal virtio-pci devices?
>>
>> The vendor that provides this device. So something like this
>>
>> I notice that MMIO also had this so it looks to me it's not pci specific.
> Ok. Would be good to specify this more explicitly.

Ok.

>
>>
>>>
>>>> + * @get_status: Get the device status
>>>> + * @mdev: mediated device
>>>> + * Returns u8: virtio device status
>>>> + * @set_status: Set the device status
>>>> + * @mdev: mediated device
>>>> + * @status: virtio device status
>>>> + * @get_config: Read from device specific configuration space
>>>> + * @mdev: mediated device
>>>> + * @offset: offset from the beginning of
>>>> + * configuration space
>>>> + * @buf: buffer used to read to
>>>> + * @len: the length to read from
>>>> + * configration space
>>>> + * @set_config: Write to device specific configuration space
>>>> + * @mdev: mediated device
>>>> + * @offset: offset from the beginning of
>>>> + * configuration space
>>>> + * @buf: buffer used to write from
>>>> + * @len: the length to write to
>>>> + * configration space
>>>> + * @get_mdev_features: Get the feature of virtio mdev device
>>>> + * @mdev: mediated device
>>>> + * Returns the mdev features (API) support by
>>>> + * the device.
>>> What kind of 'features' are supposed to go in there? Are these bits,
>>> like you defined for VIRTIO_MDEV_F_VERSION_1 above?
>>
>> It's the API or mdev features other than virtio features. It could be
>> used by driver to determine the capability of the mdev device. Besides
>> _F_VERSION_1, we may add dirty page tracking etc which means we need new
>> device ops.
> Ok, so that's supposed to be distinct bits that can be or'ed together?

Yes.

> Makes sense, but probably needs some more documentation somewhere.

Let me add some doc above this helper.

>
>>
>>>
>>>> + * @get_generation: Get device generaton
>>>> + * @mdev: mediated device
>>>> + * Returns u32: device generation
>>> Is that callback mandatory?
>>
>> I think so, it's hard to emulate that completely in virtio-mdev transport.
> IIRC, the generation stuff is not mandatory in the current version of
> virtio, as not all transports have that concept.

It looks to me we can have workaround as what has been done by virtio.c.
Return 0 if this helper is not provided.

>
> Generally, are any of the callbacks optional, or are all of them
> mandatory? From what I understand, you plan to add new things that
> depend on features... would that mean non-mandatory callbacks?

Yes, not all of them were mandatory, I will explicit explain which are
mandatory and which are not.

Thanks