Re: Lockdep show 6.6-rc regression in Xen HVM CPU hotplug

[Juergen Gross]

On 23.10.23 22:50, David Woodhouse wrote:

Since commit 87797fad6cce ("xen/events: replace evtchn_rwlock with
RCU"), I can no longer hotplug CPUs under Xen with lockdep enabled.

(This is real Xen 4.17.1; annoyingly I have different failure modes
with Xen guests under qemu/kvm and I'll deal with those next.)

Offlining complains thus:

[root@localhost cpu1]# echo 0 > online
[   52.936265]
[   52.936271] =
[   52.936274] WARNING: suspicious RCU usage
[   52.936277] 6.6.0-rc5+ #1357 Not tainted
[   52.936280] -
[   52.936282] lib/maple_tree.c:856 suspicious rcu_dereference_check() usage!
[   52.936287]
[   52.936287] other info that might help us debug this:
[   52.936287]
[   52.936289]
[   52.936289] RCU used illegally from offline CPU!
[   52.936289] rcu_scheduler_active = 2, debug_locks = 1
[   52.936293] 1 lock held by swapper/1/0:
[   52.936296]  #0: 89c03820 (rcu_read_lock){}-{1:3}, at: 
mtree_load+0x90/0x590
[   52.936321]
[   52.936321] stack backtrace:
[   52.936324] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 6.6.0-rc5+ #1357
[   52.936329] Hardware name: Xen HVM domU, BIOS 4.17.1 09/26/2023
[   52.936332] Call Trace:
[   52.936335]  
[   52.936340]  dump_stack_lvl+0x5b/0x90
[   52.936350]  lockdep_rcu_suspicious+0x15a/0x1c0
[   52.936366]  mtree_load+0x49e/0x590
[   52.936385]  irq_get_irq_data+0xe/0x20
[   52.936394]  xen_send_IPI_one+0xa4/0x100
[   52.936404]  __xen_send_IPI_mask+0x1b/0x50
[   52.936414]  generic_exec_single+0x35/0x1c0
[   52.936423]  smp_call_function_single+0xc2/0x140
[   52.936436]  ? cpuhp_report_idle_dead+0x42/0x60
[   52.936444]  do_idle+0xda/0xe0
[   52.936451]  cpu_startup_entry+0x2a/0x30
[   52.936456]  start_secondary+0x123/0x140
[   52.936465]  secondary_startup_64_no_verify+0x178/0x17b
[   52.936490]  


I'm puzzled. This path doesn't contain any of the RCU usage I've added in
commit 87797fad6cce.

Are you sure that with just reverting commit 87797fad6cce the issue doesn't
manifest anymore? I'd rather expect commit 721255b9826b having caused this
behavior, just telling from the messages above.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH] MAINTAINERS: make Michal Orzel ARM Maintainer

[Michal Orzel]

Hi Stefano,

Thanks!

On 23/10/2023 22:56, Stefano Stabellini wrote:
> 
> 
> Signed-off-by: Stefano Stabellini 
Acked-by: Michal Orzel 

~Michal

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Jan Beulich]

On 24.10.2023 00:05, Stefano Stabellini wrote:
> On Mon, 23 Oct 2023, Jan Beulich wrote:
>> On 23.10.2023 17:23, Simone Ballarin wrote:
>>> On 23/10/23 15:34, Jan Beulich wrote:
 On 18.10.2023 16:18, Simone Ballarin wrote:
> --- a/xen/include/xen/pdx.h
> +++ b/xen/include/xen/pdx.h
> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long 
> pfn)
>* @param pdx Page index
>* @return Obtained pfn after decompressing the pdx
>*/
> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long 
> pdx)
>   {
>   return (pdx & pfn_pdx_bottom_mask) |
>  ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);

 Taking this as an example for what I've said above: The compiler can't
 know that the globals used by the functions won't change value. Even
 within Xen it is only by convention that these variables are assigned
 their values during boot, and then aren't changed anymore. Which makes
 me wonder: Did you check carefully that around the time the variables
 have their values established, no calls to the functions exist (which
 might then be subject to folding)?
>>>
>>> There is no need to check that, the GCC documentation explicitly says:
>>>
>>> However, functions declared with the pure attribute *can safely read any 
>>> non-volatile objects*, and modify the value of objects in a way that 
>>> does not affect their return value or the observable state of the program.
>>
>> I did quote this same text in response to what Andrew has said, but I also
>> did note there that this needs to be taken with a grain of salt: The
>> compiler generally assumes a single-threaded environment, i.e. no changes
>> to globals behind the back of the code it is processing.
> 
> Let's start from the beginning. The reason for Simone to add
> __attribute_pure__ to pdx_to_pfn and other functions is for
> documentation purposes. It is OK if it doesn't serve any purpose other
> than documentation.
> 
> Andrew, for sure we do not want to lie to the compiler and introduce
> undefined behavior. If we think there is a risk of it, we should not do
> it.
> 
> So, what do we want to document? We want to document that the function
> does not have side effects according to MISRA's definition of it, which
> might subtly differ from GCC's definition.
> 
> Looking at GCC's definition of __attribute_pure__, with the
> clarification statement copy/pasted above by both Simone and Jan, it
> seems that __attribute_pure__ matches MISRA's definition of a function
> without side effects. It also seems that pdx_to_pfn abides to that
> definition.
> 
> Jan has a point that GCC might be making other assumptions
> (single-thread execution) that might not hold true in our case. Given
> the way the GCC statement is written I think this is low risk. But maybe
> not all GCC versions we want to support in the project might have the
> same definition of __attribute_pure__. So we could end up using
> __attribute_pure__ correctly for the GCC version used for safety (GCC
> 12.1, see docs/misra/C-language-toolchain.rst) but it might actually
> break an older GCC version.
> 
> 
> So Option#1 is to use __attribute_pure__ taking the risk that a GCC or
> Clang version might interpret __attribute_pure__ differently and
> potentially misbehave.
> 
> Option#2 is to avoid this risk, by not using __attribute_pure__.
> Instead, we can use SAF-xx-safe or deviations.rst to document that
> pdx_to_pfn and other functions like it are without side effects
> according to MISRA's definition.
> 
> 
> Both options have pros and cons. To me the most important factor is how
> many GCC versions come with the statement "pure attribute can safely
> read any non-volatile objects, and modify the value of objects in a way
> that does not affect their return value or the observable state of the
> program".
> 
> I checked and these are the results:
> - gcc 4.0.2: no statement
> - gcc 5.1.0: no statement
> - gcc 6.1.0: no statement
> - gcc 7.1.0: no statement
> - gcc 8.1.0: alternative statement "The pure attribute imposes similar
>   but looser restrictions on a function’s definition than the const
>   attribute: it allows the function to read global variables."
> - gcc 9.1.0: yes statement
> 
> 
> So based on the above, __attribute_pure__ comes with its current
> definition only from gcc 9 onward. I don't know if as a Xen community we
> clearly declare a range of supported compilers, but I would imagine we
> would still want to support gcc versions older than 9? (Not to mention
> clang, which I haven't checked.)
> 
> It doesn't seem to me that __attribute_pure__ could be correctly used on
> pdx_to_pfn with GCC 7.1.0 for example.

The absence of documentation doesn't mean the attribute had different
(or even undefined) meaning in earlier versions. Instead it means one
would need to consult other places (sourc

Re: [XEN PATCH][for-4.19 v4 1/8] xen: modify or add declarations for variables where needed

[Jan Beulich]

On 24.10.2023 01:03, Stefano Stabellini wrote:
> On Mon, 23 Oct 2023, Nicola Vetrini wrote:
>> Some variables with external linkage used in C code do not have
>> a visible declaration where they are defined. Other variables
>> can become static, thereby eliminating the need for a declaration.
>> Doing so also resolves violations of MISRA C:2012 Rule 8.4.
>>
>> Fix typo s/mcinfo_dumpped/mcinfo_dumped/ while making
>> the variable static.
>>
>> Signed-off-by: Nicola Vetrini 
> 
> Reviewed-by: Stefano Stabellini 

(for x86)
Acked-by: Jan Beulich 

Re: [XEN PATCH][for-4.19 v3 1/8] xen/include: add macro LOWEST_BIT

[Jan Beulich]

On 23.10.2023 22:44, Stefano Stabellini wrote:
> On Mon, 23 Oct 2023, Jan Beulich wrote:
>> On 23.10.2023 15:19, Nicola Vetrini wrote:
>>> On 23/10/2023 11:19, Nicola Vetrini wrote:
 On 23/10/2023 09:48, Jan Beulich wrote:
> On 20.10.2023 17:28, Nicola Vetrini wrote:
>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -246,6 +246,12 @@ constant expressions are required.\""
>>"any()"}
>>  -doc_end
>>
>> +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern 
>> to obtain the value
>> +2^ffs(x) for unsigned integers on two's complement architectures
>> +(all the architectures supported by Xen satisfy this requirement)."
>> +-config=MC3R1.R10.1,reports+={safe, 
>> "any_area(any_loc(any_exp(macro(^LOWEST_BIT$"}
>> +-doc_end
>
> This being deviated this way (rather than by SAF-* comments) wants
> justifying in the description. You did reply to my respective
> comment on v2, but such then (imo) needs propagating into the actual
> patch as well.
>

 Sure, will do.

>> --- a/docs/misra/deviations.rst
>> +++ b/docs/misra/deviations.rst
>> @@ -192,6 +192,13 @@ Deviations related to MISRA C:2012 Rules:
>> See automation/eclair_analysis/deviations.ecl for the full 
>> explanation.
>>   - Tagged as `safe` for ECLAIR.
>>
>> +   * - R10.1
>> + - The well-known pattern (x & -x) applied to unsigned integer 
>> values on 2's
>> +   complement architectures (i.e., all architectures supported 
>> by Xen), used
>> +   to obtain the value 2^ffs(x), where ffs(x) is the position of 
>> the first
>> +   bit set. If no bits are set, zero is returned.
>> + - Tagged as `safe` for ECLAIR.
>
> In such an explanation there shall not be any ambiguity. Here I see
> an issue with ffs() returning 1-based bit position numbers, which
> isn't in line with the use in 2^ffs(x). (Arguably use of ^ itself is
> also problematic, as that's XOR in C, not POW. I'd suggest 2^^ffs(x)
> or 2**ffs(x).)
>

 Makes sense, I think I'll use an plain english explanation to avoid
 any confusion
 about notation.

>> --- a/xen/include/xen/macros.h
>> +++ b/xen/include/xen/macros.h
>> @@ -8,8 +8,11 @@
>>  #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
>>  #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
>>
>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
>> +/* Returns the 2^ffs(x) or 0, where ffs(x) is the index of the 
>> lowest set bit */
>> +#define LOWEST_BIT(x) ((x) & -(x))
>
> I'm afraid my concern regarding this new macro's name (voiced on v2) 
> hasn't
> been addressed (neither verbally nor by finding a more suitable name).
>
> Jan

 I didn't come up with much better names, so I left it as is. Here's a 
 few ideas:

 - LOWEST_SET
 - MASK_LOWEST_SET
 - MASK_LOWEST_BIT
 - MASK_FFS_0
 - LOWEST_ONE

 and also yours, included here for convenience, even though you felt it
 was too long:

 - ISOLATE_LOW_BIT()

 All maintainers from THE REST are CC-ed, so we can see if anyone has
 any suggestion.
>>>
>>> There's also LOWEST_BIT_MASK as another possible name.
>>
>> While naming-wise okay to me, it has the same "too long" issue as
>> ISOLATE_LOW_BIT(). Considering x86, in the BMI ISA extension, has an
>> insn doing exactly that (BLSI), taking inspiration from its mnemonic
>> may also be an option.
> 
> I don't mean to make things difficult but I prefer LOWEST_BIT or
> MASK_LOWEST_BIT. It is clear at first glance. BLSI is not as clear,
> unless you work on the specific ISA with BLSI.
> 
> In general, I do appreciate shorter names, but if one option is much
> clearer than the other, I prefer clarity over shortness.

That's fine with me, but note that neither LOWEST_BIT nor MASK_LOWEST_BIT
really provide the aimed at clarity. The shortest that I could think of
that would be derived from that would be LOWEST_SET_BIT_MASK() (albeit
even that leaves a bit of ambiguity, thinking about it for a little
longer). The main point I'm trying to make that _if_ we need a wrapper
macro for this in the first place (note the other thread about macros
still requiring deviation comments at all use sites for Eclair), its name
needs to somehow express the precise operation it does (or, like would be
the case for BLSI, make people not recognizing the name go look rather
than guess).

Jan

Re: Refactor arm64/domctl.c 'subarch_do_domctl' to avoid unreachable break.

[Jan Beulich]

On 23.10.2023 18:07, Julien Grall wrote:
> Hi Jan,
> 
> On 23/10/2023 16:15, Jan Beulich wrote:
>> On 23.10.2023 17:00, Julien Grall wrote:
>>>
>>>
>>> On 23/10/2023 15:51, Nicola Vetrini wrote:
 Hi,
>>>
>>> Hi Nicola,
>>>
 while taking care of some patches regarding MISRA C Rule 2.1 (code
 shouldn't be unreachable), I
 came across this function:

 long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
      XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 {
       switch ( domctl->cmd )
       {
       case XEN_DOMCTL_set_address_size:
       switch ( domctl->u.address_size.size )
       {
       case 32:
       if ( !cpu_has_el1_32 )
       return -EINVAL;
       /* SVE is not supported for 32 bit domain */
       if ( is_sve_domain(d) )
       return -EINVAL;
       return switch_mode(d, DOMAIN_32BIT);
       case 64:
       return switch_mode(d, DOMAIN_64BIT);
       default:
       return -EINVAL;
       }
       break;

       default:
       return -ENOSYS;
       }
 }

 here the break after the innermost switch is clearly unreachable, but
 it's also guarding a possible fallthrough.
 I can see a couple of solutions to this:

 - mark the part after the switch unreachable;
 - introduce a variable 'long rc' to store the return value, and
 consequently rework the control flow of all the switches
     (e.g. rc = -EINVAL and similar);
 - remove the break, but I consider this a risky move, unless -ENOSYS
 would be an ok value to be returned if some case
     from the switch above does not have a return statement.
>>>
>>> - move the nested switch in a separate function, so the code in
>>> subarch_do_domctl() can be replaced with:
>>>
>>> return set_address_size(...);
>>
>> But that would help only if inside the new function you still re-
>> layout the switch() (or replace it by, say, if/else-if/else),
>> wouldn't it?
> 
> I am not sure why I would need to re-layout the switch. This should work 
> (untested):
> 
> diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
> index 14fc622e9956..8720d126c97d 100644
> --- a/xen/arch/arm/arm64/domctl.c
> +++ b/xen/arch/arm/arm64/domctl.c
> @@ -33,27 +33,31 @@ static long switch_mode(struct domain *d, enum 
> domain_type type)
>   return 0;
>   }
> 
> +static long set_address_size(struct domain *d, uint32_t address_size)
> +{
> +switch ( address_size )
> +{
> +case 32:
> +if ( !cpu_has_el1_32 )
> +return -EINVAL;
> +/* SVE is not supported for 32 bit domain */
> +if ( is_sve_domain(d) )
> +return -EINVAL;
> +return switch_mode(d, DOMAIN_32BIT);
> +case 64:
> +return switch_mode(d, DOMAIN_64BIT);
> +default:
> +return -EINVAL;
> +}
> +}

Well, yes, if you're happy to have a function returning a value without
a return statement at its end.

Jan

Re: [PATCH for-4.19] xen/arm64: domctl: Avoid unreachable code in subarch_do_domctl()

[Henry Wang]

Hi Julien,

(+Stefano and Bertrand)

> On Oct 24, 2023, at 01:52, Julien Grall  wrote:
> 
> From: Julien Grall 
> 
> The 'break' the XEN_DOMCTL_set_address_size is unreachable and tools
> like Eclair will report as a violation of Misra Rule 2.1.
> 
> Furthermore, the nested switch is not very easy to read. So move
> out the nested switch in a separate function to improve the
> readability and hopefully address the MISRA violation.
> 
> Reported-by: Nicola Vetrini 
> Signed-off-by: Julien Grall 

Reviewed-by: Henry Wang 

> ---
> Only compiled tested. Waiting for the CI to confirm there is no
> regression.

I also tested this patch on top of today’s staging in Arm’s internal CI, and 
this
patch looks good.

Tested-by: Henry Wang 

Kind regards,
Henry

Re: [PATCH 1/2] x86/xen/pvh: Set up percpu for stack canary in 32-bit kernel entry

[Hou Wenlong]

On Mon, Oct 23, 2023 at 08:02:02PM +0800, Andy Shevchenko wrote:
> On Mon, Oct 23, 2023 at 12:10 PM Hou Wenlong
>  wrote:
> >
> > In a 32-bit SMP kernel, the stack canary is a percpu variable accessed
> > as %fs:__stack_chk_guard. However, the ABI for PVH entry does not
> > specify the %fs register state. It currently works because the initial
> > %fs register is 0x10 for QEMU, which is the same as $PVH_DS_SEL.
> 
> > %However, for added safety, the percpu should be set up explicitly
> > %before calling xen_prepare_pvh(), which accesses the stack canary.
> 
> Stray leading % in two lines above.
>
Oh, sorry for that. It was added by mistake by my editor, and I didn't
carefully review it before sending.
 
Thanks!

> -- 
> With Best Regards,
> Andy Shevchenko

[linux-linus test] 183499: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 linuxe017769f4ce20dc0d3fa3220d4d359dcc4431274
baseline version:
 linux05d3ef8bba77c1b5f98d941d8b2d4aeab8118ef1

Last test of basis   183494  2023-10-23 02:28:17 Z0 days
Testing same since   183499  2023-10-23 18:11:24 Z0 days1 attempts


People who touched revisions under test:
  Catalin Marinas 
  David Sterba 
  Dragos Tatulea 
  Eric Auger 
  Filipe Manana 
  Gavin Shan 
  Jason Wang 
  Liming Wu 
  Linus Torvalds 
  Maximilian Heyne 
  Michael S. Tsirkin 
  Shawn.Shao 
  Xuan Zhuo 
  zhenwei pi 
  Zhenyu Zhang 

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

Re: [PATCH] MAINTAINERS: make Michal Orzel ARM Maintainer

[Henry Wang]

Hi Stefano,

> On Oct 24, 2023, at 04:56, Stefano Stabellini  wrote:
> 
> Signed-off-by: Stefano Stabellini 

I saw I was CCed so:

Release-acked-by: Henry Wang 

Kind regards,
Henry

> ---
> MAINTAINERS | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f61b5a32a1..a5a5f2bffb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -246,6 +246,7 @@ ARM (W/ VIRTUALISATION EXTENSIONS) ARCHITECTURE
> M: Stefano Stabellini 
> M: Julien Grall 
> M: Bertrand Marquis 
> +M: Michal Orzel 
> R: Volodymyr Babchuk 
> S: Supported
> L: xen-devel@lists.xenproject.org
> -- 
> 2.25.1
> 

[ovmf test] 183501: all pass - PUSHED

[osstest service owner]

