Re: [XEN PATCH][for-4.19 v4 1/8] xen/include: add macro ISOLATE_LOW_BIT

[Jan Beulich]

On 30.10.2023 23:44, Stefano Stabellini wrote:
> On Mon, 30 Oct 2023, Jan Beulich wrote:
>> On 27.10.2023 15:34, Nicola Vetrini wrote:
>>> --- a/xen/include/xen/macros.h
>>> +++ b/xen/include/xen/macros.h
>>> @@ -8,8 +8,14 @@
>>>  #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
>>>  #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
>>>  
>>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
>>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
>>> +/*
>>> + * Given an unsigned integer argument, expands to a mask where just the 
>>> least
>>> + * significant nonzero bit of the argument is set, or 0 if no bits are set.
>>> + */
>>> +#define ISOLATE_LOW_BIT(x) ((x) & -(x))
>>
>> Not even considering future Misra changes (which aiui may require that
>> anyway), this generalization of the macro imo demands that its argument
>> now be evaluated only once.
> 
> Fur sure that would be an improvement, but I don't see a trivial way to
> do it and this issue is also present today before the patch.

This was an issue here for MASK_EXTR() and MASK_INSR(), yes, but the new
macro has wider use, and there was no issue elsewhere so far.

> I think it
> would be better to avoid scope-creeping this patch as we are already at
> v4 for something that was expected to be a trivial mechanical change. I
> would rather review the fix as a separate patch, maybe sent by you as
> you probably have a specific implementation in mind?

#define ISOLATE_LOW_BIT(x) ({ \
typeof(x) x_ = (x); \
x_ & -x_; \
})

Hard to see the scope creep here. What I would consider scope creep I
specifically didn't even ask for: I'd like this macro to be overridable
by an arch. Specifically (see my earlier naming hint) I'd like to use
x86's BMI insn BLSI in the context of "x86: allow Kconfig control over
psABI level", when ABI v2 or higher is in use.

Jan

Re: [XEN PATCH][for-4.19 v5 2/8] x86: add deviation for asm-only functions

[Jan Beulich]

On 31.10.2023 00:02, Stefano Stabellini wrote:
> On Mon, 30 Oct 2023, Jan Beulich wrote:
>> On 30.10.2023 10:11, Nicola Vetrini wrote:
>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> @@ -163,6 +163,15 @@ Therefore the absence of prior declarations is safe."
>>>  -config=MC3R1.R8.4,reports+={safe, "first_area(any_loc(file(gcov)))"}
>>>  -doc_end
>>>  
>>> +-doc_begin="Recognize the occurrence of current_stack_pointer as a 
>>> declaration."
>>> +-file_tag+={asm_defns, "^xen/arch/x86/include/asm/asm_defns\\.h$"}
>>> +-config=MC3R1.R8.4,declarations+={safe, 
>>> "loc(file(asm_defns))&&^current_stack_pointer$"}
>>> +-doc_end
>>> +
>>> +-doc_begin="asmlinkage is a marker to indicate that the function is only 
>>> used from asm modules."
>>> +-config=MC3R1.R8.4,declarations+={safe,"loc(text(^.*asmlinkage.*$, 
>>> -1..0))"}
>>> +-doc_end
>>
>> In the longer run asmlinkage will want using for functions used either way
>> between C and assembly (i.e. C->asm calls as well as asm->C ones). I'd like
>> to ask that the text please allow for that (e.g. s/from/to interface with/).
>>
>>> --- a/xen/arch/x86/hvm/svm/intr.c
>>> +++ b/xen/arch/x86/hvm/svm/intr.c
>>> @@ -123,7 +123,7 @@ static void svm_enable_intr_window(struct vcpu *v, 
>>> struct hvm_intack intack)
>>>  vmcb, general1_intercepts | GENERAL1_INTERCEPT_VINTR);
>>>  }
>>>  
>>> -void svm_intr_assist(void)
>>> +asmlinkage void svm_intr_assist(void)
>>
>> Nit (here and below): Attributes, unless impossible for some specific
>> reason, should always go between type and identifier. Not all our code
>> is conforming to that, but I think a majority is, and hence you should
>> be able to find ample examples (taking e.g. __init).
> 
> Hi Jan, in general I agree with this principle but I noticed that this
> is not the way Linux uses asmlinkage, a couple of examples:
> 
> asmlinkage void do_page_fault(struct pt_regs *regs);
> asmlinkage const sys_call_ptr_t sys_call_table[];
> 
> Should we go our way or follow Linux on this in terms of code style?

Linux isn't very consistent in attribute placement anyway, and doing it
"randomly" relies on the compiler guys never going to tighten what
attributes mean dependent on their placement (iirc gcc doc has text to
the effect of this possibly changing at any time). Aiui part of the
reason why parsing is more relaxed than it should be is that certain
attributes cannot be placed unambiguously at their nominally dedicated
place.

So my personal view on your question is that we should go our more
consistent way.

Jan

Re: [XEN PATCH][for-4.19 v5 2/8] x86: add deviation for asm-only functions

[Nicola Vetrini]

On 2023-10-30 23:54, Stefano Stabellini wrote:

On Mon, 30 Oct 2023, Julien Grall wrote:

Hi Nicola,

On 30/10/2023 09:11, Nicola Vetrini wrote:
> As stated in rules.rst, functions used only in asm modules
> are allowed to have no prior declaration visible when being
> defined, hence these functions are marked with an
> 'asmlinkage' macro, which is then deviated for MISRA C:2012
> Rule 8.4.

AFAIU, this is a replacement of SAF-1. If so, I would like a 
consistent way to
address Rule 8.4. So can you write a patch to replace all the use of 
SAF-1

with asmlinkage and remove SAF-1?


+1


Ok, no problem

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

Re: [XEN PATCH][for-4.19 v5 2/8] x86: add deviation for asm-only functions

[Nicola Vetrini]

On 2023-10-31 08:50, Jan Beulich wrote:

On 31.10.2023 00:02, Stefano Stabellini wrote:

On Mon, 30 Oct 2023, Jan Beulich wrote:

On 30.10.2023 10:11, Nicola Vetrini wrote:

--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -163,6 +163,15 @@ Therefore the absence of prior declarations is 
safe."
 -config=MC3R1.R8.4,reports+={safe, 
"first_area(any_loc(file(gcov)))"}

 -doc_end

+-doc_begin="Recognize the occurrence of current_stack_pointer as a 
declaration."

+-file_tag+={asm_defns, "^xen/arch/x86/include/asm/asm_defns\\.h$"}
+-config=MC3R1.R8.4,declarations+={safe, 
"loc(file(asm_defns))&&^current_stack_pointer$"}

+-doc_end
+
+-doc_begin="asmlinkage is a marker to indicate that the function is 
only used from asm modules."
+-config=MC3R1.R8.4,declarations+={safe,"loc(text(^.*asmlinkage.*$, 
-1..0))"}

+-doc_end


In the longer run asmlinkage will want using for functions used 
either way
between C and assembly (i.e. C->asm calls as well as asm->C ones). 
I'd like
to ask that the text please allow for that (e.g. s/from/to interface 
with/).




Will do


--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -123,7 +123,7 @@ static void svm_enable_intr_window(struct vcpu 
*v, struct hvm_intack intack)

 vmcb, general1_intercepts | GENERAL1_INTERCEPT_VINTR);
 }

-void svm_intr_assist(void)
+asmlinkage void svm_intr_assist(void)


Nit (here and below): Attributes, unless impossible for some specific
reason, should always go between type and identifier. Not all our 
code
is conforming to that, but I think a majority is, and hence you 
should

be able to find ample examples (taking e.g. __init).


Hi Jan, in general I agree with this principle but I noticed that this
is not the way Linux uses asmlinkage, a couple of examples:

asmlinkage void do_page_fault(struct pt_regs *regs);
asmlinkage const sys_call_ptr_t sys_call_table[];

Should we go our way or follow Linux on this in terms of code style?


Linux isn't very consistent in attribute placement anyway, and doing it
"randomly" relies on the compiler guys never going to tighten what
attributes mean dependent on their placement (iirc gcc doc has text to
the effect of this possibly changing at any time). Aiui part of the
reason why parsing is more relaxed than it should be is that certain
attributes cannot be placed unambiguously at their nominally dedicated
place.

So my personal view on your question is that we should go our more
consistent way.

Jan


Ok.

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

Re: [XEN PATCH][for-4.19 v5 2/8] x86: add deviation for asm-only functions

[Nicola Vetrini]

On 2023-10-30 16:12, Jan Beulich wrote:

On 30.10.2023 10:11, Nicola Vetrini wrote:

--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -163,6 +163,15 @@ Therefore the absence of prior declarations is 
safe."

 -config=MC3R1.R8.4,reports+={safe, "first_area(any_loc(file(gcov)))"}
 -doc_end

+-doc_begin="Recognize the occurrence of current_stack_pointer as a 
declaration."

+-file_tag+={asm_defns, "^xen/arch/x86/include/asm/asm_defns\\.h$"}
+-config=MC3R1.R8.4,declarations+={safe, 
"loc(file(asm_defns))&&^current_stack_pointer$"}

+-doc_end
+
+-doc_begin="asmlinkage is a marker to indicate that the function is 
only used from asm modules."
+-config=MC3R1.R8.4,declarations+={safe,"loc(text(^.*asmlinkage.*$, 
-1..0))"}

+-doc_end


In the longer run asmlinkage will want using for functions used either 
way
between C and assembly (i.e. C->asm calls as well as asm->C ones). I'd 
like
to ask that the text please allow for that (e.g. s/from/to interface 
with/).



--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -123,7 +123,7 @@ static void svm_enable_intr_window(struct vcpu *v, 
struct hvm_intack intack)

 vmcb, general1_intercepts | GENERAL1_INTERCEPT_VINTR);
 }

-void svm_intr_assist(void)
+asmlinkage void svm_intr_assist(void)


Nit (here and below): Attributes, unless impossible for some specific
reason, should always go between type and identifier. Not all our code
is conforming to that, but I think a majority is, and hence you should
be able to find ample examples (taking e.g. __init).


--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -159,6 +159,9 @@
 # define ASM_FLAG_OUT(yes, no) no
 #endif

+/* Mark a function or variable as used only from asm */
+#define asmlinkage


I appreciate this being an immediately "natural" place, but considering
what we know from Linux I think we ought to allow for arch overrides 
here

right away. For that I'm afraid compiler.h isn't best; it may still be
okay as long as at least an #ifndef is put around it. Imo, however, 
this

ought to go into xen/linkage.h, as is being introduced by "common:
assembly entry point type/size annotations". It's somewhat a shame that
this and the rest of that series has missed 4.18 ...

Jan


An #ifndef around what, exactly? Anyway, making (part of) this series 
wait for approval
until the other has been accepted into 4.19 (for which I have no 
specific timeframe)

does not seem that desirable to me.

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

Re: [XEN PATCH][for-4.19 v5 2/8] x86: add deviation for asm-only functions

[Jan Beulich]

On 31.10.2023 09:22, Nicola Vetrini wrote:
> On 2023-10-30 16:12, Jan Beulich wrote:
>> On 30.10.2023 10:11, Nicola Vetrini wrote:
>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> @@ -163,6 +163,15 @@ Therefore the absence of prior declarations is 
>>> safe."
>>>  -config=MC3R1.R8.4,reports+={safe, "first_area(any_loc(file(gcov)))"}
>>>  -doc_end
>>>
>>> +-doc_begin="Recognize the occurrence of current_stack_pointer as a 
>>> declaration."
>>> +-file_tag+={asm_defns, "^xen/arch/x86/include/asm/asm_defns\\.h$"}
>>> +-config=MC3R1.R8.4,declarations+={safe, 
>>> "loc(file(asm_defns))&&^current_stack_pointer$"}
>>> +-doc_end
>>> +
>>> +-doc_begin="asmlinkage is a marker to indicate that the function is 
>>> only used from asm modules."
>>> +-config=MC3R1.R8.4,declarations+={safe,"loc(text(^.*asmlinkage.*$, 
>>> -1..0))"}
>>> +-doc_end
>>
>> In the longer run asmlinkage will want using for functions used either 
>> way
>> between C and assembly (i.e. C->asm calls as well as asm->C ones). I'd 
>> like
>> to ask that the text please allow for that (e.g. s/from/to interface 
>> with/).
>>
>>> --- a/xen/arch/x86/hvm/svm/intr.c
>>> +++ b/xen/arch/x86/hvm/svm/intr.c
>>> @@ -123,7 +123,7 @@ static void svm_enable_intr_window(struct vcpu *v, 
>>> struct hvm_intack intack)
>>>  vmcb, general1_intercepts | GENERAL1_INTERCEPT_VINTR);
>>>  }
>>>
>>> -void svm_intr_assist(void)
>>> +asmlinkage void svm_intr_assist(void)
>>
>> Nit (here and below): Attributes, unless impossible for some specific
>> reason, should always go between type and identifier. Not all our code
>> is conforming to that, but I think a majority is, and hence you should
>> be able to find ample examples (taking e.g. __init).
>>
>>> --- a/xen/include/xen/compiler.h
>>> +++ b/xen/include/xen/compiler.h
>>> @@ -159,6 +159,9 @@
>>>  # define ASM_FLAG_OUT(yes, no) no
>>>  #endif
>>>
>>> +/* Mark a function or variable as used only from asm */
>>> +#define asmlinkage
>>
>> I appreciate this being an immediately "natural" place, but considering
>> what we know from Linux I think we ought to allow for arch overrides 
>> here
>> right away. For that I'm afraid compiler.h isn't best; it may still be
>> okay as long as at least an #ifndef is put around it. Imo, however, 
>> this
>> ought to go into xen/linkage.h, as is being introduced by "common:
>> assembly entry point type/size annotations". It's somewhat a shame that
>> this and the rest of that series has missed 4.18 ...
> 
> An #ifndef around what, exactly?

The #define. That way at least an arch's config.h can override it.

> Anyway, making (part of) this series 
> wait for approval
> until the other has been accepted into 4.19 (for which I have no 
> specific timeframe)
> does not seem that desirable to me.

It's not ideal, but you or anyone else are free to help that other work
make progress.

Jan

Re: [XEN PATCH][for-4.19 v4 1/8] xen/include: add macro ISOLATE_LOW_BIT

[Nicola Vetrini]

On 2023-10-31 08:43, Jan Beulich wrote:

On 30.10.2023 23:44, Stefano Stabellini wrote:

On Mon, 30 Oct 2023, Jan Beulich wrote:

On 27.10.2023 15:34, Nicola Vetrini wrote:

--- a/xen/include/xen/macros.h
+++ b/xen/include/xen/macros.h
@@ -8,8 +8,14 @@
 #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
 #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))

-#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
-#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
+/*
+ * Given an unsigned integer argument, expands to a mask where just 
the least
+ * significant nonzero bit of the argument is set, or 0 if no bits 
are set.

+ */
+#define ISOLATE_LOW_BIT(x) ((x) & -(x))


Not even considering future Misra changes (which aiui may require 
that
anyway), this generalization of the macro imo demands that its 
argument

now be evaluated only once.


Fur sure that would be an improvement, but I don't see a trivial way 
to

do it and this issue is also present today before the patch.


This was an issue here for MASK_EXTR() and MASK_INSR(), yes, but the 
new

macro has wider use, and there was no issue elsewhere so far.


I think it
would be better to avoid scope-creeping this patch as we are already 
at
v4 for something that was expected to be a trivial mechanical change. 
I

would rather review the fix as a separate patch, maybe sent by you as
you probably have a specific implementation in mind?


#define ISOLATE_LOW_BIT(x) ({ \
typeof(x) x_ = (x); \
x_ & -x_; \
})

Hard to see the scope creep here. What I would consider scope creep I
specifically didn't even ask for: I'd like this macro to be overridable
by an arch. Specifically (see my earlier naming hint) I'd like to use
x86's BMI insn BLSI in the context of "x86: allow Kconfig control over
psABI level", when ABI v2 or higher is in use.

Jan


I appreciate you suggesting an implementation; I'll send a v5 
incorporating it.


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

[XEN PATCH v2] xen/set_{c,p}x_pminfo: address violations od MISRA C:2012 Rule 8.3

[Federico Serafini]

Make function definitions and declarations consistent.
No functional change.

Signed-off-by: Federico Serafini 
---
Changes in v2:
- removed unwanted changes to cpu_callback();
- improved code style.
---
 xen/arch/x86/x86_64/cpu_idle.c |  5 +--
 xen/arch/x86/x86_64/cpufreq.c  |  6 ++--
 xen/drivers/cpufreq/cpufreq.c  | 58 --
 xen/include/xen/pmstat.h   |  4 +--
 4 files changed, 36 insertions(+), 37 deletions(-)

diff --git a/xen/arch/x86/x86_64/cpu_idle.c b/xen/arch/x86/x86_64/cpu_idle.c
index e2195d57be..fcd6fc0fc2 100644
--- a/xen/arch/x86/x86_64/cpu_idle.c
+++ b/xen/arch/x86/x86_64/cpu_idle.c
@@ -62,7 +62,8 @@ static int copy_from_compat_state(xen_processor_cx_t 
*xen_state,
 return 0;
 }
 
-long compat_set_cx_pminfo(uint32_t cpu, struct compat_processor_power *power)
+long compat_set_cx_pminfo(uint32_t acpi_id,
+  struct compat_processor_power *power)
 {
 struct xen_processor_power *xen_power;
 unsigned long xlat_page_current;
@@ -106,5 +107,5 @@ long compat_set_cx_pminfo(uint32_t cpu, struct 
compat_processor_power *power)
 XLAT_processor_power(xen_power, power);
 #undef XLAT_processor_power_HNDL_states
 
-return set_cx_pminfo(cpu, xen_power);
+return set_cx_pminfo(acpi_id, xen_power);
 }
diff --git a/xen/arch/x86/x86_64/cpufreq.c b/xen/arch/x86/x86_64/cpufreq.c
index 9e1e2050da..e4f3d5b436 100644
--- a/xen/arch/x86/x86_64/cpufreq.c
+++ b/xen/arch/x86/x86_64/cpufreq.c
@@ -30,8 +30,8 @@ CHECK_processor_px;
 
 DEFINE_XEN_GUEST_HANDLE(compat_processor_px_t);
 
-int 
-compat_set_px_pminfo(uint32_t cpu, struct compat_processor_performance *perf)
+int compat_set_px_pminfo(uint32_t acpi_id,
+ struct compat_processor_performance *perf)
 {
 struct xen_processor_performance *xen_perf;
 unsigned long xlat_page_current;
@@ -52,5 +52,5 @@ compat_set_px_pminfo(uint32_t cpu, struct 
compat_processor_performance *perf)
 XLAT_processor_performance(xen_perf, perf);
 #undef XLAT_processor_performance_HNDL_states
 
-return set_px_pminfo(cpu, xen_perf);
+return set_px_pminfo(acpi_id, xen_perf);
 }
diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index 6e5c400849..f5c714611e 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -457,14 +457,14 @@ static void print_PPC(unsigned int platform_limit)
 printk("\t_PPC: %d\n", platform_limit);
 }
 
-int set_px_pminfo(uint32_t acpi_id, struct xen_processor_performance 
*dom0_px_info)
+int set_px_pminfo(uint32_t acpi_id, struct xen_processor_performance *perf)
 {
 int ret=0, cpuid;
 struct processor_pminfo *pmpt;
 struct processor_performance *pxpt;
 
 cpuid = get_cpu_id(acpi_id);
-if ( cpuid < 0 || !dom0_px_info)
+if ( cpuid < 0 || !perf )
 {
 ret = -EINVAL;
 goto out;
@@ -488,22 +488,22 @@ int set_px_pminfo(uint32_t acpi_id, struct 
xen_processor_performance *dom0_px_in
 pmpt->acpi_id = acpi_id;
 pmpt->id = cpuid;
 
-if ( dom0_px_info->flags & XEN_PX_PCT )
+if ( perf->flags & XEN_PX_PCT )
 {
 /* space_id check */
-if (dom0_px_info->control_register.space_id != 
-dom0_px_info->status_register.space_id)
+if ( perf->control_register.space_id !=
+ perf->status_register.space_id )
 {
 ret = -EINVAL;
 goto out;
 }
 
-memcpy ((void *)&pxpt->control_register,
-(void *)&dom0_px_info->control_register,
-sizeof(struct xen_pct_register));
-memcpy ((void *)&pxpt->status_register,
-(void *)&dom0_px_info->status_register,
-sizeof(struct xen_pct_register));
+memcpy((void *)&pxpt->control_register,
+   (void *)&perf->control_register,
+   sizeof(struct xen_pct_register));
+memcpy((void *)&pxpt->status_register,
+   (void *)&perf->status_register,
+   sizeof(struct xen_pct_register));
 
 if ( cpufreq_verbose )
 {
@@ -512,69 +512,67 @@ int set_px_pminfo(uint32_t acpi_id, struct 
xen_processor_performance *dom0_px_in
 }
 }
 
-if ( dom0_px_info->flags & XEN_PX_PSS ) 
+if ( perf->flags & XEN_PX_PSS )
 {
 /* capability check */
-if (dom0_px_info->state_count <= 1)
+if ( perf->state_count <= 1 )
 {
 ret = -EINVAL;
 goto out;
 }
 
 if ( !(pxpt->states = xmalloc_array(struct xen_processor_px,
-dom0_px_info->state_count)) )
+perf->state_count)) )
 {
 ret = -ENOMEM;
 goto out;
 }
-if ( copy_from_guest(pxpt->states, dom0_px_info->states,
- dom0_px_info->state_count) )
+if ( copy_from_guest(pxpt->states, perf->states, perf->state_count) )
 {
 ret = -EFAULT;

Re: [XEN PATCH v2] xen/set_{c,p}x_pminfo: address violations od MISRA C:2012 Rule 8.3

[Jan Beulich]

On 31.10.2023 09:33, Federico Serafini wrote:
> Make function definitions and declarations consistent.
> No functional change.
> 
> Signed-off-by: Federico Serafini 

Reviewed-by: Jan Beulich 

> @@ -488,22 +488,22 @@ int set_px_pminfo(uint32_t acpi_id, struct 
> xen_processor_performance *dom0_px_in
>  pmpt->acpi_id = acpi_id;
>  pmpt->id = cpuid;
>  
> -if ( dom0_px_info->flags & XEN_PX_PCT )
> +if ( perf->flags & XEN_PX_PCT )
>  {
>  /* space_id check */
> -if (dom0_px_info->control_register.space_id != 
> -dom0_px_info->status_register.space_id)
> +if ( perf->control_register.space_id !=
> + perf->status_register.space_id )
>  {
>  ret = -EINVAL;
>  goto out;
>  }
>  
> -memcpy ((void *)&pxpt->control_register,
> -(void *)&dom0_px_info->control_register,
> -sizeof(struct xen_pct_register));
> -memcpy ((void *)&pxpt->status_register,
> -(void *)&dom0_px_info->status_register,
> -sizeof(struct xen_pct_register));
> +memcpy((void *)&pxpt->control_register,
> +   (void *)&perf->control_register,
> +   sizeof(struct xen_pct_register));
> +memcpy((void *)&pxpt->status_register,
> +   (void *)&perf->status_register,
> +   sizeof(struct xen_pct_register));

Along with the other coding style changes it might have been nice to
also drop the bogus casts here and ...

> @@ -512,69 +512,67 @@ int set_px_pminfo(uint32_t acpi_id, struct 
> xen_processor_performance *dom0_px_in
>  }
>  }
>  
> -if ( dom0_px_info->flags & XEN_PX_PSS ) 
> +if ( perf->flags & XEN_PX_PSS )
>  {
>  /* capability check */
> -if (dom0_px_info->state_count <= 1)
> +if ( perf->state_count <= 1 )
>  {
>  ret = -EINVAL;
>  goto out;
>  }
>  
>  if ( !(pxpt->states = xmalloc_array(struct xen_processor_px,
> -dom0_px_info->state_count)) )
> +perf->state_count)) )
>  {
>  ret = -ENOMEM;
>  goto out;
>  }
> -if ( copy_from_guest(pxpt->states, dom0_px_info->states,
> - dom0_px_info->state_count) )
> +if ( copy_from_guest(pxpt->states, perf->states, perf->state_count) )
>  {
>  ret = -EFAULT;
>  goto out;
>  }
> -pxpt->state_count = dom0_px_info->state_count;
> +pxpt->state_count = perf->state_count;
>  
>  if ( cpufreq_verbose )
>  print_PSS(pxpt->states,pxpt->state_count);
>  }
>  
> -if ( dom0_px_info->flags & XEN_PX_PSD )
> +if ( perf->flags & XEN_PX_PSD )
>  {
>  /* check domain coordination */
> -if (dom0_px_info->shared_type != CPUFREQ_SHARED_TYPE_ALL &&
> -dom0_px_info->shared_type != CPUFREQ_SHARED_TYPE_ANY &&
> -dom0_px_info->shared_type != CPUFREQ_SHARED_TYPE_HW)
> +if ( perf->shared_type != CPUFREQ_SHARED_TYPE_ALL &&
> + perf->shared_type != CPUFREQ_SHARED_TYPE_ANY &&
> + perf->shared_type != CPUFREQ_SHARED_TYPE_HW )
>  {
>  ret = -EINVAL;
>  goto out;
>  }
>  
> -pxpt->shared_type = dom0_px_info->shared_type;
> -memcpy ((void *)&pxpt->domain_info,
> -(void *)&dom0_px_info->domain_info,
> -sizeof(struct xen_psd_package));
> +pxpt->shared_type = perf->shared_type;
> +memcpy((void *)&pxpt->domain_info,
> +   (void *)&perf->domain_info,
> +   sizeof(struct xen_psd_package));

... here. If I end up committing this, I may take the liberty to do so.

Jan

Re: [XEN PATCH][for-4.19 v4 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Nicola Vetrini]

On 2023-10-27 22:50, Stefano Stabellini wrote:

On Fri, 27 Oct 2023, Nicola Vetrini wrote:

The definition of MC_NCLASSES contained a violation of MISRA C:2012
Rule 10.1, therefore by moving it as an enumeration constant resolves 
the
violation and makes it more resilient to possible additions to that 
enum.


Signed-off-by: Nicola Vetrini 


Reviewed-by: Stefano Stabellini 


I appreciate your review here, but I forgot to put Andrew's A-by;
he already included this patch in its own for-next tree.

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

Re: [PATCH] x86/PVH: allow Dom0 ELF parsing to be verbose

[Roger Pau Monné]

On Mon, Oct 30, 2023 at 02:14:44PM +0100, Jan Beulich wrote:
> VERBOSE had ceased to exist already before the introduction of this ELF
> parsing code.
> 
> Fixes: 62ba982424cb ("x86: parse Dom0 kernel for PVHv2")
> Signed-off-by: Jan Beulich 

Acked-by: Roger Pau Monné 

Some of the printed information might not be relevant for PVH mode,
like the 'ELF: addresses:' virt_* fields.

Note also that in 62ba982424cb opt_dom0_verbose wasn't available yet,
that option got introduced a couple of years later by 525ef6584f852.

Maybe it's 679216943f545 that should have also switched the
elf_set_verbose() in the PVH dom0 builder to use opt_dom0_verbose, at
the same time that the PV one was switched?

Thanks, Roger.

[XEN PATCH v2] xen/domain_page: address violations of MISRA C:2012 Rule 8.3

[Federico Serafini]

Make function defintions and declarations consistent.
No functional change.

Signed-off-by: Federico Serafini 
---
Changes in v2:
- use 'ptr' do denote a const void * parameter.
---
 xen/arch/arm/domain_page.c| 10 +-
 xen/include/xen/domain_page.h | 12 ++--
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/domain_page.c b/xen/arch/arm/domain_page.c
index b7c02c9190..3a43601623 100644
--- a/xen/arch/arm/domain_page.c
+++ b/xen/arch/arm/domain_page.c
@@ -74,9 +74,9 @@ void *map_domain_page_global(mfn_t mfn)
 return vmap(&mfn, 1);
 }
 
-void unmap_domain_page_global(const void *va)
+void unmap_domain_page_global(const void *ptr)
 {
-vunmap(va);
+vunmap(ptr);
 }
 
 /* Map a page of domheap memory */
@@ -149,13 +149,13 @@ void *map_domain_page(mfn_t mfn)
 }
 
 /* Release a mapping taken with map_domain_page() */
