Re: [PATCH -next] xen: Fix one kernel-doc comment

[Juergen Gross]

On 31.07.23 05:00, Yang Li wrote:

Use colon to separate parameter name from their specific meaning.
silence the warning:

drivers/xen/grant-table.c:1051: warning: Function parameter or member 
'nr_pages' not described in 'gnttab_free_pages'

Reported-by: Abaci Robot 
Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=6030
Signed-off-by: Yang Li 


Pushed to xen/tip.git for-linus-6.6


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH -next] xen: xenbus: Use helper function IS_ERR_OR_NULL()

[Juergen Gross]

On 17.08.23 03:47, Li Zetao wrote:

Use IS_ERR_OR_NULL() to detect an error pointer or a null pointer
open-coding to simplify the code.

Signed-off-by: Li Zetao 


Pushed to xen/tip.git for-linus-6.6


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH -next] xen: Switch to use kmemdup() helper

[Juergen Gross]

On 15.08.23 11:24, Ruan Jinjie wrote:

Use kmemdup() helper instead of open-coding to
simplify the code.

Signed-off-by: Ruan Jinjie 


Pushed to xen/tip.git for-linus-6.6


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH -next] xen-pciback: Remove unused function declarations

[Juergen Gross]

On 08.08.23 17:09, Yue Haibing wrote:

Commit a92336a1176b ("xen/pciback: Drop two backends, squash and cleanup some 
code.")
declared but never implemented these functions.

Signed-off-by: Yue Haibing 


Pushed to xen/tip.git for-linus-6.6


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH] x86/xen: Make virt_to_pfn() a static inline

[Juergen Gross]

On 10.08.23 09:27, Linus Walleij wrote:

Making virt_to_pfn() a static inline taking a strongly typed
(const void *) makes the contract of a passing a pointer of that
type to the function explicit and exposes any misuse of the
macro virt_to_pfn() acting polymorphic and accepting many types
such as (void *), (unitptr_t) or (unsigned long) as arguments
without warnings.

Also fix all offending call sites to pass a (void *) rather
than an unsigned long. Since virt_to_mfn() is wrapping
virt_to_pfn() this function has become polymorphic as well
so the usage need to be fixed up.

Signed-off-by: Linus Walleij 


Pushed to xen/tip.git for-linus-6.6


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH v1] xen: remove a confusing comment on auto-translated guest I/O

[Juergen Gross]

On 02.08.23 18:31, Petr Tesarik wrote:

From: Petr Tesarik 

After removing the conditional return from xen_create_contiguous_region(),
the accompanying comment was left in place, but it now precedes an
unrelated conditional and confuses readers.

Fixes: 989513a735f5 ("xen: cleanup pvh leftovers from pv-only sources")
Signed-off-by: Petr Tesarik 


Pushed to xen/tip.git for-linus-6.6


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH -next] xen/evtchn: Remove unused function declaration xen_set_affinity_evtchn()

[Juergen Gross]

On 01.08.23 16:54, Yue Haibing wrote:

Commit 67473b8194bc ("xen/events: Remove disfunct affinity spreading")
leave this unused declaration.

Signed-off-by: Yue Haibing 


Pushed to xen/tip.git for-linus-6.6


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH] xen/xenbus: Avoid a lockdep warning when adding a watch

[Juergen Gross]

On 07.06.23 14:36, Petr Pavlu wrote:

The following lockdep warning appears during boot on a Xen dom0 system:

[   96.388794] ==
[   96.388797] WARNING: possible circular locking dependency detected
[   96.388799] 6.4.0-rc5-default+ #8 Tainted: GEL
[   96.388803] --
[   96.388804] xenconsoled/1330 is trying to acquire lock:
[   96.388808] 82acdd10 (xs_watch_rwsem){}-{3:3}, at: 
register_xenbus_watch+0x45/0x140
[   96.388847]
but task is already holding lock:
[   96.388849] 888100c92068 (>msgbuffer_mutex){+.+.}-{3:3}, at: 
xenbus_file_write+0x2c/0x600
[   96.388862]
which lock already depends on the new lock.

[   96.388864]
the existing dependency chain (in reverse order) is:
[   96.388866]
-> #2 (>msgbuffer_mutex){+.+.}-{3:3}:
[   96.388874]__mutex_lock+0x85/0xb30
[   96.35]xenbus_dev_queue_reply+0x48/0x2b0
[   96.388890]xenbus_thread+0x1d7/0x950
[   96.388897]kthread+0xe7/0x120
[   96.388905]ret_from_fork+0x2c/0x50
[   96.388914]
-> #1 (xs_response_mutex){+.+.}-{3:3}:
[   96.388923]__mutex_lock+0x85/0xb30
[   96.388930]xenbus_backend_ioctl+0x56/0x1c0
[   96.388935]__x64_sys_ioctl+0x90/0xd0
[   96.388942]do_syscall_64+0x5c/0x90
[   96.388950]entry_SYSCALL_64_after_hwframe+0x72/0xdc
[   96.388957]
-> #0 (xs_watch_rwsem){}-{3:3}:
[   96.388965]__lock_acquire+0x1538/0x2260
[   96.388972]lock_acquire+0xc6/0x2b0
[   96.388976]down_read+0x2d/0x160
[   96.388983]register_xenbus_watch+0x45/0x140
[   96.388990]xenbus_file_write+0x53d/0x600
[   96.388994]vfs_write+0xe4/0x490
[   96.389003]ksys_write+0xb8/0xf0
[   96.389011]do_syscall_64+0x5c/0x90
[   96.389017]entry_SYSCALL_64_after_hwframe+0x72/0xdc
[   96.389023]
other info that might help us debug this:

[   96.389025] Chain exists of:
  xs_watch_rwsem --> xs_response_mutex --> >msgbuffer_mutex

[   96.413429]  Possible unsafe locking scenario:

[   96.413430]CPU0CPU1
[   96.413430]
[   96.413431]   lock(>msgbuffer_mutex);
[   96.413432]lock(xs_response_mutex);
[   96.413433]lock(>msgbuffer_mutex);
[   96.413434]   rlock(xs_watch_rwsem);
[   96.413436]
 *** DEADLOCK ***

[   96.413436] 1 lock held by xenconsoled/1330:
[   96.413438]  #0: 888100c92068 (>msgbuffer_mutex){+.+.}-{3:3}, at: 
xenbus_file_write+0x2c/0x600
[   96.413446]

