Re: [XEN PATCH 4/4] x86/traps: address violation of MISRA C Rule 8.4

[Jan Beulich]

On 27.05.2024 16:53, Nicola Vetrini wrote:
> Rule 8.4 states: "A compatible declaration shall be visible when
> an object or function with external linkage is defined".
> 
> The function do_general_protection is either used is asm code
> or only within this unit, so there is no risk of this getting
> out of sync with its definition, but the function must remain
> extern.
> 
> Therefore, this function is deviated using a comment-based deviation.
> No functional change.
> 
> Signed-off-by: Nicola Vetrini 

Acked-by: Jan Beulich 

Albeit here, too, I'm not entirely happy with the wording, but I'll let
it go as is. While "there is no risk of this getting out of sync with
its definition" is true for the C side, there's always the risk of
assembly going out of sync with C, simply because the two cannot be
properly connected by any means.

Jan

Re: [XEN PATCH 3/4] x86: address violations of MISRA C Rule 8.4

[Jan Beulich]

On 27.05.2024 16:53, Nicola Vetrini wrote:
> Rule 8.4 states: "A compatible declaration shall be visible when
> an object or function with external linkage is defined."
> 
> These variables are only referenced from asm modules, so they
> need to be extern and there is negligible risk of them being
> used improperly without noticing.

"asm modules" isn't quite correct, as there's one use from inline
assembly. I have to admit I have no good wording suggestion other than
explicitly covering both: "asm modules or inline assembly". Yet that
then is ambiguous, as a use in inline assembly may also mean that
symbol is actually visible to the compiler by being mentioned as on of
the operands. Better ideas?

> As a result, they can be exempted using a comment-based deviation.
> No functional change.
> 
> Signed-off-by: Nicola Vetrini 

With suitably adjusted wording:
Acked-by: Jan Beulich 

Jan

Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()

[Jan Beulich]

On 24.05.2024 13:08, Oleksii Kurochko wrote:
> The following generic functions were introduced:
> * test_bit
> * generic__test_and_set_bit
> * generic__test_and_clear_bit
> * generic__test_and_change_bit
> 
> These functions and macros can be useful for architectures
> that don't have corresponding arch-specific instructions.
> 
> Also, the patch introduces the following generics which are
> used by the functions mentioned above:
> * BITOP_BITS_PER_WORD
> * BITOP_MASK
> * BITOP_WORD
> * BITOP_TYPE
> 
> BITS_PER_BYTE was moved to xen/bitops.h as BITS_PER_BYTE is the same
> across architectures.
> 
> The following approach was chosen for generic*() and arch*() bit
> operation functions:
> If the bit operation function that is going to be generic starts
> with the prefix "__", then the corresponding generic/arch function
> will also contain the "__" prefix. For example:
>  * test_bit() will be defined using arch_test_bit() and
>generic_test_bit().
>  * __test_and_set_bit() will be defined using
>arch__test_and_set_bit() and generic__test_and_set_bit().
> 
> Signed-off-by: Oleksii Kurochko 
> ---
>  Reviewed-by: Jan Beulich jbeul...@suse.com? Jan gave his R-by for the 
> previous
>  version of the patch, but some changes were done, so I wasn't sure if I could
>  use the R-by in this patch version.

That really depends on the nature of the changes. Seeing the pretty
long list below, I think it was appropriate to drop the R-b.

> ---
> Changes in V11:
>  - fix identation in generic_test_bit() function.
>  - move definition of BITS_PER_BYTE from /bitops.h to xen/bitops.h

I realize you were asked to do this, but I'm not overly happy with it.
BITS_PER_BYTE is far more similar to BITS_PER_LONG than to
BITOP_BITS_PER_WORD. Plus in any event that change is entirely unrelated
here.

>  - drop the changes in arm64/livepatch.c.
>  - update the the comments on top of functions: generic__test_and_set_bit(),
>generic__test_and_clear_bit(),  generic__test_and_change_bit(),
>generic_test_bit().
>  - Update footer after Signed-off section.
>  - Rebase the patch on top of staging branch, so it can be merged when 
> necessary
>approves will be given.
>  - Update the commit message.
> ---
>  xen/arch/arm/include/asm/bitops.h |  69 ---
>  xen/arch/ppc/include/asm/bitops.h |  54 -
>  xen/arch/ppc/include/asm/page.h   |   2 +-
>  xen/arch/ppc/mm-radix.c   |   2 +-
>  xen/arch/x86/include/asm/bitops.h |  31 ++---
>  xen/include/xen/bitops.h  | 185 ++
>  6 files changed, 196 insertions(+), 147 deletions(-)
> 
> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -103,8 +103,193 @@ static inline int generic_flsl(unsigned long x)
>   * Include this here because some architectures need generic_ffs/fls in
>   * scope
>   */
> +
> +#define BITOP_BITS_PER_WORD 32
> +typedef uint32_t bitop_uint_t;
> +
> +#define BITOP_MASK(nr)  ((bitop_uint_t)1 << ((nr) % BITOP_BITS_PER_WORD))
> +
> +#define BITOP_WORD(nr)  ((nr) / BITOP_BITS_PER_WORD)
> +
> +#define BITS_PER_BYTE 8

This, if to be moved at all, very clearly doesn't belong here. As much
as it's unrelated to this change, it's also unrelated to bitops as a
whole.

> +extern void __bitop_bad_size(void);
> +
> +#define bitop_bad_size(addr) (sizeof(*(addr)) < sizeof(bitop_uint_t))
> +
> +/* - Please tidy above here - */
> +
>  #include 
>  
> +/**
> + * generic__test_and_set_bit - Set a bit and return its old value
> + * @nr: Bit to set
> + * @addr: Address to count from
> + *
> + * This operation is non-atomic and can be reordered.
> + * If two examples of this operation race, one can appear to succeed

Why "examples"? Do you mean "instances"? It's further unclear what
"succeed" and "fail" here mean. The function after all has two
purposes: Checking and setting the specified bit. Therefore I think
you mean "succeed in updating the bit in memory", yet then it's
still unclear what the "appear" here means: The caller has no
indication of success/failure. Therefore I think "appear to" also
wants dropping.

> + * but actually fail.  You must protect multiple accesses with a lock.

That's not "must". An often better alternative is to use the atomic
forms instead. "Multiple" is imprecise, too: "Potentially racy" or
some such would come closer.

Also did Julien(?) really ask that these comments be reproduced on
both the functions supposed to be used throughout the codebase _and_
the generic_*() ones (being merely internal helpers/fallbacks)?

> +/**
> + * generic_test_bit - Determine whether a bit is set
> + * @nr: bit number to test
> + * @addr: Address to start counting from
> + *
> + * This operation is non-atomic and can be reordered.
> + * If two examples of this operation race, one can appear to succeed
> + * but actually fail.  You must protect multiple accesses with a lock.
> + */

You got carried away updating comments - there's no raciness for
simple test_bi

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 xen  96af090e33130b0bf0953f3ccab8e7a163392318
baseline version:
 xen  ac572152e578a8853de0534384c1539ec21f11e0

Last test of basis   186142  2024-05-25 00:02:11 Z3 days
Testing same since   186164  2024-05-28 02:00:23 Z0 days1 attempts


People who touched revisions under test:
  Jan Beulich 
  Nicola Vetrini 
  Stefano Stabellini 
  Stefano Stabellini 

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



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

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

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

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


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   ac572152e5..96af090e33  96af090e33130b0bf0953f3ccab8e7a163392318 -> smoke

Re: [XEN PATCH 2/4] automation/eclair_analysis: avoid an ECLAIR warning about escaping

[Stefano Stabellini]

On Mon, 27 May 2024, Jan Beulich wrote:
> On 27.05.2024 16:53, Nicola Vetrini wrote:
> > The parentheses in this regular expression should be doubly
> > escaped because they are undergo escaping twice.
> 
> Do you maybe mean "undergo expansion twice"?

Ahah yes. I fixed it on commit:

