[PATCH 0/4] x86/misra: fix remaining violations of rule 20.7

[PATCH 0/4] x86/misra: fix remaining violations of rule 20.7

[Roger Pau Monne]

Hello,

Following series attempts to fix the remaining violation for rules 20.7,
and as a result make it blocking on x86 also (as it's already the case
for ARM).

Thanks, Roger.

Roger Pau Monne (4):
  x8&/mm: fix IS_LnE_ALIGNED() to comply with Misra Rule 20.7
  x86/msi: fix Misra Rule 20.7 in msi.h
  x86/uaccess: rework user access speculative harden guards
  automation/eclair: make Misra rule 20.7 blocking for x86 also

 automation/eclair_analysis/ECLAIR/tagging.ecl |  3 +-
 xen/arch/x86/include/asm/msi.h                | 35 ++++++---------
 xen/arch/x86/include/asm/uaccess.h            | 45 +++++++++----------
 xen/arch/x86/mm.c                             |  2 +-
 xen/arch/x86/usercopy.c                       | 26 +++++------
 5 files changed, 52 insertions(+), 59 deletions(-)

-- 
2.46.0

[PATCH 1/4] x8&/mm: fix IS_LnE_ALIGNED() to comply with Misra Rule 20.7

[Roger Pau Monne]

While not strictly needed to guarantee operator precedence is as expected, add
the parentheses to comply with Misra Rule 20.7.

