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

[Max Gurtovoy <mgurtovoy@nvidia.com>]

On 5/17/2022 2:33 AM, Michael S. Tsirkin wrote:
> 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.

vdev_id -> dst_id ??

what is this supposed to be ? Do you have example for -> in the spec ?

> > 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.