Re: [PATCH 3/3] xen/MISRA: Remove nonstandard inline keywords
On 2023-11-22 23:20, Andrew Cooper wrote: On 22/11/2023 10:13 pm, Stefano Stabellini wrote: On Wed, 22 Nov 2023, Andrew Cooper wrote: The differences between inline, __inline and __inline__ keywords are a vestigial remnant of older C standards, and in Xen we use inline almost exclusively. Replace __inline and __inline__ with regular inline, and remove their exceptions from the MISRA configuration. Signed-off-by: Andrew Cooper diff --git a/docs/misra/C-language-toolchain.rst b/docs/misra/C-language-toolchain.rst index 2866cb191b1a..b7c2000992ac 100644 --- a/docs/misra/C-language-toolchain.rst +++ b/docs/misra/C-language-toolchain.rst @@ -84,7 +84,7 @@ The table columns are as follows: see Sections "6.48 Alternate Keywords" and "6.47 How to Use Inline Assembly Language in C Code" of GCC_MANUAL. __volatile__: see Sections "6.48 Alternate Keywords" and "6.47.2.1 Volatile" of GCC_MANUAL. - __const__, __inline__, __inline: + __const__: see Section "6.48 Alternate Keywords" of GCC_MANUAL. typeof, __typeof__: see Section "6.7 Referring to a Type with typeof" of GCC_MANUAL. Asking the Bugseng guys as well, do we need to add to C-language-toolchain.rst: inline __attribute__((__always_inline__)) inline __attribute__((__gnu_inline__)) __attribute__ itself is in the list of permitted non-standard tokens, in both files. However, neither file has anything concerning the parameter(s) to the __attribute__, and we do use an awful lot of them. If they want discussing, then that's going to be a lot of work. Just __attribute__ is fine, since we point to Section "6.39 Attribute Syntax" of GCC_MANUAL. which describes the syntax for the token and gives pointers to other relevant sections of the manual. Given that the problem was also present before this patch: Reviewed-by: Stefano Stabellini Thanks. -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [PATCH 3/3] xen/MISRA: Remove nonstandard inline keywords
On 2023-11-22 17:46, Andrew Cooper wrote: On 22/11/2023 4:39 pm, Nicola Vetrini wrote: On 2023-11-22 15:27, Andrew Cooper wrote: The differences between inline, __inline and __inline__ keywords are a vestigial remnant of older C standards, and in Xen we use inline almost exclusively. Replace __inline and __inline__ with regular inline, and remove their exceptions from the MISRA configuration. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Roger Pau Monné CC: Wei Liu CC: Stefano Stabellini CC: Roberto Bagnara CC: Nicola Vetrini CC: Simone Ballarin I'm entirely guessing at the Eclair configuration. --- The configuration changes are ok. One observation below. Thanks. Can I take that as an Ack/Reviewed-by ? I see that Simone already gave one; that should suffice. diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h index 04b8bc18df0e..16d554f2a593 100644 --- a/xen/include/xen/compiler.h +++ b/xen/include/xen/compiler.h @@ -83,7 +82,7 @@ * inline functions not expanded inline get placed in .init.text. */ #include -#define __inline__ __inline__ __init +#define inline inline __init The violation of Rule 20.4 (A macro shall not be defined with the same name as a keyword) is still present due to this macro. I was expecting this to come up. There's a comment half out of context above, but to expand on it, we have a feature in the build system where if you say obj-y += foo.init.o then it gets compiled as normal and then all symbols checked for being in the relevant .init sections. It's a safeguard around init-only code ending up in the runtime image (which is good for other goals of safety). This particular define is necessary to cause out-of-lined static inlines to end up in the right section, without having to invent a new __inline_or_init macro and rewriting half the header files in the project. I think it's going to need a local deviation. It's deliberate, and all we're doing is using the inline keyword to hook in an extra __section() attribute. ~Andrew That's fair. I also agree that an exception for this use of inline can be made. -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
[XEN PATCH v3 1/2] xen/vmap: use ISOLATE_LSB to wrap a violation of Rule 10.1
No functional change. Signed-off-by: Nicola Vetrini Reviewed-by: Stefano Stabellini --- Changes in v2: - Changed macro name Changes in v3: - Changed macro name --- xen/common/vmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/common/vmap.c b/xen/common/vmap.c index 4fd6b3067ec1..330e2ba897d7 100644 --- a/xen/common/vmap.c +++ b/xen/common/vmap.c @@ -53,7 +53,7 @@ static void *vm_alloc(unsigned int nr, unsigned int align, if ( !align ) align = 1; else if ( align & (align - 1) ) -align &= -align; +align = ISOLATE_LSB(align); ASSERT((t >= VMAP_DEFAULT) && (t < VMAP_REGION_NR)); if ( !vm_base[t] ) -- 2.34.1
[XEN PATCH v3 0/2] use the macro ISOLATE_LSB where appropriate
This series replaces two instances of the pattern (x & -x) with the macro ISOLATE_LSB. Nicola Vetrini (2): xen/vmap: use ISOLATE_LSB to wrap a violation of Rule 10.1 xen/iommu: use ISOLATE_LSB to wrap a violation of Rule 10.1 xen/common/vmap.c | 2 +- xen/drivers/passthrough/iommu.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) -- 2.34.1
[XEN PATCH v3 2/2] xen/iommu: use ISOLATE_LSB to wrap a violation of Rule 10.1
No functional change.
Signed-off-by: Nicola Vetrini
Reviewed-by: Stefano Stabellini
Acked-by: Jan Beulich
---
Changes in v2:
- Changed macro name
Changes in v3:
- Changed macro name
---
xen/drivers/passthrough/iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index f9a9f53dbd44..996c31be1284 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -560,7 +560,7 @@ int __init iommu_setup(void)
rc = iommu_hardware_setup();
if ( !rc )
ops = iommu_get_ops();
-if ( ops && (ops->page_sizes & -ops->page_sizes) != PAGE_SIZE )
+if ( ops && (ISOLATE_LSB(ops->page_sizes)) != PAGE_SIZE )
{
printk(XENLOG_ERR "IOMMU: page size mask %lx unsupported\n",
ops->page_sizes);
--
2.34.1
[XEN PATCH v5 0/3] address violations of MISRA C:2012 Rule 10.1
This series contains the leftover patches from [1] with the rename s/ISOLATE_LOW_BIT/ISOLATE_LSB/ applied. All the already committed patches from the aforementioned series are dropped. [1] https://marc.info/?l=xen-devel=169841347803987 Nicola Vetrini (3): arm/bitops: encapsulate violation of MISRA C:2012 Rule 10.1 xen/pdx: amend definition of PDX_GROUP_COUNT x86_64/mm: express macro CNT using ISOLATE_LSB xen/arch/arm/include/asm/bitops.h | 4 ++-- xen/arch/x86/x86_64/mm.c | 12 ++-- xen/include/xen/pdx.h | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) -- 2.34.1
[XEN PATCH v5 2/3] xen/pdx: amend definition of PDX_GROUP_COUNT
The definition of PDX_GROUP_COUNT causes violations of MISRA C:2012 Rule 10.1, therefore the problematic part now uses the ISOLATE_LSB macro, which encapsulates the pattern. Signed-off-by: Nicola Vetrini Reviewed-by: Stefano Stabellini --- Changes in v4: - Changed macro name. Changes in v5: - Changed macro name. --- xen/include/xen/pdx.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h index bd535009ea8f..23f3956db8db 100644 --- a/xen/include/xen/pdx.h +++ b/xen/include/xen/pdx.h @@ -70,7 +70,7 @@ extern unsigned long max_pdx; #define PDX_GROUP_COUNT ((1 << PDX_GROUP_SHIFT) / \ - (sizeof(*frame_table) & -sizeof(*frame_table))) + (ISOLATE_LSB(sizeof(*frame_table extern unsigned long pdx_group_valid[]; /** -- 2.34.1
[XEN PATCH v5 1/3] arm/bitops: encapsulate violation of MISRA C:2012 Rule 10.1
The definitions of ffs{l}? violate Rule 10.1, by using the well-known
pattern (x & -x); its usage is wrapped by the ISOLATE_LSB macro.
No functional change.
Signed-off-by: Nicola Vetrini
Reviewed-by: Stefano Stabellini
---
Changes in v4:
- Changed macro name.
Changes in v5:
- Changed macro name.
---
xen/arch/arm/include/asm/bitops.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/include/asm/bitops.h
b/xen/arch/arm/include/asm/bitops.h
index 71ae14cab355..673a8abae5bf 100644
--- a/xen/arch/arm/include/asm/bitops.h
+++ b/xen/arch/arm/include/asm/bitops.h
@@ -155,8 +155,8 @@ static inline int fls(unsigned int x)
}
-#define ffs(x) ({ unsigned int __t = (x); fls(__t & -__t); })
-#define ffsl(x) ({ unsigned long __t = (x); flsl(__t & -__t); })
+#define ffs(x) ({ unsigned int __t = (x); fls(ISOLATE_LSB(__t)); })
+#define ffsl(x) ({ unsigned long __t = (x); flsl(ISOLATE_LSB(__t)); })
/**
* find_first_set_bit - find the first set bit in @word
--
2.34.1
[XEN PATCH v5 3/3] x86_64/mm: express macro CNT using ISOLATE_LSB
The various definitions of macro CNT (and the related BUILD_BUG_ON) can be rewritten using ISOLATE_LSB, encapsulating a violation of MISRA C:2012 Rule 10.1. Signed-off-by: Nicola Vetrini Reviewed-by: Stefano Stabellini Acked-by: Jan Beulich --- Changes in v4: - Changed macro name Changes in v5: - Add A-by - Changed macro name. --- xen/arch/x86/x86_64/mm.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c index c3ebb777144a..b2a280fba369 100644 --- a/xen/arch/x86/x86_64/mm.c +++ b/xen/arch/x86/x86_64/mm.c @@ -351,9 +351,9 @@ static int setup_compat_m2p_table(struct mem_hotadd_info *info) ~((1UL << (L2_PAGETABLE_SHIFT - 2)) - 1) ); #define MFN(x) (((x) << L2_PAGETABLE_SHIFT) / sizeof(unsigned int)) -#define CNT ((sizeof(*frame_table) & -sizeof(*frame_table)) / \ +#define CNT (ISOLATE_LSB(sizeof(*frame_table)) / \ sizeof(*compat_machine_to_phys_mapping)) -BUILD_BUG_ON((sizeof(*frame_table) & -sizeof(*frame_table)) % \ +BUILD_BUG_ON(ISOLATE_LSB(sizeof(*frame_table)) % \ sizeof(*compat_machine_to_phys_mapping)); for ( i = smap; i < emap; i += (1UL << (L2_PAGETABLE_SHIFT - 2)) ) @@ -410,10 +410,10 @@ static int setup_m2p_table(struct mem_hotadd_info *info) va = RO_MPT_VIRT_START + smap * sizeof(*machine_to_phys_mapping); #define MFN(x) (((x) << L2_PAGETABLE_SHIFT) / sizeof(unsigned long)) -#define CNT ((sizeof(*frame_table) & -sizeof(*frame_table)) / \ +#define CNT (ISOLATE_LSB(sizeof(*frame_table)) / \ sizeof(*machine_to_phys_mapping)) -BUILD_BUG_ON((sizeof(*frame_table) & -sizeof(*frame_table)) % \ +BUILD_BUG_ON(ISOLATE_LSB(sizeof(*frame_table)) % \ sizeof(*machine_to_phys_mapping)); i = smap; @@ -539,7 +539,7 @@ void __init paging_init(void) mpt_size = (max_page * BYTES_PER_LONG) + (1UL << L2_PAGETABLE_SHIFT) - 1; mpt_size &= ~((1UL << L2_PAGETABLE_SHIFT) - 1UL); #define MFN(x) (((x) << L2_PAGETABLE_SHIFT) / sizeof(unsigned long)) -#define CNT ((sizeof(*frame_table) & -sizeof(*frame_table)) / \ +#define CNT (ISOLATE_LSB(sizeof(*frame_table)) / \ sizeof(*machine_to_phys_mapping)) BUILD_BUG_ON((sizeof(*frame_table) & ~sizeof(*frame_table)) % \ sizeof(*machine_to_phys_mapping)); @@ -666,7 +666,7 @@ void __init paging_init(void) mpt_size = 0; #define MFN(x) (((x) << L2_PAGETABLE_SHIFT) / sizeof(unsigned int)) -#define CNT ((sizeof(*frame_table) & -sizeof(*frame_table)) / \ +#define CNT (ISOLATE_LSB(sizeof(*frame_table)) / \ sizeof(*compat_machine_to_phys_mapping)) BUILD_BUG_ON((sizeof(*frame_table) & ~sizeof(*frame_table)) % \ sizeof(*compat_machine_to_phys_mapping)); -- 2.34.1
Re: [PATCH v2] x86/cpuid: enumerate and expose PREFETCHIT{0,1}
On 22.11.2023 13:25, Andrew Cooper wrote: > On 22/11/2023 7:43 am, Jan Beulich wrote: >> --- a/xen/tools/gen-cpuid.py >> +++ b/xen/tools/gen-cpuid.py >> @@ -274,7 +274,7 @@ def crunch_numbers(state): >> # superpages, PCID and PKU are only available in 4 level paging. >> # NO_LMSL indicates the absense of Long Mode Segment Limits, which >> # have been dropped in hardware. >> -LM: [CX16, PCID, LAHF_LM, PAGE1GB, PKU, NO_LMSL], >> +LM: [CX16, PCID, LAHF_LM, PAGE1GB, PKU, NO_LMSL, PREFETCHI], > > I know this is what the ISE says, but I'm not sure it's a legitimate > dependency. > > It is an implementation detail that Intel depend on a RIP-relative > address, but there are no architectural reason why other implementations > couldn't make this work in 32bit too. > > The worst that happens without this dependency is that 32bit-only VMs > see a hint bit about certain NOPs having uarch side effects, which > they'll ignore for other reasons. I'm okay either way. Adding the dependency was the only reason to have a v2 ... > So I recommend dropping the dependency. If you're happy, then > Reviewed-by: Andrew Cooper Thanks. Jan
Re: [PATCH v2 0/2] tools: add two local .gitignore files
On 22.11.2023 14:02, Juergen Gross wrote: > After a new build on my system (OpenSUSE Leap 15.5) "git status" will > print out: > > Untracked files: > (use "git add ..." to include in what will be committed) > tools/pygrub/pygrub.egg-info/ > tools/python/xen.egg-info/ > > This small patch series fixes that by adding the related entries to > local .gitignore files, while moving the existing global entries for > those directories to them. > > Changes in V2: > - use "/dir/" as matching pattern for directories > > Juergen Gross (2): > tools/pygrub: add .gitignore file > tools/python: add .gitignore file Reviewed-by: Jan Beulich
Re: [linux-linus test] 183794: regressions - FAIL
On 23.11.23 00:07, Stefano Stabellini wrote: On Wed, 22 Nov 2023, Juergen Gross wrote: On 22.11.23 04:07, Stefano Stabellini wrote: On Mon, 20 Nov 2023, Stefano Stabellini wrote: On Mon, 20 Nov 2023, Juergen Gross wrote: On 20.11.23 03:21, osstest service owner wrote: flight 183794 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/183794/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-arm64-arm64-examine 8 reboot fail REGR. vs. 183766 I'm seeing the following in the serial log: Nov 20 00:25:41.586712 [0.567318] kernel BUG at arch/arm64/xen/../../arm/xen/enlighten.c:164! Nov 20 00:25:41.598711 [0.574002] Internal error: Oops - BUG: f2000800 [#1] PREEMPT SMP The related source code lines in the kernel are: err = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_info, xen_vcpu_nr(cpu), ); BUG_ON(err); I suspect commit 20f3b8eafe0ba to be the culprit. Stefano, could you please have a look? The good news and bad news is that I cannot repro this neither with nor without CONFIG_UNMAP_KERNEL_AT_EL0. I looked at commit 20f3b8eafe0ba but I cannot see anything wrong with it. Looking at the register dump, from: x0 : fffa I am guessing the error was -ENXIO which is returned from map_guest_area in Xen. Could it be that the struct is crossing a page boundary? Or that it is not 64-bit aligned? Do we need to do something like the following? diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index 9afdc4c4a5dc..5326070c5dc0 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -484,7 +485,7 @@ static int __init xen_guest_init(void) * for secondary CPUs as they are brought up. * For uniformity we use VCPUOP_register_vcpu_info even on cpu0. */ - xen_vcpu_info = alloc_percpu(struct vcpu_info); + xen_vcpu_info = __alloc_percpu(struct vcpu_info, PAGE_SIZE); if (xen_vcpu_info == NULL) return -ENOMEM; May I suggest to use a smaller alignment? What about: 1 << fls(sizeof(struct vcpu_info) - 1) See below --- [PATCH] arm/xen: fix xen_vcpu_info allocation alignment xen_vcpu_info is a percpu area than needs to be mapped by Xen. Currently, it could cross a page boundary resulting in Xen being unable to map it: [0.567318] kernel BUG at arch/arm64/xen/../../arm/xen/enlighten.c:164! [0.574002] Internal error: Oops - BUG: f2000800 [#1] PREEMPT SMP Fix the issue by using __alloc_percpu and requesting alignment for the memory allocation. Signed-off-by: Stefano Stabellini diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index 9afdc4c4a5dc..09eb74a07dfc 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -484,7 +484,8 @@ static int __init xen_guest_init(void) * for secondary CPUs as they are brought up. * For uniformity we use VCPUOP_register_vcpu_info even on cpu0. */ - xen_vcpu_info = alloc_percpu(struct vcpu_info); + xen_vcpu_info = __alloc_percpu(sizeof(struct vcpu_info), + 1 << fls(sizeof(struct vcpu_info) - 1)); Nit: one tab less, please (can be fixed while committing). if (xen_vcpu_info == NULL) return -ENOMEM; Reviewed-by: Juergen Gross Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 4/5] arm/efi: Simplify efi_arch_handle_cmdline()
Hi, > On Nov 23, 2023, at 12:20, Henry Wang wrote: > > Hi, > >> On Nov 23, 2023, at 02:03, Andrew Cooper wrote: >> >> On 22/11/2023 3:49 pm, Luca Fancellu wrote: >>> On 21 Nov 2023, at 20:41, Andrew Cooper wrote: On 21/11/2023 8:33 pm, Luca Fancellu wrote: > + CC henry > >> On 21 Nov 2023, at 20:15, Andrew Cooper >> wrote: >> >> -Wwrite-strings is unhappy with assigning "xen" to a mutable pointer, >> but this >> logic looks incorrect. It was inherited from the x86 side, where the >> logic >> was redundant and has now been removed. >> >> In the ARM case it inserts the image name into "xen,xen-bootargs" and >> there is >> no logic at all to strip this before parsing it as the command line. >> >> The absence of any logic to strip an image name suggests that it >> shouldn't >> exist there, or having a Xen image named e.g. "hmp-unsafe" in the >> filesystem >> is going to lead to some unexpected behaviour on boot. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper >> --- >> v2: >> * New. >> >> I'm afraid that all of this reasoning is based on reading the source >> code. I >> don't have any way to try this out in a real ARM UEFI environment. > I will test this one tomorrow on an arm board >>> I confirm that booting though UEFI on an arm board works >>> >>> Reviewed-by: Luca Fancellu >>> Tested-by: Luca Fancellu >> >> Thanks, and you confirmed that the first cmdline parameter is still usable? > > Today I tried this series on an N1SDP board using UEFI boot. I had a device > tree with > xen,xen-bootargs = "console=dtuart dtuart=serial0:115200n8 noreboot > dom0_mem=1024M bootscrub=0 iommu=no"; > > Xen can be successfully boot on the board with the series applied, and I got > ``` > (XEN) Command line: console=dtuart dtuart=serial0:115200n8 noreboot > dom0_mem=1024M bootscrub=0 iommu=no > […] > ``` > > Also I can interact with the board: > ``` > n1sdp login: root > root@n1sdp:~# ^C > root@n1sdp:~# ^C > root@n1sdp:~# ^C > ``` > > So I think the first cmdline parameter is still usable. I will wait for Luca > to confirm on his > side as I believe he used a different board in his test. Also I tried to switch the order of the cmdline parameter in the device tree, for example use: xen,xen-bootargs = “dom0_mem=512M console=dtuart dtuart=serial0:115200n8 noreboot bootscrub=0 iommu=no”; This time I had: ``` [...] (XEN) Command line: dom0_mem=512M console=dtuart dtuart=serial0:115200n8 noreboot bootscrub=0 iommu=no […] (XEN) Allocating 1:1 mappings totalling 512MB for dom0: […] ``` So I think we are fine. Kind regards, Henry > > Tested-by: Henry Wang > > Kind regards, > Henry > >> >> ~Andrew >
Re: [PATCH v2 4/5] arm/efi: Simplify efi_arch_handle_cmdline()
Hi, > On Nov 23, 2023, at 02:03, Andrew Cooper wrote: > > On 22/11/2023 3:49 pm, Luca Fancellu wrote: >> >>> On 21 Nov 2023, at 20:41, Andrew Cooper wrote: >>> >>> On 21/11/2023 8:33 pm, Luca Fancellu wrote: + CC henry > On 21 Nov 2023, at 20:15, Andrew Cooper wrote: > > -Wwrite-strings is unhappy with assigning "xen" to a mutable pointer, but > this > logic looks incorrect. It was inherited from the x86 side, where the > logic > was redundant and has now been removed. > > In the ARM case it inserts the image name into "xen,xen-bootargs" and > there is > no logic at all to strip this before parsing it as the command line. > > The absence of any logic to strip an image name suggests that it shouldn't > exist there, or having a Xen image named e.g. "hmp-unsafe" in the > filesystem > is going to lead to some unexpected behaviour on boot. > > No functional change. > > Signed-off-by: Andrew Cooper > --- > v2: > * New. > > I'm afraid that all of this reasoning is based on reading the source > code. I > don't have any way to try this out in a real ARM UEFI environment. I will test this one tomorrow on an arm board >> I confirm that booting though UEFI on an arm board works >> >> Reviewed-by: Luca Fancellu >> Tested-by: Luca Fancellu > > Thanks, and you confirmed that the first cmdline parameter is still usable? Today I tried this series on an N1SDP board using UEFI boot. I had a device tree with xen,xen-bootargs = "console=dtuart dtuart=serial0:115200n8 noreboot dom0_mem=1024M bootscrub=0 iommu=no"; Xen can be successfully boot on the board with the series applied, and I got ``` (XEN) Command line: console=dtuart dtuart=serial0:115200n8 noreboot dom0_mem=1024M bootscrub=0 iommu=no […] ``` Also I can interact with the board: ``` n1sdp login: root root@n1sdp:~# ^C root@n1sdp:~# ^C root@n1sdp:~# ^C ``` So I think the first cmdline parameter is still usable. I will wait for Luca to confirm on his side as I believe he used a different board in his test. Tested-by: Henry Wang Kind regards, Henry > > ~Andrew
Re: [PATCH v2 3/6] xen: xenstore: add possibility to preserve owner
Hi David,
David Woodhouse writes:
> [[S/MIME Signed Part:Undecided]]
> On Tue, 2023-11-21 at 22:10 +, Volodymyr Babchuk wrote:
>>
>> --- a/hw/xen/xen-operations.c
>> +++ b/hw/xen/xen-operations.c
>> @@ -300,6 +300,18 @@ static bool libxenstore_create(struct qemu_xs_handle
>> *h, xs_transaction_t t,
>> return false;
>> }
>>
>> + if (owner == XS_PRESERVE_OWNER) {
>> + struct xs_permissions *tmp;
>> + unsigned int num;
>> +
>> + tmp = xs_get_permissions(h->xsh, t, path, );
>> + if (tmp == NULL) {
>> + return false;
>> + }
>> + perms_list[0].id = tmp[0].id;
>> + free(tmp);
>> + }
>> +
>> return xs_set_permissions(h->xsh, t, path, perms_list,
>> ARRAY_SIZE(perms_list));
>> }
>
> If the existing transaction is XBT_NULL I think you want to create a
> new transaction for it, don't you?
I must say that your comment is valid even without my
changes. xenstore_mkdir() calls qemu_xen_xs_create, providing just plain
"0" (not even XBT_NULL) as a transaction, but actual xenstore interface
implementation, like xs_be_create(), issue multiple calls to lower
layer, passing "t" downwards. For example, xs_be_create() calls
xs_impl_read, xs_impl_write and xs_impl_set_perms(). If called from
xesntore_mkdir(), those three operations will be non-atomic. I don't
know if this can lead to a problem, because apparently it was so for a
long time...
--
WBR, Volodymyr
Re: [PATCH v2 6/6] xen_arm: Add virtual PCIe host bridge support
Hi Vikram, Vikram Garhwal writes: > Hi Volodymyr, > Thank you sharing this patch. I have few comments below > On Wed, Nov 22, 2023 at 02:39:46PM -0800, Stefano Stabellini wrote: >> +Vikram >> >> On Tue, 21 Nov 2023, Volodymyr Babchuk wrote: >> > From: Oleksandr Tyshchenko >> > >> > The bridge is needed for virtio-pci support, as QEMU can emulate the >> > whole bridge with any virtio-pci devices connected to it. >> > >> > This patch provides a flexible way to configure PCIe brige resources >> > with xenstore. We made this for several reasons: >> > >> > - We don't want to clash with vPCI devices, so we need information >> > from Xen toolstack on which PCI bus to use. >> > - The guest memory layout that describes these resources is not stable >> > and may vary between guests, so we cannot rely on static resources >> > to be always the same for both ends. >> > - Also the device-models which run in different domains and serve >> > virtio-pci devices for the same guest should use different host >> > bridge resources for Xen to distinguish. The rule for the guest >> > device-tree generation is one PCI host bridge per backend domain. >> > >> > Signed-off-by: Oleksandr Tyshchenko >> > Signed-off-by: Volodymyr Babchuk >> >> There is still a discussion ongoing on xen-devel if / how to register a >> PCI Root Complex or individual BDFs. In the meantime a couple of >> comments. >> >> Typically emulated devices are configured in QEMU via QEMU command line >> parameters, not xenstore. If you are running QEMU in a domU (instead of >> Dom0) you can always read config parameters from xenstore from a wrapper >> bash script (using xenstore-read) and then pass the right command line >> options to QEMU. >> >> If you need help in adding new QEMU command line options, Vikram (CCed) >> can help. >> >> > I agree with Stefano here. Setting properties would be better and easier. Okay, I'll look at this. > I have one questions here: > 1. If there are multiple QEMU backends, which means xen tools will end up > creating lot of entries in xenstore and we need to remove those xenstore > entries when backend goes away. Is this already handled in Xen tools? Well, this is not a classic PV backend, so common code in Xen Tools does not handle those entries. I am not sure if tools remove entries right now, because I am not the original author. But we definitely will remove them in the final version of patches. -- WBR, Volodymyr
Re: [PATCH v2 4/6] xen_pvdev: Do not assume Dom0 when creating a directory
Hi, Volodymyr Babchuk writes: > Hi Stefano, > > Stefano Stabellini writes: > >> On Wed, 22 Nov 2023, David Woodhouse wrote: >>> On Wed, 2023-11-22 at 15:09 -0800, Stefano Stabellini wrote: >>> > On Wed, 22 Nov 2023, David Woodhouse wrote: >>> > > On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote: >>> > > > On Wed, 22 Nov 2023, Paul Durrant wrote: >>> > > > > On 21/11/2023 22:10, Volodymyr Babchuk wrote: >>> > > > > > From: Oleksandr Tyshchenko >>> > > > > > >>> > > > > > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to >>> > > > > > inherit the owner of the directory. >>> > > > > >>> > > > > Ah... so that's why the previous patch is there. >>> > > > > >>> > > > > This is not the right way to fix it. The QEMU Xen support is >>> > > > > *assuming* that >>> > > > > QEMU is either running in, or emulating, dom0. In the emulation >>> > > > > case this is >>> > > > > probably fine, but the 'real Xen' case it should be using the >>> > > > > correct domid >>> > > > > for node creation. I guess this could either be supplied on the >>> > > > > command line >>> > > > > or discerned by reading the local domain 'domid' node. >>> > > > >>> > > > yes, it should be passed as command line option to QEMU >>> > > >>> > > I'm not sure I like the idea of a command line option for something >>> > > which QEMU could discover for itself. >>> > >>> > That's fine too. I meant to say "yes, as far as I know the toolstack >>> > passes the domid to QEMU as a command line option today". >>> >>> The -xen-domid argument on the QEMU command line today is the *guest* >>> domain ID, not the domain ID in which QEMU itself is running. >>> >>> Or were you thinking of something different? >> >> Ops, you are right and I understand your comment better now. The backend >> domid is not on the command line but it should be discoverable (on >> xenstore if I remember right). > > Yes, it is just "~/domid". I'll add a function that reads it. Just a quick question to QEMU folks: is it better to add a global variable where we will store own Domain ID or it will be okay to read domid from Xenstore every time we need it? If global variable variant is better, what is proffered place to define this variable? system/globals.c ? -- WBR, Volodymyr
Re: [PATCH v2 1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it
On Wed, 2023-11-22 at 23:49 +, Volodymyr Babchuk wrote: > > I can just pull it from this link, if you don't mind. Please do; thank you! smime.p7s Description: S/MIME cryptographic signature Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
Re: [PATCH v2 1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it
Hi David, "Woodhouse, David" writes: > On Wed, 2023-11-22 at 17:05 +, Paul Durrant wrote: >> On 21/11/2023 22:10, Volodymyr Babchuk wrote: >> > From: David Woodhouse >> > >> > This allows a XenDevice implementation to know whether it was created >> > by QEMU, or merely discovered in XenStore after the toolstack created >> > it. This will allow us to create frontend/backend nodes only when we >> > should, rather than unconditionally attempting to overwrite them from >> > a driver domain which doesn't have privileges to do so. >> > >> > As an added benefit, it also means we no longer have to call the >> > xen_backend_set_device() function from the device models immediately >> > after calling qdev_realize_and_unref(). Even though we could make >> > the argument that it's safe to do so, and the pointer to the unreffed >> > device *will* actually still be valid, it still made my skin itch to >> > look at it. >> > >> > Signed-off-by: David Woodhouse >> > --- >> > hw/block/xen-block.c | 3 +-- >> > hw/char/xen_console.c | 2 +- >> > hw/net/xen_nic.c | 2 +- >> > hw/xen/xen-bus.c | 4 >> > include/hw/xen/xen-backend.h | 2 -- >> > include/hw/xen/xen-bus.h | 2 ++ >> > 6 files changed, 9 insertions(+), 6 deletions(-) >> > >> >> Actually, I think you should probably update >> xen_backend_try_device_destroy() in this patch too. It currently uses >> xen_backend_list_find() to check whether the specified XenDevice has an >> associated XenBackendInstance. > > I think I did that in > https://git.infradead.org/users/dwmw2/qemu.git/commitdiff/94f1b474478ce0ede > but didn't get round to sending it out again because of travel. I can just pull it from this link, if you don't mind. -- WBR, Volodymyr
Re: [PATCH v2 4/6] xen_pvdev: Do not assume Dom0 when creating a directory
Hi Stefano, Stefano Stabellini writes: > On Wed, 22 Nov 2023, David Woodhouse wrote: >> On Wed, 2023-11-22 at 15:09 -0800, Stefano Stabellini wrote: >> > On Wed, 22 Nov 2023, David Woodhouse wrote: >> > > On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote: >> > > > On Wed, 22 Nov 2023, Paul Durrant wrote: >> > > > > On 21/11/2023 22:10, Volodymyr Babchuk wrote: >> > > > > > From: Oleksandr Tyshchenko >> > > > > > >> > > > > > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to >> > > > > > inherit the owner of the directory. >> > > > > >> > > > > Ah... so that's why the previous patch is there. >> > > > > >> > > > > This is not the right way to fix it. The QEMU Xen support is >> > > > > *assuming* that >> > > > > QEMU is either running in, or emulating, dom0. In the emulation case >> > > > > this is >> > > > > probably fine, but the 'real Xen' case it should be using the >> > > > > correct domid >> > > > > for node creation. I guess this could either be supplied on the >> > > > > command line >> > > > > or discerned by reading the local domain 'domid' node. >> > > > >> > > > yes, it should be passed as command line option to QEMU >> > > >> > > I'm not sure I like the idea of a command line option for something >> > > which QEMU could discover for itself. >> > >> > That's fine too. I meant to say "yes, as far as I know the toolstack >> > passes the domid to QEMU as a command line option today". >> >> The -xen-domid argument on the QEMU command line today is the *guest* >> domain ID, not the domain ID in which QEMU itself is running. >> >> Or were you thinking of something different? > > Ops, you are right and I understand your comment better now. The backend > domid is not on the command line but it should be discoverable (on > xenstore if I remember right). Yes, it is just "~/domid". I'll add a function that reads it. -- WBR, Volodymyr
Re: [PATCH v2 6/6] xen_arm: Add virtual PCIe host bridge support
Hi Volodymyr,
Thank you sharing this patch. I have few comments below
On Wed, Nov 22, 2023 at 02:39:46PM -0800, Stefano Stabellini wrote:
> +Vikram
>
> On Tue, 21 Nov 2023, Volodymyr Babchuk wrote:
> > From: Oleksandr Tyshchenko
> >
> > The bridge is needed for virtio-pci support, as QEMU can emulate the
> > whole bridge with any virtio-pci devices connected to it.
> >
> > This patch provides a flexible way to configure PCIe brige resources
> > with xenstore. We made this for several reasons:
> >
> > - We don't want to clash with vPCI devices, so we need information
> > from Xen toolstack on which PCI bus to use.
> > - The guest memory layout that describes these resources is not stable
> > and may vary between guests, so we cannot rely on static resources
> > to be always the same for both ends.
> > - Also the device-models which run in different domains and serve
> > virtio-pci devices for the same guest should use different host
> > bridge resources for Xen to distinguish. The rule for the guest
> > device-tree generation is one PCI host bridge per backend domain.
> >
> > Signed-off-by: Oleksandr Tyshchenko
> > Signed-off-by: Volodymyr Babchuk
>
> There is still a discussion ongoing on xen-devel if / how to register a
> PCI Root Complex or individual BDFs. In the meantime a couple of
> comments.
>
> Typically emulated devices are configured in QEMU via QEMU command line
> parameters, not xenstore. If you are running QEMU in a domU (instead of
> Dom0) you can always read config parameters from xenstore from a wrapper
> bash script (using xenstore-read) and then pass the right command line
> options to QEMU.
>
> If you need help in adding new QEMU command line options, Vikram (CCed)
> can help.
>
>
I agree with Stefano here. Setting properties would be better and easier.
I have one questions here:
1. If there are multiple QEMU backends, which means xen tools will end up
creating lot of entries in xenstore and we need to remove those xenstore
entries when backend goes away. Is this already handled in Xen tools?
Regards,
Vikram
> > ---
> >
> > Changes from v1:
> >
> > - Renamed virtio_pci_host to pcie_host entries in XenStore, because
> > there is nothing specific to virtio-pci: any PCI device can be
> > emulated via this newly created bridge.
> > ---
> > hw/arm/xen_arm.c| 186
> > hw/xen/xen-hvm-common.c | 9 +-
> > include/hw/xen/xen_native.h | 8 +-
> > 3 files changed, 200 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
> > index b9c3ae14b6..d506d55d0f 100644
> > --- a/hw/arm/xen_arm.c
> > +++ b/hw/arm/xen_arm.c
> > @@ -22,6 +22,7 @@
> > */
> >
> > #include "qemu/osdep.h"
> > +#include "qemu/cutils.h"
> > #include "qemu/error-report.h"
> > #include "qapi/qapi-commands-migration.h"
> > #include "qapi/visitor.h"
> > @@ -34,6 +35,9 @@
> > #include "hw/xen/xen-hvm-common.h"
> > #include "sysemu/tpm.h"
> > #include "hw/xen/arch_hvm.h"
> > +#include "exec/address-spaces.h"
> > +#include "hw/pci-host/gpex.h"
> > +#include "hw/virtio/virtio-pci.h"
> >
> > #define TYPE_XEN_ARM MACHINE_TYPE_NAME("xenpvh")
> > OBJECT_DECLARE_SIMPLE_TYPE(XenArmState, XEN_ARM)
> > @@ -58,6 +62,11 @@ struct XenArmState {
> > struct {
> > uint64_t tpm_base_addr;
> > } cfg;
> > +
> > +MemMapEntry pcie_mmio;
> > +MemMapEntry pcie_ecam;
> > +MemMapEntry pcie_mmio_high;
> > +int pcie_irq;
> > };
> >
> > static MemoryRegion ram_lo, ram_hi;
> > @@ -73,6 +82,7 @@ static MemoryRegion ram_lo, ram_hi;
> > #define NR_VIRTIO_MMIO_DEVICES \
> > (GUEST_VIRTIO_MMIO_SPI_LAST - GUEST_VIRTIO_MMIO_SPI_FIRST)
> >
> > +/* TODO It should be xendevicemodel_set_pci_intx_level() for PCI
> > interrupts. */
> > static void xen_set_irq(void *opaque, int irq, int level)
> > {
> > if (xendevicemodel_set_irq_level(xen_dmod, xen_domid, irq, level)) {
> > @@ -129,6 +139,176 @@ static void xen_init_ram(MachineState *machine)
> > }
> > }
> >
> > +static void xen_create_pcie(XenArmState *xam)
> > +{
> > +MemoryRegion *mmio_alias, *mmio_alias_high, *mmio_reg;
> > +MemoryRegion *ecam_alias, *ecam_reg;
> > +DeviceState *dev;
> > +int i;
> > +
> > +dev = qdev_new(TYPE_GPEX_HOST);
> > +sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
> > +
> > +/* Map ECAM space */
> > +ecam_alias = g_new0(MemoryRegion, 1);
> > +ecam_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> > +memory_region_init_alias(ecam_alias, OBJECT(dev), "pcie-ecam",
> > + ecam_reg, 0, xam->pcie_ecam.size);
> > +memory_region_add_subregion(get_system_memory(), xam->pcie_ecam.base,
> > +ecam_alias);
> > +
> > +/* Map the MMIO space */
> > +mmio_alias = g_new0(MemoryRegion, 1);
> > +mmio_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
> > +
[xen-unstable test] 183821: regressions - FAIL
flight 183821 xen-unstable real [real] flight 183829 xen-unstable real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/183821/ http://logs.test-lab.xenproject.org/osstest/logs/183829/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-dom0pvh-xl-intel 18 guest-localmigrate fail REGR. vs. 183807 test-amd64-amd64-qemuu-nested-intel 20 debian-hvm-install/l1/l2 fail REGR. vs. 183807 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 183807 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 183807 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 183807 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 183807 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 183807 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 183807 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 183807 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 183807 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 183807 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 183807 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 183807 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 183807 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-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-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-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-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 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 version targeted for testing: xen 820ee3ec4dd5679715bd49a1d12b81cb1502260c baseline version: xen
Re: [PATCH v2 4/6] xen_pvdev: Do not assume Dom0 when creating a directory
On Wed, 22 Nov 2023, David Woodhouse wrote: > On Wed, 2023-11-22 at 15:09 -0800, Stefano Stabellini wrote: > > On Wed, 22 Nov 2023, David Woodhouse wrote: > > > On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote: > > > > On Wed, 22 Nov 2023, Paul Durrant wrote: > > > > > On 21/11/2023 22:10, Volodymyr Babchuk wrote: > > > > > > From: Oleksandr Tyshchenko > > > > > > > > > > > > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to > > > > > > inherit the owner of the directory. > > > > > > > > > > Ah... so that's why the previous patch is there. > > > > > > > > > > This is not the right way to fix it. The QEMU Xen support is > > > > > *assuming* that > > > > > QEMU is either running in, or emulating, dom0. In the emulation case > > > > > this is > > > > > probably fine, but the 'real Xen' case it should be using the correct > > > > > domid > > > > > for node creation. I guess this could either be supplied on the > > > > > command line > > > > > or discerned by reading the local domain 'domid' node. > > > > > > > > yes, it should be passed as command line option to QEMU > > > > > > I'm not sure I like the idea of a command line option for something > > > which QEMU could discover for itself. > > > > That's fine too. I meant to say "yes, as far as I know the toolstack > > passes the domid to QEMU as a command line option today". > > The -xen-domid argument on the QEMU command line today is the *guest* > domain ID, not the domain ID in which QEMU itself is running. > > Or were you thinking of something different? Ops, you are right and I understand your comment better now. The backend domid is not on the command line but it should be discoverable (on xenstore if I remember right).
Re: [PATCH v2 2/6] xen: backends: touch some XenStore nodes only if device...
On Wed, 2023-11-22 at 22:49 +, Volodymyr Babchuk wrote: > > > On 21/11/23 23:10, Volodymyr Babchuk wrote: > > > was created by QEMU > > > > Please do not split lines between subject and content. Rewrite the > > full line. Preferably restrict the subject to 72 chars. > > I tried to come with shorter description, but failed. I'll rewrite it in > the next version. Even if you just put those last four words *into* the subject, it's only 75 characters once the leaving [PATCH...] is stripped. xen: backends: touch some XenStore nodes only if device was created by QEMU And this would be 9 characters fewer: xen: backends: don't overwrite XenStore nodes created by toolstack smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v2 4/6] xen_pvdev: Do not assume Dom0 when creating a directory
On Wed, 2023-11-22 at 15:09 -0800, Stefano Stabellini wrote: > On Wed, 22 Nov 2023, David Woodhouse wrote: > > On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote: > > > On Wed, 22 Nov 2023, Paul Durrant wrote: > > > > On 21/11/2023 22:10, Volodymyr Babchuk wrote: > > > > > From: Oleksandr Tyshchenko > > > > > > > > > > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to > > > > > inherit the owner of the directory. > > > > > > > > Ah... so that's why the previous patch is there. > > > > > > > > This is not the right way to fix it. The QEMU Xen support is *assuming* > > > > that > > > > QEMU is either running in, or emulating, dom0. In the emulation case > > > > this is > > > > probably fine, but the 'real Xen' case it should be using the correct > > > > domid > > > > for node creation. I guess this could either be supplied on the command > > > > line > > > > or discerned by reading the local domain 'domid' node. > > > > > > yes, it should be passed as command line option to QEMU > > > > I'm not sure I like the idea of a command line option for something > > which QEMU could discover for itself. > > That's fine too. I meant to say "yes, as far as I know the toolstack > passes the domid to QEMU as a command line option today". The -xen-domid argument on the QEMU command line today is the *guest* domain ID, not the domain ID in which QEMU itself is running. Or were you thinking of something different? smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v2 4/6] xen_pvdev: Do not assume Dom0 when creating a directory
On Wed, 22 Nov 2023, David Woodhouse wrote: > On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote: > > On Wed, 22 Nov 2023, Paul Durrant wrote: > > > On 21/11/2023 22:10, Volodymyr Babchuk wrote: > > > > From: Oleksandr Tyshchenko > > > > > > > > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to > > > > inherit the owner of the directory. > > > > > > Ah... so that's why the previous patch is there. > > > > > > This is not the right way to fix it. The QEMU Xen support is *assuming* > > > that > > > QEMU is either running in, or emulating, dom0. In the emulation case this > > > is > > > probably fine, but the 'real Xen' case it should be using the correct > > > domid > > > for node creation. I guess this could either be supplied on the command > > > line > > > or discerned by reading the local domain 'domid' node. > > > > yes, it should be passed as command line option to QEMU > > I'm not sure I like the idea of a command line option for something > which QEMU could discover for itself. That's fine too. I meant to say "yes, as far as I know the toolstack passes the domid to QEMU as a command line option today".
Re: [linux-linus test] 183794: regressions - FAIL
On Wed, 22 Nov 2023, Juergen Gross wrote: > On 22.11.23 04:07, Stefano Stabellini wrote: > > On Mon, 20 Nov 2023, Stefano Stabellini wrote: > > > On Mon, 20 Nov 2023, Juergen Gross wrote: > > > > On 20.11.23 03:21, osstest service owner wrote: > > > > > flight 183794 linux-linus real [real] > > > > > http://logs.test-lab.xenproject.org/osstest/logs/183794/ > > > > > > > > > > Regressions :-( > > > > > > > > > > Tests which did not succeed and are blocking, > > > > > including tests which could not be run: > > > > >test-arm64-arm64-examine 8 reboot fail REGR. > > > > > vs. > > > > > 183766 > > > > > > > > I'm seeing the following in the serial log: > > > > > > > > Nov 20 00:25:41.586712 [0.567318] kernel BUG at > > > > arch/arm64/xen/../../arm/xen/enlighten.c:164! > > > > Nov 20 00:25:41.598711 [0.574002] Internal error: Oops - BUG: > > > > f2000800 [#1] PREEMPT SMP > > > > > > > > The related source code lines in the kernel are: > > > > > > > > err = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_info, > > > > xen_vcpu_nr(cpu), > > > > ); > > > > BUG_ON(err); > > > > > > > > I suspect commit 20f3b8eafe0ba to be the culprit. > > > > > > > > Stefano, could you please have a look? > > > > The good news and bad news is that I cannot repro this neither with nor > > without CONFIG_UNMAP_KERNEL_AT_EL0. I looked at commit 20f3b8eafe0ba but > > I cannot see anything wrong with it. Looking at the register dump, from: > > > > x0 : fffa > > > > I am guessing the error was -ENXIO which is returned from map_guest_area > > in Xen. > > > > Could it be that the struct is crossing a page boundary? Or that it is > > not 64-bit aligned? Do we need to do something like the following? > > > > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c > > index 9afdc4c4a5dc..5326070c5dc0 100644 > > --- a/arch/arm/xen/enlighten.c > > +++ b/arch/arm/xen/enlighten.c > > @@ -484,7 +485,7 @@ static int __init xen_guest_init(void) > > * for secondary CPUs as they are brought up. > > * For uniformity we use VCPUOP_register_vcpu_info even on cpu0. > > */ > > - xen_vcpu_info = alloc_percpu(struct vcpu_info); > > + xen_vcpu_info = __alloc_percpu(struct vcpu_info, PAGE_SIZE); > > if (xen_vcpu_info == NULL) > > return -ENOMEM; > > > > May I suggest to use a smaller alignment? What about: > > 1 << fls(sizeof(struct vcpu_info) - 1) See below --- [PATCH] arm/xen: fix xen_vcpu_info allocation alignment xen_vcpu_info is a percpu area than needs to be mapped by Xen. Currently, it could cross a page boundary resulting in Xen being unable to map it: [0.567318] kernel BUG at arch/arm64/xen/../../arm/xen/enlighten.c:164! [0.574002] Internal error: Oops - BUG: f2000800 [#1] PREEMPT SMP Fix the issue by using __alloc_percpu and requesting alignment for the memory allocation. Signed-off-by: Stefano Stabellini diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index 9afdc4c4a5dc..09eb74a07dfc 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -484,7 +484,8 @@ static int __init xen_guest_init(void) * for secondary CPUs as they are brought up. * For uniformity we use VCPUOP_register_vcpu_info even on cpu0. */ - xen_vcpu_info = alloc_percpu(struct vcpu_info); + xen_vcpu_info = __alloc_percpu(sizeof(struct vcpu_info), + 1 << fls(sizeof(struct vcpu_info) - 1)); if (xen_vcpu_info == NULL) return -ENOMEM;
Re: [PATCH v2 3/6] xen: xenstore: add possibility to preserve owner
Hi David,
David Woodhouse writes:
> [[S/MIME Signed Part:Undecided]]
> On Tue, 2023-11-21 at 22:10 +, Volodymyr Babchuk wrote:
>>
>> --- a/hw/xen/xen-operations.c
>> +++ b/hw/xen/xen-operations.c
>> @@ -300,6 +300,18 @@ static bool libxenstore_create(struct qemu_xs_handle
>> *h, xs_transaction_t t,
>> return false;
>> }
>>
>> + if (owner == XS_PRESERVE_OWNER) {
>> + struct xs_permissions *tmp;
>> + unsigned int num;
>> +
>> + tmp = xs_get_permissions(h->xsh, t, path, );
>> + if (tmp == NULL) {
>> + return false;
>> + }
>> + perms_list[0].id = tmp[0].id;
>> + free(tmp);
>> + }
>> +
>> return xs_set_permissions(h->xsh, t, path, perms_list,
>> ARRAY_SIZE(perms_list));
>> }
>
> If the existing transaction is XBT_NULL I think you want to create a
> new transaction for it, don't you?
As per Stefano's and Paul's comments I'll drop this patch
completely. Thanks for review, thought.
--
WBR, Volodymyr
Re: [PATCH v2 1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it
On Wed, 2023-11-22 at 22:56 +, Volodymyr Babchuk wrote: > > > Paul Durrant writes: > > > On 21/11/2023 22:10, Volodymyr Babchuk wrote: > > > From: David Woodhouse > > > This allows a XenDevice implementation to know whether it was > > > created > > > by QEMU, or merely discovered in XenStore after the toolstack created > > > it. This will allow us to create frontend/backend nodes only when we > > > should, rather than unconditionally attempting to overwrite them from > > > a driver domain which doesn't have privileges to do so. > > > As an added benefit, it also means we no longer have to call the > > > xen_backend_set_device() function from the device models immediately > > > after calling qdev_realize_and_unref(). Even though we could make > > > the argument that it's safe to do so, and the pointer to the unreffed > > > device *will* actually still be valid, it still made my skin itch to > > > look at it. > > > Signed-off-by: David Woodhouse > > > --- > > > hw/block/xen-block.c | 3 +-- > > > hw/char/xen_console.c | 2 +- > > > hw/net/xen_nic.c | 2 +- > > > hw/xen/xen-bus.c | 4 > > > include/hw/xen/xen-backend.h | 2 -- > > > include/hw/xen/xen-bus.h | 2 ++ > > > 6 files changed, 9 insertions(+), 6 deletions(-) > > > > > > > Actually, I think you should probably update > > xen_backend_try_device_destroy() in this patch too. It currently uses > > xen_backend_list_find() to check whether the specified XenDevice has > > an associated XenBackendInstance. > > Sure. Looks like it is the only user of xen_backend_list_find(), so we > can get rid of it as well. > > I'll drop your R-b tag, because of those additional changes in the new > version. I think it's fine to keep. He told me to do it :) smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v2 4/6] xen_pvdev: Do not assume Dom0 when creating a directory
On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote: > On Wed, 22 Nov 2023, Paul Durrant wrote: > > On 21/11/2023 22:10, Volodymyr Babchuk wrote: > > > From: Oleksandr Tyshchenko > > > > > > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to > > > inherit the owner of the directory. > > > > Ah... so that's why the previous patch is there. > > > > This is not the right way to fix it. The QEMU Xen support is *assuming* that > > QEMU is either running in, or emulating, dom0. In the emulation case this is > > probably fine, but the 'real Xen' case it should be using the correct domid > > for node creation. I guess this could either be supplied on the command line > > or discerned by reading the local domain 'domid' node. > > yes, it should be passed as command line option to QEMU I'm not sure I like the idea of a command line option for something which QEMU could discover for itself. smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v2 3/6] xen: xenstore: add possibility to preserve owner
On Tue, 2023-11-21 at 22:10 +, Volodymyr Babchuk wrote:
>
> --- a/hw/xen/xen-operations.c
> +++ b/hw/xen/xen-operations.c
> @@ -300,6 +300,18 @@ static bool libxenstore_create(struct qemu_xs_handle *h,
> xs_transaction_t t,
> return false;
> }
>
> + if (owner == XS_PRESERVE_OWNER) {
> + struct xs_permissions *tmp;
> + unsigned int num;
> +
> + tmp = xs_get_permissions(h->xsh, t, path, );
> + if (tmp == NULL) {
> + return false;
> + }
> + perms_list[0].id = tmp[0].id;
> + free(tmp);
> + }
> +
> return xs_set_permissions(h->xsh, t, path, perms_list,
> ARRAY_SIZE(perms_list));
> }
If the existing transaction is XBT_NULL I think you want to create a
new transaction for it, don't you?
smime.p7s
Description: S/MIME cryptographic signature
Re: [XEN PATCH] automation/eclair: improve scheduled analyses
On Wed, 22 Nov 2023, Simone Ballarin wrote:
> > > diff --git a/automation/gitlab-ci/build.yaml
> > > b/automation/gitlab-ci/build.yaml
> > > index 32af30cced..6b2ac97248 100644
> > > --- a/automation/gitlab-ci/build.yaml
> > > +++ b/automation/gitlab-ci/build.yaml
> > > @@ -1,6 +1,10 @@
> > > .build-tmpl:
> > > stage: build
> > > image: registry.gitlab.com/xen-project/xen/${CONTAINER}
> > > + rules:
> > > +- if: $CI_PIPELINE_SOURCE == "schedule"
> > > + when: never
> > > +- when: always
> >
> > ...does it mean that we are going to stop all the build jobs...
> >
> >
> > > script:
> > > - ./automation/scripts/build 2>&1 | tee build.log
> > > artifacts:
> > > diff --git a/automation/gitlab-ci/test.yaml
> > > b/automation/gitlab-ci/test.yaml
> > > index 61e642cce0..47fc8cb3eb 100644
> > > --- a/automation/gitlab-ci/test.yaml
> > > +++ b/automation/gitlab-ci/test.yaml
> > > @@ -1,6 +1,10 @@
> > > .test-jobs-common:
> > > stage: test
> > > image: registry.gitlab.com/xen-project/xen/${CONTAINER}
> > > + rules:
> > > +- if: $CI_PIPELINE_SOURCE == "schedule"
> > > + when: never
> > > +- when: always
> >
> > ...and also stop all the test jobs?
> >
> > So basically the only thing left is .eclair-analysis:on-schedule ?
>
> Yes, you're right. I don't know if this is the indented behavior,
> but without these changes all jobs run implicitly.
>
> If test and build stages are supposed to run on scheduled pipelines,
> I suggest making it explicit by reversing the guard.
Yes I think it is OK to run build and test jobs on scheduled pipelines,
it is not worth optimized them out. I would reduce this patch to the
below. Would that work?
If so, please resend with only the below, and you can add my
Reviewed-by: Stefano Stabellini
diff --git a/automation/eclair_analysis/ECLAIR/action.settings
b/automation/eclair_analysis/ECLAIR/action.settings
index f96368ffc7..3cba1a3afb 100644
--- a/automation/eclair_analysis/ECLAIR/action.settings
+++ b/automation/eclair_analysis/ECLAIR/action.settings
@@ -134,7 +134,7 @@ push)
badgeLabel="ECLAIR ${ANALYSIS_KIND} ${ref}${variantHeadline} #${jobId}"
;;
auto_pull_request)
-git remote remove autoPRRemote || true
+git remote remove autoPRRemote 2>/dev/null || true
git remote add autoPRRemote "${autoPRRemoteUrl}"
git fetch -q autoPRRemote
subDir="${ref}"
diff --git a/automation/eclair_analysis/ECLAIR/analysis.ecl
b/automation/eclair_analysis/ECLAIR/analysis.ecl
index fe418d6da1..2507a8e787 100644
--- a/automation/eclair_analysis/ECLAIR/analysis.ecl
+++ b/automation/eclair_analysis/ECLAIR/analysis.ecl
@@ -2,7 +2,13 @@
-project_name=getenv("ECLAIR_PROJECT_NAME")
-project_root=getenv("ECLAIR_PROJECT_ROOT")
--setq=data_dir,getenv("ECLAIR_DATA_DIR")
+setq(data_dir,getenv("ECLAIR_DATA_DIR"))
+setq(analysis_kind,getenv("ANALYSIS_KIND"))
+setq(scheduled_analysis,nil)
+
+strings_map("scheduled-analysis",500,"","^.*scheduled$",0,setq(scheduled_analysis,t))
+strings_map("scheduled-analysis",500,"","^.*$",0)
+map_strings("scheduled-analysis",analysis_kind)
-verbose
@@ -15,7 +21,9 @@
-eval_file=toolchain.ecl
-eval_file=public_APIs.ecl
--eval_file=out_of_scope.ecl
+if(scheduled_analysis,
+eval_file("out_of_scope.ecl")
+)
-eval_file=deviations.ecl
-eval_file=call_properties.ecl
-eval_file=tagging.ecl
diff --git a/automation/gitlab-ci/analyze.yaml
b/automation/gitlab-ci/analyze.yaml
index bd9a68de31..6631db53fa 100644
--- a/automation/gitlab-ci/analyze.yaml
+++ b/automation/gitlab-ci/analyze.yaml
@@ -28,6 +28,8 @@
extends: .eclair-analysis
allow_failure: true
rules:
+- if: $CI_PIPELINE_SOURCE == "schedule"
+ when: never
- if: $WTOKEN && $CI_PROJECT_PATH =~ /^xen-project\/people\/.*$/
when: manual
- !reference [.eclair-analysis, rules]
Re: [PATCH v2 1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it
Paul Durrant writes: > On 21/11/2023 22:10, Volodymyr Babchuk wrote: >> From: David Woodhouse >> This allows a XenDevice implementation to know whether it was >> created >> by QEMU, or merely discovered in XenStore after the toolstack created >> it. This will allow us to create frontend/backend nodes only when we >> should, rather than unconditionally attempting to overwrite them from >> a driver domain which doesn't have privileges to do so. >> As an added benefit, it also means we no longer have to call the >> xen_backend_set_device() function from the device models immediately >> after calling qdev_realize_and_unref(). Even though we could make >> the argument that it's safe to do so, and the pointer to the unreffed >> device *will* actually still be valid, it still made my skin itch to >> look at it. >> Signed-off-by: David Woodhouse >> --- >> hw/block/xen-block.c | 3 +-- >> hw/char/xen_console.c| 2 +- >> hw/net/xen_nic.c | 2 +- >> hw/xen/xen-bus.c | 4 >> include/hw/xen/xen-backend.h | 2 -- >> include/hw/xen/xen-bus.h | 2 ++ >> 6 files changed, 9 insertions(+), 6 deletions(-) >> > > Actually, I think you should probably update > xen_backend_try_device_destroy() in this patch too. It currently uses > xen_backend_list_find() to check whether the specified XenDevice has > an associated XenBackendInstance. Sure. Looks like it is the only user of xen_backend_list_find(), so we can get rid of it as well. I'll drop your R-b tag, because of those additional changes in the new version. -- WBR, Volodymyr
Re: [PATCH v2 2/6] xen: backends: touch some XenStore nodes only if device...
Hi Paul, Paul Durrant writes: > On 21/11/2023 22:10, Volodymyr Babchuk wrote: >> was created by QEMU >> Xen PV devices in QEMU can be created in two ways: either by QEMU >> itself, if they were passed via command line, or by Xen toolstack. In >> the latter case, QEMU scans XenStore entries and configures devices >> accordingly. >> In the second case we don't want QEMU to write/delete front-end >> entries for two reasons: it might have no access to those entries if >> it is running in un-privileged domain and it is just incorrect to >> overwrite entries already provided by Xen toolstack, because toolstack >> manages those nodes. For example, it might read backend- or frontend- >> state to be sure that they are both disconnected and it is safe to >> destroy a domain. >> This patch checks presence of xendev->backend to check if Xen PV >> device is acting as a backend (i.e. it was configured by Xen > > Technally *all* XenDevice objects are backends. > Yes, you are right of course. I'll rephrase this paragraph in the next version. -- WBR, Volodymyr
Re: [PATCH v2 2/6] xen: backends: touch some XenStore nodes only if device...
Hi Philippe, Philippe Mathieu-Daudé writes: > Hi Volodymyr, > > On 21/11/23 23:10, Volodymyr Babchuk wrote: >> was created by QEMU > > Please do not split lines between subject and content. Rewrite the > full line. Preferably restrict the subject to 72 chars. I tried to come with shorter description, but failed. I'll rewrite it in the next version. -- WBR, Volodymyr
Re: [PATCH v2 2/6] xen: backends: touch some XenStore nodes only if device...
On Wed, 2023-11-22 at 17:03 +, Paul Durrant wrote: > > > This patch checks presence of xendev->backend to check if Xen PV > > device is acting as a backend (i.e. it was configured by Xen > > Technally *all* XenDevice objects are backends. Right. The key criterion is whether the backend was created by QEMU, or merely discovered by QEMU after the toolstack created it. smime.p7s Description: S/MIME cryptographic signature Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
Re: [PATCH v2 1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it
On Wed, 2023-11-22 at 17:05 +, Paul Durrant wrote: > On 21/11/2023 22:10, Volodymyr Babchuk wrote: > > From: David Woodhouse > > > > This allows a XenDevice implementation to know whether it was created > > by QEMU, or merely discovered in XenStore after the toolstack created > > it. This will allow us to create frontend/backend nodes only when we > > should, rather than unconditionally attempting to overwrite them from > > a driver domain which doesn't have privileges to do so. > > > > As an added benefit, it also means we no longer have to call the > > xen_backend_set_device() function from the device models immediately > > after calling qdev_realize_and_unref(). Even though we could make > > the argument that it's safe to do so, and the pointer to the unreffed > > device *will* actually still be valid, it still made my skin itch to > > look at it. > > > > Signed-off-by: David Woodhouse > > --- > > hw/block/xen-block.c | 3 +-- > > hw/char/xen_console.c | 2 +- > > hw/net/xen_nic.c | 2 +- > > hw/xen/xen-bus.c | 4 > > include/hw/xen/xen-backend.h | 2 -- > > include/hw/xen/xen-bus.h | 2 ++ > > 6 files changed, 9 insertions(+), 6 deletions(-) > > > > Actually, I think you should probably update > xen_backend_try_device_destroy() in this patch too. It currently uses > xen_backend_list_find() to check whether the specified XenDevice has an > associated XenBackendInstance. I think I did that in https://git.infradead.org/users/dwmw2/qemu.git/commitdiff/94f1b474478ce0ede but didn't get round to sending it out again because of travel. smime.p7s Description: S/MIME cryptographic signature Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
Re: [PATCH v2 6/6] xen_arm: Add virtual PCIe host bridge support
+Vikram
On Tue, 21 Nov 2023, Volodymyr Babchuk wrote:
> From: Oleksandr Tyshchenko
>
> The bridge is needed for virtio-pci support, as QEMU can emulate the
> whole bridge with any virtio-pci devices connected to it.
>
> This patch provides a flexible way to configure PCIe brige resources
> with xenstore. We made this for several reasons:
>
> - We don't want to clash with vPCI devices, so we need information
> from Xen toolstack on which PCI bus to use.
> - The guest memory layout that describes these resources is not stable
> and may vary between guests, so we cannot rely on static resources
> to be always the same for both ends.
> - Also the device-models which run in different domains and serve
> virtio-pci devices for the same guest should use different host
> bridge resources for Xen to distinguish. The rule for the guest
> device-tree generation is one PCI host bridge per backend domain.
>
> Signed-off-by: Oleksandr Tyshchenko
> Signed-off-by: Volodymyr Babchuk
There is still a discussion ongoing on xen-devel if / how to register a
PCI Root Complex or individual BDFs. In the meantime a couple of
comments.
Typically emulated devices are configured in QEMU via QEMU command line
parameters, not xenstore. If you are running QEMU in a domU (instead of
Dom0) you can always read config parameters from xenstore from a wrapper
bash script (using xenstore-read) and then pass the right command line
options to QEMU.
If you need help in adding new QEMU command line options, Vikram (CCed)
can help.
> ---
>
> Changes from v1:
>
> - Renamed virtio_pci_host to pcie_host entries in XenStore, because
> there is nothing specific to virtio-pci: any PCI device can be
> emulated via this newly created bridge.
> ---
> hw/arm/xen_arm.c| 186
> hw/xen/xen-hvm-common.c | 9 +-
> include/hw/xen/xen_native.h | 8 +-
> 3 files changed, 200 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
> index b9c3ae14b6..d506d55d0f 100644
> --- a/hw/arm/xen_arm.c
> +++ b/hw/arm/xen_arm.c
> @@ -22,6 +22,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
> #include "qemu/error-report.h"
> #include "qapi/qapi-commands-migration.h"
> #include "qapi/visitor.h"
> @@ -34,6 +35,9 @@
> #include "hw/xen/xen-hvm-common.h"
> #include "sysemu/tpm.h"
> #include "hw/xen/arch_hvm.h"
> +#include "exec/address-spaces.h"
> +#include "hw/pci-host/gpex.h"
> +#include "hw/virtio/virtio-pci.h"
>
> #define TYPE_XEN_ARM MACHINE_TYPE_NAME("xenpvh")
> OBJECT_DECLARE_SIMPLE_TYPE(XenArmState, XEN_ARM)
> @@ -58,6 +62,11 @@ struct XenArmState {
> struct {
> uint64_t tpm_base_addr;
> } cfg;
> +
> +MemMapEntry pcie_mmio;
> +MemMapEntry pcie_ecam;
> +MemMapEntry pcie_mmio_high;
> +int pcie_irq;
> };
>
> static MemoryRegion ram_lo, ram_hi;
> @@ -73,6 +82,7 @@ static MemoryRegion ram_lo, ram_hi;
> #define NR_VIRTIO_MMIO_DEVICES \
> (GUEST_VIRTIO_MMIO_SPI_LAST - GUEST_VIRTIO_MMIO_SPI_FIRST)
>
> +/* TODO It should be xendevicemodel_set_pci_intx_level() for PCI interrupts.
> */
> static void xen_set_irq(void *opaque, int irq, int level)
> {
> if (xendevicemodel_set_irq_level(xen_dmod, xen_domid, irq, level)) {
> @@ -129,6 +139,176 @@ static void xen_init_ram(MachineState *machine)
> }
> }
>
> +static void xen_create_pcie(XenArmState *xam)
> +{
> +MemoryRegion *mmio_alias, *mmio_alias_high, *mmio_reg;
> +MemoryRegion *ecam_alias, *ecam_reg;
> +DeviceState *dev;
> +int i;
> +
> +dev = qdev_new(TYPE_GPEX_HOST);
> +sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
> +
> +/* Map ECAM space */
> +ecam_alias = g_new0(MemoryRegion, 1);
> +ecam_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> +memory_region_init_alias(ecam_alias, OBJECT(dev), "pcie-ecam",
> + ecam_reg, 0, xam->pcie_ecam.size);
> +memory_region_add_subregion(get_system_memory(), xam->pcie_ecam.base,
> +ecam_alias);
> +
> +/* Map the MMIO space */
> +mmio_alias = g_new0(MemoryRegion, 1);
> +mmio_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
> +memory_region_init_alias(mmio_alias, OBJECT(dev), "pcie-mmio",
> + mmio_reg,
> + xam->pcie_mmio.base,
> + xam->pcie_mmio.size);
> +memory_region_add_subregion(get_system_memory(), xam->pcie_mmio.base,
> +mmio_alias);
> +
> +/* Map the MMIO_HIGH space */
> +mmio_alias_high = g_new0(MemoryRegion, 1);
> +memory_region_init_alias(mmio_alias_high, OBJECT(dev), "pcie-mmio-high",
> + mmio_reg,
> + xam->pcie_mmio_high.base,
> + xam->pcie_mmio_high.size);
> +memory_region_add_subregion(get_system_memory(),
>
Re: [PATCH 5/6] tools/pygrub: Expose libfsimage's fdopen() to python
On 06/11/2023 3:05 pm, Alejandro Vallejo wrote:
> Create a wrapper for the new fdopen() function of libfsimage.
>
> Signed-off-by: Alejandro Vallejo
I'd appreciate it if Marek would cast his eye (as python maintainer)
over it.
That said, ...
> diff --git a/tools/pygrub/src/fsimage/fsimage.c
> b/tools/pygrub/src/fsimage/fsimage.c
> index 12dfcff6e3..216f265331 100644
> --- a/tools/pygrub/src/fsimage/fsimage.c
> +++ b/tools/pygrub/src/fsimage/fsimage.c
> @@ -270,6 +270,30 @@ fsimage_open(PyObject *o, PyObject *args, PyObject
> *kwargs)
> return (PyObject *)fs;
> }
>
> +static PyObject *
> +fsimage_fdopen(PyObject *o, PyObject *args, PyObject *kwargs)
> +{
> + static char *kwlist[] = { "fd", "offset", "options", NULL };
> + int fd;
> + char *options = NULL;
> + uint64_t offset = 0;
> + fsimage_fs_t *fs;
> +
> + if (!PyArg_ParseTupleAndKeywords(args, kwargs, "i|Ls", kwlist,
> + , , ))
> + return (NULL);
> +
> + if ((fs = PyObject_NEW(fsimage_fs_t, _fs_type)) == NULL)
> + return (NULL);
> +
> + if ((fs->fs = fsi_fdopen_fsimage(fd, offset, options)) == NULL) {
> + PyErr_SetFromErrno(PyExc_IOError);
Don't we need a Py_DECREF(fs) here to avoid leaking it?
~Andrew
> + return (NULL);
> + }
> +
> + return (PyObject *)fs;
> +}
> +
> static PyObject *
> fsimage_getbootstring(PyObject *o, PyObject *args)
> {
> @@ -302,6 +326,13 @@ PyDoc_STRVAR(fsimage_open__doc__,
> "offset - offset of file system within file image.\n"
> "options - mount options string.\n");
>
> +PyDoc_STRVAR(fsimage_fdopen__doc__,
> +"fdopen(fd, [offset=off]) - Use the file provided by the given fd as a
> filesystem image.\n"
> +"\n"
> +"fd - File descriptor to use.\n"
> +"offset - offset of file system within file image.\n"
> +"options - mount options string.\n");
> +
> PyDoc_STRVAR(fsimage_getbootstring__doc__,
> "getbootstring(fs) - Return the boot string needed for this file system "
> "or NULL if none is needed.\n");
> @@ -315,6 +346,8 @@ static struct PyMethodDef fsimage_module_methods[] = {
> METH_VARARGS, fsimage_init__doc__ },
> { "open", (PyCFunction)fsimage_open,
> METH_VARARGS|METH_KEYWORDS, fsimage_open__doc__ },
> + { "fdopen", (PyCFunction)fsimage_fdopen,
> + METH_VARARGS|METH_KEYWORDS, fsimage_fdopen__doc__ },
> { "getbootstring", (PyCFunction)fsimage_getbootstring,
> METH_VARARGS, fsimage_getbootstring__doc__ },
> { NULL, NULL, 0, NULL }
Re: [PATCH 4/6] tools/libfsimage: Add an fdopen() interface to libfsimage
On 06/11/2023 3:05 pm, Alejandro Vallejo wrote:
> diff --git a/tools/libfsimage/common/fsimage_priv.h
> b/tools/libfsimage/common/fsimage_priv.h
> index 2274403557..779e433b37 100644
> --- a/tools/libfsimage/common/fsimage_priv.h
> +++ b/tools/libfsimage/common/fsimage_priv.h
> @@ -29,6 +29,7 @@ extern C {
> #endif
>
> #include
> +#include
>
> #include "xenfsimage.h"
> #include "xenfsimage_plugin.h"
> @@ -54,7 +55,7 @@ struct fsi_file {
> void *ff_data;
> };
>
> -int find_plugin(fsi_t *, const char *, const char *);
> +int find_plugin(fsi_t *, const char *);
>
> #ifdef __cplusplus
> };
These are the only two hunks in this file. Is the stdbool include stale?
It seems to compile fine with it removed.
> diff --git a/tools/libfsimage/ext2fs-lib/ext2fs-lib.c
> b/tools/libfsimage/ext2fs-lib/ext2fs-lib.c
> index 864a15b349..9f07ea288f 100644
> --- a/tools/libfsimage/ext2fs-lib/ext2fs-lib.c
> +++ b/tools/libfsimage/ext2fs-lib/ext2fs-lib.c
> @@ -25,15 +25,25 @@
> #include INCLUDE_EXTFS_H
> #include
> #include
> +#include
>
> static int
> -ext2lib_mount(fsi_t *fsi, const char *name, const char *options)
> +ext2lib_mount(fsi_t *fsi, const char *options)
> {
> int err;
> char opts[30] = "";
> ext2_filsys *fs;
> uint64_t offset = fsip_fs_offset(fsi);
>
> + /*
> + * We must choose unixfd_io_manager rather than unix_io_manager in
> + * order for the library to accept fd strings instead of paths. It
> + * still means we must pass a string representing an fd rather than
> + * an int, but at least this way we don't need to pass paths around
> + */
> + char name[32] = {0};
For an int? 12 will do fine including a terminator, and practical
system limits leave it far smaller than that generally.
Given that it is guaranteed long enough, you don't need to zero it just
to have snprintf() write a well-formed string in.
~Andrew
Re: [PATCH v2 4/6] xen_pvdev: Do not assume Dom0 when creating a directory
On Wed, 22 Nov 2023, Paul Durrant wrote:
> On 21/11/2023 22:10, Volodymyr Babchuk wrote:
> > From: Oleksandr Tyshchenko
> >
> > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
> > inherit the owner of the directory.
>
> Ah... so that's why the previous patch is there.
>
> This is not the right way to fix it. The QEMU Xen support is *assuming* that
> QEMU is either running in, or emulating, dom0. In the emulation case this is
> probably fine, but the 'real Xen' case it should be using the correct domid
> for node creation. I guess this could either be supplied on the command line
> or discerned by reading the local domain 'domid' node.
yes, it should be passed as command line option to QEMU
> > Note that for other than Dom0 domain (non toolstack domain) the
> > "driver_domain" property should be set in domain config file for the
> > toolstack to create required directories in advance.
> >
> > Signed-off-by: Oleksandr Tyshchenko
> > Signed-off-by: Volodymyr Babchuk
> > ---
> > hw/xen/xen_pvdev.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
> > index c5ad71e8dc..42bdd4f6c8 100644
> > --- a/hw/xen/xen_pvdev.c
> > +++ b/hw/xen/xen_pvdev.c
> > @@ -60,7 +60,8 @@ void xen_config_cleanup(void)
> > int xenstore_mkdir(char *path, int p)
> > {
> > -if (!qemu_xen_xs_create(xenstore, 0, 0, xen_domid, p, path)) {
> > +if (!qemu_xen_xs_create(xenstore, 0, XS_PRESERVE_OWNER,
> > +xen_domid, p, path)) {
> > xen_pv_printf(NULL, 0, "xs_mkdir %s: failed\n", path);
> > return -1;
> > }
>
Re: [PATCH v2 3/6] xen: xenstore: add possibility to preserve owner
On Wed, 22 Nov 2023, Paul Durrant wrote: > On 21/11/2023 22:10, Volodymyr Babchuk wrote: > > Add option to preserve owner when creating an entry in Xen Store. This > > may be needed in cases when Qemu is working as device model in a > > *may* be needed? > > I don't undertstand why this patch is necessary and the commit comment isn't > really helping me. > > > domain that is Domain-0, e.g. in driver domain. I think Volodymyr meant: domain that is *not* Domain-0 So I am guessing this patch is needed otherwise QEMU will run into permissions errors doing xenstore operations > > "owner" parameter for qemu_xen_xs_create() function can have special > > value XS_PRESERVE_OWNER, which will make specific implementation to > > get original owner of an entry and pass it back to > > set_permissions() call. > > > > Please note, that XenStore inherits permissions, so even if entry is > > newly created by, it already has the owner set to match owner of entry > > at previous level. > > > > Signed-off-by: Volodymyr Babchuk >
Re: [XEN PATCH v4 2/2] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3
On Wed, 22 Nov 2023, Federico Serafini wrote: > Add missing parameter names and uniform the interfaces of > modify_xen_mappings() and modify_xen_mappings_lite(). > > No functional change. > > Signed-off-by: Federico Serafini Reviewed-by: Stefano Stabellini
Re: [PATCH 3/3] xen/MISRA: Remove nonstandard inline keywords
On 22/11/2023 10:13 pm, Stefano Stabellini wrote: > On Wed, 22 Nov 2023, Andrew Cooper wrote: >> The differences between inline, __inline and __inline__ keywords are a >> vestigial remnant of older C standards, and in Xen we use inline almost >> exclusively. >> >> Replace __inline and __inline__ with regular inline, and remove their >> exceptions from the MISRA configuration. >> >> Signed-off-by: Andrew Cooper >> >> diff --git a/docs/misra/C-language-toolchain.rst >> b/docs/misra/C-language-toolchain.rst >> index 2866cb191b1a..b7c2000992ac 100644 >> --- a/docs/misra/C-language-toolchain.rst >> +++ b/docs/misra/C-language-toolchain.rst >> @@ -84,7 +84,7 @@ The table columns are as follows: >>see Sections "6.48 Alternate Keywords" and "6.47 How to Use >> Inline Assembly Language in C Code" of GCC_MANUAL. >> __volatile__: >>see Sections "6.48 Alternate Keywords" and "6.47.2.1 Volatile" of >> GCC_MANUAL. >> - __const__, __inline__, __inline: >> + __const__: >>see Section "6.48 Alternate Keywords" of GCC_MANUAL. >> typeof, __typeof__: >>see Section "6.7 Referring to a Type with typeof" of GCC_MANUAL. > Asking the Bugseng guys as well, do we need to add to > C-language-toolchain.rst: > inline __attribute__((__always_inline__)) > inline __attribute__((__gnu_inline__)) __attribute__ itself is in the list of permitted non-standard tokens, in both files. However, neither file has anything concerning the parameter(s) to the __attribute__, and we do use an awful lot of them. If they want discussing, then that's going to be a lot of work. > Given that the problem was also present before this patch: > > Reviewed-by: Stefano Stabellini Thanks.
Re: [XEN PATCH v4 1/2] x86/mm: preparation work to uniform modify_xen_mappings* interfaces
On Wed, 22 Nov 2023, Federico Serafini wrote: > The objective is to use parameter name "nf" to denote "new flags" > in all the modify_xen_mappings* functions. > Since modify_xen_mappings_lite() is currently using "nf" as identifier > for a local variable, bad things could happen if new uses of such > variable are committed while a renaming patch is waiting for the > approval. > To avoid such danger, as first thing rename the local variable from > "nf" to "flags". > > No functional change. > > Suggested-by: Jan Beulich > Signed-off-by: Federico Serafini Reviewed-by: Stefano Stabellini
Re: [PATCH 3/3] xen/MISRA: Remove nonstandard inline keywords
On Wed, 22 Nov 2023, Andrew Cooper wrote: > The differences between inline, __inline and __inline__ keywords are a > vestigial remnant of older C standards, and in Xen we use inline almost > exclusively. > > Replace __inline and __inline__ with regular inline, and remove their > exceptions from the MISRA configuration. > > Signed-off-by: Andrew Cooper > > diff --git a/docs/misra/C-language-toolchain.rst > b/docs/misra/C-language-toolchain.rst > index 2866cb191b1a..b7c2000992ac 100644 > --- a/docs/misra/C-language-toolchain.rst > +++ b/docs/misra/C-language-toolchain.rst > @@ -84,7 +84,7 @@ The table columns are as follows: >see Sections "6.48 Alternate Keywords" and "6.47 How to Use Inline > Assembly Language in C Code" of GCC_MANUAL. > __volatile__: >see Sections "6.48 Alternate Keywords" and "6.47.2.1 Volatile" of > GCC_MANUAL. > - __const__, __inline__, __inline: > + __const__: >see Section "6.48 Alternate Keywords" of GCC_MANUAL. > typeof, __typeof__: >see Section "6.7 Referring to a Type with typeof" of GCC_MANUAL. Asking the Bugseng guys as well, do we need to add to C-language-toolchain.rst: inline __attribute__((__always_inline__)) inline __attribute__((__gnu_inline__)) Given that the problem was also present before this patch: Reviewed-by: Stefano Stabellini > diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h > index 04b8bc18df0e..16d554f2a593 100644 > --- a/xen/include/xen/compiler.h > +++ b/xen/include/xen/compiler.h > @@ -20,9 +20,8 @@ > #define likely(x) __builtin_expect(!!(x),1) > #define unlikely(x) __builtin_expect(!!(x),0) > > -#define inline__inline__ > -#define always_inline __inline__ __attribute__ ((__always_inline__)) > -#define gnu_inline__inline__ __attribute__ ((__gnu_inline__)) > +#define always_inline inline __attribute__((__always_inline__)) > +#define gnu_inlineinline __attribute__((__gnu_inline__)) > #define noinline __attribute__((__noinline__)) > > #define noreturn __attribute__((__noreturn__)) This is where they are used. > @@ -83,7 +82,7 @@ > * inline functions not expanded inline get placed in .init.text. > */ > #include > -#define __inline__ __inline__ __init > +#define inline inline __init > #endif > > #define __attribute_pure__ __attribute__((__pure__)) > -- > 2.30.2 > >
Re: [PATCH 2/3] x86/apic: Drop the APIC_MSR_BASE constant
On Wed, 22 Nov 2023, Andrew Cooper wrote: > Use MSR_X2APIC_FIRST from msr-index.h instead. > > Signed-off-by: Andrew Cooper Reviewed-by: Stefano Stabellini
Re: [PATCH 1/3] x86/apic: Drop atomic accessors
On Wed, 22 Nov 2023, Andrew Cooper wrote:
> The last users were dropped in commit 413e92e9bf13 ("x86/apic: Drop
> workarounds for Pentium/82489DX erratum").
>
> Signed-off-by: Andrew Cooper
Reviewed-by: Stefano Stabellini
> ---
> CC: Jan Beulich
> CC: Roger Pau Monné
> CC: Wei Liu
> ---
> xen/arch/x86/include/asm/apic.h | 13 -
> 1 file changed, 13 deletions(-)
>
> diff --git a/xen/arch/x86/include/asm/apic.h b/xen/arch/x86/include/asm/apic.h
> index f7ad7b20dd80..288b4933eb38 100644
> --- a/xen/arch/x86/include/asm/apic.h
> +++ b/xen/arch/x86/include/asm/apic.h
> @@ -54,11 +54,6 @@ static __inline void apic_mem_write(unsigned long reg, u32
> v)
> *((volatile u32 *)(APIC_BASE+reg)) = v;
> }
>
> -static __inline void apic_mem_write_atomic(unsigned long reg, u32 v)
> -{
> - (void)xchg((volatile u32 *)(APIC_BASE+reg), v);
> -}
> -
> static __inline u32 apic_mem_read(unsigned long reg)
> {
> return *((volatile u32 *)(APIC_BASE+reg));
> @@ -97,14 +92,6 @@ static __inline void apic_write(unsigned long reg, u32 v)
> apic_mem_write(reg, v);
> }
>
> -static __inline void apic_write_atomic(unsigned long reg, u32 v)
> -{
> -if ( x2apic_enabled )
> -apic_wrmsr(reg, v);
> -else
> -apic_mem_write_atomic(reg, v);
> -}
> -
> static __inline u32 apic_read(unsigned long reg)
> {
> if ( x2apic_enabled )
> --
> 2.30.2
>
>
Re: [PATCH v7 1/2] xen/vpci: header: status register handler
On 11/17/23 08:33, Roger Pau Monné wrote: > On Wed, Sep 13, 2023 at 10:35:46AM -0400, Stewart Hildebrand wrote: >> +int vpci_add_register_mask(struct vpci *vpci, vpci_read_t *read_handler, >> + vpci_write_t *write_handler, unsigned int offset, >> + unsigned int size, void *data, uint32_t >> rsvdz_mask, >> + uint32_t ro_mask, uint32_t rw1c_mask) > > Forgot to mention, it would be good if you can add some tests in > tools/tests/vpci that ensure the masks work as expected. Will do > > Thanks, Roger.
[PATCH] do_multicall and MISRA Rule 8.3
Two out of three do_multicall definitions/declarations use uint32_t as type for the "nr_calls" parameters. Change the third one to be consistent with the other two. Link: https://lore.kernel.org/xen-devel/7e3abd4c0ef5127a07a60de1bf090a8aefac8e5c.1692717906.git.federico.seraf...@bugseng.com/ Link: https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2308251502430.6458@ubuntu-linux-20-04-desktop/ Signed-off-by: Stefano Stabellini --- Note that a previous discussion showed disagreement between maintainers on this topic. The source of disagreements are that we don't want to change a guest-visible ABI and we haven't properly documented how to use types for guest ABIs. As an example, fixed-width types have the advantage of being explicit about their size but sometimes register-size types are required (e.g. unsigned long). The C specification says little about the size of unsigned long and today, and we even use unsigned int in guest ABIs without specifying the expected width of unsigned int on the various arches. As Jan pointed out, in Xen we assume sizeof(int) >= 4, but that's not written anywhere as far as I can tell. I think the appropriate solution would be to document properly our expectations of both fixed-width and non-fixed-width types, and how to use them for guest-visible ABIs. In this patch I used uint32_t for a couple of reasons: - until we have better documentation, I feel more confident in using explicitly-sized integers in guest-visible ABIs - 2/3 of the do_multicall definition/declaration are using uint32_t diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c index 6d361ddfce..8b8ee16074 100644 --- a/xen/include/hypercall-defs.c +++ b/xen/include/hypercall-defs.c @@ -172,7 +172,7 @@ console_io(unsigned int cmd, unsigned int count, char *buffer) vm_assist(unsigned int cmd, unsigned int type) event_channel_op(int cmd, void *arg) mmuext_op(mmuext_op_t *uops, unsigned int count, unsigned int *pdone, unsigned int foreigndom) -multicall(multicall_entry_t *call_list, unsigned int nr_calls) +multicall(multicall_entry_t *call_list, uint32_t nr_calls) #ifdef CONFIG_PV mmu_update(mmu_update_t *ureqs, unsigned int count, unsigned int *pdone, unsigned int foreigndom) stack_switch(unsigned long ss, unsigned long esp)
Re: [PATCH] tools/xenpvboot: remove as unable to convert to Python 3
On 22/11/2023 9:27 pm, Olaf Hering wrote: > Wed, 22 Nov 2023 20:46:15 + Andrew Cooper : > >> But that entirely depends on whether you think anyone is using it or not. > We never got any bugreport, nor have we any indication for actual usage. > The script was in the sources. Python3 is supposed to be the default > interpreter since a number of years, so something had to be done with > every installed python script in the sources. > > I think if anyone shows up and demands this script, it could be restored. > Otherwise it is one less thing to worry about. Ok. Lets leave it to rest as it is. If we need to resurrect it, there's enough context on this thread to get a Py3 compatible one. ~Andrew
Re: [PATCH] tools/xenpvboot: remove as unable to convert to Python 3
Wed, 22 Nov 2023 20:46:15 + Andrew Cooper : > But that entirely depends on whether you think anyone is using it or not. We never got any bugreport, nor have we any indication for actual usage. The script was in the sources. Python3 is supposed to be the default interpreter since a number of years, so something had to be done with every installed python script in the sources. I think if anyone shows up and demands this script, it could be restored. Otherwise it is one less thing to worry about. Olaf pgp743EUyWp9S.pgp Description: Digitale Signatur von OpenPGP
Re: Devise macros to encapsulate (x & -x)
On Wed, 22 Nov 2023, Nicola Vetrini wrote: > > > > > > Jan, would you be willing to accept that other maintainers have a > > > preference for having a single MACRO even if suboptimal? > > > > I can live with that, even if I'm surprised by this perspective that others > > take. How can we, in reviews, tell people to make sure arguments are > > evaluated only once, when then we deliberately do otherwise in a case like > > the one here? The criteria of "not likely to be used in cases that have > > side effects" is an extremely fuzzy and sufficiently weak one, imo. I for > > one am even worried about the uses in MASK_EXTR() / MASK_INSR(), and would > > have considered introducing single-evaluation forms there as well. > > > > > If so, can we go ahead and commit the original patches? > > > > Well, the renaming needs to be done there anyway. > > > > I can do the renaming if you don't feel particularly safe doing it on commit Please resend
Re: [PATCH v10 13/17] vpci: add initial support for virtual PCI bus topology
On Wed, 22 Nov 2023, Roger Pau Monné wrote: > On Tue, Nov 21, 2023 at 05:12:15PM -0800, Stefano Stabellini wrote: > > On Tue, 20 Nov 2023, Volodymyr Babchuk wrote: > > > Stefano Stabellini writes: > > > > On Fri, 17 Nov 2023, Volodymyr Babchuk wrote: > > > >> > On Fri, 17 Nov 2023, Volodymyr Babchuk wrote: > > > >> >> Hi Julien, > > > >> >> > > > >> >> Julien Grall writes: > > > >> >> > > > >> >> > Hi Volodymyr, > > > >> >> > > > > >> >> > On 17/11/2023 14:09, Volodymyr Babchuk wrote: > > > >> >> >> Hi Stefano, > > > >> >> >> Stefano Stabellini writes: > > > >> >> >> > > > >> >> >>> On Fri, 17 Nov 2023, Volodymyr Babchuk wrote: > > > >> >> > I still think, no matter the BDF allocation scheme, that we > > > >> >> > should try > > > >> >> > to avoid as much as possible to have two different PCI Root > > > >> >> > Complex > > > >> >> > emulators. Ideally we would have only one PCI Root Complex > > > >> >> > emulated by > > > >> >> > Xen. Having 2 PCI Root Complexes both of them emulated by Xen > > > >> >> > would be > > > >> >> > tolerable but not ideal. > > > >> >> > > > >> >> But what is exactly wrong with this setup? > > > >> >> >>> > > > >> >> >>> [...] > > > >> >> >>> > > > >> >> > The worst case I would like to avoid is to have > > > >> >> > two PCI Root Complexes, one emulated by Xen and one emulated > > > >> >> > by QEMU. > > > >> >> > > > >> >> This is how our setup works right now. > > > >> >> >>> > > > >> >> >>> If we have: > > > >> >> >>> - a single PCI Root Complex emulated in Xen > > > >> >> >>> - Xen is safety certified > > > >> >> >>> - individual Virtio devices emulated by QEMU with grants for > > > >> >> >>> memory > > > >> >> >>> > > > >> >> >>> We can go very far in terms of being able to use Virtio in > > > >> >> >>> safety > > > >> >> >>> use-cases. We might even be able to use Virtio (frontends) in a > > > >> >> >>> SafeOS. > > > >> >> >>> > > > >> >> >>> On the other hand if we put an additional Root Complex in QEMU: > > > >> >> >>> - we pay a price in terms of complexity of the codebase > > > >> >> >>> - we pay a price in terms of resource utilization > > > >> >> >>> - we have one additional problem in terms of using this setup > > > >> >> >>> with a > > > >> >> >>>SafeOS (one more device emulated by a non-safe component) > > > >> >> >>> > > > >> >> >>> Having 2 PCI Root Complexes both emulated in Xen is a middle > > > >> >> >>> ground > > > >> >> >>> solution because: > > > >> >> >>> - we still pay a price in terms of resource utilization > > > >> >> >>> - the code complexity goes up a bit but hopefully not by much > > > >> >> >>> - there is no impact on safety compared to the ideal scenario > > > >> >> >>> > > > >> >> >>> This is why I wrote that it is tolerable. > > > >> >> >> Ah, I see now. Yes, I am agree with this. Also I want to add some > > > >> >> >> more > > > >> >> >> points: > > > >> >> >> - There is ongoing work on implementing virtio backends as a > > > >> >> >> separate > > > >> >> >>applications, written in Rust. Linaro are doing this part. > > > >> >> >> Right now > > > >> >> >>they are implementing only virtio-mmio, but if they want to > > > >> >> >> provide > > > >> >> >>virtio-pci as well, they will need a mechanism to plug only > > > >> >> >>virtio-pci, without Root Complex. This is argument for using > > > >> >> >> single Root > > > >> >> >>Complex emulated in Xen. > > > >> >> >> - As far as I know (actually, Oleksandr told this to me), QEMU > > > >> >> >> has > > > >> >> >> no > > > >> >> >>mechanism for exposing virtio-pci backends without exposing > > > >> >> >> PCI root > > > >> >> >>complex as well. Architecturally, there should be a PCI bus > > > >> >> >> to which > > > >> >> >>virtio-pci devices are connected. Or we need to make some > > > >> >> >> changes to > > > >> >> >>QEMU internals to be able to create virtio-pci backends that > > > >> >> >> are not > > > >> >> >>connected to any bus. Also, added benefit that PCI Root > > > >> >> >> Complex > > > >> >> >>emulator in QEMU handles legacy PCI interrupts for us. This is > > > >> >> >>argument for separate Root Complex for QEMU. > > > >> >> >> As right now we have only virtio-pci backends provided by QEMU > > > >> >> >> and > > > >> >> >> this > > > >> >> >> setup is already working, I propose to stick to this > > > >> >> >> solution. Especially, taking into account that it does not > > > >> >> >> require any > > > >> >> >> changes to hypervisor code. > > > >> >> > > > > >> >> > I am not against two hostbridge as a temporary solution as long as > > > >> >> > this is not a one way door decision. I am not concerned about the > > > >> >> > hypervisor itself, I am more concerned about the interface > > > >> >> > exposed by > > > >> >> > the toolstack and QEMU. > > > >> > > > > >> > I agree with this... > > > >> > > > > >> > > > > >> >> > To clarify, I don't
Re: [PATCH] tools/xenpvboot: remove as unable to convert to Python 3
On 22/11/2023 8:43 pm, Olaf Hering wrote: > Wed, 22 Nov 2023 20:30:59 + Andrew Cooper : > >> Does this mean there are SLES/OpenSUSE users of xenpvboot ? > We do not know. It is gone by now, in 4.18: > > https://build.opensuse.org/request/show/1126897 Yes. The email I replied to was the patch we merged into 4.18 deleting it because we thought noone had ever made it function for Py3, with the implication being that if this was going to be a problem, we could reinstate it with the fixes from your patch. But that entirely depends on whether you think anyone is using it or not. ~Andrew
Re: [PATCH] tools/xenpvboot: remove as unable to convert to Python 3
Wed, 22 Nov 2023 20:30:59 + Andrew Cooper : > Does this mean there are SLES/OpenSUSE users of xenpvboot ? We do not know. It is gone by now, in 4.18: https://build.opensuse.org/request/show/1126897 Olaf pgp4eQYvj2jrR.pgp Description: Digitale Signatur von OpenPGP
Re: [PATCH] tools/xenpvboot: remove as unable to convert to Python 3
On 06/10/2023 3:50 pm, Roger Pau Monne wrote: > The script heavily relies on the urlgrabber python module, which doesn't seem > to be packaged by all distros, it's missing from newer Debian versions at > least. > > Also the usage of the commands module has been deprecated since Python 2.6, > and > removed in Python 3, so the code would need to be re-written to not rely on > urlgrabber and the commands modules. > > Arguably the purpose of the xenpvnetboot bootloader can also be achieved with > an isolated script that fetches the kernel and ramdisk before attempting to > launch the domain, without having to run in libxl context. There are no > xl.cfg > options consumed by the bootloader apart from the base location and the > subpaths of the kernel and ramdisk to fetch. > > Any interested parties in keeping such functionality are free to submit an > updated script that works with Python 3. > > Signed-off-by: Roger Pau Monné I've just found https://build.opensuse.org/package/view_file/openSUSE:Leap:15.5:Update/xen/bin-python3-conversion.patch?expand=1 Which includes the modifications to make this work with Python 3. Does this mean there are SLES/OpenSUSE users of xenpvboot ? ~Andrew
Re: [PATCH v7 1/2] xen/vpci: header: status register handler
On 11/17/23 07:40, Roger Pau Monné wrote:
> On Wed, Sep 13, 2023 at 10:35:46AM -0400, Stewart Hildebrand wrote:
>> Introduce a handler for the PCI status register, with ability to mask the
>> capabilities bit. The status register contains RsvdZ bits, read-only bits,
>> and
>> write-1-to-clear bits, so introduce bitmasks to handle these in vPCI. If a
>> bit
>> in the bitmask is set, then the special meaning applies:
>>
>> rsvdz_mask: read as zero, guest write ignore (write zero to hardware)
>> ro_mask: read normal, guest write ignore (preserve on write to hardware)
>> rw1c_mask: read normal, write 1 to clear
>>
>> The RsvdZ naming was borrowed from the PCI Express Base 4.0 specification.
>>
>> Xen preserves the value of read-only bits on write to hardware, discarding
>> the
>> guests write value.
>>
>> The mask_cap_list flag will be set in a follow-on patch.
>>
>> Signed-off-by: Stewart Hildebrand
>> ---
>> v6->v7:
>> * re-work args passed to vpci_add_register_mask() (called in init_bars())
>> * also check for overlap of (rsvdz_mask & ro_mask) in add_register()
>> * slightly adjust masking operation in vpci_write_helper()
>>
>> v5->v6:
>> * remove duplicate PCI_STATUS_CAP_LIST in constant definition
>> * style fixup in constant definitions
>> * s/res_mask/rsvdz_mask/
>> * combine a new masking operation into single line
>> * preserve r/o bits on write
>> * get rid of status_read. Instead, use rsvdz_mask for conditionally masking
>> PCI_STATUS_CAP_LIST bit
>> * add comment about PCI_STATUS_CAP_LIST and rsvdp behavior
>> * add sanity checks in add_register
>> * move mask_cap_list from struct vpci_header to local variable
>>
>> v4->v5:
>> * add support for res_mask
>> * add support for ro_mask (squash ro_mask patch)
>> * add constants for reserved, read-only, and rw1c masks
>>
>> v3->v4:
>> * move mask_cap_list setting to the capabilities patch
>> * single pci_conf_read16 in status_read
>> * align mask_cap_list bitfield in struct vpci_header
>> * change to rw1c bit mask instead of treating whole register as rw1c
>> * drop subsystem prefix on renamed add_register function
>>
>> v2->v3:
>> * new patch
>> ---
>> xen/drivers/vpci/header.c | 16 +++
>> xen/drivers/vpci/vpci.c| 55 +-
>> xen/include/xen/pci_regs.h | 9 +++
>> xen/include/xen/vpci.h | 8 ++
>> 4 files changed, 76 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index 767c1ba718d7..af267b75ac31 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -521,6 +521,7 @@ static int cf_check init_bars(struct pci_dev *pdev)
>> struct vpci_header *header = >vpci->header;
>> struct vpci_bar *bars = header->bars;
>> int rc;
>> +bool mask_cap_list = false;
>>
>> switch ( pci_conf_read8(pdev->sbdf, PCI_HEADER_TYPE) & 0x7f )
>> {
>> @@ -544,6 +545,21 @@ static int cf_check init_bars(struct pci_dev *pdev)
>> if ( rc )
>> return rc;
>>
>> +/*
>> + * Utilize rsvdz_mask to hide PCI_STATUS_CAP_LIST from the guest for
>> now. If
>> + * support for rsvdp (reserved & preserved) is added in the future, the
>> + * rsvdp mask should be used instead.
>> + */
>> +rc = vpci_add_register_mask(pdev->vpci, vpci_hw_read16, vpci_hw_write16,
>> +PCI_STATUS, 2, NULL,
>> +PCI_STATUS_RSVDZ_MASK |
>> +(mask_cap_list ? PCI_STATUS_CAP_LIST :
>> 0),
>> +PCI_STATUS_RO_MASK &
>> +~(mask_cap_list ? PCI_STATUS_CAP_LIST :
>> 0),
>> +PCI_STATUS_RW1C_MASK);
>> +if ( rc )
>> +return rc;
>> +
>> if ( pdev->ignore_bars )
>> return 0;
>>
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index 3bec9a4153da..b4cde7db1b3f 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -29,6 +29,9 @@ struct vpci_register {
>> unsigned int offset;
>> void *private;
>> struct list_head node;
>> +uint32_t rsvdz_mask;
>> +uint32_t ro_mask;
>> +uint32_t rw1c_mask;
>
> I understand that we need the rw1c_mask in order to properly merge
> values when doing partial writes, but the other fields I'm not sure we
> do need them. RO bits don't care about what's written to them, and
> RsvdZ are always read as 0 and written as 0, so the mask shouldn't
> affect the merging.
See discussions at [1] and [2]. I'll keep ro_mask/rsvdz_mask, add rsvdp_mask,
and expand the commit message. For another data point, qemu
(hw/xen/xen_pt_config_init.c:xen_pt_emu_reg_header0) also has a similar way of
masking bits.
[1] https://lists.xenproject.org/archives/html/xen-devel/2023-11/msg01398.html
[2] https://lists.xenproject.org/archives/html/xen-devel/2023-11/msg01693.html
>
>> };
>>
>> #ifdef __XEN__
>> @@
Re: [PATCH 3/6] tools/pygrub: Restrict depriv operation with RLIMIT_AS
On 06/11/2023 3:05 pm, Alejandro Vallejo wrote: > diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub > index 327cf51774..b96bdfd849 100755 > --- a/tools/pygrub/src/pygrub > +++ b/tools/pygrub/src/pygrub > @@ -75,6 +80,11 @@ def downgrade_rlimits(): > resource.setrlimit(resource.RLIMIT_CORE, (0, 0)) > resource.setrlimit(resource.RLIMIT_MEMLOCK, (0, 0)) > > +max_ram_usage = LIMIT_AS > +if "PYGRUB_MAX_RAM_USAGE_MB" in os.environ.keys(): With the .keys() dropped as per patch 2.5/6, Acked-by: Andrew Cooper Happy to do this on commit.
Re: [PATCH 2/6] tools/pygrub: Fix bug in LIMIT_FSIZE env variable override
On 06/11/2023 3:05 pm, Alejandro Vallejo wrote:
> The env variable must be interpreted as an integer. As it is, the override
> logic simply causes an exception.
Fixes: e0342ae5556f ("tools/pygrub: Deprivilege pygrub")
> Signed-off-by: Alejandro Vallejo
> ---
> tools/pygrub/src/pygrub | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
> index 08540ad288..327cf51774 100755
> --- a/tools/pygrub/src/pygrub
> +++ b/tools/pygrub/src/pygrub
> @@ -89,7 +89,7 @@ def downgrade_rlimits():
> # write permissions are bound.
> fsize = LIMIT_FSIZE
> if "PYGRUB_MAX_FILE_SIZE_MB" in os.environ.keys():
> -fsize = os.environ["PYGRUB_MAX_FILE_SIZE_MB"] << 20
> +fsize = int(os.environ["PYGRUB_MAX_FILE_SIZE_MB"]) << 20
>
> resource.setrlimit(resource.RLIMIT_FSIZE, (fsize, fsize))
>
This change on its own is correct, so Acked-by: Andrew Cooper
However, there's a bug/misfeature which you've copied in patch 3, so
I've inserted a patch 2.5 to try and fix it in a nice order. It's
probably a little rude to merge the pythonic-fix into this functional fix.
~Andrew
[PATCH v2.5/6] tools/pygrub: Fix expression before it's copied elsewhere
This has an identical meaning, and is the more pythonic way of writing it. Signed-off-by: Andrew Cooper --- CC: Wei Liu CC: Anthony PERARD CC: Alejandro Vallejo --- tools/pygrub/src/pygrub | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub index 327cf51774fc..2c06684d6532 100755 --- a/tools/pygrub/src/pygrub +++ b/tools/pygrub/src/pygrub @@ -88,7 +88,7 @@ def downgrade_rlimits(): # filesystem we set RLIMIT_FSIZE to a high bound, so that the file # write permissions are bound. fsize = LIMIT_FSIZE -if "PYGRUB_MAX_FILE_SIZE_MB" in os.environ.keys(): +if "PYGRUB_MAX_FILE_SIZE_MB" in os.environ: fsize = int(os.environ["PYGRUB_MAX_FILE_SIZE_MB"]) << 20 resource.setrlimit(resource.RLIMIT_FSIZE, (fsize, fsize)) -- 2.30.2
Re: [PATCH 1/6] tools/pygrub: Set mount propagation to private recursively
On 22/11/2023 7:46 pm, Andrew Cooper wrote:
> On 06/11/2023 3:05 pm, Alejandro Vallejo wrote:
>> This is important in order for every mount done inside a mount namespace to
>> go away after the namespace itself goes away. The comment referring to
>> unreliability in Linux 4.19 was just wrong.
>>
>> This patch sets the story straight and makes the depriv pygrub a bit more
>> confined should a layer of the onion be vulnerable.
>>
>> Signed-off-by: Alejandro Vallejo
> Acked-by: Andrew Cooper
Sorry, wants
Fixes: e0342ae5556f ("tools/pygrub: Deprivilege pygrub")
too. Will fix on commit.
~Andrew
Re: [PATCH 1/6] tools/pygrub: Set mount propagation to private recursively
On 06/11/2023 3:05 pm, Alejandro Vallejo wrote: > This is important in order for every mount done inside a mount namespace to > go away after the namespace itself goes away. The comment referring to > unreliability in Linux 4.19 was just wrong. > > This patch sets the story straight and makes the depriv pygrub a bit more > confined should a layer of the onion be vulnerable. > > Signed-off-by: Alejandro Vallejo Acked-by: Andrew Cooper
Re: [PATCH] x86/mem_sharing: Release domain if we are not able to enable memory sharing
On 22/11/2023 4:39 pm, Frediano Ziglio wrote:
> In case it's not possible to enable memory sharing (mem_sharing_control
> fails) we just return the error code without releasing the domain
> acquired some lines above by rcu_lock_live_remote_domain_by_id.
Fixes: 72f8d45d69b8 ("x86/mem_sharing: enable mem_sharing on first memop")
> Signed-off-by: Frediano Ziglio
Reviewed-by: Andrew Cooper
That was simply a broken transformation in the patch.
Re: Enabling more than one HVC console on Arm64 platform
Hi Ayan,
On 22/11/2023 20:00, Ayan Kumar Halder wrote:
> Hi Amit/All,
>
> We came across this scenario and would be helpful if you can provide
> some pointers
>
>
> Linux running as Dom0 on Xen hypervisor with HVC_DCC = y and HVC_XEN = y
> on Arm64 platform.
>
> This causes a crash when Dom0 tries to access "DBGDTRTX_EL0" register,
> it traps to Xen.
>
> Xen does not emulate this register so it crashes.
>
> |Logs - https://paste.debian.net/1298983/|
>
> |
> |
>
> |My aim is to avoid the crash and let Xen boot Dom0 even though there
> might not be a debug console available.|
>
> |So, I am trying to add emulation for |"DBGDTRTX_EL0" register in Xen.
>
>
> As a quick trial (may be not the perfect solution), I am trying to
> emulate this register as "read as zero/write ignore" (similar to KVM).
>
> However, I could not see logs to the Xen console (ie HVC0).
>
>
> So my question is
>
> 1. Is it possible in Linux to probe more than one HVC console so that
> Linux can put the same logs in HVC_DCC and HVC_XEN drivers ?
>
> So that the user can always see the logs in the default Xen console (ie
> hvc0) even if the debug console is not present.
If you have both HVC DCC and Xen enabled and DCC gets assigned hvc0, just use
console=hvc1
to see the messages from Xen.
>
>
> Another possible alternative I am exploring is to enable trapping for
> read of MDCCSR_EL0 in Xen, so that Xen returns with MDCCSR_EL0.TXfull
> set to 1.
>
> static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
> {
> int i;
>
> for (i = 0; i < count; i++) {
> while (__dcc_getstatus() & DCC_STATUS_TX)
> cpu_relax(); <<--- So this will be invoked.
FWICS, this won't be invoked given that in init call hvc_dcc_check() will
return false
due to condition "if (!(__dcc_getstatus() & DCC_STATUS_TX))" being false. This
is the whole
point to make Linux return -ENODEV quickly.
~Michal
Re: [PATCH v2 4/5] arm/efi: Simplify efi_arch_handle_cmdline()
On Wed, 22 Nov 2023, Andrew Cooper wrote: > On 22/11/2023 3:49 pm, Luca Fancellu wrote: > > > >> On 21 Nov 2023, at 20:41, Andrew Cooper wrote: > >> > >> On 21/11/2023 8:33 pm, Luca Fancellu wrote: > >>> + CC henry > >>> > On 21 Nov 2023, at 20:15, Andrew Cooper > wrote: > > -Wwrite-strings is unhappy with assigning "xen" to a mutable pointer, > but this > logic looks incorrect. It was inherited from the x86 side, where the > logic > was redundant and has now been removed. > > In the ARM case it inserts the image name into "xen,xen-bootargs" and > there is > no logic at all to strip this before parsing it as the command line. > > The absence of any logic to strip an image name suggests that it > shouldn't > exist there, or having a Xen image named e.g. "hmp-unsafe" in the > filesystem > is going to lead to some unexpected behaviour on boot. > > No functional change. > > Signed-off-by: Andrew Cooper > --- > CC: Jan Beulich > CC: Roger Pau Monné > CC: Wei Liu > CC: Stefano Stabellini > CC: Julien Grall > CC: Volodymyr Babchuk > CC: Bertrand Marquis > CC: Roberto Bagnara > CC: Nicola Vetrini > > v2: > * New. > > I'm afraid that all of this reasoning is based on reading the source > code. I > don't have any way to try this out in a real ARM UEFI environment. > >>> I will test this one tomorrow on an arm board > > I confirm that booting though UEFI on an arm board works > > > > Reviewed-by: Luca Fancellu > > Tested-by: Luca Fancellu > > Thanks, and you confirmed that the first cmdline parameter is still usable? Assuming Luca or Henry come back with a confirmation that the first command line parameter is still passed through correctly: Reviewed-by: Stefano Stabellini
Re: [PATCH v2 1/5] x86/setup: Clean up cmdline handling in create_dom0()
On 22/11/2023 4:20 pm, Andrew Cooper wrote:
> On 22/11/2023 9:02 am, Jan Beulich wrote:
>> On 21.11.2023 21:15, Andrew Cooper wrote:
>>> @@ -913,33 +914,30 @@ static struct domain *__init create_dom0(const
>>> module_t *image,
>>> panic("Error creating d%uv0\n", domid);
>>>
>>> /* Grab the DOM0 command line. */
>>> -cmdline = image->string ? __va(image->string) : NULL;
>>> -if ( cmdline || kextra )
>>> +if ( image->string || kextra )
>>> {
>>> -static char __initdata dom0_cmdline[MAX_GUEST_CMDLINE];
>>> -
>>> -cmdline = cmdline_cook(cmdline, loader);
>>> -safe_strcpy(dom0_cmdline, cmdline);
>>> +if ( image->string )
>>> +safe_strcpy(cmdline, cmdline_cook(__va(image->string),
>>> loader));
>>>
>>> if ( kextra )
>>> /* kextra always includes exactly one leading space. */
>>> -safe_strcat(dom0_cmdline, kextra);
>>> +safe_strcat(cmdline, kextra);
>>>
>>> /* Append any extra parameters. */
>>> -if ( skip_ioapic_setup && !strstr(dom0_cmdline, "noapic") )
>>> -safe_strcat(dom0_cmdline, " noapic");
>>> +if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
>>> +safe_strcat(cmdline, " noapic");
>>> +
>>> if ( (strlen(acpi_param) == 0) && acpi_disabled )
>>> {
>>> printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
>>> safe_strcpy(acpi_param, "off");
>>> }
>>> -if ( (strlen(acpi_param) != 0) && !strstr(dom0_cmdline, "acpi=") )
>>> +
>>> +if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
>> As you're touching this anyway, how about replacing the strlen() by just
>> *acpi_param? We don't really care exactly how long the string is. (As an
>> aside, strstr() uses like the one here are of course also pretty fragile.
>> But of course that's nothing to care about in this change.)
> There's the same pattern just above it, not touched, and this patch is
> already getting complicated.
>
> I think there's other cleanup to do here, so I'll defer it to later.
It turns out that the optimiser already makes this transformation.
I shan't admit to how long I've just spent thinking I had a problem with
my build...
~Andrew
Enabling more than one HVC console on Arm64 platform
Hi Amit/All,
We came across this scenario and would be helpful if you can provide
some pointers
Linux running as Dom0 on Xen hypervisor with HVC_DCC = y and HVC_XEN = y
on Arm64 platform.
This causes a crash when Dom0 tries to access "DBGDTRTX_EL0" register,
it traps to Xen.
Xen does not emulate this register so it crashes.
|Logs - https://paste.debian.net/1298983/|
|
|
|My aim is to avoid the crash and let Xen boot Dom0 even though there
might not be a debug console available.|
|So, I am trying to add emulation for |"DBGDTRTX_EL0" register in Xen.
As a quick trial (may be not the perfect solution), I am trying to
emulate this register as "read as zero/write ignore" (similar to KVM).
However, I could not see logs to the Xen console (ie HVC0).
So my question is
1. Is it possible in Linux to probe more than one HVC console so that
Linux can put the same logs in HVC_DCC and HVC_XEN drivers ?
So that the user can always see the logs in the default Xen console (ie
hvc0) even if the debug console is not present.
Another possible alternative I am exploring is to enable trapping for
read of MDCCSR_EL0 in Xen, so that Xen returns with MDCCSR_EL0.TXfull
set to 1.
static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
{
int i;
for (i = 0; i < count; i++) {
while (__dcc_getstatus() & DCC_STATUS_TX)
cpu_relax(); <<--- So this will be invoked.
__dcc_putchar(buf[i]);
}
return count;
}
Any pointers are highly appreciated.
Kind regards,
Ayan
Testing Xen v4.17 on ARMv8
Dear Sir or Madam, I am a firmware developer working on ARM-based SoC. We have recently added Xen support (v4.17) to our firmware. Now I am trying to find a test framework for routine testing of Xen on our SoCs. Unfortunately, wiki.xenproject.org is not very up-to-date, and the situation with tests (https://wiki.xenproject.org/wiki/Testing_Xen, https://wiki.xenproject.org/wiki/Category:Test_Day) is even worse. As far as I can see, the latest tested version of Xen mentioned there is 4.15. I found the Smoke test ( https://wiki.xenproject.org/wiki/Xen_ARM_Manual_Smoke_Test). It passes successfully, but its coverage is quite low. Besides that, I have found the Xen OSSTest framework ( http://xenbits.xen.org/gitweb/?p=osstest.git;a=summary). If I'm not mistaken, it was written for internal use approximately in 2013. But currently, I have not managed to make the framework usable for individual ad-hoc testing in standalone mode for ARM64. Also there is mentioned XenRT test ( https://wiki.xenproject.org/wiki/Category:XenRT), but there are no *working* links for its source code. Could you kindly tell me: 1. Which tests or test suites would you recommend for testing Xen (including regression tests)? 2. Where can I find source code of the XenRT test framework? 3. Is there any clear and up-to-date manual on using and porting the Osstest? Sincerely yours, Dmitry Zyuzin, firmware developer.
Re: [PATCH v2 4/5] arm/efi: Simplify efi_arch_handle_cmdline()
On 22/11/2023 3:49 pm, Luca Fancellu wrote: > >> On 21 Nov 2023, at 20:41, Andrew Cooper wrote: >> >> On 21/11/2023 8:33 pm, Luca Fancellu wrote: >>> + CC henry >>> On 21 Nov 2023, at 20:15, Andrew Cooper wrote: -Wwrite-strings is unhappy with assigning "xen" to a mutable pointer, but this logic looks incorrect. It was inherited from the x86 side, where the logic was redundant and has now been removed. In the ARM case it inserts the image name into "xen,xen-bootargs" and there is no logic at all to strip this before parsing it as the command line. The absence of any logic to strip an image name suggests that it shouldn't exist there, or having a Xen image named e.g. "hmp-unsafe" in the filesystem is going to lead to some unexpected behaviour on boot. No functional change. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Roger Pau Monné CC: Wei Liu CC: Stefano Stabellini CC: Julien Grall CC: Volodymyr Babchuk CC: Bertrand Marquis CC: Roberto Bagnara CC: Nicola Vetrini v2: * New. I'm afraid that all of this reasoning is based on reading the source code. I don't have any way to try this out in a real ARM UEFI environment. >>> I will test this one tomorrow on an arm board > I confirm that booting though UEFI on an arm board works > > Reviewed-by: Luca Fancellu > Tested-by: Luca Fancellu Thanks, and you confirmed that the first cmdline parameter is still usable? ~Andrew
Re: [PATCH 6/7] tools/xg: Simplify xc_cpuid_xend_policy() and xc_msr_policy()
Hi, while rebasing with something else I noticed that... On Tue, Nov 07, 2023 at 03:49:20PM +, Alejandro Vallejo wrote: > Remove all duplication in CPUID and MSR xend-style overrides. They had an > incredible amount of overhead for no good reason. > > After this patch, CPU policy application takes a number of hypercalls to > recover the policy state and then those are passed to the xend-style > override code so it can avoid them. > > Furthermore, the ultimate reapplication of the policy to the domain in Xen > is done only once after both CPUID and MSRs have been fixed up. > > BUG!!! apply_policy is sending the policy after deserialise when it poked > at it in its serialised form. > > Signed-off-by: Alejandro Vallejo > --- > tools/libs/guest/xg_cpuid_x86.c | 261 +--- > 1 file changed, 38 insertions(+), 223 deletions(-) there's a blunder in the commit message. This was a note to myself to fix something while in mid-dev, and I did, but then didn't remove the paragraph from the commit message. Obviously those last 2 lines before the S-by shouldn't be there. Cheers, Alejandro
Re: [PATCH 3/3] xen/MISRA: Remove nonstandard inline keywords
On 22/11/23 15:27, Andrew Cooper wrote:
The differences between inline, __inline and __inline__ keywords are a
vestigial remnant of older C standards, and in Xen we use inline almost
exclusively.
Replace __inline and __inline__ with regular inline, and remove their
exceptions from the MISRA configuration.
Signed-off-by: Andrew Cooper
--- > CC: Jan Beulich
CC: Roger Pau Monné
CC: Wei Liu
CC: Stefano Stabellini
CC: Roberto Bagnara
CC: Nicola Vetrini
CC: Simone Ballarin
I'm entirely guessing at the Eclair configuration.
---
.../eclair_analysis/ECLAIR/toolchain.ecl | 6 +++---
docs/misra/C-language-toolchain.rst | 2 +-
xen/arch/x86/include/asm/apic.h | 20 +--
xen/include/acpi/cpufreq/cpufreq.h| 4 ++--
xen/include/xen/bitops.h | 4 ++--
xen/include/xen/compiler.h| 7 +++
6 files changed, 21 insertions(+), 22 deletions(-)
diff --git a/automation/eclair_analysis/ECLAIR/toolchain.ecl
b/automation/eclair_analysis/ECLAIR/toolchain.ecl
index e6cd289b5e92..71a1e2cce029 100644
--- a/automation/eclair_analysis/ECLAIR/toolchain.ecl
+++ b/automation/eclair_analysis/ECLAIR/toolchain.ecl
@@ -15,7 +15,7 @@
_Static_assert: see Section \"2.1 C Language\" of "GCC_MANUAL".
asm, __asm__: see Sections \"6.48 Alternate Keywords\" and \"6.47 How to Use Inline
Assembly Language in C Code\" of "GCC_MANUAL".
__volatile__: see Sections \"6.48 Alternate Keywords\" and \"6.47.2.1 Volatile\" of
"GCC_MANUAL".
-__const__, __inline__, __inline: see Section \"6.48 Alternate Keywords\" of
"GCC_MANUAL".
+__const__ : see Section \"6.48 Alternate Keywords\" of "GCC_MANUAL".
typeof, __typeof__: see Section \"6.7 Referring to a Type with typeof\" of
"GCC_MANUAL".
__alignof__, __alignof: see Sections \"6.48 Alternate Keywords\" and \"6.44 Determining
the Alignment of Functions, Types or Variables\" of "GCC_MANUAL".
__attribute__: see Section \"6.39 Attribute Syntax\" of "GCC_MANUAL".
@@ -23,8 +23,8 @@
__builtin_va_arg: non-documented GCC extension.
__builtin_offsetof: see Section \"6.53 Support for offsetof\" of
"GCC_MANUAL".
"
--config=STD.tokenext,behavior+={c99, GCC_ARM64,
"^(_Static_assert|asm|__asm__|__volatile__|__const__|__inline__|typeof|__typeof__|__alignof__|__attribute__|__builtin_types_compatible_p|__builtin_va_arg|__builtin_offsetof)$"}
--config=STD.tokenext,behavior+={c99, GCC_X86_64,
"^(_Static_assert|asm|__asm__|__volatile__|__const__|__inline__|__inline|typeof|__typeof__|__alignof__|__alignof|__attribute__|__builtin_types_compatible_p|__builtin_va_arg|__builtin_offsetof)$"}
+-config=STD.tokenext,behavior+={c99, GCC_ARM64,
"^(_Static_assert|asm|__asm__|__volatile__|__const__|typeof|__typeof__|__alignof__|__attribute__|__builtin_types_compatible_p|__builtin_va_arg|__builtin_offsetof)$"}
+-config=STD.tokenext,behavior+={c99, GCC_X86_64,
"^(_Static_assert|asm|__asm__|__volatile__|__const__|typeof|__typeof__|__alignof__|__alignof|__attribute__|__builtin_types_compatible_p|__builtin_va_arg|__builtin_offsetof)$"}
-doc_end
-doc_begin="Non-documented GCC extension."
diff --git a/docs/misra/C-language-toolchain.rst
b/docs/misra/C-language-toolchain.rst
index 2866cb191b1a..b7c2000992ac 100644
--- a/docs/misra/C-language-toolchain.rst
+++ b/docs/misra/C-language-toolchain.rst
@@ -84,7 +84,7 @@ The table columns are as follows:
see Sections "6.48 Alternate Keywords" and "6.47 How to Use Inline
Assembly Language in C Code" of GCC_MANUAL.
__volatile__:
see Sections "6.48 Alternate Keywords" and "6.47.2.1 Volatile" of
GCC_MANUAL.
- __const__, __inline__, __inline:
+ __const__:
see Section "6.48 Alternate Keywords" of GCC_MANUAL.
typeof, __typeof__:
see Section "6.7 Referring to a Type with typeof" of GCC_MANUAL.
diff --git a/xen/arch/x86/include/asm/apic.h b/xen/arch/x86/include/asm/apic.h
index 486d689478b2..b20fae7ebc6a 100644
--- a/xen/arch/x86/include/asm/apic.h
+++ b/xen/arch/x86/include/asm/apic.h
@@ -49,12 +49,12 @@ const struct genapic *apic_x2apic_probe(void);
* Basic functions accessing APICs.
*/
-static __inline void apic_mem_write(unsigned long reg, u32 v)
+static inline void apic_mem_write(unsigned long reg, u32 v)
{
*((volatile u32 *)(APIC_BASE+reg)) = v;
}
-static __inline u32 apic_mem_read(unsigned long reg)
+static inline u32 apic_mem_read(unsigned long reg)
{
return *((volatile u32 *)(APIC_BASE+reg));
}
@@ -63,7 +63,7 @@ static __inline u32 apic_mem_read(unsigned long reg)
* access the 64-bit ICR register.
*/
-static __inline void apic_wrmsr(unsigned long reg, uint64_t msr_content)
+static inline void apic_wrmsr(unsigned long reg, uint64_t msr_content)
{
if (reg == APIC_DFR || reg == APIC_ID || reg == APIC_LDR ||
reg == APIC_LVR)
@@ -72,7 +72,7 @@ static __inline void
Re: [PATCH v3] xen/x86: On x2APIC mode, derive LDR from APIC ID
On Wed, Nov 22, 2023 at 04:08:17PM +, Alejandro Vallejo wrote:
> Both Intel and AMD manuals agree that on x2APIC mode, the APIC LDR and ID
> registers are derivable from each other through a fixed formula.
>
> Xen uses that formula, but applies it to vCPU IDs (which are sequential)
> rather than x2APIC IDs (which are not, at the moment). As I understand it,
> this is an attempt to tightly pack vCPUs into clusters so each cluster has
> 16 vCPUs rather than 8, but this is problematic for OSs that might read the
> x2APIC ID and internally derive LDR (or the other way around)
>
> This patch fixes the implementation so we follow the rules in the x2APIC
> spec(s) and covers migrations from broken hypervisors, so LDRs are
> preserved even for hotppluggable CPUs and across APIC resets.
>
> While touching that area, I removed the existing printk statement in
> vlapic_load_fixup() (as the checks it performed didn't make sense in x2APIC
> mode and wouldn't affect the outcome) and put another printk as an else
> branch so we get warnings trying to load nonsensical LDR values we don't
> know about.
>
> Fixes: f9e0cccf7b35 ("x86/HVM: fix ID handling of x2APIC emulation")
> Signed-off-by: Alejandro Vallejo
> ---
> v3:
> * Removed underscores from (x2)APIC_ID in commit message
> * Added commit msg explaining the removal of the vlapic_load_fixup() printk
> * Restored lowercase to hex "F"s
> * Refactored a bit vlapic_load_fixup() so it has a trailing printk in
> case of spotting a nonsensical LDR it doesn't understand.
> * Moved field in domain.h with the other booleans
> * Trimmed down field name in domain.h
> * Trimmed down field comment in domain.h
>
> If the field name in domain.h still seems too long I'm happy for it to be
> trimmed further down, but I do want to preserve the "_bug_" part of it.
I sent this one before Roger had a chance to reply to my reply on v2, which
was...
>
> OK, if you think mentioning `bug` is helpful, I think it would be best
> placed as a prefix: bug_x2apic_ldr_vcpu_id. Having it in the middle
> of the field name makes it harder to spot.
>
> Thanks, Roger.
... and I'm fine with that adjustment here.
Cheers,
Alejandro
Re: [PATCH v2 4/6] xen_pvdev: Do not assume Dom0 when creating a directory
On 21/11/2023 22:10, Volodymyr Babchuk wrote:
From: Oleksandr Tyshchenko
Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
inherit the owner of the directory.
Ah... so that's why the previous patch is there.
This is not the right way to fix it. The QEMU Xen support is *assuming*
that QEMU is either running in, or emulating, dom0. In the emulation
case this is probably fine, but the 'real Xen' case it should be using
the correct domid for node creation. I guess this could either be
supplied on the command line or discerned by reading the local domain
'domid' node.
Note that for other than Dom0 domain (non toolstack domain) the
"driver_domain" property should be set in domain config file for the
toolstack to create required directories in advance.
Signed-off-by: Oleksandr Tyshchenko
Signed-off-by: Volodymyr Babchuk
---
hw/xen/xen_pvdev.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
index c5ad71e8dc..42bdd4f6c8 100644
--- a/hw/xen/xen_pvdev.c
+++ b/hw/xen/xen_pvdev.c
@@ -60,7 +60,8 @@ void xen_config_cleanup(void)
int xenstore_mkdir(char *path, int p)
{
-if (!qemu_xen_xs_create(xenstore, 0, 0, xen_domid, p, path)) {
+if (!qemu_xen_xs_create(xenstore, 0, XS_PRESERVE_OWNER,
+xen_domid, p, path)) {
xen_pv_printf(NULL, 0, "xs_mkdir %s: failed\n", path);
return -1;
}
[xen-unstable-smoke test] 183826: tolerable all pass - PUSHED
flight 183826 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/183826/ 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 c22fe7213c9b1f99cbc64c33e391afa223f9cd08 baseline version: xen 820ee3ec4dd5679715bd49a1d12b81cb1502260c Last test of basis 183817 2023-11-22 02:03:46 Z0 days Testing same since 183826 2023-11-22 14:00:36 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Christian Lindig Henry Wang Jan Beulich Juergen Gross 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 820ee3ec4d..c22fe7213c c22fe7213c9b1f99cbc64c33e391afa223f9cd08 -> smoke
Re: [PATCH v2 3/6] xen: xenstore: add possibility to preserve owner
On 21/11/2023 22:10, Volodymyr Babchuk wrote: Add option to preserve owner when creating an entry in Xen Store. This may be needed in cases when Qemu is working as device model in a *may* be needed? I don't undertstand why this patch is necessary and the commit comment isn't really helping me. domain that is Domain-0, e.g. in driver domain. "owner" parameter for qemu_xen_xs_create() function can have special value XS_PRESERVE_OWNER, which will make specific implementation to get original owner of an entry and pass it back to set_permissions() call. Please note, that XenStore inherits permissions, so even if entry is newly created by, it already has the owner set to match owner of entry at previous level. Signed-off-by: Volodymyr Babchuk
Re: [PATCH v2 1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it
On 21/11/2023 22:10, Volodymyr Babchuk wrote: From: David Woodhouse This allows a XenDevice implementation to know whether it was created by QEMU, or merely discovered in XenStore after the toolstack created it. This will allow us to create frontend/backend nodes only when we should, rather than unconditionally attempting to overwrite them from a driver domain which doesn't have privileges to do so. As an added benefit, it also means we no longer have to call the xen_backend_set_device() function from the device models immediately after calling qdev_realize_and_unref(). Even though we could make the argument that it's safe to do so, and the pointer to the unreffed device *will* actually still be valid, it still made my skin itch to look at it. Signed-off-by: David Woodhouse --- hw/block/xen-block.c | 3 +-- hw/char/xen_console.c| 2 +- hw/net/xen_nic.c | 2 +- hw/xen/xen-bus.c | 4 include/hw/xen/xen-backend.h | 2 -- include/hw/xen/xen-bus.h | 2 ++ 6 files changed, 9 insertions(+), 6 deletions(-) Actually, I think you should probably update xen_backend_try_device_destroy() in this patch too. It currently uses xen_backend_list_find() to check whether the specified XenDevice has an associated XenBackendInstance. Paul
Re: [PATCH v2] xen/x86: On x2APIC mode, derive LDR from APIC_ID
On Wed, Nov 22, 2023 at 03:11:52PM +, Alejandro Vallejo wrote:
> On Wed, Nov 22, 2023 at 02:40:02PM +0100, Roger Pau Monné wrote:
> > On Tue, Nov 21, 2023 at 04:26:04PM +, Alejandro Vallejo wrote:
> > > diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> > > index a8e87c4446..7f169f1e5f 100644
> > > --- a/xen/arch/x86/hvm/vlapic.c
> > > +++ b/xen/arch/x86/hvm/vlapic.c
> > > @@ -1061,13 +1061,23 @@ static const struct hvm_mmio_ops vlapic_mmio_ops
> > > = {
> > > .write = vlapic_mmio_write,
> > > };
> > >
> > > +static uint32_t x2apic_ldr_from_id(uint32_t id)
> > > +{
> > > +return ((id & ~0xF) << 12) | (1 << (id & 0xF));
> >
> > Seeing other usages in vlapic.c I think the preference is to use lower
> > case for hex numbers.
> I thought it was covered by a MISRA rule, but it seems to apply only to the
> literal suffixes. Fair enough, I'll revert to lowercase.
FWIW, I'm thinking that I want to move x2apic_ldr_from_id() to a
header file, so that it can also be used by the native APIC driver in
order to detect when the LDR field is not configured according to the
spec (so adding a consistency check in
init_apic_ldr_x2apic_cluster()).
Anyway, might look into doing that after this patch is in.
> > > +else if ( bad_ldr == vlapic->loaded.ldr )
> > > /*
> > > - * This is optional: ID != 0 contradicts LDR == 1. It's being
> > > added
> > > - * to aid in eventual debugging of issues arising from the fixup
> > > done
> > > - * here, but can be dropped as soon as it is found to conflict
> > > with
> > > - * other (future) changes.
> > > + * This is a migration from a broken Xen between 4.4 and 4.18
> > > and we
> > > + * must _PRESERVE_ LDRs so new vCPUs use consistent derivations.
> > > In
> > > + * this case we set this domain boolean so future CPU hotplugs
> > > + * derive an LDR consistent with the older Xen's broken idea of
> > > + * consistency.
> > > */
> > > -if ( GET_xAPIC_ID(id) != vlapic_vcpu(vlapic)->vcpu_id * 2 ||
> > > - id != SET_xAPIC_ID(GET_xAPIC_ID(id)) )
> > > -printk(XENLOG_G_WARNING "%pv: bogus APIC ID %#x loaded\n",
> > > - vlapic_vcpu(vlapic), id);
> ... these. But they _seem_ bogus for several reasons. And I just got rid of
> them on these grounds:
>
> * It's using the GET_xAPIC_ID() macro on the register, but the LAPIC is
> not in xAPIC mode (due to the previous check), so the legacy APIC must
> be derived from the lowest octet, not the highest. That macro is meant
> to be used on the MMIO register, not the MSR one.
> * The printk (wants to be) triggered when the ID field is not "canonical"
> OR the x2APIC ID is not a pure xAPIC ID. Neither of these things are
> problems per se, the former just happens to be true at the moment, but
> the latter may very well not be, and that shouldn't trigger a scary
> printk.
> * The error message seems to imply the effective APIC ID eventually left
> loaded is the bogus one, but that's not the case.
>
> Actually, I might just move the printk into a separate else in line with
> your previous suggestion.
Sounds good. And I agree that using {GET,SET}_xAPIC_ID() on the
x2APIC 32bit width IDs looks to be wrong.
> > > static int cf_check lapic_load_hidden(struct domain *d,
> > > hvm_domain_context_t *h)
> > > diff --git a/xen/arch/x86/include/asm/hvm/domain.h
> > > b/xen/arch/x86/include/asm/hvm/domain.h
> > > index 6e53ce4449..a42a6e99bb 100644
> > > --- a/xen/arch/x86/include/asm/hvm/domain.h
> > > +++ b/xen/arch/x86/include/asm/hvm/domain.h
> > > @@ -61,6 +61,19 @@ struct hvm_domain {
> > > /* Cached CF8 for guest PCI config cycles */
> > > uint32_tpci_cf8;
> > >
> > > +/*
> > > + * Xen had a bug between 4.4 and 4.18 by which the x2APIC LDR was
> > > + * derived from the vcpu_id rather than the x2APIC ID. This is wrong,
> > > + * but changing this behaviour is tricky because guests might have
> > > + * already read the LDR and used it accordingly. In the interest of
> > > not
> > > + * breaking migrations from those hypervisors we track here whether
> > > + * this domain suffers from this or not so a hotplugged vCPU or an
> > > APIC
> > > + * reset can recover the same LDR it would've had on the older host.
> > > + *
> > > + * Yuck!
> > > + */
> > > +bool has_inconsistent_x2apic_ldr_bug;
> >
> > Could you place the new field after the existing boolean fields in the
> > struct? (AFAICT there's plenty of padding left there)
> Sure. I placed it somewhere with unused padding. (that u32 is sandwiched
> between an "unsigned long" and a pointer), but I'm happy to stash it with
> the other booleans.
Yes, there's plenty of padding but I feel it's better to place it with
the rest of the booleans, as there's also padding there.
> >
> > I also think the field
Re: [PATCH v2 2/6] xen: backends: touch some XenStore nodes only if device...
On 21/11/2023 22:10, Volodymyr Babchuk wrote: was created by QEMU Xen PV devices in QEMU can be created in two ways: either by QEMU itself, if they were passed via command line, or by Xen toolstack. In the latter case, QEMU scans XenStore entries and configures devices accordingly. In the second case we don't want QEMU to write/delete front-end entries for two reasons: it might have no access to those entries if it is running in un-privileged domain and it is just incorrect to overwrite entries already provided by Xen toolstack, because toolstack manages those nodes. For example, it might read backend- or frontend- state to be sure that they are both disconnected and it is safe to destroy a domain. This patch checks presence of xendev->backend to check if Xen PV device is acting as a backend (i.e. it was configured by Xen Technally *all* XenDevice objects are backends. toolstack) to decide if it should touch frontend entries in XenStore. Also, when we need to remove XenStore entries during device teardown only if they weren't created by Xen toolstack. If they were created by toolstack, then it is toolstack's job to do proper clean-up.
[PATCH] x86/ACPI: constify acpi_enter_sleep argument
Minor style change, structure is not changed.
No functional change.
Signed-off-by: Frediano Ziglio
---
xen/arch/x86/acpi/power.c | 2 +-
xen/arch/x86/include/asm/acpi.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 81233738b1..861d12aab0 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -342,7 +342,7 @@ static long cf_check enter_state_helper(void *data)
* Dom0 issues this hypercall in place of writing pm1a_cnt. Xen then
* takes over the control and put the system into sleep state really.
*/
-int acpi_enter_sleep(struct xenpf_enter_acpi_sleep *sleep)
+int acpi_enter_sleep(const struct xenpf_enter_acpi_sleep *sleep)
{
if ( sleep->sleep_state == ACPI_STATE_S3 &&
(!acpi_sinfo.wakeup_vector || !acpi_sinfo.vector_width ||
diff --git a/xen/arch/x86/include/asm/acpi.h b/xen/arch/x86/include/asm/acpi.h
index 6ce79ce465..68cee10f9a 100644
--- a/xen/arch/x86/include/asm/acpi.h
+++ b/xen/arch/x86/include/asm/acpi.h
@@ -106,7 +106,7 @@ extern s8 acpi_numa;
extern struct acpi_sleep_info acpi_sinfo;
#define acpi_video_flags bootsym(video_flags)
struct xenpf_enter_acpi_sleep;
-extern int acpi_enter_sleep(struct xenpf_enter_acpi_sleep *sleep);
+extern int acpi_enter_sleep(const struct xenpf_enter_acpi_sleep *sleep);
extern int acpi_enter_state(u32 state);
struct acpi_sleep_info {
--
2.34.1
Re: [PATCH 3/3] xen/MISRA: Remove nonstandard inline keywords
On 22/11/2023 4:39 pm, Nicola Vetrini wrote: > On 2023-11-22 15:27, Andrew Cooper wrote: >> The differences between inline, __inline and __inline__ keywords are a >> vestigial remnant of older C standards, and in Xen we use inline almost >> exclusively. >> >> Replace __inline and __inline__ with regular inline, and remove their >> exceptions from the MISRA configuration. >> >> Signed-off-by: Andrew Cooper >> --- >> CC: Jan Beulich >> CC: Roger Pau Monné >> CC: Wei Liu >> CC: Stefano Stabellini >> CC: Roberto Bagnara >> CC: Nicola Vetrini >> CC: Simone Ballarin >> >> I'm entirely guessing at the Eclair configuration. >> --- > > The configuration changes are ok. One observation below. Thanks. Can I take that as an Ack/Reviewed-by ? >> diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h >> index 04b8bc18df0e..16d554f2a593 100644 >> --- a/xen/include/xen/compiler.h >> +++ b/xen/include/xen/compiler.h >> @@ -83,7 +82,7 @@ >> * inline functions not expanded inline get placed in .init.text. >> */ >> #include >> -#define __inline__ __inline__ __init >> +#define inline inline __init > > The violation of Rule 20.4 (A macro shall not be defined with the same > name as a keyword) is still present due to this macro. I was expecting this to come up. There's a comment half out of context above, but to expand on it, we have a feature in the build system where if you say obj-y += foo.init.o then it gets compiled as normal and then all symbols checked for being in the relevant .init sections. It's a safeguard around init-only code ending up in the runtime image (which is good for other goals of safety). This particular define is necessary to cause out-of-lined static inlines to end up in the right section, without having to invent a new __inline_or_init macro and rewriting half the header files in the project. I think it's going to need a local deviation. It's deliberate, and all we're doing is using the inline keyword to hook in an extra __section() attribute. ~Andrew
[PATCH] x86/mem_sharing: Release domain if we are not able to enable memory sharing
In case it's not possible to enable memory sharing (mem_sharing_control
fails) we just return the error code without releasing the domain
acquired some lines above by rcu_lock_live_remote_domain_by_id.
Signed-off-by: Frediano Ziglio
--
I didn't manage to check the change, I was just looking at the code
for different purposes.
---
xen/arch/x86/mm/mem_sharing.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 9647e651f9..4f810706a3 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -2013,7 +2013,7 @@ int
mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
if ( !mem_sharing_enabled(d) &&
(rc = mem_sharing_control(d, true, 0)) )
-return rc;
+goto out;
switch ( mso.op )
{
--
2.34.1
Re: [PATCH 3/3] xen/MISRA: Remove nonstandard inline keywords
On 2023-11-22 15:27, Andrew Cooper wrote:
The differences between inline, __inline and __inline__ keywords are a
vestigial remnant of older C standards, and in Xen we use inline almost
exclusively.
Replace __inline and __inline__ with regular inline, and remove their
exceptions from the MISRA configuration.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Roger Pau Monné
CC: Wei Liu
CC: Stefano Stabellini
CC: Roberto Bagnara
CC: Nicola Vetrini
CC: Simone Ballarin
I'm entirely guessing at the Eclair configuration.
---
The configuration changes are ok. One observation below.
.../eclair_analysis/ECLAIR/toolchain.ecl | 6 +++---
docs/misra/C-language-toolchain.rst | 2 +-
xen/arch/x86/include/asm/apic.h | 20 +--
xen/include/acpi/cpufreq/cpufreq.h| 4 ++--
xen/include/xen/bitops.h | 4 ++--
xen/include/xen/compiler.h| 7 +++
6 files changed, 21 insertions(+), 22 deletions(-)
diff --git a/automation/eclair_analysis/ECLAIR/toolchain.ecl
b/automation/eclair_analysis/ECLAIR/toolchain.ecl
index e6cd289b5e92..71a1e2cce029 100644
--- a/automation/eclair_analysis/ECLAIR/toolchain.ecl
+++ b/automation/eclair_analysis/ECLAIR/toolchain.ecl
@@ -15,7 +15,7 @@
_Static_assert: see Section \"2.1 C Language\" of "GCC_MANUAL".
asm, __asm__: see Sections \"6.48 Alternate Keywords\" and \"6.47
How to Use Inline Assembly Language in C Code\" of "GCC_MANUAL".
__volatile__: see Sections \"6.48 Alternate Keywords\" and
\"6.47.2.1 Volatile\" of "GCC_MANUAL".
-__const__, __inline__, __inline: see Section \"6.48 Alternate
Keywords\" of "GCC_MANUAL".
+__const__ : see Section \"6.48 Alternate Keywords\" of
"GCC_MANUAL".
typeof, __typeof__: see Section \"6.7 Referring to a Type with
typeof\" of "GCC_MANUAL".
__alignof__, __alignof: see Sections \"6.48 Alternate Keywords\"
and \"6.44 Determining the Alignment of Functions, Types or Variables\"
of "GCC_MANUAL".
__attribute__: see Section \"6.39 Attribute Syntax\" of
"GCC_MANUAL".
@@ -23,8 +23,8 @@
__builtin_va_arg: non-documented GCC extension.
__builtin_offsetof: see Section \"6.53 Support for offsetof\" of
"GCC_MANUAL".
"
--config=STD.tokenext,behavior+={c99, GCC_ARM64,
"^(_Static_assert|asm|__asm__|__volatile__|__const__|__inline__|typeof|__typeof__|__alignof__|__attribute__|__builtin_types_compatible_p|__builtin_va_arg|__builtin_offsetof)$"}
--config=STD.tokenext,behavior+={c99, GCC_X86_64,
"^(_Static_assert|asm|__asm__|__volatile__|__const__|__inline__|__inline|typeof|__typeof__|__alignof__|__alignof|__attribute__|__builtin_types_compatible_p|__builtin_va_arg|__builtin_offsetof)$"}
+-config=STD.tokenext,behavior+={c99, GCC_ARM64,
"^(_Static_assert|asm|__asm__|__volatile__|__const__|typeof|__typeof__|__alignof__|__attribute__|__builtin_types_compatible_p|__builtin_va_arg|__builtin_offsetof)$"}
+-config=STD.tokenext,behavior+={c99, GCC_X86_64,
"^(_Static_assert|asm|__asm__|__volatile__|__const__|typeof|__typeof__|__alignof__|__alignof|__attribute__|__builtin_types_compatible_p|__builtin_va_arg|__builtin_offsetof)$"}
-doc_end
-doc_begin="Non-documented GCC extension."
diff --git a/docs/misra/C-language-toolchain.rst
b/docs/misra/C-language-toolchain.rst
index 2866cb191b1a..b7c2000992ac 100644
--- a/docs/misra/C-language-toolchain.rst
+++ b/docs/misra/C-language-toolchain.rst
@@ -84,7 +84,7 @@ The table columns are as follows:
see Sections "6.48 Alternate Keywords" and "6.47 How to Use
Inline Assembly Language in C Code" of GCC_MANUAL.
__volatile__:
see Sections "6.48 Alternate Keywords" and "6.47.2.1
Volatile" of GCC_MANUAL.
- __const__, __inline__, __inline:
+ __const__:
see Section "6.48 Alternate Keywords" of GCC_MANUAL.
typeof, __typeof__:
see Section "6.7 Referring to a Type with typeof" of
GCC_MANUAL.
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index 04b8bc18df0e..16d554f2a593 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -20,9 +20,8 @@
#define likely(x) __builtin_expect(!!(x),1)
#define unlikely(x) __builtin_expect(!!(x),0)
-#define inline__inline__
-#define always_inline __inline__ __attribute__ ((__always_inline__))
-#define gnu_inline__inline__ __attribute__ ((__gnu_inline__))
+#define always_inline inline __attribute__((__always_inline__))
+#define gnu_inlineinline __attribute__((__gnu_inline__))
#define noinline __attribute__((__noinline__))
#define noreturn __attribute__((__noreturn__))
@@ -83,7 +82,7 @@
* inline functions not expanded inline get placed in .init.text.
*/
#include
-#define __inline__ __inline__ __init
+#define inline inline __init
The violation of Rule 20.4 (A macro shall not be defined with the same
name as a keyword) is still present due to this macro.
#endif
#define
Re: [PATCH] livepatch: do not use .livepatch.funcs section to store internal state
On Thu, Nov 16, 2023 at 1:54 PM Roger Pau Monné wrote:
>
> On Thu, Nov 16, 2023 at 01:39:27PM +0100, Jan Beulich wrote:
> > On 16.11.2023 12:58, Roger Pau Monne wrote:
> > > --- a/xen/include/public/sysctl.h
> > > +++ b/xen/include/public/sysctl.h
> > > @@ -991,10 +991,7 @@ struct livepatch_func {
> > > uint32_t new_size;
> > > uint32_t old_size;
> > > uint8_t version;/* MUST be LIVEPATCH_PAYLOAD_VERSION. */
> > > -uint8_t opaque[LIVEPATCH_OPAQUE_SIZE];
> > > -uint8_t applied;
> > > -uint8_t patch_offset;
> > > -uint8_t _pad[6];
> > > +uint8_t _pad[39];
> >
> > Should this be LIVEPATCH_OPAQUE_SIZE+8 and ...
>
> Hm, I'm not sure that's any clearer. In fact I think
> LIVEPATCH_OPAQUE_SIZE shouldn't have leaked into sysctl.h in the first
> place.
>
> If we later want to add more fields and bump the version, isn't it
> easier to have the padding size clearly specified as a number?
I prefer using the number as it makes it clear that this padding is not
(anymore) related to the size of the instruction buffer in livepatch_fstate.
Do you think it would be better to call livepatch_fstate.opaque
something like livepatch_fstate.insn_buffer instead (and rename
the constant accordingly) since it is internal to Xen and is not
hiding something from tools building live patches?
(Otherwise this patch looks generally fine to me.)
Ross
Re: [PATCH] x86/mem_sharing: Fix typo in comment
On 22/11/2023 4:26 pm, Frediano Ziglio wrote: > ambigious -> ambiguous > > Signed-off-by: Frediano Ziglio Acked-by: Andrew Cooper
[PATCH] x86/mem_sharing: Fix typo in comment
ambigious -> ambiguous Signed-off-by: Frediano Ziglio --- xen/arch/x86/mm/mem_sharing.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c index 142258f16a..9647e651f9 100644 --- a/xen/arch/x86/mm/mem_sharing.c +++ b/xen/arch/x86/mm/mem_sharing.c @@ -1123,7 +1123,7 @@ err_out: /* * This function is intended to be used for plugging a "hole" in the client's * physmap with a shared memory entry. Unfortunately the definition of a "hole" - * is currently ambigious. There are two cases one can run into a "hole": + * is currently ambiguous. There are two cases one can run into a "hole": * 1) there is no pagetable entry at all * 2) there is a pagetable entry with a type that passes p2m_is_hole * -- 2.34.1
[ovmf test] 183825: all pass - PUSHED
flight 183825 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/183825/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 8736b8fdca85e02933cdb0a13309de14c9799ece baseline version: ovmf 23dbb8a07d108a7b8589e31639b6302b70445b9f Last test of basis 183810 2023-11-21 14:42:50 Z1 days Testing same since 183825 2023-11-22 13:41:06 Z0 days1 attempts People who touched revisions under test: Igor Kulchytskyy Leif Lindholm Liming Gao jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass test-amd64-i386-xl-qemuu-ovmf-amd64 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git 23dbb8a07d..8736b8fdca 8736b8fdca85e02933cdb0a13309de14c9799ece -> xen-tested-master
Re: [PATCH v2 3/5] x86/efi: Simplify efi_arch_handle_cmdline()
On 22/11/2023 9:27 am, Jan Beulich wrote: > On 21.11.2023 21:15, Andrew Cooper wrote: >> -Wwrite-strings is unhappy with assigning "xen" to a mutable pointer, but all >> this work is useless; it's making a memory allocation just to prepend the >> image name which cmdline_cook() intentionally strips back out. >> >> Simply forgo the work and identify EFI_LOADER as one of the loaders which >> doesn't prepend the image name. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper > Reviewed-by: Jan Beulich Thanks. > with one nit: > >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -850,8 +850,11 @@ static const char *__init cmdline_cook(const char *p, >> const char *loader_name) >> while ( *p == ' ' ) >> p++; >> >> -/* GRUB2 and PVH don't not include image name as first item on command >> line. */ >> -if ( xen_guest || loader_is_grub2(loader_name) ) >> +/* >> + * PVH, our EFI loader, and GRUB2 don't not include image name as first > Just "don't", or "do not"? (I realize it was this way before, but perhaps a > good chance to tidy.) Will fix. I completely missed that while rewording it. ~Andrew
Re: [PATCH v2 1/5] x86/setup: Clean up cmdline handling in create_dom0()
On 22/11/2023 9:02 am, Jan Beulich wrote:
> On 21.11.2023 21:15, Andrew Cooper wrote:
>> There's a confusing mix of variables; a static dom0_cmdline[], and a cmdline
>> pointer which points to image->string before being pointed at the static
>> buffer in order to be passed into construct_dom0(). cmdline being a mutable
>> pointer falls over -Wwrite-strings builds.
>>
>> Delete the cmdline pointer, and rename dom0_cmdline[] to cmdline extending it
>> to have full function scope.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper
> Reviewed-by: Jan Beulich
Thanks.
> with two remarks:
>
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -873,6 +873,8 @@ static struct domain *__init create_dom0(const module_t
>> *image,
>> module_t *initrd, const char
>> *kextra,
>> const char *loader)
>> {
>> +static char __initdata cmdline[MAX_GUEST_CMDLINE];
>> +
>> struct xen_domctl_createdomain dom0_cfg = {
> While I think I see why you're adding the blank line there, I'm not overly
> happy about you doing so: Our usual use of blank lines after declarations
> is to separate from statements, not from other (in this case non-static)
> declarations. (And really, just a remark, leaving it to you to keep as is
> or adjust.)
We have plenty of examples of this pattern in the codebase already. Not
quite half of the cases with both static and local variable
declarations, but not far off either.
>From a clarity point of view it is helpful to separate the stack locals
from the globals-with-local-scope.
>
>> @@ -913,33 +914,30 @@ static struct domain *__init create_dom0(const
>> module_t *image,
>> panic("Error creating d%uv0\n", domid);
>>
>> /* Grab the DOM0 command line. */
>> -cmdline = image->string ? __va(image->string) : NULL;
>> -if ( cmdline || kextra )
>> +if ( image->string || kextra )
>> {
>> -static char __initdata dom0_cmdline[MAX_GUEST_CMDLINE];
>> -
>> -cmdline = cmdline_cook(cmdline, loader);
>> -safe_strcpy(dom0_cmdline, cmdline);
>> +if ( image->string )
>> +safe_strcpy(cmdline, cmdline_cook(__va(image->string), loader));
>>
>> if ( kextra )
>> /* kextra always includes exactly one leading space. */
>> -safe_strcat(dom0_cmdline, kextra);
>> +safe_strcat(cmdline, kextra);
>>
>> /* Append any extra parameters. */
>> -if ( skip_ioapic_setup && !strstr(dom0_cmdline, "noapic") )
>> -safe_strcat(dom0_cmdline, " noapic");
>> +if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
>> +safe_strcat(cmdline, " noapic");
>> +
>> if ( (strlen(acpi_param) == 0) && acpi_disabled )
>> {
>> printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
>> safe_strcpy(acpi_param, "off");
>> }
>> -if ( (strlen(acpi_param) != 0) && !strstr(dom0_cmdline, "acpi=") )
>> +
>> +if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
> As you're touching this anyway, how about replacing the strlen() by just
> *acpi_param? We don't really care exactly how long the string is. (As an
> aside, strstr() uses like the one here are of course also pretty fragile.
> But of course that's nothing to care about in this change.)
There's the same pattern just above it, not touched, and this patch is
already getting complicated.
I think there's other cleanup to do here, so I'll defer it to later.
~Andrew
[PATCH v3] xen/x86: On x2APIC mode, derive LDR from APIC ID
Both Intel and AMD manuals agree that on x2APIC mode, the APIC LDR and ID
registers are derivable from each other through a fixed formula.
Xen uses that formula, but applies it to vCPU IDs (which are sequential)
rather than x2APIC IDs (which are not, at the moment). As I understand it,
this is an attempt to tightly pack vCPUs into clusters so each cluster has
16 vCPUs rather than 8, but this is problematic for OSs that might read the
x2APIC ID and internally derive LDR (or the other way around)
This patch fixes the implementation so we follow the rules in the x2APIC
spec(s) and covers migrations from broken hypervisors, so LDRs are
preserved even for hotppluggable CPUs and across APIC resets.
While touching that area, I removed the existing printk statement in
vlapic_load_fixup() (as the checks it performed didn't make sense in x2APIC
mode and wouldn't affect the outcome) and put another printk as an else
branch so we get warnings trying to load nonsensical LDR values we don't
know about.
Fixes: f9e0cccf7b35 ("x86/HVM: fix ID handling of x2APIC emulation")
Signed-off-by: Alejandro Vallejo
---
v3:
* Removed underscores from (x2)APIC_ID in commit message
* Added commit msg explaining the removal of the vlapic_load_fixup() printk
* Restored lowercase to hex "F"s
* Refactored a bit vlapic_load_fixup() so it has a trailing printk in
case of spotting a nonsensical LDR it doesn't understand.
* Moved field in domain.h with the other booleans
* Trimmed down field name in domain.h
* Trimmed down field comment in domain.h
If the field name in domain.h still seems too long I'm happy for it to be
trimmed further down, but I do want to preserve the "_bug_" part of it.
---
xen/arch/x86/hvm/vlapic.c | 62 +--
xen/arch/x86/include/asm/hvm/domain.h | 3 ++
2 files changed, 43 insertions(+), 22 deletions(-)
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 5cb87f8649..cd929c3716 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1061,13 +1061,23 @@ static const struct hvm_mmio_ops vlapic_mmio_ops = {
.write = vlapic_mmio_write,
};
+static uint32_t x2apic_ldr_from_id(uint32_t id)
+{
+return ((id & ~0xf) << 12) | (1 << (id & 0xf));
+}
+
static void set_x2apic_id(struct vlapic *vlapic)
{
-u32 id = vlapic_vcpu(vlapic)->vcpu_id;
-u32 ldr = ((id & ~0xf) << 12) | (1 << (id & 0xf));
+uint32_t vcpu_id = vlapic_vcpu(vlapic)->vcpu_id;
+uint32_t apic_id = vcpu_id * 2;
+uint32_t apic_ldr = x2apic_ldr_from_id(apic_id);
+
+/* This is a migration bug workaround. See wall of text in
lapic_load_fixup() */
+if ( vlapic_domain(vlapic)->arch.hvm.x2apic_ldr_bug_with_vcpu_id )
+apic_ldr = x2apic_ldr_from_id(vcpu_id);
-vlapic_set_reg(vlapic, APIC_ID, id * 2);
-vlapic_set_reg(vlapic, APIC_LDR, ldr);
+vlapic_set_reg(vlapic, APIC_ID, apic_id);
+vlapic_set_reg(vlapic, APIC_LDR, apic_ldr);
}
int guest_wrmsr_apic_base(struct vcpu *v, uint64_t val)
@@ -1498,27 +1508,35 @@ static int cf_check lapic_save_regs(struct vcpu *v,
hvm_domain_context_t *h)
*/
static void lapic_load_fixup(struct vlapic *vlapic)
{
-uint32_t id = vlapic->loaded.id;
+/* Skip fixups on xAPIC mode, or if the x2APIC LDR is already correct */
+if ( !vlapic_x2apic_mode(vlapic) ||
+ (vlapic->loaded.ldr == x2apic_ldr_from_id(vlapic->loaded.id)) )
+return;
-if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 )
-{
+if ( vlapic->loaded.ldr == 1 )
+ /*
+* Xen <= 4.4 had a bug by which all the APICs configured in x2APIC
+* mode got LDR = 1. We can't leave it as-is because it assigned the
+* same LDR to every CPU. We'll fix fix the bug now and assign an
+* LDR value consistent with the APIC ID.
+*/
+set_x2apic_id(vlapic);
+else if ( vlapic->loaded.ldr ==
+ x2apic_ldr_from_id(vlapic_vcpu(vlapic)->vcpu_id) )
/*
- * This is optional: ID != 0 contradicts LDR == 1. It's being added
- * to aid in eventual debugging of issues arising from the fixup done
- * here, but can be dropped as soon as it is found to conflict with
- * other (future) changes.
+ * This is a migration from a broken Xen between 4.4 and 4.18 and
+ * we must _PRESERVE_ LDRs so new vCPUs use consistent derivations.
+ * This is so existing running guests that may have already read
+ * the LDR at the source host aren't surprised when IPIs stop
+ * working as they did at the other end. To address this, we set
+ * this domain boolean so future CPU hotplugs derive an LDR
+ * consistent with the older Xen's broken idea of consistency.
*/
-if ( GET_xAPIC_ID(id) != vlapic_vcpu(vlapic)->vcpu_id * 2 ||
- id != SET_xAPIC_ID(GET_xAPIC_ID(id)) )
-printk(XENLOG_G_WARNING "%pv: bogus APIC ID %#x loaded\n",
Re: [PATCH 3/3] xen/MISRA: Remove nonstandard inline keywords
On 22/11/2023 2:27 pm, Andrew Cooper wrote: > The differences between inline, __inline and __inline__ keywords are a > vestigial remnant of older C standards, and in Xen we use inline almost > exclusively. > > Replace __inline and __inline__ with regular inline, and remove their > exceptions from the MISRA configuration. > > Signed-off-by: Andrew Cooper > --- > CC: Jan Beulich > CC: Roger Pau Monné > CC: Wei Liu > CC: Stefano Stabellini > CC: Roberto Bagnara > CC: Nicola Vetrini > CC: Simone Ballarin > > I'm entirely guessing at the Eclair configuration. https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/5596360097 is Eclair running on this change, and it came back green But I'll have to defer to bugsend to judge whether it was correctly configured. ~Andrew
Re: [PATCH v2 1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it
On 21/11/2023 22:10, Volodymyr Babchuk wrote: From: David Woodhouse This allows a XenDevice implementation to know whether it was created by QEMU, or merely discovered in XenStore after the toolstack created it. This will allow us to create frontend/backend nodes only when we should, rather than unconditionally attempting to overwrite them from a driver domain which doesn't have privileges to do so. As an added benefit, it also means we no longer have to call the xen_backend_set_device() function from the device models immediately after calling qdev_realize_and_unref(). Even though we could make the argument that it's safe to do so, and the pointer to the unreffed device *will* actually still be valid, it still made my skin itch to look at it. Signed-off-by: David Woodhouse --- hw/block/xen-block.c | 3 +-- hw/char/xen_console.c| 2 +- hw/net/xen_nic.c | 2 +- hw/xen/xen-bus.c | 4 include/hw/xen/xen-backend.h | 2 -- include/hw/xen/xen-bus.h | 2 ++ 6 files changed, 9 insertions(+), 6 deletions(-) Reviewed-by: Paul Durrant
Re: [PATCH v2 4/5] arm/efi: Simplify efi_arch_handle_cmdline()
> On 21 Nov 2023, at 20:41, Andrew Cooper wrote: > > On 21/11/2023 8:33 pm, Luca Fancellu wrote: >> + CC henry >> >>> On 21 Nov 2023, at 20:15, Andrew Cooper wrote: >>> >>> -Wwrite-strings is unhappy with assigning "xen" to a mutable pointer, but >>> this >>> logic looks incorrect. It was inherited from the x86 side, where the logic >>> was redundant and has now been removed. >>> >>> In the ARM case it inserts the image name into "xen,xen-bootargs" and there >>> is >>> no logic at all to strip this before parsing it as the command line. >>> >>> The absence of any logic to strip an image name suggests that it shouldn't >>> exist there, or having a Xen image named e.g. "hmp-unsafe" in the filesystem >>> is going to lead to some unexpected behaviour on boot. >>> >>> No functional change. >>> >>> Signed-off-by: Andrew Cooper >>> --- >>> CC: Jan Beulich >>> CC: Roger Pau Monné >>> CC: Wei Liu >>> CC: Stefano Stabellini >>> CC: Julien Grall >>> CC: Volodymyr Babchuk >>> CC: Bertrand Marquis >>> CC: Roberto Bagnara >>> CC: Nicola Vetrini >>> >>> v2: >>> * New. >>> >>> I'm afraid that all of this reasoning is based on reading the source code. >>> I >>> don't have any way to try this out in a real ARM UEFI environment. >> I will test this one tomorrow on an arm board > I confirm that booting though UEFI on an arm board works Reviewed-by: Luca Fancellu Tested-by: Luca Fancellu
xen | Successful pipeline for staging | c22fe721
Pipeline #1081610073 has passed! Project: xen ( https://gitlab.com/xen-project/xen ) Branch: staging ( https://gitlab.com/xen-project/xen/-/commits/staging ) Commit: c22fe721 ( https://gitlab.com/xen-project/xen/-/commit/c22fe7213c9b1f99cbc64c33e391afa223f9cd08 ) Commit Message: automation: switch to multi-platform images whe... Commit Author: Roger Pau Monne Committed by: Andrew Cooper ( https://gitlab.com/andyhhp ) Pipeline #1081610073 ( https://gitlab.com/xen-project/xen/-/pipelines/1081610073 ) triggered by Ganis ( https://gitlab.com/ganis ) successfully completed 129 jobs in 3 stages. -- You're receiving this email because of your account on gitlab.com.
Re: [PATCH v2] xen/x86: On x2APIC mode, derive LDR from APIC_ID
On Wed, Nov 22, 2023 at 02:40:02PM +0100, Roger Pau Monné wrote:
> On Tue, Nov 21, 2023 at 04:26:04PM +, Alejandro Vallejo wrote:
> > Both Intel and AMD manuals agree that on x2APIC mode, the APIC LDR and ID
> > registers are derivable from each other through a fixed formula.
> >
> > Xen uses that formula, but applies it to vCPU IDs (which are sequential)
> > rather than x2APIC_IDs (which are not, at the moment). As I understand it,
> > this is an attempt to tightly pack vCPUs into clusters so each cluster has
> > 16 vCPUs rather than 8, but this is problematic for OSs that might read the
> > x2APIC_ID and internally derive LDR (or the other way around)
> >
> > This patch fixes the implementation so we follow the rules in the x2APIC
> > spec(s).
> >
> > The patch also covers migrations from broken hypervisors, so LDRs are
> > preserved even for hotppluggable CPUs and across APIC resets.
> >
> > Fixes: f9e0cccf7b35 ("x86/HVM: fix ID handling of x2APIC emulation")
> > Signed-off-by: Alejandro Vallejo
>
> LGTM, just a couple of style comments.
>
> > ---
> > I tested this by creating 3 checkpoints.
> > 1. One with pre-4.4 broken state (every LDR=1, by hacking save_regs)
> > 2. One with 4.4 onwards broken state (LDRs packed in their clusters)
> > 3. One with correct LDR values
> >
> > (1) and (3) restores to the same thing. Consistent APIC_ID+LDR
> > (2) restores to what it previously had and hotplugs follow the same logic
> > ---
> > xen/arch/x86/hvm/vlapic.c | 81 +++
> > xen/arch/x86/include/asm/hvm/domain.h | 13 +
> > 2 files changed, 72 insertions(+), 22 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> > index a8e87c4446..7f169f1e5f 100644
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -1061,13 +1061,23 @@ static const struct hvm_mmio_ops vlapic_mmio_ops = {
> > .write = vlapic_mmio_write,
> > };
> >
> > +static uint32_t x2apic_ldr_from_id(uint32_t id)
> > +{
> > +return ((id & ~0xF) << 12) | (1 << (id & 0xF));
>
> Seeing other usages in vlapic.c I think the preference is to use lower
> case for hex numbers.
I thought it was covered by a MISRA rule, but it seems to apply only to the
literal suffixes. Fair enough, I'll revert to lowercase.
>
> > +}
> > +
> > static void set_x2apic_id(struct vlapic *vlapic)
> > {
> > -u32 id = vlapic_vcpu(vlapic)->vcpu_id;
> > -u32 ldr = ((id & ~0xf) << 12) | (1 << (id & 0xf));
> > +uint32_t vcpu_id = vlapic_vcpu(vlapic)->vcpu_id;
> > +uint32_t apic_id = vcpu_id * 2;
> > +uint32_t apic_ldr = x2apic_ldr_from_id(apic_id);
> >
> > -vlapic_set_reg(vlapic, APIC_ID, id * 2);
> > -vlapic_set_reg(vlapic, APIC_LDR, ldr);
> > +/* This is a migration bug workaround. See wall of text in
> > lapic_load_fixup() */
> > +if ( vlapic_domain(vlapic)->arch.hvm.has_inconsistent_x2apic_ldr_bug )
> > +apic_ldr = x2apic_ldr_from_id(vcpu_id);
> > +
> > +vlapic_set_reg(vlapic, APIC_ID, apic_id);
> > +vlapic_set_reg(vlapic, APIC_LDR, apic_ldr);
> > }
> >
> > int guest_wrmsr_apic_base(struct vcpu *v, uint64_t val)
> > @@ -1495,30 +1505,57 @@ static int cf_check lapic_save_regs(struct vcpu *v,
> > hvm_domain_context_t *h)
> > /*
> > * Following lapic_load_hidden()/lapic_load_regs() we may need to
> > * correct ID and LDR when they come from an old, broken hypervisor.
> > + *
> > + * Xen <= 4.4 had a bug by which all the APICs configured in x2APIC mode
> > + * got LDR = 1. This was fixed back then, but another bug was introduced
> > + * causing APIC ID and LDR to break the consistency they are meant to have
> > + * according to the specs (LDR was derived from vCPU ID, rather than APIC
> > + * ID)
> > + *
> > + * Long story short, we can detect both cases here. For the LDR=1 case we
> > + * want to fix it up on migrate, as IPIs just don't work on non-physical
> > + * mode otherwise. For the other case we actually want to preserve previous
> > + * behaviour so that existing running instances that may have already read
> > + * the LDR at the source host aren't surprised when IPIs stop working as
> > + * they did at the other end.
> > + *
> > + * Note that "x2apic_id == 0" has always been correct and can't be used to
> > + * discriminate these cases.
> > + *
>
> I think it's best if this big comment was split between the relevant
> parts of the if below used to detect the broken states, as that makes
> the comments more in-place with the code.
Ack
>
> > + * Yuck!
> > */
> > static void lapic_load_fixup(struct vlapic *vlapic)
> > {
> > -uint32_t id = vlapic->loaded.id;
> > +/*
> > + * This LDR would be present in broken versions of Xen 4.4 through
> > 4.18.
> > + * It's correct for the cpu with x2apic_id=0 (vcpu0), but invalid for
> > + * any other.
> > + */
> > +uint32_t bad_ldr = x2apic_ldr_from_id(vlapic_vcpu(vlapic)->vcpu_id);
> >
> > -if (
[PATCH] xen/arm: gicv3: clean up GICD_CTRL write
GICD_CTL_ENABLE is a GICv2 bit. Remove it. The definitions of GICD_CTL_ENABLE and GICD_CTLR_ENABLE_G1 are identical, so the value written is unchanged. Signed-off-by: Stewart Hildebrand --- xen/arch/arm/gic-v3.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 172ff8c005ff..9b35a8c8a735 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -633,8 +633,8 @@ static void __init gicv3_dist_init(void) gicv3_dist_wait_for_rwp(); /* Turn on the distributor */ -writel_relaxed(GICD_CTL_ENABLE | GICD_CTLR_ARE_NS | -GICD_CTLR_ENABLE_G1A | GICD_CTLR_ENABLE_G1, GICD + GICD_CTLR); +writel_relaxed(GICD_CTLR_ARE_NS | GICD_CTLR_ENABLE_G1A | + GICD_CTLR_ENABLE_G1, GICD + GICD_CTLR); /* Route all global IRQs to this CPU */ affinity = gicv3_mpidr_to_affinity(smp_processor_id()); base-commit: c22fe7213c9b1f99cbc64c33e391afa223f9cd08 -- 2.43.0
[libvirt test] 183819: tolerable all pass - PUSHED
flight 183819 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/183819/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 183808 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 183808 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 183808 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt 16 saverestore-support-checkfail never pass test-amd64-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-qcow2 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass version targeted for testing: libvirt 7f31ee5cf5b137275697bd8ca17c7ae591261b87 baseline version: libvirt b31380c7583d144380e3a3b7affccfefccff41f9 Last test of basis 183808 2023-11-21 04:18:56 Z1 days Testing same since 183819 2023-11-22 04:20:25 Z0 days1 attempts People who touched revisions under test: Daniel P. Berrangé Ján Tomko Michal Privoznik jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-arm64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-arm64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass test-amd64-amd64-libvirt-xsm pass test-arm64-arm64-libvirt-xsm pass test-amd64-i386-libvirt-xsm pass test-amd64-amd64-libvirt pass test-arm64-arm64-libvirt pass test-armhf-armhf-libvirt pass test-amd64-i386-libvirt pass test-amd64-amd64-libvirt-pairpass test-amd64-i386-libvirt-pair pass test-arm64-arm64-libvirt-qcow2 pass test-armhf-armhf-libvirt-qcow2 pass test-arm64-arm64-libvirt-raw pass test-armhf-armhf-libvirt-raw pass test-amd64-i386-libvirt-raw pass test-amd64-amd64-libvirt-vhd pass 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
[PATCH 2/3] x86/apic: Drop the APIC_MSR_BASE constant
Use MSR_X2APIC_FIRST from msr-index.h instead. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Roger Pau Monné CC: Wei Liu --- xen/arch/x86/include/asm/apic.h| 4 ++-- xen/arch/x86/include/asm/apicdef.h | 3 --- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/xen/arch/x86/include/asm/apic.h b/xen/arch/x86/include/asm/apic.h index 288b4933eb38..486d689478b2 100644 --- a/xen/arch/x86/include/asm/apic.h +++ b/xen/arch/x86/include/asm/apic.h @@ -69,7 +69,7 @@ static __inline void apic_wrmsr(unsigned long reg, uint64_t msr_content) reg == APIC_LVR) return; -wrmsrl(APIC_MSR_BASE + (reg >> 4), msr_content); +wrmsrl(MSR_X2APIC_FIRST + (reg >> 4), msr_content); } static __inline uint64_t apic_rdmsr(unsigned long reg) @@ -79,7 +79,7 @@ static __inline uint64_t apic_rdmsr(unsigned long reg) if (reg == APIC_DFR) return -1u; -rdmsrl(APIC_MSR_BASE + (reg >> 4), msr_content); +rdmsrl(MSR_X2APIC_FIRST + (reg >> 4), msr_content); return msr_content; } diff --git a/xen/arch/x86/include/asm/apicdef.h b/xen/arch/x86/include/asm/apicdef.h index 8d1b0087d4ef..c4068ccc10f4 100644 --- a/xen/arch/x86/include/asm/apicdef.h +++ b/xen/arch/x86/include/asm/apicdef.h @@ -124,9 +124,6 @@ #define APIC_BASE __fix_to_virt(FIX_APIC_BASE) -/* It's only used in x2APIC mode of an x2APIC unit. */ -#define APIC_MSR_BASE 0x800 - #define MAX_IO_APICS 128 extern bool x2apic_enabled; -- 2.30.2
[PATCH 0/3] xen/MISRA: Remove nonstandard inline keywords
Along with two minor pieces of cleanup in x86/apic found while doing this work. Gitlab CI: https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1081682400 Andrew Cooper (3): x86/apic: Drop atomic accessors x86/apic: Drop the APIC_MSR_BASE constant xen/MISRA: Remove nonstandard inline keywords .../eclair_analysis/ECLAIR/toolchain.ecl | 6 +-- docs/misra/C-language-toolchain.rst | 2 +- xen/arch/x86/include/asm/apic.h | 37 ++- xen/arch/x86/include/asm/apicdef.h| 3 -- xen/include/acpi/cpufreq/cpufreq.h| 4 +- xen/include/xen/bitops.h | 4 +- xen/include/xen/compiler.h| 7 ++-- 7 files changed, 23 insertions(+), 40 deletions(-) base-commit: c22fe7213c9b1f99cbc64c33e391afa223f9cd08 -- 2.30.2
[PATCH 3/3] xen/MISRA: Remove nonstandard inline keywords
The differences between inline, __inline and __inline__ keywords are a
vestigial remnant of older C standards, and in Xen we use inline almost
exclusively.
Replace __inline and __inline__ with regular inline, and remove their
exceptions from the MISRA configuration.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Roger Pau Monné
CC: Wei Liu
CC: Stefano Stabellini
CC: Roberto Bagnara
CC: Nicola Vetrini
CC: Simone Ballarin
I'm entirely guessing at the Eclair configuration.
---
.../eclair_analysis/ECLAIR/toolchain.ecl | 6 +++---
docs/misra/C-language-toolchain.rst | 2 +-
xen/arch/x86/include/asm/apic.h | 20 +--
xen/include/acpi/cpufreq/cpufreq.h| 4 ++--
xen/include/xen/bitops.h | 4 ++--
xen/include/xen/compiler.h| 7 +++
6 files changed, 21 insertions(+), 22 deletions(-)
diff --git a/automation/eclair_analysis/ECLAIR/toolchain.ecl
b/automation/eclair_analysis/ECLAIR/toolchain.ecl
index e6cd289b5e92..71a1e2cce029 100644
--- a/automation/eclair_analysis/ECLAIR/toolchain.ecl
+++ b/automation/eclair_analysis/ECLAIR/toolchain.ecl
@@ -15,7 +15,7 @@
_Static_assert: see Section \"2.1 C Language\" of "GCC_MANUAL".
asm, __asm__: see Sections \"6.48 Alternate Keywords\" and \"6.47 How to
Use Inline Assembly Language in C Code\" of "GCC_MANUAL".
__volatile__: see Sections \"6.48 Alternate Keywords\" and \"6.47.2.1
Volatile\" of "GCC_MANUAL".
-__const__, __inline__, __inline: see Section \"6.48 Alternate Keywords\"
of "GCC_MANUAL".
+__const__ : see Section \"6.48 Alternate Keywords\" of "GCC_MANUAL".
typeof, __typeof__: see Section \"6.7 Referring to a Type with typeof\" of
"GCC_MANUAL".
__alignof__, __alignof: see Sections \"6.48 Alternate Keywords\" and
\"6.44 Determining the Alignment of Functions, Types or Variables\" of
"GCC_MANUAL".
__attribute__: see Section \"6.39 Attribute Syntax\" of "GCC_MANUAL".
@@ -23,8 +23,8 @@
__builtin_va_arg: non-documented GCC extension.
__builtin_offsetof: see Section \"6.53 Support for offsetof\" of
"GCC_MANUAL".
"
--config=STD.tokenext,behavior+={c99, GCC_ARM64,
"^(_Static_assert|asm|__asm__|__volatile__|__const__|__inline__|typeof|__typeof__|__alignof__|__attribute__|__builtin_types_compatible_p|__builtin_va_arg|__builtin_offsetof)$"}
--config=STD.tokenext,behavior+={c99, GCC_X86_64,
"^(_Static_assert|asm|__asm__|__volatile__|__const__|__inline__|__inline|typeof|__typeof__|__alignof__|__alignof|__attribute__|__builtin_types_compatible_p|__builtin_va_arg|__builtin_offsetof)$"}
+-config=STD.tokenext,behavior+={c99, GCC_ARM64,
"^(_Static_assert|asm|__asm__|__volatile__|__const__|typeof|__typeof__|__alignof__|__attribute__|__builtin_types_compatible_p|__builtin_va_arg|__builtin_offsetof)$"}
+-config=STD.tokenext,behavior+={c99, GCC_X86_64,
"^(_Static_assert|asm|__asm__|__volatile__|__const__|typeof|__typeof__|__alignof__|__alignof|__attribute__|__builtin_types_compatible_p|__builtin_va_arg|__builtin_offsetof)$"}
-doc_end
-doc_begin="Non-documented GCC extension."
diff --git a/docs/misra/C-language-toolchain.rst
b/docs/misra/C-language-toolchain.rst
index 2866cb191b1a..b7c2000992ac 100644
--- a/docs/misra/C-language-toolchain.rst
+++ b/docs/misra/C-language-toolchain.rst
@@ -84,7 +84,7 @@ The table columns are as follows:
see Sections "6.48 Alternate Keywords" and "6.47 How to Use Inline
Assembly Language in C Code" of GCC_MANUAL.
__volatile__:
see Sections "6.48 Alternate Keywords" and "6.47.2.1 Volatile" of
GCC_MANUAL.
- __const__, __inline__, __inline:
+ __const__:
see Section "6.48 Alternate Keywords" of GCC_MANUAL.
typeof, __typeof__:
see Section "6.7 Referring to a Type with typeof" of GCC_MANUAL.
diff --git a/xen/arch/x86/include/asm/apic.h b/xen/arch/x86/include/asm/apic.h
index 486d689478b2..b20fae7ebc6a 100644
--- a/xen/arch/x86/include/asm/apic.h
+++ b/xen/arch/x86/include/asm/apic.h
@@ -49,12 +49,12 @@ const struct genapic *apic_x2apic_probe(void);
* Basic functions accessing APICs.
*/
-static __inline void apic_mem_write(unsigned long reg, u32 v)
+static inline void apic_mem_write(unsigned long reg, u32 v)
{
*((volatile u32 *)(APIC_BASE+reg)) = v;
}
-static __inline u32 apic_mem_read(unsigned long reg)
+static inline u32 apic_mem_read(unsigned long reg)
{
return *((volatile u32 *)(APIC_BASE+reg));
}
@@ -63,7 +63,7 @@ static __inline u32 apic_mem_read(unsigned long reg)
* access the 64-bit ICR register.
*/
-static __inline void apic_wrmsr(unsigned long reg, uint64_t msr_content)
+static inline void apic_wrmsr(unsigned long reg, uint64_t msr_content)
{
if (reg == APIC_DFR || reg == APIC_ID || reg == APIC_LDR ||
reg == APIC_LVR)
@@ -72,7 +72,7 @@ static __inline void apic_wrmsr(unsigned long reg, uint64_t
msr_content)
wrmsrl(MSR_X2APIC_FIRST + (reg >> 4),
