Re: [PATCH 80/82] xen-netback: Refactor intentional wrap-around test
On 23.01.2024 01:27, Kees Cook wrote: > --- a/drivers/net/xen-netback/hash.c > +++ b/drivers/net/xen-netback/hash.c > @@ -345,7 +345,7 @@ u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, > u32 len, > .flags = GNTCOPY_source_gref > }}; > > - if ((off + len < off) || (off + len > vif->hash.size) || > + if ((add_would_overflow(off, len)) || (off + len > vif->hash.size) || I'm not maintainer of this code, but if I was I would ask that the excess parentheses be removed, to improve readability. Jan
Re: [PATCH v5] x86/livepatch: align functions to ensure minimal distance between entry points
On 22.01.2024 18:27, Roger Pau Monné wrote: > On Mon, Jan 22, 2024 at 12:21:47PM +0100, Jan Beulich wrote: >> On 22.01.2024 12:02, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/xen.lds.S >>> +++ b/xen/arch/x86/xen.lds.S >>> @@ -99,6 +99,10 @@ SECTIONS >>> *(.text) >>> #ifdef CONFIG_CC_SPLIT_SECTIONS >>> *(.text.*) >>> +#endif >>> +#ifdef CONFIG_FUNCTION_ALIGNMENT >>> + /* Ensure enough distance with the next placed section. */ >>> + . = ALIGN(CONFIG_FUNCTION_ALIGNMENT); >>> #endif >>> *(.text.__x86_indirect_thunk_*) >> >> I continue to fail to see how an alignment directive can guarantee minimum >> distance. In the worst case such a directive inserts nothing at all. > > I'm confused, you did provide a RB for this in v4: > > https://lore.kernel.org/xen-devel/4cad003f-dda0-4e22-a770-5a5ff56f4...@suse.com/ > > Which is basically the same code with a few comments and wording > adjustments. Hmm, yes. I think the aspect above was raised before, but then (perhaps) kind of addressed. (I'm puzzled then too: Why did you drop the R-b, when nothing substantially changed?) Yet re-reading the description, there's nothing said to this effect. Specifically ... >> IOW >> at the very least there's a non-spelled-out assumption here about the last >> item in the earlier section having suitable alignment and thus, if small >> in size, being suitably padded. > > Please bear with me, but I'm afraid I don't understand your concerns. > > For livepatch build tools (which is the only consumer of such > alignments) we already have the requirement that a function in order > to be suitable for being live patched must reside in it's own > section. > > We do want to aim for functions (even assembly ones) to live in their > own sections in order to be live patched, and to be properly aligned. > However it's also fine for functions to use a different (smaller) > alignment, the livepatch build tools will detect this and use the > alignment reported. ... I don't think this and ... > While we want to get to a point where everything that we care to patch > lives in it's own section, and is properly padded to ensure minimal > required space, I don't see why the proposed approach here should be > blocked, as it's a step in the right direction of achieving the > goal. > > Granted, there's still assembly code that won't be suitably padded, > but the livepatch build tools won't assume it to be padded. ... this is being pointed out. Which I think is relevant to make explicit not the least because the build tools aren't part of the main Xen tree. Plus many (like me) may not be overly familiar with how they work. > After > your series to enable assembly annotations we can also make sure the > assembly annotated functions live in separate sections and are > suitably aligned. > >> Personally I don't think merely spelling >> out such a requirement would help - it would end up being a trap for >> someone to fall into. > >> I'm further curious why .text.__x86_indirect_thunk_* is left past the >> inserted alignment. While pretty unlikely, isn't it in principle possible >> for the thunks there to also need patching? Aren't we instead requiring >> then that assembly functions (and thunks) all be suitably aligned as well? > > Those are defined in assembly, so requires CONFIG_FUNCTION_ALIGNMENT > to also be applied to the function entry points in assembly files. I see. Yet the question then remains: Why is the alignment not inserted after them? Or will the insertion need to move later on (which would feel odd)? Jan
Re: Community Process Group - Proposal
On 22.01.2024 23:47, Stefano Stabellini wrote: > On Mon, 22 Jan 2024, Jan Beulich wrote: >> What definitely needs clarifying is what "review" is: Are R-b tags >> counted, or is it the number of replies sent commenting on patches? > > Yes, I think this needs to be clarified. I would say Reviewed-by tags. Which may end up unfair. It's not uncommon for one person to do a lot of review on a patch, and for someone else to then ack the final version that goes in. In the end this is then no different from basing the decision on simple numbers, without regard to actual (potentially heavily differing) effort behind each individual instance. Jan
[linux-linus test] 184427: tolerable FAIL - PUSHED
flight 184427 linux-linus real [real] flight 184430 linux-linus real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/184427/ http://logs.test-lab.xenproject.org/osstest/logs/184430/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-arm64-arm64-xl-credit2 18 guest-start/debian.repeat fail pass in 184430-retest Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 184422 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 184422 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 184422 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 184422 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 184422 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 184422 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 184422 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 184422 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-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-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-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass version targeted for testing: linux610347effc2ecb5ededf5037e82240b151f883ab baseline version: linux6613476e225e090cc9aad49be7fa504e290dd33d Last test of basis 184422 2024-01-22 03:31:56 Z1 days Testing same since 184427 2024-01-22 18:10:39 Z0 days1 attempts People who touched revisions under test: Gustavo A. R. Silva Jan Beulich Linus Torvalds jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf
Re: Ping: [PATCH v5 6/8] PPC: switch entry point annotations to common model
Hi Jan, On 1/22/24 7:20 AM, Jan Beulich wrote: > On 15.01.2024 15:38, Jan Beulich wrote: >> Use the generic framework in xen/linkage.h. No change in generated code >> except of course the converted symbols change to be hidden ones. >> >> Signed-off-by: Jan Beulich > > The other architectures have been at least partly switched; would be nice > for PPC to follow suit. May I ask for an ack (or otherwise here), please? > Sorry for the delay. Acked-by: Shawn Anastasio > Thanks, Jan Thanks, Shawn
[xen-unstable test] 184426: tolerable FAIL - PUSHED
flight 184426 xen-unstable real [real] flight 184428 xen-unstable real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/184426/ http://logs.test-lab.xenproject.org/osstest/logs/184428/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-armhf-armhf-xl-vhd 13 guest-start fail pass in 184428-retest Tests which did not succeed, but are not blocking: test-armhf-armhf-xl-vhd 14 migrate-support-check fail in 184428 never pass test-armhf-armhf-xl-vhd 15 saverestore-support-check fail in 184428 never pass test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 184419 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 184419 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 184419 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 184419 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 184419 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 184419 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 184419 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 184419 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 184419 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 184419 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 184419 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 184419 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt-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-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-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass version targeted for testing: xen 4900c939cb9b876c51cfc7a4c854f54c722a30b5 baseline version: xen b25607e528f6ce7851e907ed59ad5ff583aa1840 Last test of basis 184419 2024-01-22 01:53:59 Z
[PATCH 80/82] xen-netback: Refactor intentional wrap-around test
In an effort to separate intentional arithmetic wrap-around from unexpected wrap-around, we need to refactor places that depend on this kind of math. One of the most common code patterns of this is: VAR + value < VAR Notably, this is considered "undefined behavior" for signed and pointer types, which the kernel works around by using the -fno-strict-overflow option in the build[1] (which used to just be -fwrapv). Regardless, we want to get the kernel source to the position where we can meaningfully instrument arithmetic wrap-around conditions and catch them when they are unexpected, regardless of whether they are signed[2], unsigned[3], or pointer[4] types. Refactor open-coded wrap-around addition test to use add_would_overflow(). This paves the way to enabling the wrap-around sanitizers in the future. Link: https://git.kernel.org/linus/68df3755e383e6fecf2354a67b08f92f18536594 [1] Link: https://github.com/KSPP/linux/issues/26 [2] Link: https://github.com/KSPP/linux/issues/27 [3] Link: https://github.com/KSPP/linux/issues/344 [4] Cc: Wei Liu Cc: Paul Durrant Cc: "David S. Miller" Cc: Eric Dumazet Cc: Jakub Kicinski Cc: Paolo Abeni Cc: xen-devel@lists.xenproject.org Cc: net...@vger.kernel.org Signed-off-by: Kees Cook --- drivers/net/xen-netback/hash.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/xen-netback/hash.c b/drivers/net/xen-netback/hash.c index ff96f22648ef..69b03b4feba9 100644 --- a/drivers/net/xen-netback/hash.c +++ b/drivers/net/xen-netback/hash.c @@ -345,7 +345,7 @@ u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, u32 len, .flags = GNTCOPY_source_gref }}; - if ((off + len < off) || (off + len > vif->hash.size) || + if ((add_would_overflow(off, len)) || (off + len > vif->hash.size) || len > XEN_PAGE_SIZE / sizeof(*mapping)) return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER; -- 2.34.1
Re: [RFC KERNEL PATCH v4 3/3] PCI/sysfs: Add gsi sysfs for pci_dev
On Fri, Jan 05, 2024 at 02:22:17PM +0800, Jiqian Chen wrote:
> There is a need for some scenarios to use gsi sysfs.
> For example, when xen passthrough a device to dumU, it will
> use gsi to map pirq, but currently userspace can't get gsi
> number.
> So, add gsi sysfs for that and for other potential scenarios.
Isn't GSI really an ACPI-specific concept?
I don't know enough about Xen to know why it needs the GSI in
userspace. Is this passthrough brand new functionality that can't be
done today because we don't expose the GSI yet?
How does userspace use the GSI? I see "to map pirq", but I think we
should have more concrete details about exactly what is needed and how
it is used before adding something new in sysfs.
Is there some more generic kernel interface we could use
for this?
s/dumU/DomU/ ? (I dunno, but https://www.google.com/search?q=xen+dumu
suggests it :))
> Co-developed-by: Huang Rui
> Signed-off-by: Jiqian Chen
> ---
> drivers/acpi/pci_irq.c | 1 +
> drivers/pci/pci-sysfs.c | 11 +++
> include/linux/pci.h | 2 ++
> 3 files changed, 14 insertions(+)
>
> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
> index 630fe0a34bc6..739a58755df2 100644
> --- a/drivers/acpi/pci_irq.c
> +++ b/drivers/acpi/pci_irq.c
> @@ -449,6 +449,7 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
> kfree(entry);
> return 0;
> }
> + dev->gsi = gsi;
>
> rc = acpi_register_gsi(>dev, gsi, triggering, polarity);
> if (rc < 0) {
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 2321fdfefd7d..c51df88d079e 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -71,6 +71,16 @@ static ssize_t irq_show(struct device *dev,
> }
> static DEVICE_ATTR_RO(irq);
>
> +static ssize_t gsi_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> +
> + return sysfs_emit(buf, "%u\n", pdev->gsi);
> +}
> +static DEVICE_ATTR_RO(gsi);
> +
> static ssize_t broken_parity_status_show(struct device *dev,
>struct device_attribute *attr,
>char *buf)
> @@ -596,6 +606,7 @@ static struct attribute *pci_dev_attrs[] = {
> _attr_revision.attr,
> _attr_class.attr,
> _attr_irq.attr,
> + _attr_gsi.attr,
> _attr_local_cpus.attr,
> _attr_local_cpulist.attr,
> _attr_modalias.attr,
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index dea043bc1e38..0618d4a87a50 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -529,6 +529,8 @@ struct pci_dev {
>
> /* These methods index pci_reset_fn_methods[] */
> u8 reset_methods[PCI_NUM_RESET_METHODS]; /* In priority order */
> +
> + unsigned intgsi;
> };
>
> static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
> --
> 2.34.1
>
>
Re: Community Process Group - Proposal
I am only addressing a couple of Jan's points below. On Mon, 22 Jan 2024, Jan Beulich wrote: > > A CPG will be your second point of call, where you can escalate matters > > quickly for a democratic solution. > > Between informal voting and this "second point of call", where does > formal voting go? Formal voting is still available as "third point of call" after informal voting and CPG. > > *How are members selected?* > > The CPG will be composed of 5 randomly selected members in total. > > An odd number has been purposely selected to avoid an impasse during > > decisions. > > > > The criteria: > > Individual members must be active contributors and are willing to help the > > community succeed. As such they must be a part of the following groups: > > > >- Committers > >- Active Maintainers: maintainers with >= 20 reviews in the last 2 > >releases > >- Active Contributors: contributors with >= 10 commits in the last 2 > >releases > > I'm afraid I can't leave this uncommented, as matching a common pattern > I'm generally unhappy with. Whatever the numbers you select in such > criteria, they'll open up an easy road for faking. At the same time it > of course is difficult to come up with any non-numeric or not-only- > numeric criteria. For example, I'd be heavily inclined to ask that > "non-trivial" be added to both of the numbers. Yet then there arises a > judgement issue: What's non-trivial can be entirely different > depending on who you ask. I share your observations and thoughts on the matter. I understand and share your thinking about adding "non-trivial" but then it is becomes a judgment call, as you wrote. I think it would be best if the criteria doesn't require human judgment. > What definitely needs clarifying is what "review" is: Are R-b tags > counted, or is it the number of replies sent commenting on patches? Yes, I think this needs to be clarified. I would say Reviewed-by tags.
Thoughts on current Xen EDAC/MCE situation
I've been mentioning this on a regular basis, but the state of MCE handling with Xen seems poor. I find the present handling of MCE in Xen an odd choice. Having Xen do most of the handling of MCE events is a behavior matching a traditional stand-alone hypervisor. Yet Xen was originally pushing any task not requiring hypervisor action onto Domain 0. MCE seems a perfect match for sharing responsibility with Domain 0. Domain 0 needs to know about any MCE event, this is where system administrators will expect to find logs. In fact, if the event is a Correctable Error, then *only* Domain 0 needs to know. For a CE, Xen may need no action at all (an implementation could need help) and the effected domain would need no action. It is strictly for Uncorrectable Errors that action beside logging is needed. For a UE memory error, the best approach might be for Domain 0 to decode the error. Once Domain 0 determines it is UE, invoke a hypercall to pass the GPFN to Xen. Xen would then forcibly unmap the page (similar to what Linux does to userspace for corrupted pages). Xen would then identify what the page was used for, alert the domain and return that to Domain 0. The key advantage of this approach is it makes MCE handling act very similar to MCE handling without Xen. Documentation about how MCEs are reported/decoded would apply equally to Xen. Another rather important issue is it means less maintenance work to keep MCE handling working with cutting-edge hardware. I've noticed one vendor being sluggish about getting patches into Linux and I fear similar issues may apply more severely to Xen. -- (\___(\___(\__ --=> 8-) EHM <=-- __/)___/)___/) \BS (| ehem+sig...@m5p.com PGP 87145445 |) / \_CS\ | _ -O #include O- _ | / _/ 8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445
[PATCH] pmstat: Limit hypercalls under HWP
When HWP is active, the cpufreq P-state information is not updated. In
that case, return -ENODEV instead of bogus, incomplete info. The xenpm
command already supports skipping results when -ENODEV is returned, so
it is re-used when -EOPNOTSUPP might be more accurate.
Similarly, set_cpufreq_para() is not applicable when HWP is active.
Many of the options already checked the governor and were inaccessible,
but SCALING_MIN/MAX_FREQ was still accessible (though it would do
nothing). Add an ealier HWP check to handle all cases.
Signed-off-by: Jason Andryuk
---
`xenpm get-cpufreq-states` now doesn't return any output. It also exits
successfully since xenpm doesn't check the returns there. Other
commands will fail:
xenpm set-scaling-maxfreq 11 110
failed to set scaling max freq (95 - Operation not supported)
xen/drivers/acpi/pmstat.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c
index 85097d463c..4c4d298d1c 100644
--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -66,6 +66,8 @@ int do_get_pm_info(struct xen_sysctl_get_pmstat *op)
return -ENODEV;
if ( !cpufreq_driver.init )
return -ENODEV;
+if ( hwp_active() )
+return -ENODEV;
if ( !pmpt || !(pmpt->perf.init & XEN_PX_INIT) )
return -EINVAL;
break;
@@ -329,6 +331,9 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op)
if ( !policy || !policy->governor )
return -EINVAL;
+if ( hwp_active() )
+return -EOPNOTSUPP;
+
switch(op->u.set_para.ctrl_type)
{
case SCALING_MAX_FREQ:
--
2.43.0
Re: [PATCH 2/3] x86/entry: Make #PF/NMI more amenable to livepatching
On 22/01/2024 6:17 pm, Andrew Cooper wrote: > It is bad form to have inter-function fallthrough. It only functions right > now because alignment padding bytes are NOPs. > > However, it also interferes with livepatching binary diffs, because the > implicit grouping of the two functions isn't expressed in the ELF metadata. > > Signed-off-by: Andrew Cooper Please disregard this, and look at the other patch 2. ~Andrew
Xen Security Advisory 448 v2 (CVE-2023-46838) - Linux: netback processing of zero-length transmit fragment
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 Xen Security Advisory CVE-2023-46838 / XSA-448 version 2 Linux: netback processing of zero-length transmit fragment UPDATES IN VERSION 2 Public release. ISSUE DESCRIPTION = Transmit requests in Xen's virtual network protocol can consist of multiple parts. While not really useful, except for the initial part any of them may be of zero length, i.e. carry no data at all. Besides a certain initial portion of the to be transferred data, these parts are directly translated into what Linux calls SKB fragments. Such converted request parts can, when for a particular SKB they are all of length zero, lead to a de-reference of NULL in core networking code. IMPACT == An unprivileged guest can cause Denial of Service (DoS) of the host by sending network packets to the backend, causing the backend to crash. Data corruption or privilege escalation have not been ruled out. VULNERABLE SYSTEMS == All systems using a Linux based network backend with kernel 4.14 and newer are vulnerable. Earlier versions may also be vulnerable. Systems using other network backends are not known to be vulnerable. MITIGATION == Using a userspace PV network backend (e.g. the qemu based "qnic" backend) will mitigate the problem. Using a dedicated network driver domain per guest will mitigate the problem. CREDITS === This issue was discovered by Pratyush Yadav of Amazon. RESOLUTION == Applying the attached patch resolves this issue. xsa448-linux.patch Linux 6.7-rc - 6.5 $ sha256sum xsa448* f8c87cf546c2bc70970ca151c0ec8c1940f969e29c4cb3d2ec37ff9e43ddfc36 xsa448-linux.patch $ NOTE CONCERNING EARLY DISCLOSURE The embargo was intended to be 2024-01-23 12:00 UTC, but a downstream had a mixup of days and published early. DEPLOYMENT DURING EMBARGO = Deployment of the patches and/or mitigations described above (or others which are substantially similar) is permitted during the embargo, even on public-facing systems with untrusted guest users and administrators. But: Distribution of updated software is prohibited (except to other members of the predisclosure list). Predisclosure list members who wish to deploy significantly different patches and/or mitigations, please contact the Xen Project Security Team. (Note: this during-embargo deployment notice is retained in post-embargo publicly released Xen Project advisories, even though it is then no longer applicable. This is to enable the community to have oversight of the Xen Project Security Team's decisionmaking.) For more information about permissible uses of embargoed information, consult the Xen Project community's agreed Security Policy: http://www.xenproject.org/security-policy.html -BEGIN PGP SIGNATURE- iQFABAEBCAAqFiEEI+MiLBRfRHX6gGCng/4UyVfoK9kFAmWutGMMHHBncEB4ZW4u b3JnAAoJEIP+FMlX6CvZ9h0H/26sgfTHO0vnTZ8cnisn3aC5VTvrx9nY5fcCe2cJ /KgN3q3mtb3w41/2LD/rR0Zpw4SkeTaFp69Mz2hQa37gLVDSK5lDJDR61lwhiwrQ MSsdPHs91EDJhF6aX/S7wsQkBZYPq1S9aOuIxJbDYN3D9WsTUWvuocXNxeqTx5q9 iWVSJTH5NkRSAaIVldyNVkQ7pWaSrwqmBzolnrZIsDUjYU1Lk/j0u6GFbkOF9SIg onFiFbJhCOaIZOIP2Tfz7nHGBnxucI4cjjwy4BWM+Va35Pg4mbHaBuVGnQsaBtVF UdY6/jw6Qk4ktV34il3+jlgGfAFC6GILJoraASjaFCEQ7jM= =IPLz -END PGP SIGNATURE- xsa448-linux.patch Description: Binary data
[PATCH 2/3] x86/entry: Make #PF/NMI more amenable to livepatching
It is bad form to have inter-function fallthrough. It only functions right now because alignment padding bytes are NOPs. However, it also interferes with livepatching binary diffs, because the implicit grouping of the two functions isn't expressed in the ELF metadata. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Roger Pau Monné CC: Wei Liu CC: Konrad Rzeszutek Wilk CC: Ross Lagerwall --- xen/arch/x86/x86_64/entry.S | 3 +++ 1 file changed, 3 insertions(+) diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S index c3f6b667a72a..fc64ef1fd460 100644 --- a/xen/arch/x86/x86_64/entry.S +++ b/xen/arch/x86/x86_64/entry.S @@ -723,7 +723,9 @@ END(common_interrupt) FUNC(entry_PF) ENDBR64 movl $X86_EXC_PF, 4(%rsp) +jmp handle_exception END(entry_PF) + /* No special register assumptions. */ FUNC(handle_exception, 0) ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP @@ -1023,6 +1025,7 @@ FUNC(entry_NMI) ENDBR64 pushq $0 movl $X86_EXC_NMI, 4(%rsp) +jmp handle_ist_exception END(entry_NMI) FUNC(handle_ist_exception) -- 2.30.2
[PATCH 3/3] x86/entry: Make intra-funciton symbols properly local
Each of these symbols are local to their main function. By not having them globally visible, livepatch's binary diffing logic can reason about the functions properly. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Roger Pau Monné CC: Wei Liu CC: Konrad Rzeszutek Wilk CC: Ross Lagerwall sysenter_eflags_saved() is an open question. This does need to be globally visible, and I think any livepatch modifying sysenter_entry() will need a manual adjustment to do_debug() to update the reference there. --- xen/arch/x86/x86_64/compat/entry.S | 20 ++-- xen/arch/x86/x86_64/entry.S| 22 +++--- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S index 4fbd89cea1a9..1e9e0455eaf3 100644 --- a/xen/arch/x86/x86_64/compat/entry.S +++ b/xen/arch/x86/x86_64/compat/entry.S @@ -41,7 +41,7 @@ FUNC(compat_test_all_events) shll $IRQSTAT_shift,%eax leaq irq_stat+IRQSTAT_softirq_pending(%rip),%rcx cmpl $0,(%rcx,%rax,1) -jne compat_process_softirqs +jne .L_compat_process_softirqs /* Inject exception if pending. */ lea VCPU_trap_bounce(%rbx), %rdx @@ -49,11 +49,11 @@ FUNC(compat_test_all_events) jnz .Lcompat_process_trapbounce cmpb $0, VCPU_mce_pending(%rbx) -jne compat_process_mce +jne .L_compat_process_mce .Lcompat_test_guest_nmi: cmpb $0, VCPU_nmi_pending(%rbx) -jne compat_process_nmi -compat_test_guest_events: +jne .L_compat_process_nmi +.L_compat_test_guest_events: movq VCPU_vcpu_info(%rbx),%rax movzwl COMPAT_VCPUINFO_upcall_pending(%rax),%eax decl %eax @@ -71,7 +71,7 @@ compat_test_guest_events: jmp compat_test_all_events /* %rbx: struct vcpu */ -LABEL_LOCAL(compat_process_softirqs) +LABEL_LOCAL(.L_compat_process_softirqs) sti call do_softirq jmp compat_test_all_events @@ -84,7 +84,7 @@ LABEL_LOCAL(.Lcompat_process_trapbounce) jmp compat_test_all_events /* %rbx: struct vcpu */ -LABEL_LOCAL(compat_process_mce) +LABEL_LOCAL(.L_compat_process_mce) testb $1 << VCPU_TRAP_MCE,VCPU_async_exception_mask(%rbx) jnz .Lcompat_test_guest_nmi sti @@ -96,12 +96,12 @@ LABEL_LOCAL(compat_process_mce) movb %dl,VCPU_mce_old_mask(%rbx)# iret hypercall orl $1 << VCPU_TRAP_MCE,%edx movb %dl,VCPU_async_exception_mask(%rbx) -jmp compat_process_trap +jmp .L_compat_process_trap /* %rbx: struct vcpu */ -LABEL_LOCAL(compat_process_nmi) +LABEL_LOCAL(.L_compat_process_nmi) testb $1 << VCPU_TRAP_NMI,VCPU_async_exception_mask(%rbx) -jnz compat_test_guest_events +jnz .L_compat_test_guest_events sti movb $0, VCPU_nmi_pending(%rbx) call set_guest_nmi_trapbounce @@ -112,7 +112,7 @@ LABEL_LOCAL(compat_process_nmi) orl $1 << VCPU_TRAP_NMI,%edx movb %dl,VCPU_async_exception_mask(%rbx) /* FALLTHROUGH */ -compat_process_trap: +.L_compat_process_trap: leaq VCPU_trap_bounce(%rbx),%rdx call compat_create_bounce_frame jmp compat_test_all_events diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S index fc64ef1fd460..130462ba0e1a 100644 --- a/xen/arch/x86/x86_64/entry.S +++ b/xen/arch/x86/x86_64/entry.S @@ -188,7 +188,7 @@ FUNC_LOCAL(restore_all_guest) RESTORE_ALL testw $TRAP_syscall,4(%rsp) -jziret_exit_to_guest +jz.L_iret_exit_to_guest movq 24(%rsp),%r11 # RFLAGS andq $~(X86_EFLAGS_IOPL | X86_EFLAGS_VM), %r11 @@ -220,7 +220,7 @@ FUNC_LOCAL(restore_all_guest) LABEL_LOCAL(.Lrestore_rcx_iret_exit_to_guest) movq 8(%rsp), %rcx # RIP /* No special register assumptions. */ -iret_exit_to_guest: +.L_iret_exit_to_guest: andl $~(X86_EFLAGS_IOPL | X86_EFLAGS_VM), 24(%rsp) orl $X86_EFLAGS_IF,24(%rsp) addq $8,%rsp @@ -432,10 +432,10 @@ UNLIKELY_END(msi_check) cmove %rdi, %rdx test %rdx, %rdx -jzint80_slow_path +jz.L_int80_slow_path #else test %rdi, %rdi -jzint80_slow_path +jz.L_int80_slow_path #endif /* Construct trap_bounce from trap_ctxt[0x80]. */ @@ -457,7 +457,7 @@ UNLIKELY_END(msi_check) call create_bounce_frame jmp test_all_events -int80_slow_path: +.L_int80_slow_path: /* * Setup entry vector and error code as if this was a GPF caused by an * IDT entry with DPL==0. @@ -472,7 +472,7 @@ int80_slow_path: * need to set up %r14 here, while %r15 is required to still be zero. */ GET_STACK_END(14) -jmp handle_exception_saved +jmp .L_handle_exception_saved END(entry_int80)
[PATCH 1/3] x86/entry: Fix ELF metadata for NMI and handle_ist_exception
handle_ist_exception isn't part of the NMI handler, just like handle_exception
isn't part of #PF.
Fixes: b3a9037550df ("x86: annotate entry points with type and size")
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Roger Pau Monné
CC: Wei Liu
CC: Konrad Rzeszutek Wilk
CC: Ross Lagerwall
---
xen/arch/x86/x86_64/entry.S | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 482c91d4f533..c3f6b667a72a 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -1023,7 +1023,9 @@ FUNC(entry_NMI)
ENDBR64
pushq $0
movl $X86_EXC_NMI, 4(%rsp)
-handle_ist_exception:
+END(entry_NMI)
+
+FUNC(handle_ist_exception)
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
SAVE_ALL
@@ -1150,7 +1152,7 @@ handle_ist_exception:
ASSERT_CONTEXT_IS_XEN
jmp restore_all_xen
#endif
-END(entry_NMI)
+END(handle_ist_exception)
FUNC(entry_MC)
ENDBR64
--
2.30.2
[PATCH 2/3] x86/entry: Make #PF/NMI/INT0x82 more amenable to livepatching
It is bad form to have inter-function fallthrough. It only functions right now because alignment padding bytes are NOPs. However, it also interferes with livepatching binary diffs, because the implicit grouping of the two functions isn't expressed in the ELF metadata. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Roger Pau Monné CC: Wei Liu CC: Konrad Rzeszutek Wilk CC: Ross Lagerwall --- xen/arch/x86/x86_64/compat/entry.S | 1 + xen/arch/x86/x86_64/entry.S| 3 +++ 2 files changed, 4 insertions(+) diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S index 49811a56e965..4fbd89cea1a9 100644 --- a/xen/arch/x86/x86_64/compat/entry.S +++ b/xen/arch/x86/x86_64/compat/entry.S @@ -29,6 +29,7 @@ FUNC(entry_int82) mov %rsp, %rdi call do_entry_int82 +jmp compat_test_all_events END(entry_int82) /* %rbx: struct vcpu */ diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S index c3f6b667a72a..fc64ef1fd460 100644 --- a/xen/arch/x86/x86_64/entry.S +++ b/xen/arch/x86/x86_64/entry.S @@ -723,7 +723,9 @@ END(common_interrupt) FUNC(entry_PF) ENDBR64 movl $X86_EXC_PF, 4(%rsp) +jmp handle_exception END(entry_PF) + /* No special register assumptions. */ FUNC(handle_exception, 0) ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP @@ -1023,6 +1025,7 @@ FUNC(entry_NMI) ENDBR64 pushq $0 movl $X86_EXC_NMI, 4(%rsp) +jmp handle_ist_exception END(entry_NMI) FUNC(handle_ist_exception) -- 2.30.2
[PATCH 0/3] x86/entry: ELF fixes and improvments
Patch 1 is a bugfix. Patches 2 and 3 are to improve livepatchability. Andrew Cooper (3): x86/entry: Fix ELF metadata for NMI and handle_ist_exception x86/entry: Make #PF/NMI/INT0x82 more amenable to livepatching x86/entry: Make intra-funciton symbols properly local xen/arch/x86/x86_64/compat/entry.S | 21 ++-- xen/arch/x86/x86_64/entry.S| 31 +- 2 files changed, 29 insertions(+), 23 deletions(-) base-commit: 4900c939cb9b876c51cfc7a4c854f54c722a30b5 -- 2.30.2
Re: [PATCH v5 8/8] common: honor CONFIG_CC_SPLIT_SECTIONS also for assembly functions
On Mon, Jan 22, 2024 at 11:50:08AM +0100, Jan Beulich wrote: > On 19.01.2024 11:36, Roger Pau Monné wrote: > > On Mon, Jan 15, 2024 at 03:40:19PM +0100, Jan Beulich wrote: > >> Leverage the new infrastructure in xen/linkage.h to also switch to per- > >> function sections (when configured), deriving the specific name from the > >> "base" section in use at the time FUNC() is invoked. > >> > >> Signed-off-by: Jan Beulich > >> --- > >> TBD: Since we use .subsection in UNLIKELY_START(), a perhaps not really > >> wanted side effect of this change is that respective out-of-line > >> code now moves much closer to its original (invoking) code. > > > > Hm, I'm afraid I don't have much useful suggestions here. > > > >> TBD: Of course something with the same overall effect, but less > >> impactful might do in Config.mk. E.g. $(filter-out -D%,$(3)) > >> instead of $(firstword (3)). > > > > I don't have a strong opinion re those two options My preference > > however (see below for reasoning) would be to put this detection in > > Kconfig. > > > >> TBD: On top of Roger's respective patch (for livepatch), also respect > >> CONFIG_FUNCTION_ALIGNMENT. > > > > I think you can drop that, as the series is blocked. > > Considering the series here has been pending for quite some time, too, > I guess I'd like to keep it just in case that other functionality > becomes unblocked or available by some other means, even if only to > remind myself. So as you have seen I've posted a new version of just the function alignment patch, that I expected wasn't controversial. > >> --- a/xen/Makefile > >> +++ b/xen/Makefile > >> @@ -409,6 +409,9 @@ AFLAGS += -D__ASSEMBLY__ > >> > >> $(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--noexecstack) > >> > >> +# Check to see whether the assmbler supports the --sectname-subst option. > >> +$(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--sectname-subst > >> -DHAVE_AS_SECTNAME_SUBST) > > > > I guess you already know what I'm going to comment on. I think this > > would be clearer if it was a Kconfig option. For once because I think > > we should gate livapatch support on the option being available, and > > secondly it would avoid having to pass the extra -D around. > > > > I think it's relevant to have a consistent set of build tool options > > requirements for livepatch support, so that when enabled the support > > is consistent across builds. With this approach livepatch could be > > enabled in Kconfig, but depending on the tools support the resulting > > binary might or might not support live patching of assembly code. > > Such behavior is IMO unhelpful from a user PoV, and can lead to > > surprises down the road. > > I can see the desire to have LIVEPATCH grow such a dependency. Yet there > is the bigger still open topic of the criteria towards what, if anything, > to probe for in Kconfig, what, if anything, to probe for in Makefile, and > which of the probing perhaps do in both places. I'm afraid my attempts to > move us closer to a resolution (topic on summit, making proposals on > list) have utterly failed. IOW I don't currently see how the existing > disagreement there can be resolved, which will result in me to continue > following the (traditional) Makefile approach unless I clearly view > Kconfig superior in a particular case. I could perhaps be talked into > following a "mixed Kconfig/Makefile model", along the lines of "x86: > convert CET tool chain feature checks to mixed Kconfig/Makefile model", > albeit I'm sure you're aware there are issues to sort out there, which I > see no value in putting time into as long as I can't expect things to > make progress subsequently. I think there are more subtle cases where it's indeed arguable that putting it in Kconfig or a Makefile makes no difference from a user experience PoV, hence in the context here I do believe it wants to be in Kconfig as LIVEPATCH can be make dependent on it. > Dropping this patch, while an option, would seem undesirable to me, since > even without Kconfig probing using new enough tool chains the splitting > would yield reliable results, and provide - imo - an improvement over > what we have right now. I could send a followup afterwards to arrange for the check to be in Kconfig, but it would feel a bit odd to me this is not done in the first place. I don't want to block the patch because I think it's useful, so worst case I'm willing to give my Ack and provide an alternative Kconfig based patch afterwards. Thanks, Roger.
Re: [PATCH v5] x86/livepatch: align functions to ensure minimal distance between entry points
On Mon, Jan 22, 2024 at 12:21:47PM +0100, Jan Beulich wrote: > On 22.01.2024 12:02, Roger Pau Monne wrote: > > The minimal function size requirements for an x86 livepatch are either 5 > > bytes > > (for jmp) or 9 bytes (for endbr + jmp), and always 4 bytes on Arm. Ensure > > that > > distance between functions entry points is always at least of the minimal > > required size for livepatch instruction replacement to be successful. > > > > Add an additional align directive to the linker script, in order to ensure > > that > > the next section placed after the .text.* (per-function sections) is also > > aligned to the required boundary, so that the distance of the last function > > entry point with the next symbol is also of minimal size. > > > > Note that it's possible for the compiler to end up using a higher function > > alignment regardless of the passed value, so this change just make sure that > > the minimum required for livepatch to work is present. Different compilers > > handle the option differently, as clang will ignore -falign-functions value > > if it's smaller than the one that would be set by the optimization level, > > while > > gcc seems to always honor the function alignment passed in > > -falign-functions. > > In order to cope with this behavior and avoid that setting -falign-functions > > results in an alignment inferior to what the optimization level would have > > selected force x86 release builds to use a function alignment of 16 bytes. > > Nit: A comma after "selected" may help readability. > > > --- a/xen/Kconfig > > +++ b/xen/Kconfig > > @@ -37,6 +37,27 @@ config CC_HAS_VISIBILITY_ATTRIBUTE > > config CC_SPLIT_SECTIONS > > bool > > > > +# Set function alignment. > > +# > > +# Allow setting on a boolean basis, and then convert such selection to an > > +# integer for the build system and code to consume more easily. > > +# > > +# Requires clang >= 7.0.0 > > +config CC_HAS_FUNCTION_ALIGNMENT > > + def_bool $(cc-option,-falign-functions) > > Nit: Maybe better have a blank line here? > > > +config FUNCTION_ALIGNMENT_4B > > + bool > > +config FUNCTION_ALIGNMENT_8B > > + bool > > +config FUNCTION_ALIGNMENT_16B > > + bool > > +config FUNCTION_ALIGNMENT > > + int > > + depends on CC_HAS_FUNCTION_ALIGNMENT > > + default 16 if FUNCTION_ALIGNMENT_16B > > + default 8 if FUNCTION_ALIGNMENT_8B > > + default 4 if FUNCTION_ALIGNMENT_4B > > + > > source "arch/$(SRCARCH)/Kconfig" > > > > config DEFCONFIG_LIST > >[...] > > --- a/xen/arch/x86/xen.lds.S > > +++ b/xen/arch/x86/xen.lds.S > > @@ -99,6 +99,10 @@ SECTIONS > > *(.text) > > #ifdef CONFIG_CC_SPLIT_SECTIONS > > *(.text.*) > > +#endif > > +#ifdef CONFIG_FUNCTION_ALIGNMENT > > + /* Ensure enough distance with the next placed section. */ > > + . = ALIGN(CONFIG_FUNCTION_ALIGNMENT); > > #endif > > *(.text.__x86_indirect_thunk_*) > > I continue to fail to see how an alignment directive can guarantee minimum > distance. In the worst case such a directive inserts nothing at all. I'm confused, you did provide a RB for this in v4: https://lore.kernel.org/xen-devel/4cad003f-dda0-4e22-a770-5a5ff56f4...@suse.com/ Which is basically the same code with a few comments and wording adjustments. > IOW > at the very least there's a non-spelled-out assumption here about the last > item in the earlier section having suitable alignment and thus, if small > in size, being suitably padded. Please bear with me, but I'm afraid I don't understand your concerns. For livepatch build tools (which is the only consumer of such alignments) we already have the requirement that a function in order to be suitable for being live patched must reside in it's own section. We do want to aim for functions (even assembly ones) to live in their own sections in order to be live patched, and to be properly aligned. However it's also fine for functions to use a different (smaller) alignment, the livepatch build tools will detect this and use the alignment reported. While we want to get to a point where everything that we care to patch lives in it's own section, and is properly padded to ensure minimal required space, I don't see why the proposed approach here should be blocked, as it's a step in the right direction of achieving the goal. Granted, there's still assembly code that won't be suitably padded, but the livepatch build tools won't assume it to be padded. After your series to enable assembly annotations we can also make sure the assembly annotated functions live in separate sections and are suitably aligned. > Personally I don't think merely spelling > out such a requirement would help - it would end up being a trap for > someone to fall into. > I'm further curious why .text.__x86_indirect_thunk_* is left past the > inserted alignment. While pretty unlikely, isn't it in principle possible > for the thunks there to also need patching? Aren't we instead requiring > then that assembly
[ovmf test] 184425: all pass - PUSHED
flight 184425 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/184425/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 313f9f01552227138e08a7a7f44be48e5ba20a99 baseline version: ovmf 0b09397dfa0123b9a27c2c52fd2ddafd7a902137 Last test of basis 184423 2024-01-22 11:14:34 Z0 days Testing same since 184425 2024-01-22 15:42:46 Z0 days1 attempts People who touched revisions under test: Michael Kubacki jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass test-amd64-i386-xl-qemuu-ovmf-amd64 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git 0b09397dfa..313f9f0155 313f9f01552227138e08a7a7f44be48e5ba20a99 -> xen-tested-master
Re: [BUG]i2c_hid_acpi broken with 4.17.2 on Framework Laptop 13 AMD
Le mer. 17 janv. 2024 à 03:20, Mario Limonciello a écrit : > On 1/16/2024 10:18, Jan Beulich wrote: > > On 16.01.2024 16:52, Sébastien Chaumat wrote: > >> Le mar. 2 janv. 2024 à 21:23, Sébastien Chaumat a > >> écrit : > >> > >>> > >>> output of gpioinfo > > kernel alone : > > line 5: unnamed input active-low consumer=interrupt > line 84: unnamed input active-low consumer=interrupt > > xen: > > line 5: unnamed input active-low > line 84: unnamed input active-low > > xen with skipping IRQ7 double init : > > line 5: unnamed input active-low consumer=interrupt > line 84: unnamed input active-low > > > So definitely progressing. > > >>> > >>> Checking /sys/kernel/irq/7 > >>> > >>> kernel alone : > >>> actions: pinctrl_amd > >>> chip_name: IR-IO-APIC > >>> hwirq: 7 > >>> name: fasteoi > >>> per_cpu_count: 0,0,0,0,0,20,0,0,0,0,0,0,0,0,0,0 > >>> type: level > >>> wakeup: enabled > >>> > >>> xen skipping IRQ7 double init : > >>> > >>> actions: pinctrl_amd > >>> chip_name: xen-pirq > >>> hwirq: > >>> name: ioapic-level > >>> per_cpu_count: 0,0,0,0,0,0,1,0,0,0,0,0,0,0,0,0 > >>> type: edge > >>> wakeup: disabled > >>> > >>> So the skip of IRQ7 in pci_xen_initial_domain() sets the correct > handler > >>> (IIUC xen uses the ioapic-level and handles the eoi separately), but > not > >>> the correct type (still edge). > >>> I guess this may explains the results above. > >>> > >>> > >> Mario (in CC) patched the pinctrl_amd to flush pending interrupt > before > >> starting the driver for the GPIO. > >> > >> This helped in the sense of there's no more pending interrupt on IRQ7 > >> (whatever the handler is, level or edge) but then the touchpad is not > >> detected by i2c-hid. > >> > >> Is there any work in progress related to the incorrect IRQ > configuration ? > > > > I'm not aware of any. As per my recollection it's still not entirely > > clear where in the kernel things go astray. And to be honest I don't > > feel comfortable trying to half-blindly address this, e.g. by trying > > to circumvent / defer the early setting up of the low 16 IRQs. > > > > Jan > > Shot in the dark - but could this be a problem where PCAT_COMPAT from > the MADT is being ignored causing PIC not to be setup properly in the > Xen case? > > See https://lore.kernel.org/all/875y2u5s8g.ffs@tglx/ for some context. > > At least we know that no MADT override is found by xen for INT7 as no INT_SRC_OVR message is printed. Do we expect one @Mario Limonciello ?
Re: [PATCH v3 15/34] xen/riscv: introduce atomic.h
On 22.12.2023 16:12, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/atomic.h
> @@ -0,0 +1,384 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Taken and modified from Linux.
> + *
> + * Copyright (C) 2007 Red Hat, Inc. All Rights Reserved.
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2017 SiFive
> + * Copyright (C) 2021 Vates SAS
> + */
> +
> +#ifndef _ASM_RISCV_ATOMIC_H
> +#define _ASM_RISCV_ATOMIC_H
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +void __bad_atomic_size(void);
> +
> +static always_inline void read_atomic_size(const volatile void *p,
> + void *res,
> + unsigned int size)
> +{
> +switch ( size )
> +{
> +case 1: *(uint8_t *)res = readb((const uint8_t *)p); break;
> +case 2: *(uint16_t *)res = readw((const uint16_t *)p); break;
> +case 4: *(uint32_t *)res = readl((const uint32_t *)p); break;
> +case 8: *(uint32_t *)res = readq((const uint64_t *)p); break;
Just like const, you should also avoid casting away volatile.
> +default: __bad_atomic_size(); break;
> +}
> +}
> +
> +#define read_atomic(p) ({ \
> +union { typeof(*p) val; char c[0]; } x_;\
> +read_atomic_size(p, x_.c, sizeof(*p)); \
> +x_.val; \
> +})
> +
> +
Nit: No double blank lines please.
> +#define write_atomic(p, x) ({ \
> +typeof(*p) x__ = (x); \
> +switch ( sizeof(*p) )
> \
> +{
> \
These lines look excessively long, possibly as a result of leaving hard tabs
in place.
Overall some of the style comments on the earlier patch seem to apply here
as well.
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/fence.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef _ASM_RISCV_FENCE_H
> +#define _ASM_RISCV_FENCE_H
> +
> +#ifdef CONFIG_SMP
> +#define RISCV_ACQUIRE_BARRIER"\tfence r , rw\n"
> +#define RISCV_RELEASE_BARRIER"\tfence rw, w\n"
> +#else
> +#define RISCV_ACQUIRE_BARRIER
> +#define RISCV_RELEASE_BARRIER
> +#endif
Do you really care about the !SMP case? On x86 at least we stopped special-
casing that configuration many years ago (the few cases where for typically
build reasons it matters, using CONFIG_NR_CPUS is sufficient). If you care
about it, there needs to be somewhere you actually #define CONFIG_SMP.
Jan
Re: [PATCH v3 13/34] xen/riscv: introduce cmpxchg.h
On 22.12.2023 16:12, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/cmpxchg.h
> @@ -0,0 +1,496 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright (C) 2014 Regents of the University of California */
> +
> +#ifndef _ASM_RISCV_CMPXCHG_H
> +#define _ASM_RISCV_CMPXCHG_H
> +
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#define __xchg_relaxed(ptr, new, size) \
> +({ \
> +__typeof__(ptr) ptr__ = (ptr); \
> +__typeof__(new) new__ = (new); \
> +__typeof__(*(ptr)) ret__; \
I expect the types of new and *ptr want to actually match. Which
you then want to enforce, so that issues at use sites would either
be reported by the compiler, or be permitted by a type conversion
of new.
> +switch (size) \
> + { \
Nit: Hard tab left here. (Also again you want to either stick to
Linux style or fully switch to Xen style.)
> +case 4: \
> +asm volatile( \
> +"amoswap.w %0, %2, %1\n" \
I don't think a leading tab (or leading whitespace in general) is
needed in single-line-output asm()s. The trailing \n also isn't
needed if I'm not mistaken.
> +: "=r" (ret__), "+A" (*ptr__) \
> +: "r" (new__) \
> +: "memory" ); \
> +break; \
> +case 8: \
> +asm volatile( \
> +"amoswap.d %0, %2, %1\n" \
> +: "=r" (ret__), "+A" (*ptr__) \
> +: "r" (new__) \
> +: "memory" ); \
> +break; \
> +default: \
> +ASSERT_UNREACHABLE(); \
If at all possible this wants to trigger a build failure, not a runtime
one.
> +} \
> +ret__; \
> +})
> +
> +#define xchg_relaxed(ptr, x) \
> +({ \
> +__typeof__(*(ptr)) x_ = (x); \
> +(__typeof__(*(ptr))) __xchg_relaxed((ptr), x_, sizeof(*(ptr))); \
Nit: Stray blank after cast. For readability I'd also suggest to
drop parentheses in cases like the first argument passed to
__xchg_relaxed() here.
> +})
For both: What does "relaxed" describe? I'm asking because it's not
really clear whether the memory clobbers are actually needed.
> +#define __xchg_acquire(ptr, new, size) \
> +({ \
> +__typeof__(ptr) ptr__ = (ptr); \
> +__typeof__(new) new__ = (new); \
> +__typeof__(*(ptr)) ret__; \
> +switch (size) \
> + { \
> +case 4: \
> +asm volatile( \
> +"amoswap.w %0, %2, %1\n" \
> +RISCV_ACQUIRE_BARRIER \
> +: "=r" (ret__), "+A" (*ptr__) \
> +: "r" (new__) \
> +: "memory" ); \
> +break; \
> +case 8: \
> +asm volatile( \
> +"amoswap.d %0, %2, %1\n" \
> +RISCV_ACQUIRE_BARRIER \
> +: "=r" (ret__), "+A" (*ptr__) \
> +: "r" (new__) \
> +: "memory" ); \
> +break; \
> +default: \
> +ASSERT_UNREACHABLE(); \
> +} \
> +ret__; \
> +})
If I'm not mistaken this differs from __xchg_relaxed() only in the use
of RISCV_ACQUIRE_BARRIER, and ...
> +#define xchg_acquire(ptr, x) \
> +({ \
> +__typeof__(*(ptr)) x_ = (x); \
> +(__typeof__(*(ptr))) __xchg_acquire((ptr), x_, sizeof(*(ptr))); \
> +})
> +
> +#define __xchg_release(ptr, new, size) \
> +({ \
> +__typeof__(ptr) ptr__ = (ptr); \
> +__typeof__(new) new__ = (new); \
> +__typeof__(*(ptr)) ret__; \
> +switch (size) \
> + { \
> +case 4: \
> +asm volatile ( \
> +RISCV_RELEASE_BARRIER \
> +"amoswap.w %0, %2, %1\n" \
> +: "=r" (ret__), "+A" (*ptr__) \
> +: "r" (new__) \
> +: "memory"); \
> +break; \
> +case 8: \
> +asm volatile ( \
> +RISCV_RELEASE_BARRIER \
> +"amoswap.d %0, %2, %1\n" \
> +: "=r" (ret__), "+A" (*ptr__) \
> +: "r" (new__) \
> +: "memory"); \
> +break; \
> +default: \
> +ASSERT_UNREACHABLE(); \
> +} \
> +ret__; \
> +})
this only in the use of RISCV_RELEASE_BARRIER. If so they likely want
folding, to limit redundancy and make eventual updating easier. (Same
for the cmpxchg helper further down, as it seems.)
> +#define xchg_release(ptr, x) \
> +({ \
> +__typeof__(*(ptr)) x_ = (x); \
> +(__typeof__(*(ptr))) __xchg_release((ptr), x_, sizeof(*(ptr))); \
> +})
> +
> +static always_inline uint32_t __xchg_case_4(volatile uint32_t *ptr,
> +uint32_t new)
> +{
> +__typeof__(*(ptr)) ret;
> +
> +asm volatile (
> +" amoswap.w.aqrl %0, %2, %1\n"
> +: "=r" (ret), "+A" (*ptr)
> +: "r" (new)
> +: "memory" );
> +
> +return ret;
> +}
> +
> +static always_inline uint64_t __xchg_case_8(volatile uint64_t *ptr,
> +uint64_t new)
> +{
> +__typeof__(*(ptr)) ret;
> +
> +asm volatile( \
> +" amoswap.d.aqrl %0, %2, %1\n" \
> +: "=r" (ret), "+A"
[xen-unstable-smoke test] 184424: tolerable all pass - PUSHED
flight 184424 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/184424/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen 4900c939cb9b876c51cfc7a4c854f54c722a30b5 baseline version: xen b25607e528f6ce7851e907ed59ad5ff583aa1840 Last test of basis 184399 2024-01-19 02:00:25 Z3 days Testing same since 184424 2024-01-22 13:00:26 Z0 days1 attempts People who touched revisions under test: Jan Beulich jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git b25607e528..4900c939cb 4900c939cb9b876c51cfc7a4c854f54c722a30b5 -> smoke
Re: [PATCH v2] lib{fdt,elf}: move lib{fdt,elf}-temp.o and their deps to $(targets)
On 22.01.2024 15:46, Anthony PERARD wrote:
> On Mon, Jan 22, 2024 at 12:39:55PM +0100, Michal Orzel wrote:
>> At the moment, trying to run xencov read/reset (calling SYSCTL_coverage_op
>> under the hood) results in a crash. This is due to a profiler trying to
>> access data in the .init.* sections (libfdt for Arm and libelf for x86)
>> that are stripped after boot. Normally, the build system compiles any
>> *.init.o file without COV_FLAGS. However, these two libraries are
>> handled differently as sections will be renamed to init after linking.
>>
>> To override COV_FLAGS to empty for these libraries, lib{fdt,elf}.o were
>> added to nocov-y. This worked until e321576f4047 ("xen/build: start using
>> if_changed") that added lib{fdt,elf}-temp.o and their deps to extra-y.
>> This way, even though these objects appear as prerequisites of
>> lib{fdt,elf}.o and the settings should propagate to them, make can also
>> build them as a prerequisite of __build, in which case COV_FLAGS would
>> still have the unwanted flags. Fix it by switching to $(targets) instead.
>>
>> Also, for libfdt, append libfdt.o to nocov-y only if CONFIG_OVERLAY_DTB
>> is not set. Otherwise, there is no section renaming and we should be able
>> to run the coverage.
>>
>> Fixes: e321576f4047 ("xen/build: start using if_changed")
>> Signed-off-by: Michal Orzel
>
> Reviewed-by: Anthony PERARD
Acked-by: Jan Beulich
Re: [PATCH v2] lib{fdt,elf}: move lib{fdt,elf}-temp.o and their deps to $(targets)
On Mon, Jan 22, 2024 at 12:39:55PM +0100, Michal Orzel wrote:
> At the moment, trying to run xencov read/reset (calling SYSCTL_coverage_op
> under the hood) results in a crash. This is due to a profiler trying to
> access data in the .init.* sections (libfdt for Arm and libelf for x86)
> that are stripped after boot. Normally, the build system compiles any
> *.init.o file without COV_FLAGS. However, these two libraries are
> handled differently as sections will be renamed to init after linking.
>
> To override COV_FLAGS to empty for these libraries, lib{fdt,elf}.o were
> added to nocov-y. This worked until e321576f4047 ("xen/build: start using
> if_changed") that added lib{fdt,elf}-temp.o and their deps to extra-y.
> This way, even though these objects appear as prerequisites of
> lib{fdt,elf}.o and the settings should propagate to them, make can also
> build them as a prerequisite of __build, in which case COV_FLAGS would
> still have the unwanted flags. Fix it by switching to $(targets) instead.
>
> Also, for libfdt, append libfdt.o to nocov-y only if CONFIG_OVERLAY_DTB
> is not set. Otherwise, there is no section renaming and we should be able
> to run the coverage.
>
> Fixes: e321576f4047 ("xen/build: start using if_changed")
> Signed-off-by: Michal Orzel
Reviewed-by: Anthony PERARD
Thanks,
--
Anthony PERARD
[PATCH v2 5/8] PV-shim: drop pv_console_rx()'s regs parameter
It's not needed anymore. This is in preparation of dropping the register
parameters from IRQ handler functions.
Signed-off-by: Jan Beulich
Reviewed-by: Andrew Cooper
--- a/xen/arch/x86/guest/xen/xen.c
+++ b/xen/arch/x86/guest/xen/xen.c
@@ -181,7 +181,7 @@ static void cf_check xen_evtchn_upcall(s
port += l1 * BITS_PER_LONG;
if ( pv_console && port == pv_console_evtchn() )
-pv_console_rx(regs);
+pv_console_rx();
else if ( pv_shim )
pv_shim_inject_evtchn(port);
}
--- a/xen/drivers/char/xen_pv_console.c
+++ b/xen/drivers/char/xen_pv_console.c
@@ -94,7 +94,7 @@ evtchn_port_t pv_console_evtchn(void)
return cons_evtchn;
}
-size_t pv_console_rx(struct cpu_user_regs *regs)
+size_t pv_console_rx(void)
{
char c;
XENCONS_RING_IDX cons, prod;
--- a/xen/include/xen/pv_console.h
+++ b/xen/include/xen/pv_console.h
@@ -9,7 +9,7 @@ void pv_console_init(void);
void pv_console_set_rx_handler(serial_rx_fn fn);
void pv_console_init_postirq(void);
void pv_console_puts(const char *buf, size_t nr);
-size_t pv_console_rx(struct cpu_user_regs *regs);
+size_t pv_console_rx(void);
evtchn_port_t pv_console_evtchn(void);
#else
@@ -18,7 +18,7 @@ static inline void pv_console_init(void)
static inline void pv_console_set_rx_handler(serial_rx_fn fn) { }
static inline void pv_console_init_postirq(void) { }
static inline void pv_console_puts(const char *buf, size_t nr) { }
-static inline size_t pv_console_rx(struct cpu_user_regs *regs) { return 0; }
+static inline size_t pv_console_rx(void) { return 0; }
#endif /* !CONFIG_XEN_GUEST */
#endif /* __XEN_PV_CONSOLE_H__ */
Re: [XEN PATCH 1/3] xen: introduce static_assert_unreachable()
On 22.01.2024 14:48, Federico Serafini wrote:
> Introduce macro static_asser_unreachable() to check that a program
> point is considered unreachable by the static analysis performed by the
> compiler, even at optimization level -O0.
Is it really intended to limit use of this macro to cases where even
at -O0 the compiler would eliminate respective code? Note that right
now even debug builds are done with some optimization, and some of
the DCE we're relying depends on that (iirc).
> --- a/xen/include/xen/compiler.h
> +++ b/xen/include/xen/compiler.h
> @@ -64,6 +64,14 @@
> # define fallthroughdo {} while (0) /* fallthrough */
> #endif
>
> +/*
> + * Add the following macro to check that a program point is considered
> + * unreachable by the static analysis performed by the compiler,
> + * even at optimization level -O0.
> + */
> +#define static_assert_unreachable() \
> +asm(".error \"unreachable program point reached\"");
Did you check the diagnostic that results when this check actually
triggers? I expect it will be not really obvious from the message
you introduce where the issue actually is. I expect we will want
to use some of __FILE__ / __LINE__ / __FUNCTION__ to actually
supply such context.
Also: Stray semicolon and (nit) missing blanks.
Finally I wonder about case: We have ASSERT_UNREACHABLE() and it
may be indicated to use all uppercase her as well.
Jan
[PATCH v2 8/8] x86/APIC: drop regs parameter from direct vector handler functions
The only place it was needed is in the spurious handler, and there we
can use get_irq_regs() instead.
Signed-off-by: Jan Beulich
Reviewed-by: Andrew Cooper
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1324,7 +1324,7 @@ int reprogram_timer(s_time_t timeout)
return apic_tmict || !timeout;
}
-static void cf_check apic_timer_interrupt(struct cpu_user_regs *regs)
+static void cf_check apic_timer_interrupt(void)
{
ack_APIC_irq();
perfc_incr(apic_timer);
@@ -1343,7 +1343,7 @@ void smp_send_state_dump(unsigned int cp
/*
* Spurious interrupts should _never_ happen with our APIC/SMP architecture.
*/
-static void cf_check spurious_interrupt(struct cpu_user_regs *regs)
+static void cf_check spurious_interrupt(void)
{
/*
* Check if this is a vectored interrupt (most likely, as this is probably
@@ -1357,7 +1357,7 @@ static void cf_check spurious_interrupt(
is_spurious = !nmi_check_continuation();
if (this_cpu(state_dump_pending)) {
this_cpu(state_dump_pending) = false;
-dump_execstate(regs);
+dump_execstate(get_irq_regs());
is_spurious = false;
}
@@ -1374,7 +1374,7 @@ static void cf_check spurious_interrupt(
* This interrupt should never happen with our APIC/SMP architecture
*/
-static void cf_check error_interrupt(struct cpu_user_regs *regs)
+static void cf_check error_interrupt(void)
{
static const char *const esr_fields[] = {
", Send CS error",
@@ -1409,7 +1409,7 @@ static void cf_check error_interrupt(str
* This interrupt handles performance counters interrupt
*/
-static void cf_check pmu_interrupt(struct cpu_user_regs *regs)
+static void cf_check pmu_interrupt(void)
{
ack_APIC_irq();
vpmu_do_interrupt();
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -58,7 +58,7 @@ bool __read_mostly lmce_support;
#define MCE_RING0x1
static DEFINE_PER_CPU(int, last_state);
-static void cf_check intel_thermal_interrupt(struct cpu_user_regs *regs)
+static void cf_check intel_thermal_interrupt(void)
{
uint64_t msr_content;
unsigned int cpu = smp_processor_id();
@@ -642,7 +642,7 @@ static void cpu_mcheck_disable(void)
clear_cmci();
}
-static void cf_check cmci_interrupt(struct cpu_user_regs *regs)
+static void cf_check cmci_interrupt(void)
{
mctelem_cookie_t mctc;
struct mca_summary bs;
--- a/xen/arch/x86/guest/xen/xen.c
+++ b/xen/arch/x86/guest/xen/xen.c
@@ -158,7 +158,7 @@ static void __init init_memmap(void)
}
}
-static void cf_check xen_evtchn_upcall(struct cpu_user_regs *regs)
+static void cf_check xen_evtchn_upcall(void)
{
struct vcpu_info *vcpu_info = this_cpu(vcpu_info);
unsigned long pending;
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2776,7 +2776,7 @@ static struct hvm_function_table __initd
};
/* Handle VT-d posted-interrupt when VCPU is blocked. */
-static void cf_check pi_wakeup_interrupt(struct cpu_user_regs *regs)
+static void cf_check pi_wakeup_interrupt(void)
{
struct vmx_vcpu *vmx, *tmp;
spinlock_t *lock = _cpu(vmx_pi_blocking, smp_processor_id()).lock;
@@ -2808,7 +2808,7 @@ static void cf_check pi_wakeup_interrupt
}
/* Handle VT-d posted-interrupt when VCPU is running. */
-static void cf_check pi_notification_interrupt(struct cpu_user_regs *regs)
+static void cf_check pi_notification_interrupt(void)
{
ack_APIC_irq();
this_cpu(irq_count)++;
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -72,17 +72,15 @@ extern int opt_irq_vector_map;
#define platform_legacy_irq(irq) ((irq) < 16)
-void cf_check event_check_interrupt(struct cpu_user_regs *regs);
-void cf_check invalidate_interrupt(struct cpu_user_regs *regs);
-void cf_check call_function_interrupt(struct cpu_user_regs *regs);
-void cf_check irq_move_cleanup_interrupt(struct cpu_user_regs *regs);
+void cf_check event_check_interrupt(void);
+void cf_check invalidate_interrupt(void);
+void cf_check call_function_interrupt(void);
+void cf_check irq_move_cleanup_interrupt(void);
uint8_t alloc_hipriority_vector(void);
-void set_direct_apic_vector(
-uint8_t vector, void (*handler)(struct cpu_user_regs *regs));
-void alloc_direct_apic_vector(
-uint8_t *vector, void (*handler)(struct cpu_user_regs *regs));
+void set_direct_apic_vector(uint8_t vector, void (*handler)(void));
+void alloc_direct_apic_vector(uint8_t *vector, void (*handler)(void));
void do_IRQ(struct cpu_user_regs *regs);
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -743,7 +743,7 @@ void move_native_irq(struct irq_desc *de
desc->handler->enable(desc);
}
-void cf_check irq_move_cleanup_interrupt(struct cpu_user_regs *regs)
+void cf_check irq_move_cleanup_interrupt(void)
{
unsigned vector, me;
@@ -913,16 +913,14 @@ uint8_t alloc_hipriority_vector(void)
return next++;
}
-static void
[PATCH v2 7/8] IRQ: drop regs parameter from handler functions
It's simply not needed anymore. Note how Linux made this change many
years ago already, in 2.6.19 (late 2006, see [1]).
Signed-off-by: Jan Beulich
Reviewed-by: Andrew Cooper
Acked-by: Julien Grall
[1] https://git.kernel.org/torvalds/c/7d12e780e003f93433d49ce78cfedf4b4c52adc5
---
v2: Arm build fixes.
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -397,7 +397,7 @@ void gic_interrupt(struct cpu_user_regs
} while (1);
}
-static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs
*regs)
+static void maintenance_interrupt(int irq, void *dev_id)
{
/*
* This is a dummy interrupt handler.
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -182,8 +182,7 @@ void irq_set_affinity(struct irq_desc *d
}
int request_irq(unsigned int irq, unsigned int irqflags,
-void (*handler)(int irq, void *dev_id,
-struct cpu_user_regs *regs),
+void (*handler)(int irq, void *dev_id),
const char *devname, void *dev_id)
{
struct irqaction *action;
@@ -276,7 +275,7 @@ void do_IRQ(struct cpu_user_regs *regs,
do
{
-action->handler(irq, action->dev_id, regs);
+action->handler(irq, action->dev_id);
action = action->next;
} while ( action );
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -241,7 +241,7 @@ int reprogram_timer(s_time_t timeout)
}
/* Handle the firing timer */
-static void htimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
+static void htimer_interrupt(int irq, void *dev_id)
{
if ( unlikely(!(READ_SYSREG(CNTHP_CTL_EL2) & CNTx_CTL_PENDING)) )
return;
@@ -255,7 +255,7 @@ static void htimer_interrupt(int irq, vo
WRITE_SYSREG(0, CNTHP_CTL_EL2);
}
-static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
+static void vtimer_interrupt(int irq, void *dev_id)
{
/*
* Edge-triggered interrupts can be used for the virtual timer. Even
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -962,7 +962,7 @@ static int __init cf_check irq_ratelimit
__initcall(irq_ratelimit_init);
int __init request_irq(unsigned int irq, unsigned int irqflags,
-void (*handler)(int irq, void *dev_id, struct cpu_user_regs *regs),
+void (*handler)(int irq, void *dev_id),
const char * devname, void *dev_id)
{
struct irqaction * action;
@@ -2009,7 +2009,7 @@ void do_IRQ(struct cpu_user_regs *regs)
spin_unlock_irq(>lock);
tsc_in = tb_init_done ? get_cycles() : 0;
-action->handler(irq, action->dev_id, regs);
+action->handler(irq, action->dev_id);
TRACE_3D(TRC_HW_IRQ_HANDLED, irq, tsc_in, get_cycles());
spin_lock_irq(>lock);
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -237,8 +237,7 @@ again:
}
}
-static void cf_check hpet_interrupt_handler(
-int irq, void *data, struct cpu_user_regs *regs)
+static void cf_check hpet_interrupt_handler(int irq, void *data)
{
struct hpet_event_channel *ch = data;
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -198,8 +198,7 @@ static void smp_send_timer_broadcast_ipi
}
}
-static void cf_check timer_interrupt(
-int irq, void *dev_id, struct cpu_user_regs *regs)
+static void cf_check timer_interrupt(int irq, void *dev_id)
{
ASSERT(local_irq_is_enabled());
--- a/xen/common/irq.c
+++ b/xen/common/irq.c
@@ -29,7 +29,7 @@ int init_one_irq_desc(struct irq_desc *d
return err;
}
-void cf_check no_action(int cpl, void *dev_id, struct cpu_user_regs *regs)
+void cf_check no_action(int cpl, void *dev_id)
{
}
--- a/xen/drivers/char/cadence-uart.c
+++ b/xen/drivers/char/cadence-uart.c
@@ -40,7 +40,7 @@ static struct cuart {
#define cuart_read(uart, off) readl((uart)->regs + (off))
#define cuart_write(uart, off,val) writel((val), (uart)->regs + (off))
-static void cuart_interrupt(int irq, void *data, struct cpu_user_regs *regs)
+static void cuart_interrupt(int irq, void *data)
{
struct serial_port *port = data;
struct cuart *uart = port->uart;
--- a/xen/drivers/char/exynos4210-uart.c
+++ b/xen/drivers/char/exynos4210-uart.c
@@ -45,7 +45,7 @@ static struct exynos4210_uart {
#define exynos4210_read(uart, off) readl((uart)->regs + off)
#define exynos4210_write(uart, off, val)writel(val, (uart->regs) + off)
-static void exynos4210_uart_interrupt(int irq, void *data, struct
cpu_user_regs *regs)
+static void exynos4210_uart_interrupt(int irq, void *data)
{
struct serial_port *port = data;
struct exynos4210_uart *uart = port->uart;
--- a/xen/drivers/char/imx-lpuart.c
+++ b/xen/drivers/char/imx-lpuart.c
@@ -37,8 +37,7 @@ static struct imx_lpuart {
struct vuart_info vuart;
} imx8_com;
-static void imx_lpuart_interrupt(int irq, void *data,
- struct cpu_user_regs *regs)
+static void imx_lpuart_interrupt(int irq, void *data)
{
struct
[PATCH v2 6/8] serial: drop serial_[rt]x_interrupt()'s regs parameter
They're simply not needed anymore.
Signed-off-by: Jan Beulich
---
v2: Setting of IRQ regs split off to an earlier patch.
--- a/xen/drivers/char/cadence-uart.c
+++ b/xen/drivers/char/cadence-uart.c
@@ -51,7 +51,7 @@ static void cuart_interrupt(int irq, voi
/* ACK. */
if ( status & UART_SR_INTR_RTRIG )
{
-serial_rx_interrupt(port, regs);
+serial_rx_interrupt(port);
cuart_write(uart, R_UART_CISR, UART_SR_INTR_RTRIG);
}
} while ( status & UART_SR_INTR_RTRIG );
--- a/xen/drivers/char/ehci-dbgp.c
+++ b/xen/drivers/char/ehci-dbgp.c
@@ -1273,10 +1273,10 @@ static void cf_check _ehci_dbgp_poll(str
old_regs = set_irq_regs(regs);
if ( dbgp->in.chunk )
-serial_rx_interrupt(port, regs);
+serial_rx_interrupt(port);
if ( empty )
-serial_tx_interrupt(port, regs);
+serial_tx_interrupt(port);
set_irq_regs(old_regs);
--- a/xen/drivers/char/exynos4210-uart.c
+++ b/xen/drivers/char/exynos4210-uart.c
@@ -81,7 +81,7 @@ static void exynos4210_uart_interrupt(in
if ( status & (UINTM_RXD | UINTM_ERROR) )
{
/* uart->regs[UINTM] |= RXD|ERROR; */
-serial_rx_interrupt(port, regs);
+serial_rx_interrupt(port);
/* uart->regs[UINTM] &= ~(RXD|ERROR); */
exynos4210_write(uart, UINTP, UINTM_RXD | UINTM_ERROR);
}
@@ -89,7 +89,7 @@ static void exynos4210_uart_interrupt(in
if ( status & (UINTM_TXD | UINTM_MODEM) )
{
/* uart->regs[UINTM] |= TXD|MODEM; */
-serial_tx_interrupt(port, regs);
+serial_tx_interrupt(port);
/* uart->regs[UINTM] &= ~(TXD|MODEM); */
exynos4210_write(uart, UINTP, UINTM_TXD | UINTM_MODEM);
}
--- a/xen/drivers/char/imx-lpuart.c
+++ b/xen/drivers/char/imx-lpuart.c
@@ -48,10 +48,10 @@ static void imx_lpuart_interrupt(int irq
rxcnt = imx_lpuart_read(uart, UARTWATER) >> UARTWATER_RXCNT_OFF;
if ( (sts & UARTSTAT_RDRF) || (rxcnt > 0) )
- serial_rx_interrupt(port, regs);
+ serial_rx_interrupt(port);
if ( sts & UARTSTAT_TDRE )
- serial_tx_interrupt(port, regs);
+ serial_tx_interrupt(port);
imx_lpuart_write(uart, UARTSTAT, sts);
}
--- a/xen/drivers/char/meson-uart.c
+++ b/xen/drivers/char/meson-uart.c
@@ -69,10 +69,10 @@ static void meson_uart_interrupt(int irq
uint32_t st = readl(uart->regs + AML_UART_STATUS_REG);
if ( !(st & AML_UART_RX_FIFO_EMPTY) )
-serial_rx_interrupt(port, regs);
+serial_rx_interrupt(port);
if ( !(st & AML_UART_TX_FIFO_FULL) )
-serial_tx_interrupt(port, regs);
+serial_tx_interrupt(port);
}
static void __init meson_uart_init_preirq(struct serial_port *port)
--- a/xen/drivers/char/mvebu-uart.c
+++ b/xen/drivers/char/mvebu-uart.c
@@ -76,10 +76,10 @@ static void mvebu3700_uart_interrupt(int
if ( st & (STATUS_RX_RDY | STATUS_OVR_ERR | STATUS_FRM_ERR |
STATUS_BRK_DET) )
-serial_rx_interrupt(port, regs);
+serial_rx_interrupt(port);
if ( st & STATUS_TX_RDY )
-serial_tx_interrupt(port, regs);
+serial_tx_interrupt(port);
}
static void __init mvebu3700_uart_init_preirq(struct serial_port *port)
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -188,9 +188,9 @@ static void cf_check ns16550_interrupt(
u8 lsr = ns_read_reg(uart, UART_LSR);
if ( (lsr & uart->lsr_mask) == uart->lsr_mask )
-serial_tx_interrupt(port, regs);
+serial_tx_interrupt(port);
if ( lsr & UART_LSR_DR )
-serial_rx_interrupt(port, regs);
+serial_rx_interrupt(port);
/* A "busy-detect" condition is observed on Allwinner/sunxi UART
* after LCR is written during setup. It needs to be cleared at
@@ -224,11 +224,11 @@ static void cf_check __ns16550_poll(stru
if ( ns16550_ioport_invalid(uart) )
goto out;
-serial_rx_interrupt(port, regs);
+serial_rx_interrupt(port);
}
if ( ( ns_read_reg(uart, UART_LSR) & uart->lsr_mask ) == uart->lsr_mask )
-serial_tx_interrupt(port, regs);
+serial_tx_interrupt(port);
out:
set_irq_regs(old_regs);
--- a/xen/drivers/char/omap-uart.c
+++ b/xen/drivers/char/omap-uart.c
@@ -70,9 +70,9 @@ static void omap_uart_interrupt(int irq,
{
lsr = omap_read(uart, UART_LSR) & 0xff;
if ( lsr & UART_LSR_THRE )
-serial_tx_interrupt(port, regs);
+serial_tx_interrupt(port);
if ( lsr & UART_LSR_DR )
-serial_rx_interrupt(port, regs);
+serial_rx_interrupt(port);
if ( port->txbufc == port->txbufp ) {
reg = omap_read(uart, UART_IER);
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -95,7 +95,7 @@ static void pl011_interrupt(int irq, voi
[PATCH v2 4/8] serial: drop serial_rx_fn's regs parameter
It's simply not needed anymore.
Signed-off-by: Jan Beulich
---
v2: Re-base over earlier (new/split) patches.
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -527,7 +527,7 @@ static void switch_serial_input(void)
printk("\n");
}
-static void __serial_rx(char c, struct cpu_user_regs *regs)
+static void __serial_rx(char c)
{
switch ( console_rx )
{
@@ -579,7 +579,7 @@ static void __serial_rx(char c, struct c
#endif
}
-static void cf_check serial_rx(char c, struct cpu_user_regs *regs)
+static void cf_check serial_rx(char c)
{
static int switch_code_count = 0;
@@ -595,10 +595,10 @@ static void cf_check serial_rx(char c, s
}
for ( ; switch_code_count != 0; switch_code_count-- )
-__serial_rx(switch_code, regs);
+__serial_rx(switch_code);
/* Finally process the just-received character. */
-__serial_rx(c, regs);
+__serial_rx(c);
}
static void cf_check notify_dom0_con_ring(void *unused)
--- a/xen/drivers/char/serial.c
+++ b/xen/drivers/char/serial.c
@@ -68,7 +68,7 @@ void serial_rx_interrupt(struct serial_p
spin_unlock_irqrestore(>rx_lock, flags);
if ( fn != NULL )
-(*fn)(c & 0x7f, regs);
+fn(c & 0x7f);
}
void serial_tx_interrupt(struct serial_port *port, struct cpu_user_regs *regs)
--- a/xen/drivers/char/xen_pv_console.c
+++ b/xen/drivers/char/xen_pv_console.c
@@ -118,7 +118,7 @@ size_t pv_console_rx(struct cpu_user_reg
{
c = cons_ring->in[MASK_XENCONS_IDX(cons++, cons_ring->in)];
if ( cons_rx_handler )
-cons_rx_handler(c, regs);
+cons_rx_handler(c);
recv++;
}
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -15,7 +15,7 @@
struct cpu_user_regs;
/* Register a character-receive hook on the specified COM port. */
-typedef void (*serial_rx_fn)(char c, struct cpu_user_regs *regs);
+typedef void (*serial_rx_fn)(char c);
void serial_set_rx_handler(int handle, serial_rx_fn fn);
/* Number of characters we buffer for a polling receiver. */
[PATCH v2 3/8] keyhandler: drop regs parameter from handle_keyregs()
In preparation for further removal of regs parameters, drop it here. In
the two places where it's actually needed, retrieve IRQ context if
available, or else guest context.
Suggested-by: Andrew Cooper
Signed-off-by: Jan Beulich
---
As an alternative to the new boolean parameter, I wonder if we couldn't
special-case the idle vCPU case: It's only there where we would not have
proper context retrievable via guest_cpu_user_regs().
---
v2: New.
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -76,12 +76,12 @@ static struct keyhandler {
static void cf_check keypress_action(void *unused)
{
-handle_keypress(keypress_key, NULL);
+handle_keypress(keypress_key, true);
}
static DECLARE_TASKLET(keypress_tasklet, keypress_action, NULL);
-void handle_keypress(unsigned char key, struct cpu_user_regs *regs)
+void handle_keypress(unsigned char key, bool need_context)
{
struct keyhandler *h;
@@ -91,7 +91,7 @@ void handle_keypress(unsigned char key,
if ( !in_irq() || h->irq_callback )
{
console_start_log_everything();
-h->irq_callback ? h->irq_fn(key, regs) : h->fn(key);
+h->irq_callback ? h->irq_fn(key, need_context) : h->fn(key);
console_end_log_everything();
}
else
@@ -172,7 +172,7 @@ void cf_check dump_execstate(struct cpu_
}
static void cf_check dump_registers(
-unsigned char key, struct cpu_user_regs *regs)
+unsigned char key, bool need_context)
{
unsigned int cpu;
@@ -185,8 +185,8 @@ static void cf_check dump_registers(
cpumask_copy(_execstate_mask, _online_map);
/* Get local execution state out immediately, in case we get stuck. */
-if ( regs )
-dump_execstate(regs);
+if ( !need_context )
+dump_execstate(get_irq_regs() ?: guest_cpu_user_regs());
else
run_in_exception_handler(dump_execstate);
@@ -248,8 +248,7 @@ static void cf_check dump_hwdom_register
}
}
-static void cf_check reboot_machine(
-unsigned char key, struct cpu_user_regs *regs)
+static void cf_check reboot_machine(unsigned char key, bool unused)
{
printk("'%c' pressed -> rebooting machine\n", key);
machine_restart(0);
@@ -477,8 +476,7 @@ static void cf_check run_all_nonirq_keyh
static DECLARE_TASKLET(run_all_keyhandlers_tasklet,
run_all_nonirq_keyhandlers, NULL);
-static void cf_check run_all_keyhandlers(
-unsigned char key, struct cpu_user_regs *regs)
+static void cf_check run_all_keyhandlers(unsigned char key, bool need_context)
{
struct keyhandler *h;
unsigned int k;
@@ -494,7 +492,7 @@ static void cf_check run_all_keyhandlers
if ( !h->irq_fn || !h->diagnostic || !h->irq_callback )
continue;
printk("[%c: %s]\n", k, h->desc);
-h->irq_fn(k, regs);
+h->irq_fn(k, need_context);
}
watchdog_enable();
@@ -511,17 +509,16 @@ static void cf_check do_debugger_trap_fa
barrier();
}
-static void cf_check do_debug_key(unsigned char key, struct cpu_user_regs
*regs)
+static void cf_check do_debug_key(unsigned char key, bool need_context)
{
printk("'%c' pressed -> trapping into debugger\n", key);
-if ( regs )
-do_debugger_trap_fatal(regs);
+if ( !need_context )
+do_debugger_trap_fatal(get_irq_regs() ?: guest_cpu_user_regs());
else
run_in_exception_handler(do_debugger_trap_fatal);
}
-static void cf_check do_toggle_alt_key(
-unsigned char key, struct cpu_user_regs *regs)
+static void cf_check do_toggle_alt_key(unsigned char key, bool unused)
{
alt_key_handling = !alt_key_handling;
printk("'%c' pressed -> using %s key handling\n", key,
@@ -586,7 +583,7 @@ void keyhandler_crash_action(enum crash_
if ( *action == '+' )
mdelay(10);
else
-handle_keypress(*action, NULL);
+handle_keypress(*action, true);
action++;
}
}
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -134,7 +134,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe
{
if ( copy_from_guest_offset(, op->u.debug_keys.keys, i, 1) )
goto out;
-handle_keypress(c, guest_cpu_user_regs());
+handle_keypress(c, false);
}
ret = 0;
copyback = 0;
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -280,7 +280,7 @@ static int *__read_mostly upper_thresh_a
static int *__read_mostly lower_thresh_adj = _lower_thresh;
static const char *__read_mostly thresh_adj = "standard";
-static void cf_check do_toggle_guest(unsigned char key, struct cpu_user_regs
*regs)
+static void cf_check do_toggle_guest(unsigned char key, bool unused)
{
if ( upper_thresh_adj == _upper_thresh )
{
@@ -307,13 +307,13 @@ static void do_adj_thresh(unsigned char
loglvl_str(*upper_thresh_adj));
}
-static void cf_check do_inc_thresh(unsigned char key, struct cpu_user_regs
*regs)
+static void cf_check
[PATCH v2 2/8] serial: fake IRQ-regs context in poll handlers
In preparation of dropping the register parameters from
serial_[rt]x_interrupt() and in turn from IRQ handler functions,
register state needs making available another way for the few key
handlers which need it. Fake IRQ-like state.
Signed-off-by: Jan Beulich
---
The use of guest_cpu_user_regs() in dbc_uart_poll() is inconsistent with
other console poll functions we have, and it's unclear whether that's
actually generally correct.
Andrew suggested to move set_irq_regs() to BUGFRAME_run_fn handling;
it's not clear to me whether that would be (a) correct from an abstract
pov (that's exception, not interrupt context after all) and (b) really
beneficial.
---
v2: New.
--- a/xen/drivers/char/ehci-dbgp.c
+++ b/xen/drivers/char/ehci-dbgp.c
@@ -1253,6 +1253,7 @@ static void cf_check _ehci_dbgp_poll(str
unsigned long flags;
unsigned int timeout = MICROSECS(DBGP_CHECK_INTERVAL);
bool empty = false;
+struct cpu_user_regs *old_regs;
if ( !dbgp->ehci_debug )
return;
@@ -1268,12 +1269,17 @@ static void cf_check _ehci_dbgp_poll(str
spin_unlock_irqrestore(>tx_lock, flags);
}
+/* Mimic interrupt context. */
+old_regs = set_irq_regs(regs);
+
if ( dbgp->in.chunk )
serial_rx_interrupt(port, regs);
if ( empty )
serial_tx_interrupt(port, regs);
+set_irq_regs(old_regs);
+
if ( spin_trylock_irqsave(>tx_lock, flags) )
{
if ( dbgp->state == dbgp_idle && !dbgp->in.chunk &&
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -211,10 +211,14 @@ static void cf_check __ns16550_poll(stru
{
struct serial_port *port = this_cpu(poll_port);
struct ns16550 *uart = port->uart;
+struct cpu_user_regs *old_regs;
if ( uart->intr_works )
return; /* Interrupts work - no more polling */
+/* Mimic interrupt context. */
+old_regs = set_irq_regs(regs);
+
while ( ns_read_reg(uart, UART_LSR) & UART_LSR_DR )
{
if ( ns16550_ioport_invalid(uart) )
@@ -227,6 +231,7 @@ static void cf_check __ns16550_poll(stru
serial_tx_interrupt(port, regs);
out:
+set_irq_regs(old_regs);
set_timer(>timer, NOW() + MILLISECS(uart->timeout_ms));
}
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -1164,6 +1164,7 @@ static void cf_check dbc_uart_poll(void
struct dbc_uart *uart = port->uart;
struct dbc *dbc = >dbc;
unsigned long flags = 0;
+struct cpu_user_regs *old_regs;
if ( spin_trylock_irqsave(>tx_lock, flags) )
{
@@ -1175,10 +1176,15 @@ static void cf_check dbc_uart_poll(void
spin_unlock_irqrestore(>tx_lock, flags);
}
+/* Mimic interrupt context. */
+old_regs = set_irq_regs(guest_cpu_user_regs());
+
while ( dbc_work_ring_size(>dbc_iwork) )
serial_rx_interrupt(port, guest_cpu_user_regs());
serial_tx_interrupt(port, guest_cpu_user_regs());
+
+set_irq_regs(old_regs);
set_timer(>timer, NOW() + MICROSECS(DBC_POLL_INTERVAL));
}
[XEN PATCH 3/3] automation/eclair: add deviation for MISRA C:2012 Rule 16.3
Update ECLAIR configuration to consider safe switch clauses ending
with static_assert_unreachable().
Update docs/misra/deviations.rst accordingly.
Signed-off-by: Federico Serafini
---
automation/eclair_analysis/ECLAIR/deviations.ecl | 4
docs/misra/deviations.rst| 4
2 files changed, 8 insertions(+)
diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl
b/automation/eclair_analysis/ECLAIR/deviations.ecl
index fd32ff8a9c..b0cd904d2d 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -368,6 +368,10 @@ safe."
-config=MC3R1.R16.3,reports+={safe,
"any_area(end_loc(any_exp(text(/BUG\\(\\);/"}
-doc_end
+-doc_begin="Switch clauses ending with failure method
\"static_assert_unreachable()\" are safe."
+-config=MC3R1.R16.3,reports+={safe,
"any_area(end_loc(any_exp(text(/static_assert_unreachable\\(\\);/"}
+-doc_end
+
-doc_begin="Switch clauses not ending with the break statement are safe if an
explicit comment indicating the fallthrough intention is present."
-config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(^(?s).*/\\*
[fF]all ?through.? \\*/.*$,0..1"}
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 123c78e20a..875f0d9160 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -307,6 +307,10 @@ Deviations related to MISRA C:2012 Rules:
- Switch clauses ending with failure method \"BUG()\" are safe.
- Tagged as `safe` for ECLAIR.
+ * - R16.3
+ - Switch clauses ending with macro static_assert_unreachable() are safe.
+ - Tagged as `safe` for ECLAIR.
+
* - R16.3
- Existing switch clauses not ending with the break statement are safe if
an explicit comment indicating the fallthrough intention is present.
--
2.34.1
[PATCH v2 1/8] IRQ: generalize [gs]et_irq_regs()
Move functions (and their data) to common code, and invoke the functions
on Arm as well. This is in preparation of dropping the register
parameters from handler functions.
Signed-off-by: Jan Beulich
Reviewed-by: Andrew Cooper
Reviewed-by: Julien Grall
---
To limit visibility of the per-CPU data item, we may want to consider
making the functions out-of-line ones (in common/irq.c).
---
v2: Add a blank line.
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -221,6 +221,7 @@ void do_IRQ(struct cpu_user_regs *regs,
{
struct irq_desc *desc = irq_to_desc(irq);
struct irqaction *action;
+struct cpu_user_regs *old_regs = set_irq_regs(regs);
perfc_incr(irqs);
@@ -288,6 +289,7 @@ out:
out_no_end:
spin_unlock(>lock);
irq_exit();
+set_irq_regs(old_regs);
}
void release_irq(unsigned int irq, const void *dev_id)
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -70,27 +70,6 @@ extern bool opt_noirqbalance;
extern int opt_irq_vector_map;
-/*
- * Per-cpu current frame pointer - the location of the last exception frame on
- * the stack
- */
-DECLARE_PER_CPU(struct cpu_user_regs *, __irq_regs);
-
-static inline struct cpu_user_regs *get_irq_regs(void)
-{
- return this_cpu(__irq_regs);
-}
-
-static inline struct cpu_user_regs *set_irq_regs(struct cpu_user_regs
*new_regs)
-{
- struct cpu_user_regs *old_regs, **pp_regs = _cpu(__irq_regs);
-
- old_regs = *pp_regs;
- *pp_regs = new_regs;
- return old_regs;
-}
-
-
#define platform_legacy_irq(irq) ((irq) < 16)
void cf_check event_check_interrupt(struct cpu_user_regs *regs);
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -53,8 +53,6 @@ static DEFINE_SPINLOCK(vector_lock);
DEFINE_PER_CPU(vector_irq_t, vector_irq);
-DEFINE_PER_CPU(struct cpu_user_regs *, __irq_regs);
-
static LIST_HEAD(irq_ratelimit_list);
static DEFINE_SPINLOCK(irq_ratelimit_lock);
static struct timer irq_ratelimit_timer;
--- a/xen/common/irq.c
+++ b/xen/common/irq.c
@@ -1,6 +1,8 @@
#include
#include
+DEFINE_PER_CPU(struct cpu_user_regs *, irq_regs);
+
int init_one_irq_desc(struct irq_desc *desc)
{
int err;
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -131,6 +131,27 @@ void cf_check irq_actor_none(struct irq_
#define irq_disable_none irq_actor_none
#define irq_enable_none irq_actor_none
+/*
+ * Per-cpu interrupted context register state - the inner-most interrupt frame
+ * on the stack.
+ */
+DECLARE_PER_CPU(struct cpu_user_regs *, irq_regs);
+
+static inline struct cpu_user_regs *get_irq_regs(void)
+{
+ return this_cpu(irq_regs);
+}
+
+static inline struct cpu_user_regs *set_irq_regs(struct cpu_user_regs
*new_regs)
+{
+ struct cpu_user_regs *old_regs, **pp_regs = _cpu(irq_regs);
+
+ old_regs = *pp_regs;
+ *pp_regs = new_regs;
+
+ return old_regs;
+}
+
struct domain;
struct vcpu;
[XEN PATCH 1/3] xen: introduce static_assert_unreachable()
Introduce macro static_asser_unreachable() to check that a program
point is considered unreachable by the static analysis performed by the
compiler, even at optimization level -O0.
The use of such macro will lead to one of the following outcomes:
- the program point identified by the macro is considered unreachable,
then the compiler removes the macro;
- the program point identified by the macro is not considered
unreachable, then the compiler does not remove the macro, which will
lead to a failure in the build process caused by an assembler error.
Signed-off-by: Federico Serafini
---
xen/include/xen/compiler.h | 8
1 file changed, 8 insertions(+)
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index 16d554f2a5..ad0520f5d4 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -64,6 +64,14 @@
# define fallthroughdo {} while (0) /* fallthrough */
#endif
+/*
+ * Add the following macro to check that a program point is considered
+ * unreachable by the static analysis performed by the compiler,
+ * even at optimization level -O0.
+ */
+#define static_assert_unreachable() \
+asm(".error \"unreachable program point reached\"");
+
#ifdef __clang__
/* Clang can replace some vars with new automatic ones that go in .data;
* mark all explicit-segment vars 'used' to prevent that. */
--
2.34.1
[XEN PATCH 0/3] Introduce and use static_assert_unreachable()
Introduce macro static_assert_unreachable(), use it to replace
__{get,put}_user_bad() and update ECLAIR configuration to allow the use of
such macro at the end of switch-caluses.
Federico Serafini (3):
xen: introduce static_assert_unreachable()
x86/uaccess: replace __{get,put}_user_bad() with
static_assert_unreachable()
automation/eclair: add deviation for MISRA C:2012 Rule 16.3
automation/eclair_analysis/ECLAIR/deviations.ecl | 4
docs/misra/deviations.rst| 4
xen/arch/x86/include/asm/uaccess.h | 7 ++-
xen/include/xen/compiler.h | 8
4 files changed, 18 insertions(+), 5 deletions(-)
--
2.34.1
[XEN PATCH 2/3] x86/uaccess: replace __{get,put}_user_bad() with static_assert_unreachable()
Use static_assert_unreachable() to improve readability and anticipate
the build failure (from a linker error to an assembler error) in case
of wrong size.
Signed-off-by: Federico Serafini
---
xen/arch/x86/include/asm/uaccess.h | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/include/asm/uaccess.h
b/xen/arch/x86/include/asm/uaccess.h
index 7443519d5b..ce608fc2b5 100644
--- a/xen/arch/x86/include/asm/uaccess.h
+++ b/xen/arch/x86/include/asm/uaccess.h
@@ -21,9 +21,6 @@ unsigned int copy_from_guest_ll(void *to, const void __user
*from, unsigned int
unsigned int copy_to_unsafe_ll(void *to, const void *from, unsigned int n);
unsigned int copy_from_unsafe_ll(void *to, const void *from, unsigned int n);
-extern long __get_user_bad(void);
-extern void __put_user_bad(void);
-
#define UA_KEEP(args...) args
#define UA_DROP(args...)
@@ -208,7 +205,7 @@ do {
\
case 8:\
put_unsafe_asm(x, ptr, grd, retval, "q", "", "ir", errret); \
break; \
-default: __put_user_bad(); \
+default: static_assert_unreachable(); \
} \
clac();\
} while ( false )
@@ -227,7 +224,7 @@ do {
\
case 2: get_unsafe_asm(x, ptr, grd, retval, "w", "=r", errret); break; \
case 4: get_unsafe_asm(x, ptr, grd, retval, "k", "=r", errret); break; \
case 8: get_unsafe_asm(x, ptr, grd, retval, "", "=r", errret); break; \
-default: __get_user_bad(); \
+default: static_assert_unreachable(); \
} \
clac();\
} while ( false )
--
2.34.1
[PATCH v2 0/8] limit passing around of cpu_user_regs
Unlike (synchronous) exception handlers, interrupt handlers don't normally have a need to know the outer context's register state. Similarly, the vast majority of key handlers has no need for such. 1: IRQ: generalize [gs]et_irq_regs() 2: serial: fake IRQ-regs context in poll handlers 3: keyhandler: drop regs parameter from handle_keyregs() 4: serial: drop serial_rx_fn's regs parameter 5: PV-shim: drop pv_console_rx()'s regs parameter 6: serial: drop serial_[rt]x_interrupt()'s regs parameter 7: IRQ: drop regs parameter from handler functions 8: x86/APIC: drop regs parameter from direct vector handler functions Jan
[PATCH v2] x86: Dom0 "broken ELF" reporting adjustments
elf_load_binary() isn't the primary source of brokenness being
indicated. Therefore make the respective PVH log message there
conditional (much like PV has it), and add another instance when
elf_xen_parse() failed (again matching behavior in the PV case).
Make the PV side match the (new) use of %pd here.
Signed-off-by: Jan Beulich
---
v2: Use %pd and bring PV in line with that as well.
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -570,6 +570,8 @@ static int __init pvh_load_kernel(struct
if ( (rc = elf_xen_parse(, , true)) != 0 )
{
printk("Unable to parse kernel for ELFNOTES\n");
+if ( elf_check_broken() )
+printk("%pd kernel: broken ELF: %s\n", d, elf_check_broken());
return rc;
}
@@ -588,7 +590,8 @@ static int __init pvh_load_kernel(struct
if ( rc < 0 )
{
printk("Failed to load kernel: %d\n", rc);
-printk("Xen dom0 kernel broken ELF: %s\n", elf_check_broken());
+if ( elf_check_broken() )
+printk("%pd kernel: broken ELF: %s\n", d, elf_check_broken());
return rc;
}
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -1041,8 +1041,7 @@ int __init dom0_construct_pv(struct doma
out:
if ( elf_check_broken() )
-printk(XENLOG_WARNING "Dom0 kernel broken ELF: %s\n",
- elf_check_broken());
+printk("%pd kernel: broken ELF: %s\n", d, elf_check_broken());
return rc;
}
Re: [PATCH v5 4/8] Arm: annotate entry points with type and size
On 15.01.2024 15:36, Jan Beulich wrote: > Use the generic framework in xen/linkage.h. No change in generated code > except for the changed padding value (noticable when config.gz isn't a > multiple of 4 in size). Plus of course the converted symbols change to > be hidden ones. > > Note that ASM_INT() is switched to DATA(), not DATA_LOCAL(), as the only > use site wants the symbol global anyway. > > Signed-off-by: Jan Beulich > Reviewed-by: Julien Grall > --- > Only one each of the assembly files is being converted for now. More > could be done right here or as follow-on in separate patches. As this was meanwhile committed, I'd like to understand you preference for further conversion steps: I can certainly see to find time to make some actual progress here, but it might also be that you prefer to do so yourself. Please let me know. Jan
Ping: [PATCH v5 6/8] PPC: switch entry point annotations to common model
On 15.01.2024 15:38, Jan Beulich wrote: > Use the generic framework in xen/linkage.h. No change in generated code > except of course the converted symbols change to be hidden ones. > > Signed-off-by: Jan Beulich The other architectures have been at least partly switched; would be nice for PPC to follow suit. May I ask for an ack (or otherwise here), please? Thanks, Jan
Re: [PATCH v13 08/35] x86/fred: Disable FRED by default in its early stage
On Tue, Dec 05, 2023 at 02:49:57AM -0800, Xin Li wrote: > Warning: use of this parameter will taint the kernel > and may cause unknown problems. > > + fred[X86-64] > + Enable flexible return and event delivery Let's make it accept multiple options from the get-go: fred=on,disable-when,foo,bar,bla... in case we need to tweak its behavior. If it is only "fred" it will propagate this way downstream and it'll lead to confusion later when people have to update their scripts and config files when "fred" alone doesn't do what they're expecting anymore. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
[ovmf test] 184423: all pass - PUSHED
flight 184423 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/184423/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 0b09397dfa0123b9a27c2c52fd2ddafd7a902137 baseline version: ovmf 0c6d29be8b1731ff6880d59e0189184395e45968 Last test of basis 184421 2024-01-22 03:11:07 Z0 days Testing same since 184423 2024-01-22 11:14:34 Z0 days1 attempts People who touched revisions under test: Patrick Rudolph jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass test-amd64-i386-xl-qemuu-ovmf-amd64 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git 0c6d29be8b..0b09397dfa 0b09397dfa0123b9a27c2c52fd2ddafd7a902137 -> xen-tested-master
Re: [PATCH v13 07/35] x86/fred: Disable FRED support if CONFIG_X86_FRED is disabled
On Tue, Dec 05, 2023 at 02:49:56AM -0800, Xin Li wrote: > From: "H. Peter Anvin (Intel)" > > Add CONFIG_X86_FRED to to make > cpu_feature_enabled() work correctly with FRED. > > Originally-by: Megha Dey > Signed-off-by: H. Peter Anvin (Intel) > Tested-by: Shan Kang > Signed-off-by: Xin Li > --- > > Changes since v10: > * FRED feature is defined in cpuid word 12, not 13 (Nikolay Borisov). > --- > arch/x86/include/asm/disabled-features.h | 8 +++- > tools/arch/x86/include/asm/disabled-features.h | 8 +++- > 2 files changed, 14 insertions(+), 2 deletions(-) Whoever applies this: this one and the previous one can be merged into one patch. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
[linux-linus test] 184422: tolerable FAIL - PUSHED
flight 184422 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/184422/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 184418 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 184418 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 184418 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 184418 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 184418 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 184418 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 184418 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 184418 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-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-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-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass version targeted for testing: linux6613476e225e090cc9aad49be7fa504e290dd33d baseline version: linux4fbbed7872677b0a28ba8237169968171a61efbd Last test of basis 184418 2024-01-21 19:40:22 Z0 days Testing same since 184422 2024-01-22 03:31:56 Z0 days1 attempts People who touched revisions under test: Colin Ian King Kent Overstreet Linus Torvalds Su Yue jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-arm64-libvirt pass
Re: Sketch of an idea for handling the "mixed workload" problem
On Mon, Jan 22, 2024 at 1:02 PM George Dunlap wrote: > 2. two VMs running kernbench, but not competing for vcpu > 3. four VMs running kernbench, competing for vcpus Sorry, this should be competing for *P*cpus -George
Re: Sketch of an idea for handling the "mixed workload" problem
On Mon, Jan 22, 2024 at 12:50 PM Marek Marczykowski-Górecki wrote: > > On Mon, Jan 22, 2024 at 12:25:58PM +, George Dunlap wrote: > > On Mon, Jan 22, 2024 at 12:17 PM Marek Marczykowski-Górecki > > wrote: > > > > > > On Mon, Jan 22, 2024 at 11:54:14AM +, George Dunlap wrote: > > > > The other issue I have with this (and essentially where I got stuck > > > > developing credit2 in the first place) is testing: how do you ensure > > > > that it has the properties that you expect? > > > > > > Audio is actually quite nice use case at this, since it's quite > > > sensitive for scheduling jitter. I think even a simple "PCI passthrough a > > > sound card and play/record something" should show results. Especially > > > you can measure how hard you can push the system (for example artificial > > > load in other domains) until it breaks. > > > > Are we going have a gitlab runner which says, "Marek sits in front of > > his test machine and listens to audio for pops"? :-) > > Kinda ;) > We have already audio tests in qubes CI. They do more or less the above, > but using our audio virtualization. Play something, record in another > domain, and compare. Running the very same thing in gitlab-ci may be too > complicated (require bringing in some qubes infrastructure to make PV > audio work), but maybe similar test can be done based on qemu-emulated > audio or other pv audio solution? > > > > > How do you develop a > > > > "regression test" to make sure that server-based workloads don't have > > > > issues in this sort of case? > > > > > > For this I believe there are several benchmarking methods already, > > > starting with old trusty "Linux kernel build time". > > > > First of all, AFAICT "Linux kernel bulid time" is not representative > > of almost any actual server workload; and the end-to-end throughput > > completely misses what most server workloads will actually care about, > > like latency. > > > > Secondly, what you're testing isn't the performance of a single > > workload on an empty system; you're testing how workloads *interact*. > > If you want ideal throughput for a single workload on an empty system, > > use the null scheduler; more complex schedulers are only necessary > > when multiple different workloads interact. > > I should have clarified I meant `make -jNN`. But still, that's the same > workload on multiple vCPUs. See, you're still not getting it. :-) What you need is not multiple vcpus across a single VM, but multiple instances of different workloads across different VMs. For example: 1. One VM running kernbench 2. two VMs running kernbench, but not competing for vcpu 3. four VMs running kernbench, competing for vcpus 4. three VMs running kernbench, and one playing audio 5. four VMs running kernbench, one of which is *also* playing audio And then you have to collect several metrics: 1. Total kernbench throughput of entire system 2. Kernbench performance of each VM, compared with expected "fair share" 3. Some measure of latency for the audio VM And figure out how to compare trade-offs -- how much total throughput hit should we tolerate to increase fairness? How much fairness hit should we take to decrease latency? And as I said, kernbench isn't really a great server workload; you should do something request-based, measuring both throughput and latency. -George
Re: Sketch of an idea for handling the "mixed workload" problem
On Mon, Jan 22, 2024 at 12:25:58PM +, George Dunlap wrote: > On Mon, Jan 22, 2024 at 12:17 PM Marek Marczykowski-Górecki > wrote: > > > > On Mon, Jan 22, 2024 at 11:54:14AM +, George Dunlap wrote: > > > The other issue I have with this (and essentially where I got stuck > > > developing credit2 in the first place) is testing: how do you ensure > > > that it has the properties that you expect? > > > > Audio is actually quite nice use case at this, since it's quite > > sensitive for scheduling jitter. I think even a simple "PCI passthrough a > > sound card and play/record something" should show results. Especially > > you can measure how hard you can push the system (for example artificial > > load in other domains) until it breaks. > > Are we going have a gitlab runner which says, "Marek sits in front of > his test machine and listens to audio for pops"? :-) Kinda ;) We have already audio tests in qubes CI. They do more or less the above, but using our audio virtualization. Play something, record in another domain, and compare. Running the very same thing in gitlab-ci may be too complicated (require bringing in some qubes infrastructure to make PV audio work), but maybe similar test can be done based on qemu-emulated audio or other pv audio solution? > > > How do you develop a > > > "regression test" to make sure that server-based workloads don't have > > > issues in this sort of case? > > > > For this I believe there are several benchmarking methods already, > > starting with old trusty "Linux kernel build time". > > First of all, AFAICT "Linux kernel bulid time" is not representative > of almost any actual server workload; and the end-to-end throughput > completely misses what most server workloads will actually care about, > like latency. > > Secondly, what you're testing isn't the performance of a single > workload on an empty system; you're testing how workloads *interact*. > If you want ideal throughput for a single workload on an empty system, > use the null scheduler; more complex schedulers are only necessary > when multiple different workloads interact. I should have clarified I meant `make -jNN`. But still, that's the same workload on multiple vCPUs. > FWIW this was my first stab at trying to be systematic about testing > the scheduler: > > https://github.com/gwd/schedbench > > The rump kernel project has basically died AFAIK, so anyone trying to > resurrect this would probably have to try to rebase that bit of it > against something like XTF or unikernels. > > -George -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab signature.asc Description: PGP signature
Fwd: The Xen Project’s 20th Anniversary - Upcoming Social Event!
Hi all, A reminder our next social will be on *Wednesday 21st February 2024 in Cambridge! * Please reply to me directly if you're interested in attending. Have a chance to connect with the community! Food and drinks will be provided. Many thanks, Kelly Choi Community Manager Xen Project -- Forwarded message - From: Kelly Choi Date: Thu, Nov 23, 2023 at 1:01 PM Subject: The Xen Project’s 20th Anniversary - Upcoming Social Event! To: , , *The Xen Project’s 20th Anniversary* *Let's get together for an informal social, likely to be pizza/drinks and getting involved with the community. I hope to run these in future locations to give everyone a chance to attend.* *Date placeholder: Wednesday 21st February 2024* *Location: Cambridge* *Details TBC - **If you're interested, please reply to me directly and I will add you to the list.* *Celebrating Two Decades of Innovation* It’s hard to believe that two decades have passed since the inception of the Xen Project, a trailblazing force in the world of open-source virtualization. As we raise our glasses to commemorate this momentous occasion, it’s not just a celebration of time but a reflection on the incredible journey that has defined the Xen Project’s legacy. *A Legacy of Innovation* In the year 2003, the Xen Project emerged as a pioneering open-source hypervisor, laying the groundwork for some of the most influential cloud infrastructures that shape our digital landscape today. Over the past 20 years, the Xen Project has not only endured but has thrived, continuously evolving to meet the dynamic demands of the ever-changing tech landscape. *Driving Technological Frontiers* >From data center and server virtualization to cloud computing, desktop virtualization, and fortifying desktop security and hardware appliances, the Xen Project has been at the forefront of driving technological innovation. With 20 years of relentless development, it has become synonymous with reliability, scalability, and adaptability. *Venturing into New Horizons* As we celebrate this milestone, we also look forward to the exciting new territories that the Xen Project is venturing into. From embedded virtualization to even making strides in the automotive industry, the Xen Project continues to push boundaries and redefine what’s possible in the world of open-source virtualization. *The Annual Event: Xen Project Developer and Design Summit* At the heart of this remarkable journey is the Xen Project Developer and Design Summit, an annual gathering of the community’s brilliant minds and power users. More than just a conference, it’s a celebration of idea exchange, a showcase of the latest advancements, a platform for sharing invaluable experiences, and a forum for strategic planning and collaborative efforts. Be sure to look out for our upcoming event in 2024. *A Vibrant Community Defining the Future* Beyond the code and technological achievements, the Xen Project’s strength lies in its vibrant community. It’s a community that has come together to celebrate successes, overcome challenges, and collectively shape the future of open-source virtualization technology. Even to this day, community contributions and reviews are still going! *Looking Ahead* As we commemorate 20 years of innovation, we also eagerly anticipate the next chapter in the Xen Project’s journey. With gratitude for the past and excitement for the future, we extend our deepest thanks to everyone who has contributed to this incredible legacy. Here’s to 20 years of pushing boundaries, fostering collaboration, and shaping the digital landscape. Happy anniversary, Xen Project! The best is yet to come and I can’t wait to see what we all achieve. Many thanks, Kelly Choi Open Source Community Manager XenServer, Cloud Software Group
Re: Sketch of an idea for handling the "mixed workload" problem
On Mon, Jan 22, 2024 at 12:17 PM Marek Marczykowski-Górecki wrote: > > On Mon, Jan 22, 2024 at 11:54:14AM +, George Dunlap wrote: > > The other issue I have with this (and essentially where I got stuck > > developing credit2 in the first place) is testing: how do you ensure > > that it has the properties that you expect? > > Audio is actually quite nice use case at this, since it's quite > sensitive for scheduling jitter. I think even a simple "PCI passthrough a > sound card and play/record something" should show results. Especially > you can measure how hard you can push the system (for example artificial > load in other domains) until it breaks. Are we going have a gitlab runner which says, "Marek sits in front of his test machine and listens to audio for pops"? :-) > > > How do you develop a > > "regression test" to make sure that server-based workloads don't have > > issues in this sort of case? > > For this I believe there are several benchmarking methods already, > starting with old trusty "Linux kernel build time". First of all, AFAICT "Linux kernel bulid time" is not representative of almost any actual server workload; and the end-to-end throughput completely misses what most server workloads will actually care about, like latency. Secondly, what you're testing isn't the performance of a single workload on an empty system; you're testing how workloads *interact*. If you want ideal throughput for a single workload on an empty system, use the null scheduler; more complex schedulers are only necessary when multiple different workloads interact. FWIW this was my first stab at trying to be systematic about testing the scheduler: https://github.com/gwd/schedbench The rump kernel project has basically died AFAIK, so anyone trying to resurrect this would probably have to try to rebase that bit of it against something like XTF or unikernels. -George
Re: Sketch of an idea for handling the "mixed workload" problem
On Mon, Jan 22, 2024 at 11:54:14AM +, George Dunlap wrote: > The other issue I have with this (and essentially where I got stuck > developing credit2 in the first place) is testing: how do you ensure > that it has the properties that you expect? Audio is actually quite nice use case at this, since it's quite sensitive for scheduling jitter. I think even a simple "PCI passthrough a sound card and play/record something" should show results. Especially you can measure how hard you can push the system (for example artificial load in other domains) until it breaks. > How do you develop a > "regression test" to make sure that server-based workloads don't have > issues in this sort of case? For this I believe there are several benchmarking methods already, starting with old trusty "Linux kernel build time". -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab signature.asc Description: PGP signature
Re: Sketch of an idea for handling the "mixed workload" problem
On Mon, Jan 22, 2024 at 12:31 AM Demi Marie Obenour wrote: > > On Fri, Sep 29, 2023 at 05:42:16PM +0100, George Dunlap wrote: > > The basic credit2 algorithm goes something like this: > > > > 1. All vcpus start with the same number of credits; about 10ms worth > > if everyone has the same weight > > > > 2. vcpus burn credits as they consume cpu, based on the relative > > weights: higher weights burn slower, lower weights burn faster > > > > 3. At any given point in time, the runnable vcpu with the highest > > credit is allowed to run > > > > 4. When the "next runnable vcpu" on a runqueue is negative, credit is > > reset: everyone gets another 10ms, and can carry over at most 2ms of > > credit over the reset. > > > > Generally speaking, vcpus that use less than their quota and have lots > > of interrupts are scheduled immediately, since when they wake up they > > always have more credit than the vcpus who are burning through their > > slices. > > > > But what about a situation as described recently on Matrix, where a VM > > uses a non-negligible amount of cpu doing un-accelerated encryption > > and decryption, which can be delayed by a few MS, as well as handling > > audio events? How can we make sure that: > > > > 1. We can run whenever interrupts happen > > 2. We get no more than our fair share of the cpu? > > > > The counter-intuitive key here is that in order to achieve the above, > > you need to *deschedule or preempt early*, so that when the interrupt > > comes, you have spare credit to run the interrupt handler. How do we > > manage that? > > > > The idea I'm working out comes from a phrase I used in the Matrix > > discussion, about a vcpu that "foolishly burned all its credits". > > Naturally the thing you want to do to have credits available is to > > save them up. > > > > So the idea would be this. Each vcpu would have a "boost credit > > ratio" and a "default boost interval"; there would be sensible > > defaults based on typical workloads, but these could be tweaked for > > individual VMs. > > > > When credit is assigned, all VMs would get the same amount of credit, > > but divided into two "buckets", according to the boost credit ratio. > > > > Under certain conditions, a vcpu would be considered "boosted"; this > > state would last either until the default boost interval, or until > > some other event (such as a de-boost yield). > > > > The queue would be sorted thus: > > > > * Boosted vcpus, by boost credit available > > * Non-boosted vcpus, by non-boost credit available > > > > Getting more boost credit means having lower priority when not > > boosted; and burning through your boost credit means not being > > scheduled when you need to be. > > > > Other ways we could consider putting a vcpu into a boosted state (some > > discussed on Matrix or emails linked from Matrix): > > * Xen is about to preempt, but finds that the vcpu interrupts are > > blocked (this sort of overlaps with the "when we deliver an interrupt" > > one) > > * Xen is about to preempt, but finds that the (currently out-of-tree) > > "dont_desched" bit has been set in the shared memory area > > I think both of these would be good. Another one would be when Xen is > about to deliver an interrupt to a guest, provided that there is no > storm of interrupts. I’ve seen a USB webcam cause a system-wide latency > spike through what I presume is an interrupt storm, and I suspect that > others have observed similar behavior with USB external drives. How would you determine that a given interrupt was part of a "storm", and what would you do differently as a result of determining that? > > Other ways to consider de-boosting: > > * There's a way to trigger a VMEXIT when interrupts have been > > re-enabled; setting this up when the VM is in the boost state > > That’s a good idea, but should be conditional on “dont_desched” _not_ > being set. This handles the case where the guest is running a realtime > thread. In which case we need some way for the "enlightened" guest to know how to de-boost itself; a yield might do. > Generally, I’d like to see something like this: > > - A vCPU with sufficient boost credit is boosted by Xen under the > following conditions: > > 1. Xen interrupts the guest. I take it you mean, "delivers an interrupt to the guest"? > 2. Xen is about to preempt, but detects that “dont_desched” is set. > 3. Xen is about to preempt, but detects that interrupts are disabled. > > - A vCPU is deboosted if: > > 1. It runs out of boost credit, even if “dont_desched” is set. > 2. An interrupt handler returns, but only if “dont_desched” is not set. > 3. Interrupts are re-enabled, but only if “dont_desched” is not set. > > The first case is an abnormal condition and typically means that > either the system is overloaded or a vCPU is running boosted for too > long. To help debug this situation, Xen will log a warning and > increment both a system-wide and a per-domain counter. dom0 can > retrieve
Community call recording: 18th January 2024
Hi all, After receiving community feedback, our monthly calls will be recorded. This will enable members who couldn't attend on the day to listen in on discussions. Currently, these are unlisted and published on our new YouTube channel. Only users with the specific link below will be able to access the recording. https://youtu.be/eIkPU6l4bzY?si=bBH2J2SF13KwAga6 Many thanks, Kelly Choi Community Manager Xen Project
[PATCH v2] lib{fdt,elf}: move lib{fdt,elf}-temp.o and their deps to $(targets)
At the moment, trying to run xencov read/reset (calling SYSCTL_coverage_op
under the hood) results in a crash. This is due to a profiler trying to
access data in the .init.* sections (libfdt for Arm and libelf for x86)
that are stripped after boot. Normally, the build system compiles any
*.init.o file without COV_FLAGS. However, these two libraries are
handled differently as sections will be renamed to init after linking.
To override COV_FLAGS to empty for these libraries, lib{fdt,elf}.o were
added to nocov-y. This worked until e321576f4047 ("xen/build: start using
if_changed") that added lib{fdt,elf}-temp.o and their deps to extra-y.
This way, even though these objects appear as prerequisites of
lib{fdt,elf}.o and the settings should propagate to them, make can also
build them as a prerequisite of __build, in which case COV_FLAGS would
still have the unwanted flags. Fix it by switching to $(targets) instead.
Also, for libfdt, append libfdt.o to nocov-y only if CONFIG_OVERLAY_DTB
is not set. Otherwise, there is no section renaming and we should be able
to run the coverage.
Fixes: e321576f4047 ("xen/build: start using if_changed")
Signed-off-by: Michal Orzel
---
Changes in v2:
- was "coverage: filter out lib{fdt,elf}-temp.o"
- switch to $(targets), update rationale
---
xen/common/libelf/Makefile | 2 +-
xen/common/libfdt/Makefile | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/xen/common/libelf/Makefile b/xen/common/libelf/Makefile
index 8a4522e4e141..917d12b006f7 100644
--- a/xen/common/libelf/Makefile
+++ b/xen/common/libelf/Makefile
@@ -13,4 +13,4 @@ $(obj)/libelf.o: $(obj)/libelf-temp.o FORCE
$(obj)/libelf-temp.o: $(addprefix $(obj)/,$(libelf-objs)) FORCE
$(call if_changed,ld)
-extra-y += libelf-temp.o $(libelf-objs)
+targets += libelf-temp.o $(libelf-objs)
diff --git a/xen/common/libfdt/Makefile b/xen/common/libfdt/Makefile
index d50487aa6e32..6ce679f98f47 100644
--- a/xen/common/libfdt/Makefile
+++ b/xen/common/libfdt/Makefile
@@ -5,10 +5,10 @@ SECTIONS := text data $(SPECIAL_DATA_SECTIONS)
# For CONFIG_OVERLAY_DTB, libfdt functionalities will be needed during runtime.
ifneq ($(CONFIG_OVERLAY_DTB),y)
OBJCOPYFLAGS := $(foreach s,$(SECTIONS),--rename-section .$(s)=.init.$(s))
+nocov-y += libfdt.o
endif
obj-y += libfdt.o
-nocov-y += libfdt.o
CFLAGS-y += -I$(srctree)/include/xen/libfdt/
@@ -18,4 +18,4 @@ $(obj)/libfdt.o: $(obj)/libfdt-temp.o FORCE
$(obj)/libfdt-temp.o: $(addprefix $(obj)/,$(LIBFDT_OBJS)) FORCE
$(call if_changed,ld)
-extra-y += libfdt-temp.o $(LIBFDT_OBJS)
+targets += libfdt-temp.o $(LIBFDT_OBJS)
--
2.25.1
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
On 22.01.2024 12:05, Anthony PERARD wrote: > On Mon, Jan 22, 2024 at 10:54:13AM +, Anthony PERARD wrote: >> On Mon, Jan 22, 2024 at 11:04:41AM +0100, Jan Beulich wrote: >>> On 19.01.2024 16:25, Anthony PERARD wrote: On Fri, Jan 19, 2024 at 09:43:30AM +0100, Michal Orzel wrote: > Is my understanding correct that by switching from extra-y to targets we > are preventing these objects to > appear in non-init-objects (and thus having COV_FLAGS appended) while > retaining the proper if_changed behavior? > > According to docs/misc/xen-makefiles/makefiles.rst: > Any target that utilises if_changed must be listed in $(targets), > otherwise the command line check will fail, and the target will > always be built. Indeed, and $(extra-y) is added to $(targets) via $(targets-for-builtin). While switching from $(extra-y) to $(targets) prevents the objects from been added to $(non-init-objets), it doesn't matter because "libelf.o" is in that variable, so $(COV_FLAGS) is added to $(_c_flags) and its value is used in all the prerequisites of "libelf.o" which includes "libelf-temp.o" and for example "libelf-dominfo.o". So the only thing preventing $(COV_FLAGS) from been added when building "libelf-tools.o" for example is that we set `COV_FLAGS:=` for "libelf.o". >>> >>> Yet doesn't that (again) mean things should actually work as-is, [...] >> >> No, because I've explain how it should work, in the hypothetical world >> where we have `targets += libelf-temp.o $(libelf-objs)`. > > The problem is that there's currently two "paths" to build libelf-temp.o > (and even three I think for libelf-tools.o libelf-loader.o > libelf-dominfo.o): > > Simplified makefile: > > obj-y := libelf.o > extra-y := libelf-temp.o > COV_FLAGS := -fcoverage > > __build: $(extra-y) built_in.o Oh, okay - this is the piece I was missing. Thanks for the explanation. Jan > built_in.o: $(obj-y) > libelf.o: COV_FLAGS := > libelf.o: libelf-temp.o > > So, make can build "libelf-temp.o" as prerequisite of "__build" the > default target, or as prerequisite of "libelf.o". > In the first case, COV_FLAGS would have all the flags, and in the second > case, COV_FLAGS would be reset, but that second case is too late as make > is more likely to have already built libelf-temp.o with all the flags. > > Cheers, >
Re: [PATCH v5] x86/livepatch: align functions to ensure minimal distance between entry points
On 22.01.2024 12:02, Roger Pau Monne wrote: > The minimal function size requirements for an x86 livepatch are either 5 bytes > (for jmp) or 9 bytes (for endbr + jmp), and always 4 bytes on Arm. Ensure > that > distance between functions entry points is always at least of the minimal > required size for livepatch instruction replacement to be successful. > > Add an additional align directive to the linker script, in order to ensure > that > the next section placed after the .text.* (per-function sections) is also > aligned to the required boundary, so that the distance of the last function > entry point with the next symbol is also of minimal size. > > Note that it's possible for the compiler to end up using a higher function > alignment regardless of the passed value, so this change just make sure that > the minimum required for livepatch to work is present. Different compilers > handle the option differently, as clang will ignore -falign-functions value > if it's smaller than the one that would be set by the optimization level, > while > gcc seems to always honor the function alignment passed in -falign-functions. > In order to cope with this behavior and avoid that setting -falign-functions > results in an alignment inferior to what the optimization level would have > selected force x86 release builds to use a function alignment of 16 bytes. Nit: A comma after "selected" may help readability. > --- a/xen/Kconfig > +++ b/xen/Kconfig > @@ -37,6 +37,27 @@ config CC_HAS_VISIBILITY_ATTRIBUTE > config CC_SPLIT_SECTIONS > bool > > +# Set function alignment. > +# > +# Allow setting on a boolean basis, and then convert such selection to an > +# integer for the build system and code to consume more easily. > +# > +# Requires clang >= 7.0.0 > +config CC_HAS_FUNCTION_ALIGNMENT > + def_bool $(cc-option,-falign-functions) Nit: Maybe better have a blank line here? > +config FUNCTION_ALIGNMENT_4B > + bool > +config FUNCTION_ALIGNMENT_8B > + bool > +config FUNCTION_ALIGNMENT_16B > + bool > +config FUNCTION_ALIGNMENT > + int > + depends on CC_HAS_FUNCTION_ALIGNMENT > + default 16 if FUNCTION_ALIGNMENT_16B > + default 8 if FUNCTION_ALIGNMENT_8B > + default 4 if FUNCTION_ALIGNMENT_4B > + > source "arch/$(SRCARCH)/Kconfig" > > config DEFCONFIG_LIST >[...] > --- a/xen/arch/x86/xen.lds.S > +++ b/xen/arch/x86/xen.lds.S > @@ -99,6 +99,10 @@ SECTIONS > *(.text) > #ifdef CONFIG_CC_SPLIT_SECTIONS > *(.text.*) > +#endif > +#ifdef CONFIG_FUNCTION_ALIGNMENT > + /* Ensure enough distance with the next placed section. */ > + . = ALIGN(CONFIG_FUNCTION_ALIGNMENT); > #endif > *(.text.__x86_indirect_thunk_*) I continue to fail to see how an alignment directive can guarantee minimum distance. In the worst case such a directive inserts nothing at all. IOW at the very least there's a non-spelled-out assumption here about the last item in the earlier section having suitable alignment and thus, if small in size, being suitably padded. Personally I don't think merely spelling out such a requirement would help - it would end up being a trap for someone to fall into. I'm further curious why .text.__x86_indirect_thunk_* is left past the inserted alignment. While pretty unlikely, isn't it in principle possible for the thunks there to also need patching? Aren't we instead requiring then that assembly functions (and thunks) all be suitably aligned as well? Jan
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
On 22/01/2024 12:05, Anthony PERARD wrote: > > > On Mon, Jan 22, 2024 at 10:54:13AM +, Anthony PERARD wrote: >> On Mon, Jan 22, 2024 at 11:04:41AM +0100, Jan Beulich wrote: >>> On 19.01.2024 16:25, Anthony PERARD wrote: On Fri, Jan 19, 2024 at 09:43:30AM +0100, Michal Orzel wrote: > Is my understanding correct that by switching from extra-y to targets we > are preventing these objects to > appear in non-init-objects (and thus having COV_FLAGS appended) while > retaining the proper if_changed behavior? > > According to docs/misc/xen-makefiles/makefiles.rst: > Any target that utilises if_changed must be listed in $(targets), > otherwise the command line check will fail, and the target will > always be built. Indeed, and $(extra-y) is added to $(targets) via $(targets-for-builtin). While switching from $(extra-y) to $(targets) prevents the objects from been added to $(non-init-objets), it doesn't matter because "libelf.o" is in that variable, so $(COV_FLAGS) is added to $(_c_flags) and its value is used in all the prerequisites of "libelf.o" which includes "libelf-temp.o" and for example "libelf-dominfo.o". So the only thing preventing $(COV_FLAGS) from been added when building "libelf-tools.o" for example is that we set `COV_FLAGS:=` for "libelf.o". >>> >>> Yet doesn't that (again) mean things should actually work as-is, [...] >> >> No, because I've explain how it should work, in the hypothetical world >> where we have `targets += libelf-temp.o $(libelf-objs)`. > > The problem is that there's currently two "paths" to build libelf-temp.o > (and even three I think for libelf-tools.o libelf-loader.o > libelf-dominfo.o): > > Simplified makefile: > > obj-y := libelf.o > extra-y := libelf-temp.o > COV_FLAGS := -fcoverage > > __build: $(extra-y) built_in.o > built_in.o: $(obj-y) > libelf.o: COV_FLAGS := > libelf.o: libelf-temp.o > > So, make can build "libelf-temp.o" as prerequisite of "__build" the > default target, or as prerequisite of "libelf.o". > In the first case, COV_FLAGS would have all the flags, and in the second > case, COV_FLAGS would be reset, but that second case is too late as make > is more likely to have already built libelf-temp.o with all the flags. Thanks for detailed explanation. I will follow your rationale in v2. ~Michal
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
On Mon, Jan 22, 2024 at 10:54:13AM +, Anthony PERARD wrote: > On Mon, Jan 22, 2024 at 11:04:41AM +0100, Jan Beulich wrote: > > On 19.01.2024 16:25, Anthony PERARD wrote: > > > On Fri, Jan 19, 2024 at 09:43:30AM +0100, Michal Orzel wrote: > > >> Is my understanding correct that by switching from extra-y to targets we > > >> are preventing these objects to > > >> appear in non-init-objects (and thus having COV_FLAGS appended) while > > >> retaining the proper if_changed behavior? > > >> > > >> According to docs/misc/xen-makefiles/makefiles.rst: > > >> Any target that utilises if_changed must be listed in $(targets), > > >> otherwise the command line check will fail, and the target will > > >> always be built. > > > > > > Indeed, and $(extra-y) is added to $(targets) via > > > $(targets-for-builtin). > > > > > > While switching from $(extra-y) to $(targets) prevents the objects from > > > been added to $(non-init-objets), it doesn't matter because "libelf.o" > > > is in that variable, so $(COV_FLAGS) is added to $(_c_flags) and its > > > value is used in all the prerequisites of "libelf.o" which includes > > > "libelf-temp.o" and for example "libelf-dominfo.o". So the only thing > > > preventing $(COV_FLAGS) from been added when building "libelf-tools.o" > > > for example is that we set `COV_FLAGS:=` for "libelf.o". > > > > Yet doesn't that (again) mean things should actually work as-is, [...] > > No, because I've explain how it should work, in the hypothetical world > where we have `targets += libelf-temp.o $(libelf-objs)`. The problem is that there's currently two "paths" to build libelf-temp.o (and even three I think for libelf-tools.o libelf-loader.o libelf-dominfo.o): Simplified makefile: obj-y := libelf.o extra-y := libelf-temp.o COV_FLAGS := -fcoverage __build: $(extra-y) built_in.o built_in.o: $(obj-y) libelf.o: COV_FLAGS := libelf.o: libelf-temp.o So, make can build "libelf-temp.o" as prerequisite of "__build" the default target, or as prerequisite of "libelf.o". In the first case, COV_FLAGS would have all the flags, and in the second case, COV_FLAGS would be reset, but that second case is too late as make is more likely to have already built libelf-temp.o with all the flags. Cheers, -- Anthony PERARD
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
On 22.01.2024 11:54, Anthony PERARD wrote: > On Mon, Jan 22, 2024 at 11:04:41AM +0100, Jan Beulich wrote: >> On 19.01.2024 16:25, Anthony PERARD wrote: >>> On Fri, Jan 19, 2024 at 09:43:30AM +0100, Michal Orzel wrote: Is my understanding correct that by switching from extra-y to targets we are preventing these objects to appear in non-init-objects (and thus having COV_FLAGS appended) while retaining the proper if_changed behavior? According to docs/misc/xen-makefiles/makefiles.rst: Any target that utilises if_changed must be listed in $(targets), otherwise the command line check will fail, and the target will always be built. >>> >>> Indeed, and $(extra-y) is added to $(targets) via >>> $(targets-for-builtin). >>> >>> While switching from $(extra-y) to $(targets) prevents the objects from >>> been added to $(non-init-objets), it doesn't matter because "libelf.o" >>> is in that variable, so $(COV_FLAGS) is added to $(_c_flags) and its >>> value is used in all the prerequisites of "libelf.o" which includes >>> "libelf-temp.o" and for example "libelf-dominfo.o". So the only thing >>> preventing $(COV_FLAGS) from been added when building "libelf-tools.o" >>> for example is that we set `COV_FLAGS:=` for "libelf.o". >> >> Yet doesn't that (again) mean things should actually work as-is, [...] > > No, because I've explain how it should work, in the hypothetical world > where we have `targets += libelf-temp.o $(libelf-objs)`. Yes and no: Why would the COV_FLAGS propagation to prereqs not work today? Whether libelf-*.o are prereqs to libelf-temp.o or to libelf.o shouldn't matter, nor should it matter whether libelf-temp.o or libelf.o (or both) are listed in $(targets). As soon as either libelf-temp.o or libelf.o has COV_FLAGS overridden (to empty), libelf-*.o ought to be built with empty COV_FLAGS. What am I missing? Jan
[PATCH v5] x86/livepatch: align functions to ensure minimal distance between entry points
The minimal function size requirements for an x86 livepatch are either 5 bytes
(for jmp) or 9 bytes (for endbr + jmp), and always 4 bytes on Arm. Ensure that
distance between functions entry points is always at least of the minimal
required size for livepatch instruction replacement to be successful.
Add an additional align directive to the linker script, in order to ensure that
the next section placed after the .text.* (per-function sections) is also
aligned to the required boundary, so that the distance of the last function
entry point with the next symbol is also of minimal size.
Note that it's possible for the compiler to end up using a higher function
alignment regardless of the passed value, so this change just make sure that
the minimum required for livepatch to work is present. Different compilers
handle the option differently, as clang will ignore -falign-functions value
if it's smaller than the one that would be set by the optimization level, while
gcc seems to always honor the function alignment passed in -falign-functions.
In order to cope with this behavior and avoid that setting -falign-functions
results in an alignment inferior to what the optimization level would have
selected force x86 release builds to use a function alignment of 16 bytes.
For Arm the default compiler selection of function alignment matches the
requirements of livepatch, which are 4 bytes.
The compiler option -falign-functions is not available on at least clang 3.8,
so introduce a Kconfig check for it and make the livepatch option depend on the
compiler supporting the option.
The naming of the option(s) CONFIG_FUNCTION_ALIGNMENT is explicitly not
mentioning CC in preparation for the option also being used by assembly code.
Signed-off-by: Roger Pau Monné
---
Changes since v4:
- Split from the rest of the livepatch testing series.
- Reword and expand a bit the commit message.
- Add a comment about falign-functions clang version requirement.
Changes since v3:
- Test for compiler option with -falign-functions.
- Make FUNCTION_ALIGNMENT depend on CC_HAS_FUNCTION_ALIGNMENT.
- Set 16byte function alignment for x86 release builds.
Changes since v2:
- Add Arm side.
- Align end of section in the linker script to ensure enough padding for the
last function.
- Expand commit message and subject.
- Rework Kconfig options.
- Check that the compiler supports the option.
Changes since v1:
- New in this version.
---
xen/Kconfig | 21 +
xen/Makefile | 3 +++
xen/arch/arm/livepatch.c | 2 ++
xen/arch/arm/xen.lds.S | 4
xen/arch/x86/Kconfig | 1 +
xen/arch/x86/livepatch.c | 4
xen/arch/x86/xen.lds.S | 4
xen/common/Kconfig | 5 -
8 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/xen/Kconfig b/xen/Kconfig
index 134e6e68ad84..feb5fa5ecb0a 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -37,6 +37,27 @@ config CC_HAS_VISIBILITY_ATTRIBUTE
config CC_SPLIT_SECTIONS
bool
+# Set function alignment.
+#
+# Allow setting on a boolean basis, and then convert such selection to an
+# integer for the build system and code to consume more easily.
+#
+# Requires clang >= 7.0.0
+config CC_HAS_FUNCTION_ALIGNMENT
+ def_bool $(cc-option,-falign-functions)
+config FUNCTION_ALIGNMENT_4B
+ bool
+config FUNCTION_ALIGNMENT_8B
+ bool
+config FUNCTION_ALIGNMENT_16B
+ bool
+config FUNCTION_ALIGNMENT
+ int
+ depends on CC_HAS_FUNCTION_ALIGNMENT
+ default 16 if FUNCTION_ALIGNMENT_16B
+ default 8 if FUNCTION_ALIGNMENT_8B
+ default 4 if FUNCTION_ALIGNMENT_4B
+
source "arch/$(SRCARCH)/Kconfig"
config DEFCONFIG_LIST
diff --git a/xen/Makefile b/xen/Makefile
index 21832d640225..162cb2bda1c5 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -390,6 +390,9 @@ CFLAGS += -fomit-frame-pointer
endif
CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections
+ifdef CONFIG_FUNCTION_ALIGNMENT
+CFLAGS += -falign-functions=$(CONFIG_FUNCTION_ALIGNMENT)
+endif
CFLAGS += -nostdinc -fno-builtin -fno-common
CFLAGS += -Werror -Wredundant-decls -Wwrite-strings -Wno-pointer-arith
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index bbca1e5a5ed3..aa8ae8c38d28 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -68,6 +68,8 @@ void arch_livepatch_revive(void)
int arch_livepatch_verify_func(const struct livepatch_func *func)
{
+BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > CONFIG_FUNCTION_ALIGNMENT);
+
/* If NOPing only do up to maximum amount we can put in the ->opaque. */
if ( !func->new_addr && (func->new_size > LIVEPATCH_OPAQUE_SIZE ||
func->new_size % ARCH_PATCH_INSN_SIZE) )
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 59b80d122fd0..afaf1e996b0e 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -44,6 +44,10 @@ SECTIONS
#ifdef CONFIG_CC_SPLIT_SECTIONS
*(.text.*)
#endif
+#ifdef
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
On Mon, Jan 22, 2024 at 11:04:41AM +0100, Jan Beulich wrote: > On 19.01.2024 16:25, Anthony PERARD wrote: > > On Fri, Jan 19, 2024 at 09:43:30AM +0100, Michal Orzel wrote: > >> Is my understanding correct that by switching from extra-y to targets we > >> are preventing these objects to > >> appear in non-init-objects (and thus having COV_FLAGS appended) while > >> retaining the proper if_changed behavior? > >> > >> According to docs/misc/xen-makefiles/makefiles.rst: > >> Any target that utilises if_changed must be listed in $(targets), > >> otherwise the command line check will fail, and the target will > >> always be built. > > > > Indeed, and $(extra-y) is added to $(targets) via > > $(targets-for-builtin). > > > > While switching from $(extra-y) to $(targets) prevents the objects from > > been added to $(non-init-objets), it doesn't matter because "libelf.o" > > is in that variable, so $(COV_FLAGS) is added to $(_c_flags) and its > > value is used in all the prerequisites of "libelf.o" which includes > > "libelf-temp.o" and for example "libelf-dominfo.o". So the only thing > > preventing $(COV_FLAGS) from been added when building "libelf-tools.o" > > for example is that we set `COV_FLAGS:=` for "libelf.o". > > Yet doesn't that (again) mean things should actually work as-is, [...] No, because I've explain how it should work, in the hypothetical world where we have `targets += libelf-temp.o $(libelf-objs)`. -- Anthony PERARD
Re: [PATCH v5 8/8] common: honor CONFIG_CC_SPLIT_SECTIONS also for assembly functions
On 19.01.2024 11:36, Roger Pau Monné wrote: > On Mon, Jan 15, 2024 at 03:40:19PM +0100, Jan Beulich wrote: >> Leverage the new infrastructure in xen/linkage.h to also switch to per- >> function sections (when configured), deriving the specific name from the >> "base" section in use at the time FUNC() is invoked. >> >> Signed-off-by: Jan Beulich >> --- >> TBD: Since we use .subsection in UNLIKELY_START(), a perhaps not really >> wanted side effect of this change is that respective out-of-line >> code now moves much closer to its original (invoking) code. > > Hm, I'm afraid I don't have much useful suggestions here. > >> TBD: Of course something with the same overall effect, but less >> impactful might do in Config.mk. E.g. $(filter-out -D%,$(3)) >> instead of $(firstword (3)). > > I don't have a strong opinion re those two options My preference > however (see below for reasoning) would be to put this detection in > Kconfig. > >> TBD: On top of Roger's respective patch (for livepatch), also respect >> CONFIG_FUNCTION_ALIGNMENT. > > I think you can drop that, as the series is blocked. Considering the series here has been pending for quite some time, too, I guess I'd like to keep it just in case that other functionality becomes unblocked or available by some other means, even if only to remind myself. >> --- a/xen/Makefile >> +++ b/xen/Makefile >> @@ -409,6 +409,9 @@ AFLAGS += -D__ASSEMBLY__ >> >> $(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--noexecstack) >> >> +# Check to see whether the assmbler supports the --sectname-subst option. >> +$(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--sectname-subst >> -DHAVE_AS_SECTNAME_SUBST) > > I guess you already know what I'm going to comment on. I think this > would be clearer if it was a Kconfig option. For once because I think > we should gate livapatch support on the option being available, and > secondly it would avoid having to pass the extra -D around. > > I think it's relevant to have a consistent set of build tool options > requirements for livepatch support, so that when enabled the support > is consistent across builds. With this approach livepatch could be > enabled in Kconfig, but depending on the tools support the resulting > binary might or might not support live patching of assembly code. > Such behavior is IMO unhelpful from a user PoV, and can lead to > surprises down the road. I can see the desire to have LIVEPATCH grow such a dependency. Yet there is the bigger still open topic of the criteria towards what, if anything, to probe for in Kconfig, what, if anything, to probe for in Makefile, and which of the probing perhaps do in both places. I'm afraid my attempts to move us closer to a resolution (topic on summit, making proposals on list) have utterly failed. IOW I don't currently see how the existing disagreement there can be resolved, which will result in me to continue following the (traditional) Makefile approach unless I clearly view Kconfig superior in a particular case. I could perhaps be talked into following a "mixed Kconfig/Makefile model", along the lines of "x86: convert CET tool chain feature checks to mixed Kconfig/Makefile model", albeit I'm sure you're aware there are issues to sort out there, which I see no value in putting time into as long as I can't expect things to make progress subsequently. Dropping this patch, while an option, would seem undesirable to me, since even without Kconfig probing using new enough tool chains the splitting would yield reliable results, and provide - imo - an improvement over what we have right now. Jan
Re: Xen 4.18rc/ARM64 on Raspberry Pi 4B: Traffic in DomU crashing Dom0 when using VLANs
On Fri, Jan 19, 2024 at 8:32 PM Elliott Mitchell wrote: > > On Sun, Jan 14, 2024 at 10:54:24PM +0100, Paul Leiber wrote: > > > > Am 22.10.2023 um 07:42 schrieb Paul Leiber: > > > Am 13.10.2023 um 20:56 schrieb Paul Leiber: > > >> Hi Xen developers list, > > >> > > >> TL;DR: > > >> -- > > >> > > >> Causing certain web server traffic on a secondary VLAN on Raspberry Pi > > >> under vanilla Debian/UEFI in combination with Xen leads to complete > > >> system reboot (watchdog triggering for Dom0). Other strange things are > > >> happening. > > >> > > >> Description: > > >> -- > > >> > > >> I recently set up Xen (self compiled, Version 4.18-rc) on a Raspberry > > >> Pi 4B (on vanilla Debian Bookworm, UEFI boot mode). Until some time > > >> ago, everything worked well with Dom0, one DomU and one bridge. > > >> > > >> Then I wanted to actually make use of the virtualization and started > > >> to set up a second Debian Bookworm DomU (using xen-create-image) for > > >> monitoring my systems with zabbix (a webserver based system monitoring > > >> solution). The bridge used for this setup was the device bridging the > > >> hardware NIC. I installed zabbix, set it up, and everything went well, > > >> I could access the web interface without any problem. > > >> > > >> Then I set up VLANs (initally using VLAN numbers 1 and 2) to separate > > >> network traffic between the DomUs. I made the existing device bridge > > >> VLAN 1 (bridge 1) and created a secondary device for bridging VLAN 2 > > >> (bridge 2). Using only bridge 1 / VLAN 1 everything works well, I can > > >> access the zabbix web interface without any noticeable issue. After > > >> switching the zabbix DomU to VLAN 2 / bridge 2, everything seemingly > > >> keeps on working well, I can ping different devices in my network from > > >> the zabbix DomU and vice versa, I can ssh into the machine. > > >> > > >> However, as soon as I remotely access the zabbix web interface, the > > >> complete system (DomUs and Dom0) becomes unresponsive and reboots > > >> after some time (usually seconds, sometimes 1-2 minutes). The reboot > > >> is reliably reproducable. > > >> > > >> I didn't see any error message in any log (zabbix, DomU syslog, Dom0 > > >> syslog) except for the following lines immediately before the system > > >> reboots on the Xen serial console: > > >> > > >> (XEN) Watchdog timer fired for domain 0 > > >> (XEN) Hardware Dom0 shutdown: watchdog rebooting machine > > >> > > >> As soon as I change the bridge to bridge 1 (with or without VLAN > > >> setup), the web interface is accessible again after booting the zabbix > > >> DomU, no reboots. > > >> > > >> So I assume that causing specific traffic on the virtual NIC when > > >> using a VLAN setup with more than one VLAN under Xen makes the Dom0 > > >> system hard crash. Of course, there might be other causes that I'm not > > >> aware of, but to me, this seems to be the most likely explanation > > >> right now. > > >> > > >> What I tried: > > >> - > > >> > > >> 1. I changed the VLAN numbers. First to 101, 102, 103 etc. This was > > >> when I noticed another strange thing: VLANs with numbers >99 simply > > >> don't work on my Raspberry Pi under Debian, with or without Xen. VLAN > > >> 99 works, VLAN 100 (or everything else >99 that I tried) doesn't work. > > >> If I choose a number >99, the VLAN is not configured, "ip a" doesn't > > >> list it. Other Debian systems on x64 architecture don't show this > > >> behavior, there, it was no problem to set up VLANs > 99. Therefore, > > >> I've changed the VLANs to 10, 20, 30 etc., which worked. But it didn't > > >> solve the initial problem of the crashing Dom0 and DomUs. > > >> > > >> 2. Different bridge options, without noticable effect: > > >> bridge_stp off # dont use STP (spanning tree proto) > > >> bridge_waitport 0 # dont wait for port to be available > > >> bridge_fd 0 # no forward delay > > >> > > >> 3. Removing IPv6: No noticable effect. > > >> > > >> 4. Network traffic analysis: Now, here it becomes _really_ strange. I > > >> started tcpdumps on Dom0, and depending on on which interface/bridge > > >> traffic was logged, the problem went away, meaning, the DomU was > > >> running smoothly for hours, even when accessing the zabbix web > > >> interface. Stopping the log makes the system crash (as above, after > > >> seconds up to 1-2 minutes) reproducably if I access the zabbix web > > >> interface. > > >> > > >> Logging enabcm6e4ei0 (NIC): no crashes > > >> Logging enabcm6e4ei0.10 (VLAN 10): instant crash > > >> Logging enabcm6e4ei0.20 (VLAN 20): no crashes > > >> Logging xenbr0 (on VLAN 10): instant crash > > >> Logging xenbr1 (on VLAN 20): no crashes > > >> > > >> I am clinging to the thought that there must be a rational explanation > > >> for why logging the traffic on certain interfaces/bridges should avoid > > >> the crash of the complete system, while logging other > > >> interfaces/bridges doesn't. I myself can't
Re: Community Process Group - Proposal
On 17.01.2024 18:10, Kelly Choi wrote: > Hi everyone, > > I've spent a bit of time talking to various individuals and the advisory > board about setting up a new Community Process Group. > > A survey was recently conducted to identify how the community as a whole > feels about a certain situation. It was not intended to ban specific > wording or create a policy to do so, but more to give context that the > community has a wide range of ideas, and individuals may agree and disagree > a lot more frequently than we as individuals might think. It helps us > understand that as a community there are many situations where it is not > clear. As such, the results indicated a very even split among the > community, which indicates a larger problem as we may not always come to > agreement. > > There is obvious frustration with how certain matters are handled, as some > members may want the project to move faster, whereas others like to take a > cautious approach. Given we are an open source project, differences in > opinion are likely to happen and what we don’t want to do is cause further > frustration. > > *This is where I would like to propose the idea of a ‘Community Process > Group’.* > > A CPG can help as they will be able to advise members on similar issues > regarding community processes or appeals and decide on the best way > forward. To help with this process, I have consulted with various > individuals including some committers and conduct team members. > > *What is a CPG’s purpose?* > In the first instance, we would expect an informal vote resolves most > disagreements. However, there will be certain circumstances where the > outcome is not always agreed on. > > A CPG will be your second point of call, where you can escalate matters > quickly for a democratic solution. Between informal voting and this "second point of call", where does formal voting go? > Their purpose is to resolve process issues and informal vote appeals to > avoid matters going to a formal vote, but also act as a representative on > behalf of others in the community on future matters. > > For example: > >- Naming conventions >- Whether feedback requesting changes on a patch series is acceptable >- How to move forward in case of non-actionable feedback to a patch >series >- How to move forward when a contributor or reviewer has not been >responsive >- Policy questions not related to the code of conduct > > *What is their role and responsibility?* > > The CPG has the authority to propose a resolution to situations where there > are disagreements, that don’t involve large technical decisions. Their > decision proposed should be accepted as final since members will have > discussed the best steps and come to a consensus vote. > > The CPG does not aim to replace the committers' authority or the advisory > board but instead holds the authority to make decisions that are in the > best interest of the community in relation to processes. Committers still > hold the power should there be a formal escalation regarding technical > decisions, but this would be extremely rare. Advisory Board members hold > the final power regarding project and business-wide decisions. Nevertheless it doesn't become clear to me how adding yet another authority besides the committers will actually help. > *How are members selected?* > The CPG will be composed of 5 randomly selected members in total. > An odd number has been purposely selected to avoid an impasse during > decisions. > > The criteria: > Individual members must be active contributors and are willing to help the > community succeed. As such they must be a part of the following groups: > >- Committers >- Active Maintainers: maintainers with >= 20 reviews in the last 2 >releases >- Active Contributors: contributors with >= 10 commits in the last 2 >releases I'm afraid I can't leave this uncommented, as matching a common pattern I'm generally unhappy with. Whatever the numbers you select in such criteria, they'll open up an easy road for faking. At the same time it of course is difficult to come up with any non-numeric or not-only- numeric criteria. For example, I'd be heavily inclined to ask that "non-trivial" be added to both of the numbers. Yet then there arises a judgement issue: What's non-trivial can be entirely different depending on who you ask. What definitely needs clarifying is what "review" is: Are R-b tags counted, or is it the number of replies sent commenting on patches? > Future rotations of CPG members: > New members will be selected randomly for each new release to ensure > fairness. > > *Expectations* > CPG members are expected to use their best judgement of what is best for > the community in terms of conflict resolution and process improvements. > They can propose an outcome that progresses the project forward. > The CPG is also expected to address wider concerns, feedback, and ideas > during a monthly meeting with all
Re: Community Process Group - Proposal
On 19.01.2024 17:37, Kelly Choi wrote: > On Thu, Jan 18, 2024 at 10:09 AM Yann Dirson wrote: >> On 1/17/24 18:10, Kelly Choi wrote: >>> A survey was recently conducted to identify how the community as a whole >>> feels about a certain situation. It was not intended to ban specific >>> wording or create a policy to do so, but more to give context that the >>> community has a wide range of ideas, and individuals may agree and >>> disagree a lot more frequently than we as individuals might think. It >>> helps us understand that as a community there are many situations where >>> it is not clear. As such, the results indicated a very even split among >>> the community, which indicates a larger problem as we may not always >>> come to agreement. >>> >>> There is obvious frustration with how certain matters are handled, as >>> some members may want the project to move faster, whereas others like to >>> take a cautious approach. Given we are an open source project, >>> differences in opinion are likely to happen and what we don’t want to do >>> is cause further frustration. >>> >>> *This is where I would like to propose the idea of a ‘Community Process >>> Group’.* >> >> That made me look for a list of official roles in the project, which I >> found at [0]. How up-to-date is this list? The Release Manager role is >> mentioned there but not described, the Community Manager role is not >> mentioned at all, and the only link to get project leadership info [1] >> redirects to unrelated information. >> >> [0] https://xenproject.org/developers/governance/#roles-local >> [1] https://xenproject.org/developers/teams/hypervisor >> >> I feel it would be necessary to have a clear view on the current >> situation, before adding more structures. >> > > Aspects of the information on the website are outdated and do require > reviewing as part of a wider governance update. > However, the majority of the information such as the roles of committers is > still accurate. In this specific instance, the CPG would act fairly similar > to a project lead in terms of progressing the project forward. Rather than > it being one person, it will be a collective group of elected members. From > my understanding, we haven't had a project lead for a very long time within > the project and this was before the governance was formalized. If the > community is happy, we can replace the project lead role with the CPG. It was my understanding that with the departure of Keir, it was intentional that the "project lead" became a team (the committers) rather than again a (then largely randomly selected) individual. If that understanding of mine matches that of others, I don't think there's a need to change anything. Jan
Re: [v2] x86/xen: Add some null pointer checking to smp.c
>>> How do you think about to use another label like “e_nomem”? >> I'll add a new label to simply the code. > > I'm not a Xen maintainer so I can't really comment on their style choices. Linux contributors can discuss various implementation details. > However, as one of the kernel-janitors list people, I would > say that not everyone agrees with Markus's style preferences. Can a corresponding document be improved accordingly? Centralized exiting of functions https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.8-rc1#n526 Do you find a related information source helpful? https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources Regards, Markus
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
On 19.01.2024 16:25, Anthony PERARD wrote: > On Fri, Jan 19, 2024 at 09:43:30AM +0100, Michal Orzel wrote: >> Is my understanding correct that by switching from extra-y to targets we are >> preventing these objects to >> appear in non-init-objects (and thus having COV_FLAGS appended) while >> retaining the proper if_changed behavior? >> >> According to docs/misc/xen-makefiles/makefiles.rst: >> Any target that utilises if_changed must be listed in $(targets), >> otherwise the command line check will fail, and the target will >> always be built. > > Indeed, and $(extra-y) is added to $(targets) via > $(targets-for-builtin). > > While switching from $(extra-y) to $(targets) prevents the objects from > been added to $(non-init-objets), it doesn't matter because "libelf.o" > is in that variable, so $(COV_FLAGS) is added to $(_c_flags) and its > value is used in all the prerequisites of "libelf.o" which includes > "libelf-temp.o" and for example "libelf-dominfo.o". So the only thing > preventing $(COV_FLAGS) from been added when building "libelf-tools.o" > for example is that we set `COV_FLAGS:=` for "libelf.o". Yet doesn't that (again) mean things should actually work as-is, yet Michal is observing this not being the case? Jan
Re: [v3] x86/xen: Add some null pointer checking to smp.c
>> How do you think about to refer to the function name >> instead of the file name in the patch subject? >> > The main goal is to assign a errno to rc. So use 'fail_mem is good to > understand. You responded with information which can fit to the patch body. How do you think about consequences for a subject variant like the following? x86/xen: Add some null pointer checks in xen_smp_intr_init() Regards, Markus
[xen-unstable test] 184419: tolerable FAIL
flight 184419 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/184419/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-armhf-armhf-xl-credit1 8 xen-boot fail pass in 184415 Tests which did not succeed, but are not blocking: test-armhf-armhf-xl-credit1 15 migrate-support-check fail in 184415 never pass test-armhf-armhf-xl-credit1 16 saverestore-support-check fail in 184415 never pass test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 184415 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 184415 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 184415 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 184415 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 184415 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 184415 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 184415 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 184415 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 184415 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 184415 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 184415 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 184415 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-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-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-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 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass version targeted for testing: xen b25607e528f6ce7851e907ed59ad5ff583aa1840 baseline version: xen b25607e528f6ce7851e907ed59ad5ff583aa1840 Last test of basis 184419 2024-01-22 01:53:59 Z0 days Testing same since (not found) 0 attempts jobs:
Re: [PATCH v2] x86/xen: Add some null pointer checking to smp.c
On 2024/1/22 16:30, Dan Carpenter wrote:
On Fri, Jan 19, 2024 at 05:22:25PM +0800, Kunwu Chan wrote:
On 2024/1/17 18:40, Markus Elfring wrote:
kasprintf() returns a pointer to dynamically allocated memory
which can be NULL upon failure. Ensure the allocation was successful
by checking the pointer validity.
…
+++ b/arch/x86/xen/smp.c
@@ -61,10 +61,14 @@ void xen_smp_intr_free(unsigned int cpu)
int xen_smp_intr_init(unsigned int cpu)
{
- int rc;
+ int rc = 0;
I find the indication of a successful function execution sufficient by
the statement “return 0;” at the end.
How do you think about to omit such an extra variable initialisation?
Thanks, it's no need now. I'll remove it in v3.
This advice is good. Don't do unnecessary assignments.
Thanks for your suggestions, I'll keep it in mind.
char *resched_name, *callfunc_name, *debug_name;
resched_name = kasprintf(GFP_KERNEL, "resched%d", cpu);
+ if (!resched_name) {
+ rc = -ENOMEM;
+ goto fail;
+ }
per_cpu(xen_resched_irq, cpu).name = resched_name;
rc = bind_ipi_to_irqhandler(XEN_RESCHEDULE_VECTOR,
cpu,
You propose to apply the same error code in four if branches.
I suggest to avoid the specification of duplicate assignment statements
for this purpose.
How do you think about to use another label like “e_nomem”?
I'll add a new label to simply the code.
I'm not a Xen maintainer so I can't really comment on their style
choices. However, as one of the kernel-janitors list people, I would
say that not everyone agrees with Markus's style preferences. Markus
was banned from the list for a while, but we unbanned everyone when we
transitioned to the new list infrastructure. Do a search on lore to
find out more. https://lore.kernel.org/all/?q=Elfring
Perhaps wait for feedback from the maintainers for how to proceed?
OK, I'll wait for the feedback.
regards,
dan carpenter
--
Thanks,
Kunwu
Re: [PATCH v2] x86/xen: Add some null pointer checking to smp.c
On Fri, Jan 19, 2024 at 05:22:25PM +0800, Kunwu Chan wrote:
> On 2024/1/17 18:40, Markus Elfring wrote:
> > > kasprintf() returns a pointer to dynamically allocated memory
> > > which can be NULL upon failure. Ensure the allocation was successful
> > > by checking the pointer validity.
> > …
> > > +++ b/arch/x86/xen/smp.c
> > > @@ -61,10 +61,14 @@ void xen_smp_intr_free(unsigned int cpu)
> > >
> > > int xen_smp_intr_init(unsigned int cpu)
> > > {
> > > - int rc;
> > > + int rc = 0;
> >
> > I find the indication of a successful function execution sufficient by
> > the statement “return 0;” at the end.
> > How do you think about to omit such an extra variable initialisation?
> Thanks, it's no need now. I'll remove it in v3.
This advice is good. Don't do unnecessary assignments.
> >
> >
> > > char *resched_name, *callfunc_name, *debug_name;
> > >
> > > resched_name = kasprintf(GFP_KERNEL, "resched%d", cpu);
> > > + if (!resched_name) {
> > > + rc = -ENOMEM;
> > > + goto fail;
> > > + }
> > > per_cpu(xen_resched_irq, cpu).name = resched_name;
> > > rc = bind_ipi_to_irqhandler(XEN_RESCHEDULE_VECTOR,
> > > cpu,
> >
> > You propose to apply the same error code in four if branches.
> > I suggest to avoid the specification of duplicate assignment statements
> > for this purpose.
> > How do you think about to use another label like “e_nomem”?
> I'll add a new label to simply the code.
I'm not a Xen maintainer so I can't really comment on their style
choices. However, as one of the kernel-janitors list people, I would
say that not everyone agrees with Markus's style preferences. Markus
was banned from the list for a while, but we unbanned everyone when we
transitioned to the new list infrastructure. Do a search on lore to
find out more. https://lore.kernel.org/all/?q=Elfring
Perhaps wait for feedback from the maintainers for how to proceed?
regards,
dan carpenter