An ioctl call IOCTL_XENBUS_BACKEND_SETUP (record #1 in the report)
results in calling xenbus_alloc() -> xs_suspend() which introduces
ordering xs_watch_rwsem --> xs_response_mutex. The xenbus_thread()
operation (record #2) creates xs_response_mutex --> >msgbuffer_mutex.
An XS_WATCH write to the xenbus file then results in a complain about
the opposite lock order >msgbuffer_mutex --> xs_watch_rwsem.

The dependency xs_watch_rwsem --> xs_response_mutex is spurious. Avoid
it and the warning by changing the ordering in xs_suspend(), first
acquire xs_response_mutex and then xs_watch_rwsem. Reverse also the
unlocking order in xs_suspend_cancel() for consistency, but keep
xs_resume() as is because it needs to have xs_watch_rwsem unlocked only
after exiting xs suspend and re-adding all watches.

Signed-off-by: Petr Pavlu 


Reviewed-by: Juergen Gross 

Sorry it took so long,


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH v5 08/13] xen/arm: Fold pmap and fixmap into MMU system

[Henry Wang]

Hi Julien,

> On Aug 22, 2023, at 02:14, Julien Grall  wrote:
> 
> Hi Henry,
> 
> On 14/08/2023 05:25, Henry Wang wrote:
>> From: Penny Zheng 
>> 
>>  diff --git a/xen/arch/arm/include/asm/fixmap.h 
>> b/xen/arch/arm/include/asm/fixmap.h
>> index 734eb9b1d4..5d5de6995a 100644
>> --- a/xen/arch/arm/include/asm/fixmap.h
>> +++ b/xen/arch/arm/include/asm/fixmap.h
>> @@ -36,12 +36,7 @@ extern void clear_fixmap(unsigned int map);
>>#define fix_to_virt(slot) ((void *)FIXMAP_ADDR(slot))
>>  -static inline unsigned int virt_to_fix(vaddr_t vaddr)
>> -{
>> -BUG_ON(vaddr >= FIXADDR_TOP || vaddr < FIXADDR_START);
>> -
>> -return ((vaddr - FIXADDR_START) >> PAGE_SHIFT);
>> -}
>> +extern unsigned int virt_to_fix(vaddr_t vaddr);
> 
> AFAICT, virt_to_fix() is not going to be implemented for the MPU code. This 
> implies that no-one should call it.
> 
> Also, none of the definitions in fixmap.h actually makes sense for the MPU. I 
> would prefer if we instead try to lmit the include of fixmap to when this is 
> strictly necessary. Looking for the inclusion in staging I could find:
> 
> 42sh> ack "\#include" | ack "fixmap" | ack -v x86
> arch/arm/acpi/lib.c:28:#include 
> arch/arm/kernel.c:19:#include 
> arch/arm/mm.c:27:#include 
> arch/arm/include/asm/fixmap.h:7:#include 
> arch/arm/include/asm/fixmap.h:8:#include 
> arch/arm/include/asm/pmap.h:6:#include 
> arch/arm/include/asm/early_printk.h:14:#include 
> common/efi/boot.c:30:#include 
> common/pmap.c:7:#include 
> drivers/acpi/apei/erst.c:36:#include 
> drivers/acpi/apei/apei-io.c:32:#include 
> drivers/char/xhci-dbc.c:30:#include 
> drivers/char/ehci-dbgp.c:16:#include 
> drivers/char/ns16550.c:40:#include 
> drivers/char/xen_pv_console.c:28:#include 
> 
> Some of them are gone after your rework. The only remaining that we care are 
> in kernel.h (but I think it can be removed after your series).

I think you are correct, so I reverted the virt_to_fix() change from this patch,
deleted the include of asm/fixmap.h in kernel.c and put this patch as the (last 
- 1)th
patch of the series. The building of Xen works fine.

Does below updated patch look good to you?
```
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index eb0413336b..8a7b79b4b5 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -15,7 +15,6 @@ config ARM
select HAS_DEVICE_TREE
select HAS_PASSTHROUGH
select HAS_PDX
-   select HAS_PMAP
select HAS_UBSAN
select IOMMU_FORCE_PT_SHARE

@@ -61,6 +60,7 @@ config PADDR_BITS

 config MMU
def_bool y
+   select HAS_PMAP

 source "arch/Kconfig"

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 0d433a32e7..bc3e5bd6f9 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -16,7 +16,6 @@
 #include 

 #include 
-#include 
 #include 
 #include 
```

I will update the commit message accordingly.

Kind regards,
Henry

> 
> So I think it would be feasible to not touch fixmap.h at all.
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

Re: [PATCH v5 13/13] xen/arm: mmu: enable SMMU subsystem only in MMU

[Henry Wang]

Hi Julien,

> On Aug 22, 2023, at 05:34, Julien Grall  wrote:
> 
> Hi,
> 
> On 14/08/2023 05:25, Henry Wang wrote:
>> From: Penny Zheng 
>> SMMU subsystem is only supported in MMU system, so we make it dependent
>> on CONFIG_HAS_MMU.
> 
> "only supported" as in it doesn't work with Xen or the HW is not supporting 
> it?

I think currently there are no hardware combination of MPU + SMMU, but
theoretically I think this is a valid combination since SMMU supports the linear
mapping. So would below reword looks good to you:

“Currently the hardware use case of connecting SMMU to MPU system is rarely
seen, so we make CONFIG_ARM_SMMU and CONFIG_ARM_SMMU_V3
dependent on CONFIG_MMU." 

> 
> Also, I am not entirely convinced that anything in passthrough would properly 
> work with MPU. At least none of the IOMMU drivers are. So I would consider to 
> completely disable HAS_PASSTHROUGH.

I agree, do you think adding below addition diff to this patch makes sense to 
you?
If so I guess would also need to mention this in commit message.

```
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 8a7b79b4b5..fd29d14ed6 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -13,7 +13,7 @@ config ARM
def_bool y
select HAS_ALTERNATIVE
select HAS_DEVICE_TREE
-   select HAS_PASSTHROUGH
+   select HAS_PASSTHROUGH if MMU
select HAS_PDX
select HAS_UBSAN
select IOMMU_FORCE_PT_SHARE
```

Kind regards,
Henry

Re: [PATCH] docs/misra: add exceptions to rules

[Stefano Stabellini]

On Mon, 21 Aug 2023, Jan Beulich wrote:
> On 19.08.2023 03:24, Stefano Stabellini wrote:
> > From: Stefano Stabellini 
> > 
> > During the discussions that led to the acceptable of the Rules, we
> > decided on a few exceptions that were not properly recorded in
> > rules.rst. Other times, the exceptions were decided later when it came
> > to enabling a rule in ECLAIR.
> 
> In a number of cases I'm unaware of such decisions. May be worth splitting
> the patch into a controversial and an uncontroversial part, such that the
> latter can go in while we discuss the former.

yes, I'll extract a couple of non-controversial changes and send them
out


> > --- a/docs/misra/rules.rst
> > +++ b/docs/misra/rules.rst
> > @@ -59,7 +59,8 @@ maintainers if you want to suggest a change.
> >   - Required
> >   - Precautions shall be taken in order to prevent the contents of a
> > header file being included more than once
> > - -
> > + - Files that are intended to be included more than once do not need to
> > +   conform to the directive (e.g. autogenerated or empty header files)
> 
> Auto-generated isn't a reason for an exception here. The logic generating
> the header can very well be adjusted. Same for empty headers - there's no
> reason they couldn't gain guards. An exception is needed for headers which
> we deliberately include more than once, in order to have a single central
> place for attributes, enumerations, and alike.

OK


> > @@ -106,7 +107,23 @@ maintainers if you want to suggest a change.
> > * - `Rule 2.1 
> > `_
> >   - Required
> >   - A project shall not contain unreachable code
> > - -
> > + - The following are allowed:
> > + - Invariantly constant conditions (e.g. while(0) { S; })
> 
> When (and why) was this decided? The example given looks exactly like what
> Misra wants us to not have.

This covers things like:

if (IS_ENABLED(CONFIG_HVM))

which could resolve in if (0) in certain configurations. I think we gave
feedback to Roberto that we wanted to keep this type of things as is.


> > + - Switch with a controlling value incompatible with labeled
> > +   statements
> 
> What does this mean?

I am not certain about this one actually. It could be when we have:

switch (var) {
  case 1:
  something();
  break;
  case 2:
  something();
  break;
}

and var could be 3 in theory?

Nicola, please confirm.


> > + - Functions that are intended to be never referenced from C
> > +   code, or are referenced in builds not under analysis (e.g.
> > +   'do_trap_fiq' for the former and 'check_for_unexpected_msi'
> > +   for the latter)
> 
> I agree with the "not referenced from C", but I don't see why the other
> kind couldn't be properly addressed.

I think you have a point


> > + - Unreachability caused by the following macros/functions is
> > +   deliberate: BUG, assert_failed, ERROR_EXIT, ERROR_EXIT_DOM,
> > +   PIN_FAIL, __builtin_unreachable, panic, do_unexpected_trap,
> > +   machine_halt, machine_restart, machine_reboot,
> > +   ASSERT_UNREACHABLE
> 
> Base infrastructure items like BUG() are imo fine to mention here. But
> specific items shouldn't be; the more we mention here, the more we invite
> the list to be grown. Note also how you mention items which no longer
> exist (ERROR_EXIT{,_DOM}, PIN_FAIL).

The question is whether we want this list to be exhaustive (so we want
to mention everything for which we make an exception) or only an example
(in which case just BUG is fine.)

Let's say we only mention BUG. Where should we keep the exhaustive list?
Is it OK if it only lives as an ECLAIR config file under
automation/eclair_analysis? There is another very similar question
below.

BTW I think both options are OK.

If we only mention BUG, we are basically saying that as a general rule
only BUG is an exception. Then we have a longer more detailed list for
ECLAIR because in practice things are always complicated.

On the other hand if we have the full list here, then the documentation
is more precise, but it looks a bit "strange" to see such a detailed
list in this document and also we need to make sure to keep the list
up-to-date.


> > + - asm-offsets.c, as they are not linked deliberately, because
> > +   they are used to generate definitions for asm modules
> > + - pure declarations (i.e. declarations without
> > +   initialization) are safe, as they are not executed
> 
> I don't think "pure declarations" is a term used in the spec. Let's better
> call it the way it is called elsewhere - declarations without initializer
> (where, as mentioned before, the term "unreachable code" is questionable
> anyway).

OK


> > @@ -167,7 +184,7 @@ maintainers if you want to suggest a change.
> > * - `Rule 5.6 
> > 

[PATCH v3 4/4] xen/vpci: header: status register handler

[Stewart Hildebrand]

Introduce a handler for the PCI status register, with ability to mask the
capabilities bit. The status register is write-1-to-clear, so introduce handling
for this type of register in vPCI.

Signed-off-by: Stewart Hildebrand 
---
v2->v3:
* new patch
---
 xen/drivers/vpci/header.c | 24 +++
 xen/drivers/vpci/vpci.c   | 41 +--
 xen/include/xen/vpci.h|  9 +
 3 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index b531ab03cec1..7061b85e337b 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -413,6 +413,17 @@ static void cf_check cmd_write(
 pci_conf_write16(pdev->sbdf, reg, cmd);
 }
 
+static uint32_t cf_check status_read(const struct pci_dev *pdev,
+ unsigned int reg, void *data)
+{
+struct vpci_header *header = data;
+
+if ( header->mask_cap_list )
+return pci_conf_read16(pdev->sbdf, reg) & ~(PCI_STATUS_CAP_LIST);
+
+return pci_conf_read16(pdev->sbdf, reg);
+}
+
 static void cf_check bar_write(
 const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data)
 {
@@ -556,6 +567,11 @@ static int cf_check init_bars(struct pci_dev *pdev)
 if ( rc )
 return rc;
 
+rc = vpci_add_rw1c_register(pdev->vpci, status_read, vpci_hw_write16,
+PCI_STATUS, 2, header);
+if ( rc )
+return rc;
+
 if ( !is_hardware_domain(pdev->domain) )
 {
 if ( !(pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST) )
@@ -583,6 +599,14 @@ static int cf_check init_bars(struct pci_dev *pdev)
 
 next &= ~3;
 
+if ( !next )
+/*
+ * If we don't have any supported capabilities to expose to the
+ * guest, mask the PCI_STATUS_CAP_LIST bit in the status
+ * register.
+ */
+header->mask_cap_list = true;
+
 while ( next && ttl )
 {
 uint8_t pos = next;
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 4a96aa50494d..a34d85f4ed3c 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -29,6 +29,7 @@ struct vpci_register {
 unsigned int offset;
 void *private;
 struct list_head node;
+bool rw1c : 1;
 };
 
 #ifdef __XEN__
@@ -157,9 +158,15 @@ uint32_t cf_check vpci_hw_read32(
 return pci_conf_read32(pdev->sbdf, reg);
 }
 
-int vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler,
-  vpci_write_t *write_handler, unsigned int offset,
-  unsigned int size, void *data)
+void cf_check vpci_hw_write16(
+const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data)
+{
+pci_conf_write16(pdev->sbdf, reg, val);
+}
+
+static int _vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler,
+  vpci_write_t *write_handler, unsigned int offset,
+  unsigned int size, void *data, bool rw1c)
 {
 struct list_head *prev;
 struct vpci_register *r;
@@ -179,6 +186,7 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t 
*read_handler,
 r->size = size;
 r->offset = offset;
 r->private = data;
+r->rw1c = rw1c;
 
 spin_lock(>lock);
 
@@ -205,6 +213,22 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t 
*read_handler,
 return 0;
 }
 
+int vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler,
+  vpci_write_t *write_handler, unsigned int offset,
+  unsigned int size, void *data)
+{
+return _vpci_add_register(vpci, read_handler, write_handler, offset, size,
+  data, false);
+}
+
+int vpci_add_rw1c_register(struct vpci *vpci, vpci_read_t *read_handler,
+   vpci_write_t *write_handler, unsigned int offset,
+   unsigned int size, void *data)
+{
+return _vpci_add_register(vpci, read_handler, write_handler, offset, size,
+  data, true);
+}
+
 int vpci_remove_register(struct vpci *vpci, unsigned int offset,
  unsigned int size)
 {
@@ -419,11 +443,6 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, 
unsigned int size)
 
 /*
  * Perform a maybe partial write to a register.
- *
- * Note that this will only work for simple registers, if Xen needs to
- * trap accesses to rw1c registers (like the status PCI header register)
- * the logic in vpci_write will have to be expanded in order to correctly
- * deal with them.
  */
 static void vpci_write_helper(const struct pci_dev *pdev,
   const struct vpci_register *r, unsigned int size,
@@ -433,9 +452,11 @@ static void vpci_write_helper(const struct pci_dev *pdev,
 
 if ( size != r->size )
 {
-uint32_t val;
+uint32_t val = 0;

[PATCH v3 3/4] xen/vpci: header: filter PCI capabilities

[Stewart Hildebrand]

Currently, Xen vPCI only supports virtualizing the MSI and MSI-X capabilities.
Hide all other PCI capabilities (including extended capabilities) from domUs for
now, even though there may be certain devices/drivers that depend on being able
to discover certain capabilities.

We parse the physical PCI capabilities linked list and add vPCI register
handlers for the next elements, inserting our own next value, thus presenting a
modified linked list to the domU.

Introduce helper functions vpci_hw_read8 and vpci_read_val. The vpci_read_val
helper function returns a fixed value, which may be used for RAZ registers, or
registers whose value doesn't change.

Introduce pci_find_next_cap_ttl() helper while adapting the logic from
pci_find_next_cap() to suit our needs, and implement the existing
pci_find_next_cap() in terms of the new helper.

Signed-off-by: Stewart Hildebrand 
---
v2->v3:
* get rid of > 0 in loop condition
* implement pci_find_next_cap in terms of new pci_find_next_cap_ttl function so
  that hypothetical future callers wouldn't be required to pass 
* change NULL to (void *)0 for RAZ value passed to vpci_read_val
* change type of ttl to unsigned int
* remember to mask off the low 2 bits of next in the initial loop iteration
* change return type of pci_find_next_cap and pci_find_next_cap_ttl
* avoid wrapping the PCI_STATUS_CAP_LIST condition by using ! instead of == 0

v1->v2:
* change type of ttl to int
* use switch statement instead of if/else
* adapt existing pci_find_next_cap helper instead of rolling our own
* pass ttl as in/out
* "pass through" the lower 2 bits of the next pointer
* squash helper functions into this patch to avoid transient dead code situation
* extended capabilities RAZ/WI
---
 xen/drivers/pci/pci.c | 24 +-
 xen/drivers/vpci/header.c | 69 +++
 xen/drivers/vpci/vpci.c   | 12 +++
 xen/include/xen/pci.h |  5 ++-
 xen/include/xen/vpci.h|  5 +++
 5 files changed, 106 insertions(+), 9 deletions(-)

diff --git a/xen/drivers/pci/pci.c b/xen/drivers/pci/pci.c
index 3bcb74040284..f60051694dc5 100644
--- a/xen/drivers/pci/pci.c
+++ b/xen/drivers/pci/pci.c
@@ -39,30 +39,38 @@ int pci_find_cap_offset(pci_sbdf_t sbdf, u8 cap)
 return 0;
 }
 
-int pci_find_next_cap(pci_sbdf_t sbdf, u8 pos, int cap)
+uint8_t pci_find_next_cap_ttl(pci_sbdf_t sbdf, uint8_t pos,
+  bool (*is_match)(uint8_t), unsigned int *ttl)
 {
-u8 id;
-int ttl = 48;
+uint8_t id;
 
-while ( ttl-- )
+while ( (*ttl)-- )
 {
 pos = pci_conf_read8(sbdf, pos);
 if ( pos < 0x40 )
 break;
 
-pos &= ~3;
-id = pci_conf_read8(sbdf, pos + PCI_CAP_LIST_ID);
+id = pci_conf_read8(sbdf, (pos & ~3) + PCI_CAP_LIST_ID);
 
 if ( id == 0xff )
 break;
-if ( id == cap )
+if ( is_match(id) )
 return pos;
 
-pos += PCI_CAP_LIST_NEXT;
+pos = (pos & ~3) + PCI_CAP_LIST_NEXT;
 }
+
 return 0;
 }
 
+uint8_t pci_find_next_cap(pci_sbdf_t sbdf, uint8_t pos,
+  bool (*is_match)(uint8_t))
+{
+unsigned int ttl = 48;
+
+return pci_find_next_cap_ttl(sbdf, pos, is_match, ) & ~3;
+}
+
 /**
  * pci_find_ext_capability - Find an extended capability
  * @sbdf: PCI device to query
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 60f7049e3498..b531ab03cec1 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -513,6 +513,18 @@ static void cf_check rom_write(
 rom->addr = val & PCI_ROM_ADDRESS_MASK;
 }
 
+static bool cf_check vpci_cap_supported(uint8_t id)
+{
+switch ( id )
+{
+case PCI_CAP_ID_MSI:
+case PCI_CAP_ID_MSIX:
+return true;
+default:
+return false;
+}
+}
+
 static int cf_check init_bars(struct pci_dev *pdev)
 {
 uint16_t cmd;
@@ -544,6 +556,63 @@ static int cf_check init_bars(struct pci_dev *pdev)
 if ( rc )
 return rc;
 
+if ( !is_hardware_domain(pdev->domain) )
+{
+if ( !(pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST) )
+{
+/* RAZ/WI */
+rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
+   PCI_CAPABILITY_LIST, 1, (void *)0);
+if ( rc )
+return rc;
+}
+else
+{
+/* Only expose capabilities to the guest that vPCI can handle. */
+uint8_t next;
+unsigned int ttl = 48;
+
+next = pci_find_next_cap_ttl(pdev->sbdf, PCI_CAPABILITY_LIST,
+ vpci_cap_supported, );
+
+rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
+   PCI_CAPABILITY_LIST, 1,
+   (void *)(uintptr_t)next);
+if ( rc )
+return rc;
+
+next &= ~3;
+
+while ( next 

[PATCH v3 2/4] xen/pci: convert pci_find_*cap* to pci_sbdf_t

[Stewart Hildebrand]

Convert pci_find_*cap* functions and call sites to pci_sbdf_t, and remove some
now unused local variables. No functional change.

Signed-off-by: Stewart Hildebrand 
---
I built with EXTRA_CFLAGS_XEN_CORE="-Wunused-but-set-variable" (and
unfortunately -Wno-error=unused-but-set-variable too) to identify locations of
unneeded local variables as a result of the change to pci_sbdf_t.

v2->v3:
* new patch
---
 xen/arch/x86/msi.c | 47 ++
 xen/drivers/char/ehci-dbgp.c   |  3 +-
 xen/drivers/passthrough/amd/iommu_detect.c |  2 +-
 xen/drivers/passthrough/ats.c  |  4 +-
 xen/drivers/passthrough/ats.h  |  6 ++-
 xen/drivers/passthrough/msi.c  |  6 +--
 xen/drivers/passthrough/pci.c  | 20 -
 xen/drivers/passthrough/vtd/quirks.c   | 10 ++---
 xen/drivers/passthrough/vtd/x86/ats.c  |  3 +-
 xen/drivers/pci/pci.c  | 28 ++---
 xen/drivers/vpci/msi.c |  4 +-
 xen/drivers/vpci/msix.c|  4 +-
 xen/include/xen/pci.h  |  9 ++---
 13 files changed, 53 insertions(+), 93 deletions(-)

diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index d0bf63df1def..13a1f0319a8a 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -283,7 +283,7 @@ static void msi_set_enable(struct pci_dev *dev, int enable)
 u8 slot = PCI_SLOT(dev->devfn);
 u8 func = PCI_FUNC(dev->devfn);
 
-pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSI);
+pos = pci_find_cap_offset(dev->sbdf, PCI_CAP_ID_MSI);
 if ( pos )
 __msi_set_enable(seg, bus, slot, func, pos, enable);
 }
@@ -291,12 +291,9 @@ static void msi_set_enable(struct pci_dev *dev, int enable)
 static void msix_set_enable(struct pci_dev *dev, int enable)
 {
 int pos;
-u16 control, seg = dev->seg;
-u8 bus = dev->bus;
-u8 slot = PCI_SLOT(dev->devfn);
-u8 func = PCI_FUNC(dev->devfn);
+u16 control;
 
-pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSIX);
+pos = pci_find_cap_offset(dev->sbdf, PCI_CAP_ID_MSIX);
 if ( pos )
 {
 control = pci_conf_read16(dev->sbdf, msix_control_reg(pos));
@@ -318,17 +315,12 @@ static bool msi_set_mask_bit(struct irq_desc *desc, bool 
host, bool guest)
 {
 struct msi_desc *entry = desc->msi_desc;
 struct pci_dev *pdev;
-u16 seg, control;
-u8 bus, slot, func;
+u16 control;
 bool flag = host || guest, maskall;
 
 ASSERT(spin_is_locked(>lock));
 BUG_ON(!entry || !entry->dev);
 pdev = entry->dev;
-seg = pdev->seg;
-bus = pdev->bus;
-slot = PCI_SLOT(pdev->devfn);
-func = PCI_FUNC(pdev->devfn);
 switch ( entry->msi_attrib.type )
 {
 case PCI_CAP_ID_MSI:
@@ -608,13 +600,10 @@ static int msi_capability_init(struct pci_dev *dev,
 struct msi_desc *entry;
 int pos;
 unsigned int i, mpos;
-u16 control, seg = dev->seg;
-u8 bus = dev->bus;
-u8 slot = PCI_SLOT(dev->devfn);
-u8 func = PCI_FUNC(dev->devfn);
+u16 control;
 
 ASSERT(pcidevs_locked());
-pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSI);
+pos = pci_find_cap_offset(dev->sbdf, PCI_CAP_ID_MSI);
 if ( !pos )
 return -ENODEV;
 control = pci_conf_read16(dev->sbdf, msi_control_reg(pos));
@@ -685,8 +674,8 @@ static u64 read_pci_mem_bar(u16 seg, u8 bus, u8 slot, u8 
func, u8 bir, int vf)
 {
 struct pci_dev *pdev = pci_get_pdev(NULL,
 PCI_SBDF(seg, bus, slot, func));
-unsigned int pos = pci_find_ext_capability(seg, bus,
-   PCI_DEVFN(slot, func),
+unsigned int pos = pci_find_ext_capability(PCI_SBDF(seg, bus, slot,
+func),
PCI_EXT_CAP_ID_SRIOV);
 uint16_t ctrl = pci_conf_read16(PCI_SBDF(seg, bus, slot, func),
 pos + PCI_SRIOV_CTRL);
@@ -777,8 +766,7 @@ static int msix_capability_init(struct pci_dev *dev,
 u8 slot = PCI_SLOT(dev->devfn);
 u8 func = PCI_FUNC(dev->devfn);
 bool maskall = msix->host_maskall, zap_on_error = false;
-unsigned int pos = pci_find_cap_offset(seg, bus, slot, func,
-   PCI_CAP_ID_MSIX);
+unsigned int pos = pci_find_cap_offset(dev->sbdf, PCI_CAP_ID_MSIX);
 
 if ( !pos )
 return -ENODEV;
@@ -1102,12 +1090,7 @@ static void _pci_cleanup_msix(struct arch_msix *msix)
 static void __pci_disable_msix(struct msi_desc *entry)
 {
 struct pci_dev *dev = entry->dev;
-u16 seg = dev->seg;
-u8 bus = dev->bus;
-u8 slot = PCI_SLOT(dev->devfn);
-u8 func = PCI_FUNC(dev->devfn);
-unsigned int pos = pci_find_cap_offset(seg, bus, slot, func,
-   PCI_CAP_ID_MSIX);
+unsigned int pos = 

[PATCH v3 1/4] xen/pci: address a violation of MISRA C:2012 Rule 8.3

[Stewart Hildebrand]

Make the paramater names of the prototype match the definition. No functional
change.

Signed-off-by: Stewart Hildebrand 
---
v2->v3:
* new patch
---
 xen/drivers/pci/pci.c | 2 +-
 xen/include/xen/pci.h | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/pci/pci.c b/xen/drivers/pci/pci.c
index e411876a1518..c73a8c4124af 100644
--- a/xen/drivers/pci/pci.c
+++ b/xen/drivers/pci/pci.c
@@ -80,7 +80,7 @@ int pci_find_ext_capability(int seg, int bus, int devfn, int 
cap)
 /**
  * pci_find_next_ext_capability - Find another extended capability
  * @seg/@bus/@devfn: PCI device to query
- * @pos: starting position
+ * @start: starting position
  * @cap: capability code
  *
  * Returns the address of the requested extended capability structure
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 5975ca2f3032..a8c8c4ff11c3 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -196,7 +196,8 @@ int pci_mmcfg_write(unsigned int seg, unsigned int bus,
 int pci_find_cap_offset(u16 seg, u8 bus, u8 dev, u8 func, u8 cap);
 int pci_find_next_cap(u16 seg, u8 bus, unsigned int devfn, u8 pos, int cap);
 int pci_find_ext_capability(int seg, int bus, int devfn, int cap);
-int pci_find_next_ext_capability(int seg, int bus, int devfn, int pos, int 
cap);
+int pci_find_next_ext_capability(int seg, int bus, int devfn, int start,
+ int cap);
 const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus,
   unsigned int *dev, unsigned int *func);
 const char *parse_pci_seg(const char *, unsigned int *seg, unsigned int *bus,
-- 
2.41.0

[PATCH v3 0/4] vPCI capabilities filtering

[Stewart Hildebrand]

This small series enables vPCI to filter which PCI capabilities we expose to a
domU. This series adds vPCI register handlers within
xen/drivers/vpci/header.c:init_bars(), along with some supporting functions.

Note there are minor rebase conflicts with the in-progress vPCI series [1].
These conflicts fall into the category of functions and code being added
adjacent to one another, so are easily resolved. I did not identify any
dependency on the vPCI locking work, and the two series deal with different
aspects of emulating the PCI header.

Future work may involve adding handlers for more registers in the vPCI header,
such as VID/DID, etc. Future work may also involve exposing additional
capabilities to the guest for broader device/driver support.

v2->v3:
* drop RFC "xen/vpci: header: avoid cast for value passed to vpci_read_val"
* minor misra C violation fixup in preparatory patch
* switch to pci_sbdf_t in preparatory patch
* introduce status handler

v1->v2:
* squash helper functions into the patch where they are used to avoid transient
  dead code situation
* add new RFC patch, possibly throwaway, to get an idea of what it would look
  like to get rid of the (void *)(uintptr_t) cast by introducing a new memory
  allocation

[1] https://lists.xenproject.org/archives/html/xen-devel/2023-07/msg01281.html

Stewart Hildebrand (4):
  xen/pci: address a violation of MISRA C:2012 Rule 8.3
  xen/pci: convert pci_find_*cap* to pci_sbdf_t
  xen/vpci: header: filter PCI capabilities
  xen/vpci: header: status register handler

 xen/arch/x86/msi.c | 47 +++
 xen/drivers/char/ehci-dbgp.c   |  3 +-
 xen/drivers/passthrough/amd/iommu_detect.c |  2 +-
 xen/drivers/passthrough/ats.c  |  4 +-
 xen/drivers/passthrough/ats.h  |  6 +-
 xen/drivers/passthrough/msi.c  |  6 +-
 xen/drivers/passthrough/pci.c  | 20 ++---
 xen/drivers/passthrough/vtd/quirks.c   | 10 +--
 xen/drivers/passthrough/vtd/x86/ats.c  |  3 +-
 xen/drivers/pci/pci.c  | 50 +++-
 xen/drivers/vpci/header.c  | 93 ++
 xen/drivers/vpci/msi.c |  4 +-
 xen/drivers/vpci/msix.c|  4 +-
 xen/drivers/vpci/vpci.c| 53 +---
 xen/include/xen/pci.h  | 11 ++-
 xen/include/xen/vpci.h | 14 
 16 files changed, 221 insertions(+), 109 deletions(-)


base-commit: 3fae7c56b313a12288a05e0a8c14c47bfd4dc40e
-- 
2.41.0

[PATCH v2] docs/misra: add exceptions to rules

[Stefano Stabellini]

From: Stefano Stabellini 

During the discussions that led to the acceptable of the Rules, we
decided on a few exceptions that were not properly recorded in
rules.rst. Other times, the exceptions were decided later when it came
to enabling a rule in ECLAIR.

Either way, update rules.rst with appropriate notes.

Signed-off-by: Stefano Stabellini 
---
v2:
- remove autogenerated from D4.10
- remove R2.1
- remove R5.6
- remove R7.1
- reword R8.3
---
 docs/misra/rules.rst | 36 +---
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
index 8f0e4d3f25..62bd4620fd 100644
--- a/docs/misra/rules.rst
+++ b/docs/misra/rules.rst
@@ -59,7 +59,8 @@ maintainers if you want to suggest a change.
  - Required
  - Precautions shall be taken in order to prevent the contents of a
header file being included more than once
- -
+ - Files that are intended to be included more than once do not need to
+   conform to the directive
 
* - `Dir 4.11 
`_
  - Required
@@ -117,7 +131,7 @@ maintainers if you want to suggest a change.
  - Required
  - The character sequences /* and // shall not be used within a
comment
- -
+ - Comments containing hyperlinks inside C-style block comments are safe
 
* - `Rule 3.2 
`_
  - Required
@@ -239,13 +256,16 @@ maintainers if you want to suggest a change.
  - Required
  - All declarations of an object or function shall use the same
names and type qualifiers
- -
+ - The type ret_t maybe be deliberately used and defined as int or
+   long depending on the type of guest to service
 
* - `Rule 8.4 
`_
  - Required
  - A compatible declaration shall be visible when an object or
function with external linkage is defined
- -
+ - Allowed exceptions: asm-offsets.c (definitions for asm modules
+   not called from C code), gcov_base.c (definitions only used in
+   non-release builds)
 
* - `Rule 8.5 
`_
  - Required
@@ -369,7 +389,9 @@ maintainers if you want to suggest a change.
  - Required
  - Expressions resulting from the expansion of macro parameters
shall be enclosed in parentheses
- -
+ - Extra parentheses are not required when macro parameters are used
+   as function arguments, as macro arguments, array indices, lhs in
+   assignments
 
* - `Rule 20.13 
`_
  - Required
-- 
2.25.1

[xen-4.17-testing test] 182410: tolerable FAIL - PUSHED

[osstest service owner]

flight 182410 xen-4.17-testing real [real]
flight 182415 xen-4.17-testing real-retest [real]
http://logs.test-lab.xenproject.org/osstest/logs/182410/
http://logs.test-lab.xenproject.org/osstest/logs/182415/

Failures :-/ but no regressions.

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

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

version targeted for testing:
 xen  8d84be5b557b27e9cc53e48285aebad28a48468c
baseline version:
 xen  322a20add00a4687cd46d9183616fa6fecbef81f

Last test of basis   182237  2023-08-08 

[PATCH v2] docs/misra: document gcc-specific behavior with shifting signed integers

[Stefano Stabellini]

From: Stefano Stabellini 

Signed-off-by: Stefano Stabellini 
---
v2:
- split << and >>
- do not use the word "shift" instead of << or >>
---
 docs/misra/C-language-toolchain.rst | 9 +
 1 file changed, 9 insertions(+)

diff --git a/docs/misra/C-language-toolchain.rst 
b/docs/misra/C-language-toolchain.rst
index 785aed1eaf..4c4942a113 100644
--- a/docs/misra/C-language-toolchain.rst
+++ b/docs/misra/C-language-toolchain.rst
@@ -200,6 +200,15 @@ The table columns are as follows:
  - ARM64, X86_64
  - See Section "6.29 Designated Initializers" of GCC_MANUAL
 
+   * - Signed << compiler-defined behavior
+ - All architectures
+ - See Section "4.5 Integers" of GCC_MANUAL. As an extension to the
+   C language, GCC does not use the latitude given in C99 and C11
+   only to treat certain aspects of signed << as undefined.
+
+   * - Signed >> acts on negative numbers by sign extension
+ - All architectures
+ - See Section "4.5 Integers" of GCC_MANUAL.
 
 Translation Limits
 __
-- 
2.25.1

Re: [PATCH] docs/misra: document gcc-specific behavior with shifting signed integers

[Stefano Stabellini]

On Mon, 21 Aug 2023, Jan Beulich wrote:
> On 19.08.2023 02:33, Stefano Stabellini wrote:
> > From: Stefano Stabellini 
> > 
> > Signed-off-by: Stefano Stabellini 
> > ---
> > Changes in v2:
> > - use "shift" instead of << or >>
> > - use All Architectures (I haven't changed all the other instances of
> > x86/arm in the file yet)
> > ---
> >  docs/misra/C-language-toolchain.rst | 6 ++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/docs/misra/C-language-toolchain.rst 
> > b/docs/misra/C-language-toolchain.rst
> > index 785aed1eaf..f5ca7bd2c8 100644
> > --- a/docs/misra/C-language-toolchain.rst
> > +++ b/docs/misra/C-language-toolchain.rst
> > @@ -200,6 +200,12 @@ The table columns are as follows:
> >   - ARM64, X86_64
> >   - See Section "6.29 Designated Initializers" of GCC_MANUAL
> >  
> > +   * - Signed shift acts on negative numbers by sign extension
> > + - All architectures
> > + - See Section "4.5 Integers" of GCC_MANUAL. As an extension to the
> > +   C language, GCC does not use the latitude given in C99 and C11
> > +   only to treat certain aspects of signed shift as undefined.
> 
> I'm sorry, but that's still not what the doc says. Replacing << and >> by
> "shifts" was imo wrong. What's needed instead is that either this is split
> into two top-level bullet points (one for << and one for >>), or the first
> sub-bullet-point (which acts as kind of the title) be generalized, with
> the << and >> details fully moved to the "explanatory" sub-bullet-point.

I think I got your point now

Re: [XEN PATCH v2 1/2] automation/eclair: avoid unintentional ECLAIR analysis

[Stefano Stabellini]

On Mon, 21 Aug 2023, Simone Ballarin wrote:
> With this patch, ECLAIR jobs will need to be manually
> started for "people/.*" pipelines and will not be triggered
> if the WTOKEN variable is missing.
> 
> This avoids occupying the runner on analyzes that might
> not be used by developers.
> 
> If developers want to analyze their own repositories
> they need to launch them from GitLab.
> 
> Signed-off-by: Simone Ballarin 

Great job!

Reviewed-by: Stefano Stabellini 


> ---
> Changes in v2:
> - avoid ECLAIR jobs if the WTOKEN variable is not defined.
> ---
>  automation/gitlab-ci/analyze.yaml | 22 +-
>  automation/scripts/eclair |  5 -
>  2 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/automation/gitlab-ci/analyze.yaml 
> b/automation/gitlab-ci/analyze.yaml
> index 4aa4abe2ee..bd9a68de31 100644
> --- a/automation/gitlab-ci/analyze.yaml
> +++ b/automation/gitlab-ci/analyze.yaml
> @@ -18,28 +18,40 @@
>- '*.log'
>  reports:
>codequality: gl-code-quality-report.json
> +  rules:
> +- if: $WTOKEN == null
> +  when: never
> +- when: always
>needs: []
>  
> -eclair-x86_64:
> +.eclair-analysis:triggered:
>extends: .eclair-analysis
> +  allow_failure: true
> +  rules:
> +- if: $WTOKEN && $CI_PROJECT_PATH =~ /^xen-project\/people\/.*$/
> +  when: manual
> +- !reference [.eclair-analysis, rules]
> +
> +eclair-x86_64:
> +  extends: .eclair-analysis:triggered
>variables:
>  LOGFILE: "eclair-x86_64.log"
>  VARIANT: "X86_64"
>  RULESET: "Set1"
> -  allow_failure: true
>  
>  eclair-ARM64:
> -  extends: .eclair-analysis
> +  extends: .eclair-analysis:triggered
>variables:
>  LOGFILE: "eclair-ARM64.log"
>  VARIANT: "ARM64"
>  RULESET: "Set1"
> -  allow_failure: true
>  
>  .eclair-analysis:on-schedule:
>extends: .eclair-analysis
>rules:
> -- if: $CI_PIPELINE_SOURCE == "schedule"
> +- if: $CI_PIPELINE_SOURCE != "schedule"
> +  when: never
> +- !reference [.eclair-analysis, rules]
>  
>  eclair-x86_64-Set1:on-schedule:
>extends: .eclair-analysis:on-schedule
> diff --git a/automation/scripts/eclair b/automation/scripts/eclair
> index 813a56eb6a..14e47a6f97 100755
> --- a/automation/scripts/eclair
> +++ b/automation/scripts/eclair
> @@ -4,11 +4,6 @@ ECLAIR_ANALYSIS_DIR=automation/eclair_analysis
>  ECLAIR_DIR="${ECLAIR_ANALYSIS_DIR}/ECLAIR"
>  ECLAIR_OUTPUT_DIR=$(realpath "${ECLAIR_OUTPUT_DIR}")
>  
> -if [ -z "${WTOKEN:-}" ]; then
> -echo "Failure: the WTOKEN variable is not defined." >&2
> -exit 1
> -fi
> -
>  "${ECLAIR_ANALYSIS_DIR}/prepare.sh" "${VARIANT}"
>  
>  ex=0
> -- 
> 2.34.1
> 

Re: [XEN PATCH v2 2/2] automation: avoid pipelines on specific branches

[Stefano Stabellini]

On Mon, 21 Aug 2023, Simone Ballarin wrote:
> This patch avoids the execution of pipelines in the
> following branches:
> - master
> - smoke
> - coverirty-tested/.*
> - stable-.*
> 
> The job-level exclusions have been removed as they are
> pointless with this new workspace-level exclusion.
> 
> Signed-off-by: Simone Ballarin 

Reviewed-by: Stefano Stabellini 


> ---
> Changes in v2:
> - remove useless except clause in .yocto-test.
> ---
>  .gitlab-ci.yml  |  6 ++
>  automation/gitlab-ci/build.yaml | 11 ---
>  automation/gitlab-ci/test.yaml  |  5 -
>  3 files changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index ee5430b8b7..ef4484e09a 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -1,3 +1,9 @@
> +workflow:
> +  rules:
> +- if: $CI_COMMIT_BRANCH =~ 
> /^(master|smoke|^coverity-tested\/.*|stable-.*)$/
> +  when: never
> +- when: always
> +
>  stages:
>- analyze
>- build
> diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
> index 1a4a5e490d..b633facff4 100644
> --- a/automation/gitlab-ci/build.yaml
> +++ b/automation/gitlab-ci/build.yaml
> @@ -12,11 +12,6 @@
>- '*/*.log'
>  when: always
>needs: []
> -  except:
> -- master
> -- smoke
> -- /^coverity-tested\/.*/
> -- /^stable-.*/
>  
>  .gcc-tmpl:
>variables: 
> @@ -214,11 +209,6 @@
>  .yocto-test:
>stage: build
>image: registry.gitlab.com/xen-project/xen/${CONTAINER}
> -  except:
> -- master
> -- smoke
> -- /^coverity-tested\/.*/
> -- /^stable-.*/
>script:
>  - ./automation/build/yocto/build-yocto.sh -v --log-dir=./logs 
> --xen-dir=`pwd` ${YOCTO_BOARD} ${YOCTO_OUTPUT}
>variables:
> @@ -269,7 +259,6 @@
>  .test-jobs-artifact-common:
>stage: build
>needs: []
> -  except: !reference [.test-jobs-common, except]
>  
>  # Arm test artifacts
>  
> diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
> index 810631bc46..8737f367c8 100644
> --- a/automation/gitlab-ci/test.yaml
> +++ b/automation/gitlab-ci/test.yaml
> @@ -1,11 +1,6 @@
>  .test-jobs-common:
>stage: test
>image: registry.gitlab.com/xen-project/xen/${CONTAINER}
> -  except:
> -- master
> -- smoke
> -- /^coverity-tested\/.*/
> -- /^stable-.*/
>  
>  .arm64-test-needs: 
>- alpine-3.18-arm64-rootfs-export
> -- 
> 2.34.1
> 

Re: [PATCH v5 13/13] xen/arm: mmu: enable SMMU subsystem only in MMU

[Julien Grall]

Hi,

On 14/08/2023 05:25, Henry Wang wrote:

From: Penny Zheng 

SMMU subsystem is only supported in MMU system, so we make it dependent
on CONFIG_HAS_MMU.


"only supported" as in it doesn't work with Xen or the HW is not 
supporting it?


Also, I am not entirely convinced that anything in passthrough would 
properly work with MPU. At least none of the IOMMU drivers are. So I 
would consider to completely disable HAS_PASSTHROUGH.




Signed-off-by: Penny Zheng 
Signed-off-by: Wei Chen 
Signed-off-by: Henry Wang 
Acked-by: Jan Beulich 
---
v5:
- Add Acked-by tag from Jan.
v4:
- No change
v3:
- new patch
---
  xen/drivers/passthrough/Kconfig | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig
index 864fcf3b0c..ebb350bc37 100644
--- a/xen/drivers/passthrough/Kconfig
+++ b/xen/drivers/passthrough/Kconfig
@@ -5,6 +5,7 @@ config HAS_PASSTHROUGH
  if ARM
  config ARM_SMMU
bool "ARM SMMUv1 and v2 driver"
+   depends on MMU
default y
---help---
  Support for implementations of the ARM System MMU architecture
@@ -15,7 +16,7 @@ config ARM_SMMU
  
  config ARM_SMMU_V3

bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support" if EXPERT
-   depends on ARM_64 && (!ACPI || BROKEN)
+   depends on ARM_64 && (!ACPI || BROKEN) && MMU
---help---
 Support for implementations of the ARM System MMU architecture
 version 3. Driver is in experimental stage and should not be used in


Cheers,

--
Julien Grall

Re: [PATCH v5 12/13] xen/arm: mmu: relocate copy_from_paddr() to setup.c

[Julien Grall]

Hi,

On 14/08/2023 05:25, Henry Wang wrote:

From: Penny Zheng 

Function copy_from_paddr() is defined in asm/setup.h, so it is better
to be implemented in setup.c.


I don't agree with this reasoning. We used setup.h to declare prototype 
for function that are out of setup.c.


Looking at the overall of this series, it is unclear to me what is the 
difference between mmu/mm.c and mmu/setup.c. I know this is technically 
not a new problem but as we split the code, it would be a good 
opportunity to have a better split.


For instance, we have setup_mm() defined in setup.c but 
setup_pagetables() and mm.c. Both are technically related to the memory 
management. So having them in separate file is a bit odd.


I also don't like the idea of having again a massive mm.c files. So 
maybe we need a split like:

  * File 1: Boot CPU0 MM bringup (mmu/setup.c)
  * File 2: Secondary CPUs MM bringup (mmu/smpboot.c)
  * File 3: Page tables update. (mmu/pt.c)

Ideally file 1 should contain only init code/data so it can be marked as 
.init. So the static pagetables may want to be defined in mmu/pt.c.


Bertrand, Stefano, any thoughts?

[...]


diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
index e05cca3f86..889ada6b87 100644
--- a/xen/arch/arm/mmu/setup.c
+++ b/xen/arch/arm/mmu/setup.c
@@ -329,6 +329,33 @@ void __init setup_mm(void)
  }
  #endif
  
+/*


Why did the second '*' disappear?


+ * copy_from_paddr - copy data from a physical address
+ * @dst: destination virtual address
+ * @paddr: source physical address
+ * @len: length to copy
+ */
+void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
+{
+void *src = (void *)FIXMAP_ADDR(FIXMAP_MISC);
+
+while (len) {
+unsigned long l, s;
+
+s = paddr & (PAGE_SIZE-1);
+l = min(PAGE_SIZE - s, len);
+
+set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr), PAGE_HYPERVISOR_WC);
+memcpy(dst, src + s, l);
+clean_dcache_va_range(dst, l);
+clear_fixmap(FIXMAP_MISC);
+
+paddr += l;
+dst += l;
+len -= l;
+}
+}
+
  /*
   * Local variables:
   * mode: C


Cheers,

--
Julien Grall

Re: [PATCH v5 10/13] xen/arm: mmu: move MMU-specific setup_mm to mmu/setup.c

[Julien Grall]

Hi,

On 14/08/2023 05:25, Henry Wang wrote:

From: Penny Zheng 

setup_mm is used for Xen to setup memory management subsystem at boot
time, like boot allocator, direct-mapping, xenheap initialization,
frametable and static memory pages.

We could inherit some components seamlessly in later MPU system like
boot allocator, whilst we need to implement some components differently
in MPU, like xenheap, etc. There are some components that is specific
to MMU only, like direct-mapping.

In the commit, we move MMU-specific components into mmu/setup.c, in
preparation of implementing MPU version of setup_mm later in future
commit. Also, make init_pdx(), init_staticmem_pages(), setup_mm(), and
populate_boot_allocator() public for future MPU inplementation.


Typo: s/inplementation/implementation/



Signed-off-by: Penny Zheng 
Signed-off-by: Wei Chen 
Signed-off-by: Henry Wang 
---
v5:
- No change
v4:
- No change
---
  xen/arch/arm/include/asm/setup.h |   5 +
  xen/arch/arm/mmu/Makefile|   1 +
  xen/arch/arm/mmu/setup.c | 339 +++
  xen/arch/arm/setup.c | 326 +
  4 files changed, 349 insertions(+), 322 deletions(-)
  create mode 100644 xen/arch/arm/mmu/setup.c

diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index f0f64d228c..0922549631 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -156,6 +156,11 @@ struct bootcmdline 
*boot_cmdline_find_by_kind(bootmodule_kind kind);
  struct bootcmdline * boot_cmdline_find_by_name(const char *name);
  const char *boot_module_kind_as_string(bootmodule_kind kind);
  
+extern void init_pdx(void);

+extern void init_staticmem_pages(void);
+extern void populate_boot_allocator(void);
+extern void setup_mm(void);


Please avoid the 'extern' for new function declaration.


+
  extern uint32_t hyp_traps_vector[];
  void init_traps(void);
  
diff --git a/xen/arch/arm/mmu/Makefile b/xen/arch/arm/mmu/Makefile

index b18cec4836..4aa1fb466d 100644
--- a/xen/arch/arm/mmu/Makefile
+++ b/xen/arch/arm/mmu/Makefile
@@ -1 +1,2 @@
  obj-y += mm.o
+obj-y += setup.o
diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
new file mode 100644
index 00..e05cca3f86
--- /dev/null
+++ b/xen/arch/arm/mmu/setup.c
@@ -0,0 +1,339 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * xen/arch/arm/mmu/setup.c
+ *
+ * MMU-specific early bringup code for an ARMv7-A with virt extensions.


You modify the comment in arm/setup.c to mention ARMv8 but not here. The 
former feels somewhat unrelated.



+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#ifdef CONFIG_ARM_32


AFAICT, mmu/mm.c has nothing common between arm32 and arm64. So can we 
introduce arm{32, 64}/mmu/setup.c and move the respective code there?


[...]


+void __init setup_mm(void)
+{
+paddr_t ram_start, ram_end, ram_size, e, bank_start, bank_end, bank_size;
+paddr_t static_heap_end = 0, static_heap_size = 0;
+unsigned long heap_pages, xenheap_pages, domheap_pages;
+unsigned int i;
+const uint32_t ctr = READ_CP32(CTR);
+
+if ( !bootinfo.mem.nr_banks )
+panic("No memory bank\n");
+
+/* We only supports instruction caches implementing the IVIPT extension. */
+if ( ((ctr >> CTR_L1IP_SHIFT) & CTR_L1IP_MASK) == ICACHE_POLICY_AIVIVT )
+panic("AIVIVT instruction cache not supported\n");


Thi check is unlikely going to be MMU setup.c.


+
+init_pdx();
+
+ram_start = bootinfo.mem.bank[0].start;
+ram_size  = bootinfo.mem.bank[0].size;
+ram_end   = ram_start + ram_size;
+
+for ( i = 1; i < bootinfo.mem.nr_banks; i++ )
+{
+bank_start = bootinfo.mem.bank[i].start;
+bank_size = bootinfo.mem.bank[i].size;
+bank_end = bank_start + bank_size;
+
+ram_size  = ram_size + bank_size;
+ram_start = min(ram_start,bank_start);
+ram_end   = max(ram_end,bank_end);
+}
+
+total_pages = ram_size >> PAGE_SHIFT;
+
+if ( bootinfo.static_heap )
+{
+for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
+{
+if ( bootinfo.reserved_mem.bank[i].type != MEMBANK_STATIC_HEAP )
+continue;
+
+bank_start = bootinfo.reserved_mem.bank[i].start;
+bank_size = bootinfo.reserved_mem.bank[i].size;
+bank_end = bank_start + bank_size;
+
+static_heap_size += bank_size;
+static_heap_end = max(static_heap_end, bank_end);
+}
+
+heap_pages = static_heap_size >> PAGE_SHIFT;
+}
+else
+heap_pages = total_pages;
+
+/*
+ * If the user has not requested otherwise via the command line
+ * then locate the xenheap using these constraints:
+ *
+ *  - must be contiguous
+ *  - must be 32 MiB aligned
+ *  - must not include Xen itself or the boot modules
+ *  - must be at most 1GB or 1/32 the 

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

[osstest service owner]

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

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  3fae7c56b313a12288a05e0a8c14c47bfd4dc40e
baseline version:
 xen  bf0bd6cf590a0a1b29845289159f0a17d5e4064f

Last test of basis   182392  2023-08-19 02:00:26 Z2 days
Testing same since   182412  2023-08-21 18:02:03 Z0 days1 attempts


People who touched revisions under test:
  Henry Wang 
  Julien Grall 
  Penny Zheng 
  Wei Chen 

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
   bf0bd6cf59..3fae7c56b3  3fae7c56b313a12288a05e0a8c14c47bfd4dc40e -> smoke

[linux-linus test] 182409: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-arm64-pvops 6 kernel-build   fail in 182408 REGR. vs. 182405

Tests which are failing intermittently (not blocking):
 test-amd64-amd64-xl-qemut-debianhvm-amd64 18 guest-localmigrate/x10 fail in 
182408 pass in 182409
 test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 21 guest-start.2 fail in 
182408 pass in 182409
 test-amd64-amd64-xl-qemuu-win7-amd64  8 xen-boot   fail pass in 182408
 test-amd64-amd64-dom0pvh-xl-intel 22 guest-start/debian.repeat fail pass in 
182408
 test-amd64-amd64-xl-qemut-win7-amd64 12 windows-installfail pass in 182408

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

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

[Julien Grall]

Hi,

On 21/08/2023 20:46, Vikram Garhwal wrote:

Hi Julien
On Fri, Aug 18, 2023 at 09:35:02PM +0100, Julien Grall wrote:

Hi Vikram,

On 18/08/2023 20:52, Vikram Garhwal wrote:

Hi Jan
On Thu, Aug 17, 2023 at 09:05:44AM +0200, Jan Beulich wrote:

On 17.08.2023 02:39, Vikram Garhwal wrote:

--- /dev/null
+++ b/xen/include/xen/iommu-private.h


I don't think private headers should live in include/xen/. Judging from only
the patches I was Cc-ed on, ...

Thank you for suggestion. Do you where can i place it then?
Please see another comment down regarding who might be using this function.



@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * xen/iommu-private.h
+ */
+#ifndef __XEN_IOMMU_PRIVATE_H__
+#define __XEN_IOMMU_PRIVATE_H__
+
+#ifdef CONFIG_HAS_DEVICE_TREE
+#include 
+
+/*
+ * Checks if dt_device_node is assigned to a domain or not. This function
+ * expects to be called with dtdevs_lock acquired by caller.
+ */
+bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev);
+#endif


... I don't even see the need for the declaration, as the function is used
only from the file also defining it. But of course if there is a use
elsewhere (in Arm-only code, as is suggested by the description here), then
the header (under a suitable name) wants to live under drivers/passthrough/
(and of course be included only from anywhere in that sub-tree).


This is also use in smmu.c:arm_smmu_dt_remove_device_legacy(). This is added in
12/19 patch(xen/smmu: Add remove_device callback for smmu_iommu ops).


AFAICT, the caller of this function (iommu_remove_dt_device()) will already
check if the device was assigned and bail out if that's the case.


So why do we need to check it again in the SMMU driver?


This was comment from you in v2:
"Even if the IOMMU subsystem check it, it would be good that the SMMU
driver also check the device is not currently used before removing it.

If it is, then we should return -EBUSY."
Link:https://patchew.org/Xen/1636441347-133850-1-git-send-email-fnu.vik...@xilinx.com/1636441347-133850-7-git-send-email-fnu.vik...@xilinx.com/


A lot of water flown under the bridge since that discussion. If you want 
to check the device is not attached in the SMMU driver, then you should 
use the internal SMMU state rather than the generic state.


You can look at arm_smmu_attach_dev() for an example how to do it.

Cheers,

--
Julien Grall

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

[Vikram Garhwal]

Hi Julien
On Fri, Aug 18, 2023 at 09:35:02PM +0100, Julien Grall wrote:
> Hi Vikram,
> 
> On 18/08/2023 20:52, Vikram Garhwal wrote:
> > Hi Jan
> > On Thu, Aug 17, 2023 at 09:05:44AM +0200, Jan Beulich wrote:
> > > On 17.08.2023 02:39, Vikram Garhwal wrote:
> > > > --- /dev/null
> > > > +++ b/xen/include/xen/iommu-private.h
> > > 
> > > I don't think private headers should live in include/xen/. Judging from 
> > > only
> > > the patches I was Cc-ed on, ...
> > Thank you for suggestion. Do you where can i place it then?
> > Please see another comment down regarding who might be using this function.
> > > 
> > > > @@ -0,0 +1,28 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > +/*
> > > > + * xen/iommu-private.h
> > > > + */
> > > > +#ifndef __XEN_IOMMU_PRIVATE_H__
> > > > +#define __XEN_IOMMU_PRIVATE_H__
> > > > +
> > > > +#ifdef CONFIG_HAS_DEVICE_TREE
> > > > +#include 
> > > > +
> > > > +/*
> > > > + * Checks if dt_device_node is assigned to a domain or not. This 
> > > > function
> > > > + * expects to be called with dtdevs_lock acquired by caller.
> > > > + */
> > > > +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node 
> > > > *dev);
> > > > +#endif
> > > 
> > > ... I don't even see the need for the declaration, as the function is used
> > > only from the file also defining it. But of course if there is a use
> > > elsewhere (in Arm-only code, as is suggested by the description here), 
> > > then
> > > the header (under a suitable name) wants to live under 
> > > drivers/passthrough/
> > > (and of course be included only from anywhere in that sub-tree).
> > > 
> > This is also use in smmu.c:arm_smmu_dt_remove_device_legacy(). This is 
> > added in
> > 12/19 patch(xen/smmu: Add remove_device callback for smmu_iommu ops).
> 
> AFAICT, the caller of this function (iommu_remove_dt_device()) will already
> check if the device was assigned and bail out if that's the case.
> 
> 
> So why do we need to check it again in the SMMU driver?
> 
This was comment from you in v2:
"Even if the IOMMU subsystem check it, it would be good that the SMMU 
driver also check the device is not currently used before removing it.

If it is, then we should return -EBUSY."
Link:https://patchew.org/Xen/1636441347-133850-1-git-send-email-fnu.vik...@xilinx.com/1636441347-133850-7-git-send-email-fnu.vik...@xilinx.com/

And there was similar comment from Michal in v5.

That's why this was kept.

Regards,
Vikram

> And if you really need to then you most likely want to check the internal
> state of the SMMU driver rather than the generic state.
> 
> Cheers,
> 
> -- 
> Julien Grall

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

[Vikram Garhwal]

Hi Jen,
On Mon, Aug 21, 2023 at 08:53:38AM +0200, Jan Beulich wrote:
> On 19.08.2023 02:28, Vikram Garhwal wrote:
> > Rename iommu_dt_device_is_assigned() to 
> > iommu_dt_device_is_assigned_locked().
> > Remove static type so this can also be used by SMMU drivers to check if the
> > device is being used before removing.
> > 
> > Moving spin_lock to caller was done to prevent the concurrent access to
> > iommu_dt_device_is_assigned while doing add/remove/assign/deassign. 
> > Follow-up
> > patches in this series introduces node add/remove feature.
> > 
> > Also, caller is required hold the correct lock so moved the function 
> > prototype
> > to a private header.
> > 
> > Signed-off-by: Vikram Garhwal 
> > 
> > ---
> > Changes from v7:
> > Update commit message.
> > Add ASSERT().
> > ---
> > ---
> >  xen/drivers/passthrough/device_tree.c | 26 +
> >  xen/include/xen/iommu-private.h   | 28 +++
> >  2 files changed, 50 insertions(+), 4 deletions(-)
> >  create mode 100644 xen/include/xen/iommu-private.h
> 
> I find it odd for new versions to be sent when discussion on the prior
> version hasn't settled yet, and when you then also don't incorporate
> the feedback given. I also find it odd to now be Cc-ed on the entire
> series. Imo instead of that patches which aren't self-explanatory /
> self-consistent simply need their descriptions improved (in the case
> here to mention what further use of a function is intended). Yet
> better would be to add declarations (and remove static) only at the
> point that's actually needed. This then eliminates the risk of
> subsequent changes in response to feedback (like Julien's here)
> resulting in the declaration no longer being needed, thus leading to
> a Misra violation (besides the general tidiness aspect).
> 
Reason behind sending new version: Patch 15/19 is largest patch in terms of 
change
and there was a long discussion with Julien and Stefano regarding a big change.
Last week(after v8) we agreed on change and Stefano and Julien were okay to send
v9 so it will be easier to review with that change.

Regarding cc-ing you to whole series, there was following comment on v8 09/1
"I don't think private headers should live in include/xen/. Judging from only
the patches I was Cc-ed on." I thought you wanted to see the whole full series.
Looks like i misunderstood the comment and Will remove the from cc-list in next
version.

The function itself will be needed in further patches in this series.
I am okay with keeping static and other things same here to avoid MISRA 
violation
and change wherever they are called first time.

Regarding Julien's comment i am reply back in same thread on why this was
not taken in account.
> Jan

Re: [XEN][PATCH v9 17/19] tools/libs/ctrl: Implement new xc interfaces for dt overlay

[Vikram Garhwal]

Hi Anthony,
On Mon, Aug 21, 2023 at 05:18:27PM +0100, Anthony PERARD wrote:
> On Fri, Aug 18, 2023 at 05:28:48PM -0700, Vikram Garhwal wrote:
> > xc_dt_overlay() sends the device tree binary overlay, size of .dtbo and 
> > overlay
> > operation type i.e. add or remove to xen.
> > 
> > Signed-off-by: Vikram Garhwal 
> 
> Hi Vikram,
> 
> I've given some comments on the v7 of this patch at [1], there's been no reply
> and the patch hasn't changed.
> 
> [1] 
> https://lore.kernel.org/xen-devel/9975f41c-c149-445a-8122-c15cfe5511b0@perard/
> 
> Did this fell through the cracks?
> 
Yes, you are right. It skipped through my update list. Sorry, for missing this.
Will change it in next version.

Thank you!
Vikram
> Cheers,
> 
> -- 
> Anthony PERARD

Re: [PATCH v5 09/13] xen/arm: mm: Use generic variable/function names for extendability

[Julien Grall]

Hi,

On 14/08/2023 05:25, Henry Wang wrote:

From: Penny Zheng 

As preparation for MPU support, which will use some variables/functions
for both MMU and MPU system, We rename the affected variable/function
to more generic names:
- init_ttbr -> init_mm,


You moved init_ttbr to mm/mmu.c. So why does this need to be renamed?

And if you really planned to use it for the MPU code. Then init_ttbr 
should not have been moved.



- mmu_init_secondary_cpu() -> mm_init_secondary_cpu()
- init_secondary_pagetables() -> init_secondary_mm()


The original naming were not great but the new one are a lot more 
confusing as they seem to just be a reshuffle of word.


mm_init_secondary_cpu() is only setting the WxN bit. For the MMU, I 
think it can be done much earlier. Do you have anything to add in it? If 
not, then I would consider to get rid of it.


For init_secondary_mm(), I would renamed it to prepare_secondary_mm().


- Add a wrapper update_mm_mapping() for MMU system's
   update_identity_mapping()

Modify the related in-code comment to reflect above changes, take the
opportunity to fix the incorrect coding style of the in-code comments.

Signed-off-by: Penny Zheng 
Signed-off-by: Henry Wang 
---
v5:
- Rebase on top of xen/arm: mm: add missing extern variable declaration
v4:
- Extract the renaming part from the original patch:
   "[v3,13/52] xen/mmu: extract mmu-specific codes from mm.c/mm.h"
---
  xen/arch/arm/arm32/head.S   |  4 ++--
  xen/arch/arm/arm64/mmu/head.S   |  2 +-
  xen/arch/arm/arm64/mmu/mm.c | 11 ---
  xen/arch/arm/arm64/smpboot.c|  6 +++---
  xen/arch/arm/include/asm/arm64/mm.h |  7 ---
  xen/arch/arm/include/asm/mm.h   | 12 +++-
  xen/arch/arm/mmu/mm.c   | 20 ++--
  xen/arch/arm/smpboot.c  |  4 ++--
  8 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 33b038e7e0..03ab68578a 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -238,11 +238,11 @@ GLOBAL(init_secondary)
  secondary_switched:
  /*
   * Non-boot CPUs need to move on to the proper pagetables, which were
- * setup in init_secondary_pagetables.
+ * setup in init_secondary_mm.
   *
   * XXX: This is not compliant with the Arm Arm.
   */
-mov_w r4, init_ttbr  /* VA of HTTBR value stashed by CPU 0 */
+mov_w r4, init_mm/* VA of HTTBR value stashed by CPU 0 */
  ldrd  r4, r5, [r4]   /* Actual value */
  dsb
  mcrr  CP64(r4, r5, HTTBR)
diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S
index ba2ddd7e67..58d91c9088 100644
--- a/xen/arch/arm/arm64/mmu/head.S
+++ b/xen/arch/arm/arm64/mmu/head.S
@@ -302,7 +302,7 @@ ENDPROC(enable_mmu)
  ENTRY(enable_secondary_cpu_mm)
  mov   x5, lr
  
-load_paddr x0, init_ttbr

+load_paddr x0, init_mm
  ldr   x0, [x0]
  
  blenable_mmu

diff --git a/xen/arch/arm/arm64/mmu/mm.c b/xen/arch/arm/arm64/mmu/mm.c
index 78b7c7eb00..ed0fc5ff7b 100644
--- a/xen/arch/arm/arm64/mmu/mm.c
+++ b/xen/arch/arm/arm64/mmu/mm.c
@@ -106,7 +106,7 @@ void __init arch_setup_page_tables(void)
  prepare_runtime_identity_mapping();
  }
  
-void update_identity_mapping(bool enable)

+static void update_identity_mapping(bool enable)


Why not simply renaming this function to update_mm_mapping()? But...


  {
  paddr_t id_addr = virt_to_maddr(_start);
  int rc;
@@ -120,6 +120,11 @@ void update_identity_mapping(bool enable)
  BUG_ON(rc);
  }
  
+void update_mm_mapping(bool enable)


... the new name it quite confusing. What is the mapping it is referring to?

If you don't want to keep update_identity_mapping(), then I would 
consider to simply wrap...



+{
+update_identity_mapping(enable);
+}
+
  extern void switch_ttbr_id(uint64_t ttbr);
  
  typedef void (switch_ttbr_fn)(uint64_t ttbr);

@@ -131,7 +136,7 @@ void __init switch_ttbr(uint64_t ttbr)
  lpae_t pte;
  
  /* Enable the identity mapping in the boot page tables */

-update_identity_mapping(true);
+update_mm_mapping(true);
  
  /* Enable the identity mapping in the runtime page tables */

  pte = pte_of_xenaddr((vaddr_t)switch_ttbr_id);
@@ -148,7 +153,7 @@ void __init switch_ttbr(uint64_t ttbr)
   * Note it is not necessary to disable it in the boot page tables
   * because they are not going to be used by this CPU anymore.
   */
-update_identity_mapping(false);
+update_mm_mapping(false);
  }
  
  /*

diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
index 9637f42469..2b1d086a1e 100644
--- a/xen/arch/arm/arm64/smpboot.c
+++ b/xen/arch/arm/arm64/smpboot.c
@@ -111,18 +111,18 @@ int arch_cpu_up(int cpu)
  if ( !smp_enable_ops[cpu].prepare_cpu )
  return -ENODEV;
  
-update_identity_mapping(true);

+

Re: [PATCH v5 08/13] xen/arm: Fold pmap and fixmap into MMU system

[Julien Grall]

Hi Henry,

On 14/08/2023 05:25, Henry Wang wrote:

From: Penny Zheng 

fixmap and pmap are MMU-specific features, so fold them to MMU system.
Do the folding for pmap by moving the HAS_PMAP Kconfig selection under
HAS_MMU. Do the folding for fixmap by moving the implementation of
virt_to_fix() to mmu/mm.c, so that unnecessary stubs can be avoided.

Signed-off-by: Penny Zheng 
Signed-off-by: Henry Wang 
---
v5:
- Rebase on top of xen/arm: Introduce CONFIG_MMU Kconfig option
v4:
- Rework "[v3,11/52] xen/arm: mmu: fold FIXMAP into MMU system",
   change the order of this patch and avoid introducing stubs.
---
  xen/arch/arm/Kconfig  | 2 +-
  xen/arch/arm/include/asm/fixmap.h | 7 +--
  xen/arch/arm/mmu/mm.c | 7 +++
  3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index eb0413336b..8a7b79b4b5 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -15,7 +15,6 @@ config ARM
select HAS_DEVICE_TREE
select HAS_PASSTHROUGH
select HAS_PDX
-   select HAS_PMAP
select HAS_UBSAN
select IOMMU_FORCE_PT_SHARE
  
@@ -61,6 +60,7 @@ config PADDR_BITS
  
  config MMU

def_bool y
+   select HAS_PMAP
  
  source "arch/Kconfig"
  
diff --git a/xen/arch/arm/include/asm/fixmap.h b/xen/arch/arm/include/asm/fixmap.h

index 734eb9b1d4..5d5de6995a 100644
--- a/xen/arch/arm/include/asm/fixmap.h
+++ b/xen/arch/arm/include/asm/fixmap.h
@@ -36,12 +36,7 @@ extern void clear_fixmap(unsigned int map);
  
  #define fix_to_virt(slot) ((void *)FIXMAP_ADDR(slot))
  
-static inline unsigned int virt_to_fix(vaddr_t vaddr)

-{
-BUG_ON(vaddr >= FIXADDR_TOP || vaddr < FIXADDR_START);
-
-return ((vaddr - FIXADDR_START) >> PAGE_SHIFT);
-}
+extern unsigned int virt_to_fix(vaddr_t vaddr);


AFAICT, virt_to_fix() is not going to be implemented for the MPU code. 
This implies that no-one should call it.


Also, none of the definitions in fixmap.h actually makes sense for the 
MPU. I would prefer if we instead try to lmit the include of fixmap to 
when this is strictly necessary. Looking for the inclusion in staging I 
could find:


42sh> ack "\#include" | ack "fixmap" | ack -v x86
arch/arm/acpi/lib.c:28:#include 
arch/arm/kernel.c:19:#include 
arch/arm/mm.c:27:#include 
arch/arm/include/asm/fixmap.h:7:#include 
arch/arm/include/asm/fixmap.h:8:#include 
arch/arm/include/asm/pmap.h:6:#include 
arch/arm/include/asm/early_printk.h:14:#include 
common/efi/boot.c:30:#include 
common/pmap.c:7:#include 
drivers/acpi/apei/erst.c:36:#include 
drivers/acpi/apei/apei-io.c:32:#include 
drivers/char/xhci-dbc.c:30:#include 
drivers/char/ehci-dbgp.c:16:#include 
drivers/char/ns16550.c:40:#include 
drivers/char/xen_pv_console.c:28:#include 

Some of them are gone after your rework. The only remaining that we care 
are in kernel.h (but I think it can be removed after your series).


So I think it would be feasible to not touch fixmap.h at all.

Cheers,

--
Julien Grall

Re: [PATCH v5 07/13] xen/arm: Extract MMU-specific code

[Julien Grall]

Hi Henry,

On 14/08/2023 05:25, Henry Wang wrote:

Currently, most of the MMU-specific code is in mm.{c,h}. To make the
mm extendable, this commit extract the MMU-specific code by firstly:
- Create a arch/arm/include/asm/mmu/ subdir.
- Create a arch/arm/mmu/ subdir.

Then move the MMU-specific code to above mmu subdir, which includes
below changes:
- Move arch/arm/arm64/mm.c to arch/arm/arm64/mmu/mm.c
- Move MMU-related declaration in arch/arm/include/asm/mm.h to
   arch/arm/include/asm/mmu/mm.h
- Move the MMU-related declaration dump_pt_walk() in asm/page.h
While I agree that dump_pt_walk() is better to be defined in mm.h, I am 
not entirely sure for ...



   and pte_of_xenaddr() in asm/setup.h to the new asm/mmu/mm.h.


... pte_of_xenaddr(). It was defined in setup.h because this is an 
helper that is only intended to be used during early boot.



That said, it is probably not worth creating a new helper for that. So I 
would suggest to at least mark pte_of_xenaddr() __init to make clear of 
the intended usage.



- Move MMU-related code in arch/arm/mm.c to arch/arm/mmu/mm.c.

Also modify the build system (Makefiles in this case) to pick above
mentioned code changes.

This patch is a pure code movement, no functional change intended.

Signed-off-by: Henry Wang 
---
With the code movement of this patch, the descriptions on top of
xen/arch/arm/mm.c and xen/arch/arm/mmu/mm.c might need some changes,
suggestions?
v5:
- Rebase on top of xen/arm: Introduce CONFIG_MMU Kconfig option and
   xen/arm: mm: add missing extern variable declaration
v4:
- Rework "[v3,13/52] xen/mmu: extract mmu-specific codes from
   mm.c/mm.h" with the lastest staging branch, only do the code movement
   in this patch to ease the review.
---
  xen/arch/arm/Makefile |1 +
  xen/arch/arm/arm64/Makefile   |1 -
  xen/arch/arm/arm64/mmu/Makefile   |1 +
  xen/arch/arm/arm64/{ => mmu}/mm.c |0
  xen/arch/arm/include/asm/mm.h |   20 +-
  xen/arch/arm/include/asm/mmu/mm.h |   55 ++
  xen/arch/arm/include/asm/page.h   |   15 -
  xen/arch/arm/include/asm/setup.h  |3 -
  xen/arch/arm/mm.c | 1119 
  xen/arch/arm/mmu/Makefile |1 +
  xen/arch/arm/mmu/mm.c | 1146 +


I noticed you transferred everything in mm.c But I think some part could 
go in arm{32,64}/mmu/mm.c.



  11 files changed, 1208 insertions(+), 1154 deletions(-)
  rename xen/arch/arm/arm64/{ => mmu}/mm.c (100%)
  create mode 100644 xen/arch/arm/include/asm/mmu/mm.h
  create mode 100644 xen/arch/arm/mmu/Makefile
  create mode 100644 xen/arch/arm/mmu/mm.c


(I haven't checked if the code was moved correctly. I only checked if 
the split makes sense).


To ease the review, I think this patch can be split in a more piecemeal 
patches. The first two pieces would be :

  * Patch #1 transfer xen_pt_update()/dump_pt_walk() and their dependencies
  * Patch #2 transfer root page-table allocation

Then you can have some for each small functions.

[...]


diff --git a/xen/arch/arm/arm64/mm.c b/xen/arch/arm/arm64/mmu/mm.c
similarity index 100%
rename from xen/arch/arm/arm64/mm.c
rename to xen/arch/arm/arm64/mmu/mm.c
diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index aaacba3f04..dc1458b047 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -14,6 +14,10 @@
  # error "unknown ARM variant"
  #endif
  
+#ifdef CONFIG_MMU

+#include 


I am guessing you will need to include  at some point. So 
I would add a:


#else
# error "Unknown memory management layout"

This would be easier to find out where includes might be missing.

[...]


@@ -1233,11 +119,6 @@ int map_pages_to_xen(unsigned long virt,
  return xen_pt_update(virt, mfn, nr_mfns, flags);
  }
  


[...]


+/* MMU setup for secondary CPUS (which already have paging enabled) */
+void mmu_init_secondary_cpu(void)
+{
+xen_pt_enforce_wnx();
+}
+
+#ifdef CONFIG_ARM_32


Rather than #ifdef, I would prefer if we move each implementation to 
arm32/mmu/mm.c and ...



+/*
+ * Set up the direct-mapped xenheap:
+ * up to 1GB of contiguous, always-mapped memory.
+ */
+void __init setup_directmap_mappings(unsigned long base_mfn,
+ unsigned long nr_mfns)
+{
+int rc;
+
+rc = map_pages_to_xen(XENHEAP_VIRT_START, _mfn(base_mfn), nr_mfns,
+  PAGE_HYPERVISOR_RW | _PAGE_BLOCK);
+if ( rc )
+panic("Unable to setup the directmap mappings.\n");
+
+/* Record where the directmap is, for translation routines. */
+directmap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
+}
+#else /* CONFIG_ARM_64 */
+/* Map the region in the directmap area. */
+void __init setup_directmap_mappings(unsigned long base_mfn,
+ unsigned long nr_mfns)


... arm64/mmu/mm.c.


+{
+int rc;
+
+/* First call sets the directmap physical and virtual offset. */
+if ( 

Re: [PATCH 0/3] xen/arm: Some clean-up found with -Wconversion and -Warith-conversion

[Julien Grall]

Hi,

On 17/08/2023 22:43, Julien Grall wrote:

From: Julien Grall 

Hi all,

This is a small series to fix some of the issues found while playing
with -Wconversion and -Warith-conversion.

There are a lot more but the bulk are in
  - bitmap
  - cpumask
  - nodemask
  - bitops/atomics
  - find_*

Some are not too difficult to address but other there are even
prototype conflicts between arm x86.

Cheers,

Julien Grall (3):
   xen/arm: vmmio: The number of entries cannot be negative
   xen/arm: vgic: Use 'unsigned int' rather than 'int' whenever it is
 possible
   xen/public: arch-arm: All PSR_* defines should be unsigned


I have committed the series.

Cheers,

--
Julien Grall

Re: [PATCH v5 03/13] xen/arm64: prepare for moving MMU related code from head.S

[Julien Grall]

Hi Henry,

On 21/08/2023 09:54, Henry Wang wrote:

On Aug 21, 2023, at 16:44, Julien Grall  wrote:
On 14/08/2023 05:25, Henry Wang wrote:

From: Wei Chen 
We want to reuse head.S for MPU systems, but there are some
code are implemented for MMU systems only. We will move such
code to another MMU specific file. But before that we will
do some indentations fix in this patch to make them be easier
for reviewing:
1. Fix the indentations and incorrect style of code comments.
2. Fix the indentations for .text.header section.
3. Rename puts() to asm_puts() for global export
Signed-off-by: Wei Chen 
Signed-off-by: Penny Zheng 
Signed-off-by: Henry Wang 
Reviewed-by: Ayan Kumar Halder 
Reviewed-by: Julien Grall 


Is this patch depends on the first two? If not, I will commit it before v6.


Good point, no this patch is independent from the first two. Also I just
tested applying this patch on top of staging and building with and without
Earlyprintk. Xen and Dom0 boot fine on FVP for both cases.


Thanks for confirming. It is now ...



So please commit this patch if you have time. Thanks!


... committed.

Cheers,

--
Julien Grall

Re: [PATCH 2/3] xen/arm: vgic: Use 'unsigned int' rather than 'int' whenever it is possible

[Julien Grall]

Hi Michal,

On 18/08/2023 08:02, Michal Orzel wrote:

On 17/08/2023 23:43, Julien Grall wrote:



From: Julien Grall 

Switch to unsigned int for the return/parameters of the following
functions:
 * REG_RANK_NR(): 'b' (number of bits) and the return is always positive.
   'n' doesn't need to be size specific.
 * vgic_rank_offset(): 'b' (number of bits), 'n' (register index),
   's' (size of the access) are always positive.
 * vgic_{enable, disable}_irqs(): 'n' (rank index) is always positive
 * vgic_get_virq_type(): 'n' (rank index) and 'index' (register
   index) are always positive.

It looks like you forgot to mention the modification done to 'vgic_get_rank()'


I have added:

* vgic_get_rank(): 'rank' is an index and therefore always 
positive.



With this addressed:
Reviewed-by: Michal Orzel 


Thanks!

Cheers,

--
Julien Grall

Re: [PATCH 2/3] xen/arm: vgic: Use 'unsigned int' rather than 'int' whenever it is possible

[Julien Grall]

Hi Stefano,

On 18/08/2023 00:04, Stefano Stabellini wrote:

On Thu, 17 Aug 2023, Julien Grall wrote:

From: Julien Grall 

Switch to unsigned int for the return/parameters of the following
functions:
 * REG_RANK_NR(): 'b' (number of bits) and the return is always positive.
   'n' doesn't need to be size specific.
 * vgic_rank_offset(): 'b' (number of bits), 'n' (register index),
   's' (size of the access) are always positive.
 * vgic_{enable, disable}_irqs(): 'n' (rank index) is always positive
 * vgic_get_virq_type(): 'n' (rank index) and 'index' (register
   index) are always positive.

Take the opportunity to propogate the unsignedness to the local
variable used for the arguments.

This will remove some of the warning reported by GCC 12.2.1 when
passing the flags -Wsign-conversion/-Wconversion.

Signed-off-by: Julien Grall 
---
  xen/arch/arm/include/asm/vgic.h | 11 +++
  xen/arch/arm/vgic-v2.c  | 12 ++--
  xen/arch/arm/vgic.c | 21 -
  3 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
index 6901a05c0669..922779ce146a 100644
--- a/xen/arch/arm/include/asm/vgic.h
+++ b/xen/arch/arm/include/asm/vgic.h
@@ -252,7 +252,7 @@ struct vgic_ops {
   * Rank containing GICD_ for GICD_ with
   * -bits-per-interrupt
   */
-static inline int REG_RANK_NR(int b, uint32_t n)
+static inline unsigned int REG_RANK_NR(unsigned int b, unsigned int n)
  {
  switch ( b )
  {
@@ -297,10 +297,13 @@ extern void gic_remove_from_lr_pending(struct vcpu *v, 
struct pending_irq *p);
  extern void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq);
  extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
  extern struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq);
-extern struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n, 
int s);
+extern struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v,
+  unsigned int b,
+  unsigned int n,
+  unsigned int s);
  extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq);
-extern void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n);
-extern void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n);
+extern void vgic_disable_irqs(struct vcpu *v, uint32_t r, unsigned int n);
+extern void vgic_enable_irqs(struct vcpu *v, uint32_t r, unsigned int n);
  extern void vgic_set_irqs_pending(struct vcpu *v, uint32_t r,
unsigned int rank);
  extern void register_vgic_ops(struct domain *d, const struct vgic_ops *ops);
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 2a2eda2e6f4c..0aa10fff0f10 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -161,7 +161,11 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, 
mmio_info_t *info,
  {
  struct hsr_dabt dabt = info->dabt;
  struct vgic_irq_rank *rank;
-int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
+/*
+ * gpa/dbase are paddr_t which size may be higher than 32-bit. Yet
+ * the difference will always be smaller than 32-bit.
+ */
+unsigned int gicd_reg = info->gpa - v->domain->arch.vgic.dbase;
  unsigned long flags;
  
  perfc_incr(vgicd_reads);

@@ -403,7 +407,11 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, 
mmio_info_t *info,
  {
  struct hsr_dabt dabt = info->dabt;
  struct vgic_irq_rank *rank;
-int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
+/*
+ * gpa/dbase are paddr_t which size may be higher than 32-bit. Yet
+ * the difference will always be smaller than 32-bit.
+ */
+unsigned int gicd_reg = info->gpa - v->domain->arch.vgic.dbase;
  uint32_t tr;
  unsigned long flags;
  
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c

index afcac791fe4b..269b804974e0 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -24,7 +24,8 @@
  #include 
  #include 
  
-static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v, int rank)

+static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
+  unsigned int rank)
  {
  if ( rank == 0 )
  return v->arch.vgic.private_irqs;
@@ -38,17 +39,17 @@ static inline struct vgic_irq_rank *vgic_get_rank(struct 
vcpu *v, int rank)
   * Returns rank corresponding to a GICD_ register for
   * GICD_ with -bits-per-interrupt.
   */
-struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n,
-  int s)
+struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, unsigned int b,
+   unsigned int n, unsigned int s)
  {
-int rank = REG_RANK_NR(b, (n >> s));
+unsigned int rank = 

[RFC PATCH 0/3] docs/misra: add documentation to address MISRA C:2012 Dir 4.1

[Nicola Vetrini]

The headline of Directive 4.1 states: "Run-time failures shall be minimized".
Thus, it requires the project to supply documentation that pertains the measures
and techinques used to prevent run-time failures from happening. For ease of
reading, the documentation is in RST format, but since ECLAIR needs a source 
file
to check that the needed subsections and their format is the one expected, the
Makefiles for the docs/ are amended to generate such a file.

Moreover, the format and categories of the subsections in the .rst file can be
customized based on feedback from the community: the one provided is just a
basic skeleton that should be tailored to the project.

Nicola Vetrini (3):
  docs/misra: add documentation for MISRA C:2012 Dir 4.1
  docs: make the docs for MISRA C:2012 Dir 4.1 visible to ECLAIR
  automation/eclair: build docs/misra to address MISRA C:2012 Dir 4.1

 automation/eclair_analysis/build.sh   |  10 +-
 automation/eclair_analysis/prepare.sh |   1 +
 docs/Makefile |   7 +-
 docs/misra/C-runtime-failures.rst | 239 ++
 docs/misra/Makefile   |  36 
 docs/misra/rules.rst  |   7 +-
 6 files changed, 296 insertions(+), 4 deletions(-)
 create mode 100644 docs/misra/C-runtime-failures.rst
 create mode 100644 docs/misra/Makefile

--
2.34.1

[RFC PATCH 3/3] automation/eclair: build docs/misra to address MISRA C:2012 Dir 4.1

[Nicola Vetrini]

The documentation pertaining Directive 4.1 is contained in docs/misra.
The build script driving the analysis is amended to allow ECLAIR to
find it and thus resolving violations of the directive.

Signed-off-by: Nicola Vetrini 
---
 automation/eclair_analysis/build.sh   | 10 --
 automation/eclair_analysis/prepare.sh |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/automation/eclair_analysis/build.sh 
b/automation/eclair_analysis/build.sh
index ec087dd822fa..a0433eedeb4d 100755
--- a/automation/eclair_analysis/build.sh
+++ b/automation/eclair_analysis/build.sh
@@ -34,8 +34,14 @@ else
 fi
 
 (
-  cd xen
-
+  cd docs
+  make "-j${PROCESSORS}" "-l${PROCESSORS}.0"\
+   "CROSS_COMPILE=${CROSS_COMPILE}" \
+   "CC=${CROSS_COMPILE}gcc-12"  \
+   "CXX=${CROSS_COMPILE}g++-12" \
+   "XEN_TARGET_ARCH=${XEN_TARGET_ARCH}" \
+   misra
+  cd ../xen
   make "-j${PROCESSORS}" "-l${PROCESSORS}.0"\
"CROSS_COMPILE=${CROSS_COMPILE}" \
"CC=${CROSS_COMPILE}gcc-12"  \
diff --git a/automation/eclair_analysis/prepare.sh 
b/automation/eclair_analysis/prepare.sh
index 275a1a3f517c..10854741790e 100755
--- a/automation/eclair_analysis/prepare.sh
+++ b/automation/eclair_analysis/prepare.sh
@@ -35,6 +35,7 @@ else
 fi
 
 (
+./configure
 cd xen
 cp "${CONFIG_FILE}" .config
 make clean
-- 
2.34.1

[RFC PATCH 2/3] docs: make the docs for MISRA C:2012 Dir 4.1 visible to ECLAIR

[Nicola Vetrini]

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 
---
 docs/Makefile   |  7 ++-
 docs/misra/Makefile | 36 
 2 files changed, 42 insertions(+), 1 deletion(-)
 create mode 100644 docs/misra/Makefile

diff --git a/docs/Makefile b/docs/Makefile
index 966a104490ac..ff991a0c3ca2 100644
--- a/docs/Makefile
+++ b/docs/Makefile
@@ -43,7 +43,7 @@ DOC_PDF  := $(patsubst %.pandoc,pdf/%.pdf,$(PANDOCSRC-y)) \
 all: build
 
 .PHONY: build
-build: html txt pdf man-pages figs
+build: html txt pdf man-pages figs misra
 
 .PHONY: sphinx-html
 sphinx-html:
@@ -66,9 +66,14 @@ endif
 .PHONY: pdf
 pdf: $(DOC_PDF)
 
+.PHONY: misra
+misra:
+   $(MAKE) -C misra
+
 .PHONY: clean
 clean: clean-man-pages
$(MAKE) -C figs clean
+   $(MAKE) -C misra clean
rm -rf .word_count *.aux *.dvi *.bbl *.blg *.glo *.idx *~
rm -rf *.ilg *.log *.ind *.toc *.bak *.tmp core
rm -rf html txt pdf sphinx/html
diff --git a/docs/misra/Makefile b/docs/misra/Makefile
new file mode 100644
index ..f62cd936bfcc
--- /dev/null
+++ b/docs/misra/Makefile
@@ -0,0 +1,36 @@
+XEN_ROOT=$(CURDIR)/../..
+include $(XEN_ROOT)/Config.mk
+-include $(XEN_ROOT)/config/Docs.mk
+
+
+TARGETS := $(addprefix C-runtime-failures,.c .o)
+
+all: $(TARGETS)
+
+define MISRA_HEADER
+/*
+
+endef
+
+define MISRA_FOOTER
+
+*/
+
+endef
+export MISRA_HEADER
+export MISRA_FOOTER
+
+C-runtime-failures.c: C-runtime-failures.rst
+# sed is used in place of cat to prevent occurrences of '*/'
+# in the .rst from breaking the compilation
+   ( \
+ echo "$${MISRA_HEADER}"; \
+ sed -e 's|*/|*//*|' $<; \
+ echo "$${MISRA_FOOTER}" \
+   ) > $@
+
+%.o: %.c
+   $(CC) -c $< -o $@
+
+clean:
+   rm -f *.c *.o
-- 
2.34.1

[RFC PATCH 1/3] docs/misra: add documentation for MISRA C:2012 Dir 4.1

[Nicola Vetrini]

The aforementioned directive requires the project to supply documentation
on the measures taken towards the minimization of run-time failures.

The 'rules.rst' file is updated accordingly to mention the newly
added documentation.

Signed-off-by: Nicola Vetrini 
---
 docs/misra/C-runtime-failures.rst | 239 ++
 docs/misra/rules.rst  |   7 +-
 2 files changed, 245 insertions(+), 1 deletion(-)
 create mode 100644 docs/misra/C-runtime-failures.rst

diff --git a/docs/misra/C-runtime-failures.rst 
b/docs/misra/C-runtime-failures.rst
new file mode 100644
index ..f72385b08417
--- /dev/null
+++ b/docs/misra/C-runtime-failures.rst
@@ -0,0 +1,239 @@
+===
+Measures taken towards the minimization of Run-time failures in Xen
+===
+
+This document specifies which procedures and techinques are used troughout the
+Xen codebase to prevent or minimize the impact of certain classes of run-time
+errors that can occurr in the execution of a C program, due to the very minimal
+built-in checks that are present in the language.
+
+The presence of such documentation is requested by MISRA C:2012 Directive 4.1,
+whose headline states: "Run-time failures shall be minimized".
+
+
+Documentation for MISRA C:2012 Dir 4.1: overflow
+
+
+To be written.
+Example: Pervasive use of assertions and extensive test suite.
+
+
+Documentation for MISRA C:2012 Dir 4.1: unexpected wrapping
+___
+
+To be written.
+Example: The only wrapping the is present in the code concerns
+unsigned integers and they are all expected.
+
+
+Documentation for MISRA C:2012 Dir 4.1: invalid shift
+_
+
+To be written.
+Example: Pervasive use of assertions and extensive test suite.
+
+
+Documentation for MISRA C:2012 Dir 4.1: division/remainder by zero
+__
+
+To be written.
+Example:
+There division or remainder operations in the project code ensure that
+their second argument is never zero.
+
+
+Documentation for MISRA C:2012 Dir 4.1: unsequenced side effects
+
+
+To be written.
+Example:
+No function in this project is meant to be executed from interrupt handlers
+or in multi-threading environments.
+
+
+Documentation for MISRA C:2012 Dir 4.1: read from uninitialized automatic 
object
+
+
+To be written.
+Example:
+Automatic variables are used to store temporary parameters and they
+are always initialized to either a default value or a proper value
+before usage.
+
+
+Documentation for MISRA C:2012 Dir 4.1: read from uninitialized allocated 
object
+
+
+To be written.
+Example:
+The code does not use dynamically allocated storage.
+
+
+Documentation for MISRA C:2012 Dir 4.1: write to string literal or const object
+___
+
+To be written.
+Example:
+The toolchain puts every string literal and const object into a read-only
+section of memory.  The hardware exception raised when a write is attempted
+on such a memory section is correctly handled.
+
+
+Documentation for MISRA C:2012 Dir 4.1: non-volatile access to volatile object
+__
+
+To be written.
+Example:
+Volatile access is limited to registers that are always accessed
+through macros or inline functions.
+
+
+Documentation for MISRA C:2012 Dir 4.1: access to dead allocated object
+___
+
+To be written.
+Example:
+The code does not use dynamically allocated storage.
+
+
+Documentation for MISRA C:2012 Dir 4.1: access to dead automatic object
+___
+
+To be written.
+Example:
+Pointers to automatic variables are never returned, nor stored in
+wider-scoped objects.  No function does the same on any pointer
+received as a parameter.
+
+
+Documentation for MISRA C:2012 Dir 4.1: access to dead thread object
+
+
+To be written.
+Example:
+The program does not use per-thread variables.
+
+
+Documentation for MISRA C:2012 Dir 4.1: access using null pointer
+_
+
+To be written.
+Example:
+All possibly null pointers are checked before access.
+
+
+Documentation for MISRA C:2012 Dir 4.1: access using invalid pointer
+
+
+To be written.

Re: [PATCH v1 44/57] xen/riscv: introduce asm/vm_event.h

[Tamas K Lengyel]

On Wed, Aug 16, 2023 at 12:30 PM Oleksii Kurochko
 wrote:
>
> Signed-off-by: Oleksii Kurochko 
> ---
>  xen/arch/riscv/include/asm/vm_event.h | 52 +++
>  1 file changed, 52 insertions(+)
>  create mode 100644 xen/arch/riscv/include/asm/vm_event.h

I don't think we ought to replicate this header for every arch when
clearly the functions in them are only relevant for specific
architectures. Would make more sense to me to just conditionalize the
caller side to the specific archs where these functions are actually
defined, which would largely just be CONFIG_X86.

Tamas

Re: [XEN][PATCH v9 17/19] tools/libs/ctrl: Implement new xc interfaces for dt overlay

[Anthony PERARD]

On Fri, Aug 18, 2023 at 05:28:48PM -0700, Vikram Garhwal wrote:
> xc_dt_overlay() sends the device tree binary overlay, size of .dtbo and 
> overlay
> operation type i.e. add or remove to xen.
> 
> Signed-off-by: Vikram Garhwal 

Hi Vikram,

I've given some comments on the v7 of this patch at [1], there's been no reply
and the patch hasn't changed.

[1] 
https://lore.kernel.org/xen-devel/9975f41c-c149-445a-8122-c15cfe5511b0@perard/

Did this fell through the cracks?

Cheers,

-- 
Anthony PERARD

Re: [PATCH 1/5] x86: Fix calculation of %dr6/7 reserved bits

[Jinoh Kang]

On 8/22/23 00:56, Jinoh Kang wrote:
> From: Andrew Cooper 
> 
> The reserved bit calculations for %dr6 and %dr7 depend on whether the VM has
> the Restricted Transnational Memory feature available.

s/Transnational/Transactional/.

It was in the original review, but I missed the change.  Apologies.

-- 
Sincerely,
Jinoh Kang

[PATCH 2/5] x86/emul: Add pending_dbg field to x86_event

[Jinoh Kang]

From: Andrew Cooper 

All #DB exceptions result in an update of %dr6, but this isn't captured in
Xen's handling.

PV guests generally work by modifying %dr6 before raising #DB, whereas HVM
guests do nothing and have a single-step special case in the lowest levels of
{vmx,svm}_inject_event().  All of this is buggy, but in particular, task
switches with the trace flag never end up signalling BT in %dr6.

To begin resolving this issue, add a new pending_dbg field to x86_event
(unioned with cr2 to avoid taking any extra space), and introduce
{pv,hvm}_inject_debug_exn() helpers to replace the current callers using
{pv,hvm}_inject_hw_exception().

A key property is that pending_dbg is taken with positive polarity to deal
with RTM sensibly.  Most callers pass in a constant, but callers passing in a
hardware %dr6 value need to xor the value with X86_DR6_DEFAULT to flip the
polarity of RTM and reserved fields.

For PV guests, move the ad-hoc updating of %dr6 into pv_inject_event().  This
in principle breaks the handing of RTM in do_debug(), but PV guests can't
actually enable MSR_DEBUGCTL.RTM yet, so this doesn't matter in practice.

Signed-off-by: Jinoh Kang 
---
CC: Jan Beulich 
CC: Wei Liu 
CC: Roger Pau Monné 
CC: Jun Nakajima 
CC: Kevin Tian 
CC: Tim Deegan 
---
 xen/arch/x86/hvm/emulate.c |  3 ++-
 xen/arch/x86/hvm/hvm.c |  2 +-
 xen/arch/x86/hvm/svm/svm.c |  9 ++---
 xen/arch/x86/hvm/vmx/vmx.c | 13 -
 xen/arch/x86/include/asm/debugreg.h|  3 +++
 xen/arch/x86/include/asm/domain.h  | 12 
 xen/arch/x86/include/asm/hvm/hvm.h | 15 ++-
 xen/arch/x86/mm/shadow/multi.c |  5 +++--
 xen/arch/x86/pv/emul-priv-op.c | 11 +--
 xen/arch/x86/pv/emulate.c  |  6 ++
 xen/arch/x86/pv/ro-page-fault.c|  3 ++-
 xen/arch/x86/pv/traps.c| 16 
 xen/arch/x86/traps.c   |  6 +-
 xen/arch/x86/x86_emulate/x86_emulate.h |  5 -
 14 files changed, 75 insertions(+), 34 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 9b6e4c8bc6..129403ad90 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -16,6 +16,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -2673,7 +2674,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt 
*hvmemul_ctxt,
 }
 
 if ( hvmemul_ctxt->ctxt.retire.singlestep )
-hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
+hvm_inject_debug_exn(X86_DR6_BS);
 
 new_intr_shadow = hvmemul_ctxt->intr_shadow;
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 66ead0b878..c3d46841c2 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3229,7 +3229,7 @@ void hvm_task_switch(
 }
 
 if ( (tss.trace & 1) && !exn_raised )
-hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
+hvm_inject_debug_exn(X86_DR6_BT);
 
  out:
 hvm_unmap_entry(optss_desc);
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 01dd592d9b..ac3123c56f 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -96,7 +96,7 @@ void __update_guest_eip(struct cpu_user_regs *regs, unsigned 
int inst_len)
 curr->arch.hvm.svm.vmcb->int_stat.intr_shadow = 0;
 
 if ( regs->eflags & X86_EFLAGS_TF )
-hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
+hvm_inject_debug_exn(X86_DR6_BS);
 }
 
 static void cf_check svm_cpu_down(void)
@@ -2755,7 +2755,10 @@ void svm_vmexit_handler(void)
 goto unexpected_exit_type;
 if ( !rc )
 hvm_inject_exception(X86_EXC_DB,
- trap_type, insn_len, X86_EVENT_NO_EC);
+ trap_type, insn_len, X86_EVENT_NO_EC,
+ exit_reason == VMEXIT_ICEBP ? 0 :
+ /* #DB - Hardware already updated dr6. */
+ vmcb_get_dr6(vmcb) ^ X86_DR6_DEFAULT);
 }
 else
 domain_pause_for_debugger();
@@ -2785,7 +2788,7 @@ void svm_vmexit_handler(void)
if ( !rc )
hvm_inject_exception(X86_EXC_BP,
 X86_EVENTTYPE_SW_EXCEPTION,
-insn_len, X86_EVENT_NO_EC);
+insn_len, X86_EVENT_NO_EC, 0 /* N/A */);
 }
 break;
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 7ec44018d4..716115332b 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3068,7 +3068,7 @@ void update_guest_eip(void)
 }
 
 if ( regs->eflags & X86_EFLAGS_TF )
-hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
+hvm_inject_debug_exn(X86_DR6_BS);
 }
 
 static void cf_check vmx_fpu_dirty_intercept(void)
@@ 

[PATCH 4/5] x86: Fix merging of new status bits into %dr6

[Jinoh Kang]

From: Andrew Cooper 

The current logic used to update %dr6 when injecting #DB is buggy.  The
architectural behaviour is to overwrite B{0..3} (rather than accumulate) and
accumulate all other bits.

Introduce a new merge_dr6() helper, which also takes care of handing RTM
correctly.

Signed-off-by: Jinoh Kang 
---
CC: Jan Beulich 
CC: Wei Liu 
CC: Roger Pau Monné 
CC: Jun Nakajima 
CC: Kevin Tian 
---
 xen/arch/x86/hvm/svm/svm.c   |  3 ++-
 xen/arch/x86/hvm/vmx/vmx.c   |  3 ++-
 xen/arch/x86/include/asm/debugreg.h  | 30 +++-
 xen/arch/x86/include/asm/x86-defns.h | 10 --
 xen/arch/x86/pv/traps.c  |  3 ++-
 5 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index bd3adde0ec..7a5652f3df 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1338,7 +1338,8 @@ static void cf_check svm_inject_event(const struct 
x86_event *event)
  * Item 2 is done by hardware when injecting a #DB exception.
  */
 __restore_debug_registers(vmcb, curr);
-vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | _event.pending_dbg);
+vmcb_set_dr6(vmcb, merge_dr6(vmcb_get_dr6(vmcb), _event.pending_dbg,
+ curr->domain->arch.cpuid->feat.rtm));
 
 /* fall through */
 case X86_EXC_BP:
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 65166348f1..1fd902ed74 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2031,7 +2031,8 @@ static void cf_check vmx_inject_event(const struct 
x86_event *event)
  * All actions are left up to the hypervisor to perform.
  */
 __restore_debug_registers(curr);
-write_debugreg(6, read_debugreg(6) | event->pending_dbg);
+write_debugreg(6, merge_dr6(read_debugreg(6), event->pending_dbg,
+curr->domain->arch.cpuid->feat.rtm));
 
 if ( !nestedhvm_vcpu_in_guestmode(curr) ||
  !nvmx_intercepts_exception(curr, X86_EXC_DB, _event.error_code) )
diff --git a/xen/arch/x86/include/asm/debugreg.h 
b/xen/arch/x86/include/asm/debugreg.h
index a1df74c02e..fd32846397 100644
--- a/xen/arch/x86/include/asm/debugreg.h
+++ b/xen/arch/x86/include/asm/debugreg.h
@@ -23,6 +23,14 @@
 #define X86_DR6_BT  (1u << 15)  /* Task switch */
 #define X86_DR6_RTM (1u << 16)  /* #DB/#BP in RTM region   */
 
+#define X86_DR6_BP_MASK \
+(X86_DR6_B0 | X86_DR6_B1 | X86_DR6_B2 | X86_DR6_B3)
+
+#define X86_DR6_KNOWN_MASK  \
+(X86_DR6_BP_MASK | X86_DR6_BD | X86_DR6_BS | X86_DR6_BT | X86_DR6_RTM)
+
+#define X86_DR6_DEFAULT 0x0ff0  /* Default %dr6 value. */
+
 #define DR_TRAP0(0x1)   /* db0 */
 #define DR_TRAP1(0x2)   /* db1 */
 #define DR_TRAP2(0x4)   /* db2 */
@@ -30,7 +38,6 @@
 #define DR_STEP (0x4000)/* single-step */
 #define DR_SWITCH   (0x8000)/* task switch */
 #define DR_NOT_RTM  (0x1)   /* clear: #BP inside RTM region */
-#define DR_STATUS_RESERVED_ONE  0x0ff0UL /* Reserved, read as one */
 
 /* Now define a bunch of things for manipulating the control register.
The top two bytes of the control register consist of 4 fields of 4
@@ -74,6 +81,8 @@
 #define DR_RTM_ENABLE(0x0800UL) /* RTM debugging enable */
 #define DR_GENERAL_DETECT(0x2000UL) /* General detect enable */
 
+#define X86_DR7_DEFAULT 0x0400  /* Default %dr7 value. */
+
 #define write_debugreg(reg, val) do {   \
 unsigned long __val = val;  \
 asm volatile ( "mov %0,%%db" #reg : : "r" (__val) );\
@@ -102,6 +111,25 @@ static inline unsigned long adjust_dr6_rsvd(unsigned long 
dr6, bool rtm)
 return dr6;
 }
 
+static inline unsigned long merge_dr6(unsigned long dr6, unsigned long new,
+  bool rtm)
+{
+/* Flip dr6 to have positive polarity. */
+dr6 ^= X86_DR6_DEFAULT;
+
+/* Sanity check that only known values are passed in. */
+ASSERT(!(dr6 & ~X86_DR6_KNOWN_MASK));
+ASSERT(!(new & ~X86_DR6_KNOWN_MASK));
+
+/* Breakpoints 0-3 overridden.  BD, BS, BT and RTM accumulate. */
+dr6 = (dr6 & ~X86_DR6_BP_MASK) | new;
+
+/* Flip dr6 back to having default polarity. */
+dr6 ^= X86_DR6_DEFAULT;
+
+return adjust_dr6_rsvd(dr6, rtm);
+}
+
 static inline unsigned long adjust_dr7_rsvd(unsigned long dr7, bool rtm)
 {
 /*
diff --git a/xen/arch/x86/include/asm/x86-defns.h 
b/xen/arch/x86/include/asm/x86-defns.h
index e350227e57..7fbc74850a 100644
--- a/xen/arch/x86/include/asm/x86-defns.h
+++ b/xen/arch/x86/include/asm/x86-defns.h
@@ -100,16 +100,6 @@
 #define X86_XCR0_LWP_POS  62
 #define X86_XCR0_LWP  (1ULL << 

[PATCH 5/5] x86/dbg: Cleanup of legacy dr6 constants

[Jinoh Kang]

From: Andrew Cooper 

Replace the few remaining uses with X86_DR6_* constants.

Signed-off-by: Jinoh Kang 
---
CC: Jan Beulich 
CC: Wei Liu 
CC: Roger Pau Monné 
---
 xen/arch/x86/hvm/vmx/vmx.c  |  2 +-
 xen/arch/x86/include/asm/debugreg.h | 17 -
 xen/arch/x86/pv/emul-priv-op.c  |  2 +-
 xen/arch/x86/traps.c|  2 +-
 4 files changed, 3 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 1fd902ed74..43b57a444e 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4306,7 +4306,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
 
 __vmread(GUEST_PENDING_DBG_EXCEPTIONS, _dbg);
 __vmwrite(GUEST_PENDING_DBG_EXCEPTIONS,
-  pending_dbg | DR_STEP);
+  pending_dbg | X86_DR6_BS);
 }
 }
 
diff --git a/xen/arch/x86/include/asm/debugreg.h 
b/xen/arch/x86/include/asm/debugreg.h
index fd32846397..1061fdaaae 100644
--- a/xen/arch/x86/include/asm/debugreg.h
+++ b/xen/arch/x86/include/asm/debugreg.h
@@ -1,15 +1,6 @@
 #ifndef _X86_DEBUGREG_H
 #define _X86_DEBUGREG_H
 
-
-/* Indicate the register numbers for a number of the specific
-   debug registers.  Registers 0-3 contain the addresses we wish to trap on */
-
-#define DR_FIRSTADDR 0
-#define DR_LASTADDR  3
-#define DR_STATUS6
-#define DR_CONTROL   7
-
 /*
  * DR6 status bits.
  *   N.B. For backwards compatibility, X86_DR6_RTM has inverted polarity.
@@ -31,14 +22,6 @@
 
 #define X86_DR6_DEFAULT 0x0ff0  /* Default %dr6 value. */
 
-#define DR_TRAP0(0x1)   /* db0 */
-#define DR_TRAP1(0x2)   /* db1 */
-#define DR_TRAP2(0x4)   /* db2 */
-#define DR_TRAP3(0x8)   /* db3 */
-#define DR_STEP (0x4000)/* single-step */
-#define DR_SWITCH   (0x8000)/* task switch */
-#define DR_NOT_RTM  (0x1)   /* clear: #BP inside RTM region */
-
 /* Now define a bunch of things for manipulating the control register.
The top two bytes of the control register consist of 4 fields of 4
bits - each field corresponds to one of the four debug registers,
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 72d0514e74..78a1f4aff7 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -1359,7 +1359,7 @@ int pv_emulate_privileged_op(struct cpu_user_regs *regs)
 {
 case X86EMUL_OKAY:
 if ( ctxt.ctxt.retire.singlestep )
-ctxt.bpmatch |= DR_STEP;
+ctxt.bpmatch |= X86_DR6_BS;
 
 if ( ctxt.bpmatch &&
  !(curr->arch.pv.trap_bounce.flags & TBF_EXCEPTION) )
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 640b60e0e5..829741ff78 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1955,7 +1955,7 @@ void do_debug(struct cpu_user_regs *regs)
  * If however we do, safety measures need to be enacted.  Use a big
  * hammer and clear all debug settings.
  */
-if ( dr6 & (DR_TRAP3 | DR_TRAP2 | DR_TRAP1 | DR_TRAP0) )
+if ( dr6 & X86_DR6_BP_MASK )
 {
 unsigned int bp, dr7 = read_debugreg(7);
 
-- 
2.41.0

[PATCH 3/5] x86/hvm: RFC - PROBABLY BROKEN - Defer all debugging/monitor actions to {svm,vmx}_inject_event()

[Jinoh Kang]

From: Andrew Cooper 

Currently, there is a lot of functionality in the #DB intercepts, and some
repeated functionality in the *_inject_event() logic.

The gdbsx code is implemented at both levels (albeit differently for #BP,
which is presumably due to the fact that the old emulator behaviour used to be
to move %rip forwards for traps), while the monitor behaviour only exists at
the intercept level.

Updating of %dr6 is implemented (buggily) at both levels, but having it at
both levels is problematic to implement correctly.

Rearrange the logic to have nothing interesting at the intercept level, and
everything implemented at the injection level.  Amongst other things, this
means that the monitor subsystem will pick up debug actions from emulated
events.

Signed-off-by: Jinoh Kang 
---
CC: Jan Beulich 
CC: Wei Liu 
CC: Roger Pau Monné 
CC: Jun Nakajima 
CC: Kevin Tian 
CC: Razvan Cojocaru 
CC: Tamas K Lengyel 

This is RFC because it probably breaks introspection, as injection replies
from the introspection engine will (probably, but I haven't confirmed) trigger
new monitor events.

First of all, monitoring emulated debug events is a change in behaviour,
although IMO it is more of a bugfix than a new feature.  Also, similar changes
will happen to other monitored events as we try to unify the
intercept/emulation paths.

As for the recursive triggering of monitor events, I was considering extending
the monitor infrastructure to have a "in monitor reply" boolean which causes
hvm_monitor_debug() to ignore the recursive request.

Does this plan sound ok, or have I missed something more subtle with monitor
handling?
---
 xen/arch/x86/hvm/svm/svm.c | 126 -
 xen/arch/x86/hvm/vmx/vmx.c | 102 +-
 2 files changed, 110 insertions(+), 118 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index ac3123c56f..bd3adde0ec 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1328,19 +1328,50 @@ static void cf_check svm_inject_event(const struct 
x86_event *event)
 switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) )
 {
 case X86_EXC_DB:
-if ( regs->eflags & X86_EFLAGS_TF )
-{
-__restore_debug_registers(vmcb, curr);
-vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | DR_STEP);
-}
+/*
+ * On AMD hardware, a #DB exception:
+ *  1) Merges new status bits into %dr6
+ *  2) Clears %dr7.gd and MSR_DEBUGCTL.{LBR,BTF}
+ *
+ * Item 1 is done by hardware before a #DB intercepted vmexit, but we
+ * may end up here from emulation so have to repeat it ourselves.
+ * Item 2 is done by hardware when injecting a #DB exception.
+ */
+__restore_debug_registers(vmcb, curr);
+vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | _event.pending_dbg);
+
 /* fall through */
 case X86_EXC_BP:
 if ( curr->domain->debugger_attached )
 {
 /* Debug/Int3: Trap to debugger. */
+if ( _event.vector == X86_EXC_BP )
+{
+/* N.B. Can't use __update_guest_eip() for risk of recusion. */
+regs->rip += _event.insn_len;
+regs->eflags &= ~X86_EFLAGS_RF;
+curr->arch.gdbsx_vcpu_event = X86_EXC_BP;
+}
+
 domain_pause_for_debugger();
 return;
 }
+else
+{
+int rc = hvm_monitor_debug(regs->rip,
+   _event.vector == X86_EXC_DB
+   ? HVM_MONITOR_DEBUG_EXCEPTION
+   : HVM_MONITOR_SOFTWARE_BREAKPOINT,
+   _event.type, _event.insn_len,
+   _event.pending_dbg);
+if ( rc < 0 )
+{
+gprintk(XENLOG_ERR, "Monitor debug error %d\n", rc);
+return svm_crash_or_fault(curr);
+}
+if ( rc )
+return; /* VCPU paused.  Wait for monitor. */
+}
 break;
 
 case X86_EXC_PF:
@@ -2730,67 +2761,46 @@ void svm_vmexit_handler(void)
 
 case VMEXIT_ICEBP:
 case VMEXIT_EXCEPTION_DB:
-if ( !v->domain->debugger_attached )
+case VMEXIT_EXCEPTION_BP:
+{
+unsigned int vec, type, len, extra;
+
+switch ( exit_reason )
 {
-unsigned int trap_type;
+case VMEXIT_ICEBP:
+vec   = X86_EXC_DB;
+type  = X86_EVENTTYPE_PRI_SW_EXCEPTION;
+len   = svm_get_insn_len(v, INSTR_ICEBP);
+extra = 0;
+break;
 
-if ( likely(exit_reason != VMEXIT_ICEBP) )
-{
-trap_type = X86_EVENTTYPE_HW_EXCEPTION;
-insn_len = 0;
-}
-else
-{
-trap_type = 

[PATCH 1/5] x86: Fix calculation of %dr6/7 reserved bits

[Jinoh Kang]

From: Andrew Cooper 

The reserved bit calculations for %dr6 and %dr7 depend on whether the VM has
the Restricted Transnational Memory feature available.

Introduce adjust_dr{6,7}_rsvd() and replace the opencoded logic and constants
(except for DR_STATUS_RESERVED_ONE which is (mis)used elsewhere and will be
removed after future bugfixes).  The use of these helpers in set_debugreg()
covers toolstack values for PV guests, but HVM guests need similar treatment.

The use of the guests cpuid policy is less than optimal in the create/restore
paths.  However in such cases, the policy will be the guest maximum policy,
which will be more permissive with respect to the RTM feature.

Signed-off-by: Jinoh Kang 
---
CC: Jan Beulich 
CC: Wei Liu 
CC: Roger Pau Monné 
---
 xen/arch/x86/domain.c   |  7 +++--
 xen/arch/x86/hvm/hvm.c  |  6 ++--
 xen/arch/x86/include/asm/debugreg.h | 44 +
 xen/arch/x86/pv/misc-hypercalls.c   | 16 +++
 4 files changed, 50 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index fe86a7f853..a39710b5af 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1053,6 +1053,7 @@ int arch_set_info_guest(
 struct vcpu *v, vcpu_guest_context_u c)
 {
 struct domain *d = v->domain;
+const struct cpu_policy *cp = d->arch.cpuid;
 unsigned int i;
 unsigned long flags;
 bool compat;
@@ -1165,10 +1166,10 @@ int arch_set_info_guest(
 
 if ( is_hvm_domain(d) )
 {
-for ( i = 0; i < ARRAY_SIZE(v->arch.dr); ++i )
+for ( i = 0; i < ARRAY_SIZE(v->arch.dr) - 2; ++i )
 v->arch.dr[i] = c(debugreg[i]);
-v->arch.dr6 = c(debugreg[6]);
-v->arch.dr7 = c(debugreg[7]);
+v->arch.dr6 = adjust_dr6_rsvd(c(debugreg[6]), cp->feat.rtm);
+v->arch.dr7 = adjust_dr7_rsvd(c(debugreg[7]), cp->feat.rtm);
 
 if ( v->vcpu_id == 0 )
 d->vm_assist = c.nat->vm_assist;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 3a99c0ff20..66ead0b878 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -985,6 +986,7 @@ unsigned long hvm_cr4_guest_valid_bits(const struct domain 
*d)
 
 static int cf_check hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t 
*h)
 {
+const struct cpu_policy *cp = d->arch.cpuid;
 unsigned int vcpuid = hvm_load_instance(h);
 struct vcpu *v;
 struct hvm_hw_cpu ctxt;
@@ -1166,8 +1168,8 @@ static int cf_check hvm_load_cpu_ctxt(struct domain *d, 
hvm_domain_context_t *h)
 v->arch.dr[1] = ctxt.dr1;
 v->arch.dr[2] = ctxt.dr2;
 v->arch.dr[3] = ctxt.dr3;
-v->arch.dr6   = ctxt.dr6;
-v->arch.dr7   = ctxt.dr7;
+v->arch.dr6   = adjust_dr6_rsvd(ctxt.dr6, cp->feat.rtm);
+v->arch.dr7   = adjust_dr7_rsvd(ctxt.dr7, cp->feat.rtm);
 
 hvmemul_cancel(v);
 
diff --git a/xen/arch/x86/include/asm/debugreg.h 
b/xen/arch/x86/include/asm/debugreg.h
index 86aa6d7143..8be60910b4 100644
--- a/xen/arch/x86/include/asm/debugreg.h
+++ b/xen/arch/x86/include/asm/debugreg.h
@@ -10,9 +10,18 @@
 #define DR_STATUS6
 #define DR_CONTROL   7
 
-/* Define a few things for the status register.  We can use this to determine
-   which debugging register was responsible for the trap.  The other bits
-   are either reserved or not of interest to us. */
+/*
+ * DR6 status bits.
+ *   N.B. For backwards compatibility, X86_DR6_RTM has inverted polarity.
+ */
+#define X86_DR6_B0  (1u <<  0)  /* Breakpoint 0 triggered  */
+#define X86_DR6_B1  (1u <<  1)  /* Breakpoint 1 triggered  */
+#define X86_DR6_B2  (1u <<  2)  /* Breakpoint 2 triggered  */
+#define X86_DR6_B3  (1u <<  3)  /* Breakpoint 3 triggered  */
+#define X86_DR6_BD  (1u << 13)  /* Debug register accessed */
+#define X86_DR6_BS  (1u << 14)  /* Single step */
+#define X86_DR6_BT  (1u << 15)  /* Task switch */
+#define X86_DR6_RTM (1u << 16)  /* #DB/#BP in RTM region   */
 
 #define DR_TRAP0(0x1)   /* db0 */
 #define DR_TRAP1(0x2)   /* db1 */
@@ -21,7 +30,6 @@
 #define DR_STEP (0x4000)/* single-step */
 #define DR_SWITCH   (0x8000)/* task switch */
 #define DR_NOT_RTM  (0x1)   /* clear: #BP inside RTM region */
-#define DR_STATUS_RESERVED_ZERO (~0xefffUL) /* Reserved, read as zero */
 #define DR_STATUS_RESERVED_ONE  0x0ff0UL /* Reserved, read as one */
 
 /* Now define a bunch of things for manipulating the control register.
@@ -61,8 +69,6 @@
We can slow the instruction pipeline for instructions coming via the
gdt or the ldt if we want to.  I am not sure why this is an advantage */
 
-#define DR_CONTROL_RESERVED_ZERO (~0x27ffUL) /* Reserved, read as zero */
-#define DR_CONTROL_RESERVED_ONE  (0x0400UL) /* 

[PATCH 0/5] Fixes to debugging facilities

[Jinoh Kang]

This is a rebased version of Andrew Cooper's debugging facilities patch:
https://lore.kernel.org/xen-devel/1528120755-17455-1-git-send-email-andrew.coop...@citrix.com/

> So this started as a small fix for the vmentry failure (penultimate patch),
> and has snowballed...
>
> I'm fairly confident that everything involving DEBUGCTL.BTF is broken, and
> there are definitely bugs with configuring DEBUGCTL.RTM (which really isn't
> helped by the fact that the GCC TSX intrinsics render the resulting code
> un-debuggable.)  I'll defer fixing these swamps for now.
>
> The first 4 patches probably want backporting to the stable trees, so I've
> taken care to move them ahead of patch 6 for backport reasons.  While all
> fixes would ideally be backported, I can't find a way of fixing %dr6 merging
> (as it needs to be done precicely once) without a behavioural change in the
> monitor subsystem.
>
> Patch 8 probably breaks introspection, so can't be taken at this point.  See
> that patch for discussion of the problem and my best guess at a solution.

6 out of 11 patches from the 2018 patch series above, including the
vmentry failure fix, have already been committed.  This covers the
remaining 5 patches.

One particular bug that the patch series fixes involves simultaneous
hardware breakpoint exception and single-stepping exception occurring at
the same PC (IP).  Xen blindly sets singlestep (DR6.BS := 1) in this
case, which interferes with userland debugging and allows (otherwise
restricted) usermode programs to detect Xen HVM (or PVH).  The following
Linux x86-64 program demonstrates the bug:

---8<---

#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

#define ABORT_ON_ERR(x) if ((x) == -1) abort();

int main(void)
{
unsigned long cur_rip, cur_eflags, cur_dr6;
int wstatus, exit_code;
pid_t pid;

ABORT_ON_ERR(pid = fork());
if (pid == 0) {
ABORT_ON_ERR(ptrace(PTRACE_TRACEME, 0, NULL, NULL));
ABORT_ON_ERR(raise(SIGSTOP));
_exit(0);
}

/* Wait for first ptrace event */
if (waitpid(pid, , 0) != pid) abort();
if (!WIFSTOPPED(wstatus)) abort();

/* Obtain current RIP value and perform sanity check */
cur_rip = ptrace(PTRACE_PEEKUSER, pid, (void *)offsetof(struct user, 
regs.rip), _rip);
cur_dr6 = ptrace(PTRACE_PEEKUSER, pid, (void *)offsetof(struct user, 
u_debugreg[6]), _dr6);
assert(cur_dr6 == 0x0ff0UL);

/* Set up debug registers and set EFLAGS.TF */
cur_eflags = ptrace(PTRACE_PEEKUSER, pid, (void *)offsetof(struct user, 
regs.eflags), _eflags);
ABORT_ON_ERR(ptrace(PTRACE_POKEUSER, pid, (void *)offsetof(struct user, 
regs.eflags), (void *)(cur_eflags | 0x100UL)));
ABORT_ON_ERR(ptrace(PTRACE_POKEUSER, pid, (void *)offsetof(struct user, 
u_debugreg[0]), (void *)cur_rip));
ABORT_ON_ERR(ptrace(PTRACE_POKEUSER, pid, (void *)offsetof(struct user, 
u_debugreg[7]), (void *)1L));

/* Continue execution to trigger hardware breakpoint */
ABORT_ON_ERR(ptrace(PTRACE_CONT, pid, NULL, (unsigned long)0));
if (waitpid(pid, , 0) != pid) abort();
if (!(WIFSTOPPED(wstatus) && WSTOPSIG(wstatus) == SIGTRAP)) abort();

/* Detect if Xen has tampered with DR6 */
cur_dr6 = ptrace(PTRACE_PEEKUSER, pid, (void *)offsetof(struct user, 
u_debugreg[6]), _dr6);
fprintf(stderr, "DR6 = 0x%08lx\n", cur_dr6);
if (cur_dr6 == 0x0ff1UL)
{
fputs("Running on bare-metal, Xen PV, or non-Xen VMM\n", stdout);
exit_code = EXIT_FAILURE;
}
else
{
fputs("Running on Xen HVM\n", stdout);
exit_code = EXIT_SUCCESS;
}

/* Tear down debug registers and unset EFLAGS.TF */
cur_eflags = ptrace(PTRACE_PEEKUSER, pid, (void *)offsetof(struct user, 
regs.eflags), _eflags);
ABORT_ON_ERR(ptrace(PTRACE_POKEUSER, pid, (void *)offsetof(struct user, 
regs.eflags), (void *)(cur_eflags & ~0x100UL)));
ABORT_ON_ERR(ptrace(PTRACE_POKEUSER, pid, (void *)offsetof(struct user, 
u_debugreg[7]), (void *)0L));

/* Continue execution to let child process exit */
ABORT_ON_ERR(ptrace(PTRACE_CONT, pid, NULL, (unsigned long)0));
if (waitpid(pid, , 0) != pid) abort();
if (!(WIFEXITED(wstatus) && WEXITSTATUS(wstatus) == 0)) abort();

return exit_code;
}

---8<---

Andrew Cooper (5):
  x86: Fix calculation of %dr6/7 reserved bits
  x86/emul: Add pending_dbg field to x86_event
  x86/hvm: RFC - PROBABLY BROKEN - Defer all debugging/monitor actions
to {svm,vmx}_inject_event()
  x86: Fix merging of new status bits into %dr6
  x86/dbg: Cleanup of legacy dr6 constants

 xen/arch/x86/domain.c  |   7 +-
 xen/arch/x86/hvm/emulate.c |   3 +-
 xen/arch/x86/hvm/hvm.c |   8 +-
 xen/arch/x86/hvm/svm/svm.c | 126 ++---
 xen/arch/x86/hvm/vmx/vmx.c | 

Re: [PATCH v6] tools/xenstore: move xenstored sources into dedicated directory

[Juergen Gross]

On 21.08.23 17:17, Anthony PERARD wrote:

On Mon, Aug 21, 2023 at 10:14:22AM +0200, Juergen Gross wrote:

diff --git a/tools/xenstored/.gitignore b/tools/xenstored/.gitignore
new file mode 100644
index 00..edbb5d79fe
--- /dev/null
+++ b/tools/xenstored/.gitignore
@@ -0,0 +1 @@
+xenstored


Could you write that "/xenstored" ? The prefix "/" just makes sure that
only the file in the current directory is ignored, and not any
"xenstored" in subdirectory. Just in case.


Okay.




diff --git a/tools/xenstored/Makefile b/tools/xenstored/Makefile
new file mode 100644
index 00..f3bd3d43c4
--- /dev/null
+++ b/tools/xenstored/Makefile
@@ -0,0 +1,48 @@
+XEN_ROOT=$(CURDIR)/../..
+include $(XEN_ROOT)/tools/Rules.mk
+
+include Makefile.common
+
+xenstored: LDLIBS += $(LDLIBS_libxenevtchn)
+xenstored: LDLIBS += $(LDLIBS_libxengnttab)
+xenstored: LDLIBS += $(LDLIBS_libxenctrl)
+xenstored: LDLIBS += -lrt
+xenstored: LDLIBS += $(SOCKET_LIBS)
+
+ifeq ($(CONFIG_SYSTEMD),y)
+$(XENSTORED_OBJS-y): CFLAGS += $(SYSTEMD_CFLAGS)
+xenstored: LDLIBS += $(SYSTEMD_LIBS)
+endif
+
+TARGETS += xenstored


Could you change that to := instead of += ? TARGETS is currently
introduced with a := (in tools/xenstore/Makefile).


Fine with me.




diff --git a/tools/xs-clients/.gitignore b/tools/xs-clients/.gitignore
new file mode 100644
index 00..233fd8228a
--- /dev/null
+++ b/tools/xs-clients/.gitignore
@@ -0,0 +1,10 @@
+xenstore
+xenstore-chmod
+xenstore-control
+xenstore-exists
+xenstore-list
+xenstore-ls
+xenstore-read
+xenstore-rm
+xenstore-watch
+xenstore-write


Same thing here, could you prefix all those entries with "/"?


Yes.




diff --git a/tools/xenstore/Makefile b/tools/xs-clients/Makefile
similarity index 74%
rename from tools/xenstore/Makefile
rename to tools/xs-clients/Makefile
index dc39b6cb31..1c5740450a 100644
--- a/tools/xenstore/Makefile
+++ b/tools/xs-clients/Makefile


I'm tempted to ask for the targets "clients-install" and
"clients-uninstall" to be removed from this makefile. Nothing is calling
them in our build system and something outside the git tree that rely on
that would need to be adjusted to the new directory. But maybe that can
be done in a followup patch as it would help with reverting it if the
targets are actually useful.


I'll remove those.




In any case, the patch is already good:
Acked-by: Anthony PERARD 


Thanks,

Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH v6] tools/xenstore: move xenstored sources into dedicated directory

[Anthony PERARD]

On Mon, Aug 21, 2023 at 10:14:22AM +0200, Juergen Gross wrote:
> diff --git a/tools/xenstored/.gitignore b/tools/xenstored/.gitignore
> new file mode 100644
> index 00..edbb5d79fe
> --- /dev/null
> +++ b/tools/xenstored/.gitignore
> @@ -0,0 +1 @@
> +xenstored

Could you write that "/xenstored" ? The prefix "/" just makes sure that
only the file in the current directory is ignored, and not any
"xenstored" in subdirectory. Just in case.

> diff --git a/tools/xenstored/Makefile b/tools/xenstored/Makefile
> new file mode 100644
> index 00..f3bd3d43c4
> --- /dev/null
> +++ b/tools/xenstored/Makefile
> @@ -0,0 +1,48 @@
> +XEN_ROOT=$(CURDIR)/../..
> +include $(XEN_ROOT)/tools/Rules.mk
> +
> +include Makefile.common
> +
> +xenstored: LDLIBS += $(LDLIBS_libxenevtchn)
> +xenstored: LDLIBS += $(LDLIBS_libxengnttab)
> +xenstored: LDLIBS += $(LDLIBS_libxenctrl)
> +xenstored: LDLIBS += -lrt
> +xenstored: LDLIBS += $(SOCKET_LIBS)
> +
> +ifeq ($(CONFIG_SYSTEMD),y)
> +$(XENSTORED_OBJS-y): CFLAGS += $(SYSTEMD_CFLAGS)
> +xenstored: LDLIBS += $(SYSTEMD_LIBS)
> +endif
> +
> +TARGETS += xenstored

Could you change that to := instead of += ? TARGETS is currently
introduced with a := (in tools/xenstore/Makefile).

> diff --git a/tools/xs-clients/.gitignore b/tools/xs-clients/.gitignore
> new file mode 100644
> index 00..233fd8228a
> --- /dev/null
> +++ b/tools/xs-clients/.gitignore
> @@ -0,0 +1,10 @@
> +xenstore
> +xenstore-chmod
> +xenstore-control
> +xenstore-exists
> +xenstore-list
> +xenstore-ls
> +xenstore-read
> +xenstore-rm
> +xenstore-watch
> +xenstore-write

Same thing here, could you prefix all those entries with "/"?

> diff --git a/tools/xenstore/Makefile b/tools/xs-clients/Makefile
> similarity index 74%
> rename from tools/xenstore/Makefile
> rename to tools/xs-clients/Makefile
> index dc39b6cb31..1c5740450a 100644
> --- a/tools/xenstore/Makefile
> +++ b/tools/xs-clients/Makefile

I'm tempted to ask for the targets "clients-install" and
"clients-uninstall" to be removed from this makefile. Nothing is calling
them in our build system and something outside the git tree that rely on
that would need to be adjusted to the new directory. But maybe that can
be done in a followup patch as it would help with reverting it if the
targets are actually useful.


In any case, the patch is already good:
Acked-by: Anthony PERARD 

Thanks,

-- 
Anthony PERARD

Ping: [PATCH] mem-sharing: move (x86) / drop (Arm) arch_dump_shared_mem_info()

[Jan Beulich]

On 08.08.2023 14:02, Jan Beulich wrote:
> When !MEM_SHARING no useful output is produced. Move the function into
> mm/mem_sharing.c while conditionalizing the call to it, thus allowing to
> drop it altogether from Arm (and eliminating the need to introduce stubs
> on PPC and RISC-V).
> 
> Signed-off-by: Jan Beulich 

Tamas - any chance of an ack?

Thanks, Jan

> ---
> I wasn't really sure whether introducing a stub in xen/mm.h would be any
> better than adding the (further) #ifdef to dump_domains().
> 
> We could go further and also eliminate the need for the stub variants
> of mem_sharing_get_nr_{shared,saved}_mfns() by moving the
> XENMEM_get_sharing_{shared,freed}_pages cases in
> {,compat_}arch_memory_op() into the already existing #ifdef-s there.
> Returning an error for those sub-ops may be slightly more appropriate
> than returning 0 when !MEM_SHARING.
> 
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1297,10 +1297,6 @@ void free_init_memory(void)
>  printk("Freed %ldkB init memory.\n", 
> (long)(__init_end-__init_begin)>>10);
>  }
>  
> -void arch_dump_shared_mem_info(void)
> -{
> -}
> -
>  int steal_page(
>  struct domain *d, struct page_info *page, unsigned int memflags)
>  {
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -6265,13 +6265,6 @@ void memguard_unguard_stack(void *p)
>  map_pages_to_xen((unsigned long)p, virt_to_mfn(p), 1, 
> PAGE_HYPERVISOR_RW);
>  }
>  
> -void arch_dump_shared_mem_info(void)
> -{
> -printk("Shared frames %u -- Saved frames %u\n",
> -mem_sharing_get_nr_shared_mfns(),
> -mem_sharing_get_nr_saved_mfns());
> -}
> -
>  const struct platform_bad_page *__init get_platform_badpages(unsigned int 
> *array_size)
>  {
>  u32 igd_id;
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -2329,3 +2329,10 @@ int mem_sharing_domctl(struct domain *d,
>  
>  return rc;
>  }
> +
> +void arch_dump_shared_mem_info(void)
> +{
> +printk("Shared frames %u -- Saved frames %u\n",
> +mem_sharing_get_nr_shared_mfns(),
> +mem_sharing_get_nr_saved_mfns());
> +}
> --- a/xen/common/keyhandler.c
> +++ b/xen/common/keyhandler.c
> @@ -365,7 +365,9 @@ static void cf_check dump_domains(unsign
>  }
>  }
>  
> +#ifdef CONFIG_MEM_SHARING
>  arch_dump_shared_mem_info();
> +#endif
>  
>  rcu_read_unlock(_read_lock);
>  }
> 

Re: [PATCH -next] xen: Fix one kernel-doc comment

[Juergen Gross]

On 31.07.23 05:00, Yang Li wrote:

Use colon to separate parameter name from their specific meaning.
silence the warning:

drivers/xen/grant-table.c:1051: warning: Function parameter or member 
'nr_pages' not described in 'gnttab_free_pages'

Reported-by: Abaci Robot 
Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=6030
Signed-off-by: Yang Li 


Acked-by: Juergen Gross 


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

[linux-linus test] 182408: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-xl-qemut-debianhvm-amd64 18 guest-localmigrate/x10 fail REGR. 
vs. 182405
 test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 21 guest-start.2 fail 
REGR. vs. 182405
 build-arm64-pvops 6 kernel-build fail REGR. vs. 182405

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

version targeted for testing:
 linuxf7757129e3dea336c407551c98f50057c22bb266
baseline version:
 linux706a741595047797872e669b3101429ab8d378ef

Last test of basis   182405  2023-08-20 18:11:48 Z0 days
Testing same since   182408  2023-08-21 05:42:03 Z0 days1 attempts


People who touched revisions under test:
  Herbert Xu 
  Linus Torvalds 
  Pavel Skripkin 

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-pvopsfail  

Re: Community Manager update - August 2023

[Kelly Choi]

Hey Chuck,

Thanks for your feedback and highlighting this to me.
You're correct, they did rebrand so I will update this to 'IBM Cloud'.

Many thanks,
Kelly Choi

Open Source Community Manager, XenServer
Cloud Software Group


On Sat, Aug 19, 2023 at 12:34 AM Chuck Zmudzinski 
wrote:

> On 8/18/2023 6:55 AM, Kelly Choi wrote:
> > Hi everyone! :)
> >
> > I hope you're all well.
> >
> > If we haven't met before, I'd like to introduce myself. I'm Kelly, the
> Community Manager for The Xen Project. My role is to support everyone and
> make sure the project is healthy and thriving.
> >
> > *The latest update below requires your attention:*
> > *
> > *
> >
> >   * *We will be moving IRC channels fully to Matrix in September 2023.
> Once the channels have been created, further information will be shared. *
> >   * *New Mission Statement, goals, and purpose is attached to this email
> and will be shared publicly.*
> >
> > *Should anyone have any concerns or feedback, please let me know*
> >
> > Many thanks,
> > Kelly Choi
> >
> > Open Source Community Manager, XenServer
> > Cloud Software Group
>
> This looks good, but I thought IBM rebranded Softlayer as IBM Cloud
> several years ago. Maybe IBM Softlayer should be changed to IBM Cloud?
> Thanks.
>
> Chuck
>

Re: [PATCH v2 3/4] virtio: use defer_call() in virtio_irqfd_notify()

[Ilya Maximets]

On 8/17/23 17:58, Stefan Hajnoczi wrote:
> virtio-blk and virtio-scsi invoke virtio_irqfd_notify() to send Used
> Buffer Notifications from an IOThread. This involves an eventfd
> write(2) syscall. Calling this repeatedly when completing multiple I/O
> requests in a row is wasteful.
> 
> Use the defer_call() API to batch together virtio_irqfd_notify() calls
> made during thread pool (aio=threads), Linux AIO (aio=native), and
> io_uring (aio=io_uring) completion processing.
> 
> Behavior is unchanged for emulated devices that do not use
> defer_call_begin()/defer_call_end() since defer_call() immediately
> invokes the callback when called outside a
> defer_call_begin()/defer_call_end() region.
> 
> fio rw=randread bs=4k iodepth=64 numjobs=8 IOPS increases by ~9% with a
> single IOThread and 8 vCPUs. iodepth=1 decreases by ~1% but this could
> be noise. Detailed performance data and configuration specifics are
> available here:
> https://gitlab.com/stefanha/virt-playbooks/-/tree/blk_io_plug-irqfd
> 
> This duplicates the BH that virtio-blk uses for batching. The next
> commit will remove it.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  block/io_uring.c   |  6 ++
>  block/linux-aio.c  |  4 
>  hw/virtio/virtio.c | 11 ++-
>  util/thread-pool.c |  5 +
>  4 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/block/io_uring.c b/block/io_uring.c
> index 3a1e1f45b3..7cdd00e9f1 100644
> --- a/block/io_uring.c
> +++ b/block/io_uring.c
> @@ -125,6 +125,9 @@ static void luring_process_completions(LuringState *s)
>  {
>  struct io_uring_cqe *cqes;
>  int total_bytes;
> +
> +defer_call_begin();
> +
>  /*
>   * Request completion callbacks can run the nested event loop.
>   * Schedule ourselves so the nested event loop will "see" remaining
> @@ -217,7 +220,10 @@ end:
>  aio_co_wake(luringcb->co);
>  }
>  }
> +
>  qemu_bh_cancel(s->completion_bh);
> +
> +defer_call_end();
>  }
>  
>  static int ioq_submit(LuringState *s)
> diff --git a/block/linux-aio.c b/block/linux-aio.c
> index 62380593c8..ab607ade6a 100644
> --- a/block/linux-aio.c
> +++ b/block/linux-aio.c
> @@ -205,6 +205,8 @@ static void qemu_laio_process_completions(LinuxAioState 
> *s)
>  {
>  struct io_event *events;
>  
> +defer_call_begin();
> +
>  /* Reschedule so nested event loops see currently pending completions */
>  qemu_bh_schedule(s->completion_bh);
>  
> @@ -231,6 +233,8 @@ static void qemu_laio_process_completions(LinuxAioState 
> *s)
>   * own `for` loop.  If we are the last all counters droped to zero. */
>  s->event_max = 0;
>  s->event_idx = 0;
> +
> +defer_call_end();
>  }
>  
>  static void qemu_laio_process_completions_and_submit(LinuxAioState *s)
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 309038fd46..5eb1f91b41 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -15,6 +15,7 @@
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-virtio.h"
>  #include "trace.h"
> +#include "qemu/defer-call.h"
>  #include "qemu/error-report.h"
>  #include "qemu/log.h"
>  #include "qemu/main-loop.h"
> @@ -28,6 +29,7 @@
>  #include "hw/virtio/virtio-bus.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/virtio/virtio-access.h"
> +#include "sysemu/block-backend.h"

An artifact from the previous version.

>  #include "sysemu/dma.h"
>  #include "sysemu/runstate.h"
>  #include "virtio-qmp.h"
> @@ -2426,6 +2428,13 @@ static bool virtio_should_notify(VirtIODevice *vdev, 
> VirtQueue *vq)
>  }
>  }
>  
> +/* Batch irqs while inside a defer_call_begin()/defer_call_end() section */
> +static void virtio_notify_irqfd_deferred_fn(void *opaque)
> +{
> +EventNotifier *notifier = opaque;
> +event_notifier_set(notifier);
> +}
> +
>  void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq)
>  {
>  WITH_RCU_READ_LOCK_GUARD() {
> @@ -2452,7 +2461,7 @@ void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue 
> *vq)
>   * to an atomic operation.
>   */
>  virtio_set_isr(vq->vdev, 0x1);
> -event_notifier_set(>guest_notifier);
> +defer_call(virtio_notify_irqfd_deferred_fn, >guest_notifier);

Should we move the trace from this function to deferred one?
Or maybe add a new trace?

>  }
>  
>  static void virtio_irq(VirtQueue *vq)
> diff --git a/util/thread-pool.c b/util/thread-pool.c
> index e3d8292d14..d84961779a 100644
> --- a/util/thread-pool.c
> +++ b/util/thread-pool.c
> @@ -15,6 +15,7 @@
>   * GNU GPL, version 2 or (at your option) any later version.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/defer-call.h"
>  #include "qemu/queue.h"
>  #include "qemu/thread.h"
>  #include "qemu/coroutine.h"
> @@ -175,6 +176,8 @@ static void thread_pool_completion_bh(void *opaque)
>  ThreadPool *pool = opaque;
>  ThreadPoolElement *elem, *next;
>  
> +defer_call_begin(); /* cb() may use defer_call() to coalesce work */
> +
>  restart:
>  QLIST_FOREACH_SAFE(elem, >head, all, 

Re: [PATCH v3 0/8] Follow-up static shared memory PART I

[Michal Orzel]

Hi Penny,

On 21/08/2023 06:00, Penny Zheng wrote:
> 
> 
> There are some unsolving issues on current 4.17 static shared memory
> feature[1], including:
> - In order to avoid keeping growing 'membank', having the shared memory
> info in separate structures is preferred.
> - Missing implementation on having the host address optional in
> "xen,shared-mem" property
> - Removing static shared memory from extended regions
> - Missing reference release on foreign superpage
> - Missing "xen,offset" feature, which is introduced in Linux DOC[2]
> 
> All above objects have been divided into two parts to complete. And this
> patch serie is PART I.
> 
> [1] https://lore.kernel.org/all/20220908135513.1800511-1-penny.zh...@arm.com/
> [2] 
> https://www.kernel.org/doc/Documentation/devicetree/bindings/reserved-memory/xen%2Cshared-memory.txt

It looks like there is a problem with the changes introduced in this series.
The gitlab static shared memory tests failed:
https://gitlab.com/xen-project/patchew/xen/-/pipelines/973985190
No Xen logs meaning the failure occurred before serial console initialization.

Now, I would like to share some observations after playing around with the 
current static shared mem code today.
1) Static shared memory region is advertised to a domain by creating a child 
node under reserved-memory.
/reserved-memory is nothing but a way to carve out a region from the normal 
memory specified in /memory node.
For me, such regions should be described in domain's /memory node as well. This 
is not the case at the moment
for static shm unlike to other sub-nodes of /reserved-memory (present in host 
dtb) for which Xen creates separate
/memory nodes.

2) Domain dtb parsing issue with two /reserved-memory nodes present.
In case there is a /reserved-memory node already present in the host dtb, Xen 
would create yet another /reserved-memory
node for the static shm (to be observed in case of dom0). This is a bug as 
there can be only one /reserved-memory node.
This leads to an error when dumping with dtc and leads to a shm node not being 
visible to a domain (guest OS relies on
a presence of a single /reserved-memory node). The issue is because in 
make_resv_memory_node(), you are not checking if
such node already exists.

I haven't looked closely at this series yet. It might be that these issues are 
fixed. If not, I would definitely
suggest to fix them in the first place.

~Michal

Re: [PATCH v5 04/13] xen/arm64: Split and move MMU-specific head.S to mmu/head.S

[Henry Wang]

Hi Julien,

> On Aug 21, 2023, at 18:16, Julien Grall  wrote:
> On 21/08/2023 10:29, Henry Wang wrote:
>>> On Aug 21, 2023, at 17:18, Julien Grall  wrote:
>>> On 14/08/2023 05:25, Henry Wang wrote:
 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.
 - Use ENTRY() for enable_secondary_cpu_mm, enable_boot_cpu_mm and
   setup_fixmap to please the compiler after the code movement.
>>> 
>>> I am not sure I understand why you are saying "to please the compiler" 
>>> here. Isn't it necessary for the linker (not the compiler) to find the 
>>> function? And therefore there is no pleasing (as in this is not a bug in 
>>> the toolchain).
>> Yes it meant to be linker, sorry for the confusion. What I want to express is
>> without the ENTRY(), for example if we remove the ENTRY() around the
>> setup_fixmap(), we will have:
>> ```
>> aarch64-none-linux-gnu-ld: prelink.o: in function `primary_switched':
>> /home/xinwan02/repos_for_development/xen_playground/xen/xen/arch/arm/arm64/head.S:278:
>>  undefined reference to `setup_fixmap'
>> /home/xinwan02/repos_for_development/xen_playground/xen/xen/arch/arm/arm64/head.S:278:(.text.header+0x1a0):
>>  relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol 
>> `setup_fixmap'
>> make[2]: *** [arch/arm/Makefile:95: xen-syms] Error 1
>> make[1]: *** [build.mk:90: xen] Error 2
>> make: *** [Makefile:598: xen] Error 2
>> ```
>> I will use the word “linker” in v6 if you agree.
> 
> The sentence also need to be reworded. How about:
> 
> "Use ENTRY() for ... as they will be used externally."

Sure, I will use the suggested sentence.

> 
>>> 
>>> Other than that, the split looks good to me.
>> May I please take this as a Reviewed-by tag? I will add the tag if you are
>> happy with that.
> 
> Sure. Reviewed-by: Julien Grall 

Thanks!

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v5 04/13] xen/arm64: Split and move MMU-specific head.S to mmu/head.S

