[linux-6.1 test] 184734: tolerable FAIL - PUSHED

[osstest service owner]

flight 184734 linux-6.1 real [real]
flight 184747 linux-6.1 real-retest [real]
http://logs.test-lab.xenproject.org/osstest/logs/184734/
http://logs.test-lab.xenproject.org/osstest/logs/184747/

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 12 debian-hvm-install fail 
pass in 184747-retest

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

version targeted for testing:
 linux81e1dc2f70014b9523dd02ca763788e4f81e5bac
baseline version:
 linux8b4118fabd6eb75fed19483b04dab3a036886489

Last test of basis   184687  2024-02-16 18:13:56 Z7 days
Testing same since   184734  2024-02-23 08:41:05 Z0 days1 attempts


People who touched revisions under test:
  "Christian A. Ehrhardt" 
  Aaron Conole 
  Aleksander Mazur 
  Alex Deucher 
  Alexander Stein 
  Alexandra Winter 
  Alexandru Gagniuc 
  Alexey Khoroshilov 
  Allen Pais 
  Andi Shyti 
  Andrei Vagin 
  Andrejs Cainikovs 
  Andrew Morton 
  Andy Chi 
  Arnd Bergmann 
  Audra Mitchell 
  Bagas Sanjaya 
  Baokun Li 
  Bibo Mao 
  Bjorn Andersson 
  bo liu 
  Boris Burkov 
  Borislav Petkov (AMD) 
  Borislav Petkov 
  Brad Spengler 
  

Re: [PATCH v1] automation: remove bin86/dev86 from tumbleweed image

[Olaf Hering]

Fri, 23 Feb 2024 15:22:38 -0800 (PST) Stefano Stabellini 
:

> Do you have a successful gitlab pipeline with this patch applied that
> you can give me as proof of testing and success?

Yes, all of them since the patch went out.


Olaf


pgpzXM2EomBWs.pgp
Description: Digitale Signatur von OpenPGP

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

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 15 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-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  92babc88f67ed0ef3dc575a8b9534040274678ee
baseline version:
 xen  f5e1c527d0a0d09ca0cb1dcd8d4ab4a1a5261e91

Last test of basis   184739  2024-02-23 18:00:28 Z0 days
Testing same since   184745  2024-02-23 23:03:51 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 

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
   f5e1c527d0..92babc88f6  92babc88f67ed0ef3dc575a8b9534040274678ee -> smoke

[PATCH v2 bpf-next 3/3] mm: Introduce VM_SPARSE kind and vm_area_[un]map_pages().

[Alexei Starovoitov]

From: Alexei Starovoitov 

vmap/vmalloc APIs are used to map a set of pages into contiguous kernel virtual 
space.

get_vm_area() with appropriate flag is used to request an area of kernel 
address range.
It'se used for vmalloc, vmap, ioremap, xen use cases.
- vmalloc use case dominates the usage. Such vm areas have VM_ALLOC flag.
- the areas created by vmap() function should be tagged with VM_MAP.
- ioremap areas are tagged with VM_IOREMAP.
- xen use cases are VM_XEN.

BPF would like to extend the vmap API to implement a lazily-populated
sparse, yet contiguous kernel virtual space.
Introduce VM_SPARSE vm_area flag and
vm_area_map_pages(area, start_addr, count, pages) API to map a set
of pages within a given area.
It has the same sanity checks as vmap() does.
It also checks that get_vm_area() was created with VM_SPARSE flag
which identifies such areas in /proc/vmallocinfo
and returns zero pages on read through /proc/kcore.

The next commits will introduce bpf_arena which is a sparsely populated shared
memory region between bpf program and user space process. It will map
privately-managed pages into a sparse vm area with the following steps:

  area = get_vm_area(area_size, VM_SPARSE);  // at bpf prog verification time
  vm_area_map_pages(area, kaddr, 1, page);   // on demand
// it will return an error if kaddr is out of range
  vm_area_unmap_pages(area, kaddr, 1);
  free_vm_area(area);// after bpf prog is unloaded

Signed-off-by: Alexei Starovoitov 
---
 include/linux/vmalloc.h |  4 +++
 mm/vmalloc.c| 55 +++--
 2 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index 223e51c243bc..416bc7b0b4db 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -29,6 +29,7 @@ struct iov_iter;  /* in uio.h */
 #define VM_MAP_PUT_PAGES   0x0200  /* put pages and free array in 
vfree */
 #define VM_ALLOW_HUGE_VMAP 0x0400  /* Allow for huge pages on 
archs with HAVE_ARCH_HUGE_VMALLOC */
 #define VM_XEN 0x0800  /* xen use cases */
+#define VM_SPARSE  0x1000  /* sparse vm_area. not all 
pages are present. */
 
 #if (defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)) && \
!defined(CONFIG_KASAN_VMALLOC)
@@ -233,6 +234,9 @@ static inline bool is_vm_area_hugepages(const void *addr)
 }
 
 #ifdef CONFIG_MMU
