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

[Jason Wang <jasowang@redhat.com>]

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