[Julien Grall]

On 21/08/2023 10:29, Henry Wang wrote:

On Aug 21, 2023, at 17:18, Julien Grall  wrote:
On 14/08/2023 05:25, Henry Wang wrote:

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.
- Use ENTRY() for enable_secondary_cpu_mm, enable_boot_cpu_mm and
   setup_fixmap to please the compiler after the code movement.


I am not sure I understand why you are saying "to please the compiler" here. 
Isn't it necessary for the linker (not the compiler) to find the function? And therefore 
there is no pleasing (as in this is not a bug in the toolchain).


Yes it meant to be linker, sorry for the confusion. What I want to express is
without the ENTRY(), for example if we remove the ENTRY() around the
setup_fixmap(), we will have:

```
aarch64-none-linux-gnu-ld: prelink.o: in function `primary_switched':
/home/xinwan02/repos_for_development/xen_playground/xen/xen/arch/arm/arm64/head.S:278:
 undefined reference to `setup_fixmap'
/home/xinwan02/repos_for_development/xen_playground/xen/xen/arch/arm/arm64/head.S:278:(.text.header+0x1a0):
 relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol 
`setup_fixmap'
make[2]: *** [arch/arm/Makefile:95: xen-syms] Error 1
make[1]: *** [build.mk:90: xen] Error 2
make: *** [Makefile:598: xen] Error 2
```

I will use the word “linker” in v6 if you agree.


The sentence also need to be reworded. How about:

"Use ENTRY() for ... as they will be used externally."





Other than that, the split looks good to me.


May I please take this as a Reviewed-by tag? I will add the tag if you are
happy with that.


Sure. Reviewed-by: Julien Grall 

Cheers,

--
Julien Grall

[xen-unstable test] 182406: tolerable FAIL

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 7 xen-install fail in 
182403 pass in 182406
 test-amd64-i386-libvirt-pair 10 xen-install/src_host fail in 182403 pass in 
182406
 test-amd64-i386-examine-uefi  6 xen-installfail pass in 182403
 test-amd64-i386-libvirt   7 xen-installfail pass in 182403
 test-amd64-amd64-xl-rtds 22 guest-start/debian.repeat  fail pass in 182403
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 20 guest-start/debianhvm.repeat 
fail pass in 182403

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

Re: [PATCH v5 06/13] xen/arm64: Fold setup_fixmap() to create_page_tables()

[Henry Wang]

> On Aug 21, 2023, at 17:22, Julien Grall  wrote:
> 
> Hi Henry,
> 
> On 14/08/2023 05:25, Henry Wang wrote:
>> The original assembly setup_fixmap() is actually doing two seperate
>> tasks, one is enabling the early UART when earlyprintk on, and the
>> other is to set up the fixmap (even when earlyprintk is off).
>> Per discussion in [1], since commit
>> 9d267c049d92 ("xen/arm64: Rework the memory layout"), there is no
>> chance that the fixmap and the mapping of early UART will clash with
>> the 1:1 mapping. Therefore the mapping of both the fixmap and the
>> early UART can be moved to the end of create_pagetables().
>> No functional change intended.
> 
> I would drop this sentence because the fixmap is now prepared much earlier in 
> the code. So there is technically some functional change.

Sure, I will drop this sentence in v6.

> 
>> [1] 
>> https://lore.kernel.org/xen-devel/78862bb8-fd7f-5a51-a7ae-3c5b5998e...@xen.org/
>> Signed-off-by: Henry Wang 
> 
> Reviewed-by: Julien Grall 

Thanks!

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v5 04/13] xen/arm64: Split and move MMU-specific head.S to mmu/head.S

[Henry Wang]

Hi Julien,

> On Aug 21, 2023, at 17:18, Julien Grall  wrote:
> 
> Hi Henry,
> 
> On 14/08/2023 05:25, Henry Wang wrote:
>> 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.
>> - Use ENTRY() for enable_secondary_cpu_mm, enable_boot_cpu_mm and
>>   setup_fixmap to please the compiler after the code movement.
> 
> I am not sure I understand why you are saying "to please the compiler" here. 
> Isn't it necessary for the linker (not the compiler) to find the function? 
> And therefore there is no pleasing (as in this is not a bug in the toolchain).

Yes it meant to be linker, sorry for the confusion. What I want to express is
without the ENTRY(), for example if we remove the ENTRY() around the
setup_fixmap(), we will have:

```
aarch64-none-linux-gnu-ld: prelink.o: in function `primary_switched':
/home/xinwan02/repos_for_development/xen_playground/xen/xen/arch/arm/arm64/head.S:278:
 undefined reference to `setup_fixmap'
