Re: [PATCH 1/2] docs/about: Deprecate 32-bit x86 hosts and qemu-system-i386

[Thomas Huth]

On 27/02/2023 21.25, Richard Henderson wrote:

On 2/27/23 01:50, Daniel P. Berrangé wrote:

On Mon, Feb 27, 2023 at 12:10:49PM +0100, Thomas Huth wrote:

Hardly anybody still uses 32-bit x86 hosts today, so we should
start deprecating them to finally have less test efforts.
With regards to 32-bit KVM support in the x86 Linux kernel,
the developers confirmed that they do not need a recent
qemu-system-i386 binary here:

  https://lore.kernel.org/kvm/y%2ffkts5ajfy0h...@google.com/

Signed-off-by: Thomas Huth 
---
  docs/about/deprecated.rst | 13 +
  1 file changed, 13 insertions(+)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 15084f7bea..98517f5187 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -196,6 +196,19 @@ CI coverage support may bitrot away before the 
deprecation process

  completes. The little endian variants of MIPS (both 32 and 64 bit) are
  still a supported host architecture.
+32-bit x86 hosts and ``qemu-system-i386`` (since 8.0)
+'
+
+Testing 32-bit x86 host OS support takes a lot of precious time during the
+QEMU contiguous integration tests, and considering that most OS vendors
+stopped shipping 32-bit variants of their x86 OS distributions and most
+x86 hardware from the past >10 years is capable of 64-bit, keeping the
+32-bit support alive is an inadequate burden for the QEMU project. Thus
+QEMU will soon drop the support for 32-bit x86 host systems and the
+``qemu-system-i386`` binary. Use ``qemu-system-x86_64`` (which is a proper
+superset of ``qemu-system-i386``) on a 64-bit host machine instead.


I feel like we should have separate deprecation entries for the
i686 host support, and for qemu-system-i386 emulator binary, as
although they're related they are independant features with
differing impact.


Agreed.


OK, fair, I'll rework my patch according to your suggestion, Daniel.


32-bit x86 hosts


Support for 32-bit x86 host deployments is increasingly uncommon in
mainstream Linux distributions given the widespread availability of
64-bit x86 hardware. The QEMU project no longer considers 32-bit
x86 support to be an effective use of its limited resources, and
thus intends to discontinue it.

Current users of QEMU on 32-bit x86 hosts should either continue
using existing releases of QEMU, with the caveat that they will
no longer get security fixes, or migrate to a 64-bit platform
which remains capable of running 32-bit guests if needed.

Ack.



