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

[Cornelia Huck]

On Fri, 18 Oct 2019 18:55:02 +0800
Jason Wang <jasowang@xxxxxxxxxx> wrote:

> On 2019/10/18 äå5:46, Cornelia Huck wrote:
> > On Thu, 17 Oct 2019 18:48:34 +0800
> > Jason Wang <jasowang@xxxxxxxxxx> 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.

> 
> 
> >  
> >> + * @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?
Makes sense, but probably needs some more documentation somewhere.

> 
> 
> >  
> >> + * @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.

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?