/home/xinwan02/repos_for_development/xen_playground/xen/xen/arch/arm/arm64/head.S:278:(.text.header+0x1a0):
 relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol 
`setup_fixmap'
make[2]: *** [arch/arm/Makefile:95: xen-syms] Error 1
make[1]: *** [build.mk:90: xen] Error 2
make: *** [Makefile:598: xen] Error 2
``` 

I will use the word “linker” in v6 if you agree.

> 
> Other than that, the split looks good to me.

May I please take this as a Reviewed-by tag? I will add the tag if you are
happy with that.

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v5 06/13] xen/arm64: Fold setup_fixmap() to create_page_tables()

[Julien Grall]

Hi Henry,

On 14/08/2023 05:25, Henry Wang wrote:

The original assembly setup_fixmap() is actually doing two seperate
tasks, one is enabling the early UART when earlyprintk on, and the
other is to set up the fixmap (even when earlyprintk is off).

Per discussion in [1], since commit
9d267c049d92 ("xen/arm64: Rework the memory layout"), there is no
chance that the fixmap and the mapping of early UART will clash with
the 1:1 mapping. Therefore the mapping of both the fixmap and the
early UART can be moved to the end of create_pagetables().

No functional change intended.


I would drop this sentence because the fixmap is now prepared much 
earlier in the code. So there is technically some functional change.