flight 183501 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/183501/

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf 2ad52adb6606f89f0c475faa6552fee69c02d082
baseline version:
 ovmf 309450db268c8721afa102c7c49adccd153b0e88

Last test of basis   183500  2023-10-23 19:40:49 Z0 days
Testing same since   183501  2023-10-23 22:12:16 Z0 days1 attempts


People who touched revisions under test:
  Joey Vagedes 
  Joey Vagedes 

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
   309450db26..2ad52adb66  2ad52adb6606f89f0c475faa6552fee69c02d082 -> 
xen-tested-master

[PATCH] misra: add R14.4 R21.1 R21.2

[Stefano Stabellini]

Add 14.4, with the same note and exception already listed for 10.1.

Add 21.1 and 21.2, with a longer comment to explain how strategy with
leading underscores and why we think we are safe today.

Signed-off-by: Stefano Stabellini 

diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
index b423580b23..56eec8bafd 100644
--- a/docs/misra/rules.rst
+++ b/docs/misra/rules.rst
@@ -422,6 +422,13 @@ maintainers if you want to suggest a change.
 
while(0) and while(1) and alike are allowed.
 
+   * - `Rule 14.4 
`_
+ - Required
+ - The controlling expression of an if statement and the controlling
+   expression of an iteration-statement shall have essentially
+   Boolean type
+ - Implicit conversions to boolean are allowed
+
* - `Rule 16.7 
`_
  - Required
  - A switch-expression shall not have essentially Boolean type
@@ -479,6 +486,24 @@ maintainers if you want to suggest a change.
they are related
  -
 
+   * - `Rule 21.1 
`_
+ - Required
+ - #define and #undef shall not be used on a reserved identifier or
+   reserved macro name
+ - No identifiers should start with _BUILTIN to avoid clashes with
+   GCC reserved identifiers. In general, all identifiers starting with
+   an underscore are reserved, and are best avoided. However, Xen
+   makes extensive usage of leading underscores in header guards,
+   bitwise manipulation functions, and a few other places. They are
+   considered safe as checks have been done against the list of
+   GCC's reserved identifiers. They don't need to be replaced.
+
+   * - `Rule 21.2 
`_
+ - Required
+ - A reserved identifier or reserved macro name shall not be
+   declared
+ - See comment for Rule 21.1
+
* - `Rule 21.13 
`_
  - Mandatory
  - Any value passed to a function in  shall be representable as an

Re: [XEN PATCH][for-4.19 v4 1/8] xen: modify or add declarations for variables where needed

[Stefano Stabellini]

On Mon, 23 Oct 2023, Nicola Vetrini wrote:
> Some variables with external linkage used in C code do not have
> a visible declaration where they are defined. Other variables
> can become static, thereby eliminating the need for a declaration.
> Doing so also resolves violations of MISRA C:2012 Rule 8.4.
> 
> Fix typo s/mcinfo_dumpped/mcinfo_dumped/ while making
> the variable static.
> 
> Signed-off-by: Nicola Vetrini 

Reviewed-by: Stefano Stabellini 

Re: [XEN PATCH][for-4.19 v3 7/8] xen/types: address Rule 10.1 for DECLARE_BITMAP use

[Stefano Stabellini]

On Fri, 20 Oct 2023, Nicola Vetrini wrote:
> Given its use in the declaration
> 'DECLARE_BITMAP(features, IOMMU_FEAT_count)' the argument
> 'bits' has essential type 'enum iommu_feature', which is not
> allowed by the Rule as an operand to the addition operator
> in macro 'BITS_TO_LONGS'.
> 
> This construct is deviated with a deviation comment.
> 
> Signed-off-by: Nicola Vetrini 

Reviewed-by: Stefano Stabellini 

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Stefano Stabellini]

> It doesn't seem to me that __attribute_pure__ could be correctly used on
> pdx_to_pfn with GCC 7.1.0 for example.
> 
> So in conclusion, I think it is better to avoid __attribute_pure__ and
> use SAF-xx-safe or an alternative approach instead.

I was thinking that another option would be to introduce a macro "pure"
that is defined as __attribute_pure__ for GCC 9 or later and defined to
nothing for GCC 8 or older.

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Stefano Stabellini]

