[XEN,02/13] AMD/IOMMU: fixed violations of MISRA C:2012 Rule 7.2

Commit Message

Simone Ballarin June 20, 2023, 10:34 a.m. UTC

From: Gianluca Luparini <[e-mail]>

The xen sources contains violations of MISRA C:2012 Rule 7.2 whose headline states:
"A "u" or "U" suffix shall be applied to all integer constants that are represented in an unsigned type".

I propose to use "U" as a suffix to explicitly state when an integer constant is represented in an unsigned type.
For homogeneity, I also added the "U" suffix in some cases that the tool didn't report as violations.

Signed-off-by: Simone Ballarin <[e-mail]>
---
 xen/drivers/passthrough/amd/iommu-defs.h | 122 +++++++++++------------
 1 file changed, 61 insertions(+), 61 deletions(-)

Comments

Jan Beulich June 20, 2023, 12:39 p.m. UTC | #1

On 20.06.2023 12:34, Simone Ballarin wrote:
> --- a/xen/drivers/passthrough/amd/iommu-defs.h
> +++ b/xen/drivers/passthrough/amd/iommu-defs.h
> @@ -38,49 +38,49 @@
>          ((uint64_t)(offset) << (12 + (PTE_PER_TABLE_SHIFT * ((level) - 1))))
>  
>  /* IOMMU Capability */
> -#define PCI_CAP_ID_MASK		0x000000FF
> +#define PCI_CAP_ID_MASK		0x000000FFU

Seeing this and similar uses further down, I think there's a purely
stylistic question to be resolved first (and hence I'm widening the To:
list here): Do we want to allow either case of U to be used
interchangeably, leaving the choice up to the patch author?

Personally at least for hex numbers I'd prefer the suffix to stand out: If
the digits use upper case, have the suffix be lower case and vice versa.
(If there are only numbers, then surrounding context would need
consulting.) I realize though that this will not fit well with the desire
of avoiding lower-case l in number suffixes; it would be fine for e.g.
0xFFul. Not sure in how many cases this would matter though, as I expect
most constants want to be unsigned.

Jan

Luca Fancellu June 20, 2023, 8:08 p.m. UTC | #2

> On 20 Jun 2023, at 13:39, Jan Beulich <[e-mail]> wrote:
> 
> On 20.06.2023 12:34, Simone Ballarin wrote:
>> --- a/xen/drivers/passthrough/amd/iommu-defs.h
>> +++ b/xen/drivers/passthrough/amd/iommu-defs.h
>> @@ -38,49 +38,49 @@
>>         ((uint64_t)(offset) << (12 + (PTE_PER_TABLE_SHIFT * ((level) - 1))))
>> 
>> /* IOMMU Capability */
>> -#define PCI_CAP_ID_MASK 0x000000FF
>> +#define PCI_CAP_ID_MASK 0x000000FFU
> 
> Seeing this and similar uses further down, I think there's a purely
> stylistic question to be resolved first (and hence I'm widening the To:
> list here): Do we want to allow either case of U to be used
> interchangeably, leaving the choice up to the patch author?

Because you ask to the list, my personal opinion is that we might want to
have only capital U, so that it’s consistent across the codebase and we don’t
need to create too many rules for the developer, it stands out anyway as it can’t
be an hex digit, also maybe every hex constant is nicer to have with all capital,
but this is another subject.

Said that, clearly it’s up to the maintainers to choose.

> 
> Personally at least for hex numbers I'd prefer the suffix to stand out: If
> the digits use upper case, have the suffix be lower case and vice versa.
> (If there are only numbers, then surrounding context would need
> consulting.) I realize though that this will not fit well with the desire
> of avoiding lower-case l in number suffixes; it would be fine for e.g.
> 0xFFul. Not sure in how many cases this would matter though, as I expect
> most constants want to be unsigned.
> 
> Jan
>

Stefano Stabellini June 20, 2023, 8:41 p.m. UTC | #3

On Tue, 20 Jun 2023, Luca Fancellu wrote:
> > On 20 Jun 2023, at 13:39, Jan Beulich <[e-mail]> wrote:
> > 
> > On 20.06.2023 12:34, Simone Ballarin wrote:
> >> --- a/xen/drivers/passthrough/amd/iommu-defs.h
> >> +++ b/xen/drivers/passthrough/amd/iommu-defs.h
> >> @@ -38,49 +38,49 @@
> >>         ((uint64_t)(offset) << (12 + (PTE_PER_TABLE_SHIFT * ((level) - 1))))
> >> 
> >> /* IOMMU Capability */
> >> -#define PCI_CAP_ID_MASK 0x000000FF
> >> +#define PCI_CAP_ID_MASK 0x000000FFU
> > 
> > Seeing this and similar uses further down, I think there's a purely
> > stylistic question to be resolved first (and hence I'm widening the To:
> > list here): Do we want to allow either case of U to be used
> > interchangeably, leaving the choice up to the patch author?
> 
> Because you ask to the list, my personal opinion is that we might want to
> have only capital U, so that it’s consistent across the codebase and we don’t
> need to create too many rules for the developer, it stands out anyway as it can’t
> be an hex digit

I agree with Luca

Stefano Stabellini June 20, 2023, 8:48 p.m. UTC | #4

On Tue, 20 Jun 2023, Simone Ballarin wrote:
> From: Gianluca Luparini <[e-mail]>
> 
> The xen sources contains violations of MISRA C:2012 Rule 7.2 whose headline states:
> "A "u" or "U" suffix shall be applied to all integer constants that are represented in an unsigned type".
> 
> I propose to use "U" as a suffix to explicitly state when an integer constant is represented in an unsigned type.
> For homogeneity, I also added the "U" suffix in some cases that the tool didn't report as violations.
> 
> Signed-off-by: Simone Ballarin <[e-mail]>

The changes look good to me, so aside from the wording of the commit
message that needs an update:

Acked-by: Stefano Stabellini <[e-mail]>