+int vm_area_map_pages(struct vm_struct *area, unsigned long addr, unsigned int 
count,
+ struct page **pages);
+int vm_area_unmap_pages(struct vm_struct *area, unsigned long addr, unsigned 
int count);
 void vunmap_range(unsigned long addr, unsigned long end);
 static inline void set_vm_flush_reset_perms(void *addr)
 {
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index d769a65bddad..a05dfbbacb78 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -648,6 +648,54 @@ static int vmap_pages_range(unsigned long addr, unsigned 
long end,
return err;
 }
 
+/**
+ * vm_area_map_pages - map pages inside given vm_area
+ * @area: vm_area
+ * @addr: start address inside vm_area
+ * @count: number of pages
+ * @pages: pages to map (always PAGE_SIZE pages)
+ */
+int vm_area_map_pages(struct vm_struct *area, unsigned long addr, unsigned int 
count,
+ struct page **pages)
+{
+   unsigned long size = ((unsigned long)count) * PAGE_SIZE;
+   unsigned long end = addr + size;
+
+   might_sleep();
+   if (WARN_ON_ONCE(area->flags & VM_FLUSH_RESET_PERMS))
+   return -EINVAL;
+   if (WARN_ON_ONCE(area->flags & VM_NO_GUARD))
+   return -EINVAL;
+   if (WARN_ON_ONCE(!(area->flags & VM_SPARSE)))
+   return -EINVAL;
+   if (count > totalram_pages())
+   return -E2BIG;
+   if (addr < (unsigned long)area->addr || (void *)end > area->addr + 
area->size)
+   return -ERANGE;
+
+   return vmap_pages_range(addr, end, PAGE_KERNEL, pages, PAGE_SHIFT);
+}
+
+/**
+ * vm_area_unmap_pages - unmap pages inside given vm_area
+ * @area: vm_area
+ * @addr: start address inside vm_area
+ * @count: number of pages to unmap
+ */
+int vm_area_unmap_pages(struct vm_struct *area, unsigned long addr, unsigned 
int count)
+{
+   unsigned long size = ((unsigned long)count) * PAGE_SIZE;
+   unsigned long end = addr + size;
+
+   if (WARN_ON_ONCE(!(area->flags & VM_SPARSE)))
+   return -EINVAL;
+   if (addr < (unsigned long)area->addr || (void *)end > area->addr + 
area->size)
+   return -ERANGE;
+
+   vunmap_range(addr, end);
+   return 0;
+}
+
 int is_vmalloc_or_module_addr(const void *x)
 {
/*
@@ -3822,9 +3870,9 @@ long vread_iter(struct iov_iter *iter, const char *addr, 
size_t count)
 
if (flags & VMAP_RAM)
copied = vmap_ram_vread_iter(iter, addr, n, flags);
-   else if 

[PATCH v2 bpf-next 1/3] mm: Enforce VM_IOREMAP flag and range in ioremap_page_range.

[Alexei Starovoitov]

From: Alexei Starovoitov 

There are various users of get_vm_area() + ioremap_page_range() APIs.
Enforce that get_vm_area() was requested as VM_IOREMAP type and range passed to
ioremap_page_range() matches created vm_area to avoid accidentally ioremap-ing
into wrong address range.

Signed-off-by: Alexei Starovoitov 
---
 mm/vmalloc.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index d12a17fc0c17..f42f98a127d5 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -307,8 +307,21 @@ static int vmap_range_noflush(unsigned long addr, unsigned 
long end,
 int ioremap_page_range(unsigned long addr, unsigned long end,
phys_addr_t phys_addr, pgprot_t prot)
 {
+   struct vm_struct *area;
int err;
 
+   area = find_vm_area((void *)addr);
+   if (!area || !(area->flags & VM_IOREMAP)) {
+   WARN_ONCE(1, "vm_area at addr %lx is not marked as 
VM_IOREMAP\n", addr);
+   return -EINVAL;
+   }
+   if (addr != (unsigned long)area->addr ||
+   (void *)end != area->addr + get_vm_area_size(area)) {
+   WARN_ONCE(1, "ioremap request [%lx,%lx) doesn't match vm_area 
[%lx, %lx)\n",
+ addr, end, (long)area->addr,
+ (long)area->addr + get_vm_area_size(area));
+   return -ERANGE;
+   }
err = vmap_range_noflush(addr, end, phys_addr, pgprot_nx(prot),
 ioremap_max_page_shift);
flush_cache_vmap(addr, end);
-- 
2.34.1

IMPORTANT - : Need help on USB port virtualization with Xen hypervisor

[GOURLOT Francois]

Dear All,

We send you a message few days ago.

We have major performance with XEN USB Drivers. We use your driver to load data 
in a device and we need USB3 High Speed USB protocole

Do you have a new issue of the HCI and NEC USB Xen Driver ?

We hope you will contact us quickly.

Best regards

Program Manager

Thales SIX GTS France

T: +33(0) 1 41 30 26 67
M: +33 (0) 6 85 23 12 30

4 Avenue des Louvresses
92622 Gennevilliers
France






De : LARRIEU Dominique 
Envoyé : jeudi 22 février 2024 13:58
À : xen-devel@lists.xenproject.org
Cc : WILLEMS Louis ; GRUO Nicolas 
; GOURLOT Francois 
; Kelly Choi 
Objet : Need help on USB port virtualization with Xen hypervisor
Critère de diffusion : Confidentiel

Dear all,

We are detecting several issues with USB port virtualization with the Xen 
hypervisor.
- We cannot do PCI passthrough of the PCI usb bus on a Windows 10 1607 64-bit 
virtual machine. The bad result is a Windows blue screen.
- When we use the passthrough functionality on a Windows 21H2 virtual machine, 
we notice that the speed of the USB port is not high speed but full speed on a 
USB 3.0 port
- We notice instabilities when using the nec-usb-xhci driver,  USB 2.0 keys are 
not recognized by the Windows virtual machine (incorrect descriptor)

We need your help to find a solution for these problems.

The Software used are :

-Debian 11 version 5.10.0-20

-Xen version 4.14

-Windows 10 1607 and 21H2 for virtual machines. Virtual Machine HVM

Thanks in advance for your help.

Best regards,




Dominique LARRIEU

Cyber Project Manager

Thales


4, avenue des Louvresses
92230 Gennevilliers, France

[cid:image001.png@01D7E1DE.77862BB0]

Retrouvez Thales sur les réseaux sociaux et sur 
www.thalesgroup.com

[PATCH v2 bpf-next 2/3] mm, xen: Separate xen use cases from ioremap.

[Alexei Starovoitov]

From: Alexei Starovoitov 

xen grant table and xenbus ring are not ioremap the way arch specific code is 
using it,
so let's add VM_XEN flag to separate them from VM_IOREMAP users.
xen will not and should not be calling ioremap_page_range() on that range.
/proc/vmallocinfo will print such region as "xen" instead of "ioremap" as well.

Signed-off-by: Alexei Starovoitov 
---
 arch/x86/xen/grant-table.c | 2 +-
 drivers/xen/xenbus/xenbus_client.c | 2 +-
 include/linux/vmalloc.h| 1 +
 mm/vmalloc.c   | 7 +--
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c
index 1e681bf62561..b816db0349c4 100644
--- a/arch/x86/xen/grant-table.c
+++ b/arch/x86/xen/grant-table.c
@@ -104,7 +104,7 @@ static int arch_gnttab_valloc(struct gnttab_vm_area *area, 
unsigned nr_frames)
area->ptes = kmalloc_array(nr_frames, sizeof(*area->ptes), GFP_KERNEL);
if (area->ptes == NULL)
return -ENOMEM;
-   area->area = get_vm_area(PAGE_SIZE * nr_frames, VM_IOREMAP);
+   area->area = get_vm_area(PAGE_SIZE * nr_frames, VM_XEN);
if (!area->area)
goto out_free_ptes;
if (apply_to_page_range(_mm, (unsigned long)area->area->addr,
diff --git a/drivers/xen/xenbus/xenbus_client.c 
b/drivers/xen/xenbus/xenbus_client.c
index 32835b4b9bc5..b9c81a2d578b 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -758,7 +758,7 @@ static int xenbus_map_ring_pv(struct xenbus_device *dev,
bool leaked = false;
int err = -ENOMEM;
 
-   area = get_vm_area(XEN_PAGE_SIZE * nr_grefs, VM_IOREMAP);
+   area = get_vm_area(XEN_PAGE_SIZE * nr_grefs, VM_XEN);
if (!area)
return -ENOMEM;
if (apply_to_page_range(_mm, (unsigned long)area->addr,
diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index c720be70c8dd..223e51c243bc 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -28,6 +28,7 @@ struct iov_iter;  /* in uio.h */
 #define VM_FLUSH_RESET_PERMS   0x0100  /* reset direct map and flush 
TLB on unmap, can't be freed in atomic context */
 #define VM_MAP_PUT_PAGES   0x0200  /* put pages and free array in 
vfree */
 #define VM_ALLOW_HUGE_VMAP 0x0400  /* Allow for huge pages on 
archs with HAVE_ARCH_HUGE_VMALLOC */
+#define VM_XEN 0x0800  /* xen use cases */
 
 #if (defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)) && \
!defined(CONFIG_KASAN_VMALLOC)
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index f42f98a127d5..d769a65bddad 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -3822,9 +3822,9 @@ long vread_iter(struct iov_iter *iter, const char *addr, 
size_t count)
 
if (flags & VMAP_RAM)
copied = vmap_ram_vread_iter(iter, addr, n, flags);
-   else if (!(vm && (vm->flags & VM_IOREMAP)))
+   else if (!(vm && (vm->flags & (VM_IOREMAP | VM_XEN
copied = aligned_vread_iter(iter, addr, n);
-   else /* IOREMAP area is treated as memory hole */
+   else /* IOREMAP|XEN area is treated as memory hole */
copied = zero_iter(iter, n);
 
addr += copied;
@@ -4415,6 +4415,9 @@ static int s_show(struct seq_file *m, void *p)
if (v->flags & VM_IOREMAP)
seq_puts(m, " ioremap");
 
+   if (v->flags & VM_XEN)
+   seq_puts(m, " xen");
+
if (v->flags & VM_ALLOC)
seq_puts(m, " vmalloc");
 
-- 
2.34.1

[PATCH v2 bpf-next 0/3] mm: Cleanup and identify various users of kernel virtual address space

[Alexei Starovoitov]

From: Alexei Starovoitov 

There are various users of kernel virtual address space: vmalloc, vmap, 
ioremap, xen.

- vmalloc use case dominates the usage. Such vm areas have VM_ALLOC flag
and these areas are treated differently by KASAN.

- the areas created by vmap() function should be tagged with VM_MAP
(as majority of the users do).

- ioremap areas are tagged with VM_IOREMAP and vm area start is aligned to size
of the area unlike vmalloc/vmap.

- there is also xen usage that is marked as VM_IOREMAP, but it doesn't
call ioremap_page_range() unlike all other VM_IOREMAP users.

To clean this up:
1. Enforce that ioremap_page_range() checks the range and VM_IOREMAP flag.
2. Introduce VM_XEN flag to separate xen us cases from ioremap.

In addition BPF would like to reserve regions of kernel virtual address
space and populate it lazily, similar to xen use cases.
For that reason, introduce VM_SPARSE flag and vm_area_[un]map_pages() helpers
to populate this sparse area.

In the end the /proc/vmallocinfo will show
"vmalloc"
"vmap"
"ioremap"
"xen"
"sparse"
categories for different kinds of address regions.

ioremap, xen, sparse will return zero when dumped through /proc/kcore

Alexei Starovoitov (3):
  mm: Enforce VM_IOREMAP flag and range in ioremap_page_range.
  mm, xen: Separate xen use cases from ioremap.
  mm: Introduce VM_SPARSE kind and vm_area_[un]map_pages().

 arch/x86/xen/grant-table.c |  2 +-
 drivers/xen/xenbus/xenbus_client.c |  2 +-
 include/linux/vmalloc.h|  5 +++
 mm/vmalloc.c   | 71 +-
 4 files changed, 76 insertions(+), 4 deletions(-)

-- 
2.34.1

Re: [PATCH v1] automation: remove bin86/dev86 from tumbleweed image

[Stefano Stabellini]

On Wed, 13 Dec 2023, Olaf Hering wrote:
> https://build.opensuse.org/request/show/1126240
> 
> Signed-off-by: Olaf Hering 

Hi Olaf,

Do you have a successful gitlab pipeline with this patch applied that
you can give me as proof of testing and success?


> ---
>  automation/build/suse/opensuse-tumbleweed.dockerfile | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/automation/build/suse/opensuse-tumbleweed.dockerfile 
> b/automation/build/suse/opensuse-tumbleweed.dockerfile
> index 38f6fda2ff..f00e03eda7 100644
> --- a/automation/build/suse/opensuse-tumbleweed.dockerfile
> +++ b/automation/build/suse/opensuse-tumbleweed.dockerfile
> @@ -11,13 +11,11 @@ RUN zypper ref && zypper dup -y --no-recommends
>  RUN zypper install -y --no-recommends \
>  acpica \
>  bc \
> -bin86 \
>  bison \
>  bzip2 \
>  checkpolicy \
>  clang \
>  cmake \
> -dev86 \
>  diffutils \
>  discount \
>  flex \
> 

Re: [PATCH v2 2/6] xen/ppc: address violations of MISRA C:2012 Rule 11.8.

[Stefano Stabellini]

Shawn,

I am thinking of committing this, if you disagree please say something
in the next couple of days


On Tue, 19 Dec 2023, Simone Ballarin wrote:
> From: Maria Celeste Cesario 
> 
> The xen sources contain violations of MISRA C:2012 Rule 11.8 whose
> headline states:
> "A conversion shall not remove any const, volatile or _Atomic qualification
> from the type pointed to by a pointer".
> 
> Fix violation by adding missing const qualifier in cast.
> 
> Signed-off-by: Maria Celeste Cesario  
> Signed-off-by: Simone Ballarin  
> Reviewed-by: Stefano Stabellini 
> ---
> Adaptation requested by the community to make the code more consistent.
> ---
>  xen/arch/ppc/include/asm/atomic.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/ppc/include/asm/atomic.h 
> b/xen/arch/ppc/include/asm/atomic.h
> index 64168aa3f1..fe778579fb 100644
> --- a/xen/arch/ppc/include/asm/atomic.h
> +++ b/xen/arch/ppc/include/asm/atomic.h
> @@ -16,7 +16,7 @@
>  
>  static inline int atomic_read(const atomic_t *v)
>  {
> -return *(volatile int *)>counter;
> +return *(const volatile int *)>counter;
>  }
>  
>  static inline int _atomic_read(atomic_t v)
> -- 
> 2.40.0
> 

Re: [XEN PATCH] xen: cache clearing and invalidation helpers refactoring

[Stefano Stabellini]

On Fri, 23 Feb 2024, Nicola Vetrini wrote:
> On 2024-02-19 16:14, Nicola Vetrini wrote:
> > The cache clearing and invalidation helpers in x86 and Arm didn't
> > comply with MISRA C Rule 17.7: "The value returned by a function
> > having non-void return type shall be used". On Arm they
> > were always returning 0, while some in x86 returned -EOPNOTSUPP
> > and in common/grant_table the return value is saved.
> > 
> > As a consequence, a common helper arch_grant_cache_flush that returns
> > an integer is introduced, so that each architecture can choose whether to
> > return an error value on certain conditions, and the helpers have either
> > been changed to return void (on Arm) or deleted entirely (on x86).
> > 
> > Signed-off-by: Julien Grall 
> > Signed-off-by: Nicola Vetrini 
> > ---
> > The original refactor idea came from Julien Grall in [1]; I edited that
> > proposal
> > to fix build errors.
> > 
> > I did introduce a cast to void for the call to flush_area_local on x86,
> > because
> > even before this patch the return value of that function wasn't checked in
> > all
> > but one use in x86/smp.c, and in this context the helper (perhaps
> > incidentally)
> > ignored the return value of flush_area_local.
> > 
> > [1]
> > https://lore.kernel.org/xen-devel/09589e8f-77b6-47f7-b5bd-cf485e4b6...@xen.org/
> > ---
> >  xen/arch/arm/include/asm/page.h | 33 ++---
> >  xen/arch/x86/include/asm/flushtlb.h | 23 ++--
> >  xen/common/grant_table.c|  9 +---
> >  3 files changed, 34 insertions(+), 31 deletions(-)
> > 
> 
> I'll put this patch in the backlog at the moment: too many intricacies while
> trying to untangle xen/flushtlb from xen/mm.h, and there are easier cases that
> can be done faster. If someone is interested I can post the partial work I've
> done so far, even though it doesn't
> build on x86.

I understand that the blocker is:

diff --git a/xen/arch/arm/include/asm/page.h b/xen/arch/arm/include/asm/page.h
index 69f817d1e6..e90c9de361 100644
--- a/xen/arch/arm/include/asm/page.h
+++ b/xen/arch/arm/include/asm/page.h
@@ -123,6 +123,7 @@
 
 #ifndef __ASSEMBLY__
 
+#include 
 #include 
 #include 
 #include 


And the headers disentagling required to solve it, right?


Let me ask a silly question. public/grant_table.h seems needed by
arch_grant_cache_flush. Can we move arch_grant_cache_flush somewhere
else? It is not like page.h is a perfect fit for it anyway.

For instance, can we move it to 

xen/arch/arm/include/asm/grant_table.h

?

Re: [XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7

[Stefano Stabellini]

On Fri, 23 Feb 2024, Nicola Vetrini wrote:
> Refactor cpu_notifier_call_chain into two functions:
> - the variant that is allowed to fail loses the nofail flag
> - the variant that shouldn't fail is encapsulated in a call
>   to the failing variant, with an additional check.
> 
> This prevents uses of the function that are not supposed to
> fail from ignoring the return value, thus violating Rule 17.7:
> "The value returned by a function having non-void return type shall
> be used".
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini 

Reviewed-by: Stefano Stabellini 


> ---
>  xen/common/cpu.c | 31 +++
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/common/cpu.c b/xen/common/cpu.c
> index 8709db4d2957..0b7cf54c4264 100644
> --- a/xen/common/cpu.c
> +++ b/xen/common/cpu.c
> @@ -78,20 +78,27 @@ void __init register_cpu_notifier(struct notifier_block 
> *nb)
>  }
>  
>  static int cpu_notifier_call_chain(unsigned int cpu, unsigned long action,
> -   struct notifier_block **nb, bool nofail)
> +   struct notifier_block **nb)
>  {
>  void *hcpu = (void *)(long)cpu;
>  int notifier_rc = notifier_call_chain(_chain, action, hcpu, nb);
>  int ret =  notifier_to_errno(notifier_rc);
>  
> -BUG_ON(ret && nofail);
> -
>  return ret;
>  }
>  
> +static void cpu_notifier_call_chain_nofail(unsigned int cpu,
> +   unsigned long action,
> +   struct notifier_block **nb)
> +{
> +int ret = cpu_notifier_call_chain(cpu, action, nb);
> +
> +BUG_ON(ret);
> +}
> +
>  static void cf_check _take_cpu_down(void *unused)
>  {
> -cpu_notifier_call_chain(smp_processor_id(), CPU_DYING, NULL, true);
> +cpu_notifier_call_chain_nofail(smp_processor_id(), CPU_DYING, NULL);
>  __cpu_disable();
>  }
>  
> @@ -116,7 +123,7 @@ int cpu_down(unsigned int cpu)
>  if ( !cpu_online(cpu) )
>  goto out;
>  
> -err = cpu_notifier_call_chain(cpu, CPU_DOWN_PREPARE, , false);
> +err = cpu_notifier_call_chain(cpu, CPU_DOWN_PREPARE, );
>  if ( err )
>  goto fail;
>  
> @@ -129,14 +136,14 @@ int cpu_down(unsigned int cpu)
>  err = cpu_online(cpu);
>  BUG_ON(err);
>  
> -cpu_notifier_call_chain(cpu, CPU_DEAD, NULL, true);
> +cpu_notifier_call_chain_nofail(cpu, CPU_DEAD, NULL);
>  
>  send_global_virq(VIRQ_PCPU_STATE);
>  cpu_hotplug_done();
>  return 0;
>  
>   fail:
> -cpu_notifier_call_chain(cpu, CPU_DOWN_FAILED, , true);
> +cpu_notifier_call_chain_nofail(cpu, CPU_DOWN_FAILED, );
>   out:
>  cpu_hotplug_done();
>  return err;
> @@ -157,7 +164,7 @@ int cpu_up(unsigned int cpu)
>  if ( cpu_online(cpu) )
>  goto out;
>  
> -err = cpu_notifier_call_chain(cpu, CPU_UP_PREPARE, , false);
> +err = cpu_notifier_call_chain(cpu, CPU_UP_PREPARE, );
>  if ( err )
>  goto fail;
>  
> @@ -165,7 +172,7 @@ int cpu_up(unsigned int cpu)
>  if ( err < 0 )
>  goto fail;
>  
> -cpu_notifier_call_chain(cpu, CPU_ONLINE, NULL, true);
> +cpu_notifier_call_chain_nofail(cpu, CPU_ONLINE, NULL);
>  
>  send_global_virq(VIRQ_PCPU_STATE);
>  
> @@ -173,7 +180,7 @@ int cpu_up(unsigned int cpu)
>  return 0;
>  
>   fail:
> -cpu_notifier_call_chain(cpu, CPU_UP_CANCELED, , true);
> +cpu_notifier_call_chain_nofail(cpu, CPU_UP_CANCELED, );
>   out:
>  cpu_hotplug_done();
>  return err;
> @@ -181,7 +188,7 @@ int cpu_up(unsigned int cpu)
>  
>  void notify_cpu_starting(unsigned int cpu)
>  {
> -cpu_notifier_call_chain(cpu, CPU_STARTING, NULL, true);
> +cpu_notifier_call_chain_nofail(cpu, CPU_STARTING, NULL);
>  }
>  
>  static cpumask_t frozen_cpus;
> @@ -237,7 +244,7 @@ void enable_nonboot_cpus(void)
>  }
>  
>  for_each_cpu ( cpu, _cpus )
> -cpu_notifier_call_chain(cpu, CPU_RESUME_FAILED, NULL, true);
> +cpu_notifier_call_chain_nofail(cpu, CPU_RESUME_FAILED, NULL);
>  
>  cpumask_clear(_cpus);
>  }
> -- 
> 2.34.1
> 

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

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 15 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-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  f5e1c527d0a0d09ca0cb1dcd8d4ab4a1a5261e91
baseline version:
 xen  812bdc99f4c5d05d20b6fac03b90920c0dbf9a2b

Last test of basis   184730  2024-02-22 17:02:09 Z1 days
Testing same since   184739  2024-02-23 18:00:28 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Anthony PERARD 
  Christian Lindig 
  Edwin Török 
  George Dunlap  # Golang bits
  Juergen Gross 
  Julien Grall 
  Petr Beneš 
  Samuel Thibault 
  Shawn Anastasio 
  Tamas K Lengyel 

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
   812bdc99f4..f5e1c527d0  f5e1c527d0a0d09ca0cb1dcd8d4ab4a1a5261e91 -> smoke

Re: [XEN PATCH 1/2] xen/console: drop return value from consoled_guest_rx/tx

[Stefano Stabellini]

On Fri, 23 Feb 2024, Nicola Vetrini wrote:
> These functions never saw a usage of their return value since
> they were introduced, so it can be dropped since their usages
> violate MISRA C Rule 17.7:
> "The value returned by a function having non-void return type shall be used".
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini 

Reviewed-by: Stefano Stabellini 


> ---
>  xen/drivers/char/consoled.c | 17 +
>  xen/include/xen/consoled.h  |  4 ++--
>  2 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c
> index 222e01844271..b415b632cecc 100644
> --- a/xen/drivers/char/consoled.c
> +++ b/xen/drivers/char/consoled.c
> @@ -43,13 +43,13 @@ struct xencons_interface *consoled_get_ring_addr(void)
>  static char buf[BUF_SZ + 1];
>  
>  /* Receives characters from a domain's PV console */
> -size_t consoled_guest_rx(void)
> +void consoled_guest_rx(void)
>  {
> -size_t recv = 0, idx = 0;
> +size_t idx = 0;
>  XENCONS_RING_IDX cons, prod;
>  
>  if ( !cons_ring )
> -return 0;
> +return;
>  
>  spin_lock(_lock);
>  
> @@ -73,7 +73,6 @@ size_t consoled_guest_rx(void)
>  char c = cons_ring->out[MASK_XENCONS_IDX(cons++, cons_ring->out)];
>  
>  buf[idx++] = c;
> -recv++;
>  
>  if ( idx >= BUF_SZ )
>  {
> @@ -92,18 +91,15 @@ size_t consoled_guest_rx(void)
>  
>   out:
>  spin_unlock(_lock);
> -
> -return recv;
>  }
>  
>  /* Sends a character into a domain's PV console */
> -size_t consoled_guest_tx(char c)
> +void consoled_guest_tx(char c)
>  {
> -size_t sent = 0;
>  XENCONS_RING_IDX cons, prod;
>  
>  if ( !cons_ring )
> -return 0;
> +return;
>  
>  cons = ACCESS_ONCE(cons_ring->in_cons);
>  prod = cons_ring->in_prod;
> @@ -121,7 +117,6 @@ size_t consoled_guest_tx(char c)
>  goto notify;
>  
>  cons_ring->in[MASK_XENCONS_IDX(prod++, cons_ring->in)] = c;
> -sent++;
>  
>  /* Write to the ring before updating the pointer */
>  smp_wmb();
> @@ -130,8 +125,6 @@ size_t consoled_guest_tx(char c)
>   notify:
>  /* Always notify the guest: prevents receive path from getting stuck. */
>  pv_shim_inject_evtchn(pv_console_evtchn());
> -
> -return sent;
>  }
>  
>  /*
> diff --git a/xen/include/xen/consoled.h b/xen/include/xen/consoled.h
> index 2b30516b3a0a..bd7ab6329ee8 100644
> --- a/xen/include/xen/consoled.h
> +++ b/xen/include/xen/consoled.h
> @@ -5,8 +5,8 @@
>  
>  void consoled_set_ring_addr(struct xencons_interface *ring);
>  struct xencons_interface *consoled_get_ring_addr(void);
> -size_t consoled_guest_rx(void);
> -size_t consoled_guest_tx(char c);
> +void consoled_guest_rx(void);
> +void consoled_guest_tx(char c);
>  
>  #endif /* __XEN_CONSOLED_H__ */
>  /*
> -- 
> 2.34.1
> 

Re: [XEN PATCH 1/2] automation/eclair: fully deviate MISRA C:2012 Rules 5.7 and 18.7

[Stefano Stabellini]

On Fri, 23 Feb 2024, Federico Serafini wrote:
> Update ECLAIR configuration to fully deviate Rules 5.7 and 18.7
> as agreed during MISRA meeetings.
> 
> Signed-off-by: Federico Serafini 

Reviewed-by: Stefano Stabellini 


> ---
>  automation/eclair_analysis/ECLAIR/deviations.ecl | 14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
> b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index fd32ff8a9c..02eae39786 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -107,6 +107,11 @@ number of guest paging levels."
>  
> -config=MC3R1.R5.6,reports+={deliberate,"any_area(any_loc(file(adopted_r5_6)))"}
>  -doc_end
>  
> +-doc_begin="The project intentionally reuses tag names in order to have 
> identifiers matching the applicable external specifications as well as 
> established internal conventions.
> +As there is little possibility for developer confusion not resulting into 
> compilation errors, the risk of renaming outweighs the potential advantages 
> of compliance."
> +-config=MC3R1.R5.7,reports+={deliberate,"any()"}
> +-doc_end
> +
>  #
>  # Series 7.
>  #
> @@ -373,6 +378,15 @@ explicit comment indicating the fallthrough intention is 
> present."
>  -config=MC3R1.R16.3,reports+={safe, 
> "any_area(end_loc(any_exp(text(^(?s).*/\\* [fF]all ?through.? 
> \\*/.*$,0..1"}
>  -doc_end
>  
> +#
> +# Series 18.
> +#
> +
> +-doc_begin="Flexible array members are deliberately used and XEN developers 
> are aware of the dangers related to them:
> +unexpected result when the structure is given as argument to a sizeof() 
> operator and the truncation in assignment between structures."
> +-config=MC3R1.R18.7,reports+={deliberate, "any()"}
> +-doc_end
> +
>  #
>  # Series 20.
>  #
> -- 
> 2.34.1
> 

Re: [XEN PATCH 2/2] automation/eclair: tag MISRA C:2012 Rule 8.2 as clean

[Stefano Stabellini]

On Fri, 23 Feb 2024, Federico Serafini wrote:
> Update ECLAIR configuration to consider Rule 8.2 as clean.
> 
> Signed-off-by: Federico Serafini 

Acked-by: Stefano Stabellini 


> ---
>  automation/eclair_analysis/ECLAIR/tagging.ecl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl 
> b/automation/eclair_analysis/ECLAIR/tagging.ecl
> index 900c532196..a1dea32b21 100644
> --- a/automation/eclair_analysis/ECLAIR/tagging.ecl
> +++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
> @@ -30,7 +30,7 @@
>  
>  -doc_begin="Clean guidelines: new violations for these guidelines are not 
> accepted."
>  
> --service_selector={clean_guidelines_common,"MC3R1.D1.1||MC3R1.D2.1||MC3R1.D4.11||MC3R1.D4.14||MC3R1.R1.1||MC3R1.R1.3||MC3R1.R1.4||MC3R1.R2.2||MC3R1.R3.1||MC3R1.R3.2||MC3R1.R4.1||MC3R1.R4.2||MC3R1.R5.1||MC3R1.R5.2||MC3R1.R5.4||MC3R1.R5.6||MC3R1.R6.1||MC3R1.R6.2||MC3R1.R7.1||MC3R1.R8.1||MC3R1.R8.5||MC3R1.R8.6||MC3R1.R8.8||MC3R1.R8.10||MC3R1.R8.12||MC3R1.R8.14||MC3R1.R9.2||MC3R1.R9.4||MC3R1.R9.5||MC3R1.R12.5||MC3R1.R17.3||MC3R1.R17.4||MC3R1.R17.6||MC3R1.R20.13||MC3R1.R20.14||MC3R1.R21.13||MC3R1.R21.19||MC3R1.R21.21||MC3R1.R22.2||MC3R1.R22.4||MC3R1.R22.5||MC3R1.R22.6"
> +-service_selector={clean_guidelines_common,"MC3R1.D1.1||MC3R1.D2.1||MC3R1.D4.11||MC3R1.D4.14||MC3R1.R1.1||MC3R1.R1.3||MC3R1.R1.4||MC3R1.R2.2||MC3R1.R3.1||MC3R1.R3.2||MC3R1.R4.1||MC3R1.R4.2||MC3R1.R5.1||MC3R1.R5.2||MC3R1.R5.4||MC3R1.R5.6||MC3R1.R6.1||MC3R1.R6.2||MC3R1.R7.1||MC3R1.R8.1||MC3R1.R8.2||MC3R1.R8.5||MC3R1.R8.6||MC3R1.R8.8||MC3R1.R8.10||MC3R1.R8.12||MC3R1.R8.14||MC3R1.R9.2||MC3R1.R9.4||MC3R1.R9.5||MC3R1.R12.5||MC3R1.R17.3||MC3R1.R17.4||MC3R1.R17.6||MC3R1.R20.13||MC3R1.R20.14||MC3R1.R21.13||MC3R1.R21.19||MC3R1.R21.21||MC3R1.R22.2||MC3R1.R22.4||MC3R1.R22.5||MC3R1.R22.6"
>  }
>  
>  -setq=target,getenv("XEN_TARGET_ARCH")
> -- 
> 2.34.1
> 

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Stefano Stabellini]

On Fri, 23 Feb 2024, Jan Beulich wrote:
> On 23.02.2024 02:32, Stefano Stabellini wrote:
> > On Tue, 24 Oct 2023, Jan Beulich wrote:
> >> On 24.10.2023 00:05, Stefano Stabellini wrote:
> >>> On Mon, 23 Oct 2023, Jan Beulich wrote:
>  On 23.10.2023 17:23, Simone Ballarin wrote:
> > On 23/10/23 15:34, Jan Beulich wrote:
> >> On 18.10.2023 16:18, Simone Ballarin wrote:
> >>> --- a/xen/include/xen/pdx.h
> >>> +++ b/xen/include/xen/pdx.h
> >>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned 
> >>> long pfn)
> >>>* @param pdx Page index
> >>>* @return Obtained pfn after decompressing the pdx
> >>>*/
> >>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
> >>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned 
> >>> long pdx)
> >>>   {
> >>>   return (pdx & pfn_pdx_bottom_mask) |
> >>>  ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
> >>
> >> Taking this as an example for what I've said above: The compiler can't
> >> know that the globals used by the functions won't change value. Even
> >> within Xen it is only by convention that these variables are assigned
> >> their values during boot, and then aren't changed anymore. Which makes
> >> me wonder: Did you check carefully that around the time the variables
> >> have their values established, no calls to the functions exist (which
> >> might then be subject to folding)?
> >
> > There is no need to check that, the GCC documentation explicitly says:
> >
> > However, functions declared with the pure attribute *can safely read 
> > any 
> > non-volatile objects*, and modify the value of objects in a way that 
> > does not affect their return value or the observable state of the 
> > program.
> 
>  I did quote this same text in response to what Andrew has said, but I 
>  also
>  did note there that this needs to be taken with a grain of salt: The
>  compiler generally assumes a single-threaded environment, i.e. no changes
>  to globals behind the back of the code it is processing.
> >>>
> >>> Let's start from the beginning. The reason for Simone to add
> >>> __attribute_pure__ to pdx_to_pfn and other functions is for
> >>> documentation purposes. It is OK if it doesn't serve any purpose other
> >>> than documentation.
> >>>
> >>> Andrew, for sure we do not want to lie to the compiler and introduce
> >>> undefined behavior. If we think there is a risk of it, we should not do
> >>> it.
> >>>
> >>> So, what do we want to document? We want to document that the function
> >>> does not have side effects according to MISRA's definition of it, which
> >>> might subtly differ from GCC's definition.
> >>>
> >>> Looking at GCC's definition of __attribute_pure__, with the
> >>> clarification statement copy/pasted above by both Simone and Jan, it
> >>> seems that __attribute_pure__ matches MISRA's definition of a function
> >>> without side effects. It also seems that pdx_to_pfn abides to that
> >>> definition.
> >>>
> >>> Jan has a point that GCC might be making other assumptions
> >>> (single-thread execution) that might not hold true in our case. Given
> >>> the way the GCC statement is written I think this is low risk. But maybe
> >>> not all GCC versions we want to support in the project might have the
> >>> same definition of __attribute_pure__. So we could end up using
> >>> __attribute_pure__ correctly for the GCC version used for safety (GCC
> >>> 12.1, see docs/misra/C-language-toolchain.rst) but it might actually
> >>> break an older GCC version.
> >>>
> >>>
> >>> So Option#1 is to use __attribute_pure__ taking the risk that a GCC or
> >>> Clang version might interpret __attribute_pure__ differently and
> >>> potentially misbehave.
> >>>
> >>> Option#2 is to avoid this risk, by not using __attribute_pure__.
> >>> Instead, we can use SAF-xx-safe or deviations.rst to document that
> >>> pdx_to_pfn and other functions like it are without side effects
> >>> according to MISRA's definition.
> >>>
> >>>
> >>> Both options have pros and cons. To me the most important factor is how
> >>> many GCC versions come with the statement "pure attribute can safely
> >>> read any non-volatile objects, and modify the value of objects in a way
> >>> that does not affect their return value or the observable state of the
> >>> program".
> >>>
> >>> I checked and these are the results:
> >>> - gcc 4.0.2: no statement
> >>> - gcc 5.1.0: no statement
> >>> - gcc 6.1.0: no statement
> >>> - gcc 7.1.0: no statement
> >>> - gcc 8.1.0: alternative statement "The pure attribute imposes similar
> >>>   but looser restrictions on a function’s definition than the const
> >>>   attribute: it allows the function to read global variables."
> >>> - gcc 9.1.0: yes statement
> >>>
> >>>
> >>> So based on the above, __attribute_pure__ comes with its current
> >>> definition only from 

Re: [RFC XEN PATCH v5 2/5] x86/pvh: Allow (un)map_pirq when dom0 is PVH

[Stefano Stabellini]

On Fri, 23 Feb 2024, Chen, Jiqian wrote:
> On 2024/2/23 08:40, Stefano Stabellini wrote:
> > On Fri, 12 Jan 2024, Jiqian Chen wrote:
> >> If run Xen with PVH dom0 and hvm domU, hvm will map a pirq for
> >> a passthrough device by using gsi, see
> >> xen_pt_realize->xc_physdev_map_pirq and
> >> pci_add_dm_done->xc_physdev_map_pirq. Then xc_physdev_map_pirq
> >> will call into Xen, but in hvm_physdev_op, PHYSDEVOP_map_pirq
> >> is not allowed because currd is PVH dom0 and PVH has no
> >> X86_EMU_USE_PIRQ flag, it will fail at has_pirq check.
> >>
> >> So, allow PHYSDEVOP_map_pirq when dom0 is PVH and also allow
> >> PHYSDEVOP_unmap_pirq for the failed path to unmap pirq. And
> >> add a new check to prevent self map when caller has no PIRQ
> >> flag.
> >>
> >> Co-developed-by: Huang Rui 
> >> Signed-off-by: Jiqian Chen 
> >> ---
> >>  xen/arch/x86/hvm/hypercall.c |  2 ++
> >>  xen/arch/x86/physdev.c   | 22 ++
> >>  2 files changed, 24 insertions(+)
> >>
> >> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> >> index 6ad5b4d5f11f..493998b42ec5 100644
> >> --- a/xen/arch/x86/hvm/hypercall.c
> >> +++ b/xen/arch/x86/hvm/hypercall.c
> >> @@ -74,6 +74,8 @@ long hvm_physdev_op(int cmd, 
> >> XEN_GUEST_HANDLE_PARAM(void) arg)
> >>  {
> >>  case PHYSDEVOP_map_pirq:
> >>  case PHYSDEVOP_unmap_pirq:
> >> +break;
> >> +
> >>  case PHYSDEVOP_eoi:
> >>  case PHYSDEVOP_irq_status_query:
> >>  case PHYSDEVOP_get_free_pirq:
> >> diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
> >> index 47c4da0af7e1..7f2422c2a483 100644
> >> --- a/xen/arch/x86/physdev.c
> >> +++ b/xen/arch/x86/physdev.c
> >> @@ -303,11 +303,22 @@ ret_t do_physdev_op(int cmd, 
> >> XEN_GUEST_HANDLE_PARAM(void) arg)
> >>  case PHYSDEVOP_map_pirq: {
> >>  physdev_map_pirq_t map;
> >>  struct msi_info msi;
> >> +struct domain *d;
> >>  
> >>  ret = -EFAULT;
> >>  if ( copy_from_guest(, arg, 1) != 0 )
> >>  break;
> >>  
> >> +d = rcu_lock_domain_by_any_id(map.domid);
> >> +if ( d == NULL )
> >> +return -ESRCH;
> >> +if ( !is_pv_domain(d) && !has_pirq(d) )
> > 
> > I think this could just be:
> > 
> > if ( !has_pirq(d) )
> > 
> > Right?
> No. In the beginning, I only set this condition here, but it destroyed PV 
> dom0.
> Because  PV has no pirq flag too, it can match this condition and return 
> -EOPNOTSUPP, PHYSDEVOP_map_pirq will fail.

Yes I get it now. Thanks for the explanation. I think "has_pirq" is
misnamed and should probably be hvm_has_pirq or something like that.

I am not asking to fix it, but maybe an in-code comment here would help,
like:

/* if it is an HVM guest, check if it has PIRQs */

in any case:

Reviewed-by: Stefano Stabellini 

[linux-5.4 test] 184733: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 linux6e1f54a4985b63bc1b55a09e5e75a974c5d6719b
baseline version:
 linuxf0602893f43a54097fcf22bd8c2f7b8e75ca643e

Last test of basis   184523  2024-01-29 23:43:48 Z   24 days
Testing same since   184733  2024-02-23 07:45:41 Z0 days1 attempts


People who touched revisions under test:
  "Guilherme G. Piccoli" 
  Adrian Hunter 
  Ajay Kaher 
 

Re: question about virtio-vsock on xen

[Stefano Stabellini]

Hi Peng,

We haven't tried to setup virtio-vsock yet.

In general, I am very supportive of using QEMU for virtio backends. We
use QEMU to provide virtio-net, virtio-block, virtio-console and more.

However, typically virtio-vsock comes into play for VM-to-VM
communication, which is different. Going via QEMU in Dom0 just to have 1
VM communicate with another VM is not an ideal design: it adds latency
and uses resources in Dom0 when actually we could do without it.

A better model for VM-to-VM communication would be to have the VM talk
to each other directly via grant table or pre-shared memory (see the
static shared memory feature) or via Xen hypercalls (see Argo.)

For a good Xen design, I think the virtio-vsock backend would need to be
in Xen itself (the hypervisor).

Of course that is more work and it doesn't help you with the specific
question you had below :-)

For that, I don't have a pointer to help you but maybe others in CC
have.

Cheers,

Stefano


On Fri, 23 Feb 2024, Peng Fan wrote:
> Hi All,
> 
> Has anyone make virtio-vsock on xen work? My dm args as below:
> 
> virtio = [
> 'backend=0,type=virtio,device,transport=pci,bdf=05:00.0,backend_type=qemu,grant_usage=true'
> ]
> device_model_args = [
> '-D', '/home/root/qemu_log.txt',
> '-d', 
> 'trace:*vsock*,trace:*vhost*,trace:*virtio*,trace:*pci_update*,trace:*pci_route*,trace:*handle_ioreq*,trace:*xen*',
> '-device', 
> 'vhost-vsock-pci,iommu_platform=false,id=vhost-vsock-pci0,bus=pcie.0,addr=5.0,guest-cid=3']
> 
> During my test, it always return failure in dom0 kernel in below code:
> 
> vhost_transport_do_send_pkt {
> ...
>nbytes = copy_to_iter(hdr, sizeof(*hdr), _iter);   
>  
> if (nbytes != sizeof(*hdr)) { 
>   
> vq_err(vq, "Faulted on copying pkt hdr %x %x %x 
> %px\n", nbytes, sizeof(*hdr),
> __builtin_object_size(hdr, 0), _iter);
> kfree_skb(skb);   
>   
> break;
>   
> }  
> }
> 
> I checked copy_to_iter, it is copy data to __user addr, but it never pass, 
> the copy to __user addr always return 0 bytes copied.
> 
> The asm code "sttr x7, [x6]" will trigger data abort, the kernel will run
> into do_page_fault, but lock_mm_and_find_vma report it is VM_FAULT_BADMAP,
> that means the __user addr is not mapped, no vma has this addr.
> 
> I am not sure what may cause this. Appreciate if any comments.
> 
> BTW: I tested blk pci, it works, so the virtio pci should work on my setup.
> 
> Thanks,
> Peng.
> 

Re: [PATCH v4 25/30] xen/riscv: add minimal stuff to processor.h to build full Xen

[Oleksii]

On Mon, 2024-02-19 at 09:06 +0100, Jan Beulich wrote:
> On 16.02.2024 12:16, Oleksii wrote:
> > On Thu, 2024-02-15 at 17:43 +0100, Jan Beulich wrote:
> > > On 15.02.2024 17:38, Oleksii wrote:
> > > > On Tue, 2024-02-13 at 14:33 +0100, Jan Beulich wrote:
> > > > > On 05.02.2024 16:32, Oleksii Kurochko wrote:
> > > > > > +   depends on LLD_VERSION >= 15 || LD_VERSION >=
> > > > > > 23600
> > > > > 
> > > > > What's the linker dependency here? Depending on the answer I
> > > > > might
> > > > > further
> > > > > ask why "TOOLCHAIN" when elsewhere we use CC_HAS_ or HAS_CC_
> > > > > or
> > > > > HAS_AS_.
> > > > I missed to introduce {L}LLD_VERSION config. It should output
> > > > from
> > > > the
> > > > command:
> > > >   riscv64-linux-gnu-ld --version
> > > 
> > > Doesn't answer my question though where the linker version
> > > matters
> > > here.
> > Then I misinterpreted your initial question.
> > Could you please provide further clarification or rephrase it for
> > better understanding?
> > 
> > Probably, your question was about why linker dependency is needed
> > here,
> > then
> > it is not sufficient to check if a toolchain supports a particular 
> > extension without checking if the linker supports that extension   
> > too.
> > For example, Clang 15 supports Zihintpause but GNU bintutils
> > 2.35.2 does not, leading build errors like so:
> >     
> >    riscv64-linux-gnu-ld: -march=rv64i_zihintpause2p0: Invalid or
> >    unknown z ISA extension: 'zihintpause'
> 
> Hmm, that's certainly "interesting" behavior of the RISC-V linker.
> Yet
> isn't the linker capability expected to be tied to that of gas? I
> would
> find it far more natural if a gas dependency existed here. If such a
> connection cannot be taken for granted, I'm pretty sure you'd need to
> probe both then anyway.

Wouldn't it be enough in this case instead of introducing of new
configs and etc, just to do the following:
   +ifeq ($(CONFIG_RISCV_64),y)
   +has_zihintpause = $(call as-insn,$(CC) -mabi=lp64 -
   march=rv64i_zihintpause, "pause",_zihintpause,)
   +else
   +has_zihintpause = $(call as-insn,$(CC) -mabi=ilp32 -
   march=rv32i_zihintpause, "pause",_zihintpause,)
   +endif
   +
riscv-march-$(CONFIG_RISCV_ISA_RV64G) := rv64g
riscv-march-$(CONFIG_RISCV_ISA_C)   := $(riscv-march-y)c
   -riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE) := $(riscv-march-
   y)_zihintpause

# Note that -mcmodel=medany is used so that Xen can be mapped
# into the upper half _or_ the lower half of the address space.
# -mcmodel=medlow would force Xen into the lower half.

   -CFLAGS += -march=$(riscv-march-y) -mstrict-align -mcmodel=medany
   +CFLAGS += -march=$(riscv-march-y)$(has_zihintpause) -mstrict-align
   -
   mcmodel=medany

Probably, it would be better:
   ...
   +CFLAGS += -march=$(riscv-march-y)$(call or,$(has_zihintpause)) -
   mstrict-align -
   mcmodel=medany


~ Oleksii

Re: [RFC XEN PATCH v5 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

[Anthony PERARD]

On Thu, Feb 22, 2024 at 07:22:45AM +, Chen, Jiqian wrote:
> On 2024/2/21 23:03, Anthony PERARD wrote:
> > On Fri, Jan 12, 2024 at 02:13:17PM +0800, Jiqian Chen wrote:
> >> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> >> index a1c6e82631e9..4136a860a048 100644
> >> --- a/tools/libs/light/libxl_pci.c
> >> +++ b/tools/libs/light/libxl_pci.c
> >> @@ -1421,6 +1421,8 @@ static void pci_add_dm_done(libxl__egc *egc,
> >>  uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
> >>  uint32_t domainid = domid;
> >>  bool isstubdom = libxl_is_stubdom(ctx, domid, );
> >> +int gsi;
> >> +bool has_gsi = true;
> > 
> > Why is this function has "gsi" variable, and "gsi = irq" but the next
> > function pci_remove_detached() does only have "irq" variable?
> Because in pci_add_dm_done, it calls " r = xc_physdev_map_pirq(ctx->xch, 
> domid, irq, ); ", it passes the pointer of irq in, and then irq will be 
> changed, so I need to use gsi to save the origin value.

I'm sorry but unconditional "gsi = irq" looks very wrong, that line
needs to be removed or changed, otherwise we have two functions doing the
same thing just after that (xc_domain_irq_permission and
xc_domain_gsi_permission). Instead, my guess is that the
arguments of xc_physdev_map_pirq() needs to be changes. What does
contain `irq` after the map_pirq() call? A "pirq" of some kind? Maybe
xc_physdev_map_pirq(ctx->xch, domid, irq, ) would make things more
clear about what's going on.


And BTW, maybe renaming the variable "has_gsi" to "is_gsi" might make
things a bit clearer as well, as in: "the variable 'irq' $is_gsi",
instead of a variable that have a meaning of "the system $has_gsi"
without necessarily meaning that the code is using it.

Maybe using (abusing?) the variable name "irq" might be a bit wrong now?
What does "GSI" stand for anyway? And about "PIRQ"? This is just some
question to figure out if there's potentially a better name for the
variables names.

Thanks,

-- 
Anthony PERARD

Re: [XEN v5 2/3] xen/arm: arm64: Add emulation of Debug Data Transfer Registers

[Ayan Kumar Halder]

Hi,

On 20/02/2024 12:17, Ayan Kumar Halder wrote:

From: Michal Orzel 

Currently, if user enables HVC_DCC config option in Linux, it invokes access
to debug data transfer registers (i.e. DBGDTRTX_EL0 on arm64, DBGDTRTXINT on
arm32). As these registers are not emulated, Xen injects an undefined
exception to the guest and Linux crashes.

To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0
(these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as TXfull.

Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0
"If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN".

Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before
using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() ---> hvc_dcc_check(),
and returns -ENODEV in case TXfull bit is still set after writing a test
character. This way we prevent the guest from making use of HVC DCC as a
console.

Signed-off-by: Michal Orzel 
Signed-off-by: Ayan Kumar Halder 
Reviewed-by: Michal Orzel 
---
Changes from

v1 :- 1. DBGDTR_EL0 does not emulate RXfull. This is to avoid giving the OS any
indication that the RX buffer is full and is waiting to be read.

2. In Arm32, DBGOSLSR is emulated. Also DBGDTRTXINT is emulated at EL0 only.

3. Fixed the commit message and inline code comments.

v2 :- 1. Split the patch into two (separate patches for arm64 and arm32).
2. Removed the "fail" label.
3. Fixed the commit message.

v3 :- 1. "HSR_SYSREG_MDCCSR_EL0" emulation differs based on whether
partial_emulation_enabled is true or not.

2. If partial_emulation_enabled is false, then access to HSR_SYSREG_DBGDTR_EL0,
HSR_SYSREG_DBGDTRTX_EL0 would lead to undefined exception.

v4 :- 1. Invoked "goto fail" from "default:" to ensure compliance with
MISRA 15.3.

  xen/arch/arm/arm64/vsysreg.c | 68 +++-
  xen/arch/arm/include/asm/arm64/hsr.h |  3 ++
  2 files changed, 50 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index b5d54c569b..80918bc799 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -82,6 +82,7 @@ TVM_REG(CONTEXTIDR_EL1)
  void do_sysreg(struct cpu_user_regs *regs,
 const union hsr hsr)
  {
+const struct hsr_sysreg sysreg = hsr.sysreg;
  int regidx = hsr.sysreg.reg;
  struct vcpu *v = current;
  
@@ -159,9 +160,6 @@ void do_sysreg(struct cpu_user_regs *regs,

   *
   * Unhandled:
   *MDCCINT_EL1
- *DBGDTR_EL0
- *DBGDTRRX_EL0
- *DBGDTRTX_EL0
   *OSDTRRX_EL1
   *OSDTRTX_EL1
   *OSECCR_EL1
@@ -171,12 +169,42 @@ void do_sysreg(struct cpu_user_regs *regs,
   */
  case HSR_SYSREG_MDSCR_EL1:
  return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
+
+/*
+ * Xen doesn't expose a real (or emulated) Debug Communications Channel
+ * (DCC) to a domain. Yet the Arm ARM implies this is not an optional
+ * feature. So some domains may start to probe it. For instance, the
+ * HVC_DCC driver in Linux (since f35dc083 and at least up to v6.7),
+ * will try to write some characters and check if the transmit buffer
+ * has emptied.
+ */
  case HSR_SYSREG_MDCCSR_EL0:
  /*
+ * By setting TX status bit (only if partial emulation is enabled) to
+ * indicate the transmit buffer is full, we would hint the OS that the
+ * DCC is probably not working.
+ *
+ * Bit 29: TX full
+ *
   * Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate 
that
   * register as RAZ/WI above. So RO at both EL0 and EL1.
   */
-return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
+return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 0,
+  partial_emulation ? (1U << 29) : 0);
+
+case HSR_SYSREG_DBGDTR_EL0:
+/* DBGDTR[TR]X_EL0 share the same encoding */
+case HSR_SYSREG_DBGDTRTX_EL0:
+/*
+ * Emulate as RAZ/WI (only if partial emulation is enabled) to prevent
+ * injecting undefined exception.
+ * Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate 
that
+ * register as RAZ/WI.
+ */
+if ( !partial_emulation )
+goto fail;
+return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0);
+
  HSR_SYSREG_DBG_CASES(DBGBVR):
  HSR_SYSREG_DBG_CASES(DBGBCR):
  HSR_SYSREG_DBG_CASES(DBGWVR):
@@ -394,26 +422,24 @@ void do_sysreg(struct cpu_user_regs *regs,
   * And all other unknown registers.
   */
  default:
-{
-const struct hsr_sysreg sysreg = hsr.sysreg;
-
-gdprintk(XENLOG_ERR,
- "%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",
- sysreg.read ? "mrs" : "msr",
- sysreg.op0, sysreg.op1,
- sysreg.crn, sysreg.crm,
-   

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 xen  812bdc99f4c5d05d20b6fac03b90920c0dbf9a2b
baseline version:
 xen  9ee7dc877b8754ce2fc82500feea52c04d4e6409

Last test of basis   184729  2024-02-22 14:07:38 Z1 days
Testing same since   184732  2024-02-23 04:57:51 Z0 days1 attempts


People who touched revisions under test:
  Jan Beulich 
  Julien Grall 
  Luca Fancellu 
  

[ovmf test] 184736: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf 11ad164bcea6b0ed3628d595090f84892c367086
baseline version:
 ovmf 8ccd63d14da5678a4b95df0aa954a2378355af9b

Last test of basis   184724  2024-02-22 03:43:20 Z1 days
Testing same since   184736  2024-02-23 13:13:11 Z0 days1 attempts


People who touched revisions under test:
  Dhaval 
  Dhaval Sharma 

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



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

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

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

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


Pushing revision :

To xenbits.xen.org:/home/xen/git/osstest/ovmf.git
   8ccd63d14d..11ad164bce  11ad164bcea6b0ed3628d595090f84892c367086 -> 
xen-tested-master

Re: [PATCH 1/5] xen/livepatch: register livepatch regions when loaded

[Roger Pau Monné]

On Fri, Feb 23, 2024 at 09:45:15AM +, Ross Lagerwall wrote:
> On Thu, Nov 30, 2023 at 2:30 PM Roger Pau Monne  wrote:
> >
> > Currently livepatch regions are registered as virtual regions only after the
> > livepatch has been applied.
> >
> > This can lead to issues when using the pre-apply or post-revert hooks, as at
> > the point the livepatch is not in the virtual regions list.  If a livepatch
> > pre-apply hook contains a WARN() it would trigger an hypervisor crash, as 
> > the
> > code to handle the bug frame won't be able to find the instruction pointer 
> > that
> > triggered the #UD in any of the registered virtual regions, and hence crash.
> >
> > Fix this by adding the livepatch payloads as virtual regions as soon as 
> > loaded,
> > and only remove them once the payload is unloaded.  This requires some 
> > changes
> > to the virtual regions code, as the removal of the virtual regions is no 
> > longer
> > done in stop machine context, and hence an RCU barrier is added in order to
> > make sure there are no users of the virtual region after it's been removed 
> > from
> > the list.
> >
> > Fixes: 8313c864fa95 ('livepatch: Implement pre-|post- apply|revert hooks')
> > Signed-off-by: Roger Pau Monné 
> > ---
> >  xen/common/livepatch.c  |  5 +++--
> >  xen/common/virtual_region.c | 40 +++--
> >  2 files changed, 15 insertions(+), 30 deletions(-)
> >
> > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> > index 1209fea2566c..3199432f11f5 100644
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -942,6 +942,8 @@ static int prepare_payload(struct payload *payload,
> >  }
> >  }
> >
> > +register_virtual_region(region);
> > +
> >  return 0;
> >  }
> >
> 
> The region is registered in prepare_payload() but if e.g. the build id
> check in load_payload_data() fails, it won't unregister the region
> since the failure path calls free_payload_data(), not free_payload().
> Perhaps the call to register_virtual_region() could be done as the last
> thing in load_payload_data()?

I've moved it to livepatch_upload(), as I think it's more natural to
be done together with the addition to the list of livepatches.  Thanks
for spotting it, I guess I got confused between free_payload_data()
free_payload().

Roger.

[PATCH v3] x86/altcall: use an union as register type for function parameters on clang

[Roger Pau Monne]

The current code for alternative calls uses the caller parameter types as the
types for the register variables that serve as function parameters:

uint8_t foo;
[...]
alternative_call(myfunc, foo);

Would expand roughly into:

register unint8_t a1_ asm("rdi") = foo;
register unsigned long a2_ asm("rsi");
[...]
asm volatile ("call *%c[addr](%%rip)"...);

However with -O2 clang will generate incorrect code, given the following
example:

unsigned int func(uint8_t t)
{
return t;
}

static void bar(uint8_t b)
{
int ret_;
register uint8_t di asm("rdi") = b;
register unsigned long si asm("rsi");
register unsigned long dx asm("rdx");
register unsigned long cx asm("rcx");
register unsigned long r8 asm("r8");
register unsigned long r9 asm("r9");
register unsigned long r10 asm("r10");
register unsigned long r11 asm("r11");

asm volatile ( "call %c[addr]"
   : "+r" (di), "=r" (si), "=r" (dx),
 "=r" (cx), "=r" (r8), "=r" (r9),
 "=r" (r10), "=r" (r11), "=a" (ret_)
   : [addr] "i" (&(func)), "g" (func)
   : "memory" );
}

void foo(unsigned int a)
{
bar(a);
}

Clang generates the following assembly code:

func:   # @func
movl%edi, %eax
retq
foo:# @foo
callq   func
retq

Note the truncation of the unsigned int parameter 'a' of foo() to uint8_t when
passed into bar() is lost.  clang doesn't zero extend the parameters in the
callee when required, as the psABI mandates.

The above can be worked around by using an union when defining the register
variables, so that `di` becomes:

register union {
uint8_t e;
unsigned long r;
} di asm("rdi") = { .e = b };

Which results in following code generated for `foo()`:

foo:# @foo
movzbl  %dil, %edi
callq   func
retq

So the truncation is not longer lost.  Apply such workaround only when built
with clang.

Reported-by: Matthew Grooms 
Link: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=277200
Link: https://github.com/llvm/llvm-project/issues/12579
Link: https://github.com/llvm/llvm-project/issues/82598
Signed-off-by: Roger Pau Monné 
---
Changes since v2:
 - Expand the code comment.

Changes since v1:
 - Only apply the union workaround with clang.

Seems like all gitlab build tests are OK with this approach.
---
 xen/arch/x86/include/asm/alternative.h | 25 +
 1 file changed, 25 insertions(+)

diff --git a/xen/arch/x86/include/asm/alternative.h 
b/xen/arch/x86/include/asm/alternative.h
index a1cd6a9fe5b8..3c14db5078ba 100644
--- a/xen/arch/x86/include/asm/alternative.h
+++ b/xen/arch/x86/include/asm/alternative.h
@@ -167,9 +167,34 @@ extern void alternative_branches(void);
 #define ALT_CALL_arg5 "r8"
 #define ALT_CALL_arg6 "r9"
 
+#ifdef CONFIG_CC_IS_CLANG
+/*
+ * Use a union with an unsigned long in order to prevent clang from
+ * skipping a possible truncation of the value.  By using the union any
+ * truncation is carried before the call instruction, in turn covering
+ * for ABI-non-compliance in that the necessary clipping / extension of
+ * the value is supposed to be carried out in the callee.
+ *
+ * Note this behavior is not mandated by the standard, and hence could
+ * stop being a viable workaround, or worse, could cause a different set
+ * of code-generation issues in future clang versions.
+ *
+ * This has been reported upstream:
+ * https://github.com/llvm/llvm-project/issues/12579
+ * https://github.com/llvm/llvm-project/issues/82598
+ */
+#define ALT_CALL_ARG(arg, n)\
+register union {\
+typeof(arg) e;  \
+unsigned long r;\
+} a ## n ## _ asm ( ALT_CALL_arg ## n ) = { \
+.e = ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })   \
+}
+#else
 #define ALT_CALL_ARG(arg, n) \
 register typeof(arg) a ## n ## _ asm ( ALT_CALL_arg ## n ) = \
 ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })
+#endif
 #define ALT_CALL_NO_ARG(n) \
 register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n )
 
-- 
2.43.0

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

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-xl   8 xen-bootfail pass in 184735-retest

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

version targeted for testing:
 linuxffd2cb6b718e189e7e2d5d0c19c25611f92e061a
baseline version:
 linux39133352cbed6626956d38ed72012f49b0421e7b

Last test of basis   184725  2024-02-22 08:27:10 Z1 days
Testing same since   184731  2024-02-22 21:44:16 Z0 days1 attempts


People who touched revisions under test:
  Alex Elder 
  Alexander Gordeev 
  Alexander Kobel 
  Alexei Starovoitov 
  Andrii Nakryiko 
  Avihai Horon 
  Baokun Li 
  Bart Van Assche 
  Christian Brauner 
  Daniel Borkmann 
  Daniil Dulov 
  David Howells 
  David S. Miller 
  David Vernet 
  Dominique Martinet 
  Eric Dumazet 
  Florian Fainelli 
  Florian Westphal 
  Geliang Tang 
  Gianmarco Lusvardi 
  Greg Joyce 
  Hangbin Liu 
  Hans de Goede 
  Hari Bathini 
  Horatiu Vultur 
  Hou Tao 
  Jakub 

Re: [PATCH v2] x86/altcall: use an union as register type for function parameters

[Jan Beulich]

On 23.02.2024 13:18, Roger Pau Monné wrote:
> On Fri, Feb 23, 2024 at 11:43:14AM +0100, Jan Beulich wrote:
>> On 23.02.2024 10:19, Roger Pau Monné wrote:
>>> On Thu, Feb 22, 2024 at 05:55:00PM +0100, Jan Beulich wrote:
 On 22.02.2024 17:44, Roger Pau Monne wrote:
> --- a/xen/arch/x86/include/asm/alternative.h
> +++ b/xen/arch/x86/include/asm/alternative.h
> @@ -167,9 +167,25 @@ extern void alternative_branches(void);
>  #define ALT_CALL_arg5 "r8"
>  #define ALT_CALL_arg6 "r9"
>  
> +#ifdef CONFIG_CC_IS_CLANG
> +/*
> + * Use an union with an unsigned long in order to prevent clang from 
> skipping a
> + * possible truncation of the value.  By using the union any truncation 
> is
> + * carried before the call instruction.
> + * https://github.com/llvm/llvm-project/issues/82598
> + */

 I think it needs saying that this is relying on compiler behavior not
 mandated by the standard, thus explaining why it's restricted to
 Clang (down the road we may even want to restrict to old versions,
 assuming they fix the issue at some point). Plus also giving future
 readers a clear understanding that if something breaks with this, it's
 not really a surprise.
>>>
>>> What about:
>>>
>>> Use a union with an unsigned long in order to prevent clang from
>>> skipping a possible truncation of the value.  By using the union any
>>> truncation is carried before the call instruction.
>>
>> ..., in turn covering for ABI-non-compliance in that the necessary
>> clipping / extension of the value is supposed to be carried out in
>> the callee.
>>
>>>  Note this
>>> behavior is not mandated by the standard, and hence could stop being
>>> a viable workaround, or worse, could cause a different set of
>>> code-generation issues in future clang versions.
>>>
>>> This has been reported upstream at:
>>> https://github.com/llvm/llvm-project/issues/82598
>>>
 Aiui this bug is only a special case of the other, much older one, so
 referencing that one here too would seem advisable.
>>>
>>> My report has been resolved as a duplicate of:
>>>
>>> https://github.com/llvm/llvm-project/issues/43573
>>>
>>> FWIW, I think for the context the link is used in (altcall) my bug
>>> report is more representative, and readers can always follow the trail
>>> into the other inter-related bugs.
>>
>> While true, the comment extension suggested above goes beyond that
>> territory, and there the other bug is quite relevant directly. After all
>> what your change does is papering over a knock-on effect of them not
>> following the ABI. And that simply because it is pretty hard to see how
>> we could work around the ABI non-conformance itself (which btw could
>> bite us if we had any affected C function called from assembly).
>>
>> 43537 looks to be a newer instance of 12579; funny they didn't close
>> that as a duplicate then, too.
> 
> So would you be OK with the following:

Yes, ...

> Use a union with an unsigned long in order to prevent clang from
> skipping a possible truncation of the value.  By using the union any
> truncation is carried before the call instruction, in turn covering
> for ABI-non-compliance in that the necessary clipping / extension of
> the value is supposed to be carried out in the callee.
> 
> Note this behavior is not mandated by the standard, and hence could
> stop being a viable workaround, or worse, could cause a different set
> of code-generation issues in future clang versions.
> 
> This has been reported upstream at:
> https://github.com/llvm/llvm-project/issues/12579

... yet perhaps still list your new bug report here as well.

Jan

Re: [PATCH v4 12/30] xen/riscv: introduce cmpxchg.h

[Oleksii]

> 
> > > > As 1- and 2-byte cases are emulated I decided that is not to
> > > > provide
> > > > sfx argument for emulation macros as it will not have to much
> > > > affect on
> > > > emulated types and just consume more performance on acquire and
> > > > release
> > > > version of sc/ld instructions.
> > > 
> > > Question is whether the common case (4- and 8-byte accesses)
> > > shouldn't
> > > be valued higher, with 1- and 2-byte emulation being there just
> > > to
> > > allow things to not break altogether.
> > If I understand you correctly, it would make sense to add the 'sfx'
> > argument for the 1/2-byte access case, ensuring that all options
> > are
> > available for 1/2-byte access case as well.
> 
> That's one of the possibilities. As said, I'm not overly worried
> about
> the emulated cases. For the initial implementation I'd recommend
> going
> with what is easiest there, yielding the best possible result for the
> 4- and 8-byte cases. If later it turns out repeated acquire/release
> accesses are a problem in the emulation loop, things can be changed
> to explicit barriers, without touching the 4- and 8-byte cases.
I am confused then a little bit if emulated case is not an issue.

For 4- and 8-byte cases for xchg .aqrl is used, for relaxed and aqcuire
version of xchg barries are used.

The similar is done for cmpxchg.

If something will be needed to change in emulation loop it won't
require to change 4- and 8-byte cases.

~ Oleksii

Re: [PATCH v2] x86/altcall: use an union as register type for function parameters

[Roger Pau Monné]

On Fri, Feb 23, 2024 at 11:43:14AM +0100, Jan Beulich wrote:
> On 23.02.2024 10:19, Roger Pau Monné wrote:
> > On Thu, Feb 22, 2024 at 05:55:00PM +0100, Jan Beulich wrote:
> >> On 22.02.2024 17:44, Roger Pau Monne wrote:
> >>> --- a/xen/arch/x86/include/asm/alternative.h
> >>> +++ b/xen/arch/x86/include/asm/alternative.h
> >>> @@ -167,9 +167,25 @@ extern void alternative_branches(void);
> >>>  #define ALT_CALL_arg5 "r8"
> >>>  #define ALT_CALL_arg6 "r9"
> >>>  
> >>> +#ifdef CONFIG_CC_IS_CLANG
> >>> +/*
> >>> + * Use an union with an unsigned long in order to prevent clang from 
> >>> skipping a
> >>> + * possible truncation of the value.  By using the union any truncation 
> >>> is
> >>> + * carried before the call instruction.
> >>> + * https://github.com/llvm/llvm-project/issues/82598
> >>> + */
> >>
> >> I think it needs saying that this is relying on compiler behavior not
> >> mandated by the standard, thus explaining why it's restricted to
> >> Clang (down the road we may even want to restrict to old versions,
> >> assuming they fix the issue at some point). Plus also giving future
> >> readers a clear understanding that if something breaks with this, it's
> >> not really a surprise.
> > 
> > What about:
> > 
> > Use a union with an unsigned long in order to prevent clang from
> > skipping a possible truncation of the value.  By using the union any
> > truncation is carried before the call instruction.
> 
> ..., in turn covering for ABI-non-compliance in that the necessary
> clipping / extension of the value is supposed to be carried out in
> the callee.
> 
> >  Note this
> > behavior is not mandated by the standard, and hence could stop being
> > a viable workaround, or worse, could cause a different set of
> > code-generation issues in future clang versions.
> > 
> > This has been reported upstream at:
> > https://github.com/llvm/llvm-project/issues/82598
> > 
> >> Aiui this bug is only a special case of the other, much older one, so
> >> referencing that one here too would seem advisable.
> > 
> > My report has been resolved as a duplicate of:
> > 
> > https://github.com/llvm/llvm-project/issues/43573
> > 
> > FWIW, I think for the context the link is used in (altcall) my bug
> > report is more representative, and readers can always follow the trail
> > into the other inter-related bugs.
> 
> While true, the comment extension suggested above goes beyond that
> territory, and there the other bug is quite relevant directly. After all
> what your change does is papering over a knock-on effect of them not
> following the ABI. And that simply because it is pretty hard to see how
> we could work around the ABI non-conformance itself (which btw could
> bite us if we had any affected C function called from assembly).
> 
> 43537 looks to be a newer instance of 12579; funny they didn't close
> that as a duplicate then, too.

So would you be OK with the following:

Use a union with an unsigned long in order to prevent clang from
skipping a possible truncation of the value.  By using the union any
truncation is carried before the call instruction, in turn covering
for ABI-non-compliance in that the necessary clipping / extension of
the value is supposed to be carried out in the callee.

Note this behavior is not mandated by the standard, and hence could
stop being a viable workaround, or worse, could cause a different set
of code-generation issues in future clang versions.

This has been reported upstream at:
https://github.com/llvm/llvm-project/issues/12579

Thanks, Roger.

question about virtio-vsock on xen

[Peng Fan]

Hi All,

Has anyone make virtio-vsock on xen work? My dm args as below:

virtio = [
'backend=0,type=virtio,device,transport=pci,bdf=05:00.0,backend_type=qemu,grant_usage=true'
]
device_model_args = [
'-D', '/home/root/qemu_log.txt',
'-d', 
'trace:*vsock*,trace:*vhost*,trace:*virtio*,trace:*pci_update*,trace:*pci_route*,trace:*handle_ioreq*,trace:*xen*',
'-device', 
'vhost-vsock-pci,iommu_platform=false,id=vhost-vsock-pci0,bus=pcie.0,addr=5.0,guest-cid=3']

During my test, it always return failure in dom0 kernel in below code:

vhost_transport_do_send_pkt {
...
   nbytes = copy_to_iter(hdr, sizeof(*hdr), _iter); 
   
if (nbytes != sizeof(*hdr)) {   

vq_err(vq, "Faulted on copying pkt hdr %x %x %x %px\n", 
nbytes, sizeof(*hdr),
__builtin_object_size(hdr, 0), _iter);
kfree_skb(skb); 

break;  

}  
}

I checked copy_to_iter, it is copy data to __user addr, but it never pass, 
the copy to __user addr always return 0 bytes copied.

The asm code "sttr x7, [x6]" will trigger data abort, the kernel will run
into do_page_fault, but lock_mm_and_find_vma report it is VM_FAULT_BADMAP,
that means the __user addr is not mapped, no vma has this addr.

I am not sure what may cause this. Appreciate if any comments.

BTW: I tested blk pci, it works, so the virtio pci should work on my setup.

Thanks,
Peng.

[PATCH v2 3/3] x86/spec: fix INDIRECT_THUNK option to only be set when build-enabled

[Roger Pau Monne]

Attempt to provide a more helpful error message when the user attempts to set
spec-ctrl=bti-thunk option but the support is build-time disabled.

Reported-by: Andrew Cooper 
Signed-off-by: Roger Pau Monné 
---
Changes since v1:
 - New in this version.
---
 xen/arch/x86/spec_ctrl.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 4ce8a7a0b8ef..f3432f1a6c80 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -239,6 +239,7 @@ static int __init cf_check parse_spec_ctrl(const char *s)
 /* Xen's speculative sidechannel mitigation settings. */
 else if ( !strncmp(s, "bti-thunk=", 10) )
 {
+#ifdef CONFIG_INDIRECT_THUNK
 s += 10;
 
 if ( !cmdline_strcmp(s, "retpoline") )
@@ -249,6 +250,10 @@ static int __init cf_check parse_spec_ctrl(const char *s)
 opt_thunk = THUNK_JMP;
 else
 rc = -EINVAL;
+#else
+no_config_param("INDIRECT_THUNK", "spec-ctrl", s, ss);
+rc = -EINVAL;
+#endif
 }
 
 /* Bits in MSR_SPEC_CTRL. */
-- 
2.43.0

Re: [PATCH v1 2/2] oxenstored: make Quota.t pure

[Andrew Cooper]

On 23/02/2024 11:35 am, Edwin Torok wrote:
>> On 31 Jan 2024, at 16:27, Edwin Torok  wrote:
>>> On 31 Jan 2024, at 11:17, Christian Lindig
>>>  wrote:
 On 31 Jan 2024, at 10:52, Edwin Török  wrote:

 Now that we no longer have a hashtable inside we can make Quota.t pure,
 and push the mutable update to its callers.
 Store.t already had a mutable Quota.t field.

 No functional change.
>>>
>>> Acked-by: Christian Lindig 
>>>
>>> This is shifting copying working to GC work, at least potentially. I
>>> would agree that this is a good trade-off and the code looks correct
>>> to me. But I think we should see more testing and benchmarking
>>> before merging this unless we are fine merging speculative improvements.
>>>
>>> — C
>>>
>>>
>>
>>
>> I’ve written this [1] microbenchmark which creates ~300_000 entries
>> in xenstore, assigns quota to 1000 domains and then measure how long
>> it takes to list xenstore
>> (It doesn’t matter whether these domains exist or not).
>> It shows a measurable improvement with this patch, once I’ve run a
>> more realistic test on the original machine with 1000 real VMs I’ll
>> post those results too:
>
> The machine that can run this test is offline now due to a lab move,
> but I managed to get this data before it went away, and I think this
> patch series is ready to be committed.
>
> Flamegraph without my changes, where Hashtbl.copy takes up a
> significant amount of oxenstored
> time: 
> https://cdn.jsdelivr.net/gh/edwintorok/xen@oxenstored-coverletter/docs/original.svg?x=153.0=1269
> 
> Flamegraph with this patch series, where Hashtbl.copy does not show up
> at
> all: 
> https://cdn.jsdelivr.net/gh/edwintorok/xen@oxenstored-coverletter/docs/oxenstored_no_hashtbl_copy.svg?x=294.3=1301
> 
> (There are of course still hashtbl in the flame graph, due to the
> well-known inefficient poll_select implementation, and we see hashtbl
> iteration as a parent caller, which is fine)
>
> IMHO this means the patch series is a worthwhile improvement: it
> removes a codepath that was previously a hotspot completely from
> oxenstored.

Agreed.  I'll queue this series in due course.

~Andrew

[PATCH v2 1/3] xen/cmdline: fix printf format specifier in no_config_param()

[Roger Pau Monne]

'*' sets the width field, which is the minimum number of characters to output,
but what we want in no_config_param() is the precision instead, which is '.*'
as it imposes a maximum limit on the output.

Fixes: 68d757df8dd2 ('x86/pv: Options to disable and/or compile out 32bit PV 
support')
Signed-off-by: Roger Pau Monné 
---
Changes since v1:
 - New in this version.
---
 xen/include/xen/param.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/xen/param.h b/xen/include/xen/param.h
index 9170455cde5c..6442a92aff8e 100644
--- a/xen/include/xen/param.h
+++ b/xen/include/xen/param.h
@@ -191,7 +191,7 @@ static inline void no_config_param(const char *cfg, const 
char *param,
 {
 int len = e ? ({ ASSERT(e >= s); e - s; }) : strlen(s);
 
-printk(XENLOG_INFO "CONFIG_%s disabled - ignoring '%s=%*s' setting\n",
+printk(XENLOG_INFO "CONFIG_%s disabled - ignoring '%s=%.*s' setting\n",
cfg, param, len, s);
 }
 
-- 
2.43.0

[PATCH v2 2/3] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled

[Roger Pau Monne]

The current logic to handle the BRANCH_HARDEN option will report it as enabled
even when build-time disabled. Fix this by only allowing the option to be set
when support for it is built into Xen.

Fixes: 2d6f36daa086 ('x86/nospec: Introduce CONFIG_SPECULATIVE_HARDEN_BRANCH')
Signed-off-by: Roger Pau Monné 
---
Changes since v1:
 - Use no_config_param().
---
 xen/arch/x86/spec_ctrl.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 421fe3f640df..4ce8a7a0b8ef 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1;
 int8_t __ro_after_init opt_ibpb_ctxt_switch = -1;
 int8_t __read_mostly opt_eager_fpu = -1;
 int8_t __read_mostly opt_l1d_flush = -1;
-static bool __initdata opt_branch_harden = true;
+static bool __initdata opt_branch_harden =
+IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH);
 
 bool __initdata bsp_delay_spec_ctrl;
 uint8_t __read_mostly default_xen_spec_ctrl;
@@ -268,7 +269,14 @@ static int __init cf_check parse_spec_ctrl(const char *s)
 else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )
 opt_l1d_flush = val;
 else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 )
+{
+#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH
 opt_branch_harden = val;
+#else
+no_config_param("SPECULATIVE_HARDEN_BRANCH", "spec-ctrl", s, ss);
+rc = -EINVAL;
+#endif
+}
 else if ( (val = parse_boolean("srb-lock", s, ss)) >= 0 )
 opt_srb_lock = val;
 else if ( (val = parse_boolean("unpriv-mmio", s, ss)) >= 0 )
-- 
2.43.0

[PATCH v2 0/3] x86/spec: improve command line parsing

[Roger Pau Monne]

Hello,

A couple of improvements for the spec-ctrl command line parsing, and one
bugfix for no_config_param().

Thanks, Roger.

Roger Pau Monne (3):
  xen/cmdline: fix printf format specifier in no_config_param()
  x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled
  x86/spec: fix INDIRECT_THUNK option to only be set when build-enabled

 xen/arch/x86/spec_ctrl.c | 15 ++-
 xen/include/xen/param.h  |  2 +-
 2 files changed, 15 insertions(+), 2 deletions(-)

-- 
2.43.0

Re: [PATCH v1 2/2] oxenstored: make Quota.t pure

[Edwin Torok]

> On 31 Jan 2024, at 16:27, Edwin Torok  wrote:
> 
> 
> 
>> On 31 Jan 2024, at 11:17, Christian Lindig  
>> wrote:
>> 
>> 
>> 
>>> On 31 Jan 2024, at 10:52, Edwin Török  wrote:
>>> 
>>> Now that we no longer have a hashtable inside we can make Quota.t pure,
>>> and push the mutable update to its callers.
>>> Store.t already had a mutable Quota.t field.
>>> 
>>> No functional change.
>> 
>> Acked-by: Christian Lindig 
>> 
>> This is shifting copying working to GC work, at least potentially. I would 
>> agree that this is a good trade-off and the code looks correct to me. But I 
>> think we should see more testing and benchmarking before merging this unless 
>> we are fine merging speculative improvements.
>> 
>> — C
>> 
>> 
> 
> 
> I’ve written this [1] microbenchmark which creates ~300_000 entries in 
> xenstore, assigns quota to 1000 domains and then measure how long it takes to 
> list xenstore
> (It doesn’t matter whether these domains exist or not).
> It shows a measurable improvement with this patch, once I’ve run a more 
> realistic test on the original machine with 1000 real VMs I’ll post those 
> results too:

The machine that can run this test is offline now due to a lab move, but I 
managed to get this data before it went away, and I think this patch series is 
ready to be committed.

Flamegraph without my changes, where Hashtbl.copy takes up a significant amount 
of oxenstored time: 
https://cdn.jsdelivr.net/gh/edwintorok/xen@oxenstored-coverletter/docs/original.svg?x=153.0=1269
Flamegraph with this patch series, where Hashtbl.copy does not show up at all: 
https://cdn.jsdelivr.net/gh/edwintorok/xen@oxenstored-coverletter/docs/oxenstored_no_hashtbl_copy.svg?x=294.3=1301
(There are of course still hashtbl in the flame graph, due to the well-known 
inefficient poll_select implementation, and we see hashtbl iteration as a 
parent caller, which is fine)

IMHO this means the patch series is a worthwhile improvement: it removes a 
codepath that was previously a hotspot completely from oxenstored.

The timings on the test also show improvements with this patch:

Booting 575 VMs:
* without this patch series: 1099s
* with this patch series: 604s

Booting 626 VMs:
* without this patch series: 4027s
* with this patch series: 2115s

Booting 627 VMs:
* without this patch series: 4038s
* with this patch series: 4120s

This shows that *with* the patch series the system scales better until it hits 
a breaking point around 627 VMs where everything is ~equally slow

Not everything is ideal, there is also a slowdown around the 789 booted VM mark:
* without this patch series: 168s
* with this patch series: 394s
I wouldn’t worry about this result, because by this point some VMs have already 
crashed, and I’d consider the test to have failed by this point. What results 
you get at this point depends on how much CPU oxenstored gets compared to the 
qemus ioreq handler that is spinning handling the crash on the serial port.
To actually reach 1000 VMs it will require more fixes outsides of oxenstored 
(e.g. wrt to using correct groups with tapdisk, qemu, etc., or making qemu 
better cope with flooding on serial port from the guest), and probably some 
fixes to the poll_select in oxenstored that I may address in a future patch 
series.



Best regards,
—Edwin

> 
> On an Intel Xeon Gold 6354 @ 3.0 Ghz:
> * without my patch: 12.756 +- 0.152 seconds time elapsed  ( +-  1.19% )
> * with my patch: 5.6051 +- 0.0467 seconds time elapsed  ( +-  0.83% )
> 
> [1]
> # cat >create.py < #!/usr/bin/env python3
> from xen.lowlevel.xs import xs
> 
> xenstore = xs()
> 
> for i in range(1,1000):
>   txn = xenstore.transaction_start()
>   for j in range(1,10):
> for k in range(1,30):
> path=f"/quotatest/{i}/{j}/{k}/x"
> xenstore.write(txn, path, "val")
> # assign quota to domid i
> xenstore.set_permissions(txn, path, [{"dom": i}])
>   xenstore.transaction_end(txn)
> EOF
> # python3 create.py
> # perf stat -r 10 sh -c 'xenstore-ls $(for i in $(seq 1 100); do echo 
> "/quotatest/$i"; done) >/dev/null’
> 
> Best regards,
> —Edwin