On Mon, 23 Oct 2023, Jan Beulich wrote:
> On 23.10.2023 17:23, Simone Ballarin wrote:
> > On 23/10/23 15:34, Jan Beulich wrote:
> >> On 18.10.2023 16:18, Simone Ballarin wrote:
> >>> --- a/xen/include/xen/pdx.h
> >>> +++ b/xen/include/xen/pdx.h
> >>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long 
> >>> pfn)
> >>>* @param pdx Page index
> >>>* @return Obtained pfn after decompressing the pdx
> >>>*/
> >>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
> >>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long 
> >>> pdx)
> >>>   {
> >>>   return (pdx & pfn_pdx_bottom_mask) |
> >>>  ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
> >>
> >> Taking this as an example for what I've said above: The compiler can't
> >> know that the globals used by the functions won't change value. Even
> >> within Xen it is only by convention that these variables are assigned
> >> their values during boot, and then aren't changed anymore. Which makes
> >> me wonder: Did you check carefully that around the time the variables
> >> have their values established, no calls to the functions exist (which
> >> might then be subject to folding)?
> > 
> > There is no need to check that, the GCC documentation explicitly says:
> > 
> > However, functions declared with the pure attribute *can safely read any 
> > non-volatile objects*, and modify the value of objects in a way that 
> > does not affect their return value or the observable state of the program.
> 
> I did quote this same text in response to what Andrew has said, but I also
> did note there that this needs to be taken with a grain of salt: The
> compiler generally assumes a single-threaded environment, i.e. no changes
> to globals behind the back of the code it is processing.

Let's start from the beginning. The reason for Simone to add
__attribute_pure__ to pdx_to_pfn and other functions is for
documentation purposes. It is OK if it doesn't serve any purpose other
than documentation.

Andrew, for sure we do not want to lie to the compiler and introduce
undefined behavior. If we think there is a risk of it, we should not do
it.

So, what do we want to document? We want to document that the function
does not have side effects according to MISRA's definition of it, which
might subtly differ from GCC's definition.

Looking at GCC's definition of __attribute_pure__, with the
clarification statement copy/pasted above by both Simone and Jan, it
seems that __attribute_pure__ matches MISRA's definition of a function
without side effects. It also seems that pdx_to_pfn abides to that
definition.

Jan has a point that GCC might be making other assumptions
(single-thread execution) that might not hold true in our case. Given
the way the GCC statement is written I think this is low risk. But maybe
not all GCC versions we want to support in the project might have the
same definition of __attribute_pure__. So we could end up using
__attribute_pure__ correctly for the GCC version used for safety (GCC
12.1, see docs/misra/C-language-toolchain.rst) but it might actually
break an older GCC version.


So Option#1 is to use __attribute_pure__ taking the risk that a GCC or
Clang version might interpret __attribute_pure__ differently and
potentially misbehave.

Option#2 is to avoid this risk, by not using __attribute_pure__.
Instead, we can use SAF-xx-safe or deviations.rst to document that
pdx_to_pfn and other functions like it are without side effects
according to MISRA's definition.


Both options have pros and cons. To me the most important factor is how
many GCC versions come with the statement "pure attribute can safely
read any non-volatile objects, and modify the value of objects in a way
that does not affect their return value or the observable state of the
program".

I checked and these are the results:
- gcc 4.0.2: no statement
- gcc 5.1.0: no statement
- gcc 6.1.0: no statement
- gcc 7.1.0: no statement
- gcc 8.1.0: alternative statement "The pure attribute imposes similar
  but looser restrictions on a function’s definition than the const
  attribute: it allows the function to read global variables."
- gcc 9.1.0: yes statement


So based on the above, __attribute_pure__ comes with its current
definition only from gcc 9 onward. I don't know if as a Xen community we
clearly declare a range of supported compilers, but I would imagine we
would still want to support gcc versions older than 9? (Not to mention
clang, which I haven't checked.)

It doesn't seem to me that __attribute_pure__ could be correctly used on
pdx_to_pfn with GCC 7.1.0 for example.

So in conclusion, I think it is better to avoid __attribute_pure__ and
use SAF-xx-safe or an alternative approach instead.

[ovmf test] 183500: all pass - PUSHED

[osstest service owner]

flight 183500 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/183500/

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf 309450db268c8721afa102c7c49adccd153b0e88
baseline version:
 ovmf ec7f73436646a9232c6494d1ce23fb38000e10d3

Last test of basis   183498  2023-10-23 17:10:46 Z0 days
Testing same since   183500  2023-10-23 19:40:49 Z0 days1 attempts


People who touched revisions under test:
  Jeshua Smith 
  Leif Lindholm 

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
   ec7f734366..309450db26  309450db268c8721afa102c7c49adccd153b0e88 -> 
xen-tested-master

[PATCH] MAINTAINERS: make Michal Orzel ARM Maintainer

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f61b5a32a1..a5a5f2bffb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -246,6 +246,7 @@ ARM (W/ VIRTUALISATION EXTENSIONS) ARCHITECTURE
 M: Stefano Stabellini 
 M: Julien Grall 
 M: Bertrand Marquis 
+M: Michal Orzel 
 R: Volodymyr Babchuk 
 S: Supported
 L: xen-devel@lists.xenproject.org
-- 
2.25.1

Lockdep show 6.6-rc regression in Xen HVM CPU hotplug

[David Woodhouse]

Since commit 87797fad6cce ("xen/events: replace evtchn_rwlock with
RCU"), I can no longer hotplug CPUs under Xen with lockdep enabled.

(This is real Xen 4.17.1; annoyingly I have different failure modes
with Xen guests under qemu/kvm and I'll deal with those next.)

Offlining complains thus:

[root@localhost cpu1]# echo 0 > online 
[   52.936265] 
[   52.936271] =
[   52.936274] WARNING: suspicious RCU usage
[   52.936277] 6.6.0-rc5+ #1357 Not tainted
[   52.936280] -
[   52.936282] lib/maple_tree.c:856 suspicious rcu_dereference_check() usage!
[   52.936287] 
[   52.936287] other info that might help us debug this:
[   52.936287] 
[   52.936289] 
[   52.936289] RCU used illegally from offline CPU!
[   52.936289] rcu_scheduler_active = 2, debug_locks = 1
[   52.936293] 1 lock held by swapper/1/0:
[   52.936296]  #0: 89c03820 (rcu_read_lock){}-{1:3}, at: 
mtree_load+0x90/0x590
[   52.936321] 
[   52.936321] stack backtrace:
[   52.936324] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 6.6.0-rc5+ #1357
[   52.936329] Hardware name: Xen HVM domU, BIOS 4.17.1 09/26/2023
[   52.936332] Call Trace:
[   52.936335]  
[   52.936340]  dump_stack_lvl+0x5b/0x90
[   52.936350]  lockdep_rcu_suspicious+0x15a/0x1c0
[   52.936366]  mtree_load+0x49e/0x590
[   52.936385]  irq_get_irq_data+0xe/0x20
[   52.936394]  xen_send_IPI_one+0xa4/0x100
[   52.936404]  __xen_send_IPI_mask+0x1b/0x50
[   52.936414]  generic_exec_single+0x35/0x1c0
[   52.936423]  smp_call_function_single+0xc2/0x140
[   52.936436]  ? cpuhp_report_idle_dead+0x42/0x60
[   52.936444]  do_idle+0xda/0xe0
[   52.936451]  cpu_startup_entry+0x2a/0x30
[   52.936456]  start_secondary+0x123/0x140
[   52.936465]  secondary_startup_64_no_verify+0x178/0x17b
[   52.936490]  
[   52.936492] 
[   52.936494] =
[   52.936496] WARNING: suspicious RCU usage
[   52.936498] 6.6.0-rc5+ #1357 Not tainted
[   52.936500] -
[   52.936502] lib/maple_tree.c:812 suspicious rcu_dereference_check() usage!
[   52.936505] 
[   52.936505] other info that might help us debug this:
[   52.936505] 
[   52.936507] 
[   52.936507] RCU used illegally from offline CPU!
[   52.936507] rcu_scheduler_active = 2, debug_locks = 1
[   52.936510] 1 lock held by swapper/1/0:
[   52.936513]  #0: 89c03820 (rcu_read_lock){}-{1:3}, at: 
mtree_load+0x90/0x590
[   52.936530] 
[   52.936530] stack backtrace:
[   52.936532] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 6.6.0-rc5+ #1357
[   52.936536] Hardware name: Xen HVM domU, BIOS 4.17.1 09/26/2023
[   52.936538] Call Trace:
[   52.936540]  
[   52.936543]  dump_stack_lvl+0x5b/0x90
[   52.936549]  lockdep_rcu_suspicious+0x15a/0x1c0
[   52.936562]  mtree_load+0x3b7/0x590
[   52.936580]  irq_get_irq_data+0xe/0x20
[   52.936586]  xen_send_IPI_one+0xa4/0x100
[   52.936594]  __xen_send_IPI_mask+0x1b/0x50
[   52.936601]  generic_exec_single+0x35/0x1c0
[   52.936609]  smp_call_function_single+0xc2/0x140
[   52.936621]  ? cpuhp_report_idle_dead+0x42/0x60
[   52.936626]  do_idle+0xda/0xe0
[   52.936632]  cpu_startup_entry+0x2a/0x30
[   52.936643]  start_secondary+0x123/0x140
[   52.936649]  secondary_startup_64_no_verify+0x178/0x17b
[   52.936672]  
[   52.937164] smpboot: CPU 1 is now offline
[root@localhost cpu1]# 

Onlining triple-faults:

[root@localhost cpu1]# echo 1 > online 
[   58.049051] installing Xen timer for CPU 1
[   58.051533] smpboot: Booting Node 0 Processor 1 APIC 0x2

... and it dumps me back to the host prompt, where 'xl dmesg' says:

(XEN) *** Dumping Dom7 vcpu#1 state: ***
(XEN) [ Xen-4.17.1  x86_64  debug=n  Not tainted ]
(XEN) CPU:6
(XEN) RIP:0010:[]
(XEN) RFLAGS: 00010002   CONTEXT: hvm guest (d7v1)
(XEN) rax: 0001   rbx:    rcx: 0008
(XEN) rdx: fe001000   rsi: 8f364e10b048   rdi: 0001
(XEN) rbp:    rsp: a6f6400aff38   r8:  fe5b
(XEN) r9:     r10: 8f364e106078   r11: 
(XEN) r12:    r13:    r14: 
(XEN) r15:    cr0: 80050033   cr4: 001300a0
(XEN) cr3: 2583c000   cr2: 
(XEN) fsb:    gsb: 8f364e10   gss: 
(XEN) ds:    es:    fs:    gs:    ss:    cs: 0010

Not 100% sure where 88041879 is but there's a 'ud' at 
0x81041879 in load_current_idt() for the
lockdep_assert_irqs_disabled().

And yes, if I comment that assertion out then onlining does succeed
without a triplefault but with a different warning:

[root@localhost cpu1]# echo 1 > online 
[   35.843897] installing Xen timer for CPU 1
[   35.846134] smpboot: Booting Node 0 Processor 1 APIC 0x2
[   35.847297] [ cut here ]
[   35.847307] WARNING: CPU: 1 PID: 0 at arch/x86/kernel/cpu/common.c:454 
cr4_update_irqsoff+0x32/0x60

Re: [XEN PATCH v2 00/10] address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Mon, 23 Oct 2023, Jan Beulich wrote:
> On 21.10.2023 01:26, Stefano Stabellini wrote:
> > On Fri, 20 Oct 2023, Jan Beulich wrote:
> >> On 19.10.2023 18:19, Stefano Stabellini wrote:
> >>> On Thu, 19 Oct 2023, Jan Beulich wrote:
>  On 19.10.2023 02:44, Stefano Stabellini wrote:
> > On Wed, 18 Oct 2023, Jan Beulich wrote:
> >> On 18.10.2023 02:48, Stefano Stabellini wrote:
> >>> On Mon, 16 Oct 2023, Jan Beulich wrote:
>  On 29.09.2023 00:24, Stefano Stabellini wrote:
> > If it is not a MISRA requirement, then I think we should go for the 
> > path
> > of least resistance and try to make the smallest amount of changes
> > overall, which seems to be:
> 
>  ... "least resistance" won't gain us much, as hardly any guards don't
>  start with an underscore.
> 
> > - for xen/include/blah.h, __BLAH_H__
> > - for xen/arch/arm/asm/include/blah.h, __ASM_ARM_BLAH_H__
> > - for xen/arch/x86/asm/include/blah.h, it is far less consistent, 
> > maybe __ASM_X86_BLAH_H__ ?
> 
>  There are no headers in xen/include/. For (e.g.) xen/include/xen/ we
>  may go with XEN_BLAH_H; whether ASM prefixes are needed I'm not sure;
>  we could go with just ARM_BLAH_H and X86_BLAH_H?
> 
>  The primary question though is (imo) how to deal with private 
>  headers,
>  such that the risk of name collisions is as small as possible.
> >>>
> >>> Looking at concrete examples under xen/include/xen:
> >>> xen/include/xen/mm.h __XEN_MM_H__
> >>> xen/include/xen/dm.h __XEN_DM_H__
> >>> xen/include/xen/hypfs.h __XEN_HYPFS_H__
> >>>
> >>> So I think we should do for consistency:
> >>> xen/include/xen/blah.h __XEN_BLAH_H__
> >>>
> >>> Even if we know the leading underscore are undesirable, in this case I
> >>> would prefer consistency.
> >>
> >> I'm kind of okay with that. FTAOD - here and below you mean to make 
> >> this
> >> one explicit first exception from the "no new leading underscores" 
> >> goal,
> >> for newly added headers?
> >
> > Yes. The reason is for consistency with the existing header files.
> >
> >
> >>> On the other hand looking at ARM examples:
> >>> xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__
> >>> xen/arch/arm/include/asm/time.h __ARM_TIME_H__
> >>> xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H
> >>> xen/arch/arm/include/asm/io.h _ASM_IO_H
> >>>
> >>> And also looking at x86 examples:
> >>> xen/arch/x86/include/asm/paging.h _XEN_PAGING_H
> >>> xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H
> >>> xen/arch/x86/include/asm/io.h _ASM_IO_H
> >>>
> >>> Thet are very inconsistent.
> >>>
> >>>
> >>> So for ARM and X86 headers I think we are free to pick anything we 
> >>> want,
> >>> including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine by
> >>> me.
> >>
> >> To be honest, I'd prefer a global underlying pattern, i.e. if common
> >> headers are "fine" to use leading underscores for guards, arch header
> >> should, too.
> >
> > I am OK with that too. We could go with:
> > __ASM_ARM_BLAH_H__
> > __ASM_X86_BLAH_H__
> >
> > I used "ASM" to make it easier to differentiate with the private headers
> > below. Also the version without "ASM" would work but it would only
> > differ with the private headers in terms of leading underscores. I
> > thought that also having "ASM" would help readability and help avoid
> > confusion.
> >
> >
> >>> For private headers such as:
> >>> xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__
> >>> xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_
> >>> xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__
> >>> xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H
> >>>
> >>> More similar but still inconsistent. I would go with ARCH_ARM_BLAH_H 
> >>> and
> >>> ARCH_X86_BLAH_H for new headers.
> >>
> >> I'm afraid I don't like this, as deeper paths would lead to unwieldy
> >> guard names. If we continue to use double-underscore prefixed names
> >> in common and arch headers, why don't we demand no leading underscores
> >> and no path-derived prefixes in private headers? That'll avoid any
> >> collisions between the two groups.
> >
> > OK, so for private headers:
> >
> > ARM_BLAH_H
> > X86_BLAH_H
> >
> > What that work for you?
> 
>  What are the ARM_ and X86_ prefixes supposed to indicate here? Or to ask
>  differently, how would you see e.g. common/decompress.h's guard named?
> >>>
> >>> I meant that:
> >>>
> >>> xen/arch/arm/blah.h would use ARM_BLAH_H
> >>> xen/arch/x86/blah.h would use X86_BLAH_H
> >>>
> >>> You have a good question on something like xen/common/decompress.h and
> >>> xen/common/event_channe

Re: [XEN PATCH][for-4.19 v3 1/8] xen/include: add macro LOWEST_BIT

[Stefano Stabellini]

On Mon, 23 Oct 2023, Jan Beulich wrote:
> On 23.10.2023 15:19, Nicola Vetrini wrote:
> > On 23/10/2023 11:19, Nicola Vetrini wrote:
> >> On 23/10/2023 09:48, Jan Beulich wrote:
> >>> On 20.10.2023 17:28, Nicola Vetrini wrote:
>  --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>  +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>  @@ -246,6 +246,12 @@ constant expressions are required.\""
> "any()"}
>   -doc_end
> 
>  +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern 
>  to obtain the value
>  +2^ffs(x) for unsigned integers on two's complement architectures
>  +(all the architectures supported by Xen satisfy this requirement)."
>  +-config=MC3R1.R10.1,reports+={safe, 
>  "any_area(any_loc(any_exp(macro(^LOWEST_BIT$"}
>  +-doc_end
> >>>
> >>> This being deviated this way (rather than by SAF-* comments) wants
> >>> justifying in the description. You did reply to my respective
> >>> comment on v2, but such then (imo) needs propagating into the actual
> >>> patch as well.
> >>>
> >>
> >> Sure, will do.
> >>
>  --- a/docs/misra/deviations.rst
>  +++ b/docs/misra/deviations.rst
>  @@ -192,6 +192,13 @@ Deviations related to MISRA C:2012 Rules:
>  See automation/eclair_analysis/deviations.ecl for the full 
>  explanation.
>    - Tagged as `safe` for ECLAIR.
> 
>  +   * - R10.1
>  + - The well-known pattern (x & -x) applied to unsigned integer 
>  values on 2's
>  +   complement architectures (i.e., all architectures supported 
>  by Xen), used
>  +   to obtain the value 2^ffs(x), where ffs(x) is the position of 
>  the first
>  +   bit set. If no bits are set, zero is returned.
>  + - Tagged as `safe` for ECLAIR.
> >>>
> >>> In such an explanation there shall not be any ambiguity. Here I see
> >>> an issue with ffs() returning 1-based bit position numbers, which
> >>> isn't in line with the use in 2^ffs(x). (Arguably use of ^ itself is
> >>> also problematic, as that's XOR in C, not POW. I'd suggest 2^^ffs(x)
> >>> or 2**ffs(x).)
> >>>
> >>
> >> Makes sense, I think I'll use an plain english explanation to avoid
> >> any confusion
> >> about notation.
> >>
>  --- a/xen/include/xen/macros.h
>  +++ b/xen/include/xen/macros.h
>  @@ -8,8 +8,11 @@
>   #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
>   #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
> 
>  -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
>  -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
>  +/* Returns the 2^ffs(x) or 0, where ffs(x) is the index of the 
>  lowest set bit */
>  +#define LOWEST_BIT(x) ((x) & -(x))
> >>>
> >>> I'm afraid my concern regarding this new macro's name (voiced on v2) 
> >>> hasn't
> >>> been addressed (neither verbally nor by finding a more suitable name).
> >>>
> >>> Jan
> >>
> >> I didn't come up with much better names, so I left it as is. Here's a 
> >> few ideas:
> >>
> >> - LOWEST_SET
> >> - MASK_LOWEST_SET
> >> - MASK_LOWEST_BIT
> >> - MASK_FFS_0
> >> - LOWEST_ONE
> >>
> >> and also yours, included here for convenience, even though you felt it
> >> was too long:
> >>
> >> - ISOLATE_LOW_BIT()
> >>
> >> All maintainers from THE REST are CC-ed, so we can see if anyone has
> >> any suggestion.
> > 
> > There's also LOWEST_BIT_MASK as another possible name.
> 
> While naming-wise okay to me, it has the same "too long" issue as
> ISOLATE_LOW_BIT(). Considering x86, in the BMI ISA extension, has an
> insn doing exactly that (BLSI), taking inspiration from its mnemonic
> may also be an option.

I don't mean to make things difficult but I prefer LOWEST_BIT or
MASK_LOWEST_BIT. It is clear at first glance. BLSI is not as clear,
unless you work on the specific ISA with BLSI.

In general, I do appreciate shorter names, but if one option is much
clearer than the other, I prefer clarity over shortness.

Maybe we need a third opinion here to make progress a bit faster.
Julien?

[ovmf test] 183498: all pass - PUSHED

[osstest service owner]

flight 183498 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/183498/

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf ec7f73436646a9232c6494d1ce23fb38000e10d3
baseline version:
 ovmf c591395f4ab5359c14e783481473685cf432fe75

Last test of basis   183445  2023-10-20 13:10:42 Z3 days
Testing same since   183498  2023-10-23 17:10:46 Z0 days1 attempts


People who touched revisions under test:
  Jeshua Smith 

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
   c591395f4a..ec7f734366  ec7f73436646a9232c6494d1ce23fb38000e10d3 -> 
xen-tested-master

Re: Refactor arm64/domctl.c 'subarch_do_domctl' to avoid unreachable break.

[Julien Grall]

Hi Nicola.

On 23/10/2023 16:16, Nicola Vetrini wrote:

On 23/10/2023 17:00, Julien Grall wrote:

On 23/10/2023 15:51, Nicola Vetrini wrote:

Hi,


Hi Nicola,

while taking care of some patches regarding MISRA C Rule 2.1 (code 
shouldn't be unreachable), I

came across this function:

long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
{
 switch ( domctl->cmd )
 {
 case XEN_DOMCTL_set_address_size:
 switch ( domctl->u.address_size.size )
 {
 case 32:
 if ( !cpu_has_el1_32 )
 return -EINVAL;
 /* SVE is not supported for 32 bit domain */
 if ( is_sve_domain(d) )
 return -EINVAL;
 return switch_mode(d, DOMAIN_32BIT);
 case 64:
 return switch_mode(d, DOMAIN_64BIT);
 default:
 return -EINVAL;
 }
 break;

 default:
 return -ENOSYS;
 }
}

here the break after the innermost switch is clearly unreachable, but 
it's also guarding a possible fallthrough.

I can see a couple of solutions to this:

- mark the part after the switch unreachable;
- introduce a variable 'long rc' to store the return value, and 
consequently rework the control flow of all the switches

   (e.g. rc = -EINVAL and similar);
- remove the break, but I consider this a risky move, unless -ENOSYS 
would be an ok value to be returned if some case

   from the switch above does not have a return statement.


- move the nested switch in a separate function, so the code in
subarch_do_domctl() can be replaced with:

return set_address_size(...);



What would be the preferred way of addressing this violation?


I would actually prefer the 4th option I suggested.

Cheers,


Would you mind sending the patch yourself?


No. It is sent: 
https://lore.kernel.org/20231023175220.42781-1-jul...@xen.org


Cheers,

--
Julien Grall

[PATCH for-4.19] xen/arm64: domctl: Avoid unreachable code in subarch_do_domctl()

[Julien Grall]

From: Julien Grall 

The 'break' the XEN_DOMCTL_set_address_size is unreachable and tools
like Eclair will report as a violation of Misra Rule 2.1.

Furthermore, the nested switch is not very easy to read. So move
out the nested switch in a separate function to improve the
readability and hopefully address the MISRA violation.

Reported-by: Nicola Vetrini 
Signed-off-by: Julien Grall 

---

Only compiled tested. Waiting for the CI to confirm there is no
regression.
---
 xen/arch/arm/arm64/domctl.c | 34 +++---
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
index 14fc622e9956..8720d126c97d 100644
--- a/xen/arch/arm/arm64/domctl.c
+++ b/xen/arch/arm/arm64/domctl.c
@@ -33,27 +33,31 @@ static long switch_mode(struct domain *d, enum domain_type 
type)
 return 0;
 }
 
+static long set_address_size(struct domain *d, uint32_t address_size)
+{
+switch ( address_size )
+{
+case 32:
+if ( !cpu_has_el1_32 )
+return -EINVAL;
+/* SVE is not supported for 32 bit domain */
+if ( is_sve_domain(d) )
+return -EINVAL;
+return switch_mode(d, DOMAIN_32BIT);
+case 64:
+return switch_mode(d, DOMAIN_64BIT);
+default:
+return -EINVAL;
+}
+}
+
 long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 {
 switch ( domctl->cmd )
 {
 case XEN_DOMCTL_set_address_size:
-switch ( domctl->u.address_size.size )
-{
-case 32:
-if ( !cpu_has_el1_32 )
-return -EINVAL;
-/* SVE is not supported for 32 bit domain */
-if ( is_sve_domain(d) )
-return -EINVAL;
-return switch_mode(d, DOMAIN_32BIT);
-case 64:
-return switch_mode(d, DOMAIN_64BIT);
-default:
-return -EINVAL;
-}
-break;
+return set_address_size(d, domctl->u.address_size.size);
 
 default:
 return -ENOSYS;
-- 
2.40.1

[PATCH] xen/vpci: allow BAR write if value is the same

[Stewart Hildebrand]

During xl pci-assignable-add, pciback may reset the device while memory decoding
(PCI_COMMAND_MEMORY) is enabled. After device reset, memory decoding may be
disabled in hardware, and the BARs may be zeroed/reset in hardware. However, Xen
vPCI still thinks memory decoding is enabled, and BARs will remain mapped in
p2m. In other words, memory decoding may become disabled and BARs reset in
hardware, bypassing the respective vPCI command and BAR register handlers.
Subsequently, when pciback attempts to restore state to the device, including
BARs, it happens to write the BARs before writing the command register.
Restoring/writing the BARs silently fails because Xen vPCI mistakenly thinks
memory decoding is enabled.

Fix the BAR write by allowing it to succeed if the value written is the same as
the Xen vPCI stored value. pciback will subsequently restore the command
register and the state of the BARs and memory decoding bit will then be in sync
between hardware and vPCI again.

While here, remove a nearby stray newline.

Signed-off-by: Stewart Hildebrand 
---
Do we need similar handling in rom_write()?

We may consider additionally checking during bar_write() if the memory decoding
state has become out of sync between hardware and vPCI. During bar_write(), we
would check the device's memory decoding bit, compare it to our vPCI state, and
invoke modify_bars() if needed. Please let me know your thoughts.

I considered additionally checking if the hardware and vPCI memory decoding
state are out of sync in new cmd_read()/bar_read() handlers, and calling
modify_bars() if needed. However, I decided not to do this because it would
impose an unnecessary implication on the in-progress vPCI series with the
rwlock: calling modify_bars() would require holding the lock in write/exclusive
mode, whereas in vPCI read handlers we would only hold the lock in read mode.

I have only observed the inconsistency after device reset when pciback (i.e.
dom0/hardware domain) is restoring the state to the device. Since pciback will
also restore the command register, the state will be back in sync after pciback
is finished restoring the state.
---
 xen/drivers/vpci/header.c | 14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 767c1ba718d7..446ecf539e89 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -430,19 +430,17 @@ static void cf_check bar_write(
 
 /*
  * Xen only cares whether the BAR is mapped into the p2m, so allow BAR
- * writes as long as the BAR is not mapped into the p2m.
+ * writes as long as the BAR is not mapped into the p2m. If the value
+ * written is the current one allow the write regardless to ensure
+ * consistent state between hardware and vPCI.
  */
-if ( bar->enabled )
+if ( bar->enabled && val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
 {
-/* If the value written is the current one avoid printing a warning. */
-if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
-gprintk(XENLOG_WARNING,
-"%pp: ignored BAR %zu write while mapped\n",
-&pdev->sbdf, bar - pdev->vpci->header.bars + hi);
+gprintk(XENLOG_WARNING, "%pp: ignored BAR %zu write while mapped\n",
+&pdev->sbdf, bar - pdev->vpci->header.bars + hi);
 return;
 }
 
-
 /*
  * Update the cached address, so that when memory decoding is enabled
  * Xen can map the BAR into the guest p2m.

base-commit: bad1ac345b1910b820b8a703ad1b9f66412ea844
-- 
2.42.0

Re: Refactor arm64/domctl.c 'subarch_do_domctl' to avoid unreachable break.

[Julien Grall]

Hi Jan,

On 23/10/2023 16:15, Jan Beulich wrote:

On 23.10.2023 17:00, Julien Grall wrote:



On 23/10/2023 15:51, Nicola Vetrini wrote:

Hi,


Hi Nicola,


while taking care of some patches regarding MISRA C Rule 2.1 (code
shouldn't be unreachable), I
came across this function:

long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
     XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
{
      switch ( domctl->cmd )
      {
      case XEN_DOMCTL_set_address_size:
      switch ( domctl->u.address_size.size )
      {
      case 32:
      if ( !cpu_has_el1_32 )
      return -EINVAL;
      /* SVE is not supported for 32 bit domain */
      if ( is_sve_domain(d) )
      return -EINVAL;
      return switch_mode(d, DOMAIN_32BIT);
      case 64:
      return switch_mode(d, DOMAIN_64BIT);
      default:
      return -EINVAL;
      }
      break;

      default:
      return -ENOSYS;
      }
}

here the break after the innermost switch is clearly unreachable, but
it's also guarding a possible fallthrough.
I can see a couple of solutions to this:

- mark the part after the switch unreachable;
- introduce a variable 'long rc' to store the return value, and
consequently rework the control flow of all the switches
    (e.g. rc = -EINVAL and similar);
- remove the break, but I consider this a risky move, unless -ENOSYS
would be an ok value to be returned if some case
    from the switch above does not have a return statement.


- move the nested switch in a separate function, so the code in
subarch_do_domctl() can be replaced with:

return set_address_size(...);


But that would help only if inside the new function you still re-
layout the switch() (or replace it by, say, if/else-if/else),
wouldn't it?


I am not sure why I would need to re-layout the switch. This should work 
(untested):


diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
index 14fc622e9956..8720d126c97d 100644
--- a/xen/arch/arm/arm64/domctl.c
+++ b/xen/arch/arm/arm64/domctl.c
@@ -33,27 +33,31 @@ static long switch_mode(struct domain *d, enum 
domain_type type)

 return 0;
 }

+static long set_address_size(struct domain *d, uint32_t address_size)
+{
+switch ( address_size )
+{
+case 32:
+if ( !cpu_has_el1_32 )
+return -EINVAL;
+/* SVE is not supported for 32 bit domain */
+if ( is_sve_domain(d) )
+return -EINVAL;
+return switch_mode(d, DOMAIN_32BIT);
+case 64:
+return switch_mode(d, DOMAIN_64BIT);
+default:
+return -EINVAL;
+}
+}
+
 long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 {
 switch ( domctl->cmd )
 {
 case XEN_DOMCTL_set_address_size:
-switch ( domctl->u.address_size.size )
-{
-case 32:
-if ( !cpu_has_el1_32 )
-return -EINVAL;
-/* SVE is not supported for 32 bit domain */
-if ( is_sve_domain(d) )
-return -EINVAL;
-return switch_mode(d, DOMAIN_32BIT);
-case 64:
-return switch_mode(d, DOMAIN_64BIT);
-default:
-return -EINVAL;
-}
-break;
+return set_address_size(d, domctl->u.address_size.size);

 default:
 return -ENOSYS;

Cheers,

--
Julien Grall

Re: [XEN PATCH][for-4.19 v3 5/8] x86/io_apic: address violation of MISRA C:2012 Rule 10.1

[Jan Beulich]

On 20.10.2023 17:28, Nicola Vetrini wrote:
> The definition of IO_APIC_BASE contains a sum of an essentially enum
> value (FIX_IO_APIC_BASE_0) that is positive with an index that, in all
> instances, is unsigned, therefore the former is cast to unsigned, so that
> the operands are of the same essential type.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini 

Acked-by: Jan Beulich 

[linux-linus test] 183494: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-xl-credit1  10 host-ping-check-xen fail pass in 183496-retest

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

version targeted for testing:
 linux05d3ef8bba77c1b5f98d941d8b2d4aeab8118ef1
baseline version:
 linuxfe3cfe869d5e0453754cf2b4c75110276b5e8527

Last test of basis   183492  2023-10-22 17:41:44 Z0 days
Testing same since   183494  2023-10-23 02:28:17 Z0 days1 attempts


People who touched revisions under test:
  Linus Torvalds 

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

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Jan Beulich]

On 23.10.2023 17:23, Simone Ballarin wrote:
> On 23/10/23 15:34, Jan Beulich wrote:
>> On 18.10.2023 16:18, Simone Ballarin wrote:
>>> --- a/xen/include/xen/pdx.h
>>> +++ b/xen/include/xen/pdx.h
>>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long 
>>> pfn)
>>>* @param pdx Page index
>>>* @return Obtained pfn after decompressing the pdx
>>>*/
>>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
>>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long 
>>> pdx)
>>>   {
>>>   return (pdx & pfn_pdx_bottom_mask) |
>>>  ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
>>
>> Taking this as an example for what I've said above: The compiler can't
>> know that the globals used by the functions won't change value. Even
>> within Xen it is only by convention that these variables are assigned
>> their values during boot, and then aren't changed anymore. Which makes
>> me wonder: Did you check carefully that around the time the variables
>> have their values established, no calls to the functions exist (which
>> might then be subject to folding)?
> 
> There is no need to check that, the GCC documentation explicitly says:
> 
> However, functions declared with the pure attribute *can safely read any 
> non-volatile objects*, and modify the value of objects in a way that 
> does not affect their return value or the observable state of the program.

I did quote this same text in response to what Andrew has said, but I also
did note there that this needs to be taken with a grain of salt: The
compiler generally assumes a single-threaded environment, i.e. no changes
to globals behind the back of the code it is processing.

Jan

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Simone Ballarin]

On 23/10/23 15:34, Jan Beulich wrote:

On 18.10.2023 16:18, Simone Ballarin wrote:

Add const and pure attributes to address reports
of MISRA C:2012 Rule 13.1: Initializer lists shall
not contain persistent side effects

Add pure attribute to function pdx_to_pfn.
Add const attribute to functions generated by TYPE_SAFE.

These functions are used in initializer lists: adding
the attributes ensures that no effect will be performed
by them.


Adding the attribute does, according to my understanding, ensure nothing
The compiler may (but isn't required to) diagnose wrong uses of the
attributes, but it may also make use of the attributes (on the
declaration) without regard to the attribute potentially being wrongly
applied. Since further for inline functions the compiler commonly infers
attributes from the expanded code (discarding the attribute), the only
thing achieved here is a documentation aspect, I think.


Yes, you're right. I will rephrase the commit message.





--- a/xen/include/xen/pdx.h
+++ b/xen/include/xen/pdx.h
@@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
   * @param pdx Page index
   * @return Obtained pfn after decompressing the pdx
   */
-static inline unsigned long pdx_to_pfn(unsigned long pdx)
+static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
  {
  return (pdx & pfn_pdx_bottom_mask) |
 ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);


Taking this as an example for what I've said above: The compiler can't
know that the globals used by the functions won't change value. Even
within Xen it is only by convention that these variables are assigned
their values during boot, and then aren't changed anymore. Which makes
me wonder: Did you check carefully that around the time the variables
have their values established, no calls to the functions exist (which
might then be subject to folding)?


There is no need to check that, the GCC documentation explicitly says:

However, functions declared with the pure attribute *can safely read any 
non-volatile objects*, and modify the value of objects in a way that 
does not affect their return value or the observable state of the program.


https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html


Additionally - what about the sibling function pfn_to_pdx()?

Jan


I will add the attribute also to the sibling.

--
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: Refactor arm64/domctl.c 'subarch_do_domctl' to avoid unreachable break.

[Nicola Vetrini]

On 23/10/2023 17:00, Julien Grall wrote:

On 23/10/2023 15:51, Nicola Vetrini wrote:

Hi,


Hi Nicola,

while taking care of some patches regarding MISRA C Rule 2.1 (code 
shouldn't be unreachable), I

came across this function:

long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
{
     switch ( domctl->cmd )
     {
     case XEN_DOMCTL_set_address_size:
     switch ( domctl->u.address_size.size )
     {
     case 32:
     if ( !cpu_has_el1_32 )
     return -EINVAL;
     /* SVE is not supported for 32 bit domain */
     if ( is_sve_domain(d) )
     return -EINVAL;
     return switch_mode(d, DOMAIN_32BIT);
     case 64:
     return switch_mode(d, DOMAIN_64BIT);
     default:
     return -EINVAL;
     }
     break;

     default:
     return -ENOSYS;
     }
}

here the break after the innermost switch is clearly unreachable, but 
it's also guarding a possible fallthrough.

I can see a couple of solutions to this:

- mark the part after the switch unreachable;
- introduce a variable 'long rc' to store the return value, and 
consequently rework the control flow of all the switches

   (e.g. rc = -EINVAL and similar);
- remove the break, but I consider this a risky move, unless -ENOSYS 
would be an ok value to be returned if some case

   from the switch above does not have a return statement.


- move the nested switch in a separate function, so the code in
subarch_do_domctl() can be replaced with:

return set_address_size(...);



What would be the preferred way of addressing this violation?


I would actually prefer the 4th option I suggested.

Cheers,


Would you mind sending the patch yourself?

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

Re: Refactor arm64/domctl.c 'subarch_do_domctl' to avoid unreachable break.

[Jan Beulich]

On 23.10.2023 17:00, Julien Grall wrote:
> 
> 
> On 23/10/2023 15:51, Nicola Vetrini wrote:
>> Hi,
> 
> Hi Nicola,
> 
>> while taking care of some patches regarding MISRA C Rule 2.1 (code 
>> shouldn't be unreachable), I
>> came across this function:
>>
>> long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>     XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>> {
>>      switch ( domctl->cmd )
>>      {
>>      case XEN_DOMCTL_set_address_size:
>>      switch ( domctl->u.address_size.size )
>>      {
>>      case 32:
>>      if ( !cpu_has_el1_32 )
>>      return -EINVAL;
>>      /* SVE is not supported for 32 bit domain */
>>      if ( is_sve_domain(d) )
>>      return -EINVAL;
>>      return switch_mode(d, DOMAIN_32BIT);
>>      case 64:
>>      return switch_mode(d, DOMAIN_64BIT);
>>      default:
>>      return -EINVAL;
>>      }
>>      break;
>>
>>      default:
>>      return -ENOSYS;
>>      }
>> }
>>
>> here the break after the innermost switch is clearly unreachable, but 
>> it's also guarding a possible fallthrough.
>> I can see a couple of solutions to this:
>>
>> - mark the part after the switch unreachable;
>> - introduce a variable 'long rc' to store the return value, and 
>> consequently rework the control flow of all the switches
>>    (e.g. rc = -EINVAL and similar);
>> - remove the break, but I consider this a risky move, unless -ENOSYS 
>> would be an ok value to be returned if some case
>>    from the switch above does not have a return statement.
> 
> - move the nested switch in a separate function, so the code in 
> subarch_do_domctl() can be replaced with:
> 
> return set_address_size(...);

But that would help only if inside the new function you still re-
layout the switch() (or replace it by, say, if/else-if/else),
wouldn't it?

Jan

Re: [XEN PATCH 1/4] xen/arm: address violations of MISRA C:2012 Rule 13.1

[Simone Ballarin]

On 20/10/23 10:28, Julien Grall wrote:

Hi Stefano,

On 19/10/2023 19:30, Stefano Stabellini wrote:

We usually use this trick when 'current' is used multiple time to save
process (using 'current' is not cost free). But in this case, this is
renaming without any apparent benefits.

If we wanted to go down this route, then this would likely want a 
comment.

At which point we should just deviate.

I will have a think if there are something else we can do. Could we 
consider

to not address it for now?



I totally agree.


I am wondering if we could deviate "current" because even with the
implementation using volatile we know it doesn't have "side effects" in
the sense of changing things for other code outside the function.


I will let Simone to confirm whether it is possible to do it from a 
technical point of view.


Yes, it is possible.


Leaving the technical part aside, is the only violations are the one in 
this patch? If so, I don't think it makes sense to deviate 'current' 
globally. It would be better to have local deviations.


Cheers,


For this specific case I agree in using a local deviation.
In the next version of the patch, I will use a SAF comment.

--
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: Refactor arm64/domctl.c 'subarch_do_domctl' to avoid unreachable break.

[Julien Grall]

On 23/10/2023 15:51, Nicola Vetrini wrote:

Hi,


Hi Nicola,

while taking care of some patches regarding MISRA C Rule 2.1 (code 
shouldn't be unreachable), I

came across this function:

long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
{
     switch ( domctl->cmd )
     {
     case XEN_DOMCTL_set_address_size:
     switch ( domctl->u.address_size.size )
     {
     case 32:
     if ( !cpu_has_el1_32 )
     return -EINVAL;
     /* SVE is not supported for 32 bit domain */
     if ( is_sve_domain(d) )
     return -EINVAL;
     return switch_mode(d, DOMAIN_32BIT);
     case 64:
     return switch_mode(d, DOMAIN_64BIT);
     default:
     return -EINVAL;
     }
     break;

     default:
     return -ENOSYS;
     }
}

here the break after the innermost switch is clearly unreachable, but 
it's also guarding a possible fallthrough.

I can see a couple of solutions to this:

- mark the part after the switch unreachable;
- introduce a variable 'long rc' to store the return value, and 
consequently rework the control flow of all the switches

   (e.g. rc = -EINVAL and similar);
- remove the break, but I consider this a risky move, unless -ENOSYS 
would be an ok value to be returned if some case

   from the switch above does not have a return statement.


- move the nested switch in a separate function, so the code in 
subarch_do_domctl() can be replaced with:


return set_address_size(...);



What would be the preferred way of addressing this violation?


I would actually prefer the 4th option I suggested.

Cheers,

--
Julien Grall

Refactor arm64/domctl.c 'subarch_do_domctl' to avoid unreachable break.

[Nicola Vetrini]

Hi,

while taking care of some patches regarding MISRA C Rule 2.1 (code 
shouldn't be unreachable), I

came across this function:

long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
   XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
{
switch ( domctl->cmd )
{
case XEN_DOMCTL_set_address_size:
switch ( domctl->u.address_size.size )
{
case 32:
if ( !cpu_has_el1_32 )
return -EINVAL;
/* SVE is not supported for 32 bit domain */
if ( is_sve_domain(d) )
return -EINVAL;
return switch_mode(d, DOMAIN_32BIT);
case 64:
return switch_mode(d, DOMAIN_64BIT);
default:
return -EINVAL;
}
break;

default:
return -ENOSYS;
}
}

here the break after the innermost switch is clearly unreachable, but 
it's also guarding a possible fallthrough.

I can see a couple of solutions to this:

- mark the part after the switch unreachable;
- introduce a variable 'long rc' to store the return value, and 
consequently rework the control flow of all the switches

  (e.g. rc = -EINVAL and similar);
- remove the break, but I consider this a risky move, unless -ENOSYS 
would be an ok value to be returned if some case

  from the switch above does not have a return statement.

What would be the preferred way of addressing this violation?

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

Re: [XEN PATCH 7/7] x86/hpet: address violations of MISRA C:2012 Rule 8.2

[Jan Beulich]

On 18.10.2023 16:26, Federico Serafini wrote:
> --- a/xen/arch/x86/include/asm/hpet.h
> +++ b/xen/arch/x86/include/asm/hpet.h
> @@ -60,7 +60,7 @@ extern int8_t opt_hpet_legacy_replacement;
>   * Return value is zero if HPET is unavailable.
>   */
>  u64 hpet_setup(void);
> -void hpet_resume(u32 *);
> +void hpet_resume(u32 *boot_cfg);

Ideally also switching to uint32_t while touching this:
Acked-by: Jan Beulich 

Jan

Re: [XEN PATCH 6/7] x86/vmce: address violations of MISRA C:2012 Rule 8.2

[Jan Beulich]

On 18.10.2023 16:25, Federico Serafini wrote:
> Add missing parameter names. No functional change.
> 
> Signed-off-by: Federico Serafini 

Acked-by: Jan Beulich 

Same remark here wrt folding with patch 1.

Jan

Re: [XEN PATCH 5/7] x86/mcaction: address a violation of MISRA C:2012 Rule 8.2

[Jan Beulich]

On 18.10.2023 16:25, Federico Serafini wrote:
> Add missing parameter name. No functional change.
> 
> Signed-off-by: Federico Serafini 

Acked-by: Jan Beulich 

Albeit I think that this would best be folded with patch 1, using
"x86/mce: " as the component prefix.

Jan

Re: [XEN PATCH 4/7] x86/cpuidle: address violations of MISRA C:2012 Rule 8.2

[Jan Beulich]

On 18.10.2023 16:25, Federico Serafini wrote:
> Add missing parameter names. No functional change.
> 
> Signed-off-by: Federico Serafini 

Acked-by: Jan Beulich 

Re: [XEN PATCH 3/7] x86/domain: address violations of MISRA C:2012 Rule 8.2

[Jan Beulich]

On 18.10.2023 16:25, Federico Serafini wrote:
> Add missing parameter names. No functional change.
> 
> Signed-off-by: Federico Serafini 

Acked-by: Jan Beulich 

Re: [XEN PATCH 2/7] x86/mtrr: address violation of MISRA C:2012 Rule 8.2

[Jan Beulich]

On 18.10.2023 16:25, Federico Serafini wrote:
> Add missing parameter names. No functional change.
> 
> Signed-off-by: Federico Serafini 

Acked-by: Jan Beulich 

Re: [XEN PATCH 1/7] x86/mctelem: address violations of MISRA C:2012 Rule 8.2

[Jan Beulich]

On 18.10.2023 16:25, Federico Serafini wrote:
> Add missing parameter names. No functional change.
> 
> Signed-off-by: Federico Serafini 

Acked-by: Jan Beulich 

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Jan Beulich]

On 23.10.2023 15:51, Andrew Cooper wrote:
> On 23/10/2023 2:34 pm, Jan Beulich wrote:
>> On 18.10.2023 16:18, Simone Ballarin wrote:
>>> --- a/xen/include/xen/pdx.h
>>> +++ b/xen/include/xen/pdx.h
>>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long 
>>> pfn)
>>>   * @param pdx Page index
>>>   * @return Obtained pfn after decompressing the pdx
>>>   */
>>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
>>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long 
>>> pdx)
>>>  {
>>>  return (pdx & pfn_pdx_bottom_mask) |
>>> ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
>> Taking this as an example for what I've said above: The compiler can't
>> know that the globals used by the functions won't change value. Even
>> within Xen it is only by convention that these variables are assigned
>> their values during boot, and then aren't changed anymore. Which makes
>> me wonder: Did you check carefully that around the time the variables
>> have their values established, no calls to the functions exist (which
>> might then be subject to folding)?
> 
> I was actually going to point this out, but hadn't found the words.
> 
> pdx_to_pfn() is not pure.  It violates the requirements for being
> declared pure, in a way that the compiler can see.

Hmm, I think you're remembering old wording in the doc. Nowadays it says
"However, functions declared with the pure attribute can safely read any
 nonvolatile objects, and modify the value of objects in a way that does
 not affect their return value or the observable state of the program."

To me this reads as if reads of globals are okay, constrained by the
generally single-threaded view a compiler takes.

Irrespective I agree that without said further checking (and perhaps
even some way of making sure the requirements can't be violated by
future changes), ...

> Right now, this will cause GCC to ignore the attribute, but who's to say
> that future GCCs don't start emitting a diagnostic (in which case we'd
> have to delete them to make them compile), or start honouring them (at
> which point this logic will start to malfunction around the boot time
> modification to the masks).
> 
> 
> It is undefined behaviour to intentionally lie to the compiler using
> attributes.  This is intentionally introducing undefined behaviour to
> placate Eclair.

... we'd effectively be lying to the compiler, putting ourselves at risk.

Jan

> So why are we bending over backwards to remove UB in other areas, but
> deliberately introducing here?  How does that conform with the spirit of
> MISRA?
> 
> ~Andrew

Re: [XEN PATCH 4/4] xen: address violations of MISRA C:2012 Rule 13.1

[Jan Beulich]

On 18.10.2023 16:18, Simone Ballarin wrote:
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -1504,6 +1504,8 @@ long vcpu_yield(void)
>  {
>  struct vcpu * v=current;
>  spinlock_t *lock;
> +domid_t domain_id;
> +int vcpu_id;

While I realize that the field this variable is initialized from is
plain "int", I still think that being wrong means the new variables
here and below want to be "unsigned int".

> @@ -1515,7 +1517,9 @@ long vcpu_yield(void)
>  
>  SCHED_STAT_CRANK(vcpu_yield);
>  
> -TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
> +domain_id = current->domain->domain_id;
> +vcpu_id = current->vcpu_id;
> +TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id);

If you touch this, I think you ought to also switch to using "v" in
place of "current" (making the supposed side effect go away aiui).

Yet then (for the further changes to this file) - what persistent
side effect does reading "current" have? Clarification of that is
part of the justification for this change, and hence ought to be
spelled out in the description.

> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -445,10 +445,12 @@ static void __init cf_check ns16550_init_postirq(struct 
> serial_port *port)
>  struct msi_info msi = {
>  .sbdf = PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>   uart->ps_bdf[2]),
> -.irq = rc = uart->irq,
> +.irq = uart->irq,
>  .entry_nr = 1
>  };
>  
> +rc = uart->irq;
> +
>  if ( rc > 0 )

If this needs transforming, I think we'd better switch it to

rc = 0;

if ( uart->irq > 0 )

thus matching what we have elsewhere in the function.

Jan

Re: [XEN PATCH] x86/p2m: address violations of MISRA C:2012 Rules 8.2 and 8.3

[Andrew Cooper]

On 23/10/2023 2:47 pm, Federico Serafini wrote:
> Add missing parameter name and make function declarations and
> definitions consistent. No functional change.
>
> Signed-off-by: Federico Serafini 

Acked-by: Andrew Cooper 

Queued in for-next.

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Andrew Cooper]

On 23/10/2023 2:34 pm, Jan Beulich wrote:
> On 18.10.2023 16:18, Simone Ballarin wrote:
>> --- a/xen/include/xen/pdx.h
>> +++ b/xen/include/xen/pdx.h
>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
>>   * @param pdx Page index
>>   * @return Obtained pfn after decompressing the pdx
>>   */
>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
>>  {
>>  return (pdx & pfn_pdx_bottom_mask) |
>> ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
> Taking this as an example for what I've said above: The compiler can't
> know that the globals used by the functions won't change value. Even
> within Xen it is only by convention that these variables are assigned
> their values during boot, and then aren't changed anymore. Which makes
> me wonder: Did you check carefully that around the time the variables
> have their values established, no calls to the functions exist (which
> might then be subject to folding)?

I was actually going to point this out, but hadn't found the words.

pdx_to_pfn() is not pure.  It violates the requirements for being
declared pure, in a way that the compiler can see.

Right now, this will cause GCC to ignore the attribute, but who's to say
that future GCCs don't start emitting a diagnostic (in which case we'd
have to delete them to make them compile), or start honouring them (at
which point this logic will start to malfunction around the boot time
modification to the masks).


It is undefined behaviour to intentionally lie to the compiler using
attributes.  This is intentionally introducing undefined behaviour to
placate Eclair.

So why are we bending over backwards to remove UB in other areas, but
deliberately introducing here?  How does that conform with the spirit of
MISRA?

~Andrew

[XEN PATCH] x86/p2m: address violations of MISRA C:2012 Rules 8.2 and 8.3

[Federico Serafini]

Add missing parameter name and make function declarations and
definitions consistent. No functional change.

Signed-off-by: Federico Serafini 
---
 xen/arch/x86/include/asm/p2m.h | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/include/asm/p2m.h b/xen/arch/x86/include/asm/p2m.h
index 40545f5fa8..f2c7d58b59 100644
--- a/xen/arch/x86/include/asm/p2m.h
+++ b/xen/arch/x86/include/asm/p2m.h
@@ -632,7 +632,7 @@ void p2m_change_type_range(struct domain *d,
p2m_type_t ot, p2m_type_t nt);
 
 /* Compare-exchange the type of a single p2m entry */
-int p2m_change_type_one(struct domain *d, unsigned long gfn,
+int p2m_change_type_one(struct domain *d, unsigned long gfn_l,
 p2m_type_t ot, p2m_type_t nt);
 
 /* Synchronously change the p2m type for a range of gfns */
@@ -649,7 +649,7 @@ static inline bool p2m_is_global_logdirty(const struct 
domain *d)
 #endif
 }
 
-int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start,
+int p2m_is_logdirty_range(struct p2m_domain *p2m, unsigned long start,
   unsigned long end);
 
 /* Set mmio addresses in the p2m table (for pass-through) */
@@ -661,9 +661,9 @@ int set_identity_p2m_entry(struct domain *d, unsigned long 
gfn,
p2m_access_t p2ma, unsigned int flag);
 int clear_identity_p2m_entry(struct domain *d, unsigned long gfn);
 /* HVM-only callers can use these directly: */
-int p2m_add_identity_entry(struct domain *d, unsigned long gfn,
+int p2m_add_identity_entry(struct domain *d, unsigned long gfn_l,
p2m_access_t p2ma, unsigned int flag);
-int p2m_remove_identity_entry(struct domain *d, unsigned long gfn);
+int p2m_remove_identity_entry(struct domain *d, unsigned long gfn_l);
 
 /* 
  * Populate-on-demand
@@ -924,7 +924,7 @@ int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
 p2m_type_t p2mt, p2m_access_t p2ma);
 
 /* Set a specific p2m view visibility */
-int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int idx,
+int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int altp2m_idx,
uint8_t visible);
 #else /* !CONFIG_HVM */
 struct p2m_domain *p2m_get_altp2m(struct vcpu *v);
-- 
2.34.1

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[Kevin Wolf]

Am 23.10.2023 um 11:30 hat Igor Mammedov geschrieben:
> On Wed, 18 Oct 2023 09:32:47 +0100
> David Woodhouse  wrote:
> 
> > On Wed, 2023-10-18 at 09:32 +0200, Igor Mammedov wrote:
> > > On Mon, 16 Oct 2023 16:19:08 +0100
> > > David Woodhouse  wrote:
> > >   
> > > > From: David Woodhouse 
> > > >   
> > > 
> > > is this index a user (guest) visible?  
> > 
> > Yes. It defines what block device (e.g. /dev/xvda) the disk appears as
> > in the guest. In the common case, it literally encodes the Linux
> > major/minor numbers. So xvda (major 202) is 0xca00, xvdb is 0xca10 etc.
> 
> that makes 'index' an implicit ABI and a subject to versioning
> when the way it's assigned changes (i.e. one has to use versioned
> machine types to keep older versions working the they used to).
> 
> From what I remember it's discouraged to make QEMU invent
> various IDs that are part of ABI (guest or mgmt side).
> Instead it's preferred for mgmt side/user to provide that explicitly.
> 
> Basically you are trading off manageability/simplicity at QEMU
> level with CLI usability for human user.
> I don't care much as long as it is hidden within xen code base,
> but maybe libvirt does.

