Re: [PATCH RFC 07/10] domain: map/unmap GADDR based shared guest areas

[Julien Grall]

Hi Jan,

On 28/11/2022 10:01, Jan Beulich wrote:

On 24.11.2022 22:29, Julien Grall wrote:

On 19/10/2022 09:43, Jan Beulich wrote:

--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1563,7 +1563,82 @@ int map_guest_area(struct vcpu *v, paddr
  struct guest_area *area,
  void (*populate)(void *dst, struct vcpu *v))
   {
-return -EOPNOTSUPP;
+struct domain *currd = v->domain;
+void *map = NULL;
+struct page_info *pg = NULL;
+int rc = 0;
+
+if ( gaddr )


0 is technically a valid (guest) physical address on Arm.


I guess it is everywhere; it certainly also is on x86. While perhaps a
little unfortunate in ordering, the public header changes coming only
in the following patches was the best way I could think of to split
this work into reasonable size pieces. There the special meaning of 0
is clearly documented. And I don't really see it as a meaningful
limitation to not allow guests to register such areas at address zero.
I would expect an OS to allocate the region using the generic physical 
allocator. This allocator may decide that '0' is a valid address and 
return it.


So I think your approach could potentially complicate the OS 
implementation. I think it would be better to use an all Fs value as 
this cannot be valid for this hypercall (we require at least 4-byte 
alignment).





+{
+unsigned long gfn = PFN_DOWN(gaddr);


This could be gfn_t for adding some type safety.


Indeed I did consider doing so, but the resulting code would imo be
less legible. But this difference perhaps isn't significant enough
for me to object to changing, in case you (or others) think the
type safety is really a meaningful gain here.


In general, my preference is to always use the typesafe version because 
it reduces the number of "unsigned long".


[...]


The first function will set v->is_running to false (see
vcpu_context_saved()). So I think the "area" could be touched even afte
vcpu_pause() is returned.

Therefore, I think we will need _update_runstate_area() (or
update_runstate_area()) to be called first.


... I don't see a need for adjustment. The corresponding

 _update_runstate_area(prev);

sits quite a bit earlier in context_switch(). (Arm code is quite a bit
different, but this particular aspect looks to apply there as well.)


You are right. Sorry I misread the code.




@@ -1573,6 +1648,22 @@ int map_guest_area(struct vcpu *v, paddr
*/
   void unmap_guest_area(struct vcpu *v, struct guest_area *area)
   {
+struct domain *d = v->domain;
+void *map;
+struct page_info *pg;


AFAIU, the assumption is the vCPU should be paused here.


Yes, as the comment ahead of the function (introduced by an earlier
patch) says.


Ah, I missed that. Thanks!




Should we add an ASSERT()?


I was going from unmap_vcpu_info(), which had the same requirement,
while also only recording it by way of a comment. I certainly could
add an ASSERT(), but besides this being questionable as to the rules
set forth in ./CODING_STYLE I also view assertions of "paused" state
as being of limited use - the entity in question may become unpaused
on the clock cycle after the check was done. 


That's correct. However, that race you mention is unlikely to happen 
*every* time. So there are a very high chance the ASSERT() will hit if 
miscalled.



(But yes, such are no
different from e.g. the fair number of spin_is_locked() checks we
have scattered around, which don't really provide guarantees either.)
And that's fine to not provide the full guarantee. You are still 
probably going to catch 95% (if not more) of the callers that forgot to 
call it with the spin lock held.


At least for me, those ASSERTs() were super helpful during development 
in more than a few cases.


Cheers,

--
Julien Grall

Re: [PATCH v3 1/2] xen/arm: vpl011: emulate non-SBSA registers as WI/RAZ

[Julien Grall]

Hi,

On 29/11/2022 03:39, Jiamei Xie wrote:

When the guest kernel enables DMA engine with "CONFIG_DMA_ENGINE=y",
Linux SBSA PL011 driver will access PL011 DMACR register in some
functions. As chapter "B Generic UART" in "ARM Server Base System
Architecture"[1] documentation describes, SBSA UART doesn't support
DMA. In current code, when the kernel tries to access DMACR register,
Xen will inject a data abort:
Unhandled fault at 0xffc00944d048
Mem abort info:
   ESR = 0x9600
   EC = 0x25: DABT (current EL), IL = 32 bits
   SET = 0, FnV = 0
   EA = 0, S1PTW = 0
   FSC = 0x00: ttbr address size fault
Data abort info:
   ISV = 0, ISS = 0x
   CM = 0, WnR = 0
swapper pgtable: 4k pages, 39-bit VAs, pgdp=20e2e000
[ffc00944d048] pgd=10003803, p4d=10003803, 
pud=10003803, pmd=10003fffa803, pte=00689c090f13
Internal error: ttbr address size fault: 9600 [#1] PREEMPT SMP
...
Call trace:
  pl011_stop_rx+0x70/0x80
  tty_port_shutdown+0x7c/0xb4
  tty_port_close+0x60/0xcc
  uart_close+0x34/0x8c
  tty_release+0x144/0x4c0
  __fput+0x78/0x220
  fput+0x1c/0x30
  task_work_run+0x88/0xc0
  do_notify_resume+0x8d0/0x123c
  el0_svc+0xa8/0xc0
  el0t_64_sync_handler+0xa4/0x130
  el0t_64_sync+0x1a0/0x1a4
Code: b983 b901f001 794038a0 8b42 (b941)
---[ end trace 83dd93df15c3216f ]---
note: bootlogd[132] exited with preempt_count 1
/etc/rcS.d/S07bootlogd: line 47: 132 Segmentation fault start-stop-daemon

As discussed in [2], this commit makes the access to non-SBSA registers
RAZ/WI as an improvement.

[1] https://developer.arm.com/documentation/den0094/c/?lang=en
[2] 
https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2211161552420.4020@ubuntu-linux-20-04-desktop/

Signed-off-by: Jiamei Xie 
---
v2 -> v3
- emulate non-SBSA registers as WI/RAZ in default case
- update commit message
v1 -> v2
- print a message using XENLOG_G_DEBUG when it's write-ignore
---
  xen/arch/arm/vpl011.c | 13 +++--
  1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 43522d48fd..1bf803fc1f 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -414,11 +414,19 @@ static int vpl011_mmio_read(struct vcpu *v,
  default:
  gprintk(XENLOG_ERR, "vpl011: unhandled read r%d offset %#08x\n",
  dabt.reg, vpl011_reg);
-return 0;
+goto read_as_zero;
  }
  
  return 1;
  
+read_as_zero:

+if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;


We don't know the registers and therefore I don't think we should check 
the size.



+
+VPL011_LOCK(d, flags);
+*r = 0;
+VPL011_UNLOCK(d, flags);

There is no need to lock/unlock here.


+return 1;
+
  bad_width:
  gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n",
  dabt.size, dabt.reg, vpl011_reg);
@@ -486,10 +494,11 @@ static int vpl011_mmio_write(struct vcpu *v,
  default:
  gprintk(XENLOG_ERR, "vpl011: unhandled write r%d offset %#08x\n",
  dabt.reg, vpl011_reg);
-return 0;
+goto write_ignore;
  }
  
  write_ignore:

+if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;


Same as for the read_as_zero, the size is unknown and shouldn't be checked.

Cheers,

--
Julien Grall

Re: [PATCH v3 2/2] xen/arm: vpl011: drop redundancy in mmio_write/read

[Julien Grall]

Hi,

On 29/11/2022 03:39, Jiamei Xie wrote:

This commit is to drop redundancy in the function vpl011_mmio_write
and vpl011_mmio_read:
- In vpl011_mmio_read switch block, all cases have a return. So the
   outside one can be removed.
- Each switch case checks access by the same if statments. So we can
   just use one and put it before the switch block.
- The goto label bad_width and read_as_zero is used only once, put the
   code directly

Signed-off-by: Jiamei Xie 
---
  xen/arch/arm/vpl011.c | 66 +--
  1 file changed, 19 insertions(+), 47 deletions(-)

diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 1bf803fc1f..80b859baed 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -354,11 +354,15 @@ static int vpl011_mmio_read(struct vcpu *v,
  struct domain *d = v->domain;
  unsigned long flags;
  
+if ( !vpl011_reg32_check_access(dabt) ) {


As I pointed out in the previous version, we don't know the size of the 
registers for the one not described in the SBSA UART. So I don't think 
this check should be consolidated.


Also, coding style:

if (  )
{


+gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n",
+dabt.size, dabt.reg, vpl011_reg);
+return 0;
+}
+
  switch ( vpl011_reg )
  {
  case DR:
-if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
  if ( vpl011->backend_in_domain )
  *r = vreg_reg32_extract(vpl011_read_data(d), info);
  else
@@ -366,31 +370,23 @@ static int vpl011_mmio_read(struct vcpu *v,
  return 1;
  
  case RSR:

-if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
  /* It always returns 0 as there are no physical errors. */
  *r = 0;
  return 1;
  
  case FR:

-if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
  VPL011_LOCK(d, flags);
  *r = vreg_reg32_extract(vpl011->uartfr, info);
  VPL011_UNLOCK(d, flags);
  return 1;
  
  case RIS:

-if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
  VPL011_LOCK(d, flags);
  *r = vreg_reg32_extract(vpl011->uartris, info);
  VPL011_UNLOCK(d, flags);
  return 1;
  
  case MIS:

-if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
  VPL011_LOCK(d, flags);
  *r = vreg_reg32_extract(vpl011->uartris & vpl011->uartimsc,
  info);
@@ -398,40 +394,25 @@ static int vpl011_mmio_read(struct vcpu *v,
  return 1;
  
  case IMSC:

-if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
  VPL011_LOCK(d, flags);
  *r = vreg_reg32_extract(vpl011->uartimsc, info);
  VPL011_UNLOCK(d, flags);
  return 1;
  
  case ICR:

-if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
  /* Only write is valid. */
  return 0;
  
  default:

  gprintk(XENLOG_ERR, "vpl011: unhandled read r%d offset %#08x\n",
  dabt.reg, vpl011_reg);
-goto read_as_zero;
-}
-
-return 1;
-
-read_as_zero:


In general, we don't want to introduce and remove the same code within a 
series. If you don't want to keep read_as_zero, then you should not 
introduce it.


However... I think using the read_as_zero label could still be 
beneficial to reduce the numbers of lines where the registers are RAZ 
(e.g. default, RSR...).



-if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
-VPL011_LOCK(d, flags);
-*r = 0;
-VPL011_UNLOCK(d, flags);
-return 1;
-
-bad_width:
-gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n",
-dabt.size, dabt.reg, vpl011_reg);
-return 0;
  
+/* Read as zero */

+VPL011_LOCK(d, flags);
+*r = 0;
+VPL011_UNLOCK(d, flags);
+return 1;
+}
  }
  
  static int vpl011_mmio_write(struct vcpu *v,

@@ -446,14 +427,18 @@ static int vpl011_mmio_write(struct vcpu *v,
  struct domain *d = v->domain;
  unsigned long flags;
  
-switch ( vpl011_reg )

+   if ( !vpl011_reg32_check_access(dabt) ) {
+   gprintk(XENLOG_ERR, "vpl011: bad write width %d r%d offset %#08x\n",
+   dabt.size, dabt.reg, vpl011_reg);
+   return 0;
+   }


Same remarks as for the read part.


+
+   switch ( vpl011_reg )
  {
  case DR:
  {
  uint32_t data = 0;
  
-if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;

-
  vreg_reg32_update(&data, r, info);
  data &= 0xFF;
  if ( vpl011->backend_in_domain )
@@ -464,8 +449,6 @@ static int vpl011_mmio_write(struct vcpu *v,
  }
  
  case RSR: /* Nothing to clear. */

-if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
-
  return 1;
  
  case FR:

@@ -474,8 +457,6 @@ static int vpl011_mmio_write(struct vcpu *v,
  goto write_ignore;
  

Re: [PATCH 2/4] xen/scripts: add cppcheck tool to the xen-analysis.py script

[Jan Beulich]

On 28.11.2022 16:37, Luca Fancellu wrote:
>> On 28 Nov 2022, at 15:19, Jan Beulich  wrote:
>> On 28.11.2022 15:10, Luca Fancellu wrote:
>>> Change cppcheck invocation method by using the xen-analysis.py
>>> script using the arguments --run-cppcheck.
>>>
>>> Now cppcheck analysis will build Xen while the analysis is performed
>>> on the source files, it will produce a text report and an additional
>>> html output when the script is called with --cppcheck-html.
>>>
>>> With this patch cppcheck will benefit of platform configuration files
>>> that will help it to understand the target of the compilation and
>>> improve the analysis.
>>>
>>> To do so:
>>> - remove cppcheck rules from Makefile and move them to the script.
>>> - Update xen-analysis.py with the code to integrate cppcheck.
>>> - merge the script merge_cppcheck_reports.py into the xen-analysis
>>>   script package and rework the code to integrate it.
>>> - add platform configuration files for cppcheck..
>>> - add cppcheck-cc.sh script that is a wrapper for cppcheck and it's
>>>   used as Xen compiler, it will intercept all flags given from the
>>>   make build system and it will execute cppcheck on the compiled
>>>   file together with the file compilation.
>>> - guarded hypercall-defs.c with CPPCHECK define because cppcheck
>>>   gets confused as the file does not contain c code.
>>> - add false-positive-cppcheck.json file
>>> - update documentation.
>>> - update .gitignore
>>>
>>> Signed-off-by: Luca Fancellu 
>>
>> Just two and a half questions, not a full review:
>>
>>> ---
>>> .gitignore|   8 +-
>>> docs/misra/cppcheck.txt   |  27 +-
>>> docs/misra/documenting-violations.rst |   7 +-
>>> docs/misra/false-positive-cppcheck.json   |  12 +
>>> docs/misra/xen-static-analysis.rst|  42 ++-
>>> xen/Makefile  | 116 +---
>>> xen/include/hypercall-defs.c  |   9 +
>>> xen/scripts/xen-analysis.py   |  18 +-
>>> xen/scripts/xen_analysis/cppcheck_analysis.py | 272 ++
>>> .../xen_analysis/cppcheck_report_utils.py | 130 +
>>> xen/scripts/xen_analysis/generic_analysis.py  |  21 +-
>>> xen/scripts/xen_analysis/settings.py  |  77 -
>>> xen/scripts/xen_analysis/utils.py |  21 +-
>>> xen/tools/cppcheck-cc.sh  | 223 ++
>>> xen/tools/cppcheck-plat/arm32-wchar_t4.xml|  17 ++
>>> xen/tools/cppcheck-plat/arm64-wchar_t2.xml|  17 ++
>>> xen/tools/cppcheck-plat/arm64-wchar_t4.xml|  17 ++
>>> xen/tools/cppcheck-plat/x86_64-wchar_t2.xml   |  17 ++
>>> xen/tools/cppcheck-plat/x86_64-wchar_t4.xml   |  17 ++
>>
>> What are these last five files about? There's nothing about them in
>> the description afaics.
> 
> They are cppcheck platform configuration files, they help cppcheck to 
> understand
> the size of the types depending on the target of the compilation.
> 
> This section in the commit message is to introduce them:
> 
> With this patch cppcheck will benefit of platform configuration files
> that will help it to understand the target of the compilation and
> improve the analysis.
> 
> Do you think I should say it differently? Or maybe say that they reside in 
> xen/tools/cppcheck-plat/ ?

Perhaps (I didn't read that paragraph as relating to _anything_ in
tree), e.g.:

With this patch cppcheck will benefit from platform configuration files
that will help it to understand the target of the compilation and
improve the analysis. These are XML files placed in
xen/tools/cppcheck-plat/, describing ... (I don't know what to put here).

Please write the description here such that people not familiar with
cppcheck (or more generally with any external tool) can still follow
what you're talking about and what the patch is doing.

>>> --- /dev/null
>>> +++ b/xen/scripts/xen_analysis/cppcheck_analysis.py
>>> @@ -0,0 +1,272 @@
>>> +#!/usr/bin/env python3
>>> +
>>> +import os, re, shutil
>>> +from . import settings, utils, cppcheck_report_utils
>>> +
>>> +class GetMakeVarsPhaseError(Exception):
>>> +pass
>>> +
>>> +class CppcheckDepsPhaseError(Exception):
>>> +pass
>>> +
>>> +class CppcheckReportPhaseError(Exception):
>>> +pass
>>> +
>>> +CPPCHECK_BUILD_DIR = "build-dir-cppcheck"
>>> +CPPCHECK_HTMLREPORT_OUTDIR = "cppcheck-htmlreport"
>>> +CPPCHECK_REPORT_OUTDIR = "cppcheck-report"
>>> +cppcheck_extra_make_args = ""
>>> +xen_cc = ""
>>> +
>>> +def get_make_vars():
>>> +global xen_cc
>>> +invoke_make = utils.invoke_command(
>>> +"make -C {} {} export-variable-CC"
>>> +.format(settings.xen_dir, settings.make_forward_args),
>>> +True, GetMakeVarsPhaseError,
>>> +"Error occured retrieving make vars:\n{}"
>>> +)
>>> +
>>> +cc_var_regex = re.search('^CC=(.*)$', invoke_make, flags=re.M)
>>> +if cc_var_regex:
>>> +xen_cc = cc_var_regex.group(1)
>>> +
>>> +i

Re: [PATCH 0/4] Static analyser finding deviation

[Luca Fancellu]

> On 29 Nov 2022, at 01:55, Stefano Stabellini  wrote:
> 
> On Mon, 28 Nov 2022, Luca Fancellu wrote:
>> This serie introduces a way to suppress a static analyser finding providing a
>> proper justification for it.
>> The process is explained in the docs/misra/documenting-violations.rst 
>> document
>> that this serie will provide.
>> The tools currently supported are eclair, coverity and cppcheck, but the 
>> design
>> is open to support many other static analysis tool.
>> 
>> The changes are split between the first two patches to reduce the review 
>> effort,
>> the first patch is introducing the deviation process for the eclair and 
>> coverity
>> tools, this is because their analysis system is similar.
>> 
>> The second patch is introducing the same deviation process for cppcheck,
>> modifying the current way it is called from the makefile and improving its
>> analysis.
>> 
>> The third patch is a fix for a tool used for cppcheck and the fourth patch
>> is an example of how a deviation can be applied for some MISRA findings.

Hi Stefano,

> 
> I tried testing this series with:
> 
> # scripts/xen-analysis.py --build-only --cppcheck-html --run-cppcheck 
> --cppcheck-bin=/local/repos/cppcheck/cppcheck 
> --cppcheck-html-bin=/local/repos/cppcheck/htmlreport/cppcheck-htmlreport
> 
> But I get this error:
> 
> ERROR: Can't find cppcheck version or version is not 2.7
> 
> 
> Note that my cppcheck is 2.7.4:
> 
> # ./cppcheck --version
> Cppcheck 2.7.4

Yes this is a bug, I’m strictly checking for 2.7, I will modify it to 2.7.x if 
you agree

> 
> 
> After removing the version check in cppcheck_analysis.py, the process
> starts correctly.
> 
> Also, where is the output html report created by cppcheck-html by
> default?


The html output should be in the xen folder 
[xen_repo]/xen/cppcheck-htmlreport/html but when you specify --build-only the 
reports are not generated, only the build phase is executed.

Have you tried without --build-only to test the report generations?

Cheers,
Luca

Re: [PATCH] Introduce more MISRA C rules to docs/misra/rules.rst

[Jan Beulich]

On 29.11.2022 01:18, Stefano Stabellini wrote:
> From: Stefano Stabellini 
> 
> Add the new MISRA C rules agreed by the MISRA C working group to
> docs/misra/rules.rst.
> 
> Add a comment for Rule 19.1 to explain that Eclair's findings are
> "caution" reports, not violations.
> 
> Signed-off-by: Stefano Stabellini 

Acked-by: Jan Beulich 

Re: [PATCH 2/4] xen/scripts: add cppcheck tool to the xen-analysis.py script

[Luca Fancellu]

> On 29 Nov 2022, at 09:42, Jan Beulich  wrote:
> 
> On 28.11.2022 16:37, Luca Fancellu wrote:
>>> On 28 Nov 2022, at 15:19, Jan Beulich  wrote:
>>> On 28.11.2022 15:10, Luca Fancellu wrote:
 Change cppcheck invocation method by using the xen-analysis.py
 script using the arguments --run-cppcheck.
 
 Now cppcheck analysis will build Xen while the analysis is performed
 on the source files, it will produce a text report and an additional
 html output when the script is called with --cppcheck-html.
 
 With this patch cppcheck will benefit of platform configuration files
 that will help it to understand the target of the compilation and
 improve the analysis.
 
 To do so:
 - remove cppcheck rules from Makefile and move them to the script.
 - Update xen-analysis.py with the code to integrate cppcheck.
 - merge the script merge_cppcheck_reports.py into the xen-analysis
  script package and rework the code to integrate it.
 - add platform configuration files for cppcheck..
 - add cppcheck-cc.sh script that is a wrapper for cppcheck and it's
  used as Xen compiler, it will intercept all flags given from the
  make build system and it will execute cppcheck on the compiled
  file together with the file compilation.
 - guarded hypercall-defs.c with CPPCHECK define because cppcheck
  gets confused as the file does not contain c code.
 - add false-positive-cppcheck.json file
 - update documentation.
 - update .gitignore
 
 Signed-off-by: Luca Fancellu 
>>> 
>>> Just two and a half questions, not a full review:
>>> 
 ---
 .gitignore|   8 +-
 docs/misra/cppcheck.txt   |  27 +-
 docs/misra/documenting-violations.rst |   7 +-
 docs/misra/false-positive-cppcheck.json   |  12 +
 docs/misra/xen-static-analysis.rst|  42 ++-
 xen/Makefile  | 116 +---
 xen/include/hypercall-defs.c  |   9 +
 xen/scripts/xen-analysis.py   |  18 +-
 xen/scripts/xen_analysis/cppcheck_analysis.py | 272 ++
 .../xen_analysis/cppcheck_report_utils.py | 130 +
 xen/scripts/xen_analysis/generic_analysis.py  |  21 +-
 xen/scripts/xen_analysis/settings.py  |  77 -
 xen/scripts/xen_analysis/utils.py |  21 +-
 xen/tools/cppcheck-cc.sh  | 223 ++
 xen/tools/cppcheck-plat/arm32-wchar_t4.xml|  17 ++
 xen/tools/cppcheck-plat/arm64-wchar_t2.xml|  17 ++
 xen/tools/cppcheck-plat/arm64-wchar_t4.xml|  17 ++
 xen/tools/cppcheck-plat/x86_64-wchar_t2.xml   |  17 ++
 xen/tools/cppcheck-plat/x86_64-wchar_t4.xml   |  17 ++
>>> 
>>> What are these last five files about? There's nothing about them in
>>> the description afaics.
>> 
>> They are cppcheck platform configuration files, they help cppcheck to 
>> understand
>> the size of the types depending on the target of the compilation.
>> 
>> This section in the commit message is to introduce them:
>> 
>> With this patch cppcheck will benefit of platform configuration files
>> that will help it to understand the target of the compilation and
>> improve the analysis.
>> 
>> Do you think I should say it differently? Or maybe say that they reside in 
>> xen/tools/cppcheck-plat/ ?
> 
> Perhaps (I didn't read that paragraph as relating to _anything_ in
> tree), e.g.:
> 
> With this patch cppcheck will benefit from platform configuration files
> that will help it to understand the target of the compilation and
> improve the analysis. These are XML files placed in
> xen/tools/cppcheck-plat/, describing ... (I don't know what to put here).
> 
> Please write the description here such that people not familiar with
> cppcheck (or more generally with any external tool) can still follow
> what you're talking about and what the patch is doing.

Ok I can modify the description to add more details

> 
 --- /dev/null
 +++ b/xen/scripts/xen_analysis/cppcheck_analysis.py
 @@ -0,0 +1,272 @@
 +#!/usr/bin/env python3
 +
 +import os, re, shutil
 +from . import settings, utils, cppcheck_report_utils
 +
 +class GetMakeVarsPhaseError(Exception):
 +pass
 +
 +class CppcheckDepsPhaseError(Exception):
 +pass
 +
 +class CppcheckReportPhaseError(Exception):
 +pass
 +
 +CPPCHECK_BUILD_DIR = "build-dir-cppcheck"
 +CPPCHECK_HTMLREPORT_OUTDIR = "cppcheck-htmlreport"
 +CPPCHECK_REPORT_OUTDIR = "cppcheck-report"
 +cppcheck_extra_make_args = ""
 +xen_cc = ""
 +
 +def get_make_vars():
 +global xen_cc
 +invoke_make = utils.invoke_command(
 +"make -C {} {} export-variable-CC"
 +.format(settings.xen_dir, settings.make_forward_args),
 +True, GetMakeVarsPhaseError,
>>>

Re: [PATCH] Introduce more MISRA C rules to docs/misra/rules.rst

[Bertrand Marquis]

Hi,

> On 29 Nov 2022, at 00:18, Stefano Stabellini  wrote:
> 
> From: Stefano Stabellini 
> 
> Add the new MISRA C rules agreed by the MISRA C working group to
> docs/misra/rules.rst.
> 
> Add a comment for Rule 19.1 to explain that Eclair's findings are
> "caution" reports, not violations.
> 
> Signed-off-by: Stefano Stabellini 
Reviewed-by: Bertrand Marquis 

Cheers
Bertrand

> ---
> docs/misra/rules.rst | 36 
> 1 file changed, 36 insertions(+)
> 
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> index 8a659d8d47..dcceab9388 100644
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -77,11 +77,32 @@ existing codebase are work-in-progress.
>behaviour
>  -
> 
> +   * - `Rule 2.6 
> `_
> + - Advisory
> + - A function should not contain unused label declarations
> + -
> +
> +   * - `Rule 3.1 
> `_
> + - Required
> + - The character sequences /* and // shall not be used within a
> +   comment
> + -
> +
>* - `Rule 3.2 
> `_
>  - Required
>  - Line-splicing shall not be used in // comments
>  -
> 
> +   * - `Rule 4.1 
> `_
> + - Required
> + - Octal and hexadecimal escape sequences shall be terminated
> + -
> +
> +   * - `Rule 4.2 
> `_
> + - Advisory
> + - Trigraphs should not be used
> + -
> +
>* - `Rule 5.1 
> `_
>  - Required
>  - External identifiers shall be distinct
> @@ -200,6 +221,21 @@ existing codebase are work-in-progress.
>have an explicit return statement with an expression
>  -
> 
> +   * - `Rule 17.6 
> `_
> + - Mandatory
> + - The declaration of an array parameter shall not contain the
> +   static keyword between the [ ]
> + -
> +
> +   * - `Rule 19.1 
> `_
> + - Mandatory
> + - An object shall not be assigned or copied to an overlapping
> +   object
> + - Be aware that the static analysis tool Eclair might report
> +   several findings for Rule 19.1 of type "caution". These are
> +   instances where Eclair is unable to verify that the code is valid
> +   in regard to Rule 19.1. Caution reports are not violations.
> +
>* - `Rule 20.7 
> `_
>  - Required
>  - Expressions resulting from the expansion of macro parameters
> -- 
> 2.25.1
> 

[PATCH] hvc/xen: prevent concurrent accesses to the shared ring

[Roger Pau Monne]

The hvc machinery registers both a console and a tty device based on
the hv ops provided by the specific implementation.  Those two
interfaces however have different locks, and there's no single locks
that's shared between the tty and the console implementations, hence
the driver needs to protect itself against concurrent accesses.
Otherwise concurrent calls using the split interfaces are likely to
corrupt the ring indexes, leaving the console unusable.

Introduce a lock to xencons_info to serialize accesses to the shared
ring.  This is only required when using the shared memory console,
concurrent accesses to the hypercall based console implementation are
not an issue.

Note the conditional logic in domU_read_console() is slightly modified
so the notify_daemon() call can be done outside of the locked region:
it's an hypercall and there's no need for it to be done with the lock
held.

Fixes: b536b4b96230 ('xen: use the hvc console infrastructure for Xen console')
Signed-off-by: Roger Pau Monné 
---
While the write handler (domU_write_console()) is used by both the
console and the tty ops, that's not the case for the read side
(domU_read_console()).  It's not obvious to me whether we could get
concurrent poll calls from the poll_get_char tty hook, hence stay on
the safe side also serialize read accesses in domU_read_console().
---
 drivers/tty/hvc/hvc_xen.c | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index 7c23112dc923..d65741983837 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -43,6 +43,7 @@ struct xencons_info {
int irq;
int vtermno;
grant_ref_t gntref;
+   spinlock_t ring_lock;
 };
 
 static LIST_HEAD(xenconsoles);
@@ -84,12 +85,15 @@ static int __write_console(struct xencons_info *xencons,
XENCONS_RING_IDX cons, prod;
struct xencons_interface *intf = xencons->intf;
int sent = 0;
+   unsigned long flags;
 
+   spin_lock_irqsave(&xencons->ring_lock, flags);
cons = intf->out_cons;
prod = intf->out_prod;
mb();   /* update queue values before going on */
 
if ((prod - cons) > sizeof(intf->out)) {
+   spin_unlock_irqrestore(&xencons->ring_lock, flags);
pr_err_once("xencons: Illegal ring page indices");
return -EINVAL;
}
@@ -99,6 +103,7 @@ static int __write_console(struct xencons_info *xencons,
 
wmb();  /* write ring before updating pointer */
intf->out_prod = prod;
+   spin_unlock_irqrestore(&xencons->ring_lock, flags);
 
if (sent)
notify_daemon(xencons);
@@ -141,16 +146,19 @@ static int domU_read_console(uint32_t vtermno, char *buf, 
int len)
int recv = 0;
struct xencons_info *xencons = vtermno_to_xencons(vtermno);
unsigned int eoiflag = 0;
+   unsigned long flags;
 
if (xencons == NULL)
return -EINVAL;
intf = xencons->intf;
 
+   spin_lock_irqsave(&xencons->ring_lock, flags);
cons = intf->in_cons;
prod = intf->in_prod;
mb();   /* get pointers before reading ring */
 
if ((prod - cons) > sizeof(intf->in)) {
+   spin_unlock_irqrestore(&xencons->ring_lock, flags);
pr_err_once("xencons: Illegal ring page indices");
return -EINVAL;
}
@@ -174,10 +182,13 @@ static int domU_read_console(uint32_t vtermno, char *buf, 
int len)
xencons->out_cons = intf->out_cons;
xencons->out_cons_same = 0;
}
+   if (!recv && xencons->out_cons_same++ > 1) {
+   eoiflag = XEN_EOI_FLAG_SPURIOUS;
+   }
+   spin_unlock_irqrestore(&xencons->ring_lock, flags);
+
if (recv) {
notify_daemon(xencons);
-   } else if (xencons->out_cons_same++ > 1) {
-   eoiflag = XEN_EOI_FLAG_SPURIOUS;
}
 
xen_irq_lateeoi(xencons->irq, eoiflag);
@@ -576,6 +587,7 @@ static int __init xen_hvc_init(void)
 
info = vtermno_to_xencons(HVC_COOKIE);
info->irq = bind_evtchn_to_irq_lateeoi(info->evtchn);
+   spin_lock_init(&info->ring_lock);
}
if (info->irq < 0)
info->irq = 0; /* NO_IRQ */
-- 
2.37.3

[ovmf test] 174987: all pass - PUSHED

[osstest service owner]

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

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

Last test of basis   174986  2022-11-29 04:42:06 Z0 days
Testing same since   174987  2022-11-29 10:13:15 Z0 days1 attempts


People who touched revisions under test:
  Sunil V L 
  Zhihao Li 

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
   c8c978d328..b92e049522  b92e0495221a3b298b069d9fb01e48fd2a0469f6 -> 
xen-tested-master

Re: [PATCH 0/4] Static analyser finding deviation

[Luca Fancellu]

> On 29 Nov 2022, at 09:46, Luca Fancellu  wrote:
> 
> 
> 
>> On 29 Nov 2022, at 01:55, Stefano Stabellini  wrote:
>> 
>> On Mon, 28 Nov 2022, Luca Fancellu wrote:
>>> This serie introduces a way to suppress a static analyser finding providing 
>>> a
>>> proper justification for it.
>>> The process is explained in the docs/misra/documenting-violations.rst 
>>> document
>>> that this serie will provide.
>>> The tools currently supported are eclair, coverity and cppcheck, but the 
>>> design
>>> is open to support many other static analysis tool.
>>> 
>>> The changes are split between the first two patches to reduce the review 
>>> effort,
>>> the first patch is introducing the deviation process for the eclair and 
>>> coverity
>>> tools, this is because their analysis system is similar.
>>> 
>>> The second patch is introducing the same deviation process for cppcheck,
>>> modifying the current way it is called from the makefile and improving its
>>> analysis.
>>> 
>>> The third patch is a fix for a tool used for cppcheck and the fourth patch
>>> is an example of how a deviation can be applied for some MISRA findings.
> 
> Hi Stefano,
> 
>> 
>> I tried testing this series with:
>> 
>> # scripts/xen-analysis.py --build-only --cppcheck-html --run-cppcheck 
>> --cppcheck-bin=/local/repos/cppcheck/cppcheck 
>> --cppcheck-html-bin=/local/repos/cppcheck/htmlreport/cppcheck-htmlreport
>> 
>> But I get this error:
>> 
>> ERROR: Can't find cppcheck version or version is not 2.7
>> 
>> 
>> Note that my cppcheck is 2.7.4:
>> 
>> # ./cppcheck --version
>> Cppcheck 2.7.4
> 
> Yes this is a bug, I’m strictly checking for 2.7, I will modify it to 2.7.x 
> if you agree
> 
>> 
>> 
>> After removing the version check in cppcheck_analysis.py, the process
>> starts correctly.
>> 
>> Also, where is the output html report created by cppcheck-html by
>> default?
> 
> 
> The html output should be in the xen folder 
> [xen_repo]/xen/cppcheck-htmlreport/html but when you specify --build-only the 
> reports are not generated, only the build phase is executed.
> 
> Have you tried without --build-only to test the report generations?

However I’ve found another bug, when building using your command line (at least 
on my x86 machine)

I have that xen is not building and it’s ending with this:

ld-melf_x86_64  -T arch/x86/xen.lds -N prelink.o --build-id=sha1 \
./common/symbols-dummy.o -o ./.xen-syms.0
nm -pa --format=sysv ./.xen-syms.0 \
| ./tools/symbols --all-symbols --sort-by-name --sysv --sort \
>./.xen-syms.0.S
make -f ./Rules.mk obj=. ./.xen-syms.0.o
  CC  .xen-syms.0.o
ld-melf_x86_64  -T arch/x86/xen.lds -N prelink.o --build-id=sha1 \
./.xen-syms.0.o -o ./.xen-syms.1
nm -pa --format=sysv ./.xen-syms.1 \
| ./tools/symbols --all-symbols --sort-by-name --sysv --sort 
--error-dup \
>./.xen-syms.1.S
make -f ./Rules.mk obj=. ./.xen-syms.1.o
  CC  .xen-syms.1.o
ld-melf_x86_64  -T arch/x86/xen.lds -N prelink.o --build-id=sha1 \
--orphan-handling=warn ./.xen-syms.1.o -o xen-syms
nm -pa --format=sysv ./xen-syms \
| ./tools/symbols --all-symbols --xensyms --sysv --sort \
>./xen-syms.map
rm -f ./.xen-syms.[0-9]* ./..xen-syms.[0-9]*
  HOSTCC  arch/x86/efi/mkreloc
Checking arch/x86/efi/mkreloc.c ...
Checking arch/x86/efi/mkreloc.c: CPPCHECK=1;...
nm: 'arch/x86/efi/relocs-dummy.o': No such file
nm: 'arch/x86/efi/relocs-dummy.o': No such file
nm: 'arch/x86/efi/relocs-dummy.o': No such file
nm: 'arch/x86/efi/relocs-dummy.o': No such file
nm: 'arch/x86/efi/relocs-dummy.o': No such file
nm: 'arch/x86/efi/relocs-dummy.o': No such file
nm: 'arch/x86/efi/relocs-dummy.o': No such file
nm: 'arch/x86/efi/relocs-dummy.o': No such file
nm: 'arch/x86/efi/relocs-dummy.o': No such file
nm: 'arch/x86/efi/relocs-dummy.o': No such file
nm: 'arch/x86/efi/relocs-dummy.o': No such file
echo "Will strip debug info from xen.efi"
Will strip debug info from xen.efi
ld -mi386pep --subsystem=10 --strip-debug --image-base=0x --stack=0,0 
--heap=0,0 --section-alignment=0x20 --file-alignment=0x20 
--major-image-version=4 --minor-image-version=17 --major-os-version=2 
--minor-os-version=0 --major-subsystem-version=2 --minor-subsystem-version=0 
--build-id=sha1 -T arch/x86/efi.lds -N prelink.o arch/x86/efi/relocs-dummy.o 
./common/symbols-dummy.o -b pe-x86-64 arch/x86/efi/buildid.o -o ./.xen.efi.0x.0 
&&  ld -mi386pep --subsystem=10 --strip-debug --image-base=0x --stack=0,0 
--heap=0,0 --section-alignment=0x20 --file-alignment=0x20 
--major-image-version=4 --minor-image-version=17 --major-os-version=2 
--minor-os-version=0 --major-subsystem-version=2 --minor-subsystem-version=0 
--build-id=sha1 -T arch/x86/efi.lds -N prelink.o arch/x86/efi/relocs-dummy.o 
./common/symbols-dummy.o -b pe-x86-64 arch/x86/efi/buildid.o -o ./.xen.efi.0x.0 
&& :
ld: cannot find arch/x86/efi/relocs-dummy.o: No such file or directory
ld: cannot find arch/x86/efi/buildid.o: No such file or directory
arch/x86/Makefile

[linux-linus test] 174985: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-arm64-arm64-examine  8 reboot   fail REGR. vs. 173462
 test-arm64-arm64-xl-xsm   8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-xl-seattle   8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-xl-vhd   8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-xl-credit2   8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-xl-credit1   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-examine  8 reboot   fail REGR. vs. 173462
 test-armhf-armhf-xl   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-xl-credit2   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-xl-vhd   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-libvirt-raw  8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-xl   8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-libvirt-raw  8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-libvirt-xsm  8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-xl-multivcpu  8 xen-bootfail REGR. vs. 173462
 test-armhf-armhf-xl-arndale   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-xl-credit1   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-libvirt  8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-libvirt-qcow2  8 xen-boot   fail REGR. vs. 173462

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

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

version targeted for testing:
 linuxb7b275e60bcd5f89771e865a8239325f86d9927d
baseline version:
 linux9d84bb40bcb30a7fa16f33baa967aeb9953dda78

Last test of basis   173462  2022-10-07 18:41:45 Z   52 days
Failing since173470  2022-10-08 06:21:34 Z   52 days  100 attempts
Testing same since   174975  2022-11-27 23:39:38 Z1 days4 attempts


1906 people touched revisions under test,
not listing them all

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-xl  pass
 test-amd64-coresched-amd64-xlpass
 test-arm64-arm64-xl  fail
 test-armhf-armhf-xl  fail
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm   pass
 test-amd64-am

Arm: AArch32: Need suggestions to support 32 bit physical addresses

[Ayan Kumar Halder]

Hi All,

I am trying to gather opinions on how to support 32 bit physical 
addresses to enable Xen running on R52.


Refer Cortex R52 TRM, Section 2.2.12 "Memory Model"

"...This is because the physical address is always the same as the 
virtual address...The virtual and physical address can be treated as 
synonyms for Cortex-R52."


Thus, I understand that R52 supports 32 bit physical address only. This 
is a bit different from Armv7 systems which supports Large Physical 
Address Extension (LPAE) ie 40 bit physical addresses.


Please correct me if I misunderstand something.

So currently, Xen supports 64 bit physical address for Arm_32 (ie Armv7) 
based system. My aim is to enable support for 32 bit physical address.


To achieve this, I see two options :-

1. Make the changes in the specific functions for R52.

https://gitlab.com/ayankuma/xen-integration/-/commit/5498cf74c2c8feadc32934b948ce5f72bd0809ee

2. Introduce a special macro (CONFIG_ARM_32_BIT_PA) to support 32 bit 
physical address.


Thus, our change will look as follows :-

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 6014c0f852..4f8b5fc4be 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -56,10 +56,10 @@ static bool __init device_tree_node_compatible(const 
void *fdt, int node,

 }

 void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
-    u32 size_cells, u64 *start, u64 *size)
+    u32 size_cells, paddr_t *start, paddr_t 
*size)

 {
-    *start = dt_next_cell(address_cells, cell);
-    *size = dt_next_cell(size_cells, cell);
+    *start = (paddr_t) dt_next_cell(address_cells, cell);
+    *size = (paddr_t) dt_next_cell(size_cells, cell);
 }

 static int __init device_tree_get_meminfo(const void *fdt, int node,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index bd30d3798c..3cbcf8f854 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1314,7 +1314,7 @@ static int __init make_memory_node(const struct 
domain *d,

 dt_dprintk("Create memory node\n");

 /* ePAPR 3.4 */
-    snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[i].start);
+    snprintf(buf, sizeof(buf), "memory@%"PRIpaddr, mem->bank[i].start);
 res = fdt_begin_node(fdt, buf);
 if ( res )
 return res;
@@ -1665,7 +1665,7 @@ static int __init find_memory_holes(const struct 
kernel_info *kinfo,

 dt_for_each_device_node( dt_host, np )
 {
 unsigned int naddr;
-    u64 addr, size;
+    paddr_t addr, size;

 naddr = dt_number_of_address(np);

@@ -2444,7 +2444,7 @@ static int __init handle_device(struct domain *d, 
struct dt_device_node *dev,

 unsigned int naddr;
 unsigned int i;
 int res;
-    u64 addr, size;
+    paddr_t addr, size;
 bool own_device = !dt_device_for_passthrough(dev);
 /*
  * We want to avoid mapping the MMIO in dom0 for the following cases:
@@ -2718,7 +2718,7 @@ static int __init make_gicv2_domU_node(struct 
kernel_info *kinfo)

 /* Placeholder for interrupt-controller@ + a 64-bit number + \0 */
 char buf[38];

-    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
+    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIpaddr,
  vgic_dist_base(&d->arch.vgic));
 res = fdt_begin_node(fdt, buf);
 if ( res )
@@ -2774,7 +2774,7 @@ static int __init make_gicv3_domU_node(struct 
kernel_info *kinfo)

 char buf[38];
 unsigned int i, len = 0;

-    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
+    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIpaddr,
  vgic_dist_base(&d->arch.vgic));

 res = fdt_begin_node(fdt, buf);
@@ -2860,7 +2860,7 @@ static int __init make_vpl011_uart_node(struct 
kernel_info *kinfo)

 /* Placeholder for sbsa-uart@ + a 64-bit number + \0 */
 char buf[27];

-    snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, 
d->arch.vpl011.base_addr);
+    snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIpaddr, 
d->arch.vpl011.base_addr);

 res = fdt_begin_node(fdt, buf);
 if ( res )
 return res;
@@ -2940,9 +2940,9 @@ static int __init handle_passthrough_prop(struct 
kernel_info *kinfo,

 if ( res )
 {
 printk(XENLOG_ERR "Unable to permit to dom%d access to"
-   " 0x%"PRIx64" - 0x%"PRIx64"\n",
+   " 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",
    kinfo->d->domain_id,
-   mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1);
+   (paddr_t) (mstart & PAGE_MASK), (paddr_t) 
(PAGE_ALIGN(mstart + size) - 1));

 return res;
 }

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index ae5bd8e95f..839623c32e 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -1058,7 +1058,7 @@ static void __init gicv2_dt_init(void)
 if ( csize < SZ_8K )
 {
 printk(XENLOG_WARNING "GICv2: WARNING: "

Re: [PATCH RFC 07/10] domain: map/unmap GADDR based shared guest areas

[Jan Beulich]

On 29.11.2022 09:40, Julien Grall wrote:
> On 28/11/2022 10:01, Jan Beulich wrote:
>> On 24.11.2022 22:29, Julien Grall wrote:
>>> On 19/10/2022 09:43, Jan Beulich wrote:
 --- a/xen/common/domain.c
 +++ b/xen/common/domain.c
 @@ -1563,7 +1563,82 @@ int map_guest_area(struct vcpu *v, paddr
   struct guest_area *area,
   void (*populate)(void *dst, struct vcpu *v))
{
 -return -EOPNOTSUPP;
 +struct domain *currd = v->domain;
 +void *map = NULL;
 +struct page_info *pg = NULL;
 +int rc = 0;
 +
 +if ( gaddr )
>>>
>>> 0 is technically a valid (guest) physical address on Arm.
>>
>> I guess it is everywhere; it certainly also is on x86. While perhaps a
>> little unfortunate in ordering, the public header changes coming only
>> in the following patches was the best way I could think of to split
>> this work into reasonable size pieces. There the special meaning of 0
>> is clearly documented. And I don't really see it as a meaningful
>> limitation to not allow guests to register such areas at address zero.
> I would expect an OS to allocate the region using the generic physical 
> allocator. This allocator may decide that '0' is a valid address and 
> return it.
> 
> So I think your approach could potentially complicate the OS 
> implementation. I think it would be better to use an all Fs value as 
> this cannot be valid for this hypercall (we require at least 4-byte 
> alignment).

Valid point, yet my avoiding of an all-Fs value was specifically with
compat callers in mind - the values would be different for these and
native ones, which would make the check more clumsy (otherwise it
could simply be "if ( ~gaddr )").

 @@ -1573,6 +1648,22 @@ int map_guest_area(struct vcpu *v, paddr
 */
void unmap_guest_area(struct vcpu *v, struct guest_area *area)
{
 +struct domain *d = v->domain;
 +void *map;
 +struct page_info *pg;
>>>
>>> AFAIU, the assumption is the vCPU should be paused here.
>>
>> Yes, as the comment ahead of the function (introduced by an earlier
>> patch) says.
> 
> Ah, I missed that. Thanks!
> 
>>
>>> Should we add an ASSERT()?
>>
>> I was going from unmap_vcpu_info(), which had the same requirement,
>> while also only recording it by way of a comment. I certainly could
>> add an ASSERT(), but besides this being questionable as to the rules
>> set forth in ./CODING_STYLE I also view assertions of "paused" state
>> as being of limited use - the entity in question may become unpaused
>> on the clock cycle after the check was done. 
> 
> That's correct. However, that race you mention is unlikely to happen 
> *every* time. So there are a very high chance the ASSERT() will hit if 
> miscalled.
> 
>> (But yes, such are no
>> different from e.g. the fair number of spin_is_locked() checks we
>> have scattered around, which don't really provide guarantees either.)
> And that's fine to not provide the full guarantee. You are still 
> probably going to catch 95% (if not more) of the callers that forgot to 
> call it with the spin lock held.
> 
> At least for me, those ASSERTs() were super helpful during development 
> in more than a few cases.

Okay, I'll add one then, but in the earlier patch, matching the comment
that's introduced there.

Jan

[PATCH] hvc/xen: lock console list traversal

[Roger Pau Monne]

The currently lockless access to the xen console list in
vtermno_to_xencons() is incorrect, as additions and removals from the
list can happen anytime, and as such the traversal of the list to get
the private console data for a given termno needs to happen with the
lock held.  Note users that modify the list already do so with the
lock taken.

While there switch from using list_for_each_entry_safe to
list_for_each_entry: the current entry cursor won't be removed as
part of the code in the loop body, so using the _safe variant is
pointless.

Fixes: 02e19f9c7cac ('hvc_xen: implement multiconsole support')
Signed-off-by: Roger Pau Monné 
---
 drivers/tty/hvc/hvc_xen.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index d65741983837..117dc48f980e 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -53,17 +53,22 @@ static DEFINE_SPINLOCK(xencons_lock);
 
 static struct xencons_info *vtermno_to_xencons(int vtermno)
 {
-   struct xencons_info *entry, *n, *ret = NULL;
+   struct xencons_info *entry, *ret = NULL;
+   unsigned long flags;
 
-   if (list_empty(&xenconsoles))
-   return NULL;
+   spin_lock_irqsave(&xencons_lock, flags);
+   if (list_empty(&xenconsoles)) {
+   spin_unlock_irqrestore(&xencons_lock, flags);
+   return NULL;
+   }
 
-   list_for_each_entry_safe(entry, n, &xenconsoles, list) {
+   list_for_each_entry(entry, &xenconsoles, list) {
if (entry->vtermno == vtermno) {
ret  = entry;
break;
}
}
+   spin_unlock_irqrestore(&xencons_lock, flags);
 
return ret;
 }
-- 
2.37.3

Re: AMD GPU problems under Xen

[Alex Deucher]

On Mon, Nov 28, 2022 at 8:59 PM Demi Marie Obenour
 wrote:
>
> On Mon, Nov 28, 2022 at 11:18:00AM -0500, Alex Deucher wrote:
> > On Mon, Nov 28, 2022 at 2:18 AM Demi Marie Obenour
> >  wrote:
> > >
> > > Dear Christian:
> > >
> > > What is the status of the AMDGPU work for Xen dom0?  That was mentioned in
> > > https://lore.kernel.org/dri-devel/b2dec9b3-03a7-e7ac-306e-1da024af8...@amd.com/
> > > and there have been bug reports to Qubes OS about problems with AMDGPU
> > > under Xen (such as https://github.com/QubesOS/qubes-issues/issues/7648).
> >
> > I would say it's a work in progress.  It depends what GPU  you have
> > and what type of xen setup you are using (PV vs PVH, etc.).
>
> The current situation is:
>
> - dom0 is PV.
> - VMs with assigned PCI devices are HVM and use a Linux-based stubdomain
>   QEMU does not run in dom0.
> - Everything else is PVH.
>
> In the future, I believe the goal is to move away from PV and HVM in
> favor of PVH, though HVM support will remain for compatibility with
> guests (such as Windows) that need emulated devices.
>
> > In general, your best bet currently is dGPU add in boards because they
> > are largely self contained.
>
> The main problem is that for the trusted GUI to work, there needs to
> be at least one GPU attached to a trusted VM, such as the host or a
> dedicated GUI VM.  That VM will typically not be running graphics-
> intensive workloads, so the compute power of a dGPU is largely wasted.
> SR-IOV support would help with that, but the only GPU vendor with open
> source SR-IOV support is Intel and it is still not upstream.  I am also
> not certain if the support extends to Arc dGPUs.

Can you elaborate on this?  Why wouldn't you just want to pass-through
a dGPU to a domU to use directly in the guest?
Are you sure?  I didn't think intel's GVT solution was actually
SR-IOV.  I think GVT is just a paravirtualized solution.  That aside,
we are working on enabling virtio gpu with our GPUs on xen in addition
to domU passthrough.

>
> > APUs and platforms with integrated dGPUs
> > are a bit more complicated as they tend to have more platform
> > dependencies like ACPI tables and methods in order for the driver to
> > be able to initialize the hardware properly.
>
> Is Xen dom0/domU support for such GPUs being worked on?  Is there an
> estimate as to when the needed support will be available upstream?  This
> is mostly directed at Christian and other people who work for hardware
> vendors.

Yes, there are some minor fixes in the driver required which we'll be
sending out soon and we had to add some ACPI tables to the whitelist
in xen, but unfortunately the ACPI tables are AMD platform specific so
there has been pushback from the xen maintainers on accepting them
because they are not an official part of the ACPI spec.

Alex

>
> > Additionally, GPUs map a
> > lot of system memory so bounce buffers aren't really viable.  You'll
> > really need IOMMU,
>
> Qubes OS already needs an IOMMU so that is not a concern.
> --
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab

Re: [XEN v4 07/11] xen/Arm: GICv3: Define ICH_LR_EL2 on AArch32

[Michal Orzel]

Hi Ayan,

On 28/11/2022 16:56, Ayan Kumar Halder wrote:
> 
> 
> Refer "Arm IHI 0069H ID020922", 12.4.6, Interrupt Controller List Registers
> 
> AArch64 System register ICH_LR_EL2 bits [31:0] are architecturally
> mapped to AArch32 System register ICH_LR[31:0].
> AArch64 System register ICH_LR_EL2 bits [63:32] are architecturally
> mapped to AArch32 System register ICH_LRC[31:0].
> 
> Defined ICH_LR<0...15>_EL2 and ICH_LRC<0...15>_EL2 for AArch32.
> For AArch32, the link register is stored as :-
> (((uint64_t) ICH_LRC<0...15>_EL2) << 32) | ICH_LR<0...15>_EL2
> 
> Also, ICR_LR macros need to be modified as ULL is 64 bits for AArch32 and
> AArch64.
> 
> Signed-off-by: Ayan Kumar Halder 

Reviewed-by: Michal Orzel ,
with two remarks (this is up to maintainers as these are rather cosmetic 
changes).

> ---
> 
> Changes from :-
> v1 - 1. Moved the coproc register definitions to asm/cpregs.h.
> 2. Use GENMASK(31, 0) to represent 0x
> 3. Use READ_CP32()/WRITE_CP32() instead of READ_SYSREG()/WRITE_SYSREG().
> 4. Multi-line macro definitions should be enclosed within ({ }).
> 
> v2 - 1. Use WRITE_SYSREG_LR(V, R) to make it consistent with before.
> 2. Defined the register alias.
> 3. Style issues.
> 
> v3 - 1. Addressed style issues.
> 
>  xen/arch/arm/gic-v3.c| 132 +++
>  xen/arch/arm/include/asm/arm32/sysregs.h |  19 
>  xen/arch/arm/include/asm/arm64/sysregs.h |   5 +
>  xen/arch/arm/include/asm/cpregs.h|  76 +
>  xen/arch/arm/include/asm/gic_v3_defs.h   |   8 +-
>  5 files changed, 170 insertions(+), 70 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 64a76307dd..6457e7033c 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -73,37 +73,37 @@ static inline void gicv3_save_lrs(struct vcpu *v)
>  switch ( gicv3_info.nr_lrs )
>  {
>  case 16:
> -v->arch.gic.v3.lr[15] = READ_SYSREG(ICH_LR15_EL2);
> +v->arch.gic.v3.lr[15] = READ_SYSREG_LR(15);
>  case 15:
> -v->arch.gic.v3.lr[14] = READ_SYSREG(ICH_LR14_EL2);
> +v->arch.gic.v3.lr[14] = READ_SYSREG_LR(14);
>  case 14:
> -v->arch.gic.v3.lr[13] = READ_SYSREG(ICH_LR13_EL2);
> +v->arch.gic.v3.lr[13] = READ_SYSREG_LR(13);
>  case 13:
> -v->arch.gic.v3.lr[12] = READ_SYSREG(ICH_LR12_EL2);
> +v->arch.gic.v3.lr[12] = READ_SYSREG_LR(12);
>  case 12:
> -v->arch.gic.v3.lr[11] = READ_SYSREG(ICH_LR11_EL2);
> +v->arch.gic.v3.lr[11] = READ_SYSREG_LR(11);
>  case 11:
> -v->arch.gic.v3.lr[10] = READ_SYSREG(ICH_LR10_EL2);
> +v->arch.gic.v3.lr[10] = READ_SYSREG_LR(10);
>  case 10:
> -v->arch.gic.v3.lr[9] = READ_SYSREG(ICH_LR9_EL2);
> +v->arch.gic.v3.lr[9] = READ_SYSREG_LR(9);
>  case 9:
> -v->arch.gic.v3.lr[8] = READ_SYSREG(ICH_LR8_EL2);
> +v->arch.gic.v3.lr[8] = READ_SYSREG_LR(8);
>  case 8:
> -v->arch.gic.v3.lr[7] = READ_SYSREG(ICH_LR7_EL2);
> +v->arch.gic.v3.lr[7] = READ_SYSREG_LR(7);
>  case 7:
> -v->arch.gic.v3.lr[6] = READ_SYSREG(ICH_LR6_EL2);
> +v->arch.gic.v3.lr[6] = READ_SYSREG_LR(6);
>  case 6:
> -v->arch.gic.v3.lr[5] = READ_SYSREG(ICH_LR5_EL2);
> +v->arch.gic.v3.lr[5] = READ_SYSREG_LR(5);
>  case 5:
> -v->arch.gic.v3.lr[4] = READ_SYSREG(ICH_LR4_EL2);
> +v->arch.gic.v3.lr[4] = READ_SYSREG_LR(4);
>  case 4:
> -v->arch.gic.v3.lr[3] = READ_SYSREG(ICH_LR3_EL2);
> +v->arch.gic.v3.lr[3] = READ_SYSREG_LR(3);
>  case 3:
> -v->arch.gic.v3.lr[2] = READ_SYSREG(ICH_LR2_EL2);
> +v->arch.gic.v3.lr[2] = READ_SYSREG_LR(2);
>  case 2:
> -v->arch.gic.v3.lr[1] = READ_SYSREG(ICH_LR1_EL2);
> +v->arch.gic.v3.lr[1] = READ_SYSREG_LR(1);
>  case 1:
> - v->arch.gic.v3.lr[0] = READ_SYSREG(ICH_LR0_EL2);
> + v->arch.gic.v3.lr[0] = READ_SYSREG_LR(0);
>   break;
>  default:
>   BUG();
> @@ -120,37 +120,37 @@ static inline void gicv3_restore_lrs(const struct vcpu 
> *v)
>  switch ( gicv3_info.nr_lrs )
>  {
>  case 16:
> -WRITE_SYSREG(v->arch.gic.v3.lr[15], ICH_LR15_EL2);
> +WRITE_SYSREG_LR(v->arch.gic.v3.lr[15], 15);
>  case 15:
> -WRITE_SYSREG(v->arch.gic.v3.lr[14], ICH_LR14_EL2);
> +WRITE_SYSREG_LR(v->arch.gic.v3.lr[14], 14);
>  case 14:
> -WRITE_SYSREG(v->arch.gic.v3.lr[13], ICH_LR13_EL2);
> +WRITE_SYSREG_LR(v->arch.gic.v3.lr[13], 13);
>  case 13:
> -WRITE_SYSREG(v->arch.gic.v3.lr[12], ICH_LR12_EL2);
> +WRITE_SYSREG_LR(v->arch.gic.v3.lr[12], 12);
>  case 12:
> -WRITE_SYSREG(v->arch.gic.v3.lr[11], ICH_LR11_EL2);
> +WRITE_SYSREG_LR(v->arch.gic.v3.lr[11], 11);
>  case 11:
> -WRITE_SYSREG(v->arch.gic.v3.lr[10], ICH_LR10_EL2);
> +WRITE_SYSREG_LR(v->arch.gic.v3.lr[10], 10);
>  case 10:
> -WRITE_SYSREG(v->arch

Re: [XEN v4 08/11] xen/Arm: GICv3: Define ICH_AP0R and ICH_AP1R for AArch32

[Michal Orzel]

Hi Ayan,

On 28/11/2022 16:56, Ayan Kumar Halder wrote:
> Adapt save_aprn_regs()/restore_aprn_regs() for AArch32.
> 
> For which we have defined the following registers:-
> 1. Interrupt Controller Hyp Active Priorities Group0 Registers 0-3
> 2. Interrupt Controller Hyp Active Priorities Group1 Registers 0-3
> 
> Signed-off-by: Ayan Kumar Halder 
Reviewed-by: Michal Orzel 

~Michal

[PATCH] bump default SeaBIOS version to 1.16.1

[Jan Beulich]

Signed-off-by: Jan Beulich 

--- a/Config.mk
+++ b/Config.mk
@@ -232,7 +232,7 @@ OVMF_UPSTREAM_REVISION ?= 7b4a99be8a39c1
 QEMU_UPSTREAM_REVISION ?= b746458e1ce1bec85e58b458386f8b7a0bedfaa6
 MINIOS_UPSTREAM_REVISION ?= 5bcb28aaeba1c2506a82fab0cdad0201cd9b54b3
 
-SEABIOS_UPSTREAM_REVISION ?= rel-1.16.0
+SEABIOS_UPSTREAM_REVISION ?= rel-1.16.1
 
 ETHERBOOT_NICS ?= rtl8139 8086100e
 

[PATCH] Arm64: make setup_virt_paging()'s pa_range_info[] static

[Jan Beulich]

While not as inefficient as it would be on x86 (due to suitable constant
loading and register pair storing instructions being available to fill
some of the fields), having the compiler construct an array of constants
on the stack still looks odd to me.

Signed-off-by: Jan Beulich 
---
Actual space savings could be had if further converting the field types
to e.g. unsigned char (all of the values fit in that type).

--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -2281,12 +2281,12 @@ void __init setup_virt_paging(void)
 val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
 val |= VTCR_SL0(0x1); /* P2M starts at first level */
 #else /* CONFIG_ARM_64 */
-const struct {
+static const struct {
 unsigned int pabits; /* Physical Address Size */
 unsigned int t0sz;   /* Desired T0SZ, minimum in comment */
 unsigned int root_order; /* Page order of the root of the p2m */
 unsigned int sl0;/* Desired SL0, maximum in comment */
-} pa_range_info[] = {
+} pa_range_info[] __initconst = {
 /* T0SZ minimum and SL0 maximum from ARM DDI 0487H.a Table D5-6 */
 /*  PA size, t0sz(min), root-order, sl0(max) */
 [0] = { 32,  32/*32*/,  0,  1 },

[PATCH] Arm/P2M: reduce locking in p2m_{alloc,free}_page()

[Jan Beulich]

It is generally preferable to not hold locks around allocation
functions. And indeed in the hwdom case there's no point at all to hold
the paging lock. Reduce the locked regions to the non-hwdom case, while
at the same time arranging for p2m_alloc_page() to have just a single
return point.

Signed-off-by: Jan Beulich 

--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -48,7 +48,6 @@ static struct page_info *p2m_alloc_page(
 {
 struct page_info *pg;
 
-spin_lock(&d->arch.paging.lock);
 /*
  * For hardware domain, there should be no limit in the number of pages 
that
  * can be allocated, so that the kernel may take advantage of the extended
@@ -58,34 +57,28 @@ static struct page_info *p2m_alloc_page(
 {
 pg = alloc_domheap_page(NULL, 0);
 if ( pg == NULL )
-{
 printk(XENLOG_G_ERR "Failed to allocate P2M pages for hwdom.\n");
-spin_unlock(&d->arch.paging.lock);
-return NULL;
-}
 }
 else
 {
+spin_lock(&d->arch.paging.lock);
 pg = page_list_remove_head(&d->arch.paging.p2m_freelist);
-if ( unlikely(!pg) )
-{
-spin_unlock(&d->arch.paging.lock);
-return NULL;
-}
+spin_unlock(&d->arch.paging.lock);
 }
-spin_unlock(&d->arch.paging.lock);
 
 return pg;
 }
 
 static void p2m_free_page(struct domain *d, struct page_info *pg)
 {
-spin_lock(&d->arch.paging.lock);
 if ( is_hardware_domain(d) )
 free_domheap_page(pg);
 else
+{
+spin_lock(&d->arch.paging.lock);
 page_list_add_tail(pg, &d->arch.paging.p2m_freelist);
-spin_unlock(&d->arch.paging.lock);
+spin_unlock(&d->arch.paging.lock);
+}
 }
 
 /* Return the size of the pool, in bytes. */

[PATCH] x86/APIC: make a few interrupt handler functions static

[Jan Beulich]

Four of them are used in apic.c only and hence better wouldn't be
exposed to other CUs. To avoid the need for forward declarations, move
apic_intr_init() past the four handlers.

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -127,21 +127,6 @@ void ack_bad_irq(unsigned int irq)
 ack_APIC_irq();
 }
 
-void __init apic_intr_init(void)
-{
-smp_intr_init();
-
-/* self generated IPI for local APIC timer */
-set_direct_apic_vector(LOCAL_TIMER_VECTOR, apic_timer_interrupt);
-
-/* IPI vectors for APIC spurious and error interrupts */
-set_direct_apic_vector(SPURIOUS_APIC_VECTOR, spurious_interrupt);
-set_direct_apic_vector(ERROR_APIC_VECTOR, error_interrupt);
-
-/* Performance Counters Interrupt */
-set_direct_apic_vector(PMU_APIC_VECTOR, pmu_apic_interrupt);
-}
-
 /* Using APIC to generate smp_local_timer_interrupt? */
 static bool __read_mostly using_apic_timer;
 
@@ -1363,7 +1348,7 @@ int reprogram_timer(s_time_t timeout)
 return apic_tmict || !timeout;
 }
 
-void cf_check apic_timer_interrupt(struct cpu_user_regs *regs)
+static void cf_check apic_timer_interrupt(struct cpu_user_regs *regs)
 {
 ack_APIC_irq();
 perfc_incr(apic_timer);
@@ -1382,7 +1367,7 @@ void smp_send_state_dump(unsigned int cp
 /*
  * Spurious interrupts should _never_ happen with our APIC/SMP architecture.
  */
-void cf_check spurious_interrupt(struct cpu_user_regs *regs)
+static void cf_check spurious_interrupt(struct cpu_user_regs *regs)
 {
 /*
  * Check if this is a vectored interrupt (most likely, as this is probably
@@ -1413,7 +1398,7 @@ void cf_check spurious_interrupt(struct
  * This interrupt should never happen with our APIC/SMP architecture
  */
 
-void cf_check error_interrupt(struct cpu_user_regs *regs)
+static void cf_check error_interrupt(struct cpu_user_regs *regs)
 {
 static const char *const esr_fields[] = {
 "Send CS error",
@@ -1446,12 +1431,27 @@ void cf_check error_interrupt(struct cpu
  * This interrupt handles performance counters interrupt
  */
 
-void cf_check pmu_apic_interrupt(struct cpu_user_regs *regs)
+static void cf_check pmu_apic_interrupt(struct cpu_user_regs *regs)
 {
 ack_APIC_irq();
 vpmu_do_interrupt(regs);
 }
 
+void __init apic_intr_init(void)
+{
+smp_intr_init();
+
+/* self generated IPI for local APIC timer */
+set_direct_apic_vector(LOCAL_TIMER_VECTOR, apic_timer_interrupt);
+
+/* IPI vectors for APIC spurious and error interrupts */
+set_direct_apic_vector(SPURIOUS_APIC_VECTOR, spurious_interrupt);
+set_direct_apic_vector(ERROR_APIC_VECTOR, error_interrupt);
+
+/* Performance Counters Interrupt */
+set_direct_apic_vector(PMU_APIC_VECTOR, pmu_apic_interrupt);
+}
+
 /*
  * This initializes the IO-APIC and APIC hardware if this is
  * a UP kernel.
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -96,10 +96,6 @@ static inline struct cpu_user_regs *set_
 void cf_check event_check_interrupt(struct cpu_user_regs *regs);
 void cf_check invalidate_interrupt(struct cpu_user_regs *regs);
 void cf_check call_function_interrupt(struct cpu_user_regs *regs);
-void cf_check apic_timer_interrupt(struct cpu_user_regs *regs);
-void cf_check error_interrupt(struct cpu_user_regs *regs);
-void cf_check pmu_apic_interrupt(struct cpu_user_regs *regs);
-void cf_check spurious_interrupt(struct cpu_user_regs *regs);
 void cf_check irq_move_cleanup_interrupt(struct cpu_user_regs *regs);
 
 uint8_t alloc_hipriority_vector(void);

[PATCH] x86/p2m: don't calculate page owner twice in p2m_add_page()

[Jan Beulich]

Neither page_get_owner() nor mfn_to_page() are entirely trivial
operations - don't do the same thing twice in close succession. Instead
help CSE (when MEM_SHARING=y) by introducing a local variable holding
the page owner.

Signed-off-by: Jan Beulich 
---
According to my observations gcc12 manages to CSE mfn_to_page() but not
(all of) page_get_owner(). The overall savings there are, partly due to
knock-on effects, 64 bytes of code.

While looking there, "mfn_eq(omfn, mfn_add(mfn, i))" near the end of the
same loop caught my eye: Is that really correct? Shouldn't we fail the
operation if the MFN which "ogfn" was derived from doesn't match the MFN
"ogfn" maps to? Excluding grant mappings here is of course okay, but
that's already taken care of by the enclosing p2m_is_ram().

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -691,8 +691,10 @@ p2m_add_page(struct domain *d, gfn_t gfn
 /* Then, look for m->p mappings for this range and deal with them */
 for ( i = 0; i < (1UL << page_order); i++ )
 {
-if ( dom_cow &&
- page_get_owner(mfn_to_page(mfn_add(mfn, i))) == dom_cow )
+const struct domain *owner =
+page_get_owner(mfn_to_page(mfn_add(mfn, i)));
+
+if ( dom_cow && owner == dom_cow )
 {
 /* This is no way to add a shared page to your physmap! */
 gdprintk(XENLOG_ERR, "Adding shared mfn %lx directly to dom%d 
physmap not allowed.\n",
@@ -700,7 +702,7 @@ p2m_add_page(struct domain *d, gfn_t gfn
 p2m_unlock(p2m);
 return -EINVAL;
 }
-if ( page_get_owner(mfn_to_page(mfn_add(mfn, i))) != d )
+if ( owner != d )
 continue;
 ogfn = mfn_to_gfn(d, mfn_add(mfn, i));
 if ( !gfn_eq(ogfn, _gfn(INVALID_M2P_ENTRY)) &&

[PATCH] x86/MSR: use latched "current"

[Jan Beulich]

There's no need to recalculate / refetch the value from the stack
(pointer).

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -417,7 +417,7 @@ int guest_rdmsr(struct vcpu *v, uint32_t
  * out of hardware.
  */
 #ifdef CONFIG_HVM
-if ( v == current && is_hvm_domain(d) && v->arch.hvm.flag_dr_dirty )
+if ( v == curr && is_hvm_domain(d) && v->arch.hvm.flag_dr_dirty )
 rdmsrl(msr, *val);
 else
 #endif
@@ -639,7 +639,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t
 {
 uint64_t xcr0 = get_xcr0();
 
-if ( v != current ||
+if ( v != curr ||
  handle_xsetbv(XCR_XFEATURE_ENABLED_MASK,
xcr0 | X86_XCR0_BNDREGS | X86_XCR0_BNDCSR) )
 goto gp_fault;

[PATCH] x86/HVM: drop stale check from hvm_load_cpu_msrs()

[Jan Beulich]

Up until f61685a66903 ("x86: remove defunct init/load/save_msr()
hvm_funcs") the check of the _rsvd field served as an error check for
the earlier hvm_funcs.save_msr() invocation. With that invocation gone
the check makes no sense anymore. While dropping it also merge the two
paths setting "err" to -ENXIO.

Signed-off-by: Jan Beulich 
---
We could go further here, removing the local "err" variable altogether,
by using "return -ENXIO". Thoughts.

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1494,13 +1494,12 @@ static int cf_check hvm_load_cpu_msrs(st
 case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
 rc = guest_wrmsr(v, ctxt->msr[i].index, ctxt->msr[i].val);
 
-if ( rc != X86EMUL_OKAY )
-err = -ENXIO;
-break;
+if ( rc == X86EMUL_OKAY )
+continue;
 
+fallthrough;
 default:
-if ( !ctxt->msr[i]._rsvd )
-err = -ENXIO;
+err = -ENXIO;
 break;
 }
 }

Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses

[Julien Grall]

On 29/11/2022 14:57, Ayan Kumar Halder wrote:

Hi All,


Hi,

I am trying to gather opinions on how to support 32 bit physical 
addresses to enable Xen running on R52.


Refer Cortex R52 TRM, Section 2.2.12 "Memory Model"

"...This is because the physical address is always the same as the 
virtual address...The virtual and physical address can be treated as 
synonyms for Cortex-R52."


Thus, I understand that R52 supports 32 bit physical address only. This 
is a bit different from Armv7 systems which supports Large Physical 
Address Extension (LPAE) ie 40 bit physical addresses. >

Please correct me if I misunderstand something. >
So currently, Xen supports 64 bit physical address for Arm_32 (ie Armv7) 
based system.


Xen supports *up to* 64-bit physical address. This may be lower in the 
HW (not all the Armv7 HW supports 40-bit address).



My aim is to enable support for 32 bit physical address.


Technically this is already supported because this is a subset of 
64-bit. I can see a use case (even on non R* HW) where you may want to 
use 32-bit paddr_t to reduce the code size (and registers used).


But I think that's more an optimization that rather than been necessary.


diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 6014c0f852..4f8b5fc4be 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -56,10 +56,10 @@ static bool __init device_tree_node_compatible(const 
void *fdt, int node,

  }

  void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
-    u32 size_cells, u64 *start, u64 *size)
+    u32 size_cells, paddr_t *start, paddr_t 
*size)


This needs to stay uint64_t because the Device-Tree may contain 64-bit 
values and you...



  {
-    *start = dt_next_cell(address_cells, cell);
-    *size = dt_next_cell(size_cells, cell);
+    *start = (paddr_t) dt_next_cell(address_cells, cell);
+    *size = (paddr_t) dt_next_cell(size_cells, cell);


... don't want to always blindly cast it. That's up to the caller to 
check that the top 32-bit are zeroed and downcast it.



  }

  static int __init device_tree_get_meminfo(const void *fdt, int node,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index bd30d3798c..3cbcf8f854 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1314,7 +1314,7 @@ static int __init make_memory_node(const struct 
domain *d,

  dt_dprintk("Create memory node\n");

  /* ePAPR 3.4 */
-    snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[i].start);
+    snprintf(buf, sizeof(buf), "memory@%"PRIpaddr, mem->bank[i].start);
  res = fdt_begin_node(fdt, buf);
  if ( res )
  return res;
@@ -1665,7 +1665,7 @@ static int __init find_memory_holes(const struct 
kernel_info *kinfo,

  dt_for_each_device_node( dt_host, np )
  {
  unsigned int naddr;
-    u64 addr, size;
+    paddr_t addr, size;

  naddr = dt_number_of_address(np);

@@ -2444,7 +2444,7 @@ static int __init handle_device(struct domain *d, 
struct dt_device_node *dev,

  unsigned int naddr;
  unsigned int i;
  int res;
-    u64 addr, size;
+    paddr_t addr, size;
  bool own_device = !dt_device_for_passthrough(dev);
  /*
   * We want to avoid mapping the MMIO in dom0 for the following cases:
@@ -2718,7 +2718,7 @@ static int __init make_gicv2_domU_node(struct 
kernel_info *kinfo)

  /* Placeholder for interrupt-controller@ + a 64-bit number + \0 */
  char buf[38];

-    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
+    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIpaddr,
   vgic_dist_base(&d->arch.vgic));
  res = fdt_begin_node(fdt, buf);
  if ( res )
@@ -2774,7 +2774,7 @@ static int __init make_gicv3_domU_node(struct 
kernel_info *kinfo)

  char buf[38];
  unsigned int i, len = 0;

-    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
+    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIpaddr,
   vgic_dist_base(&d->arch.vgic));

  res = fdt_begin_node(fdt, buf);
@@ -2860,7 +2860,7 @@ static int __init make_vpl011_uart_node(struct 
kernel_info *kinfo)

  /* Placeholder for sbsa-uart@ + a 64-bit number + \0 */
  char buf[27];

-    snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, 
d->arch.vpl011.base_addr);
+    snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIpaddr, 
d->arch.vpl011.base_addr);

  res = fdt_begin_node(fdt, buf);
  if ( res )
  return res;
@@ -2940,9 +2940,9 @@ static int __init handle_passthrough_prop(struct 
kernel_info *kinfo,

  if ( res )
  {
  printk(XENLOG_ERR "Unable to permit to dom%d access to"
-   " 0x%"PRIx64" - 0x%"PRIx64"\n",
+   " 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",


What's wrong with printing using PRIx64? At least...


     kinfo->d->domain_id,
-   mstart & PAGE_MASK,

[PATCH] x86/mm: PGC_page_table is used by shadow code only

[Jan Beulich]

By defining the constant to zero when !SHADOW_PAGING we give compilers
the chance to eliminate a little more dead code elsewhere in the tree.
Plus, as a minor benefit, the general reference count can be one bit
wider. (To simplify things, have PGC_page_table change places with
PGC_extra.)

Signed-off-by: Jan Beulich 
---
tboot.c's update_pagetable_mac() is suspicious: It effectively is a
no-op even prior to this change when !SHADOW_PAGING, which can't be
quite right. If (guest) page tables are relevant to include in the
verification, shouldn't this look for PGT_l_page_table as well? How
to deal with HAP guests there is entirely unclear.

--- a/xen/arch/x86/include/asm/mm.h
+++ b/xen/arch/x86/include/asm/mm.h
@@ -70,9 +70,9 @@
  /* Page is Xen heap? */
 #define _PGC_xen_heap PG_shift(2)
 #define PGC_xen_heap  PG_mask(1, 2)
- /* Set when is using a page as a page table */
-#define _PGC_page_table   PG_shift(3)
-#define PGC_page_tablePG_mask(1, 3)
+ /* Page is not reference counted */
+#define _PGC_extraPG_shift(3)
+#define PGC_extra PG_mask(1, 3)
  /* Page is broken? */
 #define _PGC_broken   PG_shift(4)
 #define PGC_brokenPG_mask(1, 4)
@@ -83,12 +83,20 @@
 #define PGC_state_offlined  PG_mask(2, 6)
 #define PGC_state_free  PG_mask(3, 6)
 #define page_state_is(pg, st) (((pg)->count_info&PGC_state) == PGC_state_##st)
-/* Page is not reference counted */
-#define _PGC_extraPG_shift(7)
-#define PGC_extra PG_mask(1, 7)
+#ifdef CONFIG_SHADOW_PAGING
+ /* Set when is using a page as a page table */
+#define _PGC_page_table   PG_shift(7)
+#define PGC_page_tablePG_mask(1, 7)
+#else
+#define PGC_page_table0
+#endif
 
 /* Count of references to this frame. */
+#if PGC_page_table
 #define PGC_count_width   PG_shift(7)
+#else
+#define PGC_count_width   PG_shift(6)
+#endif
 #define PGC_count_mask((1UL<

Re: [XEN v4 09/11] xen/Arm: GICv3: Define remaining GIC registers for AArch32

[Michal Orzel]

Hi Ayan,

On 28/11/2022 16:56, Ayan Kumar Halder wrote:
> Define missing assembly aliases for GIC registers on arm32, taking the ones
> defined already for arm64 as a base. Aliases are defined according to the
> GIC Architecture Specification ARM IHI 0069H.
> 
> Defined the following registers:-
> 1. Interrupt Controller Interrupt Priority Mask Register
> 2. Interrupt Controller System Register Enable register
> 3. Interrupt Controller Deactivate Interrupt Register
> 4. Interrupt Controller End Of Interrupt Register 1
> 5. Interrupt Controller Interrupt Acknowledge Register 1
> 6. Interrupt Controller Binary Point Register 1
> 7. Interrupt Controller Control Register
> 8. Interrupt Controller Interrupt Group 1 Enable register
> 9. Interrupt Controller Maintenance Interrupt State Register
> 10. Interrupt Controller End of Interrupt Status Register
> 11. Interrupt Controller Empty List Register Status Register
> 12. Interrupt Controller Virtual Machine Control Register
> 
> Signed-off-by: Ayan Kumar Halder 
Reviewed-by: Michal Orzel 
with one remark...

> ---
> 
> Changes from :-
> v1 - 1. Moved coproc regs definition to asm/cpregs.h
> 
> v2 - 1. Defined register alias.
> 2. Style issues.
> 3. Defined ELSR, MISR, EISR to make it consistent with AArch64.
> 
> v3 - 1. Rectified some of the register names.
>  
>  xen/arch/arm/include/asm/cpregs.h | 32 +++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/cpregs.h 
> b/xen/arch/arm/include/asm/cpregs.h
> index 53142fc2b2..8f4d097a15 100644
> --- a/xen/arch/arm/include/asm/cpregs.h
> +++ b/xen/arch/arm/include/asm/cpregs.h
> @@ -163,6 +163,7 @@
>  #define DACRp15,0,c3,c0,0   /* Domain Access Control Register */
>  
>  /* CP15 CR4: */
> +#define ICC_PMR p15,0,c4,c6,0   /* Interrupt Priority Mask Register 
> */
>  
>  /* CP15 CR5: Fault Status Registers */
>  #define DFSRp15,0,c5,c0,0   /* Data Fault Status Register */
> @@ -256,6 +257,7 @@
>  
>  /* CP15 CR12:  */
>  #define ICC_SGI1R   p15,0,c12   /* Interrupt Controller SGI Group 1 
> */
> +#define ICC_DIR p15,0,c12,c11,1 /* Interrupt Controller Deactivate 
> Interrupt Register */
Shouldn't ICC_DIR be placed after VBAR?

>  #define ICC_ASGI1R  p15,1,c12   /* Interrupt Controller Alias SGI 
> Group 1 Register */
>  #define ICC_SGI0R   p15,2,c12   /* Interrupt Controller SGI Group 0 
> */
>  #define VBARp15,0,c12,c0,0  /* Vector Base Address Register */
> @@ -281,6 +283,20 @@
>  #define ICH_AP1R2   __AP1Rx(2)
>  #define ICH_AP1R3   __AP1Rx(3)
>  
> +#define ICC_IAR1p15,0,c12,c12,0  /* Interrupt Controller Interrupt 
> Acknowledge Register 1 */
> +#define ICC_EOIR1   p15,0,c12,c12,1  /* Interrupt Controller End Of 
> Interrupt Register 1 */
> +#define ICC_BPR1p15,0,c12,c12,3  /* Interrupt Controller Binary 
> Point Register 1 */
> +#define ICC_CTLRp15,0,c12,c12,4  /* Interrupt Controller Control 
> Register */
> +#define ICC_SRE p15,0,c12,c12,5  /* Interrupt Controller System 
> Register Enable register */
> +#define ICC_IGRPEN1 p15,0,c12,c12,7  /* Interrupt Controller Interrupt 
> Group 1 Enable register */
> +#define ICC_HSREp15,4,c12,c9,5   /* Interrupt Controller Hyp System 
> Register Enable register */
> +#define ICH_HCR p15,4,c12,c11,0  /* Interrupt Controller Hyp Control 
> Register */
> +#define ICH_VTR p15,4,c12,c11,1  /* Interrupt Controller VGIC Type 
> Register */
> +#define ICH_MISRp15,4,c12,c11,2  /* Interrupt Controller Maintenance 
> Interrupt State Register */
> +#define ICH_EISRp15,4,c12,c11,3  /* Interrupt Controller End of 
> Interrupt Status Register */
> +#define ICH_ELRSR   p15,4,c12,c11,5  /* Interrupt Controller Empty List 
> Register Status Register */
> +#define ICH_VMCRp15,4,c12,c11,7  /* Interrupt Controller Virtual 
> Machine Control Register */
> +
>  /* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
>  #define __LR0(x)___CP32(p15, 4, c12, c12, x)
>  #define __LR8(x)___CP32(p15, 4, c12, c13, x)
> @@ -381,6 +397,15 @@
>  #define HCR_EL2 HCR
>  #define HPFAR_EL2   HPFAR
>  #define HSTR_EL2HSTR
> +#define ICC_BPR1_EL1ICC_BPR1
> +#define ICC_CTLR_EL1ICC_CTLR
> +#define ICC_DIR_EL1 ICC_DIR
> +#define ICC_EOIR1_EL1   ICC_EOIR1
> +#define ICC_IGRPEN1_EL1 ICC_IGRPEN1
> +#define ICC_PMR_EL1 ICC_PMR
> +#define ICC_SGI1R_EL1   ICC_SGI1R
> +#define ICC_SRE_EL1 ICC_SRE
> +#define ICC_SRE_EL2 ICC_HSRE
>  #define ICH_AP0R0_EL2   ICH_AP0R0
>  #define ICH_AP0R1_EL2   ICH_AP0R1
>  #define ICH_AP0R2_EL2   ICH_AP0R2
> @@ -389,6 +414,10 @@
>  #define ICH_AP1R1_EL2   ICH_AP1R1
>  #define ICH_AP1R2_EL2   ICH_AP1R2
>  #define ICH_AP1R3_EL2   ICH_AP1R3
> +#define I

Re: [XEN v4 11/11] xen/Arm: GICv3: Enable GICv3 for AArch32

[Michal Orzel]

Hi Ayan,

On 28/11/2022 16:56, Ayan Kumar Halder wrote:
> One can now use GICv3 on AArch32 systems. However, ITS is not supported.
> The reason being currently we are trying to validate GICv3 on an AArch32_v8R
> system. Refer ARM DDI 0568A.c ID110520, B1.3.1,
> "A Generic Interrupt Controller (GIC) implemented with an Armv8-R PE must not
> implement LPI support."
> 
> By default GICv3 is disabled on AArch32 and enabled on AArch64.
> 
> Updated SUPPORT.md to state that GICv3 on Arm32 is not security supported.
> 
> Signed-off-by: Ayan Kumar Halder 
Reviewed-by: Michal Orzel 

~Michal

[PATCH] drivers/xen/hypervisor: Expose VM SIF flags to userspace

[Per Bilse]

/proc/xen is a legacy pseudo filesystem which predates Xen support
getting merged into Linux.  It has largely been replaced with more
normal locations for data (/sys/hypervisor/ for info, /dev/xen/ for
user devices).  We want to compile xenfs support out of the dom0 kernel.

There is one item which only exists in /proc/xen, namely
/proc/xen/capabilities with "control_d" being the signal of "you're in
the control domain".  This ultimately comes from the SIF flags provided
at VM start.

This patch exposes all SIF flags in /sys/hypervisor/properties/flags,
which will coexist with /proc/xen while dependencies are being migrated.
Possible values are "privileged", "initdomain", "multiboot",
"mod_start_pfn", and "virtmap", with "initdomain" being the equivalent
of "control_d".

Signed-off-by: Per Bilse 
---
 drivers/xen/sys-hypervisor.c | 26 ++
 include/xen/interface/xen.h  | 13 -
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/xen/sys-hypervisor.c b/drivers/xen/sys-hypervisor.c
index fcb0792f090e..7393e04bdb6d 100644
--- a/drivers/xen/sys-hypervisor.c
+++ b/drivers/xen/sys-hypervisor.c
@@ -379,6 +379,31 @@ static ssize_t buildid_show(struct hyp_sysfs_attr *attr, 
char *buffer)
 
 HYPERVISOR_ATTR_RO(buildid);
 
+static ssize_t flags_show(struct hyp_sysfs_attr *attr, char *buffer)
+{
+   static char const *const sifstr[SIFN_NUM_SIFN] = {
+   [SIFN_PRIV]  = "privileged",
+   [SIFN_INIT]  = "initdomain",
+   [SIFN_MULTI] = "multiboot",
+   [SIFN_PFN]   = "mod_start_pfn",
+   [SIFN_VIRT]  = "virtmap"
+   };
+   unsigned sifnum, sifmask;
+   ssize_t ret = 0;
+
+   sifmask = ~(~0U << SIFN_NUM_SIFN);  // ...111...
+   if (xen_domain() && (xen_start_flags & sifmask) != 0) {
+   for (sifnum = 0; sifnum != SIFN_NUM_SIFN; sifnum++) {
+   if ((xen_start_flags & (1

Re: AMD GPU problems under Xen

[Marek Marczykowski-Górecki]

On Tue, Nov 29, 2022 at 09:32:54AM -0500, Alex Deucher wrote:
> On Mon, Nov 28, 2022 at 8:59 PM Demi Marie Obenour
>  wrote:
> >
> > On Mon, Nov 28, 2022 at 11:18:00AM -0500, Alex Deucher wrote:
> > > On Mon, Nov 28, 2022 at 2:18 AM Demi Marie Obenour
> > >  wrote:
> > > >
> > > > Dear Christian:
> > > >
> > > > What is the status of the AMDGPU work for Xen dom0?  That was mentioned 
> > > > in
> > > > https://lore.kernel.org/dri-devel/b2dec9b3-03a7-e7ac-306e-1da024af8...@amd.com/
> > > > and there have been bug reports to Qubes OS about problems with AMDGPU
> > > > under Xen (such as https://github.com/QubesOS/qubes-issues/issues/7648).
> > >
> > > I would say it's a work in progress.  It depends what GPU  you have
> > > and what type of xen setup you are using (PV vs PVH, etc.).
> >
> > The current situation is:
> >
> > - dom0 is PV.
> > - VMs with assigned PCI devices are HVM and use a Linux-based stubdomain
> >   QEMU does not run in dom0.
> > - Everything else is PVH.
> >
> > In the future, I believe the goal is to move away from PV and HVM in
> > favor of PVH, though HVM support will remain for compatibility with
> > guests (such as Windows) that need emulated devices.
> >
> > > In general, your best bet currently is dGPU add in boards because they
> > > are largely self contained.
> >
> > The main problem is that for the trusted GUI to work, there needs to
> > be at least one GPU attached to a trusted VM, such as the host or a
> > dedicated GUI VM.  That VM will typically not be running graphics-
> > intensive workloads, so the compute power of a dGPU is largely wasted.
> > SR-IOV support would help with that, but the only GPU vendor with open
> > source SR-IOV support is Intel and it is still not upstream.  I am also
> > not certain if the support extends to Arc dGPUs.
> 
> Can you elaborate on this?  Why wouldn't you just want to pass-through
> a dGPU to a domU to use directly in the guest?

You can do that, but if that's your only GPU in the system, you'll lose
graphical interface for other guests.
But yes, simply pass-through of a dGPU is enough in some setups.

> Are you sure?  I didn't think intel's GVT solution was actually
> SR-IOV.  I think GVT is just a paravirtualized solution.

Yes, it's a paravirtualized solution, with device emulation done in dom0
kernel. This, besides being rather unusual approach in Xen world
(emulators, aka IOREQ servers usually live in userspace) puts rather
complex piece of code that interacts with untrusted data (instructions
from guests) in almost the most privileged system component, without
ability to sandbox it in any way. We consider it too risky for Qubes OS,
especially since the kernel patches were never accepted upstream and the
Xen support is not maintained anymore.

The SR-IOV approach Demi is talking about is newer development,
supported since Adler Lake (technically, IGD in Tiger Lake presents
SR-IOV capability too, but officially it's supported since ADL). The driver
for managing it is in the process of upstreaming. Some links here:
https://github.com/intel/linux-intel-lts/issues/33
(I have not tried it, yet)

>  That aside,
> we are working on enabling virtio gpu with our GPUs on xen in addition
> to domU passthrough.

That's interesting development. Please note, Linux recently (part of
6.1) gained support to use grant tables with virtio. This allows having
backends without full access to guest's memory. The work is done in
generic way, so a driver using proper APIs (including DMA) should work
out in such setup out of the box. Please try to not break it :)

> >
> > > APUs and platforms with integrated dGPUs
> > > are a bit more complicated as they tend to have more platform
> > > dependencies like ACPI tables and methods in order for the driver to
> > > be able to initialize the hardware properly.
> >
> > Is Xen dom0/domU support for such GPUs being worked on?  Is there an
> > estimate as to when the needed support will be available upstream?  This
> > is mostly directed at Christian and other people who work for hardware
> > vendors.
> 
> Yes, there are some minor fixes in the driver required which we'll be
> sending out soon and we had to add some ACPI tables to the whitelist
> in xen, but unfortunately the ACPI tables are AMD platform specific so
> there has been pushback from the xen maintainers on accepting them
> because they are not an official part of the ACPI spec.

Can the driver work without them? Such dependency, as you noted above,
make things rather complicated for pass-through (specific ACPI tables
can probably be made available to the guest, but usually guest wouldn't
see all the resources they talk about anyway).

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab


signature.asc
Description: PGP signature

Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses

[Ayan Kumar Halder]

On 29/11/2022 14:52, Julien Grall wrote:



On 29/11/2022 14:57, Ayan Kumar Halder wrote:

Hi All,


Hi,


Hi Julien,

Many thanks for your inputs.



I am trying to gather opinions on how to support 32 bit physical 
addresses to enable Xen running on R52.


Refer Cortex R52 TRM, Section 2.2.12 "Memory Model"

"...This is because the physical address is always the same as the 
virtual address...The virtual and physical address can be treated as 
synonyms for Cortex-R52."


Thus, I understand that R52 supports 32 bit physical address only. 
This is a bit different from Armv7 systems which supports Large 
Physical Address Extension (LPAE) ie 40 bit physical addresses. >

Please correct me if I misunderstand something. >
So currently, Xen supports 64 bit physical address for Arm_32 (ie 
Armv7) based system.


Xen supports *up to* 64-bit physical address. This may be lower in the 
HW (not all the Armv7 HW supports 40-bit address).



My aim is to enable support for 32 bit physical address.


Technically this is already supported because this is a subset of 
64-bit. I can see a use case (even on non R* HW) where you may want to 
use 32-bit paddr_t to reduce the code size (and registers used).


But I think that's more an optimization that rather than been necessary.


diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 6014c0f852..4f8b5fc4be 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -56,10 +56,10 @@ static bool __init 
device_tree_node_compatible(const void *fdt, int node,

  }

  void __init device_tree_get_reg(const __be32 **cell, u32 
address_cells,

-    u32 size_cells, u64 *start, u64 *size)
+    u32 size_cells, paddr_t *start, 
paddr_t *size)


This needs to stay uint64_t because the Device-Tree may contain 64-bit 
values and you...


Are you saying that the device tree may contain 64 bit addresses even 
though the platform is 32 bit ?


I think then this approach (ie "typedef u32 paddr_t" for 32 bit system) 
is incorrect.


Then, the other option would be to downcast 64 bit physical addresses to 
32 bits, when we need to translate pa to va.


Do you think this approach looks better ? Or any better suggestions ?

    xen/Arm: AArch32_V8R: Use 32 bits for the physical address

    In the case of MPU (unlike MMU), there is a 1-1 mapping of virtual 
address

    to physical address (ie VA = PA).

    However, the physical addresses defined for aarch32 is u64. This is 
a problem
    for aarch32 mpu systems as the physical addresses = virtual 
addresses = 32 bits.


    Thus, when the memory region for FDT is mapped, it returns the 
virtual address
    (which is the same as physical address but truncated upto the lower 
32 bits).
    Similar logic has been used to convert machine address to virtual 
address and

    for ioremap as well.

    We do not support physical addresses beyond 32 bits.
    As this logic will fail when FDT physical address is more than 32 
bits, we have

    added a BUG() to catch this.

    Thus, the following functions has been adapted :-
    early_fdt_map()
    copy_from_paddr()
    maddr_to_virt()
    ioremap_attr()

    Also disable "BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG)" as 
PADDR_BITS = 40

    and BITS_PER_LONG = 32.

    Signed-off-by: Ayan Kumar Halder 

diff --git a/xen/arch/arm/include/asm/mm_mpu.h 
b/xen/arch/arm/include/asm/mm_mpu.h

index 306a4c497c..f4f5ae1488 100644
--- a/xen/arch/arm/include/asm/mm_mpu.h
+++ b/xen/arch/arm/include/asm/mm_mpu.h
@@ -89,7 +89,18 @@ static inline paddr_t __virt_to_maddr(vaddr_t va)
 static inline void *maddr_to_virt(paddr_t ma)
 {
 /* In MPU system, VA == PA. */
+#ifdef CONFIG_AARCH32_V8R
+    /*
+ * 64 bit physical addresses are not supported.
+ * Raise a bug if one encounters 64 bit address.
+ */
+    if (ma >> BITOP_BITS_PER_WORD)
+    BUG();
+
+    return (void *) ((uint32_t)(ma & GENMASK(31,0)));
+#else
 return (void *)ma;
+#endif
 }

 /*
diff --git a/xen/arch/arm/include/asm/setup.h 
b/xen/arch/arm/include/asm/setup.h

index b3330cd584..3f4ac7f475 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -119,7 +119,11 @@ extern struct bootinfo bootinfo;

 extern domid_t max_init_domid;

+#ifdef CONFIG_AARCH32_V8R
+void copy_from_paddr(void *dst, uint32_t paddr, unsigned long len);
+#else
 void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
+#endif

 size_t estimate_efi_size(unsigned int mem_nr_banks);

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 04c05d7a05..7a7386f33a 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -46,7 +46,11 @@ struct minimal_dtb_header {
  * @paddr: source physical address
  * @len: length to copy
  */
+#ifdef CONFIG_AARCH32_V8R
+void __init copy_from_paddr(void *dst, uint32_t paddr, unsigned long len)
+#else
 void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
+#endif
 {
 void *

Re: AMD GPU problems under Xen

[Alex Deucher]

On Tue, Nov 29, 2022 at 10:15 AM Marek Marczykowski-Górecki
 wrote:
>
> On Tue, Nov 29, 2022 at 09:32:54AM -0500, Alex Deucher wrote:
> > On Mon, Nov 28, 2022 at 8:59 PM Demi Marie Obenour
> >  wrote:
> > >
> > > On Mon, Nov 28, 2022 at 11:18:00AM -0500, Alex Deucher wrote:
> > > > On Mon, Nov 28, 2022 at 2:18 AM Demi Marie Obenour
> > > >  wrote:
> > > > >
> > > > > Dear Christian:
> > > > >
> > > > > What is the status of the AMDGPU work for Xen dom0?  That was 
> > > > > mentioned in
> > > > > https://lore.kernel.org/dri-devel/b2dec9b3-03a7-e7ac-306e-1da024af8...@amd.com/
> > > > > and there have been bug reports to Qubes OS about problems with AMDGPU
> > > > > under Xen (such as 
> > > > > https://github.com/QubesOS/qubes-issues/issues/7648).
> > > >
> > > > I would say it's a work in progress.  It depends what GPU  you have
> > > > and what type of xen setup you are using (PV vs PVH, etc.).
> > >
> > > The current situation is:
> > >
> > > - dom0 is PV.
> > > - VMs with assigned PCI devices are HVM and use a Linux-based stubdomain
> > >   QEMU does not run in dom0.
> > > - Everything else is PVH.
> > >
> > > In the future, I believe the goal is to move away from PV and HVM in
> > > favor of PVH, though HVM support will remain for compatibility with
> > > guests (such as Windows) that need emulated devices.
> > >
> > > > In general, your best bet currently is dGPU add in boards because they
> > > > are largely self contained.
> > >
> > > The main problem is that for the trusted GUI to work, there needs to
> > > be at least one GPU attached to a trusted VM, such as the host or a
> > > dedicated GUI VM.  That VM will typically not be running graphics-
> > > intensive workloads, so the compute power of a dGPU is largely wasted.
> > > SR-IOV support would help with that, but the only GPU vendor with open
> > > source SR-IOV support is Intel and it is still not upstream.  I am also
> > > not certain if the support extends to Arc dGPUs.
> >
> > Can you elaborate on this?  Why wouldn't you just want to pass-through
> > a dGPU to a domU to use directly in the guest?
>
> You can do that, but if that's your only GPU in the system, you'll lose
> graphical interface for other guests.
> But yes, simply pass-through of a dGPU is enough in some setups.
>
> > Are you sure?  I didn't think intel's GVT solution was actually
> > SR-IOV.  I think GVT is just a paravirtualized solution.
>
> Yes, it's a paravirtualized solution, with device emulation done in dom0
> kernel. This, besides being rather unusual approach in Xen world
> (emulators, aka IOREQ servers usually live in userspace) puts rather
> complex piece of code that interacts with untrusted data (instructions
> from guests) in almost the most privileged system component, without
> ability to sandbox it in any way. We consider it too risky for Qubes OS,
> especially since the kernel patches were never accepted upstream and the
> Xen support is not maintained anymore.
>
> The SR-IOV approach Demi is talking about is newer development,
> supported since Adler Lake (technically, IGD in Tiger Lake presents
> SR-IOV capability too, but officially it's supported since ADL). The driver
> for managing it is in the process of upstreaming. Some links here:
> https://github.com/intel/linux-intel-lts/issues/33
> (I have not tried it, yet)
>
> >  That aside,
> > we are working on enabling virtio gpu with our GPUs on xen in addition
> > to domU passthrough.
>
> That's interesting development. Please note, Linux recently (part of
> 6.1) gained support to use grant tables with virtio. This allows having
> backends without full access to guest's memory. The work is done in
> generic way, so a driver using proper APIs (including DMA) should work
> out in such setup out of the box. Please try to not break it :)
>
> > >
> > > > APUs and platforms with integrated dGPUs
> > > > are a bit more complicated as they tend to have more platform
> > > > dependencies like ACPI tables and methods in order for the driver to
> > > > be able to initialize the hardware properly.
> > >
> > > Is Xen dom0/domU support for such GPUs being worked on?  Is there an
> > > estimate as to when the needed support will be available upstream?  This
> > > is mostly directed at Christian and other people who work for hardware
> > > vendors.
> >
> > Yes, there are some minor fixes in the driver required which we'll be
> > sending out soon and we had to add some ACPI tables to the whitelist
> > in xen, but unfortunately the ACPI tables are AMD platform specific so
> > there has been pushback from the xen maintainers on accepting them
> > because they are not an official part of the ACPI spec.
>
> Can the driver work without them? Such dependency, as you noted above,
> make things rather complicated for pass-through (specific ACPI tables
> can probably be made available to the guest, but usually guest wouldn't
> see all the resources they talk about anyway).

Not really for APUs and dGPUs that are integrated into a 

Re: [PATCH] drivers/xen/hypervisor: Expose VM SIF flags to userspace

[Juergen Gross]

On 29.11.22 16:00, Per Bilse wrote:

/proc/xen is a legacy pseudo filesystem which predates Xen support
getting merged into Linux.  It has largely been replaced with more
normal locations for data (/sys/hypervisor/ for info, /dev/xen/ for
user devices).  We want to compile xenfs support out of the dom0 kernel.

There is one item which only exists in /proc/xen, namely
/proc/xen/capabilities with "control_d" being the signal of "you're in
the control domain".  This ultimately comes from the SIF flags provided
at VM start.

This patch exposes all SIF flags in /sys/hypervisor/properties/flags,
which will coexist with /proc/xen while dependencies are being migrated.
Possible values are "privileged", "initdomain", "multiboot",
"mod_start_pfn", and "virtmap", with "initdomain" being the equivalent
of "control_d".

Signed-off-by: Per Bilse 
---
  drivers/xen/sys-hypervisor.c | 26 ++
  include/xen/interface/xen.h  | 13 -
  2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/xen/sys-hypervisor.c b/drivers/xen/sys-hypervisor.c
index fcb0792f090e..7393e04bdb6d 100644
--- a/drivers/xen/sys-hypervisor.c
+++ b/drivers/xen/sys-hypervisor.c
@@ -379,6 +379,31 @@ static ssize_t buildid_show(struct hyp_sysfs_attr *attr, 
char *buffer)
  
  HYPERVISOR_ATTR_RO(buildid);
  
+static ssize_t flags_show(struct hyp_sysfs_attr *attr, char *buffer)

+{
+   static char const *const sifstr[SIFN_NUM_SIFN] = {
+   [SIFN_PRIV]  = "privileged",
+   [SIFN_INIT]  = "initdomain",
+   [SIFN_MULTI] = "multiboot",
+   [SIFN_PFN]   = "mod_start_pfn",
+   [SIFN_VIRT]  = "virtmap"
+   };
+   unsigned sifnum, sifmask;
+   ssize_t ret = 0;
+
+   sifmask = ~(~0U << SIFN_NUM_SIFN);  // ...111...
+   if (xen_domain() && (xen_start_flags & sifmask) != 0) {
+   for (sifnum = 0; sifnum != SIFN_NUM_SIFN; sifnum++) {
+   if ((xen_start_flags & (1<  
diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h

index 0ca23eca2a9c..762a348abe3e 100644
--- a/include/xen/interface/xen.h
+++ b/include/xen/interface/xen.h
@@ -648,11 +648,14 @@ struct start_info {
  };
  
  /* These flags are passed in the 'flags' field of start_info_t. */

-#define SIF_PRIVILEGED  (1<<0)  /* Is the domain privileged? */
-#define SIF_INITDOMAIN  (1<<1)  /* Is this the initial control domain? */
-#define SIF_MULTIBOOT_MOD   (1<<2)  /* Is mod_start a multiboot module? */
-#define SIF_MOD_START_PFN   (1<<3)  /* Is mod_start a PFN? */
-#define SIF_VIRT_P2M_4TOOLS (1<<4)  /* Do Xen tools understand a virt. mapped 
*/
+/* Text strings are printed out in sys-hypervisor.c, we guard   */
+/* against mix-ups and errors by enumerating the flags. */
+enum { SIFN_PRIV, SIFN_INIT, SIFN_MULTI, SIFN_PFN, SIFN_VIRT, SIFN_NUM_SIFN };
+#define SIF_PRIVILEGED  (1<

Please don't change this header, as it is based on its master
located in the Xen repository.

An acceptable solution would be to send a Xen patch first doing the
xen.h changes, and when that patch has been taken to modify the
related Linux header accordingly.

In case you want to go that route, please add a "XEN_" prefix to the
enum members.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses

[Julien Grall]

On 29/11/2022 16:23, Ayan Kumar Halder wrote:


On 29/11/2022 14:52, Julien Grall wrote:



On 29/11/2022 14:57, Ayan Kumar Halder wrote:

Hi All,


Hi,


Hi Julien,

Many thanks for your inputs.



I am trying to gather opinions on how to support 32 bit physical 
addresses to enable Xen running on R52.


Refer Cortex R52 TRM, Section 2.2.12 "Memory Model"

"...This is because the physical address is always the same as the 
virtual address...The virtual and physical address can be treated as 
synonyms for Cortex-R52."


Thus, I understand that R52 supports 32 bit physical address only. 
This is a bit different from Armv7 systems which supports Large 
Physical Address Extension (LPAE) ie 40 bit physical addresses. >

Please correct me if I misunderstand something. >
So currently, Xen supports 64 bit physical address for Arm_32 (ie 
Armv7) based system.


Xen supports *up to* 64-bit physical address. This may be lower in the 
HW (not all the Armv7 HW supports 40-bit address).



My aim is to enable support for 32 bit physical address.


Technically this is already supported because this is a subset of 
64-bit. I can see a use case (even on non R* HW) where you may want to 
use 32-bit paddr_t to reduce the code size (and registers used).


But I think that's more an optimization that rather than been necessary.


diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 6014c0f852..4f8b5fc4be 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -56,10 +56,10 @@ static bool __init 
device_tree_node_compatible(const void *fdt, int node,

  }

  void __init device_tree_get_reg(const __be32 **cell, u32 
address_cells,

-    u32 size_cells, u64 *start, u64 *size)
+    u32 size_cells, paddr_t *start, 
paddr_t *size)


This needs to stay uint64_t because the Device-Tree may contain 64-bit 
values and you...


Are you saying that the device tree may contain 64 bit addresses even 
though the platform is 32 bit ?


There should not be any 32-bit address but you don't know what the 
device-tree is containing because this is user input.


This is not the business of the Device-Tree parser to decide whether the 
value should be downcasted or rejected. That's the goal of the callers.




I think then this approach (ie "typedef u32 paddr_t" for 32 bit system) 
is incorrect.
I am a bit surprised you came to this conclusion just based on the 
above. As I said before, there are benefits to allow Xen to be built 
with 32-bit (e.g. smaller code size and better use of the register).




Then, the other option would be to downcast 64 bit physical addresses to 
32 bits, when we need to translate pa to va.


Do you think this approach looks better ?


Some of the changes you propose are questionable (see below).


Or any better suggestions ?


Rework you previous approach by not touching the Device-Tree code.

diff --git a/xen/arch/arm/include/asm/mm_mpu.h 
b/xen/arch/arm/include/asm/mm_mpu.h

index 306a4c497c..f4f5ae1488 100644
--- a/xen/arch/arm/include/asm/mm_mpu.h
+++ b/xen/arch/arm/include/asm/mm_mpu.h
@@ -89,7 +89,18 @@ static inline paddr_t __virt_to_maddr(vaddr_t va)
  static inline void *maddr_to_virt(paddr_t ma)
  {
  /* In MPU system, VA == PA. */
+#ifdef CONFIG_AARCH32_V8R
+    /*
+ * 64 bit physical addresses are not supported.
+ * Raise a bug if one encounters 64 bit address.
+ */
+    if (ma >> BITOP_BITS_PER_WORD)
+    BUG();
I don't particularly like the runtime check when you should be able to 
sanitize the values before hand.



+
+    return (void *) ((uint32_t)(ma & GENMASK(31,0)));


& GENMASK (...) is a bit pointless here given that you above confirmed 
the top 32-bit are zeroed.



+#else
  return (void *)ma;
+#endif
  }

  /*
diff --git a/xen/arch/arm/include/asm/setup.h 
b/xen/arch/arm/include/asm/setup.h

index b3330cd584..3f4ac7f475 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -119,7 +119,11 @@ extern struct bootinfo bootinfo;

  extern domid_t max_init_domid;

+#ifdef CONFIG_AARCH32_V8R
+void copy_from_paddr(void *dst, uint32_t paddr, unsigned long len);
+#else
  void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
+#endif


I don't understand why the probably needs to be changed here...



  size_t estimate_efi_size(unsigned int mem_nr_banks);

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 04c05d7a05..7a7386f33a 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -46,7 +46,11 @@ struct minimal_dtb_header {
   * @paddr: source physical address
   * @len: length to copy
   */
+#ifdef CONFIG_AARCH32_V8R
+void __init copy_from_paddr(void *dst, uint32_t paddr, unsigned long len)
+#else
  void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
+#endif
  {
  void *src = (void *)(unsigned long)paddr;


... because the code should compile without any issue. If you were 
really concern about ignore

Re: [PATCH] Arm/P2M: reduce locking in p2m_{alloc,free}_page()

[Julien Grall]

Hi Jan,

On 29/11/2022 15:39, Jan Beulich wrote:

It is generally preferable to not hold locks around allocation
functions. And indeed in the hwdom case there's no point at all to hold
the paging lock. Reduce the locked regions to the non-hwdom case, while
at the same time arranging for p2m_alloc_page() to have just a single
return point.

Signed-off-by: Jan Beulich 


Reviewed-by: Julien Grall 

Cheers,



--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -48,7 +48,6 @@ static struct page_info *p2m_alloc_page(
  {
  struct page_info *pg;
  
-spin_lock(&d->arch.paging.lock);

  /*
   * For hardware domain, there should be no limit in the number of pages 
that
   * can be allocated, so that the kernel may take advantage of the extended
@@ -58,34 +57,28 @@ static struct page_info *p2m_alloc_page(
  {
  pg = alloc_domheap_page(NULL, 0);
  if ( pg == NULL )
-{
  printk(XENLOG_G_ERR "Failed to allocate P2M pages for hwdom.\n");
-spin_unlock(&d->arch.paging.lock);
-return NULL;
-}
  }
  else
  {
+spin_lock(&d->arch.paging.lock);
  pg = page_list_remove_head(&d->arch.paging.p2m_freelist);
-if ( unlikely(!pg) )
-{
-spin_unlock(&d->arch.paging.lock);
-return NULL;
-}
+spin_unlock(&d->arch.paging.lock);
  }
-spin_unlock(&d->arch.paging.lock);
  
  return pg;

  }
  
  static void p2m_free_page(struct domain *d, struct page_info *pg)

  {
-spin_lock(&d->arch.paging.lock);
  if ( is_hardware_domain(d) )
  free_domheap_page(pg);
  else
+{
+spin_lock(&d->arch.paging.lock);
  page_list_add_tail(pg, &d->arch.paging.p2m_freelist);
-spin_unlock(&d->arch.paging.lock);
+spin_unlock(&d->arch.paging.lock);
+}
  }
  
  /* Return the size of the pool, in bytes. */


--
Julien Grall

Re: [PATCH] Arm64: make setup_virt_paging()'s pa_range_info[] static

[Julien Grall]

Hi Jan,

On 29/11/2022 15:39, Jan Beulich wrote:

While not as inefficient as it would be on x86 (due to suitable constant
loading and register pair storing instructions being available to fill
some of the fields), having the compiler construct an array of constants
on the stack still looks odd to me.


The function is only called once at boot. So this seems more a 
micro-optimization than anything else.




Signed-off-by: Jan Beulich 


Acked-by: Julien Grall 


---
Actual space savings could be had if further converting the field types
to e.g. unsigned char (all of the values fit in that type).


This is a micro-optimization. If you want to send it then I will review it.



--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -2281,12 +2281,12 @@ void __init setup_virt_paging(void)
  val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
  val |= VTCR_SL0(0x1); /* P2M starts at first level */
  #else /* CONFIG_ARM_64 */
-const struct {
+static const struct {
  unsigned int pabits; /* Physical Address Size */
  unsigned int t0sz;   /* Desired T0SZ, minimum in comment */
  unsigned int root_order; /* Page order of the root of the p2m */
  unsigned int sl0;/* Desired SL0, maximum in comment */
-} pa_range_info[] = {
+} pa_range_info[] __initconst = {
  /* T0SZ minimum and SL0 maximum from ARM DDI 0487H.a Table D5-6 */
  /*  PA size, t0sz(min), root-order, sl0(max) */
  [0] = { 32,  32/*32*/,  0,  1 },


--
Julien Grall

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Roger Pau Monné]

Ping?

So far I've got some feedback from Jan which I've replied to, but I
haven't got any more feedback.

Both patches 1 and 2 are required in order for Xen dom0s to properly
handle ACPI Processor related data to the hypervisor. Patch 3 can be
deal with later.

Thanks, Roger.

On Mon, Nov 21, 2022 at 11:21:10AM +0100, Roger Pau Monne wrote:
> When running as a Xen dom0 the number of CPUs available to Linux can
> be different from the number of CPUs present on the system, but in
> order to properly fetch processor performance related data _PDC must
> be executed on all the physical CPUs online on the system.
> 
> The current checks in processor_physically_present() result in some
> processor objects not getting their _PDC methods evaluated when Linux
> is running as Xen dom0.  Fix this by introducing a custom function to
> use when running as Xen dom0 in order to check whether a processor
> object matches a CPU that's online.
> 
> Fixes: 5d554a7bb064 ('ACPI: processor: add internal 
> processor_physically_present()')
> Signed-off-by: Roger Pau Monné 
> ---
>  arch/x86/include/asm/xen/hypervisor.h | 10 ++
>  arch/x86/xen/enlighten.c  | 27 +++
>  drivers/acpi/processor_pdc.c  | 11 +++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/arch/x86/include/asm/xen/hypervisor.h 
> b/arch/x86/include/asm/xen/hypervisor.h
> index 16f548a661cf..b9f512138043 100644
> --- a/arch/x86/include/asm/xen/hypervisor.h
> +++ b/arch/x86/include/asm/xen/hypervisor.h
> @@ -61,4 +61,14 @@ void __init xen_pvh_init(struct boot_params *boot_params);
>  void __init mem_map_via_hcall(struct boot_params *boot_params_p);
>  #endif
>  
> +#ifdef CONFIG_XEN_DOM0
> +bool __init xen_processor_present(uint32_t acpi_id);
> +#else
> +static inline bool xen_processor_present(uint32_t acpi_id)
> +{
> + BUG();
> + return false;
> +}
> +#endif
> +
>  #endif /* _ASM_X86_XEN_HYPERVISOR_H */
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index b8db2148c07d..d4c44361a26c 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -346,3 +346,30 @@ void xen_arch_unregister_cpu(int num)
>  }
>  EXPORT_SYMBOL(xen_arch_unregister_cpu);
>  #endif
> +
> +#ifdef CONFIG_XEN_DOM0
> +bool __init xen_processor_present(uint32_t acpi_id)
> +{
> + unsigned int i, maxid;
> + struct xen_platform_op op = {
> + .cmd = XENPF_get_cpuinfo,
> + .interface_version = XENPF_INTERFACE_VERSION,
> + };
> + int ret = HYPERVISOR_platform_op(&op);
> +
> + if (ret)
> + return false;
> +
> + maxid = op.u.pcpu_info.max_present;
> + for (i = 0; i <= maxid; i++) {
> + op.u.pcpu_info.xen_cpuid = i;
> + ret = HYPERVISOR_platform_op(&op);
> + if (ret)
> + continue;
> + if (op.u.pcpu_info.acpi_id == acpi_id)
> + return op.u.pcpu_info.flags & XEN_PCPU_FLAGS_ONLINE;
> + }
> +
> + return false;
> +}
> +#endif
> diff --git a/drivers/acpi/processor_pdc.c b/drivers/acpi/processor_pdc.c
> index 8c3f82c9fff3..18fb04523f93 100644
> --- a/drivers/acpi/processor_pdc.c
> +++ b/drivers/acpi/processor_pdc.c
> @@ -14,6 +14,8 @@
>  #include 
>  #include 
>  
> +#include 
> +
>  #include "internal.h"
>  
>  static bool __init processor_physically_present(acpi_handle handle)
> @@ -47,6 +49,15 @@ static bool __init 
> processor_physically_present(acpi_handle handle)
>   return false;
>   }
>  
> + if (xen_initial_domain())
> + /*
> +  * When running as a Xen dom0 the number of processors Linux
> +  * sees can be different from the real number of processors on
> +  * the system, and we still need to execute _PDC for all of
> +  * them.
> +  */
> + return xen_processor_present(acpi_id);
> +
>   type = (acpi_type == ACPI_TYPE_DEVICE) ? 1 : 0;
>   cpuid = acpi_get_cpuid(handle, type, acpi_id);
>  
> -- 
> 2.37.3
> 

Re: [PATCH] x86/APIC: make a few interrupt handler functions static

[Roger Pau Monné]

On Tue, Nov 29, 2022 at 03:46:30PM +0100, Jan Beulich wrote:
> Four of them are used in apic.c only and hence better wouldn't be
> exposed to other CUs. To avoid the need for forward declarations, move
> apic_intr_init() past the four handlers.
> 
> Signed-off-by: Jan Beulich 

Acked-by: Roger Pau Monné 

A nit below.

> 
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -127,21 +127,6 @@ void ack_bad_irq(unsigned int irq)
>  ack_APIC_irq();
>  }
>  
> -void __init apic_intr_init(void)
> -{
> -smp_intr_init();
> -
> -/* self generated IPI for local APIC timer */
> -set_direct_apic_vector(LOCAL_TIMER_VECTOR, apic_timer_interrupt);
> -
> -/* IPI vectors for APIC spurious and error interrupts */
> -set_direct_apic_vector(SPURIOUS_APIC_VECTOR, spurious_interrupt);
> -set_direct_apic_vector(ERROR_APIC_VECTOR, error_interrupt);
> -
> -/* Performance Counters Interrupt */
> -set_direct_apic_vector(PMU_APIC_VECTOR, pmu_apic_interrupt);
> -}
> -
>  /* Using APIC to generate smp_local_timer_interrupt? */
>  static bool __read_mostly using_apic_timer;
>  
> @@ -1363,7 +1348,7 @@ int reprogram_timer(s_time_t timeout)
>  return apic_tmict || !timeout;
>  }
>  
> -void cf_check apic_timer_interrupt(struct cpu_user_regs *regs)
> +static void cf_check apic_timer_interrupt(struct cpu_user_regs *regs)

Given that the function is now not exported out of apic.c, wouldn't it
be better to drop the apic_ prefix?

The same would likely apply to pmu_apic_interrupt() then.

Thanks, Roger.

Re: [PATCH] x86/APIC: make a few interrupt handler functions static

[Andrew Cooper]

On 29/11/2022 16:05, Roger Pau Monné wrote:
> On Tue, Nov 29, 2022 at 03:46:30PM +0100, Jan Beulich wrote:
>> Four of them are used in apic.c only and hence better wouldn't be
>> exposed to other CUs. To avoid the need for forward declarations, move
>> apic_intr_init() past the four handlers.
>>
>> Signed-off-by: Jan Beulich 
> Acked-by: Roger Pau Monné 
>
> A nit below.
>
>> --- a/xen/arch/x86/apic.c
>> +++ b/xen/arch/x86/apic.c
>> @@ -127,21 +127,6 @@ void ack_bad_irq(unsigned int irq)
>>  ack_APIC_irq();
>>  }
>>  
>> -void __init apic_intr_init(void)
>> -{
>> -smp_intr_init();
>> -
>> -/* self generated IPI for local APIC timer */
>> -set_direct_apic_vector(LOCAL_TIMER_VECTOR, apic_timer_interrupt);
>> -
>> -/* IPI vectors for APIC spurious and error interrupts */
>> -set_direct_apic_vector(SPURIOUS_APIC_VECTOR, spurious_interrupt);
>> -set_direct_apic_vector(ERROR_APIC_VECTOR, error_interrupt);
>> -
>> -/* Performance Counters Interrupt */
>> -set_direct_apic_vector(PMU_APIC_VECTOR, pmu_apic_interrupt);
>> -}
>> -
>>  /* Using APIC to generate smp_local_timer_interrupt? */
>>  static bool __read_mostly using_apic_timer;
>>  
>> @@ -1363,7 +1348,7 @@ int reprogram_timer(s_time_t timeout)
>>  return apic_tmict || !timeout;
>>  }
>>  
>> -void cf_check apic_timer_interrupt(struct cpu_user_regs *regs)
>> +static void cf_check apic_timer_interrupt(struct cpu_user_regs *regs)
> Given that the function is now not exported out of apic.c, wouldn't it
> be better to drop the apic_ prefix?

This is the handler for the apic timer, as opposed to the plethora of
other timer interrupts we have elsewhere.

Simply "timer interrupt" is too generic a name.

> The same would likely apply to pmu_apic_interrupt() then.

This one could lose the infix.  All PMU interrupts are from an LVT
vector.  It may have made a different back in the days of non-integrated
APICs, but those days are long gone.

~Andrew

Re: [PATCH] drivers/xen/hypervisor: Expose VM SIF flags to userspace

[Boris Ostrovsky]

On 11/29/22 10:00 AM, Per Bilse wrote:
  
+static ssize_t flags_show(struct hyp_sysfs_attr *attr, char *buffer)

+{
+   static char const *const sifstr[SIFN_NUM_SIFN] = {
+   [SIFN_PRIV]  = "privileged",
+   [SIFN_INIT]  = "initdomain",
+   [SIFN_MULTI] = "multiboot",
+   [SIFN_PFN]   = "mod_start_pfn",
+   [SIFN_VIRT]  = "virtmap"
+   };
+   unsigned sifnum, sifmask;
+   ssize_t ret = 0;
+
+   sifmask = ~(~0U << SIFN_NUM_SIFN);  // ...111...
+   if (xen_domain() && (xen_start_flags & sifmask) != 0) {
+   for (sifnum = 0; sifnum != SIFN_NUM_SIFN; sifnum++) {
+   if ((xen_start_flags & (1<


Why not simply show unprocessed xen_start_flags as a hex value?


-boris

Re: [PATCH] drivers/xen/hypervisor: Expose VM SIF flags to userspace

[Andrew Cooper]

On 29/11/2022 15:27, Juergen Gross wrote:
> On 29.11.22 16:00, Per Bilse wrote:
>> /proc/xen is a legacy pseudo filesystem which predates Xen support
>> getting merged into Linux.  It has largely been replaced with more
>> normal locations for data (/sys/hypervisor/ for info, /dev/xen/ for
>> user devices).  We want to compile xenfs support out of the dom0 kernel.
>>
>> There is one item which only exists in /proc/xen, namely
>> /proc/xen/capabilities with "control_d" being the signal of "you're in
>> the control domain".  This ultimately comes from the SIF flags provided
>> at VM start.
>>
>> This patch exposes all SIF flags in /sys/hypervisor/properties/flags,
>> which will coexist with /proc/xen while dependencies are being migrated.
>> Possible values are "privileged", "initdomain", "multiboot",
>> "mod_start_pfn", and "virtmap", with "initdomain" being the equivalent
>> of "control_d".
>>
>> Signed-off-by: Per Bilse 
>> ---
>>   drivers/xen/sys-hypervisor.c | 26 ++
>>   include/xen/interface/xen.h  | 13 -
>>   2 files changed, 34 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/xen/sys-hypervisor.c b/drivers/xen/sys-hypervisor.c
>> index fcb0792f090e..7393e04bdb6d 100644
>> --- a/drivers/xen/sys-hypervisor.c
>> +++ b/drivers/xen/sys-hypervisor.c
>> @@ -379,6 +379,31 @@ static ssize_t buildid_show(struct
>> hyp_sysfs_attr *attr, char *buffer)
>>     HYPERVISOR_ATTR_RO(buildid);
>>   +static ssize_t flags_show(struct hyp_sysfs_attr *attr, char *buffer)
>> +{
>> +    static char const *const sifstr[SIFN_NUM_SIFN] = {
>> +    [SIFN_PRIV]  = "privileged",
>> +    [SIFN_INIT]  = "initdomain",
>> +    [SIFN_MULTI] = "multiboot",
>> +    [SIFN_PFN]   = "mod_start_pfn",
>> +    [SIFN_VIRT]  = "virtmap"
>> +    };
>> +    unsigned sifnum, sifmask;
>> +    ssize_t ret = 0;
>> +
>> +    sifmask = ~(~0U << SIFN_NUM_SIFN);  // ...111...
>> +    if (xen_domain() && (xen_start_flags & sifmask) != 0) {
>> +    for (sifnum = 0; sifnum != SIFN_NUM_SIFN; sifnum++) {
>> +    if ((xen_start_flags & (1<> +    ret += sprintf(buffer+ret, "%s ", sifstr[sifnum]);
>> +    }
>> +    buffer[ret-1] = '\n';
>> +    }
>> +    return ret;
>> +}
>> +
>> +HYPERVISOR_ATTR_RO(flags);
>> +
>>   static struct attribute *xen_properties_attrs[] = {
>>   &capabilities_attr.attr,
>>   &changeset_attr.attr,
>> @@ -386,6 +411,7 @@ static struct attribute *xen_properties_attrs[] = {
>>   &pagesize_attr.attr,
>>   &features_attr.attr,
>>   &buildid_attr.attr,
>> +    &flags_attr.attr,
>>   NULL
>>   };
>>   diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
>> index 0ca23eca2a9c..762a348abe3e 100644
>> --- a/include/xen/interface/xen.h
>> +++ b/include/xen/interface/xen.h
>> @@ -648,11 +648,14 @@ struct start_info {
>>   };
>>     /* These flags are passed in the 'flags' field of start_info_t. */
>> -#define SIF_PRIVILEGED  (1<<0)  /* Is the domain privileged? */
>> -#define SIF_INITDOMAIN  (1<<1)  /* Is this the initial control
>> domain? */
>> -#define SIF_MULTIBOOT_MOD   (1<<2)  /* Is mod_start a multiboot
>> module? */
>> -#define SIF_MOD_START_PFN   (1<<3)  /* Is mod_start a PFN? */
>> -#define SIF_VIRT_P2M_4TOOLS (1<<4)  /* Do Xen tools understand a
>> virt. mapped */
>> +/* Text strings are printed out in sys-hypervisor.c, we guard   */
>> +/* against mix-ups and errors by enumerating the flags. */
>> +enum { SIFN_PRIV, SIFN_INIT, SIFN_MULTI, SIFN_PFN, SIFN_VIRT,
>> SIFN_NUM_SIFN };
>> +#define SIF_PRIVILEGED  (1<> privileged? */
>> +#define SIF_INITDOMAIN  (1<> control domain? */
>> +#define SIF_MULTIBOOT_MOD   (1<> multiboot module? */
>> +#define SIF_MOD_START_PFN   (1<> +#define SIF_VIRT_P2M_4TOOLS (1<> understand a virt. mapped */
>
> Please don't change this header, as it is based on its master
> located in the Xen repository.
>
> An acceptable solution would be to send a Xen patch first doing the
> xen.h changes, and when that patch has been taken to modify the
> related Linux header accordingly.
>
> In case you want to go that route, please add a "XEN_" prefix to the
> enum members.

You can't use enums in the public headers because they have
implementation defined behaviour (including the constants themselves). 
Also, you cant just rename them to XEN because the API is stable.

For Linux, just use ilog2() when constructing the table, and don't
modify the header at all.


As for the actual flags exposed, it would be very beneficial not to copy
the exist proc interface.  It would be better to expose a subdir that
had files containing booleans, because that also gives userspace an easy
way to figure out if the particular flag is known to Linux,
independently of whether the flag is set for a specific VM.

Also, I think you only care about exposing PRIV and INITDOM. 
MULTIBOOT_MOD, START_PFN and VIRT_P2M_4TOOLS are all details specific to
the kernel itself, and not relevan

Re: [PATCH] x86/p2m: don't calculate page owner twice in p2m_add_page()

[Roger Pau Monné]

On Tue, Nov 29, 2022 at 03:47:53PM +0100, Jan Beulich wrote:
> Neither page_get_owner() nor mfn_to_page() are entirely trivial
> operations - don't do the same thing twice in close succession. Instead
> help CSE (when MEM_SHARING=y) by introducing a local variable holding
> the page owner.
> 
> Signed-off-by: Jan Beulich 

Acked-by: Roger Pau Monné 

> ---
> According to my observations gcc12 manages to CSE mfn_to_page() but not
> (all of) page_get_owner(). The overall savings there are, partly due to
> knock-on effects, 64 bytes of code.
> 
> While looking there, "mfn_eq(omfn, mfn_add(mfn, i))" near the end of the
> same loop caught my eye: Is that really correct? Shouldn't we fail the
> operation if the MFN which "ogfn" was derived from doesn't match the MFN
> "ogfn" maps to?

Getting into that state possibly means something has gone wrong if we
have rules out grants and foreign maps?

So it should be:

if ( !mfn_eq(omfn, mfn_add(mfn, i)) )
{
/* Something has gone wrong, ASSERT_UNREACHABLE()? */
goto out;
}
rc = p2m_remove_entry(p2m, ogfn, omfn, 0)
if ( rc )
goto out;

but maybe I'm missing the point of the check there, I have to admit I
sometimes find the p2m code difficult to follow.

Thanks, Roger.

Re: [PATCH] drivers/xen/hypervisor: Expose VM SIF flags to userspace

[Per Bilse (3P)]

On 29/11/2022 16:29, Boris Ostrovsky wrote:
> Why not simply show unprocessed xen_start_flags as a hex value?

Providing a text representation follows what is currently the case:

[root@lcy2-dt128 /proc/xen]# cat capabilities
control_d
[root@lcy2-dt128 /proc/xen]#

The migrated form would yield:

root@dt90:/sys/hypervisor/properties# cat flags
privileged initdomain mod_start_pfn
root@dt90:/sys/hypervisor/properties#

There is good precedence for being more helpful than printing hex 
values, but I have no objections if there is consensus that a hex value 
is better.

Best,

   -- Per

Re: [PATCH] drivers/xen/hypervisor: Expose VM SIF flags to userspace

[Per Bilse (3P)]

On 29/11/2022 16:41, Andrew Cooper wrote:
> As for the actual flags exposed, it would be very beneficial not to copy
> the exist proc interface.  It would be better to expose a subdir that
> had files containing booleans, because that also gives userspace an easy
> way to figure out if the particular flag is known to Linux,
> independently of whether the flag is set for a specific VM.

OK, I'll do that instead; I thought it was the single "control_d" that 
shouldn't be perpetuated.

Best,

   -- Per

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Dave Hansen]

On 11/21/22 02:21, Roger Pau Monne wrote:
> When running as a Xen dom0 the number of CPUs available to Linux can
> be different from the number of CPUs present on the system, but in
> order to properly fetch processor performance related data _PDC must
> be executed on all the physical CPUs online on the system.

How is the number of CPUs available to Linux different?

Is this a result of the ACPI tables that dom0 sees being "wrong"?

> The current checks in processor_physically_present() result in some
> processor objects not getting their _PDC methods evaluated when Linux
> is running as Xen dom0.  Fix this by introducing a custom function to
> use when running as Xen dom0 in order to check whether a processor
> object matches a CPU that's online.

What is the end user visible effect of this problem and of the solution?

Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses

[Ayan Kumar Halder]

On 29/11/2022 15:52, Julien Grall wrote:



On 29/11/2022 16:23, Ayan Kumar Halder wrote:


On 29/11/2022 14:52, Julien Grall wrote:



On 29/11/2022 14:57, Ayan Kumar Halder wrote:

Hi All,


Hi,

Hi Julien,


Hi Julien,

Many thanks for your inputs.



I am trying to gather opinions on how to support 32 bit physical 
addresses to enable Xen running on R52.


Refer Cortex R52 TRM, Section 2.2.12 "Memory Model"

"...This is because the physical address is always the same as the 
virtual address...The virtual and physical address can be treated 
as synonyms for Cortex-R52."


Thus, I understand that R52 supports 32 bit physical address only. 
This is a bit different from Armv7 systems which supports Large 
Physical Address Extension (LPAE) ie 40 bit physical addresses. >

Please correct me if I misunderstand something. >
So currently, Xen supports 64 bit physical address for Arm_32 (ie 
Armv7) based system.


Xen supports *up to* 64-bit physical address. This may be lower in 
the HW (not all the Armv7 HW supports 40-bit address).



My aim is to enable support for 32 bit physical address.


Technically this is already supported because this is a subset of 
64-bit. I can see a use case (even on non R* HW) where you may want 
to use 32-bit paddr_t to reduce the code size (and registers used).


But I think that's more an optimization that rather than been 
necessary.



diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 6014c0f852..4f8b5fc4be 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -56,10 +56,10 @@ static bool __init 
device_tree_node_compatible(const void *fdt, int node,

  }

  void __init device_tree_get_reg(const __be32 **cell, u32 
address_cells,
-    u32 size_cells, u64 *start, u64 
*size)
+    u32 size_cells, paddr_t *start, 
paddr_t *size)


This needs to stay uint64_t because the Device-Tree may contain 
64-bit values and you...


Are you saying that the device tree may contain 64 bit addresses even 
though the platform is 32 bit ?


There should not be any 32-bit address but you don't know what the 
device-tree is containing because this is user input.


This is not the business of the Device-Tree parser to decide whether 
the value should be downcasted or rejected. That's the goal of the 
callers.




I think then this approach (ie "typedef u32 paddr_t" for 32 bit 
system) is incorrect.
I am a bit surprised you came to this conclusion just based on the 
above. As I said before, there are benefits to allow Xen to be built 
with 32-bit (e.g. smaller code size and better use of the register).




Then, the other option would be to downcast 64 bit physical addresses 
to 32 bits, when we need to translate pa to va.


Do you think this approach looks better ?


Some of the changes you propose are questionable (see below).


Or any better suggestions ?


Rework you previous approach by not touching the Device-Tree code.

diff --git a/xen/arch/arm/include/asm/mm_mpu.h 
b/xen/arch/arm/include/asm/mm_mpu.h

index 306a4c497c..f4f5ae1488 100644
--- a/xen/arch/arm/include/asm/mm_mpu.h
+++ b/xen/arch/arm/include/asm/mm_mpu.h
@@ -89,7 +89,18 @@ static inline paddr_t __virt_to_maddr(vaddr_t va)
  static inline void *maddr_to_virt(paddr_t ma)
  {
  /* In MPU system, VA == PA. */
+#ifdef CONFIG_AARCH32_V8R
+    /*
+ * 64 bit physical addresses are not supported.
+ * Raise a bug if one encounters 64 bit address.
+ */
+    if (ma >> BITOP_BITS_PER_WORD)
+    BUG();
I don't particularly like the runtime check when you should be able to 
sanitize the values before hand.


I think we can replace BUG() with ASSERT(). This is similar to what is 
being done today.





+
+    return (void *) ((uint32_t)(ma & GENMASK(31,0)));


& GENMASK (...) is a bit pointless here given that you above confirmed 
the top 32-bit are zeroed.



+#else
  return (void *)ma;
+#endif
  }

  /*
diff --git a/xen/arch/arm/include/asm/setup.h 
b/xen/arch/arm/include/asm/setup.h

index b3330cd584..3f4ac7f475 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -119,7 +119,11 @@ extern struct bootinfo bootinfo;

  extern domid_t max_init_domid;

+#ifdef CONFIG_AARCH32_V8R
+void copy_from_paddr(void *dst, uint32_t paddr, unsigned long len);
+#else
  void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
+#endif


I don't understand why the probably needs to be changed here...



  size_t estimate_efi_size(unsigned int mem_nr_banks);

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 04c05d7a05..7a7386f33a 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -46,7 +46,11 @@ struct minimal_dtb_header {
   * @paddr: source physical address
   * @len: length to copy
   */
+#ifdef CONFIG_AARCH32_V8R
+void __init copy_from_paddr(void *dst, uint32_t paddr, unsigned long 
len)

+#else
  void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long 
len)

+#e

Re: objtool warning for next-20221118

[Paul E. McKenney]

On Fri, Nov 25, 2022 at 06:30:35AM +0100, Juergen Gross wrote:
> On 24.11.22 17:39, Josh Poimboeuf wrote:
> > On Thu, Nov 24, 2022 at 08:47:47AM +0100, Juergen Gross wrote:
> > > > > +++ b/arch/x86/xen/smp_pv.c
> > > > > @@ -385,17 +385,9 @@ static void xen_pv_play_dead(void) /* used only
> > > > > with HOTPLUG_CPU */
> > > > >    {
> > > > >    play_dead_common();
> > > > >    HYPERVISOR_vcpu_op(VCPUOP_down, 
> > > > > xen_vcpu_nr(smp_processor_id()), NULL);
> > > > > -    cpu_bringup();
> > > > > -    /*
> > > > > - * commit 4b0c0f294 (tick: Cleanup NOHZ per cpu data on cpu down)
> > > > > - * clears certain data that the cpu_idle loop (which called us
> > > > > - * and that we return from) expects. The only way to get that
> > > > > - * data back is to call:
> > > > > - */
> > > > > -    tick_nohz_idle_enter();
> > > > > -    tick_nohz_idle_stop_tick_protected();
> > > > > -    cpuhp_online_idle(CPUHP_AP_ONLINE_IDLE);
> > > > > +    /* FIXME: converge cpu_bringup_and_idle() and start_secondary() 
> > > > > */
> > > > > +    cpu_bringup_and_idle();
> > > > 
> > > > I think this will leak stack memory. Multiple cpu offline/online cycles 
> > > > of
> > > > the same cpu will finally exhaust the idle stack.
> > 
> > Doh!  Of course...
> > 
> > I was actually thinking ahead, to where eventually xen_pv_play_dead()
> > can call start_cpu0(), which can be changed to automatically reset the
> > stack pointer like this:
> > 
> > SYM_CODE_START(start_cpu0)
> > ANNOTATE_NOENDBR
> > UNWIND_HINT_EMPTY
> > movqPER_CPU_VAR(pcpu_hot + X86_top_of_stack), %rax
> > leaq-PTREGS_SIZE(%rax), %rsp
> > jmp .Ljump_to_C_code
> > SYM_CODE_END(start_cpu0)
> > 
> > but that would only be possible be after more cleanups which converge
> > cpu_bringup_and_idle() with start_secondary().
> > 
> > > The attached patch seems to work fine.
> > 
> > The patch looks good to me.
> > 
> > It doesn't solve Paul's original issue where arch_cpu_idle_dead() needs
> > to be __noreturn.  But that should probably be a separate patch anyway.
> 
> Okay, I'll split this off.
> 
> > 
> > > The __noreturn annotation seems to trigger an objtool warning, though, in
> > > spite of the added BUG() at the end of xen_pv_play_dead():
> > > 
> > > arch/x86/xen/smp_pv.o: warning: objtool: xen_pv_play_dead() falls through 
> > > to
> > > next function xen_pv_cpu_die()
> > 
> > You'll need to tell objtool that xen_cpu_bringup_again() is noreturn by
> > adding "xen_cpu_bringup_again" to global_noreturns[] in
> > tools/objtool/check.c.
> 
> Ah, okay. Will do that.
> 
> > (Yes it's a pain, I'll be working an improved solution to the noreturn
> > thing...)
> 
> Should be fairly easy, no?
> 
> "Just" extend the __noreturn macro to put the function into a ".text.noreturn"
> section, which can be handled in a special way by objtool. This would need
> an __init_noreturn macro, of course, for a ".init.text.noreturn" section.

And in last night's -next run, that diagnostic was gone!

But of course another appeared in its place:

drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: 
check_relocations+0xd1: stack state mismatch: cfa1=4+32 cfa2=-1+0

The .config file is shown below.  Thoughts?

Thanx, Paul



#
# Automatically generated file; DO NOT EDIT.
# Linux/x86 6.1.0-rc1 Kernel Configuration
#
CONFIG_CC_VERSION_TEXT="Ubuntu clang version 
11.1.0-++20211011094159+1fdec59bffc1-1~exp1~20211011214622.5"
CONFIG_GCC_VERSION=0
CONFIG_CC_IS_CLANG=y
CONFIG_CLANG_VERSION=110100
CONFIG_AS_IS_LLVM=y
CONFIG_AS_VERSION=110100
CONFIG_LD_IS_BFD=y
CONFIG_LD_VERSION=23400
CONFIG_LLD_VERSION=0
CONFIG_CC_CAN_LINK=y
CONFIG_CC_CAN_LINK_STATIC=y
CONFIG_CC_HAS_ASM_GOTO_OUTPUT=y
CONFIG_CC_HAS_ASM_INLINE=y
CONFIG_PAHOLE_VERSION=0
CONFIG_IRQ_WORK=y
CONFIG_BUILDTIME_TABLE_SORT=y
CONFIG_THREAD_INFO_IN_TASK=y

#
# General setup
#
CONFIG_BROKEN_ON_SMP=y
CONFIG_INIT_ENV_ARG_LIMIT=32
# CONFIG_COMPILE_TEST is not set
CONFIG_WERROR=y
CONFIG_LOCALVERSION=""
CONFIG_LOCALVERSION_AUTO=y
CONFIG_BUILD_SALT=""
CONFIG_HAVE_KERNEL_GZIP=y
CONFIG_HAVE_KERNEL_BZIP2=y
CONFIG_HAVE_KERNEL_LZMA=y
CONFIG_HAVE_KERNEL_XZ=y
CONFIG_HAVE_KERNEL_LZO=y
CONFIG_HAVE_KERNEL_LZ4=y
CONFIG_HAVE_KERNEL_ZSTD=y
CONFIG_KERNEL_GZIP=y
# CONFIG_KERNEL_BZIP2 is not set
# CONFIG_KERNEL_LZMA is not set
# CONFIG_KERNEL_XZ is not set
# CONFIG_KERNEL_LZO is not set
# CONFIG_KERNEL_LZ4 is not set
# CONFIG_KERNEL_ZSTD is not set
CONFIG_DEFAULT_INIT=""
CONFIG_DEFAULT_HOSTNAME="(none)"
CONFIG_SYSVIPC=y
CONFIG_SYSVIPC_SYSCTL=y
CONFIG_SYSVIPC_COMPAT=y
CONFIG_POSIX_MQUEUE=y
CONFIG_POSIX_MQUEUE_SYSCTL=y
# CONFIG_WATCH_QUEUE is not set
CONFIG_CROSS_MEMORY_ATTACH=y
# CONFIG_USELIB is not set
CONFIG_AUDIT=y
CONFIG_HAVE_ARCH_AUDITSYSCALL=y
CONFIG_AUDITSYSCALL=y

#
# IRQ subsystem
#
CONFIG_GENERIC_IRQ_PROBE=y
CONFIG_GENERIC_IRQ_SHOW=y
CONFIG_HARDIRQS_SW_RESEND=y
CO

Re: [PATCH] x86/HVM: drop stale check from hvm_load_cpu_msrs()

[Andrew Cooper]

On 29/11/2022 14:51, Jan Beulich wrote:
> Up until f61685a66903 ("x86: remove defunct init/load/save_msr()
> hvm_funcs") the check of the _rsvd field served as an error check for
> the earlier hvm_funcs.save_msr() invocation. With that invocation gone
> the check makes no sense anymore. While dropping it also merge the two
> paths setting "err" to -ENXIO.
>
> Signed-off-by: Jan Beulich 
> ---
> We could go further here, removing the local "err" variable altogether,
> by using "return -ENXIO". Thoughts.

'err' is a non-standard variable name, so yeah, why not.

That said, the current code has a split loop checking the incoming _rsvd
fields in a first pass, and then calling guest_wrmsr() on the second
pass.  This was also made pointless by the identified changeset, so the
two loops ought to be merged.

~Andrew

Re: [PATCH] x86/MSR: use latched "current"

[Andrew Cooper]

On 29/11/2022 14:49, Jan Beulich wrote:
> There's no need to recalculate / refetch the value from the stack
> (pointer).
>
> Signed-off-by: Jan Beulich 

Acked-by: Andrew Cooper 

Re: [PATCH] bump default SeaBIOS version to 1.16.1

[Andrew Cooper]

On 29/11/2022 14:36, Jan Beulich wrote:
> Signed-off-by: Jan Beulich 

Acked-by: Andrew Cooper 

Re: [PATCH 1/2 RESEND] CODING_STYLE: explicitly call out label indentation

[Stefano Stabellini]

On Fri, 19 Jul 2019, Jan Beulich wrote:
> Since the behavior of "diff -p" to use an unindented label as context
> identifier often makes it harder to review patches, make explicit the
> requirement for labels to be indented.
> 
> Signed-off-by: Jan Beulich 

Acked-by: Stefano Stabellini 


> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -31,6 +31,10 @@ void fun(void)
>   }
>   }
>   
> +Due to the behavior of GNU diffutils "diff -p", labels should be
> +indented by at least one blank.  Non-case labels inside switch() bodies
> +are preferred to be indented the same as the block's case labels.
> +
>   White space
>   ---
>   
> 
> 

Re: [PATCH 2/2] CODING_STYLE: list further brace placement exceptions

[Stefano Stabellini]

On Fri, 19 Jul 2019, Jan Beulich wrote:
> For easy spotting of struct/union/enum definitions we already commonly
> place the opening braces on the initial line of such a definition.
> 
> We also often don't place the opening brace of an initializer on a
> separate line.
> 
> And finally for compound literals placing the braces on separate lines
> often makes the code more difficult to read, so it should (and in
> practice does) typically go on the same line as well.  The placement of
> the closing brace often depends on how large such a compound literal is.
> 
> Signed-off-by: Jan Beulich 

Acked-by: Stefano Stabellini 


> ---
> TBD: We may want to make explicit that for initializers both forms are
>   fine.
> 
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -64,8 +64,13 @@ Bracing
>   ---
>   
>   Braces ('{' and '}') are usually placed on a line of their own, except
> -for the do/while loop.  This is unlike the Linux coding style and
> -unlike K&R.  do/while loops are an exception. e.g.:
> +for
> +- the do/while loop
> +- the opening brace in definitions of enum, struct, and union
> +- the opening brace in initializers
> +- compound literals
> +This is unlike the Linux coding style and unlike K&R.  do/while loops
> +are one exception. e.g.:
>   
>   if ( condition )
>   {
> 
> 

Re: [PATCH] x86/mm: PGC_page_table is used by shadow code only

[Andrew Cooper]

On 29/11/2022 14:55, Jan Beulich wrote:
> By defining the constant to zero when !SHADOW_PAGING we give compilers
> the chance to eliminate a little more dead code elsewhere in the tree.
> Plus, as a minor benefit, the general reference count can be one bit
> wider. (To simplify things, have PGC_page_table change places with
> PGC_extra.)
>
> Signed-off-by: Jan Beulich 

Ahead of making this change, can we please rename it to something less
confusing, and fix it's comment which is wrong.

PGC_shadowed_pt is the best I can think of.

> ---
> tboot.c's update_pagetable_mac() is suspicious: It effectively is a
> no-op even prior to this change when !SHADOW_PAGING, which can't be
> quite right. If (guest) page tables are relevant to include in the
> verification, shouldn't this look for PGT_l_page_table as well? How
> to deal with HAP guests there is entirely unclear.

Considering the caller, it MACs every domheap page for domains with
CDF_s3_integrity.

The tboot logical also blindly assumes that any non-idle domain has an
Intel IOMMU context with it.  This only doesn't (trivially) expose
because struct domain_iommu is embedded in struct domain (rather than
allocated separately), and reaching into the wrong part of the arch
union is only mitigated by the tboot logic not being invoked on
non-intel systems.  (Also the idle domain check is useless, given that
it's in a for_each_domain() loop).

It does look a little like the caller is wanting to MAC all Xen data
that describes the guest, but doing this unilaterally for all shadowed
guests seems wrong beside the per-domain s3_integrity setting.

~Andrew

[linux-linus test] 174988: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-arm64-arm64-examine  8 reboot   fail REGR. vs. 173462
 test-arm64-arm64-xl-xsm   8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-xl-seattle   8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-xl-vhd   8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-xl-credit2   8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-xl-credit1   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-examine  8 reboot   fail REGR. vs. 173462
 test-armhf-armhf-xl   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-libvirt-raw  8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-xl   8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-libvirt-raw  8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-libvirt-xsm  8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-xl-multivcpu  8 xen-bootfail REGR. vs. 173462
 test-armhf-armhf-xl-credit2   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-xl-arndale   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-xl-vhd   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-xl-credit1   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-libvirt  8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-libvirt-qcow2  8 xen-boot   fail REGR. vs. 173462

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

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

version targeted for testing:
 linuxca57f02295f188d6c65ec02202402979880fa6d8
baseline version:
 linux9d84bb40bcb30a7fa16f33baa967aeb9953dda78

Last test of basis   173462  2022-10-07 18:41:45 Z   53 days
Failing since173470  2022-10-08 06:21:34 Z   52 days  101 attempts
Testing same since   174988  2022-11-29 13:41:02 Z0 days1 attempts


1907 people touched revisions under test,
not listing them all

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-xl  pass
 test-amd64-coresched-amd64-xlpass
 test-arm64-arm64-xl  fail
 test-armhf-armhf-xl  fail
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm   pass
 test-amd64-am

Re: [PATCH] x86/mm: PGC_page_table is used by shadow code only

[Andrew Cooper]

On 29/11/2022 20:56, Andrew Cooper wrote:
> On 29/11/2022 14:55, Jan Beulich wrote:
>> By defining the constant to zero when !SHADOW_PAGING we give compilers
>> the chance to eliminate a little more dead code elsewhere in the tree.
>> Plus, as a minor benefit, the general reference count can be one bit
>> wider. (To simplify things, have PGC_page_table change places with
>> PGC_extra.)
>>
>> Signed-off-by: Jan Beulich 
> Ahead of making this change, can we please rename it to something less
> confusing, and fix it's comment which is wrong.
>
> PGC_shadowed_pt is the best I can think of.
>
>> ---
>> tboot.c's update_pagetable_mac() is suspicious: It effectively is a
>> no-op even prior to this change when !SHADOW_PAGING, which can't be
>> quite right. If (guest) page tables are relevant to include in the
>> verification, shouldn't this look for PGT_l_page_table as well? How
>> to deal with HAP guests there is entirely unclear.
> Considering the caller, it MACs every domheap page for domains with
> CDF_s3_integrity.
>
> The tboot logical also blindly assumes that any non-idle domain has an
> Intel IOMMU context with it.  This only doesn't (trivially) expose
> because struct domain_iommu is embedded in struct domain (rather than
> allocated separately), and reaching into the wrong part of the arch
> union is only mitigated by the tboot logic not being invoked on
> non-intel systems.  (Also the idle domain check is useless, given that
> it's in a for_each_domain() loop).

Wow I really failed at typing here.  "The tboot logic", and "doesn't
(trivially) explode".

~Andrew

>
> It does look a little like the caller is wanting to MAC all Xen data
> that describes the guest, but doing this unilaterally for all shadowed
> guests seems wrong beside the per-domain s3_integrity setting.
>
> ~Andrew

Re: [PATCH] hvc/xen: prevent concurrent accesses to the shared ring

[Stefano Stabellini]

On Tue, 29 Nov 2022, Roger Pau Monne wrote:
> The hvc machinery registers both a console and a tty device based on
> the hv ops provided by the specific implementation.  Those two
> interfaces however have different locks, and there's no single locks
> that's shared between the tty and the console implementations, hence
> the driver needs to protect itself against concurrent accesses.
> Otherwise concurrent calls using the split interfaces are likely to
> corrupt the ring indexes, leaving the console unusable.
>
> Introduce a lock to xencons_info to serialize accesses to the shared
> ring.  This is only required when using the shared memory console,
> concurrent accesses to the hypercall based console implementation are
> not an issue.
> 
> Note the conditional logic in domU_read_console() is slightly modified
> so the notify_daemon() call can be done outside of the locked region:
> it's an hypercall and there's no need for it to be done with the lock
> held.
> 
> Fixes: b536b4b96230 ('xen: use the hvc console infrastructure for Xen 
> console')
> Signed-off-by: Roger Pau Monné 
> ---
> While the write handler (domU_write_console()) is used by both the
> console and the tty ops, that's not the case for the read side
> (domU_read_console()).  It's not obvious to me whether we could get
> concurrent poll calls from the poll_get_char tty hook, hence stay on
> the safe side also serialize read accesses in domU_read_console().

I think domU_read_console doesn't need it. struct hv_ops and struct
console are both already locked although independently locked.

I think we shouldn't add an unrequired lock there.


> ---
>  drivers/tty/hvc/hvc_xen.c | 16 ++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index 7c23112dc923..d65741983837 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -43,6 +43,7 @@ struct xencons_info {
>   int irq;
>   int vtermno;
>   grant_ref_t gntref;
> + spinlock_t ring_lock;
>  };
>  
>  static LIST_HEAD(xenconsoles);
> @@ -84,12 +85,15 @@ static int __write_console(struct xencons_info *xencons,
>   XENCONS_RING_IDX cons, prod;
>   struct xencons_interface *intf = xencons->intf;
>   int sent = 0;
> + unsigned long flags;
>  
> + spin_lock_irqsave(&xencons->ring_lock, flags);
>   cons = intf->out_cons;
>   prod = intf->out_prod;
>   mb();   /* update queue values before going on */
>  
>   if ((prod - cons) > sizeof(intf->out)) {
> + spin_unlock_irqrestore(&xencons->ring_lock, flags);
>   pr_err_once("xencons: Illegal ring page indices");
>   return -EINVAL;
>   }
> @@ -99,6 +103,7 @@ static int __write_console(struct xencons_info *xencons,
>  
>   wmb();  /* write ring before updating pointer */
>   intf->out_prod = prod;
> + spin_unlock_irqrestore(&xencons->ring_lock, flags);
>  
>   if (sent)
>   notify_daemon(xencons);
> @@ -141,16 +146,19 @@ static int domU_read_console(uint32_t vtermno, char 
> *buf, int len)
>   int recv = 0;
>   struct xencons_info *xencons = vtermno_to_xencons(vtermno);
>   unsigned int eoiflag = 0;
> + unsigned long flags;
>  
>   if (xencons == NULL)
>   return -EINVAL;
>   intf = xencons->intf;
>  
> + spin_lock_irqsave(&xencons->ring_lock, flags);
>   cons = intf->in_cons;
>   prod = intf->in_prod;
>   mb();   /* get pointers before reading ring */
>  
>   if ((prod - cons) > sizeof(intf->in)) {
> + spin_unlock_irqrestore(&xencons->ring_lock, flags);
>   pr_err_once("xencons: Illegal ring page indices");
>   return -EINVAL;
>   }
> @@ -174,10 +182,13 @@ static int domU_read_console(uint32_t vtermno, char 
> *buf, int len)
>   xencons->out_cons = intf->out_cons;
>   xencons->out_cons_same = 0;
>   }
> + if (!recv && xencons->out_cons_same++ > 1) {
> + eoiflag = XEN_EOI_FLAG_SPURIOUS;
> + }
> + spin_unlock_irqrestore(&xencons->ring_lock, flags);
> +
>   if (recv) {
>   notify_daemon(xencons);
> - } else if (xencons->out_cons_same++ > 1) {
> - eoiflag = XEN_EOI_FLAG_SPURIOUS;
>   }
>  
>   xen_irq_lateeoi(xencons->irq, eoiflag);
> @@ -576,6 +587,7 @@ static int __init xen_hvc_init(void)
>  
>   info = vtermno_to_xencons(HVC_COOKIE);
>   info->irq = bind_evtchn_to_irq_lateeoi(info->evtchn);
> + spin_lock_init(&info->ring_lock);

Don't we also need a call to spin_lock_init in xencons_connect_backend
and xen_cons_init and xenboot_console_setup ?


>   }
>   if (info->irq < 0)
>   info->irq = 0; /* NO_IRQ */
> -- 
> 2.37.3
> 

Re: [PATCH] hvc/xen: lock console list traversal

[Stefano Stabellini]

On Tue, 29 Nov 2022, Roger Pau Monne wrote:
> The currently lockless access to the xen console list in
> vtermno_to_xencons() is incorrect, as additions and removals from the
> list can happen anytime, and as such the traversal of the list to get
> the private console data for a given termno needs to happen with the
> lock held.  Note users that modify the list already do so with the
> lock taken.
> 
> While there switch from using list_for_each_entry_safe to
> list_for_each_entry: the current entry cursor won't be removed as
> part of the code in the loop body, so using the _safe variant is
> pointless.
> 
> Fixes: 02e19f9c7cac ('hvc_xen: implement multiconsole support')
> Signed-off-by: Roger Pau Monné 
> ---
>  drivers/tty/hvc/hvc_xen.c | 13 +
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index d65741983837..117dc48f980e 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -53,17 +53,22 @@ static DEFINE_SPINLOCK(xencons_lock);
>  
>  static struct xencons_info *vtermno_to_xencons(int vtermno)
>  {
> - struct xencons_info *entry, *n, *ret = NULL;
> + struct xencons_info *entry, *ret = NULL;
> + unsigned long flags;
>  
> - if (list_empty(&xenconsoles))
> - return NULL;
> + spin_lock_irqsave(&xencons_lock, flags);

If xencons_lock requires irqsave then we need to change all the
xencons_lock spinlocks to call irqsave, including the ones in
xen_hvm_console_init if they can happen at runtime.


> + if (list_empty(&xenconsoles)) {
> + spin_unlock_irqrestore(&xencons_lock, flags);
> + return NULL;
> + }
>  
> - list_for_each_entry_safe(entry, n, &xenconsoles, list) {
> + list_for_each_entry(entry, &xenconsoles, list) {
>   if (entry->vtermno == vtermno) {
>   ret  = entry;
>   break;
>   }
>   }
> + spin_unlock_irqrestore(&xencons_lock, flags);
>  
>   return ret;
>  }
> -- 
> 2.37.3
> 

[ovmf test] 174990: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf 8aff08c817728092fda5707ae27cfa6321108980
baseline version:
 ovmf b92e0495221a3b298b069d9fb01e48fd2a0469f6

Last test of basis   174987  2022-11-29 10:13:15 Z0 days
Testing same since   174990  2022-11-29 19:40:43 Z0 days1 attempts


People who touched revisions under test:
  Rebecca Cran 

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
   b92e049522..8aff08c817  8aff08c817728092fda5707ae27cfa6321108980 -> 
xen-tested-master

Re: [PATCH 3/4] tools/misra: fix skipped rule numbers

[Stefano Stabellini]

On Mon, 28 Nov 2022, Luca Fancellu wrote:
> Currently the script convert_misra_doc.py is using a loop through
> range(1,22) to enumerate rules that needs to be skipped, however
> range function does not include the stop counter in the enumeration
> ending up into list rules until 21.21 instead of including rule 22.
> 
> Fix the issue using a dictionary that list the rules in misra c2012.

I think I understand the problem you are trying to solve with this
patch. But I am confused about the proposed solution.

The original code is trying to list all the possible MISRA C rules that
are not in docs/misra/rules.rst. Instead of list(range(1,22)) now we
have a dictionary: misra_c2012_rules. But misra_c2012_rules doesn't have
all the possible MISRA C rules missing from docs/misra/rules.rst.

As an example Rule 13.1 is missing from docs/misra/rules.rst but it is
also missing from misra_c2012_rules.

Can you please help me understand why misra_c2012_rules has only a small
subset of MISRA C rules to be skipped?


> Fixes: 57caa5375321 ("xen: Add MISRA support to cppcheck make rule")
> Signed-off-by: Luca Fancellu 
> ---
>  xen/tools/convert_misra_doc.py | 32 ++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/tools/convert_misra_doc.py b/xen/tools/convert_misra_doc.py
> index caa4487f645f..13074d8a2e91 100755
> --- a/xen/tools/convert_misra_doc.py
> +++ b/xen/tools/convert_misra_doc.py
> @@ -14,6 +14,34 @@ Usage:
>  
>  import sys, getopt, re
>  
> +# MISRA rule are identified by two numbers, e.g. Rule 1.2, the main rule 
> number
> +# and a sub-number. This dictionary contains the number of the MISRA rule as 
> key
> +# and the maximum sub-number for that rule as value.
> +misra_c2012_rules = {
> +1:4,
> +2:7,
> +3:2,
> +4:2,
> +5:9,
> +6:2,
> +7:4,
> +8:14,
> +9:5,
> +10:8,
> +11:9,
> +12:5,
> +13:6,
> +14:4,
> +15:7,
> +16:7,
> +17:8,
> +18:8,
> +19:2,
> +20:14,
> +21:21,
> +22:10
> +}
> +
>  def main(argv):
>  infile = ''
>  outfile = ''
> @@ -142,8 +170,8 @@ def main(argv):
>  skip_list = []
>  
>  # Search for missing rules and add a dummy text with the rule number
> -for i in list(range(1,22)):
> -for j in list(range(1,22)):
> +for i in misra_c2012_rules:
> +for j in list(range(1,misra_c2012_rules[i]+1)):
>  if str(i) + '.' + str(j) not in rule_list:
>  outstr.write('Rule ' + str(i) + '.' + str(j) + '\n')
>  outstr.write('No description for rule ' + str(i) + '.' + 
> str(j)
> -- 
> 2.17.1
> 

[ovmf test] 174992: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf dd3ba82d31a6d3cc4564dc83c9229e13773b55da
baseline version:
 ovmf 8aff08c817728092fda5707ae27cfa6321108980

Last test of basis   174990  2022-11-29 19:40:43 Z0 days
Testing same since   174992  2022-11-29 23:10:27 Z0 days1 attempts


People who touched revisions under test:
  Michael Kubacki 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 pass
 test-amd64-i386-xl-qemuu-ovmf-amd64  pass



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

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

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

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


Pushing revision :

To xenbits.xen.org:/home/xen/git/osstest/ovmf.git
   8aff08c817..dd3ba82d31  dd3ba82d31a6d3cc4564dc83c9229e13773b55da -> 
xen-tested-master

Re: [PATCH 2/4] xen/scripts: add cppcheck tool to the xen-analysis.py script

[Stefano Stabellini]

On Mon, 28 Nov 2022, Luca Fancellu wrote:
> Change cppcheck invocation method by using the xen-analysis.py
> script using the arguments --run-cppcheck.
> 
> Now cppcheck analysis will build Xen while the analysis is performed
> on the source files, it will produce a text report and an additional
> html output when the script is called with --cppcheck-html.
> 
> With this patch cppcheck will benefit of platform configuration files
> that will help it to understand the target of the compilation and
> improve the analysis.
> 
> To do so:
>  - remove cppcheck rules from Makefile and move them to the script.
>  - Update xen-analysis.py with the code to integrate cppcheck.
>  - merge the script merge_cppcheck_reports.py into the xen-analysis
>script package and rework the code to integrate it.
>  - add platform configuration files for cppcheck..
>  - add cppcheck-cc.sh script that is a wrapper for cppcheck and it's
>used as Xen compiler, it will intercept all flags given from the
>make build system and it will execute cppcheck on the compiled
>file together with the file compilation.
>  - guarded hypercall-defs.c with CPPCHECK define because cppcheck
>gets confused as the file does not contain c code.
>  - add false-positive-cppcheck.json file
>  - update documentation.
>  - update .gitignore
> 
> Signed-off-by: Luca Fancellu 

I think the revert of the cppcheck integration in xen/Makefile and
xen/tools/merge_cppcheck_reports.py could be a separate patch. There is
no need to make sure cppcheck support in the xen Makefile is
"bisectable". That patch could have my acked-by already.

Also the document changes introduced in this patch have my reviewed-by:
- docs/misra/cppcheck.txt
- docs/misra/documenting-violations.rst
- docs/misra/false-positive-cppcheck.json
- docs/misra/xen-static-analysis.rst

More below


> ---
>  .gitignore|   8 +-
>  docs/misra/cppcheck.txt   |  27 +-
>  docs/misra/documenting-violations.rst |   7 +-
>  docs/misra/false-positive-cppcheck.json   |  12 +
>  docs/misra/xen-static-analysis.rst|  42 ++-
>  xen/Makefile  | 116 +---
>  xen/include/hypercall-defs.c  |   9 +
>  xen/scripts/xen-analysis.py   |  18 +-
>  xen/scripts/xen_analysis/cppcheck_analysis.py | 272 ++
>  .../xen_analysis/cppcheck_report_utils.py | 130 +
>  xen/scripts/xen_analysis/generic_analysis.py  |  21 +-
>  xen/scripts/xen_analysis/settings.py  |  77 -
>  xen/scripts/xen_analysis/utils.py |  21 +-
>  xen/tools/cppcheck-cc.sh  | 223 ++
>  xen/tools/cppcheck-plat/arm32-wchar_t4.xml|  17 ++
>  xen/tools/cppcheck-plat/arm64-wchar_t2.xml|  17 ++
>  xen/tools/cppcheck-plat/arm64-wchar_t4.xml|  17 ++
>  xen/tools/cppcheck-plat/x86_64-wchar_t2.xml   |  17 ++
>  xen/tools/cppcheck-plat/x86_64-wchar_t4.xml   |  17 ++
>  xen/tools/merge_cppcheck_reports.py   |  86 --
>  20 files changed, 899 insertions(+), 255 deletions(-)
>  create mode 100644 docs/misra/false-positive-cppcheck.json
>  create mode 100644 xen/scripts/xen_analysis/cppcheck_analysis.py
>  create mode 100644 xen/scripts/xen_analysis/cppcheck_report_utils.py
>  create mode 100755 xen/tools/cppcheck-cc.sh
>  create mode 100644 xen/tools/cppcheck-plat/arm32-wchar_t4.xml
>  create mode 100644 xen/tools/cppcheck-plat/arm64-wchar_t2.xml
>  create mode 100644 xen/tools/cppcheck-plat/arm64-wchar_t4.xml
>  create mode 100644 xen/tools/cppcheck-plat/x86_64-wchar_t2.xml
>  create mode 100644 xen/tools/cppcheck-plat/x86_64-wchar_t4.xml
>  delete mode 100755 xen/tools/merge_cppcheck_reports.py
> 
> diff --git a/.gitignore b/.gitignore
> index f5a66f6194dd..68566d0c2587 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -7,9 +7,11 @@
>  *.o
>  *.d
>  *.d2
> -*.c.cppcheck
> +*.cppcheck.txt
> +*.cppcheck.xml
>  *.opic
>  *.a
> +*.c.json
>  *.safparse
>  *.so
>  *.so.[0-9]*
> @@ -282,9 +284,11 @@ xen/arch/*/efi/efi.h
>  xen/arch/*/efi/pe.c
>  xen/arch/*/efi/runtime.c
>  xen/arch/*/include/asm/asm-offsets.h
> +xen/build-dir-cppcheck/
>  xen/common/config_data.S
>  xen/common/config.gz
>  xen/cppcheck-htmlreport/
> +xen/cppcheck-report/
>  xen/cppcheck-misra.*
>  xen/include/headers*.chk
>  xen/include/compat/*
> @@ -315,7 +319,7 @@ xen/xsm/flask/xenpolicy-*
>  tools/flask/policy/policy.conf
>  tools/flask/policy/xenpolicy-*
>  xen/xen
> -xen/xen-cppcheck.xml
> +xen/suppression-list.txt
>  xen/xen-syms
>  xen/xen-syms.map
>  xen/xen.*
> diff --git a/docs/misra/cppcheck.txt b/docs/misra/cppcheck.txt
> index 25d8c3050b72..f7b9f678b4d5 100644
> --- a/docs/misra/cppcheck.txt
> +++ b/docs/misra/cppcheck.txt
> @@ -3,8 +3,7 @@ Cppcheck for Xen static and MISRA analysis
>  
>  Xen can be analysed for both static analysis problems and MISRA violation 
> using
>  cppcheck, the open source tool allows the creation of a report wit

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 qemuuecbb6bd865d23ec412b9f2b715be784e45389f91
baseline version:
 qemuuac149498215809bfb5c0ddce1953519fbfda5004

Last test of basis   174976  2022-11-28 01:08:46 Z1 days
Testing same since   174989  2022-11-29 19:38:28 Z0 days1 attempts


People who touched revisions under test:
  Gerd Hoffmann 
  Richard Henderson 
  Stefan Hajnocz

[linux-linus test] 174991: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-arm64-arm64-examine  8 reboot   fail REGR. vs. 173462
 test-arm64-arm64-xl-xsm   8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-xl-seattle   8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-xl-vhd   8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-xl-credit2   8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-xl-credit1   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-examine  8 reboot   fail REGR. vs. 173462
 test-armhf-armhf-xl   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-libvirt-raw  8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-xl   8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-libvirt-raw  8 xen-boot fail REGR. vs. 173462
 test-arm64-arm64-libvirt-xsm  8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-xl-multivcpu  8 xen-bootfail REGR. vs. 173462
 test-armhf-armhf-xl-credit2   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-xl-arndale   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-xl-vhd   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-xl-credit1   8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-libvirt  8 xen-boot fail REGR. vs. 173462
 test-armhf-armhf-libvirt-qcow2  8 xen-boot   fail REGR. vs. 173462

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

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

version targeted for testing:
 linux01f856ae6d0ca5ad0505b79bf2d22d7ca439b2a1
baseline version:
 linux9d84bb40bcb30a7fa16f33baa967aeb9953dda78

Last test of basis   173462  2022-10-07 18:41:45 Z   53 days
Failing since173470  2022-10-08 06:21:34 Z   53 days  102 attempts
Testing same since   174991  2022-11-29 21:43:19 Z0 days1 attempts


1924 people touched revisions under test,
not listing them all

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-xl  pass
 test-amd64-coresched-amd64-xlpass
 test-arm64-arm64-xl  fail
 test-armhf-armhf-xl  fail
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm   pass
 test-amd64-am

Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses

[Jan Beulich]

On 29.11.2022 19:18, Ayan Kumar Halder wrote:
> On 29/11/2022 15:52, Julien Grall wrote:
>> On 29/11/2022 16:23, Ayan Kumar Halder wrote:
>>> On 29/11/2022 14:52, Julien Grall wrote:
 On 29/11/2022 14:57, Ayan Kumar Halder wrote:
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -2245,7 +2245,9 @@ void __init xenheap_max_mfn(unsigned long mfn)
>>>   {
>>>   ASSERT(!first_node_initialised);
>>>   ASSERT(!xenheap_bits);
>>> +#ifndef CONFIG_AARCH32_V8R
>>>   BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
>>> +#endif
>>
>> BUILD_BUG_ON() are used to indicate that the code would fall over the 
>> check pass. I can't find the justification for this change in the 
>> commit message.
> 
> I had a look at the following commit which introduced this, but I 
> couldn't get the explaination for this.
> 
> commit 88e3ed61642bb393458acc7a9bd2f96edc337190
> Author: Jan Beulich 
> Date:   Tue Sep 1 14:02:57 2015 +0200
> 
> @Jan :- Do you know why BUILD_BUG_ON() was introduced ?

You've cut too much context - the next line explains this all by itself,
I think:

xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);

Clearly addresses used for the Xen heap need to be representable in an
unsigned long (which we assume to be the same size as void *).

But I'm afraid there's further context missing for your question: Why
would that construct be a problem in your case? Is it just that you'd
need it to be > rather than the >= that's used presently? If so, why
do you add an #ifdef rather than dealing with the (apparent) off-by-1?
(I say "apparent" because I haven't checked whether the >= is really
depended upon anywhere.)

Jan

Re: [PATCH] Arm64: make setup_virt_paging()'s pa_range_info[] static

[Jan Beulich]

On 29.11.2022 17:01, Julien Grall wrote:
> On 29/11/2022 15:39, Jan Beulich wrote:
>> While not as inefficient as it would be on x86 (due to suitable constant
>> loading and register pair storing instructions being available to fill
>> some of the fields), having the compiler construct an array of constants
>> on the stack still looks odd to me.
> 
> The function is only called once at boot. So this seems more a 
> micro-optimization than anything else.

Well, yes - hence the "looks odd" as a justification, not performance or
anything.

>> Signed-off-by: Jan Beulich 
> 
> Acked-by: Julien Grall 

Thanks.

>> ---
>> Actual space savings could be had if further converting the field types
>> to e.g. unsigned char (all of the values fit in that type).
> 
> This is a micro-optimization. If you want to send it then I will review it.

I probably won't bother; I've pointed this out largely in case actual
space savings would be made a requirement to accept this change.

Jan

Re: [PATCH] x86/p2m: don't calculate page owner twice in p2m_add_page()

[Jan Beulich]

On 29.11.2022 17:44, Roger Pau Monné wrote:
> On Tue, Nov 29, 2022 at 03:47:53PM +0100, Jan Beulich wrote:
>> Neither page_get_owner() nor mfn_to_page() are entirely trivial
>> operations - don't do the same thing twice in close succession. Instead
>> help CSE (when MEM_SHARING=y) by introducing a local variable holding
>> the page owner.
>>
>> Signed-off-by: Jan Beulich 
> 
> Acked-by: Roger Pau Monné 

Thanks.

>> ---
>> According to my observations gcc12 manages to CSE mfn_to_page() but not
>> (all of) page_get_owner(). The overall savings there are, partly due to
>> knock-on effects, 64 bytes of code.
>>
>> While looking there, "mfn_eq(omfn, mfn_add(mfn, i))" near the end of the
>> same loop caught my eye: Is that really correct? Shouldn't we fail the
>> operation if the MFN which "ogfn" was derived from doesn't match the MFN
>> "ogfn" maps to?
> 
> Getting into that state possibly means something has gone wrong if we
> have rules out grants and foreign maps?
> 
> So it should be:
> 
> if ( !mfn_eq(omfn, mfn_add(mfn, i)) )
> {
> /* Something has gone wrong, ASSERT_UNREACHABLE()? */
> goto out;
> }
> rc = p2m_remove_entry(p2m, ogfn, omfn, 0)
> if ( rc )
> goto out;
> 
> but maybe I'm missing the point of the check there,

Hence my question, rather than making a patch right away. I was
hoping that maybe someone might see or recall why such a check would
have been put there.

I'm not certain enough to put ASSERT_UNREACHABLE() there, though. I
might make it a one-time warning instead.

> I have to admit I
> sometimes find the p2m code difficult to follow.

You're not the only one.

Jan

Re: [PATCH] x86/HVM: drop stale check from hvm_load_cpu_msrs()

[Jan Beulich]

On 29.11.2022 21:36, Andrew Cooper wrote:
> On 29/11/2022 14:51, Jan Beulich wrote:
>> Up until f61685a66903 ("x86: remove defunct init/load/save_msr()
>> hvm_funcs") the check of the _rsvd field served as an error check for
>> the earlier hvm_funcs.save_msr() invocation. With that invocation gone
>> the check makes no sense anymore. While dropping it also merge the two
>> paths setting "err" to -ENXIO.
>>
>> Signed-off-by: Jan Beulich 
>> ---
>> We could go further here, removing the local "err" variable altogether,
>> by using "return -ENXIO". Thoughts.
> 
> 'err' is a non-standard variable name, so yeah, why not.

Okay, I'll make a v2 for this.

> That said, the current code has a split loop checking the incoming _rsvd
> fields in a first pass, and then calling guest_wrmsr() on the second
> pass.  This was also made pointless by the identified changeset, so the
> two loops ought to be merged.

Not really, no - it would violate the "Checking finished" comment (but
of course we could also delete that one), but I'd also prefer to keep
checking for all errors we can check for early _before_ starting to
make any changes to the guest. Therefore if you really wanted that, I
guess you'd need to make a follow-on change yourself, with a convincing
justification (I wouldn't outright object to such a change, but I
probably also wouldn't ack it, leaving that to someone else).

Jan

Re: [PATCH] x86/mm: PGC_page_table is used by shadow code only

[Jan Beulich]

On 29.11.2022 21:56, Andrew Cooper wrote:
> On 29/11/2022 14:55, Jan Beulich wrote:
>> By defining the constant to zero when !SHADOW_PAGING we give compilers
>> the chance to eliminate a little more dead code elsewhere in the tree.
>> Plus, as a minor benefit, the general reference count can be one bit
>> wider. (To simplify things, have PGC_page_table change places with
>> PGC_extra.)
>>
>> Signed-off-by: Jan Beulich 
> 
> Ahead of making this change, can we please rename it to something less
> confusing, and fix it's comment which is wrong.
> 
> PGC_shadowed_pt is the best I can think of.

Can do, sure.

>> ---
>> tboot.c's update_pagetable_mac() is suspicious: It effectively is a
>> no-op even prior to this change when !SHADOW_PAGING, which can't be
>> quite right. If (guest) page tables are relevant to include in the
>> verification, shouldn't this look for PGT_l_page_table as well? How
>> to deal with HAP guests there is entirely unclear.
> 
> Considering the caller, it MACs every domheap page for domains with
> CDF_s3_integrity.
> 
> The tboot logical also blindly assumes that any non-idle domain has an
> Intel IOMMU context with it.  This only doesn't (trivially) expose
> because struct domain_iommu is embedded in struct domain (rather than
> allocated separately), and reaching into the wrong part of the arch
> union is only mitigated by the tboot logic not being invoked on
> non-intel systems.  (Also the idle domain check is useless, given that
> it's in a for_each_domain() loop).
> 
> It does look a little like the caller is wanting to MAC all Xen data
> that describes the guest, but doing this unilaterally for all shadowed
> guests seems wrong beside the per-domain s3_integrity setting.

Question is - do we care about addressing this (when, as said, it's
unclear how to deal with HAP domains; maybe their actively used p2m
pages would need including instead)? Or should we rather consider
ripping out tboot support?

Jan

Re: [PATCH] x86/APIC: make a few interrupt handler functions static

[Jan Beulich]

On 29.11.2022 17:21, Andrew Cooper wrote:
> On 29/11/2022 16:05, Roger Pau Monné wrote:
>> On Tue, Nov 29, 2022 at 03:46:30PM +0100, Jan Beulich wrote:
>>> Four of them are used in apic.c only and hence better wouldn't be
>>> exposed to other CUs. To avoid the need for forward declarations, move
>>> apic_intr_init() past the four handlers.
>>>
>>> Signed-off-by: Jan Beulich 
>> Acked-by: Roger Pau Monné 

Thanks.

>> A nit below.
>>
>>> --- a/xen/arch/x86/apic.c
>>> +++ b/xen/arch/x86/apic.c
>>> @@ -127,21 +127,6 @@ void ack_bad_irq(unsigned int irq)
>>>  ack_APIC_irq();
>>>  }
>>>  
>>> -void __init apic_intr_init(void)
>>> -{
>>> -smp_intr_init();
>>> -
>>> -/* self generated IPI for local APIC timer */
>>> -set_direct_apic_vector(LOCAL_TIMER_VECTOR, apic_timer_interrupt);
>>> -
>>> -/* IPI vectors for APIC spurious and error interrupts */
>>> -set_direct_apic_vector(SPURIOUS_APIC_VECTOR, spurious_interrupt);
>>> -set_direct_apic_vector(ERROR_APIC_VECTOR, error_interrupt);
>>> -
>>> -/* Performance Counters Interrupt */
>>> -set_direct_apic_vector(PMU_APIC_VECTOR, pmu_apic_interrupt);
>>> -}
>>> -
>>>  /* Using APIC to generate smp_local_timer_interrupt? */
>>>  static bool __read_mostly using_apic_timer;
>>>  
>>> @@ -1363,7 +1348,7 @@ int reprogram_timer(s_time_t timeout)
>>>  return apic_tmict || !timeout;
>>>  }
>>>  
>>> -void cf_check apic_timer_interrupt(struct cpu_user_regs *regs)
>>> +static void cf_check apic_timer_interrupt(struct cpu_user_regs *regs)
>> Given that the function is now not exported out of apic.c, wouldn't it
>> be better to drop the apic_ prefix?
> 
> This is the handler for the apic timer, as opposed to the plethora of
> other timer interrupts we have elsewhere.
> 
> Simply "timer interrupt" is too generic a name.

I agree with Andrew here.

>> The same would likely apply to pmu_apic_interrupt() then.
> 
> This one could lose the infix.  All PMU interrupts are from an LVT
> vector.  It may have made a different back in the days of non-integrated
> APICs, but those days are long gone.

I'm happy to drop the infix. Won't bother sending a v2 just for this,
though - I'll simply put the adjusted patch in soon after the tree
has reopened.

Jan