[linux-linus test] 178042: tolerable trouble: fail/pass/starved - PUSHED
flight 178042 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/178042/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 177979 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 177979 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 177979 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 177979 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 177979 test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-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-libvirt 1 build-check(1) starved n/a build-armhf 2 hosts-allocate starved n/a version targeted for testing: linux3f0b0903fde584a7398f82fc00bf4f8138610b87 baseline version: linux89f5349e0673322857bd432fa23113af56673739 Last test of basis 177979 2023-02-21 03:27:38 Z1 days Testing same since 178042 2023-02-21 17:44:43 Z0 days1 attempts People who touched revisions under test: Andrew Halaney # sa8540p-ride Ashok Raj Babu Moger Borislav Petkov (AMD) Borislav Petkov Brian Gerst Guangju Wang[baidu] Ingo Molnar Li Zhang Linus Torvalds Manivannan Sadhasivam Peter Zijlstra (Intel) Peter Zijlstra Qiuxu Zhuo Reinette Chatre Sai Krishna Potthuri Sebastian Andrzej Siewior Shuah Khan Shubhrajyoti Datta Shubhrajyoti Datta Smita Koralahalli Steev Klimaszewski # Thinkpad X13s Thomas Gleixner Tony Luck Yazen Ghannam Youquan Song jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf starved build-i386 pass build-amd64-libvirt pass build-arm64-libvirt pass build-armhf-libvirt starved build-i386-libvirt pass
Re: [GIT PULL] xen: branch for v6.3-rc1
The pull request you sent on Sun, 19 Feb 2023 06:33:26 +0100: > git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git > for-linus-6.3-rc1-tag has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/239451e90355be68130410ef8fadef8cd130a35d Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html
[xen-unstable test] 178027: regressions - trouble: fail/pass/starved
flight 178027 xen-unstable real [real] flight 178060 xen-unstable real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/178027/ http://logs.test-lab.xenproject.org/osstest/logs/178060/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-xl-vhd 21 guest-start/debian.repeat fail REGR. vs. 177972 Tests which are failing intermittently (not blocking): test-amd64-amd64-xl-qemuu-win7-amd64 12 windows-install fail pass in 178060-retest Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stop fail in 178060 like 177972 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 177972 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 177972 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 177972 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 177972 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 177972 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 177972 test-amd64-amd64-libvirt-vhd 19 guest-start/debian.repeatfail like 177972 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 177972 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 177972 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-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-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-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-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-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 a90a0018f605e3bb0588816e5a1f957d6e4562eb baseline version: xen 406cea1970535cd7b9d6bcf09bc164ef9bb64bac Last test of basis 177972 2023-02-21 01:54:03 Z0 days Testing same since 178027 2023-02-21 14:38:42 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Xenia Ragiadakou jobs: build-amd64-xsm pass build-arm64-xsm pass
Re: [PATCH v2 08/13] tools/xenstore: add accounting trace support
Hi Juergen,
On 21/02/2023 08:40, Juergen Gross wrote:
On 20.02.23 23:57, Julien Grall wrote:
Hi Juergen,
On 20/01/2023 10:00, Juergen Gross wrote:
Add a new trace switch "acc" and the related trace calls.
The "acc" switch is off per default.
Signed-off-by: Juergen Gross
With one reamrk (see below):
Reviewed-by: Julien Grall
---
tools/xenstore/xenstored_core.c | 2 +-
tools/xenstore/xenstored_core.h | 1 +
tools/xenstore/xenstored_domain.c | 10 ++
3 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/tools/xenstore/xenstored_core.c
b/tools/xenstore/xenstored_core.c
index 6ef60179fa..558ef491b1 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2746,7 +2746,7 @@ static void set_quota(const char *arg, bool soft)
/* Sorted by bit values of TRACE_* flags. Flag is (1u << index). */
const char *const trace_switches[] = {
- "obj", "io", "wrl",
+ "obj", "io", "wrl", "acc",
NULL
};
diff --git a/tools/xenstore/xenstored_core.h
b/tools/xenstore/xenstored_core.h
index 1f811f38cb..3e0734a6c6 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -302,6 +302,7 @@ extern unsigned int trace_flags;
#define TRACE_OBJ 0x0001
#define TRACE_IO 0x0002
#define TRACE_WRL 0x0004
+#define TRACE_ACC 0x0008
extern const char *const trace_switches[];
int set_trace_switch(const char *arg);
diff --git a/tools/xenstore/xenstored_domain.c
b/tools/xenstore/xenstored_domain.c
index b1e29edb7e..d461fd8cc8 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -538,6 +538,12 @@ static struct domain
*find_domain_by_domid(unsigned int domid)
return (d && d->introduced) ? d : NULL;
}
+#define trace_acc(...) \
The indentation of '\' looks odd.
Not for me. Maybe you have a different tab setting?
I only looked at the code from my mail client. In my editor, it looks OK.
Cheers,
--
Julien Grall
Re: [PATCH v2 04/13] tools/xenstore: add framework to commit accounting data on success only
Hi Juergen,
On 21/02/2023 08:37, Juergen Gross wrote:
On 20.02.23 23:50, Julien Grall wrote:
+ list_del(>list);
+ talloc_free(cd);
+ }
+}
+
+void acc_commit(struct connection *conn)
+{
+ struct changed_domain *cd;
+ struct buffered_data *in = conn->in;
+ enum accitem what;
+
+ conn->in = NULL; /* Avoid recursion. */
I am not sure I understand this comment. Can you provide more details
where the recursion would happen?
domain_acc_add() might do temporary accounting if conn->in isn't NULL.
I am confused. To me recursion means the function (or the caller) will
call itself. But here you seem to say you just want to avoid temporary
accounting.
What did I miss?
+ while ((cd = list_top(>acc_list, struct changed_domain,
list))) {
NIT: You could use list_for_each_safe();
Like above.
+ list_del(>list);
+ for (what = 0; what < ACC_REQ_N; what++)
There is a chance that some compiler will complain about this line
because ACC_REQ_N = 0. So this would always be true. Therefore...
Huh? It would always be false.
Yes false sorry. This doesn't change about the potential (temporary)
warning.
+ if (cd->acc[what])
+ domain_acc_add(conn, cd->domid, what,
+ cd->acc[what], true);
+
+ talloc_free(cd);
+ }
+
+ conn->in = in;
+}
+
int domain_nbentry_inc(struct connection *conn, unsigned int domid)
{
return (domain_acc_add(conn, domid, ACC_NODES, 1, false) < 0)
diff --git a/tools/xenstore/xenstored_domain.h
b/tools/xenstore/xenstored_domain.h
index 8259c114b0..cd85bd0cad 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -20,7 +20,8 @@
#define _XENSTORED_DOMAIN_H
enum accitem {
- ACC_NODES,
+ ACC_REQ_N, /* Number of elements per request and domain. */
+ ACC_NODES = ACC_REQ_N,
I would bring forward the change in patch #5. I mean:
ACC_NODES,
ACC_REQ_N
I'm not sure this is a good idea. This would activate the temporary
accounting for nodes, but keeping the error handling active. I'd rather
activate temporary accounting for nodes together with removing the
accounting correction in the error handling.
I am not sure I fully understand what you would rather do. Can you clarify?
ACC_TR_N, /* Number of elements per transaction and
domain. */
ACC_N = ACC_TR_N /* Number of elements per domain. */
};
This enum is getting extremely confusing. There are many "number of
elements per ... domain". Can you clarify?
I will add some more comments to the header. Would you like it better to
have
the limits indented more? something like:
I am afraid it doesn't help understanding the comment. For instance,
enum accitem {
ACC_NODES,
/* Number of elements per request and domain. */
you wrote "per request and domain" here. But...
ACC_REQ_N,
/* Number of elements per transaction and domain. */
... here this is "per transaction and domain". Should not nbe "elements
per transaction"? And if not, then why don't we had "per request,
transaction and domain" above?
ACC_TR_N = ACC_REQ_N,
ACC_WATCH = ACC_TR_N,
ACC_OUTST,
ACC_MEM,
ACC_TRANS,
ACC_TRANSNODES,
ACC_NPERM,
ACC_PATHLEN,
ACC_NODESZ,
/* Number of elements per domain. */
ACC_N
};
Juergen
Cheers,
--
Julien Grall
Re: [PATCH v2 01/13] tools/xenstore: don't allow creating too many nodes in a transaction
Hi Juergen, On 21/02/2023 08:10, Juergen Gross wrote: On 20.02.23 19:01, Julien Grall wrote: So I have recreated an XTF test which I think match what you wrote [1]. It is indeed failing without your patch. But then there are still some weird behavior here. I would expect the creation of the node would also fail if instead of removing the node if removed outside of the transaction. This is not the case because we are looking at the current quota. So shouldn't we snapshot the global count? As we don't do a global snapshot of the data base for a transaction (this was changed due to huge memory needs, bad performance, and a higher transaction failure rate), I am a bit surprised that the only way to do proper transaction is to have a global snapshot. Instead, you could have an overlay. I don't think we should snapshot the count either. But that would mean that the quota will change depending on modification of the global database while the transaction is inflight. I guess this is not better nor worse that the current situation. But it is still really confusing for a client because: 1) The error could happen at random point 2) You may see an inconsistent database as nodes are only cached when they are first accessed A transaction is performed atomically at the time it is finished. Therefore seeing the current global state inside the transaction (with the transaction private state on top like an overlay) is absolutely fine IMO. To me it is just showing that our concept of transaction is very broken in C Xenstored. I am curious to know whether OXenstored is behaving the same way. Anyway, I agree this is not better nor worse than the current situation. So I will acked this patch: Acked-by: Julien Grall Cheers, -- Julien Grall
Re: [PATCH v4 2/2] x86/ucode/AMD: late load the patch on every logical thread
On Tue, Feb 21, 2023 at 2:03 PM Jan Beulich wrote: > > On 15.02.2023 16:38, Sergey Dyasli wrote: > > --- a/xen/arch/x86/cpu/microcode/core.c > > +++ b/xen/arch/x86/cpu/microcode/core.c > > @@ -398,10 +398,16 @@ static int cf_check microcode_nmi_callback( > > (!ucode_in_nmi && cpu == primary) ) > > return 0; > > > > -if ( cpu == primary ) > > +if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) > > Given their origin, I'm pretty certain Hygon wants treating the same here > and below. Hygon? ucode_ops is currently initialised only for Amd and Intel. Speaking of which, I'm thinking about adding a new function is_cpu_primary() there. This would make the core code much cleaner. I'll see if I can make it work. Thanks, Sergey
[xen-unstable-smoke test] 178044: tolerable trouble: pass/starved - PUSHED
flight 178044 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/178044/ 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 c76e4ff7d531ee2bf37c9d4037c8645f9e586fcd baseline version: xen d58f3941ce3f8943df842fab2d4d761c483af6c4 Last test of basis 178030 2023-02-21 15:00:25 Z0 days Testing same since 178044 2023-02-21 18:00:27 Z0 days1 attempts People who touched revisions under test: Andrew Cooper 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 d58f3941ce..c76e4ff7d5 c76e4ff7d531ee2bf37c9d4037c8645f9e586fcd -> smoke
Re: [PATCH 1/2] xen/ioapic: Don't use local_irq_restore() to disable irqs
On 21/02/2023 1:40 pm, Jan Beulich wrote:
> On 20.02.2023 20:47, Andrew Cooper wrote:
>> Despite its name, the irq{save,restore}() APIs are only intended to
>> conditionally disable and re-enable interrupts.
> Are they?
Yes, absolutely.
And as said before, the potentially dubious naming does not give us
permission or the right to interpret the behaviour differently.
> Maybe nowadays they indeed are, but I couldn't spot any wording
> to this effect in Linux'es Documentation/ (and I don't think we have
> anywhere such could be put). Yet I'd expect such a statement to be backed
> by something.
It is backed by the rude words the owners of this API had to say on
discovering this particular use.
Conditionally enabling with a construct like this is bogus everywhere.
It is only ever safe to enable irqs if you know exactly why they were
disabled previously, and that can never be the case with a construct
like this.
It only reason this one example doesn't explode is because its an init
path not nested within an irq/irqsave lock or other critical region.
>
>> IO-APIC's timer_irq_works() violates this intention. As it is init code,
>> switch to simple irq enable/disable().
> If all callers of the function obviously did have interrupts off, I might
> agree. But the last of them sits _after_ local_irq_restore(), and all of
> this is called from underneath smp_prepare_cpus()
Which do you mean "the last of them" ?
There is a large amount of genuinely dead logic here.
~Andrew
Debian randconfig failure, Was Re: [XEN PATCH v2 0/7] automation: Update containers to allow HTTPS access to xenbits
On 21/02/2023 4:55 pm, Anthony PERARD wrote: > Building randconfig on debian unstable seems to be an issue. You're talking about https://gitlab.com/xen-project/people/anthonyper/xen/-/jobs/3769926509 ? + gcc --version gcc (Debian 12.2.0-14) 12.2.0 arch/x86/extable.c: In function 'search_pre_exception_table': arch/x86/extable.c:200:27: error: array subscript -1 is outside array bounds of 'struct exception_table_entry[1152921504606846975]' [-Werror=array-bounds] 200 | unsigned long fixup = search_one_extable( | ^~~ 201 | __start___pre_ex_table, __stop___pre_ex_table-1, addr); | ~~ In file included from arch/x86/extable.c:8: ./arch/x86/include/asm/uaccess.h:414:37: note: at offset -8 into object '__stop___pre_ex_table' of size [0, 9223372036854775807] 414 | extern struct exception_table_entry __stop___pre_ex_table[]; | ^ cc1: all warnings being treated as errors make[3]: *** [Rules.mk:246: arch/x86/extable.o] Error 1 Jan: do we need some more gcc-wrap sprinkled around? ~Andrew
Re: [XEN PATCH v2 6/7] automation: Remove testing on Debian Jessie
On 21/02/2023 5:59 pm, Andrew Cooper wrote:
> On 21/02/2023 4:55 pm, Anthony PERARD wrote:
>> Jessie as rearch EOL in 2020.
>>
>> Even if we update the containers, we would still not be able to reach
>> HTTPS webside with Let's Encrypt certificates and thus would need more
>> change to the container.
>>
>> Signed-off-by: Anthony PERARD
> How is this interact with the other patches in the series?
>
> I presume we do want to take patch 4 and rebuild the containers, for the
> older branches. And that's fine.
>
> And IMO we should be dropping jessie testing, so this is almost fine for
> staging.
>
> Except, jessie-32 is the only x86-32 build test we've got, so I think we
> want to replace it with a newer container before dropping the jessie*.
Further to this, I really don't think we need to have a 4-wide matrix of
{clang,gcc}{debug,release} for just a 32bit tools userspace. Debug
clang+gcc will do, and save on some testing cycles.
~Andrew
>
>> ---
>> Notes:
>> HTTPS would fail unless we commit "automation: Remove expired root
>> certificates used to be used by let's encrypt", that is. Patch still in
>> the series, and fix Jessie.
> If we're dropping the jessie containers, do we really need that change
> too? Because we really shouldn't be playing around with URLs on older
> branches.
>
> ~Andrew
Re: [XEN PATCH v2 6/7] automation: Remove testing on Debian Jessie
On 21/02/2023 4:55 pm, Anthony PERARD wrote: > Jessie as rearch EOL in 2020. > > Even if we update the containers, we would still not be able to reach > HTTPS webside with Let's Encrypt certificates and thus would need more > change to the container. > > Signed-off-by: Anthony PERARD How is this interact with the other patches in the series? I presume we do want to take patch 4 and rebuild the containers, for the older branches. And that's fine. And IMO we should be dropping jessie testing, so this is almost fine for staging. Except, jessie-32 is the only x86-32 build test we've got, so I think we want to replace it with a newer container before dropping the jessie*. > --- > Notes: > HTTPS would fail unless we commit "automation: Remove expired root > certificates used to be used by let's encrypt", that is. Patch still in > the series, and fix Jessie. If we're dropping the jessie containers, do we really need that change too? Because we really shouldn't be playing around with URLs on older branches. ~Andrew
Re: [XEN PATCH v2 2/7] automation: Ensure that all packages are up-to-dates in CentOS 7 container
On 21/02/2023 4:55 pm, Anthony PERARD wrote: > This was prompt by the fact that `wget https://xenbits.xenproject.org` > fails with expired certificates, which turned out to be an expired > root certificates. Updating all packages fix the issue. > > Signed-off-by: Anthony PERARD Acked-by: Andrew Cooper
Re: [XEN PATCH v2 1/7] automation: Remove CentOS 7.2 containers and builds
On 21/02/2023 4:55 pm, Anthony PERARD wrote: > We already have a container which track the latest CentOS 7, no need > for this one as well. > > Also, 7.2 have outdated root certificate which prevent connection to > website which use Let's Encrypt. > > Signed-off-by: Anthony PERARD Acked-by: Andrew Cooper For posterity, this container exists because Doug wanted something which was roughly a XenServer build environment, but we've long since moved on. ~Andrew
Re: [XEN PATCH v2 3/7] automation: Remove clang-8 from Debian unstable container
On 21/02/2023 4:55 pm, Anthony PERARD wrote: > First, apt complain that it isn't the right way to add keys anymore, > but hopefully that's just a warning. > > Second, we can't install clang-8: > The following packages have unmet dependencies: > clang-8 : Depends: libstdc++-8-dev but it is not installable >Depends: libgcc-8-dev but it is not installable >Depends: libobjc-8-dev but it is not installable >Recommends: llvm-8-dev but it is not going to be installed >Recommends: libomp-8-dev but it is not going to be installed > libllvm8 : Depends: libffi7 (>= 3.3~20180313) but it is not installable > E: Unable to correct problems, you have held broken packages. > > clang on Debian unstable is now version 14.0.6. > > Signed-off-by: Anthony PERARD > Acked-by: Andrew Cooper > --- > > This change will break "staging-*" branches as they would still test > clang-8. So patch needs to be backported. (at least build.yaml change) Well - it means we should backport this to all trees before rebuilding the container. Which is fine, but we need to be aware of this going in. CC'ing Jan just so he's aware. ~Andrew
Re: [PATCH 2/4] x86/svm: cleanup svm.h
On 21/02/2023 7:58 am, Xenia Ragiadakou wrote:
>
> On 2/21/23 01:08, Andrew Cooper wrote:
>> On 17/02/2023 6:48 pm, Xenia Ragiadakou wrote:
>>> Remove the forward declaration of struct vcpu because it is not used.
>>
>> Huh, turns out that was my fault in c/s b158e72abe, shortly after I
>> introduced them in the first place.
>>
>> Also, looking into that, there's one legitimate use of svm.h from
>> outside, which is svm_load_segs*() which means we can't make all the
>> headers be local.
>>
>> But still, most of svm.h shouldn't be includable in the rest of Xen.
>> Perhaps we can make a separate dedicated header for just this.
>>
>> [edit] And svm_{doman,vcpu}. Perhaps we want a brand new
>> include/asm/hvm/svm.h with only the things needed elsewhere.
>
> This can be done as part of the series aiming to make svm/vmx support
> configurable.
Ok, that's fine.
Honestly, there's a lot of cleanup which can be done. We probably want
to end up making a number of $foo-types.h headers so we can construct
struct vcpu/domain without xen/sched.h including the majority of Xen in
one fell swoop.
>
>>
>> That said, we do need to stea^W borrow adaptive PLE, and make the
>
> I cannot understand what do you mean by "stea^W borrow adaptive PLE".
Pause Loop Exiting is tricky to get right.
The common expectation is that PLE hits in a spinlock or other mutual
exclusion primitive.
It is generally good to let the vCPU spin for a bit, in the expectation
that the other vCPU holding lock will release it in a timely fashion.
Spinning for a few iterations (even a few hundred) is far lower overhead
than taking a vmexit.
But if the other vCPU isn't executing, it can never release the lock,
and letting the current vCPU spin does double damage because it consumes
the domain's scheduler credit, which in turn pushes out the reschedule
of the vCPU that's actually holding the lock. (This is why paravirt
spinlocks are so useful in virt. If you get them right, you end up with
only the vcpus that can usefully do work to release a lock executing.)
For overall system performance, there is a tradeoff between how long you
let a vCPU spin, and when it's better to force such a vCPU to yield.
This point depends on the typical spinlock contention inside the guest,
and the overall system busyness, both of which vary over time.
Picking fixed numbers for PLE is better than not having PLE in the first
place, but only just. There is an algorithm called adaptive-PLE which
tries to balance the PLE settings over time to optimise overall system
performance.
~Andrew
Re: [PATCH linux-next 2/2] x86/xen/time: cleanup xen_tsc_safe_clocksource
On Tue, Feb 21, 2023 at 09:47:24AM +0100, Juergen Gross wrote:
> On 21.02.23 06:51, Krister Johansen wrote:
> > On Mon, Feb 20, 2023 at 08:14:40PM -0800, Krister Johansen wrote:
> > > On Mon, Feb 20, 2023 at 11:01:18PM +0100, Thomas Gleixner wrote:
> > > > On Mon, Feb 20 2023 at 09:17, Krister Johansen wrote:
> > > > > @@ -495,8 +496,7 @@ static int __init xen_tsc_safe_clocksource(void)
> > > > > /* Leaf 4, sub-leaf 0 (0x4x03) */
> > > > > cpuid_count(xen_cpuid_base() + 3, 0, , , , );
> > > > > - /* tsc_mode = no_emulate (2) */
> > > > > - if (ebx != 2)
> > > > > + if (ebx != XEN_CPUID_TSC_MODE_NEVER_EMULATE)
> > > > > return 0;
> > > > > return 1;
> > > >
> > > > What about removing more stupidity from that function?
> > > >
> > > > static bool __init xen_tsc_safe_clocksource(void)
> > > > {
> > > > u32 eax, ebx. ecx, edx;
> > > > /* Leaf 4, sub-leaf 0 (0x4x03) */
> > > > cpuid_count(xen_cpuid_base() + 3, 0, , , , );
> > > >
> > > > return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
> > > > }
> > >
> > > I'm all for simplifying. I'm happy to clean up that return to be more
> > > idiomatic. I was under the impression, perhaps mistaken, though, that
> > > the X86_FEATURE_CONSTANT_TSC, X86_FEATURE_NONSTOP_TSC, and
> > > check_tsc_unstable() checks were actually serving a purpose: to ensure
> > > that we don't rely on the tsc in environments where it's being emulated
> > > and the OS would be better served by using a PV clock. Specifically,
> > > kvmclock_init() makes a very similar set of checks that I also thought
> > > were load-bearing.
> >
> > Bah, what I meant to say was emulated, unstable, or otherwise unsuitable
> > for use as a clocksource. IOW, even if TSC_MODE_NEVER_EMULATE is
> > set, it's possible that a user is attempting a migration from a cpu
> > that's not invariant, and we'd still want to check for that case and
> > fall back to a PV clocksource, correct?
>
> But Thomas' suggestion wasn't changing any behavior compared to your
> patch. It just makes it easier to read.
>
> If you are unsure your patch is correct, please verify the correctness
> before sending it.
Thanks, and apologies. I misunderstood and thought a more complex change
was requested.
-K
Re: [PATCH linux-next 2/2] x86/xen/time: cleanup xen_tsc_safe_clocksource
On Tue, Feb 21, 2023 at 10:14:54AM +0100, Thomas Gleixner wrote:
> On Mon, Feb 20 2023 at 21:51, Krister Johansen wrote:
> > On Mon, Feb 20, 2023 at 08:14:40PM -0800, Krister Johansen wrote:
> >> > static bool __init xen_tsc_safe_clocksource(void)
> >> > {
> >> > u32 eax, ebx. ecx, edx;
> >> >
> >> > /* Leaf 4, sub-leaf 0 (0x4x03) */
> >> > cpuid_count(xen_cpuid_base() + 3, 0, , , , );
> >> >
> >> > return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
> >> > }
> >>
> >> I'm all for simplifying. I'm happy to clean up that return to be more
> >> idiomatic. I was under the impression, perhaps mistaken, though, that
> >> the X86_FEATURE_CONSTANT_TSC, X86_FEATURE_NONSTOP_TSC, and
> >> check_tsc_unstable() checks were actually serving a purpose: to ensure
> >> that we don't rely on the tsc in environments where it's being emulated
> >> and the OS would be better served by using a PV clock. Specifically,
> >> kvmclock_init() makes a very similar set of checks that I also thought
> >> were load-bearing.
> >
> > Bah, what I meant to say was emulated, unstable, or otherwise unsuitable
> > for use as a clocksource. IOW, even if TSC_MODE_NEVER_EMULATE is
> > set, it's possible that a user is attempting a migration from a cpu
> > that's not invariant, and we'd still want to check for that case and
> > fall back to a PV clocksource, correct?
>
> Sure. But a life migration from a NEVER_EMULATE to a non-invariant host
> is a patently bad idea and has nothing to do with the __init function,
> which is gone at that point already.
>
> What I wanted to say:
>
> static bool __init xen_tsc_safe_clocksource(void)
> {
> ..
>
> /* Leaf 4, sub-leaf 0 (0x4x03) */
> cpuid_count(xen_cpuid_base() + 3, 0, , , , );
>
> return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
> }
Thanks, I'm happy to make it look like ^ that. I should have thought to
do this myself. :/
I'll send out a v2 making this correction.
> I didn't have the full context and was just looking at the condition.
> Now I checked the full context and I think that except for the
>
> if (check_tsc_unstable())
>
> check everything else can go away unless you do not trust the hypervisor
> that it only sets the NEVER_EMULATE bit when CONSTANT and NONSTOP are
> set as well. But yeah, you might prefer to be paranoid. It's virt after
> all.
Unless there are objections, I think I'd prefer to err on the side of
paranoia here. Sorry for the confusion.
-K
[xen-unstable-smoke test] 178030: tolerable trouble: pass/starved - PUSHED
flight 178030 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/178030/ 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 d58f3941ce3f8943df842fab2d4d761c483af6c4 baseline version: xen a90a0018f605e3bb0588816e5a1f957d6e4562eb Last test of basis 178016 2023-02-21 12:03:32 Z0 days Testing same since 178030 2023-02-21 15:00:25 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Jan Beulich Sergey Dyasli 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 a90a0018f6..d58f3941ce d58f3941ce3f8943df842fab2d4d761c483af6c4 -> smoke
[XEN PATCH v2 3/7] automation: Remove clang-8 from Debian unstable container
First, apt complain that it isn't the right way to add keys anymore, but hopefully that's just a warning. Second, we can't install clang-8: The following packages have unmet dependencies: clang-8 : Depends: libstdc++-8-dev but it is not installable Depends: libgcc-8-dev but it is not installable Depends: libobjc-8-dev but it is not installable Recommends: llvm-8-dev but it is not going to be installed Recommends: libomp-8-dev but it is not going to be installed libllvm8 : Depends: libffi7 (>= 3.3~20180313) but it is not installable E: Unable to correct problems, you have held broken packages. clang on Debian unstable is now version 14.0.6. Signed-off-by: Anthony PERARD Acked-by: Andrew Cooper --- This change will break "staging-*" branches as they would still test clang-8. So patch needs to be backported. (at least build.yaml change) Current container have: root@f3d1fc4f58c7:/build# clang --version clang version 8.0.1- (branches/release_80) root@113cb5730b2a:/build# clang-8 --version clang version 8.0.1- (branches/release_80) (built about 3years ago) --- automation/build/debian/unstable-llvm-8.list | 3 --- automation/build/debian/unstable.dockerfile | 12 automation/gitlab-ci/build.yaml | 10 -- 3 files changed, 25 deletions(-) delete mode 100644 automation/build/debian/unstable-llvm-8.list diff --git a/automation/build/debian/unstable-llvm-8.list b/automation/build/debian/unstable-llvm-8.list deleted file mode 100644 index dc119fa0b4..00 --- a/automation/build/debian/unstable-llvm-8.list +++ /dev/null @@ -1,3 +0,0 @@ -# Unstable LLVM 8 repos -deb http://apt.llvm.org/unstable/ llvm-toolchain-8 main -deb-src http://apt.llvm.org/unstable/ llvm-toolchain-8 main diff --git a/automation/build/debian/unstable.dockerfile b/automation/build/debian/unstable.dockerfile index 9de766d596..b560337b7a 100644 --- a/automation/build/debian/unstable.dockerfile +++ b/automation/build/debian/unstable.dockerfile @@ -51,15 +51,3 @@ RUN apt-get update && \ apt-get autoremove -y && \ apt-get clean && \ rm -rf /var/lib/apt/lists* /tmp/* /var/tmp/* - -RUN wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key|apt-key add - -COPY unstable-llvm-8.list /etc/apt/sources.list.d/ - -RUN apt-get update && \ -apt-get --quiet --yes install \ -clang-8 \ -lld-8 \ -&& \ -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 aed8fc0240..22ce1c45e7 100644 --- a/automation/gitlab-ci/build.yaml +++ b/automation/gitlab-ci/build.yaml @@ -329,16 +329,6 @@ debian-unstable-clang-debug: variables: CONTAINER: debian:unstable -debian-unstable-clang-8: - extends: .clang-8-x86-64-build - variables: -CONTAINER: debian:unstable - -debian-unstable-clang-8-debug: - extends: .clang-8-x86-64-build-debug - variables: -CONTAINER: debian:unstable - debian-unstable-gcc: extends: .gcc-x86-64-build variables: -- Anthony PERARD
[XEN PATCH v2 5/7] automation: Add more aliases in containerize
Signed-off-by: Anthony PERARD
Acked-by: Andrew Cooper
---
automation/scripts/containerize | 3 +++
1 file changed, 3 insertions(+)
diff --git a/automation/scripts/containerize b/automation/scripts/containerize
index 9e508918bf..9b1a302d05 100755
--- a/automation/scripts/containerize
+++ b/automation/scripts/containerize
@@ -33,9 +33,12 @@ case "_${CONTAINER}" in
_fedora) CONTAINER="${BASE}/fedora:29";;
_focal) CONTAINER="${BASE}/ubuntu:focal" ;;
_jessie) CONTAINER="${BASE}/debian:jessie" ;;
+_jessie-i386) CONTAINER="${BASE}/debian:jessie-i386" ;;
_stretch|_) CONTAINER="${BASE}/debian:stretch" ;;
+_stretch-i386) CONTAINER="${BASE}/debian:stretch-i386" ;;
_buster-gcc-ibt) CONTAINER="${BASE}/debian:buster-gcc-ibt" ;;
_unstable|_) CONTAINER="${BASE}/debian:unstable" ;;
+_unstable-i386) CONTAINER="${BASE}/debian:unstable-i386" ;;
_unstable-arm32-gcc) CONTAINER="${BASE}/debian:unstable-arm32-gcc" ;;
_unstable-arm64v8) CONTAINER="${BASE}/debian:unstable-arm64v8" ;;
_trusty) CONTAINER="${BASE}/ubuntu:trusty" ;;
--
Anthony PERARD
[XEN PATCH v2 4/7] automation: Use EOL tag for Jessie container
As Jessie is EOL, the official tag isn't supported anymore. Also, the GPG key for the packages on the repository on the official image are expired and it isn't possible to update or install packages. But we can use the image from "debian/eol" tag which use repositories from archive.debian.org and have workaround to ignore the validity date of the keys. There isn't a dedicated i386 tag for jessie, but we can ask docker to pull the i386 image of the "debial/eol:jessie" tag. Signed-off-by: Anthony PERARD --- Notes: v2: - new patch, this replace "automation: Ignore package authentification issue in Jessie container" workaround I've seen in the debian/eol:jessie: 'Acquire::Check-Valid-Until "false";' in /etc/apt/apt.conf.d/ And a script to replace the "gpgv" binary used by apt, which check that the only issue with a signature is that the key has expired. automation/build/debian/jessie-i386.dockerfile | 2 +- automation/build/debian/jessie.dockerfile | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/automation/build/debian/jessie-i386.dockerfile b/automation/build/debian/jessie-i386.dockerfile index 3f86d91f63..276b640ec9 100644 --- a/automation/build/debian/jessie-i386.dockerfile +++ b/automation/build/debian/jessie-i386.dockerfile @@ -1,4 +1,4 @@ -FROM i386/debian:jessie +FROM --platform=linux/i386 debian/eol:jessie LABEL maintainer.name="The Xen Project" \ maintainer.email="xen-devel@lists.xenproject.org" diff --git a/automation/build/debian/jessie.dockerfile b/automation/build/debian/jessie.dockerfile index 2f19adcad3..06128d1a40 100644 --- a/automation/build/debian/jessie.dockerfile +++ b/automation/build/debian/jessie.dockerfile @@ -1,4 +1,4 @@ -FROM debian:jessie +FROM debian/eol:jessie LABEL maintainer.name="The Xen Project" \ maintainer.email="xen-devel@lists.xenproject.org" -- Anthony PERARD
[XEN PATCH v2 6/7] automation: Remove testing on Debian Jessie
Jessie as rearch EOL in 2020. Even if we update the containers, we would still not be able to reach HTTPS webside with Let's Encrypt certificates and thus would need more change to the container. Signed-off-by: Anthony PERARD --- Notes: HTTPS would fail unless we commit "automation: Remove expired root certificates used to be used by let's encrypt", that is. Patch still in the series, and fix Jessie. automation/gitlab-ci/build.yaml | 40 - 1 file changed, 40 deletions(-) diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml index 22ce1c45e7..2be1b05d5c 100644 --- a/automation/gitlab-ci/build.yaml +++ b/automation/gitlab-ci/build.yaml @@ -221,46 +221,6 @@ centos-7-gcc-debug: variables: CONTAINER: centos:7 -debian-jessie-clang: - extends: .clang-x86-64-build - variables: -CONTAINER: debian:jessie - -debian-jessie-clang-debug: - extends: .clang-x86-64-build-debug - variables: -CONTAINER: debian:jessie - -debian-jessie-gcc: - extends: .gcc-x86-64-build - variables: -CONTAINER: debian:jessie - -debian-jessie-gcc-debug: - extends: .gcc-x86-64-build-debug - variables: -CONTAINER: debian:jessie - -debian-jessie-32-clang: - extends: .clang-x86-32-build - variables: -CONTAINER: debian:jessie-i386 - -debian-jessie-32-clang-debug: - extends: .clang-x86-32-build-debug - variables: -CONTAINER: debian:jessie-i386 - -debian-jessie-32-gcc: - extends: .gcc-x86-32-build - variables: -CONTAINER: debian:jessie-i386 - -debian-jessie-32-gcc-debug: - extends: .gcc-x86-32-build-debug - variables: -CONTAINER: debian:jessie-i386 - debian-stretch-clang: extends: .clang-x86-64-build variables: -- Anthony PERARD
[XEN PATCH v2 7/7] automation: Remove expired root certificates used to be used by let's encrypt
While the Let's Encrypt root certificate ISRG_Root_X1.crt is already present, openssl seems to still check for the root certificate DST_Root_CA_X3.crt which has expired. This prevent https connections. Removing DST_Root_CA_X3 fix the issue. Signed-off-by: Anthony PERARD --- Notes: v2: - remove unneeded changes to CentOS containers automation/build/debian/jessie-i386.dockerfile | 5 + automation/build/debian/jessie.dockerfile | 5 + automation/build/ubuntu/trusty.dockerfile | 5 + 3 files changed, 15 insertions(+) diff --git a/automation/build/debian/jessie-i386.dockerfile b/automation/build/debian/jessie-i386.dockerfile index 276b640ec9..e04b43f32f 100644 --- a/automation/build/debian/jessie-i386.dockerfile +++ b/automation/build/debian/jessie-i386.dockerfile @@ -49,3 +49,8 @@ RUN apt-get update && \ apt-get autoremove -y && \ apt-get clean && \ rm -rf /var/lib/apt/lists* /tmp/* /var/tmp/* + +# Remove expired certificate that Let's Encrypt certificates used to relie on. +# (Not needed anymore) +RUN sed -i '/mozilla\/DST_Root_CA_X3\.crt/d' /etc/ca-certificates.conf && \ +update-ca-certificates diff --git a/automation/build/debian/jessie.dockerfile b/automation/build/debian/jessie.dockerfile index 06128d1a40..e8aa0183ee 100644 --- a/automation/build/debian/jessie.dockerfile +++ b/automation/build/debian/jessie.dockerfile @@ -48,3 +48,8 @@ RUN apt-get update && \ apt-get autoremove -y && \ apt-get clean && \ rm -rf /var/lib/apt/lists* /tmp/* /var/tmp/* + +# Remove expired certificate that Let's Encrypt certificates used to relie on. +# (Not needed anymore) +RUN sed -i '/mozilla\/DST_Root_CA_X3\.crt/d' /etc/ca-certificates.conf && \ +update-ca-certificates diff --git a/automation/build/ubuntu/trusty.dockerfile b/automation/build/ubuntu/trusty.dockerfile index b4b2f85e73..16d08ca931 100644 --- a/automation/build/ubuntu/trusty.dockerfile +++ b/automation/build/ubuntu/trusty.dockerfile @@ -49,3 +49,8 @@ RUN apt-get update && \ apt-get autoremove -y && \ apt-get clean && \ rm -rf /var/lib/apt/lists* /tmp/* /var/tmp/* + +# Remove expired certificate that Let's Encrypt certificates used to relie on. +# (Not needed anymore) +RUN sed -i 's#mozilla/DST_Root_CA_X3\.crt#!\0#' /etc/ca-certificates.conf && \ +update-ca-certificates -- Anthony PERARD
[XEN PATCH v2 1/7] automation: Remove CentOS 7.2 containers and builds
We already have a container which track the latest CentOS 7, no need for this one as well. Also, 7.2 have outdated root certificate which prevent connection to website which use Let's Encrypt. Signed-off-by: Anthony PERARD --- Notes: v2: - new patch automation/build/centos/7.2.dockerfile | 52 - automation/build/centos/CentOS-7.2.repo | 35 - automation/gitlab-ci/build.yaml | 10 - 3 files changed, 97 deletions(-) delete mode 100644 automation/build/centos/7.2.dockerfile delete mode 100644 automation/build/centos/CentOS-7.2.repo diff --git a/automation/build/centos/7.2.dockerfile b/automation/build/centos/7.2.dockerfile deleted file mode 100644 index 4baa097e31..00 --- a/automation/build/centos/7.2.dockerfile +++ /dev/null @@ -1,52 +0,0 @@ -FROM centos:7.2.1511 -LABEL maintainer.name="The Xen Project" \ - maintainer.email="xen-devel@lists.xenproject.org" - -# ensure we only get bits from the vault for -# the version we want -COPY CentOS-7.2.repo /etc/yum.repos.d/CentOS-Base.repo - -# install EPEL for dev86, xz-devel and possibly other packages -RUN yum -y install https://dl.fedoraproject.org/pub/epel/epel-release-latest-7.noarch.rpm && \ -yum clean all - -RUN mkdir /build -WORKDIR /build - -# work around https://github.com/moby/moby/issues/10180 -# and install Xen depends -RUN rpm --rebuilddb && \ -yum -y install \ -yum-plugin-ovl \ -gcc \ -gcc-c++ \ -ncurses-devel \ -zlib-devel \ -openssl-devel \ -python-devel \ -libuuid-devel \ -pkgconfig \ -# gettext for Xen < 4.13 -gettext \ -flex \ -bison \ -libaio-devel \ -glib2-devel \ -yajl-devel \ -pixman-devel \ -glibc-devel \ -# glibc-devel.i686 for Xen < 4.15 -glibc-devel.i686 \ -make \ -binutils \ -git \ -wget \ -acpica-tools \ -python-markdown \ -patch \ -checkpolicy \ -dev86 \ -xz-devel \ -bzip2 \ -nasm \ -&& yum clean all diff --git a/automation/build/centos/CentOS-7.2.repo b/automation/build/centos/CentOS-7.2.repo deleted file mode 100644 index 4da27faeb5..00 --- a/automation/build/centos/CentOS-7.2.repo +++ /dev/null @@ -1,35 +0,0 @@ -# CentOS-Base.repo -# -# This is a replacement file that pins things to just use CentOS 7.2 -# from the CentOS Vault. -# - -[base] -name=CentOS-7.2.1511 - Base -baseurl=http://vault.centos.org/7.2.1511/os/$basearch/ -gpgcheck=1 -gpgkey=file:///etc/pki/rpm-gpg/RPM-GPG-KEY-CentOS-7 - -#released updates -[updates] -name=CentOS-7.2.1511 - Updates -baseurl=http://vault.centos.org/7.2.1511/updates/$basearch/ -gpgcheck=1 -gpgkey=file:///etc/pki/rpm-gpg/RPM-GPG-KEY-CentOS-7 - -#additional packages that may be useful -[extras] -name=CentOS-7.2.1511 - Extras -baseurl=http://vault.centos.org/7.2.1511/extras/$basearch/ -gpgcheck=1 -gpgkey=file:///etc/pki/rpm-gpg/RPM-GPG-KEY-CentOS-7 - -#additional packages that extend functionality of existing packages -[centosplus] -name=CentOS-7.2.1511 - Plus -baseurl=http://vault.centos.org/7.2.1511/centosplus/$basearch/ -gpgcheck=1 -gpgcheck=1 -enabled=0 -gpgkey=file:///etc/pki/rpm-gpg/RPM-GPG-KEY-CentOS-7 - diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml index 079e9b73f6..aed8fc0240 100644 --- a/automation/gitlab-ci/build.yaml +++ b/automation/gitlab-ci/build.yaml @@ -211,16 +211,6 @@ archlinux-gcc-debug: variables: CONTAINER: archlinux:current -centos-7-2-gcc: - extends: .gcc-x86-64-build - variables: -CONTAINER: centos:7.2 - -centos-7-2-gcc-debug: - extends: .gcc-x86-64-build-debug - variables: -CONTAINER: centos:7.2 - centos-7-gcc: extends: .gcc-x86-64-build variables: -- Anthony PERARD
[XEN PATCH v2 2/7] automation: Ensure that all packages are up-to-dates in CentOS 7 container
This was prompt by the fact that `wget https://xenbits.xenproject.org` fails with expired certificates, which turned out to be an expired root certificates. Updating all packages fix the issue. Signed-off-by: Anthony PERARD --- Notes: v2: - new patch, this replace a change in "Remove expired root certificates used to be used by let's encrypt" automation/build/centos/7.dockerfile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/automation/build/centos/7.dockerfile b/automation/build/centos/7.dockerfile index e688a4cece..f5264e02d9 100644 --- a/automation/build/centos/7.dockerfile +++ b/automation/build/centos/7.dockerfile @@ -15,7 +15,8 @@ RUN rpm --rebuilddb && \ rm -rf /var/cache/yum # install Xen depends -RUN yum -y install \ +RUN yum -y update \ +&& yum -y install \ gcc \ gcc-c++ \ ncurses-devel \ -- Anthony PERARD
[XEN PATCH v2 0/7] automation: Update containers to allow HTTPS access to xenbits
Patch series available in this git branch: https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git br.gitlab-containers-update-v2 v2: - Remove CentOS 7.2 - Remove Debian Jessie test, but update container recipe for the benefit of older branches. - Fix CentOS 7 containner recipe to update all packages. (Fix missing update of HTTPS root certificates) There is work in progress [1] to update urls in our repo to use https, but those https urls to xenbits don't work in our containers, due to an expired root certificate. So we need to update those containers. This series update the dockerfile where just rebuilding the container isn't enough. I've tested the new containers but didn't updated them yet. You can see the result in the following links (if you can access them). There are other containers been test which didn't need dockerfile update. (That was with v1 of the series) https://gitlab.com/xen-project/people/anthonyper/xen/-/pipelines/777474218 https://gitlab.com/xen-project/people/anthonyper/xen/-/pipelines/778218868 Building randconfig on debian unstable seems to be an issue. [1] https://lore.kernel.org/xen-devel/75d91def8234bceb41548147ee8af5fea52bd1d6.1675889602.git.d...@invisiblethingslab.com/ Cheers, Anthony PERARD (7): automation: Remove CentOS 7.2 containers and builds automation: Ensure that all packages are up-to-dates in CentOS 7 container automation: Remove clang-8 from Debian unstable container automation: Use EOL tag for Jessie container automation: Add more aliases in containerize automation: Remove testing on Debian Jessie automation: Remove expired root certificates used to be used by let's encrypt automation/build/centos/7.2.dockerfile| 52 automation/build/centos/7.dockerfile | 3 +- automation/build/centos/CentOS-7.2.repo | 35 --- .../build/debian/jessie-i386.dockerfile | 7 ++- automation/build/debian/jessie.dockerfile | 7 ++- automation/build/debian/unstable-llvm-8.list | 3 - automation/build/debian/unstable.dockerfile | 12 automation/build/ubuntu/trusty.dockerfile | 5 ++ automation/gitlab-ci/build.yaml | 60 --- automation/scripts/containerize | 3 + 10 files changed, 22 insertions(+), 165 deletions(-) delete mode 100644 automation/build/centos/7.2.dockerfile delete mode 100644 automation/build/centos/CentOS-7.2.repo delete mode 100644 automation/build/debian/unstable-llvm-8.list -- Anthony PERARD
[libvirt test] 177984: tolerable trouble: pass/starved - PUSHED
flight 177984 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/177984/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 15 saverestore-support-checkfail never pass test-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-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass build-armhf-libvirt 1 build-check(1) starved n/a test-armhf-armhf-libvirt-raw 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 build-armhf 2 hosts-allocate starved n/a version targeted for testing: libvirt f0c1ce43824f049bb3dea056d4302132d47d24e8 baseline version: libvirt 15e5eb8a7684992d1a885038d28781462a727bf2 Last test of basis 177679 2023-02-18 04:20:11 Z3 days Testing same since 177984 2023-02-21 04:18:51 Z0 days1 attempts People who touched revisions under test: Andrea Bolognani Michal Privoznik Peter Krempa Temuri Doghonadze jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf starved build-i386 pass build-amd64-libvirt pass build-arm64-libvirt pass build-armhf-libvirt starved build-i386-libvirt pass build-amd64-pvopspass build-arm64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass test-amd64-amd64-libvirt-xsm pass test-arm64-arm64-libvirt-xsm pass test-amd64-i386-libvirt-xsm pass test-amd64-amd64-libvirt pass test-arm64-arm64-libvirt pass test-armhf-armhf-libvirt starved test-amd64-i386-libvirt pass test-amd64-amd64-libvirt-pairpass test-amd64-i386-libvirt-pair pass test-arm64-arm64-libvirt-qcow2 pass test-armhf-armhf-libvirt-qcow2 starved test-arm64-arm64-libvirt-raw pass test-armhf-armhf-libvirt-raw starved test-amd64-i386-libvirt-raw pass test-amd64-amd64-libvirt-vhd pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at
Re: [PATCH v2] x86: Perform mem_sharing teardown before paging teardown
On Tue, Feb 21, 2023 at 8:54 AM Jan Beulich wrote:
>
> On 15.02.2023 18:07, Tamas K Lengyel wrote:
> > An assert failure has been observed in p2m_teardown when performing vm
> > forking and then destroying the forked VM (p2m-basic.c:173). The assert
> > checks whether the domain's shared pages counter is 0. According to the
> > patch that originally added the assert (7bedbbb5c31) the p2m_teardown
> > should only happen after mem_sharing already relinquished all shared
pages.
> >
> > In this patch we flip the order in which relinquish ops are called to
avoid
> > tripping the assert.
> >
> > Signed-off-by: Tamas K Lengyel
> > Fixes: e7aa55c0aab3 ("x86/p2m: free the paging memory pool
preemptively")
>
> Especially considering the backporting requirement I'm disappointed to
> find that you haven't extended the description to explain why this
> heavier code churn is to be preferred over an adjustment to the offending
> assertion. As said in reply to v1 - I'm willing to accept arguments
> towards this being better, but I need to know those arguments.
The assert change as you proposed is hard to understand and will be thus
harder to maintain. Conceptually sharing being torn down makes sense to
happen before paging is torn down. Considering that's how it has been
before the unfortunate regression I'm fixing here I don't think more
justification is needed. The code-churn is minimal anyway.
Tamas
Re: [PATCH] livepatch-build: Check compiler version matches
On 21/02/2023 2:14 pm, Ross Lagerwall wrote:
> For reliable live patch generation, the compiler version used should
> match the original binary. Check that this is the case and add a
> --skip-compiler-check option to override this.
>
> Signed-off-by: Ross Lagerwall
> ---
> livepatch-build | 54 +++--
> 1 file changed, 39 insertions(+), 15 deletions(-)
>
> diff --git a/livepatch-build b/livepatch-build
> index 91d203b..e4b4dba 100755
> --- a/livepatch-build
> +++ b/livepatch-build
> @@ -33,6 +33,7 @@ DEPENDS=
> XEN_DEPENDS=
> PRELINK=
> STRIP=0
> +SKIP_COMPILER_CHECK=0
> XENSYMS=xen-syms
>
> warn() {
> @@ -266,27 +267,44 @@ function create_patch()
> objcopy --set-section-flags .livepatch.xen_depends=alloc,readonly
> "${PATCHNAME}.livepatch"
> }
>
> +check_compiler() {
> +orig_ver=$(readelf -p .comment "$XENSYMS" | grep -o 'GCC.*')
This rather breaks Clang as a toolchain, but it doesn't seem to be the
only GCC-expectation in livepatch build tools.
$ readelf -p .comment xen-syms
String dump of section '.comment':
[ 0] Debian clang version 11.0.1-2
Irritatingly, while clang* --version always reports itself as "clang
version ..." matching the .ident it writes out, gcc* substitutes argv[0]
into it's --version. But the way the Xen build is invoked, I think Xen
will always substituent cc for gcc, so this may not be a problem.
A build of Xen should only use a single compiler, so I think you're
better off looking for s/[ 0] \(.*\)/\1/ rather than assuming that
GCC was used.
Also, I think you should error out if we can't identify a compiler,
because very little good will come from trying to proceed.
~Andrew
[xen-unstable-smoke test] 178016: tolerable trouble: pass/starved - PUSHED
flight 178016 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/178016/ 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 a90a0018f605e3bb0588816e5a1f957d6e4562eb baseline version: xen 406cea1970535cd7b9d6bcf09bc164ef9bb64bac Last test of basis 177843 2023-02-19 19:00:26 Z1 days Testing same since 178016 2023-02-21 12:03:32 Z0 days1 attempts People who touched revisions under test: Andrew Cooper 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 406cea1970..a90a0018f6 a90a0018f605e3bb0588816e5a1f957d6e4562eb -> smoke
[PATCH] livepatch-build: Check compiler version matches
For reliable live patch generation, the compiler version used should
match the original binary. Check that this is the case and add a
--skip-compiler-check option to override this.
Signed-off-by: Ross Lagerwall
---
livepatch-build | 54 +++--
1 file changed, 39 insertions(+), 15 deletions(-)
diff --git a/livepatch-build b/livepatch-build
index 91d203b..e4b4dba 100755
--- a/livepatch-build
+++ b/livepatch-build
@@ -33,6 +33,7 @@ DEPENDS=
XEN_DEPENDS=
PRELINK=
STRIP=0
+SKIP_COMPILER_CHECK=0
XENSYMS=xen-syms
warn() {
@@ -266,27 +267,44 @@ function create_patch()
objcopy --set-section-flags .livepatch.xen_depends=alloc,readonly
"${PATCHNAME}.livepatch"
}
+check_compiler() {
+orig_ver=$(readelf -p .comment "$XENSYMS" | grep -o 'GCC.*')
+
+in_file=$(mktemp --suffix=.c)
+out_file=$(mktemp --suffix=.o)
+echo 'int main(void) {}' > "$in_file"
+gcc -c -o "$out_file" "$in_file"
+new_ver=$(readelf -p .comment "$out_file" | grep -o 'GCC.*')
+
+rm -f "$infile" "$outfile"
+
+if [ "$orig_ver" != "$new_ver" ]; then
+die "Mismatched compiler version: Original \"$orig_ver\" New
\"$new_ver\""
+fi
+}
+
usage() {
echo "usage: $(basename $0) [options]" >&2
-echo "-h, --help Show this help message" >&2
-echo "-s, --srcdir Xen source directory" >&2
-echo "-p, --patchPatch file" >&2
-echo "-c, --config .config file" >&2
-echo "-o, --output Output directory" >&2
-echo "-j, --cpus Number of CPUs to use" >&2
-echo "-k, --skip Skip build or diff phase" >&2
-echo "-d, --debugEnable debug logging" >&2
-echo "--xen-debugBuild debug Xen (if your .config does not
have the options)" >&2
-echo "--xen-syms Build against a xen-syms" >&2
-echo "--depends Required build-id" >&2
-echo "--xen-depends Required Xen build-id" >&2
-echo "--prelink Prelink" >&2
-echo "--stripRemove all symbols that are not needed
for relocation processing." >&2
+echo "-h, --helpShow this help message" >&2
+echo "-s, --srcdir Xen source directory" >&2
+echo "-p, --patch Patch file" >&2
+echo "-c, --config .config file" >&2
+echo "-o, --output Output directory" >&2
+echo "-j, --cpusNumber of CPUs to use" >&2
+echo "-k, --skipSkip build or diff phase" >&2
+echo "-d, --debug Enable debug logging" >&2
+echo "--xen-debug Build debug Xen (if your .config does
not have the options)" >&2
+echo "--xen-symsBuild against a xen-syms" >&2
+echo "--depends Required build-id" >&2
+echo "--xen-depends Required Xen build-id" >&2
+echo "--prelink Prelink" >&2
+echo "--strip Remove all symbols that are not needed
for relocation processing." >&2
+echo "--skip-compiler-check Skip compiler version check." >&2
}
find_tools || die "can't find supporting tools"
-options=$(getopt -o hs:p:c:o:j:k:d -l
"help,srcdir:,patch:,config:,output:,cpus:,skip:,debug,xen-debug,xen-syms:,depends:,xen-depends:,prelink,strip"
-- "$@") || die "getopt failed"
+options=$(getopt -o hs:p:c:o:j:k:d -l
"help,srcdir:,patch:,config:,output:,cpus:,skip:,debug,xen-debug,xen-syms:,depends:,xen-depends:,prelink,strip,skip-compiler-check"
-- "$@") || die "getopt failed"
eval set -- "$options"
@@ -358,6 +376,10 @@ while [[ $# -gt 0 ]]; do
STRIP=1
shift
;;
+--skip-compiler-check)
+SKIP_COMPILER_CHECK=1
+shift
+;;
--)
shift
break
@@ -383,6 +405,8 @@ OUTPUT="$(readlink -m -- "$outputarg")"
[ -f "${PATCHFILE}" ] || die "Patchfile does not exist"
[ -f "${CONFIGFILE}" ] || die ".config does not exist"
+[ -f "$XENSYMS" ] && [ "$SKIP_COMPILER_CHECK" -eq 0 ] && check_compiler
+
PATCHNAME=$(make_patch_name "${PATCHFILE}")
echo "Building LivePatch patch: ${PATCHNAME}"
--
2.31.1
Re: [PATCH v4 2/2] x86/ucode/AMD: late load the patch on every logical thread
On 15.02.2023 16:38, Sergey Dyasli wrote:
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -398,10 +398,16 @@ static int cf_check microcode_nmi_callback(
> (!ucode_in_nmi && cpu == primary) )
> return 0;
>
> -if ( cpu == primary )
> +if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
Given their origin, I'm pretty certain Hygon wants treating the same here
and below.
> +/* load ucode on every logical thread/core */
> ret = primary_thread_work(nmi_patch);
> else
> -ret = secondary_nmi_work();
> +{
> +if ( cpu == primary )
> +ret = primary_thread_work(nmi_patch);
> +else
> +ret = secondary_nmi_work();
> +}
May I ask to get away with less code churn and less overall indentation?
Already
if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
/* load ucode on every logical thread/core */
ret = primary_thread_work(nmi_patch);
else if ( cpu == primary )
ret = primary_thread_work(nmi_patch);
else
ret = secondary_nmi_work();
would be shorter, but I don't see anything wrong with simply going with
if ( cpu == primary ||
/* Load ucode on every logical thread/core: */
(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
ret = primary_thread_work(nmi_patch);
else
ret = secondary_nmi_work();
Same again further down, I believe.
Jan
Re: [PATCH v4 1/2] x86/ucode/AMD: apply the patch early on every logical thread
On 15.02.2023 16:38, Sergey Dyasli wrote:
> The original issue has been reported on AMD Bulldozer-based CPUs where
> ucode loading loses the LWP feature bit in order to gain the IBPB bit.
> LWP disabling is per-SMT/CMT core modification and needs to happen on
> each sibling thread despite the shared microcode engine. Otherwise,
> logical CPUs will end up with different cpuid capabilities.
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216211
>
> Guests running under Xen happen to be not affected because of levelling
> logic for the feature masking/override MSRs which causes the LWP bit to
> fall out and hides the issue. The latest recommendation from AMD, after
> discussing this bug, is to load ucode on every logical CPU.
>
> In Linux kernel this issue has been addressed by e7ad18d1169c
> ("x86/microcode/AMD: Apply the patch early on every logical thread").
> Follow the same approach in Xen.
>
> Introduce SAME_UCODE match result and use it for early AMD ucode
> loading. Take this opportunity and move opt_ucode_allow_same out of
> compare_revisions() to the relevant callers and also modify the warning
> message based on it. Intel's side of things is modified for consistency
> but provides no functional change.
>
> Signed-off-by: Sergey Dyasli
Reviewed-by: Jan Beulich
Re: [PATCH v2] x86: Perform mem_sharing teardown before paging teardown
On 15.02.2023 18:07, Tamas K Lengyel wrote:
> An assert failure has been observed in p2m_teardown when performing vm
> forking and then destroying the forked VM (p2m-basic.c:173). The assert
> checks whether the domain's shared pages counter is 0. According to the
> patch that originally added the assert (7bedbbb5c31) the p2m_teardown
> should only happen after mem_sharing already relinquished all shared pages.
>
> In this patch we flip the order in which relinquish ops are called to avoid
> tripping the assert.
>
> Signed-off-by: Tamas K Lengyel
> Fixes: e7aa55c0aab3 ("x86/p2m: free the paging memory pool preemptively")
Especially considering the backporting requirement I'm disappointed to
find that you haven't extended the description to explain why this
heavier code churn is to be preferred over an adjustment to the offending
assertion. As said in reply to v1 - I'm willing to accept arguments
towards this being better, but I need to know those arguments.
Jan
Re: [PATCH 2/2] x86/irq: Improve local_irq_restore() code generation and performance
On 20.02.2023 20:47, Andrew Cooper wrote:
> --- a/xen/arch/x86/include/asm/system.h
> +++ b/xen/arch/x86/include/asm/system.h
> @@ -267,13 +267,8 @@ static inline unsigned long
> array_index_mask_nospec(unsigned long index,
> })
> #define local_irq_restore(x) \
> ({ \
> -BUILD_BUG_ON(sizeof(x) != sizeof(long)); \
> -asm volatile ( "pushfq\n\t" \
> - "andq %0, (%%rsp)\n\t"\
> - "orq %1, (%%rsp)\n\t"\
> - "popfq" \
> - : : "i?r" ( ~X86_EFLAGS_IF ), \
> - "ri" ( (x) & X86_EFLAGS_IF ) ); \
> +if ( (x) & X86_EFLAGS_IF ) \
> +local_irq_enable(); \
> })
Without it being written down anywhere that IRQs cannot be turned off
this way, and without there being a reference to that documentation
in the description, this is introducing a plain bug; I'm sorry to say
it that way. With both of the above fulfilled I'd of course be happy
to see the improvement take effect.
Jan
Ping: [PATCH 1/6] x86/Hyper-V: use standard C types in hyperv-tlfs.h
On 09.02.2023 11:38, Jan Beulich wrote:
> This is the only file left with a use of an __s type coming from
> Linux. Since the file has been using an apparently random mix of all
> three classes of fixed-width types (__{s,u}, {s,u}, and
> {,u}int_t), consolidate this to use exclusively standard types.
>
> No functional change intended.
>
> Signed-off-by: Jan Beulich
Ping? (I'll wait a few more days, but I'm going to commit this eventually
with just Andrew's ack if no maintainer one arrives.)
Jan
> --- a/xen/arch/x86/include/asm/guest/hyperv-tlfs.h
> +++ b/xen/arch/x86/include/asm/guest/hyperv-tlfs.h
> @@ -283,11 +283,11 @@
> * Declare the MSR used to setup pages used to communicate with the
> hypervisor.
> */
> union hv_x64_msr_hypercall_contents {
> - u64 as_uint64;
> + uint64_t as_uint64;
> struct {
> - u64 enable:1;
> - u64 reserved:11;
> - u64 guest_physical_address:52;
> + uint64_t enable:1;
> + uint64_t reserved:11;
> + uint64_t guest_physical_address:52;
> };
> };
>
> @@ -295,11 +295,11 @@ union hv_x64_msr_hypercall_contents {
> * TSC page layout.
> */
> struct ms_hyperv_tsc_page {
> - volatile u32 tsc_sequence;
> - u32 reserved1;
> - volatile u64 tsc_scale;
> - volatile s64 tsc_offset;
> - u64 reserved2[509];
> + volatile uint32_t tsc_sequence;
> + uint32_t reserved1;
> + volatile uint64_t tsc_scale;
> + volatile int64_t tsc_offset;
> + uint64_t reserved2[509];
> };
>
> /*
> @@ -343,21 +343,21 @@ union hv_guest_os_id
> };
>
> struct hv_reenlightenment_control {
> - __u64 vector:8;
> - __u64 reserved1:8;
> - __u64 enabled:1;
> - __u64 reserved2:15;
> - __u64 target_vp:32;
> + uint64_t vector:8;
> + uint64_t reserved1:8;
> + uint64_t enabled:1;
> + uint64_t reserved2:15;
> + uint64_t target_vp:32;
> };
>
> struct hv_tsc_emulation_control {
> - __u64 enabled:1;
> - __u64 reserved:63;
> + uint64_t enabled:1;
> + uint64_t reserved:63;
> };
>
> struct hv_tsc_emulation_status {
> - __u64 inprogress:1;
> - __u64 reserved:63;
> + uint64_t inprogress:1;
> + uint64_t reserved:63;
> };
>
> #define HV_X64_MSR_HYPERCALL_ENABLE 0x0001
> @@ -442,10 +442,10 @@ enum HV_GENERIC_SET_FORMAT {
> #define HV_CLOCK_HZ (NSEC_PER_SEC/100)
>
> typedef struct _HV_REFERENCE_TSC_PAGE {
> - __u32 tsc_sequence;
> - __u32 res1;
> - __u64 tsc_scale;
> - __s64 tsc_offset;
> + uint32_t tsc_sequence;
> + uint32_t res1;
> + uint64_t tsc_scale;
> + int64_t tsc_offset;
> } HV_REFERENCE_TSC_PAGE, *PHV_REFERENCE_TSC_PAGE;
>
> /* Define the number of synthetic interrupt sources. */
> @@ -499,30 +499,30 @@ enum hv_message_type {
>
> /* Define synthetic interrupt controller message flags. */
> union hv_message_flags {
> - __u8 asu8;
> + uint8_t asu8;
> struct {
> - __u8 msg_pending:1;
> - __u8 reserved:7;
> + uint8_t msg_pending:1;
> + uint8_t reserved:7;
> };
> };
>
> /* Define port identifier type. */
> union hv_port_id {
> - __u32 asu32;
> + uint32_t asu32;
> struct {
> - __u32 id:24;
> - __u32 reserved:8;
> + uint32_t id:24;
> + uint32_t reserved:8;
> } u;
> };
>
> /* Define synthetic interrupt controller message header. */
> struct hv_message_header {
> - __u32 message_type;
> - __u8 payload_size;
> + uint32_t message_type;
> + uint8_t payload_size;
> union hv_message_flags message_flags;
> - __u8 reserved[2];
> + uint8_t reserved[2];
> union {
> - __u64 sender;
> + uint64_t sender;
> union hv_port_id port;
> };
> };
> @@ -531,7 +531,7 @@ struct hv_message_header {
> struct hv_message {
> struct hv_message_header header;
> union {
> - __u64 payload[HV_MESSAGE_PAYLOAD_QWORD_COUNT];
> + uint64_t payload[HV_MESSAGE_PAYLOAD_QWORD_COUNT];
> } u;
> };
>
> @@ -542,19 +542,19 @@ struct hv_message_page {
>
> /* Define timer message payload structure. */
> struct hv_timer_message_payload {
> - __u32 timer_index;
> - __u32 reserved;
> - __u64 expiration_time; /* When the timer expired */
> - __u64 delivery_time;/* When the message was delivered */
> + uint32_t timer_index;
> + uint32_t reserved;
> + uint64_t expiration_time; /* When the timer expired */
> + uint64_t delivery_time; /* When the message was delivered */
> };
>
> struct hv_nested_enlightenments_control {
> struct {
> - __u32 directhypercall:1;
> - __u32 reserved:31;
> + uint32_t directhypercall:1;
> + uint32_t reserved:31;
> } features;
> struct {
> - __u32 reserved;
> + uint32_t reserved;
Re: [PATCH 1/2] xen/ioapic: Don't use local_irq_restore() to disable irqs
On 20.02.2023 20:47, Andrew Cooper wrote:
> Despite its name, the irq{save,restore}() APIs are only intended to
> conditionally disable and re-enable interrupts.
Are they? Maybe nowadays they indeed are, but I couldn't spot any wording
to this effect in Linux'es Documentation/ (and I don't think we have
anywhere such could be put). Yet I'd expect such a statement to be backed
by something.
> IO-APIC's timer_irq_works() violates this intention. As it is init code,
> switch to simple irq enable/disable().
If all callers of the function obviously did have interrupts off, I might
agree. But the last of them sits _after_ local_irq_restore(), and all of
this is called from underneath smp_prepare_cpus(), i.e. after IRQs were
already enabled. I'm willing to believe that the local_irq_restore()
there really comes too early, but then, ...
> No functional change.
... in order for this to be true, that'll need fixing at the same time (or
in a prereq patch).
Jan
[linux-linus test] 177979: tolerable trouble: fail/pass/starved - PUSHED
flight 177979 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/177979/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 177860 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 177860 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 177860 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 177860 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 177860 test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-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-libvirt 1 build-check(1) starved n/a build-armhf 2 hosts-allocate starved n/a version targeted for testing: linux89f5349e0673322857bd432fa23113af56673739 baseline version: linuxc9c3395d5e3dcc6daee66c6908354d47bf98cb0c Last test of basis 177860 2023-02-19 22:42:48 Z1 days Failing since177951 2023-02-20 20:44:41 Z0 days2 attempts Testing same since 177979 2023-02-21 03:27:38 Z0 days1 attempts 482 people touched revisions under test, not listing them all jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf starved build-i386 pass build-amd64-libvirt pass build-arm64-libvirt pass build-armhf-libvirt starved build-i386-libvirt pass build-amd64-pvopspass build-arm64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-xl pass test-amd64-coresched-amd64-xlpass
Re: [PATCH v2] x86/MSI: use standard C types in structures/unions
On 21/02/2023 1:27 pm, Jan Beulich wrote: > Consolidate this to use exclusively standard types, and change > indentation style to Xen's there at the same time (the file already had > a mix of styles). > > While there > - switch boolean fields to use bool, > - drop the notion of big-endian bitfields being a thing on x86, > - drop the names for reserved fields, > - adjust the comment on "dest32". > > No functional change intended. > > Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper
[PATCH v2] x86/MSI: use standard C types in structures/unions
Consolidate this to use exclusively standard types, and change
indentation style to Xen's there at the same time (the file already had
a mix of styles).
While there
- switch boolean fields to use bool,
- drop the notion of big-endian bitfields being a thing on x86,
- drop the names for reserved fields,
- adjust the comment on "dest32".
No functional change intended.
Signed-off-by: Jan Beulich
---
v2: Make secondary adjustments ("While there ..." above).
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -66,15 +66,15 @@ struct msi_info {
};
struct msi_msg {
- union {
- u64 address; /* message address */
- struct {
- u32 address_lo; /* message address low 32 bits */
- u32 address_hi; /* message address high 32 bits */
- };
- };
- u32 data; /* 16 bits of msi message data */
- u32 dest32; /* used when Interrupt Remapping with EIM is
enabled */
+union {
+uint64_t address; /* message address */
+struct {
+uint32_t address_lo; /* message address low 32 bits */
+uint32_t address_hi; /* message address high 32 bits */
+};
+};
+uint32_t data;/* 16 bits of msi message data */
+uint32_t dest32; /* used when Interrupt Remapping is enabled */
};
struct irq_desc;
@@ -94,35 +94,35 @@ extern int pci_restore_msi_state(struct
extern int pci_reset_msix_state(struct pci_dev *pdev);
struct msi_desc {
- struct msi_attrib {
- __u8type; /* {0: unused, 5h:MSI, 11h:MSI-X} */
- __u8pos;/* Location of the MSI capability */
- __u8maskbit : 1;/* mask/pending bit supported ? */
- __u8is_64 : 1;/* Address size: 0=32bit 1=64bit */
- __u8host_masked : 1;
- __u8guest_masked : 1;
- __u16 entry_nr; /* specific enabled entry */
- } msi_attrib;
-
- bool irte_initialized;
- uint8_t gvec; /* guest vector. valid when pi_desc
isn't NULL */
- const struct pi_desc *pi_desc; /* pointer to posted descriptor */
-
- struct list_head list;
-
- union {
- void __iomem *mask_base;/* va for the entry in mask table */
- struct {
- unsigned int nvec;/* number of vectors*/
- unsigned int mpos;/* location of mask register*/
- } msi;
- unsigned int hpet_id; /* HPET (dev is NULL) */
- };
- struct pci_dev *dev;
- int irq;
- int remap_index;/* index in interrupt remapping table */
+struct msi_attrib {
+uint8_t type;/* {0: unused, 5h:MSI, 11h:MSI-X} */
+uint8_t pos; /* Location of the MSI capability */
+bool maskbit : 1; /* mask/pending bit supported ? */
+bool is_64: 1; /* Address size: 0=32bit 1=64bit */
+bool host_masked : 1;
+bool guest_masked : 1;
+uint16_t entry_nr; /* specific enabled entry */
+} msi_attrib;
+
+bool irte_initialized;
+uint8_t gvec;/* guest vector. valid when pi_desc isn't NULL */
+const struct pi_desc *pi_desc; /* pointer to posted descriptor */
+
+struct list_head list;
+
+union {
+void __iomem *mask_base; /* va for the entry in mask table */
+struct {
+unsigned int nvec; /* number of vectors */
+unsigned int mpos; /* location of mask register */
+} msi;
+unsigned int hpet_id; /* HPET (dev is NULL) */
+};
+struct pci_dev *dev;
+int irq;
+int remap_index; /* index in interrupt remapping table */
- struct msi_msg msg; /* Last set MSI message */
+struct msi_msg msg; /* Last set MSI message */
};
/*
@@ -179,49 +179,27 @@ int msi_free_irq(struct msi_desc *entry)
*/
struct __packed msg_data {
-#if defined(__LITTLE_ENDIAN_BITFIELD)
- __u32 vector : 8;
- __u32 delivery_mode : 3; /* 000b: FIXED | 001b: lowest prior */
- __u32 reserved_1 : 3;
- __u32 level : 1; /* 0: deassert | 1: assert */
- __u32 trigger : 1; /* 0: edge | 1: level */
- __u32 reserved_2 : 16;
-#elif defined(__BIG_ENDIAN_BITFIELD)
- __u32 reserved_2 : 16;
- __u32 trigger : 1; /* 0: edge | 1: level */
- __u32 level : 1; /* 0: deassert | 1: assert */
- __u32 reserved_1 : 3;
- __u32 delivery_mode : 3; /* 000b: FIXED | 001b: lowest prior */
- __u32 vector : 8;
-#else
-#error "Bitfield endianness not defined! Check your byteorder.h"
-#endif
+uint32_t vector: 8;
+uint32_t
Re: [PATCH 3/4] x86/vmx: cleanup vmx.c
On 21.02.2023 12:35, Xenia Ragiadakou wrote: > On 2/21/23 13:26, Jan Beulich wrote: >> On 17.02.2023 19:48, Xenia Ragiadakou wrote: >>> Do not include the headers: >>>asm/hvm/vpic.h >>>asm/hvm/vpt.h >>>asm/io.h >>>asm/mce.h >>>asm/mem_sharing.h >>>asm/regs.h >>>public/arch-x86/cpuid.h >>>public/hvm/save.h >>> because none of the declarations and macro definitions in them is used. >>> Sort alphabetically the rest of the headers. >>> >>> Rearrange the code to replace all forward declarations with the function >>> definitions. >>> >>> Replace double new lines with one. >>> >>> Reduce scope of nvmx_enqueue_n2_exceptions() to static because it is used >>> only in this file. >>> >>> 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. >>> >>> Take the opportunity to remove all trailing spaces in vmx.c. >>> >>> No functional change intended. >>> >>> Signed-off-by: Xenia Ragiadakou >>> --- >>> xen/arch/x86/hvm/vmx/vmcs.c| 12 + >>> xen/arch/x86/hvm/vmx/vmx.c | 5844 >>> xen/arch/x86/include/asm/hvm/vmx/vmx.h |1 - >>> 3 files changed, 2913 insertions(+), 2944 deletions(-) >> >> I'm afraid this is close to unreviewable and hence absolutely needs >> splitting. >> With this massive amount of re-arrangement (it's half of vmx.c, after all) >> I'm >> also concerned of losing "git blame"-ability for fair parts of the code >> there. > > I understand. Let me split the changes apart from the one that > rearranges the code. Do you agree in principle? or do you want me to > except and sth else? Well, the large amount of code movement wants at least one other party (e.g. Kevin, Andrew, or Roger) agreeing with your approach. As said, I for one don't like this interruption in half-way easy history determination (which can be particularly helpful e.g. when you want to find a commit to put in a Fixes: tag). Jan
Re: [PATCH 1/4] x86/svm: cleanup svm.c
On 21/02/2023 11:42 am, Xenia Ragiadakou wrote: > > On 2/21/23 13:12, Jan Beulich wrote: >> On 21.02.2023 08:45, Xenia Ragiadakou wrote: >>> Hi Andrew, >>> >>> On 2/21/23 00:12, Andrew Cooper wrote: On 17/02/2023 6:48 pm, Xenia Ragiadakou wrote: > Do not include the headers: > xen/irq.h > asm/hvm/svm/intr.h > asm/io.h > asm/mem_sharing.h > asm/regs.h Out of interest, how are you calculating these? They're accurate as far as I can tell. >>> >>> I do not use a script (at least not a decent one), if that 's the >>> question :) . I verify that none of the symbols defined or declared in >>> the header is used in the file including the header. >>> Looking at asm/hvm/svm/*, intr.h itself can be straight deleted, svmdebug.h can be merged into vmcb.h, and all the others can move into xen/arch/x86/hvm/svm/ as local headers. None of them have any business being included elsewhere in Xen. >>> >>> I can send another patch for that. >>> > because none of the declarations and macro definitions in them is > used. > Sort alphabetically the rest of the headers. Minor grammar point. "Sort the rest of the headers alphabetically" would be a more normal way of phrasing this. >>> >>> I will fix it in v2. >> >> I guess this can be adjusted while committing, seeing that ... >> > Remove the forward declaration of svm_function_table and place > start_svm() > after the svm_function_table's definition. > > Replace double new lines with one. > > No functional change intended. > > Signed-off-by: Xenia Ragiadakou Acked-by: Andrew Cooper >> >> ... it's otherwise ready to be committed. > > Great, thx. I already committed this patch, with it fixed up, and one other tweak that we commonly do which is to leave a blank line between different groups of headers. It greatly helps people trying to figure out where to put a new header. ~Andrew
Re: [PATCH 3/3] x86/treewide: Drop the TRAP_* legacy names
On 20.02.2023 12:59, Andrew Cooper wrote: > We have two naming schemes for exceptions - X86_EXC_?? which use the > archtiectural abbreviations, and TRAP_* which is a mix of terminology and > nonstandard abbrevations. Switch to X86_EXC_* uniformly. > > No funcational change, confirmed by diffing the disassembly. Only 7 binary > changes, and they're all __LINE__ being passed into printk(). > > Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -2745,9 +2745,9 @@ static int cf_check sh_page_fault( > * stream under Xen's feet. > */ > if ( emul_ctxt.ctxt.event.type == X86_EVENTTYPE_HW_EXCEPTION && > - ((emul_ctxt.ctxt.event.vector == TRAP_page_fault) || > - (((emul_ctxt.ctxt.event.vector == TRAP_gp_fault) || > -(emul_ctxt.ctxt.event.vector == TRAP_stack_error)) && > + ((emul_ctxt.ctxt.event.vector == X86_EXC_PF) || > + (((emul_ctxt.ctxt.event.vector == X86_EXC_GP) || > +(emul_ctxt.ctxt.event.vector == X86_EXC_SS)) && > emul_ctxt.ctxt.event.error_code == 0)) ) > hvm_inject_event(_ctxt.ctxt.event); > else Entirely unrelated to your change, but seeing that this piece of code is being touched: Aren't we too lax here with #PF? Shouldn't we refuse propagation also for e.g. reserved bit faults and insn fetch ones? Maybe even for anything that isn't a supervisor write? Jan
Re: [PATCH v1] xen: Work around Clang-IAS macro expansion bug
On 21/02/2023 12:46 pm, Jan Beulich wrote:
> On 21.02.2023 13:26, Andrew Cooper wrote:
>> On 21/02/2023 10:31 am, Jan Beulich wrote:
>>> On 17.02.2023 13:22, Andrew Cooper wrote:
https://github.com/llvm/llvm-project/issues/60792
It turns out that Clang-IAS does not expand \@ uniquely in a translaition
unit, and the XSA-426 change tickles this bug:
:4:1: error: invalid symbol redefinition
.L1_fill_rsb_loop:
^
make[3]: *** [Rules.mk:247: arch/x86/acpi/cpu_idle.o] Error 1
Extend DO_OVERWRITE_RSB with an optional parameter so C callers can mux %=
in
too, which Clang does seem to expand properly.
Fixes: 63305e5392ec ("x86/spec-ctrl: Mitigate Cross-Thread Return Address
Predictions")
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Roger Pau Monné
CC: Wei Liu
v1 (vs RFC):
* Rename \foo to \x, reorder WRT \@ to avoid needing \() too.
>> Sadly, this is not quite correct. There needs to be a non-numeric
>> character separating \@ and \x or the concatenation of the two can end
>> up non-unique.
> How that if \@ is always 1?
It isn't always 1.
Global (file scope) have \@ expand properly from 0 to N.
Function scope have \@ expand properly from 0 to N in a single asm()
statement.
The 1 here is actually because mknops took the 0 and didn't use it. If
instead we had something like:
asm ("DO_OVERWRITE_RSB"); // \@ = 0
asm ("mkops 2;"
"DO_OVERWRITE_RSB"); // \@ = 1
then it would assemble fine, but the way we build alternatives in asm()
statements reliably causes the alternative block to consume \@=1.
>> So I think we need the \().
> Or put one at the start of the label and the other at the end?
We already played that trick with \n for the .irp, which suffers
similarly with concatenation.
~Andrew
Re: [PATCH v1] xen: Work around Clang-IAS macro expansion bug
On 21.02.2023 13:26, Andrew Cooper wrote:
> On 21/02/2023 10:31 am, Jan Beulich wrote:
>> On 17.02.2023 13:22, Andrew Cooper wrote:
>>> https://github.com/llvm/llvm-project/issues/60792
>>>
>>> It turns out that Clang-IAS does not expand \@ uniquely in a translaition
>>> unit, and the XSA-426 change tickles this bug:
>>>
>>> :4:1: error: invalid symbol redefinition
>>> .L1_fill_rsb_loop:
>>> ^
>>> make[3]: *** [Rules.mk:247: arch/x86/acpi/cpu_idle.o] Error 1
>>>
>>> Extend DO_OVERWRITE_RSB with an optional parameter so C callers can mux %=
>>> in
>>> too, which Clang does seem to expand properly.
>>>
>>> Fixes: 63305e5392ec ("x86/spec-ctrl: Mitigate Cross-Thread Return Address
>>> Predictions")
>>> Signed-off-by: Andrew Cooper
>>> ---
>>> CC: Jan Beulich
>>> CC: Roger Pau Monné
>>> CC: Wei Liu
>>>
>>> v1 (vs RFC):
>>> * Rename \foo to \x, reorder WRT \@ to avoid needing \() too.
>
> Sadly, this is not quite correct. There needs to be a non-numeric
> character separating \@ and \x or the concatenation of the two can end
> up non-unique.
How that if \@ is always 1?
> So I think we need the \().
Or put one at the start of the label and the other at the end?
>>> --- a/xen/arch/x86/include/asm/spec_ctrl.h
>>> +++ b/xen/arch/x86/include/asm/spec_ctrl.h
>>> @@ -83,7 +83,7 @@ static always_inline void
>>> spec_ctrl_new_guest_context(void)
>>> wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
>>>
>>> /* (ab)use alternative_input() to specify clobbers. */
>>> -alternative_input("", "DO_OVERWRITE_RSB", X86_BUG_IBPB_NO_RET,
>>> +alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_BUG_IBPB_NO_RET,
>>>: "rax", "rcx");
>>> }
>>>
>>> @@ -172,7 +172,7 @@ static always_inline void spec_ctrl_enter_idle(struct
>>> cpu_info *info)
>>> *
>>> * (ab)use alternative_input() to specify clobbers.
>>> */
>>> -alternative_input("", "DO_OVERWRITE_RSB", X86_FEATURE_SC_RSB_IDLE,
>>> +alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_FEATURE_SC_RSB_IDLE,
>>>: "rax", "rcx");
>>> }
>> Since inside the macro you retain the uses of \@, I'd find it desirable
>> to keep gcc-generated code tidy by omitting the extra argument there.
>
> As I said in the RFC, I tried to have x=\@ but gas really didn't like that.
>
> But given the concatenation observation, we also cannot simply replace
> \@ with %= (given the option, which I couldn't figure out), because they
> can overlap.
>
>> This could be done by introducing another C macro along the lines of:
>>
>> #ifdef __clang__
>> #define CLANG_UNIQUE(name) " " #name "=%="
>> #else
>> #define CLANG_UNIQUE(name)
>> #endif
>>
>> Besides the less confusing label names this would also have the benefit
>> of providing a link at the use sites to what the issue is that is being
>> worked around. Plus, once (if ever) fixed in Clang, we could then adjust
>> the condition to just the affected versions.
>
> It's only Clang IAS, but there's no suitable define to figure this out.
"this" being what? The case of Clang but with / without integrated as?
Since we have to turn that off, we could as well add a -D option along
with the -no-integrated-as one.
> And while I appreciate the effort to be more descriptive, this name is
> literally longer than the thing it wraps and ...
>
>>> --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
>>> +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
>>> @@ -117,11 +117,15 @@
>>> .L\@_done:
>>> .endm
>>>
>>> -.macro DO_OVERWRITE_RSB tmp=rax
>>> +.macro DO_OVERWRITE_RSB tmp=rax x=
>> The "=" in "x=" isn't needed, is it? It looks a little confusing to me,
>> raising the question "Why is this there?"
>
> Because I started out with u=\@
>
>>> /*
>>> * Requires nothing
>>> * Clobbers \tmp (%rax by default), %rcx
>>> *
>>> + * x= is an optional parameter to work around
>>> + * https://github.com/llvm/llvm-project/issues/60792 where Clang-IAS
>>> doesn't
>>> + * expand \@ uniquely, and is intended for muxing %= in too.
>> With the above suggestion I'd see this comment move to next to that
>> helper macro, with a link to there left here.
>
> ... there's no getting rid of this comment, at least in some form. The
> parameter is odd, and needs explaining.
Of course, hence my "with a link to there left here".
> Passing %= unconditionally doesn't matter for GCC/Binuitls, and in this
> case, attempts to "declutter" the niche usecase of inspecting the
> assembled file comes with a substantial complexity at the C level.
> It's really not worth the extra complexity.
I wouldn't call that one simple macro "complexity". I'm in particular
not advocating for (conditionally) removing the extra macro parameter.
Jan
Re: [PATCH v1] xen: Work around Clang-IAS macro expansion bug
On 21/02/2023 10:31 am, Jan Beulich wrote:
> On 17.02.2023 13:22, Andrew Cooper wrote:
>> https://github.com/llvm/llvm-project/issues/60792
>>
>> It turns out that Clang-IAS does not expand \@ uniquely in a translaition
>> unit, and the XSA-426 change tickles this bug:
>>
>> :4:1: error: invalid symbol redefinition
>> .L1_fill_rsb_loop:
>> ^
>> make[3]: *** [Rules.mk:247: arch/x86/acpi/cpu_idle.o] Error 1
>>
>> Extend DO_OVERWRITE_RSB with an optional parameter so C callers can mux %= in
>> too, which Clang does seem to expand properly.
>>
>> Fixes: 63305e5392ec ("x86/spec-ctrl: Mitigate Cross-Thread Return Address
>> Predictions")
>> Signed-off-by: Andrew Cooper
>> ---
>> CC: Jan Beulich
>> CC: Roger Pau Monné
>> CC: Wei Liu
>>
>> v1 (vs RFC):
>> * Rename \foo to \x, reorder WRT \@ to avoid needing \() too.
Sadly, this is not quite correct. There needs to be a non-numeric
character separating \@ and \x or the concatenation of the two can end
up non-unique. So I think we need the \().
>> I originally tried a parameter named uniq but this found a different
>> Clang-IAS
>> bug:
>>
>> In file included from arch/x86/asm-macros.c:3:
>> ./arch/x86/include/asm/spec_ctrl_asm.h:139:5: error: \u used with no
>> following hex digits; treating as '\' followed by identifier
>> [-Werror,-Wunicode]
>> .L\@\uniq\()fill_rsb_loop:
>> ^
>>
>> It appears that Clang is looking for unicode escapes before completing
>> parameter substitution. But the macro didn't originate from a context where
>> a
>> unicode escape was even applicable to begin with.
>>
>> The consequence is that you can't use macro prameters beginning with a u.
> How nice. If really needed, I guess we could use -Wno-unicode on the
> assumption that we don't use anything that could legitimately trigger that
> warning.
It's proving very stubborn to narrow down.
I really can't reproduce it outside of this specific occurrence in the
Xen build system, but my gut feeling is that it's something to do with
the asm level .include.
>> --- a/xen/arch/x86/include/asm/spec_ctrl.h
>> +++ b/xen/arch/x86/include/asm/spec_ctrl.h
>> @@ -83,7 +83,7 @@ static always_inline void spec_ctrl_new_guest_context(void)
>> wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
>>
>> /* (ab)use alternative_input() to specify clobbers. */
>> -alternative_input("", "DO_OVERWRITE_RSB", X86_BUG_IBPB_NO_RET,
>> +alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_BUG_IBPB_NO_RET,
>>: "rax", "rcx");
>> }
>>
>> @@ -172,7 +172,7 @@ static always_inline void spec_ctrl_enter_idle(struct
>> cpu_info *info)
>> *
>> * (ab)use alternative_input() to specify clobbers.
>> */
>> -alternative_input("", "DO_OVERWRITE_RSB", X86_FEATURE_SC_RSB_IDLE,
>> +alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_FEATURE_SC_RSB_IDLE,
>>: "rax", "rcx");
>> }
> Since inside the macro you retain the uses of \@, I'd find it desirable
> to keep gcc-generated code tidy by omitting the extra argument there.
As I said in the RFC, I tried to have x=\@ but gas really didn't like that.
But given the concatenation observation, we also cannot simply replace
\@ with %= (given the option, which I couldn't figure out), because they
can overlap.
> This could be done by introducing another C macro along the lines of:
>
> #ifdef __clang__
> #define CLANG_UNIQUE(name) " " #name "=%="
> #else
> #define CLANG_UNIQUE(name)
> #endif
>
> Besides the less confusing label names this would also have the benefit
> of providing a link at the use sites to what the issue is that is being
> worked around. Plus, once (if ever) fixed in Clang, we could then adjust
> the condition to just the affected versions.
It's only Clang IAS, but there's no suitable define to figure this out.
And while I appreciate the effort to be more descriptive, this name is
literally longer than the thing it wraps and ...
>> --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
>> +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
>> @@ -117,11 +117,15 @@
>> .L\@_done:
>> .endm
>>
>> -.macro DO_OVERWRITE_RSB tmp=rax
>> +.macro DO_OVERWRITE_RSB tmp=rax x=
> The "=" in "x=" isn't needed, is it? It looks a little confusing to me,
> raising the question "Why is this there?"
Because I started out with u=\@
>> /*
>> * Requires nothing
>> * Clobbers \tmp (%rax by default), %rcx
>> *
>> + * x= is an optional parameter to work around
>> + * https://github.com/llvm/llvm-project/issues/60792 where Clang-IAS doesn't
>> + * expand \@ uniquely, and is intended for muxing %= in too.
> With the above suggestion I'd see this comment move to next to that
> helper macro, with a link to there left here.
... there's no getting rid of this comment, at least in some form. The
parameter is odd, and needs explaining.
Passing %= unconditionally doesn't matter for GCC/Binuitls, and in this
case, attempts to "declutter" the niche usecase of inspecting the
assembled file comes
Re: [PATCH 1/3] x86/traps: Move do_general_protection() earlier
On 20.02.2023 12:59, Andrew Cooper wrote: > ... in order to clean up the declarations without needing to forward declare > it for handle_gdt_ldt_mapping_fault() > > No functional change. > > Signed-off-by: Andrew Cooper This is okay with me as long as the Misra related comment on patch 2 can be resolved suitably. I'll defer the ack here until then ... Jan
Re: [PATCH 2/3] x86/entry: Rework the exception entrypoints
On 20.02.2023 12:59, Andrew Cooper wrote:
> This fixes two issues preventing livepatching. First, that #PF and NMI fall
> through into other functions,
I thought this was deliberate, aiming at avoiding the unconditional branch
for the most commonly taken path each. I'm not really opposed to the change,
but I think it wants saying a little more as to how little (or big) of an
effect this has (or at least is expected to have).
> and second to add ELF metadata.
>
> Use a macro to generate the entrypoints programatically, rather than
> opencoding them all. Switch to using the architectural short names.
>
> Remove the DECLARE_TRAP_HANDLER{,_CONST}() infrastructure. Only NMI/#MC are
> referenced externally (and NMI will cease to be soon, as part of adding FRED
> support). Move the entrypoint declarations into the respective traps.c where
> they're used, rather than keeping them visible across ~all of Xen.
What about Misra? Won't they be unhappy about global functions with no
declaration in any header?
> Drop the long-stale comment at the top of init_idt_traps(). It's mostly
> discussing a 32bit Xen.
The use of interrupt gates isn't 32-bit only, and justifying why trap gates
aren't used looks to me like quite reasonable a purpose of that comment.
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -142,6 +142,50 @@ process_trap:
>
> .section .text.entry, "ax", @progbits
>
> +.macro IDT_ENTRY vec label handler
As said in reply to another recent patch: Could we agree on separating
macro parameters by commas? I also wonder whether they shouldn't all have
":req" tagged onto them, as none of them is optional.
> +ENTRY(\label)
> +ENDBR64
> + .if ((1 << \vec) & X86_EXC_HAVE_EC) == 0
Nit: Hard tab slipped in here.
> +push $0
> +.endif
> +movl $\vec, 4(%rsp)
> +jmp \handler
> +
> +.type \label, @function
> +.size \label, . - \label
> +.endm
> +
> +.macro ENTRY vec label
> +IDT_ENTRY \vec \label handle_exception
> +.endm
> +.macro ENTRY_IST vec label
> +IDT_ENTRY \vec \label handle_ist_exception
> +.endm
> +
> +
> +ENTRY X86_EXC_DE entry_DE /* 00 Divide Error */
> +ENTRY_IST X86_EXC_DB entry_DB /* 01 Debug Exception */
> +ENTRY_IST X86_EXC_NMI entry_NMI /* 02 Non-Maskable Interrupt */
> +ENTRY X86_EXC_BP entry_BP /* 03 Breakpoint (int3) */
> +ENTRY X86_EXC_OF entry_OF /* 04 Overflow (into) */
> +ENTRY X86_EXC_BR entry_BR /* 05 Bound Range */
> +ENTRY X86_EXC_UD entry_UD /* 06 Undefined Opcode */
> +ENTRY X86_EXC_NM entry_NM /* 07 No Maths (Device Not Present) */
> +/* _IST X86_EXC_DF entry_DF 08 Double Fault - Handled specially */
> +/*X86_EXC_CSO entry_CSO09 Coprocessor Segment Override - Autogen
> */
> +ENTRY X86_EXC_TS entry_TS /* 10 Invalid TSS */
> +ENTRY X86_EXC_NP entry_NP /* 11 Segment Not Present */
> +ENTRY X86_EXC_SS entry_SS /* 12 Stack Segment Fault */
> +ENTRY X86_EXC_GP entry_GP /* 13 General Protection Fault */
> +ENTRY X86_EXC_PF entry_PF /* 14 Page Fault */
> +/*X86_EXC_SPV entry_SPV15 PIC Spurious Interrupt Vector -
> Autogen */
> +ENTRY X86_EXC_MF entry_MF /* 16 Maths Fault (x87 FPU) */
> +ENTRY X86_EXC_AC entry_AC /* 17 Alignment Check */
> +ENTRY_IST X86_EXC_MC entry_MC /* 18 Machine Check */
> +ENTRY X86_EXC_XM entry_XM /* 19 SIMD Maths Fault */
> +/*X86_EXC_VE entry_VE 20 Virtualisation Exception - Not
> implemented */
> +ENTRY X86_EXC_CP entry_CP /* 21 Control-flow Protection */
I expect you won't like the request, but still: There's a lot of redundancy
here. The first two arguments could all be folded, having the macro attach
the X86_EXC_ and entry_ prefixes. Or wait - only quite, as long as X86_EXC_*
are C macros rather than assembler equates.
Jan
[ImageBuilder][PATCH] uboot-script-gen: Add virtio loader
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 --- 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
Re: [PATCH 1/4] x86/svm: cleanup svm.c
On 2/21/23 13:12, Jan Beulich wrote: On 21.02.2023 08:45, Xenia Ragiadakou wrote: Hi Andrew, On 2/21/23 00:12, Andrew Cooper wrote: On 17/02/2023 6:48 pm, Xenia Ragiadakou wrote: Do not include the headers: xen/irq.h asm/hvm/svm/intr.h asm/io.h asm/mem_sharing.h asm/regs.h Out of interest, how are you calculating these? They're accurate as far as I can tell. I do not use a script (at least not a decent one), if that 's the question :) . I verify that none of the symbols defined or declared in the header is used in the file including the header. Looking at asm/hvm/svm/*, intr.h itself can be straight deleted, svmdebug.h can be merged into vmcb.h, and all the others can move into xen/arch/x86/hvm/svm/ as local headers. None of them have any business being included elsewhere in Xen. I can send another patch for that. because none of the declarations and macro definitions in them is used. Sort alphabetically the rest of the headers. Minor grammar point. "Sort the rest of the headers alphabetically" would be a more normal way of phrasing this. I will fix it in v2. I guess this can be adjusted while committing, seeing that ... Remove the forward declaration of svm_function_table and place start_svm() after the svm_function_table's definition. Replace double new lines with one. No functional change intended. Signed-off-by: Xenia Ragiadakou Acked-by: Andrew Cooper ... it's otherwise ready to be committed. Great, thx. Jan -- Xenia
Re: [PATCH 4/4] x86/vmx: cleanup vmx.h
On 2/21/23 13:23, Jan Beulich wrote: On 17.02.2023 19:48, Xenia Ragiadakou wrote: Do not include the headers: asm/i387.h asm/hvm/trace.h asm/processor.h asm/regs.h because none of the declarations and macro definitions in them is used in this file. Sort alphabetically the rest of the headers. Fix build by including asm/i387.h in vmx.c, needed for vcpu_restore_fpu_lazy(). Move the definition of GAS_VMX_OP just above the functions that use it and undefine it after its usage. Move in vmcs.c the definitions of: ept_sync_all() __vmxoff() __vmxon() because they are used only in this file. Take the opportunity to remove a trailing white space. While the latter two are probably fine to be moved, I think the first one wants to remain where it is, as new uses might appear. Ok I will leave it where it is. Move in vmx.c the definitions of: pi_test_and_set_pir() pi_test_pir() pi_test_and_set_on() pi_set_on() pi_test_and_clear_on() pi_test_on() pi_get_pir() pi_test_sn() pi_set_sn() pi_clear_sn() vpid_sync_vcpu_gva() because they are used only in this file. I'm not fully convinced of such movement. As a general remark: We typically avoid "inline" for functions in .c files. Yet most if not all of the above are pretty good candidates for being explicitly marked "inline". I could create a private header. Move in vmx.c the declarations of: ve_info_t ept_qual_t idt_or_gdt_instr_info_t ldt_or_tr_instr_info_t because they are used only in this file. I disagree with the movement of such types. Sooner or later they may want using by vvmx.c as well. If you want to move them, then to a private header under xen/arch/x86/hvm/vmx/. Ok will do. Finally I think at least the individual groups of what is being moved or adjusted want splitting into separate patches. Ok. Jan -- Xenia
Re: [PATCH 3/4] x86/vmx: cleanup vmx.c
Hi Jan, On 2/21/23 13:26, Jan Beulich wrote: On 17.02.2023 19:48, Xenia Ragiadakou wrote: Do not include the headers: asm/hvm/vpic.h asm/hvm/vpt.h asm/io.h asm/mce.h asm/mem_sharing.h asm/regs.h public/arch-x86/cpuid.h public/hvm/save.h because none of the declarations and macro definitions in them is used. Sort alphabetically the rest of the headers. Rearrange the code to replace all forward declarations with the function definitions. Replace double new lines with one. Reduce scope of nvmx_enqueue_n2_exceptions() to static because it is used only in this file. 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. Take the opportunity to remove all trailing spaces in vmx.c. No functional change intended. Signed-off-by: Xenia Ragiadakou --- xen/arch/x86/hvm/vmx/vmcs.c| 12 + xen/arch/x86/hvm/vmx/vmx.c | 5844 xen/arch/x86/include/asm/hvm/vmx/vmx.h |1 - 3 files changed, 2913 insertions(+), 2944 deletions(-) I'm afraid this is close to unreviewable and hence absolutely needs splitting. With this massive amount of re-arrangement (it's half of vmx.c, after all) I'm also concerned of losing "git blame"-ability for fair parts of the code there. I understand. Let me split the changes apart from the one that rearranges the code. Do you agree in principle? or do you want me to except and sth else? Jan -- Xenia
Re: [PATCH 3/4] x86/vmx: cleanup vmx.c
On 17.02.2023 19:48, Xenia Ragiadakou wrote: > Do not include the headers: > asm/hvm/vpic.h > asm/hvm/vpt.h > asm/io.h > asm/mce.h > asm/mem_sharing.h > asm/regs.h > public/arch-x86/cpuid.h > public/hvm/save.h > because none of the declarations and macro definitions in them is used. > Sort alphabetically the rest of the headers. > > Rearrange the code to replace all forward declarations with the function > definitions. > > Replace double new lines with one. > > Reduce scope of nvmx_enqueue_n2_exceptions() to static because it is used > only in this file. > > 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. > > Take the opportunity to remove all trailing spaces in vmx.c. > > No functional change intended. > > Signed-off-by: Xenia Ragiadakou > --- > xen/arch/x86/hvm/vmx/vmcs.c| 12 + > xen/arch/x86/hvm/vmx/vmx.c | 5844 > xen/arch/x86/include/asm/hvm/vmx/vmx.h |1 - > 3 files changed, 2913 insertions(+), 2944 deletions(-) I'm afraid this is close to unreviewable and hence absolutely needs splitting. With this massive amount of re-arrangement (it's half of vmx.c, after all) I'm also concerned of losing "git blame"-ability for fair parts of the code there. Jan
Re: [PATCH 4/4] x86/vmx: cleanup vmx.h
On 17.02.2023 19:48, Xenia Ragiadakou wrote: > Do not include the headers: > asm/i387.h > asm/hvm/trace.h > asm/processor.h > asm/regs.h > because none of the declarations and macro definitions in them is used in > this file. Sort alphabetically the rest of the headers. > Fix build by including asm/i387.h in vmx.c, needed for > vcpu_restore_fpu_lazy(). > > Move the definition of GAS_VMX_OP just above the functions that use it and > undefine it after its usage. > > Move in vmcs.c the definitions of: > ept_sync_all() > __vmxoff() > __vmxon() > because they are used only in this file. Take the opportunity to remove a > trailing white space. While the latter two are probably fine to be moved, I think the first one wants to remain where it is, as new uses might appear. > Move in vmx.c the definitions of: > pi_test_and_set_pir() > pi_test_pir() > pi_test_and_set_on() > pi_set_on() > pi_test_and_clear_on() > pi_test_on() > pi_get_pir() > pi_test_sn() > pi_set_sn() > pi_clear_sn() > vpid_sync_vcpu_gva() > because they are used only in this file. I'm not fully convinced of such movement. As a general remark: We typically avoid "inline" for functions in .c files. Yet most if not all of the above are pretty good candidates for being explicitly marked "inline". > Move in vmx.c the declarations of: > ve_info_t > ept_qual_t > idt_or_gdt_instr_info_t > ldt_or_tr_instr_info_t > because they are used only in this file. I disagree with the movement of such types. Sooner or later they may want using by vvmx.c as well. If you want to move them, then to a private header under xen/arch/x86/hvm/vmx/. Finally I think at least the individual groups of what is being moved or adjusted want splitting into separate patches. Jan
Re: [PATCH 1/4] x86/svm: cleanup svm.c
On 21.02.2023 08:45, Xenia Ragiadakou wrote: > Hi Andrew, > > On 2/21/23 00:12, Andrew Cooper wrote: >> On 17/02/2023 6:48 pm, Xenia Ragiadakou wrote: >>> Do not include the headers: >>>xen/irq.h >>>asm/hvm/svm/intr.h >>>asm/io.h >>>asm/mem_sharing.h >>>asm/regs.h >> >> Out of interest, how are you calculating these? They're accurate as far >> as I can tell. > > I do not use a script (at least not a decent one), if that 's the > question :) . I verify that none of the symbols defined or declared in > the header is used in the file including the header. > >> >> Looking at asm/hvm/svm/*, intr.h itself can be straight deleted, >> svmdebug.h can be merged into vmcb.h, and all the others can move into >> xen/arch/x86/hvm/svm/ as local headers. None of them have any business >> being included elsewhere in Xen. > > I can send another patch for that. > >> >>> because none of the declarations and macro definitions in them is used. >>> Sort alphabetically the rest of the headers. >> >> Minor grammar point. "Sort the rest of the headers alphabetically" would >> be a more normal way of phrasing this. > > I will fix it in v2. I guess this can be adjusted while committing, seeing that ... >>> Remove the forward declaration of svm_function_table and place start_svm() >>> after the svm_function_table's definition. >>> >>> Replace double new lines with one. >>> >>> No functional change intended. >>> >>> Signed-off-by: Xenia Ragiadakou >> >> Acked-by: Andrew Cooper ... it's otherwise ready to be committed. Jan
Re: [PATCH RFC] xen: Annotate printk() as cold
On 20.02.2023 14:13, Andrew Cooper wrote: > There is no such thing as a fastpath with a printk() on it, making printk() an > excellent heuristic for slowpaths. > > Net delta is: > > add/remove: 595/2 grow/shrink: 56/762 up/down: 70879/-87331 (-16452) > Total: Before=4085425, After=4068973, chg -0.40% > > because cold functions are optimised differently. For example, one function > with a particularly large swing is: > > vmcs_dump_vcpu.cold-2172 +2172 > vmcs_dump_vcpu 7030 408 -6622 > > with a net delta of 7030 down to 4450. > > Signed-off-by: Andrew Cooper Acked-by: Jan Beulich > There are other functions which will be good heuristics to annotate, but we > probaby want to collect .cold together in one location rather than having them > spread out across all translation units. Doesn't the compiler put it in .text.cold? Or is that compiler variant and/or version dependent? Jan
Re: [PATCH] x86/asm: ELF metadata for simple cases
On 20.02.2023 12:04, Andrew Cooper wrote: > This is generally good practice, and necessary for livepatch binary diffing to > work. > > With this, livepatching of the SVM entry path works. The only complication is > with svm_stgi_label which is only used by oprofile to guestimate (not > completely correctly) when an NMI hit guest context. > > Livepatching of VMX is still an open question, because the logic doesn't form > anything remotely resembling functions. Both code fragments jump into each > other so need to be updated in tandem. Also, both code fragment entries need > trampolines in the case that patching actually occurs. For now, just treat it > as a single function. I agree. > Signed-off-by: Andrew Cooper Acked-by: Jan Beulich
Re: [PATCH] x86/asm: ELF metadata for simple cases
On 20.02.2023 12:51, Ross Lagerwall wrote: >> --- a/xen/arch/x86/clear_page.S >> +++ b/xen/arch/x86/clear_page.S >> @@ -16,3 +16,6 @@ ENTRY(clear_page_sse2) >> >> sfence >> ret >> + >> + .type clear_page_sse2, @function >> + .size clear_page_sse2, . - clear_page_sse2 > > Would it be worth wrapping this pattern in a macro? Funny you should ask this: Yes, certainly, but we can't quite agree what shape that macro (or really set of macros) is to take. Hence we did agree on this minimalistic approach as an intermediate solution. Jan
Re: [PATCH 3/3] x86/kexec: Annotate functions with ELF metadata
On 17.02.2023 18:48, Andrew Cooper wrote: > @@ -90,7 +91,10 @@ ENTRY(kexec_reloc) > push%rax > lretq > > -relocate_pages: > +.type kexec_reloc, @function > +.size kexec_reloc, . - kexec_reloc > + > +ENTRY(relocate_pages) > /* %rdi - indirection page maddr */ > pushq %rbx > > @@ -137,9 +141,12 @@ relocate_pages: > popq%rbx > ret > > +.type relocate_pages, @function > +.size relocate_pages, . - relocate_pages > + > .code32 > > -compatibility_mode: > +ENTRY(compatibility_mode) Do you really mean to make both labels global, thus potentially risking a link-time name collision down the road? In C files we try to move the other direction after all, making symbols static which can be. > @@ -167,7 +174,14 @@ compatibility_mode: > call*%ebp > ud2 > > -.align 4 > +.type compatibility_mode, @function > +.size compatibility_mode, . - compatibility_mode > + > +/* > + * Ensure data is in a different cache line to code. > + */ Nit (style): Strictly speaking this is a single-line comment. Jan > +.align SMP_CACHE_BYTES, 0 > + > compat_mode_gdt_desc: > .word .Lcompat_mode_gdt_end - compat_mode_gdt -1 > .quad . - kexec_reloc/* Relocated before use */
Re: [PATCH 2/3] x86/kexec: Simplify the relocation of compat_mode_gdt_desc
On 17.02.2023 18:48, Andrew Cooper wrote: > Assemble the GDT base relative to kexec_reloc, and simply add the identity map > base address to relocate. > > Adjust a stale comment, and drop the unused matching label. Only kind of - the comment is referencing call_32_bit, and hence wasn't really stale. And what was (and would remain to be) dead is call_64_bit. May want slightly re-wording. > @@ -81,9 +80,8 @@ ENTRY(kexec_reloc) > /* Setup IDT. */ > lidtcompat_mode_idt(%rip) > > -/* Load compat GDT. */ > -leaqcompat_mode_gdt(%rip), %rax > -movq%rax, (compat_mode_gdt_desc + 2)(%rip) > +/* Relocate and load compat GDT. */ > +add %rdi, 2 + compat_mode_gdt_desc(%rip) > lgdtcompat_mode_gdt_desc(%rip) Where's %rdi being populated for this? At kexec_reloc %rdi points at the code page, but prior to calling relocate_pages the register is overwritten (and the original value is lost). relocate_pages also has normal C calling convention afaict; kind of as a result %rdi is actually being clobbered there. Jan
Re: [PATCH 1/3] x86/kexec: Drop compatibility_mode_far
On 17.02.2023 18:48, Andrew Cooper wrote: > ljmp is (famously?) incompatible between Intel and AMD CPUs, and while we're > using one of the compatible forms, we've got a good stack and lret is the far > more common way of doing this. > > No functional change. > > Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich One question though: > --- a/xen/arch/x86/x86_64/kexec_reloc.S > +++ b/xen/arch/x86/x86_64/kexec_reloc.S > @@ -86,12 +86,11 @@ ENTRY(kexec_reloc) > movq%rax, (compat_mode_gdt_desc + 2)(%rip) > lgdtcompat_mode_gdt_desc(%rip) > > -/* Relocate compatibility mode entry point address. */ > -lealcompatibility_mode(%rip), %eax > -movl%eax, compatibility_mode_far(%rip) > - > /* Enter compatibility mode. */ > -ljmp*compatibility_mode_far(%rip) > +lea compatibility_mode(%rip), %rax > +push$0x10 Any thought about making this literal number a proper expression, rendering the code a little less fragile? Jan
Re: [PATCH v1] xen: Work around Clang-IAS macro expansion bug
On 17.02.2023 13:22, Andrew Cooper wrote:
> https://github.com/llvm/llvm-project/issues/60792
>
> It turns out that Clang-IAS does not expand \@ uniquely in a translaition
> unit, and the XSA-426 change tickles this bug:
>
> :4:1: error: invalid symbol redefinition
> .L1_fill_rsb_loop:
> ^
> make[3]: *** [Rules.mk:247: arch/x86/acpi/cpu_idle.o] Error 1
>
> Extend DO_OVERWRITE_RSB with an optional parameter so C callers can mux %= in
> too, which Clang does seem to expand properly.
>
> Fixes: 63305e5392ec ("x86/spec-ctrl: Mitigate Cross-Thread Return Address
> Predictions")
> Signed-off-by: Andrew Cooper
> ---
> CC: Jan Beulich
> CC: Roger Pau Monné
> CC: Wei Liu
>
> v1 (vs RFC):
> * Rename \foo to \x, reorder WRT \@ to avoid needing \() too.
>
> I originally tried a parameter named uniq but this found a different Clang-IAS
> bug:
>
> In file included from arch/x86/asm-macros.c:3:
> ./arch/x86/include/asm/spec_ctrl_asm.h:139:5: error: \u used with no
> following hex digits; treating as '\' followed by identifier
> [-Werror,-Wunicode]
> .L\@\uniq\()fill_rsb_loop:
> ^
>
> It appears that Clang is looking for unicode escapes before completing
> parameter substitution. But the macro didn't originate from a context where a
> unicode escape was even applicable to begin with.
>
> The consequence is that you can't use macro prameters beginning with a u.
How nice. If really needed, I guess we could use -Wno-unicode on the
assumption that we don't use anything that could legitimately trigger that
warning.
> --- a/xen/arch/x86/include/asm/spec_ctrl.h
> +++ b/xen/arch/x86/include/asm/spec_ctrl.h
> @@ -83,7 +83,7 @@ static always_inline void spec_ctrl_new_guest_context(void)
> wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
>
> /* (ab)use alternative_input() to specify clobbers. */
> -alternative_input("", "DO_OVERWRITE_RSB", X86_BUG_IBPB_NO_RET,
> +alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_BUG_IBPB_NO_RET,
>: "rax", "rcx");
> }
>
> @@ -172,7 +172,7 @@ static always_inline void spec_ctrl_enter_idle(struct
> cpu_info *info)
> *
> * (ab)use alternative_input() to specify clobbers.
> */
> -alternative_input("", "DO_OVERWRITE_RSB", X86_FEATURE_SC_RSB_IDLE,
> +alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_FEATURE_SC_RSB_IDLE,
>: "rax", "rcx");
> }
Since inside the macro you retain the uses of \@, I'd find it desirable
to keep gcc-generated code tidy by omitting the extra argument there.
This could be done by introducing another C macro along the lines of:
#ifdef __clang__
#define CLANG_UNIQUE(name) " " #name "=%="
#else
#define CLANG_UNIQUE(name)
#endif
Besides the less confusing label names this would also have the benefit
of providing a link at the use sites to what the issue is that is being
worked around. Plus, once (if ever) fixed in Clang, we could then adjust
the condition to just the affected versions.
> --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
> +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
> @@ -117,11 +117,15 @@
> .L\@_done:
> .endm
>
> -.macro DO_OVERWRITE_RSB tmp=rax
> +.macro DO_OVERWRITE_RSB tmp=rax x=
The "=" in "x=" isn't needed, is it? It looks a little confusing to me,
raising the question "Why is this there?" Furthermore I think it would
be good practice if macro parameters were comma-separated. I realize we
don't have this anyway near consistent right now, but still.
> /*
> * Requires nothing
> * Clobbers \tmp (%rax by default), %rcx
> *
> + * x= is an optional parameter to work around
> + * https://github.com/llvm/llvm-project/issues/60792 where Clang-IAS doesn't
> + * expand \@ uniquely, and is intended for muxing %= in too.
With the above suggestion I'd see this comment move to next to that
helper macro, with a link to there left here.
Just to clarify: I'm not going to insist on any of the suggested
adjustments, but personally I think they would be beneficial. If you
are pretty certain the other way around, please let me know, and I'll
consider ack-ing (largely) as-is.
Jan
Re: [PATCH v4 2/3] Build system: Replace git:// and http:// with https://
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.) Jan
Re: [Discussion] Xen grants and access permissions
On 20-02-23, 07:13, Juergen Gross wrote: > There are no permission flags in Xen PV device protocols either. The kind of a > mapping (RO or RW) in the backend is selected via the I/O operation: in case > it > is a write type operation (guest writing data to a device), the related grants > are mapper as RO in the backend, in all other cases they are mapped as RW. > > The same applies to granted pages for virtio: the frontend side will grant the > page as RO in case the I/O operation is flagged as "DMA_TO_DEVICE", and as RW > in all other cases. The backend should always know, which direction the data > is > flowing, so it should be able to do the mapping with the correct access mode. Right, so the back-end actually knows the permission details, but it is getting lost while we do some vhost-user operations. Anyway, I have taken this in a different direction now and suggested a change to vhost-user protocol itself. That lets the back-end know that it is actually running on Xen and then it can do the mapping itself instead of asking the front-end, which doesn't make us loose the permission details. This also lets us write the backends in hypervisor agnostic way, hypervisor specific stuff is handled in vhost-user protocol's implementation now. https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg05946.html -- viresh
[RFC QEMU] docs: vhost-user: Add custom memory mapping support
The current model of memory mapping at the back-end works fine with Qemu, where a standard call to mmap() for the respective file descriptor, passed from front-end, is generally all we need to do before the front-end can start accessing the guest memory. There are other complex cases though, where we need more information at the backend and need to do more than just an mmap() call. For example, Xen, a type-1 hypervisor, currently supports memory mapping via two different methods, foreign-mapping (via /dev/privcmd) and grant-dev (via /dev/gntdev). In both these cases, the back-end needs to call mmap() followed by an ioctl() (or vice-versa), and need to pass extra information via the ioctl(), like the Xen domain-id of the guest whose memory we are trying to map. Add a new protocol feature, 'VHOST_USER_PROTOCOL_F_CUSTOM_MMAP', which lets the back-end know about the additional memory mapping requirements. When this feature is negotiated, the front-end can send the 'VHOST_USER_CUSTOM_MMAP' message type to provide the additional information to the back-end. Signed-off-by: Viresh Kumar --- docs/interop/vhost-user.rst | 32 1 file changed, 32 insertions(+) diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index 3f18ab424eb0..f2b1d705593a 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -258,6 +258,23 @@ Inflight description :queue size: a 16-bit size of virtqueues +Custom mmap description +^^^ + ++---+---+ +| flags | value | ++---+---+ + +:flags: 64-bit bit field + +- Bit 0 is Xen foreign memory access flag - needs Xen foreign memory mapping. +- Bit 1 is Xen grant memory access flag - needs Xen grant memory mapping. + +:value: a 64-bit hypervisor specific value. + +- For Xen foreign or grant memory access, this is set with guest's xen domain + id. + C structure --- @@ -867,6 +884,7 @@ Protocol features #define VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS 14 #define VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS 15 #define VHOST_USER_PROTOCOL_F_STATUS 16 + #define VHOST_USER_PROTOCOL_F_CUSTOM_MMAP 17 Front-end message types --- @@ -1422,6 +1440,20 @@ Front-end message types query the back-end for its device status as defined in the Virtio specification. +``VHOST_USER_CUSTOM_MMAP`` + :id: 41 + :equivalent ioctl: N/A + :request payload: Custom mmap description + :reply payload: N/A + + When the ``VHOST_USER_PROTOCOL_F_CUSTOM_MMAP`` protocol feature has been + successfully negotiated, this message is submitted by the front-end to + notify the back-end of the special memory mapping requirements, that the + back-end needs to take care of, while mapping any memory regions sent + over by the front-end. The front-end must send this message before + any memory-regions are sent to the back-end via ``VHOST_USER_SET_MEM_TABLE`` + or ``VHOST_USER_ADD_MEM_REG`` message types. + Back-end message types -- -- 2.31.1.272.g89b43f80a514
Re: [RFC PATCH 08/10] xen: pci: remove pcidev_[un]lock[ed] calls
On 21.02.2023 00:13, Volodymyr Babchuk wrote: > Stefano Stabellini writes: >> On Wed, 31 Aug 2022, Volodymyr Babchuk wrote: >>> As pci devices are refcounted now and all list that store them are >>> protected by separate locks, we can safely drop global pcidevs_lock. >>> >>> Signed-off-by: Volodymyr Babchuk >> >> Up until this patch this patch series introduces: >> - d->pdevs_lock to protect d->pdev_list >> - pci_seg->alldevs_lock to protect pci_seg->alldevs_list >> - iommu->ats_list_lock to protect iommu->ats_devices >> - pdev refcounting to detect a pdev in-use and when to free it >> - pdev->lock to protect pdev->msi_list >> >> They cover a lot of ground. Are they collectively covering everything >> pcidevs_lock() was protecting? > > Well, that is the question. Those patch are in RFC stage because I can't > fully answer your question. I tried my best to introduce proper locking, > but apparently missed couple of places, like > >> deassign_device is not protected by pcidevs_lock anymore. >> deassign_device accesses a number of pdev fields, including quarantine, >> phantom_stride and fault.count. >> >> deassign_device could run at the same time as assign_device who sets >> quarantine and other fields. >> > > I hope this is all, but problem is that PCI subsystem is old, large and > complex. Fo example, as I wrote earlier, there are places that are > protected with pcidevs_lock(), but do nothing with PCI. I just don't > know what to do with such places. I have a hope that x86 maintainers > would review my changes and give feedback on missed spots. At the risk of it sounding unfair, at least initially: While review may spot issues, you will want to keep in mind that none of the people who originally wrote that code are around anymore. And even if they were, it would be uncertain - just like for the x86 maintainers - that they would recall (if they were aware at some time in the first place) all the corner cases. Therefore I'm afraid that proving correctness and safety of the proposed transformations can only be done by properly auditing all involved code paths. Yet that's something that imo wants to already have been done by the time patches are submitted for review. Reviewers would then "merely" (hard enough perhaps) check the results of that audit. I might guess that this locking situation is one of the reasons why Andrew in particular thinks (afaik) that the IOMMU code we have would better be re-written almost from scratch. I assume it's clear to him (it certainly is to me) that this is something that could only be expected to happen in an ideal work: I see no-one taking on such an exercise. We already have too little bandwidth. Jan
[xen-unstable test] 177972: tolerable trouble: fail/pass/starved
flight 177972 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/177972/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-i386-libvirt-pair 11 xen-install/dst_host fail like 177883 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 177883 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 177883 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 177883 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 177883 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 177883 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 177883 test-amd64-amd64-libvirt-vhd 19 guest-start/debian.repeatfail like 177883 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 177883 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 177883 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 177883 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-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-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-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-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 406cea1970535cd7b9d6bcf09bc164ef9bb64bac baseline version: xen 406cea1970535cd7b9d6bcf09bc164ef9bb64bac Last test of basis 177972 2023-02-21 01:54:03 Z0 days Testing same since (not found) 0 attempts jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64-xtf pass build-amd64 pass build-arm64 pass build-armhf starved build-i386 pass build-amd64-libvirt
Re: [PATCH linux-next 2/2] x86/xen/time: cleanup xen_tsc_safe_clocksource
On Mon, Feb 20 2023 at 21:51, Krister Johansen wrote:
> On Mon, Feb 20, 2023 at 08:14:40PM -0800, Krister Johansen wrote:
>> > static bool __init xen_tsc_safe_clocksource(void)
>> > {
>> >u32 eax, ebx. ecx, edx;
>> >
>> >/* Leaf 4, sub-leaf 0 (0x4x03) */
>> >cpuid_count(xen_cpuid_base() + 3, 0, , , , );
>> >
>> >return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
>> > }
>>
>> I'm all for simplifying. I'm happy to clean up that return to be more
>> idiomatic. I was under the impression, perhaps mistaken, though, that
>> the X86_FEATURE_CONSTANT_TSC, X86_FEATURE_NONSTOP_TSC, and
>> check_tsc_unstable() checks were actually serving a purpose: to ensure
>> that we don't rely on the tsc in environments where it's being emulated
>> and the OS would be better served by using a PV clock. Specifically,
>> kvmclock_init() makes a very similar set of checks that I also thought
>> were load-bearing.
>
> Bah, what I meant to say was emulated, unstable, or otherwise unsuitable
> for use as a clocksource. IOW, even if TSC_MODE_NEVER_EMULATE is
> set, it's possible that a user is attempting a migration from a cpu
> that's not invariant, and we'd still want to check for that case and
> fall back to a PV clocksource, correct?
Sure. But a life migration from a NEVER_EMULATE to a non-invariant host
is a patently bad idea and has nothing to do with the __init function,
which is gone at that point already.
What I wanted to say:
static bool __init xen_tsc_safe_clocksource(void)
{
..
/* Leaf 4, sub-leaf 0 (0x4x03) */
cpuid_count(xen_cpuid_base() + 3, 0, , , , );
return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
}
I didn't have the full context and was just looking at the condition.
Now I checked the full context and I think that except for the
if (check_tsc_unstable())
check everything else can go away unless you do not trust the hypervisor
that it only sets the NEVER_EMULATE bit when CONSTANT and NONSTOP are
set as well. But yeah, you might prefer to be paranoid. It's virt after
all.
Thanks,
tglx
Re: [RFC PATCH v2 1/2] xen/memory : Add a stats_table resource type
On 20.02.2023 17:51, Matias Ezequiel Vara Larsen wrote:
> On Thu, Feb 16, 2023 at 04:15:29PM +0100, Jan Beulich wrote:
>> On 16.02.2023 16:07, Matias Ezequiel Vara Larsen wrote:
>>> On Wed, Dec 14, 2022 at 08:29:53AM +0100, Jan Beulich wrote:
On 07.10.2022 14:39, Matias Ezequiel Vara Larsen wrote:
> @@ -287,6 +289,20 @@ static inline void vcpu_runstate_change(
> }
>
> v->runstate.state = new_state;
> +
> +vcpustats_va = (shared_vcpustatspage_t*)d->vcpustats_page.va;
> +if ( vcpustats_va )
> +{
> + vcpustats_va->vcpu_info[v->vcpu_id].version =
> + version_update_begin(vcpustats_va->vcpu_info[v->vcpu_id].version);
> +smp_wmb();
> +
> memcpy(_va->vcpu_info[v->vcpu_id].runstate_running_time,
> + >runstate.time[RUNSTATE_running],
> + sizeof(v->runstate.time[RUNSTATE_running]));
> +smp_wmb();
> +vcpustats_va->vcpu_info[v->vcpu_id].version =
> +
> version_update_end(vcpustats_va->vcpu_info[v->vcpu_id].version);
> +}
A further aspect to consider here is cache line ping-pong. I think the
per-vCPU elements of the array want to be big enough to not share a
cache line. The interface being generic this presents some challenge
in determining what the supposed size is to be. However, taking into
account the extensibility question, maybe the route to take is to
simply settle on a power-of-2 value somewhere between x86'es and Arm's
cache line sizes and the pretty common page size of 4k, e.g. 512 bytes
or 1k?
>>>
>>> I do not now how to address this. I was thinking to align each vcpu_stats
>>> instance to a multiple of the cache-line. I would pick up the first multiple
>>> that is bigger to the size of the vcpu_stats structure. For example,
>>> currently
>>> the structure is 16 bytes so I would align each instance in a frame to 64
>>> bytes. Would it make sense?
>>
>> Well, 64 may be an option, but I gave higher numbers for a reason. One thing
>> I don't know is what common cache line sizes are on Arm or e.g. RISC-V.
>
> Thanks. I found that structures that require cache-aligment are defined with
> "__cacheline_aligned" that uses L1_CACHE_BYTES. For example, in x86, this
> aligns to 128 bytes. What is the reason to use a higher value like 512 bytes
> or
> 1k?.
You cannot bake an x86 property (which may even change: at some point we may
choose to drop the 128-byte special for the very few CPUs actually using
such, when the majority uses 64-byte cache lines) into the public interface.
You also don't want to make an aspect of the public interface arch-dependent
when not really needed. My suggestion for a higher value was in the hope that
we may never see a port to an architecture with cache lines wider than, say,
512 bytes. What exactly the value should be is of course up for discussion,
but I think it wants to include some slack on top of what we currently
support (arch-wise).
Jan
Re: [PATCH linux-next 2/2] x86/xen/time: cleanup xen_tsc_safe_clocksource
On 21.02.23 06:51, Krister Johansen wrote:
On Mon, Feb 20, 2023 at 08:14:40PM -0800, Krister Johansen wrote:
On Mon, Feb 20, 2023 at 11:01:18PM +0100, Thomas Gleixner wrote:
On Mon, Feb 20 2023 at 09:17, Krister Johansen wrote:
@@ -495,8 +496,7 @@ static int __init xen_tsc_safe_clocksource(void)
/* Leaf 4, sub-leaf 0 (0x4x03) */
cpuid_count(xen_cpuid_base() + 3, 0, , , , );
- /* tsc_mode = no_emulate (2) */
- if (ebx != 2)
+ if (ebx != XEN_CPUID_TSC_MODE_NEVER_EMULATE)
return 0;
return 1;
What about removing more stupidity from that function?
static bool __init xen_tsc_safe_clocksource(void)
{
u32 eax, ebx. ecx, edx;
/* Leaf 4, sub-leaf 0 (0x4x03) */
cpuid_count(xen_cpuid_base() + 3, 0, , , , );
return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
}
I'm all for simplifying. I'm happy to clean up that return to be more
idiomatic. I was under the impression, perhaps mistaken, though, that
the X86_FEATURE_CONSTANT_TSC, X86_FEATURE_NONSTOP_TSC, and
check_tsc_unstable() checks were actually serving a purpose: to ensure
that we don't rely on the tsc in environments where it's being emulated
and the OS would be better served by using a PV clock. Specifically,
kvmclock_init() makes a very similar set of checks that I also thought
were load-bearing.
Bah, what I meant to say was emulated, unstable, or otherwise unsuitable
for use as a clocksource. IOW, even if TSC_MODE_NEVER_EMULATE is
set, it's possible that a user is attempting a migration from a cpu
that's not invariant, and we'd still want to check for that case and
fall back to a PV clocksource, correct?
But Thomas' suggestion wasn't changing any behavior compared to your
patch. It just makes it easier to read.
If you are unsure your patch is correct, please verify the correctness
before sending it.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v2 09/13] tools/xenstore: add TDB access trace support
On 20.02.23 23:59, Julien Grall wrote:
Hi,
On 20/01/2023 10:00, Juergen Gross wrote:
Add a new trace switch "tdb" and the related trace calls.
The "tdb" switch is off per default.
Signed-off-by: Juergen Gross
With one remark (see below):
Reviewed-by: Julien Grall
---
tools/xenstore/xenstored_core.c | 8 +++-
tools/xenstore/xenstored_core.h | 6 ++
tools/xenstore/xenstored_transaction.c | 7 ++-
3 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 558ef491b1..49e196e7ae 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -589,6 +589,8 @@ static void get_acc_data(TDB_DATA *key, struct
node_account_data *acc)
if (old_data.dptr == NULL) {
acc->memory = 0;
} else {
+ trace_tdb("read %s size %zu\n", key->dptr,
+ old_data.dsize + key->dsize);
hdr = (void *)old_data.dptr;
acc->memory = old_data.dsize;
acc->domid = hdr->perms[0].id;
@@ -655,6 +657,7 @@ int do_tdb_write(struct connection *conn, TDB_DATA *key,
TDB_DATA *data,
errno = EIO;
return errno;
}
+ trace_tdb("store %s size %zu\n", key->dptr, data->dsize + key->dsize);
if (acc) {
/* Don't use new_domid, as it might be a transaction node. */
@@ -682,6 +685,7 @@ int do_tdb_delete(struct connection *conn, TDB_DATA *key,
errno = EIO;
return errno;
}
+ trace_tdb("delete %s\n", key->dptr);
if (acc->memory) {
domid = get_acc_domid(conn, key, acc->domid);
@@ -731,6 +735,8 @@ struct node *read_node(struct connection *conn, const void
*ctx,
goto error;
}
+ trace_tdb("read %s size %zu\n", key.dptr, data.dsize + key.dsize);
+
node->parent = NULL;
talloc_steal(node, data.dptr);
@@ -2746,7 +2752,7 @@ static void set_quota(const char *arg, bool soft)
/* Sorted by bit values of TRACE_* flags. Flag is (1u << index). */
const char *const trace_switches[] = {
- "obj", "io", "wrl", "acc",
+ "obj", "io", "wrl", "acc", "tdb",
NULL
};
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 3e0734a6c6..419a144396 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -303,8 +303,14 @@ extern unsigned int trace_flags;
#define TRACE_IO 0x0002
#define TRACE_WRL 0x0004
#define TRACE_ACC 0x0008
+#define TRACE_TDB 0x0010
extern const char *const trace_switches[];
int set_trace_switch(const char *arg);
Add a newline here.
Okay.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v2 08/13] tools/xenstore: add accounting trace support
On 20.02.23 23:57, Julien Grall wrote:
Hi Juergen,
On 20/01/2023 10:00, Juergen Gross wrote:
Add a new trace switch "acc" and the related trace calls.
The "acc" switch is off per default.
Signed-off-by: Juergen Gross
With one reamrk (see below):
Reviewed-by: Julien Grall
---
tools/xenstore/xenstored_core.c | 2 +-
tools/xenstore/xenstored_core.h | 1 +
tools/xenstore/xenstored_domain.c | 10 ++
3 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 6ef60179fa..558ef491b1 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2746,7 +2746,7 @@ static void set_quota(const char *arg, bool soft)
/* Sorted by bit values of TRACE_* flags. Flag is (1u << index). */
const char *const trace_switches[] = {
- "obj", "io", "wrl",
+ "obj", "io", "wrl", "acc",
NULL
};
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 1f811f38cb..3e0734a6c6 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -302,6 +302,7 @@ extern unsigned int trace_flags;
#define TRACE_OBJ 0x0001
#define TRACE_IO 0x0002
#define TRACE_WRL 0x0004
+#define TRACE_ACC 0x0008
extern const char *const trace_switches[];
int set_trace_switch(const char *arg);
diff --git a/tools/xenstore/xenstored_domain.c
b/tools/xenstore/xenstored_domain.c
index b1e29edb7e..d461fd8cc8 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -538,6 +538,12 @@ static struct domain *find_domain_by_domid(unsigned int
domid)
return (d && d->introduced) ? d : NULL;
}
+#define trace_acc(...) \
The indentation of '\' looks odd.
Not for me. Maybe you have a different tab setting?
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: linux-next: duplicate patches in the xen-tip tree
* Peter Zijlstra wrote:
> On Tue, Feb 14, 2023 at 12:47:00PM +1100, Stephen Rothwell wrote:
> > Hi all,
> >
> > The following commits are also in the tip tree as different commits
> > (but the same patches):
> >
> > 415dab3c1796 ("drivers/xen/hypervisor: Expose Xen SIF flags to userspace")
> > 336f560a8917 ("x86/xen: don't let xen_pv_play_dead() return")
> > f697cb00afa9 ("x86/xen: mark xen_pv_play_dead() as __noreturn")
> >
> > These are commits
> >
> > 859761e770f8 ("drivers/xen/hypervisor: Expose Xen SIF flags to userspace")
> > 076cbf5d2163 ("x86/xen: don't let xen_pv_play_dead() return")
> > 1aff0d2658e5 ("x86/xen: mark xen_pv_play_dead() as __noreturn")
> >
> > in the tip tree.
>
> This was intentional (dependencies) and the plan is to only offer the
> tip branch for merge after the Xen tree goes in.
The rebase & *duplication* was not intentional at all - I assumed
1aff0d2658e5 won't get rebased. :-/
We'll probably have to redo the objtool tree.
Thanks,
Ingo
Re: [PATCH v2 04/13] tools/xenstore: add framework to commit accounting data on success only
On 20.02.23 23:50, Julien Grall wrote:
Hi Juergen,
On 20/01/2023 10:00, Juergen Gross wrote:
Instead of modifying accounting data and undo those modifications in
case of an error during further processing, add a framework for
collecting the needed changes and commit them only when the whole
operation has succeeded.
This scheme can reuse large parts of the per transaction accounting.
The changed_domain handling can be reused, but the array size of the
accounting data should be possible to be different for both use cases.
Signed-off-by: Juergen Gross
---
tools/xenstore/xenstored_core.c | 5 +++
tools/xenstore/xenstored_core.h | 3 ++
tools/xenstore/xenstored_domain.c | 64 +++
tools/xenstore/xenstored_domain.h | 5 ++-
4 files changed, 68 insertions(+), 9 deletions(-)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 27dfbe9593..2d10cacf35 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1023,6 +1023,9 @@ static void send_error(struct connection *conn, int error)
break;
}
}
+
+ acc_drop(conn);
+
send_reply(conn, XS_ERROR, xsd_errors[i].errstring,
strlen(xsd_errors[i].errstring) + 1);
}
@@ -1060,6 +1063,7 @@ void send_reply(struct connection *conn, enum
xsd_sockmsg_type type,
}
conn->in = NULL;
+ acc_commit(conn);
AFAIU, if send_reply() is called then we would need to commit the accounting
even if we can't send the reply (i.e. send_reply()). So shouldn't this be call
right at the beginning of send_reply()?
Oh, indeed. Good catch!
/* Update relevant header fields and fill in the message body. */
bdata->hdr.msg.type = type;
@@ -2195,6 +2199,7 @@ struct connection *new_connection(const struct
interface_funcs *funcs)
new->is_stalled = false;
new->transaction_started = 0;
INIT_LIST_HEAD(>out_list);
+ INIT_LIST_HEAD(>acc_list);
INIT_LIST_HEAD(>ref_list);
INIT_LIST_HEAD(>watches);
INIT_LIST_HEAD(>transaction_list);
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index c59b06551f..1f811f38cb 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -139,6 +139,9 @@ struct connection
struct list_head out_list;
uint64_t timeout_msec;
+ /* Not yet committed accounting data (valid if in != NULL). */
+ struct list_head acc_list;
+
/* Referenced requests no longer pending. */
struct list_head ref_list;
diff --git a/tools/xenstore/xenstored_domain.c
b/tools/xenstore/xenstored_domain.c
index f459c5aabb..33105dba8f 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -100,7 +100,7 @@ struct changed_domain
unsigned int domid;
/* Accounting data. */
- int acc[ACC_TR_N];
+ int acc[];
Is this actually worth it? How much memory would we save?
Hmm, true. While being more generic, the saved memory is actually zero,
as ACC_TR_N and ACC_REQ_N have the same value. And even if they should
differ in future, we can just use the higher of both values here.
};
static struct hashtable *domhash;
@@ -577,6 +577,7 @@ static struct changed_domain
*acc_find_changed_domain(struct list_head *head,
static struct changed_domain *acc_get_changed_domain(const void *ctx,
struct list_head *head,
+ enum accitem acc_sz,
unsigned int domid)
{
struct changed_domain *cd;
@@ -585,7 +586,7 @@ static struct changed_domain *acc_get_changed_domain(const
void *ctx,
if (cd)
return cd;
- cd = talloc_zero(ctx, struct changed_domain);
+ cd = talloc_zero_size(ctx, sizeof(*cd) + acc_sz * sizeof(cd->acc[0]));
if (!cd)
return NULL;
@@ -596,13 +597,13 @@ static struct changed_domain
*acc_get_changed_domain(const void *ctx,
}
static int acc_add_changed_dom(const void *ctx, struct list_head *head,
- enum accitem what, int val, unsigned int domid)
+ enum accitem acc_sz, enum accitem what,
+ int val, unsigned int domid)
{
struct changed_domain *cd;
- assert(what < ARRAY_SIZE(cd->acc));
-
- cd = acc_get_changed_domain(ctx, head, domid);
+ assert(what < acc_sz);
+ cd = acc_get_changed_domain(ctx, head, acc_sz, domid);
if (!cd)
return 0;
@@ -1071,6 +1072,7 @@ static int domain_acc_add(struct connection *conn,
unsigned int domid,
enum accitem what, int add, bool no_dom_alloc)
{
struct domain *d;
+ struct changed_domain *cd;
struct list_head *head;
int ret;
@@ -1091,10 +1093,26 @@ static int domain_acc_add(struct connection *conn,
unsigned int domid,
}
}
+ /* Temporary accounting data until final commit? */
+ if (conn && conn->in && what < ACC_REQ_N) {
+ /* Consider
Re: [PATCH v2 01/13] tools/xenstore: don't allow creating too many nodes in a transaction
On 20.02.23 19:01, Julien Grall wrote: On 20/02/2023 14:21, Juergen Gross wrote: On 20.02.23 15:15, Julien Grall wrote: On 20/02/2023 13:49, Juergen Gross wrote: On 20.02.23 13:07, Julien Grall wrote: Hi Juergen, On 20/02/2023 11:04, Juergen Gross wrote: On 20.02.23 10:46, Julien Grall wrote: Hi Juergen, On 20/01/2023 10:00, Juergen Gross wrote: The accounting for the number of nodes of a domain in an active transaction is not working correctly, as it allows to create arbitrary number of nodes. The transaction will finally fail due to exceeding the number of nodes quota, but before closing the transaction an unprivileged guest could cause Xenstore to use a lot of memory. I know I said I would delay my decision on this patch. However, I was still expecting the commit message to be updated based on our previous discussion. As said before, I was waiting on the settlement of our discussion before doing the update. This is not a very good practice to resend a patch without recording the disagreement because new reviewers may not be aware of the disagreement and this could end up to be committed by mistake... You said you wanted to look into this patch in detail after the previous series, so I move it into this series. The wording update would strongly depend on the outcome of the discussion IMO, so I didn't do it yet. This could have been mentioned after ---. This could allow people to understand the concern and then participate. Will do so in future. Also thinking more about it, "The transaction will finally fail due to exceeding the number of nodes quota" may not be true for a couple of reasons: 1) The transaction may removed a node afterwards. Yes, and? Just because it is a transaction, this is still a violation of the quota. Even outside a transaction you could use the same reasoning, but you don't (which is correct, of course). I can understand your point. However, to me, the goal of the transaction is to commit everything in one go at the end. So the violations in the middle should not matter. Of course they should. We wouldn't allow to write over-sized nodes, even if they could be rewritten in the same transaction with a smaller size. I view the two different. We wouldn't allow to create nodes which would violate overall memory consumption. We wouldn't allow nodes with more permission entries than allowed, even if they could be reduced in the same transaction again. I don't see why the number of nodes shouldn't be taken into account. Furthermore, I would expect a transaction to work on a snapshot of the system. So it feels really strange to me that we are constantly checking the quota with the updated values rather than the initial one. You are aware that the code without this patch is just neglecting the nodes created in the transaction? It just is using the current number of nodes outside any transaction for quota check. Are you referring to the quota check within the transaction? I'm referring to the quota check in create_node(). > So I could easily create a scenario where my new code will succeed, but the current one is failing: Assume node quota is 1000, and at start of the transaction the guest is owning 999 nodes. In the transaction the guest is deleting 10 nodes, then dom0 is creating 5 additional nodes owned by the guest. The central node counter is now 1004 (over quota), while the in-transaction count is 994. In the transaction the guest can now happily create a new node (#995) with my patch, but will fail to do so without (it sees the 1004 due to the transaction count being ignored). This doesn't sound correct. To do you have any test I could use to check the behavior? Try it: - create nodes in a guest until you hit the ENOSPC return code due to too many nodes - start a transaction deleting some nodes and then trying to create another one, which fail fail with ENOSPC. So I have recreated an XTF test which I think match what you wrote [1]. It is indeed failing without your patch. But then there are still some weird behavior here. I would expect the creation of the node would also fail if instead of removing the node if removed outside of the transaction. This is not the case because we are looking at the current quota. So shouldn't we snapshot the global count? As we don't do a global snapshot of the data base for a transaction (this was changed due to huge memory needs, bad performance, and a higher transaction failure rate), I don't think we should snapshot the count either. A transaction is performed atomically at the time it is finished. Therefore seeing the current global state inside the transaction (with the transaction private state on top like an overlay) is absolutely fine IMO. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
