[ovmf test] 183200: all pass - PUSHED

[osstest service owner]

flight 183200 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/183200/

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf f36e1ec1f0a5fd3be84913e09181d7813444b620
baseline version:
 ovmf ad1c0394b1770315099e511de7c88a04d7af76f2

Last test of basis   183190  2023-09-27 05:23:10 Z1 days
Testing same since   183200  2023-09-28 02:10:46 Z0 days1 attempts


People who touched revisions under test:
  Gao Cheng 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 pass
 test-amd64-i386-xl-qemuu-ovmf-amd64  pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/osstest/ovmf.git
   ad1c0394b1..f36e1ec1f0  f36e1ec1f0a5fd3be84913e09181d7813444b620 -> 
xen-tested-master

[xen-unstable-smoke test] 183197: tolerable all pass - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   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-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  a363089e68ed677fef12c296253535753fe817e3
baseline version:
 xen  a1f8b32af001c15ce3c6be2364826b1ca35b6caa

Last test of basis   183193  2023-09-27 11:02:05 Z0 days
Testing same since   183197  2023-09-28 00:02:01 Z0 days1 attempts


People who touched revisions under test:
  Shawn Anastasio 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-amd64-libvirt pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   a1f8b32af0..a363089e68  a363089e68ed677fef12c296253535753fe817e3 -> smoke

[linux-5.4 test] 183192: regressions - FAIL

[osstest service owner]

flight 183192 linux-5.4 real [real]
flight 183198 linux-5.4 real-retest [real]
http://logs.test-lab.xenproject.org/osstest/logs/183192/
http://logs.test-lab.xenproject.org/osstest/logs/183198/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-armhf-armhf-xl-credit1  19 guest-start.2  fail in 183144 REGR. vs. 182613

Tests which are failing intermittently (not blocking):
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 12 debian-hvm-install fail in 
183144 pass in 183192
 test-armhf-armhf-libvirt-qcow2 17 guest-start/debian.repeat fail in 183144 
pass in 183192
 test-armhf-armhf-xl-credit1 18 guest-start/debian.repeat fail in 183183 pass 
in 183144
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 7 xen-install fail pass in 
183183
 test-armhf-armhf-xl-credit2  14 guest-startfail pass in 183183
 test-armhf-armhf-xl-credit1  14 guest-startfail pass in 183183

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-xl-multivcpu 18 guest-start/debian.repeat fail blocked in 
182613
 test-armhf-armhf-xl-rtds 18 guest-start/debian.repeat fail in 183144 like 
182613
 test-armhf-armhf-xl-credit2 18 guest-start/debian.repeat fail in 183183 like 
182613
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail in 183183 never pass
 test-armhf-armhf-xl-credit2 15 migrate-support-check fail in 183183 never pass
 test-armhf-armhf-xl-credit2 16 saverestore-support-check fail in 183183 never 
pass
 test-armhf-armhf-xl-credit1 15 migrate-support-check fail in 183183 never pass
 test-armhf-armhf-xl-credit1 16 saverestore-support-check fail in 183183 never 
pass
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 182613
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 182613
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 182613
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 182613
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 182613
 test-armhf-armhf-xl-arndale  18 guest-start/debian.repeatfail  like 182613
 test-armhf-armhf-xl  18 guest-start/debian.repeatfail  like 182613
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 182613
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 182613
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 182613
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 182613
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 182613
 test-armhf-armhf-libvirt-raw 17 guest-start/debian.repeatfail  like 182613
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 182613
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 182613
 test-amd64-i386-xl-pvshim14 guest-start  fail   never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-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-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  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-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-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail 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-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 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
 

Re: [XEN PATCH v2 1/3] docs/misra: add documentation skeleton for MISRA C:2012 Dir 4.1

[Stefano Stabellini]

On Wed, 27 Sep 2023, Nicola Vetrini wrote:
> The aforementioned directive requires the project to supply documentation
> on the measures taken towards the minimization of run-time failures.
> 
> The actual content of the documentation still needs feedback from the
> community.
> 
> The 'rules.rst' file is updated accordingly to mention the newly
> added documentation.
> 
> Signed-off-by: Nicola Vetrini 
> ---
> Changes in v2:
> - Incorporated suggestions from Stefano.
> ---
>  docs/misra/C-runtime-failures.rst | 200 ++
>  docs/misra/rules.rst  |   8 +-
>  2 files changed, 207 insertions(+), 1 deletion(-)
>  create mode 100644 docs/misra/C-runtime-failures.rst
> 
> diff --git a/docs/misra/C-runtime-failures.rst 
> b/docs/misra/C-runtime-failures.rst
> new file mode 100644
> index ..325d3fab1fa5
> --- /dev/null
> +++ b/docs/misra/C-runtime-failures.rst
> @@ -0,0 +1,200 @@
> +===
> +Measures taken towards the minimization of Run-time failures in Xen
> +===
> +
> +This document specifies which procedures and techinques are used troughout 
> the
> +Xen codebase to prevent or minimize the impact of certain classes of run-time
> +errors that can occurr in the execution of a C program, due to the very 
> minimal
> +built-in checks that are present in the language.
> +
> +The presence of such documentation is requested by MISRA C:2012 Directive 
> 4.1,
> +whose headline states: "Run-time failures shall be minimized".
> +
> +
> +Documentation for MISRA C:2012 Dir 4.1: overflow
> +
> +
> +Pervasive use of assertions and extensive test suite.
> +
> +
> +Documentation for MISRA C:2012 Dir 4.1: unexpected wrapping
> +___
> +
> +The only wrapping the is present in the code concerns
 ^ that


> +unsigned integers and they are all expected.
> +
> +
> +Documentation for MISRA C:2012 Dir 4.1: invalid shift
> +_
> +
> +Pervasive use of assertions and extensive test suite.
> +
> +
> +Documentation for MISRA C:2012 Dir 4.1: division/remainder by zero
> +__
> +
> +The division or remainder operations in the project code ensure that
> +their second argument is never zero.
> +
> +
> +Documentation for MISRA C:2012 Dir 4.1: unsequenced side effects
> +
> +
> +Code executed in interrupt handlers uses spinlocks or disables interrupts
> +at the right locations to avoid unsequenced side effects.
> +
> +
> +Documentation for MISRA C:2012 Dir 4.1: read from uninitialized automatic 
> object
> +
> +
> +The amount of dynamically allocated objects is limited at runtime in
> +static configurations. We make sure to initialize dynamically allocated
> +objects before reading them, and we utilize static analysis tools to
> +help check for that.
> +
> +
> +Documentation for MISRA C:2012 Dir 4.1: read from uninitialized allocated 
> object
> +
> +
> +Dynamically allocated storage is used in a controlled manner, to prevent the
> +access to uninitialized allocated storage.
> +
> +
> +Documentation for MISRA C:2012 Dir 4.1: write to string literal or const 
> object
> +___
> +
> +The toolchain puts every string literal and const object into a read-only
> +section of memory.  The hardware exception raised when a write is attempted
> +on such a memory section is correctly handled.
> +
> +
> +Documentation for MISRA C:2012 Dir 4.1: non-volatile access to volatile 
> object
> +__
> +
> +Volatile access is limited to registers that are always accessed
> +through macros or inline functions, or by limited code chunks that are only 
> used
> +to access a register.
> +
> +
> +Documentation for MISRA C:2012 Dir 4.1: access to dead allocated object
> +___
> +
> +Although dynamically allocated storage is used in the project, in safety
> +configurations its usage is very limited at runtime (it is "almost" only used
> +at boot time). Coverity is regularly used to scan the code to detect 
> non-freed
> +allocated objects.
> +
> +
> +Documentation for MISRA C:2012 Dir 4.1: access to dead automatic object
> +___
> +
> +Pointers to automatic variables are never returned, nor stored in
> +wider-scoped objects.  No function does the same on any pointer
> 

Re: [XEN PATCH v2 2/3] docs: make the docs for MISRA C:2012 Dir 4.1 visible to ECLAIR

[Henry Wang]

> On Sep 28, 2023, at 08:49, Stefano Stabellini  wrote:
> 
> On Wed, 27 Sep 2023, Nicola Vetrini wrote:
>> To be able to check for the existence of the necessary subsections in
>> the documentation for MISRA C:2012 Dir 4.1, ECLAIR needs to have a source
>> file that is built.
>> 
>> This file is generated from 'C-runtime-failures.rst' in docs/misra
>> and the configuration is updated accordingly.
>> 
>> Signed-off-by: Nicola Vetrini 
> 
> Reviewed-by: Stefano Stabellini 

Release-acked-by: Henry Wang 

Kind regards,
Henry
> 
> 
>> ---
>> Changes from RFC:
>> - Dropped unused/useless code
>> - Revised the sed command
>> - Revised the clean target
>> 
>> Changes in v2:
>> - Added explanative comment to the makefile
>> - printf instead of echo
>> ---
>> docs/Makefile   |  7 ++-
>> docs/misra/Makefile | 22 ++
>> 2 files changed, 28 insertions(+), 1 deletion(-)
>> create mode 100644 docs/misra/Makefile
>> 
>> diff --git a/docs/Makefile b/docs/Makefile
>> index 966a104490ac..ff991a0c3ca2 100644
>> --- a/docs/Makefile
>> +++ b/docs/Makefile
>> @@ -43,7 +43,7 @@ DOC_PDF  := $(patsubst %.pandoc,pdf/%.pdf,$(PANDOCSRC-y)) \
>> all: build
>> 
>> .PHONY: build
>> -build: html txt pdf man-pages figs
>> +build: html txt pdf man-pages figs misra
>> 
>> .PHONY: sphinx-html
>> sphinx-html:
>> @@ -66,9 +66,14 @@ endif
>> .PHONY: pdf
>> pdf: $(DOC_PDF)
>> 
>> +.PHONY: misra
>> +misra:
>> + $(MAKE) -C misra
>> +
>> .PHONY: clean
>> clean: clean-man-pages
>> $(MAKE) -C figs clean
>> + $(MAKE) -C misra clean
>> rm -rf .word_count *.aux *.dvi *.bbl *.blg *.glo *.idx *~
>> rm -rf *.ilg *.log *.ind *.toc *.bak *.tmp core
>> rm -rf html txt pdf sphinx/html
>> diff --git a/docs/misra/Makefile b/docs/misra/Makefile
>> new file mode 100644
>> index ..8fd89404e96b
>> --- /dev/null
>> +++ b/docs/misra/Makefile
>> @@ -0,0 +1,22 @@
>> +TARGETS := C-runtime-failures.o
>> +
>> +all: $(TARGETS)
>> +
>> +# This Makefile will generate the object files indicated in TARGETS by 
>> taking
>> +# the corresponding .rst file, converting its content to a C block comment 
>> and
>> +# then compiling the resulting .c file. This is needed for the file's 
>> content to
>> +# be available when performing static analysis with ECLAIR on the project.
>> +
>> +# sed is used in place of cat to prevent occurrences of '*/'
>> +# in the .rst from breaking the compilation
>> +$(TARGETS:.o=.c): %.c: %.rst
>> + printf "/*\n\n" > $@.tmp
>> + sed -e 's|\*/|*//*|g' $< >> $@.tmp
>> + printf "\n\n*/" >> $@.tmp
>> + mv $@.tmp $@
>> +
>> +%.o: %.c
>> + $(CC) -c $< -o $@
>> +
>> +clean:
>> + rm -f C-runtime-failures.c *.o *.tmp
>> -- 
>> 2.34.1
>> 

Re: [XEN PATCH v2 2/3] docs: make the docs for MISRA C:2012 Dir 4.1 visible to ECLAIR

[Stefano Stabellini]

On Wed, 27 Sep 2023, Nicola Vetrini wrote:
> To be able to check for the existence of the necessary subsections in
> the documentation for MISRA C:2012 Dir 4.1, ECLAIR needs to have a source
> file that is built.
> 
> This file is generated from 'C-runtime-failures.rst' in docs/misra
> and the configuration is updated accordingly.
> 
> Signed-off-by: Nicola Vetrini 

Reviewed-by: Stefano Stabellini 


> ---
> Changes from RFC:
> - Dropped unused/useless code
> - Revised the sed command
> - Revised the clean target
> 
> Changes in v2:
> - Added explanative comment to the makefile
> - printf instead of echo
> ---
>  docs/Makefile   |  7 ++-
>  docs/misra/Makefile | 22 ++
>  2 files changed, 28 insertions(+), 1 deletion(-)
>  create mode 100644 docs/misra/Makefile
> 
> diff --git a/docs/Makefile b/docs/Makefile
> index 966a104490ac..ff991a0c3ca2 100644
> --- a/docs/Makefile
> +++ b/docs/Makefile
> @@ -43,7 +43,7 @@ DOC_PDF  := $(patsubst %.pandoc,pdf/%.pdf,$(PANDOCSRC-y)) \
>  all: build
>  
>  .PHONY: build
> -build: html txt pdf man-pages figs
> +build: html txt pdf man-pages figs misra
>  
>  .PHONY: sphinx-html
>  sphinx-html:
> @@ -66,9 +66,14 @@ endif
>  .PHONY: pdf
>  pdf: $(DOC_PDF)
>  
> +.PHONY: misra
> +misra:
> + $(MAKE) -C misra
> +
>  .PHONY: clean
>  clean: clean-man-pages
>   $(MAKE) -C figs clean
> + $(MAKE) -C misra clean
>   rm -rf .word_count *.aux *.dvi *.bbl *.blg *.glo *.idx *~
>   rm -rf *.ilg *.log *.ind *.toc *.bak *.tmp core
>   rm -rf html txt pdf sphinx/html
> diff --git a/docs/misra/Makefile b/docs/misra/Makefile
> new file mode 100644
> index ..8fd89404e96b
> --- /dev/null
> +++ b/docs/misra/Makefile
> @@ -0,0 +1,22 @@
> +TARGETS := C-runtime-failures.o
> +
> +all: $(TARGETS)
> +
> +# This Makefile will generate the object files indicated in TARGETS by taking
> +# the corresponding .rst file, converting its content to a C block comment 
> and
> +# then compiling the resulting .c file. This is needed for the file's 
> content to
> +# be available when performing static analysis with ECLAIR on the project.
> +
> +# sed is used in place of cat to prevent occurrences of '*/'
> +# in the .rst from breaking the compilation
> +$(TARGETS:.o=.c): %.c: %.rst
> + printf "/*\n\n" > $@.tmp
> + sed -e 's|\*/|*//*|g' $< >> $@.tmp
> + printf "\n\n*/" >> $@.tmp
> + mv $@.tmp $@
> +
> +%.o: %.c
> + $(CC) -c $< -o $@
> +
> +clean:
> + rm -f C-runtime-failures.c *.o *.tmp
> -- 
> 2.34.1
> 

Re: [PATCH v3] docs/misra: add rule 2.1 exceptions

[Stefano Stabellini]