> ---
>  xen/drivers/passthrough/amd/iommu-defs.h | 122 +++++++++++------------
>  1 file changed, 61 insertions(+), 61 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu-defs.h b/xen/drivers/passthrough/amd/iommu-defs.h
> index 35de548e3a..c145248f9a 100644
> --- a/xen/drivers/passthrough/amd/iommu-defs.h
> +++ b/xen/drivers/passthrough/amd/iommu-defs.h
> @@ -38,49 +38,49 @@
>          ((uint64_t)(offset) << (12 + (PTE_PER_TABLE_SHIFT * ((level) - 1))))
>  
>  /* IOMMU Capability */
> -#define PCI_CAP_ID_MASK		0x000000FF
> +#define PCI_CAP_ID_MASK		0x000000FFU
>  #define PCI_CAP_ID_SHIFT	0
> -#define PCI_CAP_NEXT_PTR_MASK	0x0000FF00
> +#define PCI_CAP_NEXT_PTR_MASK	0x0000FF00U
>  #define PCI_CAP_NEXT_PTR_SHIFT	8
> -#define PCI_CAP_TYPE_MASK	0x00070000
> +#define PCI_CAP_TYPE_MASK	0x00070000U
>  #define PCI_CAP_TYPE_SHIFT	16
> -#define PCI_CAP_REV_MASK	0x00F80000
> +#define PCI_CAP_REV_MASK	0x00F80000U
>  #define PCI_CAP_REV_SHIFT	19
> -#define PCI_CAP_IOTLB_MASK	0x01000000
> +#define PCI_CAP_IOTLB_MASK	0x01000000U
>  #define PCI_CAP_IOTLB_SHIFT	24
> -#define PCI_CAP_HT_TUNNEL_MASK	0x02000000
> +#define PCI_CAP_HT_TUNNEL_MASK	0x02000000U
>  #define PCI_CAP_HT_TUNNEL_SHIFT	25
> -#define PCI_CAP_NP_CACHE_MASK	0x04000000
> +#define PCI_CAP_NP_CACHE_MASK	0x04000000U
>  #define PCI_CAP_NP_CACHE_SHIFT	26
>  #define PCI_CAP_EFRSUP_SHIFT    27
> -#define PCI_CAP_RESET_MASK	0x80000000
> +#define PCI_CAP_RESET_MASK	0x80000000U
>  #define PCI_CAP_RESET_SHIFT	31
>  
>  #define PCI_CAP_TYPE_IOMMU		0x3
>  
>  #define PCI_CAP_MMIO_BAR_LOW_OFFSET	0x04
>  #define PCI_CAP_MMIO_BAR_HIGH_OFFSET	0x08
> -#define PCI_CAP_MMIO_BAR_LOW_MASK	0xFFFFC000
> +#define PCI_CAP_MMIO_BAR_LOW_MASK	0xFFFFC000U
>  #define IOMMU_MMIO_REGION_LENGTH	0x4000
>  
>  #define PCI_CAP_RANGE_OFFSET		0x0C
> -#define PCI_CAP_BUS_NUMBER_MASK		0x0000FF00
> +#define PCI_CAP_BUS_NUMBER_MASK		0x0000FF00U
>  #define PCI_CAP_BUS_NUMBER_SHIFT	8
> -#define PCI_CAP_FIRST_DEVICE_MASK	0x00FF0000
> +#define PCI_CAP_FIRST_DEVICE_MASK	0x00FF0000U
>  #define PCI_CAP_FIRST_DEVICE_SHIFT	16
> -#define PCI_CAP_LAST_DEVICE_MASK	0xFF000000
> +#define PCI_CAP_LAST_DEVICE_MASK	0xFF000000U
>  #define PCI_CAP_LAST_DEVICE_SHIFT	24
>  
> -#define PCI_CAP_UNIT_ID_MASK    0x0000001F
> +#define PCI_CAP_UNIT_ID_MASK    0x0000001FU
>  #define PCI_CAP_UNIT_ID_SHIFT   0
>  #define PCI_CAP_MISC_INFO_OFFSET    0x10
> -#define PCI_CAP_MSI_NUMBER_MASK     0x0000001F
> +#define PCI_CAP_MSI_NUMBER_MASK     0x0000001FU
>  #define PCI_CAP_MSI_NUMBER_SHIFT    0
>  
>  /* Device Table */
>  #define IOMMU_DEV_TABLE_BASE_LOW_OFFSET		0x00
>  #define IOMMU_DEV_TABLE_BASE_HIGH_OFFSET	0x04
> -#define IOMMU_DEV_TABLE_SIZE_MASK		0x000001FF
> +#define IOMMU_DEV_TABLE_SIZE_MASK		0x000001FFU
>  #define IOMMU_DEV_TABLE_SIZE_SHIFT		0
>  
>  #define IOMMU_DEV_TABLE_ENTRIES_PER_BUS		256
> @@ -159,13 +159,13 @@ struct amd_iommu_dte {
>  #define IOMMU_CMD_BUFFER_BASE_HIGH_OFFSET	0x0C
>  #define IOMMU_CMD_BUFFER_HEAD_OFFSET		0x2000
>  #define IOMMU_CMD_BUFFER_TAIL_OFFSET		0x2008
> -#define IOMMU_CMD_BUFFER_LENGTH_MASK		0x0F000000
> +#define IOMMU_CMD_BUFFER_LENGTH_MASK		0x0F000000U
>  #define IOMMU_CMD_BUFFER_LENGTH_SHIFT		24
>  
>  #define IOMMU_CMD_BUFFER_ENTRY_ORDER            4
>  #define IOMMU_CMD_BUFFER_MAX_ENTRIES            (1u << 15)
>  
> -#define IOMMU_CMD_OPCODE_MASK			0xF0000000
> +#define IOMMU_CMD_OPCODE_MASK			0xF0000000U
>  #define IOMMU_CMD_OPCODE_SHIFT			28
>  #define IOMMU_CMD_COMPLETION_WAIT		0x1
>  #define IOMMU_CMD_INVALIDATE_DEVTAB_ENTRY	0x2
> @@ -178,50 +178,50 @@ struct amd_iommu_dte {
>  /* COMPLETION_WAIT command */
>  #define IOMMU_COMP_WAIT_DATA_BUFFER_SIZE	8
>  #define IOMMU_COMP_WAIT_DATA_BUFFER_ALIGNMENT	8
> -#define IOMMU_COMP_WAIT_S_FLAG_MASK		0x00000001
> -#define IOMMU_COMP_WAIT_I_FLAG_MASK		0x00000002
> -#define IOMMU_COMP_WAIT_F_FLAG_MASK		0x00000004
> -#define IOMMU_COMP_WAIT_ADDR_LOW_MASK		0xFFFFFFF8
> +#define IOMMU_COMP_WAIT_S_FLAG_MASK		0x00000001U
> +#define IOMMU_COMP_WAIT_I_FLAG_MASK		0x00000002U
> +#define IOMMU_COMP_WAIT_F_FLAG_MASK		0x00000004U
> +#define IOMMU_COMP_WAIT_ADDR_LOW_MASK		0xFFFFFFF8U
>  #define IOMMU_COMP_WAIT_ADDR_LOW_SHIFT		3
> -#define IOMMU_COMP_WAIT_ADDR_HIGH_MASK		0x000FFFFF
> +#define IOMMU_COMP_WAIT_ADDR_HIGH_MASK		0x000FFFFFU
>  #define IOMMU_COMP_WAIT_ADDR_HIGH_SHIFT		0
>  
>  /* INVALIDATE_IOMMU_PAGES command */
> -#define IOMMU_INV_IOMMU_PAGES_DOMAIN_ID_MASK	0x0000FFFF
> +#define IOMMU_INV_IOMMU_PAGES_DOMAIN_ID_MASK	0x0000FFFFU
>  #define IOMMU_INV_IOMMU_PAGES_DOMAIN_ID_SHIFT	0
> -#define IOMMU_INV_IOMMU_PAGES_S_FLAG_MASK	0x00000001
> +#define IOMMU_INV_IOMMU_PAGES_S_FLAG_MASK	0x00000001U
>  #define IOMMU_INV_IOMMU_PAGES_S_FLAG_SHIFT	0
> -#define IOMMU_INV_IOMMU_PAGES_PDE_FLAG_MASK	0x00000002
> +#define IOMMU_INV_IOMMU_PAGES_PDE_FLAG_MASK	0x00000002U
>  #define IOMMU_INV_IOMMU_PAGES_PDE_FLAG_SHIFT	1
> -#define IOMMU_INV_IOMMU_PAGES_ADDR_LOW_MASK	0xFFFFF000
> +#define IOMMU_INV_IOMMU_PAGES_ADDR_LOW_MASK	0xFFFFF000U
>  #define IOMMU_INV_IOMMU_PAGES_ADDR_LOW_SHIFT	12
> -#define IOMMU_INV_IOMMU_PAGES_ADDR_HIGH_MASK	0xFFFFFFFF
> +#define IOMMU_INV_IOMMU_PAGES_ADDR_HIGH_MASK	0xFFFFFFFFU
>  #define IOMMU_INV_IOMMU_PAGES_ADDR_HIGH_SHIFT	0
>  
>  /* INVALIDATE_DEVTAB_ENTRY command */
> -#define IOMMU_INV_DEVTAB_ENTRY_DEVICE_ID_MASK   0x0000FFFF
> +#define IOMMU_INV_DEVTAB_ENTRY_DEVICE_ID_MASK   0x0000FFFFU
>  #define IOMMU_INV_DEVTAB_ENTRY_DEVICE_ID_SHIFT  0
>  
>  /* INVALIDATE_INTERRUPT_TABLE command */
> -#define IOMMU_INV_INT_TABLE_DEVICE_ID_MASK   0x0000FFFF
> +#define IOMMU_INV_INT_TABLE_DEVICE_ID_MASK   0x0000FFFFU
>  #define IOMMU_INV_INT_TABLE_DEVICE_ID_SHIFT  0
>  
>  /* INVALIDATE_IOTLB_PAGES command */
> -#define IOMMU_INV_IOTLB_PAGES_MAXPEND_MASK          0xff000000
> +#define IOMMU_INV_IOTLB_PAGES_MAXPEND_MASK          0xff000000U
>  #define IOMMU_INV_IOTLB_PAGES_MAXPEND_SHIFT         24
> -#define IOMMU_INV_IOTLB_PAGES_PASID1_MASK           0x00ff0000
> +#define IOMMU_INV_IOTLB_PAGES_PASID1_MASK           0x00ff0000U
>  #define IOMMU_INV_IOTLB_PAGES_PASID1_SHIFT          16
> -#define IOMMU_INV_IOTLB_PAGES_PASID2_MASK           0x0fff0000
> +#define IOMMU_INV_IOTLB_PAGES_PASID2_MASK           0x0fff0000U
>  #define IOMMU_INV_IOTLB_PAGES_PASID2_SHIFT          16
> -#define IOMMU_INV_IOTLB_PAGES_QUEUEID_MASK          0x0000ffff
> +#define IOMMU_INV_IOTLB_PAGES_QUEUEID_MASK          0x0000ffffU
>  #define IOMMU_INV_IOTLB_PAGES_QUEUEID_SHIFT         0
> -#define IOMMU_INV_IOTLB_PAGES_DEVICE_ID_MASK        0x0000FFFF
> +#define IOMMU_INV_IOTLB_PAGES_DEVICE_ID_MASK        0x0000FFFFU
>  #define IOMMU_INV_IOTLB_PAGES_DEVICE_ID_SHIFT       0
> -#define IOMMU_INV_IOTLB_PAGES_ADDR_LOW_MASK         0xFFFFF000
> +#define IOMMU_INV_IOTLB_PAGES_ADDR_LOW_MASK         0xFFFFF000U
>  #define IOMMU_INV_IOTLB_PAGES_ADDR_LOW_SHIFT        12
> -#define IOMMU_INV_IOTLB_PAGES_ADDR_HIGH_MASK        0xFFFFFFFF
> +#define IOMMU_INV_IOTLB_PAGES_ADDR_HIGH_MASK        0xFFFFFFFFU
>  #define IOMMU_INV_IOTLB_PAGES_ADDR_HIGH_SHIFT       0
> -#define IOMMU_INV_IOTLB_PAGES_S_FLAG_MASK           0x00000001
> +#define IOMMU_INV_IOTLB_PAGES_S_FLAG_MASK           0x00000001U
>  #define IOMMU_INV_IOTLB_PAGES_S_FLAG_SHIFT          0
>  
>  /* Event Log */
> @@ -229,18 +229,18 @@ struct amd_iommu_dte {
>  #define IOMMU_EVENT_LOG_BASE_HIGH_OFFSET	0x14
>  #define IOMMU_EVENT_LOG_HEAD_OFFSET		0x2010
>  #define IOMMU_EVENT_LOG_TAIL_OFFSET		0x2018
> -#define IOMMU_EVENT_LOG_LENGTH_MASK		0x0F000000
> +#define IOMMU_EVENT_LOG_LENGTH_MASK		0x0F000000U
>  #define IOMMU_EVENT_LOG_LENGTH_SHIFT		24
> -#define IOMMU_EVENT_LOG_HEAD_MASK		0x0007FFF0
> +#define IOMMU_EVENT_LOG_HEAD_MASK		0x0007FFF0U
>  #define IOMMU_EVENT_LOG_HEAD_SHIFT		4
> -#define IOMMU_EVENT_LOG_TAIL_MASK		0x0007FFF0
> +#define IOMMU_EVENT_LOG_TAIL_MASK		0x0007FFF0U
>  #define IOMMU_EVENT_LOG_TAIL_SHIFT		4
>  
>  #define IOMMU_EVENT_LOG_ENTRY_SIZE 			16
>  #define IOMMU_EVENT_LOG_POWER_OF2_ENTRIES_PER_PAGE	8
>  #define IOMMU_EVENT_LOG_U32_PER_ENTRY	(IOMMU_EVENT_LOG_ENTRY_SIZE / 4)
>  
> -#define IOMMU_EVENT_CODE_MASK			0xF0000000
> +#define IOMMU_EVENT_CODE_MASK			0xF0000000U
>  #define IOMMU_EVENT_CODE_SHIFT			28
>  #define IOMMU_EVENT_ILLEGAL_DEV_TABLE_ENTRY	0x1
>  #define IOMMU_EVENT_IO_PAGE_FAULT		0x2
> @@ -251,12 +251,12 @@ struct amd_iommu_dte {
>  #define IOMMU_EVENT_IOTLB_INV_TIMEOUT		0x7
>  #define IOMMU_EVENT_INVALID_DEV_REQUEST		0x8
>  
> -#define IOMMU_EVENT_DOMAIN_ID_MASK           0x0000FFFF
> +#define IOMMU_EVENT_DOMAIN_ID_MASK           0x0000FFFFU
>  #define IOMMU_EVENT_DOMAIN_ID_SHIFT          0
> -#define IOMMU_EVENT_DEVICE_ID_MASK           0x0000FFFF
> +#define IOMMU_EVENT_DEVICE_ID_MASK           0x0000FFFFU
>  #define IOMMU_EVENT_DEVICE_ID_SHIFT          0
>  #define IOMMU_EVENT_FLAGS_SHIFT              16
> -#define IOMMU_EVENT_FLAGS_MASK               0x0FFF0000
> +#define IOMMU_EVENT_FLAGS_MASK               0x0FFF0000U
>  
>  /* PPR Log */
>  #define IOMMU_PPR_LOG_ENTRY_SIZE                        16
> @@ -265,21 +265,21 @@ struct amd_iommu_dte {
>  
>  #define IOMMU_PPR_LOG_BASE_LOW_OFFSET                   0x0038
>  #define IOMMU_PPR_LOG_BASE_HIGH_OFFSET                  0x003C
> -#define IOMMU_PPR_LOG_BASE_LOW_MASK                     0xFFFFF000
> +#define IOMMU_PPR_LOG_BASE_LOW_MASK                     0xFFFFF000U
>  #define IOMMU_PPR_LOG_BASE_LOW_SHIFT                    12
> -#define IOMMU_PPR_LOG_BASE_HIGH_MASK                    0x000FFFFF
> +#define IOMMU_PPR_LOG_BASE_HIGH_MASK                    0x000FFFFFU
>  #define IOMMU_PPR_LOG_BASE_HIGH_SHIFT                   0
> -#define IOMMU_PPR_LOG_LENGTH_MASK                       0x0F000000
> +#define IOMMU_PPR_LOG_LENGTH_MASK                       0x0F000000U
>  #define IOMMU_PPR_LOG_LENGTH_SHIFT                      24
> -#define IOMMU_PPR_LOG_HEAD_MASK                         0x0007FFF0
> +#define IOMMU_PPR_LOG_HEAD_MASK                         0x0007FFF0U
>  #define IOMMU_PPR_LOG_HEAD_SHIFT                        4
> -#define IOMMU_PPR_LOG_TAIL_MASK                         0x0007FFF0
> +#define IOMMU_PPR_LOG_TAIL_MASK                         0x0007FFF0U
>  #define IOMMU_PPR_LOG_TAIL_SHIFT                        4
>  #define IOMMU_PPR_LOG_HEAD_OFFSET                       0x2030
>  #define IOMMU_PPR_LOG_TAIL_OFFSET                       0x2038
> -#define IOMMU_PPR_LOG_DEVICE_ID_MASK                    0x0000FFFF
> +#define IOMMU_PPR_LOG_DEVICE_ID_MASK                    0x0000FFFFU
>  #define IOMMU_PPR_LOG_DEVICE_ID_SHIFT                   0
> -#define IOMMU_PPR_LOG_CODE_MASK                         0xF0000000
> +#define IOMMU_PPR_LOG_CODE_MASK                         0xF0000000U
>  #define IOMMU_PPR_LOG_CODE_SHIFT                        28
>  
>  #define IOMMU_LOG_ENTRY_TIMEOUT                         1000
> @@ -342,17 +342,17 @@ union amd_iommu_control {
>  #define IOMMU_EXCLUSION_BASE_HIGH_OFFSET	0x24
>  #define IOMMU_EXCLUSION_LIMIT_LOW_OFFSET	0x28
>  #define IOMMU_EXCLUSION_LIMIT_HIGH_OFFSET	0x2C
> -#define IOMMU_EXCLUSION_BASE_LOW_MASK		0xFFFFF000
> +#define IOMMU_EXCLUSION_BASE_LOW_MASK		0xFFFFF000U
>  #define IOMMU_EXCLUSION_BASE_LOW_SHIFT		12
> -#define IOMMU_EXCLUSION_BASE_HIGH_MASK		0xFFFFFFFF
> +#define IOMMU_EXCLUSION_BASE_HIGH_MASK		0xFFFFFFFFU
>  #define IOMMU_EXCLUSION_BASE_HIGH_SHIFT		0
> -#define IOMMU_EXCLUSION_RANGE_ENABLE_MASK	0x00000001
> +#define IOMMU_EXCLUSION_RANGE_ENABLE_MASK	0x00000001U
>  #define IOMMU_EXCLUSION_RANGE_ENABLE_SHIFT	0
> -#define IOMMU_EXCLUSION_ALLOW_ALL_MASK		0x00000002
> +#define IOMMU_EXCLUSION_ALLOW_ALL_MASK		0x00000002U
>  #define IOMMU_EXCLUSION_ALLOW_ALL_SHIFT		1
> -#define IOMMU_EXCLUSION_LIMIT_LOW_MASK		0xFFFFF000
> +#define IOMMU_EXCLUSION_LIMIT_LOW_MASK		0xFFFFF000U
>  #define IOMMU_EXCLUSION_LIMIT_LOW_SHIFT		12
> -#define IOMMU_EXCLUSION_LIMIT_HIGH_MASK		0xFFFFFFFF
> +#define IOMMU_EXCLUSION_LIMIT_HIGH_MASK		0xFFFFFFFFU
>  #define IOMMU_EXCLUSION_LIMIT_HIGH_SHIFT	0
>  
>  /* Extended Feature Register */
> @@ -476,14 +476,14 @@ union amd_iommu_pte {
>  
>  #define INV_IOMMU_ALL_PAGES_ADDRESS      ((1ULL << 63) - 1)
>  
> -#define IOMMU_RING_BUFFER_PTR_MASK                  0x0007FFF0
> +#define IOMMU_RING_BUFFER_PTR_MASK                  0x0007FFF0U
>  
> -#define IOMMU_CMD_DEVICE_ID_MASK                    0x0000FFFF
> +#define IOMMU_CMD_DEVICE_ID_MASK                    0x0000FFFFU
>  #define IOMMU_CMD_DEVICE_ID_SHIFT                   0
>  
> -#define IOMMU_REG_BASE_ADDR_LOW_MASK                0xFFFFF000
> +#define IOMMU_REG_BASE_ADDR_LOW_MASK                0xFFFFF000U
>  #define IOMMU_REG_BASE_ADDR_LOW_SHIFT               12
> -#define IOMMU_REG_BASE_ADDR_HIGH_MASK               0x000FFFFF
> +#define IOMMU_REG_BASE_ADDR_HIGH_MASK               0x000FFFFFU
>  #define IOMMU_REG_BASE_ADDR_HIGH_SHIFT              0
>  
>  #endif /* AMD_IOMMU_DEFS_H */
> -- 
> 2.41.0
>

Simone Ballarin June 21, 2023, 9:38 a.m. UTC | #5

Il giorno mar 20 giu 2023 alle ore 22:41 Stefano Stabellini <
[e-mail]> ha scritto:

> On Tue, 20 Jun 2023, Luca Fancellu wrote:
> > > On 20 Jun 2023, at 13:39, Jan Beulich <[e-mail]> wrote:
> > >
> > > On 20.06.2023 12:34, Simone Ballarin wrote:
> > >> --- a/xen/drivers/passthrough/amd/iommu-defs.h
> > >> +++ b/xen/drivers/passthrough/amd/iommu-defs.h
> > >> @@ -38,49 +38,49 @@
> > >>         ((uint64_t)(offset) << (12 + (PTE_PER_TABLE_SHIFT * ((level)
> - 1))))
> > >>
> > >> /* IOMMU Capability */
> > >> -#define PCI_CAP_ID_MASK 0x000000FF
> > >> +#define PCI_CAP_ID_MASK 0x000000FFU
> > >
> > > Seeing this and similar uses further down, I think there's a purely
> > > stylistic question to be resolved first (and hence I'm widening the To:
> > > list here): Do we want to allow either case of U to be used
> > > interchangeably, leaving the choice up to the patch author?
> >
> > Because you ask to the list, my personal opinion is that we might want to
> > have only capital U, so that it’s consistent across the codebase and we
> don’t
> > need to create too many rules for the developer, it stands out anyway as
> it can’t
> > be an hex digit
>
> I agree with Luca


I also agree with Luca.

Patch

diff --git a/xen/drivers/passthrough/amd/iommu-defs.h b/xen/drivers/passthrough/amd/iommu-defs.h
index 35de548e3a..c145248f9a 100644
--- a/xen/drivers/passthrough/amd/iommu-defs.h
+++ b/xen/drivers/passthrough/amd/iommu-defs.h
@@ -38,49 +38,49 @@ 
         ((uint64_t)(offset) << (12 + (PTE_PER_TABLE_SHIFT * ((level) - 1))))
 
 /* IOMMU Capability */
-#define PCI_CAP_ID_MASK		0x000000FF
+#define PCI_CAP_ID_MASK		0x000000FFU
 #define PCI_CAP_ID_SHIFT	0
-#define PCI_CAP_NEXT_PTR_MASK	0x0000FF00
+#define PCI_CAP_NEXT_PTR_MASK	0x0000FF00U
 #define PCI_CAP_NEXT_PTR_SHIFT	8
-#define PCI_CAP_TYPE_MASK	0x00070000
+#define PCI_CAP_TYPE_MASK	0x00070000U
 #define PCI_CAP_TYPE_SHIFT	16
-#define PCI_CAP_REV_MASK	0x00F80000
+#define PCI_CAP_REV_MASK	0x00F80000U
 #define PCI_CAP_REV_SHIFT	19
-#define PCI_CAP_IOTLB_MASK	0x01000000
+#define PCI_CAP_IOTLB_MASK	0x01000000U
 #define PCI_CAP_IOTLB_SHIFT	24
-#define PCI_CAP_HT_TUNNEL_MASK	0x02000000
+#define PCI_CAP_HT_TUNNEL_MASK	0x02000000U
 #define PCI_CAP_HT_TUNNEL_SHIFT	25
-#define PCI_CAP_NP_CACHE_MASK	0x04000000
+#define PCI_CAP_NP_CACHE_MASK	0x04000000U
 #define PCI_CAP_NP_CACHE_SHIFT	26
 #define PCI_CAP_EFRSUP_SHIFT    27
-#define PCI_CAP_RESET_MASK	0x80000000
+#define PCI_CAP_RESET_MASK	0x80000000U
 #define PCI_CAP_RESET_SHIFT	31
 
 #define PCI_CAP_TYPE_IOMMU		0x3
 
 #define PCI_CAP_MMIO_BAR_LOW_OFFSET	0x04
 #define PCI_CAP_MMIO_BAR_HIGH_OFFSET	0x08
-#define PCI_CAP_MMIO_BAR_LOW_MASK	0xFFFFC000
+#define PCI_CAP_MMIO_BAR_LOW_MASK	0xFFFFC000U
 #define IOMMU_MMIO_REGION_LENGTH	0x4000
 
 #define PCI_CAP_RANGE_OFFSET		0x0C
-#define PCI_CAP_BUS_NUMBER_MASK		0x0000FF00
+#define PCI_CAP_BUS_NUMBER_MASK		0x0000FF00U
 #define PCI_CAP_BUS_NUMBER_SHIFT	8
-#define PCI_CAP_FIRST_DEVICE_MASK	0x00FF0000
+#define PCI_CAP_FIRST_DEVICE_MASK	0x00FF0000U
 #define PCI_CAP_FIRST_DEVICE_SHIFT	16
-#define PCI_CAP_LAST_DEVICE_MASK	0xFF000000
+#define PCI_CAP_LAST_DEVICE_MASK	0xFF000000U
 #define PCI_CAP_LAST_DEVICE_SHIFT	24
 
-#define PCI_CAP_UNIT_ID_MASK    0x0000001F
+#define PCI_CAP_UNIT_ID_MASK    0x0000001FU
 #define PCI_CAP_UNIT_ID_SHIFT   0
 #define PCI_CAP_MISC_INFO_OFFSET    0x10
-#define PCI_CAP_MSI_NUMBER_MASK     0x0000001F
+#define PCI_CAP_MSI_NUMBER_MASK     0x0000001FU
 #define PCI_CAP_MSI_NUMBER_SHIFT    0
 
 /* Device Table */
 #define IOMMU_DEV_TABLE_BASE_LOW_OFFSET		0x00
 #define IOMMU_DEV_TABLE_BASE_HIGH_OFFSET	0x04
-#define IOMMU_DEV_TABLE_SIZE_MASK		0x000001FF
+#define IOMMU_DEV_TABLE_SIZE_MASK		0x000001FFU
 #define IOMMU_DEV_TABLE_SIZE_SHIFT		0
 
 #define IOMMU_DEV_TABLE_ENTRIES_PER_BUS		256
@@ -159,13 +159,13 @@  struct amd_iommu_dte {
 #define IOMMU_CMD_BUFFER_BASE_HIGH_OFFSET	0x0C
 #define IOMMU_CMD_BUFFER_HEAD_OFFSET		0x2000
 #define IOMMU_CMD_BUFFER_TAIL_OFFSET		0x2008
-#define IOMMU_CMD_BUFFER_LENGTH_MASK		0x0F000000
+#define IOMMU_CMD_BUFFER_LENGTH_MASK		0x0F000000U
 #define IOMMU_CMD_BUFFER_LENGTH_SHIFT		24
 
 #define IOMMU_CMD_BUFFER_ENTRY_ORDER            4
 #define IOMMU_CMD_BUFFER_MAX_ENTRIES            (1u << 15)
 
-#define IOMMU_CMD_OPCODE_MASK			0xF0000000
+#define IOMMU_CMD_OPCODE_MASK			0xF0000000U
 #define IOMMU_CMD_OPCODE_SHIFT			28
 #define IOMMU_CMD_COMPLETION_WAIT		0x1
 #define IOMMU_CMD_INVALIDATE_DEVTAB_ENTRY	0x2
@@ -178,50 +178,50 @@  struct amd_iommu_dte {
 /* COMPLETION_WAIT command */
 #define IOMMU_COMP_WAIT_DATA_BUFFER_SIZE	8
 #define IOMMU_COMP_WAIT_DATA_BUFFER_ALIGNMENT	8
-#define IOMMU_COMP_WAIT_S_FLAG_MASK		0x00000001
-#define IOMMU_COMP_WAIT_I_FLAG_MASK		0x00000002
-#define IOMMU_COMP_WAIT_F_FLAG_MASK		0x00000004
-#define IOMMU_COMP_WAIT_ADDR_LOW_MASK		0xFFFFFFF8
+#define IOMMU_COMP_WAIT_S_FLAG_MASK		0x00000001U
+#define IOMMU_COMP_WAIT_I_FLAG_MASK		0x00000002U
+#define IOMMU_COMP_WAIT_F_FLAG_MASK		0x00000004U
+#define IOMMU_COMP_WAIT_ADDR_LOW_MASK		0xFFFFFFF8U
 #define IOMMU_COMP_WAIT_ADDR_LOW_SHIFT		3
-#define IOMMU_COMP_WAIT_ADDR_HIGH_MASK		0x000FFFFF
+#define IOMMU_COMP_WAIT_ADDR_HIGH_MASK		0x000FFFFFU
 #define IOMMU_COMP_WAIT_ADDR_HIGH_SHIFT		0
 
 /* INVALIDATE_IOMMU_PAGES command */
-#define IOMMU_INV_IOMMU_PAGES_DOMAIN_ID_MASK	0x0000FFFF
+#define IOMMU_INV_IOMMU_PAGES_DOMAIN_ID_MASK	0x0000FFFFU
 #define IOMMU_INV_IOMMU_PAGES_DOMAIN_ID_SHIFT	0
-#define IOMMU_INV_IOMMU_PAGES_S_FLAG_MASK	0x00000001
+#define IOMMU_INV_IOMMU_PAGES_S_FLAG_MASK	0x00000001U
 #define IOMMU_INV_IOMMU_PAGES_S_FLAG_SHIFT	0
-#define IOMMU_INV_IOMMU_PAGES_PDE_FLAG_MASK	0x00000002
+#define IOMMU_INV_IOMMU_PAGES_PDE_FLAG_MASK	0x00000002U
 #define IOMMU_INV_IOMMU_PAGES_PDE_FLAG_SHIFT	1
-#define IOMMU_INV_IOMMU_PAGES_ADDR_LOW_MASK	0xFFFFF000
+#define IOMMU_INV_IOMMU_PAGES_ADDR_LOW_MASK	0xFFFFF000U
 #define IOMMU_INV_IOMMU_PAGES_ADDR_LOW_SHIFT	12
-#define IOMMU_INV_IOMMU_PAGES_ADDR_HIGH_MASK	0xFFFFFFFF
+#define IOMMU_INV_IOMMU_PAGES_ADDR_HIGH_MASK	0xFFFFFFFFU
 #define IOMMU_INV_IOMMU_PAGES_ADDR_HIGH_SHIFT	0
 
 /* INVALIDATE_DEVTAB_ENTRY command */
-#define IOMMU_INV_DEVTAB_ENTRY_DEVICE_ID_MASK   0x0000FFFF
+#define IOMMU_INV_DEVTAB_ENTRY_DEVICE_ID_MASK   0x0000FFFFU
 #define IOMMU_INV_DEVTAB_ENTRY_DEVICE_ID_SHIFT  0
 
 /* INVALIDATE_INTERRUPT_TABLE command */
-#define IOMMU_INV_INT_TABLE_DEVICE_ID_MASK   0x0000FFFF
+#define IOMMU_INV_INT_TABLE_DEVICE_ID_MASK   0x0000FFFFU
 #define IOMMU_INV_INT_TABLE_DEVICE_ID_SHIFT  0
 
 /* INVALIDATE_IOTLB_PAGES command */
-#define IOMMU_INV_IOTLB_PAGES_MAXPEND_MASK          0xff000000
+#define IOMMU_INV_IOTLB_PAGES_MAXPEND_MASK          0xff000000U
 #define IOMMU_INV_IOTLB_PAGES_MAXPEND_SHIFT         24
-#define IOMMU_INV_IOTLB_PAGES_PASID1_MASK           0x00ff0000
+#define IOMMU_INV_IOTLB_PAGES_PASID1_MASK           0x00ff0000U
 #define IOMMU_INV_IOTLB_PAGES_PASID1_SHIFT          16
-#define IOMMU_INV_IOTLB_PAGES_PASID2_MASK           0x0fff0000
+#define IOMMU_INV_IOTLB_PAGES_PASID2_MASK           0x0fff0000U
 #define IOMMU_INV_IOTLB_PAGES_PASID2_SHIFT          16
-#define IOMMU_INV_IOTLB_PAGES_QUEUEID_MASK          0x0000ffff
+#define IOMMU_INV_IOTLB_PAGES_QUEUEID_MASK          0x0000ffffU
 #define IOMMU_INV_IOTLB_PAGES_QUEUEID_SHIFT         0
-#define IOMMU_INV_IOTLB_PAGES_DEVICE_ID_MASK        0x0000FFFF
+#define IOMMU_INV_IOTLB_PAGES_DEVICE_ID_MASK        0x0000FFFFU
 #define IOMMU_INV_IOTLB_PAGES_DEVICE_ID_SHIFT       0
-#define IOMMU_INV_IOTLB_PAGES_ADDR_LOW_MASK         0xFFFFF000
+#define IOMMU_INV_IOTLB_PAGES_ADDR_LOW_MASK         0xFFFFF000U
 #define IOMMU_INV_IOTLB_PAGES_ADDR_LOW_SHIFT        12
-#define IOMMU_INV_IOTLB_PAGES_ADDR_HIGH_MASK        0xFFFFFFFF
+#define IOMMU_INV_IOTLB_PAGES_ADDR_HIGH_MASK        0xFFFFFFFFU
 #define IOMMU_INV_IOTLB_PAGES_ADDR_HIGH_SHIFT       0
-#define IOMMU_INV_IOTLB_PAGES_S_FLAG_MASK           0x00000001
+#define IOMMU_INV_IOTLB_PAGES_S_FLAG_MASK           0x00000001U
 #define IOMMU_INV_IOTLB_PAGES_S_FLAG_SHIFT          0
 
 /* Event Log */
@@ -229,18 +229,18 @@  struct amd_iommu_dte {
 #define IOMMU_EVENT_LOG_BASE_HIGH_OFFSET	0x14
 #define IOMMU_EVENT_LOG_HEAD_OFFSET		0x2010
 #define IOMMU_EVENT_LOG_TAIL_OFFSET		0x2018
-#define IOMMU_EVENT_LOG_LENGTH_MASK		0x0F000000
+#define IOMMU_EVENT_LOG_LENGTH_MASK		0x0F000000U
 #define IOMMU_EVENT_LOG_LENGTH_SHIFT		24
-#define IOMMU_EVENT_LOG_HEAD_MASK		0x0007FFF0
+#define IOMMU_EVENT_LOG_HEAD_MASK		0x0007FFF0U
 #define IOMMU_EVENT_LOG_HEAD_SHIFT		4
-#define IOMMU_EVENT_LOG_TAIL_MASK		0x0007FFF0
+#define IOMMU_EVENT_LOG_TAIL_MASK		0x0007FFF0U
 #define IOMMU_EVENT_LOG_TAIL_SHIFT		4
 
 #define IOMMU_EVENT_LOG_ENTRY_SIZE 			16
 #define IOMMU_EVENT_LOG_POWER_OF2_ENTRIES_PER_PAGE	8
 #define IOMMU_EVENT_LOG_U32_PER_ENTRY	(IOMMU_EVENT_LOG_ENTRY_SIZE / 4)
 
-#define IOMMU_EVENT_CODE_MASK			0xF0000000
+#define IOMMU_EVENT_CODE_MASK			0xF0000000U
 #define IOMMU_EVENT_CODE_SHIFT			28
 #define IOMMU_EVENT_ILLEGAL_DEV_TABLE_ENTRY	0x1
 #define IOMMU_EVENT_IO_PAGE_FAULT		0x2
@@ -251,12 +251,12 @@  struct amd_iommu_dte {
 #define IOMMU_EVENT_IOTLB_INV_TIMEOUT		0x7
 #define IOMMU_EVENT_INVALID_DEV_REQUEST		0x8
 
-#define IOMMU_EVENT_DOMAIN_ID_MASK           0x0000FFFF
+#define IOMMU_EVENT_DOMAIN_ID_MASK           0x0000FFFFU
 #define IOMMU_EVENT_DOMAIN_ID_SHIFT          0
-#define IOMMU_EVENT_DEVICE_ID_MASK           0x0000FFFF
+#define IOMMU_EVENT_DEVICE_ID_MASK           0x0000FFFFU
 #define IOMMU_EVENT_DEVICE_ID_SHIFT          0
 #define IOMMU_EVENT_FLAGS_SHIFT              16
-#define IOMMU_EVENT_FLAGS_MASK               0x0FFF0000
+#define IOMMU_EVENT_FLAGS_MASK               0x0FFF0000U
 
 /* PPR Log */
 #define IOMMU_PPR_LOG_ENTRY_SIZE                        16
@@ -265,21 +265,21 @@  struct amd_iommu_dte {
 
 #define IOMMU_PPR_LOG_BASE_LOW_OFFSET                   0x0038
 #define IOMMU_PPR_LOG_BASE_HIGH_OFFSET                  0x003C
-#define IOMMU_PPR_LOG_BASE_LOW_MASK                     0xFFFFF000
+#define IOMMU_PPR_LOG_BASE_LOW_MASK                     0xFFFFF000U
 #define IOMMU_PPR_LOG_BASE_LOW_SHIFT                    12
-#define IOMMU_PPR_LOG_BASE_HIGH_MASK                    0x000FFFFF
+#define IOMMU_PPR_LOG_BASE_HIGH_MASK                    0x000FFFFFU
 #define IOMMU_PPR_LOG_BASE_HIGH_SHIFT                   0
-#define IOMMU_PPR_LOG_LENGTH_MASK                       0x0F000000
+#define IOMMU_PPR_LOG_LENGTH_MASK                       0x0F000000U
 #define IOMMU_PPR_LOG_LENGTH_SHIFT                      24
-#define IOMMU_PPR_LOG_HEAD_MASK                         0x0007FFF0
+#define IOMMU_PPR_LOG_HEAD_MASK                         0x0007FFF0U
 #define IOMMU_PPR_LOG_HEAD_SHIFT                        4
-#define IOMMU_PPR_LOG_TAIL_MASK                         0x0007FFF0
+#define IOMMU_PPR_LOG_TAIL_MASK                         0x0007FFF0U
 #define IOMMU_PPR_LOG_TAIL_SHIFT                        4
 #define IOMMU_PPR_LOG_HEAD_OFFSET                       0x2030
 #define IOMMU_PPR_LOG_TAIL_OFFSET                       0x2038
-#define IOMMU_PPR_LOG_DEVICE_ID_MASK                    0x0000FFFF
+#define IOMMU_PPR_LOG_DEVICE_ID_MASK                    0x0000FFFFU
 #define IOMMU_PPR_LOG_DEVICE_ID_SHIFT                   0
-#define IOMMU_PPR_LOG_CODE_MASK                         0xF0000000
+#define IOMMU_PPR_LOG_CODE_MASK                         0xF0000000U
 #define IOMMU_PPR_LOG_CODE_SHIFT                        28
 
 #define IOMMU_LOG_ENTRY_TIMEOUT                         1000
@@ -342,17 +342,17 @@  union amd_iommu_control {
 #define IOMMU_EXCLUSION_BASE_HIGH_OFFSET	0x24
 #define IOMMU_EXCLUSION_LIMIT_LOW_OFFSET	0x28
 #define IOMMU_EXCLUSION_LIMIT_HIGH_OFFSET	0x2C
-#define IOMMU_EXCLUSION_BASE_LOW_MASK		0xFFFFF000
+#define IOMMU_EXCLUSION_BASE_LOW_MASK		0xFFFFF000U
 #define IOMMU_EXCLUSION_BASE_LOW_SHIFT		12
-#define IOMMU_EXCLUSION_BASE_HIGH_MASK		0xFFFFFFFF
+#define IOMMU_EXCLUSION_BASE_HIGH_MASK		0xFFFFFFFFU
 #define IOMMU_EXCLUSION_BASE_HIGH_SHIFT		0
-#define IOMMU_EXCLUSION_RANGE_ENABLE_MASK	0x00000001
+#define IOMMU_EXCLUSION_RANGE_ENABLE_MASK	0x00000001U
 #define IOMMU_EXCLUSION_RANGE_ENABLE_SHIFT	0
-#define IOMMU_EXCLUSION_ALLOW_ALL_MASK		0x00000002
+#define IOMMU_EXCLUSION_ALLOW_ALL_MASK		0x00000002U
 #define IOMMU_EXCLUSION_ALLOW_ALL_SHIFT		1
-#define IOMMU_EXCLUSION_LIMIT_LOW_MASK		0xFFFFF000
+#define IOMMU_EXCLUSION_LIMIT_LOW_MASK		0xFFFFF000U
 #define IOMMU_EXCLUSION_LIMIT_LOW_SHIFT		12
-#define IOMMU_EXCLUSION_LIMIT_HIGH_MASK		0xFFFFFFFF
+#define IOMMU_EXCLUSION_LIMIT_HIGH_MASK		0xFFFFFFFFU
 #define IOMMU_EXCLUSION_LIMIT_HIGH_SHIFT	0
 
 /* Extended Feature Register */
@@ -476,14 +476,14 @@  union amd_iommu_pte {
 
 #define INV_IOMMU_ALL_PAGES_ADDRESS      ((1ULL << 63) - 1)
 
-#define IOMMU_RING_BUFFER_PTR_MASK                  0x0007FFF0
+#define IOMMU_RING_BUFFER_PTR_MASK                  0x0007FFF0U
 
-#define IOMMU_CMD_DEVICE_ID_MASK                    0x0000FFFF
+#define IOMMU_CMD_DEVICE_ID_MASK                    0x0000FFFFU
 #define IOMMU_CMD_DEVICE_ID_SHIFT                   0
 
-#define IOMMU_REG_BASE_ADDR_LOW_MASK                0xFFFFF000
+#define IOMMU_REG_BASE_ADDR_LOW_MASK                0xFFFFF000U
 #define IOMMU_REG_BASE_ADDR_LOW_SHIFT               12
-#define IOMMU_REG_BASE_ADDR_HIGH_MASK               0x000FFFFF
+#define IOMMU_REG_BASE_ADDR_HIGH_MASK               0x000FFFFFU
 #define IOMMU_REG_BASE_ADDR_HIGH_SHIFT              0
 
 #endif /* AMD_IOMMU_DEFS_H */