Re: [PATCH] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled

[Andrew Cooper]

On 23/02/2024 11:11 am, Roger Pau Monné wrote:
> On Fri, Feb 23, 2024 at 10:26:15AM +, Andrew Cooper wrote:
>> On 23/02/2024 10:17 am, Roger Pau Monné wrote:
>>> On Fri, Feb 23, 2024 at 09:46:27AM +, Andrew Cooper wrote:
 On 23/02/2024 9:42 am, Roger Pau Monne wrote:
> The current logic to handle the BRANCH_HARDEN option will report it as 
> enabled
> even when build-time disabled. Fix this by only allowing the option to be 
> set
> when support for it is built into Xen.
>
> Fixes: 2d6f36daa086 ('x86/nospec: Introduce 
> CONFIG_SPECULATIVE_HARDEN_BRANCH')
> Signed-off-by: Roger Pau Monné 
> ---
>  xen/arch/x86/spec_ctrl.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
> index 421fe3f640df..e634c6b559b4 100644
> --- a/xen/arch/x86/spec_ctrl.c
> +++ b/xen/arch/x86/spec_ctrl.c
> @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1;
>  int8_t __ro_after_init opt_ibpb_ctxt_switch = -1;
>  int8_t __read_mostly opt_eager_fpu = -1;
>  int8_t __read_mostly opt_l1d_flush = -1;
> -static bool __initdata opt_branch_harden = true;
> +static bool __initdata opt_branch_harden =
> +IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH);
>  
>  bool __initdata bsp_delay_spec_ctrl;
>  uint8_t __read_mostly default_xen_spec_ctrl;
> @@ -267,7 +268,8 @@ static int __init cf_check parse_spec_ctrl(const char 
> *s)
>  opt_eager_fpu = val;
>  else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )
>  opt_l1d_flush = val;
> -else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 )
> +else if ( IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH) &&
> +  (val = parse_boolean("branch-harden", s, ss)) >= 0 )
>  opt_branch_harden = val;
 Yeah, we should definitely fix this, but could we use no_config_param()
 here for the compiled-out case ?

 See cet= for an example.  If we're going to ignore what the user asks,
 we should tell them why.