-drive is mostly a convenience option for human users anyway. Management
tools should use a combination of -blockdev and -device.

Kevin

Re: [XEN PATCH][for-4.19 v3 1/8] xen/include: add macro LOWEST_BIT

[Jan Beulich]

On 23.10.2023 15:19, Nicola Vetrini wrote:
> On 23/10/2023 11:19, Nicola Vetrini wrote:
>> On 23/10/2023 09:48, Jan Beulich wrote:
>>> On 20.10.2023 17:28, Nicola Vetrini wrote:
 --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
 +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
 @@ -246,6 +246,12 @@ constant expressions are required.\""
"any()"}
  -doc_end

 +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern 
 to obtain the value
 +2^ffs(x) for unsigned integers on two's complement architectures
 +(all the architectures supported by Xen satisfy this requirement)."
 +-config=MC3R1.R10.1,reports+={safe, 
 "any_area(any_loc(any_exp(macro(^LOWEST_BIT$"}
 +-doc_end
>>>
>>> This being deviated this way (rather than by SAF-* comments) wants
>>> justifying in the description. You did reply to my respective
>>> comment on v2, but such then (imo) needs propagating into the actual
>>> patch as well.
>>>
>>
>> Sure, will do.
>>
 --- a/docs/misra/deviations.rst
 +++ b/docs/misra/deviations.rst
 @@ -192,6 +192,13 @@ Deviations related to MISRA C:2012 Rules:
 See automation/eclair_analysis/deviations.ecl for the full 
 explanation.
   - Tagged as `safe` for ECLAIR.

 +   * - R10.1
 + - The well-known pattern (x & -x) applied to unsigned integer 
 values on 2's
 +   complement architectures (i.e., all architectures supported 
 by Xen), used
 +   to obtain the value 2^ffs(x), where ffs(x) is the position of 
 the first
 +   bit set. If no bits are set, zero is returned.
 + - Tagged as `safe` for ECLAIR.
