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@xxxxxxxxxx>
> >>> Signed-off-by: Shijith Thotton <sthotton@xxxxxxxxxxx>
> >>> ---
> >>> 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.