[1] 
https://lore.kernel.org/xen-devel/78862bb8-fd7f-5a51-a7ae-3c5b5998e...@xen.org/

Signed-off-by: Henry Wang 


Reviewed-by: Julien Grall 

Cheers,

--
Julien Grall

Re: [PATCH v5 04/13] xen/arm64: Split and move MMU-specific head.S to mmu/head.S

[Julien Grall]

Hi Henry,

On 14/08/2023 05:25, Henry Wang wrote:

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.
- Use ENTRY() for enable_secondary_cpu_mm, enable_boot_cpu_mm and
   setup_fixmap to please the compiler after the code movement.


I am not sure I understand why you are saying "to please the compiler" 
here. Isn't it necessary for the linker (not the compiler) to find the 
function? And therefore there is no pleasing (as in this is not a bug in 
the toolchain).


Other than that, the split looks good to me.

Cheers,

--
Julien Grall

[XEN PATCH v2 2/2] automation: avoid pipelines on specific branches

[Simone Ballarin]

This patch avoids the execution of pipelines in the
following branches:
- master
- smoke
- coverirty-tested/.*
- stable-.*

The job-level exclusions have been removed as they are
pointless with this new workspace-level exclusion.

Signed-off-by: Simone Ballarin 

---
Changes in v2:
- remove useless except clause in .yocto-test.
---
 .gitlab-ci.yml  |  6 ++
 automation/gitlab-ci/build.yaml | 11 ---
 automation/gitlab-ci/test.yaml  |  5 -
 3 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index ee5430b8b7..ef4484e09a 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -1,3 +1,9 @@
