[linux-linus test] 183518: tolerable trouble: fail/pass/starved - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

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

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

version targeted for testing:
 linux4f82870119a46b0d04d91ef4697ac4977a255a9d
baseline version:
 linuxd88520ad73b79e71e3ddf08de335b8520ae41c5c

Last test of basis   183510  2023-10-24 11:41:38 Z0 days
Testing same since   183518  2023-10-24 20:09:53 Z0 days1 attempts


People who touched revisions under test:
  Alexandre Ghiti 
  Andrew Morton 
  Arnd Bergmann 
  Bartosz Golaszewski 
  Gregory Price 
  Gregory Price 
  Haibo Li 
  Johannes Weiner 
  Kees Cook 
  Kemeng Shi 
  Liam R. Howlett 
  Linus Torvalds 
  Mike Kravetz 
  Muhammad Usama Anjum 
  Naoya Horiguchi 
  Nhat Pham 
  Oleksij Rempel 
  Ondrej Jirman 
  Palmer Dabbelt 
  Rik van Riel 
  Samasth Norway Ananda 
  Sebastian Ott 

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

[Stefano Stabellini]

On Tue, 24 Oct 2023, Jan Beulich wrote:
> On 24.10.2023 01:31, Stefano Stabellini wrote:> --- a/docs/misra/rules.rst
> > +++ b/docs/misra/rules.rst
> > @@ -422,6 +422,13 @@ maintainers if you want to suggest a change.
> >  
> > while(0) and while(1) and alike are allowed.
> >  
> > +   * - `Rule 14.4 
> > `_
> > + - Required
> > + - The controlling expression of an if statement and the controlling
> > +   expression of an iteration-statement shall have essentially
> > +   Boolean type
> > + - Implicit conversions to boolean are allowed
> 
> What, if anything, remains of this rule with this exception?

Not much, but there is a difference between following a rule with a
deviation (in this case we allow implicit conversions of integers and
pointers to bool because we claim all Xen developers know how they work)
and not follow the rule at all, at least for the assessors. Also,
anything that doesn't implicitly convert to a boolean is not allowed,
although I guess probably it wouldn't compile either.

We could also try to find a better wording to reduce the deviation only
to integer and pointers. Any suggestions?


> > @@ -479,6 +486,24 @@ maintainers if you want to suggest a change.
> > they are related
> >   -
> >  
> > +   * - `Rule 21.1 
> > `_
> > + - Required
> > + - #define and #undef shall not be used on a reserved identifier or
> > +   reserved macro name
> > + - No identifiers should start with _BUILTIN to avoid clashes with
> 
> DYM "__builtin_"? Also gcc introduces far more than just __builtin_...()
> into the name space.

Yes agreed, but in my notes that is the only one I wrote down. I recall
that the complete list is a couple of pages long, I don't think we can
possibly add it here in full and if I recall it is not available in the
GCC documentation. More on this below.


> > +   GCC reserved identifiers. In general, all identifiers starting with
> > +   an underscore are reserved, and are best avoided.
> 
> This isn't precise enough. A leading underscore followed by a lower-case
> letter or a number is okay to use for file-scope symbols. Imo we should
> not aim at removing such uses, but rather encourage more use.

More on this below


> In this context, re-reading some of the C99 spec, I have to realize that
> my commenting on underscore-prefixed macro parameters (but not underscore-
> prefixed locals in macros) was based on ambiguous information in the spec.
> I will try to remember to not comment on such anymore going forward.
>
> > However, Xen
> > +   makes extensive usage of leading underscores in header guards,
> > +   bitwise manipulation functions, and a few other places. They are
> > +   considered safe as checks have been done against the list of
> > +   GCC's reserved identifiers. They don't need to be replaced.
> 
> This leaves quite vague what wants and what does not want replacing, nor
> what might be okay to introduce subsequently despite violation this rule.
 
My goals here were to convey the following:
1) avoid clashes with gcc reserved identifiers
2) in general try to reduce the usage of leading underscores except for
   known existing locations (header guards, bitwise manipulation
   functions)

However, for 1) the problem is that we don't have the full list and for
2) the problem is that it is too vague.

Instead I suggest we do the following:
- we get the full list of gcc reserved identifiers from Roberto and we
  commit it to xen.git as a seprate file under docs/misra
- we reference the list here saying one should avoid clashes with those
  identifiers as reserved by gcc


And if we can find a clear general comment about the usage of leading
underscores in Xen I am happy to add it too (e.g. header guards), but
from MISRA point of view the above is sufficient.

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

[osstest service owner]

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

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  d9f07b06cfc967e128371c11f56699cf5ee40b43
baseline version:
 xen  bad1ac345b1910b820b8a703ad1b9f66412ea844

Last test of basis   183447  2023-10-20 15:00:26 Z4 days
Testing same since   183517  2023-10-24 19:02:08 Z0 days1 attempts


People who touched revisions under test:
  Henry Wang 
  Jan Beulich 
  Julien Grall 
  Stefano Stabellini 

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
   bad1ac345b..d9f07b06cf  d9f07b06cfc967e128371c11f56699cf5ee40b43 -> smoke

Re: [XEN PATCH][for-4.19 v3] xen: Add deviations for MISRA C:2012 Rule 7.1

[Stefano Stabellini]

On Tue, 24 Oct 2023, Nicola Vetrini wrote:
> As specified in rules.rst, these constants can be used
> in the code.
> 
> Signed-off-by: Nicola Vetrini 
> ---
> Changes in v2:
> - replace some SAF deviations with configurations
> Changes in v3:
> - refine configurations and justifications
> ---
>  automation/eclair_analysis/ECLAIR/deviations.ecl | 10 ++
>  docs/misra/deviations.rst|  5 +
>  docs/misra/safe.json |  8 
>  xen/arch/x86/hvm/svm/emulate.c   |  6 +++---
>  xen/common/inflate.c |  4 ++--
>  5 files changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
> b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index fa56e5c00a27..ea5e0eb1813f 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -85,10 +85,12 @@ conform to the directive."
>  # Series 7.
>  #
>  
> --doc_begin="Usage of the following constants is safe, since they are given 
> as-is
> -in the inflate algorithm specification and there is therefore no risk of them
> -being interpreted as decimal constants."
> --config=MC3R1.R7.1,literals={safe, 
> "^0(007|37|070|213|236|300|321|330|331|332|333|334|335|337|371)$"}
> +-doc_begin="It is safe to use certain octal constants the way they are 
> defined in
> +specifications, manuals, and algorithm descriptions."
> +-file_tag+={x86_svm_h, "^xen/arch/x86/hvm/svm/svm\\.h$"}
> +-file_tag+={x86_emulate_c, "^xen/arch/x86/hvm/svm/emulate\\.c$"}
> +-config=MC3R1.R7.1,reports+={safe, 
> "any_area(any_loc(any_exp(file(x86_svm_h)&(^INSTR_ENC$"}
> +-config=MC3R1.R7.1,reports+={safe, 
> "any_area(text(^.*octal-ok.*$)&_loc(any_exp(file(x86_emulate_c)&(^MASK_EXTR$"}
>  -doc_end
>  
>  -doc_begin="Violations in files that maintainers have asked to not modify in 
> the
> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
> index 8511a189253b..26c6dbbc9ffe 100644
> --- a/docs/misra/deviations.rst
> +++ b/docs/misra/deviations.rst
> @@ -90,6 +90,11 @@ Deviations related to MISRA C:2012 Rules:
>   - __emulate_2op and __emulate_2op_nobyte
>   - read_debugreg and write_debugreg
>  
> +   * - R7.1
> + - It is safe to use certain octal constants the way they are defined in
> +   specifications, manuals, and algorithm descriptions.

I think we should add that these cases have "octal-ok" as a in-code
comment. Everything else looks OK so this small change could be done on
commit.

Reviewed-by: Stefano Stabellini 