``qemu-system-i386`` binary removal
'''

The ``qemu-system-x86_64`` binary can be used to run 32-bit guests
by selecting a 32-bit CPU model, including KVM support on x86_64
hosts. Once support for the 32-bit x86 host platform is discontinued,
the ``qemu-system-i386`` binary will be redundant.


Missing "kvm" in this last sentence?  It is otherwise untrue for tcg.


I assume that Daniel only thought of 32-bit x86 hosts here, but indeed, it's 
untrue for non-x86 32-bit hosts. So this really should refer to KVM on 
32-bit x86 hosts instead. I'll rephrase it in v2.


 Thomas

Re: [PATCH 1/2] docs/about: Deprecate 32-bit x86 hosts and qemu-system-i386

[Thomas Huth]

On 27/02/2023 21.12, Michael S. Tsirkin wrote:

On Mon, Feb 27, 2023 at 11:50:07AM +, Daniel P. Berrangé wrote:

I feel like we should have separate deprecation entries for the
i686 host support, and for qemu-system-i386 emulator binary, as
although they're related they are independant features with
differing impact. eg removing qemu-system-i386 affects all
host architectures, not merely 32-bit x86 host, so I think we
can explain the impact more clearly if we separate them.


Removing qemu-system-i386 seems ok to me - I think qemu-system-x86_64 is
a superset.

Removing support for building on 32 bit systems seems like a pity - it's
one of a small number of ways to run 64 bit binaries on 32 bit systems,
and the maintainance overhead is quite small.


Note: We're talking about 32-bit *x86* hosts here. Do you really think that 
someone is still using QEMU usermode emulation

to run 64-bit binaries on a 32-bit x86 host?? ... If so, I'd be very surprised!


In fact, keeping this support around forces correct use of
posix APIs such as e.g. PRIx64 which makes the code base
more future-proof.


If you're concerned about PRIx64 and friends: We still continue to do 
compile testing with 32-bit MIPS cross-compilers and Windows 32-bit 
cross-compilers for now. The only thing we'd lose is the 32-bit "make check" 
run in the CI.


 Thomas

Re: [PATCH v3 10/14] x86/vmx: move declarations used only by vmx code from vmx.h to private headers

[Jan Beulich]

On 27.02.2023 17:26, Andrew Cooper wrote:
> On 24/02/2023 6:50 pm, Xenia Ragiadakou wrote:
>> Create two new private headers in arch/x86/hvm/vmx called vmx.h and pi.h.
>> Move all the definitions and declarations that are used solely by vmx code
>> into the private vmx.h, apart from the ones related to posted interrupts that
>> are moved into pi.h.
>>
>> EPT related declarations and definitions stay in asm/hvm/vmx/vmx.h because
>> they are used in arch/x86/mm and drivers/passthrough/vtd.
>>
>> Also, __vmread(), used in arch/x86/cpu, and consequently the opcodes stay in
>> asm/hvm/vmx/vmx.h.
> 
> Every time I read the vpmu code, I get increasingly sad.
> 
> That is dangerously unsafe, and comes with a chance of exploding completely.
> 
> That __vmread() is in NMI context, which means `current` isn't safe to
> deference (we might hit in the middle of a context switch), and more
> generally there's no guarantee that the loaded VMCS is the one
> associated with `current` (we might hit in the middle of a remote VMCS
> access).

Are you mixing up oprofile (using NMI) and vPMU (using an ordinary vectored
interrupt)? Or am I overlooking a vPMU mode of operation where NMI could be
used (i.e. other than apic_intr_init()'s calling of set_direct_apic_vector()
and other than pmu_interrupt() invoking vpmu_do_interrupt() /after/ acking
the IRQ at the LAPIC)?

Jan

> vpmu is generally not supported, and BTS needs further custom enablement
> because it is only useable with a custom bus analyser.
> 
> 
> The __vmread() needs deleting - its absolutely not safe to say.
> 
> I'm tempted to hardwire the return 0, and punt the problem to whomever
> next uses BTS.
> 
> Alternatively, MSR_DBGCTL needs wiring into the hvm_get_reg()
> infrastructure, but I'm not convinced this will actually work in either
> of the two problem cases above, hence preferring the previous option.
> 
> Thoughts?
> 
> ~Andrew

Re: [PATCH 1/2] docs/about: Deprecate 32-bit x86 hosts and qemu-system-i386

[Thomas Huth]

On 27/02/2023 23.32, Philippe Mathieu-Daudé wrote:

On 27/2/23 21:12, Michael S. Tsirkin wrote:

On Mon, Feb 27, 2023 at 11:50:07AM +, Daniel P. Berrangé wrote:

I feel like we should have separate deprecation entries for the
i686 host support, and for qemu-system-i386 emulator binary, as
although they're related they are independant features with
differing impact. eg removing qemu-system-i386 affects all
host architectures, not merely 32-bit x86 host, so I think we
can explain the impact more clearly if we separate them.


Removing qemu-system-i386 seems ok to me - I think qemu-system-x86_64 is
a superset.


Doesn't qemu-system-i386 start the CPU in a different mode that
qemu-system-x86_64? Last time we discussed it, we mention adding
-32 and -64 CLI flags to maintain compat, and IIRC this flag would
add boot code to switch the CPU in 32-b. But then maybe I misunderstood.
Thomas said, "CPUs must start in the same mode they start in HW".


No, I think you misunderstood something here. x86 CPUs always start in 
16-bit mode, as far as I know, and the firmware / OS then has to switch to 
32-bit or 64-bit mode as desired.


 Thomas

Re: [PATCH 0/2] Deprecate support for 32-bit x86 and arm hosts

[Thomas Huth]

On 27/02/2023 19.38, Daniel P. Berrangé wrote:

On Mon, Feb 27, 2023 at 12:10:48PM +0100, Thomas Huth wrote:

We're struggling quite badly with our CI minutes on the shared
gitlab runners, so we urgently need to think of ways to cut down
our supported build and target environments. qemu-system-i386 and
qemu-system-arm are not really required anymore, since nobody uses
KVM on the corresponding systems for production anymore, and the
-x86_64 and -arch64 variants are a proper superset of those binaries.
So it's time to deprecate them and the corresponding 32-bit host
environments now.

This is a follow-up patch series from the previous discussion here:

  https://lore.kernel.org/qemu-devel/20230130114428.1297295-1-th...@redhat.com/

where people still mentioned that there is still interest in certain
support for 32-bit host hardware. But as far as I could see, there is
no real need for 32-bit host support for system emulation on x86 and
arm anymore, so it should be fine if we drop these host environments
now (these are also the two architectures that contribute the most to
the long test times in our CI, so we would benefit a lot by dropping
those).


Your description here is a little ambiguous about what's being
proposed. When you say dropping 32-bit host support do you mean
just for the system emulator binaries, or for QEMU entirely ?


Just for system emulation. Some people said that user emulation still might 
be useful for some 32-bit environments.



And when the deprecation period is passed, are you proposing
to actively prevent 32-bit builds, or merely stopping CI testing
and leave 32-bit builds still working if people want them ?


CI is the main pain point, so that's the most important thing. So whether we 
throw a warning or a hard error while configuring the build, I don't care 
too much.


 Thomas

Re: [PATCH v3 10/14] x86/vmx: move declarations used only by vmx code from vmx.h to private headers

[Xenia Ragiadakou]

On 2/27/23 17:25, Jan Beulich wrote:

On 24.02.2023 19:50, Xenia Ragiadakou wrote:

--- /dev/null
+++ b/xen/arch/x86/hvm/vmx/pi.h
@@ -0,0 +1,78 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * pi.h: VMX Posted Interrupts
+ *
+ * Copyright (c) 2004, Intel Corporation.
+ */
+
+#ifndef __X86_HVM_VMX_PI_PRIV_H__
+#define __X86_HVM_VMX_PI_PRIV_H__


I can see that you need something to disambiguate the two vmx.h. But
here you don't need the PRIV infix, do you? Even in the private vmx.h
I'd like to ask to consider e.g. __VMX_H__ as the guard (and then
__PI_H__ here), rather than one which doesn't really match the
filename.


I do agree that adding _PRIV_ is off target.
For the purpose of disambiguation, the header guards need to conform to 
a well specified pattern guaranteed not to be used for anything else.
For that reason I would suggest the guard to include the path, not just 
the file name.
In any case, the pattern that is used to generate the header guards 
should be mentioned in the coding style doc.




Jan


--
Xenia

Re: [ANNOUNCE] Call for agenda items for 3 March Community Call @ 1600 UTC

[Jan Beulich]

On 27.02.2023 22:41, George Dunlap wrote:
> Note the following administrative conventions for the call:
> * Unless, agreed in the previous meeting otherwise, the call is on the 1st
> Thursday of each month at 1600 British Time (either GMT or BST)

Since nothing else was said, I suppose the title is off by one and it's
Thursday March 2nd?

Jan

Re: [PATCH v3 00/14] x86/hvm: {svm,vmx} {c,h} cleanup

[Jan Beulich]

On 28.02.2023 08:09, Xenia Ragiadakou wrote:
> 
> On 2/27/23 14:17, Jan Beulich wrote:
>> On 27.02.2023 13:06, Andrew Cooper wrote:
>>> On 27/02/2023 11:33 am, Jan Beulich wrote:
 On 27.02.2023 12:15, Andrew Cooper wrote:
> On 27/02/2023 10:46 am, Jan Beulich wrote:
>> On 24.02.2023 22:33, Andrew Cooper wrote:
>>> But I think we want to change tact slightly at this point, so I'm not
>>> going to do any further tweaking on commit.
>>>
>>> Next, I think we want to rename asm/hvm/svm/svm.h to asm/hvm/svm.h,
>>> updating the non-SVM include paths as we go.  Probably best to
>>> chain-include the other svm/hvm/svm/*.h headers temporarily, so we've
>>> only got one tree-wide cleanup of the external include paths.
>>>
>>> Quick tangent - I will be making all of that cpu_has_svm_*
>>> infrastructure disappear by moving it into the normal CPUID handling,
>>> but I've not had sufficient time to finish that yet.
>>>
>>> Next, hvm/svm/nestedsvm.h can merge straight into hvm/svm.h, and
>>> disappear (after my decoupling patch has gone in).
>> Why would you want to fold hvm/svm/nestedsvm.h into hvm/svm/svm.h?
>> The latter doesn't use anything from the former, does it?
> It's about what else uses them.
>
> hvm_vcpu needs both svm_vcpu and nestedsvm, so both headers are always
> included in tandem.
 Well, yes, that's how things are today. But can you explain to me why
 hvm_vcpu has to know nestedsvm's layout?
>>>
>>> Because it's part of the same single memory allocation.
>>
>> Which keeps growing, and sooner or later we'll need to find something
>> again to split off, so we won't exceed a page in size. The nested
>> structures would, to me, look to be prime candidates for such.
>>
 If the field was a pointer,
 a forward decl of that struct would suffice, and any entity in the
 rest of Xen not caring about struct nestedsvm would no longer see it
 (and hence also no longer be re-built if a change is made there).
>>>
>>> Yes, you could hide it as a pointer.  The cost of doing so is an
>>> unnecessary extra memory allocation, and extra pointer handling on
>>> create/destroy, not to mention extra pointer chasing in memory during use.
>>>
> nestedsvm is literally just one struct now, and no subsystem ought to
> have multiple headers when one will do.
 When one will do, yes. Removing build dependencies is a good reason
 to have separate headers, though.
>>>
>>> Its not the only only option, and an #ifdef CONFIG_NESTED_VIRT inside
>>> the struct would be an equally acceptable way of doing this which
>>> wouldn't involve making an extra memory allocation.
>>
>> That would make it a build-time decision, but then on NESTED_VIRT=y
>> hypervisors there might still be guests not meaning to use that
>> functionality, and for quite some time that may actually be a majority.
>>
>>> Everything you've posed here is way out of scope for Xenia's series.
>>
>> There was never an intention to extend the scope of the work she's doing.
>> Instead I was trying to limit the scope by suggesting to avoid a piece
>> of rework which later may want undoing.
> 
> Can I suggest to leave hvm/svm/svm.h and hvm/svm/nestedsvm.h separate 
> for now?

As per before - that's my preference. It'll be Andrew who you would need
to convince, as he did suggest the folding.

Jan

Re: [PATCH v3 00/14] x86/hvm: {svm,vmx} {c,h} cleanup

[Xenia Ragiadakou]

On 2/27/23 14:17, Jan Beulich wrote:

On 27.02.2023 13:06, Andrew Cooper wrote:

On 27/02/2023 11:33 am, Jan Beulich wrote:

On 27.02.2023 12:15, Andrew Cooper wrote:

On 27/02/2023 10:46 am, Jan Beulich wrote:

On 24.02.2023 22:33, Andrew Cooper wrote:

But I think we want to change tact slightly at this point, so I'm not
going to do any further tweaking on commit.

Next, I think we want to rename asm/hvm/svm/svm.h to asm/hvm/svm.h,
updating the non-SVM include paths as we go.  Probably best to
chain-include the other svm/hvm/svm/*.h headers temporarily, so we've
only got one tree-wide cleanup of the external include paths.

Quick tangent - I will be making all of that cpu_has_svm_*
infrastructure disappear by moving it into the normal CPUID handling,
but I've not had sufficient time to finish that yet.

Next, hvm/svm/nestedsvm.h can merge straight into hvm/svm.h, and
disappear (after my decoupling patch has gone in).

Why would you want to fold hvm/svm/nestedsvm.h into hvm/svm/svm.h?
The latter doesn't use anything from the former, does it?

It's about what else uses them.

hvm_vcpu needs both svm_vcpu and nestedsvm, so both headers are always
included in tandem.

Well, yes, that's how things are today. But can you explain to me why
hvm_vcpu has to know nestedsvm's layout?


Because it's part of the same single memory allocation.


Which keeps growing, and sooner or later we'll need to find something
again to split off, so we won't exceed a page in size. The nested
structures would, to me, look to be prime candidates for such.


If the field was a pointer,
a forward decl of that struct would suffice, and any entity in the
rest of Xen not caring about struct nestedsvm would no longer see it
(and hence also no longer be re-built if a change is made there).


Yes, you could hide it as a pointer.  The cost of doing so is an
unnecessary extra memory allocation, and extra pointer handling on
create/destroy, not to mention extra pointer chasing in memory during use.


nestedsvm is literally just one struct now, and no subsystem ought to
have multiple headers when one will do.

When one will do, yes. Removing build dependencies is a good reason
to have separate headers, though.


Its not the only only option, and an #ifdef CONFIG_NESTED_VIRT inside
the struct would be an equally acceptable way of doing this which
wouldn't involve making an extra memory allocation.


That would make it a build-time decision, but then on NESTED_VIRT=y
hypervisors there might still be guests not meaning to use that
functionality, and for quite some time that may actually be a majority.


Everything you've posed here is way out of scope for Xenia's series.


There was never an intention to extend the scope of the work she's doing.
Instead I was trying to limit the scope by suggesting to avoid a piece
of rework which later may want undoing.


Can I suggest to leave hvm/svm/svm.h and hvm/svm/nestedsvm.h separate 
for now?




Jan


--
Xenia

Is xl vcpu-set broken

[Joe Jin]

Hi,

We encountered a vcpu-set issue on old xen, when I tried to confirm
if xen upstream xen has the same issue I find neither my upstream build
nor ubuntu 22.04 xen-hypervisor-4.16 work.

I can add vcpus(8->16) to my guest but I can not reduce vcpu number:

root@ubuntu2204:~/vm# xl list
Name    ID   Mem VCPUs    State    Time(s)
Domain-0 0 255424    32 r- 991.9
testvm   1   4088    16 -b  94.6
root@ubuntu2204:~/vm# xl vcpu-set testvm 8
root@ubuntu2204:~/vm# xl list
Name    ID   Mem VCPUs    State    Time(s)
Domain-0 0 255424    32 r- 998.5
testvm   1   4088    16 -b  97.3

After xl vcpu-set, xenstore showed online cpu number reduced to 8:

/local/domain/1/vm = "/vm/aa109ea0-2fde-4712-81d8-75f73bd73715"
/local/domain/1/name = "testvm"
/local/domain/1/cpu = ""
/local/domain/1/cpu/0 = ""
/local/domain/1/cpu/0/availability = "online"
/local/domain/1/cpu/1 = ""
/local/domain/1/cpu/1/availability = "online"
/local/domain/1/cpu/2 = ""
/local/domain/1/cpu/2/availability = "online"
/local/domain/1/cpu/3 = ""
/local/domain/1/cpu/3/availability = "online"
/local/domain/1/cpu/4 = ""
/local/domain/1/cpu/4/availability = "online"
/local/domain/1/cpu/5 = ""
/local/domain/1/cpu/5/availability = "online"
/local/domain/1/cpu/6 = ""
/local/domain/1/cpu/6/availability = "online"
/local/domain/1/cpu/7 = ""
/local/domain/1/cpu/7/availability = "online"
/local/domain/1/cpu/8 = ""
/local/domain/1/cpu/8/availability = "offline"
/local/domain/1/cpu/9 = ""
/local/domain/1/cpu/9/availability = "offline"
/local/domain/1/cpu/10 = ""
/local/domain/1/cpu/10/availability = "offline"
/local/domain/1/cpu/11 = ""
/local/domain/1/cpu/11/availability = "offline"
/local/domain/1/cpu/12 = ""
/local/domain/1/cpu/12/availability = "offline"
/local/domain/1/cpu/13 = ""
/local/domain/1/cpu/13/availability = "offline"
/local/domain/1/cpu/14 = ""
/local/domain/1/cpu/14/availability = "offline"
/local/domain/1/cpu/15 = ""
/local/domain/1/cpu/15/availability = "offline"
/local/domain/1/cpu/16 = ""
/local/domain/1/cpu/16/availability = "offline"

But guest did not received any offline events at all.

>From source code my understand is for cpu online, libxl will
send device_add to qemu to trigger vcpu add, for cpu offline,
it still rely on xenstore, is this correct? anything can block
cpu scale down?

Appreciate for any suggestions!

Thanks,
Joe

[xen-unstable test] 178673: regressions - trouble: fail/pass/starved

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-libvirt-vhd 20 guest-start.2fail REGR. vs. 178515

Tests which did not succeed, but are not blocking:
 test-amd64-i386-xl-qemuu-debianhvm-amd64  7 xen-install   fail like 178616
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 178616
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 178616
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 178616
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 178616
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 178616
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 178616
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 178616
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 178616
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 178616
 test-amd64-i386-xl-pvshim14 guest-start  fail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  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-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  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-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-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 build-armhf-libvirt   1 build-check(1)   starved  n/a
 test-armhf-armhf-examine  1 build-check(1)   starved  n/a
 test-armhf-armhf-libvirt  1 build-check(1)   starved  n/a
 test-armhf-armhf-libvirt-qcow2  1 build-check(1)   starved  n/a
 test-armhf-armhf-libvirt-raw  1 build-check(1)   starved  n/a
 test-armhf-armhf-xl   1 build-check(1)   starved  n/a
 test-armhf-armhf-xl-credit1   1 build-check(1)   starved  n/a
 test-armhf-armhf-xl-credit2   1 build-check(1)   starved  n/a
 test-armhf-armhf-xl-cubietruck  1 build-check(1)   starved  n/a
 test-armhf-armhf-xl-multivcpu  1 build-check(1)   starved  n/a
 test-armhf-armhf-xl-rtds  1 build-check(1)   starved  n/a
 test-armhf-armhf-xl-vhd   1 build-check(1)   starved  n/a
 build-armhf   2 hosts-allocate   starved  n/a

version targeted for testing:
 xen  4d6df4ec7544d7c912ffab6b6edb4cbefaa01f4c
baseline version:
 xen  608f85a1818697156b72ace4913a17c8178a0ef5

Last test of basis   178616  2023-02-27 01:53:22 Z1 days
Testing same since   178673  2023-02-27 15:09:21 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Jan Beulich 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64-xtf  pass
 build-amd64   

[xen-unstable-smoke test] 178708: tolerable trouble: pass/starved - PUSHED

[osstest service owner]

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

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   1 build-check(1)   starved  n/a
 build-armhf   2 hosts-allocate   starved  n/a

version targeted for testing:
 xen  bfc3780f23ded229f42a2565783e21c32083bbfd
baseline version:
 xen  49b1cb27413034c81023d1faf7af43690e87291a

Last test of basis   178690  2023-02-27 19:01:55 Z0 days
Testing same since   178708  2023-02-27 23:00:25 Z0 days1 attempts


People who touched revisions under test:
  Michal Orzel 
  Stefano Stabellini 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  starved 
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  starved 
 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
   49b1cb2741..bfc3780f23  bfc3780f23ded229f42a2565783e21c32083bbfd -> smoke

Re: [PATCH] xen-netback: remove unused variables pending_idx and index

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski :

On Sun, 26 Feb 2023 11:34:29 -0500 you wrote:
> building with gcc and W=1 reports
> drivers/net/xen-netback/netback.c:886:21: error: variable
>   ‘pending_idx’ set but not used [-Werror=unused-but-set-variable]
>   886 | u16 pending_idx;
>   | ^~~
> 
> pending_idx is not used so remove it.  Since index was only
> used to set pending_idx, remove index as well.
> 
> [...]

Here is the summary with links:
  - xen-netback: remove unused variables pending_idx and index
https://git.kernel.org/netdev/net-next/c/ccf8f7d71424

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH] xen-netback: remove unused variables pending_idx and index

[Jakub Kicinski]

On Mon, 27 Feb 2023 09:29:30 +0100 Juergen Gross wrote:
> On 26.02.23 17:34, Tom Rix wrote:
> > building with gcc and W=1 reports
> > drivers/net/xen-netback/netback.c:886:21: error: variable
> >‘pending_idx’ set but not used [-Werror=unused-but-set-variable]
> >886 | u16 pending_idx;
> >| ^~~
> > 
> > pending_idx is not used so remove it.  Since index was only
> > used to set pending_idx, remove index as well.
> > 
> > Signed-off-by: Tom Rix   
> 
> Reviewed-by: Juergen Gross 

Applied, thanks!

Re: [PATCH v3] automation: Add container and build jobs to run cppcheck analysis

[Stefano Stabellini]

On Fri, 24 Feb 2023, Michal Orzel wrote:
> Add a debian container with cppcheck installation routine inside,
> capable of performing cppcheck analysis on Xen-only build including
> cross-builds for arm32 and x86_64.
> 
> Populate build jobs making use of that container to run cppcheck
> analysis to produce a text report (xen-cppcheck.txt) containing the list
> of all the findings.
> 
> This patch does not aim at performing any sort of bisection. Cppcheck is
> imperfect and for now, our goal is to at least be aware of its reports,
> so that we can compare them with the ones produced by better tools and
> to be able to see how these reports change as a result of further
> infrastructure improvements (e.g. exception list, rules exclusion).
> 
> Signed-off-by: Michal Orzel 

Reviewed-by: Stefano Stabellini 


> ---
> Changes in v3:
>  - use multi-stage build to reduce the size of container
>  - drop Stefano Rb as a result of dockefile changes
> 
> Changes in v2:
>  - use arm64 container instead of x86 to make pipeline faster
>  - explicitly set HYPERVISOR_ONLY=y for cppcheck jobs
> ---
>  .../build/debian/unstable-cppcheck.dockerfile | 53 +++
>  automation/gitlab-ci/build.yaml   | 43 +++
>  automation/scripts/build  | 11 +++-
>  3 files changed, 106 insertions(+), 1 deletion(-)
>  create mode 100644 automation/build/debian/unstable-cppcheck.dockerfile
> 
> diff --git a/automation/build/debian/unstable-cppcheck.dockerfile 
> b/automation/build/debian/unstable-cppcheck.dockerfile
> new file mode 100644
> index ..adc192cea645
> --- /dev/null
> +++ b/automation/build/debian/unstable-cppcheck.dockerfile
> @@ -0,0 +1,53 @@
> +FROM arm64v8/debian:unstable AS builder
> +
> +ENV DEBIAN_FRONTEND=noninteractive
> +ENV CPPCHECK_VERSION=2.7
> +ENV USER root
> +
> +# dependencies for cppcheck build
> +RUN apt-get update && \
> +apt-get --quiet --yes install \
> +curl \
> +build-essential \
> +python-is-python3 \
> +libpcre3-dev
> +
> +RUN mkdir /build
> +WORKDIR /build
> +
> +# cppcheck release build (see cppcheck readme.md)
> +RUN curl -fsSLO 
> https://github.com/danmar/cppcheck/archive/"$CPPCHECK_VERSION".tar.gz && \
> +tar xvzf "$CPPCHECK_VERSION".tar.gz && \
> +cd cppcheck-"$CPPCHECK_VERSION" && \
> +make install -j$(nproc) \
> +MATCHCOMPILER=yes \
> +FILESDIR=/usr/share/cppcheck \
> +HAVE_RULES=yes CXXFLAGS="-O2 -DNDEBUG -Wall -Wno-sign-compare 
> -Wno-unused-function"
> +
> +FROM arm64v8/debian:unstable
> +COPY --from=builder /usr/bin/cppcheck /usr/bin/cppcheck
> +COPY --from=builder /usr/share/cppcheck /usr/share/cppcheck
> +
> +LABEL maintainer.name="The Xen Project" \
> +  maintainer.email="xen-devel@lists.xenproject.org"
> +
> +ENV DEBIAN_FRONTEND=noninteractive
> +ENV USER root
> +
> +RUN mkdir /build
> +WORKDIR /build
> +
> +# dependencies for cppcheck analysis including Xen-only build/cross-build
> +RUN apt-get update && \
> +apt-get --quiet --yes install \
> +build-essential \
> +python-is-python3 \
> +libpcre3-dev \
> +flex \
> +bison \
> +gcc-arm-linux-gnueabihf \
> +gcc-x86-64-linux-gnu \
> +&& \
> +apt-get autoremove -y && \
> +apt-get clean && \
> +rm -rf /var/lib/apt/lists* /tmp/* /var/tmp/*
> diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
> index 22ce1c45e7cd..0835b7a65190 100644
> --- a/automation/gitlab-ci/build.yaml
> +++ b/automation/gitlab-ci/build.yaml
> @@ -7,6 +7,7 @@
>  paths:
>- binaries/
>- xen-config
> +  - xen-cppcheck.txt
>- '*.log'
>- '*/*.log'
>  when: always
> @@ -199,6 +200,23 @@
>variables:
>  <<: *gcc
>  
> +.x86-64-cross-build-tmpl:
> +  <<: *build
> +  variables:
> +XEN_TARGET_ARCH: x86_64
> +  tags:
> +- arm64
> +
> +.x86-64-cross-build:
> +  extends: .x86-64-cross-build-tmpl
> +  variables:
> +debug: n
> +
> +.gcc-x86-64-cross-build:
> +  extends: .x86-64-cross-build
> +  variables:
> +<<: *gcc
> +
>  # Jobs below this line
>  
>  archlinux-gcc:
> @@ -679,6 +697,31 @@ archlinux-current-gcc-riscv64-debug-randconfig:
>  EXTRA_FIXED_RANDCONFIG:
>CONFIG_COVERAGE=n
>  
> +# Cppcheck analysis jobs
> +
> +debian-unstable-gcc-cppcheck:
> +  extends: .gcc-x86-64-cross-build
> +  variables:
> +CONTAINER: debian:unstable-cppcheck
> +CROSS_COMPILE: /usr/bin/x86_64-linux-gnu-
> +CPPCHECK: y
> +HYPERVISOR_ONLY: y
> +
> +debian-unstable-gcc-arm32-cppcheck:
> +  extends: .gcc-arm32-cross-build
> +  variables:
> +CONTAINER: debian:unstable-cppcheck
> +CROSS_COMPILE: /usr/bin/arm-linux-gnueabihf-
> +CPPCHECK: y
> +HYPERVISOR_ONLY: y
> +
> +debian-unstable-gcc-arm64-cppcheck:
> +  extends: .gcc-arm64-build
> +  variables:
> +CONTAINER: debian:unstable-cppcheck
> +CPPCHECK: y
> +HYPERVISOR_ONLY: y
> +
>  ## 

Re: [PATCH 1/2] docs/about: Deprecate 32-bit x86 hosts and qemu-system-i386

[Philippe Mathieu-Daudé]

On 27/2/23 21:12, Michael S. Tsirkin wrote:

On Mon, Feb 27, 2023 at 11:50:07AM +, Daniel P. Berrangé wrote:

I feel like we should have separate deprecation entries for the
i686 host support, and for qemu-system-i386 emulator binary, as
although they're related they are independant features with
differing impact. eg removing qemu-system-i386 affects all
host architectures, not merely 32-bit x86 host, so I think we
can explain the impact more clearly if we separate them.


Removing qemu-system-i386 seems ok to me - I think qemu-system-x86_64 is
a superset.


Doesn't qemu-system-i386 start the CPU in a different mode that
qemu-system-x86_64? Last time we discussed it, we mention adding
-32 and -64 CLI flags to maintain compat, and IIRC this flag would
add boot code to switch the CPU in 32-b. But then maybe I misunderstood.
Thomas said, "CPUs must start in the same mode they start in HW".


Removing support for building on 32 bit systems seems like a pity - it's
one of a small number of ways to run 64 bit binaries on 32 bit systems,
and the maintainance overhead is quite small.

In fact, keeping this support around forces correct use of
posix APIs such as e.g. PRIx64 which makes the code base
more future-proof.

[PATCH v7 13/41] mm: Make pte_mkwrite() take a VMA

[Rick Edgecombe]

The x86 Control-flow Enforcement Technology (CET) feature includes a new
type of memory called shadow stack. This shadow stack memory has some
unusual properties, which requires some core mm changes to function
properly.

One of these unusual properties is that shadow stack memory is writable,
but only in limited ways. These limits are applied via a specific PTE
bit combination. Nevertheless, the memory is writable, and core mm code
will need to apply the writable permissions in the typical paths that
call pte_mkwrite().

In addition to VM_WRITE, the shadow stack VMA's will have a flag denoting
that they are special shadow stack flavor of writable memory. So make
pte_mkwrite() take a VMA, so that the x86 implementation of it can know to
create regular writable memory or shadow stack memory.

Apply the same changes for pmd_mkwrite() and huge_pte_mkwrite().

No functional change.

Cc: linux-...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Cc: linux-al...@vger.kernel.org
Cc: linux-snps-...@lists.infradead.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-c...@vger.kernel.org
Cc: linux-hexa...@vger.kernel.org
Cc: linux-i...@vger.kernel.org
Cc: loonga...@lists.linux.dev
Cc: linux-m...@lists.linux-m68k.org
Cc: Michal Simek 
Cc: Dinh Nguyen 
Cc: linux-m...@vger.kernel.org
Cc: linux-openr...@vger.kernel.org
Cc: linux-par...@vger.kernel.org
Cc: linuxppc-...@lists.ozlabs.org
Cc: linux-ri...@lists.infradead.org
Cc: linux-s...@vger.kernel.org
Cc: linux...@vger.kernel.org
Cc: sparcli...@vger.kernel.org
Cc: linux...@lists.infradead.org
Cc: xen-devel@lists.xenproject.org
Cc: linux-a...@vger.kernel.org
Cc: linux...@kvack.org
Tested-by: Pengfei Xu 
Tested-by: John Allen 
Tested-by: Kees Cook 
Acked-by: Mike Rapoport (IBM) 
Acked-by: Michael Ellerman 
Acked-by: David Hildenbrand 
Reviewed-by: Kees Cook 
Suggested-by: David Hildenbrand 
Signed-off-by: Rick Edgecombe 

---
Hi Non-x86 Arch’s,

x86 has a feature that allows for the creation of a special type of
writable memory (shadow stack) that is only writable in limited specific
ways. Previously, changes were proposed to core MM code to teach it to
decide when to create normally writable memory or the special shadow stack
writable memory, but David Hildenbrand suggested[0] to change
pXX_mkwrite() to take a VMA, so awareness of shadow stack memory can be
moved into x86 code.

Since pXX_mkwrite() is defined in every arch, it requires some tree-wide
changes. So that is why you are seeing some patches out of a big x86
series pop up in your arch mailing list. There is no functional change.
After this refactor, the shadow stack series goes on to use the arch
helpers to push shadow stack memory details inside arch/x86.

Testing was just 0-day build testing.

Hopefully that is enough context. Thanks!

[0] 
https://lore.kernel.org/lkml/0e29a2d0-08d8-bcd6-ff26-4bea0e403...@redhat.com/#t

v6:
 - New patch
---
 Documentation/mm/arch_pgtable_helpers.rst|  9 ++---
 arch/alpha/include/asm/pgtable.h |  6 +-
 arch/arc/include/asm/hugepage.h  |  2 +-
 arch/arc/include/asm/pgtable-bits-arcv2.h|  7 ++-
 arch/arm/include/asm/pgtable-3level.h|  7 ++-
 arch/arm/include/asm/pgtable.h   |  2 +-
 arch/arm64/include/asm/pgtable.h |  4 ++--
 arch/csky/include/asm/pgtable.h  |  2 +-
 arch/hexagon/include/asm/pgtable.h   |  2 +-
 arch/ia64/include/asm/pgtable.h  |  2 +-
 arch/loongarch/include/asm/pgtable.h |  4 ++--
 arch/m68k/include/asm/mcf_pgtable.h  |  2 +-
 arch/m68k/include/asm/motorola_pgtable.h |  6 +-
 arch/m68k/include/asm/sun3_pgtable.h |  6 +-
 arch/microblaze/include/asm/pgtable.h|  2 +-
 arch/mips/include/asm/pgtable.h  |  6 +++---
 arch/nios2/include/asm/pgtable.h |  2 +-
 arch/openrisc/include/asm/pgtable.h  |  2 +-
 arch/parisc/include/asm/pgtable.h|  6 +-
 arch/powerpc/include/asm/book3s/32/pgtable.h |  2 +-
 arch/powerpc/include/asm/book3s/64/pgtable.h |  4 ++--
 arch/powerpc/include/asm/nohash/32/pgtable.h |  2 +-
 arch/powerpc/include/asm/nohash/32/pte-8xx.h |  2 +-
 arch/powerpc/include/asm/nohash/64/pgtable.h |  2 +-
 arch/riscv/include/asm/pgtable.h |  6 +++---
 arch/s390/include/asm/hugetlb.h  |  4 ++--
 arch/s390/include/asm/pgtable.h  |  4 ++--
 arch/sh/include/asm/pgtable_32.h | 10 --
 arch/sparc/include/asm/pgtable_32.h  |  2 +-
 arch/sparc/include/asm/pgtable_64.h  |  6 +++---
 arch/um/include/asm/pgtable.h|  2 +-
 arch/x86/include/asm/pgtable.h   |  6 --
 arch/xtensa/include/asm/pgtable.h|  2 +-
 include/asm-generic/hugetlb.h|  4 ++--
 include/linux/mm.h   |  2 +-
 mm/debug_vm_pgtable.c| 16 
 mm/huge_memory.c |  6 +++---
 mm/hugetlb.c 

[PATCH v7 11/41] mm: Introduce pte_mkwrite_kernel()

[Rick Edgecombe]

The x86 Control-flow Enforcement Technology (CET) feature includes a new
type of memory called shadow stack. This shadow stack memory has some
unusual properties, which requires some core mm changes to function
properly.

One of these changes is to allow for pte_mkwrite() to create different
types of writable memory (the existing conventionally writable type and
also the new shadow stack type). Future patches will convert pte_mkwrite()
to take a VMA in order to facilitate this, however there are places in the
kernel where pte_mkwrite() is called outside of the context of a VMA.
These are for kernel memory. So create a new variant called
pte_mkwrite_kernel() and switch the kernel users over to it. Have
pte_mkwrite() and pte_mkwrite_kernel() be the same for now. Future patches
will introduce changes to make pte_mkwrite() take a VMA.

Only do this for architectures that need it because they call pte_mkwrite()
in arch code without an associated VMA. Since it will only currently be
used in arch code, so do not include it in arch_pgtable_helpers.rst.

Cc: linux-...@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-s...@vger.kernel.org
Cc: xen-devel@lists.xenproject.org
Cc: linux-a...@vger.kernel.org
Cc: linux...@kvack.org
Tested-by: Pengfei Xu 
Tested-by: John Allen 
Tested-by: Kees Cook 
Acked-by: Mike Rapoport (IBM) 
Acked-by: David Hildenbrand 
Reviewed-by: Kees Cook 
Suggested-by: David Hildenbrand 
Signed-off-by: Rick Edgecombe 

---
Hi Non-x86 Arch’s,

x86 has a feature that allows for the creation of a special type of
writable memory (shadow stack) that is only writable in limited specific
ways. Previously, changes were proposed to core MM code to teach it to
decide when to create normally writable memory or the special shadow stack
writable memory, but David Hildenbrand suggested[0] to change
pXX_mkwrite() to take a VMA, so awareness of shadow stack memory can be
moved into x86 code.

Since pXX_mkwrite() is defined in every arch, it requires some tree-wide
changes. So that is why you are seeing some patches out of a big x86
series pop up in your arch mailing list. There is no functional change.
After this refactor, the shadow stack series goes on to use the arch
helpers to push shadow stack memory details inside arch/x86.

Testing was just 0-day build testing.

Hopefully that is enough context. Thanks!

[0] 
https://lore.kernel.org/lkml/0e29a2d0-08d8-bcd6-ff26-4bea0e403...@redhat.com/#t

v6:
 - New patch
---
 arch/arm64/include/asm/pgtable.h | 7 ++-
 arch/arm64/mm/trans_pgd.c| 4 ++--
 arch/s390/include/asm/pgtable.h  | 7 ++-
 arch/s390/mm/pageattr.c  | 2 +-
 arch/x86/include/asm/pgtable.h   | 7 ++-
 arch/x86/xen/mmu_pv.c| 2 +-
 6 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index b6ba466e2e8a..cccf8885792e 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -180,13 +180,18 @@ static inline pmd_t set_pmd_bit(pmd_t pmd, pgprot_t prot)
return pmd;
 }
 
-static inline pte_t pte_mkwrite(pte_t pte)
+static inline pte_t pte_mkwrite_kernel(pte_t pte)
 {
pte = set_pte_bit(pte, __pgprot(PTE_WRITE));
pte = clear_pte_bit(pte, __pgprot(PTE_RDONLY));
return pte;
 }
 
+static inline pte_t pte_mkwrite(pte_t pte)
+{
+   return pte_mkwrite_kernel(pte);
+}
+
 static inline pte_t pte_mkclean(pte_t pte)
 {
pte = clear_pte_bit(pte, __pgprot(PTE_DIRTY));
diff --git a/arch/arm64/mm/trans_pgd.c b/arch/arm64/mm/trans_pgd.c
index 4ea2eefbc053..5c07e68d80ea 100644
--- a/arch/arm64/mm/trans_pgd.c
+++ b/arch/arm64/mm/trans_pgd.c
@@ -40,7 +40,7 @@ static void _copy_pte(pte_t *dst_ptep, pte_t *src_ptep, 
unsigned long addr)
 * read only (code, rodata). Clear the RDONLY bit from
 * the temporary mappings we use during restore.
 */
-   set_pte(dst_ptep, pte_mkwrite(pte));
+   set_pte(dst_ptep, pte_mkwrite_kernel(pte));
} else if (debug_pagealloc_enabled() && !pte_none(pte)) {
/*
 * debug_pagealloc will removed the PTE_VALID bit if
@@ -53,7 +53,7 @@ static void _copy_pte(pte_t *dst_ptep, pte_t *src_ptep, 
unsigned long addr)
 */
BUG_ON(!pfn_valid(pte_pfn(pte)));
 
-   set_pte(dst_ptep, pte_mkpresent(pte_mkwrite(pte)));
+   set_pte(dst_ptep, pte_mkpresent(pte_mkwrite_kernel(pte)));
}
 }
 
diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
index 2c70b4d1263d..d4943f2d3f00 100644
--- a/arch/s390/include/asm/pgtable.h
+++ b/arch/s390/include/asm/pgtable.h
@@ -1005,7 +1005,7 @@ static inline pte_t pte_wrprotect(pte_t pte)
return set_pte_bit(pte, __pgprot(_PAGE_PROTECT));
 }
 
-static inline pte_t pte_mkwrite(pte_t pte)
+static inline pte_t pte_mkwrite_kernel(pte_t pte)
 {
pte = set_pte_bit(pte, 

[linux-linus test] 178669: regressions - trouble: fail/pass/starved

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-qemuu-nested-intel  8 xen-boot  fail REGR. vs. 178042
 test-amd64-amd64-freebsd12-amd64  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-qemuu-ws16-amd64  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-dom0pvh-xl-amd 14 guest-start   fail REGR. vs. 178042
 test-amd64-amd64-xl-pvshim8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-vhd   8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-pvhv2-intel  8 xen-boot  fail REGR. vs. 178042
 test-amd64-amd64-xl-qemuu-win7-amd64  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-qemut-win7-amd64  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-xsm   8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-credit1   8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 8 xen-boot fail REGR. 
vs. 178042
 test-amd64-amd64-xl   8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 8 xen-boot fail REGR. vs. 
178042
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 8 xen-boot fail REGR. vs. 
178042
 test-amd64-amd64-xl-qemuu-debianhvm-amd64  8 xen-bootfail REGR. vs. 178042
 test-amd64-amd64-xl-shadow8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-qemuu-nested-amd  8 xen-bootfail REGR. vs. 178042
 test-amd64-amd64-xl-pvhv2-amd  8 xen-bootfail REGR. vs. 178042
 test-amd64-amd64-xl-qemut-debianhvm-i386-xsm  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-qemut-ws16-amd64  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-freebsd11-amd64  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-libvirt-raw  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-pygrub   8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 8 xen-boot fail REGR. 
vs. 178042
 test-amd64-amd64-dom0pvh-xl-intel 14 guest-start fail REGR. vs. 178042
 test-amd64-amd64-pair12 xen-boot/src_hostfail REGR. vs. 178042
 test-amd64-amd64-pair13 xen-boot/dst_hostfail REGR. vs. 178042
 test-amd64-amd64-libvirt-qcow2  8 xen-boot   fail REGR. vs. 178042
 test-amd64-amd64-libvirt-xsm  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-qemut-debianhvm-amd64  8 xen-bootfail REGR. vs. 178042
 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-libvirt  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-libvirt-pair 12 xen-boot/src_host   fail REGR. vs. 178042
 test-amd64-amd64-libvirt-pair 13 xen-boot/dst_host   fail REGR. vs. 178042
 test-amd64-coresched-amd64-xl  8 xen-bootfail REGR. vs. 178042
 test-arm64-arm64-xl-thunderx 18 guest-start/debian.repeat fail REGR. vs. 178042
 test-arm64-arm64-xl-credit1 18 guest-start/debian.repeat fail REGR. vs. 178042
 test-amd64-amd64-examine-uefi  8 reboot  fail REGR. vs. 178042
 test-amd64-amd64-xl-qemuu-ovmf-amd64  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-credit2   8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-examine  8 reboot   fail REGR. vs. 178042
 test-amd64-amd64-examine-bios  8 reboot  fail REGR. vs. 178042
 test-amd64-amd64-xl-multivcpu  8 xen-bootfail REGR. vs. 178042
 test-arm64-arm64-xl-vhd  12 debian-di-installfail REGR. vs. 178042
 test-arm64-arm64-libvirt-raw 12 debian-di-installfail REGR. vs. 178042
 test-arm64-arm64-xl-xsm 18 guest-start/debian.repeat fail in 178595 REGR. vs. 
178042
 test-arm64-arm64-xl  17 guest-stop fail in 178623 REGR. vs. 178042
 test-arm64-arm64-xl-credit2  17 guest-stop fail in 178623 REGR. vs. 178042
 test-arm64-arm64-libvirt-xsm 17 guest-stop fail in 178623 REGR. vs. 178042

Tests which are failing intermittently (not blocking):
 test-arm64-arm64-xl-credit1  14 guest-start  fail in 178595 pass in 178669
 test-arm64-arm64-xl-credit1  17 guest-stop   fail in 178623 pass in 178669
 test-arm64-arm64-xl-thunderx 14 guest-start  fail in 178623 pass in 178669
 test-arm64-arm64-xl-xsm  14 guest-startfail pass in 178595
 test-arm64-arm64-xl  14 guest-startfail pass in 178623
 test-arm64-arm64-xl-credit2  14 guest-startfail pass in 178623
 test-arm64-arm64-libvirt-xsm 14 guest-startfail pass in 178623

Regressions which are regarded as allowable (not blocking):
 test-amd64-amd64-xl-rtds  8 xen-boot fail REGR. vs. 178042

Tests 

Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json

[Stefano Stabellini]

On Fri, 24 Feb 2023, Luca Fancellu wrote:
> Hi Stefano,
> 
> >> Hi Jan,
> >> 
> >> my personal opinion is that we can’t handle them for files that needs to 
> >> be kept
> >> in sync from an external source, because we can’t justify findings or tag 
> >> false
> >> positives, if we are lucky we might fix the non compliances but even in 
> >> that case
> >> there might be times when the fix goes through and cases where the fix is 
> >> objected
> >> by the maintainers just because their project is not following the MISRA 
> >> standard.
> >> 
> >> On our side, what we can do is track these files and from time to time 
> >> check that
> >> they are still eligible to backport, because when they are not anymore we 
> >> could
> >> just port them to Xen coding style and start doing direct changes.
> >> 
> >> 
> >> @Stefano/@Bertrand/@Julien thanks for your effort on the list, yesterday 
> >> morning
> >> I’ve also had a look on the Michal’s file list and I’ve tracked down the 
> >> origin, here
> >> my findings, maybe we could merge your list with mine?
> > 
> > Yes to merge the two lists, but I think we should keep only items that we
> > actually need for the sake of having a cleaner baseline. So I would only
> > add things we need today to reduce the noise on cppcheck results
> > (excluding 20.7 and also excluding unusedStructMember which I think we
> > should probably stop scanning with cppcheck altogether).
> 
> Yes I thought about excluding unusedStructMember, I see there are a lot of 
> findings on x86
> which are not real findings, it’s just that the tool has not the complete 
> picture.
> 
> Here the ways are two:
> 1) exclude globally unusedStructMember
> 2) exclude unusedStructMember only on some selected files (available only on 
> cppcheck)
> this requires some work to add a field to this list, like 
> “cppcheck-error-exclude” and a list,
> comma separated, of error-id to be suppressed from the corresponding file.

For now, I would exclude globally


> Regarding the list, I merged your list with mine (and Michal’s work) to 
> create a complete list,
> I think it’s better to have it complete because all those files are external 
> and even if today we don’t
> have findings for some of these files, we could have some in the future, and 
> since we know today 
> that we can’t do direct changes to them, in my opinion it’s better to list 
> them now instead of forgetting
> them later.
> 
> I left out for now these files to start a discussion for them, because I 
> think they should be included in
> the analysis:
> 
> common/symbols-dummy.c:
>   this file accepts direct changes, cppcheck complains about misra-c2012-8.4 
> but I think it is right, also
>   Coverity complains about the same findings, they can be fixed adding 
> declarations on symbols.h I think
>   and removing the declarations from symbol.c module
> 
> common/version.c:
>   Apart from unusedStructMember, cppcheck is confused only on 2 findings that 
> compares one local symbol
>   and one linker defined symbol, could we have just one tag to suppress those 
> two findings instead of removing
>   completely the file? This file is under our control, so we could push 
> changes.
> 
> include/xen/lib.h:
>   Findings are from the bsearch function, which is derived from Linux, but I 
> can see the codestyle is the Xen style
>   and it seems (I might be wrong) that it has diverged from Linux, so we 
> might do changes and fix the findings that
>   are correct, void* arithmetic is working because gcc make it work assigning 
> a sizeof of 1, using char* pointers we
>   have the same result without having the undefined behaviour (correct me if 
> I am wrong).
> 
> include/xen/sort.h:
>   Also this one is derived from Linux, but seems that we converted it to our 
> coding style and we can do direct changes,
>   the same reasoning about void* pointers arithmetic applies here.
> 
> 
> What are your thoughts?

I am good with this


> Here the merged list, capturing also Jan comments:
> 
> {
>"version": "1.0",
>"content": [
>{
>"rel_path": "arch/arm/arm64/cpufeature.c",
>"comment": "Imported from Linux, ignore for now"
>},
>{
>"rel_path": "arch/arm/arm64/insn.c",
>"comment": "Imported on Linux"
>},
>{
>"rel_path": "arch/arm/arm64/lib/find_next_bit.c",
>"comment": "Imported from Linux, ignore for now"
>},
>{
>"rel_path": "arch/x86/acpi/boot.c",
>"comment": "Imported from Linux, ignore for now"
>},
>{
>"rel_path": "arch/x86/acpi/cpu_idle.c",
>"comment": "Imported from Linux, ignore for now"
>},
>{
>"rel_path": "arch/x86/acpi/cpufreq/cpufreq.c",
>"comment": "Imported from Linux, ignore for now"
>},
>{
>"rel_path": "arch/x86/acpi/cpuidle_menu.c",
>"comment": "Imported from 

Re: [ImageBuilder][PATCH] uboot-script-gen: Add virtio loader

[Stefano Stabellini]

On Tue, 21 Feb 2023, Pavel Zhukov wrote:
> uboot supports virtio-blk drives and can load kernel image from it.
> Adding option to use '-t virtio' for loading image from virtio device
> 
> Signed-off-by: Pavel Zhukov 

Reviewed-by: Stefano Stabellini 


> ---
>  README.md| 14 +++---
>  scripts/uboot-script-gen |  3 +++
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/README.md b/README.md
> index cb0cb13..64e62cd 100644
> --- a/README.md
> +++ b/README.md
> @@ -267,9 +267,9 @@ Where:\
>  -d specifies the "root" directory (paths in the config file are relative
> to it), this is not a working directory (any output file locations
> are specified in the config and any temporary files are in /tmp)\
> --t specifies the u-boot command to load the binaries. "tftp", "sd", "usb"
> -   and "scsi" are shorthands for "tftpb", "load mmc 0:1", "fatload usb 0:1"
> -   and "load scsi 0:1", but actually any arbitrary command can be used,
> +-t specifies the u-boot command to load the binaries. "tftp", "sd", "usb", 
> "virtio"
> +   and "scsi" are shorthands for "tftpb", "load mmc 0:1", "fatload usb 0:1",
> +   "virtio load 0:1" and "load scsi 0:1", but actually any arbitrary command 
> can be used,
> for instance -t "fatload" is valid.  The only special commands are:
> fit, which generates a FIT image using a script, and fit_std, which
> produces a standard style of fit image without a script, but has
> @@ -339,10 +339,10 @@ Where:\
>  -o specifies the output disk image file name\
>  -a specifies whether the disk image size is to be aligned to the nearest
> power of two\
> --t specifies the u-boot command to load the binaries. "tftp", "sd", "usb"
> -   and "scsi" are shorthands for "tftpb", "load mmc 0:1", "fatload usb 0:1"
> -   and "load scsi 0:1", but actually any arbitrary command can be used,
> -   for instance -t "fatload" is valid.
> +-t specifies the u-boot command to load the binaries. "tftp", "sd", "usb",
> +   "virtio" and "scsi" are shorthands for "tftpb", "load mmc 0:1",
> +   "fatload usb 0:1", "load virtio 0:1" and "load scsi 0:1", but actually
> +   any arbitrary command can be used, for instance -t "fatload" is valid.
>  
>  
>  disk\_image supports these additional parameters on the config file:
> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
> index 7e5cc08..f07e334 100755
> --- a/scripts/uboot-script-gen
> +++ b/scripts/uboot-script-gen
> @@ -1025,6 +1025,9 @@ while getopts ":c:t:d:ho:k:u:fp:" opt; do
>  sd )
>  load_opt="load mmc 0:1"
>  ;;
> +virtio )
> +load_opt="load virtio 0:1"
> +;;
>  usb )
>  load_opt="fatload usb 0:1"
>  ;;
> -- 
> 2.39.1
> 

[ANNOUNCE] Call for agenda items for 3 March Community Call @ 1600 UTC

[George Dunlap]

Hi all,

The proposed agenda is in
https://cryptpad.fr/pad/#/2/pad/edit/6pAe-+J05tY4OGg2sOobRrhO/ and you can
edit to add items.  Alternatively, you can reply to this mail directly.

Agenda items appreciated a few days before the call: please put your name
besides items if you edit the document.

Note the following administrative conventions for the call:
* Unless, agreed in the previous meeting otherwise, the call is on the 1st
Thursday of each month at 1600 British Time (either GMT or BST)
* I usually send out a meeting reminder a few days before with a
provisional agenda

* To allow time to switch between meetings, we'll plan on starting the
agenda at 16:05 sharp.  Aim to join by 16:03 if possible to allocate time
to sort out technical difficulties 

* If you want to be CC'ed please add or remove yourself from the
sign-up-sheet at
https://cryptpad.fr/pad/#/2/pad/edit/D9vGzihPxxAOe6RFPz0sRCf+/

Best Regards
George


== Dial-in Information ==
## Meeting time
16:00 - 17:00 UTC
Further International meeting times:


https://www.timeanddate.com/worldclock/meetingdetails.html?year=2023=3=2=16=0=0=1234=37=224=179


## Dial in details
Web: https://meet.jit.si/XenProjectCommunityCall

Dial-in info and pin can be found here:

https://meet.jit.si/static/dialInInfo.html?room=XenProjectCommunityCall

Re: [PATCH] CI: Simplify RISCV smoke testing

[Stefano Stabellini]

On Fri, 24 Feb 2023, Andrew Cooper wrote:
> Use a single fairly generic string as the "all done" message to look for,
> which avoids the need to patch qemu-smoke-riscv64.sh each time a new feature
> is added.
> 
> Signed-off-by: Andrew Cooper 

Acked-by: Stefano Stabellini 


> ---
> CC: Oleksii Kurochko 
> CC: Bob Eshleman 
> CC: Alistair Francis 
> CC: Connor Davis 
> CC: Anthony PERARD 
> CC: Stefano Stabellini 
> CC: Michal Orzel 
> CC: Doug Goldstein 
> 
> I considered "All set up and nowhere to go" but it's probably a little niche.
> ---
>  automation/scripts/qemu-smoke-riscv64.sh | 2 +-
>  xen/arch/riscv/setup.c   | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/automation/scripts/qemu-smoke-riscv64.sh 
> b/automation/scripts/qemu-smoke-riscv64.sh
> index e0f06360bc43..4008191302f9 100755
> --- a/automation/scripts/qemu-smoke-riscv64.sh
> +++ b/automation/scripts/qemu-smoke-riscv64.sh
> @@ -16,5 +16,5 @@ qemu-system-riscv64 \
>  |& tee smoke.serial
>  
>  set -e
> -(grep -q "Hello from C env" smoke.serial) || exit 1
> +(grep -q "All set up" smoke.serial) || exit 1
>  exit 0
> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> index d09ffe1454a4..1c87899e8e90 100644
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -11,6 +11,7 @@ void __init noreturn start_xen(void)
>  {
>  early_printk("Hello from C env\n");
>  
> +early_printk("All set up\n");
>  for ( ;; )
>  asm volatile ("wfi");
>  
> -- 
> 2.30.2
> 

[xen-unstable-smoke test] 178690: tolerable trouble: pass/starved - PUSHED

[osstest service owner]

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

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   1 build-check(1)   starved  n/a
 build-armhf   2 hosts-allocate   starved  n/a

version targeted for testing:
 xen  49b1cb27413034c81023d1faf7af43690e87291a
baseline version:
 xen  4d6df4ec7544d7c912ffab6b6edb4cbefaa01f4c

Last test of basis   178663  2023-02-27 13:03:21 Z0 days
Testing same since   178690  2023-02-27 19:01:55 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Edwin Török 
  Xenia Ragiadakou 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  starved 
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  starved 
 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
   4d6df4ec75..49b1cb2741  49b1cb27413034c81023d1faf7af43690e87291a -> smoke

Re: [PATCH v5 4/5] Build system: Replace git:// and http:// with https://

[Demi Marie Obenour]

On Mon, Feb 27, 2023 at 09:42:24AM +0100, Jan Beulich wrote:
> On 25.02.2023 21:37, Demi Marie Obenour wrote:
> > --- a/stubdom/configure
> > +++ b/stubdom/configure
> > @@ -3545,7 +3545,7 @@ if test "x$LIBPCI_URL" = "x"; then :
> > if test "x$extfiles" = "xy"; then :
> >LIBPCI_URL=\$\(XEN_EXTFILES_URL\)
> >  else
> > -  LIBPCI_URL="http://www.kernel.org/pub/software/utils/pciutils;
> > +  LIBPCI_URL="https://mirrors.edge.kernel.org/pub/software/utils/pciutils;
> >  fi
> 
> Simply replacing https:// in the original URL does work. Why did you alter
> it beyond that? Yes, either access leads to the URL you specify, but that
> forwarding (or however it's implemented) may change down the road (and it
> could, aiui, even be dependent upon where in the world the access is coming
> from). In any event, here and below, any adjustment beyond what the title
> says wants explaining in the description.
> 
> Jan

$ curl --head --fail 
https://www.kernel.org/pub/software/utils/pciutils/pciutils-2.2.9.tar.bz2
HTTP/1.1 301 Moved Permanently
Server: nginx
Date: Mon, 27 Feb 2023 20:46:38 GMT
Content-Type: text/html
Content-Length: 162
Connection: keep-alive
Location: 
https://mirrors.edge.kernel.org/pub/software/utils/pciutils/pciutils-2.2.9.tar.bz2
X-Frame-Options: DENY
X-Content-Type-Options: nosniff
Strict-Transport-Security: max-age=15768001
Referrer-Policy: same-origin
Content-Security-Policy: default-src 'self'; img-src https: data:

This means that all future requests should be made to
https://mirrors.edge.kernel.org/pub/software/utils/pciutils/pciutils-2.2.9.tar.bz2
as per the HTTP standard.  If this were a temporary redirect you would
be correct, but it is not.  See:

> Some URLS returned 301 or 302 redirects, so I replaced them with the
> URLs that were redirected to.

from the commit message.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab


signature.asc
Description: PGP signature

Re: [PATCH 1/2] docs/about: Deprecate 32-bit x86 hosts and qemu-system-i386

[Richard Henderson]

On 2/27/23 01:50, Daniel P. Berrangé wrote:

On Mon, Feb 27, 2023 at 12:10:49PM +0100, Thomas Huth wrote:

Hardly anybody still uses 32-bit x86 hosts today, so we should
start deprecating them to finally have less test efforts.
With regards to 32-bit KVM support in the x86 Linux kernel,
the developers confirmed that they do not need a recent
qemu-system-i386 binary here:

  https://lore.kernel.org/kvm/y%2ffkts5ajfy0h...@google.com/

Signed-off-by: Thomas Huth 
---
  docs/about/deprecated.rst | 13 +
  1 file changed, 13 insertions(+)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 15084f7bea..98517f5187 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -196,6 +196,19 @@ CI coverage support may bitrot away before the deprecation 
process
  completes. The little endian variants of MIPS (both 32 and 64 bit) are
  still a supported host architecture.
  
+32-bit x86 hosts and ``qemu-system-i386`` (since 8.0)

+'
+
+Testing 32-bit x86 host OS support takes a lot of precious time during the
+QEMU contiguous integration tests, and considering that most OS vendors
+stopped shipping 32-bit variants of their x86 OS distributions and most
+x86 hardware from the past >10 years is capable of 64-bit, keeping the
+32-bit support alive is an inadequate burden for the QEMU project. Thus
+QEMU will soon drop the support for 32-bit x86 host systems and the
+``qemu-system-i386`` binary. Use ``qemu-system-x86_64`` (which is a proper
+superset of ``qemu-system-i386``) on a 64-bit host machine instead.


I feel like we should have separate deprecation entries for the
i686 host support, and for qemu-system-i386 emulator binary, as
although they're related they are independant features with
differing impact.


Agreed.


32-bit x86 hosts


Support for 32-bit x86 host deployments is increasingly uncommon in
mainstream Linux distributions given the widespread availability of
64-bit x86 hardware. The QEMU project no longer considers 32-bit
x86 support to be an effective use of its limited resources, and
thus intends to discontinue it.

Current users of QEMU on 32-bit x86 hosts should either continue
using existing releases of QEMU, with the caveat that they will
no longer get security fixes, or migrate to a 64-bit platform
which remains capable of running 32-bit guests if needed.

Ack.



``qemu-system-i386`` binary removal
'''

The ``qemu-system-x86_64`` binary can be used to run 32-bit guests
by selecting a 32-bit CPU model, including KVM support on x86_64
hosts. Once support for the 32-bit x86 host platform is discontinued,
the ``qemu-system-i386`` binary will be redundant.


Missing "kvm" in this last sentence?  It is otherwise untrue for tcg.



Current users are
recommended to reconfigure their systems to use the ``qemu-system-x86_64``
binary.


Ack.


Same point for the next patch about 32-bit arm vs qemu-system-arm
binary.


Ack.


r~

Re: [PATCH 1/2] docs/about: Deprecate 32-bit x86 hosts and qemu-system-i386

[Richard Henderson]

On 2/27/23 10:12, Michael S. Tsirkin wrote:

On Mon, Feb 27, 2023 at 11:50:07AM +, Daniel P. Berrangé wrote:

I feel like we should have separate deprecation entries for the
i686 host support, and for qemu-system-i386 emulator binary, as
although they're related they are independant features with
differing impact. eg removing qemu-system-i386 affects all
host architectures, not merely 32-bit x86 host, so I think we
can explain the impact more clearly if we separate them.


Removing qemu-system-i386 seems ok to me - I think qemu-system-x86_64 is
a superset.

Removing support for building on 32 bit systems seems like a pity - it's
one of a small number of ways to run 64 bit binaries on 32 bit systems,
and the maintainance overhead is quite small.


It's not that small.  It only works for single-threaded system mode.  It silently does not 
honor atomicity for user-only mode, which is perhaps worse not working at all.



r~

Re: [PATCH v4 2/3] Build system: Replace git:// and http:// with https://

