Re: [PATCH v5 3/7] Introduce new destination type for admin commands

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, May 16, 2022 at 09:21:11PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Sunday, May 15, 2022 11:09 AM
> > 
> > > ---
> > >  admin.tex | 18 ++++++++++++++----
> > >  1 file changed, 14 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/admin.tex b/admin.tex
> > > index 6725daa..f816c3b 100644
> > > --- a/admin.tex
> > > +++ b/admin.tex
> > > @@ -11,11 +11,13 @@ \section{Administration command
> > set}\label{sec:Basic Facilities of a Virtio Devi
> > >          le16 command;
> > >          /*
> > >           * 0 - self
> > > -         * 1 - 65535 are reserved
> > > +         * 1 - other virtio device (identified by vdev_id) in the same device
> > group
> > > +         * 2 - 65535 are reserved
> > >           */
> > >          le16 dst_type;
> > > +        le64 vdev_id;
> > 
> > Alignment problems. Proposal:
> > 
> > vdev_id
> > dst_type
> > command
> 
> I remember that this has come up internal review as well.
> Though it is certainly good to naturally align, I don't think we have alignment problem spec wise based on below snippet of the spec [1].
> It was kind of counter intuitive to see vdev_id before seeing what the actual command is.
> That way current layout made more sense.

vdev_id and dst_type tell you where to send the command. They do not
depend on the command. In fact vdev_id -> dst_id might make sense.

> 
> There was some internal version or external, do not recall, where vdev_id was optional or was union.
> 
> But I think if we always going have vdev_id, it can be naturally aligned like your above proposal.
> 
> [1] "Structure Specifications
> Many device and driver in-memory structure layouts are documented using the C struct syntax. All structures
> are assumed to be without additional padding. To stress this, cases where common C compilers are known
> to insert extra padding within structures are tagged using the GNU C __attribute__((packed)) syntax."

These are pathological cases due to legacy. Better avoided.

-- 
MST