Acked-by: Stefano Stabellini 

 
> > Signed-off-by: Nicola Vetrini 
> > ---
> >  automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
> > b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > index b9b377c56b25..cf62a874d928 100644
> > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > @@ -405,8 +405,8 @@ explicit comment indicating the fallthrough intention 
> > is present."
> >  #
> >  
> >  -doc_begin="printf()-like functions are allowed to use the variadic 
> > features provided by stdarg.h."
> > --config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(^.*printk\(.*\)$)))"}
> > --config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(^.*printf\(.*\)$)))"}
> > +-config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(^.*printk\\(.*\\)$)))"}
> > +-config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(^.*printf\\(.*\\)$)))"}
> >  
> > -config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(name(panic)&&kind(function"}
> >  
> > -config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(name(elf_call_log_callback)&&kind(function"}
> >  
> > -config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(name(vprintk_common)&&kind(function"}
> 

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

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-examine  8 reboot  fail pass in 186162-retest

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

version targeted for testing:
 linux2bfcfd584ff5ccc8bb7acde19b42570414bf880b
baseline version:
 linux1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0

Last test of basis   186159  2024-05-27 06:48:45 Z0 days
Testing same since   186161  2024-05-27 16:11:54 Z0 days1 attempts


People who touched revisions under test:
  Adam Ford 
  Alexander Stein 
  Christian Brauner 
  David Howells 
  Fedor Pchelkin 
  Linus Torvalds 
  Marc Dionne 
  Shengjiu Wang 
  syzbot+d7c7a495a5e466c03...@syzkaller.appspotmail.com
  Ulf Hansson 
  Xu Yang 

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

Re: [PATCH v16 1/5] arm/vpci: honor access size when returning an error

[Julien Grall]

Hi Roger,

On 23/05/2024 08:55, Roger Pau Monné wrote:

On Wed, May 22, 2024 at 06:59:20PM -0400, Stewart Hildebrand wrote:

From: Volodymyr Babchuk 

Guest can try to read config space using different access sizes: 8,
16, 32, 64 bits. We need to take this into account when we are
returning an error back to MMIO handler, otherwise it is possible to
provide more data than requested: i.e. guest issues LDRB instruction
to read one byte, but we are writing 0x in the target
register.


Shouldn't this be taken care of in the trap handler subsystem, rather
than forcing each handler to ensure the returned data matches the
access size?


I understand how this can be useful when we return all 1s.

However, in most of the current cases, we already need to deal with the 
masking because the data is extracted from a wider field (for instance, 
see the vGIC emulation). For those handlers, I would argue it would be 
concerning/ a bug if the handler return bits above the access size. 
Although, this would only impact the guest itself.


So overall, this seems to be a matter of taste and I don't quite (yet) 
see the benefits to do it in io.c. Regardless that...




IOW, something like:

diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 96c740d5636c..b7e12df85f87 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -37,6 +37,7 @@ static enum io_state handle_read(const struct mmio_handler 
*handler,
  return IO_ABORT;

  r = sign_extend(dabt, r);
+r = r & GENMASK_ULL((1U << dabt.size) * 8 - 1, 0);


... in some case we need to sign extend up to the width of the register 
(even if the access is 8-byte). So we would need to do the masking 
*before* calling sign_extend().


Cheers,

--
Julien Grall

Re: [PATCH] docs/misra: add D4.12

[Stefano Stabellini]

On Wed, 15 May 2024, Stefano Stabellini wrote:
> On Wed, 15 May 2024, Jan Beulich wrote:
> > On 15.05.2024 01:15, Stefano Stabellini wrote:
> > > Add D4.12 with the same explanation as the rules of the R21 series.
> > > D4.12 refers to the standard library memory allocation functions and
> > > similar third party libraries with memory allocation functions. It
> > > doesn't refer to the in-tree implementation we have in Xen which is
> > > subject to MISRA C rules and MISRA C scanning.
> > > 
> > > Signed-off-by: Stefano Stabellini 
> > > 
> > > diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> > > index 80e5e972ad..bc8506add4 100644
> > > --- a/docs/misra/rules.rst
> > > +++ b/docs/misra/rules.rst
> > > @@ -76,6 +76,11 @@ maintainers if you want to suggest a change.
> > > considered libraries from MISRA C point of view as they are
> > > imported in source form)
> > >  
> > > +   * - `Dir 4.12 
> > > `_
> > > + - Required
> > > + - Dynamic memory allocation shall not be used
> > > + - Xen doesn't provide, use, or link against a Standard Library 
> > > [#xen-stdlib]_
> > 
> > I'm having trouble connecting this remark with the directive. We do have
> > dynamic memory allocation routines, and we use them. It doesn't really
> > matter that they don't come from an external library, does it?
> 
> Similarly to the 21.x rules series, it makes a difference if they are
> external libraries or code within the project. The rule points out that
> the standard library memory allocation functions can lead to undefined
> behavior. On the other hand, our own implementation under xen.git is
> subject to MISRA C scanning and all the other MISRA C rules.
> 
> The example in the link above, shows a use-after-free error that in our
> case it should be caught by other MISRA C rules scanning.

Just to close the loop on this -- I spoke with Roberto about D4.12, and
we decided that it is best to leave out this directive for now.

Re: [PATCH v3 2/4] xen/arm: Alloc XenStore page for Dom0less DomUs from hypervisor

[Stefano Stabellini]

On Mon, 27 May 2024, Jürgen Groß wrote:
> On 25.05.24 01:19, Stefano Stabellini wrote:
> > On Fri, 24 May 2024, Jürgen Groß wrote:
> > > On 24.05.24 15:58, Julien Grall wrote:
> > > > Hi Henry,
> > > > 
> > > > + Juergen as the Xenstore maintainers. I'd like his opinion on the
> > > > approach.
> > > > The documentation of the new logic is in:
> > > > 
> > > > https://lore.kernel.org/xen-devel/20240517032156.1490515-5-xin.wa...@amd.com/
> > > > 
> > > > FWIW I am happy in principle with the logic (this is what we discussed
> > > > on
> > > > the call last week). Some comments below.
> > > 
> > > I'm not against this logic, but I'm wondering why it needs to be so
> > > complicated.
> > 
> > Actually the reason I like it is that in my view, this is the simplest
> > approach. You allocate a domain, you also allocate the xenstore page
> > together with it. Initially the xenstore connection has an
> > "uninitialized" state, as it should be. That's it. At some point, when
> > xenstored is ready, the state changes to CONNECTED.
> > 
> > 
> > > Can't the domU itself allocate the Xenstore page from its RAM pages,
> > > write the PFN into the Xenstore grant tab entry, and then make it
> > > public via setting HVM_PARAM_STORE_PFN?
> > 
> > This is not simpler in my view
> 
> Okay, fine with me. I had the impression that violating the 1:1 mapping
> of the domain would add complexity, but if you are fine with that I don't
> mind your approach.

Yes, that's fine. Thanks!

Re: [PATCH v7 3/8] xen: Add xen_mr_is_memory()

[Philippe Mathieu-Daudé]

Hi Edgar,

On 24/5/24 12:51, Edgar E. Iglesias wrote:

From: "Edgar E. Iglesias" 

Add xen_mr_is_memory() to abstract away tests for the
xen_memory MR.

No functional changes.

Signed-off-by: Edgar E. Iglesias 
Reviewed-by: Stefano Stabellini 
Acked-by: David Hildenbrand 
---
  hw/xen/xen-hvm-common.c | 10 --
  include/sysemu/xen.h|  8 
  2 files changed, 16 insertions(+), 2 deletions(-)


To consolidate we could add:

  static MemoryRegion xen_memory;

  MemoryRegion *xen_mr_memory_init(uint64_t block_len)
  {
 assert(!xen_memory.size);
 memory_region_init_ram(&xen_memory, NULL, "xen.ram", block_len, 
&error_fatal);

 return &xen_memory;
  }

and remove the extern declaration.


diff --git a/include/sysemu/xen.h b/include/sysemu/xen.h
index 754ec2e6cb..dc72f83bcb 100644
--- a/include/sysemu/xen.h
+++ b/include/sysemu/xen.h
@@ -34,6 +34,8 @@ void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t 
length);
  void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
 struct MemoryRegion *mr, Error **errp);
  
+bool xen_mr_is_memory(MemoryRegion *mr);

+
  #else /* !CONFIG_XEN_IS_POSSIBLE */
  
  #define xen_enabled() 0

@@ -47,6 +49,12 @@ static inline void xen_ram_alloc(ram_addr_t ram_addr, 
ram_addr_t size,
  g_assert_not_reached();
  }
  
+static inline bool xen_mr_is_memory(MemoryRegion *mr)

+{
+g_assert_not_reached();
+return false;


No need for the stub, just always declare xen_mr_is_memory() ...

+}
+
  #endif /* CONFIG_XEN_IS_POSSIBLE */


... here.


  #endif


Removing the stub:
Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v7 6/8] xen: mapcache: Pass the ram_addr offset to xen_map_cache()

[Philippe Mathieu-Daudé]

On 24/5/24 12:51, Edgar E. Iglesias wrote:

From: "Edgar E. Iglesias" 

Pass the ram_addr offset to xen_map_cache.
This is in preparation for adding grant mappings that need
to compute the address within the RAMBlock.

No functional changes.

Signed-off-by: Edgar E. Iglesias 
Reviewed-by: David Hildenbrand 
Reviewed-by: Stefano Stabellini 
---
  hw/xen/xen-mapcache.c | 16 +++-
  include/sysemu/xen-mapcache.h |  2 ++
  system/physmem.c  |  9 +
  3 files changed, 18 insertions(+), 9 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: Code freeze date for Xen 4.19 is 31.05.2024

[Oleksii K.]

On Mon, 2024-05-27 at 17:55 +0200, Jan Beulich wrote:
> On 27.05.2024 17:52, Oleksii K. wrote:
> > On Mon, 2024-05-27 at 17:12 +0200, Jan Beulich wrote:
> > > On 27.05.2024 15:58, Oleksii K. wrote:
> > > > I would like to remind you that the code freeze date for Xen
> > > > 4.19
> > > > is
> > > > May 31, 2024.
> > > 
> > > I may be confused: With feature freeze having been last Friday
> > > aiui,
> > > isn't
> > > this a little too short a period? I was actually expecting a
> > > longer-
> > > than-
> > > normal one to account for the Xen Summit week.
> > It makes sense to move the last day to June 14. Consequently, we
> > would
> > need to adjust the schedule as follows:
> > 
> > Hard Code Freeze: from June 21 to June 28
> > Final commits: from July 5 to July 12
> > Release: July 17
> > 
> > And this schedule also looks good to me.
> > 
> > However, another option is to combine the Code Freeze and Hard Code
> > Freeze periods since both phases are about accepting only bug
> > fixes,
> > differing only in the severity of the fixes.
> 
> I'm okay with whichever way you want it. All I'm seeking is clarity.
I realized that we have the Xen Summit coming up, which means we'll
lose almost a week from the review process perspective.

I would like to hear the other maintainers' thoughts on updating our
release schedule to have the release on July 17.

~ Oleksii

Re: [PATCH v2 3/3] CHANGELOG: Mention libxl blktap/tapback support

[Oleksii K.]

On Sun, 2024-04-07 at 16:49 -0400, Jason Andryuk wrote:
> From: Jason Andryuk 
> 
> Add entry for backendtype=tap support in libxl.  blktap needs some
> changes to work with libxl, which haven't been merged.  They are
> available from this PR:
> https://github.com/xapi-project/blktap/pull/394
> 
> Signed-off-by: Jason Andryuk 
> ---
>  CHANGELOG.md | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index 8041cfb7d2..036328433d 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -21,6 +21,7 @@ The format is based on [Keep a
> Changelog](https://keepachangelog.com/en/1.0.0/)
>   for IPIs and Physical addressing mode for external interrupts.
>   - Add a new 9pfs backend running as a daemon in dom0. First user is
>     Xenstore-stubdom now being able to support full Xenstore trace
> capability.
> + - libxl support for backendtype=tap with tapback.
>  
>  ### Removed
>  - caml-stubdom.  It hasn't built since 2014, was pinned to Ocaml
> 4.02, and has

Acked-by: Oleksii Kurochko 

~ Oleksii

Re: Code freeze date for Xen 4.19 is 31.05.2024

[Jan Beulich]

On 27.05.2024 17:52, Oleksii K. wrote:
> On Mon, 2024-05-27 at 17:12 +0200, Jan Beulich wrote:
>> On 27.05.2024 15:58, Oleksii K. wrote:
>>> I would like to remind you that the code freeze date for Xen 4.19
>>> is
>>> May 31, 2024.
>>
>> I may be confused: With feature freeze having been last Friday aiui,
>> isn't
>> this a little too short a period? I was actually expecting a longer-
>> than-
>> normal one to account for the Xen Summit week.
> It makes sense to move the last day to June 14. Consequently, we would
> need to adjust the schedule as follows:
> 
> Hard Code Freeze: from June 21 to June 28
> Final commits: from July 5 to July 12
> Release: July 17
> 
> And this schedule also looks good to me.
> 
> However, another option is to combine the Code Freeze and Hard Code
> Freeze periods since both phases are about accepting only bug fixes,
> differing only in the severity of the fixes.

I'm okay with whichever way you want it. All I'm seeking is clarity.

Jan

Re: Code freeze date for Xen 4.19 is 31.05.2024

[Oleksii K.]

On Mon, 2024-05-27 at 17:12 +0200, Jan Beulich wrote:
> On 27.05.2024 15:58, Oleksii K. wrote:
> > I would like to remind you that the code freeze date for Xen 4.19
> > is
> > May 31, 2024.
> 
> I may be confused: With feature freeze having been last Friday aiui,
> isn't
> this a little too short a period? I was actually expecting a longer-
> than-
> normal one to account for the Xen Summit week.
It makes sense to move the last day to June 14. Consequently, we would
need to adjust the schedule as follows:

Hard Code Freeze: from June 21 to June 28
Final commits: from July 5 to July 12
Release: July 17

And this schedule also looks good to me.

However, another option is to combine the Code Freeze and Hard Code
Freeze periods since both phases are about accepting only bug fixes,
differing only in the severity of the fixes.

Thoughts?

~ Oleksii
> 
> Jan
> 
> > I'm okay with bug fixes being committed without my release ack
> > (just CC
> > me), except in cases where a one of maintainers gives a strong
> > NACK.
> > 
> > Have a nice week!
> > 
> > Best regards,
> > Oleksii
> 

Re: [XEN PATCH v4 0/3] x86: make Intel/AMD vPMU & MCE support configurable

[Jan Beulich]

Oleksii,

On 22.05.2024 10:37, Sergiy Kibrik wrote:
> Three remaining patches to separate support of Intel & AMD CPUs in Xen build.
> Most of related patches from previous series had already been merged.
> Specific changes since v3 are provided per-patch.
> 
> v3 series here:
> https://lore.kernel.org/xen-devel/cover.1715673586.git.sergiy_kib...@epam.com/
> 
>   -Sergiy
> 
> Sergiy Kibrik (3):
>   x86/intel: move vmce_has_lmce() routine to header
>   x86/MCE: add default switch case in init_nonfatal_mce_checker()
>   x86/MCE: optional build of AMD/Intel MCE code

As I'm apparently confused as to the state 4.19 is in, may I please ask
whether this series is still okay to go in, or whether it should be
postponed until after branching.

Thanks, Jan

Re: [XEN PATCH v4 2/3] x86/MCE: add default switch case in init_nonfatal_mce_checker()

[Jan Beulich]

On 22.05.2024 10:42, Sergiy Kibrik wrote:
> The default switch case block is wanted here, to handle situation
> e.g. of unexpected c->x86_vendor value -- then no mcheck init is done, but
> misleading message still gets logged anyway.
> 
> Signed-off-by: Sergiy Kibrik 

Acked-by: Jan Beulich 

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 linux1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
baseline version:
 linux56fb6f92854f29dcb6c3dc3ba92eeda1b615e88c

Last test of basis   186148  2024-05-25 18:11:58 Z1 days
Failing since186152  2024-05-26 05:15:42 Z1 days4 attempts
Testing same since   186156  2024-05-27 01:11:41 Z0 days2 attempts


People who touched revisions under test:
  Al Viro 
  Alexandre Belloni 
  Allen Pais 
  Allen Pais 
  Andrew Morton 
  Andrey Konovalov 
  Arnaldo Carvalho de Melo 
  Arnd Bergmann 
  Artem Ikonnikov 
  Billy Tsai 
  Borislav Petkov (AMD) 
  Carsten Tolkmit 
  Chengming Zhou 
  Christian Heusel 
  Christophe JAILLET 
  Corey Minyard 
  Corey Minyard 
  David Hildenbrand 
  David Howells 
  Dev Jain 
  dicken.ding 
  Dongli Zhang 
  Duoming Zhou 
  Frank Li 
  Guenter Roeck 
  Hailong.Liu 
  Herbert Xu 
  Ilya Dryomov 
  Javier Carrasco 
  Jeff Layton 
  Johannes Berg 
  Josh Poimboeuf 
  Kent Overstreet 
  Klara Modin 
  Konstantin Komarov 
  Krzysztof Kozlowski 
  

Re: [XEN PATCH v4 1/3] x86/intel: move vmce_has_lmce() routine to header

[Jan Beulich]

On 22.05.2024 10:40, Sergiy Kibrik wrote:
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -24,6 +24,8 @@
>  
>  #include 
>  
> +#include "cpu/mcheck/mce.h"

Considering that I asked about this before, I'm a little irritated
that this is is entirely unadorned. Such an unusual #include wants
explaining some, you'll find similar comments elsewhere:

#include "cpu/mcheck/mce.h" /* for vmce_has_lmce() */

With that, which could also be adjusted while committing,
Acked-by: Jan Beulich 

Jan

Re: [XEN PATCH 2/4] automation/eclair_analysis: avoid an ECLAIR warning about escaping

[Jan Beulich]

On 27.05.2024 16:53, Nicola Vetrini wrote:
> The parentheses in this regular expression should be doubly
> escaped because they are undergo escaping twice.

Do you maybe mean "undergo expansion twice"?

Jan

> Signed-off-by: Nicola Vetrini 
> ---
>  automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
> b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index b9b377c56b25..cf62a874d928 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -405,8 +405,8 @@ explicit comment indicating the fallthrough intention is 
> present."
>  #
>  
>  -doc_begin="printf()-like functions are allowed to use the variadic features 
> provided by stdarg.h."
> --config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(^.*printk\(.*\)$)))"}
> --config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(^.*printf\(.*\)$)))"}
> +-config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(^.*printk\\(.*\\)$)))"}
> +-config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(^.*printf\\(.*\\)$)))"}
>  
> -config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(name(panic)&&kind(function"}
>  
> -config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(name(elf_call_log_callback)&&kind(function"}
>  
> -config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(name(vprintk_common)&&kind(function"}

Re: [XEN PATCH 1/4] docs/misra: exclude gdbsx from MISRA compliance

[Jan Beulich]

On 27.05.2024 16:53, Nicola Vetrini wrote:
> These files are used when debugging Xen, and are not meant to comply
> with MISRA rules at the moment.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini 

Acked-by: Jan Beulich 

Re: Code freeze date for Xen 4.19 is 31.05.2024

[Jan Beulich]

On 27.05.2024 15:58, Oleksii K. wrote:
> I would like to remind you that the code freeze date for Xen 4.19 is
> May 31, 2024.

I may be confused: With feature freeze having been last Friday aiui, isn't
this a little too short a period? I was actually expecting a longer-than-
normal one to account for the Xen Summit week.

Jan

> I'm okay with bug fixes being committed without my release ack (just CC
> me), except in cases where a one of maintainers gives a strong NACK.
> 
> Have a nice week!
> 
> Best regards,
> Oleksii

[XEN PATCH 1/4] docs/misra: exclude gdbsx from MISRA compliance

[Nicola Vetrini]

These files are used when debugging Xen, and are not meant to comply
with MISRA rules at the moment.

No functional change.

Signed-off-by: Nicola Vetrini 
---
 docs/misra/exclude-list.json | 8 
 1 file changed, 8 insertions(+)

diff --git a/docs/misra/exclude-list.json b/docs/misra/exclude-list.json
index cd6976542793..2567e8467c78 100644
--- a/docs/misra/exclude-list.json
+++ b/docs/misra/exclude-list.json
@@ -77,6 +77,14 @@
 "rel_path": "arch/x86/dmi_scan.c",
 "comment": "Imported from Linux, ignore for now"
 },
+{
+"rel_path": "arch/x86/gdbsx.c",
+"comment": "Used for debugging Xen, ignore for now"
+},
+{
+"rel_path": "arch/x86/include/asm/gdbsx.h",
+"comment": "Used for debugging Xen, ignore for now"
+},
 {
 "rel_path": "arch/x86/mpparse.c",
 "comment": "Imported from Linux, ignore for now"
-- 
2.34.1

[XEN PATCH 2/4] automation/eclair_analysis: avoid an ECLAIR warning about escaping

[Nicola Vetrini]

The parentheses in this regular expression should be doubly
escaped because they are undergo escaping twice.

Signed-off-by: Nicola Vetrini 
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
b/automation/eclair_analysis/ECLAIR/deviations.ecl
index b9b377c56b25..cf62a874d928 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -405,8 +405,8 @@ explicit comment indicating the fallthrough intention is 
present."
 #
 
 -doc_begin="printf()-like functions are allowed to use the variadic features 
provided by stdarg.h."
--config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(^.*printk\(.*\)$)))"}
--config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(^.*printf\(.*\)$)))"}
+-config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(^.*printk\\(.*\\)$)))"}
+-config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(^.*printf\\(.*\\)$)))"}
 
