[linux-linus test] 182626: regressions - FAIL

[osstest service owner]

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

Regressions :-(

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

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

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 182531
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 182531
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 182531
 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  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 

Re: [PATCH] Arm: constrain {,u}int64_aligned_t in public header

[Henry Wang]

Hi Jan,

> On Sep 1, 2023, at 15:26, Jan Beulich  wrote:
> 
> This using a GNU extension, it may not be exposed in general, just like
> is done on x86. External consumers need to make this type available up
> front (just like we expect {,u}int_t to be supplied) - unlike on x86
> the type is actually needed outside of tools-only interfaces, because
> guest handle definitions use it.
> 
> While there also add underscores around "aligned".
> 
> Signed-off-by: Jan Beulich 
> 
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -152,8 +152,10 @@
> 
> #define XEN_HYPERCALL_TAG   0XEA1
> 
> -#define  int64_aligned_t  int64_t __attribute__((aligned(8)))
> -#define uint64_aligned_t uint64_t __attribute__((aligned(8)))
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +#define  int64_aligned_t  int64_t __attribute__((__aligned__(8)))
> +#define uint64_aligned_t uint64_t __attribute__((__aligned__(8)))
> +#endif

Today I tested this patch by our CI (on top of today’s staging), and it looks
like below error will happen for both arm32 and arm64 Yocto build:

The arm32 failure:
https://pastebin.com/raw/S7ZqmG6z

The arm64 failure:
https://pastebin.com/raw/HMFh5tuS

Kind regards,
Henry

> 
> #ifndef __ASSEMBLY__
> #define ___DEFINE_XEN_GUEST_HANDLE(name, type)  \
> 

Re: [PATCH 2/2] xen: Change parameter of generic_fls() to unsigned int

[Henry Wang]

Hi Jan and Michal,

> On Sep 4, 2023, at 23:13, Jan Beulich  wrote:
> 
> On 04.09.2023 16:03, Michal Orzel wrote:
>> On 04/09/2023 15:28, Jan Beulich wrote:
>>> On 04.09.2023 11:14, Michal Orzel wrote:
 --- a/xen/include/xen/bitops.h
 +++ b/xen/include/xen/bitops.h
 @@ -51,7 +51,7 @@ static inline int generic_ffs(int x)
  * fls: find last bit set.
  */
 
 -static __inline__ int generic_fls(int x)
 +static __inline__ int generic_fls(unsigned int x)
 {
 int r = 32;
 
>>> 
>>> Even if perhaps not affected by UBSAN, generic_ffs() then wants taking care
>>> of as well, imo. If additionally you switch __inline__ to inline, things
>>> will become nicely symmetric with generic_f{f,l}sl().
>> If others agree, I can switch generic_ffs() to take unsigned as well 
>> (together with s/__inline__/inline/).
>> However, such change would no longer fall in fixes category (i.e. nothing 
>> wrong with ffs() taking int):
>> - is it ok at this stage of release? (not sure but no problem for me to do 
>> this)
> 
> I'd say yes, but the release manager would have the ultimate say.

>From the release aspect, I am fine with doing the above-mentioned extra
changes, as we are not in the code freeze stage. From the development
point of view, I think whether doing such extra changes or not is supposed
to be an agreement between the developer and the maintainer.

Also this patch itself looks good to me so:

Reviewed-by: Henry Wang 

Kind regards,
Henry

> 
>> - is it ok to mix a fix with non-fix change? (I always prefer fixes to be 
>> done as standalone changes)
> 
> Imo it is, to avoid introducing an inconsistency. While such may
> not themselves be bugs, they increase the risk of introducing some.
> 
> Jan
> 

[linux-linus test] 182625: regressions - FAIL

[osstest service owner]

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

Regressions :-(

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

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

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 182531
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 182531
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 182531
 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  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 

[XEN PATCH v2] xen/arm: ioreq: add header for 'handle_ioserv' and 'try_fwd_ioserv'

[Nicola Vetrini]

The functions referenced by this patch should have had a compatible
declaration visible prior to their definition. This is achieved by
including the arch-specific header in 'xen/arch/arm/ioreq.c'

Fixes: cb9953d2f2bc ("arm/ioreq: Introduce arch specific bits for IOREQ/DM 
features")
Signed-off-by: Nicola Vetrini 
---
Changes in v2:
- Avoid including  in  to allow new architectures
  (e.g. ppc and riscv) not to provide one more stub header,
  as pointed out by Jan Beulich.
---
 xen/arch/arm/ioreq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
index 55854571898d..3bed0a14c050 100644
--- a/xen/arch/arm/ioreq.c
+++ b/xen/arch/arm/ioreq.c
@@ -9,6 +9,7 @@
 #include 

 #include 
+#include 

 #include 

--
2.34.1

Re: [PATCH] Arm: constrain {,u}int64_aligned_t in public header

[Bertrand Marquis]

Hi Jan,

> On 4 Sep 2023, at 17:20, Jan Beulich  wrote:
> 
> On 04.09.2023 16:05, Bertrand Marquis wrote:
>>> On 4 Sep 2023, at 16:01, Jan Beulich  wrote:
>>> 
>>> On 04.09.2023 15:42, Bertrand Marquis wrote:
> On 1 Sep 2023, at 09:26, Jan Beulich  wrote:
> 
> This using a GNU extension, it may not be exposed in general, just like
 
 Nit: Missing "is"
>>> 
>>> I would guess you would want it added as the 2nd word of the sentence. If
>>> not, please clarify where you think it is missing. If so, then I'm afraid
>>> I can't parse the sentence anymore with it added (i.e. there would need
>>> to be further modifications, e.g. at the very least "so" after the first
>>> comma).
>> 
>> Sorry yes, it should be "This is using a GNU".
> 
> So as I inferred, yet as said - according to my reading the sentence then
> ends up broken. If you continue to think the sentence is wrong as is, would
> it help if I replaced "This" by "For"?

The sentence looks a bit weird to me but I am not a native english speaker.
Any reformulation coming from me will probably not be good english anyway.
I understand that one as "we don't want to expose this in general because
it is a using a GNU extension and x86 is already not", the sentence here is
just asking me a bit more thinking that is it.

As this was a Nit, feel free to ignore and you can keep my R-b.

Cheers
Bertrand

> 
> Jan
> 
> is done on x86. External consumers need to make this type available up
> front (just like we expect {,u}int_t to be supplied) - unlike on x86
> the type is actually needed outside of tools-only interfaces, because
> guest handle definitions use it.
> 
> While there also add underscores around "aligned".
> 
> Signed-off-by: Jan Beulich 
 
 With the Nit fixed (can be done on commit):
 
 Reviewed-by: Bertrand Marquis 
>>> 
>>> Thanks, but I'm afraid I can't take it before the above is clarified.
>> 
>> Please see above.
>> 
>> Bertrand
>> 
>>> 
>>> Jan

Re: [PATCH] Arm: constrain {,u}int64_aligned_t in public header

[Jan Beulich]

On 04.09.2023 16:05, Bertrand Marquis wrote:
>> On 4 Sep 2023, at 16:01, Jan Beulich  wrote:
>>
>> On 04.09.2023 15:42, Bertrand Marquis wrote:
 On 1 Sep 2023, at 09:26, Jan Beulich  wrote:

 This using a GNU extension, it may not be exposed in general, just like
>>>
>>> Nit: Missing "is"
>>
>> I would guess you would want it added as the 2nd word of the sentence. If
>> not, please clarify where you think it is missing. If so, then I'm afraid
>> I can't parse the sentence anymore with it added (i.e. there would need
>> to be further modifications, e.g. at the very least "so" after the first
>> comma).
> 
> Sorry yes, it should be "This is using a GNU".

So as I inferred, yet as said - according to my reading the sentence then
ends up broken. If you continue to think the sentence is wrong as is, would
it help if I replaced "This" by "For"?

Jan

 is done on x86. External consumers need to make this type available up
 front (just like we expect {,u}int_t to be supplied) - unlike on x86
 the type is actually needed outside of tools-only interfaces, because
 guest handle definitions use it.

 While there also add underscores around "aligned".

 Signed-off-by: Jan Beulich 
>>>
>>> With the Nit fixed (can be done on commit):
>>>
>>> Reviewed-by: Bertrand Marquis 
>>
>> Thanks, but I'm afraid I can't take it before the above is clarified.
> 
> Please see above.
> 
> Bertrand
> 
>>
>> Jan
> 

Re: [PATCH] xen/arm: ffa: fix guest map RX/TX error code

[Bertrand Marquis]

Hi Jens,

> On 4 Sep 2023, at 16:58, Jens Wiklander  wrote:
> 
> FFA_RXTX_MAP is currently limited to mapping only one 4k page for each
> RX and TX buffer. If a guest tries to map more than one page, an error
> is returned. Until this patch, we have been using FFA_RET_NOT_SUPPORTED.
> However, that error code is reserved in the FF-A specification to report
> that the function is not implemented. Of all the other defined error
> codes, the least bad is FFA_RET_INVALID_PARAMETERS, so use that instead.

Agree and thanks for the fix.

> 
> Fixes: 38d81e7ccb11 ("xen/arm: ffa: support mapping guest RX/TX buffers")
> Signed-off-by: Jens Wiklander 

Reviewed-by: Bertrand Marquis 

Cheers
Bertrand

> ---
> xen/arch/arm/tee/ffa.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 802b2dbb1d36..183528d13388 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -706,7 +706,7 @@ static uint32_t handle_rxtx_map(uint32_t fid, register_t 
> tx_addr,
> {
> printk(XENLOG_ERR "ffa: RXTX_MAP: error: %u pages requested (limit 
> %u)\n",
>page_count, FFA_MAX_RXTX_PAGE_COUNT);
> -return FFA_RET_NOT_SUPPORTED;
> +return FFA_RET_INVALID_PARAMETERS;
> }
> 
> /* Already mapped */
> -- 
> 2.34.1
> 

Re: [PATCH 2/2] xen: Change parameter of generic_fls() to unsigned int

[Jan Beulich]

On 04.09.2023 16:03, Michal Orzel wrote:
> On 04/09/2023 15:28, Jan Beulich wrote:
>> On 04.09.2023 11:14, Michal Orzel wrote:
>>> --- a/xen/include/xen/bitops.h
>>> +++ b/xen/include/xen/bitops.h
>>> @@ -51,7 +51,7 @@ static inline int generic_ffs(int x)
>>>   * fls: find last bit set.
>>>   */
>>>
>>> -static __inline__ int generic_fls(int x)
>>> +static __inline__ int generic_fls(unsigned int x)
>>>  {
>>>  int r = 32;
>>>
>>
>> Even if perhaps not affected by UBSAN, generic_ffs() then wants taking care
>> of as well, imo. If additionally you switch __inline__ to inline, things
>> will become nicely symmetric with generic_f{f,l}sl().
> If others agree, I can switch generic_ffs() to take unsigned as well 
> (together with s/__inline__/inline/).
> However, such change would no longer fall in fixes category (i.e. nothing 
> wrong with ffs() taking int):
> - is it ok at this stage of release? (not sure but no problem for me to do 
> this)

I'd say yes, but the release manager would have the ultimate say.

> - is it ok to mix a fix with non-fix change? (I always prefer fixes to be 
> done as standalone changes)

Imo it is, to avoid introducing an inconsistency. While such may
not themselves be bugs, they increase the risk of introducing some.

Jan

[PATCH] xen/arm: ffa: fix guest map RX/TX error code

[Jens Wiklander]

FFA_RXTX_MAP is currently limited to mapping only one 4k page for each
RX and TX buffer. If a guest tries to map more than one page, an error
is returned. Until this patch, we have been using FFA_RET_NOT_SUPPORTED.
However, that error code is reserved in the FF-A specification to report
that the function is not implemented. Of all the other defined error
codes, the least bad is FFA_RET_INVALID_PARAMETERS, so use that instead.

Fixes: 38d81e7ccb11 ("xen/arm: ffa: support mapping guest RX/TX buffers")
Signed-off-by: Jens Wiklander 
---
 xen/arch/arm/tee/ffa.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 802b2dbb1d36..183528d13388 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -706,7 +706,7 @@ static uint32_t handle_rxtx_map(uint32_t fid, register_t 
tx_addr,
 {
 printk(XENLOG_ERR "ffa: RXTX_MAP: error: %u pages requested (limit 
%u)\n",
page_count, FFA_MAX_RXTX_PAGE_COUNT);
-return FFA_RET_NOT_SUPPORTED;
+return FFA_RET_INVALID_PARAMETERS;
 }
 
 /* Already mapped */
-- 
2.34.1

Re: [PATCH v1 2/2] xen/arm: Enlarge identity map space to 127TiB

[Leo Yan]

Hi Bertrand,

On Mon, Sep 04, 2023 at 01:55:12PM +, Bertrand Marquis wrote:
> Hi Leo,
> 
> > On 31 Aug 2023, at 13:01, Leo Yan  wrote:
> > 
> > On some platforms, the memory regions could be:
> > 
> >  (XEN) MODULE[0]: 0807f6df - 0807f6f3e000 Xen
> >  (XEN) MODULE[1]: 0807f8054000 - 0807f8056000 Device Tree
> >  (XEN) MODULE[2]: fa834000 - fc5de1d5 Ramdisk
> >  (XEN) MODULE[3]: fc5df000 - ffb3f810 Kernel
> > 
> > In this case, the Xen binary is loaded above 2TB, so Xen fails to boot
> > up due to the out of the identity map space.
> > 
> > This patch enlarges identity map space to 127TiB, which can support the
> > memory space [0x0 .. 0x7eff__], thus it has flexibility for
> > support different platforms.
> > 
> > Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to 
> > prepare/enable/disable")
> > Reported-by: Alexey Klimov 
> > Signed-off-by: Leo Yan 
> 
> This is not based on the current status of staging where this part of the
> code was modified.
> 
> Currently Xen virtual address is at 2TB and the extension you are making
> will potentially make it possible to load Xen at 2TB which will clash for the
> identity mapping handling in Xen.

I will check the stage code if this patch will introduce any clash
with the identity mapping handling.

> Please rebase on the latest staging and check there how you can do as
> this patch if rebased on staging right now with rightfully end in a 
> compilation
> error.

Sure, I will rebase on the latest staging branch.

Thank you for suggestions.

Leo

Re: [PATCH] Arm: constrain {,u}int64_aligned_t in public header

[Bertrand Marquis]

Hi Jan,

> On 4 Sep 2023, at 16:01, Jan Beulich  wrote:
> 
> On 04.09.2023 15:42, Bertrand Marquis wrote:
>>> On 1 Sep 2023, at 09:26, Jan Beulich  wrote:
>>> 
>>> This using a GNU extension, it may not be exposed in general, just like
>> 
>> Nit: Missing "is"
> 
> I would guess you would want it added as the 2nd word of the sentence. If
> not, please clarify where you think it is missing. If so, then I'm afraid
> I can't parse the sentence anymore with it added (i.e. there would need
> to be further modifications, e.g. at the very least "so" after the first
> comma).

Sorry yes, it should be "This is using a GNU".

> 
>>> is done on x86. External consumers need to make this type available up
>>> front (just like we expect {,u}int_t to be supplied) - unlike on x86
>>> the type is actually needed outside of tools-only interfaces, because
>>> guest handle definitions use it.
>>> 
>>> While there also add underscores around "aligned".
>>> 
>>> Signed-off-by: Jan Beulich 
>> 
>> With the Nit fixed (can be done on commit):
>> 
>> Reviewed-by: Bertrand Marquis 
> 
> Thanks, but I'm afraid I can't take it before the above is clarified.

Please see above.

Bertrand

> 
> Jan

Re: [PATCH 2/2] xen: Change parameter of generic_fls() to unsigned int

[Michal Orzel]

On 04/09/2023 15:28, Jan Beulich wrote:
> 
> 
> On 04.09.2023 11:14, Michal Orzel wrote:
>> --- a/xen/include/xen/bitops.h
>> +++ b/xen/include/xen/bitops.h
>> @@ -51,7 +51,7 @@ static inline int generic_ffs(int x)
>>   * fls: find last bit set.
>>   */
>>
>> -static __inline__ int generic_fls(int x)
>> +static __inline__ int generic_fls(unsigned int x)
>>  {
>>  int r = 32;
>>
> 
> Even if perhaps not affected by UBSAN, generic_ffs() then wants taking care
> of as well, imo. If additionally you switch __inline__ to inline, things
> will become nicely symmetric with generic_f{f,l}sl().
If others agree, I can switch generic_ffs() to take unsigned as well (together 
with s/__inline__/inline/).
However, such change would no longer fall in fixes category (i.e. nothing wrong 
with ffs() taking int):
- is it ok at this stage of release? (not sure but no problem for me to do this)
- is it ok to mix a fix with non-fix change? (I always prefer fixes to be done 
as standalone changes)

> 
> Another aspect that may be nice to take care of at this occasion is their
> return values: None of them can return negative values, so return type
> would better be unsigned int.
Looking at all the variations (including arch asm specific) of helpers to find 
first set/clear in the codebase, returning int is the de-facto standard.
So, this would result in some inconsistency or require changing at least the 
direct callers to also return unsigned.
I'd prefer not to be required to add this extra churn at this release stage.

~Michal

Re: [PATCH] Arm: constrain {,u}int64_aligned_t in public header

[Jan Beulich]

On 04.09.2023 15:42, Bertrand Marquis wrote:
>> On 1 Sep 2023, at 09:26, Jan Beulich  wrote:
>>
>> This using a GNU extension, it may not be exposed in general, just like
> 
> Nit: Missing "is"

I would guess you would want it added as the 2nd word of the sentence. If
not, please clarify where you think it is missing. If so, then I'm afraid
I can't parse the sentence anymore with it added (i.e. there would need
to be further modifications, e.g. at the very least "so" after the first
comma).

>> is done on x86. External consumers need to make this type available up
>> front (just like we expect {,u}int_t to be supplied) - unlike on x86
>> the type is actually needed outside of tools-only interfaces, because
>> guest handle definitions use it.
>>
>> While there also add underscores around "aligned".
>>
>> Signed-off-by: Jan Beulich 
> 
> With the Nit fixed (can be done on commit):
> 
> Reviewed-by: Bertrand Marquis 

Thanks, but I'm afraid I can't take it before the above is clarified.

Jan

Re: [PATCH v1 1/2] xen/arm: Add macro XEN_VM_MAPPING

[Bertrand Marquis]

Hi Leo,

> On 31 Aug 2023, at 13:01, Leo Yan  wrote:
> 
> Xen maps the virtual memory space starting from L0 slot 4, so it's open
> coded for macros with the offset '4'.
> 
> For more readable, add a new macro XEN_VM_MAPPING which defines the
> start slot for Xen virtual memory mapping, and all virtual memory
> regions are defined based on it.
> 
> Signed-off-by: Leo Yan 

As said on patch 2, please check current staging code as it does not work
like this anymore.

Cheers
Bertrand

> ---
> xen/arch/arm/include/asm/config.h | 11 ++-
> 1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/config.h 
> b/xen/arch/arm/include/asm/config.h
> index 83cbf6b0cb..21f4e68a40 100644
> --- a/xen/arch/arm/include/asm/config.h
> +++ b/xen/arch/arm/include/asm/config.h
> @@ -117,11 +117,14 @@
> #define XEN_VIRT_START  _AT(vaddr_t, MB(2))
> #else
> 
> +#define IDENTITY_MAPPING_AREA_NR_L0 4
> +#define XEN_VM_MAPPING SLOT0(IDENTITY_MAPPING_AREA_NR_L0)
> +
> #define SLOT0_ENTRY_BITS  39
> #define SLOT0(slot) (_AT(vaddr_t,slot) << SLOT0_ENTRY_BITS)
> #define SLOT0_ENTRY_SIZE  SLOT0(1)
> 
> -#define XEN_VIRT_START  (SLOT0(4) + _AT(vaddr_t, MB(2)))
> +#define XEN_VIRT_START  (XEN_VM_MAPPING + _AT(vaddr_t, MB(2)))
> #endif
> 
> /*
> @@ -184,12 +187,10 @@
> 
> #else /* ARM_64 */
> 
> -#define IDENTITY_MAPPING_AREA_NR_L0  4
> -
> -#define VMAP_VIRT_START  (SLOT0(4) + GB(1))
> +#define VMAP_VIRT_START  (XEN_VM_MAPPING + GB(1))
> #define VMAP_VIRT_SIZE   GB(1)
> 
> -#define FRAMETABLE_VIRT_START  (SLOT0(4) + GB(32))
> +#define FRAMETABLE_VIRT_START  (XEN_VM_MAPPING + GB(32))
> #define FRAMETABLE_SIZEGB(32)
> #define FRAMETABLE_NR  (FRAMETABLE_SIZE / sizeof(*frame_table))
> 
> -- 
> 2.39.2
> 

Re: [Xen-devel] [PATCH] x86/HVM: adjust hvm_interrupt_blocked()

[Jan Beulich]

On 31.08.2023 12:42, Roger Pau Monné wrote:
> On Fri, Oct 12, 2018 at 09:58:46AM -0600, Jan Beulich wrote:
>> First of all, hvm_intsrc_mce was not considered here at all, yet nothing
>> blocks #MC (other than an already in-progress #MC, but dealing with this
>> is not the purpose of this patch).
>>
>> Additionally STI-shadow only blocks maskable interrupts, but not NMI.
> 
> I've found the Table 25-3 on Intel SDM vol3 quite helpful:
> 
> "Execution of STI with RFLAGS.IF = 0 blocks maskable interrupts on the
> instruction boundary following its execution.1 Setting this bit
> indicates that this blocking is in effect."
> 
> And:
> 
> "Execution of a MOV to SS or a POP to SS blocks or suppresses certain
> debug exceptions as well as interrupts (maskable and nonmaskable) on
> the instruction boundary following its execution."
> 
> Might be worth adding to the commit message IMO.

Hmm, to be honest I'm not sure why reproducing some of one vendor's doc
would be useful here.

>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3771,19 +3771,24 @@ enum hvm_intblk hvm_interrupt_blocked(st
>>  return intr;
>>  }
>>  
>> -if ( (intack.source != hvm_intsrc_nmi) &&
>> - !(guest_cpu_user_regs()->eflags & X86_EFLAGS_IF) )
>> -return hvm_intblk_rflags_ie;
>> +if ( intack.source == hvm_intsrc_mce )
>> +return hvm_intblk_none;
> 
> I've been wondering, why do we handle #MC here, instead of doing the
> same as for other Traps/Exceptions and use hvm_inject_hw_exception()
> directly?

I'm afraid I can only guess here, but I suspect a connection to how
vMCE "works" (and the point in time when v->arch.mce_pending would be
set).

>>  intr_shadow = hvm_funcs.get_interrupt_shadow(v);
>>  
>> -if ( intr_shadow & (HVM_INTR_SHADOW_STI|HVM_INTR_SHADOW_MOV_SS) )
>> +if ( intr_shadow & HVM_INTR_SHADOW_MOV_SS )
>>  return hvm_intblk_shadow;
>>  
>>  if ( intack.source == hvm_intsrc_nmi )
>>  return ((intr_shadow & HVM_INTR_SHADOW_NMI) ?
>>  hvm_intblk_nmi_iret : hvm_intblk_none);
>>  
>> +if ( intr_shadow & HVM_INTR_SHADOW_STI )
>> +return hvm_intblk_shadow;
>> +
>> +if ( !(guest_cpu_user_regs()->eflags & X86_EFLAGS_IF) )
>> +return hvm_intblk_rflags_ie;
> 
> I do wonder whether this code would be clearer using a `switch (
> intack.source )` construct, but that's out of the scope.

If it would help, I could switch to using switch(), but the order
of checks isn't really a sequence of comparisons against intack.source.

Jan

Re: [PATCH v1 2/2] xen/arm: Enlarge identity map space to 127TiB

[Bertrand Marquis]

Hi Leo,

> On 31 Aug 2023, at 13:01, Leo Yan  wrote:
> 
> On some platforms, the memory regions could be:
> 
>  (XEN) MODULE[0]: 0807f6df - 0807f6f3e000 Xen
>  (XEN) MODULE[1]: 0807f8054000 - 0807f8056000 Device Tree
>  (XEN) MODULE[2]: fa834000 - fc5de1d5 Ramdisk
>  (XEN) MODULE[3]: fc5df000 - ffb3f810 Kernel
> 
> In this case, the Xen binary is loaded above 2TB, so Xen fails to boot
> up due to the out of the identity map space.
> 
> This patch enlarges identity map space to 127TiB, which can support the
> memory space [0x0 .. 0x7eff__], thus it has flexibility for
> support different platforms.
> 
> Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to 
> prepare/enable/disable")
> Reported-by: Alexey Klimov 
> Signed-off-by: Leo Yan 

This is not based on the current status of staging where this part of the
code was modified.

Currently Xen virtual address is at 2TB and the extension you are making
will potentially make it possible to load Xen at 2TB which will clash for the
identity mapping handling in Xen.

Please rebase on the latest staging and check there how you can do as
this patch if rebased on staging right now with rightfully end in a compilation
error.

Cheers
Bertrand

> ---
> xen/arch/arm/include/asm/config.h | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/config.h 
> b/xen/arch/arm/include/asm/config.h
> index 21f4e68a40..3e97c95b57 100644
> --- a/xen/arch/arm/include/asm/config.h
> +++ b/xen/arch/arm/include/asm/config.h
> @@ -87,11 +87,11 @@
>  *   2G -   4G   Domheap: on-demand-mapped
>  *
>  * ARM64 layout:
> - * 0x - 0x01ff (2TB, L0 slots [0..3])
> + * 0x - 0x7eff (127TB, L0 slots [0..253])
>  *
>  *  Reserved to identity map Xen
>  *
> - * 0x0200 - 0x027f (512GB, L0 slot [4])
> + * 0x07f0 - 0x7fff (1TB, L0 slot [254..255])
>  *  (Relative offsets)
>  *   0  -   2M   Unmapped
>  *   2M -  10M   Xen text, data, bss
> @@ -103,9 +103,6 @@
>  *
>  *  32G -  64G   Frametable: 56 bytes per page for 2TB of RAM
>  *
> - * 0x0280 - 0x7fff (125TB, L0 slots [5..255])
> - *  Unused
> - *
>  * 0x8000 - 0x84ff (5TB, L0 slots [256..265])
>  *  1:1 mapping of RAM
>  *
> @@ -117,7 +114,7 @@
> #define XEN_VIRT_START  _AT(vaddr_t, MB(2))
> #else
> 
> -#define IDENTITY_MAPPING_AREA_NR_L0 4
> +#define IDENTITY_MAPPING_AREA_NR_L0 254
> #define XEN_VM_MAPPING SLOT0(IDENTITY_MAPPING_AREA_NR_L0)
> 
> #define SLOT0_ENTRY_BITS  39
> -- 
> 2.39.2
> 

Re: [PATCH] Arm: constrain {,u}int64_aligned_t in public header

[Bertrand Marquis]

Hi Jan,

> On 1 Sep 2023, at 09:26, Jan Beulich  wrote:
> 
> This using a GNU extension, it may not be exposed in general, just like

Nit: Missing "is"

> is done on x86. External consumers need to make this type available up
> front (just like we expect {,u}int_t to be supplied) - unlike on x86
> the type is actually needed outside of tools-only interfaces, because
> guest handle definitions use it.
> 
> While there also add underscores around "aligned".
> 
> Signed-off-by: Jan Beulich 

With the Nit fixed (can be done on commit):

Reviewed-by: Bertrand Marquis 

Cheers
Bertrand

> 
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -152,8 +152,10 @@
> 
> #define XEN_HYPERCALL_TAG   0XEA1
> 
> -#define  int64_aligned_t  int64_t __attribute__((aligned(8)))
> -#define uint64_aligned_t uint64_t __attribute__((aligned(8)))
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +#define  int64_aligned_t  int64_t __attribute__((__aligned__(8)))
> +#define uint64_aligned_t uint64_t __attribute__((__aligned__(8)))
> +#endif
> 
> #ifndef __ASSEMBLY__
> #define ___DEFINE_XEN_GUEST_HANDLE(name, type)  \

Re: [Xen-devel] [PATCH] x86/HVM: adjust hvm_interrupt_blocked()

[Jan Beulich]

On 31.08.2023 12:57, Roger Pau Monné wrote:
> On Thu, Aug 31, 2023 at 12:42:58PM +0200, Roger Pau Monné wrote:
>> On Fri, Oct 12, 2018 at 09:58:46AM -0600, Jan Beulich wrote:
>>> First of all, hvm_intsrc_mce was not considered here at all, yet nothing
>>> blocks #MC (other than an already in-progress #MC, but dealing with this
>>> is not the purpose of this patch).
>>>
>>> Additionally STI-shadow only blocks maskable interrupts, but not NMI.
>>
>> I've found the Table 25-3 on Intel SDM vol3 quite helpful:
>>
>> "Execution of STI with RFLAGS.IF = 0 blocks maskable interrupts on the
>> instruction boundary following its execution.1 Setting this bit
>> indicates that this blocking is in effect."
>>
>> And:
>>
>> "Execution of a MOV to SS or a POP to SS blocks or suppresses certain
>> debug exceptions as well as interrupts (maskable and nonmaskable) on
>> the instruction boundary following its execution."
>>
>> Might be worth adding to the commit message IMO.
> 
> So I've found a further footnote that contains:
> 
> "Nonmaskable interrupts and system-management interrupts may also be
> inhibited on the instruction boundary following such an execution of
> STI."
> 
> So we want to take the more restrictive implementation of STI-shadow,
> and block #NMI there also.