[Demi Marie Obenour]

On Mon, Feb 27, 2023 at 09:25:32AM +0100, Jan Beulich wrote:
> On 24.02.2023 23:55, Demi Marie Obenour wrote:
> > On Tue, Feb 21, 2023 at 11:07:58AM +0100, Jan Beulich wrote:
> >> On 19.02.2023 03:46, Demi Marie Obenour wrote:
> >>> --- a/stubdom/configure
> >>> +++ b/stubdom/configure
> >>> @@ -3535,7 +3535,7 @@ if test "x$ZLIB_URL" = "x"; then :
> >>>   if test "x$extfiles" = "xy"; then :
> >>>ZLIB_URL=\$\(XEN_EXTFILES_URL\)
> >>>  else
> >>> -  ZLIB_URL="http://www.zlib.net;
> >>> +  ZLIB_URL="https://www.zlib.net;
> >>>  fi
> >>
> >> In v3 you said that this URL can't be used anymore for the version we're
> >> trying to fetch (which I can confirm). Leaving aside the question of why
> >> stubdom was never updated in that regard, what use is it to update URL
> >> (without even mentioning the aspect in the description) in such a case?
> >> (I haven't gone through any of the other URLs again, so there may well
> >> be more similar cases.)
> > 
> > Main advantage is that it will fail securely rather than downloading
> > whatever random code an MITM attacker put in there.
> 
> As said before (and implied here): At the very least you need to mention
> the aspect in the description. But then wouldn't things be failing equally
> securely if no (non-working) URL was put in place, or one which is
> guaranteed to yield an error but makes obvious that no real URL is meant?

https://lists.xenproject.org/archives/html/xen-devel/2023-02/msg01439.html
("[PATCH v5 3/5] Build system: Do not try to use broken links") does
exactly that.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab


signature.asc
Description: PGP signature

Re: [PATCH 1/2] docs/about: Deprecate 32-bit x86 hosts and qemu-system-i386

[Michael S. Tsirkin]

On Mon, Feb 27, 2023 at 11:50:07AM +, Daniel P. Berrangé wrote:
> I feel like we should have separate deprecation entries for the
> i686 host support, and for qemu-system-i386 emulator binary, as
> although they're related they are independant features with
> differing impact. eg removing qemu-system-i386 affects all
> host architectures, not merely 32-bit x86 host, so I think we
> can explain the impact more clearly if we separate them.

Removing qemu-system-i386 seems ok to me - I think qemu-system-x86_64 is
a superset.

Removing support for building on 32 bit systems seems like a pity - it's
one of a small number of ways to run 64 bit binaries on 32 bit systems,
and the maintainance overhead is quite small.

In fact, keeping this support around forces correct use of
posix APIs such as e.g. PRIx64 which makes the code base
more future-proof.

-- 
MST

Re: [PATCH v2] core-parking: fix build with gcc12 and NR_CPUS=1

[Andrew Cooper]

On 27/02/2023 7:43 pm, Andrew Cooper wrote:
> On 09/09/2022 3:30 pm, Jan Beulich wrote:
>> select HAS_ALTERNATIVE
>> select HAS_COMPAT
>> select HAS_CPUFREQ
>> --- a/xen/arch/x86/platform_hypercall.c
>> +++ b/xen/arch/x86/platform_hypercall.c
>> @@ -727,12 +727,17 @@ ret_t do_platform_op(
>>     case XEN_CORE_PARKING_SET:
>>     idle_nums = min_t(uint32_t,
>>     op->u.core_parking.idle_nums, num_present_cpus() -
>> 1);
>> -    ret = continue_hypercall_on_cpu(
>> -    0, core_parking_helper, (void *)(unsigned
>> long)idle_nums);
>> +    if ( CONFIG_NR_CPUS > 1 )
>> +    ret = continue_hypercall_on_cpu(
>> +    0, core_parking_helper,
>> +    (void *)(unsigned long)idle_nums);
>> +    else if ( idle_nums )
>> +    ret = -EINVAL;
>>     break;
>>
>>     case XEN_CORE_PARKING_GET:
>> -    op->u.core_parking.idle_nums = get_cur_idle_nums();
>> +    op->u.core_parking.idle_nums = CONFIG_NR_CPUS > 1
>> +   ? get_cur_idle_nums() : 0;
> These don't look right.
>
> If the core parking feature isn't available, it should uniformly fail,
> not report success on the get side and fail on the set side.

Huh, and in extra fun.

It turns out we've had core parking unconditionally disabled in
XenServer for ~9 years now.

It looks at the idleness of dom0 and starts taking CPUs offline, even
when the VMs are busy.

I don't see how this functionality can ever have worked suitably...  I
think there's a good argument to be made for having it user selectable,
and frankly, off-by-default.

~Andrew

Re: [PATCH v2] core-parking: fix build with gcc12 and NR_CPUS=1

[Andrew Cooper]

On 09/09/2022 3:30 pm, Jan Beulich wrote:
> Gcc12 takes issue with core_parking_remove()'s
>
>    for ( ; i < cur_idle_nums; ++i )
>    core_parking_cpunum[i] = core_parking_cpunum[i + 1];
>
> complaining that the right hand side array access is past the bounds of
> 1. Clearly the compiler can't know that cur_idle_nums can only ever be
> zero in this case (as the sole CPU cannot be parked).
>
> Arrange for core_parking.c's contents to not be needed altogether, and
> then disable its building when NR_CPUS == 1.
>
> Signed-off-by: Jan Beulich 
> ---
> v2: Disable building of core_parking.c altogether.
>
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -10,7 +10,7 @@ config X86
> select ALTERNATIVE_CALL
> select ARCH_MAP_DOMAIN_PAGE
> select ARCH_SUPPORTS_INT128
> -    select CORE_PARKING
> +    select CORE_PARKING if NR_CPUS > 1

The appropriate change is:

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 6a7825f4ba3c..2a5c3304e2b0 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -10,7 +10,7 @@ config X86
    select ALTERNATIVE_CALL
    select ARCH_MAP_DOMAIN_PAGE
    select ARCH_SUPPORTS_INT128
-   select CORE_PARKING
+   imply CORE_PARKING
    select HAS_ALTERNATIVE
    select HAS_COMPAT
    select HAS_CPUFREQ
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index f1ea3199c8eb..855c843113e3 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -10,6 +10,7 @@ config COMPAT
 
 config CORE_PARKING
    bool
+   depends on NR_CPUS > 1
 
 config GRANT_TABLE
    bool "Grant table support" if EXPERT


The core parking code really does malfunction with NR_CPUS == 1, so
really does need a hard dependency.

It turns out our version of Kbuild does understand the imply keyword,
which is the right way to spell "I want this feature unless something
else happens to rule it out".

As noted in the kbuild docs, select should only be used for immutable
properties (this arch has $X), whereas imply should be used for all "I
want $Y".  Most places we use select ought to use imply instead.



> select HAS_ALTERNATIVE
> select HAS_COMPAT
> select HAS_CPUFREQ
> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -727,12 +727,17 @@ ret_t do_platform_op(
>     case XEN_CORE_PARKING_SET:
>     idle_nums = min_t(uint32_t,
>     op->u.core_parking.idle_nums, num_present_cpus() -
> 1);
> -    ret = continue_hypercall_on_cpu(
> -    0, core_parking_helper, (void *)(unsigned
> long)idle_nums);
> +    if ( CONFIG_NR_CPUS > 1 )
> +    ret = continue_hypercall_on_cpu(
> +    0, core_parking_helper,
> +    (void *)(unsigned long)idle_nums);
> +    else if ( idle_nums )
> +    ret = -EINVAL;
>     break;
>
>     case XEN_CORE_PARKING_GET:
> -    op->u.core_parking.idle_nums = get_cur_idle_nums();
> +    op->u.core_parking.idle_nums = CONFIG_NR_CPUS > 1
> +   ? get_cur_idle_nums() : 0;

These don't look right.

If the core parking feature isn't available, it should uniformly fail,
not report success on the get side and fail on the set side.

>     ret = __copy_field_to_guest(u_xenpf_op, op, u.core_parking) ?
>   -EFAULT : 0;
>     break;
> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -157,7 +157,7 @@ long arch_do_sysctl(
>     long (*fn)(void *);
>     void *hcpu;
>
> -    switch ( op )
> +    switch ( op | -(CONFIG_NR_CPUS == 1) )

Extending what Julien has said...

We use this pattern exactly twice, and I would probably ack a patch
disallowing it in the coding style.

Its a trick that is too clever for its own good.  To anyone who hasn't
encountered it, its straight obfuscation, and even I, who knows how the
trick works, still has to reverse engineer the expression every single
time to figure out which way it ends up resolving.

~Andrew

Re: [PATCH v5 2/5] Change remaining xenbits.xen.org links to HTTPS

[Demi Marie Obenour]

On Mon, Feb 27, 2023 at 09:35:51AM +0100, Jan Beulich wrote:
> On 25.02.2023 21:37, Demi Marie Obenour wrote:
> > --- a/Config.mk
> > +++ b/Config.mk
> > @@ -191,7 +191,7 @@ APPEND_CFLAGS += $(foreach i, $(APPEND_INCLUDES), 
> > -I$(i))
> >  EMBEDDED_EXTRA_CFLAGS := -fno-pie -fno-stack-protector 
> > -fno-stack-protector-all
> >  EMBEDDED_EXTRA_CFLAGS += -fno-exceptions -fno-asynchronous-unwind-tables
> >  
> > -XEN_EXTFILES_URL ?= http://xenbits.xen.org/xen-extfiles
> > +XEN_EXTFILES_URL ?= https://xenbits.xen.org/xen-extfiles
> >  # All the files at that location were downloaded from elsewhere on
> >  # the internet.  The original download URL is preserved as a comment
> >  # near the place in the Xen Makefiles where the file is used.
> > diff --git a/tools/misc/mkrpm b/tools/misc/mkrpm
> > index 
> > 68819b2d739cea5491b53f9b944ee2bd20d92c2b..548db4b5da2691547438df5d7d58e5b4c3bd90d0
> >  100644
> > --- a/tools/misc/mkrpm
> > +++ b/tools/misc/mkrpm
> > @@ -34,7 +34,7 @@ Version: $version
> >  Release: $release
> >  License: GPL
> >  Group:   System/Hypervisor
> > -URL: http://xenbits.xenproject.org/xen.git
> > +URL: https://xenbits.xen.org/git-http/xen.git
> 
> Please can you not lose "project" from the URL? That's the more modern
> form, after all. In fact, since you're touching the other URL above
> anyway, I wonder if it wouldn't be a good idea to insert "project"
> there as well. With at least the former adjustment (which I suppose
> can be done while committing, as long as you agree)
> Acked-by: Jan Beulich 

I’m fine with either or both of those adjustments.  I was not aware that
https://xenbits.xen.org is an alias for https://xenbits.xenproject.org.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab


signature.asc
Description: PGP signature

Re: [PATCH 0/2] Deprecate support for 32-bit x86 and arm hosts

[Daniel P . Berrangé]

On Mon, Feb 27, 2023 at 12:10:48PM +0100, Thomas Huth wrote:
> We're struggling quite badly with our CI minutes on the shared
> gitlab runners, so we urgently need to think of ways to cut down
> our supported build and target environments. qemu-system-i386 and
> qemu-system-arm are not really required anymore, since nobody uses
> KVM on the corresponding systems for production anymore, and the
> -x86_64 and -arch64 variants are a proper superset of those binaries.
> So it's time to deprecate them and the corresponding 32-bit host
> environments now.
> 
> This is a follow-up patch series from the previous discussion here:
> 
>  https://lore.kernel.org/qemu-devel/20230130114428.1297295-1-th...@redhat.com/
> 
> where people still mentioned that there is still interest in certain
> support for 32-bit host hardware. But as far as I could see, there is
> no real need for 32-bit host support for system emulation on x86 and
> arm anymore, so it should be fine if we drop these host environments
> now (these are also the two architectures that contribute the most to
> the long test times in our CI, so we would benefit a lot by dropping
> those).

Your description here is a little ambiguous about what's being
proposed. When you say dropping 32-bit host support do you mean
just for the system emulator binaries, or for QEMU entirely ?

And when the deprecation period is passed, are you proposing
to actively prevent 32-bit builds, or merely stopping CI testing
and leave 32-bit builds still working if people want them ?


With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v1 2/3] xen/riscv: setup initial pagetables

[Julien Grall]

Hi,

On 27/02/2023 17:17, Oleksii wrote:

On Sat, 2023-02-25 at 18:05 +, Julien Grall wrote:

Hi,

On 24/02/2023 15:06, Oleksii Kurochko wrote:

Calculate load and linker linker image addresses and
setup initial pagetables.

Signed-off-by: Oleksii Kurochko 
---
   xen/arch/riscv/setup.c | 11 +++
   1 file changed, 11 insertions(+)

diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index b7cd438a1d..f69bc278bb 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -1,9 +1,11 @@
   #include 
   #include 
   #include 
+#include 
   
   #include 

   #include 
+#include 
   #include 
   
   /* Xen stack for bringing up the first CPU. */

@@ -43,6 +45,11 @@ static void __init disable_fpu(void)
   
   void __init noreturn start_xen(void)

   {
+    unsigned long load_start    = (unsigned long)start;
+    unsigned long load_end  = load_start + (unsigned
long)(_end - _start);


I am a bit puzzled, on top of load_addr() and linker_addr(), you
wrote
it can't use global variable/function. But here... you are using
them.
So how is this different?

I don't use load_addr() and linker_addr() macros here.


I understand that. But my comment was related to:

/*
 * WARNING: load_addr() and linker_addr() are to be called only when 
the MMU is
 * disabled and only when executed by the primary CPU.  They cannot 
refer to

 * any global variable or functions.
 */

_start and _end are global variables. So why can you use them here but 
not there?


If you could use them in load_addr() then you could simplify a lot your 
logic.





+    unsigned long linker_start  = (unsigned long)_start;
+    unsigned long linker_end    = (unsigned long)_end;


I am a bit confused with how you define the start/end for both the
linker and load. In one you use _start and the other _end.

Both are fixed at compile time, so I assume the values will be a
linked
address rather than the load address. So how is this meant to how?


_start, _end - it is label from linker script so I use them to define
linker_start and linker_end addresses.

load_start is defined as an address of start() function from head.S and
load_end is the load_start + the size  (_end - _start)


I think you misunderstood my comment. I understand what the variables 
are for. But I don't understand the computation because Xen could be 
loaded at a different address than the runtime address.





Furthermore, I would expect linker_start and load_start to point to
the
same symbol (the only different is one store the virtual address
whereas
the other the physical address). But here you are technically using
two
different symbol. Can you explain why?

It is used to make identity mapping for the range [load_addr, load_end]
and [linker_addr, linker_end]. It was done so because in Bobby's
patches in the linker script XEN_VIRT_START is defined as
_AT(vaddr_t,0x0020) but bootloader loads Xen at 0x8020 and so
in this case loadr_addr != linker_addr.
But I have changed XEN_VIRT_START to 0x8020...00 so they are equal now.


So this will be broken as soon as this code will be tested on an 
hardware where there is no RAM at 0x8020...00. I would strongly 
recommend for you to test your code with XEN_VIRT_START != load address.





+
   /*
    * The following things are passed by bootloader:
    *   a0 -> hart_id
@@ -65,6 +72,10 @@ void __init noreturn start_xen(void)
   
   test_macros_from_bug_h();
   
+    setup_initial_pagetables(load_start, load_end, linker_start,

linker_end);


Shouldn't this happen earlier in start_xen()?

It can. If to be honest I don't know if it should. I added at the end
only because it was the last thing I worked on...


I think we should enable the MMU and switch to the runtime mapping as 
early as possible.


Cheers,

--
Julien Grall

Re: [PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Julien Grall]

Hi Oleksii,

On 27/02/2023 16:52, Oleksii wrote:

On Sat, 2023-02-25 at 17:53 +, Julien Grall wrote:

+/*
+ * WARNING: load_addr() and linker_addr() are to be called only
when the MMU is
+ * disabled and only when executed by the primary CPU.  They
cannot refer to
+ * any global variable or functions.


I find interesting you are saying when _setup_initial_pagetables() is
called from setup_initial_pagetables(). Would you be able to explain
how
this is different?

I am not sure that I understand your question correctly but
_setup_initial_pagetables() was introduced to map some addresses with
write/read flag. Probably I have to rename it to something that is more
clear.


So the comment suggests that you code cannot refer to global 
functions/variables when the MMU is off. So I have multiple questions:

  * Why only global? IOW, why static would be OK?
  * setup_initial_pagetables() has a call to 
_setup_initial_pagetables() (IOW referring to another function). Why is 
it fine?
  * You have code in the next patch referring to global variables 
(mainly _start and _end). How is this different?





+ */
+
+/*
+ * Convert an addressed layed out at link time to the address
where it was loaded


Typo: s/addressed/address/ ?

Yes, it should be address. and 'layed out' should be changed to 'laid
out'...



+ * by the bootloader.
+ */


Looking at the implementation, you seem to consider that any address
not
in the range [linker_addr_start, linker_addr_end[ will have a 1:1
mappings.

I am not sure this is what you want. So I would consider to throw an
error if such address is passed.

I thought that at this stage and if no relocation was done it is 1:1
except the case when load_addr_start != linker_addr_start.


The problem is what you try to map one to one may clash with the linked 
region for Xen. So it is not always possible to map the region 1:1.


Therefore, I don't see any use for the else part here.







+#define
load_addr(linker_address)
     \
+
({
     \
+    unsigned long __linker_address = (unsigned
long)(linker_address);  \
+    if ( linker_addr_start <= __linker_address
&&  \
+    __linker_address < linker_addr_end
)   \
+
{
     \
+    __linker_address
= \
+    __linker_address - linker_addr_start +
load_addr_start;    \
+
}
     \
+
__linker_address;
     \
+    })
+
+/* Convert boot-time Xen address from where it was loaded by the
boot loader to the address it was layed out
+ * at link-time.
+ */


Coding style: The first line is too long and multi-line comments look
like:

/*
   * Foo
   * Bar
   */


+#define
linker_addr(load_address)
     \


Same remark as for load_addr() above.


+
({
     \
+    unsigned long __load_address = (unsigned
long)(load_address);  \
+    if ( load_addr_start <= __load_address
&&  \
+    __load_address < load_addr_end
)   \
+
{
     \
+    __load_address
=   \
+    __load_address - load_addr_start +
linker_addr_start;  \
+
}
     \
+
__load_address;
     \
+    })
+
+static void __attribute__((section(".entry")))
+_setup_initial_pagetables(pte_t *second, pte_t *first, pte_t
*zeroeth,

Can this be named to setup_initial_mapping() so this is clearer and
avoid the one '_' different with the function below.

Sure. It will be better.



+ unsigned long map_start,
+ unsigned long map_end,
+ unsigned long pa_start,
+ bool writable)


What about the executable bit?

It's always executable... But as you mentioned above PTE_LEAF_DEFAULT
should be either RX or RW.
I think it makes sense to add flags instead of writable.



+{
+    unsigned long page_addr;
+    unsigned long index2;
+    unsigned long index1;
+    unsigned long index0;


index* could be defined in the loop below.

It could. But I am curious why it is better?



+
+    /* align start addresses */
+    map_start &= ZEROETH_MAP_MASK;
+    pa_start &= ZEROETH_MAP_MASK;


Hmmm... I would actually expect the address to be properly aligned
and
therefore not require an alignment here.

Otherwise, this raise the question of what happen if you have region
using the same page?

That map_start &=  ZEROETH_MAP_MASK is needed to page number of address
w/o page offset.


My point is why would the page offset be non-zero?




+
+    page_addr = map_start;
+    while ( page_addr < map_end )


Looking at the loop, it looks like you are assuming that the region
will
never cross a boundary of a page-table (either L0, L1, L2). I am not
convinced you can make such assumption (see below).

But if you really want to make such assumption then you should add
some
guard (either BUILD_BUG_ON(), ASSERT(), proper check) in your code 

Re: [PATCH v1 2/3] xen/riscv: setup initial pagetables

[Oleksii]

On Sat, 2023-02-25 at 18:05 +, Julien Grall wrote:
> Hi,
> 
> On 24/02/2023 15:06, Oleksii Kurochko wrote:
> > Calculate load and linker linker image addresses and
> > setup initial pagetables.
> > 
> > Signed-off-by: Oleksii Kurochko 
> > ---
> >   xen/arch/riscv/setup.c | 11 +++
> >   1 file changed, 11 insertions(+)
> > 
> > diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> > index b7cd438a1d..f69bc278bb 100644
> > --- a/xen/arch/riscv/setup.c
> > +++ b/xen/arch/riscv/setup.c
> > @@ -1,9 +1,11 @@
> >   #include 
> >   #include 
> >   #include 
> > +#include 
> >   
> >   #include 
> >   #include 
> > +#include 
> >   #include 
> >   
> >   /* Xen stack for bringing up the first CPU. */
> > @@ -43,6 +45,11 @@ static void __init disable_fpu(void)
> >   
> >   void __init noreturn start_xen(void)
> >   {
> > +    unsigned long load_start    = (unsigned long)start;
> > +    unsigned long load_end  = load_start + (unsigned
> > long)(_end - _start);
> 
> I am a bit puzzled, on top of load_addr() and linker_addr(), you
> wrote 
> it can't use global variable/function. But here... you are using
> them. 
> So how is this different?
I don't use load_addr() and linker_addr() macros here.
> 
> > +    unsigned long linker_start  = (unsigned long)_start;
> > +    unsigned long linker_end    = (unsigned long)_end;
> 
> I am a bit confused with how you define the start/end for both the 
> linker and load. In one you use _start and the other _end.
> 
> Both are fixed at compile time, so I assume the values will be a
> linked 
> address rather than the load address. So how is this meant to how?
> 
_start, _end - it is label from linker script so I use them to define
linker_start and linker_end addresses.

load_start is defined as an address of start() function from head.S and
load_end is the load_start + the size  (_end - _start)

> Furthermore, I would expect linker_start and load_start to point to
> the 
> same symbol (the only different is one store the virtual address
> whereas 
> the other the physical address). But here you are technically using
> two 
> different symbol. Can you explain why?
It is used to make identity mapping for the range [load_addr, load_end]
and [linker_addr, linker_end]. It was done so because in Bobby's
patches in the linker script XEN_VIRT_START is defined as
_AT(vaddr_t,0x0020) but bootloader loads Xen at 0x8020 and so
in this case loadr_addr != linker_addr.
But I have changed XEN_VIRT_START to 0x8020...00 so they are equal now.
> 
> > +
> >   /*
> >    * The following things are passed by bootloader:
> >    *   a0 -> hart_id
> > @@ -65,6 +72,10 @@ void __init noreturn start_xen(void)
> >   
> >   test_macros_from_bug_h();
> >   
> > +    setup_initial_pagetables(load_start, load_end, linker_start,
> > linker_end);
> 
> Shouldn't this happen earlier in start_xen()?
It can. If to be honest I don't know if it should. I added at the end
only because it was the last thing I worked on...
> 
> Cheers,
> 
~ Oleksii

Re: [PATCH] IOMMU/VT-d: Fix iommu=no-igfx if the IOMMU scope contains phantom device

[Jan Beulich]

On 26.02.2023 01:08, Marek Marczykowski-Górecki wrote:
> If the scope for IGD's IOMMU contains additional device that doesn't
> actually exist, iommu=no-igfx would not disable that IOMMU. In this
> particular case (Thinkpad x230) it included
> 00:02.1, but there is no such device on this platform.
> Consider only existing devices for "gfx only" check.
> 

Hmm, perhaps

Fixes: 2d7f191b392e ('VT-d: generalize and correct "iommu=no-igfx" handling')

?

> Signed-off-by: Marek Marczykowski-Górecki 
> ---
> I have looked at existence check acpi_parse_one_drhd(), but re-using
> that one wouldn't work for two reasons:
>  - gfx_only logic is very much tied to acpi_parse_dev_scope()

I think this one could be dealt with, but ...

>  - pci_device_detect() in acpi_parse_one_drhd() is skipped in case of
>(implicit or explicit) iommu=force

... I agree this is a good reason to put the check in acpi_parse_dev_scope().

> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -396,6 +396,7 @@ static int __init acpi_parse_dev_scope(
>  igd_drhd_address = drhd->address;
>  
>  if ( gfx_only &&
> + pci_device_detect(seg, bus, path->dev, path->fn) &&
>   pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
>  PCI_CLASS_DEVICE + 1) != 0x03
>  /* PCI_BASE_CLASS_DISPLAY */ )

If we're adding an existence check, then maybe better in the surrounding
if(): Setting igd_drhd_address when there's not really a device at the
designated address isn't very sensible either. (In fact I think I'm going
to alter the inner part of that if() again as well.)

Jan

Re: [PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Oleksii]

On Sat, 2023-02-25 at 17:53 +, Julien Grall wrote:
> Hi Oleksii,
> 
> On 24/02/2023 15:06, Oleksii Kurochko wrote:
> > Mostly the code for setup_initial_pages was taken from Bobby's
> > repo except for the following changes:
> > * Use only a minimal part of the code enough to enable MMU
> > * rename {_}setup_initial_pagetables functions
> > * add writable argument for _setup_initial_pagetables to have
> >    an opportunity to make some sections read-only
> > * update setup_initial_pagetables function to make some sections
> >    read-only
> > * change the order of _setup_inital_pagetables()
> >    in setup_initial_pagetable():
> >    * first it is called for text, init, rodata sections
> >    * after call it for ranges [link_addr_start : link_addr_end] and
> >  [load_addr_start : load_addr_end]
> >    Before it was done first for the ranges and after for sections
> > but
> >    in that case read-only status will be equal to 'true' and
> >    as sections' addresses  can/are inside the ranges the read-only
> > status
> >    won't be updated for them as it was set up before.
> > 
> > Origin:
> > https://gitlab.com/xen-on-risc-v/xen/-/tree/riscv-rebase 4af165b468
> > af
> > Signed-off-by: Oleksii Kurochko 
> > ---
> >   xen/arch/riscv/Makefile   |   1 +
> >   xen/arch/riscv/include/asm/mm.h   |   9 ++
> >   xen/arch/riscv/include/asm/page.h |  90 
> >   xen/arch/riscv/mm.c   | 223
> > ++
> >   4 files changed, 323 insertions(+)
> >   create mode 100644 xen/arch/riscv/include/asm/mm.h
> >   create mode 100644 xen/arch/riscv/include/asm/page.h
> >   create mode 100644 xen/arch/riscv/mm.c
> > 
> > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> > index 443f6bf15f..956ceb02df 100644
> > --- a/xen/arch/riscv/Makefile
> > +++ b/xen/arch/riscv/Makefile
> > @@ -1,5 +1,6 @@
> >   obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
> >   obj-y += entry.o
> > +obj-y += mm.o
> >   obj-$(CONFIG_RISCV_64) += riscv64/
> >   obj-y += sbi.o
> >   obj-y += setup.o
> > diff --git a/xen/arch/riscv/include/asm/mm.h
> > b/xen/arch/riscv/include/asm/mm.h
> > new file mode 100644
> > index 00..fc1866b1d8
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/mm.h
> > @@ -0,0 +1,9 @@
> > +#ifndef _ASM_RISCV_MM_H
> > +#define _ASM_RISCV_MM_H
> > +
> > +void setup_initial_pagetables(unsigned long load_addr_start,
> > +  unsigned long load_addr_end,
> > +  unsigned long linker_addr_start,
> > +  unsigned long linker_addr_end);
> > +
> > +#endif /* _ASM_RISCV_MM_H */
> > diff --git a/xen/arch/riscv/include/asm/page.h
> > b/xen/arch/riscv/include/asm/page.h
> > new file mode 100644
> > index 00..fabbe1305f
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/page.h
> > @@ -0,0 +1,90 @@
> > +#ifndef _ASM_RISCV_PAGE_H
> > +#define _ASM_RISCV_PAGE_H
> > +
> > +#include 
> > +#include 
> > +
> > +#define PAGE_ENTRIES    512
> 
> NIT: AFAIU, the number here is based on ...
> 
> > +#define VPN_BITS    (9)
> 
> ... this. So I would suggest to define PAGE_ENTRIES using VPN_BITS.
Sure. It should be defined using VPN_BITS. Thanks.
> 
> > +#define VPN_MASK    ((unsigned long)((1 << VPN_BITS) -
> > 1))
> NIT: Use 1UL and you can avoid the cast.
Thanks. I'll update that in the next version of patch series.
> 
> > +
> > +#ifdef CONFIG_RISCV_64
> > +/* L3 index Bit[47:39] */
> > +#define THIRD_SHIFT (39)
> > +#define THIRD_MASK  (VPN_MASK << THIRD_SHIFT)
> > +/* L2 index Bit[38:30] */
> > +#define SECOND_SHIFT    (30)
> > +#define SECOND_MASK (VPN_MASK << SECOND_SHIFT)
> > +/* L1 index Bit[29:21] */
> > +#define FIRST_SHIFT (21)
> > +#define FIRST_MASK  (VPN_MASK << FIRST_SHIFT)
> > +/* L0 index Bit[20:12] */
> > +#define ZEROETH_SHIFT   (12)
> > +#define ZEROETH_MASK    (VPN_MASK << ZEROETH_SHIFT)
> 
> On Arm, we are trying to phase out ZEROETH_* and co because the name
> is 
> too generic. Instead, we now introduce a generic macro that take a
> level 
> and then compute the mask/shift (see XEN_PT_LEVEL_*).
> 
> You should be able to do in RISC-V and reduce the amount of defines 
> introduced.
Thanks. I'll look at XEN_PT_LEVEL_*. I'll re-read Andrew's comment but
as far as I understand after quick reading we can remove mostly that.
> 
> > +
> > +#else // CONFIG_RISCV_32
> 
> Coding style: comments in Xen are using /* ... */
> 
> > +
> > +/* L1 index Bit[31:22] */
> > +#define FIRST_SHIFT (22)
> > +#define FIRST_MASK  (VPN_MASK << FIRST_SHIFT)
> > +
> > +/* L0 index Bit[21:12] */
> > +#define ZEROETH_SHIFT   (12)
> > +#define ZEROETH_MASK    (VPN_MASK << ZEROETH_SHIFT)
> > +#endif
> > +
> > +#define THIRD_SIZE  (1 << THIRD_SHIFT)
> > +#define THIRD_MAP_MASK  (~(THIRD_SIZE - 1))
> > +#define SECOND_SIZE   

Re: [PATCH v3 10/14] x86/vmx: move declarations used only by vmx code from vmx.h to private headers

[Andrew Cooper]

On 27/02/2023 4:26 pm, Andrew Cooper wrote:
> On 24/02/2023 6:50 pm, Xenia Ragiadakou wrote:
>> Create two new private headers in arch/x86/hvm/vmx called vmx.h and pi.h.
>> Move all the definitions and declarations that are used solely by vmx code
>> into the private vmx.h, apart from the ones related to posted interrupts that
>> are moved into pi.h.
>>
>> EPT related declarations and definitions stay in asm/hvm/vmx/vmx.h because
>> they are used in arch/x86/mm and drivers/passthrough/vtd.
>>
>> Also, __vmread(), used in arch/x86/cpu, and consequently the opcodes stay in
>> asm/hvm/vmx/vmx.h.
> Every time I read the vpmu code, I get increasingly sad.
>
> That is dangerously unsafe, and comes with a chance of exploding completely.
>
> That __vmread() is in NMI context, which means `current` isn't safe to
> deference (we might hit in the middle of a context switch), and more
> generally there's no guarantee that the loaded VMCS is the one
> associated with `current` (we might hit in the middle of a remote VMCS
> access).
>
> vpmu is generally not supported, and BTS needs further custom enablement
> because it is only useable with a custom bus analyser.
>
>
> The __vmread() needs deleting - its absolutely not safe to say.

to stay*

>
> I'm tempted to hardwire the return 0, and punt the problem to whomever
> next uses BTS.
>
> Alternatively, MSR_DBGCTL needs wiring into the hvm_get_reg()
> infrastructure, but I'm not convinced this will actually work in either
> of the two problem cases above, hence preferring the previous option.
>
> Thoughts?
>
> ~Andrew

Re: [PATCH v3 10/14] x86/vmx: move declarations used only by vmx code from vmx.h to private headers

[Andrew Cooper]

On 24/02/2023 6:50 pm, Xenia Ragiadakou wrote:
> Create two new private headers in arch/x86/hvm/vmx called vmx.h and pi.h.
> Move all the definitions and declarations that are used solely by vmx code
> into the private vmx.h, apart from the ones related to posted interrupts that
> are moved into pi.h.
>
> EPT related declarations and definitions stay in asm/hvm/vmx/vmx.h because
> they are used in arch/x86/mm and drivers/passthrough/vtd.
>
> Also, __vmread(), used in arch/x86/cpu, and consequently the opcodes stay in
> asm/hvm/vmx/vmx.h.

Every time I read the vpmu code, I get increasingly sad.

That is dangerously unsafe, and comes with a chance of exploding completely.

That __vmread() is in NMI context, which means `current` isn't safe to
deference (we might hit in the middle of a context switch), and more
generally there's no guarantee that the loaded VMCS is the one
associated with `current` (we might hit in the middle of a remote VMCS
access).

vpmu is generally not supported, and BTS needs further custom enablement
because it is only useable with a custom bus analyser.


The __vmread() needs deleting - its absolutely not safe to say.

I'm tempted to hardwire the return 0, and punt the problem to whomever
next uses BTS.

Alternatively, MSR_DBGCTL needs wiring into the hvm_get_reg()
infrastructure, but I'm not convinced this will actually work in either
of the two problem cases above, hence preferring the previous option.

Thoughts?

~Andrew

Re: [PATCH v2 1/4] tools: rename xen-tools/libs.h file to common-macros.h

[Marek Marczykowski-Górecki]

On Mon, Feb 27, 2023 at 04:41:50PM +0100, Juergen Gross wrote:
> In order to better reflect the contents of the header and to make it
> more appropriate to use it for different runtime environments like
> programs, libraries, and firmware, rename the libs.h include file to
> common-macros.h. Additionally ad a comment pointing out the need to be
> self-contained.
> 
> Suggested-by: Andrew Cooper 
> Signed-off-by: Juergen Gross 
> ---
> V2:
> - new patch
> ---

For this (assuming header rename itself will be acked):

>  tools/python/xen/lowlevel/xc/xc.c  |  2 +-

Acked-by: Marek Marczykowski-Górecki 


-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab


signature.asc
Description: PGP signature

Re: [PATCH v5] x86/ucode/AMD: late load the patch on every logical thread

[Jan Beulich]

On 23.02.2023 18:39, Sergey Dyasli wrote:
> Currently late ucode loading is performed only on the first core of CPU
> siblings.  But according to the latest recommendation from AMD, late
> ucode loading should happen on every logical thread/core on AMD CPUs.
> 
> To achieve that, introduce is_cpu_primary() helper which will consider
> every logical cpu as "primary" when running on AMD CPUs.  Also include
> Hygon in the check for future-proofing.
> 
> Signed-off-by: Sergey Dyasli 

Reviewed-by: Jan Beulich 

Re: [PATCH] build: add crypto/ to SUBDIRS

[Jan Beulich]

On 27.02.2023 16:57, Jan Beulich wrote:
> Well, I'm not outright opposed. But I definitely want to hear another
> maintainer's view before deciding.

Oh, and I should have added: If to be taken, the description would need
extending to explain why the simple route is taken here, and why it is
deemed okay to consider crypto for as little or as much as is, in the
eventual final version, going to be covered in the final version (i.e.
in the version as submitted it would want clarifying why it is okay to
include for Arm despite being unused there).

Jan

Re: [PATCH v3 08/14] x86/vmx: move vmx_update_debug_state() in vmcs.c and declare it static

[Andrew Cooper]

On 24/02/2023 6:50 pm, Xenia Ragiadakou wrote:
> Move vmx_update_debug_state() in vmcs.c because it is used only in this
> file and limit its scope to this file by declaring it static and removing
> its declaration from vmx.h.
>
> No functional change intended.
>
> Signed-off-by: Xenia Ragiadakou 

Acked-by: Andrew Cooper 

Re: [PATCH] build: add crypto/ to SUBDIRS

[Jan Beulich]

On 27.02.2023 16:46, Michal Orzel wrote:
> On 27/02/2023 16:00, Jan Beulich wrote:
>> On 27.02.2023 15:46, Michal Orzel wrote:
>>> On 27/02/2023 14:54, Jan Beulich wrote:
 On 27.02.2023 14:41, Michal Orzel wrote:
> On 27/02/2023 11:10, Jan Beulich wrote:
>> On 27.02.2023 10:53, Michal Orzel wrote:
>>> --- a/xen/Makefile
>>> +++ b/xen/Makefile
>>> @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE
>>>   $(Q)$(MAKE) $(build)=. 
>>> arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
>>>   $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 
>>> 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@
>>>
>>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
>>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test
>>>  define all_sources
>>>  ( find include -type f -name '*.h' -print; \
>>>find $(SUBDIRS) -type f -name '*.[chS]' -print )
>>
>> As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo
>> also only be included when selected (or at the very least only when an
>> arch might select it, which afaics is possible on x86 only right now).
>>
>> It would also help if in the description you made explicit that SUBDIRS
>> isn't used for anything else (the name, after all, is pretty generic).
>> Which actually points at an issue: I suppose the variable would actually
>> better be used elsewhere as well, e.g. in the _clean: rule and perhaps
>> also in the setting of ALL_OBJS-y. (That'll require splitting the
>> variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and
>> $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which
>> has caused crypto to be missing from SUBDIRS.
>>
> I think what you wrote can be split into 2 parts: the part being a goal 
> of this patch
> and the cleanup/improvements that would be beneficial but not related to 
> this patch.
> The second part involves more code and there are parts to be discussed:
>
> 1) If we decide to create ALL_OBJS-y from SUBDIRS, then we would need to 
> carve out test/ dir
> that is not part of ALL_OBJS-y and add it to SUBDIRS later on. Also, the 
> order of ALL_OBJS-y matters
> for linking, so we would need to transfer the responsibility to SUBDIRS. 
> The natural placement of
> SUBDIRS (including SUBDIRS-y, etc.) would be right above ALL_OBJS-y. 
> However, when doing clean (next point),
> need-config is set to n and SUBDIRS would be empty. This means it would 
> need to be defined somewhere at the
> top of the Makefile (thus harder to make sure the linking order is 
> correct).
>
> 2) If we deicide to use SUBDIRS for _clean rule, then we would need some 
> foreach loop, right?
> Apart from that, there are other directories that are not part of SUBDIRS 
> i.e. include/, tools/.
> Together with SUBDIRS variable, it would create confusion (these dirs are 
> also sub-directories, so why
> not having them listed in this variable?). Also, I can see that we do 
> clean not only for $(TARGET_ARCH)
> but for all the existing architectures.

 I understand that the changes would be more involved, but I disagree with
 your "two parts" statement: If what I've outlined was already the case,
 your patch would not even exist (because crypto/ would already be taken
 care of wherever needed).

> I would prefer to stick for now to the goal of this patch which is to add 
> crypto/ so that it is taken
> into account for the tags/csope generation. Would the following change be 
> ok for that purpose?
>
> diff --git a/xen/Makefile b/xen/Makefile
> index 2d55bb9401f4..05bf301bd7ab 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -589,7 +589,9 @@ $(TARGET): outputmakefile FORCE
>   $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
>   $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 
> 'ALL_LIBS=$(ALL_LIBS-y)' $@
>
> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
> +SUBDIRS-$(CONFIG_CRYPTO) += crypto
> +SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test $(SUBDIRS-y)
> +
>  define all_sources
>  ( find include -type f -name '*.h' -print; \
>find $(SUBDIRS) -type f -name '*.[chS]' -print )

 The fact that, afaict, this won't do is related to the outline I gave.
 You've referred yourself to need-config. Most if not all of the goals
 SUBDIRS is (currently) relevant for are listed in no-dot-config-targets.
 Hence your change above is effectively a no-op, because CONFIG_CRYPTO
 will simply be unset in the common case. Therefore if an easy change is
 wanted, the original version of it would be it. An intermediate
 approach might be to add crypto to SUBDIRS only when 

Re: [PATCH v2 4/4] tools: add offsetof() to xen-tools/common-macros.h

[Jan Beulich]

On 27.02.2023 16:41, Juergen Gross wrote:
> --- a/tools/firmware/include/stddef.h
> +++ b/tools/firmware/include/stddef.h
> @@ -1,10 +1,10 @@
>  #ifndef _STDDEF_H_
>  #define _STDDEF_H_
>  
> +#include 
> +
>  typedef __SIZE_TYPE__ size_t;
>  
>  #define NULL ((void*)0)
>  
> -#define offsetof(t, m) __builtin_offsetof(t, m)
> -
>  #endif

The C standard is pretty specific about what a header of this name
may or (in particular here) may not define. You add much more to the
name space than just the replacement offsetof(). If this was a
header used by an individual component, this might be fine. But this
header is meant to serve all components under firmware/ which care
to include it. At present that's hvmloader (which we control, so we
can arrange for it to be free of collisions) and rombios (which we
do not really control, and which people also may not build routinely
anymore).

Jan

Re: [PATCH v2 1/4] tools: rename xen-tools/libs.h file to common-macros.h

[Jan Beulich]

On 27.02.2023 16:41, Juergen Gross wrote:
> --- a/tools/include/xen-tools/libs.h
> +++ b/tools/include/xen-tools/common-macros.h
> @@ -1,5 +1,13 @@
> -#ifndef __XEN_TOOLS_LIBS__
> -#define __XEN_TOOLS_LIBS__
> +#ifndef __XEN_TOOLS_COMMON_MACROS__
> +#define __XEN_TOOLS_COMMON_MACROS__
> +
> +/*
> + * Caution:
> + *
> + * This header must be completely self-contained. There are no external
> + * references to variables or functions allowed, as the file might be 
> included
> + * for different runtime environments, such as firmware or normal programs.
> + */

May I ask to go a tiny step further: s/normal/target and build host/ or
something alike?

Jan

Re: [PATCH] build: add crypto/ to SUBDIRS

[Michal Orzel]

On 27/02/2023 16:00, Jan Beulich wrote:
> 
> 
> On 27.02.2023 15:46, Michal Orzel wrote:
>> On 27/02/2023 14:54, Jan Beulich wrote:
>>> On 27.02.2023 14:41, Michal Orzel wrote:
 On 27/02/2023 11:10, Jan Beulich wrote:
> On 27.02.2023 10:53, Michal Orzel wrote:
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE
>>   $(Q)$(MAKE) $(build)=. 
>> arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
>>   $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 
>> 'ALL_LIBS=$(ALL_LIBS-y)' $@
>>
>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test
>>  define all_sources
>>  ( find include -type f -name '*.h' -print; \
>>find $(SUBDIRS) -type f -name '*.[chS]' -print )
>
> As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo
> also only be included when selected (or at the very least only when an
> arch might select it, which afaics is possible on x86 only right now).
>
> It would also help if in the description you made explicit that SUBDIRS
> isn't used for anything else (the name, after all, is pretty generic).
> Which actually points at an issue: I suppose the variable would actually
> better be used elsewhere as well, e.g. in the _clean: rule and perhaps
> also in the setting of ALL_OBJS-y. (That'll require splitting the
> variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and
> $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which
> has caused crypto to be missing from SUBDIRS.
>
 I think what you wrote can be split into 2 parts: the part being a goal of 
 this patch
 and the cleanup/improvements that would be beneficial but not related to 
 this patch.
 The second part involves more code and there are parts to be discussed:

 1) If we decide to create ALL_OBJS-y from SUBDIRS, then we would need to 
 carve out test/ dir
 that is not part of ALL_OBJS-y and add it to SUBDIRS later on. Also, the 
 order of ALL_OBJS-y matters
 for linking, so we would need to transfer the responsibility to SUBDIRS. 
 The natural placement of
 SUBDIRS (including SUBDIRS-y, etc.) would be right above ALL_OBJS-y. 
 However, when doing clean (next point),
 need-config is set to n and SUBDIRS would be empty. This means it would 
 need to be defined somewhere at the
 top of the Makefile (thus harder to make sure the linking order is 
 correct).

 2) If we deicide to use SUBDIRS for _clean rule, then we would need some 
 foreach loop, right?
 Apart from that, there are other directories that are not part of SUBDIRS 
 i.e. include/, tools/.
 Together with SUBDIRS variable, it would create confusion (these dirs are 
 also sub-directories, so why
 not having them listed in this variable?). Also, I can see that we do 
 clean not only for $(TARGET_ARCH)
 but for all the existing architectures.
