RE: [PATCH V2 2/4] misc: vop: do not allocate and reassign the used ring

[David Laight]

From: Sherry Sun
> Sent: 29 September 2020 14:02
> 
> Hi Christoph,
> 
> > > diff --git a/include/uapi/linux/mic_common.h
> > > b/include/uapi/linux/mic_common.h index 504e523f702c..e680fe27af69
> > > 100644
> > > --- a/include/uapi/linux/mic_common.h
> > > +++ b/include/uapi/linux/mic_common.h
> > > @@ -56,8 +56,6 @@ struct mic_device_desc {
> > >   * @vdev_reset: Set to 1 by guest to indicate virtio device has been reset.
> > >   * @guest_ack: Set to 1 by guest to ack a command.
> > >   * @host_ack: Set to 1 by host to ack a command.
> > > - * @used_address_updated: Set to 1 by guest when the used address
> > > should be
> > > - * updated.
> > >   * @c2h_vdev_db: The doorbell number to be used by guest. Set by host.
> > >   * @h2c_vdev_db: The doorbell number to be used by host. Set by guest.
> > >   */
> > > @@ -67,7 +65,6 @@ struct mic_device_ctrl {
> > >  	__u8 vdev_reset;
> > >  	__u8 guest_ack;
> > >  	__u8 host_ack;
> > > -	__u8 used_address_updated;
> > >  	__s8 c2h_vdev_db;
> > >  	__s8 h2c_vdev_db;
> > >  } __attribute__ ((aligned(8)));
> >
> > This is an ABI change.
> 
> Yes, it is. But I noticed that this structure is only used by the vop driver.
> And until now I haven't seen any user tools use it, including the user tool
> mpssd which is corresponding to the vop driver.

Just rename it as 'must_be_zero'.

I've just looked at the header.
WTF is all the __attribute__ ((aligned(8))); about?
Someone is really trying to make it slow on anything other than x86.
It would have been much better to ensure the structure members
are all 'naturally aligned'.

I'm not sure of the usage of the mic_device_ctrl structure.
But you really don't want to share a structure that has
adjacent bytes written by two different entities.
Even with coherent memory you probably should have
separated the data by cache line.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)