>>> Maybe I'm missing something: I've looked into using no_config_param(),
>>> but there's no difference really, because cmdline_parse() is called
>>> before the console is initialized, so those messages seem to be
>>> lost.
>> Look at `xl dmesg` rather than the console.  They also do appear on vga
>> in some configurations.
> Oh, my internal buffer was too small on those also got truncated, had
> to bump it.

Yeah - the default buffer size in Xen is too small.  It has been for
more than a decade, which is how long the adjustment in XenServer has
been around.

>
>> There's a separate todo to get these out in a slightly nicer way, but
>> they at least exist in logs.
> I've created:
>
> https://gitlab.com/xen-project/xen/-/issues/184

Thanks.

~Andrew

Re: [PATCH] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled

[Roger Pau Monné]

On Fri, Feb 23, 2024 at 10:26:15AM +, Andrew Cooper wrote:
> On 23/02/2024 10:17 am, Roger Pau Monné wrote:
> > On Fri, Feb 23, 2024 at 09:46:27AM +, Andrew Cooper wrote:
> >> On 23/02/2024 9:42 am, Roger Pau Monne wrote:
> >>> The current logic to handle the BRANCH_HARDEN option will report it as 
> >>> enabled
> >>> even when build-time disabled. Fix this by only allowing the option to be 
> >>> set
> >>> when support for it is built into Xen.
> >>>
> >>> Fixes: 2d6f36daa086 ('x86/nospec: Introduce 
> >>> CONFIG_SPECULATIVE_HARDEN_BRANCH')
> >>> Signed-off-by: Roger Pau Monné 
> >>> ---
> >>>  xen/arch/x86/spec_ctrl.c | 6 --
> >>>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
> >>> index 421fe3f640df..e634c6b559b4 100644
> >>> --- a/xen/arch/x86/spec_ctrl.c
> >>> +++ b/xen/arch/x86/spec_ctrl.c
> >>> @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1;
> >>>  int8_t __ro_after_init opt_ibpb_ctxt_switch = -1;
> >>>  int8_t __read_mostly opt_eager_fpu = -1;
> >>>  int8_t __read_mostly opt_l1d_flush = -1;
> >>> -static bool __initdata opt_branch_harden = true;
> >>> +static bool __initdata opt_branch_harden =
> >>> +IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH);
> >>>  
> >>>  bool __initdata bsp_delay_spec_ctrl;
> >>>  uint8_t __read_mostly default_xen_spec_ctrl;
> >>> @@ -267,7 +268,8 @@ static int __init cf_check parse_spec_ctrl(const char 
> >>> *s)
> >>>  opt_eager_fpu = val;
> >>>  else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )
> >>>  opt_l1d_flush = val;
> >>> -else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 )
> >>> +else if ( IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH) &&
> >>> +  (val = parse_boolean("branch-harden", s, ss)) >= 0 )
> >>>  opt_branch_harden = val;
> >> Yeah, we should definitely fix this, but could we use no_config_param()
> >> here for the compiled-out case ?
> >>
> >> See cet= for an example.  If we're going to ignore what the user asks,
> >> we should tell them why.
> > Maybe I'm missing something: I've looked into using no_config_param(),
> > but there's no difference really, because cmdline_parse() is called
> > before the console is initialized, so those messages seem to be
> > lost.
> 
> Look at `xl dmesg` rather than the console.  They also do appear on vga
> in some configurations.