>>>
>>> In such an explanation there shall not be any ambiguity. Here I see
>>> an issue with ffs() returning 1-based bit position numbers, which
>>> isn't in line with the use in 2^ffs(x). (Arguably use of ^ itself is
>>> also problematic, as that's XOR in C, not POW. I'd suggest 2^^ffs(x)
>>> or 2**ffs(x).)
>>>
>>
>> Makes sense, I think I'll use an plain english explanation to avoid
>> any confusion
>> about notation.
>>
 --- a/xen/include/xen/macros.h
 +++ b/xen/include/xen/macros.h
 @@ -8,8 +8,11 @@
  #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
  #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))

 -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
 -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
 +/* Returns the 2^ffs(x) or 0, where ffs(x) is the index of the 
 lowest set bit */
 +#define LOWEST_BIT(x) ((x) & -(x))
>>>
>>> I'm afraid my concern regarding this new macro's name (voiced on v2) 
>>> hasn't
>>> been addressed (neither verbally nor by finding a more suitable name).
>>>
>>> Jan
>>
>> I didn't come up with much better names, so I left it as is. Here's a 
>> few ideas:
>>
>> - LOWEST_SET
>> - MASK_LOWEST_SET
>> - MASK_LOWEST_BIT
>> - MASK_FFS_0
>> - LOWEST_ONE
>>
>> and also yours, included here for convenience, even though you felt it
>> was too long:
>>
>> - ISOLATE_LOW_BIT()
>>
>> All maintainers from THE REST are CC-ed, so we can see if anyone has
>> any suggestion.
> 
> There's also LOWEST_BIT_MASK as another possible name.

While naming-wise okay to me, it has the same "too long" issue as
ISOLATE_LOW_BIT(). Considering x86, in the BMI ISA extension, has an
insn doing exactly that (BLSI), taking inspiration from its mnemonic
may also be an option.

Jan

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Jan Beulich]

On 18.10.2023 16:18, Simone Ballarin wrote:
> Add const and pure attributes to address reports
> of MISRA C:2012 Rule 13.1: Initializer lists shall
> not contain persistent side effects
> 
> Add pure attribute to function pdx_to_pfn.
> Add const attribute to functions generated by TYPE_SAFE.
> 
> These functions are used in initializer lists: adding
> the attributes ensures that no effect will be performed
> by them.

Adding the attribute does, according to my understanding, ensure nothing.
The compiler may (but isn't required to) diagnose wrong uses of the
attributes, but it may also make use of the attributes (on the
declaration) without regard to the attribute potentially being wrongly
applied. Since further for inline functions the compiler commonly infers
attributes from the expanded code (discarding the attribute), the only
thing achieved here is a documentation aspect, I think.

> --- a/xen/include/xen/pdx.h
> +++ b/xen/include/xen/pdx.h
> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
>   * @param pdx Page index
>   * @return Obtained pfn after decompressing the pdx
>   */
> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
>  {
>  return (pdx & pfn_pdx_bottom_mask) |
> ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);

Taking this as an example for what I've said above: The compiler can't
know that the globals used by the functions won't change value. Even
within Xen it is only by convention that these variables are assigned
their values during boot, and then aren't changed anymore. Which makes
me wonder: Did you check carefully that around the time the variables
have their values established, no calls to the functions exist (which
might then be subject to folding)?

Additionally - what about the sibling function pfn_to_pdx()?

Jan

[PATCH v2] x86/i8259: do not assume interrupts always target CPU0

[Roger Pau Monne]

Sporadically we have seen the following during AP bringup on AMD platforms
only:

microcode: CPU59 updated from revision 0x830107a to 0x830107a, date = 2023-05-17
microcode: CPU60 updated from revision 0x830104d to 0x830107a, date = 2023-05-17
CPU60: No irq handler for vector 27 (IRQ -2147483648)
microcode: CPU61 updated from revision 0x830107a to 0x830107a, date = 2023-05-17

This is similar to the issue raised on Linux commit 36e9e1eab777e, where they
observed i8259 (active) vectors getting delivered to CPUs different than 0.

On AMD or Hygon platforms adjust the target CPU mask of i8259 interrupt
descriptors to contain all possible CPUs, so that APs will reserve the vector
at startup if any legacy IRQ is still delivered through the i8259.  Note that
if the IO-APIC takes over those interrupt descriptors the CPU mask will be
reset.

Spurious i8259 interrupt vectors however (IRQ7 and IRQ15) can be injected even
when all i8259 pins are masked, and hence would need to be handled on all CPUs.

Do not reserve the PIC spurious vectors on all CPUs, but do check for such
spurious interrupts on all CPUs if the vendor is AMD or Hygon.  Note that once
the vectors get used by devices detecting PIC spurious interrupts will no
longer be possible, however the device should be able to cope with spurious
interrupt.  Such PIC spurious interrupts occurring when the vector is in use by
a local APIC routed source will lead to an extra EOI, which might
unintentionally clear a different vector from ISR.  Note this is already the
current behavior, so assume it's infrequent enough to not cause real issues.

Finally, adjust the printed message to display the CPU where the spurious
interrupt has been received, so it looks like:

microcode: CPU1 updated from revision 0x830107a to 0x830107a, date = 2023-05-17
cpu1: spurious 8259A interrupt: IRQ7
microcode: CPU2 updated from revision 0x830104d to 0x830107a, date = 2023-05-17

Fixes: 3fba06ba9f8b ('x86/IRQ: re-use legacy vector ranges on APs')
Signed-off-by: Roger Pau Monné 
---
Changes since v1:
 - Do not reserved spurious PIC vectors on APs, but still check for spurious
   PIC interrupts.
 - Reword commit message.
---
Not sure if the Fixes tag is the most appropriate here, since AFAICT this is a
hardware glitch, but it makes it easier to see to which versions the fix should
be backported, because Xen previous behavior was to reserve all legacy vectors
on all CPUs.
---
 xen/arch/x86/i8259.c | 29 +++--
 xen/arch/x86/irq.c   |  1 -
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/i8259.c b/xen/arch/x86/i8259.c
index ed9f55abe51e..0935cdf07b65 100644
--- a/xen/arch/x86/i8259.c
+++ b/xen/arch/x86/i8259.c
@@ -37,6 +37,15 @@ static bool _mask_and_ack_8259A_irq(unsigned int irq);
 
 bool bogus_8259A_irq(unsigned int irq)
 {
+if ( smp_processor_id() &&
+ !(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) )
+/*
+ * For AMD/Hygon do spurious PIC interrupt detection on all CPUs, as it
+ * has been observed that during unknown circumstances spurious PIC
+ * interrupts have been delivered to CPUs different than the BSP.
+ */
+return false;
+
 return !_mask_and_ack_8259A_irq(irq);
 }
 
@@ -222,7 +231,8 @@ static bool _mask_and_ack_8259A_irq(unsigned int irq)
 is_real_irq = false;
 /* Report spurious IRQ, once per IRQ line. */
 if (!(spurious_irq_mask & irqmask)) {
-printk("spurious 8259A interrupt: IRQ%d.\n", irq);
+printk("cpu%u: spurious 8259A interrupt: IRQ%u\n",
+   smp_processor_id(), irq);
 spurious_irq_mask |= irqmask;
 }
 /*
@@ -349,7 +359,22 @@ void __init init_IRQ(void)
 continue;
 desc->handler = &i8259A_irq_type;
 per_cpu(vector_irq, cpu)[LEGACY_VECTOR(irq)] = irq;
-cpumask_copy(desc->arch.cpu_mask, cpumask_of(cpu));
+
+/*
+ * The interrupt affinity logic never targets interrupts to offline
+ * CPUs, hence it's safe to use cpumask_all here.
+ *
+ * Legacy PIC interrupts are only targeted to CPU0, but depending on
+ * the platform they can be distributed to any online CPU in hardware.
+ * Note this behavior has only been observed on AMD hardware. In order
+ * to cope install all active legacy vectors on all CPUs.
+ *
+ * IO-APIC will change the destination mask if/when taking ownership of
+ * the interrupt.
+ */
+cpumask_copy(desc->arch.cpu_mask, boot_cpu_data.x86_vendor &
+  (X86_VENDOR_AMD | X86_VENDOR_HYGON) ?
+  &cpumask_all : cpumask_of(cpu));
 desc->arch.vector = LEGACY_VECTOR(irq);
 }
 
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index f42ad539dcd5..a2f9374f5deb 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@

Re: [XEN PATCH][for-4.19 v3 8/8] xen/compat: use BUILD_BUG_ON in CHECK_SIZE macros

[Jan Beulich]

On 20.10.2023 17:28, Nicola Vetrini wrote:
> BUILD_BUG_ON is the preferred way to induce a build error
> upon statically determined incorrect conditions.
> 
> This also fixes a MISRA C:2012 Rule 10.1 violation in the
> previous formulation.
> 
> Signed-off-by: Nicola Vetrini 

Acked-by: Jan Beulich 

Re: [XEN PATCH][for-4.19 v3 1/8] xen/include: add macro LOWEST_BIT

[Nicola Vetrini]

On 23/10/2023 11:19, Nicola Vetrini wrote:

On 23/10/2023 09:48, Jan Beulich wrote:

On 20.10.2023 17:28, Nicola Vetrini wrote:

--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -246,6 +246,12 @@ constant expressions are required.\""
   "any()"}
 -doc_end

+-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern 
to obtain the value

+2^ffs(x) for unsigned integers on two's complement architectures
+(all the architectures supported by Xen satisfy this requirement)."
+-config=MC3R1.R10.1,reports+={safe, 
"any_area(any_loc(any_exp(macro(^LOWEST_BIT$"}

+-doc_end


This being deviated this way (rather than by SAF-* comments) wants
justifying in the description. You did reply to my respective
comment on v2, but such then (imo) needs propagating into the actual
patch as well.



Sure, will do.


--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -192,6 +192,13 @@ Deviations related to MISRA C:2012 Rules:
See automation/eclair_analysis/deviations.ecl for the full 
explanation.

  - Tagged as `safe` for ECLAIR.

+   * - R10.1
+ - The well-known pattern (x & -x) applied to unsigned integer 
values on 2's
+   complement architectures (i.e., all architectures supported 
by Xen), used
+   to obtain the value 2^ffs(x), where ffs(x) is the position of 
the first

+   bit set. If no bits are set, zero is returned.
+ - Tagged as `safe` for ECLAIR.


In such an explanation there shall not be any ambiguity. Here I see
an issue with ffs() returning 1-based bit position numbers, which
isn't in line with the use in 2^ffs(x). (Arguably use of ^ itself is
also problematic, as that's XOR in C, not POW. I'd suggest 2^^ffs(x)
or 2**ffs(x).)



Makes sense, I think I'll use an plain english explanation to avoid
any confusion
about notation.


--- a/xen/include/xen/macros.h
+++ b/xen/include/xen/macros.h
@@ -8,8 +8,11 @@
 #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
 #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))

-#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
-#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
+/* Returns the 2^ffs(x) or 0, where ffs(x) is the index of the 
lowest set bit */

+#define LOWEST_BIT(x) ((x) & -(x))


I'm afraid my concern regarding this new macro's name (voiced on v2) 
hasn't

been addressed (neither verbally nor by finding a more suitable name).

Jan


I didn't come up with much better names, so I left it as is. Here's a 
few ideas:


- LOWEST_SET
- MASK_LOWEST_SET
- MASK_LOWEST_BIT
- MASK_FFS_0
- LOWEST_ONE

and also yours, included here for convenience, even though you felt it
was too long:

- ISOLATE_LOW_BIT()

All maintainers from THE REST are CC-ed, so we can see if anyone has
any suggestion.


There's also LOWEST_BIT_MASK as another possible name.

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

Re: [PATCH 1/2] x86/xen/pvh: Set up percpu for stack canary in 32-bit kernel entry

[Andy Shevchenko]

On Mon, Oct 23, 2023 at 12:10 PM Hou Wenlong
 wrote:
>
> In a 32-bit SMP kernel, the stack canary is a percpu variable accessed
> as %fs:__stack_chk_guard. However, the ABI for PVH entry does not
> specify the %fs register state. It currently works because the initial
> %fs register is 0x10 for QEMU, which is the same as $PVH_DS_SEL.

> %However, for added safety, the percpu should be set up explicitly
> %before calling xen_prepare_pvh(), which accesses the stack canary.

Stray leading % in two lines above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] x86/pv-shim: fix grant table operations for 32-bit guests

[Andrew Cooper]

On 23/10/2023 12:34 pm, David Woodhouse wrote:
> From: David Woodhouse 
>
> When switching to call the shim functions from the normal handlers, the
> compat_grant_table_op() function was omitted, leaving it calling the
> real grant table operations. This leaves a 32-bit shim guest failing to
> set up its real grant table with the parent hypervisor.
>
> Fixes: e7db635f4428 ("x86/pv-shim: Don't modify the hypercall table")
> Signed-off-by: David Woodhouse 

It's a bit more nuanced than that.  It's only for shim built in
non-exclusive mode, which is probably why XenServer's testing never
found this.

Reviewed-by: Andrew Cooper 

Re: [PATCH v1 15/29] xen/asm-generic: introduce stub header xenoprof.h

[Jan Beulich]

On 23.10.2023 13:17, Oleksii wrote:
> On Thu, 2023-10-19 at 12:09 +0200, Jan Beulich wrote:
>> I've made a patch to move #include-s in xen/xenoprof.h, dropping
>> Arm's
>> header (and none going to be needed for RISC-V or PPC). I'll send
>> that
>> patch in due course.
> Could you please share a link with me? I can't find for some reason...

I said "I'll send"; I didn't do, yet. I don't really like sending minor
patches while the tree is closed anyway.

Jan

Re: [PATCH v1 12/29] xen/asm-generic: introduce stub header pci.h

[Jan Beulich]

On 23.10.2023 12:50, Oleksii wrote:
> On Thu, 2023-10-19 at 11:55 +0200, Jan Beulich wrote:
>> While more involved, I still wonder whether xen/pci.h could also
>> avoid
>> including asm/pci.h when !HAS_PCI. Of course there's more than just
>> the
>> #include which then would need #ifdef-ing out.
> It looks like we can get with #ifdef-ing. I'll push a separate patch
> for xen/pci.h.
> 
> It will probably need to remove usage of  everywhere or
> #ifdef-ing it too.
> Which option will be better?

What's "everywhere" here? The only non-arch-dependent use I can spot
is in xen/pci.h.

Jan

Re: [PATCH] x86/pv-shim: fix grant table operations for 32-bit guests

[Jan Beulich]

On 23.10.2023 13:34, David Woodhouse wrote:
> From: David Woodhouse 
> 
> When switching to call the shim functions from the normal handlers, the
> compat_grant_table_op() function was omitted, leaving it calling the
> real grant table operations. This leaves a 32-bit shim guest failing to
> set up its real grant table with the parent hypervisor.
> 
> Fixes: e7db635f4428 ("x86/pv-shim: Don't modify the hypercall table")
> Signed-off-by: David Woodhouse 

A patch with this same effect has been pending for a long time:
https://lists.xen.org/archives/html/xen-devel/2023-03/msg00041.html (v2;
I have a re-based v3 pending locally, awaiting whatever kind of feedback
on v2). The question of whether it was necessary to split out the actual
bug fix was raised yet earlier, in the context of v1 (albeit I'm not
sure whether that was in email or on irc).

Jan

Re: [PATCH v1 26/29] xen/asm-generic: introduce stub header monitor.h

[Oleksii]

