[PATCH v6 0/4] [PATCH v5 0/4] introduce generic implementation of macros from bug.h
A large part of the content of the bug.h is repeated among all
architectures (especially x86 and RISCV have the same implementation), so it
was created a new config CONFIG_GENERIC_BUG_FRAME which is used to
introduce generic implementation of do_bug_frame() and move x86's
to with the following changes:
* Add inclusion of arch-specific header
* Rename the guard and remove x86 specific changes
* Wrap macros BUG_FRAME/run_in_exception_handler/WARN/BUG/assert_failed/etc
into #ifndef "BUG_FRAME/run_in_exception_handler/WARN/BUG/assert_failed/etc"
thereby each architecture can override the generic implementation of macros.
* Add #if{n}def __ASSEMBLY__ ... #endif
* Introduce BUG_FRAME_STRUCTURE define to be able to change the structure of
bug
frame
* Introduce BUG_INSTR and BUG_ASM_CONST to make _ASM_BUGFRAME_TEXT reusable
between
architectures
* Introduce BUG_DEBUGGER_TRAP_FATAL to hide details about TRAP_invalid_op for
specific
architecture
* Introduce BUILD_BUG_ON_LINE_WIDTH(line) to make more generic BUG_FRAME
macros
* Make macros related to bug frame structure more generic.
RISC-V will be switched to use and in the future, it will use common
the version of do_bug_frame() when xen/common will work for RISC-V.
---
Changes in V6:
* Update the cover letter message: add information that
BUG_DEBUGGER_TRAP_FATAL() and
BUILD_BUG_ON_LINE_WIDTH().
* Introduce BUILD_BUG_ON_LINE_WIDTH(line) to make more generic BUG_FRAME
macros.
* fix addressed code style
* change -EINVAL to -ENOENT in case when bug_frame wasn't found in generic
do_bug_frame().
* change all 'return id' to 'break' inside switch/case of do_bug_frame().
* move up "#ifndef __ASSEMBLY__" to include BUG_DEBUGGER_TRAP_FATAL.
* update the comment of BUG_ASM_CONST.
* Introduce BUILD_BUG_ON_LINE_WIDTH(line) to make more generic BUG_FRAME macros
* remove #ifndef BUG_FRAME_STRUCT around BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH as
it is
required to be defined before as it is used by x86's
when
the header is included in assembly code.
* add undef of BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH for ARM & x86
as they were introduced unconditionally in .
* update the type of eip to 'void *' in do_invalid_op() for x86
* fix the logic of do_invalid_op() for x86
* move macros BUG_DEBUGGER_TRAP_FATAL under #ifndef __ASSEMBLY__ as
it is not necessary to be in assembly code for x86.
---
Changes in V5:
* Update the cover letter message as the patch, on which the patch series
is based, has been merged to staging.
* Remove "#ifdef BUG_FN_REG..." from generic do_bug_frame() as ARM will
use generic implementation fully.
---
Changes in V4:
* Introduce and use generic BUG_DEBUGGER_TRAP_FATAL(regs) mnacros which is
used to deal with
debugger_trap_fatal(TRAP_invalid_op, regs) where TRAP_invalid_op is
x86-specific.
* Add comment what do_bug_frame() returns.
* Do refactoring of do_bug_frame():
* invert the condition 'if ( region )' in do_bug_frame() to reduce the
indention.
* change type of variable i from 'unsigned int' to 'size_t' as
it is compared with
n_bugs which has type 'size_t'
* Remove '#include ' from as it doesn't need any
more after switch to
x86 implementation.
* Remove '#include ' as it isn't needed any more
* Move bug_*() macros inside '#ifndef BUG_FRAME_STRUCT'
* Add to fix compile issue with BUILD_ON()...
* Add documentation for BUG_ASM_CONST.
* Defines BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH were moved into
"ifndef BUG_FRAME_STRUCT" in as they are specific for 'struct
bug_frame' and so should
co-exist together. So the defines were back to until
BUG_FRAME_STRUCT will be defined in
.
* Switch ARM implementation to generic one
* Remove BUG_FN_REG from arm{16,32}/bug.h as it isn't needed after switch to
generic implementation
* Back comment /* !__ASSEMBLY__ */ for #else case in
* Remove changes related to x86/.../asm/debuger.h as do_bug_frame() prototype
was updated and cpu_user_regs
isn't const any more.
---
Changes in V3:
* Nothing was done with the comment in before
run_in_exception_handler
but I think it can be changed during the merge.
* Add debugger_trap_fatal() to do_bug_frame(). It simplifies usage of
do_bug_frame() for x86 so making handle_bug_frame() and find_bug_frame()
not needed anymore.
* Update do_bug_frame() to return -EINVAL if something goes wrong; otherwise
id of bug_frame
* Update _ASM_BUGFRAME_TEXT to make it more portable.
* Drop unnecessary comments.
* Update patch 2 not to break compilation: move some parts from patches 3 and 4
to patch 2
* As prototype and what do_bug_frame() returns was changed so patch 3 and 4
was updated to use a new version of do_bug_frame
* Change debugger_trap_fatal() prototype for x86 to align with prototype in
---
Changes in V2:
* Update cover letter.
* Switch to x86 implementation as generic as it
Re: [RFC PATCH v1 20/25] hw/xen: Hook up emulated implementation for event channel operations
On 02/03/2023 15:34, David Woodhouse wrote: From: David Woodhouse We provided the backend-facing evtchn functions very early on as part of the core Xen platform support, since things like timers and xenstore need to use them. By what may or may not be an astonishing coincidence, those functions just *happen* all to have exactly the right function prototypes to slot into the evtchn_backend_ops table and be called by the PV backends. It is indeed an amazing coincidence :-) Signed-off-by: David Woodhouse --- hw/i386/kvm/xen_evtchn.c | 15 +++ 1 file changed, 15 insertions(+) Reviewed-by: Paul Durrant
Re: [RFC PATCH v1 19/25] hw/xen: Only advertise ring-page-order for xen-block if gnttab supports it
On 02/03/2023 15:34, David Woodhouse wrote: From: David Woodhouse Whem emulating Xen, multi-page grants are distinctly non-trivial and we have elected not to support them for the time being. Don't advertise them to the guest. Signed-off-by: David Woodhouse --- hw/block/xen-block.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) Reviewed-by: Paul Durrant
[linux-linus test] 179473: regressions - trouble: blocked/fail/pass/starved
flight 179473 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/179473/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-freebsd12-amd64 13 guest-start fail REGR. vs. 178042 test-amd64-amd64-xl-multivcpu 14 guest-start fail REGR. vs. 178042 test-amd64-amd64-xl-credit1 18 guest-localmigrate fail REGR. vs. 178042 test-amd64-amd64-xl-pvshim 14 guest-start fail REGR. vs. 178042 test-amd64-amd64-dom0pvh-xl-amd 13 debian-fixup fail REGR. vs. 178042 test-amd64-amd64-xl 17 guest-saverestorefail REGR. vs. 178042 test-amd64-amd64-xl-pvhv2-intel 14 guest-start fail REGR. vs. 178042 test-amd64-amd64-xl-xsm 14 guest-start fail REGR. vs. 178042 test-amd64-amd64-pair 27 guest-migrate/dst_host/src_host fail REGR. vs. 178042 test-amd64-amd64-xl-pvhv2-amd 14 guest-start fail REGR. vs. 178042 test-amd64-amd64-freebsd11-amd64 13 guest-start fail REGR. vs. 178042 test-amd64-amd64-libvirt 14 guest-start fail REGR. vs. 178042 test-amd64-amd64-xl-shadow 14 guest-start fail REGR. vs. 178042 test-amd64-amd64-libvirt-xsm 14 guest-start fail REGR. vs. 178042 test-amd64-coresched-amd64-xl 20 guest-localmigrate/x10 fail REGR. vs. 178042 test-amd64-amd64-qemuu-nested-intel 13 nested-setup fail REGR. vs. 178042 test-amd64-amd64-xl-credit2 20 guest-localmigrate/x10 fail REGR. vs. 178042 test-amd64-amd64-xl-qemuu-ovmf-amd64 17 guest-saverestore.2 fail REGR. vs. 178042 test-amd64-amd64-libvirt-pair 28 guest-migrate/dst_host/src_host/debian.repeat fail REGR. vs. 178042 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 13 guest-stop fail REGR. vs. 178042 test-amd64-amd64-xl-qemut-debianhvm-i386-xsm 18 guest-localmigrate/x10 fail REGR. vs. 178042 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 12 debian-hvm-install fail REGR. vs. 178042 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 12 debian-hvm-install fail REGR. vs. 178042 test-amd64-amd64-xl-qemuu-debianhvm-amd64 12 debian-hvm-install fail REGR. vs. 178042 test-amd64-amd64-qemuu-nested-amd 12 debian-hvm-install fail REGR. vs. 178042 test-amd64-amd64-xl-qemut-debianhvm-amd64 12 debian-hvm-install fail REGR. vs. 178042 test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 12 debian-hvm-install fail REGR. vs. 178042 test-amd64-amd64-dom0pvh-xl-intel 14 guest-start fail REGR. vs. 178042 test-amd64-amd64-xl-vhd 12 debian-di-installfail REGR. vs. 178042 test-amd64-amd64-pygrub 12 debian-di-installfail REGR. vs. 178042 test-amd64-amd64-libvirt-raw 12 debian-di-installfail REGR. vs. 178042 test-amd64-amd64-libvirt-qcow2 12 debian-di-install fail REGR. vs. 178042 build-arm64-pvops 6 kernel-build fail REGR. vs. 178042 Regressions which are regarded as allowable (not blocking): test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 16 guest-localmigrate fail REGR. vs. 178042 test-amd64-amd64-xl-rtds22 guest-start/debian.repeat fail REGR. vs. 178042 Tests which did not succeed, but are not blocking: test-arm64-arm64-xl-credit1 1 build-check(1) blocked n/a test-arm64-arm64-xl-credit2 1 build-check(1) blocked n/a test-arm64-arm64-xl-thunderx 1 build-check(1) blocked n/a test-arm64-arm64-xl-vhd 1 build-check(1) blocked n/a test-arm64-arm64-xl-xsm 1 build-check(1) blocked n/a test-arm64-arm64-examine 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-raw 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-xl 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 178042 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 178042 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 178042 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 178042 test-armhf-armhf-examine 1 build-check(1) starved n/a test-armhf-armhf-libvirt 1 build-check(1) starved n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) starved n/a test-armhf-armhf-libvirt-raw 1 build-check(1) starved n/a test-armhf-armhf-xl 1 build-check(1) starved n/a test-armhf-armhf-xl-credit1 1 build-check(1) starved n/a test-armhf-armhf-xl-credit2 1 build-check(1) starved n/a test-armhf-armhf-xl-cubietruck 1 build-check(1) starved n/a test-armhf-armhf-xl-multivcpu 1 build-check(1) starved n/a test-armhf-armhf-xl-rtds 1 build-check(1) starved n/a tes
Re: [RFC PATCH v1 18/25] hw/xen: Avoid crash when backend watch fires too early
On 02/03/2023 15:34, David Woodhouse wrote: From: Paul Durrant The xen-block code ends up calling aio_poll() through blkconf_geometry(), which means we see watch events during the indirect call to xendev_class->realize() in xen_device_realize(). Unfortunately this call is made before populating the initial frontend and backend device nodes in xenstore and hence xen_block_frontend_changed() (which is called from a watch event) fails to read the frontend's 'state' node, and hence believes the device is being torn down. This in-turn sets the backend state to XenbusStateClosed and causes the device to be deleted before it is fully set up, leading to the crash. By simply moving the call to xendev_class->realize() after the initial xenstore nodes are populated, this sorry state of affairs is avoided. Reported-by: David Woodhouse Signed-off-by: Paul Durrant Signed-off-by: David Woodhouse --- hw/xen/xen-bus.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) Reviewed-by: Paul Durrant
Re: [RFC PATCH v1 17/25] hw/xen: Build PV backend drivers for CONFIG_XEN_BUS
On 02/03/2023 15:34, David Woodhouse wrote: From: David Woodhouse Now that we have the redirectable Xen backend operations we can build the PV backends even without the Xen libraries. Signed-off-by: David Woodhouse --- hw/9pfs/meson.build| 2 +- hw/block/dataplane/meson.build | 2 +- hw/block/meson.build | 2 +- hw/char/meson.build| 2 +- hw/display/meson.build | 2 +- hw/usb/meson.build | 2 +- hw/xen/meson.build | 5 - 7 files changed, 10 insertions(+), 7 deletions(-) Reviewed-by: Paul Durrant
Re: [PATCH v1 2/2] arch/arm: time: Add support for parsing interrupts by names
Hi Andrei,
> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS)
> wrote:
>
> From: Andrei Cherechesu
>
> Added support for parsing the ARM generic timer interrupts DT
> node by the "interrupt-names" property, if it is available.
>
> If not available, the usual parsing based on the expected
> IRQ order is performed.
>
> Also added the "hyp-virt" PPI to the timer PPI list, even
> though it's currently not in use. If the "hyp-virt" PPI is
> not found, the hypervisor won't panic.
>
> Signed-off-by: Andrei Cherechesu
> ---
> xen/arch/arm/include/asm/time.h | 3 ++-
> xen/arch/arm/time.c | 26 ++
> 2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/time.h b/xen/arch/arm/include/asm/time.h
> index 4b401c1110..49ad8c1a6d 100644
> --- a/xen/arch/arm/include/asm/time.h
> +++ b/xen/arch/arm/include/asm/time.h
> @@ -82,7 +82,8 @@ enum timer_ppi
> TIMER_PHYS_NONSECURE_PPI = 1,
> TIMER_VIRT_PPI = 2,
> TIMER_HYP_PPI = 3,
> -MAX_TIMER_PPI = 4,
> +TIMER_HYP_VIRT_PPI = 4,
> +MAX_TIMER_PPI = 5,
> };
>
> /*
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 433d7be909..794da646d6 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
>
> static unsigned int timer_irq[MAX_TIMER_PPI];
>
> +static const char *timer_irq_names[MAX_TIMER_PPI] = {
> +[TIMER_PHYS_SECURE_PPI] = "sec-phys",
> +[TIMER_PHYS_NONSECURE_PPI] = "phys",
> +[TIMER_VIRT_PPI] = "virt",
> +[TIMER_HYP_PPI] = "hyp-phys",
> +[TIMER_HYP_VIRT_PPI] = "hyp-virt",
> +};
> +
I would need some reference or a pointer to some doc to check those.
> unsigned int timer_get_irq(enum timer_ppi ppi)
> {
> ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
> @@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
> {
> int res;
> unsigned int i;
> +bool has_names;
> +
> +has_names = dt_property_read_bool(timer, "interrupt-names");
>
> /* Retrieve all IRQs for the timer */
> for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
> {
> -res = platform_get_irq(timer, i);
> -
> -if ( res < 0 )
> +if ( has_names )
> +res = platform_get_irq_byname(timer, timer_irq_names[i]);
> +else
> +res = platform_get_irq(timer, i);
> +
> +if ( res > 0 )
The behaviour of the code is changed here compared to the current
version as res = 0 will now generate a panic.
Some device tree might not specify an interrupt number and just put
0 and Xen will now panic on those systems.
As I have no idea if such systems exists and the behaviour is modified
you should justify this and mention it in the commit message or keep
the old behaviour and let 0 go through without a panic.
@stefano, julien any idea here ? should just keep the old behaviour ?
> +timer_irq[i] = res;
> +/* Do not panic if "hyp-virt" PPI is not found, since it's not
> + * currently used.
> + */
Please respect the standard for comments and keep the first line empty:
/*
* comment
*/
> +else if ( i != TIMER_HYP_VIRT_PPI )
> panic("Timer: Unable to retrieve IRQ %u from the device tree\n",
> i);
> -timer_irq[i] = res;
> }
> }
Cheers
Bertrand
>
> --
> 2.35.1
>
>
[xen-unstable test] 179451: tolerable trouble: fail/pass/starved
flight 179451 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/179451/ 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 179392 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 179392 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 179392 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 179392 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 179392 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 179392 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 179392 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 179392 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 179392 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-i386-libvirt-xsm 15 migrate-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-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass build-armhf-libvirt 1 build-check(1) starved n/a test-armhf-armhf-examine 1 build-check(1) starved n/a test-armhf-armhf-libvirt 1 build-check(1) starved n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) starved n/a test-armhf-armhf-libvirt-raw 1 build-check(1) starved n/a test-armhf-armhf-xl 1 build-check(1) starved n/a test-armhf-armhf-xl-credit1 1 build-check(1) starved n/a test-armhf-armhf-xl-credit2 1 build-check(1) starved n/a test-armhf-armhf-xl-cubietruck 1 build-check(1) starved n/a test-armhf-armhf-xl-multivcpu 1 build-check(1) starved n/a test-armhf-armhf-xl-rtds 1 build-check(1) starved n/a test-armhf-armhf-xl-vhd 1 build-check(1) starved n/a build-armhf 2 hosts-allocate starved n/a version targeted for testing: xen 31270f11a96ebb875cd70661e2df9e5c6edd7564 baseline version: xen 31270f11a96ebb875cd70661e2df9e5c6edd7564 Last test of basis 179451 2023-03-07 01:51:56 Z0 days Testing same since (not found) 0 attempts jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64-xtf pass build-amd64 pass build-arm64 pass build-armhf starved build-i386 pass build-amd64-libvirt pass build-arm64-libvirt pass build-armhf-libvirt
Re: [PATCH v1 1/2] arch/arm: irq: Add platform_get_irq_byname() implementation
Hi Andrei,
> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS)
> wrote:
>
> From: Andrei Cherechesu
>
> Moved implementation for the function which parses the IRQs of a DT
> node by the "interrupt-names" property from the SMMU-v3 driver
> to the IRQ core code and made it non-static to be used as helper.
>
> Also changed it to receive a "struct dt_device_node*" as parameter,
> like its counterpart, platform_get_irq(). Updated its usage inside
> the SMMU-v3 driver accordingly.
>
> Signed-off-by: Andrei Cherechesu
Reviewed-by: Bertrand Marquis
Cheers
Bertrand
> ---
> xen/arch/arm/include/asm/irq.h| 2 ++
> xen/arch/arm/irq.c| 14 +++
> xen/drivers/passthrough/arm/smmu-v3.c | 35 +--
> 3 files changed, 22 insertions(+), 29 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/irq.h b/xen/arch/arm/include/asm/irq.h
> index 245f49dcba..af94f41994 100644
> --- a/xen/arch/arm/include/asm/irq.h
> +++ b/xen/arch/arm/include/asm/irq.h
> @@ -89,6 +89,8 @@ int irq_set_type(unsigned int irq, unsigned int type);
>
> int platform_get_irq(const struct dt_device_node *device, int index);
>
> +int platform_get_irq_byname(struct dt_device_node *np, const char *name);
> +
> void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
>
> /*
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 79718f68e7..ded495792b 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -718,6 +718,20 @@ int platform_get_irq(const struct dt_device_node
> *device, int index)
> return irq;
> }
>
> +int platform_get_irq_byname(struct dt_device_node *np, const char *name)
> +{
> + int index;
> +
> + if ( unlikely(!name) )
> + return -EINVAL;
> +
> + index = dt_property_match_string(np, "interrupt-names", name);
> + if ( index < 0 )
> + return index;
> +
> + return platform_get_irq(np, index);
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c
> b/xen/drivers/passthrough/arm/smmu-v3.c
> index d58c5cd0bf..bfdb62b395 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -200,30 +200,6 @@ static inline void dev_iommu_priv_set(struct device
> *dev, void *priv)
> fwspec->iommu_priv = priv;
> }
>
> -static int platform_get_irq_byname_optional(struct device *dev,
> - const char *name)
> -{
> - int index, ret;
> - struct dt_device_node *np = dev_to_dt(dev);
> -
> - if (unlikely(!name))
> - return -EINVAL;
> -
> - index = dt_property_match_string(np, "interrupt-names", name);
> - if (index < 0) {
> - dev_info(dev, "IRQ %s not found\n", name);
> - return index;
> - }
> -
> - ret = platform_get_irq(np, index);
> - if (ret < 0) {
> - dev_err(dev, "failed to get irq index %d\n", index);
> - return -ENODEV;
> - }
> -
> - return ret;
> -}
> -
> /* Start of Linux SMMUv3 code */
> static bool disable_bypass = 1;
>
> @@ -2434,6 +2410,7 @@ static int arm_smmu_device_probe(struct platform_device
> *pdev)
> int irq, ret;
> paddr_t ioaddr, iosize;
> struct arm_smmu_device *smmu;
> + struct dt_device_node *np = dev_to_dt(pdev);
>
> smmu = xzalloc(struct arm_smmu_device);
> if (!smmu)
> @@ -2451,7 +2428,7 @@ static int arm_smmu_device_probe(struct platform_device
> *pdev)
> }
>
> /* Base address */
> - ret = dt_device_get_address(dev_to_dt(pdev), 0, &ioaddr, &iosize);
> + ret = dt_device_get_address(np, 0, &ioaddr, &iosize);
> if (ret)
> goto out_free_smmu;
>
> @@ -2484,19 +2461,19 @@ static int arm_smmu_device_probe(struct
> platform_device *pdev)
>
> /* Interrupt lines */
>
> - irq = platform_get_irq_byname_optional(pdev, "combined");
> + irq = platform_get_irq_byname(np, "combined");
> if (irq > 0)
> smmu->combined_irq = irq;
> else {
> - irq = platform_get_irq_byname_optional(pdev, "eventq");
> + irq = platform_get_irq_byname(np, "eventq");
> if (irq > 0)
> smmu->evtq.q.irq = irq;
>
> - irq = platform_get_irq_byname_optional(pdev, "priq");
> + irq = platform_get_irq_byname(np, "priq");
> if (irq > 0)
> smmu->priq.q.irq = irq;
>
> - irq = platform_get_irq_byname_optional(pdev, "gerror");
> + irq = platform_get_irq_byname(np, "gerror");
> if (irq > 0)
> smmu->gerr_irq = irq;
> }
> --
> 2.35.1
>
>
Re: [PATCH v1 0/2] Fix ARM Generic Timer interrupt parsing
Hi Andrei, When submitting patches, please use the add_maintainer.pl script so that maintainers of the code modified are added in CC. > On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) > wrote: > > From: Andrei Cherechesu > > This 2-patch series fixes the parsing of the ARM Generic Timer > interrupts from the device tree. > > If the generic timer interrupts order in the DT was different than > the expected order in Xen code, these interrupts would no longer be > correctly parsed and registered by Xen, and would result in boot failure. > > This method with using "interrupt-names" for the generic timer interrupts > instead of having them hardcoded in the DTB in a specific order is the newer > approach already implemented in Linux. Xen did not have the necessary code for > this approach, and it has been implemented by the means of this patch series. Would mind giving a link to an example or the Linux documentation if there is one ? Cheers Bertrand > > Functionality should remain the same if "interrupt-names" is not present in > the > Generic Timer DTB node of the platform, but the interrupts should then still > be > present in the expected "sec-phys", "phys", "virt", "hyp-phys", "hyp-virt" > order. > If "interrupt-names" is present, now it is also correctly handled. > > Andrei Cherechesu (2): > arch/arm: irq: Add platform_get_irq_byname() implementation > arch/arm: time: Add support for parsing interrupts by names > > xen/arch/arm/include/asm/irq.h| 2 ++ > xen/arch/arm/include/asm/time.h | 3 ++- > xen/arch/arm/irq.c| 14 +++ > xen/arch/arm/time.c | 26 +--- > xen/drivers/passthrough/arm/smmu-v3.c | 35 +-- > 5 files changed, 46 insertions(+), 34 deletions(-) > > -- > 2.35.1 > >
Re: [PATCH v2 2/2] automation: introduce a dom0less test run on Xilinx hardware
On Mon, Mar 06, 2023 at 03:02:51PM -0800, Stefano Stabellini wrote: > On Mon, 6 Mar 2023, Andrew Cooper wrote: > > On 03/03/2023 11:57 pm, Stefano Stabellini wrote: > > > + only: > > > +variables: > > > + - $XILINX_JOBS == "true" && $CI_COMMIT_REF_PROTECTED == "true" > > > > We don't want to protect every branch of a tree that only a select > > number of people can push to, > > Actually this is useful, more on this below > > > > nor (for this, or others configured with > > the runner), want to impose branching conventions on them. > > > > In all anticipated cases, those able to push would also be able to > > reconfigure the protected-ness of branches, so this doesn't gain us any > > security I don't think, but it certainly puts more hoops in the way to > > be jumped through. > > It is true that it adds a small inconvenience to the user, but I think > the benefits outweigh the inconvenience at the moment (that could change > though.) > > With this, I can register the gitlab runner with a specific gitlab > project (for instance > https://gitlab.com/xen-project/people/sstabellini/xen) then I can mark > all branches as "protected" and select very specific access permissions, > e.g. I can give individual access to Julien, Bertrand, Michal, anyone, > to specific branches, which is great to allow them to run individual > pre-commit tests permanently or temporarily. > > I couldn't find another way to do it at the moment, as non-protected > branches don't come with detailed access permissions. But it is possible > that as we setup a new sub-group under https://gitlab.com/xen-project > for people with access to the runner, then we might be able to remove > this restriction because it becomes unnecessary. We can remove the > protected check at that point. You can configure runner to run only jobs from protected branches. This way it actually prevent running jobs from non-protected branches. Just a condition in .gitlab-ci.yml can be simply removed by anybody who wants to abuse your runner (and have push access to non-protected branch - which may or may not include all of patchew). -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab signature.asc Description: PGP signature
Re: [PATCH 2/2] xen: update CONFIG_DEBUG_INFO help text
On 07.03.2023 16:02, Juergen Gross wrote: > On 07.03.23 15:34, Jan Beulich wrote: >> On 07.03.2023 15:23, Juergen Gross wrote: >>> On 07.03.23 15:18, Jan Beulich wrote: On 07.03.2023 15:04, Juergen Gross wrote: > On 07.03.23 11:41, Jan Beulich wrote: >> On 07.03.2023 07:32, Juergen Gross wrote: >>> --- a/xen/Kconfig.debug >>> +++ b/xen/Kconfig.debug >>> @@ -15,8 +15,11 @@ config DEBUG_INFO >>> bool "Compile Xen with debug info" >>> default DEBUG >>> ---help--- >>> - If you say Y here the resulting Xen will include debugging >>> info >>> - resulting in a larger binary image. >>> + Say Y here if you want to build Xen with debug information. >>> This >>> + information is needed e.g. for doing crash dump analysis of >>> the >>> + hypervisor via the "crash" tool. >>> + Saying Y will increase the size of xen-syms and the built EFI >>> + binary. >> >> Largely fine with me, just one question: Why do you mention xen-syms by >> name, but then verbally describe xen.efi? And since, unlike for xen-syms, > > For xen-syms I couldn't find an easily understandable wording. I'd be fine > with just saying "xen.efi". > >> this affects the installed binary actually used for booting (which may >> be placed on a space constrained partition), it may be prudent to >> mention INSTALL_EFI_STRIP here (as a way to reduce the binary size of >> what ends up on the EFI partition, even if that wouldn't affect the >> "normal" way of putting the binary on the EFI partition - people would >> still need to take care of that in their distros). > > What about adding a related Kconfig option instead? How would a Kconfig option possibly affect this? You want debug info in the xen.efi in its standard install location (outside of the EFI partition); or else if you don't want it there why would you want it in xen-syms? It is the step of populating the EFI partition from the standard install location where some equivalent of INSTALL_EFI_STRIP would come into play. That step is done outside of Xen's build system and hence outside of any Kconfig control. >>> >>> We have 2 binaries for the non-EFI hypervisor (xen-syms and xen[.gz]). >>> Why can't we have the same for EFI? E.g. xen-syms.efi and xen.efi. >>> The former would have the debug-info, the latter could be installed >>> into the EFI partition. >> >> I view the two-binaries model of the non-EFI case as merely an >> implementation detail; > > The ability to do crash dump analysis is more than an implementation > detail IMHO. It is a feature and as such the availability of xen-syms > should be seen as an interface which functionality should be kept. That you're looking the opposite way at things: The oddity is that we can't use xen-syms directly for booting (which is also why it has this specific name; otherwise "xen" would be what the linker produces). >> it just so happens that there's little point >> in mkelf32 retaining debug info. I therefore don't view it as very >> reasonable to artificially introduce yet another binary. > > In case there is no other way to enable hypervisor crash dump analysis > I don't see this as an unreasonable approach. > > It should be verified that this approach is really enabling the crash > dump analysis of a crash dump from a xen.efi booted system, of course. Right. First question would be whether they manage to consume Dwarf debug info from a PE/COFF executable. Possibly the way to go is to separate Dwarf data out of xen.efi into an ELF "container"; I have no idea whether objcopy could be used for something like that. Jan
Re: [PATCH 2/2] xen: update CONFIG_DEBUG_INFO help text
On 07.03.23 15:34, Jan Beulich wrote: On 07.03.2023 15:23, Juergen Gross wrote: On 07.03.23 15:18, Jan Beulich wrote: On 07.03.2023 15:04, Juergen Gross wrote: On 07.03.23 11:41, Jan Beulich wrote: On 07.03.2023 07:32, Juergen Gross wrote: --- a/xen/Kconfig.debug +++ b/xen/Kconfig.debug @@ -15,8 +15,11 @@ config DEBUG_INFO bool "Compile Xen with debug info" default DEBUG ---help--- - If you say Y here the resulting Xen will include debugging info - resulting in a larger binary image. + Say Y here if you want to build Xen with debug information. This + information is needed e.g. for doing crash dump analysis of the + hypervisor via the "crash" tool. + Saying Y will increase the size of xen-syms and the built EFI + binary. Largely fine with me, just one question: Why do you mention xen-syms by name, but then verbally describe xen.efi? And since, unlike for xen-syms, For xen-syms I couldn't find an easily understandable wording. I'd be fine with just saying "xen.efi". this affects the installed binary actually used for booting (which may be placed on a space constrained partition), it may be prudent to mention INSTALL_EFI_STRIP here (as a way to reduce the binary size of what ends up on the EFI partition, even if that wouldn't affect the "normal" way of putting the binary on the EFI partition - people would still need to take care of that in their distros). What about adding a related Kconfig option instead? How would a Kconfig option possibly affect this? You want debug info in the xen.efi in its standard install location (outside of the EFI partition); or else if you don't want it there why would you want it in xen-syms? It is the step of populating the EFI partition from the standard install location where some equivalent of INSTALL_EFI_STRIP would come into play. That step is done outside of Xen's build system and hence outside of any Kconfig control. We have 2 binaries for the non-EFI hypervisor (xen-syms and xen[.gz]). Why can't we have the same for EFI? E.g. xen-syms.efi and xen.efi. The former would have the debug-info, the latter could be installed into the EFI partition. I view the two-binaries model of the non-EFI case as merely an implementation detail; The ability to do crash dump analysis is more than an implementation detail IMHO. It is a feature and as such the availability of xen-syms should be seen as an interface which functionality should be kept. it just so happens that there's little point in mkelf32 retaining debug info. I therefore don't view it as very reasonable to artificially introduce yet another binary. In case there is no other way to enable hypervisor crash dump analysis I don't see this as an unreasonable approach. It should be verified that this approach is really enabling the crash dump analysis of a crash dump from a xen.efi booted system, of course. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [RFC PATCH v1 16/25] hw/xen: Rename xen_common.h to xen_native.h
On 02/03/2023 15:34, David Woodhouse wrote:
From: David Woodhouse
This header is now only for native Xen code, not PV backends that may be
used in Xen emulation. Since the toolstack libraries may depend on the
specific version of Xen headers that they pull in (and will set the
__XEN_TOOLS__ macro to enable internal definitions that they depend on),
the rule is that xen_native.h (and thus the toolstack library headers)
must be included *before* any of the headers in include/hw/xen/interface.
Signed-off-by: David Woodhouse
---
accel/xen/xen-all.c | 1 +
hw/9pfs/xen-9p-backend.c | 1 +
hw/block/dataplane/xen-block.c| 3 ++-
hw/block/xen-block.c | 1 -
hw/i386/pc_piix.c | 4 ++--
hw/i386/xen/xen-hvm.c | 11 +-
hw/i386/xen/xen-mapcache.c| 2 +-
hw/i386/xen/xen_platform.c| 7 +++---
hw/xen/trace-events | 2 +-
hw/xen/xen-operations.c | 2 +-
hw/xen/xen_pt.c | 2 +-
hw/xen/xen_pt.h | 2 +-
hw/xen/xen_pt_config_init.c | 2 +-
hw/xen/xen_pt_msi.c | 4 ++--
include/hw/xen/xen.h | 22 ---
include/hw/xen/{xen_common.h => xen_native.h} | 10 ++---
include/hw/xen/xen_pvdev.h| 3 ++-
17 files changed, 47 insertions(+), 32 deletions(-)
rename include/hw/xen/{xen_common.h => xen_native.h} (98%)
Reviewed-by: Paul Durrant
Re: [RFC PATCH v1 13/25] hw/xen: Add xenstore operations to allow redirection to internal emulation
On 07/03/2023 14:52, David Woodhouse wrote: On Tue, 2023-03-07 at 14:44 +, Paul Durrant wrote: On 02/03/2023 15:34, David Woodhouse wrote: From: Paul Durrant Signed-off-by: Paul Durrant Signed-off-by: David Woodhouse --- Reviewed-by: Paul Durrant You're reviewing your own code on some of those... :) Just making it clear I'm happy with the final version, since it's a joint S-o-b. Do we need to get review from *another* person listed in MAINTAINERS for Xen? Or shall I add my own R-by tags for those ones too?
Re: [RFC PATCH v1 12/25] hw/xen: Add foreignmem operations to allow redirection to internal emulation
On 07/03/2023 14:48, David Woodhouse wrote: On Tue, 2023-03-07 at 14:40 +, Paul Durrant wrote: - -#define xenforeignmemory_unmap(h, p, s) munmap(p, s * XC_PAGE_SIZE) - Actually, probably best 'static inline' that, or at least put brackets round the 'p' and 's' for safety. That's the one we're *removing* :) D'oh... so we are :-) With either one of those options chosen... Reviewed-by: Paul Durrant Taking that anyway.
Re: [RFC PATCH v1 13/25] hw/xen: Add xenstore operations to allow redirection to internal emulation
On Tue, 2023-03-07 at 14:44 +, Paul Durrant wrote: > On 02/03/2023 15:34, David Woodhouse wrote: > > From: Paul Durrant > > > > Signed-off-by: Paul Durrant > > Signed-off-by: David Woodhouse > > --- > > Reviewed-by: Paul Durrant You're reviewing your own code on some of those... :) Do we need to get review from *another* person listed in MAINTAINERS for Xen? Or shall I add my own R-by tags for those ones too? smime.p7s Description: S/MIME cryptographic signature
Re: [RFC PATCH v1 12/25] hw/xen: Add foreignmem operations to allow redirection to internal emulation
On Tue, 2023-03-07 at 14:40 +, Paul Durrant wrote: > > > - > > -#define xenforeignmemory_unmap(h, p, s) munmap(p, s * XC_PAGE_SIZE) > > - > > Actually, probably best 'static inline' that, or at least put brackets > round the 'p' and 's' for safety. > That's the one we're *removing* :) > With either one of those options chosen... > > Reviewed-by: Paul Durrant Taking that anyway. smime.p7s Description: S/MIME cryptographic signature
Re: [RFC PATCH v1 15/25] hw/xen: Use XEN_PAGE_SIZE in PV backend drivers
On 02/03/2023 15:34, David Woodhouse wrote: From: David Woodhouse XC_PAGE_SIZE comes from the actual Xen libraries, while XEN_PAGE_SIZE is provided by QEMU itself in xen_backend_ops.h. For backends which may be built for emulation mode, use the latter. Signed-off-by: David Woodhouse --- hw/block/dataplane/xen-block.c | 8 hw/display/xenfb.c | 12 ++-- hw/net/xen_nic.c | 12 ++-- hw/usb/xen-usb.c | 8 4 files changed, 20 insertions(+), 20 deletions(-) Reviewed-by: Paul Durrant
Re: [RFC PATCH v1 14/25] hw/xen: Move xenstore_store_pv_console_info to xen_console.c
On 02/03/2023 15:34, David Woodhouse wrote: From: David Woodhouse There's no need for this to be in the Xen accel code, and as we want to use the Xen console support with KVM-emulated Xen we'll want to have a platform-agnostic version of it. Make it use GString to build up the path while we're at it. Signed-off-by: David Woodhouse --- accel/xen/xen-all.c | 61 --- hw/char/xen_console.c | 45 +-- include/hw/xen/xen.h | 2 -- 3 files changed, 43 insertions(+), 65 deletions(-) Reviewed-by: Paul Durrant
Re: [RFC PATCH v2 1/2] xen/memory : Add a stats_table resource type
On Thu, Feb 23, 2023 at 01:42:08PM +0100, Jan Beulich wrote:
> On 23.02.2023 13:16, Matias Ezequiel Vara Larsen wrote:
> > On Fri, Feb 17, 2023 at 03:10:53PM +0100, Jan Beulich wrote:
> >> On 17.02.2023 10:29, Matias Ezequiel Vara Larsen wrote:
> >>> On Fri, Feb 17, 2023 at 09:57:43AM +0100, Jan Beulich wrote:
> On 17.02.2023 09:50, Matias Ezequiel Vara Larsen wrote:
> > On Wed, Dec 14, 2022 at 08:56:57AM +0100, Jan Beulich wrote:
> >> On 14.12.2022 08:29, Jan Beulich wrote:
> >>> On 07.10.2022 14:39, Matias Ezequiel Vara Larsen wrote:
> +static int stats_vcpu_alloc_mfn(struct domain *d)
> +{
> +struct page_info *pg;
> +
> +pg = alloc_domheap_page(d, MEMF_no_refcount);
> >>>
> >>> The ioreq and vmtrace resources are also allocated this way, but
> >>> they're
> >>> HVM-specific. The one here being supposed to be VM-type independent,
> >>> I'm
> >>> afraid such pages will be accessible by an "owning" PV domain (it'll
> >>> need to guess the MFN, but that's no excuse).
> >>
> >> Which might be tolerable if it then can't write to the page. That would
> >> require "locking" the page r/o (from guest pov), which ought to be
> >> possible by leveraging a variant of what share_xen_page_with_guest()
> >> does: It marks pages PGT_none with a single type ref. This would mean
> >> ...
> >>
> +if ( !pg )
> +return -ENOMEM;
> +
> +if ( !get_page_and_type(pg, d, PGT_writable_page) ) {
> >>
> >> ... using PGT_none here. Afaict this _should_ work, but we have no
> >> precedent of doing so in the tree, and I may be overlooking something
> >> which prevents that from working.
> >>
> >
> > I do not fully understand this. I checked share_xen_page_with_guest()
> > and I
> > think you're talking about doing something like this for each allocated
> > page to
> > set them ro from a pv guest pov:
> >
> > pg->u.inuse.type_info = PGT_none;
> > pg->u.inuse.type_info |= PGT_validated | 1;
> > page_set_owner(page, d); // not sure if this is needed
> >
> > Then, I should use PGT_none instead of PGT_writable_page in
> > get_page_and_type(). Am I right?
>
> No, if at all possible you should avoid open-coding anything. As said,
> simply passing PGT_none to get_page_and_type() ought to work (again, as
> said, unless I'm overlooking something). share_xen_page_with_guest()
> can do what it does because the page isn't owned yet. For a page with
> owner you may not fiddle with type_info in such an open-coded manner.
>
> >>>
> >>> Thanks. I got the following bug when passing PGT_none:
> >>>
> >>> (XEN) Bad type in validate_page 0 t=0001 c=8042
> >>> (XEN) Xen BUG at mm.c:2643
> >>
> >> The caller of the function needs to avoid the call not only for writable
> >> and shared pages, but also for this new case of PGT_none.
> >
> > Thanks. If I understand correctly, _get_page_type() needs to avoid to call
> > validate_page() when type = PGT_none.
>
> Yes.
>
> > For the writable and shared pages, this
> > is avoided by setting nx |= PGT_validated. Am I right?
>
> Well, no, I wouldn't describe it like that. The two (soon three) types not
> requiring validation simply set the flag without calling validate_page().
>
I see, thanks. I added the corresponding check at _get_page_type() to set the
flag without calling validate_page() for the PGT_none type. I think I am
missing something when I am releasing the pages. I am triggering the following
BUG() when issuing put_page_and_type():
(XEN) Xen BUG at mm.c:2698
This is at devalidate_page(). I guess the call to devalidate_page() shall be
also avoided. I was wondering if put_page_and_type() is required in this case.
Matias
Re: [RFC PATCH v1 13/25] hw/xen: Add xenstore operations to allow redirection to internal emulation
On 02/03/2023 15:34, David Woodhouse wrote: From: Paul Durrant Signed-off-by: Paul Durrant Signed-off-by: David Woodhouse --- accel/xen/xen-all.c | 11 +- hw/char/xen_console.c | 2 +- hw/i386/kvm/xen_xenstore.c | 3 - hw/i386/kvm/xenstore_impl.h | 8 +- hw/xen/xen-bus-helper.c | 62 +++ hw/xen/xen-bus.c| 261 hw/xen/xen-legacy-backend.c | 119 +++-- hw/xen/xen-operations.c | 198 + hw/xen/xen_devconfig.c | 4 +- hw/xen/xen_pt_graphics.c| 1 - hw/xen/xen_pvdev.c | 49 +- include/hw/xen/xen-bus-helper.h | 26 +-- include/hw/xen/xen-bus.h| 17 +- include/hw/xen/xen-legacy-backend.h | 6 +- include/hw/xen/xen_backend_ops.h| 163 + include/hw/xen/xen_common.h | 1 - include/hw/xen/xen_pvdev.h | 2 +- softmmu/globals.c | 1 + 18 files changed, 525 insertions(+), 409 deletions(-) Reviewed-by: Paul Durrant
Re: [RFC PATCH v1 12/25] hw/xen: Add foreignmem operations to allow redirection to internal emulation
On 02/03/2023 15:34, David Woodhouse wrote:
From: David Woodhouse
Signed-off-by: David Woodhouse
Signed-off-by: Paul Durrant
---
hw/char/xen_console.c| 8 +++---
hw/display/xenfb.c | 20 +++---
hw/xen/xen-operations.c | 45
include/hw/xen/xen_backend_ops.h | 26 ++
include/hw/xen/xen_common.h | 13 -
softmmu/globals.c| 1 +
tests/unit/test-xs-node.c| 1 +
7 files changed, 88 insertions(+), 26 deletions(-)
[snip]
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index d4d10d3ff1..632ce617cc 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -32,19 +32,6 @@ typedef xc_interface xenforeignmemory_handle;
#define xenforeignmemory_open(l, f) xen_xc
#define xenforeignmemory_close(h)
-static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
- int prot, size_t pages,
- const xen_pfn_t arr[/*pages*/],
- int err[/*pages*/])
-{
-if (err)
-return xc_map_foreign_bulk(h, dom, prot, arr, err, pages);
-else
-return xc_map_foreign_pages(h, dom, prot, arr, pages);
-}
-
-#define xenforeignmemory_unmap(h, p, s) munmap(p, s * XC_PAGE_SIZE)
-
Actually, probably best 'static inline' that, or at least put brackets
round the 'p' and 's' for safety.
With either one of those options chosen...
Reviewed-by: Paul Durrant
Re: [PATCH 2/2] xen: update CONFIG_DEBUG_INFO help text
On 07.03.2023 15:23, Juergen Gross wrote: > On 07.03.23 15:18, Jan Beulich wrote: >> On 07.03.2023 15:04, Juergen Gross wrote: >>> On 07.03.23 11:41, Jan Beulich wrote: On 07.03.2023 07:32, Juergen Gross wrote: > --- a/xen/Kconfig.debug > +++ b/xen/Kconfig.debug > @@ -15,8 +15,11 @@ config DEBUG_INFO > bool "Compile Xen with debug info" > default DEBUG > ---help--- > - If you say Y here the resulting Xen will include debugging info > - resulting in a larger binary image. > + Say Y here if you want to build Xen with debug information. This > + information is needed e.g. for doing crash dump analysis of the > + hypervisor via the "crash" tool. > + Saying Y will increase the size of xen-syms and the built EFI > + binary. Largely fine with me, just one question: Why do you mention xen-syms by name, but then verbally describe xen.efi? And since, unlike for xen-syms, >>> >>> For xen-syms I couldn't find an easily understandable wording. I'd be fine >>> with just saying "xen.efi". >>> this affects the installed binary actually used for booting (which may be placed on a space constrained partition), it may be prudent to mention INSTALL_EFI_STRIP here (as a way to reduce the binary size of what ends up on the EFI partition, even if that wouldn't affect the "normal" way of putting the binary on the EFI partition - people would still need to take care of that in their distros). >>> >>> What about adding a related Kconfig option instead? >> >> How would a Kconfig option possibly affect this? You want debug info >> in the xen.efi in its standard install location (outside of the EFI >> partition); or else if you don't want it there why would you want it >> in xen-syms? It is the step of populating the EFI partition from the >> standard install location where some equivalent of INSTALL_EFI_STRIP >> would come into play. That step is done outside of Xen's build >> system and hence outside of any Kconfig control. > > We have 2 binaries for the non-EFI hypervisor (xen-syms and xen[.gz]). > Why can't we have the same for EFI? E.g. xen-syms.efi and xen.efi. > The former would have the debug-info, the latter could be installed > into the EFI partition. I view the two-binaries model of the non-EFI case as merely an implementation detail; it just so happens that there's little point in mkelf32 retaining debug info. I therefore don't view it as very reasonable to artificially introduce yet another binary. Another thing would be if there was a way to produce a binary without debug info accompanied by a separate file holding just the debug info. Yet I'm unaware of such being possible with PE/COFF binaries. But yes - technically this might be an option (although, just to mention it, I'm having a vague recollection of there being an issue with this, but I can't say what this might be, plus it is easily possible that I'm misremembering or mixing up things). Jan
Re: [RFC PATCH v1 11/25] hw/xen: Pass grant ref to gnttab unmap operation
On 02/03/2023 15:34, David Woodhouse wrote: From: David Woodhouse The previous commit introduced redirectable gnttab operations fairly much like-for-like, with the exception of the extra arguments to the ->open() call which were always NULL/0 anyway. This *changes* the arguments to the ->unmap() operation to include the original ref# that was mapped. Under real Xen it isn't necessary; all we need to do from QEMU is munmap(), then the kernel will release the grant, and Xen does the tracking/refcounting for the guest. When we have emulated grant tables though, we need to do all that for ourselves. So let's have the back ends keep track of what they mapped and pass it in to the ->unmap() method for us. Signed-off-by: David Woodhouse --- hw/9pfs/xen-9p-backend.c| 7 --- hw/block/dataplane/xen-block.c | 1 + hw/char/xen_console.c | 2 +- hw/net/xen_nic.c| 13 - hw/usb/xen-usb.c| 21 - hw/xen/xen-bus.c| 4 ++-- hw/xen/xen-legacy-backend.c | 4 ++-- hw/xen/xen-operations.c | 9 - include/hw/xen/xen-bus.h| 2 +- include/hw/xen/xen-legacy-backend.h | 6 +++--- include/hw/xen/xen_backend_ops.h| 7 --- 11 files changed, 50 insertions(+), 26 deletions(-) Reviewed-by: Paul Durrant
Re: [PATCH 2/2] xen: update CONFIG_DEBUG_INFO help text
On 07.03.23 15:18, Jan Beulich wrote: On 07.03.2023 15:04, Juergen Gross wrote: On 07.03.23 11:41, Jan Beulich wrote: On 07.03.2023 07:32, Juergen Gross wrote: --- a/xen/Kconfig.debug +++ b/xen/Kconfig.debug @@ -15,8 +15,11 @@ config DEBUG_INFO bool "Compile Xen with debug info" default DEBUG ---help--- - If you say Y here the resulting Xen will include debugging info - resulting in a larger binary image. + Say Y here if you want to build Xen with debug information. This + information is needed e.g. for doing crash dump analysis of the + hypervisor via the "crash" tool. + Saying Y will increase the size of xen-syms and the built EFI + binary. Largely fine with me, just one question: Why do you mention xen-syms by name, but then verbally describe xen.efi? And since, unlike for xen-syms, For xen-syms I couldn't find an easily understandable wording. I'd be fine with just saying "xen.efi". this affects the installed binary actually used for booting (which may be placed on a space constrained partition), it may be prudent to mention INSTALL_EFI_STRIP here (as a way to reduce the binary size of what ends up on the EFI partition, even if that wouldn't affect the "normal" way of putting the binary on the EFI partition - people would still need to take care of that in their distros). What about adding a related Kconfig option instead? How would a Kconfig option possibly affect this? You want debug info in the xen.efi in its standard install location (outside of the EFI partition); or else if you don't want it there why would you want it in xen-syms? It is the step of populating the EFI partition from the standard install location where some equivalent of INSTALL_EFI_STRIP would come into play. That step is done outside of Xen's build system and hence outside of any Kconfig control. We have 2 binaries for the non-EFI hypervisor (xen-syms and xen[.gz]). Why can't we have the same for EFI? E.g. xen-syms.efi and xen.efi. The former would have the debug-info, the latter could be installed into the EFI partition. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [RFC PATCH v1 10/25] hw/xen: Add gnttab operations to allow redirection to internal emulation
On 02/03/2023 15:34, David Woodhouse wrote:
From: David Woodhouse
Move the existing code using libxengnttab to xen-operations.c and allow
the operations to be redirected so that we can add emulation of grant
table mapping for backend drivers.
In emulation, mapping more than one grant ref to be virtually contiguous
would be fairly difficult. The best way to do it might be to make the
ram_block mappings actually backed by a file (shmem or a deleted file,
perhaps) so that we can have multiple *shared* mappings of it. But that
would be fairly intrusive.
Making the backend drivers cope with page *lists* instead of expecting
the mapping to be contiguous is also non-trivial, since some structures
would actually *cross* page boundaries (e.g. the 32-bit blkif responses
which are 12 bytes).
So for now, we'll support only single-page mappings in emulation. Add a
XEN_GNTTAB_OP_FEATURE_MAP_MULTIPLE flag to indicate that the native Xen
implementation *does* support multi-page maps, and a helper function to
query it.
Signed-off-by: David Woodhouse
Signed-off-by: Paul Durrant
[snip]
@@ -65,7 +195,34 @@ struct evtchn_backend_ops libxenevtchn_backend_ops = {
.pending = xenevtchn_pending,
};
+static xengnttab_handle *libxengnttab_backend_open(void)
+{
+return xengnttab_open(NULL, 0);
+}
+
+
+static struct gnttab_backend_ops libxengnttab_backend_ops = {
+.features = XEN_GNTTAB_OP_FEATURE_MAP_MULTIPLE,
+.open = libxengnttab_backend_open,
+.close = xengnttab_close,
+.grant_copy = libxengnttab_fallback_grant_copy,
+.set_max_grants = xengnttab_set_max_grants,
+.map_refs = xengnttab_map_domain_grant_refs,
+.unmap = xengnttab_unmap,
+};
+
void setup_xen_backend_ops(void)
{
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40800
+xengnttab_handle *xgt = xengnttab_open(NULL, 0);
+
+if (xgt) {
+if (xengnttab_grant_copy(xgt, 0, NULL) == 0) {
+xen_gnttab_ops->grant_copy = libxengnttab_backend_grant_copy;
As we found out, this ^^^ is dereferencing a NULL pointer. Switching
'xen_gnttab_ops->' for the obviously intended
'libxengnttab_backend_ops.' fixes the problem.
+}
+xengnttab_close(xgt);
+}
+#endif
xen_evtchn_ops = &libxenevtchn_backend_ops;
+xen_gnttab_ops = &libxengnttab_backend_ops;
With that rectified...
Reviewed-by: Paul Durrant
}
Re: [PATCH 2/2] xen: update CONFIG_DEBUG_INFO help text
On 07.03.2023 15:04, Juergen Gross wrote: > On 07.03.23 11:41, Jan Beulich wrote: >> On 07.03.2023 07:32, Juergen Gross wrote: >>> --- a/xen/Kconfig.debug >>> +++ b/xen/Kconfig.debug >>> @@ -15,8 +15,11 @@ config DEBUG_INFO >>> bool "Compile Xen with debug info" >>> default DEBUG >>> ---help--- >>> - If you say Y here the resulting Xen will include debugging info >>> - resulting in a larger binary image. >>> + Say Y here if you want to build Xen with debug information. This >>> + information is needed e.g. for doing crash dump analysis of the >>> + hypervisor via the "crash" tool. >>> + Saying Y will increase the size of xen-syms and the built EFI >>> + binary. >> >> Largely fine with me, just one question: Why do you mention xen-syms by >> name, but then verbally describe xen.efi? And since, unlike for xen-syms, > > For xen-syms I couldn't find an easily understandable wording. I'd be fine > with just saying "xen.efi". > >> this affects the installed binary actually used for booting (which may >> be placed on a space constrained partition), it may be prudent to >> mention INSTALL_EFI_STRIP here (as a way to reduce the binary size of >> what ends up on the EFI partition, even if that wouldn't affect the >> "normal" way of putting the binary on the EFI partition - people would >> still need to take care of that in their distros). > > What about adding a related Kconfig option instead? How would a Kconfig option possibly affect this? You want debug info in the xen.efi in its standard install location (outside of the EFI partition); or else if you don't want it there why would you want it in xen-syms? It is the step of populating the EFI partition from the standard install location where some equivalent of INSTALL_EFI_STRIP would come into play. That step is done outside of Xen's build system and hence outside of any Kconfig control. Jan > I'd be fine with a textual mentioning of INSTALL_EFI_STRIP, too. > >> I guess this size aspect wrt the EFI partition is actually something >> that should also be mentioned in patch 1, because this can be an argument >> against exposure of the option (precisely because it requires extra >> activity to prevent the EFI partition running out of space). > > This might be another reason to make it easily configurable. > > > Juergen
Re: [PATCH v6 20/20] irqdomain: Switch to per-domain locking
On Tue, 2023-03-07 at 15:06 +0100, Juergen Gross wrote: > On 07.03.23 14:51, David Woodhouse wrote: > > On Mon, 2023-02-13 at 11:43 +0100, Johan Hovold wrote: > > > The IRQ domain structures are currently protected by the global > > > irq_domain_mutex. Switch to using more fine-grained per-domain locking, > > > which can speed up parallel probing by reducing lock contention. > > > > > > On a recent arm64 laptop, the total time spent waiting for the locks > > > during boot drops from 160 to 40 ms on average, while the maximum > > > aggregate wait time drops from 550 to 90 ms over ten runs for example. > > > > > > Note that the domain lock of the root domain (innermost domain) must be > > > used for hierarchical domains. For non-hierarchical domains (as for root > > > domains), the new root pointer is set to the domain itself so that > > > &domain->root->mutex always points to the right lock. > > > > > > Also note that hierarchical domains should be constructed using > > > irq_domain_create_hierarchy() (or irq_domain_add_hierarchy()) to avoid > > > having racing allocations access a not fully initialised domain. As a > > > safeguard, the lockdep assertion in irq_domain_set_mapping() will catch > > > any offenders that also fail to set the root domain pointer. > > > > > > Tested-by: Hsin-Yi Wang > > > Tested-by: Mark-PK Tsai > > > Signed-off-by: Johan Hovold > > > > Broke Xen. > > Fixed with commit ad32ab9604f2. Thanks. smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v6 20/20] irqdomain: Switch to per-domain locking
On 07.03.23 14:51, David Woodhouse wrote: On Mon, 2023-02-13 at 11:43 +0100, Johan Hovold wrote: The IRQ domain structures are currently protected by the global irq_domain_mutex. Switch to using more fine-grained per-domain locking, which can speed up parallel probing by reducing lock contention. On a recent arm64 laptop, the total time spent waiting for the locks during boot drops from 160 to 40 ms on average, while the maximum aggregate wait time drops from 550 to 90 ms over ten runs for example. Note that the domain lock of the root domain (innermost domain) must be used for hierarchical domains. For non-hierarchical domains (as for root domains), the new root pointer is set to the domain itself so that &domain->root->mutex always points to the right lock. Also note that hierarchical domains should be constructed using irq_domain_create_hierarchy() (or irq_domain_add_hierarchy()) to avoid having racing allocations access a not fully initialised domain. As a safeguard, the lockdep assertion in irq_domain_set_mapping() will catch any offenders that also fail to set the root domain pointer. Tested-by: Hsin-Yi Wang Tested-by: Mark-PK Tsai Signed-off-by: Johan Hovold Broke Xen. Fixed with commit ad32ab9604f2. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [RFC PATCH v1 09/25] hw/xen: Add evtchn operations to allow redirection to internal emulation
On 02/03/2023 15:34, David Woodhouse wrote: From: David Woodhouse The existing implementation calling into the real libxenevtchn moves to a new file hw/xen/xen-operations.c, and is called via a function table which in a subsequent commit will also be able to invoke the emulated event channel support. Signed-off-by: David Woodhouse Signed-off-by: Paul Durrant --- hw/9pfs/xen-9p-backend.c| 24 +++--- hw/i386/xen/xen-hvm.c | 27 --- hw/xen/meson.build | 1 + hw/xen/xen-bus.c| 22 +++--- hw/xen/xen-legacy-backend.c | 8 +- hw/xen/xen-operations.c | 71 + hw/xen/xen_pvdev.c | 12 +-- include/hw/xen/xen-bus.h| 1 + include/hw/xen/xen-legacy-backend.h | 1 + include/hw/xen/xen_backend_ops.h| 118 include/hw/xen/xen_common.h | 12 --- include/hw/xen/xen_pvdev.h | 1 + softmmu/globals.c | 1 + 13 files changed, 242 insertions(+), 57 deletions(-) create mode 100644 hw/xen/xen-operations.c create mode 100644 include/hw/xen/xen_backend_ops.h Reviewed-by: Paul Durrant
Re: [PATCH 2/2] xen: update CONFIG_DEBUG_INFO help text
On 07.03.23 11:41, Jan Beulich wrote: On 07.03.2023 07:32, Juergen Gross wrote: --- a/xen/Kconfig.debug +++ b/xen/Kconfig.debug @@ -15,8 +15,11 @@ config DEBUG_INFO bool "Compile Xen with debug info" default DEBUG ---help--- - If you say Y here the resulting Xen will include debugging info - resulting in a larger binary image. + Say Y here if you want to build Xen with debug information. This + information is needed e.g. for doing crash dump analysis of the + hypervisor via the "crash" tool. + Saying Y will increase the size of xen-syms and the built EFI + binary. Largely fine with me, just one question: Why do you mention xen-syms by name, but then verbally describe xen.efi? And since, unlike for xen-syms, For xen-syms I couldn't find an easily understandable wording. I'd be fine with just saying "xen.efi". this affects the installed binary actually used for booting (which may be placed on a space constrained partition), it may be prudent to mention INSTALL_EFI_STRIP here (as a way to reduce the binary size of what ends up on the EFI partition, even if that wouldn't affect the "normal" way of putting the binary on the EFI partition - people would still need to take care of that in their distros). What about adding a related Kconfig option instead? I'd be fine with a textual mentioning of INSTALL_EFI_STRIP, too. I guess this size aspect wrt the EFI partition is actually something that should also be mentioned in patch 1, because this can be an argument against exposure of the option (precisely because it requires extra activity to prevent the EFI partition running out of space). This might be another reason to make it easily configurable. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [RFC PATCH v1 08/25] hw/xen: Create initial XenStore nodes
On 02/03/2023 15:34, David Woodhouse wrote: From: Paul Durrant Signed-off-by: Paul Durrant Signed-off-by: David Woodhouse --- hw/i386/kvm/xen_xenstore.c | 70 ++ 1 file changed, 70 insertions(+) Reviewed-by: Paul Durrant
Re: [PATCH v6 20/20] irqdomain: Switch to per-domain locking
On Mon, 2023-02-13 at 11:43 +0100, Johan Hovold wrote: > The IRQ domain structures are currently protected by the global > irq_domain_mutex. Switch to using more fine-grained per-domain locking, > which can speed up parallel probing by reducing lock contention. > > On a recent arm64 laptop, the total time spent waiting for the locks > during boot drops from 160 to 40 ms on average, while the maximum > aggregate wait time drops from 550 to 90 ms over ten runs for example. > > Note that the domain lock of the root domain (innermost domain) must be > used for hierarchical domains. For non-hierarchical domains (as for root > domains), the new root pointer is set to the domain itself so that > &domain->root->mutex always points to the right lock. > > Also note that hierarchical domains should be constructed using > irq_domain_create_hierarchy() (or irq_domain_add_hierarchy()) to avoid > having racing allocations access a not fully initialised domain. As a > safeguard, the lockdep assertion in irq_domain_set_mapping() will catch > any offenders that also fail to set the root domain pointer. > > Tested-by: Hsin-Yi Wang > Tested-by: Mark-PK Tsai > Signed-off-by: Johan Hovold Broke Xen. And it's *so* easy to test. As long as you have qemu master branch from no older than last Thursday, that is... $ qemu-system-x86_64 -serial mon:stdio -display none \ -accel kvm,xen-version=0x4000e,kernel-irqchip=split \ -kernel arch/x86/boot/bzImage -append "console=ttyS0" ... [0.466554] BUG: kernel NULL pointer dereference, address: 00c0 [0.467249] #PF: supervisor read access in kernel mode [0.467249] #PF: error_code(0x) - not-present page [0.467249] PGD 0 P4D 0 [0.467249] Oops: [#1] PREEMPT SMP PTI [0.467249] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.2.0-rc4+ #1206 [0.467249] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.1-0-g3208b098f51a-prebuilt.qemu.org 04/01/2014 [0.467249] RIP: 0010:irq_domain_create_hierarchy+0x2c/0x70 [0.467249] Code: 1e fa 0f 1f 44 00 00 41 54 49 89 fc 48 89 cf 55 89 f5 53 85 d2 74 40 89 d6 31 c9 89 d2 e8 2c fa ff ff 48 89 c3 48 85 db 74 21 <49> 8b 84 24 c0 00 00 00 09 6b 28 48 89 df 4c 89 a3 f0 00 00 00 48 [0.467249] RSP: :c9013e60 EFLAGS: 00010286 [0.467249] RAX: 8880053a1a00 RBX: 8880053a1a00 RCX: [0.467249] RDX: RSI: 0001 RDI: 84828fa0 [0.467249] RBP: 0010 R08: 0003 R09: [0.467249] R10: 25a89be7 R11: 442a63fa R12: [0.467249] R13: 83ac1b98 R14: R15: [0.467249] FS: () GS:888007a0() knlGS: [0.467249] CS: 0010 DS: ES: CR0: 80050033 [0.467249] CR2: 00c0 CR3: 02824000 CR4: 06f0 [0.467249] Call Trace: [0.467249] [0.467249] ? __pfx_pci_arch_init+0x10/0x10 [0.467249] __msi_create_irq_domain+0x85/0x170 [0.467249] ? __pfx_pci_arch_init+0x10/0x10 [0.467249] xen_create_pci_msi_domain+0x34/0x40 [0.467249] x86_create_pci_msi_domain+0x12/0x1e [0.467249] pci_arch_init+0x31/0x7a [0.467249] ? __pfx_pci_arch_init+0x10/0x10 [0.467249] do_one_initcall+0x5f/0x320 [0.467249] ? rcu_read_lock_sched_held+0x43/0x80 [0.467249] kernel_init_freeable+0x189/0x1c6 [0.467249] ? __pfx_kernel_init+0x10/0x10 [0.467249] kernel_init+0x1a/0x130 [0.467249] ret_from_fork+0x2c/0x50 [0.467249] [0.467249] Modules linked in: [0.467249] CR2: 00c0 [0.467249] ---[ end trace ]--- [0.467249] RIP: 0010:irq_domain_create_hierarchy+0x2c/0x70 [0.467249] Code: 1e fa 0f 1f 44 00 00 41 54 49 89 fc 48 89 cf 55 89 f5 53 85 d2 74 40 89 d6 31 c9 89 d2 e8 2c fa ff ff 48 89 c3 48 85 db 74 21 <49> 8b 84 24 c0 00 00 00 09 6b 28 48 89 df 4c 89 a3 f0 00 00 00 48 [0.467249] RSP: :c9013e60 EFLAGS: 00010286 [0.467249] RAX: 8880053a1a00 RBX: 8880053a1a00 RCX: [0.467249] RDX: RSI: 0001 RDI: 84828fa0 [0.467249] RBP: 0010 R08: 0003 R09: [0.467249] R10: 25a89be7 R11: 442a63fa R12: [0.467249] R13: 83ac1b98 R14: R15: [0.467249] FS: () GS:888007a0() knlGS: [0.467249] CS: 0010 DS: ES: CR0: 80050033 [0.467249] CR2: 00c0 CR3: 02824000 CR4: 06f0 [0.467249] Kernel panic - not syncing: Fatal exception smime.p7s Description: S/MIME cryptographic signature
Re: [RFC PATCH v1 07/25] hw/xen: Implement core serialize/deserialize methods for xenstore_impl
On 02/03/2023 15:34, David Woodhouse wrote:
From: David Woodhouse
In fact I think we want to only serialize the contents of the domain's
path in /local/domain/${domid} and leave the rest to be recreated? Will
defer to Paul for that.
Yes. Assuming backends have properly implemented save/restore code then
they should be able to re-create their xenstore content. Also, it's
generally convenient for them to start back in 'InitWait' state the
drive their state machine back to 'Connected' so that grant refs can be
mapped, event channels bound etc. along the way.
Signed-off-by: David Woodhouse
---
hw/i386/kvm/xen_xenstore.c | 25 +-
hw/i386/kvm/xenstore_impl.c | 574 +++-
hw/i386/kvm/xenstore_impl.h | 5 +
tests/unit/test-xs-node.c | 236 ++-
4 files changed, 824 insertions(+), 16 deletions(-)
The code LGTM though; just need to limit the saved tree to
'/local/domain/'
Paul
[ovmf test] 179496: all pass - PUSHED
flight 179496 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/179496/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf aa1cd447b346e8cc8141df2fe2d321b032c08acb baseline version: ovmf c7c25997595aa34ce0a7a21ca2e1fc5b0f9b38a6 Last test of basis 179487 2023-03-07 09:43:43 Z0 days Testing same since 179496 2023-03-07 11:42:13 Z0 days1 attempts People who touched revisions under test: Gerd Hoffmann Liu, Zhiguang Zhiguang Liu 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 c7c2599759..aa1cd447b3 aa1cd447b346e8cc8141df2fe2d321b032c08acb -> xen-tested-master
Re: [RFC PATCH v1 06/25] hw/xen: Implement XenStore permissions
On 02/03/2023 15:34, David Woodhouse wrote: From: Paul Durrant Store perms as a GList of strings, check permissions. Signed-off-by: Paul Durrant Signed-off-by: David Woodhouse --- hw/i386/kvm/xen_xenstore.c | 2 +- hw/i386/kvm/xenstore_impl.c | 259 +--- hw/i386/kvm/xenstore_impl.h | 8 +- tests/unit/test-xs-node.c | 27 +++- 4 files changed, 275 insertions(+), 21 deletions(-) Reviewed-by: Paul Durrant
Re: [RFC PATCH v1 05/25] hw/xen: Watches on XenStore transactions
On Tue, 2023-03-07 at 13:32 +, Paul Durrant wrote: > > Reviewed-by: Paul Durrant > > ... with a couple of nits in comments called out below... Thanks, I'm fixing these and pushing them out to my tree at https://git.infradead.org/users/dwmw2/qemu.git/shortlog/refs/heads/xenfv (and occasionally to Gitlab CI) as you go. Still harbouring the fantasy that we might manage to make the soft freeze deadline :) smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v5 4/4] xen/x86: switch x86 to use generic implemetation of bug.h
On Tue, 2023-03-07 at 13:59 +0100, Jan Beulich wrote:
> On 07.03.2023 13:52, Oleksii wrote:
> > On Mon, 2023-03-06 at 11:36 +0100, Jan Beulich wrote:
> > > On 03.03.2023 11:38, Oleksii Kurochko wrote:
> > > > --- a/xen/arch/x86/include/asm/bug.h
> > > > +++ b/xen/arch/x86/include/asm/bug.h
> > > > @@ -1,79 +1,12 @@
> > > > #ifndef __X86_BUG_H__
> > > > #define __X86_BUG_H__
> > > >
> > > > -#define BUG_DISP_WIDTH 24
> > > > -#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> > > > -#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> > > > +#define BUG_DEBUGGER_TRAP_FATAL(regs)
> > > > debugger_trap_fatal(X86_EXC_GP,regs)
> > >
> > > Along the lines of a comment on an earlier patch, please move
> > > this
> > > ...
> > >
> > > > #ifndef __ASSEMBLY__
> > >
> > > ... into the thus guarded section.
> > But it was defined there before the patch series and these definies
> > are
> > used in "#else /* !__ASSEMBLY__ */"
>
> Since you use plural, maybe a misunderstanding? My comment was solely
> on the line you add; the removed lines are there merely as context.
Really. I misunderstood you.
>
> > > > @@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const
> > > > struct
> > > > vcpu *v, uint32_t leaf,
> > > >
> > > > void do_invalid_op(struct cpu_user_regs *regs)
> > > > {
> > > > - const struct bug_frame *bug = NULL;
> > > > u8 bug_insn[2];
> > > > - const char *prefix = "", *filename, *predicate, *eip =
> > > > (char
> > > > *)regs->rip;
> > > > - unsigned long fixup;
> > > > - int id = -1, lineno;
> > > > - const struct virtual_region *region;
> > > > + const char *eip = (char *)regs->rip;
> > >
> > > I realize "char" was used before, but now that this is all on its
> > > own,
> > > can this at least become "unsigned char", but yet better "void"?
> > If we change it to "void" shouldn't it require additional casts
> > here (
> > which is not nice ):
> > eip += sizeof(bug_insn);
>
> Arithmetic on pointers to void is a GNU extension which we make
> heavy use of all over the hypervisor.
Got it. Than I'll rework it with 'void *'.
>
> > > > switch ( id )
> > > > {
> > > > + case BUGFRAME_run_fn:
> > > > case BUGFRAME_warn:
> > > > - printk("Xen WARN at %s%s:%d\n", prefix, filename,
> > > > lineno);
> > > > - show_execution_state(regs);
> > > > fixup_exception_return(regs, (unsigned long)eip);
> > > > return;
> > > > -
> > > > - case BUGFRAME_bug:
> > > > - printk("Xen BUG at %s%s:%d\n", prefix, filename,
> > > > lineno);
> > > > -
> > > > - if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
> > > > - return;
> > >
> > > This and ...
> > >
> > > > - show_execution_state(regs);
> > > > - panic("Xen BUG at %s%s:%d\n", prefix, filename,
> > > > lineno);
> > > > -
> > > > - case BUGFRAME_assert:
> > > > - /* ASSERT: decode the predicate string pointer. */
> > > > - predicate = bug_msg(bug);
> > > > - if ( !is_kernel(predicate) && !is_patch(predicate) )
> > > > - predicate = "";
> > > > -
> > > > - printk("Assertion '%s' failed at %s%s:%d\n",
> > > > - predicate, prefix, filename, lineno);
> > > > -
> > > > - if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
> > > > - return;
> > >
> > > ... this return look to have no proper representation in the new
> > > logic; both cases fall through to ...
> > >
> > > > - show_execution_state(regs);
> > > > - panic("Assertion '%s' failed at %s%s:%d\n",
> > > > - predicate, prefix, filename, lineno);
> > > > }
> > > >
> > > > die:
> > >
> > > ... here now, which is an unwanted functional change.
> > Oh, you are right. So then in case we have correct id it is needed
> > to
> > do only return:
> > switch ( id )
> > {
> > case BUGFRAME_run_fn:
> > case BUGFRAME_warn:
> > fixup_exception_return(regs, (unsigned long)eip);
> > break;
> > }
> >
> > return;
>
> Except the "return" needs to remain inside the switch; unrecognized
> id
> values should still fall through to the "die" label.
Yeah, it is still possible to get unrecognized id.
Thanks.
>
~ Oleksii
Re: [RFC PATCH v1 05/25] hw/xen: Watches on XenStore transactions
On 02/03/2023 15:34, David Woodhouse wrote:
From: David Woodhouse
Firing watches on the nodes that still exist is relatively easy; just
walk the tree and look at the nodes with refcount of one.
Firing watches on *deleted* nodes is more fun. We add 'modified_in_tx'
and 'deleted_in_tx' flags to each node. Nodes with those flags cannot
be shared, as they will always be unique to the transaction in which
they were created.
When xs_node_walk would need to *create* a node as scaffolding and it
encounters a deleted_in_tx node, it can resurrect it simply by clearing
its deleted_in_tx flag. If that node originally had any *data*, they're
gone, and the modified_in_tx flag will have been set when it was first
deleted.
We then attempt to send appropriate watches when the transaction is
committed, properly delete the deleted_in_tx nodes, and remove the
modified_in_tx flag from the others.
Signed-off-by: David Woodhouse
---
hw/i386/kvm/xenstore_impl.c | 151 ++-
tests/unit/test-xs-node.c | 231 +++-
2 files changed, 380 insertions(+), 2 deletions(-)
Reviewed-by: Paul Durrant
... with a couple of nits in comments called out below...
[snip]
+static gboolean tx_commit_walk(gpointer key, gpointer value,
+ gpointer user_data)
+{
+struct walk_op *op = user_data;
+int path_len = strlen(op->path);
+int key_len = strlen(key);
+bool fire_parents = true;
+XsWatch *watch;
+XsNode *n = value;
+
+if (n->ref != 1) {
+return false;
+}
+
+if (n->deleted_in_tx) {
+/*
+ * We first watches on our parents if we are the *first* node
We first *fire* watches on our parents...
+ * to be deleted (the topmost one). This matches the behaviour
+ * when deleting in the live tree.
+ */
+fire_parents = !op->deleted_in_tx;
+
+/* Only used on the way down so no need to clear it later */
+op->deleted_in_tx = true;
+}
+
+assert(key_len + path_len + 2 <= sizeof(op->path));
+op->path[path_len] = '/';
+memcpy(op->path + path_len + 1, key, key_len + 1);
+
+watch = g_hash_table_lookup(op->s->watches, op->path);
+if (watch) {
+op->watches = g_list_append(op->watches, watch);
+}
+
+if (n->children) {
+g_hash_table_foreach_remove(n->children, tx_commit_walk, op);
+}
+
+if (watch) {
+op->watches = g_list_remove(op->watches, watch);
+}
+
+/*
+ * Don't fire watches if this node was only copied because a
+ * descendent was changed. The modifieD_in_tx flag indicates the
s/modifieD/modified
+ * ones which were really changed.
+ */
+if (n->modified_in_tx || n->deleted_in_tx) {
+fire_watches(op, fire_parents);
+n->modified_in_tx = false;
+}
+op->path[path_len] = '\0';
+
+/* Deleted nodes really do get expunged when we commit */
+return n->deleted_in_tx;
+}
Re: [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
On Tue, 2023-03-07 at 14:16 +0100, Jan Beulich wrote:
> On 07.03.2023 14:13, Oleksii wrote:
> > > > > > > +
> > > > > > > +#define BUG_FRAME(type, line, ptr, second_frame, msg) do
> > > > > > > { \
> > > > > > > + BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
> > > > > > > BUG_LINE_HI_WIDTH)); \
> > > > > > > + BUILD_BUG_ON((type) >=
> > > > > > > BUGFRAME_NR); \
> > > > > > > + asm volatile (
> > > > > > > _ASM_BUGFRAME_TEXT(second_frame)
> > > > > > > \
> > > > > > > + :: _ASM_BUGFRAME_INFO(type, line,
> > > > > > > ptr,
> > > > > > > msg)
> > > > > > > ); \
> > > > > > > +} while (0)
> > > > >
> > > > > Isn't this tied to BUG_FRAME_STRUCT being defined (or not)?
> > > > > At
> > > > > least
> > > > > the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to
> > > > > use
> > > > > the generic struct: With how you have things right now
> > > > > BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check
> > > > > would
> > > > > also
> > > > > be at risk of causing false positives. Perhaps it's
> > > > > appropriate
> > > > > to
> > > > > have a separate #ifdef (incl the distinct identifier used),
> > > > > but
> > > > > that
> > > > > first BUILD_BUG_ON() needs abstracting out.
> > > Missed that. Thanks.
> > > I'll introduce that a separate #ifdef before BUG_FRAME:
> > >
> > > #ifndef BUILD_BUG_ON_LINE_WIDTH
> > > #define BUILD_BUG_ON_LINE_WIDTH \
> > > BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
> > > BUG_LINE_HI_WIDTH))
> > > #endif
> > I think even better will be to do in the following way:
> >
> > #ifndef LINE_WIDTH
> > #define LINE_WIDTH (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)
> > #endif
> >
> > #define BUG_FRAME(type, line, ptr, second_frame, msg) do
> > {
> > \
> > BUILD_BUG_ON((line) >>
> > LINE_WIDTH);
> > \
> > BUILD_BUG_ON((type) >=
> > BUGFRAME_NR);
> > \
> > asm volatile (
> > _ASM_BUGFRAME_TEXT(second_frame)
> > \
> > :: _ASM_BUGFRAME_INFO(type, line, ptr, msg)
> > );
> > \
> > } while (false)
>
> Not sure - that'll still require arches to define LINE_WIDTH. What
> if they store the line number in a 32-bit field? Then defining
> LINE_WIDTH to 32 would yield undefined behavior here.
>
It might be an issue.
Than it will be better to have function-like macros mentioned in
previous e-mail.
Thanks
~ Oleksii
Re: [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
On 07.03.2023 14:13, Oleksii wrote:
>> +
>> +#define BUG_FRAME(type, line, ptr, second_frame, msg) do
>> { \
>> + BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
>> BUG_LINE_HI_WIDTH)); \
>> + BUILD_BUG_ON((type) >=
>> BUGFRAME_NR); \
>> + asm volatile (
>> _ASM_BUGFRAME_TEXT(second_frame) \
>> + :: _ASM_BUGFRAME_INFO(type, line, ptr,
>> msg)
>> ); \
>> +} while (0)
Isn't this tied to BUG_FRAME_STRUCT being defined (or not)? At
least
the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to use
the generic struct: With how you have things right now
BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check would
also
be at risk of causing false positives. Perhaps it's appropriate
to
have a separate #ifdef (incl the distinct identifier used), but
that
first BUILD_BUG_ON() needs abstracting out.
>> Missed that. Thanks.
>> I'll introduce that a separate #ifdef before BUG_FRAME:
>>
>> #ifndef BUILD_BUG_ON_LINE_WIDTH
>> #define BUILD_BUG_ON_LINE_WIDTH \
>> BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
>> BUG_LINE_HI_WIDTH))
>> #endif
> I think even better will be to do in the following way:
>
> #ifndef LINE_WIDTH
> #define LINE_WIDTH (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)
> #endif
>
> #define BUG_FRAME(type, line, ptr, second_frame, msg) do {
> \
> BUILD_BUG_ON((line) >> LINE_WIDTH);
> \
> BUILD_BUG_ON((type) >= BUGFRAME_NR);
> \
> asm volatile ( _ASM_BUGFRAME_TEXT(second_frame)
> \
>:: _ASM_BUGFRAME_INFO(type, line, ptr, msg) );
> \
> } while (false)
Not sure - that'll still require arches to define LINE_WIDTH. What
if they store the line number in a 32-bit field? Then defining
LINE_WIDTH to 32 would yield undefined behavior here.
Jan
Re: [RFC PATCH v1 04/25] hw/xen: Implement XenStore transactions
On 02/03/2023 15:34, David Woodhouse wrote: From: David Woodhouse Given that the whole thing supported copy on write from the beginning, transactions end up being fairly simple. On starting a transaction, just take a ref of the existing root; swap it back in on a successful commit. The main tree has a transaction ID too, and we keep a record of the last transaction ID given out. if the main tree is ever modified when it isn't the latest, it gets a new transaction ID. A commit can only succeed if the main tree hasn't moved on since it was forked. Strictly speaking, the XenStore protocol allows a transaction to succeed as long as nothing *it* read or wrote has changed in the interim, but no implementations do that; *any* change is sufficient to abort a transaction. This does not yet fire watches on the changed nodes on a commit. That bit is more fun and will come in a follow-on commit. Signed-off-by: David Woodhouse --- hw/i386/kvm/xenstore_impl.c | 150 ++-- tests/unit/test-xs-node.c | 118 2 files changed, 262 insertions(+), 6 deletions(-) Reviewed-by: Paul Durrant
Re: [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
> > > > > +
> > > > > +#define BUG_FRAME(type, line, ptr, second_frame, msg) do
> > > > > { \
> > > > > + BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
> > > > > BUG_LINE_HI_WIDTH)); \
> > > > > + BUILD_BUG_ON((type) >=
> > > > > BUGFRAME_NR); \
> > > > > + asm volatile (
> > > > > _ASM_BUGFRAME_TEXT(second_frame) \
> > > > > + :: _ASM_BUGFRAME_INFO(type, line, ptr,
> > > > > msg)
> > > > > ); \
> > > > > +} while (0)
> > >
> > > Isn't this tied to BUG_FRAME_STRUCT being defined (or not)? At
> > > least
> > > the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to use
> > > the generic struct: With how you have things right now
> > > BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check would
> > > also
> > > be at risk of causing false positives. Perhaps it's appropriate
> > > to
> > > have a separate #ifdef (incl the distinct identifier used), but
> > > that
> > > first BUILD_BUG_ON() needs abstracting out.
> Missed that. Thanks.
> I'll introduce that a separate #ifdef before BUG_FRAME:
>
> #ifndef BUILD_BUG_ON_LINE_WIDTH
> #define BUILD_BUG_ON_LINE_WIDTH \
> BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
> BUG_LINE_HI_WIDTH))
> #endif
I think even better will be to do in the following way:
#ifndef LINE_WIDTH
#define LINE_WIDTH (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)
#endif
#define BUG_FRAME(type, line, ptr, second_frame, msg) do {
\
BUILD_BUG_ON((line) >> LINE_WIDTH);
\
BUILD_BUG_ON((type) >= BUGFRAME_NR);
\
asm volatile ( _ASM_BUGFRAME_TEXT(second_frame)
\
:: _ASM_BUGFRAME_INFO(type, line, ptr, msg) );
\
} while (false)
~ Oleksii
Re: [PATCH v5 4/4] xen/x86: switch x86 to use generic implemetation of bug.h
On 07.03.2023 13:52, Oleksii wrote:
> On Mon, 2023-03-06 at 11:36 +0100, Jan Beulich wrote:
>> On 03.03.2023 11:38, Oleksii Kurochko wrote:
>>> --- a/xen/arch/x86/include/asm/bug.h
>>> +++ b/xen/arch/x86/include/asm/bug.h
>>> @@ -1,79 +1,12 @@
>>> #ifndef __X86_BUG_H__
>>> #define __X86_BUG_H__
>>>
>>> -#define BUG_DISP_WIDTH 24
>>> -#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
>>> -#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
>>> +#define BUG_DEBUGGER_TRAP_FATAL(regs)
>>> debugger_trap_fatal(X86_EXC_GP,regs)
>>
>> Along the lines of a comment on an earlier patch, please move this
>> ...
>>
>>> #ifndef __ASSEMBLY__
>>
>> ... into the thus guarded section.
> But it was defined there before the patch series and these definies are
> used in "#else /* !__ASSEMBLY__ */"
Since you use plural, maybe a misunderstanding? My comment was solely
on the line you add; the removed lines are there merely as context.
>>> @@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const struct
>>> vcpu *v, uint32_t leaf,
>>>
>>> void do_invalid_op(struct cpu_user_regs *regs)
>>> {
>>> - const struct bug_frame *bug = NULL;
>>> u8 bug_insn[2];
>>> - const char *prefix = "", *filename, *predicate, *eip = (char
>>> *)regs->rip;
>>> - unsigned long fixup;
>>> - int id = -1, lineno;
>>> - const struct virtual_region *region;
>>> + const char *eip = (char *)regs->rip;
>>
>> I realize "char" was used before, but now that this is all on its
>> own,
>> can this at least become "unsigned char", but yet better "void"?
> If we change it to "void" shouldn't it require additional casts here (
> which is not nice ):
>eip += sizeof(bug_insn);
Arithmetic on pointers to void is a GNU extension which we make
heavy use of all over the hypervisor.
>>> switch ( id )
>>> {
>>> + case BUGFRAME_run_fn:
>>> case BUGFRAME_warn:
>>> - printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
>>> - show_execution_state(regs);
>>> fixup_exception_return(regs, (unsigned long)eip);
>>> return;
>>> -
>>> - case BUGFRAME_bug:
>>> - printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
>>> -
>>> - if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
>>> - return;
>>
>> This and ...
>>
>>> - show_execution_state(regs);
>>> - panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
>>> -
>>> - case BUGFRAME_assert:
>>> - /* ASSERT: decode the predicate string pointer. */
>>> - predicate = bug_msg(bug);
>>> - if ( !is_kernel(predicate) && !is_patch(predicate) )
>>> - predicate = "";
>>> -
>>> - printk("Assertion '%s' failed at %s%s:%d\n",
>>> - predicate, prefix, filename, lineno);
>>> -
>>> - if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
>>> - return;
>>
>> ... this return look to have no proper representation in the new
>> logic; both cases fall through to ...
>>
>>> - show_execution_state(regs);
>>> - panic("Assertion '%s' failed at %s%s:%d\n",
>>> - predicate, prefix, filename, lineno);
>>> }
>>>
>>> die:
>>
>> ... here now, which is an unwanted functional change.
> Oh, you are right. So then in case we have correct id it is needed to
> do only return:
> switch ( id )
> {
> case BUGFRAME_run_fn:
> case BUGFRAME_warn:
> fixup_exception_return(regs, (unsigned long)eip);
> break;
> }
>
> return;
Except the "return" needs to remain inside the switch; unrecognized id
values should still fall through to the "die" label.
Jan
Re: [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
On 07.03.2023 12:32, Oleksii wrote:
> On Mon, 2023-03-06 at 11:17 +0100, Jan Beulich wrote:
>>> On 03.03.2023 11:38, Oleksii Kurochko wrote:
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -28,6 +28,9 @@ config ALTERNATIVE_CALL
> config ARCH_MAP_DOMAIN_PAGE
> bool
>
> +config GENERIC_BUG_FRAME
> + bool
>>>
>>> With Arm now also going to use the generic logic, do we actually
>>> need
>>> this
>>> control anymore (provided things have been proven to work on Arm
>>> for
>>> the
>>> compiler range we support there)? It looks like because of the way
>>> the
>>> series is partitioned it may be necessary transiently, but it
>>> should
>>> be
>>> possible to drop it again in a new 5th patch.
> We still need it because RISC-V doesn't ready yet to use do_bug_frame()
> from xen/common/bug.c as it requires find_text_region(),
> virtual_region() and panic().
> The mentioned functionality isn't ready now for RISC-V so RISC-V will
> use only generic things from only for some time.
Oh, right. So let's leave it for the time being, but consider dropping
it once RISC-V is more complete.
> --- /dev/null
> +++ b/xen/include/xen/bug.h
> @@ -0,0 +1,158 @@
> +#ifndef __XEN_BUG_H__
> +#define __XEN_BUG_H__
> +
> +#define BUGFRAME_run_fn 0
> +#define BUGFRAME_warn 1
> +#define BUGFRAME_bug 2
> +#define BUGFRAME_assert 3
> +
> +#define BUGFRAME_NR 4
> +
> +#ifndef BUG_FRAME_STRUCT
>>>
>>> This check won't help when it comes ahead of ...
>>>
> +#define BUG_DISP_WIDTH 24
> +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> +#endif
> +
> +#include
>>>
>>> ... this. But is the #ifdef actually necessary? Or can the #define-
>>> s
>>> be moved ...
>>>
> +#ifndef BUG_DEBUGGER_TRAP_FATAL
> +#define BUG_DEBUGGER_TRAP_FATAL(regs) 0
> +#endif
> +
> +#ifndef __ASSEMBLY__
> +
> +#include
> +
> +#ifndef BUG_FRAME_STRUCT
>>>
>>> ... here? (I guess having them defined early, but unconditionally
>>> is
>>> better. If an arch wants to override them, they can #undef and then
>>> #define.)
> We can't move it to #indef __ASSEMBLY__ because in this case x86
> compilation will fail as in x86's BUG_DISP_WIDTH,
> BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH are used in case when the header
> is included in assembly code.
Oh, of course.
> I agree that there is no any sense to have the defines under "#ifndef
> BUG_FRAME_STUCT" before but it is necessary to define them
> before . The defines were put under "#ifndef
> BUG_FRAME_STUCT" because it seems to me that logically the defines
> should go only with definition of 'struct bug_frame'.
>
> So it looks like the only one way we have. It is define them
> unconditionally before and #undef if it will be necessary
> for specific architecture.
> As an option we can move the defines to #ifndef BUG_FRAME_STRUCT under
> #ifndef __ASSEMBLY__ and then define them explicitly in x86's
> for case when the header is included some where in assembly
> code. But this option looks really weird.
Indeed.
> +#define BUG_FRAME(type, line, ptr, second_frame, msg) do
> { \
> + BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
> BUG_LINE_HI_WIDTH)); \
> + BUILD_BUG_ON((type) >=
> BUGFRAME_NR); \
> + asm volatile (
> _ASM_BUGFRAME_TEXT(second_frame) \
> + :: _ASM_BUGFRAME_INFO(type, line, ptr, msg)
> ); \
> +} while (0)
>>>
>>> Isn't this tied to BUG_FRAME_STRUCT being defined (or not)? At
>>> least
>>> the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to use
>>> the generic struct: With how you have things right now
>>> BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check would also
>>> be at risk of causing false positives. Perhaps it's appropriate to
>>> have a separate #ifdef (incl the distinct identifier used), but
>>> that
>>> first BUILD_BUG_ON() needs abstracting out.
> Missed that. Thanks.
> I'll introduce that a separate #ifdef before BUG_FRAME:
>
> #ifndef BUILD_BUG_ON_LINE_WIDTH
> #define BUILD_BUG_ON_LINE_WIDTH \
> BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH))
> #endif
But then please make this a function-like macro.
I'm also not convinced of the #ifndef - an arch using an entirely
different layout should have no need for this check. So I'd rather
attach the #define to what's inside #ifndef BUG_FRAME_STRUCT and
have an #else there to supply a #define to alternatively.
Jan
Re: [PATCH v5 4/4] xen/x86: switch x86 to use generic implemetation of bug.h
On Mon, 2023-03-06 at 11:36 +0100, Jan Beulich wrote:
> On 03.03.2023 11:38, Oleksii Kurochko wrote:
> > The following changes were made:
> > * Make GENERIC_BUG_FRAME mandatory for X86
> > * Update asm/bug.h using generic implementation in
> > * Change prototype of debugger_trap_fatal() in asm/debugger.h
> > to align it with generic prototype in .
>
> Is this part of the description stale? The patch ...
It is stale. Updated now.
>
> > ---
> > xen/arch/x86/Kconfig | 1 +
> > xen/arch/x86/include/asm/bug.h | 73 ++
> > xen/arch/x86/traps.c | 81 +++---
> >
> > 3 files changed, 11 insertions(+), 144 deletions(-)
>
> ... doesn't touch asm/debugger.h at all.
>
> > --- a/xen/arch/x86/include/asm/bug.h
> > +++ b/xen/arch/x86/include/asm/bug.h
> > @@ -1,79 +1,12 @@
> > #ifndef __X86_BUG_H__
> > #define __X86_BUG_H__
> >
> > -#define BUG_DISP_WIDTH 24
> > -#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> > -#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> > +#define BUG_DEBUGGER_TRAP_FATAL(regs)
> > debugger_trap_fatal(X86_EXC_GP,regs)
>
> Along the lines of a comment on an earlier patch, please move this
> ...
>
> > #ifndef __ASSEMBLY__
>
> ... into the thus guarded section.
But it was defined there before the patch series and these definies are
used in "#else /* !__ASSEMBLY__ */"
>
> > @@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const struct
> > vcpu *v, uint32_t leaf,
> >
> > void do_invalid_op(struct cpu_user_regs *regs)
> > {
> > - const struct bug_frame *bug = NULL;
> > u8 bug_insn[2];
> > - const char *prefix = "", *filename, *predicate, *eip = (char
> > *)regs->rip;
> > - unsigned long fixup;
> > - int id = -1, lineno;
> > - const struct virtual_region *region;
> > + const char *eip = (char *)regs->rip;
>
> I realize "char" was used before, but now that this is all on its
> own,
> can this at least become "unsigned char", but yet better "void"?
If we change it to "void" shouldn't it require additional casts here (
which is not nice ):
eip += sizeof(bug_insn);
Probably 'unsgined char' will be better.
>
> > @@ -1185,83 +1183,18 @@ void do_invalid_op(struct cpu_user_regs
> > *regs)
> > memcmp(bug_insn, "\xf\xb", sizeof(bug_insn)) )
> > goto die;
> >
> > - region = find_text_region(regs->rip);
> > - if ( region )
> > - {
> > - for ( id = 0; id < BUGFRAME_NR; id++ )
> > - {
> > - const struct bug_frame *b;
> > - unsigned int i;
> > -
> > - for ( i = 0, b = region->frame[id].bugs;
> > - i < region->frame[id].n_bugs; b++, i++ )
> > - {
> > - if ( bug_loc(b) == eip )
> > - {
> > - bug = b;
> > - goto found;
> > - }
> > - }
> > - }
> > - }
> > -
> > - found:
> > - if ( !bug )
> > + id = do_bug_frame(regs, regs->rip);
> > + if ( id < 0 )
> > goto die;
> > - eip += sizeof(bug_insn);
> > - if ( id == BUGFRAME_run_fn )
> > - {
> > - void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
> > -
> > - fn(regs);
> > - fixup_exception_return(regs, (unsigned long)eip);
> > - return;
> > - }
> >
> > - /* WARN, BUG or ASSERT: decode the filename pointer and line
> > number. */
> > - filename = bug_ptr(bug);
> > - if ( !is_kernel(filename) && !is_patch(filename) )
> > - goto die;
> > - fixup = strlen(filename);
> > - if ( fixup > 50 )
> > - {
> > - filename += fixup - 47;
> > - prefix = "...";
> > - }
> > - lineno = bug_line(bug);
> > + eip += sizeof(bug_insn);
> >
> > switch ( id )
> > {
> > + case BUGFRAME_run_fn:
> > case BUGFRAME_warn:
> > - printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
> > - show_execution_state(regs);
> > fixup_exception_return(regs, (unsigned long)eip);
> > return;
> > -
> > - case BUGFRAME_bug:
> > - printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> > -
> > - if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
> > - return;
>
> This and ...
>
> > - show_execution_state(regs);
> > - panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> > -
> > - case BUGFRAME_assert:
> > - /* ASSERT: decode the predicate string pointer. */
> > - predicate = bug_msg(bug);
> > - if ( !is_kernel(predicate) && !is_patch(predicate) )
> > - predicate = "";
> > -
> > - printk("Assertion '%s' failed at %s%s:%d\n",
> > - predicate, prefix, filename, lineno);
> > -
> > - if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
> > - return;
>
> ... this return look to have no proper representation in the new
> logic; both cases fall through to ...
>
> > - show_execution_state(regs);
> > -
Re: [PATCH 1/2] xen: move CONFIG_DEBUG_INFO out of EXPERT section
On 07.03.2023 13:04, Juergen Gross wrote: > On 07.03.23 12:42, Jan Beulich wrote: >> On 07.03.2023 12:33, Juergen Gross wrote: >>> On 07.03.23 11:31, Jan Beulich wrote: On 07.03.2023 07:32, Juergen Gross wrote: > Using a rather oldish gcc (7.5) it was verified that code generation > doesn't really differ between CONFIG_DEBUG_INFO on or off without > CONFIG_DEBUG being set (only observed differences were slightly > different symbol addresses, verified via "objdump -d"). The old gcc > version selection was based on the assumption, that newer gcc won't > regress in this regard. This is good to know, but I'm still curious about the mentioned differences in symbol addresses: If code generation didn't change, what caused addresses to differ? Is that merely because individual functions or objects are emitted in different order by the compiler? (If so I'd be inclined to infer that comparing generated code must have been quite a bit of effort, as first of all you would have had to undo that re-ordering.) >>> >>> I did a simple diff of the two disassembly outputs and got only small >>> differences for %rip relative addresses (the differences were in the >>> range of +/- 32 bytes). >> >> That's still odd and hence imo wants understanding. Do you still have >> both binaries around? > > I have just regenerated the one with debug-info. It is a 4.17 build. > > I just found the at least one reason: xen_config_data has a different size > (obviously!) and this finally leads to an offset of 32 bytes for symbols > at higher addresses (with some items only differing by 8 bytes due to > alignment). Thanks for checking - this kind of a delta is of course to be expected. Jan
Re: [RFC PATCH v1 03/25] hw/xen: Implement XenStore watches
On Tue, 2023-03-07 at 11:29 +, Paul Durrant wrote:
>
> I think you could stash a tail pointer here...
>
> > + }
> > +
> > + if (dom_id && s->nr_domu_watches >= XS_MAX_WATCHES) {
> > + return E2BIG;
> > + }
> > +
> > + w = g_new0(XsWatch, 1);
> > + w->token = g_strdup(token);
> > + w->cb = fn;
> > + w->cb_opaque = opaque;
> > + w->dom_id = dom_id;
> > + w->rel_prefix = strlen(abspath) - strlen(path);
> > +
> > + l = g_hash_table_lookup(s->watches, abspath);
>
> ... to avoid the duplicate hash lookup here.
Good point. The EEXIST check was added later as I was reviewing and
comparing with the real xenstored, so I didn't spot that. Thanks.
--- a/hw/i386/kvm/xenstore_impl.c
+++ b/hw/i386/kvm/xenstore_impl.c
@@ -673,7 +673,7 @@ int xs_impl_watch(XenstoreImplState *s, unsigned int
dom_id, const char *path,
}
/* Check for duplicates */
-w = g_hash_table_lookup(s->watches, abspath);
+l = w = g_hash_table_lookup(s->watches, abspath);
while (w) {
if (!g_strcmp0(token, w->token) && opaque == w->cb_opaque &&
fn == w->cb && dom_id == w->dom_id) {
@@ -693,7 +693,7 @@ int xs_impl_watch(XenstoreImplState *s, unsigned int
dom_id, const char *path,
w->dom_id = dom_id;
w->rel_prefix = strlen(abspath) - strlen(path);
-l = g_hash_table_lookup(s->watches, abspath);
+/* l was looked up above when checking for duplicates */
if (l) {
w->next = l->next;
l->next = w;
smime.p7s
Description: S/MIME cryptographic signature
Re: [PATCH 1/2] xen: move CONFIG_DEBUG_INFO out of EXPERT section
On 07.03.23 12:42, Jan Beulich wrote: On 07.03.2023 12:33, Juergen Gross wrote: On 07.03.23 11:31, Jan Beulich wrote: On 07.03.2023 07:32, Juergen Gross wrote: Using a rather oldish gcc (7.5) it was verified that code generation doesn't really differ between CONFIG_DEBUG_INFO on or off without CONFIG_DEBUG being set (only observed differences were slightly different symbol addresses, verified via "objdump -d"). The old gcc version selection was based on the assumption, that newer gcc won't regress in this regard. This is good to know, but I'm still curious about the mentioned differences in symbol addresses: If code generation didn't change, what caused addresses to differ? Is that merely because individual functions or objects are emitted in different order by the compiler? (If so I'd be inclined to infer that comparing generated code must have been quite a bit of effort, as first of all you would have had to undo that re-ordering.) I did a simple diff of the two disassembly outputs and got only small differences for %rip relative addresses (the differences were in the range of +/- 32 bytes). That's still odd and hence imo wants understanding. Do you still have both binaries around? I have just regenerated the one with debug-info. It is a 4.17 build. I just found the at least one reason: xen_config_data has a different size (obviously!) and this finally leads to an offset of 32 bytes for symbols at higher addresses (with some items only differing by 8 bytes due to alignment). Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
[qemu-mainline test] 179449: tolerable trouble: fail/pass/starved - PUSHED
flight 179449 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/179449/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stop fail blocked in 176342 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 176342 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 176342 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 176342 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 176342 test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 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 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 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-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 1 build-check(1) starved n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) starved n/a test-armhf-armhf-libvirt-raw 1 build-check(1) starved n/a test-armhf-armhf-xl 1 build-check(1) starved n/a test-armhf-armhf-xl-credit1 1 build-check(1) starved n/a test-armhf-armhf-xl-multivcpu 1 build-check(1) starved n/a test-armhf-armhf-xl-rtds 1 build-check(1) starved n/a test-armhf-armhf-xl-vhd 1 build-check(1) starved n/a build-armhf-libvirt 1 build-check(1) starved n/a test-armhf-armhf-xl-cubietruck 1 build-check(1) starved n/a test-armhf-armhf-xl-credit2 1 build-check(1) starved n/a build-armhf 2 hosts-allocate starved n/a version targeted for testing: qemuu817fd33836e73812df2f1907612b57750fcb9491 baseline version: qemuu387b2b52558bbb44ad74634415e1ab488d3c62a7 Last test of basis 176342 2023-02-03 03:30:18 Z 32 days Failing since176352 2023-02-03 17:40:20 Z 31 days7 attempts Testing same since 179449 2023-03-07 01:40:20 Z0 days1 attempts People who touched revisions under test: "John Berberian, Jr" Akihiko Odaki Alberto Faria Alex Bennée Alex Williamson Alexander Bulekov Alexander Graf Alistair Francis Andrey Zhadchenko Ankur Arora Antoine Damhet Anton Johansson Anton Kochkov Anup Patel Avihai Horon BALATON Zoltan Bastian Koppelmann Bernhard Beschow Bibo Mao Bin Meng Brad Smith Carlos López Charles Frey Christian Schoenebeck Christian Svensson Christoph Müllner Claudio Fontana Claudio Imbrenda Corey Minyard Cornelia Huck Cédric Le Goater Cédric Le Goater Daniel Henrique Barboza Daniel Henrique Barboza Daniel P. Berrangé David Hildenbrand David Woodhouse David Woodhouse David Wooodhouse Deepak Gupta Dinah Baum Dongli Zhang Doug Rabson Dov Murik Dr. David Alan Gilbert Drew DeVault Eitan Eliahu Emanuele Giuseppe Esposito Emilio Cota Eric Auger Eugenio Pérez Evgeny Iakovlev Fabiano Rosas Fan Ni F
Re: [PATCH 1/2] xen: move CONFIG_DEBUG_INFO out of EXPERT section
On 07.03.2023 12:33, Juergen Gross wrote: > On 07.03.23 11:31, Jan Beulich wrote: >> On 07.03.2023 07:32, Juergen Gross wrote: >>> Using a rather oldish gcc (7.5) it was verified that code generation >>> doesn't really differ between CONFIG_DEBUG_INFO on or off without >>> CONFIG_DEBUG being set (only observed differences were slightly >>> different symbol addresses, verified via "objdump -d"). The old gcc >>> version selection was based on the assumption, that newer gcc won't >>> regress in this regard. >> >> This is good to know, but I'm still curious about the mentioned >> differences in symbol addresses: If code generation didn't change, what >> caused addresses to differ? Is that merely because individual functions >> or objects are emitted in different order by the compiler? (If so I'd >> be inclined to infer that comparing generated code must have been >> quite a bit of effort, as first of all you would have had to undo that >> re-ordering.) > > I did a simple diff of the two disassembly outputs and got only small > differences for %rip relative addresses (the differences were in the > range of +/- 32 bytes). That's still odd and hence imo wants understanding. Do you still have both binaries around? Jan
Re: [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
On Mon, 2023-03-06 at 11:41 +0100, Jan Beulich wrote:
> On 03.03.2023 11:38, Oleksii Kurochko wrote:
> > +int do_bug_frame(struct cpu_user_regs *regs, unsigned long pc)
> > +{
> > + const struct bug_frame *bug = NULL;
> > + const struct virtual_region *region;
> > + const char *prefix = "", *filename, *predicate;
> > + unsigned long fixup;
> > + unsigned int id = BUGFRAME_NR, lineno;
> > +
> > + region = find_text_region(pc);
> > + if ( !region )
> > + return -EINVAL;
> > +
> > + for ( id = 0; id < BUGFRAME_NR; id++ )
> > + {
> > + const struct bug_frame *b;
> > + size_t i;
> > +
> > + for ( i = 0, b = region->frame[id].bugs;
> > + i < region->frame[id].n_bugs; b++, i++ )
> > + {
> > + if ( bug_loc(b) == pc )
> > + {
> > + bug = b;
> > + goto found;
> > + }
> > + }
> > + }
> > +
> > + found:
> > + if ( !bug )
> > + return -EINVAL;
> > +
> > + if ( id == BUGFRAME_run_fn )
> > + {
> > + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> > +
> > + fn(regs);
> > +
> > + return id;
> > + }
> > +
> > + /* WARN, BUG or ASSERT: decode the filename pointer and line
> > number. */
> > + filename = bug_ptr(bug);
> > + if ( !is_kernel(filename) && !is_patch(filename) )
> > + return -EINVAL;
> > + fixup = strlen(filename);
> > + if ( fixup > 50 )
> > + {
> > + filename += fixup - 47;
> > + prefix = "...";
> > + }
> > + lineno = bug_line(bug);
> > +
> > + switch ( id )
> > + {
> > + case BUGFRAME_warn:
> > + printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
> > + show_execution_state(regs);
> > +
> > + return id;
> > +
> > + case BUGFRAME_bug:
> > + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> > +
> > + if ( BUG_DEBUGGER_TRAP_FATAL(regs) )
> > + return id;
> > +
> > + show_execution_state(regs);
> > + panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> > +
> > + case BUGFRAME_assert:
> > + /* ASSERT: decode the predicate string pointer. */
> > + predicate = bug_msg(bug);
> > + if ( !is_kernel(predicate) && !is_patch(predicate) )
> > + predicate = "";
> > +
> > + printk("Assertion '%s' failed at %s%s:%d\n",
> > + predicate, prefix, filename, lineno);
> > +
> > + if ( BUG_DEBUGGER_TRAP_FATAL(regs) )
> > + return id;
> > +
> > + show_execution_state(regs);
> > + panic("Assertion '%s' failed at %s%s:%d\n",
> > + predicate, prefix, filename, lineno);
> > + }
> > +
> > + return id;
> > +}
>
> This final "return" looks like almost dead code (it isn't when an
> unrecognized id is found). May I suggest to switch all "return id"
> inside the switch() block to just "break", to make this final
> "return" be (a) the central one and (b) more obviously a used path?
>
Sure It will be much better.
~ Oleksii
Re: [PATCH 1/2] xen: move CONFIG_DEBUG_INFO out of EXPERT section
On 07.03.23 11:31, Jan Beulich wrote: On 07.03.2023 07:32, Juergen Gross wrote: In order to support hypervisor analysis of crash dumps, xen-syms needs to contain debug_info. It should be allowed to configure the hypervisor to be built with CONFIG_DEBUG_INFO in non-debug builds without having to enable EXPERT. In how far does this apply to xen.efi as well? (You can't really use xen-syms for crash debugging when the crash occurred with xen.efi in use.) TBH I don't know what is needed for analysis of crash dumps with the xen.efi binary. Using a rather oldish gcc (7.5) it was verified that code generation doesn't really differ between CONFIG_DEBUG_INFO on or off without CONFIG_DEBUG being set (only observed differences were slightly different symbol addresses, verified via "objdump -d"). The old gcc version selection was based on the assumption, that newer gcc won't regress in this regard. This is good to know, but I'm still curious about the mentioned differences in symbol addresses: If code generation didn't change, what caused addresses to differ? Is that merely because individual functions or objects are emitted in different order by the compiler? (If so I'd be inclined to infer that comparing generated code must have been quite a bit of effort, as first of all you would have had to undo that re-ordering.) I did a simple diff of the two disassembly outputs and got only small differences for %rip relative addresses (the differences were in the range of +/- 32 bytes). The other thing to at least mention here is that with new enough binutils, when Dwarf debug info can be enabled for keeping in xen.efi, linking time of xen.efi increases quite a bit with DEBUG_INFO=y (which is a result of linking ELF objects into a non-ELF binary, when at least GNU ld optimizes only the ELF -> ELF case when processing the [massive amount of] relocations). Okay, I can add this. So move CONFIG_DEBUG_INFO out of the section guarded by EXPERT. Isn't the prior DEBUG dependency as relevant? Not for the case "non-debug builds" the patch is addressing. --- a/xen/Kconfig.debug +++ b/xen/Kconfig.debug @@ -11,6 +11,13 @@ config DEBUG You probably want to say 'N' here. +config DEBUG_INFO + bool "Compile Xen with debug info" + default DEBUG + ---help--- Nit: Even if just code movement, please use "help" in the moved instance. Okay. + If you say Y here the resulting Xen will include debugging info + resulting in a larger binary image. + if DEBUG || EXPERT The new placement isn't very helpful when considering some of the ways kconfig data is presented. At least for the non-graphical presentation it used to be the case that hierarchies were presented properly only when dependencies immediately followed their dependents (i.e. here: DEBUG is a dependent of everything inside the "if" above). Therefore I think rather than moving the block up you may better move it down past the "endif". Fine with me. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
On Mon, 2023-03-06 at 11:17 +0100, Jan Beulich wrote:
> > On 03.03.2023 11:38, Oleksii Kurochko wrote:
> > > > --- a/xen/common/Kconfig
> > > > +++ b/xen/common/Kconfig
> > > > @@ -28,6 +28,9 @@ config ALTERNATIVE_CALL
> > > > config ARCH_MAP_DOMAIN_PAGE
> > > > bool
> > > >
> > > > +config GENERIC_BUG_FRAME
> > > > + bool
> >
> > With Arm now also going to use the generic logic, do we actually
> > need
> > this
> > control anymore (provided things have been proven to work on Arm
> > for
> > the
> > compiler range we support there)? It looks like because of the way
> > the
> > series is partitioned it may be necessary transiently, but it
> > should
> > be
> > possible to drop it again in a new 5th patch.
We still need it because RISC-V doesn't ready yet to use do_bug_frame()
from xen/common/bug.c as it requires find_text_region(),
virtual_region() and panic().
The mentioned functionality isn't ready now for RISC-V so RISC-V will
use only generic things from only for some time.
Another one reason I would like to leave the config it is probably the
same situation will be for future architectures.
This is not something mandatory if the config isn't really necessary
than it should be removed after RISC-V will be ready to migrate full on
generic implementation.
> >
> > > > --- /dev/null
> > > > +++ b/xen/common/bug.c
> > > > @@ -0,0 +1,103 @@
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +
> > > > +#include
> > > > +
> > > > +/*
> > > > + * Returns a negative value in case of an error otherwise the
> > > > bug
> > > > type.
> > > > + */
> >
> > Nit: Style (this is a single line comment). But see also below
> > related
> > comment on the function's declaration.
Thanks. I'll fix that.
> >
> > > > +int do_bug_frame(struct cpu_user_regs *regs, unsigned long pc)
> > > > +{
> > > > + const struct bug_frame *bug = NULL;
> > > > + const struct virtual_region *region;
> > > > + const char *prefix = "", *filename, *predicate;
> > > > + unsigned long fixup;
> > > > + unsigned int id = BUGFRAME_NR, lineno;
> > > > +
> > > > + region = find_text_region(pc);
> > > > + if ( !region )
> > > > + return -EINVAL;
> > > > +
> > > > + for ( id = 0; id < BUGFRAME_NR; id++ )
> > > > + {
> > > > + const struct bug_frame *b;
> > > > + size_t i;
> > > > +
> > > > + for ( i = 0, b = region->frame[id].bugs;
> > > > + i < region->frame[id].n_bugs; b++, i++ )
> >
> > Nit: Indentation (the "i" on the 2nd line wants to align with that
> > on the 1st one).
Thanks. I'll update the indentation.
> >
> > > > + {
> > > > + if ( bug_loc(b) == pc )
> > > > + {
> > > > + bug = b;
> > > > + goto found;
> > > > + }
> > > > + }
> > > > + }
> > > > +
> > > > + found:
> > > > + if ( !bug )
> > > > + return -EINVAL;
> >
> > While I'm generally unhappy with many uses of -EINVAL (it's used to
> > indicate way too many different kinds of errors), can we at least
> > consider using -ENOENT here instead? (I'm sorry, I should have
> > asked
> > for this earlier on.)
Sure. Done.
> >
> > > > --- /dev/null
> > > > +++ b/xen/include/xen/bug.h
> > > > @@ -0,0 +1,158 @@
> > > > +#ifndef __XEN_BUG_H__
> > > > +#define __XEN_BUG_H__
> > > > +
> > > > +#define BUGFRAME_run_fn 0
> > > > +#define BUGFRAME_warn 1
> > > > +#define BUGFRAME_bug 2
> > > > +#define BUGFRAME_assert 3
> > > > +
> > > > +#define BUGFRAME_NR 4
> > > > +
> > > > +#ifndef BUG_FRAME_STRUCT
> >
> > This check won't help when it comes ahead of ...
> >
> > > > +#define BUG_DISP_WIDTH 24
> > > > +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> > > > +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> > > > +#endif
> > > > +
> > > > +#include
> >
> > ... this. But is the #ifdef actually necessary? Or can the #define-
> > s
> > be moved ...
> >
> > > > +#ifndef BUG_DEBUGGER_TRAP_FATAL
> > > > +#define BUG_DEBUGGER_TRAP_FATAL(regs) 0
> > > > +#endif
> > > > +
> > > > +#ifndef __ASSEMBLY__
> > > > +
> > > > +#include
> > > > +
> > > > +#ifndef BUG_FRAME_STRUCT
> >
> > ... here? (I guess having them defined early, but unconditionally
> > is
> > better. If an arch wants to override them, they can #undef and then
> > #define.)
We can't move it to #indef __ASSEMBLY__ because in this case x86
compilation will fail as in x86's BUG_DISP_WIDTH,
BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH are used in case when the header
is included in assembly code.
I agree that there is no any sense to have the defines under "#ifndef
BUG_FRAME_STUCT" before but it is necessary to define them
before . The defines were put under "#ifndef
BUG_FRAME_STUCT" because it seems to me that logically the defines
should go only with definition of 'struct bug_frame'.
So it looks like the only one way we have. It is define them
uncond
[ovmf test] 179487: all pass - PUSHED
flight 179487 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/179487/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf c7c25997595aa34ce0a7a21ca2e1fc5b0f9b38a6 baseline version: ovmf 46f51898ff716e53921b93e8d78af0fc7d06a2f9 Last test of basis 179476 2023-03-07 07:10:44 Z0 days Testing same since 179487 2023-03-07 09:43:43 Z0 days1 attempts People who touched revisions under test: Gerd Hoffmann Xie, Yuanhao Yuanhao Xie 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 46f51898ff..c7c2599759 c7c25997595aa34ce0a7a21ca2e1fc5b0f9b38a6 -> xen-tested-master
Re: [RFC PATCH v1 03/25] hw/xen: Implement XenStore watches
On 02/03/2023 15:34, David Woodhouse wrote:
From: David Woodhouse
Starts out fairly simple: a hash table of watches based on the path.
Except there can be multiple watches on the same path, so the watch ends
up being a simple linked list, and the head of that list is in the hash
table. Which makes removal a bit of a PITA but it's not so bad; we just
special-case "I had to remove the head of the list and now I have to
replace it in / remove it from the hash table". And if we don't remove
the head, it's a simple linked-list operation.
We do need to fire watches on *deleted* nodes, so instead of just a simple
xs_node_unref() on the topmost victim, we need to recurse down and fire
watches on them all.
Signed-off-by: David Woodhouse
---
hw/i386/kvm/xenstore_impl.c | 253 +---
tests/unit/test-xs-node.c | 85
2 files changed, 323 insertions(+), 15 deletions(-)
Reviewed-by: Paul Durrant
... with one suggestion...
[snip]
+/* Check for duplicates */
+w = g_hash_table_lookup(s->watches, abspath);
+while (w) {
+if (!g_strcmp0(token, w->token) && opaque == w->cb_opaque &&
+fn == w->cb && dom_id == w->dom_id) {
+return EEXIST;
+}
+w = w->next;
I think you could stash a tail pointer here...
+}
+
+if (dom_id && s->nr_domu_watches >= XS_MAX_WATCHES) {
+return E2BIG;
+}
+
+w = g_new0(XsWatch, 1);
+w->token = g_strdup(token);
+w->cb = fn;
+w->cb_opaque = opaque;
+w->dom_id = dom_id;
+w->rel_prefix = strlen(abspath) - strlen(path);
+
+l = g_hash_table_lookup(s->watches, abspath);
... to avoid the duplicate hash lookup here.
+if (l) {
+w->next = l->next;
+l->next = w;
+} else {
+g_hash_table_insert(s->watches, g_strdup(abspath), w);
+}
+if (dom_id) {
+s->nr_domu_watches++;
+}
+
+/* A new watch should fire immediately */
+fn(opaque, path, token);
+
+return 0;
+}
Re: [RFC PATCH v1 02/25] hw/xen: Add basic XenStore tree walk and write/read/directory support
On 02/03/2023 15:34, David Woodhouse wrote: From: David Woodhouse This is a fairly simple implementation of a copy-on-write tree. The node walk function starts off at the root, with 'inplace == true'. If it ever encounters a node with a refcount greater than one (including the root node), then that node is shared with other trees, and cannot be modified in place, so the inplace flag is cleared and we copy on write from there on down. Xenstore write has 'mkdir -p' semantics and will create the intermediate nodes if they don't already exist, so in that case we flip the inplace flag back to true as as populated the newly-created nodes. Something has gone wrong with the comment there... 'as we populate' perhaps? We put a copy of the absolute path into the buffer in the struct walk_op, with *two* NUL terminators at the end. As xs_node_walk() goes down the tree, it replaces the next '/' separator with a NUL so that it can use the 'child name' in place. The next recursion down then puts the '/' back and repeats the exercise for the next path element... if it doesn't hit that *second* NUL termination which indicates the true end of the path. Signed-off-by: David Woodhouse --- hw/i386/kvm/xenstore_impl.c | 527 +++- tests/unit/meson.build | 1 + tests/unit/test-xs-node.c | 197 ++ 3 files changed, 718 insertions(+), 7 deletions(-) create mode 100644 tests/unit/test-xs-node.c With the comment fixed... Reviewed-by: Paul Durrant
Re: [RFC PATCH v1 01/25] hw/xen: Add xenstore wire implementation and implementation stubs
On 02/03/2023 15:34, David Woodhouse wrote: From: David Woodhouse This implements the basic wire protocol for the XenStore commands, punting all the actual implementation to xs_impl_* functions which all just return errors for now. Signed-off-by: David Woodhouse --- hw/i386/kvm/meson.build | 1 + hw/i386/kvm/trace-events| 15 + hw/i386/kvm/xen_xenstore.c | 871 +++- hw/i386/kvm/xenstore_impl.c | 117 + hw/i386/kvm/xenstore_impl.h | 58 +++ 5 files changed, 1054 insertions(+), 8 deletions(-) create mode 100644 hw/i386/kvm/xenstore_impl.c create mode 100644 hw/i386/kvm/xenstore_impl.h Reviewed-by: Paul Durrant
Re: [PATCH 2/2] xen: update CONFIG_DEBUG_INFO help text
On 07.03.2023 07:32, Juergen Gross wrote: > --- a/xen/Kconfig.debug > +++ b/xen/Kconfig.debug > @@ -15,8 +15,11 @@ config DEBUG_INFO > bool "Compile Xen with debug info" > default DEBUG > ---help--- > - If you say Y here the resulting Xen will include debugging info > - resulting in a larger binary image. > + Say Y here if you want to build Xen with debug information. This > + information is needed e.g. for doing crash dump analysis of the > + hypervisor via the "crash" tool. > + Saying Y will increase the size of xen-syms and the built EFI > + binary. Largely fine with me, just one question: Why do you mention xen-syms by name, but then verbally describe xen.efi? And since, unlike for xen-syms, this affects the installed binary actually used for booting (which may be placed on a space constrained partition), it may be prudent to mention INSTALL_EFI_STRIP here (as a way to reduce the binary size of what ends up on the EFI partition, even if that wouldn't affect the "normal" way of putting the binary on the EFI partition - people would still need to take care of that in their distros). I guess this size aspect wrt the EFI partition is actually something that should also be mentioned in patch 1, because this can be an argument against exposure of the option (precisely because it requires extra activity to prevent the EFI partition running out of space). Jan
Re: [PATCH 1/2] xen: move CONFIG_DEBUG_INFO out of EXPERT section
On 07.03.2023 07:32, Juergen Gross wrote: > In order to support hypervisor analysis of crash dumps, xen-syms needs > to contain debug_info. It should be allowed to configure the hypervisor > to be built with CONFIG_DEBUG_INFO in non-debug builds without having > to enable EXPERT. In how far does this apply to xen.efi as well? (You can't really use xen-syms for crash debugging when the crash occurred with xen.efi in use.) > Using a rather oldish gcc (7.5) it was verified that code generation > doesn't really differ between CONFIG_DEBUG_INFO on or off without > CONFIG_DEBUG being set (only observed differences were slightly > different symbol addresses, verified via "objdump -d"). The old gcc > version selection was based on the assumption, that newer gcc won't > regress in this regard. This is good to know, but I'm still curious about the mentioned differences in symbol addresses: If code generation didn't change, what caused addresses to differ? Is that merely because individual functions or objects are emitted in different order by the compiler? (If so I'd be inclined to infer that comparing generated code must have been quite a bit of effort, as first of all you would have had to undo that re-ordering.) The other thing to at least mention here is that with new enough binutils, when Dwarf debug info can be enabled for keeping in xen.efi, linking time of xen.efi increases quite a bit with DEBUG_INFO=y (which is a result of linking ELF objects into a non-ELF binary, when at least GNU ld optimizes only the ELF -> ELF case when processing the [massive amount of] relocations). > So move CONFIG_DEBUG_INFO out of the section guarded by EXPERT. Isn't the prior DEBUG dependency as relevant? > --- a/xen/Kconfig.debug > +++ b/xen/Kconfig.debug > @@ -11,6 +11,13 @@ config DEBUG > > You probably want to say 'N' here. > > +config DEBUG_INFO > + bool "Compile Xen with debug info" > + default DEBUG > + ---help--- Nit: Even if just code movement, please use "help" in the moved instance. > + If you say Y here the resulting Xen will include debugging info > + resulting in a larger binary image. > + > if DEBUG || EXPERT The new placement isn't very helpful when considering some of the ways kconfig data is presented. At least for the non-graphical presentation it used to be the case that hierarchies were presented properly only when dependencies immediately followed their dependents (i.e. here: DEBUG is a dependent of everything inside the "if" above). Therefore I think rather than moving the block up you may better move it down past the "endif". Jan
Re: [PATCH] tools/tests: remove vhpet tests
On 07.03.23 11:02, Jan Beulich wrote: On 06.03.2023 17:29, Juergen Gross wrote: tools/tests/vhpet tests don't build since ages (at least since 4.10) and they can't be activated from outside of tools/tests/vhpet. This isn't exactly true - see the run-tests-% goal in the top level Makefile. Ah, I missed that one. Remove them as they seem to be irrelevant. Signed-off-by: Juergen Gross --- Andrew seems to remember that Roger wanted to keep those tests, but this information might be stale or based on false assumptions. So this patch should only go in with Roger's Ack. Having tried a little I can see that it's going to be quite a bit of work to get the thing to at least build again. In fact it looks as if it, having been introduced in 4.5, may already not have built successfully anymore by the time 4.5 was released: hpet_init() takes a struct domain *, but is passed &vcpu0 (matching what I can see in 4.4); I've noticed this while trying to figure out from where it got a declaration of the function in the first place. Nevertheless the test once served a purpose, so it may be worth making it work again. So yes, to decide which direction to move we will want Roger's opinion. Yes, I agree. The main reason to write this patch was to come to a decision what to do with the test: discard it or repair it. Whether just building it by default is useful I'm not entirely certain. What I'd consider more useful is if tests like this were _run_ in the course of routine automated testing. In which case it would need to built at least by this test step. I agree this would be a sensible thing to do. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: API/ABIs: Re: [RFC PATCH v2 0/2] Add a new acquire resource to query vcpu statistics
On 06.03.2023 15:23, Matias Ezequiel Vara Larsen wrote: > Regarding your email, I have the following comments: > > - I still do not know how to choose the value of cacheline_size. I understand > this value shall be between the actual cacheline_size and PAGE_SIZE. A value > that could match most architectures could be 256 bytes. This isn't a concern anymore when offset and stride are stored in the header. It was a concern when trying to come up with a layout where these value were to be inferred (or known a priori). > - Xen shall use the "stats_active" field to indicate what it is producing. In > this field, reserved bits shall be 0. This shall allow us to agree on the > layout even when producer and consumer are compiled with different headers. I wonder how well such a bitfield is going to scale. It provides for only 32 (maybe 64) counters. Of course this may seem a lot right now, but you never know how quickly something like this can grow. Plus with ... > - In the vcpu_stats structure, new fields can only ever be appended. ... this rule the only ambiguity that could arise to consumers when no active flags existed would be that they can't tell "event never occurred" from "hypervisor doesn't know about this anymore". Jan
[PATCH v1 2/2] arch/arm: time: Add support for parsing interrupts by names
From: Andrei Cherechesu
Added support for parsing the ARM generic timer interrupts DT
node by the "interrupt-names" property, if it is available.
If not available, the usual parsing based on the expected
IRQ order is performed.
Also added the "hyp-virt" PPI to the timer PPI list, even
though it's currently not in use. If the "hyp-virt" PPI is
not found, the hypervisor won't panic.
Signed-off-by: Andrei Cherechesu
---
xen/arch/arm/include/asm/time.h | 3 ++-
xen/arch/arm/time.c | 26 ++
2 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/include/asm/time.h b/xen/arch/arm/include/asm/time.h
index 4b401c1110..49ad8c1a6d 100644
--- a/xen/arch/arm/include/asm/time.h
+++ b/xen/arch/arm/include/asm/time.h
@@ -82,7 +82,8 @@ enum timer_ppi
TIMER_PHYS_NONSECURE_PPI = 1,
TIMER_VIRT_PPI = 2,
TIMER_HYP_PPI = 3,
-MAX_TIMER_PPI = 4,
+TIMER_HYP_VIRT_PPI = 4,
+MAX_TIMER_PPI = 5,
};
/*
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 433d7be909..794da646d6 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
static unsigned int timer_irq[MAX_TIMER_PPI];
+static const char *timer_irq_names[MAX_TIMER_PPI] = {
+[TIMER_PHYS_SECURE_PPI] = "sec-phys",
+[TIMER_PHYS_NONSECURE_PPI] = "phys",
+[TIMER_VIRT_PPI] = "virt",
+[TIMER_HYP_PPI] = "hyp-phys",
+[TIMER_HYP_VIRT_PPI] = "hyp-virt",
+};
+
unsigned int timer_get_irq(enum timer_ppi ppi)
{
ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
@@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
{
int res;
unsigned int i;
+bool has_names;
+
+has_names = dt_property_read_bool(timer, "interrupt-names");
/* Retrieve all IRQs for the timer */
for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
{
-res = platform_get_irq(timer, i);
-
-if ( res < 0 )
+if ( has_names )
+res = platform_get_irq_byname(timer, timer_irq_names[i]);
+else
+res = platform_get_irq(timer, i);
+
+if ( res > 0 )
+timer_irq[i] = res;
+/* Do not panic if "hyp-virt" PPI is not found, since it's not
+ * currently used.
+ */
+else if ( i != TIMER_HYP_VIRT_PPI )
panic("Timer: Unable to retrieve IRQ %u from the device tree\n",
i);
-timer_irq[i] = res;
}
}
--
2.35.1
[PATCH v1 1/2] arch/arm: irq: Add platform_get_irq_byname() implementation
From: Andrei Cherechesu
Moved implementation for the function which parses the IRQs of a DT
node by the "interrupt-names" property from the SMMU-v3 driver
to the IRQ core code and made it non-static to be used as helper.
Also changed it to receive a "struct dt_device_node*" as parameter,
like its counterpart, platform_get_irq(). Updated its usage inside
the SMMU-v3 driver accordingly.
Signed-off-by: Andrei Cherechesu
---
xen/arch/arm/include/asm/irq.h| 2 ++
xen/arch/arm/irq.c| 14 +++
xen/drivers/passthrough/arm/smmu-v3.c | 35 +--
3 files changed, 22 insertions(+), 29 deletions(-)
diff --git a/xen/arch/arm/include/asm/irq.h b/xen/arch/arm/include/asm/irq.h
index 245f49dcba..af94f41994 100644
--- a/xen/arch/arm/include/asm/irq.h
+++ b/xen/arch/arm/include/asm/irq.h
@@ -89,6 +89,8 @@ int irq_set_type(unsigned int irq, unsigned int type);
int platform_get_irq(const struct dt_device_node *device, int index);
+int platform_get_irq_byname(struct dt_device_node *np, const char *name);
+
void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
/*
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 79718f68e7..ded495792b 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -718,6 +718,20 @@ int platform_get_irq(const struct dt_device_node *device,
int index)
return irq;
}
+int platform_get_irq_byname(struct dt_device_node *np, const char *name)
+{
+ int index;
+
+ if ( unlikely(!name) )
+ return -EINVAL;
+
+ index = dt_property_match_string(np, "interrupt-names", name);
+ if ( index < 0 )
+ return index;
+
+ return platform_get_irq(np, index);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c
b/xen/drivers/passthrough/arm/smmu-v3.c
index d58c5cd0bf..bfdb62b395 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -200,30 +200,6 @@ static inline void dev_iommu_priv_set(struct device *dev,
void *priv)
fwspec->iommu_priv = priv;
}
-static int platform_get_irq_byname_optional(struct device *dev,
- const char *name)
-{
- int index, ret;
- struct dt_device_node *np = dev_to_dt(dev);
-
- if (unlikely(!name))
- return -EINVAL;
-
- index = dt_property_match_string(np, "interrupt-names", name);
- if (index < 0) {
- dev_info(dev, "IRQ %s not found\n", name);
- return index;
- }
-
- ret = platform_get_irq(np, index);
- if (ret < 0) {
- dev_err(dev, "failed to get irq index %d\n", index);
- return -ENODEV;
- }
-
- return ret;
-}
-
/* Start of Linux SMMUv3 code */
static bool disable_bypass = 1;
@@ -2434,6 +2410,7 @@ static int arm_smmu_device_probe(struct platform_device
*pdev)
int irq, ret;
paddr_t ioaddr, iosize;
struct arm_smmu_device *smmu;
+ struct dt_device_node *np = dev_to_dt(pdev);
smmu = xzalloc(struct arm_smmu_device);
if (!smmu)
@@ -2451,7 +2428,7 @@ static int arm_smmu_device_probe(struct platform_device
*pdev)
}
/* Base address */
- ret = dt_device_get_address(dev_to_dt(pdev), 0, &ioaddr, &iosize);
+ ret = dt_device_get_address(np, 0, &ioaddr, &iosize);
if (ret)
goto out_free_smmu;
@@ -2484,19 +2461,19 @@ static int arm_smmu_device_probe(struct platform_device
*pdev)
/* Interrupt lines */
- irq = platform_get_irq_byname_optional(pdev, "combined");
+ irq = platform_get_irq_byname(np, "combined");
if (irq > 0)
smmu->combined_irq = irq;
else {
- irq = platform_get_irq_byname_optional(pdev, "eventq");
+ irq = platform_get_irq_byname(np, "eventq");
if (irq > 0)
smmu->evtq.q.irq = irq;
- irq = platform_get_irq_byname_optional(pdev, "priq");
+ irq = platform_get_irq_byname(np, "priq");
if (irq > 0)
smmu->priq.q.irq = irq;
- irq = platform_get_irq_byname_optional(pdev, "gerror");
+ irq = platform_get_irq_byname(np, "gerror");
if (irq > 0)
smmu->gerr_irq = irq;
}
--
2.35.1
[PATCH v1 0/2] Fix ARM Generic Timer interrupt parsing
From: Andrei Cherechesu This 2-patch series fixes the parsing of the ARM Generic Timer interrupts from the device tree. If the generic timer interrupts order in the DT was different than the expected order in Xen code, these interrupts would no longer be correctly parsed and registered by Xen, and would result in boot failure. This method with using "interrupt-names" for the generic timer interrupts instead of having them hardcoded in the DTB in a specific order is the newer approach already implemented in Linux. Xen did not have the necessary code for this approach, and it has been implemented by the means of this patch series. Functionality should remain the same if "interrupt-names" is not present in the Generic Timer DTB node of the platform, but the interrupts should then still be present in the expected "sec-phys", "phys", "virt", "hyp-phys", "hyp-virt" order. If "interrupt-names" is present, now it is also correctly handled. Andrei Cherechesu (2): arch/arm: irq: Add platform_get_irq_byname() implementation arch/arm: time: Add support for parsing interrupts by names xen/arch/arm/include/asm/irq.h| 2 ++ xen/arch/arm/include/asm/time.h | 3 ++- xen/arch/arm/irq.c| 14 +++ xen/arch/arm/time.c | 26 +--- xen/drivers/passthrough/arm/smmu-v3.c | 35 +-- 5 files changed, 46 insertions(+), 34 deletions(-) -- 2.35.1
Re: [PATCH] tools/tests: remove vhpet tests
On 06.03.2023 17:29, Juergen Gross wrote: > tools/tests/vhpet tests don't build since ages (at least since 4.10) > and they can't be activated from outside of tools/tests/vhpet. This isn't exactly true - see the run-tests-% goal in the top level Makefile. > Remove them as they seem to be irrelevant. > > Signed-off-by: Juergen Gross > --- > Andrew seems to remember that Roger wanted to keep those tests, but > this information might be stale or based on false assumptions. > So this patch should only go in with Roger's Ack. Having tried a little I can see that it's going to be quite a bit of work to get the thing to at least build again. In fact it looks as if it, having been introduced in 4.5, may already not have built successfully anymore by the time 4.5 was released: hpet_init() takes a struct domain *, but is passed &vcpu0 (matching what I can see in 4.4); I've noticed this while trying to figure out from where it got a declaration of the function in the first place. Nevertheless the test once served a purpose, so it may be worth making it work again. So yes, to decide which direction to move we will want Roger's opinion. Whether just building it by default is useful I'm not entirely certain. What I'd consider more useful is if tests like this were _run_ in the course of routine automated testing. Jan
[ovmf test] 179476: all pass - PUSHED
flight 179476 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/179476/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 46f51898ff716e53921b93e8d78af0fc7d06a2f9 baseline version: ovmf a1d595fc9c049874b972a371fe6090738a176f5b Last test of basis 179364 2023-03-06 06:12:20 Z1 days Testing same since 179476 2023-03-07 07:10:44 Z0 days1 attempts People who touched revisions under test: Rebecca Cran 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 a1d595fc9c..46f51898ff 46f51898ff716e53921b93e8d78af0fc7d06a2f9 -> xen-tested-master
Re: [SeaBIOS] Re: [SeaBIOS PATCH] xen: require Xen info structure at 0x1000 to detect Xen
On Thu, 2023-02-02 at 10:10 +0100, Gerd Hoffmann wrote: > > Thanks, Kevin. > > > > I'd like to get the rest of the Xen platform support in to qemu 8.0 > > if > > possible. Which is currently scheduled for March. > > > > Is there likely to be a SeaBIOS release before then which Gerd > > would > > pull into qemu anyway, or should I submit a submodule update to a > > snapshot of today's tree? That would just pull in this commit, and > > the > > one other fix that's in the SeaBIOS tree since 1.16.1? > > Tagging 1.16.2 in time for the qemu 8.0 should not be a problem given > that we have only bugfixes in master. Roughly around soft freeze is > probably a good time for that. That's, erm, at the *end* of today 2023-03-07, and the freeze happens only *after* Paul has reviewed the phase 2 Xen PV back end support, right? https://wiki.qemu.org/Planning/8.0 smime.p7s Description: S/MIME cryptographic signature