+workflow:
+  rules:
+- if: $CI_COMMIT_BRANCH =~ 
/^(master|smoke|^coverity-tested\/.*|stable-.*)$/
+  when: never
+- when: always
+
 stages:
   - analyze
   - build
diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index 1a4a5e490d..b633facff4 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -12,11 +12,6 @@
   - '*/*.log'
 when: always
   needs: []
-  except:
-- master
-- smoke
-- /^coverity-tested\/.*/
-- /^stable-.*/
 
 .gcc-tmpl:
   variables: 
@@ -214,11 +209,6 @@
 .yocto-test:
   stage: build
   image: registry.gitlab.com/xen-project/xen/${CONTAINER}
-  except:
-- master
-- smoke
-- /^coverity-tested\/.*/
-- /^stable-.*/
   script:
 - ./automation/build/yocto/build-yocto.sh -v --log-dir=./logs 
--xen-dir=`pwd` ${YOCTO_BOARD} ${YOCTO_OUTPUT}
   variables:
@@ -269,7 +259,6 @@
 .test-jobs-artifact-common:
   stage: build
   needs: []
-  except: !reference [.test-jobs-common, except]
 
 # Arm test artifacts
 
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 810631bc46..8737f367c8 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -1,11 +1,6 @@
 .test-jobs-common:
   stage: test
   image: registry.gitlab.com/xen-project/xen/${CONTAINER}