Hmm, that text says "may", not will, and imo STI affecting NMI can at best
be viewed as a quirk (quite possibly intentional, for simplifying some
internal logic on the processor). Plus I'm not convinced AMD allows similar
leeway in SVM; at least I can't spot any similar statement in their PM.

Jan

Re: [PATCH v4 1/2] xen: asm-generic support

[Bertrand Marquis]

Hi Oleksii,

> On 1 Sep 2023, at 18:02, Oleksii Kurochko  wrote:
> 
> Some headers are shared between individual architectures or are empty.
> To avoid duplication of these headers, asm-generic is introduced.
> 
> With the following patch, an architecture uses generic headers
> mentioned in the file arch/$(ARCH)/include/asm/Kbuild.

Kbuild refers to "Kernel build" I guess.
I am ok to keep that name to keep things simpler when compared to
Linux scripts but it would be good to mention that in the commit
message for future reference. 

> 
> To use a generic header is needed to add to
> arch/$(ARCH)/include/asm/Kbuild :
>generic-y += 
> 
> For each mentioned header in arch/$(ARCH)/include/asm/Kbuild,
> kbuild will generate the necessary wrapper in
> arch/$(ARCH)/include/generated/asm.
> 
> Signed-off-by: Oleksii Kurochko 
> ---
> Changes in V4:
> - introduce asm-generic support
> - update commit message
> ---
> Changes in V3:
> - Rename stubs dir to asm-generic
> ---
> Changes in V2:
> - Nothing changed.
> ---
> xen/Makefile  | 10 +-
> xen/arch/arm/include/asm/Kbuild   |  1 +
> xen/arch/ppc/include/asm/Kbuild   |  1 +
> xen/arch/riscv/include/asm/Kbuild |  1 +
> xen/arch/x86/include/asm/Kbuild   |  1 +
> xen/scripts/Makefile.asm-generic  | 23 +++
> 6 files changed, 36 insertions(+), 1 deletion(-)
> create mode 100644 xen/arch/arm/include/asm/Kbuild
> create mode 100644 xen/arch/ppc/include/asm/Kbuild
> create mode 100644 xen/arch/riscv/include/asm/Kbuild
> create mode 100644 xen/arch/x86/include/asm/Kbuild
> create mode 100644 xen/scripts/Makefile.asm-generic
> 
> diff --git a/xen/Makefile b/xen/Makefile
> index f57e5a596c..698d6ddeb8 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -438,6 +438,7 @@ ifdef building_out_of_srctree
> endif
> CFLAGS += -I$(srctree)/include
> CFLAGS += -I$(srctree)/arch/$(SRCARCH)/include
> +CFLAGS += -I$(srctree)/arch/$(SRCARCH)/include/generated