On Thu, 2023-10-19 at 13:35 +0200, Jan Beulich wrote:
> On 14.09.2023 16:56, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/include/asm-generic/monitor.h
> > @@ -0,0 +1,64 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/* SPDX-License-Identifier: GPL-2.0 */
> 
> ???
Oh, I'll drop SPDX-License-Identifier: GPL-2.0.
> 
> > +/*
> > + * include/asm-GENERIC/monitor.h
> > + *
> > + * Arch-specific monitor_op domctl handler.
> > + *
> > + * Copyright (c) 2015 Tamas K Lengyel (ta...@tklengyel.com)
> > + * Copyright (c) 2016, Bitdefender S.R.L.
> > + *
> > + */
> > +
> > +#ifndef __ASM_GENERIC_MONITOR_H__
> > +#define __ASM_GENERIC_MONITOR_H__
> > +
> > +#include 
> > +#include 
> 
> No need for this, I don't think?
Yes, I'll drop that too.
> 
> > +static inline
> > +void arch_monitor_allow_userspace(struct domain *d, bool
> > allow_userspace)
> > +{
> > +}
> > +
> > +static inline
> > +int arch_monitor_domctl_op(struct domain *d, struct
> > xen_domctl_monitor_op *mop)
> > +{
> > +    /* No arch-specific monitor ops on GENERIC. */
> > +    return -EOPNOTSUPP;
> > +}
> > +
> > +int arch_monitor_domctl_event(struct domain *d,
> > +  struct xen_domctl_monitor_op *mop);
> > +
> > +static inline
> > +int arch_monitor_init_domain(struct domain *d)
> > +{
> > +    /* No arch-specific domain initialization on GENERIC. */
> > +    return 0;
> > +}
> > +
> > +static inline
> > +void arch_monitor_cleanup_domain(struct domain *d)
> > +{
> > +    /* No arch-specific domain cleanup on GENERIC. */
> > +}
> > +
> > +static inline uint32_t arch_monitor_get_capabilities(struct domain
> > *d)
> > +{
> > +    uint32_t capabilities = 0;
> > +
> > +    return capabilities;
> 
> Just "return 0"?
Thanks. I'll update that part.


~ Oleksii

Re: [PATCH v1 22/29] xen/asm-generic: introduce stub header delay.h

[Oleksii]

On Thu, 2023-10-19 at 13:30 +0200, Jan Beulich wrote:
> On 14.09.2023 16:56, Oleksii Kurochko wrote:
> > The patch introduces header stub necessry for full Xen build.
> > 
> > Signed-off-by: Oleksii Kurochko 
> > ---
> >  xen/include/asm-generic/delay.h | 21 +
> >  1 file changed, 21 insertions(+)
> >  create mode 100644 xen/include/asm-generic/delay.h
> 
> Besides the implementation below not being acceptable, imo we should
> do
> away with asm/delay.h altogether. x86 can rename __udelay() to
> udelay(),
> and then the declaration can move to xen/delay.h.
> 
It makes sense. I'll do that.

~ Oleksii

[PATCH] x86/pv-shim: fix grant table operations for 32-bit guests

[David Woodhouse]

From: David Woodhouse 

When switching to call the shim functions from the normal handlers, the
compat_grant_table_op() function was omitted, leaving it calling the
real grant table operations. This leaves a 32-bit shim guest failing to
set up its real grant table with the parent hypervisor.

Fixes: e7db635f4428 ("x86/pv-shim: Don't modify the hypercall table")
Signed-off-by: David Woodhouse 
---
 xen/common/compat/grant_table.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index e00bc24a34..af98eade17 100644
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -63,6 +63,11 @@ int compat_grant_table_op(
 unsigned int i, cmd_op;
 XEN_GUEST_HANDLE_PARAM(void) cnt_uop;
 
+#ifdef CONFIG_PV_SHIM
+if ( unlikely(pv_shim) )
+return pv_shim_grant_table_op(cmd, uop, count);
+#endif
+
 set_xen_guest_handle(cnt_uop, NULL);
 cmd_op = cmd & GNTTABOP_CMD_MASK;
 if ( cmd_op != GNTTABOP_cache_flush )
-- 
2.34.1




smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v1 21/29] xen/asm-generic: introduce stub header altp2m.h

[Oleksii]

On Thu, 2023-10-19 at 13:27 +0200, Jan Beulich wrote:
> On 14.09.2023 16:56, Oleksii Kurochko wrote:
> > The patch introduces header stub necessry for full Xen build.
> > 
> > Signed-off-by: Oleksii Kurochko 
> > ---
> >  xen/include/asm-generic/altp2m.h | 34
> > 
> >  1 file changed, 34 insertions(+)
> >  create mode 100644 xen/include/asm-generic/altp2m.h
> 
> While odd to be needed, this looks largely okay for the moment. Just
> one remark:
> 
> > --- /dev/null
> > +++ b/xen/include/asm-generic/altp2m.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_ALTP2M_H
> > +#define __ASM_GENERIC_ALTP2M_H
> > +
> > +#include 
> > +
> > +struct domain;
> > +struct vcpu;
> > +
> > +/* Alternate p2m on/off per domain */
> > +static inline bool altp2m_active(const struct domain *d)
> > +{
> > +    /* Not implemented on GENERIC. */
> > +    return false;
> > +}
> > +
> > +/* Alternate p2m VCPU */
> > +static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v)
> 
> uint16_t is what x86 wants, but even on Arm it's suspicious. For a
> generic header I'd say make it unsigned int, thus also eliminating
> the question whether xen/types.h should be included here.

Thanks. I'll make sure to update the patch accordingly.

~ Oleksii

Re: [PATCH v1 20/29] xen/asm-generic: introduce stub header div64.h

[Oleksii]

On Thu, 2023-10-19 at 13:12 +0200, Jan Beulich wrote:
> On 14.09.2023 16:56, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/include/asm-generic/div64.h
> > @@ -0,0 +1,24 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_DIV64
> > +#define __ASM_GENERIC_DIV64
> > +
> > +#include 
> > +
> > +# define do_div(n,base) ({  \
> > +    uint32_t __base = (base);   \
> > +    uint32_t __rem; \
> > +    __rem = ((uint64_t)(n)) % __base;   \
> > +    (n) = ((uint64_t)(n)) / __base; \
> > +    __rem;  \
> > + })
> 
> While I'm fine with having just the BITS_PER_LONG == 64
> implementation
> here, this then still needs to have the #if retained that Arm has.
> Only
> with that will it then be fine to have a blank between # and define.
> 
> There are style issues though: A blank is missing after the comma,
> and according to recent agreement leading underscores should not be
> used for symbols like the ones here anymore (I also wonder whether
> "base" is really a good name for the symbol; "divisor" may be more to
> the point). There are also excess parentheses around the two cast
> expressions.
Thanks. I'll take mentioned into account.

~ Oleskii

Re: [PATCH v1 19/29] xen/asm-generic: introduce stub header hardirq.h

[Oleksii]

On Thu, 2023-10-19 at 13:04 +0200, Jan Beulich wrote:
> On 14.09.2023 16:56, Oleksii Kurochko wrote:
> > The patch introduces header stub necessry for full Xen build.
> > 
> > Signed-off-by: Oleksii Kurochko 
> 
> I agree this one can be generalized from Arm's, but ...
> 
> > --- /dev/null
> > +++ b/xen/include/asm-generic/hardirq.h
> > @@ -0,0 +1,27 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_HARDIRQ_H
> > +#define __ASM_GENERIC_HARDIRQ_H
> > +
> > +#include 
> > +
> > +typedef struct {
> > +    unsigned long __softirq_pending;
> > +    unsigned int __local_irq_count;
> > +} __cacheline_aligned irq_cpustat_t;
> 
> ... where is __cacheline_aligned going to come from without inclusion
> of xen/cache.h (as Arm has it)?
xen/cache.h should be added. thanks.

~ Oleksii

Re: [PATCH v1 18/29] xen/asm-generic: introduce stub header smp.h

[Oleksii]

On Thu, 2023-10-19 at 12:58 +0200, Jan Beulich wrote:
> On 14.09.2023 16:56, Oleksii Kurochko wrote:
> > The patch introduces header stub necessry for full Xen build.
> > 
> > Signed-off-by: Oleksii Kurochko 
> 
> Assuming you expect RISC-V to get away without its own smp.h, just
> one remark:
Not really, I've introduced only things necessary for Xen's full build.

It looks like we have a situation as with device.h header ( in this
patch series) . Probably smp.h header should be only in an arch-
specific folder. I'll apply to smp.h the same solution as for device.h
when we get to the same page on it.

Except what I introduced in this patch other functions and macros will
be in smp.h. Such as:

raw_smp_processor_id
smp_processor_id
extern void smp_clear_cpu_maps (void);
extern void smp_init_cpus(void);
extern unsigned int smp_get_max_cpus(void);
void smp_setup_processor_id(unsigned long boot_cpu_id);

/*
 * Mapping between linux logical cpu index and hartid.
 */
extern unsigned long __cpuid_to_hartid_map[NR_CPUS];
#define cpuid_to_hartid_map(cpu) __cpuid_to_hartid_map[cpu]

#define cpu_physical_id(cpu) cpuid_to_hartid_map(cpu)

Mostly all of the header can be generic but again all the mentioned
above functions are used only for RISC-V and ARM. ( probably I missed
something ).

> 
> > --- /dev/null
> > +++ b/xen/include/asm-generic/smp.h
> > @@ -0,0 +1,30 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_SMP_H
> > +#define __ASM_GENERIC_SMP_H
> > +
> > +#ifndef __ASSEMBLY__
> > +#include 
> > +#include 
> > +#endif
> 
> This #endif need moving ...
> 
> > +DECLARE_PER_CPU(cpumask_var_t, cpu_sibling_mask);
> > +DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask);
> 
> ... at least down here, if #includ-ing by assembly files is really
> necessary to permit. Preferably the #ifndef would be dropped, though.
> 
> Jan

~ Oleksii

Re: [PATCH v1 17/29] xen/asm-generic: introduce stub header percpu.h

[Oleksii]