-void unmap_domain_page(const void *va)
+void unmap_domain_page(const void *ptr)
 {
 unsigned long flags;
 lpae_t *map = this_cpu(xen_dommap);
-int slot = ((unsigned long) va - DOMHEAP_VIRT_START) >> SECOND_SHIFT;
+int slot = ((unsigned long)ptr - DOMHEAP_VIRT_START) >> SECOND_SHIFT;
 
-if ( !va )
+if ( !ptr )
 return;
 
 local_irq_save(flags);
diff --git a/xen/include/xen/domain_page.h b/xen/include/xen/domain_page.h
index 0ff5cdd294..e1dd24ae58 100644
--- a/xen/include/xen/domain_page.h
+++ b/xen/include/xen/domain_page.h
@@ -29,12 +29,12 @@ void *map_domain_page(mfn_t mfn);
  * Pass a VA within a page previously mapped in the context of the
  * currently-executing VCPU via a call to map_domain_page().
  */
-void unmap_domain_page(const void *va);
+void unmap_domain_page(const void *ptr);
 
-/* 
+/*
  * Given a VA from map_domain_page(), return its underlying MFN.
  */
-mfn_t domain_page_map_to_mfn(const void *va);
+mfn_t domain_page_map_to_mfn(const void *ptr);
 
 /*
  * Similar to the above calls, except the mapping is accessible in all
@@ -42,7 +42,7 @@ mfn_t domain_page_map_to_mfn(const void *va);
  * mappings can also be unmapped from any context.
  */
 void *map_domain_page_global(mfn_t mfn);
-void unmap_domain_page_global(const void *va);
+void unmap_domain_page_global(const void *ptr);
 
 #define __map_domain_page(pg)map_domain_page(page_to_mfn(pg))
 
@@ -55,8 +55,8 @@ static inline void *__map_domain_page_global(const struct 
page_info *pg)
 
 #define map_domain_page(mfn)__mfn_to_virt(mfn_x(mfn))
 #define __map_domain_page(pg)   page_to_virt(pg)
-#define unmap_domain_page(va)   ((void)(va))
-#define domain_page_map_to_mfn(va)  _mfn(__virt_to_mfn((unsigned 
long)(va)))
+#define unmap_domain_page(ptr)   ((void)(ptr))
+#define domain_page_map_to_mfn(ptr)  _mfn(__virt_to_mfn((unsigned 
long)(ptr)))
 
 static inline void *map_domain_page_global(mfn_t mfn)
 {
-- 
2.34.1

Re: [RFC PATCH 03/22] x86/msr: always allow a pinned Dom0 to read any unknown MSR

[Edwin Torok]

> On 30 Oct 2023, at 16:29, Jan Beulich  wrote:
> 
> On 25.10.2023 21:29, Edwin Török wrote:
>> This can be useful if you realize you have to inspect the value of an
>> MSR in production, without having to change into a new Xen first that
>> handles the MSR.
> 
> Yet on a non-pinned Dom0 you'd still be lost. Since iirc we generally
> advise against pinning,

You can temporarily pin while debugging the issue (e.g. pin just 1 CPU from 
Dom0, and "walk" all your physical CPUs with it if you have to,
so that you query them all), e.g. with 'xl vcpu-pin'.
Although that is more invasive than reading a value.
 
Or alternatively have another (privileged) interface to read the MSR for a 
given core without exposing it to any guests, that way you don't affect the 
running system at all
(which would be preferable in a production environment), i.e. a Xen equivalent 
of 'rdmsr'.

> I wonder of how much use such a change would be,
> when it effectively undoes what we deliberately did a while ago.
> 
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -1933,6 +1933,9 @@ static int cf_check svm_msr_read_intercept(
>> break;
>> 
>> default:
>> +if ( is_hwdom_pinned_vcpu(v) && !rdmsr_safe(msr, *msr_content) )
>> +break;
>> +
>> if ( d->arch.msr_relaxed && !rdmsr_safe(msr, tmp) )
>> {
>> *msr_content = 0;
> 
> If we went as far as undoing some of what was done, I'd then wonder
> whether instead we should mandate relaxed mode to be enabled on such a
> Dom0. Then, instead of returning fake 0 here, the actual value could
> be returned in the specific case of (pinned?) Dom0.


Can relaxed mode be enabled at runtime? I'd be happy with either solution, but 
it should be something that can be enabled at runtime
(if you have to reboot Xen then you may lose the bug repro that you want to 
gather more information on).
Although changing such a setting in a production environment may still be 
risky, because the guest will then become very confused that it has previously 
read some 0s, now there are some real values, and later when you flip the 
switch off it gets 0s again.

Best regards,
--Edwin

Re: [RFC PATCH 02/22] x86/msr: implement MSR_SMI_COUNT for Dom0 on Intel

[Edwin Torok]

> On 30 Oct 2023, at 16:20, Jan Beulich  wrote:
> 
> On 25.10.2023 21:29, Edwin Török wrote:
>> Dom0 should always be able to read this MSR: it is useful when
>> investigating performance issues in production.
> 
> While I'm not outright opposed, I'm also not convinced. At the very least
> ...
> 
>> Although the count is Thread scoped, in practice all cores were observed
>> to return the same count (perhaps due to implementation details of SMM),
>> so do not require the cpu to be pinned in order to read it.
> 
> ... this, even if matching your observations, is going to be properly
> misleading in case counts end up diverging.
> 
>> This MSR exists on Intel since Nehalem.
>> 
>> Backport: 4.15+
> 
> If this was a backporting candidate, I think a Fixes: tag would need
> to indicate what's being fixed here.


I used the Backport tag to indicate what is the oldest release that it is 
backportable to.
IIRC the choices were:
* 4.0+ for issues that were present for a long time (didn't look further back 
than that in history), so there isn't any particular commit that introduces the 
problem, it was like that from the very beginning, i.e. not a regression.

* 4.13+ for issues affecting only new CPU support (I think that is the release 
that added Icelake support). I can attempt to find the commit that added 
Icelake support and mark them as Fixes: that commit (although there might be 
several of them)

* 4.15+ for bugs introduced by the default read-write msr changes


> Otherwise this is merely a new
> feature.
> 

Prior to the default rdwrmsr changes it was possible to read any MSR, so I 
consider it a bug that after the default rdwrmsr changes you can no longer do 
that, it takes away a valuable debugging tool.
I can attempt to find a more specific commit to indicate with Fixes:

>> --- a/xen/arch/x86/include/asm/msr-index.h
>> +++ b/xen/arch/x86/include/asm/msr-index.h
>> @@ -641,6 +641,9 @@
>> #define MSR_NHL_LBR_SELECT 0x01c8
>> #define MSR_NHL_LASTBRANCH_TOS 0x01c9
>> 
>> +/* Nehalem and newer other MSRs */
>> +#define MSR_SMI_COUNT   0x0034
> 
> See my comment on the other patch regarding additions here.
> 
>> --- a/xen/arch/x86/msr.c
>> +++ b/xen/arch/x86/msr.c
>> @@ -139,6 +139,13 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t 
>> *val)
>> *val = msrs->misc_features_enables.raw;
>> break;
>> 
>> +case MSR_SMI_COUNT:
>> +if ( cp->x86_vendor != X86_VENDOR_INTEL )
>> +goto gp_fault;
>> +if ( is_hardware_domain(d) && !rdmsr_safe(msr, *val) )
>> +break;
>> +return X86EMUL_UNHANDLEABLE;
> 
> Why #GP for non-Intel but UNHANDLEABLE for DomU?

I wanted to match the behaviour of the 'default:' case statement, although 
looking at the other MSR handling code in this file they just usually gp_fault,
with the exception of the MSR_K8* code that returns UNHANDLEABLE, so if 
condition here could be simplified (e.g. follow how MSR_AMD_PATCHLEVEL does it).

Best regards,
--Edwin 

> 
> Jan

[xen-4.18-testing test] 183624: tolerable FAIL - PUSHED

[osstest service owner]

flight 183624 xen-4.18-testing real [real]
flight 183634 xen-4.18-testing real-retest [real]
http://logs.test-lab.xenproject.org/osstest/logs/183624/
http://logs.test-lab.xenproject.org/osstest/logs/183634/

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-i386-qemuu-rhel6hvm-amd  7 xen-install   fail pass in 183634-retest

Tests which did not succeed, but are not blocking:
 test-amd64-i386-pair 11 xen-install/dst_host fail  like 183555
 test-amd64-i386-xl-qemuu-ovmf-amd64  7 xen-installfail like 183555
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 183555
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 183555
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 183555
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 183555
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 183555
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 183555
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 183555
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 183555
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 183555
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 183555
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 183555
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 183555
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  15 migrate-support-checkfail   never pass
 test-amd64-i386-xl-pvshim14 guest-start  fail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-raw  14 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass

version targeted for testing:
 xen  b6cf4f81b5

Re: [PATCH] x86/PVH: allow Dom0 ELF parsing to be verbose

[Jan Beulich]

On 31.10.2023 10:08, Roger Pau Monné wrote:
> On Mon, Oct 30, 2023 at 02:14:44PM +0100, Jan Beulich wrote:
>> VERBOSE had ceased to exist already before the introduction of this ELF
>> parsing code.
>>
>> Fixes: 62ba982424cb ("x86: parse Dom0 kernel for PVHv2")
>> Signed-off-by: Jan Beulich 
> 
> Acked-by: Roger Pau Monné 

Thanks.

> Some of the printed information might not be relevant for PVH mode,
> like the 'ELF: addresses:' virt_* fields.

We may want to conditionalize some of that, but since you mention virt_*:
Just ahead of the printing we forcefully set e.g. virt_entry when hvm is
true. virt_base, otoh, is pretty clearly useless to log, for being set to
zero unconditionally when hvm is true.

In any event, what I was missing was the output from elf_xen_parse_note().
I had mistakenly tried to boot a PV-only kernel as PVH, but it didn't
occur to me to check for a typo in the kernel specification in the grub
entry. It was only the sequence of ELF notes that finally directed me
towards checking that ...

> Note also that in 62ba982424cb opt_dom0_verbose wasn't available yet,
> that option got introduced a couple of years later by 525ef6584f852.

Right; I've set the Fixes: tag solely from the wrong use of VERBOSE.

> Maybe it's 679216943f545 that should have also switched the
> elf_set_verbose() in the PVH dom0 builder to use opt_dom0_verbose, at
> the same time that the PV one was switched?

Perhaps that would have been a good point in time, but here we are.

Jan

Re: [RFC PATCH 02/22] x86/msr: implement MSR_SMI_COUNT for Dom0 on Intel

[Jan Beulich]

On 31.10.2023 10:42, Edwin Torok wrote:
>> On 30 Oct 2023, at 16:20, Jan Beulich  wrote:
>> On 25.10.2023 21:29, Edwin Török wrote:
>>> Dom0 should always be able to read this MSR: it is useful when
>>> investigating performance issues in production.
>>
>> While I'm not outright opposed, I'm also not convinced. At the very least
>> ...
>>
>>> Although the count is Thread scoped, in practice all cores were observed
>>> to return the same count (perhaps due to implementation details of SMM),
>>> so do not require the cpu to be pinned in order to read it.
>>
>> ... this, even if matching your observations, is going to be properly
>> misleading in case counts end up diverging.
>>
>>> This MSR exists on Intel since Nehalem.
>>>
>>> Backport: 4.15+
>>
>> If this was a backporting candidate, I think a Fixes: tag would need
>> to indicate what's being fixed here.
> 
> 
> I used the Backport tag to indicate what is the oldest release that it is 
> backportable to.
> IIRC the choices were:
> * 4.0+ for issues that were present for a long time (didn't look further back 
> than that in history), so there isn't any particular commit that introduces 
> the problem, it was like that from the very beginning, i.e. not a regression.
> 
> * 4.13+ for issues affecting only new CPU support (I think that is the 
> release that added Icelake support). I can attempt to find the commit that 
> added Icelake support and mark them as Fixes: that commit (although there 
> might be several of them)
> 
> * 4.15+ for bugs introduced by the default read-write msr changes
> 
> 
>> Otherwise this is merely a new
>> feature.
>>
> 
> Prior to the default rdwrmsr changes it was possible to read any MSR, so I 
> consider it a bug that after the default rdwrmsr changes you can no longer do 
> that, it takes away a valuable debugging tool.

As said elsewhere, making MSRs generally inaccessible was a deliberate change.
I don't think any of us x86 maintainers has so far considered that as 
introducing
a bug. MSRs being accessible as a debugging tool may be worthwhile to have as an
optional feature (see my suggestion elsewhere as to a possible way to approach
this), but I don't think this can be taken as an indication that we should 
revert
back to "blind" exposure.

Jan

Re: [PATCH for-4.18 v2] automation: fix race condition in adl-suspend test

[Henry Wang]

Hi Marek,

> On Oct 31, 2023, at 10:16, Marek Marczykowski-Górecki 
>  wrote:
> 
> If system suspends too quickly, the message for the test controller to
> wake up the system may be not sent to the console before suspending.
> This will cause the test to timeout.
> 
> Fix this by calling sync on the console and waiting a bit after printing
> the message. The test controller then resumes the system 30s after the
> message, so as long as the delay + suspending takes less time it is
> okay.
> 
> Signed-off-by: Marek Marczykowski-Górecki 

I think now that we branched, this patch should be committed to both staging 
and staging-4.18.
For staging 4.18:

Release-acked-by: Henry Wang 

I will remove the commit moratorium for staging once OSSTest does a successful 
sync between
staging and master. Thanks.

Kind regards,
Henry

Re: [XEN PATCH][for-4.19 v4 1/8] xen/include: add macro ISOLATE_LOW_BIT

[Nicola Vetrini]

On 2023-10-31 09:28, Nicola Vetrini wrote:

On 2023-10-31 08:43, Jan Beulich wrote:

On 30.10.2023 23:44, Stefano Stabellini wrote:

On Mon, 30 Oct 2023, Jan Beulich wrote:

On 27.10.2023 15:34, Nicola Vetrini wrote:

--- a/xen/include/xen/macros.h
+++ b/xen/include/xen/macros.h
@@ -8,8 +8,14 @@
 #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
 #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))

-#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
-#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
+/*
+ * Given an unsigned integer argument, expands to a mask where 
just the least
+ * significant nonzero bit of the argument is set, or 0 if no bits 
are set.

+ */
+#define ISOLATE_LOW_BIT(x) ((x) & -(x))


Not even considering future Misra changes (which aiui may require 
that
anyway), this generalization of the macro imo demands that its 
argument

now be evaluated only once.


Fur sure that would be an improvement, but I don't see a trivial way 
to

do it and this issue is also present today before the patch.


This was an issue here for MASK_EXTR() and MASK_INSR(), yes, but the 
new

macro has wider use, and there was no issue elsewhere so far.


I think it
would be better to avoid scope-creeping this patch as we are already 
at
v4 for something that was expected to be a trivial mechanical change. 
I

would rather review the fix as a separate patch, maybe sent by you as
you probably have a specific implementation in mind?


#define ISOLATE_LOW_BIT(x) ({ \
typeof(x) x_ = (x); \
x_ & -x_; \
})

Hard to see the scope creep here. What I would consider scope creep I
specifically didn't even ask for: I'd like this macro to be 
overridable

by an arch. Specifically (see my earlier naming hint) I'd like to use
x86's BMI insn BLSI in the context of "x86: allow Kconfig control over
psABI level", when ABI v2 or higher is in use.

Jan


I appreciate you suggesting an implementation; I'll send a v5 
incorporating it.


There's an issue with this approach, though: since the macro is used 
indirectly
in expressions that are e.g. case labels or array sizes, the build fails 
(see [1] for instance).

Perhaps it's best to leave it as is?

[1] https://gitlab.com/xen-project/people/bugseng/xen/-/jobs/5423693947

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

live migration fails: qemu placing pci devices at different locations

[James Dingwall]

Hi,

I'm having a bit of trouble performing live migration between hvm guests.  The
sending side is xen 4.14.5 (qemu 5.0), receiving 4.15.5 (qemu 5.1).  The error
message recorded in qemu-dm---incoming.log:

qemu-system-i386: Unknown savevm section or instance ':00:04.0/vga' 0. Make 
sure that your current VM setup matches your saved VM setup, including any 
hotplugged devices

I have patched libxl_dm.c to explicitly assign `addr=xx` values for various
devices and when these are correct the domain migrates correctly.  However
the configuration differences between guests means that the values are not
consistent.  The domain config file doesn't allow the pci address to be
expressed in the configuration for, e.g. `soundhw="DEVICE"`

e.g. 

diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 6e531863ac0..daa7c49846f 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -1441,7 +1441,7 @@ static int libxl__build_device_model_args_new(libxl__gc 
*gc,
 flexarray_append(dm_args, "-spice");
 flexarray_append(dm_args, spiceoptions);
 if (libxl_defbool_val(b_info->u.hvm.spice.vdagent)) {
-flexarray_vappend(dm_args, "-device", "virtio-serial",
+flexarray_vappend(dm_args, "-device", "virtio-serial,addr=04",
 "-chardev", "spicevmc,id=vdagent,name=vdagent", "-device",
 "virtserialport,chardev=vdagent,name=com.redhat.spice.0",
 NULL);

The order of devices on the qemu command line (below) appears to be the same
so my assumption is that the internals of qemu have resulted in things being
connected in a different order.  The output of a Windows `lspci` tool is
also included.

Could anyone make any additional suggestions on how I could try to gain
consistency between the different qemu versions?

Thanks,
James


xen 4.14.5

/usr/lib/xen/bin/qemu-system-i386 -xen-domid 19 -no-shutdown
  -chardev socket,id=libxl-cmd,fd=19,server,nowait -S 
  -mon chardev=libxl-cmd,mode=control
  -chardev 
socket,id=libxenstat-cmd,path=/var/run/xen/qmp-libxenstat-19,server,nowait
  -mon chardev=libxenstat-cmd,mode=control
  -nodefaults -no-user-config -name  -vnc 0.0.0.0:93 -display none
  -k en-us
  -spice 
port=35993,tls-port=0,addr=127.0.0.1,disable-ticketing,agent-mouse=on,disable-copy-paste,image-compression=auto_glz
 
  -device virtio-serial -chardev spicevmc,id=vdagent,name=vdagent
  -device virtserialport,chardev=vdagent,name=com.redhat.spice.0
  -device VGA,vgamem_mb=16
  -boot order=cn
  -usb -usbdevice tablet
  -soundhw hda
  -smp 2,maxcpus=2
  -device rtl8139,id=nic0,netdev=net0,mac=00:16:3e:64:c8:68
  -netdev type=tap,id=net0,ifname=vif19.0-emu,script=no,downscript=no
  -object 
tls-creds-x509,id=tls0,endpoint=client,dir=/etc/certificates/usbredir,verify-peer=yes
  -chardev 
socket,id=charredir_serial0,host=127.0.0.1,port=48052,reconnect=2,nodelay,keepalive=on,user-timeout=5
  -device isa-serial,chardev=charredir_serial0
  -chardev 
socket,id=charredir_serial1,host=127.0.0.1,port=48054,reconnect=2,nodelay,keepalive=on,user-timeout=5
  -device isa-serial,chardev=charredir_serial1
  -chardev 
socket,id=charredir_serial2,host=127.0.0.1,port=48055,reconnect=2,nodelay,keepalive=on,user-timeout=5
  -device pci-serial,chardev=charredir_serial2
  -trace events=/etc/xen/qemu-trace-options -machine xenfv -m 2032
  -drive file=/dev/drbd1002,if=ide,index=0,media=disk,format=raw,cache=writeback
  -drive file=/dev/drbd1003,if=ide,index=1,media=disk,format=raw,cache=writeback
  -runas 131091:131072

00:00.0 Host bridge: Intel Corporation 440FX - 82441FX PMC [Natoma] (rev 02)
00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA [Natoma/Triton II]
00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE [Natoma/Triton II]
00:01.2 USB controller: Intel Corporation 82371SB PIIX3 USB [Natoma/Triton II] 
(rev 01)
00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03)
00:02.0 Unassigned class [ff80]: XenSource, Inc. Xen Platform Device (rev 01)
00:03.0 Audio device: Intel Corporation 82801FB/FBM/FR/FW/FRW (ICH6 Family) 
High Definition Audio Controller (rev 01)
00:04.0 Communication controller: Red Hat, Inc Virtio console
00:05.0 VGA compatible controller: Device 1234: (rev 02)
00:07.0 Serial controller: Red Hat, Inc. QEMU PCI 16550A Adapter (rev 01)



xen 4.15.5

/usr/lib/xen/bin/qemu-system-i386 -xen-domid 15 -no-shutdown
  -chardev socket,id=libxl-cmd,fd=19,server=on,wait=off -S
  -mon chardev=libxl-cmd,mode=control
  -chardev 
socket,id=libxenstat-cmd,path=/var/run/xen/qmp-libxenstat-15,server=on,wait=off
  -mon chardev=libxenstat-cmd,mode=control
  -nodefaults -no-user-config -name  -vnc 0.0.0.0:93 -display none
  -k en-us
  -spice 
port=35993,tls-port=0,addr=127.0.0.1,disable-ticketing=on,agent-mouse=on,disable-copy-paste=on,image-compression=auto_glz
  -device virtio-serial -chardev spicevmc,id=vdagent,name=vdagent
  -device virtserialport,charde

Re: [RFC PATCH 03/22] x86/msr: always allow a pinned Dom0 to read any unknown MSR

[Jan Beulich]

On 31.10.2023 10:31, Edwin Torok wrote:
>> On 30 Oct 2023, at 16:29, Jan Beulich  wrote:
>> On 25.10.2023 21:29, Edwin Török wrote:
>>> This can be useful if you realize you have to inspect the value of an
>>> MSR in production, without having to change into a new Xen first that
>>> handles the MSR.
>>
>> Yet on a non-pinned Dom0 you'd still be lost. Since iirc we generally
>> advise against pinning,
> 
> You can temporarily pin while debugging the issue (e.g. pin just 1 CPU from 
> Dom0, and "walk" all your physical CPUs with it if you have to,
> so that you query them all), e.g. with 'xl vcpu-pin'.
> Although that is more invasive than reading a value.
>  
> Or alternatively have another (privileged) interface to read the MSR for a 
> given core without exposing it to any guests, that way you don't affect the 
> running system at all
> (which would be preferable in a production environment), i.e. a Xen 
> equivalent of 'rdmsr'.

The interface we have (XENPF_resource_op) is, despite being privileged,
deliberately (so far at least) not permitting access to arbitrary MSRs.

In our old XenoLinux forward port we had an extension to the msr.ko
module to allow pCPU-based MSR accesses (and I had a private extension
to the rdmsr/wrmsr user space tools making use of that), but even that
would have been subject to restrictions enforced by Xen as to which
MSRs are accessible.

>> I wonder of how much use such a change would be,
>> when it effectively undoes what we deliberately did a while ago.
>>
>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>> @@ -1933,6 +1933,9 @@ static int cf_check svm_msr_read_intercept(
>>> break;
>>>
>>> default:
>>> +if ( is_hwdom_pinned_vcpu(v) && !rdmsr_safe(msr, *msr_content) )
>>> +break;
>>> +
>>> if ( d->arch.msr_relaxed && !rdmsr_safe(msr, tmp) )
>>> {
>>> *msr_content = 0;
>>
>> If we went as far as undoing some of what was done, I'd then wonder
>> whether instead we should mandate relaxed mode to be enabled on such a
>> Dom0. Then, instead of returning fake 0 here, the actual value could
>> be returned in the specific case of (pinned?) Dom0.
> 
> 
> Can relaxed mode be enabled at runtime?

Not right now, no. But a hypfs control could certainly be added, with
suitable justification.

> I'd be happy with either solution, but it should be something that can be 
> enabled at runtime
> (if you have to reboot Xen then you may lose the bug repro that you want to 
> gather more information on).
> Although changing such a setting in a production environment may still be 
> risky, because the guest will then become very confused that it has 
> previously read some 0s, now there are some real values, and later when you 
> flip the switch off it gets 0s again.

Indeed. If you flipped such a control for any domain at runtime, you'd
better first check that this wouldn't cause any such issues.

Jan

[PATCH] xen: remove

[Oleksii Kurochko]

 only declares udelay() function so udelay()  
declaration was moved to xen/delay.h.

For x86, __udelay() was renamed to udelay() and removed
inclusion of  in x86 code.

Signed-off-by: Oleksii Kurochko 
---
 xen/arch/arm/include/asm/delay.h   | 14 --
 xen/arch/riscv/include/asm/delay.h | 13 -
 xen/arch/x86/cpu/microcode/core.c  |  2 +-
 xen/arch/x86/delay.c   |  2 +-
 xen/arch/x86/include/asm/delay.h   | 13 -
 xen/include/xen/delay.h|  3 ++-
 6 files changed, 4 insertions(+), 43 deletions(-)
 delete mode 100644 xen/arch/arm/include/asm/delay.h
 delete mode 100644 xen/arch/riscv/include/asm/delay.h
 delete mode 100644 xen/arch/x86/include/asm/delay.h

diff --git a/xen/arch/arm/include/asm/delay.h b/xen/arch/arm/include/asm/delay.h
deleted file mode 100644
index 042907d9d5..00
--- a/xen/arch/arm/include/asm/delay.h
+++ /dev/null
@@ -1,14 +0,0 @@
-#ifndef _ARM_DELAY_H
-#define _ARM_DELAY_H
-
-extern void udelay(unsigned long usecs);
-
-#endif /* defined(_ARM_DELAY_H) */
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/arch/riscv/include/asm/delay.h 
b/xen/arch/riscv/include/asm/delay.h
deleted file mode 100644
index 2d59622c75..00
--- a/xen/arch/riscv/include/asm/delay.h
+++ /dev/null
@@ -1,13 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * Copyright (C) 2009 Chen Liqin 
- * Copyright (C) 2016 Regents of the University of California
- */
-
-#ifndef _ASM_RISCV_DELAY_H
-#define _ASM_RISCV_DELAY_H
-
-#define udelay udelay
-extern void udelay(unsigned long usecs);
-
-#endif /* _ASM_RISCV_DELAY_H */
diff --git a/xen/arch/x86/cpu/microcode/core.c 
b/xen/arch/x86/cpu/microcode/core.c
index c3fee62906..48822360c0 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -23,6 +23,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -35,7 +36,6 @@
 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
diff --git a/xen/arch/x86/delay.c b/xen/arch/x86/delay.c
index 2662c26272..b3a41881a1 100644
--- a/xen/arch/x86/delay.c
+++ b/xen/arch/x86/delay.c
@@ -15,7 +15,7 @@
 #include 
 #include 
 
-void __udelay(unsigned long usecs)
+void udelay(unsigned long usecs)
 {
 unsigned long ticks = usecs * (cpu_khz / 1000);
 unsigned long s, e;
diff --git a/xen/arch/x86/include/asm/delay.h b/xen/arch/x86/include/asm/delay.h
deleted file mode 100644
index 9be2f46590..00
--- a/xen/arch/x86/include/asm/delay.h
+++ /dev/null
@@ -1,13 +0,0 @@
-#ifndef _X86_DELAY_H
-#define _X86_DELAY_H
-
-/*
- * Copyright (C) 1993 Linus Torvalds
- *
- * Delay routines calling functions in arch/i386/lib/delay.c
- */
-
-extern void __udelay(unsigned long usecs);
-#define udelay(n) __udelay(n)
-
-#endif /* defined(_X86_DELAY_H) */
diff --git a/xen/include/xen/delay.h b/xen/include/xen/delay.h
index 9d70ef035f..a5189329c7 100644
--- a/xen/include/xen/delay.h
+++ b/xen/include/xen/delay.h
@@ -3,8 +3,9 @@
 
 /* Copyright (C) 1993 Linus Torvalds */
 
-#include 
 #define mdelay(n) (\
{unsigned long msec=(n); while (msec--) udelay(1000);})
 
+void udelay(unsigned long usecs);
+
 #endif /* defined(_LINUX_DELAY_H) */
-- 
2.41.0

Re: [XEN PATCH][for-4.19 v4 1/8] xen/include: add macro ISOLATE_LOW_BIT

[Jan Beulich]

On 31.10.2023 11:03, Nicola Vetrini wrote:
> On 2023-10-31 09:28, Nicola Vetrini wrote:
>> On 2023-10-31 08:43, Jan Beulich wrote:
>>> On 30.10.2023 23:44, Stefano Stabellini wrote:
 On Mon, 30 Oct 2023, Jan Beulich wrote:
> On 27.10.2023 15:34, Nicola Vetrini wrote:
>> --- a/xen/include/xen/macros.h
>> +++ b/xen/include/xen/macros.h
>> @@ -8,8 +8,14 @@
>>  #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
>>  #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
>>
>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
>> +/*
>> + * Given an unsigned integer argument, expands to a mask where 
>> just the least
>> + * significant nonzero bit of the argument is set, or 0 if no bits 
>> are set.
>> + */
>> +#define ISOLATE_LOW_BIT(x) ((x) & -(x))
>
> Not even considering future Misra changes (which aiui may require 
> that
> anyway), this generalization of the macro imo demands that its 
> argument
> now be evaluated only once.

 Fur sure that would be an improvement, but I don't see a trivial way 
 to
 do it and this issue is also present today before the patch.
>>>
>>> This was an issue here for MASK_EXTR() and MASK_INSR(), yes, but the 
>>> new
>>> macro has wider use, and there was no issue elsewhere so far.
>>>
 I think it
 would be better to avoid scope-creeping this patch as we are already 
 at
 v4 for something that was expected to be a trivial mechanical change. 
 I
 would rather review the fix as a separate patch, maybe sent by you as
 you probably have a specific implementation in mind?
>>>
>>> #define ISOLATE_LOW_BIT(x) ({ \
>>> typeof(x) x_ = (x); \
>>> x_ & -x_; \
>>> })
>>>
>>> Hard to see the scope creep here. What I would consider scope creep I
>>> specifically didn't even ask for: I'd like this macro to be 
>>> overridable
>>> by an arch. Specifically (see my earlier naming hint) I'd like to use
>>> x86's BMI insn BLSI in the context of "x86: allow Kconfig control over
>>> psABI level", when ABI v2 or higher is in use.
>>
>> I appreciate you suggesting an implementation; I'll send a v5 
>> incorporating it.
> 
> There's an issue with this approach, though: since the macro is used 
> indirectly
> in expressions that are e.g. case labels or array sizes, the build fails 
> (see [1] for instance).
> Perhaps it's best to leave it as is?

Hmm. I'm afraid it's not an option to "leave as is", not the least because
- as said - I'm under the impression that another Misra rule requires
macro arguments to be evaluated exactly once. Best I can think of right
away is to have a macro for limited use (to address such build issues)
plus an inline function (for general use). But yes, maybe that then indeed
needs to be a 2nd step.

Jan

> [1] https://gitlab.com/xen-project/people/bugseng/xen/-/jobs/5423693947
> 

[PATCH v3] x86/hvm/dom0: fix PVH initrd and metadata placement

[Xenia Ragiadakou]

Zephyr image consists of multiple non-contiguous load segments
that reside in different RAM regions. For instance:
ELF: phdr: paddr=0x1000 memsz=0x8000
ELF: phdr: paddr=0x10 memsz=0x28a90
ELF: phdr: paddr=0x128aa0 memsz=0x7560
ELF: memory: 0x1000 -> 0x13

However, the logic that determines the best placement for dom0
initrd and metadata, assumes that the image is fully contained
in a single RAM region, not taking into account the cases where:
(1) start > kernel_start && end > kernel_end
(2) start < kernel_start && end < kernel_end
(3) start > kernel_start && end < kernel_end

In case (1), the evaluation will result in end = kernel_start,
i.e. end < start, and will load initrd in the middle of the kernel.
In case (2), the evaluation will result in start = kernel_end,
i.e. end < start, and will load initrd at kernel_end, that is out
of the memory region under evaluation.
In case (3), the evaluation will result in either end = kernel_start
or start = kernel_end but in both cases will be end < start, and
will either load initrd in the middle of the image, or arbitrarily
at kernel_end.

This patch reorganizes the conditionals to include so far unconsidered
cases as well, uniformly returning the lowest available address.

Fixes: 73b47eea2104 ('x86/dom0: improve PVH initrd and metadata placement')
Signed-off-by: Xenia Ragiadakou 
Signed-off-by: Jan Beulich 
Reviewed-by: Roger Pau Monné 
---

Changes in v3:
- mention Zephyr in commit message (Roger)

Changes in v2:
- cover further cases of overlap (Jan)
- mention with an in-code comment that a proper, more fine-grained
  solution can be implemented using a rangeset (Roger)

 xen/arch/x86/hvm/dom0_build.c | 21 ++---
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index c7d47d0d4c..62debc7415 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -515,16 +515,23 @@ static paddr_t __init find_memory(
 
 ASSERT(IS_ALIGNED(start, PAGE_SIZE) && IS_ALIGNED(end, PAGE_SIZE));
 
+/*
+ * NB: Even better would be to use rangesets to determine a suitable
+ * range, in particular in case a kernel requests multiple heavily
+ * discontiguous regions (which right now we fold all into one big
+ * region).
+ */
 if ( end <= kernel_start || start >= kernel_end )
-; /* No overlap, nothing to do. */
+{
+/* No overlap, just check whether the region is large enough. */
+if ( end - start >= size )
+return start;
+}
 /* Deal with the kernel already being loaded in the region. */
-else if ( kernel_start - start > end - kernel_end )
-end = kernel_start;
-else
-start = kernel_end;
-
-if ( end - start >= size )
+else if ( kernel_start > start && kernel_start - start >= size )
 return start;
+else if ( kernel_end < end && end - kernel_end >= size )
+return kernel_end;
 }
 
 return INVALID_PADDR;
-- 
2.34.1

Re: [XEN PATCH v2] xen/domain_page: address violations of MISRA C:2012 Rule 8.3

[Jan Beulich]

On 31.10.2023 10:25, Federico Serafini wrote:
> Make function defintions and declarations consistent.
> No functional change.
> 
> Signed-off-by: Federico Serafini 

Acked-by: Jan Beulich 

However, ...

> ---
> Changes in v2:
> - use 'ptr' do denote a const void * parameter.

... not even this (let alone the description) clarifies what the
inconsistency was. I had to go check to figure that x86 already uses
"ptr". Such things could do with spelling out.

> @@ -55,8 +55,8 @@ static inline void *__map_domain_page_global(const struct 
> page_info *pg)
>  
>  #define map_domain_page(mfn)__mfn_to_virt(mfn_x(mfn))
>  #define __map_domain_page(pg)   page_to_virt(pg)
> -#define unmap_domain_page(va)   ((void)(va))
> -#define domain_page_map_to_mfn(va)  _mfn(__virt_to_mfn((unsigned 
> long)(va)))
> +#define unmap_domain_page(ptr)   ((void)(ptr))
> +#define domain_page_map_to_mfn(ptr)  _mfn(__virt_to_mfn((unsigned 
> long)(ptr)))

