x86: Declarations for compat_set_{cx,px}_pminfo()

x86: Declarations for compat_set_{cx,px}_pminfo()

[Andrew Cooper]

Hello,

Eclair reports Rule 8.4 violations for these two.  They're real
violations, but fixing them is proving challenging.

Because of how x86_64/platform_hypercall.c sets up some defines and
includes ../platform_hypercall.c, the declarations of
compat_set_{cx,px}_pminfo() of pmstat.h become compat ones.

Therefore the real hypercall handler does see a correct declaration.

However, the implementation in cpufreq.c does not get the compat-ified
version of pmstat.h, so misses the declaration and thus the violation.

Worse however is the fact that cpufreq.c depends on not having the:

#define xen_processor_performance compat_processor_performance

in scope while it's transforming between the two formats, meaning we
can't simply reuse pmstat.h with suitable defines.

Any clever ideas for how to untangle this mess, before I go for the
brute force approach of an #if COMPAT section in the main hypercall handler?

~Andrew

Re: x86: Declarations for compat_set_{cx,px}_pminfo()

[Nicola Vetrini]

[-- Attachment #1: Type: text/plain, Size: 1400 bytes --]

On 2024-11-18 22:41, Andrew Cooper wrote:
> Hello,
> 
> Eclair reports Rule 8.4 violations for these two.  They're real
> violations, but fixing them is proving challenging.
> 
> Because of how x86_64/platform_hypercall.c sets up some defines and
> includes ../platform_hypercall.c, the declarations of
> compat_set_{cx,px}_pminfo() of pmstat.h become compat ones.
> 
> Therefore the real hypercall handler does see a correct declaration.
> 
> However, the implementation in cpufreq.c does not get the compat-ified
> version of pmstat.h, so misses the declaration and thus the violation.
> 
> Worse however is the fact that cpufreq.c depends on not having the:
> 
> #define xen_processor_performance compat_processor_performance
> 
> in scope while it's transforming between the two formats, meaning we
> can't simply reuse pmstat.h with suitable defines.
> 
> Any clever ideas for how to untangle this mess, before I go for the
> brute force approach of an #if COMPAT section in the main hypercall 
> handler?
> 

Yeah, that is why these were among the last ones. I'm afraid I didn't 
come up with a reasonable to deal with this either.

You can find attached a tentative patch I made (not build tested, and 
also based on an older tree). A last resort may be to special-case those 
declarations, but that's undesirable.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: R8.4.patch --]
[-- Type: text/x-diff; name=R8.4.patch, Size: 1449 bytes --]

commit caffa89a06275b6b50ace99170b99b500b7b32f7
Author: Nicola Vetrini <nicola.vetrini@bugseng.com>
Date:   Wed Nov 29 12:23:25 2023 +0100

    x86_64/platform_hypercall: provide proper declarations for compat_*
    
    Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

diff --git a/xen/arch/x86/x86_64/platform_hypercall.c b/xen/arch/x86/x86_64/platform_hypercall.c
index 347f5d65330e..3ad15742d9ef 100644
--- a/xen/arch/x86/x86_64/platform_hypercall.c
+++ b/xen/arch/x86/x86_64/platform_hypercall.c
@@ -5,6 +5,7 @@
 EMIT_FILE;
 
 #include <xen/lib.h>
+#include <xen/pmstat.h>
 #include <xen/hypercall.h>
 
 #define xen_platform_op     compat_platform_op
@@ -14,11 +15,13 @@ EMIT_FILE;
 #define efi_get_info        efi_compat_get_info
 #define efi_runtime_call(x) efi_compat_runtime_call(x)
 
-#define xen_processor_performance compat_processor_performance
-#define set_px_pminfo       compat_set_px_pminfo
+#define xen_processor_performance compat_xen_processor_performance
+int compat_set_px_pminfo(uint32_t acpi_id, 
+                         struct compat_xen_processor_performance *perf);
 
-#define xen_processor_power compat_processor_power
-#define set_cx_pminfo       compat_set_cx_pminfo
+#define xen_processor_power       compat_xen_processor_power
+long compat_set_cx_pminfo(uint32_t acpi_id,
+                          struct compat_xen_processor_power *power);
 
 #define xen_pf_pcpuinfo xenpf_pcpuinfo
 CHECK_pf_pcpuinfo;

[PATCH] x86/pmstat: deal with Misra 8.4 violations

[Jan Beulich]

While the override #define-s in x86_64/platform_hypercall.c are good for
the consuming side of the compat variants of set_{cx,px}_pminfo(), the
producers lack the respective declarations. Include pmstat.h early,
before the overrides are put in place, while adding explicit
declarations of the compat functions (alongside structure forward
declarations).

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/x86_64/platform_hypercall.c
+++ b/xen/arch/x86/x86_64/platform_hypercall.c
@@ -4,8 +4,8 @@
 
 EMIT_FILE;
 
-#include <xen/lib.h>
 #include <xen/hypercall.h>
+#include <xen/pmstat.h>
 
 #define xen_platform_op     compat_platform_op
 #define xen_platform_op_t   compat_platform_op_t
--- a/xen/include/xen/pmstat.h
+++ b/xen/include/xen/pmstat.h
@@ -7,6 +7,12 @@
 
 int set_px_pminfo(uint32_t acpi_id, struct xen_processor_performance *perf);
 long set_cx_pminfo(uint32_t acpi_id, struct xen_processor_power *power);
+#ifdef CONFIG_COMPAT
+struct compat_processor_performance;
+int compat_set_px_pminfo(uint32_t acpi_id, struct compat_processor_performance *perf);
+struct compat_processor_power;
+long compat_set_cx_pminfo(uint32_t acpi_id, struct compat_processor_power *power);
+#endif
 uint32_t pmstat_get_cx_nr(unsigned int cpu);
 int pmstat_get_cx_stat(unsigned int cpu, struct pm_cx_stat *stat);
 int pmstat_reset_cx_stat(unsigned int cpu);

Re: [PATCH] x86/pmstat: deal with Misra 8.4 violations

[Andrew Cooper]

On 19/11/2024 8:12 am, Jan Beulich wrote:
> While the override #define-s in x86_64/platform_hypercall.c are good for
> the consuming side of the compat variants of set_{cx,px}_pminfo(), the
> producers lack the respective declarations. Include pmstat.h early,
> before the overrides are put in place, while adding explicit
> declarations of the compat functions (alongside structure forward
> declarations).
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/x86_64/platform_hypercall.c
> +++ b/xen/arch/x86/x86_64/platform_hypercall.c
> @@ -4,8 +4,8 @@
>  
>  EMIT_FILE;
>  
> -#include <xen/lib.h>
>  #include <xen/hypercall.h>
> +#include <xen/pmstat.h>
>  
>  #define xen_platform_op     compat_platform_op
>  #define xen_platform_op_t   compat_platform_op_t
> --- a/xen/include/xen/pmstat.h
> +++ b/xen/include/xen/pmstat.h
> @@ -7,6 +7,12 @@
>  
>  int set_px_pminfo(uint32_t acpi_id, struct xen_processor_performance *perf);
>  long set_cx_pminfo(uint32_t acpi_id, struct xen_processor_power *power);
> +#ifdef CONFIG_COMPAT
> +struct compat_processor_performance;
> +int compat_set_px_pminfo(uint32_t acpi_id, struct compat_processor_performance *perf);
> +struct compat_processor_power;
> +long compat_set_cx_pminfo(uint32_t acpi_id, struct compat_processor_power *power);
> +#endif
>  uint32_t pmstat_get_cx_nr(unsigned int cpu);
>  int pmstat_get_cx_stat(unsigned int cpu, struct pm_cx_stat *stat);
>  int pmstat_reset_cx_stat(unsigned int cpu);
>

Thanks for the patch.  I'll give it a spin along with the other 8.4 fixes.

~Andrew

Re: [PATCH] x86/pmstat: deal with Misra 8.4 violations

[Andrew Cooper]

On 19/11/2024 10:30 am, Andrew Cooper wrote:
> On 19/11/2024 8:12 am, Jan Beulich wrote:
>> While the override #define-s in x86_64/platform_hypercall.c are good for
>> the consuming side of the compat variants of set_{cx,px}_pminfo(), the
>> producers lack the respective declarations. Include pmstat.h early,
>> before the overrides are put in place, while adding explicit
>> declarations of the compat functions (alongside structure forward
>> declarations).
>>
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/x86_64/platform_hypercall.c
>> +++ b/xen/arch/x86/x86_64/platform_hypercall.c
>> @@ -4,8 +4,8 @@
>>  
>>  EMIT_FILE;
>>  
>> -#include <xen/lib.h>
>>  #include <xen/hypercall.h>
>> +#include <xen/pmstat.h>
>>  
>>  #define xen_platform_op     compat_platform_op
>>  #define xen_platform_op_t   compat_platform_op_t
>> --- a/xen/include/xen/pmstat.h
>> +++ b/xen/include/xen/pmstat.h
>> @@ -7,6 +7,12 @@
>>  
>>  int set_px_pminfo(uint32_t acpi_id, struct xen_processor_performance *perf);
>>  long set_cx_pminfo(uint32_t acpi_id, struct xen_processor_power *power);
>> +#ifdef CONFIG_COMPAT
>> +struct compat_processor_performance;
>> +int compat_set_px_pminfo(uint32_t acpi_id, struct compat_processor_performance *perf);
>> +struct compat_processor_power;
>> +long compat_set_cx_pminfo(uint32_t acpi_id, struct compat_processor_power *power);
>> +#endif
>>  uint32_t pmstat_get_cx_nr(unsigned int cpu);
>>  int pmstat_get_cx_stat(unsigned int cpu, struct pm_cx_stat *stat);
>>  int pmstat_reset_cx_stat(unsigned int cpu);
>>
> Thanks for the patch.  I'll give it a spin along with the other 8.4 fixes.

https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1550621091
pending to run, with all 8.4 fixes including a prototype fix for the
do_mca() issue too.

~Andrew

Re: [PATCH] x86/pmstat: deal with Misra 8.4 violations

[Andrew Cooper]

On 19/11/2024 10:55 am, Andrew Cooper wrote:
> On 19/11/2024 10:30 am, Andrew Cooper wrote:
>> On 19/11/2024 8:12 am, Jan Beulich wrote:
>>> While the override #define-s in x86_64/platform_hypercall.c are good for
>>> the consuming side of the compat variants of set_{cx,px}_pminfo(), the
>>> producers lack the respective declarations. Include pmstat.h early,
>>> before the overrides are put in place, while adding explicit
>>> declarations of the compat functions (alongside structure forward
>>> declarations).
>>>
>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/arch/x86/x86_64/platform_hypercall.c
>>> +++ b/xen/arch/x86/x86_64/platform_hypercall.c
>>> @@ -4,8 +4,8 @@
>>>  
>>>  EMIT_FILE;
>>>  
>>> -#include <xen/lib.h>
>>>  #include <xen/hypercall.h>
>>> +#include <xen/pmstat.h>
>>>  
>>>  #define xen_platform_op     compat_platform_op
>>>  #define xen_platform_op_t   compat_platform_op_t
>>> --- a/xen/include/xen/pmstat.h
>>> +++ b/xen/include/xen/pmstat.h
>>> @@ -7,6 +7,12 @@
>>>  
>>>  int set_px_pminfo(uint32_t acpi_id, struct xen_processor_performance *perf);
>>>  long set_cx_pminfo(uint32_t acpi_id, struct xen_processor_power *power);
>>> +#ifdef CONFIG_COMPAT
>>> +struct compat_processor_performance;
>>> +int compat_set_px_pminfo(uint32_t acpi_id, struct compat_processor_performance *perf);
>>> +struct compat_processor_power;
>>> +long compat_set_cx_pminfo(uint32_t acpi_id, struct compat_processor_power *power);
>>> +#endif
>>>  uint32_t pmstat_get_cx_nr(unsigned int cpu);
>>>  int pmstat_get_cx_stat(unsigned int cpu, struct pm_cx_stat *stat);
>>>  int pmstat_reset_cx_stat(unsigned int cpu);
>>>
>> Thanks for the patch.  I'll give it a spin along with the other 8.4 fixes.
> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1550621091
> pending to run, with all 8.4 fixes including a prototype fix for the
> do_mca() issue too.

Eclair was happy with the result, so:

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