>>>
>>> I understand that the changes would be more involved, but I disagree with
>>> your "two parts" statement: If what I've outlined was already the case,
>>> your patch would not even exist (because crypto/ would already be taken
>>> care of wherever needed).
>>>
 I would prefer to stick for now to the goal of this patch which is to add 
 crypto/ so that it is taken
 into account for the tags/csope generation. Would the following change be 
 ok for that purpose?

 diff --git a/xen/Makefile b/xen/Makefile
 index 2d55bb9401f4..05bf301bd7ab 100644
 --- a/xen/Makefile
 +++ b/xen/Makefile
 @@ -589,7 +589,9 @@ $(TARGET): outputmakefile FORCE
   $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
   $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 
 'ALL_LIBS=$(ALL_LIBS-y)' $@

 -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
 +SUBDIRS-$(CONFIG_CRYPTO) += crypto
 +SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test $(SUBDIRS-y)
 +
  define all_sources
  ( find include -type f -name '*.h' -print; \
find $(SUBDIRS) -type f -name '*.[chS]' -print )
>>>
>>> The fact that, afaict, this won't do is related to the outline I gave.
>>> You've referred yourself to need-config. Most if not all of the goals
>>> SUBDIRS is (currently) relevant for are listed in no-dot-config-targets.
>>> Hence your change above is effectively a no-op, because CONFIG_CRYPTO
>>> will simply be unset in the common case. Therefore if an easy change is
>>> wanted, the original version of it would be it. An intermediate
>>> approach might be to add crypto to SUBDIRS only when TARGET_ARCH=x86.
>>> But neither would preclude the same issue from being introduced again,
>>> when a new subdir appears.
>> I understand 

[PATCH v2 3/4] tools: get rid of additional min() and max() definitions

[Juergen Gross]

Defining min(), min_t(), max() and max_t() at other places than
xen-tools/common-macros.h isn't needed, as the definitions in said
header can be used instead.

Same applies to BUILD_BUG_ON() in hvmloader.

Signed-off-by: Juergen Gross 
---
 tools/firmware/hvmloader/util.h |  8 ++--
 tools/libs/vchan/init.c |  3 +--
 tools/tests/vhpet/emul.h| 11 +--
 tools/tests/vpci/emul.h | 16 
 4 files changed, 4 insertions(+), 34 deletions(-)

diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 8d95eab28a..e04990ee97 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -9,6 +9,8 @@
 #include 
 #include "e820.h"
 
+#include 
+
 /* Request un-prefixed values from errno.h. */
 #define XEN_ERRNO(name, value) name = value,
 enum {
@@ -41,12 +43,6 @@ void __assert_failed(const char *assertion, const char 
*file, int line)
 void __bug(const char *file, int line) __attribute__((noreturn));
 #define BUG() __bug(__FILE__, __LINE__)
 #define BUG_ON(p) do { if (p) BUG(); } while (0)
-#define BUILD_BUG_ON(p) ((void)sizeof(char[1 - 2 * !!(p)]))
-
-#define min_t(type,x,y) \
-({ type __x = (x); type __y = (y); __x < __y ? __x: __y; })
-#define max_t(type,x,y) \
-({ type __x = (x); type __y = (y); __x > __y ? __x: __y; })
 
 #define MB(mb) (mb##ULL << 20)
 #define GB(gb) (gb##ULL << 30)
diff --git a/tools/libs/vchan/init.c b/tools/libs/vchan/init.c
index 9195bd3b98..021e1f29e1 100644
--- a/tools/libs/vchan/init.c
+++ b/tools/libs/vchan/init.c
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "vchan.h"
 
@@ -72,8 +73,6 @@
 #define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
 #endif
 
-#define max(a,b) ((a > b) ? a : b)
-
 static int init_gnt_srv(struct libxenvchan *ctrl, int domain)
 {
int pages_left = ctrl->read.order >= PAGE_SHIFT ? 1 << 
(ctrl->read.order - PAGE_SHIFT) : 0;
diff --git a/tools/tests/vhpet/emul.h b/tools/tests/vhpet/emul.h
index 610641ab0c..6af880cd43 100644
--- a/tools/tests/vhpet/emul.h
+++ b/tools/tests/vhpet/emul.h
@@ -114,16 +114,7 @@ enum
 TASKLET_SOFTIRQ,
 NR_COMMON_SOFTIRQS
 };
-/*
- * ..and if you can't take the strict
- * types, you can specify one yourself.
- *
- * Or not use min/max at all, of course.
- */
-#define min_t(type, x, y) \
-({ type __x = (x); type __y = (y); __x < __y ? __x : __y; })
-#define max_t(type, x, y) \
-({ type __x = (x); type __y = (y); __x > __y ? __x : __y; })
+
 #define offsetof(t, m) ((unsigned long )&((t *)0)->m)
 
 struct domain;
diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h
index 7169a2ea02..8c5bcadd5f 100644
--- a/tools/tests/vpci/emul.h
+++ b/tools/tests/vpci/emul.h
@@ -106,22 +106,6 @@ typedef union {
 #define BUG() assert(0)
 #define ASSERT_UNREACHABLE() assert(0)
 
-#define min(x, y) ({\
-const typeof(x) tx = (x);   \
-const typeof(y) ty = (y);   \
-\
-(void) ( == );\
-tx < ty ? tx : ty;  \
-})
-
-#define max(x, y) ({\
-const typeof(x) tx = (x);   \
-const typeof(y) ty = (y);   \
-\
-(void) ( == );\
-tx > ty ? tx : ty;  \
-})
-
 #endif
 
 /*
-- 
2.35.3

[PATCH v2 4/4] tools: add offsetof() to xen-tools/common-macros.h

[Juergen Gross]

Instead of having multiple files defining offsetof(), add the
definition to tools/include/xen-tools/common-macros.h.

Signed-off-by: Juergen Gross 
---
 tools/firmware/hvmloader/util.h | 3 ---
 tools/firmware/include/stddef.h | 4 ++--
 tools/include/xen-tools/common-macros.h | 4 
 tools/libfsimage/Rules.mk   | 2 ++
 tools/libfsimage/xfs/fsys_xfs.c | 4 +---
 tools/libs/vchan/init.c | 4 
 tools/tests/vhpet/emul.h| 2 --
 7 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index e04990ee97..7249773eeb 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -30,9 +30,6 @@ enum {
 #define SEL_DATA32  0x0020
 #define SEL_CODE64  0x0028
 
-#undef offsetof
-#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
-
 #undef NULL
 #define NULL ((void*)0)
 
diff --git a/tools/firmware/include/stddef.h b/tools/firmware/include/stddef.h
index c7f974608a..0afb426a6d 100644
--- a/tools/firmware/include/stddef.h
+++ b/tools/firmware/include/stddef.h
@@ -1,10 +1,10 @@
 #ifndef _STDDEF_H_
 #define _STDDEF_H_
 
+#include 
+
 typedef __SIZE_TYPE__ size_t;
 
 #define NULL ((void*)0)
 
-#define offsetof(t, m) __builtin_offsetof(t, m)
-
 #endif
diff --git a/tools/include/xen-tools/common-macros.h 
b/tools/include/xen-tools/common-macros.h
index 3e73b33c91..15399e9b88 100644
--- a/tools/include/xen-tools/common-macros.h
+++ b/tools/include/xen-tools/common-macros.h
@@ -79,4 +79,8 @@
 typeof( ((type *)0)->member ) *__mptr = (ptr);\
 (type *)( (char *)__mptr - offsetof(type,member) );})
 
+#ifndef offsetof
+#define offsetof(a, b) __builtin_offsetof(a, b)
+#endif
+
 #endif /* __XEN_TOOLS_COMMON_MACROS__ */
diff --git a/tools/libfsimage/Rules.mk b/tools/libfsimage/Rules.mk
index cf37d6cb0d..85674f2379 100644
--- a/tools/libfsimage/Rules.mk
+++ b/tools/libfsimage/Rules.mk
@@ -3,6 +3,8 @@ include $(XEN_ROOT)/tools/libfsimage/common.mk
 FSLIB = fsimage.so
 TARGETS += $(FSLIB)
 
+CFLAGS += $(CFLAGS_xeninclude)
+
 .PHONY: all
 all: $(TARGETS)
 
diff --git a/tools/libfsimage/xfs/fsys_xfs.c b/tools/libfsimage/xfs/fsys_xfs.c
index d735a88e55..f562daaef0 100644
--- a/tools/libfsimage/xfs/fsys_xfs.c
+++ b/tools/libfsimage/xfs/fsys_xfs.c
@@ -19,6 +19,7 @@
 
 #include 
 #include "xfs.h"
+#include 
 
 #define MAX_LINK_COUNT 8
 
@@ -182,9 +183,6 @@ fsb2daddr (xfs_fsblock_t fsbno)
 (xfs_agblock_t)(fsbno & mask32lo(xfs.agblklog)));
 }
 
-#undef offsetof
-#define offsetof(t,m)  ((size_t)&(((t *)0)->m))
-
 static inline int
 btroot_maxrecs (fsi_file_t *ffi)
 {
diff --git a/tools/libs/vchan/init.c b/tools/libs/vchan/init.c
index 021e1f29e1..ad4cc66d2c 100644
--- a/tools/libs/vchan/init.c
+++ b/tools/libs/vchan/init.c
@@ -69,10 +69,6 @@
 #define MAX_RING_SHIFT 20
 #define MAX_RING_SIZE (1 << MAX_RING_SHIFT)
 
-#ifndef offsetof
-#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
-#endif
-
 static int init_gnt_srv(struct libxenvchan *ctrl, int domain)
 {
int pages_left = ctrl->read.order >= PAGE_SHIFT ? 1 << 
(ctrl->read.order - PAGE_SHIFT) : 0;
diff --git a/tools/tests/vhpet/emul.h b/tools/tests/vhpet/emul.h
index 6af880cd43..7b3ee88fb5 100644
--- a/tools/tests/vhpet/emul.h
+++ b/tools/tests/vhpet/emul.h
@@ -115,8 +115,6 @@ enum
 NR_COMMON_SOFTIRQS
 };
 
-#define offsetof(t, m) ((unsigned long )&((t *)0)->m)
-
 struct domain;
 
 struct vcpu
-- 
2.35.3

[PATCH v2 2/4] tools: add container_of() macro to xen-tools/common-macros.h

[Juergen Gross]

Instead of having 4 identical copies of the definition of a
container_of() macro in different tools header files, add that macro
to xen-tools/common-macros.h and use that instead.

Delete the other copies of that macro.

Signed-off-by: Juergen Gross 
---
There is a similar macro CONTAINER_OF() defined in
tools/include/xentoolcore_internal.h, which allows to not only use a
type for the 2nd parameter, but a variable, too.
I'd like to get rid of that macro as well, but there are lots of use
cases especially in libxl. Any thoughts regarding that macro?
I could either:
- don't touch it at all
- enhance container_of() like CONTAINER_OF() and replace all use cases
  of CONTAINER_OF() with container_of()
- replace the few CONTAINER_OF() users outside libxl with container_of()
  and define CONTAINER_OF() in e.g. libxl_internal.h
- replace all CONTAINER_OF() use cases with container_of(), including
  the change from (.., var, ..) to (.., type, ...).

Signed-off-by: Juergen Gross 
---
 tools/include/xen-tools/common-macros.h | 4 
 tools/tests/vhpet/emul.h| 3 ---
 tools/tests/vpci/emul.h | 6 +-
 tools/tests/x86_emulator/x86-emulate.h  | 5 -
 tools/xenstore/list.h   | 6 ++
 5 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/tools/include/xen-tools/common-macros.h 
b/tools/include/xen-tools/common-macros.h
index f39cd96008..3e73b33c91 100644
--- a/tools/include/xen-tools/common-macros.h
+++ b/tools/include/xen-tools/common-macros.h
@@ -75,4 +75,8 @@
 #define __must_check __attribute__((__warn_unused_result__))
 #endif
 
+#define container_of(ptr, type, member) ({\
+typeof( ((type *)0)->member ) *__mptr = (ptr);\
+(type *)( (char *)__mptr - offsetof(type,member) );})
+
 #endif /* __XEN_TOOLS_COMMON_MACROS__ */
diff --git a/tools/tests/vhpet/emul.h b/tools/tests/vhpet/emul.h
index dfeb10f74e..610641ab0c 100644
--- a/tools/tests/vhpet/emul.h
+++ b/tools/tests/vhpet/emul.h
@@ -125,9 +125,6 @@ enum
 #define max_t(type, x, y) \
 ({ type __x = (x); type __y = (y); __x > __y ? __x : __y; })
 #define offsetof(t, m) ((unsigned long )&((t *)0)->m)
-#define container_of(ptr, type, member) ({  \
-typeof( ((type *)0)->member ) *__mptr = (ptr);  \
-(type *)( (char *)__mptr - offsetof(type,member) ); })
 
 struct domain;
 
diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h
index f03e3a56d1..7169a2ea02 100644
--- a/tools/tests/vpci/emul.h
+++ b/tools/tests/vpci/emul.h
@@ -27,11 +27,7 @@
 #include 
 #include 
 
-#define container_of(ptr, type, member) ({  \
-typeof(((type *)0)->member) *mptr = (ptr);  \
-\
-(type *)((char *)mptr - offsetof(type, member));\
-})
+#include 
 
 #define smp_wmb()
 #define prefetch(x) __builtin_prefetch(x)
diff --git a/tools/tests/x86_emulator/x86-emulate.h 
b/tools/tests/x86_emulator/x86-emulate.h
index 46d4e43cea..1af986f78d 100644
--- a/tools/tests/x86_emulator/x86-emulate.h
+++ b/tools/tests/x86_emulator/x86-emulate.h
@@ -56,11 +56,6 @@
 
 #define cf_check /* No Control Flow Integriy checking */
 
-#define container_of(ptr, type, member) ({ \
-typeof(((type *)0)->member) *mptr__ = (ptr);   \
-(type *)((char *)mptr__ - offsetof(type, member)); \
-})
-
 #define AC_(n,t) (n##t)
 #define _AC(n,t) AC_(n,t)
 
diff --git a/tools/xenstore/list.h b/tools/xenstore/list.h
index b17d13e0ec..a464a38b61 100644
--- a/tools/xenstore/list.h
+++ b/tools/xenstore/list.h
@@ -3,6 +3,8 @@
 /* Taken from Linux kernel code, but de-kernelized for userspace. */
 #include 
 
+#include 
+
 #undef LIST_HEAD_INIT
 #undef LIST_HEAD
 #undef INIT_LIST_HEAD
@@ -15,10 +17,6 @@
 #define LIST_POISON1  ((void *) 0x00100100)
 #define LIST_POISON2  ((void *) 0x00200200)
 
-#define container_of(ptr, type, member) ({ \
-typeof( ((type *)0)->member ) *__mptr = (ptr); \
-(type *)( (char *)__mptr - offsetof(type,member) );})
-
 /*
  * Simple doubly linked list implementation.
  *
-- 
2.35.3

[PATCH v2 1/4] tools: rename xen-tools/libs.h file to common-macros.h

[Juergen Gross]

In order to better reflect the contents of the header and to make it
more appropriate to use it for different runtime environments like
programs, libraries, and firmware, rename the libs.h include file to
common-macros.h. Additionally ad a comment pointing out the need to be
self-contained.

Suggested-by: Andrew Cooper 
Signed-off-by: Juergen Gross 
---
V2:
- new patch
---
 tools/console/daemon/io.c  |  2 +-
 tools/fuzz/cpu-policy/afl-policy-fuzzer.c  |  2 +-
 .../include/xen-tools/{libs.h => common-macros.h}  | 14 +++---
 tools/libs/call/buffer.c   |  2 +-
 tools/libs/ctrl/xc_pm.c|  2 +-
 tools/libs/ctrl/xc_private.h   |  2 +-
 tools/libs/foreignmemory/linux.c   |  2 +-
 tools/libs/gnttab/freebsd.c|  2 +-
 tools/libs/gnttab/linux.c  |  2 +-
 tools/libs/guest/xg_core_arm.c |  2 +-
 tools/libs/guest/xg_cpuid_x86.c|  2 +-
 tools/libs/guest/xg_dom_arm.c  |  2 +-
 tools/libs/guest/xg_dom_bzimageloader.c|  2 +-
 tools/libs/guest/xg_dom_x86.c  |  2 +-
 tools/libs/guest/xg_sr_common.c|  2 +-
 tools/libs/light/libxl_internal.h  |  2 +-
 tools/libs/light/libxl_psr.c   |  2 +-
 tools/libs/stat/xenstat_linux.c|  2 +-
 tools/misc/xen-access.c|  2 +-
 tools/misc/xen-cpuid.c |  2 +-
 tools/misc/xen-diag.c  |  2 +-
 tools/misc/xen-hptool.c|  2 +-
 tools/misc/xen-livepatch.c |  2 +-
 tools/misc/xen-mfndump.c   |  2 +-
 tools/misc/xenpm.c |  2 +-
 tools/ocaml/libs/mmap/xenmmap_stubs.c  |  2 +-
 tools/ocaml/libs/xc/xenctrl_stubs.c|  2 +-
 tools/python/xen/lowlevel/xc/xc.c  |  2 +-
 tools/tests/cpu-policy/test-cpu-policy.c   |  2 +-
 tools/tests/paging-mempool/test-paging-mempool.c   |  2 +-
 tools/tests/resource/test-resource.c   |  2 +-
 tools/tests/tsx/test-tsx.c |  2 +-
 tools/tests/vhpet/emul.h   |  2 +-
 tools/tests/x86_emulator/x86-emulate.h |  2 +-
 tools/tests/xenstore/test-xenstore.c   |  2 +-
 tools/xenstore/utils.h |  2 +-
 tools/xentrace/analyze.h   |  2 +-
 tools/xl/xl_cmdtable.c |  2 +-
 xen/lib/x86/private.h  |  2 +-
 39 files changed, 49 insertions(+), 41 deletions(-)
 rename tools/include/xen-tools/{libs.h => common-macros.h} (83%)

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 682c1f4008..6bfe96715b 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -50,7 +50,7 @@
 #include 
 #include 
 #endif
-#include 
+#include 
 
 /* Each 10 bits takes ~ 3 digits, plus one, plus one for nul terminator. */
 #define MAX_STRLEN(x) ((sizeof(x) * CHAR_BIT + CHAR_BIT-1) / 10 * 3 + 2)
diff --git a/tools/fuzz/cpu-policy/afl-policy-fuzzer.c 
b/tools/fuzz/cpu-policy/afl-policy-fuzzer.c
index 79a849a044..7d0f274c6c 100644
--- a/tools/fuzz/cpu-policy/afl-policy-fuzzer.c
+++ b/tools/fuzz/cpu-policy/afl-policy-fuzzer.c
@@ -8,7 +8,7 @@
 
 #include 
 
-#include 
+#include 
 #include 
 #include 
 #include 
diff --git a/tools/include/xen-tools/libs.h 
b/tools/include/xen-tools/common-macros.h
similarity index 83%
rename from tools/include/xen-tools/libs.h
rename to tools/include/xen-tools/common-macros.h
index bafc90e2f6..f39cd96008 100644
--- a/tools/include/xen-tools/libs.h
+++ b/tools/include/xen-tools/common-macros.h
@@ -1,5 +1,13 @@
-#ifndef __XEN_TOOLS_LIBS__
-#define __XEN_TOOLS_LIBS__
+#ifndef __XEN_TOOLS_COMMON_MACROS__
+#define __XEN_TOOLS_COMMON_MACROS__
+
+/*
+ * Caution:
+ *
+ * This header must be completely self-contained. There are no external
+ * references to variables or functions allowed, as the file might be included
+ * for different runtime environments, such as firmware or normal programs.
+ */
 
 #ifndef BUILD_BUG_ON
 #if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)
@@ -67,4 +75,4 @@
 #define __must_check __attribute__((__warn_unused_result__))
 #endif
 
