date:20231117

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

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  98758ae48974d6d24f999e4d9324e463d326f66f
baseline version:
 xen  97f8555acbf3da013ed713ca0bbe739d41c48da9

Last test of basis   183784  2023-11-17 19:03:49 Z0 days
Testing same since   183786  2023-11-18 03:02:10 Z0 days1 attempts


People who touched revisions under test:
  Federico Serafini 
  Gianluca Luparini 
  Jan Beulich 
  Julien Grall 
  Nicola Vetrini 
  Simone Ballarin 
  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
   97f8555acb..98758ae489  98758ae48974d6d24f999e4d9324e463d326f66f -> smoke

xen | Successful pipeline for staging | 98758ae4

2023-11-17 Thread GitLab



Pipeline #1076994458 has passed!

Project: xen ( https://gitlab.com/xen-project/xen )
Branch: staging ( https://gitlab.com/xen-project/xen/-/commits/staging )

Commit: 98758ae4 ( 
https://gitlab.com/xen-project/xen/-/commit/98758ae48974d6d24f999e4d9324e463d326f66f
 )
Commit Message: xen: introduce function type bug_fn_t.

Introdu...
Commit Author: Federico Serafini
Committed by: Stefano Stabellini



Pipeline #1076994458 ( 
https://gitlab.com/xen-project/xen/-/pipelines/1076994458 ) triggered by Ganis 
( https://gitlab.com/ganis )
successfully completed 129 jobs in 3 stages.

-- 
You're receiving this email because of your account on gitlab.com.

xen | Successful pipeline for staging | 83e9e305

2023-11-17 Thread GitLab



Pipeline #1076983423 has passed!

Project: xen ( https://gitlab.com/xen-project/xen )
Branch: staging ( https://gitlab.com/xen-project/xen/-/commits/staging )

Commit: 83e9e305 ( 
https://gitlab.com/xen-project/xen/-/commit/83e9e305103ef00ba3b546657d47c4aa85899cff
 )
Commit Message: automation/eclair: add a deviation for MISRA C:...
Commit Author: Federico Serafini
Committed by: Stefano Stabellini



Pipeline #1076983423 ( 
https://gitlab.com/xen-project/xen/-/pipelines/1076983423 ) triggered by Ganis 
( https://gitlab.com/ganis )
successfully completed 129 jobs in 3 stages.

-- 
You're receiving this email because of your account on gitlab.com.

[linux-linus test] 183783: regressions - FAIL

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-arm64-arm64-examine  8 reboot   fail REGR. vs. 183766
 test-arm64-arm64-libvirt-xsm  8 xen-boot fail REGR. vs. 183766
 test-arm64-arm64-xl-credit2   8 xen-boot fail REGR. vs. 183766
 test-arm64-arm64-xl-thunderx  8 xen-boot fail REGR. vs. 183766
 test-arm64-arm64-xl   8 xen-boot fail REGR. vs. 183766
 test-arm64-arm64-xl-xsm   8 xen-boot fail REGR. vs. 183766
 test-arm64-arm64-xl-vhd   8 xen-boot fail REGR. vs. 183766
 test-arm64-arm64-xl-credit1   8 xen-boot fail REGR. vs. 183766
 test-arm64-arm64-libvirt-raw  8 xen-boot fail REGR. vs. 183766

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

version targeted for testing:
 linux6bc40e44f1ddef16a787f3501b97f1fff909177c
baseline version:
 linuxc42d9eeef8e5ba9292eda36fd8e3c11f35ee065c

Last test of basis   183766  2023-11-15 17:14:16 Z2 days
Failing since183773  2023-11-16 13:12:48 Z1 days3 attempts
Testing same since   183783  2023-11-17 15:52:12 Z0 days1 attempts


People who touched revisions under test:
  "Michael S. Tsirkin" 
  Aleksandr Loktionov 
  Alex Pakhunov 
  Alexei Starovoitov 
  Alistair Francis 
  Amir Goldstein 
  Anders Roxell 
  Andrii Nakryiko 
  Arkadiusz Kubalewski 
  Arpana Arland  (A Contingent worker at Intel)
  Baruch Siach 
  Björn Töpel 
  Chandradeep Dey 
  ChunHao Lin 
  Clément Léger 
  Dan Carpenter 
  Dan Nowlin 
  David S. Miller 
  David Woodhouse 
  Dust Li 
  Eduard Zingerman 
  Erez Shitrit 
  Eric Dumazet 
  Eugenio Pérez 
  Eymen Yigit 
  Gal Pressman 
  Gavin Li 
  Geliang Tang 
  Hou Tao 
  Itamar Gozlan 
  Jakub Kicinski 
  Jakub Sitnicki 
  Jan Kiszka 
  Jason Andryuk 
  Jason Wang 
  Jay Vosburgh 
  Jian Shen 
  Jianbo Liu 
  Jijie Shao 
  Johnathan Mantey 
  Jozsef Kadlecsik 
  Juergen Gross 
  Kai Vehmanen 
  Kailang Yang 
  Linkui Xiao 
  Linus Torvalds 
  Linus Walleij 
  Lorenzo Bianconi 
  Maarten Lankhorst 
  Maciej Fijalkowski 
  Maher Sanalla 
  Marek Behún 
  Matthieu Baerts 
  Matus Malych 
  MD Danish Anwar 
  Michael S. Tsirkin 
  Niklas Söderlund 
  Pablo Neira Ayuso 
  Paolo Abeni 
  Paul Greenwalt 
  Paul Moore 
  Rahul Rameshbabu 
  Randy Dunlap

Re: [XEN PATCH 5/5] xen/xalloc: address violations of MISRA C:2012 Rule 8.2

On Fri, 17 Nov 2023, Federico Serafini wrote:
> Add missing parameter names. No functional change.
> 
> Signed-off-by: Federico Serafini 

Reviewed-by: Stefano Stabellini

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

On Fri, 17 Nov 2023, Federico Serafini wrote:
> Add missing parameter names. No functional change.
> 
> Signed-off-by: Federico Serafini 

Reviewed-by: Stefano Stabellini

Re: [XEN PATCH 3/5] xen/sort: address violations of MISRA C:2012 Rule 8.2

On Fri, 17 Nov 2023, Federico Serafini wrote:
> Add missing parameter names. No functional change.
> 
> Signed-off-by: Federico Serafini 
> ---
>  xen/include/xen/sort.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/xen/sort.h b/xen/include/xen/sort.h
> index 2f52ff85b9..1d5e3c5849 100644
> --- a/xen/include/xen/sort.h
> +++ b/xen/include/xen/sort.h
> @@ -23,8 +23,8 @@
>  extern gnu_inline
>  #endif
>  void sort(void *base, size_t num, size_t size,
> -  int (*cmp)(const void *, const void *),
> -  void (*swap)(void *, void *, size_t))
> +  int (*cmp)(const void *key, const void *elem),
> +  void (*swap)(void *a, void *b, size_t size))
>  {
>  /* pre-scale counters for performance */
>  size_t i = (num / 2) * size, n = num * size, c, r;


Ideally we should also fix swap_memory_node, swap_mmio_handler

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

2023-11-17 Thread Elliott Mitchell

On Fri, Nov 17, 2023 at 11:12:37AM +0100, Neowutran wrote:
> On 2023-11-07 11:11, Elliott Mitchell wrote:
> > On Mon, Oct 30, 2023 at 04:27:22PM +01
> > > On Mon, Oct 30, 2023 at 07:50:27AM -0700, Elliott Mitchell wrote:
> > > > On Tue, Oct 24, 2023 at 03:51:50PM +0200, Roger Pau Monne wrote:
> > > > > diff --git a/xen/arch/x86/genapic/x2apic.c 
> > > > > b/xen/arch/x86/genapic/x2apic.c
> > > > > index 707deef98c27..15632cc7332e 100644
> > > > > --- a/xen/arch/x86/genapic/x2apic.c
> > > > > +++ b/xen/arch/x86/genapic/x2apic.c
> > > > > @@ -220,38 +239,56 @@ static struct notifier_block x2apic_cpu_nfb = {
> > > > >  static int8_t __initdata x2apic_phys = -1;
> > > > >  boolean_param("x2apic_phys", x2apic_phys);
> > > > >  
> > > > > +enum {
> > > > > +   unset, physical, cluster, mixed
> > > > > +} static __initdata x2apic_mode = unset;
> > > > > +
> > > > > +static int __init parse_x2apic_mode(const char *s)
> > > > > +{
> > > > > +if ( !cmdline_strcmp(s, "physical") )
> > > > > +x2apic_mode = physical;
> > > > > +else if ( !cmdline_strcmp(s, "cluster") )
> > > > > +x2apic_mode = cluster;
> > > > > +else if ( !cmdline_strcmp(s, "mixed") )
> > > > > +   
> > > > > +else
> > > > > +return EINVAL;
> > > > > +
> > > > > +return 0;
> > > > > +}
> > > > > +custom_param("x2apic-mode", parse_x2apic_mode);
> > > > > +
> > > > >  const struct genapic *__init apic_x2apic_probe(void)
> > > > >  {
> > > > > -if ( x2apic_phys < 0 )
> > > > > +/* x2apic-mode option has preference over x2apic_phys. */
> > > > > +if ( x2apic_phys >= 0 && x2apic_mode == unset )
> > > > > +x2apic_mode = x2apic_phys ? physical : cluster;
> > > > > +
> > > > > +if ( x2apic_mode == unset )
> > > > >  {
> > > > > -/*
> > > > > - * Force physical mode if there's no (full) interrupt 
> > > > > remapping support:
> > > > > - * The ID in clustered mode requires a 32 bit destination 
> > > > > field due to
> > > > > - * the usage of the high 16 bits to hold the cluster ID.
> > > > > - */
> > > > > -x2apic_phys = iommu_intremap != iommu_intremap_full ||
> > > > > -  (acpi_gbl_FADT.flags & 
> > > > > ACPI_FADT_APIC_PHYSICAL) ||
> > > > > -
> > > > > -}
> > > > > -else if ( !x2apic_phys )
> > > > > -switch ( iommu_intremap )
> > > > > +if ( acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL )
> > > > >  {
> > > > 
> > > > Could this explain the issues with recent AMD processors/motherboards?
> > > > 
> > > > Mainly the firmware had been setting this flag, but Xen was previously
> > > > ignoring it?
> > > 
> > > No, not unless you pass {no-}x2apic_phys={false,0} on the Xen command
> > > line to force logical (clustered) destination mode.
> > > 
> > > > As such Xen had been attempting to use cluster mode on an
> > > > x2APIC where that mode was broken for physical interrupts?
> > > 
> > > No, not realy, x2apic_phys was already forced to true if
> > > acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL is set on the FADT (I
> > > just delete that line in this same chunk and move it here).
> > 
> > Okay, that was from a quick look at the patch.  Given the symptoms and
> > workaround with recent AMD motherboards, this looked
> > 
> > In that case it might be a bug in what AMD is providing to motherboard
> > manufacturers.  Mainly this bit MUST be set, but AMD's implementation
> > leaves it unset.
> > 
> > Could also be if the setup is done correctly the bit can be cleared, but
> > multiple motherboard manufacturers are breaking this.  Perhaps the steps
> > are fragile and AMD needed to provide better guidance.
> > 
> > 
> > Neowutran, are you still setup to and interested in doing
> > experimentation/testing with Xen on your AMD computer?  Would you be up
> > for trying the patch here:
> > 
> > https://lore.kernel.org/xen-devel/20231106142739.19650-1-roger@citrix.com/raw
> > 
> > I have a suspicion this *might* fix the x2APIC issue everyone has been
> > seeing.
> > 
> > How plausible would it be to release this as a bugfix/workaround on 4.17?
> > I'm expecting a "no", but figured I should ask given how widespread the
> > issue is.
> 
> I just applied the patch on my setup ( 
> https://lore.kernel.org/xen-devel/20231106142739.19650-1-roger@citrix.com/raw
>  ) 
> It seems to fix the x2APIC issue I was having. 
> 
> I only did some quick tests: 
> 
> I tried all the differents values in my bios for the X2APIC settings. 
> Now the system successfully boot in all the cases, without needing
> manual override of the x2apic_phys/x2apic_mode parameter in boot commandline .

In light of this issue effecting a large number of people with recent
hardware, I formally request the patch
"x86/x2apic: introduce a mixed physical/cluster mode" be considered for
backport release on the 4.17 and 4.18 branches.

(I'm unsure whether it is realistic for a 4.17 update, but I figure I
should ask)


--

Re: [XEN PATCH 2/5] xen/serial: address violations of MISRA C:2012 Rule 8.2

On Fri, 17 Nov 2023, Federico Serafini wrote:
> Add missing parameter names. No functional change.
> 
> Signed-off-by: Federico Serafini 

Reviewed-by: Stefano Stabellini

Re: [XEN PATCH 1/5] xen/common: address violations of MISRA C:2012 Rule 8.2

On Fri, 17 Nov 2023, Federico Serafini wrote:
> diff --git a/xen/common/stop_machine.c b/xen/common/stop_machine.c
> index 3adbe380de..398cfd507c 100644
> --- a/xen/common/stop_machine.c
> +++ b/xen/common/stop_machine.c
> @@ -46,7 +46,7 @@ struct stopmachine_data {
>  
>  unsigned int fn_cpu;
>  int fn_result;
> -int (*fn)(void *);
> +int (*fn)(void *data);
>  void *fn_data;
>  };

At least one of the possible function used here calls the parameter
"arg", see take_cpu_down. But I don't think it is a MISRA requirement to
also harmonize those?


> @@ -73,7 +73,7 @@ static void stopmachine_wait_state(void)
>   * mandatory to be called only on an idle vcpu, as otherwise active core
>   * scheduling might hang.
>   */
> -int stop_machine_run(int (*fn)(void *), void *data, unsigned int cpu)
> +int stop_machine_run(int (*fn)(void *data), void *data, unsigned int cpu)
>  {
>  unsigned int i, nr_cpus;
>  unsigned int this = smp_processor_id();
> diff --git a/xen/common/tasklet.c b/xen/common/tasklet.c
> index 3ad67b5c24..3649798e6b 100644
> --- a/xen/common/tasklet.c
> +++ b/xen/common/tasklet.c
> @@ -199,7 +199,7 @@ static void migrate_tasklets_from_cpu(unsigned int cpu, 
> struct list_head *list)
>  spin_unlock_irqrestore(_lock, flags);
>  }
>  
> -void tasklet_init(struct tasklet *t, void (*func)(void *), void *data)
> +void tasklet_init(struct tasklet *t, void (*func)(void *data), void *data)
>  {
>  memset(t, 0, sizeof(*t));
>  INIT_LIST_HEAD(>list);
> @@ -208,7 +208,8 @@ void tasklet_init(struct tasklet *t, void (*func)(void 
> *), void *data)
>  t->data = data;
>  }
>  
> -void softirq_tasklet_init(struct tasklet *t, void (*func)(void *), void 
> *data)
> +void softirq_tasklet_init(struct tasklet *t,
> +  void (*func)(void *data), void *data)
>  {
>  tasklet_init(t, func, data);
>  t->is_softirq = 1;
> diff --git a/xen/common/timer.c b/xen/common/timer.c
> index 0fddfa7487..bf7792dcb3 100644
> --- a/xen/common/timer.c
> +++ b/xen/common/timer.c
> @@ -291,7 +291,7 @@ static bool active_timer(const struct timer *timer)
>  
>  void init_timer(
>  struct timer *timer,
> -void(*function)(void *),
> +void(*function)(void *data),
>  void *data,
>  unsigned int  cpu)
>  {
> @@ -441,7 +441,7 @@ void kill_timer(struct timer *timer)
>  
>  static void execute_timer(struct timers *ts, struct timer *t)
>  {
> -void (*fn)(void *) = t->function;
> +void (*fn)(void *data) = t->function;
>  void *data = t->data;
>  
>  t->status = TIMER_STATUS_inactive;
> diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
> index 135f33f606..390f7b6082 100644
> --- a/xen/include/xen/rangeset.h
> +++ b/xen/include/xen/rangeset.h
> @@ -68,7 +68,7 @@ bool_t __must_check rangeset_overlaps_range(
>  struct rangeset *r, unsigned long s, unsigned long e);
>  int rangeset_report_ranges(
>  struct rangeset *r, unsigned long s, unsigned long e,
> -int (*cb)(unsigned long s, unsigned long e, void *), void *ctxt);
> +int (*cb)(unsigned long s, unsigned long e, void *data), void *ctxt);

Also here some of the functions use "arg" instead of ctxt


>  /*
>   * Note that the consume function can return an error value apart from
> @@ -77,7 +77,7 @@ int rangeset_report_ranges(
>   */
>  int rangeset_consume_ranges(struct rangeset *r,
>  int (*cb)(unsigned long s, unsigned long e,
> -  void *, unsigned long *c),
> +  void *ctxt, unsigned long *c),
>  void *ctxt);

Also here some of the functions use "dom" like irq_remove_cb.


But I actually like the patch as is, so if that's OK from a MISRA point
of view then I would give my reviewed-by.

Re: [XEN PATCH v3] xen: replace occurrences of SAF-1-safe with asmlinkage attribute

On Fri, 17 Nov 2023, Julien Grall wrote:
> On 16/11/2023 09:15, Nicola Vetrini wrote:
> > On 2023-11-16 10:08, Nicola Vetrini wrote:
> > > The comment-based justifications for MISRA C:2012 Rule 8.4 are replaced
> > > by the asmlinkage pseudo-attribute, for the sake of uniformity.
> > > 
> > > Add missing 'xen/compiler.h' #include-s where needed.
> > > 
> > > The text in docs/misra/deviations.rst and docs/misra/safe.json
> > > is modified to reflect this change.
> > > 
> > > Signed-off-by: Nicola Vetrini 
> > > ---
> > > This patch should be applied after patch 2 of this series.
> > > The request made by Julien to update the wording is
> > > contained in the present patch.
> > > https://lore.kernel.org/all/9ad7f6210c15f520297aac00e8af0...@bugseng.com/
> > > 
> > > Concerns about efi_multiboot2 will be dealt with separately.
> > > 
> > > Changes in v2:
> > > - Edit safe.json.
> > > - Remove mention of SAF-1-safe in deviations.rst.
> > > Changes in v3:
> > > - Sorted #include-s and rebased against
> > > 7ad0c774e474 ("x86/boot: tidy #include-s")
> > > ---
> > >  docs/misra/deviations.rst   |  5 ++---
> > >  docs/misra/safe.json    |  2 +-
> > >  xen/arch/arm/cpuerrata.c    |  7 +++
> > >  xen/arch/arm/setup.c    |  5 ++---
> > >  xen/arch/arm/smpboot.c  |  3 +--
> > >  xen/arch/arm/traps.c    | 21 +++--
> > >  xen/arch/x86/boot/cmdline.c |  5 +++--
> > >  xen/arch/x86/boot/reloc.c   |  6 +++---
> > >  xen/arch/x86/extable.c  |  3 +--
> > >  xen/arch/x86/setup.c    |  3 +--
> > >  xen/arch/x86/traps.c    | 27 +--
> > >  xen/common/efi/boot.c   |  5 ++---
> > >  12 files changed, 35 insertions(+), 57 deletions(-)
> > > 
> > 
> > In hindsight I should have added an
> > 
> > Acked-by: Julien Grall 
> > 
> > given that the comment has been addressed in my opinion.
> 
> I am a bit confused how you considered it was addressed. I see no update in
> safe.json when I clearly asked for some (I wouldn't have bothered to comment
> in v2 otherwise and just gave an ack).
> 
> To be explicit, I requested to:
>   1. update the description in [1] to clarify that SAF-1 is deprecated.
>   2. This patch is rebased on top and therefore remove completely the mention
> of SAF-1.
> 
> I am well-aware that the end result is technically the same. But patches are
> meant to be self-contained so if we revert the latest, then the meaning is
> still the same.
> 
> This patch is unlikely to be removed and this is now the nth time I asked it
> the same (maybe it was not clear enough?). So I am going to content with the
> current proposal because this is not worth to go further. But I will at least
> express my discontent how this is handled.

Just to be extra clearm, you are not happy with it, but you would
tolerate the patch to be committed as is, right?


> TBH, there are far too many MISRA patches on the ML spread across multiple
> threads. Some are based on top of the others. This makes extremely difficult
> to follow and know what is addressed or not. Can we at least try to condense
> some of work in similar area in the same series? For instance, this patch
> could have been included in the other series [1].
> 
> Lastly, right now, I have 300 emails (31 threads) with MISRA in the title in
> my inbox. It is a little unclear what has been committed/review or require
> input. I am concerned to miss key series (the patch to compile in docs/ was
> nearly missed).
> 
> Do we track anywhere which series are still inflights? Can we consider to
> pause or at least slow down the rate of new MISRA patches until the backlog is
> cleared? (Adding more patches is not really helping).

I cleared out the ones I was tracking and were acked. I hope this helps.
As far as I can tell these are the ones currently under discussion:

- [XEN PATCH v5 0/2] use the documentation for MISRA C:2012 Dir 4.1
- first 4 patches of [XEN PATCH][for-4.19 v4 0/8] address violations of MISRA 
C:2012 Rule 10.1
- [XEN PATCH][for-4.19 v2 0/2] use the macro ISOLATE_LOW_BIT where appropriate
- [XEN PATCH v2] domain: add ASSERT to help static analysis tools
- [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3
- [XEN PATCH v2] automation/eclair: add deviations for MISRA C:2012 Rule 8.3
- this patch
- [XEN PATCH 0/5] xen: address some violations of MISRA C:2012 Rule 8.2

Re: Devise macros to encapsulate (x & -x)

On Fri, 17 Nov 2023, Andrew Cooper wrote:
> On 17/11/2023 10:17 am, Nicola Vetrini wrote:
> > Hi all,
> >
> > As discussed in this thread [1], which is about complying with MISRA C
> > Rule 10.1,
> > a macro was introduced to encapsulate a well-known construct:
> >
> > /*
> >  * Given an unsigned integer argument, expands to a mask where just
> > the least
> >  * significant nonzero bit of the argument is set, or 0 if no bits are
> > set.
> >  */
> > #define ISOLATE_LSB(x) ((x) & -(x))
> >
> > This macro has a gained some calls in the subsequent patches in that
> > thread, but concerns were raised around the fact that it would be
> > better to devise a macro that evaluates its argument only once. A
> > proposed solution is this (thanks to Jan Beulich):
> >
> > #define ISOLATE_LSB(x) ({ \
> >  typeof(x) x_ = (x); \
> >  x_ & -x_; \
> > })
> 
> Of course this was going to explode.
> 
> This isn't even the first time an unwise attempt to do single-evaluation
> has needed to be reverted because it doesn't work with Integer Constant
> Expressions.
> 
> Switch it back to the first form.  It's obviously a macro to begin with,
> and not likely to be used in cases that have side effects.

+1

Re: [XEN PATCH v2] automation/eclair: add deviations for MISRA C:2012 Rule 8.3

On Fri, 17 Nov 2023, Federico Serafini wrote:
> On 27/10/23 00:55, Stefano Stabellini wrote:
> > +Roger
> > 
> > See below
> > 
> > On Thu, 26 Oct 2023, Julien Grall wrote:
> > > On 26/10/2023 15:04, Federico Serafini wrote:
> > > > On 26/10/23 15:54, Julien Grall wrote:
> > > > > Hi,
> > > > > 
> > > > > On 26/10/2023 13:13, Federico Serafini wrote:
> > > > > > On 26/10/23 12:25, Julien Grall wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On 26/10/2023 11:04, Federico Serafini wrote:
> > > > > > > > Update ECLAIR configuration to deviate Rule 8.3 ("All
> > > > > > > > declarations
> > > > > > > > of
> > > > > > > > an object or function shall use the same names and type
> > > > > > > > qualifiers")
> > > > > > > > for the following functions: guest_walk_tables_[0-9]+_levels().
> > > > > > > > Update file docs/misra/deviations.rst accordingly.
> > > > > > > > No functional change.
> > > > > > > > 
> > > > > > > > Signed-off-by: Federico Serafini 
> > > > > > > > ---
> > > > > > > > Changes in v2:
> > > > > > > >     - removed set_px_pminfo() from the scope of the deviation;
> > > > > > > >     - fixed tag of the commit.
> > > > > > > > ---
> > > > > > > >    automation/eclair_analysis/ECLAIR/deviations.ecl | 4 
> > > > > > > >    docs/misra/deviations.rst    | 6 ++
> > > > > > > >    2 files changed, 10 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl
> > > > > > > > b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > > > > > > > index d8170106b4..b99dfdafd6 100644
> > > > > > > > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> > > > > > > > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > > > > > > > @@ -204,6 +204,10 @@ const-qualified."
> > > > > > > > 
> > > > > > > > 
> > > > > > > > -config=MC3R1.R8.3,reports+={deliberate,"any_area(any_loc(file(adopted_mpparse_r8_3)))&_area(any_loc(file(^xen/arch/x86/include/asm/mpspec\\.h$)))"}
> > > > > > > >    -doc_end
> > > > > > > > +-doc_begin="For functions guest_walk_tables_[0-9]+_levels(),
> > > > > > > > parameter names of definitions deliberately differ from the ones
> > > > > > > > used in the corresponding declarations."
> > > > > > > > +-config=MC3R1.R8.3,declarations={deliberate,"^guest_walk_tables_[0-9]+_levels\\(const
> > > > > > > > struct vcpu\\*, struct p2m_domain\\*, unsigned long, walk_t\\*,
> > > > > > > > uint32_t, gfn_t, mfn_t, void\\*\\)$"}
> > > > > > > > +-doc_end
> > > > > > > > +
> > > > > > > >    -doc_begin="The following variables are compiled in multiple
> > > > > > > > translation units
> > > > > > > >    belonging to different executables and therefore are safe."
> > > > > > > >    -config=MC3R1.R8.6,declarations+={safe,
> > > > > > > > "name(current_stack_pointer||bsearch||sort)"}
> > > > > > > > diff --git a/docs/misra/deviations.rst
> > > > > > > > b/docs/misra/deviations.rst
> > > > > > > > index 8511a18925..9423b5cd6b 100644
> > > > > > > > --- a/docs/misra/deviations.rst
> > > > > > > > +++ b/docs/misra/deviations.rst
> > > > > > > > @@ -121,6 +121,12 @@ Deviations related to MISRA C:2012 Rules:
> > > > > > > >     - xen/common/unxz.c
> > > > > > > >     - xen/common/unzstd.c
> > > > > > > > +   * - R8.3
> > > > > > > > + - In some cases, parameter names used in the function
> > > > > > > > definition
> > > > > > > > +   deliberately differ from the ones used in the
> > > > > > > > corresponding
> > > > > > > > declaration.
> > > > > > > 
> > > > > > > It would be helpful to provide a bit more reasoning in your commit
> > > > > > > message why this was desired. At least for Arm and common code, I
> > > > > > > would not want anyone to do that because it adds more confusion.
> > > > > > > 
> > > > > > > > + - Tagged as `deliberate` for ECLAIR. Such functions are:
> > > > > > > > + - guest_walk_tables_[0-9]+_levels()
> > > > > > > 
> > > > > > > I think you want to be a bit mores specific. Other arch may have
> > > > > > > such
> > > > > > > function in the function and we don't want to deviate them from
> > > > > > > the
> > > > > > > start.
> > > > > > > 
> > > > > > > Cheers,
> > > > > > > 
> > > > > > 
> > > > > > Alright, thanks for the observation.
> > > > > 
> > > > > Actually, I cannot find the original discussion. Do you have link? I
> > > > > am
> > > > > interested to read the reasoning and how many maintainers expressed
> > > > > there
> > > > > view.
> > > > > 
> > > > > Cheers,
> > > > > 
> > > > 
> > > > The discussion started here:
> > > > https://lists.xenproject.org/archives/html/xen-devel/2023-07/msg00122.html
> > > > 
> > > > Then, I asked for further suggestions:
> > > > https://lists.xenproject.org/archives/html/xen-devel/2023-07/msg00855.html
> > > 
> > > Thanks! So only Jan really provided feedback here. I don't think this is a
> > > good idea to deviate in this case. If we really want to keep in sync and
> > > use
> > > 'walk' for the name, then we

Re: [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3

On Fri, 17 Nov 2023, Federico Serafini wrote:
> On 20/10/23 08:35, Jan Beulich wrote:
> > On 19.10.2023 18:26, Stefano Stabellini wrote:
> > > On Thu, 19 Oct 2023, Jan Beulich wrote:
> > > > On 19.10.2023 00:43, Stefano Stabellini wrote:
> > > > > On Mon, 16 Oct 2023, Jan Beulich wrote:
> > > > > > On 03.10.2023 17:24, Federico Serafini wrote:
> > > > > > > --- a/xen/arch/x86/mm.c
> > > > > > > +++ b/xen/arch/x86/mm.c
> > > > > > > @@ -5901,17 +5901,17 @@ int destroy_xen_mappings(unsigned long s,
> > > > > > > unsigned long e)
> > > > > > >* a problem.
> > > > > > >*/
> > > > > > >   void init_or_livepatch modify_xen_mappings_lite(
> > > > > > > -unsigned long s, unsigned long e, unsigned int _nf)
> > > > > > > +unsigned long s, unsigned long e, unsigned int nf)
> > > > > > >   {
> > > > > > > -unsigned long v = s, fm, nf;
> > > > > > > +unsigned long v = s, fm, flags;
> > > > > > 
> > > > > > While it looks correct, I consider this an unacceptably dangerous
> > > > > > change: What if by the time this is to be committed some new use of
> > > > > > the local "nf" appears, without resulting in fuzz while applying the
> > > > > > patch? Imo this needs doing in two steps: First nf -> flags, then
> > > > > > _nf -> nf.
> > > > > 
> > > > > Wouldn't it be sufficient for the committer to pay special attention
> > > > > when committing this patch? We are in code freeze anyway, the rate of
> > > > > changes affecting staging is low.
> > > > 
> > > > Any kind of risk excludes a patch from being a 4.18 candidate, imo.
> > > 
> > > I agree on that. I think it is best to commit it for 4.19 when the tree
> > > opens.
> > > 
> > > 
> > > > That was the case in early RCs already, and is even more so now. Paying
> > > > special attention is generally a possibility, yet may I remind you that
> > > > committing in general is intended to be a purely mechanical operation?
> > > 
> > > Sure, and I am not asking for a general process change. I am only
> > > suggesting that this specific concern on this patch is best solved in
> > > the simplest way: by a committer making sure the patch is correct on
> > > commit. It is meant to save time for everyone.
> > > 
> > > Jan, if you are OK with it, we could just trust you to commit it the
> > > right away as the earliest opportunity.
> > 
> > If you can get Andrew or Roger to ack this patch in its present shape,
> > I won't stand in the way. I'm not going to ack the change without the
> > indicated split.
> 
> I'll propose a new patch series where changes are splitted as indicated.
> I also noticed a discrepancy between Arm and x86 in the name of the
> last parameter of xenmem_add_to_physmap_one().
> Do you have any suggestions about how to solve it?
> If we reach an agreement, then I can put the changes related to the mm module
> in a single patch.

I think it should be "gfn"

Re: [XEN PATCH v3] xen: introduce function type bug_fn_t.

On Fri, 17 Nov 2023, Julien Grall wrote:
> Hi Federico,
> 
> On 17/11/2023 08:28, Federico Serafini wrote:
> > Introduce function type bug_fn_t. This improves readability and helps
> > to validate that the function passed to run_in_exception_handle() has
> > the expected prototype.
> Hmmm... I read the second part as you will validate the type in
> run_in_exception_handle(). But I can't find such change. How about:
> 
> "and could be used to help validating that ..."
> 
> No need to send a new revision for that. I can do it on commit.

I committed it together with the old patches I was tracking

Re: [PATCH 6/6] automation: switch to multi-platform images when possible

On Fri, 17 Nov 2023, Roger Pau Monné wrote:
> On Thu, Nov 16, 2023 at 05:14:23PM -0800, Stefano Stabellini wrote:
> > On Thu, 16 Nov 2023, Roger Pau Monne wrote:
> > > Instead of using specific architecture image, switch to using multi-arch 
> > > ones
> > > and specify the desired architecture using the --platform option.
> > > 
> > > Signed-off-by: Roger Pau Monné 
> > > ---
> > > I haven't touched the Yocto dockerfile because I'm not sure how it's used.
> > 
> > We are missing:
> > 
> > automation/build/debian/buster-gcc-ibt.dockerfile
> 
> That file was updated in patch 5/6:
> 
> https://lore.kernel.org/xen-devel/20231116121310.72210-6-roger@citrix.com/
> 
> > automation/build/debian/bookworm-cppcheck.dockerfile
> 
> Not sure I'm following, bookworm-cppcheck.dockerfile is updated...
> 
> > automation/tests-artifacts/*
> 
> Oh, didn't realize about those, I will do in a separate patch.

Thanks!


> > Aside from that, it is fine.
> > 
> > How did you test the updated containers? Have you already pushed them to
> > the registry?
> 
> I've pushed them to my local registry and changed the registry in one
> of my Xen branches, see:
> 
> https://gitlab.com/xen-project/people/royger/xen/-/pipelines/1074512137
> 
> Some jobs failed because the runners run out of space.

Oh, OK. It is going to be a lot of work to rebuild and push all the
containers and I wouldn't mind you doing that once the patches are
acked. In fact it would be great if you pushed the containers once you
tests that they work as expected. If you don't have the right access
permissions, I can do that too

Re: [PATCH 1/6] automation: remove CR characters from QEMU serial

On Fri, 17 Nov 2023, Roger Pau Monné wrote:
> On Thu, Nov 16, 2023 at 05:05:28PM -0800, Stefano Stabellini wrote:
> > On Thu, 16 Nov 2023, Roger Pau Monne wrote:
> > > The gitlab CI webpage seems to have issues displaying the \CR\CR\LF 
> > > "\r\r\n"
> > > sequence on the web interface used as line returns by the Linux kernel 
> > > serial
> > > output.  This leads to the QEMU tests output looking like:
> > > 
> > > (XEN) Guest Loglevel: Nothing (Rate-limited: Errors and warnings)
> > > (XEN) *** Serial input to DOM0 (type 'CTRL-a' three times to switch input)
> > > (XEN) Freed 664kB init memory
> > > mapping kernel into physical memory
> > > about to get started...
> > > qemu-system-x86_64: terminating on signal 15 from pid 52 (timeout)
> > > 
> > > This not helpful, so strip the CR characters from the output that goes to
> > > stdout, leaving the output in the smoke.serial untouched.
> > > 
> > > Fixes: 3030a73bf849 ('automation: add a QEMU based x86_64 Dom0/DomU test')
> > > Signed-off-by: Roger Pau Monné 
> > 
> > Thanks for the patch. Let me see if I understood correctly.
> > 
> > In the gitlab web UI everything after the last (XEN) log line
> > disappears, for instance:
> > 
> > https://gitlab.com/xen-project/xen/-/jobs/5556551478
> > 
> > (XEN) d1v0: vGICD: unhandled word write 0x00 to ICACTIVER0
> > / # qemu-system-aarch64: terminating on signal 15 from pid 145 (timeout)
> > 
> > 
> > While if I look at the full logs there are plenty of Linux kernel logs
> > after it:
> > https://cdn.artifacts.gitlab-static.net/ec/ad/ecad5145a0ec1eb179fd47d1590d5ec43d705e8af2f9a816607ac31891cb82b9/2023_11_16/5556551478/6032156805/job.log?response-content-type=text%2Fplain%3B%20charset%3Dutf-8=inline=1700183635=gprd-artifacts-cdn=vT8CBwI2Th23OvRvQKvNPgHiT5Y=
> > 
> > And this patch aims at fixing that, is that correct?
> > 
> > 
> > But I went to check your pipeline
> > https://gitlab.com/xen-project/people/royger/xen/-/pipelines/1074512137
> > and the corresponding job
> > https://gitlab.com/xen-project/people/royger/xen/-/jobs/5549620441 has
> > the same issue?
> 
> I made the change just for qemu-alpine-x86_64-gcc:
> 
> https://gitlab.com/xen-project/people/royger/xen/-/jobs/5550049674
> 
> I didn't realize qemu-smoke-dom0-arm64-gcc was also using it.  If the
> fix is acceptable I can submit v2 adding the arm instances also.

Yes the fix is fine. All the qemu scripts are copy/paste design right
now and I am aware they need to be unified. It is not just
qemu-smoke-dom0-arm64-gcc, also qemu-smoke-dom0less-arm64.sh and
basically all the other scripts that start QEMU.

Re: [XEN PATCH v4 1/2] automation/eclair: make the docs for MISRA C:2012 Dir 4.1 visible to ECLAIR

On Fri, 17 Nov 2023, Julien Grall wrote:
> Hi,
> 
> On 16/11/2023 08:45, Nicola Vetrini wrote:
> > On 2023-11-15 12:22, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 15/11/2023 11:02, Nicola Vetrini wrote:
> > > > On 2023-11-14 23:12, Julien Grall wrote:
> > > > > Hi,
> > > > > 
> > > > > On 14/11/2023 15:36, Nicola Vetrini wrote:
> > > > > > To be able to check for the existence of the necessary subsections
> > > > > > in
> > > > > > the documentation for MISRA C:2012 Dir 4.1, ECLAIR needs to have a
> > > > > > source
> > > > > > file that is built.
> > > > > > 
> > > > > > This file is generated from 'C-runtime-failures.rst' in docs/misra
> > > > > > and the configuration is updated accordingly.
> > > > > > 
> > > > > > Signed-off-by: Nicola Vetrini 
> > > > > > ---
> > > > > > Changes from RFC:
> > > > > > - Dropped unused/useless code
> > > > > > - Revised the sed command
> > > > > > - Revised the clean target
> > > > > > 
> > > > > > Changes in v2:
> > > > > > - Added explanative comment to the makefile
> > > > > > - printf instead of echo
> > > > > > 
> > > > > > Changes in v3:
> > > > > > - Terminate the generated file with a newline
> > > > > > - Build it with -std=c99, so that the documentation
> > > > > >    for D1.1 applies.
> > > > > > Changes in v5:
> > > > > > - Transform and build the file directly in the eclair-specific
> > > > > > directory
> > > > > > ---
> > > > > >   automation/eclair_analysis/build.sh   | 21 +++--
> > > > > >   automation/eclair_analysis/prepare.sh |  7 ---
> > > > > >   2 files changed, 23 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/automation/eclair_analysis/build.sh
> > > > > > b/automation/eclair_analysis/build.sh
> > > > > > index ec087dd822fa..f24292ed0643 100755
> > > > > > --- a/automation/eclair_analysis/build.sh
> > > > > > +++ b/automation/eclair_analysis/build.sh
> > > > > > @@ -33,12 +33,29 @@ else
> > > > > >     PROCESSORS=6
> > > > > >   fi
> > > > > >   +runtime_failures_docs() {
> > > > > > +  doc="C-runtime-failures.rst"
> > > > > > +  builddir="automation/eclair_analysis"
> > > > > > +
> > > > > > +  cp "docs/misra/${doc}" "${builddir}"
> > > > > 
> > > > > Is it necessary to copy the .rst? IOW, would it be sufficient to
> > > > > use...
> > > > > 
> > > > > > +  cd "${builddir}"
> > > > > > +  printf "/*\n\n" >"${doc}.tmp"
> > > > > > +  sed -e 's|\*/|*//*|g' "${doc}" >>"${doc}.tmp"
> > > > > 
> > > > > ... docs/misc/${doc} here?
> > > > > 
> > > > 
> > > > I didn't want to leave a stray file under docs/misra, but it's not
> > > > essential.
> > > 
> > > I am confused. I am suggesting to use:
> > > 
> > > sed -e 's|\*/|*//*|g' "../../docs/misc/${doc}" >> "${doc}.tmp"
> > > 
> > > So *.tmp is still created at the same place.
> > > 
> > 
> > Ok, makes sense.
> > 
> > > > 
> > > > > > +  printf "\n\n*/\n" >>"${doc}.tmp"
> > > > > > +  mv "${doc}.tmp" "${doc}.c"
> > > > > 
> > > > > NIT: I am not sure why you need to first create .tmp and then
> > > > > create.c.
> > > > > 
> > > > 
> > > > Wasn't this a pattern to defend against interruptions of the build, just
> > > > as I did in v3?
> > > > 
> > > > +$(TARGETS:.o=.c): %.c: %.rst
> > > > +    printf "/*\n\n" > $@.tmp
> > > > +    sed -e 's|\*/|*//*|g' $< >> $@.tmp
> > > > +    printf "\n\n*/\n" >> $@.tmp
> > > > +    mv $@.tmp $@
> > > 
> > > Yes but it makes sense for the Makefile because the target would not be
> > > re-executed if *.c exists.
> > > 
> > > But I don't think this is the case for you because you are using a bash
> > > script. So your function should always be re-executed regardless on
> > > whether it was interrupted or not.
> > > 
> > 
> > Ok.
> > 
> > > > 
> > > > > > +
> > > > > > +  # Cannot redirect to /dev/null because it would be excluded from
> > > > > > the analysis
> > > > > > +  "${CROSS_COMPILE}gcc-12" -std=c99 -c "${doc}.c" -o "${doc}.o"
> > > > > 
> > > > > NIT: It would be helpful to specify why -std=c99 is used. Above, you
> > > > > suggest this is to enable D1.1.
> > > > > 
> > > > 
> > > > Yeah, the comment in the changelog should be pasted here
> > > > 
> > > > > NIT: Can we define CC and use here and ...
> > > > > 
> > > > > > +  cd -
> > > > > > +}
> > > > > > +
> > > > > >   (
> > > > > > -  cd xen
> > > > > > +  runtime_failures_docs
> > > > > >   make "-j${PROCESSORS}" "-l${PROCESSORS}.0"    \
> > > > > >  "CROSS_COMPILE=${CROSS_COMPILE}" \
> > > > > >  "CC=${CROSS_COMPILE}gcc-12"  \
> > > > > 
> > > > > ...? This would make easier to re-use the code.
> > > > > 
> > > > 
> > > > I don't expect this build script to be changed much to be honest, but if
> > > > you think
> > > > this is beneficial then it's ok.
> > > 
> > > This is not only about code evolving. It makes easier to spot your are
> > > using the same compiler. I would not have made the remark if you were
> > > using 'gcc'.
> > > 
> > > But I noticed you were using gcc-12 and originally thought it was a
> > > mistake

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

On Fri, 17 Nov 2023, Nicola Vetrini wrote:
> Hi Jan,
> 
> > > > > > While I've committed this patch (hoping that I got the necessary
> > > > > > context
> > > > > > adjustment right for the
> > > > > > automation/eclair_analysis/ECLAIR/deviations.ecl
> > > > > > change), I'd like to come back to this before going further with
> > > > > > users
> > > > > > of
> > > > > > the new macro: I still think we ought to try to get to the single
> > > > > > evaluation wherever possible. The macro would then be used only in
> > > > > > cases
> > > > > > where the alternative construct (perhaps an isolate_lsb() macro,
> > > > > > living
> > > > > > perhaps in xen/bitops.h) cannot be used. ISOLATE_LSB() would then
> > > > > > want
> > > > > > to
> > > > > > gain a comment directing people to the "better" sibling. Thoughts?
> > > > > 
> > > > > Having the users in place would help me estimate the remaining work
> > > > > that
> > > > > needs to be done on this rule and see if my local counts match up with
> > > > > the counts in staging.
> > > > 
> > > > By "having the users in place", you mean you want other patches in this
> > > > and the dependent series to be committed as-is (except for the name
> > > > change)? That's what I'd like to avoid, as it would mean touching all
> > > > those use sites again where the proposed isolate_lsb() could be used
> > > > instead. I'd rather see all use sites be put into their final shape
> > > > right away.
> > > 
> > > This request is coming a bit late and also after all the patches have
> > > been reviewed already. I for one am not looking forward to review them
> > > again.
> > > 
> > > That said, if you could be more specified maybe it could become
> > > actionable:
> > > 
> > > - do you have a pseudo code implementation of the "better" macro you
> > >   would like to propose?
> > 
> > May I remind you that I made this request (including a draft implementation)
> > before already, and Nicola then merely found that the evaluate-once form
> > simply cannot be used everywhere? Anybody could have thought of the option
> > of "splitting" the macro. After all I hope that there is no disagreement on
> > macro arguments better being evaluated just once, whenever possible.
> > 
> > > - do you have an list of call sites you would like to be changed to use
> > >   the "better" macro?
> > 
> > No, I don't have a list. But the pattern is pretty clear: The "better" form
> > ought to be used wherever it actually can be used.
> > 
> > > Also, you might remember past discussions about time spent making
> > > changes yourself vs. others doing the same. This is one of those cases
> > > that it would be faster for you to make the change and send a patch than
> > > explaining someone else how to do it, then review the result (and
> > > review it again as it probably won't be exactly as you asked the first
> > > time.)
> > > 
> > > If you don't want the call sites to be changes twice, may I suggest you
> > > provide a patch on top of Nicola's series, I review and ack your patch,
> > > and Nicola or I rebase & resend the series so that the call sites are
> > > only changes once as you would like? I think that's going to be way
> > > faster.
> > 
> > I'll see if I can find time to do so. I don't normally work on top of
> > other people's uncommitted patches, though ... So I may also choose to go
> > a slightly different route. (You realize though that all still pending
> > patches using the new macro need touching again anyway, don't you?)
> > 
> > Jan
> 
> Then perhaps it's best if I give it a try at doing the single evaluation
> macro, so that I can make a series modifying the call sites only once on top
> of that and send everything in one go. Before doing that, though, I'll make a
> thread where various aspects that are not so clear yet can be discussed, so
> that we can devise a robust solution (also to dig this out of this deep
> thread).

In the meantime I committed patches from #5 onward as they don't depend
on ISOLATE_LSB

Re: [PATCH v10 13/17] vpci: add initial support for virtual PCI bus topology

On Fri, 17 Nov 2023, Volodymyr Babchuk wrote:
> > On Fri, 17 Nov 2023, Volodymyr Babchuk wrote:
> >> Hi Julien,
> >> 
> >> Julien Grall  writes:
> >> 
> >> > Hi Volodymyr,
> >> >
> >> > On 17/11/2023 14:09, Volodymyr Babchuk wrote:
> >> >> Hi Stefano,
> >> >> Stefano Stabellini  writes:
> >> >> 
> >> >>> On Fri, 17 Nov 2023, Volodymyr Babchuk wrote:
> >> > I still think, no matter the BDF allocation scheme, that we should 
> >> > try
> >> > to avoid as much as possible to have two different PCI Root Complex
> >> > emulators. Ideally we would have only one PCI Root Complex emulated 
> >> > by
> >> > Xen. Having 2 PCI Root Complexes both of them emulated by Xen would 
> >> > be
> >> > tolerable but not ideal.
> >> 
> >>  But what is exactly wrong with this setup?
> >> >>>
> >> >>> [...]
> >> >>>
> >> > The worst case I would like to avoid is to have
> >> > two PCI Root Complexes, one emulated by Xen and one emulated by QEMU.
> >> 
> >>  This is how our setup works right now.
> >> >>>
> >> >>> If we have:
> >> >>> - a single PCI Root Complex emulated in Xen
> >> >>> - Xen is safety certified
> >> >>> - individual Virtio devices emulated by QEMU with grants for memory
> >> >>>
> >> >>> We can go very far in terms of being able to use Virtio in safety
> >> >>> use-cases. We might even be able to use Virtio (frontends) in a SafeOS.
> >> >>>
> >> >>> On the other hand if we put an additional Root Complex in QEMU:
> >> >>> - we pay a price in terms of complexity of the codebase
> >> >>> - we pay a price in terms of resource utilization
> >> >>> - we have one additional problem in terms of using this setup with a
> >> >>>SafeOS (one more device emulated by a non-safe component)
> >> >>>
> >> >>> Having 2 PCI Root Complexes both emulated in Xen is a middle ground
> >> >>> solution because:
> >> >>> - we still pay a price in terms of resource utilization
> >> >>> - the code complexity goes up a bit but hopefully not by much
> >> >>> - there is no impact on safety compared to the ideal scenario
> >> >>>
> >> >>> This is why I wrote that it is tolerable.
> >> >> Ah, I see now. Yes, I am agree with this. Also I want to add some
> >> >> more
> >> >> points:
> >> >> - There is ongoing work on implementing virtio backends as a
> >> >> separate
> >> >>applications, written in Rust. Linaro are doing this part. Right now
> >> >>they are implementing only virtio-mmio, but if they want to provide
> >> >>virtio-pci as well, they will need a mechanism to plug only
> >> >>virtio-pci, without Root Complex. This is argument for using single 
> >> >> Root
> >> >>Complex emulated in Xen.
> >> >> - As far as I know (actually, Oleksandr told this to me), QEMU has
> >> >> no
> >> >>mechanism for exposing virtio-pci backends without exposing PCI root
> >> >>complex as well. Architecturally, there should be a PCI bus to which
> >> >>virtio-pci devices are connected. Or we need to make some changes to
> >> >>QEMU internals to be able to create virtio-pci backends that are not
> >> >>connected to any bus. Also, added benefit that PCI Root Complex
> >> >>emulator in QEMU handles legacy PCI interrupts for us. This is
> >> >>argument for separate Root Complex for QEMU.
> >> >> As right now we have only virtio-pci backends provided by QEMU and
> >> >> this
> >> >> setup is already working, I propose to stick to this
> >> >> solution. Especially, taking into account that it does not require any
> >> >> changes to hypervisor code.
> >> >
> >> > I am not against two hostbridge as a temporary solution as long as
> >> > this is not a one way door decision. I am not concerned about the
> >> > hypervisor itself, I am more concerned about the interface exposed by
> >> > the toolstack and QEMU.
> >
> > I agree with this...
> >
> >
> >> > To clarify, I don't particular want to have to maintain the two
> >> > hostbridges solution once we can use a single hostbridge. So we need
> >> > to be able to get rid of it without impacting the interface too much.
> >
> > ...and this
> >
> >
> >> This depends on virtio-pci backends availability. AFAIK, now only one
> >> option is to use QEMU and QEMU provides own host bridge. So if we want
> >> get rid of the second host bridge we need either another virtio-pci
> >> backend or we need to alter QEMU code so it can live without host
> >> bridge.
> >> 
> >> As for interfaces, it appears that QEMU case does not require any changes
> >> into hypervisor itself, it just boils down to writing couple of xenstore
> >> entries and spawning QEMU with correct command line arguments.
> >
> > One thing that Stewart wrote in his reply that is important: it doesn't
> > matter if QEMU thinks it is emulating a PCI Root Complex because that's
> > required from QEMU's point of view to emulate an individual PCI device.
> >
> > If we can arrange it so the QEMU PCI Root Complex is not registered
> > against Xen as part of the ioreq

[xen-unstable test] 183781: tolerable FAIL - PUSHED

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

Failures :-/ but no regressions.

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

version targeted for testing:
 xen  6cd046c501bce48cdc42f597fc7a023aa08853e7
baseline version:
 xen  b739e2067b1a06328e7f0042630b543413689eac

Last test of basis   183775  2023-11-16 16:10:43 Z1 days
Testing same since   183781  2023-11-17 08:56:21 Z0 days1 attempts


People who touched revisions under test:
  Ayan Kumar Halder 
  Henry Wang 
  Julien Grall

Re: [PATCH v10 13/17] vpci: add initial support for virtual PCI bus topology

2023-11-17 Thread Volodymyr Babchuk



Hi Stefano,

Stefano Stabellini  writes:

> On Fri, 17 Nov 2023, Volodymyr Babchuk wrote:
>> Hi Julien,
>> 
>> Julien Grall  writes:
>> 
>> > Hi Volodymyr,
>> >
>> > On 17/11/2023 14:09, Volodymyr Babchuk wrote:
>> >> Hi Stefano,
>> >> Stefano Stabellini  writes:
>> >> 
>> >>> On Fri, 17 Nov 2023, Volodymyr Babchuk wrote:
>> > I still think, no matter the BDF allocation scheme, that we should try
>> > to avoid as much as possible to have two different PCI Root Complex
>> > emulators. Ideally we would have only one PCI Root Complex emulated by
>> > Xen. Having 2 PCI Root Complexes both of them emulated by Xen would be
>> > tolerable but not ideal.
>> 
>>  But what is exactly wrong with this setup?
>> >>>
>> >>> [...]
>> >>>
>> > The worst case I would like to avoid is to have
>> > two PCI Root Complexes, one emulated by Xen and one emulated by QEMU.
>> 
>>  This is how our setup works right now.
>> >>>
>> >>> If we have:
>> >>> - a single PCI Root Complex emulated in Xen
>> >>> - Xen is safety certified
>> >>> - individual Virtio devices emulated by QEMU with grants for memory
>> >>>
>> >>> We can go very far in terms of being able to use Virtio in safety
>> >>> use-cases. We might even be able to use Virtio (frontends) in a SafeOS.
>> >>>
>> >>> On the other hand if we put an additional Root Complex in QEMU:
>> >>> - we pay a price in terms of complexity of the codebase
>> >>> - we pay a price in terms of resource utilization
>> >>> - we have one additional problem in terms of using this setup with a
>> >>>SafeOS (one more device emulated by a non-safe component)
>> >>>
>> >>> Having 2 PCI Root Complexes both emulated in Xen is a middle ground
>> >>> solution because:
>> >>> - we still pay a price in terms of resource utilization
>> >>> - the code complexity goes up a bit but hopefully not by much
>> >>> - there is no impact on safety compared to the ideal scenario
>> >>>
>> >>> This is why I wrote that it is tolerable.
>> >> Ah, I see now. Yes, I am agree with this. Also I want to add some
>> >> more
>> >> points:
>> >> - There is ongoing work on implementing virtio backends as a
>> >> separate
>> >>applications, written in Rust. Linaro are doing this part. Right now
>> >>they are implementing only virtio-mmio, but if they want to provide
>> >>virtio-pci as well, they will need a mechanism to plug only
>> >>virtio-pci, without Root Complex. This is argument for using single 
>> >> Root
>> >>Complex emulated in Xen.
>> >> - As far as I know (actually, Oleksandr told this to me), QEMU has
>> >> no
>> >>mechanism for exposing virtio-pci backends without exposing PCI root
>> >>complex as well. Architecturally, there should be a PCI bus to which
>> >>virtio-pci devices are connected. Or we need to make some changes to
>> >>QEMU internals to be able to create virtio-pci backends that are not
>> >>connected to any bus. Also, added benefit that PCI Root Complex
>> >>emulator in QEMU handles legacy PCI interrupts for us. This is
>> >>argument for separate Root Complex for QEMU.
>> >> As right now we have only virtio-pci backends provided by QEMU and
>> >> this
>> >> setup is already working, I propose to stick to this
>> >> solution. Especially, taking into account that it does not require any
>> >> changes to hypervisor code.
>> >
>> > I am not against two hostbridge as a temporary solution as long as
>> > this is not a one way door decision. I am not concerned about the
>> > hypervisor itself, I am more concerned about the interface exposed by
>> > the toolstack and QEMU.
>
> I agree with this...
>
>
>> > To clarify, I don't particular want to have to maintain the two
>> > hostbridges solution once we can use a single hostbridge. So we need
>> > to be able to get rid of it without impacting the interface too much.
>
> ...and this
>
>
>> This depends on virtio-pci backends availability. AFAIK, now only one
>> option is to use QEMU and QEMU provides own host bridge. So if we want
>> get rid of the second host bridge we need either another virtio-pci
>> backend or we need to alter QEMU code so it can live without host
>> bridge.
>> 
>> As for interfaces, it appears that QEMU case does not require any changes
>> into hypervisor itself, it just boils down to writing couple of xenstore
>> entries and spawning QEMU with correct command line arguments.
>
> One thing that Stewart wrote in his reply that is important: it doesn't
> matter if QEMU thinks it is emulating a PCI Root Complex because that's
> required from QEMU's point of view to emulate an individual PCI device.
>
> If we can arrange it so the QEMU PCI Root Complex is not registered
> against Xen as part of the ioreq interface, then QEMU's emulated PCI
> Root Complex is going to be left unused. I think that would be great
> because we still have a clean QEMU-Xen-tools interface and the only
> downside is some extra unused emulation in QEMU. It would be a
>

Re: [PATCH v10 13/17] vpci: add initial support for virtual PCI bus topology

On Fri, 17 Nov 2023, Volodymyr Babchuk wrote:
> Hi Julien,
> 
> Julien Grall  writes:
> 
> > Hi Volodymyr,
> >
> > On 17/11/2023 14:09, Volodymyr Babchuk wrote:
> >> Hi Stefano,
> >> Stefano Stabellini  writes:
> >> 
> >>> On Fri, 17 Nov 2023, Volodymyr Babchuk wrote:
> > I still think, no matter the BDF allocation scheme, that we should try
> > to avoid as much as possible to have two different PCI Root Complex
> > emulators. Ideally we would have only one PCI Root Complex emulated by
> > Xen. Having 2 PCI Root Complexes both of them emulated by Xen would be
> > tolerable but not ideal.
> 
>  But what is exactly wrong with this setup?
> >>>
> >>> [...]
> >>>
> > The worst case I would like to avoid is to have
> > two PCI Root Complexes, one emulated by Xen and one emulated by QEMU.
> 
>  This is how our setup works right now.
> >>>
> >>> If we have:
> >>> - a single PCI Root Complex emulated in Xen
> >>> - Xen is safety certified
> >>> - individual Virtio devices emulated by QEMU with grants for memory
> >>>
> >>> We can go very far in terms of being able to use Virtio in safety
> >>> use-cases. We might even be able to use Virtio (frontends) in a SafeOS.
> >>>
> >>> On the other hand if we put an additional Root Complex in QEMU:
> >>> - we pay a price in terms of complexity of the codebase
> >>> - we pay a price in terms of resource utilization
> >>> - we have one additional problem in terms of using this setup with a
> >>>SafeOS (one more device emulated by a non-safe component)
> >>>
> >>> Having 2 PCI Root Complexes both emulated in Xen is a middle ground
> >>> solution because:
> >>> - we still pay a price in terms of resource utilization
> >>> - the code complexity goes up a bit but hopefully not by much
> >>> - there is no impact on safety compared to the ideal scenario
> >>>
> >>> This is why I wrote that it is tolerable.
> >> Ah, I see now. Yes, I am agree with this. Also I want to add some
> >> more
> >> points:
> >> - There is ongoing work on implementing virtio backends as a
> >> separate
> >>applications, written in Rust. Linaro are doing this part. Right now
> >>they are implementing only virtio-mmio, but if they want to provide
> >>virtio-pci as well, they will need a mechanism to plug only
> >>virtio-pci, without Root Complex. This is argument for using single Root
> >>Complex emulated in Xen.
> >> - As far as I know (actually, Oleksandr told this to me), QEMU has
> >> no
> >>mechanism for exposing virtio-pci backends without exposing PCI root
> >>complex as well. Architecturally, there should be a PCI bus to which
> >>virtio-pci devices are connected. Or we need to make some changes to
> >>QEMU internals to be able to create virtio-pci backends that are not
> >>connected to any bus. Also, added benefit that PCI Root Complex
> >>emulator in QEMU handles legacy PCI interrupts for us. This is
> >>argument for separate Root Complex for QEMU.
> >> As right now we have only virtio-pci backends provided by QEMU and
> >> this
> >> setup is already working, I propose to stick to this
> >> solution. Especially, taking into account that it does not require any
> >> changes to hypervisor code.
> >
> > I am not against two hostbridge as a temporary solution as long as
> > this is not a one way door decision. I am not concerned about the
> > hypervisor itself, I am more concerned about the interface exposed by
> > the toolstack and QEMU.

I agree with this...


> > To clarify, I don't particular want to have to maintain the two
> > hostbridges solution once we can use a single hostbridge. So we need
> > to be able to get rid of it without impacting the interface too much.

...and this


> This depends on virtio-pci backends availability. AFAIK, now only one
> option is to use QEMU and QEMU provides own host bridge. So if we want
> get rid of the second host bridge we need either another virtio-pci
> backend or we need to alter QEMU code so it can live without host
> bridge.
> 
> As for interfaces, it appears that QEMU case does not require any changes
> into hypervisor itself, it just boils down to writing couple of xenstore
> entries and spawning QEMU with correct command line arguments.

One thing that Stewart wrote in his reply that is important: it doesn't
matter if QEMU thinks it is emulating a PCI Root Complex because that's
required from QEMU's point of view to emulate an individual PCI device.

If we can arrange it so the QEMU PCI Root Complex is not registered
against Xen as part of the ioreq interface, then QEMU's emulated PCI
Root Complex is going to be left unused. I think that would be great
because we still have a clean QEMU-Xen-tools interface and the only
downside is some extra unused emulation in QEMU. It would be a
fantastic starting point.

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

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

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  97f8555acbf3da013ed713ca0bbe739d41c48da9
baseline version:
 xen  6cd046c501bce48cdc42f597fc7a023aa08853e7

Last test of basis   183774  2023-11-16 15:02:22 Z1 days
Testing same since   183784  2023-11-17 19:03:49 Z0 days1 attempts


People who touched revisions under test:
  Julien Grall 
  Volodymyr Babchuk 

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
   6cd046c501..97f8555acb  97f8555acbf3da013ed713ca0bbe739d41c48da9 -> smoke

Re: [PATCH] arm/mm: add option to prefer IOMMU ops for DMA on Xen

On Fri, 17 Nov 2023, Fr. Chuck Zmudzinski, C.P.M. wrote:
> On 11/14/2023 5:20 PM, Stefano Stabellini wrote:
> > On Tue, 14 Nov 2023, Robin Murphy wrote:
> >> On 11/11/2023 6:45 pm, Chuck Zmudzinski wrote:
> >> > Enabling the new option, ARM_DMA_USE_IOMMU_XEN, fixes this error when
> >> > attaching the Exynos mixer in Linux dom0 on Xen on the Chromebook Snow
> >> > (and probably on other devices that use the Exynos mixer):
> >> > 
> >> > [drm] Exynos DRM: using 1440.fimd device for DMA mapping operations
> >> > exynos-drm exynos-drm: bound 1440.fimd (ops 0xc0d96354)
> >> > exynos-mixer 1445.mixer: [drm:exynos_drm_register_dma] *ERROR* Device
> >> >   1445.mixer lacks support for IOMMU
> >> > exynos-drm exynos-drm: failed to bind 1445.mixer (ops 0xc0d97554): 
> >> > -22
> >> > exynos-drm exynos-drm: adev bind failed: -22
> >> > exynos-dp: probe of 145b.dp-controller failed with error -22
> >> > 
> >> > Linux normally uses xen_swiotlb_dma_ops for DMA for all devices when
> >> > xen_swiotlb is detected even when Xen exposes an IOMMU to Linux. Enabling
> >> > the new config option allows devices such as the Exynos mixer to use the
> >> > IOMMU instead of xen_swiotlb_dma_ops for DMA and this fixes the error.
> >> > 
> >> > The new config option is not set by default because it is likely some
> >> > devices that use IOMMU for DMA on Xen will cause DMA errors and memory
> >> > corruption when Xen PV block and network drivers are in use on the 
> >> > system.
> >> > 
> >> > Link:
> >> > https://lore.kernel.org/xen-devel/acfab1c5-eed1-4930-8c70-8681e256c...@netscape.net/
> >> > 
> >> > Signed-off-by: Chuck Zmudzinski 
> >> > ---
> >> > The reported error with the Exynos mixer is not fixed by default by 
> >> > adding
> >> > a second patch to select the new option in the Kconfig definition for the
> >> > Exynos mixer if EXYNOS_IOMMU and SWIOTLB_XEN are enabled because it is
> >> > not certain setting the config option is suitable for all cases. So it is
> >> > necessary to explicitly select the new config option during the config
> >> > stage of the Linux kernel build to fix the reported error or similar
> >> > errors that have the same cause of lack of support for IOMMU on Xen. This
> >> > is necessary to avoid any regressions that might be caused by enabling 
> >> > the
> >> > new option by default for the Exynos mixer.
> >> >   arch/arm/mm/dma-mapping.c |  6 ++
> >> >   drivers/xen/Kconfig   | 16 
> >> >   2 files changed, 22 insertions(+)
> >> > 
> >> > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> >> > index 5409225b4abc..ca04fdf01be3 100644
> >> > --- a/arch/arm/mm/dma-mapping.c
> >> > +++ b/arch/arm/mm/dma-mapping.c
> >> > @@ -1779,6 +1779,12 @@ void arch_setup_dma_ops(struct device *dev, u64
> >> > dma_base, u64 size,
> >> >  if (iommu)
> >> >  arm_setup_iommu_dma_ops(dev, dma_base, size, iommu, 
> >> > coherent);
> >> >   +#ifdef CONFIG_ARM_DMA_USE_IOMMU_XEN
> >> 
> >> FWIW I don't think this really needs a config option - if Xen *has* made an
> >> IOMMU available, then there isn't really much reason not to use it, and if 
> >> for
> >> some reason someone really didn't want to then they could simply disable 
> >> the
> >> IOMMU driver anyway.
> > 
> > The fact that the Exynos IOMMU is exposed to Linux is a mistake. Xen
> > doesn't recognize the Exynos IOMMU (it is not one of the IOMMUs Xen has
> > a driver for) so it assigns the IOMMU to Dom0. It doesn't happen on
> > purpose, it happens by accident. Certain things are going to break,
> > specifically I am fairly certain PV drivers are going to break.
> > 
> > If Xen recognized the Exynos IOMMU as an IOMMU it would probably hide it
> > from Dom0. (Today Xen doesn't have a list of IOMMUs Xen recognizes but
> > doesn't have a driver for.)
> > 
> > I think it is OK for Chuck and others to play around with this
> > configuration but I wouldn't add a new kconfig option to Linux to
> > support it.
> > 
> > If we do want a kconfig option, I would add a kconfig option or Linux
> > command line option to enable/disable swiotlb-xen. Basically a way to
> > force-enable or force-disable xen_swiotlb_detect().That could be
> 
> I am trying to understand what you are proposing.
> 
> I tried disabling the CONFIG_SWIOTLB_XEN option that already
> exists and it does not seem possible to disable that option without
> also totally removing Xen dom0 support from Linux on arm. So do you
> suggest keeping that option as is and adding a Linux command line
> switch or new Linux Kconfig option that is ignored unless
> CONFIG_SWIOTLB_XEN is enabled and would make xen_swiotlb_detect()
> always return false or always return true, depending on the setting
> of the new option?

Yes. I suggest adding a Linux kernel command line option to force-enable
or force-diable swiotlb-xen and that can easily be implemented by
changing xen_swiotlb_detect() to return true/false

Re: [PATCH v10 13/17] vpci: add initial support for virtual PCI bus topology

2023-11-17 Thread Volodymyr Babchuk



Hi Julien,

Julien Grall  writes:

> Hi Volodymyr,
>
> On 17/11/2023 14:09, Volodymyr Babchuk wrote:
>> Hi Stefano,
>> Stefano Stabellini  writes:
>> 
>>> On Fri, 17 Nov 2023, Volodymyr Babchuk wrote:
> I still think, no matter the BDF allocation scheme, that we should try
> to avoid as much as possible to have two different PCI Root Complex
> emulators. Ideally we would have only one PCI Root Complex emulated by
> Xen. Having 2 PCI Root Complexes both of them emulated by Xen would be
> tolerable but not ideal.

 But what is exactly wrong with this setup?
>>>
>>> [...]
>>>
> The worst case I would like to avoid is to have
> two PCI Root Complexes, one emulated by Xen and one emulated by QEMU.

 This is how our setup works right now.
>>>
>>> If we have:
>>> - a single PCI Root Complex emulated in Xen
>>> - Xen is safety certified
>>> - individual Virtio devices emulated by QEMU with grants for memory
>>>
>>> We can go very far in terms of being able to use Virtio in safety
>>> use-cases. We might even be able to use Virtio (frontends) in a SafeOS.
>>>
>>> On the other hand if we put an additional Root Complex in QEMU:
>>> - we pay a price in terms of complexity of the codebase
>>> - we pay a price in terms of resource utilization
>>> - we have one additional problem in terms of using this setup with a
>>>SafeOS (one more device emulated by a non-safe component)
>>>
>>> Having 2 PCI Root Complexes both emulated in Xen is a middle ground
>>> solution because:
>>> - we still pay a price in terms of resource utilization
>>> - the code complexity goes up a bit but hopefully not by much
>>> - there is no impact on safety compared to the ideal scenario
>>>
>>> This is why I wrote that it is tolerable.
>> Ah, I see now. Yes, I am agree with this. Also I want to add some
>> more
>> points:
>> - There is ongoing work on implementing virtio backends as a
>> separate
>>applications, written in Rust. Linaro are doing this part. Right now
>>they are implementing only virtio-mmio, but if they want to provide
>>virtio-pci as well, they will need a mechanism to plug only
>>virtio-pci, without Root Complex. This is argument for using single Root
>>Complex emulated in Xen.
>> - As far as I know (actually, Oleksandr told this to me), QEMU has
>> no
>>mechanism for exposing virtio-pci backends without exposing PCI root
>>complex as well. Architecturally, there should be a PCI bus to which
>>virtio-pci devices are connected. Or we need to make some changes to
>>QEMU internals to be able to create virtio-pci backends that are not
>>connected to any bus. Also, added benefit that PCI Root Complex
>>emulator in QEMU handles legacy PCI interrupts for us. This is
>>argument for separate Root Complex for QEMU.
>> As right now we have only virtio-pci backends provided by QEMU and
>> this
>> setup is already working, I propose to stick to this
>> solution. Especially, taking into account that it does not require any
>> changes to hypervisor code.
>
> I am not against two hostbridge as a temporary solution as long as
> this is not a one way door decision. I am not concerned about the
> hypervisor itself, I am more concerned about the interface exposed by
> the toolstack and QEMU.
>
> To clarify, I don't particular want to have to maintain the two
> hostbridges solution once we can use a single hostbridge. So we need
> to be able to get rid of it without impacting the interface too much.

This depends on virtio-pci backends availability. AFAIK, now only one
option is to use QEMU and QEMU provides own host bridge. So if we want
get rid of the second host bridge we need either another virtio-pci
backend or we need to alter QEMU code so it can live without host
bridge.

As for interfaces, it appears that QEMU case does not require any changes
into hypervisor itself, it just boils down to writing couple of xenstore
entries and spawning QEMU with correct command line arguments.

>From the user perspective, all this is configured via xl.conf entry like

virtio=[
'backend=DomD, type=virtio,device, transport=pci, bdf=:00:01.0, 
grant_usage=1, backend_type=qemu',
]

In the future we can add backend_type=standalone for non-QEMU-based
backends. If there will be no QEMU-based backends, there will be no
second host bridge.

-- 
WBR, Volodymyr

xen | Successful pipeline for staging | 97f8555a

2023-11-17 Thread GitLab



Pipeline #1076704625 has passed!

Project: xen ( https://gitlab.com/xen-project/xen )
Branch: staging ( https://gitlab.com/xen-project/xen/-/commits/staging )

Commit: 97f8555a ( 
https://gitlab.com/xen-project/xen/-/commit/97f8555acbf3da013ed713ca0bbe739d41c48da9
 )
Commit Message: xenstored: print domain id in traces

It is ver...
Commit Author: Volodymyr Babchuk
Committed by: Julien Grall



Pipeline #1076704625 ( 
https://gitlab.com/xen-project/xen/-/pipelines/1076704625 ) triggered by Ganis 
( https://gitlab.com/ganis )
successfully completed 129 jobs in 3 stages.

-- 
You're receiving this email because of your account on gitlab.com.

[PATCH] xen/ppc: Enable Boot Allocator

2023-11-17 Thread Shawn Anastasio

Adapt arm's earlyfdt parsing code to ppc64 and enable Xen's early boot
allocator. Routines for parsing arm-specific devicetree nodes (e.g.
multiboot) were excluded, reducing the overall footprint of code that
was copied.

Signed-off-by: Shawn Anastasio 
---
 xen/arch/ppc/Makefile|   1 +
 xen/arch/ppc/bootfdt.c   | 507 +++
 xen/arch/ppc/include/asm/setup.h | 113 +++
 xen/arch/ppc/setup.c | 109 ++-
 4 files changed, 729 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/ppc/bootfdt.c

diff --git a/xen/arch/ppc/Makefile b/xen/arch/ppc/Makefile
index 71feb5e2c4..8a2a809c70 100644
--- a/xen/arch/ppc/Makefile
+++ b/xen/arch/ppc/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_PPC64) += ppc64/
 
+obj-y += bootfdt.o
 obj-$(CONFIG_EARLY_PRINTK) += early_printk.init.o
 obj-y += mm-radix.o
 obj-y += opal.o
diff --git a/xen/arch/ppc/bootfdt.c b/xen/arch/ppc/bootfdt.c
new file mode 100644
index 00..791e1ca61f
--- /dev/null
+++ b/xen/arch/ppc/bootfdt.c
@@ -0,0 +1,507 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Early Device Tree and boot info bookkeeping.
+ * Derived from arch/arm/bootfdt.c and setup.c.
+ *
+ * Copyright (C) 2012-2014 Citrix Systems, Inc.
+ * Copyright (C) Raptor Engineering LLC
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct bootinfo __initdata bootinfo;
+
+struct bootmodule __init *add_boot_module(bootmodule_kind kind,
+  paddr_t start, paddr_t size,
+  bool domU)
+{
+struct bootmodules *mods = 
+struct bootmodule *mod;
+unsigned int i;
+
+if ( mods->nr_mods == MAX_MODULES )
+{
+printk("Ignoring %s boot module at %"PRIpaddr"-%"PRIpaddr" (too 
many)\n",
+   boot_module_kind_as_string(kind), start, start + size);
+return NULL;
+}
+
+if ( check_reserved_regions_overlap(start, size) )
+return NULL;
+
+for ( i = 0 ; i < mods->nr_mods ; i++ )
+{
+mod = >module[i];
+if ( mod->kind == kind && mod->start == start )
+{
+if ( !domU )
+mod->domU = false;
+return mod;
+}
+}
+
+mod = >module[mods->nr_mods++];
+mod->kind = kind;
+mod->start = start;
+mod->size = size;
+mod->domU = domU;
+
+return mod;
+}
+
+const char * __init boot_module_kind_as_string(bootmodule_kind kind)
+{
+switch ( kind )
+{
+case BOOTMOD_XEN: return "Xen";
+case BOOTMOD_FDT: return "Device Tree";
+case BOOTMOD_KERNEL:  return "Kernel";
+default: BUG();
+}
+}
+
+/*
+ * TODO: '*_end' could be 0 if the module/region is at the end of the physical
+ * address space. This is for now not handled as it requires more rework.
+ */
+static bool __init bootmodules_overlap_check(struct bootmodules *bootmodules,
+ paddr_t region_start,
+ paddr_t region_size)
+{
+paddr_t mod_start = INVALID_PADDR, mod_end = 0;
+paddr_t region_end = region_start + region_size;
+unsigned int i, mod_num = bootmodules->nr_mods;
+
+for ( i = 0; i < mod_num; i++ )
+{
+mod_start = bootmodules->module[i].start;
+mod_end = mod_start + bootmodules->module[i].size;
+
+if ( region_end <= mod_start || region_start >= mod_end )
+continue;
+else
+{
+printk("Region: [%#"PRIpaddr", %#"PRIpaddr") overlapping with"
+   " mod[%u]: [%#"PRIpaddr", %#"PRIpaddr")\n", region_start,
+   region_end, i, mod_start, mod_end);
+return true;
+}
+}
+
+return false;
+}
+
+/*
+ * TODO: '*_end' could be 0 if the bank/region is at the end of the physical
+ * address space. This is for now not handled as it requires more rework.
+ */
+static bool __init meminfo_overlap_check(struct meminfo *meminfo,
+ paddr_t region_start,
+ paddr_t region_size)
+{
+paddr_t bank_start = INVALID_PADDR, bank_end = 0;
+paddr_t region_end = region_start + region_size;
+unsigned int i, bank_num = meminfo->nr_banks;
+
+for ( i = 0; i < bank_num; i++ )
+{
+bank_start = meminfo->bank[i].start;
+bank_end = bank_start + meminfo->bank[i].size;
+
+if ( region_end <= bank_start || region_start >= bank_end )
+continue;
+else
+{
+printk("Region: [%#"PRIpaddr", %#"PRIpaddr") overlapping with"
+   " bank[%u]: [%#"PRIpaddr", %#"PRIpaddr")\n", region_start,
+   region_end, i, bank_start, bank_end);
+return true;
+}
+}
+
+return false;
+}
+
+/*
+ * Given an input physical address range, check if this range is overlapping
+ * with the existing

Re: [XEN PATCH v4 1/2] automation/eclair: make the docs for MISRA C:2012 Dir 4.1 visible to ECLAIR


Hi,

On 16/11/2023 08:45, Nicola Vetrini wrote:

On 2023-11-15 12:22, Julien Grall wrote:

Hi,

On 15/11/2023 11:02, Nicola Vetrini wrote:

On 2023-11-14 23:12, Julien Grall wrote:

Hi,

On 14/11/2023 15:36, Nicola Vetrini wrote:

To be able to check for the existence of the necessary subsections in
the documentation for MISRA C:2012 Dir 4.1, ECLAIR needs to have a 
source

file that is built.

This file is generated from 'C-runtime-failures.rst' in docs/misra
and the configuration is updated accordingly.

Signed-off-by: Nicola Vetrini 
---
Changes from RFC:
- Dropped unused/useless code
- Revised the sed command
- Revised the clean target

Changes in v2:
- Added explanative comment to the makefile
- printf instead of echo

Changes in v3:
- Terminate the generated file with a newline
- Build it with -std=c99, so that the documentation
   for D1.1 applies.
Changes in v5:
- Transform and build the file directly in the eclair-specific 
directory

---
  automation/eclair_analysis/build.sh   | 21 +++--
  automation/eclair_analysis/prepare.sh |  7 ---
  2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/automation/eclair_analysis/build.sh 
b/automation/eclair_analysis/build.sh

index ec087dd822fa..f24292ed0643 100755
--- a/automation/eclair_analysis/build.sh
+++ b/automation/eclair_analysis/build.sh
@@ -33,12 +33,29 @@ else
    PROCESSORS=6
  fi
  +runtime_failures_docs() {
+  doc="C-runtime-failures.rst"
+  builddir="automation/eclair_analysis"
+
+  cp "docs/misra/${doc}" "${builddir}"


Is it necessary to copy the .rst? IOW, would it be sufficient to use...


+  cd "${builddir}"
+  printf "/*\n\n" >"${doc}.tmp"
+  sed -e 's|\*/|*//*|g' "${doc}" >>"${doc}.tmp"


... docs/misc/${doc} here?



I didn't want to leave a stray file under docs/misra, but it's not 
essential.


I am confused. I am suggesting to use:

sed -e 's|\*/|*//*|g' "../../docs/misc/${doc}" >> "${doc}.tmp"

So *.tmp is still created at the same place.



Ok, makes sense.




+  printf "\n\n*/\n" >>"${doc}.tmp"
+  mv "${doc}.tmp" "${doc}.c"


NIT: I am not sure why you need to first create .tmp and then create.c.



Wasn't this a pattern to defend against interruptions of the build, 
just as I did in v3?


+$(TARGETS:.o=.c): %.c: %.rst
+    printf "/*\n\n" > $@.tmp
+    sed -e 's|\*/|*//*|g' $< >> $@.tmp
+    printf "\n\n*/\n" >> $@.tmp
+    mv $@.tmp $@


Yes but it makes sense for the Makefile because the target would not 
be re-executed if *.c exists.


But I don't think this is the case for you because you are using a 
bash script. So your function should always be re-executed regardless 
on whether it was interrupted or not.




Ok.




+
+  # Cannot redirect to /dev/null because it would be excluded from 
the analysis

+  "${CROSS_COMPILE}gcc-12" -std=c99 -c "${doc}.c" -o "${doc}.o"


NIT: It would be helpful to specify why -std=c99 is used. Above, you 
suggest this is to enable D1.1.




Yeah, the comment in the changelog should be pasted here


NIT: Can we define CC and use here and ...


+  cd -
+}
+
  (
-  cd xen
+  runtime_failures_docs
  make "-j${PROCESSORS}" "-l${PROCESSORS}.0"    \
 "CROSS_COMPILE=${CROSS_COMPILE}" \
 "CC=${CROSS_COMPILE}gcc-12"  \


...? This would make easier to re-use the code.



I don't expect this build script to be changed much to be honest, but 
if you think

this is beneficial then it's ok.


This is not only about code evolving. It makes easier to spot your are 
using the same compiler. I would not have made the remark if you were 
using 'gcc'.


But I noticed you were using gcc-12 and originally thought it was a 
mistake until I saw the second use.


The advantage of a variable CC (and CXX) is you can add a comment on 
top why you are specifically requestion gcc-12? IOW, why is gcc not fine?




The assumptions in C-language-toolchain.rst (which are reflected in the 
analysis config) are using gcc-12 explicitly; that's just easier from a 
certification perspective to have a fixed version.


I am not against fixed version. It just needs to be documented. At least 
reading C-language-toolchain.rst, it is not obvious to me that this is 
only applying to GCC-12.


Cheers,

--
Julien Grall

Re: [XEN PATCH v3] xen: replace occurrences of SAF-1-safe with asmlinkage attribute

Hi Nicola,

On 16/11/2023 09:15, Nicola Vetrini wrote:

On 2023-11-16 10:08, Nicola Vetrini wrote:

The comment-based justifications for MISRA C:2012 Rule 8.4 are replaced
by the asmlinkage pseudo-attribute, for the sake of uniformity.

Add missing 'xen/compiler.h' #include-s where needed.

The text in docs/misra/deviations.rst and docs/misra/safe.json
is modified to reflect this change.

Signed-off-by: Nicola Vetrini
---
This patch should be applied after patch 2 of this series.
The request made by Julien to update the wording is
contained in the present patch.
https://lore.kernel.org/all/9ad7f6210c15f520297aac00e8af0...@bugseng.com/

Concerns about efi_multiboot2 will be dealt with separately.

Changes in v2:
- Edit safe.json.
- Remove mention of SAF-1-safe in deviations.rst.
Changes in v3:
- Sorted #include-s and rebased against
7ad0c774e474 ("x86/boot: tidy #include-s")
---
docs/misra/deviations.rst | 5 ++---
docs/misra/safe.json | 2 +-
xen/arch/arm/cpuerrata.c | 7 +++
xen/arch/arm/setup.c | 5 ++---
xen/arch/arm/smpboot.c | 3 +--
xen/arch/arm/traps.c | 21 +++--
xen/arch/x86/boot/cmdline.c | 5 +++--
xen/arch/x86/boot/reloc.c | 6 +++---
xen/arch/x86/extable.c | 3 +--
xen/arch/x86/setup.c | 3 +--
xen/arch/x86/traps.c | 27 +--
xen/common/efi/boot.c | 5 ++---
12 files changed, 35 insertions(+), 57 deletions(-)

In hindsight I should have added an

Acked-by: Julien Grall

given that the comment has been addressed in my opinion.

I am a bit confused how you considered it was addressed. I see no update
in safe.json when I clearly asked for some (I wouldn't have bothered to
comment in v2 otherwise and just gave an ack).

To be explicit, I requested to:
1. update the description in [1] to clarify that SAF-1 is deprecated.
2. This patch is rebased on top and therefore remove completely the
mention of SAF-1.

I am well-aware that the end result is technically the same. But patches
are meant to be self-contained so if we revert the latest, then the
meaning is still the same.

This patch is unlikely to be removed and this is now the nth time I
asked it the same (maybe it was not clear enough?). So I am going to
content with the current proposal because this is not worth to go
further. But I will at least express my discontent how this is handled.

TBH, there are far too many MISRA patches on the ML spread across
multiple threads. Some are based on top of the others. This makes
extremely difficult to follow and know what is addressed or not. Can we
at least try to condense some of work in similar area in the same
series? For instance, this patch could have been included in the other
series [1].

Lastly, right now, I have 300 emails (31 threads) with MISRA in the
title in my inbox. It is a little unclear what has been committed/review
or require input. I am concerned to miss key series (the patch to
compile in docs/ was nearly missed).

Do we track anywhere which series are still inflights? Can we consider
to pause or at least slow down the rate of new MISRA patches until the
backlog is cleared? (Adding more patches is not really helping).

Cheers,

[1]
https://lore.kernel.org/all/a1b5c3b273145c35535fed3647bf850d9ae5db7f.1698829473.git.nicola.vetr...@bugseng.com/

I pointed out that the patch in

--
Julien Grall

Re: [PATCH v10 13/17] vpci: add initial support for virtual PCI bus topology


Hi Volodymyr,

On 17/11/2023 14:09, Volodymyr Babchuk wrote:


Hi Stefano,

Stefano Stabellini  writes:


On Fri, 17 Nov 2023, Volodymyr Babchuk wrote:

I still think, no matter the BDF allocation scheme, that we should try
to avoid as much as possible to have two different PCI Root Complex
emulators. Ideally we would have only one PCI Root Complex emulated by
Xen. Having 2 PCI Root Complexes both of them emulated by Xen would be
tolerable but not ideal.


But what is exactly wrong with this setup?


[...]


The worst case I would like to avoid is to have
two PCI Root Complexes, one emulated by Xen and one emulated by QEMU.


This is how our setup works right now.


If we have:
- a single PCI Root Complex emulated in Xen
- Xen is safety certified
- individual Virtio devices emulated by QEMU with grants for memory

We can go very far in terms of being able to use Virtio in safety
use-cases. We might even be able to use Virtio (frontends) in a SafeOS.

On the other hand if we put an additional Root Complex in QEMU:
- we pay a price in terms of complexity of the codebase
- we pay a price in terms of resource utilization
- we have one additional problem in terms of using this setup with a
   SafeOS (one more device emulated by a non-safe component)

Having 2 PCI Root Complexes both emulated in Xen is a middle ground
solution because:
- we still pay a price in terms of resource utilization
- the code complexity goes up a bit but hopefully not by much
- there is no impact on safety compared to the ideal scenario

This is why I wrote that it is tolerable.


Ah, I see now. Yes, I am agree with this. Also I want to add some more
points:

- There is ongoing work on implementing virtio backends as a separate
   applications, written in Rust. Linaro are doing this part. Right now
   they are implementing only virtio-mmio, but if they want to provide
   virtio-pci as well, they will need a mechanism to plug only
   virtio-pci, without Root Complex. This is argument for using single Root
   Complex emulated in Xen.

- As far as I know (actually, Oleksandr told this to me), QEMU has no
   mechanism for exposing virtio-pci backends without exposing PCI root
   complex as well. Architecturally, there should be a PCI bus to which
   virtio-pci devices are connected. Or we need to make some changes to
   QEMU internals to be able to create virtio-pci backends that are not
   connected to any bus. Also, added benefit that PCI Root Complex
   emulator in QEMU handles legacy PCI interrupts for us. This is
   argument for separate Root Complex for QEMU.

As right now we have only virtio-pci backends provided by QEMU and this
setup is already working, I propose to stick to this
solution. Especially, taking into account that it does not require any
changes to hypervisor code.


I am not against two hostbridge as a temporary solution as long as this 
is not a one way door decision. I am not concerned about the hypervisor 
itself, I am more concerned about the interface exposed by the toolstack 
and QEMU.


To clarify, I don't particular want to have to maintain the two 
hostbridges solution once we can use a single hostbridge. So we need to 
be able to get rid of it without impacting the interface too much.


Cheers,

--
Julien Grall

Re: [RFC PATCH 2/6] xen/public: arch-arm: reserve resources for virtio-pci


Hi Sergiy,

On 17/11/2023 13:19, Sergiy Kibrik wrote:

+ */
+#define GUEST_VIRTIO_PCI_ECAM_BASE  xen_mk_ullong(0x3300)
+#define GUEST_VIRTIO_PCI_TOTAL_ECAM_SIZE    xen_mk_ullong(0x0100)
+#define GUEST_VIRTIO_PCI_HOST_ECAM_SIZE xen_mk_ullong(0x0020)
+
+/* 64 MB is reserved for virtio-pci memory */
+#define GUEST_VIRTIO_PCI_ADDR_TYPE_MEM    xen_mk_ullong(0x0200)
+#define GUEST_VIRTIO_PCI_MEM_ADDR xen_mk_ullong(0x3400)
+#define GUEST_VIRTIO_PCI_MEM_SIZE xen_mk_ullong(0x0400)
+
   /*
    * 16MB == 4096 pages reserved for guest to use as a region to 
map its

    * grant table in.
@@ -476,6 +489,11 @@ typedef uint64_t xen_callback_t;
   #define GUEST_MAGIC_BASE  xen_mk_ullong(0x3900)
   #define GUEST_MAGIC_SIZE  xen_mk_ullong(0x0100)
+/* 64 MB is reserved for virtio-pci Prefetch memory */


This doesn't seem a lot depending on your use case. Can you details how
you can up with "64 MB"?


the same calculation as it was done configuration space. It was observed
that only 16K is used per virtio-pci device (maybe it can be bigger for
usual PCI device, I don't know). Please look at the example of DomU log
below (to strings that contain "*BAR 4: assigned*"):


What about virtio-gpu? I would expect a bit more memory is necessary 
for that use case.


Any case, what I am looking for is for some explanation in the commit 
message of the limits. I don't particularly care about the exact limit 
because this is not part of a stable ABI.


sure, I'll put a bit more explanation in both comment and commit 
message. Should I post updated patch series, with updated resources and 
without patch #5, or shall we wait for some more comments here?


I would wait for comments before posting in particular if you haven't 
yet received any comment on the tools side.


Cheers,

--
Julien Grall

Re: [PATCH v7 1/2] xen/vpci: header: status register handler

On Fri, Nov 17, 2023 at 01:40:37PM +0100, Roger Pau Monné wrote:
> On Wed, Sep 13, 2023 at 10:35:46AM -0400, Stewart Hildebrand wrote:
> >  {
> > -uint32_t val;
> > -
> >  val = r->read(pdev, r->offset, r->private);
> > +val &= ~r->rw1c_mask;
> >  data = merge_result(val, data, size, offset);
> >  }
> >  
> > +data &= ~(r->rsvdz_mask | r->ro_mask);
> > +data |= val & r->ro_mask;
> 
> You cannot apply the register masks directly into the final value, you
> need to offset and mask them as necessary, likewise for val, see
> what's done in merge_result().

Never mind, I was wrong, there's no need to offset anything here.

Roger.

[libvirt test] 183779: tolerable all pass - PUSHED

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

Failures :-/ but no regressions.

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

version targeted for testing:
 libvirt  3ad5817053aa779c9536a624f260774b7afb64f6
baseline version:
 libvirt  1d456e18c796735c88c68742ff55314b114ad25e

Last test of basis   183734  2023-11-11 04:20:33 Z6 days
Testing same since   183779  2023-11-17 04:20:32 Z0 days1 attempts


People who touched revisions under test:
  Pavel Hrdina 

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



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

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

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

Re: Devise macros to encapsulate (x & -x)

2023-11-17 Thread Nicola Vetrini


On 2023-11-17 11:17, Nicola Vetrini wrote:

Hi all,

As discussed in this thread [1], which is about complying with MISRA C 
Rule 10.1,

a macro was introduced to encapsulate a well-known construct:

/*
 * Given an unsigned integer argument, expands to a mask where just the 
least
 * significant nonzero bit of the argument is set, or 0 if no bits are 
set.

 */
#define ISOLATE_LSB(x) ((x) & -(x))

This macro has a gained some calls in the subsequent patches in that 
thread, but concerns were raised around the fact that it would be 
better to devise a macro that evaluates its argument only once. A 
proposed solution is this (thanks to Jan Beulich):


#define ISOLATE_LSB(x) ({ \
 typeof(x) x_ = (x); \
 x_ & -x_; \
})

However, it can't be used in all call sites that the previous macro 
would have once that series is committed, as can be seen in [2]. 
Therefore, while the implementation looks ok,
a case has been made to have separate macros, of which the latter form 
is preferred.


The following points require some thought:

- where the single evaluation macro should be placed?
  One proposed location is xen/include/xen/bitops.h
- is the proposed form actually the best, or maybe it could be an 
inline function?


Then testing can happen and a subsequent version of those uncommitted 
patches introducing uses of either construct will be submitted, to 
modify all the call sites only once in the commit history.


Let me know what you think of this.
Regards,

[1] 
https://lore.kernel.org/xen-devel/8a1313b3ab5ba6dd556cf37409e3b...@bugseng.com/T/#mdeb510325e1acacb6477a88de8577e9e87351ba5

[2] https://gitlab.com/xen-project/people/bugseng/xen/-/jobs/5423693947


I did a few tests; the only places where the first form can't be 
replaced are:


- inside BUILD_BUG_ON (two instances in x86/x86_64/mm.c)
- the definition of MASK_EXTR/MASK_INSR
- the definition of PDX_GROUP_COUNT

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

Re: Devise macros to encapsulate (x & -x)

2023-11-17 Thread Nicola Vetrini


On 2023-11-17 12:39, Jan Beulich wrote:

On 17.11.2023 11:17, Nicola Vetrini wrote:

Hi all,

As discussed in this thread [1], which is about complying with MISRA C
Rule 10.1,
a macro was introduced to encapsulate a well-known construct:

/*
  * Given an unsigned integer argument, expands to a mask where just 
the

least
  * significant nonzero bit of the argument is set, or 0 if no bits 
are

set.
  */
#define ISOLATE_LSB(x) ((x) & -(x))

This macro has a gained some calls in the subsequent patches in that
thread, but concerns were raised around the fact that it would be 
better

to devise a macro that evaluates its argument only once. A proposed
solution is this (thanks to Jan Beulich):

#define ISOLATE_LSB(x) ({ \
  typeof(x) x_ = (x); \
  x_ & -x_; \
})

However, it can't be used in all call sites that the previous macro
would have once that series is committed, as can be seen in [2].
Therefore, while the implementation looks ok,
a case has been made to have separate macros, of which the latter form
is preferred.

The following points require some thought:

- where the single evaluation macro should be placed?
   One proposed location is xen/include/xen/bitops.h


Or next to the existing one in macros.h. I can see pros and cons for 
either.


- is the proposed form actually the best, or maybe it could be an 
inline

function?


How would you make such a function type generic?



Ah, yes indeed this wouldn't work.

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

[linux-linus test] 183778: regressions - FAIL

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-arm64-arm64-examine  8 reboot   fail REGR. vs. 183766
 test-arm64-arm64-libvirt-xsm  8 xen-boot fail REGR. vs. 183766
 test-arm64-arm64-xl   8 xen-boot fail REGR. vs. 183766
 test-arm64-arm64-xl-credit2   8 xen-boot fail REGR. vs. 183766
 test-arm64-arm64-xl-thunderx  8 xen-boot fail REGR. vs. 183766
 test-arm64-arm64-xl-xsm   8 xen-boot fail REGR. vs. 183766
 test-arm64-arm64-xl-vhd   8 xen-boot fail REGR. vs. 183766
 test-arm64-arm64-xl-credit1   8 xen-boot fail REGR. vs. 183766
 test-arm64-arm64-libvirt-raw  8 xen-boot fail REGR. vs. 183766

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

version targeted for testing:
 linux7475e51b87969e01a6812eac713a1c8310372e8a
baseline version:
 linuxc42d9eeef8e5ba9292eda36fd8e3c11f35ee065c

Last test of basis   183766  2023-11-15 17:14:16 Z1 days
Testing same since   183773  2023-11-16 13:12:48 Z1 days2 attempts


People who touched revisions under test:
  "Michael S. Tsirkin" 
  Aleksandr Loktionov 
  Alex Pakhunov 
  Alexei Starovoitov 
  Alistair Francis 
  Anders Roxell 
  Andrii Nakryiko 
  Arkadiusz Kubalewski 
  Arpana Arland  (A Contingent worker at Intel)
  Baruch Siach 
  Björn Töpel 
  ChunHao Lin 
  Clément Léger 
  Dan Carpenter 
  Dan Nowlin 
  David S. Miller 
  David Woodhouse 
  Dust Li 
  Eduard Zingerman 
  Erez Shitrit 
  Eric Dumazet 
  Eugenio Pérez 
  Gal Pressman 
  Gavin Li 
  Geliang Tang 
  Hou Tao 
  Itamar Gozlan 
  Jakub Kicinski 
  Jakub Sitnicki 
  Jan Kiszka 
  Jason Andryuk 
  Jason Wang 
  Jay Vosburgh 
  Jian Shen 
  Jianbo Liu 
  Jijie Shao 
  Johnathan Mantey 
  Jozsef Kadlecsik 
  Juergen Gross 
  Linkui Xiao 
  Linus Torvalds 
  Linus Walleij 
  Lorenzo Bianconi 
  Maciej Fijalkowski 
  Maher Sanalla 
  Marek Behún 
  Matthieu Baerts 
  MD Danish Anwar 
  Michael S. Tsirkin 
  Niklas Söderlund 
  Pablo Neira Ayuso 
  Paolo Abeni 
  Paul Greenwalt 
  Rahul Rameshbabu 
  Randy Dunlap 
  Ravi Gunasekaran 
  Richard Cochran 
  Roger Pau Monne 
  Roger Pau Monné 
  Saeed Mahameed 
  Shannon Nelson 
  Shigeru Yoshida 
  Stanislav Fomichev 
  Stefano Garzarella 
  Stefano Stabellini 
  Sunitha

[XEN v4 2/2] xen/arm32: head Split and move MMU-specific head.S to mmu/head.S

2023-11-17 Thread Ayan Kumar Halder

The MMU specific code in head.S will not be used on MPU systems.
Instead of introducing more #ifdefs which will bring complexity
to the code, move MMU related code to mmu/head.S and keep common
code in head.S. Two notes while moving:
 - As "fail" in original head.S is very simple and this name is too
   easy to be conflicted, duplicate it in mmu/head.S instead of
   exporting it.
 - Realigned ".macro ret" so that the alignment matches to the other
   macros.
 - Rename puts to asm_puts, putn to asm_putn (this denotes that the
   macros are used within the context of assembly only).
 - Use ENTRY() for enable_secondary_cpu_mm, enable_boot_cpu_mm,
   setup_fixmap, asm_puts, asm_putn  as they will be used externally.

Also move the assembly macros shared by head.S and mmu/head.S to
macros.h.

This is based on 6734327d76be ("xen/arm64: Split and move MMU-specific head.S 
to mmu/head.S").

Signed-off-by: Ayan Kumar Halder 
---

Changes from v1 :-

1. Added a commit message
2. Moved load_paddr to mmu/head.S

v2 :-

1. Renamed puts to asm_puts and putn to asm_putn. Exported asm_putn().
2. Moved XEN_TEMPORARY_OFFSET to head.S.
3. Some style issues.

v3 :-

1. Updated the comments on top of asm_puts() and asm_putn().
2. Removed some stubs.
3. PRINT() invokes asm_puts.

 xen/arch/arm/arm32/head.S   | 630 +---
 xen/arch/arm/arm32/mmu/Makefile |   1 +
 xen/arch/arm/arm32/mmu/head.S   | 573 +
 xen/arch/arm/include/asm/arm32/macros.h |  58 ++-
 4 files changed, 638 insertions(+), 624 deletions(-)
 create mode 100644 xen/arch/arm/arm32/mmu/head.S

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index b3f6d111b0..8cb5aef24e 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -22,86 +22,10 @@
 
 #define ZIMAGE_MAGIC_NUMBER 0x016f2818
 
-#define PT_PT 0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
-#define PT_MEM0xf7d /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=0 P=1 */
-#define PT_MEM_L3 0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
-#define PT_DEV0xe71 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=0 P=1 */
-#define PT_DEV_L3 0xe73 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=1 P=1 */
-
-#define PT_UPPER(x) (PT_##x & 0xf00)
-#define PT_LOWER(x) (PT_##x & 0x0ff)
-
-/* Convenience defines to get slot used by Xen mapping. */
-#define XEN_FIRST_SLOT  first_table_offset(XEN_VIRT_START)
-#define XEN_SECOND_SLOT second_table_offset(XEN_VIRT_START)
-
-/* Offset between the early boot xen mapping and the runtime xen mapping */
-#define XEN_TEMPORARY_OFFSET  (TEMPORARY_XEN_VIRT_START - XEN_VIRT_START)
-
 #if defined(CONFIG_EARLY_PRINTK) && defined(CONFIG_EARLY_PRINTK_INC)
 #include CONFIG_EARLY_PRINTK_INC
 #endif
 
-/*
- * Move an immediate constant into a 32-bit register using movw/movt
- * instructions.
- */
-.macro mov_w reg, word
-movw  \reg, #:lower16:\word
-movt  \reg, #:upper16:\word
-.endm
-
-/*
- * Pseudo-op for PC relative adr ,  where  is
- * within the range +/- 4GB of the PC.
- *
- * @dst: destination register
- * @sym: name of the symbol
- */
-.macro adr_l, dst, sym
-mov_w \dst, \sym - .Lpc\@
-.set  .Lpc\@, .+ 8  /* PC bias */
-add   \dst, \dst, pc
-.endm
-
-.macro load_paddr rb, sym
-mov_w \rb, \sym
-add   \rb, \rb, r10
-.endm
-
-/*
- * Flush local TLBs
- *
- * @tmp: Scratch register
- *
- * See asm/arm32/flushtlb.h for the explanation of the sequence.
- */
-.macro flush_xen_tlb_local tmp
-dsb   nshst
-mcr   CP32(\tmp, TLBIALLH)
-dsb   nsh
-isb
-.endm
-
-/*
- * Enforce Xen page-tables do not contain mapping that are both
- * Writable and eXecutables.
- *
- * This should be called on each secondary CPU.
- */
-.macro pt_enforce_wxn tmp
-mrc   CP32(\tmp, HSCTLR)
-orr   \tmp, \tmp, #SCTLR_Axx_ELx_WXN
-dsb
-mcr   CP32(\tmp, HSCTLR)
-/*
- * The TLBs may cache SCTLR_EL2.WXN. So ensure it is synchronized
- * before flushing the TLBs.
- */
-isb
-flush_xen_tlb_local \tmp
-.endm
-
 /*
  * Common register usage in this file:
  *   r0  -
@@ -121,38 +45,6 @@
  *   r14 - LR
  *   r15 - PC
  */
-#ifdef CONFIG_EARLY_PRINTK
-/*
- * Macro to print a string to the UART, if there is one.
- *
- * Clobbers r0 - r3
- */
-#define PRINT(_s)   \
-mov   r3, lr   ;\
-adr_l r0, 98f  ;\
-blputs ;\
-mov   lr, r3   ;\
-RODATA_STR(98, _s)
-
-/*
- * Macro to print the value of register \rb
- *
- * Clobbers r0 - r4
- */
-.macro print_reg rb
-mov   r0, \rb
-mov   r4, lr
-blputn
-mov   lr, r4
-.endm
-
-#else /* CONFIG_EARLY_PRINTK */
-#define PRINT(s)
-
-.macro print_reg rb
-.endm
-
-#endif /* !CONFIG_EARLY_PRINTK */
 
 .section .text.header, "ax", %progbits
 .arm
@@ -355,480 +247,6 @@ cpu_init_done:
 mov   pc, r5

[XEN v4 0/2] xen/arm: Split MMU code as the prepration of MPU work form Arm32

2023-11-17 Thread Ayan Kumar Halder

Hi,


These are the set of patches based on top of
"[PATCH v9 0/8] xen/arm: Split MMU code as the prepration of MPU work".
This is the preparation work to add MPU support on Arm32.

Changes from :-

v1 - Dropped "[XEN v1 1/4] xen/arm: arm32: Move pt_enforce_wxn() so that it can 
be bundled with other MMU functionality"
and "[XEN v1 4/4] xen/arm: traps.c: Enclose VMSA specific registers within 
CONFIG_MMU".

v2 - Changes mentioned in individual patches.

v3 - Changes mentioned in individual patches.

Ayan Kumar Halder (2):
  xen/arm32: head: Introduce enable_{boot,secondary}_cpu_mm()
  xen/arm32: head Split and move MMU-specific head.S to mmu/head.S

 xen/arch/arm/arm32/head.S   | 580 +---
 xen/arch/arm/arm32/mmu/Makefile |   1 +
 xen/arch/arm/arm32/mmu/head.S   | 573 +++
 xen/arch/arm/include/asm/arm32/macros.h |  58 ++-
 4 files changed, 642 insertions(+), 570 deletions(-)
 create mode 100644 xen/arch/arm/arm32/mmu/head.S

-- 
2.25.1

[XEN v4 1/2] xen/arm32: head: Introduce enable_{boot,secondary}_cpu_mm()

2023-11-17 Thread Ayan Kumar Halder

All the MMU related functionality have been clubbed together in
enable_boot_cpu_mm() for booting primary cpu and enable_secondary_cpu_mm() for
booting secondary cpus.
This is done in preparation for moving the code related to MMU in MMU specific
file and in order to support non MMU cpus in future.

This is based on d2f8df5b3ede ("xen/arm64: head.S: Introduce 
enable_{boot,secondary}_cpu_mm()").

Signed-off-by: Ayan Kumar Halder 
Reviewed-by: Michal Orzel 
Acked-by: Julien Grall 
---

Changes from :-

v1 - 1. Added a proper commit message.
2. Some style and other fixes suggested in v1. 

v2 - 1. Updated the comment on top of enable_boot_cpu_mm() and
enable_secondary_cpu_mm() ie mentioned the input and output registers.
2. Updated the comment inside enable_boot_cpu_mm().

v3 - 1. No changes.

 xen/arch/arm/arm32/head.S | 102 ++
 1 file changed, 80 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 3011fb34aa..b3f6d111b0 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -201,13 +201,11 @@ past_zImage:
 
 blcheck_cpu_mode
 blcpu_init
-blcreate_page_tables
 
-/* Address in the runtime mapping to jump to after the MMU is enabled 
*/
 mov_w lr, primary_switched
-b enable_mmu
+b enable_boot_cpu_mm
+
 primary_switched:
-blsetup_fixmap
 #ifdef CONFIG_EARLY_PRINTK
 /* Use a virtual address to access the UART. */
 mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
@@ -249,27 +247,11 @@ GLOBAL(init_secondary)
 #endif
 blcheck_cpu_mode
 blcpu_init
-blcreate_page_tables
 
-/* Address in the runtime mapping to jump to after the MMU is enabled 
*/
 mov_w lr, secondary_switched
-b enable_mmu
-secondary_switched:
-/*
- * Non-boot CPUs need to move on to the proper pagetables, which were
- * setup in prepare_secondary_mm.
- *
- * XXX: This is not compliant with the Arm Arm.
- */
-mov_w r4, init_ttbr  /* VA of HTTBR value stashed by CPU 0 */
-ldrd  r4, r5, [r4]   /* Actual value */
-dsb
-mcrr  CP64(r4, r5, HTTBR)
-dsb
-isb
-flush_xen_tlb_local r0
-pt_enforce_wxn r0
+b enable_secondary_cpu_mm
 
+secondary_switched:
 #ifdef CONFIG_EARLY_PRINTK
 /* Use a virtual address to access the UART. */
 mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
@@ -692,6 +674,82 @@ ready_to_switch:
 mov   pc, lr
 ENDPROC(switch_to_runtime_mapping)
 
+/*
+ * Enable mm (turn on the data cache and the MMU) for secondary CPUs.
+ * The function will return to the virtual address provided in LR (e.g. the
+ * runtime mapping).
+ *
+ * Inputs:
+ *   r9 : paddr(start)
+ *   r10: phys offset
+ *   lr : Virtual address to return to.
+ *
+ * Output:
+ *   r12: Was a temporary mapping created?
+ *
+ * Clobbers r0 - r6
+ */
+enable_secondary_cpu_mm:
+mov   r6, lr
+
+blcreate_page_tables
+
+/* Address in the runtime mapping to jump to after the MMU is enabled 
*/
+mov_w lr, 1f
+b enable_mmu
+1:
+/*
+ * Non-boot CPUs need to move on to the proper pagetables, which were
+ * setup in prepare_secondary_mm.
+ *
+ * XXX: This is not compliant with the Arm Arm.
+ */
+mov_w r4, init_ttbr  /* VA of HTTBR value stashed by CPU 0 */
+ldrd  r4, r5, [r4]   /* Actual value */
+dsb
+mcrr  CP64(r4, r5, HTTBR)
+dsb
+isb
+flush_xen_tlb_local r0
+pt_enforce_wxn r0
+
+/* Return to the virtual address requested by the caller. */
+mov   pc, r6
+ENDPROC(enable_secondary_cpu_mm)
+
+/*
+ * Enable mm (turn on the data cache and the MMU) for the boot CPU.
+ * The function will return to the virtual address provided in LR (e.g. the
+ * runtime mapping).
+ *
+ * Inputs:
+ *   r9 : paddr(start)
+ *   r10: phys offset
+ *   lr : Virtual address to return to.
+ *
+ * Output:
+ *   r12: Was a temporary mapping created?
+ *
+ * Clobbers r0 - r6
+ */
+enable_boot_cpu_mm:
+mov   r6, lr
+
+blcreate_page_tables
+
+/* Address in the runtime mapping to jump to after the MMU is enabled 
*/
+mov_w lr, 1f
+b enable_mmu
+1:
+/*
+ * Prepare the fixmap. The function will return to the virtual address
+ * requested by the caller.
+ */
+mov   lr, r6
+
+b setup_fixmap
+ENDPROC(enable_boot_cpu_mm)
+
 /*
  * Remove the 1:1 map from the page-tables. It is not easy to keep track
  * where the 1:1 map was mapped, so we will look for the top-level entry
-- 
2.25.1

Re: [PATCH v10 03/17] vpci: use per-domain PCI lock to protect vpci structure

On Thu, Oct 12, 2023 at 10:09:15PM +, Volodymyr Babchuk wrote:
> From: Oleksandr Andrushchenko 
> 
> Use a previously introduced per-domain read/write lock to check
> whether vpci is present, so we are sure there are no accesses to the
> contents of the vpci struct if not. This lock can be used (and in a
> few cases is used right away) so that vpci removal can be performed
> while holding the lock in write mode. Previously such removal could
> race with vpci_read for example.
> 
> When taking both d->pci_lock and pdev->vpci->lock, they should be
> taken in this exact order: d->pci_lock then pdev->vpci->lock to avoid
> possible deadlock situations.
> 
> 1. Per-domain's pci_rwlock is used to protect pdev->vpci structure
> from being removed.
> 
> 2. Writing the command register and ROM BAR register may trigger
> modify_bars to run, which in turn may access multiple pdevs while
> checking for the existing BAR's overlap. The overlapping check, if
> done under the read lock, requires vpci->lock to be acquired on both
> devices being compared, which may produce a deadlock. It is not
> possible to upgrade read lock to write lock in such a case. So, in
> order to prevent the deadlock, use d->pci_lock instead. To prevent
> deadlock while locking both hwdom->pci_lock and dom_xen->pci_lock,
> always lock hwdom first.
> 
> All other code, which doesn't lead to pdev->vpci destruction and does
> not access multiple pdevs at the same time, can still use a
> combination of the read lock and pdev->vpci->lock.
> 
> 3. Drop const qualifier where the new rwlock is used and this is
> appropriate.
> 
> 4. Do not call process_pending_softirqs with any locks held. For that
> unlock prior the call and re-acquire the locks after. After
> re-acquiring the lock there is no need to check if pdev->vpci exists:
>  - in apply_map because of the context it is called (no race condition
>possible)
>  - for MSI/MSI-X debug code because it is called at the end of
>pdev->vpci access and no further access to pdev->vpci is made
> 
> 5. Use d->pci_lock around for_each_pdev and pci_get_pdev_by_domain
> while accessing pdevs in vpci code.
> 
> 6. We are removing multiple ASSERT(pcidevs_locked()) instances because
> they are too strict now: they should be corrected to
> ASSERT(pcidevs_locked() || rw_is_locked(>pci_lock)), but problem is
> that mentioned instances does not have access to the domain
> pointer and it is not feasible to pass a domain pointer to a function
> just for debugging purposes.
> 
> There is a possible lock inversion in MSI code, as some parts of it
> acquire pcidevs_lock() while already holding d->pci_lock.

Is this going to be addressed in a further patch?

> 
> Suggested-by: Roger Pau Monné 
> Suggested-by: Jan Beulich 
> Signed-off-by: Oleksandr Andrushchenko 
> Signed-off-by: Volodymyr Babchuk 
> 
> ---
> Changes in v10:
>  - Moved printk pas locked area
>  - Returned back ASSERTs
>  - Added new parameter to allocate_and_map_msi_pirq() so it knows if
>  it should take the global pci lock
>  - Added comment about possible improvement in vpci_write
>  - Changed ASSERT(rw_is_locked()) to rw_is_write_locked() in
>appropriate places
>  - Renamed release_domain_locks() to release_domain_write_locks()
>  - moved domain_done label in vpci_dump_msi() to correct place
> Changes in v9:
>  - extended locked region to protect vpci_remove_device and
>vpci_add_handlers() calls
>  - vpci_write() takes lock in the write mode to protect
>potential call to modify_bars()
>  - renamed lock releasing function
>  - removed ASSERT()s from msi code
>  - added trylock in vpci_dump_msi
> 
> Changes in v8:
>  - changed d->vpci_lock to d->pci_lock
>  - introducing d->pci_lock in a separate patch
>  - extended locked region in vpci_process_pending
>  - removed pcidevs_lockis vpci_dump_msi()
>  - removed some changes as they are not needed with
>the new locking scheme
>  - added handling for hwdom && dom_xen case
> ---
>  xen/arch/x86/hvm/vmsi.c| 26 -
>  xen/arch/x86/hvm/vmx/vmx.c |  2 --
>  xen/arch/x86/include/asm/irq.h |  3 +-
>  xen/arch/x86/irq.c | 12 
>  xen/arch/x86/msi.c | 10 ++-
>  xen/arch/x86/physdev.c |  2 +-
>  xen/drivers/passthrough/pci.c  |  9 +++---
>  xen/drivers/vpci/header.c  | 18 
>  xen/drivers/vpci/msi.c | 28 --
>  xen/drivers/vpci/msix.c| 52 +-
>  xen/drivers/vpci/vpci.c| 51 +++--
>  11 files changed, 166 insertions(+), 47 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> index 128f236362..6b33a80120 100644
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -468,7 +468,7 @@ int msixtbl_pt_register(struct domain *d, struct pirq 
> *pirq, uint64_t gtable)
>  struct msixtbl_entry *entry, *new_entry;
>  int r = -EINVAL;
>  
> -ASSERT(pcidevs_locked());
> +

Re: [PATCH 3/3] x86/iommu: use a rangeset for hwdom setup

2023-11-17 Thread Andrew Cooper

On 17/11/2023 9:47 am, Roger Pau Monne wrote:
> The current loop that iterates from 0 to the maximum RAM address in order to
> setup the IOMMU mappings is highly inefficient, and it will get worse as the
> amount of RAM increases.  It's also not accounting for any reserved regions
> past the last RAM address.
>
> Instead of iterating over memory addresses, iterate over the memory map 
> regions
> and use a rangeset in order to keep track of which ranges need to be identity
> mapped in the hardware domain physical address space.
>
> On an AMD EPYC 7452 with 512GiB of RAM, the time to execute
> arch_iommu_hwdom_init() in nanoseconds is:
>
> x old
> + new
> N   Min   MaxMedian   AvgStddev
> x   5 2.2364154e+10  2.338244e+10 2.2474685e+10 2.2622409e+10 4.2949869e+08
> +   5   1025012   1033036   1026188 1028276.2 3623.1194
> Difference at 95.0% confidence
>   -2.26214e+10 +/- 4.42931e+08
>   -99.9955% +/- 9.05152e-05%
>   (Student's t, pooled s = 3.03701e+08)
>
> Execution time of arch_iommu_hwdom_init() goes down from ~22s to ~0.001s.
>
> Note there's a change for HVM domains (ie: PVH dom0) that get switched to
> create the p2m mappings using map_mmio_regions() instead of
> p2m_add_identity_entry(), so that ranges can be mapped with a single function
> call if possible.  Note that the interface of map_mmio_regions() doesn't
> allow creating read-only mappings, but so far there are no such mappings
> created for PVH dom0 in arch_iommu_hwdom_init().
>
> No change intended in the resulting mappings that a hardware domain gets.
>
> Signed-off-by: Roger Pau Monné 

Very nice numbers.  And yes - straight line performance like this (good
or bad) is all about the innermost loop.

Sadly, the patch diff is horrible to read.  Patch 2 remaining in common
code will improve this a little, but probably not very much.

If there are no better ideas, it's probably best to split into 3
patches, being:

1) Introduce new rangeset forms of existing operations
2) Rewrite arch_iommu_hwdom_init() to use rangesets
3) Delete old mfn forms

That at least means that the new and the old forms aren't expressed as a
delta against each-other.

~Andrew

Re: [PATCH v10 02/17] pci: introduce per-domain PCI rwlock

On Thu, Oct 12, 2023 at 10:09:15PM +, Volodymyr Babchuk wrote:
> Add per-domain d->pci_lock that protects access to
> d->pdev_list. Purpose of this lock is to give guarantees to VPCI code
> that underlying pdev will not disappear under feet. This is a rw-lock,
> but this patch adds only write_lock()s. There will be read_lock()
> users in the next patches.
> 
> This lock should be taken in write mode every time d->pdev_list is
> altered. All write accesses also should be protected by pcidevs_lock()
> as well. Idea is that any user that wants read access to the list or
> to the devices stored in the list should use either this new
> d->pci_lock or old pcidevs_lock(). Usage of any of this two locks will
> ensure only that pdev of interest will not disappear from under feet
> and that the pdev still will be assigned to the same domain. Of
> course, any new users should use pcidevs_lock() when it is
> appropriate (e.g. when accessing any other state that is protected by
> the said lock). In case both the newly introduced per-domain rwlock
> and the pcidevs lock is taken, the later must be acquired first.
 ^ latter

> 
> Suggested-by: Roger Pau Monné 
> Suggested-by: Jan Beulich 
> Signed-off-by: Volodymyr Babchuk 

Reviewed-by: Roger Pau Monné 

I'm a bit concerned with the logic used in pci_release_devices(), but
I guess it's fine for now as long as the global pcidevs_lock is still
held.

> ---
> 
> Changes in v10:
>  - pdev->domain is assigned after removing from source domain but
>before adding to target domain in reassign_device() functions.
> 
> Changes in v9:
>  - returned back "pdev->domain = target;" in AMD IOMMU code
>  - used "source" instead of pdev->domain in IOMMU functions
>  - added comment about lock ordering in the commit message
>  - reduced locked regions
>  - minor changes non-functional changes in various places
> 
> Changes in v8:
>  - New patch
> 
> Changes in v8 vs RFC:
>  - Removed all read_locks after discussion with Roger in #xendevel
>  - pci_release_devices() now returns the first error code
>  - extended commit message
>  - added missing lock in pci_remove_device()
>  - extended locked region in pci_add_device() to protect list_del() calls
> ---
>  xen/common/domain.c |  1 +
>  xen/drivers/passthrough/amd/pci_amd_iommu.c |  9 ++-
>  xen/drivers/passthrough/pci.c   | 71 +
>  xen/drivers/passthrough/vtd/iommu.c |  9 ++-
>  xen/include/xen/sched.h |  1 +
>  5 files changed, 78 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 8f9ab01c0c..785c69e48b 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -651,6 +651,7 @@ struct domain *domain_create(domid_t domid,
>  
>  #ifdef CONFIG_HAS_PCI
>  INIT_LIST_HEAD(>pdev_list);
> +rwlock_init(>pci_lock);
>  #endif
>  
>  /* All error paths can depend on the above setup. */
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c 
> b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index 836c24b02e..36a617bed4 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -476,8 +476,15 @@ static int cf_check reassign_device(
>  
>  if ( devfn == pdev->devfn && pdev->domain != target )
>  {
> -list_move(>domain_list, >pdev_list);
> +write_lock(>pci_lock);
> +list_del(>domain_list);
> +write_unlock(>pci_lock);
> +
>  pdev->domain = target;
> +
> +write_lock(>pci_lock);
> +list_add(>domain_list, >pdev_list);
> +write_unlock(>pci_lock);
>  }
>  
>  /*
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 04d00c7c37..b8ad4fa07c 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -453,7 +453,9 @@ static void __init _pci_hide_device(struct pci_dev *pdev)
>  if ( pdev->domain )
>  return;
>  pdev->domain = dom_xen;
> +write_lock(_xen->pci_lock);
>  list_add(>domain_list, _xen->pdev_list);
> +write_unlock(_xen->pci_lock);
>  }
>  
>  int __init pci_hide_device(unsigned int seg, unsigned int bus,
> @@ -746,7 +748,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>  if ( !pdev->domain )
>  {
>  pdev->domain = hardware_domain;
> +write_lock(_domain->pci_lock);
>  list_add(>domain_list, _domain->pdev_list);
> +write_unlock(_domain->pci_lock);
>  
>  /*
>   * For devices not discovered by Xen during boot, add vPCI handlers
> @@ -756,7 +760,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>  if ( ret )
>  {
>  printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
> +write_lock(_domain->pci_lock);
>  list_del(>domain_list);
> +write_unlock(_domain->pci_lock);
>  pdev->domain = NULL;
>  goto

Re: [PATCH v10 13/17] vpci: add initial support for virtual PCI bus topology

2023-11-17 Thread Volodymyr Babchuk



Hi Stefano,

Stefano Stabellini  writes:

> On Fri, 17 Nov 2023, Volodymyr Babchuk wrote:
>> > I still think, no matter the BDF allocation scheme, that we should try
>> > to avoid as much as possible to have two different PCI Root Complex
>> > emulators. Ideally we would have only one PCI Root Complex emulated by
>> > Xen. Having 2 PCI Root Complexes both of them emulated by Xen would be
>> > tolerable but not ideal.
>> 
>> But what is exactly wrong with this setup?
>
> [...]
>
>> > The worst case I would like to avoid is to have
>> > two PCI Root Complexes, one emulated by Xen and one emulated by QEMU.
>> 
>> This is how our setup works right now.
>
> If we have:
> - a single PCI Root Complex emulated in Xen
> - Xen is safety certified
> - individual Virtio devices emulated by QEMU with grants for memory
>
> We can go very far in terms of being able to use Virtio in safety
> use-cases. We might even be able to use Virtio (frontends) in a SafeOS.
>
> On the other hand if we put an additional Root Complex in QEMU:
> - we pay a price in terms of complexity of the codebase
> - we pay a price in terms of resource utilization
> - we have one additional problem in terms of using this setup with a
>   SafeOS (one more device emulated by a non-safe component)
>
> Having 2 PCI Root Complexes both emulated in Xen is a middle ground
> solution because:
> - we still pay a price in terms of resource utilization
> - the code complexity goes up a bit but hopefully not by much
> - there is no impact on safety compared to the ideal scenario
>
> This is why I wrote that it is tolerable.

Ah, I see now. Yes, I am agree with this. Also I want to add some more
points:

- There is ongoing work on implementing virtio backends as a separate
  applications, written in Rust. Linaro are doing this part. Right now
  they are implementing only virtio-mmio, but if they want to provide
  virtio-pci as well, they will need a mechanism to plug only
  virtio-pci, without Root Complex. This is argument for using single Root
  Complex emulated in Xen.

- As far as I know (actually, Oleksandr told this to me), QEMU has no
  mechanism for exposing virtio-pci backends without exposing PCI root
  complex as well. Architecturally, there should be a PCI bus to which
  virtio-pci devices are connected. Or we need to make some changes to
  QEMU internals to be able to create virtio-pci backends that are not
  connected to any bus. Also, added benefit that PCI Root Complex
  emulator in QEMU handles legacy PCI interrupts for us. This is
  argument for separate Root Complex for QEMU.

As right now we have only virtio-pci backends provided by QEMU and this
setup is already working, I propose to stick to this
solution. Especially, taking into account that it does not require any
changes to hypervisor code.

-- 
WBR, Volodymyr

Re: [PATCH v10 01/17] pci: msi: pass pdev to pci_enable_msi() function

On Thu, Oct 12, 2023 at 10:09:14PM +, Volodymyr Babchuk wrote:
> Previously pci_enable_msi() function obtained pdev pointer by itself,
> but taking into account upcoming changes to PCI locking, it is better
> when caller passes already acquired pdev pointer to the function.

A bit more detail into why this matters for the upcoming locking
change would be useful here.

> Note that ns16550 driver does not check validity of obtained pdev
> pointer because pci_enable_msi() already does this.
> 
> Signed-off-by: Volodymyr Babchuk 
> 
> ---
> Changes in v10:
> 
>  - New in v10. This is the result of discussion in "vpci: add initial
>  support for virtual PCI bus topology"
> ---
>  xen/arch/x86/include/asm/msi.h |  3 ++-
>  xen/arch/x86/irq.c |  2 +-
>  xen/arch/x86/msi.c | 19 ++-
>  xen/drivers/char/ns16550.c |  4 +++-
>  4 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
> index a53ade95c9..836c8cd4ba 100644
> --- a/xen/arch/x86/include/asm/msi.h
> +++ b/xen/arch/x86/include/asm/msi.h
> @@ -81,7 +81,8 @@ struct irq_desc;
>  struct hw_interrupt_type;
>  struct msi_desc;
>  /* Helper functions */
> -extern int pci_enable_msi(struct msi_info *msi, struct msi_desc **desc);
> +extern int pci_enable_msi(struct msi_info *msi, struct msi_desc **desc,
> +   struct pci_dev *pdev);

Hard tabs (here and below).

I agree with Jan that it might be better for pdev to be the first
parameter.

Otherwise seems fine if the pdev is already in the caller context, as
we avoid an extra list walk.

Thanks, Roger.

Re: Important - Documentation Discussion

2023-11-17 Thread Kelly Choi

Hey Alejandro,

Thanks for your feedback.
I'll consider all your points, and any other comments from the community
before proceeding on the next steps.

If anyone else has any further ideas, please let me know *Friday 24th
November 2023.*

Many thanks,
Kelly Choi

Open Source Community Manager
XenServer, Cloud Software Group


On Wed, Nov 15, 2023 at 12:16 PM Alejandro Vallejo <
alejandro.vall...@cloud.com> wrote:

> Hi,
>
> On Wed, Nov 15, 2023 at 11:43:46AM +, Kelly Choi wrote:
> > Hi all,
> >
> > As you may be aware, we are in the process of reviewing our existing
> > documentation platform and content. In order to meet the requirements of
> > the community and project, I need your input and feedback.
> >
> > The aim of the new documentation is to encourage community members to
> > collaborate in updating content (where possible) and educate users on how
> > the project works. The updated documentation will be hosted on our new
> > upcoming website.
> >
> > *Suggestion for user-orientated documentation:*
> >
> >- git - for hosting (gitlab recommended)
> >- RST - for the format of the documentation
> >- Sphynx - to generate web pages and PDF and other formats from RST
> In my experience Sphinx's strength is in its ability to cross-reference the
> code. That isn't something terribly helpful for user documentation, and it
> makes the whole thing harder to set up for no clear benefit.
>
> For user-facing docs I'd propose `mkdocs` instead, which is a lot more
> focused and simpler to set up (can be done literally in a couple of
> minutes). The main difference would be that it takes Markdown rather than
> RST[1]. It trivially supports plugins for interesting things, like mermaid
> (for sequence/block diagrams or FSMs)
>
> Main website: https://www.mkdocs.org/
> Plugin catalog: https://github.com/mkdocs/catalog
> * mermaid plugin: https://github.com/fralau/mkdocs-mermaid2-plugin
> * kroki plugin: https://kroki.io/
>
> [1] I happen to prefer Markdown, as I find it easier to read and write, but
> that's just personal preference
>
> >
> > *Suggestion for developer reference documentation:*
> >
> >- Doxygen
> >- Kerneldoc
> >- Open to other suggestions here
> There's 2 areas here. The format for the annotations, and the visualization
> frontend. They need not be the same. Using Doxygen seems the less
> "not-invented-here" approach, while kerneldoc would
>
> As for the frontend I would suggest to _not_ use Doxygen itself as the
> generated websites are hideous to look at. Sphinx (through Breathe) or any
> other Doxygen-database parse wouldr do the job as well providing a much
> (much) better output.
>
> >
> > Example of how documentation will be split:
> >
> >1. Getting started/beginner user guides
> >2. Learning orientated tutorials
> >3. Goal-orientated how-to guides
> >4. Developer related documentation
> (1-3) seem like pretty much the same thing. Guides of increasing complexity
> meant to train a new user/admin. Dividing such a section in those 3 blocks
> seems sensible.
>
> (4) could be split in a "Developer Manual", which would contain plain
> explanation for dev-heavy concepts, and a "Reference Manual" with links to
> the Doxygen-esque websites and a higher focus on implementation details.
>
> >
> > Side note: Whilst I appreciate everyone has a different vision of what
> > ideal documentation looks like, please be mindful that not everyone's
> > thought processes or depth of knowledge are the same. All ideas are
> > welcome, and decisions made will always reflect community needs.
> >
> > Many thanks,
> > Kelly Choi
> >
> > Open Source Community Manager
> > XenServer, Cloud Software Group
>
> Cheers,
> Alejandro
>

Re: [PATCH v7 1/2] xen/vpci: header: status register handler

On Fri, Nov 17, 2023 at 02:23:42PM +0100, Jan Beulich wrote:
> On 17.11.2023 13:40, Roger Pau Monné wrote:
> > On Wed, Sep 13, 2023 at 10:35:46AM -0400, Stewart Hildebrand wrote:
> >> --- a/xen/drivers/vpci/vpci.c
> >> +++ b/xen/drivers/vpci/vpci.c
> >> @@ -29,6 +29,9 @@ struct vpci_register {
> >>  unsigned int offset;
> >>  void *private;
> >>  struct list_head node;
> >> +uint32_t rsvdz_mask;
> >> +uint32_t ro_mask;
> >> +uint32_t rw1c_mask;
> > 
> > I understand that we need the rw1c_mask in order to properly merge
> > values when doing partial writes, but the other fields I'm not sure we
> > do need them.  RO bits don't care about what's written to them, and
> > RsvdZ are always read as 0 and written as 0, so the mask shouldn't
> > affect the merging.
> 
> What some version of the spec says is r/o or reserved may be different
> in another. Also iirc devices may (wrongly?) implement r/o bits as r/w.
> When presenting a virtual view of devices to guests, in this regard I
> think we want (or even need) to enforce our view of the world.

That needs to be part of the commit message then.

Ideally we would also want to do a swept of all registers we currently
implement, in order to check for ro or rsvdz bits and properly enforce
them.

Thanks, Roger.

Re: [PATCH v7 1/2] xen/vpci: header: status register handler

On Wed, Sep 13, 2023 at 10:35:46AM -0400, Stewart Hildebrand wrote:
> +int vpci_add_register_mask(struct vpci *vpci, vpci_read_t *read_handler,
> +   vpci_write_t *write_handler, unsigned int offset,
> +   unsigned int size, void *data, uint32_t 
> rsvdz_mask,
> +   uint32_t ro_mask, uint32_t rw1c_mask)

Forgot to mention, it would be good if you can add some tests in
tools/tests/vpci that ensure the masks work as expected.

Thanks, Roger.

Re: [PATCH v7 1/2] xen/vpci: header: status register handler

2023-11-17 Thread Jan Beulich

On 17.11.2023 13:40, Roger Pau Monné wrote:
> On Wed, Sep 13, 2023 at 10:35:46AM -0400, Stewart Hildebrand wrote:
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -29,6 +29,9 @@ struct vpci_register {
>>  unsigned int offset;
>>  void *private;
>>  struct list_head node;
>> +uint32_t rsvdz_mask;
>> +uint32_t ro_mask;
>> +uint32_t rw1c_mask;
> 
> I understand that we need the rw1c_mask in order to properly merge
> values when doing partial writes, but the other fields I'm not sure we
> do need them.  RO bits don't care about what's written to them, and
> RsvdZ are always read as 0 and written as 0, so the mask shouldn't
> affect the merging.

What some version of the spec says is r/o or reserved may be different
in another. Also iirc devices may (wrongly?) implement r/o bits as r/w.
When presenting a virtual view of devices to guests, in this regard I
think we want (or even need) to enforce our view of the world.

Jan

Re: [RFC PATCH 2/6] xen/public: arch-arm: reserve resources for virtio-pci

2023-11-17 Thread Sergiy Kibrik


hi Julien, Oleksandr,

[..]



This patch series only covers use-cases where the device emulator
handles the *entire* PCI Host bridge and PCI (virtio-pci) devices behind
it (i.e. Qemu). Also this patch series doesn't touch vPCI/PCI
pass-through resources, handling, accounting, nothing. 


I understood you want to one Device Emulator to handle the entire PCI 
host bridge. But...


 From the

hypervisor we only need a help to intercept the config space accesses
happen in a range [GUEST_VIRTIO_PCI_ECAM_BASE ...
GUEST_VIRTIO_PCI_ECAM_BASE + GUEST_VIRTIO_PCI_TOTAL_ECAM_SIZE] and
forward them to the linked device emulator (if any), that's all.


... I really don't see why you need to add code in Xen to trap the 
region. If QEMU is dealing with the hostbridge, then it should be able 
to register the MMIO region and then do the translation.





[..]


I am afraid, we cannot end up exposing only single PCI Host bridge with
current model (if we use device emulators running in different domains
that handles the *entire* PCI Host bridges), this won't work.


That makes sense and it is fine. But see above, I think only the #2 is 
necessary for the hypervisor. Patch #5 should not be necessary at all.


[...]


I did checks w/o patch #5 and can confirm that indeed -- qemu & xen can 
do this work without additional modifications to qemu code. So I'll drop

this patch from this series.

[..]

+/*
+ * 16 MB is reserved for virtio-pci configuration space based on
calculation
+ * 8 bridges * 2 buses x 32 devices x 8 functions x 4 KB = 16 MB


Can you explain how youd ecided the "2"?


good question, we have a limited free space available in memory layout
(we had difficulties to find a suitable holes) also we don't expect a
lot of virtio-pci devices, so "256" used vPCI would be too much. It was
decided to reduce significantly, but select maximum to fit into free
space, with having "2" buses we still fit into the chosen holes.


If you don't expect a lot of virtio devices, then why do you need two 
buses? Wouldn't one be sufficient?




one should be reasonably sufficient, I agree







+ */
+#define GUEST_VIRTIO_PCI_ECAM_BASE  xen_mk_ullong(0x3300)
+#define GUEST_VIRTIO_PCI_TOTAL_ECAM_SIZE    xen_mk_ullong(0x0100)
+#define GUEST_VIRTIO_PCI_HOST_ECAM_SIZE xen_mk_ullong(0x0020)
+
+/* 64 MB is reserved for virtio-pci memory */
+#define GUEST_VIRTIO_PCI_ADDR_TYPE_MEM    xen_mk_ullong(0x0200)
+#define GUEST_VIRTIO_PCI_MEM_ADDR xen_mk_ullong(0x3400)
+#define GUEST_VIRTIO_PCI_MEM_SIZE xen_mk_ullong(0x0400)
+
   /*
    * 16MB == 4096 pages reserved for guest to use as a region to 
map its

    * grant table in.
@@ -476,6 +489,11 @@ typedef uint64_t xen_callback_t;
   #define GUEST_MAGIC_BASE  xen_mk_ullong(0x3900)
   #define GUEST_MAGIC_SIZE  xen_mk_ullong(0x0100)
+/* 64 MB is reserved for virtio-pci Prefetch memory */


This doesn't seem a lot depending on your use case. Can you details how
you can up with "64 MB"?


the same calculation as it was done configuration space. It was observed
that only 16K is used per virtio-pci device (maybe it can be bigger for
usual PCI device, I don't know). Please look at the example of DomU log
below (to strings that contain "*BAR 4: assigned*"):


What about virtio-gpu? I would expect a bit more memory is necessary for 
that use case.


Any case, what I am looking for is for some explanation in the commit 
message of the limits. I don't particularly care about the exact limit 
because this is not part of a stable ABI.


sure, I'll put a bit more explanation in both comment and commit 
message. Should I post updated patch series, with updated resources and 
without patch #5, or shall we wait for some more comments here?


--
regards,
 Sergiy

Re: [PATCH 2/3] x86/iommu: move xen_in_range() so it can be made static

On Fri, Nov 17, 2023 at 12:03:19PM +, Andrew Cooper wrote:
> On 17/11/2023 9:47 am, Roger Pau Monne wrote:
> > No functional change intended.
> >
> > Signed-off-by: Roger Pau Monné 
> 
> There may only be one caller (after dropping some bogus tboot logic
> recently IIRC), but this isn't an IOMMU-specific helper.
> 
> See the comment in the middle which shows the other opencoded things
> this needs to be kept up to date.  (And I'd hoped to make this common
> because every architecture seems to have different bugs opencoding this
> calculation.)

But those symbols ultimately come from the linker script, and hence
didn't see them that tied to the setup code.  The setup code uses them,
just as the IOMMU will do now, and likely other parts of the code like
livepatch.

> Switching to rangesets is fine, but the result still wants to be
> something generic, rather than IOMMU-specific.

I'm fine with leaving the function in setup.c, this was mostly a
cleanup attempt.

Thanks, Roger.

[xen-4.18-testing test] 183777: tolerable FAIL - PUSHED

flight 183777 xen-4.18-testing real [real]
flight 183782 xen-4.18-testing real-retest [real]
http://logs.test-lab.xenproject.org/osstest/logs/183777/
http://logs.test-lab.xenproject.org/osstest/logs/183782/

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-amd64-xl-credit2 22 guest-start/debian.repeat fail pass in 
183782-retest

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

version targeted for testing:
 xen  d75f1e9b74314cea91ce435730d4e3539ecca77d
baseline version:
 xen  3ddbddf143fbd164c55e46c5a276f6606e5cc843

Last test of basis   183772  2023-11-16

Re: [PATCH v7 1/2] xen/vpci: header: status register handler