> + - Tagged as `safe` for ECLAIR.
> +
> * - R7.2
>   - Violations caused by __HYPERVISOR_VIRT_START are related to the
> particular use of it done in xen_mk_ulong.
> diff --git a/docs/misra/safe.json b/docs/misra/safe.json
> index 39c5c056c7d4..7ea47344ffcc 100644
> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -20,6 +20,14 @@
>  },
>  {
>  "id": "SAF-2-safe",
> +"analyser": {
> +"eclair": "MC3R1.R7.1"
> +},
> +"name": "Rule 7.1: constants defined in specifications, manuals, 
> and algorithm descriptions",
> +"text": "It is safe to use certain octal constants the way they 
> are defined in specifications, manuals, and algorithm descriptions."
> +},
> +{
> +"id": "SAF-3-safe",
>  "analyser": {},
>  "name": "Sentinel",
>  "text": "Next ID to be used"
> diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
> index aa2c61c433b3..93ac1d3435f9 100644
> --- a/xen/arch/x86/hvm/svm/emulate.c
> +++ b/xen/arch/x86/hvm/svm/emulate.c
> @@ -90,9 +90,9 @@ unsigned int svm_get_insn_len(struct vcpu *v, unsigned int 
> instr_enc)
>  if ( !instr_modrm )
>  return emul_len;
>  
> -if ( modrm_mod   == MASK_EXTR(instr_modrm, 0300) &&
> - (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
> - (modrm_rm  & 7) == MASK_EXTR(instr_modrm, 0007) )
> +if ( modrm_mod   == MASK_EXTR(instr_modrm, 0300) && /* octal-ok 
> */
> + (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && /* octal-ok 
> */
> + (modrm_rm  & 7) == MASK_EXTR(instr_modrm, 0007) )  /* octal-ok 
> */
>  return emul_len;
>  }
>  
> diff --git a/xen/common/inflate.c b/xen/common/inflate.c
> index 8fa4b96d12a3..be6a9115187e 100644
> --- a/xen/common/inflate.c
> +++ b/xen/common/inflate.c
> @@ -1201,8 +1201,8 @@ static int __init gunzip(void)
>  magic[1] = NEXTBYTE();
>  method   = NEXTBYTE();
>  
> -if (magic[0] != 037 ||
> -((magic[1] != 0213) && (magic[1] != 0236))) {
> +/* SAF-2-safe */
> +if (magic[0] != 037 || ((magic[1] != 0213) && (magic[1] != 0236))) {
>  error("bad gzip magic numbers");
>  return -1;
>  }
> -- 
> 2.34.1
> 

Re: [RFC 1/4] x86/ioemul: address MISRA C:2012 Rule 9.3

[Stefano Stabellini]

On Tue, 24 Oct 2023, Jan Beulich wrote:
> On 24.10.2023 16:31, Nicola Vetrini wrote:
> > Partially explicitly initalized .matches arrays result in violations
> > of Rule 9.3; this is resolved by using designated initializers,
> > which is permitted by the Rule.
> > 
> > Mechanical changes.
> > 
> > Signed-off-by: Nicola Vetrini 
> 
> While not overly bad, I'm still not really seeing the improvement.
> Yet aiui changes induced by Misra are supposed to improve things in
> some direction?
 
I think the improvement is clarity, in the sense that the designated
initializers make it clearer that the array may be sparsely initialized
and that the remaining elements should be initialized to zero
automatically.
 

> > --- a/xen/arch/x86/ioport_emulate.c
> > +++ b/xen/arch/x86/ioport_emulate.c
> > @@ -44,57 +44,57 @@ static const struct dmi_system_id __initconstrel 
> > ioport_quirks_tbl[] = {
> >  {
> >  .ident = "HP ProLiant DL3xx",
> >  .matches = {
> > -DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > -DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL3"),
> > +[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > +[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL3"),
> >  },
> >  },
> >  {
> >  .ident = "HP ProLiant DL5xx",
> >  .matches = {
> > -DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > -DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL5"),
> > +[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > +[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL5"),
> >  },
> >  },
> >  {
> >  .ident = "HP ProLiant DL7xx",
> >  .matches = {
> > -DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > -DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL7"),
> > +[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > +[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL7"),
> >  },
> >  },
> >  {
> >  .ident = "HP ProLiant ML3xx",
> >  .matches = {
> > -DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > -DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML3"),
> > +[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > +[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML3"),
> >  },
> >  },
> >  {
> >  .ident = "HP ProLiant ML5xx",
> >  .matches = {
> > -DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > -DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML5"),
> > +[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > +[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML5"),
> >  },
> >  },
> >  {
> >  .ident = "HP ProLiant BL2xx",
> >  .matches = {
> > -DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > -DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL2"),
> > +[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > +[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL2"),
> >  },
> >  },
> >  {
> >  .ident = "HP ProLiant BL4xx",
> >  .matches = {
> > -DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > -DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL4"),
> > +[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > +[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL4"),
> >  },
> >  },
> >  {
> >  .ident = "HP ProLiant BL6xx",
> >  .matches = {
> > -DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > -DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL6"),
> > +[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > +[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL6"),
> >  },
> >  },
> >  { }
> > --
> > 2.34.1
> 

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

[Stefano Stabellini]

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

Re: [PATCH for-4.19 v2] docs/arm: Document where Xen should be loaded in memory

[Stefano Stabellini]

On Tue, 24 Oct 2023, Bertrand Marquis wrote:
> Hi Julien,
> 
> > On 24 Oct 2023, at 12:28, Julien Grall  wrote:
> > 
> > From: Julien Grall 
> > 
> > In commit 9d267c049d92 ("xen/arm64: Rework the memory layout"),
> > we decided to require Xen to be loaded below 2 TiB to simplify
> > the logic to enable the MMU. The limit was decided based on
> > how known platform boot plus some slack.
> > 
> > We had a recent report that this is not sufficient on the AVA
> > platform with a old firmware [1]. But the restriction is not
> > going to change in Xen 4.18. So document the limit clearly
> > in docs/misc/arm/booting.txt.
> > 
> > [1] https://lore.kernel.org/20231013122658.1270506-3-leo@linaro.org
> > 
> > Signed-off-by: Julien Grall 
> > Reviewed-by: Michal Orzel 
> 
> Reviewed-by: Bertrand Marquis 

Added for for-4.19

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

[Stefano Stabellini]

On Tue, 24 Oct 2023, Nicola Vetrini wrote:
> On 24/10/2023 10:14, Jan Beulich wrote:
> > On 24.10.2023 10:01, Nicola Vetrini wrote:
> > > On 24/10/2023 09:50, Jan Beulich wrote:
> > > > On 23.10.2023 11:56, Nicola Vetrini wrote:
> > > > > As stated in rules.rst, functions used only in asm code
> > > > > are allowed to have no prior declaration visible when being
> > > > > defined, hence these functions are deviated.
> > > > > This also fixes violations of MISRA C:2012 Rule 8.4.
> > > > > 
> > > > > Signed-off-by: Nicola Vetrini 
> > > > > Reviewed-by: Stefano Stabellini 
> > > > > ---
> > > > > Changes in v3:
> > > > > - added SAF deviations for vmx counterparts to svm functions.
> > > > 
> > > > Same comment regarding the R-b here as for patch 2.
> > > > 
> > > > > --- a/xen/arch/x86/hvm/svm/intr.c
> > > > > +++ b/xen/arch/x86/hvm/svm/intr.c
> > > > > @@ -123,6 +123,7 @@ static void svm_enable_intr_window(struct vcpu *v,
> > > > > struct hvm_intack intack)
> > > > >  vmcb, general1_intercepts | GENERAL1_INTERCEPT_VINTR);
> > > > >  }
> > > > > 
> > > > > +/* SAF-1-safe */
> > > > >  void svm_intr_assist(void)
> > > > >  {
> > > > >  struct vcpu *v = current;
> > > > 
> > > > Linux has the concept of "asmlinkage" for functions interfacing C and
> > > > assembly. Was it considered to use that - even if expanding to nothing
> > > > for all present architectures - as a way to annotate affected
> > > > definitions
> > > > in place of the SAF-*-safe comments?
> > > 
> > > It was proposed by Julien a while ago (I think it the thread on
> > > deviations.rst) to define
> > > a macro asmcall that expands to nothing, to mark all such functions.
> > > Right now, it's not
> > > strictly necessary (given that there are already some uses of SAF in
> > > Stefano's for-4.19 branch.
> > 
> > Can this then be revisited please before any such reaches staging?
> > 
> > Jan
> 
> I'll let Stefano answer this one.

Yes it can. If Nicola sends new patches I'll make sure to remove the
corresponding ones from for-4.19.

Nicola, you might want to send me privately the list of commits to take
out from for-4.19.

Re: [PATCH v10 16/17] xen/arm: vpci: permit access to guest vpci space

[Stewart Hildebrand]

On 10/16/23 07:00, Jan Beulich wrote:
> On 13.10.2023 00:09, Volodymyr Babchuk wrote:
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -695,6 +695,9 @@ struct domain *domain_create(domid_t domid,
>>  radix_tree_init(>pirq_tree);
>>  }
>>
>> +if ( !is_idle_domain(d) )
>> +d->iomem_caps = rangeset_new(d, "I/O Memory", 
>> RANGESETF_prettyprint_hex);
>> +
>>  if ( (err = arch_domain_create(d, config, flags)) != 0 )
>>  goto fail;
>>  init_status |= INIT_arch;
>> @@ -704,7 +707,6 @@ struct domain *domain_create(domid_t domid,
>>  watchdog_domain_init(d);
>>  init_status |= INIT_watchdog;
>>
>> -d->iomem_caps = rangeset_new(d, "I/O Memory", 
>> RANGESETF_prettyprint_hex);
>>  d->irq_caps   = rangeset_new(d, "Interrupts", 0);
>>  if ( !d->iomem_caps || !d->irq_caps )
>>  goto fail;
> 
> It's not really logical to move one, not both. Plus you didn't move the
> error check, so if the earlier initialization is really needed, you set
> things up for a NULL deref.
> 
> Jan

Good catch, I'll move both along with the error check (and I'll coordinate the 
update with Volodymyr). Thanks.

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

[Stefano Stabellini]

On Tue, 24 Oct 2023, Henry Wang wrote:
> Hi Stefano,
> 
> > On Oct 24, 2023, at 04:56, Stefano Stabellini  
> > wrote:
> > 
> > Signed-off-by: Stefano Stabellini 
> 
> I saw I was CCed so:
> 
> Release-acked-by: Henry Wang 

Thanks everyone for the acks. For simplicity I'll add it to my for-4.19
branch instead of staging.

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

[Stefano Stabellini]

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

I added the patch to my for-4.19 branch

[ovmf test] 183516: all pass - PUSHED

[osstest service owner]

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

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

Last test of basis   183511  2023-10-24 12:10:45 Z0 days
Testing same since   183516  2023-10-24 17:13:56 Z0 days1 attempts


People who touched revisions under test:
  Aaron Young 
  Ard Biesheuvel 
  Laszlo Ersek 

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
   e17e58e81b..d85bf54b7f  d85bf54b7f462eb0297351b5f8dfde09adf617fb -> 
xen-tested-master

[linux-linus test] 183510: tolerable trouble: fail/pass/starved - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 linuxd88520ad73b79e71e3ddf08de335b8520ae41c5c
baseline version:
 linux84186fcb834ecc55604efaf383e17e6b5e9baa50

Last test of basis   183503  2023-10-24 02:26:34 Z0 days
Testing same since   183510  2023-10-24 11:41:38 Z0 days1 attempts


People who touched revisions under test:
  Al Viro 
  Chuck Lever 
  Jeff Layton 
  Linus Torvalds 

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

Re: [PATCH v5 2/9] iommu/arm: Add iommu_dt_xlate()

[Julien Grall]

Hi Stewart,

On 04/10/2023 15:55, Stewart Hildebrand wrote:

From: Oleksandr Tyshchenko 

Move code for processing DT IOMMU specifier to a separate helper.
This helper will be re-used for adding PCI devices by the subsequent
patches as we will need exact the same actions for processing
DT PCI-IOMMU specifier.

While at it introduce NO_IOMMU to avoid magic "1".

Signed-off-by: Oleksandr Tyshchenko 
Signed-off-by: Stewart Hildebrand 
---
v4->v5:
* rebase on top of "dynamic node programming using overlay dtbo" series
* move #define NO_IOMMU 1 to header
* s/these/this/ inside comment

v3->v4:
* make dt_phandle_args *iommu_spec const
* move !ops->add_device check to helper

v2->v3:
* no change

v1->v2:
* no change

downstream->v1:
* trivial rebase
* s/dt_iommu_xlate/iommu_dt_xlate/

(cherry picked from commit c26bab0415ca303df86aba1d06ef8edc713734d3 from
  the downstream branch poc/pci-passthrough from
  https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
---
  xen/drivers/passthrough/device_tree.c | 48 +--
  xen/include/xen/iommu.h   |  2 ++
  2 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/xen/drivers/passthrough/device_tree.c 
b/xen/drivers/passthrough/device_tree.c
index 075fb25a3706..159ace9856c9 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -137,6 +137,30 @@ int iommu_release_dt_devices(struct domain *d)
  return 0;
  }
  
+static int iommu_dt_xlate(struct device *dev,

+  const struct dt_phandle_args *iommu_spec)
+{
+const struct iommu_ops *ops = iommu_get_ops();


NIT: Rather than calling iommu_get_ops(), how about passing the ops as a 
parameter of iommu_dt_xlate()?


Reviewed-by: Julien Grall 

Cheers,

--
Julien Grall

Re: [PATCH v2 16/24] hw/xen: handle soft reset for primary console

[David Woodhouse]

On Tue, 2023-10-24 at 17:22 +0100, Paul Durrant wrote:
> On 24/10/2023 16:48, David Woodhouse wrote:
> > On Tue, 2023-10-24 at 16:44 +0100, Paul Durrant wrote:
> > > On 19/10/2023 16:40, David Woodhouse wrote:
> > > > From: David Woodhouse 
> > > > 
> > > > On soft reset, the prinary console event channel needs to be rebound to
> > > > the backend port (in the xen-console driver). We could put that into the
> > > > xen-console driver itself, but it's slightly less ugly to keep it within
> > > > the KVM/Xen code, by stashing the backend port# on event channel reset
> > > > and then rebinding in the primary console reset when it has to recreate
> > > > the guest port anyway.
> > > 
> > > Does Xen re-bind the primary console on EVTCHNOP_reset? That's news to
> > > me. I go check.
> > 
> > I spent an unhapp hour trying to work out how Xen actually does any of
> > this :)
> > 
> > In the short term I'm more interested in having soft reset work, than
> > an explicit EVTCHNOP_reset. And I can't work out *how*, but we do seem
> > to have console again after a kexec in real Xen.
> 
> *Soft* reset may do it, but not the EVTCHNOP_reset hypercall itself, 
> because there's a bunch of impenetrable toolstack magic involved the 
> former. Perhaps you could just push the re-bind code up a layer into
> kvm_xen_soft_reset().

Actually, all we're doing here is *storing* the be_port so that the
*console* reset code can later connect to it. So the actual reconnect 
is already called separately from a layer up in kvm_xen_soft_reset().

If the guest just calls EVTCHNOP_reset then it'll just get the event
channels reset and the console *won't* reconnect.

Actually.. if the guest just calls EVTCHNOP_reset I think they'll just
hit the assert(qemu_mutex_iothread_locked()) at line 1109. We need a  
QEMU_IOTHREAD_LOCK_GUARD() in the penultimate line of
xen_evtchn_reset_op(). I'll fix that separately.


smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v3 0/4] Final CHANGELOG changes for 4.18

[Julien Grall]

On 23/10/2023 10:21, Henry Wang wrote:

Hi all,


Hi Henry,



This series should be the final CHANGELOG changes for 4.18.

The first patch is mentioning the MISRA-C improvement during the
4.18 dev cycle, second patch is added as a clean-up patch during
the review of this series, so these should be committed before
we branch.

The third patch sets the release date and tag of 4.18 release
and should be included in both the staging and stable-4.18 once
we branch.

The fourth patch starts a new unstable section, so should be in
unstable master/staging only.

Thanks.

Henry Wang (4):
   CHANGELOG.md: Mention the MISRA-C improvement in 4.18 dev cycle
   CHANGELOG.md: Use "xenbits.xenproject.org" in links
   CHANGELOG.md: Set 4.18 release date and tag


I have committed the first 3 patches. I will commit ...


   CHANGELOG.md: Start new "unstable" section


... the last one after we branched for 4.18.

Cheers,


--
Julien Grall

Re: [PATCH v3 3/4] CHANGELOG.md: Set 4.18 release date and tag

[Julien Grall]

Hi Henry,

On 23/10/2023 10:21, Henry Wang wrote:

Signed-off-by: Henry Wang 

Acked-by: Julien Grall 
   
   
  > ---

v3:
- Don't set the release date.
---
  CHANGELOG.md | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 25d29fe59d..3ca7969699 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,7 +4,7 @@ Notable changes to Xen will be documented in this file.
  
  The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
  
-## [unstable UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=staging) - TBD

+## 
[4.18.0](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.18.0)
 - 2023-XX-XX


I can update the date just before I tag the release.

Cheers,

  
  ### Changed

   - Repurpose command line gnttab_max_{maptrack_,}frames options so they don't


--
Julien Grall

[ovmf test] 183511: all pass - PUSHED

[osstest service owner]

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

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

Last test of basis   183506  2023-10-24 08:12:27 Z0 days
Testing same since   183511  2023-10-24 12:10:45 Z0 days1 attempts


People who touched revisions under test:
  Jose Marinho 

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
   fb044b7fe8..e17e58e81b  e17e58e81b356a347102ee6d780bf699544e9b81 -> 
xen-tested-master

Re: [PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode

[David Woodhouse]

On Tue, 2023-10-24 at 17:25 +0100, Paul Durrant wrote:
> On 24/10/2023 16:49, David Woodhouse wrote:
> > On Tue, 2023-10-24 at 16:39 +0100, Paul Durrant wrote:
> > > On 24/10/2023 16:37, David Woodhouse wrote:
> > > > On Tue, 2023-10-24 at 15:20 +0100, Paul Durrant wrote:
> > > > > On 16/10/2023 16:19, David Woodhouse wrote:
> > > > > > From: David Woodhouse 
> > > > > > 
> > > > > > The primary console is special because the toolstack maps a page at 
> > > > > > a
> > > > > > fixed GFN and also allocates the guest-side event channel. Add 
> > > > > > support
> > > > > > for that in emulated mode, so that we can have a primary console.
> > > > > > 
> > > > > > Add a *very* rudimentary stub of foriegnmem ops for emulated mode, 
> > > > > > which
> > > > > > supports literally nothing except a single-page mapping of the 
> > > > > > console
> > > > > > page. This might as well have been a hack in the xen_console 
> > > > > > driver, but
> > > > > > this way at least the special-casing is kept within the Xen 
> > > > > > emulation
> > > > > > code, and it gives us a hook for a more complete implementation 
> > > > > > if/when
> > > > > > we ever do need one.
> > > > > > 
> > > > > Why can't you map the console page via the grant table like the 
> > > > > xenstore
> > > > > page?
> > > > 
> > > > I suppose we could, but I didn't really want the generic xen-console
> > > > device code having any more of a special case for 'Xen emulation' than
> > > > it does already by having to call xen_primary_console_create().
> > > > 
> > > 
> > > But doesn't is save you the whole foreignmem thing? You can use the
> > > grant table for primary and secondary consoles.
> > 
> > Yes. And I could leave the existing foreignmem thing just for the case
> > of primary console under true Xen. It's probably not that awful a
> > special case, in the end.
> > 
> > Then again, I was surprised I didn't *already* have a foreignmem ops
> > for the emulated case, and we're probably going to want to continue
> > fleshing it out later, so I don't really mind adding it.
> > 
> 
> True. We'll need it for some of the other more fun protocols like vkbd 
> or fb. Still, I think it'd be nicer to align the xenstore and primary
> console code to look similar and punt the work until then :-)

I don't think it ends up looking like xenstore either way, does it?
Xenstore is special because it gets to use the original pointer to its
own page.

I don't think I want to hack the xen_console code to explicitly call a
xen_console_give_me_your_page() function. If not foreignmem, I think
you were suggesting that we actually call the grant mapping code to get
a pointer to the underlying page, right? 

I could kind of live with that... except that Xen has this ugly
convention that the "ring-ref" frontend node for the primary console
actually has the *MFN* not a grant ref. Which I don't understand since
the toolstack *does* populate the grant table for it (just as it does
for the xenstore page). But we'd have to add a special case exception
to that special case, so that in the emu case it's an actual grant ref
again. I think I prefer just having a stub of foreignmem, TBH.

(I didn't yet manage to get Xen to actually create a primary console of
type iomem, FWIW)




smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode

[Paul Durrant]

On 24/10/2023 16:49, David Woodhouse wrote:

On Tue, 2023-10-24 at 16:39 +0100, Paul Durrant wrote:

On 24/10/2023 16:37, David Woodhouse wrote:

On Tue, 2023-10-24 at 15:20 +0100, Paul Durrant wrote:

On 16/10/2023 16:19, David Woodhouse wrote:

From: David Woodhouse 

The primary console is special because the toolstack maps a page at a
fixed GFN and also allocates the guest-side event channel. Add support
for that in emulated mode, so that we can have a primary console.

Add a *very* rudimentary stub of foriegnmem ops for emulated mode, which
supports literally nothing except a single-page mapping of the console
page. This might as well have been a hack in the xen_console driver, but
this way at least the special-casing is kept within the Xen emulation
code, and it gives us a hook for a more complete implementation if/when
we ever do need one.


Why can't you map the console page via the grant table like the xenstore
page?


I suppose we could, but I didn't really want the generic xen-console
device code having any more of a special case for 'Xen emulation' than
it does already by having to call xen_primary_console_create().



But doesn't is save you the whole foreignmem thing? You can use the
grant table for primary and secondary consoles.


Yes. And I could leave the existing foreignmem thing just for the case
of primary console under true Xen. It's probably not that awful a
special case, in the end.

Then again, I was surprised I didn't *already* have a foreignmem ops
for the emulated case, and we're probably going to want to continue
fleshing it out later, so I don't really mind adding it.



True. We'll need it for some of the other more fun protocols like vkbd 
or fb. Still, I think it'd be nicer to align the xenstore and primary 
console code to look similar and punt the work until then :-)


  Paul

Re: [PATCH v2 16/24] hw/xen: handle soft reset for primary console

[Paul Durrant]

On 24/10/2023 16:48, David Woodhouse wrote:

On Tue, 2023-10-24 at 16:44 +0100, Paul Durrant wrote:

On 19/10/2023 16:40, David Woodhouse wrote:

From: David Woodhouse 

On soft reset, the prinary console event channel needs to be rebound to
the backend port (in the xen-console driver). We could put that into the
xen-console driver itself, but it's slightly less ugly to keep it within
the KVM/Xen code, by stashing the backend port# on event channel reset
and then rebinding in the primary console reset when it has to recreate
the guest port anyway.


Does Xen re-bind the primary console on EVTCHNOP_reset? That's news to
me. I go check.


I spent an unhapp hour trying to work out how Xen actually does any of
this :)

In the short term I'm more interested in having soft reset work, than
an explicit EVTCHNOP_reset. And I can't work out *how*, but we do seem
to have console again after a kexec in real Xen.


*Soft* reset may do it, but not the EVTCHNOP_reset hypercall itself, 
because there's a bunch of impenetrable toolstack magic involved the 
former. Perhaps you could just push the re-bind code up a layer into

kvm_xen_soft_reset().

  Paul

Re: [PATCH 2/4] hw/xen: update Xen PV NIC to XenDevice model

[Paul Durrant]

On 24/10/2023 16:17, David Woodhouse wrote:
[snip]


I don't think that's really a valid state for a network frontend. Linux
netback just ignores it.


Must we? I was thinking of making the ->frontend_changed() methods
optional and allowing backends to just provide ->connect() and
->disconnect() methods instead if they wanted to. Because we have three
identical ->frontend_changed() methods now...



Now maybe... Not sure things will look so common when other backends are 
converted. I'd prefer to maintain fidelity with Linux xen-netback as it 
is generally considered to be the canonical implementation.


  Paul

Re: [PATCH 0/12] Get Xen PV shim running in qemu

[David Woodhouse]

On Tue, 2023-10-24 at 16:24 +0100, Alex Bennée wrote:
> 
> David Woodhouse  writes:
> 
> > I hadn't got round to getting the PV shim running yet; I thought it would
> > need work on the multiboot loader. Turns out it doesn't. I *did* need to
> > fix a couple of brown-paper-bag bugs in the per-vCPU upcall vector support,
> > and implement Xen console support though. Now I can test PV guests:
> > 
> >  $ qemu-system-x86_64 --accel kvm,xen-version=0x40011,kernel-irqchip=split \
> >    -chardev stdio,mux=on,id=char0 -device xen-console,chardev=char0 \
> >    -drive file=${GUEST_IMAGE},if=xen -display none -m 1G \
> >    -kernel ~/git/xen/xen/xen -initrd ~/git/linux/arch/x86/boot/bzImage
> >  \
> 

(Reordering your questions so the answers flow better)

> Would it be possible to have some sort of overview document in our
> manual for how Xen guests are supported under KVM?

https://qemu-project.gitlab.io/qemu/system/i386/xen.html covers running
Xen HVM guests under Qemu/KVM.

What I'm adding here is the facility to support Xen PV guests. There is
a corresponding update to the documentation in my working tree at 
https://git.infradead.org/users/dwmw2/qemu.git/shortlog/refs/heads/xenfv

https://git.infradead.org/users/dwmw2/qemu.git/commitdiff/af693bf51141

PV mode is the old mode which predates proper virtualization support in
the CPUs, where a guest kernel *knows* it doesn't have access to real
(or indeed virtualized) hardware. It runs in ring 1 (or ring 3 on
x86_64) and makes hypercalls to Xen to ask it to do all the MMU
management.

When Spectre/Meltdown happened, running actual PV guests directly under
Xen became kind of insane, so we hacked a version of Xen to work as a
"shim", running inside a proper HVM guest, and just providing those MMU
management services to its guest. Its *only* guest. This shim doesn't
even do any of the PV disk/block stuff; that's passed through directly
to the real hypervisor.

So you have a real Xen hypervisor, then a "PV shim" Xen running inside
that hardware virtual machine, and a guest kernel hosted by that PV
shim.

Now, since Qemu/KVM can now pretend to be Xen and host guests that
think they're running as Xen HVM guests... Qemu/KVM can host that PV
shim too. As noted, I just had to realise that we could use '-initrd'
to trick Qemu's multiboot loader into doing it... and fix a few brown
paper bag bugs.

> So this is a KVM guest running the Xen hypervisor (via -kernel) and a
> Dom0 Linux guest (via -initrd)?

Fairly much. It's a KVM guest running that "shim" version of the Xen
hypervisor via -kernel, and a Linux guest via -initrd.

Although I wouldn't call that a "Dom0 Linux guest" because we tend to
use "Dom0" to mean the control domain, which can launch other (DomU)
guests... and that isn't what's happening here. It's more of a "Dom1".
The one and only unprivileged guest.

In particular, there's no nested virtualization here. Because in that
sense, what "Xen" does to host a PV guest isn't really virtualization.

> Should this work for any Xen architecture or is this x86 specific? Does
> the -M machine model matter?

It's currently x86-specific and KVM-specific. You can use the pc or q35
models as you see fit, although see the doc linked above for discussion
of the IDE 'unplug' mechanism. And recent patches on the list to fix it
for q35.

It would be interesting to make it work on other platforms, and even
with TCG. I've tried to keep it as portable as possible up to a point,
but without too much gratuitous overengineering to chase that goal.

Making it work with TCG would require dealing with all the struct
layouts where alignment/padding differs on different host
architectures, so we probably wouldn't be able to use the Xen public
header files directly. And we would need to implement some of the basic
event channel delivery and shared info page handling that we rely on
KVM to do for us. The latter probably isn't that hard; the former is
what stopped me even bothering.

Making it work for e.g. Arm would require porting some of the KVM
support to Arm in the kernel (it's currently x86-specific). And/or
making it work for TCG but the parts that *are* accelerated in the
kernel (timers, IPIs, etc) are there for a reason though. If we do make
it work for TCG by implementing those in userspace, I wouldn't
necessarily want a *KVM* guest to have to rely on those in userspace.


smime.p7s
Description: S/MIME cryptographic signature

Re: [XEN RFC] xen/automation: add deviations for MISRA C:2012 Rule 8.3

[Jan Beulich]

On 24.10.2023 17:22, Federico Serafini wrote:
> On 24/10/23 16:32, Jan Beulich wrote:
>> On 24.10.2023 15:22, Federico Serafini wrote:
>>> Update ECLAIR configuration to deviate Rule 8.3 ("All declarations of
>>> an object or function shall use the same names and type qualifiers")
>>> for the following functions:
>>> - set_px_pminfo();
>>> - guest_walk_tables_[0-9]+_levels().
>>>
>>> Update file docs/misra/deviations.rst accordingly.
>>> No functional change.
>>>
>>> Signed-off-by: Federico Serafini 
>>> ---
>>> I had a discussion with Jan about the reasons behind the choice of parameter
>>> name 'walk' for the definitions of functions 
>>> guest_walk_tables_[0-9]+_levels()
>>> and the parameter name 'pfec' for the corresponding declarations.
>>> Also for the function set_px_pminfo(), it seems that the parameter names are
>>> different on purpose.
>>
>> In this latter case I wonder why you think so. Did I end up making a
>> misleading comment anywhere? It looks to me as if naming the parameter
>> in question "perf" uniformly would be quite okay.
> 
> No, it was just my impression.
> What about the mismatch on the first parameter "acpi_id" vs "cpu"?

The two can't possibly be used interchangeably, so whichever entity it
is that is being passed should determine the uniformly used name.

Jan

Re: [PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode

[David Woodhouse]

On Tue, 2023-10-24 at 16:39 +0100, Paul Durrant wrote:
> On 24/10/2023 16:37, David Woodhouse wrote:
> > On Tue, 2023-10-24 at 15:20 +0100, Paul Durrant wrote:
> > > On 16/10/2023 16:19, David Woodhouse wrote:
> > > > From: David Woodhouse 
> > > > 
> > > > The primary console is special because the toolstack maps a page at a
> > > > fixed GFN and also allocates the guest-side event channel. Add support
> > > > for that in emulated mode, so that we can have a primary console.
> > > > 
> > > > Add a *very* rudimentary stub of foriegnmem ops for emulated mode, which
> > > > supports literally nothing except a single-page mapping of the console
> > > > page. This might as well have been a hack in the xen_console driver, but
> > > > this way at least the special-casing is kept within the Xen emulation
> > > > code, and it gives us a hook for a more complete implementation if/when
> > > > we ever do need one.
> > > > 
> > > Why can't you map the console page via the grant table like the xenstore
> > > page?
> > 
> > I suppose we could, but I didn't really want the generic xen-console
> > device code having any more of a special case for 'Xen emulation' than
> > it does already by having to call xen_primary_console_create().
> > 
> 
> But doesn't is save you the whole foreignmem thing? You can use the 
> grant table for primary and secondary consoles.

Yes. And I could leave the existing foreignmem thing just for the case
of primary console under true Xen. It's probably not that awful a
special case, in the end.

Then again, I was surprised I didn't *already* have a foreignmem ops
for the emulated case, and we're probably going to want to continue
fleshing it out later, so I don't really mind adding it.



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v2 16/24] hw/xen: handle soft reset for primary console

[David Woodhouse]

On Tue, 2023-10-24 at 16:44 +0100, Paul Durrant wrote:
> On 19/10/2023 16:40, David Woodhouse wrote:
> > From: David Woodhouse 
> > 
> > On soft reset, the prinary console event channel needs to be rebound to
> > the backend port (in the xen-console driver). We could put that into the
> > xen-console driver itself, but it's slightly less ugly to keep it within
> > the KVM/Xen code, by stashing the backend port# on event channel reset
> > and then rebinding in the primary console reset when it has to recreate
> > the guest port anyway.
> 
> Does Xen re-bind the primary console on EVTCHNOP_reset? That's news to 
> me. I go check.

I spent an unhapp hour trying to work out how Xen actually does any of
this :)

In the short term I'm more interested in having soft reset work, than
an explicit EVTCHNOP_reset. And I can't work out *how*, but we do seem
to have console again after a kexec in real Xen.


smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v2 16/24] hw/xen: handle soft reset for primary console

[Paul Durrant]

On 19/10/2023 16:40, David Woodhouse wrote:

From: David Woodhouse 

On soft reset, the prinary console event channel needs to be rebound to
the backend port (in the xen-console driver). We could put that into the
xen-console driver itself, but it's slightly less ugly to keep it within
the KVM/Xen code, by stashing the backend port# on event channel reset
and then rebinding in the primary console reset when it has to recreate
the guest port anyway.


Does Xen re-bind the primary console on EVTCHNOP_reset? That's news to 
me. I go check.


  Paul



Signed-off-by: David Woodhouse 
---
  hw/i386/kvm/xen_evtchn.c  |  9 +
  hw/i386/kvm/xen_primary_console.c | 29 -
  hw/i386/kvm/xen_primary_console.h |  1 +
  3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
index d72dca6591..ce4da6d37a 100644
--- a/hw/i386/kvm/xen_evtchn.c
+++ b/hw/i386/kvm/xen_evtchn.c
@@ -40,6 +40,7 @@
  #include "xen_evtchn.h"
  #include "xen_overlay.h"
  #include "xen_xenstore.h"
+#include "xen_primary_console.h"
  
  #include "sysemu/kvm.h"

  #include "sysemu/kvm_xen.h"
@@ -1098,6 +1099,7 @@ int xen_evtchn_soft_reset(void)
  {
  XenEvtchnState *s = xen_evtchn_singleton;
  bool flush_kvm_routes;
+uint16_t con_port = xen_primary_console_get_port();
  int i;
  
  if (!s) {

@@ -1108,6 +1110,13 @@ int xen_evtchn_soft_reset(void)
  
  qemu_mutex_lock(>port_lock);
  
+if (con_port) {

+XenEvtchnPort *p = >port_table[con_port];
+if (p->type == EVTCHNSTAT_interdomain) {
+xen_primary_console_set_be_port(p->u.interdomain.port);
+}
+}
+
  for (i = 0; i < s->nr_ports; i++) {
  close_port(s, i, _kvm_routes);
  }
diff --git a/hw/i386/kvm/xen_primary_console.c 
b/hw/i386/kvm/xen_primary_console.c
index 0aa1c16ad6..5e6e085ac7 100644
--- a/hw/i386/kvm/xen_primary_console.c
+++ b/hw/i386/kvm/xen_primary_console.c
@@ -112,6 +112,15 @@ uint16_t xen_primary_console_get_port(void)
  return s->guest_port;
  }
  
+void xen_primary_console_set_be_port(uint16_t port)

+{
+XenPrimaryConsoleState *s = xen_primary_console_singleton;
+if (s) {
+printf("be port set to %d\n", port);
+s->be_port = port;
+}
+}
+
  uint64_t xen_primary_console_get_pfn(void)
  {
  XenPrimaryConsoleState *s = xen_primary_console_singleton;
@@ -142,6 +151,20 @@ static void alloc_guest_port(XenPrimaryConsoleState *s)
  }
  }
  
+static void rebind_guest_port(XenPrimaryConsoleState *s)

+{
+struct evtchn_bind_interdomain inter = {
+.remote_dom = DOMID_QEMU,
+.remote_port = s->be_port,
+};
+
+if (!xen_evtchn_bind_interdomain_op()) {
+s->guest_port = inter.local_port;
+}
+
+s->be_port = 0;
+}
+
  int xen_primary_console_reset(void)
  {
  XenPrimaryConsoleState *s = xen_primary_console_singleton;
@@ -154,7 +177,11 @@ int xen_primary_console_reset(void)
  xen_overlay_do_map_page(>console_page, gpa);
  }
  
-alloc_guest_port(s);

+if (s->be_port) {
+rebind_guest_port(s);
+} else {
+alloc_guest_port(s);
+}
  
  trace_xen_primary_console_reset(s->guest_port);
  
diff --git a/hw/i386/kvm/xen_primary_console.h b/hw/i386/kvm/xen_primary_console.h

index dd4922f3f4..7e2989ea0d 100644
--- a/hw/i386/kvm/xen_primary_console.h
+++ b/hw/i386/kvm/xen_primary_console.h
@@ -16,6 +16,7 @@ void xen_primary_console_create(void);
  int xen_primary_console_reset(void);
  
  uint16_t xen_primary_console_get_port(void);

+void xen_primary_console_set_be_port(uint16_t port);
  uint64_t xen_primary_console_get_pfn(void);
  void *xen_primary_console_get_map(void);
  

Re: [PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode

[Paul Durrant]

On 24/10/2023 16:37, David Woodhouse wrote:

On Tue, 2023-10-24 at 15:20 +0100, Paul Durrant wrote:

On 16/10/2023 16:19, David Woodhouse wrote:

From: David Woodhouse 

The primary console is special because the toolstack maps a page at a
fixed GFN and also allocates the guest-side event channel. Add support
for that in emulated mode, so that we can have a primary console.

Add a *very* rudimentary stub of foriegnmem ops for emulated mode, which
supports literally nothing except a single-page mapping of the console
page. This might as well have been a hack in the xen_console driver, but
this way at least the special-casing is kept within the Xen emulation
code, and it gives us a hook for a more complete implementation if/when
we ever do need one.


Why can't you map the console page via the grant table like the xenstore
page?


I suppose we could, but I didn't really want the generic xen-console
device code having any more of a special case for 'Xen emulation' than
it does already by having to call xen_primary_console_create().



But doesn't is save you the whole foreignmem thing? You can use the 
grant table for primary and secondary consoles.


  Paul

Re: [PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode

[David Woodhouse]

On Tue, 2023-10-24 at 15:20 +0100, Paul Durrant wrote:
> On 16/10/2023 16:19, David Woodhouse wrote:
> > From: David Woodhouse 
> > 
> > The primary console is special because the toolstack maps a page at a
> > fixed GFN and also allocates the guest-side event channel. Add support
> > for that in emulated mode, so that we can have a primary console.
> > 
> > Add a *very* rudimentary stub of foriegnmem ops for emulated mode, which
> > supports literally nothing except a single-page mapping of the console
> > page. This might as well have been a hack in the xen_console driver, but
> > this way at least the special-casing is kept within the Xen emulation
> > code, and it gives us a hook for a more complete implementation if/when
> > we ever do need one.
> > 
> Why can't you map the console page via the grant table like the xenstore 
> page?

I suppose we could, but I didn't really want the generic xen-console
device code having any more of a special case for 'Xen emulation' than
it does already by having to call xen_primary_console_create().




smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v2 10/24] hw/xen: populate store frontend nodes with XenStore PFN/port

[Paul Durrant]

On 19/10/2023 16:40, David Woodhouse wrote:

From: David Woodhouse 

This is kind of redundant since without being able to get these through
some other method (HVMOP_get_param) the guest wouldn't be able to access
XenStore in order to find them. But Xen populates them, and it does
allow guests to *rebind* to the event channel port after a reset.

Signed-off-by: David Woodhouse 
---
  hw/i386/kvm/xen_xenstore.c | 11 +++
  1 file changed, 11 insertions(+)



Reviewed-by: Paul Durrant 

Re: [PATCH 0/12] Get Xen PV shim running in qemu

[Alex Bennée]

David Woodhouse  writes:

> I hadn't got round to getting the PV shim running yet; I thought it would
> need work on the multiboot loader. Turns out it doesn't. I *did* need to
> fix a couple of brown-paper-bag bugs in the per-vCPU upcall vector support,
> and implement Xen console support though. Now I can test PV guests:
>
>  $ qemu-system-x86_64 --accel kvm,xen-version=0x40011,kernel-irqchip=split \
>-chardev stdio,mux=on,id=char0 -device xen-console,chardev=char0 \
>-drive file=${GUEST_IMAGE},if=xen -display none -m 1G \
>-kernel ~/git/xen/xen/xen -initrd ~/git/linux/arch/x86/boot/bzImage
>  \

So this is a KVM guest running the Xen hypervisor (via -kernel) and a
Dom0 Linux guest (via -initrd)?

Should this work for any Xen architecture or is this x86 specific? Does
the -M machine model matter?

Would it be possible to have some sort of overview document in our
manual for how Xen guests are supported under KVM?

>-append "loglvl=all -- console=hvc0 root=/dev/xvda1"
>


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 05/24] hw/xen: fix XenStore watch delivery to guest

[David Woodhouse]

On Tue, 2023-10-24 at 16:19 +0100, Paul Durrant wrote:
> On 19/10/2023 16:40, David Woodhouse wrote:
> > From: David Woodhouse 
> > 
> > When fire_watch_cb() found the response buffer empty, it would call
> > deliver_watch() to generate the XS_WATCH_EVENT message in the response
> > buffer and send an event channel notification to the guest… without
> > actually *copying* the response buffer into the ring. So there was
> > nothing for the guest to see. The pending response didn't actually get
> > processed into the ring until the guest next triggered some activity
> > from its side.
> > 
> > Add the missing call to put_rsp().
> > 
> > It might have been slightly nicer to call xen_xenstore_event() here,
> > which would *almost* have worked. Except for the fact that it calls
> > xen_be_evtchn_pending() to check that it really does have an event
> > pending (and clear the eventfd for next time). And under Xen it's
> > defined that setting that fd to O_NONBLOCK isn't guaranteed to work,
> > so the emu implementation follows suit.
> > 
> > This fixes Xen device hot-unplug.
> > 
> > Fixes: 0254c4d19df ("hw/xen: Add xenstore wire implementation and 
> > implementation stubs")
> > Signed-off-by: David Woodhouse 
> > ---
> >   hw/i386/kvm/xen_xenstore.c | 8 +---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
> > index 660d0b72f9..82a215058a 100644
> > --- a/hw/i386/kvm/xen_xenstore.c
> > +++ b/hw/i386/kvm/xen_xenstore.c
> > @@ -1357,10 +1357,12 @@ static void fire_watch_cb(void *opaque, const char 
> > *path, const char *token)
> >   } else {
> >   deliver_watch(s, path, token);
> >   /*
> > - * If the message was queued because there was already ring 
> > activity,
> > - * no need to wake the guest. But if not, we need to send the 
> > evtchn.
> > + * Attempt to queue the message into the actual ring, and send
> > + * the event channel notification if any bytes are copied.
> >    */
> > -    xen_be_evtchn_notify(s->eh, s->be_port);
> > +    if (put_rsp(s) > 0) {
> > +    xen_be_evtchn_notify(s->eh, s->be_port);
> > +    }
> 
> There's actually the potential for an assertion failure there; if the
> guest has specified an oversize token then deliver_watch() will not set 
> rsp_pending... then put_rsp() will fail its assertion that the flag is true.
> 


Meh, I *already* had a whole paragraph in the commit message about how
it would have been nicer to just call xen_xenstore_event() :)

Thanks for spotting that; will fix it to check s->rsp_pending.




smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 1/4] hw/xen: only remove peers of PCI NICs on unplug

[David Woodhouse]

On Tue, 2023-10-24 at 15:32 +0100, Paul Durrant wrote:
> On 17/10/2023 19:25, David Woodhouse wrote:
> > From: David Woodhouse 
> > 
> > When the Xen guest asks to unplug *emulated* NICs, it's kind of unhelpful
> > also to unplug the peer of the *Xen* PV NIC.
> > 
> > Signed-off-by: David Woodhouse 
> > ---
> >    hw/i386/xen/xen_platform.c | 9 +++--
> >    1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
> > index 17457ff3de..e2dd1b536a 100644
> > --- a/hw/i386/xen/xen_platform.c
> > +++ b/hw/i386/xen/xen_platform.c
> > @@ -140,9 +140,14 @@ static void unplug_nic(PCIBus *b, PCIDevice *d, void 
> > *o)
> >    /* Remove the peer of the NIC device. Normally, this would be a tap 
> > device. */
> >    static void del_nic_peer(NICState *nic, void *opaque)
> >    {
> > -    NetClientState *nc;
> > +    NetClientState *nc = qemu_get_queue(nic);
> > +    ObjectClass *klass = module_object_class_by_name(nc->model);
> > +
> > +    /* Only delete peers of PCI NICs that we're about to delete */
> > +    if (!klass || !object_class_dynamic_cast(klass, TYPE_PCI_DEVICE)) {
> 
> Would it not be better to test for object_class_dynamic_cast(klass, 
> TYPE_XEN_DEVICE)?

Only if we also change the actual unplug to destroy non-PCI devices too.

The only non-PCI device you could have here is an ISA NE2000, I think.


smime.p7s
Description: S/MIME cryptographic signature

Re: [XEN RFC] xen/automation: add deviations for MISRA C:2012 Rule 8.3

[Federico Serafini]

On 24/10/23 16:32, Jan Beulich wrote:

On 24.10.2023 15:22, Federico Serafini wrote:

Update ECLAIR configuration to deviate Rule 8.3 ("All declarations of
an object or function shall use the same names and type qualifiers")
for the following functions:
- set_px_pminfo();
- guest_walk_tables_[0-9]+_levels().

Update file docs/misra/deviations.rst accordingly.
No functional change.

Signed-off-by: Federico Serafini 
---
I had a discussion with Jan about the reasons behind the choice of parameter
name 'walk' for the definitions of functions guest_walk_tables_[0-9]+_levels()
and the parameter name 'pfec' for the corresponding declarations.
Also for the function set_px_pminfo(), it seems that the parameter names are
different on purpose.


In this latter case I wonder why you think so. Did I end up making a
misleading comment anywhere? It looks to me as if naming the parameter
in question "perf" uniformly would be quite okay.

Jan


No, it was just my impression.
What about the mismatch on the first parameter "acpi_id" vs "cpu"?

--
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

Re: [PATCH v2 05/24] hw/xen: fix XenStore watch delivery to guest

[Paul Durrant]

On 19/10/2023 16:40, David Woodhouse wrote:

From: David Woodhouse 

When fire_watch_cb() found the response buffer empty, it would call
deliver_watch() to generate the XS_WATCH_EVENT message in the response
buffer and send an event channel notification to the guest… without
actually *copying* the response buffer into the ring. So there was
nothing for the guest to see. The pending response didn't actually get
processed into the ring until the guest next triggered some activity
from its side.

Add the missing call to put_rsp().

It might have been slightly nicer to call xen_xenstore_event() here,
which would *almost* have worked. Except for the fact that it calls
xen_be_evtchn_pending() to check that it really does have an event
pending (and clear the eventfd for next time). And under Xen it's
defined that setting that fd to O_NONBLOCK isn't guaranteed to work,
so the emu implementation follows suit.

This fixes Xen device hot-unplug.

Fixes: 0254c4d19df ("hw/xen: Add xenstore wire implementation and implementation 
stubs")
Signed-off-by: David Woodhouse 
---
  hw/i386/kvm/xen_xenstore.c | 8 +---
  1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
index 660d0b72f9..82a215058a 100644
--- a/hw/i386/kvm/xen_xenstore.c
+++ b/hw/i386/kvm/xen_xenstore.c
@@ -1357,10 +1357,12 @@ static void fire_watch_cb(void *opaque, const char 
*path, const char *token)
  } else {
  deliver_watch(s, path, token);
  /*
- * If the message was queued because there was already ring activity,
- * no need to wake the guest. But if not, we need to send the evtchn.
+ * Attempt to queue the message into the actual ring, and send
+ * the event channel notification if any bytes are copied.
   */
-xen_be_evtchn_notify(s->eh, s->be_port);
+if (put_rsp(s) > 0) {
+xen_be_evtchn_notify(s->eh, s->be_port);
+}


There's actually the potential for an assertion failure there; if the 
guest has specified an oversize token then deliver_watch() will not set 
rsp_pending... then put_rsp() will fail its assertion that the flag is true.


  Paul


  }
  }
  

Re: [PATCH 2/4] hw/xen: update Xen PV NIC to XenDevice model

[David Woodhouse]

On Tue, 2023-10-24 at 15:47 +0100, Paul Durrant wrote:
> On 17/10/2023 19:25, David Woodhouse wrote:
> > +
> > +#define xen_pv_printf(a, n, ...) qemu_printf(__VA_ARGS__)
> 
> Why define this...

In the first place, just to make it build in the short term. Then I
forgot to clean it up before posting. In my tree this is all tracing
now.

> 
> > @@ -232,7 +258,7 @@ static ssize_t net_rx_packet(NetClientState *nc, const 
> > uint8_t *buf, size_t size
> >   RING_IDX rc, rp;
> >   void *page;
> >   
> > -    if (netdev->xendev.be_state != XenbusStateConnected) {
> > +    if (netdev->rx_ring.sring == NULL) {
> 
> Why not a straight swap for xen_device_backend_get_state()? Hard to see 
> whether there any hidden side effects of this change otherwise.

Could do, but what I *actually* cared about when looking at that check,
was whether the ring pointer was NULL. So I checked that explicitly.

It should be identical.

> > +static void xen_netdev_frontend_changed(XenDevice *xendev,
> > +   enum xenbus_state frontend_state,
> > +   Error **errp)
> >   {
> > -    struct XenNetDev *netdev = container_of(xendev, struct XenNetDev, 
> > xendev);
> > -    net_tx_packets(netdev);
> > -    qemu_flush_queued_packets(qemu_get_queue(netdev->nic));
> > +    ERRP_GUARD();
> > +    enum xenbus_state backend_state = xen_device_backend_get_state(xendev);
> > +
> > +    trace_xen_netdev_frontend_changed(xendev->name, frontend_state);
> > +
> > +    switch (frontend_state) {
> > +    case XenbusStateInitialised:
> 
> I don't think that's really a valid state for a network frontend. Linux 
> netback just ignores it.

Must we? I was thinking of making the ->frontend_changed() methods
optional and allowing backends to just provide ->connect() and
->disconnect() methods instead if they wanted to. Because we have three
identical ->frontend_changed() methods now...




smime.p7s
Description: S/MIME cryptographic signature

Re: [RFC 2/4] x86/shutdown: address MISRA C:2012 Rule 9.3

[Nicola Vetrini]

On 24/10/2023 16:56, Jan Beulich wrote:

On 24.10.2023 16:31, Nicola Vetrini wrote:
@@ -225,8 +225,8 @@ static const struct dmi_system_id __initconstrel 
reboot_dmi_table[] = {

 .ident = "Dell OptiPlex 745",
 .matches = {
 DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 745"),
-DMI_MATCH(DMI_BOARD_NAME, "0MM599"),
+[0] = DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 745"),
+[1] = DMI_MATCH(DMI_BOARD_NAME, "0MM599")
 },
 },
 {/* Handle problems with rebooting on Dell Optiplex 745 with 
0KW626 */
@@ -235,8 +235,8 @@ static const struct dmi_system_id __initconstrel 
reboot_dmi_table[] = {

 .ident = "Dell OptiPlex 745",
 .matches = {
 DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 745"),
-DMI_MATCH(DMI_BOARD_NAME, "0KW626"),
+[0] = DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 745"),
+[1] = DMI_MATCH(DMI_BOARD_NAME, "0KW626")
 },
 },
 {/* Handle problems with rebooting on Dell Optiplex 330 with 
0KP561 */
@@ -245,8 +245,8 @@ static const struct dmi_system_id __initconstrel 
reboot_dmi_table[] = {

 .ident = "Dell OptiPlex 330",
 .matches = {
 DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 330"),
-DMI_MATCH(DMI_BOARD_NAME, "0KP561"),
+[0] = DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 330"),
+[1] = DMI_MATCH(DMI_BOARD_NAME, "0KP561")
 },
 },
 {/* Handle problems with rebooting on Dell Optiplex 360 with 
0T656F */
@@ -255,8 +255,8 @@ static const struct dmi_system_id __initconstrel 
reboot_dmi_table[] = {

 .ident = "Dell OptiPlex 360",
 .matches = {
 DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 360"),
-DMI_MATCH(DMI_BOARD_NAME, "0T656F"),
+[0] = DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 360"),
+[1] = DMI_MATCH(DMI_BOARD_NAME, "0T656F")
 },
 },
 {/* Handle problems with rebooting on Dell OptiPlex 760 with 
0G919G */
@@ -265,8 +265,8 @@ static const struct dmi_system_id __initconstrel 
reboot_dmi_table[] = {

 .ident = "Dell OptiPlex 760",
 .matches = {
 DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 760"),
-DMI_MATCH(DMI_BOARD_NAME, "0G919G"),
+[0] = DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 760"),
+[1] = DMI_MATCH(DMI_BOARD_NAME, "0G919G")
 },
 },
 {/* Handle problems with rebooting on Dell 2400's */


Well, no, this is what absolutely should not happen: You're breaking
the code here, but initializing slot 0 twice in multiple instances.
From looking just at the patch I probably wouldn't have noticed, but
the "always elements 0 and 1 only" pattern made me "grep -lr", where
the issue became apparent. Otherwise I was about to suggest we shrink
array size to just 2 elements.

Jan


Right, that was a shortcoming of my regex.

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

Re: [PATCH v2 04/24] hw/xen: don't clear map_track[] in xen_gnttab_reset()

[Paul Durrant]

On 19/10/2023 16:40, David Woodhouse wrote:

From: David Woodhouse 

The refcounts actually correspond to 'active_ref' structures stored in a
GHashTable per "user" on the backend side (mostly, per XenDevice).

If we zero map_track[] on reset, then when the backend drivers get torn
down and release their mapping we hit the assert(s->map_track[ref] != 0)
in gnt_unref().

So leave them in place. Each backend driver will disconnect and reconnect
as the guest comes back up again and reconnects, and it all works out OK
in the end as the old refs get dropped.

Fixes: de26b2619789 ("hw/xen: Implement soft reset for emulated gnttab")
Signed-off-by: David Woodhouse 
---
  hw/i386/kvm/xen_gnttab.c | 2 --
  1 file changed, 2 deletions(-)



Reviewed-by: Paul Durrant 

Re: [PATCH v2 03/24] hw/xen: select kernel mode for per-vCPU event channel upcall vector

[Paul Durrant]

On 19/10/2023 16:39, David Woodhouse wrote:

From: David Woodhouse 

A guest which has configured the per-vCPU upcall vector may set the
HVM_PARAM_CALLBACK_IRQ param to fairly much anything other than zero.

For example, Linux v6.0+ after commit b1c3497e604 ("x86/xen: Add support
for HVMOP_set_evtchn_upcall_vector") will just do this after setting the
vector:

/* Trick toolstack to think we are enlightened. */
if (!cpu)
rc = xen_set_callback_via(1);

That's explicitly setting the delivery to GSI#1, but it's supposed to be
overridden by the per-vCPU vector setting. This mostly works in Qemu
*except* for the logic to enable the in-kernel handling of event channels,
which falsely determines that the kernel cannot accelerate GSI delivery
in this case.

Add a kvm_xen_has_vcpu_callback_vector() to report whether vCPU#0 has
the vector set, and use that in xen_evtchn_set_callback_param() to
enable the kernel acceleration features even when the param *appears*
to be set to target a GSI.

Preserve the Xen behaviour that when HVM_PARAM_CALLBACK_IRQ is set to
*zero* the event channel delivery is disabled completely. (Which is
what that bizarre guest behaviour is working round in the first place.)

Fixes: 91cce756179 ("hw/xen: Add xen_evtchn device for event channel emulation")
Signed-off-by: David Woodhouse 
---
  hw/i386/kvm/xen_evtchn.c  | 6 ++
  include/sysemu/kvm_xen.h  | 1 +
  target/i386/kvm/xen-emu.c | 7 +++
  3 files changed, 14 insertions(+)



Reviewed-by: Paul Durrant 

Re: [RFC 2/4] x86/shutdown: address MISRA C:2012 Rule 9.3

[Jan Beulich]

On 24.10.2023 16:31, Nicola Vetrini wrote:
> @@ -225,8 +225,8 @@ static const struct dmi_system_id __initconstrel 
> reboot_dmi_table[] = {
>  .ident = "Dell OptiPlex 745",
>  .matches = {
>  DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> -DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 745"),
> -DMI_MATCH(DMI_BOARD_NAME, "0MM599"),
> +[0] = DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 745"),
> +[1] = DMI_MATCH(DMI_BOARD_NAME, "0MM599")
>  },
>  },
>  {/* Handle problems with rebooting on Dell Optiplex 745 with 0KW626 
> */
> @@ -235,8 +235,8 @@ static const struct dmi_system_id __initconstrel 
> reboot_dmi_table[] = {
>  .ident = "Dell OptiPlex 745",
>  .matches = {
>  DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> -DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 745"),
> -DMI_MATCH(DMI_BOARD_NAME, "0KW626"),
> +[0] = DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 745"),
> +[1] = DMI_MATCH(DMI_BOARD_NAME, "0KW626")
>  },
>  },
>  {/* Handle problems with rebooting on Dell Optiplex 330 with 0KP561 
> */
> @@ -245,8 +245,8 @@ static const struct dmi_system_id __initconstrel 
> reboot_dmi_table[] = {
>  .ident = "Dell OptiPlex 330",
>  .matches = {
>  DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> -DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 330"),
> -DMI_MATCH(DMI_BOARD_NAME, "0KP561"),
> +[0] = DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 330"),
> +[1] = DMI_MATCH(DMI_BOARD_NAME, "0KP561")
>  },
>  },
>  {/* Handle problems with rebooting on Dell Optiplex 360 with 0T656F 
> */
> @@ -255,8 +255,8 @@ static const struct dmi_system_id __initconstrel 
> reboot_dmi_table[] = {
>  .ident = "Dell OptiPlex 360",
>  .matches = {
>  DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> -DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 360"),
> -DMI_MATCH(DMI_BOARD_NAME, "0T656F"),
> +[0] = DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 360"),
> +[1] = DMI_MATCH(DMI_BOARD_NAME, "0T656F")
>  },
>  },
>  {/* Handle problems with rebooting on Dell OptiPlex 760 with 0G919G 
> */
> @@ -265,8 +265,8 @@ static const struct dmi_system_id __initconstrel 
> reboot_dmi_table[] = {
>  .ident = "Dell OptiPlex 760",
>  .matches = {
>  DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> -DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 760"),
> -DMI_MATCH(DMI_BOARD_NAME, "0G919G"),
> +[0] = DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 760"),
> +[1] = DMI_MATCH(DMI_BOARD_NAME, "0G919G")
>  },
>  },
>  {/* Handle problems with rebooting on Dell 2400's */

Well, no, this is what absolutely should not happen: You're breaking
the code here, but initializing slot 0 twice in multiple instances.
>From looking just at the patch I probably wouldn't have noticed, but
the "always elements 0 and 1 only" pattern made me "grep -lr", where
the issue became apparent. Otherwise I was about to suggest we shrink
array size to just 2 elements.

Jan

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

[Roger Pau Monne]

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

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

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

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

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

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

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

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

Amends: 3fba06ba9f8b ('x86/IRQ: re-use legacy vector ranges on APs')
Signed-off-by: Roger Pau Monné 
---
Changes since v2:
 - Put the AMD vendor checks in do_IRQ() instead of bogus_8259A_irq().
 - Re-indent cpumask_copy() call in init_IRQ().
 - Reword one sentence in the commit message.

Changes since v1:
 - Do not reserved spurious PIC vectors on APs, but still check for spurious
   PIC interrupts.
 - Reword commit message.
---
 xen/arch/x86/i8259.c | 21 +++--
 xen/arch/x86/irq.c   | 11 ++-
 2 files changed, 29 insertions(+), 3 deletions(-)

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

Re: [RFC 4/4] amd/iommu: fully initialize array in 'flush_command_buffer'

[Jan Beulich]

On 24.10.2023 16:31, Nicola Vetrini wrote:
> Fully explicit initialization of the cmd array resolves a violation of
> MISRA C:2012 Rule 9.3.
> 
> Signed-off-by: Nicola Vetrini 

While I'm not overly happy, we accepted the rule, so
Acked-by: Jan Beulich 

Re: [RFC 1/4] x86/ioemul: address MISRA C:2012 Rule 9.3

[Jan Beulich]

On 24.10.2023 16:31, Nicola Vetrini wrote:
> Partially explicitly initalized .matches arrays result in violations
> of Rule 9.3; this is resolved by using designated initializers,
> which is permitted by the Rule.
> 
> Mechanical changes.
> 
> Signed-off-by: Nicola Vetrini 

While not overly bad, I'm still not really seeing the improvement.
Yet aiui changes induced by Misra are supposed to improve things in
some direction?

Jan

> --- a/xen/arch/x86/ioport_emulate.c
> +++ b/xen/arch/x86/ioport_emulate.c
> @@ -44,57 +44,57 @@ static const struct dmi_system_id __initconstrel 
> ioport_quirks_tbl[] = {
>  {
>  .ident = "HP ProLiant DL3xx",
>  .matches = {
> -DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> -DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL3"),
> +[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> +[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL3"),
>  },
>  },
>  {
>  .ident = "HP ProLiant DL5xx",
>  .matches = {
> -DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> -DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL5"),
> +[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> +[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL5"),
>  },
>  },
>  {
>  .ident = "HP ProLiant DL7xx",
>  .matches = {
> -DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> -DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL7"),
> +[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> +[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL7"),
>  },
>  },
>  {
>  .ident = "HP ProLiant ML3xx",
>  .matches = {
> -DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> -DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML3"),
> +[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> +[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML3"),
>  },
>  },
>  {
>  .ident = "HP ProLiant ML5xx",
>  .matches = {
> -DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> -DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML5"),
> +[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> +[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML5"),
>  },
>  },
>  {
>  .ident = "HP ProLiant BL2xx",
>  .matches = {
> -DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> -DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL2"),
> +[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> +[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL2"),
>  },
>  },
>  {
>  .ident = "HP ProLiant BL4xx",
>  .matches = {
> -DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> -DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL4"),
> +[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> +[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL4"),
>  },
>  },
>  {
>  .ident = "HP ProLiant BL6xx",
>  .matches = {
> -DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> -DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL6"),
> +[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> +[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL6"),
>  },
>  },
>  { }
> --
> 2.34.1

Re: [PATCH 2/4] hw/xen: update Xen PV NIC to XenDevice model

[Paul Durrant]

On 17/10/2023 19:25, David Woodhouse wrote:

From: David Woodhouse 

Signed-off-by: David Woodhouse 
---
  hw/net/meson.build|   2 +-
  hw/net/trace-events   |   9 +
  hw/net/xen_nic.c  | 426 +-
  hw/xenpv/xen_machine_pv.c |   1 -
  4 files changed, 342 insertions(+), 96 deletions(-)

diff --git a/hw/net/meson.build b/hw/net/meson.build
index 2632634df3..f64651c467 100644
--- a/hw/net/meson.build
+++ b/hw/net/meson.build
@@ -1,5 +1,5 @@
  system_ss.add(when: 'CONFIG_DP8393X', if_true: files('dp8393x.c'))
-system_ss.add(when: 'CONFIG_XEN', if_true: files('xen_nic.c'))
+system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xen_nic.c'))
  system_ss.add(when: 'CONFIG_NE2000_COMMON', if_true: files('ne2000.c'))
  
  # PCI network cards

diff --git a/hw/net/trace-events b/hw/net/trace-events
index 3abfd65e5b..ee56acfbce 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -482,3 +482,12 @@ dp8393x_receive_oversize(int size) "oversize packet, pkt_size 
is %d"
  dp8393x_receive_not_netcard(void) "packet not for netcard"
  dp8393x_receive_packet(int crba) "Receive packet at 0x%"PRIx32
  dp8393x_receive_write_status(int crba) "Write status at 0x%"PRIx32
+
+# xen_nic.c
+xen_netdev_realize(int idx, const char *info) "idx %u info '%s'"
+xen_netdev_unrealize(int idx) "idx %u"
+xen_netdev_create(int idx) "idx %u"
+xen_netdev_destroy(int idx) "idx %u"
+xen_netdev_disconnect(int idx) "idx %u"
+xen_netdev_connect(int idx, unsigned int tx, unsigned int rx, int port) "idx %u tx 
%u rx %u port %u"
+xen_netdev_frontend_changed(const char *idx, int state) "idx %s state %d"
diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
index 9bbf6599fc..84914c329c 100644
--- a/hw/net/xen_nic.c
+++ b/hw/net/xen_nic.c
@@ -20,6 +20,11 @@
   */
  
  #include "qemu/osdep.h"

+#include "qemu/cutils.h"
+#include "qemu/qemu-print.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/error.h"
+
  #include 
  #include 
  #include 
@@ -27,18 +32,26 @@
  #include "net/net.h"
  #include "net/checksum.h"
  #include "net/util.h"
-#include "hw/xen/xen-legacy-backend.h"
+
+#include "hw/xen/xen-backend.h"
+#include "hw/xen/xen-bus-helper.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
  
  #include "hw/xen/interface/io/netif.h"

+#include "hw/xen/interface/io/xs_wire.h"
+
+#include "trace.h"
  
  /* - */
  
  struct XenNetDev {

-struct XenLegacyDevice  xendev;  /* must be first */
-char  *mac;
+struct XenDevice  xendev;  /* must be first */
+XenEventChannel   *event_channel;
+int   dev;
  int   tx_work;
-int   tx_ring_ref;
-int   rx_ring_ref;
+unsigned int  tx_ring_ref;
+unsigned int  rx_ring_ref;
  struct netif_tx_sring *txs;
  struct netif_rx_sring *rxs;
  netif_tx_back_ring_t  tx_ring;
@@ -47,6 +60,13 @@ struct XenNetDev {
  NICState  *nic;
  };
  
+typedef struct XenNetDev XenNetDev;

+
+#define TYPE_XEN_NET_DEVICE "xen-net-device"
+OBJECT_DECLARE_SIMPLE_TYPE(XenNetDev, XEN_NET_DEVICE)
+
+#define xen_pv_printf(a, n, ...) qemu_printf(__VA_ARGS__)


Why define this...


+
  /* - */
  
  static void net_tx_response(struct XenNetDev *netdev, netif_tx_request_t *txp, int8_t st)

@@ -68,7 +88,8 @@ static void net_tx_response(struct XenNetDev *netdev, 
netif_tx_request_t *txp, i
  netdev->tx_ring.rsp_prod_pvt = ++i;
  RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(>tx_ring, notify);
  if (notify) {
-xen_pv_send_notify(>xendev);
+xen_device_notify_event_channel(XEN_DEVICE(netdev),
+netdev->event_channel, NULL);
  }
  
  if (i == netdev->tx_ring.req_cons) {

@@ -104,8 +125,9 @@ static void net_tx_error(struct XenNetDev *netdev, 
netif_tx_request_t *txp, RING
  #endif
  }
  
-static void net_tx_packets(struct XenNetDev *netdev)

+static bool net_tx_packets(struct XenNetDev *netdev)
  {
+bool done_something = false;
  netif_tx_request_t txreq;
  RING_IDX rc, rp;
  void *page;
@@ -122,6 +144,7 @@ static void net_tx_packets(struct XenNetDev *netdev)
  }
  memcpy(, RING_GET_REQUEST(>tx_ring, rc), 
sizeof(txreq));
  netdev->tx_ring.req_cons = ++rc;
+done_something = true;
  
  #if 1

  /* should not happen in theory, we don't announce the *
@@ -151,7 +174,7 @@ static void net_tx_packets(struct XenNetDev *netdev)
  continue;
  }
  
-xen_pv_printf(>xendev, 3,

+if (0) qemu_printf(//>xendev, 3,


... and then not use it here? Perhaps the 'if (0)' ugliness can go in 
the macro too.



"tx packet ref %d, off %d, len %d, flags 
0x%x%s%s%s%s\n",

Re: [XEN PATCH] xen/iommu_init: address a violation of MISRA C:2012 Rule 8.3

[Jan Beulich]

On 24.10.2023 16:01, Federico Serafini wrote:
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -692,7 +692,7 @@ static void iommu_check_ppr_log(struct amd_iommu *iommu)
>  spin_unlock_irqrestore(>lock, flags);
>  }
>  
> -static void cf_check do_amd_iommu_irq(void *unused)
> +static void cf_check do_amd_iommu_irq(void *data)
>  {
>  struct amd_iommu *iommu;
>  
> @@ -702,6 +702,11 @@ static void cf_check do_amd_iommu_irq(void *unused)
>  return;
>  }
>  
> +/*
> + * Formal parameter is deliberately unused.
> + */
> +(void) data;

Besides me thinking that the original way of expressing things was more
clear (and still even machine-recognizable), there are (nit) also style
issues here: The comment is malformed and there shouldn't be a blank
between the cast operator and the expression it applies to.

Jan

Re: [PATCH 09/12] hw/xen: prevent duplicate device registrations

[David Woodhouse]

On Tue, 2023-10-24 at 15:10 +0100, Paul Durrant wrote:
> On 16/10/2023 16:19, David Woodhouse wrote:
> > --- a/hw/char/xen_console.c
> > +++ b/hw/char/xen_console.c
> > @@ -468,7 +468,7 @@ static void 
> > xen_console_device_create(XenBackendInstance *backend,
> >   Chardev *cd = NULL;
> >   struct qemu_xs_handle *xsh = xenbus->xsh;
> >   
> > -    if (qemu_strtoul(name, NULL, 10, )) {
> > +    if (qemu_strtoul(name, NULL, 10, ) || number >= INT_MAX) {
> >   error_setg(errp, "failed to parse name '%s'", name);
> >   goto fail;
> >   }
> I don't think this hunk belongs here, does it? Seems like it should be 
> in patch 7.

Well, console#4294967295 *did* actually work before this patch started
using -1 to mean something different. But yes, I've already moved that
into the previous patch.

In fact I've just completely dropped this patch now, as the
dedeuplication needs to happen on the *frontend* nodes, since a given
frontend can be powered by a backend of different types, or in
different driver domains.



smime.p7s
Description: S/MIME cryptographic signature

Re: [XEN RFC] xen/automation: add deviations for MISRA C:2012 Rule 8.3

[Jan Beulich]

On 24.10.2023 15:22, Federico Serafini wrote:
> Update ECLAIR configuration to deviate Rule 8.3 ("All declarations of
> an object or function shall use the same names and type qualifiers")
> for the following functions:
> - set_px_pminfo();
> - guest_walk_tables_[0-9]+_levels().
> 
> Update file docs/misra/deviations.rst accordingly.
> No functional change.
> 
> Signed-off-by: Federico Serafini 
> ---
> I had a discussion with Jan about the reasons behind the choice of parameter
> name 'walk' for the definitions of functions guest_walk_tables_[0-9]+_levels()
> and the parameter name 'pfec' for the corresponding declarations.
> Also for the function set_px_pminfo(), it seems that the parameter names are
> different on purpose.

In this latter case I wonder why you think so. Did I end up making a
misleading comment anywhere? It looks to me as if naming the parameter
in question "perf" uniformly would be quite okay.

Jan

Re: [PATCH 1/4] hw/xen: only remove peers of PCI NICs on unplug

[Paul Durrant]

On 17/10/2023 19:25, David Woodhouse wrote:

From: David Woodhouse 

When the Xen guest asks to unplug *emulated* NICs, it's kind of unhelpful
also to unplug the peer of the *Xen* PV NIC.

Signed-off-by: David Woodhouse 
---
  hw/i386/xen/xen_platform.c | 9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
index 17457ff3de..e2dd1b536a 100644
--- a/hw/i386/xen/xen_platform.c
+++ b/hw/i386/xen/xen_platform.c
@@ -140,9 +140,14 @@ static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
  /* Remove the peer of the NIC device. Normally, this would be a tap device. */
  static void del_nic_peer(NICState *nic, void *opaque)
  {
-NetClientState *nc;
+NetClientState *nc = qemu_get_queue(nic);
+ObjectClass *klass = module_object_class_by_name(nc->model);
+
+/* Only delete peers of PCI NICs that we're about to delete */
+if (!klass || !object_class_dynamic_cast(klass, TYPE_PCI_DEVICE)) {


Would it not be better to test for object_class_dynamic_cast(klass, 
TYPE_XEN_DEVICE)?


  Paul


+return;
+}
  
-nc = qemu_get_queue(nic);

  if (nc->peer)
  qemu_del_net_client(nc->peer);
  }

[RFC 4/4] amd/iommu: fully initialize array in 'flush_command_buffer'

[Nicola Vetrini]

Fully explicit initialization of the cmd array resolves a violation of
MISRA C:2012 Rule 9.3.

Signed-off-by: Nicola Vetrini 
---
 xen/drivers/passthrough/amd/iommu_cmd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c 
b/xen/drivers/passthrough/amd/iommu_cmd.c
index cb28b36abc38..49b9fcac9410 100644
--- a/xen/drivers/passthrough/amd/iommu_cmd.c
+++ b/xen/drivers/passthrough/amd/iommu_cmd.c
@@ -66,7 +66,8 @@ static void flush_command_buffer(struct amd_iommu *iommu,
  IOMMU_COMP_WAIT_S_FLAG_MASK),
 (addr >> 32) | MASK_INSR(IOMMU_CMD_COMPLETION_WAIT,
  IOMMU_CMD_OPCODE_MASK),
-CMD_COMPLETION_DONE
+CMD_COMPLETION_DONE,
+0
 };
 s_time_t start, timeout;
 static unsigned int __read_mostly threshold = 1;
--
2.34.1

[RFC 0/4] address violations of MISRA C Rule 9.3

[Nicola Vetrini]

This series addresses some of the violations of Rule 9.3, which is about
partially initialized arrays. The resolution strategy proposed in these patches
uses designated initializers, except in patch 4, allowed by MISRA for
sparse initialization. The reason why I chose this method is that, given that
most of the violations are about the 'matches' field of struct
'struct dmi_system_id', which is a sized array of structs:

struct dmi_strmatch matches[4];

Since the initialization is already partially implicit, using designated
initalizers is convenient because, if the lenght of the matches array changes,
no adjustment is needed

Another, stricter, resolution strategy is the following:

 .matches  = {
 DMI_MATCH(DMI_BOARD_VENDOR, "Quanta"),
-DMI_MATCH(DMI_BOARD_NAME,   "30B7"),
+DMI_MATCH(DMI_BOARD_NAME,   "30B7"),
+{}, {}
 }

Note that Rule 9.3 is not about array elements that may be uninitialized, but
the fact of having some explicitly initialized elements and some implicitly
initialized elements.

Nicola Vetrini (4):
  x86/ioemul: address MISRA C:2012 Rule 9.3
  x86/shutdown: address MISRA C:2012 Rule 9.3
  x86/hvm: quirks: address MISRA C:2012 Rule 9.3
  amd/iommu: fully initialize array in 'flush_command_buffer'

 xen/arch/x86/hvm/quirks.c   |  20 ++--
 xen/arch/x86/ioport_emulate.c   |  32 ++---
 xen/arch/x86/shutdown.c | 152 
 xen/drivers/passthrough/amd/iommu_cmd.c |   3 +-
 4 files changed, 104 insertions(+), 103 deletions(-)

--
2.34.1

[RFC 2/4] x86/shutdown: address MISRA C:2012 Rule 9.3

[Nicola Vetrini]

Partially explicitly initalized .matches arrays result in violations
of Rule 9.3; this is resolved by using designated initializers,
which is permitted by the Rule.

Mechanical changes.

Signed-off-by: Nicola Vetrini 
---
 xen/arch/x86/shutdown.c | 152 
 1 file changed, 76 insertions(+), 76 deletions(-)

diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c
index 7619544d14da..382c948f81a4 100644
--- a/xen/arch/x86/shutdown.c
+++ b/xen/arch/x86/shutdown.c
@@ -188,8 +188,8 @@ static const struct dmi_system_id __initconstrel 
reboot_dmi_table[] = {
 .driver_data = (void *)(long)BOOT_KBD,
 .ident = "Dell E520",
 .matches = {
-DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-DMI_MATCH(DMI_PRODUCT_NAME, "Dell DM061"),
+[0] = DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+[1] = DMI_MATCH(DMI_PRODUCT_NAME, "Dell DM061")
 },
 },
 {/* Handle problems with rebooting on Dell 1300's */
@@ -197,8 +197,8 @@ static const struct dmi_system_id __initconstrel 
reboot_dmi_table[] = {
 .driver_data = (void *)(long)BOOT_KBD,
 .ident = "Dell PowerEdge 1300",
 .matches = {
-DMI_MATCH(DMI_SYS_VENDOR, "Dell Computer Corporation"),
-DMI_MATCH(DMI_PRODUCT_NAME, "PowerEdge 1300/"),
+[0] = DMI_MATCH(DMI_SYS_VENDOR, "Dell Computer Corporation"),
+[1] = DMI_MATCH(DMI_PRODUCT_NAME, "PowerEdge 1300/")
 },
 },
 {/* Handle problems with rebooting on Dell 300's */
@@ -206,8 +206,8 @@ static const struct dmi_system_id __initconstrel 
reboot_dmi_table[] = {
 .driver_data = (void *)(long)BOOT_KBD,
 .ident = "Dell PowerEdge 300",
 .matches = {
-DMI_MATCH(DMI_SYS_VENDOR, "Dell Computer Corporation"),
-DMI_MATCH(DMI_PRODUCT_NAME, "PowerEdge 300/"),
+[0] = DMI_MATCH(DMI_SYS_VENDOR, "Dell Computer Corporation"),
+[1] = DMI_MATCH(DMI_PRODUCT_NAME, "PowerEdge 300/")
 },
 },
 {/* Handle problems with rebooting on Dell Optiplex 745's SFF */
@@ -215,8 +215,8 @@ static const struct dmi_system_id __initconstrel 
reboot_dmi_table[] = {
 .driver_data = (void *)(long)BOOT_KBD,
 .ident = "Dell OptiPlex 745",
 .matches = {
-DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 745"),
+[0] = DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+[1] = DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 745")
 },
 },
 {/* Handle problems with rebooting on Dell Optiplex 745's DFF */
@@ -225,8 +225,8 @@ static const struct dmi_system_id __initconstrel 
reboot_dmi_table[] = {
 .ident = "Dell OptiPlex 745",
 .matches = {
 DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 745"),
-DMI_MATCH(DMI_BOARD_NAME, "0MM599"),
+[0] = DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 745"),
+[1] = DMI_MATCH(DMI_BOARD_NAME, "0MM599")
 },
 },
 {/* Handle problems with rebooting on Dell Optiplex 745 with 0KW626 */
@@ -235,8 +235,8 @@ static const struct dmi_system_id __initconstrel 
reboot_dmi_table[] = {
 .ident = "Dell OptiPlex 745",
 .matches = {
 DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 745"),
-DMI_MATCH(DMI_BOARD_NAME, "0KW626"),
+[0] = DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 745"),
+[1] = DMI_MATCH(DMI_BOARD_NAME, "0KW626")
 },
 },
 {/* Handle problems with rebooting on Dell Optiplex 330 with 0KP561 */
@@ -245,8 +245,8 @@ static const struct dmi_system_id __initconstrel 
reboot_dmi_table[] = {
 .ident = "Dell OptiPlex 330",
 .matches = {
 DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 330"),
-DMI_MATCH(DMI_BOARD_NAME, "0KP561"),
+[0] = DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 330"),
+[1] = DMI_MATCH(DMI_BOARD_NAME, "0KP561")
 },
 },
 {/* Handle problems with rebooting on Dell Optiplex 360 with 0T656F */
@@ -255,8 +255,8 @@ static const struct dmi_system_id __initconstrel 
reboot_dmi_table[] = {
 .ident = "Dell OptiPlex 360",
 .matches = {
 DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 360"),
-DMI_MATCH(DMI_BOARD_NAME, "0T656F"),
+[0] = DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 360"),
+[1] = DMI_MATCH(DMI_BOARD_NAME, "0T656F")
 },
 },
 {/* Handle problems with rebooting on Dell OptiPlex 760 with 0G919G */
@@ -265,8 +265,8 @@ static const struct dmi_system_id __initconstrel 
reboot_dmi_table[] = {
 .ident = "Dell OptiPlex 760",
 .matches = {
 DMI_MATCH(DMI_SYS_VENDOR, 

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

[Stewart Hildebrand]

On 10/24/23 03:39, Roger Pau Monné wrote:
> On Tue, Oct 24, 2023 at 09:09:45AM +0200, Jan Beulich wrote:
>> On 23.10.2023 18:36, Stewart Hildebrand wrote:
>>> During xl pci-assignable-add, pciback may reset the device while memory 
>>> decoding
>>> (PCI_COMMAND_MEMORY) is enabled. After device reset, memory decoding may be
>>> disabled in hardware, and the BARs may be zeroed/reset in hardware. 
>>> However, Xen
>>> vPCI still thinks memory decoding is enabled, and BARs will remain mapped in
>>> p2m. In other words, memory decoding may become disabled and BARs reset in
>>> hardware, bypassing the respective vPCI command and BAR register handlers.
>>> Subsequently, when pciback attempts to restore state to the device, 
>>> including
>>> BARs, it happens to write the BARs before writing the command register.
>>> Restoring/writing the BARs silently fails because Xen vPCI mistakenly thinks
>>> memory decoding is enabled.
>>>
>>> Fix the BAR write by allowing it to succeed if the value written is the 
>>> same as
>>> the Xen vPCI stored value. pciback will subsequently restore the command
>>> register and the state of the BARs and memory decoding bit will then be in 
>>> sync
>>> between hardware and vPCI again.
>>>
>>> While here, remove a nearby stray newline.
>>>
>>> Signed-off-by: Stewart Hildebrand 
>>> ---
>>> Do we need similar handling in rom_write()?
>>
>> I think so, if we are to go this route. However, ...
>>
>>> We may consider additionally checking during bar_write() if the memory 
>>> decoding
>>> state has become out of sync between hardware and vPCI. During bar_write(), 
>>> we
>>> would check the device's memory decoding bit, compare it to our vPCI state, 
>>> and
>>> invoke modify_bars() if needed. Please let me know your thoughts.
>>
>> ... iirc we discussed earlier on that doing resets in Dom0 wants
>> communicating to Xen. Any way of resetting by a DomU would likely need
>> intercepting. This way the described situation can be avoided altogether.
>> We may go further and uniformly intercept resets, carrying them out on
>> behalf of Dom0 as well. The main issue is, as with any config-space-
>> based interaction with a device, that there may be device specific ways
>> of resetting.
> 
> I agree with Jan, the plan as I recall was to introduce a new
> hypercall to signal Xen of when a device has been reset, I can't
> however find that conversation right now.  It would be nice to trap
> device reset attempts, but there are some device specific reset
> methods that would be too much special casing to accommodate sadly.
> 
> The fix here is just papering over the issue, as if the device has
> been reset and Xen is not aware of it all the vPCI cached state is out
> of date, so it's not only BARs addresses that are stale, but also
> possibly MSI and MSI-X.
> 
> Thanks, Roger.

Ah, right, this makes sense. Sorry I missed that. I agree vPCI needs to know 
when the device has been reset and handle the state accordingly.

Found the thread: 
https://lists.xenproject.org/archives/html/xen-devel/2023-03/msg01687.html

[RFC 3/4] x86/hvm: quirks: address MISRA C:2012 Rule 9.3

[Nicola Vetrini]

Partially explicitly initalized .matches arrays result in violations
of Rule 9.3; this is resolved by using designated initializers,
which is permitted by the Rule.

Mechanical changes.

Signed-off-by: Nicola Vetrini 
---
 xen/arch/x86/hvm/quirks.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hvm/quirks.c b/xen/arch/x86/hvm/quirks.c
index bd30b0f881cb..75c3fdc87749 100644
--- a/xen/arch/x86/hvm/quirks.c
+++ b/xen/arch/x86/hvm/quirks.c
@@ -37,40 +37,40 @@ static int __init cf_check check_port80(void)
 .callback = dmi_hvm_deny_port80,
 .ident= "Compaq Presario V6000",
 .matches  = {
-DMI_MATCH(DMI_BOARD_VENDOR, "Quanta"),
-DMI_MATCH(DMI_BOARD_NAME,   "30B7")
+[0] = DMI_MATCH(DMI_BOARD_VENDOR, "Quanta"),
+[1] = DMI_MATCH(DMI_BOARD_NAME,   "30B7")
 }
 },
 {
 .callback = dmi_hvm_deny_port80,
 .ident= "HP Pavilion dv9000z",
 .matches  = {
-DMI_MATCH(DMI_BOARD_VENDOR, "Quanta"),
-DMI_MATCH(DMI_BOARD_NAME,   "30B9")
+[0] = DMI_MATCH(DMI_BOARD_VENDOR, "Quanta"),
+[1] = DMI_MATCH(DMI_BOARD_NAME,   "30B9")
 }
 },
 {
 .callback = dmi_hvm_deny_port80,
 .ident= "HP Pavilion dv6000",
 .matches  = {
-DMI_MATCH(DMI_BOARD_VENDOR, "Quanta"),
-DMI_MATCH(DMI_BOARD_NAME,   "30B8")
+[0] = DMI_MATCH(DMI_BOARD_VENDOR, "Quanta"),
+[1] = DMI_MATCH(DMI_BOARD_NAME,   "30B8")
 }
 },
 {
 .callback = dmi_hvm_deny_port80,
 .ident= "HP Pavilion tx1000",
 .matches  = {
-DMI_MATCH(DMI_BOARD_VENDOR, "Quanta"),
-DMI_MATCH(DMI_BOARD_NAME,   "30BF")
+[0] = DMI_MATCH(DMI_BOARD_VENDOR, "Quanta"),
+[1] = DMI_MATCH(DMI_BOARD_NAME,   "30BF")
 }
 },
 {
 .callback = dmi_hvm_deny_port80,
 .ident= "Presario F700",
 .matches  = {
-DMI_MATCH(DMI_BOARD_VENDOR, "Quanta"),
-DMI_MATCH(DMI_BOARD_NAME,   "30D3")
+[0] = DMI_MATCH(DMI_BOARD_VENDOR, "Quanta"),
+[1] = DMI_MATCH(DMI_BOARD_NAME,   "30D3")
 }
 },
 { }
-- 
2.34.1

[RFC 1/4] x86/ioemul: address MISRA C:2012 Rule 9.3

[Nicola Vetrini]

Partially explicitly initalized .matches arrays result in violations
of Rule 9.3; this is resolved by using designated initializers,
which is permitted by the Rule.

Mechanical changes.

Signed-off-by: Nicola Vetrini 
---
 xen/arch/x86/ioport_emulate.c | 32 
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/ioport_emulate.c b/xen/arch/x86/ioport_emulate.c
index 6caeb3d470ce..4f8d5136746d 100644
--- a/xen/arch/x86/ioport_emulate.c
+++ b/xen/arch/x86/ioport_emulate.c
@@ -44,57 +44,57 @@ static const struct dmi_system_id __initconstrel 
ioport_quirks_tbl[] = {
 {
 .ident = "HP ProLiant DL3xx",
 .matches = {
-DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
-DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL3"),
+[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
+[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL3"),
 },
 },
 {
 .ident = "HP ProLiant DL5xx",
 .matches = {
-DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
-DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL5"),
+[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
+[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL5"),
 },
 },
 {
 .ident = "HP ProLiant DL7xx",
 .matches = {
-DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
-DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL7"),
+[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
+[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL7"),
 },
 },
 {
 .ident = "HP ProLiant ML3xx",
 .matches = {
-DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
-DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML3"),
+[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
+[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML3"),
 },
 },
 {
 .ident = "HP ProLiant ML5xx",
 .matches = {
-DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
-DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML5"),
+[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
+[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML5"),
 },
 },
 {
 .ident = "HP ProLiant BL2xx",
 .matches = {
-DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
-DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL2"),
+[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
+[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL2"),
 },
 },
 {
 .ident = "HP ProLiant BL4xx",
 .matches = {
-DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
-DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL4"),
+[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
+[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL4"),
 },
 },
 {
 .ident = "HP ProLiant BL6xx",
 .matches = {
-DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
-DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL6"),
+[0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
+[1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL6"),
 },
 },
 { }
--
2.34.1

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

[Jan Beulich]

On 24.10.2023 15:40, Nicola Vetrini wrote:
> On 24/10/2023 10:12, Jan Beulich wrote:
>> On 24.10.2023 09:58, Nicola Vetrini wrote:
>>> On 24/10/2023 09:32, Jan Beulich wrote:
 On 23.10.2023 11:56, Nicola Vetrini wrote:
> --- a/xen/arch/x86/include/asm/asm_defns.h
> +++ b/xen/arch/x86/include/asm/asm_defns.h
> @@ -31,6 +31,7 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, "
>   * gets set up by the containing function.
>   */
>  #ifdef CONFIG_FRAME_POINTER
> +/* SAF-1-safe */
>  register unsigned long current_stack_pointer asm("rsp");
>  # define ASM_CALL_CONSTRAINT , "+r" (current_stack_pointer)
>  #else

 SAF-1-safe is about symbols "used only by asm modules". This doesn't
 apply
 to the declaration here.

>>>
>>> The wording could change to "asm code" if that is deemed clearer.
>>
>> Question is what would be meant by "asm code"; "asm modules" is quite
>> clear.
>>
> 
> Well, I don't know. It's up to the community to decide that. It can be 
> an ad-hoc
> justification, but I don't see much value in doing so. What do you 
> propose to get this patch
> approved (at least on your account)?.

Drop this change and have Eclair recognize that what we're talking
about here is just a declaration, not a definition.

Jan

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

[Jan Beulich]

On 24.10.2023 15:31, Julien Grall wrote:
> Hi Stefano,
> 
> On 23/10/2023 21:47, Stefano Stabellini wrote:
>> On Mon, 23 Oct 2023, Jan Beulich wrote:
>>> On 21.10.2023 01:26, Stefano Stabellini wrote:
 On Fri, 20 Oct 2023, Jan Beulich wrote:
> On 19.10.2023 18:19, Stefano Stabellini wrote:
>> On Thu, 19 Oct 2023, Jan Beulich wrote:
>>> On 19.10.2023 02:44, Stefano Stabellini wrote:
 On Wed, 18 Oct 2023, Jan Beulich wrote:
> On 18.10.2023 02:48, Stefano Stabellini wrote:
>> On Mon, 16 Oct 2023, Jan Beulich wrote:
>>> On 29.09.2023 00:24, Stefano Stabellini wrote:
 If it is not a MISRA requirement, then I think we should go for 
 the path
 of least resistance and try to make the smallest amount of changes
 overall, which seems to be:
>>>
>>> ... "least resistance" won't gain us much, as hardly any guards 
>>> don't
>>> start with an underscore.
>>>
 - for xen/include/blah.h, __BLAH_H__
 - for xen/arch/arm/asm/include/blah.h, __ASM_ARM_BLAH_H__
 - for xen/arch/x86/asm/include/blah.h, it is far less consistent, 
 maybe __ASM_X86_BLAH_H__ ?
>>>
>>> There are no headers in xen/include/. For (e.g.) xen/include/xen/ we
>>> may go with XEN_BLAH_H; whether ASM prefixes are needed I'm not 
>>> sure;
>>> we could go with just ARM_BLAH_H and X86_BLAH_H?
>>>
>>> The primary question though is (imo) how to deal with private 
>>> headers,
>>> such that the risk of name collisions is as small as possible.
>>
>> Looking at concrete examples under xen/include/xen:
>> xen/include/xen/mm.h __XEN_MM_H__
>> xen/include/xen/dm.h __XEN_DM_H__
>> xen/include/xen/hypfs.h __XEN_HYPFS_H__
>>
>> So I think we should do for consistency:
>> xen/include/xen/blah.h __XEN_BLAH_H__
>>
>> Even if we know the leading underscore are undesirable, in this case 
>> I
>> would prefer consistency.
>
> I'm kind of okay with that. FTAOD - here and below you mean to make 
> this
> one explicit first exception from the "no new leading underscores" 
> goal,
> for newly added headers?

 Yes. The reason is for consistency with the existing header files.


>> On the other hand looking at ARM examples:
>> xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__
>> xen/arch/arm/include/asm/time.h __ARM_TIME_H__
>> xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H
>> xen/arch/arm/include/asm/io.h _ASM_IO_H
>>
>> And also looking at x86 examples:
>> xen/arch/x86/include/asm/paging.h _XEN_PAGING_H
>> xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H
>> xen/arch/x86/include/asm/io.h _ASM_IO_H
>>
>> Thet are very inconsistent.
>>
>>
>> So for ARM and X86 headers I think we are free to pick anything we 
>> want,
>> including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine by
>> me.
>
> To be honest, I'd prefer a global underlying pattern, i.e. if common
> headers are "fine" to use leading underscores for guards, arch header
> should, too.

 I am OK with that too. We could go with:
 __ASM_ARM_BLAH_H__
 __ASM_X86_BLAH_H__

 I used "ASM" to make it easier to differentiate with the private 
 headers
 below. Also the version without "ASM" would work but it would only
 differ with the private headers in terms of leading underscores. I
 thought that also having "ASM" would help readability and help avoid
 confusion.


>> For private headers such as:
>> xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__
>> xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_
>> xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__
>> xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H
>>
>> More similar but still inconsistent. I would go with ARCH_ARM_BLAH_H 
>> and
>> ARCH_X86_BLAH_H for new headers.
>
> I'm afraid I don't like this, as deeper paths would lead to unwieldy
> guard names. If we continue to use double-underscore prefixed names
> in common and arch headers, why don't we demand no leading underscores
> and no path-derived prefixes in private headers? That'll avoid any
> collisions between the two groups.

 OK, so for private headers:

 ARM_BLAH_H
 X86_BLAH_H

 What that work for you?
>>>
>>> What are the ARM_ and X86_ prefixes supposed to indicate here? Or to ask
>>> differently, how would you see e.g. 

Re: [PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode

[Paul Durrant]

On 16/10/2023 16:19, David Woodhouse wrote:

From: David Woodhouse 

The primary console is special because the toolstack maps a page at a
fixed GFN and also allocates the guest-side event channel. Add support
for that in emulated mode, so that we can have a primary console.

Add a *very* rudimentary stub of foriegnmem ops for emulated mode, which
supports literally nothing except a single-page mapping of the console
page. This might as well have been a hack in the xen_console driver, but
this way at least the special-casing is kept within the Xen emulation
code, and it gives us a hook for a more complete implementation if/when
we ever do need one.

Why can't you map the console page via the grant table like the xenstore 
page?


  Paul

[libvirt test] 183505: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-libvirt-raw 13 guest-start fail pass in 183512-retest

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

version targeted for testing:
 libvirt  1622012cc42b983300015a6661b1ca67d40376ad
baseline version:
 libvirt  6be0d1a0d3f812cdf0b310e5e36bf3533a9b4ae4

Last test of basis   183451  2023-10-21 04:18:54 Z3 days
Testing same since   183505  2023-10-24 04:20:34 Z0 days1 attempts


People who touched revisions under test:
  Laine Stump 

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 fail
 test-amd64-i386-libvirt-raw  pass
 test-amd64-amd64-libvirt-vhd pass



sg-report-flight on 

Re: [PATCH 09/12] hw/xen: prevent duplicate device registrations

[Paul Durrant]

On 16/10/2023 16:19, David Woodhouse wrote:

From: David Woodhouse 

Ensure that we have a XenBackendInstance for every device regardless
of whether it was "discovered" in XenStore or created directly in QEMU.

This allows the backend_list to be a source of truth about whether a
given backend exists, and allows us to reject duplicates.

This also cleans up the fact that backend drivers were calling
xen_backend_set_device() with a XenDevice immediately after calling
qdev_realize_and_unref() on it, when it wasn't theirs to play with any
more.

Signed-off-by: David Woodhouse 
---
  hw/block/xen-block.c |  1 -
  hw/char/xen_console.c|  2 +-
  hw/xen/xen-backend.c | 78 ++--
  hw/xen/xen-bus.c |  8 
  include/hw/xen/xen-backend.h |  3 ++
  5 files changed, 69 insertions(+), 23 deletions(-)

diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index a07cd7eb5d..9262338535 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -975,7 +975,6 @@ static void xen_block_device_create(XenBackendInstance 
*backend,
  goto fail;
  }
  
-xen_backend_set_device(backend, xendev);

  return;
  
  fail:

diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index bd20be116c..2825b8c511 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -468,7 +468,7 @@ static void xen_console_device_create(XenBackendInstance 
*backend,
  Chardev *cd = NULL;
  struct qemu_xs_handle *xsh = xenbus->xsh;
  
-if (qemu_strtoul(name, NULL, 10, )) {

+if (qemu_strtoul(name, NULL, 10, ) || number >= INT_MAX) {
  error_setg(errp, "failed to parse name '%s'", name);
  goto fail;
  }
I don't think this hunk belongs here, does it? Seems like it should be 
in patch 7.



diff --git a/hw/xen/xen-backend.c b/hw/xen/xen-backend.c
index b9bf70a9f5..dcb4329258 100644
--- a/hw/xen/xen-backend.c
+++ b/hw/xen/xen-backend.c
@@ -101,22 +101,28 @@ static XenBackendInstance 
*xen_backend_list_find(XenDevice *xendev)
  return NULL;
  }
  
-bool xen_backend_exists(const char *type, const char *name)

+static XenBackendInstance *xen_backend_lookup(const XenBackendImpl *impl, 
const char *name)


This name is a little close to xen_backend_table_lookup()... perhaps 
that one should be renamed xen_backend_impl_lookup() for clarity.



  {
-const XenBackendImpl *impl = xen_backend_table_lookup(type);
  XenBackendInstance *backend;
  
-if (!impl) {

-return false;
-}
-
  QLIST_FOREACH(backend, _list, entry) {
  if (backend->impl == impl && !strcmp(backend->name, name)) {
-return true;
+return backend;
  }
  }
  
-return false;

+return NULL;
+}
+
+bool xen_backend_exists(const char *type, const char *name)
+{
+const XenBackendImpl *impl = xen_backend_table_lookup(type);
+
+if (!impl) {
+return false;
+}
+
+return !!xen_backend_lookup(impl, name);
  }
  
  static void xen_backend_list_remove(XenBackendInstance *backend)

@@ -138,11 +144,10 @@ void xen_backend_device_create(XenBus *xenbus, const char 
*type,
  backend = g_new0(XenBackendInstance, 1);
  backend->xenbus = xenbus;
  backend->name = g_strdup(name);
-
-impl->create(backend, opts, errp);
-
  backend->impl = impl;
  xen_backend_list_add(backend);
+
+impl->create(backend, opts, errp);
  }
  
  XenBus *xen_backend_get_bus(XenBackendInstance *backend)

@@ -155,13 +160,6 @@ const char *xen_backend_get_name(XenBackendInstance 
*backend)
  return backend->name;
  }
  
-void xen_backend_set_device(XenBackendInstance *backend,

-XenDevice *xendev)
-{
-g_assert(!backend->xendev);
-backend->xendev = xendev;
-}
-


The declaration also needs removing from xen-backend.h presumably.


  XenDevice *xen_backend_get_device(XenBackendInstance *backend)
  {
  return backend->xendev;
@@ -178,9 +176,7 @@ bool xen_backend_try_device_destroy(XenDevice *xendev, 
Error **errp)
  }
  
  impl = backend->impl;

-if (backend->xendev) {
-impl->destroy(backend, errp);
-}
+impl->destroy(backend, errp);
  
  xen_backend_list_remove(backend);

  g_free(backend->name);
@@ -188,3 +184,43 @@ bool xen_backend_try_device_destroy(XenDevice *xendev, 
Error **errp)
  
  return true;

  }
+
+bool xen_backend_device_realized(XenDevice *xendev, Error **errp)
+{
+XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
+const char *type = xendev_class->backend ? : 
object_get_typename(OBJECT(xendev));
+const XenBackendImpl *impl = xen_backend_table_lookup(type);
+XenBackendInstance *backend;
+
+if (!impl) {
+return false;
+}
+
+backend = xen_backend_lookup(impl, xendev->name);
+if (backend) {
+if (backend->xendev && backend->xendev != xendev) {
+error_setg(errp, "device %s/%s already exists", type, 
xendev->name);
+  

[XEN PATCH] xen/iommu_init: address a violation of MISRA C:2012 Rule 8.3

[Federico Serafini]

Change parameter name and emphasize that it is deliberately not used through a
comment followed by the statement '(void) data;'.
This also addresses a violation of MISRA C:2012 Rule 2.7 ("A function should
not contain unused parameters").

No functional change.

Signed-off-by: Federico Serafini 
---
 xen/drivers/passthrough/amd/iommu_init.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/amd/iommu_init.c 
b/xen/drivers/passthrough/amd/iommu_init.c
index 9c01a49435..5bfaa6cdd4 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -692,7 +692,7 @@ static void iommu_check_ppr_log(struct amd_iommu *iommu)
 spin_unlock_irqrestore(>lock, flags);
 }
 
-static void cf_check do_amd_iommu_irq(void *unused)
+static void cf_check do_amd_iommu_irq(void *data)
 {
 struct amd_iommu *iommu;
 
@@ -702,6 +702,11 @@ static void cf_check do_amd_iommu_irq(void *unused)
 return;
 }
 
+/*
+ * Formal parameter is deliberately unused.
+ */
+(void) data;
+
 /*
  * No matter from where the interrupt came from, check all the
  * IOMMUs present in the system. This allows for having just one
-- 
2.34.1

[PATCH] x86/x2apic: introduce a mixed physical/cluster mode

[Roger Pau Monne]

The current implementation of x2APIC requires to either use Cluster Logical or
Physical mode for all interrupts.  However the selection of Physical vs Logical
is not done at APIC setup, an APIC can be addressed both in Physical or Logical
destination modes concurrently.

Introduce a new x2APIC mode called mixed, which uses Logical Cluster mode for
IPIs, and Physical mode for external interrupts, thus attempting to use the
best method for each interrupt type.

Using Physical mode for external interrupts allows more vectors to be used, and
interrupt balancing to be more accurate.

Using Logical Cluster mode for IPIs allows less accesses to the ICR register
when sending those, as multiple CPUs can be targeted with a single ICR register
write.

A simple test calling flush_tlb_all() 1 times in a tight loop on a 96 CPU
box gives the following average figures:

Physical mode: 26617931ns
Mixed mode:23865337ns

So ~10% improvement versus plain Physical mode.  Note that Xen uses Cluster
mode by default, and hence is already using the fastest way for IPI delivery at
the cost of reducing the amount of vectors available system-wide.

Make the newly introduced mode the default one.

Suggested-by: Andrew Cooper 
Signed-off-by: Roger Pau Monné 
---
Do we want to keep a full Logical Cluster mode available?  I don't see a reason
to target external interrupts in Logical Cluster mode, but maybe there's
something I'm missing.

It's not clear to me whether the ACPI FADT flags are meant to apply only to
external interrupt delivery mode, or also to IPI delivery.  If
ACPI_FADT_APIC_PHYSICAL is only meant to apply to external interrupts and not
IPIs then we could possibly get rid of physical mode IPI delivery.

Still need to put this under XenServer extensive testing, but wanted to get
some feedback before in case approach greatly changes.

Based on top of 'x86/x2apic: remove usage of ACPI_FADT_APIC_CLUSTER'
---
 docs/misc/xen-command-line.pandoc | 11 
 xen/arch/x86/Kconfig  | 37 ++---
 xen/arch/x86/genapic/x2apic.c | 87 ++-
 3 files changed, 104 insertions(+), 31 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc 
b/docs/misc/xen-command-line.pandoc
index 6b07d0f3a17f..09c6f17b4b02 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2802,6 +2802,14 @@ the watchdog.
 
 Permit use of x2apic setup for SMP environments.
 
+### x2apic-mode (x86)
+> `= physical | cluster | mixed`
+
+> Default: `physical` if **FADT** mandates physical mode, `mixed` otherwise.
+
+In the case that x2apic is in use, this option switches between modes to
+address APICs in the system as interrupt destinations.
+
 ### x2apic_phys (x86)
 > `= `
 
@@ -2812,6 +2820,9 @@ In the case that x2apic is in use, this option switches 
between physical and
 clustered mode.  The default, given no hint from the **FADT**, is cluster
 mode.
 
+**WARNING: `x2apic_phys` is deprecated and superseded by `x2apic-mode`.
+The later takes precedence if both are set.**
+
 ### xenheap_megabytes (arm32)
 > `= `
 
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index eac77573bd75..a44d63b5c8dc 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -228,11 +228,18 @@ config XEN_ALIGN_2M
 
 endchoice
 
-config X2APIC_PHYSICAL
-   bool "x2APIC Physical Destination mode"
-   help
- Use x2APIC Physical Destination mode by default when available.
+choice
+   prompt "x2APIC Destination mode"
+   default X2APIC_MIXED
+   ---help---
+ Select APIC addressing when x2APIC is enabled.
 
+ The default mode is mixed which should provide the best aspects
+ of both physical and cluster modes.
+
+config X2APIC_PHYSICAL
+   tristate "Physical Destination mode"
+   ---help---
  When using this mode APICs are addressed using the Physical
  Destination mode, which allows using all dynamic vectors on each
  CPU independently.
@@ -242,9 +249,27 @@ config X2APIC_PHYSICAL
  destination inter processor interrupts (IPIs) slightly slower than
  Logical Destination mode.
 
- The mode when this option is not selected is Logical Destination.
+config X2APIC_CLUSTER
+   tristate "Cluster Destination mode"
+   ---help---
+ When using this mode APICs are addressed using the Cluster Logical
+ Destination mode.
 
- If unsure, say N.
+ Cluster Destination has the benefit of sending IPIs faster since
+ multiple APICs can be targeted as destinations of a single IPI.
+ However the vector space is shared between all CPUs on the cluster,
+ and hence using this mode reduces the number of available vectors
+ when compared to Physical mode.
+
+config X2APIC_MIXED
+   tristate "Mixed Destination mode"
+   ---help---
+ When using this mode APICs are addressed using the Cluster Logical
+ Destination mode for 

[XEN PATCH][for-4.19 v3] xen: Add deviations for MISRA C:2012 Rule 7.1

[Nicola Vetrini]

As specified in rules.rst, these constants can be used
in the code.

Signed-off-by: Nicola Vetrini 
---
Changes in v2:
- replace some SAF deviations with configurations
Changes in v3:
- refine configurations and justifications
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 10 ++
 docs/misra/deviations.rst|  5 +
 docs/misra/safe.json |  8 
 xen/arch/x86/hvm/svm/emulate.c   |  6 +++---
 xen/common/inflate.c |  4 ++--
 5 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
b/automation/eclair_analysis/ECLAIR/deviations.ecl
index fa56e5c00a27..ea5e0eb1813f 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -85,10 +85,12 @@ conform to the directive."
 # Series 7.
 #
 
--doc_begin="Usage of the following constants is safe, since they are given 
as-is
-in the inflate algorithm specification and there is therefore no risk of them
-being interpreted as decimal constants."
--config=MC3R1.R7.1,literals={safe, 
"^0(007|37|070|213|236|300|321|330|331|332|333|334|335|337|371)$"}
+-doc_begin="It is safe to use certain octal constants the way they are defined 
in
+specifications, manuals, and algorithm descriptions."
+-file_tag+={x86_svm_h, "^xen/arch/x86/hvm/svm/svm\\.h$"}
+-file_tag+={x86_emulate_c, "^xen/arch/x86/hvm/svm/emulate\\.c$"}
+-config=MC3R1.R7.1,reports+={safe, 
"any_area(any_loc(any_exp(file(x86_svm_h)&(^INSTR_ENC$"}
+-config=MC3R1.R7.1,reports+={safe, 
"any_area(text(^.*octal-ok.*$)&_loc(any_exp(file(x86_emulate_c)&(^MASK_EXTR$"}
 -doc_end
 
 -doc_begin="Violations in files that maintainers have asked to not modify in 
the
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 8511a189253b..26c6dbbc9ffe 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -90,6 +90,11 @@ Deviations related to MISRA C:2012 Rules:
  - __emulate_2op and __emulate_2op_nobyte
  - read_debugreg and write_debugreg
 
+   * - R7.1
+ - It is safe to use certain octal constants the way they are defined in
+   specifications, manuals, and algorithm descriptions.
+ - Tagged as `safe` for ECLAIR.
+
* - R7.2
  - Violations caused by __HYPERVISOR_VIRT_START are related to the
particular use of it done in xen_mk_ulong.
diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 39c5c056c7d4..7ea47344ffcc 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -20,6 +20,14 @@
 },
 {
 "id": "SAF-2-safe",
+"analyser": {
+"eclair": "MC3R1.R7.1"
+},
+"name": "Rule 7.1: constants defined in specifications, manuals, 
and algorithm descriptions",
+"text": "It is safe to use certain octal constants the way they 
are defined in specifications, manuals, and algorithm descriptions."
+},
+{
+"id": "SAF-3-safe",
 "analyser": {},
 "name": "Sentinel",
 "text": "Next ID to be used"
diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
index aa2c61c433b3..93ac1d3435f9 100644
--- a/xen/arch/x86/hvm/svm/emulate.c
+++ b/xen/arch/x86/hvm/svm/emulate.c
@@ -90,9 +90,9 @@ unsigned int svm_get_insn_len(struct vcpu *v, unsigned int 
instr_enc)
 if ( !instr_modrm )
 return emul_len;
 
-if ( modrm_mod   == MASK_EXTR(instr_modrm, 0300) &&
- (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
- (modrm_rm  & 7) == MASK_EXTR(instr_modrm, 0007) )
+if ( modrm_mod   == MASK_EXTR(instr_modrm, 0300) && /* octal-ok */
+ (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && /* octal-ok */
+ (modrm_rm  & 7) == MASK_EXTR(instr_modrm, 0007) )  /* octal-ok */
 return emul_len;
 }
 
diff --git a/xen/common/inflate.c b/xen/common/inflate.c
index 8fa4b96d12a3..be6a9115187e 100644
--- a/xen/common/inflate.c
+++ b/xen/common/inflate.c
@@ -1201,8 +1201,8 @@ static int __init gunzip(void)
 magic[1] = NEXTBYTE();
 method   = NEXTBYTE();
 
-if (magic[0] != 037 ||
-((magic[1] != 0213) && (magic[1] != 0236))) {
+/* SAF-2-safe */
+if (magic[0] != 037 || ((magic[1] != 0213) && (magic[1] != 0236))) {
 error("bad gzip magic numbers");
 return -1;
 }
-- 
2.34.1

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

[David Woodhouse]

On Tue, 2023-10-24 at 14:08 +0200, Juergen Gross wrote:
> 
> > I can probably change xen_send_IPI_one() to not need irq_get_chip_data().
> 
> David, could you test the attached patch, please? Build tested only.

No longer whines when offlining CPU1.

Still triple-faults when bringing it back online. Or if I remove the
lockdep_assert_irqs_disabled() from load_current_idt(), gives the same
warnings as before, all on the same theme about IRQs being enabled when
they shouldn't be.

[root@localhost cpu1]# echo 1 > online 
[   26.435377] installing Xen timer for CPU 1
[   26.437493] smpboot: Booting Node 0 Processor 1 APIC 0x2
[   26.438830] [ cut here ]
[   26.438842] WARNING: CPU: 1 PID: 0 at arch/x86/kernel/cpu/common.c:454 
cr4_update_irqsoff+0x2e/0x60
[   26.438858] Modules linked in:
[   26.438864] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 6.6.0-rc5+ #1382
[   26.438870] Hardware name: Xen HVM domU, BIOS 4.17.1 09/26/2023
[   26.438873] RIP: 0010:cr4_update_irqsoff+0x2e/0x60
[   26.438881] Code: 89 f7 65 48 8b 15 ba 07 fd 7d 8b 0d 54 28 dc 01 85 c9 74 
18 65 8b 0d 51 f5 fb 7d 85 c9 75 0d 65 8b 0d 42 f3 fb 7d 85 c9 74 02 <0f> 0b 48 
f7 d7 48 21 d7 48 09 c7 48 39 fa 75 05 c3 cc cc cc cc 65
[   26.438885] RSP: :b245000aff10 EFLAGS: 00010002
[   26.438891] RAX:  RBX:  RCX: 0001
[   26.438895] RDX: 001300a0 RSI: 000f RDI: 000f
[   26.438898] RBP: 9002c1398000 R08: fe51 R09: 
[   26.438902] R10: 90030e106078 R11:  R12: 0001
[   26.438905] R13:  R14:  R15: 
[   26.438908] FS:  () GS:90030e10() 
knlGS:
[   26.438912] CS:  0010 DS:  ES:  CR0: 80050033
[   26.438916] CR2:  CR3: 4483c000 CR4: 001300a0
[   26.438923] Call Trace:
[   26.438927]  
[   26.438931]  ? cr4_update_irqsoff+0x2e/0x60
[   26.438937]  ? __warn+0x81/0x170
[   26.438951]  ? cr4_update_irqsoff+0x2e/0x60
[   26.438959]  ? report_bug+0x18d/0x1c0
[   26.438974]  ? handle_bug+0x3c/0x80
[   26.438983]  ? exc_invalid_op+0x13/0x60
[   26.438990]  ? asm_exc_invalid_op+0x16/0x20
[   26.439015]  ? cr4_update_irqsoff+0x2e/0x60
[   26.439024]  cpu_init+0x54/0x1a0
[   26.439034]  start_secondary+0x2e/0x140
[   26.439044]  secondary_startup_64_no_verify+0x178/0x17b
[   26.439071]  
[   26.439074] irq event stamp: 109567
[   26.439077] hardirqs last  enabled at (109567): [] 
acpi_idle_play_dead+0x46/0x70
[   26.439087] hardirqs last disabled at (109566): [] 
do_idle+0x90/0xe0
[   26.439094] softirqs last  enabled at (109502): [] 
__do_softirq+0x31c/0x423
[   26.439105] softirqs last disabled at (109489): [] 
__irq_exit_rcu+0x91/0x100
[   26.439111] ---[ end trace  ]---
[   26.439141] [ cut here ]
[   26.439145] WARNING: CPU: 1 PID: 0 at kernel/rcu/tree.c:4429 
rcu_cpu_starting+0x192/0x1f0
[   26.439159] Modules linked in:
[   26.439164] CPU: 1 PID: 0 Comm: swapper/1 Tainted: GW  
6.6.0-rc5+ #1382
[   26.439168] Hardware name: Xen HVM domU, BIOS 4.17.1 09/26/2023
[   26.439171] RIP: 0010:rcu_cpu_starting+0x192/0x1f0
[   26.439179] Code: ff 65 8b 05 80 7c e9 7d 85 c0 75 af 65 8b 05 71 7a e9 7d 
85 c0 74 a4 0f 0b eb a0 65 8b 05 62 7a e9 7d 85 c0 0f 84 94 fe ff ff <0f> 0b e9 
8d fe ff ff 89 c6 4c 89 f7 e8 9d 71 b6 00 90 e9 c0 fe ff
[   26.439182] RSP: :b245000aff08 EFLAGS: 00010002
[   26.439188] RAX: 0001 RBX:  RCX: 
[   26.439191] RDX: 0001 RSI:  RDI: 0001
[   26.439194] RBP:  R08: 0383c000 R09: 
[   26.439197] R10: 90030e106078 R11:  R12: 
[   26.439200] R13:  R14:  R15: 
[   26.439203] FS:  () GS:90030e10() 
knlGS:
[   26.439206] CS:  0010 DS:  ES:  CR0: 80050033
[   26.439209] CR2:  CR3: 4483c001 CR4: 001706a0
[   26.439213] Call Trace:
[   26.439215]  
[   26.439218]  ? rcu_cpu_starting+0x192/0x1f0
[   26.439225]  ? __warn+0x81/0x170
[   26.439234]  ? rcu_cpu_starting+0x192/0x1f0
[   26.439244]  ? report_bug+0x18d/0x1c0
[   26.439256]  ? handle_bug+0x3c/0x80
[   26.439262]  ? exc_invalid_op+0x13/0x60
[   26.439269]  ? asm_exc_invalid_op+0x16/0x20
[   26.439290]  ? rcu_cpu_starting+0x192/0x1f0
[   26.439306]  start_secondary+0x3f/0x140
[   26.439314]  secondary_startup_64_no_verify+0x178/0x17b
[   26.439338]  
[   26.439340] irq event stamp: 109567
[   26.439343] hardirqs last  enabled at (109567): [] 
acpi_idle_play_dead+0x46/0x70
[   26.439349] hardirqs last disabled at (109566): [] 
do_idle+0x90/0xe0
[   26.439354] softirqs last  enabled at (109502): [] 
__do_softirq+0x31c/0x423
[   

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

[Nicola Vetrini]

On 24/10/2023 10:14, Jan Beulich wrote:

On 24.10.2023 10:01, Nicola Vetrini wrote:

On 24/10/2023 09:50, Jan Beulich wrote:

On 23.10.2023 11:56, Nicola Vetrini wrote:

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

Signed-off-by: Nicola Vetrini 
Reviewed-by: Stefano Stabellini 
---
Changes in v3:
- added SAF deviations for vmx counterparts to svm functions.


Same comment regarding the R-b here as for patch 2.


--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -123,6 +123,7 @@ static void svm_enable_intr_window(struct vcpu 
*v,

struct hvm_intack intack)
 vmcb, general1_intercepts | GENERAL1_INTERCEPT_VINTR);
 }

+/* SAF-1-safe */
 void svm_intr_assist(void)
 {
 struct vcpu *v = current;


Linux has the concept of "asmlinkage" for functions interfacing C and
assembly. Was it considered to use that - even if expanding to 
nothing

for all present architectures - as a way to annotate affected
definitions
in place of the SAF-*-safe comments?


It was proposed by Julien a while ago (I think it the thread on
deviations.rst) to define
a macro asmcall that expands to nothing, to mark all such functions.
Right now, it's not
strictly necessary (given that there are already some uses of SAF in
Stefano's for-4.19 branch.


Can this then be revisited please before any such reaches staging?

Jan


I'll let Stefano answer this one.

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

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

[Nicola Vetrini]

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

On 24.10.2023 09:58, Nicola Vetrini wrote:

On 24/10/2023 09:32, Jan Beulich wrote:

On 23.10.2023 11:56, Nicola Vetrini wrote:

--- a/xen/arch/x86/include/asm/asm_defns.h
+++ b/xen/arch/x86/include/asm/asm_defns.h
@@ -31,6 +31,7 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, "
  * gets set up by the containing function.
  */
 #ifdef CONFIG_FRAME_POINTER
+/* SAF-1-safe */
 register unsigned long current_stack_pointer asm("rsp");
 # define ASM_CALL_CONSTRAINT , "+r" (current_stack_pointer)
 #else


SAF-1-safe is about symbols "used only by asm modules". This doesn't
apply
to the declaration here.



The wording could change to "asm code" if that is deemed clearer.


Question is what would be meant by "asm code"; "asm modules" is quite
clear.



Well, I don't know. It's up to the community to decide that. It can be 
an ad-hoc
justification, but I don't see much value in doing so. What do you 
propose to get this patch

approved (at least on your account)?.


--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -75,6 +75,7 @@ static bool __initdata opt_invpcid = true;
 boolean_param("invpcid", opt_invpcid);
 bool __read_mostly use_invpcid;

+/* SAF-1-safe Only used in asm code and within this source file */
 unsigned long __read_mostly cr4_pv32_mask;

 /*  Linux config option: propagated to domain0. */
@@ -147,12 +148,13 @@ cpumask_t __read_mostly cpu_present_map;
 unsigned long __read_mostly xen_phys_start;

 char __section(".init.bss.stack_aligned") __aligned(STACK_SIZE)
-cpu0_stack[STACK_SIZE];
+cpu0_stack[STACK_SIZE]; /* SAF-1-safe Only used in asm code and
below */


Wasn't it that such comments need to live on the earlier line?


On the same line is fine as well. I personally found it less clear
putting that in the
line above.


But please recall that these comments are intended to cover other
scanners as well. Iirc only Eclair accepts comments on the same line.
Nevertheless I realize that putting the comment on the earlier line
is problematic (and maybe also scanner dependent) when that ends up
in the middle of a declaration / definition.

Jan


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

Re: [PATCH 06/12] hw/xen: add get_frontend_path() method to XenDeviceClass

[Paul Durrant]

On 24/10/2023 14:29, David Woodhouse wrote:

On Tue, 2023-10-24 at 13:59 +0100, Paul Durrant wrote:

On 24/10/2023 13:56, David Woodhouse wrote:

On Tue, 2023-10-24 at 13:42 +0100, Paul Durrant wrote:



--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -711,8 +711,16 @@ static void xen_device_frontend_create(XenDevice *xendev, 
Error **errp)
     {
     ERRP_GUARD();
     XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
+    XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
 
-    xendev->frontend_path = xen_device_get_frontend_path(xendev);

+    if (xendev_class->get_frontend_path) {
+    xendev->frontend_path = xendev_class->get_frontend_path(xendev, errp);
+    if (!xendev->frontend_path) {
+    return;


I think you need to update errp here to note that you are failing to
create the frontend.


If xendev_class->get_frontend_path returned NULL it will have filled in errp.



Ok, but a prepend to say that a lack of path there means we skip
frontend creation seems reasonable?


No, it *is* returning an error. Perhaps I can make it



I understand it is returning an error. I thought the point of the 
cascading error handling was to prepend text at each (meaningful) layer 
such that the eventual message conveyed what failed and also what the 
consequences of that failure were.


  Paul


 if (!xendev->frontend_path) {
 /*
  * If the ->get_frontend_path() method returned NULL, it will
  * already have set *errp accordingly. Return the error.
  */
 return /* false */;
 }



As a general rule (I'll be doing a bombing run on xen-bus once I get my
patch queue down into single digits) we should never check 'if (*errp)'
to check if a function had an error. It should *also* return a success
or failure indication, and we should cope with errp being NULL.



I'm pretty sure someone told me the exact opposite a few years back.


Then they were wrong :)

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

[Bertrand Marquis]

Hi Julien,

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

Reviewed-by: Bertrand Marquis 

Cheers
Bertrand

> 
> ---
> 
> Only compiled tested. Waiting for the CI to confirm there is no
> regression.
> ---
> xen/arch/arm/arm64/domctl.c | 34 +++---
> 1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
> index 14fc622e9956..8720d126c97d 100644
> --- a/xen/arch/arm/arm64/domctl.c
> +++ b/xen/arch/arm/arm64/domctl.c
> @@ -33,27 +33,31 @@ static long switch_mode(struct domain *d, enum 
> domain_type type)
> return 0;
> }
> 
> +static long set_address_size(struct domain *d, uint32_t address_size)
> +{
> +switch ( address_size )
> +{
> +case 32:
> +if ( !cpu_has_el1_32 )
> +return -EINVAL;
> +/* SVE is not supported for 32 bit domain */
> +if ( is_sve_domain(d) )
> +return -EINVAL;
> +return switch_mode(d, DOMAIN_32BIT);
> +case 64:
> +return switch_mode(d, DOMAIN_64BIT);
> +default:
> +return -EINVAL;
> +}
> +}
> +
> long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> {
> switch ( domctl->cmd )
> {
> case XEN_DOMCTL_set_address_size:
> -switch ( domctl->u.address_size.size )
> -{
> -case 32:
> -if ( !cpu_has_el1_32 )
> -return -EINVAL;
> -/* SVE is not supported for 32 bit domain */
> -if ( is_sve_domain(d) )
> -return -EINVAL;
> -return switch_mode(d, DOMAIN_32BIT);
> -case 64:
> -return switch_mode(d, DOMAIN_64BIT);
> -default:
> -return -EINVAL;
> -}
> -break;
> +return set_address_size(d, domctl->u.address_size.size);
> 
> default:
> return -ENOSYS;
> -- 
> 2.40.1
> 
> 

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

[Julien Grall]

Hi Stefano,

On 23/10/2023 21:47, Stefano Stabellini wrote:

On Mon, 23 Oct 2023, Jan Beulich wrote:

On 21.10.2023 01:26, Stefano Stabellini wrote:

On Fri, 20 Oct 2023, Jan Beulich wrote:

On 19.10.2023 18:19, Stefano Stabellini wrote:

On Thu, 19 Oct 2023, Jan Beulich wrote:

On 19.10.2023 02:44, Stefano Stabellini wrote:

On Wed, 18 Oct 2023, Jan Beulich wrote:

On 18.10.2023 02:48, Stefano Stabellini wrote:

On Mon, 16 Oct 2023, Jan Beulich wrote:

On 29.09.2023 00:24, Stefano Stabellini wrote:

If it is not a MISRA requirement, then I think we should go for the path
of least resistance and try to make the smallest amount of changes
overall, which seems to be:


... "least resistance" won't gain us much, as hardly any guards don't
start with an underscore.


- for xen/include/blah.h, __BLAH_H__
- for xen/arch/arm/asm/include/blah.h, __ASM_ARM_BLAH_H__
- for xen/arch/x86/asm/include/blah.h, it is far less consistent, maybe 
__ASM_X86_BLAH_H__ ?


There are no headers in xen/include/. For (e.g.) xen/include/xen/ we
may go with XEN_BLAH_H; whether ASM prefixes are needed I'm not sure;
we could go with just ARM_BLAH_H and X86_BLAH_H?

The primary question though is (imo) how to deal with private headers,
such that the risk of name collisions is as small as possible.


Looking at concrete examples under xen/include/xen:
xen/include/xen/mm.h __XEN_MM_H__
xen/include/xen/dm.h __XEN_DM_H__
xen/include/xen/hypfs.h __XEN_HYPFS_H__

So I think we should do for consistency:
xen/include/xen/blah.h __XEN_BLAH_H__

Even if we know the leading underscore are undesirable, in this case I
would prefer consistency.


I'm kind of okay with that. FTAOD - here and below you mean to make this
one explicit first exception from the "no new leading underscores" goal,
for newly added headers?


Yes. The reason is for consistency with the existing header files.



On the other hand looking at ARM examples:
xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__
xen/arch/arm/include/asm/time.h __ARM_TIME_H__
xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H
xen/arch/arm/include/asm/io.h _ASM_IO_H

And also looking at x86 examples:
xen/arch/x86/include/asm/paging.h _XEN_PAGING_H
xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H
xen/arch/x86/include/asm/io.h _ASM_IO_H

Thet are very inconsistent.


So for ARM and X86 headers I think we are free to pick anything we want,
including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine by
me.


To be honest, I'd prefer a global underlying pattern, i.e. if common
headers are "fine" to use leading underscores for guards, arch header
should, too.


I am OK with that too. We could go with:
__ASM_ARM_BLAH_H__
__ASM_X86_BLAH_H__

I used "ASM" to make it easier to differentiate with the private headers
below. Also the version without "ASM" would work but it would only
differ with the private headers in terms of leading underscores. I
thought that also having "ASM" would help readability and help avoid
confusion.



For private headers such as:
xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__
xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_
xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__
xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H

More similar but still inconsistent. I would go with ARCH_ARM_BLAH_H and
ARCH_X86_BLAH_H for new headers.


I'm afraid I don't like this, as deeper paths would lead to unwieldy
guard names. If we continue to use double-underscore prefixed names
in common and arch headers, why don't we demand no leading underscores
and no path-derived prefixes in private headers? That'll avoid any
collisions between the two groups.


OK, so for private headers:

ARM_BLAH_H
X86_BLAH_H

What that work for you?


What are the ARM_ and X86_ prefixes supposed to indicate here? Or to ask
differently, how would you see e.g. common/decompress.h's guard named?


I meant that:

xen/arch/arm/blah.h would use ARM_BLAH_H
xen/arch/x86/blah.h would use X86_BLAH_H

You have a good question on something like xen/common/decompress.h and
xen/common/event_channel.h.  What do you think about:

COMMON_BLAH_H, so specifically COMMON_DECOMPRESS_H

otherwise:

XEN_BLAH_H, so specifically XEN_DECOMPRESS_H

I prefer COMMON_BLAH_H but I think both versions are OK.


IOW you disagree with my earlier "... and no path-derived prefixes",
and you prefer e.g. DRIVERS_PASSTHROUGH_VTD_DMAR_H as a consequence?
FTAOD my earlier suggestion was simply based on the observation that
the deeper the location of a header in the tree, the more unwieldy
its guard name would end up being if path prefixes were to be used.


I don't have a strong opinion on "path-derived prefixes". I prefer
consistency and easy-to-figure-out guidelines over shortness.


We adopted the MISRA Rule 5.4 which imposed us a limit (40 for Xen) on 
the number of characters for macros. AFAIU, this would apply to guards.


In the example provided by Jan (DRIVERS_PASSTHROUGH_VTD_DMAR_H), this is 
already 31 characters. So this is quite close to the limit.

Re: [PATCH for-4.19 v2] docs/arm: Document where Xen should be loaded in memory

[Bertrand Marquis]

Hi Julien,

> On 24 Oct 2023, at 12:28, Julien Grall  wrote:
> 
> From: Julien Grall 
> 
> In commit 9d267c049d92 ("xen/arm64: Rework the memory layout"),
> we decided to require Xen to be loaded below 2 TiB to simplify
> the logic to enable the MMU. The limit was decided based on
> how known platform boot plus some slack.
> 
> We had a recent report that this is not sufficient on the AVA
> platform with a old firmware [1]. But the restriction is not
> going to change in Xen 4.18. So document the limit clearly
> in docs/misc/arm/booting.txt.
> 
> [1] https://lore.kernel.org/20231013122658.1270506-3-leo@linaro.org
> 
> Signed-off-by: Julien Grall 
> Reviewed-by: Michal Orzel 

Reviewed-by: Bertrand Marquis 

Cheers
Bertrand

> 
> ---
> 
>Changes in v2:
>- The limit is 2 TiB no 5
>- Remove unnecessary sentence in the docs
>- Add missing link
>- Add Michal's reviewed-by
> 
> I couldn't find a nice way to document it in SUPPORT.md. So I decided
> to only document the restrict in docs/misc/arm/booting.txt for now.
> 
> I also couldn't find any way from GRUB/UEFI (I didn't look much) to
> specify the loading address.
> ---
> docs/misc/arm/booting.txt | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
> index 02f7bb65ec6d..547f58a7d981 100644
> --- a/docs/misc/arm/booting.txt
> +++ b/docs/misc/arm/booting.txt
> @@ -21,7 +21,9 @@ The exceptions to this on 32-bit ARM are as follows:
>  zImage protocol should still be used and not the stricter "raw
>  (non-zImage)" protocol described in arm/Booting.
> 
> -There are no exception on 64-bit ARM.
> +The exceptions to this on 64-bit ARM are as follows:
> +
> + Xen binary should be loaded in memory below 2 TiB.
> 
> Booting Guests
> --
> -- 
> 2.40.1
> 

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

[Bertrand Marquis]

Hi Stefano,

> On 23 Oct 2023, at 22:56, Stefano Stabellini  wrote:
> 
> Signed-off-by: Stefano Stabellini 

Acked-by: Bertrand Marquis 

Cheers
Bertrand

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

Re: [PATCH 06/12] hw/xen: add get_frontend_path() method to XenDeviceClass

[David Woodhouse]

On Tue, 2023-10-24 at 13:59 +0100, Paul Durrant wrote:
> On 24/10/2023 13:56, David Woodhouse wrote:
> > On Tue, 2023-10-24 at 13:42 +0100, Paul Durrant wrote:
> > > 
> > > > --- a/hw/xen/xen-bus.c
> > > > +++ b/hw/xen/xen-bus.c
> > > > @@ -711,8 +711,16 @@ static void xen_device_frontend_create(XenDevice 
> > > > *xendev, Error **errp)
> > > >     {
> > > >     ERRP_GUARD();
> > > >     XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
> > > > +    XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
> > > >     
> > > > -    xendev->frontend_path = xen_device_get_frontend_path(xendev);
> > > > +    if (xendev_class->get_frontend_path) {
> > > > +    xendev->frontend_path = 
> > > > xendev_class->get_frontend_path(xendev, errp);
> > > > +    if (!xendev->frontend_path) {
> > > > +    return;
> > > 
> > > I think you need to update errp here to note that you are failing to
> > > create the frontend.
> > 
> > If xendev_class->get_frontend_path returned NULL it will have filled in 
> > errp.
> > 
> 
> Ok, but a prepend to say that a lack of path there means we skip 
> frontend creation seems reasonable?

No, it *is* returning an error. Perhaps I can make it

if (!xendev->frontend_path) {
/*
 * If the ->get_frontend_path() method returned NULL, it will
 * already have set *errp accordingly. Return the error.
 */
return /* false */;
}


> > As a general rule (I'll be doing a bombing run on xen-bus once I get my
> > patch queue down into single digits) we should never check 'if (*errp)'
> > to check if a function had an error. It should *also* return a success
> > or failure indication, and we should cope with errp being NULL.
> > 
> 
> I'm pretty sure someone told me the exact opposite a few years back.

Then they were wrong :)


smime.p7s
Description: S/MIME cryptographic signature

[XEN RFC] xen/automation: add deviations for MISRA C:2012 Rule 8.3

[Federico Serafini]

Update ECLAIR configuration to deviate Rule 8.3 ("All declarations of
an object or function shall use the same names and type qualifiers")
for the following functions:
- set_px_pminfo();
- guest_walk_tables_[0-9]+_levels().

Update file docs/misra/deviations.rst accordingly.
No functional change.

Signed-off-by: Federico Serafini 
---
I had a discussion with Jan about the reasons behind the choice of parameter
name 'walk' for the definitions of functions guest_walk_tables_[0-9]+_levels()
and the parameter name 'pfec' for the corresponding declarations.
Also for the function set_px_pminfo(), it seems that the parameter names are
different on purpose.
Can I submit a patch with these deviations? Do you have any comments?
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 4 
 docs/misra/deviations.rst| 7 +++
 2 files changed, 11 insertions(+)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
b/automation/eclair_analysis/ECLAIR/deviations.ecl
index d8170106b4..9485d66928 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -204,6 +204,10 @@ const-qualified."
 
-config=MC3R1.R8.3,reports+={deliberate,"any_area(any_loc(file(adopted_mpparse_r8_3)))&_area(any_loc(file(^xen/arch/x86/include/asm/mpspec\\.h$)))"}
 -doc_end
 
+-doc_begin="For functions set_px_pminfo() and 
guest_walk_tables_[0-9]+_levels(), parameter names of definitions deliberately 
differ from the ones used in the corresponding declarations."
+-config=MC3R1.R8.3,declarations={deliberate,"^set_px_pminfo\\(uint32_t, struct 
xen_processor_performance\\*\\)|guest_walk_tables_[0-9]+_levels\\(const struct 
vcpu\\*, struct p2m_domain\\*, unsigned long, walk_t\\*, uint32_t, gfn_t, 
mfn_t, void\\*\\)$"}
+-doc_end
+
 -doc_begin="The following variables are compiled in multiple translation units
 belonging to different executables and therefore are safe."
 -config=MC3R1.R8.6,declarations+={safe, 
"name(current_stack_pointer||bsearch||sort)"}
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 8511a18925..b5016412f6 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -121,6 +121,13 @@ Deviations related to MISRA C:2012 Rules:
  - xen/common/unxz.c
  - xen/common/unzstd.c
 
+   * - R8.3
+ - In some cases, parameter names used in the function definition
+   deliberately differ from the ones used in the corresponding declaration.
+ - Tagged as `deliberate` for ECLAIR. Such functions are:
+ - set_px_pminfo()
+ - guest_walk_tables_[0-9]+_levels()
+
* - R8.4
  - The definitions present in the files 'asm-offsets.c' for any 
architecture
are used to generate definitions for asm modules, and are not called by
-- 
2.34.1

Re: [PATCH 02/12] hw/xen: select kernel mode for per-vCPU event channel upcall vector

[David Woodhouse]

On Tue, 2023-10-24 at 13:29 +0100, Paul Durrant wrote:
> 
> > +    /* If the guest has set a per-vCPU callback vector, prefer that. */
> > +    if (gsi && kvm_xen_has_vcpu_callback_vector()) {
> > +    in_kernel = kvm_xen_has_cap(EVTCHN_SEND);
> > +    gsi = 0;
> > +    }
> > +
> 
> So this deals with setting the callback via after setting the upcall 
> vector. What happens if the guest then disables the upcall vector (by
> setting it to zero)? Xen checks 'v->arch.hvm.evtchn_upcall_vector != 0' 
> for every event delivery.

Qemu and KVM check before each delivery too. For a vCPU other than
vCPU0, if it isn't the per-vCPU lapic upcall vector, and it isn't the
system-wide vector, the interrupt isn't supposed to be delivered (the
GSI is tied to vCPU0).

However, we don't support dynamically disabling the kernel acceleration
(telling it to forget about the VIRQs, IPIs so that we can handle them
in userspace from now on). Not except for soft reset, when they're all
torn down anyway.

I don't really *want* to support that either. Better to make the kernel
mode unconditional, having it signal userspace via an eventfd when
vCPU0 has an upcall pending and it's GSI or INTx. But that's a *pair*
of eventfds, one for the resampler... and first I have to fix Qemu's
interrupt controller models to do the resampling properly on ack (qv).

Right now this works for all guests in practice and I have other yaks
to shave :)




smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 08/12] hw/xen: do not repeatedly try to create a failing backend device

[Paul Durrant]

On 16/10/2023 16:19, David Woodhouse wrote:

From: David Woodhouse 

If xen_backend_device_create() fails to instantiate a device, the XenBus
code will just keep trying over and over again each time the bus is
re-enumerated, as long as the backend appears online and in
XenbusStateInitialising.

The only thing which prevents the XenBus code from recreating duplicates
of devices which already exist, is the fact that xen_device_realize()
sets the backend state to XenbusStateInitWait. If the attempt to create
the device doesn't get *that* far, that's when it will keep getting
retried.

My first thought was to handle errors by setting the backend state to
XenbusStateClosed, but that doesn't work for XenConsole which wants to
*ignore* any device of type != "ioemu" completely.

So, make xen_backend_device_create() *keep* the XenBackendInstance for a
failed device, and provide a new xen_backend_exists() function to allow
xen_bus_type_enumerate() to check whether one already exists before
creating a new one.

Signed-off-by: David Woodhouse 
---
  hw/xen/xen-backend.c | 27 +--
  hw/xen/xen-bus.c |  3 ++-
  include/hw/xen/xen-backend.h |  1 +
  3 files changed, 24 insertions(+), 7 deletions(-)



Reviewed-by: Paul Durrant 

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

[Roger Pau Monné]

On Tue, Oct 24, 2023 at 02:08:42PM +0200, Jan Beulich wrote:
> On 24.10.2023 14:06, Jan Beulich wrote:
> > On 24.10.2023 13:36, Roger Pau Monné wrote:
> >> What is your reasoning for wanting the smp_processor_id() check in
> >> the caller rather than bogus_8259A_irq()?  It does seem fine to me to
> >> do such check in bogus_8259A_irq(), as whether the IRQ is bogus also
> >> depends on whether it fired on the BSP or any of the APs.
> > 
> > bogus_8259A_irq() shouldn't be concerned about the CPU it runs on; it
> > should solely deal with 8259A aspects.
> 
> Or to put it differently: The function is supposed to tell whether an
> IRQ is bogus from the pov of the PIC. The caller decides under what
> conditions to actually invoke this checking.

I understand that the PIC itself is agnostic as to which the CPU the
irq (vector) has been injected, but the added CPU vendor checks are
there to deal with possibly a bogus PIC implementation, and hence
doesn't feel that off place IMO.

Anyway, will adjust as requested, albeit I think it hampers
readability and that's more valuable than whether the check is
contextually better fit in do_IRQ() or bogus_8259A_irq().

Thanks, Roger.

Re: [PATCH 07/12] hw/xen: update Xen console to XenDevice model

[Paul Durrant]

On 16/10/2023 16:19, David Woodhouse wrote:

This allows (non-primary) console devices to be created on the command
line.

Signed-off-by: David Woodhouse 
---
  hw/char/trace-events|   8 +
  hw/char/xen_console.c   | 502 +++-
  hw/xen/xen-legacy-backend.c |   1 -
  3 files changed, 381 insertions(+), 130 deletions(-)

diff --git a/hw/char/trace-events b/hw/char/trace-events
index babf4d35ea..7a398c82a5 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -105,3 +105,11 @@ cadence_uart_baudrate(unsigned baudrate) "baudrate %u"
  # sh_serial.c
  sh_serial_read(char *id, unsigned size, uint64_t offs, uint64_t val) " %s size %d offs 
0x%02" PRIx64 " -> 0x%02" PRIx64
  sh_serial_write(char *id, unsigned size, uint64_t offs, uint64_t val) "%s size %d offs 
0x%02" PRIx64 " <- 0x%02" PRIx64
+
+# xen_console.c
+xen_console_connect(unsigned int idx, unsigned int ring_ref, unsigned int port, unsigned 
int limit) "idx %u ring_ref %u port %u limit %u"
+xen_console_disconnect(unsigned int idx) "idx %u"
+xen_console_unrealize(unsigned int idx) "idx %u"
+xen_console_realize(unsigned int idx, const char *chrdev) "idx %u chrdev %s"
+xen_console_device_create(unsigned int idx) "idx %u"
+xen_console_device_destroy(unsigned int idx) "idx %u"
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index 810dae3f44..bd20be116c 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -20,15 +20,19 @@
   */
  
  #include "qemu/osdep.h"

+#include "qemu/cutils.h"
  #include 
  #include 
  
  #include "qapi/error.h"

  #include "sysemu/sysemu.h"
  #include "chardev/char-fe.h"
-#include "hw/xen/xen-legacy-backend.h"
-
+#include "hw/xen/xen-backend.h"
+#include "hw/xen/xen-bus-helper.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
  #include "hw/xen/interface/io/console.h"
+#include "trace.h"
  
  struct buffer {

  uint8_t *data;
@@ -39,16 +43,22 @@ struct buffer {
  };
  
  struct XenConsole {

-struct XenLegacyDevice  xendev;  /* must be first */
+struct XenDevice  xendev;  /* must be first */
+XenEventChannel   *event_channel;
+int   dev;
  struct buffer buffer;
-char  console[XEN_BUFSIZE];
-int   ring_ref;
+char  *fe_path;
+unsigned int  ring_ref;
  void  *sring;
  CharBackend   chr;
  int   backlog;
  };
+typedef struct XenConsole XenConsole;
+
+#define TYPE_XEN_CONSOLE_DEVICE "xen-console"
+OBJECT_DECLARE_SIMPLE_TYPE(XenConsole, XEN_CONSOLE_DEVICE)
  
-static void buffer_append(struct XenConsole *con)

+static bool buffer_append(XenConsole *con)
  {
  struct buffer *buffer = >buffer;
  XENCONS_RING_IDX cons, prod, size;
@@ -60,7 +70,7 @@ static void buffer_append(struct XenConsole *con)
  
  size = prod - cons;

  if ((size == 0) || (size > sizeof(intf->out)))
-return;
+return false;
  
  if ((buffer->capacity - buffer->size) < size) {

  buffer->capacity += (size + 1024);
@@ -73,7 +83,7 @@ static void buffer_append(struct XenConsole *con)
  
  xen_mb();

  intf->out_cons = cons;
-xen_pv_send_notify(>xendev);
+xen_device_notify_event_channel(XEN_DEVICE(con), con->event_channel, NULL);
  
  if (buffer->max_capacity &&

  buffer->size > buffer->max_capacity) {
@@ -89,6 +99,7 @@ static void buffer_append(struct XenConsole *con)
  if (buffer->consumed > buffer->max_capacity - over)
  buffer->consumed = buffer->max_capacity - over;
  }
+return true;
  }
  
  static void buffer_advance(struct buffer *buffer, size_t len)

@@ -100,7 +111,7 @@ static void buffer_advance(struct buffer *buffer, size_t 
len)
  }
  }
  
-static int ring_free_bytes(struct XenConsole *con)

+static int ring_free_bytes(XenConsole *con)
  {
  struct xencons_interface *intf = con->sring;
  XENCONS_RING_IDX cons, prod, space;
@@ -118,13 +129,13 @@ static int ring_free_bytes(struct XenConsole *con)
  
  static int xencons_can_receive(void *opaque)

  {
-struct XenConsole *con = opaque;
+XenConsole *con = opaque;
  return ring_free_bytes(con);
  }
  
  static void xencons_receive(void *opaque, const uint8_t *buf, int len)

  {
-struct XenConsole *con = opaque;
+XenConsole *con = opaque;
  struct xencons_interface *intf = con->sring;
  XENCONS_RING_IDX prod;
  int i, max;
@@ -141,10 +152,10 @@ static void xencons_receive(void *opaque, const uint8_t 
*buf, int len)
  }
  xen_wmb();
  intf->in_prod = prod;
-xen_pv_send_notify(>xendev);
+xen_device_notify_event_channel(XEN_DEVICE(con), con->event_channel, NULL);
  }
  
-static void xencons_send(struct XenConsole *con)

+static bool xencons_send(XenConsole *con)
  {
  ssize_t len, size;
  
@@ -159,174 +170,407 @@ static void xencons_send(struct XenConsole *con)

  if (len < 1) {
  if (!con->backlog) {
  

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

[Julien Grall]

Hi,

On 23/10/2023 21:56, Stefano Stabellini wrote:

Signed-off-by: Stefano Stabellini 


Acked-by: Julien Grall 

Cheers,

--
Julien Grall

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

[Julien Grall]

Hi Jan,

On 23/10/2023 11:33, Jan Beulich wrote:

On 23.10.2023 12:17, Oleksii wrote:

On Thu, 2023-10-19 at 13:12 +0100, Julien Grall wrote:

Hi,

On 19/10/2023 12:41, Jan Beulich wrote:

On 19.10.2023 13:27, Julien Grall wrote:

that doesn't involve one arch to symlink headers from another
arch.


Whether to use symlinks or #include "../../arch/..." or yet
something else is
a matter of mechanics.


#include "../../arch/../" is pretty much in the same category. This
is
simply hiding the fact they could be in asm-generic.

Anyway, I have shared my view. Let see what the others thinks.

I have the same point: if something is shared at least between two
arch, it should go to ASM-generic.


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


TBH, I would expect that if RISC-V and Arm are using the same headers, 
then PPC would likely use it as well. So this would qualify to be in 
asm-generic/.


Now, I don't think we have to resolve the case where we have have two 
arch using one set of headers and the other another sets. We can cross 
that line once we have an example.


Cheers,

--
Julien Grall

Re: [PATCH 06/12] hw/xen: add get_frontend_path() method to XenDeviceClass

[Paul Durrant]

On 24/10/2023 13:56, David Woodhouse wrote:

On Tue, 2023-10-24 at 13:42 +0100, Paul Durrant wrote:



--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -711,8 +711,16 @@ static void xen_device_frontend_create(XenDevice *xendev, 
Error **errp)
    {
    ERRP_GUARD();
    XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
+    XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);

-    xendev->frontend_path = xen_device_get_frontend_path(xendev);

+    if (xendev_class->get_frontend_path) {
+    xendev->frontend_path = xendev_class->get_frontend_path(xendev, errp);
+    if (!xendev->frontend_path) {
+    return;


I think you need to update errp here to note that you are failing to
create the frontend.


If xendev_class->get_frontend_path returned NULL it will have filled in errp.



Ok, but a prepend to say that a lack of path there means we skip 
frontend creation seems reasonable?



As a general rule (I'll be doing a bombing run on xen-bus once I get my
patch queue down into single digits) we should never check 'if (*errp)'
to check if a function had an error. It should *also* return a success
or failure indication, and we should cope with errp being NULL.



I'm pretty sure someone told me the exact opposite a few years back.

  Paul

Re: [PATCH 01/12] i386/xen: fix per-vCPU upcall vector for Xen emulation

[David Woodhouse]

On Tue, 2023-10-24 at 13:16 +0100, Paul Durrant wrote:
> On 16/10/2023 16:18, David Woodhouse wrote:
> > From: David Woodhouse 
> > 
> > The per-vCPU upcall vector support had two problems. Firstly it was
> > using the wrong hypercall argument and would always return -EFAULT.
> > And secondly it was using the wrong ioctl() to pass the vector to
> > the kernel and thus the *kernel* would always return -EINVAL.
> > 
> > Linux doesn't (yet) use this mode so it went without decent testing
> > for a while.
> > 
> > Fixes: 105b47fdf2d0 ("i386/xen: implement
> > HVMOP_set_evtchn_upcall_vector")
> > Signed-off-by: David Woodhouse 
> > ---
> >   target/i386/kvm/xen-emu.c | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> Reviewed-by: Paul Durrant 

FWIW this patch gained a third "brown paper bag" fix this morning, when
I finally worked it out:

@@ -440,7 +440,8 @@ void kvm_xen_inject_vcpu_callback_vector(uint32_t
vcpu_id, int type)
  * deliver it as an MSI.
  */
 MSIMessage msg = {
-.address = APIC_DEFAULT_ADDRESS | X86_CPU(cs)->apic_id,
+.address = APIC_DEFAULT_ADDRESS |
+   (X86_CPU(cs)->apic_id << MSI_ADDR_DEST_ID_SHIFT),
 .data = vector | (1UL << MSI_DATA_LEVEL_SHIFT),
 };
 kvm_irqchip_send_msi(kvm_state, msg);



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 06/12] hw/xen: add get_frontend_path() method to XenDeviceClass

[David Woodhouse]

On Tue, 2023-10-24 at 13:42 +0100, Paul Durrant wrote:
> 
> > --- a/hw/xen/xen-bus.c
> > +++ b/hw/xen/xen-bus.c
> > @@ -711,8 +711,16 @@ static void xen_device_frontend_create(XenDevice 
> > *xendev, Error **errp)
> >    {
> >    ERRP_GUARD();
> >    XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
> > +    XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
> >    
> > -    xendev->frontend_path = xen_device_get_frontend_path(xendev);
> > +    if (xendev_class->get_frontend_path) {
> > +    xendev->frontend_path = xendev_class->get_frontend_path(xendev, 
> > errp);
> > +    if (!xendev->frontend_path) {
> > +    return;
> 
> I think you need to update errp here to note that you are failing to 
> create the frontend.

If xendev_class->get_frontend_path returned NULL it will have filled in errp.

As a general rule (I'll be doing a bombing run on xen-bus once I get my
patch queue down into single digits) we should never check 'if (*errp)'
to check if a function had an error. It should *also* return a success
or failure indication, and we should cope with errp being NULL.




smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 05/12] hw/xen: populate store frontend nodes with XenStore PFN/port

[David Woodhouse]

On Tue, 2023-10-24 at 13:35 +0100, Paul Durrant wrote:
> On 16/10/2023 16:19, David Woodhouse wrote:
> > From: David Woodhouse 
> > 
> > This is kind of redundant since without being able to get these
> > through
> > ome other method (HVMOP_get_param) the guest wouldn't be able to
> > access
> 
> ^ typo
> 
> > XenStore in order to find them. But Xen populates them, and it does
> > allow guests to *rebind* to the event channel port after a reset.
> > 
> 
> ... although this can also be done by querying the remote end of the 
> port before reset.
> 
I think you had to do that anyway; I don't think I was supposed to be
putting s->be_port in there anyway; it was supposed to be the
*frontend* port, and I've changed that in my tree.

I'll drop this whole sentence (and fix the typo).

> > Signed-off-by: David Woodhouse 
> > ---
> >   hw/i386/kvm/xen_xenstore.c | 10 ++
> >   1 file changed, 10 insertions(+)
> > 
> 
> Reviewed-by: Paul Durrant 
> 



smime.p7s
Description: S/MIME cryptographic signature

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

[Oleksii]

On Mon, 2023-10-23 at 12:47 +0200, Jan Beulich wrote:
> On 23.10.2023 12:43, Oleksii wrote:
> > On Thu, 2023-10-19 at 11:44 +0200, Jan Beulich wrote:
> > > On 14.09.2023 16:56, Oleksii Kurochko wrote:
> > > > --- /dev/null
> > > > +++ b/xen/include/asm-generic/iommu.h
> > > > @@ -0,0 +1,17 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > +#ifndef __ASM_GENERIC_IOMMU_H__
> > > > +#define __ASM_GENERIC_IOMMU_H__
> > > > +
> > > > +struct arch_iommu {
> > > > +};
> > > > +
> > > > +#endif /* __ASM_IOMMU_H__ */
> > > This one's perhaps slightly more "interesting": Yes, we have a
> > > HAS_PASSTHROUGH Kconfig option, which both Arm and x86 select.
> > > But it
> > > is in principle possible to support guests without any kind of
> > > IOMMU
> > > (permitting solely emulated and PV devices). In which case what's
> > > (imo) needed here in addition is
> > > 
> > > #ifdef CONFIG_HAS_PASSTHROUGH
> > > # error
> > > #endif
> > I am not 100% sure but not all platform has support of IOMMU.
> > 
> > And I thought that passthrough it is when a device is fully
> > committed
> > to  a guest domain with all MMIO things. So it is a question of
> > properly mapping MMIO to guest domain. Am I right?
> 
> Yes. And do you expect you will get away with such a stub
> implementation
> when you actually start supporting pass-through on RISC-V?
No, it will be changed. It was added to asm-generic because of the
build purpose for the full Xen build ( the same header with the same
content is used for PPC ).

But yeah, taking into account about device.h header in this patch
series looks like this patch will be arch-specific.

~ Oleksii

Re: [PATCH 06/12] hw/xen: add get_frontend_path() method to XenDeviceClass

[Paul Durrant]

On 16/10/2023 16:19, David Woodhouse wrote:

From: David Woodhouse 

The primary Xen console is special. The guest's side is set up for it by
the toolstack automatically and not by the standard PV init sequence.

Accordingly, its *frontend* doesn't appear in …/device/console/0 either;
instead it appears under …/console in the guest's XenStore node.

To allow the Xen console driver to override the frontend path for the
primary console, add a method to the XenDeviceClass which can be used
instead of the standard xen_device_get_frontend_path()

Signed-off-by: David Woodhouse 
---
  hw/xen/xen-bus.c | 10 +-
  include/hw/xen/xen-bus.h |  2 ++
  2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index ece8ec40cd..cc524ed92c 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -711,8 +711,16 @@ static void xen_device_frontend_create(XenDevice *xendev, 
Error **errp)
  {
  ERRP_GUARD();
  XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
+XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
  
-xendev->frontend_path = xen_device_get_frontend_path(xendev);

+if (xendev_class->get_frontend_path) {
+xendev->frontend_path = xendev_class->get_frontend_path(xendev, errp);
+if (!xendev->frontend_path) {
+return;


I think you need to update errp here to note that you are failing to 
create the frontend.


  Paul

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

[Oleksii]

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

I wrote everywhere because of "xen/drivers/passthrough/vtd/quirks.c"
but it is arch-dependent. So  , yes, only xen/pci.h should be updated.

~ Oleksii

Re: [PATCH 05/12] hw/xen: populate store frontend nodes with XenStore PFN/port

[Paul Durrant]

On 16/10/2023 16:19, David Woodhouse wrote:

From: David Woodhouse 

This is kind of redundant since without being able to get these through
ome other method (HVMOP_get_param) the guest wouldn't be able to access


^ typo


XenStore in order to find them. But Xen populates them, and it does
allow guests to *rebind* to the event channel port after a reset.



... although this can also be done by querying the remote end of the 
port before reset.



Signed-off-by: David Woodhouse 
---
  hw/i386/kvm/xen_xenstore.c | 10 ++
  1 file changed, 10 insertions(+)



Reviewed-by: Paul Durrant 

Re: [PATCH 04/12] i386/xen: advertise XEN_HVM_CPUID_UPCALL_VECTOR in CPUID

[Paul Durrant]

On 16/10/2023 16:19, David Woodhouse wrote:

From: David Woodhouse 

This will allow Linux guests (since v6.0) to use the per-vCPU upcall
vector delivered as MSI through the local APIC.

Signed-off-by: David Woodhouse 
---
  target/i386/kvm/kvm.c | 4 
  1 file changed, 4 insertions(+)



Reviewed-by: Paul Durrant 

Re: [PATCH 03/12] include: update Xen public headers to Xen 4.17.2 release

[Paul Durrant]

On 16/10/2023 16:19, David Woodhouse wrote:

From: David Woodhouse 

... in order to advertise the XEN_HVM_CPUID_UPCALL_VECTOR feature,
which will come in a subsequent commit.

Signed-off-by: David Woodhouse 
---
  hw/i386/kvm/xen_xenstore.c|  2 +-
  include/hw/xen/interface/arch-arm.h   | 37 +++---
  include/hw/xen/interface/arch-x86/cpuid.h | 31 +---
  .../hw/xen/interface/arch-x86/xen-x86_32.h| 19 +--
  .../hw/xen/interface/arch-x86/xen-x86_64.h| 19 +--
  include/hw/xen/interface/arch-x86/xen.h   | 26 ++
  include/hw/xen/interface/event_channel.h  | 19 +--
  include/hw/xen/interface/features.h   | 19 +--
  include/hw/xen/interface/grant_table.h| 19 +--
  include/hw/xen/interface/hvm/hvm_op.h | 19 +--
  include/hw/xen/interface/hvm/params.h | 19 +--
  include/hw/xen/interface/io/blkif.h   | 27 --
  include/hw/xen/interface/io/console.h | 19 +--
  include/hw/xen/interface/io/fbif.h| 19 +--
  include/hw/xen/interface/io/kbdif.h   | 19 +--
  include/hw/xen/interface/io/netif.h   | 25 +++---
  include/hw/xen/interface/io/protocols.h   | 19 +--
  include/hw/xen/interface/io/ring.h| 49 ++-
  include/hw/xen/interface/io/usbif.h   | 19 +--
  include/hw/xen/interface/io/xenbus.h  | 19 +--
  include/hw/xen/interface/io/xs_wire.h | 36 ++
  include/hw/xen/interface/memory.h | 30 +---
  include/hw/xen/interface/physdev.h| 23 ++---
  include/hw/xen/interface/sched.h  | 19 +--
  include/hw/xen/interface/trace.h  | 19 +--
  include/hw/xen/interface/vcpu.h   | 19 +--
  include/hw/xen/interface/version.h| 19 +--
  include/hw/xen/interface/xen-compat.h | 19 +--
  include/hw/xen/interface/xen.h| 19 +--
  29 files changed, 124 insertions(+), 523 deletions(-)



Acked-by: Paul Durrant 

Re: [PATCH 02/12] hw/xen: select kernel mode for per-vCPU event channel upcall vector

[Paul Durrant]

On 16/10/2023 16:18, David Woodhouse wrote:

From: David Woodhouse 

A guest which has configured the per-vCPU upcall vector may set the
HVM_PARAM_CALLBACK_IRQ param to fairly much anything other than zero.

For example, Linux v6.0+ after commit b1c3497e604 ("x86/xen: Add support
for HVMOP_set_evtchn_upcall_vector") will just do this after setting the
vector:

/* Trick toolstack to think we are enlightened. */
if (!cpu)
rc = xen_set_callback_via(1);

That's explicitly setting the delivery to GSI#, but it's supposed to be
overridden by the per-vCPU vector setting. This mostly works in QEMU
*except* for the logic to enable the in-kernel handling of event channels,
which falsely determines that the kernel cannot accelerate GSI delivery
in this case.

Add a kvm_xen_has_vcpu_callback_vector() to report whether vCPU#0 has
the vector set, and use that in xen_evtchn_set_callback_param() to
enable the kernel acceleration features even when the param *appears*
to be set to target a GSI.

Preserve the Xen behaviour that when HVM_PARAM_CALLBACK_IRQ is set to
*zero* the event channel delivery is disabled completely. (Which is
what that bizarre guest behaviour is working round in the first place.)

Fixes: 91cce756179 ("hw/xen: Add xen_evtchn device for event channel emulation")
Signed-off-by: David Woodhouse 
---
  hw/i386/kvm/xen_evtchn.c  | 6 ++
  include/sysemu/kvm_xen.h  | 1 +
  target/i386/kvm/xen-emu.c | 7 +++
  3 files changed, 14 insertions(+)

diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
index 4df973022c..d72dca6591 100644
--- a/hw/i386/kvm/xen_evtchn.c
+++ b/hw/i386/kvm/xen_evtchn.c
@@ -490,6 +490,12 @@ int xen_evtchn_set_callback_param(uint64_t param)
  break;
  }
  
+/* If the guest has set a per-vCPU callback vector, prefer that. */

+if (gsi && kvm_xen_has_vcpu_callback_vector()) {
+in_kernel = kvm_xen_has_cap(EVTCHN_SEND);
+gsi = 0;
+}
+


So this deals with setting the callback via after setting the upcall 
vector. What happens if the guest then disables the upcall vector (by 
setting it to zero)? Xen checks 'v->arch.hvm.evtchn_upcall_vector != 0' 
for every event delivery.


  Paul

Re: [PATCH 01/12] i386/xen: fix per-vCPU upcall vector for Xen emulation

[Paul Durrant]

On 16/10/2023 16:18, David Woodhouse wrote:

From: David Woodhouse 

The per-vCPU upcall vector support had two problems. Firstly it was
using the wrong hypercall argument and would always return -EFAULT.
And secondly it was using the wrong ioctl() to pass the vector to
the kernel and thus the *kernel* would always return -EINVAL.

Linux doesn't (yet) use this mode so it went without decent testing
for a while.

Fixes: 105b47fdf2d0 ("i386/xen: implement HVMOP_set_evtchn_upcall_vector")
Signed-off-by: David Woodhouse 
---
  target/i386/kvm/xen-emu.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)


Reviewed-by: Paul Durrant 

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

[Jan Beulich]

On 24.10.2023 14:06, Jan Beulich wrote:
> On 24.10.2023 13:36, Roger Pau Monné wrote:
>> What is your reasoning for wanting the smp_processor_id() check in
>> the caller rather than bogus_8259A_irq()?  It does seem fine to me to
>> do such check in bogus_8259A_irq(), as whether the IRQ is bogus also
>> depends on whether it fired on the BSP or any of the APs.
> 
> bogus_8259A_irq() shouldn't be concerned about the CPU it runs on; it
> should solely deal with 8259A aspects.

Or to put it differently: The function is supposed to tell whether an
IRQ is bogus from the pov of the PIC. The caller decides under what
conditions to actually invoke this checking.

Jan

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

[Juergen Gross]

On 24.10.23 12:41, Juergen Gross wrote:

On 24.10.23 09:43, David Woodhouse wrote:

On Tue, 2023-10-24 at 08:53 +0200, Juergen Gross wrote:


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

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


Retesting in the cold light of day, yes. Using v6.6-rc5 which is the
parent commit of the offending 87797fad6cce.

I now see this warning at boot time again, which I believe was an
aspect of what you were trying to fix:

[    0.059014] xen:events: Using FIFO-based ABI
[    0.059029] xen:events: Xen HVM callback vector for event delivery is enabled
[    0.059227] rcu: srcu_init: Setting srcu_struct sizes based on contention.
[    0.059296]
[    0.059297] =
[    0.059298] [ BUG: Invalid wait context ]
[    0.059299] 6.6.0-rc5 #1374 Not tainted
[    0.059300] -
[    0.059301] swapper/0/0 is trying to lock:
[    0.059303] 8ad595f8 (evtchn_rwlock){}-{3:3}, at: 
xen_evtchn_do_upcall+0x59/0xd0


Indeed.

What I still not get is why the rcu_dereference_check() splat isn't
happening without my patch.

IMHO it should be related to the fact that cpuhp_report_idle_dead()
is trying to send an IPI via xen_send_IPI_one(), which is using
notify_remote_via_irq(), which in turn needs to call irq_get_chip_data().
This is using the maple-tree since 721255b9826b, which is using
rcu_read_lock().

I can probably change xen_send_IPI_one() to not need irq_get_chip_data().


David, could you test the attached patch, please? Build tested only.


Juergen
From 9bcb7b86578db4c14f460ce954eb35b821e11ad8 Mon Sep 17 00:00:00 2001
From: Juergen Gross 
Date: Tue, 24 Oct 2023 13:51:36 +0200
Subject: [PATCH] xen/events: avoid using info_for_irq() in xen_send_IPI_one()

xen_send_IPI_one() is being used by cpuhp_report_idle_dead() after
it calls rcu_report_dead(), meaning that any RCU usage by
xen_send_IPI_one() is a bad idea.

Unfortunately xen_send_IPI_one() is using notify_remote_via_irq()
today, which is using irq_get_chip_data() via info_for_irq(). And
irq_get_chip_data() in turn is using a maple-tree lookup requiring
RCU.

Avoid this problem by caching the ipi event channels in another
percpu variable, allowing the use notify_remote_via_evtchn() in
xen_send_IPI_one().

Fixes: 721255b9826b ("genirq: Use a maple tree for interrupt descriptor management")
Reported-by: David Woodhouse 
Signed-off-by: Juergen Gross 
---
 drivers/xen/events/events_base.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 1b2136fe0fa5..2cf0c2b69386 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -164,6 +164,8 @@ static DEFINE_PER_CPU(int [NR_VIRQS], virq_to_irq) = {[0 ... NR_VIRQS-1] = -1};
 
 /* IRQ <-> IPI mapping */
 static DEFINE_PER_CPU(int [XEN_NR_IPIS], ipi_to_irq) = {[0 ... XEN_NR_IPIS-1] = -1};
+/* Cache for IPI event channels - needed for hot cpu unplug (avoid RCU usage). */
+static DEFINE_PER_CPU(evtchn_port_t [XEN_NR_IPIS], ipi_to_evtchn) = {[0 ... XEN_NR_IPIS-1] = 0};
 
 /* Event channel distribution data */
 static atomic_t channels_on_cpu[NR_CPUS];
@@ -366,6 +368,7 @@ static int xen_irq_info_ipi_setup(unsigned cpu,
 	info->u.ipi = ipi;
 
 	per_cpu(ipi_to_irq, cpu)[ipi] = irq;
+	per_cpu(ipi_to_evtchn, cpu)[ipi] = evtchn;
 
 	return xen_irq_info_common_setup(info, irq, IRQT_IPI, evtchn, 0);
 }
@@ -981,6 +984,7 @@ static void __unbind_from_irq(unsigned int irq)
 			break;
 		case IRQT_IPI:
 			per_cpu(ipi_to_irq, cpu)[ipi_from_irq(irq)] = -1;
+			per_cpu(ipi_to_evtchn, cpu)[ipi_from_irq(irq)] = 0;
 			break;
 		case IRQT_EVTCHN:
 			dev = info->u.interdomain;
@@ -1631,7 +1635,7 @@ EXPORT_SYMBOL_GPL(evtchn_put);
 
 void xen_send_IPI_one(unsigned int cpu, enum ipi_vector vector)
 {
-	int irq;
+	evtchn_port_t evtchn;
 
 #ifdef CONFIG_X86
 	if (unlikely(vector == XEN_NMI_VECTOR)) {
@@ -1642,9 +1646,9 @@ void xen_send_IPI_one(unsigned int cpu, enum ipi_vector vector)
 		return;
 	}
 #endif
-	irq = per_cpu(ipi_to_irq, cpu)[vector];
-	BUG_ON(irq < 0);
-	notify_remote_via_irq(irq);
+	evtchn = per_cpu(ipi_to_evtchn, cpu)[vector];
+	BUG_ON(evtchn == 0);
+	notify_remote_via_evtchn(evtchn);
 }
 
 struct evtchn_loop_ctrl {
-- 
2.35.3



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

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

[Jan Beulich]

On 24.10.2023 13:36, Roger Pau Monné wrote:
> On Tue, Oct 24, 2023 at 12:51:08PM +0200, Jan Beulich wrote:
>> On 24.10.2023 12:14, Roger Pau Monné wrote:
>>> On Tue, Oct 24, 2023 at 11:33:21AM +0200, Jan Beulich wrote:
 On 23.10.2023 14:46, Roger Pau Monne wrote:
> --- a/xen/arch/x86/i8259.c
> +++ b/xen/arch/x86/i8259.c
> @@ -37,6 +37,15 @@ static bool _mask_and_ack_8259A_irq(unsigned int irq);
>  
>  bool bogus_8259A_irq(unsigned int irq)
>  {
> +if ( smp_processor_id() &&
> + !(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | 
> X86_VENDOR_HYGON)) )
> +/*
> + * For AMD/Hygon do spurious PIC interrupt detection on all 
> CPUs, as it
> + * has been observed that during unknown circumstances spurious 
> PIC
> + * interrupts have been delivered to CPUs different than the BSP.
> + */
> +return false;
> +
>  return !_mask_and_ack_8259A_irq(irq);
>  }

 I don't think this function should be changed; imo the adjustment belongs
 at the call site.
>>>
>>> It makes the call site much more complex to follow, in fact I was
>>> considering pulling the PIC vector range checks into
>>> bogus_8259A_irq().  Would that convince you into placing the check here
>>> rather than in the caller context?
>>
>> Passing a vector and moving the range check into the function is something
>> that may make sense. But I'm afraid the same does not apply to the
>> smp_processor_id() check, unless the function was also renamed to
>> bogus_8259A_vector(). Which in turn doesn't make much sense, to me at
>> least, as the logic would better be in terms of IRQs (which is what the
>> chip deals with primarily), not vectors (which the chip deals with solely
>> during the INTA cycle with the CPU).
> 
> The alternative is to use:
> 
> if ( !(vector >= FIRST_LEGACY_VECTOR &&
>vector <= LAST_LEGACY_VECTOR &&
>(!smp_processor_id() ||
> /*
>  * For AMD/Hygon do spurious PIC interrupt
>  * detection on all CPUs, as it has been observed
>  * that during unknown circumstances spurious PIC
>  * interrupts have been delivered to CPUs
>  * different than the BSP.
>  */
>(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD |
> X86_VENDOR_HYGON))) &&
>bogus_8259A_irq(vector - FIRST_LEGACY_VECTOR)) )
> {
> 
> Which I find too complex to read, and prone to mistakes by future
> modifications.

>From my pov the main badness with this is pre-existing: The wrapping of
a complex expression in !(...). The replacement of the prior plain
smp_processor_id() isn't much of a problem imo.

> What is your reasoning for wanting the smp_processor_id() check in
> the caller rather than bogus_8259A_irq()?  It does seem fine to me to
> do such check in bogus_8259A_irq(), as whether the IRQ is bogus also
> depends on whether it fired on the BSP or any of the APs.

bogus_8259A_irq() shouldn't be concerned about the CPU it runs on; it
should solely deal with 8259A aspects.

Jan

[ovmf test] 183506: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-i386-xl-qemuu-ovmf-amd64  7 xen-install  fail pass in 183508-retest

version targeted for testing:
 ovmf fb044b7fe893a4545995bfe2701fd38e593355d9
baseline version:
 ovmf da73578bf7afee1fdd1abe97eaf733aa1e4bdefe

Last test of basis   183504  2023-10-24 03:10:51 Z0 days
Testing same since   183506  2023-10-24 08:12:27 Z0 days1 attempts


People who touched revisions under test:
  Nickle Wang 

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  fail



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
   da73578bf7..fb044b7fe8  fb044b7fe893a4545995bfe2701fd38e593355d9 -> 
xen-tested-master

Re: [PATCH 08/45] hw/arm/sbsa-ref: use pci_init_nic_devices()

[Leif Lindholm]

On Sun, Oct 22, 2023 at 16:51:23 +0100, David Woodhouse wrote:
> From: David Woodhouse 
> 
> Signed-off-by: David Woodhouse 

Reviewed-by: Leif Lindholm 

> ---
>  hw/arm/sbsa-ref.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 3c7dfcd6dc..582a28ce92 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -691,9 +691,7 @@ static void create_pcie(SBSAMachineState *sms)
>  
>  pci = PCI_HOST_BRIDGE(dev);
>  if (pci->bus) {
> -for (i = 0; i < nb_nics; i++) {
> -pci_nic_init_nofail(_table[i], pci->bus, mc->default_nic, 
> NULL);
> -}
> +pci_init_nic_devices(pci->bus, mc->default_nic);
>  }
>  
>  pci_create_simple(pci->bus, -1, "bochs-display");
> -- 
> 2.40.1
> 

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

[Roger Pau Monné]

On Tue, Oct 24, 2023 at 12:51:08PM +0200, Jan Beulich wrote:
> On 24.10.2023 12:14, Roger Pau Monné wrote:
> > On Tue, Oct 24, 2023 at 11:33:21AM +0200, Jan Beulich wrote:
> >> On 23.10.2023 14:46, Roger Pau Monne wrote:
> >>> --- a/xen/arch/x86/i8259.c
> >>> +++ b/xen/arch/x86/i8259.c
> >>> @@ -37,6 +37,15 @@ static bool _mask_and_ack_8259A_irq(unsigned int irq);
> >>>  
> >>>  bool bogus_8259A_irq(unsigned int irq)
> >>>  {
> >>> +if ( smp_processor_id() &&
> >>> + !(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | 
> >>> X86_VENDOR_HYGON)) )
> >>> +/*
> >>> + * For AMD/Hygon do spurious PIC interrupt detection on all 
> >>> CPUs, as it
> >>> + * has been observed that during unknown circumstances spurious 
> >>> PIC
> >>> + * interrupts have been delivered to CPUs different than the BSP.
> >>> + */
> >>> +return false;
> >>> +
> >>>  return !_mask_and_ack_8259A_irq(irq);
> >>>  }
> >>
> >> I don't think this function should be changed; imo the adjustment belongs
> >> at the call site.
> > 
> > It makes the call site much more complex to follow, in fact I was
> > considering pulling the PIC vector range checks into
> > bogus_8259A_irq().  Would that convince you into placing the check here
> > rather than in the caller context?
> 
> Passing a vector and moving the range check into the function is something
> that may make sense. But I'm afraid the same does not apply to the
> smp_processor_id() check, unless the function was also renamed to
> bogus_8259A_vector(). Which in turn doesn't make much sense, to me at
> least, as the logic would better be in terms of IRQs (which is what the
> chip deals with primarily), not vectors (which the chip deals with solely
> during the INTA cycle with the CPU).

The alternative is to use:

if ( !(vector >= FIRST_LEGACY_VECTOR &&
   vector <= LAST_LEGACY_VECTOR &&
   (!smp_processor_id() ||
/*
 * For AMD/Hygon do spurious PIC interrupt
 * detection on all CPUs, as it has been observed
 * that during unknown circumstances spurious PIC
 * interrupts have been delivered to CPUs
 * different than the BSP.
 */
   (boot_cpu_data.x86_vendor & (X86_VENDOR_AMD |
X86_VENDOR_HYGON))) &&
   bogus_8259A_irq(vector - FIRST_LEGACY_VECTOR)) )
{

Which I find too complex to read, and prone to mistakes by future
modifications.

What is your reasoning for wanting the smp_processor_id() check in
the caller rather than bogus_8259A_irq()?  It does seem fine to me to
do such check in bogus_8259A_irq(), as whether the IRQ is bogus also
depends on whether it fired on the BSP or any of the APs.

Thanks, Roger.

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 linux84186fcb834ecc55604efaf383e17e6b5e9baa50
baseline version:
 linuxe017769f4ce20dc0d3fa3220d4d359dcc4431274

Last test of basis   183499  2023-10-23 18:11:24 Z0 days
Testing same since   183503  2023-10-24 02:26:34 Z0 days1 attempts


People who touched revisions under test:
  Ammar Faizi 
  Linus Torvalds 
  Thomas Weißschuh 
  Willy Tarreau 

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  

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

[Jan Beulich]

On 24.10.2023 12:14, Roger Pau Monné wrote:
> On Tue, Oct 24, 2023 at 11:33:21AM +0200, Jan Beulich wrote:
>> On 23.10.2023 14:46, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/i8259.c
>>> +++ b/xen/arch/x86/i8259.c
>>> @@ -37,6 +37,15 @@ static bool _mask_and_ack_8259A_irq(unsigned int irq);
>>>  
>>>  bool bogus_8259A_irq(unsigned int irq)
>>>  {
>>> +if ( smp_processor_id() &&
>>> + !(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) 
>>> )
>>> +/*
>>> + * For AMD/Hygon do spurious PIC interrupt detection on all CPUs, 
>>> as it
>>> + * has been observed that during unknown circumstances spurious PIC
>>> + * interrupts have been delivered to CPUs different than the BSP.
>>> + */
>>> +return false;
>>> +
>>>  return !_mask_and_ack_8259A_irq(irq);
>>>  }
>>
>> I don't think this function should be changed; imo the adjustment belongs
>> at the call site.
> 
> It makes the call site much more complex to follow, in fact I was
> considering pulling the PIC vector range checks into
> bogus_8259A_irq().  Would that convince you into placing the check here
> rather than in the caller context?

Passing a vector and moving the range check into the function is something
that may make sense. But I'm afraid the same does not apply to the
smp_processor_id() check, unless the function was also renamed to
bogus_8259A_vector(). Which in turn doesn't make much sense, to me at
least, as the logic would better be in terms of IRQs (which is what the
chip deals with primarily), not vectors (which the chip deals with solely
during the INTA cycle with the CPU).

Jan

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

[Juergen Gross]

On 24.10.23 09:43, David Woodhouse wrote:

On Tue, 2023-10-24 at 08:53 +0200, Juergen Gross wrote:


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

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


Retesting in the cold light of day, yes. Using v6.6-rc5 which is the
parent commit of the offending 87797fad6cce.

I now see this warning at boot time again, which I believe was an
aspect of what you were trying to fix:

[0.059014] xen:events: Using FIFO-based ABI
[0.059029] xen:events: Xen HVM callback vector for event delivery is enabled
[0.059227] rcu: srcu_init: Setting srcu_struct sizes based on contention.
[0.059296]
[0.059297] =
[0.059298] [ BUG: Invalid wait context ]
[0.059299] 6.6.0-rc5 #1374 Not tainted
[0.059300] -
[0.059301] swapper/0/0 is trying to lock:
[0.059303] 8ad595f8 (evtchn_rwlock){}-{3:3}, at: 
xen_evtchn_do_upcall+0x59/0xd0


Indeed.

What I still not get is why the rcu_dereference_check() splat isn't
happening without my patch.

IMHO it should be related to the fact that cpuhp_report_idle_dead()
is trying to send an IPI via xen_send_IPI_one(), which is using
notify_remote_via_irq(), which in turn needs to call irq_get_chip_data().
This is using the maple-tree since 721255b9826b, which is using
rcu_read_lock().

I can probably change xen_send_IPI_one() to not need irq_get_chip_data().
But I'd like to understand why my patch causes the problem to surface
only now, instead of having been prominent since commit 721255b9826b.

Paul, do you have an explanation for the splat only coming out now?


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

[PATCH for-4.19 v2] docs/arm: Document where Xen should be loaded in memory

[Julien Grall]

From: Julien Grall 

In commit 9d267c049d92 ("xen/arm64: Rework the memory layout"),
we decided to require Xen to be loaded below 2 TiB to simplify
the logic to enable the MMU. The limit was decided based on
how known platform boot plus some slack.

We had a recent report that this is not sufficient on the AVA
platform with a old firmware [1]. But the restriction is not
going to change in Xen 4.18. So document the limit clearly
in docs/misc/arm/booting.txt.

[1] https://lore.kernel.org/20231013122658.1270506-3-leo@linaro.org

Signed-off-by: Julien Grall 
Reviewed-by: Michal Orzel 

---

Changes in v2:
- The limit is 2 TiB no 5
- Remove unnecessary sentence in the docs
- Add missing link
- Add Michal's reviewed-by

I couldn't find a nice way to document it in SUPPORT.md. So I decided
to only document the restrict in docs/misc/arm/booting.txt for now.

I also couldn't find any way from GRUB/UEFI (I didn't look much) to
specify the loading address.
---
 docs/misc/arm/booting.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
index 02f7bb65ec6d..547f58a7d981 100644
--- a/docs/misc/arm/booting.txt
+++ b/docs/misc/arm/booting.txt
@@ -21,7 +21,9 @@ The exceptions to this on 32-bit ARM are as follows:
  zImage protocol should still be used and not the stricter "raw
  (non-zImage)" protocol described in arm/Booting.
 
-There are no exception on 64-bit ARM.
+The exceptions to this on 64-bit ARM are as follows:
+
+ Xen binary should be loaded in memory below 2 TiB.
 
 Booting Guests
 --
-- 
2.40.1