On Thu, 2023-10-19 at 12:39 +0200, Jan Beulich wrote:
> On 14.09.2023 16:56, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/include/asm-generic/percpu.h
> > @@ -0,0 +1,35 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_PERCPU_H__
> > +#define __ASM_GENERIC_PERCPU_H__
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +#include 
> > +
> > +extern char __per_cpu_start[], __per_cpu_data_end[];
> > +extern unsigned long __per_cpu_offset[NR_CPUS];
> > +void percpu_init_areas(void);
> > +
> > +#define per_cpu(var, cpu)  \
> > +    (*RELOC_HIDE(&per_cpu__##var, __per_cpu_offset[cpu]))
> > +
> > +#define this_cpu(var) \
> > +    (*RELOC_HIDE(&per_cpu__##var,
> > __per_cpu_offset[get_processor_id()]))
> > +
> > +#define per_cpu_ptr(var, cpu)  \
> > +    (*RELOC_HIDE(var, __per_cpu_offset[cpu]))
> > +#define this_cpu_ptr(var) \
> > +    (*RELOC_HIDE(var, get_processor_id()))
> > +
> > +#endif
> > +
> > +#endif /* __ASM_GENERIC_PERCPU_H__ */
> 
> This looks okay, just one request: Please use smp_processor_id(). You
> may have seen on the Matrix channel that there's the intention to do
> away with the get_processor_id() alias that's used in only very few
> places.
Thanks. I'll update the patch.

~ Oleksii

Re: [PATCH v1 15/29] xen/asm-generic: introduce stub header xenoprof.h

[Oleksii]

On Thu, 2023-10-19 at 12:09 +0200, Jan Beulich wrote:
> I've made a patch to move #include-s in xen/xenoprof.h, dropping
> Arm's
> header (and none going to be needed for RISC-V or PPC). I'll send
> that
> patch in due course.
Could you please share a link with me? I can't find for some reason...

~ Oleksii

Re: [PATCH v2] x86/shutdown: change default reboot method preference

[Roger Pau Monné]

On Tue, Oct 03, 2023 at 01:35:25PM +0200, Roger Pau Monné wrote:
> On Wed, Sep 27, 2023 at 10:21:44AM +0200, Jan Beulich wrote:
> > On 15.09.2023 09:43, Roger Pau Monne wrote:
> > > The current logic to chose the preferred reboot method is based on the 
> > > mode Xen
> > > has been booted into, so if the box is booted from UEFI, the preferred 
> > > reboot
> > > method will be to use the ResetSystem() run time service call.
> > > 
> > > However, that method seems to be widely untested, and quite often leads 
> > > to a
> > > result similar to:
> > > 
> > > Hardware Dom0 shutdown: rebooting machine
> > > [ Xen-4.18-unstable  x86_64  debug=y  Tainted:   C]
> > > CPU:0
> > > RIP:e008:[<0017>] 0017
> > > RFLAGS: 00010202   CONTEXT: hypervisor
> > > [...]
> > > Xen call trace:
> > >[<0017>] R 0017
> > >[] S 83207eff7b50
> > >[] F machine_restart+0x1da/0x261
> > >[] F apic_wait_icr_idle+0/0x37
> > >[] F smp_call_function_interrupt+0xc7/0xcb
> > >[] F call_function_interrupt+0x20/0x34
> > >[] F do_IRQ+0x150/0x6f3
> > >[] F common_interrupt+0x132/0x140
> > >[] F 
> > > arch/x86/acpi/cpu_idle.c#acpi_idle_do_entry+0x113/0x129
> > >[] F 
> > > arch/x86/acpi/cpu_idle.c#acpi_processor_idle+0x3eb/0x5f7
> > >[] F arch/x86/domain.c#idle_loop+0xec/0xee
> > > 
> > > 
> > > Panic on CPU 0:
> > > FATAL TRAP: vector = 6 (invalid opcode)
> > > 
> > > 
> > > Which in most cases does lead to a reboot, however that's unreliable.
> > > 
> > > Change the default reboot preference to prefer ACPI over UEFI if 
> > > available and
> > > not in reduced hardware mode.
> > > 
> > > This is in line to what Linux does, so it's unlikely to cause issues on 
> > > current
> > > and future hardware, since there's a much higher chance of vendors testing
> > > hardware with Linux rather than Xen.
> > > 
> > > Add a special case for one Acer model that does require being rebooted 
> > > using
> > > ResetSystem().  See Linux commit 0082517fa4bce for rationale.
> > > 
> > > I'm not aware of using ACPI reboot causing issues on boxes that do have
> > > properly implemented ResetSystem() methods.
> > 
> > A data point from a new system I'm still in the process of setting up: The
> > ACPI reboot method, as used by Linux, unconditionally means a warm reboot.
> > The EFI method, otoh, properly distinguishes "reboot=warm" from our default
> > of explicitly requesting cold reboot. (Without taking the EFI path, I
> > assume our write to the relevant BDA location simply has no effect, for
> > this being a legacy BIOS thing, and the system apparently defaults to warm
> > reboot when using the ACPI method.)
> 
> This is unfortunate, but IMO not as worse as getting a #UD or any
> other fault while attempting a reboot.  We can always force this
> system to use UEFI reboot, if that does work better than ACPI.
> 
> > Clearly, as a secondary effect, this system adds to my personal experience
> > of so far EFI reboot consistently working on all x86 hardware I have (had)
> > direct access to. (That said, this is the first non-Intel system, which
> > likely biases my overall experience.)
> 
> I can try to gather some data, I can at least tell you that the Intel
> NUC11TNHi7 TGL does also hit a fault when attempting UEFI reboot.
> The above crash was from a Dell PowerEdge R6625.  I do recall seeing
> this with other boxes on the Citrix lab, but don't know the exact
> models.  I'm quite sure other downstreams can provide similar
> feedback.

As a further data point, Dasharo [0] a coreboot downstream was also
providing a firmware with a broken ResetSystem() method, and they
didn't notice until someone reported errors on Xen reboot:

https://github.com/Dasharo/edk2/pull/99/commits/dee75be10ac9387168bd3a8cad0f1ec6e372129a

It's quite clear no one is testing ResetSystem(), the UEFI spec
doesn't mandate using it, and we are just hurting ourselves by forcing
its usage.

Regards, Roger.

[0] https://github.com/Dasharo

Re: [PATCH v1 12/29] xen/asm-generic: introduce stub header pci.h

[Oleksii]

On Thu, 2023-10-19 at 11:55 +0200, Jan Beulich wrote:
> While more involved, I still wonder whether xen/pci.h could also
> avoid
> including asm/pci.h when !HAS_PCI. Of course there's more than just
> the
> #include which then would need #ifdef-ing out.
It looks like we can get with #ifdef-ing. I'll push a separate patch
for xen/pci.h.

It will probably need to remove usage of  everywhere or
#ifdef-ing it too.
Which option will be better?

~ Oleksii

Re: [PATCH v1 10/29] xen/asm-generic: introduce stub header iommu.h

[Jan Beulich]

On 23.10.2023 12:43, Oleksii wrote:
> On Thu, 2023-10-19 at 11:44 +0200, Jan Beulich wrote:
>> On 14.09.2023 16:56, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/include/asm-generic/iommu.h
>>> @@ -0,0 +1,17 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +#ifndef __ASM_GENERIC_IOMMU_H__
>>> +#define __ASM_GENERIC_IOMMU_H__
>>> +
>>> +struct arch_iommu {
>>> +};
>>> +
>>> +#endif /* __ASM_IOMMU_H__ */
>> This one's perhaps slightly more "interesting": Yes, we have a
>> HAS_PASSTHROUGH Kconfig option, which both Arm and x86 select. But it
>> is in principle possible to support guests without any kind of IOMMU
>> (permitting solely emulated and PV devices). In which case what's
>> (imo) needed here in addition is
>>
>> #ifdef CONFIG_HAS_PASSTHROUGH
>> # error
>> #endif
> I am not 100% sure but not all platform has support of IOMMU.
> 
> And I thought that passthrough it is when a device is fully committed
> to  a guest domain with all MMIO things. So it is a question of
> properly mapping MMIO to guest domain. Am I right?

Yes. And do you expect you will get away with such a stub implementation
when you actually start supporting pass-through on RISC-V?

Jan

Re: [PATCH v1 06/29] xen/asm-generic: introduce stub header grant_table.h

[Jan Beulich]

On 23.10.2023 12:32, Oleksii wrote:
> On Thu, 2023-10-19 at 11:19 +0200, Jan Beulich wrote:
>> On 14.09.2023 16:56, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/include/asm-generic/grant_table.h
>>> @@ -0,0 +1,14 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +#ifndef __ASM_GENERIC_GRANTTABLE_H__
>>> +#define __ASM_GENERIC_GRANTTABLE_H__
>>> +
>>> +#endif /* __ASM_GENERIC_GRANTTABLE_H__ */
>>
>> This isn't going to work with CONFIG_GRANT_TABLE=y, is it?
> Yes, it won't work with CONFIG_GRANT_TABLE=y. Missed that as
> CONFIG_GRANT_TABLE is disabled for RISC-V.
> 
> It looks like it should be moved to arch specific folder but as I
> mentioned before I don't see a lot of sense to introduce an empty
> header for new arch each time when it will be needed to enable full Xen
> build.

Here I'm okay with an almost empty header in asm-generic/, so long as
it properly rejects CONFIG_GRANT_TABLE=y (indicating in the diagnostic
that for this to build an arch needs to have its own header).

However, then the question again arises where it wouldn't be possible
to have xen/grant_table.h avoid including asm/grant_table.h when
!CONFIG_GRANT_TABLE, eliminating the question whether to have a fallback
header in asm-generic/. If that's not possible, the reason may be a good
thing to put in the description here.

Jan

Re: [PATCH v1 11/29] xen/asm-generic: introduce stub header mem_access.h

[Oleksii]

On Thu, 2023-10-19 at 11:51 +0200, Jan Beulich wrote:
> On 14.09.2023 16:56, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/include/asm-generic/mem_access.h
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_MEM_ACCESS
> > +#define __ASM_GENERIC_MEM_ACCESS
> > +
> > +#endif
> 
> Does xen/mem_access.h actually need to include asm/mem_access.h when
> !CONFIG_MEM_ACCESS? Without that, I don't think this header is
> needed.
Yes, it won't needed. I'll update xen/mem_access.h header and push a
separate patch.

Thanks.

~ Oleksii

Re: [PATCH v1 10/29] xen/asm-generic: introduce stub header iommu.h

[Oleksii]

On Thu, 2023-10-19 at 11:44 +0200, Jan Beulich wrote:
> On 14.09.2023 16:56, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/include/asm-generic/iommu.h
> > @@ -0,0 +1,17 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_IOMMU_H__
> > +#define __ASM_GENERIC_IOMMU_H__
> > +
> > +struct arch_iommu {
> > +};
> > +
> > +#endif /* __ASM_IOMMU_H__ */
> This one's perhaps slightly more "interesting": Yes, we have a
> HAS_PASSTHROUGH Kconfig option, which both Arm and x86 select. But it
> is in principle possible to support guests without any kind of IOMMU
> (permitting solely emulated and PV devices). In which case what's
> (imo) needed here in addition is
> 
> #ifdef CONFIG_HAS_PASSTHROUGH
> # error
> #endif
I am not 100% sure but not all platform has support of IOMMU.

And I thought that passthrough it is when a device is fully committed
to  a guest domain with all MMIO things. So it is a question of
properly mapping MMIO to guest domain. Am I right?

~ Oleksii

Re: [PATCH v1 05/29] xen/asm-generic: introduce stub header event.h

[Jan Beulich]

On 23.10.2023 12:23, Oleksii wrote:
> On Thu, 2023-10-19 at 11:18 +0200, Jan Beulich wrote:
>> On 14.09.2023 16:56, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/include/asm-generic/event.h
>>> @@ -0,0 +1,39 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +#ifndef __ASM_GENERIC_EVENT_H__
>>> +#define __ASM_GENERIC_EVENT_H__
>>> +
>>> +#include 
>>> +
>>> +static inline void vcpu_mark_events_pending(struct vcpu *v)
>>> +{
>>> +}
>>
>> While this will satisfy callers from a build perspective, no port
>> will be functional with an implementation like this. Yet the
>> generic headers need to provide the required functionality, not
>> just build stubs.
> It makes sense but then we will have the similar patches when new
> architecture is introduced.
> 
>>
>> Going further in the series, I won't repeat this kind of comment.
>> Unless others disagree, my view is that headers put here should
>> be of use beyond initial bring-up of a new port.
>>
> Then we have two options here:
> 1. leave only declaration of the function.

Which would then require a stub to be introduced in the arch, or else
linking will fail (unless all calls can be compile-time eliminated,
which I doubt would be the case here).

As before a requirement (imo) is that headers introduced into
asm-generic/ are functional. An arch would introduce its own instance
if it wants to do certain things better, or if it has further needs.

Jan

> 2. remove it from asm-generic.
> 
> ~ Oleksii

Re: [PATCH v1 09/29] xen/asm-generic: introduce stub header iocap.h

[Oleksii]

On Thu, 2023-10-19 at 11:25 +0200, Jan Beulich wrote:
> On 14.09.2023 16:56, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/include/asm-generic/iocap.h
> > @@ -0,0 +1,17 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_IOCAP_H__
> > +#define __ASM_GENERIC_IOCAP_H__
> > +
> > +#define cache_flush_permitted(d)    \
> > +    (!rangeset_is_empty((d)->iomem_caps))
> > +
> > +#endif /* __ASM_GENERIC_IOCAP_H__ */
> 
> This again wants to eliminate Arm's header in exchange.
Basically yes.

The situation is the same as with device.h [1].

So let's get to the same page about [1] and then I'll apply
the solution for this header too.

[1]https://lore.kernel.org/xen-devel/48c3c78d-465f-8102-87a3-cef3a5d48...@suse.com/

~ Oleksii

Re: [PATCH v1 04/29] xen/asm-generic: introduce stub header device.h

[Jan Beulich]

On 23.10.2023 12:12, Oleksii wrote:
> On Thu, 2023-10-19 at 11:14 +0200, Jan Beulich wrote:
>> On 14.09.2023 16:56, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/include/asm-generic/device.h
>>> @@ -0,0 +1,65 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +#ifndef __ASM_GENERIC_DEVICE_H__
>>> +#define __ASM_GENERIC_DEVICE_H__
>>> +
>>> +struct dt_device_node;
>>> +
>>> +enum device_type
>>> +{
>>> +    DEV_DT,
>>> +    DEV_PCI,
>>> +};
>>
>> Are both of these really generic?
>>
>>> +struct device {
>>> +    enum device_type type;
>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>> +    struct dt_device_node *of_node; /* Used by drivers imported
>>> from Linux */
>>> +#endif
>>> +};
>>> +
>>> +enum device_class
>>> +{
>>> +    DEVICE_SERIAL,
>>> +    DEVICE_IOMMU,
>>> +    DEVICE_GIC,
>>
>> This one certainly is Arm-specific.
> Yes, but the definition of GIC sounds common, so I decided to leave it.
> But it can be changed.
> 
>>
>>> +    DEVICE_PCI_HOSTBRIDGE,
>>
>> And this one's PCI-specific.
>>
>> Overall same question as before: Are you expecting that RISC-V is
>> going to
>> get away without a customized header? I wouldn't think so.
> At least right now, I am using the same header device.h as in ARM,

Are you? I just double checked, and I can't see yours matching theirs.
First example of a difference is them having struct dev_archdata.

Jan

> and
> there wasn't a need for a customized version of the header.
> 
> ~ Oleksii
> 

Re: [PATCH v1 08/29] xen/asm-generic: introduce stub hypercall.h

[Oleksii]

On Thu, 2023-10-19 at 11:24 +0200, Jan Beulich wrote:
> On 14.09.2023 16:56, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/include/asm-generic/hypercall.h
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_HYPERCALL_H__
> > +#define __ASM_GENERIC_HYPERCALL_H__
> > +
> > +#endif /* __ASM_GENERIC_HYPERCALL_H__ */
> 
> This lacks the "should not be included directly" guard that x86 and
> Arm
> headers have.
Thanks. I'll add it.

~ Oleksii

Re: MISRA C:2012 D4.11 caution on staging

[Nicola Vetrini]

On 18/10/2023 23:33, Stefano Stabellini wrote:

On Wed, 18 Oct 2023, Julien Grall wrote:

On 18/10/2023 13:52, Nicola Vetrini wrote:
> On 18/10/2023 14:38, Julien Grall wrote:
> > Hi Nicola,
> >
> > On 18/10/2023 13:30, Nicola Vetrini wrote:
> > > On 17/10/2023 15:28, Julien Grall wrote:
> > > I tested this, and the report is prevented by the ASSERT. It's up to the
> > > maintainers to
> > > decide how do you want to proceed: my suggestion is deviating it,
> >
> > It is not clear to me what would you deviate. Can you clarify?
> >
> > Cheers,
>
> The memcpy call, as in:
>
> /* SAF-x-false-positive-eclair */
> memcpy(d->handle, config->handle, sizeof(d->handle));

I am not in favor of this deviation. It could be a false positive 
today, but

it may not be in the future.

I much prefer the ASSERT() version or the rework.


Just to be clear about the next steps, Nicola are you OK with sending a
patch with the ASSERT or would you prefer Julien or someone else to do
it?


Anyone can pick this up.

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

Re: [PATCH v1 04/29] xen/asm-generic: introduce stub header device.h

[Jan Beulich]

On 23.10.2023 12:17, Oleksii wrote:
> On Thu, 2023-10-19 at 13:12 +0100, Julien Grall wrote:
>> Hi,
>>
>> On 19/10/2023 12:41, Jan Beulich wrote:
>>> On 19.10.2023 13:27, Julien Grall wrote:
 that doesn't involve one arch to symlink headers from another
 arch.
>>>
>>> Whether to use symlinks or #include "../../arch/..." or yet
>>> something else is
>>> a matter of mechanics.
>>
>> #include "../../arch/../" is pretty much in the same category. This
>> is 
>> simply hiding the fact they could be in asm-generic.
>>
>> Anyway, I have shared my view. Let see what the others thinks.
> I have the same point: if something is shared at least between two
> arch, it should go to ASM-generic.

I continue to disagree: What if one pair of arch-es shares one set
of things, and another shares another set? You can't fit both pairs
then with a single fallback header (unless of course you make it a
big #if / #else / #endif, which I'm inclined to say isn't the goal
with headers put in asm-generic/).

Jan

> And that is the reason why I pushed device.h header to asm-generic.
> It is needed to rename some stuff (e.g... GIC ) in it or add some
> ifdefs.
> 
> ~ Oleksii

Re: [PATCH v1 06/29] xen/asm-generic: introduce stub header grant_table.h

[Oleksii]

On Thu, 2023-10-19 at 11:19 +0200, Jan Beulich wrote:
> On 14.09.2023 16:56, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/include/asm-generic/grant_table.h
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_GRANTTABLE_H__
> > +#define __ASM_GENERIC_GRANTTABLE_H__
> > +
> > +#endif /* __ASM_GENERIC_GRANTTABLE_H__ */
> 
> This isn't going to work with CONFIG_GRANT_TABLE=y, is it?
Yes, it won't work with CONFIG_GRANT_TABLE=y. Missed that as
CONFIG_GRANT_TABLE is disabled for RISC-V.

It looks like it should be moved to arch specific folder but as I
mentioned before I don't see a lot of sense to introduce an empty
header for new arch each time when it will be needed to enable full Xen
build.

~ Oleksii

Re: [XEN PATCH][for-4.19 v4 8/8] docs/misra: exclude three more files

[Nicola Vetrini]

On 23/10/2023 12:23, Jan Beulich wrote:

On 23.10.2023 11:56, Nicola Vetrini wrote:

These files should not conform to MISRA guidelines at the moment,
therefore they are added to the exclusion list.

Signed-off-by: Nicola Vetrini 
---
These exclusions are automatically picked up by ECLAIR's automation
to hide reports originating from these files.

Changes in v4:
- Fixed typo


While there was an adjustment here, I don't think it invalidated
Stefano's R-b?

Jan


Yes

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

Re: [XEN PATCH][for-4.19 v4 6/8] xen/console: remove stub definition in consoled.h

[Nicola Vetrini]

On 23/10/2023 12:21, Jan Beulich wrote:

On 23.10.2023 11:56, Nicola Vetrini wrote:

The stub  definition of 'consoled_guest_tx' can be removed, since its
its single caller uses the implementation built with PV_SHIM enabled.

Fixes: 5ef49f185c2d ("x86/pv-shim: shadow PV console's page for L2 
DomU")

Signed-off-by: Nicola Vetrini 


Where did Stefano's R-b go?

Jan


I didn't notice it. Nothing has changed since, so it should be included

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

Re: [PATCH v1 02/29] xen/asm-generic: introduce stub header paging.h

[Jan Beulich]

On 23.10.2023 11:40, Oleksii wrote:
> On Thu, 2023-10-19 at 11:05 +0200, Jan Beulich wrote:
>> On 14.09.2023 16:56, Oleksii Kurochko wrote:
>>> The patch introduces stub header needed for full Xen build.
>>>
>>> Signed-off-by: Oleksii Kurochko 
>>> ---
>>>  xen/include/asm-generic/paging.h | 17 +
>>>  1 file changed, 17 insertions(+)
>>>  create mode 100644 xen/include/asm-generic/paging.h
>>>
>>> diff --git a/xen/include/asm-generic/paging.h b/xen/include/asm-
>>> generic/paging.h
>>> new file mode 100644
>>> index 00..2aab63b536
>>> --- /dev/null
>>> +++ b/xen/include/asm-generic/paging.h
>>> @@ -0,0 +1,17 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +#ifndef __ASM_GENERIC_PAGING_H__
>>> +#define __ASM_GENERIC_PAGING_H__
>>> +
>>> +#define paging_mode_translate(d)   (1)
>>> +#define paging_mode_external(d)(1)
>>> +
>>> +#endif /* __ASM_GENERIC_PAGING_H__ */
>>
>> Looks okay, but wants accompanying by dropping (i.e. effectively
>> moving)
>> Arm's respective header. The description than also wants adjusting
>> (it
>> wasn't quite suitable anyway, as there's missing context).
> If I understand you correctly, I'll re-use ARM's header, but I am not
> sure I know how the description should be changed except that it can be
> more specific regarding which one header is introduced.

Well, first of all context is missing in "full Xen build" - PPC has recently
reached that point already, and both Arm and x86 have been fully building
for quite some time. And then, as said elsewhere, imo headers needed solely
for building (but being otherwise non-functional) shouldn't be introduced.
At which point it may make sense to give a pointer as to where the
definitions are needed, and clarify why what is introduced is sufficient /
appropriate as fallback for a certain "common" default case of operation.

Jan

Re: [PATCH v3 5/5] [FUTURE] xen/arm: enable vPCI for domUs

[Anthony PERARD]

On Mon, Oct 09, 2023 at 03:57:44PM -0400, Stewart Hildebrand wrote:
> Select HAS_VPCI_GUEST_SUPPORT in Kconfig for enabling vPCI in domUs.
> 
> Set the pci flags in xen_arch_domainconfig to enable vPCI if a pci device has
> been specified in the xl domain config file.
> 
> Signed-off-by: Stewart Hildebrand 
> ---
> As the tag implies, this patch is not intended to be merged (yet).
> 
> Note that CONFIG_HAS_VPCI_GUEST_SUPPORT is not currently used in the upstream
> code base. It will be used by the vPCI series [1]. This patch is intended to 
> be
> merged as part of the vPCI series. I'll coordinate with Volodymyr to include
> this in the vPCI series or resend afterwards. Meanwhile, I'll include it here
> until the Kconfig and xen_arch_domainconfig prerequisites have been committed.
> 
> v2->v3:
> * set pci flags in toolstack
> 
> v1->v2:
> * new patch
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2023-08/msg02361.html
> ---
>  tools/libs/light/libxl_arm.c | 3 +++
>  xen/arch/arm/Kconfig | 1 +

Can we have two different patch? One for the hypervisor, one for the
tools. Or does the both depends on each other, somehow?

Is guest creation going to fails if we set XEN_DOMCTL_CONFIG_PCI_VPCI
without HAS_VPCI_GUEST_SUPPORT ?

Is the guest is going to fail to run, or fail at creation if the
hypervisor HAS_VPCI_GUEST_SUPPORT, but we didn't set
XEN_DOMCTL_CONFIG_PCI_VPCI?

Thanks,

-- 
Anthony PERARD

Re: [PATCH v3 2/4] CHANGELOG.md: Use "xenbits.xenproject.org" in links

[Jan Beulich]

On 23.10.2023 11:21, Henry Wang wrote:
> Compared to "xenbits.xen.org", "xenbits.xenproject.org" appeared
> later as a name, with the intention of becoming the canonical one.
> Therefore, this commit unifies all the links to use "xenproject"
> in the links.
> 
> Signed-off-by: Henry Wang 

Acked-by: Jan Beulich 

Re: [PATCH v1 05/29] xen/asm-generic: introduce stub header event.h

[Oleksii]

On Thu, 2023-10-19 at 11:18 +0200, Jan Beulich wrote:
> On 14.09.2023 16:56, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/include/asm-generic/event.h
> > @@ -0,0 +1,39 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_EVENT_H__
> > +#define __ASM_GENERIC_EVENT_H__
> > +
> > +#include 
> > +
> > +static inline void vcpu_mark_events_pending(struct vcpu *v)
> > +{
> > +}
> 
> While this will satisfy callers from a build perspective, no port
> will be functional with an implementation like this. Yet the
> generic headers need to provide the required functionality, not
> just build stubs.
It makes sense but then we will have the similar patches when new
architecture is introduced.

> 
> Going further in the series, I won't repeat this kind of comment.
> Unless others disagree, my view is that headers put here should
> be of use beyond initial bring-up of a new port.
> 
Then we have two options here:
1. leave only declaration of the function.
2. remove it from asm-generic.

~ Oleksii

Re: [XEN PATCH][for-4.19 v4 8/8] docs/misra: exclude three more files

[Jan Beulich]

On 23.10.2023 11:56, Nicola Vetrini wrote:
> These files should not conform to MISRA guidelines at the moment,
> therefore they are added to the exclusion list.
> 
> Signed-off-by: Nicola Vetrini 
> ---
> These exclusions are automatically picked up by ECLAIR's automation
> to hide reports originating from these files.
> 
> Changes in v4:
> - Fixed typo

While there was an adjustment here, I don't think it invalidated
Stefano's R-b?

Jan

Re: [XEN PATCH][for-4.19 v4 6/8] xen/console: remove stub definition in consoled.h

[Jan Beulich]

On 23.10.2023 11:56, Nicola Vetrini wrote:
> The stub  definition of 'consoled_guest_tx' can be removed, since its
> its single caller uses the implementation built with PV_SHIM enabled.
> 
> Fixes: 5ef49f185c2d ("x86/pv-shim: shadow PV console's page for L2 DomU")
> Signed-off-by: Nicola Vetrini 

Where did Stefano's R-b go?

Jan

Re: [PATCH v1 04/29] xen/asm-generic: introduce stub header device.h

[Oleksii]

On Thu, 2023-10-19 at 13:12 +0100, Julien Grall wrote:
> Hi,
> 
> On 19/10/2023 12:41, Jan Beulich wrote:
> > On 19.10.2023 13:27, Julien Grall wrote:
> > > that doesn't involve one arch to symlink headers from another
> > > arch.
> > 
> > Whether to use symlinks or #include "../../arch/..." or yet
> > something else is
> > a matter of mechanics.
> 
> #include "../../arch/../" is pretty much in the same category. This
> is 
> simply hiding the fact they could be in asm-generic.
> 
> Anyway, I have shared my view. Let see what the others thinks.
I have the same point: if something is shared at least between two
arch, it should go to ASM-generic.

And that is the reason why I pushed device.h header to asm-generic.
It is needed to rename some stuff (e.g... GIC ) in it or add some
ifdefs.

~ Oleksii

Re: [PATCH v1 02/29] xen/asm-generic: introduce stub header paging.h

[Jan Beulich]

On 23.10.2023 11:35, Oleksii wrote:
> On Thu, 2023-10-19 at 11:35 +0100, Julien Grall wrote:
>> On 19/10/2023 10:05, Jan Beulich wrote:
>>> On 14.09.2023 16:56, Oleksii Kurochko wrote:
 The patch introduces stub header needed for full Xen build.

 Signed-off-by: Oleksii Kurochko 
 ---
   xen/include/asm-generic/paging.h | 17 +
   1 file changed, 17 insertions(+)
   create mode 100644 xen/include/asm-generic/paging.h

 diff --git a/xen/include/asm-generic/paging.h b/xen/include/asm-
 generic/paging.h
 new file mode 100644
 index 00..2aab63b536
 --- /dev/null
 +++ b/xen/include/asm-generic/paging.h
 @@ -0,0 +1,17 @@
 +/* SPDX-License-Identifier: GPL-2.0-only */
 +#ifndef __ASM_GENERIC_PAGING_H__
 +#define __ASM_GENERIC_PAGING_H__
 +
 +#define paging_mode_translate(d)   (1)
 +#define paging_mode_external(d)(1)
>> This is more a question for Jan, in the past I recall you asked the 
>> macor to evaluate the argument. Shouldn't we do the same here?
> Could you please share a link?
> I am not sure that I am in the context.

There's no particular link to be provided, I think. It is simply good
practice to make sure macros evaluate each of the parameters exactly
once. This is simply to avoid surprises at use sites, where function-
like macro invocations - as that terminology says - look like
function invocations, where every argument expression is also
evaluated exactly once (and in unspecified order).

Jan

Re: [PATCH v1 04/29] xen/asm-generic: introduce stub header device.h

[Oleksii]

On Thu, 2023-10-19 at 11:14 +0200, Jan Beulich wrote:
> On 14.09.2023 16:56, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/include/asm-generic/device.h
> > @@ -0,0 +1,65 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_DEVICE_H__
> > +#define __ASM_GENERIC_DEVICE_H__
> > +
> > +struct dt_device_node;
> > +
> > +enum device_type
> > +{
> > +    DEV_DT,
> > +    DEV_PCI,
> > +};
> 
> Are both of these really generic?
> 
> > +struct device {
> > +    enum device_type type;
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +    struct dt_device_node *of_node; /* Used by drivers imported
> > from Linux */
> > +#endif
> > +};
> > +
> > +enum device_class
> > +{
> > +    DEVICE_SERIAL,
> > +    DEVICE_IOMMU,
> > +    DEVICE_GIC,
> 
> This one certainly is Arm-specific.
Yes, but the definition of GIC sounds common, so I decided to leave it.
But it can be changed.

> 
> > +    DEVICE_PCI_HOSTBRIDGE,
> 
> And this one's PCI-specific.
> 
> Overall same question as before: Are you expecting that RISC-V is
> going to
> get away without a customized header? I wouldn't think so.
At least right now, I am using the same header device.h as in ARM, and
there wasn't a need for a customized version of the header.

~ Oleksii

Xen 4.18 rc4

[Henry Wang]

Hi all,

Xen 4.18 rc4 is tagged. You can check that out from xen.git:

git://xenbits.xen.org/xen.git 4.18.0-rc4

For your convenience there is also a tarball at:
https://downloads.xenproject.org/release/xen/4.18.0-rc4/xen-4.18.0-rc4.tar.gz

And the signature is at:
https://downloads.xenproject.org/release/xen/4.18.0-rc4/xen-4.18.0-rc4.tar.gz.sig

Please send bug reports and test reports to xen-devel@lists.xenproject.org.
When sending bug reports, please CC relevant maintainers and me
(henry.w...@arm.com).

Kind regards,
Henry

[XEN PATCH][for-4.19 v4 8/8] docs/misra: exclude three more files

[Nicola Vetrini]

These files should not conform to MISRA guidelines at the moment,
therefore they are added to the exclusion list.

Signed-off-by: Nicola Vetrini 
---
These exclusions are automatically picked up by ECLAIR's automation
to hide reports originating from these files.

Changes in v4:
- Fixed typo
---
 docs/misra/exclude-list.json | 12 
 1 file changed, 12 insertions(+)

diff --git a/docs/misra/exclude-list.json b/docs/misra/exclude-list.json
index 575ed22a7f67..b858a0baa106 100644
--- a/docs/misra/exclude-list.json
+++ b/docs/misra/exclude-list.json
@@ -145,6 +145,10 @@
 "rel_path": "common/zstd/*",
 "comment": "Imported from Linux, ignore for now"
 },
+{
+"rel_path": "common/symbols-dummy.c",
+"comment": "The resulting code is not included in the final Xen 
binary, ignore for now"
+},
 {
 "rel_path": "crypto/*",
 "comment": "Origin is external and documented in 
crypto/README.source"
@@ -189,6 +193,14 @@
 "rel_path": "include/acpi/acpixf.h",
 "comment": "Imported from Linux, ignore for now"
 },
+{
+  "rel_path": "include/acpi/acexcep.h",
+  "comment": "Imported from Linux, ignore for now"
+},
+{
+  "rel_path": "include/acpi/acglobal.h",
+  "comment": "Imported from Linux, ignore for now"
+},
 {
 "rel_path": "include/xen/acpi.h",
 "comment": "Imported from Linux, ignore for now"
-- 
2.34.1

Re: [PATCH] tools/xendomains: Only save/restore/migrate if supported by xenlight

[Anthony PERARD]

On Thu, Oct 19, 2023 at 12:50:52PM +, Luca Fancellu wrote:
> 
> 
> > On 22 Mar 2023, at 13:58, Peter Hoyes  wrote:
> > 
> > From: Peter Hoyes 
> > 
> > Saving, restoring and migrating domains are not currently supported on
> > arm and arm64 platforms, so xendomains prints the warning:
> > 
> >  An error occurred while saving domain:
> >  command not implemented
> > 
> > when attempting to run `xendomains stop`. It otherwise continues to shut
> > down the domains cleanly, with the unsupported steps skipped.
> > 
> > Use `xl help` to detect whether save/restore/migrate is supported by the
> > platform. If not, do not attempt to run the corresponding command.
> > 
> > Signed-off-by: Peter Hoyes 
> > ---
> 
> Hi Anthony,
> 
> Regarding this patch, is there any update?
> 
> I’m asking just because here 
> https://patchwork.kernel.org/project/xen-devel/patch/20230322135800.3869458-1-peter.ho...@arm.com/
>  it seems you were volunteering to fix that in another way.

I did, 
https://patchwork.kernel.org/project/xen-devel/patch/20230519162454.50337-1-anthony.per...@citrix.com/
But looks like I need to update the patch.

I think it's a bit late for 3.18, but I should be able to update the
patch for next release.

Cheers,

-- 
Anthony PERARD

[XEN PATCH][for-4.19 v4 5/8] x86/vm_event: add missing include for hvm_vm_event_do_resume

[Nicola Vetrini]

The missing header makes the declaration visible when the function
is defined, thereby fixing a violation of MISRA C:2012 Rule 8.4.

Fixes: 1366a0e76db6 ("x86/vm_event: add hvm/vm_event.{h,c}")
Signed-off-by: Nicola Vetrini 
Acked-by: Tamas K Lengyel 
---
 xen/arch/x86/hvm/vm_event.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/hvm/vm_event.c b/xen/arch/x86/hvm/vm_event.c
index 3b064bcfade5..c1af230e7aed 100644
--- a/xen/arch/x86/hvm/vm_event.c
+++ b/xen/arch/x86/hvm/vm_event.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 static void hvm_vm_event_set_registers(const struct vcpu *v)
-- 
2.34.1

[XEN PATCH][for-4.19 v4 3/8] x86: add deviation comments for asm-only functions

[Nicola Vetrini]

As stated in rules.rst, functions used only in asm code
are allowed to have no prior declaration visible when being
defined, hence these functions are deviated.
This also fixes violations of MISRA C:2012 Rule 8.4.

Signed-off-by: Nicola Vetrini 
Reviewed-by: Stefano Stabellini 
---
Changes in v3:
- added SAF deviations for vmx counterparts to svm functions.
---
 xen/arch/x86/hvm/svm/intr.c  | 1 +
 xen/arch/x86/hvm/svm/nestedsvm.c | 1 +
 xen/arch/x86/hvm/svm/svm.c   | 2 ++
 xen/arch/x86/hvm/vmx/intr.c  | 1 +
 xen/arch/x86/hvm/vmx/vmx.c   | 2 ++
 xen/arch/x86/hvm/vmx/vvmx.c  | 1 +
 xen/arch/x86/traps.c | 1 +
 xen/arch/x86/x86_64/traps.c  | 1 +
 8 files changed, 10 insertions(+)

diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
index 192e17ebbfbb..bd9dc560bbc6 100644
--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -123,6 +123,7 @@ static void svm_enable_intr_window(struct vcpu *v, struct 
hvm_intack intack)
 vmcb, general1_intercepts | GENERAL1_INTERCEPT_VINTR);
 }
 
+/* SAF-1-safe */
 void svm_intr_assist(void)
 {
 struct vcpu *v = current;
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index a09b6abaaeaf..c80d59e0728e 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -1441,6 +1441,7 @@ nestedsvm_vcpu_vmexit(struct vcpu *v, struct 
cpu_user_regs *regs,
 }
 
 /* VCPU switch */
+/* SAF-1-safe */
 void nsvm_vcpu_switch(void)
 {
 struct cpu_user_regs *regs = guest_cpu_user_regs();
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 24c417ca7199..40ba69bcdfa1 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1056,6 +1056,7 @@ static void noreturn cf_check svm_do_resume(void)
 reset_stack_and_jump(svm_asm_do_resume);
 }
 
+/* SAF-1-safe */
 void svm_vmenter_helper(void)
 {
 const struct cpu_user_regs *regs = guest_cpu_user_regs();
@@ -2586,6 +2587,7 @@ const struct hvm_function_table * __init start_svm(void)
 return &svm_function_table;
 }
 
+/* SAF-1-safe */
 void svm_vmexit_handler(void)
 {
 struct cpu_user_regs *regs = guest_cpu_user_regs();
diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index fd719c4c01d2..1805aafb086d 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -224,6 +224,7 @@ void vmx_sync_exit_bitmap(struct vcpu *v)
 }
 }
 
+/* SAF-1-safe */
 void vmx_intr_assist(void)
 {
 struct hvm_intack intack;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 1edc7f1e919f..0936a263154e 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4035,6 +4035,7 @@ static void undo_nmis_unblocked_by_iret(void)
   guest_info | VMX_INTR_SHADOW_NMI);
 }
 
+/* SAF-1-safe */
 void vmx_vmexit_handler(struct cpu_user_regs *regs)
 {
 unsigned long exit_qualification, exit_reason, idtv_info, intr_info = 0;
@@ -4787,6 +4788,7 @@ static void lbr_fixup(void)
 }
 
 /* Returns false if the vmentry has to be restarted */
+/* SAF-1-safe */
 bool vmx_vmenter_helper(const struct cpu_user_regs *regs)
 {
 struct vcpu *curr = current;
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 16b0ef82b6c8..99d93b869307 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1490,6 +1490,7 @@ static void nvmx_eptp_update(void)
 __vmwrite(EPT_POINTER, get_shadow_eptp(curr));
 }
 
+/* SAF-1-safe */
 void nvmx_switch_guest(void)
 {
 struct vcpu *v = current;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e1356f696aba..6af35a468199 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2265,6 +2265,7 @@ void asm_domain_crash_synchronous(unsigned long addr)
 }
 
 #ifdef CONFIG_DEBUG
+/* SAF-1-safe */
 void check_ist_exit(const struct cpu_user_regs *regs, bool ist_exit)
 {
 const unsigned int ist_mask =
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index e03e80813e36..5963d26d7848 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -266,6 +266,7 @@ void show_page_walk(unsigned long addr)
l1_table_offset(addr), l1e_get_intpte(l1e), pfn);
 }
 
+/* SAF-1-safe */
 void do_double_fault(struct cpu_user_regs *regs)
 {
 unsigned int cpu;
-- 
2.34.1

[XEN PATCH][for-4.19 v4 2/8] x86: add deviations for variables only used in asm code

[Nicola Vetrini]

To avoid a violation of MISRA C:2012 Rule 8.4, as permitted
by docs/misra/rules.rst.

Signed-off-by: Nicola Vetrini 
Reviewed-by: Stefano Stabellini 
---
Changes in v3:
- Edited commit message
- Add two new variables
---
 xen/arch/x86/include/asm/asm_defns.h | 1 +
 xen/arch/x86/setup.c | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/include/asm/asm_defns.h 
b/xen/arch/x86/include/asm/asm_defns.h
index baaaccb26e17..a2516de7749b 100644
--- a/xen/arch/x86/include/asm/asm_defns.h
+++ b/xen/arch/x86/include/asm/asm_defns.h
@@ -31,6 +31,7 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, "
  * gets set up by the containing function.
  */
 #ifdef CONFIG_FRAME_POINTER
+/* SAF-1-safe */
 register unsigned long current_stack_pointer asm("rsp");
 # define ASM_CALL_CONSTRAINT , "+r" (current_stack_pointer)
 #else
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 08ba1f95d635..4480f08de718 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -75,6 +75,7 @@ static bool __initdata opt_invpcid = true;
 boolean_param("invpcid", opt_invpcid);
 bool __read_mostly use_invpcid;
 
+/* SAF-1-safe Only used in asm code and within this source file */
 unsigned long __read_mostly cr4_pv32_mask;
 
 /*  Linux config option: propagated to domain0. */
@@ -147,12 +148,13 @@ cpumask_t __read_mostly cpu_present_map;
 unsigned long __read_mostly xen_phys_start;
 
 char __section(".init.bss.stack_aligned") __aligned(STACK_SIZE)
-cpu0_stack[STACK_SIZE];
+cpu0_stack[STACK_SIZE]; /* SAF-1-safe Only used in asm code and below */
 
 /* Used by the BSP/AP paths to find the higher half stack mapping to use. */
 void *stack_start = cpu0_stack + STACK_SIZE - sizeof(struct cpu_info);
 
 /* Used by the boot asm to stash the relocated multiboot info pointer. */
+/* SAF-1-safe */
 unsigned int __initdata multiboot_ptr;
 
 struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 0, 0, -1 };
-- 
2.34.1

[XEN PATCH][for-4.19 v4 7/8] x86/mem_access: make function static

[Nicola Vetrini]

The function is used only within this file, and therefore can be static.

No functional change.

Signed-off-by: Nicola Vetrini 
Acked-by: Tamas K Lengyel 
---
Changes in v3:
- style fix
---
 xen/arch/x86/mm/mem_access.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index c472fa1ee58b..d3dd5ab5ef8b 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -249,9 +249,9 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
 return (p2ma != p2m_access_n2rwx);
 }
 