-  except:
-- master
-- smoke
-- /^coverity-tested\/.*/
-- /^stable-.*/
 
 .arm64-test-needs: 
   - alpine-3.18-arm64-rootfs-export
-- 
2.34.1

[XEN PATCH v2 1/2] automation/eclair: avoid unintentional ECLAIR analysis

[Simone Ballarin]

With this patch, ECLAIR jobs will need to be manually
started for "people/.*" pipelines and will not be triggered
if the WTOKEN variable is missing.

This avoids occupying the runner on analyzes that might
not be used by developers.

If developers want to analyze their own repositories
they need to launch them from GitLab.

Signed-off-by: Simone Ballarin 

---
Changes in v2:
- avoid ECLAIR jobs if the WTOKEN variable is not defined.
---
 automation/gitlab-ci/analyze.yaml | 22 +-
 automation/scripts/eclair |  5 -
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/automation/gitlab-ci/analyze.yaml 
b/automation/gitlab-ci/analyze.yaml
index 4aa4abe2ee..bd9a68de31 100644
--- a/automation/gitlab-ci/analyze.yaml
+++ b/automation/gitlab-ci/analyze.yaml
@@ -18,28 +18,40 @@
   - '*.log'
 reports:
   codequality: gl-code-quality-report.json
+  rules:
+- if: $WTOKEN == null
+  when: never
+- when: always
   needs: []
 
-eclair-x86_64:
+.eclair-analysis:triggered:
   extends: .eclair-analysis
+  allow_failure: true
+  rules:
+- if: $WTOKEN && $CI_PROJECT_PATH =~ /^xen-project\/people\/.*$/
+  when: manual
+- !reference [.eclair-analysis, rules]
+
+eclair-x86_64:
+  extends: .eclair-analysis:triggered
   variables:
 LOGFILE: "eclair-x86_64.log"
 VARIANT: "X86_64"
 RULESET: "Set1"
-  allow_failure: true
 
 eclair-ARM64:
-  extends: .eclair-analysis
+  extends: .eclair-analysis:triggered
   variables:
 LOGFILE: "eclair-ARM64.log"
 VARIANT: "ARM64"
 RULESET: "Set1"
-  allow_failure: true
 
 .eclair-analysis:on-schedule:
   extends: .eclair-analysis
   rules:
-- if: $CI_PIPELINE_SOURCE == "schedule"
+- if: $CI_PIPELINE_SOURCE != "schedule"
+  when: never
+- !reference [.eclair-analysis, rules]
 
 eclair-x86_64-Set1:on-schedule:
   extends: .eclair-analysis:on-schedule
diff --git a/automation/scripts/eclair b/automation/scripts/eclair
index 813a56eb6a..14e47a6f97 100755
--- a/automation/scripts/eclair
+++ b/automation/scripts/eclair
@@ -4,11 +4,6 @@ ECLAIR_ANALYSIS_DIR=automation/eclair_analysis
 ECLAIR_DIR="${ECLAIR_ANALYSIS_DIR}/ECLAIR"
 ECLAIR_OUTPUT_DIR=$(realpath "${ECLAIR_OUTPUT_DIR}")
 
-if [ -z "${WTOKEN:-}" ]; then
-echo "Failure: the WTOKEN variable is not defined." >&2
-exit 1
-fi
-
 "${ECLAIR_ANALYSIS_DIR}/prepare.sh" "${VARIANT}"
 
 ex=0
-- 
2.34.1

[XEN PATCH v2 0/2] automation: avoid unnecessary analyses

[Simone Ballarin]

This series aims to reduce the analyses performed by the ECLAIR
runner by avoiding some branches already excluded by other jobs
and requiring analyses on people/.* to be manually triggered.

---
Changes in v2:
- move some changes from 1/2 to 2/2.
- remove useless except clause in .yocto-test
- avoid ECLAIR jobs if the WTOKEN variable is not defined

Simone Ballarin (2):
  automation/eclair: avoid unintentional ECLAIR analysis
  automation: avoid pipelines on specific branches

 .gitlab-ci.yml|  6 ++
 automation/gitlab-ci/analyze.yaml | 22 +-
 automation/gitlab-ci/build.yaml   | 11 ---
 automation/gitlab-ci/test.yaml|  5 -
 automation/scripts/eclair |  5 -
 5 files changed, 23 insertions(+), 26 deletions(-)

-- 
2.34.1

Re: [PATCH v5 03/13] xen/arm64: prepare for moving MMU related code from head.S

[Henry Wang]

Hi Julien,

> On Aug 21, 2023, at 16:44, Julien Grall  wrote:
> 
> Hi Henry,
> 
> On 14/08/2023 05:25, Henry Wang wrote:
>> From: Wei Chen 
>> We want to reuse head.S for MPU systems, but there are some
>> code are implemented for MMU systems only. We will move such
>> code to another MMU specific file. But before that we will
>> do some indentations fix in this patch to make them be easier
>> for reviewing:
>> 1. Fix the indentations and incorrect style of code comments.
>> 2. Fix the indentations for .text.header section.
>> 3. Rename puts() to asm_puts() for global export
>> Signed-off-by: Wei Chen 
>> Signed-off-by: Penny Zheng 
>> Signed-off-by: Henry Wang 
>> Reviewed-by: Ayan Kumar Halder 
>> Reviewed-by: Julien Grall 
> 
> Is this patch depends on the first two? If not, I will commit it before v6.

Good point, no this patch is independent from the first two. Also I just
tested applying this patch on top of staging and building with and without
Earlyprintk. Xen and Dom0 boot fine on FVP for both cases.

So please commit this patch if you have time. Thanks!

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v5 02/13] xen/arm: Introduce CONFIG_MMU Kconfig option

[Henry Wang]

Hi Julien,

> On Aug 21, 2023, at 16:43, Julien Grall  wrote:
> 
> Hi Henry,
> 
> On 14/08/2023 05:25, Henry Wang wrote:
>> There are two types of memory system architectures available for
>> Arm-based systems, namely the Virtual Memory System Architecture (VMSA)
>> and the Protected Memory System Architecture (PMSA). According to
>> ARM DDI 0487G.a, A VMSA provides a Memory Management Unit (MMU) that
>> controls address translation, access permissions, and memory attribute
>> determination and checking, for memory accesses made by the PE. And
>> refer to ARM DDI 0600A.c, the PMSA supports a unified memory protection
>> scheme where an Memory Protection Unit (MPU) manages instruction and
>> data access. Currently, Xen only suuports VMSA.
> 
> Typo: s/suuports/supports/

Oops, sorry about this, will fix in v6.

> 
>> Introduce a Kconfig option CONFIG_MMU, which is currently default
>> set to y and unselectable because currently only VMSA is supported.
> 
> NIT: It would be worth to explicit mention that this will be used in 
> follow-up patches. So one will wonder what's the goal of introducing an 
> unused config.
> 
> Or it could have been merged in the first patch splitting the MMU code so we 
> don't introduce a config without any use.

Sure, I will think about this and fix in v6.

> 
>> Suggested-by: Julien Grall 
>> Signed-off-by: Henry Wang 
> 
> Acked-by: Julien Grall 

Thanks!

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v5 03/13] xen/arm64: prepare for moving MMU related code from head.S

[Julien Grall]

Hi Henry,

On 14/08/2023 05:25, Henry Wang wrote:

From: Wei Chen 

We want to reuse head.S for MPU systems, but there are some
code are implemented for MMU systems only. We will move such
code to another MMU specific file. But before that we will
do some indentations fix in this patch to make them be easier
for reviewing:
1. Fix the indentations and incorrect style of code comments.
2. Fix the indentations for .text.header section.
3. Rename puts() to asm_puts() for global export

Signed-off-by: Wei Chen 
Signed-off-by: Penny Zheng 
Signed-off-by: Henry Wang 
Reviewed-by: Ayan Kumar Halder 
Reviewed-by: Julien Grall 


Is this patch depends on the first two? If not, I will commit it before v6.

Cheers,

--
Julien Grall

Re: [PATCH v5 02/13] xen/arm: Introduce CONFIG_MMU Kconfig option

[Julien Grall]

Hi Henry,

On 14/08/2023 05:25, Henry Wang wrote:

There are two types of memory system architectures available for
Arm-based systems, namely the Virtual Memory System Architecture (VMSA)
and the Protected Memory System Architecture (PMSA). According to
ARM DDI 0487G.a, A VMSA provides a Memory Management Unit (MMU) that
controls address translation, access permissions, and memory attribute
determination and checking, for memory accesses made by the PE. And
refer to ARM DDI 0600A.c, the PMSA supports a unified memory protection
scheme where an Memory Protection Unit (MPU) manages instruction and
data access. Currently, Xen only suuports VMSA.


Typo: s/suuports/supports/



Introduce a Kconfig option CONFIG_MMU, which is currently default
set to y and unselectable because currently only VMSA is supported.


NIT: It would be worth to explicit mention that this will be used in 
follow-up patches. So one will wonder what's the goal of introducing an 
unused config.


Or it could have been merged in the first patch splitting the MMU code 
so we don't introduce a config without any use.




Suggested-by: Julien Grall 
Signed-off-by: Henry Wang 


Acked-by: Julien Grall 

Cheers,

--
Julien Grall

Re: [PATCH v5 01/13] xen/arm64: head.S: Introduce enable_{boot,secondary}_cpu_mm()

[Henry Wang]

Hi Julien,

> On Aug 21, 2023, at 16:33, Julien Grall  wrote:
> 
> Hi Henry,
> 
> On 14/08/2023 05:25, Henry Wang wrote:
>> From: Wei Chen 
>> At the moment, on MMU system, enable_mmu() will return to an
>> address in the 1:1 mapping, then each path is responsible to
>> switch to virtual runtime mapping. Then remove_identity_mapping()
>> is called on the boot CPU to remove all 1:1 mapping.
>> Since remove_identity_mapping() is not necessary on Non-MMU system,
>> and we also avoid creating empty function for Non-MMU system, trying
>> to keep only one codeflow in arm64/head.S, we move path switch and
>> remove_identity_mapping() in enable_mmu() on MMU system.
>> As the remove_identity_mapping should only be called for the boot
>> CPU only, so we introduce enable_boot_cpu_mm() for boot CPU and
>> enable_secondary_cpu_mm() for secondary CPUs in this patch.
>> Signed-off-by: Wei Chen 
>> Signed-off-by: Penny Zheng  > Signed-off-by: Henry Wang 
>> 
> 
> One remark below. With or without it addressed:
> 
> Reviewed-by: Julien Grall 

Thanks, I will take this tag with ...

> 
> [...]
> 
>> +/*
>> + * 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:
>> + *   lr : Virtual address to return to.
>> + *
>> + * Clobbers x0 - x5
>> + */
>> +enable_secondary_cpu_mm:
>> +mov   x5, lr
>> +
>> +load_paddr x0, init_ttbr
>> +ldr   x0, [x0]
>> +
>> +blenable_mmu
>> +mov   lr, x5
>> +
>> +/* Return to the virtual address requested by the caller. */
>> +ret
>> +ENDPROC(enable_secondary_cpu_mm)
> 
> NIT: enable_mmu() could directly return to the virtual address. This would 
> reduce the function to:
> 
> load_paddr x0, init_ttbr
> ldr   x0, [x0]
> 
> /* Return to the virtual address requested by the caller.
> b enable_mmu

…this fixed in v6 since I think there is likely to be a v6, and I think I also 
need
to address the commit message nit pointed out by Jan in the last patch.

Kind regards,
Henry

Re: [PATCH v5 01/13] xen/arm64: head.S: Introduce enable_{boot,secondary}_cpu_mm()

[Julien Grall]

Hi Henry,

On 14/08/2023 05:25, Henry Wang wrote:

From: Wei Chen 

At the moment, on MMU system, enable_mmu() will return to an
address in the 1:1 mapping, then each path is responsible to
switch to virtual runtime mapping. Then remove_identity_mapping()
is called on the boot CPU to remove all 1:1 mapping.

Since remove_identity_mapping() is not necessary on Non-MMU system,
and we also avoid creating empty function for Non-MMU system, trying
to keep only one codeflow in arm64/head.S, we move path switch and
remove_identity_mapping() in enable_mmu() on MMU system.

As the remove_identity_mapping should only be called for the boot
CPU only, so we introduce enable_boot_cpu_mm() for boot CPU and
enable_secondary_cpu_mm() for secondary CPUs in this patch.

Signed-off-by: Wei Chen 
Signed-off-by: Penny Zheng  > Signed-off-by: Henry Wang 



One remark below. With or without it addressed:

Reviewed-by: Julien Grall 

[...]


+/*
+ * 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:
+ *   lr : Virtual address to return to.
+ *
+ * Clobbers x0 - x5
+ */
+enable_secondary_cpu_mm:
+mov   x5, lr
+
+load_paddr x0, init_ttbr
+ldr   x0, [x0]
+
+blenable_mmu
+mov   lr, x5
+
+/* Return to the virtual address requested by the caller. */
+ret
+ENDPROC(enable_secondary_cpu_mm)


NIT: enable_mmu() could directly return to the virtual address. This 
would reduce the function to:


load_paddr x0, init_ttbr
ldr   x0, [x0]

/* Return to the virtual address requested by the caller.
b enable_mmu


+
+/*
+ * 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:
+ *   lr : Virtual address to return to.
+ *
+ * Clobbers x0 - x5
+ */
+enable_boot_cpu_mm:
+mov   x5, lr
+
+blcreate_page_tables
+load_paddr x0, boot_pgtable
+
+blenable_mmu
+
+/*
+ * The MMU is turned on and we are in the 1:1 mapping. Switch
+ * to the runtime mapping.
+ */
+ldr   x0, =1f
+brx0
+1:
+mov   lr, x5
+/*
+ * The 1:1 map may clash with other parts of the Xen virtual memory
+ * layout. As it is not used anymore, remove it completely to avoid
+ * having to worry about replacing existing mapping afterwards.
+ * Function will return to the virtual address requested by the caller.
+ */
+b remove_identity_mapping
+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


Cheers,

--
Julien Grall

[PATCH v6] tools/xenstore: move xenstored sources into dedicated directory

[Juergen Gross]

In tools/xenstore there are living xenstored and xenstore clients.
They are no longer sharing anything apart from the "xenstore" in their
names.

Move the xenstored sources into a new directory tools/xenstored while
dropping the "xenstored_" prefix from their names. This will make it
clearer that xenstore clients and xenstored are independent from each
other.

In order to avoid two very similar named directories below tools,
rename tools/xenstore to tools/xs-clients.

Signed-off-by: Juergen Gross 
Reviewed-by: Julien Grall 
---
After the large overhaul of xenstored I think such a reorg would make
sense to go into the same Xen version. Delaying it until the next
version would make potential backports for 4.18 harder.
V4:
- new patch
V5:
- rename xenstore directory to xs-clients (Julien Grall)
V6:
- use local .gitignore files (Andrew Cooper)
---
 .gitignore| 11 -
 MAINTAINERS   |  3 +-
 stubdom/Makefile  |  4 +-
 tools/Makefile|  3 +-
 tools/xenstored/.gitignore|  1 +
 tools/xenstored/Makefile  | 48 +++
 tools/{xenstore => xenstored}/Makefile.common | 13 +++--
 tools/{xenstore => xenstored}/README  |  0
 .../control.c}|  8 ++--
 .../control.h}|  0
 .../xenstored_core.c => xenstored/core.c} | 14 +++---
 .../xenstored_core.h => xenstored/core.h} |  0
 .../xenstored_domain.c => xenstored/domain.c} | 10 ++--
 .../xenstored_domain.h => xenstored/domain.h} |  0
 tools/{xenstore => xenstored}/hashtable.c |  0
 tools/{xenstore => xenstored}/hashtable.h |  0
 tools/{xenstore => xenstored}/list.h  |  0
 .../xenstored_lu.c => xenstored/lu.c} |  8 ++--
 .../xenstored_lu.h => xenstored/lu.h} |  0
 .../lu_daemon.c}  |  4 +-
 .../lu_minios.c}  |  2 +-
 .../xenstored_minios.c => xenstored/minios.c} |  2 +-
 .../xenstored_osdep.h => xenstored/osdep.h}   |  0
 .../xenstored_posix.c => xenstored/posix.c}   |  4 +-
 tools/{xenstore => xenstored}/talloc.c|  0
 tools/{xenstore => xenstored}/talloc.h|  0
 .../{xenstore => xenstored}/talloc_guide.txt  |  0
 .../transaction.c}|  6 +--
 .../transaction.h}|  2 +-
 tools/{xenstore => xenstored}/utils.c |  0
 tools/{xenstore => xenstored}/utils.h |  0
 .../xenstored_watch.c => xenstored/watch.c}   |  6 +--
 .../xenstored_watch.h => xenstored/watch.h}   |  2 +-
 .../include => xenstored}/xenstore_state.h|  0
 tools/xs-clients/.gitignore   | 10 
 tools/{xenstore => xs-clients}/Makefile   | 30 ++--
 .../xenstore_client.c |  0
 .../xenstore_control.c|  0
 38 files changed, 110 insertions(+), 81 deletions(-)
 create mode 100644 tools/xenstored/.gitignore
 create mode 100644 tools/xenstored/Makefile
 rename tools/{xenstore => xenstored}/Makefile.common (50%)
 rename tools/{xenstore => xenstored}/README (100%)
 rename tools/{xenstore/xenstored_control.c => xenstored/control.c} (98%)
 rename tools/{xenstore/xenstored_control.h => xenstored/control.h} (100%)
 rename tools/{xenstore/xenstored_core.c => xenstored/core.c} (99%)
 rename tools/{xenstore/xenstored_core.h => xenstored/core.h} (100%)
 rename tools/{xenstore/xenstored_domain.c => xenstored/domain.c} (99%)
 rename tools/{xenstore/xenstored_domain.h => xenstored/domain.h} (100%)
 rename tools/{xenstore => xenstored}/hashtable.c (100%)
 rename tools/{xenstore => xenstored}/hashtable.h (100%)
 rename tools/{xenstore => xenstored}/list.h (100%)
 rename tools/{xenstore/xenstored_lu.c => xenstored/lu.c} (98%)
 rename tools/{xenstore/xenstored_lu.h => xenstored/lu.h} (100%)
 rename tools/{xenstore/xenstored_lu_daemon.c => xenstored/lu_daemon.c} (97%)
 rename tools/{xenstore/xenstored_lu_minios.c => xenstored/lu_minios.c} (98%)
 rename tools/{xenstore/xenstored_minios.c => xenstored/minios.c} (97%)
 rename tools/{xenstore/xenstored_osdep.h => xenstored/osdep.h} (100%)
 rename tools/{xenstore/xenstored_posix.c => xenstored/posix.c} (98%)
 rename tools/{xenstore => xenstored}/talloc.c (100%)
 rename tools/{xenstore => xenstored}/talloc.h (100%)
 rename tools/{xenstore => xenstored}/talloc_guide.txt (100%)
 rename tools/{xenstore/xenstored_transaction.c => xenstored/transaction.c} 