Oh, my internal buffer was too small on those also got truncated, had
to bump it.

> There's a separate todo to get these out in a slightly nicer way, but
> they at least exist in logs.

I've created:

https://gitlab.com/xen-project/xen/-/issues/184

Thanks, Roger.

Re: [PATCH 0/5] xen/livepatch: fixes for the pre-apply / post-revert hooks

[Jan Beulich]

On 30.11.2023 15:29, Roger Pau Monne wrote:
> Hello,
> 
> The follow series contain a misc of fixes mostly related to the usage of
> the pre-apply / post-revert hooks.  The norevert test is also fixed to
> work as I think was expected.  Finally both the no{apply,revert}
> tests are fixed to build properly, as the files where previously
> unhooked from the build system completely.
> 
> I'm unsure how useful the apply and revert hooks really are, as without
> calling the internal apply/revert functions the state of the payload
> structure is quite likely inconsistent with the code expectations.
> 
> Thanks, Roger.
> 
> Roger Pau Monne (5):
>   xen/livepatch: register livepatch regions when loaded
>   xen/livepatch: search for symbols in all loaded payloads
>   xen/livepatch: fix norevert test attempt to open-code revert
>   xen/livepatch: fix norevert test hook setup typo
>   xen/livepatch: properly build the noapply and norevert tests

