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 <jbeul...@suse.com>
So I suppose Acked-by: Andrew Cooper <andrew.coop...@citrix.com> 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