Why are we generating files in the source tree ? 
Shouldn't we keep it unmodified ?

> 
> # Note that link order matters!
> ALL_OBJS-y:= common/built_in.o
> @@ -580,6 +581,7 @@ _clean:
> rm -f $(TARGET).efi $(TARGET).efi.map $(TARGET).efi.elf $(TARGET).efi.stripped
> rm -f asm-offsets.s arch/*/include/asm/asm-offsets.h
> rm -f .banner .allconfig.tmp include/xen/compile.h
> + rm -rf arch/*/include/generated
> 

You must use the same as for CFLAGS here so $(srctree) would be needed.
(or something else depending on the fix for previous comment)

> .PHONY: _distclean
> _distclean: clean
> @@ -589,7 +591,7 @@ $(TARGET).gz: $(TARGET)
> gzip -n -f -9 < $< > $@.new
> mv $@.new $@
> 
> -$(TARGET): outputmakefile FORCE
> +$(TARGET): outputmakefile asm-generic FORCE
> $(Q)$(MAKE) $(build)=tools
> $(Q)$(MAKE) $(build)=. include/xen/compile.h
> $(Q)$(MAKE) $(build)=include all
> @@ -667,6 +669,12 @@ endif # need-sub-make
> PHONY += FORCE
> FORCE:
> 
> +# Support for using generic headers in asm-generic
> +PHONY += asm-generic
> +asm-generic:
> + $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.asm-generic \
> +obj=arch/$(SRCARCH)/include/generated/asm
> +
> # Declare the contents of the PHONY variable as phony.  We keep that
> # information in a variable so we can use it in if_changed and friends.
> .PHONY: $(PHONY)
> diff --git a/xen/arch/arm/include/asm/Kbuild b/xen/arch/arm/include/asm/Kbuild
> new file mode 100644
> index 00..a4e40e534e
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/Kbuild
> @@ -0,0 +1 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> diff --git a/xen/arch/ppc/include/asm/Kbuild b/xen/arch/ppc/include/asm/Kbuild
> new file mode 100644
> index 00..a4e40e534e
> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/Kbuild
> @@ -0,0 +1 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> diff --git a/xen/arch/riscv/include/asm/Kbuild 
> b/xen/arch/riscv/include/asm/Kbuild
> new file mode 100644
> index 00..a4e40e534e
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/Kbuild
> @@ -0,0 +1 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> diff --git a/xen/arch/x86/include/asm/Kbuild b/xen/arch/x86/include/asm/Kbuild
> new file mode 100644
> index 00..a4e40e534e
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/Kbuild
> @@ -0,0 +1 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> diff --git a/xen/scripts/Makefile.asm-generic 
> b/xen/scripts/Makefile.asm-generic
> new file mode 100644
> index 00..0aac3f50b8
> --- /dev/null
> +++ b/xen/scripts/Makefile.asm-generic
> @@ -0,0 +1,23 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# include/asm-generic contains a lot of files that are used
> +# verbatim by several architectures.
> +#
> +# This Makefile reads the file arch/$(SRCARCH)/include/asm/Kbuild
> +# and for each file listed in this file with generic-y creates
> +# a small wrapper file in $(obj) (arch/$(SRCARCH)/include/generated/asm)
> +
> +kbuild-file := $(srctree)/arch/$(SRCARCH)/include/asm/Kbuild
> +include $(kbuild-file)
> +
> 