With the R-b-s that have arrived, could you clarify in how far these
can be committed out of order?

Jan

Re: [PATCH LIVEPATCH] Fix inclusion of new object files

[Roger Pau Monné]

On Fri, Feb 23, 2024 at 10:35:50AM +, Andrew Cooper wrote:
> Right now, there's a mixup over the xen/ part of the path for new files.
> 
>   + NEW_FILES=./arch/x86/lp-hooks.o
>   + for i in '$NEW_FILES'
>   ++ dirname ./arch/x86/lp-hooks.o
>   + mkdir -p output/./arch/x86
>   + cp patched/./arch/x86/lp-hooks.o output/./arch/x86/lp-hooks.o
>   cp: cannot stat 'patched/./arch/x86/lp-hooks.o': No such file or directory
> 
> Alter the `cd` and `find` runes to use paths relative to the root of the
> different source trees.
> 
> Signed-off-by: Andrew Cooper 

Hm, this never worked AFAICT.

Reviewed-by: Roger Pau Monné 

Thanks, Roger.

Re: [PATCH v2] x86/altcall: use an union as register type for function parameters

[Jan Beulich]

On 23.02.2024 10:19, Roger Pau Monné wrote:
> On Thu, Feb 22, 2024 at 05:55:00PM +0100, Jan Beulich wrote:
>> On 22.02.2024 17:44, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/include/asm/alternative.h
>>> +++ b/xen/arch/x86/include/asm/alternative.h
>>> @@ -167,9 +167,25 @@ extern void alternative_branches(void);
>>>  #define ALT_CALL_arg5 "r8"
>>>  #define ALT_CALL_arg6 "r9"
>>>  
>>> +#ifdef CONFIG_CC_IS_CLANG
>>> +/*
>>> + * Use an union with an unsigned long in order to prevent clang from 
>>> skipping a
>>> + * possible truncation of the value.  By using the union any truncation is
>>> + * carried before the call instruction.
>>> + * https://github.com/llvm/llvm-project/issues/82598
>>> + */
>>
>> I think it needs saying that this is relying on compiler behavior not
>> mandated by the standard, thus explaining why it's restricted to
>> Clang (down the road we may even want to restrict to old versions,
>> assuming they fix the issue at some point). Plus also giving future
>> readers a clear understanding that if something breaks with this, it's
>> not really a surprise.
> 
> What about:
> 
> Use a union with an unsigned long in order to prevent clang from
> skipping a possible truncation of the value.  By using the union any
> truncation is carried before the call instruction.

..., in turn covering for ABI-non-compliance in that the necessary
clipping / extension of the value is supposed to be carried out in
the callee.

>  Note this
> behavior is not mandated by the standard, and hence could stop being
> a viable workaround, or worse, could cause a different set of
> code-generation issues in future clang versions.
> 
> This has been reported upstream at:
> https://github.com/llvm/llvm-project/issues/82598
> 
>> Aiui this bug is only a special case of the other, much older one, so
>> referencing that one here too would seem advisable.
> 
> My report has been resolved as a duplicate of:
> 
> https://github.com/llvm/llvm-project/issues/43573
> 
> FWIW, I think for the context the link is used in (altcall) my bug
> report is more representative, and readers can always follow the trail
> into the other inter-related bugs.

While true, the comment extension suggested above goes beyond that
territory, and there the other bug is quite relevant directly. After all
what your change does is papering over a knock-on effect of them not
following the ABI. And that simply because it is pretty hard to see how
we could work around the ABI non-conformance itself (which btw could
bite us if we had any affected C function called from assembly).

43537 looks to be a newer instance of 12579; funny they didn't close
that as a duplicate then, too.

Jan

Re: [PATCH LIVEPATCH] Fix inclusion of new object files

[Ross Lagerwall]

On Fri, Feb 23, 2024 at 10:35 AM Andrew Cooper
 wrote:
>
> Right now, there's a mixup over the xen/ part of the path for new files.
>
>   + NEW_FILES=./arch/x86/lp-hooks.o
>   + for i in '$NEW_FILES'
>   ++ dirname ./arch/x86/lp-hooks.o
>   + mkdir -p output/./arch/x86
>   + cp patched/./arch/x86/lp-hooks.o output/./arch/x86/lp-hooks.o
>   cp: cannot stat 'patched/./arch/x86/lp-hooks.o': No such file or directory
>
> Alter the `cd` and `find` runes to use paths relative to the root of the
> different source trees.
>
> Signed-off-by: Andrew Cooper 
> ---
> CC: Roger Pau Monné 
> CC: Ross Lagerwall 
>
> I'm unsure whether there's a useful fixes tag to use.  AFAICT it was broken
> even when 0564f0c7a57 claimed to have it working.
> ---
>  livepatch-build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/livepatch-build b/livepatch-build
> index cdb852cc7fea..948b2acfc2f6 100755
> --- a/livepatch-build
> +++ b/livepatch-build
> @@ -227,7 +227,7 @@ function create_patch()
>  fi
>  done
>
> -NEW_FILES=$(comm -23 <(cd patched/xen && find . -type f -name '*.o' | 
> sort) <(cd original/xen && find . -type f -name '*.o' | sort))
> +NEW_FILES=$(comm -23 <(cd patched && find xen/ -type f -name '*.o' | 
> sort) <(cd original && find xen/ -type f -name '*.o' | sort))
>  for i in $NEW_FILES; do
>  mkdir -p "output/$(dirname "$i")"
>  cp "patched/$i" "output/$i"
>
> base-commit: 1b5b03b3ce4187ce99bad580fd0ee36c6337313f
> --

Reviewed-by: Ross Lagerwall 

[PATCH LIVEPATCH] Fix inclusion of new object files

[Andrew Cooper]

Right now, there's a mixup over the xen/ part of the path for new files.

  + NEW_FILES=./arch/x86/lp-hooks.o
  + for i in '$NEW_FILES'
  ++ dirname ./arch/x86/lp-hooks.o
  + mkdir -p output/./arch/x86
  + cp patched/./arch/x86/lp-hooks.o output/./arch/x86/lp-hooks.o
  cp: cannot stat 'patched/./arch/x86/lp-hooks.o': No such file or directory

Alter the `cd` and `find` runes to use paths relative to the root of the
different source trees.

Signed-off-by: Andrew Cooper 
---
CC: Roger Pau Monné 
CC: Ross Lagerwall 

I'm unsure whether there's a useful fixes tag to use.  AFAICT it was broken
even when 0564f0c7a57 claimed to have it working.
---
 livepatch-build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/livepatch-build b/livepatch-build
index cdb852cc7fea..948b2acfc2f6 100755
--- a/livepatch-build
+++ b/livepatch-build
@@ -227,7 +227,7 @@ function create_patch()
 fi
 done
 
-NEW_FILES=$(comm -23 <(cd patched/xen && find . -type f -name '*.o' | 
sort) <(cd original/xen && find . -type f -name '*.o' | sort))
+NEW_FILES=$(comm -23 <(cd patched && find xen/ -type f -name '*.o' | sort) 
<(cd original && find xen/ -type f -name '*.o' | sort))
 for i in $NEW_FILES; do
 mkdir -p "output/$(dirname "$i")"
 cp "patched/$i" "output/$i"

base-commit: 1b5b03b3ce4187ce99bad580fd0ee36c6337313f
-- 
2.30.2

Re: [PATCH 5/5] xen/livepatch: properly build the noapply and norevert tests

[Ross Lagerwall]

On Thu, Nov 30, 2023 at 2:31 PM Roger Pau Monne  wrote:
>
> It seems the build variables for those tests where copy-pasted from
> xen_action_hooks_marker-objs and not adjusted to use the correct source files.
>
> Fixes: 6047104c3ccc ('livepatch: Add per-function applied/reverted state 
> tracking marker')
> Signed-off-by: Roger Pau Monné 
> ---
>  xen/test/livepatch/Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
> index c258ab0b5940..d987a8367f15 100644
> --- a/xen/test/livepatch/Makefile
> +++ b/xen/test/livepatch/Makefile
> @@ -118,12 +118,12 @@ xen_action_hooks_marker-objs := 
> xen_action_hooks_marker.o xen_hello_world_func.o
>  $(obj)/xen_action_hooks_noapply.o: $(obj)/config.h
>
>  extra-y += xen_action_hooks_noapply.livepatch
> -xen_action_hooks_noapply-objs := xen_action_hooks_marker.o 
> xen_hello_world_func.o note.o xen_note.o
> +xen_action_hooks_noapply-objs := xen_action_hooks_noapply.o 
> xen_hello_world_func.o note.o xen_note.o
>
>  $(obj)/xen_action_hooks_norevert.o: $(obj)/config.h
>
>  extra-y += xen_action_hooks_norevert.livepatch
> -xen_action_hooks_norevert-objs := xen_action_hooks_marker.o 
> xen_hello_world_func.o note.o xen_note.o
> +xen_action_hooks_norevert-objs := xen_action_hooks_norevert.o 
> xen_hello_world_func.o note.o xen_note.o
>
>  EXPECT_BYTES_COUNT := 8
>  CODE_GET_EXPECT=$(shell $(OBJDUMP) -d --insn-width=1 $(1) | sed -n -e 
> '/<'$(2)'>:$$/,/^$$/ p' | tail -n +2 | head -n $(EXPECT_BYTES_COUNT) | awk 
> '{$$0=$$2; printf "%s", substr($$0,length-1)}' | sed 's/.\{2\}/0x&,/g' | sed 
> 's/^/{/;s/,$$/}/g')
> --

Reviewed-by: Ross Lagerwall 

Re: [PATCH] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled

[Andrew Cooper]

On 23/02/2024 10:17 am, Roger Pau Monné wrote:
> On Fri, Feb 23, 2024 at 09:46:27AM +, Andrew Cooper wrote:
>> On 23/02/2024 9:42 am, Roger Pau Monne wrote:
>>> The current logic to handle the BRANCH_HARDEN option will report it as 
>>> enabled
>>> even when build-time disabled. Fix this by only allowing the option to be 
>>> set
>>> when support for it is built into Xen.
>>>
>>> Fixes: 2d6f36daa086 ('x86/nospec: Introduce 
>>> CONFIG_SPECULATIVE_HARDEN_BRANCH')
>>> Signed-off-by: Roger Pau Monné 
>>> ---
>>>  xen/arch/x86/spec_ctrl.c | 6 --
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
>>> index 421fe3f640df..e634c6b559b4 100644
>>> --- a/xen/arch/x86/spec_ctrl.c
>>> +++ b/xen/arch/x86/spec_ctrl.c
>>> @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1;
>>>  int8_t __ro_after_init opt_ibpb_ctxt_switch = -1;
>>>  int8_t __read_mostly opt_eager_fpu = -1;
>>>  int8_t __read_mostly opt_l1d_flush = -1;
>>> -static bool __initdata opt_branch_harden = true;
>>> +static bool __initdata opt_branch_harden =
>>> +IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH);
>>>  
>>>  bool __initdata bsp_delay_spec_ctrl;
>>>  uint8_t __read_mostly default_xen_spec_ctrl;
>>> @@ -267,7 +268,8 @@ static int __init cf_check parse_spec_ctrl(const char 
>>> *s)
>>>  opt_eager_fpu = val;
>>>  else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )
>>>  opt_l1d_flush = val;
>>> -else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 )
>>> +else if ( IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH) &&
>>> +  (val = parse_boolean("branch-harden", s, ss)) >= 0 )
>>>  opt_branch_harden = val;
>> Yeah, we should definitely fix this, but could we use no_config_param()
>> here for the compiled-out case ?
>>
>> See cet= for an example.  If we're going to ignore what the user asks,
>> we should tell them why.
> Maybe I'm missing something: I've looked into using no_config_param(),
> but there's no difference really, because cmdline_parse() is called
> before the console is initialized, so those messages seem to be
> lost.

Look at `xl dmesg` rather than the console.  They also do appear on vga
in some configurations.

There's a separate todo to get these out in a slightly nicer way, but
they at least exist in logs.

~Andrew

Re: [PATCH] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled

[Roger Pau Monné]

On Fri, Feb 23, 2024 at 09:46:27AM +, Andrew Cooper wrote:
> On 23/02/2024 9:42 am, Roger Pau Monne wrote:
> > The current logic to handle the BRANCH_HARDEN option will report it as 
> > enabled
> > even when build-time disabled. Fix this by only allowing the option to be 
> > set
> > when support for it is built into Xen.
> >
> > Fixes: 2d6f36daa086 ('x86/nospec: Introduce 
> > CONFIG_SPECULATIVE_HARDEN_BRANCH')
> > Signed-off-by: Roger Pau Monné 
> > ---
> >  xen/arch/x86/spec_ctrl.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
> > index 421fe3f640df..e634c6b559b4 100644
> > --- a/xen/arch/x86/spec_ctrl.c
> > +++ b/xen/arch/x86/spec_ctrl.c
> > @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1;
> >  int8_t __ro_after_init opt_ibpb_ctxt_switch = -1;
> >  int8_t __read_mostly opt_eager_fpu = -1;
> >  int8_t __read_mostly opt_l1d_flush = -1;
> > -static bool __initdata opt_branch_harden = true;
> > +static bool __initdata opt_branch_harden =
> > +IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH);
> >  
> >  bool __initdata bsp_delay_spec_ctrl;
> >  uint8_t __read_mostly default_xen_spec_ctrl;
> > @@ -267,7 +268,8 @@ static int __init cf_check parse_spec_ctrl(const char 
> > *s)
> >  opt_eager_fpu = val;
> >  else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )
> >  opt_l1d_flush = val;
> > -else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 )
> > +else if ( IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH) &&
> > +  (val = parse_boolean("branch-harden", s, ss)) >= 0 )
> >  opt_branch_harden = val;
> 
> Yeah, we should definitely fix this, but could we use no_config_param()
> here for the compiled-out case ?
> 
> See cet= for an example.  If we're going to ignore what the user asks,
> we should tell them why.

Maybe I'm missing something: I've looked into using no_config_param(),
but there's no difference really, because cmdline_parse() is called
before the console is initialized, so those messages seem to be
lost.

Should this go into some kind of buffer which is then printed by
__start_xen() once the console has been initialized? (just after
printing cmdline itself).

Thanks, Roger.

Re: [PATCH 4/5] xen/livepatch: fix norevert test hook setup typo

[Ross Lagerwall]

