Re: [PATCH v3 4/4] vdpa/octeon_ep: read vendor-specific PCI capability

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

On Fri, Dec 13, 2024 at 02:20:24PM +0000, Shijith Thotton wrote:
> >>>
> >>> Added support to read the vendor-specific PCI capability to identify the
> >>> type of device being emulated.
> >>>
> >>> Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
> >>> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> >>> ---
> >>>  drivers/vdpa/octeon_ep/octep_vdpa.h      | 20 ++++++++++++++
> >>>  drivers/vdpa/octeon_ep/octep_vdpa_hw.c   | 33
> >>+++++++++++++++++++++++-
> >>>  drivers/vdpa/octeon_ep/octep_vdpa_main.c |  4 ++-
> >>>  3 files changed, 55 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/vdpa/octeon_ep/octep_vdpa.h
> >>b/drivers/vdpa/octeon_ep/octep_vdpa.h
> >>> index 2cadb878e679..53b020b019f7 100644
> >>> --- a/drivers/vdpa/octeon_ep/octep_vdpa.h
> >>> +++ b/drivers/vdpa/octeon_ep/octep_vdpa.h
> >>> @@ -8,6 +8,7 @@
> >>>  #include <linux/pci_regs.h>
> >>>  #include <linux/vdpa.h>
> >>>  #include <linux/virtio_pci_modern.h>
> >>> +#include <uapi/linux/virtio_crypto.h>
> >>>  #include <uapi/linux/virtio_net.h>
> >>>  #include <uapi/linux/virtio_blk.h>
> >>>  #include <uapi/linux/virtio_config.h>
> >>> @@ -52,6 +53,24 @@ struct octep_vring_info {
> >>>         phys_addr_t notify_pa;
> >>>  };
> >>>
> >>> +enum octep_pci_vndr_cfg_type {
> >>> +       OCTEP_PCI_VNDR_CFG_TYPE_VIRTIO_ID,
> >>> +       OCTEP_PCI_VNDR_CFG_TYPE_MAX,
> >>> +};
> >>> +
> >>> +struct octep_pci_vndr_data {
> >>> +       struct virtio_pci_vndr_data hdr;
> >>> +       u8 id;
> >>> +       u8 bar;
> >>> +       union {
> >>> +               u64 data;
> >>> +               struct {
> >>> +                       u32 offset;
> >>> +                       u32 length;
> >>> +               };
> >>> +       };
> >>> +};
> >>
> >>This seems not to be padded to a multiple of 4 bytes?
> >>
> >
> >Looks like, the structure is naturally aligned.
> >
> >struct virtio_pci_vndr_data {
> >        uint8_t                    cap_vndr;             /*     0     1 */
> >        uint8_t                    cap_next;             /*     1     1 */
> >        uint8_t                    cap_len;              /*     2     1 */
> >        uint8_t                    cfg_type;             /*     3     1 */
> >        uint16_t                   vendor_id;            /*     4     2 */
> >        /* size: 6, cachelines: 1, members: 5 */
> >        /* last cacheline: 6 bytes */
> >};
> >struct octep_pci_vndr_data {
> >        struct virtio_pci_vndr_data hdr;                 /*     0     6 */
> >        uint8_t                    id;                   /*     6     1 */
> >        uint8_t                    bar;                  /*     7     1 */
> >        union {
> >                uint64_t           data;                 /*     8     8 */
> >                struct {
> >                        uint32_t   offset;               /*     8     4 */
> >                        uint32_t   length;               /*    12     4 */
> >                };                                       /*     8     8 */
> >        };                                               /*     8     8 */
> >
> >        /* size: 16, cachelines: 1, members: 4 */
> >        /* last cacheline: 16 bytes */
> >};
> >
> >I will add __attribute__((aligned(4))) to be safe.
> >
> 
> Small correction, I meant   __attribute__((packed, aligned(4))).
> 
> Thanks,
> Shijith

Don't add packed pls, for me it means "something is not packed naturally
here". when in fact it is.

aligned is also bad, since it can add padding which creates all
kind of security issues.

If you want to be sure, add BUILD_BUG_ON and check that it is aligned
as expected.