Re: [PATCH 12/15] swiotlb: provide swiotlb_init variants that remap the buffer
On 4/3/22 10:05 PM, Christoph Hellwig wrote:
> To shared more code between swiotlb and xen-swiotlb, offer a
> swiotlb_init_remap interface and add a remap callback to
> swiotlb_init_late that will allow Xen to remap the buffer the
> buffer without duplicating much of the logic.
>
> Signed-off-by: Christoph Hellwig
> ---
> arch/x86/pci/sta2x11-fixup.c | 2 +-
> include/linux/swiotlb.h | 5 -
> kernel/dma/swiotlb.c | 36 +---
> 3 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/pci/sta2x11-fixup.c b/arch/x86/pci/sta2x11-fixup.c
> index c7e6faf59a861..7368afc039987 100644
> --- a/arch/x86/pci/sta2x11-fixup.c
> +++ b/arch/x86/pci/sta2x11-fixup.c
> @@ -57,7 +57,7 @@ static void sta2x11_new_instance(struct pci_dev *pdev)
> int size = STA2X11_SWIOTLB_SIZE;
> /* First instance: register your own swiotlb area */
> dev_info(&pdev->dev, "Using SWIOTLB (size %i)\n", size);
> - if (swiotlb_init_late(size, GFP_DMA))
> + if (swiotlb_init_late(size, GFP_DMA, NULL))
> dev_emerg(&pdev->dev, "init swiotlb failed\n");
> }
> list_add(&instance->list, &sta2x11_instance_list);
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index ee655f2e4d28b..7b50c82f84ce9 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -36,8 +36,11 @@ struct scatterlist;
>
> int swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, unsigned int
> flags);
> unsigned long swiotlb_size_or_default(void);
> +void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
> + int (*remap)(void *tlb, unsigned long nslabs));
> +int swiotlb_init_late(size_t size, gfp_t gfp_mask,
> + int (*remap)(void *tlb, unsigned long nslabs));
> extern int swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs);
> -int swiotlb_init_late(size_t size, gfp_t gfp_mask);
> extern void __init swiotlb_update_mem_attributes(void);
>
> phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t phys,
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 119187afc65ec..d5fe8f5e08300 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -256,9 +256,11 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned
> long nslabs,
> * Statically reserve bounce buffer space and initialize bounce buffer data
> * structures for the software IO TLB used to implement the DMA API.
> */
> -void __init swiotlb_init(bool addressing_limit, unsigned int flags)
> +void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
> + int (*remap)(void *tlb, unsigned long nslabs))
> {
> - size_t bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
> + unsigned long nslabs = default_nslabs;
> + size_t bytes;
> void *tlb;
>
> if (!addressing_limit && !swiotlb_force_bounce)
> @@ -271,12 +273,23 @@ void __init swiotlb_init(bool addressing_limit,
> unsigned int flags)
>* allow to pick a location everywhere for hypervisors with guest
>* memory encryption.
>*/
> +retry:
> + bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
> if (flags & SWIOTLB_ANY)
> tlb = memblock_alloc(bytes, PAGE_SIZE);
> else
> tlb = memblock_alloc_low(bytes, PAGE_SIZE);
> if (!tlb)
> goto fail;
> + if (remap && remap(tlb, nslabs) < 0) {
> + memblock_free(tlb, PAGE_ALIGN(bytes));
> +
> + nslabs = ALIGN(nslabs >> 1, IO_TLB_SEGSIZE);
> + if (nslabs < IO_TLB_MIN_SLABS)
> + panic("%s: Failed to remap %zu bytes\n",
> + __func__, bytes);
> + goto retry;
> + }
> if (swiotlb_init_with_tbl(tlb, default_nslabs, flags))
> goto fail_free_mem;
> return;
> @@ -287,12 +300,18 @@ void __init swiotlb_init(bool addressing_limit,
> unsigned int flags)
> pr_warn("Cannot allocate buffer");
> }
>
> +void __init swiotlb_init(bool addressing_limit, unsigned int flags)
> +{
> + return swiotlb_init_remap(addressing_limit, flags, NULL);
> +}
> +
> /*
> * Systems with larger DMA zones (those that don't support ISA) can
> * initialize the swiotlb later using the slab allocator if needed.
> * This should be just like above, but with some error catching.
> */
> -int swiotlb_init_late(size_t size, gfp_t gfp_mask)
> +int swiotlb_init_late(size_t size, gfp_t gfp_mask,
> + int (*remap)(void *tlb, unsigned long nslabs))
> {
> unsigned long nslabs = ALIGN(size >> IO_TLB_SHIFT, IO_TLB_SEGSIZE);
> unsigned long bytes;
> @@ -303,6 +322,7 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask)
> if (swiotlb_force_disable)
> return 0;
>
> +retry:
> order = get_order(nslabs << IO_TLB_SHIFT);
> nslabs = SLABS_PER_PAGE << order;
> bytes = nslabs <<
Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged
On Fri, Apr 01, 2022 at 06:52:46PM +0100, Julien Grall wrote:
> Hi,
>
> On 31/03/2022 13:36, Roger Pau Monné wrote:
> > On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
> > > There are now instances where internal hypervisor logic needs to make
> > > resource
> > > allocation calls that are protected by XSM checks. The internal
> > > hypervisor logic
> > > is represented a number of system domains which by designed are
> > > represented by
> > > non-privileged struct domain instances. To enable these logic blocks to
> > > function correctly but in a controlled manner, this commit introduces a
> > > pair
> > > of privilege escalation and demotion functions that will make a system
> > > domain
> > > privileged and then remove that privilege.
> > >
> > > Signed-off-by: Daniel P. Smith
> > > ---
> > > xen/include/xsm/xsm.h | 22 ++
> >
> > I'm not sure this needs to be in xsm code, AFAICT it could live in a
> > more generic file.
> >
> > > 1 file changed, 22 insertions(+)
> > >
> > > diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> > > index e22d6160b5..157e57151e 100644
> > > --- a/xen/include/xsm/xsm.h
> > > +++ b/xen/include/xsm/xsm.h
> > > @@ -189,6 +189,28 @@ struct xsm_operations {
> > > #endif
> > > };
> > > +static always_inline int xsm_elevate_priv(struct domain *d)
> >
> > I don't think it needs to be always_inline, using just inline would be
> > fine IMO.
> >
> > Also this needs to be __init.
>
> Hmmm I thought adding __init on function defined in header was
> pointless. In particular, if the compiler decides to inline it.
Indeed, I didn't realize, thanks for pointing this out.
> In any case, I think it would be good to check that the system_state <
> SYS_state_active (could potentially be an ASSERT()) to prevent any misuse.
My preference would be to make those non-inline then I think, we could
place them in common/domain.c maybe? There's no performance reason to
have those helpers as inline.
Another option would be what Jason suggested about creating the idle
domain as privileged and then dropping the privileges before starting
any domains (ie: before setting the system as ACTIVE).
This expands the duration the idle domain context is marked as
privileged, but OTOH we don't need to add a hook to set
is_privileged.
Thanks, Roger.
osstest: boxes pending comission
Hello, We have the following boxes pending commission: italias * Commission flight at: http://logs.test-lab.xenproject.org/osstest/logs/169015/ * Issues: All fine except for the leak-check errors: LEAKED [process 14559 run-parts] process: root 14559 14558 0 19:29 ? 00:00:00 run-parts --report /etc/cron.daily LEAKED [process 14558 /bin/sh] process: root 14558 2252 0 19:29 ? 00:00:00 /bin/sh -c run-parts --report /etc/cron.daily LEAKED [process 14564 /bin/sh] process: root 14564 14559 0 19:29 ? 00:00:00 /bin/sh /etc/cron.daily/apt-compat LEAKED [process 14571 sleep] process: root 14571 14564 0 19:29 ? 00:00:00 sleep 1091 Those processes are from tasks started by cron. Disabling cron in rc.d during the host install doesn't seem to be honored later on, I'm looking into a fix for this. joubertins * Commission flight at: http://logs.test-lab.xenproject.org/osstest/logs/169014/ * Issues: * 2 network timeouts while connecting to guests. The timeout seems to be while executing the command, not while establishing the network connection. * 1 timeout while running update-grub on bare metal (without Xen installed). I've already had the raise the timeout for update-grub to 120s, but that's doesn't seem to always be enough. debinas * Commission flight at: http://logs.test-lab.xenproject.org/osstest/logs/169016/ * No issues: Arm test failing, -amd test failing due to the box being Intel. * Will bless tomorrow (5th of April) during the day unless I hear objections.
Re: [PATCH] platform/x86/dell: add buffer allocation/free functions for SMI calls
Hi, On 3/18/22 23:28, David Laight wrote: > From: Juergen Gross >> Sent: 18 March 2022 16:56 >> >> On 18.03.22 16:22, David Laight wrote: >>> From: Juergen Gross Sent: 18 March 2022 15:10 The dcdbas driver is used to call SMI handlers for both, dcdbas and dell-smbios-smm. Both drivers allocate a buffer for communicating with the SMI handler. The physical buffer address is then passed to the called SMI handler via %ebx. Unfortunately this doesn't work when running in Xen dom0, as the physical address obtained via virt_to_phys() is only a guest physical address, and not a machine physical address as needed by SMI. >>> >>> The physical address from virt_to_phy() is always wrong. >>> That is the physical address the cpu has for the memory. >>> What you want is the address the dma master interface needs to use. >>> That can be different for a physical system - no need for virtualisation. >>> >>> On x86 they do usually match, but anything with a full iommu >>> will need completely different addresses. >> >> Yes, thanks for reminding me of that. >> >> The SMI handler is running on the cpu, right? So using the DMA >> address is wrong in case of an IOMMU. I really need the machine >> physical address. > > That ought to be handled by the 'dev' parameter to dma_alloc_coherent(). > > David I must admit that I'm not too familiar with all the intricate details of the DMA API here. So does this mean that the patch in its original form is good as is and should be merged? An Acked-by or Reviewed-by from someone more familiar with the DMA APIs would be helpful. Regards, Hans
[PATCH v2 1/2] tools/firmware: fix setting of fcf-protection=none
Setting the fcf-protection=none option in EMBEDDED_EXTRA_CFLAGS in the
Makefile doesn't get it propagated to the subdirectories, so instead
set the flag in firmware/Rules.mk, like it's done for other compiler
flags.
Fixes: 3667f7f8f7 ('x86: Introduce support for CET-IBT')
Signed-off-by: Roger Pau Monné
---
Changes since v1:
- Add the option directly to CFLAGS using cc-option-add.
---
tools/firmware/Makefile | 2 --
tools/firmware/Rules.mk | 2 ++
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
index 53ed4f161e..345037b93b 100644
--- a/tools/firmware/Makefile
+++ b/tools/firmware/Makefile
@@ -6,8 +6,6 @@ TARGET := hvmloader/hvmloader
INST_DIR := $(DESTDIR)$(XENFIRMWAREDIR)
DEBG_DIR := $(DESTDIR)$(DEBUG_DIR)$(XENFIRMWAREDIR)
-EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none
-
SUBDIRS-y :=
SUBDIRS-$(CONFIG_OVMF) += ovmf-dir
SUBDIRS-$(CONFIG_SEABIOS) += seabios-dir
diff --git a/tools/firmware/Rules.mk b/tools/firmware/Rules.mk
index 9f78a7dec9..c227fe2524 100644
--- a/tools/firmware/Rules.mk
+++ b/tools/firmware/Rules.mk
@@ -15,6 +15,8 @@ CFLAGS += -Werror
$(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
+$(call cc-option-add,CFLAGS,CC,-fcf-protection=none)
+
# Extra CFLAGS suitable for an embedded type of environment.
CFLAGS += -ffreestanding -msoft-float
--
2.35.1
[PATCH v2 0/2] firmware: build fixes with gcc-11
Hello, The following fixes some firmware build issues with gcc-11. Note that dropping of .note.gnu.property section could likely be done in the linker script in the hvmloader case, but rombios has no linker script and such note is causing a non-working image. Other options could be using objcopy to drop the section, but those seems more complicated than just using the assembler command line option. Thanks, Roger. Roger Pau Monne (2): tools/firmware: fix setting of fcf-protection=none tools/firmware: do not add a .note.gnu.property section Config.mk | 2 +- tools/firmware/Makefile | 2 -- tools/firmware/Rules.mk | 6 ++ 3 files changed, 7 insertions(+), 3 deletions(-) -- 2.35.1
[PATCH v2 2/2] tools/firmware: do not add a .note.gnu.property section
Prevent the assembler from creating a .note.gnu.property section on the output objects, as it's not useful for firmware related binaries, and breaks the resulting rombios image. This requires modifying the cc-option Makefile macro so it can test assembler options (by replacing the usage of the -S flag with -c) and also stripping the -Wa, prefix if present when checking for the test output. Signed-off-by: Roger Pau Monné --- Changes since v1: - Add the option to CFLAGS. --- Config.mk | 2 +- tools/firmware/Rules.mk | 4 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/Config.mk b/Config.mk index f56f7dc334..82832945e5 100644 --- a/Config.mk +++ b/Config.mk @@ -91,7 +91,7 @@ PYTHON_PREFIX_ARG ?= --prefix="$(prefix)" # # Usage: cflags-y += $(call cc-option,$(CC),-march=winchip-c6,-march=i586) cc-option = $(shell if test -z "`echo 'void*p=1;' | \ - $(1) $(2) -S -o /dev/null -x c - 2>&1 | grep -- $(2) -`"; \ + $(1) $(2) -c -o /dev/null -x c - 2>&1 | grep -- $(2:-Wa$(comma)%=%) -`"; \ then echo "$(2)"; else echo "$(3)"; fi ;) # cc-option-add: Add an option to compilation flags, but only if supported. diff --git a/tools/firmware/Rules.mk b/tools/firmware/Rules.mk index c227fe2524..278cca01e4 100644 --- a/tools/firmware/Rules.mk +++ b/tools/firmware/Rules.mk @@ -17,6 +17,10 @@ $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS)) $(call cc-option-add,CFLAGS,CC,-fcf-protection=none) +# Do not add the .note.gnu.property section to any of the firmware objects: it +# breaks the rombios binary and is not useful for firmware anyway. +$(call cc-option-add,CFLAGS,CC,-Wa$$(comma)-mx86-used-note=no) + # Extra CFLAGS suitable for an embedded type of environment. CFLAGS += -ffreestanding -msoft-float -- 2.35.1
Re: [PATCH v2 1/2] tools/firmware: fix setting of fcf-protection=none
On Mon, Apr 04, 2022 at 12:40:43PM +0200, Roger Pau Monne wrote:
> Setting the fcf-protection=none option in EMBEDDED_EXTRA_CFLAGS in the
> Makefile doesn't get it propagated to the subdirectories, so instead
> set the flag in firmware/Rules.mk, like it's done for other compiler
> flags.
>
> Fixes: 3667f7f8f7 ('x86: Introduce support for CET-IBT')
> Signed-off-by: Roger Pau Monné
Reviewed-by: Anthony PERARD
Thanks,
--
Anthony PERARD
Re: [PATCH v2] Grab the EFI System Resource Table and check it
> On 2 Apr 2022, at 00:14, Demi Marie Obenour > wrote: > > The EFI System Resource Table (ESRT) is necessary for fwupd to identify > firmware updates to install. According to the UEFI specification §23.4, > the table shall be stored in memory of type EfiBootServicesData. > Therefore, Xen must avoid reusing that memory for other purposes, so > that Linux can access the ESRT. Additionally, Xen must mark the memory > as reserved, so that Linux knows accessing it is safe. > > See https://lore.kernel.org/xen-devel/20200818184018.GN1679@mail-itl/T/ > for details. > > Signed-off-by: Demi Marie Obenour Hi, I’ve tested the patch on an arm machine booting Xen+Dom0 through EFI, unfortunately I could not test the functionality. Cheers, Luca
Re: [PATCH v2 2/2] tools/firmware: do not add a .note.gnu.property section
On Mon, Apr 04, 2022 at 12:40:44PM +0200, Roger Pau Monne wrote: > Prevent the assembler from creating a .note.gnu.property section on > the output objects, as it's not useful for firmware related binaries, > and breaks the resulting rombios image. > > This requires modifying the cc-option Makefile macro so it can test > assembler options (by replacing the usage of the -S flag with -c) and > also stripping the -Wa, prefix if present when checking for the test > output. > > Signed-off-by: Roger Pau Monné > --- > Changes since v1: > - Add the option to CFLAGS. > --- > Config.mk | 2 +- > tools/firmware/Rules.mk | 4 > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/Config.mk b/Config.mk > index f56f7dc334..82832945e5 100644 > --- a/Config.mk > +++ b/Config.mk > @@ -91,7 +91,7 @@ PYTHON_PREFIX_ARG ?= --prefix="$(prefix)" > # > # Usage: cflags-y += $(call cc-option,$(CC),-march=winchip-c6,-march=i586) > cc-option = $(shell if test -z "`echo 'void*p=1;' | \ > - $(1) $(2) -S -o /dev/null -x c - 2>&1 | grep -- $(2) -`"; \ > + $(1) $(2) -c -o /dev/null -x c - 2>&1 | grep -- > $(2:-Wa$(comma)%=%) -`"; \ >then echo "$(2)"; else echo "$(3)"; fi ;) Hopefully, changing "-S" to "-c" in this macro will not break anything. I would be of the opinion to create a new macro which deal with assembler options. But if that works and doesn't changes CFLAGS in the testing we do in GitLab, I guess that would be OK. Whether you introduce a macro or keep this one: Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
[libvirt test] 169154: regressions - FAIL
flight 169154 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/169154/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 151777 build-i386-libvirt6 libvirt-buildfail REGR. vs. 151777 build-arm64-libvirt 6 libvirt-buildfail REGR. vs. 151777 build-armhf-libvirt 6 libvirt-buildfail REGR. vs. 151777 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-i386-libvirt-pair 1 build-check(1) blocked n/a test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-raw 1 build-check(1) blocked n/a test-amd64-i386-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-libvirt 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-qcow2 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-raw 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) blocked n/a version targeted for testing: libvirt b7f5ad4610856dc82618ed82c5cab3124be65274 baseline version: libvirt 2c846fa6bcc11929c9fb857a22430fb9945654ad Last test of basis 151777 2020-07-10 04:19:19 Z 633 days Failing since151818 2020-07-11 04:18:52 Z 632 days 614 attempts Testing same since 169126 2022-04-02 04:20:15 Z2 days3 attempts People who touched revisions under test: Adolfo Jayme Barrientos Aleksandr Alekseev Aleksei Zakharov Andika Triwidada Andrea Bolognani Ani Sinha Balázs Meskó Barrett Schonefeld Bastian Germann Bastien Orivel BiaoXiang Ye Bihong Yu Binfeng Wu Bjoern Walk Boris Fiuczynski Brad Laue Brian Turek Bruno Haible Chris Mayo Christian Borntraeger Christian Ehrhardt Christian Kirbach Christian Schoenebeck Christophe Fergeau Claudio Fontana Cole Robinson Collin Walling Cornelia Huck Cédric Bosdonnat Côme Borsoi Daniel Henrique Barboza Daniel Letai Daniel P. Berrange Daniel P. Berrangé Didik Supriadi dinglimin Divya Garg Dmitrii Shcherbakov Dmytro Linkin Eiichi Tsukata Emilio Herrera Eric Farman Erik Skultety Fabian Affolter Fabian Freyer Fabiano Fidêncio Fangge Jin Farhan Ali Fedora Weblate Translation Franck Ridel Gavi Teitz gongwei Guoyi Tu Göran Uddeborg Halil Pasic Han Han Hao Wang Haonan Wang Hela Basa Helmut Grohne Hiroki Narukawa Hyman Huang(黄勇) Ian Wienand Ioanna Alifieraki Ivan Teterevkov Jakob Meng Jamie Strandboge Jamie Strandboge Jan Kuparinen jason lee Jean-Baptiste Holcroft Jia Zhou Jianan Gao Jim Fehlig Jin Yan Jing Qi Jinsheng Zhang Jiri Denemark Joachim Falk John Ferlan John Levon John Levon Jonathan Watt Jonathon Jongsma Julio Faracco Justin Gatzen Ján Tomko Kashyap Chamarthy Kevin Locke Kim InSoo Koichi Murase Kristina Hanicova Laine Stump Laszlo Ersek Lee Yarwood Lei Yang Liao Pingfang Lin Ma Lin Ma Lin Ma Liu Yiding Lubomir Rintel Luke Yue Luyao Zhong Marc Hartmayer Marc-André Lureau Marek Marczykowski-Górecki Markus Schade Martin Kletzander Martin Pitt Masayoshi Mizuma Matej Cepl Matt Coleman Matt Coleman Mauro Matteo Cascella Meina Li Michal Privoznik Michał Smyk Milo Casagrande Moshe Levi Muha Aliss Nathan Neal Gompa Nick Chevsky Nick Shyrokovskiy Nickys Music Group Nico Pache Nicolas Lécureuil Nicolas Lécureuil Nikolay Shirokovskiy Olaf Hering Olesya Gerasimenko Or Ozeri Orion Poplawski Pany Paolo Bonzini Patrick Magauran Paulo de Rezende Pinatti Pavel Hrdina Peng Liang Peter Krempa Pino Toscano Pino Toscano Piotr Drąg Prathamesh Chavan Praveen K Paladugu Richard W.M. Jones Ricky Tigg Robin Lee Rohit Kumar Roman Bogorodskiy Roman Bolshakov Ryan Gahagan Ryan Schmidt Sam Hartma
Re: [PATCH v2 2/2] tools/firmware: do not add a .note.gnu.property section
On Mon, Apr 04, 2022 at 12:12:54PM +0100, Anthony PERARD wrote: > On Mon, Apr 04, 2022 at 12:40:44PM +0200, Roger Pau Monne wrote: > > Prevent the assembler from creating a .note.gnu.property section on > > the output objects, as it's not useful for firmware related binaries, > > and breaks the resulting rombios image. > > > > This requires modifying the cc-option Makefile macro so it can test > > assembler options (by replacing the usage of the -S flag with -c) and > > also stripping the -Wa, prefix if present when checking for the test > > output. > > > > Signed-off-by: Roger Pau Monné > > --- > > Changes since v1: > > - Add the option to CFLAGS. > > --- > > Config.mk | 2 +- > > tools/firmware/Rules.mk | 4 > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/Config.mk b/Config.mk > > index f56f7dc334..82832945e5 100644 > > --- a/Config.mk > > +++ b/Config.mk > > @@ -91,7 +91,7 @@ PYTHON_PREFIX_ARG ?= --prefix="$(prefix)" > > # > > # Usage: cflags-y += $(call cc-option,$(CC),-march=winchip-c6,-march=i586) > > cc-option = $(shell if test -z "`echo 'void*p=1;' | \ > > - $(1) $(2) -S -o /dev/null -x c - 2>&1 | grep -- $(2) -`"; \ > > + $(1) $(2) -c -o /dev/null -x c - 2>&1 | grep -- > > $(2:-Wa$(comma)%=%) -`"; \ > >then echo "$(2)"; else echo "$(3)"; fi ;) > > Hopefully, changing "-S" to "-c" in this macro will not break anything. > I would be of the opinion to create a new macro which deal with > assembler options. But if that works and doesn't changes CFLAGS in the > testing we do in GitLab, I guess that would be OK. It looks like Linux already use "-c" for this macro, and with "-Wa," options. They just don't use grep. So asking CC to do more work here is probably fine (adding compile stage). Cheers, -- Anthony PERARD
Re: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on static allocation
On 31.03.2022 12:30, Penny Zheng wrote:
> Hi Jan
>
>> -Original Message-
>> From: Jan Beulich
>> Sent: Thursday, March 31, 2022 3:06 PM
>> To: Penny Zheng
>> Cc: Wei Chen ; Henry Wang ;
>> Andrew Cooper ; George Dunlap
>> ; Julien Grall ; Stefano Stabellini
>> ; Wei Liu ; xen-
>> de...@lists.xenproject.org
>> Subject: Re: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on
>> static allocation
>>
>> On 31.03.2022 08:13, Penny Zheng wrote:
>>> Hi Jan
>>>
-Original Message-
From: Jan Beulich
Sent: Wednesday, March 30, 2022 5:53 PM
To: Penny Zheng
Cc: Wei Chen ; Henry Wang
>> ;
Andrew Cooper ; George Dunlap
; Julien Grall ; Stefano
Stabellini ; Wei Liu ; xen-
de...@lists.xenproject.org
Subject: Re: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on
static allocation
On 30.03.2022 11:36, Penny Zheng wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -35,6 +35,10 @@
> #include
> #endif
>
> +#ifndef is_domain_on_static_allocation #define
> +is_domain_on_static_allocation(d) 0
Nit: "false", not "0".
> @@ -405,13 +409,29 @@ int guest_remove_page(struct domain *d,
unsigned long gmfn)
> * device must retrieve the same pfn when the hypercall
populate_physmap
> * is called.
> *
> + * When domain on static allocation, they should always get
> + pages from
the
> + * reserved static region when the hypercall populate_physmap is
>> called.
> + *
> * For this purpose (and to match populate_physmap() behavior), the
>> page
> * is kept allocated.
> */
> -if ( !rc && !is_domain_direct_mapped(d) )
> +if ( !rc && !(is_domain_direct_mapped(d) ||
> + is_domain_on_static_allocation(d)) )
> put_page_alloc_ref(page);
>
> put_page(page);
> +#ifdef CONFIG_STATIC_MEMORY
> +/*
> + * When domain on static allocation, we shall store pages to
resv_page_list,
> + * so the hypercall populate_physmap could retrieve pages from it,
> + * rather than allocating from heap.
> + */
> +if ( is_domain_on_static_allocation(d) )
> +{
> +page_list_add_tail(page, &d->resv_page_list);
> +d->resv_pages++;
> +}
> +#endif
I think this is wrong, as a result of not integrating with put_page().
The page should only go on that list once its last ref was dropped. I
don't recall for sure, but iirc staticmem pages are put on the
domain's page list just like other pages would be. But then you also
corrupt the list when this isn't the last ref which is put.
>>>
>>> Yes, staticmem pages are put on the domain's page list.
>>> Here, I tried to only destroy the P2M mapping, and keep the page still
>>> allocated to this domain.
>>
>> Well, much depends on what you call "allocated". For populate_physmap you
>> then take pages from resv_page_list. So I'd like to distinguish "allocated"
>> from
>> "reserved": The pages are allocated to the domain when they're on the normal
>> page list; they're reserved when they're on the new list you introduce. And
>> what you want to initiate here is an "allocated" -> "reserved" transition.
>>
>>> resv_page_list is just providing an easy way to track down the unpopulated
>> memory.
>>> '''
>>> But then you also corrupt the list when this isn't the last ref which
>>> is put.
>>> '''
>>> I'm sorry, would you like to be more specific on this comment?
>>> I want these pages to linked in the domain's page list, then it could
>>> be freed properly when domain get destroyed through relinquish_memory.
>>
>> Clearly they can't be on both lists. Hence you can put them on the new list
>> only _after_ having taken them off the "normal" list. That "taking off the
>> normal list" should happen when the last ref is dropped, not here - see
>> free_domheap_pages()'s uses of arch_free_heap_page(), recalling that
>> free_domheap_page() is what
>> put_page() calls upon dropping the last ref.
>>
>
> Right, right, I've missed the critical point "they can't be on both lists".
> Here is a thing, put_page is a very common helper that it is also beening
> used when freeing memory on domain deconstruction. At that time, I
> don't want to put these pages in resv_page_list, I'd like them to be
> freed to the heap. This putting pages in resv_page_list thing is only for
> unpopulating memory on the runtime. So I'd suggest introducing a
> new helper put_pages_resv(...) to do the work.
I'd like to suggest to avoid introducing special case helpers as much
as possible. put_page() would better remain the single, common
function to use when dropping a ref. Any special treatment for certain
kinds of pages should happen without the callers needing to care.
> About before you mentioned that "The pages are allocated t
Re: [PATCH v4 1/4] x86/APIC: calibrate against platform timer when possible
On Thu, Mar 31, 2022 at 11:29:44AM +0200, Jan Beulich wrote:
> Use the original calibration against PIT only when the platform timer
> is PIT. This implicitly excludes the "xen_guest" case from using the PIT
> logic (init_pit() fails there, and as of 5e73b2594c54 ["x86/time: minor
> adjustments to init_pit()"] using_pit also isn't being set too early
> anymore), so the respective hack there can be dropped at the same time.
> This also reduces calibration time from 100ms to 50ms, albeit this step
> is being skipped as of 0731a56c7c72 ("x86/APIC: no need for timer
> calibration when using TDT") anyway.
>
> While re-indenting the PIT logic in calibrate_APIC_clock(), besides
> adjusting style also switch around the 2nd TSC/TMCCT read pair, to match
> the order of the 1st one, yielding more consistent deltas.
>
> Signed-off-by: Jan Beulich
> ---
> Open-coding apic_read() in apic_tmcct_read() isn't overly nice, but I
> wanted to avoid x2apic_enabled being evaluated twice in close
> succession. And I also wouldn't want to have the barrier there even for
> the (uncached) MMIO read.
>
> Unlike the CPU frequencies enumerated in CPUID leaf 0x16 (which aren't
> precise), using CPUID[0x15].ECX - if populated - may be an option to
> skip calibration altogether. Aiui the value there is precise, but using
> the systems I have easy access to I cannot verify this: In the sample
> of three I have, none have ECX populated.
>
> I wonder whether the secondary CPU freq measurement (used for display
> purposes only) wouldn't better be dropped at this occasion.
I don't have a strong opinion re those informative messages.
> --- a/xen/arch/x86/include/asm/apic.h
> +++ b/xen/arch/x86/include/asm/apic.h
> @@ -192,6 +192,9 @@ extern void record_boot_APIC_mode(void);
> extern enum apic_mode current_local_apic_mode(void);
> extern void check_for_unexpected_msi(unsigned int vector);
>
> +uint64_t calibrate_apic_timer(void);
> +uint32_t apic_tmcct_read(void);
> +
> extern void check_nmi_watchdog(void);
>
> extern unsigned int nmi_watchdog;
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -26,6 +26,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -1018,6 +1019,67 @@ static u64 __init init_platform_timer(vo
> return rc;
> }
>
> +static uint64_t __init read_pt_and_tmcct(uint32_t *tmcct)
> +{
> +uint32_t tmcct_prev = *tmcct = apic_tmcct_read(), tmcct_min = ~0;
> +uint64_t best = best;
> +unsigned int i;
> +
> +for ( i = 0; ; ++i )
> +{
> +uint64_t pt = plt_src.read_counter();
> +uint32_t tmcct_cur = apic_tmcct_read();
> +uint32_t tmcct_delta = tmcct_prev - tmcct_cur;
> +
> +if ( tmcct_delta < tmcct_min )
> +{
> +tmcct_min = tmcct_delta;
> +*tmcct = tmcct_cur;
> +best = pt;
> +}
> +else if ( i > 2 )
> +break;
> +
> +tmcct_prev = tmcct_cur;
> +}
> +
> +return best;
> +}
> +
> +uint64_t __init calibrate_apic_timer(void)
> +{
> +uint32_t start, end;
> +uint64_t count = read_pt_and_tmcct(&start), elapsed;
> +uint64_t target = CALIBRATE_VALUE(plt_src.frequency), actual;
> +uint64_t mask = (uint64_t)~0 >> (64 - plt_src.counter_bits);
> +
> +/*
> + * PIT cannot be used here as it requires the timer interrupt to maintain
> + * its 32-bit software counter, yet here we run with IRQs disabled.
> + */
> +if ( using_pit )
> +return 0;
> +
> +while ( ((plt_src.read_counter() - count) & mask) < target )
> +continue;
> +
> +actual = read_pt_and_tmcct(&end) - count;
Don't you need to apply the pt mask here?
> +elapsed = start - end;
> +
> +if ( likely(actual > target) )
> +{
> +/* See the comment in calibrate_tsc(). */
I would maybe add that the counters used here might have > 32 bits,
and hence we need to trim the values for muldiv64 to scale properly.
Thanks, Roger.
Re: [PATCH v4 1/4] x86/APIC: calibrate against platform timer when possible
On 04.04.2022 13:46, Roger Pau Monné wrote:
> On Thu, Mar 31, 2022 at 11:29:44AM +0200, Jan Beulich wrote:
>> +uint64_t __init calibrate_apic_timer(void)
>> +{
>> +uint32_t start, end;
>> +uint64_t count = read_pt_and_tmcct(&start), elapsed;
>> +uint64_t target = CALIBRATE_VALUE(plt_src.frequency), actual;
>> +uint64_t mask = (uint64_t)~0 >> (64 - plt_src.counter_bits);
>> +
>> +/*
>> + * PIT cannot be used here as it requires the timer interrupt to
>> maintain
>> + * its 32-bit software counter, yet here we run with IRQs disabled.
>> + */
>> +if ( using_pit )
>> +return 0;
>> +
>> +while ( ((plt_src.read_counter() - count) & mask) < target )
>> +continue;
>> +
>> +actual = read_pt_and_tmcct(&end) - count;
>
> Don't you need to apply the pt mask here?
Oh, yes, of course. I guess I did clone an earlier mistake from
calibrate_tsc().
>> +elapsed = start - end;
>> +
>> +if ( likely(actual > target) )
>> +{
>> +/* See the comment in calibrate_tsc(). */
>
> I would maybe add that the counters used here might have > 32 bits,
> and hence we need to trim the values for muldiv64 to scale properly.
Sure - I've added "But first scale down values to actually fit
muldiv64()'s input range."
Jan
Re: [PATCH v2 02/14] x86/P2M: introduce p2m_{add,remove}_page()
On 01.04.2022 14:02, George Dunlap wrote: >> On Feb 23, 2022, at 3:58 PM, Jan Beulich wrote: >> >> Rename guest_physmap_add_entry() to p2m_add_page(); make >> guest_physmap_remove_page() a trivial wrapper around p2m_remove_page(). >> This way callers can use suitable pairs of functions (previously >> violated by hvm/grant_table.c). >> >> In HVM-specific code further avoid going through the guest_physmap_*() >> layer, and instead use the two new/renamed functions directly. >> >> Ultimately the goal is to have guest_physmap_...() functions cover all >> types of guests, but p2m_...() dealing only with translated ones. >> >> Signed-off-by: Jan Beulich >> Reviewed-by: Paul Durrant > > Description reads much better to me — thanks! Well - thanks to you, as iirc it is largely based on your suggestion. > Reviewed-by: George Dunlap Thanks. Jan
Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged
On 04.04.2022 10:08, Roger Pau Monné wrote:
> On Fri, Apr 01, 2022 at 06:52:46PM +0100, Julien Grall wrote:
>> Hi,
>>
>> On 31/03/2022 13:36, Roger Pau Monné wrote:
>>> On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
There are now instances where internal hypervisor logic needs to make
resource
allocation calls that are protected by XSM checks. The internal hypervisor
logic
is represented a number of system domains which by designed are
represented by
non-privileged struct domain instances. To enable these logic blocks to
function correctly but in a controlled manner, this commit introduces a
pair
of privilege escalation and demotion functions that will make a system
domain
privileged and then remove that privilege.
Signed-off-by: Daniel P. Smith
---
xen/include/xsm/xsm.h | 22 ++
>>>
>>> I'm not sure this needs to be in xsm code, AFAICT it could live in a
>>> more generic file.
>>>
1 file changed, 22 insertions(+)
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index e22d6160b5..157e57151e 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -189,6 +189,28 @@ struct xsm_operations {
#endif
};
+static always_inline int xsm_elevate_priv(struct domain *d)
>>>
>>> I don't think it needs to be always_inline, using just inline would be
>>> fine IMO.
>>>
>>> Also this needs to be __init.
>>
>> Hmmm I thought adding __init on function defined in header was
>> pointless. In particular, if the compiler decides to inline it.
>
> Indeed, I didn't realize, thanks for pointing this out.
The question isn't header or not, but declaration or definition.
Attributes like this one are meaningless on declarations (at least
on all the arches we care about; there may be subtleties), but
meaningful for definitions. Iirc even with always_inline the
compiler may find reasons why a function cannot be inlined, and
hence the intended section should be specified. Plus such an
annotation serves a documentation purpose.
Jan
[xen-unstable test] 169151: trouble: broken/fail/pass
flight 169151 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/169151/ Failures and problems with tests :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-xl-credit2 broken Tests which are failing intermittently (not blocking): test-armhf-armhf-xl-credit2 5 host-install(5) broken pass in 169137 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 12 debian-hvm-install fail in 169137 pass in 169151 test-armhf-armhf-xl-rtds 18 guest-start/debian.repeat fail in 169137 pass in 169151 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 12 debian-hvm-install fail in 169137 pass in 169151 Tests which did not succeed, but are not blocking: test-armhf-armhf-xl-credit2 15 migrate-support-check fail in 169137 never pass test-armhf-armhf-xl-credit2 16 saverestore-support-check fail in 169137 never pass test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 169137 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 169137 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 169137 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 169137 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 169137 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 169137 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 169137 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 169137 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 169137 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 169137 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 169137 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 169137 test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 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-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-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-check
Re: [PATCH v4 1/4] x86/APIC: calibrate against platform timer when possible
On Mon, Apr 04, 2022 at 01:55:56PM +0200, Jan Beulich wrote:
> On 04.04.2022 13:46, Roger Pau Monné wrote:
> > On Thu, Mar 31, 2022 at 11:29:44AM +0200, Jan Beulich wrote:
> >> +uint64_t __init calibrate_apic_timer(void)
> >> +{
> >> +uint32_t start, end;
> >> +uint64_t count = read_pt_and_tmcct(&start), elapsed;
> >> +uint64_t target = CALIBRATE_VALUE(plt_src.frequency), actual;
> >> +uint64_t mask = (uint64_t)~0 >> (64 - plt_src.counter_bits);
> >> +
> >> +/*
> >> + * PIT cannot be used here as it requires the timer interrupt to
> >> maintain
> >> + * its 32-bit software counter, yet here we run with IRQs disabled.
> >> + */
> >> +if ( using_pit )
> >> +return 0;
> >> +
> >> +while ( ((plt_src.read_counter() - count) & mask) < target )
> >> +continue;
> >> +
> >> +actual = read_pt_and_tmcct(&end) - count;
> >
> > Don't you need to apply the pt mask here?
>
> Oh, yes, of course. I guess I did clone an earlier mistake from
> calibrate_tsc().
>
> >> +elapsed = start - end;
> >> +
> >> +if ( likely(actual > target) )
> >> +{
> >> +/* See the comment in calibrate_tsc(). */
> >
> > I would maybe add that the counters used here might have > 32 bits,
> > and hence we need to trim the values for muldiv64 to scale properly.
>
> Sure - I've added "But first scale down values to actually fit
> muldiv64()'s input range."
With those taken care of:
Reviewed-by: Roger Pau Monné
Thanks, Roger.
Re: [PATCH v2 2/2] tools/firmware: do not add a .note.gnu.property section
On 04.04.2022 12:40, Roger Pau Monne wrote: > Prevent the assembler from creating a .note.gnu.property section on > the output objects, as it's not useful for firmware related binaries, > and breaks the resulting rombios image. > > This requires modifying the cc-option Makefile macro so it can test > assembler options (by replacing the usage of the -S flag with -c) and > also stripping the -Wa, prefix if present when checking for the test > output. I notice you've ack-ed and committed this patch, which I'm happy to see. However, I don't understand why you gave your ack here, when you did refused to ack (and to explain yourself!) for "x86emul: fix test harness build for gas 2.36". Why is this note section useful there but not similarly useful here (or, the other way around, useless)? (This, as an aside, also makes pretty clear that - unlike the title of the series suggests - this has nothing to do with gcc 11.) Jan
Re: PCI passthrough: possible bug in memory relocation
On 04.04.2022 01:24, Mateusz wrote: > Hello, > I'm working on resolving a bug in Qubes OS. The problem is that > when someone gives a VM too much RAM the GPU passthrough doesn't > work as intended. I think it's because in these cases RAM overlaps > with PCI addresses and memory relocation doesn't work in Xen. > > Here are the memory BARs of the GPU I've tried to passthrough: > Memory at f300 (32-bit, non-prefetchable) [size=16M] > Memory at e000 (64-bit, prefetchable) [size=256M] > Memory at f000 (64-bit, prefetchable) [size=32M] > > The interesting thing is that in xl dmesg hvmloader prints these > lines: > (d1) Relocating 0x pages from 0e0001000 to 18700 for lowmem MMIO hole > (d1) Relocating 0x1 pages from 0e000 to 196fff000 for lowmem MMIO hole > so it looks like it tries to move these pages to a different address > to make space for PCI memory, but for some reason it doesn't work. > Changing TOLUD to 3.5G solves the problem. > I tested it on xen 4.14.3 > Here's the issue on github regarding this problem: > https://github.com/QubesOS/qubes-issues/issues/4321 > > Is it a bug in Xen and fixing it would fix the problem or is there > something I'm missing? I'm afraid answering this requires debugging the issue. Yet you don't share any technical details (as to how things don't work, logs, and alike), and the provided link also doesn't look to point to any such information (and as an aside I consider it somewhat unfriendly to point at such a bug as an information source, not just for reference). I'm pretty sure this code in hvmloader did work at some point, but since it may be used quite rarely I could see that it might have got broken. Jan
Re: [PATCH v4 3/4] include: move STR() and IS_ALIGNED()
On Thu, Mar 31, 2022 at 11:31:02AM +0200, Jan Beulich wrote: > lib.h is imo a better fit for them than config.h. Looks like most of what's in config.h could move to lib.h. > Signed-off-by: Jan Beulich Reviewed-by: Roger Pau Monné Thanks, Roger.
Re: [PATCH v4 4/4] x86/time: use fake read_tsc()
On Thu, Mar 31, 2022 at 11:31:38AM +0200, Jan Beulich wrote:
> Go a step further than bed9ae54df44 ("x86/time: switch platform timer
> hooks to altcall") did and eliminate the "real" read_tsc() altogether:
> It's not used except in pointer comparisons, and hence it looks overall
> more safe to simply poison plt_tsc's read_counter hook.
>
> Signed-off-by: Jan Beulich
> ---
> I wasn't really sure whether it would be better to use simply void * for
> the type of the expression, resulting in an undesirable data -> function
> pointer conversion, but making it impossible to mistakenly try and call
> the (fake) function directly.
I think it's slightly better to avoid being able to call the function,
hence using void * would be my preference. What's wrong with the data
-> function pointer conversion for the comparisons?
> ---
> v2: Comment wording.
>
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -607,10 +607,12 @@ static s64 __init cf_check init_tsc(stru
> return ret;
> }
>
> -static uint64_t __init cf_check read_tsc(void)
> -{
> -return rdtsc_ordered();
> -}
> +/*
> + * plt_tsc's read_counter hook is not (and should not be) invoked via the
> + * struct field. To avoid carrying an unused, indirectly reachable function,
> + * poison the field with an easily identifiable non-canonical pointer.
> + */
> +#define read_tsc ((uint64_t(*)(void))0x75C75C75C75C75C0ul)
Instead of naming this like a suitable function, I would rather use
READ_TSC_PTR_POISON or some such.
Thanks, Roger.
Re: [PATCH v5] x86/vmx: add hvm functions to get/set non-register state
On Fri, Mar 25, 2022 at 9:34 AM Tamas K Lengyel wrote: > > During VM forking and resetting a failed vmentry has been observed due > to the guest non-register state going out-of-sync with the guest register > state. For example, a VM fork reset right after a STI instruction can trigger > the failed entry. This is due to the guest non-register state not being saved > from the parent VM, thus the reset operation only copies the register state. > > Fix this by adding a new pair of hvm functions to get/set the guest > non-register state so that the overall vCPU state remains in sync. > > Signed-off-by: Tamas K Lengyel > --- > v5: Switch to internal-only hvm funcs instead of adding to hvm_hw_cpu Patch ping.
Re: [PATCH v2 2/2] tools/firmware: do not add a .note.gnu.property section
On Mon, Apr 04, 2022 at 02:45:05PM +0200, Jan Beulich wrote: > (This, as an aside, also makes pretty clear that - unlike the title > of the series suggests - this has nothing to do with gcc 11.) I've started debugging this as reported as an issue associated with building with gcc 11, until I realized it was the assembler that was adding such section. I guess in my mind I still had the idea of fixing the build with gcc 11, and hence the misleading subject of the cover letter. The commit messages should be fine however. Thanks, Roger.
Re: preparations for 4.14.5 ?
On 01.04.2022 15:46, Marek Marczykowski-Górecki wrote: > On Wed, Mar 30, 2022 at 12:16:00PM +0200, Jan Beulich wrote: >> All, >> >> while 4.14's general support period ended in January, we're considering >> to cut an out-of-band release due to the relatively large number of >> security relevant backports which has accumulated in just 2 months. By >> doing this we would _not_ be promising to do so again in the future. >> Please voice objections to the plan (or support for it) in the next >> couple of days. >> >> Since it's a little easier to "batch" releases, I would intend to keep >> 4.14.5 aligned with 4.16.1. >> >> Commits I have queued but not committed to the branch yet (and I won't >> until in a couple of days time, to allow for objections to the plan to >> be raised): >> >> dd6c062a7a4a tools/libxl: Correctly align the ACPI tables >> aa390d513a67 build: fix exported variable name CFLAGS_stack_boundary >> e62cc29f9b6c tools/libs: Fix build dependencies >> eddf13b5e940 x86emul: fix VPBLENDMW with mask and memory operand >> 6bd1b4d35c05 x86/console: process softirqs between warning prints >> 07449ecfa425 tools/libxl: don't allow IOMMU usage with PoD >> 10454f381f91 xz: avoid overlapping memcpy() with invalid input with in-place >> decompression >> 0a21660515c2 xz: validate the value before assigning it to an enum variable >> b4f211606011 vpci/msix: fix PBA accesses >> >> Please point out backports you find missing from both the respective >> staging branch and the list above, but which you consider relevant. > > I'm not sure if "just" bugfix qualify for 4.14 at this point, but if so, > I'd propose: > 0a20a53df158 tools/libs/light: set video_mem for PVH guests > > In any case, the above should be backported to 4.15 and 4.16. Hmm, Anthony, I'd like to ask for your view here: This looks more like a cosmetic change to me at the first glance. Plus it's a little odd to see it being proposed for backporting now, when it's already almost 4 months old and hence could have gone into 4.15.2 and 4.14.4 if it was important. Jan
Re: [PATCH v2 2/2] tools/firmware: do not add a .note.gnu.property section
On 04/04/2022 13:45, Jan Beulich wrote: > On 04.04.2022 12:40, Roger Pau Monne wrote: >> Prevent the assembler from creating a .note.gnu.property section on >> the output objects, as it's not useful for firmware related binaries, >> and breaks the resulting rombios image. >> >> This requires modifying the cc-option Makefile macro so it can test >> assembler options (by replacing the usage of the -S flag with -c) and >> also stripping the -Wa, prefix if present when checking for the test >> output. > I notice you've ack-ed and committed this patch, which I'm happy to > see. However, I don't understand why you gave your ack here, when you > did refused to ack (and to explain yourself!) for "x86emul: fix test > harness build for gas 2.36". Why is this note section useful there > but not similarly useful here (or, the other way around, useless)? TBH, I'd forgotten that patch. This work of Roger's came from a real XenServer regression which causes RomBIOS to explode. It needs backporting. In the longterm, I would like to see us use real linker scripts for the artefacts which have custom link requirements, because this is still a bodge. ~Andrew
Re: preparations for 4.14.5 ?
On Mon, Apr 04, 2022 at 03:42:09PM +0200, Jan Beulich wrote: > On 01.04.2022 15:46, Marek Marczykowski-Górecki wrote: > > On Wed, Mar 30, 2022 at 12:16:00PM +0200, Jan Beulich wrote: > >> All, > >> > >> while 4.14's general support period ended in January, we're considering > >> to cut an out-of-band release due to the relatively large number of > >> security relevant backports which has accumulated in just 2 months. By > >> doing this we would _not_ be promising to do so again in the future. > >> Please voice objections to the plan (or support for it) in the next > >> couple of days. > >> > >> Since it's a little easier to "batch" releases, I would intend to keep > >> 4.14.5 aligned with 4.16.1. > >> > >> Commits I have queued but not committed to the branch yet (and I won't > >> until in a couple of days time, to allow for objections to the plan to > >> be raised): > >> > >> dd6c062a7a4a tools/libxl: Correctly align the ACPI tables > >> aa390d513a67 build: fix exported variable name CFLAGS_stack_boundary > >> e62cc29f9b6c tools/libs: Fix build dependencies > >> eddf13b5e940 x86emul: fix VPBLENDMW with mask and memory operand > >> 6bd1b4d35c05 x86/console: process softirqs between warning prints > >> 07449ecfa425 tools/libxl: don't allow IOMMU usage with PoD > >> 10454f381f91 xz: avoid overlapping memcpy() with invalid input with > >> in-place decompression > >> 0a21660515c2 xz: validate the value before assigning it to an enum variable > >> b4f211606011 vpci/msix: fix PBA accesses > >> > >> Please point out backports you find missing from both the respective > >> staging branch and the list above, but which you consider relevant. > > > > I'm not sure if "just" bugfix qualify for 4.14 at this point, but if so, > > I'd propose: > > 0a20a53df158 tools/libs/light: set video_mem for PVH guests > > > > In any case, the above should be backported to 4.15 and 4.16. > > Hmm, Anthony, I'd like to ask for your view here: This looks more > like a cosmetic change to me at the first glance. Plus it's a > little odd to see it being proposed for backporting now, when it's > already almost 4 months old and hence could have gone into 4.15.2 > and 4.14.4 if it was important. A little context (from IRC discussion on Friday) - this was only recently identified to fix videoram set to -1 on PVH: /local/domain/22/memory/static-max = "819200" (n0,r22) /local/domain/22/memory/target = "819201" (n0,r22) /local/domain/22/memory/videoram = "-1" (n0,r22) And since target = static-max - videoram, a guest started with mem=maxmem doesn't really have them equal. -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab signature.asc Description: PGP signature
Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged
On 3/31/22 08:36, Roger Pau Monné wrote:
> On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
>> There are now instances where internal hypervisor logic needs to make
>> resource
>> allocation calls that are protected by XSM checks. The internal hypervisor
>> logic
>> is represented a number of system domains which by designed are represented
>> by
>> non-privileged struct domain instances. To enable these logic blocks to
>> function correctly but in a controlled manner, this commit introduces a pair
>> of privilege escalation and demotion functions that will make a system domain
>> privileged and then remove that privilege.
>>
>> Signed-off-by: Daniel P. Smith
>> ---
>> xen/include/xsm/xsm.h | 22 ++
>
> I'm not sure this needs to be in xsm code, AFAICT it could live in a
> more generic file.
>From my perspective this is access control logic, thus why I advocate
for it to be under XSM. A personal goal is to try to get all security,
i.e. access control, centralized to the extent that it doing so does not
make the code base unnecessarily complicated.
>> 1 file changed, 22 insertions(+)
>>
>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
>> index e22d6160b5..157e57151e 100644
>> --- a/xen/include/xsm/xsm.h
>> +++ b/xen/include/xsm/xsm.h
>> @@ -189,6 +189,28 @@ struct xsm_operations {
>> #endif
>> };
>>
>> +static always_inline int xsm_elevate_priv(struct domain *d)
>
> I don't think it needs to be always_inline, using just inline would be
> fine IMO.
>
> Also this needs to be __init.
AIUI always_inline is likely the best way to preserve the speculation
safety brought in by the call to is_system_domain().
>> +{
>> +if ( is_system_domain(d) )
>> +{
>> +d->is_privileged = true;
>> +return 0;
>> +}
>> +
>
> I would add an ASSERT_UNREACHABLE(); here, I don't think we have any
> use case for trying to elevate the privileges of a non-system domain.
Ack.
v/r
dps
[ovmf test] 169156: regressions - FAIL
flight 169156 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/169156/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 168254 build-amd64-xsm 6 xen-buildfail REGR. vs. 168254 build-i3866 xen-buildfail REGR. vs. 168254 build-i386-xsm6 xen-buildfail REGR. vs. 168254 Tests which did not succeed, but are not blocking: build-amd64-libvirt 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a version targeted for testing: ovmf 3e130e40fc55f06f7fe019e87ed9bae957870a12 baseline version: ovmf b1b89f9009f2390652e0061bd7b24fc40732bc70 Last test of basis 168254 2022-02-28 10:41:46 Z 35 days Failing since168258 2022-03-01 01:55:31 Z 34 days 275 attempts Testing same since 169141 2022-04-03 08:40:51 Z1 days6 attempts People who touched revisions under test: Abdul Lateef Attar Abdul Lateef Attar via groups.io Abner Chang Akihiko Odaki Anthony PERARD Bob Feng Gerd Hoffmann Guo Dong Guomin Jiang Hao A Wu Hua Ma Huang, Li-Xia Jagadeesh Ujja Jason Jason Lou Ken Lautner Kenneth Lautner Kuo, Ted Li, Zhihao Lixia Huang Lou, Yun Ma, Hua Mara Sophie Grosch Mara Sophie Grosch via groups.io Matt DeVillier Michael Kubacki Min Xu Patrick Rudolph Purna Chandra Rao Bandaru Ray Ni Sami Mujawar Sean Rhodes Sean Rhodes sean@starlabs.systems Sebastien Boeuf Sunny Wang Ted Kuo Wenyi Xie wenyi,xie via groups.io Xiaolu.Jiang Xie, Yuanhao Yi Li Yuanhao Xie Zhihao Li jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 3211 lines long.)
Re: [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes
On Thu, Mar 31, 2022 at 11:44:10AM +0200, Jan Beulich wrote: > GrUB2 can be told to leave the screen in the graphics mode it has been > using (or any other one), via "set gfxpayload=keep" (or suitable > variants thereof). In this case we can avoid doing another mode switch > ourselves. This in particular avoids possibly setting the screen to a > less desirable mode: On one of my test systems the set of modes > reported available by the VESA BIOS depends on whether the interposed > KVM switch has that machine set as the active one. If it's not active, > only modes up to 1024x768 get reported, while when active 1280x1024 > modes are also included. For things to always work with an explicitly > specified mode (via the "vga=" option), that mode therefore needs be a > 1024x768 one. > > For some reason this only works for me with "multiboot2" (and > "module2"); "multiboot" (and "module") still forces the screen into text > mode, despite my reading of the sources suggesting otherwise. > > For starters I'm limiting this to graphics modes; I do think this ought > to also work for text modes, but > - I can't tell whether GrUB2 can set any text mode other than 80x25 > (I've only found plain "text" to be valid as a "gfxpayload" setting), > - I'm uncertain whether supporting that is worth it, since I'm uncertain > how many people would be running their systems/screens in text mode, > - I'd like to limit the amount of code added to the realmode trampoline. > > For starters I'm also limiting mode information retrieval to raw BIOS > accesses. This will allow things to work (in principle) also with other > boot environments where a graphics mode can be left in place. The > downside is that this then still is dependent upon switching back to > real mode, so retrieving the needed information from multiboot info is > likely going to be desirable down the road. I'm unsure, what's the benefit from retrieving this information from the VESA blob rather than from multiboot(2) structures? Is it because we require a VESA mode to be set before we parse the multiboot information? > > Signed-off-by: Jan Beulich > --- > I'm not convinced boot_vid_mode really needs setting here; I'm doing so > mainly because setvesabysize also does. > --- > v4: Add changelog entry. > v2: Use 0x9b instead of 0x99 for attributes check: I think the value > used by check_vesa also wants to be converted, to match vesa2's. > (Perhaps the value wants to become a #define, albeit before doing so > I'd question the requirement of the mode to be a color one.) > > --- a/CHANGELOG.md > +++ b/CHANGELOG.md > @@ -6,6 +6,10 @@ The format is based on [Keep a Changelog > > ## [unstable > UNRELEASED](https://xenbits.xen.org/gitweb/?p=xen.git;a=shortlog;h=staging) - > TBD > > +### Changed > + - On x86 "vga=current" can now be used together with GrUB2's gfxpayload > setting. Note that > + this requires use of "multiboot2" (and "module2") as the GrUB commands > loading Xen. > + > ### Removed / support downgraded > - dropped support for the (x86-only) "vesa-mtrr" and "vesa-remap" command > line options > > --- a/xen/arch/x86/boot/video.S > +++ b/xen/arch/x86/boot/video.S > @@ -575,7 +575,6 @@ set14: movw$0x, %ax > movb$0x01, %ah # Define cursor scan lines 11-12 > movw$0x0b0c, %cx > int $0x10 > -set_current: > stc > ret > > @@ -693,6 +692,39 @@ vga_modes: > .word VIDEO_80x60, 0x50,0x3c,0# 80x60 > vga_modes_end: > > +# If the current mode is a VESA graphics one, obtain its parameters. > +set_current: > +leawvesa_glob_info, %di > +movw$0x4f00, %ax > +int $0x10 > +cmpw$0x004f, %ax > +jne .Lsetc_done You don't seem to make use of the information fetched here? I guess this is somehow required to access the other functions? > +movw$0x4f03, %ax It would help readability to have defines for those values, ie: VESA_GET_CURRENT_MODE or some such (not that you need to do it here, just a comment). > +int $0x10 > +cmpw$0x004f, %ax > +jne .Lsetc_done > + > +leawvesa_mode_info, %di # Get mode information structure > +movw%bx, %cx > +movw$0x4f01, %ax > +int $0x10 > +cmpw$0x004f, %ax > +jne .Lsetc_done > + > +movb(%di), %al # Check mode attributes > +andb$0x9b, %al > +cmpb$0x9b, %al So you also check that the reserved D1 bit is set to 1 as mandated by the spec. This is slightly different than what's done in check_vesa, would you mind adding a define for this an unifying with check_vesa? Thanks, Roger.
Re: [PATCH 2/2] arch: ensure idle domain is not left privileged
On 3/31/22 08:46, Roger Pau Monné wrote: > On Wed, Mar 30, 2022 at 07:05:49PM -0400, Daniel P. Smith wrote: >> It is now possible to promote the idle domain to privileged during setup. It >> is not desirable for the idle domain to still be privileged when moving into >> a >> running state. If the idle domain was elevated and not properly demoted, it >> is >> desirable to fail at this point. This commit adds an assert for both x86 and >> Arm just before transitioning to a running state that ensures the idle is not >> privileged. >> >> Signed-off-by: Daniel P. Smith >> --- >> xen/arch/arm/setup.c | 3 +++ >> xen/arch/x86/setup.c | 3 +++ >> 2 files changed, 6 insertions(+) >> >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >> index 7968cee47d..3de394e946 100644 >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -973,6 +973,9 @@ void __init start_xen(unsigned long boot_phys_offset, >> /* Hide UART from DOM0 if we're using it */ >> serial_endboot(); >> >> +/* Ensure idle domain was not left privileged */ >> +ASSERT(current->domain->is_privileged == false) ; >> + >> system_state = SYS_STATE_active; >> >> create_domUs(); > > Hm, I think you want to use the permission promotion of the idle > domain in create_domUs() likely? Apologies, I cherry-picked this onto a branch of staging of what I thought was an up to date remote, but as Julien pointed out I was not. > At which point the check should be after create_domUs, and it would > seem that logically SYS_STATE_active should be set after creating the > domUs. > > Also, FWIW, I'm not seeing this create_domUs() call in my context, > maybe you have other patches on your queue? > >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >> index 885919d5c3..b868463f83 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -589,6 +589,9 @@ static void noinline init_done(void) >> void *va; >> unsigned long start, end; >> >> +/* Ensure idle domain was not left privileged */ >> +ASSERT(current->domain->is_privileged == false) ; > ^ extra space. > > I think you could squash this patch with the previous one and also > squash it with a patch that actually makes use of the introduced > permission promotion functions (or at least in a patch series where > further patches make use the introduced functions). Ack, I can squash them together. v/r, dps
Re: [PATCH v2 2/3] x86/mem_sharing: add fork_complete field to mem_sharing_domain
On Tue, Mar 29, 2022 at 11:51 AM Jan Beulich wrote: > > On 29.03.2022 16:03, Tamas K Lengyel wrote: > > The fork's physmap should only be populated with select special pages during > > the setup of the fork. The rest of the fork's physmap should only be > > populated > > as needed after the fork is complete. Add a field to specify when the fork > > is > > complete so fork_page() can determine whether it's time to start adding > > entries > > to the physmap. > > > > Signed-off-by: Tamas K Lengyel > > Acked-by: Jan Beulich Thanks! After further consideration I'll drop this and the empty_p2m patch from the series as there is a corner-case with PAE-mode guests which doesn't play nice with enforcing a start with an empty p2m. Working around that issue leads to a cascading changeset which ultimately isn't a worthwhile effort. Tamas
Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged
On Mon, Apr 04, 2022 at 10:21:18AM -0400, Daniel P. Smith wrote:
> On 3/31/22 08:36, Roger Pau Monné wrote:
> > On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
> >> There are now instances where internal hypervisor logic needs to make
> >> resource
> >> allocation calls that are protected by XSM checks. The internal hypervisor
> >> logic
> >> is represented a number of system domains which by designed are
> >> represented by
> >> non-privileged struct domain instances. To enable these logic blocks to
> >> function correctly but in a controlled manner, this commit introduces a
> >> pair
> >> of privilege escalation and demotion functions that will make a system
> >> domain
> >> privileged and then remove that privilege.
> >>
> >> Signed-off-by: Daniel P. Smith
> >> ---
> >> xen/include/xsm/xsm.h | 22 ++
> >
> > I'm not sure this needs to be in xsm code, AFAICT it could live in a
> > more generic file.
>
> From my perspective this is access control logic, thus why I advocate
> for it to be under XSM. A personal goal is to try to get all security,
> i.e. access control, centralized to the extent that it doing so does not
> make the code base unnecessarily complicated.
Maybe others have a different opinion, but IMO setting is_privileged
is not XSM specific. It happens to solve an XSM issue, but that's no
reason to place it in the xsm code base.
> >> 1 file changed, 22 insertions(+)
> >>
> >> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> >> index e22d6160b5..157e57151e 100644
> >> --- a/xen/include/xsm/xsm.h
> >> +++ b/xen/include/xsm/xsm.h
> >> @@ -189,6 +189,28 @@ struct xsm_operations {
> >> #endif
> >> };
> >>
> >> +static always_inline int xsm_elevate_priv(struct domain *d)
> >
> > I don't think it needs to be always_inline, using just inline would be
> > fine IMO.
> >
> > Also this needs to be __init.
>
> AIUI always_inline is likely the best way to preserve the speculation
> safety brought in by the call to is_system_domain().
There's nothing related to speculation safety in is_system_domain()
AFAICT. It's just a plain check against d->domain_id. It's my
understanding there's no need for any speculation barrier there
because d->domain_id is not an external input.
In any case this function should be __init only, at which point there
are no untrusted inputs to Xen.
Thanks, Roger.
Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged
On 04.04.2022 17:12, Roger Pau Monné wrote:
> On Mon, Apr 04, 2022 at 10:21:18AM -0400, Daniel P. Smith wrote:
>> On 3/31/22 08:36, Roger Pau Monné wrote:
>>> On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
There are now instances where internal hypervisor logic needs to make
resource
allocation calls that are protected by XSM checks. The internal hypervisor
logic
is represented a number of system domains which by designed are
represented by
non-privileged struct domain instances. To enable these logic blocks to
function correctly but in a controlled manner, this commit introduces a
pair
of privilege escalation and demotion functions that will make a system
domain
privileged and then remove that privilege.
Signed-off-by: Daniel P. Smith
---
xen/include/xsm/xsm.h | 22 ++
>>>
>>> I'm not sure this needs to be in xsm code, AFAICT it could live in a
>>> more generic file.
>>
>> From my perspective this is access control logic, thus why I advocate
>> for it to be under XSM. A personal goal is to try to get all security,
>> i.e. access control, centralized to the extent that it doing so does not
>> make the code base unnecessarily complicated.
>
> Maybe others have a different opinion, but IMO setting is_privileged
> is not XSM specific. It happens to solve an XSM issue, but that's no
> reason to place it in the xsm code base.
To be honest I can see justification for either placement.
1 file changed, 22 insertions(+)
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index e22d6160b5..157e57151e 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -189,6 +189,28 @@ struct xsm_operations {
#endif
};
+static always_inline int xsm_elevate_priv(struct domain *d)
>>>
>>> I don't think it needs to be always_inline, using just inline would be
>>> fine IMO.
>>>
>>> Also this needs to be __init.
>>
>> AIUI always_inline is likely the best way to preserve the speculation
>> safety brought in by the call to is_system_domain().
>
> There's nothing related to speculation safety in is_system_domain()
> AFAICT. It's just a plain check against d->domain_id. It's my
> understanding there's no need for any speculation barrier there
> because d->domain_id is not an external input.
Whether is_system_domain() wants hardening really depends on where
it's used. It doesn't matter as much what specific check an is_...()
function performs, but what code paths it sits on where taking the
wrong branch of a conditional could reveal data which shouldn't be
visible.
Jan
Re: [PATCH] of: of_property_read_string return -ENODATA when !length
On Fri, Apr 01, 2022 at 03:43:42PM -0700, Stefano Stabellini wrote:
> On Fri, 1 Apr 2022, Rob Herring wrote:
> > On Fri, Apr 1, 2022 at 3:49 PM Stefano Stabellini
> > wrote:
> > >
> > > On Fri, 1 Apr 2022, Rob Herring wrote:
> > > > On Thu, Mar 31, 2022 at 7:46 PM Stefano Stabellini
> > > > wrote:
> > > > >
> > > > > From: Stefano Stabellini
> > > > >
> > > > > When the length of the string is zero of_property_read_string should
> > > > > return -ENODATA according to the description of the function.
> > > >
> > > > Perhaps it is a difference of:
> > > >
> > > > prop;
> > > >
> > > > vs.
> > > >
> > > > prop = "";
> > > >
> > > > Both are 0 length by some definition. The description, '-ENODATA if
> > > > property does not have a value', matches the first case.
> > > >
> > > > >
> > > > > However, of_property_read_string doesn't check pp->length. If
> > > > > pp->length
> > > > > is zero, return -ENODATA.
> > > > >
> > > > > Without this patch the following command in u-boot:
> > > > >
> > > > > fdt set /chosen/node property-name
> > > > >
> > > > > results in of_property_read_string returning -EILSEQ when attempting
> > > > > to
> > > > > read property-name. With this patch, it returns -ENODATA as expected.
> > > >
> > > > Why do you care? Do you have a user? There could be an in tree user
> > > > that doesn't like this change.
> > >
> > > During review of a Xen patch series (we have libfdt is Xen too, synced
> > > with the kernel) Julien noticed a check for -EILSEQ. I added the check
> > > so that Xen would behave correctly in cases like the u-boot example in
> > > the patch description.
> > >
> > > Looking more into it, it seemed to be a mismatch between the description
> > > of of_property_read_string and the behavior (e.g. -ENODATA would seem to
> > > be the right return value, not -EILSEQ.)
> > >
> > > I added a printk to confirm what was going on when -EILSEQ was returned:
> > >
> > > printk("DEBUG %s %d value=%s value[0]=%d length=%u
> > > len=%lu\n",__func__,__LINE__,(char*)pp->value,
> > > *((char*)pp->value),pp->length,
> > > strlen(pp->value));
> > >
> > > This is the output:
> > > DEBUG of_property_read_string 205 value= value[0]=0 length=0 len=0
> >
> > It turns out that we never set pp->value to NULL when unflattening
> > (and libfdt always returns a value). This function is assuming we do.
> > >
> > > As the description says:
> > >
> > > *
> > > * Return: 0 on success, -EINVAL if the property does not exist, -ENODATA
> > > if
> > > * property does not have a value, and -EILSEQ if the string is not
> > > * null-terminated within the length of the property data.
> > > *
> > >
> > > It seems that this case matches "property does not have a value" which
> > > is expected to be -ENODATA instead of -EILSEQ. I guess one could also
> > > say that length is zero, so the string cannot be null-terminated,
> > > thus -EILSEQ?
> > >
> > > I am happy to go with your interpretation but -ENODATA seems to be the
> > > best match in my opinion.
> >
> > I agree. I just think empty property should have a NULL value and 0
> > length, but we should only have to check one. I don't want check
> > length as that could be different for Sparc or non-FDT. So I think we
> > need this instead:
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index ec315b060cd5..d6b2b0d49d89 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -165,7 +165,7 @@ static void populate_properties(const void *blob,
> >
> > pp->name = (char *)pname;
> > pp->length = sz;
> > - pp->value = (__be32 *)val;
> > + pp->value = sz ? (__be32 *)val : NULL;
> > *pprev = pp;
> > pprev = &pp->next;
> > }
> >
> >
> > It looks like setting 'value' has been like this at least since 2010.
>
> Yep, fixing old bugs nobody cares about, that's me :-)
:)
> I made the corresponding change to Xen to check that it fixes the
> original issue (I am using Xen only for convenience because I already
> have a reproducer setup for it.)
>
> Unfortunately it breaks the boot: the reason is that of_get_property is
> implemented as:
>
> return pp ? pp->value : NULL;
>
> and at least in Xen (maybe in Linux too) there are instances of callers
> doing:
>
> if (!of_get_property(node, "interrupt-controller", NULL))
> continue;
>
> This would now take the wrong code path because value is returned as
> NULL.
>
> So, although your patch is technically correct, it comes with higher
> risk of breaking existing code. How do you want to proceed?
We should just check 'length' is 0 and drop the !value part.
Rob
[xen-4.15-testing test] 169152: regressions - FAIL
flight 169152 xen-4.15-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/169152/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-livepatch13 livepatch-runfail REGR. vs. 168502 test-amd64-amd64-livepatch 13 livepatch-runfail REGR. vs. 168502 Tests which are failing intermittently (not blocking): test-amd64-i386-xl-qemut-debianhvm-i386-xsm 12 debian-hvm-install fail pass in 169143 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 168502 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 168502 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 168502 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 168502 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 168502 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 168502 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 168502 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 168502 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 168502 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 168502 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 168502 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 168502 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-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-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 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-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-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-
Re: [PATCH v4 4/4] x86/time: use fake read_tsc()
On 04.04.2022 15:22, Roger Pau Monné wrote:
> On Thu, Mar 31, 2022 at 11:31:38AM +0200, Jan Beulich wrote:
>> Go a step further than bed9ae54df44 ("x86/time: switch platform timer
>> hooks to altcall") did and eliminate the "real" read_tsc() altogether:
>> It's not used except in pointer comparisons, and hence it looks overall
>> more safe to simply poison plt_tsc's read_counter hook.
>>
>> Signed-off-by: Jan Beulich
>> ---
>> I wasn't really sure whether it would be better to use simply void * for
>> the type of the expression, resulting in an undesirable data -> function
>> pointer conversion, but making it impossible to mistakenly try and call
>> the (fake) function directly.
>
> I think it's slightly better to avoid being able to call the function,
> hence using void * would be my preference. What's wrong with the data
> -> function pointer conversion for the comparisons?
There's no data -> function pointer conversion for the comparisons; the
situation there is even less pleasant. What I referred to was actually
the initializer, where there would be a data -> function pointer
conversion if I used void *.
>> ---
>> v2: Comment wording.
>>
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -607,10 +607,12 @@ static s64 __init cf_check init_tsc(stru
>> return ret;
>> }
>>
>> -static uint64_t __init cf_check read_tsc(void)
>> -{
>> -return rdtsc_ordered();
>> -}
>> +/*
>> + * plt_tsc's read_counter hook is not (and should not be) invoked via the
>> + * struct field. To avoid carrying an unused, indirectly reachable function,
>> + * poison the field with an easily identifiable non-canonical pointer.
>> + */
>> +#define read_tsc ((uint64_t(*)(void))0x75C75C75C75C75C0ul)
>
> Instead of naming this like a suitable function, I would rather use
> READ_TSC_PTR_POISON or some such.
I'll be happy to name it something like this; the primary thing to
settle on is the type to use.
Jan
Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged
On 3/31/22 09:16, Jason Andryuk wrote:
> On Wed, Mar 30, 2022 at 3:05 PM Daniel P. Smith
> wrote:
>>
>> There are now instances where internal hypervisor logic needs to make
>> resource
>> allocation calls that are protected by XSM checks. The internal hypervisor
>> logic
>> is represented a number of system domains which by designed are represented
>> by
>> non-privileged struct domain instances. To enable these logic blocks to
>> function correctly but in a controlled manner, this commit introduces a pair
>> of privilege escalation and demotion functions that will make a system domain
>> privileged and then remove that privilege.
>>
>> Signed-off-by: Daniel P. Smith
>> ---
>> xen/include/xsm/xsm.h | 22 ++
>> 1 file changed, 22 insertions(+)
>>
>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
>> index e22d6160b5..157e57151e 100644
>> --- a/xen/include/xsm/xsm.h
>> +++ b/xen/include/xsm/xsm.h
>> @@ -189,6 +189,28 @@ struct xsm_operations {
>> #endif
>> };
>>
>> +static always_inline int xsm_elevate_priv(struct domain *d)
>> +{
>> +if ( is_system_domain(d) )
>> +{
>> +d->is_privileged = true;
>> +return 0;
>> +}
>> +
>> +return -EPERM;
>> +}
>
> These look sufficient for the default policy, but they don't seem
> sufficient for Flask. I think you need to create a new XSM hook. For
> Flask, you would want the demote hook to transition xen_boot_t ->
> xen_t. That would start xen_boot_t with privileges that are dropped
> in a one-way transition. Does that require all policies to then have
> xen_boot_t and xen_t? I guess it does unless the hook code has some
> logic to skip the transition.
I am still thinking this through but my initial concern for Flask is
that I don't think we want dedicated domain transitions directly in
code. My current thinking would be to use a Kconfig to use xen_boot_t
type as the initial sid for the idle domain which would then require the
default policy to include an allowed transition from xen_boot_t to
xen_t. Then rely upon a boot domain to issue an xsm_op to do a relabel
transition for the idle domain with an assertion that the idle domain is
no longer labeled with its initial sid before Xen transitions its state
to SYS_STATE_active. The one wrinkle to this is whether I will be able
to schedule the boot domain before allowing Xen to transition into
SYS_STATE_active.
> For the default policy, you could start by creating the system domains
> as privileged and just have a single hook to drop privs. Then you
> don't have to worry about the "elevate" hook existing. The patch 2
> asserts could instead become the location of xsm_drop_privs calls to
> have a clear demarcation point. That expands the window with
> privileges though. It's a little simpler, but maybe you don't want
> that. However, it seems like you can only depriv once for the Flask
> case since you want it to be one-way.
This does simplify the solution and since today we cannot differentiate
between hypervisor setup and hypervisor initiated domain construction
contexts, it does not run counter to what I have proposed. As for flask,
again I do not believe codifying a domain transition bound to a new XSM
op is the appropriate approach.
v/r,
dps
Re: [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes
(reducing Cc list some) On 04.04.2022 16:49, Roger Pau Monné wrote: > On Thu, Mar 31, 2022 at 11:44:10AM +0200, Jan Beulich wrote: >> GrUB2 can be told to leave the screen in the graphics mode it has been >> using (or any other one), via "set gfxpayload=keep" (or suitable >> variants thereof). In this case we can avoid doing another mode switch >> ourselves. This in particular avoids possibly setting the screen to a >> less desirable mode: On one of my test systems the set of modes >> reported available by the VESA BIOS depends on whether the interposed >> KVM switch has that machine set as the active one. If it's not active, >> only modes up to 1024x768 get reported, while when active 1280x1024 >> modes are also included. For things to always work with an explicitly >> specified mode (via the "vga=" option), that mode therefore needs be a >> 1024x768 one. >> >> For some reason this only works for me with "multiboot2" (and >> "module2"); "multiboot" (and "module") still forces the screen into text >> mode, despite my reading of the sources suggesting otherwise. >> >> For starters I'm limiting this to graphics modes; I do think this ought >> to also work for text modes, but >> - I can't tell whether GrUB2 can set any text mode other than 80x25 >> (I've only found plain "text" to be valid as a "gfxpayload" setting), >> - I'm uncertain whether supporting that is worth it, since I'm uncertain >> how many people would be running their systems/screens in text mode, >> - I'd like to limit the amount of code added to the realmode trampoline. >> >> For starters I'm also limiting mode information retrieval to raw BIOS >> accesses. This will allow things to work (in principle) also with other >> boot environments where a graphics mode can be left in place. The >> downside is that this then still is dependent upon switching back to >> real mode, so retrieving the needed information from multiboot info is >> likely going to be desirable down the road. > > I'm unsure, what's the benefit from retrieving this information from > the VESA blob rather than from multiboot(2) structures? As said - it allows things to work even when that data isn't provided. Note also how I say "for starters" - patch 2 adds logic to retrieve the information from MB. > Is it because we require a VESA mode to be set before we parse the > multiboot information? No, I don't think so. >> --- a/xen/arch/x86/boot/video.S >> +++ b/xen/arch/x86/boot/video.S >> @@ -575,7 +575,6 @@ set14: movw$0x, %ax >> movb$0x01, %ah # Define cursor scan lines 11-12 >> movw$0x0b0c, %cx >> int $0x10 >> -set_current: >> stc >> ret >> >> @@ -693,6 +692,39 @@ vga_modes: >> .word VIDEO_80x60, 0x50,0x3c,0# 80x60 >> vga_modes_end: >> >> +# If the current mode is a VESA graphics one, obtain its parameters. >> +set_current: >> +leawvesa_glob_info, %di >> +movw$0x4f00, %ax >> +int $0x10 >> +cmpw$0x004f, %ax >> +jne .Lsetc_done > > You don't seem to make use of the information fetched here? I guess > this is somehow required to access the other functions? See the similar logic at check_vesa. The information is used later, by mode_params (half way into mopar_gr). Quite likely this could be done just in a single place, but that would require some restructuring of the code, which I'd like to avoid doing here. >> +movw$0x4f03, %ax > > It would help readability to have defines for those values, ie: > VESA_GET_CURRENT_MODE or some such (not that you need to do it here, > just a comment). Right - this applies to all of our BIOS interfacing code, I guess. >> +int $0x10 >> +cmpw$0x004f, %ax >> +jne .Lsetc_done >> + >> +leawvesa_mode_info, %di # Get mode information structure >> +movw%bx, %cx >> +movw$0x4f01, %ax >> +int $0x10 >> +cmpw$0x004f, %ax >> +jne .Lsetc_done >> + >> +movb(%di), %al # Check mode attributes >> +andb$0x9b, %al >> +cmpb$0x9b, %al > > So you also check that the reserved D1 bit is set to 1 as mandated by > the spec. This is slightly different than what's done in check_vesa, > would you mind adding a define for this an unifying with check_vesa? Well, see the v2 changelog comment. I'm somewhat hesitant to do that here; I'd prefer to consolidate this in a separate patch. Jan
Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged
On 4/4/22 11:12, Roger Pau Monné wrote:
> On Mon, Apr 04, 2022 at 10:21:18AM -0400, Daniel P. Smith wrote:
>> On 3/31/22 08:36, Roger Pau Monné wrote:
>>> On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
There are now instances where internal hypervisor logic needs to make
resource
allocation calls that are protected by XSM checks. The internal hypervisor
logic
is represented a number of system domains which by designed are
represented by
non-privileged struct domain instances. To enable these logic blocks to
function correctly but in a controlled manner, this commit introduces a
pair
of privilege escalation and demotion functions that will make a system
domain
privileged and then remove that privilege.
Signed-off-by: Daniel P. Smith
---
xen/include/xsm/xsm.h | 22 ++
>>>
>>> I'm not sure this needs to be in xsm code, AFAICT it could live in a
>>> more generic file.
>>
>> From my perspective this is access control logic, thus why I advocate
>> for it to be under XSM. A personal goal is to try to get all security,
>> i.e. access control, centralized to the extent that it doing so does not
>> make the code base unnecessarily complicated.
>
> Maybe others have a different opinion, but IMO setting is_privileged
> is not XSM specific. It happens to solve an XSM issue, but that's no
> reason to place it in the xsm code base.
I think this deserves a separate more dedicated thread as I would take
the position that Xen privilege model is at best fractured because
is_control_domain() (and underlying is_privileged),
is_hardware_domain,() and is_xenstore_domain() are used independent of
XSM. Perhaps the discussion can be had when I get back to the XSM Roles
patches to enable disaggregating Xen with hyperlaunch (or at a Xen
Summit design session).
1 file changed, 22 insertions(+)
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index e22d6160b5..157e57151e 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -189,6 +189,28 @@ struct xsm_operations {
#endif
};
+static always_inline int xsm_elevate_priv(struct domain *d)
>>>
>>> I don't think it needs to be always_inline, using just inline would be
>>> fine IMO.
>>>
>>> Also this needs to be __init.
>>
>> AIUI always_inline is likely the best way to preserve the speculation
>> safety brought in by the call to is_system_domain().
>
> There's nothing related to speculation safety in is_system_domain()
> AFAICT. It's just a plain check against d->domain_id. It's my
> understanding there's no need for any speculation barrier there
> because d->domain_id is not an external input.
Hmmm, this actually raises a good question. Why is is_control_domain(),
is_hardware_domain, and others all have evaluate_nospec() wrapping the
check of a struct domain element while is_system_domain() does not?
> In any case this function should be __init only, at which point there
> are no untrusted inputs to Xen.
I thought it was agreed that __init on inline functions in headers had
no meaning?
RE: [PATCH 09/15] swiotlb: make the swiotlb_init interface more useful
From: Christoph Hellwig Sent: Sunday, April 3, 2022 10:06 PM
>
> Pass a bool to pass if swiotlb needs to be enabled based on the
Wording problems. I'm not sure what you meant to say.
> addressing needs and replace the verbose argument with a set of
> flags, including one to force enable bounce buffering.
>
> Note that this patch removes the possibility to force xen-swiotlb
> use using swiotlb=force on the command line on x86 (arm and arm64
> never supported that), but this interface will be restored shortly.
>
> Signed-off-by: Christoph Hellwig
> ---
> arch/arm/mm/init.c | 6 +
> arch/arm64/mm/init.c | 6 +
> arch/ia64/mm/init.c| 4 +--
> arch/mips/cavium-octeon/dma-octeon.c | 2 +-
> arch/mips/loongson64/dma.c | 2 +-
> arch/mips/sibyte/common/dma.c | 2 +-
> arch/powerpc/mm/mem.c | 3 ++-
> arch/powerpc/platforms/pseries/setup.c | 3 ---
> arch/riscv/mm/init.c | 8 +-
> arch/s390/mm/init.c| 3 +--
> arch/x86/kernel/pci-dma.c | 15 ++-
> drivers/xen/swiotlb-xen.c | 4 +--
> include/linux/swiotlb.h| 15 ++-
> include/trace/events/swiotlb.h | 29 -
> kernel/dma/swiotlb.c | 35 ++
> 15 files changed, 55 insertions(+), 82 deletions(-)
>
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index fe249ea919083..ce64bdb55a16b 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -271,11 +271,7 @@ static void __init free_highpages(void)
> void __init mem_init(void)
> {
> #ifdef CONFIG_ARM_LPAE
> - if (swiotlb_force == SWIOTLB_FORCE ||
> - max_pfn > arm_dma_pfn_limit)
> - swiotlb_init(1);
> - else
> - swiotlb_force = SWIOTLB_NO_FORCE;
> + swiotlb_init(max_pfn > arm_dma_pfn_limit, SWIOTLB_VERBOSE);
> #endif
>
> set_max_mapnr(pfn_to_page(max_pfn) - mem_map);
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index 8ac25f19084e8..7b6ea4d6733d6 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -398,11 +398,7 @@ void __init bootmem_init(void)
> */
> void __init mem_init(void)
> {
> - if (swiotlb_force == SWIOTLB_FORCE ||
> - max_pfn > PFN_DOWN(arm64_dma_phys_limit))
> - swiotlb_init(1);
> - else if (!xen_swiotlb_detect())
> - swiotlb_force = SWIOTLB_NO_FORCE;
> + swiotlb_init(max_pfn > PFN_DOWN(arm64_dma_phys_limit),
> SWIOTLB_VERBOSE);
>
> /* this will put all unused low memory onto the freelists */
> memblock_free_all();
> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
> index 5d165607bf354..3c3e15b22608f 100644
> --- a/arch/ia64/mm/init.c
> +++ b/arch/ia64/mm/init.c
> @@ -437,9 +437,7 @@ mem_init (void)
> if (iommu_detected)
> break;
> #endif
> -#ifdef CONFIG_SWIOTLB
> - swiotlb_init(1);
> -#endif
> + swiotlb_init(true, SWIOTLB_VERBOSE);
> } while (0);
>
> #ifdef CONFIG_FLATMEM
> diff --git a/arch/mips/cavium-octeon/dma-octeon.c
> b/arch/mips/cavium-octeon/dma-
> octeon.c
> index fb7547e217263..9fbba6a8fa4c5 100644
> --- a/arch/mips/cavium-octeon/dma-octeon.c
> +++ b/arch/mips/cavium-octeon/dma-octeon.c
> @@ -235,5 +235,5 @@ void __init plat_swiotlb_setup(void)
> #endif
>
> swiotlb_adjust_size(swiotlbsize);
> - swiotlb_init(1);
> + swiotlb_init(true, SWIOTLB_VERBOSE);
> }
> diff --git a/arch/mips/loongson64/dma.c b/arch/mips/loongson64/dma.c
> index 364f2f27c8723..8220a1bc0db64 100644
> --- a/arch/mips/loongson64/dma.c
> +++ b/arch/mips/loongson64/dma.c
> @@ -24,5 +24,5 @@ phys_addr_t dma_to_phys(struct device *dev, dma_addr_t
> daddr)
>
> void __init plat_swiotlb_setup(void)
> {
> - swiotlb_init(1);
> + swiotlb_init(true, SWIOTLB_VERBOSE);
> }
> diff --git a/arch/mips/sibyte/common/dma.c b/arch/mips/sibyte/common/dma.c
> index eb47a94f3583e..c5c2c782aff68 100644
> --- a/arch/mips/sibyte/common/dma.c
> +++ b/arch/mips/sibyte/common/dma.c
> @@ -10,5 +10,5 @@
>
> void __init plat_swiotlb_setup(void)
> {
> - swiotlb_init(1);
> + swiotlb_init(true, SWIOTLB_VERBOSE);
> }
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index 8e301cd8925b2..e1519e2edc656 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -17,6 +17,7 @@
> #include
> #include
>
> +#include
> #include
> #include
> #include
> @@ -251,7 +252,7 @@ void __init mem_init(void)
> if (is_secure_guest())
> svm_swiotlb_init();
> else
> - swiotlb_init(0);
> + swiotlb_init(ppc_swiotlb_enable, 0);
> #endif
>
> high_memory = (void *) __va(max_low_pfn * PAGE_SIZE);
> diff --git a/arch/powerpc/platforms/pseries/setup.c
> b/arch/powerpc/platforms/pseries/setup.c
> index 069d7b3bb142e..c6e06d91b6602 100644
[xen-unstable-smoke test] 169160: tolerable all pass - PUSHED
flight 169160 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/169160/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen e270af94280e6a9610705ebc1fdd1d7a9b1f8a98 baseline version: xen d62a34423a1a98aefd7c30e22d2d82d198f077c8 Last test of basis 169114 2022-04-01 17:01:54 Z3 days Testing same since 169160 2022-04-04 12:03:06 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Anthony PERARD Roger Pau Monne Roger Pau Monné jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git d62a34423a..e270af9428 e270af94280e6a9610705ebc1fdd1d7a9b1f8a98 -> smoke
[linux-linus test] 169157: regressions - FAIL
flight 169157 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/169157/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-arm64-pvops 6 kernel-build fail REGR. vs. 169145 Tests which did not succeed, but are not blocking: 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-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-seattle 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-armhf-armhf-libvirt-raw 15 saverestore-support-check fail blocked in 169145 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 169145 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 169145 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 169145 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 169145 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 169145 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 169145 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 169145 test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass version targeted for testing: linux3123109284176b1532874591f7c81f3837bbdc17 baseline version: linux09bb8856d4a7cf3128dedd79cd07d75bbf4a9f04 Last test of basis 169145 2022-04-03 20:41:35 Z0 days Testing same since 169157 2022-04-04 06:23:01 Z0 days1 attempts People who touched revisions under test: Linus Torvalds jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-arm64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-arm64-pvops
[ovmf test] 169161: regressions - FAIL
flight 169161 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/169161/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 168254 build-amd64-xsm 6 xen-buildfail REGR. vs. 168254 build-i3866 xen-buildfail REGR. vs. 168254 build-i386-xsm6 xen-buildfail REGR. vs. 168254 Tests which did not succeed, but are not blocking: build-amd64-libvirt 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a version targeted for testing: ovmf 3e130e40fc55f06f7fe019e87ed9bae957870a12 baseline version: ovmf b1b89f9009f2390652e0061bd7b24fc40732bc70 Last test of basis 168254 2022-02-28 10:41:46 Z 35 days Failing since168258 2022-03-01 01:55:31 Z 34 days 276 attempts Testing same since 169141 2022-04-03 08:40:51 Z1 days7 attempts People who touched revisions under test: Abdul Lateef Attar Abdul Lateef Attar via groups.io Abner Chang Akihiko Odaki Anthony PERARD Bob Feng Gerd Hoffmann Guo Dong Guomin Jiang Hao A Wu Hua Ma Huang, Li-Xia Jagadeesh Ujja Jason Jason Lou Ken Lautner Kenneth Lautner Kuo, Ted Li, Zhihao Lixia Huang Lou, Yun Ma, Hua Mara Sophie Grosch Mara Sophie Grosch via groups.io Matt DeVillier Michael Kubacki Min Xu Patrick Rudolph Purna Chandra Rao Bandaru Ray Ni Sami Mujawar Sean Rhodes Sean Rhodes sean@starlabs.systems Sebastien Boeuf Sunny Wang Ted Kuo Wenyi Xie wenyi,xie via groups.io Xiaolu.Jiang Xie, Yuanhao Yi Li Yuanhao Xie Zhihao Li jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 3211 lines long.)
Re: [PATCH] of: of_property_read_string return -ENODATA when !length
On 4/4/22 10:28, Rob Herring wrote:
> On Fri, Apr 01, 2022 at 03:43:42PM -0700, Stefano Stabellini wrote:
>> On Fri, 1 Apr 2022, Rob Herring wrote:
>>> On Fri, Apr 1, 2022 at 3:49 PM Stefano Stabellini
>>> wrote:
On Fri, 1 Apr 2022, Rob Herring wrote:
> On Thu, Mar 31, 2022 at 7:46 PM Stefano Stabellini
> wrote:
>>
>> From: Stefano Stabellini
>>
>> When the length of the string is zero of_property_read_string should
>> return -ENODATA according to the description of the function.
>
> Perhaps it is a difference of:
>
> prop;
>
> vs.
>
> prop = "";
>
> Both are 0 length by some definition. The description, '-ENODATA if
> property does not have a value', matches the first case.
>
>>
>> However, of_property_read_string doesn't check pp->length. If pp->length
>> is zero, return -ENODATA.
>>
>> Without this patch the following command in u-boot:
>>
>> fdt set /chosen/node property-name
>>
>> results in of_property_read_string returning -EILSEQ when attempting to
>> read property-name. With this patch, it returns -ENODATA as expected.
>
> Why do you care? Do you have a user? There could be an in tree user
> that doesn't like this change.
During review of a Xen patch series (we have libfdt is Xen too, synced
with the kernel) Julien noticed a check for -EILSEQ. I added the check
so that Xen would behave correctly in cases like the u-boot example in
the patch description.
Looking more into it, it seemed to be a mismatch between the description
of of_property_read_string and the behavior (e.g. -ENODATA would seem to
be the right return value, not -EILSEQ.)
I added a printk to confirm what was going on when -EILSEQ was returned:
printk("DEBUG %s %d value=%s value[0]=%d length=%u
len=%lu\n",__func__,__LINE__,(char*)pp->value,
*((char*)pp->value),pp->length,
strlen(pp->value));
This is the output:
DEBUG of_property_read_string 205 value= value[0]=0 length=0 len=0
>>>
>>> It turns out that we never set pp->value to NULL when unflattening
>>> (and libfdt always returns a value). This function is assuming we do.
As the description says:
*
* Return: 0 on success, -EINVAL if the property does not exist, -ENODATA
if
* property does not have a value, and -EILSEQ if the string is not
* null-terminated within the length of the property data.
*
It seems that this case matches "property does not have a value" which
is expected to be -ENODATA instead of -EILSEQ. I guess one could also
say that length is zero, so the string cannot be null-terminated,
thus -EILSEQ?
I am happy to go with your interpretation but -ENODATA seems to be the
best match in my opinion.
>>>
>>> I agree. I just think empty property should have a NULL value and 0
>>> length, but we should only have to check one. I don't want check
>>> length as that could be different for Sparc or non-FDT. So I think we
>>> need this instead:
>>>
>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>> index ec315b060cd5..d6b2b0d49d89 100644
>>> --- a/drivers/of/fdt.c
>>> +++ b/drivers/of/fdt.c
>>> @@ -165,7 +165,7 @@ static void populate_properties(const void *blob,
>>>
>>> pp->name = (char *)pname;
>>> pp->length = sz;
>>> - pp->value = (__be32 *)val;
>>> + pp->value = sz ? (__be32 *)val : NULL;
>>> *pprev = pp;
>>> pprev = &pp->next;
>>> }
>>>
>>>
>>> It looks like setting 'value' has been like this at least since 2010.
>>
>> Yep, fixing old bugs nobody cares about, that's me :-)
>
> :)
>
>
>> I made the corresponding change to Xen to check that it fixes the
>> original issue (I am using Xen only for convenience because I already
>> have a reproducer setup for it.)
>>
>> Unfortunately it breaks the boot: the reason is that of_get_property is
>> implemented as:
>>
>> return pp ? pp->value : NULL;
>>
>> and at least in Xen (maybe in Linux too) there are instances of callers
>> doing:
>>
>> if (!of_get_property(node, "interrupt-controller", NULL))
>> continue;
>>
>> This would now take the wrong code path because value is returned as
>> NULL.
>>
>> So, although your patch is technically correct, it comes with higher
>> risk of breaking existing code. How do you want to proceed?
>
> We should just check 'length' is 0 and drop the !value part.
I agree with checking prop->length (not "pp->length" as in the original
patch because there is no "pp" in of_property_read_string()), and return
-ENODATA for that case.
I'm ok with dropping the prop->value check since we populate the field
with a non-zero value during unflattenning.
And update the function header documentation to mention that the empty
string "" has a length of 1
[ovmf test] 169165: regressions - FAIL
flight 169165 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/169165/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 168254 build-amd64-xsm 6 xen-buildfail REGR. vs. 168254 build-i3866 xen-buildfail REGR. vs. 168254 build-i386-xsm6 xen-buildfail REGR. vs. 168254 Tests which did not succeed, but are not blocking: build-amd64-libvirt 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a version targeted for testing: ovmf ad6816c319cdbd927d81e071996a0dea33c86e4a baseline version: ovmf b1b89f9009f2390652e0061bd7b24fc40732bc70 Last test of basis 168254 2022-02-28 10:41:46 Z 35 days Failing since168258 2022-03-01 01:55:31 Z 34 days 277 attempts Testing same since 169165 2022-04-04 19:43:09 Z0 days1 attempts People who touched revisions under test: Abdul Lateef Attar Abdul Lateef Attar via groups.io Abner Chang Akihiko Odaki Anthony PERARD Bob Feng Gerd Hoffmann Guo Dong Guomin Jiang Hao A Wu Hua Ma Huang, Li-Xia Jagadeesh Ujja Jason Jason Lou Ken Lautner Kenneth Lautner Kuo, Ted Laszlo Ersek Li, Zhihao Lixia Huang Lou, Yun Ma, Hua Mara Sophie Grosch Mara Sophie Grosch via groups.io Matt DeVillier Michael Kubacki Min Xu Patrick Rudolph Purna Chandra Rao Bandaru Ray Ni Sami Mujawar Sean Rhodes Sean Rhodes sean@starlabs.systems Sebastien Boeuf Sunny Wang Ted Kuo Wenyi Xie wenyi,xie via groups.io Xiaolu.Jiang Xie, Yuanhao Yi Li Yuanhao Xie Zhihao Li jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 3336 lines long.)
[xen-4.14-testing test] 169158: regressions - FAIL
flight 169158 xen-4.14-testing real [real] flight 169168 xen-4.14-testing real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/169158/ http://logs.test-lab.xenproject.org/osstest/logs/169168/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-livepatch 13 livepatch-runfail REGR. vs. 168506 test-amd64-i386-livepatch13 livepatch-runfail REGR. vs. 168506 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 168506 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 168506 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 168506 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 168506 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 168506 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 168506 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 168506 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 168506 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 168506 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 168506 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 168506 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 168506 test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 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-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-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-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail 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-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-credit1 15 migrate-support-
Re: PCI passthrough: possible bug in memory relocation
-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 > I'm afraid answering this requires debugging the issue. Yet you don't > share any technical details (as to how things don't work, logs, and > alike), and the provided link also doesn't look to point to any such > information (and as an aside I consider it somewhat unfriendly to > point at such a bug as an information source, not just for reference). > I'm pretty sure this code in hvmloader did work at some point, but > since it may be used quite rarely I could see that it might have got > broken. Thanks for responding! I only wanted to ask to see if maybe it's a known issue, but I guess not. I'll try to debug and fix it myself so that's why I haven't posted more technical details yet. -BEGIN PGP SIGNATURE- iQIzBAEBCgAdFiEE0RoD3+S7b5zXZ6lW6IGQZRJEAKsFAmJLfqYACgkQ6IGQZRJE AKuowA/+OOcS4CyBmG/NEF4Brc+oAsjdXsz3vZvMgqhc5oNU7hYOLnOg5KKZ0xvz p2+Tm5saS05A+LIiFXC302tiwoQ9Gq+9hXw92c5nI+FZWyMNvA71Y27XKiHymWpe ksORbIdc1B4st4J3bEls4R1PyjSuYyaEMcFMHH7aPGydXFjgon/8BtqenwKz2vrM ncW+/VtQkAj2BcwMJbSq/M+JUOm115Jvb3LQDhCi/XvoGrduW+HB+lavN9opnJnT jlSeS6H96L70EWYnV49+i5OBgQrcFfDQcZqZ4+dU9lFjEvWYn2d2wNzwD6PlXl1g kHX+PBM95UYJDEwlCXGBX9Dc68LRIMAfpaOyzZsEYoNHbExGUPVpzKx9a7SnZBJZ 1X94MKuxDrbIULvpP1QezNaBMojtagI30DSODbuBpmcyu6Bl+QlKL/OFeP41+Ic5 EOWoFqjklNbvSVMyG08elRvmaR63JAHncCqDHBnxQ7eThMFQGNnY2qzniKqas/UR 2H0XgU5UqzZCsr+4Yk4Ab9gS06t+UdCTJAvoX9SyS0kjSHNkpF8fKZI8AgRg/RWL mtMwouwZRaKlVmezYJKfuHvLQpb19dZFcFtkBOFN1PftXxNMPIEWq0w17rRzwWp0 QL6mzytmQXK5lfC2eeUDiggyk5gp25Gnw1bQyl6eSW/nN16cP/Q= =pAFu -END PGP SIGNATURE-
[xen-4.15-testing test] 169162: tolerable FAIL - PUSHED
flight 169162 xen-4.15-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/169162/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-rtds18 guest-start/debian.repeat fail REGR. vs. 168502 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 168502 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 168502 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 168502 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 168502 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 168502 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 168502 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 168502 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 168502 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 168502 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 168502 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 168502 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 168502 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-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-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 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-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-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass vers
[xen-unstable test] 169163: tolerable FAIL - PUSHED
flight 169163 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/169163/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 169151 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 169151 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 169151 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 169151 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 169151 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 169151 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 169151 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 169151 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 169151 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 169151 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 169151 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 169151 test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 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-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 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-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-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass version targeted for testing: xen e270af94280e6a9610705ebc1fdd1d7a9b1f8a98 baseline version: xen d62a34423a1a98ae
[ovmf test] 169169: regressions - FAIL
flight 169169 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/169169/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 168254 build-amd64-xsm 6 xen-buildfail REGR. vs. 168254 build-i3866 xen-buildfail REGR. vs. 168254 build-i386-xsm6 xen-buildfail REGR. vs. 168254 Tests which did not succeed, but are not blocking: build-amd64-libvirt 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a version targeted for testing: ovmf ad6816c319cdbd927d81e071996a0dea33c86e4a baseline version: ovmf b1b89f9009f2390652e0061bd7b24fc40732bc70 Last test of basis 168254 2022-02-28 10:41:46 Z 35 days Failing since168258 2022-03-01 01:55:31 Z 35 days 278 attempts Testing same since 169165 2022-04-04 19:43:09 Z0 days2 attempts People who touched revisions under test: Abdul Lateef Attar Abdul Lateef Attar via groups.io Abner Chang Akihiko Odaki Anthony PERARD Bob Feng Gerd Hoffmann Guo Dong Guomin Jiang Hao A Wu Hua Ma Huang, Li-Xia Jagadeesh Ujja Jason Jason Lou Ken Lautner Kenneth Lautner Kuo, Ted Laszlo Ersek Li, Zhihao Lixia Huang Lou, Yun Ma, Hua Mara Sophie Grosch Mara Sophie Grosch via groups.io Matt DeVillier Michael Kubacki Min Xu Patrick Rudolph Purna Chandra Rao Bandaru Ray Ni Sami Mujawar Sean Rhodes Sean Rhodes sean@starlabs.systems Sebastien Boeuf Sunny Wang Ted Kuo Wenyi Xie wenyi,xie via groups.io Xiaolu.Jiang Xie, Yuanhao Yi Li Yuanhao Xie Zhihao Li jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 3336 lines long.)