On Thu, Nov 30, 2023 at 2:31 PM Roger Pau Monne  wrote:
>
> The test code has a typo in using LIVEPATCH_APPLY_HOOK() instead of
> LIVEPATCH_REVERT_HOOK().
>
> Fixes: 6047104c3ccc ('livepatch: Add per-function applied/reverted state 
> tracking marker')
> Signed-off-by: Roger Pau Monné 
> ---
>  xen/test/livepatch/xen_action_hooks_norevert.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/test/livepatch/xen_action_hooks_norevert.c 
> b/xen/test/livepatch/xen_action_hooks_norevert.c
> index 074f8e1d56ce..cdfff156cede 100644
> --- a/xen/test/livepatch/xen_action_hooks_norevert.c
> +++ b/xen/test/livepatch/xen_action_hooks_norevert.c
> @@ -108,7 +108,7 @@ static void post_revert_hook(livepatch_payload_t *payload)
>  printk(KERN_DEBUG "%s: Hook done.\n", __func__);
>  }
>
> -LIVEPATCH_APPLY_HOOK(revert_hook);
> +LIVEPATCH_REVERT_HOOK(revert_hook);
>
>  LIVEPATCH_PREAPPLY_HOOK(pre_apply_hook);
>  LIVEPATCH_POSTAPPLY_HOOK(post_apply_hook);
> --

Reviewed-by: Ross Lagerwall 

Re: [PATCH 2/5] xen/livepatch: search for symbols in all loaded payloads

[Ross Lagerwall]

On Thu, Nov 30, 2023 at 2:30 PM Roger Pau Monne  wrote:
>
> When checking if an address belongs to a patch, or when resolving a symbol,
> take into account all loaded livepatch payloads, even if not applied.
>
> This is required in order for the pre-apply and post-revert hooks to work
> properly, or else Xen won't detect the intruction pointer belonging to those
> hooks as being part of the currently active text.
>
> Move the RCU handling to be used for payload_list instead of applied_list, as
> now the calls from trap code will iterate over the payload_list.
>
> Fixes: 8313c864fa95 ('livepatch: Implement pre-|post- apply|revert hooks')
> Signed-off-by: Roger Pau Monné 

Reviewed-by: Ross Lagerwall 

Re: [PATCH 3/5] xen/livepatch: fix norevert test attempt to open-code revert

[Ross Lagerwall]

On Thu, Nov 30, 2023 at 2:30 PM Roger Pau Monne  wrote:
>
> The purpose of the norevert test is to install a dummy handler that replaces
> the internal Xen revert code, and then perform the revert in the post-revert
> hook.  For that purpose the usage of the previous common_livepatch_revert() is
> not enough, as that just reverts specific functions, but not the whole state 
> of
> the payload.
>
> Remove both common_livepatch_{apply,revert}() and instead expose
> revert_payload{,_tail}() in order to perform the patch revert from the
> post-revert hook.
>
> Fixes: 6047104c3ccc ('livepatch: Add per-function applied/reverted state 
> tracking marker')
> Signed-off-by: Roger Pau Monné 
> ---
>  xen/common/livepatch.c| 41 +--
>  xen/include/xen/livepatch.h   | 32 ++-
>  .../livepatch/xen_action_hooks_norevert.c | 22 +++---
>  3 files changed, 46 insertions(+), 49 deletions(-)
>
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 4e775be66571..d81f3d11d655 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -1367,7 +1367,22 @@ static int apply_payload(struct payload *data)
>  ASSERT(!local_irq_is_enabled());
>
>  for ( i = 0; i < data->nfuncs; i++ )
> -common_livepatch_apply(>funcs[i], >fstate[i]);
> +{
> +const struct livepatch_func *func = >funcs[i];
> +struct livepatch_fstate *state = >fstate[i];
> +
> +/* If the action has been already executed on this function, do 
> nothing. */
> +if ( state->applied == LIVEPATCH_FUNC_APPLIED )
> +{
> +printk(XENLOG_WARNING LIVEPATCH
> +   "%s: %s has been already applied before\n",
> +   __func__, func->name);
> +continue;
> +}
> +
> +arch_livepatch_apply(func, state);
> +state->applied = LIVEPATCH_FUNC_APPLIED;
> +}
>
>  arch_livepatch_revive();
>
> @@ -1383,7 +1398,7 @@ static inline void apply_payload_tail(struct payload 
> *data)
>  data->state = LIVEPATCH_STATE_APPLIED;
>  }
>
> -static int revert_payload(struct payload *data)
> +int revert_payload(struct payload *data)
>  {
>  unsigned int i;
>  int rc;
> @@ -1398,7 +1413,25 @@ static int revert_payload(struct payload *data)
>  }
>
>  for ( i = 0; i < data->nfuncs; i++ )
> -common_livepatch_revert(>funcs[i], >fstate[i]);
> +{
> +const struct livepatch_func *func = >funcs[i];
> +struct livepatch_fstate *state = >fstate[i];
> +
> +/*
> + * If the apply action hasn't been executed on this function, do
> + * nothing.
> + */
> +if ( !func->old_addr || state->applied == LIVEPATCH_FUNC_NOT_APPLIED 
> )
> +{
> +printk(XENLOG_WARNING LIVEPATCH
> +   "%s: %s has not been applied before\n",
> +   __func__, func->name);
> +continue;
> +}
> +
> +arch_livepatch_revert(func, state);
> +state->applied = LIVEPATCH_FUNC_NOT_APPLIED;
> +}
>
>  /*
>   * Since we are running with IRQs disabled and the hooks may call common
> @@ -1416,7 +1449,7 @@ static int revert_payload(struct payload *data)
>  return 0;
>  }
>
> -static inline void revert_payload_tail(struct payload *data)
> +void revert_payload_tail(struct payload *data)
>  {
>  list_del(>applied_list);
>
> diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
> index df339a134e40..9da8b6939878 100644
> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -136,35 +136,11 @@ void arch_livepatch_post_action(void);
>  void arch_livepatch_mask(void);
>  void arch_livepatch_unmask(void);
>
> -static inline void common_livepatch_apply(const struct livepatch_func *func,
> -  struct livepatch_fstate *state)
> -{
> -/* If the action has been already executed on this function, do nothing. 
> */
> -if ( state->applied == LIVEPATCH_FUNC_APPLIED )
> -{
> -printk(XENLOG_WARNING LIVEPATCH "%s: %s has been already applied 
> before\n",
> -__func__, func->name);
> -return;
> -}
> -
> -arch_livepatch_apply(func, state);
> -state->applied = LIVEPATCH_FUNC_APPLIED;
> -}
> +/* Only for testing purposes. */
> +struct payload;
> +int revert_payload(struct payload *data);
> +void revert_payload_tail(struct payload *data);
>
> -static inline void common_livepatch_revert(const struct livepatch_func *func,
> -   struct livepatch_fstate *state)
> -{
> -/* If the apply action hasn't been executed on this function, do 
> nothing. */
> -if ( !func->old_addr || state->applied == LIVEPATCH_FUNC_NOT_APPLIED )
> -{
> -printk(XENLOG_WARNING LIVEPATCH "%s: %s has not been applied 
> before\n",
> -__func__, func->name);
> -return;
> -}
> -
> -

Re: [PATCH] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled

[Roger Pau Monné]

On Fri, Feb 23, 2024 at 09:46:27AM +, Andrew Cooper wrote:
> On 23/02/2024 9:42 am, Roger Pau Monne wrote:
> > The current logic to handle the BRANCH_HARDEN option will report it as 
> > enabled
> > even when build-time disabled. Fix this by only allowing the option to be 
> > set
> > when support for it is built into Xen.
> >
> > Fixes: 2d6f36daa086 ('x86/nospec: Introduce 
> > CONFIG_SPECULATIVE_HARDEN_BRANCH')
> > Signed-off-by: Roger Pau Monné 
> > ---
> >  xen/arch/x86/spec_ctrl.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
> > index 421fe3f640df..e634c6b559b4 100644
> > --- a/xen/arch/x86/spec_ctrl.c
> > +++ b/xen/arch/x86/spec_ctrl.c
> > @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1;
> >  int8_t __ro_after_init opt_ibpb_ctxt_switch = -1;
> >  int8_t __read_mostly opt_eager_fpu = -1;
> >  int8_t __read_mostly opt_l1d_flush = -1;
> > -static bool __initdata opt_branch_harden = true;
> > +static bool __initdata opt_branch_harden =
> > +IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH);
> >  
> >  bool __initdata bsp_delay_spec_ctrl;
> >  uint8_t __read_mostly default_xen_spec_ctrl;
> > @@ -267,7 +268,8 @@ static int __init cf_check parse_spec_ctrl(const char 
> > *s)
> >  opt_eager_fpu = val;
> >  else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )
> >  opt_l1d_flush = val;
> > -else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 )
> > +else if ( IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH) &&
> > +  (val = parse_boolean("branch-harden", s, ss)) >= 0 )
> >  opt_branch_harden = val;
> 
> Yeah, we should definitely fix this, but could we use no_config_param()
> here for the compiled-out case ?

Oh, didn't know that helper existed.

> See cet= for an example.  If we're going to ignore what the user asks,
> we should tell them why.
> 
> And given this as an example, shouldn't we do the same with
> CONFIG_INDIRECT_THUNK and bti=thunk= too ?

Checked for SPECULATIVE_HARDEN_ARRAY and
SPECULATIVE_HARDEN_GUEST_ACCESS, but not the thunk, will add it as a
followup patch.

Thanks, Roger.

Re: [PATCH] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled

[Andrew Cooper]

On 23/02/2024 9:42 am, Roger Pau Monne wrote:
> The current logic to handle the BRANCH_HARDEN option will report it as enabled
> even when build-time disabled. Fix this by only allowing the option to be set
> when support for it is built into Xen.
>
> Fixes: 2d6f36daa086 ('x86/nospec: Introduce CONFIG_SPECULATIVE_HARDEN_BRANCH')
> Signed-off-by: Roger Pau Monné 
> ---
>  xen/arch/x86/spec_ctrl.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
> index 421fe3f640df..e634c6b559b4 100644
> --- a/xen/arch/x86/spec_ctrl.c
> +++ b/xen/arch/x86/spec_ctrl.c
> @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1;
>  int8_t __ro_after_init opt_ibpb_ctxt_switch = -1;
>  int8_t __read_mostly opt_eager_fpu = -1;
>  int8_t __read_mostly opt_l1d_flush = -1;
> -static bool __initdata opt_branch_harden = true;
> +static bool __initdata opt_branch_harden =
> +IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH);
>  
>  bool __initdata bsp_delay_spec_ctrl;
>  uint8_t __read_mostly default_xen_spec_ctrl;
> @@ -267,7 +268,8 @@ static int __init cf_check parse_spec_ctrl(const char *s)
>  opt_eager_fpu = val;
>  else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )
>  opt_l1d_flush = val;
> -else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 )
> +else if ( IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH) &&
> +  (val = parse_boolean("branch-harden", s, ss)) >= 0 )
>  opt_branch_harden = val;

Yeah, we should definitely fix this, but could we use no_config_param()
here for the compiled-out case ?

See cet= for an example.  If we're going to ignore what the user asks,
we should tell them why.

And given this as an example, shouldn't we do the same with
CONFIG_INDIRECT_THUNK and bti=thunk= too ?

~Andrew

Re: [PATCH 1/5] xen/livepatch: register livepatch regions when loaded

[Ross Lagerwall]

On Thu, Nov 30, 2023 at 2:30 PM Roger Pau Monne  wrote:
>
> Currently livepatch regions are registered as virtual regions only after the
> livepatch has been applied.
>
> This can lead to issues when using the pre-apply or post-revert hooks, as at
> the point the livepatch is not in the virtual regions list.  If a livepatch
> pre-apply hook contains a WARN() it would trigger an hypervisor crash, as the
> code to handle the bug frame won't be able to find the instruction pointer 
> that
> triggered the #UD in any of the registered virtual regions, and hence crash.
>
> Fix this by adding the livepatch payloads as virtual regions as soon as 
> loaded,
> and only remove them once the payload is unloaded.  This requires some changes
> to the virtual regions code, as the removal of the virtual regions is no 
> longer
> done in stop machine context, and hence an RCU barrier is added in order to
> make sure there are no users of the virtual region after it's been removed 
> from
> the list.
>
> Fixes: 8313c864fa95 ('livepatch: Implement pre-|post- apply|revert hooks')
> Signed-off-by: Roger Pau Monné 
> ---
>  xen/common/livepatch.c  |  5 +++--
>  xen/common/virtual_region.c | 40 +++--
>  2 files changed, 15 insertions(+), 30 deletions(-)
>
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 1209fea2566c..3199432f11f5 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -942,6 +942,8 @@ static int prepare_payload(struct payload *payload,
>  }
>  }
>
> +register_virtual_region(region);
> +
>  return 0;
>  }
>

The region is registered in prepare_payload() but if e.g. the build id
check in load_payload_data() fails, it won't unregister the region
since the failure path calls free_payload_data(), not free_payload().
Perhaps the call to register_virtual_region() could be done as the last
thing in load_payload_data()?

Ross

[PATCH] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled

[Roger Pau Monne]

The current logic to handle the BRANCH_HARDEN option will report it as enabled
even when build-time disabled. Fix this by only allowing the option to be set
when support for it is built into Xen.

Fixes: 2d6f36daa086 ('x86/nospec: Introduce CONFIG_SPECULATIVE_HARDEN_BRANCH')
Signed-off-by: Roger Pau Monné 
---
 xen/arch/x86/spec_ctrl.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 421fe3f640df..e634c6b559b4 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1;
 int8_t __ro_after_init opt_ibpb_ctxt_switch = -1;
 int8_t __read_mostly opt_eager_fpu = -1;
 int8_t __read_mostly opt_l1d_flush = -1;
-static bool __initdata opt_branch_harden = true;
+static bool __initdata opt_branch_harden =
+IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH);
 
 bool __initdata bsp_delay_spec_ctrl;
 uint8_t __read_mostly default_xen_spec_ctrl;
@@ -267,7 +268,8 @@ static int __init cf_check parse_spec_ctrl(const char *s)
 opt_eager_fpu = val;
 else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )
 opt_l1d_flush = val;
-else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 )
+else if ( IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH) &&
+  (val = parse_boolean("branch-harden", s, ss)) >= 0 )
 opt_branch_harden = val;
 else if ( (val = parse_boolean("srb-lock", s, ss)) >= 0 )
 opt_srb_lock = val;
-- 
2.43.0

[XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7

[Nicola Vetrini]

Refactor cpu_notifier_call_chain into two functions:
- the variant that is allowed to fail loses the nofail flag
- the variant that shouldn't fail is encapsulated in a call
  to the failing variant, with an additional check.

This prevents uses of the function that are not supposed to
fail from ignoring the return value, thus violating Rule 17.7:
"The value returned by a function having non-void return type shall
be used".

No functional change.

Signed-off-by: Nicola Vetrini 
---
 xen/common/cpu.c | 31 +++
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/xen/common/cpu.c b/xen/common/cpu.c
index 8709db4d2957..0b7cf54c4264 100644
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -78,20 +78,27 @@ void __init register_cpu_notifier(struct notifier_block *nb)
 }
 
 static int cpu_notifier_call_chain(unsigned int cpu, unsigned long action,
-   struct notifier_block **nb, bool nofail)
+   struct notifier_block **nb)
 {
 void *hcpu = (void *)(long)cpu;
 int notifier_rc = notifier_call_chain(_chain, action, hcpu, nb);
 int ret =  notifier_to_errno(notifier_rc);
 
-BUG_ON(ret && nofail);
-
 return ret;
 }
 
+static void cpu_notifier_call_chain_nofail(unsigned int cpu,
+   unsigned long action,
+   struct notifier_block **nb)
+{
+int ret = cpu_notifier_call_chain(cpu, action, nb);
+
+BUG_ON(ret);
+}
+
 static void cf_check _take_cpu_down(void *unused)
 {
-cpu_notifier_call_chain(smp_processor_id(), CPU_DYING, NULL, true);
+cpu_notifier_call_chain_nofail(smp_processor_id(), CPU_DYING, NULL);
 __cpu_disable();
 }
 
@@ -116,7 +123,7 @@ int cpu_down(unsigned int cpu)
 if ( !cpu_online(cpu) )
 goto out;
 
-err = cpu_notifier_call_chain(cpu, CPU_DOWN_PREPARE, , false);
+err = cpu_notifier_call_chain(cpu, CPU_DOWN_PREPARE, );
 if ( err )
 goto fail;
 
@@ -129,14 +136,14 @@ int cpu_down(unsigned int cpu)
 err = cpu_online(cpu);
 BUG_ON(err);
 
-cpu_notifier_call_chain(cpu, CPU_DEAD, NULL, true);
+cpu_notifier_call_chain_nofail(cpu, CPU_DEAD, NULL);
 
 send_global_virq(VIRQ_PCPU_STATE);
 cpu_hotplug_done();
 return 0;
 
  fail:
-cpu_notifier_call_chain(cpu, CPU_DOWN_FAILED, , true);
+cpu_notifier_call_chain_nofail(cpu, CPU_DOWN_FAILED, );
  out:
 cpu_hotplug_done();
 return err;
@@ -157,7 +164,7 @@ int cpu_up(unsigned int cpu)
 if ( cpu_online(cpu) )
 goto out;
 
-err = cpu_notifier_call_chain(cpu, CPU_UP_PREPARE, , false);
+err = cpu_notifier_call_chain(cpu, CPU_UP_PREPARE, );
 if ( err )
 goto fail;
 
@@ -165,7 +172,7 @@ int cpu_up(unsigned int cpu)
 if ( err < 0 )
 goto fail;
 
-cpu_notifier_call_chain(cpu, CPU_ONLINE, NULL, true);
+cpu_notifier_call_chain_nofail(cpu, CPU_ONLINE, NULL);
 
 send_global_virq(VIRQ_PCPU_STATE);
 
@@ -173,7 +180,7 @@ int cpu_up(unsigned int cpu)
 return 0;
 
  fail:
-cpu_notifier_call_chain(cpu, CPU_UP_CANCELED, , true);
+cpu_notifier_call_chain_nofail(cpu, CPU_UP_CANCELED, );
  out:
 cpu_hotplug_done();
 return err;
@@ -181,7 +188,7 @@ int cpu_up(unsigned int cpu)
 
 void notify_cpu_starting(unsigned int cpu)
 {
-cpu_notifier_call_chain(cpu, CPU_STARTING, NULL, true);
+cpu_notifier_call_chain_nofail(cpu, CPU_STARTING, NULL);
 }
 
 static cpumask_t frozen_cpus;
@@ -237,7 +244,7 @@ void enable_nonboot_cpus(void)
 }
 
 for_each_cpu ( cpu, _cpus )
