Re: [PATCH 08/11] transport-pci: Introduce virtio extended capability

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

On Wed, Apr 05, 2023 at 01:16:31PM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, April 5, 2023 1:11 AM
> 
> > > > > +struct virtio_pcie_ext_cap {
> > > > > +        struct pcie_ext_cap pcie_ecap;
> > > > > +        u8 cfg_type; /* Identifies the structure. */
> > > > > +        u8 bar; /* Index of the BAR where its located */
> > > > > +        u8 id; /* Multiple capabilities of the same type */
> > > > > +        u8 zero_padding[1];
> > > > > +        le64 offset; /* Offset with the bar */
> > > > > +        le64 length; /* Length of the structure, in bytes. */
> > > > > +        u8 data[]; /* Optional variable length data */
> > > >
> > > > Maybe le64 data[], for alignment?
> > > >
> > > It gets harder to decode (typecasting ..) if its string with le64 data type.
> > 
> > In what language? In C you have to cast anyway, string is char *, often signed,
> > not u8.
> > 
> > > I will extend the comment,
> > >
> > > +        u8 data[]; /* Optional variable length data, must be aligned
> > > + to 8 bytes */
> > 
> > I'd keep it le64 or u64, it is highly unlikely we'll pass strings through this
> > interface anyway.
> 
> Ok. will change.
> 
> What about rest of the patches? If we proceed using MMR interface, rest of the patches are fine?

Biggest problem is 7/11 - the new IDs, breaking all existing drivers.

I thought I replied on that but don't see it on that specific patch.
Let me repost my thoughts now I had time to think it over.

-- 
MST