Padding wants to not be screwed by the change (one of the blanks will
want dropping). I guess this can be taken care of while committing.

Jan

Re: [PATCH] xen: remove

[Jan Beulich]

On 31.10.2023 11:12, Oleksii Kurochko wrote:
>  only declares udelay() function so udelay()  
> declaration was moved to xen/delay.h.
> 
> For x86, __udelay() was renamed to udelay() and removed
> inclusion of  in x86 code.
> 
> Signed-off-by: Oleksii Kurochko 
> ---
>  xen/arch/arm/include/asm/delay.h   | 14 --
>  xen/arch/riscv/include/asm/delay.h | 13 -
>  xen/arch/x86/cpu/microcode/core.c  |  2 +-
>  xen/arch/x86/delay.c   |  2 +-
>  xen/arch/x86/include/asm/delay.h   | 13 -
>  xen/include/xen/delay.h|  3 ++-
>  6 files changed, 4 insertions(+), 43 deletions(-)
>  delete mode 100644 xen/arch/arm/include/asm/delay.h
>  delete mode 100644 xen/arch/riscv/include/asm/delay.h
>  delete mode 100644 xen/arch/x86/include/asm/delay.h

What about xen/arch/ppc/include/asm/delay.h? With that also removed
Reviewed-by: Jan Beulich 
(and maybe also Suggested-by:?)

Jan

Re: [XEN PATCH v2] xen/domain_page: address violations of MISRA C:2012 Rule 8.3

[Julien Grall]

Hi,

On 31/10/2023 10:31, Jan Beulich wrote:

On 31.10.2023 10:25, Federico Serafini wrote:

Make function defintions and declarations consistent.


typo: s/defintions/definitions/


No functional change.

Signed-off-by: Federico Serafini 


Acked-by: Jan Beulich 

However, ...


---
Changes in v2:
- use 'ptr' do denote a const void * parameter.


... not even this (let alone the description) clarifies what the
inconsistency was. I had to go check to figure that x86 already uses
"ptr". Such things could do with spelling out.


+1. The more that x86 was the "odd" one but it was chosen to use the 
variant everywhere.


With the commit message clarified:

Acked-by: Julien Grall 

Cheers,




@@ -55,8 +55,8 @@ static inline void *__map_domain_page_global(const struct 
page_info *pg)
  
  #define map_domain_page(mfn)__mfn_to_virt(mfn_x(mfn))

  #define __map_domain_page(pg)   page_to_virt(pg)
-#define unmap_domain_page(va)   ((void)(va))
-#define domain_page_map_to_mfn(va)  _mfn(__virt_to_mfn((unsigned 
long)(va)))
+#define unmap_domain_page(ptr)   ((void)(ptr))
+#define domain_page_map_to_mfn(ptr)  _mfn(__virt_to_mfn((unsigned 
long)(ptr)))


Padding wants to not be screwed by the change (one of the blanks will
want dropping). I guess this can be taken care of while committing.

Jan


--
Julien Grall

Re: [PATCH for-4.18 v2] automation: fix race condition in adl-suspend test

[Andrew Cooper]

On 31/10/2023 9:58 am, Henry Wang wrote:
> Hi Marek,
>
>> On Oct 31, 2023, at 10:16, Marek Marczykowski-Górecki 
>>  wrote:
>>
>> If system suspends too quickly, the message for the test controller to
>> wake up the system may be not sent to the console before suspending.
>> This will cause the test to timeout.
>>
>> Fix this by calling sync on the console and waiting a bit after printing
>> the message. The test controller then resumes the system 30s after the
>> message, so as long as the delay + suspending takes less time it is
>> okay.
>>
>> Signed-off-by: Marek Marczykowski-Górecki 
> I think now that we branched, this patch should be committed to both staging 
> and staging-4.18.
> For staging 4.18:
>
> Release-acked-by: Henry Wang 
>
> I will remove the commit moratorium for staging once OSSTest does a 
> successful sync between
> staging and master. Thanks.

Acked-by: Andrew Cooper 

I'll get this sorted now.

~Andrew

Re: [PATCH v4 2/5] xen/vpci: move xen_domctl_createdomain vPCI flag to common

[Jan Beulich]

On 31.10.2023 00:52, Stewart Hildebrand wrote:
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -607,7 +607,8 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>  {
>  unsigned int max_vcpus;
>  unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap);
> -unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | 
> XEN_DOMCTL_CDF_vpmu);
> +unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | 
> XEN_DOMCTL_CDF_vpmu |
> +   XEN_DOMCTL_CDF_vpci);

Is the flag (going to be, with the initial work) okay to have for Dom0
on Arm?

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -712,7 +712,8 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>  return 0;
>  }
>  
> -static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
> +static bool emulation_flags_ok(const struct domain *d, uint32_t emflags,
> +   uint32_t cdf)

While apparently views differ, ./CODING_STYLE wants "unsigned int" to be
used for the latter two arguments.

> @@ -722,14 +723,17 @@ static bool emulation_flags_ok(const struct domain *d, 
> uint32_t emflags)
>  if ( is_hvm_domain(d) )
>  {
>  if ( is_hardware_domain(d) &&
> - emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC) )
> + (!( cdf & XEN_DOMCTL_CDF_vpci ) ||

Nit: Stray blanks inside the inner parentheses.

> +  emflags != (X86_EMU_LAPIC | X86_EMU_IOAPIC)) )
>  return false;
>  if ( !is_hardware_domain(d) &&
> - emflags != (X86_EMU_ALL & ~X86_EMU_VPCI) &&
> - emflags != X86_EMU_LAPIC )
> + ((cdf & XEN_DOMCTL_CDF_vpci) ||
> +  (emflags != X86_EMU_ALL &&
> +   emflags != X86_EMU_LAPIC)) )
>  return false;
>  }
> -else if ( emflags != 0 && emflags != X86_EMU_PIT )
> +else if ( (cdf & XEN_DOMCTL_CDF_vpci) ||

Wouldn't this better be enforced in common code?

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -892,10 +892,11 @@ static struct domain *__init create_dom0(const module_t 
> *image,
>  {
>  dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm |
> ((hvm_hap_supported() && !opt_dom0_shadow) ?
> -XEN_DOMCTL_CDF_hap : 0));
> +XEN_DOMCTL_CDF_hap : 0) |
> +   XEN_DOMCTL_CDF_vpci);

Less of a change and imo slightly neater as a result would be to simply
put the addition on the same line where CDF_hvm already is. But as with
many style aspects, views may differ here of course ...

Jan

Re: [PATCH v4 4/5] [FUTURE] xen/arm: enable vPCI for domUs

[Jan Beulich]

On 31.10.2023 00:52, Stewart Hildebrand wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1618,6 +1618,16 @@ int iommu_do_pci_domctl(
>  bus = PCI_BUS(machine_sbdf);
>  devfn = PCI_DEVFN(machine_sbdf);
>  
> +if ( IS_ENABLED(CONFIG_ARM) &&
> + !is_hardware_domain(d) &&
> + !is_system_domain(d) &&
> + (!IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) || !has_vpci(d)) )

I don't think you need the explicit ARM check; that's redundant with
checking !HAS_VPCI_GUEST_SUPPORT. It's also not really clear why you
need to check for the system domain here.

> +{
> +printk(XENLOG_G_WARNING "Cannot assign %pp to %pd: vPCI support 
> not enabled\n",
> +   &PCI_SBDF(seg, bus, devfn), d);

ret = -EPERM;

(or some other suitable error indicator)

Jan

> +break;
> +}
> +
>  pcidevs_lock();
>  ret = device_assigned(seg, bus, devfn);
>  if ( domctl->cmd == XEN_DOMCTL_test_assign_device )

Re: [RFC PATCH 04/22] x86/msr-index: add references to vendor manuals

[Andrew Cooper]

On 30/10/2023 4:15 pm, Jan Beulich wrote:
>> --- a/xen/arch/x86/include/asm/msr-index.h
>> +++ b/xen/arch/x86/include/asm/msr-index.h
>> @@ -13,6 +13,16 @@
>>   * Blocks of related constants should be sorted by MSR index.  The constant
>>   * names should be as concise as possible, and the bit names may have an
>>   * abbreviated name.  Exceptions will be considered on a case-by-case basis.
>> + *
>> + * References:
>> + * - 
>> https://software.intel.com/content/www/us/en/develop/articles/intel-sdm.html
>> + *Intel(R) 64 and IA-32 architectures SDM volume 4: Model-specific 
>> registers
>> + *Chapter 2, "Model-Specific Registers (MSRs)"
> ... at least Intel's URL has changed several times over the years. Volume
> and chapter numbers change even more frequently. Any such is liable to go
> stale at some point.

https://intel.com/sdm

This one has been valid for roughly the lifetime of intel.com, and is
committed to stay so.

>
> Jan
>
>> + * - https://developer.amd.com/resources/developer-guides-manuals/

whereas AMD really have broken this one, and don't seem to be showing
any urgency in unbreaking it...  Right now there is no landing page at
all for manuals.

~Andrew

[PATCH for-4.18] docs: Fix IOMMU command line docs some more

[Andrew Cooper]

Make the command line docs match the actual implementation, and state that the
default behaviour is selected at compile time.

Fixes: 980d6acf1517 ("IOMMU: make DMA containment of quarantined devices 
optional")
Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Wei Liu 
CC: Marek Marczykowski-Górecki 
CC: Henry Wang 
---
 docs/misc/xen-command-line.pandoc | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc 
b/docs/misc/xen-command-line.pandoc
index 6b07d0f3a17f..9a19a04157cb 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1480,7 +1480,8 @@ detection of systems known to misbehave upon accesses to 
that port.
 > Default: `new` unless directed-EOI is supported
 
 ### iommu
-= List of [ , verbose, debug, force, required, 
quarantine[=scratch-page],
+= List of [ , verbose, debug, force, required,
+quarantine=|scratch-page,
 sharept, superpages, intremap, intpost, crash-disable,
 snoop, qinval, igfx, amd-iommu-perdev-intremap,
 dom0-{passthrough,strict} ]
@@ -1519,7 +1520,8 @@ boolean (e.g. `iommu=no`) can override this and leave the 
IOMMUs disabled.
 successfully.
 
 *   The `quarantine` option can be used to control Xen's behavior when
-de-assigning devices from guests.
+de-assigning devices from guests.  The default behaviour is chosen at
+compile time, and is one of 
`CONFIG_IOMMU_QUARANTINE_{NONE,BASIC,SCRATCH_PAGE}`.
 
 When a PCI device is assigned to an untrusted domain, it is possible
 for that domain to program the device to DMA to an arbitrary address.

base-commit: 9659b2a6d73b14620e187f9c626a09323853c459
-- 
2.30.2

Re: [XEN PATCH][for-4.19 v4 1/8] xen/include: add macro ISOLATE_LOW_BIT

[Nicola Vetrini]

On 2023-10-31 11:20, Jan Beulich wrote:

On 31.10.2023 11:03, Nicola Vetrini wrote:

On 2023-10-31 09:28, Nicola Vetrini wrote:

On 2023-10-31 08:43, Jan Beulich wrote:

On 30.10.2023 23:44, Stefano Stabellini wrote:

On Mon, 30 Oct 2023, Jan Beulich wrote:

On 27.10.2023 15:34, Nicola Vetrini wrote:

--- a/xen/include/xen/macros.h
+++ b/xen/include/xen/macros.h
@@ -8,8 +8,14 @@
 #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
 #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))

-#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
-#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
+/*
+ * Given an unsigned integer argument, expands to a mask where
just the least
+ * significant nonzero bit of the argument is set, or 0 if no 
bits

are set.
+ */
+#define ISOLATE_LOW_BIT(x) ((x) & -(x))


Not even considering future Misra changes (which aiui may require
that
anyway), this generalization of the macro imo demands that its
argument
now be evaluated only once.


Fur sure that would be an improvement, but I don't see a trivial 
way

to
do it and this issue is also present today before the patch.


This was an issue here for MASK_EXTR() and MASK_INSR(), yes, but the
new
macro has wider use, and there was no issue elsewhere so far.


I think it
would be better to avoid scope-creeping this patch as we are 
already

at
v4 for something that was expected to be a trivial mechanical 
change.

I
would rather review the fix as a separate patch, maybe sent by you 
as

you probably have a specific implementation in mind?


#define ISOLATE_LOW_BIT(x) ({ \
typeof(x) x_ = (x); \
x_ & -x_; \
})

Hard to see the scope creep here. What I would consider scope creep 
I

specifically didn't even ask for: I'd like this macro to be
overridable
by an arch. Specifically (see my earlier naming hint) I'd like to 
use
x86's BMI insn BLSI in the context of "x86: allow Kconfig control 
over

psABI level", when ABI v2 or higher is in use.


I appreciate you suggesting an implementation; I'll send a v5
incorporating it.


There's an issue with this approach, though: since the macro is used
indirectly
in expressions that are e.g. case labels or array sizes, the build 
fails

(see [1] for instance).
Perhaps it's best to leave it as is?


Hmm. I'm afraid it's not an option to "leave as is", not the least 
because

- as said - I'm under the impression that another Misra rule requires
macro arguments to be evaluated exactly once. Best I can think of right
away is to have a macro for limited use (to address such build issues)
plus an inline function (for general use). But yes, maybe that then 
indeed

needs to be a 2nd step.

Jan

[1] 
https://gitlab.com/xen-project/people/bugseng/xen/-/jobs/5423693947




There is no such MISRA Rule afaik: R23.7 is similar, but only for C11 
generic selections.


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

Re: [RFC PATCH 04/22] x86/msr-index: add references to vendor manuals

[Edwin Torok]

> On 31 Oct 2023, at 11:34, Andrew Cooper  wrote:
> 
> On 30/10/2023 4:15 pm, Jan Beulich wrote:
>> 
>>> --- a/xen/arch/x86/include/asm/msr-index.h
>>> +++ b/xen/arch/x86/include/asm/msr-index.h
>>> @@ -13,6 +13,16 @@
>>> * Blocks of related constants should be sorted by MSR index. The constant
>>> * names should be as concise as possible, and the bit names may have an
>>> * abbreviated name. Exceptions will be considered on a case-by-case basis.
>>> + *
>>> + * References:
>>> + * - 
>>> https://software.intel.com/content/www/us/en/develop/articles/intel-sdm.html
>>> + * Intel(R) 64 and IA-32 architectures SDM volume 4: Model-specific 
>>> registers
>>> + * Chapter 2, "Model-Specific Registers (MSRs)"
>>> 
>> ... at least Intel's URL has changed several times over the years. Volume
>> and chapter numbers change even more frequently. Any such is liable to go
>> stale at some point.
> 
> https://intel.com/sdm
> 
> This one has been valid for roughly the lifetime of intel.com, and is 
> committed to stay so.

That is useful to know, I'll update the URL.

> 
>> 
>> Jan
>> 
>> 
>>> + * - https://developer.amd.com/resources/developer-guides-manuals/
> 
> whereas AMD really have broken this one, and don't seem to be showing any 
> urgency in unbreaking it...  Right now there is no landing page at all for 
> manuals.
> 


Linux commits appear to reference a certain bugzilla that has the manuals 
uploaded: https://bugzilla.kernel.org/show_bug.cgi?id=206537
(although they will go stale in another way, e.g. I see no 2023 manuals there, 
but at least you know which manual a given commit referenced).
Although referencing someone else's bugzilla in the Xen codebase wouldn't be a 
nice thing to do, so if we do this it'd probably have to be something hosted on 
Xen infra.

For now I'll probably drop the URL and just keep the name (so at least you'd 
know what to search for).


Best regards,
--Edwin

> ~Andrew

[linux-linus test] 183625: regressions - FAIL

[osstest service owner]

flight 183625 linux-linus real [real]
http://logs.test-lab.xenproject.org/osstest/logs/183625/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-arm64-arm64-examine  8 reboot   fail REGR. vs. 183617
 test-arm64-arm64-xl-credit2   8 xen-boot fail REGR. vs. 183617
 test-arm64-arm64-xl-thunderx  8 xen-boot fail REGR. vs. 183617
 test-arm64-arm64-xl-credit1   8 xen-boot fail REGR. vs. 183617
 test-arm64-arm64-xl   8 xen-boot fail REGR. vs. 183617
 test-arm64-arm64-xl-vhd   8 xen-boot fail REGR. vs. 183617
 test-arm64-arm64-libvirt-raw  8 xen-boot fail REGR. vs. 183617
 build-arm64-xsm   6 xen-buildfail REGR. vs. 183617
 test-armhf-armhf-libvirt 10 host-ping-check-xen  fail REGR. vs. 183617

Tests which did not succeed, but are not blocking:
 test-arm64-arm64-xl-xsm   1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 183617
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 183617
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 183617
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 183617
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 183617
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 183617
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 183617
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  15 saverestore-support-checkfail   never pass

version targeted for testing:
 linuxecb8cd2a9f7af7f99a6d4fa0a5a31822f6cfe255
baseline version:
 linuxffc253263a1375a65fa6c9f62a893e9767fbebfa

Last test of basis   183617  2023-10-30 08:36:55 Z1 days
Testing same since   183625  2023-10-31 02:27:44 Z0 days1 attempts


People who touched revisions under test:
  "Darrick J. Wong" 
  "Kuyo Chang (張建文)" 
  "Md. Haris Iqbal" 
  "Peter Zijlstra (Intel)" 
  "Rafael J. Wysocki" 
  Aaron Lu 
  Aaron Plattner 
  Adam Dunlap 
  Alexander Aring 
  Alexander Shishkin 
  Alexey Dobriyan 
  Alison Schofield 
  Amir Goldstein 
  Anand Jain 
  Anders Roxell 
  Ard Biesheuvel 
  Atul Kumar Pant 
  Babu Moger 
  Baolin Liu 
  Barry Song 
  Ben Wolsieffer 
  Bernd Schubert 
  Binbin Wu 
  Bjorn Helgaas 
  Boris Burkov 
  Borislav Petkov (AMD) 
  Borislav Petkov 
  Brett Holman 
  Brett Holman 
  Brian Foster 
  Brian Gerst 
  Chen Hanxiao 
  Chengming Zhou 
  Chris Webb 
  Christian Brauner 
  Christoph Hellwig 
  Christophe JAILLET 
  Christopher James Halse Rogers 
  Chuck Lever 
  Colin Ian King 
  Coly Li 
  Cuda-Chen 
  Cyril Hrubis 
  Dai Ngo 
  Dan Carpenter 
  Dan Robertson 
  Daniel B. Hill 
  Daniel Hill 
  Dave Hansen 
  Dave Kleikamp 
  David Hildenbrand 
  David Howells 
  David Kaplan 
  David Reaver 
  David Sterba 
  Derick Marks 
  Dominique Martinet 
  Elliot Berman 
  Eric Biggers 
  Fan Yu 
  Fangrui Song 
  Fenghua Yu 
  Filipe Manana 
  Finn Thain 
  Gao Xiang 
  Gautham R. Shenoy 
  Geert Uytterhoeven 
  Greg Kroah-Hartman 
  Guo Ren 
  Guo 

