[xen-unstable-smoke test] 171934: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-amd64-libvirt   6 libvirt-buildfail REGR. vs. 171884

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt  1 build-check(1)   blocked  n/a
 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  9dc3f006a831cd20d531123f097e3de176ac3cae
baseline version:
 xen  f732240fd3bac25116151db5ddeb7203b62e85ce

Last test of basis   171884  2022-07-27 12:03:31 Z2 days
Failing since171899  2022-07-28 19:01:47 Z1 days9 attempts
Testing same since   171934  2022-07-30 02:00:28 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Daniel P. Smith 
  George Dunlap 
  Jan Beulich 
  Jiamei Xie 
  Julien Grall 
  Julien Grall 
  Luca Fancellu 
  Oleksandr Tyshchenko 
  Stefano Stabellini 
  Xenia Ragiadakou 

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



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


Not pushing.

(No revision log; it would be 451 lines long.)

Re: [PATCH 04/36] cpuidle,intel_idle: Fix CPUIDLE_FLAG_IRQ_ENABLE

[Paul E. McKenney]

Or better yet, try the patch that Rafael proposed.  ;-)

Thanx, Paul

On Fri, Jul 29, 2022 at 08:26:22AM -0700, Paul E. McKenney wrote:
> On Fri, Jul 29, 2022 at 03:24:58AM -0700, Michel Lespinasse wrote:
> > On Thu, Jul 28, 2022 at 10:20:53AM -0700, Paul E. McKenney wrote:
> > > On Mon, Jul 25, 2022 at 12:43:06PM -0700, Michel Lespinasse wrote:
> > > > On Wed, Jun 08, 2022 at 04:27:27PM +0200, Peter Zijlstra wrote:
> > > > > Commit c227233ad64c ("intel_idle: enable interrupts before C1 on
> > > > > Xeons") wrecked intel_idle in two ways:
> > > > > 
> > > > >  - must not have tracing in idle functions
> > > > >  - must return with IRQs disabled
> > > > > 
> > > > > Additionally, it added a branch for no good reason.
> > > > > 
> > > > > Fixes: c227233ad64c ("intel_idle: enable interrupts before C1 on 
> > > > > Xeons")
> > > > > Signed-off-by: Peter Zijlstra (Intel) 
> > > > 
> > > > After this change was introduced, I am seeing "WARNING: suspicious RCU
> > > > usage" when booting a kernel with debug options compiled in. Please
> > > > see the attached dmesg output. The issue starts with commit 32d4fd5751ea
> > > > and is still present in v5.19-rc8.
> > > > 
> > > > I'm not sure, is this too late to fix or revert in v5.19 final ?
> > > 
> > > I finally got a chance to take a quick look at this.
> > > 
> > > The rcu_eqs_exit() function is making a lockdep complaint about
> > > being invoked with interrupts enabled.  This function is called from
> > > rcu_idle_exit(), which is an expected code path from cpuidle_enter_state()
> > > via its call to rcu_idle_exit().  Except that rcu_idle_exit() disables
> > > interrupts before invoking rcu_eqs_exit().
> > > 
> > > The only other call to rcu_idle_exit() does not disable interrupts,
> > > but it is via rcu_user_exit(), which would be a very odd choice for
> > > cpuidle_enter_state().
> > > 
> > > It seems unlikely, but it might be that it is the use of local_irq_save()
> > > instead of raw_local_irq_save() within rcu_idle_exit() that is causing
> > > the trouble.  If this is the case, then the commit shown below would
> > > help.  Note that this commit removes the warning from lockdep, so it
> > > is necessary to build the kernel with CONFIG_RCU_EQS_DEBUG=y to enable
> > > equivalent debugging.
> > > 
> > > Could you please try your test with the -rce commit shown below applied?
> > 
> > Thanks for looking into it.
> 
> And thank you for trying this shot in the dark!
> 
> > After checking out Peter's commit 32d4fd5751ea,
> > cherry picking your commit ed4ae5eff4b3,
> > and setting CONFIG_RCU_EQS_DEBUG=y in addition of my usual debug config,
> > I am now seeing this a few seconds into the boot:
> > 
> > [3.010650] [ cut here ]
> > [3.010651] WARNING: CPU: 0 PID: 0 at kernel/sched/clock.c:397 
> > sched_clock_tick+0x27/0x60
> 
> And this is again a complaint about interrupts not being disabled.
> 
> But it does appear that the problem was the lockdep complaint, and
> eliminating that did take care of part of the problem.  But lockdep
> remained enabled, and you therefore hit the next complaint.
> 
> > [3.010657] Modules linked in:
> > [3.010660] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
> > 5.19.0-rc1-test-5-g1be22fea0611 #1
> > [3.010662] Hardware name: LENOVO 30BFS44D00/1036, BIOS S03KT51A 
> > 01/17/2022
> > [3.010663] RIP: 0010:sched_clock_tick+0x27/0x60
> 
> The most straightforward way to get to sched_clock_tick() from
> cpuidle_enter_state() is via the call to sched_clock_idle_wakeup_event().
> 
> Except that it disables interrupts before invoking sched_clock_tick().
> 
> > [3.010665] Code: 1f 40 00 53 eb 02 5b c3 66 90 8b 05 2f c3 40 01 85 c0 
> > 74 18 65 8b 05 60 88 8f 4e 85 c0 75 0d 65 8b 05 a9 85 8f 4e 85 c0 74 02 
> > <0f> 0b e8 e2 6c 89 00 48 c7 c3 40 d5 02 00
> >  89 c0 48 03 1c c5 c0 98
> > [3.010667] RSP: :b2803e28 EFLAGS: 00010002
> > [3.010670] RAX: 0001 RBX: c8ce7fa07060 RCX: 
> > 0001
> > [3.010671] RDX:  RSI: b268dd21 RDI: 
> > b269ab13
> > [3.010673] RBP: 0001 R08: ffc300d5 R09: 
> > 0002be80
> > [3.010674] R10: 03625b53183a R11: a012b802b7a4 R12: 
> > b2aa9e80
> > [3.010675] R13: b2aa9e00 R14: 0001 R15: 
> > 
> > [3.010677] FS:  () GS:a012b800() 
> > knlGS:
> > [3.010678] CS:  0010 DS:  ES:  CR0: 80050033
> > [3.010680] CR2: a012f81ff000 CR3: 000c99612001 CR4: 
> > 003706f0
> > [3.010681] DR0:  DR1:  DR2: 
> > 
> > [3.010682] DR3:  DR6: fffe0ff0 DR7: 
> > 0400
> > [3.010683] Call Trace:
> > [3.010685]  
> > [3.010688]  cpuidle_enter_state+0xb7/0x4b0
> > [

Re: [PATCH 04/36] cpuidle,intel_idle: Fix CPUIDLE_FLAG_IRQ_ENABLE

[Paul E. McKenney]

On Fri, Jul 29, 2022 at 03:24:58AM -0700, Michel Lespinasse wrote:
> On Thu, Jul 28, 2022 at 10:20:53AM -0700, Paul E. McKenney wrote:
> > On Mon, Jul 25, 2022 at 12:43:06PM -0700, Michel Lespinasse wrote:
> > > On Wed, Jun 08, 2022 at 04:27:27PM +0200, Peter Zijlstra wrote:
> > > > Commit c227233ad64c ("intel_idle: enable interrupts before C1 on
> > > > Xeons") wrecked intel_idle in two ways:
> > > > 
> > > >  - must not have tracing in idle functions
> > > >  - must return with IRQs disabled
> > > > 
> > > > Additionally, it added a branch for no good reason.
> > > > 
> > > > Fixes: c227233ad64c ("intel_idle: enable interrupts before C1 on Xeons")
> > > > Signed-off-by: Peter Zijlstra (Intel) 
> > > 
> > > After this change was introduced, I am seeing "WARNING: suspicious RCU
> > > usage" when booting a kernel with debug options compiled in. Please
> > > see the attached dmesg output. The issue starts with commit 32d4fd5751ea
> > > and is still present in v5.19-rc8.
> > > 
> > > I'm not sure, is this too late to fix or revert in v5.19 final ?
> > 
> > I finally got a chance to take a quick look at this.
> > 
> > The rcu_eqs_exit() function is making a lockdep complaint about
> > being invoked with interrupts enabled.  This function is called from
> > rcu_idle_exit(), which is an expected code path from cpuidle_enter_state()
> > via its call to rcu_idle_exit().  Except that rcu_idle_exit() disables
> > interrupts before invoking rcu_eqs_exit().
> > 
> > The only other call to rcu_idle_exit() does not disable interrupts,
> > but it is via rcu_user_exit(), which would be a very odd choice for
> > cpuidle_enter_state().
> > 
> > It seems unlikely, but it might be that it is the use of local_irq_save()
> > instead of raw_local_irq_save() within rcu_idle_exit() that is causing
> > the trouble.  If this is the case, then the commit shown below would
> > help.  Note that this commit removes the warning from lockdep, so it
> > is necessary to build the kernel with CONFIG_RCU_EQS_DEBUG=y to enable
> > equivalent debugging.
> > 
> > Could you please try your test with the -rce commit shown below applied?
> 
> Thanks for looking into it.

And thank you for trying this shot in the dark!

> After checking out Peter's commit 32d4fd5751ea,
> cherry picking your commit ed4ae5eff4b3,
> and setting CONFIG_RCU_EQS_DEBUG=y in addition of my usual debug config,
> I am now seeing this a few seconds into the boot:
> 
> [3.010650] [ cut here ]
> [3.010651] WARNING: CPU: 0 PID: 0 at kernel/sched/clock.c:397 
> sched_clock_tick+0x27/0x60

And this is again a complaint about interrupts not being disabled.

But it does appear that the problem was the lockdep complaint, and
eliminating that did take care of part of the problem.  But lockdep
remained enabled, and you therefore hit the next complaint.

> [3.010657] Modules linked in:
> [3.010660] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
> 5.19.0-rc1-test-5-g1be22fea0611 #1
> [3.010662] Hardware name: LENOVO 30BFS44D00/1036, BIOS S03KT51A 01/17/2022
> [3.010663] RIP: 0010:sched_clock_tick+0x27/0x60

The most straightforward way to get to sched_clock_tick() from
cpuidle_enter_state() is via the call to sched_clock_idle_wakeup_event().

Except that it disables interrupts before invoking sched_clock_tick().

> [3.010665] Code: 1f 40 00 53 eb 02 5b c3 66 90 8b 05 2f c3 40 01 85 c0 74 
> 18 65 8b 05 60 88 8f 4e 85 c0 75 0d 65 8b 05 a9 85 8f 4e 85 c0 74 02 <0f> 0b 
> e8 e2 6c 89 00 48 c7 c3 40 d5 02 00
>  89 c0 48 03 1c c5 c0 98
> [3.010667] RSP: :b2803e28 EFLAGS: 00010002
> [3.010670] RAX: 0001 RBX: c8ce7fa07060 RCX: 
> 0001
> [3.010671] RDX:  RSI: b268dd21 RDI: 
> b269ab13
> [3.010673] RBP: 0001 R08: ffc300d5 R09: 
> 0002be80
> [3.010674] R10: 03625b53183a R11: a012b802b7a4 R12: 
> b2aa9e80
> [3.010675] R13: b2aa9e00 R14: 0001 R15: 
> 
> [3.010677] FS:  () GS:a012b800() 
> knlGS:
> [3.010678] CS:  0010 DS:  ES:  CR0: 80050033
> [3.010680] CR2: a012f81ff000 CR3: 000c99612001 CR4: 
> 003706f0
> [3.010681] DR0:  DR1:  DR2: 
> 
> [3.010682] DR3:  DR6: fffe0ff0 DR7: 
> 0400
> [3.010683] Call Trace:
> [3.010685]  
> [3.010688]  cpuidle_enter_state+0xb7/0x4b0
> [3.010694]  cpuidle_enter+0x29/0x40
> [3.010697]  do_idle+0x1d4/0x210
> [3.010702]  cpu_startup_entry+0x19/0x20
> [3.010704]  rest_init+0x117/0x1a0
> [3.010708]  arch_call_rest_init+0xa/0x10
> [3.010711]  start_kernel+0x6d8/0x6ff
> [3.010716]  secondary_startup_64_no_verify+0xce/0xdb
> [3.010728]  
> [3.010729] irq event stamp: 44179
> [3.010730] hardirqs last  enabled at 

Re: [ImageBuilder][PATCH v3 1/3] uboot-script-gen: Dynamically compute addr and size when loading binaries

[Stefano Stabellini]

On Wed, 13 Jul 2022, Andrei Cherechesu (OSS) wrote:
> From: Andrei Cherechesu 
> 
> Normally, the script would precompute the sizes of the loaded binaries
> and addresses where they are loaded before generating the script,
> and the sizes and addresses that needed to be provided to Xen via
> /chosen would be hardcoded in the boot script.
> 
> Added option via "-s" parameter to use the ${filesize} variable
> in u-boot, which is set automatically after a *load command.
> The value stored by filesize is now stored in a u-boot env variable
> with the name corresponding to the binary that was loaded before.
> The newly set variables are now used in setting the /chosen node,
> instead of the hardcoded values.
> 
> Also, the loading addresses for the files are dynamically computed
> and aligned to 0x20 using the `setexpr` u-boot command. Basically,
> if the option is used, there are zero hardcoded addresses inside the
> boot script, and everything is determined based on the MEMORY_START
> parameter and each binary's size.
> 
> If the "-s" parameter is not used, the script does not store the
> binaries' sizes and addresses in variables and uses the precomputed
> ones when advertising them in the /chosen node.
> 
> Signed-off-by: Andrei Cherechesu 

This patch is difficult :-)

I like the idea but it makes the code significantly more complex due to
the additional $dynamic_loading_opt case handled everywhere. Initially I
thought about only retain the code path using u-boot variables, at least
after loading the files. However, I realize that it would break the
FDTEDIT case, which I think it would be good to keep working. Also it is
nice to be able to edit the device tree at build time putting in the
right values.

So I tried to eliminated most of the new "if" statements in another way.
The best I could come up with is the below, using eval and additional
bash variables to look up the address and size based on the variable
name (e.g. dom0_kernel). If we want the address, we use the value of
$dom0_kernel_addr, if we want the u-boot variable we use ${dom0_kernel}.

This is untested, just to show the idea. What do you think? Is it
better? Do you prefer the original patch? Other ideas or opinions? 


diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
index 18c0ce1..cee22f6 100755
--- a/scripts/uboot-script-gen
+++ b/scripts/uboot-script-gen
@@ -4,6 +4,9 @@ offset=$((2*1024*1024))
 filesize=0
 prog_req=(mkimage file fdtput mktemp awk)
 
+padding_mask=`printf "0x%X\n" $(($offset - 1))`
+padding_mask_inv=`printf "0x%X\n" $((~$padding_mask))`
+
 function cleanup_and_return_err()
 {
 rm -f $UBOOT_SOURCE $UBOOT_SCRIPT
@@ -28,6 +31,7 @@ function dt_mknode()
 #   str
 #   str_a
 #   bool
+#   var
 function dt_set()
 {
 local path=$1
@@ -35,11 +39,21 @@ function dt_set()
 local data_type=$3
 local data=$4
 
+if test $data_type = "var"
+then
+eval data_addr_var="$data"_addr
+eval data_addr=\$"$data_addr_var"
+eval data_size_var="$data"_size
+eval data_size=\$"$data_size_var"
+fi
 
 if test "$UBOOT_SOURCE" && test ! "$FIT"
 then
 var=${var/\#/\\#}
-if test $data_type = "hex" || test $data_type = "int"
+if test $data_type = "var"
+then
+echo "fdt set $path $var <0x0 0x\${"$data_addr_var"} 0x0 
0x\${"$data_size_var"}>" >> $UBOOT_SOURCE
+elif test $data_type = "hex" || test $data_type = "int"
 then
 echo "fdt set $path $var <$data>" >> $UBOOT_SOURCE
 elif test $data_type = "str_a"
@@ -63,7 +77,10 @@ function dt_set()
 
 if test $FDTEDIT
 then
-if test $data_type = "hex"
+if test $data_type = "var"
+then
+fdtput $FDTEDIT -p -t x $path $var 0x0 "$data_addr" 0x0 
"$data_size"
+elif test $data_type = "hex"
 then
 fdtput $FDTEDIT -p -t x $path $var $data
 elif test $data_type = "int"
@@ -87,38 +104,35 @@ function dt_set()
 function add_device_tree_kernel()
 {
 local path=$1
-local addr=$2
-local size=$3
-local bootargs=$4
+local varname=$2
+local bootargs=$3
 
-dt_mknode "$path" "module$addr"
-dt_set "$path/module$addr" "compatible" "str_a" "multiboot,kernel 
multiboot,module"
-dt_set "$path/module$addr" "reg" "hex"  "0x0 $addr 0x0 $(printf "0x%x" 
$size)"
-dt_set "$path/module$addr" "bootargs" "str" "$bootargs"
+dt_mknode "$path" "module-$varname"
+dt_set "$path/module-$varname" "compatible" "str_a" "multiboot,kernel 
multiboot,module"
+dt_set "$path/module-$varname" "reg" "var"  "$varname"
+dt_set "$path/module-$varname" "bootargs" "str" "$bootargs"
 }
 
 
 function add_device_tree_ramdisk()
 {
 local path=$1
-local addr=$2
-local size=$3
+local varname=$2
 
-dt_mknode "$path"  "module$addr"
-dt_set "$path/module$addr" "compatible" "str_a" "multiboot,ramdisk 
multiboot,module"
-dt_set "$path/module$addr" "reg" 

[xen-unstable-smoke test] 171932: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-amd64-libvirt   6 libvirt-buildfail REGR. vs. 171884

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt  1 build-check(1)   blocked  n/a
 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  062790aca6b1faea62c9ed2737c3791efb0d0f4c
baseline version:
 xen  f732240fd3bac25116151db5ddeb7203b62e85ce

Last test of basis   171884  2022-07-27 12:03:31 Z2 days
Failing since171899  2022-07-28 19:01:47 Z1 days8 attempts
Testing same since   171917  2022-07-29 10:03:07 Z0 days4 attempts


People who touched revisions under test:
  Andrew Cooper 
  Daniel P. Smith 
  George Dunlap 
  Jan Beulich 
  Jiamei Xie 
  Julien Grall 
  Oleksandr Tyshchenko 
  Stefano Stabellini 
  Xenia Ragiadakou 

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



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


Not pushing.


commit 062790aca6b1faea62c9ed2737c3791efb0d0f4c
Author: Xenia Ragiadakou 
Date:   Fri Jul 29 08:51:31 2022 +0200

arm/atomic: fix MISRA C 2012 Rule 20.7 violation

The macro parameter 'p' is used as an expression and needs to be enclosed in
parentheses.

Signed-off-by: Xenia Ragiadakou 
Reviewed-by: Stefano Stabellini 

commit 124f138b37d595294b3100349e26ffb3f1df7b13
Author: Xenia Ragiadakou 
Date:   Fri Jul 29 08:50:58 2022 +0200

xsm/dummy: fix MISRA C 2012 Directive 4.10 violation

Protect header file from being included more than once by adding ifndef 
guard.

Signed-off-by: Xenia Ragiadakou 
Reviewed-by: Luca Fancellu 
Acked-by: Daniel P. Smith 

commit 9ff3231f955cee4d62c7be6a03d061c037d7ca69
Author: Jan Beulich 
Date:   Fri Jul 29 08:50:25 2022 +0200

x86/shadow: drop CONFIG_HVM conditionals from sh_update_cr3()

Now that we're not building multi.c anymore for 2 and 3 guest levels
when !HVM, there's no point in having these conditionals anymore. (As
somewhat a special case, the last of the removed conditionals really
builds on shadow_mode_external() always returning false when !HVM.) This
way the code becomes a tiny bit more readable.

Signed-off-by: Jan Beulich 
Acked-by: Andrew Cooper 

commit 5b04fe78646a8222626996113c9d1e598cb84831
Author: Jan Beulich 
Date:   Fri Jul 29 08:49:48 2022 +0200

x86/shadow: don't open-code shadow_remove_all_shadows()

Let's use the existing inline wrapper instead of repeating respective
commentary at every site.

Signed-off-by: Jan Beulich 
Acked-by: Andrew Cooper 

commit 8a3b89e4307da260675483bb86fc06cc62ed7c08
Author: Jan Beulich 
Date:   Fri Jul 29 08:49:06 2022 +0200

x86/shadow: exclude HVM-only code from sh_remove_shadows() when !HVM

In my (debug) build this amounts to well over 500 bytes of dead code.

Signed-off-by: Jan Beulich 
Acked-by: Andrew Cooper 

commit 3629759626ac7201a670a8a2d4d4a536e7443575
Author: Jan Beulich 
Date:   Fri Jul 29 08:48:26 2022 +0200

x86/shadow: properly handle get_page() failing

We should not blindly (in a release build) insert the new entry in the
hash if a reference to the guest page cannot be obtained, or else an
excess reference would be put when removing the hash entry again. Crash
the domain 

[linux-5.4 test] 171923: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 linux77ba2b9b46f8acead2606759e8196b7076eaeeea
baseline version:
 linux002c3bbb4713859e8f3d1e756637572a09dcca49

Last test of basis   171846  2022-07-24 18:41:18 Z5 days
Testing same since   171923  2022-07-29 15:42:06 Z0 days1 attempts


People who touched revisions under test:
  
  Al Viro 
  Alexander Aring 
  Andrew Morton 
  

Re: [PATCH 3/3] automation: qemu-smoke-arm64.sh: Fix the number of cpus in the device tree

[Stefano Stabellini]

On Fri, 29 Jul 2022, Xenia Ragiadakou wrote:
> Qemu VM is configured with 2 cpus but the device tree passed has only 1.
> Generate a device tree with 2 cpus.
> 
> Signed-off-by: Xenia Ragiadakou 
> ---
> I am not sure for this patch because I do not know whether the number of cpus
> differs deliberately.

Yes let's go with 2

Reviewed-by: Stefano Stabellini 

>  automation/scripts/qemu-smoke-arm64.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/automation/scripts/qemu-smoke-arm64.sh 
> b/automation/scripts/qemu-smoke-arm64.sh
> index 7ac82b2278..b48a20988f 100755
> --- a/automation/scripts/qemu-smoke-arm64.sh
> +++ b/automation/scripts/qemu-smoke-arm64.sh
> @@ -35,7 +35,7 @@ curl -fsSLO 
> https://github.com/qemu/qemu/raw/v5.2.0/pc-bios/efi-virtio.rom
>  ./binaries/qemu-system-aarch64 \
> -machine virtualization=true \
> -cpu cortex-a57 -machine type=virt \
> -   -m 1024 -display none \
> +   -m 1024 -smp 2 -display none \
> -machine dumpdtb=binaries/virt-gicv2.dtb
>  # XXX disable pl061 to avoid Linux crash
>  dtc -I dtb -O dts binaries/virt-gicv2.dtb > binaries/virt-gicv2.dts
> -- 
> 2.34.1
> 

Re: [PATCH 2/3] automation: qemu-smoke-arm64.sh: Rename the device tree to avoid confusion

[Stefano Stabellini]

On Fri, 29 Jul 2022, Xenia Ragiadakou wrote:
> Rename the device tree from virt-gicv3 to virt-gicv2 to avoid confusion
> since the version of the generic interrupt controller used for this test
> is the v2 and not the v3.
> 
> Signed-off-by: Xenia Ragiadakou 

Reviewed-by: Stefano Stabellini 


> ---
>  automation/scripts/qemu-smoke-arm64.sh | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/automation/scripts/qemu-smoke-arm64.sh 
> b/automation/scripts/qemu-smoke-arm64.sh
> index f483a2ffc1..7ac82b2278 100755
> --- a/automation/scripts/qemu-smoke-arm64.sh
> +++ b/automation/scripts/qemu-smoke-arm64.sh
> @@ -36,11 +36,11 @@ curl -fsSLO 
> https://github.com/qemu/qemu/raw/v5.2.0/pc-bios/efi-virtio.rom
> -machine virtualization=true \
> -cpu cortex-a57 -machine type=virt \
> -m 1024 -display none \
> -   -machine dumpdtb=binaries/virt-gicv3.dtb
> +   -machine dumpdtb=binaries/virt-gicv2.dtb
>  # XXX disable pl061 to avoid Linux crash
> -dtc -I dtb -O dts binaries/virt-gicv3.dtb > binaries/virt-gicv3.dts
> -sed 's/compatible = "arm,pl061.*/status = "disabled";/g' 
> binaries/virt-gicv3.dts > binaries/virt-gicv3-edited.dts
> -dtc -I dts -O dtb binaries/virt-gicv3-edited.dts > binaries/virt-gicv3.dtb
> +dtc -I dtb -O dts binaries/virt-gicv2.dtb > binaries/virt-gicv2.dts
> +sed 's/compatible = "arm,pl061.*/status = "disabled";/g' 
> binaries/virt-gicv2.dts > binaries/virt-gicv2-edited.dts
> +dtc -I dts -O dtb binaries/virt-gicv2-edited.dts > binaries/virt-gicv2.dtb
>  
>  
>  # Busybox
> @@ -73,7 +73,7 @@ cd ..
>  echo 'MEMORY_START="0x4000"
>  MEMORY_END="0x8000"
>  
> -DEVICE_TREE="virt-gicv3.dtb"
> +DEVICE_TREE="virt-gicv2.dtb"
>  XEN="xen"
>  DOM0_KERNEL="Image"
>  DOM0_RAMDISK="initrd"
> -- 
> 2.34.1
> 

Re: [PATCH 1/3] automation: qemu-smoke-arm64.sh: Remove some stale comments

[Stefano Stabellini]

On Fri, 29 Jul 2022, Xenia Ragiadakou wrote:
> Remove comment "# Install QEMU" because qemu is not installed, it is taken
> from a test-artifacts container.
> 
> Change comment "# Busybox Dom0" to "# Busybox" because busybox is not used
> only for the Dom0 but also for the DomU.
> 
> Signed-off-by: Xenia Ragiadakou 

Reviewed-by: Stefano Stabellini 


> ---
>  automation/scripts/qemu-smoke-arm64.sh | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/automation/scripts/qemu-smoke-arm64.sh 
> b/automation/scripts/qemu-smoke-arm64.sh
> index 898398196a..f483a2ffc1 100755
> --- a/automation/scripts/qemu-smoke-arm64.sh
> +++ b/automation/scripts/qemu-smoke-arm64.sh
> @@ -21,7 +21,6 @@ fi
>  "
>  fi
>  
> -# Install QEMU
>  export DEBIAN_FRONTENT=noninteractive
>  apt-get -qy update
>  apt-get -qy install --no-install-recommends u-boot-qemu \
> @@ -44,7 +43,7 @@ sed 's/compatible = "arm,pl061.*/status = "disabled";/g' 
> binaries/virt-gicv3.dts
>  dtc -I dts -O dtb binaries/virt-gicv3-edited.dts > binaries/virt-gicv3.dtb
>  
>  
> -# Busybox Dom0
> +# Busybox
>  mkdir -p initrd
>  mkdir -p initrd/bin
>  mkdir -p initrd/sbin
> -- 
> 2.34.1
> 

Re: [PATCH v2 0/5] xen/arm: mm: Bunch of clean-ups

[Julien Grall]

Hi,

On 20/07/2022 19:44, Julien Grall wrote:

From: Julien Grall 

Hi all,

This series is a collection of patches to clean-up the MM subsystem
I have done in preparation for the next revision of "xen/arm: Don't
switch TTBR while the MMU is on" [1].

Cheers,

[1] https://lore.kernel.org/all/20220309112048.17377-1-jul...@xen.org/

Julien Grall (5):
   xen/arm: Remove most of the *_VIRT_END defines
   xen/arm32: mm: Consolidate the domheap mappings initialization
   xen: Rename CONFIG_DOMAIN_PAGE to CONFIG_ARCH_MAP_DOMAIN_PAGE and...
   xen/arm: mm: Move domain_{,un}map_* helpers in a separate file
   xen/arm: mm: Reduce the area that xen_second covers


I have committed this series.

Cheers,

--
Julien Grall

[ovmf test] 171929: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf e9150618ec91f79e70a1719ac8c198bee34a99be
baseline version:
 ovmf 0d0bfcb4571caa65b7875003f38e67e2ac7e5560

Last test of basis   171913  2022-07-29 03:12:41 Z0 days
Testing same since   171929  2022-07-29 19:42:00 Z0 days1 attempts


People who touched revisions under test:
  Sami Mujawar 

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
   0d0bfcb457..e9150618ec  e9150618ec91f79e70a1719ac8c198bee34a99be -> 
xen-tested-master

Re: [PATCH v2 3/5] xen: Rename CONFIG_DOMAIN_PAGE to CONFIG_ARCH_MAP_DOMAIN_PAGE and...

[Julien Grall]

On 21/07/2022 09:40, Bertrand Marquis wrote:

Hi Julien,


Hi Bertrand,


On 20 Jul 2022, at 19:44, Julien Grall  wrote:

From: Julien Grall 

move it to Kconfig.

The define CONFIG_DOMAIN_PAGE indicates whether the architecture provide
helpers to map/unmap a domain page. Rename it to the define to


Maybe “the define to” can be removed in this sentence or it needs some 
rephrasing.


I have removed "the define to".




CONFIG_ARCH_MAP_DOMAIN_PAGE so it is clearer that this will not remove
support for domain page (this is not a concept that Xen can't get
away with).

Take the opportunity to move CONFIG_MAP_DOMAIN_PAGE to Kconfig as this
will soon be necessary to use it in the Makefile.

Signed-off-by: Julien Grall 


With this fixed:
Reviewed-by: Bertrand Marquis  #arm part


Thanks!

Cheers,

--
Julien Grall

Re: [PATCH v2 3/5] xen: Rename CONFIG_DOMAIN_PAGE to CONFIG_ARCH_MAP_DOMAIN_PAGE and...

[Julien Grall]

Hi Jan,

On 25/07/2022 16:51, Jan Beulich wrote:

On 20.07.2022 20:44, Julien Grall wrote:

From: Julien Grall 

move it to Kconfig.

The define CONFIG_DOMAIN_PAGE indicates whether the architecture provide
helpers to map/unmap a domain page. Rename it to the define to
CONFIG_ARCH_MAP_DOMAIN_PAGE so it is clearer that this will not remove
support for domain page (this is not a concept that Xen can't get
away with).


Especially the part in parentheses reads odd, if not backwards.


Indeed. I tweaked the sentence to:

Rename it to CONFIG_ARCH_MAP_DOMAIN_PAGE so it is clearer that support 
for domain page is not something that can be disabled in Xen.





--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -371,7 +371,7 @@ void clear_fixmap(unsigned int map)
  BUG_ON(res != 0);
  }
  
-#ifdef CONFIG_DOMAIN_PAGE

+#ifdef CONFIG_ARCH_MAP_DOMAIN_PAGE
  /*
   * Prepare the area that will be used to map domheap pages. They are
   * mapped in 2MB chunks, so we need to allocate the page-tables up to


What about the other #ifdef in build_assertions()? With that also
converted (and preferably with the description adjusted)


Good catch. I update the patch.


Reviewed-by: Jan Beulich 


Thanks for the review!

Cheers,

--
Julien Grall

Re: [PATCH v1] xen: add late init call in start_xen

[Stefano Stabellini]

On Fri, 29 Jul 2022, Boyoun Park wrote:
> I really appreciate all the comments and reviews.
> I understand that it is hard to add this patch without any usage.
> 
> On Fri, 29 Jul 2022, Stefano Stabellini:
> > On Thu, 28 Jul 2022, Jan Beulich wrote:
> > > On 28.07.2022 11:22, Boyoun Park wrote:
> > > > Hello,
> > > > 
> > > > This patch added late_initcall to deal with
> > > > 
> > > > some init functions which should be called
> > > > 
> > > > after other init functions in start_xen.
> > > > 
> > > > If this patch is added,
> > > > 
> > > > then the original initcall in xen will be treated
> > > > 
> > > > as early_initcall and the late_initcall
> > > > 
> > > > which is added by this patch will be
> > > > 
> > > > called sequentially.
> > > > 
> > > > I cannot send patches through git send-email
> > > > 
> > > > due to some security issues in my work.
> > > > 
> > > > So intead, I just send the patches manually.
> > > 
> > > Which is fine, but you want to configure your mail client such that it
> > > doesn't mangle the patch. Albeit I see that to cover for that at least
> > > you've also attached both the patch and a cover letter. For a single
> > > patch a cover letter can normally be omitted, but if you don't then
> > > even if you're sending without "git send-email" you will want to send
> > > both as separate mails, with the patch being a reply to the cover
> > > letter thread root.
> > 
> > Yeah. Boyoun, if you only have a couple of patches then it is fine to
> > send them manually. Otherwise, if you have many patches, you can send
> > them in attachment as tarball and I'll send them all to xen-devel using
> > git-send-email for you (of course keeping you as original author and
> > retaining all Signed-off-bys).
> 
> You're awesome. Thanks you so much. I'm still requesting approvals to use git 
> send-email.
> I'll let you know if I have to send many patches wihout git send-email.
> 
> > > > Subject: [PATCH v1] xen: add late init call in start_xen
> > > > 
> > > > This patch added late_initcall section in init.data.
> > > > 
> > > > The late initcall would be called after initcall
> > > > 
> > > > in the start_xen function.
> > > > 
> > > > Some initializing works on priority should be run
> > > > 
> > > > in do_initcalls and other non-prioritized works
> > > > 
> > > > would be run in do_late_initcalls.
> > > > 
> > > > To call some functions by late_initcall,
> > > > 
> > > > then it is possible by using
> > > > 
> > > > __late_initcall(/*Function Name*/);
> > > > 
> > > > Signed-off-by: Boyoun Park 
> > > 
> > > So I could imagine this patch to be in a series where a subsequent
> > > patch then adds an actual use of the new functionality. Without
> > > that what you're proposing is to add dead code.
> > 
> > Yeah, I think it would be cool to have late initcalls but it makes sense
> > to add them if we have someone that makes use of them.
> 
> I totally agree with your comments. Some drivers that I'm developing now and 
> use this function are specific to my hardware environment.
> Thus, it seems difficult to arrange them in the near future.
> I will update patches if I can suggest an actual use.

I totally understand custom setups and non-upstreamable configurations
and I have certainly some of them myself.

However, I just wanted to let you know that we are fine with accepting
platform specific drivers in Xen where it makes sense, for instance:

- Renesas IPMMU-VMSA found in R-Car Gen3/Gen4 SoCs (an IOMMU driver)
xen/drivers/passthrough/arm/ipmmu-vmsa.c

- Xilinx EEMI firmware interface "mediator" in Xen (power management)
xen/arch/arm/platforms/xilinx-zynqmp-eemi.c

Cheers,

Stefano

Re: Enable audio virtualization in Xen

[Christopher Clark]

On Thu, Jul 28, 2022 at 1:37 PM Christopher Clark
 wrote:
>
> On Thu, Jul 28, 2022 at 12:05 PM SHARMA, JYOTIRMOY
>  wrote:
> >
> > [AMD Official Use Only - General]
> >
> > Hi all,
> >
> > Can anyone please help here?

Hello again,

I have now been able to enable HVM guest audio using the Xen PV audio
front end device drivers that are included in Ubuntu 22.04.
The steps:
- remove the qemu wrapper script from my previous email, if you have
installed it, and restore the original qemu-system-i386 binary
- keep the pulseaudio system configuration from my previous email, so
that tools that run as the root user are able to talk to the desktop
host pulseaudio system
- add a vsnd device to the guest VM configuration file, in this case
using pulseaudio for output:

vsnd = [[ 'card, backend=Domain-0, buffer-size=65536,
short-name=VCard, long-name=Virtual sound card, sample-rates=48000,
sample-formats=s16_le', 'pcm, name=dev1', 'stream, unique-id=pulse,
type=P' ]]

- download, build and run the backend audio tool in the host dom0.

For the backend you'll need to download and build first libxenbe and
then snd_be from:
https://github.com/xen-troops/libxenbe
https://github.com/xen-troops/snd_be

Once you have the snd_be binary built, you can run it as root thus
before starting your guest VM:
./snd_be -v *:Debug
and then the guest audio output should be working OK.

I have not explored using ALSA rather than pulseaudio yet.

Christopher

[xen-unstable-smoke test] 171928: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-amd64-libvirt   6 libvirt-buildfail REGR. vs. 171884

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt  1 build-check(1)   blocked  n/a
 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  062790aca6b1faea62c9ed2737c3791efb0d0f4c
baseline version:
 xen  f732240fd3bac25116151db5ddeb7203b62e85ce

Last test of basis   171884  2022-07-27 12:03:31 Z2 days
Failing since171899  2022-07-28 19:01:47 Z1 days7 attempts
Testing same since   171917  2022-07-29 10:03:07 Z0 days3 attempts


People who touched revisions under test:
  Andrew Cooper 
  Daniel P. Smith 
  George Dunlap 
  Jan Beulich 
  Jiamei Xie 
  Julien Grall 
  Oleksandr Tyshchenko 
  Stefano Stabellini 
  Xenia Ragiadakou 

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



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


Not pushing.


commit 062790aca6b1faea62c9ed2737c3791efb0d0f4c
Author: Xenia Ragiadakou 
Date:   Fri Jul 29 08:51:31 2022 +0200

arm/atomic: fix MISRA C 2012 Rule 20.7 violation

The macro parameter 'p' is used as an expression and needs to be enclosed in
parentheses.

Signed-off-by: Xenia Ragiadakou 
Reviewed-by: Stefano Stabellini 

commit 124f138b37d595294b3100349e26ffb3f1df7b13
Author: Xenia Ragiadakou 
Date:   Fri Jul 29 08:50:58 2022 +0200

xsm/dummy: fix MISRA C 2012 Directive 4.10 violation

Protect header file from being included more than once by adding ifndef 
guard.

Signed-off-by: Xenia Ragiadakou 
Reviewed-by: Luca Fancellu 
Acked-by: Daniel P. Smith 

commit 9ff3231f955cee4d62c7be6a03d061c037d7ca69
Author: Jan Beulich 
Date:   Fri Jul 29 08:50:25 2022 +0200

x86/shadow: drop CONFIG_HVM conditionals from sh_update_cr3()

Now that we're not building multi.c anymore for 2 and 3 guest levels
when !HVM, there's no point in having these conditionals anymore. (As
somewhat a special case, the last of the removed conditionals really
builds on shadow_mode_external() always returning false when !HVM.) This
way the code becomes a tiny bit more readable.

Signed-off-by: Jan Beulich 
Acked-by: Andrew Cooper 

commit 5b04fe78646a8222626996113c9d1e598cb84831
Author: Jan Beulich 
Date:   Fri Jul 29 08:49:48 2022 +0200

x86/shadow: don't open-code shadow_remove_all_shadows()

Let's use the existing inline wrapper instead of repeating respective
commentary at every site.

Signed-off-by: Jan Beulich 
Acked-by: Andrew Cooper 

commit 8a3b89e4307da260675483bb86fc06cc62ed7c08
Author: Jan Beulich 
Date:   Fri Jul 29 08:49:06 2022 +0200

x86/shadow: exclude HVM-only code from sh_remove_shadows() when !HVM

In my (debug) build this amounts to well over 500 bytes of dead code.

Signed-off-by: Jan Beulich 
Acked-by: Andrew Cooper 

commit 3629759626ac7201a670a8a2d4d4a536e7443575
Author: Jan Beulich 
Date:   Fri Jul 29 08:48:26 2022 +0200

x86/shadow: properly handle get_page() failing

We should not blindly (in a release build) insert the new entry in the
hash if a reference to the guest page cannot be obtained, or else an
excess reference would be put when removing the hash entry again. Crash
the domain 

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 linux6e2c0490769ef8a95b61304389116ccc85c53e12
baseline version:
 linux6e7765cb477a9753670d4351d14de93f1e9dbbd4

Last test of basis   171891  2022-07-28 00:42:47 Z1 days
Failing since171901  2022-07-28 20:11:14 Z1 days2 attempts
Testing same since   171916  2022-07-29 06:34:59 Z0 days1 attempts


People who touched revisions under test:
  Abhishek Pandit-Subedi 
  Alejandro Lucero 
  Anirudh Venkataramanan 
  Benjamin Poirier 
  Christophe JAILLET 
  Dan Carpenter 
  Dave Airlie 
  Dave Switzer 
  David Howells 
  David S. Miller 
  Dawid Lukwinski 
  Dimitris Michailidis 
  Dimitris Michailidis 
  Duoming Zhou 
  Eric Dumazet 
  Florian 

[RFC PATCH] tools/configure: require OCaml >= 4.06.1 for oxenstored

[Edwin Török]

OCaml 4.06.1 is widely available in distributions: 
https://repology.org/project/ocaml/versions

oxenstored already includes some compatibility code to be able to run on
versions older than 4.06, however this is slightly less efficient than
just using the new features in 4.06 standard library:
https://lore.kernel.org/xen-devel/b94cd2ad099486678609909e12b045c54abb2f27.ca...@citrix.com/

The OCaml version in stubdom/ is unchanged for now as it is unclear how
this used. Typically to run OCaml code as a stubdom one would use the mirage
tooling to build a unikernel, which handles cross-compilation using
Dune.
The unikernel itself also uses Solo5 instead of MiniOS, so the OCaml
code in stubdom/ is probably stale.

Signed-off-by: Edwin Török 
Cc: Christian Lindig 
---
 tools/configure| 2 +-
 tools/configure.ac | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/configure b/tools/configure
index 41deb7fb96..8f391e2da4 100755
--- a/tools/configure
+++ b/tools/configure
@@ -6765,7 +6765,7 @@ else
  -e 's/[^0-9]//g'`
 
 
-  ax_compare_version_B=`echo "4.02.0" | sed -e 's/\([0-9]*\)/Z\1Z/g' \
+  ax_compare_version_B=`echo "4.06.1" | sed -e 's/\([0-9]*\)/Z\1Z/g' \
  -e 's/Z\([0-9]\)Z/Z0\1Z/g' \
  -e 's/Z\([0-9][0-9]\)Z/Z0\1Z/g' \
  -e 's/Z\([0-9][0-9][0-9]\)Z/Z0\1Z/g' \
diff --git a/tools/configure.ac b/tools/configure.ac
index 32cbe6bd3c..7518199ec8 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -310,7 +310,7 @@ AS_IF([test "x$ocamltools" = "xy"], [
 AC_MSG_ERROR([Ocaml tools enabled, but missing ocamlopt or 
ocamlfind])])
 ocamltools="n"
 ], [
-AX_COMPARE_VERSION([$OCAMLVERSION], [lt], [4.02.0], [
+AX_COMPARE_VERSION([$OCAMLVERSION], [lt], [4.06.1], [
 AS_IF([test "x$enable_ocamltools" = "xyes"], [
 AC_MSG_ERROR([Your version of OCaml: $OCAMLVERSION is not 
supported])])
 ocamltools="n"
-- 
2.34.1

[PATCH v1 5/7] tools/ocaml: fix compiler warnings

[Edwin Török]

Fix compiler warning about:
* unused value
* ambiguous documentation comment
* non-principal type inference (compiler version dependent)

No functional change.

Signed-off-by: Edwin Török 
---
 tools/ocaml/xenstored/connection.ml | 2 +-
 tools/ocaml/xenstored/process.ml| 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/ocaml/xenstored/connection.ml 
b/tools/ocaml/xenstored/connection.ml
index 65f99ea6f2..a94d47cdc2 100644
--- a/tools/ocaml/xenstored/connection.ml
+++ b/tools/ocaml/xenstored/connection.ml
@@ -313,7 +313,7 @@ let is_bad con = match con.dom with None -> false | Some 
dom -> Domain.is_bad_do
 let has_extra_connection_data con =
let has_in = has_input con || has_partial_input con in
let has_out = has_output con in
-   let has_socket = con.dom = None in
+   let _has_socket = con.dom = None in
let has_nondefault_perms = make_perm con.dom <> con.perm in
has_in || has_out
(* TODO: what about SIGTERM, should use systemd to store FDS
diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
index 27790d4a5c..86eed02413 100644
--- a/tools/ocaml/xenstored/process.ml
+++ b/tools/ocaml/xenstored/process.ml
@@ -59,7 +59,7 @@ let split_one_path data con =
 
 let process_watch t cons =
let oldroot = t.Transaction.oldroot in
-   let newroot = Store.get_root t.store in
+   let newroot = Store.get_root t.Transaction.store in
let ops = Transaction.get_paths t |> List.rev in
let do_op_watch op cons =
let recurse, oldroot, root = match (fst op) with
@@ -491,7 +491,7 @@ let transaction_replay c t doms cons =
ignore @@ Connection.end_transaction c tid None
)
 
-let do_watch con t _domains cons data =
+let do_watch con _t _domains cons data =
let (node, token) =
match (split None '\000' data) with
| [node; token; ""]   -> node, token
@@ -651,6 +651,7 @@ let maybe_ignore_transaction = function
 
 
 let () = Printexc.record_backtrace true
+
 (**
  * Nothrow guarantee.
  *)
-- 
2.34.1

[PATCH v1 4/7] tools/ocaml: Makefile to drive dune

[Edwin Török]

create a separate Makefile that can be used to drive dune.

Usage:
`make -f Makefile.dune`

There are some files that need to be created by the Makefile based
build system (such as all the C code in $(XEN_ROOT)/tools/libs),
and those need to exist before dune runs.

Although it'd be possible to automatically call the necessary makefile
rules from dune, it wouldn't work reliably:
* dune uses sandboxing by default (only files declared or known as
  dependencies are visible to individual build commands,
  symlinks/hardlinks are used by dune to implement this)
* the dune builds always run in a _build subdir, and calling the
  makefiles from there would get the wrong XEN_ROOT set
* running the make command in the source tree would work, but dune still
  wouldn't immediately see the build dependencies since they wouldn't
  have been copied/linked under _build

The approach here is to:
* use the Makefile to build C-only prerequisites (i.e. most of Xen)
* use Dune only to build the OCaml parts once the C prerequisites exist
* dune has dependencies declared on the C bits, so if they are missing
  you will get an error about a missing rule to create them instead of a
  cryptic compilation error
* dune is still optional - the old Makefile based buildsystem is still
  there for now
* use dune exclusively for new code going forward (e.g. OCaml test-suites)

The workspace file needs to be generated by make because this currently
cannot be generated by dune, and it doesn't support including external
files. But could be generated by configure?

LD_LIBRARY_PATH needs to be set, because even with -Wl,-rpath
executables wouldn't be able to run using the just-built libraries,
unless we'd also link all the transitive dependencies of libs.

No functional change.

Signed-off-by: Edwin Török 
---
 Makefile  |  5 ++
 tools/ocaml/Makefile.dune | 88 +++
 tools/ocaml/dune-workspace.dev.in |  2 +
 tools/ocaml/dune-workspace.in | 18 +++
 4 files changed, 113 insertions(+)
 create mode 100644 tools/ocaml/Makefile.dune
 create mode 100644 tools/ocaml/dune-workspace.dev.in
 create mode 100644 tools/ocaml/dune-workspace.in

diff --git a/Makefile b/Makefile
index b93b22c752..ddb33c3555 100644
--- a/Makefile
+++ b/Makefile
@@ -68,6 +68,11 @@ build-tools-oxenstored: build-tools-public-headers
$(MAKE) -s -C tools/libs
$(MAKE) -C tools/ocaml build-tools-oxenstored
 
+.PHONY: build-tools-oxenstored-prepare
+build-tools-oxenstored-prepare: build-tools-public-headers
+   test -f tools/config.status || (cd tools && ./configure 
--with-xenstored=oxenstored)
+   $(MAKE) -C tools/libs V=
+
 .PHONY: build-stubdom
 build-stubdom: mini-os-dir build-tools-public-headers
$(MAKE) -C stubdom build
diff --git a/tools/ocaml/Makefile.dune b/tools/ocaml/Makefile.dune
new file mode 100644
index 00..eca9cac0ca
--- /dev/null
+++ b/tools/ocaml/Makefile.dune
@@ -0,0 +1,88 @@
+XEN_ROOT = $(CURDIR)/../..
+all: dune-all-check
+
+# Dune by default uses all available CPUs. Make doesn't.
+# Query the available CPUs and use all available for any of the make rules we 
call out to.
+# -O is also needed with parallel make such that the build error and the build 
command causing
+#  the error are close together and not interspersed with other output
+NPROC=$(shell getconf _NPROCESSORS_ONLN)
+MAKEN=$(MAKE) -j$(NPROC) -O
+
+# We want to link and use the Xen libraries built locally
+# without installing them system-wide
+# (the system-wide one installed from packages will likely be too old and not 
match the locally
+# built one anyway).
+#
+# Set LIBRARY_PATH and LD_LIBRARY_PATH so that the linker
+# finds the proper libraries and the various dune commands
+# work (e.g. running tests, utop, etc.).
+#
+# The Makefile based buildsystem would use -Wl,-rpath-link= here,
+# but that only works during linking, not runtime.
+# There is a -Wl, -rpath= that can be used, but that only works
+# for libraries linked directly to the main executable:
+# the dependencies of those libraries won't get found on the rpath
+# (the rpath of the executable is apparently not used during that search).
+#
+# Use environment variables, because that way we don't make any permanent 
alternations (rpath)
+# to the executable, so once installed system-wide it won't refer to build 
paths anymore.
+#
+# Dune cannot be used to generate this file: the env-vars stanza doesn't 
support %{read:}, :include,
+# and dune-workspace doesn't support (include) stanzas.
+# So for now generate it from this Makefile
+# Cannot start with comment, so add auto-generated comment at the end
+LIB_DIRS=$(abspath $(wildcard ../libs/*/.))
+LIBRARY_PATH=$(subst $(eval) ,:,$(LIB_DIRS))
+../dune-workspace ../dune-workspace.dev: dune-workspace.in 
dune-workspace.dev.in Makefile.dune
+   @( sed -e "s|@LIBRARY_PATH@|$(LIBRARY_PATH)|" <$< \
+   && echo "; DO NOT EDIT: autogenerated from ocaml/dune-workspace.in") 

[PATCH v1 1/7] tools/ocaml/Makefile: do not run ocamldep during make clean

[Edwin Török]

Trying to include .ocamldep.make will cause it to be generated if it
doesn't exist.
We do not want this during make clean: we would remove it anyway.

Speeds up make clean.

Before (measured on f732240fd3bac25116151db5ddeb7203b62e85ce, July 2022):
```
Parsing 
/home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
Parsing 
/home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
Parsing 
/home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
Parsing 
/home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
Parsing 
/home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl

 Performance counter stats for 'make clean -j8 -s' (5 runs):

4.2233 +- 0.0208 seconds time elapsed  ( +-  0.49% )
```

After:
```
perf stat -r 5 --null make clean -j8 -s

 Performance counter stats for 'make clean -j8 -s' (5 runs):

2.7325 +- 0.0138 seconds time elapsed  ( +-  0.51% )
```

No functional change.

Signed-off-by: Edwin Török 
---
 tools/ocaml/Makefile.rules | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/ocaml/Makefile.rules b/tools/ocaml/Makefile.rules
index 7e4db457a1..d368308d9b 100644
--- a/tools/ocaml/Makefile.rules
+++ b/tools/ocaml/Makefile.rules
@@ -44,8 +44,10 @@ META: META.in
 
 ALL_OCAML_OBJ_SOURCES=$(addsuffix .ml, $(ALL_OCAML_OBJS))
 
+ifneq ($(MAKECMDGOALS),clean)
 .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile 
$(OCAML_TOPLEVEL)/Makefile.rules
$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli 
$o,MLDEP,)
+endif
 
 clean: $(CLEAN_HOOKS)
$(Q)rm -f .*.d *.o *.so *.a *.cmo *.cmi *.cma *.cmx *.cmxa *.annot 
*.spot *.spit $(LIBS) $(PROGRAMS) $(GENERATED_FILES) .ocamldep.make META
-- 
2.34.1

[PATCH v1 3/7] tools/ocaml/*/dune: dune based build system

[Edwin Török]

Based on Christian Lindig's work.

Initially this will be used to build unit tests, and to make development
easier.

Dune supports proper incremental builds and editor integration with
merlin/LSP.

For now the Makefile based build system is retained too: this is not a
hard dependency on Dune.

Using version 2.1 of Dune build language here, because that is the one
available in Ubuntu Focal (part of the CI here).

No functional change.

Signed-off-by: Edwin Török 
---
 tools/.gitignore   |  7 +
 tools/dune |  5 
 tools/dune-project |  1 +
 tools/ocaml/dune-project   | 27 ++
 tools/ocaml/libs/eventchn/dune | 11 
 tools/ocaml/libs/mmap/dune |  9 ++
 tools/ocaml/libs/xb/dune   | 10 +++
 tools/ocaml/libs/xc/dune   | 16 +++
 tools/ocaml/libs/xs/dune   | 15 ++
 tools/ocaml/xenstored/dune | 51 ++
 10 files changed, 152 insertions(+)
 create mode 100644 tools/.gitignore
 create mode 100644 tools/dune
 create mode 100644 tools/dune-project
 create mode 100644 tools/ocaml/dune-project
 create mode 100644 tools/ocaml/libs/eventchn/dune
 create mode 100644 tools/ocaml/libs/mmap/dune
 create mode 100644 tools/ocaml/libs/xb/dune
 create mode 100644 tools/ocaml/libs/xc/dune
 create mode 100644 tools/ocaml/libs/xs/dune
 create mode 100644 tools/ocaml/xenstored/dune

diff --git a/tools/.gitignore b/tools/.gitignore
new file mode 100644
index 00..c211749a3b
--- /dev/null
+++ b/tools/.gitignore
@@ -0,0 +1,7 @@
+dune-workspace*
+_build/
+.merlin
+*.h.gch
+*.opam
+ocaml/*.install
+include/_xentoolcore_list.h
diff --git a/tools/dune b/tools/dune
new file mode 100644
index 00..febbd078f0
--- /dev/null
+++ b/tools/dune
@@ -0,0 +1,5 @@
+; only look inside ocaml and include subdirectory, speeds up the build
+; since dune doesn't need to copy/hash/monitor all the other files
+(dirs ocaml)
+
+(data_only_dirs include libs)
diff --git a/tools/dune-project b/tools/dune-project
new file mode 100644
index 00..cd8d4e3d86
--- /dev/null
+++ b/tools/dune-project
@@ -0,0 +1 @@
+(lang dune 2.1)
diff --git a/tools/ocaml/dune-project b/tools/ocaml/dune-project
new file mode 100644
index 00..1dae7b0acb
--- /dev/null
+++ b/tools/ocaml/dune-project
@@ -0,0 +1,27 @@
+(lang dune 2.1)
+
+(name xen)
+
+(formatting (enabled_for dune))
+(generate_opam_files true)
+
+(maintainers christian.lin...@citrix.com)
+(license LGPL)
+
+(package
+ (name xen)
+ (synopsis "Xen interfaces")
+ (depends
+  base-unix
+  (dune (>= 2.1))
+ )
+)
+
+(package
+ (name xenstored)
+ (synopsis "In-memory key-value store for the Xen hypervisor")
+ (depends
+  base-unix
+  (dune (>= 2.1))
+ )
+)
diff --git a/tools/ocaml/libs/eventchn/dune b/tools/ocaml/libs/eventchn/dune
new file mode 100644
index 00..4468f2e769
--- /dev/null
+++ b/tools/ocaml/libs/eventchn/dune
@@ -0,0 +1,11 @@
+(library
+ (foreign_stubs
+  (language c)
+  (names xeneventchn_stubs)
+  (extra_deps ../../../include/xen/xen.h ../../../libs/evtchn/libxenevtchn.so)
+  (include_dirs ../../../include))
+ (name xeneventchn)
+ (public_name xen.eventchn)
+ (libraries unix)
+ (no_dynlink)
+ (c_library_flags -lxenevtchn))
diff --git a/tools/ocaml/libs/mmap/dune b/tools/ocaml/libs/mmap/dune
new file mode 100644
index 00..57a8ab5b9b
--- /dev/null
+++ b/tools/ocaml/libs/mmap/dune
@@ -0,0 +1,9 @@
+(library
+ (foreign_stubs
+  (language c)
+  (names xenmmap_stubs))
+ (name xenmmap)
+ (public_name xen.mmap)
+ (libraries unix)
+ (no_dynlink)
+ (install_c_headers mmap_stubs))
diff --git a/tools/ocaml/libs/xb/dune b/tools/ocaml/libs/xb/dune
new file mode 100644
index 00..13a507ea87
--- /dev/null
+++ b/tools/ocaml/libs/xb/dune
@@ -0,0 +1,10 @@
+(library
+ (foreign_stubs
+  (language c)
+  (extra_deps ../../../include/xen/xen.h)
+  (include_dirs ../../../include)
+  (names xenbus_stubs xs_ring_stubs))
+ (name xenbus)
+ (public_name xen.bus)
+ (no_dynlink)
+ (libraries unix xenmmap))
diff --git a/tools/ocaml/libs/xc/dune b/tools/ocaml/libs/xc/dune
new file mode 100644
index 00..6f9450cd27
--- /dev/null
+++ b/tools/ocaml/libs/xc/dune
@@ -0,0 +1,16 @@
+(rule
+ (with-stdout-to
+  xenctrl_abi_check.h
+  (run perl -w %{dep:abi-check} %{dep:xenctrl_stubs.c} %{dep:xenctrl.ml})))
+
+(library
+ (foreign_stubs
+  (language c)
+  (names xenctrl_stubs)
+  (extra_deps ../../../include/xen/xen.h ../../../libs/ctrl/libxenctrl.so)
+  (include_dirs ../../../include))
+ (name xenctrl)
+ (public_name xen.ctrl)
+ (libraries unix xenmmap)
+ (no_dynlink)
+ (c_library_flags -lxenctrl -lxenguest))
diff --git a/tools/ocaml/libs/xs/dune b/tools/ocaml/libs/xs/dune
new file mode 100644
index 00..086259f51d
--- /dev/null
+++ b/tools/ocaml/libs/xs/dune
@@ -0,0 +1,15 @@
+; fallback mode: the files may have been generated by configure already
+
+(rule
+ (targets paths.ml)
+ (deps paths.ml.in)
+ (mode fallback)
+ (action
+  (run 

[PATCH v1 6/7] tools/ocaml/libs/xb: hide type of Xb.t

[Edwin Török]

The only user of 'xb' that I can find is in-tree oxenstored.
Other code (e.g. xenopsd) would use the mirage 'xenstore' implementation
instead, so changing the API here shouldn't require anyone to update
their code.

Hiding the type will make it easier to change the implementation
in the future without breaking code that relies on it.

No functional change.

Signed-off-by: Edwin Török 
---
 tools/ocaml/libs/xb/xb.ml   | 3 +++
 tools/ocaml/libs/xb/xb.mli  | 9 ++---
 tools/ocaml/xenstored/connection.ml | 8 ++--
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/tools/ocaml/libs/xb/xb.ml b/tools/ocaml/libs/xb/xb.ml
index 104d319d77..8404ddd8a6 100644
--- a/tools/ocaml/libs/xb/xb.ml
+++ b/tools/ocaml/libs/xb/xb.ml
@@ -196,6 +196,9 @@ let peek_output con = Queue.peek con.pkt_out
 let input_len con = Queue.length con.pkt_in
 let has_in_packet con = Queue.length con.pkt_in > 0
 let get_in_packet con = Queue.pop con.pkt_in
+let has_partial_input con = match con.partial_in with
+   | HaveHdr _ -> true
+   | NoHdr (n, _) -> n < Partial.header_size ()
 let has_more_input con =
match con.backend with
| Fd _ -> false
diff --git a/tools/ocaml/libs/xb/xb.mli b/tools/ocaml/libs/xb/xb.mli
index 3a00da6cdd..794e35bb34 100644
--- a/tools/ocaml/libs/xb/xb.mli
+++ b/tools/ocaml/libs/xb/xb.mli
@@ -66,13 +66,7 @@ type backend_mmap = {
 type backend_fd = { fd : Unix.file_descr; }
 type backend = Fd of backend_fd | Xenmmap of backend_mmap
 type partial_buf = HaveHdr of Partial.pkt | NoHdr of int * bytes
-type t = {
-  backend : backend;
-  pkt_in : Packet.t Queue.t;
-  pkt_out : Packet.t Queue.t;
-  mutable partial_in : partial_buf;
-  mutable partial_out : string;
-}
+type t
 val init_partial_in : unit -> partial_buf
 val reconnect : t -> unit
 val queue : t -> Packet.t -> unit
@@ -97,6 +91,7 @@ val has_output : t -> bool
 val peek_output : t -> Packet.t
 val input_len : t -> int
 val has_in_packet : t -> bool
+val has_partial_input : t -> bool
 val get_in_packet : t -> Packet.t
 val has_more_input : t -> bool
 val is_selectable : t -> bool
diff --git a/tools/ocaml/xenstored/connection.ml 
b/tools/ocaml/xenstored/connection.ml
index a94d47cdc2..0ce54cd7f9 100644
--- a/tools/ocaml/xenstored/connection.ml
+++ b/tools/ocaml/xenstored/connection.ml
@@ -125,9 +125,7 @@ let get_perm con =
 let set_target con target_domid =
con.perm <- Perms.Connection.set_target (get_perm con) 
~perms:[Perms.READ; Perms.WRITE] target_domid
 
-let is_backend_mmap con = match con.xb.Xenbus.Xb.backend with
-   | Xenbus.Xb.Xenmmap _ -> true
-   | _ -> false
+let is_backend_mmap con = Xenbus.Xb.is_mmap con.xb
 
 let send_reply con tid rid ty data =
if (String.length data) > xenstore_payload_max && (is_backend_mmap con) 
then
@@ -280,9 +278,7 @@ let get_transaction con tid =
 
 let do_input con = Xenbus.Xb.input con.xb
 let has_input con = Xenbus.Xb.has_in_packet con.xb
-let has_partial_input con = match con.xb.Xenbus.Xb.partial_in with
-   | HaveHdr _ -> true
-   | NoHdr (n, _) -> n < Xenbus.Partial.header_size ()
+let has_partial_input con = Xenbus.Xb.has_partial_input con.xb
 let pop_in con = Xenbus.Xb.get_in_packet con.xb
 let has_more_input con = Xenbus.Xb.has_more_input con.xb
 
-- 
2.34.1

[PATCH v1 2/7] tools/ocaml/*/Makefile: generate paths.ml from configure

[Edwin Török]

paths.ml contains various paths known to configure,
and currently is generated via a Makefile rule.
Simplify this and generate it through configure, similar to how
oxenstored.conf is generated from oxenstored.conf.in.

This will allow to reuse the generated file more easily with Dune.

No functional change.

Signed-off-by: Edwin Török 
---
 tools/configure   | 4 +++-
 tools/configure.ac| 2 ++
 tools/ocaml/libs/xs/Makefile  | 5 -
 tools/ocaml/libs/xs/paths.ml.in   | 1 +
 tools/ocaml/xenstored/Makefile| 5 -
 tools/ocaml/xenstored/paths.ml.in | 4 
 6 files changed, 10 insertions(+), 11 deletions(-)
 create mode 100644 tools/ocaml/libs/xs/paths.ml.in
 create mode 100644 tools/ocaml/xenstored/paths.ml.in

diff --git a/tools/configure b/tools/configure
index a052c186a5..41deb7fb96 100755
--- a/tools/configure
+++ b/tools/configure
@@ -2453,7 +2453,7 @@ ac_compiler_gnu=$ac_cv_c_compiler_gnu
 
 
 
-ac_config_files="$ac_config_files ../config/Tools.mk 
hotplug/FreeBSD/rc.d/xencommons hotplug/FreeBSD/rc.d/xendriverdomain 
hotplug/Linux/init.d/sysconfig.xencommons 
hotplug/Linux/init.d/sysconfig.xendomains hotplug/Linux/init.d/xen-watchdog 
hotplug/Linux/init.d/xencommons hotplug/Linux/init.d/xendomains 
hotplug/Linux/init.d/xendriverdomain hotplug/Linux/launch-xenstore 
hotplug/Linux/vif-setup hotplug/Linux/xen-hotplug-common.sh 
hotplug/Linux/xendomains hotplug/NetBSD/rc.d/xencommons 
hotplug/NetBSD/rc.d/xendriverdomain ocaml/xenstored/oxenstored.conf"
+ac_config_files="$ac_config_files ../config/Tools.mk 
hotplug/FreeBSD/rc.d/xencommons hotplug/FreeBSD/rc.d/xendriverdomain 
hotplug/Linux/init.d/sysconfig.xencommons 
hotplug/Linux/init.d/sysconfig.xendomains hotplug/Linux/init.d/xen-watchdog 
hotplug/Linux/init.d/xencommons hotplug/Linux/init.d/xendomains 
hotplug/Linux/init.d/xendriverdomain hotplug/Linux/launch-xenstore 
hotplug/Linux/vif-setup hotplug/Linux/xen-hotplug-common.sh 
hotplug/Linux/xendomains hotplug/NetBSD/rc.d/xencommons 
hotplug/NetBSD/rc.d/xendriverdomain ocaml/libs/xs/paths.ml 
ocaml/xenstored/paths.ml ocaml/xenstored/oxenstored.conf"
 
 ac_config_headers="$ac_config_headers config.h"
 
@@ -10935,6 +10935,8 @@ do
 "hotplug/Linux/xendomains") CONFIG_FILES="$CONFIG_FILES 
hotplug/Linux/xendomains" ;;
 "hotplug/NetBSD/rc.d/xencommons") CONFIG_FILES="$CONFIG_FILES 
hotplug/NetBSD/rc.d/xencommons" ;;
 "hotplug/NetBSD/rc.d/xendriverdomain") CONFIG_FILES="$CONFIG_FILES 
hotplug/NetBSD/rc.d/xendriverdomain" ;;
+"ocaml/libs/xs/paths.ml") CONFIG_FILES="$CONFIG_FILES 
ocaml/libs/xs/paths.ml" ;;
+"ocaml/xenstored/paths.ml") CONFIG_FILES="$CONFIG_FILES 
ocaml/xenstored/paths.ml" ;;
 "ocaml/xenstored/oxenstored.conf") CONFIG_FILES="$CONFIG_FILES 
ocaml/xenstored/oxenstored.conf" ;;
 "config.h") CONFIG_HEADERS="$CONFIG_HEADERS config.h" ;;
 "hotplug/Linux/systemd/proc-xen.mount") CONFIG_FILES="$CONFIG_FILES 
hotplug/Linux/systemd/proc-xen.mount" ;;
diff --git a/tools/configure.ac b/tools/configure.ac
index 1094d896fc..32cbe6bd3c 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -21,6 +21,8 @@ hotplug/Linux/xen-hotplug-common.sh
 hotplug/Linux/xendomains
 hotplug/NetBSD/rc.d/xencommons
 hotplug/NetBSD/rc.d/xendriverdomain
+ocaml/libs/xs/paths.ml
+ocaml/xenstored/paths.ml
 ocaml/xenstored/oxenstored.conf
 ])
 AC_CONFIG_HEADERS([config.h])
diff --git a/tools/ocaml/libs/xs/Makefile b/tools/ocaml/libs/xs/Makefile
index e934bbb550..e160e6a711 100644
--- a/tools/ocaml/libs/xs/Makefile
+++ b/tools/ocaml/libs/xs/Makefile
@@ -44,8 +44,3 @@ uninstall:
$(OCAMLFIND) remove -destdir $(OCAMLDESTDIR) xenstore
 
 include $(OCAML_TOPLEVEL)/Makefile.rules
-
-genpath-target = $(call buildmakevars2module,paths.ml)
-$(eval $(genpath-target))
-
-GENERATED_FILES += paths.ml
diff --git a/tools/ocaml/libs/xs/paths.ml.in b/tools/ocaml/libs/xs/paths.ml.in
new file mode 100644
index 00..c067f8d012
--- /dev/null
+++ b/tools/ocaml/libs/xs/paths.ml.in
@@ -0,0 +1 @@
+let xen_run_stored = "@XEN_RUN_STORED@"
diff --git a/tools/ocaml/xenstored/Makefile b/tools/ocaml/xenstored/Makefile
index 0b5711b507..6f7333926e 100644
--- a/tools/ocaml/xenstored/Makefile
+++ b/tools/ocaml/xenstored/Makefile
@@ -93,8 +93,3 @@ uninstall:
rm -f $(DESTDIR)$(sbindir)/oxenstored
 
 include $(OCAML_TOPLEVEL)/Makefile.rules
-
-genpath-target = $(call buildmakevars2module,paths.ml)
-$(eval $(genpath-target))
-
-GENERATED_FILES += paths.ml
diff --git a/tools/ocaml/xenstored/paths.ml.in 
b/tools/ocaml/xenstored/paths.ml.in
new file mode 100644
index 00..37949dc8f3
--- /dev/null
+++ b/tools/ocaml/xenstored/paths.ml.in
@@ -0,0 +1,4 @@
+let xen_log_dir = "@XEN_LOG_DIR@"
+let xen_config_dir = "@XEN_CONFIG_DIR@"
+let xen_run_dir = "@XEN_RUN_DIR@"
+let xen_run_stored = "@XEN_RUN_STORED@"
-- 
2.34.1

[PATCH v1 7/7] tools/ocaml/libs/eventchn: do not leak event channels and OCaml 5.0 compat

[Edwin Török]

Add a finalizer on the event channel value, so that it calls
`xenevtchn_close` when the value would be GCed.

In practice oxenstored seems to be the only user of this,
and it creates a single global event channel only,
but freeing this could still be useful when run with OCAMLRUNPARAM=c

The code was previously casting a C pointer to an OCaml value,
which should be avoided: OCaml 5.0 won't support it.
(all "naked" C pointers must be wrapped inside an OCaml value,
 either an Abstract tag, or Nativeint, see the manual
 https://ocaml.org/manual/intfc.html#ss:c-outside-head)

Signed-off-by: Edwin Török 
---
 tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 29 +--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c 
b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
index f889a7a2e4..c0d57e2954 100644
--- a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
+++ b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
@@ -33,7 +33,30 @@
 #include 
 #include 
 
-#define _H(__h) ((xenevtchn_handle *)(__h))
+/* We want to close the event channel when it is no longer in use,
+   which can only be done safely with a finalizer.
+   Event channels are typically long lived, so we don't need tighter control 
over resource deallocation.
+   Use a custom block
+*/
+
+/* Access the xenevtchn_t* part of the OCaml custom block */
+#define _H(__h) (*((xenevtchn_handle**)Data_custom_val(__h)))
+
+static void stub_evtchn_finalize(value v)
+{
+   /* docs say to not use any CAMLparam* macros here */
+   xenevtchn_close(_H(v));
+}
+
+static struct custom_operations xenevtchn_ops = {
+   "xenevtchn",
+   stub_evtchn_finalize,
+   custom_compare_default, /* raises Failure, cannot compare */
+   custom_hash_default, /* ignored */
+   custom_serialize_default, /* raises Failure, can't serialize */
+   custom_deserialize_default, /* raises Failure, can't deserialize */
+   custom_compare_ext_default /* raises Failure */
+};
 
 CAMLprim value stub_eventchn_init(void)
 {
@@ -48,7 +71,9 @@ CAMLprim value stub_eventchn_init(void)
if (xce == NULL)
caml_failwith("open failed");
 
-   result = (value)xce;
+   /* contains file descriptors, trigger full GC at least every 128 
allocations */
+   result = caml_alloc_custom(_ops, sizeof(xce), 1, 128);
+   _H(result) = xce;
CAMLreturn(result);
 }
 
-- 
2.34.1

[PATCH v1 0/7] tools/ocaml code and build cleanups

[Edwin Török]

Various OCaml code cleanups to make building and working on Oxenstored easier,
including compatibility with newer language versions.
This does not yet change the minimum version of OCaml.

A version of this series in a git repository is publicly available at:
https://github.com/edwintorok/xen.git
https://github.com/edwintorok/xen/compare/private/edvint/public?expand=1

Edwin Török (7):
  tools/ocaml/Makefile: do not run ocamldep during make clean
  tools/ocaml/*/Makefile: generate paths.ml from configure
  tools/ocaml/*/dune: dune based build system
  tools/ocaml: Makefile to drive dune
  tools/ocaml: fix compiler warnings
  tools/ocaml/libs/xb: hide type of Xb.t
  tools/ocaml/libs/eventchn: do not leak event channels and OCaml 5.0
compat

 Makefile  |  5 ++
 tools/.gitignore  |  7 ++
 tools/configure   |  4 +-
 tools/configure.ac|  2 +
 tools/dune|  5 ++
 tools/dune-project|  1 +
 tools/ocaml/Makefile.dune | 88 +++
 tools/ocaml/Makefile.rules|  2 +
 tools/ocaml/dune-project  | 27 ++
 tools/ocaml/dune-workspace.dev.in |  2 +
 tools/ocaml/dune-workspace.in | 18 
 tools/ocaml/libs/eventchn/dune| 11 +++
 tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 29 +-
 tools/ocaml/libs/mmap/dune|  9 ++
 tools/ocaml/libs/xb/dune  | 10 +++
 tools/ocaml/libs/xb/xb.ml |  3 +
 tools/ocaml/libs/xb/xb.mli|  9 +-
 tools/ocaml/libs/xc/dune  | 16 
 tools/ocaml/libs/xs/Makefile  |  5 --
 tools/ocaml/libs/xs/dune  | 15 
 tools/ocaml/libs/xs/paths.ml.in   |  1 +
 tools/ocaml/xenstored/Makefile|  5 --
 tools/ocaml/xenstored/connection.ml   | 10 +--
 tools/ocaml/xenstored/dune| 51 +++
 tools/ocaml/xenstored/paths.ml.in |  4 +
 tools/ocaml/xenstored/process.ml  |  5 +-
 26 files changed, 315 insertions(+), 29 deletions(-)
 create mode 100644 tools/.gitignore
 create mode 100644 tools/dune
 create mode 100644 tools/dune-project
 create mode 100644 tools/ocaml/Makefile.dune
 create mode 100644 tools/ocaml/dune-project
 create mode 100644 tools/ocaml/dune-workspace.dev.in
 create mode 100644 tools/ocaml/dune-workspace.in
 create mode 100644 tools/ocaml/libs/eventchn/dune
 create mode 100644 tools/ocaml/libs/mmap/dune
 create mode 100644 tools/ocaml/libs/xb/dune
 create mode 100644 tools/ocaml/libs/xc/dune
 create mode 100644 tools/ocaml/libs/xs/dune
 create mode 100644 tools/ocaml/libs/xs/paths.ml.in
 create mode 100644 tools/ocaml/xenstored/dune
 create mode 100644 tools/ocaml/xenstored/paths.ml.in

-- 
2.34.1

Re: [xen-unstable-smoke bisection] complete build-amd64-libvirt

[Julien Grall]

Hi Jan,

On 29/07/2022 07:22, Jan Beulich wrote:

On 29.07.2022 03:04, osstest service owner wrote:

branch xen-unstable-smoke
xenbranch xen-unstable-smoke
job build-amd64-libvirt
testid libvirt-build

Tree: libvirt git://xenbits.xen.org/libvirt.git
Tree: libvirt_keycodemapdb https://gitlab.com/keycodemap/keycodemapdb.git
Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
Tree: qemuu git://xenbits.xen.org/qemu-xen.git
Tree: xen git://xenbits.xen.org/xen.git

*** Found and reproduced problem changeset ***

   Bug is in tree:  xen git://xenbits.xen.org/xen.git
   Bug introduced:  66dd1c62b2a3c707bd5c55750d10a8223fbd577f
   Bug not present: f732240fd3bac25116151db5ddeb7203b62e85ce
   Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/171909/


   commit 66dd1c62b2a3c707bd5c55750d10a8223fbd577f
   Author: Oleksandr Tyshchenko 
   Date:   Fri Jul 15 22:20:24 2022 +0300
   
   libxl: Add support for Virtio disk configuration


Just in case you didn't notice it: Something's wrong here. I didn't look
at the details at all. Please advise whether a fix will soon arrive or
whether we should revert for the time being.


We had discussion on IRC about this today. This is an issue in libvirt 
rather than Xen. So I think a revert is not warrant here.


Instead, it was suggested to force push because it is going to take some 
times to fix libvirt (see more below).


Oleksandr already sent a patch to fix libvirt [1]. The problem is even 
if this is accepted, our testing branch for libvirt is 2 years behind 
because they switched to Meson and Osstest has not been adapted to the 
new build system.


Anthony kindly offered to update Osstest.

Regarding force pushing, I am waiting for the Osstest result to confirm 
that only the libvirt tests are failing in staging (we already have the 
results for smoke). So my plan is to force push on Monday.


Please let me know on Monday morning if you have some concerns with this 
approach.


Cheers,

[1] 
https://lore.kernel.org/xen-devel/20220729155024.3327364-1-olekst...@gmail.com/

--
Julien Grall

[xen-unstable-smoke test] 171919: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-amd64-libvirt   6 libvirt-buildfail REGR. vs. 171884

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt  1 build-check(1)   blocked  n/a
 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  062790aca6b1faea62c9ed2737c3791efb0d0f4c
baseline version:
 xen  f732240fd3bac25116151db5ddeb7203b62e85ce

Last test of basis   171884  2022-07-27 12:03:31 Z2 days
Failing since171899  2022-07-28 19:01:47 Z0 days6 attempts
Testing same since   171917  2022-07-29 10:03:07 Z0 days2 attempts


People who touched revisions under test:
  Andrew Cooper 
  Daniel P. Smith 
  George Dunlap 
  Jan Beulich 
  Jiamei Xie 
  Julien Grall 
  Oleksandr Tyshchenko 
  Stefano Stabellini 
  Xenia Ragiadakou 

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



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


Not pushing.


commit 062790aca6b1faea62c9ed2737c3791efb0d0f4c
Author: Xenia Ragiadakou 
Date:   Fri Jul 29 08:51:31 2022 +0200

arm/atomic: fix MISRA C 2012 Rule 20.7 violation

The macro parameter 'p' is used as an expression and needs to be enclosed in
parentheses.

Signed-off-by: Xenia Ragiadakou 
Reviewed-by: Stefano Stabellini 

commit 124f138b37d595294b3100349e26ffb3f1df7b13
Author: Xenia Ragiadakou 
Date:   Fri Jul 29 08:50:58 2022 +0200

xsm/dummy: fix MISRA C 2012 Directive 4.10 violation

Protect header file from being included more than once by adding ifndef 
guard.

Signed-off-by: Xenia Ragiadakou 
Reviewed-by: Luca Fancellu 
Acked-by: Daniel P. Smith 

commit 9ff3231f955cee4d62c7be6a03d061c037d7ca69
Author: Jan Beulich 
Date:   Fri Jul 29 08:50:25 2022 +0200

x86/shadow: drop CONFIG_HVM conditionals from sh_update_cr3()

Now that we're not building multi.c anymore for 2 and 3 guest levels
when !HVM, there's no point in having these conditionals anymore. (As
somewhat a special case, the last of the removed conditionals really
builds on shadow_mode_external() always returning false when !HVM.) This
way the code becomes a tiny bit more readable.

Signed-off-by: Jan Beulich 
Acked-by: Andrew Cooper 

commit 5b04fe78646a8222626996113c9d1e598cb84831
Author: Jan Beulich 
Date:   Fri Jul 29 08:49:48 2022 +0200

x86/shadow: don't open-code shadow_remove_all_shadows()

Let's use the existing inline wrapper instead of repeating respective
commentary at every site.

Signed-off-by: Jan Beulich 
Acked-by: Andrew Cooper 

commit 8a3b89e4307da260675483bb86fc06cc62ed7c08
Author: Jan Beulich 
Date:   Fri Jul 29 08:49:06 2022 +0200

x86/shadow: exclude HVM-only code from sh_remove_shadows() when !HVM

In my (debug) build this amounts to well over 500 bytes of dead code.

Signed-off-by: Jan Beulich 
Acked-by: Andrew Cooper 

commit 3629759626ac7201a670a8a2d4d4a536e7443575
Author: Jan Beulich 
Date:   Fri Jul 29 08:48:26 2022 +0200

x86/shadow: properly handle get_page() failing

We should not blindly (in a release build) insert the new entry in the
hash if a reference to the guest page cannot be obtained, or else an
excess reference would be put when removing the hash entry again. Crash
the domain 

[qemu-mainline test] 171912: tolerable FAIL - PUSHED

[osstest service owner]

flight 171912 qemu-mainline real [real]
flight 171922 qemu-mainline real-retest [real]
http://logs.test-lab.xenproject.org/osstest/logs/171912/
http://logs.test-lab.xenproject.org/osstest/logs/171922/

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-amd64-xl-multivcpu 12 debian-install fail pass in 171922-retest
 test-amd64-amd64-xl-qcow2 21 guest-start/debian.repeat fail pass in 
171922-retest
 test-arm64-arm64-libvirt-raw 12 debian-di-install   fail pass in 171922-retest
 test-amd64-i386-xl-vhd 21 guest-start/debian.repeat fail pass in 171922-retest

Regressions which are regarded as allowable (not blocking):
 test-armhf-armhf-xl-rtds18 guest-start/debian.repeat fail REGR. vs. 171900

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

Re: [PATCH V7 01/11] xen/pci: arm: add stub for is_memory_hole

[Oleksandr]

Hello Rahul


On 19.07.22 20:42, Oleksandr Tyshchenko wrote:

From: Oleksandr Andrushchenko 

Add a stub for is_memory_hole which is required for PCI passthrough
on Arm.

Signed-off-by: Oleksandr Andrushchenko 
---
OT: It looks like the discussion got stuck. As I understand this
patch is not immediately needed in the context of current series
as PCI passthrough is not enabled on Arm at the moment. So the patch
could be added later on, but it is needed to allow PCI passthrough
to be built on Arm for those who want to test it.

Copy here some context provided by Julien:

Here a summary of the discussion (+ some my follow-up thoughts):

is_memory_hole() was recently introduced on x86 (see commit 75cc460a1b8c
"xen/pci: detect when BARs are not suitably positioned") to check
whether the BAR are positioned outside of a valid memory range. This was
introduced to work-around quirky firmware.

In theory, this could also happen on Arm. In practice, this may not
happen but it sounds better to sanity check that the BAR contains
"valid" I/O range.

On x86, this is implemented by checking the region is not described is
in the e820. IIUC, on Arm, the BARs have to be positioned in pre-defined
ranges. So I think it would be possible to implement is_memory_hole() by
going through the list of hostbridges and check the ranges.

But first, I'd like to confirm my understanding with Rahul, and others.



May I please ask about your opinion on that?




If we were going to go this route, I would also rename the function to
be better match what it is doing (i.e. it checks the BAR is correctly
placed). As a potentially optimization/hardening for Arm, we could pass
the hostbridge so we don't have to walk all of them.
---
  xen/arch/arm/mm.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 009b8cd9ef..bb34b97eb5 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1708,6 +1708,12 @@ unsigned long get_upper_mfn_bound(void)
  return max_page - 1;
  }
  
+bool is_memory_hole(mfn_t start, mfn_t end)

+{
+/* TODO: this needs to be properly implemented. */
+return true;
+}
+
  /*
   * Local variables:
   * mode: C


--
Regards,

Oleksandr Tyshchenko

Re: [PATCH V7 10/11] xen/arm: translate virtual PCI bus topology for guests

[Oleksandr]

On 29.07.22 09:06, Jan Beulich wrote:

Hello Jan


On 28.07.2022 18:35, Oleksandr wrote:

On 28.07.22 10:15, Jan Beulich wrote:

On 27.07.2022 21:39, Oleksandr wrote:

On 27.07.22 20:54, Oleksandr wrote:

On 26.07.22 18:16, Jan Beulich wrote:

On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:

--- a/xen/arch/arm/vpci.c
+++ b/xen/arch/arm/vpci.c
@@ -41,6 +41,16 @@ static int vpci_mmio_read(struct vcpu *v,
mmio_info_t *info,
    /* data is needed to prevent a pointer cast on 32bit */
    unsigned long data;
    +    /*
+ * For the passed through devices we need to map their virtual
SBDF
+ * to the physical PCI device being passed through.
+ */
+    if ( !bridge && !vpci_translate_virtual_device(v->domain, ) )
+    {
+    *r = ~0ul;
+    return 1;
+    }

I'm probably simply lacking specific Arm-side knowledge, but it strikes
me as odd that the need for translation would be dependent upon
"bridge".

I am afraid I cannot answer immediately.

I will analyze that question and provide an answer later on.

Well, most likely that "valid" bridge pointer here is just used as an
indicator of hwdom currently, so no need to perform virt->phys
translation for sbdf.

You can see that domain_vpci_init() passes a valid value for hwdom and
NULL for other domains when setting up vpci_mmio* callbacks.

Oh, I see.


Alternatively, I guess we could use "!is_hardware_domain(v->domain)"
instead of "!bridge" in the first part of that check. Shall I?

Maybe simply add a comment? Surely checking "bridge" is cheaper than
using is_hardware_domain(), so I can see the benefit. But the larger
arm/vpci.c grows, the less obvious the connection will be without a
comment.


Agree the connection is worth a comment ...




   (Instead of a comment, an alternative may be a suitable
assertion, which then documents the connection at the same time, e.g.
ASSERT(!bridge == !is_hardware_domain(v->domain)). But that won't be
possible in e.g. vpci_sbdf_from_gpa(), where apparently a similar
assumption is being made.)


     ... or indeed to put such ASSERT _before_ vpci_sbdf_from_gpa().

This will cover assumption being made in both places.


diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
index a9fc5817f9..1d4b1ef39e 100644
--- a/xen/arch/arm/vpci.c
+++ b/xen/arch/arm/vpci.c
@@ -37,10 +37,24 @@ static int vpci_mmio_read(struct vcpu *v,
mmio_info_t *info,
     register_t *r, void *p)
   {
   struct pci_host_bridge *bridge = p;
-    pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
+    pci_sbdf_t sbdf;
   /* data is needed to prevent a pointer cast on 32bit */
   unsigned long data;

+    ASSERT(!bridge == !is_hardware_domain(v->domain));
+
+    sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
+
+    /*
+ * For the passed through devices we need to map their virtual SBDF
+ * to the physical PCI device being passed through.
+ */
+    if ( !bridge && !vpci_translate_virtual_device(v->domain, ) )
+    {
+    *r = ~0ul;
+    return 1;
+    }
+
   if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
   1U << info->dabt.size, ) )
   {
@@ -57,7 +71,18 @@ static int vpci_mmio_write(struct vcpu *v,
mmio_info_t *info,
      register_t r, void *p)
   {
   struct pci_host_bridge *bridge = p;
-    pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
+    pci_sbdf_t sbdf;
+
+    ASSERT(!bridge == !is_hardware_domain(v->domain));
+
+    sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
+
+    /*
+ * For the passed through devices we need to map their virtual SBDF
+ * to the physical PCI device being passed through.
+ */
+    if ( !bridge && !vpci_translate_virtual_device(v->domain, ) )
+    return 1;

   return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa),
      1U << info->dabt.size, r);
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index d4601ecf9b..fc2c51dc3e 100644


Any preference here?


Personally, I think that such ASSERT will better explain the connection
than the comment will do.

Indeed I'd also prefer ASSERT()s being put there.


good



  But my opinion is
secondary here, as I'm not a maintainer of this code.



sure, let's see what the Arm maintainers will say




Jan


--
Regards,

Oleksandr Tyshchenko

Re: [PATCH v3] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Julien Grall]

Hi,

On 29/07/2022 16:47, Xenia Ragiadakou wrote:

On 7/29/22 18:15, Julien Grall wrote:

Hi Xenia,

On 29/07/2022 15:03, Xenia Ragiadakou wrote:


On 7/29/22 16:41, Bertrand Marquis wrote:

Hi Xenia,

On 29 Jul 2022, at 07:31, Xenia Ragiadakou  
wrote:


Hi Jan,

On 7/29/22 09:26, Jan Beulich wrote:

On 28.07.2022 18:21, Xenia Ragiadakou wrote:

--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -63,7 +63,7 @@ static void do_idle(void)
  rcu_idle_exit(cpu);
  }
  -void idle_loop(void)
+static void idle_loop(void)

While you're adding "noreturn" below, shouldn't this one be marked so
as well? Preferably with the addition:
Reviewed-by: Jan Beulich 


Yes, but I was not sure if this should go in this patch or in a 
separate one.


As you modify the function to make it static, I think it is ok to 
also add the noreturn in the same patch.


With that done, you can add my:
Reviewed-by: Bertrand Marquis 

Cheers
Bertrand


I consider this change unrelated to the patch. I think it is a bad 
practice to squash unrelated changes in a single patch. Also, I do 
not think it is unfair to be obliged to make it in order for the 
patch to be accepted.
I could have taken the opportunity to fix this in the same patch but 
I decided to not take it.


In general, I don't like having multiple changes within a patch. 
However, here this is a consistency problem. You are modifying the 3 
prototypes (well one is technically a declaration) and it reads odd 
that 2 are using noreturn but not the other one.




The patch adds the 2 function declarations, it does not modify them.


Fair enough, you are adding 2 declarations and modifying one. This 
doesn't change the inconsistency problem though.



Since they do not return, they are declared noreturn.
If the function idle_loop was not declared noreturn, although it should 
have been, for me is a completely different issue.

[...]

I do not agree with you saying that the patch introduced this 
inconsistency. The function idle_loop should have been declared noreturn 
in the first place.


I think everyone involved in the discussion agrees that idle_loop() 
should have been marked as noreturn from the start.


And we all agree that this needs to be fixed. So I don't think this is 
particularly useful to spend time arguing on whether this needs to be 
handled within or separately. 3 of the reviewers agree that this should 
be preferably added here. So...


If you would like to fix this in the current patch, 
it is up to you.


... I will commit it with the change on the next swipe.

Cheers,

--
Julien Grall

Re: [xen-unstable-smoke bisection] complete build-amd64-libvirt

[Oleksandr]

On 29.07.22 13:29, Julien Grall wrote:

Hi Oleksandr,



Hello Julien




On 29/07/2022 11:25, Oleksandr Tyshchenko wrote:


On 29.07.22 12:08, Julien Grall wrote:


Hello Julien


(+ Anthony)

Hi,

On 29/07/2022 07:48, Oleksandr Tyshchenko wrote:


On 29.07.22 09:22, Jan Beulich wrote:

Hello Jan


On 29.07.2022 03:04, osstest service owner wrote:

branch xen-unstable-smoke
xenbranch xen-unstable-smoke
job build-amd64-libvirt
testid libvirt-build

Tree: libvirt git://xenbits.xen.org/libvirt.git
Tree: libvirt_keycodemapdb
https://urldefense.com/v3/__https://gitlab.com/keycodemap/keycodemapdb.git__;!!GF_29dbcQIUBPA!0s_nyAgds977dw0dGPgFJGkIaBiKiXH3nR11Ni6gGjN5gQmB0DEhKrm5SUX4R0WhK8YkQemR6RVhiojpzij9yLM$ 


[gitlab[.]com]
Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
Tree: qemuu git://xenbits.xen.org/qemu-xen.git
Tree: xen git://xenbits.xen.org/xen.git

*** Found and reproduced problem changeset ***

 Bug is in tree:  xen git://xenbits.xen.org/xen.git
 Bug introduced: 66dd1c62b2a3c707bd5c55750d10a8223fbd577f
 Bug not present: f732240fd3bac25116151db5ddeb7203b62e85ce
 Last fail repro:
https://urldefense.com/v3/__http://logs.test-lab.xenproject.org/osstest/logs/171909/__;!!GF_29dbcQIUBPA!0s_nyAgds977dw0dGPgFJGkIaBiKiXH3nR11Ni6gGjN5gQmB0DEhKrm5SUX4R0WhK8YkQemR6RVhiojpmYABJkc$ 


[logs[.]test-lab[.]xenproject[.]org]


 commit 66dd1c62b2a3c707bd5c55750d10a8223fbd577f
 Author: Oleksandr Tyshchenko 
 Date:   Fri Jul 15 22:20:24 2022 +0300
     libxl: Add support for Virtio disk configuration

Just in case you didn't notice it: Something's wrong here. I didn't
look
at the details at all. Please advise whether a fix will soon 
arrive or

whether we should revert for the time being.


Sorry for the breakage. At the moment I have no idea what is wrong 
here,


 From the build log:

../../src/libxl/xen_xl.c: In function 'xenParseXLDisk':
../../src/libxl/xen_xl.c:779:17: error: enumeration value
'LIBXL_DISK_BACKEND_STANDALONE' not handled in switch
[-Werror=switch-enum]
  switch (libxldisk->backend) {
  ^~

The switch contains a default branch [1], so I am a bit puzzled why
GCC is not happy here.


Libvirt seems to compiled with -Wswitch-enum

And https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html says the
following:

"-Wswitch-enum

Warn whenever a switch statement has an index of enumerated type and
lacks a case for one or more of the named codes of that enumeration.
case labels outside the enumeration range also provoke warnings when
this option is used. The only difference between -Wswitch and this
option is that this option gives a warning about an omitted enumeration
code even if there is a default label."


Thanks for digging! That's explained the error. I still don't think we 
can solve the error in libxl.


So I would suggest to involve the libvirt folks to know how they want 
to solve the issue.



Already pushed an immediate fix with detailed description:

https://lore.kernel.org/xen-devel/20220729155024.3327364-1-olekst...@gmail.com/




Cheers,


--
Regards,

Oleksandr Tyshchenko

[libvirt PATCH] libxl: Fix build with recent Xen that introduces new disk backend type

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko 

Xen toolstack has gained basic Virtio support recently which becides
adding various virtio related stuff introduces new disk backend type
LIBXL_DISK_BACKEND_STANDALONE [1].

Unfortunately, this caused a regression in libvirt build with Xen support
enabled, reported by the osstest today [2]:

CC   libxl/libvirt_driver_libxl_impl_la-xen_xl.lo
../../src/libxl/xen_xl.c: In function 'xenParseXLDisk':
../../src/libxl/xen_xl.c:779:17: error: enumeration value 
'LIBXL_DISK_BACKEND_STANDALONE'
   not handled in switch [-Werror=switch-enum]
 switch (libxldisk->backend) {
 ^~
cc1: all warnings being treated as errors

The interesting fact is that switch already has a default branch (which ought
to cover such new addition), but the error is triggered as -Wswitch-enum
gives a warning about an omitted enumeration code even if there is a default
label.

Also there is a similar issue in libxlUpdateDiskDef() which I have reproduced
after fixing the first one, but it that case the corresponding switch doesn't
have a default branch.

Fix both issues by inserting required enumeration item to make the compiler
happy and adding ifdef guard to be able to build against old Xen libraries
as well (without LIBXL_HAVE_DEVICE_DISK_SPECIFICATION). Also add a default
branch to switch in libxlUpdateDiskDef().

Please note, that current patch doesn't implement the proper handling of
LIBXL_DISK_BACKEND_STANDALONE and friends, it is just intended to fix
the regression immediately to unblock the osstest.  Also it worth mentioning
that current patch won't solve the possible additions in the future.

[1] 
https://lore.kernel.org/xen-devel/20220716163745.28712-1-olekst...@gmail.com/
[2] 
https://lore.kernel.org/xen-devel/e1oheqo-0008ga...@osstest.test-lab.xenproject.org/

Signed-off-by: Oleksandr Tyshchenko 
---
Cc: Julien Grall 
Cc: Anthony PERARD 
Cc: Michal Privoznik 

Please note, the patch is tested on:
https://xenbits.xen.org/gitweb/?p=libvirt.git;a=shortlog;h=refs/heads/xen-tested-master
but should work on the master as well (as the same code is present here).
---
 src/libxl/libxl_conf.c | 4 
 src/libxl/xen_xl.c | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index aa3d7925ec..526f0b2b08 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1240,6 +1240,10 @@ libxlUpdateDiskDef(virDomainDiskDef *l_disk, 
libxl_device_disk *x_disk)
 driver = "phy";
 break;
 case LIBXL_DISK_BACKEND_UNKNOWN:
+#ifdef LIBXL_HAVE_DEVICE_DISK_SPECIFICATION
+case LIBXL_DISK_BACKEND_STANDALONE:
+#endif
+default:
 break;
 }
 if (driver)
diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
index 4de4e3140f..6919325623 100644
--- a/src/libxl/xen_xl.c
+++ b/src/libxl/xen_xl.c
@@ -715,6 +715,9 @@ xenParseXLDisk(virConf *conf, virDomainDef *def)
 virDomainDiskSetDriver(disk, "phy");
 virDomainDiskSetType(disk, VIR_STORAGE_TYPE_BLOCK);
 break;
+#ifdef LIBXL_HAVE_DEVICE_DISK_SPECIFICATION
+case LIBXL_DISK_BACKEND_STANDALONE:
+#endif
 default:
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk backend not supported: %s"),
-- 
2.25.1

Re: [PATCH v3] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Xenia Ragiadakou]

Hi Julien,

Hi Julien,

On 7/29/22 18:15, Julien Grall wrote:

Hi Xenia,

On 29/07/2022 15:03, Xenia Ragiadakou wrote:


On 7/29/22 16:41, Bertrand Marquis wrote:

Hi Xenia,

On 29 Jul 2022, at 07:31, Xenia Ragiadakou  
wrote:


Hi Jan,

On 7/29/22 09:26, Jan Beulich wrote:

On 28.07.2022 18:21, Xenia Ragiadakou wrote:

--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -63,7 +63,7 @@ static void do_idle(void)
  rcu_idle_exit(cpu);
  }
  -void idle_loop(void)
+static void idle_loop(void)

While you're adding "noreturn" below, shouldn't this one be marked so
as well? Preferably with the addition:
Reviewed-by: Jan Beulich 


Yes, but I was not sure if this should go in this patch or in a 
separate one.


As you modify the function to make it static, I think it is ok to 
also add the noreturn in the same patch.


With that done, you can add my:
Reviewed-by: Bertrand Marquis 

Cheers
Bertrand


I consider this change unrelated to the patch. I think it is a bad 
practice to squash unrelated changes in a single patch. Also, I do not 
think it is unfair to be obliged to make it in order for the patch to 
be accepted.
I could have taken the opportunity to fix this in the same patch but I 
decided to not take it.


In general, I don't like having multiple changes within a patch. 
However, here this is a consistency problem. You are modifying the 3 
prototypes (well one is technically a declaration) and it reads odd that 
2 are using noreturn but not the other one.




The patch adds the 2 function declarations, it does not modify them. 
Since they do not return, they are declared noreturn.
If the function idle_loop was not declared noreturn, although it should 
have been, for me is a completely different issue.


I would actually argue that if this patch goes in like that, then the 
commit message ought to explain why there is a lack of consistency.




I do not agree with you saying that the patch introduced this 
inconsistency. The function idle_loop should have been declared noreturn 
in the first place. If you would like to fix this in the current patch, 
it is up to you.


Anyway, I agree with Bertrand that it would be preferable to add 
noreturn to the declaration of idle_loop() in this patch.


To avoid a round trip, I would be OK to handle on commit.

Cheers,



--
Xenia

Re: [PATCH v3] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Julien Grall]

Hi Xenia,

On 29/07/2022 15:03, Xenia Ragiadakou wrote:


On 7/29/22 16:41, Bertrand Marquis wrote:

Hi Xenia,

On 29 Jul 2022, at 07:31, Xenia Ragiadakou  
wrote:


Hi Jan,

On 7/29/22 09:26, Jan Beulich wrote:

On 28.07.2022 18:21, Xenia Ragiadakou wrote:

--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -63,7 +63,7 @@ static void do_idle(void)
  rcu_idle_exit(cpu);
  }
  -void idle_loop(void)
+static void idle_loop(void)

While you're adding "noreturn" below, shouldn't this one be marked so
as well? Preferably with the addition:
Reviewed-by: Jan Beulich 


Yes, but I was not sure if this should go in this patch or in a 
separate one.


As you modify the function to make it static, I think it is ok to also 
add the noreturn in the same patch.


With that done, you can add my:
Reviewed-by: Bertrand Marquis 

Cheers
Bertrand


I consider this change unrelated to the patch. I think it is a bad 
practice to squash unrelated changes in a single patch. Also, I do not 
think it is unfair to be obliged to make it in order for the patch to be 
accepted.
I could have taken the opportunity to fix this in the same patch but I 
decided to not take it.


In general, I don't like having multiple changes within a patch. 
However, here this is a consistency problem. You are modifying the 3 
prototypes (well one is technically a declaration) and it reads odd that 
2 are using noreturn but not the other one.


I would actually argue that if this patch goes in like that, then the 
commit message ought to explain why there is a lack of consistency.


Anyway, I agree with Bertrand that it would be preferable to add 
noreturn to the declaration of idle_loop() in this patch.


To avoid a round trip, I would be OK to handle on commit.

Cheers,

--
Julien Grall

Re: [PATCH 04/36] cpuidle,intel_idle: Fix CPUIDLE_FLAG_IRQ_ENABLE

[Rafael J. Wysocki]

On Fri, Jul 29, 2022 at 12:25 PM Michel Lespinasse
 wrote:
>
> On Thu, Jul 28, 2022 at 10:20:53AM -0700, Paul E. McKenney wrote:
> > On Mon, Jul 25, 2022 at 12:43:06PM -0700, Michel Lespinasse wrote:
> > > On Wed, Jun 08, 2022 at 04:27:27PM +0200, Peter Zijlstra wrote:
> > > > Commit c227233ad64c ("intel_idle: enable interrupts before C1 on
> > > > Xeons") wrecked intel_idle in two ways:
> > > >
> > > >  - must not have tracing in idle functions
> > > >  - must return with IRQs disabled
> > > >
> > > > Additionally, it added a branch for no good reason.
> > > >
> > > > Fixes: c227233ad64c ("intel_idle: enable interrupts before C1 on Xeons")
> > > > Signed-off-by: Peter Zijlstra (Intel) 
> > >
> > > After this change was introduced, I am seeing "WARNING: suspicious RCU
> > > usage" when booting a kernel with debug options compiled in. Please
> > > see the attached dmesg output. The issue starts with commit 32d4fd5751ea
> > > and is still present in v5.19-rc8.
> > >
> > > I'm not sure, is this too late to fix or revert in v5.19 final ?
> >
> > I finally got a chance to take a quick look at this.
> >
> > The rcu_eqs_exit() function is making a lockdep complaint about
> > being invoked with interrupts enabled.  This function is called from
> > rcu_idle_exit(), which is an expected code path from cpuidle_enter_state()
> > via its call to rcu_idle_exit().  Except that rcu_idle_exit() disables
> > interrupts before invoking rcu_eqs_exit().
> >
> > The only other call to rcu_idle_exit() does not disable interrupts,
> > but it is via rcu_user_exit(), which would be a very odd choice for
> > cpuidle_enter_state().
> >
> > It seems unlikely, but it might be that it is the use of local_irq_save()
> > instead of raw_local_irq_save() within rcu_idle_exit() that is causing
> > the trouble.  If this is the case, then the commit shown below would
> > help.  Note that this commit removes the warning from lockdep, so it
> > is necessary to build the kernel with CONFIG_RCU_EQS_DEBUG=y to enable
> > equivalent debugging.
> >
> > Could you please try your test with the -rce commit shown below applied?
>
> Thanks for looking into it.
>
> After checking out Peter's commit 32d4fd5751ea,
> cherry picking your commit ed4ae5eff4b3,
> and setting CONFIG_RCU_EQS_DEBUG=y in addition of my usual debug config,
> I am now seeing this a few seconds into the boot:
>
> [3.010650] [ cut here ]
> [3.010651] WARNING: CPU: 0 PID: 0 at kernel/sched/clock.c:397 
> sched_clock_tick+0x27/0x60
> [3.010657] Modules linked in:
> [3.010660] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
> 5.19.0-rc1-test-5-g1be22fea0611 #1
> [3.010662] Hardware name: LENOVO 30BFS44D00/1036, BIOS S03KT51A 01/17/2022
> [3.010663] RIP: 0010:sched_clock_tick+0x27/0x60
> [3.010665] Code: 1f 40 00 53 eb 02 5b c3 66 90 8b 05 2f c3 40 01 85 c0 74 
> 18 65 8b 05 60 88 8f 4e 85 c0 75 0d 65 8b 05 a9 85 8f 4e 85 c0 74 02 <0f> 0b 
> e8 e2 6c 89 00 48 c7 c3 40 d5 02 00
>  89 c0 48 03 1c c5 c0 98
> [3.010667] RSP: :b2803e28 EFLAGS: 00010002
> [3.010670] RAX: 0001 RBX: c8ce7fa07060 RCX: 
> 0001
> [3.010671] RDX:  RSI: b268dd21 RDI: 
> b269ab13
> [3.010673] RBP: 0001 R08: ffc300d5 R09: 
> 0002be80
> [3.010674] R10: 03625b53183a R11: a012b802b7a4 R12: 
> b2aa9e80
> [3.010675] R13: b2aa9e00 R14: 0001 R15: 
> 
> [3.010677] FS:  () GS:a012b800() 
> knlGS:
> [3.010678] CS:  0010 DS:  ES:  CR0: 80050033
> [3.010680] CR2: a012f81ff000 CR3: 000c99612001 CR4: 
> 003706f0
> [3.010681] DR0:  DR1:  DR2: 
> 
> [3.010682] DR3:  DR6: fffe0ff0 DR7: 
> 0400
> [3.010683] Call Trace:
> [3.010685]  
> [3.010688]  cpuidle_enter_state+0xb7/0x4b0
> [3.010694]  cpuidle_enter+0x29/0x40
> [3.010697]  do_idle+0x1d4/0x210
> [3.010702]  cpu_startup_entry+0x19/0x20
> [3.010704]  rest_init+0x117/0x1a0
> [3.010708]  arch_call_rest_init+0xa/0x10
> [3.010711]  start_kernel+0x6d8/0x6ff
> [3.010716]  secondary_startup_64_no_verify+0xce/0xdb
> [3.010728]  
> [3.010729] irq event stamp: 44179
> [3.010730] hardirqs last  enabled at (44179): [] 
> asm_sysvec_apic_timer_interrupt+0x1b/0x20
> [3.010734] hardirqs last disabled at (44177): [] 
> __do_softirq+0x3f0/0x498
> [3.010736] softirqs last  enabled at (44178): [] 
> __do_softirq+0x332/0x498
> [3.010738] softirqs last disabled at (44171): [] 
> irq_exit_rcu+0xab/0xf0
> [3.010741] ---[ end trace  ]---

Can you please give this patch a go:
https://patchwork.kernel.org/project/linux-pm/patch/Yt/axpfi88new...@e126311.manchester.arm.com/
?

[PATCH 3/3] automation: qemu-smoke-arm64.sh: Fix the number of cpus in the device tree

[Xenia Ragiadakou]

Qemu VM is configured with 2 cpus but the device tree passed has only 1.
Generate a device tree with 2 cpus.

Signed-off-by: Xenia Ragiadakou 
---
I am not sure for this patch because I do not know whether the number of cpus
differs deliberately.

 automation/scripts/qemu-smoke-arm64.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/automation/scripts/qemu-smoke-arm64.sh 
b/automation/scripts/qemu-smoke-arm64.sh
index 7ac82b2278..b48a20988f 100755
--- a/automation/scripts/qemu-smoke-arm64.sh
+++ b/automation/scripts/qemu-smoke-arm64.sh
@@ -35,7 +35,7 @@ curl -fsSLO 
https://github.com/qemu/qemu/raw/v5.2.0/pc-bios/efi-virtio.rom
 ./binaries/qemu-system-aarch64 \
-machine virtualization=true \
-cpu cortex-a57 -machine type=virt \
-   -m 1024 -display none \
+   -m 1024 -smp 2 -display none \
-machine dumpdtb=binaries/virt-gicv2.dtb
 # XXX disable pl061 to avoid Linux crash
 dtc -I dtb -O dts binaries/virt-gicv2.dtb > binaries/virt-gicv2.dts
-- 
2.34.1

[PATCH 2/3] automation: qemu-smoke-arm64.sh: Rename the device tree to avoid confusion

[Xenia Ragiadakou]

Rename the device tree from virt-gicv3 to virt-gicv2 to avoid confusion
since the version of the generic interrupt controller used for this test
is the v2 and not the v3.

Signed-off-by: Xenia Ragiadakou 
---
 automation/scripts/qemu-smoke-arm64.sh | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/automation/scripts/qemu-smoke-arm64.sh 
b/automation/scripts/qemu-smoke-arm64.sh
index f483a2ffc1..7ac82b2278 100755
--- a/automation/scripts/qemu-smoke-arm64.sh
+++ b/automation/scripts/qemu-smoke-arm64.sh
@@ -36,11 +36,11 @@ curl -fsSLO 
https://github.com/qemu/qemu/raw/v5.2.0/pc-bios/efi-virtio.rom
-machine virtualization=true \
-cpu cortex-a57 -machine type=virt \
-m 1024 -display none \
-   -machine dumpdtb=binaries/virt-gicv3.dtb
+   -machine dumpdtb=binaries/virt-gicv2.dtb
 # XXX disable pl061 to avoid Linux crash
-dtc -I dtb -O dts binaries/virt-gicv3.dtb > binaries/virt-gicv3.dts
-sed 's/compatible = "arm,pl061.*/status = "disabled";/g' 
binaries/virt-gicv3.dts > binaries/virt-gicv3-edited.dts
-dtc -I dts -O dtb binaries/virt-gicv3-edited.dts > binaries/virt-gicv3.dtb
+dtc -I dtb -O dts binaries/virt-gicv2.dtb > binaries/virt-gicv2.dts
+sed 's/compatible = "arm,pl061.*/status = "disabled";/g' 
binaries/virt-gicv2.dts > binaries/virt-gicv2-edited.dts
+dtc -I dts -O dtb binaries/virt-gicv2-edited.dts > binaries/virt-gicv2.dtb
 
 
 # Busybox
@@ -73,7 +73,7 @@ cd ..
 echo 'MEMORY_START="0x4000"
 MEMORY_END="0x8000"
 
-DEVICE_TREE="virt-gicv3.dtb"
+DEVICE_TREE="virt-gicv2.dtb"
 XEN="xen"
 DOM0_KERNEL="Image"
 DOM0_RAMDISK="initrd"
-- 
2.34.1

[PATCH 0/3] automation: qemu-smoke-arm64.sh: Minor fixes to prevent confusion

[Xenia Ragiadakou]

This patch series performs some minor cleanups in qemu-smoke-arm64.sh to
prevent confusion that may arise to somebody reading the test.
More specifically, this patch series
- cleanups some stale comments
- fixes the name used for the device tree
- fixes the number of cpus in the dt to reflect the number of cpus used
in qemu

Xenia Ragiadakou (3):
  automation: qemu-smoke-arm64.sh: Remove some stale comments
  automation: qemu-smoke-arm64.sh: Rename the device tree to avoid
confusion
  automation: qemu-smoke-arm64.sh: Fix the number of cpus in the device
tree

 automation/scripts/qemu-smoke-arm64.sh | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH 1/3] automation: qemu-smoke-arm64.sh: Remove some stale comments

[Xenia Ragiadakou]

Remove comment "# Install QEMU" because qemu is not installed, it is taken
from a test-artifacts container.

Change comment "# Busybox Dom0" to "# Busybox" because busybox is not used
only for the Dom0 but also for the DomU.

Signed-off-by: Xenia Ragiadakou 
---
 automation/scripts/qemu-smoke-arm64.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/automation/scripts/qemu-smoke-arm64.sh 
b/automation/scripts/qemu-smoke-arm64.sh
index 898398196a..f483a2ffc1 100755
--- a/automation/scripts/qemu-smoke-arm64.sh
+++ b/automation/scripts/qemu-smoke-arm64.sh
@@ -21,7 +21,6 @@ fi
 "
 fi
 
-# Install QEMU
 export DEBIAN_FRONTENT=noninteractive
 apt-get -qy update
 apt-get -qy install --no-install-recommends u-boot-qemu \
@@ -44,7 +43,7 @@ sed 's/compatible = "arm,pl061.*/status = "disabled";/g' 
binaries/virt-gicv3.dts
 dtc -I dts -O dtb binaries/virt-gicv3-edited.dts > binaries/virt-gicv3.dtb
 
 
-# Busybox Dom0
+# Busybox
 mkdir -p initrd
 mkdir -p initrd/bin
 mkdir -p initrd/sbin
-- 
2.34.1

Re: [PATCH v3] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Xenia Ragiadakou]

On 7/29/22 16:41, Bertrand Marquis wrote:

Hi Xenia,


On 29 Jul 2022, at 07:31, Xenia Ragiadakou  wrote:

Hi Jan,

On 7/29/22 09:26, Jan Beulich wrote:

On 28.07.2022 18:21, Xenia Ragiadakou wrote:

--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -63,7 +63,7 @@ static void do_idle(void)
  rcu_idle_exit(cpu);
  }
  -void idle_loop(void)
+static void idle_loop(void)

While you're adding "noreturn" below, shouldn't this one be marked so
as well? Preferably with the addition:
Reviewed-by: Jan Beulich 


Yes, but I was not sure if this should go in this patch or in a separate one.


As you modify the function to make it static, I think it is ok to also add the 
noreturn in the same patch.

With that done, you can add my:
Reviewed-by: Bertrand Marquis 

Cheers
Bertrand


I consider this change unrelated to the patch. I think it is a bad 
practice to squash unrelated changes in a single patch. Also, I do not 
think it is unfair to be obliged to make it in order for the patch to be 
accepted.
I could have taken the opportunity to fix this in the same patch but I 
decided to not take it.


--
Xenia

Re: [PATCH v3] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Bertrand Marquis]

Hi Xenia,

> On 29 Jul 2022, at 07:31, Xenia Ragiadakou  wrote:
> 
> Hi Jan,
> 
> On 7/29/22 09:26, Jan Beulich wrote:
>> On 28.07.2022 18:21, Xenia Ragiadakou wrote:
>>> --- a/xen/arch/arm/domain.c
>>> +++ b/xen/arch/arm/domain.c
>>> @@ -63,7 +63,7 @@ static void do_idle(void)
>>>  rcu_idle_exit(cpu);
>>>  }
>>>  -void idle_loop(void)
>>> +static void idle_loop(void)
>> While you're adding "noreturn" below, shouldn't this one be marked so
>> as well? Preferably with the addition:
>> Reviewed-by: Jan Beulich 
> 
> Yes, but I was not sure if this should go in this patch or in a separate one.

As you modify the function to make it static, I think it is ok to also add the 
noreturn in the same patch.

With that done, you can add my:
Reviewed-by: Bertrand Marquis 

Cheers
Bertrand
> 
> -- 
> Xenia

[XEN PATCH 0/X] tools/libxl: XSA-403 follow-up

[Anthony PERARD]

Hi,

Two patches:
- one for stable branches (I've rework the XSA's patch on 4.16 so
  patch will need to be backported);
- and one patch for staging, forward porting the patch for stable
  branches.

Those patches are a rework of the patch for the stable branches available
in XSA-403. The environment variable is now in upper case, like one
would expect, and now a value of "0" as the same meaning as the variable
been absent. Also, there's a bit of documentation in `man xl`.

Thanks,

-- 
Anthony PERARD

[XEN PATCH stable-4.16] tools/libxl: env variable to signal whether disk/nic backend is trusted

[Anthony PERARD]

From: Roger Pau Monne 

Introduce support in libxl for fetching the default backend trusted
option for disk and nic devices.

Users can set LIBXL_{DISK,NIC}_BACKEND_UNTRUSTED environment variable
to notify libxl of whether the backends for disk and nic devices
should be trusted.  Such information is passed into the frontend so it
can take the appropriate measures.

This is part of XSA-403.

Signed-off-by: Roger Pau Monné 
Signed-off-by: Anthony PERARD 
---
changes:
- envvar now upper case
- documentation in xl man page
- value "0" also mean "trusted"
---
 docs/man/xl.1.pod.in  | 18 ++
 tools/libs/light/libxl_disk.c |  5 +
 tools/libs/light/libxl_nic.c  |  7 +++
 3 files changed, 30 insertions(+)

diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in
index e2176bd696..45e1430aeb 100644
--- a/docs/man/xl.1.pod.in
+++ b/docs/man/xl.1.pod.in
@@ -1946,6 +1946,24 @@ shows the decimal value. For non-linear mode, it shows 
hexadecimal value.
 
 =back
 
+=head1 ENVIRONMENT
+
+=over 4
+
+=item B
+
+Set this environment variable to "1" to suggest to the guest that the disk
+backend shouldn't be trusted. If the variable is absent or set to "0", the
+backend will be trusted.
+
+=item B
+
+Set this environment variable to "1" to suggest to the guest that the network
+backend shouldn't be trusted. If the variable is absent or set to "0", the
+backend will be trusted.
+
+=back
+
 =head1 IGNORED FOR COMPATIBILITY WITH XM
 
 xl is mostly command-line compatible with the old xm utility used with
diff --git a/tools/libs/light/libxl_disk.c b/tools/libs/light/libxl_disk.c
index 93936d0dd0..67d1cc1857 100644
--- a/tools/libs/light/libxl_disk.c
+++ b/tools/libs/light/libxl_disk.c
@@ -246,6 +246,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
 libxl_domain_config d_config;
 libxl_device_disk disk_saved;
 libxl__flock *lock = NULL;
+const char *envvar;
 
 libxl_domain_config_init(_config);
 libxl_device_disk_init(_saved);
@@ -395,6 +396,10 @@ static void device_disk_add(libxl__egc *egc, uint32_t 
domid,
 flexarray_append(front, GCSPRINTF("%d", device->devid));
 flexarray_append(front, "device-type");
 flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk");
+flexarray_append(front, "trusted");
+envvar = getenv("LIBXL_DISK_BACKEND_UNTRUSTED");
+/* Set "trusted=1" if envvar missing or is "0". */
+flexarray_append(front, !envvar || !strcmp("0", envvar) ? "1" : "0");
 
 /*
  * Old PV kernel disk frontends before 2.6.26 rely on tool stack to
diff --git a/tools/libs/light/libxl_nic.c b/tools/libs/light/libxl_nic.c
index 0b9e70c9d1..f87890d1d6 100644
--- a/tools/libs/light/libxl_nic.c
+++ b/tools/libs/light/libxl_nic.c
@@ -132,6 +132,8 @@ static int libxl__set_xenstore_nic(libxl__gc *gc, uint32_t 
domid,
flexarray_t *back, flexarray_t *front,
flexarray_t *ro_front)
 {
+const char *envvar;
+
 flexarray_grow(back, 2);
 
 if (nic->script)
@@ -255,6 +257,11 @@ static int libxl__set_xenstore_nic(libxl__gc *gc, uint32_t 
domid,
 flexarray_append(back, "hotplug-status");
 flexarray_append(back, "");
 
+flexarray_append(front, "trusted");
+envvar = getenv("LIBXL_NIC_BACKEND_UNTRUSTED");
+/* Set "trusted=1" if envvar missing or is "0". */
+flexarray_append(front, !envvar || !strcmp("0", envvar) ? "1" : "0");
+
 return 0;
 }
 
-- 
Anthony PERARD

[XEN PATCH] tools/libxl: env variable to trusted default

[Anthony PERARD]

This is a forward port of "tools/libxl: env variable to signal whether
disk/nic backend is trusted", to allow the environment variable to
still work when upgrading from 4.16 or earlier.

Introduce support in libxl for fetching the default backend trusted
option for disk and nic devices.

This is part of XSA-403.

Signed-off-by: Anthony PERARD 
---
 docs/man/xl.1.pod.in  | 24 
 tools/libs/light/libxl_disk.c |  6 +-
 tools/libs/light/libxl_nic.c  |  5 -
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in
index 101e14241d..a5a2af5df9 100644
--- a/docs/man/xl.1.pod.in
+++ b/docs/man/xl.1.pod.in
@@ -1945,6 +1945,30 @@ shows the decimal value. For non-linear mode, it shows 
hexadecimal value.
 
 =back
 
+=head1 ENVIRONMENT
+
+=over 4
+
+=item B
+
+Use B or B from L instead for a
+more fine grain setting.
+
+This environment variable allows to changed the default value of B;
+if it is set to "1", the default will be B; if the variable is
+absent or set to "0", the default will be B.
+
+=item B
+
+Use B / B from L instead for a
+more fine grain setting.
+
+This environment variable allows to changed the default value of B;
+if it is set to "1", the default will be B; if the variable is
+absent or set to "0", the default will be B.
+
+=back
+
 =head1 IGNORED FOR COMPATIBILITY WITH XM
 
 xl is mostly command-line compatible with the old xm utility used with
diff --git a/tools/libs/light/libxl_disk.c b/tools/libs/light/libxl_disk.c
index 9da2b2ed27..7564a12868 100644
--- a/tools/libs/light/libxl_disk.c
+++ b/tools/libs/light/libxl_disk.c
@@ -155,11 +155,15 @@ static int libxl__device_disk_setdefault(libxl__gc *gc, 
uint32_t domid,
  libxl_device_disk *disk, bool hotplug)
 {
 int rc;
+const char *envvar;
 
 libxl_defbool_setdefault(>discard_enable, !!disk->readwrite);
 libxl_defbool_setdefault(>colo_enable, false);
 libxl_defbool_setdefault(>colo_restore_enable, false);
-libxl_defbool_setdefault(>trusted, true);
+
+envvar = getenv("LIBXL_DISK_BACKEND_UNTRUSTED");
+/* Default to trusted if envvar missing or is "0". */
+libxl_defbool_setdefault(>trusted, !envvar || !strcmp("0", envvar));
 
 rc = libxl__resolve_domid(gc, disk->backend_domname, >backend_domid);
 if (rc < 0) return rc;
diff --git a/tools/libs/light/libxl_nic.c b/tools/libs/light/libxl_nic.c
index d6bf06fc34..ff3aede6ea 100644
--- a/tools/libs/light/libxl_nic.c
+++ b/tools/libs/light/libxl_nic.c
@@ -59,6 +59,7 @@ static int libxl__device_nic_setdefault(libxl__gc *gc, 
uint32_t domid,
 libxl_device_nic *nic, bool hotplug)
 {
 int rc;
+const char *envvar;
 
 if (!nic->mtu)
 nic->mtu = LIBXL_DEVICE_NIC_MTU_DEFAULT;
@@ -116,7 +117,9 @@ static int libxl__device_nic_setdefault(libxl__gc *gc, 
uint32_t domid,
 abort();
 }
 
-libxl_defbool_setdefault(>trusted, true);
+envvar = getenv("LIBXL_NIC_BACKEND_UNTRUSTED");
+/* Default to trusted if envvar missing or is "0". */
+libxl_defbool_setdefault(>trusted, !envvar || !strcmp("0", envvar));
 
 return rc;
 }
-- 
Anthony PERARD

Re: [PATCH 04/36] cpuidle,intel_idle: Fix CPUIDLE_FLAG_IRQ_ENABLE

[Michel Lespinasse]

On Thu, Jul 28, 2022 at 10:20:53AM -0700, Paul E. McKenney wrote:
> On Mon, Jul 25, 2022 at 12:43:06PM -0700, Michel Lespinasse wrote:
> > On Wed, Jun 08, 2022 at 04:27:27PM +0200, Peter Zijlstra wrote:
> > > Commit c227233ad64c ("intel_idle: enable interrupts before C1 on
> > > Xeons") wrecked intel_idle in two ways:
> > > 
> > >  - must not have tracing in idle functions
> > >  - must return with IRQs disabled
> > > 
> > > Additionally, it added a branch for no good reason.
> > > 
> > > Fixes: c227233ad64c ("intel_idle: enable interrupts before C1 on Xeons")
> > > Signed-off-by: Peter Zijlstra (Intel) 
> > 
> > After this change was introduced, I am seeing "WARNING: suspicious RCU
> > usage" when booting a kernel with debug options compiled in. Please
> > see the attached dmesg output. The issue starts with commit 32d4fd5751ea
> > and is still present in v5.19-rc8.
> > 
> > I'm not sure, is this too late to fix or revert in v5.19 final ?
> 
> I finally got a chance to take a quick look at this.
> 
> The rcu_eqs_exit() function is making a lockdep complaint about
> being invoked with interrupts enabled.  This function is called from
> rcu_idle_exit(), which is an expected code path from cpuidle_enter_state()
> via its call to rcu_idle_exit().  Except that rcu_idle_exit() disables
> interrupts before invoking rcu_eqs_exit().
> 
> The only other call to rcu_idle_exit() does not disable interrupts,
> but it is via rcu_user_exit(), which would be a very odd choice for
> cpuidle_enter_state().
> 
> It seems unlikely, but it might be that it is the use of local_irq_save()
> instead of raw_local_irq_save() within rcu_idle_exit() that is causing
> the trouble.  If this is the case, then the commit shown below would
> help.  Note that this commit removes the warning from lockdep, so it
> is necessary to build the kernel with CONFIG_RCU_EQS_DEBUG=y to enable
> equivalent debugging.
> 
> Could you please try your test with the -rce commit shown below applied?

Thanks for looking into it.

After checking out Peter's commit 32d4fd5751ea,
cherry picking your commit ed4ae5eff4b3,
and setting CONFIG_RCU_EQS_DEBUG=y in addition of my usual debug config,
I am now seeing this a few seconds into the boot:

[3.010650] [ cut here ]
[3.010651] WARNING: CPU: 0 PID: 0 at kernel/sched/clock.c:397 
sched_clock_tick+0x27/0x60
[3.010657] Modules linked in:
[3.010660] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
5.19.0-rc1-test-5-g1be22fea0611 #1
[3.010662] Hardware name: LENOVO 30BFS44D00/1036, BIOS S03KT51A 01/17/2022
[3.010663] RIP: 0010:sched_clock_tick+0x27/0x60
[3.010665] Code: 1f 40 00 53 eb 02 5b c3 66 90 8b 05 2f c3 40 01 85 c0 74 
18 65 8b 05 60 88 8f 4e 85 c0 75 0d 65 8b 05 a9 85 8f 4e 85 c0 74 02 <0f> 0b e8 
e2 6c 89 00 48 c7 c3 40 d5 02 00
 89 c0 48 03 1c c5 c0 98
[3.010667] RSP: :b2803e28 EFLAGS: 00010002
[3.010670] RAX: 0001 RBX: c8ce7fa07060 RCX: 0001
[3.010671] RDX:  RSI: b268dd21 RDI: b269ab13
[3.010673] RBP: 0001 R08: ffc300d5 R09: 0002be80
[3.010674] R10: 03625b53183a R11: a012b802b7a4 R12: b2aa9e80
[3.010675] R13: b2aa9e00 R14: 0001 R15: 
[3.010677] FS:  () GS:a012b800() 
knlGS:
[3.010678] CS:  0010 DS:  ES:  CR0: 80050033
[3.010680] CR2: a012f81ff000 CR3: 000c99612001 CR4: 003706f0
[3.010681] DR0:  DR1:  DR2: 
[3.010682] DR3:  DR6: fffe0ff0 DR7: 0400
[3.010683] Call Trace:
[3.010685]  
[3.010688]  cpuidle_enter_state+0xb7/0x4b0
[3.010694]  cpuidle_enter+0x29/0x40
[3.010697]  do_idle+0x1d4/0x210
[3.010702]  cpu_startup_entry+0x19/0x20
[3.010704]  rest_init+0x117/0x1a0
[3.010708]  arch_call_rest_init+0xa/0x10
[3.010711]  start_kernel+0x6d8/0x6ff
[3.010716]  secondary_startup_64_no_verify+0xce/0xdb
[3.010728]  
[3.010729] irq event stamp: 44179
[3.010730] hardirqs last  enabled at (44179): [] 
asm_sysvec_apic_timer_interrupt+0x1b/0x20
[3.010734] hardirqs last disabled at (44177): [] 
__do_softirq+0x3f0/0x498
[3.010736] softirqs last  enabled at (44178): [] 
__do_softirq+0x332/0x498
[3.010738] softirqs last disabled at (44171): [] 
irq_exit_rcu+0xab/0xf0
[3.010741] ---[ end trace  ]---

[xen-unstable test] 171910: tolerable FAIL

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-arm64-arm64-libvirt-xsm 12 debian-install fail pass in 171896
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 12 debian-hvm-install 
fail pass in 171896

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

[xen-unstable-smoke test] 171917: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-amd64-libvirt   6 libvirt-buildfail REGR. vs. 171884

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt  1 build-check(1)   blocked  n/a
 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  062790aca6b1faea62c9ed2737c3791efb0d0f4c
baseline version:
 xen  f732240fd3bac25116151db5ddeb7203b62e85ce

Last test of basis   171884  2022-07-27 12:03:31 Z2 days
Failing since171899  2022-07-28 19:01:47 Z0 days5 attempts
Testing same since   171917  2022-07-29 10:03:07 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Daniel P. Smith 
  George Dunlap 
  Jan Beulich 
  Jiamei Xie 
  Julien Grall 
  Oleksandr Tyshchenko 
  Stefano Stabellini 
  Xenia Ragiadakou 

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



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


Not pushing.


commit 062790aca6b1faea62c9ed2737c3791efb0d0f4c
Author: Xenia Ragiadakou 
Date:   Fri Jul 29 08:51:31 2022 +0200

arm/atomic: fix MISRA C 2012 Rule 20.7 violation

The macro parameter 'p' is used as an expression and needs to be enclosed in
parentheses.

Signed-off-by: Xenia Ragiadakou 
Reviewed-by: Stefano Stabellini 

commit 124f138b37d595294b3100349e26ffb3f1df7b13
Author: Xenia Ragiadakou 
Date:   Fri Jul 29 08:50:58 2022 +0200

xsm/dummy: fix MISRA C 2012 Directive 4.10 violation

Protect header file from being included more than once by adding ifndef 
guard.

Signed-off-by: Xenia Ragiadakou 
Reviewed-by: Luca Fancellu 
Acked-by: Daniel P. Smith 

commit 9ff3231f955cee4d62c7be6a03d061c037d7ca69
Author: Jan Beulich 
Date:   Fri Jul 29 08:50:25 2022 +0200

x86/shadow: drop CONFIG_HVM conditionals from sh_update_cr3()

Now that we're not building multi.c anymore for 2 and 3 guest levels
when !HVM, there's no point in having these conditionals anymore. (As
somewhat a special case, the last of the removed conditionals really
builds on shadow_mode_external() always returning false when !HVM.) This
way the code becomes a tiny bit more readable.

Signed-off-by: Jan Beulich 
Acked-by: Andrew Cooper 

commit 5b04fe78646a8222626996113c9d1e598cb84831
Author: Jan Beulich 
Date:   Fri Jul 29 08:49:48 2022 +0200

x86/shadow: don't open-code shadow_remove_all_shadows()

Let's use the existing inline wrapper instead of repeating respective
commentary at every site.

Signed-off-by: Jan Beulich 
Acked-by: Andrew Cooper 

commit 8a3b89e4307da260675483bb86fc06cc62ed7c08
Author: Jan Beulich 
Date:   Fri Jul 29 08:49:06 2022 +0200

x86/shadow: exclude HVM-only code from sh_remove_shadows() when !HVM

In my (debug) build this amounts to well over 500 bytes of dead code.

Signed-off-by: Jan Beulich 
Acked-by: Andrew Cooper 

commit 3629759626ac7201a670a8a2d4d4a536e7443575
Author: Jan Beulich 
Date:   Fri Jul 29 08:48:26 2022 +0200

x86/shadow: properly handle get_page() failing

We should not blindly (in a release build) insert the new entry in the
hash if a reference to the guest page cannot be obtained, or else an
excess reference would be put when removing the hash entry again. Crash
the domain 

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Rahul Singh]

Hi Julien

> On 28 Jul 2022, at 9:50 pm, Julien Grall  wrote:
> 
> Hi Rahul,
> 
> On 28/07/2022 16:37, Rahul Singh wrote:
>> As you mentioned, if we don’t restrict the number of events channel for the 
>> dom0 system will boot slower.
>> This is a good reason to restrict the number of event channels for dom0.
> Let me start that I am still fine if you want to push for a new parameter (so 
> long it is not Arm specific). However, I am afraid that I will not be able to 
> argue for it because I don't see a strict need for it.
> 
> Let me play the devil's advocate for a bit. AFAIU, you would like to 
> introduce the new parameter just to tell the admin the boot is going to be 
> slower if you use a event channel ID higher than N.
> 
> To me this sounds like the same as if an admin decide to use 10GB rather than 
> 1GB. There will be slow down.
> 
> This slowness is only boot specific and will not vary. So one could argue 
> this is easily noticeable and an admin can take remediation.
> 
> Given Jan's objection, I would like to propose to document it in the bindings 
> instead (a concerned admin will likely read it). Below a rough proposal for 
> the documentation:
> 
> "It is recommended to use low event channel ID."
> 
> Would that be suitable for you?

Yes, that will works for me. I will restrict the max event channel for domU 
only and also add the comment in 
"docs/misc/arm/device-tree/booting.txt” as suggested by you.

Regards,
Rahul

[libvirt test] 171914: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-arm64-libvirt   6 libvirt-buildfail REGR. vs. 151777
 build-armhf-libvirt   6 libvirt-buildfail REGR. vs. 151777
 build-amd64-libvirt   6 libvirt-buildfail REGR. vs. 151777
 build-i386-libvirt6 libvirt-buildfail REGR. vs. 151777

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt  1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-pair  1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a
 test-amd64-amd64-libvirt-vhd  1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-pair  1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a
 test-amd64-i386-libvirt-raw   1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-xsm   1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-qcow2  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-raw  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-raw  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-qcow2  1 build-check(1)   blocked  n/a

version targeted for testing:
 libvirt  640d185f01858b7a8db401235c929ac4798592d0
baseline version:
 libvirt  2c846fa6bcc11929c9fb857a22430fb9945654ad

Last test of basis   151777  2020-07-10 04:19:19 Z  749 days
Failing since151818  2020-07-11 04:18:52 Z  748 days  730 attempts
Testing same since   171914  2022-07-29 04:20:27 Z0 days1 attempts


People who touched revisions under test:
Adolfo Jayme Barrientos 
  Aleksandr Alekseev 
  Aleksei Zakharov 
  Amneesh Singh 
  Andika Triwidada 
  Andrea Bolognani 
  Andrew Melnychenko 
  Ani Sinha 
  Balázs Meskó 
  Barrett Schonefeld 
  Bastian Germann 
  Bastien Orivel 
  BiaoXiang Ye 
  Bihong Yu 
  Binfeng Wu 
  Bjoern Walk 
  Boris Fiuczynski 
  Brad Laue 
  Brian Turek 
  Bruno Haible 
  Chris Mayo 
  Christian Borntraeger 
  Christian Ehrhardt 
  Christian Kirbach 
  Christian Schoenebeck 
  Christophe Fergeau 
  Claudio Fontana 
  Cole Robinson 
  Collin Walling 
  Cornelia Huck 
  Cédric Bosdonnat 
  Côme Borsoi 
  Daniel Henrique Barboza 
  Daniel Letai 
  Daniel P. Berrange 
  Daniel P. Berrangé 
  David Michael 
  Didik Supriadi 
  dinglimin 
  Divya Garg 
  Dmitrii Shcherbakov 
  Dmytro Linkin 
  Eiichi Tsukata 
  Emilio Herrera 
  Eric Farman 
  Erik Skultety 
  Eugenio Pérez 
  Fabian Affolter 
  Fabian Freyer 
  Fabiano Fidêncio 
  Fangge Jin 
  Farhan Ali 
  Fedora Weblate Translation 
  Florian Schmidt 
  Franck Ridel 
  Gavi Teitz 
  gongwei 
  Guoyi Tu
  Göran Uddeborg 
  Halil Pasic 
  Han Han 
  Hao Wang 
  Haonan Wang 
  Hela Basa 
  Helmut Grohne 
  Hiroki Narukawa 
  Hyman Huang(黄勇) 
  Ian Wienand 
  Ioanna Alifieraki 
  Ivan Teterevkov 
  Jakob Meng 
  Jamie Strandboge 
  Jamie Strandboge 
  Jan Kuparinen 
  jason lee 
  Jean-Baptiste Holcroft 
  Jia Zhou 
  Jianan Gao 
  Jim Fehlig 
  Jin Yan 
  Jing Qi 
  Jinsheng Zhang 
  Jiri Denemark 
  Joachim Falk 
  John Ferlan 
  John Levon 
  John Levon 
  Jonathan Watt 
  Jonathon Jongsma 
  Julio Faracco 
  Justin Gatzen 
  Ján Tomko 
  Kashyap Chamarthy 
  Kevin Locke 
  Kim InSoo 
  Koichi Murase 
  Kristina Hanicova 
  Laine Stump 
  Laszlo Ersek 
  Lee Yarwood 
  Lei Yang 
  Lena Voytek 
  Liang Yan 
  Liang Yan 
  Liao Pingfang 
  Lin Ma 
  Lin Ma 
  Lin Ma 
  Liu Yiding 
  Lubomir Rintel 
  Luke Yue 
  Luyao Zhong 
  luzhipeng 
  Marc Hartmayer 
  Marc-André Lureau 
  Marek Marczykowski-Górecki 
  Mark Mielke 
  Markus Schade 
  Martin Kletzander 
  Martin Pitt 
  Masayoshi Mizuma 
  Matej Cepl 
  Matt Coleman 
  Matt Coleman 
  Mauro Matteo Cascella 
  Max Goodhart 
  Maxim Nestratov 
  Meina Li 
  Michal Privoznik 
  Michał Smyk 
  Milo Casagrande 
  minglei.liu 
  Moshe Levi 
  Moteen Shah 
  Moteen Shah 
  Muha Aliss 
  Nathan 
  Neal Gompa 
  Nick Chevsky 
  Nick Shyrokovskiy 
  Nickys Music Group 
  Nico Pache 
  Nicolas Lécureuil 
  Nicolas Lécureuil 
  Nikolay Shirokovskiy 
  Nikolay Shirokovskiy 
  Nikolay Shirokovskiy 
  Niteesh Dubey 
  Olaf Hering 
  Olesya Gerasimenko 
  Or Ozeri 
  Orion Poplawski 
  Pany 
  Paolo Bonzini 
  Patrick Magauran 
  

Re: [PATCH v1] xen: add late init call in start_xen

[Boyoun Park]

I really appreciate all the comments and reviews.
I understand that it is hard to add this patch without any usage.

On Fri, 29 Jul 2022, Stefano Stabellini:
> On Thu, 28 Jul 2022, Jan Beulich wrote:
> > On 28.07.2022 11:22, Boyoun Park wrote:
> > > Hello,
> > > 
> > > This patch added late_initcall to deal with
> > > 
> > > some init functions which should be called
> > > 
> > > after other init functions in start_xen.
> > > 
> > > If this patch is added,
> > > 
> > > then the original initcall in xen will be treated
> > > 
> > > as early_initcall and the late_initcall
> > > 
> > > which is added by this patch will be
> > > 
> > > called sequentially.
> > > 
> > > I cannot send patches through git send-email
> > > 
> > > due to some security issues in my work.
> > > 
> > > So intead, I just send the patches manually.
> > 
> > Which is fine, but you want to configure your mail client such that it
> > doesn't mangle the patch. Albeit I see that to cover for that at least
> > you've also attached both the patch and a cover letter. For a single
> > patch a cover letter can normally be omitted, but if you don't then
> > even if you're sending without "git send-email" you will want to send
> > both as separate mails, with the patch being a reply to the cover
> > letter thread root.
> 
> Yeah. Boyoun, if you only have a couple of patches then it is fine to
> send them manually. Otherwise, if you have many patches, you can send
> them in attachment as tarball and I'll send them all to xen-devel using
> git-send-email for you (of course keeping you as original author and
> retaining all Signed-off-bys).

You're awesome. Thanks you so much. I'm still requesting approvals to use git 
send-email.
I'll let you know if I have to send many patches wihout git send-email.

> > > Subject: [PATCH v1] xen: add late init call in start_xen
> > > 
> > > This patch added late_initcall section in init.data.
> > > 
> > > The late initcall would be called after initcall
> > > 
> > > in the start_xen function.
> > > 
> > > Some initializing works on priority should be run
> > > 
> > > in do_initcalls and other non-prioritized works
> > > 
> > > would be run in do_late_initcalls.
> > > 
> > > To call some functions by late_initcall,
> > > 
> > > then it is possible by using
> > > 
> > > __late_initcall(/*Function Name*/);
> > > 
> > > Signed-off-by: Boyoun Park 
> > 
> > So I could imagine this patch to be in a series where a subsequent
> > patch then adds an actual use of the new functionality. Without
> > that what you're proposing is to add dead code.
> 
> Yeah, I think it would be cool to have late initcalls but it makes sense
> to add them if we have someone that makes use of them.

I totally agree with your comments. Some drivers that I'm developing now and 
use this function are specific to my hardware environment.
Thus, it seems difficult to arrange them in the near future.
I will update patches if I can suggest an actual use.

> > > @@ -1964,6 +1966,7 @@ void __init noreturn __start_xen(unsigned long 
>mbi_p)
> > > 
> > >   bsp_info = get_cpu_info_from_stack((unsigned long)bsp_stack);
> > > 
> > >   *bsp_info = *info;
> > > 
> > > +/* Jump to the 1:1 virtual mappings of cpu0_stack. */
> > > 
> > >   asm volatile ("mov %[stk], %%rsp; jmp %c[fn]" ::
> > > 
> > > [stk] "g" (_info->guest_cpu_user_regs),
> > > 
> > > [fn] "i" (reinit_bsp_stack) : "memory");
> > > 
> > How does this addition of a comment relate to the purpose of the
> > patch?

It seems that this is unintentionally added while updating a commit base.
I'm so sorry for not checking thoroughly.
I will check carefully before sending next patches.

Boyoun Park

Re: [xen-unstable-smoke bisection] complete build-amd64-libvirt

[Julien Grall]

Hi Oleksandr,

On 29/07/2022 11:25, Oleksandr Tyshchenko wrote:


On 29.07.22 12:08, Julien Grall wrote:


Hello Julien


(+ Anthony)

Hi,

On 29/07/2022 07:48, Oleksandr Tyshchenko wrote:


On 29.07.22 09:22, Jan Beulich wrote:

Hello Jan


On 29.07.2022 03:04, osstest service owner wrote:

branch xen-unstable-smoke
xenbranch xen-unstable-smoke
job build-amd64-libvirt
testid libvirt-build

Tree: libvirt git://xenbits.xen.org/libvirt.git
Tree: libvirt_keycodemapdb
https://urldefense.com/v3/__https://gitlab.com/keycodemap/keycodemapdb.git__;!!GF_29dbcQIUBPA!0s_nyAgds977dw0dGPgFJGkIaBiKiXH3nR11Ni6gGjN5gQmB0DEhKrm5SUX4R0WhK8YkQemR6RVhiojpzij9yLM$
[gitlab[.]com]
Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
Tree: qemuu git://xenbits.xen.org/qemu-xen.git
Tree: xen git://xenbits.xen.org/xen.git

*** Found and reproduced problem changeset ***

     Bug is in tree:  xen git://xenbits.xen.org/xen.git
     Bug introduced: 66dd1c62b2a3c707bd5c55750d10a8223fbd577f
     Bug not present: f732240fd3bac25116151db5ddeb7203b62e85ce
     Last fail repro:
https://urldefense.com/v3/__http://logs.test-lab.xenproject.org/osstest/logs/171909/__;!!GF_29dbcQIUBPA!0s_nyAgds977dw0dGPgFJGkIaBiKiXH3nR11Ni6gGjN5gQmB0DEhKrm5SUX4R0WhK8YkQemR6RVhiojpmYABJkc$
[logs[.]test-lab[.]xenproject[.]org]


     commit 66dd1c62b2a3c707bd5c55750d10a8223fbd577f
     Author: Oleksandr Tyshchenko 
     Date:   Fri Jul 15 22:20:24 2022 +0300
         libxl: Add support for Virtio disk configuration

Just in case you didn't notice it: Something's wrong here. I didn't
look
at the details at all. Please advise whether a fix will soon arrive or
whether we should revert for the time being.


Sorry for the breakage. At the moment I have no idea what is wrong here,


 From the build log:

../../src/libxl/xen_xl.c: In function 'xenParseXLDisk':
../../src/libxl/xen_xl.c:779:17: error: enumeration value
'LIBXL_DISK_BACKEND_STANDALONE' not handled in switch
[-Werror=switch-enum]
  switch (libxldisk->backend) {
  ^~

The switch contains a default branch [1], so I am a bit puzzled why
GCC is not happy here.


Libvirt seems to compiled with -Wswitch-enum

And https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html says the
following:

"-Wswitch-enum

Warn whenever a switch statement has an index of enumerated type and
lacks a case for one or more of the named codes of that enumeration.
case labels outside the enumeration range also provoke warnings when
this option is used. The only difference between -Wswitch and this
option is that this option gives a warning about an omitted enumeration
code even if there is a default label."


Thanks for digging! That's explained the error. I still don't think we 
can solve the error in libxl.


So I would suggest to involve the libvirt folks to know how they want to 
solve the issue.


Cheers,

--
Julien Grall

Re: [xen-unstable-smoke bisection] complete build-amd64-libvirt

[Oleksandr Tyshchenko]

On 29.07.22 12:08, Julien Grall wrote:


Hello Julien

> (+ Anthony)
>
> Hi,
>
> On 29/07/2022 07:48, Oleksandr Tyshchenko wrote:
>>
>> On 29.07.22 09:22, Jan Beulich wrote:
>>
>> Hello Jan
>>
>>> On 29.07.2022 03:04, osstest service owner wrote:
 branch xen-unstable-smoke
 xenbranch xen-unstable-smoke
 job build-amd64-libvirt
 testid libvirt-build

 Tree: libvirt git://xenbits.xen.org/libvirt.git
 Tree: libvirt_keycodemapdb 
 https://urldefense.com/v3/__https://gitlab.com/keycodemap/keycodemapdb.git__;!!GF_29dbcQIUBPA!0s_nyAgds977dw0dGPgFJGkIaBiKiXH3nR11Ni6gGjN5gQmB0DEhKrm5SUX4R0WhK8YkQemR6RVhiojpzij9yLM$
  
 [gitlab[.]com]
 Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
 Tree: qemuu git://xenbits.xen.org/qemu-xen.git
 Tree: xen git://xenbits.xen.org/xen.git

 *** Found and reproduced problem changeset ***

     Bug is in tree:  xen git://xenbits.xen.org/xen.git
     Bug introduced: 66dd1c62b2a3c707bd5c55750d10a8223fbd577f
     Bug not present: f732240fd3bac25116151db5ddeb7203b62e85ce
     Last fail repro: 
 https://urldefense.com/v3/__http://logs.test-lab.xenproject.org/osstest/logs/171909/__;!!GF_29dbcQIUBPA!0s_nyAgds977dw0dGPgFJGkIaBiKiXH3nR11Ni6gGjN5gQmB0DEhKrm5SUX4R0WhK8YkQemR6RVhiojpmYABJkc$
  
 [logs[.]test-lab[.]xenproject[.]org]


     commit 66dd1c62b2a3c707bd5c55750d10a8223fbd577f
     Author: Oleksandr Tyshchenko 
     Date:   Fri Jul 15 22:20:24 2022 +0300
         libxl: Add support for Virtio disk configuration
>>> Just in case you didn't notice it: Something's wrong here. I didn't 
>>> look
>>> at the details at all. Please advise whether a fix will soon arrive or
>>> whether we should revert for the time being.
>>
>> Sorry for the breakage. At the moment I have no idea what is wrong here,
>
> From the build log:
>
> ../../src/libxl/xen_xl.c: In function 'xenParseXLDisk':
> ../../src/libxl/xen_xl.c:779:17: error: enumeration value 
> 'LIBXL_DISK_BACKEND_STANDALONE' not handled in switch 
> [-Werror=switch-enum]
>  switch (libxldisk->backend) {
>  ^~
>
> The switch contains a default branch [1], so I am a bit puzzled why 
> GCC is not happy here.

Libvirt seems to compiled with -Wswitch-enum

And https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html says the 
following:

"-Wswitch-enum

Warn whenever a switch statement has an index of enumerated type and 
lacks a case for one or more of the named codes of that enumeration. 
case labels outside the enumeration range also provoke warnings when 
this option is used. The only difference between -Wswitch and this 
option is that this option gives a warning about an omitted enumeration 
code even if there is a default label."


[snip]

-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH v4 2/2] xen/arm: add FF-A mediator

[Jens Wiklander]

Hi,

On Thu, Jul 28, 2022 at 9:15 PM Julien Grall  wrote:
>
> Hi,
>
> On 26/07/2022 07:17, Jens Wiklander wrote:
> > On Fri, Jul 8, 2022 at 3:41 PM Julien Grall  wrote:
> >>
> >> Hi Jens,
> >>
> >> I haven't checked whether the FFA driver is complaint with the spec. I
> >> mainly checked whether the code makes sense from Xen PoV.
> >>
> >> This is a fairly long patch to review. So I will split my review in
> >> multiple session/e-mail.
> >>
> >> On 22/06/2022 14:42, Jens Wiklander wrote:
> >>> Adds a FF-A version 1.1 [1] mediator to communicate with a Secure
> >>> Partition in secure world.
> >>>
> >>> The implementation is the bare minimum to be able to communicate with
> >>> OP-TEE running as an SPMC at S-EL1.
> >>>
> >>> This is loosely based on the TEE mediator framework and the OP-TEE
> >>> mediator.
> >>>
> >>> [1] https://developer.arm.com/documentation/den0077/latest
> >>> Signed-off-by: Jens Wiklander 
> >>> ---
> >>>SUPPORT.md|7 +
> >>>tools/libs/light/libxl_arm.c  |3 +
> >>>tools/libs/light/libxl_types.idl  |1 +
> >>>tools/xl/xl_parse.c   |3 +
> >>
> >> These changes would need an ack from the toolstack maintainers.
> >
> > OK, I'll keep them in CC.
> >
> >>
> >>>xen/arch/arm/Kconfig  |   11 +
> >>>xen/arch/arm/Makefile |1 +
> >>>xen/arch/arm/domain.c |   10 +
> >>>xen/arch/arm/domain_build.c   |1 +
> >>>xen/arch/arm/ffa.c| 1683 +
> >>>xen/arch/arm/include/asm/domain.h |4 +
> >>>xen/arch/arm/include/asm/ffa.h|   71 ++
> >>>xen/arch/arm/vsmc.c   |   17 +-
> >>>xen/include/public/arch-arm.h |2 +
> >>>13 files changed, 1811 insertions(+), 3 deletions(-)
> >>>create mode 100644 xen/arch/arm/ffa.c
> >>>create mode 100644 xen/arch/arm/include/asm/ffa.h
> >>>
> >>> diff --git a/SUPPORT.md b/SUPPORT.md
> >>> index 70e98964cbc0..215bb3c9043b 100644
> >>> --- a/SUPPORT.md
> >>> +++ b/SUPPORT.md
> >>> @@ -785,6 +785,13 @@ that covers the DMA of the device to be passed 
> >>> through.
> >>>
> >>>No support for QEMU backends in a 16K or 64K domain.
> >>>
> >>> +### ARM: Firmware Framework for Arm A-profile (FF-A) Mediator
> >>> +
> >>> +Status, Arm64: Tech Preview
> >>> +
> >>> +There are still some code paths where a vCPU may hog a pCPU longer than
> >>> +necessary. The FF-A mediator is not yet implemented for Arm32.
> >>> +
> >>>### ARM: Guest Device Tree support
> >>>
> >>>Status: Supported
> >>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> >>> index eef1de093914..a985609861c7 100644
> >>> --- a/tools/libs/light/libxl_arm.c
> >>> +++ b/tools/libs/light/libxl_arm.c
> >>> @@ -101,6 +101,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> >>>return ERROR_FAIL;
> >>>}
> >>>
> >>> +config->arch.ffa_enabled =
> >>> +libxl_defbool_val(d_config->b_info.arch_arm.ffa_enabled);
> >>> +
> >>>return 0;
> >>>}
> >>>
> >>> diff --git a/tools/libs/light/libxl_types.idl 
> >>> b/tools/libs/light/libxl_types.idl
> >>> index 2a42da2f7d78..bf4544bef399 100644
> >>> --- a/tools/libs/light/libxl_types.idl
> >>> +++ b/tools/libs/light/libxl_types.idl
> >>> @@ -646,6 +646,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> >>>
> >>>("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> >>>   ("vuart", libxl_vuart_type),
> >>> +   ("ffa_enabled", libxl_defbool),
> >>
> >> This needs to be accompagnied with a define LIBXL_HAVE_* in
> >> tools/include/libxl.h. Please see the examples in the header.
> >
> > OK, I'll add something. I'm not entirely sure how this is used so I'm
> > afraid it will be a bit of Cargo Cult programming from my side.
>
> The LIBXL_HAVE* by toolstack built on top of libxl (like virtio) to know
> whether a future is supported by the current library.
>
> [...]
>
> >>
> >>> +
> >>> +static inline uint64_t reg_pair_to_64(uint32_t reg0, uint32_t reg1)
> >>> +{
> >>> +return (uint64_t)reg0 << 32 | reg1;
> >>> +}
> >>> +
> >>> +static void inline reg_pair_from_64(uint32_t *reg0, uint32_t *reg1,
> >>> +uint64_t val)
> >>> +{
> >>> +*reg0 = val >> 32;
> >>> +*reg1 = val;
> >>> +}
> >>
> >> Those two functions are the same as optee.c but with a different. I
> >> would rather prefer if we avoid the duplication and provide generic
> >> helpers in a pre-requisite.
> >
> > These functions are trivial but at the same time for a special purpose
> > which happens to coincide with the usage in optee.c. I can't find a
> > suitable common .h file and creating a new one seems a bit much.
>
> I would implement it in regs.h.

OK, thanks.

>
> [...]
>
> >>> +.a4 = pg_count,
> >>> +};
> >>> +struct arm_smccc_1_2_regs resp;
> >>> +
> >>> +/*
> 

Re: [PATCH] xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation

[Xenia Ragiadakou]

On 7/29/22 10:22, Jan Beulich wrote:

On 29.07.2022 09:01, Xenia Ragiadakou wrote:

On 7/29/22 09:16, Jan Beulich wrote:

On 29.07.2022 07:23, Xenia Ragiadakou wrote:

On 7/29/22 01:56, Stefano Stabellini wrote:

On Thu, 28 Jul 2022, Julien Grall wrote:

On 28/07/2022 15:20, Jan Beulich wrote:

On 28.07.2022 15:56, Julien Grall wrote:

On 28/07/2022 14:49, Xenia Ragiadakou wrote:

--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -461,7 +461,7 @@
  /* Access to system registers */
 #define WRITE_SYSREG64(v, name) do {\
-uint64_t _r = v;\
+uint64_t _r = (v);  \


I am failing to see why the parentheses are necessary here. Could you
give an example where the lack of them would end up to different code?


I think it is merely good practice to parenthesize the right sides of =.
Indeed with assignment operators having second to lowest precedence, and
with comma (the lowest precedence one) requiring parenthesization at the
macro invocation site, there should be no real need for parentheses here.


I am not really happy with adding those parentheses because they are
pointless. But if there are a consensus to use it, then the commit message
should be updated to clarify this is just here to please MISRA (to me "need"
implies it would be bug).


Let me premise that I don't know if this counts as a MISRA violation or
not. (Also I haven't checked if cppcheck/eclair report it as violation.)

But I think the reason for making the change would be to follow our
coding style / coding practices. It makes the code simpler to figure out
that it is correct, to review and maintain if we always add the
parenthesis even in cases like this one where they are not strictly
necessary. We are going to save our future selves some mental cycles.

So the explanation on the commit message could be along those lines.


First, the rule 20.7 states "Expressions resulting from the expansion of
macro parameters shall
be enclosed in parentheses". So, here it is a clear violation of the
rule because the right side of the assignment operator is an expression.

Second, as I stated in a previous email, if v is not enclosed in
parentheses, I could write the story of my life in there and compile it
:) So, it would be a bug.

So, I recommend the title and the explanation i.e
"xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation

The macro parameter 'v' is used as an expression and needs to be enclosed in
parentheses."
to remain as is because they are accurate.


I'm afraid you're following the MISRA wording too much to the latter.
Earlier on you agreed that when macro parameters are used as function
arguments, the parentheses can be omitted. Yet by what you say above
those are also expressions.


Yes, those are also expressions (that's why I added parentheses
initially) and I agreed with you that the parentheses there may not be
necessary because I could not think of an example that will produce
different behaviors with and without the parentheses. This will need a
formal deviation I imagine or maybe a MISRA C expert could provide a
justification regarding why parentheses are needed around function
arguments that we may have not think of.



With the example that Jan provided I just realized, if function 
arguments are not parenthesized, somebody could alter the rest of the 
arguments with which a function is called via an intermediate macro ... 
a rather far fetched example but still ...



As indicated before - I think parentheses
are wanted here, but it's strictly "wanted", and hence the title
better wouldn't say "fix" (but e.g. "improve") and the description
also should be "softened".



Regarding the latter, are you saying that the parentheses are not needed?
In my opinion they are needed to prevent the bug described in the
previous email i.e passing multiple statements to the macro.


Any such use would be rejected during review, I'm sure.

However I think there's another case which might indeed make this
more than just a "want" (and then responses further down are to be
viewed only in the context of earlier discussion):

#define wr(v) ({ \
unsigned r_ = v; \
asm("" :: "r" (r_)); \
})

#define M x, y

void test(unsigned x) {
wr(M);
}

While this would result in an unused variable warning, it's surely
misleading (and less certain to be noticed during review) - which
is what Misra wants to avoid. Let's see what Julien thinks.


By whom are they wanted? I 'm afraid I cannot understand.


By us as a community: This can be viewed as one of many agreements we
have on coding style. (As such it may want to be written down somewhere.)


Also, are you proposing to change the title into "Improve MISRA C 2012
Rule 20.7 violation" ?


Obviously not. I was thinking of "improve to avoid ...".

Jan


--
Xenia

Re: [xen-unstable-smoke bisection] complete build-amd64-libvirt

[Oleksandr Tyshchenko]

On 29.07.22 12:08, Julien Grall wrote:


Hello Julien

> (+ Anthony)
>
> Hi,
>
> On 29/07/2022 07:48, Oleksandr Tyshchenko wrote:
>>
>> On 29.07.22 09:22, Jan Beulich wrote:
>>
>> Hello Jan
>>
>>> On 29.07.2022 03:04, osstest service owner wrote:
 branch xen-unstable-smoke
 xenbranch xen-unstable-smoke
 job build-amd64-libvirt
 testid libvirt-build

 Tree: libvirt git://xenbits.xen.org/libvirt.git
 Tree: libvirt_keycodemapdb 
 https://urldefense.com/v3/__https://gitlab.com/keycodemap/keycodemapdb.git__;!!GF_29dbcQIUBPA!0s_nyAgds977dw0dGPgFJGkIaBiKiXH3nR11Ni6gGjN5gQmB0DEhKrm5SUX4R0WhK8YkQemR6RVhiojpzij9yLM$
  
 [gitlab[.]com]
 Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
 Tree: qemuu git://xenbits.xen.org/qemu-xen.git
 Tree: xen git://xenbits.xen.org/xen.git

 *** Found and reproduced problem changeset ***

     Bug is in tree:  xen git://xenbits.xen.org/xen.git
     Bug introduced: 66dd1c62b2a3c707bd5c55750d10a8223fbd577f
     Bug not present: f732240fd3bac25116151db5ddeb7203b62e85ce
     Last fail repro: 
 https://urldefense.com/v3/__http://logs.test-lab.xenproject.org/osstest/logs/171909/__;!!GF_29dbcQIUBPA!0s_nyAgds977dw0dGPgFJGkIaBiKiXH3nR11Ni6gGjN5gQmB0DEhKrm5SUX4R0WhK8YkQemR6RVhiojpmYABJkc$
  
 [logs[.]test-lab[.]xenproject[.]org]


     commit 66dd1c62b2a3c707bd5c55750d10a8223fbd577f
     Author: Oleksandr Tyshchenko 
     Date:   Fri Jul 15 22:20:24 2022 +0300
         libxl: Add support for Virtio disk configuration
>>> Just in case you didn't notice it: Something's wrong here. I didn't 
>>> look
>>> at the details at all. Please advise whether a fix will soon arrive or
>>> whether we should revert for the time being.
>>
>> Sorry for the breakage. At the moment I have no idea what is wrong here,
>
> From the build log:
>
> ../../src/libxl/xen_xl.c: In function 'xenParseXLDisk':
> ../../src/libxl/xen_xl.c:779:17: error: enumeration value 
> 'LIBXL_DISK_BACKEND_STANDALONE' not handled in switch 
> [-Werror=switch-enum]
>  switch (libxldisk->backend) {
>  ^~
>
> The switch contains a default branch [1], so I am a bit puzzled why 
> GCC is not happy here.
>
>>
>> I will try to investigate and provide a fix by the end of the day.
>
> So the general expectation is libvirt should be able to compile 
> without using the new features provided by libxl.
>
> In this case, I am not sure there is anything we could do in libxl 
> without large rework to conditionally define 
> LIBXL_DISK_BACKEND_STANDALONE.
>
> So if a fix is necessary, then it will probably need to be in libvirt.


I haven't even imagined that I would need to build something else except 
xen tools itself to check whether patch series doesn't break anything, 
sorry.

I have just reproduced an issue by building libvirt according to the 
steps in the osstest build log (this was a fun). Except an error being 
reported by test there is one more, almost the similar to reported one, 
in libxlUpdateDiskDef() but there switch doesn't contain a default branch.

With that diff I was able to build w/o issues.

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 8e63d40376..6745212ae8 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1219,6 +1219,7 @@ libxlUpdateDiskDef(virDomainDiskDefPtr l_disk, 
libxl_device_disk *x_disk)
  driver = "phy";
  break;
  case LIBXL_DISK_BACKEND_UNKNOWN:
+    case LIBXL_DISK_BACKEND_STANDALONE:
  break;
  }
  if (driver)
diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
index f9dc18ab18..a55871bb56 100644
--- a/src/libxl/xen_xl.c
+++ b/src/libxl/xen_xl.c
@@ -796,6 +796,7 @@ xenParseXLDisk(virConfPtr conf, virDomainDefPtr def)
  goto fail;
  virDomainDiskSetType(disk, VIR_STORAGE_TYPE_BLOCK);
  break;
+    case LIBXL_DISK_BACKEND_STANDALONE:
  default:
  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
     _("disk backend not supported: %s"),


>
> Cheers,
>
> [1] 
> https://urldefense.com/v3/__https://xenbits.xen.org/gitweb/?p=libvirt.git;a=blob;f=src*libxl*xen_xl.c;h=f9dc18ab18b208d319282ce422f46c75135c0673;hb=refs*heads*xen-tested-master*l779__;Ly8vLyM!!GF_29dbcQIUBPA!2IntpOXH95iBjVgHqZ8H_0-otnI7PwzKbLVSf6Dg3vsyxkrQOiJQPjPSqJliGoPuaf1JXmszMlKloTcFUDYBOA$
>  
> [xenbits[.]xen[.]org]
>
-- 
Regards,

Oleksandr Tyshchenko

[xen-unstable-smoke test] 171915: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-amd64-libvirt   6 libvirt-buildfail REGR. vs. 171884

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt  1 build-check(1)   blocked  n/a
 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  108e6f282d2c2b8442ac9e1487e6fd7865cd6ede
baseline version:
 xen  f732240fd3bac25116151db5ddeb7203b62e85ce

Last test of basis   171884  2022-07-27 12:03:31 Z1 days
Failing since171899  2022-07-28 19:01:47 Z0 days4 attempts
Testing same since   171911  2022-07-29 02:00:25 Z0 days2 attempts


People who touched revisions under test:
  George Dunlap 
  Jiamei Xie 
  Julien Grall 
  Oleksandr Tyshchenko 
  Stefano Stabellini 
  Xenia Ragiadakou 

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



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


Not pushing.


commit 108e6f282d2c2b8442ac9e1487e6fd7865cd6ede
Author: Xenia Ragiadakou 
Date:   Thu Jul 28 10:58:56 2022 +0300

automation: arm64: Create a test job for testing static allocation on qemu

Enable CONFIG_STATIC_MEMORY in the existing arm64 build.

Create a new test job, called qemu-smoke-arm64-gcc-staticmem.

Adjust qemu-smoke-arm64.sh script to accomodate the static memory test as a
new test variant. The test variant is determined based on the first argument
passed to the script. For testing static memory, the argument is 
'static-mem'.

The test configures DOM1 with a static memory region and adds a check in the
init script.
The check consists in comparing the contents of the /proc/device-tree
memory entry with the static memory range with which DOM1 was configured.
If the memory layout is correct, a message gets printed by DOM1.

At the end of the qemu run, the script searches for the specific message
in the logs and fails if not found.

Signed-off-by: Xenia Ragiadakou 
Signed-off-by: Stefano Stabellini 
Reviewed-by: Penny Zheng 
Reviewed-by: Stefano Stabellini 

commit 37339ba9ef46cf55e077ca50235279f058b01779
Author: Xenia Ragiadakou 
Date:   Thu Jul 28 10:58:55 2022 +0300

automation: Remove XEN_CONFIG_EXPERT leftovers

The EXPERT config option cannot anymore be selected via the environmental
variable XEN_CONFIG_EXPERT. Remove stale references to XEN_CONFIG_EXPERT
from the automation code.

Signed-off-by: Xenia Ragiadakou 
Reviewed-by: Stefano Stabellini 

commit ca45d3cb4586372909f350e54482246f994e1bc7
Author: Oleksandr Tyshchenko 
Date:   Fri Jul 15 22:20:26 2022 +0300

libxl/arm: Create specific IOMMU node to be referred by virtio-mmio device

Reuse generic IOMMU device tree bindings to communicate Xen specific
information for the virtio devices for which the restricted memory
access using Xen grant mappings need to be enabled.

Insert "iommus" property pointed to the IOMMU node with "xen,grant-dma"
compatible to all virtio devices which backends are going to run in
non-hardware domains (which are non-trusted by default).

Based on device-tree binding from Linux:
Documentation/devicetree/bindings/iommu/xen,grant-dma.yaml

The example of generated nodes:

xen_iommu {
compatible = "xen,grant-dma";

Re: [xen-unstable-smoke bisection] complete build-amd64-libvirt

[Julien Grall]

(+ Anthony)

Hi,

On 29/07/2022 07:48, Oleksandr Tyshchenko wrote:


On 29.07.22 09:22, Jan Beulich wrote:

Hello Jan


On 29.07.2022 03:04, osstest service owner wrote:

branch xen-unstable-smoke
xenbranch xen-unstable-smoke
job build-amd64-libvirt
testid libvirt-build

Tree: libvirt git://xenbits.xen.org/libvirt.git
Tree: libvirt_keycodemapdb 
https://urldefense.com/v3/__https://gitlab.com/keycodemap/keycodemapdb.git__;!!GF_29dbcQIUBPA!0s_nyAgds977dw0dGPgFJGkIaBiKiXH3nR11Ni6gGjN5gQmB0DEhKrm5SUX4R0WhK8YkQemR6RVhiojpzij9yLM$
 [gitlab[.]com]
Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
Tree: qemuu git://xenbits.xen.org/qemu-xen.git
Tree: xen git://xenbits.xen.org/xen.git

*** Found and reproduced problem changeset ***

Bug is in tree:  xen git://xenbits.xen.org/xen.git
Bug introduced:  66dd1c62b2a3c707bd5c55750d10a8223fbd577f
Bug not present: f732240fd3bac25116151db5ddeb7203b62e85ce
Last fail repro: 
https://urldefense.com/v3/__http://logs.test-lab.xenproject.org/osstest/logs/171909/__;!!GF_29dbcQIUBPA!0s_nyAgds977dw0dGPgFJGkIaBiKiXH3nR11Ni6gGjN5gQmB0DEhKrm5SUX4R0WhK8YkQemR6RVhiojpmYABJkc$
 [logs[.]test-lab[.]xenproject[.]org]


commit 66dd1c62b2a3c707bd5c55750d10a8223fbd577f
Author: Oleksandr Tyshchenko 
Date:   Fri Jul 15 22:20:24 2022 +0300

libxl: Add support for Virtio disk configuration

Just in case you didn't notice it: Something's wrong here. I didn't look
at the details at all. Please advise whether a fix will soon arrive or
whether we should revert for the time being.


Sorry for the breakage. At the moment I have no idea what is wrong here,


From the build log:

../../src/libxl/xen_xl.c: In function 'xenParseXLDisk':
../../src/libxl/xen_xl.c:779:17: error: enumeration value 
'LIBXL_DISK_BACKEND_STANDALONE' not handled in switch [-Werror=switch-enum]

 switch (libxldisk->backend) {
 ^~

The switch contains a default branch [1], so I am a bit puzzled why GCC 
is not happy here.




I will try to investigate and provide a fix by the end of the day.


So the general expectation is libvirt should be able to compile without 
using the new features provided by libxl.


In this case, I am not sure there is anything we could do in libxl 
without large rework to conditionally define LIBXL_DISK_BACKEND_STANDALONE.


So if a fix is necessary, then it will probably need to be in libvirt.

Cheers,

[1] 
https://xenbits.xen.org/gitweb/?p=libvirt.git;a=blob;f=src/libxl/xen_xl.c;h=f9dc18ab18b208d319282ce422f46c75135c0673;hb=refs/heads/xen-tested-master#l779


--
Julien Grall

[PATCH v2] arm/vgic-v3: fix virq offset in the rank when storing irouter

[Hongda Deng]

When vGIC performs irouter registers emulation, to get the target vCPU
via virq conveniently, Xen doesn't store the irouter value directly,
instead it will use the value (affinities) in irouter to calculate the
target vCPU, and then save the target vCPU in irq rank->vcpu[offset].

When vGIC tries to get the target vCPU, it first calculates the target
vCPU index via
  int target = read_atomic(>vcpu[virq & INTERRUPT_RANK_MASK]);
and then it gets the target vCPU via
  v->domain->vcpu[target];

When vGIC tries to store irouter for one virq, the target vCPU index
in the rank is computed as
  offset &= virq & INTERRUPT_RANK_MASK;
finally it gets the target vCPU via
  d->vcpu[read_atomic(>vcpu[offset])];

There is a difference between them while getting the target vCPU index
in the rank. Actually (virq & INTERRUPT_RANK_MASK) would already get
the target vCPU index in the rank, it's wrong to add '&' before '=' when
calculate the offset.

For example, the target vCPU index in the rank should be 6 for virq 38,
but vGIC will get offset=0 when vGIC stores the irouter for this virq,
and finally vGIC will access the wrong target vCPU index in the rank
when updating the irouter.

Fixes: 5d495f4349b5 ("xen/arm: vgic: Optimize the way to store the target vCPU 
in the rank")
Signed-off-by: Hongda Deng 
---
 xen/arch/arm/vgic-v3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index e4ba9a6476..7fb99a9ff2 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -135,7 +135,7 @@ static void vgic_store_irouter(struct domain *d, struct 
vgic_irq_rank *rank,
 ASSERT(virq >= 32);
 
 /* Get the index in the rank */
-offset &= virq & INTERRUPT_RANK_MASK;
+offset = virq & INTERRUPT_RANK_MASK;
 
 new_vcpu = vgic_v3_irouter_to_vcpu(d, irouter);
 old_vcpu = d->vcpu[read_atomic(>vcpu[offset])];
-- 
2.25.1

[ovmf test] 171913: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf 0d0bfcb4571caa65b7875003f38e67e2ac7e5560
baseline version:
 ovmf 3eca64f157c340f9bbf552d89a69698a3090c080

Last test of basis   171898  2022-07-28 17:13:05 Z0 days
Testing same since   171913  2022-07-29 03:12:41 Z0 days1 attempts


People who touched revisions under test:
  Chasel Chiu 

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
   3eca64f157..0d0bfcb457  0d0bfcb4571caa65b7875003f38e67e2ac7e5560 -> 
xen-tested-master

Re: [PATCH] xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation

[Xenia Ragiadakou]

On 7/29/22 10:22, Jan Beulich wrote:

On 29.07.2022 09:01, Xenia Ragiadakou wrote:

On 7/29/22 09:16, Jan Beulich wrote:

On 29.07.2022 07:23, Xenia Ragiadakou wrote:

On 7/29/22 01:56, Stefano Stabellini wrote:

On Thu, 28 Jul 2022, Julien Grall wrote:

On 28/07/2022 15:20, Jan Beulich wrote:

On 28.07.2022 15:56, Julien Grall wrote:

On 28/07/2022 14:49, Xenia Ragiadakou wrote:

--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -461,7 +461,7 @@
  /* Access to system registers */
 #define WRITE_SYSREG64(v, name) do {\
-uint64_t _r = v;\
+uint64_t _r = (v);  \


I am failing to see why the parentheses are necessary here. Could you
give an example where the lack of them would end up to different code?


I think it is merely good practice to parenthesize the right sides of =.
Indeed with assignment operators having second to lowest precedence, and
with comma (the lowest precedence one) requiring parenthesization at the
macro invocation site, there should be no real need for parentheses here.


I am not really happy with adding those parentheses because they are
pointless. But if there are a consensus to use it, then the commit message
should be updated to clarify this is just here to please MISRA (to me "need"
implies it would be bug).


Let me premise that I don't know if this counts as a MISRA violation or
not. (Also I haven't checked if cppcheck/eclair report it as violation.)

But I think the reason for making the change would be to follow our
coding style / coding practices. It makes the code simpler to figure out
that it is correct, to review and maintain if we always add the
parenthesis even in cases like this one where they are not strictly
necessary. We are going to save our future selves some mental cycles.

So the explanation on the commit message could be along those lines.


First, the rule 20.7 states "Expressions resulting from the expansion of
macro parameters shall
be enclosed in parentheses". So, here it is a clear violation of the
rule because the right side of the assignment operator is an expression.

Second, as I stated in a previous email, if v is not enclosed in
parentheses, I could write the story of my life in there and compile it
:) So, it would be a bug.

So, I recommend the title and the explanation i.e
"xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation

The macro parameter 'v' is used as an expression and needs to be enclosed in
parentheses."
to remain as is because they are accurate.


I'm afraid you're following the MISRA wording too much to the latter.
Earlier on you agreed that when macro parameters are used as function
arguments, the parentheses can be omitted. Yet by what you say above
those are also expressions.


Yes, those are also expressions (that's why I added parentheses
initially) and I agreed with you that the parentheses there may not be
necessary because I could not think of an example that will produce
different behaviors with and without the parentheses. This will need a
formal deviation I imagine or maybe a MISRA C expert could provide a
justification regarding why parentheses are needed around function
arguments that we may have not think of.


As indicated before - I think parentheses
are wanted here, but it's strictly "wanted", and hence the title
better wouldn't say "fix" (but e.g. "improve") and the description
also should be "softened".



Regarding the latter, are you saying that the parentheses are not needed?
In my opinion they are needed to prevent the bug described in the
previous email i.e passing multiple statements to the macro.


Any such use would be rejected during review, I'm sure.


I would argue that any such use should be rejected by the compiler.



However I think there's another case which might indeed make this
more than just a "want" (and then responses further down are to be
viewed only in the context of earlier discussion):

#define wr(v) ({ \
unsigned r_ = v; \
asm("" :: "r" (r_)); \
})

#define M x, y

void test(unsigned x) {
wr(M);
}

While this would result in an unused variable warning, it's surely
misleading (and less certain to be noticed during review) - which
is what Misra wants to avoid. Let's see what Julien thinks.


By whom are they wanted? I 'm afraid I cannot understand.


By us as a community: This can be viewed as one of many agreements we
have on coding style. (As such it may want to be written down somewhere.)



In that case, first it should be mentioned in the coding style document 
and then make reference to it in commit messages.



Also, are you proposing to change the title into "Improve MISRA C 2012
Rule 20.7 violation" ?


Obviously not. I was thinking of "improve to avoid ...".



--
Xenia

Re: [PATCH] xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation

[Jan Beulich]

On 29.07.2022 09:01, Xenia Ragiadakou wrote:
> On 7/29/22 09:16, Jan Beulich wrote:
>> On 29.07.2022 07:23, Xenia Ragiadakou wrote:
>>> On 7/29/22 01:56, Stefano Stabellini wrote:
 On Thu, 28 Jul 2022, Julien Grall wrote:
> On 28/07/2022 15:20, Jan Beulich wrote:
>> On 28.07.2022 15:56, Julien Grall wrote:
>>> On 28/07/2022 14:49, Xenia Ragiadakou wrote:
 --- a/xen/arch/arm/include/asm/arm64/sysregs.h
 +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
 @@ -461,7 +461,7 @@
  /* Access to system registers */
 #define WRITE_SYSREG64(v, name) do {\
 -uint64_t _r = v;\
 +uint64_t _r = (v);  \
>>>
>>> I am failing to see why the parentheses are necessary here. Could you
>>> give an example where the lack of them would end up to different code?
>>
>> I think it is merely good practice to parenthesize the right sides of =.
>> Indeed with assignment operators having second to lowest precedence, and
>> with comma (the lowest precedence one) requiring parenthesization at the
>> macro invocation site, there should be no real need for parentheses here.
>
> I am not really happy with adding those parentheses because they are
> pointless. But if there are a consensus to use it, then the commit message
> should be updated to clarify this is just here to please MISRA (to me 
> "need"
> implies it would be bug).

 Let me premise that I don't know if this counts as a MISRA violation or
 not. (Also I haven't checked if cppcheck/eclair report it as violation.)

 But I think the reason for making the change would be to follow our
 coding style / coding practices. It makes the code simpler to figure out
 that it is correct, to review and maintain if we always add the
 parenthesis even in cases like this one where they are not strictly
 necessary. We are going to save our future selves some mental cycles.

 So the explanation on the commit message could be along those lines.
>>>
>>> First, the rule 20.7 states "Expressions resulting from the expansion of
>>> macro parameters shall
>>>be enclosed in parentheses". So, here it is a clear violation of the
>>> rule because the right side of the assignment operator is an expression.
>>>
>>> Second, as I stated in a previous email, if v is not enclosed in
>>> parentheses, I could write the story of my life in there and compile it
>>> :) So, it would be a bug.
>>>
>>> So, I recommend the title and the explanation i.e
>>> "xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation
>>>
>>> The macro parameter 'v' is used as an expression and needs to be enclosed in
>>>parentheses."
>>> to remain as is because they are accurate.
>>
>> I'm afraid you're following the MISRA wording too much to the latter.
>> Earlier on you agreed that when macro parameters are used as function
>> arguments, the parentheses can be omitted. Yet by what you say above
>> those are also expressions. 
> 
> Yes, those are also expressions (that's why I added parentheses 
> initially) and I agreed with you that the parentheses there may not be 
> necessary because I could not think of an example that will produce 
> different behaviors with and without the parentheses. This will need a 
> formal deviation I imagine or maybe a MISRA C expert could provide a 
> justification regarding why parentheses are needed around function 
> arguments that we may have not think of.
> 
>> As indicated before - I think parentheses
>> are wanted here, but it's strictly "wanted", and hence the title
>> better wouldn't say "fix" (but e.g. "improve") and the description
>> also should be "softened".
>>
> 
> Regarding the latter, are you saying that the parentheses are not needed?
> In my opinion they are needed to prevent the bug described in the 
> previous email i.e passing multiple statements to the macro.

Any such use would be rejected during review, I'm sure.

However I think there's another case which might indeed make this
more than just a "want" (and then responses further down are to be
viewed only in the context of earlier discussion):

#define wr(v) ({ \
unsigned r_ = v; \
asm("" :: "r" (r_)); \
})

#define M x, y

void test(unsigned x) {
wr(M);
}

While this would result in an unused variable warning, it's surely
misleading (and less certain to be noticed during review) - which
is what Misra wants to avoid. Let's see what Julien thinks.

> By whom are they wanted? I 'm afraid I cannot understand.

By us as a community: This can be viewed as one of many agreements we
have on coding style. (As such it may want to be written down somewhere.)

> Also, are you proposing to change the title into "Improve MISRA C 2012 
> Rule 20.7 violation" ?

Obviously not. I was thinking of "improve to avoid ...".

Jan

[PATCH v4] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector

[Jane Malalane]

Implement support for the HVMOP_set_evtchn_upcall_vector hypercall in
order to set the per-vCPU event channel vector callback on Linux and
use it in preference of HVM_PARAM_CALLBACK_IRQ.

If the per-VCPU vector setup is successful on BSP, use this method
for the APs. If not, fallback to the global vector-type callback.

Also register callback_irq at per-vCPU event channel setup to trick
toolstack to think the domain is enlightened.

Suggested-by: "Roger Pau Monné" 
Signed-off-by: Jane Malalane 
Reviewed-by: Boris Ostrovsky 
---
CC: Juergen Gross 
CC: Boris Ostrovsky 
CC: Thomas Gleixner 
CC: Ingo Molnar 
CC: Borislav Petkov 
CC: Dave Hansen 
CC: x...@kernel.org
CC: "H. Peter Anvin" 
CC: Stefano Stabellini 
CC: Oleksandr Tyshchenko 
CC: Jan Beulich 
CC: Maximilian Heyne 
CC: xen-devel@lists.xenproject.org

v4:
 * amend code comment

v3:
 * comment style
 * add comment on toolstack trick
 * remove unnecessary variable and function call
 * surround x86-specific code with #ifdef

v2:
 * remove no_vector_callback
 * make xen_have_vector_callback a bool
 * rename xen_ack_upcall to xen_percpu_upcall
 * fail to bring CPU up on init instead of crashing kernel
 * add and use xen_set_upcall_vector where suitable
 * xen_setup_upcall_vector -> xen_init_setup_upcall_vector for clarity
---
 arch/x86/include/asm/xen/cpuid.h   |  2 ++
 arch/x86/include/asm/xen/events.h  |  3 ++-
 arch/x86/xen/enlighten.c   |  2 +-
 arch/x86/xen/enlighten_hvm.c   | 24 -
 arch/x86/xen/suspend_hvm.c | 10 ++-
 drivers/xen/events/events_base.c   | 53 +-
 include/xen/hvm.h  |  2 ++
 include/xen/interface/hvm/hvm_op.h | 19 ++
 8 files changed, 100 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/xen/cpuid.h b/arch/x86/include/asm/xen/cpuid.h
index 78e667a31d6c..6daa9b0c8d11 100644
--- a/arch/x86/include/asm/xen/cpuid.h
+++ b/arch/x86/include/asm/xen/cpuid.h
@@ -107,6 +107,8 @@
  * ID field from 8 to 15 bits, allowing to target APIC IDs up 32768.
  */
 #define XEN_HVM_CPUID_EXT_DEST_ID  (1u << 5)
+/* Per-vCPU event channel upcalls */
+#define XEN_HVM_CPUID_UPCALL_VECTOR(1u << 6)
 
 /*
  * Leaf 6 (0x4x05)
diff --git a/arch/x86/include/asm/xen/events.h 
b/arch/x86/include/asm/xen/events.h
index 068d9b067c83..62bdceb594f1 100644
--- a/arch/x86/include/asm/xen/events.h
+++ b/arch/x86/include/asm/xen/events.h
@@ -23,7 +23,7 @@ static inline int xen_irqs_disabled(struct pt_regs *regs)
 /* No need for a barrier -- XCHG is a barrier on x86. */
 #define xchg_xen_ulong(ptr, val) xchg((ptr), (val))
 
-extern int xen_have_vector_callback;
+extern bool xen_have_vector_callback;
 
 /*
  * Events delivered via platform PCI interrupts are always
@@ -34,4 +34,5 @@ static inline bool xen_support_evtchn_rebind(void)
return (!xen_hvm_domain() || xen_have_vector_callback);
 }
 
+extern bool xen_percpu_upcall;
 #endif /* _ASM_X86_XEN_EVENTS_H */
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 30c6e986a6cd..b8db2148c07d 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -51,7 +51,7 @@ EXPORT_SYMBOL_GPL(xen_start_info);
 
 struct shared_info xen_dummy_shared_info;
 
-__read_mostly int xen_have_vector_callback;
+__read_mostly bool xen_have_vector_callback = true;
 EXPORT_SYMBOL_GPL(xen_have_vector_callback);
 
 /*
diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index 8b71b1dd7639..198d3cd3e9a5 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -7,6 +7,8 @@
 
 #include 
 #include 
+#include 
+#include 
 #include 
 
 #include 
@@ -30,6 +32,9 @@
 
 static unsigned long shared_info_pfn;
 
+__ro_after_init bool xen_percpu_upcall;
+EXPORT_SYMBOL_GPL(xen_percpu_upcall);
+
 void xen_hvm_init_shared_info(void)
 {
struct xen_add_to_physmap xatp;
@@ -125,6 +130,9 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_xen_hvm_callback)
 {
struct pt_regs *old_regs = set_irq_regs(regs);
 
+   if (xen_percpu_upcall)
+   ack_APIC_irq();
+
inc_irq_stat(irq_hv_callback_count);
 
xen_hvm_evtchn_do_upcall();
@@ -168,6 +176,15 @@ static int xen_cpu_up_prepare_hvm(unsigned int cpu)
if (!xen_have_vector_callback)
return 0;
 
+   if (xen_percpu_upcall) {
+   rc = xen_set_upcall_vector(cpu);
+   if (rc) {
+   WARN(1, "HVMOP_set_evtchn_upcall_vector"
+" for CPU %d failed: %d\n", cpu, rc);
+   return rc;
+   }
+   }
+
if (xen_feature(XENFEAT_hvm_safe_pvclock))
xen_setup_timer(cpu);
 
@@ -188,8 +205,6 @@ static int xen_cpu_dead_hvm(unsigned int cpu)
return 0;
 }
 
-static bool no_vector_callback __initdata;
-
 static void __init xen_hvm_guest_init(void)
 {
if (xen_pv_domain())
@@ -211,9 +226,6 @@ static void __init xen_hvm_guest_init(void)
 

Re: [PATCH] xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation

[Xenia Ragiadakou]

Hi Jan,

On 7/29/22 09:16, Jan Beulich wrote:

On 29.07.2022 07:23, Xenia Ragiadakou wrote:

Hi Stefano,

On 7/29/22 01:56, Stefano Stabellini wrote:

On Thu, 28 Jul 2022, Julien Grall wrote:

On 28/07/2022 15:20, Jan Beulich wrote:

On 28.07.2022 15:56, Julien Grall wrote:

On 28/07/2022 14:49, Xenia Ragiadakou wrote:

--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -461,7 +461,7 @@
 /* Access to system registers */
#define WRITE_SYSREG64(v, name) do {\
-uint64_t _r = v;\
+uint64_t _r = (v);  \


I am failing to see why the parentheses are necessary here. Could you
give an example where the lack of them would end up to different code?


I think it is merely good practice to parenthesize the right sides of =.
Indeed with assignment operators having second to lowest precedence, and
with comma (the lowest precedence one) requiring parenthesization at the
macro invocation site, there should be no real need for parentheses here.


I am not really happy with adding those parentheses because they are
pointless. But if there are a consensus to use it, then the commit message
should be updated to clarify this is just here to please MISRA (to me "need"
implies it would be bug).


Let me premise that I don't know if this counts as a MISRA violation or
not. (Also I haven't checked if cppcheck/eclair report it as violation.)

But I think the reason for making the change would be to follow our
coding style / coding practices. It makes the code simpler to figure out
that it is correct, to review and maintain if we always add the
parenthesis even in cases like this one where they are not strictly
necessary. We are going to save our future selves some mental cycles.

So the explanation on the commit message could be along those lines.


First, the rule 20.7 states "Expressions resulting from the expansion of
macro parameters shall
   be enclosed in parentheses". So, here it is a clear violation of the
rule because the right side of the assignment operator is an expression.

Second, as I stated in a previous email, if v is not enclosed in
parentheses, I could write the story of my life in there and compile it
:) So, it would be a bug.

So, I recommend the title and the explanation i.e
"xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation

The macro parameter 'v' is used as an expression and needs to be enclosed in
   parentheses."
to remain as is because they are accurate.


I'm afraid you're following the MISRA wording too much to the latter.
Earlier on you agreed that when macro parameters are used as function
arguments, the parentheses can be omitted. Yet by what you say above
those are also expressions. 


Yes, those are also expressions (that's why I added parentheses 
initially) and I agreed with you that the parentheses there may not be 
necessary because I could not think of an example that will produce 
different behaviors with and without the parentheses. This will need a 
formal deviation I imagine or maybe a MISRA C expert could provide a 
justification regarding why parentheses are needed around function 
arguments that we may have not think of.



As indicated before - I think parentheses
are wanted here, but it's strictly "wanted", and hence the title
better wouldn't say "fix" (but e.g. "improve") and the description
also should be "softened".



Regarding the latter, are you saying that the parentheses are not needed?
In my opinion they are needed to prevent the bug described in the 
previous email i.e passing multiple statements to the macro.


By whom are they wanted? I 'm afraid I cannot understand.
Also, are you proposing to change the title into "Improve MISRA C 2012 
Rule 20.7 violation" ?


--
Xenia

Re: [xen-unstable-smoke bisection] complete build-amd64-libvirt

[Oleksandr Tyshchenko]

On 29.07.22 09:22, Jan Beulich wrote:

Hello Jan

> On 29.07.2022 03:04, osstest service owner wrote:
>> branch xen-unstable-smoke
>> xenbranch xen-unstable-smoke
>> job build-amd64-libvirt
>> testid libvirt-build
>>
>> Tree: libvirt git://xenbits.xen.org/libvirt.git
>> Tree: libvirt_keycodemapdb 
>> https://urldefense.com/v3/__https://gitlab.com/keycodemap/keycodemapdb.git__;!!GF_29dbcQIUBPA!0s_nyAgds977dw0dGPgFJGkIaBiKiXH3nR11Ni6gGjN5gQmB0DEhKrm5SUX4R0WhK8YkQemR6RVhiojpzij9yLM$
>>  [gitlab[.]com]
>> Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
>> Tree: qemuu git://xenbits.xen.org/qemu-xen.git
>> Tree: xen git://xenbits.xen.org/xen.git
>>
>> *** Found and reproduced problem changeset ***
>>
>>Bug is in tree:  xen git://xenbits.xen.org/xen.git
>>Bug introduced:  66dd1c62b2a3c707bd5c55750d10a8223fbd577f
>>Bug not present: f732240fd3bac25116151db5ddeb7203b62e85ce
>>Last fail repro: 
>> https://urldefense.com/v3/__http://logs.test-lab.xenproject.org/osstest/logs/171909/__;!!GF_29dbcQIUBPA!0s_nyAgds977dw0dGPgFJGkIaBiKiXH3nR11Ni6gGjN5gQmB0DEhKrm5SUX4R0WhK8YkQemR6RVhiojpmYABJkc$
>>  [logs[.]test-lab[.]xenproject[.]org]
>>
>>
>>commit 66dd1c62b2a3c707bd5c55750d10a8223fbd577f
>>Author: Oleksandr Tyshchenko 
>>Date:   Fri Jul 15 22:20:24 2022 +0300
>>
>>libxl: Add support for Virtio disk configuration
> Just in case you didn't notice it: Something's wrong here. I didn't look
> at the details at all. Please advise whether a fix will soon arrive or
> whether we should revert for the time being.

Sorry for the breakage. At the moment I have no idea what is wrong here,

I will try to investigate and provide a fix by the end of the day.


>
> Jan

-- 
Regards,

Oleksandr Tyshchenko

[linux-linus test] 171901: trouble: broken/fail/pass

[osstest service owner]

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

Failures and problems with tests :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-xl-qemut-debianhvm-amd64   broken
 test-amd64-amd64-xl-qemut-debianhvm-amd64 5 host-install(5) broken REGR. vs. 
171891

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

version targeted for testing:
 linux33ea1340bafe1f394e5bf96fceef73e9771d066b
baseline version:
 linux6e7765cb477a9753670d4351d14de93f1e9dbbd4

Last test of basis   171891  2022-07-28 00:42:47 Z1 days
Testing same since   171901  2022-07-28 20:11:14 Z0 days1 attempts


People who touched revisions under test:
  Abhishek Pandit-Subedi 
  Alejandro Lucero 
  Anirudh Venkataramanan 
  Benjamin Poirier 
  Christophe JAILLET 
  Dan Carpenter 
  Dave 

Re: [PATCH v3] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Xenia Ragiadakou]

Hi Jan,

On 7/29/22 09:26, Jan Beulich wrote:

On 28.07.2022 18:21, Xenia Ragiadakou wrote:

--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -63,7 +63,7 @@ static void do_idle(void)
  rcu_idle_exit(cpu);
  }
  
-void idle_loop(void)

+static void idle_loop(void)


While you're adding "noreturn" below, shouldn't this one be marked so
as well? Preferably with the addition:
Reviewed-by: Jan Beulich 



Yes, but I was not sure if this should go in this patch or in a separate 
one.


--
Xenia

Re: [PATCH v3] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Jan Beulich]

On 28.07.2022 18:21, Xenia Ragiadakou wrote:
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -63,7 +63,7 @@ static void do_idle(void)
>  rcu_idle_exit(cpu);
>  }
>  
> -void idle_loop(void)
> +static void idle_loop(void)

While you're adding "noreturn" below, shouldn't this one be marked so
as well? Preferably with the addition:
Reviewed-by: Jan Beulich 

Jan

Re: [xen-unstable-smoke bisection] complete build-amd64-libvirt

[Jan Beulich]

On 29.07.2022 03:04, osstest service owner wrote:
> branch xen-unstable-smoke
> xenbranch xen-unstable-smoke
> job build-amd64-libvirt
> testid libvirt-build
> 
> Tree: libvirt git://xenbits.xen.org/libvirt.git
> Tree: libvirt_keycodemapdb https://gitlab.com/keycodemap/keycodemapdb.git
> Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
> Tree: qemuu git://xenbits.xen.org/qemu-xen.git
> Tree: xen git://xenbits.xen.org/xen.git
> 
> *** Found and reproduced problem changeset ***
> 
>   Bug is in tree:  xen git://xenbits.xen.org/xen.git
>   Bug introduced:  66dd1c62b2a3c707bd5c55750d10a8223fbd577f
>   Bug not present: f732240fd3bac25116151db5ddeb7203b62e85ce
>   Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/171909/
> 
> 
>   commit 66dd1c62b2a3c707bd5c55750d10a8223fbd577f
>   Author: Oleksandr Tyshchenko 
>   Date:   Fri Jul 15 22:20:24 2022 +0300
>   
>   libxl: Add support for Virtio disk configuration

Just in case you didn't notice it: Something's wrong here. I didn't look
at the details at all. Please advise whether a fix will soon arrive or
whether we should revert for the time being.

Jan

Re: [PATCH] xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation

[Jan Beulich]

On 29.07.2022 07:23, Xenia Ragiadakou wrote:
> Hi Stefano,
> 
> On 7/29/22 01:56, Stefano Stabellini wrote:
>> On Thu, 28 Jul 2022, Julien Grall wrote:
>>> On 28/07/2022 15:20, Jan Beulich wrote:
 On 28.07.2022 15:56, Julien Grall wrote:
> On 28/07/2022 14:49, Xenia Ragiadakou wrote:
>> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
>> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
>> @@ -461,7 +461,7 @@
>> /* Access to system registers */
>>#define WRITE_SYSREG64(v, name) do {\
>> -uint64_t _r = v;\
>> +uint64_t _r = (v);  \
>
> I am failing to see why the parentheses are necessary here. Could you
> give an example where the lack of them would end up to different code?

 I think it is merely good practice to parenthesize the right sides of =.
 Indeed with assignment operators having second to lowest precedence, and
 with comma (the lowest precedence one) requiring parenthesization at the
 macro invocation site, there should be no real need for parentheses here.
>>>
>>> I am not really happy with adding those parentheses because they are
>>> pointless. But if there are a consensus to use it, then the commit message
>>> should be updated to clarify this is just here to please MISRA (to me "need"
>>> implies it would be bug).
>>
>> Let me premise that I don't know if this counts as a MISRA violation or
>> not. (Also I haven't checked if cppcheck/eclair report it as violation.)
>>
>> But I think the reason for making the change would be to follow our
>> coding style / coding practices. It makes the code simpler to figure out
>> that it is correct, to review and maintain if we always add the
>> parenthesis even in cases like this one where they are not strictly
>> necessary. We are going to save our future selves some mental cycles.
>>
>> So the explanation on the commit message could be along those lines.
> 
> First, the rule 20.7 states "Expressions resulting from the expansion of 
> macro parameters shall
>   be enclosed in parentheses". So, here it is a clear violation of the 
> rule because the right side of the assignment operator is an expression.
> 
> Second, as I stated in a previous email, if v is not enclosed in 
> parentheses, I could write the story of my life in there and compile it 
> :) So, it would be a bug.
> 
> So, I recommend the title and the explanation i.e
> "xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation
> 
> The macro parameter 'v' is used as an expression and needs to be enclosed in
>   parentheses."
> to remain as is because they are accurate.

I'm afraid you're following the MISRA wording too much to the latter.
Earlier on you agreed that when macro parameters are used as function
arguments, the parentheses can be omitted. Yet by what you say above
those are also expressions. As indicated before - I think parentheses
are wanted here, but it's strictly "wanted", and hence the title
better wouldn't say "fix" (but e.g. "improve") and the description
also should be "softened".

Jan

Re: [PATCH V7 10/11] xen/arm: translate virtual PCI bus topology for guests

[Jan Beulich]

On 28.07.2022 18:35, Oleksandr wrote:
> On 28.07.22 10:15, Jan Beulich wrote:
>> On 27.07.2022 21:39, Oleksandr wrote:
>>> On 27.07.22 20:54, Oleksandr wrote:
 On 26.07.22 18:16, Jan Beulich wrote:
> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>> --- a/xen/arch/arm/vpci.c
>> +++ b/xen/arch/arm/vpci.c
>> @@ -41,6 +41,16 @@ static int vpci_mmio_read(struct vcpu *v,
>> mmio_info_t *info,
>>    /* data is needed to prevent a pointer cast on 32bit */
>>    unsigned long data;
>>    +    /*
>> + * For the passed through devices we need to map their virtual
>> SBDF
>> + * to the physical PCI device being passed through.
>> + */
>> +    if ( !bridge && !vpci_translate_virtual_device(v->domain, ) )
>> +    {
>> +    *r = ~0ul;
>> +    return 1;
>> +    }
> I'm probably simply lacking specific Arm-side knowledge, but it strikes
> me as odd that the need for translation would be dependent upon
> "bridge".

 I am afraid I cannot answer immediately.

 I will analyze that question and provide an answer later on.
>>>
>>> Well, most likely that "valid" bridge pointer here is just used as an
>>> indicator of hwdom currently, so no need to perform virt->phys
>>> translation for sbdf.
>>>
>>> You can see that domain_vpci_init() passes a valid value for hwdom and
>>> NULL for other domains when setting up vpci_mmio* callbacks.
>> Oh, I see.
>>
>>> Alternatively, I guess we could use "!is_hardware_domain(v->domain)"
>>> instead of "!bridge" in the first part of that check. Shall I?
>> Maybe simply add a comment? Surely checking "bridge" is cheaper than
>> using is_hardware_domain(), so I can see the benefit. But the larger
>> arm/vpci.c grows, the less obvious the connection will be without a
>> comment.
> 
> 
> Agree the connection is worth a comment ...
> 
> 
> 
>>   (Instead of a comment, an alternative may be a suitable
>> assertion, which then documents the connection at the same time, e.g.
>> ASSERT(!bridge == !is_hardware_domain(v->domain)). But that won't be
>> possible in e.g. vpci_sbdf_from_gpa(), where apparently a similar
>> assumption is being made.)
> 
> 
>     ... or indeed to put such ASSERT _before_ vpci_sbdf_from_gpa().
> 
> This will cover assumption being made in both places.
> 
> 
> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> index a9fc5817f9..1d4b1ef39e 100644
> --- a/xen/arch/arm/vpci.c
> +++ b/xen/arch/arm/vpci.c
> @@ -37,10 +37,24 @@ static int vpci_mmio_read(struct vcpu *v, 
> mmio_info_t *info,
>     register_t *r, void *p)
>   {
>   struct pci_host_bridge *bridge = p;
> -    pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
> +    pci_sbdf_t sbdf;
>   /* data is needed to prevent a pointer cast on 32bit */
>   unsigned long data;
> 
> +    ASSERT(!bridge == !is_hardware_domain(v->domain));
> +
> +    sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
> +
> +    /*
> + * For the passed through devices we need to map their virtual SBDF
> + * to the physical PCI device being passed through.
> + */
> +    if ( !bridge && !vpci_translate_virtual_device(v->domain, ) )
> +    {
> +    *r = ~0ul;
> +    return 1;
> +    }
> +
>   if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>   1U << info->dabt.size, ) )
>   {
> @@ -57,7 +71,18 @@ static int vpci_mmio_write(struct vcpu *v, 
> mmio_info_t *info,
>      register_t r, void *p)
>   {
>   struct pci_host_bridge *bridge = p;
> -    pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
> +    pci_sbdf_t sbdf;
> +
> +    ASSERT(!bridge == !is_hardware_domain(v->domain));
> +
> +    sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
> +
> +    /*
> + * For the passed through devices we need to map their virtual SBDF
> + * to the physical PCI device being passed through.
> + */
> +    if ( !bridge && !vpci_translate_virtual_device(v->domain, ) )
> +    return 1;
> 
>   return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa),
>      1U << info->dabt.size, r);
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index d4601ecf9b..fc2c51dc3e 100644
> 
> 
> Any preference here?
> 
> 
> Personally, I think that such ASSERT will better explain the connection 
> than the comment will do.

Indeed I'd also prefer ASSERT()s being put there. But my opinion is
secondary here, as I'm not a maintainer of this code.

Jan