-#endif /* __XEN_TOOLS_LIBS__ */
+#endif /* __XEN_TOOLS_COMMON_MACROS__ */
diff --git a/tools/libs/call/buffer.c b/tools/libs/call/buffer.c
index 085674d882..2579b8c719 100644
--- a/tools/libs/call/buffer.c
+++ b/tools/libs/call/buffer.c
@@ -16,7 +16,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 
 #include "private.h"
 
diff --git a/tools/libs/ctrl/xc_pm.c b/tools/libs/ctrl/xc_pm.c
index 76d7eb7f26..c3a9864bf7 100644
--- a/tools/libs/ctrl/xc_pm.c
+++ b/tools/libs/ctrl/xc_pm.c
@@ -21,7 +21,7 @@
 #include 
 #include 

[PATCH v2 0/4] tools: use xen-tools/libs.h for common definitions

[Juergen Gross]

There are some macros defined multiple times in tools. Use only
a single header file for defining those macros and drop the copies.

V2:
- add patch 1 (Andrew Cooper)

Juergen Gross (4):
  tools: rename xen-tools/libs.h file to common-macros.h
  tools: add container_of() macro to xen-tools/common-macros.h
  tools: get rid of additional min() and max() definitions
  tools: add offsetof() to xen-tools/common-macros.h

 tools/console/daemon/io.c |  2 +-
 tools/firmware/hvmloader/util.h   | 11 ++
 tools/firmware/include/stddef.h   |  4 ++--
 tools/fuzz/cpu-policy/afl-policy-fuzzer.c |  2 +-
 .../xen-tools/{libs.h => common-macros.h} | 22 ---
 tools/libfsimage/Rules.mk |  2 ++
 tools/libfsimage/xfs/fsys_xfs.c   |  4 +---
 tools/libs/call/buffer.c  |  2 +-
 tools/libs/ctrl/xc_pm.c   |  2 +-
 tools/libs/ctrl/xc_private.h  |  2 +-
 tools/libs/foreignmemory/linux.c  |  2 +-
 tools/libs/gnttab/freebsd.c   |  2 +-
 tools/libs/gnttab/linux.c |  2 +-
 tools/libs/guest/xg_core_arm.c|  2 +-
 tools/libs/guest/xg_cpuid_x86.c   |  2 +-
 tools/libs/guest/xg_dom_arm.c |  2 +-
 tools/libs/guest/xg_dom_bzimageloader.c   |  2 +-
 tools/libs/guest/xg_dom_x86.c |  2 +-
 tools/libs/guest/xg_sr_common.c   |  2 +-
 tools/libs/light/libxl_internal.h |  2 +-
 tools/libs/light/libxl_psr.c  |  2 +-
 tools/libs/stat/xenstat_linux.c   |  2 +-
 tools/libs/vchan/init.c   |  7 +-
 tools/misc/xen-access.c   |  2 +-
 tools/misc/xen-cpuid.c|  2 +-
 tools/misc/xen-diag.c |  2 +-
 tools/misc/xen-hptool.c   |  2 +-
 tools/misc/xen-livepatch.c|  2 +-
 tools/misc/xen-mfndump.c  |  2 +-
 tools/misc/xenpm.c|  2 +-
 tools/ocaml/libs/mmap/xenmmap_stubs.c |  2 +-
 tools/ocaml/libs/xc/xenctrl_stubs.c   |  2 +-
 tools/python/xen/lowlevel/xc/xc.c |  2 +-
 tools/tests/cpu-policy/test-cpu-policy.c  |  2 +-
 .../paging-mempool/test-paging-mempool.c  |  2 +-
 tools/tests/resource/test-resource.c  |  2 +-
 tools/tests/tsx/test-tsx.c|  2 +-
 tools/tests/vhpet/emul.h  | 16 +-
 tools/tests/vpci/emul.h   | 22 +--
 tools/tests/x86_emulator/x86-emulate.h|  7 +-
 tools/tests/xenstore/test-xenstore.c  |  2 +-
 tools/xenstore/list.h |  6 ++---
 tools/xenstore/utils.h|  2 +-
 tools/xentrace/analyze.h  |  2 +-
 tools/xl/xl_cmdtable.c|  2 +-
 xen/lib/x86/private.h |  2 +-
 46 files changed, 68 insertions(+), 105 deletions(-)
 rename tools/include/xen-tools/{libs.h => common-macros.h} (74%)

-- 
2.35.3

Re: [PATCH v1 2/3] xen/riscv: setup initial pagetables

[Julien Grall]

On 27/02/2023 15:17, Jan Beulich wrote:

On 25.02.2023 19:05, Julien Grall wrote:

On 24/02/2023 15:06, Oleksii Kurochko wrote:

@@ -43,6 +45,11 @@ static void __init disable_fpu(void)
   
   void __init noreturn start_xen(void)

   {
+unsigned long load_start= (unsigned long)start;
+unsigned long load_end  = load_start + (unsigned long)(_end - _start);


I am a bit puzzled, on top of load_addr() and linker_addr(), you wrote
it can't use global variable/function. But here... you are using them.
So how is this different?


I guess "use" means "access" (i.e. call a function or read/write a
variable). I suppose it does not mean "take the address of".


If so, then I don't understand why we need to pass linker_start, 
linker_end in parameters for setup_initial_pages().


Cheers,

--
Julien Grall

Re: [XEN PATCH v7 10/20] xen/arm: ffa: add direct request support

[Bertrand Marquis]

Hi Jens,

> On 22 Feb 2023, at 16:33, Jens Wiklander  wrote:
> 
> Adds support for sending a FF-A direct request. Checks that the SP also
> supports handling a 32-bit direct request. 64-bit direct requests are
> not used by the mediator itself so there is not need to check for that.
> 
> Signed-off-by: Jens Wiklander 
> ---
> xen/arch/arm/tee/ffa.c | 119 +
> 1 file changed, 119 insertions(+)
> 
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 463fd7730573..a5d8a12635b6 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -142,6 +142,7 @@
> 
> struct ffa_ctx {
> uint32_t guest_vers;
> +bool interrupted;

This is added and set here for one special error code but is never used.
I would suggest to introduce this when there will be an action based on it.

> };
> 
> /* Negotiated FF-A version to use with the SPMC */
> @@ -167,6 +168,55 @@ static bool ffa_get_version(uint32_t *vers)
> return true;
> }
> 
> +static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp)
> +{
> +switch ( resp->a0 )
> +{
> +case FFA_ERROR:
> +if ( resp->a2 )
> +return resp->a2;
> +else
> +return FFA_RET_NOT_SUPPORTED;
> +case FFA_SUCCESS_32:
> +case FFA_SUCCESS_64:
> +return FFA_RET_OK;
> +default:
> +return FFA_RET_NOT_SUPPORTED;
> +}
> +}
> +
> +static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t a2,
> +   register_t a3, register_t a4)
> +{
> +const struct arm_smccc_1_2_regs arg = {
> +.a0 = fid,
> +.a1 = a1,
> +.a2 = a2,
> +.a3 = a3,
> +.a4 = a4,
> +};
> +struct arm_smccc_1_2_regs resp;
> +
> +arm_smccc_1_2_smc(, );
> +
> +return get_ffa_ret_code();
> +}
> +
> +static int32_t ffa_features(uint32_t id)
> +{
> +return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0);
> +}
> +
> +static bool check_mandatory_feature(uint32_t id)
> +{
> +uint32_t ret = ffa_features(id);
> +
> +if (ret)
> +printk(XENLOG_ERR "ffa: mandatory feature id %#x missing\n", id);

It might be useful here to actually print the error code.
Are we sure that all errors actually mean not supported ?

> +
> +return !ret;
> +}
> +
> static uint16_t get_vm_id(const struct domain *d)
> {
> /* +1 since 0 is reserved for the hypervisor in FF-A */
> @@ -208,6 +258,66 @@ static void handle_version(struct cpu_user_regs *regs)
> set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0);
> }
> 
> +static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t 
> fid)
> +{
> +struct arm_smccc_1_2_regs arg = { .a0 = fid, };
> +struct arm_smccc_1_2_regs resp = { };
> +struct domain *d = current->domain;
> +struct ffa_ctx *ctx = d->arch.tee;
> +uint32_t src_dst;
> +uint64_t mask;
> +
> +if ( smccc_is_conv_64(fid) )
> +mask = GENMASK_ULL(63, 0);
> +else
> +mask = GENMASK_ULL(31, 0);
> +
> +src_dst = get_user_reg(regs, 1);
> +if ( (src_dst >> 16) != get_vm_id(d) )
> +{
> +resp.a0 = FFA_ERROR;
> +resp.a2 = FFA_RET_INVALID_PARAMETERS;
> +goto out;
> +}
> +
> +arg.a1 = src_dst;
> +arg.a2 = get_user_reg(regs, 2) & mask;
> +arg.a3 = get_user_reg(regs, 3) & mask;
> +arg.a4 = get_user_reg(regs, 4) & mask;
> +arg.a5 = get_user_reg(regs, 5) & mask;
> +arg.a6 = get_user_reg(regs, 6) & mask;
> +arg.a7 = get_user_reg(regs, 7) & mask;
> +
> +while ( true )
> +{
> +arm_smccc_1_2_smc(, );
> +
> +switch ( resp.a0 )
> +{
> +case FFA_INTERRUPT:
> +ctx->interrupted = true;
> +goto out;
> +case FFA_ERROR:
> +case FFA_SUCCESS_32:
> +case FFA_SUCCESS_64:
> +case FFA_MSG_SEND_DIRECT_RESP_32:
> +case FFA_MSG_SEND_DIRECT_RESP_64:
> +goto out;
> +default:
> +/* Bad fid, report back. */
> +memset(, 0, sizeof(arg));
> +arg.a0 = FFA_ERROR;
> +arg.a1 = src_dst;
> +arg.a2 = FFA_RET_NOT_SUPPORTED;
> +continue;

There is a potential infinite loop here and i do not understand
why this needs to be done.
Here if something is returning a value that you do not understand
you send back an ERROR to it. I do not find in the spec where this
is supposed to be done.
Can you explain a bit here ?

> +}
> +}
> +
> +out:
> +set_regs(regs, resp.a0, resp.a1 & mask, resp.a2 & mask, resp.a3 & mask,
> + resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask);
> +}
> +
> static bool ffa_handle_call(struct cpu_user_regs *regs)
> {
> uint32_t fid = get_user_reg(regs, 0);
> @@ -225,6 +335,12 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> case FFA_ID_GET:
> set_regs_success(regs, get_vm_id(d), 0);
> return true;
> +case 

Re: [PATCH v3 10/14] x86/vmx: move declarations used only by vmx code from vmx.h to private headers

[Jan Beulich]

On 24.02.2023 19:50, Xenia Ragiadakou wrote:
> --- /dev/null
> +++ b/xen/arch/x86/hvm/vmx/pi.h
> @@ -0,0 +1,78 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * pi.h: VMX Posted Interrupts
> + *
> + * Copyright (c) 2004, Intel Corporation.
> + */
> +
> +#ifndef __X86_HVM_VMX_PI_PRIV_H__
> +#define __X86_HVM_VMX_PI_PRIV_H__

I can see that you need something to disambiguate the two vmx.h. But
here you don't need the PRIV infix, do you? Even in the private vmx.h
I'd like to ask to consider e.g. __VMX_H__ as the guard (and then
__PI_H__ here), rather than one which doesn't really match the
filename.

Jan

Re: [XEN v3 6/9] xen/arm: Introduce choice to enable 64/32 bit physical addressing

[Ayan Kumar Halder]

On 11/02/2023 10:01, Julien Grall wrote:

Hi Ayan,


Hi Julien,

I need some clarification.



On 08/02/2023 12:05, Ayan Kumar Halder wrote:

Some Arm based hardware platforms which does not support LPAE
(eg Cortex-R52), uses 32 bit physical addresses.
Also, users may choose to use 32 bits to represent physical addresses
for optimization.

To support the above use cases, we have introduced arch independent
configs to choose if the physical address can be represented using
32 bits (PHYS_ADDR_32) or 64 bits (PHYS_ADDR_64).
For now only ARM_32 provides support to enable 32 bit physical
addressing.

Signed-off-by: Ayan Kumar Halder 
---

Changes from -
v1 - 1. Extracted from "[XEN v1 8/9] xen/arm: Other adaptations 
required to support 32bit paddr".


v2 - 1. Introduced Kconfig choice. ARM_64 can select PHYS_ADDR_64 
only whereas

ARM_32 can select PHYS_ADDR_32 or PHYS_ADDR_64.
2. For CONFIG_ARM_PA_32, paddr_t is defined as 'unsigned long'.

(Jan,Julien please let me know if I understood your suggestion about 
Kconfig correctly).


  xen/arch/Kconfig | 12 +++
  xen/arch/arm/Kconfig | 31 
  xen/arch/arm/include/asm/page-bits.h |  2 ++
  xen/arch/arm/include/asm/types.h |  6 ++
  4 files changed, 51 insertions(+)

diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
index 7028f7b74f..1eff312b51 100644
--- a/xen/arch/Kconfig
+++ b/xen/arch/Kconfig
@@ -1,6 +1,18 @@
  config 64BIT
  bool
  +config PHYS_ADDR_32
+    bool
+    help
+  Select this option if the physical addresses can be 
represented by

+  32 bits.
+
+config PHYS_ADDR_64
+    bool
+    help
+  Select this option if the physical addresses can be represented
+  64 bits.
+
So you likely don't want to allow the user to select them directly 
(IOW remove the help section). However, I don't see any code using it. 
Did you actually intended to use PHYS_ADDR_{32, 64} in the code?



  config NR_CPUS
  int "Maximum number of CPUs"
  range 1 4095
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 239d3aed3c..0955891e86 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -18,6 +18,37 @@ config ARM
  select HAS_PDX
  select HAS_PMAP
  select IOMMU_FORCE_PT_SHARE
+    choice


I think this is a bit odd that this choice is part of CONFIG_ARM. It 
would be better it is separate. You can do that by removing one 
indentation.


+    bool "Representative width for any physical address (default 
64bit)"

+    optional
+    ---help---
+  You may want to specify the width to represent the physical
+  address space.
+  By default, the physical addresses are represented using
+  64 bits.
+
+  However in certain platforms, the physical addresses may be
+  represented using 32 bits.
+  Also, the user may decide that the physical addresses can be
+  represented using 32 bits for a given SoC. In those cases,
+  user may want to enable 32 bit physical address for
+  optimization.
+  For now, we have enabled this choice for ARM_32 only.
+
+    config ARM_PA_64
+    select PHYS_ADDR_64
+    bool "Select 64 bits to represent physical address"
+    ---help---
+  Use 64 bits to represent physical address.
+
+    config ARM_PA_32
+    select PHYS_ADDR_32
+    depends on ARM_32
+    bool "Select 32 bits to represent physical address"
+    ---help---
+  Use 32 bits to represent physical address.


As I wrote in v2, I think this is a bit odd to ask the user what would 
be the width of paddr_t. It is also not scalable if we decide in the 
future to define different PADDR_BITS (i.e. 48, 40, 36, 32).


So it would be better to allow the user to define PADDR_BITS that can 
then be translated by Xen to which ever paddr_t is suitable.


Something like:

choice
   prompt: "Physical address space size" if ARM_32
   default ARM_PA_48 if ARM_64
   default AMR_PA_40 if ARM_32
   help
 ...

config ARM_PA_BITS_32
   bool "32-bit"
   help
   XXX Add help here to explain the benefits of using 32-bit.

   select PHYS_ADDR_T_32
   depends on ARM_32

config ARM_PA_BITS_40
   bool "40-bit"
   select PHYS_ADDR_T_64
   depends on ARM_32

config ARM_PA_BITS_48
   bool "40-bit"
   select PHYS_ADDR_T_64
   depends on ARM_48
endchoice

config PADDR_BITS
   int
   default 32 if ARM_PA_BITS_32
   default 40 if ARM_PA_BITS_40
   default 48 if ARM_PA_BITS_48

With this approach, there would be no structural change in the Kconfig 
if we wanted to support 32/40-bit on Arm64.



+
+    endchoice
    config ARCH_DEFCONFIG
  string
diff --git a/xen/arch/arm/include/asm/page-bits.h 
b/xen/arch/arm/include/asm/page-bits.h

index 5d6477e599..8f4dcebcfd 100644
--- a/xen/arch/arm/include/asm/page-bits.h
+++ b/xen/arch/arm/include/asm/page-bits.h
@@ -5,6 +5,8 @@
    #ifdef CONFIG_ARM_64
  

Re: [PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Jan Beulich]

On 27.02.2023 16:12, Jan Beulich wrote:
> On 24.02.2023 16:06, Oleksii Kurochko wrote:
>> +static void __attribute__((section(".entry")))
>> +_setup_initial_pagetables(pte_t *second, pte_t *first, pte_t *zeroeth,
> 
> Why the special section (also again further down)?

Looking at patch 2 it occurred to me that you probably mean __init here.

Jan

Re: [PATCH v1 2/3] xen/riscv: setup initial pagetables

[Jan Beulich]

On 25.02.2023 19:05, Julien Grall wrote:
> On 24/02/2023 15:06, Oleksii Kurochko wrote:
>> @@ -43,6 +45,11 @@ static void __init disable_fpu(void)
>>   
>>   void __init noreturn start_xen(void)
>>   {
>> +unsigned long load_start= (unsigned long)start;
>> +unsigned long load_end  = load_start + (unsigned long)(_end - 
>> _start);
> 
> I am a bit puzzled, on top of load_addr() and linker_addr(), you wrote 
> it can't use global variable/function. But here... you are using them. 
> So how is this different?

I guess "use" means "access" (i.e. call a function or read/write a
variable). I suppose it does not mean "take the address of".

Jan

Re: [PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Jan Beulich]

On 24.02.2023 16:06, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/page.h
> @@ -0,0 +1,90 @@
> +#ifndef _ASM_RISCV_PAGE_H
> +#define _ASM_RISCV_PAGE_H
> +
> +#include 
> +#include 
> +
> +#define PAGE_ENTRIES512
> +#define VPN_BITS(9)
> +#define VPN_MASK((unsigned long)((1 << VPN_BITS) - 1))
> +
> +#ifdef CONFIG_RISCV_64
> +/* L3 index Bit[47:39] */
> +#define THIRD_SHIFT (39)
> +#define THIRD_MASK  (VPN_MASK << THIRD_SHIFT)
> +/* L2 index Bit[38:30] */
> +#define SECOND_SHIFT(30)
> +#define SECOND_MASK (VPN_MASK << SECOND_SHIFT)
> +/* L1 index Bit[29:21] */
> +#define FIRST_SHIFT (21)
> +#define FIRST_MASK  (VPN_MASK << FIRST_SHIFT)
> +/* L0 index Bit[20:12] */
> +#define ZEROETH_SHIFT   (12)
> +#define ZEROETH_MASK(VPN_MASK << ZEROETH_SHIFT)
> +
> +#else // CONFIG_RISCV_32
> +
> +/* L1 index Bit[31:22] */
> +#define FIRST_SHIFT (22)
> +#define FIRST_MASK  (VPN_MASK << FIRST_SHIFT)
> +
> +/* L0 index Bit[21:12] */
> +#define ZEROETH_SHIFT   (12)
> +#define ZEROETH_MASK(VPN_MASK << ZEROETH_SHIFT)
> +#endif
> +
> +#define THIRD_SIZE  (1 << THIRD_SHIFT)
> +#define THIRD_MAP_MASK  (~(THIRD_SIZE - 1))
> +#define SECOND_SIZE (1 << SECOND_SHIFT)
> +#define SECOND_MAP_MASK (~(SECOND_SIZE - 1))
> +#define FIRST_SIZE  (1 << FIRST_SHIFT)
> +#define FIRST_MAP_MASK  (~(FIRST_SIZE - 1))
> +#define ZEROETH_SIZE(1 << ZEROETH_SHIFT)
> +#define ZEROETH_MAP_MASK(~(ZEROETH_SIZE - 1))
> +
> +#define PTE_SHIFT   10
> +
> +#define PTE_VALID   BIT(0, UL)
> +#define PTE_READABLEBIT(1, UL)
> +#define PTE_WRITABLEBIT(2, UL)
> +#define PTE_EXECUTABLE  BIT(3, UL)
> +#define PTE_USERBIT(4, UL)
> +#define PTE_GLOBAL  BIT(5, UL)
> +#define PTE_ACCESSEDBIT(6, UL)
> +#define PTE_DIRTY   BIT(7, UL)
> +#define PTE_RSW (BIT(8, UL) | BIT(9, UL))
> +
> +#define PTE_LEAF_DEFAULT(PTE_VALID | PTE_READABLE | PTE_WRITABLE | 
> PTE_EXECUTABLE)
> +#define PTE_TABLE   (PTE_VALID)
> +
> +/* Calculate the offsets into the pagetables for a given VA */
> +#define zeroeth_linear_offset(va)   ((va) >> ZEROETH_SHIFT)
> +#define first_linear_offset(va) ((va) >> FIRST_SHIFT)
> +#define second_linear_offset(va)((va) >> SECOND_SHIFT)
> +#define third_linear_offset(va) ((va) >> THIRD_SHIFT)
> +
> +#define pagetable_zeroeth_index(va) zeroeth_linear_offset((va) & 
> ZEROETH_MASK)
> +#define pagetable_first_index(va)   first_linear_offset((va) & FIRST_MASK)
> +#define pagetable_second_index(va)  second_linear_offset((va) & SECOND_MASK)
> +#define pagetable_third_index(va)   third_linear_offset((va) & THIRD_MASK)
> +
> +/* Page Table entry */
> +typedef struct {
> +uint64_t pte;
> +} pte_t;
> +
> +/* Shift the VPN[x] or PPN[x] fields of a virtual or physical address
> + * to become the shifted PPN[x] fields of a page table entry */
> +#define addr_to_ppn(x) (((x) >> PAGE_SHIFT) << PTE_SHIFT)
> +
> +static inline pte_t paddr_to_pte(unsigned long paddr)
> +{
> +return (pte_t) { .pte = addr_to_ppn(paddr) };
> +}
> +
> +static inline bool pte_is_valid(pte_t *p)

Btw - const whenever possible please, especially in such basic helpers.

> --- /dev/null
> +++ b/xen/arch/riscv/mm.c
> @@ -0,0 +1,223 @@
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * xen_second_pagetable is indexed with the VPN[2] page table entry field
> + * xen_first_pagetable is accessed from the VPN[1] page table entry field
> + * xen_zeroeth_pagetable is accessed from the VPN[0] page table entry field
> + */
> +pte_t xen_second_pagetable[PAGE_ENTRIES] 
> __attribute__((__aligned__(PAGE_SIZE)));

static?

> +static pte_t xen_first_pagetable[PAGE_ENTRIES]
> +__attribute__((__aligned__(PAGE_SIZE)));
> +static pte_t xen_zeroeth_pagetable[PAGE_ENTRIES]
> +__attribute__((__aligned__(PAGE_SIZE)));

Please use __aligned() instead of open-coding it. You also may want to
specifiy the section here explicitly, as .bss.page_aligned (as we do
elsewhere).

> +extern unsigned long _stext;
> +extern unsigned long _etext;
> +extern unsigned long __init_begin;
> +extern unsigned long __init_end;
> +extern unsigned long _srodata;
> +extern unsigned long _erodata;

Please use kernel.h and drop then colliding declarations. For what's
left please use array types, as suggested elsewhere already.