(99%)
 rename tools/{xenstore/xenstored_transaction.h => xenstored/transaction.h} 
(98%)
 rename tools/{xenstore => xenstored}/utils.c (100%)
 rename tools/{xenstore => xenstored}/utils.h (100%)
 rename tools/{xenstore/xenstored_watch.c => xenstored/watch.c} (98%)
 rename tools/{xenstore/xenstored_watch.h => xenstored/watch.h} (98%)
 rename tools/{xenstore/include => xenstored}/xenstore_state.h (100%)
 create mode 100644 tools/xs-clients/.gitignore
 

Re: [PATCH v5] tools/xenstore: move xenstored sources into dedicated directory

[Juergen Gross]

On 21.08.23 09:59, Julien Grall wrote:

Hi,

On 18/08/2023 15:20, Andrew Cooper wrote:

On 18/08/2023 3:08 pm, Juergen Gross wrote:

diff --git a/.gitignore b/.gitignore
index c1b73b0968..c6489c4e70 100644
--- a/.gitignore
+++ b/.gitignore
@@ -237,22 +237,22 @@ tools/xenmon/xentrace_setmask
  tools/xenmon/xenbaked
  tools/xenpaging/xenpaging
  tools/xenpmd/xenpmd
-tools/xenstore/xenstore
-tools/xenstore/xenstore-chmod
-tools/xenstore/xenstore-control
-tools/xenstore/xenstore-exists
-tools/xenstore/xenstore-list
-tools/xenstore/xenstore-ls
-tools/xenstore/xenstore-read
-tools/xenstore/xenstore-rm
-tools/xenstore/xenstore-watch
-tools/xenstore/xenstore-write
-tools/xenstore/xenstored
+tools/xenstored/xenstored
  tools/xentop/xentop
  tools/xentrace/xentrace_setsize
  tools/xentrace/tbctl
  tools/xentrace/xenctx
  tools/xentrace/xentrace
+tools/xs-clients/xenstore
+tools/xs-clients/xenstore-chmod
+tools/xs-clients/xenstore-control
+tools/xs-clients/xenstore-exists
+tools/xs-clients/xenstore-list
+tools/xs-clients/xenstore-ls
+tools/xs-clients/xenstore-read
+tools/xs-clients/xenstore-rm
+tools/xs-clients/xenstore-watch
+tools/xs-clients/xenstore-write
  xen/**/*.i
  xen/**/*.s
  xen/.banner


Please take the opportunity to move these into local .gitignore files.
One less area of churn in future renaming.  Probably can be fixed on
commit as its only mechanical.
If you end up to the be committer then sure. I would prefer not doing it while 
committing.


I'll send a V6.


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [XEN PATCH 2/2] automation/eclair: avoid unintentional ECLAIR analysis

[Simone Ballarin]

On 14/08/2023 23:52, Stefano Stabellini wrote:

On Mon, 14 Aug 2023, Stefano Stabellini wrote:

On Sat, 12 Aug 2023, Simone Ballarin wrote:
> On 12/08/2023 00:04, Stefano Stabellini wrote:
> > On Fri, 11 Aug 2023, Simone Ballarin wrote:
> > > With this patch, ECLAIR jobs will need to be manually
> > > started for "people/.*" pipelines.
> > >
> > > This avoids occupying the runner on analyzes that might
> > > not be used by developers.
> > >
> > > If developers want to analyze their own repositories
> > > they need to launch them from GitLab.
> > >
> > > Signed-off-by: Simone Ballarin 
> > > ---
> > >  automation/gitlab-ci/analyze.yaml | 14 ++
> > >  automation/gitlab-ci/build.yaml   |  6 --
> > >  automation/gitlab-ci/test.yaml|  5 -
> > >  3 files changed, 10 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/automation/gitlab-ci/analyze.yaml
> > > b/automation/gitlab-ci/analyze.yaml
> > > index 4aa4abe2ee..f04ff99093 100644
> > > --- a/automation/gitlab-ci/analyze.yaml
> > > +++ b/automation/gitlab-ci/analyze.yaml
> > > @@ -20,21 +20,27 @@
> > >codequality: gl-code-quality-report.json
> > >needs: []
> > >
> > > -eclair-x86_64:
> > > +.eclair-analysis:triggered:
> > >extends: .eclair-analysis
> > > +  allow_failure: true
> > > +  rules:
> > > +- if: $CI_PROJECT_PATH =~ /^xen-project\/people\/.*$/
> > > +  when: manual
> > > +- when: always
> > > +
> > > +eclair-x86_64:
> > > +  extends: .eclair-analysis:triggered
> > >variables:
> > >  LOGFILE: "eclair-x86_64.log"
> > >  VARIANT: "X86_64"
> > >  RULESET: "Set1"
> > > -  allow_failure: true
> > >
> > >  eclair-ARM64:
> > > -  extends: .eclair-analysis
> > > +  extends: .eclair-analysis:triggered
> > >variables:
> > >  LOGFILE: "eclair-ARM64.log"
> > >  VARIANT: "ARM64"
> > >  RULESET: "Set1"
> > > -  allow_failure: true
> > >
> > >  .eclair-analysis:on-schedule:
> > >extends: .eclair-analysis
> >
> > Everything so far looks great and I am ready to Ack.
> >
> >
> > > diff --git a/automation/gitlab-ci/build.yaml
> > > b/automation/gitlab-ci/build.yaml
> > > index 173613567c..e4b601943c 100644
> > > --- a/automation/gitlab-ci/build.yaml
> > > +++ b/automation/gitlab-ci/build.yaml
> > > @@ -12,11 +12,6 @@
> > >- '*/*.log'
> > >  when: always
> > >needs: []
> > > -  except:
> > > -- master
> > > -- smoke
> > > -- /^coverity-tested\/.*/
> > > -- /^stable-.*/
> > >
> > >  .gcc-tmpl:
> > >variables: 
> > > @@ -269,7 +264,6 @@
> > >  .test-jobs-artifact-common:
> > >stage: build
> > >needs: []
> > > -  except: !reference [.test-jobs-common, except]
> > >
> > >  # Arm test artifacts
> > >
> > > diff --git a/automation/gitlab-ci/test.yaml
> > > b/automation/gitlab-ci/test.yaml
> > > index 8ccce1fe26..11cb97ea4b 100644
> > > --- a/automation/gitlab-ci/test.yaml
> > > +++ b/automation/gitlab-ci/test.yaml
> > > @@ -1,11 +1,6 @@
> > >  .test-jobs-common:
> > >stage: test
> > >image: registry.gitlab.com/xen-project/xen/${CONTAINER}
> > > -  except:
> > > -- master
> > > -- smoke
> > > -- /^coverity-tested\/.*/
> > > -- /^stable-.*/
> > >
> > >  .arm64-test-needs: 
> > >- alpine-3.18-arm64-rootfs-export
> >
> > These changes instead belongs to the first patch, right?
>
> Yes, sorry. Moreover, it is the answer to your question on
> the other patch.

No problem. Please re-send. One more comment. In the first patch we
have:

+workflow:
+  rules:
+- if: $CI_COMMIT_BRANCH =~ 
/^(master|smoke|^coverity-tested\/.*|stable-.*)$/

+  when: never
+- when: always

I think it is an excellent idea to skip "master" as well as the 
others,

but maybe we could make them "manual" instead of "never" just in case
someone wants to see the results on master. It might happen on special
occasions.



"when:manual" cannot be used at workflow-level, but only at job-level.
I've made a couple of experiments and I found several problems in doing 
that

related to the clauses "allow_failure" and "needs".



One more thing: today the Eclair jobs always start even if WTOKEN is 
not
defined (hence the jobs cannot actually succeed). Would it make sense 
to
also add a check on the existence of WTOKEN? If WTOKEN exists, start 
the

Eclair jobs, otherwise do not ?


No problem.

--
Simone Ballarin, M.Sc.

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

Re: [PATCH v5] tools/xenstore: move xenstored sources into dedicated directory

[Julien Grall]

Hi Juergen,

On 18/08/2023 15:08, Juergen Gross wrote:

In tools/xenstore there are living xenstored and xenstore clients.
They are no longer sharing anything apart from the "xenstore" in their
names.

Move the xenstored sources into a new directory tools/xenstored while
dropping the "xenstored_" prefix from their names. This will make it
clearer that xenstore clients and xenstored are independent from each
other.

In order to avoid two very similar named directories below tools,
rename tools/xenstore to tools/xs-clients.

Signed-off-by: Juergen Gross 


Reviewed-by: Julien Grall 

Cheers,

--
Julien Grall

Re: [PATCH v5] tools/xenstore: move xenstored sources into dedicated directory

[Julien Grall]

Hi,

On 18/08/2023 15:20, Andrew Cooper wrote:

On 18/08/2023 3:08 pm, Juergen Gross wrote:

diff --git a/.gitignore b/.gitignore
index c1b73b0968..c6489c4e70 100644
--- a/.gitignore
+++ b/.gitignore
@@ -237,22 +237,22 @@ tools/xenmon/xentrace_setmask
  tools/xenmon/xenbaked
  tools/xenpaging/xenpaging
  tools/xenpmd/xenpmd
-tools/xenstore/xenstore
-tools/xenstore/xenstore-chmod
-tools/xenstore/xenstore-control
-tools/xenstore/xenstore-exists
-tools/xenstore/xenstore-list
-tools/xenstore/xenstore-ls
-tools/xenstore/xenstore-read
-tools/xenstore/xenstore-rm
-tools/xenstore/xenstore-watch
-tools/xenstore/xenstore-write
-tools/xenstore/xenstored
+tools/xenstored/xenstored
  tools/xentop/xentop
  tools/xentrace/xentrace_setsize
  tools/xentrace/tbctl
  tools/xentrace/xenctx
  tools/xentrace/xentrace
+tools/xs-clients/xenstore
+tools/xs-clients/xenstore-chmod
+tools/xs-clients/xenstore-control
+tools/xs-clients/xenstore-exists
+tools/xs-clients/xenstore-list
+tools/xs-clients/xenstore-ls
+tools/xs-clients/xenstore-read
+tools/xs-clients/xenstore-rm
+tools/xs-clients/xenstore-watch
+tools/xs-clients/xenstore-write
  xen/**/*.i
  xen/**/*.s
  xen/.banner


Please take the opportunity to move these into local .gitignore files.
One less area of churn in future renaming.  Probably can be fixed on
commit as its only mechanical.
If you end up to the be committer then sure. I would prefer not doing it 
while committing.


Cheers,

--
Julien Grall

Re: [XEN PATCH 2/2] automation/eclair: avoid unintentional ECLAIR analysis

[Simone Ballarin]

On 14/08/2023 23:45, Stefano Stabellini wrote:

On Sat, 12 Aug 2023, Simone Ballarin wrote:

On 12/08/2023 00:04, Stefano Stabellini wrote:
> On Fri, 11 Aug 2023, Simone Ballarin wrote:
> > With this patch, ECLAIR jobs will need to be manually
> > started for "people/.*" pipelines.
> >
> > This avoids occupying the runner on analyzes that might
> > not be used by developers.
> >
> > If developers want to analyze their own repositories
> > they need to launch them from GitLab.
> >
> > Signed-off-by: Simone Ballarin 
> > ---
> >  automation/gitlab-ci/analyze.yaml | 14 ++
> >  automation/gitlab-ci/build.yaml   |  6 --
> >  automation/gitlab-ci/test.yaml|  5 -
> >  3 files changed, 10 insertions(+), 15 deletions(-)
> >
> > diff --git a/automation/gitlab-ci/analyze.yaml
> > b/automation/gitlab-ci/analyze.yaml
> > index 4aa4abe2ee..f04ff99093 100644
> > --- a/automation/gitlab-ci/analyze.yaml
> > +++ b/automation/gitlab-ci/analyze.yaml
> > @@ -20,21 +20,27 @@
> >codequality: gl-code-quality-report.json
> >needs: []
> >
> > -eclair-x86_64:
> > +.eclair-analysis:triggered:
> >extends: .eclair-analysis
> > +  allow_failure: true
> > +  rules:
> > +- if: $CI_PROJECT_PATH =~ /^xen-project\/people\/.*$/
> > +  when: manual
> > +- when: always
> > +
> > +eclair-x86_64:
> > +  extends: .eclair-analysis:triggered
> >variables:
> >  LOGFILE: "eclair-x86_64.log"
> >  VARIANT: "X86_64"
> >  RULESET: "Set1"
> > -  allow_failure: true
> >
> >  eclair-ARM64:
> > -  extends: .eclair-analysis
> > +  extends: .eclair-analysis:triggered
> >variables:
> >  LOGFILE: "eclair-ARM64.log"
> >  VARIANT: "ARM64"
> >  RULESET: "Set1"
> > -  allow_failure: true
> >
> >  .eclair-analysis:on-schedule:
> >extends: .eclair-analysis
>
> Everything so far looks great and I am ready to Ack.
>
>
> > diff --git a/automation/gitlab-ci/build.yaml
> > b/automation/gitlab-ci/build.yaml
> > index 173613567c..e4b601943c 100644
> > --- a/automation/gitlab-ci/build.yaml
> > +++ b/automation/gitlab-ci/build.yaml
> > @@ -12,11 +12,6 @@
> >- '*/*.log'
> >  when: always
> >needs: []
> > -  except:
> > -- master
> > -- smoke
> > -- /^coverity-tested\/.*/
> > -- /^stable-.*/
> >
> >  .gcc-tmpl:
> >variables: 
> > @@ -269,7 +264,6 @@
> >  .test-jobs-artifact-common:
> >stage: build
> >needs: []
> > -  except: !reference [.test-jobs-common, except]
> >
> >  # Arm test artifacts
> >
> > diff --git a/automation/gitlab-ci/test.yaml
> > b/automation/gitlab-ci/test.yaml
> > index 8ccce1fe26..11cb97ea4b 100644
> > --- a/automation/gitlab-ci/test.yaml
> > +++ b/automation/gitlab-ci/test.yaml
> > @@ -1,11 +1,6 @@
> >  .test-jobs-common:
> >stage: test
> >image: registry.gitlab.com/xen-project/xen/${CONTAINER}
> > -  except:
> > -- master
> > -- smoke
> > -- /^coverity-tested\/.*/
> > -- /^stable-.*/
> >
> >  .arm64-test-needs: 
> >- alpine-3.18-arm64-rootfs-export
>
> These changes instead belongs to the first patch, right?

Yes, sorry. Moreover, it is the answer to your question on
the other patch.


No problem. Please re-send. One more comment. In the first patch we
have:

+workflow:
+  rules:
+- if: $CI_COMMIT_BRANCH =~
/^(master|smoke|^coverity-tested\/.*|stable-.*)$/
+  when: never
+- when: always

I think it is an excellent idea to skip "master" as well as the others,
but maybe we could make them "manual" instead of "never" just in case
someone wants to see the results on master. It might happen on special
occasions.


"manual" cannot be used at workflow-level only at job-level.
I've made a couple of experiments and I find several problems in doing 
that:



--
Simone Ballarin, M.Sc.

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

Re: [PATCH] docs/misra: document gcc-specific behavior with shifting signed integers

[Jan Beulich]

On 19.08.2023 02:33, Stefano Stabellini wrote:
> From: Stefano Stabellini 
> 
> Signed-off-by: Stefano Stabellini 
> ---
> Changes in v2:
> - use "shift" instead of << or >>
> - use All Architectures (I haven't changed all the other instances of
> x86/arm in the file yet)
> ---
>  docs/misra/C-language-toolchain.rst | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/docs/misra/C-language-toolchain.rst 
> b/docs/misra/C-language-toolchain.rst
> index 785aed1eaf..f5ca7bd2c8 100644
> --- a/docs/misra/C-language-toolchain.rst
> +++ b/docs/misra/C-language-toolchain.rst
> @@ -200,6 +200,12 @@ The table columns are as follows:
>   - ARM64, X86_64
>   - See Section "6.29 Designated Initializers" of GCC_MANUAL
>  
> +   * - Signed shift acts on negative numbers by sign extension
> + - All architectures
> + - See Section "4.5 Integers" of GCC_MANUAL. As an extension to the
> +   C language, GCC does not use the latitude given in C99 and C11
> +   only to treat certain aspects of signed shift as undefined.

I'm sorry, but that's still not what the doc says. Replacing << and >> by
"shifts" was imo wrong. What's needed instead is that either this is split
into two top-level bullet points (one for << and one for >>), or the first
sub-bullet-point (which acts as kind of the title) be generalized, with
the << and >> details fully moved to the "explanatory" sub-bullet-point.

Jan

Re: [PATCH] docs/misra: add exceptions to rules

[Jan Beulich]

On 19.08.2023 03:24, Stefano Stabellini wrote:
> From: Stefano Stabellini 
> 
> During the discussions that led to the acceptable of the Rules, we
> decided on a few exceptions that were not properly recorded in
> rules.rst. Other times, the exceptions were decided later when it came
> to enabling a rule in ECLAIR.

In a number of cases I'm unaware of such decisions. May be worth splitting
the patch into a controversial and an uncontroversial part, such that the
latter can go in while we discuss the former.

> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -59,7 +59,8 @@ maintainers if you want to suggest a change.
>   - Required
>   - Precautions shall be taken in order to prevent the contents of a
> header file being included more than once
> - -
> + - Files that are intended to be included more than once do not need to
> +   conform to the directive (e.g. autogenerated or empty header files)

Auto-generated isn't a reason for an exception here. The logic generating
the header can very well be adjusted. Same for empty headers - there's no
reason they couldn't gain guards. An exception is needed for headers which
we deliberately include more than once, in order to have a single central
place for attributes, enumerations, and alike.

> @@ -106,7 +107,23 @@ maintainers if you want to suggest a change.
> * - `Rule 2.1 
> `_
>   - Required
>   - A project shall not contain unreachable code
> - -
> + - The following are allowed:
> + - Invariantly constant conditions (e.g. while(0) { S; })

When (and why) was this decided? The example given looks exactly like what
Misra wants us to not have.

> + - Switch with a controlling value incompatible with labeled
> +   statements

What does this mean?

> + - Functions that are intended to be never referenced from C
> +   code, or are referenced in builds not under analysis (e.g.
> +   'do_trap_fiq' for the former and 'check_for_unexpected_msi'
> +   for the latter)

I agree with the "not referenced from C", but I don't see why the other
kind couldn't be properly addressed.

> + - Unreachability caused by the following macros/functions is
> +   deliberate: BUG, assert_failed, ERROR_EXIT, ERROR_EXIT_DOM,
> +   PIN_FAIL, __builtin_unreachable, panic, do_unexpected_trap,
> +   machine_halt, machine_restart, machine_reboot,
> +   ASSERT_UNREACHABLE

Base infrastructure items like BUG() are imo fine to mention here. But
specific items shouldn't be; the more we mention here, the more we invite
the list to be grown. Note also how you mention items which no longer
exist (ERROR_EXIT{,_DOM}, PIN_FAIL).

> + - asm-offsets.c, as they are not linked deliberately, because
> +   they are used to generate definitions for asm modules
> + - pure declarations (i.e. declarations without
> +   initialization) are safe, as they are not executed

I don't think "pure declarations" is a term used in the spec. Let's better
call it the way it is called elsewhere - declarations without initializer
(where, as mentioned before, the term "unreachable code" is questionable
anyway).

> @@ -167,7 +184,7 @@ maintainers if you want to suggest a change.
> * - `Rule 5.6 
> `_
>   - Required
>   - A typedef name shall be a unique identifier
> - -
> + - BOOLEAN, UINT{8,32,64} and INT{8,32,64} are allowed

I think this permission needs to be limited as much as possible.

> @@ -183,7 +200,10 @@ maintainers if you want to suggest a change.
> * - `Rule 7.1 
> `_
>   - Required
>   - Octal constants shall not be used
> - -
> + - Usage of the following constants is safe, since they are given
> +   as-is in the inflate algorithm specification and there is
> +   therefore no risk of them being interpreted as decimal constants:
> +   ^0(007|37|070|213|236|300|321|330|331|332|333|334|335|337|371)$

This is a very odd set of exceptions, which by stating them here you then
grant to be exercised everywhere. Once again I don't think special cases
dealing with a single source or sub-component should be globally named.

> @@ -239,13 +259,16 @@ maintainers if you want to suggest a change.
>   - Required
>   - All declarations of an object or function shall use the same
> names and type qualifiers
> - -
> + - The type ret_t is deliberately used and defined as int or long
> +   depending on the architecture

That's not depending on the architecture, but depending on the type of
guest to service. I'd also suggest "may", not "is".

Jan

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

[Jan Beulich]

On 19.08.2023 02:28, Vikram Garhwal wrote:
> Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_locked().
> Remove static type so this can also be used by SMMU drivers to check if the
> device is being used before removing.
> 
> Moving spin_lock to caller was done to prevent the concurrent access to
> iommu_dt_device_is_assigned while doing add/remove/assign/deassign. Follow-up
> patches in this series introduces node add/remove feature.
> 
> Also, caller is required hold the correct lock so moved the function prototype
> to a private header.
> 
> Signed-off-by: Vikram Garhwal 
> 
> ---
> Changes from v7:
> Update commit message.
> Add ASSERT().
> ---
> ---
>  xen/drivers/passthrough/device_tree.c | 26 +
>  xen/include/xen/iommu-private.h   | 28 +++
>  2 files changed, 50 insertions(+), 4 deletions(-)
>  create mode 100644 xen/include/xen/iommu-private.h

I find it odd for new versions to be sent when discussion on the prior
version hasn't settled yet, and when you then also don't incorporate
the feedback given. I also find it odd to now be Cc-ed on the entire
series. Imo instead of that patches which aren't self-explanatory /
self-consistent simply need their descriptions improved (in the case
here to mention what further use of a function is intended). Yet
better would be to add declarations (and remove static) only at the
point that's actually needed. This then eliminates the risk of
subsequent changes in response to feedback (like Julien's here)
resulting in the declaration no longer being needed, thus leading to
a Misra violation (besides the general tidiness aspect).

Jan

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

[Jan Beulich]

On 18.08.2023 21:52, Vikram Garhwal wrote:
> On Thu, Aug 17, 2023 at 09:05:44AM +0200, Jan Beulich wrote:
>> On 17.08.2023 02:39, Vikram Garhwal wrote:
>>> --- /dev/null
>>> +++ b/xen/include/xen/iommu-private.h
>>
>> I don't think private headers should live in include/xen/. Judging from only
>> the patches I was Cc-ed on, ...
> Thank you for suggestion. Do you where can i place it then?

Irrespective of Julien's reply (potentially rendering this moot), see ...

> Please see another comment down regarding who might be using this function.
>>
>>> @@ -0,0 +1,28 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * xen/iommu-private.h
>>> + */
>>> +#ifndef __XEN_IOMMU_PRIVATE_H__
>>> +#define __XEN_IOMMU_PRIVATE_H__
>>> +
>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>> +#include 
>>> +
>>> +/*
>>> + * Checks if dt_device_node is assigned to a domain or not. This function
>>> + * expects to be called with dtdevs_lock acquired by caller.
>>> + */
>>> +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node 
>>> *dev);
>>> +#endif
>>
>> ... I don't even see the need for the declaration, as the function is used
>> only from the file also defining it. But of course if there is a use
>> elsewhere (in Arm-only code, as is suggested by the description here), then
>> the header (under a suitable name) wants to live under drivers/passthrough/

... my earlier reply.

Jan

>> (and of course be included only from anywhere in that sub-tree).
>>
> This is also use in smmu.c:arm_smmu_dt_remove_device_legacy(). This is added 
> in
> 12/19 patch(xen/smmu: Add remove_device callback for smmu_iommu ops).
> 
> I will make sure to cc you for all the patches in v9 series. I plan to send
> it today.
> 
>> Jan

Re: [XEN PATCH v11 00/14] Xen FF-A mediator

[Jens Wiklander]

Hi Julien,

On Fri, Aug 18, 2023 at 10:32 AM Julien Grall  wrote:
>
> Hi Jens,
>
> On 31/07/2023 13:15, Jens Wiklander wrote:
> > Jens Wiklander (14):
> >xen/arm: ffa: add direct request support
> >xen/arm: ffa: map SPMC rx/tx buffers
> >xen/arm: ffa: send guest events to Secure Partitions
> >xen/arm: ffa: support mapping guest RX/TX buffers
> >xen/arm: ffa: support guest FFA_PARTITION_INFO_GET
> >xen/arm: move regpair_to_uint64() and uint64_to_regpair() to regs.h
> >xen/arm: ffa: add defines for sharing memory
> >xen/arm: ffa: add ABI structs for sharing memory
> >xen/arm: ffa: support sharing memory
> >xen/arm: ffa: add support to reclaim shared memory
> >xen/arm: ffa: improve lock granularity
> >xen/arm: ffa: list current limitations
> >tools: add Arm FF-A mediator
> >docs: add Arm FF-A mediator
>
> The series is now committed. Note, I had to resolve a context conflict
> in the CHANGELOG.md.

Thanks for the update, and a big thank you to all of you who have
helped me to get this into a committable state.

Cheers,
Jens

>
> Cheers,
>
> --
> Julien Grall