Re: [PATCH 2/2] xen: Change parameter of generic_fls() to unsigned int

[Jan Beulich]

On 04.09.2023 11:14, Michal Orzel wrote:
> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -51,7 +51,7 @@ static inline int generic_ffs(int x)
>   * fls: find last bit set.
>   */
>  
> -static __inline__ int generic_fls(int x)
> +static __inline__ int generic_fls(unsigned int x)
>  {
>  int r = 32;
>  

Even if perhaps not affected by UBSAN, generic_ffs() then wants taking care
of as well, imo. If additionally you switch __inline__ to inline, things
will become nicely symmetric with generic_f{f,l}sl().

Another aspect that may be nice to take care of at this occasion is their
return values: None of them can return negative values, so return type
would better be unsigned int.

Jan

Documentation survey

[Kelly Choi]

Hey everyone,

Hope you've all had a good weekend!

I know that documentation is an important part of The Xen Project, and I
have received feedback from a number of users on this topic. Currently,
there are some barriers to entry and improvements that can be made.

In order to understand further, please could you answer the following
survey (~5-10 mins) on your thoughts around the existing documentation.
This feedback will help achieve a plan of action.

*All responses will be anonymous. *

*https://cryptpad.fr/form/#/2/form/view/aIaNqMdkkV85YkQSzM0+ddwMY36XSTf+Vl3k2APoP-U/
*


Many thanks,
Kelly Choi

Open Source Community Manager, XenServer
Cloud Software Group

Re: [XEN PATCH 05/13] automation/eclair: add deviation for usercopy.c

[Luca Fancellu]

> On 28 Aug 2023, at 23:27, Stefano Stabellini  wrote:
> 
> +Nicola, Luca
> 
> On Mon, 28 Aug 2023, Simone Ballarin wrote:
>> xen/arch/x86/usercopy.c includes itself, so it is not supposed to
>> comply with Directive 4.10:
>> "Precautions shall be taken in order to prevent the contents of a
>> header file being included more than once"
>> 
>> This patch adds a deviation for the file.
>> 
>> Signed-off-by: Simone Ballarin 
>> 
>> ---
>> automation/eclair_analysis/ECLAIR/deviations.ecl | 4 
>> docs/misra/rules.rst | 2 ++
>> 2 files changed, 6 insertions(+)
>> 
>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
>> b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> index 2681a4cff5..a7d4f29b43 100644
>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -96,6 +96,10 @@ conform to the directive."
>> -config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no 
>> inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"}
>> -doc_end
>> 
>> +-doc_begin="xen/arch/x86/usercopy.c includes itself: it is not supposed to 
>> comply with the directive"
>> +-config=MC3R1.D4.10,reports+={deliberate, 
>> "all_area(all_loc(file("^xen/arch/x86/usercopy\\.c$")))"}
>> +-doc_end
>> +
>> #
>> # Series 5.
>> #
>> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
>> index 4b1a7b02b6..45e13d0302 100644
>> --- a/docs/misra/rules.rst
>> +++ b/docs/misra/rules.rst
>> @@ -62,6 +62,8 @@ maintainers if you want to suggest a change.
>>  - Files that are intended to be included more than once do not need to
>>conform to the directive. Files that explicitly avoid inclusion guards
>>under specific circumstances do not need to conform the directive.
>> +   xen/arch/x86/usercopy.c includes itself: it is not supposed to comply
>> +   with the directive.
> 
> 
> We need to find a consistent way to document this kind of deviations in
> a non-ECLAIR specific way, without adding the complete list of
> deviations to rules.rst.
> 
> Can we use safe.json and add an in-code comment at the top of
> usercopy.c? E.g.:
> 
> diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c
> index b8c2d1cc0b..8bb591f472 100644
> --- a/xen/arch/x86/usercopy.c
> +++ b/xen/arch/x86/usercopy.c
> @@ -1,3 +1,4 @@
> +/* SAF-1-safe */
> /* 
>  * User address space access functions.
>  *
> 
> Otherwise, maybe we should extend safe.json to also have an extra field
> with a list of paths. For instance see "files" below:
> 
> {
>"version": "1.0",
>"content": [
>{
>"id": "SAF-0-safe",
>"analyser": {
>"eclair": "MC3R1.R8.6",
>"coverity": "misra_c_2012_rule_8_6_violation"
>},
>"name": "Rule 8.6: linker script defined symbols",
>"text": "It is safe to declare this symbol because it is defined 
> in the linker script."
>},
>{
>"id": "SAF-1-safe",
>"analyser": {
>"eclair": "MC3R1.D4.10"
>},
>"name": "Dir 4.10: files that include themselves",
>"text": "Files purposely written to include themselves are not 
> supposed to comply with D4.10.",
>"files": ["xen/arch/x86/usercopy.c"]

Why couldn’t we do it without the “files” field? The presence of the tag in the 
file and the justification (I think)
are enough. 

>},
>{
>"id": "SAF-2-safe",
>"analyser": {},
>"name": "Sentinel",
>"text": "Next ID to be used"
>}
>]
> }

Re: xen-analysis ECLAIR support

[Luca Fancellu]

> On 25 Aug 2023, at 22:56, Stefano Stabellini  wrote:
> 
> On Fri, 25 Aug 2023, Nicola Vetrini wrote:
>> On 25/08/2023 00:24, Stefano Stabellini wrote:
>>> Hi Luca,
>>> 
>>> We are looking into adding ECLAIR support for xen-analysis so that we
>>> can use the SAF-n-safe tags also with ECLAIR.
>>> 
>>> One question that came up is about multi-line statements. For instance,
>>> in a case like the following:
>>> 
>>> diff --git a/xen/common/inflate.c b/xen/common/inflate.c
>>> index 8fa4b96d12..8bdc9208da 100644
>>> --- a/xen/common/inflate.c
>>> +++ b/xen/common/inflate.c
>>> @@ -1201,6 +1201,7 @@ static int __init gunzip(void)
>>> magic[1] = NEXTBYTE();
>>> method   = NEXTBYTE();
>>> 
>>> +/* SAF-1-safe */
>>> if (magic[0] != 037 ||
>>> ((magic[1] != 0213) && (magic[1] != 0236))) {
>>> error("bad gzip magic numbers");
>>> 
>>> 
>>> Would SAF-1-safe cover both 037, and also 0213 and 0213?
>>> Or would it cover only 037?
>>> 
>>> We haven't use SAFE-n-safe extensively through the codebase yet but
>>> my understanding is that SAFE-n-safe would cover the entire statement of
>>> the following line, even if it is multi-line. Is that also your
>>> understanding? Does it work like that with cppcheck?
>>> 
>>> 
>>> It looks like ECLAIR requires a written-down number of lines of code to
>>> deviate if it is more than 1 line. In this example it would be 2 lines:
>>> 
>>> /* SAF-1-safe 2 */
>>> if (magic[0] != 037 ||
>>> ((magic[1] != 0213) && (magic[1] != 0236))) {
>>> 
>>> One option that I was thinking about is whether we can add the number of
>>> lines automatically in xen-analysis based on the number of lines of the
>>> next statement. What do you think?
>>> 
>>> It seems fragile to actually keep the number of lines inside the SAF
>>> comment in the code. I am afraid it could get out of sync due to code
>>> style refactoring or other mechanical changes.
>>> 
>> 
>> Having the number of lines automatically inferred from the code following the
>> comment
>> does not seem that robust either, given the minimal information in the SAF
>> comments
>> (e.g., what if the whole if statement needs to be deviated, rather
>> than the controlling expression?).
>> 
>> An alternative proposal could be the following:
>>  /* SAF-n-safe begin */
>>  if (magic[0] != 037 ||
>>  ((magic[1] != 0213) && (magic[1] != 0236))) {
>>  /* SAF-n-safe end */
>> 
>> which is translated, for ECLAIR, into:
>> 
>>/* -E> safe  2  */
>>if (magic[0] != 037 ||
>>  ((magic[1] != 0213) && (magic[1] != 0236))) {
>> 
>> this will deviate however many lines are between the begin and end markers.
> 
> I think this could be a good way to solve the problem when multi-line
> deviation support is required. In this case, like Jan suggested, it
> is easier to put everything on a single line, but in other cases it
> might not be possible.

Yes, in that case however we are tied to what the underlying tool are 
supporting,
for example eclair is pretty nice and support an in-code comment with advanced
Syntax, but for example cppcheck and also coverity don’t, so in the end we used
the common denominator where the comment suppress the next line (containing 
code).

Re: xen-analysis ECLAIR support

[Luca Fancellu]

> On 25 Aug 2023, at 09:28, Jan Beulich  wrote:
> 
> On 25.08.2023 10:18, Michal Orzel wrote:
>> Hi Stefano,
>> 
>> On 25/08/2023 00:24, Stefano Stabellini wrote:
>>> 
>>> 
>>> Hi Luca,
>>> 
>>> We are looking into adding ECLAIR support for xen-analysis so that we
>>> can use the SAF-n-safe tags also with ECLAIR.
>>> 
>>> One question that came up is about multi-line statements. For instance,
>>> in a case like the following:
>>> 
>>> diff --git a/xen/common/inflate.c b/xen/common/inflate.c
>>> index 8fa4b96d12..8bdc9208da 100644
>>> --- a/xen/common/inflate.c
>>> +++ b/xen/common/inflate.c
>>> @@ -1201,6 +1201,7 @@ static int __init gunzip(void)
>>> magic[1] = NEXTBYTE();
>>> method   = NEXTBYTE();
>>> 
>>> +/* SAF-1-safe */
>>> if (magic[0] != 037 ||
>>> ((magic[1] != 0213) && (magic[1] != 0236))) {
>>> error("bad gzip magic numbers");
>>> 
>>> 
>>> Would SAF-1-safe cover both 037, and also 0213 and 0213?
>>> Or would it cover only 037?
>>> 
>>> We haven't use SAFE-n-safe extensively through the codebase yet but
>>> my understanding is that SAFE-n-safe would cover the entire statement of
>>> the following line, even if it is multi-line. Is that also your
>>> understanding? Does it work like that with cppcheck?
>> Looking at the docs and the actual script, only the single line below SAF 
>> comment is excluded.
>> So in your case you would require:
>> 
>> /* SAF-1-safe */
>> if (magic[0] != 037 ||
>>/* SAF-1-safe */
>>((magic[1] != 0213) && (magic[1] != 0236))) {
>>error("bad gzip magic numbers");
> 
> Or (perhaps more neatly):
> 
>/* SAF-1-safe */
>if (magic[0] != 037 || (magic[1] != 0213 && magic[1] != 0236)) {
>error("bad gzip magic numbers");

+1 for this approach, I was going to suggest it

> 
> Jan

Re: xen-analysis ECLAIR support

[Luca Fancellu]

> On 25 Aug 2023, at 09:18, Michal Orzel  wrote:
> 
> Hi Stefano,
> 
> On 25/08/2023 00:24, Stefano Stabellini wrote:
>> 
>> 
>> Hi Luca,
>> 
>> We are looking into adding ECLAIR support for xen-analysis so that we
>> can use the SAF-n-safe tags also with ECLAIR.
>> 
>> One question that came up is about multi-line statements. For instance,
>> in a case like the following:
>> 
>> diff --git a/xen/common/inflate.c b/xen/common/inflate.c
>> index 8fa4b96d12..8bdc9208da 100644
>> --- a/xen/common/inflate.c
>> +++ b/xen/common/inflate.c
>> @@ -1201,6 +1201,7 @@ static int __init gunzip(void)
>> magic[1] = NEXTBYTE();
>> method   = NEXTBYTE();
>> 
>> +/* SAF-1-safe */
>> if (magic[0] != 037 ||
>> ((magic[1] != 0213) && (magic[1] != 0236))) {
>> error("bad gzip magic numbers");
>> 
>> 
>> Would SAF-1-safe cover both 037, and also 0213 and 0213?
>> Or would it cover only 037?
>> 
>> We haven't use SAFE-n-safe extensively through the codebase yet but
>> my understanding is that SAFE-n-safe would cover the entire statement of
>> the following line, even if it is multi-line. Is that also your
>> understanding? Does it work like that with cppcheck?
> Looking at the docs and the actual script, only the single line below SAF 
> comment is excluded.
> So in your case you would require:
> 
> /* SAF-1-safe */
> if (magic[0] != 037 ||
>/* SAF-1-safe */
>((magic[1] != 0213) && (magic[1] != 0236))) {
>error("bad gzip magic numbers");

Yes correct

> 
> I guess this was done so that it is clear that someone took all the parts of 
> the statements into account
> and all of them fall into the same justification (which might not be the 
> case).
> 
> BTW. I don't think we have also covered the case where there is more than one 
> violation in a single line
> that we want to deviate (e.g. sth like /* SAF-1-safe, SAF-2-safe */

You are right, but it should work adding multiple comments in this way:

/* SAF-1-safe */
/* SAF-2-safe */


> 
> ~Michal

[linux-linus test] 182624: regressions - FAIL

[osstest service owner]

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

Regressions :-(

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

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

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 182531
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 182531
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 182531
 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  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 

Re: [PATCH 1/2] xen/arm: smmuv3: Add missing U for shifted constant

[Bertrand Marquis]

Hi Michal,

> On 4 Sep 2023, at 11:14, Michal Orzel  wrote:
> 
> When running with SMMUv3 and UBSAN enabled, the following is printed:
> 
> (XEN) UBSAN: Undefined behaviour in drivers/passthrough/arm/smmu-v3.c:297:12
> (XEN) left shift of 1 by 31 places cannot be represented in type 'int'
> 
> This refers to shift in Q_OVERFLOW_FLAG that is missing 'U' suffix.
> While there, also fix the same in GBPA_UPDATE.
> 
> This should address MISRA Rule 7.2:
>A "u" or "U" suffix shall be applied to all integer constants that
>are represented in an unsigned type
> 
> Signed-off-by: Michal Orzel 

Reviewed-by: Bertrand Marquis 

Cheers
Bertrand

> ---
> xen/drivers/passthrough/arm/smmu-v3.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.h 
> b/xen/drivers/passthrough/arm/smmu-v3.h
> index b381ad373845..05f6b1fb7e33 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.h
> +++ b/xen/drivers/passthrough/arm/smmu-v3.h
> @@ -87,7 +87,7 @@
> #define CR2_E2H (1 << 0)
> 
> #define ARM_SMMU_GBPA 0x44
> -#define GBPA_UPDATE (1 << 31)
> +#define GBPA_UPDATE (1U << 31)
> #define GBPA_ABORT (1 << 20)
> 
> #define ARM_SMMU_IRQ_CTRL 0x50
> @@ -159,7 +159,7 @@
> 
> #define Q_IDX(llq, p) ((p) & ((1 << (llq)->max_n_shift) - 1))
> #define Q_WRP(llq, p) ((p) & (1 << (llq)->max_n_shift))
> -#define Q_OVERFLOW_FLAG (1 << 31)
> +#define Q_OVERFLOW_FLAG (1U << 31)
> #define Q_OVF(p) ((p) & Q_OVERFLOW_FLAG)
> #define Q_ENT(q, p) ((q)->base + \
> Q_IDX(&((q)->llq), p) * \
> -- 
> 2.25.1
> 

Re: [XEN][PATCH v11 15/20] arm/asm/setup.h: Update struct map_range_data to add rangeset.

[Michal Orzel]

On 01/09/2023 06:59, Vikram Garhwal wrote:
> Add rangesets for IRQs and IOMEMs. This was done to accommodate dynamic 
> overlay
> node addition/removal operations. With overlay operations, new IRQs and IOMEMs
> are added in dt_host and routed. While removing overlay nodes, nodes are 
> removed
> from dt_host and their IRQs and IOMEMs routing is also removed. Storing IRQs 
> and
> IOMEMs in the rangeset will avoid re-parsing the device tree nodes to get the
> IOMEM and IRQ ranges for overlay remove ops.
> 
> Dynamic overlay node add/remove will be introduced in follow-up patches.
> 
> Signed-off-by: Vikram Garhwal 
> 
> ---
> Changes from v10:
> Replace paddr_to_pfn(PAGE_ALIGN()) with paddr_to_pfn_aligned().
> Change data type of irq.
> fix function change for handle_device().
> Remove unnecessary change .d = d in mr_data.
> ---
> ---
>  xen/arch/arm/device.c| 43 +---
>  xen/arch/arm/domain_build.c  |  4 +--
>  xen/arch/arm/include/asm/setup.h |  9 ---
>  3 files changed, 42 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> index 327e4d24fb..1f631d3274 100644
> --- a/xen/arch/arm/device.c
> +++ b/xen/arch/arm/device.c
> @@ -165,6 +165,15 @@ int map_range_to_domain(const struct dt_device_node *dev,
>  dt_dprintk("  - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
> addr, addr + len, mr_data->p2mt);
>  
> +if ( mr_data->iomem_ranges )
> +{
> +res = rangeset_add_range(mr_data->iomem_ranges,
> + paddr_to_pfn(addr),
> + paddr_to_pfn_aligned(addr + len - 1));
> +if ( res )
> +return res;
> +}
> +
>  return 0;
>  }
>  
> @@ -178,10 +187,11 @@ int map_range_to_domain(const struct dt_device_node 
> *dev,
>   */
>  int map_device_irqs_to_domain(struct domain *d,
>struct dt_device_node *dev,
> -  bool need_mapping)
> +  bool need_mapping,
> +  struct rangeset *irq_ranges)
>  {
>  unsigned int i, nirq;
> -int res;
> +int res, irq;
You could make use of res to store irq just as it was done before without 
introducing new local var.
Anyway, if you think it improves readability:
Reviewed-by: Michal Orzel 

~Michal

Re: [PATCH v4 2/2] xen: move arm/include/asm/vm_event.h to asm-generic

[Oleksii]

On Mon, 2023-09-04 at 08:54 +0200, Jan Beulich wrote:
> On 01.09.2023 18:02, Oleksii Kurochko wrote:
> > asm/vm_event.h is common for ARM and RISC-V so it will be moved to
> > asm-generic dir.
> > 
> > Original asm/vm_event.h from ARM was updated:
> >  * use SPDX-License-Identifier.
> >  * update comment messages of stubs.
> >  * update #ifdef.
> >  * change public/domctl.h to public/vm_event.h.
> > 
> > Signed-off-by: Oleksii Kurochko 
> > Acked-by: Stefano Stabellini 
> > ---
> > Changes in V4:
> >  - update path of vm_event.h from include/asm-generic/asm to
> > include/asm-generic
> > ---
> > Changes in V3:
> >  - add Acked-by: Stefano Stabellini  for
> > "xen: move arm/include/asm/vm_event.h to asm-generic"
> >  - update SPDX tag.
> >  - move asm/vm_event.h to asm-generic.
> > ---
> > Changes in V2:
> >  - change public/domctl.h to public/vm_event.h.
> >  - update commit message of [PATCH v2 2/2] xen: move
> > arm/include/asm/vm_event.h to stubs
> > ---
> >  xen/include/asm-generic/vm_event.h | 55
> > ++
> >  1 file changed, 55 insertions(+)
> >  create mode 100644 xen/include/asm-generic/vm_event.h
> 
> But that's not "move" anymore, as the Arm header isn't being deleted.
Yes, it is correct.

But I think we still have to remove it as this is not any sense to have
the same file for ARM, PPC and RISC-V.

Re: [PATCH v4 2/2] xen: move arm/include/asm/vm_event.h to asm-generic

[Oleksii]

On Mon, 2023-09-04 at 08:53 +0200, Jan Beulich wrote:
> On 01.09.2023 19:14, Oleksii Kurochko wrote:
> > The change which adds generic-y += vm_event.h to ARM's Kbuild was
> > lost
> > during creation of the patch. Should be added in the next patch
> > version
> 
> Which I guess is an indication that ...
> 
> > On Fri, Sep 1, 2023, 18:02 Oleksii Kurochko
> > 
> > wrote:
> > 
> > > asm/vm_event.h is common for ARM and RISC-V so it will be moved
> > > to
> > > asm-generic dir.
> > > 
> > > Original asm/vm_event.h from ARM was updated:
> > >  * use SPDX-License-Identifier.
> > >  * update comment messages of stubs.
> > >  * update #ifdef.
> > >  * change public/domctl.h to public/vm_event.h.
> > > 
> > > Signed-off-by: Oleksii Kurochko 
> > > Acked-by: Stefano Stabellini 
> 
> ... any earlier acks would better have been dropped.
You are right. I will remove acked-by in the next patch series version.

~ Oleksii

[xen-unstable test] 182623: tolerable FAIL

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-xl-rtds  8 xen-boot fail in 182620 pass in 182623
 test-armhf-armhf-libvirt-qcow2 18 guest-start.2  fail in 182620 pass in 182623
 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 12 debian-hvm-install fail pass 
in 182620

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stop  fail in 182620 like 182609
 test-amd64-amd64-xl-qemuu-win7-amd64 12 windows-install   fail like 182615
 test-amd64-i386-xl-qemuu-ovmf-amd64  7 xen-installfail like 182620
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 182620
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 182620
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 182620
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 182620
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 182620
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 182620
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 182620
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 182620
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 182620
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 182620
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 182620
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 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-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-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-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-i386-libvirt  15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-amd64-i386-libvirt-raw  14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-armhf-armhf-xl-vhd  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail   never pass

Re: [XEN][PATCH v11 14/20] common/device_tree: Add rwlock for dt_host

[Michal Orzel]

On 01/09/2023 06:59, Vikram Garhwal wrote:
> Dynamic programming ops will modify the dt_host and there might be other
> functions which are browsing the dt_host at the same time. To avoid the race
> conditions, adding rwlock for browsing the dt_host during runtime. dt_host
> writer will be added in the follow-up patch for device tree overlay
> functionalities.
> 
> Reason behind adding rwlock instead of spinlock:
> For now, dynamic programming is the sole modifier of dt_host in Xen during
> run time. All other access functions like iommu_release_dt_device() are
> just reading the dt_host during run-time. So, there is a need to protect
> others from browsing the dt_host while dynamic programming is modifying
> it. rwlock is better suitable for this task as spinlock won't be able to
> differentiate between read and write access.
> 
> Signed-off-by: Vikram Garhwal 
> Reviewed-by: Michal Orzel 
> ---
> Changes from v10:
> Add ASSERT for iommu_assign_dt_device() and iommu_add_dt_device().
> Changes from v9:
> Update commit message and fix indentation.
> Add ASSERT() for iommu_deassign_dt_device() and iommu_remove_dt_device().
> Fix code styles.
> Remove rwlock_init in unflatten_device_tree() and do DEFINE_RWLOCK in
> device-tree.c
> Changes from v7:
> Keep one lock for dt_host instead of lock for each node under dt_host.
> ---
> ---
>  xen/common/device_tree.c  |  1 +
>  xen/drivers/passthrough/device_tree.c | 28 +--
>  xen/include/xen/device_tree.h |  7 +++
>  3 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index f38f51ec0b..b1c2952951 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -31,6 +31,7 @@ dt_irq_xlate_func dt_irq_xlate;
>  struct dt_device_node *dt_host;
>  /* Interrupt controller node*/
>  const struct dt_device_node *dt_interrupt_controller;
> +DEFINE_RWLOCK(dt_host_lock);
>  
>  /**
>   * struct dt_alias_prop - Alias property in 'aliases' node
> diff --git a/xen/drivers/passthrough/device_tree.c 
> b/xen/drivers/passthrough/device_tree.c
> index 80f6efc606..1f9cfccf95 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -31,6 +31,8 @@ int iommu_assign_dt_device(struct domain *d, struct 
> dt_device_node *dev)
>  int rc = -EBUSY;
>  struct domain_iommu *hd = dom_iommu(d);
>  
> +ASSERT(system_state <= SYS_STATE_active || rw_is_locked(_host_lock));
This looks not right (I know Julien suggested this). The second part will be 
checked only if state > active i.e. suspend/resume.
I think this wants to be:
ASSERT(system_state < SYS_STATE_active || rw_is_locked(_host_lock));
so that once the state is >= active, we require dt_host_lock to be locked.

~Michal

Re: [XEN][PATCH v11 12/20] xen/smmu: Add remove_device callback for smmu_iommu ops

[Michal Orzel]

On 01/09/2023 06:59, Vikram Garhwal wrote:
> Add remove_device callback for removing the device entry from smmu-master 
> using
> following steps:
> 1. Find if SMMU master exists for the device node.
> 2. Check if device is currently in use.
Just like in v10: you are not checking it. I asked you to add a check following 
Julien suggestion
but you did not reply to it. Even if you do not want to add this extra layer of 
protection, you
should mention that you rely on a check in iommu_remove_dt_device() instead. 
You can wait for Stefano
to give his opinion (and possibly ack this patch as is).

~Michal

Re: [XEN][PATCH v11 11/20] xen/iommu: Introduce iommu_remove_dt_device()

[Michal Orzel]

On 01/09/2023 06:59, Vikram Garhwal wrote:
> Remove master device from the IOMMU. This will be helpful when removing the
> overlay nodes using dynamic programming during run time.
> 
> Signed-off-by: Vikram Garhwal 
> 
> ---
> Changes from v10:
> Add comment regarding return values of iommu_remove_dt_device().
> Add ASSERT to check if is_protected is removed or not.
> Changes from v7:
> Add check if IOMMU is enabled.
> Fix indentation of fail.
> ---
> ---
>  xen/drivers/passthrough/device_tree.c | 43 +++
>  xen/include/xen/iommu.h   | 10 +++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/device_tree.c 
> b/xen/drivers/passthrough/device_tree.c
> index 687c61e7da..80f6efc606 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -127,6 +127,49 @@ int iommu_release_dt_devices(struct domain *d)
>  return 0;
>  }
>  
> +int iommu_remove_dt_device(struct dt_device_node *np)
> +{
> +const struct iommu_ops *ops = iommu_get_ops();
> +struct device *dev = dt_to_dev(np);
> +int rc;
> +
> +if ( !iommu_enabled )
> +return 1;
> +
> +if ( !ops )
> +return -EOPNOTSUPP;
> +
> +spin_lock(_lock);
> +
> +if ( iommu_dt_device_is_assigned_locked(np) )
> +{
> +rc = -EBUSY;
> +goto fail;
> +}
> +
> +if ( !ops->remove_device )
> +{
> +rc = -EOPNOTSUPP;
> +goto fail;
> +}
> +
> +/*
> + * De-register the device from the IOMMU driver.
> + * The driver is responsible for removing is_protected flag.
> + */
> +rc = ops->remove_device(0, dev);
> +
> +if ( !rc )
> +{
> +ASSERT(!dt_device_is_protected(np));
> +iommu_fwspec_free(dev);
> +}
> +
> + fail:
> +spin_unlock(_lock);
> +return rc;
> +}
> +
>  int iommu_add_dt_device(struct dt_device_node *np)
>  {
>  const struct iommu_ops *ops = iommu_get_ops();
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index a18b68e247..84bd77395e 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -235,6 +235,16 @@ int iommu_add_dt_device(struct dt_device_node *np);
>  int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
> XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>  
> +/*
> + * Helper to remove master device from the IOMMU.
> + *
> + * Return values:
> + *  0 : device is de-registerd from IOMMU.
s/registerd/registered/

> + * <0 : error while removing the device from IOMMU.
> + * >0 : IOMMU is not enabled/present or device is not connected to it.
The first part refers to "iommu_enabled" but I cannot see how you check for the 
second part
(and return >0).

Apart from that:
Reviewed-by: Michal Orzel 

~Michal

Re: [PATCH] tools/misc/xencov_split: Add python 3 compatibility

[Alejandro Vallejo]

Hi,

On Mon, Sep 04, 2023 at 08:51:31AM +0200, Jan Beulich wrote:
> On 02.09.2023 18:21, Javi Merino wrote:
> > Closes #154
> > 
> > Signed-off-by: Javi Merino 
> 
> The title isn't really in line with ...
> 
> > --- a/tools/misc/xencov_split
> > +++ b/tools/misc/xencov_split
> > @@ -1,5 +1,7 @@
> > -#!/usr/bin/env python
> > +#!/usr/bin/env python3
> 
> ... this part of the change, and making Py3 a requirement here (assuming
> that's indeed necessary) surely wants adding a few words as description.
> Grep-ing the tools/ sub-tree I notice that so far we've avoided explicit
> uses of "python3", and I assume we would better continue doing so as on
> a distro with only Py3 a "python3" alias may legitimately not exist.
> 
> Jan
> 
The only credible reason for doing that would be to have scripts compatible
with both python2 and python3. Python2 has been deprecated since 2020 and
it's no longer security supported[1]. No one should be even remotely
encouraged to use that interpreter. The libs it uses are also in similar
positions. The recommended approach is to migrate as much as possible to
python3 and live happily ever after.

On that topic, the official PEP states:

(From https://peps.python.org/pep-0394/)

  Some Linux distributions will not provide a python command at all by
  default, but will provide a python3 command by default.

Which seems to imply the correct shebang is indeed `/usr/bin/env python3`

Thanks,
Alejandro

[PATCH 1/2] xen/arm: smmuv3: Add missing U for shifted constant

[Michal Orzel]

When running with SMMUv3 and UBSAN enabled, the following is printed:

(XEN) UBSAN: Undefined behaviour in drivers/passthrough/arm/smmu-v3.c:297:12
(XEN) left shift of 1 by 31 places cannot be represented in type 'int'

This refers to shift in Q_OVERFLOW_FLAG that is missing 'U' suffix.
While there, also fix the same in GBPA_UPDATE.

This should address MISRA Rule 7.2:
A "u" or "U" suffix shall be applied to all integer constants that
are represented in an unsigned type

Signed-off-by: Michal Orzel 
---
 xen/drivers/passthrough/arm/smmu-v3.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu-v3.h 
b/xen/drivers/passthrough/arm/smmu-v3.h
index b381ad373845..05f6b1fb7e33 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.h
+++ b/xen/drivers/passthrough/arm/smmu-v3.h
@@ -87,7 +87,7 @@
 #define CR2_E2H(1 << 0)
 
 #define ARM_SMMU_GBPA  0x44
-#define GBPA_UPDATE(1 << 31)
+#define GBPA_UPDATE(1U << 31)
 #define GBPA_ABORT (1 << 20)
 
 #define ARM_SMMU_IRQ_CTRL  0x50
@@ -159,7 +159,7 @@
 
 #define Q_IDX(llq, p)  ((p) & ((1 << (llq)->max_n_shift) - 1))
 #define Q_WRP(llq, p)  ((p) & (1 << (llq)->max_n_shift))
-#define Q_OVERFLOW_FLAG(1 << 31)
+#define Q_OVERFLOW_FLAG(1U << 31)
 #define Q_OVF(p)   ((p) & Q_OVERFLOW_FLAG)
 #define Q_ENT(q, p)((q)->base +\
 Q_IDX(&((q)->llq), p) *\
-- 
2.25.1

[PATCH 2/2] xen: Change parameter of generic_fls() to unsigned int

[Michal Orzel]

When running with SMMUv3 and UBSAN enabled on arm64, there are a lot of
warnings printed related to shifting into sign bit in generic_fls()
as it takes parameter of type int.

Example:
(XEN) UBSAN: Undefined behaviour in ./include/xen/bitops.h:69:11
(XEN) left shift of 134217728 by 4 places cannot be represented in type 'int'

It does not make a lot of sense to ask for the last set bit of a negative
value. We don't have a direct user of this helper and all the wrappers
pass value of type unsigned {int,long}.

Linux did the same as part of commit:
3fc2579e6f16 ("fls: change parameter to unsigned int")

Signed-off-by: Michal Orzel 
---
It looks like generic_fls() is only used by Arm and invoked only if
the arguement passed is a compile time constant. This is true for SMMUv3
which makes use of ffs64() in FIELD_{PREP,GET} macros.
---
 xen/include/xen/bitops.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
index 654f525fb437..b2d7bbd66687 100644
--- a/xen/include/xen/bitops.h
+++ b/xen/include/xen/bitops.h
@@ -51,7 +51,7 @@ static inline int generic_ffs(int x)
  * fls: find last bit set.
  */
 
-static __inline__ int generic_fls(int x)
+static __inline__ int generic_fls(unsigned int x)
 {
 int r = 32;
 
-- 
2.25.1

[PATCH 0/2] xen: fixes for UBSAN findings with SMMUv3 enabled

[Michal Orzel]

This series contains fixes for UBSAN findings reported when running
with SMMUv3 enabled.

Michal Orzel (2):
  xen/arm: smmuv3: Add missing U for shifted constant
  xen: Change parameter of generic_fls() to unsigned int

 xen/drivers/passthrough/arm/smmu-v3.h | 4 ++--
 xen/include/xen/bitops.h  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.25.1

Re: [XEN PATCH 1/2] xen: apply deviation for Rule 8.4 (asm-only definitions)

[Jan Beulich]

On 04.09.2023 09:19, Nicola Vetrini wrote:
> On 04/09/2023 09:02, Jan Beulich wrote:
>> Further in the cover letter you say "Deviating variables needs more 
>> care, and
>> is therefore postponed to another patch", yet then here you annotate a 
>> couple
>> of variables as well. Could you clarify what the criteria are for 
>> "needs more
>> care"?
> 
> I see. I did not intend for those changes to end up in this patch, 
> although those two are
> clearly only used by asm code and therefore are excepted.
> Most of the variables I left out are also used by C code, therefore they 
> may be eligible for
> a declaration, but where to put this declaration requires a careful 
> examination in my opinion.
> They are not too many, tough.

Anything also used by C code (and not in the same CU) clearly wants a
declaration.

Jan

[PATCH v1] automation: add awk to opensuse images

[Olaf Hering]

Some awk binary is used in many places during build,
make sure it is part of the image.

Signed-off-by: Olaf Hering 
---
 automation/build/suse/opensuse-leap.dockerfile   | 1 +
 automation/build/suse/opensuse-tumbleweed.dockerfile | 1 +
 2 files changed, 2 insertions(+)

diff --git a/automation/build/suse/opensuse-leap.dockerfile 
b/automation/build/suse/opensuse-leap.dockerfile
index 79de83ac20..98ee42970d 100644
--- a/automation/build/suse/opensuse-leap.dockerfile
+++ b/automation/build/suse/opensuse-leap.dockerfile
@@ -21,6 +21,7 @@ RUN zypper install -y --no-recommends \
 diffutils \
 discount \
 flex \
+gawk \
 gcc \
 gcc-c++ \
 git \
diff --git a/automation/build/suse/opensuse-tumbleweed.dockerfile 
b/automation/build/suse/opensuse-tumbleweed.dockerfile
index abb25c8c84..aed81f0240 100644
--- a/automation/build/suse/opensuse-tumbleweed.dockerfile
+++ b/automation/build/suse/opensuse-tumbleweed.dockerfile
@@ -21,6 +21,7 @@ RUN zypper install -y --no-recommends \
 diffutils \
 discount \
 flex \
+gawk \
 gcc \
 gcc-c++ \
 git \

Re: [XEN PATCH 1/2] xen: apply deviation for Rule 8.4 (asm-only definitions)

[Nicola Vetrini]

On 04/09/2023 09:02, Jan Beulich wrote:

On 01.09.2023 18:34, Nicola Vetrini wrote:
As stated in 'docs/misra/rules.rst' the functions that are used only 
by

asm modules do not need to conform to MISRA C:2012 Rule 8.4.
The deviations are carried out with a SAF comment.

Signed-off-by: Nicola Vetrini 
---
Where the identifier for a function definition is on the next line 
w.r.t. the
return type, they have been put on the same line (e.g. efi_start) to 
avoid

stylistically questionable constructs, such as

int
/* SAF-1-safe */
func(void) {
...
}


And

/* SAF-1-safe */
int
func(void) {

is not an option?



Not at the moment, as it would deviate the line with the return type and 
not the one below,
and this is not configurable in the scripts under 
xen/scripts/xen-analysis:


/* SAF-1-safe */ -> /* -E> safe ... 1 */
int int
func(void)  func(void)

As I said, this can perhaps be solved by allowing markers to specify 
either a row count, such as


/* SAF-1-safe 2 */ -> /* -E> safe ... 2 */
int   int
func(void)func(void)

or count the line span of the whole function declarator in the python 
script and translating

/* SAF-1-safe */ -> /* -E> safe ... 2 */.

Further in the cover letter you say "Deviating variables needs more 
care, and
is therefore postponed to another patch", yet then here you annotate a 
couple
of variables as well. Could you clarify what the criteria are for 
"needs more

care"?



I see. I did not intend for those changes to end up in this patch, 
although those two are

clearly only used by asm code and therefore are excepted.
Most of the variables I left out are also used by C code, therefore they 
may be eligible for
a declaration, but where to put this declaration requires a careful 
examination in my opinion.

They are not too many, tough.

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 1/2] xen: apply deviation for Rule 8.4 (asm-only definitions)

[Jan Beulich]

On 01.09.2023 18:34, Nicola Vetrini wrote:
> As stated in 'docs/misra/rules.rst' the functions that are used only by
> asm modules do not need to conform to MISRA C:2012 Rule 8.4.
> The deviations are carried out with a SAF comment.
> 
> Signed-off-by: Nicola Vetrini 
> ---
> Where the identifier for a function definition is on the next line w.r.t. the
> return type, they have been put on the same line (e.g. efi_start) to avoid
> stylistically questionable constructs, such as
> 
> int
> /* SAF-1-safe */
> func(void) {
>   ...
> }

And

/* SAF-1-safe */
int
func(void) {

is not an option?

Further in the cover letter you say "Deviating variables needs more care, and
is therefore postponed to another patch", yet then here you annotate a couple
of variables as well. Could you clarify what the criteria are for "needs more
care"?

Jan

Re: [PATCH v4 2/2] xen: move arm/include/asm/vm_event.h to asm-generic

[Jan Beulich]

On 01.09.2023 18:02, Oleksii Kurochko wrote:
> asm/vm_event.h is common for ARM and RISC-V so it will be moved to
> asm-generic dir.
> 
> Original asm/vm_event.h from ARM was updated:
>  * use SPDX-License-Identifier.
>  * update comment messages of stubs.
>  * update #ifdef.
>  * change public/domctl.h to public/vm_event.h.
> 
> Signed-off-by: Oleksii Kurochko 
> Acked-by: Stefano Stabellini 
> ---
> Changes in V4:
>  - update path of vm_event.h from include/asm-generic/asm to 
> include/asm-generic
> ---
> Changes in V3:
>  - add Acked-by: Stefano Stabellini  for "xen: move 
> arm/include/asm/vm_event.h to asm-generic"
>  - update SPDX tag.
>  - move asm/vm_event.h to asm-generic.
> ---
> Changes in V2:
>  - change public/domctl.h to public/vm_event.h.
>  - update commit message of [PATCH v2 2/2] xen: move 
> arm/include/asm/vm_event.h to stubs
> ---
>  xen/include/asm-generic/vm_event.h | 55 ++
>  1 file changed, 55 insertions(+)
>  create mode 100644 xen/include/asm-generic/vm_event.h

But that's not "move" anymore, as the Arm header isn't being deleted.

Jan

Re: [PATCH v4 2/2] xen: move arm/include/asm/vm_event.h to asm-generic

[Jan Beulich]

On 01.09.2023 19:14, Oleksii Kurochko wrote:
> The change which adds generic-y += vm_event.h to ARM's Kbuild was lost
> during creation of the patch. Should be added in the next patch version

Which I guess is an indication that ...

> On Fri, Sep 1, 2023, 18:02 Oleksii Kurochko 
> wrote:
> 
>> asm/vm_event.h is common for ARM and RISC-V so it will be moved to
>> asm-generic dir.
>>
>> Original asm/vm_event.h from ARM was updated:
>>  * use SPDX-License-Identifier.
>>  * update comment messages of stubs.
>>  * update #ifdef.
>>  * change public/domctl.h to public/vm_event.h.
>>
>> Signed-off-by: Oleksii Kurochko 
>> Acked-by: Stefano Stabellini 

... any earlier acks would better have been dropped.

Jan

Re: [PATCH] tools/misc/xencov_split: Add python 3 compatibility

[Jan Beulich]

On 02.09.2023 18:21, Javi Merino wrote:
> Closes #154
> 
> Signed-off-by: Javi Merino 

The title isn't really in line with ...

> --- a/tools/misc/xencov_split
> +++ b/tools/misc/xencov_split
> @@ -1,5 +1,7 @@
> -#!/usr/bin/env python
> +#!/usr/bin/env python3

... this part of the change, and making Py3 a requirement here (assuming
that's indeed necessary) surely wants adding a few words as description.
Grep-ing the tools/ sub-tree I notice that so far we've avoided explicit
uses of "python3", and I assume we would better continue doing so as on
a distro with only Py3 a "python3" alias may legitimately not exist.

Jan

Re: [QEMU PATCH v4 06/13] virtio-gpu: Configure context init for virglrenderer

[Huang Rui]

On Thu, Aug 31, 2023 at 05:39:38PM +0800, Philippe Mathieu-Daudé wrote:
> On 31/8/23 11:32, Huang Rui wrote:
> > Configure context init feature flag for virglrenderer.
> > 
> > Originally-by: Antonio Caggiano 
> > Signed-off-by: Huang Rui 
> > ---
> > 
> > New patch, result of splitting
> > [RFC QEMU PATCH 04/18] virtio-gpu: CONTEXT_INIT feature
> > 
> >   meson.build | 4 
> >   1 file changed, 4 insertions(+)
> > 
> > diff --git a/meson.build b/meson.build
> > index 98e68ef0b1..ff20d3c249 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1068,6 +1068,10 @@ if not get_option('virglrenderer').auto() or 
> > have_system or have_vhost_user_gpu
> >  prefix: '#include 
> > ',
> >  dependencies: virgl))
> > endif
> > +  config_host_data.set('HAVE_VIRGL_CONTEXT_INIT',
> > +   
> > cc.has_function('virgl_renderer_context_create_with_flags',
> > +   prefix: '#include 
> > ',
> > +   dependencies: virgl))
> 
> Shouldn't this be inverted with previous patch?
> 

Yes, this should be patch 3 because we should configure
HAVE_VIRGL_CONTEXT_INIT firstly. I will update it in next version.

Thanks
Ray

Re: [PATCH v4 3/3] xen/ppc: Implement initial Radix MMU support

[Jan Beulich]

On 01.09.2023 19:52, Shawn Anastasio wrote:
> On 8/23/23 12:36 PM, Shawn Anastasio wrote:
>> On 8/23/23 9:04 AM, Jan Beulich wrote:
>>> On 23.08.2023 01:03, Shawn Anastasio wrote:
 Add code to construct early identity-mapped page tables as well as the
 required process and partition tables to enable the MMU.

 Signed-off-by: Shawn Anastasio 
>>>
>>> Acked-by: Jan Beulich 
> 
> Just a quick ping to see if you are still good to merge this patch. If
> you'd like me to submit a new revision let me know.

Ah, yes, I managed to overlook this (perhaps more than once) because the
earlier two went in already. I'll try to remember to include this in the
next batch to push out.

Jan