> +paddr_t phys_offset;
> +
> +#define resolve_early_addr(x) \
> +({   
>\
> + unsigned long * __##x;  
>\
> +if ( load_addr_start <= x && x < load_addr_end ) 
>\

Nit: Mismatched 

[PATCH] libs/guest: Fix resource leaks in xc_core_arch_map_p2m_tree_rw()

[Andrew Cooper]

Edwin, with the help of GCC's -fanalyzer, identified that p2m_frame_list_list
gets leaked.  What fanalyzer can't see is that the live_p2m_frame_list_list
and live_p2m_frame_list foreign mappings are leaked too.

Rework the logic so the out path is executed unconditionally, which cleans up
all the intermediate allocations/mappings appropriately.

Fixes: bd7a29c3d0b9 ("tools/libs/ctrl: fix xc_core_arch_map_p2m() to support 
linear p2m table")
Reported-by: Edwin Török 
Signed-off-by: Andrew Cooper 
Reviewed-by: Juergen Gross 
---
 tools/libs/guest/xg_core_x86.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tools/libs/guest/xg_core_x86.c b/tools/libs/guest/xg_core_x86.c
index 61106b98b877..c5e4542c 100644
--- a/tools/libs/guest/xg_core_x86.c
+++ b/tools/libs/guest/xg_core_x86.c
@@ -229,11 +229,11 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch, struct 
domain_info_context *dinf
  uint32_t dom, shared_info_any_t *live_shinfo)
 {
 /* Double and single indirect references to the live P2M table */
-xen_pfn_t *live_p2m_frame_list_list;
+xen_pfn_t *live_p2m_frame_list_list = NULL;
 xen_pfn_t *live_p2m_frame_list = NULL;
 /* Copies of the above. */
 xen_pfn_t *p2m_frame_list_list = NULL;
-xen_pfn_t *p2m_frame_list;
+xen_pfn_t *p2m_frame_list = NULL;
 
 int err;
 int i;
@@ -297,8 +297,6 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch, struct 
domain_info_context *dinf
 
 dinfo->p2m_frames = P2M_FL_ENTRIES;
 
-return p2m_frame_list;
-
  out:
 err = errno;
 
@@ -312,7 +310,7 @@ xc_core_arch_map_p2m_tree_rw(xc_interface *xch, struct 
domain_info_context *dinf
 
 errno = err;
 
-return NULL;
+return p2m_frame_list;
 }
 
 static int
-- 
2.30.2

Re: [PATCH] build: add crypto/ to SUBDIRS

[Jan Beulich]

On 27.02.2023 15:46, Michal Orzel wrote:
> On 27/02/2023 14:54, Jan Beulich wrote:
>> On 27.02.2023 14:41, Michal Orzel wrote:
>>> On 27/02/2023 11:10, Jan Beulich wrote:
 On 27.02.2023 10:53, Michal Orzel wrote:
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE
>   $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
>   $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 
> 'ALL_LIBS=$(ALL_LIBS-y)' $@
>
> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
> +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test
>  define all_sources
>  ( find include -type f -name '*.h' -print; \
>find $(SUBDIRS) -type f -name '*.[chS]' -print )

 As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo
 also only be included when selected (or at the very least only when an
 arch might select it, which afaics is possible on x86 only right now).

 It would also help if in the description you made explicit that SUBDIRS
 isn't used for anything else (the name, after all, is pretty generic).
 Which actually points at an issue: I suppose the variable would actually
 better be used elsewhere as well, e.g. in the _clean: rule and perhaps
 also in the setting of ALL_OBJS-y. (That'll require splitting the
 variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and
 $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which
 has caused crypto to be missing from SUBDIRS.

>>> I think what you wrote can be split into 2 parts: the part being a goal of 
>>> this patch
>>> and the cleanup/improvements that would be beneficial but not related to 
>>> this patch.
>>> The second part involves more code and there are parts to be discussed:
>>>
>>> 1) If we decide to create ALL_OBJS-y from SUBDIRS, then we would need to 
>>> carve out test/ dir
>>> that is not part of ALL_OBJS-y and add it to SUBDIRS later on. Also, the 
>>> order of ALL_OBJS-y matters
>>> for linking, so we would need to transfer the responsibility to SUBDIRS. 
>>> The natural placement of
>>> SUBDIRS (including SUBDIRS-y, etc.) would be right above ALL_OBJS-y. 
>>> However, when doing clean (next point),
>>> need-config is set to n and SUBDIRS would be empty. This means it would 
>>> need to be defined somewhere at the
>>> top of the Makefile (thus harder to make sure the linking order is correct).
>>>
>>> 2) If we deicide to use SUBDIRS for _clean rule, then we would need some 
>>> foreach loop, right?
>>> Apart from that, there are other directories that are not part of SUBDIRS 
>>> i.e. include/, tools/.
>>> Together with SUBDIRS variable, it would create confusion (these dirs are 
>>> also sub-directories, so why
>>> not having them listed in this variable?). Also, I can see that we do clean 
>>> not only for $(TARGET_ARCH)
>>> but for all the existing architectures.
>>
>> I understand that the changes would be more involved, but I disagree with
>> your "two parts" statement: If what I've outlined was already the case,
>> your patch would not even exist (because crypto/ would already be taken
>> care of wherever needed).
>>
>>> I would prefer to stick for now to the goal of this patch which is to add 
>>> crypto/ so that it is taken
>>> into account for the tags/csope generation. Would the following change be 
>>> ok for that purpose?
>>>
>>> diff --git a/xen/Makefile b/xen/Makefile
>>> index 2d55bb9401f4..05bf301bd7ab 100644
>>> --- a/xen/Makefile
>>> +++ b/xen/Makefile
>>> @@ -589,7 +589,9 @@ $(TARGET): outputmakefile FORCE
>>>   $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
>>>   $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 
>>> 'ALL_LIBS=$(ALL_LIBS-y)' $@
>>>
>>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
>>> +SUBDIRS-$(CONFIG_CRYPTO) += crypto
>>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test $(SUBDIRS-y)
>>> +
>>>  define all_sources
>>>  ( find include -type f -name '*.h' -print; \
>>>find $(SUBDIRS) -type f -name '*.[chS]' -print )
>>
>> The fact that, afaict, this won't do is related to the outline I gave.
>> You've referred yourself to need-config. Most if not all of the goals
>> SUBDIRS is (currently) relevant for are listed in no-dot-config-targets.
>> Hence your change above is effectively a no-op, because CONFIG_CRYPTO
>> will simply be unset in the common case. Therefore if an easy change is
>> wanted, the original version of it would be it. An intermediate
>> approach might be to add crypto to SUBDIRS only when TARGET_ARCH=x86.
>> But neither would preclude the same issue from being introduced again,
>> when a new subdir appears.
> I understand your concerns. However, I cannot see how your suggested changes
> e.g. making use of SUBDIRS in ALL_OBJS and _clean would solve our issue.
> They would reduce 

Re: [XEN PATCH v7 09/20] xen/arm: ffa: add support for FFA_ID_GET

[Julien Grall]

Hi,

On 27/02/2023 14:48, Bertrand Marquis wrote:

On 22 Feb 2023, at 16:33, Jens Wiklander  wrote:

Adds support for the FF-A function FFA_ID_GET to return the ID of the
calling client.

Signed-off-by: Jens Wiklander 
---
xen/arch/arm/tee/ffa.c | 21 -
1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 8b0b80ce1ff5..463fd7730573 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -167,6 +167,12 @@ static bool ffa_get_version(uint32_t *vers)
 return true;
}

+static uint16_t get_vm_id(const struct domain *d)
+{
+/* +1 since 0 is reserved for the hypervisor in FF-A */
+return d->domain_id + 1;
+}
+
static void set_regs(struct cpu_user_regs *regs, register_t v0, register_t v1,
  register_t v2, register_t v3, register_t v4, register_t 
v5,
  register_t v6, register_t v7)
@@ -181,6 +187,12 @@ static void set_regs(struct cpu_user_regs *regs, 
register_t v0, register_t v1,
 set_user_reg(regs, 7, v7);
}

+static void set_regs_success(struct cpu_user_regs *regs, uint32_t w2,
+ uint32_t w3)
+{
+set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, 0, 0, 0, 0);
+}
+
static void handle_version(struct cpu_user_regs *regs)
{
 struct domain *d = current->domain;
@@ -210,6 +222,9 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
 case FFA_VERSION:
 handle_version(regs);
 return true;
+case FFA_ID_GET:
+set_regs_success(regs, get_vm_id(d), 0);
+return true;

 default:
 gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
@@ -221,7 +236,11 @@ static int ffa_domain_init(struct domain *d)
{
 struct ffa_ctx *ctx;

-if ( !ffa_version )
+ /*
+  * We can't use that last possible domain ID or get_vm_id() would cause
+  * an overflow.
+  */
+if ( !ffa_version || d->domain_id == UINT16_MAX)
 return -ENODEV;


In reality the overflow could only happen if this is called by the IDLE domain 
right now.
Anyway this could change and this is making the code more robust at no real 
cost.

I would just suggest here to return a different error code like ERANGE for 
example to
prevent missing ENODEV with other cases not related to FFA not being available.


+1. I would also like to suggest to use >= rather than == in case we 
decide to support more than 16-bit domid.


Cheers,

--
Julien Grall

Re: [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak

[Andrew Cooper]

On 27/02/2023 2:42 pm, Juergen Gross wrote:
> On 24.02.23 15:56, Andrew Cooper wrote:
>> On 24/02/2023 1:36 pm, Edwin Török wrote:
>>> From: Edwin Török 
>>>
>>> Prior to bd7a29c3d0 'out' would've always been executed and memory
>>> freed, but that commit changed it such that it returns early and leaks.
>>>
>>> Found using gcc 12.2.1 `-fanalyzer`:
>>> ```
>>> xg_core_x86.c: In function ‘xc_core_arch_map_p2m_tree_rw’:
>>> xg_core_x86.c:300:5: error: leak of ‘p2m_frame_list_list’ [CWE-401]
>>> [-Werror=analyzer-malloc-leak]
>>>    300 | return p2m_frame_list;
>>>    | ^~
>>>    ‘xc_core_arch_map_p2m_writable’: events 1-2
>>>  |
>>>  |  378 | xc_core_arch_map_p2m_writable(xc_interface *xch,
>>> struct domain_info_context *dinfo, xc_dominfo_t *info,
>>>  |  | ^
>>>  |  | |
>>>  |  | (1) entry to ‘xc_core_arch_map_p2m_writable’
>>>  |..
>>>  |  381 | return xc_core_arch_map_p2m_rw(xch, dinfo, info,
>>> live_shinfo, live_p2m, 1);
>>>  |  |   
>>> ~~~
>>>  |  |    |
>>>  |  |    (2) calling ‘xc_core_arch_map_p2m_rw’ from
>>> ‘xc_core_arch_map_p2m_writable’
>>>  |
>>>  +--> ‘xc_core_arch_map_p2m_rw’: events 3-10
>>>     |
>>>     |  319 | xc_core_arch_map_p2m_rw(xc_interface *xch,
>>> struct domain_info_context *dinfo, xc_dominfo_t *info,
>>>     |  | ^~~
>>>     |  | |
>>>     |  | (3) entry to ‘xc_core_arch_map_p2m_rw’
>>>     |..
>>>     |  328 | if ( xc_domain_nr_gpfns(xch, info->domid,
>>> >p2m_size) < 0 )
>>>     |  |    ~
>>>     |  |    |
>>>     |  |    (4) following ‘false’ branch...
>>>     |..
>>>     |  334 | if ( dinfo->p2m_size < info->nr_pages  )
>>>     |  | ~~ ~
>>>     |  | |  |
>>>     |  | |  (6) following ‘false’ branch...
>>>     |  | (5) ...to here
>>>     |..
>>>     |  340 | p2m_cr3 = GET_FIELD(live_shinfo,
>>> arch.p2m_cr3, dinfo->guest_width);
>>>     |  | ~~~
>>>     |  | |
>>>     |  | (7) ...to here
>>>     |  341 |
>>>     |  342 | p2m_frame_list = p2m_cr3 ?
>>> xc_core_arch_map_p2m_list_rw(xch, dinfo, dom, live_shinfo, p2m_cr3)
>>>     |  | 
>>> ~
>>>     |  343 |  :
>>> xc_core_arch_map_p2m_tree_rw(xch, dinfo, dom, live_shinfo);
>>>     |  | 
>>> 
>>>     |  |  | |
>>>     |  |  | (9) ...to here
>>>     |  |  | (10) calling
>>> ‘xc_core_arch_map_p2m_tree_rw’ from ‘xc_core_arch_map_p2m_rw’
>>>     |  |  (8) following
>>> ‘false’ branch...
>>>     |
>>>     +--> ‘xc_core_arch_map_p2m_tree_rw’: events 11-24
>>>    |
>>>    |  228 |
>>> xc_core_arch_map_p2m_tree_rw(xc_interface *xch, struct
>>> domain_info_context *dinfo,
>>>    |  | ^~~~
>>>    |  | |
>>>    |  | (11) entry to
>>> ‘xc_core_arch_map_p2m_tree_rw’
>>>    |..
>>>    |  245 | if ( !live_p2m_frame_list_list )
>>>    |  |    ~
>>>    |  |    |
>>>    |  |    (12) following ‘false’ branch
>>> (when ‘live_p2m_frame_list_list’ is non-NULL)...
>>>    |..
>>>    |  252 | if ( !(p2m_frame_list_list =
>>> malloc(PAGE_SIZE)) )
>>>    |  | ~~ ~
>>> ~
>>>    |  | |  | |
>>>    |  | |  | (14)
>>> allocated here
>>>    |  | |  (15) assuming
>>> ‘p2m_frame_list_list’ is non-NULL
>>>    |  | |  (16) following ‘false’ branch
>>> (when ‘p2m_frame_list_list’ is non-NULL)...
>>>    |  | (13) ...to here
>>>    |..
>>>    |  257 | memcpy(p2m_frame_list_list,
>>> live_p2m_frame_list_list, PAGE_SIZE);
>>>    |  | ~~
>>>    |  | |
>>>    |  | (17) ...to here
>>>    |..
>>>    |  266 | else if ( dinfo->guest_width <
>>> sizeof(unsigned long) )
>>>    

Re: [XEN PATCH v7 09/20] xen/arm: ffa: add support for FFA_ID_GET

[Bertrand Marquis]

Hi Jens,

> On 22 Feb 2023, at 16:33, Jens Wiklander  wrote:
> 
> Adds support for the FF-A function FFA_ID_GET to return the ID of the
> calling client.
> 
> Signed-off-by: Jens Wiklander 
> ---
> xen/arch/arm/tee/ffa.c | 21 -
> 1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 8b0b80ce1ff5..463fd7730573 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -167,6 +167,12 @@ static bool ffa_get_version(uint32_t *vers)
> return true;
> }
> 
> +static uint16_t get_vm_id(const struct domain *d)
> +{
> +/* +1 since 0 is reserved for the hypervisor in FF-A */
> +return d->domain_id + 1;
> +}
> +
> static void set_regs(struct cpu_user_regs *regs, register_t v0, register_t v1,
>  register_t v2, register_t v3, register_t v4, register_t 
> v5,
>  register_t v6, register_t v7)
> @@ -181,6 +187,12 @@ static void set_regs(struct cpu_user_regs *regs, 
> register_t v0, register_t v1,
> set_user_reg(regs, 7, v7);
> }
> 
> +static void set_regs_success(struct cpu_user_regs *regs, uint32_t w2,
> + uint32_t w3)
> +{
> +set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, 0, 0, 0, 0);
> +}
> +
> static void handle_version(struct cpu_user_regs *regs)
> {
> struct domain *d = current->domain;
> @@ -210,6 +222,9 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> case FFA_VERSION:
> handle_version(regs);
> return true;
> +case FFA_ID_GET:
> +set_regs_success(regs, get_vm_id(d), 0);
> +return true;
> 
> default:
> gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> @@ -221,7 +236,11 @@ static int ffa_domain_init(struct domain *d)
> {
> struct ffa_ctx *ctx;
> 
> -if ( !ffa_version )
> + /*
> +  * We can't use that last possible domain ID or get_vm_id() would cause
> +  * an overflow.
> +  */
> +if ( !ffa_version || d->domain_id == UINT16_MAX)
> return -ENODEV;

In reality the overflow could only happen if this is called by the IDLE domain 
right now.
Anyway this could change and this is making the code more robust at no real 
cost.

I would just suggest here to return a different error code like ERANGE for 
example to
prevent missing ENODEV with other cases not related to FFA not being available.

Cheers
Bertrand

> 
> ctx = xzalloc(struct ffa_ctx);
> -- 
> 2.34.1
> 

Re: [PATCH] build: add crypto/ to SUBDIRS

[Michal Orzel]

On 27/02/2023 14:54, Jan Beulich wrote:
> 
> 
> On 27.02.2023 14:41, Michal Orzel wrote:
>> Hi Jan,
>>
>> On 27/02/2023 11:10, Jan Beulich wrote:
>>>
>>>
>>> On 27.02.2023 10:53, Michal Orzel wrote:
 --- a/xen/Makefile
 +++ b/xen/Makefile
 @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE
   $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
   $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 
 'ALL_LIBS=$(ALL_LIBS-y)' $@

 -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
 +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test
  define all_sources
  ( find include -type f -name '*.h' -print; \
find $(SUBDIRS) -type f -name '*.[chS]' -print )
>>>
>>> As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo
>>> also only be included when selected (or at the very least only when an
>>> arch might select it, which afaics is possible on x86 only right now).
>>>
>>> It would also help if in the description you made explicit that SUBDIRS
>>> isn't used for anything else (the name, after all, is pretty generic).
>>> Which actually points at an issue: I suppose the variable would actually
>>> better be used elsewhere as well, e.g. in the _clean: rule and perhaps
>>> also in the setting of ALL_OBJS-y. (That'll require splitting the
>>> variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and
>>> $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which
>>> has caused crypto to be missing from SUBDIRS.
>>>
>> I think what you wrote can be split into 2 parts: the part being a goal of 
>> this patch
>> and the cleanup/improvements that would be beneficial but not related to 
>> this patch.
>> The second part involves more code and there are parts to be discussed:
>>
>> 1) If we decide to create ALL_OBJS-y from SUBDIRS, then we would need to 
>> carve out test/ dir
>> that is not part of ALL_OBJS-y and add it to SUBDIRS later on. Also, the 
>> order of ALL_OBJS-y matters
>> for linking, so we would need to transfer the responsibility to SUBDIRS. The 
>> natural placement of
>> SUBDIRS (including SUBDIRS-y, etc.) would be right above ALL_OBJS-y. 
>> However, when doing clean (next point),
>> need-config is set to n and SUBDIRS would be empty. This means it would need 
>> to be defined somewhere at the
>> top of the Makefile (thus harder to make sure the linking order is correct).
>>
>> 2) If we deicide to use SUBDIRS for _clean rule, then we would need some 
>> foreach loop, right?
>> Apart from that, there are other directories that are not part of SUBDIRS 
>> i.e. include/, tools/.
>> Together with SUBDIRS variable, it would create confusion (these dirs are 
>> also sub-directories, so why
>> not having them listed in this variable?). Also, I can see that we do clean 
>> not only for $(TARGET_ARCH)
>> but for all the existing architectures.
> 
> I understand that the changes would be more involved, but I disagree with
> your "two parts" statement: If what I've outlined was already the case,
> your patch would not even exist (because crypto/ would already be taken
> care of wherever needed).
> 
>> I would prefer to stick for now to the goal of this patch which is to add 
>> crypto/ so that it is taken
>> into account for the tags/csope generation. Would the following change be ok 
>> for that purpose?
>>
>> diff --git a/xen/Makefile b/xen/Makefile
>> index 2d55bb9401f4..05bf301bd7ab 100644
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -589,7 +589,9 @@ $(TARGET): outputmakefile FORCE
>>   $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
>>   $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 
>> 'ALL_LIBS=$(ALL_LIBS-y)' $@
>>
>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
>> +SUBDIRS-$(CONFIG_CRYPTO) += crypto
>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test $(SUBDIRS-y)
>> +
>>  define all_sources
>>  ( find include -type f -name '*.h' -print; \
>>find $(SUBDIRS) -type f -name '*.[chS]' -print )
> 
> The fact that, afaict, this won't do is related to the outline I gave.
> You've referred yourself to need-config. Most if not all of the goals
> SUBDIRS is (currently) relevant for are listed in no-dot-config-targets.
> Hence your change above is effectively a no-op, because CONFIG_CRYPTO
> will simply be unset in the common case. Therefore if an easy change is
> wanted, the original version of it would be it. An intermediate
> approach might be to add crypto to SUBDIRS only when TARGET_ARCH=x86.
> But neither would preclude the same issue from being introduced again,
> when a new subdir appears.
I understand your concerns. However, I cannot see how your suggested changes
e.g. making use of SUBDIRS in ALL_OBJS and _clean would solve our issue.
They would reduce the repetitions (hence I called them cleanup/improvements)
but as we want to keep tags,cscope,clean as 

Re: [PATCH v3 4/4] xen/x86: switch x86 to use generic implemetation of bug.h

[Jan Beulich]

On 24.02.2023 12:31, Oleksii Kurochko wrote:
> The following changes were made:
> * Make GENERIC_BUG_FRAME mandatory for X86
> * Update asm/bug.h using generic implementation in 
> * Change prototype of debugger_trap_fatal() in asm/debugger.h
>   to align it with generic prototype in .
> * Update do_invalid_op using generic do_bug_frame()
> 
> Signed-off-by: Oleksii Kurochko 

Hmm, there's the question of where to draw the boundary between patch 2
and the one here. Personally I'd say the earlier patch should drop
everything that becomes redundant there. I can see the more minimalistic
earlier change as viable, but then the description there needs to say
why things are being kept which - in principle - could be dropped.

> --- a/xen/arch/x86/include/asm/bug.h
> +++ b/xen/arch/x86/include/asm/bug.h
> @@ -3,75 +3,10 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define BUG_FRAME_STRUCT
> +#define BUG_INSTR   "ud2"
> +#define BUG_ASM_CONST   "c"
>  
> -struct bug_frame {
> -signed int loc_disp:BUG_DISP_WIDTH;
> -unsigned int line_hi:BUG_LINE_HI_WIDTH;
> -signed int ptr_disp:BUG_DISP_WIDTH;
> -unsigned int line_lo:BUG_LINE_LO_WIDTH;
> -signed int msg_disp[];
> -};
> -
> -#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
> -#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp)
> -#define bug_line(b) (b)->line_hi + ((b)->loc_disp < 0)) &
> \
> -   ((1 << BUG_LINE_HI_WIDTH) - 1)) <<
> \
> -  BUG_LINE_LO_WIDTH) +   
> \
> - (((b)->line_lo + ((b)->ptr_disp < 0)) & 
> \
> -  ((1 << BUG_LINE_LO_WIDTH) - 1)))
> -#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
> -
> -#define _ASM_BUGFRAME_TEXT(second_frame) 
> \
> -".Lbug%=: ud2\n" 
> \
> -".pushsection .bug_frames.%c[bf_type], \"a\", @progbits\n"   
> \
> -".p2align 2\n"   
> \
> -".Lfrm%=:\n" 
> \
> -".long (.Lbug%= - .Lfrm%=) + %c[bf_line_hi]\n"   
> \
> -".long (%c[bf_ptr] - .Lfrm%=) + %c[bf_line_lo]\n"
> \
> -".if " #second_frame "\n"
> \
> -".long 0, %c[bf_msg] - .Lfrm%=\n"
> \
> -".endif\n"   
> \
> -".popsection\n"  
> \
> -
> -#define _ASM_BUGFRAME_INFO(type, line, ptr, msg) 
> \
> -[bf_type]"i" (type), 
> \
> -[bf_ptr] "i" (ptr),  
> \
> -[bf_msg] "i" (msg),  
> \
> -[bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1))
> \
> -  << BUG_DISP_WIDTH),
> \
> -[bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)
> -
> -#define BUG_FRAME(type, line, ptr, second_frame, msg) do {   
> \
> -BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)); 
> \
> -BUILD_BUG_ON((type) >= BUGFRAME_NR); 
> \
> -asm volatile ( _ASM_BUGFRAME_TEXT(second_frame)  
> \
> -   :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) );
> \
> -} while (0)
> -
> -
> -#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL)
> -#define BUG() do {  \
> -BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, NULL);  \
> -unreachable();  \
> -} while (0)
> -
> -/*
> - * TODO: untangle header dependences, break BUILD_BUG_ON() out of xen/lib.h,
> - * and use a real static inline here to get proper type checking of fn().
> - */
> -#define run_in_exception_handler(fn)\
> -do {\
> -(void)((fn) == (void (*)(struct cpu_user_regs *))NULL); \
> -BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL); \
> -} while ( 0 )
> -
> -#define assert_failed(msg) do { \
> -BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \
> -unreachable();  \
> -} while (0)
> -
> -#else  /* !__ASSEMBLY__ */
> +#else

While for new #ifdef-ary such comments can reasonably be omitted when
the blocks are short, I'd recommend keeping existing comments (except
in extreme cases) in the interest of reduced code churn. In the case

[xen-unstable-smoke test] 178663: tolerable trouble: pass/starved - PUSHED

[osstest service owner]

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

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   1 build-check(1)   starved  n/a
 build-armhf   2 hosts-allocate   starved  n/a

version targeted for testing:
 xen  4d6df4ec7544d7c912ffab6b6edb4cbefaa01f4c
baseline version:
 xen  608f85a1818697156b72ace4913a17c8178a0ef5

Last test of basis   178391  2023-02-24 21:00:27 Z2 days
Testing same since   178663  2023-02-27 13:03:21 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Jan Beulich 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  starved 
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  starved 
 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
   608f85a181..4d6df4ec75  4d6df4ec7544d7c912ffab6b6edb4cbefaa01f4c -> smoke

Re: [PATCH v1 1/2] xc_core_arch_map_p2m_tree_rw: fix memory leak

[Juergen Gross]

On 24.02.23 15:56, Andrew Cooper wrote:

On 24/02/2023 1:36 pm, Edwin Török wrote:

From: Edwin Török 

Prior to bd7a29c3d0 'out' would've always been executed and memory
freed, but that commit changed it such that it returns early and leaks.

Found using gcc 12.2.1 `-fanalyzer`:
```
xg_core_x86.c: In function ‘xc_core_arch_map_p2m_tree_rw’:
xg_core_x86.c:300:5: error: leak of ‘p2m_frame_list_list’ [CWE-401] 
[-Werror=analyzer-malloc-leak]
   300 | return p2m_frame_list;
   | ^~
   ‘xc_core_arch_map_p2m_writable’: events 1-2
 |
 |  378 | xc_core_arch_map_p2m_writable(xc_interface *xch, struct 
domain_info_context *dinfo, xc_dominfo_t *info,
 |  | ^
 |  | |
 |  | (1) entry to ‘xc_core_arch_map_p2m_writable’
 |..
 |  381 | return xc_core_arch_map_p2m_rw(xch, dinfo, info, live_shinfo, 
live_p2m, 1);
 |  |
~~~
 |  ||
 |  |(2) calling ‘xc_core_arch_map_p2m_rw’ from 
‘xc_core_arch_map_p2m_writable’
 |
 +--> ‘xc_core_arch_map_p2m_rw’: events 3-10
|
|  319 | xc_core_arch_map_p2m_rw(xc_interface *xch, struct 
domain_info_context *dinfo, xc_dominfo_t *info,
|  | ^~~
|  | |
|  | (3) entry to ‘xc_core_arch_map_p2m_rw’
|..
|  328 | if ( xc_domain_nr_gpfns(xch, info->domid, 
>p2m_size) < 0 )
|  |~
|  ||
|  |(4) following ‘false’ branch...
|..
|  334 | if ( dinfo->p2m_size < info->nr_pages  )
|  | ~~ ~
|  | |  |
|  | |  (6) following ‘false’ branch...
|  | (5) ...to here
|..
|  340 | p2m_cr3 = GET_FIELD(live_shinfo, arch.p2m_cr3, 
dinfo->guest_width);
|  | ~~~
|  | |
|  | (7) ...to here
|  341 |
|  342 | p2m_frame_list = p2m_cr3 ? 
xc_core_arch_map_p2m_list_rw(xch, dinfo, dom, live_shinfo, p2m_cr3)
|  |  
~
|  343 |  : 
xc_core_arch_map_p2m_tree_rw(xch, dinfo, dom, live_shinfo);
|  |  

|  |  | |
|  |  | (9) ...to here
|  |  | (10) calling 
‘xc_core_arch_map_p2m_tree_rw’ from ‘xc_core_arch_map_p2m_rw’
|  |  (8) following ‘false’ 
branch...
|
+--> ‘xc_core_arch_map_p2m_tree_rw’: events 11-24
   |
   |  228 | xc_core_arch_map_p2m_tree_rw(xc_interface *xch, 
struct domain_info_context *dinfo,
   |  | ^~~~
   |  | |
   |  | (11) entry to ‘xc_core_arch_map_p2m_tree_rw’
   |..
   |  245 | if ( !live_p2m_frame_list_list )
   |  |~
   |  ||
   |  |(12) following ‘false’ branch (when 
‘live_p2m_frame_list_list’ is non-NULL)...
   |..
   |  252 | if ( !(p2m_frame_list_list = malloc(PAGE_SIZE)) 
)
   |  | ~~ ~ ~
   |  | |  | |
   |  | |  | (14) allocated here
   |  | |  (15) assuming ‘p2m_frame_list_list’ is 
non-NULL
   |  | |  (16) following ‘false’ branch (when 
‘p2m_frame_list_list’ is non-NULL)...
   |  | (13) ...to here
   |..
   |  257 | memcpy(p2m_frame_list_list, 
live_p2m_frame_list_list, PAGE_SIZE);
   |  | ~~
   |  | |
   |  | (17) ...to here
   |..
   |  266 | else if ( dinfo->guest_width < sizeof(unsigned 
long) )
   |  | ~
   |  | |
   |  | (18) following ‘false’ branch...
   |..
   |  270 | live_p2m_frame_list =
   |  | ~~~
   |  | |
   |  | (19) ...to here
   |..
   |  275 | if ( !live_p2m_frame_list )
   

Re: [PATCH v3 2/4] xen: change to

[Jan Beulich]

On 24.02.2023 12:31, Oleksii Kurochko wrote:
> Since the generic version of bug.h stuff was introduced use 
> instead of unnecessary 

You keep saying "unnecessary" here, but that's not really correct.
Including asm/bug.h alone simply becomes meaningless. So how about
"... instead of now useless (in isolation) "?

> --- a/xen/arch/x86/include/asm/bug.h
> +++ b/xen/arch/x86/include/asm/bug.h
> @@ -1,19 +1,10 @@
>  #ifndef __X86_BUG_H__
>  #define __X86_BUG_H__
>  
> -#define BUG_DISP_WIDTH24
> -#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> -#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> -
> -#define BUGFRAME_run_fn 0
> -#define BUGFRAME_warn   1
> -#define BUGFRAME_bug2
> -#define BUGFRAME_assert 3
> -
> -#define BUGFRAME_NR 4
> -
>  #ifndef __ASSEMBLY__
>  
> +#define BUG_FRAME_STRUCT
> +
>  struct bug_frame {
>  signed int loc_disp:BUG_DISP_WIDTH;
>  unsigned int line_hi:BUG_LINE_HI_WIDTH;

Why would x86 continue to define its own bug_frame (and other items)?

Jan

Re: [PATCH v5 1/5] Use HTTPS for all xenbits.xen.org Git repos

[Anthony PERARD]

On Sat, Feb 25, 2023 at 11:34:32PM +0100, Marek Marczykowski-Górecki wrote:
> On Sat, Feb 25, 2023 at 03:37:11PM -0500, Demi Marie Obenour wrote:
> > Obtaining code over an insecure transport is a terrible idea for
> > blatently obvious reasons.  Even for non-executable data, insecure
> > transports are considered deprecated.
> > 
> > This patch enforces the use of secure transports for all xenbits.xen.org
> > Git repositories.  It was generated with the following shell script:
> > 
> > git ls-files -z |
> > xargs -0 -- sed -Ei -- 
> > 's@(git://xenbits\.xen\.org|http://xenbits\.xen\.org/git-http)/@https://xenbits.xen.org/git-http/@g'
> > 
> > All altered links have been tested and are known to work.
> > 
> > Signed-off-by: Demi Marie Obenour 
> 
> It seems expired Lets Encrypt root issue applies to few other containers
> too:

Yes, I haven't finished rebuilding all containers needed to be rebuilt.
I've mostly took care of fixing dockerfiles for those needed to change.

Cheers,

> - archlinux:current: 
> https://gitlab.com/xen-project/patchew/xen/-/jobs/3834739751
> - debian:stretch-i386: 
> https://gitlab.com/xen-project/patchew/xen/-/jobs/3834739762
> - debian:unstable-i386: 
> https://gitlab.com/xen-project/patchew/xen/-/jobs/3834739771



-- 
Anthony PERARD

Re: [PATCH v3 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME

[Jan Beulich]

On 24.02.2023 12:31, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/common/bug.c
> @@ -0,0 +1,109 @@
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +/* Set default value for TRAP_invalid_op as it is defined only for X86 now */
> +#ifndef TRAP_invalid_op
> +#define TRAP_invalid_op 0
> +#endif
> +
> +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc)
> +{
> +const struct bug_frame *bug = NULL;
> +const struct virtual_region *region;
> +const char *prefix = "", *filename, *predicate;
> +unsigned long fixup;
> +unsigned int id = BUGFRAME_NR, lineno;
> +
> +region = find_text_region(pc);
> +if ( region )
> +{
> +for ( id = 0; id < BUGFRAME_NR; id++ )
> +{
> +const struct bug_frame *b;
> +unsigned int i;
> +
> +for ( i = 0, b = region->frame[id].bugs;
> +  i < region->frame[id].n_bugs; b++, i++ )
> +{
> +if ( bug_loc(b) == pc )
> +{
> +bug = b;
> +goto found;
> +}
> +}
> +}
> +}
> +
> + found:
> +if ( !bug )
> +return -EINVAL;
> +
> +if ( id == BUGFRAME_run_fn )
> +{
> +#ifdef BUG_FN_REG
> +void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG;
> +#else
> +void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> +#endif
> +
> +fn(regs);
> +
> +return id;
> +}
> +
> +/* WARN, BUG or ASSERT: decode the filename pointer and line number. */
> +filename = bug_ptr(bug);
> +if ( !is_kernel(filename) && !is_patch(filename) )
> +return -EINVAL;
> +fixup = strlen(filename);
> +if ( fixup > 50 )
> +{
> +filename += fixup - 47;
> +prefix = "...";
> +}
> +lineno = bug_line(bug);
> +
> +switch ( id )
> +{
> +case BUGFRAME_warn:
> +printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
> +show_execution_state(regs);
> +
> +return id;
> +
> +case BUGFRAME_bug:
> +printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> +
> +if ( debugger_trap_fatal(TRAP_invalid_op, regs) )

TRAP_invalid_op is, as said, about to disappear on x86 as well. I think
this construct wants abstracting by another asm/bug.h provided macro
(taking just regs).

Jan

Re: [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h

[Andrew Cooper]

On 27/02/2023 2:12 pm, Jan Beulich wrote:
> On 27.02.2023 15:06, Andrew Cooper wrote:
>> On 27/02/2023 12:43 pm, Jan Beulich wrote:
>>> On 27.02.2023 13:34, Juergen Gross wrote:
 On 27.02.23 13:31, Jan Beulich wrote:
> On 27.02.2023 13:09, Juergen Gross wrote:
>> --- a/tools/firmware/hvmloader/util.h
>> +++ b/tools/firmware/hvmloader/util.h
>> @@ -30,9 +30,6 @@ enum {
>>   #define SEL_DATA32  0x0020
>>   #define SEL_CODE64  0x0028
>>   
>> -#undef offsetof
>> -#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
>> -
>>   #undef NULL
>>   #define NULL ((void*)0)
>>   
>> diff --git a/tools/firmware/include/stddef.h 
>> b/tools/firmware/include/stddef.h
>> index c7f974608a..926ae12fa5 100644
>> --- a/tools/firmware/include/stddef.h
>> +++ b/tools/firmware/include/stddef.h
>> @@ -1,10 +1,10 @@
>>   #ifndef _STDDEF_H_
>>   #define _STDDEF_H_
>>   
>> +#include 
> I'm not convinced firmware/ files can validly include this header.
 This file only contains macros which don't call any external functions.

 Would you be fine with me adding a related comment to it?
>>> If it was to be usable like this, that would be a prereq, but personally
>>> I don't view this as sufficient. The environment code runs in that lives
>>> under firmware/ is simply different (hvmloader, for example, is 32-bit
>>> no matter that the toolstack would normally be 64-bit), so to me it
>>> feels like setting up a trap for ourselves. If Andrew or Roger are fully
>>> convinced this is a good move, I won't be standing in the way ...
>> We still support 32bit builds of the toolstack on multiple
>> architectures, so I don't see bitness as an argument against.
> Bitness by itself wasn't the argument. Mixed bitness is what concerns
> me.

I still don't see how that would matter.  The bitness would always be
consistent inside a single translation.

>
>> I am slightly uneasy adding a header like this, but it really is only
>> common macros and I don't see how it could possibly change from that
>> given the existing uses.  OTOH, removing things like the problematic
>> definition of offsetof() is an improvement.
>>
>> I'd probably tentatively ack this patch, seeing as it is a minor
>> improvlement, but I think there's a middle option too.  Rename libs.h to
>> macros.h or common-macros.h?  IMO that would be something that's far
>> more obviously safe to include into firmware/, and something rather more
>> descriptive at all of its include sites.
>>
>> Thoughts?
> Maybe. One fundamental requirement would need to be made prominently
> visible in such a header: It has to be entirely self-contained, i.e.
> it may in particular not gain any dependencies on configure's output.

That's a reasonable restriction IMO.  Again, I don't see how we'd ever
come to violate that in the future, given the current use here.

~Andrew

Re: [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h

[Jan Beulich]

On 27.02.2023 15:06, Andrew Cooper wrote:
> On 27/02/2023 12:43 pm, Jan Beulich wrote:
>> On 27.02.2023 13:34, Juergen Gross wrote:
>>> On 27.02.23 13:31, Jan Beulich wrote:
 On 27.02.2023 13:09, Juergen Gross wrote:
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -30,9 +30,6 @@ enum {
>   #define SEL_DATA32  0x0020
>   #define SEL_CODE64  0x0028
>   
> -#undef offsetof
> -#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
> -
>   #undef NULL
>   #define NULL ((void*)0)
>   
> diff --git a/tools/firmware/include/stddef.h 
> b/tools/firmware/include/stddef.h
> index c7f974608a..926ae12fa5 100644
> --- a/tools/firmware/include/stddef.h
> +++ b/tools/firmware/include/stddef.h
> @@ -1,10 +1,10 @@
>   #ifndef _STDDEF_H_
>   #define _STDDEF_H_
>   
> +#include 
 I'm not convinced firmware/ files can validly include this header.
>>> This file only contains macros which don't call any external functions.
>>>
>>> Would you be fine with me adding a related comment to it?
>> If it was to be usable like this, that would be a prereq, but personally
>> I don't view this as sufficient. The environment code runs in that lives
>> under firmware/ is simply different (hvmloader, for example, is 32-bit
>> no matter that the toolstack would normally be 64-bit), so to me it
>> feels like setting up a trap for ourselves. If Andrew or Roger are fully
>> convinced this is a good move, I won't be standing in the way ...
> 
> We still support 32bit builds of the toolstack on multiple
> architectures, so I don't see bitness as an argument against.

Bitness by itself wasn't the argument. Mixed bitness is what concerns
me.

> I am slightly uneasy adding a header like this, but it really is only
> common macros and I don't see how it could possibly change from that
> given the existing uses.  OTOH, removing things like the problematic
> definition of offsetof() is an improvement.
> 
> I'd probably tentatively ack this patch, seeing as it is a minor
> improvlement, but I think there's a middle option too.  Rename libs.h to
> macros.h or common-macros.h?  IMO that would be something that's far
> more obviously safe to include into firmware/, and something rather more
> descriptive at all of its include sites.
> 
> Thoughts?

Maybe. One fundamental requirement would need to be made prominently
visible in such a header: It has to be entirely self-contained, i.e.
it may in particular not gain any dependencies on configure's output.

Jan

Re: [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h

[Juergen Gross]

On 27.02.23 15:06, Andrew Cooper wrote:

On 27/02/2023 12:43 pm, Jan Beulich wrote:

On 27.02.2023 13:34, Juergen Gross wrote:

On 27.02.23 13:31, Jan Beulich wrote:

On 27.02.2023 13:09, Juergen Gross wrote:

--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -30,9 +30,6 @@ enum {
   #define SEL_DATA32  0x0020
   #define SEL_CODE64  0x0028
   
-#undef offsetof

-#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
-
   #undef NULL
   #define NULL ((void*)0)
   
diff --git a/tools/firmware/include/stddef.h b/tools/firmware/include/stddef.h

index c7f974608a..926ae12fa5 100644
--- a/tools/firmware/include/stddef.h
+++ b/tools/firmware/include/stddef.h
@@ -1,10 +1,10 @@
   #ifndef _STDDEF_H_
   #define _STDDEF_H_
   
+#include 

I'm not convinced firmware/ files can validly include this header.

This file only contains macros which don't call any external functions.

Would you be fine with me adding a related comment to it?

If it was to be usable like this, that would be a prereq, but personally
I don't view this as sufficient. The environment code runs in that lives
under firmware/ is simply different (hvmloader, for example, is 32-bit
no matter that the toolstack would normally be 64-bit), so to me it
feels like setting up a trap for ourselves. If Andrew or Roger are fully
convinced this is a good move, I won't be standing in the way ...


We still support 32bit builds of the toolstack on multiple
architectures, so I don't see bitness as an argument against.

I am slightly uneasy adding a header like this, but it really is only
common macros and I don't see how it could possibly change from that
given the existing uses.  OTOH, removing things like the problematic
definition of offsetof() is an improvement.

I'd probably tentatively ack this patch, seeing as it is a minor
improvlement, but I think there's a middle option too.  Rename libs.h to
macros.h or common-macros.h?  IMO that would be something that's far
more obviously safe to include into firmware/, and something rather more
descriptive at all of its include sites.

Thoughts?


I'm fine with that.

My preference would be "common-macros.h".


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h

[Andrew Cooper]

On 27/02/2023 12:43 pm, Jan Beulich wrote:
> On 27.02.2023 13:34, Juergen Gross wrote:
>> On 27.02.23 13:31, Jan Beulich wrote:
>>> On 27.02.2023 13:09, Juergen Gross wrote:
 --- a/tools/firmware/hvmloader/util.h
 +++ b/tools/firmware/hvmloader/util.h
 @@ -30,9 +30,6 @@ enum {
   #define SEL_DATA32  0x0020
   #define SEL_CODE64  0x0028
   
 -#undef offsetof
 -#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
 -
   #undef NULL
   #define NULL ((void*)0)
   
 diff --git a/tools/firmware/include/stddef.h 
 b/tools/firmware/include/stddef.h
 index c7f974608a..926ae12fa5 100644
 --- a/tools/firmware/include/stddef.h
 +++ b/tools/firmware/include/stddef.h
 @@ -1,10 +1,10 @@
   #ifndef _STDDEF_H_
   #define _STDDEF_H_
   
 +#include 
>>> I'm not convinced firmware/ files can validly include this header.
>> This file only contains macros which don't call any external functions.
>>
>> Would you be fine with me adding a related comment to it?
> If it was to be usable like this, that would be a prereq, but personally
> I don't view this as sufficient. The environment code runs in that lives
> under firmware/ is simply different (hvmloader, for example, is 32-bit
> no matter that the toolstack would normally be 64-bit), so to me it
> feels like setting up a trap for ourselves. If Andrew or Roger are fully
> convinced this is a good move, I won't be standing in the way ...

We still support 32bit builds of the toolstack on multiple
architectures, so I don't see bitness as an argument against.

I am slightly uneasy adding a header like this, but it really is only
common macros and I don't see how it could possibly change from that
given the existing uses.  OTOH, removing things like the problematic
definition of offsetof() is an improvement.

I'd probably tentatively ack this patch, seeing as it is a minor
improvlement, but I think there's a middle option too.  Rename libs.h to
macros.h or common-macros.h?  IMO that would be something that's far
more obviously safe to include into firmware/, and something rather more
descriptive at all of its include sites.

Thoughts?

~Andrew

Re: [PATCH v3 05/12] x86/xen: set MTRR state when running as Xen PV initial domain

[Juergen Gross]

On 27.02.23 14:52, Boris Ostrovsky wrote:



On 2/27/23 2:12 AM, Juergen Gross wrote:

On 24.02.23 22:00, Boris Ostrovsky wrote:


On 2/23/23 4:32 AM, Juergen Gross wrote:

+
+    for (reg = 0; reg < MTRR_MAX_VAR_RANGES; reg++) {
+    op.u.read_memtype.reg = reg;
+    if (HYPERVISOR_platform_op())
+    break;



If we fail on the first iteration, do we still want to mark MTRRs are 
enabled/set in mtrr_overwrite_state()?


Hmm, good idea.

I think we should just drop the call of mtrr_overwrite_state() in this
case.



TBH I am not sure what the right way is to handle errors here. What if the 
hypercall fails on second iteration?


The main reason would be that only one variable MTRR is available.

Its not as if there are very complicated scenarios leading to failures here.

Either the interface is usable and then it will work, or it isn't usable
and we can fall back to today's handling by ignoring MTRRs.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH] build: add crypto/ to SUBDIRS

[Jan Beulich]

On 27.02.2023 14:41, Michal Orzel wrote:
> Hi Jan,
> 
> On 27/02/2023 11:10, Jan Beulich wrote:
>>
>>
>> On 27.02.2023 10:53, Michal Orzel wrote:
>>> --- a/xen/Makefile
>>> +++ b/xen/Makefile
>>> @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE
>>>   $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
>>>   $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 
>>> 'ALL_LIBS=$(ALL_LIBS-y)' $@
>>>
>>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
>>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test
>>>  define all_sources
>>>  ( find include -type f -name '*.h' -print; \
>>>find $(SUBDIRS) -type f -name '*.[chS]' -print )
>>
>> As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo
>> also only be included when selected (or at the very least only when an
>> arch might select it, which afaics is possible on x86 only right now).
>>
>> It would also help if in the description you made explicit that SUBDIRS
>> isn't used for anything else (the name, after all, is pretty generic).
>> Which actually points at an issue: I suppose the variable would actually
>> better be used elsewhere as well, e.g. in the _clean: rule and perhaps
>> also in the setting of ALL_OBJS-y. (That'll require splitting the
>> variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and
>> $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which
>> has caused crypto to be missing from SUBDIRS.
>>
> I think what you wrote can be split into 2 parts: the part being a goal of 
> this patch
> and the cleanup/improvements that would be beneficial but not related to this 
> patch.
> The second part involves more code and there are parts to be discussed:
> 
> 1) If we decide to create ALL_OBJS-y from SUBDIRS, then we would need to 
> carve out test/ dir
> that is not part of ALL_OBJS-y and add it to SUBDIRS later on. Also, the 
> order of ALL_OBJS-y matters
> for linking, so we would need to transfer the responsibility to SUBDIRS. The 
> natural placement of
> SUBDIRS (including SUBDIRS-y, etc.) would be right above ALL_OBJS-y. However, 
> when doing clean (next point),
> need-config is set to n and SUBDIRS would be empty. This means it would need 
> to be defined somewhere at the
> top of the Makefile (thus harder to make sure the linking order is correct).
> 
> 2) If we deicide to use SUBDIRS for _clean rule, then we would need some 
> foreach loop, right?
> Apart from that, there are other directories that are not part of SUBDIRS 
> i.e. include/, tools/.
> Together with SUBDIRS variable, it would create confusion (these dirs are 
> also sub-directories, so why
> not having them listed in this variable?). Also, I can see that we do clean 
> not only for $(TARGET_ARCH)
> but for all the existing architectures.

I understand that the changes would be more involved, but I disagree with
your "two parts" statement: If what I've outlined was already the case,
your patch would not even exist (because crypto/ would already be taken
care of wherever needed).

> I would prefer to stick for now to the goal of this patch which is to add 
> crypto/ so that it is taken
> into account for the tags/csope generation. Would the following change be ok 
> for that purpose?
> 
> diff --git a/xen/Makefile b/xen/Makefile
> index 2d55bb9401f4..05bf301bd7ab 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -589,7 +589,9 @@ $(TARGET): outputmakefile FORCE
>   $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
>   $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 
> 'ALL_LIBS=$(ALL_LIBS-y)' $@
>  
> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
> +SUBDIRS-$(CONFIG_CRYPTO) += crypto
> +SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test $(SUBDIRS-y)
> +
>  define all_sources
>  ( find include -type f -name '*.h' -print; \
>find $(SUBDIRS) -type f -name '*.[chS]' -print )

The fact that, afaict, this won't do is related to the outline I gave.
You've referred yourself to need-config. Most if not all of the goals
SUBDIRS is (currently) relevant for are listed in no-dot-config-targets.
Hence your change above is effectively a no-op, because CONFIG_CRYPTO
will simply be unset in the common case. Therefore if an easy change is
wanted, the original version of it would be it. An intermediate
approach might be to add crypto to SUBDIRS only when TARGET_ARCH=x86.
But neither would preclude the same issue from being introduced again,
when a new subdir appears.

Jan

Re: [PATCH v3 05/12] x86/xen: set MTRR state when running as Xen PV initial domain

[Boris Ostrovsky]

On 2/27/23 2:12 AM, Juergen Gross wrote:

On 24.02.23 22:00, Boris Ostrovsky wrote:


On 2/23/23 4:32 AM, Juergen Gross wrote:

+
+    for (reg = 0; reg < MTRR_MAX_VAR_RANGES; reg++) {
+    op.u.read_memtype.reg = reg;
+    if (HYPERVISOR_platform_op())
+    break;



If we fail on the first iteration, do we still want to mark MTRRs are 
enabled/set in mtrr_overwrite_state()?


Hmm, good idea.

I think we should just drop the call of mtrr_overwrite_state() in this
case.



TBH I am not sure what the right way is to handle errors here. What if the 
hypercall fails on second iteration?


-boris

Re: [PATCH] build: add crypto/ to SUBDIRS

[Michal Orzel]

Hi Jan,

On 27/02/2023 11:10, Jan Beulich wrote:
> 
> 
> On 27.02.2023 10:53, Michal Orzel wrote:
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE
>>   $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
>>   $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 
>> 'ALL_LIBS=$(ALL_LIBS-y)' $@
>>
>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test
>>  define all_sources
>>  ( find include -type f -name '*.h' -print; \
>>find $(SUBDIRS) -type f -name '*.[chS]' -print )
> 
> As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo
> also only be included when selected (or at the very least only when an
> arch might select it, which afaics is possible on x86 only right now).
> 
> It would also help if in the description you made explicit that SUBDIRS
> isn't used for anything else (the name, after all, is pretty generic).
> Which actually points at an issue: I suppose the variable would actually
> better be used elsewhere as well, e.g. in the _clean: rule and perhaps
> also in the setting of ALL_OBJS-y. (That'll require splitting the
> variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and
> $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which
> has caused crypto to be missing from SUBDIRS.
> 
I think what you wrote can be split into 2 parts: the part being a goal of this 
patch
and the cleanup/improvements that would be beneficial but not related to this 
patch.
The second part involves more code and there are parts to be discussed:

1) If we decide to create ALL_OBJS-y from SUBDIRS, then we would need to carve 
out test/ dir
that is not part of ALL_OBJS-y and add it to SUBDIRS later on. Also, the order 
of ALL_OBJS-y matters
for linking, so we would need to transfer the responsibility to SUBDIRS. The 
natural placement of
SUBDIRS (including SUBDIRS-y, etc.) would be right above ALL_OBJS-y. However, 
when doing clean (next point),
need-config is set to n and SUBDIRS would be empty. This means it would need to 
be defined somewhere at the
top of the Makefile (thus harder to make sure the linking order is correct).

2) If we deicide to use SUBDIRS for _clean rule, then we would need some 
foreach loop, right?
Apart from that, there are other directories that are not part of SUBDIRS i.e. 
include/, tools/.
Together with SUBDIRS variable, it would create confusion (these dirs are also 
sub-directories, so why
not having them listed in this variable?). Also, I can see that we do clean not 
only for $(TARGET_ARCH)
but for all the existing architectures.

I would prefer to stick for now to the goal of this patch which is to add 
crypto/ so that it is taken
into account for the tags/csope generation. Would the following change be ok 
for that purpose?

diff --git a/xen/Makefile b/xen/Makefile
index 2d55bb9401f4..05bf301bd7ab 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -589,7 +589,9 @@ $(TARGET): outputmakefile FORCE
$(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
$(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 
'ALL_LIBS=$(ALL_LIBS-y)' $@
 
-SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
+SUBDIRS-$(CONFIG_CRYPTO) += crypto
+SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test $(SUBDIRS-y)
+
 define all_sources
 ( find include -type f -name '*.h' -print; \
   find $(SUBDIRS) -type f -name '*.[chS]' -print )


~Michal

[linux-linus test] 178623: regressions - trouble: fail/pass/starved

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-qemuu-nested-intel  8 xen-boot  fail REGR. vs. 178042
 test-amd64-amd64-freebsd12-amd64  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-qemuu-ws16-amd64  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-dom0pvh-xl-amd 14 guest-start   fail REGR. vs. 178042
 test-amd64-amd64-xl-pvshim8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-vhd   8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-pvhv2-intel  8 xen-boot  fail REGR. vs. 178042
 test-amd64-amd64-xl-qemuu-win7-amd64  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-qemut-win7-amd64  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-xsm   8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-credit1   8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 8 xen-boot fail REGR. 
vs. 178042
 test-amd64-amd64-xl   8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 8 xen-boot fail REGR. vs. 
178042
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 8 xen-boot fail REGR. vs. 
178042
 test-amd64-amd64-xl-qemuu-debianhvm-amd64  8 xen-bootfail REGR. vs. 178042
 test-amd64-amd64-xl-shadow8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-qemuu-nested-amd  8 xen-bootfail REGR. vs. 178042
 test-amd64-amd64-xl-pvhv2-amd  8 xen-bootfail REGR. vs. 178042
 test-amd64-amd64-xl-qemut-debianhvm-i386-xsm  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-qemut-ws16-amd64  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-freebsd11-amd64  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-libvirt-raw  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-pygrub   8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 8 xen-boot fail REGR. 
vs. 178042
 test-amd64-amd64-dom0pvh-xl-intel 14 guest-start fail REGR. vs. 178042
 test-amd64-amd64-pair12 xen-boot/src_hostfail REGR. vs. 178042
 test-amd64-amd64-pair13 xen-boot/dst_hostfail REGR. vs. 178042
 test-amd64-amd64-libvirt-qcow2  8 xen-boot   fail REGR. vs. 178042
 test-amd64-amd64-libvirt-xsm  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-qemut-debianhvm-amd64  8 xen-bootfail REGR. vs. 178042
 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm  8 xen-boot fail REGR. vs. 178042
 test-arm64-arm64-xl  17 guest-stop   fail REGR. vs. 178042
 test-arm64-arm64-xl-credit1  17 guest-stop   fail REGR. vs. 178042
 test-arm64-arm64-xl-credit2  17 guest-stop   fail REGR. vs. 178042
 test-arm64-arm64-libvirt-xsm 17 guest-stop   fail REGR. vs. 178042
 test-amd64-amd64-libvirt  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-libvirt-pair 12 xen-boot/src_host   fail REGR. vs. 178042
 test-amd64-amd64-libvirt-pair 13 xen-boot/dst_host   fail REGR. vs. 178042
 test-amd64-coresched-amd64-xl  8 xen-bootfail REGR. vs. 178042
 test-arm64-arm64-xl-thunderx 14 guest-start  fail REGR. vs. 178042
 test-amd64-amd64-examine  8 reboot   fail REGR. vs. 178042
 test-amd64-amd64-examine-uefi  8 reboot  fail REGR. vs. 178042
 test-amd64-amd64-xl-qemuu-ovmf-amd64  8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-xl-credit2   8 xen-boot fail REGR. vs. 178042
 test-amd64-amd64-examine-bios  8 reboot  fail REGR. vs. 178042
 test-amd64-amd64-xl-multivcpu  8 xen-bootfail REGR. vs. 178042
 test-arm64-arm64-xl-vhd  12 debian-di-installfail REGR. vs. 178042
 test-arm64-arm64-libvirt-raw 12 debian-di-installfail REGR. vs. 178042
 test-arm64-arm64-xl-xsm 18 guest-start/debian.repeat fail in 178595 REGR. vs. 
178042

Tests which are failing intermittently (not blocking):
 test-arm64-arm64-xl-credit1  14 guest-start  fail in 178595 pass in 178623
 test-arm64-arm64-xl  14 guest-start  fail in 178595 pass in 178623
 test-arm64-arm64-xl-credit2  14 guest-start  fail in 178595 pass in 178623
 test-arm64-arm64-xl-xsm  14 guest-startfail pass in 178595

Regressions which are regarded as allowable (not blocking):
 test-amd64-amd64-xl-rtds  8 xen-boot fail REGR. vs. 178042

Tests which did not succeed, but are not blocking:
 test-arm64-arm64-xl-xsm 15 migrate-support-check fail in 178595 never pass
 test-arm64-arm64-xl-xsm 16 saverestore-support-check fail in 178595 never pass
 test-arm64-arm64-xl  15 

Re: [PATCH v4 3/5] xen/riscv: introduce an implementation of macros from

[Jan Beulich]

On 24.02.2023 12:35, Oleksii Kurochko wrote:
> The patch introduces macros: BUG(), WARN(), run_in_exception(),
> assert_failed.
> 
> The implementation uses "ebreak" instruction in combination with
> diffrent bug frame tables (for each type) which contains useful
> information.
> 
> Signed-off-by: Oleksii Kurochko 
> ---
> Changes in V4:
>   - Updates in RISC-V's :
> * Add explanatory comment about why there is only defined for 32-bits 
> length
>   instructions and 16/32-bits BUG_INSN_{16,32}.
> * Change 'unsigned long' to 'unsigned int' inside GET_INSN_LENGTH().
> * Update declaration of is_valid_bugaddr(): switch return type from int 
> to bool
>   and the argument from 'unsigned int' to 'vaddr'.
>   - Updates in RISC-V's traps.c:
> * replace /xen and /asm includes 
> * update definition of is_valid_bugaddr():switch return type from int to 
> bool
>   and the argument from 'unsigned int' to 'vaddr'. Code style inside 
> function
>   was updated too.
> * do_bug_frame() refactoring:
>   * local variables start and bug became 'const struct bug_frame'
>   * bug_frames[] array became 'static const struct bug_frame[] = ...'
>   * remove all casts
>   * remove unneeded comments and add an explanatory comment that the 
> do_bug_frame()
> will be switched to a generic one.
> * do_trap() refactoring:
>   * read 16-bits value instead of 32-bits as compressed instruction can
> be used and it might happen than only 16-bits may be accessible.
>   * code style updates
>   * re-use instr variable instead of re-reading instruction.
>   - Updates in setup.c:
> * add blank line between xen/ and asm/ includes.
> ---
> Changes in V3:
>   - Rebase the patch "xen/riscv: introduce an implementation of macros
> from " on top of patch series [introduce generic implementation
> of macros from bug.h]
> ---
> Changes in V2:
>   - Remove __ in define namings
>   - Update run_in_exception_handler() with
> register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn);
>   - Remove bug_instr_t type and change it's usage to uint32_t
> ---
>  xen/arch/riscv/include/asm/bug.h   |  48 ++
>  xen/arch/riscv/include/asm/processor.h |   2 +
>  xen/arch/riscv/setup.c |   1 +
>  xen/arch/riscv/traps.c | 125 +
>  xen/arch/riscv/xen.lds.S   |  10 ++
>  5 files changed, 186 insertions(+)
>  create mode 100644 xen/arch/riscv/include/asm/bug.h
> 
> diff --git a/xen/arch/riscv/include/asm/bug.h 
> b/xen/arch/riscv/include/asm/bug.h
> new file mode 100644
> index 00..67ade6f895
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/bug.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2021-2023 Vates
> + *
> + */
> +#ifndef _ASM_RISCV_BUG_H
> +#define _ASM_RISCV_BUG_H
> +
> +#include 
> +
> +#ifndef __ASSEMBLY__

This #ifndef contradicts the xen/types.h inclusion. Either the #include
moves down below here, or the #ifndef goes away as pointless. This is
because xen/types.h cannot be include in assembly files.

> +#define BUG_INSTR "ebreak"
> +
> +/*
> + * The base instruction set has a fixed length of 32-bit naturally aligned
> + * instructions.
> + *
> + * There are extensions of variable length ( where each instruction can be
> + * any number of 16-bit parcels in length ) but they aren't used in Xen
> + * and Linux kernel ( where these definitions were taken from ).
> + *
> + * Compressed ISA is used now where the instruction length is 16 bit  and
> + * 'ebreak' instruction, in this case, can be either 16 or 32 bit (
> + * depending on if compressed ISA is used or not )
> + */
> +#define INSN_LENGTH_MASK_UL(0x3)
> +#define INSN_LENGTH_32  _UL(0x3)
> +
> +#define BUG_INSN_32 _UL(0x00100073) /* ebreak */
> +#define BUG_INSN_16 _UL(0x9002) /* c.ebreak */
> +#define COMPRESSED_INSN_MASK_UL(0x)
> +
> +#define GET_INSN_LENGTH(insn)   \
> +({  \
> +unsigned int len;   \
> +len = ((insn & INSN_LENGTH_MASK) == INSN_LENGTH_32) ?   \
> +4UL : 2UL;  \

Why the UL suffixes?

> +len;\
> +})

And anyway - can't the whole macro be written without using any
extension:

#define GET_INSN_LENGTH(insn)   \
(((insn) & INSN_LENGTH_MASK) == INSN_LENGTH_32 ? 4 : 2) \

? (Note also that uses of macro parameters should be properly
parenthesized.)

> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -1,3 +1,4 @@
> +#include 
>  #include 
>  #include 

Why? Eventually this will be needed, I agree, but right now?

> @@ -99,7 +100,131 @@ static void do_unexpected_trap(const 

Re: [PATCH v4 2/5] xen/riscv: introduce trap_init()

[Jan Beulich]

On 24.02.2023 12:35, Oleksii Kurochko wrote:
> @@ -11,6 +13,8 @@ void __init noreturn start_xen(void)
>  {
>  early_printk("Hello from C env\n");
>  
> +trap_init();
> +
>  for ( ;; )
>  asm volatile ("wfi");

Along the lines of what Andrew has said there - it's hard to see
how this can come (both code flow and patch ordering wise) ahead
of clearing .bss.

Jan

Re: [PATCH v4 1/5] xen/riscv: introduce decode_cause() stuff

[Jan Beulich]

On 24.02.2023 12:35, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/traps.c
> +++ b/xen/arch/riscv/traps.c
> @@ -4,10 +4,95 @@
>   *
>   * RISC-V Trap handlers
>   */
> +
> +#include 

I couldn't spot anything in the file which would require this inclusion.

> +#include 
> +
> +#include 
> +#include 
>  #include 
>  #include 
>  
> -void do_trap(struct cpu_user_regs *cpu_regs)
> +static const char *decode_trap_cause(unsigned long cause)
> +{
> +static const char *const trap_causes[] = {
> +[CAUSE_MISALIGNED_FETCH] = "Instruction Address Misaligned",
> +[CAUSE_FETCH_ACCESS] = "Instruction Access Fault",
> +[CAUSE_ILLEGAL_INSTRUCTION] = "Illegal Instruction",
> +[CAUSE_BREAKPOINT] = "Breakpoint",
> +[CAUSE_MISALIGNED_LOAD] = "Load Address Misaligned",
> +[CAUSE_LOAD_ACCESS] = "Load Access Fault",
> +[CAUSE_MISALIGNED_STORE] = "Store/AMO Address Misaligned",
> +[CAUSE_STORE_ACCESS] = "Store/AMO Access Fault",
> +[CAUSE_USER_ECALL] = "Environment Call from U-Mode",
> +[CAUSE_SUPERVISOR_ECALL] = "Environment Call from S-Mode",
> +[CAUSE_MACHINE_ECALL] = "Environment Call from M-Mode",
> +[CAUSE_FETCH_PAGE_FAULT] = "Instruction Page Fault",
> +[CAUSE_LOAD_PAGE_FAULT] = "Load Page Fault",
> +[CAUSE_STORE_PAGE_FAULT] = "Store/AMO Page Fault",
> +[CAUSE_FETCH_GUEST_PAGE_FAULT] = "Instruction Guest Page Fault",
> +[CAUSE_LOAD_GUEST_PAGE_FAULT] = "Load Guest Page Fault",
> +[CAUSE_VIRTUAL_INST_FAULT] = "Virtualized Instruction Fault",
> +[CAUSE_STORE_GUEST_PAGE_FAULT] = "Guest Store/AMO Page Fault",
> +};
> +
> +if ( cause < ARRAY_SIZE(trap_causes) && trap_causes[cause] )
> +return trap_causes[cause];
> +return "UNKNOWN";
> +}
> +
> +const char *decode_reserved_interrupt_cause(unsigned long irq_cause)

For any non-static function that you add you will need a declaration
in a header, which the defining C file then includes. I understand
that during initial bringup functions without (external) callers may
want to (temporarily) exist, but briefly clarifying what the future
expectation regarding external uses might help. Any function that's
not expected to gain external callers should be static.

Jan

Re: [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h

[Jan Beulich]

On 27.02.2023 13:34, Juergen Gross wrote:
> On 27.02.23 13:31, Jan Beulich wrote:
>> On 27.02.2023 13:09, Juergen Gross wrote:
>>> --- a/tools/firmware/hvmloader/util.h
>>> +++ b/tools/firmware/hvmloader/util.h
>>> @@ -30,9 +30,6 @@ enum {
>>>   #define SEL_DATA32  0x0020
>>>   #define SEL_CODE64  0x0028
>>>   
>>> -#undef offsetof
>>> -#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
>>> -
>>>   #undef NULL
>>>   #define NULL ((void*)0)
>>>   
>>> diff --git a/tools/firmware/include/stddef.h 
>>> b/tools/firmware/include/stddef.h
>>> index c7f974608a..926ae12fa5 100644
>>> --- a/tools/firmware/include/stddef.h
>>> +++ b/tools/firmware/include/stddef.h
>>> @@ -1,10 +1,10 @@
>>>   #ifndef _STDDEF_H_
>>>   #define _STDDEF_H_
>>>   
>>> +#include 
>>
>> I'm not convinced firmware/ files can validly include this header.
> 
> This file only contains macros which don't call any external functions.
> 
> Would you be fine with me adding a related comment to it?

If it was to be usable like this, that would be a prereq, but personally
I don't view this as sufficient. The environment code runs in that lives
under firmware/ is simply different (hvmloader, for example, is 32-bit
no matter that the toolstack would normally be 64-bit), so to me it
feels like setting up a trap for ourselves. If Andrew or Roger are fully
convinced this is a good move, I won't be standing in the way ...

Jan

Re: [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h

[Juergen Gross]

On 27.02.23 13:31, Jan Beulich wrote:

On 27.02.2023 13:09, Juergen Gross wrote:

--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -30,9 +30,6 @@ enum {
  #define SEL_DATA32  0x0020
  #define SEL_CODE64  0x0028
  
-#undef offsetof

-#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
-
  #undef NULL
  #define NULL ((void*)0)
  
diff --git a/tools/firmware/include/stddef.h b/tools/firmware/include/stddef.h

index c7f974608a..926ae12fa5 100644
--- a/tools/firmware/include/stddef.h
+++ b/tools/firmware/include/stddef.h
@@ -1,10 +1,10 @@
  #ifndef _STDDEF_H_
  #define _STDDEF_H_
  
+#include 


I'm not convinced firmware/ files can validly include this header.


This file only contains macros which don't call any external functions.

Would you be fine with me adding a related comment to it?


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h

[Jan Beulich]

On 27.02.2023 13:09, Juergen Gross wrote:
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -30,9 +30,6 @@ enum {
>  #define SEL_DATA32  0x0020
>  #define SEL_CODE64  0x0028
>  
> -#undef offsetof
> -#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
> -
>  #undef NULL
>  #define NULL ((void*)0)
>  
> diff --git a/tools/firmware/include/stddef.h b/tools/firmware/include/stddef.h
> index c7f974608a..926ae12fa5 100644
> --- a/tools/firmware/include/stddef.h
> +++ b/tools/firmware/include/stddef.h
> @@ -1,10 +1,10 @@
>  #ifndef _STDDEF_H_
>  #define _STDDEF_H_
>  
> +#include 

I'm not convinced firmware/ files can validly include this header.

Jan

Re: [PATCH v3 00/14] x86/hvm: {svm,vmx} {c,h} cleanup

[Jan Beulich]

On 27.02.2023 13:06, Andrew Cooper wrote:
> On 27/02/2023 11:33 am, Jan Beulich wrote:
>> On 27.02.2023 12:15, Andrew Cooper wrote:
>>> On 27/02/2023 10:46 am, Jan Beulich wrote:
 On 24.02.2023 22:33, Andrew Cooper wrote:
> But I think we want to change tact slightly at this point, so I'm not
> going to do any further tweaking on commit.
>
> Next, I think we want to rename asm/hvm/svm/svm.h to asm/hvm/svm.h,
> updating the non-SVM include paths as we go.  Probably best to
> chain-include the other svm/hvm/svm/*.h headers temporarily, so we've
> only got one tree-wide cleanup of the external include paths.
>
> Quick tangent - I will be making all of that cpu_has_svm_*
> infrastructure disappear by moving it into the normal CPUID handling,
> but I've not had sufficient time to finish that yet.
>
> Next, hvm/svm/nestedsvm.h can merge straight into hvm/svm.h, and
> disappear (after my decoupling patch has gone in).
 Why would you want to fold hvm/svm/nestedsvm.h into hvm/svm/svm.h?
 The latter doesn't use anything from the former, does it?
>>> It's about what else uses them.
>>>
>>> hvm_vcpu needs both svm_vcpu and nestedsvm, so both headers are always
>>> included in tandem.
>> Well, yes, that's how things are today. But can you explain to me why
>> hvm_vcpu has to know nestedsvm's layout?
> 
> Because it's part of the same single memory allocation.

Which keeps growing, and sooner or later we'll need to find something
again to split off, so we won't exceed a page in size. The nested
structures would, to me, look to be prime candidates for such.

>> If the field was a pointer,
>> a forward decl of that struct would suffice, and any entity in the
>> rest of Xen not caring about struct nestedsvm would no longer see it
>> (and hence also no longer be re-built if a change is made there).
> 
> Yes, you could hide it as a pointer.  The cost of doing so is an
> unnecessary extra memory allocation, and extra pointer handling on
> create/destroy, not to mention extra pointer chasing in memory during use.
> 
>>> nestedsvm is literally just one struct now, and no subsystem ought to
>>> have multiple headers when one will do.
>> When one will do, yes. Removing build dependencies is a good reason
>> to have separate headers, though.
> 
> Its not the only only option, and an #ifdef CONFIG_NESTED_VIRT inside
> the struct would be an equally acceptable way of doing this which
> wouldn't involve making an extra memory allocation.

That would make it a build-time decision, but then on NESTED_VIRT=y
hypervisors there might still be guests not meaning to use that
functionality, and for quite some time that may actually be a majority.

> Everything you've posed here is way out of scope for Xenia's series. 

There was never an intention to extend the scope of the work she's doing.
Instead I was trying to limit the scope by suggesting to avoid a piece
of rework which later may want undoing.

Jan

Re: [PATCH] xen/riscv: read hart_id and dtb_base passed by bootloader

[Oleksii]

On Mon, 2023-02-27 at 12:37 +0100, Jan Beulich wrote:
> On 27.02.2023 12:19, Oleksii wrote:
> > On Mon, 2023-02-27 at 10:17 +0100, Jan Beulich wrote:
> > > On 24.02.2023 17:36, Oleksii wrote:
> > > > On Fri, 2023-02-24 at 16:04 +0100, Jan Beulich wrote:
> > > > > On 24.02.2023 15:48, Oleksii Kurochko wrote:
> > > > > > Signed-off-by: Oleksii Kurochko
> > > > > > 
> > > > > > ---
> > > > > >  xen/arch/riscv/setup.c | 12 
> > > > > >  1 file changed, 12 insertions(+)
> > > > > > 
> > > > > > diff --git a/xen/arch/riscv/setup.c
> > > > > > b/xen/arch/riscv/setup.c
> > > > > > index b3f8b10f71..154bf3a0bc 100644
> > > > > > --- a/xen/arch/riscv/setup.c
> > > > > > +++ b/xen/arch/riscv/setup.c
> > > > > > @@ -26,6 +26,18 @@ static void test_macros_from_bug_h(void)
> > > > > >  
> > > > > >  void __init noreturn start_xen(void)
> > > > > >  {
> > > > > > +    /*
> > > > > > + * The following things are passed by bootloader:
> > > > > > + *   a0 -> hart_id
> > > > > > + *   a1 -> dtb_base
> > > > > > +    */
> > > > > > +    register unsigned long hart_id  asm("a0");
> > > > > > +    register unsigned long dtb_base asm("a1");
> > > > > > +
> > > > > > +    asm volatile( "mv %0, a0" : "=r" (hart_id) );
> > > > > > +
> > > > > > +    asm volatile( "mv %0, a1" : "=r" (dtb_base) );
> > > > > 
> > > > > Are you sure this (a) works and (b) is what you want? You're
> > > > > inserting
> > > > Oh, yeah... it should be:
> > > >   unsigned long hart_id;
> > > >   unsigned long dtb_base;
> > > 
> > > As per below - no, I don't think so. I continue to think these
> > > want
> > > to be function parameters.
> > > 
> > > > I did experiments with 'register' and 'asm()' and after rebase
> > > > of
> > > > my
> > > > internal branches git backed these changes...
> > > > 
> > > > Sorry for that I have to double check patches next time.
> > > > 
> > > > It looks like it works only because the variables aren't used
> > > > anywhere.
> > > > > "mov a0, a0" and "mov a1, a1". I suppose as long as the two
> > > > > local
> > > > > variables aren't used further down in the function, the
> > > > > compiler
> > > > > will
> > > > > be able to recognize both registers as dead, and hence use
> > > > > them
> > > > > for
> > > > > argument passing to later functions that you call. But I
> > > > > would
> > > > > expect
> > > > > that to break once you actually consume either of the
> > > > > variables.
> > > > > 
> > > > > Fundamentally I think this is the kind of thing you want do
> > > > > to in
> > > > > assembly. However, in the specific case here, can't you
> > > > > simply
> > > > > have
> > > > > 
> > > > > void __init noreturn start_xen(unsigned long hard_id,
> > > > >    unsigned long dtb_base)
> > > > > {
> > > > >     ...
> > > > > 
> > > > > and all is going to be fine, without any asm()?
> > > > One of the things that I would like to do is to not use an
> > > > assembler as
> > > > much as possible. And as we have C environment ready after a
> > > > few
> > > > assembly instructions in head.S I thought it would be OK to do
> > > > it
> > > > in C
> > > > code somewhere at the start so someone/sonething doesn't alter
> > > > register
> > > > a0 and a1.
> > > 
> > > Avoiding assembly code where possible if of course appreciated,
> > > because
> > > generally C code is easier to maintain. But of course this can
> > > only
> > > be
> > > done if things can be expressed correctly. And you need to keep
> > > in
> > > mind
> > > that asm() statements also are assembly code, just lower volume.
> > > 
> > Let's get hard_id and dtb_base in head.S and pass them as arguments
> > of
> > start_xen() function as you mentioned before.
> 
> Still looks like a misunderstanding to me. Aiui a0 and a1 are the
> registers to hold first and second function arguments each. If
> firmware places the two values in these two registers, then
> start_xen(), with the adjusted declaration, will fit the purpose
> without any assembly code.
> 
It will work for the code we have now, but it will be more save to save
registers a0 and a1 to some variables and pass them to start_xen() as
arguments as they can be changed by something before the start_xen()
call in the future.

~ Oleksii

Re: S0ix support in Xen

[Jan Beulich]

On 27.02.2023 12:48, Simon Gaiser wrote:
> PIT timer: During some previous private discussion it was mentioned that
> the PIT timer that Xen initializes for IO-APIC testing prevents S0ix
> residency and therefore that part needs to be reworked. But if I'm
> reading the current code correctly Xen can already use the HPET timer
> instead, either with an automatic fallback if PIT is unavailable or by
> forcing it via hpet=legacy-replacement=1. Looking at the rest I think
> the PIT isn't used if Xen finds another clocksource. Did I miss
> something?

I think the concern was with calibrating the APIC clock against PIT,
but as you say that connection has been cut. So I think there's no
prereq work left there; Andrew may prove me wrong, though.

> mwait idle driver: While mwait-idle.c share a lot of code with Linux's
> intel_idle.c and the imported code seems to have been updated (for
> example for Alder Lake) it only supports the CPUs with hardcoded
> cstates. Linux's code also has a code path to read the cstate config
> from ACPI if the driver doesn't has a hard coded config for the model.
> This is needed for example for Tiger Lake. For my current testing I
> added the values the Linux code reads from ACPI by hand. But that's of
> course no proper solution. How should this be handled in Xen (IIUC some
> ACPI things are handled by Xen and some by dom0)?

Well, there's first of all some lack of understanding on my part: It
hasn't become clear to me why Linux now has two CPU idle drivers
consuming ACPI data (intel_idle and the purely ACPI-based one). If
two are really needed, then I guess mwait-idle would need extending
to also consume data passed up from Dom0, just like the ACPI-only
driver in Xen does (and which doesn't do anything until such data
was supplied).

> I'm not sure yet is what else in Xen needs to learn about S0ix. Running
> domains need to be halted, that can be handled by the toolstack. What
> other Xen internal things need to be aware of S0ix? Like avoiding
> unnecessary timer wakeups or similar. That's currently my biggest
> unknown and it would be great if someone with more insight and overview
> could give some hints here.

I didn't think the higher-level-software side of things was different
from S3. Instead I thought it's merely a matter of different
interaction with hardware that's needed (which, as per above, includes
avoiding certain things, i.e. in particular the PIT).

Jan

[PATCH 3/3] tools: add offsetof() to xen-tools/libs.h

[Juergen Gross]

Instead of having multiple files defining offsetof(), add the
definition to tools/include/xen-tools/libs.h.

Signed-off-by: Juergen Gross 
---
 tools/firmware/hvmloader/util.h | 3 ---
 tools/firmware/include/stddef.h | 4 ++--
 tools/include/xen-tools/libs.h  | 4 
 tools/libfsimage/Rules.mk   | 2 ++
 tools/libfsimage/xfs/fsys_xfs.c | 4 +---
 tools/libs/vchan/init.c | 4 
 tools/tests/vhpet/emul.h| 2 --
 7 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 69afcc6daa..668ee74f3c 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -30,9 +30,6 @@ enum {
 #define SEL_DATA32  0x0020
 #define SEL_CODE64  0x0028
 
-#undef offsetof
-#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
-
 #undef NULL
 #define NULL ((void*)0)
 
diff --git a/tools/firmware/include/stddef.h b/tools/firmware/include/stddef.h
index c7f974608a..926ae12fa5 100644
--- a/tools/firmware/include/stddef.h
+++ b/tools/firmware/include/stddef.h
@@ -1,10 +1,10 @@
 #ifndef _STDDEF_H_
 #define _STDDEF_H_
 
+#include 
+
 typedef __SIZE_TYPE__ size_t;
 
 #define NULL ((void*)0)
 
-#define offsetof(t, m) __builtin_offsetof(t, m)
-
 #endif
diff --git a/tools/include/xen-tools/libs.h b/tools/include/xen-tools/libs.h
index 3672486daa..c222aa5bb0 100644
--- a/tools/include/xen-tools/libs.h
+++ b/tools/include/xen-tools/libs.h
@@ -71,4 +71,8 @@
 typeof( ((type *)0)->member ) *__mptr = (ptr);\
 (type *)( (char *)__mptr - offsetof(type,member) );})
 
+#ifndef offsetof
+#define offsetof(a, b) __builtin_offsetof(a, b)
+#endif
+
 #endif /* __XEN_TOOLS_LIBS__ */
diff --git a/tools/libfsimage/Rules.mk b/tools/libfsimage/Rules.mk
index cf37d6cb0d..85674f2379 100644
--- a/tools/libfsimage/Rules.mk
+++ b/tools/libfsimage/Rules.mk
@@ -3,6 +3,8 @@ include $(XEN_ROOT)/tools/libfsimage/common.mk
 FSLIB = fsimage.so
 TARGETS += $(FSLIB)
 
+CFLAGS += $(CFLAGS_xeninclude)
+
 .PHONY: all
 all: $(TARGETS)
 
diff --git a/tools/libfsimage/xfs/fsys_xfs.c b/tools/libfsimage/xfs/fsys_xfs.c
index d735a88e55..4c0cde9777 100644
--- a/tools/libfsimage/xfs/fsys_xfs.c
+++ b/tools/libfsimage/xfs/fsys_xfs.c
@@ -19,6 +19,7 @@
 
 #include 
 #include "xfs.h"
+#include 
 
 #define MAX_LINK_COUNT 8
 
@@ -182,9 +183,6 @@ fsb2daddr (xfs_fsblock_t fsbno)
 (xfs_agblock_t)(fsbno & mask32lo(xfs.agblklog)));
 }
 
-#undef offsetof
-#define offsetof(t,m)  ((size_t)&(((t *)0)->m))
-
 static inline int
 btroot_maxrecs (fsi_file_t *ffi)
 {
diff --git a/tools/libs/vchan/init.c b/tools/libs/vchan/init.c
index ea2d7f2c54..9c0c5ca0c5 100644
--- a/tools/libs/vchan/init.c
+++ b/tools/libs/vchan/init.c
@@ -69,10 +69,6 @@
 #define MAX_RING_SHIFT 20
 #define MAX_RING_SIZE (1 << MAX_RING_SHIFT)
 
-#ifndef offsetof
-#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
-#endif
-
 static int init_gnt_srv(struct libxenvchan *ctrl, int domain)
 {
int pages_left = ctrl->read.order >= PAGE_SHIFT ? 1 << 
(ctrl->read.order - PAGE_SHIFT) : 0;
diff --git a/tools/tests/vhpet/emul.h b/tools/tests/vhpet/emul.h
index efd04ed313..2eeefa7098 100644
--- a/tools/tests/vhpet/emul.h
+++ b/tools/tests/vhpet/emul.h
@@ -115,8 +115,6 @@ enum
 NR_COMMON_SOFTIRQS
 };
 
-#define offsetof(t, m) ((unsigned long )&((t *)0)->m)
-
 struct domain;
 
 struct vcpu
-- 
2.35.3

[PATCH 2/3] tools: get rid of additional min() and max() definitions

[Juergen Gross]

Defining min(), min_t(), max() and max_t() at other places than
xen-tools/libs.h isn't needed, as the definitions in said header can
be used instead.

Same applies to BUILD_BUG_ON() in hvmloader.

Signed-off-by: Juergen Gross 
---
 tools/firmware/hvmloader/util.h |  8 ++--
 tools/libs/vchan/init.c |  3 +--
 tools/tests/vhpet/emul.h| 11 +--
 tools/tests/vpci/emul.h | 16 
 4 files changed, 4 insertions(+), 34 deletions(-)

diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 8d95eab28a..69afcc6daa 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -9,6 +9,8 @@
 #include 
 #include "e820.h"
 
+#include 
+
 /* Request un-prefixed values from errno.h. */
 #define XEN_ERRNO(name, value) name = value,
 enum {
@@ -41,12 +43,6 @@ void __assert_failed(const char *assertion, const char 
*file, int line)
 void __bug(const char *file, int line) __attribute__((noreturn));
 #define BUG() __bug(__FILE__, __LINE__)
 #define BUG_ON(p) do { if (p) BUG(); } while (0)
-#define BUILD_BUG_ON(p) ((void)sizeof(char[1 - 2 * !!(p)]))
-
-#define min_t(type,x,y) \
-({ type __x = (x); type __y = (y); __x < __y ? __x: __y; })
-#define max_t(type,x,y) \
-({ type __x = (x); type __y = (y); __x > __y ? __x: __y; })
 
 #define MB(mb) (mb##ULL << 20)
 #define GB(gb) (gb##ULL << 30)
diff --git a/tools/libs/vchan/init.c b/tools/libs/vchan/init.c
index 9195bd3b98..ea2d7f2c54 100644
--- a/tools/libs/vchan/init.c
+++ b/tools/libs/vchan/init.c
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "vchan.h"
 
@@ -72,8 +73,6 @@
 #define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
 #endif
 
-#define max(a,b) ((a > b) ? a : b)
-
 static int init_gnt_srv(struct libxenvchan *ctrl, int domain)
 {
int pages_left = ctrl->read.order >= PAGE_SHIFT ? 1 << 
(ctrl->read.order - PAGE_SHIFT) : 0;
diff --git a/tools/tests/vhpet/emul.h b/tools/tests/vhpet/emul.h
index 7db47c5beb..efd04ed313 100644
--- a/tools/tests/vhpet/emul.h
+++ b/tools/tests/vhpet/emul.h
@@ -114,16 +114,7 @@ enum
 TASKLET_SOFTIRQ,
 NR_COMMON_SOFTIRQS
 };
-/*
- * ..and if you can't take the strict
- * types, you can specify one yourself.
- *
- * Or not use min/max at all, of course.
- */
-#define min_t(type, x, y) \
-({ type __x = (x); type __y = (y); __x < __y ? __x : __y; })
-#define max_t(type, x, y) \
-({ type __x = (x); type __y = (y); __x > __y ? __x : __y; })
+
 #define offsetof(t, m) ((unsigned long )&((t *)0)->m)
 
 struct domain;
diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h
index 237edb4e95..bf3cef5586 100644
--- a/tools/tests/vpci/emul.h
+++ b/tools/tests/vpci/emul.h
@@ -106,22 +106,6 @@ typedef union {
 #define BUG() assert(0)
 #define ASSERT_UNREACHABLE() assert(0)
 
-#define min(x, y) ({\
-const typeof(x) tx = (x);   \
-const typeof(y) ty = (y);   \
-\
-(void) ( == );\
-tx < ty ? tx : ty;  \
-})
-
-#define max(x, y) ({\
-const typeof(x) tx = (x);   \
-const typeof(y) ty = (y);   \
-\
-(void) ( == );\
-tx > ty ? tx : ty;  \
-})
-
 #endif
 
 /*
-- 
2.35.3

[PATCH 1/3] tools: add container_of() macro to xen-tools/libs.h

[Juergen Gross]

Instead of having 4 identical copies of the definition of a
container_of() macro in different tools header files, add that macro
to xen-tools/libs.h and use that instead.

Delete the other copies of that macro.

Signed-off-by: Juergen Gross 
---
There is a similar macro CONTAINER_OF() defined in
tools/include/xentoolcore_internal.h, which allows to not only use a
type for the 2nd parameter, but a variable, too.
I'd like to get rid of that macro as well, but there are lots of use
cases especially in libxl. Any thoughts regarding that macro?
I could either:
- don't touch it at all
- enhance container_of() like CONTAINER_OF() and replace all use cases
  of CONTAINER_OF() with container_of()
- replace the few CONTAINER_OF() users outside libxl with container_of()
  and define CONTAINER_OF() in e.g. libxl_internal.h
- replace all CONTAINER_OF() use cases with container_of(), including
  the change from (.., var, ..) to (.., type, ...).

Signed-off-by: Juergen Gross 
---
 tools/include/xen-tools/libs.h | 4 
 tools/tests/vhpet/emul.h   | 3 ---
 tools/tests/vpci/emul.h| 6 +-
 tools/tests/x86_emulator/x86-emulate.h | 5 -
 tools/xenstore/list.h  | 6 ++
 5 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/tools/include/xen-tools/libs.h b/tools/include/xen-tools/libs.h
index bafc90e2f6..3672486daa 100644
--- a/tools/include/xen-tools/libs.h
+++ b/tools/include/xen-tools/libs.h
@@ -67,4 +67,8 @@
 #define __must_check __attribute__((__warn_unused_result__))
 #endif
 
+#define container_of(ptr, type, member) ({\
+typeof( ((type *)0)->member ) *__mptr = (ptr);\
+(type *)( (char *)__mptr - offsetof(type,member) );})
+
 #endif /* __XEN_TOOLS_LIBS__ */
diff --git a/tools/tests/vhpet/emul.h b/tools/tests/vhpet/emul.h
index b022cc0eab..7db47c5beb 100644
--- a/tools/tests/vhpet/emul.h
+++ b/tools/tests/vhpet/emul.h
@@ -125,9 +125,6 @@ enum
 #define max_t(type, x, y) \
 ({ type __x = (x); type __y = (y); __x > __y ? __x : __y; })
 #define offsetof(t, m) ((unsigned long )&((t *)0)->m)
-#define container_of(ptr, type, member) ({  \
-typeof( ((type *)0)->member ) *__mptr = (ptr);  \
-(type *)( (char *)__mptr - offsetof(type,member) ); })
 
 struct domain;
 
diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h
index f03e3a56d1..237edb4e95 100644
--- a/tools/tests/vpci/emul.h
+++ b/tools/tests/vpci/emul.h
@@ -27,11 +27,7 @@
 #include 
 #include 
 
-#define container_of(ptr, type, member) ({  \
-typeof(((type *)0)->member) *mptr = (ptr);  \
-\
-(type *)((char *)mptr - offsetof(type, member));\
-})
+#include 
 
 #define smp_wmb()
 #define prefetch(x) __builtin_prefetch(x)
diff --git a/tools/tests/x86_emulator/x86-emulate.h 
b/tools/tests/x86_emulator/x86-emulate.h
index 18ae40d017..0849c07dbc 100644
--- a/tools/tests/x86_emulator/x86-emulate.h
+++ b/tools/tests/x86_emulator/x86-emulate.h
@@ -56,11 +56,6 @@
 
 #define cf_check /* No Control Flow Integriy checking */
 
-#define container_of(ptr, type, member) ({ \
-typeof(((type *)0)->member) *mptr__ = (ptr);   \
-(type *)((char *)mptr__ - offsetof(type, member)); \
-})
-
 #define AC_(n,t) (n##t)
 #define _AC(n,t) AC_(n,t)
 
diff --git a/tools/xenstore/list.h b/tools/xenstore/list.h
index b17d13e0ec..1f469eafaf 100644
--- a/tools/xenstore/list.h
+++ b/tools/xenstore/list.h
@@ -3,6 +3,8 @@
 /* Taken from Linux kernel code, but de-kernelized for userspace. */
 #include 
 
+#include 
+
 #undef LIST_HEAD_INIT
 #undef LIST_HEAD
 #undef INIT_LIST_HEAD
@@ -15,10 +17,6 @@
 #define LIST_POISON1  ((void *) 0x00100100)
 #define LIST_POISON2  ((void *) 0x00200200)
 
-#define container_of(ptr, type, member) ({ \
-typeof( ((type *)0)->member ) *__mptr = (ptr); \
-(type *)( (char *)__mptr - offsetof(type,member) );})
-
 /*
  * Simple doubly linked list implementation.
  *
-- 
2.35.3

[PATCH 0/3] tools: use xen-tools/libs.h for common definitions

[Juergen Gross]

There are some macros defined multiple times in tools. Use only
xen-tools/libs.h for defining those macros and drop the copies.

Juergen Gross (3):
  tools: add container_of() macro to xen-tools/libs.h
  tools: get rid of additional min() and max() definitions
  tools: add offsetof() to xen-tools/libs.h

 tools/firmware/hvmloader/util.h| 11 ++-
 tools/firmware/include/stddef.h|  4 ++--
 tools/include/xen-tools/libs.h |  8 
 tools/libfsimage/Rules.mk  |  2 ++
 tools/libfsimage/xfs/fsys_xfs.c|  4 +---
 tools/libs/vchan/init.c|  7 +--
 tools/tests/vhpet/emul.h   | 14 --
 tools/tests/vpci/emul.h| 22 +-
 tools/tests/x86_emulator/x86-emulate.h |  5 -
 tools/xenstore/list.h  |  6 ++
 10 files changed, 19 insertions(+), 64 deletions(-)

-- 
2.35.3

Re: [PATCH v3 00/14] x86/hvm: {svm,vmx} {c,h} cleanup

[Andrew Cooper]

On 27/02/2023 11:33 am, Jan Beulich wrote:
> On 27.02.2023 12:15, Andrew Cooper wrote:
>> On 27/02/2023 10:46 am, Jan Beulich wrote:
>>> On 24.02.2023 22:33, Andrew Cooper wrote:
 But I think we want to change tact slightly at this point, so I'm not
 going to do any further tweaking on commit.

 Next, I think we want to rename asm/hvm/svm/svm.h to asm/hvm/svm.h,
 updating the non-SVM include paths as we go.  Probably best to
 chain-include the other svm/hvm/svm/*.h headers temporarily, so we've
 only got one tree-wide cleanup of the external include paths.

 Quick tangent - I will be making all of that cpu_has_svm_*
 infrastructure disappear by moving it into the normal CPUID handling,
 but I've not had sufficient time to finish that yet.

 Next, hvm/svm/nestedsvm.h can merge straight into hvm/svm.h, and
 disappear (after my decoupling patch has gone in).
>>> Why would you want to fold hvm/svm/nestedsvm.h into hvm/svm/svm.h?
>>> The latter doesn't use anything from the former, does it?
>> It's about what else uses them.
>>
>> hvm_vcpu needs both svm_vcpu and nestedsvm, so both headers are always
>> included in tandem.
> Well, yes, that's how things are today. But can you explain to me why
> hvm_vcpu has to know nestedsvm's layout?

Because it's part of the same single memory allocation.

> If the field was a pointer,
> a forward decl of that struct would suffice, and any entity in the
> rest of Xen not caring about struct nestedsvm would no longer see it
> (and hence also no longer be re-built if a change is made there).

Yes, you could hide it as a pointer.  The cost of doing so is an
unnecessary extra memory allocation, and extra pointer handling on
create/destroy, not to mention extra pointer chasing in memory during use.

>> nestedsvm is literally just one struct now, and no subsystem ought to
>> have multiple headers when one will do.
> When one will do, yes. Removing build dependencies is a good reason
> to have separate headers, though.

Its not the only only option, and an #ifdef CONFIG_NESTED_VIRT inside
the struct would be an equally acceptable way of doing this which
wouldn't involve making an extra memory allocation.


Everything you've posed here is way out of scope for Xenia's series. 
You are welcome to try and make the changes in the future if you want,
but you're going to have a uphill battle trying to justify it as a
sensible move.

~Andrew

Re: [PATCH 1/2] docs/about: Deprecate 32-bit x86 hosts and qemu-system-i386

[Daniel P . Berrangé]

On Mon, Feb 27, 2023 at 12:10:49PM +0100, Thomas Huth wrote:
> Hardly anybody still uses 32-bit x86 hosts today, so we should
> start deprecating them to finally have less test efforts.
> With regards to 32-bit KVM support in the x86 Linux kernel,
> the developers confirmed that they do not need a recent
> qemu-system-i386 binary here:
> 
>  https://lore.kernel.org/kvm/y%2ffkts5ajfy0h...@google.com/
> 
> Signed-off-by: Thomas Huth 
> ---
>  docs/about/deprecated.rst | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 15084f7bea..98517f5187 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -196,6 +196,19 @@ CI coverage support may bitrot away before the 
> deprecation process
>  completes. The little endian variants of MIPS (both 32 and 64 bit) are
>  still a supported host architecture.
>  
> +32-bit x86 hosts and ``qemu-system-i386`` (since 8.0)
> +'
> +
> +Testing 32-bit x86 host OS support takes a lot of precious time during the
> +QEMU contiguous integration tests, and considering that most OS vendors
> +stopped shipping 32-bit variants of their x86 OS distributions and most
> +x86 hardware from the past >10 years is capable of 64-bit, keeping the
> +32-bit support alive is an inadequate burden for the QEMU project. Thus
> +QEMU will soon drop the support for 32-bit x86 host systems and the
> +``qemu-system-i386`` binary. Use ``qemu-system-x86_64`` (which is a proper
> +superset of ``qemu-system-i386``) on a 64-bit host machine instead.

I feel like we should have separate deprecation entries for the
i686 host support, and for qemu-system-i386 emulator binary, as
although they're related they are independant features with
differing impact. eg removing qemu-system-i386 affects all
host architectures, not merely 32-bit x86 host, so I think we
can explain the impact more clearly if we separate them.

How about something like


32-bit x86 hosts


Support for 32-bit x86 host deployments is increasingly uncommon in
mainstream Linux distributions given the widespread availability of
64-bit x86 hardware. The QEMU project no longer considers 32-bit
x86 support to be an effective use of its limited resources, and
thus intends to discontinue it.

Current users of QEMU on 32-bit x86 hosts should either continue
using existing releases of QEMU, with the caveat that they will
no longer get security fixes, or migrate to a 64-bit platform
which remains capable of running 32-bit guests if needed.

``qemu-system-i386`` binary removal
'''

The ``qemu-system-x86_64`` binary can be used to run 32-bit guests
by selecting a 32-bit CPU model, including KVM support on x86_64
hosts. Once support for the 32-bit x86 host platform is discontinued,
the ``qemu-system-i386`` binary will be redundant. Current users are
recommended to reconfigure their systems to use the ``qemu-system-x86_64``
binary.



Same point for the next patch about 32-bit arm vs qemu-system-arm
binary.

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

S0ix support in Xen

[Simon Gaiser]

Hi,

I have been looking into using S0ix with Xen. On systems with with 11th
gen (Tiger Lake) Intel mobile CPUs or newer this is often the only
supported suspend method, thus we want to support it in Qubes OS.

Below a summary of my current understanding of what's needed (and known
unknowns). I would appreciate some feedback (what's missing, preferred
solutions, etc.).

Note this topic is much above my previous experience with Xen and x86
power management internals, so sorry if I'm missing things that are
obvious to you.

PIT timer: During some previous private discussion it was mentioned that
the PIT timer that Xen initializes for IO-APIC testing prevents S0ix
residency and therefore that part needs to be reworked. But if I'm
reading the current code correctly Xen can already use the HPET timer
instead, either with an automatic fallback if PIT is unavailable or by
forcing it via hpet=legacy-replacement=1. Looking at the rest I think
the PIT isn't used if Xen finds another clocksource. Did I miss
something?

mwait idle driver: While mwait-idle.c share a lot of code with Linux's
intel_idle.c and the imported code seems to have been updated (for
example for Alder Lake) it only supports the CPUs with hardcoded
cstates. Linux's code also has a code path to read the cstate config
from ACPI if the driver doesn't has a hard coded config for the model.
This is needed for example for Tiger Lake. For my current testing I
added the values the Linux code reads from ACPI by hand. But that's of
course no proper solution. How should this be handled in Xen (IIUC some
ACPI things are handled by Xen and some by dom0)?

The debugging code in xen/arch/x86/acpi/cpu_idle.c (and maybe other
places) need to learn about deeper package C states, for example for
Tiger Lake.

In general having the power management debugging available that you have
under plain Linux through /sys/kernel/debug/pmc_core, etc. would be
nice. Some things are as easy as allowing some dom0 to read some MSRs.
But didn't looked into it further, some functions might also be required
more complex integration (like extending xenpm's functionality).

I'm not sure yet is what else in Xen needs to learn about S0ix. Running
domains need to be halted, that can be handled by the toolstack. What
other Xen internal things need to be aware of S0ix? Like avoiding
unnecessary timer wakeups or similar. That's currently my biggest
unknown and it would be great if someone with more insight and overview
could give some hints here.

On my test system (NUC11TNHi5, Tiger Lake) I haven't seen it reaching
cstates lower PC7 with Xen (so it can of course not reach S0ix
residency). With plain Linux I got it working on that system although it
was rather fiddly and probably something is fishy on the firmware side.
After seeing it very occasionally not working under plain Linux I have
installed a newer firmware version. With that Xen currently doesn't wake
up after triggering s2idle in dom0. I'm currently looking into that and
will follow up if I have more details on that part.

Thanks, Simon


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH v2] x86/svm: Decouple types in struct nestedsvm

[Andrew Cooper]

On 27/02/2023 11:41 am, Jan Beulich wrote:
> On 27.02.2023 12:35, Andrew Cooper wrote:
>> struct nestedvm uses mostly plain integer types, except for virt_ext_t which
>> is a union wrapping two bitfield names.
>>
>> However, it turns out that this is a write-only variable.  Delete it, 
>> allowing
>> us to drop the include of vmcb.h
>>
>> Signed-off-by: Andrew Cooper 
>> ---
>> CC: Jan Beulich 
>> CC: Xenia Ragiadakou 
>>
>> v2:
>>  * Delete the variable entirely.
>>
>> This probably means that nested lbr/vmloadsave is broken, but that wouldn't 
>> be
>> a surprise at all.
> Well, yes, short of figuring out what's missing this is probably the least
> bad option:
> Reviewed-by: Jan Beulich 

Thanks.

I'm pretty certain a working version of nested virt won't need this
cached information like this at all.  I'm pretty sure it's buggy not to
be referencing the appropriate one of the 3 relevant VMCBs.

~Andrew

Re: [PATCH v2] x86/svm: Decouple types in struct nestedsvm

[Jan Beulich]

On 27.02.2023 12:35, Andrew Cooper wrote:
> struct nestedvm uses mostly plain integer types, except for virt_ext_t which
> is a union wrapping two bitfield names.
> 
> However, it turns out that this is a write-only variable.  Delete it, allowing
> us to drop the include of vmcb.h
> 
> Signed-off-by: Andrew Cooper 
> ---
> CC: Jan Beulich 
> CC: Xenia Ragiadakou 
> 
> v2:
>  * Delete the variable entirely.
> 
> This probably means that nested lbr/vmloadsave is broken, but that wouldn't be
> a surprise at all.

Well, yes, short of figuring out what's missing this is probably the least
bad option:
Reviewed-by: Jan Beulich 

Jan

Re: [PATCH] xen/riscv: read hart_id and dtb_base passed by bootloader

[Jan Beulich]

On 27.02.2023 12:19, Oleksii wrote:
> On Mon, 2023-02-27 at 10:17 +0100, Jan Beulich wrote:
>> On 24.02.2023 17:36, Oleksii wrote:
>>> On Fri, 2023-02-24 at 16:04 +0100, Jan Beulich wrote:
 On 24.02.2023 15:48, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko 
> ---
>  xen/arch/riscv/setup.c | 12 
>  1 file changed, 12 insertions(+)
>
> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> index b3f8b10f71..154bf3a0bc 100644
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -26,6 +26,18 @@ static void test_macros_from_bug_h(void)
>  
>  void __init noreturn start_xen(void)
>  {
> +    /*
> + * The following things are passed by bootloader:
> + *   a0 -> hart_id
> + *   a1 -> dtb_base
> +    */
> +    register unsigned long hart_id  asm("a0");
> +    register unsigned long dtb_base asm("a1");
> +
> +    asm volatile( "mv %0, a0" : "=r" (hart_id) );
> +
> +    asm volatile( "mv %0, a1" : "=r" (dtb_base) );

 Are you sure this (a) works and (b) is what you want? You're
 inserting
>>> Oh, yeah... it should be:
>>>   unsigned long hart_id;
>>>   unsigned long dtb_base;
>>
>> As per below - no, I don't think so. I continue to think these want
>> to be function parameters.
>>
>>> I did experiments with 'register' and 'asm()' and after rebase of
>>> my
>>> internal branches git backed these changes...
>>>
>>> Sorry for that I have to double check patches next time.
>>>
>>> It looks like it works only because the variables aren't used
>>> anywhere.
 "mov a0, a0" and "mov a1, a1". I suppose as long as the two local
 variables aren't used further down in the function, the compiler
 will
 be able to recognize both registers as dead, and hence use them
 for
 argument passing to later functions that you call. But I would
 expect
 that to break once you actually consume either of the variables.

 Fundamentally I think this is the kind of thing you want do to in
 assembly. However, in the specific case here, can't you simply
 have

 void __init noreturn start_xen(unsigned long hard_id,
    unsigned long dtb_base)
 {
     ...

 and all is going to be fine, without any asm()?
>>> One of the things that I would like to do is to not use an
>>> assembler as
>>> much as possible. And as we have C environment ready after a few
>>> assembly instructions in head.S I thought it would be OK to do it
>>> in C
>>> code somewhere at the start so someone/sonething doesn't alter
>>> register
>>> a0 and a1.
>>
>> Avoiding assembly code where possible if of course appreciated,
>> because
>> generally C code is easier to maintain. But of course this can only
>> be
>> done if things can be expressed correctly. And you need to keep in
>> mind
>> that asm() statements also are assembly code, just lower volume.
>>
> Let's get hard_id and dtb_base in head.S and pass them as arguments of
> start_xen() function as you mentioned before.

Still looks like a misunderstanding to me. Aiui a0 and a1 are the
registers to hold first and second function arguments each. If
firmware places the two values in these two registers, then
start_xen(), with the adjusted declaration, will fit the purpose
without any assembly code.

Jan