Re: [PATCH for-4.18] docs: Fix IOMMU command line docs some more

[Roger Pau Monné]

On Tue, Oct 31, 2023 at 12:02:15PM +, Andrew Cooper wrote:
> Make the command line docs match the actual implementation, and state that the
> default behaviour is selected at compile time.
> 
> Fixes: 980d6acf1517 ("IOMMU: make DMA containment of quarantined devices 
> optional")
> Signed-off-by: Andrew Cooper 
> ---
> CC: Jan Beulich 
> CC: Roger Pau Monné 
> CC: Wei Liu 
> CC: Marek Marczykowski-Górecki 
> CC: Henry Wang 
> ---
>  docs/misc/xen-command-line.pandoc | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.pandoc 
> b/docs/misc/xen-command-line.pandoc
> index 6b07d0f3a17f..9a19a04157cb 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1480,7 +1480,8 @@ detection of systems known to misbehave upon accesses 
> to that port.
>  > Default: `new` unless directed-EOI is supported
>  
>  ### iommu
> -= List of [ , verbose, debug, force, required, 
> quarantine[=scratch-page],
> += List of [ , verbose, debug, force, required,
> +quarantine=|scratch-page,

I think this should be quarantine=[|scratch-page], as just using
iommu=quarantine is a valid syntax and will enable basic quarantine.
IOW: the bool or scratch-page parameters are optional.

>  sharept, superpages, intremap, intpost, crash-disable,
>  snoop, qinval, igfx, amd-iommu-perdev-intremap,
>  dom0-{passthrough,strict} ]
> @@ -1519,7 +1520,8 @@ boolean (e.g. `iommu=no`) can override this and leave 
> the IOMMUs disabled.
>  successfully.
>  
>  *   The `quarantine` option can be used to control Xen's behavior when
> -de-assigning devices from guests.
> +de-assigning devices from guests.  The default behaviour is chosen at
> +compile time, and is one of 
> `CONFIG_IOMMU_QUARANTINE_{NONE,BASIC,SCRATCH_PAGE}`.

Do we also want to state that the current build time default is BASIC
if the user hasn't selected otherwise?

It's kind of problematic though, as distros might select a different
build time default and then the documentation would be out of sync.

Thanks, Roger.

Re: [PATCH v1 12/29] xen/asm-generic: introduce stub header pci.h

[Oleksii]

On Mon, 2023-10-30 at 17:43 +0100, Jan Beulich wrote:
> On 30.10.2023 17:34, Oleksii wrote:
> > Hello Jan,
> > 
> > On Thu, 2023-10-19 at 11:55 +0200, Jan Beulich wrote:
> > > On 14.09.2023 16:56, Oleksii Kurochko wrote:
> > > > --- /dev/null
> > > > +++ b/xen/include/asm-generic/pci.h
> > > > @@ -0,0 +1,18 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > +#ifndef __ASM_GENERIC_PCI_H__
> > > > +#define __ASM_GENERIC_PCI_H__
> > > > +
> > > > +struct arch_pci_dev {
> > > > +};
> > > > +
> > > > +#endif /* __ASM_GENERIC_PCI_H__ */
> > > 
> > > While more involved, I still wonder whether xen/pci.h could also
> > > avoid
> > > including asm/pci.h when !HAS_PCI. Of course there's more than
> > > just
> > > the
> > > #include which then would need #ifdef-ing out.
> > > 
> > > Jan
> > 
> > It looks like we can do that but only one question should be
> > resolved.
> > In ARM case, in  there is !HAS_PCI branch:
> > 
> > #else   /*!CONFIG_HAS_PCI*/
> > 
> > struct arch_pci_dev { };
> > 
> > static always_inline bool is_pci_passthrough_enabled(void)
> > {
> >     return false;
> > }
> > 
> > struct pci_dev;
> > 
> > static inline void arch_pci_init_pdev(struct pci_dev *pdev) {}
> > 
> > static inline int pci_get_host_bridge_segment(const struct
> > dt_device_node *node,
> >   uint16_t *segment)
> > {
> >     ASSERT_UNREACHABLE();
> >     return -EINVAL;
> > }
> > 
> > static inline int pci_get_new_domain_nr(void)
> > {
> >     ASSERT_UNREACHABLE();
> >     return -1;
> > }
> > 
> > #endif  /*!CONFIG_HAS_PCI*/
> > 
> > And if is_pci_passthrough_enabled(), arch_pci_init_pdev() is used
> > by
> > all architrectures but pci_get_host_bridge_segment() and
> > pci_get_new_domain_nr() is ARM specific.
> > Does it make sense to add them to  and ifdef them?
> 
> Counter question: Is the arch_pci_init_pdev() stub actually needed?
> The sole caller looks to be in a file which is only built when
> HAS_PCI=y.
You are right. It seems that there is no need for pci_init_pdev() stub.

> 
> For the Arm-only stubs (which are called from Arm-specific code
> afaics)
> all it would take is that the respective .c files include asm/pci.h
> (possibly alongside xen/pci.h).
We can do in that way.

Thanks.

~ Oleksii

Re: [PATCH for-4.18] docs: Fix IOMMU command line docs some more

[Henry Wang]

Hi Andrew,

> On Oct 31, 2023, at 20:02, Andrew Cooper  wrote:
> 
> Make the command line docs match the actual implementation, and state that the
> default behaviour is selected at compile time.
> 
> Fixes: 980d6acf1517 ("IOMMU: make DMA containment of quarantined devices 
> optional")
> Signed-off-by: Andrew Cooper 

Release-acked-by: Henry Wang 

Kind regards,
Henry

Re: Cambridge University Talk - 9th November 2023

[Ayan Kumar Halder]

Hi Xen Maintainers/developers,


As part of my talk, I wish to provide some examples of tasks that a 
newbie can easily pick up and contribute.


This need not be a dedicated project, but something that can be 
contributed on an ad-hoc basis.


The idea is to get more people interested in Xen project. :)


I found some examples of this :-

1. Misra C fixes - Refer "Misra rule 10.3 violations report script" . 
Luca has provided an awesome script to identify the MISRA violations. 
This can be used to provide fixes.


2. https://wiki.xenproject.org/wiki/Outreach_Program_Projects - I think 
this page provides some pointers, but I am not sure if this is up to date.



Please let me know if there are more of these examples.


Kind regards,

Ayan


On 30/10/2023 17:54, Kelly Choi wrote:

Hello Xen Community!

I'm excited to share that we will be presenting a talk at Cambridge 
University!

This is free and open to everyone, including students and the public.

Make sure to add this to your calendars and come along.

*Date: Thursday 9th November 2023*
*Time: 3 - 4pm *
*Location:
*
*Computer Laboratory
William Gates Building
15 JJ Thomson Avenue*
*Cambridge CB3 0FD
https://www.cl.cam.ac.uk/directions/ *

Title: Navigating the Open Source Landscape: Insights from Ayan Kumar 
and Edwin Torok 


Join us for an illuminating seminar featuring two distinguished 
speakers, Ayan Kumar and Edwin Torok, who will delve into the 
intricate world of open-source projects.


Ayan Kumar: In his engaging presentation, Ayan Kumar will be your 
guide through the inner workings of open-source projects, using the 
Xen hypervisor as a compelling example. With a keen focus on 
demystifying the nuances of open-source collaborations, Ayan will walk 
you through the step-by-step workflow for contributions, shedding 
light on the collaborative modes that fuel innovation. Get ready to be 
inspired by the fascinating ongoing developments in the Xen 
hypervisor. Ayan will also provide invaluable insights for newcomers, 
outlining promising avenues for their initial contributions. The 
session will culminate in a hands-on demonstration featuring a 
selection of noteworthy open-source projects.


Edwin Torok: Edwin Torok will offer invaluable wisdom on the unique 
challenges of joining and maintaining a venerable, decade-old 
codebase, drawing from his extensive experience with the XAPI project. 
With a deep dive into the strategies and practices that sustain such a 
longstanding project, Edwin will equip you with the insights needed to 
navigate and contribute effectively to large-scale, established 
codebases.


Don't miss this opportunity to gain firsthand knowledge from these two 
seasoned experts in the open-source arena. Join us for an enriching 
seminar that promises to empower both beginners and seasoned 
developers alike.


Many thanks,
Kelly Choi

Open Source Community Manager
XenServer, Cloud Software Group

Re: [PATCH v4 4/5] [FUTURE] xen/arm: enable vPCI for domUs

[Julien Grall]

Hi,

On 31/10/2023 11:03, Jan Beulich wrote:

On 31.10.2023 00:52, Stewart Hildebrand wrote:

--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1618,6 +1618,16 @@ int iommu_do_pci_domctl(
  bus = PCI_BUS(machine_sbdf);
  devfn = PCI_DEVFN(machine_sbdf);
  
+if ( IS_ENABLED(CONFIG_ARM) &&

+ !is_hardware_domain(d) &&
+ !is_system_domain(d) &&
+ (!IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) || !has_vpci(d)) )


I don't think you need the explicit ARM check; that's redundant with
checking !HAS_VPCI_GUEST_SUPPORT. It's also not really clear why you
need to check for the system domain here.


I might be missing but I wouldn't expect the domain to have vPCI enabled 
if CONFIG_HAVE_VPCI_GUEST_SUPPORT=n. So why can't this simply be:


if ( !has_vcpi(d) )
{
   ...
}

Cheers,

--
Julien Grall

[PATCH for-4.18 0/3] CHANGELOG: More 4.18 content

[Andrew Cooper]

Andrew Cooper (3):
  CHANGELOG: Reformat
  CHANGELOG: More 4.18 content
  CHANGELOG: Keep unstable section

 CHANGELOG.md | 44 
 1 file changed, 28 insertions(+), 16 deletions(-)


base-commit: 9659b2a6d73b14620e187f9c626a09323853c459
-- 
2.30.2

[PATCH for-4.18 2/3] CHANGELOG: More 4.18 content

[Andrew Cooper]

Signed-off-by: Andrew Cooper 
---
CC: George Dunlap 
CC: Jan Beulich 
CC: Stefano Stabellini 
CC: Wei Liu 
CC: Julien Grall 
CC: Roger Pau Monné 
CC: Henry Wang 
---
 CHANGELOG.md | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index edc0d69898ed..a827054cf27d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -18,10 +18,17 @@ The format is based on [Keep a 
Changelog](https://keepachangelog.com/en/1.0.0/)
 
 ### Added
  - On x86:
+   - On all Intel systems, MSR_ARCH_CAPS is now visible in guests, and
+ controllable from the VM's config file.  For CPUs from ~2019 onwards,
+ this allows guest kernels to see details about hardware fixes for
+ speculative mitigations.  (Backported as XSA-435 to older releases).
- xl/libxl can customize SMBIOS strings for HVM guests.
- Support for enforcing system-wide operation in Data Operand Independent
  Timing Mode.
- Add Intel Hardware P-States (HWP) cpufreq driver.
+   - Support for features new in AMD Genoa CPUs:
+ - CPUID_USER_DIS (CPUID Faulting) used by Xen to control PV guest's view
+   of CPUID data.
- Support for features new in Intel Sapphire Rapids CPUs:
  - PKS (Protection Key Supervisor) available to HVM/PVH guests.
  - VM-Notify used by Xen to mitigate certain micro-architectural pipeline
-- 
2.30.2

[PATCH for-4.18 1/3] CHANGELOG: Reformat

[Andrew Cooper]

Collect all x86 and ARM changes together instead of having them scattered.
Tweak grammar as necessary.

No change.

Signed-off-by: Andrew Cooper 
---
CC: George Dunlap 
CC: Jan Beulich 
CC: Stefano Stabellini 
CC: Wei Liu 
CC: Julien Grall 
CC: Roger Pau Monné 
CC: Henry Wang 
---
 CHANGELOG.md | 35 +++
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3ca796969990..edc0d69898ed 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -17,24 +17,27 @@ The format is based on [Keep a 
Changelog](https://keepachangelog.com/en/1.0.0/)
Hotplug" for clarity
 
 ### Added
- - On x86, support for features new in Intel Sapphire Rapids CPUs:
-   - PKS (Protection Key Supervisor) available to HVM/PVH guests.
-   - VM-Notify used by Xen to mitigate certain micro-architectural pipeline
- livelocks, instead of crashing the entire server.
-   - Bus-lock detection, used by Xen to mitigate (by rate-limiting) the system
- wide impact of a guest misusing atomic instructions.
- - xl/libxl can customize SMBIOS strings for HVM guests.
- - Add support for AVX512-FP16 on x86.
- - On Arm, Xen supports guests running SVE/SVE2 instructions. (Tech Preview)
- - On Arm, add suport for Firmware Framework for Arm A-profile (FF-A) Mediator
-   (Tech Preview)
- - Add Intel Hardware P-States (HWP) cpufreq driver.
- - On Arm, experimental support for dynamic addition/removal of Xen device tree
-   nodes using a device tree overlay binary (.dtbo).
+ - On x86:
+   - xl/libxl can customize SMBIOS strings for HVM guests.
+   - Support for enforcing system-wide operation in Data Operand Independent
+ Timing Mode.
+   - Add Intel Hardware P-States (HWP) cpufreq driver.
+   - Support for features new in Intel Sapphire Rapids CPUs:
+ - PKS (Protection Key Supervisor) available to HVM/PVH guests.
+ - VM-Notify used by Xen to mitigate certain micro-architectural pipeline
+   livelocks, instead of crashing the entire server.
+ - Bus-lock detection, used by Xen to mitigate (by rate-limiting) the
+   system wide impact of a guest misusing atomic instructions.
+   - Support for features new in Intel Granite Rapids CPUs:
+ - AVX512-FP16.
+ - On Arm:
+   - Xen supports guests running SVE/SVE2 instructions. (Tech Preview)
+   - Add suport for Firmware Framework for Arm A-profile (FF-A) Mediator (Tech
+ Preview)
+   - Experimental support for dynamic addition/removal of Xen device tree
+ nodes using a device tree overlay binary (.dtbo).
  - Introduce two new hypercalls to map the vCPU runstate and time areas by
physical rather than linear/virtual addresses.
- - On x86, support for enforcing system-wide operation in Data Operand
-   Independent Timing Mode.
  - The project has now officially adopted 6 directives and 65 rules of MISRA-C.
 
 ### Removed
-- 
2.30.2

[PATCH for-4.19 3/3] CHANGELOG: Keep unstable section

[Andrew Cooper]

Signed-off-by: Andrew Cooper 
---
CC: George Dunlap 
CC: Jan Beulich 
CC: Stefano Stabellini 
CC: Wei Liu 
CC: Julien Grall 
CC: Roger Pau Monné 
CC: Henry Wang 
---
 CHANGELOG.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index a827054cf27d..cf0c9c3f8cb9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,8 @@ Notable changes to Xen will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 
+## [unstable 
UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=staging)
 - TBD
+
 ## 
[4.18.0](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.18.0)
 - 2023-XX-XX
 
 ### Changed
-- 
2.30.2

[RFC PATCH v2 1/8] cppcheck: rework exclusion_file_list.py code

[Luca Fancellu]

Rework the exclusion_file_list.py code to have the function
load_exclusion_file_list() detached from the xen-analysis.py tool,
in a way so that other modules can use the function.
The xen-analysis tool and in particular its module cppcheck_analysis.py
will use a new function cppcheck_exclusion_file_list().

No functional changes are intended.

Signed-off-by: Luca Fancellu 
Reviewed-by: Stefano Stabellini 
---
 xen/scripts/xen_analysis/cppcheck_analysis.py |  6 ++--
 .../xen_analysis/exclusion_file_list.py   | 31 ++-
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/xen/scripts/xen_analysis/cppcheck_analysis.py 
b/xen/scripts/xen_analysis/cppcheck_analysis.py
index 8dc45e653b79..e54848aa5339 100644
--- a/xen/scripts/xen_analysis/cppcheck_analysis.py
+++ b/xen/scripts/xen_analysis/cppcheck_analysis.py
@@ -2,7 +2,8 @@
 
 import os, re, shutil
 from . import settings, utils, cppcheck_report_utils, exclusion_file_list
-from .exclusion_file_list import ExclusionFileListError
+from .exclusion_file_list import (ExclusionFileListError,
+  cppcheck_exclusion_file_list)
 
 class GetMakeVarsPhaseError(Exception):
 pass
@@ -54,8 +55,7 @@ def __generate_suppression_list(out_file):
 try:
 exclusion_file = \
 "{}/docs/misra/exclude-list.json".format(settings.repo_dir)
-exclusion_list = \
-
exclusion_file_list.load_exclusion_file_list(exclusion_file)
+exclusion_list = cppcheck_exclusion_file_list(exclusion_file)
 except ExclusionFileListError as e:
 raise CppcheckDepsPhaseError(
 "Issue with reading file {}: {}".format(exclusion_file, e)
diff --git a/xen/scripts/xen_analysis/exclusion_file_list.py 
b/xen/scripts/xen_analysis/exclusion_file_list.py
index 871e480586bb..79ebd34f55ec 100644
--- a/xen/scripts/xen_analysis/exclusion_file_list.py
+++ b/xen/scripts/xen_analysis/exclusion_file_list.py
@@ -7,16 +7,24 @@ class ExclusionFileListError(Exception):
 pass
 
 
-def __cppcheck_path_exclude_syntax(path):
-# Prepending * to the relative path to match every path where the Xen
-# codebase could be
-path = "*" + path
+def cppcheck_exclusion_file_list(input_file):
+ret = []
+excl_list = load_exclusion_file_list(input_file)
+
+for entry in excl_list:
+# Prepending * to the relative path to match every path where the Xen
+# codebase could be
+ret.append("*" + entry[0])
 
-return path
+return ret
 
 
-# Reads the exclusion file list and returns a list of relative path to be
-# excluded.
+# Reads the exclusion file list and returns an array containing a set where the
+# first entry is what was listed in the exclusion list file, and the second
+# entry is the absolute path of the first entry.
+# If the first entry contained a wildcard '*', the second entry will have an
+# array of the solved absolute path for that entry.
+# Returns [('path',[path,path,...]), ('path',[path,path,...]), ...]
 def load_exclusion_file_list(input_file):
 ret = []
 try:
@@ -58,13 +66,6 @@ def load_exclusion_file_list(input_file):
 .format(path, filepath_object)
 )
 
-if settings.analysis_tool == "cppcheck":
-path = __cppcheck_path_exclude_syntax(path)
-else:
-raise ExclusionFileListError(
-"Unimplemented for {}!".format(settings.analysis_tool)
-)
-
-ret.append(path)
+ret.append((path, check_path))
 
 return ret
-- 
2.34.1

[RFC PATCH v2 3/8] [WIP]xen/scripts: add codestyle.py script

[Luca Fancellu]

This script finds every .c and .h file in the xen hypervisor
codebase, takes the exclusion list from docs/misra, removes the
file excluded from the list and for the remaining files is
calling clang-format on them.

TBD: write it better

Signed-off-by: Luca Fancellu 
---
 xen/scripts/codestyle.py | 265 +++
 1 file changed, 265 insertions(+)
 create mode 100755 xen/scripts/codestyle.py

diff --git a/xen/scripts/codestyle.py b/xen/scripts/codestyle.py
new file mode 100755
index ..ab3df66fc2e2
--- /dev/null
+++ b/xen/scripts/codestyle.py
@@ -0,0 +1,265 @@
+#!/usr/bin/env python3
+
+import glob
+import os
+import re
+import sys
+from xen_analysis.settings import xen_dir, repo_dir
+from xen_analysis import utils
+from xen_analysis import exclusion_file_list
+from xen_analysis.exclusion_file_list import ExclusionFileListError
+
+# The Xen codestyle states that labels needs to be indented by at least one
+# blank, but clang-format doesn't have an option for that and if it encounters
+# a label indented by blank characters that are less than its indent
+# configuration, it removes the indentation.
+# So this action is meant as post step and checks every label syntax match and
+# it adds one blank before the label
+def action_fix_label_indent(filename, file_lines):
+label_rgx = re.compile('^[a-zA-Z_][a-zA-Z0-9_]*\s*:.*$')
+
+for i in range(0, len(file_lines)):
+if label_rgx.match(file_lines[i]):
+file_lines[i] = ' ' + file_lines[i]
+
+return file_lines
+
+
+# clang-format most of the time breaks the content of asm(...) instructions,
+# so with this function, we protect all the asm sections using the special
+# in code comments that tells clang-format to don't touch the block.
+# asm(...) instruction could be also inside macros, so in that case we protect
+# the entire macro that is enclosing the instruction, in the un-protect stage
+# however, we need to do clang-format's job at least on the tab-space 
conversion
+# and to put the backslash on the right side.
+def action_protect_asm(filename, file_lines, protect):
+opening_asm = False
+cf_off_comment = '/* clang-format off */'
+cf_on_comment = '/* clang-format on */'
+asm_stx = '(?:asm|__asm__)(?:\s(?:volatile|__volatile__))?\s?\('
+asm_stx_close = ');'
+
+if protect:
+# Look for closing parenthesis with semicolon ');'
+closing_asm_rgx_rule = rf'^.*{re.escape(asm_stx_close)}.*$'
+# Look for opening asm syntax
+opening_asm_rgx_rule = rf'^\s*{asm_stx}.*$'
+macro_start_rgx_rule = r'^\s?#\s?define.*\\$'
+opening_asm_find = rf'({asm_stx})'
+opening_asm_replace = cf_off_comment + r'\1'
+opening_def_find = r'#\s?define'
+opening_def_replace = f'{cf_off_comment}#define'
+closing_asm_find= re.escape(asm_stx_close)
+closing_asm_replace = asm_stx_close + cf_on_comment
+closing_def_find= '\n'
+closing_def_replace = cf_on_comment + '\n'
+else:
+# Look for closing parenthesis with semicolon ');' and with the
+# special clang-format comment
+closing_asm_rgx_rule = \
+rf'^.*{re.escape(asm_stx_close)}.*{re.escape(cf_on_comment)}.*$'
+# Look for opening asm syntax preceded by the special clang-format
+# comment, the comment is optional to generalise the algorithm to
+# un-protect asm outside and inside macros. The case outside is easy
+# because we will find '/* clang-format off */asm', instead the case
+# inside is more tricky and we are going to find only 'asm' and then
+# go backwards until we find '/* clang-format off */#define'
+opening_asm_rgx_rule = \
+rf'^\s*({re.escape(cf_off_comment)})?{asm_stx}.*$'
+# Look for the define just before the asm invocation, here we look for
+# '/* clang-format off */#define' or '#define', this is to handle a 
rare
+# corner case where an asm invocation is inside a macro, but was not
+# protected in the 'protect stage', because it was on the same line
+# of the define and was not ending with backslash but it was exceeding
+# line width so clang-format formatted anyway.
+# It's safe because we won't change code that has no clang-format
+# comments, but at least the tool won't complain
+macro_start_rgx_rule = \
+rf'^\s?(?:{re.escape(cf_off_comment)})?#\s?define.*\\$'
+opening_asm_find = rf'({re.escape(cf_off_comment)}({asm_stx}))'
+opening_asm_replace = r'\2'
+opening_def_find = rf'(?:{re.escape(cf_off_comment)})?#\s?define'
+opening_def_replace = '#define'
+closing_asm_find \
+= rf'{re.escape(asm_stx_close)}.*{re.escape(cf_on_comment)}'
+closing_asm_replace = asm_stx_close
+closing_def_find= cf_on_comment + '\n'
+closing_def_replace = '\n'
+
+opening_asm_rgx = re.

[RFC PATCH v2 5/8] [WIP]codestyle.py: Protect generic piece of code

[Luca Fancellu]

Add a way to protect generic piece of code from being formatted.

Use the exclude-list to pass also a structure to the scripts,
that structure will be used from the codestyle.py script to
understand which piece of code of which file needs to be left
with the original format.

Update exclude-list.rst documentation.

Signed-off-by: Luca Fancellu 
---
 docs/misra/exclude-list.rst   |  6 +-
 xen/scripts/codestyle.py  | 96 ---
 .../xen_analysis/exclusion_file_list.py   | 15 ++-
 3 files changed, 76 insertions(+), 41 deletions(-)

diff --git a/docs/misra/exclude-list.rst b/docs/misra/exclude-list.rst
index ade314100663..946f3793aad7 100644
--- a/docs/misra/exclude-list.rst
+++ b/docs/misra/exclude-list.rst
@@ -25,7 +25,8 @@ Here is an example of the exclude-list.json file::
 |{
 |"rel_path": "relative/path/from/xen/folder/*",
 |"comment": "This folder is a library",
-|"checkers": "xen-analysis some-checker"
+|"checkers": "xen-analysis some-checker",
+|"xen-analysis": {...}
 |},
 |{
 |"rel_path": "relative/path/from/xen/mem*.c",
@@ -48,6 +49,9 @@ Here is an explanation of the fields inside an object of the 
"content" array:
   and static analysis scan. (Implemented only for Cppcheck tool)
 - codestyle: the codestyle.py script exclude this entry from the formatting
   tool.
+ - : an optional parameter to pass a configuration to the checker 
about
+   this entry. The parameter to be specified is one of the value listed for the
+   "checkers" value.
 
 To ease the review and the modifications of the entries, they shall be listed 
in
 alphabetical order referring to the rel_path field.
diff --git a/xen/scripts/codestyle.py b/xen/scripts/codestyle.py
index ab3df66fc2e2..92482d586f7a 100755
--- a/xen/scripts/codestyle.py
+++ b/xen/scripts/codestyle.py
@@ -36,36 +36,36 @@ def action_protect_asm(filename, file_lines, protect):
 opening_asm = False
 cf_off_comment = '/* clang-format off */'
 cf_on_comment = '/* clang-format on */'
-asm_stx = '(?:asm|__asm__)(?:\s(?:volatile|__volatile__))?\s?\('
-asm_stx_close = ');'
+
+config = filename[1]["protect"]
 
 if protect:
 # Look for closing parenthesis with semicolon ');'
-closing_asm_rgx_rule = rf'^.*{re.escape(asm_stx_close)}.*$'
+closing_asm_rgx_rule = lambda cl_stx: rf'^.*{re.escape(cl_stx)}.*$'
 # Look for opening asm syntax
-opening_asm_rgx_rule = rf'^\s*{asm_stx}.*$'
+opening_asm_rgx_rule = lambda op_stx: rf'^\s*{op_stx}.*$'
 macro_start_rgx_rule = r'^\s?#\s?define.*\\$'
-opening_asm_find = rf'({asm_stx})'
+opening_asm_find = lambda op_stx: rf'({op_stx})'
 opening_asm_replace = cf_off_comment + r'\1'
 opening_def_find = r'#\s?define'
 opening_def_replace = f'{cf_off_comment}#define'
-closing_asm_find= re.escape(asm_stx_close)
-closing_asm_replace = asm_stx_close + cf_on_comment
+closing_asm_find= lambda cl_stx: re.escape(cl_stx)
+closing_asm_replace = lambda cl_stx: cl_stx + cf_on_comment
 closing_def_find= '\n'
 closing_def_replace = cf_on_comment + '\n'
 else:
 # Look for closing parenthesis with semicolon ');' and with the
 # special clang-format comment
-closing_asm_rgx_rule = \
-rf'^.*{re.escape(asm_stx_close)}.*{re.escape(cf_on_comment)}.*$'
+closing_asm_rgx_rule = lambda cl_stx: \
+rf'^.*{re.escape(cl_stx)}.*{re.escape(cf_on_comment)}.*$'
 # Look for opening asm syntax preceded by the special clang-format
 # comment, the comment is optional to generalise the algorithm to
 # un-protect asm outside and inside macros. The case outside is easy
 # because we will find '/* clang-format off */asm', instead the case
 # inside is more tricky and we are going to find only 'asm' and then
 # go backwards until we find '/* clang-format off */#define'
-opening_asm_rgx_rule = \
-rf'^\s*({re.escape(cf_off_comment)})?{asm_stx}.*$'
+opening_asm_rgx_rule = lambda op_stx: \
+rf'^\s*({re.escape(cf_off_comment)})?{op_stx}.*$'
 # Look for the define just before the asm invocation, here we look for
 # '/* clang-format off */#define' or '#define', this is to handle a 
rare
 # corner case where an asm invocation is inside a macro, but was not
@@ -76,24 +76,27 @@ def action_protect_asm(filename, file_lines, protect):
 # comments, but at least the tool won't complain
 macro_start_rgx_rule = \
 rf'^\s?(?:{re.escape(cf_off_comment)})?#\s?define.*\\$'
-opening_asm_find = rf'({re.escape(cf_off_comment)}({asm_stx}))'
+opening_asm_find = \
+lambda op_stx: rf'({re.escape(cf_off_comment)}({op_stx}))'
 opening_asm_replace = r'

[RFC PATCH v2 7/8] xen: Add clang-format configuration

[Luca Fancellu]

Add a clang format configuration for the Xen Hypervisor.

Signed-off-by: Luca Fancellu 
---
 xen/.clang-format | 693 ++
 1 file changed, 693 insertions(+)
 create mode 100644 xen/.clang-format

diff --git a/xen/.clang-format b/xen/.clang-format
new file mode 100644
index ..7880709fe1fd
--- /dev/null
+++ b/xen/.clang-format
@@ -0,0 +1,693 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# clang-format configuration file. Intended for clang-format >= 15.
+#
+# For more information, see:
+#
+#   Documentation/process/clang-format.rst
+#   https://clang.llvm.org/docs/ClangFormat.html
+#   https://clang.llvm.org/docs/ClangFormatStyleOptions.html
+#
+---
+
+# [not specified]
+# Align function parameter that goes into a new line, under the open bracket
+# (supported in clang-format 3.8)
+AlignAfterOpenBracket: Align
+
+# [not specified]
+# Align array of struct's elements by column and justify
+# struct test demo[] =
+# {
+# {56, 23,"hello"},
+# {-1, 93463, "world"},
+# {7,  5, "!!"   }
+# };
+# (supported in clang-format 13)
+AlignArrayOfStructures: Left
+
+# [not specified]
+# Align consecutive assignments (supported in clang-format 3.8)
+AlignConsecutiveAssignments:
+  Enabled: true
+  AcrossEmptyLines: true
+  AcrossComments: false
+
+# [not specified]
+# Do not align consecutive bit fields (supported in clang-format 11)
+AlignConsecutiveBitFields: None
+
+# [not specified]
+# Do not align values of consecutive declarations
+# (supported in clang-format 3.8)
+AlignConsecutiveDeclarations: None
+
+# [not specified]
+# Align values of consecutive macros (supported in clang-format 9)
+AlignConsecutiveMacros:
+  Enabled: true
+  AcrossEmptyLines: true
+  AcrossComments: true
+
+# [not specified]
+# Align escaped newlines to the right (supported in clang-format 5)
+AlignEscapedNewlines: Right
+
+# [not specified]
+# Aligns operands of a single expression that needs to be split over multiple
+# lines (supported in clang-format 3.5)
+AlignOperands: Align
+
+# Do not align trailing consecutive comments (It helps to make clang-format
+# reproduce the same output when it runs on an already formatted file)
+# (supported in clang-format 3.7)
+AlignTrailingComments: false
+
+# [not specified]
+# Do not put all function call arguments on a new line, try to have at least
+# the first one close to the opening parenthesis (supported in clang-format 9)
+AllowAllArgumentsOnNextLine: false
+
+# [not specified]
+# Do not put all function declaration parameters on a new line, try to have at
+# least the first one close to the opening parenthesis
+# (supported in clang-format 3.3)
+AllowAllParametersOfDeclarationOnNextLine: false
+
+# Bracing condition needs to be respected even if the line is so short that the
+# final block brace can stay on a single line
+# (supported in clang-format 3.5)
+AllowShortBlocksOnASingleLine: Never
+
+# (supported in clang-format 3.6)
+AllowShortCaseLabelsOnASingleLine: false
+
+# (supported in clang-format 3.5)
+AllowShortFunctionsOnASingleLine: None
+
+# (supported in clang-format 3.3)
+AllowShortIfStatementsOnASingleLine: Never
+
+# (supported in clang-format 3.7)
+AllowShortLoopsOnASingleLine: false
+
+# [not specified]
+# Do not add a break after the definition return type
+# (supported in clang-format 3.8)
+AlwaysBreakAfterReturnType: None
+
+# [not specified]
+# There is no need to use a break after an assigment to a multiline string
+# (supported in clang-format 3.4)
+AlwaysBreakBeforeMultilineStrings: false
+
+# (supported in clang-format 3.4)
+AlwaysBreakTemplateDeclarations: false
+
+# Specify Xen's macro attributes (supported in clang-format 12)
+AttributeMacros:
+  - '__init'
+  - '__exit'
+  - '__initdata'
+  - '__initconst'
+  - '__initconstrel'
+  - '__initdata_cf_clobber'
+  - '__initconst_cf_clobber'
+  - '__hwdom_init'
+  - '__hwdom_initdata'
+  - '__maybe_unused'
+  - '__packed'
+  - '__stdcall'
+  - '__vfp_aligned'
+  - '__alt_call_maybe_initdata'
+  - '__cacheline_aligned'
+  - '__ro_after_init'
+  - 'always_inline'
+  - 'noinline'
+  - 'noreturn'
+  - '__weak'
+  - '__inline__'
+  - '__attribute_const__'
+  - '__transparent__'
+  - '__used'
+  - '__must_check'
+  - '__kprobes'
+
+# [not specified]
+# Try always to pack function call arguments on the same line before breaking
+# (supported in clang-format 3.7)
+BinPackArguments: true
+
+# [not specified]
+# Try always to pack function declaration parameters on the same line before
+# breaking (supported in clang-format 3.7)
+BinPackParameters: true
+
+# [not specified]
+# Do not add a spaces on bitfield 'unsigned bf:2;'
+# (supported in clang-format 12)
+BitFieldColonSpacing: None
+
+# Xen's coding style does not follow clang-format already available profiles 
for
+# breaking before braces, so set it to Custom and specify each case separately
+# (supported in clang-format 3.8)
+BraceWrapping:
+  # Braces ('{' and '}') are usually placed on a line of their 

[RFC PATCH v2 6/8] [WIP]x86/exclude-list: protect mm_type_tbl in mtrr from being formatted

[Luca Fancellu]

The array mm_type_tbl initialization is formatted in a way that
the formatting tool can't keep, so disable the formatting on that
array initialization.

Signed-off-by: Luca Fancellu 
---
 docs/misra/exclude-list.json | 13 +
 1 file changed, 13 insertions(+)

diff --git a/docs/misra/exclude-list.json b/docs/misra/exclude-list.json
index d48dcf3ac971..b8976bc671a4 100644
--- a/docs/misra/exclude-list.json
+++ b/docs/misra/exclude-list.json
@@ -99,6 +99,19 @@
 "rel_path": "arch/x86/cpu/mwait-idle.c",
 "comment": "Imported from Linux, ignore for now"
 },
+{
+"rel_path": "arch/x86/hvm/mtrr.c",
+"comment": "Contains structure formatted in a particular way",
+"checkers": "codestyle",
+"codestyle": {
+"protect": [
+{
+"syntax_opening": "static const uint8_t mm_type_tbl",
+"syntax_closing": "};"
+}
+]
+}
+},
 {
 "rel_path": "arch/x86/include/asm/alternative-asm.h",
 "comment": "Includes mostly assembly macro and it's meant to be 
included only in assembly code",
-- 
2.34.1

[RFC PATCH v2 4/8] exclude-list: add entries to the excluded list for codestyle

[Luca Fancellu]

Add entries to the exclusion list, so that they can be excluded
from the formatting tool.

Signed-off-by: Luca Fancellu 
---
 docs/misra/exclude-list.json | 100 +++
 docs/misra/exclude-list.rst  |   2 +
 2 files changed, 102 insertions(+)

diff --git a/docs/misra/exclude-list.json b/docs/misra/exclude-list.json
index 575ed22a7f67..d48dcf3ac971 100644
--- a/docs/misra/exclude-list.json
+++ b/docs/misra/exclude-list.json
@@ -1,6 +1,11 @@
 {
 "version": "1.0",
 "content": [
+{
+"rel_path": "arch/arm/arm32/lib/assembler.h",
+"comment": "Includes mostly assembly macro and it's meant to be 
included only in assembly code",
+"checkers": "codestyle"
+},
 {
 "rel_path": "arch/arm/arm64/cpufeature.c",
 "comment": "Imported from Linux, ignore for now"
@@ -13,6 +18,31 @@
 "rel_path": "arch/arm/arm64/lib/find_next_bit.c",
 "comment": "Imported from Linux, ignore for now"
 },
+{
+"rel_path": "arch/arm/include/asm/arm32/macros.h",
+"comment": "Includes only assembly macro",
+"checkers": "codestyle"
+},
+{
+"rel_path": "arch/arm/include/asm/arm64/macros.h",
+"comment": "Includes only assembly macro",
+"checkers": "codestyle"
+},
+{
+"rel_path": "arch/arm/include/asm/alternative.h",
+"comment": "Imported from Linux, ignore for now",
+"checkers": "codestyle"
+},
+{
+"rel_path": "arch/arm/include/asm/asm_defns.h",
+"comment": "Includes mostly assembly macro",
+"checkers": "codestyle"
+},
+{
+"rel_path": "arch/arm/include/asm/macros.h",
+"comment": "Includes mostly assembly macro and it's meant to be 
included only in assembly code",
+"checkers": "codestyle"
+},
 {
 "rel_path": "arch/x86/acpi/boot.c",
 "comment": "Imported from Linux, ignore for now"
@@ -69,6 +99,36 @@
 "rel_path": "arch/x86/cpu/mwait-idle.c",
 "comment": "Imported from Linux, ignore for now"
 },
+{
+"rel_path": "arch/x86/include/asm/alternative-asm.h",
+"comment": "Includes mostly assembly macro and it's meant to be 
included only in assembly code",
+"checkers": "codestyle"
+},
+{
+"rel_path": "arch/x86/include/asm/asm_defns.h",
+"comment": "Includes mostly assembly macro",
+"checkers": "codestyle"
+},
+{
+"rel_path": "arch/x86/include/asm/asm-defns.h",
+"comment": "Includes mostly assembly macro",
+"checkers": "codestyle"
+},
+{
+"rel_path": "arch/x86/include/asm/bug.h",
+"comment": "Includes mostly assembly macro",
+"checkers": "codestyle"
+},
+{
+"rel_path": "arch/x86/include/asm/mpspec.h",
+"comment": "Imported from Linux, also designated initializers 
ranges are not handled very well by clang-format, ignore for now",
+"checkers": "codestyle"
+},
+{
+"rel_path": "arch/x86/include/asm/spec_ctrl_asm.h",
+"comment": "Includes mostly assembly macro",
+"checkers": "codestyle"
+},
 {
 "rel_path": "arch/x86/delay.c",
 "comment": "Imported from Linux, ignore for now"
@@ -189,10 +249,45 @@
 "rel_path": "include/acpi/acpixf.h",
 "comment": "Imported from Linux, ignore for now"
 },
+{
+"rel_path": "include/efi/*.h",
+"comment": "Imported from gnu-efi-3.0k, prefer their formatting",
+"checkers": "codestyle"
+},
+{
+"rel_path": "include/public/arch-x86/cpufeatureset.h",
+"comment": "This file contains some inputs for the gen-cpuid.py 
script, leave it out",
+"checkers": "codestyle"
+},
+{
+"rel_path": "include/public/*",
+"comment": "Public headers are quite sensitive to format tools",
+"checkers": "codestyle"
+},
 {
 "rel_path": "include/xen/acpi.h",
 "comment": "Imported from Linux, ignore for now"
 },
+{
+"rel_path": "include/xen/cper.h",
+"comment": "Header does not follow Xen coding style",
+"checkers": "codestyle"
+},
+{
+"rel_path": "include/xen/nodemask.h",
+"comment": "Imported from Linux, also designated initializers 
ranges are not handled by clang-format, ignore for now",
+"checkers": "codestyle"
+},
+{
+"rel_path": "include/xen/xen.lds.h",
+"comment": "This file contains only macros used i

[RFC PATCH v2 8/8] feedback from the community

[Luca Fancellu]

Signed-off-by: Luca Fancellu 
---
 xen/.clang-format | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/.clang-format b/xen/.clang-format
index 7880709fe1fd..bfc1d104af84 100644
--- a/xen/.clang-format
+++ b/xen/.clang-format
@@ -29,8 +29,8 @@ AlignArrayOfStructures: Left
 # [not specified]
 # Align consecutive assignments (supported in clang-format 3.8)
 AlignConsecutiveAssignments:
-  Enabled: true
-  AcrossEmptyLines: true
+  Enabled: false
+  AcrossEmptyLines: false
   AcrossComments: false
 
 # [not specified]
@@ -46,8 +46,8 @@ AlignConsecutiveDeclarations: None
 # Align values of consecutive macros (supported in clang-format 9)
 AlignConsecutiveMacros:
   Enabled: true
-  AcrossEmptyLines: true
-  AcrossComments: true
+  AcrossEmptyLines: false
+  AcrossComments: false
 
 # [not specified]
 # Align escaped newlines to the right (supported in clang-format 5)
-- 
2.34.1

[RFC PATCH v2 2/8] exclude-list: generalise exclude-list

[Luca Fancellu]

Currently exclude-list.json is used by the xen-analysis tool to
remove from the report (cppcheck for now) violations from the
files listed in it, however that list can be used by different
users that might want to exclude some of the files from their
computation for many reason.

So add a new field that can be part of each entry to link
the tool supposed to consider that exclusion.

Update exclusion_file_list.py to implement the logic and update
the documentation to reflect this change.

Signed-off-by: Luca Fancellu 
Reviewed-by: Stefano Stabellini 
---
 docs/misra/exclude-list.rst   | 31 ---
 .../xen_analysis/exclusion_file_list.py   | 16 --
 2 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/docs/misra/exclude-list.rst b/docs/misra/exclude-list.rst
index c97431a86120..42dbceb82523 100644
--- a/docs/misra/exclude-list.rst
+++ b/docs/misra/exclude-list.rst
@@ -1,17 +1,16 @@
 .. SPDX-License-Identifier: CC-BY-4.0
 
-Exclude file list for xen-analysis script
-=
+Exclude file list for xen scripts
+=
 
-The code analysis is performed on the Xen codebase for both MISRA
-checkers and static analysis checkers, there are some files however that
-needs to be removed from the findings report for various reasons (e.g.
-they are imported from external sources, they generate too many false
-positive results, etc.).
+Different Xen scripts can perform operations on the codebase to check its
+compliance for a set of rules, however Xen contains some files that are taken
+from other projects (e.g. linux) and they can't be updated to ease backporting
+fixes from their source, for this reason the file docs/misra/exclude-list.json
+is kept as a source of all these files that are external to the Xen project.
 
-For this reason the file docs/misra/exclude-list.json is used to exclude every
-entry listed in that file from the final report.
-Currently only the cppcheck analysis will use this file.
+Every entry of the file can be linked to different checkers, so that this list
+can be used by multiple scripts selecting only the required entries.
 
 Here is an example of the exclude-list.json file::
 
@@ -20,11 +19,13 @@ Here is an example of the exclude-list.json file::
 |"content": [
 |{
 |"rel_path": "relative/path/from/xen/file",
-|"comment": "This file is originated from ..."
+|"comment": "This file is originated from ...",
+|"checkers": "xen-analysis"
 |},
 |{
 |"rel_path": "relative/path/from/xen/folder/*",
-|"comment": "This folder is a library"
+|"comment": "This folder is a library",
+|"checkers": "xen-analysis some-checker"
 |},
 |{
 |"rel_path": "relative/path/from/xen/mem*.c",
@@ -39,6 +40,12 @@ Here is an explanation of the fields inside an object of the 
"content" array:
match more than one file/folder at the time. This field is mandatory.
  - comment: an optional comment to explain why the file is removed from the
analysis.
+ - checkers: an optional list of checkers that will exclude this entries from
+   their results. This field is optional and when not specified, it means every
+   checker will use that entry.
+   Current implemented values for this field are:
+- xen-analysis: the xen-analysis.py script exclude this entry for both 
MISRA
+  and static analysis scan. (Implemented only for Cppcheck tool)
 
 To ease the review and the modifications of the entries, they shall be listed 
in
 alphabetical order referring to the rel_path field.
diff --git a/xen/scripts/xen_analysis/exclusion_file_list.py 
b/xen/scripts/xen_analysis/exclusion_file_list.py
index 79ebd34f55ec..8b10665a19e8 100644
--- a/xen/scripts/xen_analysis/exclusion_file_list.py
+++ b/xen/scripts/xen_analysis/exclusion_file_list.py
@@ -9,7 +9,7 @@ class ExclusionFileListError(Exception):
 
 def cppcheck_exclusion_file_list(input_file):
 ret = []
-excl_list = load_exclusion_file_list(input_file)
+excl_list = load_exclusion_file_list(input_file, "xen-analysis")
 
 for entry in excl_list:
 # Prepending * to the relative path to match every path where the Xen
@@ -25,7 +25,7 @@ def cppcheck_exclusion_file_list(input_file):
 # If the first entry contained a wildcard '*', the second entry will have an
 # array of the solved absolute path for that entry.
 # Returns [('path',[path,path,...]), ('path',[path,path,...]), ...]
-def load_exclusion_file_list(input_file):
+def load_exclusion_file_list(input_file, checker=""):
 ret = []
 try:
 with open(input_file, "rt") as handle:
@@ -51,6 +51,18 @@ def load_exclusion_file_list(input_file):
 raise ExclusionFileListError(
 "Malformed JSON entry: rel_path field not found!"
 )
+# Check the checker field
+try:
+entry_checker

[RFC PATCH v2 0/8] clang-format for Xen

[Luca Fancellu]

## Introduction 

In this serie, I would like to get feedbacks on the output generated by the
configuration of clang-format, unfortunately we can't use only clang-format, but
we need to call it using a wrapper, because we need the information of what
files need to be excluded from the tool.

Another reason is that clang-format has some limitation when formatting asm()
instruction and most of the time it format them in a very ugly way or it breaks
the code for example removing spaces that were there for a reason (I don't think
it's a tool to format asm), so in the wrapper script we protect all asm()
invocation or macros where there are asm() invocation with in-code comments that
stops clang-format to act on that section:

/* clang-format off */section/* clang-format on */

I've read the past threads about the brave people who dared to try to introduce
clang-format for the xen codebase, some of them from 5 years ago, two points
were clear: 1) goto label needs to be indented and 2) do-while loops have the
braket in the same line.
While point 1) was quite a blocker, it seemd to me that point 2) was less
controversial to be changed in the Xen codestyle, so the current wrapper script
handles only the point 1 (which is easy), the point 2 can be more tricky to
handle.

## The clang-format configuration ##

In my clang-format configuration I've taken inspiration from EPAM's work, then
from the configuration in Linux and finally from the clang-format manual, to try
to produce a comprehensive configuration.

Every configuration parameter has on top a comment with the description and
when it was supported, finally I've added also a [not specified] if that
behavior is not clearly specified in the Xen coding style, I've done that so
we could discuss about adding more specification in our CODING_STYLE.
Every comment can be stripped out in the final release of the file, but I think
that now they are useful for the discussion.

The minimum clang-format version for the file is 15, my ubuntu 22.04 comes with
it, we can reason if it's too high, or if we could also use the latest version
maybe shipped inside a docker image.

For every [not specified] behavior, I've tried to guess it from the codebase,
I've seen that also in that case it's not easy as there is (sometimes) low
consistency between modules, so we can discuss on every configurable.

Worth to mention, the public header are all excluded from the format tool,
because formatting them breaks the build on X86, because there are scripts for
auto-generation that don't handle the formatted headers, I didn't investigate
on it, maybe it can be added as technical debt.

So I've tried building arm32, arm64 and x86_64 with the formatted output and
they build, I've used Yocto for that.

## How to try it? ##

So how to generate everything? Just invoke the codestyle.py script without
parameter and it will format every .c and .h file in the hypervisor codebase.

./xen/scripts/codestyle.py

Optionally you can also pass one or more relative path from the folder you are
invoking the script and it will format only them.

## What I expect from this RFC #

I expect feedback on the output, some agreement on what configuration to use,
and I expect to find possible blocker before working seriously on this serie,
because if there are outstanding blockers on the adoption of the tool and we
can't reach an agreement, I won't spend further time on it.

v2 changes: I've introduced a way to suppress the code formatter on certain
piece of code, currently it is implemented inside the exclude-list, but we could
do in a different list, it's not very important at this stage and I did in this
way just to save time.

Here is a link to the current serie output:
https://gitlab.com/luca.fancellu/xen-clang/-/commit/8938bf2196be66b05693a48752ebbdf363e8d8e1.patch

We could reason about the output, arrange some meetings to discuss about the
clang-format configuration if we think we could go ahead with the adoption.

Luca Fancellu (8):
  cppcheck: rework exclusion_file_list.py code
  exclude-list: generalise exclude-list
  [WIP]xen/scripts: add codestyle.py script
  exclude-list: add entries to the excluded list for codestyle
  [WIP]codestyle.py: Protect generic piece of code
  [WIP]x86/exclude-list: protect mm_type_tbl in mtrr from being
formatted
  xen: Add clang-format configuration
  feedback from the community

 docs/misra/exclude-list.json  | 113 +++
 docs/misra/exclude-list.rst   |  37 +-
 xen/.clang-format | 693 ++
 xen/scripts/codestyle.py  | 289 
 xen/scripts/xen_analysis/cppcheck_analysis.py |   6 +-
 .../xen_analysis/exclusion_file_list.py   |  52 +-
 6 files changed, 1159 insert

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Julien Grall]

Hi Stefano,

On 30/10/2023 22:49, Stefano Stabellini wrote:

On Mon, 30 Oct 2023, Julien Grall wrote:

Hi Nicola,

On 27/10/2023 16:11, Nicola Vetrini wrote:

diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 8511a189253b..81473fb4 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
- __emulate_2op and __emulate_2op_nobyte
- read_debugreg and write_debugreg
   +   * - R7.1
+ - It is safe to use certain octal constants the way they are defined
+   in specifications, manuals, and algorithm descriptions. Such places
+   are marked safe with a /\* octal-ok \*/ in-code comment, or with a
SAF
+   comment (see safe.json).


Reading this, it is unclear to me why we have two ways to deviate the rule
r7.1. And more importantely, how would the developper decide which one to use?


I agree with you on this and we were discussing this topic just this
morning in the FUSA community call. I think we need a way to do this
with the SAF framework:

if (some code with violation) /* SAF-xx-safe */

This doesn't work today unfortunately. It can only be done this way:

/* SAF-xx-safe */
if (some code with violation)

Which is not always desirable. octal-ok is just an ad-hoc solution for
one specific violation but we need a generic way to do this. Luca is
investigating possible ways to support the previous format in SAF.


Why can't we use octal-ok everywhere for now? My point here is to make 
simple for the developper to know what to use.




I think we should take this patch for now and harmonize it once SAF is
improved.


The description of the deviation needs some improvement. To give an 
example, with the current wording, one could they can use octal-ok 
everywhere. But above, you are implying that SAF-xx-safe should be

preferred.

I would still strongly prefer if we use octal-ok everywhere because this 
is simple to remember. But if the other are happy to have both SAF-XX 
and octal-ok, then the description needs to be completely unambiguous 
and the patch should contain some explanation why we have two different 
ways to deviate.


Cheers,

--
Julien Grall

Re: [PATCH for-4.18] docs: Fix IOMMU command line docs some more

[Andrew Cooper]

On 31/10/2023 12:24 pm, Roger Pau Monné wrote:
> On Tue, Oct 31, 2023 at 12:02:15PM +, Andrew Cooper wrote:
>> Make the command line docs match the actual implementation, and state that 
>> the
>> default behaviour is selected at compile time.
>>
>> Fixes: 980d6acf1517 ("IOMMU: make DMA containment of quarantined devices 
>> optional")
>> Signed-off-by: Andrew Cooper 
>> ---
>> CC: Jan Beulich 
>> CC: Roger Pau Monné 
>> CC: Wei Liu 
>> CC: Marek Marczykowski-Górecki 
>> CC: Henry Wang 
>> ---
>>  docs/misc/xen-command-line.pandoc | 6 --
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/misc/xen-command-line.pandoc 
>> b/docs/misc/xen-command-line.pandoc
>> index 6b07d0f3a17f..9a19a04157cb 100644
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -1480,7 +1480,8 @@ detection of systems known to misbehave upon accesses 
>> to that port.
>>  > Default: `new` unless directed-EOI is supported
>>  
>>  ### iommu
>> -= List of [ , verbose, debug, force, required, 
>> quarantine[=scratch-page],
>> += List of [ , verbose, debug, force, required,
>> +quarantine=|scratch-page,
> I think this should be quarantine=[|scratch-page], as just using
> iommu=quarantine is a valid syntax and will enable basic quarantine.
> IOW: the bool or scratch-page parameters are optional.

= already has that meaning, and this is the form we use elsewhere.

>
>>  sharept, superpages, intremap, intpost, crash-disable,
>>  snoop, qinval, igfx, amd-iommu-perdev-intremap,
>>  dom0-{passthrough,strict} ]
>> @@ -1519,7 +1520,8 @@ boolean (e.g. `iommu=no`) can override this and leave 
>> the IOMMUs disabled.
>>  successfully.
>>  
>>  *   The `quarantine` option can be used to control Xen's behavior when
>> -de-assigning devices from guests.
>> +de-assigning devices from guests.  The default behaviour is chosen at
>> +compile time, and is one of 
>> `CONFIG_IOMMU_QUARANTINE_{NONE,BASIC,SCRATCH_PAGE}`.
> Do we also want to state that the current build time default is BASIC
> if the user hasn't selected otherwise?

This is an instruction to look at the .config file and see which it is.

The exceptional case of someone doing a build from clean isn't
particularly interesting.  Not least because they will be prompted for
it and given the choices.

~Andrew

[PATCH] x86/irq: do not insert IRQ_MSI_EMU in emuirq mappings

[Xenia Ragiadakou]

Do not use emuirq mappings for MSIs injected by emulated devices.
This kind of pirq shares the same emuirq value and is not remapped.

Fixes: 88fccdd11ca0 ('xen: event channel remapping for emulated MSIs')
Signed-off-by: Xenia Ragiadakou 
---

Question: is there any strong reason why Linux HVM guests still use pirqs?

 xen/arch/x86/irq.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index f42ad539dc..cdc8dc5a55 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2684,7 +2684,7 @@ int map_domain_emuirq_pirq(struct domain *d, int pirq, 
int emuirq)
 }
 
 old_emuirq = domain_pirq_to_emuirq(d, pirq);
-if ( emuirq != IRQ_PT )
+if ( (emuirq != IRQ_PT) && (emuirq != IRQ_MSI_EMU) )
 old_pirq = domain_emuirq_to_pirq(d, emuirq);
 
 if ( (old_emuirq != IRQ_UNBOUND && (old_emuirq != emuirq) ) ||
@@ -2699,8 +2699,8 @@ int map_domain_emuirq_pirq(struct domain *d, int pirq, 
int emuirq)
 if ( !info )
 return -ENOMEM;
 
-/* do not store emuirq mappings for pt devices */
-if ( emuirq != IRQ_PT )
+/* do not store emuirq mappings for pt devices and emulated MSIs */
+if ( (emuirq != IRQ_PT) && (emuirq != IRQ_MSI_EMU) )
 {
 int err = radix_tree_insert(&d->arch.hvm.emuirq_pirq, emuirq,
 radix_tree_int_to_ptr(pirq));
@@ -2753,7 +2753,7 @@ int unmap_domain_pirq_emuirq(struct domain *d, int pirq)
 info->arch.hvm.emuirq = IRQ_UNBOUND;
 pirq_cleanup_check(info, d);
 }
-if ( emuirq != IRQ_PT )
+if ( (emuirq != IRQ_PT) && (emuirq != IRQ_MSI_EMU) )
 radix_tree_delete(&d->arch.hvm.emuirq_pirq, emuirq);
 
  done:
-- 
2.34.1

Re: [PATCH for-4.19 3/3] CHANGELOG: Keep unstable section

[Julien Grall]

Hi,

On 31/10/2023 13:19, Andrew Cooper wrote:

Signed-off-by: Andrew Cooper 


Henry already provided a similar patch [1]. The only reason it is not 
yet committed is because we haven't yet set a final date for 4.18 and I 
want to avoid any clash when that patch will appear.


Cheers,

[1] 20231023092123.1756426-5-henry.w...@arm.com


---
CC: George Dunlap 
CC: Jan Beulich 
CC: Stefano Stabellini 
CC: Wei Liu 
CC: Julien Grall 
CC: Roger Pau Monné 
CC: Henry Wang 
---
  CHANGELOG.md | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index a827054cf27d..cf0c9c3f8cb9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,8 @@ Notable changes to Xen will be documented in this file.
  
  The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
  
+## [unstable UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=staging) - TBD

+
  ## 
[4.18.0](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.18.0)
 - 2023-XX-XX
  
  ### Changed


--
Julien Grall

Re: [PATCH for-4.19 3/3] CHANGELOG: Keep unstable section

[Andrew Cooper]

On 31/10/2023 1:31 pm, Julien Grall wrote:
> Hi,
>
> On 31/10/2023 13:19, Andrew Cooper wrote:
>> Signed-off-by: Andrew Cooper 
>
> Henry already provided a similar patch [1]. The only reason it is not
> yet committed is because we haven't yet set a final date for 4.18 and
> I want to avoid any clash when that patch will appear.
>
> Cheers,
>
> [1] 20231023092123.1756426-5-henry.w...@arm.com

This section should not have been deleted in d9f07b06cfc9.

It's fine to have an unstable section before the 4.18 date is confirmed,
and the section must exist before staging is re-opened for 4.19 content.

I don't mind which of these two patches gets committed, but one of them
is getting committed today ahead of staging re-opening.  Part of
branching ought to ensure that this section exists.

Henry, your choice.

~Andrew

Re: [PATCH for-4.18] docs: Fix IOMMU command line docs some more

[Roger Pau Monné]

On Tue, Oct 31, 2023 at 01:29:04PM +, Andrew Cooper wrote:
> On 31/10/2023 12:24 pm, Roger Pau Monné wrote:
> > On Tue, Oct 31, 2023 at 12:02:15PM +, Andrew Cooper wrote:
> >> Make the command line docs match the actual implementation, and state that 
> >> the
> >> default behaviour is selected at compile time.
> >>
> >> Fixes: 980d6acf1517 ("IOMMU: make DMA containment of quarantined devices 
> >> optional")
> >> Signed-off-by: Andrew Cooper 

Reviewed-by: Roger Pau Monné 

> >> ---
> >> CC: Jan Beulich 
> >> CC: Roger Pau Monné 
> >> CC: Wei Liu 
> >> CC: Marek Marczykowski-Górecki 
> >> CC: Henry Wang 
> >> ---
> >>  docs/misc/xen-command-line.pandoc | 6 --
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/docs/misc/xen-command-line.pandoc 
> >> b/docs/misc/xen-command-line.pandoc
> >> index 6b07d0f3a17f..9a19a04157cb 100644
> >> --- a/docs/misc/xen-command-line.pandoc
> >> +++ b/docs/misc/xen-command-line.pandoc
> >> @@ -1480,7 +1480,8 @@ detection of systems known to misbehave upon 
> >> accesses to that port.
> >>  > Default: `new` unless directed-EOI is supported
> >>  
> >>  ### iommu
> >> -= List of [ , verbose, debug, force, required, 
> >> quarantine[=scratch-page],
> >> += List of [ , verbose, debug, force, required,
> >> +quarantine=|scratch-page,
> > I think this should be quarantine=[|scratch-page], as just using
> > iommu=quarantine is a valid syntax and will enable basic quarantine.
> > IOW: the bool or scratch-page parameters are optional.
> 
> = already has that meaning, and this is the form we use elsewhere.

I guess I got confused by some other options using `[ ]` to denote
optional parameters, but I see it's not used by all of them.

Thanks, Roger.

Re: [PATCH for-4.19 3/3] CHANGELOG: Keep unstable section

[Julien Grall]

On 31/10/2023 13:38, Andrew Cooper wrote:

On 31/10/2023 1:31 pm, Julien Grall wrote:

Hi,

On 31/10/2023 13:19, Andrew Cooper wrote:

Signed-off-by: Andrew Cooper 


Henry already provided a similar patch [1]. The only reason it is not
yet committed is because we haven't yet set a final date for 4.18 and
I want to avoid any clash when that patch will appear.

Cheers,

[1] 20231023092123.1756426-5-henry.w...@arm.com


This section should not have been deleted in d9f07b06cfc9.


Why? This has always been our process. We should not ship 4.18 with the 
UNSTABLE section. So it was correct to delete it in d9f07b06cfc9.




It's fine to have an unstable section before the 4.18 date is confirmed,
and the section must exist before staging is re-opened for 4.19 content.


I disagree. 4.19 will not be fully re-open until we finally release. So 
I wouldn't expect any new features to be merged.




I don't mind which of these two patches gets committed, but one of them
is getting committed today ahead of staging re-opening.  Part of
branching ought to ensure that this section exists.


If you want to go down that route, then please update the 
docs/process/branching-checklist.txt. Otherwise, I will continue to do 
as I did previously.


Cheers,

--
Julien Grall

Re: [PATCH for-4.18] docs: Fix IOMMU command line docs some more

[Andrew Cooper]

On 31/10/2023 1:45 pm, Roger Pau Monné wrote:
> On Tue, Oct 31, 2023 at 01:29:04PM +, Andrew Cooper wrote:
>> On 31/10/2023 12:24 pm, Roger Pau Monné wrote:
>>> On Tue, Oct 31, 2023 at 12:02:15PM +, Andrew Cooper wrote:
 Make the command line docs match the actual implementation, and state that 
 the
 default behaviour is selected at compile time.

 Fixes: 980d6acf1517 ("IOMMU: make DMA containment of quarantined devices 
 optional")
 Signed-off-by: Andrew Cooper 
> Reviewed-by: Roger Pau Monné 

Thanks.

>
 ---
 CC: Jan Beulich 
 CC: Roger Pau Monné 
 CC: Wei Liu 
 CC: Marek Marczykowski-Górecki 
 CC: Henry Wang 
 ---
  docs/misc/xen-command-line.pandoc | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

 diff --git a/docs/misc/xen-command-line.pandoc 
 b/docs/misc/xen-command-line.pandoc
 index 6b07d0f3a17f..9a19a04157cb 100644
 --- a/docs/misc/xen-command-line.pandoc
 +++ b/docs/misc/xen-command-line.pandoc
 @@ -1480,7 +1480,8 @@ detection of systems known to misbehave upon 
 accesses to that port.
  > Default: `new` unless directed-EOI is supported
  
  ### iommu
 -= List of [ , verbose, debug, force, required, 
 quarantine[=scratch-page],
 += List of [ , verbose, debug, force, required,
 +quarantine=|scratch-page,
>>> I think this should be quarantine=[|scratch-page], as just using
>>> iommu=quarantine is a valid syntax and will enable basic quarantine.
>>> IOW: the bool or scratch-page parameters are optional.
>> = already has that meaning, and this is the form we use elsewhere.
> I guess I got confused by some other options using `[ ]` to denote
> optional parameters, but I see it's not used by all of them.

Yeah, it's a mess, sadly.  One of many things I've not had time to fix,
but at least this is closer to the normal syntax than before.

~Andrew

Re: [PATCH for-4.18 1/3] CHANGELOG: Reformat

[Henry Wang]

Hi Andrew,

> On Oct 31, 2023, at 21:19, Andrew Cooper  wrote:
> 
> Collect all x86 and ARM changes together instead of having them scattered.
> Tweak grammar as necessary.
> 
> No change.
> 
> Signed-off-by: Andrew Cooper 

Acked-by: Henry Wang 
Release-acked-by: Henry Wang 

Kind regards,
Henry

Re: [PATCH for-4.18 2/3] CHANGELOG: More 4.18 content

[Henry Wang]

Hi Andrew,

> On Oct 31, 2023, at 21:19, Andrew Cooper  wrote:
> 
> Signed-off-by: Andrew Cooper 

Acked-by: Henry Wang 
Release-acked-by: Henry Wang 

Kind regards,
Henry

Re: [PATCH for-4.19 3/3] CHANGELOG: Keep unstable section

[Andrew Cooper]

On 31/10/2023 1:45 pm, Julien Grall wrote:
> If you want to go down that route, then please update the
> docs/process/branching-checklist.txt. Otherwise, I will continue to do
> as I did previously.

It *is* in the checklist, and for all previous releases even 4.17, the
staging section was opened at the time of branching.

The thing that is different between 4.17 and previously is that 4.17
called it "unstable" where previously (and in the checklist) it says to
make the new section match the updated Xen major/minor number.

~Andrew

Re: [PATCH for-4.19 3/3] CHANGELOG: Keep unstable section

[Henry Wang]

Hi Julien, Andrew,

> On Oct 31, 2023, at 21:45, Julien Grall  wrote:
> On 31/10/2023 13:38, Andrew Cooper wrote:
>> On 31/10/2023 1:31 pm, Julien Grall wrote:
>>> Hi,
>>> 
>>> On 31/10/2023 13:19, Andrew Cooper wrote:
 Signed-off-by: Andrew Cooper 
>>> 
>>> Henry already provided a similar patch [1]. The only reason it is not
>>> yet committed is because we haven't yet set a final date for 4.18 and
>>> I want to avoid any clash when that patch will appear.
>>> 
>>> Cheers,
>>> 
>>> [1] 20231023092123.1756426-5-henry.w...@arm.com
>> This section should not have been deleted in d9f07b06cfc9.
> 
> Why? This has always been our process. We should not ship 4.18 with the 
> UNSTABLE section. So it was correct to delete it in d9f07b06cfc9.

I agree with Julien on this. removing the “unstable” section is the tradition 
that we had previously,
see below (I am using github link for a easier finding of history commits):
- [1] for 4.15
- [2] for 4.16
- [3] for 4.17

I am not sure there is any specific reason for changing the existing process.

The other two patches in this series looks good and I’ve acked both of them. 
Thanks for taking care
of it.

[1] 
https://github.com/xen-project/xen/commit/b8eaedbb51e8e51808728d7de392d3e9117fdece
[2] 
https://github.com/xen-project/xen/commit/eef266eb770128db0d5258009b744f0e0c31c9bd
[3] 
https://github.com/xen-project/xen/commit/0829a2f3fcaba9233dfdce9a8cee9d126a51bd4d

Kind regards,
Henry

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Luca Fancellu]

> On 31 Oct 2023, at 13:27, Julien Grall  wrote:
> 
> Hi Stefano,
> 
> On 30/10/2023 22:49, Stefano Stabellini wrote:
>> On Mon, 30 Oct 2023, Julien Grall wrote:
>>> Hi Nicola,
>>> 
>>> On 27/10/2023 16:11, Nicola Vetrini wrote:
 diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
 index 8511a189253b..81473fb4 100644
 --- a/docs/misra/deviations.rst
 +++ b/docs/misra/deviations.rst
 @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
- __emulate_2op and __emulate_2op_nobyte
- read_debugreg and write_debugreg
   +   * - R7.1
 + - It is safe to use certain octal constants the way they are defined
 +   in specifications, manuals, and algorithm descriptions. Such places
 +   are marked safe with a /\* octal-ok \*/ in-code comment, or with a
 SAF
 +   comment (see safe.json).
>>> 
>>> Reading this, it is unclear to me why we have two ways to deviate the rule
>>> r7.1. And more importantely, how would the developper decide which one to 
>>> use?
>> I agree with you on this and we were discussing this topic just this
>> morning in the FUSA community call. I think we need a way to do this
>> with the SAF framework:
>> if (some code with violation) /* SAF-xx-safe */
>> This doesn't work today unfortunately. It can only be done this way:
>> /* SAF-xx-safe */
>> if (some code with violation)
>> Which is not always desirable. octal-ok is just an ad-hoc solution for
>> one specific violation but we need a generic way to do this. Luca is
>> investigating possible ways to support the previous format in SAF.
> 
> Why can't we use octal-ok everywhere for now? My point here is to make simple 
> for the developper to know what to use.
> 
>> I think we should take this patch for now and harmonize it once SAF is
>> improved.
> 
> The description of the deviation needs some improvement. To give an example, 
> with the current wording, one could they can use octal-ok everywhere. But 
> above, you are implying that SAF-xx-safe should be
> preferred.
> 
> I would still strongly prefer if we use octal-ok everywhere because this is 
> simple to remember. But if the other are happy to have both SAF-XX and 
> octal-ok, then the description needs to be completely unambiguous and the 
> patch should contain some explanation why we have two different ways to 
> deviate.

Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */

So that the suppression engine do what it should (currently it doesn’t suppress 
the same line, but we could do something about it) and the developer
has a way to understand what is the violation here without going to the 
justification database.

Re: [PATCH v4 4/5] [FUTURE] xen/arm: enable vPCI for domUs

[Stewart Hildebrand]

On 10/31/23 09:17, Julien Grall wrote:
> Hi,
> 
> On 31/10/2023 11:03, Jan Beulich wrote:
>> On 31.10.2023 00:52, Stewart Hildebrand wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -1618,6 +1618,16 @@ int iommu_do_pci_domctl(
>>>   bus = PCI_BUS(machine_sbdf);
>>>   devfn = PCI_DEVFN(machine_sbdf);
>>>   +    if ( IS_ENABLED(CONFIG_ARM) &&
>>> + !is_hardware_domain(d) &&
>>> + !is_system_domain(d) &&
>>> + (!IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) || !has_vpci(d)) )
>>
>> I don't think you need the explicit ARM check; that's redundant with
>> checking !HAS_VPCI_GUEST_SUPPORT.

Currently that is true. However, this is allowing for the possibility that we 
eventually may want to enable PCI passthrough for PVH domU using vPCI (e.g. 
hyperlaunch, or eliminating qemu backend), in which case we may want to enable 
CONFIG_HAS_VPCI_GUEST_SUPPORT=y on x86.

>> It's also not really clear why you
>> need to check for the system domain here.

xl pci-assignable-add will assign the device to domIO, which doesn't have vPCI, 
but it is still a valid assignment. Perhaps an in code comment would be helpful 
for clarity?

> 
> I might be missing but I wouldn't expect the domain to have vPCI enabled if 
> CONFIG_HAVE_VPCI_GUEST_SUPPORT=n. So why can't this simply be:
> 
> if ( !has_vcpi(d) )
> {
>    ...
> }

Right, the CONFIG_HAVE_VPCI_GUEST_SUPPORT check here is not strictly needed 
because this case is already caught by the other half of this patch in 
xen/arch/arm/vpci.c. This simplifies it to:

if ( IS_ENABLED(CONFIG_ARM) &&
 !is_hardware_domain(d) &&
 !is_system_domain(d) /* !domIO */ &&
 !has_vpci(d) )

On x86, unless I misunderstood something, I think it's valid to assign PCI 
devices to a domU without has_vpci().

BTW, it's valid for has_vpci() to be true and CONFIG_HAVE_VPCI_GUEST_SUPPORT=n 
for dom0.

Re: [PATCH for-4.19 3/3] CHANGELOG: Keep unstable section

[Julien Grall]

On 31/10/2023 14:06, Andrew Cooper wrote:

On 31/10/2023 1:45 pm, Julien Grall wrote:

If you want to go down that route, then please update the
docs/process/branching-checklist.txt. Otherwise, I will continue to do
as I did previously.


It *is* in the checklist, and for all previous releases even 4.17, the
staging section was opened at the time of branching.


It doesn't tell me when it has to be done. The difference with 4.17 is 
we don't yet have a date for the release. Hence why I delayed.




The thing that is different between 4.17 and previously is that 4.17
called it "unstable" where previously (and in the checklist) it says to
make the new section match the updated Xen major/minor number.



I still don't see the problem of delaying the CHANGELOG as I decided to 
do. If you are not happy with it, how about you take over the release 
technician process? It is already complex enough that I don't need 
someone else to tell me exactly how I should do it.


Cheers,

--
Julien Grall

Re: Cambridge University Talk - 9th November 2023

[Roger Pau Monné]

On Tue, Oct 31, 2023 at 01:05:11PM +, Ayan Kumar Halder wrote:
> Hi Xen Maintainers/developers,
> 
> 
> As part of my talk, I wish to provide some examples of tasks that a newbie
> can easily pick up and contribute.
> 
> This need not be a dedicated project, but something that can be contributed
> on an ad-hoc basis.
> 
> The idea is to get more people interested in Xen project. :)
> 
> 
> I found some examples of this :-
> 
> 1. Misra C fixes - Refer "Misra rule 10.3 violations report script" . Luca
> has provided an awesome script to identify the MISRA violations. This can be
> used to provide fixes.

TBH, I think doing MISRA fixes is not that attractive for a new comer,
as those are (mostly?) non-functional fixes, but I might be wrong.

> 2. https://wiki.xenproject.org/wiki/Outreach_Program_Projects - I think this
> page provides some pointers, but I am not sure if this is up to date.

I'm not sure how up to date this is.

> 
> Please let me know if there are more of these examples.

gitlab contains some:

https://gitlab.com/groups/xen-project/-/issues/?label_name%5B%5D=Difficulty%3A%3A1-GOOD%20FIRST%20ISSUE
https://gitlab.com/groups/xen-project/-/issues/?label_name%5B%5D=Difficulty%3A%3A1-EASY

Regards, Roger.

[PATCH v2] xen: remove

[Oleksii Kurochko]

 only declares udelay() function so udelay()
declaration was moved to xen/delay.h.

For x86, __udelay() was renamed to udelay() and removed
inclusion of  in x86 code.

For ppc, udelay() stub definition was moved to ppc/stubs.c.

Suggested-by: Jan Beulich 
Signed-off-by: Oleksii Kurochko 
Reviewed-by: Jan Beulich 
---
Changes in v2:
 - rebase on top of the latest staging.
 - add Suggested-by:/Reviewed-by: Jan Beulich .
 - remove  for PPC.
 - remove changes related to RISC-V's  as they've not
   introduced in staging branch yet.
---
 xen/arch/arm/include/asm/delay.h  | 14 --
 xen/arch/ppc/include/asm/delay.h  | 12 
 xen/arch/ppc/stubs.c  |  7 +++
 xen/arch/x86/cpu/microcode/core.c |  2 +-
 xen/arch/x86/delay.c  |  2 +-
 xen/arch/x86/include/asm/delay.h  | 13 -
 xen/include/xen/delay.h   |  2 +-
 7 files changed, 10 insertions(+), 42 deletions(-)
 delete mode 100644 xen/arch/arm/include/asm/delay.h
 delete mode 100644 xen/arch/ppc/include/asm/delay.h
 delete mode 100644 xen/arch/x86/include/asm/delay.h

diff --git a/xen/arch/arm/include/asm/delay.h b/xen/arch/arm/include/asm/delay.h
deleted file mode 100644
index 042907d9d5..00
--- a/xen/arch/arm/include/asm/delay.h
+++ /dev/null
@@ -1,14 +0,0 @@
-#ifndef _ARM_DELAY_H
-#define _ARM_DELAY_H
-
-extern void udelay(unsigned long usecs);
-
-#endif /* defined(_ARM_DELAY_H) */
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/arch/ppc/include/asm/delay.h b/xen/arch/ppc/include/asm/delay.h
deleted file mode 100644
index da6635888b..00
--- a/xen/arch/ppc/include/asm/delay.h
+++ /dev/null
@@ -1,12 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-#ifndef __ASM_PPC_DELAY_H__
-#define __ASM_PPC_DELAY_H__
-
-#include 
-
-static inline void udelay(unsigned long usecs)
-{
-BUG_ON("unimplemented");
-}
-
-#endif /* __ASM_PPC_DELAY_H__ */
diff --git a/xen/arch/ppc/stubs.c b/xen/arch/ppc/stubs.c
index 4c276b0e39..a96e45626d 100644
--- a/xen/arch/ppc/stubs.c
+++ b/xen/arch/ppc/stubs.c
@@ -337,3 +337,10 @@ int __init parse_arch_dom0_param(const char *s, const char 
*e)
 {
 BUG_ON("unimplemented");
 }
+
+/* delay.c */
+
+void udelay(unsigned long usecs)
+{
+BUG_ON("unimplemented");
+}
diff --git a/xen/arch/x86/cpu/microcode/core.c 
b/xen/arch/x86/cpu/microcode/core.c
index 65ebeb50de..22d5e04552 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -23,6 +23,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -35,7 +36,6 @@
 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
diff --git a/xen/arch/x86/delay.c b/xen/arch/x86/delay.c
index 2662c26272..b3a41881a1 100644
--- a/xen/arch/x86/delay.c
+++ b/xen/arch/x86/delay.c
@@ -15,7 +15,7 @@
 #include 
 #include 
 
-void __udelay(unsigned long usecs)
+void udelay(unsigned long usecs)
 {
 unsigned long ticks = usecs * (cpu_khz / 1000);
 unsigned long s, e;
diff --git a/xen/arch/x86/include/asm/delay.h b/xen/arch/x86/include/asm/delay.h
deleted file mode 100644
index 9be2f46590..00
--- a/xen/arch/x86/include/asm/delay.h
+++ /dev/null
@@ -1,13 +0,0 @@
-#ifndef _X86_DELAY_H
-#define _X86_DELAY_H
-
-/*
- * Copyright (C) 1993 Linus Torvalds
- *
- * Delay routines calling functions in arch/i386/lib/delay.c
- */
-
-extern void __udelay(unsigned long usecs);
-#define udelay(n) __udelay(n)
-
-#endif /* defined(_X86_DELAY_H) */
diff --git a/xen/include/xen/delay.h b/xen/include/xen/delay.h
index 9150226271..8fd3b8f99f 100644
--- a/xen/include/xen/delay.h
+++ b/xen/include/xen/delay.h
@@ -3,7 +3,7 @@
 
 /* Copyright (C) 1993 Linus Torvalds */
 
-#include 
+void udelay(unsigned long usecs);
 
 static inline void mdelay(unsigned long msec)
 {
-- 
2.41.0

Re: [PATCH v1 22/29] xen/asm-generic: introduce stub header delay.h

[Oleksii]

Instead of introducing stub header for delay.h it was decided to remove
 in a separate patch:
https://lore.kernel.org/xen-devel/3d55bce44bd6ab9973cbe0ea2fc136cc44d35df2.1698759633.git.oleksii.kuroc...@gmail.com/T/#u

~ Oleksii

[PATCH v4] CHANGELOG.md: Start new "unstable" section

[Henry Wang]

Signed-off-by: Henry Wang 
---
v4:
- Set the release date.
---
 CHANGELOG.md | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3ca7969699..cbdc9bceac 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,7 +4,15 @@ Notable changes to Xen will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 
-## 
[4.18.0](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.18.0)
 - 2023-XX-XX
+## [unstable 
UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=staging)
 - TBD
+
+### Changed
+
+### Added
+
+### Removed
+
+## 
[4.18.0](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.18.0)
 - 2023-11-16
 
 ### Changed
  - Repurpose command line gnttab_max_{maptrack_,}frames options so they don't
-- 
2.25.1

Re: [PATCH v4] CHANGELOG.md: Start new "unstable" section

[Henry Wang]

Hi,

> On Oct 31, 2023, at 22:34, Henry Wang  wrote:
> 
> Signed-off-by: Henry Wang 
> ---
> v4:
> - Set the release date.
> ---
> CHANGELOG.md | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index 3ca7969699..cbdc9bceac 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -4,7 +4,15 @@ Notable changes to Xen will be documented in this file.
> 
> The format is based on [Keep a 
> Changelog](https://keepachangelog.com/en/1.0.0/)
> 
> -## 
> [4.18.0](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.18.0)
>  - 2023-XX-XX
> +## [unstable 
> UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=staging)
>  - TBD
> +
> +### Changed
> +
> +### Added
> +
> +### Removed
> +
> +## 
> [4.18.0](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.18.0)
>  - 2023-11-16

To be honest I will split them for the convenience of the release technician’s 
work. Sorry..

Kind regards,
Henry

[PATCH v5 0/2] Finalize the 4.18 release date

[Henry Wang]

Hi all,

This series finializes the 4.18 release date and starts a
new unstable release after branching.

Thanks.

Henry Wang (2):
  CHANGELOG.md: Finalize the 4.18 release date
  CHANGELOG.md: Start new "unstable" section

 CHANGELOG.md | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH v5 2/2] CHANGELOG.md: Start new "unstable" section

[Henry Wang]

Signed-off-by: Henry Wang 
---
v5:
- Rebase on previous patches.
---
 CHANGELOG.md | 8 
 1 file changed, 8 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 94dbd83894..cbdc9bceac 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,14 @@ Notable changes to Xen will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 
+## [unstable 
UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=staging)
 - TBD
+
+### Changed
+
+### Added
+
+### Removed
+
 ## 
[4.18.0](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.18.0)
 - 2023-11-16
 
 ### Changed
-- 
2.25.1

[PATCH v5 1/2] CHANGELOG.md: Finalize the 4.18 release date

[Henry Wang]

Signed-off-by: Henry Wang 
---
v5:
- New patch
---
 CHANGELOG.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3ca7969699..94dbd83894 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,7 +4,7 @@ Notable changes to Xen will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 
-## 
[4.18.0](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.18.0)
 - 2023-XX-XX
+## 
[4.18.0](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.18.0)
 - 2023-11-16
 
 ### Changed
  - Repurpose command line gnttab_max_{maptrack_,}frames options so they don't
-- 
2.25.1

Re: [PATCH v5 1/2] CHANGELOG.md: Finalize the 4.18 release date

[Julien Grall]

On 31/10/2023 14:49, Henry Wang wrote:

Signed-off-by: Henry Wang 


Acked-by: Julien Grall 

Cheers,

--
Julien Grall

Re: [PATCH for-4.19 3/3] CHANGELOG: Keep unstable section

[Henry Wang]

Hi both,

> On Oct 31, 2023, at 22:19, Julien Grall  wrote:
> On 31/10/2023 14:06, Andrew Cooper wrote:
>> On 31/10/2023 1:45 pm, Julien Grall wrote:
>>> If you want to go down that route, then please update the
>>> docs/process/branching-checklist.txt. Otherwise, I will continue to do
>>> as I did previously.
>> It *is* in the checklist, and for all previous releases even 4.17, the
>> staging section was opened at the time of branching.
> 
> It doesn't tell me when it has to be done. The difference with 4.17 is we 
> don't yet have a date for the release. Hence why I delayed.

I’ve sent the updated [1] out, hopefully this will make both of you happy 
(Still I am thinking
both of you are actually mentioning the same thing, i.e. starting a new 
unstable section after
the branching).

[1] 
https://lore.kernel.org/xen-devel/20231031144925.2416266-1-henry.w...@arm.com/

Kind regards,
Henry

Re: [PATCH v5 2/2] CHANGELOG.md: Start new "unstable" section

[Julien Grall]

Hi Henry,

On 31/10/2023 14:49, Henry Wang wrote:

Signed-off-by: Henry Wang 


Acked-by: Julien Grall 

Cheers,

--
Julien Grall

[PATCH v2] x86/x2apic: introduce a mixed physical/cluster mode

[Roger Pau Monne]

The current implementation of x2APIC requires to either use Cluster Logical or
Physical mode for all interrupts.  However the selection of Physical vs Logical
is not done at APIC setup, an APIC can be addressed both in Physical or Logical
destination modes concurrently.

Introduce a new x2APIC mode called mixed, which uses Logical Cluster mode for
IPIs, and Physical mode for external interrupts, thus attempting to use the
best method for each interrupt type.

Using Physical mode for external interrupts allows more vectors to be used, and
interrupt balancing to be more accurate.

Using Logical Cluster mode for IPIs allows less accesses to the ICR register
when sending those, as multiple CPUs can be targeted with a single ICR register
write.

A simple test calling flush_tlb_all() 1 times in a tight loop on a 96 CPU
box gives the following average figures:

Physical mode: 26617931ns
Mixed mode:23865337ns

So ~10% improvement versus plain Physical mode.  Note that Xen uses Cluster
mode by default, and hence is already using the fastest way for IPI delivery at
the cost of reducing the amount of vectors available system-wide.

Make the newly introduced mode the default one.

Suggested-by: Andrew Cooper 
Signed-off-by: Roger Pau Monné 
---
Changes since v1:
 - Add change log entry.
 - Fix indentation and usage of tristate in Kconfig.
 - Adjust comment regarding hooks used by external interrupts in
   apic_x2apic_mixed.
---
 CHANGELOG.md  |  6 ++
 docs/misc/xen-command-line.pandoc | 12 
 xen/arch/x86/Kconfig  | 35 ++--
 xen/arch/x86/genapic/x2apic.c | 91 ++-
 4 files changed, 114 insertions(+), 30 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3ca796969990..9b04849b0336 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,12 @@ Notable changes to Xen will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 
+## [unstable 
UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=staging)
 - TBD
+
+### Added
+ - On x86 introduce a new x2APIC driver that uses Cluster Logical addressing
+   mode for IPIs and Physical addressing mode for external interrupts.
+
 ## 
[4.18.0](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.18.0)
 - 2023-XX-XX
 
 ### Changed
diff --git a/docs/misc/xen-command-line.pandoc 
b/docs/misc/xen-command-line.pandoc
index 6b07d0f3a17f..cbe9b4802c61 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2802,6 +2802,15 @@ the watchdog.
 
 Permit use of x2apic setup for SMP environments.
 
+### x2apic-mode (x86)
+> `= physical | cluster | mixed`
+
+> Default: `physical` if **FADT** mandates physical mode, otherwise set at
+>  build time by CONFIG_X2APIC_{PHYSICAL,LOGICAL,MIXED}.
+
+In the case that x2apic is in use, this option switches between modes to
+address APICs in the system as interrupt destinations.
+
 ### x2apic_phys (x86)
 > `= `
 
@@ -2812,6 +2821,9 @@ In the case that x2apic is in use, this option switches 
between physical and
 clustered mode.  The default, given no hint from the **FADT**, is cluster
 mode.
 
+**WARNING: `x2apic_phys` is deprecated and superseded by `x2apic-mode`.
+The latter takes precedence if both are set.**
+
 ### xenheap_megabytes (arm32)
 > `= `
 
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index eac77573bd75..cd9286f295e5 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -228,11 +228,18 @@ config XEN_ALIGN_2M
 
 endchoice
 
-config X2APIC_PHYSICAL
-   bool "x2APIC Physical Destination mode"
+choice
+   prompt "x2APIC Destination mode"
+   default X2APIC_MIXED
help
- Use x2APIC Physical Destination mode by default when available.
+ Select APIC addressing when x2APIC is enabled.
+
+ The default mode is mixed which should provide the best aspects
+ of both physical and cluster modes.
 
+config X2APIC_PHYSICAL
+   bool "Physical Destination mode"
+   help
  When using this mode APICs are addressed using the Physical
  Destination mode, which allows using all dynamic vectors on each
  CPU independently.
@@ -242,9 +249,27 @@ config X2APIC_PHYSICAL
  destination inter processor interrupts (IPIs) slightly slower than
  Logical Destination mode.
 
- The mode when this option is not selected is Logical Destination.
+config X2APIC_CLUSTER
+   bool "Cluster Destination mode"
+   help
+ When using this mode APICs are addressed using the Cluster Logical
+ Destination mode.
+
+ Cluster Destination has the benefit of sending IPIs faster since
+ multiple APICs can be targeted as destinations of a single IPI.
+ However the vector space is shared between all CPUs on the cluster,
+ and hence using this mode reduces the number of available vectors
+ 

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Nicola Vetrini]

On 2023-10-31 15:13, Luca Fancellu wrote:

On 31 Oct 2023, at 13:27, Julien Grall  wrote:

Hi Stefano,

On 30/10/2023 22:49, Stefano Stabellini wrote:

On Mon, 30 Oct 2023, Julien Grall wrote:

Hi Nicola,

On 27/10/2023 16:11, Nicola Vetrini wrote:

diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 8511a189253b..81473fb4 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
   - __emulate_2op and __emulate_2op_nobyte
   - read_debugreg and write_debugreg
  +   * - R7.1
+ - It is safe to use certain octal constants the way they are 
defined
+   in specifications, manuals, and algorithm descriptions. 
Such places
+   are marked safe with a /\* octal-ok \*/ in-code comment, or 
with a

SAF
+   comment (see safe.json).


Reading this, it is unclear to me why we have two ways to deviate 
the rule
r7.1. And more importantely, how would the developper decide which 
one to use?

I agree with you on this and we were discussing this topic just this
morning in the FUSA community call. I think we need a way to do this
with the SAF framework:
if (some code with violation) /* SAF-xx-safe */
This doesn't work today unfortunately. It can only be done this way:
/* SAF-xx-safe */
if (some code with violation)
Which is not always desirable. octal-ok is just an ad-hoc solution 
for

one specific violation but we need a generic way to do this. Luca is
investigating possible ways to support the previous format in SAF.


Why can't we use octal-ok everywhere for now? My point here is to make 
simple for the developper to know what to use.


I think we should take this patch for now and harmonize it once SAF 
is

improved.


The description of the deviation needs some improvement. To give an 
example, with the current wording, one could they can use octal-ok 
everywhere. But above, you are implying that SAF-xx-safe should be

preferred.

I would still strongly prefer if we use octal-ok everywhere because 
this is simple to remember. But if the other are happy to have both 
SAF-XX and octal-ok, then the description needs to be completely 
unambiguous and the patch should contain some explanation why we have 
two different ways to deviate.


Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */

So that the suppression engine do what it should (currently it doesn’t 
suppress the same line, but we could do something about it) and the 
developer
has a way to understand what is the violation here without going to the 
justification database.


I guess. It could overflow the 80-char limit in 
xen/arch/x86/hvm/svm/svm.h, though.


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

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Luca Fancellu]

> On 31 Oct 2023, at 15:10, Nicola Vetrini  wrote:
> 
> On 2023-10-31 15:13, Luca Fancellu wrote:
>>> On 31 Oct 2023, at 13:27, Julien Grall  wrote:
>>> Hi Stefano,
>>> On 30/10/2023 22:49, Stefano Stabellini wrote:
 On Mon, 30 Oct 2023, Julien Grall wrote:
> Hi Nicola,
> On 27/10/2023 16:11, Nicola Vetrini wrote:
>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>> index 8511a189253b..81473fb4 100644
>> --- a/docs/misra/deviations.rst
>> +++ b/docs/misra/deviations.rst
>> @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
>>   - __emulate_2op and __emulate_2op_nobyte
>>   - read_debugreg and write_debugreg
>>  +   * - R7.1
>> + - It is safe to use certain octal constants the way they are 
>> defined
>> +   in specifications, manuals, and algorithm descriptions. Such 
>> places
>> +   are marked safe with a /\* octal-ok \*/ in-code comment, or with 
>> a
>> SAF
>> +   comment (see safe.json).
> Reading this, it is unclear to me why we have two ways to deviate the rule
> r7.1. And more importantely, how would the developper decide which one to 
> use?
 I agree with you on this and we were discussing this topic just this
 morning in the FUSA community call. I think we need a way to do this
 with the SAF framework:
 if (some code with violation) /* SAF-xx-safe */
 This doesn't work today unfortunately. It can only be done this way:
 /* SAF-xx-safe */
 if (some code with violation)
 Which is not always desirable. octal-ok is just an ad-hoc solution for
 one specific violation but we need a generic way to do this. Luca is
 investigating possible ways to support the previous format in SAF.
>>> Why can't we use octal-ok everywhere for now? My point here is to make 
>>> simple for the developper to know what to use.
 I think we should take this patch for now and harmonize it once SAF is
 improved.
>>> The description of the deviation needs some improvement. To give an 
>>> example, with the current wording, one could they can use octal-ok 
>>> everywhere. But above, you are implying that SAF-xx-safe should be
>>> preferred.
>>> I would still strongly prefer if we use octal-ok everywhere because this is 
>>> simple to remember. But if the other are happy to have both SAF-XX and 
>>> octal-ok, then the description needs to be completely unambiguous and the 
>>> patch should contain some explanation why we have two different ways to 
>>> deviate.
>> Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */
>> So that the suppression engine do what it should (currently it doesn’t 
>> suppress the same line, but we could do something about it) and the developer
>> has a way to understand what is the violation here without going to the 
>> justification database.
> 
> I guess. It could overflow the 80-char limit in xen/arch/x86/hvm/svm/svm.h, 
> though.

Yeah, but we could rule out something in code_style to allow only this kind of 
trailing comments to exceed the 80 chars

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

Re: [PATCH v5 2/2] CHANGELOG.md: Start new "unstable" section

[Andrew Cooper]

On 31/10/2023 2:49 pm, Henry Wang wrote:
> Signed-off-by: Henry Wang 
> ---
> v5:
> - Rebase on previous patches.
> ---
>  CHANGELOG.md | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index 94dbd83894..cbdc9bceac 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -4,6 +4,14 @@ Notable changes to Xen will be documented in this file.
>  
>  The format is based on [Keep a 
> Changelog](https://keepachangelog.com/en/1.0.0/)
>  
> +## [unstable 
> UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=staging)
>  - TBD

As I've just found it in the checklist, this should be [4.19.0 UNRELEASED].

Happy to fix on commit.

~Andrew

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Julien Grall]

Hi,

On 31/10/2023 15:12, Luca Fancellu wrote:

On 31 Oct 2023, at 15:10, Nicola Vetrini  wrote:

On 2023-10-31 15:13, Luca Fancellu wrote:

On 31 Oct 2023, at 13:27, Julien Grall  wrote:
Hi Stefano,
On 30/10/2023 22:49, Stefano Stabellini wrote:

On Mon, 30 Oct 2023, Julien Grall wrote:

Hi Nicola,
On 27/10/2023 16:11, Nicola Vetrini wrote:

diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 8511a189253b..81473fb4 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
   - __emulate_2op and __emulate_2op_nobyte
   - read_debugreg and write_debugreg
  +   * - R7.1
+ - It is safe to use certain octal constants the way they are defined
+   in specifications, manuals, and algorithm descriptions. Such places
+   are marked safe with a /\* octal-ok \*/ in-code comment, or with a
SAF
+   comment (see safe.json).

Reading this, it is unclear to me why we have two ways to deviate the rule
r7.1. And more importantely, how would the developper decide which one to use?

I agree with you on this and we were discussing this topic just this
morning in the FUSA community call. I think we need a way to do this
with the SAF framework:
if (some code with violation) /* SAF-xx-safe */
This doesn't work today unfortunately. It can only be done this way:
/* SAF-xx-safe */
if (some code with violation)
Which is not always desirable. octal-ok is just an ad-hoc solution for
one specific violation but we need a generic way to do this. Luca is
investigating possible ways to support the previous format in SAF.

Why can't we use octal-ok everywhere for now? My point here is to make simple 
for the developper to know what to use.

I think we should take this patch for now and harmonize it once SAF is
improved.

The description of the deviation needs some improvement. To give an example, 
with the current wording, one could they can use octal-ok everywhere. But 
above, you are implying that SAF-xx-safe should be
preferred.
I would still strongly prefer if we use octal-ok everywhere because this is 
simple to remember. But if the other are happy to have both SAF-XX and 
octal-ok, then the description needs to be completely unambiguous and the patch 
should contain some explanation why we have two different ways to deviate.

Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */
So that the suppression engine do what it should (currently it doesn’t suppress 
the same line, but we could do something about it) and the developer
has a way to understand what is the violation here without going to the 
justification database.


I guess. It could overflow the 80-char limit in xen/arch/x86/hvm/svm/svm.h, 
though.


Yeah, but we could rule out something in code_style to allow only this kind of 
trailing comments to exceed the 80 chars


In the past I expressed concerned with this kind of the rule because it 
is not entirely clear how an automatic formatter will be able to check it.


Can you clarify whether clang-format would be able to handle your 
proposed rule?


Cheers,

--
Julien Grall

Re: [PATCH v5 2/2] CHANGELOG.md: Start new "unstable" section

[Henry Wang]

Hi Andrew,

> On Oct 31, 2023, at 23:20, Andrew Cooper  wrote:
> 
> On 31/10/2023 2:49 pm, Henry Wang wrote:
>> Signed-off-by: Henry Wang 
>> ---
>> v5:
>> - Rebase on previous patches.
>> ---
>> CHANGELOG.md | 8 
>> 1 file changed, 8 insertions(+)
>> 
>> diff --git a/CHANGELOG.md b/CHANGELOG.md
>> index 94dbd83894..cbdc9bceac 100644
>> --- a/CHANGELOG.md
>> +++ b/CHANGELOG.md
>> @@ -4,6 +4,14 @@ Notable changes to Xen will be documented in this file.
>> 
>> The format is based on [Keep a 
>> Changelog](https://keepachangelog.com/en/1.0.0/)
>> 
>> +## [unstable 
>> UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=staging)
>>  - TBD
> 
> As I've just found it in the checklist, this should be [4.19.0 UNRELEASED].

Actually, good point, somehow we seems to have a mix in previous releases (see 
[1] - as an incorrect
example I took for 4.17). I am ok to use 4.19 instead of unstable.

> 
> Happy to fix on commit.

Thank you very much for doing so.

[1] 
https://github.com/xen-project/xen/commit/6c1c97e24f830a921a23e3b9694e20493c9986ee

Kind regards,
Henry

> 
> ~Andrew

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Luca Fancellu]

> On 31 Oct 2023, at 15:27, Julien Grall  wrote:
> 
> Hi,
> 
> On 31/10/2023 15:12, Luca Fancellu wrote:
>>> On 31 Oct 2023, at 15:10, Nicola Vetrini  wrote:
>>> 
>>> On 2023-10-31 15:13, Luca Fancellu wrote:
> On 31 Oct 2023, at 13:27, Julien Grall  wrote:
> Hi Stefano,
> On 30/10/2023 22:49, Stefano Stabellini wrote:
>> On Mon, 30 Oct 2023, Julien Grall wrote:
>>> Hi Nicola,
>>> On 27/10/2023 16:11, Nicola Vetrini wrote:
 diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
 index 8511a189253b..81473fb4 100644
 --- a/docs/misra/deviations.rst
 +++ b/docs/misra/deviations.rst
 @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
   - __emulate_2op and __emulate_2op_nobyte
   - read_debugreg and write_debugreg
  +   * - R7.1
 + - It is safe to use certain octal constants the way they are 
 defined
 +   in specifications, manuals, and algorithm descriptions. Such 
 places
 +   are marked safe with a /\* octal-ok \*/ in-code comment, or 
 with a
 SAF
 +   comment (see safe.json).
>>> Reading this, it is unclear to me why we have two ways to deviate the 
>>> rule
>>> r7.1. And more importantely, how would the developper decide which one 
>>> to use?
>> I agree with you on this and we were discussing this topic just this
>> morning in the FUSA community call. I think we need a way to do this
>> with the SAF framework:
>> if (some code with violation) /* SAF-xx-safe */
>> This doesn't work today unfortunately. It can only be done this way:
>> /* SAF-xx-safe */
>> if (some code with violation)
>> Which is not always desirable. octal-ok is just an ad-hoc solution for
>> one specific violation but we need a generic way to do this. Luca is
>> investigating possible ways to support the previous format in SAF.
> Why can't we use octal-ok everywhere for now? My point here is to make 
> simple for the developper to know what to use.
>> I think we should take this patch for now and harmonize it once SAF is
>> improved.
> The description of the deviation needs some improvement. To give an 
> example, with the current wording, one could they can use octal-ok 
> everywhere. But above, you are implying that SAF-xx-safe should be
> preferred.
> I would still strongly prefer if we use octal-ok everywhere because this 
> is simple to remember. But if the other are happy to have both SAF-XX and 
> octal-ok, then the description needs to be completely unambiguous and the 
> patch should contain some explanation why we have two different ways to 
> deviate.
 Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */
 So that the suppression engine do what it should (currently it doesn’t 
 suppress the same line, but we could do something about it) and the 
 developer
 has a way to understand what is the violation here without going to the 
 justification database.
>>> 
>>> I guess. It could overflow the 80-char limit in xen/arch/x86/hvm/svm/svm.h, 
>>> though.
>> Yeah, but we could rule out something in code_style to allow only this kind 
>> of trailing comments to exceed the 80 chars
> 
> In the past I expressed concerned with this kind of the rule because it is 
> not entirely clear how an automatic formatter will be able to check it.
> 
> Can you clarify whether clang-format would be able to handle your proposed 
> rule?

So, yesterday Bertrand pointed out a StackOverflow thread for this issue and if 
we use ReflowComments: false we should
be able to let the line as it is (not tested).

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#reflowcomments



> 
> Cheers,
> 
> -- 
> Julien Grall

[ANNOUNCE] Call for agenda items for 2 November Community Call @ 16:00 GMT

[Kelly Choi]

Hi all,

Please add your proposed agenda and name next to any items in this *link
here* 

If there are any action items that have been resolved, please remove them
from the sheet.

*COMMUNITY CALL INFORMATION*

*CALL LINK: https://meet.jit.si/XenProjectCommunityCall
*

*DATE: 1st Thursday of each month*

*TIME: 16:00 British Time (either BST or GMT)*
*To allow time to switch between meetings, we will start at 16:05.  Aim to
join by 16:00 if possible to allocate time for technical difficulties etc. *

*SIGN UP SHEET:* Please add or remove yourself from the sign-up-sheet to be
CC'd in these emails in this *link here
 *

--

*Dial-in info and pin can be found here:*

https://meet.jit.si/static/dialInInfo.html?room=XenProjectCommunityCall

*Meeting time:*

https://www.timeanddate.com/worldclock/meetingdetails.html?year=2023&month=11&day=2&hour=16&min=0&sec=0&p1=1234&p2=37&p3=224&p4=179

Many thanks,
Kelly Choi

Open Source Community Manager
XenServer, Cloud Software Group

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Julien Grall]

On 31/10/2023 15:32, Luca Fancellu wrote:




On 31 Oct 2023, at 15:27, Julien Grall  wrote:

Hi,

On 31/10/2023 15:12, Luca Fancellu wrote:

On 31 Oct 2023, at 15:10, Nicola Vetrini  wrote:

On 2023-10-31 15:13, Luca Fancellu wrote:

On 31 Oct 2023, at 13:27, Julien Grall  wrote:
Hi Stefano,
On 30/10/2023 22:49, Stefano Stabellini wrote:

On Mon, 30 Oct 2023, Julien Grall wrote:

Hi Nicola,
On 27/10/2023 16:11, Nicola Vetrini wrote:

diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 8511a189253b..81473fb4 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
   - __emulate_2op and __emulate_2op_nobyte
   - read_debugreg and write_debugreg
  +   * - R7.1
+ - It is safe to use certain octal constants the way they are defined
+   in specifications, manuals, and algorithm descriptions. Such places
+   are marked safe with a /\* octal-ok \*/ in-code comment, or with a
SAF
+   comment (see safe.json).

Reading this, it is unclear to me why we have two ways to deviate the rule
r7.1. And more importantely, how would the developper decide which one to use?

I agree with you on this and we were discussing this topic just this
morning in the FUSA community call. I think we need a way to do this
with the SAF framework:
if (some code with violation) /* SAF-xx-safe */
This doesn't work today unfortunately. It can only be done this way:
/* SAF-xx-safe */
if (some code with violation)
Which is not always desirable. octal-ok is just an ad-hoc solution for
one specific violation but we need a generic way to do this. Luca is
investigating possible ways to support the previous format in SAF.

Why can't we use octal-ok everywhere for now? My point here is to make simple 
for the developper to know what to use.

I think we should take this patch for now and harmonize it once SAF is
improved.

The description of the deviation needs some improvement. To give an example, 
with the current wording, one could they can use octal-ok everywhere. But 
above, you are implying that SAF-xx-safe should be
preferred.
I would still strongly prefer if we use octal-ok everywhere because this is 
simple to remember. But if the other are happy to have both SAF-XX and 
octal-ok, then the description needs to be completely unambiguous and the patch 
should contain some explanation why we have two different ways to deviate.

Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */
So that the suppression engine do what it should (currently it doesn’t suppress 
the same line, but we could do something about it) and the developer
has a way to understand what is the violation here without going to the 
justification database.


I guess. It could overflow the 80-char limit in xen/arch/x86/hvm/svm/svm.h, 
though.

Yeah, but we could rule out something in code_style to allow only this kind of 
trailing comments to exceed the 80 chars


In the past I expressed concerned with this kind of the rule because it is not 
entirely clear how an automatic formatter will be able to check it.

Can you clarify whether clang-format would be able to handle your proposed rule?


So, yesterday Bertrand pointed out a StackOverflow thread for this issue and if 
we use ReflowComments: false we should
be able to let the line as it is (not tested).


Wouldn't that prevent reflow for all the comments? If so, I don't think 
this is we want. Instead, we want to allow reflow for any comments but 
the one done at the end of the line.


Cheers,

--
Julien Grall

[xen-unstable test] 183626: regressions - FAIL

[osstest service owner]

flight 183626 xen-unstable real [real]
http://logs.test-lab.xenproject.org/osstest/logs/183626/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-pair 11 xen-install/dst_host fail REGR. vs. 183615
 test-amd64-i386-qemut-rhel6hvm-amd  7 xen-installfail REGR. vs. 183615

Tests which are failing intermittently (not blocking):
 test-amd64-i386-examine-uefi  6 xen-install  fail in 183620 pass in 183626
 test-amd64-i386-pair 10 xen-install/src_host fail in 183620 pass in 183626
 test-amd64-i386-examine-bios  6 xen-install  fail in 183620 pass in 183626
 test-amd64-i386-qemuu-rhel6hvm-amd  7 xen-install  fail pass in 183620

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemuu-win7-amd64 12 windows-install   fail like 183547
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 183566
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 183615
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 183615
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 183615
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 183615
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 183615
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 183615
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 183615
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 183615
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 183615
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 183615
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-i386-xl-pvshim14 guest-start  fail   never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-raw  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-armhf-armhf-xl-vhd  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl

Re: Commit moratorium for branching 4.18

[Henry Wang]

Hi,

> On Oct 25, 2023, at 18:18, Henry Wang  wrote:
> 
> Hi committers,
> 
> We will be branching the tree for Xen 4.18 in the next few days. Please 
> avoid committing any new patches to staging until further notice.

The branching has been finished. The staging is reopened for 4.19 development.

Thanks.

Kind regards,
Henry

> 
> Kind regards,
> Henry

Re: [PATCH v3 0/4] virtio-blk: use blk_io_plug_call() instead of notification BH

[Kevin Wolf]

Am 13.09.2023 um 22:00 hat Stefan Hajnoczi geschrieben:
> v3:
> - Add comment pointing to API documentation in .c file [Philippe]
> - Add virtio_notify_irqfd_deferred_fn trace event [Ilya]
> - Remove outdated #include [Ilya]
> v2:
> - Rename blk_io_plug() to defer_call() and move it to util/ so the net
>   subsystem can use it [Ilya]
> - Add defer_call_begin()/end() to thread_pool_completion_bh() to match Linux
>   AIO and io_uring completion batching
> 
> Replace the seldom-used virtio-blk notification BH mechanism with
> blk_io_plug(). This is part of an effort to enable the multi-queue block layer
> in virtio-blk. The notification BH was not multi-queue friendly.
> 
> The blk_io_plug() mechanism improves fio rw=randread bs=4k iodepth=64 
> numjobs=8
> IOPS by ~9% with a single IOThread and 8 vCPUs (this is not even a multi-queue
> block layer configuration) compared to no completion batching. iodepth=1
> decreases by ~1% but this could be noise. Benchmark details are available 
> here:
> https://gitlab.com/stefanha/virt-playbooks/-/tree/blk_io_plug-irqfd

Thanks, applied to the block branch.

Kevin

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Luca Fancellu]

> On 31 Oct 2023, at 15:36, Julien Grall  wrote:
> 
> 
> 
> On 31/10/2023 15:32, Luca Fancellu wrote:
>>> On 31 Oct 2023, at 15:27, Julien Grall  wrote:
>>> 
>>> Hi,
>>> 
>>> On 31/10/2023 15:12, Luca Fancellu wrote:
> On 31 Oct 2023, at 15:10, Nicola Vetrini  
> wrote:
> 
> On 2023-10-31 15:13, Luca Fancellu wrote:
>>> On 31 Oct 2023, at 13:27, Julien Grall  wrote:
>>> Hi Stefano,
>>> On 30/10/2023 22:49, Stefano Stabellini wrote:
 On Mon, 30 Oct 2023, Julien Grall wrote:
> Hi Nicola,
> On 27/10/2023 16:11, Nicola Vetrini wrote:
>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>> index 8511a189253b..81473fb4 100644
>> --- a/docs/misra/deviations.rst
>> +++ b/docs/misra/deviations.rst
>> @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
>>   - __emulate_2op and __emulate_2op_nobyte
>>   - read_debugreg and write_debugreg
>>  +   * - R7.1
>> + - It is safe to use certain octal constants the way they are 
>> defined
>> +   in specifications, manuals, and algorithm descriptions. Such 
>> places
>> +   are marked safe with a /\* octal-ok \*/ in-code comment, or 
>> with a
>> SAF
>> +   comment (see safe.json).
> Reading this, it is unclear to me why we have two ways to deviate the 
> rule
> r7.1. And more importantely, how would the developper decide which 
> one to use?
 I agree with you on this and we were discussing this topic just this
 morning in the FUSA community call. I think we need a way to do this
 with the SAF framework:
 if (some code with violation) /* SAF-xx-safe */
 This doesn't work today unfortunately. It can only be done this way:
 /* SAF-xx-safe */
 if (some code with violation)
 Which is not always desirable. octal-ok is just an ad-hoc solution for
 one specific violation but we need a generic way to do this. Luca is
 investigating possible ways to support the previous format in SAF.
>>> Why can't we use octal-ok everywhere for now? My point here is to make 
>>> simple for the developper to know what to use.
 I think we should take this patch for now and harmonize it once SAF is
 improved.
>>> The description of the deviation needs some improvement. To give an 
>>> example, with the current wording, one could they can use octal-ok 
>>> everywhere. But above, you are implying that SAF-xx-safe should be
>>> preferred.
>>> I would still strongly prefer if we use octal-ok everywhere because 
>>> this is simple to remember. But if the other are happy to have both 
>>> SAF-XX and octal-ok, then the description needs to be completely 
>>> unambiguous and the patch should contain some explanation why we have 
>>> two different ways to deviate.
>> Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */
>> So that the suppression engine do what it should (currently it doesn’t 
>> suppress the same line, but we could do something about it) and the 
>> developer
>> has a way to understand what is the violation here without going to the 
>> justification database.
> 
> I guess. It could overflow the 80-char limit in 
> xen/arch/x86/hvm/svm/svm.h, though.
 Yeah, but we could rule out something in code_style to allow only this 
 kind of trailing comments to exceed the 80 chars
>>> 
>>> In the past I expressed concerned with this kind of the rule because it is 
>>> not entirely clear how an automatic formatter will be able to check it.
>>> 
>>> Can you clarify whether clang-format would be able to handle your proposed 
>>> rule?
>> So, yesterday Bertrand pointed out a StackOverflow thread for this issue and 
>> if we use ReflowComments: false we should
>> be able to let the line as it is (not tested).
> 
> Wouldn't that prevent reflow for all the comments? If so, I don't think this 
> is we want. Instead, we want to allow reflow for any comments but the one 
> done at the end of the line.

Ok well, I was optimistic, in reality with the option as false, it would anyway 
reflow the line leaving the comment untouched.

E.g. from this:

if ( modrm_mod == MASK_EXTR(instr_modrm, 0300) && /* SAF-2-safe 
octal-ok */
 (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && /* SAF-2-safe 
octal-ok */
 (modrm_rm & 7) == MASK_EXTR(instr_modrm, 0007) ) /* SAF-2-safe 
octal-ok */
return emul_len;

To this:

if ( modrm_mod ==
 MASK_EXTR(instr_modrm, 0300) && /* SAF-2-safe octal-ok */
 (modrm_reg & 7) ==
 MASK_EXTR(instr_modrm, 0070) && /* SAF-2-safe octal-ok */
 (modrm_rm & 7) ==
 MASK_EXTR(instr_modrm, 0007) ) /* SAF-2-safe octal-ok

Re: Commit moratorium for branching 4.18

[Henry Wang]

Hi all,

> On Nov 1, 2023, at 00:01, Henry Wang  wrote:
> 
> Hi,
> 
>> On Oct 25, 2023, at 18:18, Henry Wang  wrote:
>> 
>> Hi committers,
>> 
>> We will be branching the tree for Xen 4.18 in the next few days. Please 
>> avoid committing any new patches to staging until further notice.
> 
> The branching has been finished. The staging is reopened for 4.19 development.

One small note, given the state that we definitely cannot freeze for another few
weeks and we are technically not 4.18 released, I would say currently the 
staging
is "soft-open”, basically "nothing that's likely to make backports to 4.18 
hard” would
be candidates to be pushed to staging now.

Kind regards,
Henry


> 
> Thanks.
> 
> Kind regards,
> Henry
> 
>> 
>> Kind regards,
>> Henry
> 

Re: [PATCH v4 4/5] [FUTURE] xen/arm: enable vPCI for domUs

[Jan Beulich]

On 31.10.2023 15:15, Stewart Hildebrand wrote:
> On 10/31/23 09:17, Julien Grall wrote:
>> On 31/10/2023 11:03, Jan Beulich wrote:
>>> On 31.10.2023 00:52, Stewart Hildebrand wrote:
 --- a/xen/drivers/passthrough/pci.c
 +++ b/xen/drivers/passthrough/pci.c
 @@ -1618,6 +1618,16 @@ int iommu_do_pci_domctl(
   bus = PCI_BUS(machine_sbdf);
   devfn = PCI_DEVFN(machine_sbdf);
   +    if ( IS_ENABLED(CONFIG_ARM) &&
 + !is_hardware_domain(d) &&
 + !is_system_domain(d) &&
 + (!IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) || !has_vpci(d)) 
 )
>>>
>>> I don't think you need the explicit ARM check; that's redundant with
>>> checking !HAS_VPCI_GUEST_SUPPORT.
> 
> Currently that is true. However, this is allowing for the possibility that we 
> eventually may want to enable PCI passthrough for PVH domU using vPCI (e.g. 
> hyperlaunch, or eliminating qemu backend), in which case we may want to 
> enable CONFIG_HAS_VPCI_GUEST_SUPPORT=y on x86.

That's precisely why I'd like to see the ARM check go away here.

>>> It's also not really clear why you
>>> need to check for the system domain here.
> 
> xl pci-assignable-add will assign the device to domIO, which doesn't have 
> vPCI, but it is still a valid assignment. Perhaps an in code comment would be 
> helpful for clarity?

And/or specifically check for DomIO, not any system domain.

Jan

[RFC PATCH v3 0/3] Add a new acquire resource to query vcpu statistics

[Matias Ezequiel Vara Larsen]

Hello all and apologies for the delay in sending v3,

the purpose of this RFC is to get feedback about a new acquire resource that
exposes vcpu statistics for a given domain. The current mechanism to get those
statistics is by querying the hypervisor. This mechanism relies on a hypercall
and holds the domctl spinlock during its execution. When a pv tool like xcp-rrdd
periodically samples these counters, it ends up affecting other paths that share
that spinlock. By using acquire resources, the pv tool only requires a few
hypercalls to set the shared memory region and samples are got without issuing
any other hypercall. The original idea has been suggested by Andrew Cooper to
which I have been discussing about how to implement the current PoC. You can
find the RFC patch series at [1]. The series is rebased on top of stable-4.16.

The current series includes a simple pv tool that shows how this new interface 
is
used. This tool maps the counter and periodically samples it.

Any feedback/help would be appreciated.

Thanks, Matias.

[1] https://github.com/MatiasVara/xen/commits/feature_vcpu_stats

Changes in v3:
- use memory layout discussed at
  https://lists.xenproject.org/archives/html/xen-devel/2023-03/msg00383.html
 
Changes in v2:
- rework to ensure that consumer fetches consistent data

Changes in v1:
- rework how the resource is allocated and released
- rework when the resource is allocated that happens only when the resource is
  requested 
- rework the structure shared between the tool and Xen to make it extensible to
  new counters and declare it in a public header

There are still the following questions:
   - resource shall be released when there are no more readers otherwise we keep
 updating it during a hot path

Matias Ezequiel Vara Larsen (3):
  xen/memory : Add a stats_table resource type
  x86/mm: Do not validate/devalidate PGT_none type
  tools/misc: Add xen-vcpus-stats tool

 tools/misc/Makefile  |   6 ++
 tools/misc/xen-vcpus-stats.c | 132 +++
 xen/arch/x86/mm.c|   3 +-
 xen/common/domain.c  |   1 +
 xen/common/memory.c  | 167 +++
 xen/common/sched/core.c  |  20 +
 xen/include/public/memory.h  |   3 +
 xen/include/public/vcpu.h|  27 ++
 xen/include/xen/mm.h |   2 +
 xen/include/xen/sched.h  |   5 ++
 10 files changed, 365 insertions(+), 1 deletion(-)
 create mode 100644 tools/misc/xen-vcpus-stats.c

-- 
2.34.1

[RFC PATCH v3 1/3] xen/memory : Add a stats_table resource type

[Matias Ezequiel Vara Larsen]

This commit proposes a new mechanism to query the RUNSTATE_running counter for
a given vcpu from a dom0 userspace application. This commit proposes to expose
that counter by using the acquire_resource interface. For this purpose, the
commit adds a new resource named XENMEM_resource_stats_table and a
type-specific resource named XENMEM_resource_stats_table_id_vcpustats. This
type-specific resource is aiming at exposing per-VCPU counters.

The current mechanism relies on the XEN_DOMCTL_getvcpuinfo and holds a single
global domctl_lock for the entire hypercall; and iterate over every vcpu in the
system for every update thus impacting operations that share that lock. This
commit proposes to expose vcpu RUNSTATE_running via the xenforeignmemory
interface thus preventing to issue the hypercall and holding the lock.

For that purpose, a new resource type named XENMEM_resource_stats_table is
added. In particular, the type-specific resource
XENMEM_resource_stats_table_id_vcpustats is added to host per-VCPU counters.
These counters are mapped in frames. The allocation of these frames only
happens if the resource is requested. The number of allocated frames is equal
to the domain's max_vcpus. Frames are released after the domain is destroyed.

To expose this information to a consumer, two structures are required:
1. vcpu_shmem_stats
2. vcpu_stats[]

The memory layout has been discussed at [1]. The laylout looks like:

struct vcpu_shmem_stats {
#define VCPU_STATS_MAGIC 0xaabbccdd
uint32_t magic;
uint32_t offset;  // roundup(sizeof(vcpu_shmem_stats), cacheline_size)
uint32_t size;// sizeof(vcpu_stats)
uint32_t stride;  // roundup(sizeof(vcpu_stats), cacheline_size)
};

struct vcpu_stats {
/*
 * If the least-significant bit of the seq number is set then an update
 * is in progress and the consumer must wait to read a consistent set of
 * values. This mechanism is similar to Linux's seqlock.
 */
uint32_t seq;
uint32 _pad;
/*
 * If the most-significant bit of a counter is set then the counter
 * is inactive and the consumer must ignore its value. Note that this
 * could also indicate that the counter has overflowed.
 */
uint64_t stats_a; // e.g., runstate_running_time
uint64_t stats_b; // e.g.,
uint64_t stats_c; // e.g.,
...
};

All padding fields shall be marked as "inactive". The consumer can't
distinguish inactive from overflowed. Also, the consumer shall always
verify before reading that:

  offsetof(struct vcpu_stats, stats_y) < size.

in case the consumer knows about a counter, e.g., stats_y, that Xen does
not it.

The vcpu_shmem_stats structure exposes a magic number, the size of the
vcpu_stats structure, the offset in which the vcpu_stats structures begin and
the stride for each instance. The address of the vcpu_shmem_stats and
vcpu_stats instances are cache line aligned to prevent cache ping-pong when
accessing per-vcpu elements. In the vcpu_stats structure, most-significant bit
of a counter is set to indicate that either the counter has overflowed or it is
inactive so the consumer must ignore it.

Note that the updating of vcpu's counters is in a hot path, thus, in this 
commit,
copying only happens if it is specifically required.

Note that the resource is extensible in two ways. First, the structure
vcpu_stats can be extended with new per-vcpu counters. To do so, new per-vcpu
counters shall be added after the last element of the structure definition to
not break existing consumers. Second, new type-specific resources can be added
to host a different sort of counters.

[1]
https://lists.xenproject.org/archives/html/xen-devel/2023-03/msg00383.html

Signed-off-by: Matias Ezequiel Vara Larsen 
---
Changes in v3:
- allow to host an arbitrary nuumber of vcpus
- release resource during domain_kill() since it is guest-type
  independent and arch agnostic
- rework stats_table_max_frames()
- use memory layout as discussed at
  https://lists.xenproject.org/archives/html/xen-devel/2023-03/msg00383.html

Changes in v2:
- rework to ensure that guest reads a coherent value by using a version
  number in the vcpu_stats structure
- add version to the vcpu_stats structure

Changes in v1:
- rework the allocation and releasing of the frames
- use the zero frame for per-vcpu counters that are listed as an array
- allocate vcpu stats frames only when the resource is requested
- rewrite commit message
- add the vcpu_stats structure to keep per-vcpu counters
- add the shared_vcpustatspage to keep an array of per-vcpu counters for a
  given domain
- declare the structures in a public header 
- define the vcpustats_page in the domain structure
---
 xen/common/domain.c |   1 +
 xen/common/memory.c | 167 
 xen/common/sched/core.c |  20 +
 xen/include/public/memory.h |   3 +
 xen/include/public/vcpu.h   |  27 ++
 xen/include/xen/mm.h|   2 +
 xen/include/xen/sched.h 

[RFC PATCH v3 2/3] x86/mm: Do not validate/devalidate PGT_none type

[Matias Ezequiel Vara Larsen]

This commit prevents PGT_none type pages to be validated/devalidated.
This is required for the use-case in which a guest-agnostic resource is
allocated. In this case, these pages may be accessible by an "owning" PV
domain. To lock the page from guest pov, pages are required to be marked
with PGT_none with a single type ref. In particular, this commit makes
the stats_table resource type to use this flag during
get_page_and_type(). 

Signed-off-by: Matias Ezequiel Vara Larsen 
---
 xen/arch/x86/mm.c   | 3 ++-
 xen/common/memory.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 5812321cae..d2f311abe4 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2787,6 +2787,7 @@ static int _put_page_type(struct page_info *page, 
unsigned int flags,
 {
 case 0:
 if ( unlikely((nx & PGT_type_mask) <= PGT_l4_page_table) &&
+ unlikely((nx & PGT_type_mask) >= PGT_l1_page_table) &&
  likely(nx & (PGT_validated|PGT_partial)) )
 {
 int rc;
@@ -3072,7 +3073,7 @@ static int _get_page_type(struct page_info *page, 
unsigned long type,
  *
  * per validate_page(), non-atomic updates are fine here.
  */
-if ( type == PGT_writable_page || type == PGT_shared_page )
+if ( type == PGT_writable_page || type == PGT_shared_page || type == 
PGT_none )
 page->u.inuse.type_info |= PGT_validated;
 else
 {
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 2acac40c63..e26ba21d75 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1254,7 +1254,7 @@ static int stats_vcpu_alloc_mfn(struct domain *d)
 
 for ( i = 0; i < nr_frames; i++ )
 {
-if ( unlikely(!get_page_and_type(&pg[i], d, PGT_writable_page)) )
+if ( unlikely(!get_page_and_type(&pg[i], d, PGT_none)) )
 /*
  * The domain can't possibly know about this page yet, so failure
  * here is a clear indication of something fishy going on.
-- 
2.34.1

[RFC PATCH v3 3/3] tools/misc: Add xen-vcpus-stats tool

[Matias Ezequiel Vara Larsen]

Add a demonstration tool that uses the stats_table resource to
query vcpus' RUNSTATE_running counter for a DomU.

Signed-off-by: Matias Ezequiel Vara Larsen 
---
Changes in v3:
- use memory layout as discussed at
  https://lists.xenproject.org/archives/html/xen-devel/2023-03/msg00383.html
- use virt_*()
- issue xenforeignmemory_close()

Changes in v2:
- use period instead of frec
- rely on version to ensure reading is coherent 

Changes in v1:
- change the name of the tool to xen-vcpus-stats
- set command line parameters in the same order that are passed
- remove header libs.h
- build by default
- remove errno, strerrno, "\n", and identation
- use errx when errno is not needed
- address better the number of pages requested and error msgs
- use the shared_vcpustatspage_t structure
- use the correct frame id when requesting the resource
---
 tools/misc/Makefile  |   6 ++
 tools/misc/xen-vcpus-stats.c | 132 +++
 2 files changed, 138 insertions(+)
 create mode 100644 tools/misc/xen-vcpus-stats.c

diff --git a/tools/misc/Makefile b/tools/misc/Makefile
index 8b9558b93f..9c7cb50483 100644
--- a/tools/misc/Makefile
+++ b/tools/misc/Makefile
@@ -51,6 +51,7 @@ TARGETS_COPY += xenpvnetboot
 
 # Everything which needs to be built
 TARGETS_BUILD := $(filter-out $(TARGETS_COPY),$(TARGETS_ALL))
+TARGETS_BUILD += xen-vcpus-stats
 
 # ... including build-only targets
 TARGETS_BUILD += $(TARGETS_BUILD-y)
@@ -139,4 +140,9 @@ xencov: xencov.o
 xen-ucode: xen-ucode.o
$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
 
+xen-vcpus-stats.o: CFLAGS += $(CFLAGS_libxenforeginmemory)
+
+xen-vcpus-stats: xen-vcpus-stats.o
+   $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) 
$(LDLIBS_libxenforeignmemory) $(APPEND_LDFLAGS)
+
 -include $(DEPS_INCLUDE)
diff --git a/tools/misc/xen-vcpus-stats.c b/tools/misc/xen-vcpus-stats.c
new file mode 100644
index 00..f277b6ce8f
--- /dev/null
+++ b/tools/misc/xen-vcpus-stats.c
@@ -0,0 +1,132 @@
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+/*
+ * Note that virt_*() is used when ordering is required between the hypevisor
+ * and the tool domain. This tool is meant to be arch-agnostic so add the
+ * corresponding barrier for each architecture.
+ *
+ */
+#if defined(__x86_64__)
+#define barrier() asm volatile("" ::: "memory")
+#define virt_rmb() barrier()
+#elif defined(__aarch64__)
+#define dmb(opt) asm volatile("dmb " #opt : : : "memory")
+#define virt_rmb() dmb(ishld)
+#else
+#error Please fill in barrier macros
+#endif
+
+static sig_atomic_t interrupted;
+static void close_handler(int signum)
+{
+interrupted = 1;
+}
+
+int main(int argc, char **argv)
+{
+xenforeignmemory_handle *fh;
+xenforeignmemory_resource_handle *res;
+size_t size;
+int rc, domid, period, vcpu;
+xen_vcpu_shmemstats_t *info_shmem;
+xen_shared_vcpustats_t *info;
+struct sigaction act;
+uint32_t seq;
+uint64_t value;
+
+if ( argc != 4 )
+{
+fprintf(stderr, "Usage: %s   \n", argv[0]);
+return 1;
+}
+
+domid = atoi(argv[1]);
+vcpu = atoi(argv[2]);
+period = atoi(argv[3]);
+
+act.sa_handler = close_handler;
+act.sa_flags = 0;
+sigemptyset(&act.sa_mask);
+sigaction(SIGHUP,  &act, NULL);
+sigaction(SIGTERM, &act, NULL);
+sigaction(SIGINT,  &act, NULL);
+sigaction(SIGALRM, &act, NULL);
+
+fh = xenforeignmemory_open(NULL, 0);
+
+if ( !fh )
+err(1, "xenforeignmemory_open");
+
+rc = xenforeignmemory_resource_size(
+fh, domid, XENMEM_resource_stats_table,
+XENMEM_resource_stats_table_id_vcpustats, &size);
+
+if ( rc )
+err(1, "Fail: Get size");
+
+res = xenforeignmemory_map_resource(
+fh, domid, XENMEM_resource_stats_table,
+XENMEM_resource_stats_table_id_vcpustats, 0, size >> XC_PAGE_SHIFT,
+(void **)&info_shmem, PROT_READ, 0);
+
+if ( !res )
+err(1, "Fail: Map");
+
+if ( info_shmem->magic != VCPU_STATS_MAGIC )
+{
+fprintf(stderr, "Wrong magic number\n");
+return 1;
+}
+
+if ( offsetof(struct vcpu_stats, runstate_running_time) > info_shmem->size 
)
+{
+fprintf(stderr, "The counter is not produced\n");
+return 1;
+}
+
+info = (xen_shared_vcpustats_t*)((void*)info_shmem
+ + info_shmem->offset
+ + info_shmem->size * vcpu);
+
+if ( info->runstate_running_time & ((uint64_t)1 << 63) )
+{
+fprintf(stderr, "The counter is inactived or has overflowed\n");
+return 1;
+}
+
+while ( !interrupted )
+{
+sleep(period);
+do {
+seq = info[vcpu].seq;
+virt_rmb();
+value = info[vcpu].runstate_running_time;
+virt_rmb();
+} while ( (info[vcpu].seq & 1) ||
+  (seq != in

Re: [PATCH] x86/irq: do not insert IRQ_MSI_EMU in emuirq mappings

[Roger Pau Monné]

On Tue, Oct 31, 2023 at 03:30:37PM +0200, Xenia Ragiadakou wrote:
> Do not use emuirq mappings for MSIs injected by emulated devices.
> This kind of pirq shares the same emuirq value and is not remapped.

AFAICT adding the extra emuirq mappings is harmless, and just adds
an extra layer of translation?

Or is this causing issues, but we haven't realized because we don't
provide emulated devices that use MSI(-X) by default?

> Fixes: 88fccdd11ca0 ('xen: event channel remapping for emulated MSIs')
> Signed-off-by: Xenia Ragiadakou 
> ---
> 
> Question: is there any strong reason why Linux HVM guests still use pirqs?

Baggage I guess.  I've suggested in the past to switch PIRQs off by
default for HVM, but I had no figures to show how much of a
performance penalty that would be for passthrough devices.

My suggestion would be to introduce an xl.cfg option to select the
availability of PIRQs for HVM guests, and set it to off by default.
You would also need to make sure that migration (or save/restore)
works fine, and that incoming guests from previous Xen versions (that
won't have the option) will result in PIRQs still being enabled.

There's already a XEN_X86_EMU_USE_PIRQ flag in xen_arch_domainconfig,
so you just need to wire the tools side in order to allow selection by
users.

> 
>  xen/arch/x86/irq.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index f42ad539dc..cdc8dc5a55 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2684,7 +2684,7 @@ int map_domain_emuirq_pirq(struct domain *d, int pirq, 
> int emuirq)
>  }
>  
>  old_emuirq = domain_pirq_to_emuirq(d, pirq);
> -if ( emuirq != IRQ_PT )
> +if ( (emuirq != IRQ_PT) && (emuirq != IRQ_MSI_EMU) )
>  old_pirq = domain_emuirq_to_pirq(d, emuirq);

I think you can just use emuirq >= 0, as we then only need the emuirq
translation for passthrough interrupts, same for the rest of the
changed conditions.

Looking further, the function seems to be useless when called with
emuirq < 0, and hence it might be better to avoid such calls in the
first place?

I have to admit I've always been very confused with the PIRQ logic, so
it's possible I'm missing some relevant stuff here.

Thanks, Roger.

Re: [PATCH v4 2/5] xen/vpci: move xen_domctl_createdomain vPCI flag to common

[Stewart Hildebrand]

On 10/31/23 06:56, Jan Beulich wrote:
> On 31.10.2023 00:52, Stewart Hildebrand wrote:
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -607,7 +607,8 @@ int arch_sanitise_domain_config(struct 
>> xen_domctl_createdomain *config)
>>  {
>>  unsigned int max_vcpus;
>>  unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap);
>> -unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | 
>> XEN_DOMCTL_CDF_vpmu);
>> +unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | 
>> XEN_DOMCTL_CDF_vpmu |
>> +   XEN_DOMCTL_CDF_vpci);
> 
> Is the flag (going to be, with the initial work) okay to have for Dom0
> on Arm?

Hm. Allowing/enabling vPCI for dom0 on ARM should follow or be part of the PCI 
passthrough SMMU series [1]. I'll move this change to the next patch ("xen/arm: 
enable vPCI for dom0") and add a note over there.

[1] https://lists.xenproject.org/archives/html/xen-devel/2023-10/msg00210.html

> 
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -712,7 +712,8 @@ int arch_sanitise_domain_config(struct 
>> xen_domctl_createdomain *config)
>>  return 0;
>>  }
>>  
>> -static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>> +static bool emulation_flags_ok(const struct domain *d, uint32_t emflags,
>> +   uint32_t cdf)
> 
> While apparently views differ, ./CODING_STYLE wants "unsigned int" to be
> used for the latter two arguments.

OK, I'll change both to unsigned int.

> 
>> @@ -722,14 +723,17 @@ static bool emulation_flags_ok(const struct domain *d, 
>> uint32_t emflags)
>>  if ( is_hvm_domain(d) )
>>  {
>>  if ( is_hardware_domain(d) &&
>> - emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC) )
>> + (!( cdf & XEN_DOMCTL_CDF_vpci ) ||
> 
> Nit: Stray blanks inside the inner parentheses.

OK, I'll fix.

> 
>> +  emflags != (X86_EMU_LAPIC | X86_EMU_IOAPIC)) )
>>  return false;
>>  if ( !is_hardware_domain(d) &&
>> - emflags != (X86_EMU_ALL & ~X86_EMU_VPCI) &&
>> - emflags != X86_EMU_LAPIC )
>> + ((cdf & XEN_DOMCTL_CDF_vpci) ||
>> +  (emflags != X86_EMU_ALL &&
>> +   emflags != X86_EMU_LAPIC)) )
>>  return false;
>>  }
>> -else if ( emflags != 0 && emflags != X86_EMU_PIT )
>> +else if ( (cdf & XEN_DOMCTL_CDF_vpci) ||
> 
> Wouldn't this better be enforced in common code?

Yes, I will move it to xen/common/domain.c:sanitise_domain_config().

> 
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -892,10 +892,11 @@ static struct domain *__init create_dom0(const 
>> module_t *image,
>>  {
>>  dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm |
>> ((hvm_hap_supported() && !opt_dom0_shadow) ?
>> -XEN_DOMCTL_CDF_hap : 0));
>> +XEN_DOMCTL_CDF_hap : 0) |
>> +   XEN_DOMCTL_CDF_vpci);
> 
> Less of a change and imo slightly neater as a result would be to simply
> put the addition on the same line where CDF_hvm already is. But as with
> many style aspects, views may differ here of course ...

I'll change it.

> 
> Jan