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

[Jason Wang]

On 2019/10/18 äå9:30, Cornelia Huck wrote:
> 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.


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