Re: [PATCH 4/6] x86/MSI: use standard C types in structures/unions

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/02/2023 10:39 am, Jan Beulich wrote:
> Consolidate this to use exclusively standard types, and change
> indentation style to Xen's there at the same time (the file already had
> a mix of styles).
>
> No functional change intended.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

So I suppose Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> because
this is an improvement on the status quo, but I have quite a few requests.

> ---
> For most (all?) of the single bit I was on the edge of switching them to
> bool - thoughts?

Yes.

>
> --- a/xen/arch/x86/include/asm/msi.h
> +++ b/xen/arch/x86/include/asm/msi.h
> @@ -66,15 +66,15 @@ struct msi_info {
>  };
>  
>  struct msi_msg {
> -     union {
> -             u64     address; /* message address */
> -             struct {
> -                     u32     address_lo; /* message address low 32 bits */
> -                     u32     address_hi; /* message address high 32 bits */
> -             };
> -     };
> -     u32     data;           /* 16 bits of msi message data */
> -     u32     dest32;         /* used when Interrupt Remapping with EIM is 
> enabled */
> +    union {
> +        uint64_t address; /* message address */
> +        struct {
> +            uint32_t address_lo; /* message address low 32 bits */
> +            uint32_t address_hi; /* message address high 32 bits */
> +        };
> +    };
> +    uint32_t data;        /* 16 bits of msi message data */
> +    uint32_t dest32;      /* used when Interrupt Remapping with EIM is 
> enabled */

The 16 is actively wrong for data, but honestly it's only this dest32
comment which has any value whatsoever (when it has been de-Intel'd).

I'd correct dest32 to reference the AMD too, and delete the rest.

>  };
>  
>  struct irq_desc;
> @@ -94,35 +94,35 @@ extern int pci_restore_msi_state(struct
>  extern int pci_reset_msix_state(struct pci_dev *pdev);
>  
>  struct msi_desc {
> -     struct msi_attrib {
> -             __u8    type;           /* {0: unused, 5h:MSI, 11h:MSI-X} */
> -             __u8    pos;            /* Location of the MSI capability */
> -             __u8    maskbit : 1;    /* mask/pending bit supported ?   */
> -             __u8    is_64   : 1;    /* Address size: 0=32bit 1=64bit  */
> -             __u8    host_masked : 1;
> -             __u8    guest_masked : 1;
> -             __u16   entry_nr;       /* specific enabled entry         */
> -     } msi_attrib;
> -
> -     bool irte_initialized;
> -     uint8_t gvec;                   /* guest vector. valid when pi_desc 
> isn't NULL */
> -     const struct pi_desc *pi_desc;  /* pointer to posted descriptor */
> -
> -     struct list_head list;
> -
> -     union {
> -             void __iomem *mask_base;/* va for the entry in mask table */
> -             struct {
> -                     unsigned int nvec;/* number of vectors            */
> -                     unsigned int mpos;/* location of mask register    */
> -             } msi;
> -             unsigned int hpet_id;   /* HPET (dev is NULL)             */
> -     };
> -     struct pci_dev *dev;
> -     int irq;
> -     int remap_index;                /* index in interrupt remapping table */
> +    struct msi_attrib {
> +        uint8_t type;        /* {0: unused, 5h:MSI, 11h:MSI-X} */
> +        uint8_t pos;         /* Location of the MSI capability */
> +        uint8_t maskbit      : 1; /* mask/pending bit supported ?   */
> +        uint8_t is_64        : 1; /* Address size: 0=32bit 1=64bit  */
> +        uint8_t host_masked  : 1;
> +        uint8_t guest_masked : 1;
> +        uint16_t entry_nr;   /* specific enabled entry */

entry_nr wants to move up to between pos and maskbit, and then we shrink
the total structure by 8 bytes (I think).

> +    } msi_attrib;
> +
> +    bool irte_initialized;
> +    uint8_t gvec;            /* guest vector. valid when pi_desc isn't NULL 
> */
> +    const struct pi_desc *pi_desc; /* pointer to posted descriptor */
> +
> +    struct list_head list;
> +
> +    union {
> +        void __iomem *mask_base; /* va for the entry in mask table */
> +        struct {
> +            unsigned int nvec; /* number of vectors */
> +            unsigned int mpos; /* location of mask register */
> +        } msi;
> +        unsigned int hpet_id; /* HPET (dev is NULL) */
> +    };
> +    struct pci_dev *dev;
> +    int irq;
> +    int remap_index;         /* index in interrupt remapping table */
>  
> -     struct msi_msg msg;             /* Last set MSI message */
> +    struct msi_msg msg;      /* Last set MSI message */
>  };
>  
>  /*
> @@ -180,48 +180,48 @@ int msi_free_irq(struct msi_desc *entry)
>  
>  struct __packed msg_data {
>  #if defined(__LITTLE_ENDIAN_BITFIELD)

There's no such thing as a big endian x86 bitfield.  Just delete this
ifdefary to simplify the result.

Additionally, the structure doesn't need to be packed - its a single
uint32_t's worth of bitfield.

Finally, can we drop the reserved fields and leave them as anonymous
bitfields?

> -     __u32   vector          :  8;
> -     __u32   delivery_mode   :  3;   /* 000b: FIXED | 001b: lowest prior */
> -     __u32   reserved_1      :  3;
> -     __u32   level           :  1;   /* 0: deassert | 1: assert */
> -     __u32   trigger         :  1;   /* 0: edge | 1: level */
> -     __u32   reserved_2      : 16;
> +    uint32_t vector        :  8;
> +    uint32_t delivery_mode :  3;    /* 000b: FIXED | 001b: lowest prior */
> +    uint32_t reserved_1    :  3;
> +    uint32_t level         :  1;    /* 0: deassert | 1: assert */
> +    uint32_t trigger       :  1;    /* 0: edge | 1: level */
> +    uint32_t reserved_2    : 16;
>  #elif defined(__BIG_ENDIAN_BITFIELD)
> -     __u32   reserved_2      : 16;
> -     __u32   trigger         :  1;   /* 0: edge | 1: level */
> -     __u32   level           :  1;   /* 0: deassert | 1: assert */
> -     __u32   reserved_1      :  3;
> -     __u32   delivery_mode   :  3;   /* 000b: FIXED | 001b: lowest prior */
> -     __u32   vector          :  8;
> +    uint32_t reserved_2    : 16;
> +    uint32_t trigger       :  1;    /* 0: edge | 1: level */
> +    uint32_t level         :  1;    /* 0: deassert | 1: assert */
> +    uint32_t reserved_1    :  3;
> +    uint32_t delivery_mode :  3;    /* 000b: FIXED | 001b: lowest prior */
> +    uint32_t vector        :  8;
>  #else
>  #error "Bitfield endianness not defined! Check your byteorder.h"
>  #endif
>  };
>  
>  struct __packed msg_address {
> -     union {
> -             struct {
> +    union {
> +        struct {
>  #if defined(__LITTLE_ENDIAN_BITFIELD)

Same here for ifdefary and packed.

> -                     __u32   reserved_1      :  2;
> -                     __u32   dest_mode       :  1;   /*0:physic | 1:logic */
> -                     __u32   redirection_hint:  1;   /*0: dedicated CPU
> -                                                       1: lowest priority */
> -                     __u32   reserved_2      :  4;
> -                     __u32   dest_id         : 24;   /* Destination ID */
> +            uint32_t reserved_1       :  2;
> +            uint32_t dest_mode        :  1; /* 0:phys | 1:logic */
> +            uint32_t redirection_hint :  1; /* 0: dedicated CPU
> +                                               1: lowest priority */
> +            uint32_t reserved_2       :  4;
> +            uint32_t dest_id          : 24; /* Destination ID */

Considering that these fields are stale (its missing the remappable bit
for one), I do have to question if we actually use them correctly in code...

But that's not something for this patch.

~Andrew