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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/02/2023 2:16 pm, Jan Beulich wrote:
> 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 ...

Everything I've suggested is minimal enough IMO.

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

Well... I've just found that my local PCI reference doesn't actually
match the spec when it comes to stating the width of the message field. 
Guess I need to stop using this reference.

But the rules given would require this to turn into uint16_t as that's
the specified size of the register...  But that will probably require a
separate patch.

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

Yeah, probably the easiest adjustment.  AMD is more complicated anyway IIRC.

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

Ah, so I see what went wrong.  Right now, we've got:

8b type
8b pos
4b the bitfields
12b padding
16b entry_nr

and rearranging it moves the padding to the end but doesn't drop it,
because overall the structure has 16b alignment because of the uint16_t

If it were packed, then the following byte fields would shuffle up into
the padding, and there would ba knockon effect.

But don't worry seeing as it doesn't make a difference.

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

Personally I'd consider dropping some __packed as sufficiently relevant
to this change to be included, but fine.

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

Well, its easy to try right now.  And if not, then it needs a different
patch anyway.

~Andrew