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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.02.2023 11:55, Andrew Cooper wrote:
> 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.

Thanks. I'll be happy to carry out some of them (but the sheer amount makes
it so I'd rather not apply the A-b to the result). It's always difficult to
judge how much "while doing this" is going to be acceptable ...

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

It it? The upper 16 bits aren't used, are they?

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

I guess I'll simply drop "with EIM".

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

The struct is 6 bytes now and will be 6 bytes with the adjustment you
suggest. Plus I'd prefer to not do any re-ordering in this patch.

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

Will do.

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

Like with re-ordering I would prefer to not touch entirely unrelated
aspects. I'll see if I can motivate myself to make a separate follow-on
change.

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

Perhaps - I can give that a try, hoping that we don't access them
anywhere by their name (even if just to e.g. zero them).

Jan