No functional change intended.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Fixes: 5b52e1b0436f ('x86/mm: skip super-page alignment checks for non-present entries')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/mm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 494c14e80ff9..fa21903eb25a 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5498,7 +5498,7 @@ int map_pages_to_xen(
  * be INVALID_MFN, since alignment is only relevant for present entries.
  */
 #define IS_LnE_ALIGNED(v, m, n) ({                              \
-    mfn_t m_ = m;                                               \
+    mfn_t m_ = (m);                                             \
                                                                 \
     ASSERT(!mfn_eq(m_, INVALID_MFN));                           \
     IS_ALIGNED(PFN_DOWN(v) | mfn_x(m_),                         \
-- 
2.46.0

Re: [PATCH 1/4] x8&/mm: fix IS_LnE_ALIGNED() to comply with Misra Rule 20.7

[Frediano Ziglio]

On Tue, Nov 19, 2024 at 10:35 AM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> While not strictly needed to guarantee operator precedence is as expected, add
> the parentheses to comply with Misra Rule 20.7.
>
> No functional change intended.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Fixes: 5b52e1b0436f ('x86/mm: skip super-page alignment checks for non-present entries')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/mm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 494c14e80ff9..fa21903eb25a 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5498,7 +5498,7 @@ int map_pages_to_xen(
>   * be INVALID_MFN, since alignment is only relevant for present entries.
>   */
>  #define IS_LnE_ALIGNED(v, m, n) ({                              \
> -    mfn_t m_ = m;                                               \
> +    mfn_t m_ = (m);                                             \
>                                                                  \
>      ASSERT(!mfn_eq(m_, INVALID_MFN));                           \
>      IS_ALIGNED(PFN_DOWN(v) | mfn_x(m_),                         \

Minor, typo in subject: x8& -> x86

Frediano

Re: [PATCH 1/4] x8&/mm: fix IS_LnE_ALIGNED() to comply with Misra Rule 20.7

[Roger Pau Monné]

On Tue, Nov 19, 2024 at 10:52:26AM +0000, Frediano Ziglio wrote:
> On Tue, Nov 19, 2024 at 10:35 AM Roger Pau Monne <roger.pau@citrix.com> wrote:
> >
> > While not strictly needed to guarantee operator precedence is as expected, add
> > the parentheses to comply with Misra Rule 20.7.
> >
> > No functional change intended.
> >
> > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Fixes: 5b52e1b0436f ('x86/mm: skip super-page alignment checks for non-present entries')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  xen/arch/x86/mm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > index 494c14e80ff9..fa21903eb25a 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -5498,7 +5498,7 @@ int map_pages_to_xen(
> >   * be INVALID_MFN, since alignment is only relevant for present entries.
> >   */
> >  #define IS_LnE_ALIGNED(v, m, n) ({                              \
> > -    mfn_t m_ = m;                                               \
> > +    mfn_t m_ = (m);                                             \
> >                                                                  \
> >      ASSERT(!mfn_eq(m_, INVALID_MFN));                           \
> >      IS_ALIGNED(PFN_DOWN(v) | mfn_x(m_),                         \
> 
> Minor, typo in subject: x8& -> x86

Nice, that's what you get when you press shift + 6 on a Spanish
keyboard.  Hope it can be adjusted at commit.

Thanks.

Re: [PATCH 1/4] x8&/mm: fix IS_LnE_ALIGNED() to comply with Misra Rule 20.7

[Jan Beulich]

On 19.11.2024 11:34, Roger Pau Monne wrote:
> While not strictly needed to guarantee operator precedence is as expected, add
> the parentheses to comply with Misra Rule 20.7.
> 
> No functional change intended.
> 
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Fixes: 5b52e1b0436f ('x86/mm: skip super-page alignment checks for non-present entries')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

[PATCH 2/4] x86/msi: fix Misra Rule 20.7 in msi.h

[Roger Pau Monne]

Prune unused macros and adjust the remaining ones to parenthesize macro
arguments.

No functional change intended.

Singed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/include/asm/msi.h | 35 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index 748bc3cd6d8b..49a576383288 100644
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -147,33 +147,26 @@ int msi_free_irq(struct msi_desc *entry);
  */
 #define NR_HP_RESERVED_VECTORS 	20
 
-#define msi_control_reg(base)		(base + PCI_MSI_FLAGS)
-#define msi_lower_address_reg(base)	(base + PCI_MSI_ADDRESS_LO)
-#define msi_upper_address_reg(base)	(base + PCI_MSI_ADDRESS_HI)
+#define msi_control_reg(base)		((base) + PCI_MSI_FLAGS)
+#define msi_lower_address_reg(base)	((base) + PCI_MSI_ADDRESS_LO)
+#define msi_upper_address_reg(base)	((base) + PCI_MSI_ADDRESS_HI)
 #define msi_data_reg(base, is64bit)	\
-	( (is64bit == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )
+	((base) + ((is64bit) ? PCI_MSI_DATA_64 : PCI_MSI_DATA_32))
 #define msi_mask_bits_reg(base, is64bit) \
-	( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : base+PCI_MSI_MASK_BIT-4)
+	((base) + PCI_MSI_MASK_BIT - ((is64bit) ? 0 : 4))
 #define msi_pending_bits_reg(base, is64bit) \
 	((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
-#define msi_disable(control)		control &= ~PCI_MSI_FLAGS_ENABLE
 #define multi_msi_capable(control) \
-	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
+	(1U << MASK_EXTR(control, PCI_MSI_FLAGS_QMASK))
 #define multi_msi_enable(control, num) \
-	control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
-#define is_64bit_address(control)	(!!(control & PCI_MSI_FLAGS_64BIT))
-#define is_mask_bit_support(control)	(!!(control & PCI_MSI_FLAGS_MASKBIT))
-#define msi_enable(control, num) multi_msi_enable(control, num); \
-	control |= PCI_MSI_FLAGS_ENABLE
-
-#define msix_control_reg(base)		(base + PCI_MSIX_FLAGS)
-#define msix_table_offset_reg(base)	(base + PCI_MSIX_TABLE)
-#define msix_pba_offset_reg(base)	(base + PCI_MSIX_PBA)
-#define msix_enable(control)	 	control |= PCI_MSIX_FLAGS_ENABLE
-#define msix_disable(control)	 	control &= ~PCI_MSIX_FLAGS_ENABLE
-#define msix_table_size(control) 	((control & PCI_MSIX_FLAGS_QSIZE)+1)
-#define msix_unmask(address)	 	(address & ~PCI_MSIX_VECTOR_BITMASK)
-#define msix_mask(address)		(address | PCI_MSIX_VECTOR_BITMASK)
+	((control) |= MASK_INSR(fls(num) - 1, PCI_MSI_FLAGS_QSIZE))
+#define is_64bit_address(control)	!!((control) & PCI_MSI_FLAGS_64BIT)
+#define is_mask_bit_support(control)	!!((control) & PCI_MSI_FLAGS_MASKBIT)
+
+#define msix_control_reg(base)		((base) + PCI_MSIX_FLAGS)
+#define msix_table_offset_reg(base)	((base) + PCI_MSIX_TABLE)
+#define msix_pba_offset_reg(base)	((base) + PCI_MSIX_PBA)
+#define msix_table_size(control) 	(((control) & PCI_MSIX_FLAGS_QSIZE) + 1)
 
 /*
  * MSI Defined Data Structures
-- 
2.46.0

Re: [PATCH 2/4] x86/msi: fix Misra Rule 20.7 in msi.h

[Andrew Cooper]

On 19/11/2024 10:34 am, Roger Pau Monne wrote:
> Prune unused macros and adjust the remaining ones to parenthesize macro
> arguments.
>
> No functional change intended.
>
> Singed-off-by: Roger Pau Monné <roger.pau@citrix.com>

It's a little early for carol season, isn't it?

It would help to identify which macros are being dropped, because the
diff is far from simple to read.

AFAICT, its:

  msi_disable()
  msi_enable()
  msix_enable()
  msix_disable()
  msix_unmask()
  msix_mask()

Splitting this change does make a marginal improvement in the diff, and
a substantial improvement in `git diff --color-word`'s ability to review
this change.

You've also introduced uses of MASK_EXTR() and MASK_INSR(), which at
least ought to be noted in the commit message.  Technically I think it's
a bugfix for multi_msi_enable(), because I think it now won't overflow
the 3-bit field if an overly large num is passed in.


Bloat-o-meter reports:

add/remove: 0/0 grow/shrink: 3/1 up/down: 15/-61 (-46)
Function                                     old     new   delta
set_iommu_interrupt_handler                  366     373      +7
write_msi_msg                                348     352      +4
init_msi                                     574     578      +4
pci_enable_msi                              1084    1023     -61


Taking the first example, that's caused by swapping this:

> iommu->msi.msi.mpos = ( ((!!(control & 0x80)) == 1) ?
> iommu->msi.msi_attrib.pos+16 : iommu->msi.msi_attrib.pos+16 -4);

for this:

> iommu->msi.msi.mpos = ((iommu->msi.msi_attrib.pos) + 16 -
> (((!!((control) & 0x80))) ? 0 : 4));

and code generation changing from a CMOV to straight-line arithmetic.

In write_msi_msg(), we actually drop a conditional branch and replace it
with straight-line arithmetic.

init_msi() gets a substantial restructuring, but it looks like two
branches are dropped.

pci_enable_msi() has the biggest change, but doesn't obviously reduce
the number of branches.  There is clearly less register setup around
existing branches, so my best guess is that the new macro forms are more
amenable to common-sub-expression-elimination.


Either way, it's all minor.  Staring at the diff for long enough, I'm
pretty sure it's all good.

> ---
>  xen/arch/x86/include/asm/msi.h | 35 ++++++++++++++--------------------
>  1 file changed, 14 insertions(+), 21 deletions(-)
>
> diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
> index 748bc3cd6d8b..49a576383288 100644
> --- a/xen/arch/x86/include/asm/msi.h
> +++ b/xen/arch/x86/include/asm/msi.h
> @@ -147,33 +147,26 @@ int msi_free_irq(struct msi_desc *entry);
>   */
>  #define NR_HP_RESERVED_VECTORS 	20
>  
> -#define msi_control_reg(base)		(base + PCI_MSI_FLAGS)
> -#define msi_lower_address_reg(base)	(base + PCI_MSI_ADDRESS_LO)
> -#define msi_upper_address_reg(base)	(base + PCI_MSI_ADDRESS_HI)
> +#define msi_control_reg(base)		((base) + PCI_MSI_FLAGS)
> +#define msi_lower_address_reg(base)	((base) + PCI_MSI_ADDRESS_LO)
> +#define msi_upper_address_reg(base)	((base) + PCI_MSI_ADDRESS_HI)
>  #define msi_data_reg(base, is64bit)	\
> -	( (is64bit == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )
> +	((base) + ((is64bit) ? PCI_MSI_DATA_64 : PCI_MSI_DATA_32))
>  #define msi_mask_bits_reg(base, is64bit) \
> -	( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : base+PCI_MSI_MASK_BIT-4)
> +	((base) + PCI_MSI_MASK_BIT - ((is64bit) ? 0 : 4))
>  #define msi_pending_bits_reg(base, is64bit) \
>  	((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
> -#define msi_disable(control)		control &= ~PCI_MSI_FLAGS_ENABLE
>  #define multi_msi_capable(control) \
> -	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
> +	(1U << MASK_EXTR(control, PCI_MSI_FLAGS_QMASK))
>  #define multi_msi_enable(control, num) \
> -	control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
> -#define is_64bit_address(control)	(!!(control & PCI_MSI_FLAGS_64BIT))
> -#define is_mask_bit_support(control)	(!!(control & PCI_MSI_FLAGS_MASKBIT))
> -#define msi_enable(control, num) multi_msi_enable(control, num); \
> -	control |= PCI_MSI_FLAGS_ENABLE
> -
> -#define msix_control_reg(base)		(base + PCI_MSIX_FLAGS)
> -#define msix_table_offset_reg(base)	(base + PCI_MSIX_TABLE)
> -#define msix_pba_offset_reg(base)	(base + PCI_MSIX_PBA)
> -#define msix_enable(control)	 	control |= PCI_MSIX_FLAGS_ENABLE
> -#define msix_disable(control)	 	control &= ~PCI_MSIX_FLAGS_ENABLE
> -#define msix_table_size(control) 	((control & PCI_MSIX_FLAGS_QSIZE)+1)
> -#define msix_unmask(address)	 	(address & ~PCI_MSIX_VECTOR_BITMASK)
> -#define msix_mask(address)		(address | PCI_MSIX_VECTOR_BITMASK)
> +	((control) |= MASK_INSR(fls(num) - 1, PCI_MSI_FLAGS_QSIZE))
> +#define is_64bit_address(control)	!!((control) & PCI_MSI_FLAGS_64BIT)
> +#define is_mask_bit_support(control)	!!((control) & PCI_MSI_FLAGS_MASKBIT)

You need to retain the outermost brackets for other MISRA reasons.

I'm happy to fix up on commit, even splitting the patch (seeing as I've
already done the split in order to review the rest).

~Andrew

Re: [PATCH 2/4] x86/msi: fix Misra Rule 20.7 in msi.h

[Roger Pau Monné]

On Tue, Nov 19, 2024 at 02:21:35PM +0000, Andrew Cooper wrote:
> On 19/11/2024 10:34 am, Roger Pau Monne wrote:
> > Prune unused macros and adjust the remaining ones to parenthesize macro
> > arguments.
> >
> > No functional change intended.
> >
> > Singed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> It's a little early for carol season, isn't it?
> 
> It would help to identify which macros are being dropped, because the
> diff is far from simple to read.
> 
> AFAICT, its:
> 
>   msi_disable()
>   msi_enable()
>   msix_enable()
>   msix_disable()
>   msix_unmask()
>   msix_mask()
> 
> Splitting this change does make a marginal improvement in the diff, and
> a substantial improvement in `git diff --color-word`'s ability to review
> this change.

Hm, yes, it would likely be easier to parse, I just went on a spree to
clean it up.

> You've also introduced uses of MASK_EXTR() and MASK_INSR(), which at
> least ought to be noted in the commit message.  Technically I think it's
> a bugfix for multi_msi_enable(), because I think it now won't overflow
> the 3-bit field if an overly large num is passed in.

Hm, I've become used to MASK_{EXTR,INSR}(), so the change felt natural
since I was already adjusting the code.

> 
> Bloat-o-meter reports:
> 
> add/remove: 0/0 grow/shrink: 3/1 up/down: 15/-61 (-46)
> Function                                     old     new   delta
> set_iommu_interrupt_handler                  366     373      +7
> write_msi_msg                                348     352      +4
> init_msi                                     574     578      +4
> pci_enable_msi                              1084    1023     -61
> 
> 
> Taking the first example, that's caused by swapping this:
> 
> > iommu->msi.msi.mpos = ( ((!!(control & 0x80)) == 1) ?
> > iommu->msi.msi_attrib.pos+16 : iommu->msi.msi_attrib.pos+16 -4);
> 
> for this:
> 
> > iommu->msi.msi.mpos = ((iommu->msi.msi_attrib.pos) + 16 -
> > (((!!((control) & 0x80))) ? 0 : 4));
> 
> and code generation changing from a CMOV to straight-line arithmetic.
> 
> In write_msi_msg(), we actually drop a conditional branch and replace it
> with straight-line arithmetic.
> 
> init_msi() gets a substantial restructuring, but it looks like two
> branches are dropped.
> 
> pci_enable_msi() has the biggest change, but doesn't obviously reduce
> the number of branches.  There is clearly less register setup around
> existing branches, so my best guess is that the new macro forms are more
> amenable to common-sub-expression-elimination.
> 
> 
> Either way, it's all minor.  Staring at the diff for long enough, I'm
> pretty sure it's all good.

Thanks.

> > ---
> >  xen/arch/x86/include/asm/msi.h | 35 ++++++++++++++--------------------
> >  1 file changed, 14 insertions(+), 21 deletions(-)
> >
> > diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
> > index 748bc3cd6d8b..49a576383288 100644
> > --- a/xen/arch/x86/include/asm/msi.h
> > +++ b/xen/arch/x86/include/asm/msi.h
> > @@ -147,33 +147,26 @@ int msi_free_irq(struct msi_desc *entry);
> >   */
> >  #define NR_HP_RESERVED_VECTORS 	20
> >  
> > -#define msi_control_reg(base)		(base + PCI_MSI_FLAGS)
> > -#define msi_lower_address_reg(base)	(base + PCI_MSI_ADDRESS_LO)
> > -#define msi_upper_address_reg(base)	(base + PCI_MSI_ADDRESS_HI)
> > +#define msi_control_reg(base)		((base) + PCI_MSI_FLAGS)
> > +#define msi_lower_address_reg(base)	((base) + PCI_MSI_ADDRESS_LO)
> > +#define msi_upper_address_reg(base)	((base) + PCI_MSI_ADDRESS_HI)
> >  #define msi_data_reg(base, is64bit)	\
> > -	( (is64bit == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )
> > +	((base) + ((is64bit) ? PCI_MSI_DATA_64 : PCI_MSI_DATA_32))
> >  #define msi_mask_bits_reg(base, is64bit) \
> > -	( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : base+PCI_MSI_MASK_BIT-4)
> > +	((base) + PCI_MSI_MASK_BIT - ((is64bit) ? 0 : 4))
> >  #define msi_pending_bits_reg(base, is64bit) \
> >  	((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
> > -#define msi_disable(control)		control &= ~PCI_MSI_FLAGS_ENABLE
> >  #define multi_msi_capable(control) \
> > -	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
> > +	(1U << MASK_EXTR(control, PCI_MSI_FLAGS_QMASK))
> >  #define multi_msi_enable(control, num) \
> > -	control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
> > -#define is_64bit_address(control)	(!!(control & PCI_MSI_FLAGS_64BIT))
> > -#define is_mask_bit_support(control)	(!!(control & PCI_MSI_FLAGS_MASKBIT))
> > -#define msi_enable(control, num) multi_msi_enable(control, num); \
> > -	control |= PCI_MSI_FLAGS_ENABLE
> > -
> > -#define msix_control_reg(base)		(base + PCI_MSIX_FLAGS)
> > -#define msix_table_offset_reg(base)	(base + PCI_MSIX_TABLE)
> > -#define msix_pba_offset_reg(base)	(base + PCI_MSIX_PBA)
> > -#define msix_enable(control)	 	control |= PCI_MSIX_FLAGS_ENABLE
> > -#define msix_disable(control)	 	control &= ~PCI_MSIX_FLAGS_ENABLE
> > -#define msix_table_size(control) 	((control & PCI_MSIX_FLAGS_QSIZE)+1)
> > -#define msix_unmask(address)	 	(address & ~PCI_MSIX_VECTOR_BITMASK)
> > -#define msix_mask(address)		(address | PCI_MSIX_VECTOR_BITMASK)
> > +	((control) |= MASK_INSR(fls(num) - 1, PCI_MSI_FLAGS_QSIZE))
> > +#define is_64bit_address(control)	!!((control) & PCI_MSI_FLAGS_64BIT)
> > +#define is_mask_bit_support(control)	!!((control) & PCI_MSI_FLAGS_MASKBIT)
> 
> You need to retain the outermost brackets for other MISRA reasons.

I was borderline on dropping those braces, as I was expecting Misra to
require them.

> I'm happy to fix up on commit, even splitting the patch (seeing as I've
> already done the split in order to review the rest).

Fine, by split I think you mean the pruning of unused macros vs the
fixing of the parentheses?

Thanks, Roger.

Re: [PATCH 2/4] x86/msi: fix Misra Rule 20.7 in msi.h

[Andrew Cooper]

On 19/11/2024 2:39 pm, Roger Pau Monné wrote:
> On Tue, Nov 19, 2024 at 02:21:35PM +0000, Andrew Cooper wrote:
>> On 19/11/2024 10:34 am, Roger Pau Monne wrote:
>>> Prune unused macros and adjust the remaining ones to parenthesize macro
>>> arguments.
>>>
>>> No functional change intended.
>>>
>>> Singed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> It's a little early for carol season, isn't it?
>>
>> It would help to identify which macros are being dropped, because the
>> diff is far from simple to read.
>>
>> AFAICT, its:
>>
>>   msi_disable()
>>   msi_enable()
>>   msix_enable()
>>   msix_disable()
>>   msix_unmask()
>>   msix_mask()
>>
>> Splitting this change does make a marginal improvement in the diff, and
>> a substantial improvement in `git diff --color-word`'s ability to review
>> this change.
> Hm, yes, it would likely be easier to parse, I just went on a spree to
> clean it up.
>
>> You've also introduced uses of MASK_EXTR() and MASK_INSR(), which at
>> least ought to be noted in the commit message.  Technically I think it's
>> a bugfix for multi_msi_enable(), because I think it now won't overflow
>> the 3-bit field if an overly large num is passed in.
> Hm, I've become used to MASK_{EXTR,INSR}(), so the change felt natural
> since I was already adjusting the code.
>
>> Bloat-o-meter reports:
>>
>> add/remove: 0/0 grow/shrink: 3/1 up/down: 15/-61 (-46)
>> Function                                     old     new   delta
>> set_iommu_interrupt_handler                  366     373      +7
>> write_msi_msg                                348     352      +4
>> init_msi                                     574     578      +4
>> pci_enable_msi                              1084    1023     -61
>>
>>
>> Taking the first example, that's caused by swapping this:
>>
>>> iommu->msi.msi.mpos = ( ((!!(control & 0x80)) == 1) ?
>>> iommu->msi.msi_attrib.pos+16 : iommu->msi.msi_attrib.pos+16 -4);
>> for this:
>>
>>> iommu->msi.msi.mpos = ((iommu->msi.msi_attrib.pos) + 16 -
>>> (((!!((control) & 0x80))) ? 0 : 4));
>> and code generation changing from a CMOV to straight-line arithmetic.
>>
>> In write_msi_msg(), we actually drop a conditional branch and replace it
>> with straight-line arithmetic.
>>
>> init_msi() gets a substantial restructuring, but it looks like two
>> branches are dropped.
>>
>> pci_enable_msi() has the biggest change, but doesn't obviously reduce
>> the number of branches.  There is clearly less register setup around
>> existing branches, so my best guess is that the new macro forms are more
>> amenable to common-sub-expression-elimination.
>>
>>
>> Either way, it's all minor.  Staring at the diff for long enough, I'm
>> pretty sure it's all good.
> Thanks.
>
>>> ---
>>>  xen/arch/x86/include/asm/msi.h | 35 ++++++++++++++--------------------
>>>  1 file changed, 14 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
>>> index 748bc3cd6d8b..49a576383288 100644
>>> --- a/xen/arch/x86/include/asm/msi.h
>>> +++ b/xen/arch/x86/include/asm/msi.h
>>> @@ -147,33 +147,26 @@ int msi_free_irq(struct msi_desc *entry);
>>>   */
>>>  #define NR_HP_RESERVED_VECTORS 	20
>>>  
>>> -#define msi_control_reg(base)		(base + PCI_MSI_FLAGS)
>>> -#define msi_lower_address_reg(base)	(base + PCI_MSI_ADDRESS_LO)
>>> -#define msi_upper_address_reg(base)	(base + PCI_MSI_ADDRESS_HI)
>>> +#define msi_control_reg(base)		((base) + PCI_MSI_FLAGS)
>>> +#define msi_lower_address_reg(base)	((base) + PCI_MSI_ADDRESS_LO)
>>> +#define msi_upper_address_reg(base)	((base) + PCI_MSI_ADDRESS_HI)
>>>  #define msi_data_reg(base, is64bit)	\
>>> -	( (is64bit == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )
>>> +	((base) + ((is64bit) ? PCI_MSI_DATA_64 : PCI_MSI_DATA_32))
>>>  #define msi_mask_bits_reg(base, is64bit) \
>>> -	( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : base+PCI_MSI_MASK_BIT-4)
>>> +	((base) + PCI_MSI_MASK_BIT - ((is64bit) ? 0 : 4))
>>>  #define msi_pending_bits_reg(base, is64bit) \
>>>  	((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
>>> -#define msi_disable(control)		control &= ~PCI_MSI_FLAGS_ENABLE
>>>  #define multi_msi_capable(control) \
>>> -	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
>>> +	(1U << MASK_EXTR(control, PCI_MSI_FLAGS_QMASK))
>>>  #define multi_msi_enable(control, num) \
>>> -	control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
>>> -#define is_64bit_address(control)	(!!(control & PCI_MSI_FLAGS_64BIT))
>>> -#define is_mask_bit_support(control)	(!!(control & PCI_MSI_FLAGS_MASKBIT))
>>> -#define msi_enable(control, num) multi_msi_enable(control, num); \
>>> -	control |= PCI_MSI_FLAGS_ENABLE
>>> -
>>> -#define msix_control_reg(base)		(base + PCI_MSIX_FLAGS)
>>> -#define msix_table_offset_reg(base)	(base + PCI_MSIX_TABLE)
>>> -#define msix_pba_offset_reg(base)	(base + PCI_MSIX_PBA)
>>> -#define msix_enable(control)	 	control |= PCI_MSIX_FLAGS_ENABLE
>>> -#define msix_disable(control)	 	control &= ~PCI_MSIX_FLAGS_ENABLE
>>> -#define msix_table_size(control) 	((control & PCI_MSIX_FLAGS_QSIZE)+1)
>>> -#define msix_unmask(address)	 	(address & ~PCI_MSIX_VECTOR_BITMASK)
>>> -#define msix_mask(address)		(address | PCI_MSIX_VECTOR_BITMASK)
>>> +	((control) |= MASK_INSR(fls(num) - 1, PCI_MSI_FLAGS_QSIZE))
>>> +#define is_64bit_address(control)	!!((control) & PCI_MSI_FLAGS_64BIT)
>>> +#define is_mask_bit_support(control)	!!((control) & PCI_MSI_FLAGS_MASKBIT)
>> You need to retain the outermost brackets for other MISRA reasons.
> I was borderline on dropping those braces, as I was expecting Misra to
> require them.
>
>> I'm happy to fix up on commit, even splitting the patch (seeing as I've
>> already done the split in order to review the rest).
> Fine, by split I think you mean the pruning of unused macros vs the
> fixing of the parentheses?

Split pruning out into another patch, fold the bracket fix into this one.

~Andrew

Re: [PATCH 2/4] x86/msi: fix Misra Rule 20.7 in msi.h

[Roger Pau Monné]

On Tue, Nov 19, 2024 at 03:35:34PM +0000, Andrew Cooper wrote:
> On 19/11/2024 2:39 pm, Roger Pau Monné wrote:
> > On Tue, Nov 19, 2024 at 02:21:35PM +0000, Andrew Cooper wrote:
> >> On 19/11/2024 10:34 am, Roger Pau Monne wrote:
> >>> ---
> >>>  xen/arch/x86/include/asm/msi.h | 35 ++++++++++++++--------------------
> >>>  1 file changed, 14 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
> >>> index 748bc3cd6d8b..49a576383288 100644
> >>> --- a/xen/arch/x86/include/asm/msi.h
> >>> +++ b/xen/arch/x86/include/asm/msi.h
> >>> @@ -147,33 +147,26 @@ int msi_free_irq(struct msi_desc *entry);
> >>>   */
> >>>  #define NR_HP_RESERVED_VECTORS 	20
> >>>  
> >>> -#define msi_control_reg(base)		(base + PCI_MSI_FLAGS)
> >>> -#define msi_lower_address_reg(base)	(base + PCI_MSI_ADDRESS_LO)
> >>> -#define msi_upper_address_reg(base)	(base + PCI_MSI_ADDRESS_HI)
> >>> +#define msi_control_reg(base)		((base) + PCI_MSI_FLAGS)
> >>> +#define msi_lower_address_reg(base)	((base) + PCI_MSI_ADDRESS_LO)
> >>> +#define msi_upper_address_reg(base)	((base) + PCI_MSI_ADDRESS_HI)
> >>>  #define msi_data_reg(base, is64bit)	\
> >>> -	( (is64bit == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )
> >>> +	((base) + ((is64bit) ? PCI_MSI_DATA_64 : PCI_MSI_DATA_32))
> >>>  #define msi_mask_bits_reg(base, is64bit) \
> >>> -	( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : base+PCI_MSI_MASK_BIT-4)
> >>> +	((base) + PCI_MSI_MASK_BIT - ((is64bit) ? 0 : 4))
> >>>  #define msi_pending_bits_reg(base, is64bit) \
> >>>  	((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
> >>> -#define msi_disable(control)		control &= ~PCI_MSI_FLAGS_ENABLE
> >>>  #define multi_msi_capable(control) \
> >>> -	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
> >>> +	(1U << MASK_EXTR(control, PCI_MSI_FLAGS_QMASK))
> >>>  #define multi_msi_enable(control, num) \
> >>> -	control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
> >>> -#define is_64bit_address(control)	(!!(control & PCI_MSI_FLAGS_64BIT))
> >>> -#define is_mask_bit_support(control)	(!!(control & PCI_MSI_FLAGS_MASKBIT))
> >>> -#define msi_enable(control, num) multi_msi_enable(control, num); \
> >>> -	control |= PCI_MSI_FLAGS_ENABLE
> >>> -
> >>> -#define msix_control_reg(base)		(base + PCI_MSIX_FLAGS)
> >>> -#define msix_table_offset_reg(base)	(base + PCI_MSIX_TABLE)
> >>> -#define msix_pba_offset_reg(base)	(base + PCI_MSIX_PBA)
> >>> -#define msix_enable(control)	 	control |= PCI_MSIX_FLAGS_ENABLE
> >>> -#define msix_disable(control)	 	control &= ~PCI_MSIX_FLAGS_ENABLE
> >>> -#define msix_table_size(control) 	((control & PCI_MSIX_FLAGS_QSIZE)+1)
> >>> -#define msix_unmask(address)	 	(address & ~PCI_MSIX_VECTOR_BITMASK)
> >>> -#define msix_mask(address)		(address | PCI_MSIX_VECTOR_BITMASK)
> >>> +	((control) |= MASK_INSR(fls(num) - 1, PCI_MSI_FLAGS_QSIZE))
> >>> +#define is_64bit_address(control)	!!((control) & PCI_MSI_FLAGS_64BIT)
> >>> +#define is_mask_bit_support(control)	!!((control) & PCI_MSI_FLAGS_MASKBIT)
> >> You need to retain the outermost brackets for other MISRA reasons.
> > I was borderline on dropping those braces, as I was expecting Misra to
> > require them.
> >
> >> I'm happy to fix up on commit, even splitting the patch (seeing as I've
> >> already done the split in order to review the rest).
> > Fine, by split I think you mean the pruning of unused macros vs the
> > fixing of the parentheses?
> 
> Split pruning out into another patch, fold the bracket fix into this one.

If you want I can do that myself, as I will likely need to resend #3
to drop the unneeded __maybe_unused attribute.

Thanks, Roger.

[PATCH 3/4] x86/uaccess: rework user access speculative harden guards

[Roger Pau Monne]

The current guards to select whether user accesses should be speculative
hardened violate Misra rule 20.7, as the UA_KEEP() macro doesn't (and can't)
parenthesize the 'args' argument.

Change the logic so the guard is implemented inside the assembly block using
the .if assembly directive.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
The guard check could be moved inside of the guest_access_mask_ptr macro, but
given it's limited usages it's clearer to keep the check in the callers IMO.
---
 xen/arch/x86/include/asm/uaccess.h | 45 +++++++++++++++---------------
 xen/arch/x86/usercopy.c            | 26 ++++++++---------
 2 files changed, 35 insertions(+), 36 deletions(-)

diff --git a/xen/arch/x86/include/asm/uaccess.h b/xen/arch/x86/include/asm/uaccess.h
index 2d01669b9610..6b8150ac221a 100644
--- a/xen/arch/x86/include/asm/uaccess.h
+++ b/xen/arch/x86/include/asm/uaccess.h
@@ -24,9 +24,6 @@ unsigned int copy_from_unsafe_ll(void *to, const void *from, unsigned int n);
 void noreturn __get_user_bad(void);
 void noreturn __put_user_bad(void);
 
-#define UA_KEEP(args...) args
-#define UA_DROP(args...)
-
 /**
  * get_guest: - Get a simple variable from guest space.
  * @x:   Variable to store result.
@@ -104,7 +101,7 @@ void noreturn __put_user_bad(void);
 #define put_unsafe(x, ptr)						\
 ({									\
 	int err_; 							\
-	put_unsafe_size(x, ptr, sizeof(*(ptr)), UA_DROP, err_, -EFAULT);\
+	put_unsafe_size(x, ptr, sizeof(*(ptr)), 0, err_, -EFAULT);      \
 	err_;								\
 })
 
@@ -126,7 +123,7 @@ void noreturn __put_user_bad(void);
 #define get_unsafe(x, ptr)						\
 ({									\
 	int err_; 							\
-	get_unsafe_size(x, ptr, sizeof(*(ptr)), UA_DROP, err_, -EFAULT);\
+	get_unsafe_size(x, ptr, sizeof(*(ptr)), 0, err_, -EFAULT);	\
 	err_;								\
 })
 
@@ -155,9 +152,9 @@ struct __large_struct { unsigned long buf[100]; };
  */
 #define put_unsafe_asm(x, addr, GUARD, err, itype, rtype, ltype, errret) \
 	__asm__ __volatile__(						\
-		GUARD(							\
+		".if " STR(GUARD) "\n"					\
 		"	guest_access_mask_ptr %[ptr], %[scr1], %[scr2]\n" \
-		)							\
+		".endif\n"						\
 		"1:	mov"itype" %"rtype"[val], (%[ptr])\n"		\
 		"2:\n"							\
 		".section .fixup,\"ax\"\n"				\
@@ -165,16 +162,16 @@ struct __large_struct { unsigned long buf[100]; };
 		"	jmp 2b\n"					\
 		".previous\n"						\
 		_ASM_EXTABLE(1b, 3b)					\
-		: [ret] "+r" (err), [ptr] "=&r" (dummy_)		\
-		  GUARD(, [scr1] "=&r" (dummy_), [scr2] "=&r" (dummy_))	\
+		: [ret] "+r" (err), [ptr] "=&r" (dummy_),		\
+		  [scr1] "=&r" (dummy_), [scr2] "=&r" (dummy_)		\
 		: [val] ltype (x), "m" (__m(addr)),			\
 		  "[ptr]" (addr), [errno] "i" (errret))
 
 #define get_unsafe_asm(x, addr, GUARD, err, rtype, ltype, errret)	\
 	__asm__ __volatile__(						\
-		GUARD(							\
+		".if " STR(GUARD) "\n"					\
 		"	guest_access_mask_ptr %[ptr], %[scr1], %[scr2]\n" \
-		)							\
+		".endif\n"						\
 		"1:	mov (%[ptr]), %"rtype"[val]\n"			\
 		"2:\n"							\
 		".section .fixup,\"ax\"\n"				\
@@ -184,14 +181,15 @@ struct __large_struct { unsigned long buf[100]; };
 		".previous\n"						\
 		_ASM_EXTABLE(1b, 3b)					\
 		: [ret] "+r" (err), [val] ltype (x),			\
-		  [ptr] "=&r" (dummy_)					\
-		  GUARD(, [scr1] "=&r" (dummy_), [scr2] "=&r" (dummy_))	\
+		  [ptr] "=&r" (dummy_),					\
+		  [scr1] "=&r" (dummy_), [scr2] "=&r" (dummy_)		\
 		: "m" (__m(addr)), "[ptr]" (addr),			\
 		  [errno] "i" (errret))
 
 #define put_unsafe_size(x, ptr, size, grd, retval, errret)                 \
 do {                                                                       \
     retval = 0;                                                            \
+    BUILD_BUG_ON((grd) != 0 && (grd) != 1);                                \
     stac();                                                                \
     switch ( size )                                                        \
     {                                                                      \
@@ -214,11 +212,12 @@ do {                                                                       \
 } while ( false )
 
 #define put_guest_size(x, ptr, size, retval, errret) \
-    put_unsafe_size(x, ptr, size, UA_KEEP, retval, errret)
+    put_unsafe_size(x, ptr, size, 1, retval, errret)
 
 #define get_unsafe_size(x, ptr, size, grd, retval, errret)                 \
 do {                                                                       \
     retval = 0;                                                            \
+    BUILD_BUG_ON((grd) != 0 && (grd) != 1);                                \
     stac();                                                                \
     switch ( size )                                                        \
     {                                                                      \
@@ -233,7 +232,7 @@ do {                                                                       \
 } while ( false )
 
 #define get_guest_size(x, ptr, size, retval, errret)                       \
-    get_unsafe_size(x, ptr, size, UA_KEEP, retval, errret)
+    get_unsafe_size(x, ptr, size, 1, retval, errret)
 
 /**
  * __copy_to_guest_pv: - Copy a block of data into guest space, with less
@@ -333,16 +332,16 @@ copy_to_unsafe(void __user *to, const void *from, unsigned int n)
 
         switch (n) {
         case 1:
-            put_unsafe_size(*(const uint8_t *)from, to, 1, UA_DROP, ret, 1);
+            put_unsafe_size(*(const uint8_t *)from, to, 1, 0, ret, 1);
             return ret;
         case 2:
-            put_unsafe_size(*(const uint16_t *)from, to, 2, UA_DROP, ret, 2);
+            put_unsafe_size(*(const uint16_t *)from, to, 2, 0, ret, 2);
             return ret;
         case 4:
-            put_unsafe_size(*(const uint32_t *)from, to, 4, UA_DROP, ret, 4);
+            put_unsafe_size(*(const uint32_t *)from, to, 4, 0, ret, 4);
             return ret;
         case 8:
-            put_unsafe_size(*(const uint64_t *)from, to, 8, UA_DROP, ret, 8);
+            put_unsafe_size(*(const uint64_t *)from, to, 8, 0, ret, 8);
             return ret;
         }
     }
@@ -374,16 +373,16 @@ copy_from_unsafe(void *to, const void __user *from, unsigned int n)
         switch ( n )
         {
         case 1:
-            get_unsafe_size(*(uint8_t *)to, from, 1, UA_DROP, ret, 1);
+            get_unsafe_size(*(uint8_t *)to, from, 1, 0, ret, 1);
             return ret;
         case 2:
-            get_unsafe_size(*(uint16_t *)to, from, 2, UA_DROP, ret, 2);
+            get_unsafe_size(*(uint16_t *)to, from, 2, 0, ret, 2);
             return ret;
         case 4:
-            get_unsafe_size(*(uint32_t *)to, from, 4, UA_DROP, ret, 4);
+            get_unsafe_size(*(uint32_t *)to, from, 4, 0, ret, 4);
             return ret;
         case 8:
-            get_unsafe_size(*(uint64_t *)to, from, 8, UA_DROP, ret, 8);
+            get_unsafe_size(*(uint64_t *)to, from, 8, 0, ret, 8);
             return ret;
         }
     }
diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c
index 7ab2009efe4c..d66beecc5507 100644
--- a/xen/arch/x86/usercopy.c
+++ b/xen/arch/x86/usercopy.c
@@ -11,23 +11,23 @@
 #include <asm/uaccess.h>
 
 #ifndef GUARD
-# define GUARD UA_KEEP
+# define GUARD 1
 #endif
 
 unsigned int copy_to_guest_ll(void __user *to, const void *from, unsigned int n)
 {
-    GUARD(unsigned dummy);
+    unsigned __maybe_unused dummy;
 
     stac();
     asm volatile (
-        GUARD(
+        ".if " STR(GUARD) "\n"
         "    guest_access_mask_ptr %[to], %q[scratch1], %q[scratch2]\n"
-        )
+        ".endif\n"
         "1:  rep movsb\n"
         "2:\n"
         _ASM_EXTABLE(1b, 2b)
-        : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from)
-          GUARD(, [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy))
+        : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from),
+          [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy)
         :: "memory" );
     clac();
 
@@ -40,9 +40,9 @@ unsigned int copy_from_guest_ll(void *to, const void __user *from, unsigned int
 
     stac();
     asm volatile (
-        GUARD(
+        ".if " STR(GUARD) "\n"
         "    guest_access_mask_ptr %[from], %q[scratch1], %q[scratch2]\n"
-        )
+        ".endif\n"
         "1:  rep movsb\n"
         "2:\n"
         ".section .fixup,\"ax\"\n"
@@ -56,15 +56,15 @@ unsigned int copy_from_guest_ll(void *to, const void __user *from, unsigned int
         ".previous\n"
         _ASM_EXTABLE(1b, 6b)
         : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from),
-          [aux] "=&r" (dummy)
-          GUARD(, [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy))
+          [aux] "=&r" (dummy),
+          [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy)
         :: "memory" );
     clac();
 
     return n;
 }
 
-#if GUARD(1) + 0
+#if GUARD
 
 /**
  * copy_to_guest_pv: - Copy a block of data into PV guest space.
@@ -140,14 +140,14 @@ unsigned int copy_from_guest_pv(void *to, const void __user *from,
 }
 
 # undef GUARD
-# define GUARD UA_DROP
+# define GUARD 0
 # define copy_to_guest_ll copy_to_unsafe_ll
 # define copy_from_guest_ll copy_from_unsafe_ll
 # undef __user
 # define __user
 # include __FILE__
 
-#endif /* GUARD(1) */
+#endif /* GUARD */
 
 /*
  * Local variables:
-- 
2.46.0

Re: [PATCH 3/4] x86/uaccess: rework user access speculative harden guards

[Jan Beulich]

On 19.11.2024 11:34, Roger Pau Monne wrote:
> The current guards to select whether user accesses should be speculative
> hardened violate Misra rule 20.7, as the UA_KEEP() macro doesn't (and can't)
> parenthesize the 'args' argument.
> 
> Change the logic so the guard is implemented inside the assembly block using
> the .if assembly directive.

Hmm, interesting idea. I don't overly like emitting stuff to pre-processed
and even assembly files, but doing so is probably warranted here. Nevertheless:
Did we consider at all to deviate these macros instead?

> --- a/xen/arch/x86/usercopy.c
> +++ b/xen/arch/x86/usercopy.c
> @@ -11,23 +11,23 @@
>  #include <asm/uaccess.h>
>  
>  #ifndef GUARD
> -# define GUARD UA_KEEP
> +# define GUARD 1
>  #endif

At least in cases like this one I think a comment is necessary, perhaps as
terse as /* Keep */ (and /* Drop */ further down).

Jan

Re: [PATCH 3/4] x86/uaccess: rework user access speculative harden guards

[Roger Pau Monné]

On Tue, Nov 19, 2024 at 03:29:58PM +0100, Jan Beulich wrote:
> On 19.11.2024 11:34, Roger Pau Monne wrote:
> > The current guards to select whether user accesses should be speculative
> > hardened violate Misra rule 20.7, as the UA_KEEP() macro doesn't (and can't)
> > parenthesize the 'args' argument.
> > 
> > Change the logic so the guard is implemented inside the assembly block using
> > the .if assembly directive.
> 
> Hmm, interesting idea. I don't overly like emitting stuff to pre-processed
> and even assembly files, but doing so is probably warranted here. Nevertheless:
> Did we consider at all to deviate these macros instead?

I think the proposal is not overly ugly, as I would otherwise simply
suggest to deviate.  I'm assuming the preference is to attempt to fix
when possible rather than deviate.

> > --- a/xen/arch/x86/usercopy.c
> > +++ b/xen/arch/x86/usercopy.c
> > @@ -11,23 +11,23 @@
> >  #include <asm/uaccess.h>
> >  
> >  #ifndef GUARD
> > -# define GUARD UA_KEEP
> > +# define GUARD 1
> >  #endif
> 
> At least in cases like this one I think a comment is necessary, perhaps as
> terse as /* Keep */ (and /* Drop */ further down).

Right, can adjust if we agree this is the way forward.

Thanks, Roger.

Re: [PATCH 3/4] x86/uaccess: rework user access speculative harden guards

[Andrew Cooper]

On 19/11/2024 10:34 am, Roger Pau Monne wrote:
> The current guards to select whether user accesses should be speculative
> hardened violate Misra rule 20.7, as the UA_KEEP() macro doesn't (and can't)
> parenthesize the 'args' argument.
>
> Change the logic so the guard is implemented inside the assembly block using
> the .if assembly directive.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> The guard check could be moved inside of the guest_access_mask_ptr macro, but
> given it's limited usages it's clearer to keep the check in the callers IMO.

Overall this is far more legible, and I'm tempted to take it on that
justification alone.  But this is Jan's pile of magic.

There is a weird effect from this change:

add/remove: 2/0 grow/shrink: 2/2 up/down: 740/-739 (1)
Function                                     old     new   delta
build_symbol_table                             -     686    +686
build_symbol_table.cold                        -      48     +48
pv_map_ldt_shadow_page                       641     644      +3
pv_emulate_gate_op                          2919    2922      +3
livepatch_op.cold                            557     509     -48
livepatch_op                                5952    5261    -691

which is clearly changing inlining decisions.  I suspect it's related to...

> diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c
> index 7ab2009efe4c..d66beecc5507 100644
> --- a/xen/arch/x86/usercopy.c
> +++ b/xen/arch/x86/usercopy.c
> @@ -11,23 +11,23 @@
>  #include <asm/uaccess.h>
>  
>  #ifndef GUARD
> -# define GUARD UA_KEEP
> +# define GUARD 1
>  #endif
>  
>  unsigned int copy_to_guest_ll(void __user *to, const void *from, unsigned int n)
>  {
> -    GUARD(unsigned dummy);
> +    unsigned __maybe_unused dummy;

... this.  This doesn't need to be __maybe_unused, because ...

>  
>      stac();
>      asm volatile (
> -        GUARD(
> +        ".if " STR(GUARD) "\n"
>          "    guest_access_mask_ptr %[to], %q[scratch1], %q[scratch2]\n"
> -        )
> +        ".endif\n"
>          "1:  rep movsb\n"
>          "2:\n"
>          _ASM_EXTABLE(1b, 2b)
> -        : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from)
> -          GUARD(, [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy))
> +        : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from),
> +          [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy)

... these parameters are referenced unconditionally.

However, it does mean the compiler is spilling the scratch registers
even when guard is 0.  I expect this is what is affecting the inlining
decisions.

~Andrew

Re: [PATCH 3/4] x86/uaccess: rework user access speculative harden guards

[Roger Pau Monné]

On Tue, Nov 19, 2024 at 03:31:34PM +0000, Andrew Cooper wrote:
> On 19/11/2024 10:34 am, Roger Pau Monne wrote:
> > The current guards to select whether user accesses should be speculative
> > hardened violate Misra rule 20.7, as the UA_KEEP() macro doesn't (and can't)
> > parenthesize the 'args' argument.
> >
> > Change the logic so the guard is implemented inside the assembly block using
> > the .if assembly directive.
> >
> > No functional change intended.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > The guard check could be moved inside of the guest_access_mask_ptr macro, but
> > given it's limited usages it's clearer to keep the check in the callers IMO.
> 
> Overall this is far more legible, and I'm tempted to take it on that
> justification alone.  But this is Jan's pile of magic.
> 
> There is a weird effect from this change:
> 
> add/remove: 2/0 grow/shrink: 2/2 up/down: 740/-739 (1)
> Function                                     old     new   delta
> build_symbol_table                             -     686    +686
> build_symbol_table.cold                        -      48     +48
> pv_map_ldt_shadow_page                       641     644      +3
> pv_emulate_gate_op                          2919    2922      +3
> livepatch_op.cold                            557     509     -48
> livepatch_op                                5952    5261    -691

So build_symbol_table() is no longer inlined in load_payload_data()
and thus livepatch_op().

> which is clearly changing inlining decisions.  I suspect it's related to...
> 
> > diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c
> > index 7ab2009efe4c..d66beecc5507 100644
> > --- a/xen/arch/x86/usercopy.c
> > +++ b/xen/arch/x86/usercopy.c
> > @@ -11,23 +11,23 @@
> >  #include <asm/uaccess.h>
> >  
> >  #ifndef GUARD
> > -# define GUARD UA_KEEP
> > +# define GUARD 1
> >  #endif
> >  
> >  unsigned int copy_to_guest_ll(void __user *to, const void *from, unsigned int n)
> >  {
> > -    GUARD(unsigned dummy);
> > +    unsigned __maybe_unused dummy;
> 
> ... this.  This doesn't need to be __maybe_unused, because ...

This is a leftover from when I attempted to use preprocessor #if GUARD
below, and the compiler would then complain about dummy being
unused.

I can fix if there's agreement on the basis of the change.

> >  
> >      stac();
> >      asm volatile (
> > -        GUARD(
> > +        ".if " STR(GUARD) "\n"
> >          "    guest_access_mask_ptr %[to], %q[scratch1], %q[scratch2]\n"
> > -        )
> > +        ".endif\n"
> >          "1:  rep movsb\n"
> >          "2:\n"
> >          _ASM_EXTABLE(1b, 2b)
> > -        : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from)
> > -          GUARD(, [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy))
> > +        : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from),
> > +          [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy)
> 
> ... these parameters are referenced unconditionally.
> 
> However, it does mean the compiler is spilling the scratch registers
> even when guard is 0.  I expect this is what is affecting the inlining
> decisions.

That's my understanding yes, the registers will be spilled.

Thanks, Roger.

Re: [PATCH 3/4] x86/uaccess: rework user access speculative harden guards

[Jan Beulich]

On 19.11.2024 16:31, Andrew Cooper wrote:
> On 19/11/2024 10:34 am, Roger Pau Monne wrote:
> Overall this is far more legible, and I'm tempted to take it on that
> justification alone.  But this is Jan's pile of magic.
> 
> There is a weird effect from this change:
> 
> add/remove: 2/0 grow/shrink: 2/2 up/down: 740/-739 (1)
> Function                                     old     new   delta
> build_symbol_table                             -     686    +686
> build_symbol_table.cold                        -      48     +48
> pv_map_ldt_shadow_page                       641     644      +3
> pv_emulate_gate_op                          2919    2922      +3
> livepatch_op.cold                            557     509     -48
> livepatch_op                                5952    5261    -691
> 
> which is clearly changing inlining decisions.  I suspect it's related to...
> 
>> --- a/xen/arch/x86/usercopy.c
>> +++ b/xen/arch/x86/usercopy.c
>> @@ -11,23 +11,23 @@
>>  #include <asm/uaccess.h>
>>  
>>  #ifndef GUARD
>> -# define GUARD UA_KEEP
>> +# define GUARD 1
>>  #endif
>>  
>>  unsigned int copy_to_guest_ll(void __user *to, const void *from, unsigned int n)
>>  {
>> -    GUARD(unsigned dummy);
>> +    unsigned __maybe_unused dummy;
> 
> ... this.  This doesn't need to be __maybe_unused, because ...
> 
>>  
>>      stac();
>>      asm volatile (
>> -        GUARD(
>> +        ".if " STR(GUARD) "\n"
>>          "    guest_access_mask_ptr %[to], %q[scratch1], %q[scratch2]\n"
>> -        )
>> +        ".endif\n"
>>          "1:  rep movsb\n"
>>          "2:\n"
>>          _ASM_EXTABLE(1b, 2b)
>> -        : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from)
>> -          GUARD(, [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy))
>> +        : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from),
>> +          [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy)
> 
> ... these parameters are referenced unconditionally.
> 
> However, it does mean the compiler is spilling the scratch registers
> even when guard is 0.  I expect this is what is affecting the inlining
> decisions.

Or the increased number of newlines that the asm() expands to (or the
combination of both).

Jan

[PATCH 4/4] automation/eclair: make Misra rule 20.7 blocking for x86 also

[Roger Pau Monne]

There are no violations left, make the rule globally blocking for both x86 and
ARM.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 automation/eclair_analysis/ECLAIR/tagging.ecl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl b/automation/eclair_analysis/ECLAIR/tagging.ecl
index 9318e5b10ca8..dcddba2e061b 100644
--- a/automation/eclair_analysis/ECLAIR/tagging.ecl
+++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
@@ -79,6 +79,7 @@ MC3R1.R20.2||
 MC3R1.R20.3||
 MC3R1.R20.4||
 MC3R1.R20.6||
+MC3R1.R20.7||
 MC3R1.R20.9||
 MC3R1.R20.11||
 MC3R1.R20.12||
@@ -115,7 +116,7 @@ if(string_equal(target,"x86_64"),
 )
 
 if(string_equal(target,"arm64"),
-    service_selector({"additional_clean_guidelines","MC3R1.R2.1||MC3R1.R5.3||MC3.R11.2||MC3R1.R16.6||MC3R1.R20.7"})
+    service_selector({"additional_clean_guidelines","MC3R1.R2.1||MC3R1.R5.3||MC3.R11.2||MC3R1.R16.6"})
 )
 
 -reports+={clean:added,"service(clean_guidelines_common||additional_clean_guidelines)"}
-- 
2.46.0

Re: [PATCH 4/4] automation/eclair: make Misra rule 20.7 blocking for x86 also

[Andrew Cooper]

On 19/11/2024 10:34 am, Roger Pau Monne wrote:
> There are no violations left, make the rule globally blocking for both x86 and
> ARM.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Obviously subject to all the other patches going in.

~Andrew