On Wed, 27 Sep 2023, Bertrand Marquis wrote:
> > On 27 Sep 2023, at 09:53, Nicola Vetrini  wrote:
> > 
> >>> > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> >>> > index 695d2fa1f1..2a8527cacc 100644
> >>> > --- a/xen/arch/arm/psci.c
> >>> > +++ b/xen/arch/arm/psci.c
> >>> > @@ -59,6 +59,7 @@ void call_psci_cpu_off(void)
> >>> >   }
> >>> >   }
> >>> >   +/* SAF-2-safe */
> >>> I think any use of SAF-2-safe should be accompanied with an attribute...
> >>> >   void call_psci_system_off(void)
> >>> ... noreturn for function or ...
> >>> >   {
> >>> >   if ( psci_ver > PSCI_VERSION(0, 1) )
> >>> > diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c
> >>> > index 7619544d14..47e0f59024 100644
> >>> > --- a/xen/arch/x86/shutdown.c
> >>> > +++ b/xen/arch/x86/shutdown.c
> >>> > @@ -118,6 +118,7 @@ static inline void kb_wait(void)
> >>> >   break;
> >>> >   }
> >>> >   +/* SAF-2-safe */
> >>> >   static void noreturn cf_check __machine_halt(void *unused)
> >>> >   {
> >>> >   local_irq_disable();
> >>> > diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
> >>> > index e8a4eea71a..d47c54f034 100644
> >>> > --- a/xen/include/xen/bug.h
> >>> > +++ b/xen/include/xen/bug.h
> >>> > @@ -117,6 +117,7 @@ struct bug_frame {
> >>> >   #endif
> >>> > #ifndef BUG
> >>> > +/* SAF-2-safe */
> >>> >   #define BUG() do {  \
> >>> >   BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, NULL);  \
> >>> >   unreachable();  \
> >>> ... unreachable for macros. But the /* SAF-2-safe */ feels a little bit
> >>> redundant when a function is marked as 'noreturn'.
> >>> Is there any way to teach eclair about noreturn?
> >> Actually I had the same thought while writing this patch. If we can
> >> adopt unreachable and noreturn consistently maybe we don't need
> >> SAF-2-safe. If the checker can support it.
> >> Nicola, what do you think?
> > 
> > A couple of remarks:
> > - if you put the noreturn attribute on some functions, then surely the code 
> > after those is
> > reported as unreachable. ECLAIR should pick up all forms of noreturn 
> > automatically; otherwise, a simple configuration can be used.
> > 
> > - Note that the cause of unreachability in the vast majority of cases is 
> > the call to
> > __builtin_unreachable(), therefore a textual deviation on the definition of 
> > unreachable, plus
> > a bit of ECLAIR configuration, can deviate it (to be clear, just the SAF 
> > comment is not
> > sufficient, since deviations comments are meant to be applied at the top 
> > expansion location,
> > which is not on the macro definition).
> > This is what it should look like, roughly:
> > 
> > -config=MC3R1.R2.1,reports+={deliberate, "any_area(any_loc(text(^$, 
> > -1)))"}
> > 
> > #if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__ < 5))
> > /* SAF-2-safe */
> > #define unreachable() do {} while (1)
> > #else
> > /* SAF-2-safe */
> > #define unreachable() __builtin_unreachable()
> > #endif
> > 
> > where REGEX will match the translation of SAF-2-safe.
> > 
> > However, this will then entail that *some* SAF comments are treated 
> > specially and, moreover,
> > that some modification to the definition of unreachable won't work
> > (e.g.
> > #define M() __builtin_unreachable()
> > /* SAF-2-safe */
> > #define unreachable() M()
> > 
> > My opinion is that it's far easier for this to be an eclair configuration 
> > (which has the
> > advantage not to depend on the exact definition of unreachable) and then 
> > perhaps a comment
> > above it explaining the situation.
> 
> I agree here and it is easier to make an overall exception where we list the 
> cases
> where this is acceptable (ie all flavors of unreacheable) and document that 
> eclair
> was configured using "" to handle this.

In that case it looks like we all agree that we can go ahead with this
patch with just the changes to docs/misra/rules.rst to add rule 2.1 and
remove everything else. Which is v2 of this patch:
https://marc.info/?l=xen-devel=169283027729298

Henry, can I get one more release-ack for v2 of this patch (only changes
to docs/misra, no code changes)?

Also Bertrand can you provide a formal Ack for v2?


I do think we should have a document to track this kind of deviations
that are not managed by safe.json or exclude-list.json. But I think for
now the rules.rst notes and the ECLAIR config file (which is under
xen.git) will suffice.

Re: [PATCH] docs/misra: add rule 11.9

[Henry Wang]

Hi Stefano,

> On Sep 28, 2023, at 08:22, Stefano Stabellini  wrote:
> 
> Hi Henry, since we are doing doc changes, would you also agree on this
> one?
> 

Yes, same as the last patch that I just release acked.

Kind regards,
Henry


> 
> On Wed, 27 Sep 2023, Bertrand Marquis wrote:
>> Hi Stefano,
>> 
>>> On 27 Sep 2023, at 03:05, Stefano Stabellini  wrote:
>>> 
>>> From: Stefano Stabellini 
>>> 
>>> Following up from the MISRA C group discussion, add Rule 11.9 to
>>> docs/misra/rules.rst.
>>> 
>>> Signed-off-by: Stefano Stabellini 
>> 
>> I agree with Jan on dropping the "integer" word here.
>> 
>> With that done:
>> Acked-by: Bertrand Marquis 
>> 
>> Cheers
>> Bertrand
>> 
>>> ---
>>> Rule 13.1 also discussed but it is already in docs/misra/rules.rst
>>> ---
>>> docs/misra/rules.rst | 5 +
>>> 1 file changed, 5 insertions(+)
>>> 
>>> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
>>> index 8e7d17d242..28dc3a4d66 100644
>>> --- a/docs/misra/rules.rst
>>> +++ b/docs/misra/rules.rst
>>> @@ -373,6 +373,11 @@ maintainers if you want to suggest a change.
>>> - A cast shall not remove any const or volatile qualification from the 
>>> type pointed to by a pointer
>>> -
>>> 
>>> +   * - `Rule 11.9 
>>> `_
>>> + - Required
>>> + - The macro NULL shall be the only permitted form of integer null 
>>> pointer constant
>>> + -
>>> +
>>>   * - `Rule 12.5 
>>> `_
>>> - Mandatory
>>> - The sizeof operator shall not have an operand which is a function
>>> -- 
>>> 2.25.1
>>> 
>> 
>> 
> 

Re: [PATCH] docs/misra: add rule 11.9

[Stefano Stabellini]

Hi Henry, since we are doing doc changes, would you also agree on this
one?


On Wed, 27 Sep 2023, Bertrand Marquis wrote:
> Hi Stefano,
> 
> > On 27 Sep 2023, at 03:05, Stefano Stabellini  wrote:
> > 
> > From: Stefano Stabellini 
> > 
> > Following up from the MISRA C group discussion, add Rule 11.9 to
> > docs/misra/rules.rst.
> > 
> > Signed-off-by: Stefano Stabellini 
> 
> I agree with Jan on dropping the "integer" word here.
> 
> With that done:
> Acked-by: Bertrand Marquis 
> 
> Cheers
> Bertrand
> 
> > ---
> > Rule 13.1 also discussed but it is already in docs/misra/rules.rst
> > ---
> > docs/misra/rules.rst | 5 +
> > 1 file changed, 5 insertions(+)
> > 
> > diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> > index 8e7d17d242..28dc3a4d66 100644
> > --- a/docs/misra/rules.rst
> > +++ b/docs/misra/rules.rst
> > @@ -373,6 +373,11 @@ maintainers if you want to suggest a change.
> >  - A cast shall not remove any const or volatile qualification from the 
> > type pointed to by a pointer
> >  -
> > 
> > +   * - `Rule 11.9 
> > `_
> > + - Required
> > + - The macro NULL shall be the only permitted form of integer null 
> > pointer constant
> > + -
> > +
> >* - `Rule 12.5 
> > `_
> >  - Mandatory
> >  - The sizeof operator shall not have an operand which is a function
> > -- 
> > 2.25.1
> > 
> 
> 

Re: Xen 4.18 release: Reminder about code freeze

[Stefano Stabellini]

On Thu, 28 Sep 2023, Andrew Cooper wrote:
> On 28/09/2023 12:04 am, Stefano Stabellini wrote:
> > On Mon, 25 Sep 2023, Henry Wang wrote:
> >> 3. dom0less vs xenstored setup race Was: xen | Failed pipeline for staging 
> >> | 6a47ba2f
> >>
> >> https://marc.info/?l=xen-devel=168312468808977
> >>
> >> https://marc.info/?l=xen-devel=168312687610283
> > For this issue I suggest to go with this fix unless someone can produce
> > a better fix before RC2. I don't think we should cut RC2 with this issue
> > unsolved.
> >
> > ---
> > [PATCH] xenstored: reset domain connection before XENSTORE_CONNECTED
> >
> > From: Julien Grall 
> >
> > xenstored will set interface->connection to XENSTORE_CONNECTED before
> > finalizing the connection which can cause initialization errors on the
> > guest side. Make sure to call domain_conn_reset(domain) before setting
> > XENSTORE_CONNECTED.
> >
> > Signed-off-by: Julien Grall 
> > [stefano: rebase, commit message]
> > Signed-off-by: Stefano Stabellini 
> > Acked-by: Stefano Stabellini 
> 
> No - this hasn't got any better at fixing the problem than the last time
> it failed to fix the problem.

It does solve one of the issues: the sporadic failure of the gitlab-ci
job. Even if the fix is not complete, if nothing else, it does that.

Of course, if someone comes up with a better fix all the better!

There hasn't been a lot of movement on this issue so I am being
pessimistic and I am offering the (maybe partial) solution we have
today. I don't mean to take anything away from the value of doing a
better fix.


> You cannot have 3 entities in parallel fight for control in a 2-way
> communication channel.
> 
> Failure to understand this is what created the problem to begin with.
> 
> You took an existing ABI from oxenstored, and implemented it
> incompatibly in other entities, had init-dom0less corrupt a shared comms
> buffer that it isn't the producer or consumer of, and added bug in Linux
> because you didn't write down the behaviour you wanted, let alone the
> behaviour you actually provided.

I think I could write a document covering the intended behavior at the
time of contributing the original feature. That might be good to have
regardless of the bug.


> Stop tinkering in the hope that it hides the problem.  You're only
> making it harder to fix properly.
 
Making it harder to fix properly would be a valid reason not to commit
the (maybe partial) fix. But looking at the fix again:

diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
index a6cd199fdc..9cd6678015 100644
--- a/tools/xenstored/domain.c
+++ b/tools/xenstored/domain.c
@@ -989,6 +989,7 @@ static struct domain *introduce_domain(const void *ctx,
talloc_steal(domain->conn, domain);
 
if (!restore) {
+   domain_conn_reset(domain);
/* Notify the domain that xenstore is available */
interface->connection = XENSTORE_CONNECTED;
xenevtchn_notify(xce_handle, domain->port);
@@ -1031,8 +1032,6 @@ int do_introduce(const void *ctx, struct connection *conn,
if (!domain)
return errno;
 
-   domain_conn_reset(domain);
-
send_ack(conn, XS_INTRODUCE);

It is a 1-line movement. Textually small. Easy to understand and to
revert. It doesn't seem to be making things harder to fix? We could
revert it any time if a better fix is offered.

Maybe we could have a XXX note in the commit message or in-code
comment?


> Tell me, when was the last time this failed...

I went through all the gitlab reports and this is the last one I found
which is from 1 month ago:
https://gitlab.com/xen-project/xen/-/pipelines/953569140

Even if the heinsenbug manifests only once a month I think it is bad.
Of course it is up to Henry but I don't think we should go far into the
release process without this problem being (at least partially) fixed.

Re: [PATCH v3] docs/misra: add 14.3

[Henry Wang]

Hi Stefano,

> On Sep 28, 2023, at 07:53, Stefano Stabellini  wrote:
> 
> Actually adding Henry
> 
> On Wed, 27 Sep 2023, Stefano Stabellini wrote:
>> Hi Henry,
>> 
>> This patch is now acked. Should it go in 4.18?

Sure, a doc change should be harmless so:

Release-acked-by: Henry Wang 

Kind regards,
Henry


>> 
>> In terms of risk of breaking, it is zero as nothing builds or runs based
>> on this document.
>> 
>> At the same time, the benefit is also low because the main value of this
>> document is for future coding changes that would be too late now for
>> 4.18. So the benefits of committing it now are ease of keeping track of
>> the change and positive PR when we make the 4.18 release and we talk
>> about the total number of MISRA C rules we adopted.
>> 
>> 
>> 
>> On Fri, 8 Sep 2023, Stefano Stabellini wrote:
>>> From: Stefano Stabellini 
>>> 
>>> Add 14.3, with project-wide deviations.
>>> 
>>> Also take the opportunity to clarify that parameters of function pointer
>>> types are expected to have names (Rule 8.2).
>>> 
>>> Signed-off-by: Stefano Stabellini 
>>> ---
>>> Changes in v3:
>>> - add ,
>>> - add switch(sizeof(...)) and switch(offsetof(...))
>>> ---
>>> docs/misra/rules.rst | 15 ++-
>>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
>>> index 34916e266a..ac76e20a9c 100644
>>> --- a/docs/misra/rules.rst
>>> +++ b/docs/misra/rules.rst
>>> @@ -234,7 +234,8 @@ maintainers if you want to suggest a change.
>>>* - `Rule 8.2 
>>> `_
>>>  - Required
>>>  - Function types shall be in prototype form with named parameters
>>> - -
>>> + - Clarification: both function and function pointers types shall
>>> +   have named parameters.
>>> 
>>>* - `Rule 8.3 
>>> `_
>>>  - Required
>>> @@ -385,6 +386,18 @@ maintainers if you want to suggest a change.
>>>  - A loop counter shall not have essentially floating type
>>>  -
>>> 
>>> +   * - `Rule 14.3 
>>> `_
>>> + - Required
>>> + - Controlling expressions shall not be invariant
>>> + - Due to the extensive usage of IS_ENABLED, sizeof compile-time
>>> +   checks, and other constructs that are detected as errors by MISRA
>>> +   C scanners, managing the configuration of a MISRA C scanner for
>>> +   this rule would be unmanageable. Thus, this rule is adopted with
>>> +   a project-wide deviation on if, ?:, switch(sizeof(...)), and
>>> +   switch(offsetof(...)) statements.
>>> +
>>> +   while(0) and while(1) and alike are allowed.
>>> +
>>>* - `Rule 16.7 
>>> `_
>>>  - Required
>>>  - A switch-expression shall not have essentially Boolean type
>>> -- 
>>> 2.25.1
>>> 
>> 

Re: [PATCH v3] docs/misra: add 14.3

[Stefano Stabellini]

Actually adding Henry

On Wed, 27 Sep 2023, Stefano Stabellini wrote:
> Hi Henry,
> 
> This patch is now acked. Should it go in 4.18?
> 
> In terms of risk of breaking, it is zero as nothing builds or runs based
> on this document.
> 
> At the same time, the benefit is also low because the main value of this
> document is for future coding changes that would be too late now for
> 4.18. So the benefits of committing it now are ease of keeping track of
> the change and positive PR when we make the 4.18 release and we talk
> about the total number of MISRA C rules we adopted.
> 
> 
> 
> On Fri, 8 Sep 2023, Stefano Stabellini wrote:
> > From: Stefano Stabellini 
> > 
> > Add 14.3, with project-wide deviations.
> > 
> > Also take the opportunity to clarify that parameters of function pointer
> > types are expected to have names (Rule 8.2).
> > 
> > Signed-off-by: Stefano Stabellini 
> > ---
> > Changes in v3:
> > - add ,
> > - add switch(sizeof(...)) and switch(offsetof(...))
> > ---
> >  docs/misra/rules.rst | 15 ++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> > index 34916e266a..ac76e20a9c 100644
> > --- a/docs/misra/rules.rst
> > +++ b/docs/misra/rules.rst
> > @@ -234,7 +234,8 @@ maintainers if you want to suggest a change.
> > * - `Rule 8.2 
> > `_
> >   - Required
> >   - Function types shall be in prototype form with named parameters
> > - -
> > + - Clarification: both function and function pointers types shall
> > +   have named parameters.
> >  
> > * - `Rule 8.3 
> > `_
> >   - Required
> > @@ -385,6 +386,18 @@ maintainers if you want to suggest a change.
> >   - A loop counter shall not have essentially floating type
> >   -
> >  
> > +   * - `Rule 14.3 
> > `_
> > + - Required
> > + - Controlling expressions shall not be invariant
> > + - Due to the extensive usage of IS_ENABLED, sizeof compile-time
> > +   checks, and other constructs that are detected as errors by MISRA
> > +   C scanners, managing the configuration of a MISRA C scanner for
> > +   this rule would be unmanageable. Thus, this rule is adopted with
> > +   a project-wide deviation on if, ?:, switch(sizeof(...)), and
> > +   switch(offsetof(...)) statements.
> > +
> > +   while(0) and while(1) and alike are allowed.
> > +
> > * - `Rule 16.7 
> > `_
> >   - Required
> >   - A switch-expression shall not have essentially Boolean type
> > -- 
> > 2.25.1
> > 
> 

Re: [PATCH v3] docs/misra: add 14.3

[Stefano Stabellini]

Hi Henry,

This patch is now acked. Should it go in 4.18?

In terms of risk of breaking, it is zero as nothing builds or runs based
on this document.

At the same time, the benefit is also low because the main value of this
document is for future coding changes that would be too late now for
4.18. So the benefits of committing it now are ease of keeping track of
the change and positive PR when we make the 4.18 release and we talk
about the total number of MISRA C rules we adopted.



On Fri, 8 Sep 2023, Stefano Stabellini wrote:
> From: Stefano Stabellini 
> 
> Add 14.3, with project-wide deviations.
> 
> Also take the opportunity to clarify that parameters of function pointer
> types are expected to have names (Rule 8.2).
> 
> Signed-off-by: Stefano Stabellini 
> ---
> Changes in v3:
> - add ,
> - add switch(sizeof(...)) and switch(offsetof(...))
> ---
>  docs/misra/rules.rst | 15 ++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> index 34916e266a..ac76e20a9c 100644
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -234,7 +234,8 @@ maintainers if you want to suggest a change.
> * - `Rule 8.2 
> `_
>   - Required
>   - Function types shall be in prototype form with named parameters
> - -
> + - Clarification: both function and function pointers types shall
> +   have named parameters.
>  
> * - `Rule 8.3 
> `_
>   - Required
> @@ -385,6 +386,18 @@ maintainers if you want to suggest a change.
>   - A loop counter shall not have essentially floating type
>   -
>  
> +   * - `Rule 14.3 
> `_
> + - Required
> + - Controlling expressions shall not be invariant
> + - Due to the extensive usage of IS_ENABLED, sizeof compile-time
> +   checks, and other constructs that are detected as errors by MISRA
> +   C scanners, managing the configuration of a MISRA C scanner for
> +   this rule would be unmanageable. Thus, this rule is adopted with
> +   a project-wide deviation on if, ?:, switch(sizeof(...)), and
> +   switch(offsetof(...)) statements.
> +
> +   while(0) and while(1) and alike are allowed.
> +
> * - `Rule 16.7 
> `_
>   - Required
>   - A switch-expression shall not have essentially Boolean type
> -- 
> 2.25.1
> 

Re: Xen 4.18 release: Reminder about code freeze

[Andrew Cooper]

On 28/09/2023 12:04 am, Stefano Stabellini wrote:
> On Mon, 25 Sep 2023, Henry Wang wrote:
>> 3. dom0less vs xenstored setup race Was: xen | Failed pipeline for staging | 
>> 6a47ba2f
>>
>> https://marc.info/?l=xen-devel=168312468808977
>>
>> https://marc.info/?l=xen-devel=168312687610283
> For this issue I suggest to go with this fix unless someone can produce
> a better fix before RC2. I don't think we should cut RC2 with this issue
> unsolved.
>
> ---
> [PATCH] xenstored: reset domain connection before XENSTORE_CONNECTED
>
> From: Julien Grall 
>
> xenstored will set interface->connection to XENSTORE_CONNECTED before
> finalizing the connection which can cause initialization errors on the
> guest side. Make sure to call domain_conn_reset(domain) before setting
> XENSTORE_CONNECTED.
>
> Signed-off-by: Julien Grall 
> [stefano: rebase, commit message]
> Signed-off-by: Stefano Stabellini 
> Acked-by: Stefano Stabellini 

No - this hasn't got any better at fixing the problem than the last time
it failed to fix the problem.

You cannot have 3 entities in parallel fight for control in a 2-way
communication channel.

Failure to understand this is what created the problem to begin with.

You took an existing ABI from oxenstored, and implemented it
incompatibly in other entities, had init-dom0less corrupt a shared comms
buffer that it isn't the producer or consumer of, and added bug in Linux
because you didn't write down the behaviour you wanted, let alone the
behaviour you actually provided.

Stop tinkering in the hope that it hides the problem.  You're only
making it harder to fix properly.

Tell me, when was the last time this failed...

~Andrew

Re: [PATCH v3] SUPPORT: downgrade Physical CPU Hotplug to Experimental

[Henry Wang]

Hi Stefano,

> On Sep 28, 2023, at 07:20, Stefano Stabellini  wrote:
> 
> From: Stefano Stabellini 
> 
> The feature is not commonly used, and we don't have hardware to test it,
> not in OSSTest, not in Gitlab, and not even ad-hoc manually by community
> members. We could use QEMU to test it, but even that it is known not to
> work.
> 
> Also take the opportunity to rename the feature to "ACPI CPU Hotplug"
> for clarity.
> 
> Signed-off-by: Stefano Stabellini 

Release-acked-by: Henry Wang 

Kind regards,
Henry

[PATCH v3] SUPPORT: downgrade Physical CPU Hotplug to Experimental

[Stefano Stabellini]

From: Stefano Stabellini 

The feature is not commonly used, and we don't have hardware to test it,
not in OSSTest, not in Gitlab, and not even ad-hoc manually by community
members. We could use QEMU to test it, but even that it is known not to
work.

Also take the opportunity to rename the feature to "ACPI CPU Hotplug"
for clarity.

Signed-off-by: Stefano Stabellini 
---
Changes in v3:
- add note to CHANGELOG.md
---
 CHANGELOG.md | 2 ++
 SUPPORT.md   | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 24636b8eaf..e33cf4e1b1 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,6 +13,8 @@ The format is based on [Keep a 
Changelog](https://keepachangelog.com/en/1.0.0/)
known user doesn't use it properly, leading to in-guest breakage.
  - The "dom0" option is now supported on Arm and "sve=" sub-option can be used
to enable dom0 guest to use SVE/SVE2 instructions.
+ - Physical CPU Hotplug downgraded to Experimental and renamed "ACPI CPU
+   Hotplug" for clarity
 
 ### Added
  - On x86, support for features new in Intel Sapphire Rapids CPUs:
diff --git a/SUPPORT.md b/SUPPORT.md
index 3461f5cf2f..3472b6edfa 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -46,9 +46,9 @@ For the Cortex A77 r0p0 - r1p0, see Errata 1508412.
 
 ## Host hardware support
 
-### Physical CPU Hotplug
+### ACPI CPU Hotplug
 
-Status, x86: Supported
+Status, x86: Experimental
 
 ### Physical Memory
 
-- 
2.25.1

Re: Xen 4.18 release: Reminder about code freeze

[Stefano Stabellini]

On Mon, 25 Sep 2023, Henry Wang wrote:
> 3. dom0less vs xenstored setup race Was: xen | Failed pipeline for staging | 
> 6a47ba2f
> 
> https://marc.info/?l=xen-devel=168312468808977
> 
> https://marc.info/?l=xen-devel=168312687610283

For this issue I suggest to go with this fix unless someone can produce
a better fix before RC2. I don't think we should cut RC2 with this issue
unsolved.

---
[PATCH] xenstored: reset domain connection before XENSTORE_CONNECTED

From: Julien Grall 

xenstored will set interface->connection to XENSTORE_CONNECTED before
finalizing the connection which can cause initialization errors on the
guest side. Make sure to call domain_conn_reset(domain) before setting
XENSTORE_CONNECTED.

Signed-off-by: Julien Grall 
[stefano: rebase, commit message]
Signed-off-by: Stefano Stabellini 
Acked-by: Stefano Stabellini 

diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
index a6cd199fdc..9cd6678015 100644
--- a/tools/xenstored/domain.c
+++ b/tools/xenstored/domain.c
@@ -989,6 +989,7 @@ static struct domain *introduce_domain(const void *ctx,
talloc_steal(domain->conn, domain);
 
if (!restore) {
+   domain_conn_reset(domain);
/* Notify the domain that xenstore is available */
interface->connection = XENSTORE_CONNECTED;
xenevtchn_notify(xce_handle, domain->port);
@@ -1031,8 +1032,6 @@ int do_introduce(const void *ctx, struct connection *conn,
if (!domain)
return errno;
 
-   domain_conn_reset(domain);
-
send_ack(conn, XS_INTRODUCE);
 
return 0;

[linux-linus test] 183189: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 183181
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 183181
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 183181
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 183181
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 183181
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 183181
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 183181
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 183181
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qcow2 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-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  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-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-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-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-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-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-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-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-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  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:
 linux0e945134b680040b8613e962f586d91b6d40292d
baseline version:
 linux50768a425b46ad7d98f6d88c22d41aa026c463cf

Last test of basis   183181  2023-09-26 16:40:34 Z1 days
Testing same since   183189  2023-09-27 04:54:33 Z0 days1 attempts


People who touched revisions under test:
  David Sterba 
  Filipe Manana 
  Jens Axboe 
  Josef Bacik 
  Linus Torvalds 
  Qu Wenruo 
  Steven Rostedt (Google) 
  Tejun Heo 
  Zqiang 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-arm64  pass
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt  

Re: [PATCH v9 08/16] vpci/header: handle p2m range sets per BAR

[Stewart Hildebrand]

On 8/29/23 19:19, Volodymyr Babchuk wrote:
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index e96d7b2b37..3cc6a96849 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -161,63 +161,101 @@ static void modify_decoding(const struct pci_dev 
> *pdev, uint16_t cmd,
> 
>  bool vpci_process_pending(struct vcpu *v)
>  {
> -if ( v->vpci.mem )
> +struct pci_dev *pdev = v->vpci.pdev;
> +struct map_data data = {
> +.d = v->domain,
> +.map = v->vpci.cmd & PCI_COMMAND_MEMORY,
> +};
> +struct vpci_header *header = NULL;
> +unsigned int i;
> +
> +if ( !pdev )
> +return false;
> +
> +read_lock(>domain->pci_lock);
> +header = >vpci->header;
> +for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
>  {
> -struct map_data data = {
> -.d = v->domain,
> -.map = v->vpci.cmd & PCI_COMMAND_MEMORY,
> -};
> -int rc = rangeset_consume_ranges(v->vpci.mem, map_range, );
> +struct vpci_bar *bar = >bars[i];
> +int rc;
> +
> +if ( rangeset_is_empty(bar->mem) )
> +continue;
> +
> +rc = rangeset_consume_ranges(bar->mem, map_range, );
> 
>  if ( rc == -ERESTART )
> +{
> +read_unlock(>domain->pci_lock);
>  return true;
> +}
> 
> -write_lock(>domain->pci_lock);
> -spin_lock(>vpci.pdev->vpci->lock);
> -/* Disable memory decoding unconditionally on failure. */
> -modify_decoding(v->vpci.pdev,
> -rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : v->vpci.cmd,
> -!rc && v->vpci.rom_only);
> -spin_unlock(>vpci.pdev->vpci->lock);
> -
> -rangeset_destroy(v->vpci.mem);
> -v->vpci.mem = NULL;
>  if ( rc )
> -/*
> - * FIXME: in case of failure remove the device from the domain.
> - * Note that there might still be leftover mappings. While this 
> is
> - * safe for Dom0, for DomUs the domain will likely need to be
> - * killed in order to avoid leaking stale p2m mappings on
> - * failure.
> - */
> -vpci_deassign_device(v->vpci.pdev);
> -write_unlock(>domain->pci_lock);
> +{
> +spin_lock(>vpci->lock);
> +/* Disable memory decoding unconditionally on failure. */
> +modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
> +false);
> +spin_unlock(>vpci->lock);
> +
> +v->vpci.pdev = NULL;
> +
> +read_unlock(>domain->pci_lock);
> +
> +if ( is_hardware_domain(v->domain) )
> +{
> +write_lock(>domain->pci_lock);
> +vpci_deassign_device(v->vpci.pdev);

s/v->vpci.pdev/pdev/ since v->vpci.pdev was assigned NULL a few lines earlier.

Re: [PATCH v9 15/16] xen/arm: vpci: check guest range

[Stewart Hildebrand]

On 9/26/23 11:48, Roger Pau Monné wrote:
> On Tue, Sep 26, 2023 at 11:27:48AM -0400, Stewart Hildebrand wrote:
>> On 9/26/23 04:07, Roger Pau Monné wrote:
>>> On Mon, Sep 25, 2023 at 05:49:00PM -0400, Stewart Hildebrand wrote:
 On 9/22/23 04:44, Roger Pau Monné wrote:
> On Tue, Aug 29, 2023 at 11:19:47PM +, Volodymyr Babchuk wrote:
>> From: Stewart Hildebrand 
>>
>> Skip mapping the BAR if it is not in a valid range.
>>
>> Signed-off-by: Stewart Hildebrand 
>> ---
>>  xen/drivers/vpci/header.c | 9 +
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index 1d243eeaf9..dbabdcbed2 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -345,6 +345,15 @@ static int modify_bars(const struct pci_dev *pdev, 
>> uint16_t cmd, bool rom_only)
>>   bar->enabled == !!(cmd & PCI_COMMAND_MEMORY) )
>>  continue;
>>
>> +#ifdef CONFIG_ARM
>> +if ( !is_hardware_domain(pdev->domain) )
>> +{
>> +if ( (start_guest < PFN_DOWN(GUEST_VPCI_MEM_ADDR)) ||
>> + (end_guest >= PFN_DOWN(GUEST_VPCI_MEM_ADDR + 
>> GUEST_VPCI_MEM_SIZE)) )
>> +continue;
>> +}
>> +#endif
>
> Hm, I think this should be in a hook similar to pci_check_bar() that
> can be implemented per-arch.
>
> IIRC at least on x86 we allow the guest to place the BARs whenever it
> wants, would such placement cause issues to the hypervisor on Arm?

 Hm. I wrote this patch in a hurry to make v9 of this series work on ARM. 
 In my haste I also forgot about the prefetchable range starting at 
 GUEST_VPCI_PREFETCH_MEM_ADDR, but that won't matter as we can probably 
 throw this patch out.

 Now that I've had some more time to investigate, I believe the check in 
 this patch is more or less redundant to the existing check in map_range() 
 added in baa6ea700386 ("vpci: add permission checks to map_range()").

 The issue is that during initialization bar->guest_addr is zeroed, and 
 this initial value of bar->guest_addr will fail the permissions check in 
 map_range() and crash the domain. When the guest writes a new valid BAR, 
 the old invalid address remains in the rangeset to be mapped. If we simply 
 remove the old invalid BAR from the rangeset, that seems to fix the issue. 
 So something like this:
>>>
>>> It does seem to me we are missing a proper cleanup of the rangeset
>>> contents in some paths then.  In the above paragraph you mention "the
>>> old invalid address remains in the rangeset to be mapped", how does it
>>> get in there in the first place, and why is the rangeset not emptied
>>> if the mapping failed?
>>
>> Back in ("vpci/header: handle p2m range sets per BAR") I added a v->domain 
>> == pdev->domain check near the top of vpci_process_pending() as you 
>> appropriately suggested.
>>
>> +if ( v->domain != pdev->domain )
>> +{
>> +read_unlock(>domain->pci_lock);
>> +return false;
>> +}
>>
>> I have also reverted this patch ("xen/arm: vpci: check guest range").
>>
>> The sequence of events leading to the old value remaining in the rangeset 
>> are:
>>
>> # xl pci-assignable-add 01:00.0
>> drivers/vpci/vpci.c:vpci_deassign_device()
>> deassign :01:00.0 from d0
>> # grep pci domu.cfg
>> pci = [ "01:00.0" ]
>> # xl create domu.cfg
>> drivers/vpci/vpci.c:vpci_deassign_device()
>> deassign :01:00.0 from d[IO]
>> drivers/vpci/vpci.c:vpci_assign_device()
>> assign :01:00.0 to d1
>> bar->guest_addr is initialized to zero because of the line: pdev->vpci = 
>> xzalloc(struct vpci);
>> drivers/vpci/header.c:init_bars()
>> drivers/vpci/header.c:modify_bars()
> 
> I think I've commented this on another patch, but why is the device
> added with memory decoding enabled?  I would expect the FLR performed
> before assigning would leave the device with memory decoding disabled?

It seems the device is indeed being assigned to the domU with memory decoding 
enabled, but I'm not entirely sure why. The device I'm testing with doesn't 
support FLR, but it does support pm bus reset:
# cat /sys/bus/pci/devices/\:01\:00.0/reset_method
pm bus

As I understand it, libxl__device_pci_reset() should still be able to issue a 
reset in this case.

> Otherwise we might have to force init_bars() to assume memory decoding
> to be disabled, IOW: memory decoding would be set as disabled in the
> guest cmd view, and leave the physical device cmd as-is.  We might
> also consider switching memory decoding off unconditionally for domUs
> on the physical device.

I did a quick test and it works as expected with my apparently quirky test case:

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index de29e5322d34..0ad0ad947759 100644
--- 

[libvirt test] 183188: tolerable FAIL - PUSHED

[osstest service owner]

flight 183188 libvirt real [real]
flight 183195 libvirt real-retest [real]
http://logs.test-lab.xenproject.org/osstest/logs/183188/
http://logs.test-lab.xenproject.org/osstest/logs/183195/

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-i386-libvirt-xsm   7 xen-install fail pass in 183195-retest

Tests which did not succeed, but are not blocking:
 test-amd64-i386-libvirt-xsm 15 migrate-support-check fail in 183195 never pass
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 183172
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 183172
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 183172
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  15 migrate-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-arm64-arm64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt 16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-arm64-arm64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-arm64-arm64-libvirt-qcow2 15 saverestore-support-checkfail never pass
 test-amd64-i386-libvirt-raw  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-amd64-amd64-libvirt-vhd 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

version targeted for testing:
 libvirt  d77cc21d4b5f60f950aa7f0106d4190dde2b0f1b
baseline version:
 libvirt  51a074e74c6ef2fb95e6f53d41315e3f1e00be77

Last test of basis   183172  2023-09-26 04:20:27 Z1 days
Testing same since   183188  2023-09-27 04:20:33 Z0 days1 attempts


People who touched revisions under test:
  Andrea Bolognani 
  Fedora Weblate Translation 
  Jiri Denemark 
  Weblate 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-arm64  pass
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-arm64-libvirt  pass
 build-armhf-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-arm64-pvopspass
 build-armhf-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm   pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass
 test-amd64-amd64-libvirt-xsm pass
 test-arm64-arm64-libvirt-xsm pass
 test-amd64-i386-libvirt-xsm  fail
 test-amd64-amd64-libvirt pass
 test-arm64-arm64-libvirt pass
 test-armhf-armhf-libvirt pass
 test-amd64-i386-libvirt  pass
 test-amd64-amd64-libvirt-pairpass
 test-amd64-i386-libvirt-pair pass
 test-arm64-arm64-libvirt-qcow2   pass
 test-armhf-armhf-libvirt-qcow2   pass
 test-arm64-arm64-libvirt-raw pass
 test-armhf-armhf-libvirt-raw pass
 test-amd64-i386-libvirt-raw  pass
 test-amd64-amd64-libvirt-vhd pass

Re: [PATCH v4 0/29] block: Make blkdev_get_by_*() return handle

[Christian Brauner]

On Wed, 27 Sep 2023 11:34:07 +0200, Jan Kara wrote:
> Create struct bdev_handle that contains all parameters that need to be
> passed to blkdev_put() and provide bdev_open_* functions that return
> this structure instead of plain bdev pointer. This will eventually allow
> us to pass one more argument to blkdev_put() (renamed to bdev_release())
> without too much hassle.
> 
> 
> [...]

> to ease review / testing. Christian, can you pull the patches to your tree
> to get some exposure in linux-next as well? Thanks!

Yep. So I did it slighly differently. I pulled in the btrfs prereqs and
then applied your series on top of it so we get all the Link: tags right.
I'm running tests right now. Please double-check.

---

Applied to the vfs.super branch of the vfs/vfs.git tree.
Patches in the vfs.super branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.super

[01/29] block: Provide bdev_open_* functions
   https://git.kernel.org/vfs/vfs/c/b7c828aa0b3c
[02/29] block: Use bdev_open_by_dev() in blkdev_open()
https://git.kernel.org/vfs/vfs/c/d4e36f27b45a
[03/29] block: Use bdev_open_by_dev() in disk_scan_partitions() and 
blkdev_bszset()
https://git.kernel.org/vfs/vfs/c/5f9bd6764c7a
[04/29] drdb: Convert to use bdev_open_by_path()
https://git.kernel.org/vfs/vfs/c/0220ca8e443d
[05/29] pktcdvd: Convert to bdev_open_by_dev()
https://git.kernel.org/vfs/vfs/c/7af10b889789
[06/29] rnbd-srv: Convert to use bdev_open_by_path()
https://git.kernel.org/vfs/vfs/c/3d27892a4be7
[07/29] xen/blkback: Convert to bdev_open_by_dev()
https://git.kernel.org/vfs/vfs/c/26afb0ed10b3
[08/29] zram: Convert to use bdev_open_by_dev()
https://git.kernel.org/vfs/vfs/c/efc8e3f4c6dc
[09/29] bcache: Convert to bdev_open_by_path()
https://git.kernel.org/vfs/vfs/c/dc893f51d24a
[10/29] dm: Convert to bdev_open_by_dev()
https://git.kernel.org/vfs/vfs/c/80c2267c6d07
[11/29] md: Convert to bdev_open_by_dev()
https://git.kernel.org/vfs/vfs/c/15db36126ca6
[12/29] mtd: block2mtd: Convert to bdev_open_by_dev/path()
https://git.kernel.org/vfs/vfs/c/4c27234bf3ce
[13/29] nvmet: Convert to bdev_open_by_path()
https://git.kernel.org/vfs/vfs/c/70cffddcc300
[14/29] s390/dasd: Convert to bdev_open_by_path()
https://git.kernel.org/vfs/vfs/c/5581d03457f8
[15/29] scsi: target: Convert to bdev_open_by_path()
https://git.kernel.org/vfs/vfs/c/43de7d844d47
[16/29] PM: hibernate: Convert to bdev_open_by_dev()
https://git.kernel.org/vfs/vfs/c/105ea4a2fd18
[17/29] PM: hibernate: Drop unused snapshot_test argument
https://git.kernel.org/vfs/vfs/c/b589a66e3688
[18/29] mm/swap: Convert to use bdev_open_by_dev()
https://git.kernel.org/vfs/vfs/c/615af8e29233
[19/29] fs: Convert to bdev_open_by_dev()
https://git.kernel.org/vfs/vfs/c/5173192bcfe6
[20/29] btrfs: Convert to bdev_open_by_path()
https://git.kernel.org/vfs/vfs/c/8cf64782764f
[21/29] erofs: Convert to use bdev_open_by_path()
https://git.kernel.org/vfs/vfs/c/4d41880bf249
[22/29] ext4: Convert to bdev_open_by_dev()
https://git.kernel.org/vfs/vfs/c/f7507612395e
[23/29] f2fs: Convert to bdev_open_by_dev/path()
https://git.kernel.org/vfs/vfs/c/d9ff8e3b6498
[24/29] jfs: Convert to bdev_open_by_dev()
https://git.kernel.org/vfs/vfs/c/459dc6376338
[25/29] nfs/blocklayout: Convert to use bdev_open_by_dev/path()
https://git.kernel.org/vfs/vfs/c/5b1df9a40929
[26/29] ocfs2: Convert to use bdev_open_by_dev()
https://git.kernel.org/vfs/vfs/c/b6b95acbd943
[27/29] reiserfs: Convert to bdev_open_by_dev/path()
https://git.kernel.org/vfs/vfs/c/7e3615ff6119
[28/29] xfs: Convert to bdev_open_by_path()
https://git.kernel.org/vfs/vfs/c/176ccb99e207
[29/29] block: Remove blkdev_get_by_*() functions
https://git.kernel.org/vfs/vfs/c/953863a5a2ff

Re: [PATCH v3 7/8] x86: introduce GADDR based secondary time area registration alternative

[Jan Beulich]

On 27.09.2023 17:50, Roger Pau Monné wrote:
> On Wed, May 03, 2023 at 05:58:01PM +0200, Jan Beulich wrote:
>> The registration by virtual/linear address has downsides: The access is
>> expensive for HVM/PVH domains. Furthermore for 64-bit PV domains the area
>> is inaccessible (and hence cannot be updated by Xen) when in guest-user
>> mode.
>>
>> Introduce a new vCPU operation allowing to register the secondary time
>> area by guest-physical address.
>>
>> An at least theoretical downside to using physically registered areas is
>> that PV then won't see dirty (and perhaps also accessed) bits set in its
>> respective page table entries.
>>
>> Signed-off-by: Jan Beulich 
> 
> Reviewed-by: Roger Pau Monné 

Thanks.

>> ---
>> v3: Re-base.
>> v2: Forge version in force_update_secondary_system_time().

For your question below, note this revision log entry. I didn't have the
compensation originally, and my made-up XTF test thus failed.

>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -1633,6 +1633,16 @@ void force_update_vcpu_system_time(struc
>>  __update_vcpu_system_time(v, 1);
>>  }
>>  
>> +void force_update_secondary_system_time(struct vcpu *v,
>> +struct vcpu_time_info *map)
>> +{
>> +struct vcpu_time_info u;
>> +
>> +collect_time_info(v, );
>> +u.version = -1; /* Compensate for version_update_end(). */
> 
> Hm, we do not seem to compensate in
> VCPUOP_register_vcpu_time_memory_area, what's more, in that case the
> initial version is picked from the contents of the guest address.
> Hopefully the guest will have zeroed the memory.
> 
> FWIW, I would be fine with leaving this at 0, so the first version
> guest sees is 1.

No, we can't. Odd numbers mean "update in progress". In
__update_vcpu_system_time() we properly use version_update_begin(),
so there's no need for any compensation. If the guest puts a non-zero
value there, that's also fine from the perspective of the protocol.
Just that if the initial value is odd, the guest will mislead itself
into thinking "oh, the hypervisor is updating this right now" until
the first real update has happened. Yet even that is hypothetical,
since upon registration the area is first populated, so upon the
registration hypercall returning the field will already properly have
an even value.

Jan

Re: [XEN PATCH v2 2/3] docs: make the docs for MISRA C:2012 Dir 4.1 visible to ECLAIR

[Anthony PERARD]

On Wed, Sep 27, 2023 at 11:52:31AM +0200, Nicola Vetrini wrote:
> To be able to check for the existence of the necessary subsections in
> the documentation for MISRA C:2012 Dir 4.1, ECLAIR needs to have a source
> file that is built.
> 
> This file is generated from 'C-runtime-failures.rst' in docs/misra
> and the configuration is updated accordingly.
> 
> Signed-off-by: Nicola Vetrini 
> ---
> Changes from RFC:
> - Dropped unused/useless code
> - Revised the sed command
> - Revised the clean target
> 
> Changes in v2:
> - Added explanative comment to the makefile
> - printf instead of echo

Reviewed-by: Anthony PERARD 

Thanks,

-- 
Anthony PERARD

Re: [PATCH v3 7/8] x86: introduce GADDR based secondary time area registration alternative

[Roger Pau Monné]

On Wed, May 03, 2023 at 05:58:01PM +0200, Jan Beulich wrote:
> The registration by virtual/linear address has downsides: The access is
> expensive for HVM/PVH domains. Furthermore for 64-bit PV domains the area
> is inaccessible (and hence cannot be updated by Xen) when in guest-user
> mode.
> 
> Introduce a new vCPU operation allowing to register the secondary time
> area by guest-physical address.
> 
> An at least theoretical downside to using physically registered areas is
> that PV then won't see dirty (and perhaps also accessed) bits set in its
> respective page table entries.
> 
> Signed-off-by: Jan Beulich 

Reviewed-by: Roger Pau Monné 

> ---
> v3: Re-base.
> v2: Forge version in force_update_secondary_system_time().
> 
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1504,6 +1504,15 @@ int arch_vcpu_reset(struct vcpu *v)
>  return 0;
>  }
>  
> +static void cf_check
> +time_area_populate(void *map, struct vcpu *v)
> +{
> +if ( is_pv_vcpu(v) )
> +v->arch.pv.pending_system_time.version = 0;
> +
> +force_update_secondary_system_time(v, map);
> +}
> +
>  long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) 
> arg)
>  {
>  long rc = 0;
> @@ -1541,6 +1550,25 @@ long do_vcpu_op(int cmd, unsigned int vc
>  
>  break;
>  }
> +
> +case VCPUOP_register_vcpu_time_phys_area:
> +{
> +struct vcpu_register_time_memory_area area;
> +
> +rc = -EFAULT;
> +if ( copy_from_guest(, arg, 1) )
> +break;
> +
> +rc = map_guest_area(v, area.addr.p,
> +sizeof(vcpu_time_info_t),
> +>arch.time_guest_area,
> +time_area_populate);
> +if ( rc == -ERESTART )
> +rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iih",
> +   cmd, vcpuid, arg);
> +
> +break;
> +}
>  
>  case VCPUOP_get_physid:
>  {
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -692,6 +692,8 @@ void domain_cpu_policy_changed(struct do
>  
>  bool update_secondary_system_time(struct vcpu *,
>struct vcpu_time_info *);
> +void force_update_secondary_system_time(struct vcpu *,
> +struct vcpu_time_info *);
>  
>  void vcpu_show_registers(const struct vcpu *);
>  
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1633,6 +1633,16 @@ void force_update_vcpu_system_time(struc
>  __update_vcpu_system_time(v, 1);
>  }
>  
> +void force_update_secondary_system_time(struct vcpu *v,
> +struct vcpu_time_info *map)
> +{
> +struct vcpu_time_info u;
> +
> +collect_time_info(v, );
> +u.version = -1; /* Compensate for version_update_end(). */

Hm, we do not seem to compensate in
VCPUOP_register_vcpu_time_memory_area, what's more, in that case the
initial version is picked from the contents of the guest address.
Hopefully the guest will have zeroed the memory.

FWIW, I would be fine with leaving this at 0, so the first version
guest sees is 1.

Thanks, Roger.

Re: [PATCH v3 6/8] domain: introduce GADDR based runstate area registration alternative

[Jan Beulich]

On 27.09.2023 17:24, Roger Pau Monné wrote:
> On Wed, May 03, 2023 at 05:57:40PM +0200, Jan Beulich wrote:
>> The registration by virtual/linear address has downsides: At least on
>> x86 the access is expensive for HVM/PVH domains. Furthermore for 64-bit
>> PV domains the area is inaccessible (and hence cannot be updated by Xen)
>> when in guest-user mode.
>>
>> Introduce a new vCPU operation allowing to register the runstate area by
>> guest-physical address.
>>
>> An at least theoretical downside to using physically registered areas is
>> that PV then won't see dirty (and perhaps also accessed) bits set in its
>> respective page table entries.
>>
>> Signed-off-by: Jan Beulich 
> 
> Reviewed-by: Roger Pau Monné 

Thanks.

> One comment below.
> 
>> --- a/xen/include/public/vcpu.h
>> +++ b/xen/include/public/vcpu.h
>> @@ -221,6 +221,19 @@ struct vcpu_register_time_memory_area {
>>  typedef struct vcpu_register_time_memory_area 
>> vcpu_register_time_memory_area_t;
>>  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>>  
>> +/*
>> + * Like the respective VCPUOP_register_*_memory_area, just using the 
>> "addr.p"
>> + * field of the supplied struct as a guest physical address (i.e. in GFN 
>> space).
>> + * The respective area may not cross a page boundary.  Pass ~0 to 
>> unregister an
>> + * area.  Note that as long as an area is registered by physical address, 
>> the
>> + * linear address based area will not be serviced (updated) by the 
>> hypervisor.
>> + *
>> + * Note that the area registered via VCPUOP_register_runstate_memory_area 
>> will
>> + * be updated in the same manner as the one registered via virtual address 
>> PLUS
>> + * VMASST_TYPE_runstate_update_flag engaged by the domain.
>> + */
>> +#define VCPUOP_register_runstate_phys_area  14
> 
> Just to make it more obvious, it might be nice to add a note in the
> comment on VCPUOP_register_runstate_memory_area that `p` can also be
> used with the `VCPUOP_register_runstate_phys_area` hypercall.

I can add a reference there (and then in the later patch also for the
time area), but I don't think it wants or needs to emphasize p. It
merely needs to point to the alternative sub-function imo.

Jan

Re: [PATCH v3 5/8] domain: map/unmap GADDR based shared guest areas

[Jan Beulich]

On 27.09.2023 16:53, Roger Pau Monné wrote:
> On Wed, May 03, 2023 at 05:57:09PM +0200, Jan Beulich wrote:
>> RFC: In map_guest_area() I'm not checking the P2M type, instead - just
>>  like map_vcpu_info() - solely relying on the type ref acquisition.
>>  Checking for p2m_ram_rw alone would be wrong, as at least
>>  p2m_ram_logdirty ought to also be okay to use here (and in similar
>>  cases, e.g. in Argo's find_ring_mfn()). p2m_is_pageable() could be
>>  used here (like altp2m_vcpu_enable_ve() does) as well as in
>>  map_vcpu_info(), yet then again the P2M type is stale by the time
>>  it is being looked at anyway without the P2M lock held.
> 
> check_get_page_from_gfn() already does some type checks on the page.

But that's still of no help without holding the p2m lock for long enough.
Also the checks there all are to fail the call, whereas the remark above
talks about distinguishing when to permit and when to fail the request
here.

>> ---
>> v2: currd -> d, to cover mem-sharing's copy_guest_area(). Re-base over
>> change(s) earlier in the series. Use ~0 as "unmap" request indicator.
>>
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -1576,7 +1576,82 @@ int map_guest_area(struct vcpu *v, paddr
>> struct guest_area *area,
>> void (*populate)(void *dst, struct vcpu *v))
>>  {
>> -return -EOPNOTSUPP;
>> +struct domain *d = v->domain;
>> +void *map = NULL;
>> +struct page_info *pg = NULL;
>> +int rc = 0;
> 
> Should you check/assert that size != 0?

Not really, no, the more that this is an internal interface, and we
don't (and shouldn't) check similar aspects elsewhere. There's just
a single use of the value (see below).

>> +if ( ~gaddr )
> 
> I guess I will find in future patches, but why this special handling
> for ~0 address?

Future patches aren't very likely to make this explicit. The most
explicit indicator for now was the revision log entry for v2.

> Might be worth to add a comment here, or in the patch description.
> Otherwise I would expect that when passed a ~0 address the first check
> for page boundary crossing to fail.

I've added "/* Map (i.e. not just unmap)? */" on the same line.

>> +{
>> +unsigned long gfn = PFN_DOWN(gaddr);
>> +unsigned int align;
>> +p2m_type_t p2mt;
>> +
>> +if ( gfn != PFN_DOWN(gaddr + size - 1) )
>> +return -ENXIO;

This is the only use of size. If size is 0 and gaddr is on a page boundary,
the call will fail. For any other gaddr nothing bad will happen (afaict).

>> +#ifdef CONFIG_COMPAT
>> +if ( has_32bit_shinfo(d) )
>> +align = alignof(compat_ulong_t);
>> +else
>> +#endif
>> +align = alignof(xen_ulong_t);
>> +if ( gaddr & (align - 1) )
> 
> IS_ALIGNED() might be clearer.

Can switch, sure.

>> +return -ENXIO;
>> +
>> +rc = check_get_page_from_gfn(d, _gfn(gfn), false, , );
>> +if ( rc )
>> +return rc;
>> +
>> +if ( !get_page_type(pg, PGT_writable_page) )
>> +{
>> +put_page(pg);
>> +return -EACCES;
>> +}
>> +
>> +map = __map_domain_page_global(pg);
>> +if ( !map )
>> +{
>> +put_page_and_type(pg);
>> +return -ENOMEM;
>> +}
>> +map += PAGE_OFFSET(gaddr);
>> +}
>> +
>> +if ( v != current )
>> +{
>> +if ( !spin_trylock(>hypercall_deadlock_mutex) )
>> +{
>> +rc = -ERESTART;
>> +goto unmap;
>> +}
>> +
>> +vcpu_pause(v);
>> +
>> +spin_unlock(>hypercall_deadlock_mutex);
>> +}
>> +
>> +domain_lock(d);
>> +
>> +if ( map )
>> +populate(map, v);
> 
> The call to map_guest_area() in copy_guest_area() does pass NULL as
> the populate parameter, hence this will crash?
> 
> Should you either assert that populate != NULL or change the if
> condition to be map && populate?

Oh, yes - the latter.

>> @@ -1587,9 +1662,24 @@ int map_guest_area(struct vcpu *v, paddr
>>  void unmap_guest_area(struct vcpu *v, struct guest_area *area)
>>  {
>>  struct domain *d = v->domain;
>> +void *map;
>> +struct page_info *pg;
>>  
>>  if ( v != current )
>>  ASSERT(atomic_read(>pause_count) | atomic_read(>pause_count));
>> +
>> +domain_lock(d);
>> +map = area->map;
>> +area->map = NULL;
>> +pg = area->pg;
>> +area->pg = NULL;
> 
> FWIW you could also use SWAP() here by initializing both map and pg to
> NULL (I know it uses one extra local variable).

When truly exchanging two values (like further up), I'm fine with using
SWAP() (and as you saw I do use it), but here I think it would be more
heavy than wanted/needed.

Jan

Re: [PATCH v3 6/8] domain: introduce GADDR based runstate area registration alternative

[Roger Pau Monné]

On Wed, May 03, 2023 at 05:57:40PM +0200, Jan Beulich wrote:
> The registration by virtual/linear address has downsides: At least on
> x86 the access is expensive for HVM/PVH domains. Furthermore for 64-bit
> PV domains the area is inaccessible (and hence cannot be updated by Xen)
> when in guest-user mode.
> 
> Introduce a new vCPU operation allowing to register the runstate area by
> guest-physical address.
> 
> An at least theoretical downside to using physically registered areas is
> that PV then won't see dirty (and perhaps also accessed) bits set in its
> respective page table entries.
> 
> Signed-off-by: Jan Beulich 

Reviewed-by: Roger Pau Monné 

One comment below.

> --- a/xen/include/public/vcpu.h
> +++ b/xen/include/public/vcpu.h
> @@ -221,6 +221,19 @@ struct vcpu_register_time_memory_area {
>  typedef struct vcpu_register_time_memory_area 
> vcpu_register_time_memory_area_t;
>  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>  
> +/*
> + * Like the respective VCPUOP_register_*_memory_area, just using the "addr.p"
> + * field of the supplied struct as a guest physical address (i.e. in GFN 
> space).
> + * The respective area may not cross a page boundary.  Pass ~0 to unregister 
> an
> + * area.  Note that as long as an area is registered by physical address, the
> + * linear address based area will not be serviced (updated) by the 
> hypervisor.
> + *
> + * Note that the area registered via VCPUOP_register_runstate_memory_area 
> will
> + * be updated in the same manner as the one registered via virtual address 
> PLUS
> + * VMASST_TYPE_runstate_update_flag engaged by the domain.
> + */
> +#define VCPUOP_register_runstate_phys_area  14

Just to make it more obvious, it might be nice to add a note in the
comment on VCPUOP_register_runstate_memory_area that `p` can also be
used with the `VCPUOP_register_runstate_phys_area` hypercall.

Thanks, Roger.

Re: [PATCH v3 4/8] x86/mem-sharing: copy GADDR based shared guest areas

[Jan Beulich]

On 27.09.2023 16:05, Roger Pau Monné wrote:
> On Wed, Sep 27, 2023 at 02:06:47PM +0200, Jan Beulich wrote:
>> On 27.09.2023 13:08, Roger Pau Monné wrote:
>>> On Wed, May 03, 2023 at 05:56:46PM +0200, Jan Beulich wrote:
 --- a/xen/arch/x86/mm/mem_sharing.c
 +++ b/xen/arch/x86/mm/mem_sharing.c
 @@ -1641,6 +1641,68 @@ static void copy_vcpu_nonreg_state(struc
  hvm_set_nonreg_state(cd_vcpu, );
  }
  
 +static int copy_guest_area(struct guest_area *cd_area,
 +   const struct guest_area *d_area,
 +   struct vcpu *cd_vcpu,
 +   const struct domain *d)
 +{
 +mfn_t d_mfn, cd_mfn;
 +
 +if ( !d_area->pg )
 +return 0;
 +
 +d_mfn = page_to_mfn(d_area->pg);
 +
 +/* Allocate & map a page for the area if it hasn't been already. */
 +if ( !cd_area->pg )
 +{
 +gfn_t gfn = mfn_to_gfn(d, d_mfn);
 +struct p2m_domain *p2m = p2m_get_hostp2m(cd_vcpu->domain);
 +p2m_type_t p2mt;
 +p2m_access_t p2ma;
 +unsigned int offset;
 +int ret;
 +
 +cd_mfn = p2m->get_entry(p2m, gfn, , , 0, NULL, NULL);
 +if ( mfn_eq(cd_mfn, INVALID_MFN) )
 +{
 +struct page_info *pg = alloc_domheap_page(cd_vcpu->domain, 0);
 +
 +if ( !pg )
 +return -ENOMEM;
 +
 +cd_mfn = page_to_mfn(pg);
 +set_gpfn_from_mfn(mfn_x(cd_mfn), gfn_x(gfn));
 +
 +ret = p2m->set_entry(p2m, gfn, cd_mfn, PAGE_ORDER_4K, 
 p2m_ram_rw,
 + p2m->default_access, -1);
 +if ( ret )
 +return ret;
 +}
 +else if ( p2mt != p2m_ram_rw )
 +return -EBUSY;
>>>
>>> Shouldn't the populate of the underlying gfn in the fork case
>>> be done by map_guest_area itself?
>>
>> I've used the existing logic for the info area to base my code on. As
>> noted in the patch switching the info area logic to use the generalize
>> infrastructure, I've taken the liberty to address an issue in the
>> original logic. But it was never a goal to re-write things from scratch.
>> If, as Tamas appears to agree, there a better way of layering things
>> here, then please as a follow-on patch.
> 
> Hm, I'm unsure the code that allocates the page and adds it to the p2m
> for the vcpu_info area is required?  map_vcpu_info() should already be
> able to handle this case and fork the page from the previous VM.

I don't think that's the case. It would be able to unshare, but the fork
doesn't use shared pages aiui. I think it instead runs on an extremely
sparse p2m, where pages from the parent are brought in only as they're
touched. Tamas?

Jan

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

[Federico Serafini]

Make function declarations and definitions consistent.
No functional change.

Signed-off-by: Federico Serafini 
---
 xen/arch/x86/domain_page.c | 36 ++--
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/domain_page.c b/xen/arch/x86/domain_page.c
index eac5e3304f..1cfa992a02 100644
--- a/xen/arch/x86/domain_page.c
+++ b/xen/arch/x86/domain_page.c
@@ -173,18 +173,18 @@ void *map_domain_page(mfn_t mfn)
 return (void *)MAPCACHE_VIRT_START + pfn_to_paddr(idx);
 }
 
-void unmap_domain_page(const void *ptr)
+void unmap_domain_page(const void *va)
 {
 unsigned int idx;
 struct vcpu *v;
 struct mapcache_domain *dcache;
-unsigned long va = (unsigned long)ptr, mfn, flags;
+unsigned long addr = (unsigned long)va, mfn, flags;
 struct vcpu_maphash_entry *hashent;
 
-if ( !va || va >= DIRECTMAP_VIRT_START )
+if ( !addr || addr >= DIRECTMAP_VIRT_START )
 return;
 
-ASSERT(va >= MAPCACHE_VIRT_START && va < MAPCACHE_VIRT_END);
+ASSERT(addr >= MAPCACHE_VIRT_START && addr < MAPCACHE_VIRT_END);
 
 v = mapcache_current_vcpu();
 ASSERT(v && is_pv_vcpu(v));
@@ -192,7 +192,7 @@ void unmap_domain_page(const void *ptr)
 dcache = >domain->arch.pv.mapcache;
 ASSERT(dcache->inuse);
 
-idx = PFN_DOWN(va - MAPCACHE_VIRT_START);
+idx = PFN_DOWN(addr - MAPCACHE_VIRT_START);
 mfn = l1e_get_pfn(MAPCACHE_L1ENT(idx));
 hashent = >arch.pv.mapcache.hash[MAPHASH_HASHFN(mfn)];
 
@@ -317,30 +317,30 @@ void *map_domain_page_global(mfn_t mfn)
 return vmap(, 1);
 }
 
-void unmap_domain_page_global(const void *ptr)
+void unmap_domain_page_global(const void *va)
 {
-unsigned long va = (unsigned long)ptr;
+unsigned long addr = (unsigned long)va;
 
-if ( va >= DIRECTMAP_VIRT_START )
+if ( addr >= DIRECTMAP_VIRT_START )
 return;
 
-ASSERT(va >= VMAP_VIRT_START && va < VMAP_VIRT_END);
+ASSERT(addr >= VMAP_VIRT_START && addr < VMAP_VIRT_END);
 
-vunmap(ptr);
+vunmap(va);
 }
 
 /* Translate a map-domain-page'd address to the underlying MFN */
-mfn_t domain_page_map_to_mfn(const void *ptr)
+mfn_t domain_page_map_to_mfn(const void *va)
 {
-unsigned long va = (unsigned long)ptr;
+unsigned long addr = (unsigned long)va;
 
-if ( va >= DIRECTMAP_VIRT_START )
-return _mfn(virt_to_mfn(ptr));
+if ( addr >= DIRECTMAP_VIRT_START )
+return _mfn(virt_to_mfn(va));
 
-if ( va >= VMAP_VIRT_START && va < VMAP_VIRT_END )
-return vmap_to_mfn(va);
+if ( addr >= VMAP_VIRT_START && addr < VMAP_VIRT_END )
+return vmap_to_mfn(addr);
 
-ASSERT(va >= MAPCACHE_VIRT_START && va < MAPCACHE_VIRT_END);
+ASSERT(addr >= MAPCACHE_VIRT_START && addr < MAPCACHE_VIRT_END);
 
-return l1e_get_mfn(__linear_l1_table[l1_linear_offset(va)]);
+return l1e_get_mfn(__linear_l1_table[l1_linear_offset(addr)]);
 }
-- 
2.34.1

Re: [PATCH v3 5/8] domain: map/unmap GADDR based shared guest areas

[Roger Pau Monné]

On Wed, May 03, 2023 at 05:57:09PM +0200, Jan Beulich wrote:
> The registration by virtual/linear address has downsides: At least on
> x86 the access is expensive for HVM/PVH domains. Furthermore for 64-bit
> PV domains the areas are inaccessible (and hence cannot be updated by
> Xen) when in guest-user mode, and for HVM guests they may be
> inaccessible when Meltdown mitigations are in place. (There are yet
> more issues.)
> 
> In preparation of the introduction of new vCPU operations allowing to
> register the respective areas (one of the two is x86-specific) by
> guest-physical address, flesh out the map/unmap functions.
> 
> Noteworthy differences from map_vcpu_info():
> - areas can be registered more than once (and de-registered),
> - remote vCPU-s are paused rather than checked for being down (which in
>   principle can change right after the check),
> - the domain lock is taken for a much smaller region.
> 
> Signed-off-by: Jan Beulich 
> ---
> RFC: By using global domain page mappings the demand on the underlying
>  VA range may increase significantly. I did consider to use per-
>  domain mappings instead, but they exist for x86 only. Of course we
>  could have arch_{,un}map_guest_area() aliasing global domain page
>  mapping functions on Arm and using per-domain mappings on x86. Yet
>  then again map_vcpu_info() doesn't (and can't) do so.

I guess it's fine as you propose now, we might see about using
per-domain mappings.

> RFC: In map_guest_area() I'm not checking the P2M type, instead - just
>  like map_vcpu_info() - solely relying on the type ref acquisition.
>  Checking for p2m_ram_rw alone would be wrong, as at least
>  p2m_ram_logdirty ought to also be okay to use here (and in similar
>  cases, e.g. in Argo's find_ring_mfn()). p2m_is_pageable() could be
>  used here (like altp2m_vcpu_enable_ve() does) as well as in
>  map_vcpu_info(), yet then again the P2M type is stale by the time
>  it is being looked at anyway without the P2M lock held.

check_get_page_from_gfn() already does some type checks on the page.

> ---
> v2: currd -> d, to cover mem-sharing's copy_guest_area(). Re-base over
> change(s) earlier in the series. Use ~0 as "unmap" request indicator.
> 
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1576,7 +1576,82 @@ int map_guest_area(struct vcpu *v, paddr
> struct guest_area *area,
> void (*populate)(void *dst, struct vcpu *v))
>  {
> -return -EOPNOTSUPP;
> +struct domain *d = v->domain;
> +void *map = NULL;
> +struct page_info *pg = NULL;
> +int rc = 0;

Should you check/assert that size != 0?

> +if ( ~gaddr )

I guess I will find in future patches, but why this special handling
for ~0 address?

Might be worth to add a comment here, or in the patch description.
Otherwise I would expect that when passed a ~0 address the first check
for page boundary crossing to fail.

> +{
> +unsigned long gfn = PFN_DOWN(gaddr);
> +unsigned int align;
> +p2m_type_t p2mt;
> +
> +if ( gfn != PFN_DOWN(gaddr + size - 1) )
> +return -ENXIO;
> +
> +#ifdef CONFIG_COMPAT
> +if ( has_32bit_shinfo(d) )
> +align = alignof(compat_ulong_t);
> +else
> +#endif
> +align = alignof(xen_ulong_t);
> +if ( gaddr & (align - 1) )

IS_ALIGNED() might be clearer.

> +return -ENXIO;
> +
> +rc = check_get_page_from_gfn(d, _gfn(gfn), false, , );
> +if ( rc )
> +return rc;
> +
> +if ( !get_page_type(pg, PGT_writable_page) )
> +{
> +put_page(pg);
> +return -EACCES;
> +}
> +
> +map = __map_domain_page_global(pg);
> +if ( !map )
> +{
> +put_page_and_type(pg);
> +return -ENOMEM;
> +}
> +map += PAGE_OFFSET(gaddr);
> +}
> +
> +if ( v != current )
> +{
> +if ( !spin_trylock(>hypercall_deadlock_mutex) )
> +{
> +rc = -ERESTART;
> +goto unmap;
> +}
> +
> +vcpu_pause(v);
> +
> +spin_unlock(>hypercall_deadlock_mutex);
> +}
> +
> +domain_lock(d);
> +
> +if ( map )
> +populate(map, v);

The call to map_guest_area() in copy_guest_area() does pass NULL as
the populate parameter, hence this will crash?

Should you either assert that populate != NULL or change the if
condition to be map && populate?

> +
> +SWAP(area->pg, pg);
> +SWAP(area->map, map);
> +
> +domain_unlock(d);
> +
> +if ( v != current )
> +vcpu_unpause(v);
> +
> + unmap:
> +if ( pg )
> +{
> +unmap_domain_page_global(map);
> +put_page_and_type(pg);
> +}
> +
> +return rc;
>  }
>  
>  /*
> @@ -1587,9 +1662,24 @@ int map_guest_area(struct vcpu *v, paddr
>  void unmap_guest_area(struct vcpu *v, struct guest_area *area)
>  {
>  struct domain *d = 

Re: [PATCH v11 05/37] x86/trapnr: Add event type macros to

[H. Peter Anvin]

On September 26, 2023 1:10:51 AM PDT, Nikolay Borisov  
wrote:
>
>
>On 23.09.23 г. 12:41 ч., Xin Li wrote:
>> Intel VT-x classifies events into eight different types, which is
>> inherited by FRED for event identification. As such, event type
>> becomes a common x86 concept, and should be defined in a common x86
>> header.
>> 
>> Add event type macros to , and use it in .
>> 
>> Suggested-by: H. Peter Anvin (Intel) 
>> Tested-by: Shan Kang 
>> Signed-off-by: Xin Li 
>> ---
>> 
>> Changes since v10:
>> * A few comment fixes and improvements (Andrew Cooper).
>> ---
>>   arch/x86/include/asm/trapnr.h | 12 
>>   arch/x86/include/asm/vmx.h| 17 +
>>   2 files changed, 21 insertions(+), 8 deletions(-)
>> 
>> diff --git a/arch/x86/include/asm/trapnr.h b/arch/x86/include/asm/trapnr.h
>> index f5d2325aa0b7..8d1154cdf787 100644
>> --- a/arch/x86/include/asm/trapnr.h
>> +++ b/arch/x86/include/asm/trapnr.h
>> @@ -2,6 +2,18 @@
>>   #ifndef _ASM_X86_TRAPNR_H
>>   #define _ASM_X86_TRAPNR_H
>>   +/*
>> + * Event type codes used by FRED, Intel VT-x and AMD SVM
>> + */
>> +#define EVENT_TYPE_EXTINT   0   // External interrupt
>> +#define EVENT_TYPE_RESERVED 1
>> +#define EVENT_TYPE_NMI  2   // NMI
>> +#define EVENT_TYPE_HWEXC3   // Hardware originated traps, exceptions
>> +#define EVENT_TYPE_SWINT4   // INT n
>> +#define EVENT_TYPE_PRIV_SWEXC   5   // INT1
>> +#define EVENT_TYPE_SWEXC6   // INTO, INT3
>
>nit: This turned into INTO (Oh) rather than INT0( zero) in v11
>
>

INTO (letter) is correct.

[xen-unstable test] 183185: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 183175
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 183175
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 183175
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 183175
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 183175
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 183175
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 183175
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 183175
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 183175
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 183175
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 183175
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 183175
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-i386-xl-pvshim14 guest-start  fail   never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   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  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-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-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  15 migrate-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  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  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-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-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-raw  14 migrate-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-armhf-armhf-libvirt-qcow2 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-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
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  cbb71b95dd708b1e26899bbe1e7bf9a85081fd60
baseline version:
 xen  d6351a10c80fcbbf2b5996d351b7181ba17b3b32

Last test of basis   183175  2023-09-26 08:45:53 Z1 days
Testing same since   183185  2023-09-26 23:40:35 Z0 days1 attempts


People who touched revisions under test:
  Jan Beulich 
  Nicola Vetrini 
  Shawn Anastasio 
  

Re: [PATCH v2] SUPPORT: downgrade Physical CPU Hotplug to Experimental

[George Dunlap]

On Wed, Sep 27, 2023 at 1:29 AM Stefano Stabellini
 wrote:
>
> From: Stefano Stabellini 
>
> The feature is not commonly used, and we don't have hardware to test it,
> not in OSSTest, not in Gitlab, and not even ad-hoc manually by community
> members. We could use QEMU to test it, but even that it is known not to
> work.
>
> Also take the opportunity to rename the feature to "ACPI CPU Hotplug"
> for clarity.
>
> Signed-off-by: Stefano Stabellini 

LGTM -- I guess it does want a changelog entry though.

 -George

[xen-unstable-smoke test] 183193: tolerable all pass - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   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-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  a1f8b32af001c15ce3c6be2364826b1ca35b6caa
baseline version:
 xen  cbb71b95dd708b1e26899bbe1e7bf9a85081fd60

Last test of basis   183179  2023-09-26 14:00:25 Z1 days
Testing same since   183193  2023-09-27 11:02:05 Z0 days1 attempts


People who touched revisions under test:
  Stewart Hildebrand 
  Volodymyr Babchuk 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-amd64-libvirt pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   cbb71b95dd..a1f8b32af0  a1f8b32af001c15ce3c6be2364826b1ca35b6caa -> smoke

Re: [PATCH v4 0/29] block: Make blkdev_get_by_*() return handle

[Jens Axboe]

On Wed, Sep 27, 2023 at 3:34?AM Jan Kara  wrote:
>
> Hello,
>
> this is a v3 of the patch series which implements the idea of 
> blkdev_get_by_*()

v4?

> calls returning bdev_handle which is then passed to blkdev_put() [1]. This
> makes the get and put calls for bdevs more obviously matching and allows us to
> propagate context from get to put without having to modify all the users
> (again!). In particular I need to propagate used open flags to blkdev_put() to
> be able count writeable opens and add support for blocking writes to mounted
> block devices. I'll send that series separately.
>
> The series is based on Btrfs tree's for-next branch [2] as of today as the
> series depends on Christoph's changes to btrfs device handling.  Patches have
> passed some reasonable testing - I've tested block changes, md, dm, bcache,
> xfs, btrfs, ext4, swap. More testing or review is always welcome. Thanks! I've
> pushed out the full branch to:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git bdev_handle
>
> to ease review / testing. Christian, can you pull the patches to your tree
> to get some exposure in linux-next as well? Thanks!

For the block bits:

Acked-by: Jens Axboe 

-- 
Jens Axboe

Re: [PATCH v3 4/8] x86/mem-sharing: copy GADDR based shared guest areas

[Roger Pau Monné]

On Wed, Sep 27, 2023 at 02:06:47PM +0200, Jan Beulich wrote:
> On 27.09.2023 13:08, Roger Pau Monné wrote:
> > On Wed, May 03, 2023 at 05:56:46PM +0200, Jan Beulich wrote:
> >> In preparation of the introduction of new vCPU operations allowing to
> >> register the respective areas (one of the two is x86-specific) by
> >> guest-physical address, add the necessary fork handling (with the
> >> backing function yet to be filled in).
> >>
> >> Signed-off-by: Jan Beulich 
> > 
> > Given the very limited and specific usage of the current Xen forking
> > code, do we really need to bother about copying such areas?
> > 
> > IOW: I doubt that not updating the runstate/time areas will make any
> > difference to the forking code ATM.
> 
> I expect Tamas'es reply has sufficiently addressed this question.

Seeing the TODO in copy_vcpu_settings() makes me wonder how well we
copy information for PV interfaces for forks.  Timers are not
migrated, neither runstate areas for example.

Which is all fine, but I expect VMs this is currently tested against
don't use (a lot of) PV interfaces, or else I don't know how they can
survive.

> >> --- a/xen/arch/x86/mm/mem_sharing.c
> >> +++ b/xen/arch/x86/mm/mem_sharing.c
> >> @@ -1641,6 +1641,68 @@ static void copy_vcpu_nonreg_state(struc
> >>  hvm_set_nonreg_state(cd_vcpu, );
> >>  }
> >>  
> >> +static int copy_guest_area(struct guest_area *cd_area,
> >> +   const struct guest_area *d_area,
> >> +   struct vcpu *cd_vcpu,
> >> +   const struct domain *d)
> >> +{
> >> +mfn_t d_mfn, cd_mfn;
> >> +
> >> +if ( !d_area->pg )
> >> +return 0;
> >> +
> >> +d_mfn = page_to_mfn(d_area->pg);
> >> +
> >> +/* Allocate & map a page for the area if it hasn't been already. */
> >> +if ( !cd_area->pg )
> >> +{
> >> +gfn_t gfn = mfn_to_gfn(d, d_mfn);
> >> +struct p2m_domain *p2m = p2m_get_hostp2m(cd_vcpu->domain);
> >> +p2m_type_t p2mt;
> >> +p2m_access_t p2ma;
> >> +unsigned int offset;
> >> +int ret;
> >> +
> >> +cd_mfn = p2m->get_entry(p2m, gfn, , , 0, NULL, NULL);
> >> +if ( mfn_eq(cd_mfn, INVALID_MFN) )
> >> +{
> >> +struct page_info *pg = alloc_domheap_page(cd_vcpu->domain, 0);
> >> +
> >> +if ( !pg )
> >> +return -ENOMEM;
> >> +
> >> +cd_mfn = page_to_mfn(pg);
> >> +set_gpfn_from_mfn(mfn_x(cd_mfn), gfn_x(gfn));
> >> +
> >> +ret = p2m->set_entry(p2m, gfn, cd_mfn, PAGE_ORDER_4K, 
> >> p2m_ram_rw,
> >> + p2m->default_access, -1);
> >> +if ( ret )
> >> +return ret;
> >> +}
> >> +else if ( p2mt != p2m_ram_rw )
> >> +return -EBUSY;
> > 
> > Shouldn't the populate of the underlying gfn in the fork case
> > be done by map_guest_area itself?
> 
> I've used the existing logic for the info area to base my code on. As
> noted in the patch switching the info area logic to use the generalize
> infrastructure, I've taken the liberty to address an issue in the
> original logic. But it was never a goal to re-write things from scratch.
> If, as Tamas appears to agree, there a better way of layering things
> here, then please as a follow-on patch.

Hm, I'm unsure the code that allocates the page and adds it to the p2m
for the vcpu_info area is required?  map_vcpu_info() should already be
able to handle this case and fork the page from the previous VM.

> > What if a forked guest attempts to register a new runstate/time info
> > against an address not yet populated?
> 
> What if the same happened for the info area? Again, I'm not trying to
> invent anything new here. But hopefully Tamas'es reply has helped here
> as well.

Yes, I don't think we should need to allocate and map a page for the
vcpu_info area before calling map_vcpu_info().

> >> --- a/xen/common/domain.c
> >> +++ b/xen/common/domain.c
> >> @@ -1572,6 +1572,13 @@ void unmap_vcpu_info(struct vcpu *v)
> >>  put_page_and_type(mfn_to_page(mfn));
> >>  }
> >>  
> >> +int map_guest_area(struct vcpu *v, paddr_t gaddr, unsigned int size,
> >> +   struct guest_area *area,
> >> +   void (*populate)(void *dst, struct vcpu *v))
> > 
> > Oh, the prototype for this is added in patch 1, almost missed it.
> > 
> > Why not also add this dummy implementation in patch 1 then?
> 
> The prototype isn't dead code, but the function would be until it gains
> users here. If anything, I'd move the prototype introduction here; it
> merely seemed desirable to me to touch xen/include/xen/domain.h no
> more frequently than necessary.
> 
> Also, to be quite frank, I find this kind of nitpicking odd after the
> series has been pending for almost a year.

TBH when I saw this I was about to comment that the patch won't build
because the prototypes was missing, but then I remembered about patch
1.  I don't think 

[PATCH v2 0/5] Fine granular configuration

[Luca Fancellu]

This serie aims to add more modularity to some feature that can be excluded
without issues from the build.

The first patch is already reviewed.

v2 update: So I've tried to see how to put the dom0less code in the common code,
but the amount of modifications are not trivial, even putting only the common
part and protecting them with ARM, leaving the ARM specific stuff under arch/
like gic etc... will leave a status that is not very nice, so I've decided for
now to keep everything on the arm architecture so that who is going to work
on unifying the code in common can just take from there and do the proper
rework.

This serie is not targeting 4.18.

Luca Fancellu (5):
  arm/gicv2: make GICv2 driver and vGICv2 optional
  xen/arm: Add asm/domain.h include to kernel.h
  arm/dom0less: put dom0less feature code in a separate module
  xen/arm: Move static memory build code in separate modules
  arm/dom0less: introduce Kconfig for dom0less feature

 xen/arch/arm/Kconfig  |   28 +
 xen/arch/arm/Makefile |7 +-
 xen/arch/arm/bootfdt.c|  161 +-
 xen/arch/arm/dom0less-build.c | 1087 ++
 xen/arch/arm/domain_build.c   | 3676 ++---
 xen/arch/arm/efi/efi-boot.h   |4 +
 xen/arch/arm/gic-v3.c |4 +
 xen/arch/arm/include/asm/dom0less-build.h |   35 +
 xen/arch/arm/include/asm/domain_build.h   |   34 +
 xen/arch/arm/include/asm/kernel.h |1 +
 xen/arch/arm/include/asm/setup.h  |1 -
 xen/arch/arm/include/asm/static-memory.h  |   50 +
 xen/arch/arm/include/asm/static-shmem.h   |   72 +
 xen/arch/arm/setup.c  |   58 +-
 xen/arch/arm/static-memory.c  |  294 ++
 xen/arch/arm/static-shmem.c   |  515 +++
 xen/arch/arm/vgic.c   |2 +
 xen/arch/arm/vgic/Makefile|4 +-
 18 files changed, 3149 insertions(+), 2884 deletions(-)
 create mode 100644 xen/arch/arm/dom0less-build.c
 create mode 100644 xen/arch/arm/include/asm/dom0less-build.h
 create mode 100644 xen/arch/arm/include/asm/static-memory.h
 create mode 100644 xen/arch/arm/include/asm/static-shmem.h
 create mode 100644 xen/arch/arm/static-memory.c
 create mode 100644 xen/arch/arm/static-shmem.c

-- 
2.34.1

[PATCH v2 4/5] xen/arm: Move static memory build code in separate modules

[Luca Fancellu]

Move static memory and static shared memory code in separate modules
so that they are included only when the corresponding feature is
enabled, doing that we modularise the features and we remove some
ifdefs from the code to improve readability.

Move process_shm_node function from bootfdt module and make it
externally visible.

A static inline helper called process_shm_chosen is introduced, it
will call the process_shm function for the '/chosen' node, and will
be used by the function construct_dom0 instead of using directly
process_shm, allowing some #ifdef to be removed.

No functional changes are intended.

Signed-off-by: Luca Fancellu 
---
 xen/arch/arm/Makefile |   2 +
 xen/arch/arm/bootfdt.c| 161 +-
 xen/arch/arm/dom0less-build.c |   4 +-
 xen/arch/arm/domain_build.c   | 614 +-
 xen/arch/arm/include/asm/dom0less-build.h |   2 -
 xen/arch/arm/include/asm/domain_build.h   |  24 -
 xen/arch/arm/include/asm/static-memory.h  |  50 ++
 xen/arch/arm/include/asm/static-shmem.h   |  72 +++
 xen/arch/arm/setup.c  |  25 +-
 xen/arch/arm/static-memory.c  | 294 +++
 xen/arch/arm/static-shmem.c   | 515 ++
 11 files changed, 939 insertions(+), 824 deletions(-)
 create mode 100644 xen/arch/arm/include/asm/static-memory.h
 create mode 100644 xen/arch/arm/include/asm/static-shmem.h
 create mode 100644 xen/arch/arm/static-memory.c
 create mode 100644 xen/arch/arm/static-shmem.c

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 70dd7201ef30..89ef0c9075b5 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -50,6 +50,8 @@ obj-y += setup.o
 obj-y += shutdown.o
 obj-y += smp.o
 obj-y += smpboot.o
+obj-$(CONFIG_STATIC_MEMORY) += static-memory.init.o
+obj-$(CONFIG_STATIC_SHM) += static-shmem.init.o
 obj-y += sysctl.o
 obj-y += time.o
 obj-y += traps.o
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 2673ad17a1e1..fcf851b4c99b 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 
 static bool __init device_tree_node_matches(const void *fdt, int node,
 const char *match)
@@ -402,166 +403,6 @@ static int __init process_domain_node(const void *fdt, 
int node,
MEMBANK_STATIC_DOMAIN);
 }
 
-#ifdef CONFIG_STATIC_SHM
-static int __init process_shm_node(const void *fdt, int node,
-   uint32_t address_cells, uint32_t size_cells)
-{
-const struct fdt_property *prop, *prop_id, *prop_role;
-const __be32 *cell;
-paddr_t paddr, gaddr, size;
-struct meminfo *mem = _mem;
-unsigned int i;
-int len;
-bool owner = false;
-const char *shm_id;
-
-if ( address_cells < 1 || size_cells < 1 )
-{
-printk("fdt: invalid #address-cells or #size-cells for static shared 
memory node.\n");
-return -EINVAL;
-}
-
-/*
- * "xen,shm-id" property holds an arbitrary string with a strict limit
- * on the number of characters, MAX_SHM_ID_LENGTH
- */
-prop_id = fdt_get_property(fdt, node, "xen,shm-id", NULL);
-if ( !prop_id )
-return -ENOENT;
-shm_id = (const char *)prop_id->data;
-if ( strnlen(shm_id, MAX_SHM_ID_LENGTH) == MAX_SHM_ID_LENGTH )
-{
-printk("fdt: invalid xen,shm-id %s, it must be limited to %u 
characters\n",
-   shm_id, MAX_SHM_ID_LENGTH);
-return -EINVAL;
-}
-
-/*
- * "role" property is optional and if it is defined explicitly,
- * it must be either `owner` or `borrower`.
- */
-prop_role = fdt_get_property(fdt, node, "role", NULL);
-if ( prop_role )
-{
-if ( !strcmp(prop_role->data, "owner") )
-owner = true;
-else if ( strcmp(prop_role->data, "borrower") )
-{
-printk("fdt: invalid `role` property for static shared memory 
node.\n");
-return -EINVAL;
-}
-}
-
-/*
- * xen,shared-mem = ;
- * Memory region starting from physical address #paddr of #size shall
- * be mapped to guest physical address #gaddr as static shared memory
- * region.
- */
-prop = fdt_get_property(fdt, node, "xen,shared-mem", );
-if ( !prop )
-return -ENOENT;
-
-if ( len != dt_cells_to_size(address_cells + size_cells + address_cells) )
-{
-if ( len == dt_cells_to_size(size_cells + address_cells) )
-printk("fdt: host physical address must be chosen by users at the 
moment.\n");
-
-printk("fdt: invalid `xen,shared-mem` property.\n");
-return -EINVAL;
-}
-
-cell = (const __be32 *)prop->data;
-device_tree_get_reg(, address_cells, address_cells, , );
-size = dt_next_cell(size_cells, );
-
-if ( !size )
-{
-printk("fdt: the size for static shared memory region can 

[PATCH v2 2/5] xen/arm: Add asm/domain.h include to kernel.h

[Luca Fancellu]

The 'enum domain_type' is defined by 'asm/domain.h' which is not
included (directly or indirectly) by 'asm/kernel.h'.

This currently doesn't break the compilation because asm/domain.h will
included by the user of 'kernel.h'. But it would be better to avoid
relying on it. So add the include in 'asm/domain.h'.

Signed-off-by: Luca Fancellu 
---
Changes from v1:
 - Rephrased commit message (Julien)
---
 xen/arch/arm/include/asm/kernel.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/arm/include/asm/kernel.h 
b/xen/arch/arm/include/asm/kernel.h
index 4617cdc83bac..0a23e86c2d37 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -7,6 +7,7 @@
 #define __ARCH_ARM_KERNEL_H__
 
 #include 
+#include 
 #include 
 
 /*
-- 
2.34.1

[PATCH v2 5/5] arm/dom0less: introduce Kconfig for dom0less feature

[Luca Fancellu]

Introduce a Kconfig for the dom0less feature, enabled by default,
to be able to choose if the feature should be compiled or not.

Provide static inline stubs when the option is disabled for the
functions externally visible.

Use the new Kconfig to remove dom0less DT binding from the efi-boot.h
code when the Kconfig is not enabled.

Signed-off-by: Luca Fancellu 
---
 xen/arch/arm/Kconfig  |  9 +
 xen/arch/arm/Makefile |  2 +-
 xen/arch/arm/efi/efi-boot.h   |  4 
 xen/arch/arm/include/asm/dom0less-build.h | 12 
 4 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 8dc704e802bc..f84b2cc22d08 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -89,6 +89,15 @@ config GICV2
  Driver for the ARM Generic Interrupt Controller v2.
  If unsure, say Y
 
+config DOM0LESS_BOOT
+   bool "Dom0less boot support" if EXPERT
+   depends on ARM
+   default y
+   help
+ Dom0less boot support enables Xen to create and start domU guests 
during
+ Xen boot without the need of a control domain (Dom0), which could be
+ present anyway.
+
 config GICV3
bool "GICv3 driver"
depends on !NEW_VGIC
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 89ef0c9075b5..5daf8f10114d 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -15,7 +15,7 @@ obj-y += cpufeature.o
 obj-y += decode.o
 obj-y += device.o
 obj-$(CONFIG_IOREQ_SERVER) += dm.o
-obj-y += dom0less-build.init.o
+obj-$(CONFIG_DOM0LESS_BOOT) += dom0less-build.init.o
 obj-y += domain.o
 obj-y += domain_build.init.o
 obj-$(CONFIG_ARCH_MAP_DOMAIN_PAGE) += domain_page.o
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 1c3640bb65fd..689dc016d081 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -802,6 +802,7 @@ static int __init handle_module_node(const EFI_LOADED_IMAGE 
*loaded_image,
 return 1;
 }
 
+#ifdef CONFIG_DOM0LESS_BOOT
 /*
  * This function checks for boot modules under the domU guest domain node
  * in the DT.
@@ -849,6 +850,7 @@ static int __init handle_dom0less_domain_node(const 
EFI_LOADED_IMAGE *loaded_ima
 
 return mb_modules_found;
 }
+#endif
 
 /*
  * This function checks for xen domain nodes under the /chosen node for 
possible
@@ -876,6 +878,7 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE 
*loaded_image)
 {
 int ret;
 
+#ifdef CONFIG_DOM0LESS_BOOT
 if ( !fdt_node_check_compatible(fdt_efi, node, "xen,domain") )
 {
 /* Found a node with compatible xen,domain; handle this node. */
@@ -884,6 +887,7 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE 
*loaded_image)
 return ERROR_DT_MODULE_DOMU;
 }
 else
+#endif
 {
 ret = handle_module_node(loaded_image, _handle, node, addr_len,
  size_len, false);
diff --git a/xen/arch/arm/include/asm/dom0less-build.h 
b/xen/arch/arm/include/asm/dom0less-build.h
index d95cb6234b62..859944eece16 100644
--- a/xen/arch/arm/include/asm/dom0less-build.h
+++ b/xen/arch/arm/include/asm/dom0less-build.h
@@ -8,9 +8,21 @@
 #ifndef __ARM_DOM0LESS_BUILD_H_
 #define __ARM_DOM0LESS_BUILD_H_
 
+#ifdef CONFIG_DOM0LESS_BOOT
+
 void create_domUs(void);
 bool is_dom0less_mode(void);
 
+#else  /* !CONFIG_DOM0LESS_BOOT */
+
+static inline void create_domUs(void) {}
+static inline bool is_dom0less_mode(void)
+{
+return false;
+}
+
+#endif /* CONFIG_DOM0LESS_BOOT */
+
 #endif  /* __ARM_DOM0LESS_BUILD_H_ */
 
 /*
-- 
2.34.1

[PATCH v2 1/5] arm/gicv2: make GICv2 driver and vGICv2 optional

[Luca Fancellu]

Introduce Kconfig GICV2 to be able to compile the GICv2 driver only
when needed, the option is active by default.

Introduce Kconfig VGICV2 that compiles the Generic Interrupt
Controller v2 emulation for domains, it is required only when using
GICv2 driver, otherwise using the GICv3 driver it is optional and can
be deselected if the user doesn't want to offer the v2 emulation to
domains or maybe its GICv3 hardware can't offer the GICv2 compatible
mode.

Signed-off-by: Luca Fancellu 
Reviewed-by: Julien Grall 
Reviewed-by: Michal Orzel 
---
 xen/arch/arm/Kconfig| 19 +++
 xen/arch/arm/Makefile   |  4 ++--
 xen/arch/arm/domain_build.c |  4 
 xen/arch/arm/gic-v3.c   |  4 
 xen/arch/arm/vgic.c |  2 ++
 xen/arch/arm/vgic/Makefile  |  4 ++--
 6 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 632dd9792e3c..8dc704e802bc 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -81,6 +81,14 @@ config ARM_EFI
  UEFI firmware. A UEFI stub is provided to allow Xen to
  be booted as an EFI application.
 
+config GICV2
+   bool "GICv2 driver"
+   default y
+   select VGICV2
+   help
+ Driver for the ARM Generic Interrupt Controller v2.
+ If unsure, say Y
+
 config GICV3
bool "GICv3 driver"
depends on !NEW_VGIC
@@ -100,11 +108,22 @@ config OVERLAY_DTB
help
  Dynamic addition/removal of Xen device tree nodes using a dtbo.
 
+config VGICV2
+   bool "vGICv2 interface for domains"
+   default y
+   help
+ Allow Xen to expose a Generic Interrupt Controller version 2 like to 
Xen
+ domains. This can be configured at the domain creation.
+ This option is mandatory when using GICv2.
+ For GICv3, this allows domain to use GICv2 when the hardware supports 
it.
+ If unsure say Y.
+
 config HVM
 def_bool y
 
 config NEW_VGIC
bool "Use new VGIC implementation"
+   select GICV2
---help---
 
This is an alternative implementation of the ARM GIC interrupt
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 7bf07e992046..81c31c36fc3d 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -22,7 +22,7 @@ obj-y += domctl.o
 obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
 obj-y += efi/
 obj-y += gic.o
-obj-y += gic-v2.o
+obj-$(CONFIG_GICV2) += gic-v2.o
 obj-$(CONFIG_GICV3) += gic-v3.o
 obj-$(CONFIG_HAS_ITS) += gic-v3-its.o
 obj-$(CONFIG_HAS_ITS) += gic-v3-lpi.o
@@ -57,7 +57,7 @@ obj-$(CONFIG_NEW_VGIC) += vgic/
 ifneq ($(CONFIG_NEW_VGIC),y)
 obj-y += gic-vgic.o
 obj-y += vgic.o
-obj-y += vgic-v2.o
+obj-$(CONFIG_VGICV2) += vgic-v2.o
 obj-$(CONFIG_GICV3) += vgic-v3.o
 obj-$(CONFIG_HAS_ITS) += vgic-v3-its.o
 endif
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 24c9019cc43c..f69f238ccea4 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2490,6 +2490,7 @@ static int __init handle_node(struct domain *d, struct 
kernel_info *kinfo,
 return res;
 }
 
+#ifdef CONFIG_VGICV2
 static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
 {
 void *fdt = kinfo->fdt;
@@ -2541,6 +2542,7 @@ static int __init make_gicv2_domU_node(struct kernel_info 
*kinfo)
 
 return res;
 }
+#endif
 
 #ifdef CONFIG_GICV3
 static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
@@ -2616,8 +2618,10 @@ static int __init make_gic_domU_node(struct kernel_info 
*kinfo)
 case GIC_V3:
 return make_gicv3_domU_node(kinfo);
 #endif
+#ifdef CONFIG_VGICV2
 case GIC_V2:
 return make_gicv2_domU_node(kinfo);
+#endif
 default:
 panic("Unsupported GIC version\n");
 }
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 95e4f020febe..d18a3317ccc4 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1334,6 +1334,7 @@ static paddr_t __initdata dbase = INVALID_PADDR;
 static paddr_t __initdata vbase = INVALID_PADDR, vsize = 0;
 static paddr_t __initdata cbase = INVALID_PADDR, csize = 0;
 
+#ifdef CONFIG_VGICV2
 /* If the GICv3 supports GICv2, initialize it */
 static void __init gicv3_init_v2(void)
 {
@@ -1359,6 +1360,9 @@ static void __init gicv3_init_v2(void)
 
 vgic_v2_setup_hw(dbase, cbase, csize, vbase, 0);
 }
+#else
+static inline void gicv3_init_v2(void) { }
+#endif
 
 static void __init gicv3_ioremap_distributor(paddr_t dist_paddr)
 {
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index f6b49766f97a..c04fc4f83f96 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -96,10 +96,12 @@ int domain_vgic_register(struct domain *d, unsigned int 
*mmio_count)
return -ENODEV;
 break;
 #endif
+#ifdef CONFIG_VGICV2
 case GIC_V2:
 if ( vgic_v2_init(d, mmio_count) )
 return -ENODEV;
 break;
+#endif
 default:
 printk(XENLOG_G_ERR "d%d: Unknown vGIC version %u\n",

[PATCH v2 3/5] arm/dom0less: put dom0less feature code in a separate module

[Luca Fancellu]

Currently the dom0less feature code is mostly inside domain_build.c
and setup.c, it is a feature that may not be useful to everyone so
put the code in a different compilation module in order to make it
easier to disable the feature in the future.

Move gic_interrupt_t in domain_build.h to use it with the function
declaration, move its comment above the declaration.

The following functions are now visible externally from domain_build
because they are used also from the dom0less-build module:
 - get_allocation_size
 - set_interrupt
 - domain_fdt_begin_node
 - make_memory_node
 - make_resv_memory_node
 - make_hypervisor_node
 - make_psci_node
 - make_cpus_node
 - make_timer_node
 - handle_device_interrupts
 - construct_domain
 - process_shm

The functions allocate_static_memory and assign_static_memory_11
are now externally visible, so put their declarations into
domain_build.h and move the #else and stub definition in the header
as well.

Move is_dom0less_mode from setup.c to dom0less-build.c and make it
externally visible.

Where spotted, fix code style issues.

No functional change is intended.

Signed-off-by: Luca Fancellu 
---
 xen/arch/arm/Makefile |1 +
 xen/arch/arm/dom0less-build.c | 1087 ++
 xen/arch/arm/domain_build.c   | 1276 ++---
 xen/arch/arm/include/asm/dom0less-build.h |   25 +
 xen/arch/arm/include/asm/domain_build.h   |   58 +
 xen/arch/arm/include/asm/setup.h  |1 -
 xen/arch/arm/setup.c  |   33 +-
 7 files changed, 1288 insertions(+), 1193 deletions(-)
 create mode 100644 xen/arch/arm/dom0less-build.c
 create mode 100644 xen/arch/arm/include/asm/dom0less-build.h

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 81c31c36fc3d..70dd7201ef30 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -15,6 +15,7 @@ obj-y += cpufeature.o
 obj-y += decode.o
 obj-y += device.o
 obj-$(CONFIG_IOREQ_SERVER) += dm.o
+obj-y += dom0less-build.init.o
 obj-y += domain.o
 obj-y += domain_build.init.o
 obj-$(CONFIG_ARCH_MAP_DOMAIN_PAGE) += domain_page.o
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
new file mode 100644
index ..dc9c90cf00a7
--- /dev/null
+++ b/xen/arch/arm/dom0less-build.c
@@ -0,0 +1,1087 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * xen/arch/arm/dom0less-build.c
+ *
+ * Code related to the dom0less functionality
+ *
+ * Copyright (C) 2023 Arm Ltd.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+bool __init is_dom0less_mode(void)
+{
+struct bootmodules *mods = 
+struct bootmodule *mod;
+unsigned int i;
+bool dom0found = false;
+bool domUfound = false;
+
+/* Look into the bootmodules */
+for ( i = 0 ; i < mods->nr_mods ; i++ )
+{
+mod = >module[i];
+/* Find if dom0 and domU kernels are present */
+if ( mod->kind == BOOTMOD_KERNEL )
+{
+if ( mod->domU == false )
+{
+dom0found = true;
+break;
+}
+else
+domUfound = true;
+}
+}
+
+/*
+ * If there is no dom0 kernel but at least one domU, then we are in
+ * dom0less mode
+ */
+return ( !dom0found && domUfound );
+}
+
+static bool __init allocate_bank_memory(struct domain *d,
+struct kernel_info *kinfo,
+gfn_t sgfn,
+paddr_t tot_size)
+{
+int res;
+struct page_info *pg;
+struct membank *bank;
+unsigned int max_order = ~0;
+
+/*
+ * allocate_bank_memory can be called with a tot_size of zero for
+ * the second memory bank. It is not an error and we can safely
+ * avoid creating a zero-size memory bank.
+ */
+if ( tot_size == 0 )
+return true;
+
+bank = >mem.bank[kinfo->mem.nr_banks];
+bank->start = gfn_to_gaddr(sgfn);
+bank->size = tot_size;
+
+while ( tot_size > 0 )
+{
+unsigned int order = get_allocation_size(tot_size);
+
+order = min(max_order, order);
+
+pg = alloc_domheap_pages(d, order, 0);
+if ( !pg )
+{
+/*
+ * If we can't allocate one page, then it is unlikely to
+ * succeed in the next iteration. So bail out.
+ */
+if ( !order )
+return false;
+
+/*
+ * If we can't allocate memory with order, then it is
+ * unlikely to succeed in the next iteration.
+ * Record the order - 1 to avoid re-trying.
+ */
+max_order = order - 1;
+continue;
+}
+
+res = guest_physmap_add_page(d, sgfn, page_to_mfn(pg), order);
+if ( res )
+{
+dprintk(XENLOG_ERR, 

Re: [PATCH v3 4/8] x86/mem-sharing: copy GADDR based shared guest areas

[Roger Pau Monné]

On Wed, Sep 27, 2023 at 07:43:26AM -0400, Tamas K Lengyel wrote:
> On Wed, Sep 27, 2023 at 7:08 AM Roger Pau Monné  wrote:
> >
> > On Wed, May 03, 2023 at 05:56:46PM +0200, Jan Beulich wrote:
> > > In preparation of the introduction of new vCPU operations allowing to
> > > register the respective areas (one of the two is x86-specific) by
> > > guest-physical address, add the necessary fork handling (with the
> > > backing function yet to be filled in).
> > >
> > > Signed-off-by: Jan Beulich 
> >
> > Given the very limited and specific usage of the current Xen forking
> > code, do we really need to bother about copying such areas?
> >
> > IOW: I doubt that not updating the runstate/time areas will make any
> > difference to the forking code ATM.
> 
> The current implementation of forking allows for fully functional VM
> forks to be setup, including launching the dm for them. The toolstack
> side hasn't been merged for that into Xen but that doesn't mean it's
> not available or used already. So any internal changes to Xen that
> create guest-states that could potentially be interacted with and
> relied on by a guest should get properly wired in for forking. So I'm
> happy Jan took the initiative here for wiring this up, even if the
> use-case seems niche.

But there are still some bits missing in Xen, seeing the comment in
copy_vcpu_settings().  If we don't copy the timers already then I'm
unsure whether copying the runstate/time shared areas is very
relevant.

> >
> > > ---
> > > v3: Extend comment.
> > >
> > > --- a/xen/arch/x86/mm/mem_sharing.c
> > > +++ b/xen/arch/x86/mm/mem_sharing.c
> > > @@ -1641,6 +1641,68 @@ static void copy_vcpu_nonreg_state(struc
> > >  hvm_set_nonreg_state(cd_vcpu, );
> > >  }
> > >
> > > +static int copy_guest_area(struct guest_area *cd_area,
> > > +   const struct guest_area *d_area,
> > > +   struct vcpu *cd_vcpu,
> > > +   const struct domain *d)
> > > +{
> > > +mfn_t d_mfn, cd_mfn;
> > > +
> > > +if ( !d_area->pg )
> > > +return 0;
> > > +
> > > +d_mfn = page_to_mfn(d_area->pg);
> > > +
> > > +/* Allocate & map a page for the area if it hasn't been already. */
> > > +if ( !cd_area->pg )
> > > +{
> > > +gfn_t gfn = mfn_to_gfn(d, d_mfn);
> > > +struct p2m_domain *p2m = p2m_get_hostp2m(cd_vcpu->domain);
> > > +p2m_type_t p2mt;
> > > +p2m_access_t p2ma;
> > > +unsigned int offset;
> > > +int ret;
> > > +
> > > +cd_mfn = p2m->get_entry(p2m, gfn, , , 0, NULL, NULL);
> > > +if ( mfn_eq(cd_mfn, INVALID_MFN) )
> > > +{
> > > +struct page_info *pg = alloc_domheap_page(cd_vcpu->domain, 
> > > 0);
> > > +
> > > +if ( !pg )
> > > +return -ENOMEM;
> > > +
> > > +cd_mfn = page_to_mfn(pg);
> > > +set_gpfn_from_mfn(mfn_x(cd_mfn), gfn_x(gfn));
> > > +
> > > +ret = p2m->set_entry(p2m, gfn, cd_mfn, PAGE_ORDER_4K, 
> > > p2m_ram_rw,
> > > + p2m->default_access, -1);
> > > +if ( ret )
> > > +return ret;
> > > +}
> > > +else if ( p2mt != p2m_ram_rw )
> > > +return -EBUSY;
> >
> > Shouldn't the populate of the underlying gfn in the fork case
> > be done by map_guest_area itself?
> 
> Would seem more logical, I agree, but then the call should be
> map_guest_area first, which conditionally calls down into a
> mem_sharing_copy_guest_area if the domain is a fork.
> 
> >
> > What if a forked guest attempts to register a new runstate/time info
> > against an address not yet populated?
> 
> Unpopulated memory will get forked from the parent for all access
> paths currently in existence, including access to a forked VMs memory
> from dom0/dm and the various internal Xen access paths. If the memory
> range is not mapped in the parent registering on that range ought to
> fail by I assume existing checks that validate that the memory is
> present during registration.

What I'm trying to say is that map_guest_area() already has to deal
with forked guests, and hence the populating of the gfn shouldn't be
needed as map_guest_area() must know how to deal with such guest
anyway.

Thanks, Roger.

Re: [XEN PATCH v2 3/3] automation/eclair: build docs/misra to address MISRA C:2012 Dir 4.1

[Nicola Vetrini]

On 27/09/2023 11:52, Nicola Vetrini wrote:

The documentation pertaining Directive 4.1 is contained in docs/misra.
The build script driving the analysis is amended to allow ECLAIR to
analyze such file.

Signed-off-by: Nicola Vetrini 
---
Changes in v2:
- removed useless make flags
---
 automation/eclair_analysis/build.sh   | 6 +++---
 automation/eclair_analysis/prepare.sh | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/automation/eclair_analysis/build.sh
b/automation/eclair_analysis/build.sh
index ec087dd822fa..ea7a1e5a59b0 100755
--- a/automation/eclair_analysis/build.sh
+++ b/automation/eclair_analysis/build.sh
@@ -34,11 +34,11 @@ else
 fi

 (
-  cd xen
-
+  make -C docs misra
   make "-j${PROCESSORS}" "-l${PROCESSORS}.0"\
"CROSS_COMPILE=${CROSS_COMPILE}" \
"CC=${CROSS_COMPILE}gcc-12"  \
"CXX=${CROSS_COMPILE}g++-12" \
-   "XEN_TARGET_ARCH=${XEN_TARGET_ARCH}"
+   "XEN_TARGET_ARCH=${XEN_TARGET_ARCH}" \
+   -C xen
 )
diff --git a/automation/eclair_analysis/prepare.sh
b/automation/eclair_analysis/prepare.sh
index 0cac5eba00ae..ebd5a2dde676 100755
--- a/automation/eclair_analysis/prepare.sh
+++ b/automation/eclair_analysis/prepare.sh
@@ -35,8 +35,8 @@ else
 fi

 (
-cd xen
-cp "${CONFIG_FILE}" .config
+./configure
+cp "${CONFIG_FILE}" xen/.config
 make clean
 find . -type f -name "*.safparse" -print -delete
 make -f ${script_dir}/Makefile.prepare prepare


Hi, I observed a failure when running the analysis job of this series 
through patchew, so
I think it's a good idea to put this patch on hold until I've figured 
out what's wrong.

Sorry for the inconvenience.

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

Re: Xen on AWS EC2 Graviton 2 metal instances (c6g.metal)

[Julien Grall]

Hi Dan,

Thanks for the report.

On 26/09/2023 20:41, Driscoll, Dan wrote:

First off - sorry for the very long email, but there are a lot of 
details related to this topic and I figured more details might be better than 
less but I could be wrong here

Within Siemens Embedded, we have been doing some prototyping using Xen 
for some upcoming customer related work - this email thread attempts to explain 
what has been done here and our analysis of the problems we are having.

We have done some initial prototyping to get Xen running on an AWS Graviton 2 
instance using an EC2 Arm64 "metal" instance (c6g.metal - no AWS hypervisor) 
and ran into some problems during this prototyping.

Since the Edge Workload Abstraction and Orchestration Layer (EWAOL) 
that is part of SOAFEE already has some enablement of Xen in various 
environments (including an Arm64 server environment), we used this as a 
starting point.

We were able to successfully bring up Xen and a Yocto dom0 and multiple 
domu Yocto guests on an Arm AVA server (AVA Developer Platform - 32 core 
Neoverse N1 server) following documented steps with some minimal configuration 
changes (we simply extended the configuration to include 3 Linux guests): 
https://ewaol.docs.arm.com/en/kirkstone-dev/manual/build_system.html#build-system

So, this specific EWAOL support has all the proper bitbake layers to 
generate images for both bare-metal (Linux running natively) and a 
virtualization build (using Xen) for AVA and also a Neoverse N1 System 
Development Platform (N1SDP), but we only verified this on AVA.
c6g.medium
AWS also has support for EWAOL on Graviton 2, but the only supported 
configuration is a bare-metal configuration (Linux running natively) and the 
virtualization build hasn't been implemented in the bitbake layers in their 
repo - here is the URL for information / instructions on this support: 
https://github.com/aws4embeddedlinux/meta-aws-ewaol
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/grub.html
As part of our effort to bring this up, we did a VERY minimal patch to 
the repo used for the AWS EWAOL to generate a virtualization build (attached 
meta-aws-ewaol.patch).  The resultant build of the AWS EWAOL support with this 
patch applied does result in Xen being built as well as a dom0 Yocto kernel, 
but there is definitely missing support to properly build everything for this 
virtualization layer.  Following the instructions for meta-aws-ewaol,  we 
generated an AMI and started an EC2 instance with this AMI (c6g.metal type).  
The resultant image does boot, but it boots into the dom0 Linux kernel with 
problems recorded in the boot log related to Xen (see dom0-linux-boot.txt).

Looking more closely at the EFI partition, it was clear that systemd-boot was 
being used and it was set-up to boot the dom0 Linux kernel and not boot into Xen - the 
Xen EFI images were not present in the EFI partition and obviously no launch entries 
existed for Xen.  To rectify this, the Xen EFI image that were built as part of the AWS 
EWAOL build mentioned above where placed in the EFI partition, along with a Xen config 
file that provided the dom0 Linux kernel image details.  A new entry was added into the 
EFI image for Xen and the launch conf file was updated to boot Xen instead of dom0 Linux. 
 This resulted in the EC2 instance becoming "bricked" and no longer accessible.

Details on the EFI related content and changes we made are captured in the meta-aws-ewaol-efi-boot-changes.txt file attached above.

The next step was comparing the AVA Xen output that was working and we noticed a few differences - the AVA build did enable ACPI and UNSUPPORTED kconfig settings whereas the AWS Xen build did not.  So, we tried again to bring up another EC2 metal instance using the same AMI as before and utilized the AVA Xen EFI image instead and same Xen config file.  The result was the same - a "bricked" instance.

We will likely try to use the entire AVA flow on AWS Graviton next as it is using GRUB 2 instead of systemd-boot and we hope to maybe extend or enable some of the debug output during boot.  The AWS EC2 instances have a "serial console", but we have yet to see any output on this console prior to Linux boot logs - no success in getting EC2 serial output during EFI booting.


That's interesting. The documentation for AWS [1] suggests that the logs 
from boot should be seen. They even have a page for troubleshooting 
using GRUB [2].


I just launched a c6g.metal and I could access the serial console but 
then it didn't work across reboot.


I have tried a c6g.medium and the serial was working across reboot (I 
could see some logs). So I wonder whether the serial console is there is 
a missing configuration for baremetal?



We have had a call and some email exchanges with AWS on this topic (Luke Harvey, Jeremy Dahan, Robert 

Re: [PATCH] xen/arm: Skip memory nodes if not enabled

[Michal Orzel]

Hi Julien,

On 27/09/2023 13:01, Julien Grall wrote:
> 
> 
> Hi Michal,
> 
> On 26/09/2023 09:36, Michal Orzel wrote:
>> On 26/09/2023 07:33, Leo Yan wrote:
>>>
>>>
>>> During the Linux kernel booting, an error is reported by the Xen
>>> hypervisor:
>>>
>>>(XEN) arch/arm/p2m.c:2202: d0v0: Failing to acquire the MFN 0x1a02dc
>>>
>>> The kernel attempts to use an invalid memory frame number, which can be
>>> converted to: 0x1a02dc << PAGE_SHIFT, resulting in 0x1_a02d_c000.
>>>
>>> The invalid memory frame falls into a reserved memory node, which is
>>> defined in the device tree as below:
>>>
>>>reserved-memory {
>>>#address-cells = <0x02>;
>>>#size-cells = <0x02>;
>>>ranges;
>>>
>>>...
>>>
>>>ethosn_reserved {
>>>compatible = "shared-dma-pool";
>>>reg = <0x01 0xa000 0x00 0x2000>;
>>>status = "disabled";
>>>no-map;
>>>};
>>>
>>>...
>>>};
>>>
>>> Xen excludes all reserved memory regions from the frame management
>>> through the dt_unreserved_regions() function. On the other hand, the
>>> reserved memory nodes are passed to the Linux kernel. However, the Linux
>>> kernel ignores the 'ethosn_reserved' node since its status is
>>> "disabled". This leads to the Linux kernel to allocate pages from the
>>> reserved memory range.
>>>
>>> As a result, when the kernel passes the data structure residing in the
>>> frame 0x1_a02d_c000 in the Xen hypervisor, the hypervisor detects that
>>> it misses to manage the frame and reports the error.
>>>
>>> Essentially, this issue is caused by the Xen hypervisor which misses to
>>> handle the status for the memory nodes (for both the normal memory nodes
>>> and the reserved memory nodes) and transfers them to the Linux kernel.
>>>
>>> This patch introduces a function memory_node_is_available(). If it
>>> detects a memory node is not enabled, the hypervisor will not add the
>>> memory region into the memory lists. In the end, this avoids to generate
>>> the memory nodes from the memory lists sent to the kernel and the kernel
>>> cannot use the disabled memory nodes any longer.
>>
>> Interesting. So FWICS, we have 2 issues that have a common ground:
>> 1) If the reserved memory node has a status "disabled", it implies that this 
>> region
>> is no longer reserved and can be used which is not handled today by Xen and 
>> leads
>> to the above mentioned problem.
>>
>> 2) If the memory node has a status "disabled" it implies that it should not 
>> be used
>> which is not the case in current Xen. This means that at the moment, Xen 
>> would try
>> to use such a memory region which is incorrect.
>>
>> I think the commit msg should be more generic and focus on these two issues.
>> Summary:
>> Xen does not handle the status property of memory nodes and ends up using 
>> them.
>> Xen does not handle the status property of reserved memory nodes. If status 
>> is disabled
>> it means the reserved region shall no longer be treated as reserved. Xen 
>> passes the reserved
>> memory nodes untouched to dom0 fdt and creates a memory node to cover 
>> reserved regions.
>> Disabled reserved memory nodes are ignored by the guest which leaves us with 
>> the exposed
>> memory nodes. Guest can access such a region but it is excluded from the 
>> page management in Xen
>> which results in Xen being unable to obtain the corresponding MFN.
>>
>>>
>>> Signed-off-by: Leo Yan 
>>> ---
>>>   xen/arch/arm/bootfdt.c | 16 
>>>   1 file changed, 16 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>>> index 2673ad17a1..b46dde06a9 100644
>>> --- a/xen/arch/arm/bootfdt.c
>>> +++ b/xen/arch/arm/bootfdt.c
>>> @@ -206,11 +206,27 @@ int __init device_tree_for_each_node(const void *fdt, 
>>> int node,
>>>   return 0;
>>>   }
>>>
>>> +static bool __init memory_node_is_available(const void *fdt, unsigned long 
>>> node)
>> This function is not strictly related to memory node so it would be better 
>> to call it e.g. device_tree_node_is_available.
> 
> +1.
> 
>>> +{
>>> +const char *status = fdt_getprop(fdt, node, "status", NULL);
>>> +
>>> +if (!status)
>> white spaces between brackets and condition
>> if ( !status )
>>
>>> +return true;
>>> +
>>> +if (!strcmp(status, "ok") || !strcmp(status, "okay"))
>> white spaces between brackets and condition
>> if ( !strcmp(status, "ok") || !strcmp(status, "okay") )
>>
>>> +return true;
>>> +
>>> +return false;
>>> +}
>>> +
>>>   static int __init process_memory_node(const void *fdt, int node,
>>> const char *name, int depth,
>>> u32 address_cells, u32 size_cells,
>>> void *data)
>>>   {
>>> +if (!memory_node_is_available(fdt, node))
>>> +return 0;
>> I would move this check to 

Re: [PATCH v3 4/8] x86/mem-sharing: copy GADDR based shared guest areas

[Jan Beulich]

On 27.09.2023 13:08, Roger Pau Monné wrote:
> On Wed, May 03, 2023 at 05:56:46PM +0200, Jan Beulich wrote:
>> In preparation of the introduction of new vCPU operations allowing to
>> register the respective areas (one of the two is x86-specific) by
>> guest-physical address, add the necessary fork handling (with the
>> backing function yet to be filled in).
>>
>> Signed-off-by: Jan Beulich 
> 
> Given the very limited and specific usage of the current Xen forking
> code, do we really need to bother about copying such areas?
> 
> IOW: I doubt that not updating the runstate/time areas will make any
> difference to the forking code ATM.

I expect Tamas'es reply has sufficiently addressed this question.

>> --- a/xen/arch/x86/mm/mem_sharing.c
>> +++ b/xen/arch/x86/mm/mem_sharing.c
>> @@ -1641,6 +1641,68 @@ static void copy_vcpu_nonreg_state(struc
>>  hvm_set_nonreg_state(cd_vcpu, );
>>  }
>>  
>> +static int copy_guest_area(struct guest_area *cd_area,
>> +   const struct guest_area *d_area,
>> +   struct vcpu *cd_vcpu,
>> +   const struct domain *d)
>> +{
>> +mfn_t d_mfn, cd_mfn;
>> +
>> +if ( !d_area->pg )
>> +return 0;
>> +
>> +d_mfn = page_to_mfn(d_area->pg);
>> +
>> +/* Allocate & map a page for the area if it hasn't been already. */
>> +if ( !cd_area->pg )
>> +{
>> +gfn_t gfn = mfn_to_gfn(d, d_mfn);
>> +struct p2m_domain *p2m = p2m_get_hostp2m(cd_vcpu->domain);
>> +p2m_type_t p2mt;
>> +p2m_access_t p2ma;
>> +unsigned int offset;
>> +int ret;
>> +
>> +cd_mfn = p2m->get_entry(p2m, gfn, , , 0, NULL, NULL);
>> +if ( mfn_eq(cd_mfn, INVALID_MFN) )
>> +{
>> +struct page_info *pg = alloc_domheap_page(cd_vcpu->domain, 0);
>> +
>> +if ( !pg )
>> +return -ENOMEM;
>> +
>> +cd_mfn = page_to_mfn(pg);
>> +set_gpfn_from_mfn(mfn_x(cd_mfn), gfn_x(gfn));
>> +
>> +ret = p2m->set_entry(p2m, gfn, cd_mfn, PAGE_ORDER_4K, 
>> p2m_ram_rw,
>> + p2m->default_access, -1);
>> +if ( ret )
>> +return ret;
>> +}
>> +else if ( p2mt != p2m_ram_rw )
>> +return -EBUSY;
> 
> Shouldn't the populate of the underlying gfn in the fork case
> be done by map_guest_area itself?

I've used the existing logic for the info area to base my code on. As
noted in the patch switching the info area logic to use the generalize
infrastructure, I've taken the liberty to address an issue in the
original logic. But it was never a goal to re-write things from scratch.
If, as Tamas appears to agree, there a better way of layering things
here, then please as a follow-on patch.

> What if a forked guest attempts to register a new runstate/time info
> against an address not yet populated?

What if the same happened for the info area? Again, I'm not trying to
invent anything new here. But hopefully Tamas'es reply has helped here
as well.

>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -1572,6 +1572,13 @@ void unmap_vcpu_info(struct vcpu *v)
>>  put_page_and_type(mfn_to_page(mfn));
>>  }
>>  
>> +int map_guest_area(struct vcpu *v, paddr_t gaddr, unsigned int size,
>> +   struct guest_area *area,
>> +   void (*populate)(void *dst, struct vcpu *v))
> 
> Oh, the prototype for this is added in patch 1, almost missed it.
> 
> Why not also add this dummy implementation in patch 1 then?

The prototype isn't dead code, but the function would be until it gains
users here. If anything, I'd move the prototype introduction here; it
merely seemed desirable to me to touch xen/include/xen/domain.h no
more frequently than necessary.

Also, to be quite frank, I find this kind of nitpicking odd after the
series has been pending for almost a year.

Jan

Re: [PATCH v3 1/8] domain: GADDR based shared guest area registration alternative - teardown

[Jan Beulich]

On 27.09.2023 12:50, Roger Pau Monné wrote:
> On Wed, Sep 27, 2023 at 12:46:07PM +0200, Jan Beulich wrote:
>> On 27.09.2023 12:42, Roger Pau Monné wrote:
>>> On Wed, Sep 27, 2023 at 11:55:19AM +0200, Jan Beulich wrote:
 On 27.09.2023 10:51, Roger Pau Monné wrote:
> On Wed, May 03, 2023 at 05:54:47PM +0200, Jan Beulich wrote:
>> +{
>> +struct domain *d = v->domain;
>> +
>> +if ( v != current )
>> +ASSERT(atomic_read(>pause_count) | 
>> atomic_read(>pause_count));
>
> Isn't this racy?

 It is, yes.

>  What guarantees that the vcpu won't be kicked just
> after the check has been performed?

 Nothing. This check isn't any better than assertions towards an ordinary
 spinlock being held. I assume you realize that we've got a number of such
 assertions elsewhere already.
>>>
>>> Right, but different from spinlock assertions, the code here could be
>>> made safe just by pausing the vCPU?
>>
>> That's what the assertion is checking (see also the comment ahead of the
>> function). It's just that the assertions cannot be made more strict, at
>> least from all I can tell.
> 
> But the assertion might no longer be true by the time the code
> afterwards is executed.  Why not wrap the code in a pair of
> vcpu_{,un}pause() calls?

Because it's not quite as simple (if I was to do so, I'd want to do it
correctly, and not just give the impression of universal usability). See
how map_guest_area() involves hypercall_deadlock_mutex. Hence I continue
to think it is okay the way I have it, with all callers satisfying the
requirement (afaict).

Jan

Re: [PATCH v3] docs/misra: add rule 2.1 exceptions

[Luca Fancellu]

> On 27 Sep 2023, at 09:35, Bertrand Marquis  wrote:
> 
> Hi Jan,
> 
>> On 27 Sep 2023, at 10:23, Jan Beulich  wrote:
>> 
>> On 27.09.2023 10:14, Bertrand Marquis wrote:
 On 27 Sep 2023, at 09:53, Nicola Vetrini  
 wrote:
 My opinion is that it's far easier for this to be an eclair configuration 
 (which has the
 advantage not to depend on the exact definition of unreachable) and then 
 perhaps a comment
 above it explaining the situation.
>>> 
>>> I agree here and it is easier to make an overall exception where we list 
>>> the cases
>>> where this is acceptable (ie all flavors of unreacheable) and document that 
>>> eclair
>>> was configured using "" to handle this.
>> 
>> What about cppcheck then, for example?
> 
> Good point we should check if cppcheck or coverity can do such things.
> @Luca: any idea ?

So, for cppcheck I don’t think we have such granularity, the only thing we can 
do are suppress all violations
for a file, suppress some violations for a file or suppress globally all 
violations regarding certain rules.

For coverity, I’ve found the way to remove files (translation units) from the 
report, but I didn’t find anything
about how to specify some patterns to be excluded. For now I can only exclude 
entire files or I can exclude
rules globally.
I will try to get some support from Synopsys to see if there is any way to 
specify some exclusion pattern for
specific rules.

Anyway I’ve run Coverity and for the 2.1 it is finding 14 violations but none 
of them are about __builtin_unreachable().

I’ve also run Cppcheck and it is not complaining, not that I was looking for it 
to be a benchmark in any case!

So I guess Eclair is more strict on the checks and needs to have a proper 
configuration that can’t be generalised
for all the tools.

Cheers,
Luca

Re: [PATCH v3 4/8] x86/mem-sharing: copy GADDR based shared guest areas

[Roger Pau Monné]

On Wed, May 03, 2023 at 05:56:46PM +0200, Jan Beulich wrote:
> In preparation of the introduction of new vCPU operations allowing to
> register the respective areas (one of the two is x86-specific) by
> guest-physical address, add the necessary fork handling (with the
> backing function yet to be filled in).
> 
> Signed-off-by: Jan Beulich 

Given the very limited and specific usage of the current Xen forking
code, do we really need to bother about copying such areas?

IOW: I doubt that not updating the runstate/time areas will make any
difference to the forking code ATM.

> ---
> v3: Extend comment.
> 
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1641,6 +1641,68 @@ static void copy_vcpu_nonreg_state(struc
>  hvm_set_nonreg_state(cd_vcpu, );
>  }
>  
> +static int copy_guest_area(struct guest_area *cd_area,
> +   const struct guest_area *d_area,
> +   struct vcpu *cd_vcpu,
> +   const struct domain *d)
> +{
> +mfn_t d_mfn, cd_mfn;
> +
> +if ( !d_area->pg )
> +return 0;
> +
> +d_mfn = page_to_mfn(d_area->pg);
> +
> +/* Allocate & map a page for the area if it hasn't been already. */
> +if ( !cd_area->pg )
> +{
> +gfn_t gfn = mfn_to_gfn(d, d_mfn);
> +struct p2m_domain *p2m = p2m_get_hostp2m(cd_vcpu->domain);
> +p2m_type_t p2mt;
> +p2m_access_t p2ma;
> +unsigned int offset;
> +int ret;
> +
> +cd_mfn = p2m->get_entry(p2m, gfn, , , 0, NULL, NULL);
> +if ( mfn_eq(cd_mfn, INVALID_MFN) )
> +{
> +struct page_info *pg = alloc_domheap_page(cd_vcpu->domain, 0);
> +
> +if ( !pg )
> +return -ENOMEM;
> +
> +cd_mfn = page_to_mfn(pg);
> +set_gpfn_from_mfn(mfn_x(cd_mfn), gfn_x(gfn));
> +
> +ret = p2m->set_entry(p2m, gfn, cd_mfn, PAGE_ORDER_4K, p2m_ram_rw,
> + p2m->default_access, -1);
> +if ( ret )
> +return ret;
> +}
> +else if ( p2mt != p2m_ram_rw )
> +return -EBUSY;

Shouldn't the populate of the underlying gfn in the fork case
be done by map_guest_area itself?

What if a forked guest attempts to register a new runstate/time info
against an address not yet populated?

> +/*
> + * Map the into the guest. For simplicity specify the entire range up
> + * to the end of the page: All the function uses it for is to check
> + * that the range doesn't cross page boundaries. Having the area 
> mapped
> + * in the original domain implies that it fits there and therefore 
> will
> + * also fit in the clone.
> + */
> +offset = PAGE_OFFSET(d_area->map);
> +ret = map_guest_area(cd_vcpu, gfn_to_gaddr(gfn) + offset,
> + PAGE_SIZE - offset, cd_area, NULL);
> +if ( ret )
> +return ret;
> +}
> +else
> +cd_mfn = page_to_mfn(cd_area->pg);
> +
> +copy_domain_page(cd_mfn, d_mfn);
> +
> +return 0;
> +}
> +
>  static int copy_vpmu(struct vcpu *d_vcpu, struct vcpu *cd_vcpu)
>  {
>  struct vpmu_struct *d_vpmu = vcpu_vpmu(d_vcpu);
> @@ -1733,6 +1795,16 @@ static int copy_vcpu_settings(struct dom
>  copy_domain_page(new_vcpu_info_mfn, vcpu_info_mfn);
>  }
>  
> +/* Same for the (physically registered) runstate and time info 
> areas. */
> +ret = copy_guest_area(_vcpu->runstate_guest_area,
> +  _vcpu->runstate_guest_area, cd_vcpu, d);
> +if ( ret )
> +return ret;
> +ret = copy_guest_area(_vcpu->arch.time_guest_area,
> +  _vcpu->arch.time_guest_area, cd_vcpu, d);
> +if ( ret )
> +return ret;
> +
>  ret = copy_vpmu(d_vcpu, cd_vcpu);
>  if ( ret )
>  return ret;
> @@ -1974,7 +2046,10 @@ int mem_sharing_fork_reset(struct domain
>  
>   state:
>  if ( reset_state )
> +{
>  rc = copy_settings(d, pd);
> +/* TBD: What to do here with -ERESTART? */
> +}
>  
>  domain_unpause(d);
>  
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1572,6 +1572,13 @@ void unmap_vcpu_info(struct vcpu *v)
>  put_page_and_type(mfn_to_page(mfn));
>  }
>  
> +int map_guest_area(struct vcpu *v, paddr_t gaddr, unsigned int size,
> +   struct guest_area *area,
> +   void (*populate)(void *dst, struct vcpu *v))

Oh, the prototype for this is added in patch 1, almost missed it.

Why not also add this dummy implementation in patch 1 then?

Thanks, Roger.

Re: [PATCH] xen/arm: Skip memory nodes if not enabled

[Julien Grall]

Hi Michal,

On 26/09/2023 09:36, Michal Orzel wrote:

On 26/09/2023 07:33, Leo Yan wrote:



During the Linux kernel booting, an error is reported by the Xen
hypervisor:

   (XEN) arch/arm/p2m.c:2202: d0v0: Failing to acquire the MFN 0x1a02dc

The kernel attempts to use an invalid memory frame number, which can be
converted to: 0x1a02dc << PAGE_SHIFT, resulting in 0x1_a02d_c000.

The invalid memory frame falls into a reserved memory node, which is
defined in the device tree as below:

   reserved-memory {
   #address-cells = <0x02>;
   #size-cells = <0x02>;
   ranges;

   ...

   ethosn_reserved {
   compatible = "shared-dma-pool";
   reg = <0x01 0xa000 0x00 0x2000>;
   status = "disabled";
   no-map;
   };

   ...
   };

Xen excludes all reserved memory regions from the frame management
through the dt_unreserved_regions() function. On the other hand, the
reserved memory nodes are passed to the Linux kernel. However, the Linux
kernel ignores the 'ethosn_reserved' node since its status is
"disabled". This leads to the Linux kernel to allocate pages from the
reserved memory range.

As a result, when the kernel passes the data structure residing in the
frame 0x1_a02d_c000 in the Xen hypervisor, the hypervisor detects that
it misses to manage the frame and reports the error.

Essentially, this issue is caused by the Xen hypervisor which misses to
handle the status for the memory nodes (for both the normal memory nodes
and the reserved memory nodes) and transfers them to the Linux kernel.

This patch introduces a function memory_node_is_available(). If it
detects a memory node is not enabled, the hypervisor will not add the
memory region into the memory lists. In the end, this avoids to generate
the memory nodes from the memory lists sent to the kernel and the kernel
cannot use the disabled memory nodes any longer.


Interesting. So FWICS, we have 2 issues that have a common ground:
1) If the reserved memory node has a status "disabled", it implies that this 
region
is no longer reserved and can be used which is not handled today by Xen and 
leads
to the above mentioned problem.

2) If the memory node has a status "disabled" it implies that it should not be 
used
which is not the case in current Xen. This means that at the moment, Xen would 
try
to use such a memory region which is incorrect.

I think the commit msg should be more generic and focus on these two issues.
Summary:
Xen does not handle the status property of memory nodes and ends up using them.
Xen does not handle the status property of reserved memory nodes. If status is 
disabled
it means the reserved region shall no longer be treated as reserved. Xen passes 
the reserved
memory nodes untouched to dom0 fdt and creates a memory node to cover reserved 
regions.
Disabled reserved memory nodes are ignored by the guest which leaves us with 
the exposed
memory nodes. Guest can access such a region but it is excluded from the page 
management in Xen
which results in Xen being unable to obtain the corresponding MFN.



Signed-off-by: Leo Yan 
---
  xen/arch/arm/bootfdt.c | 16 
  1 file changed, 16 insertions(+)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 2673ad17a1..b46dde06a9 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -206,11 +206,27 @@ int __init device_tree_for_each_node(const void *fdt, int 
node,
  return 0;
  }

+static bool __init memory_node_is_available(const void *fdt, unsigned long 
node)

This function is not strictly related to memory node so it would be better to 
call it e.g. device_tree_node_is_available.


+1.


+{
+const char *status = fdt_getprop(fdt, node, "status", NULL);
+
+if (!status)

white spaces between brackets and condition
if ( !status )


+return true;
+
+if (!strcmp(status, "ok") || !strcmp(status, "okay"))

white spaces between brackets and condition
if ( !strcmp(status, "ok") || !strcmp(status, "okay") )


+return true;
+
+return false;
+}
+
  static int __init process_memory_node(const void *fdt, int node,
const char *name, int depth,
u32 address_cells, u32 size_cells,
void *data)
  {
+if (!memory_node_is_available(fdt, node))
+return 0;

I would move this check to device_tree_get_meminfo()


I am ok with that. But the commit message would need to gain a paragraph 
explaining that we will now support "status" for static memory/heap.



+
  return device_tree_get_meminfo(fdt, node, "reg", address_cells, 
size_cells,
 data, MEMBANK_DEFAULT);
  }
--
2.39.2




Also, I think it would be nice to add ASSERT(bootinfo.mem.nr_banks); e.g. in 
boot_fdt_info().
Otherwise the panic from Xen when there is no memory bank:
The frametable 

Re: [PATCH v3 1/8] domain: GADDR based shared guest area registration alternative - teardown

[Roger Pau Monné]

On Wed, Sep 27, 2023 at 12:46:07PM +0200, Jan Beulich wrote:
> On 27.09.2023 12:42, Roger Pau Monné wrote:
> > On Wed, Sep 27, 2023 at 11:55:19AM +0200, Jan Beulich wrote:
> >> On 27.09.2023 10:51, Roger Pau Monné wrote:
> >>> On Wed, May 03, 2023 at 05:54:47PM +0200, Jan Beulich wrote:
>  +{
>  +struct domain *d = v->domain;
>  +
>  +if ( v != current )
>  +ASSERT(atomic_read(>pause_count) | 
>  atomic_read(>pause_count));
> >>>
> >>> Isn't this racy?
> >>
> >> It is, yes.
> >>
> >>>  What guarantees that the vcpu won't be kicked just
> >>> after the check has been performed?
> >>
> >> Nothing. This check isn't any better than assertions towards an ordinary
> >> spinlock being held. I assume you realize that we've got a number of such
> >> assertions elsewhere already.
> > 
> > Right, but different from spinlock assertions, the code here could be
> > made safe just by pausing the vCPU?
> 
> That's what the assertion is checking (see also the comment ahead of the
> function). It's just that the assertions cannot be made more strict, at
> least from all I can tell.

But the assertion might no longer be true by the time the code
afterwards is executed.  Why not wrap the code in a pair of
vcpu_{,un}pause() calls?

Thanks, Roger.

Re: [PATCH] xen/arm: Skip memory nodes if not enabled

[Julien Grall]

Hi Leo,

Adding some comments on top of what already said.

On 26/09/2023 06:33, Leo Yan wrote:

+static bool __init memory_node_is_available(const void *fdt, unsigned long 
node)
+{
+const char *status = fdt_getprop(fdt, node, "status", NULL);
+
+if (!status)
+return true;
+


We have a similar implement based on the unflatten DT (see 
dt_device_is_available()). While trying to consolidate them is probably 
not worth it, I think the behavior should match.


In this case, you want to check the len is greater than 0 before doing 
the comparison.


Cheers,

--
Julien Grall

Re: [PATCH v3 1/8] domain: GADDR based shared guest area registration alternative - teardown

[Jan Beulich]

On 27.09.2023 12:42, Roger Pau Monné wrote:
> On Wed, Sep 27, 2023 at 11:55:19AM +0200, Jan Beulich wrote:
>> On 27.09.2023 10:51, Roger Pau Monné wrote:
>>> On Wed, May 03, 2023 at 05:54:47PM +0200, Jan Beulich wrote:
>>> I guess we should also zap the runstate handlers, or else we might
>>> corrupt guest state.
>>
>> So you think the guest might re-register a different area post resume?
>> I can certainly add another patch to zap the handles; I don't think it
>> should be done right here, not the least because if we see room for
>> such behavior, that change may even want backporting.
> 
> For correctness it would be good to zap them, but there's no rush, as
> I do think guests will set to the same address on resume.

Well, I've already added a new patch coming ahead of this one.

 @@ -1568,6 +1572,19 @@ void unmap_vcpu_info(struct vcpu *v)
  put_page_and_type(mfn_to_page(mfn));
  }
  
 +/*
 + * This is only intended to be used for domain cleanup (or more generally 
 only
 + * with at least the respective vCPU, if it's not the current one, 
 reliably
 + * paused).
 + */
 +void unmap_guest_area(struct vcpu *v, struct guest_area *area)
>>>
>>> vcpu param could be const given the current code, but I assume this
>>> will be changed by future patches.  Same could be said about
>>> guest_area but I'm confident that will change.
>>
>> True for both.
>>
 +{
 +struct domain *d = v->domain;
 +
 +if ( v != current )
 +ASSERT(atomic_read(>pause_count) | 
 atomic_read(>pause_count));
>>>
>>> Isn't this racy?
>>
>> It is, yes.
>>
>>>  What guarantees that the vcpu won't be kicked just
>>> after the check has been performed?
>>
>> Nothing. This check isn't any better than assertions towards an ordinary
>> spinlock being held. I assume you realize that we've got a number of such
>> assertions elsewhere already.
> 
> Right, but different from spinlock assertions, the code here could be
> made safe just by pausing the vCPU?

That's what the assertion is checking (see also the comment ahead of the
function). It's just that the assertions cannot be made more strict, at
least from all I can tell.

Jan

[PATCH v3 for-4.18?] x86: support data operand independent timing mode

[Jan Beulich]

[1] specifies a long list of instructions which are intended to exhibit
timing behavior independent of the data they operate on. On certain
hardware this independence is optional, controlled by a bit in a new
MSR. Provide a command line option to control the mode Xen and its
guests are to operate in, with a build time control over the default.
Longer term we may want to allow guests to control this.

Since Arm64 supposedly also has such a control, put command line option
and Kconfig control in common files.

[1] 
https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html

Requested-by: Demi Marie Obenour 
Signed-off-by: Jan Beulich 
Release-acked-by: Henry Wang 
---
This may be viewed as a new feature, and hence be too late for 4.18. It
may, however, also be viewed as security relevant, which is why I'd like
to propose to at least consider it.

Slightly RFC, in particular for whether the Kconfig option should
default to Y or N.

I would have wanted to invoke setup_doitm() from cpu_init(), but that
works only on the BSP. On APs cpu_init() runs before ucode loading.
Plus recheck_cpu_features() invoking identify_cpu() takes care of the
BSP during S3 resume.
---
v3: Extend command line doc. Add changelog entry.
v2: Introduce and use cpu_has_doitm. Add comment "borrowed" from the
XenServer patch queue patch providing similar functionality.
Re-base.

--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -29,6 +29,8 @@ The format is based on [Keep a Changelog
  - 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, support for enforcing system-wide operation in Data Operand
+   Independent Timing Mode.
 
 ### Removed
  - On x86, the "pku" command line option has been removed.  It has never
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -788,6 +788,16 @@ Specify the size of the console debug tr
 additionally a trace buffer of the specified size is allocated per cpu.
 The debug trace feature is only enabled in debugging builds of Xen.
 
+### dit (x86/Intel)
+> `= `
+
+> Default: `CONFIG_DIT_DEFAULT`
+
+Specify whether Xen and guests should operate in Data Independent Timing
+mode. Note that enabling this option cannot guarantee anything beyond what
+underlying hardware guarantees (with, where available and known to Xen,
+respective tweaks applied).
+
 ### dma_bits
 > `= `
 
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -15,6 +15,7 @@ config X86
select HAS_ALTERNATIVE
select HAS_COMPAT
select HAS_CPUFREQ
+   select HAS_DIT
select HAS_EHCI
select HAS_EX_TABLE
select HAS_FAST_MULTIPLY
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -204,6 +204,28 @@ void ctxt_switch_levelling(const struct
alternative_vcall(ctxt_switch_masking, next);
 }
 
+static void setup_doitm(void)
+{
+uint64_t msr;
+
+if ( !cpu_has_doitm )
+return;
+
+/*
+ * We don't currently enumerate DOITM to guests.  As a conseqeuence, guest
+ * kernels will believe they're safe even when they are not.
+ *
+ * For now, set it unilaterally.  This prevents otherwise-correct crypto
+ * code from becoming vulnerable to timing sidechannels.
+ */
+
+rdmsrl(MSR_UARCH_MISC_CTRL, msr);
+msr |= UARCH_CTRL_DOITM;
+if ( !opt_dit )
+msr &= ~UARCH_CTRL_DOITM;
+wrmsrl(MSR_UARCH_MISC_CTRL, msr);
+}
+
 bool opt_cpu_info;
 boolean_param("cpuinfo", opt_cpu_info);
 
@@ -589,6 +611,8 @@ void identify_cpu(struct cpuinfo_x86 *c)
 
mtrr_bp_init();
}
+
+   setup_doitm();
 }
 
 /* leaf 0xb SMT level */
--- a/xen/arch/x86/include/asm/cpufeature.h
+++ b/xen/arch/x86/include/asm/cpufeature.h
@@ -201,6 +201,7 @@ static inline bool boot_cpu_has(unsigned
 #define cpu_has_if_pschange_mc_no boot_cpu_has(X86_FEATURE_IF_PSCHANGE_MC_NO)
 #define cpu_has_tsx_ctrlboot_cpu_has(X86_FEATURE_TSX_CTRL)
 #define cpu_has_taa_no  boot_cpu_has(X86_FEATURE_TAA_NO)
+#define cpu_has_doitm   boot_cpu_has(X86_FEATURE_DOITM)
 #define cpu_has_fb_clearboot_cpu_has(X86_FEATURE_FB_CLEAR)
 #define cpu_has_rrsba   boot_cpu_has(X86_FEATURE_RRSBA)
 #define cpu_has_gds_ctrlboot_cpu_has(X86_FEATURE_GDS_CTRL)
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -41,6 +41,9 @@ config HAS_COMPAT
 config HAS_DEVICE_TREE
bool
 
+config HAS_DIT # Data Independent Timing
+   bool
+
 config HAS_EX_TABLE
bool
 
@@ -175,6 +178,18 @@ config SPECULATIVE_HARDEN_GUEST_ACCESS
 
 endmenu
 
+config DIT_DEFAULT
+   bool "Data Independent Timing default"
+   depends on HAS_DIT
+   help
+ Hardware often surfaces instructions the timing of which is dependent
+ on the data they process. 

Re: [PATCH v3 1/8] domain: GADDR based shared guest area registration alternative - teardown

[Roger Pau Monné]

On Wed, Sep 27, 2023 at 11:55:19AM +0200, Jan Beulich wrote:
> On 27.09.2023 10:51, Roger Pau Monné wrote:
> > On Wed, May 03, 2023 at 05:54:47PM +0200, Jan Beulich wrote:
> > I guess we should also zap the runstate handlers, or else we might
> > corrupt guest state.
> 
> So you think the guest might re-register a different area post resume?
> I can certainly add another patch to zap the handles; I don't think it
> should be done right here, not the least because if we see room for
> such behavior, that change may even want backporting.

For correctness it would be good to zap them, but there's no rush, as
I do think guests will set to the same address on resume.

> >> @@ -1568,6 +1572,19 @@ void unmap_vcpu_info(struct vcpu *v)
> >>  put_page_and_type(mfn_to_page(mfn));
> >>  }
> >>  
> >> +/*
> >> + * This is only intended to be used for domain cleanup (or more generally 
> >> only
> >> + * with at least the respective vCPU, if it's not the current one, 
> >> reliably
> >> + * paused).
> >> + */
> >> +void unmap_guest_area(struct vcpu *v, struct guest_area *area)
> > 
> > vcpu param could be const given the current code, but I assume this
> > will be changed by future patches.  Same could be said about
> > guest_area but I'm confident that will change.
> 
> True for both.
> 
> >> +{
> >> +struct domain *d = v->domain;
> >> +
> >> +if ( v != current )
> >> +ASSERT(atomic_read(>pause_count) | 
> >> atomic_read(>pause_count));
> > 
> > Isn't this racy?
> 
> It is, yes.
> 
> >  What guarantees that the vcpu won't be kicked just
> > after the check has been performed?
> 
> Nothing. This check isn't any better than assertions towards an ordinary
> spinlock being held. I assume you realize that we've got a number of such
> assertions elsewhere already.

Right, but different from spinlock assertions, the code here could be
made safe just by pausing the vCPU?

Thanks, Roger.

Re: [for-4.18] [PATCH v2] ARM: GICv3 ITS: flush caches for newly allocated ITT

[Julien Grall]

On 25/09/2023 23:19, Henry Wang wrote:

Hi Julien,


Hi Henry,


Yes, I was about to ask the same question but somehow forgot it. This is a quite
low risk patch and I think it is fine to have this in 4.18, so if the "Fixes” 
tag
can be added on commit, please also add:

Release-acked-by: Henry Wang 


Thanks! It is now committed.

Cheers,

--
Julien Grall

Re: [PATCH v3] xen-pciback: Consider INTx disabled when MSI/MSI-X is enabled

[Marek Marczykowski-Górecki]

On Fri, Nov 18, 2022 at 04:49:23PM +0100, Marek Marczykowski-Górecki wrote:
> Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until
> the table is filled. Then it disables INTx just before clearing MASKALL
> bit. Currently this approach is rejected by xen-pciback.
> According to the PCIe spec, device cannot use INTx when MSI/MSI-X is
> enabled (in other words: enabling MSI/MSI-X implicitly disables INTx).
> 
> Change the logic to consider INTx disabled if MSI/MSI-X is enabled. This
> applies to three places:
>  - checking currently enabled interrupts type,
>  - transition to MSI/MSI-X - where INTx would be implicitly disabled,
>  - clearing INTx disable bit - which can be allowed even if MSI/MSI-X is
>enabled, as device should consider INTx disabled anyway in that case
> 
> Fixes: 5e29500eba2a ("xen-pciback: Allow setting PCI_MSIX_FLAGS_MASKALL too")
> Signed-off-by: Marek Marczykowski-Górecki 

Ping?

The issue pointed out by Jason was fixed in Xen:
https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=913751d7af6e78d65c1e2adf4887193c827f0c5e

> ---
> Changes in v3:
>  - allow clearing INTx regardless of MSI/MSI-X state, to be consistent
>with enabling MSI/MSI-X
> Changes in v2:
>  - restructure the patch to consider not only MASKALL bit, but enabling
>MSI/MSI-X generally, without explicitly disabling INTx first
> ---
>  drivers/xen/xen-pciback/conf_space.c  | 19 +++--
>  .../xen/xen-pciback/conf_space_capability.c   |  3 ++-
>  drivers/xen/xen-pciback/conf_space_header.c   | 21 +++
>  3 files changed, 18 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/xen/xen-pciback/conf_space.c 
> b/drivers/xen/xen-pciback/conf_space.c
> index 059de92aea7d..d47eee6c5143 100644
> --- a/drivers/xen/xen-pciback/conf_space.c
> +++ b/drivers/xen/xen-pciback/conf_space.c
> @@ -288,12 +288,6 @@ int xen_pcibk_get_interrupt_type(struct pci_dev *dev)
>   u16 val;
>   int ret = 0;
>  
> - err = pci_read_config_word(dev, PCI_COMMAND, );
> - if (err)
> - return err;
> - if (!(val & PCI_COMMAND_INTX_DISABLE))
> - ret |= INTERRUPT_TYPE_INTX;
> -
>   /*
>* Do not trust dev->msi(x)_enabled here, as enabling could be done
>* bypassing the pci_*msi* functions, by the qemu.
> @@ -316,6 +310,19 @@ int xen_pcibk_get_interrupt_type(struct pci_dev *dev)
>   if (val & PCI_MSIX_FLAGS_ENABLE)
>   ret |= INTERRUPT_TYPE_MSIX;
>   }
> +
> + /*
> +  * PCIe spec says device cannot use INTx if MSI/MSI-X is enabled,
> +  * so check for INTx only when both are disabled.
> +  */
> + if (!ret) {
> + err = pci_read_config_word(dev, PCI_COMMAND, );
> + if (err)
> + return err;
> + if (!(val & PCI_COMMAND_INTX_DISABLE))
> + ret |= INTERRUPT_TYPE_INTX;
> + }
> +
>   return ret ?: INTERRUPT_TYPE_NONE;
>  }
>  
> diff --git a/drivers/xen/xen-pciback/conf_space_capability.c 
> b/drivers/xen/xen-pciback/conf_space_capability.c
> index 097316a74126..eb4c1af44f5c 100644
> --- a/drivers/xen/xen-pciback/conf_space_capability.c
> +++ b/drivers/xen/xen-pciback/conf_space_capability.c
> @@ -236,10 +236,11 @@ static int msi_msix_flags_write(struct pci_dev *dev, 
> int offset, u16 new_value,
>   return PCIBIOS_SET_FAILED;
>  
>   if (new_value & field_config->enable_bit) {
> - /* don't allow enabling together with other interrupt types */
> + /* don't allow enabling together with other interrupt type */
>   int int_type = xen_pcibk_get_interrupt_type(dev);
>  
>   if (int_type == INTERRUPT_TYPE_NONE ||
> + int_type == INTERRUPT_TYPE_INTX ||
>   int_type == field_config->int_type)
>   goto write;
>   return PCIBIOS_SET_FAILED;
> diff --git a/drivers/xen/xen-pciback/conf_space_header.c 
> b/drivers/xen/xen-pciback/conf_space_header.c
> index 981435103af1..fc0332645966 100644
> --- a/drivers/xen/xen-pciback/conf_space_header.c
> +++ b/drivers/xen/xen-pciback/conf_space_header.c
> @@ -104,24 +104,9 @@ static int command_write(struct pci_dev *dev, int 
> offset, u16 value, void *data)
>   pci_clear_mwi(dev);
>   }
>  
> - if (dev_data && dev_data->allow_interrupt_control) {
> - if ((cmd->val ^ value) & PCI_COMMAND_INTX_DISABLE) {
> - if (value & PCI_COMMAND_INTX_DISABLE) {
> - pci_intx(dev, 0);
> - } else {
> - /* Do not allow enabling INTx together with MSI 
> or MSI-X. */
> - switch (xen_pcibk_get_interrupt_type(dev)) {
> - case INTERRUPT_TYPE_NONE:
> - pci_intx(dev, 1);
> - break;
> - case 

Re: [PATCH v3 2/8] domain: update GADDR based runstate guest area

[Jan Beulich]

On 27.09.2023 11:44, Roger Pau Monné wrote:
> On Wed, May 03, 2023 at 05:55:11PM +0200, Jan Beulich wrote:
>> Before adding a new vCPU operation to register the runstate area by
>> guest-physical address, add code to actually keep such areas up-to-date.
>>
>> Note that updating of the area will be done exclusively following the
>> model enabled by VMASST_TYPE_runstate_update_flag for virtual-address
>> based registered areas.
>>
>> Note further that pages aren't marked dirty when written to (matching
>> the handling of space mapped by map_vcpu_info()), on the basis that the
>> registrations are lost anyway across migration (or would need re-
>> populating at the target for transparent migration). Plus the contents
>> of the areas in question have to be deemed volatile in the first place
>> (so saving a "most recent" value is pretty meaningless even for e.g.
>> snapshotting).
>>
>> Signed-off-by: Jan Beulich 
> 
> Reviewed-by: Roger Pau Monné 

Thanks.

>> ---
>> RFC: HVM guests (on x86) can change bitness and hence layout (and size!
>>  and alignment) of the runstate area. I don't think it is an option
>>  to require 32-bit code to pass a range such that even the 64-bit
>>  layout wouldn't cross a page boundary (and be suitably aligned). I
>>  also don't see any other good solution, so for now a crude approach
>>  with an extra boolean is used (using has_32bit_shinfo() isn't race
>>  free and could hence lead to overrunning the mapped space).
> 
> Shouldn't a well behaved guest attempt to unmap the runstate area
> before changing bitness?  I would expect this to happen for example
> when OVMF relinquishes control to the OS.

Well, the OVMF example falls in a different class: Before transferring
ownership of the system, it wants to unmap regardless of whether bitness
is going to change, or else memory corruption can occur to the following
entity.

Jan

Re: [PATCH v3 3/8] x86: update GADDR based secondary time area

[Roger Pau Monné]

On Wed, May 03, 2023 at 05:55:52PM +0200, Jan Beulich wrote:
> Before adding a new vCPU operation to register the secondary time area
> by guest-physical address, add code to actually keep such areas up-to-
> date.
> 
> Note that pages aren't marked dirty when written to (matching the
> handling of space mapped by map_vcpu_info()), on the basis that the
> registrations are lost anyway across migration (or would need re-
> populating at the target for transparent migration). Plus the contents
> of the areas in question have to be deemed volatile in the first place
> (so saving a "most recent" value is pretty meaningless even for e.g.
> snapshotting).
> 
> Signed-off-by: Jan Beulich 

Reviewed-by: Roger Pau Monné 

Thanks, Roger.

changing Dom0 data during smc call inside of xen during cache coloring

[Oleg Nikitenko]

Hello,

It is necessary to change some structure contents from xen.
I have access to the registers.
One of them keeps the physical address of the structure.
But this physical address is valid for Dom0 only.
Dom0 kernel gets it by the call dma_alloc_coherent
A lower mentioned scheme does not work.

uint64_t paramaddr = (uint64_t)get_user_reg(regs, 2);
uint64_t phyaddr = mfn_to_maddr(gfn_to_mfn(current->domain,
gaddr_to_gfn(paramaddr)));
phyaddr += (paramaddr & ~PAGE_MASK);

Regards,
Oleg Nikitenko

Re: [XEN PATCH v2 0/3] docs/misra: add documentation skeleton to address MISRA C:2012 Dir 4.1

[Henry Wang]

Hi Nicola,

> On Sep 27, 2023, at 17:52, Nicola Vetrini  wrote:
> 
> The headline of Directive 4.1 states: "Run-time failures shall be minimized".
> Thus, it requires the project to supply documentation that pertains the 
> measures
> and techinques used to prevent run-time failures from happening. For ease of
> reading, the documentation is in RST format, but since ECLAIR needs a source 
> file
> to check that the needed subsections and their format is the one expected, the
> Makefiles for the docs/ are amended to generate such a file.
> 
> The format and categories of the subsections in the .rst file can be
> customized based on feedback from the community: the one provided is just a
> basic skeleton that should be tailored to the project.
> 
> CC-ing also Henry Wang, as these are just documentation and CI changes

Indeed, so technically it is safe to include this series to 4.18, if this 
series can
be properly reviewed, I am ok to add my release-ack tag for each patch:

Release-acked-by: Henry Wang 

Kind regards,
Henry


> 
> Nicola Vetrini (3):
>  docs/misra: add documentation skeleton for MISRA C:2012 Dir 4.1
>  docs: make the docs for MISRA C:2012 Dir 4.1 visible to ECLAIR
>  automation/eclair: build docs/misra to address MISRA C:2012 Dir 4.1
> 
> automation/eclair_analysis/build.sh   |   6 +-
> automation/eclair_analysis/prepare.sh |   4 +-
> docs/Makefile |   7 +-
> docs/misra/C-runtime-failures.rst | 200 ++
> docs/misra/Makefile   |  22 +++
> docs/misra/rules.rst  |   8 +-
> 6 files changed, 240 insertions(+), 7 deletions(-)
> create mode 100644 docs/misra/C-runtime-failures.rst
> create mode 100644 docs/misra/Makefile
> 
> --
> 2.34.1

Re: [PATCH v3 1/8] domain: GADDR based shared guest area registration alternative - teardown

[Jan Beulich]

On 27.09.2023 10:51, Roger Pau Monné wrote:
> On Wed, May 03, 2023 at 05:54:47PM +0200, Jan Beulich wrote:
>> In preparation of the introduction of new vCPU operations allowing to
>> register the respective areas (one of the two is x86-specific) by
>> guest-physical address, add the necessary domain cleanup hooks.
>>
>> Signed-off-by: Jan Beulich 
>> Reviewed-by: Julien Grall 
>> ---
>> RFC: Zapping the areas in pv_shim_shutdown() may not be strictly
>>  necessary: Aiui unmap_vcpu_info() is called only because the vCPU
>>  info area cannot be re-registered. Beyond that I guess the
>>  assumption is that the areas would only be re-registered as they
>>  were before. If that's not the case I wonder whether the guest
>>  handles for both areas shouldn't also be zapped.
> 
> IIRC the reason was to unset v->vcpu_info_mfn so that it could be set
> again on resume from suspension, or else the hypercall will return an
> error.

Right, that's the re-registration aspect mentioned.

> I guess we should also zap the runstate handlers, or else we might
> corrupt guest state.

So you think the guest might re-register a different area post resume?
I can certainly add another patch to zap the handles; I don't think it
should be done right here, not the least because if we see room for
such behavior, that change may even want backporting.

>> @@ -1568,6 +1572,19 @@ void unmap_vcpu_info(struct vcpu *v)
>>  put_page_and_type(mfn_to_page(mfn));
>>  }
>>  
>> +/*
>> + * This is only intended to be used for domain cleanup (or more generally 
>> only
>> + * with at least the respective vCPU, if it's not the current one, reliably
>> + * paused).
>> + */
>> +void unmap_guest_area(struct vcpu *v, struct guest_area *area)
> 
> vcpu param could be const given the current code, but I assume this
> will be changed by future patches.  Same could be said about
> guest_area but I'm confident that will change.

True for both.

>> +{
>> +struct domain *d = v->domain;
>> +
>> +if ( v != current )
>> +ASSERT(atomic_read(>pause_count) | atomic_read(>pause_count));
> 
> Isn't this racy?

It is, yes.

>  What guarantees that the vcpu won't be kicked just
> after the check has been performed?

Nothing. This check isn't any better than assertions towards an ordinary
spinlock being held. I assume you realize that we've got a number of such
assertions elsewhere already.

Jan

[XEN PATCH v2 2/3] docs: make the docs for MISRA C:2012 Dir 4.1 visible to ECLAIR

[Nicola Vetrini]

To be able to check for the existence of the necessary subsections in
the documentation for MISRA C:2012 Dir 4.1, ECLAIR needs to have a source
file that is built.

This file is generated from 'C-runtime-failures.rst' in docs/misra
and the configuration is updated accordingly.

Signed-off-by: Nicola Vetrini 
---
Changes from RFC:
- Dropped unused/useless code
- Revised the sed command
- Revised the clean target

Changes in v2:
- Added explanative comment to the makefile
- printf instead of echo
---
 docs/Makefile   |  7 ++-
 docs/misra/Makefile | 22 ++
 2 files changed, 28 insertions(+), 1 deletion(-)
 create mode 100644 docs/misra/Makefile

diff --git a/docs/Makefile b/docs/Makefile
index 966a104490ac..ff991a0c3ca2 100644
--- a/docs/Makefile
+++ b/docs/Makefile
@@ -43,7 +43,7 @@ DOC_PDF  := $(patsubst %.pandoc,pdf/%.pdf,$(PANDOCSRC-y)) \
 all: build
 
 .PHONY: build
-build: html txt pdf man-pages figs
+build: html txt pdf man-pages figs misra
 
 .PHONY: sphinx-html
 sphinx-html:
@@ -66,9 +66,14 @@ endif
 .PHONY: pdf
 pdf: $(DOC_PDF)
 
+.PHONY: misra
+misra:
+   $(MAKE) -C misra
+
 .PHONY: clean
 clean: clean-man-pages
$(MAKE) -C figs clean
+   $(MAKE) -C misra clean
rm -rf .word_count *.aux *.dvi *.bbl *.blg *.glo *.idx *~
rm -rf *.ilg *.log *.ind *.toc *.bak *.tmp core
rm -rf html txt pdf sphinx/html
diff --git a/docs/misra/Makefile b/docs/misra/Makefile
new file mode 100644
index ..8fd89404e96b
--- /dev/null
+++ b/docs/misra/Makefile
@@ -0,0 +1,22 @@
+TARGETS := C-runtime-failures.o
+
+all: $(TARGETS)
+
+# This Makefile will generate the object files indicated in TARGETS by taking
+# the corresponding .rst file, converting its content to a C block comment and
+# then compiling the resulting .c file. This is needed for the file's content 
to
+# be available when performing static analysis with ECLAIR on the project.
+
+# sed is used in place of cat to prevent occurrences of '*/'
+# in the .rst from breaking the compilation
+$(TARGETS:.o=.c): %.c: %.rst
+   printf "/*\n\n" > $@.tmp
+   sed -e 's|\*/|*//*|g' $< >> $@.tmp
+   printf "\n\n*/" >> $@.tmp
+   mv $@.tmp $@
+
+%.o: %.c
+   $(CC) -c $< -o $@
+
+clean:
+   rm -f C-runtime-failures.c *.o *.tmp
-- 
2.34.1

[XEN PATCH v2 1/3] docs/misra: add documentation skeleton for MISRA C:2012 Dir 4.1

[Nicola Vetrini]

The aforementioned directive requires the project to supply documentation
on the measures taken towards the minimization of run-time failures.

The actual content of the documentation still needs feedback from the
community.

The 'rules.rst' file is updated accordingly to mention the newly
added documentation.

Signed-off-by: Nicola Vetrini 
---
Changes in v2:
- Incorporated suggestions from Stefano.
---
 docs/misra/C-runtime-failures.rst | 200 ++
 docs/misra/rules.rst  |   8 +-
 2 files changed, 207 insertions(+), 1 deletion(-)
 create mode 100644 docs/misra/C-runtime-failures.rst

diff --git a/docs/misra/C-runtime-failures.rst 
b/docs/misra/C-runtime-failures.rst
new file mode 100644
index ..325d3fab1fa5
--- /dev/null
+++ b/docs/misra/C-runtime-failures.rst
@@ -0,0 +1,200 @@
+===
+Measures taken towards the minimization of Run-time failures in Xen
+===
+
+This document specifies which procedures and techinques are used troughout the
+Xen codebase to prevent or minimize the impact of certain classes of run-time
+errors that can occurr in the execution of a C program, due to the very minimal
+built-in checks that are present in the language.
+
+The presence of such documentation is requested by MISRA C:2012 Directive 4.1,
+whose headline states: "Run-time failures shall be minimized".
+
+
+Documentation for MISRA C:2012 Dir 4.1: overflow
+
+
+Pervasive use of assertions and extensive test suite.
+
+
+Documentation for MISRA C:2012 Dir 4.1: unexpected wrapping
+___
+
+The only wrapping the is present in the code concerns
+unsigned integers and they are all expected.
+
+
+Documentation for MISRA C:2012 Dir 4.1: invalid shift
+_
+
+Pervasive use of assertions and extensive test suite.
+
+
+Documentation for MISRA C:2012 Dir 4.1: division/remainder by zero
+__
+
+The division or remainder operations in the project code ensure that
+their second argument is never zero.
+
+
+Documentation for MISRA C:2012 Dir 4.1: unsequenced side effects
+
+
+Code executed in interrupt handlers uses spinlocks or disables interrupts
+at the right locations to avoid unsequenced side effects.
+
+
+Documentation for MISRA C:2012 Dir 4.1: read from uninitialized automatic 
object
+
+
+The amount of dynamically allocated objects is limited at runtime in
+static configurations. We make sure to initialize dynamically allocated
+objects before reading them, and we utilize static analysis tools to
+help check for that.
+
+
+Documentation for MISRA C:2012 Dir 4.1: read from uninitialized allocated 
object
+
+
+Dynamically allocated storage is used in a controlled manner, to prevent the
+access to uninitialized allocated storage.
+
+
+Documentation for MISRA C:2012 Dir 4.1: write to string literal or const object
+___
+
+The toolchain puts every string literal and const object into a read-only
+section of memory.  The hardware exception raised when a write is attempted
+on such a memory section is correctly handled.
+
+
+Documentation for MISRA C:2012 Dir 4.1: non-volatile access to volatile object
+__
+
+Volatile access is limited to registers that are always accessed
+through macros or inline functions, or by limited code chunks that are only 
used
+to access a register.
+
+
+Documentation for MISRA C:2012 Dir 4.1: access to dead allocated object
+___
+
+Although dynamically allocated storage is used in the project, in safety
+configurations its usage is very limited at runtime (it is "almost" only used
+at boot time). Coverity is regularly used to scan the code to detect non-freed
+allocated objects.
+
+
+Documentation for MISRA C:2012 Dir 4.1: access to dead automatic object
+___
+
+Pointers to automatic variables are never returned, nor stored in
+wider-scoped objects.  No function does the same on any pointer
+received as a parameter.
+
+
+Documentation for MISRA C:2012 Dir 4.1: access to dead thread object
+
+
+The program does not use per-thread variables.
+
+
+Documentation for MISRA C:2012 Dir 4.1: access using null pointer

[XEN PATCH v2 0/3] docs/misra: add documentation skeleton to address MISRA C:2012 Dir 4.1

[Nicola Vetrini]

The headline of Directive 4.1 states: "Run-time failures shall be minimized".
Thus, it requires the project to supply documentation that pertains the measures
and techinques used to prevent run-time failures from happening. For ease of
reading, the documentation is in RST format, but since ECLAIR needs a source 
file
to check that the needed subsections and their format is the one expected, the
Makefiles for the docs/ are amended to generate such a file.

The format and categories of the subsections in the .rst file can be
customized based on feedback from the community: the one provided is just a
basic skeleton that should be tailored to the project.

CC-ing also Henry Wang, as these are just documentation and CI changes

Nicola Vetrini (3):
  docs/misra: add documentation skeleton for MISRA C:2012 Dir 4.1
  docs: make the docs for MISRA C:2012 Dir 4.1 visible to ECLAIR
  automation/eclair: build docs/misra to address MISRA C:2012 Dir 4.1

 automation/eclair_analysis/build.sh   |   6 +-
 automation/eclair_analysis/prepare.sh |   4 +-
 docs/Makefile |   7 +-
 docs/misra/C-runtime-failures.rst | 200 ++
 docs/misra/Makefile   |  22 +++
 docs/misra/rules.rst  |   8 +-
 6 files changed, 240 insertions(+), 7 deletions(-)
 create mode 100644 docs/misra/C-runtime-failures.rst
 create mode 100644 docs/misra/Makefile

--
2.34.1

[XEN PATCH v2 3/3] automation/eclair: build docs/misra to address MISRA C:2012 Dir 4.1

[Nicola Vetrini]

The documentation pertaining Directive 4.1 is contained in docs/misra.
The build script driving the analysis is amended to allow ECLAIR to
analyze such file.

Signed-off-by: Nicola Vetrini 
---
Changes in v2:
- removed useless make flags
---
 automation/eclair_analysis/build.sh   | 6 +++---
 automation/eclair_analysis/prepare.sh | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/automation/eclair_analysis/build.sh 
b/automation/eclair_analysis/build.sh
index ec087dd822fa..ea7a1e5a59b0 100755
--- a/automation/eclair_analysis/build.sh
+++ b/automation/eclair_analysis/build.sh
@@ -34,11 +34,11 @@ else
 fi
 
 (
-  cd xen
-
+  make -C docs misra
   make "-j${PROCESSORS}" "-l${PROCESSORS}.0"\
"CROSS_COMPILE=${CROSS_COMPILE}" \
"CC=${CROSS_COMPILE}gcc-12"  \
"CXX=${CROSS_COMPILE}g++-12" \
-   "XEN_TARGET_ARCH=${XEN_TARGET_ARCH}"
+   "XEN_TARGET_ARCH=${XEN_TARGET_ARCH}" \
+   -C xen
 )
diff --git a/automation/eclair_analysis/prepare.sh 
b/automation/eclair_analysis/prepare.sh
index 0cac5eba00ae..ebd5a2dde676 100755
--- a/automation/eclair_analysis/prepare.sh
+++ b/automation/eclair_analysis/prepare.sh
@@ -35,8 +35,8 @@ else
 fi
 
 (
-cd xen
-cp "${CONFIG_FILE}" .config
+./configure
+cp "${CONFIG_FILE}" xen/.config
 make clean
 find . -type f -name "*.safparse" -print -delete
 make -f ${script_dir}/Makefile.prepare prepare
-- 
2.34.1

Re: [PATCH v3 2/8] domain: update GADDR based runstate guest area

[Roger Pau Monné]

On Wed, May 03, 2023 at 05:55:11PM +0200, Jan Beulich wrote:
> Before adding a new vCPU operation to register the runstate area by
> guest-physical address, add code to actually keep such areas up-to-date.
> 
> Note that updating of the area will be done exclusively following the
> model enabled by VMASST_TYPE_runstate_update_flag for virtual-address
> based registered areas.
> 
> Note further that pages aren't marked dirty when written to (matching
> the handling of space mapped by map_vcpu_info()), on the basis that the
> registrations are lost anyway across migration (or would need re-
> populating at the target for transparent migration). Plus the contents
> of the areas in question have to be deemed volatile in the first place
> (so saving a "most recent" value is pretty meaningless even for e.g.
> snapshotting).
> 
> Signed-off-by: Jan Beulich 

Reviewed-by: Roger Pau Monné 

> ---
> RFC: HVM guests (on x86) can change bitness and hence layout (and size!
>  and alignment) of the runstate area. I don't think it is an option
>  to require 32-bit code to pass a range such that even the 64-bit
>  layout wouldn't cross a page boundary (and be suitably aligned). I
>  also don't see any other good solution, so for now a crude approach
>  with an extra boolean is used (using has_32bit_shinfo() isn't race
>  free and could hence lead to overrunning the mapped space).

Shouldn't a well behaved guest attempt to unmap the runstate area
before changing bitness?  I would expect this to happen for example
when OVMF relinquishes control to the OS.

Thanks, Roger.

Re: [PATCH] net/xen-netback: Break build if netback slots > max_skbs + 1

[Paul Durrant]

On 27/09/2023 09:29, David Kahurani wrote:

If XEN_NETBK_LEGACY_SLOTS_MAX and MAX_SKB_FRAGS have a difference of
more than 1, with MAX_SKB_FRAGS being the lesser value, it opens up a
path for null-dereference. It was also noted that some distributions
were modifying upstream behaviour in that direction which necessitates
this patch.

Signed-off-by: David Kahurani 


Acked-by: Paul Durrant 

[ovmf test] 183190: all pass - PUSHED

[osstest service owner]

flight 183190 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/183190/

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf ad1c0394b1770315099e511de7c88a04d7af76f2
baseline version:
 ovmf be971fc302cbef8f47e2612eda114135f21205e6

Last test of basis   183186  2023-09-27 00:12:32 Z0 days
Testing same since   183190  2023-09-27 05:23:10 Z0 days1 attempts


People who touched revisions under test:
  Nate DeSimone 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 pass
 test-amd64-i386-xl-qemuu-ovmf-amd64  pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/osstest/ovmf.git
   be971fc302..ad1c0394b1  ad1c0394b1770315099e511de7c88a04d7af76f2 -> 
xen-tested-master

[PATCH 07/29] xen/blkback: Convert to bdev_open_by_dev()

[Jan Kara]

Convert xen/blkback to use bdev_open_by_dev() and pass the
handle around.

CC: xen-devel@lists.xenproject.org
Acked-by: Christoph Hellwig 
Acked-by: Christian Brauner 
Signed-off-by: Jan Kara 
---
 drivers/block/xen-blkback/blkback.c |  4 +--
 drivers/block/xen-blkback/common.h  |  4 +--
 drivers/block/xen-blkback/xenbus.c  | 40 +++--
 3 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c 
b/drivers/block/xen-blkback/blkback.c
index c362f4ad80ab..4defd7f387c7 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -465,7 +465,7 @@ static int xen_vbd_translate(struct phys_req *req, struct 
xen_blkif *blkif,
}
 
req->dev  = vbd->pdevice;
-   req->bdev = vbd->bdev;
+   req->bdev = vbd->bdev_handle->bdev;
rc = 0;
 
  out:
@@ -969,7 +969,7 @@ static int dispatch_discard_io(struct xen_blkif_ring *ring,
int err = 0;
int status = BLKIF_RSP_OKAY;
struct xen_blkif *blkif = ring->blkif;
-   struct block_device *bdev = blkif->vbd.bdev;
+   struct block_device *bdev = blkif->vbd.bdev_handle->bdev;
struct phys_req preq;
 
xen_blkif_get(blkif);
diff --git a/drivers/block/xen-blkback/common.h 
b/drivers/block/xen-blkback/common.h
index 40f67bfc052d..5ff50e76cee5 100644
--- a/drivers/block/xen-blkback/common.h
+++ b/drivers/block/xen-blkback/common.h
@@ -221,7 +221,7 @@ struct xen_vbd {
unsigned char   type;
/* phys device that this vbd maps to. */
u32 pdevice;
-   struct block_device *bdev;
+   struct bdev_handle  *bdev_handle;
/* Cached size parameter. */
sector_tsize;
unsigned intflush_support:1;
@@ -360,7 +360,7 @@ struct pending_req {
 };
 
 
-#define vbd_sz(_v) bdev_nr_sectors((_v)->bdev)
+#define vbd_sz(_v) bdev_nr_sectors((_v)->bdev_handle->bdev)
 
 #define xen_blkif_get(_b) (atomic_inc(&(_b)->refcnt))
 #define xen_blkif_put(_b)  \
diff --git a/drivers/block/xen-blkback/xenbus.c 
b/drivers/block/xen-blkback/xenbus.c
index bb66178c432b..e34219ea2b05 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -81,7 +81,7 @@ static void xen_update_blkif_status(struct xen_blkif *blkif)
int i;
 
/* Not ready to connect? */
-   if (!blkif->rings || !blkif->rings[0].irq || !blkif->vbd.bdev)
+   if (!blkif->rings || !blkif->rings[0].irq || !blkif->vbd.bdev_handle)
return;
 
/* Already connected? */
@@ -99,12 +99,13 @@ static void xen_update_blkif_status(struct xen_blkif *blkif)
return;
}
 
-   err = sync_blockdev(blkif->vbd.bdev);
+   err = sync_blockdev(blkif->vbd.bdev_handle->bdev);
if (err) {
xenbus_dev_error(blkif->be->dev, err, "block flush");
return;
}
-   invalidate_inode_pages2(blkif->vbd.bdev->bd_inode->i_mapping);
+   invalidate_inode_pages2(
+   blkif->vbd.bdev_handle->bdev->bd_inode->i_mapping);
 
for (i = 0; i < blkif->nr_rings; i++) {
ring = >rings[i];
@@ -472,9 +473,9 @@ static void xenvbd_sysfs_delif(struct xenbus_device *dev)
 
 static void xen_vbd_free(struct xen_vbd *vbd)
 {
-   if (vbd->bdev)
-   blkdev_put(vbd->bdev, NULL);
-   vbd->bdev = NULL;
+   if (vbd->bdev_handle)
+   bdev_release(vbd->bdev_handle);
+   vbd->bdev_handle = NULL;
 }
 
 static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle,
@@ -482,7 +483,7 @@ static int xen_vbd_create(struct xen_blkif *blkif, 
blkif_vdev_t handle,
  int cdrom)
 {
struct xen_vbd *vbd;
-   struct block_device *bdev;
+   struct bdev_handle *bdev_handle;
 
vbd = >vbd;
vbd->handle   = handle;
@@ -491,17 +492,17 @@ static int xen_vbd_create(struct xen_blkif *blkif, 
blkif_vdev_t handle,
 
vbd->pdevice  = MKDEV(major, minor);
 
-   bdev = blkdev_get_by_dev(vbd->pdevice, vbd->readonly ?
+   bdev_handle = bdev_open_by_dev(vbd->pdevice, vbd->readonly ?
 BLK_OPEN_READ : BLK_OPEN_WRITE, NULL, NULL);
 
-   if (IS_ERR(bdev)) {
+   if (IS_ERR(bdev_handle)) {
pr_warn("xen_vbd_create: device %08x could not be opened\n",
vbd->pdevice);
return -ENOENT;
}
 
-   vbd->bdev = bdev;
-   if (vbd->bdev->bd_disk == NULL) {
+   vbd->bdev_handle = bdev_handle;
+   if (vbd->bdev_handle->bdev->bd_disk == NULL) {
pr_warn("xen_vbd_create: device %08x doesn't exist\n",
vbd->pdevice);
xen_vbd_free(vbd);
@@ -509,14 +510,14 @@ static int xen_vbd_create(struct xen_blkif *blkif, 
blkif_vdev_t handle,
}
vbd->size = vbd_sz(vbd);
 
-   if 

Re: [PATCH v3 1/8] domain: GADDR based shared guest area registration alternative - teardown

[Roger Pau Monné]

On Wed, May 03, 2023 at 05:54:47PM +0200, Jan Beulich wrote:
> In preparation of the introduction of new vCPU operations allowing to
> register the respective areas (one of the two is x86-specific) by
> guest-physical address, add the necessary domain cleanup hooks.
> 
> Signed-off-by: Jan Beulich 
> Reviewed-by: Julien Grall 
> ---
> RFC: Zapping the areas in pv_shim_shutdown() may not be strictly
>  necessary: Aiui unmap_vcpu_info() is called only because the vCPU
>  info area cannot be re-registered. Beyond that I guess the
>  assumption is that the areas would only be re-registered as they
>  were before. If that's not the case I wonder whether the guest
>  handles for both areas shouldn't also be zapped.

IIRC the reason was to unset v->vcpu_info_mfn so that it could be set
again on resume from suspension, or else the hypercall will return an
error.

I guess we should also zap the runstate handlers, or else we might
corrupt guest state.

> ---
> v2: Add assertion in unmap_guest_area().
> 
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1019,7 +1019,10 @@ int arch_domain_soft_reset(struct domain
>  }
>  
>  for_each_vcpu ( d, v )
> +{
>  set_xen_guest_handle(v->arch.time_info_guest, NULL);
> +unmap_guest_area(v, >arch.time_guest_area);
> +}
>  
>   exit_put_gfn:
>  put_gfn(d, gfn_x(gfn));
> @@ -2356,6 +2359,8 @@ int domain_relinquish_resources(struct d
>  if ( ret )
>  return ret;
>  
> +unmap_guest_area(v, >arch.time_guest_area);
> +
>  vpmu_destroy(v);
>  }
>  
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -669,6 +669,7 @@ struct arch_vcpu
>  
>  /* A secondary copy of the vcpu time info. */
>  XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
> +struct guest_area time_guest_area;
>  
>  struct arch_vm_event *vm_event;
>  
> --- a/xen/arch/x86/pv/shim.c
> +++ b/xen/arch/x86/pv/shim.c
> @@ -382,8 +382,10 @@ int pv_shim_shutdown(uint8_t reason)
>  
>  for_each_vcpu ( d, v )
>  {
> -/* Unmap guest vcpu_info pages. */
> +/* Unmap guest vcpu_info page and runstate/time areas. */
>  unmap_vcpu_info(v);
> +unmap_guest_area(v, >runstate_guest_area);
> +unmap_guest_area(v, >arch.time_guest_area);
>  
>  /* Reset the periodic timer to the default value. */
>  vcpu_set_periodic_timer(v, MILLISECS(10));
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -963,7 +963,10 @@ int domain_kill(struct domain *d)
>  if ( cpupool_move_domain(d, cpupool0) )
>  return -ERESTART;
>  for_each_vcpu ( d, v )
> +{
>  unmap_vcpu_info(v);
> +unmap_guest_area(v, >runstate_guest_area);
> +}
>  d->is_dying = DOMDYING_dead;
>  /* Mem event cleanup has to go here because the rings 
>   * have to be put before we call put_domain. */
> @@ -1417,6 +1420,7 @@ int domain_soft_reset(struct domain *d,
>  {
>  set_xen_guest_handle(runstate_guest(v), NULL);
>  unmap_vcpu_info(v);
> +unmap_guest_area(v, >runstate_guest_area);
>  }
>  
>  rc = arch_domain_soft_reset(d);
> @@ -1568,6 +1572,19 @@ void unmap_vcpu_info(struct vcpu *v)
>  put_page_and_type(mfn_to_page(mfn));
>  }
>  
> +/*
> + * This is only intended to be used for domain cleanup (or more generally 
> only
> + * with at least the respective vCPU, if it's not the current one, reliably
> + * paused).
> + */
> +void unmap_guest_area(struct vcpu *v, struct guest_area *area)

vcpu param could be const given the current code, but I assume this
will be changed by future patches.  Same could be said about
guest_area but I'm confident that will change.

> +{
> +struct domain *d = v->domain;
> +
> +if ( v != current )
> +ASSERT(atomic_read(>pause_count) | atomic_read(>pause_count));

Isn't this racy?  What guarantees that the vcpu won't be kicked just
after the check has been performed?

Thanks, Roger.

[linux-5.4 test] 183183: regressions - FAIL

[osstest service owner]

flight 183183 linux-5.4 real [real]
flight 183191 linux-5.4 real-retest [real]
http://logs.test-lab.xenproject.org/osstest/logs/183183/
http://logs.test-lab.xenproject.org/osstest/logs/183191/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-armhf-armhf-xl-credit1  19 guest-start.2  fail in 183144 REGR. vs. 182613

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-xl-credit2  14 guest-start  fail in 183144 pass in 183183
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 12 debian-hvm-install fail in 
183144 pass in 183183
 test-armhf-armhf-libvirt-qcow2 17 guest-start/debian.repeat fail in 183144 
pass in 183183
 test-armhf-armhf-xl-credit1  18 guest-start/debian.repeat  fail pass in 183144

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-xl-multivcpu 18 guest-start/debian.repeat fail blocked in 
182613
 test-armhf-armhf-xl-rtds 18 guest-start/debian.repeat fail in 183144 like 
182613
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 182613
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 182613
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 182613
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 182613
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 182613
 test-armhf-armhf-xl-credit2  18 guest-start/debian.repeatfail  like 182613
 test-armhf-armhf-xl-arndale  18 guest-start/debian.repeatfail  like 182613
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 182613
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 182613
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 182613
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 182613
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 182613
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 182613
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 182613
 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 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-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-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  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-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-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-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  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-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  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-armhf-armhf-libvirt 15 migrate-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-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-amd64-i386-libvirt-raw  14 migrate-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 

Re: [XEN PATCH] x86/mem_access: address violations of MISRA C:2012 Rule 8.3

[Henry Wang]

Hi Federico,

> On Sep 27, 2023, at 16:41, Federico Serafini  
> wrote:
> 
> Make function declarations and definitions consistent.
> No functional change.
> 
> Signed-off-by: Federico Serafini 

This looks like a harmless change so with proper review:

Release-acked-by: Henry Wang 

Kind regards,
Henry

[XEN PATCH] x86/mem_access: address violations of MISRA C:2012 Rule 8.3

[Federico Serafini]

Make function declarations and definitions consistent.
No functional change.

Signed-off-by: Federico Serafini 
---
 xen/arch/x86/include/asm/mem_access.h | 2 +-
 xen/arch/x86/mm/mem_access.c  | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/include/asm/mem_access.h 
b/xen/arch/x86/include/asm/mem_access.h
index 8957e1181c..1a52a10322 100644
--- a/xen/arch/x86/include/asm/mem_access.h
+++ b/xen/arch/x86/include/asm/mem_access.h
@@ -39,7 +39,7 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool 
suppress_ve,
 
 struct xen_hvm_altp2m_suppress_ve_multi;
 int p2m_set_suppress_ve_multi(struct domain *d,
-  struct xen_hvm_altp2m_suppress_ve_multi 
*suppress_ve);
+  struct xen_hvm_altp2m_suppress_ve_multi *sve);
 
 int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
 unsigned int altp2m_idx);
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index c472fa1ee5..3449e0ee85 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -70,7 +70,7 @@ static int _p2m_get_mem_access(struct p2m_domain *p2m, gfn_t 
gfn,
 }
 
 bool p2m_mem_access_emulate_check(struct vcpu *v,
-  const vm_event_response_t *rsp)
+  const struct vm_event_st *rsp)
 {
 xenmem_access_t access;
 bool violation = true;
@@ -129,7 +129,7 @@ bool p2m_mem_access_emulate_check(struct vcpu *v,
 
 bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
   struct npfec npfec,
-  vm_event_request_t **req_ptr)
+  struct vm_event_st **req_ptr)
 {
 struct vcpu *v = current;
 gfn_t gfn = gaddr_to_gfn(gpa);
-- 
2.34.1

Re: [PATCH v3] docs/misra: add rule 2.1 exceptions

[Bertrand Marquis]

Hi Jan,

> On 27 Sep 2023, at 10:23, Jan Beulich  wrote:
> 
> On 27.09.2023 10:14, Bertrand Marquis wrote:
>>> On 27 Sep 2023, at 09:53, Nicola Vetrini  wrote:
>>> My opinion is that it's far easier for this to be an eclair configuration 
>>> (which has the
>>> advantage not to depend on the exact definition of unreachable) and then 
>>> perhaps a comment
>>> above it explaining the situation.
>> 
>> I agree here and it is easier to make an overall exception where we list the 
>> cases
>> where this is acceptable (ie all flavors of unreacheable) and document that 
>> eclair
>> was configured using "" to handle this.
> 
> What about cppcheck then, for example?

Good point we should check if cppcheck or coverity can do such things.
@Luca: any idea ?

Cheers
Bertrand

> 
> Jan
> 

[PATCH] net/xen-netback: Break build if netback slots > max_skbs + 1

[David Kahurani]

If XEN_NETBK_LEGACY_SLOTS_MAX and MAX_SKB_FRAGS have a difference of
more than 1, with MAX_SKB_FRAGS being the lesser value, it opens up a
path for null-dereference. It was also noted that some distributions
were modifying upstream behaviour in that direction which necessitates
this patch.

Signed-off-by: David Kahurani 
---
 drivers/net/xen-netback/netback.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/xen-netback/netback.c 
b/drivers/net/xen-netback/netback.c
index 88f760a7cbc3..df032e33787f 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1005,6 +1005,7 @@ static void xenvif_tx_build_gops(struct xenvif_queue 
*queue,
break;
}
 
+   BUILD_BUG_ON(XEN_NETBK_LEGACY_SLOTS_MAX > MAX_SKB_FRAGS + 1);
if (ret >= XEN_NETBK_LEGACY_SLOTS_MAX - 1 && data_len < 
txreq.size)
data_len = txreq.size;
 
-- 
2.25.1

Re: [PATCH v3] docs/misra: add rule 2.1 exceptions

[Jan Beulich]

On 27.09.2023 10:14, Bertrand Marquis wrote:
>> On 27 Sep 2023, at 09:53, Nicola Vetrini  wrote:
>> My opinion is that it's far easier for this to be an eclair configuration 
>> (which has the
>> advantage not to depend on the exact definition of unreachable) and then 
>> perhaps a comment
>> above it explaining the situation.
> 
> I agree here and it is easier to make an overall exception where we list the 
> cases
> where this is acceptable (ie all flavors of unreacheable) and document that 
> eclair
> was configured using "" to handle this.

What about cppcheck then, for example?

Jan

Re: [PATCH v2] x86/shutdown: change default reboot method preference

[Jan Beulich]

On 15.09.2023 09:43, Roger Pau Monne wrote:
> The current logic to chose the preferred reboot method is based on the mode 
> Xen
> has been booted into, so if the box is booted from UEFI, the preferred reboot
> method will be to use the ResetSystem() run time service call.
> 
> However, that method seems to be widely untested, and quite often leads to a
> result similar to:
> 
> Hardware Dom0 shutdown: rebooting machine
> [ Xen-4.18-unstable  x86_64  debug=y  Tainted:   C]
> CPU:0
> RIP:e008:[<0017>] 0017
> RFLAGS: 00010202   CONTEXT: hypervisor
> [...]
> Xen call trace:
>[<0017>] R 0017
>[] S 83207eff7b50
>[] F machine_restart+0x1da/0x261
>[] F apic_wait_icr_idle+0/0x37
>[] F smp_call_function_interrupt+0xc7/0xcb
>[] F call_function_interrupt+0x20/0x34
>[] F do_IRQ+0x150/0x6f3
>[] F common_interrupt+0x132/0x140
>[] F 
> arch/x86/acpi/cpu_idle.c#acpi_idle_do_entry+0x113/0x129
>[] F 
> arch/x86/acpi/cpu_idle.c#acpi_processor_idle+0x3eb/0x5f7
>[] F arch/x86/domain.c#idle_loop+0xec/0xee
> 
> 
> Panic on CPU 0:
> FATAL TRAP: vector = 6 (invalid opcode)
> 
> 
> Which in most cases does lead to a reboot, however that's unreliable.
> 
> Change the default reboot preference to prefer ACPI over UEFI if available and
> not in reduced hardware mode.
> 
> This is in line to what Linux does, so it's unlikely to cause issues on 
> current
> and future hardware, since there's a much higher chance of vendors testing
> hardware with Linux rather than Xen.
> 
> Add a special case for one Acer model that does require being rebooted using
> ResetSystem().  See Linux commit 0082517fa4bce for rationale.
> 
> I'm not aware of using ACPI reboot causing issues on boxes that do have
> properly implemented ResetSystem() methods.

A data point from a new system I'm still in the process of setting up: The
ACPI reboot method, as used by Linux, unconditionally means a warm reboot.
The EFI method, otoh, properly distinguishes "reboot=warm" from our default
of explicitly requesting cold reboot. (Without taking the EFI path, I
assume our write to the relevant BDA location simply has no effect, for
this being a legacy BIOS thing, and the system apparently defaults to warm
reboot when using the ACPI method.)

Clearly, as a secondary effect, this system adds to my personal experience
of so far EFI reboot consistently working on all x86 hardware I have (had)
direct access to. (That said, this is the first non-Intel system, which
likely biases my overall experience.)

Jan

Re: [PATCH] docs/misra: add rule 11.9

[Bertrand Marquis]

Hi Stefano,

> On 27 Sep 2023, at 03:05, Stefano Stabellini  wrote:
> 
> From: Stefano Stabellini 
> 
> Following up from the MISRA C group discussion, add Rule 11.9 to
> docs/misra/rules.rst.
> 
> Signed-off-by: Stefano Stabellini 

I agree with Jan on dropping the "integer" word here.

With that done:
Acked-by: Bertrand Marquis 

Cheers
Bertrand

> ---
> Rule 13.1 also discussed but it is already in docs/misra/rules.rst
> ---
> docs/misra/rules.rst | 5 +
> 1 file changed, 5 insertions(+)
> 
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> index 8e7d17d242..28dc3a4d66 100644
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -373,6 +373,11 @@ maintainers if you want to suggest a change.
>  - A cast shall not remove any const or volatile qualification from the 
> type pointed to by a pointer
>  -
> 
> +   * - `Rule 11.9 
> `_
> + - Required
> + - The macro NULL shall be the only permitted form of integer null 
> pointer constant
> + -
> +
>* - `Rule 12.5 
> `_
>  - Mandatory
>  - The sizeof operator shall not have an operand which is a function
> -- 
> 2.25.1
> 

Re: [PATCH v3] docs/misra: add rule 2.1 exceptions

[Bertrand Marquis]

Hi,

> On 27 Sep 2023, at 09:53, Nicola Vetrini  wrote:
> 
>>> > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>>> > index 695d2fa1f1..2a8527cacc 100644
>>> > --- a/xen/arch/arm/psci.c
>>> > +++ b/xen/arch/arm/psci.c
>>> > @@ -59,6 +59,7 @@ void call_psci_cpu_off(void)
>>> >   }
>>> >   }
>>> >   +/* SAF-2-safe */
>>> I think any use of SAF-2-safe should be accompanied with an attribute...
>>> >   void call_psci_system_off(void)
>>> ... noreturn for function or ...
>>> >   {
>>> >   if ( psci_ver > PSCI_VERSION(0, 1) )
>>> > diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c
>>> > index 7619544d14..47e0f59024 100644
>>> > --- a/xen/arch/x86/shutdown.c
>>> > +++ b/xen/arch/x86/shutdown.c
>>> > @@ -118,6 +118,7 @@ static inline void kb_wait(void)
>>> >   break;
>>> >   }
>>> >   +/* SAF-2-safe */
>>> >   static void noreturn cf_check __machine_halt(void *unused)
>>> >   {
>>> >   local_irq_disable();
>>> > diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
>>> > index e8a4eea71a..d47c54f034 100644
>>> > --- a/xen/include/xen/bug.h
>>> > +++ b/xen/include/xen/bug.h
>>> > @@ -117,6 +117,7 @@ struct bug_frame {
>>> >   #endif
>>> > #ifndef BUG
>>> > +/* SAF-2-safe */
>>> >   #define BUG() do {  \
>>> >   BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, NULL);  \
>>> >   unreachable();  \
>>> ... unreachable for macros. But the /* SAF-2-safe */ feels a little bit
>>> redundant when a function is marked as 'noreturn'.
>>> Is there any way to teach eclair about noreturn?
>> Actually I had the same thought while writing this patch. If we can
>> adopt unreachable and noreturn consistently maybe we don't need
>> SAF-2-safe. If the checker can support it.
>> Nicola, what do you think?
> 
> A couple of remarks:
> - if you put the noreturn attribute on some functions, then surely the code 
> after those is
> reported as unreachable. ECLAIR should pick up all forms of noreturn 
> automatically; otherwise, a simple configuration can be used.
> 
> - Note that the cause of unreachability in the vast majority of cases is the 
> call to
> __builtin_unreachable(), therefore a textual deviation on the definition of 
> unreachable, plus
> a bit of ECLAIR configuration, can deviate it (to be clear, just the SAF 
> comment is not
> sufficient, since deviations comments are meant to be applied at the top 
> expansion location,
> which is not on the macro definition).
> This is what it should look like, roughly:
> 
> -config=MC3R1.R2.1,reports+={deliberate, "any_area(any_loc(text(^$, 
> -1)))"}
> 
> #if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__ < 5))
> /* SAF-2-safe */
> #define unreachable() do {} while (1)
> #else
> /* SAF-2-safe */
> #define unreachable() __builtin_unreachable()
> #endif
> 
> where REGEX will match the translation of SAF-2-safe.
> 
> However, this will then entail that *some* SAF comments are treated specially 
> and, moreover,
> that some modification to the definition of unreachable won't work
> (e.g.
> #define M() __builtin_unreachable()
> /* SAF-2-safe */
> #define unreachable() M()
> 
> My opinion is that it's far easier for this to be an eclair configuration 
> (which has the
> advantage not to depend on the exact definition of unreachable) and then 
> perhaps a comment
> above it explaining the situation.

I agree here and it is easier to make an overall exception where we list the 
cases
where this is acceptable (ie all flavors of unreacheable) and document that 
eclair
was configured using "" to handle this.

Cheers
Bertrand

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

Re: [PATCH v3] docs/misra: add 14.3

[Bertrand Marquis]

Hi Stefano,

> On 8 Sep 2023, at 22:27, Stefano Stabellini  wrote:
> 
> From: Stefano Stabellini 
> 
> Add 14.3, with project-wide deviations.
> 
> Also take the opportunity to clarify that parameters of function pointer
> types are expected to have names (Rule 8.2).
> 
> Signed-off-by: Stefano Stabellini 

I am still not quite sure if we should accept a rule if we are deviating
on so much cases but as we can revisit anyway:

Acked-by: Bertrand Marquis 

Cheers
Bertrand

> ---
> Changes in v3:
> - add ,
> - add switch(sizeof(...)) and switch(offsetof(...))
> ---
> docs/misra/rules.rst | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> index 34916e266a..ac76e20a9c 100644
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -234,7 +234,8 @@ maintainers if you want to suggest a change.
>* - `Rule 8.2 
> `_
>  - Required
>  - Function types shall be in prototype form with named parameters
> - -
> + - Clarification: both function and function pointers types shall
> +   have named parameters.
> 
>* - `Rule 8.3 
> `_
>  - Required
> @@ -385,6 +386,18 @@ maintainers if you want to suggest a change.
>  - A loop counter shall not have essentially floating type
>  -
> 
> +   * - `Rule 14.3 
> `_
> + - Required
> + - Controlling expressions shall not be invariant
> + - Due to the extensive usage of IS_ENABLED, sizeof compile-time
> +   checks, and other constructs that are detected as errors by MISRA
> +   C scanners, managing the configuration of a MISRA C scanner for
> +   this rule would be unmanageable. Thus, this rule is adopted with
> +   a project-wide deviation on if, ?:, switch(sizeof(...)), and
> +   switch(offsetof(...)) statements.
> +
> +   while(0) and while(1) and alike are allowed.
> +
>* - `Rule 16.7 
> `_
>  - Required
>  - A switch-expression shall not have essentially Boolean type
> -- 
> 2.25.1
> 

Re: [PATCH v3] docs/misra: add rule 2.1 exceptions

[Nicola Vetrini]

> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index 695d2fa1f1..2a8527cacc 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -59,6 +59,7 @@ void call_psci_cpu_off(void)
>   }
>   }
>   +/* SAF-2-safe */

I think any use of SAF-2-safe should be accompanied with an 
attribute...


>   void call_psci_system_off(void)

... noreturn for function or ...

>   {
>   if ( psci_ver > PSCI_VERSION(0, 1) )
> diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c
> index 7619544d14..47e0f59024 100644
> --- a/xen/arch/x86/shutdown.c
> +++ b/xen/arch/x86/shutdown.c
> @@ -118,6 +118,7 @@ static inline void kb_wait(void)
>   break;
>   }
>   +/* SAF-2-safe */
>   static void noreturn cf_check __machine_halt(void *unused)
>   {
>   local_irq_disable();
> diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
> index e8a4eea71a..d47c54f034 100644
> --- a/xen/include/xen/bug.h
> +++ b/xen/include/xen/bug.h
> @@ -117,6 +117,7 @@ struct bug_frame {
>   #endif
> #ifndef BUG
> +/* SAF-2-safe */
>   #define BUG() do {  \
>   BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, NULL);  \
>   unreachable();  \

... unreachable for macros. But the /* SAF-2-safe */ feels a little 
bit

redundant when a function is marked as 'noreturn'.

Is there any way to teach eclair about noreturn?


Actually I had the same thought while writing this patch. If we can
adopt unreachable and noreturn consistently maybe we don't need
SAF-2-safe. If the checker can support it.

Nicola, what do you think?


A couple of remarks:
- if you put the noreturn attribute on some functions, then surely the 
code after those is
reported as unreachable. ECLAIR should pick up all forms of noreturn 
automatically; otherwise, a simple configuration can be used.


- Note that the cause of unreachability in the vast majority of cases is 
the call to
__builtin_unreachable(), therefore a textual deviation on the definition 
of unreachable, plus
a bit of ECLAIR configuration, can deviate it (to be clear, just the SAF 
comment is not
sufficient, since deviations comments are meant to be applied at the top 
expansion location,

which is not on the macro definition).
This is what it should look like, roughly:

-config=MC3R1.R2.1,reports+={deliberate, 
"any_area(any_loc(text(^$, -1)))"}


#if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__ < 5))
/* SAF-2-safe */
#define unreachable() do {} while (1)
#else
/* SAF-2-safe */
#define unreachable() __builtin_unreachable()
#endif

where REGEX will match the translation of SAF-2-safe.

However, this will then entail that *some* SAF comments are treated 
specially and, moreover,

that some modification to the definition of unreachable won't work
(e.g.
#define M() __builtin_unreachable()
/* SAF-2-safe */
#define unreachable() M()

My opinion is that it's far easier for this to be an eclair 
configuration (which has the
advantage not to depend on the exact definition of unreachable) and then 
perhaps a comment

above it explaining the situation.

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

Re: [PATCH v3] docs/misra: add 14.3

[Jan Beulich]

On 08.09.2023 22:27, Stefano Stabellini wrote:
> From: Stefano Stabellini 
> 
> Add 14.3, with project-wide deviations.
> 
> Also take the opportunity to clarify that parameters of function pointer
> types are expected to have names (Rule 8.2).
> 
> Signed-off-by: Stefano Stabellini 

I'm not overly happy with ...

> @@ -385,6 +386,18 @@ maintainers if you want to suggest a change.
>   - A loop counter shall not have essentially floating type
>   -
>  
> +   * - `Rule 14.3 
> `_
> + - Required
> + - Controlling expressions shall not be invariant
> + - Due to the extensive usage of IS_ENABLED, sizeof compile-time
> +   checks, and other constructs that are detected as errors by MISRA
> +   C scanners, managing the configuration of a MISRA C scanner for
> +   this rule would be unmanageable. Thus, this rule is adopted with
> +   a project-wide deviation on if, ?:, switch(sizeof(...)), and
> +   switch(offsetof(...)) statements.
> +
> +   while(0) and while(1) and alike are allowed.

... the final result, but seeing that you didn't get any ack in almost
3 weeks:
Acked-by: Jan Beulich 

Jan

Re: [PATCH v3 2/2] xen/common: Add NUMA node id bounds check to page_alloc.c/node_to_scrub

[Jan Beulich]

On 27.09.2023 08:32, Jan Beulich wrote:
> On 27.09.2023 00:37, Shawn Anastasio wrote:
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1211,6 +1211,14 @@ static unsigned int node_to_scrub(bool get_node)
>>  } while ( !cpumask_empty(_to_cpumask(node)) &&
>>(node != local_node) );
>>
>> +/*
>> + * In practice `node` will always be within MAX_NUMNODES, but GCC 
>> can't
>> + * always see that, so an explicit check is necessary to avoid 
>> tripping
>> + * its out-of-bounds array access warning (-Warray-bounds).
>> + */
>> +if ( node >= MAX_NUMNODES )
>> +break;
>> +
>>  if ( node == local_node )
>>  break;
> 
> My comment on v1 wasn't addressed, either verbally or by a code change.

I have to apologize, you did respond, and I didn't spot the response earlier
on. I'm not happy about the added code, but at least it has a comment now.
Hence I guess I simply withdraw my objection, so the change can go in.

Jan

> Imo
> that would move us a tiny step closer to what Andrew was asking for as well.
> 
> Jan
> 

Re: [PATCH] docs/misra: add rule 11.9

[Jan Beulich]

On 27.09.2023 03:05, Stefano Stabellini wrote:
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -373,6 +373,11 @@ maintainers if you want to suggest a change.
>   - A cast shall not remove any const or volatile qualification from the 
> type pointed to by a pointer
>   -
>  
> +   * - `Rule 11.9 
> `_
> + - Required
> + - The macro NULL shall be the only permitted form of integer null 
> pointer constant
> + -

I think there's a terminology issue, which I'd prefer if we didn't inherit
from the Misra spec: NULL is not an integer constant expression. A null
pointer constant can be either plain 0 or ((void*)0); the former is an
integer constant expression, while the latter is not. Aiui it is the
"Amplification" text in the doc which correlates with the use of "integer"
in the title; since (as per above) I think the text is wrong in this
regard, the use of the word here also is at best misleading. The C99 spec
also doesn't know the term "integer null pointer constant"; only "null
pointer constant" is specified and used.

With the word dropped despite then not being in sync with the doc:
Acked-by: Jan Beulich 

Jan

Re: [PATCH v2] SUPPORT: downgrade Physical CPU Hotplug to Experimental

[Jan Beulich]

On 27.09.2023 02:29, Stefano Stabellini wrote:
> From: Stefano Stabellini 
> 
> The feature is not commonly used, and we don't have hardware to test it,
> not in OSSTest, not in Gitlab, and not even ad-hoc manually by community
> members. We could use QEMU to test it, but even that it is known not to
> work.
> 
> Also take the opportunity to rename the feature to "ACPI CPU Hotplug"
> for clarity.
> 
> Signed-off-by: Stefano Stabellini 
> ---
> Changes in v2:
> - rename the feature following Roger's suggestion
> ---
>  SUPPORT.md | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

As indicated before, imo this needs accompanying by a CHANGELOG.md entry
(and I don't think that should be a separate commit).

Jan

Re: [PATCH v3 2/2] xen/common: Add NUMA node id bounds check to page_alloc.c/node_to_scrub

[Jan Beulich]

On 27.09.2023 00:37, Shawn Anastasio wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1211,6 +1211,14 @@ static unsigned int node_to_scrub(bool get_node)
>  } while ( !cpumask_empty(_to_cpumask(node)) &&
>(node != local_node) );
> 
> +/*
> + * In practice `node` will always be within MAX_NUMNODES, but GCC 
> can't
> + * always see that, so an explicit check is necessary to avoid 
> tripping
> + * its out-of-bounds array access warning (-Warray-bounds).
> + */
> +if ( node >= MAX_NUMNODES )
> +break;
> +
>  if ( node == local_node )
>  break;

My comment on v1 wasn't addressed, either verbally or by a code change. Imo
that would move us a tiny step closer to what Andrew was asking for as well.

Jan

Re: [PATCH 1/3] automation: Drop ppc64le-*randconfig jobs

[Jan Beulich]

On 26.09.2023 21:49, Andrew Cooper wrote:
> On 25/09/2023 11:42 pm, Shawn Anastasio wrote:
>> Since ppc64le is still undergoing early bringup, disable the randconfig
>> CI build which was causing spurious CI failures.
>>
>> Signed-off-by: Shawn Anastasio 
>> Reported-by: Jan Beulich 
>> ---
>>  automation/gitlab-ci/build.yaml | 18 --
>>  1 file changed, 18 deletions(-)
>>
>> diff --git a/automation/gitlab-ci/build.yaml 
>> b/automation/gitlab-ci/build.yaml
>> index 1619e9a558..32af30cced 100644
>> --- a/automation/gitlab-ci/build.yaml
>> +++ b/automation/gitlab-ci/build.yaml
>> @@ -563,24 +563,6 @@ debian-bullseye-gcc-ppc64le-debug:
>>  KBUILD_DEFCONFIG: ppc64_defconfig
>>  HYPERVISOR_ONLY: y
>>  
>> -debian-bullseye-gcc-ppc64le-randconfig:
>> -  extends: .gcc-ppc64le-cross-build
>> -  variables:
>> -CONTAINER: debian:bullseye-ppc64le
>> -KBUILD_DEFCONFIG: ppc64_defconfig
>> -RANDCONFIG: y
>> -EXTRA_FIXED_RANDCONFIG:
>> -  CONFIG_COVERAGE=n
>> -
>> -debian-bullseye-gcc-ppc64le-debug-randconfig:
>> -  extends: .gcc-ppc64le-cross-build-debug
>> -  variables:
>> -CONTAINER: debian:bullseye-ppc64le
>> -KBUILD_DEFCONFIG: ppc64_defconfig
>> -RANDCONFIG: y
>> -EXTRA_FIXED_RANDCONFIG:
>> -  CONFIG_COVERAGE=n
> 
> I know this has been committed, but it shouldn't have been.  Randconfig
> is important to have even at this point in the bringup.

Well. As so often you only comment when it's too late. The question whether
randconfig is sensible to have in the bringup phase was raised earlier. You
could have said "yes, keep it" there. With there not having been any comment
on the suggestion to drop this (and I similarly think for RISC-V, which is
likely to be similarly affected the moment the full build is enabled there),
I didn't expect any opposition to the patch, and hence went ahead committing
it.

> For options which are known-incompatible, append them to
> EXTRA_FIXED_RANDCONFIG, just like COVERAGE already is.

But in this early phase that merely means re-stating some/all of what the
default config states (which may in fact state a few too many fixed
settings).

> However, it was only grant tables which showed up as broken, and ought
> to be easy to address.

It may only be grant tables right now (that's what CI had spotted), but
other settings are likely to become (transient) problems as well. See
e.g.

#define smp_processor_id() 0 /* TODO: Fix this */

which imo might lead to the compiler spot bogus constructs, just because
at the same time NR_CPUS > 1.

Jan

Re: [PATCH 2/3] automation: Change build script to use arch defconfig

[Jan Beulich]

On 26.09.2023 20:35, Shawn Anastasio wrote:
> On 9/26/23 12:43 PM, Shawn Anastasio wrote:
>> On 9/26/23 2:19 AM, Jan Beulich wrote:
>>> On 26.09.2023 01:12, Stefano Stabellini wrote:
 On Mon, 25 Sep 2023, Shawn Anastasio wrote:
> Change automation build script to call the make defconfig target before
> setting CONFIG_DEBUG and extra options. This fixes issues on Power where
> the build fails without using the ppc64_defconfig.
>
> Signed-off-by: Shawn Anastasio 
> Reported-by: Jan Beulich 
>>>
>>> Nit: Tags in chronological order please (also affects patch 1).
>>>
>> Will fix.
>>
 What is the problem specifically? Is the issue that CONFIG_DEBUG enabled
 before make olddefconfig causes certain DEBUG options also to default to
 yes, and these additional options don't work well on Power?
>>>
>>> No, the issue is that "make olddefconfig" takes the existing .config
>>> without even considering the arch's default configuration that was
>>> specified (KBUILD_DEFCONFIG).
>>>
> --- a/automation/scripts/build
> +++ b/automation/scripts/build
> @@ -22,7 +22,12 @@ if [[ "${RANDCONFIG}" == "y" ]]; then
>  # RANDCONFIG implies HYPERVISOR_ONLY
>  HYPERVISOR_ONLY="y"
>  else
> -echo "CONFIG_DEBUG=${debug}" > xen/.config
> +# Start off with arch's defconfig
> +make -C xen defconfig
> +
> +# Drop existing CONFIG_DEBUG and replace with value of ${debug}
> +sed -i 's/^CONFIG_DEBUG=[yn]//g' xen/.config
> +echo "CONFIG_DEBUG=${debug}" >> xen/.config
>
>  if [[ -n "${EXTRA_XEN_CONFIG}" ]]; then
>  echo "${EXTRA_XEN_CONFIG}" >> xen/.config
>>>
>>> It never really became clear to me whether kconfig honors the first,
>>> last, or any random setting in a .config that it takes as input, when
>>> a certain option appears there more than once. The change you make
>>> implies it's consistently "last" - can you confirm that's the actual
>>> behavior of kconfig?
>>>
>>
>> I actually tried to avoid this issue alltogether with the sed command I
>> added before the echo to drop any existing CONFIG_DEBUG= line.
>>
> 
> Just realized that options in $EXTRA_XEN_CONFIG would also be subject to
> this which is likely what you meant to point out -- my apologies.
> 
> I've tested locally and Kconfig does indeed seem to honor the last
> setting. It throws some warnings about the overridden symbol but the
> build continues with the latest value, e.g:
> 
> .config:94:warning: override: reassigning to symbol DEBUG
> .config:95:warning: override: reassigning to symbol DEBUG

I might guess such warnings are okay to appear, but this needs confirming
by one of the people more familiar with how the CI works and what output
is or is not okay to appear. I can see upsides and downsides to such
warnings ...

Jan