[PATCH v2] MAINTAINERS: consolidate vm-event/monitor entry

[Jan Beulich]

If the F: description is to be trusted, the two xen/arch/x86/hvm/
lines were fully redundant with the earlier wildcard ones. Arch header
files, otoh, were no longer covered by anything as of the move from
include/asm-*/ to arch/*/include/asm/. Further also generalize (by
folding) the x86- and Arm-specific mem_access.c entries.

Finally, again assuming the F: description can be trusted, there's no
point listing arch/, common/, and include/ entries separately. Fold
them all.

Signed-off-by: Jan Beulich 
---
v2: Further fold patterns.
---
Triggered by me looking at the entry in the context of Oleksii's RISC-V
preparatory patch.

--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -558,20 +558,9 @@ R: Alexandru Isaila 
 S: Supported
 F: tools/misc/xen-access.c
-F: xen/arch/*/monitor.c
-F: xen/arch/*/vm_event.c
-F: xen/arch/arm/mem_access.c
-F: xen/arch/x86/include/asm/hvm/monitor.h
-F: xen/arch/x86/include/asm/hvm/vm_event.h
-F: xen/arch/x86/mm/mem_access.c
-F: xen/arch/x86/hvm/monitor.c
-F: xen/arch/x86/hvm/vm_event.c
-F: xen/common/mem_access.c
-F: xen/common/monitor.c
-F: xen/common/vm_event.c
-F: xen/include/*/mem_access.h
-F: xen/include/*/monitor.h
-F: xen/include/*/vm_event.h
+F: xen/*/mem_access.[ch]
+F: xen/*/monitor.[ch]
+F: xen/*/vm_event.[ch]
 
 VPCI
 M: Roger Pau Monné 

Re: [PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Jan Beulich]

On 30.08.2023 19:02, Andrew Cooper wrote:
> On 30/08/2023 5:13 pm, Jan Beulich wrote:
>> On 30.08.2023 17:28, Andrew Cooper wrote:
>>> On 30/08/2023 4:12 pm, Jan Beulich wrote:
 On 30.08.2023 16:35, Andrew Cooper wrote:
> On 29/08/2023 3:08 pm, Jan Beulich wrote:
>> On 29.08.2023 15:43, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -1074,8 +1074,27 @@ int arch_set_info_guest(
>>>  #endif
>>>  flags = c(flags);
>>>  
>>> +if ( !compat )
>>> +{
>>> +if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
>>> + c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
>>> +return -EINVAL;
>>> +}
>>> +
>>>  if ( is_pv_domain(d) )
>>>  {
>>> +/*
>>> + * Prior to Xen 4.11, dr5 was used to hold the emulated-only
>>> + * subset of dr7, and dr4 was unused.
>>> + *
>>> + * In Xen 4.11 and later, dr4/5 are written as zero, ignored 
>>> for
>>> + * backwards compatibility, and dr7 emulation is handled
>>> + * internally.
>>> + */
>>> +for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
>>> +if ( !access_ok(c(debugreg[i]), sizeof(long)) )
>> Don't you mean __addr_ok() here, i.e. not including the
>> is_compat_arg_xlat_range() check? (Else I would have asked why
>> sizeof(long), but that question resolves itself with using the other
>> macro.)
> For now, I'm simply moving a check from set_debugreg() earlier in
> arch_set_info_guest().
>
> I think it would be beneficial to keep that change independent.
 Hmm, difficult. I'd be okay if you indeed moved the other check. But
 you duplicate it here, and duplicating questionable code is, well,
 questionable.
>>> It can't be removed in set_debugreg() because that's used in other paths
>>> too.
>> Sure, I understand that.
>>
>>> And the error from set_debugreg() can't fail arch_set_info_guest()
>>> because that introduces a failure after mutation of the vCPU state.
>>>
>>> This isn't a fastpath.  It's used approximately once per vCPU lifetime.
>> But fast or not isn't the point here.
> 
> No.  The point is no change from the existing code.

Having thought about it over night: It's not nice but okay to duplicate
the bogus check here, but then please say that and why you do so in the
description. With that suitably added
Acked-by: Jan Beulich 

> If you think it's wrong, it in a separate change and don't block this fix.

I would like to ask you to think about the opposite case occurring: I'm
pretty sure you wouldn't let me get away. Either - like so often - you'd
simply not reply anymore at a certain point, or - like here - you'd
expect me to adjust to your expectations.

Jan

[qemu-mainline test] 182570: tolerable FAIL - PUSHED

[osstest service owner]

flight 182570 qemu-mainline real [real]
flight 182578 qemu-mainline real-retest [real]
http://logs.test-lab.xenproject.org/osstest/logs/182570/
http://logs.test-lab.xenproject.org/osstest/logs/182578/

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-amd64-xl-multivcpu 20 guest-localmigrate/x10 fail pass in 
182578-retest
 test-arm64-arm64-xl-credit1   8 xen-bootfail pass in 182578-retest
 test-armhf-armhf-xl-vhd  13 guest-start fail pass in 182578-retest

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

version targeted for testing:
 qemuu156618d9ea67f2f2e31d9dedd97f2dcccbe6808c
baseline version:
 qemuu813bac3d8d70d85cb7835f7945eb9eed84c2d8d0

Last test of basis   182555  2023-08-29 14:08:50 Z1 days
Testing same since   182570  2023-08-30 16:37:20 Z0 days1 attempts


Peopl

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

[Stefano Stabellini]

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

Yes, this is fine, it was always intended to be possible to add the
name of the deviation or a short comment in the in-code comment


> /*
>  * User address space access functions.
>  *
>  * Copyright 1997 Andi Kleen 
>  * Copyright 1997 Linus Torvalds
>  * Copyright 2002 Andi Kleen 
>  */
> 
> -- 
> Simone Ballarin, M.Sc.
> 
> Field Application Engineer, BUGSENG (https://bugseng.com)
> 

[PATCH v2] docs/misra: add 14.3

[Stefano Stabellini]

From: Stefano Stabellini 

Add 14.3, with project-wide deviations.

Also take the opportunity to clarify that parameters of function pointer
types are expected to have names (Rule 8.2).

Signed-off-by: Stefano Stabellini 
---
v2:
- remove 14.4
- better wording for the 8.2 clarification
- add while(0) and while(1) to 14.3
- add ?: to the deviation
- remove list of statements the rule applies to
---
 docs/misra/rules.rst | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
index db30632b93..9389976290 100644
--- a/docs/misra/rules.rst
+++ b/docs/misra/rules.rst
@@ -234,7 +234,8 @@ maintainers if you want to suggest a change.
* - `Rule 8.2 
`_
  - Required
  - Function types shall be in prototype form with named parameters
- -
+ - Clarification: both function and function pointers types shall
+   have named parameters.
 
* - `Rule 8.3 
`_
  - Required
@@ -332,6 +333,17 @@ maintainers if you want to suggest a change.
  - A loop counter shall not have essentially floating type
  -
 
+   * - `Rule 14.3 
`_
+ - Required
+ - Controlling expressions shall not be invariant
+ - Due to the extensive usage of IS_ENABLED, sizeof compile-time
+   checks, and other constructs that are detected as errors by MISRA
+   C scanners, managing the configuration of a MISRA C scanner for
+   this rule would be unmanageable. Thus, this rule is adopted with
+   a project-wide deviation on if and ?: statements.
+
+   while(0) and while(1) are allowed.
+
* - `Rule 16.7 
`_
  - Required
  - A switch-expression shall not have essentially Boolean type
-- 
2.25.1

Re: [PATCH] xen/scsifront: shost_priv() can never return NULL

[Martin K. Petersen]

On Tue, 22 Aug 2023 08:48:17 +0200, Juergen Gross wrote:

> There is no need to check whether shost_priv() returns a non-NULL
> value, as the pointer returned is just an offset to the passed in
> parameter.
> 
> While at it replace an open coded shost_priv() instance.
> 
> 
> [...]

Applied to 6.6/scsi-queue, thanks!

[1/1] xen/scsifront: shost_priv() can never return NULL
  https://git.kernel.org/mkp/scsi/c/73c7881b5066

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] docs/misra: add 14.3 and 14.4

[Stefano Stabellini]

On Wed, 30 Aug 2023, Bertrand Marquis wrote:
> > On 30 Aug 2023, at 09:58, Jan Beulich  wrote:
> > 
> > On 30.08.2023 09:54, Bertrand Marquis wrote:
> >>> On 30 Aug 2023, at 02:59, Stefano Stabellini  
> >>> wrote:
> >>> +   * - `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 of integers, pointers, and chars to boolean
> >>> +   are allowed
> >> 
> >> I am a bit wondering here what is remaining after this deviation.
> > 
> > Hmm, good point - floating point (and alike) types, which we don't use 
> > anyway.
> 
> So we accept the rule but we deviate all cases that would apply.
> I do not think we should do that.

In the past we have been accepting rules that don't apply to Xen because
it is not much that they don't apply. It is that we have no violations
and we don't think there is any risk of getting violations in the
future.

But theoretically they apply, and for example if we end up with floating
points in Xen at some point in the future we would want rules affecting
floating points to apply, hence we accepted rules about floating points.

This rule is a bit different though: we deviate most of the rule and the
remaining part is small. In addition, we have no violations and we don't
think there is any risk of getting violations in the future for what's
left of the rule.

So I don't know if it is worth adding the rule or not. I think we should
ask Roberto next time. For now, I remove it from this patch.

Re: [PATCH] docs/misra: add 14.3 and 14.4

[Stefano Stabellini]

On Wed, 30 Aug 2023, Bertrand Marquis wrote:
> Hi Stefano,
> 
> > On 30 Aug 2023, at 02:59, Stefano Stabellini  wrote:
> > 
> > From: Stefano Stabellini 
> > 
> > Add 14.3, with a project-wide deviations on if statements.
> > Add 14.4, clarifying that implicit conversions of integers, chars and
> > pointers to bool are allowed.
> > 
> > Also take the opportunity to clarify that parameters of function pointer
> > types are expected to have names (Rule 8.2).
> > 
> > Signed-off-by: Stefano Stabellini 
> > ---
> > docs/misra/rules.rst | 20 +++-
> > 1 file changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> > index db30632b93..6cde4feeae 100644
> > --- a/docs/misra/rules.rst
> > +++ b/docs/misra/rules.rst
> > @@ -234,7 +234,7 @@ maintainers if you want to suggest a change.
> >* - `Rule 8.2 
> > `_
> >  - Required
> >  - Function types shall be in prototype form with named parameters
> > - -
> > + - Function pointer types shall have named parameters too.
> 
> 
> I would just modify to Function and Function pointers types shall be ...

Sure, I can do that.


> > 
> >* - `Rule 8.3 
> > `_
> >  - Required
> > @@ -332,6 +332,24 @@ maintainers if you want to suggest a change.
> >  - A loop counter shall not have essentially floating type
> >  -
> > 
> > +   * - `Rule 14.3 
> > `_
> > + - Required
> > + - Controlling expressions shall not be invariant
> > + - Due to the extensive usage of IS_ENABLED, sizeof compile-time
> > +   checks, and other constructs that are detected as errors by MISRA
> > +   C scanners, managing the configuration of a MISRA C scanner for
> > +   this rule would be unmanageable. Thus, this rule is adopted with
> > +   a project-wide deviation on 'if' statements. The rule only
> > +   applies to while, for, do ... while, ?:, and switch statements.
> 
> Didn't we also said that we would accept while(0) and while(1) ?
> Also i agree with Jan, ? is really the same as if so we should not treat it 
> differently.

I took the list of things the rule applies to from the text of the rule
itself. However, I think you are right about the ?: and it should be
deviated together with if. I can also add while(0) and while(1).

Re: [PATCH] docs/misra: add 14.3 and 14.4

[Stefano Stabellini]

On Wed, 30 Aug 2023, Jan Beulich wrote:
> On 30.08.2023 02:59, Stefano Stabellini wrote:
> > --- a/docs/misra/rules.rst
> > +++ b/docs/misra/rules.rst
> > @@ -234,7 +234,7 @@ maintainers if you want to suggest a change.
> > * - `Rule 8.2 
> > `_
> >   - Required
> >   - Function types shall be in prototype form with named parameters
> > - -
> > + - Function pointer types shall have named parameters too.
> 
> This isn't an exception; do we really need to state such? I would have
> expected something to appear here only if we intended to deviate certain
> constructs.

Yes, it is not an exception. However, as there was genuine confusion in
the community about whether the rule should apply or not to function
pointer types I think it would be good to clarify. To avoid any doubts
in the future. My preference is to keep this as clarification.



> > @@ -332,6 +332,24 @@ maintainers if you want to suggest a change.
> >   - A loop counter shall not have essentially floating type
> >   -
> >  
> > +   * - `Rule 14.3 
> > `_
> > + - Required
> > + - Controlling expressions shall not be invariant
> > + - Due to the extensive usage of IS_ENABLED, sizeof compile-time
> > +   checks, and other constructs that are detected as errors by MISRA
> > +   C scanners, managing the configuration of a MISRA C scanner for
> > +   this rule would be unmanageable. Thus, this rule is adopted with
> > +   a project-wide deviation on 'if' statements. The rule only
> > +   applies to while, for, do ... while, ?:, and switch statements.
> 
> The sizeof() aspect mentioned particularly applies to switch() as well.
> Furthermore ?: is really only shorthand for simple if(), so I don't see
> treating it different from if() as helpful.

I'll answer in another email.



> That said, I'd be a little hesitant to give an ack here anyway. If you'd
> split 14.3 and 14.4, I'd be happy to ack 14.4's addition.
> 
> Jan
> 
> > +   * - `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 of integers, pointers, and chars to boolean
> > +   are allowed
> > +
> > * - `Rule 16.7 
> > `_
> >   - Required
> >   - A switch-expression shall not have essentially Boolean type
> 

RE: [EXT] Re: xen arm64 low power sleep support

[Stefano Stabellini]

On Wed, 30 Aug 2023, Anthony Chan wrote:
> On Tue, 29 Aug 2023, Stefano Stabellini wrote:
> > On Tue, 29 Aug 2023, Anthony Chan wrote:
> > > Hi all,
> > >
> > > My name is Tony and I've been researching/developing using Xen for
> > potential upcoming uses in our embedded systems.  I started with Xen
> > using Xilinx tools about a year ago and still have lots to learn about what 
> > it
> > can to do in the embedded space.  So far, I've managed to integrate Xen
> > and Linux into an existing product that exclusively runs bare-metal code on
> > a ZynqMP SoC and migrate some of the functionality into custom Linux
> > driver/userspace.
> > >
> > > I'm now looking at low power support, for now at least between Xen
> > (4.16) and Linux (5.15) dom0.  I've tried a few different Linux kernel
> > configs around power management and each time I try to suspend from
> > linux dom0 (via sysfs or systemctl), Xen will watchdog on dom0 guest.
> > AFAIK, Xen should trap on a 'WFI' from guests, but from what I can tell
> > debugging through the linux suspend process is it's spinning in a 'suspend-
> > to-idle' loop before it can get to issuing a 'WFI' or using PSCI interface 
> > to
> > notify Xen.  I'm beginning to suspect that 'low power' support for
> > embedded arm64 just isn't quite there yet, or am I missing something in
> > the configs?
> > >
> > > I realize this could very well be a Linux 'issue' but checking here 
> > > first.  I
> > know Xen presents a flattened device tree to Linux without CPU idle-state
> > nodes and maybe this is causing the linux guest to only do the suspend-
> > to-idle mode?  I should mention that I'm booting up using dom0less
> > feature if that matters.
> >
> >
> > Hi Anthony,
> >
> > Assuming you are using the default Xen command line parameters for
> > Xilinx boards: sched=null vwfi=native, then if the guest uses WFI, the CPU
> > will execute WFI directly and go into low power mode.
> Yes, using these command line params.
> 
> > Given the issue you are describing, I am suspecting the guest is not issuing
> > WFI: that is simple and known to work. Instead, I suspect that Linux might
> > be trying to use PSCI_suspend in a way that is not supported or well-
> > implemented by Xen.
> >
> > Can you check? You can add a printk in Linux
> > drivers/firmware/psci/psci.c:__psci_cpu_suspend or in Xen
> > xen/arch/arm/vpsci.c:do_psci_0_2_cpu_suspend
> Instrumented both places it doesn't appear to reach there.  In 
> kernel/power/suspend.c, there's a call to s2idle_loop that it's currently 
> 'stuck' in and I think it doesn't get to the psci suspend your referring till 
> afterwards, when suspend_ops->enter is called.  Unfortunately, without any 
> idle-states nodes in the FDT, the only suspend state Linux is defaults to is 
> 'suspend to idle'.
 
The fact that Linux uses "suspend to idle" is not a problem because as I
mentioned WFI or PSCI_suspent are not different on Xen. That part is OK.

However, if the issue is not PSCI_suspend then I don't have another easy
guess. Please post a full stack trace or more information about the
error in Linux and I might be able to see where it is coming from.

Re: QEMU features useful for Xen development?

[Stefano Stabellini]

Hi Alex,

Thanks for reaching out. QEMU is an important development tool for the
Xen community and we are using QEMU as part of our upstream gitlab-ci
testing, see automation/scripts/qemu-*.

As Xen is gaining R52 and R82 support, it would be great to be able to
use QEMU for development and testing there as well, but I don't think
QEMU can emulate EL2 properly for the Cortex-R architecture. We would
need EL2 support in the GIC/timer for R52/R82 as well.

On Cortex-As, in addition to a PCI root complex and an arbitrary PCI
device, SMMUv3 emulation (both stages) and GICv3 ITS are needed to be
able to test PCI Passthrough. However, if I recall correctly SMMUv3
emulation in QEMU might not be complete enough to enable us to use it.

For Virtio, using QEMU on target (not develpment/testing, but
production), it would greatly help if we could improve the build system
to only build what is strictly necessary for the xenpvh machine to run.

Cheers,

Stefano


On Wed, 30 Aug 2023, Alex Bennée wrote:
> Dear Xen community,
> 
> Linaro is significantly invested in QEMU development, with a special
> focus on Arm-related aspects. We recognize the value of QEMU as a
> readily available software reference platform for projects that need to
> test their software well before the availability of real hardware.
> 
> The primary focus of our effort is on adding core architectural elements
> to the CPU emulation. For an overview of the current feature set, please
> see:
> 
>   https://qemu.readthedocs.io/en/master/system/arm/emulation.html
> 
> Besides the -cpu max, providing an approximation of a v9.0 baseline CPU,
> we have also recently added several specific CPU types like the
> Neoverse-N1 and V1 processor types as well as numerous Cortex CPU
> models.
> 
> Our most utilized machine model is "virt", which is primarily designed
> for guest operation and therefore has minimal resemblance to actual
> hardware. "sbsa-ref" was implemented to more closely simulate a real
> machine that aligns with Arm's SBSA specification.
> 
> In our work on VirtIO, we often use QEMU. Most of our rust-vmm
> vhost-device backends, for instance, were initially tested on QEMU.
> 
> Now that everyone is up-to-date, I would welcome any feedback from the
> Xen community on features that would increase QEMU's usefulness as a
> development target.
> 
> Do you have interest in any upcoming Arm CPU features? For example, we
> recently added FEAT_RME support for Arm's new confidential computing,
> but currently do not implement FEAT_NV/NV2.
> 
> How about the HW emulation in QEMU? Is the PCI emulation reliable enough
> to ensure confidence while testing changes to Xen's PCI management? What
> about the few peripherals that the hypervisor accesses directly?
> 
> Are there other development features you consider essential? Have you
> noticed any limitations with gdbstub? Does anyone use the record/replay
> or reverse debug functions? Has anyone tried TCG plugins for analysing
> the behavior of the hypervisor?
> 
> While I cannot promise to implement every wish-list item (performance
> counter emulation, for example, as we are not a uArch simulator), I am
> eager to gather feedback on how QEMU could be improved to help the Xen
> community deliver it's roadmap faster.
> 
> Thank you for your time and I look forward to any feedback :-)
> 
> -- 
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
> 

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

[Stefano Stabellini]