-int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
-  struct p2m_domain *ap2m, p2m_access_t a,
-  gfn_t gfn)
+static int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
+ struct p2m_domain *ap2m, p2m_access_t a,
+ gfn_t gfn)
 {
 mfn_t mfn;
 p2m_type_t t;
-- 
2.34.1

[XEN PATCH][for-4.19 v4 4/8] x86/grant: switch included header to make declarations visible

[Nicola Vetrini]

The declarations for {create,replace}_grant_p2m_mapping are
not visible when these functions are defined, therefore the right
header needs to be included to allow them to be visible.

Signed-off-by: Nicola Vetrini 
Reviewed-by: Stefano Stabellini 
---
Changes in v3:
- asm/paging.h can be replaced with mm-frame.h, because just the
  definition of mfn_t is needed.
---
 xen/arch/x86/hvm/grant_table.c | 3 +--
 xen/arch/x86/include/asm/hvm/grant_table.h | 2 ++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/grant_table.c b/xen/arch/x86/hvm/grant_table.c
index 30d51d54a949..afe449d8882c 100644
--- a/xen/arch/x86/hvm/grant_table.c
+++ b/xen/arch/x86/hvm/grant_table.c
@@ -9,8 +9,7 @@
 
 #include 
 
-#include 
-
+#include 
 #include 
 
 int create_grant_p2m_mapping(uint64_t addr, mfn_t frame,
diff --git a/xen/arch/x86/include/asm/hvm/grant_table.h 
b/xen/arch/x86/include/asm/hvm/grant_table.h
index 33c1da1a25f3..01e23f79b8cf 100644
--- a/xen/arch/x86/include/asm/hvm/grant_table.h
+++ b/xen/arch/x86/include/asm/hvm/grant_table.h
@@ -10,6 +10,8 @@
 #ifndef __X86_HVM_GRANT_TABLE_H__
 #define __X86_HVM_GRANT_TABLE_H__
 
+#include 
+
 #ifdef CONFIG_HVM
 
 int create_grant_p2m_mapping(uint64_t addr, mfn_t frame,
-- 
2.34.1

[XEN PATCH][for-4.19 v4 0/8] Fix or deviate various instances of missing declarations

[Nicola Vetrini]

The patches in this series aim to fix or deviate various instances where a
function or variable do not have a declaration visible when such entity is
defined (in violation of MISRA C:2012 Rule 8.4).
An exception listed under docs/misra/rules.rst allows asm-only functions and
variables to be exempted, while the other instances are either changed
(e.g., making them static) or a missing header inclusion is added.

Nicola Vetrini (8):
  xen: modify or add declarations for variables where needed
  x86: add deviations for variables only used in asm code
  x86: add deviation comments for  asm-only functions
  x86/grant: switch included header to make declarations visible
  x86/vm_event: add missing include for hvm_vm_event_do_resume
  xen/console: remove stub definition in consoled.h
  x86/mem_access: make function static
  docs/misra: exclude three more files

 docs/misra/exclude-list.json   | 12 
 xen/arch/arm/include/asm/setup.h   |  3 +++
 xen/arch/arm/include/asm/smp.h |  3 +++
 xen/arch/arm/platform_hypercall.c  |  2 +-
 xen/arch/x86/cpu/mcheck/mce.c  |  7 ---
 xen/arch/x86/hvm/grant_table.c |  3 +--
 xen/arch/x86/hvm/svm/intr.c|  1 +
 xen/arch/x86/hvm/svm/nestedsvm.c   |  1 +
 xen/arch/x86/hvm/svm/svm.c |  2 ++
 xen/arch/x86/hvm/vm_event.c|  1 +
 xen/arch/x86/hvm/vmx/intr.c|  1 +
 xen/arch/x86/hvm/vmx/vmx.c |  2 ++
 xen/arch/x86/hvm/vmx/vvmx.c|  1 +
 xen/arch/x86/include/asm/asm_defns.h   |  1 +
 xen/arch/x86/include/asm/hvm/grant_table.h |  2 ++
 xen/arch/x86/irq.c |  2 +-
 xen/arch/x86/mm/mem_access.c   |  6 +++---
 xen/arch/x86/setup.c   |  4 +++-
 xen/arch/x86/traps.c   |  1 +
 xen/arch/x86/x86_64/traps.c|  1 +
 xen/include/xen/consoled.h |  7 ---
 xen/include/xen/symbols.h  |  1 +
 22 files changed, 46 insertions(+), 18 deletions(-)

--
2.34.1