Re: [PATCH] xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation
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.
Xenia
[xen-unstable-smoke test] 171911: regressions - FAIL
flight 171911 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/171911/
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 days3 attempts
Testing same since 171911 2022-07-29 02:00:25 Z0 days1 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";
[qemu-mainline test] 171900: tolerable FAIL - PUSHED
flight 171900 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/171900/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 171897 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 171897 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 171897 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 171897 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 171897 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 171897 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 171897 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 171897 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-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-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 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-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-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 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: qemuua17001c42329f809c7f1768925b8089324564312 baseline version: qemuu3e4abe2c92964aadd35344a635b0f32cb487fd5c Last test of basis 171897 2022-07-28 12:08:28 Z0 days Testing same since 171900 2022-07-28 20:08:23 Z0 days1 attempts People who touched revisions under test: Alistair Francis Atish Patra Palmer Dabbelt
[xen-unstable-smoke test] 171903: regressions - FAIL
flight 171903 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/171903/
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 ca45d3cb4586372909f350e54482246f994e1bc7
baseline version:
xen f732240fd3bac25116151db5ddeb7203b62e85ce
Last test of basis 171884 2022-07-27 12:03:31 Z1 days
Testing same since 171899 2022-07-28 19:01:47 Z0 days2 attempts
People who touched revisions under test:
George Dunlap
Jiamei Xie
Julien Grall
Oleksandr Tyshchenko
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 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";
#iommu-cells = <0x01>;
phandle = <0xfde9>;
};
virtio@200 {
compatible = "virtio,mmio";
reg = <0x00 0x200 0x00 0x200>;
interrupts = <0x00 0x01 0xf01>;
interrupt-parent = <0xfde8>;
dma-coherent;
iommus = <0xfde9 0x01>;
};
virtio@2000200 {
compatible = "virtio,mmio";
reg = <0x00 0x2000200 0x00 0x200>;
interrupts = <0x00 0x02 0xf01>;
interrupt-parent = <0xfde8>;
dma-coherent;
iommus = <0xfde9 0x01>;
};
Signed-off-by: Oleksandr Tyshchenko
Reviewed-by: Stefano Stabellini
Reviewed-by: Anthony PERARD
commit 2128143c114c52c7536e37c32935fdd77f23edc1
Author: Julien Grall
Date: Fri Jul 15 22:20:25 2022 +0300
libxl: Introduce basic virtio-mmio support on Arm
This patch introduces helpers to allocate Virtio MMIO params
(IRQ and memory region) and create specific device node in
the Guest device-tree with allocated params. In order to deal
with multiple Virtio devices, reserve corresponding ranges.
For now, we reserve 1MB for memory regions and 10 SPIs.
As these helpers should be used for every Virtio device attached
to the Guest, call them for Virtio disk(s).
Please note, with statically allocated Virtio IRQs there is
a risk of a clash with a physical IRQs of passthrough devices.
For the first version, it's fine, but we should consider allocating
the Virtio IRQs automatically. Thankfully, we know in advance which
IRQs will be used for passthrough to be able to choose non-clashed
ones.
Signed-off-by: Julien Grall
Signed-off-by: Oleksandr Tyshchenko
RE: Proposal for Porting Xen to Armv8-R64 - DraftC (Archive for Day1)
Hi Stefano, > -Original Message- > From: Stefano Stabellini > Sent: 2022年7月29日 4:14 > To: Wei Chen > Cc: xen-devel@lists.xenproject.org; jul...@xen.org; Stefano Stabellini > ; Bertrand Marquis ; > Penny Zheng > Subject: Re: Proposal for Porting Xen to Armv8-R64 - DraftC (Archive for > Day1) > > Hi Wei, > > I am really sorry I didn't manage to read this proposal before now. Too > many things :-) > It doesn't matter : ) > Is it still valid? Do you want me to read it in the next days, or do you > have another update you would rather send first? > Yes it's still valid, our day1 patch hasn't been sent because we are want some pre-series from Henry to be merged first. If you can give more comments, we can also make changes for day 1 patches and update the proposal. Cheers, Wei Chen > Cheers, > > Stefano > > > On Sun, 8 May 2022, Wei Chen wrote: > > # Proposal for Porting Xen to Armv8-R64 > > > > (Update to Draft-C as a discussion archive for Day1 patches) > > > > This proposal will introduce the PoC work of porting Xen to Armv8-R64, > > which includes: > > - The changes of current Xen capability, like Xen build system, memory > > management, domain management, vCPU context switch. > > - The expanded Xen capability, like static-allocation and direct-map. > > > > ***Notes:*** > > 1. ***This proposal only covers the work of porting Xen to Armv8-R64*** > >***single CPU.Xen SMP support on Armv8-R64 relates to Armv8-R*** > >***Trusted-Frimware (TF-R). This is an external dependency,*** > >***so we think the discussion of Xen SMP support on Armv8-R64*** > >***should be started when single-CPU support is complete.*** > > 2. ***This proposal will not touch xen-tools. In current stange,*** > >***Xen on Armv8-R64 only support dom0less, all guests should*** > >***be booted from device tree.*** > > > > ## Changelogs > > Draft-B -> Draft-C: > > 1. Update the event-channel support section to use runtime > >MPU mapping instead of whole MPU context switch. > > 2. Split two stages for Armv8-R64 Xen start address solution. > > 3. Remove "malicious tools". > > 4. Remove GCC version bump. > > > > Draft-A -> Draft-B: > > 1. Update Kconfig options usage. > > 2. Update the section for XEN_START_ADDRESS. > > 3. Add description of MPU initialization before parsing device tree. > > 4. Remove CONFIG_ARM_MPU_EL1_PROTECTION_REGIONS. > > 5. Update the description of ioremap_nocache/cache. > > 6. Update about the free_init_memory on Armv8-R. > > 7. Describe why we need to switch the MPU configuration later. > > 8. Add alternative proposal in TODO. > > 9. Add use tool to generate Xen Armv8-R device tree in TODO. > > 10. Add Xen PIC/PIE discussion in TODO. > > 11. Add Xen event channel support in TODO. > > > > ## Contributors: > > Wei Chen > > Penny Zheng > > > > ## 1. Essential Background > > > > ### 1.1. Armv8-R64 Profile > > The Armv-R architecture profile was designed to support use cases that > > have a high sensitivity to deterministic execution. (e.g. Fuel Injection, > > Brake control, Drive trains, Motor control etc) > > > > Arm announced Armv8-R in 2013, it is the latest generation Arm > architecture > > targeted at the Real-time profile. It introduces virtualization at the > highest > > security level while retaining the Protected Memory System Architecture > (PMSA) > > based on a Memory Protection Unit (MPU). In 2020, Arm announced Cortex- > R82, > > which is the first Arm 64-bit Cortex-R processor based on Armv8-R64. > > > > - The latest Armv8-R64 document can be found here: > > [Arm Architecture Reference Manual Supplement - Armv8, for Armv8-R > AArch64 architecture > profile](https://developer.arm.com/documentation/ddi0600/latest/). > > > > - Armv-R Architecture progression: > > Armv7-R -> Armv8-R AArch32 -> Armv8 AArch64 > > The following figure is a simple comparison of "R" processors based on > > different Armv-R Architectures. > >  > > > > - The Armv8-R architecture evolved additional features on top of Armv7-R: > > - An exception model that is compatible with the Armv8-A model > > - Virtualization with support for guest operating systems > > - PMSA virtualization using MPUs In EL2. > > - The new features of Armv8-R64 architecture > > - Adds support for the 64-bit A64 instruction set, previously Armv8- > R > > only supported A32. > > - Supports up to 48-bit physical addressing, previously up to 32-bit > > addressing was supported. > > - Optional Arm Neon technology and Advanced SIMD > > - Supports three Exception Levels (ELs) > > - Secure EL2 - The Highest Privilege, MPU only, for firmware, > hypervisor > > - Secure EL1 - RichOS (MMU) or RTOS (MPU) > > - Secure EL0 - Application Workloads > > - Optionally supports Virtual Memory System Architecture at S-EL1/S- > EL0. > > This means it's possible to
[xen-unstable-smoke bisection] complete build-amd64-libvirt
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
This patch adds basic support for configuring and assisting virtio-mmio
based virtio-disk backend (emulator) which is intended to run out of
Qemu and could be run in any domain.
Although the Virtio block device is quite different from traditional
Xen PV block device (vbd) from the toolstack's point of view:
- as the frontend is virtio-blk which is not a Xenbus driver, nothing
written to Xenstore are fetched by the frontend currently ("vdev"
is not passed to the frontend). But this might need to be revised
in future, so frontend data might be written to Xenstore in order to
support hotplugging virtio devices or passing the backend domain id
on arch where the device-tree is not available.
- the ring-ref/event-channel are not used for the backend<->frontend
communication, the proposed IPC for Virtio is IOREQ/DM
it is still a "block device" and ought to be integrated in existing
"disk" handling. So, re-use (and adapt) "disk" parsing/configuration
logic to deal with Virtio devices as well.
For the immediate purpose and an ability to extend that support for
other use-cases in future (Qemu, virtio-pci, etc) perform the following
actions:
- Add new disk backend type (LIBXL_DISK_BACKEND_STANDALONE) and reflect
that in the configuration
- Introduce new disk "specification" and "transport" fields to struct
libxl_device_disk. Both are written to the Xenstore. The transport
field is only used for the specification "virtio" and it assumes
only "mmio" value for now.
- Introduce new "specification" option with "xen" communication
protocol being default value.
- Add new device kind (LIBXL__DEVICE_KIND_VIRTIO_DISK) as current
one (LIBXL__DEVICE_KIND_VBD) doesn't fit into Virtio disk model
An example of domain configuration for Virtio disk:
disk = [ 'phy:/dev/mmcblk0p3, xvda1, backendtype=standalone,
specification=virtio']
Nothing has changed for default Xen disk configuration.
Please note, this patch is not enough for virtio-disk to work
on Xen (Arm), as for every Virtio device (including disk) we need
to allocate Virtio MMIO params (IRQ and memory region) and pass
them to the backend, also update Guest device-tree. The subsequent
patch will add these missing bits. For the current patch,
the default "irq" and "base" are just written to the Xenstore.
This is not an ideal splitting, but this way we avoid breaking
the bisectability.
Signed-off-by: Oleksandr Tyshchenko
Reviewed-by: Anthony PERARD
Acked-by: George Dunlap
Tested-by: Jiamei Xie
For bisection revision-tuple graph see:
http://logs.test-lab.xenproject.org/osstest/results/bisect/xen-unstable-smoke/build-amd64-libvirt.libvirt-build.html
Revision IDs in each graph node refer, respectively, to the Trees above.
Running cs-bisection-step
--graph-out=/home/logs/results/bisect/xen-unstable-smoke/build-amd64-libvirt.libvirt-build
--summary-out=tmp/171909.bisection-summary --basis-template=171884
--blessings=real,real-bisect,real-retry xen-unstable-smoke build-amd64-libvirt
libvirt-build
Searching for failure / basis pass:
171899 fail [host=himrod1] / 171884 ok.
Failure / basis pass flights: 171899 / 171884
(tree with no url: minios)
(tree with no url: ovmf)
(tree with no url: seabios)
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
Latest 2c846fa6bcc11929c9fb857a22430fb9945654ad
27acf0ef828bf719b2053ba398b195829413dbdd
3d273dd05e51e5a1ffba3d98c7437ee84e8f8764
b746458e1ce1bec85e58b458386f8b7a0bedfaa6
ca45d3cb4586372909f350e54482246f994e1bc7
Basis pass 2c846fa6bcc11929c9fb857a22430fb9945654ad
27acf0ef828bf719b2053ba398b195829413dbdd
[PATCH] automation: disable xen,enhanced in qemu-smoke-arm64
Disable xen,enhanced because we don't use PV drivers in this test and also because the kernel used for testing is old and unpatched and would break if xen,enhanced is passed. This patch unbreaks gitlab-ci. Signed-off-by: Stefano Stabellini --- automation/scripts/qemu-smoke-arm64.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/automation/scripts/qemu-smoke-arm64.sh b/automation/scripts/qemu-smoke-arm64.sh index 898398196a..e6f1510c0f 100755 --- a/automation/scripts/qemu-smoke-arm64.sh +++ b/automation/scripts/qemu-smoke-arm64.sh @@ -84,6 +84,7 @@ NUM_DOMUS=1 DOMU_KERNEL[0]="Image" DOMU_RAMDISK[0]="initrd" DOMU_MEM[0]="256" +DOMU_ENHANCED[0]=0 LOAD_CMD="tftpb" UBOOT_SOURCE="boot.source" -- 2.25.1
Re: [PATCH v3 2/2] automation: arm64: Create a test job for testing static allocation on qemu
On Thu, 28 Jul 2022, Xenia Ragiadakou wrote:
> 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
> Reviewed-by: Penny Zheng
> ---
>
> Changes in v2:
> - enable CONFIG_STATIC_MEMORY for all arm64 builds
> - use the existing qemu-smoke-arm64.sh with an argument passed for
> distinguishing between tests, instead of creating a new script
> - test does not rely on kernel logs but prints a message itself on success
> - remove the part that enables custom configs for arm64 builds
> - add comments
> - adapt commit message
>
> Changes in v3:
> - refactor the changes to improve readability, no functionality change
> intended
> - add Penny's R-b tag
>
> automation/gitlab-ci/test.yaml | 18 ++
> automation/scripts/build | 10 +-
> automation/scripts/qemu-smoke-arm64.sh | 25 -
> 3 files changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
> index 26bdbcc3ea..6f9f64a8da 100644
> --- a/automation/gitlab-ci/test.yaml
> +++ b/automation/gitlab-ci/test.yaml
> @@ -80,6 +80,24 @@ qemu-smoke-arm64-gcc:
>tags:
> - arm64
>
> +qemu-smoke-arm64-gcc-staticmem:
> + extends: .test-jobs-common
> + variables:
> +CONTAINER: debian:unstable-arm64v8
> + script:
> +- ./automation/scripts/qemu-smoke-arm64.sh static-mem 2>&1 | tee
> qemu-smoke-arm64.log
> + needs:
> +- debian-unstable-gcc-arm64
> +- kernel-5.9.9-arm64-export
> +- qemu-system-aarch64-6.0.0-arm64-export
> + artifacts:
> +paths:
> + - smoke.serial
> + - '*.log'
> +when: always
> + tags:
> +- arm64
> +
> qemu-smoke-arm32-gcc:
>extends: .test-jobs-common
>variables:
> diff --git a/automation/scripts/build b/automation/scripts/build
> index 21b3bc57c8..2b9f2d2b54 100755
> --- a/automation/scripts/build
> +++ b/automation/scripts/build
> @@ -15,7 +15,15 @@ if [[ "${RANDCONFIG}" == "y" ]]; then
> make -j$(nproc) -C xen KCONFIG_ALLCONFIG=tools/kconfig/allrandom.config
> randconfig
> hypervisor_only="y"
> else
> -make -j$(nproc) -C xen defconfig
> +if [[ "${XEN_TARGET_ARCH}" = "arm64" ]]; then
> +echo "
> +CONFIG_EXPERT=y
> +CONFIG_UNSUPPORTED=y
> +CONFIG_STATIC_MEMORY=y" > xen/.config
> +make -j$(nproc) -C xen olddefconfig
> +else
> +make -j$(nproc) -C xen defconfig
> +fi
> fi
>
> # Save the config file before building because build failure causes the
> script
> diff --git a/automation/scripts/qemu-smoke-arm64.sh
> b/automation/scripts/qemu-smoke-arm64.sh
> index 53086a5ac7..69d9eae7a9 100755
> --- a/automation/scripts/qemu-smoke-arm64.sh
> +++ b/automation/scripts/qemu-smoke-arm64.sh
> @@ -2,6 +2,23 @@
>
> set -ex
>
> +test_variant=$1
> +
> +passed="BusyBox"
> +check=""
> +
> +if [[ "${test_variant}" == "static-mem" ]]; then
> +# Memory range that is statically allocated to DOM1
> +domu_base="5000"
> +domu_size="1000"
> +passed="${test_variant} test passed"
> +check="current=\$(hexdump -e '16/1 \"%02x\"'
> /proc/device-tree/memory@${domu_base}/reg 2>/dev/null)
> +expected=$(printf \"%016x%016x\" 0x${domu_base} 0x${domu_size})
> +if [[ \"\${expected}\" == \"\${current}\" ]]; then
> + echo \"${passed}\"
> +fi"
> +fi
I would make a minor code style improvement here:
+if [[ "${test_variant}" == "static-mem" ]]; then
+# Memory range that is statically allocated to DOM1
+domu_base="5000"
+domu_size="1000"
+passed="${test_variant} test passed"
+check="
+current=\$(hexdump -e '16/1 \"%02x\"'
/proc/device-tree/memory@${domu_base}/reg 2>/dev/null)
+expected=$(printf \"%016x%016x\" 0x${domu_base} 0x${domu_size})
+if [[ \"\${expected}\" == \"\${current}\" ]]; then
+ echo \"${passed}\"
+fi
+"
+fi
I can do that on commit. Everything looks good.
Reviewed-by: Stefano Stabellini
Re: [PATCH v3] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
On 7/28/22 8:52 AM, Jane Malalane wrote:
+/*
+ * Setup per-vCPU vector-type callbacks and trick toolstack to think
The comment should be adjusted -- no need to mention toolstack now that that
code has been factored out.
Other than that
Reviewed-by: Boris Ostrovsky
+ * we are enlightened. If this setup is unavailable, fallback to the
+ * global vector-type callback.
+ */
+static __init void xen_init_setup_upcall_vector(void)
+{
Re: [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation
On Thu, 28 Jul 2022, Bertrand Marquis wrote: > > On 28 Jul 2022, at 11:21, Julien Grall wrote: > > On 28/07/2022 10:45, Bertrand Marquis wrote: > >>> On 28 Jul 2022, at 10:35, Julien Grall wrote: > >>> On 28/07/2022 08:57, Bertrand Marquis wrote: > Hi Julien, > >>> > >>> Hi Bertrand, > >>> > > On 27 Jul 2022, at 16:46, Julien Grall wrote: > > > > Hi Xenia, > > > > On 27/07/2022 16:32, Xenia Ragiadakou wrote: > >> Remove unused macro atomic_xchg(). > >> Signed-off-by: Xenia Ragiadakou > >> --- > >> xen/arch/arm/include/asm/atomic.h | 2 -- > >> 1 file changed, 2 deletions(-) > >> diff --git a/xen/arch/arm/include/asm/atomic.h > >> b/xen/arch/arm/include/asm/atomic.h > >> index f5ef744b4b..a2dc125291 100644 > >> --- a/xen/arch/arm/include/asm/atomic.h > >> +++ b/xen/arch/arm/include/asm/atomic.h > >> @@ -223,8 +223,6 @@ static inline int atomic_add_unless(atomic_t *v, > >> int a, int u) > >> return __atomic_add_unless(v, a, u); > >> } > >> -#define atomic_xchg(v, new) (xchg(&((v)->counter), new)) > >> - > > > > While I agree this is unused today, the wrapper is quite trivial and > > part of the generic API (x86 also provides one). So I am not in favor > > of removing it just to please MISRA. > > > > That said, if Bertrand and Stefano agrees with removing it then you > > should also remove the x86 version to avoid inconsistency. > I think we can keep this and maybe add a comment on top to document a > known violation: > /* TODO: MISRA_VIOLATION 2.5 */ > >>> > >>> While I am fine with this goal of the comment (i.e. indicating where Xen > >>> is not MISRA compliant), I think this is one place where I would rather > >>> not want one because it can get stale if someones decide to use the > >>> function. > >> I think the one doing that will have to update the comment otherwise we > >> will never manage to have an analysis without findings. > > > > I was under the impression that Xen will never officially follow some of > > the MISRA rules. So I would expect the tools to be able to detect such > > cases so we don't have to add a comment for every deviation on something we > > will never support. > > > >> Having those kind of comments in the code for violation also means that > >> they must be updated if the violation is solved. > > > > Right, but for thing like unused function, this is quite easy to miss by > > both the developer and reviewers. So we are going to end up to comments for > > nothing. > > > >> Maybe we will need a run ignoring those to identify possible violations > >> which are not violations anymore but this might be hard to do. > > > > TBH, I think it would be best if we can teach the tools to ignore certain > > rules. > > Definitely it is possible to instruct the tool to ignore this you are right > and for 2.5 we should (for some reason I was under the impression that we > said we would follow 2.5 but accept deviations). Absolutely possible, basically we (the community) are the ones providing the list of rules to the MISRA C checkers. > @Xenia: please ignore and do not add a comment for this. > > I think we will need to distinguish 2 kind of not following: > - not following at all (disable in the tools) > - accepting some deviations (documented in the code) Yes, exactly right. > As much as we can, I think we should target the second unless we have a lot > of violations. +1
Re: [PATCH] xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation
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.
[xen-unstable-smoke test] 171899: regressions - FAIL
flight 171899 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/171899/
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 ca45d3cb4586372909f350e54482246f994e1bc7
baseline version:
xen f732240fd3bac25116151db5ddeb7203b62e85ce
Last test of basis 171884 2022-07-27 12:03:31 Z1 days
Testing same since 171899 2022-07-28 19:01:47 Z0 days1 attempts
People who touched revisions under test:
George Dunlap
Jiamei Xie
Julien Grall
Oleksandr Tyshchenko
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 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";
#iommu-cells = <0x01>;
phandle = <0xfde9>;
};
virtio@200 {
compatible = "virtio,mmio";
reg = <0x00 0x200 0x00 0x200>;
interrupts = <0x00 0x01 0xf01>;
interrupt-parent = <0xfde8>;
dma-coherent;
iommus = <0xfde9 0x01>;
};
virtio@2000200 {
compatible = "virtio,mmio";
reg = <0x00 0x2000200 0x00 0x200>;
interrupts = <0x00 0x02 0xf01>;
interrupt-parent = <0xfde8>;
dma-coherent;
iommus = <0xfde9 0x01>;
};
Signed-off-by: Oleksandr Tyshchenko
Reviewed-by: Stefano Stabellini
Reviewed-by: Anthony PERARD
commit 2128143c114c52c7536e37c32935fdd77f23edc1
Author: Julien Grall
Date: Fri Jul 15 22:20:25 2022 +0300
libxl: Introduce basic virtio-mmio support on Arm
This patch introduces helpers to allocate Virtio MMIO params
(IRQ and memory region) and create specific device node in
the Guest device-tree with allocated params. In order to deal
with multiple Virtio devices, reserve corresponding ranges.
For now, we reserve 1MB for memory regions and 10 SPIs.
As these helpers should be used for every Virtio device attached
to the Guest, call them for Virtio disk(s).
Please note, with statically allocated Virtio IRQs there is
a risk of a clash with a physical IRQs of passthrough devices.
For the first version, it's fine, but we should consider allocating
the Virtio IRQs automatically. Thankfully, we know in advance which
IRQs will be used for passthrough to be able to choose non-clashed
ones.
Signed-off-by: Julien Grall
Signed-off-by: Oleksandr Tyshchenko
Re: Question to mem-path support at QEMU for Xen
On Thu, 28 Jul 2022, Igor Mammedov wrote: > On Thu, 28 Jul 2022 15:17:49 +0800 > Huang Rui wrote: > > > Hi Igor, > > > > Appreciate you for the reply! > > > > On Wed, Jul 27, 2022 at 04:19:30PM +0800, Igor Mammedov wrote: > > > On Tue, 26 Jul 2022 15:27:07 +0800 > > > Huang Rui wrote: > > > > > > > Hi Anthony and other Qemu/Xen guys, > > > > > > > > We are trying to enable venus on Xen virtualization platform. And we > > > > would > > > > like to use the backend memory with memory-backend-memfd,id=mem1,size=4G > > > > options on QEMU, however, the QEMU will tell us the "-mem-path" is not > > > > supported with Xen. I verified the same function on KVM. > > > > > > > > qemu-system-i386: -mem-path not supported with Xen > > > > > > > > So may I know whether Xen has any limitation that support > > > > memory-backend-memfd in QEMU or just implementation is not done yet? > > > > > > Currently Xen doesn't use guest RAM allocation the way the rest of > > > accelerators do. (it has hacks in memory_region/ramblock API, > > > that divert RAM allocation calls to Xen specific API) > > > > I am new for Xen and QEMU, we are working on GPU. We would like to have a > > piece of backend memroy like video memory for VirtIO GPU to support guest > > VM Mesa Vulkan (Venus). Do you mean we can the memory_region/ramblock APIs > > to work around it? > > > > > > > > The sane way would extend Xen to accept RAM regions (whatever they are > > > ram or fd based) QEMU allocates instead of going its own way. This way > > > it could reuse all memory backends that QEMU provides for the rest of > > > the non-Xen world. (not to mention that we could drop non trivial > > > Xen hacks so that guest RAM handling would be consistent with other > > > accelerators) > > > > > > > May I know what do you mean by "going its own way"? This sounds good, could > > you please elaborate on how can we implement this? We would like to give a > > try to address the problem on Xen. Would you mind to point somewhere that I > > can learn and understand the RAM region. Very happy to see your > > suggestions! > > see for example see ram_block_add(), if Xen could be persuaded to use memory > allocated by '!xen_enabled()' branch then it's likely file base backends > would also become usable. > > Whether it is possible for Xen or not I don't know, > I guess CCed Xen folks will suggest something useful. Hi Igor, Huang, I think there is room for improvement in the way Xen support in QEMU is implemented, especially because it predates support for other accelerators in QEMU. I am happy to help come up with a good idea or two on how to do it better. At the same time it is not trivial. The way Xen works with QEMU is that Xen is the one allocating memory for the VM. Keep in mind that in a Xen setup, the VM where QEMU is running is just one of the many VMs in the system (even if it is dom0) and doesn't have ownership over the entire memory. It is also possible to run QEMU in a regular guest for security benefits. Given that, Xen is typically the one allocating memory for the VM and by the time QEMU is started the main memory is already allocated. That is the reason why normally ram_block_add() is implemented in a special way for Xen using xen_ram_alloc. In most cases, there is no need to allocate any memory at all. However, it is also possible for QEMU to allocate memory for the VM. It is done by QEMU issuing a hypercall to Xen. See for instance the call to xc_domain_populate_physmap_exact. As you can see from the implementation of xen_ram_alloc, it is already used for things that are not normal memory (see the check mr == _memory). So if you want to allocate memory for the VM, you could use xc_domain_populate_physmap_exact. Another thing to note is that Xen can connect to multiple emulators at the same time. Each emulator (each instance of QEMU, or other emulators) uses a hypercall to connect to Xen and register a PCI device or memory range that it is emulating. Each emulator is called "IOREQ server" in Xen terminology because it is handling "IO emulation REQuests" from Xen. The IOREQ interface is very simple. each IOREQ is defined a ioreq_t struct, see: xen/include/public/hvm/ioreq.h:ioreq_t So if you are looking to connect a secondary emulator to QEMU, a different, more efficient, way to solve the problem on Xen would be to connect the secondary emulator directly to Xen. Instead of: Xen -> QEMU -> secondary emulator You would do: Xen ---> QEMU | +-> secondary emulator The second approach is much faster in terms of latency because you are going to skip a step in the notification chain. See how QEMU calls xen_register_ioreq to register itself with Xen.
Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port
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? Cheers, -- Julien Grall
Re: Enable audio virtualization in Xen
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
I have now been able to reproduce the HVM guest audio failure that you
reported, with the same versions of Ubuntu, Xen and Qemu that you
indicated. I have also been able to correct it and successfully enable
guest audio output. I hope we can enable the same for you.
The issues I encountered are:
1) The command line arguments passed to qemu from libxl need some
adjustment for this system configuration.
2) Pulseaudio on the host runs as the desktop session user, whereas
the qemu device emulator runs as root, and some configuration change
is required to connect the two.
I also had to ensure that I had a version of Qemu with SDL enabled to
match your configuration, and a recent enough version of seabios to
successfully boot the guest.
The following are workarounds. A new patch to libxl would be
appropriate for improving handling of passing audio configuration from
VM config files to qemu, and there is likely a better way of enabling
host audio configuration for qemu than my outline below, but this
appears to work and is hopefully sufficient for unblocking your
progress.
To work around the command line arguments to qemu:
# Move the qemu-system-i386 binary to a different filename:
mv /usr/lib/xen/bin/qemu-system-i386 /usr/lib/xen/bin/qemu-system-i386.real
# Create a wrapper script file in place of the previous binary, to add
the audio parameters: /usr/lib/xen/bin/qemu-system-i386
#!/bin/sh
/usr/lib/xen/bin/qemu-system-i386.real -audiodev id=snd0,driver=pa
-device ich9-intel-hda -device hda-output,audiodev=snd0 $@
chmod 755 /usr/lib/xen/bin/qemu-system-i386
Ensure that you have removed any sound configuration from the VM
config file; the wrapper script will be providing this now instead.
To enable sound from the qemu process that is running as root, I
followed the answer on this page:
https://stackoverflow.com/questions/66775654/how-can-i-make-pulseaudio-run-as-root
Summarizing those pulseaudio configuration steps here, perform the
following as root:
# Write /etc/systemd/system/pulseaudio.service file:
[Unit]
Description=PulseAudio system server
[Service]
Type=notify
ExecStart=pulseaudio --daemonize=no --system --realtime --log-target=journal
[Install]
WantedBy=multi-user.target
# Enable the new service:
sudo systemctl --system enable pulseaudio.service
sudo systemctl --system start pulseaudio.service
sudo systemctl --system status pulseaudio.service
# Edit items in : /etc/pulse/client.conf
default-server = /var/run/pulse/native
autospawn = no
# Add root to pulse group
sudo adduser root pulse-access
# Also add your desktop user to pulse group, to retain working audio
for that host desktop user
sudo adduser $USER pulse-access
# Reboot
reboot
You should now be able to start your guest VM with working audio. I
would note that the qemu-dm log files in /var/log/xen/ can be useful
for observing error messages from Qemu if you still encounter
difficulty.
> Also, how can I make use of the xen front end drivers in a HVM guest?
I don't believe that the Xen audio front end drivers are necessary for
enabling HVM guest audio output, and I don't have experience with
using them yet but I am interested to hear of reports of using them on
x86.
thanks,
Christopher
>
> -Original Message-
> From: SHARMA, JYOTIRMOY
> Sent: Tuesday, July 26, 2022 4:27 PM
> To: Christopher Clark
> Cc: xen-devel@lists.xenproject.org; xen-us...@lists.xenproject.org
> Subject: RE: Enable audio virtualization in Xen
>
> [AMD Official Use Only - General]
>
> Hi Christopher,
>
> Thank you for the quick response. Please find answers below.
>
> >> Does audio playback work OK from your Ubuntu dom0?
> Yes.
>
> >> Do you know which version of Ubuntu you are using? ('cat /etc/lsb-release')
> Ubuntu 22.04 LTS.
>
> >> Do you know which version of Xen you are using? ('xl info' in dom0 should
> >> help)
> 4.16.2-pre.
>
> >> Do you know which version of Qemu you have installed in dom0?
> QEMU emulator version 6.1.1.
>
> >> Did you build and install Xen from source code, or are you using binary
> >> packages of Xen and its tools?
> We built and installed Xen from source code (git clone
> https://xenbits.xen.org/git-http/xen.git).
>
> >> Are you using the xl tools, or libvirt tools for configuring and running
> >> your guest? -- ie. how do you start your domU VM?
> We are using xl tools (sudo xl -v create ).
>
> >> When your domU is running, please could you run 'ps auxwww' in dom0 and
> >> obtain the process information about the qemu instance that is running, so
> >> that we can see what command line arguments have been supplied to it
>
> This is the log we get right after booting dom 0:
>
> root 723 0.0 0.2 243792 14468 ? Sl 15:51 0:00
> /usr/local/lib/xen/bin/qemu-system-i386 -xen-domid 0 -xen-attach -name dom0
> -nographic -M xenpv -daemonize -monitor
Re: Proposal for Porting Xen to Armv8-R64 - DraftC (Archive for Day1)
Hi Wei, I am really sorry I didn't manage to read this proposal before now. Too many things :-) Is it still valid? Do you want me to read it in the next days, or do you have another update you would rather send first? Cheers, Stefano On Sun, 8 May 2022, Wei Chen wrote: > # Proposal for Porting Xen to Armv8-R64 > > (Update to Draft-C as a discussion archive for Day1 patches) > > This proposal will introduce the PoC work of porting Xen to Armv8-R64, > which includes: > - The changes of current Xen capability, like Xen build system, memory > management, domain management, vCPU context switch. > - The expanded Xen capability, like static-allocation and direct-map. > > ***Notes:*** > 1. ***This proposal only covers the work of porting Xen to Armv8-R64*** >***single CPU.Xen SMP support on Armv8-R64 relates to Armv8-R*** >***Trusted-Frimware (TF-R). This is an external dependency,*** >***so we think the discussion of Xen SMP support on Armv8-R64*** >***should be started when single-CPU support is complete.*** > 2. ***This proposal will not touch xen-tools. In current stange,*** >***Xen on Armv8-R64 only support dom0less, all guests should*** >***be booted from device tree.*** > > ## Changelogs > Draft-B -> Draft-C: > 1. Update the event-channel support section to use runtime >MPU mapping instead of whole MPU context switch. > 2. Split two stages for Armv8-R64 Xen start address solution. > 3. Remove "malicious tools". > 4. Remove GCC version bump. > > Draft-A -> Draft-B: > 1. Update Kconfig options usage. > 2. Update the section for XEN_START_ADDRESS. > 3. Add description of MPU initialization before parsing device tree. > 4. Remove CONFIG_ARM_MPU_EL1_PROTECTION_REGIONS. > 5. Update the description of ioremap_nocache/cache. > 6. Update about the free_init_memory on Armv8-R. > 7. Describe why we need to switch the MPU configuration later. > 8. Add alternative proposal in TODO. > 9. Add use tool to generate Xen Armv8-R device tree in TODO. > 10. Add Xen PIC/PIE discussion in TODO. > 11. Add Xen event channel support in TODO. > > ## Contributors: > Wei Chen > Penny Zheng > > ## 1. Essential Background > > ### 1.1. Armv8-R64 Profile > The Armv-R architecture profile was designed to support use cases that > have a high sensitivity to deterministic execution. (e.g. Fuel Injection, > Brake control, Drive trains, Motor control etc) > > Arm announced Armv8-R in 2013, it is the latest generation Arm architecture > targeted at the Real-time profile. It introduces virtualization at the highest > security level while retaining the Protected Memory System Architecture (PMSA) > based on a Memory Protection Unit (MPU). In 2020, Arm announced Cortex-R82, > which is the first Arm 64-bit Cortex-R processor based on Armv8-R64. > > - The latest Armv8-R64 document can be found here: > [Arm Architecture Reference Manual Supplement - Armv8, for Armv8-R AArch64 > architecture > profile](https://developer.arm.com/documentation/ddi0600/latest/). > > - Armv-R Architecture progression: > Armv7-R -> Armv8-R AArch32 -> Armv8 AArch64 > The following figure is a simple comparison of "R" processors based on > different Armv-R Architectures. > >  > > - The Armv8-R architecture evolved additional features on top of Armv7-R: > - An exception model that is compatible with the Armv8-A model > - Virtualization with support for guest operating systems > - PMSA virtualization using MPUs In EL2. > - The new features of Armv8-R64 architecture > - Adds support for the 64-bit A64 instruction set, previously Armv8-R > only supported A32. > - Supports up to 48-bit physical addressing, previously up to 32-bit > addressing was supported. > - Optional Arm Neon technology and Advanced SIMD > - Supports three Exception Levels (ELs) > - Secure EL2 - The Highest Privilege, MPU only, for firmware, > hypervisor > - Secure EL1 - RichOS (MMU) or RTOS (MPU) > - Secure EL0 - Application Workloads > - Optionally supports Virtual Memory System Architecture at S-EL1/S-EL0. > This means it's possible to run rich OS kernels - like Linux - either > bare-metal or as a guest. > - Differences with the Armv8-A AArch64 architecture > - Supports only a single Security state - Secure. There is not Non-Secure > execution state supported. > - EL3 is not supported, EL2 is mandatory. This means secure EL2 is the > highest EL. > - Supports the A64 ISA instruction > - With a small set of well-defined differences > - Provides a PMSA (Protected Memory System Architecture) based > virtualization model. > - As opposed to Armv8-A AArch64's VMSA based Virtualization > - Can support address bits up to 52 if FEAT_LPA is enabled, > otherwise 48 bits. > - Determines the access permissions and memory
Re: [PATCH v1] xen: add late init call in start_xen
Hi Boyoun, Thanks for your interest in Xen and thanks for your contribution! 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). > > 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.
[qemu-mainline test] 171897: tolerable FAIL - PUSHED
flight 171897 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/171897/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-rtds18 guest-start/debian.repeat fail REGR. vs. 171885 Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 171885 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 171885 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 171885 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 171885 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 171885 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 171885 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 171885 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 171885 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-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-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 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-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-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 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: qemuu3e4abe2c92964aadd35344a635b0f32cb487fd5c baseline version: qemuu7b17a1a841fc2336eba53afade9cadb14bd3dd9a Last test of basis 171885 2022-07-27 12:04:19 Z1 days Testing same since 171897 2022-07-28 12:08:28 Z0 days1 attempts
Re: [PATCH v4 2/2] xen/arm: add FF-A mediator
Hi,
On 27/07/2022 07:33, Jens Wiklander wrote:
On Fri, Jul 8, 2022 at 9:54 PM Julien Grall wrote:
+unsigned int n;
+unsigned int m;
+p2m_type_t t;
+uint64_t addr;
+
+for ( n = 0; n < range_count; n++ )
+{
+for ( m = 0; m < range[n].page_count; m++ )
+{
+if ( pg_idx >= shm->page_count )
+return FFA_RET_INVALID_PARAMETERS;
Shouldn't we call put_page() to drop the references taken by
get_page_from_gfn()?
Yes, and that's done by put_shm_pages(). One would normally expect
get_shm_pages() to do this on error, but that's not needed here since
we're always calling put_shm_pages() just before freeing the shm. I
can change to let get_shm_pages() do the cleanup on error instead if
you prefer that.
I am fine with the current approach. I would suggest to document it on
top of get_shm_pages().
Also, if you expect put_shm_pages() to always be called before freeing
shm, then I think it would be worth adding an helper that is doing the
two. So the requirement is clearer.
[...]
How do you guarantee that both Xen and the domain agree on the page size?
For now, I'll add a BUILD_BUG_ON() to check that the hypervisor page
size is 4K to simplify the initial implementation. We can update to
support a larger minimal memory granule later on.
I am fine with that. FWIW, this is also what we did in the OP-TEE case.
+for ( n = 1; n < shm->page_count; last_pa = pa, n++ )
+{
+pa = page_to_maddr(shm->pages[n]);
+if ( last_pa + PAGE_SIZE == pa )
+{
Coding style: We usually avoid {} for single line.
OK
+continue;
+}
+region_descr->address_range_count++;
+}
+
+tot_len = sizeof(*descr) + sizeof(*mem_access_array) +
+ sizeof(*region_descr) +
+ region_descr->address_range_count * sizeof(*addr_range);
How do you make sure that you will not write past the end of ffa_tx?
I think it would be worth to consider adding an helper that would allow
you to allocate space in ffa_tx and zero it. This would return an error
if there is not enough space.
That's what I'm doing with frag_len. If the descriptor cannot fit it's
divided into fragments that will fit.
Oh, so this is what the loop below is for, am I correct? If so, I would
suggest to document a bit the code because this function is fairly
confusing to understand.
[...]
+if ( read_atomic(_access->access_perm.perm) != FFA_MEM_ACC_RW )
+{
+ret = FFA_RET_NOT_SUPPORTED;
+goto out_unlock;
+}
+
+region_offs = read_atomic(_access->region_offs);
+if ( sizeof(*region_descr) + region_offs > frag_len )
+{
+ret = FFA_RET_NOT_SUPPORTED;
+goto out_unlock;
+}
+
+region_descr = (void *)((vaddr_t)ctx->tx + region_offs);
+range_count = read_atomic(_descr->address_range_count);
+page_count = read_atomic(_descr->total_page_count);
+
+shm = xzalloc_flex_struct(struct ffa_shm_mem, pages, page_count)
This will allow a guest to allocate an arbitrarily large array in Xen.
So please sanitize page_count before using it.
This is tricky, what is a reasonable limit?
Indeed. We need a limit that will prevent an untrusted domain to DoS Xen
and at the same doesn't prevent the majority of well-behave domain to
function.
How is this call going to be used?
If we do set a limit the
guest can still share many separate memory ranges.
This would also need to be limited if there is a desire to support
untrusted domain.
[...]
+ret = get_shm_pages(d, shm, region_descr->address_range_array, range_count,
+0, _page_idx);
+if ( ret )
+goto out;
+if ( last_page_idx != shm->page_count )
+{
+ret = FFA_RET_INVALID_PARAMETERS;
+goto out;
+}
+
+/* Note that share_shm() uses our tx buffer */
+spin_lock(_buffer_lock);
+ret = share_shm(shm);
+spin_unlock(_buffer_lock);
+if ( ret )
+goto out;
+
+spin_lock(_mem_list_lock);
+list_add_tail(>list, _mem_list);
A couple of questions:
- What is the maximum size of the list?
Currently, there is no limit. I'm not sure what is a reasonable limit
more than five for sure, but depending on the use case more than 100
might be excessive.
This is fine to be excessive so long it doesn't allow a guest to drive
Xen out of memory or allow long running operations.
As I wrote above, the idea is we need limits that protect Xen but at the
same time doesn't prevent the majority well-behave guest to function.
As this is a tech preview, the limits can be low. We can raise the
limits as we get a better understanding how this will be used.
Cheers,
--
Julien Grall
Re: [PATCH v4 2/2] xen/arm: add FF-A mediator
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.
[...]
+.a4 = pg_count,
+};
+struct arm_smccc_1_2_regs resp;
+
+/*
+ * For arm64 we must use 64-bit calling convention if the buffer isn't
+ * passed in our tx buffer.
+ */
Can you explain why we would want to use the 32-bit calling convention
if addr is 0?
I was trying to avoid the 64-bit calling convention where possible,
OOI, why are you trying to avoid the 64-bit calling convention?
[...]
--
Julien Grall
[ovmf test] 171898: all pass - PUSHED
flight 171898 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/171898/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 3eca64f157c340f9bbf552d89a69698a3090c080 baseline version: ovmf 0e7add1d75fc75762208af84579e6809589ea6e5 Last test of basis 171894 2022-07-28 03:43:30 Z0 days Testing same since 171898 2022-07-28 17:13:05 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 0e7add1d75..3eca64f157 3eca64f157c340f9bbf552d89a69698a3090c080 -> xen-tested-master
RE: Enable audio virtualization in Xen
[AMD Official Use Only - General]
Hi all,
Can anyone please help here?
Also, how can I make use of the xen front end drivers in a HVM guest?
Regards,
Jyotirmoy
-Original Message-
From: SHARMA, JYOTIRMOY
Sent: Tuesday, July 26, 2022 4:27 PM
To: Christopher Clark
Cc: xen-devel@lists.xenproject.org; xen-us...@lists.xenproject.org
Subject: RE: Enable audio virtualization in Xen
[AMD Official Use Only - General]
Hi Christopher,
Thank you for the quick response. Please find answers below.
>> Does audio playback work OK from your Ubuntu dom0?
Yes.
>> Do you know which version of Ubuntu you are using? ('cat /etc/lsb-release')
Ubuntu 22.04 LTS.
>> Do you know which version of Xen you are using? ('xl info' in dom0 should
>> help)
4.16.2-pre.
>> Do you know which version of Qemu you have installed in dom0?
QEMU emulator version 6.1.1.
>> Did you build and install Xen from source code, or are you using binary
>> packages of Xen and its tools?
We built and installed Xen from source code (git clone
https://xenbits.xen.org/git-http/xen.git).
>> Are you using the xl tools, or libvirt tools for configuring and running
>> your guest? -- ie. how do you start your domU VM?
We are using xl tools (sudo xl -v create ).
>> When your domU is running, please could you run 'ps auxwww' in dom0 and
>> obtain the process information about the qemu instance that is running, so
>> that we can see what command line arguments have been supplied to it
This is the log we get right after booting dom 0:
root 723 0.0 0.2 243792 14468 ? Sl 15:51 0:00
/usr/local/lib/xen/bin/qemu-system-i386 -xen-domid 0 -xen-attach -name dom0
-nographic -M xenpv -daemonize -monitor /dev/null -serial /dev/null -parallel
/dev/null -pidfile /var/run/xen/qemu-dom0.pid
This log is seen after we launch dom U (sudo xl -v create ):
root 2152 20.7 2.3 2858204 133496 ? Ssl 15:53 0:09
/usr/local/lib/xen/bin/qemu-system-i386 -xen-domid 1 -no-shutdown -chardev
socket,id=libxl-cmd,path=/var/run/xen/qmp-libxl-1,server=on,wait=off -mon
chardev=libxl-cmd,mode=control -chardev
socket,id=libxenstat-cmd,path=/var/run/xen/qmp-libxenstat-1,server=on,wait=off
-mon chardev=libxenstat-cmd,mode=control -nodefaults -no-user-config -name
domu-ubuntu.hvm -vnc 127.0.0.1:0,to=99 -display sdl,gl=on -sdl -device
virtio-vga-gl -boot order=dc -usb -usbdevice tablet -smp 2,maxcpus=2 -net none
-machine xenfv,suppress-vmdesc=on -m 2040 -drive
file=/home/amd/u2004_ubuntu.qcow2,if=ide,index=0,media=disk,format=qcow2,cache=writeback
>> A little more information about what you're running will help with providing
>> guidance here. The xl man page indicates that there is a "soundhw" option in
>> the VM configuration file for passing sound hardware configution through to
>> qemu, so if you are using the xl toolstack, you could try adding this to the
>> config file: soundhw="hda"
I tried giving soundhw="hda" option in the hvm config file. However, I do not
hear any sound when I play a wave file in dom U. Here is the 'ps auxwww' output
after making this change:
root 2568 14.8 2.3 2770864 134720 ? Ssl 15:58 0:09
/usr/local/lib/xen/bin/qemu-system-i386 -xen-domid 2 -no-shutdown -chardev
socket,id=libxl-cmd,path=/var/run/xen/qmp-libxl-2,server=on,wait=off -mon
chardev=libxl-cmd,mode=control -chardev
socket,id=libxenstat-cmd,path=/var/run/xen/qmp-libxenstat-2,server=on,wait=off
-mon chardev=libxenstat-cmd,mode=control -nodefaults -no-user-config -name
domu-ubuntu-audio.hvm -vnc 127.0.0.1:0,to=99 -display sdl,gl=on -sdl -device
virtio-vga-gl -boot order=dc -usb -usbdevice tablet -soundhw hda -smp
2,maxcpus=2 -net none -machine xenfv,suppress-vmdesc=on -m 2040 -drive
file=/home/amd/u2004_ubuntu.qcow2,if=ide,index=0,media=disk,format=qcow2,cache=writeback
Also, I tried giving soundhw="all" in the config file, however this throws
following error:
libxl: error: libxl_dm.c:3130:device_model_spawn_outcome: Domain 4:domain 4
device model: spawn failed (rc=-3)
libxl: error: libxl_dm.c:3350:device_model_postconfig_done: Domain 4:Post DM
startup configs failed, rc=-3
libxl: error: libxl_create.c:1867:domcreate_devmodel_started: Domain 4:device
model did not start: -3
libxl: error: libxl_aoutils.c:646:libxl__kill_xs_path: Device Model already
exited
libxl: error: libxl_domain.c:1183:libxl__destroy_domid: Domain 4:Non-existant
domain
libxl: error: libxl_domain.c:1137:domain_destroy_callback: Domain 4:Unable to
destroy guest
libxl: error: libxl_domain.c:1064:domain_destroy_cb: Domain 4:Destruction of
domain failed
Can you please let me know what parameters I should use in the config file to
play audio from dom U?
Regards,
Jyotirmoy
-Original Message-
From: Christopher Clark
Sent: Tuesday, July 26, 2022 2:44 AM
To: SHARMA, JYOTIRMOY
Cc: xen-devel@lists.xenproject.org; xen-us...@lists.xenproject.org
Subject: Re: Enable audio virtualization in Xen
[CAUTION: External Email]
On Mon, Jul 25, 2022 at 4:45 AM SHARMA, JYOTIRMOY
wrote:
>
>
Re: [PATCH V11.1 1/3] libxl: Add support for Virtio disk configuration
чт, 28 июл. 2022 г., 21:28 Julien Grall : > Hi Oleksandr, > Hello Julien (sorry for the possible format issues) > On 16/07/2022 17:37, Oleksandr Tyshchenko wrote: > > Changes V10.1 -> V11: > > - Anthony already gave his Reviewed-by, I dropped it due to the > changes > > - George already gave his Acked-by, I dropped it due to the changes > > - s/other/standalone for the backendtype > > As discussed with Andrew on IRC, the golang changes are autogenerated. > So I have kept George's acked-by and committed the series. > Thank you very much! > Cheers, > > -- > Julien Grall >
Re: [PATCH 04/36] cpuidle,intel_idle: Fix CPUIDLE_FLAG_IRQ_ENABLE
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?
Thanx, Paul
commit ed4ae5eff4b38797607cbdd80da394149110fb37
Author: Paul E. McKenney
Date: Tue May 17 21:00:04 2022 -0700
rcu: Apply noinstr to rcu_idle_enter() and rcu_idle_exit()
This commit applies the "noinstr" tag to the rcu_idle_enter() and
rcu_idle_exit() functions, which are invoked from portions of the idle
loop that cannot be instrumented. These tags require reworking the
rcu_eqs_enter() and rcu_eqs_exit() functions that these two functions
invoke in order to cause them to use normal assertions rather than
lockdep. In addition, within rcu_idle_exit(), the raw versions of
local_irq_save() and local_irq_restore() are used, again to avoid issues
with lockdep in uninstrumented code.
This patch is based in part on an earlier patch by Jiri Olsa, discussions
with Peter Zijlstra and Frederic Weisbecker, earlier changes by Thomas
Gleixner, and off-list discussions with Yonghong Song.
Link:
https://lore.kernel.org/lkml/20220515203653.4039075-1-jo...@kernel.org/
Reported-by: Jiri Olsa
Reported-by: Alexei Starovoitov
Reported-by: Andrii Nakryiko
Signed-off-by: Paul E. McKenney
Reviewed-by: Yonghong Song
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index c25ba442044a6..9a5edab5558c9 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -631,8 +631,8 @@ static noinstr void rcu_eqs_enter(bool user)
return;
}
- lockdep_assert_irqs_disabled();
instrumentation_begin();
+ lockdep_assert_irqs_disabled();
trace_rcu_dyntick(TPS("Start"), rdp->dynticks_nesting, 0,
atomic_read(>dynticks));
WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !user &&
!is_idle_task(current));
rcu_preempt_deferred_qs(current);
@@ -659,9 +659,9 @@ static noinstr void rcu_eqs_enter(bool user)
* If you add or remove a call to rcu_idle_enter(), be sure to test with
* CONFIG_RCU_EQS_DEBUG=y.
*/
-void rcu_idle_enter(void)
+void noinstr rcu_idle_enter(void)
{
- lockdep_assert_irqs_disabled();
+ WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !raw_irqs_disabled());
rcu_eqs_enter(false);
}
EXPORT_SYMBOL_GPL(rcu_idle_enter);
@@ -861,7 +861,7 @@ static void noinstr rcu_eqs_exit(bool user)
struct rcu_data *rdp;
long oldval;
- lockdep_assert_irqs_disabled();
+ WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !raw_irqs_disabled());
rdp = this_cpu_ptr(_data);
oldval = rdp->dynticks_nesting;
WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && oldval < 0);
@@ -896,13 +896,13 @@ static void noinstr rcu_eqs_exit(bool user)
* If you add or remove a call to rcu_idle_exit(), be sure to test with
* CONFIG_RCU_EQS_DEBUG=y.
*/
-void rcu_idle_exit(void)
+void noinstr rcu_idle_exit(void)
{
unsigned long flags;
- local_irq_save(flags);
+ raw_local_irq_save(flags);
rcu_eqs_exit(false);
- local_irq_restore(flags);
+
Re: [PATCH V11.1 1/3] libxl: Add support for Virtio disk configuration
Hi Oleksandr, On 16/07/2022 17:37, Oleksandr Tyshchenko wrote: Changes V10.1 -> V11: - Anthony already gave his Reviewed-by, I dropped it due to the changes - George already gave his Acked-by, I dropped it due to the changes - s/other/standalone for the backendtype As discussed with Andrew on IRC, the golang changes are autogenerated. So I have kept George's acked-by and committed the series. Cheers, -- Julien Grall
Re: [PATCH] arm/domain: fix comment for arch_set_info_guest
> On 28 Jul 2022, at 19:07, Julien Grall wrote: > > Hi Luca, > > On 25/07/2022 15:46, Luca Fancellu wrote: >> The function arch_set_info_guest is not reached anymore through >> VCPUOP_initialise on arm, update the comment. >> Signed-off-by: Luca Fancellu >> --- >> xen/arch/arm/domain.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >> index 2f8eaab7b56b..6451cd013c1a 100644 >> --- a/xen/arch/arm/domain.c >> +++ b/xen/arch/arm/domain.c >> @@ -882,9 +882,9 @@ static int is_guest_pv64_psr(uint64_t psr) >> #endif >> /* >> - * Initialise VCPU state. The context can be supplied by either the >> - * toolstack (XEN_DOMCTL_setvcpucontext) or the guest >> - * (VCPUOP_initialise) and therefore must be properly validated. >> + * Initialise VCPU state. The context can be supplied by the toolstack >> + * (XEN_DOMCTL_setvcpucontext) and therefore must be properly validated, >> + * or by PSCI call (PSCI_cpu_on) handled by vpsci module. >> */ > > I would prefer if the comment doesn't mention who are the callers. So there > are no need to modify the comment the next time we add/remove a caller. How > about something like: > > "Initialise vCPU state. The context may be supplied by an external entity, so > we need to validate it" Sounds good! I’ll update and push it soon! > > Cheers, > > -- > Julien Grall
Re: [PATCH] arm/domain: fix comment for arch_set_info_guest
Hi Luca, On 25/07/2022 15:46, Luca Fancellu wrote: The function arch_set_info_guest is not reached anymore through VCPUOP_initialise on arm, update the comment. Signed-off-by: Luca Fancellu --- xen/arch/arm/domain.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 2f8eaab7b56b..6451cd013c1a 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -882,9 +882,9 @@ static int is_guest_pv64_psr(uint64_t psr) #endif /* - * Initialise VCPU state. The context can be supplied by either the - * toolstack (XEN_DOMCTL_setvcpucontext) or the guest - * (VCPUOP_initialise) and therefore must be properly validated. + * Initialise VCPU state. The context can be supplied by the toolstack + * (XEN_DOMCTL_setvcpucontext) and therefore must be properly validated, + * or by PSCI call (PSCI_cpu_on) handled by vpsci module. */ I would prefer if the comment doesn't mention who are the callers. So there are no need to modify the comment the next time we add/remove a caller. How about something like: "Initialise vCPU state. The context may be supplied by an external entity, so we need to validate it" Cheers, -- Julien Grall
Re: [PATCH V7 10/11] xen/arm: translate virtual PCI bus topology for guests
On 28.07.22 10:15, Jan Beulich wrote:
Hello Jan
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.
Jan
--
Regards,
Oleksandr Tyshchenko
[PATCH v3] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
The function idle_loop() is referenced only in domain.c.
Change its linkage from external to internal by adding the storage-class
specifier static to its definitions.
Add the function as a 'fake' input operand to the inline assembly statement,
to make the compiler aware that the function is used.
Fake means that the function is not actually used as an operand by the asm code.
That is because there is not a suitable gcc arm32 asm constraint for labels.
Declare return_to_new_vcpu32() and return_to_new_vcpu64() that are also
referenced by this inline asm statement.
Also, this patch resolves indirectly a MISRA C 2012 Rule 8.4 violation warning.
Signed-off-by: Xenia Ragiadakou
---
Changes in v2:
- remove the 'used' attribute and pass the function as input operand to
the inline asm statement
- declare return_to_new_vcpu32() and return_to_new_vcpu64()
Changes in v3:
- remove the declarations of return_to_new_vcpu32() and return_to_new_vcpu64()
from asm/current.h because this is not the appropriate header
- place them in arm/domain.c to restrict their visibilty to this particular file
- declare them as noreturn
xen/arch/arm/domain.c | 5 -
xen/arch/arm/include/asm/current.h | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 2f8eaab7b5..ce1089f0c6 100644
--- 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)
{
unsigned int cpu = smp_processor_id();
@@ -331,6 +331,9 @@ static void schedule_tail(struct vcpu *prev)
update_vcpu_system_time(current);
}
+extern void noreturn return_to_new_vcpu32(void);
+extern void noreturn return_to_new_vcpu64(void);
+
static void continue_new_vcpu(struct vcpu *prev)
{
current->arch.actlr = READ_SYSREG(ACTLR_EL1);
diff --git a/xen/arch/arm/include/asm/current.h
b/xen/arch/arm/include/asm/current.h
index 73e81458e5..6973eeb1d1 100644
--- a/xen/arch/arm/include/asm/current.h
+++ b/xen/arch/arm/include/asm/current.h
@@ -45,7 +45,7 @@ static inline struct cpu_info *get_cpu_info(void)
#define guest_cpu_user_regs() (_cpu_info()->guest_cpu_user_regs)
#define switch_stack_and_jump(stack, fn) do { \
-asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" ); \
+asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory"
); \
unreachable(); \
} while ( false )
--
2.34.1
Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port
On 28.07.2022 17:37, Rahul Singh wrote: >> On 26 Jul 2022, at 6:37 pm, Julien Grall wrote: >> On 21/07/2022 16:37, Rahul Singh wrote: >>> Ok. I will not add the warning. Just to confirm again is that okay If I add >>> new command line option "max_event_channels” in >>> next version for dom0 to restrict the max number of evtchn. >> >> Personally I am fine with a command line option to *globally* restrict the >> number of events channel. But Jan seemed to have some reservation. Quoting >> what he wrote previously: >> >> "Imo there would need to be a very good reason to limit Dom0's port range. > > 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. > > @Jan: I need your input on this before I send the next version of the patch > series. I have to admit that it's not clear to me what you expect - I continue to think the way expressed before. Jan
Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port
Hi Julien
> On 26 Jul 2022, at 6:37 pm, Julien Grall wrote:
>
>
>
> On 21/07/2022 16:37, Rahul Singh wrote:
>> Hi Julien,
>
> Hi Rahul,
>
>>> On 21 Jul 2022, at 2:29 pm, Julien Grall wrote:
>>>
>>> On 21/07/2022 13:50, Rahul Singh wrote:
Hi Julien,
>>>
>>> Hi Rahul,
>>>
> On 20 Jul 2022, at 12:16 pm, Julien Grall wrote:
>
> Hi Rahul,
>
> On 20/07/2022 10:59, Rahul Singh wrote:
>>> On 13 Jul 2022, at 1:29 pm, Julien Grall wrote:
>>>
>>>
>>>
>>> On 13/07/2022 13:12, Bertrand Marquis wrote:
> On 13 Jul 2022, at 12:31, Julien Grall wrote:
>> I can't
>> see why it would be wrong to have a more tight limit on static ports
>> than on traditional ("dynamic") ones. Even if only to make sure so
>> many dynamic ones are left.
>
> This is similar to Xen forbidding to close a static port: it is not
> the hypervisor business to check that there are enough event channel
> ports freed for dynamic allocation.
On other side we need to be cautious not to add too much complexity in
the code by trying to make things always magically work.
If you want Xen to be accessible to non expert by magically working
all the time, there would be a lot of work to do.
>>>
>>> It is not clear to me whether you are referring to a developper or
>>> admin here.
>>>
>>> On the admin side, we need to make sure they have an easy way to
>>> configure event channels. One knob is always going to easier than two
>>> knobs.
>>>
>>> On the developper side, this could be resolved by better documentation
>>> in the code/interface.
>>>
>>> Cheers,
>> To conclude the discussion, If everyone agree I will add the below patch
>> or similar in the next version to restrict the
>> max number of evtchn supported as suggested.
>
> I am fine if the limit for domU is fixed by Xen for now. However, for
> dom0, 4096 is potentially too low if you have many PV drivers (each
> backend will need a few event channels). So I don't think this wants to
> be fixed by Xen.
Agree.
>
> I am not entirely sure we want to limit the number of event channels for
> dom0. But if you want to, then this will have to be done via a command
> line option (or device-tree property).
We need to support the static event channel for dom0 also, in that case,
we need to restrict the max number of evtchn for dom0 to mitigate the
security issue.
>>>
>>> It sounds like there are some misundertanding or I am missing some context.
>>> The static event channels will be allocated at boot, so the worse that can
>>> happen is it will be slower to boot.
>>>
>>> My point regarding fifo was more in the generic case of allowing the caller
>>> to select the port. This would be a concern in the context of
>>> non-cooperative live-migration. An easy way is to restrict the number of
>>> ports. For you, this is just an increase in boot time.
>>>
>>> Furthermore, there is an issue for dom0less domUs because we don't limit
>>> the number of port by default. This means that a domU can allocate a large
>>> amount of memory in Xen (we need some per-event channel state). Hence why I
>>> suggested to update max_evtchn_channel.
>> Thanks for the clarification.
>>>
If the admin set the value greater than 4096 (or what we agreed on) and
static event channel support is enabled we will print the warning to the
user related to fill
the hole issue for FIFO ABI.
>>>
>>> See above. I don't see the need for a warning. The admin will notice that
>>> it is slower to boot.
>> Ok. I will not add the warning. Just to confirm again is that okay If I add
>> new command line option "max_event_channels” in
>> next version for dom0 to restrict the max number of evtchn.
>
> Personally I am fine with a command line option to *globally* restrict the
> number of events channel. But Jan seemed to have some reservation. Quoting
> what he wrote previously:
>
> "Imo there would need to be a very good reason to limit Dom0's port range.
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.
@Jan: I need your input on this before I send the next version of the patch
series.
Regards,
Rahul
Re: [PATCH] xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation
On 28/07/2022 16:15, Xenia Ragiadakou wrote:
On 7/28/22 16:56, Julien Grall wrote:
Hi,
On 28/07/2022 14:49, Xenia Ragiadakou wrote:
The macro parameter 'v' is used as an expression and needs to be
enclosed in
parentheses.
Signed-off-by: Xenia Ragiadakou
---
xen/arch/arm/include/asm/arm64/sysregs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h
b/xen/arch/arm/include/asm/arm64/sysregs.h
index 54670084c3..f5a7269a27 100644
--- 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?
Here v is supposed to be used as an expression. So maybe the rule wants
to enforce that in the macro we will pass an expression and not multiple
statements (?) ... not sure.
Do you mean something like below?
#include
#include
#define foo(v) do { \
uint64_t _r = v;\
\
printf("_r %lu\n", _r); \
} while (0)
int main(void)
{
foo(1; printf("Bar\n"));
}
If yes, then I agree that the compiler will still be happy. Or if you
pass "1; 2", v would be set to 1 rather than 2. I am not sure why one
would want to write such code. But I guess we want to prevent that.
Cheers,
--
Julien Grall
[xen-unstable test] 171896: tolerable FAIL
flight 171896 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/171896/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 171887 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 171887 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 171887 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 171887 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 171887 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 171887 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 171887 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 171887 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 171887 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 171887 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 171887 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 171887 test-amd64-i386-xl-pvshim14 guest-start fail 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-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-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-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 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 never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass version targeted for testing: xen f732240fd3bac25116151db5ddeb7203b62e85ce baseline version: xen
Re: [PATCH] xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation
On 7/28/22 17:34, Julien Grall wrote:
Hi,
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).
The parentheses are not pointless if the intention is v to be used as an
expression to the assignment. If this is not the intended usage for v,
then the rule does not apply anyway.
--
Xenia
Re: [PATCH] xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation
Hi Julien,
On 7/28/22 16:56, Julien Grall wrote:
Hi,
On 28/07/2022 14:49, Xenia Ragiadakou wrote:
The macro parameter 'v' is used as an expression and needs to be
enclosed in
parentheses.
Signed-off-by: Xenia Ragiadakou
---
xen/arch/arm/include/asm/arm64/sysregs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h
b/xen/arch/arm/include/asm/arm64/sysregs.h
index 54670084c3..f5a7269a27 100644
--- 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?
Here v is supposed to be used as an expression. So maybe the rule wants
to enforce that in the macro we will pass an expression and not multiple
statements (?) ... not sure.
asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \
} while (0)
#define READ_SYSREG64(name) ({ \
Cheers,
--
Xenia
Re: [PATCH V7 03/11] vpci/header: implement guest BAR register handlers
On 28.07.22 10:01, Jan Beulich wrote:
Hello Jan
On 27.07.2022 18:17, Oleksandr wrote:
On 27.07.22 13:15, Jan Beulich wrote:
On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
@@ -527,6 +592,17 @@ static int cf_check init_bars(struct pci_dev *pdev)
if ( (val & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO )
{
bars[i].type = VPCI_BAR_IO;
+
+#ifndef CONFIG_X86
+if ( !is_hwdom )
+{
+rc = vpci_add_register(pdev->vpci, empty_bar_read, NULL,
+ reg, 4, [i]);
+if ( rc )
+goto fail;
+}
+#endif
Since long term this can't be correct, it wants a TODO comment put next
to it.
Looking into the previous versions of this patch (up to V3) I failed to
find any changes in current version which hadn't been discussed (and
agreed in some form).
Could you please clarify what exactly can't be correct the long term,
for me to put the proper TODO here. Do you perhaps mean that TODO needs
to explain why we have to diverge?
If a device has I/O port ranges, then that's typically for a reason.
Drivers (in the guest) may therefore want to use those ranges to
communicate with the device. Imagine in particular a device without
any MMIO BARs, and with only I/O port one(s).
@@ -553,34 +635,47 @@ static int cf_check init_bars(struct pci_dev *pdev)
bars[i].size = size;
bars[i].prefetchable = val & PCI_BASE_ADDRESS_MEM_PREFETCH;
-rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, reg, 4,
- [i]);
+rc = vpci_add_register(pdev->vpci,
+ is_hwdom ? vpci_hw_read32 : guest_bar_read,
+ is_hwdom ? bar_write : guest_bar_write,
+ reg, 4, [i]);
if ( rc )
-{
-pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
-return rc;
-}
+goto fail;
}
-/* Check expansion ROM. */
-rc = pci_size_mem_bar(pdev->sbdf, rom_reg, , , PCI_BAR_ROM);
-if ( rc > 0 && size )
+/* Check expansion ROM: we do not handle ROM for guests. */
+if ( is_hwdom )
This again can't be right long-term. Personally I'd prefer if the code
was (largely) left as is, with adjustments (with suitable TODO comments)
made on a much smaller scope only.
I can revive a comment that Oleksandr Andrushchenko provided for earlier
version by transforming into TODO:
ROM BAR is only handled for the hardware domain and for guest domains
there is a stub: at the moment PCI expansion ROM handling is supported
for x86 only and it might not be used by other architectures without
emulating x86. Other use-cases may include using that expansion ROM before
Xen boots, hence no emulation is needed in Xen itself. Or when a guest
wants to use the ROM code which seems to be rare.
ROMs can contain other than x86 code. While reportedly mostly dead, EFI
bytecode was an example of an abstraction layer supporting arbitrary
architectures. Therefore a comment along these lines would be okay, but
personally I'd prefer it to be less verbose - along the lines of the
one to be supplied for the I/O port restriction.
Thanks for the clarification. I will add two TODOs.
Jan
--
Regards,
Oleksandr Tyshchenko
Re: [PATCH] xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation
Hi Jan,
On 28/07/2022 15:17, Jan Beulich wrote:
On 28.07.2022 15: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); \
asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \
Out of curiosity - why is the intermediate variable necessary?
Can't v be used directly in the asm(), possibly with a suitable
modifier added to %0 such that it'll always be x (and not
w) which is used as the operand to "msr"?
It should be possible to use %x0. However, We may need to use (uint64_t)(v).
Linux seems to be use it, but IIRC they are not supported GCC versions
as old as ours. So we would want to check when %x0 was introduced.
Cheers,
--
Julien Grall
Re: [PATCH V7 09/11] vpci: add initial support for virtual PCI bus topology
On 28.07.22 17:26, Jan Beulich wrote: Hello Jan On 28.07.2022 16:16, Oleksandr wrote: On 27.07.22 13:32, Jan Beulich wrote: On 19.07.2022 19:42, Oleksandr Tyshchenko wrote: From: Oleksandr Andrushchenko Assign SBDF to the PCI devices being passed through with bus 0. The resulting topology is where PCIe devices reside on the bus 0 of the root complex itself (embedded endpoints). This implementation is limited to 32 devices which are allowed on a single PCI bus. Please note, that at the moment only function 0 of a multifunction device can be passed through. I've not been able to spot where this restriction is being enforced - can you please point me at the respective code? Nor have I found the respective code. Could you please suggest a place where to put such enforcement (I guess, this should be present in the toolstack)? Such check should be in the tool stack primarily to give a sensible error message to the user. Yet the hypervisor needs to check itself nevertheless. You know the code you're adding much better than I do, so I guess I'm a little puzzled by you asking me to suggest a place. (And for the tool stack I guess asking tool stack folks would get you better mileage.) Thanks for the clarification. I am still getting used to the changes which that patch series makes (I didn't write that code). Asking for suggestion I didn't mean to point an exact place in the code, but rather a subsystem/software layer, sorry if I was unclear. @@ -124,6 +191,7 @@ void vpci_deassign_device(struct pci_dev *pdev) if ( !has_vpci(pdev->domain) ) return; +vpci_remove_virtual_device(pdev); vpci_remove_device(pdev); } And other call sites of vpci_remove_device() do not have a need of cleaning up guest_sbdf / vpci_dev_assigned_map? I am not 100% sure, but it looks like they don't need. On the other hand, even if they don't need that, doing the cleaning won't be an issue at all, there is a check before cleaning (which will be extended as I proposed above), so ... IOW I wonder if it wouldn't be better to have vpci_remove_device() do this as well (retaining - see my comment on the earlier patch) the simple aliasing of vpci_deassign_device() to vpci_remove_device()). ... maybe yes. Shall I do that change? Well - yes please, afaic. ok, will do Jan -- Regards, Oleksandr Tyshchenko
Re: [PATCH] xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation
Hi,
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).
Cheers,
--
Julien Grall
Re: [PATCH V7 09/11] vpci: add initial support for virtual PCI bus topology
On 28.07.2022 16:16, Oleksandr wrote: > On 27.07.22 13:32, Jan Beulich wrote: >> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote: >>> From: Oleksandr Andrushchenko >>> >>> Assign SBDF to the PCI devices being passed through with bus 0. >>> The resulting topology is where PCIe devices reside on the bus 0 of the >>> root complex itself (embedded endpoints). >>> This implementation is limited to 32 devices which are allowed on >>> a single PCI bus. >>> >>> Please note, that at the moment only function 0 of a multifunction >>> device can be passed through. >> I've not been able to spot where this restriction is being enforced - >> can you please point me at the respective code? > > Nor have I found the respective code. > > Could you please suggest a place where to put such enforcement (I guess, > this should be present in the toolstack)? Such check should be in the tool stack primarily to give a sensible error message to the user. Yet the hypervisor needs to check itself nevertheless. You know the code you're adding much better than I do, so I guess I'm a little puzzled by you asking me to suggest a place. (And for the tool stack I guess asking tool stack folks would get you better mileage.) >>> @@ -124,6 +191,7 @@ void vpci_deassign_device(struct pci_dev *pdev) >>> if ( !has_vpci(pdev->domain) ) >>> return; >>> >>> +vpci_remove_virtual_device(pdev); >>> vpci_remove_device(pdev); >>> } >> And other call sites of vpci_remove_device() do not have a need of >> cleaning up guest_sbdf / vpci_dev_assigned_map? > > I am not 100% sure, but it looks like they don't need. On the other > hand, even if they don't need that, doing the cleaning won't be an issue > at all, > > there is a check before cleaning (which will be extended as I proposed > above), so ... > > >> IOW I wonder if it >> wouldn't be better to have vpci_remove_device() do this as well >> (retaining - see my comment on the earlier patch) the simple aliasing >> of vpci_deassign_device() to vpci_remove_device()). > > > ... maybe yes. Shall I do that change? Well - yes please, afaic. Jan
Re: [PATCH] xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation
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.
Jan
Re: [PATCH] xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation
On 28.07.2022 15: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); \
> asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \
Out of curiosity - why is the intermediate variable necessary?
Can't v be used directly in the asm(), possibly with a suitable
modifier added to %0 such that it'll always be x (and not
w) which is used as the operand to "msr"?
Jan
Re: [PATCH V7 09/11] vpci: add initial support for virtual PCI bus topology
On 27.07.22 13:32, Jan Beulich wrote:
Hello Jan
On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
From: Oleksandr Andrushchenko
Assign SBDF to the PCI devices being passed through with bus 0.
The resulting topology is where PCIe devices reside on the bus 0 of the
root complex itself (embedded endpoints).
This implementation is limited to 32 devices which are allowed on
a single PCI bus.
Please note, that at the moment only function 0 of a multifunction
device can be passed through.
I've not been able to spot where this restriction is being enforced -
can you please point me at the respective code?
Nor have I found the respective code.
Could you please suggest a place where to put such enforcement (I guess,
this should be present in the toolstack)?
@@ -99,6 +102,62 @@ int vpci_add_handlers(struct pci_dev *pdev)
}
#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
+static int add_virtual_device(struct pci_dev *pdev)
+{
+struct domain *d = pdev->domain;
+pci_sbdf_t sbdf = { 0 };
+unsigned long new_dev_number;
+
+if ( is_hardware_domain(d) )
+return 0;
+
+ASSERT(pcidevs_write_locked());
+
+/*
+ * Each PCI bus supports 32 devices/slots at max or up to 256 when
+ * there are multi-function ones which are not yet supported.
+ */
+if ( pdev->info.is_extfn )
+{
+gdprintk(XENLOG_ERR, "%pp: only function 0 passthrough supported\n",
+ >sbdf);
+return -EOPNOTSUPP;
+}
+
+new_dev_number = find_first_zero_bit(d->vpci_dev_assigned_map,
+ VPCI_MAX_VIRT_DEV);
+if ( new_dev_number >= VPCI_MAX_VIRT_DEV )
+return -ENOSPC;
+
+__set_bit(new_dev_number, >vpci_dev_assigned_map);
+
+/*
+ * Both segment and bus number are 0:
+ * - we emulate a single host bridge for the guest, e.g. segment 0
+ * - with bus 0 the virtual devices are seen as embedded
+ *endpoints behind the root complex
+ *
+ * TODO: add support for multi-function devices.
+ */
+sbdf.devfn = PCI_DEVFN(new_dev_number, 0);
+pdev->vpci->guest_sbdf = sbdf;
+
+return 0;
+
+}
+
+static void vpci_remove_virtual_device(const struct pci_dev *pdev)
+{
+ASSERT(pcidevs_write_locked());
+
+if ( pdev->vpci )
+{
+__clear_bit(pdev->vpci->guest_sbdf.dev,
+>domain->vpci_dev_assigned_map);
+pdev->vpci->guest_sbdf.sbdf = ~0;
+}
+}
Feels like I did comment on this before: When ...
@@ -111,8 +170,16 @@ int vpci_assign_device(struct pci_dev *pdev)
rc = vpci_add_handlers(pdev);
if ( rc )
-vpci_deassign_device(pdev);
+goto fail;
... this path is taken and hence ...
+rc = add_virtual_device(pdev);
... this is bypassed, ...
+if ( rc )
+goto fail;
+
+return 0;
+ fail:
+vpci_deassign_device(pdev);
... the function here will see guest_sbdf still as ~0, while pdev->vpci
is non-NULL. Therefore mistakenly bit 31 of vpci_dev_assigned_map will
be cleared.
Indeed, good catch, thanks! I assume this can be just fixed by extending
a check in vpci_remove_virtual_device():
if ( pdev->vpci && (pdev->vpci->guest_sbdf.sbdf != ~0) )
@@ -124,6 +191,7 @@ void vpci_deassign_device(struct pci_dev *pdev)
if ( !has_vpci(pdev->domain) )
return;
+vpci_remove_virtual_device(pdev);
vpci_remove_device(pdev);
}
And other call sites of vpci_remove_device() do not have a need of
cleaning up guest_sbdf / vpci_dev_assigned_map?
I am not 100% sure, but it looks like they don't need. On the other
hand, even if they don't need that, doing the cleaning won't be an issue
at all,
there is a check before cleaning (which will be extended as I proposed
above), so ...
IOW I wonder if it
wouldn't be better to have vpci_remove_device() do this as well
(retaining - see my comment on the earlier patch) the simple aliasing
of vpci_deassign_device() to vpci_remove_device()).
... maybe yes. Shall I do that change?
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -457,6 +457,14 @@ struct domain
#ifdef CONFIG_HAS_PCI
struct list_head pdev_list;
+#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
+/*
+ * The bitmap which shows which device numbers are already used by the
+ * virtual PCI bus topology and is used to assign a unique SBDF to the
+ * next passed through virtual PCI device.
+ */
+DECLARE_BITMAP(vpci_dev_assigned_map, VPCI_MAX_VIRT_DEV);
+#endif
#endif
Hmm, yet another reason to keep sched.h including vpci.h, which
imo would better be dropped - sched.h already has way too many
dependencies. (Just a remark, not strictly a request to change
anything.)
I see.
Jan
--
Regards,
Oleksandr Tyshchenko
Re: [PATCH] xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation
Hi,
On 28/07/2022 14:49, Xenia Ragiadakou wrote:
The macro parameter 'v' is used as an expression and needs to be enclosed in
parentheses.
Signed-off-by: Xenia Ragiadakou
---
xen/arch/arm/include/asm/arm64/sysregs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h
b/xen/arch/arm/include/asm/arm64/sysregs.h
index 54670084c3..f5a7269a27 100644
--- 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?
asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \
} while (0)
#define READ_SYSREG64(name) ({ \
Cheers,
--
Julien Grall
Re: Question to mem-path support at QEMU for Xen
On Thu, 28 Jul 2022 15:17:49 +0800 Huang Rui wrote: > Hi Igor, > > Appreciate you for the reply! > > On Wed, Jul 27, 2022 at 04:19:30PM +0800, Igor Mammedov wrote: > > On Tue, 26 Jul 2022 15:27:07 +0800 > > Huang Rui wrote: > > > > > Hi Anthony and other Qemu/Xen guys, > > > > > > We are trying to enable venus on Xen virtualization platform. And we would > > > like to use the backend memory with memory-backend-memfd,id=mem1,size=4G > > > options on QEMU, however, the QEMU will tell us the "-mem-path" is not > > > supported with Xen. I verified the same function on KVM. > > > > > > qemu-system-i386: -mem-path not supported with Xen > > > > > > So may I know whether Xen has any limitation that support > > > memory-backend-memfd in QEMU or just implementation is not done yet? > > > > Currently Xen doesn't use guest RAM allocation the way the rest of > > accelerators do. (it has hacks in memory_region/ramblock API, > > that divert RAM allocation calls to Xen specific API) > > I am new for Xen and QEMU, we are working on GPU. We would like to have a > piece of backend memroy like video memory for VirtIO GPU to support guest > VM Mesa Vulkan (Venus). Do you mean we can the memory_region/ramblock APIs > to work around it? > > > > > The sane way would extend Xen to accept RAM regions (whatever they are > > ram or fd based) QEMU allocates instead of going its own way. This way > > it could reuse all memory backends that QEMU provides for the rest of > > the non-Xen world. (not to mention that we could drop non trivial > > Xen hacks so that guest RAM handling would be consistent with other > > accelerators) > > > > May I know what do you mean by "going its own way"? This sounds good, could > you please elaborate on how can we implement this? We would like to give a > try to address the problem on Xen. Would you mind to point somewhere that I > can learn and understand the RAM region. Very happy to see your > suggestions! see for example see ram_block_add(), if Xen could be persuaded to use memory allocated by '!xen_enabled()' branch then it's likely file base backends would also become usable. Whether it is possible for Xen or not I don't know, I guess CCed Xen folks will suggest something useful. > Thanks & Best Regards, > Ray >
[PATCH] 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.
Signed-off-by: Xenia Ragiadakou
---
xen/arch/arm/include/asm/arm64/sysregs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h
b/xen/arch/arm/include/asm/arm64/sysregs.h
index 54670084c3..f5a7269a27 100644
--- 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); \
asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \
} while (0)
#define READ_SYSREG64(name) ({ \
--
2.34.1
Re: [PATCH] xen/xsm: dummy.h: Fix MISRA C 2012 Directive 4.10 violation
On 7/27/22 11:19, Xenia Ragiadakou wrote: Protect header file from being included more than once by adding ifndef guard. Signed-off-by: Xenia Ragiadakou --- xen/include/xsm/dummy.h | 5 + 1 file changed, 5 insertions(+) diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 77f27e7163..8671af1ba4 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -15,6 +15,9 @@ * value of action. */ +#ifndef __XEN_XSM_DUMMY_H__ +#define __XEN_XSM_DUMMY_H__ + #include #include #include @@ -843,3 +846,5 @@ static XSM_INLINE int cf_check xsm_domain_resource_map( XSM_ASSERT_ACTION(XSM_DM_PRIV); return xsm_default_action(action, current->domain, d); } + +#endif /* __XEN_XSM_DUMMY_H__ */ Acked-by: Daniel P. Smith
Re: [PATCH v7] evtchn: convert domain event lock to an r/w one
On 7/27/22 11:56, Jan Beulich wrote: Especially for the use in evtchn_move_pirqs() (called when moving a vCPU across pCPU-s) and the ones in EOI handling in PCI pass-through code, serializing perhaps an entire domain isn't helpful when no state (which isn't e.g. further protected by the per-channel lock) changes. Unfortunately this implies dropping of lock profiling for this lock, until r/w locks may get enabled for such functionality. While ->notify_vcpu_id is now meant to be consistently updated with the per-channel lock held, an extension applies to ECS_PIRQ: The field is also guaranteed to not change with the per-domain event lock held for writing. Therefore the link_pirq_port() call from evtchn_bind_pirq() could in principle be moved out of the per-channel locked regions, but this further code churn didn't seem worth it. Signed-off-by: Jan Beulich --- v6: Re-vive and re-base. v5: Re-base, also over dropped earlier patch. v4: Re-base, in particular over new earlier patches. Acquire both per-domain locks for writing in evtchn_close(). Adjust spin_barrier() related comments. v3: Re-base. v2: Consistently lock for writing in evtchn_reset(). Fix error path in pci_clean_dpci_irqs(). Lock for writing in pt_irq_time_out(), hvm_dirq_assist(), hvm_dpci_eoi(), and hvm_dpci_isairq_eoi(). Move rw_barrier() introduction here. Re-base over changes earlier in the series. --- a/xen/xsm/flask/flask_op.c +++ b/xen/xsm/flask/flask_op.c @@ -530,7 +530,7 @@ static int flask_get_peer_sid(struct xen struct evtchn *chn; struct domain_security_struct *dsec; -spin_lock(>event_lock); +read_lock(>event_lock); if ( !port_is_valid(d, arg->evtchn) ) goto out; @@ -548,7 +548,7 @@ static int flask_get_peer_sid(struct xen rv = 0; out: -spin_unlock(>event_lock); +read_unlock(>event_lock); return rv; } Acked-by: Daniel P. Smith
Re: [PATCH v2] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
On 7/28/22 16:05, Julien Grall wrote: Hi, On 28/07/2022 10:26, Jan Beulich wrote: On 28.07.2022 09:57, Xenia Ragiadakou wrote: --- a/xen/arch/arm/include/asm/current.h +++ b/xen/arch/arm/include/asm/current.h @@ -44,8 +44,11 @@ static inline struct cpu_info *get_cpu_info(void) #define guest_cpu_user_regs() (_cpu_info()->guest_cpu_user_regs) +extern void return_to_new_vcpu32(void); +extern void return_to_new_vcpu64(void); While ultimately it's the Arm maintainers to judge, may I suggest that these be put in arm/domain.c to limit visibility? In general, I am not in favor of declaring prototype outside of headers. That said, I would be okay with it for the two prototypes because: 1) they are prototypes for assembly functions 2) declaring in current.h sounds wrong. A better place would be domain.h but this would not reduce the visibility too much 3) this is unlikely to be used by other part of Xen What I will ask is irrelevant to the placement but relevant to the declaration itself. I should also have declared them noreturn, right? -- Xenia
Re: [PATCH v2] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
Hi, On 28/07/2022 10:26, Jan Beulich wrote: On 28.07.2022 09:57, Xenia Ragiadakou wrote: --- a/xen/arch/arm/include/asm/current.h +++ b/xen/arch/arm/include/asm/current.h @@ -44,8 +44,11 @@ static inline struct cpu_info *get_cpu_info(void) #define guest_cpu_user_regs() (_cpu_info()->guest_cpu_user_regs) +extern void return_to_new_vcpu32(void); +extern void return_to_new_vcpu64(void); While ultimately it's the Arm maintainers to judge, may I suggest that these be put in arm/domain.c to limit visibility? In general, I am not in favor of declaring prototype outside of headers. That said, I would be okay with it for the two prototypes because: 1) they are prototypes for assembly functions 2) declaring in current.h sounds wrong. A better place would be domain.h but this would not reduce the visibility too much 3) this is unlikely to be used by other part of Xen Cheers, -- Julien Grall
Re: [PATCH v2] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
Hi Jan, On 7/28/22 12:26, Jan Beulich wrote: On 28.07.2022 09:57, Xenia Ragiadakou wrote: --- a/xen/arch/arm/include/asm/current.h +++ b/xen/arch/arm/include/asm/current.h @@ -44,8 +44,11 @@ static inline struct cpu_info *get_cpu_info(void) #define guest_cpu_user_regs() (_cpu_info()->guest_cpu_user_regs) +extern void return_to_new_vcpu32(void); +extern void return_to_new_vcpu64(void); While ultimately it's the Arm maintainers to judge, may I suggest that these be put in arm/domain.c to limit visibility? I agree with you. Will fix and resend. -- Xenia
[PATCH v3] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
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
---
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
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 | 54 +-
include/xen/hvm.h | 2 ++
include/xen/interface/hvm/hvm_op.h | 19 ++
8 files changed, 101 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)
xen_panic_handler_init();
- if (!no_vector_callback &&
Re: [PATCH v1] xen: add late init call in start_xen
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.
> Sorry for the inconvenience.
>
> I made this patch during using xen for a project.
>
> And I want to share it and ask for review.
>
> Boyoun Park.
>
> From: Boyoun Park
>
> To: xen-devel@lists.xenproject.org
>
> Cc: Stefano Stabellini
>
> Cc: Julien Grall
>
> Cc: Bertrand Marquis
>
> Cc: Volodymyr Babchuk
>
> Cc: Andrew Cooper
>
> Cc: George Dunlap
>
> Cc: Jan Beulich
>
> Cc: Wei Liu
>
> Cc: "Roger Pau Monné"
>
> Date: Tue, 15 Mar 2022 12:57:59 +0900
>
> 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.
> @@ -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?
Jan
[libvirt test] 171895: regressions - FAIL
flight 171895 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/171895/ 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-amd64-libvirt 6 libvirt-buildfail REGR. vs. 151777 build-i386-libvirt6 libvirt-buildfail REGR. vs. 151777 build-armhf-libvirt 6 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 9c3d398df11024ef6c00a50c98fcc0f1f66c16a1 baseline version: libvirt 2c846fa6bcc11929c9fb857a22430fb9945654ad Last test of basis 151777 2020-07-10 04:19:19 Z 748 days Failing since151818 2020-07-11 04:18:52 Z 747 days 729 attempts Testing same since 171895 2022-07-28 04:20:13 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 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation
Hi Julien, > On 28 Jul 2022, at 11:21, Julien Grall wrote: > > > > On 28/07/2022 10:45, Bertrand Marquis wrote: >>> On 28 Jul 2022, at 10:35, Julien Grall wrote: >>> >>> >>> >>> On 28/07/2022 08:57, Bertrand Marquis wrote: Hi Julien, >>> >>> Hi Bertrand, >>> > On 27 Jul 2022, at 16:46, Julien Grall wrote: > > Hi Xenia, > > On 27/07/2022 16:32, Xenia Ragiadakou wrote: >> Remove unused macro atomic_xchg(). >> Signed-off-by: Xenia Ragiadakou >> --- >> xen/arch/arm/include/asm/atomic.h | 2 -- >> 1 file changed, 2 deletions(-) >> diff --git a/xen/arch/arm/include/asm/atomic.h >> b/xen/arch/arm/include/asm/atomic.h >> index f5ef744b4b..a2dc125291 100644 >> --- a/xen/arch/arm/include/asm/atomic.h >> +++ b/xen/arch/arm/include/asm/atomic.h >> @@ -223,8 +223,6 @@ static inline int atomic_add_unless(atomic_t *v, int >> a, int u) >> return __atomic_add_unless(v, a, u); >> } >> -#define atomic_xchg(v, new) (xchg(&((v)->counter), new)) >> - > > While I agree this is unused today, the wrapper is quite trivial and part > of the generic API (x86 also provides one). So I am not in favor of > removing it just to please MISRA. > > That said, if Bertrand and Stefano agrees with removing it then you > should also remove the x86 version to avoid inconsistency. I think we can keep this and maybe add a comment on top to document a known violation: /* TODO: MISRA_VIOLATION 2.5 */ >>> >>> While I am fine with this goal of the comment (i.e. indicating where Xen is >>> not MISRA compliant), I think this is one place where I would rather not >>> want one because it can get stale if someones decide to use the function. >> I think the one doing that will have to update the comment otherwise we will >> never manage to have an analysis without findings. > > I was under the impression that Xen will never officially follow some of the > MISRA rules. So I would expect the tools to be able to detect such cases so > we don't have to add a comment for every deviation on something we will never > support. > >> Having those kind of comments in the code for violation also means that they >> must be updated if the violation is solved. > > Right, but for thing like unused function, this is quite easy to miss by both > the developer and reviewers. So we are going to end up to comments for > nothing. > >> Maybe we will need a run ignoring those to identify possible violations >> which are not violations anymore but this might be hard to do. > > TBH, I think it would be best if we can teach the tools to ignore certain > rules. Definitely it is possible to instruct the tool to ignore this you are right and for 2.5 we should (for some reason I was under the impression that we said we would follow 2.5 but accept deviations). @Xenia: please ignore and do not add a comment for this. I think we will need to distinguish 2 kind of not following: - not following at all (disable in the tools) - accepting some deviations (documented in the code) As much as we can, I think we should target the second unless we have a lot of violations. Cheers Bertrand > > Cheers, > > -- > Julien Grall
Re: [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation
On 28/07/2022 10:45, Bertrand Marquis wrote: On 28 Jul 2022, at 10:35, Julien Grall wrote: On 28/07/2022 08:57, Bertrand Marquis wrote: Hi Julien, Hi Bertrand, On 27 Jul 2022, at 16:46, Julien Grall wrote: Hi Xenia, On 27/07/2022 16:32, Xenia Ragiadakou wrote: Remove unused macro atomic_xchg(). Signed-off-by: Xenia Ragiadakou --- xen/arch/arm/include/asm/atomic.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/xen/arch/arm/include/asm/atomic.h b/xen/arch/arm/include/asm/atomic.h index f5ef744b4b..a2dc125291 100644 --- a/xen/arch/arm/include/asm/atomic.h +++ b/xen/arch/arm/include/asm/atomic.h @@ -223,8 +223,6 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u) return __atomic_add_unless(v, a, u); } -#define atomic_xchg(v, new) (xchg(&((v)->counter), new)) - While I agree this is unused today, the wrapper is quite trivial and part of the generic API (x86 also provides one). So I am not in favor of removing it just to please MISRA. That said, if Bertrand and Stefano agrees with removing it then you should also remove the x86 version to avoid inconsistency. I think we can keep this and maybe add a comment on top to document a known violation: /* TODO: MISRA_VIOLATION 2.5 */ While I am fine with this goal of the comment (i.e. indicating where Xen is not MISRA compliant), I think this is one place where I would rather not want one because it can get stale if someones decide to use the function. I think the one doing that will have to update the comment otherwise we will never manage to have an analysis without findings. I was under the impression that Xen will never officially follow some of the MISRA rules. So I would expect the tools to be able to detect such cases so we don't have to add a comment for every deviation on something we will never support. Having those kind of comments in the code for violation also means that they must be updated if the violation is solved. Right, but for thing like unused function, this is quite easy to miss by both the developer and reviewers. So we are going to end up to comments for nothing. Maybe we will need a run ignoring those to identify possible violations which are not violations anymore but this might be hard to do. TBH, I think it would be best if we can teach the tools to ignore certain rules. Cheers, -- Julien Grall
[linux-linus test] 171891: tolerable FAIL - PUSHED
flight 171891 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/171891/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 171882 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 171882 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 171882 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 171882 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 171882 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 171882 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 171882 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 171882 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-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-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-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-armhf-armhf-xl 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: linux6e7765cb477a9753670d4351d14de93f1e9dbbd4 baseline version: linux39c3c396f8131f3db454c80e0fcfcdc54ed9ec01 Last test of basis 171882 2022-07-27 09:15:12 Z1 days Testing same since 171891 2022-07-28 00:42:47 Z0 days1 attempts People who touched revisions under test: Arnd Bergmann Baolin Wang Claudiu Beznea Florian Fainelli Linus Torvalds Linus Walleij Lukas Bulwahn Michael Walle jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64
Re: [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation
Hi Julien, > On 28 Jul 2022, at 10:35, Julien Grall wrote: > > > > On 28/07/2022 08:57, Bertrand Marquis wrote: >> Hi Julien, > > Hi Bertrand, > >>> On 27 Jul 2022, at 16:46, Julien Grall wrote: >>> >>> Hi Xenia, >>> >>> On 27/07/2022 16:32, Xenia Ragiadakou wrote: Remove unused macro atomic_xchg(). Signed-off-by: Xenia Ragiadakou --- xen/arch/arm/include/asm/atomic.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/xen/arch/arm/include/asm/atomic.h b/xen/arch/arm/include/asm/atomic.h index f5ef744b4b..a2dc125291 100644 --- a/xen/arch/arm/include/asm/atomic.h +++ b/xen/arch/arm/include/asm/atomic.h @@ -223,8 +223,6 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u) return __atomic_add_unless(v, a, u); } -#define atomic_xchg(v, new) (xchg(&((v)->counter), new)) - >>> >>> While I agree this is unused today, the wrapper is quite trivial and part >>> of the generic API (x86 also provides one). So I am not in favor of >>> removing it just to please MISRA. >>> >>> That said, if Bertrand and Stefano agrees with removing it then you should >>> also remove the x86 version to avoid inconsistency. >> I think we can keep this and maybe add a comment on top to document a known >> violation: >> /* TODO: MISRA_VIOLATION 2.5 */ > > While I am fine with this goal of the comment (i.e. indicating where Xen is > not MISRA compliant), I think this is one place where I would rather not want > one because it can get stale if someones decide to use the function. I think the one doing that will have to update the comment otherwise we will never manage to have an analysis without findings. Having those kind of comments in the code for violation also means that they must be updated if the violation is solved. Maybe we will need a run ignoring those to identify possible violations which are not violations anymore but this might be hard to do. Cheers Bertrand > > Cheers, > > -- > Julien Grall
Re: [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation
On 28/07/2022 08:57, Bertrand Marquis wrote: Hi Julien, Hi Bertrand, On 27 Jul 2022, at 16:46, Julien Grall wrote: Hi Xenia, On 27/07/2022 16:32, Xenia Ragiadakou wrote: Remove unused macro atomic_xchg(). Signed-off-by: Xenia Ragiadakou --- xen/arch/arm/include/asm/atomic.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/xen/arch/arm/include/asm/atomic.h b/xen/arch/arm/include/asm/atomic.h index f5ef744b4b..a2dc125291 100644 --- a/xen/arch/arm/include/asm/atomic.h +++ b/xen/arch/arm/include/asm/atomic.h @@ -223,8 +223,6 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u) return __atomic_add_unless(v, a, u); } -#define atomic_xchg(v, new) (xchg(&((v)->counter), new)) - While I agree this is unused today, the wrapper is quite trivial and part of the generic API (x86 also provides one). So I am not in favor of removing it just to please MISRA. That said, if Bertrand and Stefano agrees with removing it then you should also remove the x86 version to avoid inconsistency. I think we can keep this and maybe add a comment on top to document a known violation: /* TODO: MISRA_VIOLATION 2.5 */ While I am fine with this goal of the comment (i.e. indicating where Xen is not MISRA compliant), I think this is one place where I would rather not want one because it can get stale if someones decide to use the function. Cheers, -- Julien Grall
Re: [PATCH v2] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
On 28.07.2022 09:57, Xenia Ragiadakou wrote: > --- a/xen/arch/arm/include/asm/current.h > +++ b/xen/arch/arm/include/asm/current.h > @@ -44,8 +44,11 @@ static inline struct cpu_info *get_cpu_info(void) > > #define guest_cpu_user_regs() (_cpu_info()->guest_cpu_user_regs) > > +extern void return_to_new_vcpu32(void); > +extern void return_to_new_vcpu64(void); While ultimately it's the Arm maintainers to judge, may I suggest that these be put in arm/domain.c to limit visibility? Jan
[PATCH v1] xen: add late init call in start_xen
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.
Sorry for the inconvenience.
I made this patch during using xen for a project.
And I want to share it and ask for review.
Boyoun Park.
From: Boyoun Park
To: xen-devel@lists.xenproject.org
Cc: Stefano Stabellini
Cc: Julien Grall
Cc: Bertrand Marquis
Cc: Volodymyr Babchuk
Cc: Andrew Cooper
Cc: George Dunlap
Cc: Jan Beulich
Cc: Wei Liu
Cc: "Roger Pau Monné"
Date: Tue, 15 Mar 2022 12:57:59 +0900
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
---
xen/arch/arm/setup.c | 2 ++
xen/arch/arm/xen.lds.S | 2 ++
xen/arch/x86/setup.c | 3 +++
xen/arch/x86/xen.lds.S | 2 ++
xen/common/kernel.c| 9 -
xen/include/xen/init.h | 3 +++
6 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 85ff956..332a207 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -1063,6 +1063,8 @@ void __init start_xen(unsigned long boot_phys_offset,
/* Hide UART from DOM0 if we're using it */
serial_endboot();
+do_late_initcalls();
+
if ( (rc = xsm_set_system_active()) != 0 )
panic("xsm: unable to switch to SYSTEM_ACTIVE privilege: %d\n", rc);
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 1e986e2..215e2c3 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -163,6 +163,8 @@ SECTIONS
__presmp_initcall_end = .;
*(.initcall1.init)
__initcall_end = .;
+ *(.initcalllate.init)
+ __late_initcall_end = .;
. = ALIGN(4);
__alt_instructions = .;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index f08b07b..d59298b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1952,6 +1952,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
setup_io_bitmap(dom0);
+do_late_initcalls();
+
if ( bsp_delay_spec_ctrl )
{
info->spec_ctrl_flags &= ~SCF_use_shadow;
@@ -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");
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 8930e14..c90c7b0 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -250,6 +250,8 @@ SECTIONS
__presmp_initcall_end = .;
*(.initcall1.init)
__initcall_end = .;
+ *(.initcalllate.init)
+ __late_initcall_end = .;
*(.init.data)
*(.init.data.rel)
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index f8134d3..5a3d037 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -369,7 +369,7 @@ void add_taint(unsigned int flag)
}
extern const initcall_t __initcall_start[], __presmp_initcall_end[],
-__initcall_end[];
+__initcall_end[], __late_initcall_end[];
void __init do_presmp_initcalls(void)
{
@@ -385,6 +385,13 @@ void __init do_initcalls(void)
(*call)();
}
+void __init do_late_initcalls(void)
+{
+const initcall_t *call;
+for ( call = __initcall_end; call < __late_initcall_end; call++ )
+(*call)();
+}
+
#ifdef CONFIG_HYPFS
static unsigned int __read_mostly major_version;
static unsigned int __read_mostly minor_version;
diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
index 0af0e23..48210ee 100644
--- a/xen/include/xen/init.h
+++ b/xen/include/xen/init.h
@@ -68,11 +68,14 @@ typedef void (*exitcall_t)(void);
const static initcall_t __initcall_##fn __init_call("presmp") = fn
#define __initcall(fn) \
const static initcall_t __initcall_##fn __init_call("1") = fn
+#define __late_initcall(fn) \
+const static initcall_t __initcall_##fn __init_call("late") = fn
#define __exitcall(fn) \
static exitcall_t __exitcall_##fn __exit_call
[xen-unstable test] 171887: tolerable FAIL - PUSHED
flight 171887 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/171887/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stop fail blocked in 171873 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 171873 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 171873 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 171873 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 171873 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 171873 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 171873 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 171873 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 171873 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 171873 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 171873 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 171873 test-amd64-i386-xl-pvshim14 guest-start fail 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-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-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-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 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-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 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-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 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-libvirt 15 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-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl-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 never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass version targeted for testing: xen f732240fd3bac25116151db5ddeb7203b62e85ce baseline version: xen
[PATCH v3 2/2] 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
Reviewed-by: Penny Zheng
---
Changes in v2:
- enable CONFIG_STATIC_MEMORY for all arm64 builds
- use the existing qemu-smoke-arm64.sh with an argument passed for
distinguishing between tests, instead of creating a new script
- test does not rely on kernel logs but prints a message itself on success
- remove the part that enables custom configs for arm64 builds
- add comments
- adapt commit message
Changes in v3:
- refactor the changes to improve readability, no functionality change intended
- add Penny's R-b tag
automation/gitlab-ci/test.yaml | 18 ++
automation/scripts/build | 10 +-
automation/scripts/qemu-smoke-arm64.sh | 25 -
3 files changed, 51 insertions(+), 2 deletions(-)
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 26bdbcc3ea..6f9f64a8da 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -80,6 +80,24 @@ qemu-smoke-arm64-gcc:
tags:
- arm64
+qemu-smoke-arm64-gcc-staticmem:
+ extends: .test-jobs-common
+ variables:
+CONTAINER: debian:unstable-arm64v8
+ script:
+- ./automation/scripts/qemu-smoke-arm64.sh static-mem 2>&1 | tee
qemu-smoke-arm64.log
+ needs:
+- debian-unstable-gcc-arm64
+- kernel-5.9.9-arm64-export
+- qemu-system-aarch64-6.0.0-arm64-export
+ artifacts:
+paths:
+ - smoke.serial
+ - '*.log'
+when: always
+ tags:
+- arm64
+
qemu-smoke-arm32-gcc:
extends: .test-jobs-common
variables:
diff --git a/automation/scripts/build b/automation/scripts/build
index 21b3bc57c8..2b9f2d2b54 100755
--- a/automation/scripts/build
+++ b/automation/scripts/build
@@ -15,7 +15,15 @@ if [[ "${RANDCONFIG}" == "y" ]]; then
make -j$(nproc) -C xen KCONFIG_ALLCONFIG=tools/kconfig/allrandom.config
randconfig
hypervisor_only="y"
else
-make -j$(nproc) -C xen defconfig
+if [[ "${XEN_TARGET_ARCH}" = "arm64" ]]; then
+echo "
+CONFIG_EXPERT=y
+CONFIG_UNSUPPORTED=y
+CONFIG_STATIC_MEMORY=y" > xen/.config
+make -j$(nproc) -C xen olddefconfig
+else
+make -j$(nproc) -C xen defconfig
+fi
fi
# Save the config file before building because build failure causes the script
diff --git a/automation/scripts/qemu-smoke-arm64.sh
b/automation/scripts/qemu-smoke-arm64.sh
index 53086a5ac7..69d9eae7a9 100755
--- a/automation/scripts/qemu-smoke-arm64.sh
+++ b/automation/scripts/qemu-smoke-arm64.sh
@@ -2,6 +2,23 @@
set -ex
+test_variant=$1
+
+passed="BusyBox"
+check=""
+
+if [[ "${test_variant}" == "static-mem" ]]; then
+# Memory range that is statically allocated to DOM1
+domu_base="5000"
+domu_size="1000"
+passed="${test_variant} test passed"
+check="current=\$(hexdump -e '16/1 \"%02x\"'
/proc/device-tree/memory@${domu_base}/reg 2>/dev/null)
+expected=$(printf \"%016x%016x\" 0x${domu_base} 0x${domu_size})
+if [[ \"\${expected}\" == \"\${current}\" ]]; then
+ echo \"${passed}\"
+fi"
+fi
+
# Install QEMU
export DEBIAN_FRONTENT=noninteractive
apt-get -qy update
@@ -43,6 +60,7 @@ echo "#!/bin/sh
mount -t proc proc /proc
mount -t sysfs sysfs /sys
mount -t devtmpfs devtmpfs /dev
+${check}
/bin/sh" > initrd/init
chmod +x initrd/init
cd initrd
@@ -68,6 +86,11 @@ DOMU_MEM[0]="256"
LOAD_CMD="tftpb"
UBOOT_SOURCE="boot.source"
UBOOT_SCRIPT="boot.scr"' > binaries/config
+
+if [[ "${test_variant}" == "static-mem" ]]; then
+echo -e "\nDOMU_STATIC_MEM[0]=\"0x${domu_base} 0x${domu_size}\"" >>
binaries/config
+fi
+
rm -rf imagebuilder
git clone https://gitlab.com/ViryaOS/imagebuilder
bash imagebuilder/scripts/uboot-script-gen -t tftp -d binaries/ -c
binaries/config
@@ -89,5 +112,5 @@ timeout -k 1 240 \
-bios /usr/lib/u-boot/qemu_arm64/u-boot.bin |& tee smoke.serial
set -e
-(grep -q "^BusyBox" smoke.serial && grep -q "DOM1: BusyBox" smoke.serial) ||
exit 1
+(grep -q "^BusyBox" smoke.serial && grep -q "DOM1: ${passed}" smoke.serial) ||
exit 1
exit 0
--
2.34.1
[PATCH v3 1/2] 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
---
Changes in v2:
- add Stefano's R-b tag
Changes in v3:
- none
automation/build/README.md | 3 ---
automation/scripts/build| 4 ++--
automation/scripts/containerize | 1 -
3 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/automation/build/README.md b/automation/build/README.md
index 2137957408..00305eed03 100644
--- a/automation/build/README.md
+++ b/automation/build/README.md
@@ -65,9 +65,6 @@ understands.
- CONTAINER_NO_PULL: If set to 1, the script will not pull from docker hub.
This is useful when testing container locally.
-- XEN_CONFIG_EXPERT: If this is defined in your shell it will be
- automatically passed through to the container.
-
If your docker host has Linux kernel > 4.11, and you want to use containers
that run old glibc (for example, CentOS 6 or SLES11SP4), you may need to add
diff --git a/automation/scripts/build b/automation/scripts/build
index 281f8b1fcc..21b3bc57c8 100755
--- a/automation/scripts/build
+++ b/automation/scripts/build
@@ -91,6 +91,6 @@ for cfg in `ls ${cfg_dir}`; do
echo "Building $cfg"
make -j$(nproc) -C xen clean
rm -f xen/.config
-make -C xen KBUILD_DEFCONFIG=../../../../${cfg_dir}/${cfg}
XEN_CONFIG_EXPERT=y defconfig
-make -j$(nproc) -C xen XEN_CONFIG_EXPERT=y
+make -C xen KBUILD_DEFCONFIG=../../../../${cfg_dir}/${cfg} defconfig
+make -j$(nproc) -C xen
done
diff --git a/automation/scripts/containerize b/automation/scripts/containerize
index 8992c67278..9d4beca4fa 100755
--- a/automation/scripts/containerize
+++ b/automation/scripts/containerize
@@ -101,7 +101,6 @@ exec ${docker_cmd} run \
-v "${CONTAINER_PATH}":/build:rw${selinux} \
-v "${HOME}/.ssh":/root/.ssh:ro \
${SSH_AUTH_DIR:+-v "${SSH_AUTH_DIR}":/tmp/ssh-agent${selinux}} \
-${XEN_CONFIG_EXPERT:+-e XEN_CONFIG_EXPERT=${XEN_CONFIG_EXPERT}} \
${CONTAINER_ARGS} \
-${termint}i --rm -- \
${CONTAINER} \
--
2.34.1
[PATCH v3 0/2] Create a test job for testing static memory on qemu
This patch series - removes all the references to the XEN_CONFIG_EXPERT environmental variable which is not used anymore - creates a trivial arm64 test job that boots xen on qemu with dom0 and a direct mapped dom0less domu with static memory and verifies that domu's memory ranges are the same as the static memory ranges with which it was configured The static memory test relies on the existing qemu-smoke-arm64.sh script. This script uses the kernel-5.9.9 from the test-artifacts container as domu kernel. This particular kernel does not work with dom0less enhanced enabled. More specifically, domu crashes when it attempts to dereference the xenstore interface which is still uninitialized, So, qemu-smoke-arm64-gcc test, as well as its static memory version, fails. To be able to test, I had to disable dom0less enhanced by adding the following commands to the script. sed -i 's/xen,enhanced "enabled"/xen,enhanced "disabled"/g' binaries/boot.source mkimage -A arm64 -T script -C none -d binaries/boot.source binaries/boot.scr These commands are not part of the patch. Since dom0less enhanced is enabled by default, a newer kernel version would be more appropriate for testing dom0less. Xenia Ragiadakou (2): automation: Remove XEN_CONFIG_EXPERT leftovers automation: arm64: Create a test job for testing static allocation on qemu automation/build/README.md | 3 --- automation/gitlab-ci/test.yaml | 18 ++ automation/scripts/build | 14 +++--- automation/scripts/containerize| 1 - automation/scripts/qemu-smoke-arm64.sh | 25 - 5 files changed, 53 insertions(+), 8 deletions(-) -- 2.34.1
Re: [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation
Hi Julien, > On 27 Jul 2022, at 16:46, Julien Grall wrote: > > Hi Xenia, > > On 27/07/2022 16:32, Xenia Ragiadakou wrote: >> Remove unused macro atomic_xchg(). >> Signed-off-by: Xenia Ragiadakou >> --- >> xen/arch/arm/include/asm/atomic.h | 2 -- >> 1 file changed, 2 deletions(-) >> diff --git a/xen/arch/arm/include/asm/atomic.h >> b/xen/arch/arm/include/asm/atomic.h >> index f5ef744b4b..a2dc125291 100644 >> --- a/xen/arch/arm/include/asm/atomic.h >> +++ b/xen/arch/arm/include/asm/atomic.h >> @@ -223,8 +223,6 @@ static inline int atomic_add_unless(atomic_t *v, int a, >> int u) >> return __atomic_add_unless(v, a, u); >> } >> -#define atomic_xchg(v, new) (xchg(&((v)->counter), new)) >> - > > While I agree this is unused today, the wrapper is quite trivial and part of > the generic API (x86 also provides one). So I am not in favor of removing it > just to please MISRA. > > That said, if Bertrand and Stefano agrees with removing it then you should > also remove the x86 version to avoid inconsistency. I think we can keep this and maybe add a comment on top to document a known violation: /* TODO: MISRA_VIOLATION 2.5 */ The FUSA SIG is still working on defining how to document those in the code. I think I suggested one way to do this at some point but the discussion never finished. Cheers Bertrand > > Cheers, > > -- > Julien Grall
[PATCH v2] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
The function idle_loop() is referenced only in domain.c.
Change its linkage from external to internal by adding the storage-class
specifier static to its definitions.
Add the function as a 'fake' input operand to the inline assembly statement,
to make the compiler aware that the function is used.
Fake means that the function is not actually used as an operand by the asm code.
That is because there is not a suitable gcc arm32 asm constraint for labels.
Declare return_to_new_vcpu32() and return_to_new_vcpu64() that are also
referenced by this inline asm statement.
Also, this patch resolves indirectly a MISRA C 2012 Rule 8.4 violation warning.
Signed-off-by: Xenia Ragiadakou
---
Changes in v2:
- remove the 'used' attribute and pass the function as input operand to
the inline asm statement
- declare return_to_new_vcpu32() and return_to_new_vcpu64()
xen/arch/arm/domain.c | 2 +-
xen/arch/arm/include/asm/current.h | 5 -
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 2f8eaab7b5..780b6bcfaa 100644
--- 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)
{
unsigned int cpu = smp_processor_id();
diff --git a/xen/arch/arm/include/asm/current.h
b/xen/arch/arm/include/asm/current.h
index 73e81458e5..225e00af71 100644
--- a/xen/arch/arm/include/asm/current.h
+++ b/xen/arch/arm/include/asm/current.h
@@ -44,8 +44,11 @@ static inline struct cpu_info *get_cpu_info(void)
#define guest_cpu_user_regs() (_cpu_info()->guest_cpu_user_regs)
+extern void return_to_new_vcpu32(void);
+extern void return_to_new_vcpu64(void);
+
#define switch_stack_and_jump(stack, fn) do { \
-asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" ); \
+asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory"
); \
unreachable(); \
} while ( false )
--
2.34.1
Re: [PATCH] xen/xsm: dummy.h: Fix MISRA C 2012 Directive 4.10 violation
> On 27 Jul 2022, at 16:19, Xenia Ragiadakou wrote: > > Protect header file from being included more than once by adding ifndef guard. > > Signed-off-by: Xenia Ragiadakou It makes sense! Reviewed-by: Luca Fancellu
Re: Question to mem-path support at QEMU for Xen
Hi Igor, Appreciate you for the reply! On Wed, Jul 27, 2022 at 04:19:30PM +0800, Igor Mammedov wrote: > On Tue, 26 Jul 2022 15:27:07 +0800 > Huang Rui wrote: > > > Hi Anthony and other Qemu/Xen guys, > > > > We are trying to enable venus on Xen virtualization platform. And we would > > like to use the backend memory with memory-backend-memfd,id=mem1,size=4G > > options on QEMU, however, the QEMU will tell us the "-mem-path" is not > > supported with Xen. I verified the same function on KVM. > > > > qemu-system-i386: -mem-path not supported with Xen > > > > So may I know whether Xen has any limitation that support > > memory-backend-memfd in QEMU or just implementation is not done yet? > > Currently Xen doesn't use guest RAM allocation the way the rest of > accelerators do. (it has hacks in memory_region/ramblock API, > that divert RAM allocation calls to Xen specific API) I am new for Xen and QEMU, we are working on GPU. We would like to have a piece of backend memroy like video memory for VirtIO GPU to support guest VM Mesa Vulkan (Venus). Do you mean we can the memory_region/ramblock APIs to work around it? > > The sane way would extend Xen to accept RAM regions (whatever they are > ram or fd based) QEMU allocates instead of going its own way. This way > it could reuse all memory backends that QEMU provides for the rest of > the non-Xen world. (not to mention that we could drop non trivial > Xen hacks so that guest RAM handling would be consistent with other > accelerators) > May I know what do you mean by "going its own way"? This sounds good, could you please elaborate on how can we implement this? We would like to give a try to address the problem on Xen. Would you mind to point somewhere that I can learn and understand the RAM region. Very happy to see your suggestions! Thanks & Best Regards, Ray
Re: [PATCH V7 10/11] xen/arm: translate virtual PCI bus topology for guests
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. (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.)
Jan
Re: [PATCH V7 06/11] vpci/header: program p2m with guest BAR view
On 27.07.2022 19:06, Oleksandr wrote: > On 27.07.22 13:19, Jan Beulich wrote: >> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote: >>> From: Oleksandr Andrushchenko >>> >>> Take into account guest's BAR view and program its p2m accordingly: >>> gfn is guest's view of the BAR and mfn is the physical BAR value as set >>> up by the PCI bus driver in the hardware domain. >>> This way hardware domain sees physical BAR values and guest sees >>> emulated ones. >>> >>> Signed-off-by: Oleksandr Andrushchenko >> If taking the previous patch as given, the patch here looks okay to me. > > This is a good news, thank you. > > >> But since I'm still not really agreeing with what the previous patch >> does, > > Previous? Sorry, I didn't see any comments given for "[PATCH V7 05/11] > vpci/header: handle p2m range sets per BAR". > > Or do you perhaps mean "[PATCH V7 03/11] vpci/header: implement guest > BAR register handlers" where you explicitly mentioned concerns? No, I mean the previous patch, to which I had given comments in a much earlier version. Roger looks to agree with the approach taken, so my comments were (legitimately) put off. But with me not agreeing with the approach, it's not very reasonable for me to (further) review that change. Hence my deferral to Roger. Jan
Re: [PATCH V7 03/11] vpci/header: implement guest BAR register handlers
On 27.07.2022 18:17, Oleksandr wrote:
> On 27.07.22 13:15, Jan Beulich wrote:
>> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>>> @@ -527,6 +592,17 @@ static int cf_check init_bars(struct pci_dev *pdev)
>>> if ( (val & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO )
>>> {
>>> bars[i].type = VPCI_BAR_IO;
>>> +
>>> +#ifndef CONFIG_X86
>>> +if ( !is_hwdom )
>>> +{
>>> +rc = vpci_add_register(pdev->vpci, empty_bar_read, NULL,
>>> + reg, 4, [i]);
>>> +if ( rc )
>>> +goto fail;
>>> +}
>>> +#endif
>> Since long term this can't be correct, it wants a TODO comment put next
>> to it.
>
>
> Looking into the previous versions of this patch (up to V3) I failed to
> find any changes in current version which hadn't been discussed (and
> agreed in some form).
>
> Could you please clarify what exactly can't be correct the long term,
> for me to put the proper TODO here. Do you perhaps mean that TODO needs
> to explain why we have to diverge?
If a device has I/O port ranges, then that's typically for a reason.
Drivers (in the guest) may therefore want to use those ranges to
communicate with the device. Imagine in particular a device without
any MMIO BARs, and with only I/O port one(s).
>>> @@ -553,34 +635,47 @@ static int cf_check init_bars(struct pci_dev *pdev)
>>> bars[i].size = size;
>>> bars[i].prefetchable = val & PCI_BASE_ADDRESS_MEM_PREFETCH;
>>>
>>> -rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, reg,
>>> 4,
>>> - [i]);
>>> +rc = vpci_add_register(pdev->vpci,
>>> + is_hwdom ? vpci_hw_read32 : guest_bar_read,
>>> + is_hwdom ? bar_write : guest_bar_write,
>>> + reg, 4, [i]);
>>> if ( rc )
>>> -{
>>> -pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
>>> -return rc;
>>> -}
>>> +goto fail;
>>> }
>>>
>>> -/* Check expansion ROM. */
>>> -rc = pci_size_mem_bar(pdev->sbdf, rom_reg, , , PCI_BAR_ROM);
>>> -if ( rc > 0 && size )
>>> +/* Check expansion ROM: we do not handle ROM for guests. */
>>> +if ( is_hwdom )
>> This again can't be right long-term. Personally I'd prefer if the code
>> was (largely) left as is, with adjustments (with suitable TODO comments)
>> made on a much smaller scope only.
>
>
> I can revive a comment that Oleksandr Andrushchenko provided for earlier
> version by transforming into TODO:
>
>
> ROM BAR is only handled for the hardware domain and for guest domains
> there is a stub: at the moment PCI expansion ROM handling is supported
> for x86 only and it might not be used by other architectures without
> emulating x86. Other use-cases may include using that expansion ROM before
> Xen boots, hence no emulation is needed in Xen itself. Or when a guest
> wants to use the ROM code which seems to be rare.
ROMs can contain other than x86 code. While reportedly mostly dead, EFI
bytecode was an example of an abstraction layer supporting arbitrary
architectures. Therefore a comment along these lines would be okay, but
personally I'd prefer it to be less verbose - along the lines of the
one to be supplied for the I/O port restriction.
Jan
Re: [PATCH 8/8] x86/mm: re-arrange type check around _get_page_type()'s TLB flush
On 27.07.2022 20:31, Andrew Cooper wrote: > On 26/07/2022 17:07, Jan Beulich wrote: >> Checks dependent on only d and x can be pulled out, thus allowing to >> skip the flush mask calculation. >> >> Signed-off-by: Jan Beulich > > Do I get a Suggested-by seeing as this was on the very long list of > improvements ? I can add one, but you weren't the only one to notice. > Either way, Reviewed-by: Andrew Cooper Thanks. Jan
Re: [PATCH 7/8] x86/mm: adjust type check around _get_page_type()'s TLB flush
On 27.07.2022 20:25, Andrew Cooper wrote: > On 26/07/2022 17:06, Jan Beulich wrote: >> While "type" can include PGT_pae_xen_l2, "x" can't, as the bit is >> cleared upon de-validation (see also the respective assertion earlier in >> the function). > > While this statement is true, it doesn't really explain why this is > relevant (or not) to TLB flushing. > > As far as the change goes, it's safe on 64bit builds of Xen (I think), > but would not be on 32bit builds when this logic was first written. How that? The flush is needed if the type changes. If (for the purposes here) PGT_pae_xen_l2 was considered part of the type, then the bit being lost upon de-validation would mean we're flushing too little. Jan
Re: [PATCH 2/8] x86/shadow: properly handle get_page() failing
On 27.07.2022 21:06, Andrew Cooper wrote:
> On 27/07/2022 13:53, Jan Beulich wrote:
>> On 27.07.2022 14:46, Andrew Cooper wrote:
>>> On 26/07/2022 17:04, Jan Beulich wrote:
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -132,7 +132,8 @@ delete_fl1_shadow_status(struct domain *
SHADOW_PRINTK("gfn=%"SH_PRI_gfn", type=%08x, smfn=%"PRI_mfn"\n",
gfn_x(gfn), SH_type_fl1_shadow, mfn_x(smfn));
ASSERT(mfn_to_page(smfn)->u.sh.head);
-shadow_hash_delete(d, gfn_x(gfn), SH_type_fl1_shadow, smfn);
+if ( !shadow_hash_delete(d, gfn_x(gfn), SH_type_fl1_shadow, smfn) )
+ASSERT_UNREACHABLE();
>>> I'd recommend crashing the domain here too. I know the we've already
>>> got the return value we want, but this represents corruption in the
>>> shadow pagetable metadata, and I highly doubt it is safe to let the
>>> guest continue to execute in such circumstances.
>> I'm happy to add or convert, but please clarify: DYM in addition to
>> the assertion or in place of it?
>
> Erm pass.
>
> We still don't have a sensible construct for ASSERT_OR_DOMAIN_CRASH(),
> or a well-thought-through piece of guidance.
>
> Probably keep the ASSERT() ? Almost all state checking in the shadow
> code is via asserts.
Meanwhile I was actually leaning towards dropping the assertion. The
goal is to catch attention when things go wrong. A suddenly dying
domain surely ought to be as noticable as an assertion triggering.
> Mostly what I'm concerned by is it feeling weird to have one path which
> only domain crashes, and one path which only asserts.
I'm afraid I've not been successful in determining which other path it
is you're referring to. If I knew, I might be up for converting that
as well (not in this same patch, perhaps). I don't think you mean
set_shadow_status(), since there we can't validly ASSERT(), as the
condition is (in principle) guest controllable.
Jan
[ovmf test] 171894: all pass - PUSHED
flight 171894 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/171894/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 0e7add1d75fc75762208af84579e6809589ea6e5 baseline version: ovmf 1774a44ad91d01294bace32b0060ce26da2f0140 Last test of basis 171892 2022-07-28 00:43:04 Z0 days Testing same since 171894 2022-07-28 03:43:30 Z0 days1 attempts People who touched revisions under test: Anthony PERARD Ard Biesheuvel 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 1774a44ad9..0e7add1d75 0e7add1d75fc75762208af84579e6809589ea6e5 -> xen-tested-master
