| Message ID | 9f375055-eff2-010b-c904-e4122b834777@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fixed width type adjustments | expand |
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@suse.com> So I suppose Acked-by: Andrew Cooper <andrew.cooper3@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
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@suse.com> > > So I suppose Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> 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
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@suse.com> >> So I suppose Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> 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
--- 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 */ }; 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 */ + } 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) - __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) - __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 */ #elif defined(__BIG_ENDIAN_BITFIELD) - __u32 dest_id : 24; /* Destination ID */ - __u32 reserved_2 : 4; - __u32 redirection_hint: 1; /*0: dedicated CPU - 1: lowest priority */ - __u32 dest_mode : 1; /*0:physic | 1:logic */ - __u32 reserved_1 : 2; + uint32_t dest_id : 24; /* Destination ID */ + uint32_t reserved_2 : 4; + uint32_t redirection_hint : 1; /* 0: dedicated CPU + 1: lowest priority */ + uint32_t dest_mode : 1; /* 0:phys | 1:logic */ + uint32_t reserved_1 : 2; #else #error "Bitfield endianness not defined! Check your byteorder.h" #endif - }u; - __u32 value; - }lo_address; - __u32 hi_address; + } u; + uint32_t value; + } lo_address; + uint32_t hi_address; }; #define MAX_MSIX_TABLE_ENTRIES (PCI_MSIX_FLAGS_QSIZE + 1)
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@suse.com> --- For most (all?) of the single bit I was on the edge of switching them to bool - thoughts?