-cpu_notifier_call_chain(cpu, CPU_RESUME_FAILED, NULL, true);
+cpu_notifier_call_chain_nofail(cpu, CPU_RESUME_FAILED, NULL);
 
 cpumask_clear(_cpus);
 }
-- 
2.34.1

[XEN PATCH 0/2] address some violations of MISRA C Rule 17.7

[Nicola Vetrini]

MISRA C Rule 17.7 states: "The value returned by a function having non-void
return type shall be used". Therefore the functions that are subject to this
series are refactored to address these concerns.

Nicola Vetrini (2):
  xen/console: drop return value from consoled_guest_rx/tx
  xen/cpu: address MISRA C Rule 17.7

 xen/common/cpu.c| 31 +++
 xen/drivers/char/consoled.c | 17 +
 xen/include/xen/consoled.h  |  4 ++--
 3 files changed, 26 insertions(+), 26 deletions(-)

-- 
2.34.1

[XEN PATCH 1/2] xen/console: drop return value from consoled_guest_rx/tx

[Nicola Vetrini]

These functions never saw a usage of their return value since
they were introduced, so it can be dropped since their usages
violate MISRA C Rule 17.7:
"The value returned by a function having non-void return type shall be used".

No functional change.

Signed-off-by: Nicola Vetrini 
---
 xen/drivers/char/consoled.c | 17 +
 xen/include/xen/consoled.h  |  4 ++--
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c
index 222e01844271..b415b632cecc 100644
--- a/xen/drivers/char/consoled.c
+++ b/xen/drivers/char/consoled.c
@@ -43,13 +43,13 @@ struct xencons_interface *consoled_get_ring_addr(void)
 static char buf[BUF_SZ + 1];
 
 /* Receives characters from a domain's PV console */
-size_t consoled_guest_rx(void)
+void consoled_guest_rx(void)
 {
-size_t recv = 0, idx = 0;
+size_t idx = 0;
 XENCONS_RING_IDX cons, prod;
 
 if ( !cons_ring )
-return 0;
+return;
 
 spin_lock(_lock);
 
@@ -73,7 +73,6 @@ size_t consoled_guest_rx(void)
 char c = cons_ring->out[MASK_XENCONS_IDX(cons++, cons_ring->out)];
 
 buf[idx++] = c;
-recv++;
 
 if ( idx >= BUF_SZ )
 {
@@ -92,18 +91,15 @@ size_t consoled_guest_rx(void)
 
  out:
 spin_unlock(_lock);
-
-return recv;
 }
 
 /* Sends a character into a domain's PV console */
-size_t consoled_guest_tx(char c)
+void consoled_guest_tx(char c)
 {
-size_t sent = 0;
 XENCONS_RING_IDX cons, prod;
 
 if ( !cons_ring )
-return 0;
+return;
 
 cons = ACCESS_ONCE(cons_ring->in_cons);
 prod = cons_ring->in_prod;
@@ -121,7 +117,6 @@ size_t consoled_guest_tx(char c)
 goto notify;
 
 cons_ring->in[MASK_XENCONS_IDX(prod++, cons_ring->in)] = c;
-sent++;
 
 /* Write to the ring before updating the pointer */
 smp_wmb();
@@ -130,8 +125,6 @@ size_t consoled_guest_tx(char c)
  notify:
 /* Always notify the guest: prevents receive path from getting stuck. */
 pv_shim_inject_evtchn(pv_console_evtchn());
-
-return sent;
 }
 
 /*
diff --git a/xen/include/xen/consoled.h b/xen/include/xen/consoled.h
index 2b30516b3a0a..bd7ab6329ee8 100644
--- a/xen/include/xen/consoled.h
+++ b/xen/include/xen/consoled.h
@@ -5,8 +5,8 @@
 
 void consoled_set_ring_addr(struct xencons_interface *ring);
 struct xencons_interface *consoled_get_ring_addr(void);
-size_t consoled_guest_rx(void);
-size_t consoled_guest_tx(char c);
+void consoled_guest_rx(void);
+void consoled_guest_tx(char c);
 
 #endif /* __XEN_CONSOLED_H__ */
 /*
-- 
2.34.1

Re: [PATCH v2] x86/altcall: use an union as register type for function parameters

[Roger Pau Monné]

On Thu, Feb 22, 2024 at 05:55:00PM +0100, Jan Beulich wrote:
> On 22.02.2024 17:44, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/include/asm/alternative.h
> > +++ b/xen/arch/x86/include/asm/alternative.h
> > @@ -167,9 +167,25 @@ extern void alternative_branches(void);
> >  #define ALT_CALL_arg5 "r8"
> >  #define ALT_CALL_arg6 "r9"
> >  
> > +#ifdef CONFIG_CC_IS_CLANG
> > +/*
> > + * Use an union with an unsigned long in order to prevent clang from 
> > skipping a
> > + * possible truncation of the value.  By using the union any truncation is
> > + * carried before the call instruction.
> > + * https://github.com/llvm/llvm-project/issues/82598
> > + */
> 
> I think it needs saying that this is relying on compiler behavior not
> mandated by the standard, thus explaining why it's restricted to
> Clang (down the road we may even want to restrict to old versions,
> assuming they fix the issue at some point). Plus also giving future
> readers a clear understanding that if something breaks with this, it's
> not really a surprise.

What about:

Use a union with an unsigned long in order to prevent clang from
skipping a possible truncation of the value.  By using the union any
truncation is carried before the call instruction.  Note this
behavior is not mandated by the standard, and hence could stop being
a viable workaround, or worse, could cause a different set of
code-generation issues in future clang versions.

This has been reported upstream at:
https://github.com/llvm/llvm-project/issues/82598

> Aiui this bug is only a special case of the other, much older one, so
> referencing that one here too would seem advisable.

My report has been resolved as a duplicate of:

https://github.com/llvm/llvm-project/issues/43573

FWIW, I think for the context the link is used in (altcall) my bug
report is more representative, and readers can always follow the trail
into the other inter-related bugs.

> As a nit: I think it is "a union" (spelling according to pronunciation),
> and I guess the title could now do with saying "optionally" or
> mentioning Clang or some such.

OK.

Thanks, Roger

Re: [PATCH v6 10/11] virtio-gpu: Initialize Venus

[Huang Rui]

On Tue, Jan 02, 2024 at 09:33:11PM +0800, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Dec 19, 2023 at 11:55 AM Huang Rui  wrote:
> >
> > From: Antonio Caggiano 
> >
> > Request Venus when initializing VirGL.
> >
> > Signed-off-by: Antonio Caggiano 
> > Signed-off-by: Huang Rui 
> > ---
> >
> > Changes in v6:
> > - Remove the unstable API flags check because virglrenderer is already 1.0.
> > - Squash the render server flag support into "Initialize Venus".
> >
> >  hw/display/virtio-gpu-virgl.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index f35a751824..c523a6717a 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -964,6 +964,10 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
> >  }
> >  #endif
> >
> > +#ifdef VIRGL_RENDERER_VENUS
> > +flags |= VIRGL_RENDERER_VENUS | VIRGL_RENDERER_RENDER_SERVER;
> > +#endif
> > +
> 
> I wonder if it's a good idea to initialize venus by default. It
> doesn't seem to require vulkan during initialization, but this may
> evolve. Make it optional?
> 

I am fine. In fact, vulkan is widely used for graphic area such as gaming,
compute, VR/AR, etc.

Thanks,
Ray

> >  ret = virgl_renderer_init(g, flags, _gpu_3d_cbs);
> >  if (ret != 0) {
> >  error_report("virgl could not be initialized: %d", ret);
> > --
> > 2.25.1
> >
> 
> 
> -- 
> Marc-André Lureau

[XEN PATCH 0/2] automation/eclair: update deviations and tags

[Federico Serafini]

Deviate Rules 5.7 and 18.7.

Tag Rule 8.2 ad clean.

Federico Serafini (2):
  automation/eclair: fully deviate MISRA C:2012 Rules 5.7 and 18.7
  automation/eclair: tag MISRA C:2012 Rule 8.2 as clean

 automation/eclair_analysis/ECLAIR/deviations.ecl | 14 ++
 automation/eclair_analysis/ECLAIR/tagging.ecl|  2 +-
 2 files changed, 15 insertions(+), 1 deletion(-)

-- 
2.34.1

[XEN PATCH 2/2] automation/eclair: tag MISRA C:2012 Rule 8.2 as clean

[Federico Serafini]

Update ECLAIR configuration to consider Rule 8.2 as clean.

Signed-off-by: Federico Serafini 
---
 automation/eclair_analysis/ECLAIR/tagging.ecl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl 
b/automation/eclair_analysis/ECLAIR/tagging.ecl
index 900c532196..a1dea32b21 100644
--- a/automation/eclair_analysis/ECLAIR/tagging.ecl
+++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
@@ -30,7 +30,7 @@
 
 -doc_begin="Clean guidelines: new violations for these guidelines are not 
accepted."
 
--service_selector={clean_guidelines_common,"MC3R1.D1.1||MC3R1.D2.1||MC3R1.D4.11||MC3R1.D4.14||MC3R1.R1.1||MC3R1.R1.3||MC3R1.R1.4||MC3R1.R2.2||MC3R1.R3.1||MC3R1.R3.2||MC3R1.R4.1||MC3R1.R4.2||MC3R1.R5.1||MC3R1.R5.2||MC3R1.R5.4||MC3R1.R5.6||MC3R1.R6.1||MC3R1.R6.2||MC3R1.R7.1||MC3R1.R8.1||MC3R1.R8.5||MC3R1.R8.6||MC3R1.R8.8||MC3R1.R8.10||MC3R1.R8.12||MC3R1.R8.14||MC3R1.R9.2||MC3R1.R9.4||MC3R1.R9.5||MC3R1.R12.5||MC3R1.R17.3||MC3R1.R17.4||MC3R1.R17.6||MC3R1.R20.13||MC3R1.R20.14||MC3R1.R21.13||MC3R1.R21.19||MC3R1.R21.21||MC3R1.R22.2||MC3R1.R22.4||MC3R1.R22.5||MC3R1.R22.6"
+-service_selector={clean_guidelines_common,"MC3R1.D1.1||MC3R1.D2.1||MC3R1.D4.11||MC3R1.D4.14||MC3R1.R1.1||MC3R1.R1.3||MC3R1.R1.4||MC3R1.R2.2||MC3R1.R3.1||MC3R1.R3.2||MC3R1.R4.1||MC3R1.R4.2||MC3R1.R5.1||MC3R1.R5.2||MC3R1.R5.4||MC3R1.R5.6||MC3R1.R6.1||MC3R1.R6.2||MC3R1.R7.1||MC3R1.R8.1||MC3R1.R8.2||MC3R1.R8.5||MC3R1.R8.6||MC3R1.R8.8||MC3R1.R8.10||MC3R1.R8.12||MC3R1.R8.14||MC3R1.R9.2||MC3R1.R9.4||MC3R1.R9.5||MC3R1.R12.5||MC3R1.R17.3||MC3R1.R17.4||MC3R1.R17.6||MC3R1.R20.13||MC3R1.R20.14||MC3R1.R21.13||MC3R1.R21.19||MC3R1.R21.21||MC3R1.R22.2||MC3R1.R22.4||MC3R1.R22.5||MC3R1.R22.6"
 }
 
 -setq=target,getenv("XEN_TARGET_ARCH")
-- 
2.34.1

[XEN PATCH 1/2] automation/eclair: fully deviate MISRA C:2012 Rules 5.7 and 18.7

[Federico Serafini]

Update ECLAIR configuration to fully deviate Rules 5.7 and 18.7
as agreed during MISRA meeetings.

Signed-off-by: Federico Serafini 
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
b/automation/eclair_analysis/ECLAIR/deviations.ecl
index fd32ff8a9c..02eae39786 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -107,6 +107,11 @@ number of guest paging levels."
 
-config=MC3R1.R5.6,reports+={deliberate,"any_area(any_loc(file(adopted_r5_6)))"}
 -doc_end
 
+-doc_begin="The project intentionally reuses tag names in order to have 
identifiers matching the applicable external specifications as well as 
established internal conventions.
+As there is little possibility for developer confusion not resulting into 
compilation errors, the risk of renaming outweighs the potential advantages of 
compliance."
+-config=MC3R1.R5.7,reports+={deliberate,"any()"}
+-doc_end
+
 #
 # Series 7.
 #
@@ -373,6 +378,15 @@ explicit comment indicating the fallthrough intention is 
present."
 -config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(^(?s).*/\\* 
[fF]all ?through.? \\*/.*$,0..1"}
 -doc_end
 
+#
+# Series 18.
+#
+
+-doc_begin="Flexible array members are deliberately used and XEN developers 
are aware of the dangers related to them:
+unexpected result when the structure is given as argument to a sizeof() 
operator and the truncation in assignment between structures."
+-config=MC3R1.R18.7,reports+={deliberate, "any()"}
+-doc_end
+
 #
 # Series 20.
 #
-- 
2.34.1

Re: [PATCH v6 08/11] virtio-gpu: Resource UUID

[Huang Rui]

On Tue, Jan 02, 2024 at 08:49:54PM +0800, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Dec 19, 2023 at 11:55 AM Huang Rui  wrote:
> >
> > From: Antonio Caggiano 
> >
> > Enable resource UUID feature and implement command resource assign UUID.
> > This is done by introducing a hash table to map resource IDs to their
> > UUIDs.
> 
> I agree with Akihiko, what about putting QemuUUID in struct
> virtio_gpu_simple_resource?

OK, I will add a member of UUID in simple resource structure.

> 
> (I also doubt about the hash table usefulness, but I don't know
> how/why the UUID is used)
> 

The system cannot be without this patch, let me figure it out the reason.

Thanks,
Ray

> >
> > Signed-off-by: Antonio Caggiano 
> > Signed-off-by: Huang Rui 
> > ---
> >
> > Changes in v6:
> > - Set resource uuid as option.
> > - Implement optional subsection of vmstate_virtio_gpu_resource_uuid_state
> >   or virtio live migration.
> > - Use g_int_hash/g_int_equal instead of the default.
> > - Move virtio_vgpu_simple_resource initialization in the earlier new patch
> >   "virtio-gpu: Introduce virgl_gpu_resource structure"
> >
> >  hw/display/trace-events|   1 +
> >  hw/display/virtio-gpu-base.c   |   4 ++
> >  hw/display/virtio-gpu-virgl.c  |   3 +
> >  hw/display/virtio-gpu.c| 119 +
> >  include/hw/virtio/virtio-gpu.h |   7 ++
> >  5 files changed, 134 insertions(+)
> >
> > diff --git a/hw/display/trace-events b/hw/display/trace-events
> > index 2336a0ca15..54d6894c59 100644
> > --- a/hw/display/trace-events
> > +++ b/hw/display/trace-events
> > @@ -41,6 +41,7 @@ virtio_gpu_cmd_res_create_blob(uint32_t res, uint64_t 
> > size) "res 0x%x, size %" P
> >  virtio_gpu_cmd_res_unref(uint32_t res) "res 0x%x"
> >  virtio_gpu_cmd_res_back_attach(uint32_t res) "res 0x%x"
> >  virtio_gpu_cmd_res_back_detach(uint32_t res) "res 0x%x"
> > +virtio_gpu_cmd_res_assign_uuid(uint32_t res) "res 0x%x"
> >  virtio_gpu_cmd_res_xfer_toh_2d(uint32_t res) "res 0x%x"
> >  virtio_gpu_cmd_res_xfer_toh_3d(uint32_t res) "res 0x%x"
> >  virtio_gpu_cmd_res_xfer_fromh_3d(uint32_t res) "res 0x%x"
> > diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> > index 37af256219..6bcee3882f 100644
> > --- a/hw/display/virtio-gpu-base.c
> > +++ b/hw/display/virtio-gpu-base.c
> > @@ -236,6 +236,10 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, 
> > uint64_t features,
> >  features |= (1 << VIRTIO_GPU_F_CONTEXT_INIT);
> >  }
> >
> > +if (virtio_gpu_resource_uuid_enabled(g->conf)) {
> > +features |= (1 << VIRTIO_GPU_F_RESOURCE_UUID);
> > +}
> > +
> >  return features;
> >  }
> >
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index 5a3a292f79..be9da6e780 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -777,6 +777,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >  /* TODO add security */
> >  virgl_cmd_ctx_detach_resource(g, cmd);
> >  break;
> > +case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
> > +virtio_gpu_resource_assign_uuid(g, cmd);
> > +break;
> >  case VIRTIO_GPU_CMD_GET_CAPSET_INFO:
> >  virgl_cmd_get_capset_info(g, cmd);
> >  break;
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 8189c392dc..466debb256 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -958,6 +958,37 @@ virtio_gpu_resource_detach_backing(VirtIOGPU *g,
> >  virtio_gpu_cleanup_mapping(g, res);
> >  }
> >
> > +void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
> > + struct virtio_gpu_ctrl_command *cmd)
> > +{
> > +struct virtio_gpu_simple_resource *res;
> > +struct virtio_gpu_resource_assign_uuid assign;
> > +struct virtio_gpu_resp_resource_uuid resp;
> > +QemuUUID *uuid;
> > +
> > +VIRTIO_GPU_FILL_CMD(assign);
> > +virtio_gpu_bswap_32(, sizeof(assign));
> > +trace_virtio_gpu_cmd_res_assign_uuid(assign.resource_id);
> > +
> > +res = virtio_gpu_find_check_resource(g, assign.resource_id, false, 
> > __func__, >error);
> > +if (!res) {
> > +return;
> > +}
> > +
> > +memset(, 0, sizeof(resp));
> > +resp.hdr.type = VIRTIO_GPU_RESP_OK_RESOURCE_UUID;
> > +
> > +uuid = g_hash_table_lookup(g->resource_uuids, _id);
> > +if (!uuid) {
> > +uuid = g_new(QemuUUID, 1);
> > +qemu_uuid_generate(uuid);
> > +g_hash_table_insert(g->resource_uuids, _id, uuid);
> > +}
> > +
> > +memcpy(resp.uuid, uuid, sizeof(QemuUUID));
> > +virtio_gpu_ctrl_response(g, cmd, , sizeof(resp));
> > +}
> > +
> >  void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
> > struct virtio_gpu_ctrl_command *cmd)
> >  {
> > @@ -1006,6 +1037,9 @@ void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
> >  case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
> >