-config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(name(panic)&&kind(function"}
 
-config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(name(elf_call_log_callback)&&kind(function"}
 
-config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(name(vprintk_common)&&kind(function"}
-- 
2.34.1

[XEN PATCH 0/4] various ECLAIR and MISRA improvements

[Nicola Vetrini]

Hi all,

this series contains various miscellaneous changes to the ECLAIR and deviations
for MISRA rules

Nicola Vetrini (4):
  docs/misra: exclude gdbsx from MISRA compliance
  automation/eclair_analysis: avoid an ECLAIR warning about escaping
  x86: address violations of MISRA C Rule 8.4
  x86/traps: address violation of MISRA C Rule 8.4

 automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++--
 docs/misra/exclude-list.json | 8 
 xen/arch/x86/desc.c  | 1 +
 xen/arch/x86/mm.c| 2 +-
 xen/arch/x86/traps.c | 1 +
 5 files changed, 13 insertions(+), 3 deletions(-)

-- 
2.34.1

[XEN PATCH 4/4] x86/traps: address violation of MISRA C Rule 8.4

[Nicola Vetrini]

Rule 8.4 states: "A compatible declaration shall be visible when
an object or function with external linkage is defined".

The function do_general_protection is either used is asm code
or only within this unit, so there is no risk of this getting
out of sync with its definition, but the function must remain
extern.

Therefore, this function is deviated using a comment-based deviation.
No functional change.

Signed-off-by: Nicola Vetrini 
---
 xen/arch/x86/traps.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 468a03608102..9906e874d593 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1257,6 +1257,7 @@ void asmlinkage do_int3(struct cpu_user_regs *regs)
 pv_inject_hw_exception(X86_EXC_BP, X86_EVENT_NO_EC);
 }
 
+/* SAF-1-safe */
 void do_general_protection(struct cpu_user_regs *regs)
 {
 #ifdef CONFIG_PV
-- 
2.34.1

[XEN PATCH 3/4] x86: address violations of MISRA C Rule 8.4

[Nicola Vetrini]

Rule 8.4 states: "A compatible declaration shall be visible when
an object or function with external linkage is defined."

These variables are only referenced from asm modules, so they
need to be extern and there is negligible risk of them being
used improperly without noticing.

As a result, they can be exempted using a comment-based deviation.
No functional change.

Signed-off-by: Nicola Vetrini 
---
Adding the asmlinkage macro to variables is not appropriate, as
this pseudo-attribute may expand, for instance, to a different calling
convention in the future (e.g. stdcall)
---
 xen/arch/x86/desc.c | 1 +
 xen/arch/x86/mm.c   | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/desc.c b/xen/arch/x86/desc.c
index 39080ca67211..9f639281540a 100644
--- a/xen/arch/x86/desc.c
+++ b/xen/arch/x86/desc.c
@@ -91,6 +91,7 @@ seg_desc_t boot_compat_gdt[PAGE_SIZE / sizeof(seg_desc_t)] =
  * References boot_cpu_gdt_table for a short period, until the CPUs switch
  * onto their per-CPU GDTs.
  */
+/* SAF-1-safe */
 const struct desc_ptr boot_gdtr = {
 .limit = LAST_RESERVED_GDT_BYTE,
 .base = (unsigned long)(boot_gdt - FIRST_RESERVED_GDT_ENTRY),
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d968bbbc7315..17987eb5199e 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -144,7 +144,7 @@
 l1_pgentry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
 l1_fixmap[L1_PAGETABLE_ENTRIES];
 l1_pgentry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
-l1_fixmap_x[L1_PAGETABLE_ENTRIES];
+l1_fixmap_x[L1_PAGETABLE_ENTRIES]; /* SAF-1-safe */
 
 bool __read_mostly machine_to_phys_mapping_valid;
 
-- 
2.34.1

Re: [PATCH v3 2/4] xen/arm: Alloc XenStore page for Dom0less DomUs from hypervisor

[Jürgen Groß]

On 25.05.24 01:19, Stefano Stabellini wrote:

On Fri, 24 May 2024, Jürgen Groß wrote:

On 24.05.24 15:58, Julien Grall wrote:

Hi Henry,

+ Juergen as the Xenstore maintainers. I'd like his opinion on the approach.
The documentation of the new logic is in:

https://lore.kernel.org/xen-devel/20240517032156.1490515-5-xin.wa...@amd.com/

FWIW I am happy in principle with the logic (this is what we discussed on
the call last week). Some comments below.


I'm not against this logic, but I'm wondering why it needs to be so
complicated.


Actually the reason I like it is that in my view, this is the simplest
approach. You allocate a domain, you also allocate the xenstore page
together with it. Initially the xenstore connection has an
"uninitialized" state, as it should be. That's it. At some point, when
xenstored is ready, the state changes to CONNECTED.



Can't the domU itself allocate the Xenstore page from its RAM pages,
write the PFN into the Xenstore grant tab entry, and then make it
public via setting HVM_PARAM_STORE_PFN?


This is not simpler in my view


Okay, fine with me. I had the impression that violating the 1:1 mapping
of the domain would add complexity, but if you are fine with that I don't
mind your approach.


Juergen

Re: [PATCH v4 2/4] xen/arm: Alloc XenStore page for Dom0less DomUs from hypervisor

[Oleksii K.]

On Sat, 2024-05-25 at 00:06 +0100, Julien Grall wrote:
> Hi Stefano,
> 
> You have sent a new version. But you didn't reply to Juergen's
> comment.
> 
> While he is not the maintainer of the Arm code, he is the Xenstore 
> maintainer. Even if I agree with this approach I don't feel this is 
> right to even ack this patch without pending questions addressed.
> 
> In this case, we are changing yet another time the sequence for 
> initializing Xenstore dom0less domain. I would rather not want to
> have 
> to change a 4th time...
> 
> I don't think it is a big problem if this is not merged for the code 
> freeze as this is technically a bug fix.
Agree, this is not a problem as it is still looks to me as a bug fix.

~ Oleksii

> 
> Cheers,
> 
> On 24/05/2024 23:55, Stefano Stabellini wrote:
> > From: Henry Wang 
> > 
> > There are use cases (for example using the PV driver) in Dom0less
> > setup that require Dom0less DomUs start immediately with Dom0, but
> > initialize XenStore later after Dom0's successful boot and call to
> > the init-dom0less application.
> > 
> > An error message can seen from the init-dom0less application on
> > 1:1 direct-mapped domains:
> > ```
> > Allocating magic pages
> > memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
> > Error on alloc magic pages
> > ```
> > 
> > The "magic page" is a terminology used in the toolstack as reserved
> > pages for the VM to have access to virtual platform capabilities.
> > Currently the magic pages for Dom0less DomUs are populated by the
> > init-dom0less app through populate_physmap(), and
> > populate_physmap()
> > automatically assumes gfn == mfn for 1:1 direct mapped domains.
> > This
> > cannot be true for the magic pages that are allocated later from
> > the
> > init-dom0less application executed in Dom0. For domain using
> > statically
> > allocated memory but not 1:1 direct-mapped, similar error "failed
> > to
> > retrieve a reserved page" can be seen as the reserved memory list
> > is
> > empty at that time.
> > 
> > Since for init-dom0less, the magic page region is only for
> > XenStore.
> > To solve above issue, this commit allocates the XenStore page for
> > Dom0less DomUs at the domain construction time. The PFN will be
> > noted and communicated to the init-dom0less application executed
> > from Dom0. To keep the XenStore late init protocol, set the
> > connection
> > status to XENSTORE_RECONNECT.
> > 
> > Reported-by: Alec Kwapis 
> > Suggested-by: Daniel P. Smith 
> > Signed-off-by: Henry Wang 
> > Signed-off-by: Stefano Stabellini 
> > ---
> >   xen/arch/arm/dom0less-build.c | 55
> > ++-
> >   1 file changed, 54 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-
> > build.c
> > index 74f053c242..2963ecc0b3 100644
> > --- a/xen/arch/arm/dom0less-build.c
> > +++ b/xen/arch/arm/dom0less-build.c
> > @@ -1,5 +1,6 @@
> >   /* SPDX-License-Identifier: GPL-2.0-only */
> >   #include 
> > +#include 
> >   #include 
> >   #include 
> >   #include 
> > @@ -10,6 +11,8 @@
> >   #include 
> >   #include 
> >   
> > +#include 
> > +
> >   #include 
> >   #include 
> >   #include 
> > @@ -739,6 +742,53 @@ static int __init alloc_xenstore_evtchn(struct
> > domain *d)
> >   return 0;
> >   }
> >   
> > +#define XENSTORE_PFN_OFFSET 1
> > +static int __init alloc_xenstore_page(struct domain *d)
> > +{
> > +    struct page_info *xenstore_pg;
> > +    struct xenstore_domain_interface *interface;
> > +    mfn_t mfn;
> > +    gfn_t gfn;
> > +    int rc;
> > +
> > +    if ( (UINT_MAX - d->max_pages) < 1 )
> > +    {
> > +    printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages
> > by 1 page.\n",
> > +   d);
> > +    return -EINVAL;
> > +    }
> > +    d->max_pages += 1;
> > +    xenstore_pg = alloc_domheap_page(d, MEMF_bits(32));
> > +    if ( xenstore_pg == NULL && is_64bit_domain(d) )
> > +    xenstore_pg = alloc_domheap_page(d, 0);
> > +    if ( xenstore_pg == NULL )
> > +    return -ENOMEM;
> > +
> > +    mfn = page_to_mfn(xenstore_pg);
> > +    if ( !mfn_x(mfn) )
> > +    return -ENOMEM;
> > +
> > +    if ( !is_domain_direct_mapped(d) )
> > +    gfn = gaddr_to_gfn(GUEST_MAGIC_BASE +
> > +   (XENSTORE_PFN_OFFSET << PAGE_SHIFT));
> > +    else
> > +    gfn = gaddr_to_gfn(mfn_to_maddr(mfn));
> > +
> > +    rc = guest_physmap_add_page(d, gfn, mfn, 0);
> > +    if ( rc )
> > +    {
> > +    free_domheap_page(xenstore_pg);
> > +    return rc;
> > +    }
> > +
> > +    d->arch.hvm.params[HVM_PARAM_STORE_PFN] = gfn_x(gfn);
> > +    interface = map_domain_page(mfn);
> > +    interface->connection = XENSTORE_RECONNECT;
> > +    unmap_domain_page(interface);
> > +
> > +    return 0;
> > +}
> > +
> >   static int __init construct_domU(struct domain *d,
> >    const struct dt_device_node
> > *node)
> >   {
> > @@ -839,7 +889,10 @@ static int __init construct_domU(struc

Code freeze date for Xen 4.19 is 31.05.2024

[Oleksii K.]

Hi all,

I would like to remind you that the code freeze date for Xen 4.19 is
May 31, 2024.

I'm okay with bug fixes being committed without my release ack (just CC
me), except in cases where a one of maintainers gives a strong NACK.

Have a nice week!

Best regards,
Oleksii

Re: [PATCH v2 for-4.19 00/13] xen/bitops: Untangle ffs()/fls() infrastructure

[Oleksii K.]

I think we can consider to have this patch series in Xen 4.19 release:
 Release-acked-by: Oleksii Kurochko  

~ Oleksii
On Fri, 2024-05-24 at 21:03 +0100, Andrew Cooper wrote:
> bitops.h is a mess.  It has grown organtically over many years, and
> forces
> unreasonable repsonsibilities out into the per-arch stubs.
> 
> Start cleaning it up with ffs() and friends.  Across the board, this
> adds:
> 
>  * Functioning bitops without arch-specific asm
>  * An option for arches to provide more optimal code generation
>  * Compile-time constant folding
>  * Testing at both compile time and during init that the basic
> operations
>    behave according to spec.
> 
> and the only reason this series isn't a net reduction in code alone
> is the
> because of the new unit testing.
> 
> This form is superior in many ways, including getting RISC-V support
> for free.
> 
> v2:
>  * Many changes.  See patches for details
>  * Include the fls() side of the infrastructure too.
> 
> Testing:
>  
> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1304664544
>   https://cirrus-ci.com/github/andyhhp/xen/
> 
> Series-wide net bloat-o-meter:
> 
>   x86:   up/down: 51/-247 (-196)
>   ARM64: up/down: 40/-400 (-360)
> 
> and PPC64 reproduced in full, just to demonstrate how absurd it was
> to have
> generic_f?s() as static inlines...
> 
>   add/remove: 1/0 grow/shrink: 1/11 up/down: 228/-4832 (-4604)
>   Function old new   delta
>   init_constructors  - 220    +220
>   start_xen 92 100  +8
>   alloc_heap_pages    1980    1744    -236
>   xenheap_max_mfn  360 120    -240
>   free_heap_pages  784 536    -248
>   find_next_zero_bit   564 276    -288
>   find_next_bit    548 260    -288
>   find_first_zero_bit  444 148    -296
>   find_first_bit   444 132    -312
>   xmem_pool_free  1776    1440    -336
>   __do_softirq 604 252    -352
>   init_heap_pages 2328    1416    -912
>   xmem_pool_alloc 2920    1596   -1324
> 
> 
> Andrew Cooper (12):
>   ppc/boot: Run constructors on boot
>   xen/bitops: Cleanup ahead of rearrangements
>   ARM/bitops: Change find_first_set_bit() to be a define
>   xen/page_alloc: Coerce min(flsl(), foo) expressions to being
> unsigned
>   xen/bitops: Implement generic_f?sl() in lib/
>   xen/bitops: Implement ffs() in common logic
>   x86/bitops: Improve arch_ffs() in the general case
>   xen/bitops: Implement ffsl() in common logic
>   xen/bitops: Replace find_first_set_bit() with ffsl() - 1
>   xen/bitops: Delete find_first_set_bit()
>   xen/bitops: Clean up ffs64()/fls64() definitions
>   xen/bitops: Rearrange the top of xen/bitops.h
> 
> Oleksii Kurochko (1):
>   xen/bitops: Implement fls()/flsl() in common logic
> 
>  xen/arch/arm/include/asm/arm32/bitops.h  |   2 -
>  xen/arch/arm/include/asm/arm64/bitops.h  |  12 --
>  xen/arch/arm/include/asm/bitops.h    |  35 +---
>  xen/arch/ppc/include/asm/bitops.h    |  17 +-
>  xen/arch/ppc/setup.c |   2 +
>  xen/arch/x86/guest/xen/xen.c |   4 +-
>  xen/arch/x86/hvm/dom0_build.c    |   2 +-
>  xen/arch/x86/hvm/hpet.c  |   8 +-
>  xen/arch/x86/include/asm/bitops.h    | 114 +++--
>  xen/arch/x86/include/asm/pt-contig-markers.h |   2 +-
>  xen/arch/x86/mm.c    |   2 +-
>  xen/arch/x86/mm/p2m-pod.c    |   4 +-
>  xen/common/Makefile  |   1 +
>  xen/common/bitops.c  |  89 +++
>  xen/common/page_alloc.c  |   6 +-
>  xen/common/softirq.c |   2 +-
>  xen/drivers/passthrough/amd/iommu_map.c  |   2 +-
>  xen/drivers/passthrough/iommu.c  |   4 +-
>  xen/drivers/passthrough/x86/iommu.c  |   4 +-
>  xen/include/xen/bitops.h | 159 -
> --
>  xen/include/xen/boot-check.h |  60 +++
>  xen/include/xen/compiler.h   |   3 +-
>  xen/lib/Makefile |   2 +
>  xen/lib/generic-ffsl.c   |  65 
>  xen/lib/generic-flsl.c   |  68 
>  25 files changed, 444 insertions(+), 225 deletions(-)
>  create mode 100644 xen/common/bitops.c
>  create mode 100644 xen/include/xen/boot-check.h
>  create mode 100644 xen/lib/generic-ffsl.c
>  create mode 100644 xen/lib/generic-flsl.c
> 

Re: [PATCH v2 13/13] xen/bitops: Rearrange the top of xen/bitops.h

[Jan Beulich]

On 24.05.2024 22:03, Andrew Cooper wrote:
> The #include  can move to the top of the file now now that
> generic_f?s() have been untangled.
> 
> Signed-off-by: Andrew Cooper 

Acked-by: Jan Beulich 

Re: [PATCH v2 12/13] xen/bitops: Clean up ffs64()/fls64() definitions

[Jan Beulich]

On 24.05.2024 22:03, Andrew Cooper wrote:
> Implement ffs64() and fls64() as plain static inlines, dropping the ifdefary
> and intermediate generic_f?s64() forms.
> 
> Add tests for all interesting bit positions at 32bit boundaries.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 
with two remarks:

> --- a/xen/common/bitops.c
> +++ b/xen/common/bitops.c
> @@ -24,6 +24,22 @@ static void __init test_ffs(void)
>  CHECK(ffsl, 1UL << 32, 33);
>  CHECK(ffsl, 1UL << 63, 64);
>  #endif
> +
> +/*
> + * unsigned int ffs64(uint64_t)
> + *
> + * 32-bit builds of Xen have to split this into two adjacent operations,
> + * so test all interesting bit positions across the divide.
> + */
> +CHECK(ffs64, 0, 0);
> +CHECK(ffs64, 1, 1);
> +CHECK(ffs64, 3, 1);
> +CHECK(ffs64, 7, 1);
> +CHECK(ffs64, 6, 2);
> +
> +CHECK(ffs64, 0x80008000ULL, 32);
> +CHECK(ffs64, 0x8001ULL, 33);
> +CHECK(ffs64, 0x8000ULL, 64);

With the intermediate blank line, the respective part of the comment doesn't
look to be related to these 3 lines. Could I talk you into moving that part
down?

> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -60,6 +60,14 @@ static always_inline __pure unsigned int ffsl(unsigned 
> long x)
>  #endif
>  }
>  
> +static always_inline __pure unsigned int ffs64(uint64_t x)
> +{
> +if ( BITS_PER_LONG == 64 )

In principle >= 64 would be okay here, and hence I'd prefer if we used that
less strict form. Yet I'm not going to insist.

Jan

Re: [PATCH v2 11/13] xen/bitops: Implement fls()/flsl() in common logic

[Jan Beulich]

On 24.05.2024 22:03, Andrew Cooper wrote:
> From: Oleksii Kurochko 
> 
> This is most easily done together because of how arm32 is currently
> structured, but it does just mirror the existing ffs()/ffsl() work.
> 
> Introduce compile and boot time testing.
> 
> Signed-off-by: Oleksii Kurochko 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 
with small adjustments possibly to be done on the earlier similar patches
also done here.

Jan

Re: [PATCH v2 07/13] x86/bitops: Improve arch_ffs() in the general case

[Jan Beulich]

On 27.05.2024 15:27, Jan Beulich wrote:
> On 24.05.2024 22:03, Andrew Cooper wrote:
>> --- a/xen/arch/x86/include/asm/bitops.h
>> +++ b/xen/arch/x86/include/asm/bitops.h
>> @@ -432,12 +432,28 @@ static inline int ffsl(unsigned long x)
>>  
>>  static always_inline unsigned int arch_ffs(unsigned int x)
>>  {
>> -int r;
>> +unsigned int r;
>> +
>> +if ( __builtin_constant_p(x > 0) && x > 0 )
>> +{
>> +/* Safe, when the compiler knows that x is nonzero. */
>> +asm ( "bsf %[val], %[res]"
>> +  : [res] "=r" (r)
>> +  : [val] "rm" (x) );
>> +}
> 
> In patch 11 relevant things are all in a single patch, making it easier
> to spot that this is dead code: The sole caller already has a
> __builtin_constant_p(), hence I don't see how the one here could ever
> return true. With that the respective part of the description is then
> questionable, too, I'm afraid: Where did you observe any actual effect
> from this? Or if you did - what am I missing?

Hmm, thinking about it: I suppose that's why you have
__builtin_constant_p(x > 0), not __builtin_constant_p(x). I have to admit
I'm (positively) surprised that the former may return true when the latter
doesn't. Nevertheless I'm inclined to think this deserves a brief comment.

As an aside, to better match the comment inside the if()'s body, how about

if ( __builtin_constant_p(!!x) && x )

? That also may make a little more clear that this isn't just a style
choice, but actually needed for the intended purpose.

Jan

Re: [PATCH v2 07/13] x86/bitops: Improve arch_ffs() in the general case

[Jan Beulich]

On 24.05.2024 22:03, Andrew Cooper wrote:
> --- a/xen/arch/x86/include/asm/bitops.h
> +++ b/xen/arch/x86/include/asm/bitops.h
> @@ -432,12 +432,28 @@ static inline int ffsl(unsigned long x)
>  
>  static always_inline unsigned int arch_ffs(unsigned int x)
>  {
> -int r;
> +unsigned int r;
> +
> +if ( __builtin_constant_p(x > 0) && x > 0 )
> +{
> +/* Safe, when the compiler knows that x is nonzero. */
> +asm ( "bsf %[val], %[res]"
> +  : [res] "=r" (r)
> +  : [val] "rm" (x) );
> +}

In patch 11 relevant things are all in a single patch, making it easier
to spot that this is dead code: The sole caller already has a
__builtin_constant_p(), hence I don't see how the one here could ever
return true. With that the respective part of the description is then
questionable, too, I'm afraid: Where did you observe any actual effect
from this? Or if you did - what am I missing?

Jan

[PATCH] loadpolicy: Verifies memory allocation during policy loading

[yskelg]

From: Yunseong Kim 

memory allocation failure handling in the loadpolicy module.

Signed-off-by: Yunseong Kim 
---
 tools/flask/utils/loadpolicy.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/tools/flask/utils/loadpolicy.c b/tools/flask/utils/loadpolicy.c
index 76710a256c..7f6bab4dcd 100644
--- a/tools/flask/utils/loadpolicy.c
+++ b/tools/flask/utils/loadpolicy.c
@@ -58,6 +58,11 @@ int main (int argCnt, const char *args[])
 }
 
 polMemCp = malloc(info.st_size);
+if (!polMemCp) {
+fprintf(stderr, "Error occurred allocating %ld bytes\n", info.st_size);
+ret = -ENOMEM;
+goto cleanup;
+}
 
 #ifdef USE_MMAP
 polMem = mmap(NULL, info.st_size, PROT_READ, MAP_SHARED, polFd, 0);
-- 
2.34.1

[PATCH 3/4] x86/pci/xen: Fix PCIBIOS_* return code handling

[Ilpo Järvinen]

xen_pcifront_enable_irq() uses pci_read_config_byte() that returns
PCIBIOS_* codes. The error handling, however, assumes the codes are
normal errnos because it checks for < 0.

xen_pcifront_enable_irq() also returns the PCIBIOS_* code back to the
caller but the function is used as the (*pcibios_enable_irq) function
which should return normal errnos.

Convert the error check to plain non-zero check which works for
PCIBIOS_* return codes and convert the PCIBIOS_* return code using
pcibios_err_to_errno() into normal errno before returning it.

Fixes: 3f2a230caf21 ("xen: handled remapped IRQs when enabling a pcifront PCI 
device.")
Signed-off-by: Ilpo Järvinen 
Cc: sta...@vger.kernel.org
---
 arch/x86/pci/xen.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 652cd53e77f6..0f2fe524f60d 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -38,10 +38,10 @@ static int xen_pcifront_enable_irq(struct pci_dev *dev)
u8 gsi;
 
rc = pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &gsi);
-   if (rc < 0) {
+   if (rc) {
dev_warn(&dev->dev, "Xen PCI: failed to read interrupt line: 
%d\n",
 rc);
-   return rc;
+   return pcibios_err_to_errno(rc);
}
/* In PV DomU the Xen PCI backend puts the PIRQ in the interrupt line.*/
pirq = gsi;
-- 
2.39.2

Re: [PATCH v2 10/13] xen/bitops: Delete find_first_set_bit()

[Jan Beulich]

On 24.05.2024 22:03, Andrew Cooper wrote:
> No more users.
> 
> Signed-off-by: Andrew Cooper 

Acked-by: Jan Beulich 

Re: [PATCH v2 09/13] xen/bitops: Replace find_first_set_bit() with ffsl() - 1

[Jan Beulich]

On 24.05.2024 22:03, Andrew Cooper wrote:
> --- a/xen/arch/x86/hvm/hpet.c
> +++ b/xen/arch/x86/hvm/hpet.c
> @@ -335,7 +335,7 @@ static void timer_sanitize_int_route(HPETState *h, 
> unsigned int tn)
>   * enabled pick the first irq.
>   */
>  timer_config(h, tn) |=
> -MASK_INSR(find_first_set_bit(timer_int_route_cap(h, tn)),
> +MASK_INSR(ffsl(timer_int_route_cap(h, tn)) - 1,
>HPET_TN_ROUTE);
>  }

This can be just ffs().

> @@ -409,7 +409,7 @@ static int cf_check hpet_write(
>  {
>  bool active;
>  
> -i = find_first_set_bit(new_val);
> +i = ffsl(new_val) - 1;
>  if ( i >= HPET_TIMER_NUM )
>  break;

This in principle can be, too, but would require a little further care.

> @@ -535,14 +535,14 @@ static int cf_check hpet_write(
>  /* stop/start timers whos state was changed by this write. */
>  while (stop_timers)
>  {
> -i = find_first_set_bit(stop_timers);
> +i = ffsl(stop_timers) - 1;
>  __clear_bit(i, &stop_timers);
>  hpet_stop_timer(h, i, guest_time);
>  }
>  
>  while (start_timers)
>  {
> -i = find_first_set_bit(start_timers);
> +i = ffsl(start_timers) - 1;
>  __clear_bit(i, &start_timers);
>  hpet_set_timer(h, i, guest_time);
>  }

Same here; in fact {start,stop}_timers are needlessly unsigned long in
the first place.

> --- a/xen/arch/x86/include/asm/pt-contig-markers.h
> +++ b/xen/arch/x86/include/asm/pt-contig-markers.h
> @@ -60,7 +60,7 @@ static bool pt_update_contig_markers(uint64_t *pt, unsigned 
> int idx,
>  /* Step 1: Reduce markers in lower numbered entries. */
>  while ( i )
>  {
> -b = find_first_set_bit(i);
> +b = ffsl(i) - 1;
>  i &= ~(1U << b);

Considering i's type and the immediately following expression, this again
can easily be just ffs().

> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -137,7 +137,7 @@ static void set_iommu_ptes_present(unsigned long pt_mfn,
>  ASSERT(!pde->u);
>  
>  if ( pde > table )
> -ASSERT(pde->ign0 == find_first_set_bit(pde - table));
> +ASSERT(pde->ign0 == ffsl(pde - table) - 1);

pde pointing into the page starting at table, this can be ffs(), too.

Preferably with at least the easy adjustments done:
Reviewed-by: Jan Beulich 

Jan

Re: [PATCH v2 08/13] xen/bitops: Implement ffsl() in common logic

[Jan Beulich]

On 24.05.2024 22:03, Andrew Cooper wrote:
> Just like ffs() in the previous changes.  Express the upper bound of the
> testing in terms of BITS_PER_LONG as it varies between architectures.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 

> @@ -458,6 +441,24 @@ static always_inline unsigned int arch_ffs(unsigned int 
> x)
>  }
>  #define arch_ffs arch_ffs
>  
> +static always_inline unsigned int arch_ffsl(unsigned long x)
> +{
> +unsigned int r;
> +
> +/* See arch_ffs() for safety discussions. */
> +if ( __builtin_constant_p(x > 0) && x > 0 )

See remark on arch_ffs() for possible slight reduction of code.

Jan

Re: [PATCH v2 07/13] x86/bitops: Improve arch_ffs() in the general case

[Jan Beulich]

On 24.05.2024 22:03, Andrew Cooper wrote:
> The asm in arch_ffs() is safe but inefficient.
> 
> CMOV would be an improvement over a conditional branch, but for 64bit CPUs
> both Intel and AMD have provided enough details about the behaviour for a zero
> input.  It is safe to pre-load the destination register with -1 and drop the
> conditional logic.
> 
> However, it is common to find ffs() in a context where the optimiser knows
> that x in nonzero even if it the value isn't known precisely, and in that case
> it's safe to drop the preload of -1 too.
> 
> There are only a handful of uses of ffs() in the x86 build, and all of them
> improve as a result of this:
> 
>   add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-31 (-31)
>   Function old new   delta
>   mask_write   114 107  -7
>   xmem_pool_alloc 10631039 -24
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 
with one suggestion:

> --- a/xen/arch/x86/include/asm/bitops.h
> +++ b/xen/arch/x86/include/asm/bitops.h
> @@ -432,12 +432,28 @@ static inline int ffsl(unsigned long x)
>  
>  static always_inline unsigned int arch_ffs(unsigned int x)
>  {
> -int r;
> +unsigned int r;
> +
> +if ( __builtin_constant_p(x > 0) && x > 0 )

__builtin_constant_p(x) surely will do. In fact even the other "> 0" could
in principle be left out here.

Jan

Re: [PATCH v2 06/13] xen/bitops: Implement ffs() in common logic

[Jan Beulich]

On 24.05.2024 22:03, Andrew Cooper wrote:
> Perform constant-folding unconditionally, rather than having it implemented
> inconsistency between architectures.
> 
> Confirm the expected behaviour with compile time and boot time tests.
> 
> For non-constant inputs, use arch_ffs() if provided but fall back to
> generic_ffsl() if not.  In particular, RISC-V doesn't have a builtin that
> works in all configurations.
> 
> For x86, rename ffs() to arch_ffs() and adjust the prototype.
> 
> For PPC, __builtin_ctz() is 1/3 of the size of size of the transform to
> generic_fls().  Drop the definition entirely.  ARM too benefits in the general
> case by using __builtin_ctz(), but less dramatically because it using
> optimised asm().
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 

[ovmf test] 186160: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf 08281572aab5b1f7e05bf26de4148af19eddc8b7
baseline version:
 ovmf 88a4de450f17c6a319c3e8b2135cd7068a07d0f8

Last test of basis   186158  2024-05-27 02:13:38 Z0 days
Testing same since   186160  2024-05-27 09:43:09 Z0 days1 attempts


People who touched revisions under test:
  Jiewen Yao 
  Wenxing Hou 

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



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
   88a4de450f..08281572aa  08281572aab5b1f7e05bf26de4148af19eddc8b7 -> 
xen-tested-master

Re: CVE-2021-47377: kernel: xen/balloon: use a kernel thread instead a workqueue

[Juergen Gross]

Hi,

I'd like to dispute CVE-2021-47377: the issue fixed by upstream commit
8480ed9c2bbd56fc86524998e5f2e3e22f5038f6 can in no way be triggered by
an unprivileged user or by a remote attack of the system, as it requires
initiation of memory ballooning of the running system. This can be done
only by either a host admin or by an admin of the guest which might
suffer the detection of the hanging workqueue.

Please revoke this CVE.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [XEN PATCH v2 07/15] x86: guard cpu_has_{svm/vmx} macros with CONFIG_{SVM/VMX}

[Jan Beulich]

On 27.05.2024 12:27, Sergiy Kibrik wrote:
> 23.05.24 17:50, Jan Beulich:
>> On 23.05.2024 15:07, Sergiy Kibrik wrote:
>>> 16.05.24 14:12, Jan Beulich:
 On 15.05.2024 11:12, Sergiy Kibrik wrote:
> --- a/xen/arch/x86/include/asm/cpufeature.h
> +++ b/xen/arch/x86/include/asm/cpufeature.h
> @@ -81,7 +81,8 @@ static inline bool boot_cpu_has(unsigned int feat)
>#define cpu_has_sse3boot_cpu_has(X86_FEATURE_SSE3)
>#define cpu_has_pclmulqdq   boot_cpu_has(X86_FEATURE_PCLMULQDQ)
>#define cpu_has_monitor boot_cpu_has(X86_FEATURE_MONITOR)
> -#define cpu_has_vmx boot_cpu_has(X86_FEATURE_VMX)
> +#define cpu_has_vmx ( IS_ENABLED(CONFIG_VMX) && \
> +  boot_cpu_has(X86_FEATURE_VMX))
>#define cpu_has_eistboot_cpu_has(X86_FEATURE_EIST)
>#define cpu_has_ssse3   boot_cpu_has(X86_FEATURE_SSSE3)
>#define cpu_has_fma boot_cpu_has(X86_FEATURE_FMA)
> @@ -109,7 +110,8 @@ static inline bool boot_cpu_has(unsigned int feat)
>
>/* CPUID level 0x8001.ecx */
>#define cpu_has_cmp_legacy  boot_cpu_has(X86_FEATURE_CMP_LEGACY)
> -#define cpu_has_svm boot_cpu_has(X86_FEATURE_SVM)
> +#define cpu_has_svm ( IS_ENABLED(CONFIG_SVM) && \
> +  boot_cpu_has(X86_FEATURE_SVM))
>#define cpu_has_sse4a   boot_cpu_has(X86_FEATURE_SSE4A)
>#define cpu_has_xop boot_cpu_has(X86_FEATURE_XOP)
>#define cpu_has_skinit  boot_cpu_has(X86_FEATURE_SKINIT)

 Hmm, leaving aside the style issue (stray blanks after opening parentheses,
 and as a result one-off indentation on the wrapped lines) I'm not really
 certain we can do this. The description goes into detail why we would want
 this, but it doesn't cover at all why it is safe for all present (and
 ideally also future) uses. I wouldn't be surprised if we had VMX/SVM checks
 just to derive further knowledge from that, without them being directly
 related to the use of VMX/SVM. Take a look at calculate_hvm_max_policy(),
 for example. While it looks to be okay there, it may give you an idea of
 what I mean.

 Things might become better separated if instead for such checks we used
 host and raw CPU policies instead of cpuinfo_x86.x86_capability[]. But
 that's still pretty far out, I'm afraid.
>>>
>>> I've followed a suggestion you made for patch in previous series:
>>>
> [..]
>>
>> See the "If not, ..." that I had put there. Doing the change just 
>> mechanically
>> isn't enough, you also need to make clear (in the description) that you
>> verified it's safe to have this way.
>>
>>> yet if this approach can potentially be unsafe (I'm not completely sure
>>> it's safe), should we instead fallback to the way it was done in v1
>>> series? I.e. guard calls to vmx/svm-specific calls where needed, like in
>>> these 3 patches:
>>>
> [..]
>>
>> I don't like this sprinkling around of IS_ENABLED() very much. Maybe we want
>> to have two new helpers (say using_svm() and using_vmx()), to be used in 
>> place
>> of most but possibly not all cpu_has_{svm,vmx}? Doing such a transformation
>> would then kind of implicitly answer the safety question above, as at every
>> use site you'd need to judge whether the replacement is correct. If it's
>> correct everywhere, the construct(s) as proposed in this version could then 
>> be
>> considered to be used in this very shape (instead of introducing the two new
>> helpers). But of course the transition could also be done gradually then,
>> touching only those uses that previously you touched in 1), 2), and 3).
>>
> 
> now I might be seeing your concerns, if I understood correctly, 
> situation is the following.
> 
>   As an example of cpu_has_vmx macro, it can be used to prove either of 
> following two statements: 1) VMX features can be used or 2) CPU provides 
> VMX features.
> Currently they're the same for Xen, yet after this patch series they're 
> not, as the situation possible when non-vmx build would be able to get 
> executed on vmx-enabled machine. E.g. the case of PV guest, or (if that 
> makes any sense) at least hypervisor's code is still able to run until 
> HVM guest has to be created. Changes in this patch makes 
> indistinguishable for a user whether VMX support is absent in code or in 
> hardware -- hence we may need two separate macros for these.
> 
> Still the question remains whether a separate macro to check if CPU 
> provides VMX/SVM is really needed at all at this point.
> 
> I've counted only 16 uses of cpu_has_vmx in the code, not that much to 
> check every one of them, so I did that.
> Most of uses are obviously checks before using vmx features, so logic 
> not broken.
> As for the others, the surrounding context presumes that HVM domain 
> required there or had already been

[xen-unstable test] 186157: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-armhf   6 xen-buildfail REGR. vs. 186145

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

version targeted for testing:
 xen  ac572152e578a8853de0534384c1539ec21f11e0
baseline version:
 xen  ac572152e578a8853de0534384c1539ec21f11e0

Last test of basis   186157  2024-05-27 01:53:44 Z0 days
Testing same since  (not found) 0 attempts

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64-xtf  pass
 build-amd64  pass
 build-arm64  pass
 build-armhf  fail
 build-i386   pass
 build-amd64-libvirt  pass
 build-arm64-libvirt  pass
 build-armhf-libvirt  blocked 
 build-i386-libvirt   pass
 build-amd64-prev pass
 build-i386-prev  pass
 build-amd64-pvopspass
 build-arm64-pvopspass
 build-armhf-pvopspass
 build-i386-pvops pass

Re: [XEN PATCH v2 07/15] x86: guard cpu_has_{svm/vmx} macros with CONFIG_{SVM/VMX}

[Sergiy Kibrik]

23.05.24 17:50, Jan Beulich:

On 23.05.2024 15:07, Sergiy Kibrik wrote:

16.05.24 14:12, Jan Beulich:

On 15.05.2024 11:12, Sergiy Kibrik wrote:

--- a/xen/arch/x86/include/asm/cpufeature.h
+++ b/xen/arch/x86/include/asm/cpufeature.h
@@ -81,7 +81,8 @@ static inline bool boot_cpu_has(unsigned int feat)
   #define cpu_has_sse3boot_cpu_has(X86_FEATURE_SSE3)
   #define cpu_has_pclmulqdq   boot_cpu_has(X86_FEATURE_PCLMULQDQ)
   #define cpu_has_monitor boot_cpu_has(X86_FEATURE_MONITOR)
-#define cpu_has_vmx boot_cpu_has(X86_FEATURE_VMX)
+#define cpu_has_vmx ( IS_ENABLED(CONFIG_VMX) && \
+  boot_cpu_has(X86_FEATURE_VMX))
   #define cpu_has_eistboot_cpu_has(X86_FEATURE_EIST)
   #define cpu_has_ssse3   boot_cpu_has(X86_FEATURE_SSSE3)
   #define cpu_has_fma boot_cpu_has(X86_FEATURE_FMA)
@@ -109,7 +110,8 @@ static inline bool boot_cpu_has(unsigned int feat)
   
   /* CPUID level 0x8001.ecx */

   #define cpu_has_cmp_legacy  boot_cpu_has(X86_FEATURE_CMP_LEGACY)
-#define cpu_has_svm boot_cpu_has(X86_FEATURE_SVM)
+#define cpu_has_svm ( IS_ENABLED(CONFIG_SVM) && \
+  boot_cpu_has(X86_FEATURE_SVM))
   #define cpu_has_sse4a   boot_cpu_has(X86_FEATURE_SSE4A)
   #define cpu_has_xop boot_cpu_has(X86_FEATURE_XOP)
   #define cpu_has_skinit  boot_cpu_has(X86_FEATURE_SKINIT)


Hmm, leaving aside the style issue (stray blanks after opening parentheses,
and as a result one-off indentation on the wrapped lines) I'm not really
certain we can do this. The description goes into detail why we would want
this, but it doesn't cover at all why it is safe for all present (and
ideally also future) uses. I wouldn't be surprised if we had VMX/SVM checks
just to derive further knowledge from that, without them being directly
related to the use of VMX/SVM. Take a look at calculate_hvm_max_policy(),
for example. While it looks to be okay there, it may give you an idea of
what I mean.

Things might become better separated if instead for such checks we used
host and raw CPU policies instead of cpuinfo_x86.x86_capability[]. But
that's still pretty far out, I'm afraid.


I've followed a suggestion you made for patch in previous series:


[..]


See the "If not, ..." that I had put there. Doing the change just mechanically
isn't enough, you also need to make clear (in the description) that you
verified it's safe to have this way.


yet if this approach can potentially be unsafe (I'm not completely sure
it's safe), should we instead fallback to the way it was done in v1
series? I.e. guard calls to vmx/svm-specific calls where needed, like in
these 3 patches:


[..]


I don't like this sprinkling around of IS_ENABLED() very much. Maybe we want
to have two new helpers (say using_svm() and using_vmx()), to be used in place
of most but possibly not all cpu_has_{svm,vmx}? Doing such a transformation
would then kind of implicitly answer the safety question above, as at every
use site you'd need to judge whether the replacement is correct. If it's
correct everywhere, the construct(s) as proposed in this version could then be
considered to be used in this very shape (instead of introducing the two new
helpers). But of course the transition could also be done gradually then,
touching only those uses that previously you touched in 1), 2), and 3).



now I might be seeing your concerns, if I understood correctly, 
situation is the following.


 As an example of cpu_has_vmx macro, it can be used to prove either of 
following two statements: 1) VMX features can be used or 2) CPU provides 
VMX features.
Currently they're the same for Xen, yet after this patch series they're 
not, as the situation possible when non-vmx build would be able to get 
executed on vmx-enabled machine. E.g. the case of PV guest, or (if that 
makes any sense) at least hypervisor's code is still able to run until 
HVM guest has to be created. Changes in this patch makes 
indistinguishable for a user whether VMX support is absent in code or in 
hardware -- hence we may need two separate macros for these.


Still the question remains whether a separate macro to check if CPU 
provides VMX/SVM is really needed at all at this point.


I've counted only 16 uses of cpu_has_vmx in the code, not that much to 
check every one of them, so I did that.
Most of uses are obviously checks before using vmx features, so logic 
not broken.
As for the others, the surrounding context presumes that HVM domain 
required there or had already been created. But non-vmx build can't 
create HVM VMX domain anyway, so the logic not broken either.


As for cpu_has_svm only 8 uses I've counted, all but one also don't seem 
to break logic as described above. One check of cpu_has_svm in 
init_speculation_mitigations(), where default speculation control flag 
gets set, not uses SVM features directly. Yet from the comment I can

Re: [PATCH v2 5/8] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves

[Roger Pau Monné]

On Fri, May 24, 2024 at 04:16:01PM +0100, Alejandro Vallejo wrote:
> On 23/05/2024 17:13, Roger Pau Monné wrote:
> > On Wed, May 08, 2024 at 01:39:24PM +0100, Alejandro Vallejo wrote:
> >> @@ -86,10 +113,11 @@ static void boot_cpu(unsigned int cpu)
> >>  BUG();
> >>  
> >>  /*
> >> - * Wait for the secondary processor to complete initialisation.
> >> + * Wait for the secondary processor to complete initialisation,
> >> + * which is signaled by its x2APIC ID being writted to the LUT.
> >>   * Do not touch shared resources meanwhile.
> >>   */
> >> -while ( !ap_callin )
> >> +while ( !ACCESS_ONCE(CPU_TO_X2APICID[cpu]) )
> >>  cpu_relax();
> > 
> > As a further improvement, we could launch all APs in pararell, and use
> > a for loop to wait until all positions of the CPU_TO_X2APICID array
> > are set.
> 
> I thought about it, but then we'd need locking for the prints as well,
> or refactor things so only the BSP prints on success.

Hm, I see, yes, we would likely need to refactor the printing a bit so
each AP only prints one line, and then add locking around the calls in
`cpu_setup()`.

> The time taken is truly negligible, so I reckon it's better left for
> another patch.

Oh, indeed, sorry if I made it look it should be part of this patch,
that wasn't my intention.  Just something that might be worth looking
into doing in the future.

Thanks, Roger.

Re: [PATCH v2 5/8] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves

[Roger Pau Monné]

On Fri, May 24, 2024 at 04:15:34PM +0100, Alejandro Vallejo wrote:
> On 24/05/2024 08:21, Roger Pau Monné wrote:
> > On Wed, May 08, 2024 at 01:39:24PM +0100, Alejandro Vallejo wrote:
> >> Make it so the APs expose their own APIC IDs in a LUT. We can use that LUT 
> >> to
> >> populate the MADT, decoupling the algorithm that relates CPU IDs and APIC 
> >> IDs
> >> from hvmloader.
> >>
> >> While at this also remove ap_callin, as writing the APIC ID may serve the 
> >> same
> >> purpose.
> >>
> >> Signed-off-by: Alejandro Vallejo 
> >> ---
> >> v2:
> >>   * New patch. Replaces adding cpu policy to hvmloader in v1.
> >> ---
> >>  tools/firmware/hvmloader/config.h|  6 -
> >>  tools/firmware/hvmloader/hvmloader.c |  4 +--
> >>  tools/firmware/hvmloader/smp.c   | 40 +++-
> >>  tools/firmware/hvmloader/util.h  |  5 
> >>  xen/arch/x86/include/asm/hvm/hvm.h   |  1 +
> >>  5 files changed, 47 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/tools/firmware/hvmloader/config.h 
> >> b/tools/firmware/hvmloader/config.h
> >> index c82adf6dc508..edf6fa9c908c 100644
> >> --- a/tools/firmware/hvmloader/config.h
> >> +++ b/tools/firmware/hvmloader/config.h
> >> @@ -4,6 +4,8 @@
> >>  #include 
> >>  #include 
> >>  
> >> +#include 
> >> +
> >>  enum virtual_vga { VGA_none, VGA_std, VGA_cirrus, VGA_pt };
> >>  extern enum virtual_vga virtual_vga;
> >>  
> >> @@ -49,8 +51,10 @@ extern uint8_t ioapic_version;
> >>  
> >>  #define IOAPIC_ID   0x01
> >>  
> >> +extern uint32_t CPU_TO_X2APICID[HVM_MAX_VCPUS];
> >> +
> >>  #define LAPIC_BASE_ADDRESS  0xfee0
> >> -#define LAPIC_ID(vcpu_id)   ((vcpu_id) * 2)
> >> +#define LAPIC_ID(vcpu_id)   (CPU_TO_X2APICID[(vcpu_id)])
> >>  
> >>  #define PCI_ISA_DEVFN   0x08/* dev 1, fn 0 */
> >>  #define PCI_ISA_IRQ_MASK0x0c20U /* ISA IRQs 5,10,11 are PCI connected 
> >> */
> >> diff --git a/tools/firmware/hvmloader/hvmloader.c 
> >> b/tools/firmware/hvmloader/hvmloader.c
> >> index c58841e5b556..1eba92229925 100644
> >> --- a/tools/firmware/hvmloader/hvmloader.c
> >> +++ b/tools/firmware/hvmloader/hvmloader.c
> >> @@ -342,11 +342,11 @@ int main(void)
> >>  
> >>  printf("CPU speed is %u MHz\n", get_cpu_mhz());
> >>  
> >> +smp_initialise();
> >> +
> >>  apic_setup();
> >>  pci_setup();
> >>  
> >> -smp_initialise();
> >> -
> >>  perform_tests();
> >>  
> >>  if ( bios->bios_info_setup )
> >> diff --git a/tools/firmware/hvmloader/smp.c 
> >> b/tools/firmware/hvmloader/smp.c
> >> index a668f15d7e1f..4d75f239c2f5 100644
> >> --- a/tools/firmware/hvmloader/smp.c
> >> +++ b/tools/firmware/hvmloader/smp.c
> >> @@ -29,7 +29,34 @@
> >>  
> >>  #include 
> >>  
> >> -static int ap_callin, ap_cpuid;
> >> +static int ap_cpuid;
> >> +
> >> +/**
> >> + * Lookup table of x2APIC IDs.
> >> + *
> >> + * Each entry is populated its respective CPU as they come online. This 
> >> is required
> >> + * for generating the MADT with minimal assumptions about ID 
> >> relationships.
> >> + */
> >> +uint32_t CPU_TO_X2APICID[HVM_MAX_VCPUS];
> >> +
> >> +static uint32_t read_apic_id(void)
> >> +{
> >> +uint32_t apic_id;
> >> +
> >> +cpuid(1, NULL, &apic_id, NULL, NULL);
> >> +apic_id >>= 24;
> >> +
> >> +/*
> >> + * APIC IDs over 255 are represented by 255 in leaf 1 and are meant 
> >> to be
> >> + * read from topology leaves instead. Xen exposes x2APIC IDs in leaf 
> >> 0xb,
> >> + * but only if the x2APIC feature is present. If there are that many 
> >> CPUs
> >> + * it's guaranteed to be there so we can avoid checking for it 
> >> specifically
> >> + */
> >> +if ( apic_id == 255 )
> >> +cpuid(0xb, NULL, NULL, NULL, &apic_id);
> >> +
> >> +return apic_id;
> >> +}
> >>  
> >>  static void ap_start(void)
> >>  {
> >> @@ -37,12 +64,12 @@ static void ap_start(void)
> >>  cacheattr_init();
> >>  printf("done.\n");
> >>  
> >> +wmb();
> >> +ACCESS_ONCE(CPU_TO_X2APICID[ap_cpuid]) = read_apic_id();
> > 
> > Further thinking about this: do we really need the wmb(), given the
> > usage of ACCESS_ONCE()?
> > 
> > wmb() is a compiler barrier, and the usage of volatile in
> > ACCESS_ONCE() should already prevent any compiler re-ordering.
> > 
> > Thanks, Roger.
> 
> volatile reads/writes cannot be reordered with other volatile
> reads/writes, but volatile reads/writes can be reordered with
> non-volatile reads/writes, AFAIR.

Right, I've read the C99 spec and it does indeed only guarantee the
state of volatile objects to be as expected at sequence points.  I
think however this specific code would still be fine since the
function calls are considered to have side-effects, and those must all
be completed before the volatile access.

> My intent here was to prevent the compiler omitting the write (though in
> practice it didn't). I think the barrier is still required to prevent
> reordering according to the spec.

Yes, my understating is that you added the ACCESS_ONCE() 

Re: [PATCH v2 05/13] xen/bitops: Implement generic_f?sl() in lib/

[Jan Beulich]

On 24.05.2024 22:03, Andrew Cooper wrote:
> generic_f?s() being static inline is the cause of lots of the complexity
> between the common and arch-specific bitops.h
> 
> They appear to be static inline for constant-folding reasons (ARM uses them
> for this), but there are better ways to achieve the same effect.
> 
> It is presumptuous that an unrolled binary search is the right algorithm to
> use on all microarchitectures.  Indeed, it's not for the eventual users, but
> that can be addressed at a later point.
> 
> It is also nonsense to implement the int form as the base primitive and
> construct the long form from 2x int in 64-bit builds, when it's just one extra
> step to operate at the native register width.
> 
> Therefore, implement generic_f?sl() in lib/.  They're not actually needed in
> x86/ARM/PPC by the end of the cleanup (i.e. the functions will be dropped by
> the linker), and they're only expected be needed by RISC-V on hardware which
> lacks the Zbb extension.
> 
> Implement generic_fls() in terms of generic_flsl() for now, but this will be
> cleaned up in due course.
> 
> Provide basic runtime testing using __constructor inside the lib/ file.  This
> is important, as it means testing runs if and only if generic_f?sl() are used
> elsewhere in Xen.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 
with a suggestion and a question.

> I suspect we want to swap CONFIG_DEBUG for CONFIG_BOOT_UNIT_TESTS in due
> course.  These ought to be able to be used in a release build too.

+1

> --- /dev/null
> +++ b/xen/lib/generic-ffsl.c
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include 
> +#include 
> +#include 
> +
> +unsigned int generic_ffsl(unsigned long x)
> +{
> +unsigned int r = 1;
> +
> +if ( !x )
> +return 0;
> +
> +#if BITS_PER_LONG > 32

To be future-proof, perhaps ahead of this

#if BITS_PER_LONG > 64
# error "..."
#endif

or a functionally similar BUILD_BUG_ON()?

> --- /dev/null
> +++ b/xen/lib/generic-flsl.c
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include 
> +#include 
> +#include 
> +
> +/* Mask of type UL with the upper x bits set. */
> +#define UPPER_MASK(x) (~0UL << (BITS_PER_LONG - (x)))
> +
> +unsigned int generic_flsl(unsigned long x)
> +{
> +unsigned int r = BITS_PER_LONG;
> +
> +if ( !x )
> +return 0;
> +
> +#if BITS_PER_LONG > 32
> +if ( !(x & UPPER_MASK(32)) )
> +{
> +x <<= 32;
> +r -= 32;
> +}
> +#endif
> +if ( !(x & UPPER_MASK(16)) )
> +{
> +x <<= 16;
> +r -= 16;
> +}
> +if ( !(x & UPPER_MASK(8)) )
> +{
> +x <<= 8;
> +r -= 8;
> +}
> +if ( !(x & UPPER_MASK(4)) )
> +{
> +x <<= 4;
> +r -= 4;
> +}
> +if ( !(x & UPPER_MASK(2)) )
> +{
> +x <<= 2;
> +r -= 2;
> +}
> +if ( !(x & UPPER_MASK(1)) )
> +{
> +x <<= 1;
> +r -= 1;
> +}
> +
> +return r;
> +}

While, as you say, the expectation is for this code to not commonly come
into actual use, I still find the algorithm a little inefficient in terms
of the constants used, specifically considering how they would need
instantiating in resulting assembly. It may be that Arm's fancy constant-
move insns can actually efficiently synthesize them, but I think on most
other architectures it would be more efficient (and presumably no less
efficient on Arm) to shift the "remaining" value right, thus allowing for
successively smaller (and hence easier to instantiate) constants to be
used.

Jan

Re: [PATCH v2 02/13] xen/bitops: Cleanup ahead of rearrangements

[Jan Beulich]

On 24.05.2024 22:03, Andrew Cooper wrote:
>  * Rename __attribute_pure__ to just __pure before it gains users.
>  * Introduce __constructor which is going to be used in lib/, and is
>unconditionally cf_check.
>  * Identify the areas of xen/bitops.h which are a mess.
>  * Introduce xen/boot-check.h as helpers for compile and boot time testing.
>This provides a statement of the ABI, and a confirmation that arch-specific
>implementations behave as expected.
> 
> Sadly Clang 7 and older isn't happy with the compile time checks.  Skip them,
> and just rely on the runtime checks.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 

Further remarks, though:

> ---
>  xen/include/xen/bitops.h | 13 ++--
>  xen/include/xen/boot-check.h | 60 
>  xen/include/xen/compiler.h   |  3 +-
>  3 files changed, 72 insertions(+), 4 deletions(-)
>  create mode 100644 xen/include/xen/boot-check.h

The bulk of the changes isn't about bitops; it's just that you're intending
to first use it for testing there. The subject prefix therefore is somewhat
misleading.

> --- /dev/null
> +++ b/xen/include/xen/boot-check.h
> @@ -0,0 +1,60 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +/*
> + * Helpers for boot-time checks of basic logic, including confirming that
> + * examples which should be calculated by the compiler are.
> + */
> +#ifndef XEN_BOOT_CHECK_H
> +#define XEN_BOOT_CHECK_H
> +
> +#include 
> +
> +/* Hide a value from the optimiser. */
> +#define HIDE(x) \
> +({ typeof(x) _x = (x); asm volatile ( "" : "+r" (_x) ); _x; })

In principle this is a macro that could be of use elsewhere. That's also
reflected in its entirely generic name. It therefore feels mis-placed in
this header. Otoh though the use of "+r" is more restricting than truly
necessary: While I'm not sure if "+g" would work, i.e. if that wouldn't
cause issues with literals, pretty surely "+rm" ought to work, removing
the strict requirement for the compiler to put a certain value in a
register.

Assuming you may have reservations against "+g" / "+rm" (and hence the
construct wants keeping here), maybe rename to e.g. BOOT_CHECK_HIDE()?
Alternatively, if generalized, moving to xen/macros.h would seem
appropriate to me.

Finally, plainly as a remark with no request for any change (but
possibly a minor argument against moving to xen/macros.h), this construct
won't, afaict, work if x is of array(-of-const) type. A more specialized
variant may need introducing, should any such use ever appear.

Jan

Re: [PATCH v2 8/8] xen/x86: Synthesise domain topologies

[Roger Pau Monné]

On Fri, May 24, 2024 at 06:16:01PM +0100, Alejandro Vallejo wrote:
> On 24/05/2024 09:58, Roger Pau Monné wrote:
> > On Wed, May 08, 2024 at 01:39:27PM +0100, Alejandro Vallejo wrote:
> > 
> >> +rc = x86_topo_from_parts(&p->policy, threads_per_core, 
> >> cores_per_pkg);
> > 
> > I assume this generates the same topology as the current code, or will
> > the population of the leaves be different in some way?
> > 
> 
> The current code does not populate 0xb. This generates a topology
> consistent with the existing INTENDED topology. The actual APIC IDs will
> be different though (because there's no skipping of odd values).
> 
> All the dance in patch 1 was to make this migrate-safe. The x2apic ID is
> stored in the lapic hidden regs so differences with previous behaviour
> don't matter.

What about systems without CPU policy in the migration stream, will
those also get restored as expected?

I think you likely need to check whether 'restore' is set and keep the
old logic in that case?

As otherwise migrated systems without a CPU policy will get the new
topology information instead of the old one?

> IOW, The differences are:
>   * 0xb is exposed, whereas previously it wasn't
>   * APIC IDs are compacted such that new_apicid=old_apicid/2
>   * There's also a cleanup of the murkier paths to put the right core
> counts in the right leaves (whereas previously it was bonkers)

This needs to be in the commit message IMO.

> > 
> > Note that currently the host policy also gets the topology leaves
> > cleared, is it intended to not clear them anymore after this change?
> > 
> > (as you only clear the leaves for the guest {max,def} policies)
> > 
> > Thanks, Roger.
> 
> It was like that originally in v1, I changed in v2 as part of feedback
> from Jan.

I think that's fine, but this divergence from current behavior of
cleaning the topology for the host policy needs to be mentioned in
the commit message.

Thanks, Roger.

Re: [PATCH v2 7/8] xen/x86: Derive topologically correct x2APIC IDs from the policy

[Roger Pau Monné]

On Fri, May 24, 2024 at 06:03:22PM +0100, Alejandro Vallejo wrote:
> On 24/05/2024 09:39, Roger Pau Monné wrote:
> > On Wed, May 08, 2024 at 01:39:26PM +0100, Alejandro Vallejo wrote:
> > 
> > Also you could initialize x2apic_id at definition:
> > 
> > const struct test *t = &tests[j];
> > struct cpu_policy policy = { .x86_vendor = vendors[i] };
> > int rc = x86_topo_from_parts(&policy, t->threads_per_core, 
> > t->cores_per_pkg);
> > uint32_t x2apic_id = x86_x2apic_id_from_vcpu_id(&policy, t->vcpu_id);
> 
> Seeing this snippet I just realized there's a bug. The second loop
> should use j rather than i. Ugh.

Well, you shadow the outer variable with the inner one, which makes it
still fine.  Yet I don't like that shadowing much.  I was going to
comment, but for the requested change you need to not shadow the outer
loop variable (in the example chunk I've used 'j' to signal the outer
loop index).

> >> +}
> >> +
> >>  uint32_t x86_x2apic_id_from_vcpu_id(const struct cpu_policy *p, uint32_t 
> >> id)
> >>  {
> >> +uint32_t shift = 0, x2apic_id = 0;
> >> +
> >> +/* In the absence of topology leaves, fallback to traditional mapping 
> >> */
> >> +if ( !p->topo.subleaf[0].type )
> >> +return id * 2;
> >> +
> >>  /*
> >> - * TODO: Derive x2APIC ID from the topology information inside `p`
> >> - *   rather than from vCPU ID. This bodge is a temporary measure
> >> - *   until all infra is in place to retrieve or derive the initial
> >> - *   x2APIC ID from migrated domains.
> > 
> > I'm a bit confused with this, the policy is domain wide, so we will
> > always need to pass the vCPU ID into x86_x2apic_id_from_vcpu_id()?
> > IOW: the x2APIC ID will always be derived from the vCPU ID.
> > 
> > Thanks, Roger.
> 
> The x2APIC ID is derived (after the series) from the vCPU ID _and_ the
> topology information. The vCPU alone will work out in all cases because
> it'll be cached in the vlapic hvm structure.
> 
> I guess the comment could be rewritten as "... rather than from the vCPU
> ID alone..."

Yup, that's clearer :).

Thanks, Roger.

Re: [PATCH v16 4/5] xen/arm: translate virtual PCI bus topology for guests

[Roger Pau Monné]

On Fri, May 24, 2024 at 02:21:09PM +0100, Julien Grall wrote:
> Hi,
> 
> Sorry I didn't notice there was a v16 and posted comments on the v15. The
> only one is about the size of the list we iterate.
> 
> On 23/05/2024 08:48, Roger Pau Monné wrote:
> > On Wed, May 22, 2024 at 06:59:23PM -0400, Stewart Hildebrand wrote:
> > > From: Oleksandr Andrushchenko 
> > > +}
> > > -return sbdf;
> > > +return translated;
> > >   }
> > >   static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
> > > register_t *r, void *p)
> > >   {
> > >   struct pci_host_bridge *bridge = p;
> > > -pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
> > > +pci_sbdf_t sbdf;
> > >   const unsigned int access_size = (1U << info->dabt.size) * 8;
> > >   const register_t invalid = GENMASK_ULL(access_size - 1, 0);
> > 
> > Do you know why the invalid value is truncated to the access size.
> 
> Because no other callers are doing the truncation and therefore the guest
> would read 1s even for 8-byte unsigned access.

I think forcing all handlers to do the truncation is a lot of
duplication, and more risky than just doing it in the dispatcher
itself (handle_read()), see my reply to 1/5.

Thanks, Roger.