On Wed, 30 Aug 2023, Vikram Garhwal wrote:
> Hi Michal,
> On Tue, Aug 29, 2023 at 10:23:30AM +0200, Michal Orzel wrote:
> > 
> > 
> > On 25/08/2023 10:02, Vikram Garhwal wrote:
> > > Remove master device from the IOMMU. This will be helpful when removing 
> > > the
> > > overlay nodes using dynamic programming during run time.
> > > 
> > > Signed-off-by: Vikram Garhwal 
> > > Acked-by: Jan Beulich 
> > 
> > You don't seem to handle Julien remarks for this patch made in v9.
> > I will forward them here to avoid answering to old version, but for the 
> > future, do not carry the exact same patch
> > if you haven't yet addressed someone's remarks.
> This got skipped as I cannot find direct email from Julien. The only email 
> reply
> on this patch is can find is from: xen-devel-boun...@lists.xenproject.org and
> this got messed up with other larger set of email xen-devel sends.
> 
> Did you get direct email?
> > 
> > > 
> > > ---
> > > Changes from v7:
> > > Add check if IOMMU is enabled.
> > > Fix indentation of fail.
> > > ---
> > > ---
> > >  xen/drivers/passthrough/device_tree.c | 44 +++
> > >  xen/include/xen/iommu.h   |  1 +
> > >  2 files changed, 45 insertions(+)
> > > 
> > > diff --git a/xen/drivers/passthrough/device_tree.c 
> > > b/xen/drivers/passthrough/device_tree.c
> > > index 1202eac625..3fad65fb69 100644
> > > --- a/xen/drivers/passthrough/device_tree.c
> > > +++ b/xen/drivers/passthrough/device_tree.c
> > > @@ -128,6 +128,50 @@ int iommu_release_dt_devices(struct domain *d)
> > >  return 0;
> > >  }
> > >  
> > > +int iommu_remove_dt_device(struct dt_device_node *np)
> > > +{
> > > +const struct iommu_ops *ops = iommu_get_ops();
> > > +struct device *dev = dt_to_dev(np);
> > > +int rc;
> > > +
> > > +if ( !iommu_enabled )
> > > +return 1;
> > J:
> > The caller doesn't seem to check if the error code is > 0. So can we 
> > instead return a -ERRNO?
> Will change the check in caller. I want to keep this as it as so it looks
> similar to iommu_add_dt_device().

That is OK to me as long as the check on the return value is done


> > If you want to continue to return a value > 0 then I think it should be 
> > documented in a comment like we did for iommu_add_dt_device().
> >
> Will add comment before iommu_remove_dt_device().
> > > +
> > > +if ( !ops )
> > > +return -EOPNOTSUPP;
> > > +
> > > +spin_lock(&dtdevs_lock);
> > > +
> > > +if ( iommu_dt_device_is_assigned_locked(np) )
> > > +{
> > > +rc = -EBUSY;
> > > +goto fail;
> > > +}
> > > +
> > > +/*
> > > + * The driver which supports generic IOMMU DT bindings must have this
> > > + * callback implemented.
> > > + */
> > J:
> > I have questioned this message in v7 and I still question it. I guess 
> > you copied the comment on top of add_device(), this was add there 
> > because we have a different way to add legacy device.
> > 
> > But here there are no such requirement. In fact, you are not adding the 
> > the callback to all the IOMMU drivers... Yet all of them support the 
> > generic IOMMU DT bindings.
> Will change this.
> > 
> > > +if ( !ops->remove_device )
> > > +{
> > > +rc = -EOPNOTSUPP;
> > > +goto fail;
> > > +}
> > > +
> > > +/*
> > > + * Remove master device from the IOMMU if latter is present and 
> > > available.
> > J:
> > I read this as this will not return an error if the device is protected. 
> > However, AFAICT, the implement in the SMMU driver provided in this 
> > series will return an error. So I would suggest to replace this sentence 
> > with:
> > 
> > de-register the device from the IOMMU driver.
> Will change the comment.
> > 
> > > + * The driver is responsible for removing is_protected flag.
> > J:
> > Can you add an assert in the 'if ( !rc )' block to confirm that 
> > is_protected was effectively removed. Something like:
> > 
> > ASSERT(!dt_device_is_protected(dev));
> Is ASSERT really required here. remove callback can return before setting 
> is_protected as false.

But if ops->remove_device didn't actually set is_protected to false,
then it should return an error (rc != 0). What Julien is suggesting is
the following:

rc = ops->remove_device(0, dev);

if ( !rc )
{
ASSERT(!dt_device_is_protected(dev));
iommu_fwspec_free(dev);
}

Every time remove_device returns rc == 0 then is_protected should be
false, right?


> > 
> > This would help to confirm the driver is respecting what you expect.
> > 
> > > + */
> > > +rc = ops->remove_device(0, dev);
> > > +
> > > +if ( !rc )
> > > +iommu_fwspec_free(dev);
> > > +
> > > + fail:
> > > +spin_unlock(&dtdevs_lock);
> > > +return rc;
> > > +}
> > > +
> > >  int iommu_add_dt_device(struct dt_device_node *np)
> > >  {
> > >  const struct iommu_ops *ops = iommu_get_ops();
> > > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> > >

[ovmf test] 182573: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf c5753c3e38f3fde23eec9641cb3c433f443ff99e
baseline version:
 ovmf 0c4d0b6c8a05a6a62c0dca042f8e15e579e6f4af

Last test of basis   182568  2023-08-30 13:13:53 Z0 days
Testing same since   182573  2023-08-30 21:40:51 Z0 days1 attempts


People who touched revisions under test:
  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
   0c4d0b6c8a..c5753c3e38  c5753c3e38f3fde23eec9641cb3c433f443ff99e -> 
xen-tested-master

[linux-linus test] 182566: regressions - FAIL

[osstest service owner]

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

Regressions :-(

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

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

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 182531
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 182531
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 182531
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-x

[linux-5.4 test] 182569: regressions - trouble: fail/pass/starved

[osstest service owner]

flight 182569 linux-5.4 real [real]
flight 182572 linux-5.4 real-retest [real]
http://logs.test-lab.xenproject.org/osstest/logs/182569/
http://logs.test-lab.xenproject.org/osstest/logs/182572/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-armhf-armhf-xl-multivcpu 18 guest-start/debian.repeat fail REGR. vs. 
182363
 test-armhf-armhf-xl-credit1 18 guest-start/debian.repeat fail REGR. vs. 182363

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-xl-rtds   18 guest-start/debian.repeat fail blocked in 182363
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 182363
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 182363
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 182363
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 182363
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 182363
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 182363
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 182363
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 182363
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 182363
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 182363
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 182363
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 182363
 test-amd64-i386-libvirt-xsm  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-i386-xl-pvshim14 guest-start  fail   never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-raw  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  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-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-armhf-armhf-xl-vhd  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  15 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw  3 hosts-allocate   starved  n/a
 test-arm64-arm64-xl   3 hosts-allocate   starved  n/a
 test-arm64-arm64-libvirt-xsm  3 hosts-allocate   starved  n/a
 test-arm64-arm64-xl-xsm   3 hosts-allocate   starved  n/a
 test-arm64-arm64-xl-credit1   3 hosts-allocate   starved  n/a
 test-arm64-arm64-xl-credit2   3 hosts-allocate   starved  n/a
 test-arm64-arm64-xl-thunderx  3 hosts-allocate   starved  n/a
 test-arm64-arm64-xl-vhd   3 hosts-allocate   starved  n/a

version targeted for testing:
 linux5eb967dd50a5a29952ab6e6b1ef4bf216cf1652c
baseline version:
 linuxfd2a1d1f32ea37c57a8b46a0857f06fd7274dd2c

Last test of basis   182363  2023-08-16 16:45:54 Z   14 days
Testing same since   182569  2023-08-30 14:44:38 Z0 days1 attempts


People who touched revisions under test:
  Abel Wu 
  Adrian Hunter 
  Alan Liu 
  Aleksandr Loktionov 
  Alessio Igor Bogani 
  Alex Deucher 
  Alexander Aring 
  Alfred Lee 
  Amir Goldstein 
  Andreas Gruenbacher 
  Andrew Morton 
  Andrey Skvortsov 
  Andrii Staikov 
  Ankit Nautiyal 
  Armin Wolf 
  Arnd Bergmann 
  Arpana Arland  (A Contingent worker at Inte

Re: [PATCH v2 6/8] xen/ppc: Define bug frames table in linker script

[Shawn Anastasio]

On 8/30/23 8:03 AM, Jan Beulich wrote:
> On 23.08.2023 22:07, Shawn Anastasio wrote:
>> Define the bug frames table in ppc's linker script as is done by other
>> architectures.
>>
>> Signed-off-by: Shawn Anastasio 
>> Acked-by: Jan Beulich 
> 
> If I'm not mistaken this change is independent of the earlier patches,
> and hence could go in right away. Please confirm.

That's correct -- you're free to commit this independent of the rest of
the series if you'd like.

> Jan

Thanks,
Shawn

Re: [Xen-devel] [PATCH] x86/HVM: correct hvmemul_map_linear_addr() for multi-page case

[Andrew Cooper]

On 30/08/2023 3:30 pm, Roger Pau Monné wrote:
> On Wed, Sep 12, 2018 at 03:09:35AM -0600, Jan Beulich wrote:
>> The function does two translations in one go for a single guest access.
>> Any failure of the first translation step (guest linear -> guest
>> physical), resulting in #PF, ought to take precedence over any failure
>> of the second step (guest physical -> host physical).

Erm... No?

There are up to 25 translations steps, assuming a memory operand
contained entirely within a cache-line.

They intermix between gla->gpa and gpa->spa in a strict order.

There not a point where the error is ambiguous, nor is there ever a
point where a pagewalk continues beyond a faulting condition.

Hardware certainly isn't wasting transistors to hold state just to see
could try to progress further in order to hand back a different error...


When the pipeline needs to split an access, it has to generate multiple
adjacent memory accesses, because the unit of memory access is a cache line.

There is a total order of accesses in the memory queue, so any faults
from first byte of the access will be delivered before any fault from
the first byte to move into the next cache line.


I'm not necessarily saying that Xen's behaviour in
hvmemul_map_linear_addr() is correct in all cases, but it looks a hell
of a lot more correct in it's current form than what this patch presents.

Or do you have a concrete example where you think
hvmemul_map_linear_addr() behaves incorrectly?

~Andrew

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

[Vikram Garhwal]

Hi Michal,
On Tue, Aug 29, 2023 at 10:23:30AM +0200, Michal Orzel wrote:
> 
> 
> On 25/08/2023 10:02, Vikram Garhwal wrote:
> > Remove master device from the IOMMU. This will be helpful when removing the
> > overlay nodes using dynamic programming during run time.
> > 
> > Signed-off-by: Vikram Garhwal 
> > Acked-by: Jan Beulich 
> 
> You don't seem to handle Julien remarks for this patch made in v9.
> I will forward them here to avoid answering to old version, but for the 
> future, do not carry the exact same patch
> if you haven't yet addressed someone's remarks.
This got skipped as I cannot find direct email from Julien. The only email reply
on this patch is can find is from: xen-devel-boun...@lists.xenproject.org and
this got messed up with other larger set of email xen-devel sends.

Did you get direct email?
> 
> > 
> > ---
> > Changes from v7:
> > Add check if IOMMU is enabled.
> > Fix indentation of fail.
> > ---
> > ---
> >  xen/drivers/passthrough/device_tree.c | 44 +++
> >  xen/include/xen/iommu.h   |  1 +
> >  2 files changed, 45 insertions(+)
> > 
> > diff --git a/xen/drivers/passthrough/device_tree.c 
> > b/xen/drivers/passthrough/device_tree.c
> > index 1202eac625..3fad65fb69 100644
> > --- a/xen/drivers/passthrough/device_tree.c
> > +++ b/xen/drivers/passthrough/device_tree.c
> > @@ -128,6 +128,50 @@ int iommu_release_dt_devices(struct domain *d)
> >  return 0;
> >  }
> >  
> > +int iommu_remove_dt_device(struct dt_device_node *np)
> > +{
> > +const struct iommu_ops *ops = iommu_get_ops();
> > +struct device *dev = dt_to_dev(np);
> > +int rc;
> > +
> > +if ( !iommu_enabled )
> > +return 1;
> J:
> The caller doesn't seem to check if the error code is > 0. So can we 
> instead return a -ERRNO?
Will change the check in caller. I want to keep this as it as so it looks
similar to iommu_add_dt_device().
> 
> If you want to continue to return a value > 0 then I think it should be 
> documented in a comment like we did for iommu_add_dt_device().
>
Will add comment before iommu_remove_dt_device().
> > +
> > +if ( !ops )
> > +return -EOPNOTSUPP;
> > +
> > +spin_lock(&dtdevs_lock);
> > +
> > +if ( iommu_dt_device_is_assigned_locked(np) )
> > +{
> > +rc = -EBUSY;
> > +goto fail;
> > +}
> > +
> > +/*
> > + * The driver which supports generic IOMMU DT bindings must have this
> > + * callback implemented.
> > + */
> J:
> I have questioned this message in v7 and I still question it. I guess 
> you copied the comment on top of add_device(), this was add there 
> because we have a different way to add legacy device.
> 
> But here there are no such requirement. In fact, you are not adding the 
> the callback to all the IOMMU drivers... Yet all of them support the 
> generic IOMMU DT bindings.
Will change this.
> 
> > +if ( !ops->remove_device )
> > +{
> > +rc = -EOPNOTSUPP;
> > +goto fail;
> > +}
> > +
> > +/*
> > + * Remove master device from the IOMMU if latter is present and 
> > available.
> J:
> I read this as this will not return an error if the device is protected. 
> However, AFAICT, the implement in the SMMU driver provided in this 
> series will return an error. So I would suggest to replace this sentence 
> with:
> 
> de-register the device from the IOMMU driver.
Will change the comment.
> 
> > + * The driver is responsible for removing is_protected flag.
> J:
> Can you add an assert in the 'if ( !rc )' block to confirm that 
> is_protected was effectively removed. Something like:
> 
> ASSERT(!dt_device_is_protected(dev));
Is ASSERT really required here. remove callback can return before setting 
is_protected as false.
> 
> This would help to confirm the driver is respecting what you expect.
> 
> > + */
> > +rc = ops->remove_device(0, dev);
> > +
> > +if ( !rc )
> > +iommu_fwspec_free(dev);
> > +
> > + fail:
> > +spin_unlock(&dtdevs_lock);
> > +return rc;
> > +}
> > +
> >  int iommu_add_dt_device(struct dt_device_node *np)
> >  {
> >  const struct iommu_ops *ops = iommu_get_ops();
> > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> > index 110693c59f..a8e9bc9a2d 100644
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -233,6 +233,7 @@ int iommu_add_dt_device(struct dt_device_node *np);
> >  
> >  int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
> > XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
> > +int iommu_remove_dt_device(struct dt_device_node *np);
> >  
> >  #endif /* HAS_DEVICE_TREE */
> >  
> 
> ~Michal

Re: [XEN][PATCH v10 09/20] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller

[Vikram Garhwal]

Hi Michal,
On Tue, Aug 29, 2023 at 10:05:55AM +0200, Michal Orzel wrote:
> 
> 
> On 25/08/2023 10:02, Vikram Garhwal wrote:
> > Rename iommu_dt_device_is_assigned() to 
> > iommu_dt_device_is_assigned_locked().
> > 
> > Moving spin_lock to caller was done to prevent the concurrent access to
> > iommu_dt_device_is_assigned while doing add/remove/assign/deassign. 
> > Follow-up
> > patches in this series introduces node add/remove feature.
> > 
> > Signed-off-by: Vikram Garhwal 
> > 
> > ---
> > Changes from v9:
> > Make iommu_dt_device_is_assigned_locked() static and delete header.
> > Move dtdevs_lock before iommu_dt_device_is_assigned_locked().
> > Changes from v7:
> > Update commit message.
> > Add ASSERT().
> > ---
> > ---
> >  xen/drivers/passthrough/device_tree.c | 16 
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/drivers/passthrough/device_tree.c 
> > b/xen/drivers/passthrough/device_tree.c
> > index 1c32d7b50c..5d84c07b50 100644
> > --- a/xen/drivers/passthrough/device_tree.c
> > +++ b/xen/drivers/passthrough/device_tree.c
> > @@ -83,16 +83,17 @@ fail:
> >  return rc;
> >  }
> >  
> > -static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
> > +static bool_t
> > +iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev)
> This does not apply cleanly due to recent change from bool_t to bool. Please 
> rebase for v11 (the function
> should then fit in a single line I think).
Fixed the changes here and made it one-line.
> 
> >  {
> >  bool_t assigned = 0;
> >  
> > +ASSERT(spin_is_locked(&dtdevs_lock));
> > +
> >  if ( !dt_device_is_protected(dev) )
> >  return 0;
> >  
> > -spin_lock(&dtdevs_lock);
> >  assigned = !list_empty(&dev->domain_list);
> > -spin_unlock(&dtdevs_lock);
> >  
> >  return assigned;
> >  }
> > @@ -223,17 +224,24 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, 
> > struct domain *d,
> >  if ( ret )
> >  break;
> >  
> > +spin_lock(&dtdevs_lock);
> Why is this lock placed here instead of ...
> > +
> >  if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
> >  {
> > -if ( iommu_dt_device_is_assigned(dev) )
> > +
> ... here, right before iommu_dt_device_is_assigned_locked()?
Moved the lock before iommu_dt_device_is_assigned_locked().
> > +if ( iommu_dt_device_is_assigned_locked(dev) )
> >  {
> >  printk(XENLOG_G_ERR "%s already assigned.\n",
> > dt_node_full_name(dev));
> >  ret = -EINVAL;
> >  }
> > +
> > +spin_unlock(&dtdevs_lock);
> >  break;
> >  }
> >  
> > +spin_unlock(&dtdevs_lock);
> You could then remove this one.
Ok!
> 
> With the remarks addressed:
> Reviewed-by: Michal Orzel 
> 
> ~Michal

Re: [XEN][PATCH v10 05/20] xen/arm: Add CONFIG_OVERLAY_DTB

[Vikram Garhwal]

On Tue, Aug 29, 2023 at 09:23:55AM +0200, Michal Orzel wrote:
> 
> 
> On 25/08/2023 10:02, Vikram Garhwal wrote:
> > 
> > 
> > Introduce a config option where the user can enable support for 
> > adding/removing
> > device tree nodes using a device tree binary overlay.
> > 
> > Update SUPPORT.md and CHANGELOG.md to state the Device Tree Overlays 
> > support for
> > Arm.
> > 
> > Signed-off-by: Vikram Garhwal 
> > Acked-by: Henry Wang 
> > Reviewed-by: Michal Orzel 
> > 
> > ---
> > Changes from v9:
> > Fix style.
> > Changes from v7:
> > Add this feature as "experimental support" in CHANGELOG.md
> > ---
> > ---
> >  CHANGELOG.md | 3 ++-
> This patch does not apply cleanly on latest staging so please rebase for v11.
Rebased this for 11.
> 
> ~Michal

Re: [XEN][PATCH v10 03/20] xen/arm/device: Remove __init from function type

[Vikram Garhwal]

Hi Michal,
On Tue, Aug 29, 2023 at 09:17:37AM +0200, Michal Orzel wrote:
> 
> 
> On 25/08/2023 10:02, Vikram Garhwal wrote:
> > Remove __init from following function to access during runtime:
> > 1. map_irq_to_domain()
> > 2. handle_device_interrupts()
> > 3. map_range_to_domain()
> > 4. unflatten_dt_node()
> > 5. handle_device()
> > 6. map_device_children()
> > 7. map_dt_irq_to_domain()
> > Move map_irq_to_domain() prototype from domain_build.h to setup.h.
> > 
> > Above changes will create an error on build as non-init function are still
> > in domain_build.c file. So, to avoid build fails, following changes are 
> > done:
> > 1. Move map_irq_to_domain(), handle_device_interrupts(), 
> > map_range_to_domain(),
> > handle_device(), map_device_children() and map_dt_irq_to_domain()
> > to device.c. After removing __init type,  these functions are not 
> > specific
> > to domain building, so moving them out of domain_build.c to device.c.
> > 2. Remove static type from handle_device_interrupts().
> > 
> > Overall, these changes are done to support the dynamic programming of a 
> > nodes
> > where an overlay node will be added to fdt and unflattened node will be 
> > added to
> > dt_host. Furthermore, IRQ and mmio mapping will be done for the added node.
> > 
> > Signed-off-by: Vikram Garhwal 
> > 
> > ---
> > Changes from v9:
> > Move handle_device(), map_device_children() and map_dt_irq_to_domain() 
> > out
> > of domain_build.c
> > ---
> > ---
> >  xen/arch/arm/device.c   | 293 
> >  xen/arch/arm/domain_build.c | 293 
> >  xen/arch/arm/include/asm/domain_build.h |   2 -
> >  xen/arch/arm/include/asm/setup.h|   9 +
> >  xen/common/device_tree.c|  12 +-
> >  5 files changed, 308 insertions(+), 301 deletions(-)
> > 
> > diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> > index ca8539dee5..857f171a27 100644
> > --- a/xen/arch/arm/device.c
> > +++ b/xen/arch/arm/device.c
> > @@ -9,8 +9,10 @@
> >   */
> >  
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  
> >  extern const struct device_desc _sdevice[], _edevice[];
> > @@ -75,6 +77,297 @@ enum device_class device_get_class(const struct 
> > dt_device_node *dev)
> >  return DEVICE_UNKNOWN;
> >  }
> >  
> > +int map_irq_to_domain(struct domain *d, unsigned int irq,
> > +  bool need_mapping, const char *devname)
> > +{
> > +int res;
> > +
> > +res = irq_permit_access(d, irq);
> > +if ( res )
> > +{
> > +printk(XENLOG_ERR "Unable to permit to %pd access to IRQ %u\n", d, 
> > irq);
> > +return res;
> > +}
> > +
> > +if ( need_mapping )
> > +{
> > +/*
> > + * Checking the return of vgic_reserve_virq is not
> > + * necessary. It should not fail except when we try to map
> > + * the IRQ twice. This can legitimately happen if the IRQ is shared
> > + */
> > +vgic_reserve_virq(d, irq);
> > +
> > +res = route_irq_to_guest(d, irq, irq, devname);
> > +if ( res < 0 )
> > +{
> > +printk(XENLOG_ERR "Unable to map IRQ%u to %pd\n", irq, d);
> > +return res;
> > +}
> > +}
> > +
> > +dt_dprintk("  - IRQ: %u\n", irq);
> > +return 0;
> > +}
> > +
> > +int map_range_to_domain(const struct dt_device_node *dev,
> > +uint64_t addr, uint64_t len, void *data)
> > +{
> > +struct map_range_data *mr_data = data;
> > +struct domain *d = mr_data->d;
> > +int res;
> > +
> > +if ( (addr != (paddr_t)addr) || (((paddr_t)~0 - addr) < len) )
> > +{
> > +printk(XENLOG_ERR "%s: [0x%"PRIx64", 0x%"PRIx64"] exceeds the 
> > maximum allowed PA width (%u bits)",
> > +   dt_node_full_name(dev), addr, (addr + len), PADDR_BITS);
> > +return -ERANGE;
> > +}
> > +
> > +/*
> > + * reserved-memory regions are RAM carved out for a special purpose.
> > + * They are not MMIO and therefore a domain should not be able to
> > + * manage them via the IOMEM interface.
> > + */
> > +if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
> > + strlen("/reserved-memory/")) != 0 )
> > +{
> > +res = iomem_permit_access(d, paddr_to_pfn(addr),
> > +paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> > +if ( res )
> > +{
> > +printk(XENLOG_ERR "Unable to permit to dom%d access to"
> > +" 0x%"PRIx64" - 0x%"PRIx64"\n",
> > +d->domain_id,
> > +addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> > +return res;
> > +}
> > +}
> > +
> > +if ( !mr_data->skip_mapping )
> > +{
> > +res = map_regions_p2mt(d,
> > +   gaddr_to_gfn(addr),
> > +

Re: [PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Andrew Cooper]

On 30/08/2023 5:13 pm, Jan Beulich wrote:
> On 30.08.2023 17:28, Andrew Cooper wrote:
>> On 30/08/2023 4:12 pm, Jan Beulich wrote:
>>> On 30.08.2023 16:35, Andrew Cooper wrote:
 On 29/08/2023 3:08 pm, Jan Beulich wrote:
> On 29.08.2023 15:43, Andrew Cooper wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1074,8 +1074,27 @@ int arch_set_info_guest(
>>  #endif
>>  flags = c(flags);
>>  
>> +if ( !compat )
>> +{
>> +if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
>> + c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
>> +return -EINVAL;
>> +}
>> +
>>  if ( is_pv_domain(d) )
>>  {
>> +/*
>> + * Prior to Xen 4.11, dr5 was used to hold the emulated-only
>> + * subset of dr7, and dr4 was unused.
>> + *
>> + * In Xen 4.11 and later, dr4/5 are written as zero, ignored for
>> + * backwards compatibility, and dr7 emulation is handled
>> + * internally.
>> + */
>> +for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
>> +if ( !access_ok(c(debugreg[i]), sizeof(long)) )
> Don't you mean __addr_ok() here, i.e. not including the
> is_compat_arg_xlat_range() check? (Else I would have asked why
> sizeof(long), but that question resolves itself with using the other
> macro.)
 For now, I'm simply moving a check from set_debugreg() earlier in
 arch_set_info_guest().

 I think it would be beneficial to keep that change independent.
>>> Hmm, difficult. I'd be okay if you indeed moved the other check. But
>>> you duplicate it here, and duplicating questionable code is, well,
>>> questionable.
>> It can't be removed in set_debugreg() because that's used in other paths
>> too.
> Sure, I understand that.
>
>> And the error from set_debugreg() can't fail arch_set_info_guest()
>> because that introduces a failure after mutation of the vCPU state.
>>
>> This isn't a fastpath.  It's used approximately once per vCPU lifetime.
> But fast or not isn't the point here.

No.  The point is no change from the existing code.

If you think it's wrong, it in a separate change and don't block this fix.

~Andrew

[PATCH v3 0/2] introduce stub directory to storing empty/stub headers

[Oleksii Kurochko]

A lot of empty/stub headers should be introduced during the early steps of 
adding
support of new architecture.

An example can be found here:
1. 
https://lore.kernel.org/xen-devel/cover.1692181079.git.oleksii.kuroc...@gmail.com/
2. 
https://lore.kernel.org/xen-devel/a92f99e8f697da99d77bfde562a549dbef3760ce.1692816595.git.sanasta...@raptorengineering.com/

As part of the patch series, asm/vm_event.h was moved to the stubs directory 
because
It is the same for ARM, PPC, and RISC-V.

---
Changes in V3:
 - add Acked-by: Stefano Stabellini  for "xen: move 
arm/include/asm/vm_event.h to asm-generic"
 - update SPDX tag.
 - move asm/vm_event.h to asm-generic.
 - rename stubs dir to asm-generic.

---
Changes in V2:
 - change public/domctl.h to public/vm_event.h.
 - update commit message of [PATCH v2 2/2] xen: move arm/include/asm/vm_event.h 
to stubs

Oleksii Kurochko (2):
  xen: add asm-generic dir to include path
  xen: move arm/include/asm/vm_event.h to stubs

 xen/Makefile   |  1 +
 xen/arch/arm/include/asm/vm_event.h| 66 --
 xen/include/asm-generic/asm/vm_event.h | 55 +
 3 files changed, 56 insertions(+), 66 deletions(-)
 delete mode 100644 xen/arch/arm/include/asm/vm_event.h
 create mode 100644 xen/include/asm-generic/asm/vm_event.h

-- 
2.41.0

[PATCH v3 1/2] xen: add asm-generic dir to include path

[Oleksii Kurochko]

asm-generic dir will contain empty/stubs generic for all architectures.

Signed-off-by: Oleksii Kurochko 
---
Changes in V3:
 - Rename stubs dir to asm-generic
---
Changes in V2:
 - Nothing changed.
---
 xen/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/Makefile b/xen/Makefile
index f57e5a596c..5482c86080 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -438,6 +438,7 @@ ifdef building_out_of_srctree
 endif
 CFLAGS += -I$(srctree)/include
 CFLAGS += -I$(srctree)/arch/$(SRCARCH)/include
+CFLAGS += -I$(srctree)/include/asm-generic
 
 # Note that link order matters!
 ALL_OBJS-y:= common/built_in.o
-- 
2.41.0

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

[Oleksii Kurochko]

asm/vm_event.h is common for ARM and RISC-V so it will be moved to
asm-generic dir.

Original asm/vm_event.h from ARM was updated:
 * use SPDX-License-Identifier.
 * update comment messages of stubs.
 * update #ifdef.
 * change public/domctl.h to public/vm_event.h.

Signed-off-by: Oleksii Kurochko 
Acked-by: Stefano Stabellini 
---
Changes in V3:
 - add Acked-by: Stefano Stabellini  for "xen: move 
arm/include/asm/vm_event.h to asm-generic"
 - update SPDX tag.
 - move asm/vm_event.h to asm-generic.
---
Changes in V2:
 - change public/domctl.h to public/vm_event.h.
 - update commit message of [PATCH v2 2/2] xen: move arm/include/asm/vm_event.h 
to stubs
---
 xen/arch/arm/include/asm/vm_event.h| 66 --
 xen/include/asm-generic/asm/vm_event.h | 55 +
 2 files changed, 55 insertions(+), 66 deletions(-)
 delete mode 100644 xen/arch/arm/include/asm/vm_event.h
 create mode 100644 xen/include/asm-generic/asm/vm_event.h

diff --git a/xen/arch/arm/include/asm/vm_event.h 
b/xen/arch/arm/include/asm/vm_event.h
deleted file mode 100644
index 4d861373b3..00
--- a/xen/arch/arm/include/asm/vm_event.h
+++ /dev/null
@@ -1,66 +0,0 @@
-/*
- * vm_event.h: architecture specific vm_event handling routines
- *
- * Copyright (c) 2015 Tamas K Lengyel (ta...@tklengyel.com)
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program; If not, see .
- */
-
-#ifndef __ASM_ARM_VM_EVENT_H__
-#define __ASM_ARM_VM_EVENT_H__
-
-#include 
-#include 
-
-static inline int vm_event_init_domain(struct domain *d)
-{
-/* Nothing to do. */
-return 0;
-}
-
-static inline void vm_event_cleanup_domain(struct domain *d)
-{
-memset(&d->monitor, 0, sizeof(d->monitor));
-}
-
-static inline void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v,
-  vm_event_response_t *rsp)
-{
-/* Not supported on ARM. */
-}
-
-static inline
-void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
-{
-/* Not supported on ARM. */
-}
-
-static inline
-void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
-{
-/* Not supported on ARM. */
-}
-
-static inline
-void vm_event_sync_event(struct vcpu *v, bool value)
-{
-/* Not supported on ARM. */
-}
-
-static inline
-void vm_event_reset_vmtrace(struct vcpu *v)
-{
-/* Not supported on ARM. */
-}
-
-#endif /* __ASM_ARM_VM_EVENT_H__ */
diff --git a/xen/include/asm-generic/asm/vm_event.h 
b/xen/include/asm-generic/asm/vm_event.h
new file mode 100644
index 00..29ab1b01b4
--- /dev/null
+++ b/xen/include/asm-generic/asm/vm_event.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier:  GPL-2.0-only */
+/*
+ * vm_event.h: stubs for architecture specific vm_event handling routines
+ *
+ * Copyright (c) 2015 Tamas K Lengyel (ta...@tklengyel.com)
+ */
+
+#ifndef __ASM_STUB_VM_EVENT_H__
+#define __ASM_STUB_VM_EVENT_H__
+
+#include 
+#include 
+
+static inline int vm_event_init_domain(struct domain *d)
+{
+/* Nothing to do. */
+return 0;
+}
+
+static inline void vm_event_cleanup_domain(struct domain *d)
+{
+memset(&d->monitor, 0, sizeof(d->monitor));
+}
+
+static inline void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v,
+  vm_event_response_t *rsp)
+{
+/* Nothing to do. */
+}
+
+static inline
+void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
+{
+/* Nothing to do. */
+}
+
+static inline
+void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
+{
+/* Nothing to do. */
+}
+
+static inline
+void vm_event_sync_event(struct vcpu *v, bool value)
+{
+/* Nothing to do. */
+}
+
+static inline
+void vm_event_reset_vmtrace(struct vcpu *v)
+{
+/* Nothing to do. */
+}
+
+#endif /* __ASM_STUB_VM_EVENT_H__ */
-- 
2.41.0

QEMU features useful for Xen development?

[Alex Bennée]

Dear Xen community,

Linaro is significantly invested in QEMU development, with a special
focus on Arm-related aspects. We recognize the value of QEMU as a
readily available software reference platform for projects that need to
test their software well before the availability of real hardware.

The primary focus of our effort is on adding core architectural elements
to the CPU emulation. For an overview of the current feature set, please
see:

  https://qemu.readthedocs.io/en/master/system/arm/emulation.html

Besides the -cpu max, providing an approximation of a v9.0 baseline CPU,
we have also recently added several specific CPU types like the
Neoverse-N1 and V1 processor types as well as numerous Cortex CPU
models.

Our most utilized machine model is "virt", which is primarily designed
for guest operation and therefore has minimal resemblance to actual
hardware. "sbsa-ref" was implemented to more closely simulate a real
machine that aligns with Arm's SBSA specification.

In our work on VirtIO, we often use QEMU. Most of our rust-vmm
vhost-device backends, for instance, were initially tested on QEMU.

Now that everyone is up-to-date, I would welcome any feedback from the
Xen community on features that would increase QEMU's usefulness as a
development target.

Do you have interest in any upcoming Arm CPU features? For example, we
recently added FEAT_RME support for Arm's new confidential computing,
but currently do not implement FEAT_NV/NV2.

How about the HW emulation in QEMU? Is the PCI emulation reliable enough
to ensure confidence while testing changes to Xen's PCI management? What
about the few peripherals that the hypervisor accesses directly?

Are there other development features you consider essential? Have you
noticed any limitations with gdbstub? Does anyone use the record/replay
or reverse debug functions? Has anyone tried TCG plugins for analysing
the behavior of the hypervisor?

While I cannot promise to implement every wish-list item (performance
counter emulation, for example, as we are not a uArch simulator), I am
eager to gather feedback on how QEMU could be improved to help the Xen
community deliver it's roadmap faster.

Thank you for your time and I look forward to any feedback :-)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[xen-unstable test] 182563: tolerable FAIL

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 xen  8c01f267eff3d6c1ea04273e9885bf6d2fda1c44
baseline version:
 xen  8c01f267eff3d6c1ea04273e9885bf6d2fda1c44

Last test of basis   182563  2023-08-30 05:57:17 Z0 days
Testing same since  (not found) 0 attempts

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i3

Re: [PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Jan Beulich]

On 30.08.2023 17:28, Andrew Cooper wrote:
> On 30/08/2023 4:12 pm, Jan Beulich wrote:
>> On 30.08.2023 16:35, Andrew Cooper wrote:
>>> On 29/08/2023 3:08 pm, Jan Beulich wrote:
 On 29.08.2023 15:43, Andrew Cooper wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1074,8 +1074,27 @@ int arch_set_info_guest(
>  #endif
>  flags = c(flags);
>  
> +if ( !compat )
> +{
> +if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
> + c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
> +return -EINVAL;
> +}
> +
>  if ( is_pv_domain(d) )
>  {
> +/*
> + * Prior to Xen 4.11, dr5 was used to hold the emulated-only
> + * subset of dr7, and dr4 was unused.
> + *
> + * In Xen 4.11 and later, dr4/5 are written as zero, ignored for
> + * backwards compatibility, and dr7 emulation is handled
> + * internally.
> + */
> +for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
> +if ( !access_ok(c(debugreg[i]), sizeof(long)) )
 Don't you mean __addr_ok() here, i.e. not including the
 is_compat_arg_xlat_range() check? (Else I would have asked why
 sizeof(long), but that question resolves itself with using the other
 macro.)
>>> For now, I'm simply moving a check from set_debugreg() earlier in
>>> arch_set_info_guest().
>>>
>>> I think it would be beneficial to keep that change independent.
>> Hmm, difficult. I'd be okay if you indeed moved the other check. But
>> you duplicate it here, and duplicating questionable code is, well,
>> questionable.
> 
> It can't be removed in set_debugreg() because that's used in other paths
> too.

Sure, I understand that.

> And the error from set_debugreg() can't fail arch_set_info_guest()
> because that introduces a failure after mutation of the vCPU state.
> 
> This isn't a fastpath.  It's used approximately once per vCPU lifetime.

But fast or not isn't the point here. The point is that both the use
of access_ok() and the use of sizeof(long) are bogus in this context.
Switching to __addr_ok() will tighten the check here (and hence not
risk set_debugreg() later raising an error).

Jan

[PATCH v8 4/4] x86/microcode: Disable microcode update handler if DIS_MCU_UPDATE is set

[Alejandro Vallejo]

If IA32_MSR_MCU_CONTROL exists then it's possible a CPU may be unable to
perform microcode updates. This is controlled through the DIS_MCU_LOAD bit
and is intended for baremetal clouds where the owner may not trust the
tenant to choose the microcode version in use. If we notice that bit being
set then simply disable the "apply_microcode" handler so we can't even try
to perform update (as it's known to be silently dropped).

While at it, remove the Intel family check, as microcode loading is
supported on every Intel64 CPU.

Signed-off-by: Alejandro Vallejo 
Reviewed-by: Jan Beulich 
---
v8:
 * No change
---
 xen/arch/x86/cpu/microcode/core.c | 20 ++--
 xen/arch/x86/cpu/microcode/intel.c| 13 +
 xen/arch/x86/cpu/microcode/private.h  |  7 +++
 xen/arch/x86/include/asm/cpufeature.h |  1 +
 xen/arch/x86/include/asm/msr-index.h  |  5 +
 5 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c 
b/xen/arch/x86/cpu/microcode/core.c
index b3df4d40e6..65ebeb50de 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -847,17 +847,21 @@ int __init early_microcode_init(unsigned long *module_map,
 {
 const struct cpuinfo_x86 *c = &boot_cpu_data;
 int rc = 0;
+bool can_load = false;
 
 switch ( c->x86_vendor )
 {
 case X86_VENDOR_AMD:
 if ( c->x86 >= 0x10 )
+{
 ucode_ops = amd_ucode_ops;
+can_load = true;
+}
 break;
 
 case X86_VENDOR_INTEL:
-if ( c->x86 >= 6 )
-ucode_ops = intel_ucode_ops;
+ucode_ops = intel_ucode_ops;
+can_load = intel_can_load_microcode();
 break;
 }
 
@@ -871,13 +875,17 @@ int __init early_microcode_init(unsigned long *module_map,
 
 /*
  * Some hypervisors deliberately report a microcode revision of -1 to
- * mean that they will not accept microcode updates. We take the hint
- * and ignore the microcode interface in that case.
+ * mean that they will not accept microcode updates.
+ *
+ * It's also possible the hardware might have built-in support to disable
+ * updates and someone (e.g: a baremetal cloud provider) disabled them.
+ *
+ * Take the hint in either case and ignore the microcode interface.
  */
-if ( this_cpu(cpu_sig).rev == ~0 )
+if ( this_cpu(cpu_sig).rev == ~0 || !can_load )
 {
 printk(XENLOG_INFO "Microcode loading disabled due to: %s\n",
-   "rev = ~0");
+   can_load ? "rev = ~0" : "HW toggle");
 ucode_ops.apply_microcode = NULL;
 return -ENODEV;
 }
diff --git a/xen/arch/x86/cpu/microcode/intel.c 
b/xen/arch/x86/cpu/microcode/intel.c
index 8d4d6574aa..060c529a6e 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -385,6 +385,19 @@ static struct microcode_patch *cf_check 
cpu_request_microcode(
 return patch;
 }
 
+bool __init intel_can_load_microcode(void)
+{
+uint64_t mcu_ctrl;
+
+if ( !cpu_has_mcu_ctrl )
+return true;
+
+rdmsrl(MSR_MCU_CONTROL, mcu_ctrl);
+
+/* If DIS_MCU_LOAD is set applying microcode updates won't work */
+return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);
+}
+
 const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
 .cpu_request_microcode= cpu_request_microcode,
 .collect_cpu_info = collect_cpu_info,
diff --git a/xen/arch/x86/cpu/microcode/private.h 
b/xen/arch/x86/cpu/microcode/private.h
index 626aeb4d08..d80787205a 100644
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -60,6 +60,13 @@ struct microcode_ops {
 const struct microcode_patch *new, const struct microcode_patch *old);
 };
 
+/**
+ * Checks whether we can perform microcode updates on this Intel system
+ *
+ * @return True iff the microcode update facilities are enabled
+ */
+bool intel_can_load_microcode(void);
+
 extern const struct microcode_ops amd_ucode_ops, intel_ucode_ops;
 
 #endif /* ASM_X86_MICROCODE_PRIVATE_H */
diff --git a/xen/arch/x86/include/asm/cpufeature.h 
b/xen/arch/x86/include/asm/cpufeature.h
index 0825343945..213c184b1c 100644
--- a/xen/arch/x86/include/asm/cpufeature.h
+++ b/xen/arch/x86/include/asm/cpufeature.h
@@ -201,6 +201,7 @@ static inline bool boot_cpu_has(unsigned int feat)
 #define cpu_has_if_pschange_mc_no boot_cpu_has(X86_FEATURE_IF_PSCHANGE_MC_NO)
 #define cpu_has_tsx_ctrlboot_cpu_has(X86_FEATURE_TSX_CTRL)
 #define cpu_has_taa_no  boot_cpu_has(X86_FEATURE_TAA_NO)
+#define cpu_has_mcu_ctrlboot_cpu_has(X86_FEATURE_MCU_CTRL)
 #define cpu_has_fb_clearboot_cpu_has(X86_FEATURE_FB_CLEAR)
 #define cpu_has_rrsba   boot_cpu_has(X86_FEATURE_RRSBA)
 #define cpu_has_gds_ctrlboot_cpu_has(X86_FEATURE_GDS_CTRL)
diff --git a/xen/arch/x86/include/asm/msr-index.h 
b/xen/arch/x86/include/asm/msr-index.h
index 11ffed543a..586

[PATCH v8 1/4] x86/microcode: WARN->INFO for the "no ucode loading" log message

[Alejandro Vallejo]

Currently there's a printk statement triggered when no ucode loading
facilities are discovered. This statement should have severity INFO rather
than WARNING because it's not reporting anything wrong. Warnings ought
to be reserved for recoverable system errors.

Signed-off-by: Alejandro Vallejo 

--
v8:
  * New patch to unify the severity of this printk statement with the
statement introduced in patch 2
---
 xen/arch/x86/cpu/microcode/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c 
b/xen/arch/x86/cpu/microcode/core.c
index 9fcb9c1c3a..e5e03cad34 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -863,7 +863,7 @@ int __init early_microcode_init(unsigned long *module_map,
 
 if ( !ucode_ops.apply_microcode )
 {
-printk(XENLOG_WARNING "Microcode loading not available\n");
+printk(XENLOG_INFO "Microcode loading not available\n");
 return -ENODEV;
 }
 
-- 
2.34.1

[PATCH v8 3/4] x86: Read MSR_ARCH_CAPS immediately after early_microcode_init()

[Alejandro Vallejo]

Move MSR_ARCH_CAPS read code from tsx_init() to early_cpu_init(). Because
microcode updates might make them that MSR to appear/have different values
we also must reload it after a microcode update in early_microcode_init().

Signed-off-by: Alejandro Vallejo 
Reviewed-by: Jan Beulich 
---
v8:
 * No change
---
 xen/arch/x86/cpu/common.c | 20 +++-
 xen/arch/x86/cpu/microcode/core.c |  9 +
 xen/arch/x86/include/asm/setup.h  |  2 +-
 xen/arch/x86/setup.c  |  2 +-
 xen/arch/x86/tsx.c| 16 
 5 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 6fada384a1..3fd4fd0654 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -299,7 +299,7 @@ static inline u32 phys_pkg_id(u32 cpuid_apic, int index_msb)
 
WARNING: this function is only called on the BP.  Don't add code here
that is supposed to run on all CPUs. */
-void __init early_cpu_init(void)
+void __init early_cpu_init(bool verbose)
 {
struct cpuinfo_x86 *c = &boot_cpu_data;
u32 eax, ebx, ecx, edx;
@@ -320,6 +320,8 @@ void __init early_cpu_init(void)
case X86_VENDOR_SHANGHAI: this_cpu = &shanghai_cpu_dev; break;
case X86_VENDOR_HYGON:this_cpu = &hygon_cpu_dev;break;
default:
+   if (!verbose)
+   break;
printk(XENLOG_ERR
   "Unrecognised or unsupported CPU vendor '%.12s'\n",
   c->x86_vendor_id);
@@ -336,10 +338,13 @@ void __init early_cpu_init(void)
c->x86_capability[FEATURESET_1d] = edx;
c->x86_capability[FEATURESET_1c] = ecx;
 
-   printk(XENLOG_INFO
-  "CPU Vendor: %s, Family %u (%#x), Model %u (%#x), Stepping %u 
(raw %08x)\n",
-  x86_cpuid_vendor_to_str(c->x86_vendor), c->x86, c->x86,
-  c->x86_model, c->x86_model, c->x86_mask, eax);
+   if (verbose)
+   printk(XENLOG_INFO
+  "CPU Vendor: %s, Family %u (%#x), "
+  "Model %u (%#x), Stepping %u (raw %08x)\n",
+  x86_cpuid_vendor_to_str(c->x86_vendor), c->x86,
+  c->x86, c->x86_model, c->x86_model, c->x86_mask,
+  eax);
 
if (c->cpuid_level >= 7) {
uint32_t max_subleaf;
@@ -348,6 +353,11 @@ void __init early_cpu_init(void)
&c->x86_capability[FEATURESET_7c0],
&c->x86_capability[FEATURESET_7d0]);
 
+   if (test_bit(X86_FEATURE_ARCH_CAPS, c->x86_capability))
+   rdmsr(MSR_ARCH_CAPABILITIES,
+ c->x86_capability[FEATURESET_m10Al],
+ c->x86_capability[FEATURESET_m10Ah]);
+
if (max_subleaf >= 1)
cpuid_count(7, 1, &eax, &ebx, &ecx,
&c->x86_capability[FEATURESET_7d1]);
diff --git a/xen/arch/x86/cpu/microcode/core.c 
b/xen/arch/x86/cpu/microcode/core.c
index 01f1dd4710..b3df4d40e6 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -887,5 +887,14 @@ int __init early_microcode_init(unsigned long *module_map,
 if ( ucode_mod.mod_end || ucode_blob.size )
 rc = early_microcode_update_cpu();
 
+/*
+ * Some CPUID leaves and MSRs are only present after microcode updates
+ * on some processors. We take the chance here to make sure what little
+ * state we have already probed is re-probed in order to ensure we do
+ * not use stale values. tsx_init() in particular needs to have up to
+ * date MSR_ARCH_CAPS.
+ */
+early_cpu_init(false);
+
 return rc;
 }
diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index b0e6a39e23..8350167650 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -15,7 +15,7 @@ extern uint64_t boot_tsc_stamp;
 
 extern void *stack_start;
 
-void early_cpu_init(void);
+void early_cpu_init(bool verbose);
 void early_time_init(void);
 
 void set_nr_cpu_ids(unsigned int max_cpus);
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 3358d9a0ff..3641d5fbac 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1213,7 +1213,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 panic("Bootloader provided no memory information\n");
 
 /* This must come before e820 code because it sets paddr_bits. */
-early_cpu_init();
+early_cpu_init(true);
 
 /* Choose shadow stack early, to set infrastructure up appropriately. */
 if ( !boot_cpu_has(X86_FEATURE_CET_SS) )
diff --git a/xen/arch/x86/tsx.c b/xen/arch/x86/tsx.c
index 80c6f4cedd..50d8059f23 100644
--- a/xen/arch/x86/tsx.c
+++ b/xen/arch/x86/tsx.c
@@ -39,9 +39,10 @@ void tsx_init(void)
 static bool __read_mostly once;
 
 /*
- * This function is first

[PATCH v8 0/4] Prevent attempting updates known to fail

[Alejandro Vallejo]

Now that XENLOG_INFO is the default logging level...

v8:
  * Fixed last bits mentioned by Jan in v7/patch1 (v8/patch2)
  * Rolled back to having new printk as INFO
  * Added v8/patch1 to ensure the existing early exit from
early_cpu_init() has INFO severity as well.

Original cover letter:

Under certain conditions a CPU may not be able to perform microcode updates
even if hardware exists to that effect. In particular:

 * If Xen runs under certain hypervisors they won't allow microcode
   updates, and will signal this fact by reporting a microcode revision of
   -1.
 * If the DIS_MCU_LOAD bit is set, which is expected in some baremetal
   clouds where the owner may not trust the tenant, then the CPU is not
   capable of loading new microcode.

This series adds logic so that in both of these cases we don't needlessly
attempt updates that are not going to succeed. Patch summary:

Patch 1 Modifies the severity of the printk statement in
early_microcode_init() to be INFO

Patch 2 Ignores microcode facilities when the current microcode revision is -1
(was v7/patch1)

Patch 3 Moves the MSR_ARCH_CAPS read in tsx_init() to early_cpu_init() and
early_microcode_init()
(was v7/patch2)

Patch 4 Adds the logic to detect microcode updates being disabled on Intel.
(was v7/patch3)

Alejandro Vallejo (4):
  x86/microcode: WARN->INFO for the "no ucode loading" log message
  x86/microcode: Ignore microcode loading interface for revision = -1
  x86: Read MSR_ARCH_CAPS immediately after early_microcode_init()
  x86/microcode: Disable microcode update handler if DIS_MCU_UPDATE is
set

 xen/arch/x86/cpu/common.c | 20 ++
 xen/arch/x86/cpu/microcode/core.c | 40 +++
 xen/arch/x86/cpu/microcode/intel.c| 13 +
 xen/arch/x86/cpu/microcode/private.h  |  7 +
 xen/arch/x86/include/asm/cpufeature.h |  1 +
 xen/arch/x86/include/asm/msr-index.h  |  5 
 xen/arch/x86/include/asm/setup.h  |  2 +-
 xen/arch/x86/setup.c  |  2 +-
 xen/arch/x86/tsx.c| 16 +++
 9 files changed, 82 insertions(+), 24 deletions(-)

-- 
2.34.1

[PATCH v8 2/4] x86/microcode: Ignore microcode loading interface for revision = -1

[Alejandro Vallejo]

Some hypervisors report ~0 as the microcode revision to mean "don't issue
microcode updates". Ignore the microcode loading interface in that case.

Signed-off-by: Alejandro Vallejo 
Reviewed-by: Jan Beulich 
---
v8:
  * Added missing newline in printk statement
  * Reduced indentation of second line of printk statement
  * Turned printk statement to INFO (because that's now the default)
---
 xen/arch/x86/cpu/microcode/core.c | 17 +++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c 
b/xen/arch/x86/cpu/microcode/core.c
index e5e03cad34..01f1dd4710 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -867,10 +867,23 @@ int __init early_microcode_init(unsigned long *module_map,
 return -ENODEV;
 }
 
-microcode_grab_module(module_map, mbi);
-
 ucode_ops.collect_cpu_info();
 
+/*
+ * Some hypervisors deliberately report a microcode revision of -1 to
+ * mean that they will not accept microcode updates. We take the hint
+ * and ignore the microcode interface in that case.
+ */
+if ( this_cpu(cpu_sig).rev == ~0 )
+{
+printk(XENLOG_INFO "Microcode loading disabled due to: %s\n",
+   "rev = ~0");
+ucode_ops.apply_microcode = NULL;
+return -ENODEV;
+}
+
+microcode_grab_module(module_map, mbi);
+
 if ( ucode_mod.mod_end || ucode_blob.size )
 rc = early_microcode_update_cpu();
 
-- 
2.34.1

Re: [XEN PATCH] x86/ACPI: Ignore entries with invalid APIC IDs when parsing MADT

[Thomas Gleixner]

Jan!

On Wed, Aug 30 2023 at 09:20, Jan Beulich wrote:
> On 30.08.2023 00:54, Thomas Gleixner wrote:
>> On Tue, Aug 29 2023 at 16:25, Roger Pau Monné wrote:
>> 
>> Correct. These IDs are invalid independent of any flag value. 
>
> What we apparently agree on is these special UID values to be invalid,
> and that UIDs can't change. But that's not the same for the APIC IDs;
> see below. (As a side note, Xen range-checks UID against its
> implementation limit MAX_MADT_ENTRIES, so it's more than just the
> all-ones values which we'd reject. Not really correct, I know. Looks
> like Linux has done away with the simple x86_acpiid_to_apicid[]
> translation mechanism. This being a statically sized array requires
> this restriction in Xen, for the time being.)

Linux ignores entries too once the the maximum number of CPUs is reached.

>>> I think Jan's point (if I understood correctly) is that Processor or
>>> Device objects can have a _MAT method that returns updated MADT
>>> entries, and some ACPI implementations might modify the original
>>> entries on the MADT and return them from that method when CPU
>>> hotplug takes place.
>
> Just to mention it: It's not just "might". I've seen DSDT code doing
> so on more than one occasion.

That does not make it more correct or better.

> As stated before, unless putting in place extra restrictions, I can't
> even see how firmware would be able to up front determine APIC IDs for
> unpopulated sockets: It simply can't know the topology of a package
> that's not there yet. Requiring all packages to have identical
> topology might be a restriction OSes put in place, but I'd be inclined
> to call firmware buggy if it did (short of me being aware of there
> being anything in the spec putting in place such a restriction).

The ACPI specification does not care about restrictions which are in the
realm of hardware. You simply cannot mix random CPUs in a system just as
you see fit.

But that aside. ACPI based hotplug is purely used by virtualization. The
efforts to support real physical hotplug have never advanced beyond the
proof of concept state as the required complexity turned out to be just
not worth the potential benefit.

Of course virtualization people might think that everything which is
imaginable and not explicitly forbidden by some specification is
something which should be supported. That's just a recipe for disaster
as it needlessly expands the complexity space for absolutely zero value.

In reality most OSes will require that all possible APIC IDs are
enumerated during initialization in order to size things correctly and
to make system wide decisions correctly during boot or they will either
fail to accept hot-added CPUs later on or end up in a situation where
after accepting a hot-added CPU it turns out that system wide boot time
decisions are wrong or data is sized incorrectly, which means it is
pretty much up a creek without a paddle.

So in order to avoid these hard to handle, hard to debug and diagnose
failure cases, it's sensible when OSes mandate enumeration requirements
which have been omitted from the specification for whatever reason.

Linux expects this today and in case the expectation is not met it has
issues due to the non-enforcement, which cause hard to diagnose
malfunction. Those issues might be fixable by some definition of
fixable, but the value of doing that is close to zero. In fact it'd be a
net negative because the increased complexity will just put a
maintainability burden on the code base which is completely
unjustifiable.

Coming back to the specification issues. As of v6.3 the Online Capable
flag was added with the following rationale (paraphrased):

 Operating systems need to size resources at boot time and therefore
 they count the APIC IDs which have the enabled flag cleared to size
 correctly for potential hotplug operations.

 But that has diametral effects on bare metal because the OS is not
 able to distinguish between hot-plugable APIC ID and truly disabled
 entries. That results in overallocation or suboptimal distribution
 of multi-queue devices.

The benefit (verbatim):

 The proposed “Online Capable” flag will allow OSPM to unequivocally
 discern the platform’s intention regarding processors that are not
 enabled at OS boot-time.

Now look at the outcome:

Enabled:

If this bit is set the processor is ready for use. If this bit
is clear and the Online Capable bit is set, system hardware
supports enabling this processor during OS runtime. If this bit
is clear and the Online Capable bit is also clear, this
processor is unusable, and OSPM shall ignore the contents of the
Processor Local APIC Structure.

So while I conceed that this does not expressis verbis mandate that
hot-pluggable CPUs must be enumerated it's not far fetched to interpret
it this way. The key is 'OSPM shall ignore the contents...'

As MADT is the only source of information from

[ovmf test] 182568: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf 0c4d0b6c8a05a6a62c0dca042f8e15e579e6f4af
baseline version:
 ovmf a481c1114474160f53b1662fd3726b48c88ae82e

Last test of basis   182567  2023-08-30 10:12:15 Z0 days
Testing same since   182568  2023-08-30 13:13:53 Z0 days1 attempts


People who touched revisions under test:
  Dun Tan 
  Tan, Dun 
  Wu, Mingliang 
  Wu, MingliangX 

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
   a481c11144..0c4d0b6c8a  0c4d0b6c8a05a6a62c0dca042f8e15e579e6f4af -> 
xen-tested-master

RE: [EXT] Re: xen arm64 low power sleep support

[Dan Waqar]

-Original Message-
From: Stefano Stabellini 
Sent: Tuesday, August 29, 2023 4:13 PM
To: Anthony Chan 
Cc: xen-devel@lists.xenproject.org; bertrand.marq...@arm.com; jul...@xen.org; 
volodymyr_babc...@epam.com; michal.or...@amd.com; Dan Waqar 
; sstabell...@kernel.org
Subject: [EXT] Re: xen arm64 low power sleep support

On Tue, 29 Aug 2023, Anthony Chan wrote:
> Hi all,
>
> My name is Tony and I've been researching/developing using Xen for potential 
> upcoming uses in our embedded systems.  I started with Xen using Xilinx tools 
> about a year ago and still have lots to learn about what it can to do in the 
> embedded space.  So far, I've managed to integrate Xen and Linux into an 
> existing product that exclusively runs bare-metal code on a ZynqMP SoC and 
> migrate some of the functionality into custom Linux driver/userspace.
>
> I'm now looking at low power support, for now at least between Xen (4.16) and 
> Linux (5.15) dom0.  I've tried a few different Linux kernel configs around 
> power management and each time I try to suspend from linux dom0 (via sysfs or 
> systemctl), Xen will watchdog on dom0 guest.  AFAIK, Xen should trap on a 
> 'WFI' from guests, but from what I can tell debugging through the linux 
> suspend process is it's spinning in a 'suspend-to-idle' loop before it can 
> get to issuing a 'WFI' or using PSCI interface to notify Xen.  I'm beginning 
> to suspect that 'low power' support for embedded arm64 just isn't quite there 
> yet, or am I missing something in the configs?
>
> I realize this could very well be a Linux 'issue' but checking here first.  I 
> know Xen presents a flattened device tree to Linux without CPU idle-state 
> nodes and maybe this is causing the linux guest to only do the 
> suspend-to-idle mode?  I should mention that I'm booting up using dom0less 
> feature if that matters.


Hi Anthony,

Assuming you are using the default Xen command line parameters for Xilinx 
boards: sched=null vwfi=native, then if the guest uses WFI, the CPU will 
execute WFI directly and go into low power mode.

Given the issue you are describing, I am suspecting the guest is not issuing 
WFI: that is simple and known to work. Instead, I suspect that Linux might be 
trying to use PSCI_suspend in a way that is not supported or well-implemented 
by Xen.

Can you check? You can add a printk in Linux 
drivers/firmware/psci/psci.c:__psci_cpu_suspend or in Xen 
xen/arch/arm/vpsci.c:do_psci_0_2_cpu_suspend

If that is the case, the attached patch might work because it disables 
PSCI_suspend support in Xen and Linux should fall back to WFI. (There is no 
power saving in using PSCI_suspend versus WFI.)

Cheers,

Stefano

CONFIDENTIALITY NOTICE: This e-mail, including any attachments, may contain 
information that is confidential and privileged. Any unauthorized disclosure, 
reproduction or use of this e-mail is prohibited. If you are not the intended 
recipient, please notify the sender by reply e-mail or telephone and 
permanently delete this e-mail and any reproductions immediately.

Re: [PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Andrew Cooper]

On 30/08/2023 4:12 pm, Jan Beulich wrote:
> On 30.08.2023 16:35, Andrew Cooper wrote:
>> On 29/08/2023 3:08 pm, Jan Beulich wrote:
>>> On 29.08.2023 15:43, Andrew Cooper wrote:
 --- a/xen/arch/x86/domain.c
 +++ b/xen/arch/x86/domain.c
 @@ -1074,8 +1074,27 @@ int arch_set_info_guest(
  #endif
  flags = c(flags);
  
 +if ( !compat )
 +{
 +if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
 + c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
 +return -EINVAL;
 +}
 +
  if ( is_pv_domain(d) )
  {
 +/*
 + * Prior to Xen 4.11, dr5 was used to hold the emulated-only
 + * subset of dr7, and dr4 was unused.
 + *
 + * In Xen 4.11 and later, dr4/5 are written as zero, ignored for
 + * backwards compatibility, and dr7 emulation is handled
 + * internally.
 + */
 +for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
 +if ( !access_ok(c(debugreg[i]), sizeof(long)) )
>>> Don't you mean __addr_ok() here, i.e. not including the
>>> is_compat_arg_xlat_range() check? (Else I would have asked why
>>> sizeof(long), but that question resolves itself with using the other
>>> macro.)
>> For now, I'm simply moving a check from set_debugreg() earlier in
>> arch_set_info_guest().
>>
>> I think it would be beneficial to keep that change independent.
> Hmm, difficult. I'd be okay if you indeed moved the other check. But
> you duplicate it here, and duplicating questionable code is, well,
> questionable.

It can't be removed in set_debugreg() because that's used in other paths
too.

And the error from set_debugreg() can't fail arch_set_info_guest()
because that introduces a failure after mutation of the vCPU state.

This isn't a fastpath.  It's used approximately once per vCPU lifetime.

~Andrew

Re: [XEN PATCH 07/13] x86/asm: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

On 29/08/23 00:30, Stefano Stabellini wrote:

On Mon, 28 Aug 2023, Simone Ballarin wrote:

Add or move inclusion guards to address violations of
MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
to prevent the contents of a header file being included more than
once").

Inclusion guards must appear at the beginning of the headers
(comments are permitted anywhere).

The text of the beggining comment of cpufeatures.h has been changed
to match the deviation in automation/eclair_analysis/ECLAIR/deviations.ecl,
moreover this new formulation is already used in other files.


I don't think it is a good idea to do this kind of textual matching.
Instead we should use the format introduced by safe.json, e.g.
SAF-1-safe



I agree. I will use a comment-based deviation.




Mechanical change.

Signed-off-by: Simone Ballarin 
---
  xen/arch/x86/include/asm/compat.h  | 5 +
  xen/arch/x86/include/asm/cpufeatures.h | 4 +---
  xen/arch/x86/include/asm/efibind.h | 5 +
  xen/arch/x86/include/asm/hypercall.h   | 6 +++---
  4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/include/asm/compat.h 
b/xen/arch/x86/include/asm/compat.h
index 818cad87db..3d3891d061 100644
--- a/xen/arch/x86/include/asm/compat.h
+++ b/xen/arch/x86/include/asm/compat.h
@@ -2,6 +2,9 @@
   * compat.h
   */
  
+#ifndef __ASM_X86_COMPAT_H__

+#define __ASM_X86_COMPAT_H__
+
  #ifdef CONFIG_COMPAT
  
  #define COMPAT_BITS_PER_LONG 32

@@ -18,3 +21,5 @@ int switch_compat(struct domain *);
  #include 
  static inline int switch_compat(struct domain *d) { return -EOPNOTSUPP; }
  #endif
+
+#endif /* __ASM_X86_COMPAT_H__ */
diff --git a/xen/arch/x86/include/asm/cpufeatures.h 
b/xen/arch/x86/include/asm/cpufeatures.h
index da0593de85..1dfdd478ab 100644
--- a/xen/arch/x86/include/asm/cpufeatures.h
+++ b/xen/arch/x86/include/asm/cpufeatures.h
@@ -1,6 +1,4 @@
-/*
- * Explicitly intended for multiple inclusion.
- */
+/* This file is legitimately included multiple times */
  
  #include 
  
diff --git a/xen/arch/x86/include/asm/efibind.h b/xen/arch/x86/include/asm/efibind.h

index bce02f3707..f2eb8b5496 100644
--- a/xen/arch/x86/include/asm/efibind.h
+++ b/xen/arch/x86/include/asm/efibind.h
@@ -1,2 +1,7 @@
+#ifndef __ASM_X86_EFIBIND_H__
+#define __ASM_X86_EFIBIND_H__
+
  #include 
  #include 
+
+#endif /* __ASM_X86_EFIBIND_H__ */
diff --git a/xen/arch/x86/include/asm/hypercall.h 
b/xen/arch/x86/include/asm/hypercall.h
index ec2edc771e..2ade5d71b8 100644
--- a/xen/arch/x86/include/asm/hypercall.h
+++ b/xen/arch/x86/include/asm/hypercall.h
@@ -2,13 +2,13 @@
   * asm-x86/hypercall.h
   */
  
+#ifndef __ASM_X86_HYPERCALL_H__

+#define __ASM_X86_HYPERCALL_H__
+
  #ifndef __XEN_HYPERCALL_H__
  #error "asm/hypercall.h should not be included directly - include xen/hypercall.h 
instead"
  #endif
  
-#ifndef __ASM_X86_HYPERCALL_H__

-#define __ASM_X86_HYPERCALL_H__
-
  #include 
  #include 
  #include 
--
2.34.1



--
Simone Ballarin, M.Sc.

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

Re: [PATCH v2 3/8] x86/emul: Add pending_dbg field to x86_event

[Jan Beulich]

On 30.08.2023 16:20, Andrew Cooper wrote:
> On 30/08/2023 2:39 pm, Jan Beulich wrote:
>> On 24.08.2023 17:26, Jinoh Kang wrote:
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>>> @@ -78,7 +78,10 @@ struct x86_event {
>>>  uint8_t   type; /* X86_EVENTTYPE_* */
>>>  uint8_t   insn_len; /* Instruction length */
>>>  int32_t   error_code;   /* X86_EVENT_NO_EC if n/a */
>>> -unsigned long cr2;  /* Only for X86_EXC_PF h/w exception */
>>> +union {
>>> +unsigned long cr2; /* #PF */
>>> +unsigned long pending_dbg; /* #DB (new DR6 bits, positive 
>>> polarity) */
> 
> As a tangent, since I wrote the original series, there's #NM and
> MSR_XFD_ERR which needs to fit into this union for AMX support.

In "x86: XFD enabling" (posted over 2 years ago) I'm getting away
without this quite fine, and I didn't think it's wrong to write the
MSR right from the emulator (using the write_msr() hook).

Jan

Re: [XEN PATCH 06/13] x86/EFI: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

On 29/08/23 15:27, Jan Beulich wrote:

On 28.08.2023 15:20, Simone Ballarin wrote:

Add inclusion guards to address violations of
MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
to prevent the contents of a header file being included more than
once").

Mechanical change.

Signed-off-by: Simone Ballarin 
---
  xen/arch/x86/efi/efi-boot.h | 6 ++
  xen/arch/x86/efi/runtime.h  | 5 +
  2 files changed, 11 insertions(+)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 92f4cfe8bd..2c6be062cc 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -3,6 +3,10 @@
   * is intended to be included by common/efi/boot.c _only_, and
   * therefore can define arch specific global variables.
   */
+
+#ifndef __X86_EFI_EFI_BOOT_H__
+#define __X86_EFI_EFI_BOOT_H__


Considering the comment, I find the need for a guard here quite
"interesting", to be honest.

Jan
I don't think a simple comment is enough to say that "precautions have 
been taken". For the moment, I will drop this change to keep this patch 
going.


--
Simone Ballarin, M.Sc.

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

Re: [PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Jan Beulich]

On 30.08.2023 16:35, Andrew Cooper wrote:
> On 29/08/2023 3:08 pm, Jan Beulich wrote:
>> On 29.08.2023 15:43, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -1074,8 +1074,27 @@ int arch_set_info_guest(
>>>  #endif
>>>  flags = c(flags);
>>>  
>>> +if ( !compat )
>>> +{
>>> +if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
>>> + c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
>>> +return -EINVAL;
>>> +}
>>> +
>>>  if ( is_pv_domain(d) )
>>>  {
>>> +/*
>>> + * Prior to Xen 4.11, dr5 was used to hold the emulated-only
>>> + * subset of dr7, and dr4 was unused.
>>> + *
>>> + * In Xen 4.11 and later, dr4/5 are written as zero, ignored for
>>> + * backwards compatibility, and dr7 emulation is handled
>>> + * internally.
>>> + */
>>> +for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
>>> +if ( !access_ok(c(debugreg[i]), sizeof(long)) )
>> Don't you mean __addr_ok() here, i.e. not including the
>> is_compat_arg_xlat_range() check? (Else I would have asked why
>> sizeof(long), but that question resolves itself with using the other
>> macro.)
> 
> For now, I'm simply moving a check from set_debugreg() earlier in
> arch_set_info_guest().
> 
> I think it would be beneficial to keep that change independent.

Hmm, difficult. I'd be okay if you indeed moved the other check. But
you duplicate it here, and duplicating questionable code is, well,
questionable.

Jan

RE: [EXT] Re: xen arm64 low power sleep support

[Anthony Chan]

On Tue, 29 Aug 2023, Stefano Stabellini wrote:
> On Tue, 29 Aug 2023, Anthony Chan wrote:
> > Hi all,
> >
> > My name is Tony and I've been researching/developing using Xen for
> potential upcoming uses in our embedded systems.  I started with Xen
> using Xilinx tools about a year ago and still have lots to learn about what it
> can to do in the embedded space.  So far, I've managed to integrate Xen
> and Linux into an existing product that exclusively runs bare-metal code on
> a ZynqMP SoC and migrate some of the functionality into custom Linux
> driver/userspace.
> >
> > I'm now looking at low power support, for now at least between Xen
> (4.16) and Linux (5.15) dom0.  I've tried a few different Linux kernel
> configs around power management and each time I try to suspend from
> linux dom0 (via sysfs or systemctl), Xen will watchdog on dom0 guest.
> AFAIK, Xen should trap on a 'WFI' from guests, but from what I can tell
> debugging through the linux suspend process is it's spinning in a 'suspend-
> to-idle' loop before it can get to issuing a 'WFI' or using PSCI interface to
> notify Xen.  I'm beginning to suspect that 'low power' support for
> embedded arm64 just isn't quite there yet, or am I missing something in
> the configs?
> >
> > I realize this could very well be a Linux 'issue' but checking here first.  
> > I
> know Xen presents a flattened device tree to Linux without CPU idle-state
> nodes and maybe this is causing the linux guest to only do the suspend-
> to-idle mode?  I should mention that I'm booting up using dom0less
> feature if that matters.
>
>
> Hi Anthony,
>
> Assuming you are using the default Xen command line parameters for
> Xilinx boards: sched=null vwfi=native, then if the guest uses WFI, the CPU
> will execute WFI directly and go into low power mode.
Yes, using these command line params.

> Given the issue you are describing, I am suspecting the guest is not issuing
> WFI: that is simple and known to work. Instead, I suspect that Linux might
> be trying to use PSCI_suspend in a way that is not supported or well-
> implemented by Xen.
>
> Can you check? You can add a printk in Linux
> drivers/firmware/psci/psci.c:__psci_cpu_suspend or in Xen
> xen/arch/arm/vpsci.c:do_psci_0_2_cpu_suspend
Instrumented both places it doesn't appear to reach there.  In 
kernel/power/suspend.c, there's a call to s2idle_loop that it's currently 
'stuck' in and I think it doesn't get to the psci suspend your referring till 
afterwards, when suspend_ops->enter is called.  Unfortunately, without any 
idle-states nodes in the FDT, the only suspend state Linux is defaults to is 
'suspend to idle'.

Sorry about the boilerplate confidentiality footer below, I am not allowed to 
disable it...



CONFIDENTIALITY NOTICE: This e-mail, including any attachments, may contain 
information that is confidential and privileged. Any unauthorized disclosure, 
reproduction or use of this e-mail is prohibited. If you are not the intended 
recipient, please notify the sender by reply e-mail or telephone and 
permanently delete this e-mail and any reproductions immediately.

Re: [PATCH v7 9/9] swiotlb: search the software IO TLB only if the device makes use of it

[Jonathan Corbet]

So it seems this code got merged without this question ever being
answered.  Sorry if it's a dumb one, but I don't think this
functionality works as advertised...

Thanks,

jon

Jonathan Corbet  writes:

> Petr Tesarik  writes:
>
>> From: Petr Tesarik 
>>
>> Skip searching the software IO TLB if a device has never used it, making
>> sure these devices are not affected by the introduction of multiple IO TLB
>> memory pools.
>>
>> Additional memory barrier is required to ensure that the new value of the
>> flag is visible to other CPUs after mapping a new bounce buffer. For
>> efficiency, the flag check should be inlined, and then the memory barrier
>> must be moved to is_swiotlb_buffer(). However, it can replace the existing
>> barrier in swiotlb_find_pool(), because all callers use is_swiotlb_buffer()
>> first to verify that the buffer address belongs to the software IO TLB.
>>
>> Signed-off-by: Petr Tesarik 
>> ---
>
> Excuse me if this is a silly question, but I'm not able to figure it out
> on my own...
>
>>  include/linux/device.h  |  2 ++
>>  include/linux/swiotlb.h |  7 ++-
>>  kernel/dma/swiotlb.c| 14 ++
>>  3 files changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 5fd89c9d005c..6fc808d22bfd 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -628,6 +628,7 @@ struct device_physical_location {
>>   * @dma_io_tlb_mem: Software IO TLB allocator.  Not for driver use.
>>   * @dma_io_tlb_pools:   List of transient swiotlb memory pools.
>>   * @dma_io_tlb_lock:Protects changes to the list of active pools.
>> + * @dma_uses_io_tlb: %true if device has used the software IO TLB.
>>   * @archdata:   For arch-specific additions.
>>   * @of_node:Associated device tree node.
>>   * @fwnode: Associated device node supplied by platform firmware.
>> @@ -737,6 +738,7 @@ struct device {
>>  #ifdef CONFIG_SWIOTLB_DYNAMIC
>>  struct list_head dma_io_tlb_pools;
>>  spinlock_t dma_io_tlb_lock;
>> +bool dma_uses_io_tlb;
>
> You add this new member here, fine...
>
>>  #endif
>>  /* arch specific additions */
>>  struct dev_archdata archdata;
>> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
>> index 8371c92a0271..b4536626f8ff 100644
>> --- a/include/linux/swiotlb.h
>> +++ b/include/linux/swiotlb.h
>> @@ -172,8 +172,13 @@ static inline bool is_swiotlb_buffer(struct device 
>> *dev, phys_addr_t paddr)
>>  if (!mem)
>>  return false;
>>  
>> -if (IS_ENABLED(CONFIG_SWIOTLB_DYNAMIC))
>> +if (IS_ENABLED(CONFIG_SWIOTLB_DYNAMIC)) {
>> +/* Pairs with smp_wmb() in swiotlb_find_slots() and
>> + * swiotlb_dyn_alloc(), which modify the RCU lists.
>> + */
>> +smp_rmb();
>>  return swiotlb_find_pool(dev, paddr);
>> +}
>>  return paddr >= mem->defpool.start && paddr < mem->defpool.end;
>>  }
>>  
>> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
>> index adf80dec42d7..d7eac84f975b 100644
>> --- a/kernel/dma/swiotlb.c
>> +++ b/kernel/dma/swiotlb.c
>> @@ -730,7 +730,7 @@ static void swiotlb_dyn_alloc(struct work_struct *work)
>>  
>>  add_mem_pool(mem, pool);
>>  
>> -/* Pairs with smp_rmb() in swiotlb_find_pool(). */
>> +/* Pairs with smp_rmb() in is_swiotlb_buffer(). */
>>  smp_wmb();
>>  }
>>  
>> @@ -764,11 +764,6 @@ struct io_tlb_pool *swiotlb_find_pool(struct device 
>> *dev, phys_addr_t paddr)
>>  struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
>>  struct io_tlb_pool *pool;
>>  
>> -/* Pairs with smp_wmb() in swiotlb_find_slots() and
>> - * swiotlb_dyn_alloc(), which modify the RCU lists.
>> - */
>> -smp_rmb();
>> -
>>  rcu_read_lock();
>>  list_for_each_entry_rcu(pool, &mem->pools, node) {
>>  if (paddr >= pool->start && paddr < pool->end)
>> @@ -813,6 +808,7 @@ void swiotlb_dev_init(struct device *dev)
>>  #ifdef CONFIG_SWIOTLB_DYNAMIC
>>  INIT_LIST_HEAD(&dev->dma_io_tlb_pools);
>>  spin_lock_init(&dev->dma_io_tlb_lock);
>> +dev->dma_uses_io_tlb = false;
>
> ...here you initialize it, fine...
>
>>  #endif
>>  }
>>  
>> @@ -1157,9 +1153,11 @@ static int swiotlb_find_slots(struct device *dev, 
>> phys_addr_t orig_addr,
>>  list_add_rcu(&pool->node, &dev->dma_io_tlb_pools);
>>  spin_unlock_irqrestore(&dev->dma_io_tlb_lock, flags);
>>  
>> -/* Pairs with smp_rmb() in swiotlb_find_pool(). */
>> -smp_wmb();
>>  found:
>> +dev->dma_uses_io_tlb = true;
>> +/* Pairs with smp_rmb() in is_swiotlb_buffer() */
>> +smp_wmb();
>> +
>
> ...and here you set it if swiotlb is used.
>
> But, as far as I can tell, you don't actually *use* this field anywhere.
> What am I missing?
>
> Thanks,
>
> jon

Re: [XEN PATCH 11/13] xen/sched: address violations of MISRA C:2012 Directive 4.10

[George Dunlap]

On Mon, Aug 28, 2023 at 2:20 PM Simone Ballarin
 wrote:
>
> Add inclusion guards to address violations of
> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> to prevent the contents of a header file being included more than
> once").
>
> Mechanical change.
>
> Signed-off-by: Simone Ballarin 

Reviewed-by: George Dunlap 

Re: [PATCH v2 2/8] x86/hvm: Only populate info->cr2 for #PF in hvm_get_pending_event()

[Jinoh Kang]

On 8/30/23 22:41, Jan Beulich wrote:
> On 24.08.2023 17:25, Jinoh Kang wrote:
>> Prepare for an upcoming patch that overloads the 'cr2' field for #DB.
> 
> Seeing the subsequent change and the fact that earlier on Andrew didn't
> need such an adjustment, I'm afraid I can't see the need for this change,
> and the one sentence above also doesn't answer the "Why?", but only the
> "What?"

This is part of the hvm_monitor_interrupt() fix (patch 4), which would
otherwise get CR2 value (instead of PENDING_DBG) even for #DB.

I might have been overzealous, though, since there is no known (broken)
use for VM_EVENT_REASON_INTERRUPT in the first place.

> 
> Jan
> 
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -503,9 +503,14 @@ void hvm_migrate_pirqs(struct vcpu *v)
>>  
>>  static bool hvm_get_pending_event(struct vcpu *v, struct x86_event *info)
>>  {
>> -info->cr2 = v->arch.hvm.guest_cr[2];
>> +if ( !alternative_call(hvm_funcs.get_pending_event, v, info) )
>> +return false;
>> +
>> +if ( info->type == X86_EVENTTYPE_HW_EXCEPTION &&
>> + info->vector == X86_EXC_PF )
>> +info->cr2 = v->arch.hvm.guest_cr[2];
>>  
>> -return alternative_call(hvm_funcs.get_pending_event, v, info);
>> +return true;
>>  }
>>  
>>  void hvm_do_resume(struct vcpu *v)
> 

-- 
Sincerely,
Jinoh Kang

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

[Simone Ballarin]

On 29/08/23 00:27, Stefano Stabellini wrote:

+Nicola, Luca

On Mon, 28 Aug 2023, Simone Ballarin wrote:

xen/arch/x86/usercopy.c includes itself, so it is not supposed to
comply with Directive 4.10:
"Precautions shall be taken in order to prevent the contents of a
header file being included more than once"

This patch adds a deviation for the file.

Signed-off-by: Simone Ballarin 

---
  automation/eclair_analysis/ECLAIR/deviations.ecl | 4 
  docs/misra/rules.rst | 2 ++
  2 files changed, 6 insertions(+)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
b/automation/eclair_analysis/ECLAIR/deviations.ecl
index 2681a4cff5..a7d4f29b43 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -96,6 +96,10 @@ conform to the directive."
  -config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no 
inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"}
  -doc_end
  
+-doc_begin="xen/arch/x86/usercopy.c includes itself: it is not supposed to comply with the directive"

+-config=MC3R1.D4.10,reports+={deliberate, 
"all_area(all_loc(file("^xen/arch/x86/usercopy\\.c$")))"}
+-doc_end
+
  #
  # Series 5.
  #
diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
index 4b1a7b02b6..45e13d0302 100644
--- a/docs/misra/rules.rst
+++ b/docs/misra/rules.rst
@@ -62,6 +62,8 @@ maintainers if you want to suggest a change.
   - Files that are intended to be included more than once do not need to
 conform to the directive. Files that explicitly avoid inclusion guards
 under specific circumstances do not need to conform the directive.
+   xen/arch/x86/usercopy.c includes itself: it is not supposed to comply
+   with the directive.



We need to find a consistent way to document this kind of deviations in
a non-ECLAIR specific way, without adding the complete list of
deviations to rules.rst.

Can we use safe.json and add an in-code comment at the top of
usercopy.c? E.g.:

diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c
index b8c2d1cc0b..8bb591f472 100644
--- a/xen/arch/x86/usercopy.c
+++ b/xen/arch/x86/usercopy.c
@@ -1,3 +1,4 @@
+/* SAF-1-safe */
  /*
   * User address space access functions.
   *
 > Otherwise, maybe we should extend safe.json to also have an extra field
with a list of paths. For instance see "files" below >
{
 "version": "1.0",
 "content": [
 {
 "id": "SAF-0-safe",
 "analyser": {
 "eclair": "MC3R1.R8.6",
 "coverity": "misra_c_2012_rule_8_6_violation"
 },
 "name": "Rule 8.6: linker script defined symbols",
 "text": "It is safe to declare this symbol because it is defined in the 
linker script."
 },
 {
 "id": "SAF-1-safe",
 "analyser": {
 "eclair": "MC3R1.D4.10"
 },
 "name": "Dir 4.10: files that include themselves",
 "text": "Files purposely written to include themselves are not supposed 
to comply with D4.10.",
 "files": ["xen/arch/x86/usercopy.c"]
 },
 {
 "id": "SAF-2-safe",
 "analyser": {},
 "name": "Sentinel",
 "text": "Next ID to be used"
 }
 ]
}

In general, I prefer the first option for such ad hoc deviation (the 
comment at the beginning of the file): this way, anyone who touches the 
file will immediately see the comment and think as its changes will 
affect the deviation (is it still safe? is it still necessary?).


To help the developer more, I think it is better to also add the "name" 
in the comment, this is my proposal:


/* SAF-4-safe Dir 4.10: files that include themselves*/
/*
 * User address space access functions.
 *
 * Copyright 1997 Andi Kleen 
 * Copyright 1997 Linus Torvalds
 * Copyright 2002 Andi Kleen 
 */

--
Simone Ballarin, M.Sc.

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

Re: [PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Andrew Cooper]

On 30/08/2023 7:46 am, Jan Beulich wrote:
> On 29.08.2023 15:43, Andrew Cooper wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1074,8 +1074,27 @@ int arch_set_info_guest(
>>  #endif
>>  flags = c(flags);
>>  
>> +if ( !compat )
>> +{
>> +if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
>> + c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
>> +return -EINVAL;
>> +}
>> +
>>  if ( is_pv_domain(d) )
>>  {
>> +/*
>> + * Prior to Xen 4.11, dr5 was used to hold the emulated-only
>> + * subset of dr7, and dr4 was unused.
>> + *
>> + * In Xen 4.11 and later, dr4/5 are written as zero, ignored for
>> + * backwards compatibility, and dr7 emulation is handled
>> + * internally.
>> + */
>> +for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
>> +if ( !access_ok(c(debugreg[i]), sizeof(long)) )
>> +return -EINVAL;
>> +
>>  if ( !compat )
>>  {
>>  if ( !is_canonical_address(c.nat->user_regs.rip) ||
> One more thing here: v->arch.dr is an array of 4 elements, i.e. doesn't
> cover %dr4 and up.

Correct (as of the same changeset relevant in this comment).

> That's not directly visible here, though, so the
> comment ahead of the loop talking about those other 4 registers is a
> little misleading. Would you mind moving it below the loop?

Can do.

~Andrew

Re: [PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Andrew Cooper]

On 29/08/2023 3:08 pm, Jan Beulich wrote:
> On 29.08.2023 15:43, Andrew Cooper wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1074,8 +1074,27 @@ int arch_set_info_guest(
>>  #endif
>>  flags = c(flags);
>>  
>> +if ( !compat )
>> +{
>> +if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
>> + c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
>> +return -EINVAL;
>> +}
>> +
>>  if ( is_pv_domain(d) )
>>  {
>> +/*
>> + * Prior to Xen 4.11, dr5 was used to hold the emulated-only
>> + * subset of dr7, and dr4 was unused.
>> + *
>> + * In Xen 4.11 and later, dr4/5 are written as zero, ignored for
>> + * backwards compatibility, and dr7 emulation is handled
>> + * internally.
>> + */
>> +for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
>> +if ( !access_ok(c(debugreg[i]), sizeof(long)) )
> Don't you mean __addr_ok() here, i.e. not including the
> is_compat_arg_xlat_range() check? (Else I would have asked why
> sizeof(long), but that question resolves itself with using the other
> macro.)

For now, I'm simply moving a check from set_debugreg() earlier in
arch_set_info_guest().

I think it would be beneficial to keep that change independent.

~Andrew

Re: [PATCH v2 1/2] xen: add stubs dir to include path

[Oleksii]

On Wed, 2023-08-30 at 09:30 +0200, Jan Beulich wrote:
> On 29.08.2023 16:34, Oleksii Kurochko wrote:
> > --- a/xen/Makefile
> > +++ b/xen/Makefile
> > @@ -438,6 +438,7 @@ ifdef building_out_of_srctree
> >  endif
> >  CFLAGS += -I$(srctree)/include
> >  CFLAGS += -I$(srctree)/arch/$(SRCARCH)/include
> > +CFLAGS += -I$(srctree)/include/stubs
> >  
> >  # Note that link order matters!
> >  ALL_OBJS-y    := common/built_in.o
> 
> I have to admit that I'm not entirely happy with the name. I wonder
> what
> other REST maintainers think. In particular I would consider using
> Linux'es "asm-generic" name instead, allowing to cover both stub
> headers
> like the one to be introduced here, but also other kinds of
> "fallback"
> ones.
It make sense. I'll update the patch series.


~ Oleksii

Re: [Xen-devel] [PATCH] x86/HVM: correct hvmemul_map_linear_addr() for multi-page case

[Roger Pau Monné]

On Wed, Sep 12, 2018 at 03:09:35AM -0600, Jan Beulich wrote:
> The function does two translations in one go for a single guest access.
> Any failure of the first translation step (guest linear -> guest
> physical), resulting in #PF, ought to take precedence over any failure
> of the second step (guest physical -> host physical).

If my understanding is correct, this is done so that any #PF that
would arise from the access is injected to the guest, regardless of
whether there might also be gfn -> mfn errors that would otherwise
prevent the #PF from being detected.

> Bail out of the
> loop early solely when translation produces HVMTRANS_bad_linear_to_gfn,
> and record the most relevant of perhaps multiple different errors
> otherwise. (The choice of ZERO_BLOCK_PTR as sentinel is arbitrary.)
> 
> Signed-off-by: Jan Beulich 
> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -531,6 +531,20 @@ static int hvmemul_do_mmio_addr(paddr_t
>  return hvmemul_do_io_addr(1, mmio_gpa, reps, size, dir, df, ram_gpa);
>  }
>  
> +static void *update_map_err(void *err, void *new)
> +{
> +if ( err == ZERO_BLOCK_PTR || err == ERR_PTR(~X86EMUL_OKAY) )
> +return new;
> +
> +if ( new == ERR_PTR(~X86EMUL_OKAY) )
> +return err;
> +
> +if ( err == ERR_PTR(~X86EMUL_RETRY) )
> +return new;
> +
> +return err;
> +}
> +
>  /*
>   * Map the frame(s) covering an individual linear access, for writeable
>   * access.  May return NULL for MMIO, or ERR_PTR(~X86EMUL_*) for other errors
> @@ -544,7 +558,7 @@ static void *hvmemul_map_linear_addr(
>  struct hvm_emulate_ctxt *hvmemul_ctxt)
>  {
>  struct vcpu *curr = current;
> -void *err, *mapping;
> +void *err = ZERO_BLOCK_PTR, *mapping;
>  unsigned int nr_frames = ((linear + bytes - !!bytes) >> PAGE_SHIFT) -
>  (linear >> PAGE_SHIFT) + 1;
>  unsigned int i;
> @@ -600,27 +614,28 @@ static void *hvmemul_map_linear_addr(
>  goto out;
>  
>  case HVMTRANS_bad_gfn_to_mfn:
> -err = NULL;
> -goto out;
> +err = update_map_err(err, NULL);
> +continue;
>  
>  case HVMTRANS_gfn_paged_out:
>  case HVMTRANS_gfn_shared:
> -err = ERR_PTR(~X86EMUL_RETRY);
> -goto out;
> +err = update_map_err(err, ERR_PTR(~X86EMUL_RETRY));
> +continue;
>  
>  default:
> -goto unhandleable;
> +err = update_map_err(err, ERR_PTR(~X86EMUL_UNHANDLEABLE));
> +continue;
>  }
>  
>  *mfn++ = page_to_mfn(page);

I have to admit it find it weird that since now we don't exit the loop
when HVMTRANS_bad_gfn_to_mfn is returned, the item at mfn[0] might
point to the gfn -> mfn translation for the second half of the access.
AFAICT that would happen if the first half of the access fails to
translate with an error !HVMTRANS_bad_linear_to_gfn and the second
half is successful.

I guess it doesn't matter much, because the vmap below will be
skipped, might still be worth to add a comment.

In fact, if the first translation fails the following ones could use
the cheaper paging_gva_to_gfn(), as we no longer care to get the
underlying page, and just need to figure out whether the access would
trigger a #PF?

Thanks, Roger.

Re: [PATCH v2 3/8] x86/emul: Add pending_dbg field to x86_event

[Andrew Cooper]

On 30/08/2023 2:39 pm, Jan Beulich wrote:
> On 24.08.2023 17:26, Jinoh Kang wrote:
>> @@ -62,9 +63,16 @@ void pv_inject_event(const struct x86_event *event)
>>  error_code |= PFEC_user_mode;
>>  
>>  trace_pv_page_fault(event->cr2, error_code);
>> -}
>> -else
>> +break;
>> +
>> +case X86_EXC_DB:
>> +curr->arch.dr6 |= event->pending_dbg;
>> +/* Fallthrough */
> I guess I have another question here, perhaps more to Andrew: How come
> this is just an OR? Not only with some of the bits having inverted sense
> and earlier logic being ...
>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -1989,17 +1989,17 @@ void do_debug(struct cpu_user_regs *regs)
>>  return;
>>  }
>>  
>> -/* Save debug status register where guest OS can peek at it */
>> -v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
>> -v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
> ... an OR and an AND, but also with ...
>
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>> @@ -78,7 +78,10 @@ struct x86_event {
>>  uint8_t   type; /* X86_EVENTTYPE_* */
>>  uint8_t   insn_len; /* Instruction length */
>>  int32_t   error_code;   /* X86_EVENT_NO_EC if n/a */
>> -unsigned long cr2;  /* Only for X86_EXC_PF h/w exception */
>> +union {
>> +unsigned long cr2; /* #PF */
>> +unsigned long pending_dbg; /* #DB (new DR6 bits, positive polarity) 
>> */

As a tangent, since I wrote the original series, there's #NM and
MSR_XFD_ERR which needs to fit into this union for AMX support.

Sadly, the only AMX hardware on the market right now has an errata where
XFD_ERR doesn't behave properly here.

> ... the comment here saying "positive polarity", which I understand
> to mean that inverted bits need inverting by the consumer of this
> field. If this is solely because none of the inverted bits are
> supported for PV, then I guess this wants a comment at the use site
> (not the least because it would need adjusting as soon as one such
> would become supported).

Part of this patch is (or was) introducing pending_dbg with no logical
change, but as I said, I don't think I had the original series split up
quite correctly either.


This field is more than just the inversion.  It needs to match the
semantics of the VMCS PENDING_DBG field, which is architectural but
otherwise hidden pipeline state, similar to the segment descriptor
cache.  The other necessary property is the (lack of) stickiness of bits
in the pending_dbg field.

All of that said, having talked to some pipeline people recent, I think
pending_dbg needs to be elsewhere.  Or perhaps a second copy elsewhere.

Some bits stay pending in pending_dbg across multiple instructions. 
This is how we get the MovSS-delays-breakpoints property that is a
security disaster elsewhere.

The problem with this is that we can't get at the pipeline pending_dbg
state for PV guests (where we've only got an architectural #DB to work
with) or for SVM guests (where this state isn't presented in the VMCB
despite existing internally).

~Andrew

Re: [PATCH v4 4/6] xen/vpci: header: status register handler

[Jan Beulich]

On 28.08.2023 19:56, Stewart Hildebrand wrote:
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -413,6 +413,18 @@ static void cf_check cmd_write(
>  pci_conf_write16(pdev->sbdf, reg, cmd);
>  }
>  
> +static uint32_t cf_check status_read(const struct pci_dev *pdev,
> + unsigned int reg, void *data)
> +{
> +struct vpci_header *header = data;
> +uint32_t status = pci_conf_read16(pdev->sbdf, reg);
> +
> +if ( header->mask_cap_list )
> +status &= ~PCI_STATUS_CAP_LIST;
> +
> +return status;
> +}

Imo we also cannot validly pass through any of the reserved bits. Doing so
is an option only once we know what purpose they might gain. (In this
context I notice our set of PCI_STATUS_* constants isn't quite up-to-date.)

> @@ -544,6 +556,11 @@ static int cf_check init_bars(struct pci_dev *pdev)
>  if ( rc )
>  return rc;
>  
> +rc = vpci_add_rw1c_register(pdev->vpci, status_read, vpci_hw_write16,
> +PCI_STATUS, 2, header, 0xF900);

Rather than a literal number, imo this wants to be an OR of the respective
PCI_STATUS_* constants (which, if you like, could of course be consolidated
into a new PCI_STATUS_RW1C_MASK, to help readability).

> @@ -167,6 +174,7 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t 
> *read_handler,
>  r->size = size;
>  r->offset = offset;
>  r->private = data;
> +r->rw1c_mask = rw1c_mask;

To avoid surprises with ...

> @@ -424,6 +443,7 @@ static void vpci_write_helper(const struct pci_dev *pdev,
>  uint32_t val;
>  
>  val = r->read(pdev, r->offset, r->private);
> +val &= ~r->rw1c_mask;
>  data = merge_result(val, data, size, offset);

... the user of this field, should you either assert that no bits beyond
the field size are set, or simply mask to the respective number of bits?

Jan

Re: [PATCH v2 3/8] x86/emul: Add pending_dbg field to x86_event

[Andrew Cooper]

On 30/08/2023 2:30 pm, Jan Beulich wrote:
> On 24.08.2023 17:26, Jinoh Kang wrote:
>> From: Andrew Cooper 
>>
>> All #DB exceptions result in an update of %dr6, but this isn't captured in
>> Xen's handling.
>>
>> PV guests generally work by modifying %dr6 before raising #DB, whereas HVM
>> guests do nothing and have a single-step special case in the lowest levels of
>> {vmx,svm}_inject_event().  All of this is buggy, but in particular, task
>> switches with the trace flag never end up signalling BT in %dr6.
>>
>> To begin resolving this issue, add a new pending_dbg field to x86_event
>> (unioned with cr2 to avoid taking any extra space), and introduce
>> {pv,hvm}_inject_debug_exn() helpers to replace the current callers using
>> {pv,hvm}_inject_hw_exception().
>>
>> A key property is that pending_dbg is taken with positive polarity to deal
>> with RTM sensibly.  Most callers pass in a constant, but callers passing in a
>> hardware %dr6 value need to xor the value with X86_DR6_DEFAULT to flip the
>> polarity of RTM and reserved fields.
>>
>> For PV guests, move the ad-hoc updating of %dr6 into pv_inject_event().  This
>> in principle breaks the handing of RTM in do_debug(), but PV guests can't
>> actually enable MSR_DEBUGCTL.RTM yet, so this doesn't matter in practice.
>>
>> Signed-off-by: Andrew Cooper 
>> Reviewed-by: Roger Pau Monné 
>> Reviewed-by: Jan Beulich 
>> [ jinoh: Rebase onto staging, forward declare struct vcpu ]
>> Signed-off-by: Jinoh Kang 
>> ---
>> CC: Andrew Cooper 
>> CC: Jan Beulich 
>> CC: Wei Liu 
>> CC: Roger Pau Monné 
>> CC: Jun Nakajima 
>> CC: Kevin Tian 
>> CC: Tim Deegan 
>> CC: Tamas K Lengyel 
>> CC: Alexandru Isaila 
>> CC: Petre Pircalabu 
>>
>> v1 -> v2: [S-o-b fixes. More details below.]
>>
>> - Update DR6 for gdbsx when trapped in PV guest kernel mode
> I take it that this refers to ...
>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -1989,17 +1989,17 @@ void do_debug(struct cpu_user_regs *regs)
>>  return;
>>  }
>>  
>> -/* Save debug status register where guest OS can peek at it */
>> -v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
>> -v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
>> -
>>  if ( guest_kernel_mode(v, regs) && v->domain->debugger_attached )
>>  {
>> +/* Save debug status register where gdbsx can peek at it */
>> +v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
>> +v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
>> +
>>  domain_pause_for_debugger();
>>  return;
>>  }
> ... this code movement. I'm afraid this should have resulted in you
> dropping the earlier R-b, and I'm further afraid I'm not convinced 
> this is correct, despite seeing why you would want to do this. The
> issue is that this way you also alter guest-visible state, when the
> intention is that such now happen via ...
>
>> -pv_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
>> +pv_inject_debug_exn(dr6 ^ X86_DR6_DEFAULT);
>>  }
> ... this call alone. I fear I can't currently see how to get both
> aspects right, other than by breaking up

I think it was wrongly broken up in my RFC series too.

With hindsight, I think the series wants rearranging to introduce
x86_merge_dr6() first, and then fix up PV and HVM separately.  They're
independent logic paths.

~Andrew

Re: [PATCH v4 3/6] x86/msi: rearrange read_pci_mem_bar slightly

[Jan Beulich]

On 28.08.2023 19:56, Stewart Hildebrand wrote:
> Use pdev->sbdf instead of the PCI_SBDF macro in calls to pci_* functions
> where appropriate. Move NULL check earlier.
> 
> Suggested-by: Jan Beulich 
> Signed-off-by: Stewart Hildebrand 

Reviewed-by: Jan Beulich 

Re: [PATCH v2 2/8] x86/hvm: Only populate info->cr2 for #PF in hvm_get_pending_event()

[Andrew Cooper]

On 30/08/2023 2:41 pm, Jan Beulich wrote:
> On 24.08.2023 17:25, Jinoh Kang wrote:
>> Prepare for an upcoming patch that overloads the 'cr2' field for #DB.
> Seeing the subsequent change and the fact that earlier on Andrew didn't
> need such an adjustment, I'm afraid I can't see the need for this change,
> and the one sentence above also doesn't answer the "Why?", but only the
> "What?"

I agree WRT the commit message.

I don't see why, but I also didn't spot this specific bug so I can't
rule out a bug in my original series.

That said, my original series was RFC because of the Monitor breakage
and didn't get much testing.

>
> Jan
>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -503,9 +503,14 @@ void hvm_migrate_pirqs(struct vcpu *v)
>>  
>>  static bool hvm_get_pending_event(struct vcpu *v, struct x86_event *info)
>>  {
>> -info->cr2 = v->arch.hvm.guest_cr[2];
>> +if ( !alternative_call(hvm_funcs.get_pending_event, v, info) )
>> +return false;
>> +
>> +if ( info->type == X86_EVENTTYPE_HW_EXCEPTION &&
>> + info->vector == X86_EXC_PF )
>> +info->cr2 = v->arch.hvm.guest_cr[2];

For the change itself, this needs pushing down into the vmx/svm hooks,
because guest_cr[2] has different liveness between Intel and AMD, and
this callpath needs to work for both scheduled-in and scheduled-out guests.

On AMD, I think you need to pull it straight out of the VMCB, rather
than relying on this being correct for a scheduled-in guest.

~Andrew

Re: [PATCH v4 2/6] xen/pci: convert pci_find_*cap* to pci_sbdf_t

[Jan Beulich]

On 28.08.2023 19:56, Stewart Hildebrand wrote:
> Convert pci_find_*cap* functions and call sites to pci_sbdf_t, and remove some
> now unused local variables. Also change to more appropriate types on lines 
> that
> are already being modified as a result of the pci_sbdf_t conversion.
> 
> Signed-off-by: Stewart Hildebrand 

Reviewed-by: Jan Beulich 

Re: [PATCH v2 2/8] x86/hvm: Only populate info->cr2 for #PF in hvm_get_pending_event()

[Jan Beulich]

On 24.08.2023 17:25, Jinoh Kang wrote:
> Prepare for an upcoming patch that overloads the 'cr2' field for #DB.

Seeing the subsequent change and the fact that earlier on Andrew didn't
need such an adjustment, I'm afraid I can't see the need for this change,
and the one sentence above also doesn't answer the "Why?", but only the
"What?"

Jan

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -503,9 +503,14 @@ void hvm_migrate_pirqs(struct vcpu *v)
>  
>  static bool hvm_get_pending_event(struct vcpu *v, struct x86_event *info)
>  {
> -info->cr2 = v->arch.hvm.guest_cr[2];
> +if ( !alternative_call(hvm_funcs.get_pending_event, v, info) )
> +return false;
> +
> +if ( info->type == X86_EVENTTYPE_HW_EXCEPTION &&
> + info->vector == X86_EXC_PF )
> +info->cr2 = v->arch.hvm.guest_cr[2];
>  
> -return alternative_call(hvm_funcs.get_pending_event, v, info);
> +return true;
>  }
>  
>  void hvm_do_resume(struct vcpu *v)

Re: [PATCH v2 3/8] x86/emul: Add pending_dbg field to x86_event

[Jan Beulich]

On 24.08.2023 17:26, Jinoh Kang wrote:
> @@ -62,9 +63,16 @@ void pv_inject_event(const struct x86_event *event)
>  error_code |= PFEC_user_mode;
>  
>  trace_pv_page_fault(event->cr2, error_code);
> -}
> -else
> +break;
> +
> +case X86_EXC_DB:
> +curr->arch.dr6 |= event->pending_dbg;
> +/* Fallthrough */

I guess I have another question here, perhaps more to Andrew: How come
this is just an OR? Not only with some of the bits having inverted sense
and earlier logic being ...

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1989,17 +1989,17 @@ void do_debug(struct cpu_user_regs *regs)
>  return;
>  }
>  
> -/* Save debug status register where guest OS can peek at it */
> -v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
> -v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);

... an OR and an AND, but also with ...

> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -78,7 +78,10 @@ struct x86_event {
>  uint8_t   type; /* X86_EVENTTYPE_* */
>  uint8_t   insn_len; /* Instruction length */
>  int32_t   error_code;   /* X86_EVENT_NO_EC if n/a */
> -unsigned long cr2;  /* Only for X86_EXC_PF h/w exception */
> +union {
> +unsigned long cr2; /* #PF */
> +unsigned long pending_dbg; /* #DB (new DR6 bits, positive polarity) 
> */

... the comment here saying "positive polarity", which I understand
to mean that inverted bits need inverting by the consumer of this
field. If this is solely because none of the inverted bits are
supported for PV, then I guess this wants a comment at the use site
(not the least because it would need adjusting as soon as one such
would become supported).

Jan

Re: [PATCH v2 3/8] x86/emul: Add pending_dbg field to x86_event

[Jan Beulich]

On 24.08.2023 17:26, Jinoh Kang wrote:
> From: Andrew Cooper 
> 
> All #DB exceptions result in an update of %dr6, but this isn't captured in
> Xen's handling.
> 
> PV guests generally work by modifying %dr6 before raising #DB, whereas HVM
> guests do nothing and have a single-step special case in the lowest levels of
> {vmx,svm}_inject_event().  All of this is buggy, but in particular, task
> switches with the trace flag never end up signalling BT in %dr6.
> 
> To begin resolving this issue, add a new pending_dbg field to x86_event
> (unioned with cr2 to avoid taking any extra space), and introduce
> {pv,hvm}_inject_debug_exn() helpers to replace the current callers using
> {pv,hvm}_inject_hw_exception().
> 
> A key property is that pending_dbg is taken with positive polarity to deal
> with RTM sensibly.  Most callers pass in a constant, but callers passing in a
> hardware %dr6 value need to xor the value with X86_DR6_DEFAULT to flip the
> polarity of RTM and reserved fields.
> 
> For PV guests, move the ad-hoc updating of %dr6 into pv_inject_event().  This
> in principle breaks the handing of RTM in do_debug(), but PV guests can't
> actually enable MSR_DEBUGCTL.RTM yet, so this doesn't matter in practice.
> 
> Signed-off-by: Andrew Cooper 
> Reviewed-by: Roger Pau Monné 
> Reviewed-by: Jan Beulich 
> [ jinoh: Rebase onto staging, forward declare struct vcpu ]
> Signed-off-by: Jinoh Kang 
> ---
> CC: Andrew Cooper 
> CC: Jan Beulich 
> CC: Wei Liu 
> CC: Roger Pau Monné 
> CC: Jun Nakajima 
> CC: Kevin Tian 
> CC: Tim Deegan 
> CC: Tamas K Lengyel 
> CC: Alexandru Isaila 
> CC: Petre Pircalabu 
> 
> v1 -> v2: [S-o-b fixes. More details below.]
> 
> - Update DR6 for gdbsx when trapped in PV guest kernel mode

I take it that this refers to ...

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1989,17 +1989,17 @@ void do_debug(struct cpu_user_regs *regs)
>  return;
>  }
>  
> -/* Save debug status register where guest OS can peek at it */
> -v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
> -v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
> -
>  if ( guest_kernel_mode(v, regs) && v->domain->debugger_attached )
>  {
> +/* Save debug status register where gdbsx can peek at it */
> +v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
> +v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
> +
>  domain_pause_for_debugger();
>  return;
>  }

... this code movement. I'm afraid this should have resulted in you
dropping the earlier R-b, and I'm further afraid I'm not convinced 
this is correct, despite seeing why you would want to do this. The
issue is that this way you also alter guest-visible state, when the
intention is that such now happen via ...

> -pv_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
> +pv_inject_debug_exn(dr6 ^ X86_DR6_DEFAULT);
>  }

... this call alone. I fear I can't currently see how to get both
aspects right, other than by breaking up 

Jan

Re: [PATCH v2 7/8] xen/ppc: Add stub function and symbol definitions

[Jan Beulich]

On 23.08.2023 22:07, Shawn Anastasio wrote:
> +/* irq.c */
> +
> +struct pirq *alloc_pirq_struct(struct domain *d)
> +{
> +BUG_ON("unimplemented");
> +}
> +
> +int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, int will_share)
> +{
> +BUG_ON("unimplemented");
> +}
> +
> +void pirq_guest_unbind(struct domain *d, struct pirq *pirq)
> +{
> +BUG_ON("unimplemented");
> +}
> +
> +void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask)
> +{
> +BUG_ON("unimplemented");
> +}
> +
> +static void ack_none(struct irq_desc *irq)
> +{
> +BUG_ON("unimplemented");
> +}
> +
> +hw_irq_controller no_irq_type = {
> +.typename = "none",
> +.startup = irq_startup_none,
> +.shutdown = irq_shutdown_none,
> +.enable = irq_enable_none,
> +.disable = irq_disable_none,
> +.ack = ack_none,
> +};

As said before, I think no new function should be introduced to fill any
of the hook pointers. I.e. I would suggest to also drop ack_none(). But
in the end it's your call of course. Either way
Acked-by: Jan Beulich 

Jan

Re: [XEN PATCH 03/13] xen/arm: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

On 30/08/23 15:01, Jan Beulich wrote:

On 30.08.2023 14:53, Simone Ballarin wrote:

On 29/08/23 00:10, Julien Grall wrote:

On Mon, 28 Aug 2023 at 09:20, Simone Ballarin 
wrote:

--- a/xen/arch/arm/include/asm/hypercall.h
+++ b/xen/arch/arm/include/asm/hypercall.h
@@ -1,10 +1,10 @@
+#ifndef __ASM_ARM_HYPERCALL_H__
+#define __ASM_ARM_HYPERCALL_H__
+
   #ifndef __XEN_HYPERCALL_H__
   #error "asm/hypercall.h should not be included directly - include
xen/hypercall.h instead"
   #endif

-#ifndef __ASM_ARM_HYPERCALL_H__
-#define __ASM_ARM_HYPERCALL_H__
-



I understand that you are trying to fix a misra violation. However, this
feels like it was done on purpose.

With the new change, you would not always check that the file were included
at the correct place. I am not against this change but this ought to be
explained.

I don't think the semantics have changed. Please correct me if I'm wrong.

With this change, the only situation where the check is not performed is
when __ASM_ARM_HYPERCALL_H__ is defined (i.e. the file has already been
successfully included). This implies that if __ASM_ARM_HYPERCALL_H__ is
defined, then __XEN_HYPERCALL_H__ is also defined, so the check would be
useless.

The same thing happened with the code before the change: if I include
the file after xen/hypercall.h, the check will always succeed.


Hmm, I think you're right, but I draw a different conclusion: The check
fails to work as intended. And this can only be repaired without your
adjustment.



Ok, I will just deviate these cases.


Jan


--
Simone Ballarin, M.Sc.

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

Re: [PATCH v2 6/8] xen/ppc: Define bug frames table in linker script

[Jan Beulich]

On 23.08.2023 22:07, Shawn Anastasio wrote:
> Define the bug frames table in ppc's linker script as is done by other
> architectures.
> 
> Signed-off-by: Shawn Anastasio 
> Acked-by: Jan Beulich 

If I'm not mistaken this change is independent of the earlier patches,
and hence could go in right away. Please confirm.

Jan

Re: [XEN PATCH 03/13] xen/arm: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 30.08.2023 14:53, Simone Ballarin wrote:
> On 29/08/23 00:10, Julien Grall wrote:
>> On Mon, 28 Aug 2023 at 09:20, Simone Ballarin 
>> wrote:
>>> --- a/xen/arch/arm/include/asm/hypercall.h
>>> +++ b/xen/arch/arm/include/asm/hypercall.h
>>> @@ -1,10 +1,10 @@
>>> +#ifndef __ASM_ARM_HYPERCALL_H__
>>> +#define __ASM_ARM_HYPERCALL_H__
>>> +
>>>   #ifndef __XEN_HYPERCALL_H__
>>>   #error "asm/hypercall.h should not be included directly - include
>>> xen/hypercall.h instead"
>>>   #endif
>>>
>>> -#ifndef __ASM_ARM_HYPERCALL_H__
>>> -#define __ASM_ARM_HYPERCALL_H__
>>> -
>>
>>
>> I understand that you are trying to fix a misra violation. However, this
>> feels like it was done on purpose.
>>
>> With the new change, you would not always check that the file were included
>> at the correct place. I am not against this change but this ought to be
>> explained.
> I don't think the semantics have changed. Please correct me if I'm wrong.
> 
> With this change, the only situation where the check is not performed is 
> when __ASM_ARM_HYPERCALL_H__ is defined (i.e. the file has already been 
> successfully included). This implies that if __ASM_ARM_HYPERCALL_H__ is 
> defined, then __XEN_HYPERCALL_H__ is also defined, so the check would be 
> useless.
> 
> The same thing happened with the code before the change: if I include 
> the file after xen/hypercall.h, the check will always succeed.

Hmm, I think you're right, but I draw a different conclusion: The check
fails to work as intended. And this can only be repaired without your
adjustment.

Jan

Re: [XEN PATCH 03/13] xen/arm: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

On 29/08/23 00:10, Julien Grall wrote:

Hi,

On Mon, 28 Aug 2023 at 09:20, Simone Ballarin 
wrote:


Add or move inclusion guards to address violations of
MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
to prevent the contents of a header file being included more than
once").

Inclusion guards must appear at the beginning of the headers
(comments are permitted anywhere).

Mechanical change.

Signed-off-by: Simone Ballarin 
---
  xen/arch/arm/efi/efi-boot.h  | 6 ++
  xen/arch/arm/include/asm/hypercall.h | 6 +++---
  xen/arch/arm/include/asm/iocap.h | 6 +++---
  3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 1c3640bb65..aba522ead5 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -3,6 +3,10 @@
   * is intended to be included by common/efi/boot.c _only_, and
   * therefore can define arch specific global variables.
   */
+
+#ifndef __ARM_EFI_EFI_BOOT_H__
+#define __ARM_EFI_EFI_BOOT_H__
+
  #include 
  #include 
  #include 
@@ -1003,6 +1007,8 @@ static void __init efi_arch_flush_dcache_area(const
void *vaddr, UINTN size)
  __flush_dcache_area(vaddr, size);
  }

+#endif /* __ARM_EFI_EFI_BOOT_H__*/
+
  /*
   * Local variables:
   * mode: C
diff --git a/xen/arch/arm/include/asm/hypercall.h
b/xen/arch/arm/include/asm/hypercall.h
index ccd26c5184..4f4d96f1f2 100644
--- a/xen/arch/arm/include/asm/hypercall.h
+++ b/xen/arch/arm/include/asm/hypercall.h
@@ -1,10 +1,10 @@
+#ifndef __ASM_ARM_HYPERCALL_H__
+#define __ASM_ARM_HYPERCALL_H__
+
  #ifndef __XEN_HYPERCALL_H__
  #error "asm/hypercall.h should not be included directly - include
xen/hypercall.h instead"
  #endif

-#ifndef __ASM_ARM_HYPERCALL_H__
-#define __ASM_ARM_HYPERCALL_H__
-



I understand that you are trying to fix a misra violation. However, this
feels like it was done on purpose.

With the new change, you would not always check that the file were included
at the correct place. I am not against this change but this ought to be
explained.

I don't think the semantics have changed. Please correct me if I'm wrong.

With this change, the only situation where the check is not performed is 
when __ASM_ARM_HYPERCALL_H__ is defined (i.e. the file has already been 
successfully included). This implies that if __ASM_ARM_HYPERCALL_H__ is 
defined, then __XEN_HYPERCALL_H__ is also defined, so the check would be 
useless.


The same thing happened with the code before the change: if I include 
the file after xen/hypercall.h, the check will always succeed.






  #include  /* for arch_do_domctl */

  long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
diff --git a/xen/arch/arm/include/asm/iocap.h
b/xen/arch/arm/include/asm/iocap.h
index 276fefbc59..4db1b16839 100644
--- a/xen/arch/arm/include/asm/iocap.h
+++ b/xen/arch/arm/include/asm/iocap.h
@@ -1,10 +1,10 @@
-#ifndef __X86_IOCAP_H__
-#define __X86_IOCAP_H__
+#ifndef __ASM_ARM_IOCAP_H__
+#define __ASM_ARM_IOCAP_H__

  #define cache_flush_permitted(d)\
  (!rangeset_is_empty((d)->iomem_caps))

-#endif
+#endif /* __ASM_ARM_IOCAP_H__ */



I don’t understand how this is related to the rest of the patch. You wrote
that inclusion must appear first and this is the case here.

However the name is technically not correct. Is this really related to
directive 4.10? If so, this should be clarified in the commit message. If
not, then I think this should be in a separate commit.



Yes, you are right. This is not correlated to this series. I will put it 
on a separate commit.



Cheers,




  /*
   * Local variables:
--
2.34.1






--
Simone Ballarin, M.Sc.

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

[libvirt test] 182562: tolerable all pass - PUSHED

[osstest service owner]

flight 182562 libvirt real [real]
http://logs.test-lab.xenproject.org/osstest/logs/182562/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 182550
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 182550
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 182550
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  15 migrate-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-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt 16 saverestore-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-arm64-arm64-libvirt-qcow2 15 saverestore-support-checkfail never pass
 test-amd64-i386-libvirt-raw  14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail   never pass

version targeted for testing:
 libvirt  120724bc6d1c2f8c029e67bd91335b48c243404c
baseline version:
 libvirt  d40c6cad64fda4767ea7b2567d129eeac8557d6f

Last test of basis   182550  2023-08-29 04:20:35 Z1 days
Testing same since   182562  2023-08-30 04:18:48 Z0 days1 attempts


People who touched revisions under test:
  Daniel P. Berrangé 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-arm64  pass
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-arm64-libvirt  pass
 build-armhf-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-arm64-pvopspass
 build-armhf-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm   pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass
 test-amd64-amd64-libvirt-xsm pass
 test-arm64-arm64-libvirt-xsm pass
 test-amd64-i386-libvirt-xsm  pass
 test-amd64-amd64-libvirt pass
 test-arm64-arm64-libvirt pass
 test-armhf-armhf-libvirt pass
 test-amd64-i386-libvirt  pass
 test-amd64-amd64-libvirt-pairpass
 test-amd64-i386-libvirt-pair pass
 test-arm64-arm64-libvirt-qcow2   pass
 test-armhf-armhf-libvirt-qcow2   pass
 test-arm64-arm64-libvirt-raw pass
 test-armhf-armhf-libvirt-raw pass
 test-amd64-i386-libvirt-raw  pass
 test-amd64-amd64-libvirt-vhd pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is

[ovmf test] 182567: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf a481c1114474160f53b1662fd3726b48c88ae82e
baseline version:
 ovmf 5f46eb2307dd6d4ea163b6899ded81e795780059

Last test of basis   182564  2023-08-30 06:40:43 Z0 days
Testing same since   182567  2023-08-30 10:12:15 Z0 days1 attempts


People who touched revisions under test:
  Nhi Pham 

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
   5f46eb2307..a481c11144  a481c1114474160f53b1662fd3726b48c88ae82e -> 
xen-tested-master

[xen-unstable-smoke test] 182565: tolerable all pass - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  e5522c71beaa83f2f5d2118724ace9f90c22e583
baseline version:
 xen  8c01f267eff3d6c1ea04273e9885bf6d2fda1c44

Last test of basis   182556  2023-08-29 15:00:28 Z0 days
Testing same since   182565  2023-08-30 09:02:08 Z0 days1 attempts


People who touched revisions under test:
  Roger Pau Monné 
  Stewart Hildebrand 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-amd64-libvirt pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   8c01f267ef..e5522c71be  e5522c71beaa83f2f5d2118724ace9f90c22e583 -> smoke

Re: [XEN PATCH 02/13] automation/eclair: add text-based deviation for empty headers

[Simone Ballarin]

On 29/08/23 08:35, Jan Beulich wrote:

On 28.08.2023 15:19, Simone Ballarin wrote:

--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -80,6 +80,7 @@ inline functions."
  
  -doc_begin="This header file is autogenerated or empty, therefore it poses no

  risk if included more than once."


While unrelated to, the change at hand, I still have a question on this:
How come it is deemed universally safe to multi-include generated headers.
I would have said that whether that's safe depends on the nature of the
generated code in the header. Only truly empty ones are uniformly safe to
include any number of times.


Yes, I agree with you. The mere fact that a file is auto-generated does 
not imply anything, moreover, this deviation is not even reported in 
rule.rst. In the next series, I'll drop it.




Jan


+-config=MC3R1.D4.10,reports+={safe, "first_area(text(^/\\* empty \\*/$, 
begin-1))"}
  -file_tag+={empty_header, "^xen/arch/arm/efi/runtime\\.h$"}
  -file_tag+={autogen_headers, 
"^xen/include/xen/compile\\.h$||^xen/include/generated/autoconf.h$||^xen/include/xen/hypercall-defs.h$"}
  -config=MC3R1.D4.10,reports+={safe, 
"all_area(all_loc(file(empty_header||autogen_headers)))"}




--
Simone Ballarin, M.Sc.

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

Re: [PATCH v2 5/8] xen/ppc: Define minimal stub headers required for full build

[Jan Beulich]

On 23.08.2023 22:07, Shawn Anastasio wrote:
> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/altp2m.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __ASM_PPC_ALTP2M_H__
> +#define __ASM_PPC_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 PPC. */
> +return false;
> +}
> +
> +/* Alternate p2m VCPU */
> +static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v)
> +{
> +/* Not implemented on PPC, should not be reached. */
> +BUG_ON("unimplemented");

I would have thought this construct is meant to flag places that need
work in the course of bringing up Xen on PPC. This isn't one of those,
I think. Perhaps ASSERT_UNREACHABLE() is slightly better here than
Arm's BUG()?

> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/current.h
> @@ -0,0 +1,42 @@
> +#ifndef __ASM_PPC_CURRENT_H__
> +#define __ASM_PPC_CURRENT_H__
> +
> +#include 
> +
> +#ifndef __ASSEMBLY__
> +
> +struct vcpu;
> +
> +/* Which VCPU is "current" on this PCPU. */
> +DECLARE_PER_CPU(struct vcpu *, curr_vcpu);
> +
> +#define current(this_cpu(curr_vcpu))
> +#define set_current(vcpu)  do { current = (vcpu); } while (0)
> +#define get_cpu_current(cpu)  (per_cpu(curr_vcpu, cpu))
> +
> +/* Per-VCPU state that lives at the top of the stack */
> +struct cpu_info {
> +struct cpu_user_regs guest_cpu_user_regs;
> +unsigned long elr;
> +uint32_t flags;

May I suggest that you pick one of fixed-width types or basic C types
for consistent use here?

> +};
> +
> +static inline struct cpu_info *get_cpu_info(void)
> +{
> +#ifdef __clang__
> +unsigned long sp;
> +
> +asm ("mr %0, 1" : "=r" (sp));

Nit: Style.

> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/device.h
> @@ -0,0 +1,53 @@
> +#ifndef __ASM_PPC_DEVICE_H__
> +#define __ASM_PPC_DEVICE_H__
> +
> +enum device_type
> +{
> +DEV_DT,
> +DEV_PCI,
> +};
> +
> +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,
> +DEVICE_PCI_HOSTBRIDGE,
> +/* Use for error */
> +DEVICE_UNKNOWN,
> +};
> +
> +struct device_desc {
> +/* Device name */
> +const char *name;
> +/* Device class */
> +enum device_class class;
> +/* List of devices supported by this driver */
> +const struct dt_device_match *dt_match;
> +/*
> + * Device initialization.
> + *
> + * -EAGAIN is used to indicate that device probing is deferred.
> + */
> +int (*init)(struct dt_device_node *dev, const void *data);
> +};
> +
> +typedef struct device device_t;
> +
> +#define DT_DEVICE_START(_name, _namestr, _class)\
> +static const struct device_desc __dev_desc_##_name __used   \
> +__section(".dev.info") = {  \
> +.name = _namestr,   \
> +.class = _class,\
> +
> +#define DT_DEVICE_END   \
> +};
> +
> +#endif /* __ASM_PPC_DEVICE_H__ */

Do you really need everything you put in here?

> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/div64.h
> @@ -0,0 +1,14 @@
> +#ifndef __ASM_PPC_DIV64_H__
> +#define __ASM_PPC_DIV64_H__
> +
> +#include 
> +
> +#define do_div(n,base) ({   \
> +uint32_t __base = (base);   \
> +uint32_t __rem; \
> +__rem = ((uint64_t)(n)) % __base;   \
> +(n) = ((uint64_t)(n)) / __base; \
> +__rem;  \
> +})

I understand you're merely copying this from elsewhere, but it would be
really nice if style could be corrected for such new instances (no
leading underscores, blank after comma, and ideally also no excess
parentheses).

> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/domain.h
> @@ -0,0 +1,46 @@
> +#ifndef __ASM_PPC_DOMAIN_H__
> +#define __ASM_PPC_DOMAIN_H__
> +
> +#include 
> +#include 
> +
> +struct hvm_domain
> +{
> +uint64_t  params[HVM_NR_PARAMS];
> +};
> +
> +#define is_domain_direct_mapped(d) ((void)(d), 0)
> +
> +/* TODO: Implement */
> +#define guest_mode(r) ({ (void) (r); BUG_ON("unimplemented"); 0; })

Nit: Stray blank after cast (more instances of this pattern elsewhere).

> diff --git a/xen/arch/ppc/include/asm/grant_table.h 
> b/xen/arch/ppc/include/asm/grant_table.h
> new file mode 100644
> index 00..e69de29bb2

Instead of introducing a completely empty file, perhaps better to put
one in that at least has the usual header guard?

> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/guest_access.h
> @@ -0,0 +1,54 @@
> +#ifndef __ASM_PPC_GUEST_ACCESS_H__
> +#defi

Re: [XEN PATCH 02/13] automation/eclair: add text-based deviation for empty headers

[Simone Ballarin]

On 29/08/23 00:00, Stefano Stabellini wrote:

On Mon, 28 Aug 2023, Simone Ballarin wrote:

This patch adds a text-based deviation for Directive 4.10:
"Precautions shall be taken in order to prevent the contents of
a header file being included more than once"

Headers starting with the following comment are not supposed to
comply with the directive:
"/* empty */"

These headers should be empty, therefore they pose no risk if included
more than once.

Signed-off-by: Simone Ballarin 


Acked-by: Stefano Stabellini 

However I think we should also update rules.rst and/or update
docs/misra/safe.json


I will do it in the next submission.




---
  automation/eclair_analysis/ECLAIR/deviations.ecl | 1 +
  1 file changed, 1 insertion(+)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
b/automation/eclair_analysis/ECLAIR/deviations.ecl
index 5f068377fa..2681a4cff5 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -80,6 +80,7 @@ inline functions."
  
  -doc_begin="This header file is autogenerated or empty, therefore it poses no

  risk if included more than once."
+-config=MC3R1.D4.10,reports+={safe, "first_area(text(^/\\* empty \\*/$, 
begin-1))"}
  -file_tag+={empty_header, "^xen/arch/arm/efi/runtime\\.h$"}
  -file_tag+={autogen_headers, 
"^xen/include/xen/compile\\.h$||^xen/include/generated/autoconf.h$||^xen/include/xen/hypercall-defs.h$"}
  -config=MC3R1.D4.10,reports+={safe, 
"all_area(all_loc(file(empty_header||autogen_headers)))"}
--
2.34.1





--
Simone Ballarin, M.Sc.

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

[ovmf test] 182564: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf 5f46eb2307dd6d4ea163b6899ded81e795780059
baseline version:
 ovmf 9896a9c61836a5afba72c47d7c64f4e24f0805ba

Last test of basis   182560  2023-08-30 00:13:12 Z0 days
Testing same since   182564  2023-08-30 06:40:43 Z0 days1 attempts


People who touched revisions under test:
  Nhi Pham 

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
   9896a9c618..5f46eb2307  5f46eb2307dd6d4ea163b6899ded81e795780059 -> 
xen-tested-master

[linux-linus test] 182559: regressions - FAIL

[osstest service owner]

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

Regressions :-(

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

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

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 182531
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 182531
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 182531
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-x

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

[Michal Orzel]

On 30/08/2023 00:45, Stefano Stabellini wrote:
> 
> 
> On Tue, 29 Aug 2023, Michal Orzel wrote:
>> On 25/08/2023 10:02, Vikram Garhwal wrote:
>>> Add remove_device callback for removing the device entry from smmu-master 
>>> using
>>> following steps:
>>> 1. Find if SMMU master exists for the device node.
>>> 2. Check if device is currently in use.
>> Since you removed a call to iommu_dt_device_is_assigned_locked(), you do not 
>> check it from SMMU, right?
>> You are relying on a check done in iommu_remove_dt_device().
>> This wants to be mentioned. However, Julien suggested to do the check for 
>> internal SMMU state.
>> Looking at the code, when the device is assigned, we do:
>> dev_iommu_domain(dev) = domain;
>> and when de-assigned:
>> dev_iommu_domain(dev) = NULL;
>>
>> This means that before calling remove_smmu_master() you could do:
>>
>> /* Make sure device is not assigned */
>> if (dev_iommu_domain(dev))
>> return -EBUSY;
>>
>> @Julien, @Stefano?
> 
> I think it is OK without it, as we have a call to
> iommu_dt_device_is_assigned_locked(np) already in
> iommu_remove_dt_device?

Yes, but this would add an extra layer of protection by checking for IOMMU 
domain for this device.

~Michal

Re: 4.17 backports

[Jan Beulich]

On 22.08.2023 21:25, Andrew Cooper wrote:
> Looking at the patchqueue, the following should be considered for backport:
> 
> 19c6cbd90965 xen/vcpu: ignore VCPU_SSHOTTMR_future

I will admit I'm a little hesitant here, for being an ABI change (even
if to the better), but I guess I'll still include it.

> 0946068e7fae x86/head: check base address alignment
> eaa324bfebcf x86/trampoline: load the GDT located in the trampoline page

Reading its description again, this was done merely for consistency. It
doesn't read as if it fixed anything.

> aab4b38b5d77 xenalyze: Handle start-of-day ->RUNNING transitions
> c81b287e00b1 xenalyze: Basic TRC_HVM_EMUL handling

While the former of these two clearly is a bug fix, the latter looks to
be more like functionality addition. IOW I'm not convinced here.

> 813da5f0e73b x86/ioapic: sanitize IO-APIC pins before enabling lapic
> LVTERR/ESR
> cdc48cb5a74b x86/ioapic: add a raw field to RTE struct
> ef7995ed1bcd x86/ioapic: RTE modifications must use ioapic_write_entry
> a478b38c01b6 iommu/vtd: rename io_apic_read_remap_rte() local variable
> 3e033172b025 x86/iommu: pass full IO-APIC RTE for remapping table update

These I really merely didn't want to take right away. Now that they've
been there for a month, I'll include them.

Jan

Re: [XEN PATCH 01/13] misra: add deviation for headers that explicitly avoid guards

[Simone Ballarin]

On 29/08/23 00:32, Stefano Stabellini wrote:

On Mon, 28 Aug 2023, Stefano Stabellini wrote:

On Mon, 28 Aug 2023, Simone Ballarin wrote:

Some headers, under specific circumstances (documented in a comment at
the beginning of the file), explicitly avoid inclusion guards: the caller
is responsible for including them correctly.

These files are not supposed to comply with Directive 4.10:
"Precautions shall be taken in order to prevent the contents of a header
file being included more than once"

This patch adds a deviation for all headers that contain the following
in a comment text:
"In this case, no inclusion guards apply and the caller is responsible"

Signed-off-by: Simone Ballarin 


Acked-by: Stefano Stabellini 


Actually one question



---
  automation/eclair_analysis/ECLAIR/deviations.ecl | 4 
  docs/misra/rules.rst | 3 ++-
  2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
b/automation/eclair_analysis/ECLAIR/deviations.ecl
index d8170106b4..5f068377fa 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -91,6 +91,10 @@ conform to the directive."
  -config=MC3R1.D4.10,reports+={safe, "first_area(text(^/\\* Generated file, do not 
edit! \\*/$, begin-3))"}
  -doc_end
  
+-doc_begin="Some headers, under specific circumstances, explicitly avoid inclusion guards."

+-config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no 
inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"}
+-doc_end


Is this supposed to match with any files starting with "In this case,
no inclusion..." ?

We should use the format introduced by safe.json instead



I agree, I will do it in the next submission.

--
Simone Ballarin, M.Sc.

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

Re: [XEN PATCH 01/13] misra: add deviation for headers that explicitly avoid guards

[Simone Ballarin]

On 29/08/23 08:33, Jan Beulich wrote:

On 28.08.2023 15:19, Simone Ballarin wrote:

--- a/docs/misra/rules.rst
+++ b/docs/misra/rules.rst
@@ -60,7 +60,8 @@ maintainers if you want to suggest a change.
   - Precautions shall be taken in order to prevent the contents of a
 header file being included more than once
   - Files that are intended to be included more than once do not need to
-   conform to the directive
+   conform to the directive. Files that explicitly avoid inclusion guards
+   under specific circumstances do not need to conform the directive.


Nit: There's a "to" missing near the end of the added sentence. Can likely
be taken care of while committing, since Stefano has already ack-ed this,
but I'd like to still raise the question of the utility of this statement:
How is one to know whether omission of guards is intentional?


As suggested by Stefano, this kind of deviation should be done using
the format specified in safe.json, and I will follow his suggestion in 
the next submission. The statement will disappear.




Jan





--
Simone Ballarin, M.Sc.

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

[PATCH] MAINTAINERS: consolidate vm-event/monitor entry

[Jan Beulich]

If the F: description is to be trusted, the two xen/arch/x86/hvm/
lines were fully redundant with the earlier wildcard ones. Arch header
files, otoh, were no longer covered by anything as of the move from
include/asm-*/ to arch/*/include/asm/. Finally also generalize (by
folding) the x86- and Arm-specific mem_access.c entries.

Signed-off-by: Jan Beulich 
---
Triggered by me looking at the entry in the context of Oleksii's RISC-V
preparatory patch.

We could go further, to simply
F:  xen/*/mem_access.[ch]
F:  xen/*/monitor.[ch]
F:  xen/*/vm_event.[ch]
Thoughts anyone?

--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -558,14 +558,9 @@ R: Alexandru Isaila 
 S: Supported
 F: tools/misc/xen-access.c
-F: xen/arch/*/monitor.c
-F: xen/arch/*/vm_event.c
-F: xen/arch/arm/mem_access.c
-F: xen/arch/x86/include/asm/hvm/monitor.h
-F: xen/arch/x86/include/asm/hvm/vm_event.h
-F: xen/arch/x86/mm/mem_access.c
-F: xen/arch/x86/hvm/monitor.c
-F: xen/arch/x86/hvm/vm_event.c
+F: xen/arch/*/mem_access.[ch]
+F: xen/arch/*/monitor.[ch]
+F: xen/arch/*/vm_event.[ch]
 F: xen/common/mem_access.c
 F: xen/common/monitor.c
 F: xen/common/vm_event.c

Re: [PATCH] docs/misra: add 14.3 and 14.4

[Jan Beulich]

On 30.08.2023 10:00, Bertrand Marquis wrote:
> Hi,
> 
>> On 30 Aug 2023, at 09:58, Jan Beulich  wrote:
>>
>> On 30.08.2023 09:54, Bertrand Marquis wrote:
 On 30 Aug 2023, at 02:59, Stefano Stabellini  
 wrote:
 +   * - `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 of integers, pointers, and chars to boolean
 +   are allowed
>>>
>>> I am a bit wondering here what is remaining after this deviation.
>>
>> Hmm, good point - floating point (and alike) types, which we don't use 
>> anyway.
> 
> So we accept the rule but we deviate all cases that would apply.
> I do not think we should do that.

Right; Stefano - I withdraw my offer to ack a 14.4-only patch.

Jan

Re: [PATCH] docs/misra: add 14.3 and 14.4

[Bertrand Marquis]

Hi,

> On 30 Aug 2023, at 09:58, Jan Beulich  wrote:
> 
> On 30.08.2023 09:54, Bertrand Marquis wrote:
>>> On 30 Aug 2023, at 02:59, Stefano Stabellini  wrote:
>>> +   * - `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 of integers, pointers, and chars to boolean
>>> +   are allowed
>> 
>> I am a bit wondering here what is remaining after this deviation.
> 
> Hmm, good point - floating point (and alike) types, which we don't use anyway.

So we accept the rule but we deviate all cases that would apply.
I do not think we should do that.

Bertrand

> 
> Jan
> 

Re: [PATCH] docs/misra: add 14.3 and 14.4

[Jan Beulich]

On 30.08.2023 09:54, Bertrand Marquis wrote:
>> On 30 Aug 2023, at 02:59, Stefano Stabellini  wrote:
>> +   * - `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 of integers, pointers, and chars to boolean
>> +   are allowed
> 
> I am a bit wondering here what is remaining after this deviation.

Hmm, good point - floating point (and alike) types, which we don't use anyway.

Jan

Re: [PATCH v2 1/2] xen: add stubs dir to include path

[Bertrand Marquis]

Hi Jan,

> On 30 Aug 2023, at 09:30, Jan Beulich  wrote:
> 
> On 29.08.2023 16:34, Oleksii Kurochko wrote:
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -438,6 +438,7 @@ ifdef building_out_of_srctree
>> endif
>> CFLAGS += -I$(srctree)/include
>> CFLAGS += -I$(srctree)/arch/$(SRCARCH)/include
>> +CFLAGS += -I$(srctree)/include/stubs
>> 
>> # Note that link order matters!
>> ALL_OBJS-y:= common/built_in.o
> 
> I have to admit that I'm not entirely happy with the name. I wonder what
> other REST maintainers think. In particular I would consider using
> Linux'es "asm-generic" name instead, allowing to cover both stub headers
> like the one to be introduced here, but also other kinds of "fallback"
> ones.
> 

This is a great idea.
+1 on asm-generic

Cheers
Bertrand

> Jan
> 

Re: [PATCH] docs/misra: add 14.3 and 14.4

[Bertrand Marquis]

Hi Stefano,

> On 30 Aug 2023, at 02:59, Stefano Stabellini  wrote:
> 
> From: Stefano Stabellini 
> 
> Add 14.3, with a project-wide deviations on if statements.
> Add 14.4, clarifying that implicit conversions of integers, chars and
> pointers to bool are allowed.
> 
> Also take the opportunity to clarify that parameters of function pointer
> types are expected to have names (Rule 8.2).
> 
> Signed-off-by: Stefano Stabellini 
> ---
> docs/misra/rules.rst | 20 +++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> index db30632b93..6cde4feeae 100644
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -234,7 +234,7 @@ maintainers if you want to suggest a change.
>* - `Rule 8.2 
> `_
>  - Required
>  - Function types shall be in prototype form with named parameters
> - -
> + - Function pointer types shall have named parameters too.


I would just modify to Function and Function pointers types shall be ...

> 
>* - `Rule 8.3 
> `_
>  - Required
> @@ -332,6 +332,24 @@ maintainers if you want to suggest a change.
>  - A loop counter shall not have essentially floating type
>  -
> 
> +   * - `Rule 14.3 
> `_
> + - Required
> + - Controlling expressions shall not be invariant
> + - Due to the extensive usage of IS_ENABLED, sizeof compile-time
> +   checks, and other constructs that are detected as errors by MISRA
> +   C scanners, managing the configuration of a MISRA C scanner for
> +   this rule would be unmanageable. Thus, this rule is adopted with
> +   a project-wide deviation on 'if' statements. The rule only
> +   applies to while, for, do ... while, ?:, and switch statements.

Didn't we also said that we would accept while(0) and while(1) ?
Also i agree with Jan, ? is really the same as if so we should not treat it 
differently.

> +
> +   * - `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 of integers, pointers, and chars to boolean
> +   are allowed

I am a bit wondering here what is remaining after this deviation.

> +
>* - `Rule 16.7 
> `_
>  - Required
>  - A switch-expression shall not have essentially Boolean type
> -- 
> 2.25.1
> 

Cheers
Bertrand

Re: [PATCH v2] docs/misra: add rules 10.1 10.2 10.3 10.4

[Bertrand Marquis]

Hi Stefano,

> On 25 Aug 2023, at 23:48, Stefano Stabellini  wrote:
> 
> From: Stefano Stabellini 
> 
> 10.1 with several caveats, described in the notes.
> 10.3 and 10.4 as "aspirational" guidelines, as clarified in the notes.
> 
> Signed-off-by: Stefano Stabellini 

Reviewed-by: Bertrand Marquis 

Cheers
Bertrand

> ---
> v2:
> - typo fix
> - Implicit conversions to boolean for conditionals and logical operators
> - make -C xen
> ---
> docs/misra/rules.rst | 53 
> 1 file changed, 53 insertions(+)
> 
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> index db30632b93..34916e266a 100644
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -310,6 +310,59 @@ maintainers if you want to suggest a change.
>  - An element of an object shall not be initialized more than once
>  -
> 
> +   * - `Rule 10.1 
> `_
> + - Required
> + - Operands shall not be of an inappropriate essential type
> + - The following are allowed:
> + - Value-preserving conversions of integer constants
> + - Bitwise and, or, xor, one's complement, bitwise and assignment,
> +   bitwise or assignment, bitwise xor assignment (bitwise and, or, 
> xor
> +   are safe on non-negative integers; also Xen assumes two's 
> complement
> +   representation)
> + - Left shift, right shift, left shift assignment, right shift
> +   assignment (see C-language-toolchain.rst for uses of
> +   compilers' extensions)
> + - Implicit conversions to boolean for conditionals (?: if while
> +   for) and logical operators (! || &&)
> +
> +   * - `Rule 10.2 
> `_
> + - Required
> + - Expressions of essentially character type shall not be used
> +   inappropriately in addition and subtraction operations
> + -
> +
> +   * - `Rule 10.3 
> `_
> + - Required
> + - The value of an expression shall not be assigned to an object
> +   with a narrower essential type or of a different essential type
> +   category
> + - Please beware that this rule has many violations in the Xen
> +   codebase today, and its adoption is aspirational. However, when
> +   submitting new patches please try to decrease the number of
> +   violations when possible.
> +
> +   gcc has a helpful warning that can help you spot and remove
> +   violations of this kind: conversion. For instance, you can use
> +   it as follows:
> +
> +   CFLAGS="-Wconversion -Wno-error=sign-conversion 
> -Wno-error=conversion" make -C xen
> +
> +   * - `Rule 10.4 
> `_
> + - Required
> + - Both operands of an operator in which the usual arithmetic
> +   conversions are performed shall have the same essential type
> +   category
> + - Please beware that this rule has many violations in the Xen
> +   codebase today, and its adoption is aspirational. However, when
> +   submitting new patches please try to decrease the number of
> +   violations when possible.
> +
> +   gcc has a helpful warning that can help you spot and remove
> +   violations of this kind: arith-conversion. For instance, you
> +   can use it as follows:
> +
> +   CFLAGS="-Warith-conversion -Wno-error=arith-conversion" make -C xen
> +
>* - `Rule 12.5 
> `_
>  - Mandatory
>  - The sizeof operator shall not have an operand which is a function
> -- 
> 2.25.1
> 

Re: [PATCH] docs/misra: add 14.3 and 14.4

[Jan Beulich]

On 30.08.2023 02:59, Stefano Stabellini wrote:
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -234,7 +234,7 @@ maintainers if you want to suggest a change.
> * - `Rule 8.2 
> `_
>   - Required
>   - Function types shall be in prototype form with named parameters
> - -
> + - Function pointer types shall have named parameters too.

This isn't an exception; do we really need to state such? I would have
expected something to appear here only if we intended to deviate certain
constructs.

> @@ -332,6 +332,24 @@ maintainers if you want to suggest a change.
>   - A loop counter shall not have essentially floating type
>   -
>  
> +   * - `Rule 14.3 
> `_
> + - Required
> + - Controlling expressions shall not be invariant
> + - Due to the extensive usage of IS_ENABLED, sizeof compile-time
> +   checks, and other constructs that are detected as errors by MISRA
> +   C scanners, managing the configuration of a MISRA C scanner for
> +   this rule would be unmanageable. Thus, this rule is adopted with
> +   a project-wide deviation on 'if' statements. The rule only
> +   applies to while, for, do ... while, ?:, and switch statements.

The sizeof() aspect mentioned particularly applies to switch() as well.
Furthermore ?: is really only shorthand for simple if(), so I don't see
treating it different from if() as helpful.

That said, I'd be a little hesitant to give an ack here anyway. If you'd
split 14.3 and 14.4, I'd be happy to ack 14.4's addition.

Jan

> +   * - `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 of integers, pointers, and chars to boolean
> +   are allowed
> +
> * - `Rule 16.7 
> `_
>   - Required
>   - A switch-expression shall not have essentially Boolean type

Re: [PATCH v9 12/16] vpci: add initial support for virtual PCI bus topology

[Jan Beulich]

On 30.08.2023 01:19, Volodymyr Babchuk wrote:
> From: Oleksandr Andrushchenko 
> 
> Assign SBDF to the PCI devices being passed through with bus 0.
> The resulting topology is where PCIe devices reside on the bus 0 of the
> root complex itself (embedded endpoints).
> This implementation is limited to 32 devices which are allowed on
> a single PCI bus.
> 
> Please note, that at the moment only function 0 of a multifunction
> device can be passed through.
> 
> Signed-off-by: Oleksandr Andrushchenko 
> ---
> Since v9:
> - Lock in add_virtual_device() replaced with ASSERT (thanks, Stewart)

Also peeking at a few other patches where similar change remarks exist,
I'm slightly confused by them: Is this submission v9 or v10?

Jan

Re: [PATCH v2 1/2] xen: add stubs dir to include path

[Jan Beulich]

On 29.08.2023 16:34, Oleksii Kurochko wrote:
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -438,6 +438,7 @@ ifdef building_out_of_srctree
>  endif
>  CFLAGS += -I$(srctree)/include
>  CFLAGS += -I$(srctree)/arch/$(SRCARCH)/include
> +CFLAGS += -I$(srctree)/include/stubs
>  
>  # Note that link order matters!
>  ALL_OBJS-y:= common/built_in.o

I have to admit that I'm not entirely happy with the name. I wonder what
other REST maintainers think. In particular I would consider using
Linux'es "asm-generic" name instead, allowing to cover both stub headers
like the one to be introduced here, but also other kinds of "fallback"
ones.

Jan

Re: [XEN PATCH] x86/ACPI: Ignore entries with invalid APIC IDs when parsing MADT

[Jan Beulich]

On 30.08.2023 00:54, Thomas Gleixner wrote:
> On Tue, Aug 29 2023 at 16:25, Roger Pau Monné wrote:
>> On Sun, Aug 27, 2023 at 05:44:15PM +0200, Thomas Gleixner wrote:
>>> The APIC/X2APIC description of MADT specifies flags:
>>>
>>> Enabled If this bit is set the processor is ready for use. If
>>> this bit is clear and the Online Capable bit is set,
>>> system hardware supports enabling this processor during
>>> OS runtime. If this bit is clear and the Online Capable
>>> bit is also clear, this processor is unusable, and OSPM
>>> shall ignore the contents of the Processor Local APIC
>>> Structure.
>>>
>>> Online Capable  The information conveyed by this bit depends on the
>>> value of the Enabled bit. If the Enabled bit is set,
>>> this bit is reserved and must be zero. Otherwise, if
>>> this this bit is set, system hardware supports enabling
>>> this processor during OS runtime.
>>
>> Sadly this flag is only present starting with MADT v5.
> 
> Correct. The difference between pre v5 MADT and v5+ is that the latter
> allows the OS to make more informed decisions, but the lack of this flag
> does not make the claim that randomly assigning APIC IDs after the
> initial parsing is a valid approach any more correct. Why?
> 
> Simply because the other relationships vs. processor UIDs and SRAT/SLIT
> are not magically going away due to the lack of that flag.
> 
>>> Otherwise you'd end up with a CPU hotplugged which is outside of the
>>> resource space allocated during init.
>>
>> It's my understating that ACPI UIDs 0xff and 0xfff for local ACPI
>> and x2APIC structures respectively are invalid, as that's the
>> broadcast value used by the local (x2)APIC NMI Structures.
> 
> Correct. These IDs are invalid independent of any flag value. 

What we apparently agree on is these special UID values to be invalid,
and that UIDs can't change. But that's not the same for the APIC IDs;
see below. (As a side note, Xen range-checks UID against its
implementation limit MAX_MADT_ENTRIES, so it's more than just the
all-ones values which we'd reject. Not really correct, I know. Looks
like Linux has done away with the simple x86_acpiid_to_apicid[]
translation mechanism. This being a statically sized array requires
this restriction in Xen, for the time being.)

>> I think Jan's point (if I understood correctly) is that Processor or
>> Device objects can have a _MAT method that returns updated MADT
>> entries, and some ACPI implementations might modify the original
>> entries on the MADT and return them from that method when CPU
>> hotplug takes place.

Just to mention it: It's not just "might". I've seen DSDT code doing
so on more than one occasion.

>>  The spec notes that "OSPM does not expect the
>> information provided in this table to be updated if the processor
>> information changes during the lifespan of an OS boot." so that the
>> MADT doesn't need to be updated when CPU hotplug happens, but I don't
>> see that sentence as preventing the MADT to be updated if CPU hotplug
>> takes place, it's just not required.
> 
> Right. But if you read carefully what I wrote then you will figure out
> that any randomly made up APIC ID post MADT enumeration cannot work.

I just went back and re-read your earlier reply. I can't see such
following from what you wrote.

>> I don't see anywhere in the spec that states that APIC IDs 0xff and
>> 0x are invalid and entries using those IDs should be ignored,
>> but I do think that any system that supports CPU hotplug better has
>> those IDs defined since boot.  Also it seems vendors have started
>> relying on using 0xff and 0x APIC IDs to signal non-present
>> CPUs, and Linux has been ignoring such entries for quite some time
>> already  without reported issues.
> 
> There is no requirement for the ACPI spec to state this simply because
> these APIC IDs are invalid to address a processor at the architectural
> level. ACPI does not care about architectural restrictions unless really
> required, e.g. like the LAPIC vs. X2APIC exclusiveness.

Correct, active processors can't use such APIC IDs. But placeholder
MADT entries can, so long as valid APIC IDs are put in place by
firmware (and announced via _MAT) at the time a socket is newly
populated. They'll be uniquely identified by their UID, so the OS very
well knows which originally parsed (from MADT) entries are affected.

As stated before, unless putting in place extra restrictions, I can't
even see how firmware would be able to up front determine APIC IDs for
unpopulated sockets: It simply can't know the topology of a package
that's not there yet. Requiring all packages to have identical
topology might be a restriction OSes put in place, but I'd be inclined
to call firmware buggy if it did (short of me being aware of there
being anything in the spec putting in place such a restriction).

Jan