Re: [RFC PATCH 04/10] xen: add reference counter support
On 17.02.2023 02:56, Volodymyr Babchuk wrote:
> Jan Beulich writes:
>> On 31.08.2022 16:10, Volodymyr Babchuk wrote:
>>> --- /dev/null
>>> +++ b/xen/include/xen/refcnt.h
>>> @@ -0,0 +1,28 @@
>>> +#ifndef __XEN_REFCNT_H__
>>> +#define __XEN_REFCNT_H__
>>> +
>>> +#include
>>> +
>>> +typedef atomic_t refcnt_t;
>>
>> Like Linux has it, I think this would better be a separate struct. At
>> least in debug builds, i.e. it could certainly use typesafe.h if that
>> ended up to be a good fit (which I'm not sure it would, so this is
>> merely a thought).
>
> Sadly, TYPE_SAFE does not support pointers. e.g I can't get pointer to
> an encapsulated value which is also passed as a pointer. I can expand
> TYPE_SAFE with $FOO_x_ptr():
>
> static inline _type *_name##_x_ptr(_name##_t *n) { &return n->_name; }
>
> or make custom encapsulation in refcnt.h. Which one you prefer?
First of all, as said - typesafe.h may not be a good fit. And then the
helper you suggest looks to be UB if the passed in pointer was to an
array rather than a singular object, so having something like that in
a very generic piece of infrastructure is inappropriate anyway.
>>> +static inline void refcnt_init(refcnt_t *refcnt)
>>> +{
>>> + atomic_set(refcnt, 1);
>>> +}
>>> +
>>> +static inline void refcnt_get(refcnt_t *refcnt)
>>> +{
>>> +#ifndef NDEBUG
>>> + ASSERT(atomic_add_unless(refcnt, 1, 0) > 0);
>>> +#else
>>> + atomic_add_unless(refcnt, 1, 0);
>>> +#endif
>>> +}
>
>> I think this wants doing without any #ifdef-ary, e.g.
>>
>> static inline void refcnt_get(refcnt_t *refcnt)
>> {
>> int ret = atomic_add_unless(refcnt, 1, 0);
>>
>> ASSERT(ret > 0);
>> }
>>
>
> Thanks, did as you suggested. I was afraid that compiler would complain
> about unused ret in non-debug builds.
>
>> I wonder though whether certain callers may not want to instead know
>> whether a refcount was successfully obtained, i.e. whether instead of
>> asserting here you don't want to return a boolean success indicator,
>> which callers then would deal with (either by asserting or by suitably
>> handling the case). See get_page() and page_get_owner_and_reference()
>> for similar behavior we have (and use) already.
>
> For now there are no such callers, so I don't want to implement unused
> functionality. But, if you prefer this way, I'll do this.
Well, I can see your point about unused functionality. That needs to be
weighed against this being a pretty basic piece of infrastructure, which
may want using elsewhere as well. Such re-use would then better not
trigger touching all the code which already uses it (in principle the
domain ref counting might be able to re-use it, for example, but there's
that DOMAIN_DESTROYED special case which may require it to continue to
have a custom implementation).
What you may want to do is check Linux'es equivalent. Depending on how
close ours is going to be, using the same naming may also want considering.
Jan
Re: [RFC PATCH 03/10] xen: pci: introduce ats_list_lock
On 17.02.2023 02:20, Volodymyr Babchuk wrote:
> Jan Beulich writes:
>> On 27.01.2023 00:56, Stefano Stabellini wrote:
>>> On Wed, 31 Aug 2022, Volodymyr Babchuk wrote:
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1641,6 +1641,7 @@ void iommu_dev_iotlb_flush_timeout(struct domain *d,
struct pci_dev *pdev)
{
pcidevs_lock();
+/* iommu->ats_list_lock is taken by the caller of this function */
>>>
>>> This is a locking inversion. In all other places we take pcidevs_lock
>>> first, then ats_list_lock lock. For instance look at
>>> xen/drivers/passthrough/pci.c:deassign_device that is called with
>>> pcidevs_locked and then calls iommu_call(... reassign_device ...) which
>>> ends up taking ats_list_lock.
>>>
>>> This is the only exception. I think we need to move the
>>> spin_lock(ats_list_lock) from qinval.c to here.
>>
>> First question here is what the lock is meant to protect: Just the list,
>> or also the ATS state change (i.e. serializing enable and disable against
>> one another). In the latter case the lock also wants naming differently.
>
> My intention was to protect list only. But I believe you are right and
> we should protect the whole state. I'll rename the lock to ats_lock.
>
>> Second question is who is to acquire the lock. Why isn't this done _in_
>> {en,dis}able_ats_device() themselves? That would then allow to further
>> reduce the locked range, because at least the pci_find_ext_capability()
>> call and the final logging can occur without the lock held.
>
> You are right, I'll extended {en,dis}able_ats_device() API to pass
> pointer to the lock.
Hmm, that'll make for an odd interface. I was wondering in the past
already why we don't have a backref from the PCI dev to its controlling
IOMMU (might be ambiguous and hence better left unset for bridges,
especially host ones, but I think ATS is being fiddled with only for
leaf devices; would need double checking of course).
Jan
Re: [PATCH] xen: Remove the use of K&R functions
On 17.02.2023 00:17, Andrew Cooper wrote:
> On 16/02/2023 11:02 pm, Andrew Cooper wrote:
>> On 16/02/2023 10:44 pm, Andrew Cooper wrote:
>>> Clang-15 (as seen in the FreeBSD 14 tests) complains:
>>>
>>> arch/x86/time.c:1364:20: error: a function declaration without a
>>> prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
>>> s_time_t get_s_time()
>>> ^
>>> void
>>>
>>> The error message is a bit confusing but appears to new as part of
>>> -Wdeprecated-non-prototype which is part of supporting C2x which formally
>>> removes K&R syntax.
>>>
>>> Either way, fix the offending functions.
>>>
>>> Signed-off-by: Andrew Cooper
>>> ---
>>> CC: Jan Beulich
>>> CC: Roger Pau Monné
>>> CC: Wei Liu
>>>
>>> These are all the examples found in a default build of Xen. I'm still
>>> finding
>>> toolstack violations.
>> Apparently not. int cf_check vmx_cpu_up() too.
>
> Ok, finally got a clean Clang-15 build. I've folded this hunk into the
> patch:
>
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 09edbd23b399..e1c268789e7e 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -781,7 +781,7 @@ static int _vmx_cpu_up(bool bsp)
> return 0;
> }
>
> -int cf_check vmx_cpu_up()
> +int cf_check vmx_cpu_up(void)
> {
> return _vmx_cpu_up(false);
> }
>
>
> but am not intending to send a v2 given how trivial it is.
Sure.
Reviewed-by: Jan Beulich
Jan
Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
On 17.02.2023 02:44, Stefano Stabellini wrote: > On Thu, 16 Feb 2023, Luca Fancellu wrote: >>> On 16 Feb 2023, at 08:19, Jan Beulich wrote: >>> On 16.02.2023 00:49, Stefano Stabellini wrote: On Wed, 15 Feb 2023, Julien Grall wrote: > On 14/02/2023 22:25, Stefano Stabellini wrote: >>> Patch 1's example has a "comment" field, which no entry makes use of. >>> Without that, how does it become clear _why_ a particular file is to >>> be excluded? The patch description here also doesn't provide any >>> justification ... >> >> It would be good to have a couple of pre-canned justifications as the >> reason for excluding one file could be different from the reason for >> excluding another file. Some of the reasons: > > I think the reasons should be ambiguous. This is ... > >> - imported from Linux > > ... the case here but... > > This reason is pretty clear to me but... > >> - too many false positives > > ... not here. What is too many? > >> That said, we don't necessarily need to know the exact reason for >> excluding one file to be able to start scanning xen with cppcheck >> automatically. If someone wants to remove a file from the exclude list >> in the future they just need to show that cppcheck does a good job at >> scanning the file and we can handle the number of violations. > > I disagree. A good reasoning from the start will be helpful to decide > when we > can remove a file from the list. Furthermore, we need to set good example > for > any new file we want to exclude. > > Furthermore, if we exclude some files, then it will be difficult for the > reviewers to know when they can be removed from the list. What if this is > fine > with CPPCheck but not EClair (or any other)? Yes, the reason would help. In previous incarnations of this work, there was a request for detailed information on external files, such as: - where this file is coming from - if coming from Linux, which version of Linux - maintenance status - coding style But this is not what you are asking. You are only asking for a reason and "imported from Linux" would be good enough. Please correct me if I am wrong. >>> >>> I guess you mean s/would/could/. Personally I find "imported from Linux" >>> as an entirely unacceptable justification: Why would the origin of a file >>> matter on whether it has violations? Dealing with the violations may be >>> more cumbersome (because preferably the adjustments would go to the >>> original files first). Yet not dealing with them - especially if there >>> are many - reduces the benefit of the work we do quite a bit, because it >>> may leave much more work for downstreams to do to actually be able to do >>> any certification. That may go to the extent of questioning why we would >>> bother dealing with a few dozen violations if hundreds remain but are >>> hidden. > > Yes, we need to figure out a way to deal with these files. However, at > the moment they are getting in the way of easier low hanging fruits. > > One "easy" low hanging fruit is to use cppcheck to scan new patches for > new MISRA violations. That would give immediate benefits to the > community. It is not easy to "diff" results with cppcheck and in any > case it would help a lot if we had a cleaner baseline because it would > make it far easier to read the results and make sense of them. > > The goal of this exclusion list is to give us that: a cleaner baseline > so that we can make progress faster on low hanging fruits. This list is > not meant to be fixed and stay unchanged for a long time. In fact, it > could even live under automation/ as part of the gitlab-ci test that > triggers cppcheck, if we prefer. Okay, that sounds reasonable to me as an intermediate goal. But then description of the change and justifications should also state so imo (for the latter e.g. "from Linux, ignore for now"). Jan
Re: [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
On 16.02.2023 21:44, Oleksii wrote:
> On Thu, 2023-02-16 at 12:19 +, Andrew Cooper wrote:
>> On 16/02/2023 12:09 pm, Oleksii wrote:
>>> On Thu, 2023-02-16 at 12:44 +0200, Oleksii wrote:
On Thu, 2023-02-16 at 08:31 +0100, Jan Beulich wrote:
> On 15.02.2023 18:59, Oleksii wrote:
>> Hello Jan and community,
>>
>> I experimented and switched RISC-V to x86 implementation. All
>> that
>> I
>> changed in x86 implementation for RISC-V was
>> _ASM_BUGFRAME_TEXT.
>> Other
>> things are the same as for x86.
>>
>> For RISC-V it is fine to skip '%c' modifier so
>> _ASM_BUGFRAME_TEXT
>> will
>> look like:
>>
>> #define _ASM_BUGFRAME_TEXT(second_frame) \
>> ".Lbug%=: ebreak\n"
>> ".pushsection .bug_frames.%[bf_type], \"a\", @progbits\n"
>> ".p2align 2\n"
>> ".Lfrm%=:\n"
>> ".long (.Lbug%= - .Lfrm%=) + %[bf_line_hi]\n"
>> ".long (%[bf_ptr] - .Lfrm%=) + %[bf_line_lo]\n"
>> ".if " #second_frame "\n"
>> ".long 0, %[bf_msg] - .Lfrm%=\n"
>> ".endif\n"
>> ".popsection\n"
> I expect this could be further abstracted such that only the
> actual
> instruction is arch-specific.
Generally, the only thing that can't be abstracted ( as you
mentioned
is an instruction ).
So we can make additional defines:
1. #define MODIFIER "" or "c" (depends on architecture) and use
it
the following way:
".pushsection .bug_frames.%"MODIFIER"[bf_type], \"a\",
@progbits\n"
...
2. #define BUG_INSTR "ebreak" | "ud2" | "..." and use it in the
following way:
".Lbug%=: "BUG_ISNTR"\n"
...
Except for these defines which should be specified for each
architecture
all other code will be the same for all architectures.
But as I understand with modifier 'c' can be issues that not all
compilers support but for ARM and x86 before immediate is
present
punctuation # or $ which are removed by modifier 'c'. And I am
wondering what other ways to remove punctuation before immediate
exist.
Do you think we should consider that as an issue?
>> The only thing I am worried about is:
>>
>> #define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \
>> [bf_type] "i" (type), ...
>> because as I understand it can be an issue with 'i' modifier
>> in
>> case of
>> PIE. I am not sure that Xen enables PIE somewhere but
>> still...
>> If it is not an issue then we can use x86 implementation as a
>> generic
>> one.
> "i" is not generally an issue with PIE, it only is when the
> value
> is
> the
> address of a symbol. Here "type" is a constant in all cases.
> (Or
> else
> how would you express an immediate operand of an instruction in
> an
> asm()?)
Hmm. I don't know other ways to express an immediate operand
except
'i'.
It looks like type, line, msg are always 'constants'
(
they possibly can be passed without 'i' and used directly inside
asm():
...
".pushsection .bug_frames." __stringify(type) ", \"a\",
%progbits\n"\
...
) but the issue will be with 'ptr' for which we have to use 'i'.
And I am not sure about all 'constants'. For example, I think
that it
can be an issue with 'line' = '((line & ((1 << BUG_LINE_LO_WIDTH)
-
1))
<< BUG_DISP_WIDTH)' if we will use that directly inside asm(...).
>>> I think we can avoid 'i' by using 'r' + some kind of mov
>>> instruction to
>>> write a register value to memory. The code below is pseudo-code:
>>> #define _ASM_BUGFRAME_TEXT(second_frame)
>>> ...
>>> "loc_disp:\n"
>>> " .long 0x0"
>>> "mov [loc_disp], some_register"
>>> ...
>>> And the we have to define mov for each architecture.
>>
>> No, you really cannot. This is the asm equivalent of writing
>> something
>> like this:
>>
>> static struct bugframe __section(.bug_frames.1) foo = {
>> .loc = insn - &foo + LINE_LO,
>> .msg = "bug string" - &foo + LINE_HI,
>> };
>>
>> It is a data structure, not executable code.
> Thanks for the clarification.
>
> What about mainly using C for BUG_FRAME and only allocating bug_frame
> sections in assembly?
>
> Please look at POC below:
That's still using statements (assignments), not initializers. We expect
the table to be populated at build time, and we expect it to be read-only
at runtime. Plus your approach once again increases overall size (just
that this time you add code, not data).
Jan
> #include
> #include
>
> #define BUG_DISP_WIDTH24
> #define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> #define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
>
> struct bug_frame {
> signed int loc_disp:BUG_DISP_WIDTH;
> unsigned int line_hi:BUG_LINE_HI_WIDTH;
> signed int ptr_disp:BUG_DISP_WIDTH;
> un
[linux-linus test] 177531: tolerable trouble: fail/pass/starved - PUSHED
flight 177531 linux-linus real [real] flight 177568 linux-linus real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/177531/ http://logs.test-lab.xenproject.org/osstest/logs/177568/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-amd64-freebsd12-amd64 21 guest-start/freebsd.repeat fail pass in 177568-retest test-amd64-amd64-xl-qemut-debianhvm-i386-xsm 20 guest-start/debianhvm.repeat fail pass in 177568-retest Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 177451 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 177451 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 177451 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 177451 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 177451 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-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-examine 1 build-check(1) starved n/a test-armhf-armhf-libvirt 1 build-check(1) starved n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) starved n/a test-armhf-armhf-libvirt-raw 1 build-check(1) starved n/a test-armhf-armhf-xl 1 build-check(1) starved n/a test-armhf-armhf-xl-credit1 1 build-check(1) starved n/a test-armhf-armhf-xl-credit2 1 build-check(1) starved n/a test-armhf-armhf-xl-cubietruck 1 build-check(1) starved n/a test-armhf-armhf-xl-multivcpu 1 build-check(1) starved n/a test-armhf-armhf-xl-rtds 1 build-check(1) starved n/a test-armhf-armhf-xl-vhd 1 build-check(1) starved n/a build-armhf-libvirt 1 build-check(1) starved n/a build-armhf 2 hosts-allocate starved n/a version targeted for testing: linux3ac88fa4605ec98e545fb3ad0154f575fda2de5f baseline version: linux033c40a89f55525139fd5b6342281b09b97d05bf Last test of basis 177451 2023-02-16 02:59:06 Z1 days Testing same since 177531 2023-02-16 20:42:54 Z0 days1 attempts People who touched revisions under test: Alexander Stein Andy Shevchenko Andy Shevchenko Bartosz Golaszewski Chandan Kumar Rout (A Contingent Worker at Intel) Christoph Hellwig Corinna Vinschen Cristian Ciocaltea Daniel Wagner David S. Miller Eelco Chaudron Eric Dumazet Felix Riemann Guillaume Nault Guolin Yang Gurucharan G (A Contingent worker at Intel) Hangyu Hua Hector Martin Herbert Xu Hyunwoo Kim Ido Schimmel Irvin Cote Jakub Kicinski Jamal Hadi Salim Jamie Bainbridge Janne Grunau Jason Xing Jens Axboe Jesse Brandeburg Johannes Zink Jon Maloy Kuniyuki Iwashima Larysa Zaremba Linus Torvalds Linus Walleij Maciej Fijalkowski Mark Brown Mateusz Palczewski Michael Chan Michael Kelley Michal Wilczynski Miko Larsson Miroslav Lichvar Natalia Petrova Paolo Abeni Pedro Tammela Peng Li Pietro Borrello Raag Jadav Rafal Romanowski Rafał Miłecki Ronak Dosh
Re: [RFC PATCH 04/10] xen: add reference counter support
Hello Jan,
Jan Beulich writes:
> On 31.08.2022 16:10, Volodymyr Babchuk wrote:
>> --- /dev/null
>> +++ b/xen/include/xen/refcnt.h
>> @@ -0,0 +1,28 @@
>> +#ifndef __XEN_REFCNT_H__
>> +#define __XEN_REFCNT_H__
>> +
>> +#include
>> +
>> +typedef atomic_t refcnt_t;
>
> Like Linux has it, I think this would better be a separate struct. At
> least in debug builds, i.e. it could certainly use typesafe.h if that
> ended up to be a good fit (which I'm not sure it would, so this is
> merely a thought).
Sadly, TYPE_SAFE does not support pointers. e.g I can't get pointer to
an encapsulated value which is also passed as a pointer. I can expand
TYPE_SAFE with $FOO_x_ptr():
static inline _type *_name##_x_ptr(_name##_t *n) { &return n->_name; }
or make custom encapsulation in refcnt.h. Which one you prefer?
>> +static inline void refcnt_init(refcnt_t *refcnt)
>> +{
>> +atomic_set(refcnt, 1);
>> +}
>> +
>> +static inline void refcnt_get(refcnt_t *refcnt)
>> +{
>> +#ifndef NDEBUG
>> +ASSERT(atomic_add_unless(refcnt, 1, 0) > 0);
>> +#else
>> +atomic_add_unless(refcnt, 1, 0);
>> +#endif
>> +}
> I think this wants doing without any #ifdef-ary, e.g.
>
> static inline void refcnt_get(refcnt_t *refcnt)
> {
> int ret = atomic_add_unless(refcnt, 1, 0);
>
> ASSERT(ret > 0);
> }
>
Thanks, did as you suggested. I was afraid that compiler would complain
about unused ret in non-debug builds.
> I wonder though whether certain callers may not want to instead know
> whether a refcount was successfully obtained, i.e. whether instead of
> asserting here you don't want to return a boolean success indicator,
> which callers then would deal with (either by asserting or by suitably
> handling the case). See get_page() and page_get_owner_and_reference()
> for similar behavior we have (and use) already.
For now there are no such callers, so I don't want to implement unused
functionality. But, if you prefer this way, I'll do this.
[...]
Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
On Thu, 16 Feb 2023, Luca Fancellu wrote: > > On 16 Feb 2023, at 08:19, Jan Beulich wrote: > > On 16.02.2023 00:49, Stefano Stabellini wrote: > >> On Wed, 15 Feb 2023, Julien Grall wrote: > >>> On 14/02/2023 22:25, Stefano Stabellini wrote: > > Patch 1's example has a "comment" field, which no entry makes use of. > > Without that, how does it become clear _why_ a particular file is to > > be excluded? The patch description here also doesn't provide any > > justification ... > > It would be good to have a couple of pre-canned justifications as the > reason for excluding one file could be different from the reason for > excluding another file. Some of the reasons: > >>> > >>> I think the reasons should be ambiguous. This is ... > >>> > - imported from Linux > >>> > >>> ... the case here but... > >>> > >>> This reason is pretty clear to me but... > >>> > - too many false positives > >>> > >>> ... not here. What is too many? > >>> > That said, we don't necessarily need to know the exact reason for > excluding one file to be able to start scanning xen with cppcheck > automatically. If someone wants to remove a file from the exclude list > in the future they just need to show that cppcheck does a good job at > scanning the file and we can handle the number of violations. > >>> > >>> I disagree. A good reasoning from the start will be helpful to decide > >>> when we > >>> can remove a file from the list. Furthermore, we need to set good example > >>> for > >>> any new file we want to exclude. > >>> > >>> Furthermore, if we exclude some files, then it will be difficult for the > >>> reviewers to know when they can be removed from the list. What if this is > >>> fine > >>> with CPPCheck but not EClair (or any other)? > >> > >> Yes, the reason would help. In previous incarnations of this work, there > >> was a request for detailed information on external files, such as: > >> - where this file is coming from > >> - if coming from Linux, which version of Linux > >> - maintenance status > >> - coding style > >> > >> But this is not what you are asking. You are only asking for a reason > >> and "imported from Linux" would be good enough. Please correct me if I > >> am wrong. > > > > I guess you mean s/would/could/. Personally I find "imported from Linux" > > as an entirely unacceptable justification: Why would the origin of a file > > matter on whether it has violations? Dealing with the violations may be > > more cumbersome (because preferably the adjustments would go to the > > original files first). Yet not dealing with them - especially if there > > are many - reduces the benefit of the work we do quite a bit, because it > > may leave much more work for downstreams to do to actually be able to do > > any certification. That may go to the extent of questioning why we would > > bother dealing with a few dozen violations if hundreds remain but are > > hidden. Yes, we need to figure out a way to deal with these files. However, at the moment they are getting in the way of easier low hanging fruits. One "easy" low hanging fruit is to use cppcheck to scan new patches for new MISRA violations. That would give immediate benefits to the community. It is not easy to "diff" results with cppcheck and in any case it would help a lot if we had a cleaner baseline because it would make it far easier to read the results and make sense of them. The goal of this exclusion list is to give us that: a cleaner baseline so that we can make progress faster on low hanging fruits. This list is not meant to be fixed and stay unchanged for a long time. In fact, it could even live under automation/ as part of the gitlab-ci test that triggers cppcheck, if we prefer. > Hi Jan, > > my personal opinion is that we can’t handle them for files that needs to be > kept > in sync from an external source, because we can’t justify findings or tag > false > positives, if we are lucky we might fix the non compliances but even in that > case > there might be times when the fix goes through and cases where the fix is > objected > by the maintainers just because their project is not following the MISRA > standard. > > On our side, what we can do is track these files and from time to time check > that > they are still eligible to backport, because when they are not anymore we > could > just port them to Xen coding style and start doing direct changes. > > > @Stefano/@Bertrand/@Julien thanks for your effort on the list, yesterday > morning > I’ve also had a look on the Michal’s file list and I’ve tracked down the > origin, here > my findings, maybe we could merge your list with mine? Yes to merge the two lists, but I think we should keep only items that we actually need for the sake of having a cleaner baseline. So I would only add things we need today to reduce the noise on cppcheck results (excluding 20.7 and also excluding unusedStructMember which I think w
[xen-unstable-smoke test] 177547: tolerable trouble: pass/starved - PUSHED
flight 177547 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/177547/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl 1 build-check(1) starved n/a build-armhf 2 hosts-allocate starved n/a version targeted for testing: xen f5c1a6374aae8be471b895a43359dcff355577f5 baseline version: xen 720944ea26014a1830f9b44bda13b79e8e7d753b Last test of basis 177537 2023-02-16 22:02:03 Z0 days Testing same since 177547 2023-02-17 00:01:55 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Anthony PERARD jobs: build-arm64-xsm pass build-amd64 pass build-armhf starved build-amd64-libvirt pass test-armhf-armhf-xl starved test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 720944ea26..f5c1a6374a f5c1a6374aae8be471b895a43359dcff355577f5 -> smoke
Re: [RFC PATCH 03/10] xen: pci: introduce ats_list_lock
Hello Jan,
Jan Beulich writes:
> On 27.01.2023 00:56, Stefano Stabellini wrote:
>> On Wed, 31 Aug 2022, Volodymyr Babchuk wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -1641,6 +1641,7 @@ void iommu_dev_iotlb_flush_timeout(struct domain *d,
>>> struct pci_dev *pdev)
>>> {
>>> pcidevs_lock();
>>>
>>> +/* iommu->ats_list_lock is taken by the caller of this function */
>>
>> This is a locking inversion. In all other places we take pcidevs_lock
>> first, then ats_list_lock lock. For instance look at
>> xen/drivers/passthrough/pci.c:deassign_device that is called with
>> pcidevs_locked and then calls iommu_call(... reassign_device ...) which
>> ends up taking ats_list_lock.
>>
>> This is the only exception. I think we need to move the
>> spin_lock(ats_list_lock) from qinval.c to here.
>
> First question here is what the lock is meant to protect: Just the list,
> or also the ATS state change (i.e. serializing enable and disable against
> one another). In the latter case the lock also wants naming differently.
My intention was to protect list only. But I believe you are right and
we should protect the whole state. I'll rename the lock to ats_lock.
> Second question is who is to acquire the lock. Why isn't this done _in_
> {en,dis}able_ats_device() themselves? That would then allow to further
> reduce the locked range, because at least the pci_find_ext_capability()
> call and the final logging can occur without the lock held.
You are right, I'll extended {en,dis}able_ats_device() API to pass
pointer to the lock.
[PATCH RFC] xen: Work around Clang-IAS macro expansion bug.
https://github.com/llvm/llvm-project/issues/60792
RFC. I very much dislike this patch, but it does work for me.
Why the parameter name of foo? Turns out I found a different Clang-IAS
bug/misfeature when trying to name the parameter uniq.
In file included from arch/x86/asm-macros.c:3:
./arch/x86/include/asm/spec_ctrl_asm.h:139:5: error: \u used with no
following hex digits; treating as '\' followed by identifier [-Werror,-Wunicode]
.L\@\uniq\()fill_rsb_loop:
^
It appears you can't have any macro parameters beginning with a u.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Roger Pau Monné
CC: Wei Liu
---
xen/arch/x86/include/asm/spec_ctrl.h | 4 ++--
xen/arch/x86/include/asm/spec_ctrl_asm.h | 14 +++---
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/xen/arch/x86/include/asm/spec_ctrl.h
b/xen/arch/x86/include/asm/spec_ctrl.h
index 3cf8a7d304d4..cd727be7c689 100644
--- a/xen/arch/x86/include/asm/spec_ctrl.h
+++ b/xen/arch/x86/include/asm/spec_ctrl.h
@@ -83,7 +83,7 @@ static always_inline void spec_ctrl_new_guest_context(void)
wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
/* (ab)use alternative_input() to specify clobbers. */
-alternative_input("", "DO_OVERWRITE_RSB", X86_BUG_IBPB_NO_RET,
+alternative_input("", "DO_OVERWRITE_RSB foo=%=", X86_BUG_IBPB_NO_RET,
: "rax", "rcx");
}
@@ -172,7 +172,7 @@ static always_inline void spec_ctrl_enter_idle(struct
cpu_info *info)
*
* (ab)use alternative_input() to specify clobbers.
*/
-alternative_input("", "DO_OVERWRITE_RSB", X86_FEATURE_SC_RSB_IDLE,
+alternative_input("", "DO_OVERWRITE_RSB foo=%=", X86_FEATURE_SC_RSB_IDLE,
: "rax", "rcx");
}
diff --git a/xen/arch/x86/include/asm/spec_ctrl_asm.h
b/xen/arch/x86/include/asm/spec_ctrl_asm.h
index fab27ff5532b..06455c5663bb 100644
--- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
+++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
@@ -117,7 +117,7 @@
.L\@_done:
.endm
-.macro DO_OVERWRITE_RSB tmp=rax
+.macro DO_OVERWRITE_RSB tmp=rax foo=
/*
* Requires nothing
* Clobbers \tmp (%rax by default), %rcx
@@ -136,27 +136,27 @@
mov $16, %ecx /* 16 iterations, two calls per loop */
mov %rsp, %\tmp /* Store the current %rsp */
-.L\@_fill_rsb_loop:
+.L\@\foo\()fill_rsb_loop:
.irp n, 1, 2/* Unrolled twice. */
-call .L\@_insert_rsb_entry_\n /* Create an RSB entry. */
+call .L\@\foo\()insert_rsb_entry_\n /* Create an RSB entry. */
int3/* Halt rogue speculation. */
-.L\@_insert_rsb_entry_\n:
+.L\@\foo\()insert_rsb_entry_\n:
.endr
sub $1, %ecx
-jnz .L\@_fill_rsb_loop
+jnz .L\@\foo\()fill_rsb_loop
mov %\tmp, %rsp /* Restore old %rsp */
#ifdef CONFIG_XEN_SHSTK
mov $1, %ecx
rdsspd %ecx
cmp $1, %ecx
-je .L\@_shstk_done
+je .L\@\foo\()shstk_done
mov $64, %ecx /* 64 * 4 bytes, given incsspd */
incsspd %ecx/* Restore old SSP */
-.L\@_shstk_done:
+.L\@\foo\()shstk_done:
#endif
.endm
--
2.30.2
Re: [PATCH] xen: Remove the use of K&R functions
On 16/02/2023 11:02 pm, Andrew Cooper wrote:
> On 16/02/2023 10:44 pm, Andrew Cooper wrote:
>> Clang-15 (as seen in the FreeBSD 14 tests) complains:
>>
>> arch/x86/time.c:1364:20: error: a function declaration without a
>> prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
>> s_time_t get_s_time()
>> ^
>> void
>>
>> The error message is a bit confusing but appears to new as part of
>> -Wdeprecated-non-prototype which is part of supporting C2x which formally
>> removes K&R syntax.
>>
>> Either way, fix the offending functions.
>>
>> Signed-off-by: Andrew Cooper
>> ---
>> CC: Jan Beulich
>> CC: Roger Pau Monné
>> CC: Wei Liu
>>
>> These are all the examples found in a default build of Xen. I'm still
>> finding
>> toolstack violations.
> Apparently not. int cf_check vmx_cpu_up() too.
Ok, finally got a clean Clang-15 build. I've folded this hunk into the
patch:
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 09edbd23b399..e1c268789e7e 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -781,7 +781,7 @@ static int _vmx_cpu_up(bool bsp)
return 0;
}
-int cf_check vmx_cpu_up()
+int cf_check vmx_cpu_up(void)
{
return _vmx_cpu_up(false);
}
but am not intending to send a v2 given how trivial it is.
~Andrew
[PATCH] tools: Remove the use of K&R functions
Clang-15 (as seen in the FreeBSD 14 tests) complains:
xg_main.c:1248 error: a function declaration without a
prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
xg_init()
^
void
The error message is a bit confusing but appears to new as part of
-Wdeprecated-non-prototype which is part of supporting C2x which formally
removes K&R syntax.
Either way, fix the offending functions.
Signed-off-by: Andrew Cooper
---
CC: Anthony PERARD
CC: George Dunlap
CC: Jan Beulich
CC: Stefano Stabellini
CC: Wei Liu
CC: Julien Grall
---
tools/debugger/gdbsx/xg/xg_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/debugger/gdbsx/xg/xg_main.c
b/tools/debugger/gdbsx/xg/xg_main.c
index 4576c762af0c..580fe237b20e 100644
--- a/tools/debugger/gdbsx/xg/xg_main.c
+++ b/tools/debugger/gdbsx/xg/xg_main.c
@@ -121,7 +121,7 @@ xgprt(const char *fn, const char *fmt, ...)
* -1 failure, errno set.
*/
int
-xg_init()
+xg_init(void)
{
int flags, saved_errno;
--
2.30.2
[xen-unstable-smoke test] 177537: tolerable trouble: pass/starved - PUSHED
flight 177537 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/177537/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl 1 build-check(1) starved n/a build-armhf 2 hosts-allocate starved n/a version targeted for testing: xen 720944ea26014a1830f9b44bda13b79e8e7d753b baseline version: xen 2e52dcc853a7183784cd9bdfb1e78ff366035209 Last test of basis 177483 2023-02-16 10:00:26 Z0 days Testing same since 177537 2023-02-16 22:02:03 Z0 days1 attempts People who touched revisions under test: Henry Wang #Arm Julien Grall jobs: build-arm64-xsm pass build-amd64 pass build-armhf starved build-amd64-libvirt pass test-armhf-armhf-xl starved test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 2e52dcc853..720944ea26 720944ea26014a1830f9b44bda13b79e8e7d753b -> smoke
Re: [PATCH] xen: Remove the use of K&R functions
On 16/02/2023 10:44 pm, Andrew Cooper wrote: > Clang-15 (as seen in the FreeBSD 14 tests) complains: > > arch/x86/time.c:1364:20: error: a function declaration without a > prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes] > s_time_t get_s_time() > ^ > void > > The error message is a bit confusing but appears to new as part of > -Wdeprecated-non-prototype which is part of supporting C2x which formally > removes K&R syntax. > > Either way, fix the offending functions. > > Signed-off-by: Andrew Cooper > --- > CC: Jan Beulich > CC: Roger Pau Monné > CC: Wei Liu > > These are all the examples found in a default build of Xen. I'm still finding > toolstack violations. Apparently not. int cf_check vmx_cpu_up() too. ~Andrew
[PATCH] xen: Remove the use of K&R functions
Clang-15 (as seen in the FreeBSD 14 tests) complains:
arch/x86/time.c:1364:20: error: a function declaration without a
prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
s_time_t get_s_time()
^
void
The error message is a bit confusing but appears to new as part of
-Wdeprecated-non-prototype which is part of supporting C2x which formally
removes K&R syntax.
Either way, fix the offending functions.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Roger Pau Monné
CC: Wei Liu
These are all the examples found in a default build of Xen. I'm still finding
toolstack violations.
---
xen/arch/x86/time.c | 2 +-
xen/drivers/passthrough/iommu.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 782b11c8a97b..4e44a43cc5e8 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1361,7 +1361,7 @@ s_time_t get_s_time_fixed(u64 at_tsc)
return t->stamp.local_stime + scale_delta(delta, &t->tsc_scale);
}
-s_time_t get_s_time()
+s_time_t get_s_time(void)
{
return get_s_time_fixed(0);
}
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 921b71e81904..0e187f6ae33c 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -606,7 +606,7 @@ int __init iommu_setup(void)
return rc;
}
-int iommu_suspend()
+int iommu_suspend(void)
{
if ( iommu_enabled )
return iommu_call(iommu_get_ops(), suspend);
@@ -614,7 +614,7 @@ int iommu_suspend()
return 0;
}
-void iommu_resume()
+void iommu_resume(void)
{
if ( iommu_enabled )
iommu_vcall(iommu_get_ops(), resume);
--
2.30.2
Re: [XEN v6 2/2] xen/arm: domain_build: Use pfn start and end address for rangeset_{xxx}_range()
Hi Ayan,
The title is a bit strange to read as a 'pfn' is not an address. So how
about:
xen/arm: domain_build: Track unallocated pages using the frame number
On 13/02/2023 12:44, Ayan Kumar Halder wrote:
rangeset_{xxx}_range() functions are invoked with 'start' and 'size' as
arguments which are either 'uint64_t' or 'paddr_t'. However, the function
accepts 'unsigned long' for 'start' and 'size'. 'unsigned long' is 32 bits for
ARM_32. Thus, there is an implicit downcasting from 'uint64_t'/'paddr_t' to
'unsigned long' when invoking rangeset_{xxx}_range().
However, it may seem there is a possibility of lose of data due to truncation.
In reality, 'start' and 'size' are always page aligned. And ARM_32 currently
supports 40 bits as the width of physical address.
So if the addresses are page aligned, the last 12 bits contain zeroes.
Thus, we could instead pass page frame number which will contain 28 bits (40-12
on Arm_32) and this can be represented using 'unsigned long'.
On Arm_64, this change will not induce any adverse side effect as the width of
physical address is 48 bits. Thus, the width of 'pfn' (ie 48 - 12 = 36) can be
Technically, this will an MFN rather than PFN (Yes, I now the macro is
called PFN_DOWN() but this is a generic name).
represented using 'unsigned long' (which is 64 bits wide).
Signed-off-by: Ayan Kumar Halder
---
Changes from -
v1 - v5 - NA (New patch introduced in v6).
xen/arch/arm/domain_build.c | 22 +-
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index a798e0b256..6a8c7206ae 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1566,7 +1566,8 @@ static int __init find_unallocated_memory(const struct
kernel_info *kinfo,
{
start = bootinfo.mem.bank[i].start;
end = bootinfo.mem.bank[i].start + bootinfo.mem.bank[i].size;
-res = rangeset_add_range(unalloc_mem, start, end - 1);
+res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
+ PFN_DOWN(end - 1));
if ( res )
{
printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
@@ -1580,7 +1581,8 @@ static int __init find_unallocated_memory(const struct
kernel_info *kinfo,
{
start = assign_mem->bank[i].start;
end = assign_mem->bank[i].start + assign_mem->bank[i].size;
-res = rangeset_remove_range(unalloc_mem, start, end - 1);
+res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
+PFN_DOWN(end - 1));
if ( res )
{
printk(XENLOG_ERR "Failed to remove:
%#"PRIpaddr"->%#"PRIpaddr"\n",
@@ -1595,7 +1597,8 @@ static int __init find_unallocated_memory(const struct
kernel_info *kinfo,
start = bootinfo.reserved_mem.bank[i].start;
end = bootinfo.reserved_mem.bank[i].start +
bootinfo.reserved_mem.bank[i].size;
-res = rangeset_remove_range(unalloc_mem, start, end - 1);
+res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
+PFN_DOWN(end - 1));
if ( res )
{
printk(XENLOG_ERR "Failed to remove:
%#"PRIpaddr"->%#"PRIpaddr"\n",
@@ -1607,7 +1610,7 @@ static int __init find_unallocated_memory(const struct
kernel_info *kinfo,
/* Remove grant table region */
start = kinfo->gnttab_start;
end = kinfo->gnttab_start + kinfo->gnttab_size;
-res = rangeset_remove_range(unalloc_mem, start, end - 1);
+res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), PFN_DOWN(end -
1));
if ( res )
{
printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
@@ -1617,7 +1620,7 @@ static int __init find_unallocated_memory(const struct
kernel_info *kinfo,
start = 0;
end = (1ULL << p2m_ipa_bits) - 1;
-res = rangeset_report_ranges(unalloc_mem, start, end,
+res = rangeset_report_ranges(unalloc_mem, PFN_DOWN(start), PFN_DOWN(end),
add_ext_regions, ext_regions);
I believe you also need to modify add_ext_regions() because the existing
code expect an address and we will now provide a frame number.
if ( res )
ext_regions->nr_banks = 0;
@@ -1639,7 +1642,7 @@ static int __init handle_pci_range(const struct
dt_device_node *dev,
start = addr & PAGE_MASK;
end = PAGE_ALIGN(addr + len);
-res = rangeset_remove_range(mem_holes, start, end - 1);
+res = rangeset_remove_range(mem_holes, PFN_DOWN(start),PFN_DOWN(end - 1));
Coding style: missing space after ","
if ( res )
{
printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
@@ -1677,7 +1680,7 @@ static int __init find_memory_holes(const struct
kernel_info *kinfo,
/* Start with maximum possible addressable physical memory range */
start = 0;
[xen-unstable test] 177485: regressions - trouble: broken/fail/pass/starved
flight 177485 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/177485/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-livepatch broken test-amd64-amd64-livepatch5 host-install(5)broken REGR. vs. 177428 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 7 xen-install fail REGR. vs. 177428 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 7 xen-install fail REGR. vs. 177428 test-amd64-i386-qemuu-rhel6hvm-amd 7 xen-installfail REGR. vs. 177428 test-amd64-amd64-xl-qcow2 21 guest-start/debian.repeat fail REGR. vs. 177428 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 177428 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 177428 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 177428 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 177428 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 177428 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 177428 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 177428 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 177428 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 177428 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass build-armhf-libvirt 1 build-check(1) starved n/a test-armhf-armhf-examine 1 build-check(1) starved n/a test-armhf-armhf-libvirt 1 build-check(1) starved n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) starved n/a test-armhf-armhf-libvirt-raw 1 build-check(1) starved n/a test-armhf-armhf-xl 1 build-check(1) starved n/a test-armhf-armhf-xl-credit1 1 build-check(1) starved n/a test-armhf-armhf-xl-credit2 1 build-check(1) starved n/a test-armhf-armhf-xl-cubietruck 1 build-check(1) starved n/a test-armhf-armhf-xl-multivcpu 1 build-check(1) starved n/a test-armhf-armhf-xl-rtds 1 build-check(1) starved n/a test-armhf-armhf-xl-vhd 1 build-check(1) starved n/a build-armhf 2 hosts-allocate starved n/a version targeted for testing: xen 91c45cfbab5d9878c0a7021f762988a688d5e91d baseline version: xen 5b9bb91abba7c983def3b4bef71ab08ad360a242 Last test of basis 177428 2023-02-15 22:10:26 Z0 days Testing same since 177485 2023-02-16 10:25:30 Z0 days1 attempts People who touched revisions under test: Michal Orzel jobs: build-amd64-xsm pass build-arm64-xsm
[ovmf test] 177524: all pass - PUSHED
flight 177524 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/177524/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 02fcfdce1e5ce86f1951191883e7e30de5aa08be baseline version: ovmf 5c551d6d912967ada3084033acea8acf37256043 Last test of basis 177466 2023-02-16 06:12:19 Z0 days Testing same since 177524 2023-02-16 19:12:16 Z0 days1 attempts People who touched revisions under test: Joey Vagedes 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 5c551d6d91..02fcfdce1e 02fcfdce1e5ce86f1951191883e7e30de5aa08be -> xen-tested-master
Re: [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
On Thu, 2023-02-16 at 12:19 +, Andrew Cooper wrote:
> On 16/02/2023 12:09 pm, Oleksii wrote:
> > On Thu, 2023-02-16 at 12:44 +0200, Oleksii wrote:
> > > On Thu, 2023-02-16 at 08:31 +0100, Jan Beulich wrote:
> > > > On 15.02.2023 18:59, Oleksii wrote:
> > > > > Hello Jan and community,
> > > > >
> > > > > I experimented and switched RISC-V to x86 implementation. All
> > > > > that
> > > > > I
> > > > > changed in x86 implementation for RISC-V was
> > > > > _ASM_BUGFRAME_TEXT.
> > > > > Other
> > > > > things are the same as for x86.
> > > > >
> > > > > For RISC-V it is fine to skip '%c' modifier so
> > > > > _ASM_BUGFRAME_TEXT
> > > > > will
> > > > > look like:
> > > > >
> > > > > #define _ASM_BUGFRAME_TEXT(second_frame) \
> > > > > ".Lbug%=: ebreak\n"
> > > > > ".pushsection .bug_frames.%[bf_type], \"a\", @progbits\n"
> > > > > ".p2align 2\n"
> > > > > ".Lfrm%=:\n"
> > > > > ".long (.Lbug%= - .Lfrm%=) + %[bf_line_hi]\n"
> > > > > ".long (%[bf_ptr] - .Lfrm%=) + %[bf_line_lo]\n"
> > > > > ".if " #second_frame "\n"
> > > > > ".long 0, %[bf_msg] - .Lfrm%=\n"
> > > > > ".endif\n"
> > > > > ".popsection\n"
> > > > I expect this could be further abstracted such that only the
> > > > actual
> > > > instruction is arch-specific.
> > > Generally, the only thing that can't be abstracted ( as you
> > > mentioned
> > > is an instruction ).
> > >
> > > So we can make additional defines:
> > > 1. #define MODIFIER "" or "c" (depends on architecture) and use
> > > it
> > >
> > > the following way:
> > > ".pushsection .bug_frames.%"MODIFIER"[bf_type], \"a\",
> > > @progbits\n"
> > > ...
> > > 2. #define BUG_INSTR "ebreak" | "ud2" | "..." and use it in the
> > > following way:
> > > ".Lbug%=: "BUG_ISNTR"\n"
> > > ...
> > > Except for these defines which should be specified for each
> > > architecture
> > > all other code will be the same for all architectures.
> > >
> > > But as I understand with modifier 'c' can be issues that not all
> > > compilers support but for ARM and x86 before immediate is
> > > present
> > > punctuation # or $ which are removed by modifier 'c'. And I am
> > > wondering what other ways to remove punctuation before immediate
> > > exist.
> > >
> > > Do you think we should consider that as an issue?
> > >
> > > > > The only thing I am worried about is:
> > > > >
> > > > > #define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \
> > > > > [bf_type] "i" (type), ...
> > > > > because as I understand it can be an issue with 'i' modifier
> > > > > in
> > > > > case of
> > > > > PIE. I am not sure that Xen enables PIE somewhere but
> > > > > still...
> > > > > If it is not an issue then we can use x86 implementation as a
> > > > > generic
> > > > > one.
> > > > "i" is not generally an issue with PIE, it only is when the
> > > > value
> > > > is
> > > > the
> > > > address of a symbol. Here "type" is a constant in all cases.
> > > > (Or
> > > > else
> > > > how would you express an immediate operand of an instruction in
> > > > an
> > > > asm()?)
> > > Hmm. I don't know other ways to express an immediate operand
> > > except
> > > 'i'.
> > >
> > > It looks like type, line, msg are always 'constants'
> > > (
> > > they possibly can be passed without 'i' and used directly inside
> > > asm():
> > > ...
> > > ".pushsection .bug_frames." __stringify(type) ", \"a\",
> > > %progbits\n"\
> > > ...
> > > ) but the issue will be with 'ptr' for which we have to use 'i'.
> > >
> > > And I am not sure about all 'constants'. For example, I think
> > > that it
> > > can be an issue with 'line' = '((line & ((1 << BUG_LINE_LO_WIDTH)
> > > -
> > > 1))
> > > << BUG_DISP_WIDTH)' if we will use that directly inside asm(...).
> > >
> > I think we can avoid 'i' by using 'r' + some kind of mov
> > instruction to
> > write a register value to memory. The code below is pseudo-code:
> > #define _ASM_BUGFRAME_TEXT(second_frame)
> > ...
> > "loc_disp:\n"
> > " .long 0x0"
> > "mov [loc_disp], some_register"
> > ...
> > And the we have to define mov for each architecture.
>
> No, you really cannot. This is the asm equivalent of writing
> something
> like this:
>
> static struct bugframe __section(.bug_frames.1) foo = {
> .loc = insn - &foo + LINE_LO,
> .msg = "bug string" - &foo + LINE_HI,
> };
>
> It is a data structure, not executable code.
Thanks for the clarification.
What about mainly using C for BUG_FRAME and only allocating bug_frame
sections in assembly?
Please look at POC below:
#include
#include
#define BUG_DISP_WIDTH24
#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
struct bug_frame {
signed int loc_disp:BUG_DISP_WIDTH;
unsigned int line_hi:BUG_LINE_HI_WIDTH;
signed int ptr_disp:BUG_DISP_WIDTH;
unsigned int line_lo:BUG_LINE_LO_WIDTH;
signed int msg_disp[];
};
#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
#define bug_ptr(b)
Re: [XEN PATCH 3/4] automation: Remove expired root certificates used to be used by let's encrypt
On Thu, 16 Feb 2023, Anthony PERARD wrote: > On Wed, Feb 15, 2023 at 04:14:53PM -0800, Stefano Stabellini wrote: > > On Wed, 15 Feb 2023, Andrew Cooper wrote: > > > Honestly, I think I'd prefer to drop all of these legacy versions... > > > > Good timing! It just so happens that we need to shave some of the old > > container tests as we have too many build tests on x86 :-) > > > > I would remove Jessie as it reached EOL years ago. Do we really need > > both Centos 7 and 7.2? If not, we could remove 7. > > Actually, 7.2 is older than 7, so I would remove 7.2. (7 would be 7.x so > latest 7 which is 7.9.) Sounds good > > That leaves us with Trusty and Centos 7.2 among these. I would be > > tempted to keep Trusty and add the sed hack of this patch to make it > > work. For Centos 7.2, the hack looks even worse. Would it solve the > > problem to upgrade to the latest Centos 7.x subrelease? Is there really > > no other way to solve the problem? > > So for centos7, the blacklist of the expired root certificate isn't > needed if we simply run `yum update` which for some reason is missing > from the dockerfile... That's much better!
Re: [PATCH 4/6] x86/MSI: use standard C types in structures/unions
On 16/02/2023 2:16 pm, Jan Beulich wrote:
> On 16.02.2023 11:55, Andrew Cooper wrote:
>> On 09/02/2023 10:39 am, Jan Beulich wrote:
>>> Consolidate this to use exclusively standard types, and change
>>> indentation style to Xen's there at the same time (the file already had
>>> a mix of styles).
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Jan Beulich
>> So I suppose Acked-by: Andrew Cooper because
>> this is an improvement on the status quo, but I have quite a few requests.
> Thanks. I'll be happy to carry out some of them (but the sheer amount makes
> it so I'd rather not apply the A-b to the result). It's always difficult to
> judge how much "while doing this" is going to be acceptable ...
Everything I've suggested is minimal enough IMO.
>
>>> --- a/xen/arch/x86/include/asm/msi.h
>>> +++ b/xen/arch/x86/include/asm/msi.h
>>> @@ -66,15 +66,15 @@ struct msi_info {
>>> };
>>>
>>> struct msi_msg {
>>> - union {
>>> - u64 address; /* message address */
>>> - struct {
>>> - u32 address_lo; /* message address low 32 bits */
>>> - u32 address_hi; /* message address high 32 bits */
>>> - };
>>> - };
>>> - u32 data; /* 16 bits of msi message data */
>>> - u32 dest32; /* used when Interrupt Remapping with EIM is
>>> enabled */
>>> +union {
>>> +uint64_t address; /* message address */
>>> +struct {
>>> +uint32_t address_lo; /* message address low 32 bits */
>>> +uint32_t address_hi; /* message address high 32 bits */
>>> +};
>>> +};
>>> +uint32_t data;/* 16 bits of msi message data */
>>> +uint32_t dest32; /* used when Interrupt Remapping with EIM is
>>> enabled */
>> The 16 is actively wrong for data,
> It it? The upper 16 bits aren't used, are they?
Well... I've just found that my local PCI reference doesn't actually
match the spec when it comes to stating the width of the message field.
Guess I need to stop using this reference.
But the rules given would require this to turn into uint16_t as that's
the specified size of the register... But that will probably require a
separate patch.
>
>> but honestly it's only this dest32
>> comment which has any value whatsoever (when it has been de-Intel'd).
>>
>> I'd correct dest32 to reference the AMD too, and delete the rest.
> I guess I'll simply drop "with EIM".
Yeah, probably the easiest adjustment. AMD is more complicated anyway IIRC.
>>> @@ -94,35 +94,35 @@ extern int pci_restore_msi_state(struct
>>> extern int pci_reset_msix_state(struct pci_dev *pdev);
>>>
>>> struct msi_desc {
>>> - struct msi_attrib {
>>> - __u8type; /* {0: unused, 5h:MSI, 11h:MSI-X} */
>>> - __u8pos;/* Location of the MSI capability */
>>> - __u8maskbit : 1;/* mask/pending bit supported ? */
>>> - __u8is_64 : 1;/* Address size: 0=32bit 1=64bit */
>>> - __u8host_masked : 1;
>>> - __u8guest_masked : 1;
>>> - __u16 entry_nr; /* specific enabled entry */
>>> - } msi_attrib;
>>> -
>>> - bool irte_initialized;
>>> - uint8_t gvec; /* guest vector. valid when pi_desc
>>> isn't NULL */
>>> - const struct pi_desc *pi_desc; /* pointer to posted descriptor */
>>> -
>>> - struct list_head list;
>>> -
>>> - union {
>>> - void __iomem *mask_base;/* va for the entry in mask table */
>>> - struct {
>>> - unsigned int nvec;/* number of vectors*/
>>> - unsigned int mpos;/* location of mask register*/
>>> - } msi;
>>> - unsigned int hpet_id; /* HPET (dev is NULL) */
>>> - };
>>> - struct pci_dev *dev;
>>> - int irq;
>>> - int remap_index;/* index in interrupt remapping table */
>>> +struct msi_attrib {
>>> +uint8_t type;/* {0: unused, 5h:MSI, 11h:MSI-X} */
>>> +uint8_t pos; /* Location of the MSI capability */
>>> +uint8_t maskbit : 1; /* mask/pending bit supported ? */
>>> +uint8_t is_64: 1; /* Address size: 0=32bit 1=64bit */
>>> +uint8_t host_masked : 1;
>>> +uint8_t guest_masked : 1;
>>> +uint16_t entry_nr; /* specific enabled entry */
>> entry_nr wants to move up to between pos and maskbit, and then we shrink
>> the total structure by 8 bytes (I think).
> The struct is 6 bytes now and will be 6 bytes with the adjustment you
> suggest. Plus I'd prefer to not do any re-ordering in this patch.
Ah, so I see what went wrong. Right now, we've got:
8b type
8b pos
4b the bitfields
12b padding
16b entry_nr
and rearranging it moves the padding to the end but doesn't drop it,
because overall the structure has 16b alignment because of the uint16_t
If it were packed, then the following byte fields wo
Xen Security Advisory 426 v2 (CVE-2022-27672) - x86: Cross-Thread Return Address Predictions
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256
Xen Security Advisory CVE-2022-27672 / XSA-426
version 2
x86: Cross-Thread Return Address Predictions
UPDATES IN VERSION 2
Xen 4.16 is vulnerable too. The previous analysis of impacted versions
was incorrect.
The same patch is applicable to Xen 4.16, and the staging-4.16 branch
has already had the backport applied.
ISSUE DESCRIPTION
=
It has been discovered that on some AMD CPUs, the RAS (Return Address
Stack, also called RAP - Return Address Predictor - in some AMD
documentation, and RSB - Return Stack Buffer - in Intel terminology) is
dynamically partitioned between non-idle threads. This allows an
attacker to control speculative execution on the adjacent thread.
For more details, see:
https://www.amd.com/en/corporate/product-security/bulletin/amd-sb-1045
IMPACT
==
An attacker might be able to infer the contents of arbitrary host
memory, including memory assigned to other guests.
VULNERABLE SYSTEMS
==
Only AMD CPUs are known to be potentially vulnerable. CPUs from other
hardware vendors are not believed to be impacted.
Only the Zen1 and Zen2 microarchitectures are believed to be potentially
vulnerable. Other microarchitectures are not believed to be vulnerable.
Only configurations with SMT activate are potentially vulnerable. If
SMT is disabled by the firmware, or at runtime with `smt=0` on Xen's
command line, then the platform is not vulnerable.
Xen 4.16 and later contains an optimisation, specifically:
c/s afab477fba3b ("x86/spec-ctrl: Skip RSB overwriting when safe to do so")
which in combination with disabling 32bit PV guests (either at compile
time with CONFIG_PV32=n, or at runtime with `pv=no-32` on the command
line) renders Xen vulnerable to attack from PV guests.
Note: multiple downstreams are known to have backported this
optimisation to older versions of Xen. Consult your software vendor
documentation.
MITIGATION
==
On otherwise-vulnerable configurations, the issue can be mitigated by
booting Xen with `spec-ctrl=rsb`, which will override the aforementioned
optimisation.
Alternatively, SMT can be disabled either in the firmware, or by booting
Xen with `smt=0`.
Alternatively, if 32bit PV guests are only runtime disabled in Xen, this
issue can also be mitigated by booting Xen with `pv=32` to enable
support 32bit PV guests. It is not necessary for a 32bit PV guest to
actually be running in order to mitigate the issue.
RESOLUTION
==
Applying the attached patch resolves this issue.
Note that patches for released versions are generally prepared to
apply to the stable branches, and may not apply cleanly to the most
recent release tarball. Downstreams are encouraged to update to the
tip of the stable branch before applying these patches.
xsa426.patch xen-unstable - Xen 4.16
$ sha256sum xsa426*
425b1d8931e02852afec9fe3d9f1d009f6d8a33c6387b2e8b3896f374732d470 xsa426.patch
$
-BEGIN PGP SIGNATURE-
iQFABAEBCAAqFiEEI+MiLBRfRHX6gGCng/4UyVfoK9kFAmPuawUMHHBncEB4ZW4u
b3JnAAoJEIP+FMlX6CvZW1UIAJ6tjOwbjPJigbSVVfyr5FRnIIYjzVBqkhL5ufvc
TQY6ZoPsEEkXzx+jJeVa3NveiegqNvIdK26exlp7n2NrrWCRWlrdGlp+/83TWfUA
gwxBzERTVBmi67+9razBYKzxKAwXO2zOHsvgSB2aCX43K+e9SvlKMny8Wp9j0Z99
SRGxzZ8D4I7kKnMMpQIGvp/rt5+k+Q2oxXmNHnIsnCGshF+Y+zK7VwlSEpFYE1ga
78XWYULa1qOEbaj+xsPtf9mMIiWfViwKkX7ZT/EPFBbFxGHSK/aeiQmWdNcFGI3D
6L7vfJIo1Xsw26ozja+C+m3cFPhNSYJDRj92oCKmLPl8iII=
=hFGs
-END PGP SIGNATURE-
xsa426.patch
Description: Binary data
Re: [XEN PATCH v2] libs: Fix unstable libs build on FreeBSD, auto-generate version-script
On 16/02/2023 4:55 pm, Anthony PERARD wrote: > On Thu, Feb 16, 2023 at 04:50:09PM +, Andrew Cooper wrote: >> On 16/02/2023 2:10 pm, Anthony PERARD wrote: >>> diff --git a/tools/libs/libs.mk b/tools/libs/libs.mk >>> index 0e4b5e0bd0..ffb6c9f064 100644 >>> --- a/tools/libs/libs.mk >>> +++ b/tools/libs/libs.mk >>> @@ -120,7 +127,7 @@ TAGS: >>> clean:: >>> rm -rf $(TARGETS) *~ $(DEPS_RM) $(OBJS-y) $(PIC_OBJS) >>> rm -f lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR) >>> lib$(LIB_FILE_NAME).so.$(MAJOR) >>> - rm -f headers.chk headers.lst >>> + rm -f headers.chk headers.lst lib*.map.tmp .*.tmp >> Doesn't *.tmp cover lib*.map.tmp ? > There is no "*.tmp" on this command line, but there is ".*.tmp". Oh, of course. I'm blind. Sorry. > >> Also the subject still needs a FreeBSD->LLVM fix. > Sounds good. > >> Both can be fixed on commit. > If you only fix the subject, then that's fine by me. Sure. (Although I will throw this through CircleCI first...) ~Andrew
Re: [XEN PATCH v2] libs: Fix unstable libs build on FreeBSD, auto-generate version-script
On Thu, Feb 16, 2023 at 04:50:09PM +, Andrew Cooper wrote: > On 16/02/2023 2:10 pm, Anthony PERARD wrote: > > diff --git a/tools/libs/libs.mk b/tools/libs/libs.mk > > index 0e4b5e0bd0..ffb6c9f064 100644 > > --- a/tools/libs/libs.mk > > +++ b/tools/libs/libs.mk > > @@ -120,7 +127,7 @@ TAGS: > > clean:: > > rm -rf $(TARGETS) *~ $(DEPS_RM) $(OBJS-y) $(PIC_OBJS) > > rm -f lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR) > > lib$(LIB_FILE_NAME).so.$(MAJOR) > > - rm -f headers.chk headers.lst > > + rm -f headers.chk headers.lst lib*.map.tmp .*.tmp > > Doesn't *.tmp cover lib*.map.tmp ? There is no "*.tmp" on this command line, but there is ".*.tmp". > Also the subject still needs a FreeBSD->LLVM fix. Sounds good. > Both can be fixed on commit. If you only fix the subject, then that's fine by me. Thanks, -- Anthony PERARD
Re: [XEN PATCH v2] libs: Fix unstable libs build on FreeBSD, auto-generate version-script
On 16.02.2023 17:50, Andrew Cooper wrote: > On 16/02/2023 2:10 pm, Anthony PERARD wrote: >> diff --git a/tools/libs/libs.mk b/tools/libs/libs.mk >> index 0e4b5e0bd0..ffb6c9f064 100644 >> --- a/tools/libs/libs.mk >> +++ b/tools/libs/libs.mk >> @@ -120,7 +127,7 @@ TAGS: >> clean:: >> rm -rf $(TARGETS) *~ $(DEPS_RM) $(OBJS-y) $(PIC_OBJS) >> rm -f lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR) >> lib$(LIB_FILE_NAME).so.$(MAJOR) >> -rm -f headers.chk headers.lst >> +rm -f headers.chk headers.lst lib*.map.tmp .*.tmp > > Doesn't *.tmp cover lib*.map.tmp ? It's .*.tmp, not *.tmp (but I agree the leading dot is easy to mistake). Jan
Re: [XEN PATCH v2] libs: Fix unstable libs build on FreeBSD, auto-generate version-script
On 16/02/2023 2:10 pm, Anthony PERARD wrote: > diff --git a/tools/libs/libs.mk b/tools/libs/libs.mk > index 0e4b5e0bd0..ffb6c9f064 100644 > --- a/tools/libs/libs.mk > +++ b/tools/libs/libs.mk > @@ -120,7 +127,7 @@ TAGS: > clean:: > rm -rf $(TARGETS) *~ $(DEPS_RM) $(OBJS-y) $(PIC_OBJS) > rm -f lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR) > lib$(LIB_FILE_NAME).so.$(MAJOR) > - rm -f headers.chk headers.lst > + rm -f headers.chk headers.lst lib*.map.tmp .*.tmp Doesn't *.tmp cover lib*.map.tmp ? Also the subject still needs a FreeBSD->LLVM fix. Both can be fixed on commit. ~Andrew
[libvirt test] 177457: tolerable trouble: fail/pass/starved - PUSHED
flight 177457 libvirt real [real] flight 177511 libvirt real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/177457/ http://logs.test-lab.xenproject.org/osstest/logs/177511/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-i386-libvirt-pair 10 xen-install/src_host fail in 177511 pass in 177457 test-amd64-i386-libvirt-pair 11 xen-install/dst_host fail pass in 177511-retest Tests which did not succeed, but are not blocking: test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 15 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass build-armhf-libvirt 1 build-check(1) starved n/a test-armhf-armhf-libvirt-raw 1 build-check(1) starved n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) starved n/a test-armhf-armhf-libvirt 1 build-check(1) starved n/a build-armhf 2 hosts-allocate starved n/a version targeted for testing: libvirt 598a73335d70b4ef70b84f9730d708c116f88b15 baseline version: libvirt b61c66d1dea2525290b7fa1f41ba6958bc39d63c Last test of basis 177347 2023-02-15 04:18:50 Z1 days Testing same since 177457 2023-02-16 04:21:50 Z0 days1 attempts People who touched revisions under test: Michal Privoznik Peter Krempa jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf starved build-i386 pass build-amd64-libvirt pass build-arm64-libvirt pass build-armhf-libvirt starved build-i386-libvirt pass build-amd64-pvopspass build-arm64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass test-amd64-amd64-libvirt-xsm pass test-arm64-arm64-libvirt-xsm pass test-amd64-i386-libvirt-xsm pass test-amd64-amd64-libvirt pass test-arm64-arm64-libvirt pass test-armhf-armhf-libvirt starved test-amd64-i386-libvirt pass test-amd64-amd64-libvirt-pairpass test-amd64-i386-libvirt-pair fail test-arm64-arm64-libvirt-qcow2 pass test-armhf-armhf-libvirt-qcow2 starved test-arm64-arm64-libvirt-raw pass test-armhf-armhf-libvirt-raw starved test-amd64-i386-libvirt-raw pass test-amd64-amd64-libvirt-vhd pass sg-report-flight on osste
Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On Thu, Feb 16, 2023 at 10:24 AM Jan Beulich wrote:
>
> On 16.02.2023 16:10, Tamas K Lengyel wrote:
> > On Thu, Feb 16, 2023 at 9:27 AM Jan Beulich wrote:
> >>
> >> On 16.02.2023 13:22, Tamas K Lengyel wrote:
> >>> On Thu, Feb 16, 2023 at 3:31 AM Jan Beulich wrote:
>
> On 15.02.2023 17:29, Tamas K Lengyel wrote:
> > On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich
wrote:
> >> Did you consider the alternative of adjusting the ASSERT() in
> > question
> >> instead? It could reasonably become
> >>
> >> #ifdef CONFIG_MEM_SHARING
> >> ASSERT(!p2m_is_hostp2m(p2m) || !remove_root ||
> > !atomic_read(&d->shr_pages));
> >> #endif
> >>
> >> now, I think. That would be less intrusive a change (helpful for
> >> backporting), but there may be other (so far unnamed) benefits of
> >>> pulling
> >> ahead the shared memory teardown.
> >
> > I have a hard time understanding this proposed ASSERT.
>
> It accounts for the various ways p2m_teardown() can (now) be called,
> limiting the actual check for no remaining shared pages to the last
> of all these invocations (on the host p2m with remove_root=true).
>
> Maybe
>
> /* Limit the check to the final invocation. */
> if ( p2m_is_hostp2m(p2m) && remove_root )
> ASSERT(!atomic_read(&d->shr_pages));
>
> would make this easier to follow? Another option might be to move
> the assertion to paging_final_teardown(), ahead of that specific call
> to p2m_teardown().
> >>>
> >>> AFAICT d->shr_pages would still be more then 0 when this is called
> > before
> >>> sharing is torn down so the rearrangement is necessary even if we do
> > this
> >>> assert only on the final invocation. I did a printk in place of this
> > assert
> >>> without the rearrangement and afaict it was always != 0.
> >>
> >> Was your printk() in an if() as above? paging_final_teardown() runs
really
> >> late during cleanup (when we're about to free struct domain), so I
would
> >> be somewhat concerned if by that time the count was still non-zero.
> >
> > Just replaced the assert with a printk. Without calling relinquish
shared
> > pages I don't find it odd that the count is non-zero, it only gets
> > decremented when you do call relinquish. Once the order is corrected it
is
> > zero.
>
> I may be getting you wrong, but it feels like you're still talking about
> early invocations of p2m_teardown() (from underneath domain_kill()) when
> I'm talking about the final one. No matter what ordering inside
> domain_relinquish_resources() (called from domain_kill()), the freeing
> will have happened by the time that process completes. Which is before
> the (typically last) last domain ref would be put (near the end of
> domain_kill()), and hence before the domain freeing process might be
> invoked (which is where paging_final_teardown() gets involved; see
> {,arch_}domain_destroy()).
I don't recall seeing a count with 0 before I reordered things but the
output on the serial may also have been truncated due to it printing a ton
of lines very quickly, so it could be the last one was zero just didn't
make it to my screen.
Tamas
Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On 16.02.2023 16:10, Tamas K Lengyel wrote:
> On Thu, Feb 16, 2023 at 9:27 AM Jan Beulich wrote:
>>
>> On 16.02.2023 13:22, Tamas K Lengyel wrote:
>>> On Thu, Feb 16, 2023 at 3:31 AM Jan Beulich wrote:
On 15.02.2023 17:29, Tamas K Lengyel wrote:
> On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich wrote:
>> Did you consider the alternative of adjusting the ASSERT() in
> question
>> instead? It could reasonably become
>>
>> #ifdef CONFIG_MEM_SHARING
>> ASSERT(!p2m_is_hostp2m(p2m) || !remove_root ||
> !atomic_read(&d->shr_pages));
>> #endif
>>
>> now, I think. That would be less intrusive a change (helpful for
>> backporting), but there may be other (so far unnamed) benefits of
>>> pulling
>> ahead the shared memory teardown.
>
> I have a hard time understanding this proposed ASSERT.
It accounts for the various ways p2m_teardown() can (now) be called,
limiting the actual check for no remaining shared pages to the last
of all these invocations (on the host p2m with remove_root=true).
Maybe
/* Limit the check to the final invocation. */
if ( p2m_is_hostp2m(p2m) && remove_root )
ASSERT(!atomic_read(&d->shr_pages));
would make this easier to follow? Another option might be to move
the assertion to paging_final_teardown(), ahead of that specific call
to p2m_teardown().
>>>
>>> AFAICT d->shr_pages would still be more then 0 when this is called
> before
>>> sharing is torn down so the rearrangement is necessary even if we do
> this
>>> assert only on the final invocation. I did a printk in place of this
> assert
>>> without the rearrangement and afaict it was always != 0.
>>
>> Was your printk() in an if() as above? paging_final_teardown() runs really
>> late during cleanup (when we're about to free struct domain), so I would
>> be somewhat concerned if by that time the count was still non-zero.
>
> Just replaced the assert with a printk. Without calling relinquish shared
> pages I don't find it odd that the count is non-zero, it only gets
> decremented when you do call relinquish. Once the order is corrected it is
> zero.
I may be getting you wrong, but it feels like you're still talking about
early invocations of p2m_teardown() (from underneath domain_kill()) when
I'm talking about the final one. No matter what ordering inside
domain_relinquish_resources() (called from domain_kill()), the freeing
will have happened by the time that process completes. Which is before
the (typically last) last domain ref would be put (near the end of
domain_kill()), and hence before the domain freeing process might be
invoked (which is where paging_final_teardown() gets involved; see
{,arch_}domain_destroy()).
Jan
[linux-linus test] 177451: tolerable trouble: fail/pass/starved - PUSHED
flight 177451 linux-linus real [real] flight 177502 linux-linus real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/177451/ http://logs.test-lab.xenproject.org/osstest/logs/177502/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-amd64-freebsd12-amd64 22 guest-start.2 fail pass in 177502-retest Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 177400 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 177400 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 177400 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 177400 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 177400 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-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-examine 1 build-check(1) starved n/a test-armhf-armhf-libvirt 1 build-check(1) starved n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) starved n/a test-armhf-armhf-libvirt-raw 1 build-check(1) starved n/a test-armhf-armhf-xl 1 build-check(1) starved n/a test-armhf-armhf-xl-credit1 1 build-check(1) starved n/a test-armhf-armhf-xl-credit2 1 build-check(1) starved n/a test-armhf-armhf-xl-cubietruck 1 build-check(1) starved n/a test-armhf-armhf-xl-multivcpu 1 build-check(1) starved n/a test-armhf-armhf-xl-rtds 1 build-check(1) starved n/a test-armhf-armhf-xl-vhd 1 build-check(1) starved n/a build-armhf-libvirt 1 build-check(1) starved n/a build-armhf 2 hosts-allocate starved n/a version targeted for testing: linux033c40a89f55525139fd5b6342281b09b97d05bf baseline version: linuxe1c04510f521e853019afeca2a5991a5ef8d6a5b Last test of basis 177400 2023-02-15 15:49:45 Z0 days Testing same since 177451 2023-02-16 02:59:06 Z0 days1 attempts People who touched revisions under test: Chuck Lever Jeff Layton John Johansen Linus Torvalds Steven Rostedt (Google) jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf starved build-i386 pass build-amd64-libvirt pass build-arm64-libvirt pass build-armhf-libvirt starved build-i386-libvirt pass build-amd64-pvopspass build-arm64-pvopspass build-armhf-pvopspass
Re: [RFC PATCH v2 1/2] xen/memory : Add a stats_table resource type
On 16.02.2023 16:07, Matias Ezequiel Vara Larsen wrote:
> On Wed, Dec 14, 2022 at 08:29:53AM +0100, Jan Beulich wrote:
>> On 07.10.2022 14:39, Matias Ezequiel Vara Larsen wrote:
>>> @@ -287,6 +289,20 @@ static inline void vcpu_runstate_change(
>>> }
>>>
>>> v->runstate.state = new_state;
>>> +
>>> +vcpustats_va = (shared_vcpustatspage_t*)d->vcpustats_page.va;
>>> +if ( vcpustats_va )
>>> +{
>>> + vcpustats_va->vcpu_info[v->vcpu_id].version =
>>> + version_update_begin(vcpustats_va->vcpu_info[v->vcpu_id].version);
>>> +smp_wmb();
>>> +memcpy(&vcpustats_va->vcpu_info[v->vcpu_id].runstate_running_time,
>>> + &v->runstate.time[RUNSTATE_running],
>>> + sizeof(v->runstate.time[RUNSTATE_running]));
>>> +smp_wmb();
>>> +vcpustats_va->vcpu_info[v->vcpu_id].version =
>>> +
>>> version_update_end(vcpustats_va->vcpu_info[v->vcpu_id].version);
>>> +}
>>
>> A further aspect to consider here is cache line ping-pong. I think the
>> per-vCPU elements of the array want to be big enough to not share a
>> cache line. The interface being generic this presents some challenge
>> in determining what the supposed size is to be. However, taking into
>> account the extensibility question, maybe the route to take is to
>> simply settle on a power-of-2 value somewhere between x86'es and Arm's
>> cache line sizes and the pretty common page size of 4k, e.g. 512 bytes
>> or 1k?
>>
>
> I do not now how to address this. I was thinking to align each vcpu_stats
> instance to a multiple of the cache-line. I would pick up the first multiple
> that is bigger to the size of the vcpu_stats structure. For example, currently
> the structure is 16 bytes so I would align each instance in a frame to 64
> bytes. Would it make sense?
Well, 64 may be an option, but I gave higher numbers for a reason. One thing
I don't know is what common cache line sizes are on Arm or e.g. RISC-V.
>>> --- a/xen/include/public/vcpu.h
>>> +++ b/xen/include/public/vcpu.h
>>> @@ -235,6 +235,22 @@ struct vcpu_register_time_memory_area {
>>> typedef struct vcpu_register_time_memory_area
>>> vcpu_register_time_memory_area_t;
>>> DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>>>
>>> +struct vcpu_stats{
>>> +/* If the least-significant bit of the version number is set then an
>>> update
>>> + * is in progress and the guest must wait to read a consistent set of
>>> values
>>> + * This mechanism is similar to Linux's seqlock.
>>> + */
>>> +uint32_t version;
>>> +uint32_t pad0;
>>> +uint64_t runstate_running_time;
>>> +};
>>> +
>>> +struct shared_vcpustatspage {
>>> +struct vcpu_stats vcpu_info[1];
>>> +};
>>> +
>>> +typedef struct shared_vcpustatspage shared_vcpustatspage_t;
>>
>> For new additions please avoid further name space issues: All types
>> and alike want to be prefixed by "xen_".
>
> Should I name it "xen_shared_vcpustatspage_t" instead?
Yes, that would fulfill the name space requirements. It's getting longish,
so you may want to think about abbreviating it some. For example, I'm not
sure the "page" in the name is really necessary.
Jan
Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On Thu, Feb 16, 2023 at 9:27 AM Jan Beulich wrote: > > On 16.02.2023 13:22, Tamas K Lengyel wrote: > > On Thu, Feb 16, 2023 at 3:31 AM Jan Beulich wrote: > >> > >> On 15.02.2023 17:29, Tamas K Lengyel wrote: > >>> On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich wrote: > Did you consider the alternative of adjusting the ASSERT() in question > instead? It could reasonably become > > #ifdef CONFIG_MEM_SHARING > ASSERT(!p2m_is_hostp2m(p2m) || !remove_root || > >>> !atomic_read(&d->shr_pages)); > #endif > > now, I think. That would be less intrusive a change (helpful for > backporting), but there may be other (so far unnamed) benefits of > > pulling > ahead the shared memory teardown. > >>> > >>> I have a hard time understanding this proposed ASSERT. > >> > >> It accounts for the various ways p2m_teardown() can (now) be called, > >> limiting the actual check for no remaining shared pages to the last > >> of all these invocations (on the host p2m with remove_root=true). > >> > >> Maybe > >> > >> /* Limit the check to the final invocation. */ > >> if ( p2m_is_hostp2m(p2m) && remove_root ) > >> ASSERT(!atomic_read(&d->shr_pages)); > >> > >> would make this easier to follow? Another option might be to move > >> the assertion to paging_final_teardown(), ahead of that specific call > >> to p2m_teardown(). > > > > AFAICT d->shr_pages would still be more then 0 when this is called before > > sharing is torn down so the rearrangement is necessary even if we do this > > assert only on the final invocation. I did a printk in place of this assert > > without the rearrangement and afaict it was always != 0. > > Was your printk() in an if() as above? paging_final_teardown() runs really > late during cleanup (when we're about to free struct domain), so I would > be somewhat concerned if by that time the count was still non-zero. Just replaced the assert with a printk. Without calling relinquish shared pages I don't find it odd that the count is non-zero, it only gets decremented when you do call relinquish. Once the order is corrected it is zero. Tamas
Re: [RFC PATCH v2 1/2] xen/memory : Add a stats_table resource type
On 16.02.2023 15:48, Matias Ezequiel Vara Larsen wrote:
> On Tue, Dec 13, 2022 at 06:02:55PM +0100, Jan Beulich wrote:
>> On 07.10.2022 14:39, Matias Ezequiel Vara Larsen wrote:
>>> This commit proposes a new mechanism to query the RUNSTATE_running counter
>>> for
>>> a given vcpu from a dom0 userspace application. This commit proposes to
>>> expose
>>> that counter by using the acquire_resource interface. The current mechanism
>>> relies on the XEN_DOMCTL_getvcpuinfo and holds a single global domctl_lock
>>> for
>>> the entire hypercall; and iterate over every vcpu in the system for every
>>> update thus impacting operations that share that lock.
>>>
>>> This commit proposes to expose vcpu RUNSTATE_running via the
>>> xenforeignmemory interface thus preventing to issue the hypercall and
>>> holding
>>> the lock. For that purpose, a new resource type named stats_table is added.
>>> The
>>> first frame of this resource stores per-vcpu counters. The frame has one
>>> entry
>>> of type struct vcpu_stats per vcpu. The allocation of this frame only
>>> happens
>>> if the resource is requested. The frame is released after the domain is
>>> destroyed.
>>>
>>> Note that the updating of this counter is in a hot path, thus, in this
>>> commit,
>>> copying only happens if it is specifically required.
>>>
>>> Note that the exposed structure is extensible in two ways. First, the
>>> structure
>>> vcpu_stats can be extended with new per-vcpu counters while it fits in a
>>> frame.
>>
>> I'm afraid I don't see how this is "extensible". I would recommend that
>> you outline for yourself how a change would look like to actually add
>> such a 2nd counter. While doing that keep in mind that whatever changes
>> you make may not break existing consumers.
>>
>> It's also not clear what you mean with "fits in a frame": struct
>> shared_vcpustatspage is a container for an array with a single element.
>> I may guess (looking at just the public interface) that this really is
>> meant to be a flexible array (and hence should be marked as such - see
>> other uses of XEN_FLEX_ARRAY_DIM in the public headers). Yet if that's
>> the case, then a single page already won't suffice for a domain with
>> sufficiently many vCPU-s.
>>
>
> I taclked this by using "d->max_vcpus" to calculate the number of required
> frames
> to allocate for a given guest. Also, I added a new type-specific resource
> named
> XENMEM_resource_stats_table_id_vcpustats to host per-vcpu counters. I
> completely forgot the "id" in the previous series.
May I suggest that before you submit a new version of your patches, you
make yourself (and then perhaps submit for commenting) a layout of the
data structures you want to introduce, including how they interact and
what "granularity" (global, per-domain, per-vCPU, per-pCPU, or alike)
they are. While doing that, as previously suggested, put yourself in
the position of someone later wanting to add another counter. With the
initial logic there, such an extension should then end up being pretty
mechanical, or else the arrangement likely needs further adjustment.
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -741,6 +741,8 @@ void hvm_domain_relinquish_resources(struct domain *d)
>>>
>>> ioreq_server_destroy_all(d);
>>>
>>> +stats_free_vcpu_mfn(d);
>>
>> How come this lives here? Surely this new feature should be not only
>> guest-type independent, but also arch-agnostic? Clearly you putting
>> the new data in struct domain (and not struct arch_domain or yet
>> deeper in the hierarchy) indicates you may have been meaning to make
>> it so.
>>
>
> The whole feature shall to be guest-type independent and also arch-agnostic.
> Would it be better to put it at xen/common/domain.c:domain_kill()?
Likely, and the earlier this is (safely) possible, the better.
>>> @@ -1162,6 +1171,88 @@ static int acquire_vmtrace_buf(
>>> return nr_frames;
>>> }
>>>
>>> +void stats_free_vcpu_mfn(struct domain * d)
>>> +{
>>> +struct page_info *pg = d->vcpustats_page.pg;
>>> +
>>> +if ( !pg )
>>> +return;
>>> +
>>> +d->vcpustats_page.pg = NULL;
>>> +
>>> +if ( d->vcpustats_page.va )
>>> +unmap_domain_page_global(d->vcpustats_page.va);
>>> +
>>> +d->vcpustats_page.va = NULL;
>>
>> We ought to gain UNMAP_DOMAIN_PAGE_GLOBAL() for purposes like this one,
>> paralleling UNMAP_DOMAIN_PAGE().
>>
>
> I do not understand this comment. Could you elaborate it?
The last four lines of code would better be collapsed to a single one,
using the mentioned yet-to-be-introduced construct. I assume you did
look up UNMAP_DOMAIN_PAGE() to spot its difference from
unmap_domain_page()?
>>> +static int stats_vcpu_alloc_mfn(struct domain *d)
>>> +{
>>> +struct page_info *pg;
>>> +
>>> +pg = alloc_domheap_page(d, MEMF_no_refcount);
>>> +
>>> +if ( !pg )
>>> +return -ENOMEM;
>>> +
>>> +if ( !get_page_and_type(pg, d, PGT_writable_page) ) {
>>
>> Style: Brace p
Re: [RFC PATCH v2 1/2] xen/memory : Add a stats_table resource type
On Wed, Dec 14, 2022 at 08:29:53AM +0100, Jan Beulich wrote:
> On 07.10.2022 14:39, Matias Ezequiel Vara Larsen wrote:
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -1078,6 +1078,12 @@ unsigned int ioreq_server_max_frames(const struct
> > domain *d)
> > return nr;
> > }
> >
> > +unsigned int stats_table_max_frames(const struct domain *d)
> > +{
> > +/* One frame per 512 vcpus. */
> > +return 1;
> > +}
>
> Beyond my earlier comment (and irrespective of this needing changing
> anyway): I guess this "512" was not updated to match the now larger
> size of struct vcpu_stats?
In the next series, I am calculating the number of frames by:
nr = DIV_ROUND_UP(d->max_vcpus * sizeof(struct vcpu_stats), PAGE_SIZE);
>
> > +static int stats_vcpu_alloc_mfn(struct domain *d)
> > +{
> > +struct page_info *pg;
> > +
> > +pg = alloc_domheap_page(d, MEMF_no_refcount);
>
> The ioreq and vmtrace resources are also allocated this way, but they're
> HVM-specific. The one here being supposed to be VM-type independent, I'm
> afraid such pages will be accessible by an "owning" PV domain (it'll
> need to guess the MFN, but that's no excuse).
>
> > +if ( !pg )
> > +return -ENOMEM;
> > +
> > +if ( !get_page_and_type(pg, d, PGT_writable_page) ) {
> > +put_page_alloc_ref(pg);
> > +return -ENODATA;
> > +}
> > +
> > +d->vcpustats_page.va = __map_domain_page_global(pg);
> > +if ( !d->vcpustats_page.va )
> > +goto fail;
> > +
> > +d->vcpustats_page.pg = pg;
> > +clear_page(d->vcpustats_page.va);
>
> Beyond my earlier comment: I think that by the time the surrounding
> hypercall returns the page ought to contain valid data. Otherwise I
> see no way for the consumer to know from which point on the data is
> going to be valid.
>
> > @@ -287,6 +289,20 @@ static inline void vcpu_runstate_change(
> > }
> >
> > v->runstate.state = new_state;
> > +
> > +vcpustats_va = (shared_vcpustatspage_t*)d->vcpustats_page.va;
> > +if ( vcpustats_va )
> > +{
> > + vcpustats_va->vcpu_info[v->vcpu_id].version =
> > + version_update_begin(vcpustats_va->vcpu_info[v->vcpu_id].version);
> > +smp_wmb();
> > +memcpy(&vcpustats_va->vcpu_info[v->vcpu_id].runstate_running_time,
> > + &v->runstate.time[RUNSTATE_running],
> > + sizeof(v->runstate.time[RUNSTATE_running]));
> > +smp_wmb();
> > +vcpustats_va->vcpu_info[v->vcpu_id].version =
> > +
> > version_update_end(vcpustats_va->vcpu_info[v->vcpu_id].version);
> > +}
>
> A further aspect to consider here is cache line ping-pong. I think the
> per-vCPU elements of the array want to be big enough to not share a
> cache line. The interface being generic this presents some challenge
> in determining what the supposed size is to be. However, taking into
> account the extensibility question, maybe the route to take is to
> simply settle on a power-of-2 value somewhere between x86'es and Arm's
> cache line sizes and the pretty common page size of 4k, e.g. 512 bytes
> or 1k?
>
I do not now how to address this. I was thinking to align each vcpu_stats
instance to a multiple of the cache-line. I would pick up the first multiple
that is bigger to the size of the vcpu_stats structure. For example, currently
the structure is 16 bytes so I would align each instance in a frame to 64
bytes. Would it make sense?
> > --- a/xen/include/public/vcpu.h
> > +++ b/xen/include/public/vcpu.h
> > @@ -235,6 +235,22 @@ struct vcpu_register_time_memory_area {
> > typedef struct vcpu_register_time_memory_area
> > vcpu_register_time_memory_area_t;
> > DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
> >
> > +struct vcpu_stats{
> > +/* If the least-significant bit of the version number is set then an
> > update
> > + * is in progress and the guest must wait to read a consistent set of
> > values
> > + * This mechanism is similar to Linux's seqlock.
> > + */
> > +uint32_t version;
> > +uint32_t pad0;
> > +uint64_t runstate_running_time;
> > +};
> > +
> > +struct shared_vcpustatspage {
> > +struct vcpu_stats vcpu_info[1];
> > +};
> > +
> > +typedef struct shared_vcpustatspage shared_vcpustatspage_t;
>
> For new additions please avoid further name space issues: All types
> and alike want to be prefixed by "xen_".
>
Should I name it "xen_shared_vcpustatspage_t" instead?
Matias
Re: [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
On 16.02.2023 11:44, Oleksii wrote: > On Thu, 2023-02-16 at 08:31 +0100, Jan Beulich wrote: >> On 15.02.2023 18:59, Oleksii wrote: >>> The only thing I am worried about is: >>> >>> #define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \ >>> [bf_type] "i" (type), ... >>> because as I understand it can be an issue with 'i' modifier in >>> case of >>> PIE. I am not sure that Xen enables PIE somewhere but still... >>> If it is not an issue then we can use x86 implementation as a >>> generic >>> one. >> >> "i" is not generally an issue with PIE, it only is when the value is >> the >> address of a symbol. Here "type" is a constant in all cases. (Or else >> how would you express an immediate operand of an instruction in an >> asm()?) > Hmm. I don't know other ways to express an immediate operand except > 'i'. > > It looks like type, line, msg are always 'constants' > ( > they possibly can be passed without 'i' and used directly inside asm(): >... >".pushsection .bug_frames." __stringify(type) ", \"a\", > %progbits\n"\ >... > ) but the issue will be with 'ptr' for which we have to use 'i'. > > And I am not sure about all 'constants'. For example, I think that it > can be an issue with 'line' = '((line & ((1 << BUG_LINE_LO_WIDTH) - 1)) > << BUG_DISP_WIDTH)' if we will use that directly inside asm(...). Not matter how complex an expression, the compiler will evaluate it to a plain number if it's a constant expression. The only think to worry about with "i" is, as said, is the value supplied is the address of some symbol (or an expression involving such). Jan
Re: [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
On 16.02.2023 11:48, Andrew Cooper wrote: > On 16/02/2023 7:31 am, Jan Beulich wrote: >> On 15.02.2023 18:59, Oleksii wrote: >>> Hello Jan and community, >>> >>> I experimented and switched RISC-V to x86 implementation. All that I >>> changed in x86 implementation for RISC-V was _ASM_BUGFRAME_TEXT. Other >>> things are the same as for x86. >>> >>> For RISC-V it is fine to skip '%c' modifier so _ASM_BUGFRAME_TEXT will >>> look like: >>> >>> #define _ASM_BUGFRAME_TEXT(second_frame) \ >>> ".Lbug%=: ebreak\n" >>> ".pushsection .bug_frames.%[bf_type], \"a\", @progbits\n" >>> ".p2align 2\n" >>> ".Lfrm%=:\n" >>> ".long (.Lbug%= - .Lfrm%=) + %[bf_line_hi]\n" >>> ".long (%[bf_ptr] - .Lfrm%=) + %[bf_line_lo]\n" >>> ".if " #second_frame "\n" >>> ".long 0, %[bf_msg] - .Lfrm%=\n" >>> ".endif\n" >>> ".popsection\n" >> I expect this could be further abstracted such that only the actual >> instruction is arch-specific. >> >>> The only thing I am worried about is: >>> >>> #define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \ >>> [bf_type] "i" (type), ... >>> because as I understand it can be an issue with 'i' modifier in case of >>> PIE. I am not sure that Xen enables PIE somewhere but still... >>> If it is not an issue then we can use x86 implementation as a generic >>> one. >> "i" is not generally an issue with PIE, it only is when the value is the >> address of a symbol. Here "type" is a constant in all cases. (Or else >> how would you express an immediate operand of an instruction in an >> asm()?) > > At a guess, the problem isn't type. It's the line number, which ends up > in a relocation. Why would that be a problem? If there's a relocation in the first place (not because of the line number, but because of the other part of the expression), then it would only alter the addend of that relocation. Jan
Re: [RFC PATCH v2 1/2] xen/memory : Add a stats_table resource type
Hello Jan and thanks for your comments. I addressed most of the them but
I've still some questions. Please find my questions below:
On Tue, Dec 13, 2022 at 06:02:55PM +0100, Jan Beulich wrote:
> On 07.10.2022 14:39, Matias Ezequiel Vara Larsen wrote:
> > This commit proposes a new mechanism to query the RUNSTATE_running counter
> > for
> > a given vcpu from a dom0 userspace application. This commit proposes to
> > expose
> > that counter by using the acquire_resource interface. The current mechanism
> > relies on the XEN_DOMCTL_getvcpuinfo and holds a single global domctl_lock
> > for
> > the entire hypercall; and iterate over every vcpu in the system for every
> > update thus impacting operations that share that lock.
> >
> > This commit proposes to expose vcpu RUNSTATE_running via the
> > xenforeignmemory interface thus preventing to issue the hypercall and
> > holding
> > the lock. For that purpose, a new resource type named stats_table is added.
> > The
> > first frame of this resource stores per-vcpu counters. The frame has one
> > entry
> > of type struct vcpu_stats per vcpu. The allocation of this frame only
> > happens
> > if the resource is requested. The frame is released after the domain is
> > destroyed.
> >
> > Note that the updating of this counter is in a hot path, thus, in this
> > commit,
> > copying only happens if it is specifically required.
> >
> > Note that the exposed structure is extensible in two ways. First, the
> > structure
> > vcpu_stats can be extended with new per-vcpu counters while it fits in a
> > frame.
>
> I'm afraid I don't see how this is "extensible". I would recommend that
> you outline for yourself how a change would look like to actually add
> such a 2nd counter. While doing that keep in mind that whatever changes
> you make may not break existing consumers.
>
> It's also not clear what you mean with "fits in a frame": struct
> shared_vcpustatspage is a container for an array with a single element.
> I may guess (looking at just the public interface) that this really is
> meant to be a flexible array (and hence should be marked as such - see
> other uses of XEN_FLEX_ARRAY_DIM in the public headers). Yet if that's
> the case, then a single page already won't suffice for a domain with
> sufficiently many vCPU-s.
>
I taclked this by using "d->max_vcpus" to calculate the number of required
frames
to allocate for a given guest. Also, I added a new type-specific resource named
XENMEM_resource_stats_table_id_vcpustats to host per-vcpu counters. I
completely forgot the "id" in the previous series.
> > Second, new frames can be added in case new counters are required.
>
> Are you talking of "new counters" here which aren't "new per-vcpu
> counters"? Or else what's the difference from the 1st way?
Yes, I was talking about that sort of counters. In the next series, that sort
of counters could be added by adding a new type-specific resource id.
>
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -741,6 +741,8 @@ void hvm_domain_relinquish_resources(struct domain *d)
> >
> > ioreq_server_destroy_all(d);
> >
> > +stats_free_vcpu_mfn(d);
>
> How come this lives here? Surely this new feature should be not only
> guest-type independent, but also arch-agnostic? Clearly you putting
> the new data in struct domain (and not struct arch_domain or yet
> deeper in the hierarchy) indicates you may have been meaning to make
> it so.
>
The whole feature shall to be guest-type independent and also arch-agnostic.
Would it be better to put it at xen/common/domain.c:domain_kill()?
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -1078,6 +1078,12 @@ unsigned int ioreq_server_max_frames(const struct
> > domain *d)
> > return nr;
> > }
> >
> > +unsigned int stats_table_max_frames(const struct domain *d)
> > +{
> > +/* One frame per 512 vcpus. */
> > +return 1;
> > +}
>
> As alluded to earlier already - 1 isn't going to be suitable for
> arbitrary size domains. (Yes, HVM domains are presently limited to
> 128 vCPU-s, but as per above this shouldn't be a HVM-only feature.)
>
I am going to use "d->max_vcpus" to calculate the number of required frames for
per-vcpu counters for a given guest.
> > @@ -1162,6 +1171,88 @@ static int acquire_vmtrace_buf(
> > return nr_frames;
> > }
> >
> > +void stats_free_vcpu_mfn(struct domain * d)
> > +{
> > +struct page_info *pg = d->vcpustats_page.pg;
> > +
> > +if ( !pg )
> > +return;
> > +
> > +d->vcpustats_page.pg = NULL;
> > +
> > +if ( d->vcpustats_page.va )
> > +unmap_domain_page_global(d->vcpustats_page.va);
> > +
> > +d->vcpustats_page.va = NULL;
>
> We ought to gain UNMAP_DOMAIN_PAGE_GLOBAL() for purposes like this one,
> paralleling UNMAP_DOMAIN_PAGE().
>
I do not understand this comment. Could you elaborate it?
> > +put_page_alloc_ref(pg);
> > +put_page_and_type(pg);
> > +}
> > +
> > +static int stats
Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On 16.02.2023 13:22, Tamas K Lengyel wrote: > On Thu, Feb 16, 2023 at 3:31 AM Jan Beulich wrote: >> >> On 15.02.2023 17:29, Tamas K Lengyel wrote: >>> On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich wrote: Did you consider the alternative of adjusting the ASSERT() in question instead? It could reasonably become #ifdef CONFIG_MEM_SHARING ASSERT(!p2m_is_hostp2m(p2m) || !remove_root || >>> !atomic_read(&d->shr_pages)); #endif now, I think. That would be less intrusive a change (helpful for backporting), but there may be other (so far unnamed) benefits of > pulling ahead the shared memory teardown. >>> >>> I have a hard time understanding this proposed ASSERT. >> >> It accounts for the various ways p2m_teardown() can (now) be called, >> limiting the actual check for no remaining shared pages to the last >> of all these invocations (on the host p2m with remove_root=true). >> >> Maybe >> >> /* Limit the check to the final invocation. */ >> if ( p2m_is_hostp2m(p2m) && remove_root ) >> ASSERT(!atomic_read(&d->shr_pages)); >> >> would make this easier to follow? Another option might be to move >> the assertion to paging_final_teardown(), ahead of that specific call >> to p2m_teardown(). > > AFAICT d->shr_pages would still be more then 0 when this is called before > sharing is torn down so the rearrangement is necessary even if we do this > assert only on the final invocation. I did a printk in place of this assert > without the rearrangement and afaict it was always != 0. Was your printk() in an if() as above? paging_final_teardown() runs really late during cleanup (when we're about to free struct domain), so I would be somewhat concerned if by that time the count was still non-zero. Jan
Re: [PATCH 4/6] x86/MSI: use standard C types in structures/unions
On 16.02.2023 11:55, Andrew Cooper wrote:
> On 09/02/2023 10:39 am, Jan Beulich wrote:
>> Consolidate this to use exclusively standard types, and change
>> indentation style to Xen's there at the same time (the file already had
>> a mix of styles).
>>
>> No functional change intended.
>>
>> Signed-off-by: Jan Beulich
>
> So I suppose Acked-by: Andrew Cooper because
> this is an improvement on the status quo, but I have quite a few requests.
Thanks. I'll be happy to carry out some of them (but the sheer amount makes
it so I'd rather not apply the A-b to the result). It's always difficult to
judge how much "while doing this" is going to be acceptable ...
>> --- a/xen/arch/x86/include/asm/msi.h
>> +++ b/xen/arch/x86/include/asm/msi.h
>> @@ -66,15 +66,15 @@ struct msi_info {
>> };
>>
>> struct msi_msg {
>> -union {
>> -u64 address; /* message address */
>> -struct {
>> -u32 address_lo; /* message address low 32 bits */
>> -u32 address_hi; /* message address high 32 bits */
>> -};
>> -};
>> -u32 data; /* 16 bits of msi message data */
>> -u32 dest32; /* used when Interrupt Remapping with EIM is
>> enabled */
>> +union {
>> +uint64_t address; /* message address */
>> +struct {
>> +uint32_t address_lo; /* message address low 32 bits */
>> +uint32_t address_hi; /* message address high 32 bits */
>> +};
>> +};
>> +uint32_t data;/* 16 bits of msi message data */
>> +uint32_t dest32; /* used when Interrupt Remapping with EIM is
>> enabled */
>
> The 16 is actively wrong for data,
It it? The upper 16 bits aren't used, are they?
> but honestly it's only this dest32
> comment which has any value whatsoever (when it has been de-Intel'd).
>
> I'd correct dest32 to reference the AMD too, and delete the rest.
I guess I'll simply drop "with EIM".
>> @@ -94,35 +94,35 @@ extern int pci_restore_msi_state(struct
>> extern int pci_reset_msix_state(struct pci_dev *pdev);
>>
>> struct msi_desc {
>> -struct msi_attrib {
>> -__u8type; /* {0: unused, 5h:MSI, 11h:MSI-X} */
>> -__u8pos;/* Location of the MSI capability */
>> -__u8maskbit : 1;/* mask/pending bit supported ? */
>> -__u8is_64 : 1;/* Address size: 0=32bit 1=64bit */
>> -__u8host_masked : 1;
>> -__u8guest_masked : 1;
>> -__u16 entry_nr; /* specific enabled entry */
>> -} msi_attrib;
>> -
>> -bool irte_initialized;
>> -uint8_t gvec; /* guest vector. valid when pi_desc
>> isn't NULL */
>> -const struct pi_desc *pi_desc; /* pointer to posted descriptor */
>> -
>> -struct list_head list;
>> -
>> -union {
>> -void __iomem *mask_base;/* va for the entry in mask table */
>> -struct {
>> -unsigned int nvec;/* number of vectors*/
>> -unsigned int mpos;/* location of mask register*/
>> -} msi;
>> -unsigned int hpet_id; /* HPET (dev is NULL) */
>> -};
>> -struct pci_dev *dev;
>> -int irq;
>> -int remap_index;/* index in interrupt remapping table */
>> +struct msi_attrib {
>> +uint8_t type;/* {0: unused, 5h:MSI, 11h:MSI-X} */
>> +uint8_t pos; /* Location of the MSI capability */
>> +uint8_t maskbit : 1; /* mask/pending bit supported ? */
>> +uint8_t is_64: 1; /* Address size: 0=32bit 1=64bit */
>> +uint8_t host_masked : 1;
>> +uint8_t guest_masked : 1;
>> +uint16_t entry_nr; /* specific enabled entry */
>
> entry_nr wants to move up to between pos and maskbit, and then we shrink
> the total structure by 8 bytes (I think).
The struct is 6 bytes now and will be 6 bytes with the adjustment you
suggest. Plus I'd prefer to not do any re-ordering in this patch.
>> @@ -180,48 +180,48 @@ int msi_free_irq(struct msi_desc *entry)
>>
>> struct __packed msg_data {
>> #if defined(__LITTLE_ENDIAN_BITFIELD)
>
> There's no such thing as a big endian x86 bitfield. Just delete this
> ifdefary to simplify the result.
Will do.
> Additionally, the structure doesn't need to be packed - its a single
> uint32_t's worth of bitfield.
Like with re-ordering I would prefer to not touch entirely unrelated
aspects. I'll see if I can motivate myself to make a separate follow-on
change.
> Finally, can we drop the reserved fields and leave them as anonymous
> bitfields?
Perhaps - I can give that a try, hoping that we don't access them
anywhere by their name (even if just to e.g. zero them).
Jan
[XEN PATCH v2] libs: Fix unstable libs build on FreeBSD, auto-generate version-script
Unfortunatly, --default-symver doesn't work with LLVM's LD, LLD.
Instead, we will generate a temporary version-script.
In order to allow regenerating the script, we'll have a different
filename. In order to check if the content is up-to-date, we'll always
generated it and compare.
Reported-by: Andrew Cooper
Fixes: 98d95437edb6 ("libs: Fix auto-generation of version-script for unstable
libs")
Signed-off-by: Anthony PERARD
---
Notes:
v2:
- Replace "VERS" by "lib$(LIB_FILE_NAME)" in the new .map file.
- Set version-script to lib$(LIB_FILE_NAME).map.tmp, like the filename
used by the library binaries. (instead of libxen$(LIBNAME).map.tmp)
- Write temporary file in the same directory as the target (in case the
target is in a different directory)
- remove temporary file generated by the new rule, in case it isn't
removed by move-if-changed..
- use ?= to set version-script, this mean that version-script has now a
deferred expansion instead of immediate, hoping nothing break.
CC: Jan Beulich
tools/libs/libs.mk | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/tools/libs/libs.mk b/tools/libs/libs.mk
index 0e4b5e0bd0..ffb6c9f064 100644
--- a/tools/libs/libs.mk
+++ b/tools/libs/libs.mk
@@ -5,6 +5,7 @@
# MAJOR: major version of lib (Xen version if empty)
# MINOR: minor version of lib (0 if empty)
# version-script: Specify the name of a version script to the linker.
+# (If empty, a temporary one for unstable library is created)
LIBNAME := $(notdir $(CURDIR))
@@ -27,6 +28,8 @@ ifneq ($(nosharedlibs),y)
TARGETS += lib$(LIB_FILE_NAME).so
endif
+version-script ?= lib$(LIB_FILE_NAME).map.tmp
+
PKG_CONFIG ?= $(LIB_FILE_NAME).pc
PKG_CONFIG_NAME ?= Xen$(LIBNAME)
PKG_CONFIG_DESC ?= The $(PKG_CONFIG_NAME) library for Xen hypervisor
@@ -72,6 +75,10 @@ headers.lst: FORCE
@{ set -e; $(foreach h,$(LIBHEADERS),echo $(h);) } > $@.tmp
@$(call move-if-changed,$@.tmp,$@)
+lib$(LIB_FILE_NAME).map.tmp: FORCE
+ echo 'lib$(LIB_FILE_NAME)_$(MAJOR).$(MINOR) { global: *; };'
>$(@D)/.$(@F)
+ $(call move-if-changed,$(@D)/.$(@F),$@)
+
lib$(LIB_FILE_NAME).a: $(OBJS-y)
$(AR) rc $@ $^
@@ -81,7 +88,7 @@ lib$(LIB_FILE_NAME).so.$(MAJOR):
lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR)
$(SYMLINK_SHLIB) $< $@
lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR): $(PIC_OBJS) $(version-script)
- $(CC) $(LDFLAGS) $(PTHREAD_LDFLAGS) -Wl,$(SONAME_LDFLAG)
-Wl,lib$(LIB_FILE_NAME).so.$(MAJOR) -Wl,$(if
$(version-script),--version-script=$(version-script),--default-symver)
$(SHLIB_LDFLAGS) -o $@ $(PIC_OBJS) $(LDLIBS) $(APPEND_LDFLAGS)
+ $(CC) $(LDFLAGS) $(PTHREAD_LDFLAGS) -Wl,$(SONAME_LDFLAG)
-Wl,lib$(LIB_FILE_NAME).so.$(MAJOR) -Wl,--version-script=$(version-script)
$(SHLIB_LDFLAGS) -o $@ $(PIC_OBJS) $(LDLIBS) $(APPEND_LDFLAGS)
# If abi-dumper is available, write out the ABI analysis
ifneq ($(ABI_DUMPER),)
@@ -120,7 +127,7 @@ TAGS:
clean::
rm -rf $(TARGETS) *~ $(DEPS_RM) $(OBJS-y) $(PIC_OBJS)
rm -f lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR)
lib$(LIB_FILE_NAME).so.$(MAJOR)
- rm -f headers.chk headers.lst
+ rm -f headers.chk headers.lst lib*.map.tmp .*.tmp
.PHONY: distclean
distclean: clean
--
Anthony PERARD
Re: [PATCH] x86/Xen: drop leftover VM-assist uses
On 15/02/2023 11:27 am, Jan Beulich wrote:
> Both the 4Gb-segments and the PAE-extended-CR3 one are applicable to
> 32-bit guests only. The PAE-extended-CR3 use, furthermore, was redundant
> with the PAE_MODE ELF note anyway.
>
> Signed-off-by: Jan Beulich
>
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -934,12 +934,8 @@ void xen_enable_syscall(void)
>
> static void __init xen_pvmmu_arch_setup(void)
> {
> - HYPERVISOR_vm_assist(VMASST_CMD_enable, VMASST_TYPE_4gb_segments);
> HYPERVISOR_vm_assist(VMASST_CMD_enable,
> VMASST_TYPE_writable_pagetables);
I find it disappointing that a PV guest which states a hard dependency
on writeable pagetables in its elfnotes doesn't have this activated
automatically.
The PV API/ABI truly is an undesigned mess.
~Andrew
Re: [PATCH v3] x86/vlapic: call vmx_vlapic_msr_changed through an hvm_function callback
On 16/02/2023 1:36 pm, Xenia Ragiadakou wrote:
> Hi Andrew,
>
> On 2/16/23 12:28, Andrew Cooper wrote:
>> On 13/02/2023 11:50 am, Xenia Ragiadakou wrote:
>>> diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
>>> b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
>>> index 234da4a7f4..97d6b810ec 100644
>>> --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
>>> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
>>> @@ -85,7 +85,7 @@ typedef enum {
>>> void vmx_asm_vmexit_handler(struct cpu_user_regs);
>>> void vmx_intr_assist(void);
>>> void noreturn cf_check vmx_do_resume(void);
>>> -void vmx_vlapic_msr_changed(struct vcpu *v);
>>> +void cf_check vmx_vlapic_msr_changed(struct vcpu *v);
>>
>> Hi,
>>
>> I see this patch has been committed, but this public declaration should
>> deleted, and vmx_vlapic_msr_changed() made static now that it's only
>> referenced in vmx.c.
>
> It is also used in vmcs.c
Oh, so it is. It just doesn't show up on the patch diff.
That use likely won't survive fixing the Intel APIC-V logic, but I guess
we're stuck with it for now.
Sorry for the noise.
>
>>
>> It needs a forward declaration in vmx.c because of its position relative
>> to the vmx_function_table, but that's fine - we've got plenty of other
>> examples like this.
>>
>> Could I talk you into doing an incremental fix?
>>
>> Alternatively, we could get better cleanup by forward declaring just
>> {vmx,svm}_function_table, then moving the tables to the very bottom of
>> {vmx,svm}.c at which point we can drop all the forward declarations.
>>
>> Oh top of that, I suspect we have other public function definitions
>> which can turn static, if you happen to spot any while doing this.
>
> Sure I could try to cleanup {svm,vmx}.c and the corresponding headers.
Thankyou.
~Andrew
Re: [PATCH v3] x86/vlapic: call vmx_vlapic_msr_changed through an hvm_function callback
Hi Andrew,
On 2/16/23 12:28, Andrew Cooper wrote:
On 13/02/2023 11:50 am, Xenia Ragiadakou wrote:
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
index 234da4a7f4..97d6b810ec 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
@@ -85,7 +85,7 @@ typedef enum {
void vmx_asm_vmexit_handler(struct cpu_user_regs);
void vmx_intr_assist(void);
void noreturn cf_check vmx_do_resume(void);
-void vmx_vlapic_msr_changed(struct vcpu *v);
+void cf_check vmx_vlapic_msr_changed(struct vcpu *v);
Hi,
I see this patch has been committed, but this public declaration should
deleted, and vmx_vlapic_msr_changed() made static now that it's only
referenced in vmx.c.
It is also used in vmcs.c
It needs a forward declaration in vmx.c because of its position relative
to the vmx_function_table, but that's fine - we've got plenty of other
examples like this.
Could I talk you into doing an incremental fix?
Alternatively, we could get better cleanup by forward declaring just
{vmx,svm}_function_table, then moving the tables to the very bottom of
{vmx,svm}.c at which point we can drop all the forward declarations.
Oh top of that, I suspect we have other public function definitions
which can turn static, if you happen to spot any while doing this.
Sure I could try to cleanup {svm,vmx}.c and the corresponding headers.
Thanks,
~Andrew
--
Xenia
Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On Thu, Feb 16, 2023 at 3:31 AM Jan Beulich wrote: > > On 15.02.2023 17:29, Tamas K Lengyel wrote: > > On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich wrote: > >> Did you consider the alternative of adjusting the ASSERT() in question > >> instead? It could reasonably become > >> > >> #ifdef CONFIG_MEM_SHARING > >> ASSERT(!p2m_is_hostp2m(p2m) || !remove_root || > > !atomic_read(&d->shr_pages)); > >> #endif > >> > >> now, I think. That would be less intrusive a change (helpful for > >> backporting), but there may be other (so far unnamed) benefits of pulling > >> ahead the shared memory teardown. > > > > I have a hard time understanding this proposed ASSERT. > > It accounts for the various ways p2m_teardown() can (now) be called, > limiting the actual check for no remaining shared pages to the last > of all these invocations (on the host p2m with remove_root=true). > > Maybe > > /* Limit the check to the final invocation. */ > if ( p2m_is_hostp2m(p2m) && remove_root ) > ASSERT(!atomic_read(&d->shr_pages)); > > would make this easier to follow? Another option might be to move > the assertion to paging_final_teardown(), ahead of that specific call > to p2m_teardown(). AFAICT d->shr_pages would still be more then 0 when this is called before sharing is torn down so the rearrangement is necessary even if we do this assert only on the final invocation. I did a printk in place of this assert without the rearrangement and afaict it was always != 0. Tamas
Re: [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
On Thu, 2023-02-16 at 10:48 +, Andrew Cooper wrote: > On 16/02/2023 7:31 am, Jan Beulich wrote: > > On 15.02.2023 18:59, Oleksii wrote: > > > Hello Jan and community, > > > > > > I experimented and switched RISC-V to x86 implementation. All > > > that I > > > changed in x86 implementation for RISC-V was _ASM_BUGFRAME_TEXT. > > > Other > > > things are the same as for x86. > > > > > > For RISC-V it is fine to skip '%c' modifier so _ASM_BUGFRAME_TEXT > > > will > > > look like: > > > > > > #define _ASM_BUGFRAME_TEXT(second_frame) \ > > > ".Lbug%=: ebreak\n" > > > ".pushsection .bug_frames.%[bf_type], \"a\", @progbits\n" > > > ".p2align 2\n" > > > ".Lfrm%=:\n" > > > ".long (.Lbug%= - .Lfrm%=) + %[bf_line_hi]\n" > > > ".long (%[bf_ptr] - .Lfrm%=) + %[bf_line_lo]\n" > > > ".if " #second_frame "\n" > > > ".long 0, %[bf_msg] - .Lfrm%=\n" > > > ".endif\n" > > > ".popsection\n" > > I expect this could be further abstracted such that only the actual > > instruction is arch-specific. > > > > > The only thing I am worried about is: > > > > > > #define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \ > > > [bf_type] "i" (type), ... > > > because as I understand it can be an issue with 'i' modifier in > > > case of > > > PIE. I am not sure that Xen enables PIE somewhere but still... > > > If it is not an issue then we can use x86 implementation as a > > > generic > > > one. > > "i" is not generally an issue with PIE, it only is when the value > > is the > > address of a symbol. Here "type" is a constant in all cases. (Or > > else > > how would you express an immediate operand of an instruction in an > > asm()?) > > At a guess, the problem isn't type. It's the line number, which ends > up > in a relocation. Sure. I missed that. > > That said, I'm not sure an architecture could reasonably function > without a generic 4-byte PC-relative relocation... > > ~Andrew
Re: [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
On 16/02/2023 12:09 pm, Oleksii wrote:
> On Thu, 2023-02-16 at 12:44 +0200, Oleksii wrote:
>> On Thu, 2023-02-16 at 08:31 +0100, Jan Beulich wrote:
>>> On 15.02.2023 18:59, Oleksii wrote:
Hello Jan and community,
I experimented and switched RISC-V to x86 implementation. All
that
I
changed in x86 implementation for RISC-V was _ASM_BUGFRAME_TEXT.
Other
things are the same as for x86.
For RISC-V it is fine to skip '%c' modifier so _ASM_BUGFRAME_TEXT
will
look like:
#define _ASM_BUGFRAME_TEXT(second_frame) \
".Lbug%=: ebreak\n"
".pushsection .bug_frames.%[bf_type], \"a\", @progbits\n"
".p2align 2\n"
".Lfrm%=:\n"
".long (.Lbug%= - .Lfrm%=) + %[bf_line_hi]\n"
".long (%[bf_ptr] - .Lfrm%=) + %[bf_line_lo]\n"
".if " #second_frame "\n"
".long 0, %[bf_msg] - .Lfrm%=\n"
".endif\n"
".popsection\n"
>>> I expect this could be further abstracted such that only the actual
>>> instruction is arch-specific.
>> Generally, the only thing that can't be abstracted ( as you mentioned
>> is an instruction ).
>>
>> So we can make additional defines:
>> 1. #define MODIFIER "" or "c" (depends on architecture) and use it
>>
>> the following way:
>> ".pushsection .bug_frames.%"MODIFIER"[bf_type], \"a\",
>> @progbits\n"
>> ...
>> 2. #define BUG_INSTR "ebreak" | "ud2" | "..." and use it in the
>> following way:
>> ".Lbug%=: "BUG_ISNTR"\n"
>> ...
>> Except for these defines which should be specified for each
>> architecture
>> all other code will be the same for all architectures.
>>
>> But as I understand with modifier 'c' can be issues that not all
>> compilers support but for ARM and x86 before immediate is present
>> punctuation # or $ which are removed by modifier 'c'. And I am
>> wondering what other ways to remove punctuation before immediate
>> exist.
>>
>> Do you think we should consider that as an issue?
>>
The only thing I am worried about is:
#define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \
[bf_type] "i" (type), ...
because as I understand it can be an issue with 'i' modifier in
case of
PIE. I am not sure that Xen enables PIE somewhere but still...
If it is not an issue then we can use x86 implementation as a
generic
one.
>>> "i" is not generally an issue with PIE, it only is when the value
>>> is
>>> the
>>> address of a symbol. Here "type" is a constant in all cases. (Or
>>> else
>>> how would you express an immediate operand of an instruction in an
>>> asm()?)
>> Hmm. I don't know other ways to express an immediate operand except
>> 'i'.
>>
>> It looks like type, line, msg are always 'constants'
>> (
>> they possibly can be passed without 'i' and used directly inside
>> asm():
>> ...
>> ".pushsection .bug_frames." __stringify(type) ", \"a\",
>> %progbits\n"\
>> ...
>> ) but the issue will be with 'ptr' for which we have to use 'i'.
>>
>> And I am not sure about all 'constants'. For example, I think that it
>> can be an issue with 'line' = '((line & ((1 << BUG_LINE_LO_WIDTH) -
>> 1))
>> << BUG_DISP_WIDTH)' if we will use that directly inside asm(...).
>>
> I think we can avoid 'i' by using 'r' + some kind of mov instruction to
> write a register value to memory. The code below is pseudo-code:
> #define _ASM_BUGFRAME_TEXT(second_frame)
> ...
> "loc_disp:\n"
> ".long 0x0"
> "mov [loc_disp], some_register"
> ...
> And the we have to define mov for each architecture.
No, you really cannot. This is the asm equivalent of writing something
like this:
static struct bugframe __section(.bug_frames.1) foo = {
.loc = insn - &foo + LINE_LO,
.msg = "bug string" - &foo + LINE_HI,
};
It is a data structure, not executable code.
~Andrew
[xen-unstable-smoke test] 177483: tolerable trouble: pass/starved - PUSHED
flight 177483 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/177483/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl 1 build-check(1) starved n/a build-armhf 2 hosts-allocate starved n/a version targeted for testing: xen 2e52dcc853a7183784cd9bdfb1e78ff366035209 baseline version: xen 91c45cfbab5d9878c0a7021f762988a688d5e91d Last test of basis 177433 2023-02-15 23:00:26 Z0 days Testing same since 177483 2023-02-16 10:00:26 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Jan Beulich Xenia Ragiadakou jobs: build-arm64-xsm pass build-amd64 pass build-armhf starved build-amd64-libvirt pass test-armhf-armhf-xl starved test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 91c45cfbab..2e52dcc853 2e52dcc853a7183784cd9bdfb1e78ff366035209 -> smoke
Re: [PATCH 6/6] common: __u8 is history
On 09/02/2023 10:43 am, Jan Beulich wrote: > With the last uses gone, move the type to linux-compat.h. > > No functional change intended. > > Signed-off-by: Jan Beulich Acked-by: Andrew Cooper
Re: [PATCH 5/6] x86: use standard C types in struct cpuinfo_x86
On 09/02/2023 10:42 am, Jan Beulich wrote:
> Consolidate this to use exclusively standard types, and change oprofile
> code (apparently trying to mirror those types) at the same time. Where
> sensible actually drop local variables.
>
> No functional change intended.
>
> Signed-off-by: Jan Beulich
> ---
> Much like x86_capability[] already doesn't use a fixed width type, most
> if not all of the other fields touched here probably also shouldn't. I
> wasn't sure though whether switching might be controversial for some of
> them ...
x86_capability isn't an inherently 32bit structure. It's a bitmap, and
all of Xen's bitmap operations take unsigned int *.
Most other things in this structure don't need to have specific widths
IMO, but there is huge quantity of junk here.
> --- a/xen/arch/x86/include/asm/processor.h
> +++ b/xen/arch/x86/include/asm/processor.h
> @@ -119,24 +119,24 @@ struct x86_cpu_id {
> };
>
> struct cpuinfo_x86 {
> -__u8 x86;/* CPU family */
> -__u8 x86_vendor; /* CPU vendor */
> -__u8 x86_model;
> -__u8 x86_mask;
> +uint8_t x86;/* CPU family */
> +uint8_t x86_vendor; /* CPU vendor */
> +uint8_t x86_model;
> +uint8_t x86_mask;
These specific names always irritated me. They should be vendor,
family, model, stepping and probably in that order. The x86 prefix is
entirely redundant.
> int cpuid_level;/* Maximum supported CPUID level, -1=no CPUID */
There's no such thing a "no CPUID" cpu for Xen any more.
> -__u32 extended_cpuid_level; /* Maximum supported CPUID extended level */
> +uint32_t extended_cpuid_level; /* Maximum supported CPUID extended level
> */
This does need to be this wide, but I really regret the name being this
wide...
> unsigned int x86_capability[NCAPINTS];
> char x86_vendor_id[16];
> char x86_model_id[64];
These arrays should be 12 and 48 respectively, but the vendor id is
redundant with the vendor field.
Furthermore, we do some non-trivial string rearranging of the string,
and (seeing as you rejected my patch to print it on boot) only ever gets
used to hand to dom0 in a machine check.
> int x86_cache_size; /* in KB - valid for CPUS which support this call
> */
> int x86_cache_alignment;/* In bytes */
The only interesting thing I can see about this field is that the
Netburst quirk is wrong. double-pumped IO was a firmware setting
because it was a tradeoff and different workloads favoured different
settings.
> -__u32 x86_max_cores; /* cpuid returned max cores value */
> -__u32 booted_cores; /* number of cores as seen by OS */
> -__u32 x86_num_siblings; /* cpuid logical cpus per chip value */
> -__u32 apicid;
> -__u32 phys_proc_id;/* package ID of each logical CPU */
> -__u32 cpu_core_id; /* core ID of each logical CPU*/
> -__u32 compute_unit_id; /* AMD compute unit ID of each logical CPU */
> +uint32_t x86_max_cores; /* cpuid returned max cores value */
> +uint32_t booted_cores;/* number of cores as seen by OS */
> +uint32_t x86_num_siblings; /* cpuid logical cpus per chip value */
> +uint32_t apicid;
> +uint32_t phys_proc_id;/* package ID of each logical CPU */
> +uint32_t cpu_core_id; /* core ID of each logical CPU */
> +uint32_t compute_unit_id; /* AMD compute unit ID of each logical CPU */
There's lots of redundancy here, and half of these fields can be derived
directly from the 32bit APIC ID.
For the purpose of the type cleanup, Acked-by: Andrew Cooper
.
~Andrew
Re: [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
On Thu, 2023-02-16 at 12:44 +0200, Oleksii wrote: > On Thu, 2023-02-16 at 08:31 +0100, Jan Beulich wrote: > > On 15.02.2023 18:59, Oleksii wrote: > > > Hello Jan and community, > > > > > > I experimented and switched RISC-V to x86 implementation. All > > > that > > > I > > > changed in x86 implementation for RISC-V was _ASM_BUGFRAME_TEXT. > > > Other > > > things are the same as for x86. > > > > > > For RISC-V it is fine to skip '%c' modifier so _ASM_BUGFRAME_TEXT > > > will > > > look like: > > > > > > #define _ASM_BUGFRAME_TEXT(second_frame) \ > > > ".Lbug%=: ebreak\n" > > > ".pushsection .bug_frames.%[bf_type], \"a\", @progbits\n" > > > ".p2align 2\n" > > > ".Lfrm%=:\n" > > > ".long (.Lbug%= - .Lfrm%=) + %[bf_line_hi]\n" > > > ".long (%[bf_ptr] - .Lfrm%=) + %[bf_line_lo]\n" > > > ".if " #second_frame "\n" > > > ".long 0, %[bf_msg] - .Lfrm%=\n" > > > ".endif\n" > > > ".popsection\n" > > > > I expect this could be further abstracted such that only the actual > > instruction is arch-specific. > Generally, the only thing that can't be abstracted ( as you mentioned > is an instruction ). > > So we can make additional defines: > 1. #define MODIFIER "" or "c" (depends on architecture) and use it > > the following way: > ".pushsection .bug_frames.%"MODIFIER"[bf_type], \"a\", > @progbits\n" > ... > 2. #define BUG_INSTR "ebreak" | "ud2" | "..." and use it in the > following way: > ".Lbug%=: "BUG_ISNTR"\n" > ... > Except for these defines which should be specified for each > architecture > all other code will be the same for all architectures. > > But as I understand with modifier 'c' can be issues that not all > compilers support but for ARM and x86 before immediate is present > punctuation # or $ which are removed by modifier 'c'. And I am > wondering what other ways to remove punctuation before immediate > exist. > > Do you think we should consider that as an issue? > > > > > > The only thing I am worried about is: > > > > > > #define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \ > > > [bf_type] "i" (type), ... > > > because as I understand it can be an issue with 'i' modifier in > > > case of > > > PIE. I am not sure that Xen enables PIE somewhere but still... > > > If it is not an issue then we can use x86 implementation as a > > > generic > > > one. > > > > "i" is not generally an issue with PIE, it only is when the value > > is > > the > > address of a symbol. Here "type" is a constant in all cases. (Or > > else > > how would you express an immediate operand of an instruction in an > > asm()?) > Hmm. I don't know other ways to express an immediate operand except > 'i'. > > It looks like type, line, msg are always 'constants' > ( > they possibly can be passed without 'i' and used directly inside > asm(): > ... > ".pushsection .bug_frames." __stringify(type) ", \"a\", > %progbits\n"\ > ... > ) but the issue will be with 'ptr' for which we have to use 'i'. > > And I am not sure about all 'constants'. For example, I think that it > can be an issue with 'line' = '((line & ((1 << BUG_LINE_LO_WIDTH) - > 1)) > << BUG_DISP_WIDTH)' if we will use that directly inside asm(...). > I think we can avoid 'i' by using 'r' + some kind of mov instruction to write a register value to memory. The code below is pseudo-code: #define _ASM_BUGFRAME_TEXT(second_frame) ... "loc_disp:\n" ".long 0x0" "mov [loc_disp], some_register" ... And the we have to define mov for each architecture. \ > > > > Jan > > ~ Oleksii
Re: [Discussion] Xen grants and access permissions
On 16/02/2023 11:13 am, Viresh Kumar wrote: > Hi Oleksandr, > > As you already know, I am looking at how we can integrate the Xen > grants work in our implementation of Rust based Xen vhost frontend [1]. > > The hypervisor independent vhost-user backends [2] talk to > xen-vhost-frontend using the standard vhost-user protocol [3]. Every > memory region that the backends get access to are sent to it by the > frontend as memory region descriptors, which contain only address and > size information and lack any permission flags. > > I noticed that with Xen grants, there are strict memory access > restrictions, where a memory region may be marked READ only and we > can't map it as RW anymore, trying that just fails. Because the > standard vhost-user protocol doesn't have any permission flags, the > vhost libraries (in Rust) can't do anything else but try to map > everything as RW. > > I am wondering how do I proceed on this as I am very much stuck here. > (unhelpful comment) This is what happens when people try to reinvent the wheel a little more square than it was before. If the guest grants the page read-only, then you can only map it read only. Anything else is a violation of the security model. So either you need to adjust the guest to always grant read/write, or you need to teach virtio that read only is actually a real concept. ~Andrew
Re: [XEN PATCH 3/4] automation: Remove expired root certificates used to be used by let's encrypt
On Wed, Feb 15, 2023 at 04:14:53PM -0800, Stefano Stabellini wrote: > On Wed, 15 Feb 2023, Andrew Cooper wrote: > > Honestly, I think I'd prefer to drop all of these legacy versions... > > Good timing! It just so happens that we need to shave some of the old > container tests as we have too many build tests on x86 :-) > > I would remove Jessie as it reached EOL years ago. Do we really need > both Centos 7 and 7.2? If not, we could remove 7. Actually, 7.2 is older than 7, so I would remove 7.2. (7 would be 7.x so latest 7 which is 7.9.) > That leaves us with Trusty and Centos 7.2 among these. I would be > tempted to keep Trusty and add the sed hack of this patch to make it > work. For Centos 7.2, the hack looks even worse. Would it solve the > problem to upgrade to the latest Centos 7.x subrelease? Is there really > no other way to solve the problem? So for centos7, the blacklist of the expired root certificate isn't needed if we simply run `yum update` which for some reason is missing from the dockerfile... Thanks, -- Anthony PERARD
[Discussion] Xen grants and access permissions
Hi Oleksandr, As you already know, I am looking at how we can integrate the Xen grants work in our implementation of Rust based Xen vhost frontend [1]. The hypervisor independent vhost-user backends [2] talk to xen-vhost-frontend using the standard vhost-user protocol [3]. Every memory region that the backends get access to are sent to it by the frontend as memory region descriptors, which contain only address and size information and lack any permission flags. I noticed that with Xen grants, there are strict memory access restrictions, where a memory region may be marked READ only and we can't map it as RW anymore, trying that just fails. Because the standard vhost-user protocol doesn't have any permission flags, the vhost libraries (in Rust) can't do anything else but try to map everything as RW. I am wondering how do I proceed on this as I am very much stuck here. -- viresh [1] https://github.com/vireshk/xen-vhost-frontend [2] https://github.com/rust-vmm/vhost-device [3] https://qemu.readthedocs.io/en/latest/interop/vhost-user.html [4] https://qemu.readthedocs.io/en/latest/interop/vhost-user.html#memory-regions-description
Re: [PATCH 4/6] x86/MSI: use standard C types in structures/unions
On 09/02/2023 10:39 am, Jan Beulich wrote:
> Consolidate this to use exclusively standard types, and change
> indentation style to Xen's there at the same time (the file already had
> a mix of styles).
>
> No functional change intended.
>
> Signed-off-by: Jan Beulich
So I suppose Acked-by: Andrew Cooper because
this is an improvement on the status quo, but I have quite a few requests.
> ---
> For most (all?) of the single bit I was on the edge of switching them to
> bool - thoughts?
Yes.
>
> --- a/xen/arch/x86/include/asm/msi.h
> +++ b/xen/arch/x86/include/asm/msi.h
> @@ -66,15 +66,15 @@ struct msi_info {
> };
>
> struct msi_msg {
> - union {
> - u64 address; /* message address */
> - struct {
> - u32 address_lo; /* message address low 32 bits */
> - u32 address_hi; /* message address high 32 bits */
> - };
> - };
> - u32 data; /* 16 bits of msi message data */
> - u32 dest32; /* used when Interrupt Remapping with EIM is
> enabled */
> +union {
> +uint64_t address; /* message address */
> +struct {
> +uint32_t address_lo; /* message address low 32 bits */
> +uint32_t address_hi; /* message address high 32 bits */
> +};
> +};
> +uint32_t data;/* 16 bits of msi message data */
> +uint32_t dest32; /* used when Interrupt Remapping with EIM is
> enabled */
The 16 is actively wrong for data, but honestly it's only this dest32
comment which has any value whatsoever (when it has been de-Intel'd).
I'd correct dest32 to reference the AMD too, and delete the rest.
> };
>
> struct irq_desc;
> @@ -94,35 +94,35 @@ extern int pci_restore_msi_state(struct
> extern int pci_reset_msix_state(struct pci_dev *pdev);
>
> struct msi_desc {
> - struct msi_attrib {
> - __u8type; /* {0: unused, 5h:MSI, 11h:MSI-X} */
> - __u8pos;/* Location of the MSI capability */
> - __u8maskbit : 1;/* mask/pending bit supported ? */
> - __u8is_64 : 1;/* Address size: 0=32bit 1=64bit */
> - __u8host_masked : 1;
> - __u8guest_masked : 1;
> - __u16 entry_nr; /* specific enabled entry */
> - } msi_attrib;
> -
> - bool irte_initialized;
> - uint8_t gvec; /* guest vector. valid when pi_desc
> isn't NULL */
> - const struct pi_desc *pi_desc; /* pointer to posted descriptor */
> -
> - struct list_head list;
> -
> - union {
> - void __iomem *mask_base;/* va for the entry in mask table */
> - struct {
> - unsigned int nvec;/* number of vectors*/
> - unsigned int mpos;/* location of mask register*/
> - } msi;
> - unsigned int hpet_id; /* HPET (dev is NULL) */
> - };
> - struct pci_dev *dev;
> - int irq;
> - int remap_index;/* index in interrupt remapping table */
> +struct msi_attrib {
> +uint8_t type;/* {0: unused, 5h:MSI, 11h:MSI-X} */
> +uint8_t pos; /* Location of the MSI capability */
> +uint8_t maskbit : 1; /* mask/pending bit supported ? */
> +uint8_t is_64: 1; /* Address size: 0=32bit 1=64bit */
> +uint8_t host_masked : 1;
> +uint8_t guest_masked : 1;
> +uint16_t entry_nr; /* specific enabled entry */
entry_nr wants to move up to between pos and maskbit, and then we shrink
the total structure by 8 bytes (I think).
> +} msi_attrib;
> +
> +bool irte_initialized;
> +uint8_t gvec;/* guest vector. valid when pi_desc isn't NULL
> */
> +const struct pi_desc *pi_desc; /* pointer to posted descriptor */
> +
> +struct list_head list;
> +
> +union {
> +void __iomem *mask_base; /* va for the entry in mask table */
> +struct {
> +unsigned int nvec; /* number of vectors */
> +unsigned int mpos; /* location of mask register */
> +} msi;
> +unsigned int hpet_id; /* HPET (dev is NULL) */
> +};
> +struct pci_dev *dev;
> +int irq;
> +int remap_index; /* index in interrupt remapping table */
>
> - struct msi_msg msg; /* Last set MSI message */
> +struct msi_msg msg; /* Last set MSI message */
> };
>
> /*
> @@ -180,48 +180,48 @@ int msi_free_irq(struct msi_desc *entry)
>
> struct __packed msg_data {
> #if defined(__LITTLE_ENDIAN_BITFIELD)
There's no such thing as a big endian x86 bitfield. Just delete this
ifdefary to simplify the result.
Additionally, the structure doesn't need to be packed - its a single
uint32_t's worth of bitfield.
Finally, can we drop the reserved fields and leave them as anonymous
bitfields?
> - __u32 vector
Re: [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
On 16/02/2023 7:31 am, Jan Beulich wrote: > On 15.02.2023 18:59, Oleksii wrote: >> Hello Jan and community, >> >> I experimented and switched RISC-V to x86 implementation. All that I >> changed in x86 implementation for RISC-V was _ASM_BUGFRAME_TEXT. Other >> things are the same as for x86. >> >> For RISC-V it is fine to skip '%c' modifier so _ASM_BUGFRAME_TEXT will >> look like: >> >> #define _ASM_BUGFRAME_TEXT(second_frame) \ >> ".Lbug%=: ebreak\n" >> ".pushsection .bug_frames.%[bf_type], \"a\", @progbits\n" >> ".p2align 2\n" >> ".Lfrm%=:\n" >> ".long (.Lbug%= - .Lfrm%=) + %[bf_line_hi]\n" >> ".long (%[bf_ptr] - .Lfrm%=) + %[bf_line_lo]\n" >> ".if " #second_frame "\n" >> ".long 0, %[bf_msg] - .Lfrm%=\n" >> ".endif\n" >> ".popsection\n" > I expect this could be further abstracted such that only the actual > instruction is arch-specific. > >> The only thing I am worried about is: >> >> #define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \ >> [bf_type] "i" (type), ... >> because as I understand it can be an issue with 'i' modifier in case of >> PIE. I am not sure that Xen enables PIE somewhere but still... >> If it is not an issue then we can use x86 implementation as a generic >> one. > "i" is not generally an issue with PIE, it only is when the value is the > address of a symbol. Here "type" is a constant in all cases. (Or else > how would you express an immediate operand of an instruction in an > asm()?) At a guess, the problem isn't type. It's the line number, which ends up in a relocation. That said, I'm not sure an architecture could reasonably function without a generic 4-byte PC-relative relocation... ~Andrew
Re: [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
On Thu, 2023-02-16 at 08:31 +0100, Jan Beulich wrote: > On 15.02.2023 18:59, Oleksii wrote: > > Hello Jan and community, > > > > I experimented and switched RISC-V to x86 implementation. All that > > I > > changed in x86 implementation for RISC-V was _ASM_BUGFRAME_TEXT. > > Other > > things are the same as for x86. > > > > For RISC-V it is fine to skip '%c' modifier so _ASM_BUGFRAME_TEXT > > will > > look like: > > > > #define _ASM_BUGFRAME_TEXT(second_frame) \ > > ".Lbug%=: ebreak\n" > > ".pushsection .bug_frames.%[bf_type], \"a\", @progbits\n" > > ".p2align 2\n" > > ".Lfrm%=:\n" > > ".long (.Lbug%= - .Lfrm%=) + %[bf_line_hi]\n" > > ".long (%[bf_ptr] - .Lfrm%=) + %[bf_line_lo]\n" > > ".if " #second_frame "\n" > > ".long 0, %[bf_msg] - .Lfrm%=\n" > > ".endif\n" > > ".popsection\n" > > I expect this could be further abstracted such that only the actual > instruction is arch-specific. Generally, the only thing that can't be abstracted ( as you mentioned is an instruction ). So we can make additional defines: 1. #define MODIFIER "" or "c" (depends on architecture) and use it the following way: ".pushsection .bug_frames.%"MODIFIER"[bf_type], \"a\", @progbits\n" ... 2. #define BUG_INSTR "ebreak" | "ud2" | "..." and use it in the following way: ".Lbug%=: "BUG_ISNTR"\n" ... Except for these defines which should be specified for each architecture all other code will be the same for all architectures. But as I understand with modifier 'c' can be issues that not all compilers support but for ARM and x86 before immediate is present punctuation # or $ which are removed by modifier 'c'. And I am wondering what other ways to remove punctuation before immediate exist. Do you think we should consider that as an issue? > > > The only thing I am worried about is: > > > > #define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \ > > [bf_type] "i" (type), ... > > because as I understand it can be an issue with 'i' modifier in > > case of > > PIE. I am not sure that Xen enables PIE somewhere but still... > > If it is not an issue then we can use x86 implementation as a > > generic > > one. > > "i" is not generally an issue with PIE, it only is when the value is > the > address of a symbol. Here "type" is a constant in all cases. (Or else > how would you express an immediate operand of an instruction in an > asm()?) Hmm. I don't know other ways to express an immediate operand except 'i'. It looks like type, line, msg are always 'constants' ( they possibly can be passed without 'i' and used directly inside asm(): ... ".pushsection .bug_frames." __stringify(type) ", \"a\", %progbits\n"\ ... ) but the issue will be with 'ptr' for which we have to use 'i'. And I am not sure about all 'constants'. For example, I think that it can be an issue with 'line' = '((line & ((1 << BUG_LINE_LO_WIDTH) - 1)) << BUG_DISP_WIDTH)' if we will use that directly inside asm(...). > > Jan ~ Oleksii
Re: [PATCH v3] x86/vlapic: call vmx_vlapic_msr_changed through an hvm_function callback
On 13/02/2023 11:50 am, Xenia Ragiadakou wrote:
> diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> index 234da4a7f4..97d6b810ec 100644
> --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> @@ -85,7 +85,7 @@ typedef enum {
> void vmx_asm_vmexit_handler(struct cpu_user_regs);
> void vmx_intr_assist(void);
> void noreturn cf_check vmx_do_resume(void);
> -void vmx_vlapic_msr_changed(struct vcpu *v);
> +void cf_check vmx_vlapic_msr_changed(struct vcpu *v);
Hi,
I see this patch has been committed, but this public declaration should
deleted, and vmx_vlapic_msr_changed() made static now that it's only
referenced in vmx.c.
It needs a forward declaration in vmx.c because of its position relative
to the vmx_function_table, but that's fine - we've got plenty of other
examples like this.
Could I talk you into doing an incremental fix?
Alternatively, we could get better cleanup by forward declaring just
{vmx,svm}_function_table, then moving the tables to the very bottom of
{vmx,svm}.c at which point we can drop all the forward declarations.
Oh top of that, I suspect we have other public function definitions
which can turn static, if you happen to spot any while doing this.
Thanks,
~Andrew
[xen-unstable test] 177428: tolerable trouble: fail/pass/starved - PUSHED
flight 177428 xen-unstable real [real] flight 177479 xen-unstable real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/177428/ http://logs.test-lab.xenproject.org/osstest/logs/177479/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-i386-examine 6 xen-install fail pass in 177479-retest test-amd64-amd64-libvirt-vhd 19 guest-start/debian.repeat fail pass in 177479-retest Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 177376 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 177376 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 177376 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 177376 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 177376 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 177376 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 177376 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 177376 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 177376 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass build-armhf-libvirt 1 build-check(1) starved n/a test-armhf-armhf-examine 1 build-check(1) starved n/a test-armhf-armhf-libvirt 1 build-check(1) starved n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) starved n/a test-armhf-armhf-libvirt-raw 1 build-check(1) starved n/a test-armhf-armhf-xl 1 build-check(1) starved n/a test-armhf-armhf-xl-credit1 1 build-check(1) starved n/a test-armhf-armhf-xl-credit2 1 build-check(1) starved n/a test-armhf-armhf-xl-cubietruck 1 build-check(1) starved n/a test-armhf-armhf-xl-multivcpu 1 build-check(1) starved n/a test-armhf-armhf-xl-rtds 1 build-check(1) starved n/a test-armhf-armhf-xl-vhd 1 build-check(1) starved n/a build-armhf 2 hosts-allocate starved n/a version targeted for testing: xen 5b9bb91abba7c983def3b4bef71ab08ad360a242 baseline version: xen 63305e5392ec2d17b85e7996a97462744425db80 Last test of basis 177376 2023-02-15 10:37:05 Z0 days Testing same since 177428 2023-02-15 22:10:26 Z0 days1 attempts People who touched revisions under test: Marek Marczykowski-Górecki Ross Lagerwall jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64-xtf pass build-amd
Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
> On 16 Feb 2023, at 08:19, Jan Beulich wrote:
>
> On 16.02.2023 00:49, Stefano Stabellini wrote:
>> On Wed, 15 Feb 2023, Julien Grall wrote:
>>> On 14/02/2023 22:25, Stefano Stabellini wrote:
> Patch 1's example has a "comment" field, which no entry makes use of.
> Without that, how does it become clear _why_ a particular file is to
> be excluded? The patch description here also doesn't provide any
> justification ...
It would be good to have a couple of pre-canned justifications as the
reason for excluding one file could be different from the reason for
excluding another file. Some of the reasons:
>>>
>>> I think the reasons should be ambiguous. This is ...
>>>
- imported from Linux
>>>
>>> ... the case here but...
>>>
>>> This reason is pretty clear to me but...
>>>
- too many false positives
>>>
>>> ... not here. What is too many?
>>>
That said, we don't necessarily need to know the exact reason for
excluding one file to be able to start scanning xen with cppcheck
automatically. If someone wants to remove a file from the exclude list
in the future they just need to show that cppcheck does a good job at
scanning the file and we can handle the number of violations.
>>>
>>> I disagree. A good reasoning from the start will be helpful to decide when
>>> we
>>> can remove a file from the list. Furthermore, we need to set good example
>>> for
>>> any new file we want to exclude.
>>>
>>> Furthermore, if we exclude some files, then it will be difficult for the
>>> reviewers to know when they can be removed from the list. What if this is
>>> fine
>>> with CPPCheck but not EClair (or any other)?
>>
>> Yes, the reason would help. In previous incarnations of this work, there
>> was a request for detailed information on external files, such as:
>> - where this file is coming from
>> - if coming from Linux, which version of Linux
>> - maintenance status
>> - coding style
>>
>> But this is not what you are asking. You are only asking for a reason
>> and "imported from Linux" would be good enough. Please correct me if I
>> am wrong.
>
> I guess you mean s/would/could/. Personally I find "imported from Linux"
> as an entirely unacceptable justification: Why would the origin of a file
> matter on whether it has violations? Dealing with the violations may be
> more cumbersome (because preferably the adjustments would go to the
> original files first). Yet not dealing with them - especially if there
> are many - reduces the benefit of the work we do quite a bit, because it
> may leave much more work for downstreams to do to actually be able to do
> any certification. That may go to the extent of questioning why we would
> bother dealing with a few dozen violations if hundreds remain but are
> hidden.
Hi Jan,
my personal opinion is that we can’t handle them for files that needs to be kept
in sync from an external source, because we can’t justify findings or tag false
positives, if we are lucky we might fix the non compliances but even in that
case
there might be times when the fix goes through and cases where the fix is
objected
by the maintainers just because their project is not following the MISRA
standard.
On our side, what we can do is track these files and from time to time check
that
they are still eligible to backport, because when they are not anymore we could
just port them to Xen coding style and start doing direct changes.
@Stefano/@Bertrand/@Julien thanks for your effort on the list, yesterday morning
I’ve also had a look on the Michal’s file list and I’ve tracked down the
origin, here
my findings, maybe we could merge your list with mine?
{
"version": "1.0",
"content": [
{
"rel_path": "arch/arm/arm64/cpufeature.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/arm/arm64/insn.c",
"comment": "Imported on Linux"
},
{
"rel_path": "arch/arm/arm64/lib/find_next_bit.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/x86/acpi/boot.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/x86/acpi/cpu_idle.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/x86/acpi/cpufreq/cpufreq.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/x86/acpi/cpuidle_menu.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/x86/acpi/lib.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/x86/cpu/amd.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/x86/cpu/centaur.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/x86/cpu/common.c",
"
Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On 15.02.2023 17:29, Tamas K Lengyel wrote: > On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich wrote: >> Did you consider the alternative of adjusting the ASSERT() in question >> instead? It could reasonably become >> >> #ifdef CONFIG_MEM_SHARING >> ASSERT(!p2m_is_hostp2m(p2m) || !remove_root || > !atomic_read(&d->shr_pages)); >> #endif >> >> now, I think. That would be less intrusive a change (helpful for >> backporting), but there may be other (so far unnamed) benefits of pulling >> ahead the shared memory teardown. > > I have a hard time understanding this proposed ASSERT. It accounts for the various ways p2m_teardown() can (now) be called, limiting the actual check for no remaining shared pages to the last of all these invocations (on the host p2m with remove_root=true). Maybe /* Limit the check to the final invocation. */ if ( p2m_is_hostp2m(p2m) && remove_root ) ASSERT(!atomic_read(&d->shr_pages)); would make this easier to follow? Another option might be to move the assertion to paging_final_teardown(), ahead of that specific call to p2m_teardown(). Jan
Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
On 16.02.2023 00:49, Stefano Stabellini wrote: > On Wed, 15 Feb 2023, Julien Grall wrote: >> On 14/02/2023 22:25, Stefano Stabellini wrote: Patch 1's example has a "comment" field, which no entry makes use of. Without that, how does it become clear _why_ a particular file is to be excluded? The patch description here also doesn't provide any justification ... >>> >>> It would be good to have a couple of pre-canned justifications as the >>> reason for excluding one file could be different from the reason for >>> excluding another file. Some of the reasons: >> >> I think the reasons should be ambiguous. This is ... >> >>> - imported from Linux >> >> ... the case here but... >> >> This reason is pretty clear to me but... >> >>> - too many false positives >> >> ... not here. What is too many? >> >>> That said, we don't necessarily need to know the exact reason for >>> excluding one file to be able to start scanning xen with cppcheck >>> automatically. If someone wants to remove a file from the exclude list >>> in the future they just need to show that cppcheck does a good job at >>> scanning the file and we can handle the number of violations. >> >> I disagree. A good reasoning from the start will be helpful to decide when we >> can remove a file from the list. Furthermore, we need to set good example for >> any new file we want to exclude. >> >> Furthermore, if we exclude some files, then it will be difficult for the >> reviewers to know when they can be removed from the list. What if this is >> fine >> with CPPCheck but not EClair (or any other)? > > Yes, the reason would help. In previous incarnations of this work, there > was a request for detailed information on external files, such as: > - where this file is coming from > - if coming from Linux, which version of Linux > - maintenance status > - coding style > > But this is not what you are asking. You are only asking for a reason > and "imported from Linux" would be good enough. Please correct me if I > am wrong. I guess you mean s/would/could/. Personally I find "imported from Linux" as an entirely unacceptable justification: Why would the origin of a file matter on whether it has violations? Dealing with the violations may be more cumbersome (because preferably the adjustments would go to the original files first). Yet not dealing with them - especially if there are many - reduces the benefit of the work we do quite a bit, because it may leave much more work for downstreams to do to actually be able to do any certification. That may go to the extent of questioning why we would bother dealing with a few dozen violations if hundreds remain but are hidden. Jan
[ovmf test] 177466: all pass - PUSHED
flight 177466 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/177466/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 5c551d6d912967ada3084033acea8acf37256043 baseline version: ovmf 38da9606f77842cdcdc231210c0369a6180c51a0 Last test of basis 177386 2023-02-15 12:42:35 Z0 days Testing same since 177466 2023-02-16 06:12:19 Z0 days1 attempts People who touched revisions under test: Abner Chang Ard Biesheuvel Jiewen Yao Ray Ni Sunil V L 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 38da9606f7..5c551d6d91 5c551d6d912967ada3084033acea8acf37256043 -> xen-tested-master
Re: [XEN PATCH] libs: Fix unstable libs build on FreeBSD, auto-generate version-script
On 15.02.2023 18:41, Anthony PERARD wrote: > On Wed, Feb 15, 2023 at 04:30:43PM +0100, Jan Beulich wrote: >> On 15.02.2023 16:21, Anthony PERARD wrote: >>> @@ -13,6 +14,10 @@ MAJOR := $(shell $(XEN_ROOT)/version.sh >>> $(XEN_ROOT)/xen/Makefile) >>> endif >>> MINOR ?= 0 >>> >>> +ifeq ($(origin version-script), undefined) >>> +version-script := libxen$(LIBNAME).map.tmp >>> +endif >> >> Such a use of $(origin ...) is pretty fragile. Maybe better use ?= ? > > I'm not sure why I used $(origin), I've written that more than 6 month > ago, but this way of using it is actually described in the manual, when > documenting ?= but with a = instead of := . > So, maybe I wanted to have ?= but with immediate evaluation rather than > deferred. Maybe I had issue with $(version-script) evaluating to > different values. What's wrong with using deferred evaluation for this macro? $(LIBNAME) (and $(LIB_FILE_NAME)) don't vary over time, do they? Plus ... >>> lib$(LIB_FILE_NAME).a: $(OBJS-y) >> >> Seeing this right adjacent in context - any reason you use libxen$(LIBNAME) >> and not the same lib$(LIB_FILE_NAME) for the base file name? > > That was the name used before, I guess it would be fine to rename. I > just need to set $(version-script) later as $(LIB_FILE_NAME) would not > be defined yet. ... you'd likely deal with that issue as well, unless there's a "too early" use of $(version-script) somewhere. Jan
