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 <[email protected]>
>
> So I suppose Acked-by: Andrew Cooper <[email protected]> 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
