Re: [PATCH v3 7/8] gzip: move bitbuffer into gunzip state

[Jan Beulich]

On 26.04.2024 07:55, Jan Beulich wrote:
> On 25.04.2024 21:23, Andrew Cooper wrote:
>> On 24/04/2024 5:34 pm, Daniel P. Smith wrote:
>>> --- a/xen/common/gzip/inflate.c
>>> +++ b/xen/common/gzip/inflate.c
>>> @@ -1017,8 +1014,8 @@ static int __init inflate(struct gunzip_state *s)
>>>  /* Undo too much lookahead. The next read will be byte aligned so we
>>>   * can discard unused bits in the last meaningful byte.
>>>   */
>>> -while (bk >= 8) {
>>> -bk -= 8;
>>> +while (s->bk >= 8) {
>>> +s->bk -= 8;
>>>  s->inptr--;
>>>  }
>>
>> Isn't it just me, but isn't this just:
>>
>>     s->inptr -= (s->bk >> 3);
>>     s->bk &= 7;
>>
>> ?
> 
> I'd say yes, if only there wasn't the comment talking of just a single byte,
> and even there supposedly some of the bits.

Talking of the comment: Considering what patch 1 supposedly does, how come
that isn't Xen-style (yet)?

Jan

Re: [PATCH v3 7/8] gzip: move bitbuffer into gunzip state

[Jan Beulich]

On 25.04.2024 21:23, Andrew Cooper wrote:
> On 24/04/2024 5:34 pm, Daniel P. Smith wrote:
>> --- a/xen/common/gzip/inflate.c
>> +++ b/xen/common/gzip/inflate.c
>> @@ -1017,8 +1014,8 @@ static int __init inflate(struct gunzip_state *s)
>>  /* Undo too much lookahead. The next read will be byte aligned so we
>>   * can discard unused bits in the last meaningful byte.
>>   */
>> -while (bk >= 8) {
>> -bk -= 8;
>> +while (s->bk >= 8) {
>> +s->bk -= 8;
>>  s->inptr--;
>>  }
> 
> Isn't it just me, but isn't this just:
> 
>     s->inptr -= (s->bk >> 3);
>     s->bk &= 7;
> 
> ?

I'd say yes, if only there wasn't the comment talking of just a single byte,
and even there supposedly some of the bits.

Jan

Re: [PATCH v4 0/5] DOMCTL-based guest magic region allocation for 11 domUs

[Henry Wang]

Hi Stefano, Daniel,

On 4/26/2024 6:18 AM, Stefano Stabellini wrote:

On Thu, 18 Apr 2024, Daniel P. Smith wrote:

On 4/9/24 00:53, Henry Wang wrote:

An error message can seen from the init-dom0less application on
direct-mapped 1:1 domains:
```
Allocating magic pages
memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
Error on alloc magic pages
```

This is because populate_physmap() automatically assumes gfn == mfn
for direct mapped domains. This cannot be true for the magic pages
that are allocated later for 1:1 Dom0less DomUs from the init-dom0less
helper application executed in Dom0. For domain using statically
allocated memory but not 1:1 direct-mapped, similar error "failed to
retrieve a reserved page" can be seen as the reserved memory list
is empty at that time.

This series tries to fix this issue using a DOMCTL-based approach,
because for 1:1 direct-mapped domUs, we need to avoid the RAM regions
and inform the toolstack about the region found by hypervisor for
mapping the magic pages. Patch 1 introduced a new DOMCTL to get the
guest memory map, currently only used for the magic page regions.
Patch 2 generalized the extended region finding logic so that it can
be reused for other use cases such as finding 1:1 domU magic regions.
Patch 3 uses the same approach as finding the extended regions to find
the guest magic page regions for direct-mapped DomUs. Patch 4 avoids
hardcoding all base addresses of guest magic region in the init-dom0less
application by consuming the newly introduced DOMCTL. Patch 5 is a
simple patch to do some code duplication clean-up in xc.

Hey Henry,

To help provide some perspective, these issues are not experienced with
hyperlaunch. This is because we understood early on that you cannot move a
lightweight version of the toolstack into hypervisor init and not provide a
mechanism to communicate what it did to the runtime control plane. We
evaluated the possible mechanism, to include introducing a new hypercall op,
and ultimately settled on using hypfs. The primary reason is this information
is static data that, while informative later, is only necessary for the
control plane to understand the state of the system. As a result, hyperlaunch
is able to allocate any and all special pages required as part of domain
construction and communicate their addresses to the control plane. As for XSM,
hypfs is already protected and at this time we do not see any domain builder
information needing to be restricted separately from the data already present
in hypfs.

I would like to make the suggestion that instead of continuing down this path,
perhaps you might consider adopting the hyperlaunch usage of hypfs. Then
adjust dom0less domain construction to allocate the special pages at
construction time. The original hyperlaunch series includes a patch that
provides the helper app for the xenstore announcement. And I can provide you
with updated versions if that would be helpful.

I also think that the new domctl is not needed and that the dom0less
domain builder should allocate the magic pages.


Yes this is indeed much better. Thanks Daniel for suggesting this.


On ARM, we already
allocate HVM_PARAM_CALLBACK_IRQ during dom0less domain build and set
HVM_PARAM_STORE_PFN to ~0ULL. I think it would be only natural to extend
that code to also allocate the magic pages and set HVM_PARAM_STORE_PFN
(and others) correctly. If we do it that way it is simpler and
consistent with the HVM_PARAM_CALLBACK_IRQ allocation, and we don't even
need hypfs. Currently we do not enable hypfs in our safety
certifiability configuration.


It is indeed very important to consider the safety certification (which 
I completely missed). Therefore I've sent an updated version based on 
HVMOP [1]. In the future we can switch to hypfs if needed.


[1] 
https://lore.kernel.org/xen-devel/20240426031455.579637-1-xin.wa...@amd.com/


Kind regards,
Henry

[ovmf test] 185803: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf c0dfe3ec1f364dbdaf6b241e01343e560b21dd03
baseline version:
 ovmf 66c24219ade92b85b24f3ce29b988d187a9f6517

Last test of basis   185792  2024-04-25 01:43:46 Z1 days
Testing same since   185803  2024-04-26 03:11:18 Z0 days1 attempts


People who touched revisions under test:
  Gua Guo 

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



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

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

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

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


Pushing revision :

To xenbits.xen.org:/home/xen/git/osstest/ovmf.git
   66c24219ad..c0dfe3ec1f  c0dfe3ec1f364dbdaf6b241e01343e560b21dd03 -> 
xen-tested-master

[PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Henry Wang]

For use cases such as Dom0less PV drivers, a mechanism to communicate
Dom0less DomU's static data with the runtime control plane (Dom0) is
needed. Since on Arm HVMOP is already the existing approach to address
such use cases (for example the allocation of HVM_PARAM_CALLBACK_IRQ),
add a new HVMOP key HVM_PARAM_MAGIC_BASE_PFN for storing the magic
page region base PFN. The value will be set at Dom0less DomU
construction time after Dom0less DomU's magic page region has been
allocated.

To keep consistent, also set the value for HVM_PARAM_MAGIC_BASE_PFN
for libxl guests in alloc_magic_pages().

Reported-by: Alec Kwapis 
Signed-off-by: Henry Wang 
---
 tools/libs/guest/xg_dom_arm.c   | 2 ++
 xen/arch/arm/dom0less-build.c   | 2 ++
 xen/arch/arm/hvm.c  | 1 +
 xen/include/public/hvm/params.h | 1 +
 4 files changed, 6 insertions(+)

diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c
index 8cc7f27dbb..3c08782d1d 100644
--- a/tools/libs/guest/xg_dom_arm.c
+++ b/tools/libs/guest/xg_dom_arm.c
@@ -74,6 +74,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
 xc_clear_domain_page(dom->xch, dom->guest_domid, base + 
MEMACCESS_PFN_OFFSET);
 xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn);
 
+xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MAGIC_BASE_PFN,
+base);
 xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
 dom->console_pfn);
 xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 40dc85c759..72187c167d 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -861,6 +861,8 @@ static int __init construct_domU(struct domain *d,
 free_domheap_pages(magic_pg, get_order_from_pages(NR_MAGIC_PAGES));
 return rc;
 }
+
+d->arch.hvm.params[HVM_PARAM_MAGIC_BASE_PFN] = gfn_x(gfn);
 }
 
 return rc;
diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 0989309fea..fa6141e30c 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -55,6 +55,7 @@ static int hvm_allow_get_param(const struct domain *d, 
unsigned int param)
 case HVM_PARAM_STORE_EVTCHN:
 case HVM_PARAM_CONSOLE_PFN:
 case HVM_PARAM_CONSOLE_EVTCHN:
+case HVM_PARAM_MAGIC_BASE_PFN:
 return 0;
 
 /*
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index a22b4ed45d..c1720b33b9 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -76,6 +76,7 @@
  */
 #define HVM_PARAM_STORE_PFN1
 #define HVM_PARAM_STORE_EVTCHN 2
+#define HVM_PARAM_MAGIC_BASE_PFN3
 
 #define HVM_PARAM_IOREQ_PFN5
 
-- 
2.34.1

[PATCH 0/3] Guest magic region allocation for 11 Dom0less domUs - Take two

[Henry Wang]

Hi all,

This series is trying to fix the reported guest magic region allocation
issue for 11 Dom0less domUs, an error message can seen from the
init-dom0less application on 1:1 direct-mapped Dom0less DomUs:
```
Allocating magic pages
memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
Error on alloc magic pages
```

This is because populate_physmap() automatically assumes gfn == mfn
for direct mapped domains. This cannot be true for the magic pages
that are allocated later for 1:1 Dom0less DomUs from the init-dom0less
helper application executed in Dom0. For domain using statically
allocated memory but not 1:1 direct-mapped, similar error "failed to
retrieve a reserved page" can be seen as the reserved memory list
is empty at that time.

In [1] I've tried to fix this issue by the domctl approach, and
discussions in [2] and [3] indicates that a domctl is not really
necessary, as we can simplify the issue to "allocate the Dom0less
guest magic regions at the Dom0less DomU build time and pass the
region base PFN to init-dom0less application". Therefore, the first
patch in this series will allocate magic pages for Dom0less DomUs,
the second patch will store the allocated region base PFN to HVMOP
params like HVM_PARAM_CALLBACK_IRQ, and the third patch uses the
HVMOP to get the stored guest magic region base PFN to avoid hardcoding
GUEST_MAGIC_BASE.

Gitlab CI for this series can be found in [4].

[1] https://lore.kernel.org/xen-devel/20240409045357.236802-1-xin.wa...@amd.com/
[2] 
https://lore.kernel.org/xen-devel/c7857223-eab8-409a-b618-6ec70f616...@apertussolutions.com/
[3] 
https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2404251508470.3940@ubuntu-linux-20-04-desktop/
[4] https://gitlab.com/xen-project/people/henryw/xen/-/pipelines/1268643360

Henry Wang (3):
  xen/arm/dom0less-build: Alloc magic pages for Dom0less DomUs from
hypervisor
  xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP
  tools/init-dom0less: Avoid hardcoding GUEST_MAGIC_BASE

 tools/helpers/init-dom0less.c   | 38 ++---
 tools/libs/guest/xg_dom_arm.c   |  3 ++-
 xen/arch/arm/dom0less-build.c   | 24 +
 xen/arch/arm/hvm.c  |  1 +
 xen/include/public/arch-arm.h   |  1 +
 xen/include/public/hvm/params.h |  1 +
 6 files changed, 45 insertions(+), 23 deletions(-)

-- 
2.34.1

[PATCH 3/3] tools/init-dom0less: Avoid hardcoding GUEST_MAGIC_BASE

[Henry Wang]

Currently the GUEST_MAGIC_BASE in the init-dom0less application is
hardcoded, which will lead to failures for 1:1 direct-mapped Dom0less
DomUs.

Since the guest magic region is now allocated from the hypervisor,
instead of hardcoding the guest magic pages region, use
xc_hvm_param_get() to get the guest magic region PFN, and based on
that the XenStore PFN can be calculated. Also, we don't need to set
the max mem anymore, so drop the call to xc_domain_setmaxmem(). Rename
the alloc_xs_page() to get_xs_page() to reflect the changes.

Take the opportunity to do some coding style improvements when possible.

Reported-by: Alec Kwapis 
Signed-off-by: Henry Wang 
---
 tools/helpers/init-dom0less.c | 38 +++
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
index fee93459c4..7f6953a818 100644
--- a/tools/helpers/init-dom0less.c
+++ b/tools/helpers/init-dom0less.c
@@ -19,24 +19,20 @@
 #define XENSTORE_PFN_OFFSET 1
 #define STR_MAX_LENGTH 128
 
-static int alloc_xs_page(struct xc_interface_core *xch,
- libxl_dominfo *info,
- uint64_t *xenstore_pfn)
+static int get_xs_page(struct xc_interface_core *xch, libxl_dominfo *info,
+   uint64_t *xenstore_pfn)
 {
 int rc;
-const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
-xen_pfn_t p2m = (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET;
+xen_pfn_t magic_base_pfn;
 
-rc = xc_domain_setmaxmem(xch, info->domid,
- info->max_memkb + (XC_PAGE_SIZE/1024));
-if (rc < 0)
-return rc;
-
-rc = xc_domain_populate_physmap_exact(xch, info->domid, 1, 0, 0, );
-if (rc < 0)
-return rc;
+rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_MAGIC_BASE_PFN,
+  _base_pfn);
+if (rc < 0) {
+printf("Failed to get HVM_PARAM_MAGIC_BASE_PFN\n");
+return 1;
+}
 
-*xenstore_pfn = base + XENSTORE_PFN_OFFSET;
+*xenstore_pfn = magic_base_pfn + XENSTORE_PFN_OFFSET;
 rc = xc_clear_domain_page(xch, info->domid, *xenstore_pfn);
 if (rc < 0)
 return rc;
@@ -100,6 +96,7 @@ static bool do_xs_write_vm(struct xs_handle *xsh, 
xs_transaction_t t,
  */
 static int create_xenstore(struct xs_handle *xsh,
libxl_dominfo *info, libxl_uuid uuid,
+   xen_pfn_t xenstore_pfn,
evtchn_port_t xenstore_port)
 {
 domid_t domid;
@@ -145,8 +142,7 @@ static int create_xenstore(struct xs_handle *xsh,
 rc = snprintf(target_memkb_str, STR_MAX_LENGTH, "%"PRIu64, 
info->current_memkb);
 if (rc < 0 || rc >= STR_MAX_LENGTH)
 return rc;
-rc = snprintf(ring_ref_str, STR_MAX_LENGTH, "%lld",
-  (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET);
+rc = snprintf(ring_ref_str, STR_MAX_LENGTH, "%"PRIu_xen_pfn, xenstore_pfn);
 if (rc < 0 || rc >= STR_MAX_LENGTH)
 return rc;
 rc = snprintf(xenstore_port_str, STR_MAX_LENGTH, "%u", xenstore_port);
@@ -245,8 +241,8 @@ static int init_domain(struct xs_handle *xsh,
 if (!xenstore_evtchn)
 return 0;
 
-/* Alloc xenstore page */
-if (alloc_xs_page(xch, info, _pfn) != 0) {
+/* Get xenstore page */
+if (get_xs_page(xch, info, _pfn) != 0) {
 printf("Error on alloc magic pages\n");
 return 1;
 }
@@ -278,13 +274,11 @@ static int init_domain(struct xs_handle *xsh,
 if (rc < 0)
 return rc;
 
-rc = create_xenstore(xsh, info, uuid, xenstore_evtchn);
+rc = create_xenstore(xsh, info, uuid, xenstore_pfn, xenstore_evtchn);
 if (rc)
 err(1, "writing to xenstore");
 
-rc = xs_introduce_domain(xsh, info->domid,
-(GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET,
-xenstore_evtchn);
+rc = xs_introduce_domain(xsh, info->domid, xenstore_pfn, xenstore_evtchn);
 if (!rc)
 err(1, "xs_introduce_domain");
 return 0;
-- 
2.34.1

[PATCH 1/3] xen/arm/dom0less-build: Alloc magic pages for Dom0less DomUs from hypervisor

[Henry Wang]

There are use cases (for example using the PV driver) in Dom0less
setup that require Dom0less DomUs start immediately with Dom0, but
initialize XenStore later after Dom0's successful boot and call to
the init-dom0less application.

An error message can seen from the init-dom0less application on
1:1 direct-mapped domains:
```
Allocating magic pages
memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
Error on alloc magic pages
```
This is because currently the magic pages for Dom0less DomUs are
populated by the init-dom0less app through populate_physmap(), and
populate_physmap() automatically assumes gfn == mfn for 1:1 direct
mapped domains. This cannot be true for the magic pages that are
allocated later from the init-dom0less application executed in Dom0.
For domain using statically allocated memory but not 1:1 direct-mapped,
similar error "failed to retrieve a reserved page" can be seen as the
reserved memory list is empty at that time.

To solve above issue, this commit allocates the magic pages for
Dom0less DomUs at the domain construction time. The base address/PFN
of the magic page region will be noted and communicated to the
init-dom0less application in Dom0.

Reported-by: Alec Kwapis 
Suggested-by: Daniel P. Smith 
Signed-off-by: Henry Wang 
---
 tools/libs/guest/xg_dom_arm.c |  1 -
 xen/arch/arm/dom0less-build.c | 22 ++
 xen/include/public/arch-arm.h |  1 +
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c
index 2fd8ee7ad4..8cc7f27dbb 100644
--- a/tools/libs/guest/xg_dom_arm.c
+++ b/tools/libs/guest/xg_dom_arm.c
@@ -25,7 +25,6 @@
 
 #include "xg_private.h"
 
-#define NR_MAGIC_PAGES 4
 #define CONSOLE_PFN_OFFSET 0
 #define XENSTORE_PFN_OFFSET 1
 #define MEMACCESS_PFN_OFFSET 2
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index fb63ec6fd1..40dc85c759 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -834,11 +834,33 @@ static int __init construct_domU(struct domain *d,
 
 if ( kinfo.dom0less_feature & DOM0LESS_XENSTORE )
 {
+struct page_info *magic_pg;
+mfn_t mfn;
+gfn_t gfn;
+
 ASSERT(hardware_domain);
 rc = alloc_xenstore_evtchn(d);
 if ( rc < 0 )
 return rc;
 d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL;
+
+d->max_pages += NR_MAGIC_PAGES;
+magic_pg = alloc_domheap_pages(d, 
get_order_from_pages(NR_MAGIC_PAGES), 0);
+if ( magic_pg == NULL )
+return -ENOMEM;
+
+mfn = page_to_mfn(magic_pg);
+if ( !is_domain_direct_mapped(d) )
+gfn = gaddr_to_gfn(GUEST_MAGIC_BASE);
+else
+gfn = gaddr_to_gfn(mfn_to_maddr(mfn));
+
+rc = guest_physmap_add_pages(d, gfn, mfn, NR_MAGIC_PAGES);
+if ( rc )
+{
+free_domheap_pages(magic_pg, get_order_from_pages(NR_MAGIC_PAGES));
+return rc;
+}
 }
 
 return rc;
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index e167e14f8d..f24e7bbe37 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -475,6 +475,7 @@ typedef uint64_t xen_callback_t;
 
 #define GUEST_MAGIC_BASE  xen_mk_ullong(0x3900)
 #define GUEST_MAGIC_SIZE  xen_mk_ullong(0x0100)
+#define NR_MAGIC_PAGES 4
 
 #define GUEST_RAM_BANKS   2
 
-- 
2.34.1

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 xen  232ee07c23b23fbbafbbf27e475dbbc5b27e4bbb
baseline version:
 xen  23cd1207e7f6ee3e51fb42e11dba8d7cdb28e1e5

Last test of basis   185799  2024-04-25 17:02:07 Z0 days
Testing same since   185801  2024-04-25 21:02:05 Z0 days1 attempts


People who touched revisions under test:
  Alessandro Zucchelli 
  Andrew Cooper 
  Federico Serafini 
  Michal Orzel 
  Simone Ballarin 
  Stefano Stabellini 
  Stewart Hildebrand 

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
   23cd1207e7..232ee07c23  232ee07c23b23fbbafbbf27e475dbbc5b27e4bbb -> smoke

[PATCH v1.1] xen/commom/dt-overlay: Fix missing lock when remove the device

[Henry Wang]

If CONFIG_DEBUG=y, below assertion will be triggered:
(XEN) Assertion 'rw_is_locked(_host_lock)' failed at 
drivers/passthrough/device_tree.c:146
(XEN) [ Xen-4.19-unstable  arm64  debug=y  Not tainted ]
(XEN) CPU:    0
(XEN) PC: 0a257418 iommu_remove_dt_device+0x8c/0xd4
(XEN) LR: 0a2573a0
(XEN) SP: 8000fff7fb30
(XEN) CPSR:   0249 MODE:64-bit EL2h (Hypervisor, handler)
[...]

(XEN) Xen call trace:
(XEN)    [<0a257418>] iommu_remove_dt_device+0x8c/0xd4 (PC)
(XEN)    [<0a2573a0>] iommu_remove_dt_device+0x14/0xd4 (LR)
(XEN)    [<0a20797c>] dt-overlay.c#remove_node_resources+0x8c/0x90
(XEN)    [<0a207f14>] dt-overlay.c#remove_nodes+0x524/0x648
(XEN)    [<0a208460>] dt_overlay_sysctl+0x428/0xc68
(XEN)    [<0a2707f8>] arch_do_sysctl+0x1c/0x2c
(XEN)    [<0a230b40>] do_sysctl+0x96c/0x9ec
(XEN)    [<0a271e08>] traps.c#do_trap_hypercall+0x1e8/0x288
(XEN)    [<0a273490>] do_trap_guest_sync+0x448/0x63c
(XEN)    [<0a25c480>] entry.o#guest_sync_slowpath+0xa8/0xd8
(XEN)
(XEN)
(XEN) 
(XEN) Panic on CPU 0:
(XEN) Assertion 'rw_is_locked(_host_lock)' failed at 
drivers/passthrough/device_tree.c:146
(XEN) 

This is because iommu_remove_dt_device() is called without taking the
dt_host_lock. Fix the issue by taking and releasing the lock properly.

Fixes: 7e5c4a8b86f1 ("xen/arm: Implement device tree node removal 
functionalities")
Signed-off-by: Henry Wang 
---
v1.1:
- Move the unlock position before the check of rc.
---
 xen/common/dt-overlay.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
index 1b197381f6..ab8f43aea2 100644
--- a/xen/common/dt-overlay.c
+++ b/xen/common/dt-overlay.c
@@ -381,7 +381,9 @@ static int remove_node_resources(struct dt_device_node 
*device_node)
 {
 if ( dt_device_is_protected(device_node) )
 {
+write_lock(_host_lock);
 rc = iommu_remove_dt_device(device_node);
+write_unlock(_host_lock);
 if ( rc < 0 )
 return rc;
 }
-- 
2.34.1

Re: [PATCH 11/15] tools/helpers: Add get_overlay

[Henry Wang]

Hi Stewart,

On 4/26/2024 9:45 AM, Stewart Hildebrand wrote:

On 4/24/24 20:43, Henry Wang wrote:

Hi Jan,

On 4/24/2024 2:08 PM, Jan Beulich wrote:

On 24.04.2024 05:34, Henry Wang wrote:

From: Vikram Garhwal 

This user level application copies the overlay dtbo shared by dom0 while doing
overlay node assignment operation. It uses xenstore to communicate with dom0.
More information on the protocol is writtien in docs/misc/overlay.txt file.

Signed-off-by: Vikram Garhwal 
Signed-off-by: Stefano Stabellini 
Signed-off-by: Henry Wang 
---
   tools/helpers/Makefile  |   8 +
   tools/helpers/get_overlay.c | 393 
   2 files changed, 401 insertions(+)
   create mode 100644 tools/helpers/get_overlay.c

As mentioned before on various occasions - new files preferably use dashes as
separators in preference to underscores. You not doing so is particularly
puzzling seeing ...


--- a/tools/helpers/Makefile
+++ b/tools/helpers/Makefile
@@ -12,6 +12,7 @@ TARGETS += init-xenstore-domain
   endif
   ifeq ($(CONFIG_ARM),y)
   TARGETS += init-dom0less
+TARGETS += get_overlay

... patch context here (demonstrating a whopping 3 dashes used in similar
cases).

I am not very sure why Vikram used "_" in the original patch. However I agree you are 
correct. Since I am currently doing the follow up of this series, I will use "-" in v2 as 
suggested. Thanks.

Please also add tools/helpers/get-overlay to .gitignore


Thanks for the reminder! Yes sure I will add it.

Kind regards,
Henry

Re: [PATCH 11/15] tools/helpers: Add get_overlay

[Stewart Hildebrand]

On 4/24/24 20:43, Henry Wang wrote:
> Hi Jan,
> 
> On 4/24/2024 2:08 PM, Jan Beulich wrote:
>> On 24.04.2024 05:34, Henry Wang wrote:
>>> From: Vikram Garhwal 
>>>
>>> This user level application copies the overlay dtbo shared by dom0 while 
>>> doing
>>> overlay node assignment operation. It uses xenstore to communicate with 
>>> dom0.
>>> More information on the protocol is writtien in docs/misc/overlay.txt file.
>>>
>>> Signed-off-by: Vikram Garhwal 
>>> Signed-off-by: Stefano Stabellini 
>>> Signed-off-by: Henry Wang 
>>> ---
>>>   tools/helpers/Makefile  |   8 +
>>>   tools/helpers/get_overlay.c | 393 
>>>   2 files changed, 401 insertions(+)
>>>   create mode 100644 tools/helpers/get_overlay.c
>> As mentioned before on various occasions - new files preferably use dashes as
>> separators in preference to underscores. You not doing so is particularly
>> puzzling seeing ...
>>
>>> --- a/tools/helpers/Makefile
>>> +++ b/tools/helpers/Makefile
>>> @@ -12,6 +12,7 @@ TARGETS += init-xenstore-domain
>>>   endif
>>>   ifeq ($(CONFIG_ARM),y)
>>>   TARGETS += init-dom0less
>>> +TARGETS += get_overlay
>> ... patch context here (demonstrating a whopping 3 dashes used in similar
>> cases).
> 
> I am not very sure why Vikram used "_" in the original patch. However I agree 
> you are correct. Since I am currently doing the follow up of this series, I 
> will use "-" in v2 as suggested. Thanks.

Please also add tools/helpers/get-overlay to .gitignore

[linux-linus test] 185798: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-armhf-armhf-xl   8 xen-boot fail REGR. vs. 185768
 test-armhf-armhf-xl-raw   8 xen-boot fail REGR. vs. 185768

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-examine  8 reboot   fail in 185791 pass in 185798
 test-armhf-armhf-xl-multivcpu  8 xen-boot  fail pass in 185791
 test-armhf-armhf-xl-credit2   8 xen-boot   fail pass in 185791
 test-armhf-armhf-xl-rtds  8 xen-boot   fail pass in 185791

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

version targeted for testing:
 linuxe88c4cfcb7b888ac374916806f86c17d8ecaeb67
baseline version:
 linux71b1543c83d65af8215d7558d70fc2ecbee77dcf

Last test of basis   185768  2024-04-23 07:46:10 Z2 days
Failing since185779  2024-04-24 00:13:50 Z2 days4 attempts
Testing same since   185791  2024-04-25 01:43:09 Z0 days2 attempts


People who touched revisions under test:
  David Howells 
  David Sterba 
  Johannes Thumshirn 
  Linus Torvalds 
  Naohiro Aota 
  Neal Gompa 
  Paulo Alcantara (Red Hat) 
  Paulo Alcantara 
  Qu Wenruo 
  Ronnie Sahlberg 
  Steve French 
  Sweet Tea Dorminy 
  Takayuki Nagata 
  Tom Talpey 

Re: [PATCH 1/7] x86/p2m: Add braces for better code clarity

[Stefano Stabellini]

On Wed, 24 Apr 2024, Petr Beneš wrote:
> From: Petr Beneš 
> 
> No functional change.
> 
> Signed-off-by: Petr Beneš 

Reviewed-by: Stefano Stabellini 

[PATCH v2] docs/misra: add R21.6 R21.9 R21.10 R21.14 R21.15 R21.16

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini 
---
Changes in v2:
- remove trailing whitespaces
- add rules 21.9 and 21.10
- remove deviations.rst deviations (to be done separately if required)
- add a note explaning that Xen has no standard library
---
 docs/misra/rules.rst | 60 
 1 file changed, 60 insertions(+)

diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
index b7b447e152..661879a3de 100644
--- a/docs/misra/rules.rst
+++ b/docs/misra/rules.rst
@@ -652,12 +652,72 @@ maintainers if you want to suggest a change.
declared
  - See comment for Rule 21.1
 
+   * - `Rule 21.6 
`_
+ - Required
+ - The Standard Library input/output routines shall not be used
+ - Xen doesn't provide, use, or link against any Standard Library.
+   Xen implements itself a few functions with names that match the
+   corresponding function names of the Standard Library for
+   developers' convenience. These functions are part of the Xen code
+   and subject to analysis.
+
+   * - `Rule 21.9 
`_
+ - Required
+ - The library functions bsearch and qsort of  shall not be used
+ - Xen doesn't provide, use, or link against any Standard Library.
+   Xen implements itself a few functions with names that match the
+   corresponding function names of the Standard Library for
+   developers' convenience. These functions are part of the Xen code
+   and subject to analysis.
+
+   * - `Rule 21.10 
`_
+ - Required
+ - The Standard Library time and date routines shall not be used
+ - Xen doesn't provide, use, or link against any Standard Library.
+   Xen implements itself a few functions with names that match the
+   corresponding function names of the Standard Library for
+   developers' convenience. These functions are part of the Xen code
+   and subject to analysis.
+
* - `Rule 21.13 
`_
  - Mandatory
  - Any value passed to a function in  shall be representable as an
unsigned char or be the value EOF
  -
 
+   * - `Rule 21.14 
`_
+ - Required
+ - The Standard Library function memcmp shall not be used to compare
+   null terminated strings
+ - Xen doesn't provide, use, or link against any Standard Library.
+   Xen implements itself a few functions with names that match the
+   corresponding function names of the Standard Library for
+   developers' convenience. These functions are part of the Xen code
+   and subject to analysis.
+
+   * - `Rule 21.15 
`_
+ - Required
+ - The pointer arguments to the Standard Library functions memcpy,
+   memmove and memcmp shall be pointers to qualified or unqualified
+   versions of compatible types
+ - Xen doesn't provide, use, or link against any Standard Library.
+   Xen implements itself a few functions with names that match the
+   corresponding function names of the Standard Library for
+   developers' convenience. These functions are part of the Xen code
+   and subject to analysis.
+
+   * - `Rule 21.16 
`_
+ - Required
+ - The pointer arguments to the Standard Library function memcmp
+   shall point to either a pointer type, an essentially signed type,
+   an essentially unsigned type, an essentially Boolean type or an
+   essentially enum type
+ - void* arguments are allowed. Xen doesn't provide, use, or link
+   against any Standard Library.  Xen implements itself a few
+   functions with names that match the corresponding function names
+   of the Standard Library for developers' convenience. These
+   functions are part of the Xen code and subject to analysis.
+
* - `Rule 21.17 
`_
  - Mandatory
  - Use of the string handling functions from  shall not result in
-- 
2.25.1

Re: [PATCH v2 1/1] xen/arm64: entry: Add missing code symbol annotations

[Stefano Stabellini]

On Tue, 16 Apr 2024, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" 
> 
> Use the generic xen/linkage.h macros when and add missing
^ when what?

> code symbol annotations.
> 
> Signed-off-by: Edgar E. Iglesias 

I am looking at the implementation of FUNC and as far as I can tell
there is no change compared to ENTRY. So from that point of view we are
good. I wonder if we should keep using "ENTRY" because it is nice to
mark explicitely the entry points as such but at the same time I am also
OK with this. I'll let the other ARM maintainers decide.

On the other hand, FUNC_LOCAL does introduce a change: it adds a .align
everywhere. Should not be harmful?

With the commit message fixed:

Reviewed-by: Stefano Stabellini 


> ---
>  xen/arch/arm/arm64/entry.S | 72 +-
>  1 file changed, 48 insertions(+), 24 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index f963c923bb..af9a592cae 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -289,21 +289,25 @@
>  b   do_bad_mode
>  .endm
>  
> -hyp_sync_invalid:
> +FUNC_LOCAL(hyp_sync_invalid)
>  entry   hyp=1
>  invalid BAD_SYNC
> +END(hyp_sync_invalid)
>  
> -hyp_irq_invalid:
> +FUNC_LOCAL(hyp_irq_invalid)
>  entry   hyp=1
>  invalid BAD_IRQ
> +END(hyp_irq_invalid)
>  
> -hyp_fiq_invalid:
> +FUNC_LOCAL(hyp_fiq_invalid)
>  entry   hyp=1
>  invalid BAD_FIQ
> +END(hyp_fiq_invalid)
>  
> -hyp_error_invalid:
> +FUNC_LOCAL(hyp_error_invalid)
>  entry   hyp=1
>  invalid BAD_ERROR
> +END(hyp_error_invalid)
>  
>  /*
>   * SError received while running in the hypervisor mode.
> @@ -313,11 +317,12 @@ hyp_error_invalid:
>   * simplicity, as SError should be rare and potentially fatal,
>   * all interrupts are kept masked.
>   */
> -hyp_error:
> +FUNC_LOCAL(hyp_error)
>  entry   hyp=1
>  mov x0, sp
>  bl  do_trap_hyp_serror
>  exithyp=1
> +END(hyp_error)
>  
>  /*
>   * Synchronous exception received while running in the hypervisor mode.
> @@ -327,7 +332,7 @@ hyp_error:
>   * some of them. So we want to inherit the state from the interrupted
>   * context.
>   */
> -hyp_sync:
> +FUNC_LOCAL(hyp_sync)
>  entry   hyp=1
>  
>  /* Inherit interrupts */
> @@ -338,6 +343,7 @@ hyp_sync:
>  mov x0, sp
>  bl  do_trap_hyp_sync
>  exithyp=1
> +END(hyp_sync)
>  
>  /*
>   * IRQ received while running in the hypervisor mode.
> @@ -352,7 +358,7 @@ hyp_sync:
>   * would require some rework in some paths (e.g. panic, livepatch) to
>   * ensure the ordering is enforced everywhere.
>   */
> -hyp_irq:
> +FUNC_LOCAL(hyp_irq)
>  entry   hyp=1
>  
>  /* Inherit D, A, F interrupts and keep I masked */
> @@ -365,8 +371,9 @@ hyp_irq:
>  mov x0, sp
>  bl  do_trap_irq
>  exithyp=1
> +END(hyp_irq)
>  
> -guest_sync:
> +FUNC_LOCAL(guest_sync)
>  /*
>   * Save x0, x1 in advance
>   */
> @@ -413,8 +420,9 @@ fastpath_out_workaround:
>  mov x1, xzr
>  eret
>  sb
> +END(guest_sync)
>  
> -wa2_ssbd:
> +FUNC_LOCAL(wa2_ssbd)
>  #ifdef CONFIG_ARM_SSBD
>  alternative_cb arm_enable_wa2_handling
>  b   wa2_end
> @@ -450,42 +458,55 @@ wa2_end:
>  mov x0, xzr
>  eret
>  sb
> -guest_sync_slowpath:
> +END(wa2_ssbd)
> +
> +FUNC_LOCAL(guest_sync_slowpath)
>  /*
>   * x0/x1 may have been scratch by the fast path above, so avoid
>   * to save them.
>   */
>  guest_vector compat=0, iflags=IFLAGS__AI_, trap=guest_sync, 
> save_x0_x1=0
> +END(guest_sync_slowpath)
>  
> -guest_irq:
> +FUNC_LOCAL(guest_irq)
>  guest_vector compat=0, iflags=IFLAGS__A__, trap=irq
> +END(guest_irq)
>  
> -guest_fiq_invalid:
> +FUNC_LOCAL(guest_fiq_invalid)
>  entry   hyp=0, compat=0
>  invalid BAD_FIQ
> +END(guest_fiq_invalid)
>  
> -guest_error:
> +FUNC_LOCAL(guest_error)
>  guest_vector compat=0, iflags=IFLAGS__AI_, trap=guest_serror
> +END(guest_error)
>  
> -guest_sync_compat:
> +FUNC_LOCAL(guest_sync_compat)
>  guest_vector compat=1, iflags=IFLAGS__AI_, trap=guest_sync
> +END(guest_sync_compat)
>  
> -guest_irq_compat:
> +FUNC_LOCAL(guest_irq_compat)
>  guest_vector compat=1, iflags=IFLAGS__A__, trap=irq
> +END(guest_irq_compat)
>  
> -guest_fiq_invalid_compat:
> +FUNC_LOCAL(guest_fiq_invalid_compat)
>  entry   hyp=0, compat=1
>  invalid BAD_FIQ
> +END(guest_fiq_invalid_compat)
>  
> -guest_error_compat:
> +FUNC_LOCAL(guest_error_compat)
>  guest_vector compat=1, iflags=IFLAGS__AI_, trap=guest_serror
> +END(guest_error_compat)
>  
> -ENTRY(return_to_new_vcpu32)
> +FUNC(return_to_new_vcpu32)
>  exithyp=0, compat=1
> -ENTRY(return_to_new_vcpu64)
> 

Re: [PATCH] xen: Use -Wuninitialized and -Winit-self

[Stefano Stabellini]

On Thu, 28 Dec 2023, Andrew Cooper wrote:
> The use of uninitialised data is undefined behaviour.  At -O2 with trivial
> examples, both Clang and GCC delete the variable, and in the case of a
> function return, the caller gets whatever was stale in %rax prior to the call.
> 
> Clang includes -Wuninitialized within -Wall, but GCC only includes it in
> -Wextra, which is not used by Xen at this time.
> 
> Furthermore, the specific pattern of assigning a variable to itself in its
> declaration is only diagnosed by GCC with -Winit-self.  Clang does diagnoise
> simple forms of this pattern with a plain -Wuninitialized, but it fails to
> diagnose the instances in Xen that GCC manages to find.
> 
> GCC, with -Wuninitialized and -Winit-self notices:
> 
>   arch/x86/time.c: In function ‘read_pt_and_tsc’:
>   arch/x86/time.c:297:14: error: ‘best’ is used uninitialized in this 
> function [-Werror=uninitialized]
> 297 | uint32_t best = best;
> |  ^~~~
>   arch/x86/time.c: In function ‘read_pt_and_tmcct’:
>   arch/x86/time.c:1022:14: error: ‘best’ is used uninitialized in this 
> function [-Werror=uninitialized]
>1022 | uint64_t best = best;
> |  ^~~~
> 
> and both have logic paths where best can be returned while uninitalised.  In
> both cases, initialise to ~0 like the associated *_min variable which also
> gates updating best.
> 
> Fixes: 23658e823238 ("x86/time: further improve TSC / CPU freq calibration 
> accuracy")
> Fixes: 3f3906b462d5 ("x86/APIC: calibrate against platform timer when 
> possible")
> Signed-off-by: Andrew Cooper 

Reviewed-by: Stefano Stabellini 

Re: [PATCH] public: xen: Define missing guest handle for int32_t

[Stefano Stabellini]

On Mon, 22 Apr 2024, Julien Grall wrote:
> Hi Stefano,
> 
> On 17/04/2024 19:49, Stefano Stabellini wrote:
> > On Wed, 17 Apr 2024, Julien Grall wrote:
> > > Hi Michal,
> > > 
> > > On 17/04/2024 13:14, Michal Orzel wrote:
> > > > Commit afab29d0882f ("public: s/int/int32_t") replaced int with int32_t
> > > > in XEN_GUEST_HANDLE() in memory.h but there is no guest handle defined
> > > > for it. This results in a build failure. Example on Arm:
> > > > 
> > > > ./include/public/arch-arm.h:205:41: error: unknown type name
> > > > ‘__guest_handle_64_int32_t’
> > > > 205 | #define __XEN_GUEST_HANDLE(name)__guest_handle_64_ ##
> > > > name
> > > > | ^~
> > > > ./include/public/arch-arm.h:206:41: note: in expansion of macro
> > > > ‘__XEN_GUEST_HANDLE’
> > > > 206 | #define XEN_GUEST_HANDLE(name)
> > > > __XEN_GUEST_HANDLE(name)
> > > > | ^~
> > > > ./include/public/memory.h:277:5: note: in expansion of macro
> > > > ‘XEN_GUEST_HANDLE’
> > > > 277 | XEN_GUEST_HANDLE(int32_t) errs;
> > > > 
> > > > Fix it. Also, drop guest handle definition for int given no further use.
> > > > 
> > > > Fixes: afab29d0882f ("public: s/int/int32_t")
> > > > Signed-off-by: Michal Orzel 
> > 
> > Reviewed-by: Stefano Stabellini 
> > 
> > 
> > > So it turned out that I committed v1 from Stefano. I was meant to commit
> > > the
> > > patch at all, but I think I started with a dirty staging :(. Sorry for
> > > that.
> > > 
> > > I have reverted Stefano's commit for now so we can take the correct patch.
> > > 
> > > Now, from my understanding, Andrew suggested on Matrix that this solution
> > > may
> > > actually be a good way to handle GUEST_HANLDEs (they were removed in v2).
> > > Maybe this can be folded in Stefano's patch?
> > 
> > v1 together with Michal's fix is correct. Also v2 alone is correct, or
> > v2 with Michal's fix is also correct.
> 
> I am slightly confused, v2 + Michal's fix means that XEN_GUEST_HANDLE(int) is
> removed and we introduce XEN_GUEST_INT(int32_t) with no user. So wouldn't this

You are right I apologize. I looked at Michal's patch too quickly and
I thought it was just adding XEN_GUEST_INT(int32_t) without removing
anything.

In that case, if you are OK with it, please ack and commit v2 only.

Re: [XEN PATCH v1 13/15] x86: wire cpu_has_{svm/vmx}_* to false when svm/vmx not enabled

[Stefano Stabellini]

On Thu, 18 Apr 2024, Sergiy Kibrik wrote:
> 16.04.24 16:26, Andrew Cooper:
> > I'm afraid this is going in an unhelpful direction.  We want to move
> > both of these files to be local to arch/x86/hvm/{vmx,svm}/.
> > 
> > cpu_has_svm_* isn't actually used outside of svm/; only the plain
> > SVM_FEATURE_* constants are, and that's only because they're not
> > expressed as plain cpu features yet.
> > 
> > cpu_has_vmx_* has a few more users, but most are unlikely to remain in
> > this form.  One critical set of changes to fix vulnerabilities in
> > nested-virt is to make almost of of these decisions based on per-domain
> > state, not host state.  The aspects which are host state should be in
> > regular cpu features.
> > 
> > I already volunteered to sort out the SEV feature leaf properly, and I
> > was going to do the SVM leaf while I was at it.  If you can wait a few
> > days, I might be able to make half of this problem disappear.
> 
> I guess it can wait, surely if a better solution is to be crafted at the end.
> 
> Stefano, what's your opinion on that?

I think Andrew's suggested direction is cleaner. We can certainly wait a
few days for Andrew to make progress. We can also follow Andrew's
suggestion in the next version of the series ourselves.

Re: [PATCH v4 0/5] DOMCTL-based guest magic region allocation for 11 domUs

[Stefano Stabellini]

On Thu, 18 Apr 2024, Daniel P. Smith wrote:
> On 4/9/24 00:53, Henry Wang wrote:
> > An error message can seen from the init-dom0less application on
> > direct-mapped 1:1 domains:
> > ```
> > Allocating magic pages
> > memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
> > Error on alloc magic pages
> > ```
> > 
> > This is because populate_physmap() automatically assumes gfn == mfn
> > for direct mapped domains. This cannot be true for the magic pages
> > that are allocated later for 1:1 Dom0less DomUs from the init-dom0less
> > helper application executed in Dom0. For domain using statically
> > allocated memory but not 1:1 direct-mapped, similar error "failed to
> > retrieve a reserved page" can be seen as the reserved memory list
> > is empty at that time.
> > 
> > This series tries to fix this issue using a DOMCTL-based approach,
> > because for 1:1 direct-mapped domUs, we need to avoid the RAM regions
> > and inform the toolstack about the region found by hypervisor for
> > mapping the magic pages. Patch 1 introduced a new DOMCTL to get the
> > guest memory map, currently only used for the magic page regions.
> > Patch 2 generalized the extended region finding logic so that it can
> > be reused for other use cases such as finding 1:1 domU magic regions.
> > Patch 3 uses the same approach as finding the extended regions to find
> > the guest magic page regions for direct-mapped DomUs. Patch 4 avoids
> > hardcoding all base addresses of guest magic region in the init-dom0less
> > application by consuming the newly introduced DOMCTL. Patch 5 is a
> > simple patch to do some code duplication clean-up in xc.
> 
> Hey Henry,
> 
> To help provide some perspective, these issues are not experienced with
> hyperlaunch. This is because we understood early on that you cannot move a
> lightweight version of the toolstack into hypervisor init and not provide a
> mechanism to communicate what it did to the runtime control plane. We
> evaluated the possible mechanism, to include introducing a new hypercall op,
> and ultimately settled on using hypfs. The primary reason is this information
> is static data that, while informative later, is only necessary for the
> control plane to understand the state of the system. As a result, hyperlaunch
> is able to allocate any and all special pages required as part of domain
> construction and communicate their addresses to the control plane. As for XSM,
> hypfs is already protected and at this time we do not see any domain builder
> information needing to be restricted separately from the data already present
> in hypfs.
> 
> I would like to make the suggestion that instead of continuing down this path,
> perhaps you might consider adopting the hyperlaunch usage of hypfs. Then
> adjust dom0less domain construction to allocate the special pages at
> construction time. The original hyperlaunch series includes a patch that
> provides the helper app for the xenstore announcement. And I can provide you
> with updated versions if that would be helpful.

I also think that the new domctl is not needed and that the dom0less
domain builder should allocate the magic pages. On ARM, we already
allocate HVM_PARAM_CALLBACK_IRQ during dom0less domain build and set
HVM_PARAM_STORE_PFN to ~0ULL. I think it would be only natural to extend
that code to also allocate the magic pages and set HVM_PARAM_STORE_PFN
(and others) correctly. If we do it that way it is simpler and
consistent with the HVM_PARAM_CALLBACK_IRQ allocation, and we don't even
need hypfs. Currently we do not enable hypfs in our safety
certifiability configuration.

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 xen  23cd1207e7f6ee3e51fb42e11dba8d7cdb28e1e5
baseline version:
 xen  6d5111b10e084d841284a56e962c61ad274f589e

Last test of basis   185797  2024-04-25 13:08:37 Z0 days
Testing same since   185799  2024-04-25 17:02:07 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Anthony PERARD 
  Jan Beulich 
  Roger Pau Monné 

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
   6d5111b10e..23cd1207e7  23cd1207e7f6ee3e51fb42e11dba8d7cdb28e1e5 -> smoke

[PATCH] xen/spinlock: use correct pointer

[Stewart Hildebrand]

The ->profile member is at different offsets in struct rspinlock and
struct spinlock. When initializing the profiling bits of an rspinlock,
an unrelated member in struct rspinlock was being overwritten, leading
to mild havoc. Use the correct pointer.

Fixes: b053075d1a7b ("xen/spinlock: make struct lock_profile rspinlock_t aware")
Signed-off-by: Stewart Hildebrand 
---
 xen/common/spinlock.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 558ea7ac3518..28c6e9d3ac60 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -789,7 +789,11 @@ static int __init cf_check lock_prof_init(void)
 {
 (*q)->next = lock_profile_glb_q.elem_q;
 lock_profile_glb_q.elem_q = *q;
-(*q)->ptr.lock->profile = *q;
+
+if ( (*q)->is_rlock )
+(*q)->ptr.rlock->profile = *q;
+else
+(*q)->ptr.lock->profile = *q;
 }
 
 _lock_profile_register_struct(LOCKPROF_TYPE_GLOBAL,

base-commit: 23cd1207e7f6ee3e51fb42e11dba8d7cdb28e1e5
-- 
2.43.2

Re: [XEN PATCH] automation/eclair: reorganize pipelines

[Stefano Stabellini]

On Thu, 25 Apr 2024, Federico Serafini wrote:
> On 25/04/24 02:06, Stefano Stabellini wrote:
> > On Tue, 23 Apr 2024, Federico Serafini wrote:
> > > From: Simone Ballarin 
> > > 
> > > Introduce accepted_guidelines.sh: a script to autogenerate the
> > > configuration file accepted.ecl from docs/misra/rules.rst which enables
> > > all accepted guidelines.
> > > 
> > > Introduce monitored.ecl: a manual selection of accepted guidelines
> > > which are clean or almost clean, it is intended to be used for the
> > > analyses triggered by commits.
> > > 
> > > Reorganize tagging.ecl:
> > >-Remove "accepted" tags: keeping track of accepted guidelines tagging
> > > them as "accepted" in the configuration file tagging.ecl is no
> > > longer needed since docs/rules.rst is keeping track of them.
> > >-Tag more guidelines as clean.
> > > 
> > > Reorganize eclair pipelines:
> > >- Set1, Set2, Set3 are now obsolete: remove the corresponding
> > >  pipelines and ecl files.
> > >- Amend scheduled eclair pipeline to use accepted.ecl.
> > >- Amend triggered eclair pipeline to use monitored.ecl.
> > > 
> > > Rename and improve action_check_clean_regressions.sh to print a
> > > diagnostic in case a commit introduces a violation of a clean guideline.
> > > 
> > > An example of diagnostic is the following:
> > > 
> > > Failure: 13 regressions found for clean guidelines
> > >service MC3R1.R8.2: (required) Function types shall be in prototype
> > > form with named parameters:
> > > violation: 13
> > > 
> > > Signed-off-by: Simone Ballarin 
> > > Signed-off-by: Federico Serafini 
> > > Signed-off-by: Alessandro Zucchelli 
> > 
> > Fantastic work, thank you!
> > 
> > Reviewed-by: Stefano Stabellini 
> > 
> > Is this patch safe to commit now? Or would it cause gitlab-ci breakage?
> 
> Yes, it is safe because the ECLAIR analysis is still allowed to fail.
> Committing this patch wouldn't break the CI but it will highlight some
> regressions with the orange badge and the following messages:

OK thanks, I'll commit it


> arm:
> 
> Failure: 5 regressions found for clean guidelines
>   service MC3R1.R1.1: (required) The program shall contain no violations of
> the standard C syntax and constraints, and shall not exceed the
> implementation's translation limits:
>violation: 5
> 
> x86:
> 
> Failure: 2 regressions found for clean guidelines
>   service MC3R1.R8.2: (required) Function types shall be in prototype form
> with named parameters:
>violation: 2
> 
> (George just sent a patch to address the regressions of Rule 8.2.)

As soon as we solve these two issues, I think we should remove the
allow_failure: true from the two ECLAIR jobs scanning clean
guidelines. More on this below.


> > 
> > One question below.
> > 
> > 
> > > -
> > >   
> > >   # Clean guidelines #
> > >   
> > > -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.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"
> > > +-service_selector={clean_guidelines_common,"MC3R1.D1.1||MC3R1.D2.1||MC3R1.D4.1||MC3R1.D4.11||MC3R1.D4.14||MC3R1.R1.1||MC3R1.R11.7||MC3R1.R11.9||MC3R1.R12.5||MC3R1.R1.3||MC3R1.R1.4||MC3R1.R14.1||MC3R1.R16.7||MC3R1.R17.1||MC3R1.R17.3||MC3R1.R17.4||MC3R1.R17.5||MC3R1.R17.6||MC3R1.R20.13||MC3R1.R20.14||MC3R1.R20.4||MC3R1.R20.9||MC3R1.R21.13||MC3R1.R21.19||MC3R1.R21.21||MC3R1.R2.2||MC3R1.R22.2||MC3R1.R22.4||MC3R1.R22.5||MC3R1.R22.6||MC3R1.R2.6||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.R7.4||MC3R1.R8.1||MC3R1.R8.10||MC3R1.R8.12||MC3R1.R8.14||MC3R1.R8.2||MC3R1.R8.5||MC3R1.R8.6||MC3R1.R8.8||MC3R1.R9.2||MC3R1.R9.3||MC3R1.R9.4||MC3R1.R9.5"
> > >   }
> > 
> > Is this list different from monitored.ecl? If so, why? If not, maybe we
> > don't need to repeat the list here as well?
> 
> Quick answer: this list is different from monitored.ecl and the two
> lists must coexist.
> 
> Here, we are "tagging" some guidelines as "clean":
> this list is crucial and will be (manually) updated every time a new
> guideline reaches 0 violations, it shall not be removed because this tag
> allows ECLAIR to print a diagnostic and fail in case unjustified
> violations are found for the tagged guidelines.
> 
> The monitored.ecl is the list of guidelines which are analyzed at each
> commit: the list shall include all the 

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 xen  31d6b5b4a7c84818294dacca7a07e92f52393c9d
baseline version:
 xen  410ef3343924b5a3928bbe8e392491992b322cf0

Last test of basis   185786  2024-04-24 13:05:04 Z1 days
Testing same since   185794  2024-04-25 05:57:18 Z0 days1 attempts


People who touched revisions under test:
  George Dunlap 
  Jan Beulich 
  Luca Fancellu 
  Michal Orzel 
  Nick Rosbrook 
  Penny Zheng 
  Stefano Stabellini 
  Tobias Fitschen 

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

Re: [PATCH v3 7/8] gzip: move bitbuffer into gunzip state

[Andrew Cooper]

On 24/04/2024 5:34 pm, Daniel P. Smith wrote:
> Signed-off-by: Daniel P. Smith 

Acked-by: Andrew Cooper 

> diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
> index bec8801df487..8da14880cfbe 100644
> --- a/xen/common/gzip/inflate.c
> +++ b/xen/common/gzip/inflate.c
> @@ -1017,8 +1014,8 @@ static int __init inflate(struct gunzip_state *s)
>  /* Undo too much lookahead. The next read will be byte aligned so we
>   * can discard unused bits in the last meaningful byte.
>   */
> -while (bk >= 8) {
> -bk -= 8;
> +while (s->bk >= 8) {
> +s->bk -= 8;
>  s->inptr--;
>  }

Isn't it just me, but isn't this just:

    s->inptr -= (s->bk >> 3);
    s->bk &= 7;

?

~Andrew

Re: [PATCH v3 8/8] gzip: move crc state into gunzip state

[Andrew Cooper]

On 24/04/2024 5:34 pm, Daniel P. Smith wrote:
> Move the crc and its state into struct gunzip_state. In the process, expand 
> the
> only use of CRC_VALUE as it is hides what is being compared.

"All variables here should be uint32_t rather than unsigned long, which
halves the storage space required."

> diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
> index 8da14880cfbe..dc940e59d853 100644
> --- a/xen/common/gzip/inflate.c
> +++ b/xen/common/gzip/inflate.c
> @@ -1069,11 +1065,11 @@ static void __init makecrc(void)
>  if (k & 1)
>  c ^= e;
>  }
> -crc_32_tab[i] = c;
> +s->crc_32_tab[i] = c;
>  }
>  
>  /* this is initialized here so this code could reside in ROM */
> -crc = (ulg)0xUL; /* shift register contents */
> +s->crc = (ulg)~0u; /* shift register contents */

The (ulg) wasn't ever necessary, and can be dropped as part of this cleanup.

Can fix on commit.

~Andrew

Re: [PATCH v3 1/8] gzip: clean up comments and fix code alignment

[Andrew Cooper]

On 24/04/2024 5:34 pm, Daniel P. Smith wrote:
> This commit cleans up the comments and fixes the code alignment using Xen
> coding style. This is done to make the code more legible before refactoring.
>
> Signed-off-by: Daniel P. Smith 

Acked-by: Andrew Cooper 

Re: [PATCH 1/2] tools/{c,o}xenstored: Don't link against libsystemd

[Andrew Cooper]

On 25/04/2024 7:06 pm, Anthony PERARD wrote:
> On Thu, Apr 25, 2024 at 06:32:15PM +0100, Andrew Cooper wrote:
>> libsystemd is a giant dependency for one single function, but in the wake of
>> the xz backdoor, it turns out that even systemd leadership recommend against
>> linking against libsystemd for sd_notify().
>>
>> Since commit 7b61011e1450 ("tools: make xenstore domain easy configurable") 
>> in
>> Xen 4.8, the launch-xenstore script invokes systemd-notify directly, so its
> That's not enough, it's needs to be `systemd-notify --ready` to be a
> replacement for sd_notify(READY=1).
>
>> not even necessary for the xenstored's to call sd_notify() themselves.
> So, sd_notify() or equivalent is still necessary.
>
>> Therefore, just drop the calls to sd_notify() and stop linking against
>> libsystemd.
> Sounds good, be we need to replace the call by something like:
> echo READY=1 > $NOTIFY_SOCKET
> implemented in C and ocaml. Detail to be checked.
>
> Otherwise, things won't work.

Hmm.  It worked in XenRT when stripping this all out, but that is
extremely unintuitive behaviour for `systemd-notify --booted`, seeing as
it's entirely different to --ready.

I've got no interest in keeping the C around, but if:

[ -n "$NOTIFY_SOCKET" ] && echo READY=1 > $NOTIFY_SOCKET

works, then can't we just use that after waiting for the the pidfile ?

~Andrew

Re: [PATCH 1/2] tools/{c,o}xenstored: Don't link against libsystemd

[Anthony PERARD]

On Thu, Apr 25, 2024 at 06:32:15PM +0100, Andrew Cooper wrote:
> libsystemd is a giant dependency for one single function, but in the wake of
> the xz backdoor, it turns out that even systemd leadership recommend against
> linking against libsystemd for sd_notify().
> 
> Since commit 7b61011e1450 ("tools: make xenstore domain easy configurable") in
> Xen 4.8, the launch-xenstore script invokes systemd-notify directly, so its

That's not enough, it's needs to be `systemd-notify --ready` to be a
replacement for sd_notify(READY=1).

> not even necessary for the xenstored's to call sd_notify() themselves.

So, sd_notify() or equivalent is still necessary.

> Therefore, just drop the calls to sd_notify() and stop linking against
> libsystemd.

Sounds good, be we need to replace the call by something like:
echo READY=1 > $NOTIFY_SOCKET
implemented in C and ocaml. Detail to be checked.

Otherwise, things won't work.

Thanks,

-- 
Anthony PERARD

Re: [PATCH] CI: Drop glibc-i386 from the build containers

[Stefano Stabellini]

On Thu, 24 Apr 2024, Andrew Cooper wrote:
> Xen 4.14 no longer runs in Gitlab CI.  Drop the dependency to shrink the build
> containers a little.
> 
> Signed-off-by: Andrew Cooper 

Acked-by: Stefano Stabellini 


> ---
> CC: Anthony PERARD 
> CC: Stefano Stabellini 
> CC: Michal Orzel 
> CC: Doug Goldstein 
> CC: Roger Pau Monné 
> ---
>  automation/build/archlinux/current.dockerfile| 2 --
>  automation/build/centos/7.dockerfile | 2 --
>  automation/build/debian/bookworm.dockerfile  | 2 --
>  automation/build/debian/jessie.dockerfile| 2 --
>  automation/build/debian/stretch.dockerfile   | 2 --
>  automation/build/fedora/29.dockerfile| 2 --
>  automation/build/suse/opensuse-leap.dockerfile   | 2 --
>  automation/build/suse/opensuse-tumbleweed.dockerfile | 2 --
>  automation/build/ubuntu/bionic.dockerfile| 2 --
>  automation/build/ubuntu/focal.dockerfile | 2 --
>  automation/build/ubuntu/trusty.dockerfile| 2 --
>  automation/build/ubuntu/xenial.dockerfile| 2 --
>  12 files changed, 24 deletions(-)
> 
> diff --git a/automation/build/archlinux/current.dockerfile 
> b/automation/build/archlinux/current.dockerfile
> index d974a1434fd5..3e37ab5c40c1 100644
> --- a/automation/build/archlinux/current.dockerfile
> +++ b/automation/build/archlinux/current.dockerfile
> @@ -19,8 +19,6 @@ RUN pacman -S --refresh --sysupgrade --noconfirm 
> --noprogressbar --needed \
>  iasl \
>  inetutils \
>  iproute \
> -# lib32-glibc for Xen < 4.15
> -lib32-glibc \
>  libaio \
>  libcacard \
>  libgl \
> diff --git a/automation/build/centos/7.dockerfile 
> b/automation/build/centos/7.dockerfile
> index ab450f0b3a0e..1cdc16fc05f9 100644
> --- a/automation/build/centos/7.dockerfile
> +++ b/automation/build/centos/7.dockerfile
> @@ -32,8 +32,6 @@ RUN yum -y update \
>  yajl-devel \
>  pixman-devel \
>  glibc-devel \
> -# glibc-devel.i686 for Xen < 4.15
> -glibc-devel.i686 \
>  make \
>  binutils \
>  git \
> diff --git a/automation/build/debian/bookworm.dockerfile 
> b/automation/build/debian/bookworm.dockerfile
> index 459f8e30bdc6..d893218fc4bd 100644
> --- a/automation/build/debian/bookworm.dockerfile
> +++ b/automation/build/debian/bookworm.dockerfile
> @@ -31,8 +31,6 @@ RUN apt-get update && \
>  bin86 \
>  bcc \
>  liblzma-dev \
> -# libc6-dev-i386 for Xen < 4.15
> -libc6-dev-i386 \
>  libnl-3-dev \
>  ocaml-nox \
>  libfindlib-ocaml-dev \
> diff --git a/automation/build/debian/jessie.dockerfile 
> b/automation/build/debian/jessie.dockerfile
> index 32fc952fbc2d..308675cac150 100644
> --- a/automation/build/debian/jessie.dockerfile
> +++ b/automation/build/debian/jessie.dockerfile
> @@ -37,8 +37,6 @@ RUN apt-get update && \
>  bin86 \
>  bcc \
>  liblzma-dev \
> -# libc6-dev-i386 for Xen < 4.15
> -libc6-dev-i386 \
>  libnl-3-dev \
>  ocaml-nox \
>  libfindlib-ocaml-dev \
> diff --git a/automation/build/debian/stretch.dockerfile 
> b/automation/build/debian/stretch.dockerfile
> index e2706a8f3589..59794ed4677b 100644
> --- a/automation/build/debian/stretch.dockerfile
> +++ b/automation/build/debian/stretch.dockerfile
> @@ -38,8 +38,6 @@ RUN apt-get update && \
>  bin86 \
>  bcc \
>  liblzma-dev \
> -# libc6-dev-i386 for Xen < 4.15
> -libc6-dev-i386 \
>  libnl-3-dev \
>  ocaml-nox \
>  libfindlib-ocaml-dev \
> diff --git a/automation/build/fedora/29.dockerfile 
> b/automation/build/fedora/29.dockerfile
> index 42a87ce6c84b..f473ae13e7c1 100644
> --- a/automation/build/fedora/29.dockerfile
> +++ b/automation/build/fedora/29.dockerfile
> @@ -21,8 +21,6 @@ RUN dnf -y install \
>  yajl-devel \
>  pixman-devel \
>  glibc-devel \
> -# glibc-devel.i686 for Xen < 4.15
> -glibc-devel.i686 \
>  make \
>  binutils \
>  git \
> diff --git a/automation/build/suse/opensuse-leap.dockerfile 
> b/automation/build/suse/opensuse-leap.dockerfile
> index e1ec38a41445..48d0d50d005d 100644
> --- a/automation/build/suse/opensuse-leap.dockerfile
> +++ b/automation/build/suse/opensuse-leap.dockerfile
> @@ -28,8 +28,6 @@ RUN zypper install -y --no-recommends \
>  ghostscript \
>  glib2-devel \
>  glibc-devel \
> -# glibc-devel-32bit for Xen < 4.15
> -glibc-devel-32bit \
>  gzip \
>  hostname \
>  libaio-devel \
> diff --git a/automation/build/suse/opensuse-tumbleweed.dockerfile 
> b/automation/build/suse/opensuse-tumbleweed.dockerfile
> index f00e03eda7b1..53542ba1f4d9 100644
> --- a/automation/build/suse/opensuse-tumbleweed.dockerfile
> +++ b/automation/build/suse/opensuse-tumbleweed.dockerfile
> @@ 

[PATCH] CI: Drop glibc-i386 from the build containers

[Andrew Cooper]

Xen 4.14 no longer runs in Gitlab CI.  Drop the dependency to shrink the build
containers a little.

Signed-off-by: Andrew Cooper 
---
CC: Anthony PERARD 
CC: Stefano Stabellini 
CC: Michal Orzel 
CC: Doug Goldstein 
CC: Roger Pau Monné 
---
 automation/build/archlinux/current.dockerfile| 2 --
 automation/build/centos/7.dockerfile | 2 --
 automation/build/debian/bookworm.dockerfile  | 2 --
 automation/build/debian/jessie.dockerfile| 2 --
 automation/build/debian/stretch.dockerfile   | 2 --
 automation/build/fedora/29.dockerfile| 2 --
 automation/build/suse/opensuse-leap.dockerfile   | 2 --
 automation/build/suse/opensuse-tumbleweed.dockerfile | 2 --
 automation/build/ubuntu/bionic.dockerfile| 2 --
 automation/build/ubuntu/focal.dockerfile | 2 --
 automation/build/ubuntu/trusty.dockerfile| 2 --
 automation/build/ubuntu/xenial.dockerfile| 2 --
 12 files changed, 24 deletions(-)

diff --git a/automation/build/archlinux/current.dockerfile 
b/automation/build/archlinux/current.dockerfile
index d974a1434fd5..3e37ab5c40c1 100644
--- a/automation/build/archlinux/current.dockerfile
+++ b/automation/build/archlinux/current.dockerfile
@@ -19,8 +19,6 @@ RUN pacman -S --refresh --sysupgrade --noconfirm 
--noprogressbar --needed \
 iasl \
 inetutils \
 iproute \
-# lib32-glibc for Xen < 4.15
-lib32-glibc \
 libaio \
 libcacard \
 libgl \
diff --git a/automation/build/centos/7.dockerfile 
b/automation/build/centos/7.dockerfile
index ab450f0b3a0e..1cdc16fc05f9 100644
--- a/automation/build/centos/7.dockerfile
+++ b/automation/build/centos/7.dockerfile
@@ -32,8 +32,6 @@ RUN yum -y update \
 yajl-devel \
 pixman-devel \
 glibc-devel \
-# glibc-devel.i686 for Xen < 4.15
-glibc-devel.i686 \
 make \
 binutils \
 git \
diff --git a/automation/build/debian/bookworm.dockerfile 
b/automation/build/debian/bookworm.dockerfile
index 459f8e30bdc6..d893218fc4bd 100644
--- a/automation/build/debian/bookworm.dockerfile
+++ b/automation/build/debian/bookworm.dockerfile
@@ -31,8 +31,6 @@ RUN apt-get update && \
 bin86 \
 bcc \
 liblzma-dev \
-# libc6-dev-i386 for Xen < 4.15
-libc6-dev-i386 \
 libnl-3-dev \
 ocaml-nox \
 libfindlib-ocaml-dev \
diff --git a/automation/build/debian/jessie.dockerfile 
b/automation/build/debian/jessie.dockerfile
index 32fc952fbc2d..308675cac150 100644
--- a/automation/build/debian/jessie.dockerfile
+++ b/automation/build/debian/jessie.dockerfile
@@ -37,8 +37,6 @@ RUN apt-get update && \
 bin86 \
 bcc \
 liblzma-dev \
-# libc6-dev-i386 for Xen < 4.15
-libc6-dev-i386 \
 libnl-3-dev \
 ocaml-nox \
 libfindlib-ocaml-dev \
diff --git a/automation/build/debian/stretch.dockerfile 
b/automation/build/debian/stretch.dockerfile
index e2706a8f3589..59794ed4677b 100644
--- a/automation/build/debian/stretch.dockerfile
+++ b/automation/build/debian/stretch.dockerfile
@@ -38,8 +38,6 @@ RUN apt-get update && \
 bin86 \
 bcc \
 liblzma-dev \
-# libc6-dev-i386 for Xen < 4.15
-libc6-dev-i386 \
 libnl-3-dev \
 ocaml-nox \
 libfindlib-ocaml-dev \
diff --git a/automation/build/fedora/29.dockerfile 
b/automation/build/fedora/29.dockerfile
index 42a87ce6c84b..f473ae13e7c1 100644
--- a/automation/build/fedora/29.dockerfile
+++ b/automation/build/fedora/29.dockerfile
@@ -21,8 +21,6 @@ RUN dnf -y install \
 yajl-devel \
 pixman-devel \
 glibc-devel \
-# glibc-devel.i686 for Xen < 4.15
-glibc-devel.i686 \
 make \
 binutils \
 git \
diff --git a/automation/build/suse/opensuse-leap.dockerfile 
b/automation/build/suse/opensuse-leap.dockerfile
index e1ec38a41445..48d0d50d005d 100644
--- a/automation/build/suse/opensuse-leap.dockerfile
+++ b/automation/build/suse/opensuse-leap.dockerfile
@@ -28,8 +28,6 @@ RUN zypper install -y --no-recommends \
 ghostscript \
 glib2-devel \
 glibc-devel \
-# glibc-devel-32bit for Xen < 4.15
-glibc-devel-32bit \
 gzip \
 hostname \
 libaio-devel \
diff --git a/automation/build/suse/opensuse-tumbleweed.dockerfile 
b/automation/build/suse/opensuse-tumbleweed.dockerfile
index f00e03eda7b1..53542ba1f4d9 100644
--- a/automation/build/suse/opensuse-tumbleweed.dockerfile
+++ b/automation/build/suse/opensuse-tumbleweed.dockerfile
@@ -26,8 +26,6 @@ RUN zypper install -y --no-recommends \
 ghostscript \
 glib2-devel \
 glibc-devel \
-# glibc-devel-32bit for Xen < 4.15
-glibc-devel-32bit \
 gzip \
 hostname \
 libaio-devel \
diff --git a/automation/build/ubuntu/bionic.dockerfile 

[PATCH 2/2] tools: Drop libsystemd as a dependency

[Andrew Cooper]

There are no more users, and we want to disuade people from introducing new
users just for sd_notify() and friends.  Drop the dependency.

We still want the overall --with{,out}-systemd to gate the generation of the
service/unit/mount/etc files.

Rerun autogen.sh, and mark the dependency as removed in the build containers.

Signed-off-by: Andrew Cooper 
---
CC: Anthony PERARD 
CC: Juergen Gross 
CC: Christian Lindig 
CC: Edwin Török 
CC: Stefano Stabellini 
---
 automation/build/archlinux/current.dockerfile |   1 +
 .../build/suse/opensuse-leap.dockerfile   |   1 +
 .../build/suse/opensuse-tumbleweed.dockerfile |   1 +
 automation/build/ubuntu/focal.dockerfile  |   1 +
 config/Tools.mk.in|   2 -
 m4/systemd.m4 |  23 +-
 tools/config.h.in |   3 -
 tools/configure   | 523 +-
 8 files changed, 7 insertions(+), 548 deletions(-)

diff --git a/automation/build/archlinux/current.dockerfile 
b/automation/build/archlinux/current.dockerfile
index d974a1434fd5..a350b53ad8e2 100644
--- a/automation/build/archlinux/current.dockerfile
+++ b/automation/build/archlinux/current.dockerfile
@@ -39,6 +39,7 @@ RUN pacman -S --refresh --sysupgrade --noconfirm 
--noprogressbar --needed \
 sdl2 \
 spice \
 spice-protocol \
+# systemd for Xen < 4.19
 systemd \
 transfig \
 usbredir \
diff --git a/automation/build/suse/opensuse-leap.dockerfile 
b/automation/build/suse/opensuse-leap.dockerfile
index e1ec38a41445..0483d9826d7d 100644
--- a/automation/build/suse/opensuse-leap.dockerfile
+++ b/automation/build/suse/opensuse-leap.dockerfile
@@ -61,6 +61,7 @@ RUN zypper install -y --no-recommends \
 'pkgconfig(sdl2)' \
 python3-devel \
 python3-setuptools \
+# systemd-devel for Xen < 4.19
 systemd-devel \
 tar \
 transfig \
diff --git a/automation/build/suse/opensuse-tumbleweed.dockerfile 
b/automation/build/suse/opensuse-tumbleweed.dockerfile
index f00e03eda7b1..59b168e717b2 100644
--- a/automation/build/suse/opensuse-tumbleweed.dockerfile
+++ b/automation/build/suse/opensuse-tumbleweed.dockerfile
@@ -62,6 +62,7 @@ RUN zypper install -y --no-recommends \
 'pkgconfig(sdl2)' \
 python3-devel \
 python3-setuptools \
+# systemd-devel for Xen < 4.19
 systemd-devel \
 tar \
 transfig \
diff --git a/automation/build/ubuntu/focal.dockerfile 
b/automation/build/ubuntu/focal.dockerfile
index 30a9b8e84ffe..8171347c4656 100644
--- a/automation/build/ubuntu/focal.dockerfile
+++ b/automation/build/ubuntu/focal.dockerfile
@@ -36,6 +36,7 @@ RUN apt-get update && \
 libnl-3-dev \
 ocaml-nox \
 libfindlib-ocaml-dev \
+# libsystemd-dev for Xen < 4.19
 libsystemd-dev \
 transfig \
 pandoc \
diff --git a/config/Tools.mk.in b/config/Tools.mk.in
index b54ab21f966b..50fbef841f3f 100644
--- a/config/Tools.mk.in
+++ b/config/Tools.mk.in
@@ -52,8 +52,6 @@ CONFIG_PYGRUB   := @pygrub@
 CONFIG_LIBFSIMAGE   := @libfsimage@
 
 CONFIG_SYSTEMD  := @systemd@
-SYSTEMD_CFLAGS  := @SYSTEMD_CFLAGS@
-SYSTEMD_LIBS:= @SYSTEMD_LIBS@
 XEN_SYSTEMD_DIR := @SYSTEMD_DIR@
 XEN_SYSTEMD_MODULES_LOAD := @SYSTEMD_MODULES_LOAD@
 CONFIG_9PFS := @ninepfs@
diff --git a/m4/systemd.m4 b/m4/systemd.m4
index 112dc11b5e05..aa1ebe94f56c 100644
--- a/m4/systemd.m4
+++ b/m4/systemd.m4
@@ -41,15 +41,6 @@ AC_DEFUN([AX_ALLOW_SYSTEMD_OPTS], [
 ])
 
 AC_DEFUN([AX_CHECK_SYSTEMD_LIBS], [
-   PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon],,
-   [PKG_CHECK_MODULES([SYSTEMD], [libsystemd >= 209])]
-)
-   dnl pkg-config older than 0.24 does not set these for
-   dnl PKG_CHECK_MODULES() worth also noting is that as of version 208
-   dnl of systemd pkg-config --cflags currently yields no extra flags yet.
-   AC_SUBST([SYSTEMD_CFLAGS])
-   AC_SUBST([SYSTEMD_LIBS])
-
AS_IF([test "x$SYSTEMD_DIR" = x], [
dnl In order to use the line below we need to fix upstream systemd
dnl to properly ${prefix} for child variables in
@@ -83,23 +74,11 @@ AC_DEFUN([AX_CHECK_SYSTEMD_LIBS], [
 AC_DEFUN([AX_CHECK_SYSTEMD], [
dnl Respect user override to disable
AS_IF([test "x$enable_systemd" != "xno"], [
-AS_IF([test "x$systemd" = "xy" ], [
-   AC_DEFINE([HAVE_SYSTEMD], [1], [Systemd available and enabled])
-   systemd=y
-   AX_CHECK_SYSTEMD_LIBS()
-   ],[
-   AS_IF([test "x$enable_systemd" = "xyes"],
-   [AC_MSG_ERROR([Unable to find systemd development 
library])],
-   [systemd=n])
-   ])
+   systemd=y
],[systemd=n])
 ])
 
 AC_DEFUN([AX_CHECK_SYSTEMD_ENABLE_AVAILABLE], [
-   PKG_CHECK_MODULES([SYSTEMD], 

[PATCH 0/2] Drop libsystemd

[Andrew Cooper]

On advise from the systemd leadership.  See patch 1 for details.

Andrew Cooper (2):
  tools/{c,o}xenstored: Don't link against libsystemd
  tools: Drop libsystemd as a dependency

 automation/build/archlinux/current.dockerfile |   1 +
 .../build/suse/opensuse-leap.dockerfile   |   1 +
 .../build/suse/opensuse-tumbleweed.dockerfile |   1 +
 automation/build/ubuntu/focal.dockerfile  |   1 +
 config/Tools.mk.in|   2 -
 m4/systemd.m4 |  23 +-
 tools/config.h.in |   3 -
 tools/configure   | 523 +-
 tools/ocaml/xenstored/Makefile|  12 +-
 tools/ocaml/xenstored/systemd.ml  |  15 -
 tools/ocaml/xenstored/systemd.mli |  16 -
 tools/ocaml/xenstored/systemd_stubs.c |  47 --
 tools/ocaml/xenstored/xenstored.ml|   1 -
 tools/xenstored/Makefile  |   5 -
 tools/xenstored/posix.c   |   9 -
 15 files changed, 8 insertions(+), 652 deletions(-)
 delete mode 100644 tools/ocaml/xenstored/systemd.ml
 delete mode 100644 tools/ocaml/xenstored/systemd.mli
 delete mode 100644 tools/ocaml/xenstored/systemd_stubs.c


base-commit: 23cd1207e7f6ee3e51fb42e11dba8d7cdb28e1e5
-- 
2.30.2

[PATCH 1/2] tools/{c,o}xenstored: Don't link against libsystemd

[Andrew Cooper]

libsystemd is a giant dependency for one single function, but in the wake of
the xz backdoor, it turns out that even systemd leadership recommend against
linking against libsystemd for sd_notify().

Since commit 7b61011e1450 ("tools: make xenstore domain easy configurable") in
Xen 4.8, the launch-xenstore script invokes systemd-notify directly, so its
not even necessary for the xenstored's to call sd_notify() themselves.

Therefore, just drop the calls to sd_notify() and stop linking against
libsystemd.

No functional change.

Signed-off-by: Andrew Cooper 
---
CC: Anthony PERARD 
CC: Juergen Gross 
CC: Christian Lindig 
CC: Edwin Török 
CC: Stefano Stabellini 
---
 tools/ocaml/xenstored/Makefile| 12 +--
 tools/ocaml/xenstored/systemd.ml  | 15 -
 tools/ocaml/xenstored/systemd.mli | 16 -
 tools/ocaml/xenstored/systemd_stubs.c | 47 ---
 tools/ocaml/xenstored/xenstored.ml|  1 -
 tools/xenstored/Makefile  |  5 ---
 tools/xenstored/posix.c   |  9 -
 7 files changed, 1 insertion(+), 104 deletions(-)
 delete mode 100644 tools/ocaml/xenstored/systemd.ml
 delete mode 100644 tools/ocaml/xenstored/systemd.mli
 delete mode 100644 tools/ocaml/xenstored/systemd_stubs.c

diff --git a/tools/ocaml/xenstored/Makefile b/tools/ocaml/xenstored/Makefile
index e8aaecf2e630..1e4b51cc5432 100644
--- a/tools/ocaml/xenstored/Makefile
+++ b/tools/ocaml/xenstored/Makefile
@@ -4,8 +4,6 @@ include $(OCAML_TOPLEVEL)/common.make
 
 # Include configure output (config.h)
 CFLAGS += -include $(XEN_ROOT)/tools/config.h
-CFLAGS-$(CONFIG_SYSTEMD)  += $(SYSTEMD_CFLAGS)
-LDFLAGS-$(CONFIG_SYSTEMD) += $(SYSTEMD_LIBS)
 
 CFLAGS  += $(CFLAGS-y)
 CFLAGS  += $(APPEND_CFLAGS)
@@ -25,13 +23,6 @@ poll_OBJS = poll
 poll_C_OBJS = select_stubs
 OCAML_LIBRARY = syslog poll
 
-LIBS += systemd.cma systemd.cmxa
-systemd_OBJS = systemd
-systemd_C_OBJS = systemd_stubs
-OCAML_LIBRARY += systemd
-
-LIBS_systemd += $(LDFLAGS-y)
-
 OBJS = paths \
define \
stdext \
@@ -56,12 +47,11 @@ OBJS = paths \
process \
xenstored
 
-INTF = symbol.cmi trie.cmi syslog.cmi systemd.cmi poll.cmi
+INTF = symbol.cmi trie.cmi syslog.cmi poll.cmi
 
 XENSTOREDLIBS = \
unix.cmxa \
-ccopt -L -ccopt . syslog.cmxa \
-   -ccopt -L -ccopt . systemd.cmxa \
-ccopt -L -ccopt . poll.cmxa \
-ccopt -L -ccopt $(OCAML_TOPLEVEL)/libs/mmap 
$(OCAML_TOPLEVEL)/libs/mmap/xenmmap.cmxa \
-ccopt -L -ccopt $(OCAML_TOPLEVEL)/libs/eventchn 
$(OCAML_TOPLEVEL)/libs/eventchn/xeneventchn.cmxa \
diff --git a/tools/ocaml/xenstored/systemd.ml b/tools/ocaml/xenstored/systemd.ml
deleted file mode 100644
index 39127f712d72..
--- a/tools/ocaml/xenstored/systemd.ml
+++ /dev/null
@@ -1,15 +0,0 @@
-(*
- * Copyright (C) 2014 Luis R. Rodriguez 
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU Lesser General Public License as published
- * by the Free Software Foundation; version 2.1 only. with the special
- * exception on linking described in file LICENSE.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU Lesser General Public License for more details.
- *)
-
-external sd_notify_ready: unit -> unit = "ocaml_sd_notify_ready"
diff --git a/tools/ocaml/xenstored/systemd.mli 
b/tools/ocaml/xenstored/systemd.mli
deleted file mode 100644
index 18b9331031f9..
--- a/tools/ocaml/xenstored/systemd.mli
+++ /dev/null
@@ -1,16 +0,0 @@
-(*
- * Copyright (C) 2014 Luis R. Rodriguez 
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU Lesser General Public License as published
- * by the Free Software Foundation; version 2.1 only. with the special
- * exception on linking described in file LICENSE.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU Lesser General Public License for more details.
- *)
-
-(** Tells systemd we're ready *)
-external sd_notify_ready: unit -> unit = "ocaml_sd_notify_ready"
diff --git a/tools/ocaml/xenstored/systemd_stubs.c 
b/tools/ocaml/xenstored/systemd_stubs.c
deleted file mode 100644
index f4c875075abe..
--- a/tools/ocaml/xenstored/systemd_stubs.c
+++ /dev/null
@@ -1,47 +0,0 @@
-/*
- * Copyright (C) 2014 Luis R. Rodriguez 
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU Lesser General Public License as published
- * by the Free Software Foundation; version 2.1 only. with the special
- * exception on linking described in file LICENSE.
- *
- * This program is distributed in the hope that it will be useful,
- * but 

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 xen  6d5111b10e084d841284a56e962c61ad274f589e
baseline version:
 xen  524eee7ce019da69df356a7bdd2174b94a6787b1

Last test of basis   185795  2024-04-25 08:00:24 Z0 days
Testing same since   185797  2024-04-25 13:08:37 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Christian Lindig 
  Edwin Török 

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
   524eee7ce0..6d5111b10e  6d5111b10e084d841284a56e962c61ad274f589e -> smoke

Re: [PATCH] svm: Fix MISRA 8.2 violation

[Andrew Cooper]

On 25/04/2024 10:12 am, George Dunlap wrote:
> Misra 8.2 requires named parameters in prototypes.  Use the name from
> the implementaiton.
>
> Fixes 0d19d3aab0 ("svm/nestedsvm: Introduce nested capabilities bit").

Nit.  We treat Fixes: as a tag.

Also you might want to include the Reported-by's.

Otherwise, Acked-by: Andrew Cooper  although
I have a strong wish to shorten the parameter name.  That can be done at
a later point.

~Andrew

Re: [PATCH v8 05/17] xen/riscv: introduce bitops.h

[Jan Beulich]

On 17.04.2024 12:04, Oleksii Kurochko wrote:
> Taken from Linux-6.4.0-rc1
> 
> Xen's bitops.h consists of several Linux's headers:
> * linux/arch/include/asm/bitops.h:
>   * The following function were removed as they aren't used in Xen:
> * test_and_set_bit_lock
> * clear_bit_unlock
> * __clear_bit_unlock
>   * The following functions were renamed in the way how they are
> used by common code:
> * __test_and_set_bit
> * __test_and_clear_bit
>   * The declaration and implementation of the following functios
> were updated to make Xen build happy:
> * clear_bit
> * set_bit
> * __test_and_clear_bit
> * __test_and_set_bit
> 
> Signed-off-by: Oleksii Kurochko 

Acked-by: Jan Beulich 

Re: [PATCH v8 04/17] xen/bitops: put __ffs() into linux compatible header

[Jan Beulich]

On 17.04.2024 12:04, Oleksii Kurochko wrote:
> --- a/xen/include/xen/linux-compat.h
> +++ b/xen/include/xen/linux-compat.h
> @@ -19,4 +19,6 @@ typedef int64_t __s64;
>  
>  typedef paddr_t phys_addr_t;
>  
> +#define __ffs(x) (ffsl(x) - 1)

See my v7 reply.

Jan

Re: [PATCH v8 03/17] xen/bitops: implement fls{l}() in common logic

[Jan Beulich]

On 17.04.2024 12:04, Oleksii Kurochko wrote:
> Return type was left 'int' because of the following compilation error:
> 
> ./include/xen/kernel.h:18:21: error: comparison of distinct pointer types 
> lacks a cast [-Werror]
>18 | (void) (&_x == &_y);\
>   | ^~
> common/page_alloc.c:1843:34: note: in expansion of macro 'min'
>  1843 | unsigned int inc_order = min(MAX_ORDER, flsl(e - s) - 1);

Apart from this I'm okay with this patch, assuming Andrew's won't change in
any conflicting way. As to the above - no, I don't see us having ffs() / ffsl()
returning unsigned int, fls() / flsl() returning plain int. Even more so that,
given the LHS variable's type, an unsigned quantity is really meant in the
quoted code.

Jan

Re: [PATCH v3] x86/entry: shrink insn size for some of our EFLAGS manipulation

[Andrew Cooper]

On 25/04/2024 3:26 pm, Jan Beulich wrote:
> Much like was recently done for setting entry vector, and along the
> lines of what we already had in handle_exception_saved, avoid 32-bit
> immediates where 8-bit ones do. Reduces .text.entry size by 16 bytes in
> my non-CET reference build, while in my CET reference build section size
> doesn't change (there and in .text only padding space increases).
>
> Inspired by other long->byte conversion work.
>
> Signed-off-by: Jan Beulich 

Acked-by: Andrew Cooper 

Re: [PATCH v8 02/17] xen: introduce generic non-atomic test_*bit()

[Jan Beulich]

On 17.04.2024 12:04, Oleksii Kurochko wrote:
> --- a/xen/arch/ppc/include/asm/page.h
> +++ b/xen/arch/ppc/include/asm/page.h
> @@ -4,7 +4,7 @@
>  
>  #include 
>  
> -#include 
> +#include 
>  #include 

This wants to move up into the xen/*.h group then, like you have done ...

> --- a/xen/arch/ppc/mm-radix.c
> +++ b/xen/arch/ppc/mm-radix.c
> @@ -1,11 +1,11 @@
>  /* SPDX-License-Identifier: GPL-2.0-or-later */
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
>  #include 
>  
> -#include 
>  #include 
>  #include 
>  #include 

.. e.g. here.

> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -65,10 +65,137 @@ static inline int generic_flsl(unsigned long x)
>   * scope
>   */
>  
> +#define BITOP_MASK(nr)  ((bitop_uint_t)1 << ((nr) % BITOP_BITS_PER_WORD))
> +
> +#define BITOP_WORD(nr)  ((nr) / BITOP_BITS_PER_WORD)
> +
>  /* - Please tidy above here - */
>  
>  #include 
>  
> +#ifndef arch_check_bitop_size
> +#define arch_check_bitop_size(addr)

Can this really do nothing? Passing the address of an object smaller than
bitop_uint_t will read past the object in the generic__*_bit() functions.

> +#endif
> +
> +/**
> + * generic__test_and_set_bit - Set a bit and return its old value
> + * @nr: Bit to set
> + * @addr: Address to count from
> + *
> + * This operation is non-atomic and can be reordered.
> + * If two examples of this operation race, one can appear to succeed
> + * but actually fail.  You must protect multiple accesses with a lock.
> + */
> +static always_inline bool
> +generic__test_and_set_bit(unsigned long nr, volatile void *addr)
> +{
> +bitop_uint_t mask = BITOP_MASK(nr);
> +volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) + 
> BITOP_WORD(nr);

The revision log suggests excess parentheses were dropped from such cast
expressions.

> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -64,6 +64,11 @@ typedef __u64 __be64;
>  
>  typedef unsigned int __attribute__((__mode__(__pointer__))) uintptr_t;
>  
> +#ifndef BITOP_TYPE
> +#define BITOP_BITS_PER_WORD 32
> +typedef uint32_t bitop_uint_t;

Personally I find this indentation odd / misleading. For pre-processor
directives the # preferrably remains first on a line (as was iirc
demanded by earlier C standards), followed by one or more blanks if so
desired. File-scope declarations imo should never be indented.

Jan

Re: [PATCH net] xen-netfront: Add missing skb_mark_for_recycle

[Greg KH]

On Thu, Apr 25, 2024 at 02:39:38PM +0100, George Dunlap wrote:
> Greg,
> 
> We're issuing an XSA for this; can you issue a CVE?

To ask for a cve, please contact c...@kernel.org as per our
documentation.  Please provide the git id of the commit you wish to have
the cve assigned to.

thanks,

greg k-h

[libvirt test] 185793: tolerable all pass - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 libvirt  948d496d2546807e8c3af634fa982615b2a3153b
baseline version:
 libvirt  595b95cdde39a78528e584fdfd71a1d562ba9e45

Last test of basis   185783  2024-04-24 04:20:30 Z1 days
Testing same since   185793  2024-04-25 04:22:04 Z0 days1 attempts


People who touched revisions under test:
  Cole Robinson 
  Göran Uddeborg 
  Michal Privoznik 
  Peter Krempa 

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



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

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

Explanation of these reports, and of osstest in general, is at
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/libvirt.git
   595b95cdde..948d496d25  948d496d2546807e8c3af634fa982615b2a3153b -> 
xen-tested-master

Re: [PATCH v4 1/4] xen-livepatch: fix parameter name parsing

[Ross Lagerwall]

On Wed, Apr 24, 2024 at 9:20 AM Roger Pau Monne  wrote:
>
> It's incorrect to restrict strncmp to the length of the command line input
> parameter, as then a user passing a rune like:
>
> % xen-livepatch up foo.livepatch
>
> Would match against the "upload" command, because the string comparison has
> been truncated to the length of the input argument.  Use strcmp instead which
> doesn't truncate.  Otherwise in order to keep using strncmp we would need to
> also check strings are of the same length before doing the comparison.
>

I had previously assumed that this was intentional as a way of allowing
abbreviated syntax. Regardless of the original intent, I'm OK with this
change.

Reviewed-by: Ross Lagerwall 

Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Julien Grall]

Hi,

On 25/04/2024 08:06, Henry Wang wrote:

Hi Julien,

On 4/24/2024 8:58 PM, Julien Grall wrote:

Hi Henry,

On 24/04/2024 04:34, Henry Wang wrote:

From: Vikram Garhwal 

Enable interrupt assign/remove for running VMs in CONFIG_OVERLAY_DTB.

Currently, irq_route and mapping is only allowed at the domain 
creation. Adding

exception for CONFIG_OVERLAY_DTB.


AFAICT, this is mostly reverting b8577547236f ("xen/arm: Restrict when 
a physical IRQ can be routed/removed from/to a domain").




Signed-off-by: Vikram Garhwal 
Signed-off-by: Stefano Stabellini 
Signed-off-by: Henry Wang 
---
  xen/arch/arm/gic.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 44c40e86de..a775f886ed 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -140,8 +140,10 @@ int gic_route_irq_to_guest(struct domain *d, 
unsigned int virq,

   * back to the physical IRQ. To prevent get unsync, restrict the
   * routing to when the Domain is been created.
   */


The above comment explains why the check was added. But the commit 
message doesn't explain why this can be disregarded for your use-case.


Looking at the history, I don't think you can simply remove the checks.

Regardless that...


+#ifndef CONFIG_OVERLAY_DTB


... I am against such #ifdef. A distros may want to have OVERLAY_DTB 
enabled, yet the user will not use it.


Instead, you want to remove the check once the code can properly 
handle routing an IRQ the domain is created or ...



  if ( d->creation_finished )
  return -EBUSY;
+#endif
    ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
  if ( ret )
@@ -171,8 +173,10 @@ int gic_remove_irq_from_guest(struct domain *d, 
unsigned int virq,

   * Removing an interrupt while the domain is running may have
   * undesirable effect on the vGIC emulation.
   */
+#ifndef CONFIG_OVERLAY_DTB
  if ( !d->is_dying )
  return -EBUSY;
+#endif


... removed before they domain is destroyed.


Thanks for your feeedback. After checking the b8577547236f commit 
message I think I now understand your point. Do you have any suggestion 
about how can I properly add the support to route/remove the IRQ to 
running domains? Thanks.


I haven't really look at that code in quite a while. I think we need to 
make sure that the virtual and physical IRQ state matches at the time we 
do the routing.


I am undecided on whether we want to simply prevent the action to happen 
or try to reset the state.


There is also the question of what to do if the guest is enabling the 
vIRQ before it is routed.


Overall, someone needs to spend some time reading the code and then make 
a proposal (this could be just documentation if we believe it is safe to 
do). Both the current vGIC and the new one may need an update.


Cheers,

--
Julien Grall

[PATCH v3] x86/entry: shrink insn size for some of our EFLAGS manipulation

[Jan Beulich]

Much like was recently done for setting entry vector, and along the
lines of what we already had in handle_exception_saved, avoid 32-bit
immediates where 8-bit ones do. Reduces .text.entry size by 16 bytes in
my non-CET reference build, while in my CET reference build section size
doesn't change (there and in .text only padding space increases).

Inspired by other long->byte conversion work.

Signed-off-by: Jan Beulich 
---
Numbers above are biased by me also having the straight-line-speculation
change in the tree, thus every JMP is followed by an INT3. Without that,
.text.entry size would also shrink by 16 bytes in the CET build.
---
v3: Re-base.
v2: Drop switch_to_kernel change.

--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -226,7 +226,7 @@ LABEL_LOCAL(.Lrestore_rcx_iret_exit_to_g
 /* No special register assumptions. */
 iret_exit_to_guest:
 andl  $~(X86_EFLAGS_IOPL | X86_EFLAGS_VM), EFRAME_eflags(%rsp)
-orl   $X86_EFLAGS_IF, EFRAME_eflags(%rsp)
+orb   $X86_EFLAGS_IF >> 8, EFRAME_eflags + 1(%rsp)
 
 SPEC_CTRL_COND_VERW /* Req: %rsp=eframeClob: 
efl */
 
@@ -355,7 +355,7 @@ LABEL(sysenter_eflags_saved, 0)
 /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
 
 /* PUSHF above has saved EFLAGS.IF clear (the caller had it set). */
-orl   $X86_EFLAGS_IF, UREGS_eflags(%rsp)
+orb   $X86_EFLAGS_IF >> 8, UREGS_eflags + 1(%rsp)
 mov   STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx
 test  %rcx, %rcx
 jz.Lsyse_cr3_okay
@@ -370,11 +370,11 @@ LABEL(sysenter_eflags_saved, 0)
 cmpb  $0,VCPU_sysenter_disables_events(%rbx)
 movq  VCPU_sysenter_addr(%rbx),%rax
 setne %cl
-testl $X86_EFLAGS_NT,UREGS_eflags(%rsp)
+testb $X86_EFLAGS_NT >> 8, UREGS_eflags + 1(%rsp)
 leaq  VCPU_trap_bounce(%rbx),%rdx
 UNLIKELY_START(nz, sysenter_nt_set)
 pushfq
-andl  $~X86_EFLAGS_NT,(%rsp)
+andb  $~(X86_EFLAGS_NT >> 8), 1(%rsp)
 popfq
 UNLIKELY_END(sysenter_nt_set)
 testq %rax,%rax

Re: [PATCH net] xen-netfront: Add missing skb_mark_for_recycle

[George Dunlap]

Greg,

We're issuing an XSA for this; can you issue a CVE?

Thanks,
 -George Dunlap

On Tue, Apr 2, 2024 at 9:25 PM Arthur Borsboom 
wrote:

> After having a better look, I have found the patch in linux-next
>
>
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=0cd74ffcf4fb0536718241d59d2c124578624d83
>
> On Tue, 2 Apr 2024 at 10:20, Arthur Borsboom 
> wrote:
> >
> > On Fri, 29 Mar 2024 at 10:47, Arthur Borsboom 
> wrote:
> > >
> > > On Wed, 27 Mar 2024 at 13:15, Jesper Dangaard Brouer 
> wrote:
> > > >
> > > > Notice that skb_mark_for_recycle() is introduced later than fixes
> tag in
> > > > 6a5bcd84e886 ("page_pool: Allow drivers to hint on SKB recycling").
> > > >
> > > > It is believed that fixes tag were missing a call to
> page_pool_release_page()
> > > > between v5.9 to v5.14, after which is should have used
> skb_mark_for_recycle().
> > > > Since v6.6 the call page_pool_release_page() were removed (in
> 535b9c61bdef
> > > > ("net: page_pool: hide page_pool_release_page()") and remaining
> callers
> > > > converted (in commit 6bfef2ec0172 ("Merge branch
> > > > 'net-page_pool-remove-page_pool_release_page'")).
> > > >
> > > > This leak became visible in v6.8 via commit dba1b8a7ab68
> ("mm/page_pool: catch
> > > > page_pool memory leaks").
> > > >
> > > > Fixes: 6c5aa6fc4def ("xen networking: add basic XDP support for
> xen-netfront")
> > > > Reported-by: Arthur Borsboom 
> > > > Signed-off-by: Jesper Dangaard Brouer 
> > > > ---
> > > > Compile tested only, can someone please test this
> > >
> > > I have tested this patch on Xen 4.18.1 with VM (Arch Linux) kernel
> 6.9.0-rc1.
> > >
> > > Without the patch there are many trace traces and cloning the Linux
> > > mainline git repository resulted in failures (same with kernel 6.8.1).
> > > The patched kernel 6.9.0-rc1 performs as expected; cloning the git
> > > repository was successful and no kernel traces observed.
> > > Hereby my tested by:
> > >
> > > Tested-by: Arthur Borsboom 
> > >
> > >
> > >
> > > >  drivers/net/xen-netfront.c |1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> > > > index ad29f370034e..8d2aee88526c 100644
> > > > --- a/drivers/net/xen-netfront.c
> > > > +++ b/drivers/net/xen-netfront.c
> > > > @@ -285,6 +285,7 @@ static struct sk_buff
> *xennet_alloc_one_rx_buffer(struct netfront_queue *queue)
> > > > return NULL;
> > > > }
> > > > skb_add_rx_frag(skb, 0, page, 0, 0, PAGE_SIZE);
> > > > +   skb_mark_for_recycle(skb);
> > > >
> > > > /* Align ip header to a 16 bytes boundary */
> > > > skb_reserve(skb, NET_IP_ALIGN);
> > > >
> > > >
> >
> > I don't see this patch yet in linux-next.
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log
> >
> > Any idea in which kernel release this patch will be included?
>

Re: [XEN PATCH] automation/eclair: reorganize pipelines

[Nicola Vetrini]

On 2024-04-25 11:40, Federico Serafini wrote:

On 25/04/24 02:06, Stefano Stabellini wrote:

On Tue, 23 Apr 2024, Federico Serafini wrote:

From: Simone Ballarin 

Introduce accepted_guidelines.sh: a script to autogenerate the
configuration file accepted.ecl from docs/misra/rules.rst which 
enables

all accepted guidelines.

Introduce monitored.ecl: a manual selection of accepted guidelines
which are clean or almost clean, it is intended to be used for the
analyses triggered by commits.

Reorganize tagging.ecl:
   -Remove "accepted" tags: keeping track of accepted guidelines 
tagging

them as "accepted" in the configuration file tagging.ecl is no
longer needed since docs/rules.rst is keeping track of them.
   -Tag more guidelines as clean.

Reorganize eclair pipelines:
   - Set1, Set2, Set3 are now obsolete: remove the corresponding
 pipelines and ecl files.
   - Amend scheduled eclair pipeline to use accepted.ecl.
   - Amend triggered eclair pipeline to use monitored.ecl.

Rename and improve action_check_clean_regressions.sh to print a
diagnostic in case a commit introduces a violation of a clean 
guideline.


An example of diagnostic is the following:

Failure: 13 regressions found for clean guidelines
   service MC3R1.R8.2: (required) Function types shall be in 
prototype form with named parameters:

violation: 13

Signed-off-by: Simone Ballarin 
Signed-off-by: Federico Serafini 
Signed-off-by: Alessandro Zucchelli 



Fantastic work, thank you!

Reviewed-by: Stefano Stabellini 

Is this patch safe to commit now? Or would it cause gitlab-ci 
breakage?


Yes, it is safe because the ECLAIR analysis is still allowed to fail.
Committing this patch wouldn't break the CI but it will highlight some 
regressions with the orange badge and the following messages:


arm:

Failure: 5 regressions found for clean guidelines
  service MC3R1.R1.1: (required) The program shall contain no 
violations of the standard C syntax and constraints, and shall not 
exceed the implementation's translation limits:

   violation: 5



+Cc ARM maintainers, Luca Fancellu

after some investigation on these regressions on R1.1, there are some 
concerns with the current code:

2209c1e35b479dff8ed3d3161001ffdefa0a704e introduced the struct

struct membanks {
unsigned int nr_banks;
unsigned int max_banks;
struct membank bank[];
};

with a flexible array member at the end; this is well-defined in C99, 
but what is non-standard (a GNU extension) is having this struct as a 
member to another struct/union in a position other than the last.


This is still supported by GNU (albeit reliance on this is undocumented 
for Xen), but what is concerning here is the following (taken from [1]):


"A structure containing a C99 flexible array member, or a union 
containing such a structure, is not the last field of another structure, 
for example:


struct flex  { int length; char data[]; };

struct mid_flex { int m; struct flex flex_data; int n; };

In the above, accessing a member of the array mid_flex.flex_data.data[] 
might have undefined behavior. Compilers do not handle such a case 
consistently. Any code relying on this case should be modified to ensure 
that flexible array members only end up at the ends of structures.
Please use the warning option -Wflex-array-member-not-at-end to identify 
all such cases in the source code and modify them. This extension is now 
deprecated."


It looks like this option, and the corresponding deprecation of the 
extension, will be available starting from GCC 14, so this might impact 
future compiler updates for Xen builds.


Linux is also concerned about this (see [2]), so I think the changes in 
struct layout introduced by the series [3] should be reviewed to 
determine whether this change was deliberate or happened as a byproduct. 
If this is determined not to be a legitimate concern, then this can be 
documented as an use of the GNU extension.


See [4] for the findings.

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://lkml.org/lkml/2024/4/11/1293
[3] 
https://lore.kernel.org/xen-devel/20240418073652.3622828-1-luca.fance...@arm.com/
[4] 
https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/xen/ECLAIR_normal/staging/ARM64/6713015426/PROJECT.ecd;/by_service/MC3R1.R1.1.html


Thanks,
  Nicola


x86:

Failure: 2 regressions found for clean guidelines
  service MC3R1.R8.2: (required) Function types shall be in prototype 
form with named parameters:

   violation: 2

(George just sent a patch to address the regressions of Rule 8.2.)



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

Re: [PATCH] VMX: no open-coding in vmx_get_cpl()

[Andrew Cooper]

On 25/04/2024 2:27 pm, Jan Beulich wrote:
> Neither X86_SEG_AR_DPL nor MASK_EXTR() should really be avoided here,
> using literal number instead.
>
> No difference in generated code (with gcc13 at least).
>
> Signed-off-by: Jan Beulich 

Acked-by: Andrew Cooepr 

Re: [PATCH v2 2/2] x86/spec: adjust logic to logic that elides lfence

[Andrew Cooper]

On 18/04/2024 4:52 pm, Roger Pau Monne wrote:
> It's currently too restrictive by just checking whether there's a BHB clearing
> sequence selected.  It should instead check whether BHB clearing is used on
> entry from PV or HVM specifically.
>
> Switch to use opt_bhb_entry_{pv,hvm} instead, and then remove cpu_has_bhb_seq
> since it no longer has any users.
>
> Reported-by: Jan Beulich 
> Fixes: 954c983abcee ('x86/spec-ctrl: Software BHB-clearing sequences')
> Signed-off-by: Roger Pau Monné 
> ---
> Changes since v1:
>  - New in this version.
>
> There (possibly) still a bit of overhead for dom0 if BHB clearing is not used
> for dom0, as Xen would still add the lfence if domUs require it.

This is the note about dom0 that I made on the previous patch.

"protect dom0" only has any effect if the appropriate foo_pv or foo_hvm
is also selected.  It's not possible to express "protect dom0 but not
domU of $TYPE".

This early on boot we have no idea whether dom0 is going to be PV or
HVM.  We could in principle figure it out by peeking at dom0's ELF
notes, but that needs a lot of rearranging of __start_xen() to do safely.

Reviewed-by: Andrew Cooper 

[PATCH] VMX: no open-coding in vmx_get_cpl()

[Jan Beulich]

Neither X86_SEG_AR_DPL nor MASK_EXTR() should really be avoided here,
using literal number instead.

No difference in generated code (with gcc13 at least).

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1192,7 +1192,7 @@ unsigned int vmx_get_cpl(void)
 
 __vmread(GUEST_SS_AR_BYTES, );
 
-return (attr >> 5) & 3;
+return MASK_EXTR(attr, X86_SEG_AR_DPL);
 }
 
 static unsigned int cf_check _vmx_get_cpl(struct vcpu *v)

[linux-linus test] 185791: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-armhf-armhf-xl-raw   8 xen-boot fail REGR. vs. 185768

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

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

version targeted for testing:
 linuxe88c4cfcb7b888ac374916806f86c17d8ecaeb67
baseline version:
 linux71b1543c83d65af8215d7558d70fc2ecbee77dcf

Last test of basis   185768  2024-04-23 07:46:10 Z2 days
Failing since185779  2024-04-24 00:13:50 Z1 days3 attempts
Testing same since   185791  2024-04-25 01:43:09 Z0 days1 attempts


People who touched revisions under test:
  David Howells 
  David Sterba 
  Johannes Thumshirn 
  Linus Torvalds 
  Naohiro Aota 
  Neal Gompa 
  Paulo Alcantara (Red Hat) 
  Paulo Alcantara 
  Qu Wenruo 
  Ronnie Sahlberg 
  Steve French 
  Sweet Tea Dorminy 
  Takayuki Nagata 
  Tom 

[RFC PATCH 2/2] xen/arm: Rework dt_unreserved_regions to avoid recursion

[Luca Fancellu]

The function dt_unreserved_regions is currently using recursion
to compute the non overlapping ranges of a passed region against
the reserved memory banks, in the spirit of removing the recursion
to improve safety and also improve the scalability of the function,
rework its code to use an iterative algorithm with the same result.

The function was taking an additional parameter 'first', but given
the rework and given that the function was always initially called
with this parameter as zero, remove the parameter and update the
codebase to reflect the change.

Signed-off-by: Luca Fancellu 
---
 xen/arch/arm/include/asm/setup.h|   8 +-
 xen/arch/arm/include/asm/static-shmem.h |   7 +-
 xen/arch/arm/kernel.c   |   2 +-
 xen/arch/arm/setup.c| 133 
 4 files changed, 96 insertions(+), 54 deletions(-)

diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index fc6967f9a435..24519b9ed969 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -9,7 +9,12 @@
 #define MAX_FDT_SIZE SZ_2M
 
 #define NR_MEM_BANKS 256
+
+#ifdef CONFIG_STATIC_SHM
 #define NR_SHMEM_BANKS 32
+#else
+#define NR_SHMEM_BANKS 0
+#endif
 
 #define MAX_MODULES 32 /* Current maximum useful modules */
 
@@ -215,8 +220,7 @@ void create_dom0(void);
 
 void discard_initial_modules(void);
 void fw_unreserved_regions(paddr_t s, paddr_t e,
-   void (*cb)(paddr_t ps, paddr_t pe),
-   unsigned int first);
+   void (*cb)(paddr_t ps, paddr_t pe));
 
 size_t boot_fdt_info(const void *fdt, paddr_t paddr);
 const char *boot_fdt_cmdline(const void *fdt);
diff --git a/xen/arch/arm/include/asm/static-shmem.h 
b/xen/arch/arm/include/asm/static-shmem.h
index 3b6569e5703f..1b7c7ea0e17d 100644
--- a/xen/arch/arm/include/asm/static-shmem.h
+++ b/xen/arch/arm/include/asm/static-shmem.h
@@ -7,11 +7,11 @@
 #include 
 #include 
 
-#ifdef CONFIG_STATIC_SHM
-
 /* Worst case /memory node reg element: (addrcells + sizecells) */
 #define DT_MEM_NODE_REG_RANGE_SIZE ((NR_MEM_BANKS + NR_SHMEM_BANKS) * 4)
 
+#ifdef CONFIG_STATIC_SHM
+
 int make_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
   int sizecells);
 
@@ -47,9 +47,6 @@ void shm_mem_node_fill_reg_range(const struct kernel_info 
*kinfo, __be32 *reg,
 
 #else /* !CONFIG_STATIC_SHM */
 
-/* Worst case /memory node reg element: (addrcells + sizecells) */
-#define DT_MEM_NODE_REG_RANGE_SIZE (NR_MEM_BANKS * 4)
-
 static inline int make_resv_memory_node(const struct kernel_info *kinfo,
 int addrcells, int sizecells)
 {
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 674388fa11a2..ecbeec518754 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -247,7 +247,7 @@ static __init int kernel_decompress(struct bootmodule *mod, 
uint32_t offset)
  * Free the original kernel, update the pointers to the
  * decompressed kernel
  */
-fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
+fw_unreserved_regions(addr, addr + size, init_domheap_pages);
 
 return 0;
 }
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index c4e5c19b11d6..d737fe56e539 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -204,55 +204,97 @@ static void __init processor_id(void)
 }
 
 static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
- void (*cb)(paddr_t ps, paddr_t pe),
- unsigned int first)
+ void (*cb)(paddr_t ps, paddr_t pe))
 {
-const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
-#ifdef CONFIG_STATIC_SHM
-const struct membanks *shmem = bootinfo_get_shmem();
-unsigned int offset;
-#endif
-unsigned int i;
-
 /*
- * i is the current bootmodule we are evaluating across all possible
- * kinds.
+ * The worst case scenario is to have N reserved region ovelapping the
+ * passed one, so having N+1 regions in the stack
  */
-for ( i = first; i < reserved_mem->nr_banks; i++ )
-{
-paddr_t r_s = reserved_mem->bank[i].start;
-paddr_t r_e = r_s + reserved_mem->bank[i].size;
-
-if ( s < r_e && r_s < e )
-{
-dt_unreserved_regions(r_e, e, cb, i + 1);
-dt_unreserved_regions(s, r_s, cb, i + 1);
-return;
-}
-}
-
+struct {
+paddr_t s;
+paddr_t e;
+unsigned int dict;
+unsigned int bank;
+} stack[NR_MEM_BANKS + NR_SHMEM_BANKS + 1];
+const struct membanks *mem_banks[] = {
+bootinfo_get_reserved_mem(),
 #ifdef CONFIG_STATIC_SHM
-/*
- * When retrieving the corresponding shared memory addresses
- * below, we need to index the shmem->bank starting from 0, hence
- * we need to use i - 

[RFC PATCH 0/2] xen/arm: Remove recursion from dt_unreserved_regions

[Luca Fancellu]

Hi, this is an RFC that is exploiting the new 'struct membank' interface
introduced here 2209c1e35b479dff8ed3d3161001ffdefa0a704e ("xen/arm: Introduce a
generic way to access memory bank structures") to start removing recursion
from some function, in this serie the dt_unreserved_regions is reworked for
that reason.

This is an RFC to understand if the proposed approach can be accepted.

Another function can benefit from this approach, consider_modules in
arm32/mmu/mm.c, but it might require to rework also the 'struct bootmodules'
to adhere to the new interface and have just a loop that can go through all the
different structures.

Comments are welcome.

Luca Fancellu (2):
  xen/arm: Add DT reserve map regions to bootinfo.reserved_mem
  xen/arm: Rework dt_unreserved_regions to avoid recursion

 xen/arch/arm/arm32/mmu/mm.c |  29 +
 xen/arch/arm/bootfdt.c  |  51 +---
 xen/arch/arm/domain_build.c |   3 +-
 xen/arch/arm/include/asm/setup.h|  13 +-
 xen/arch/arm/include/asm/static-shmem.h |   7 +-
 xen/arch/arm/kernel.c   |   2 +-
 xen/arch/arm/setup.c| 158 +---
 7 files changed, 135 insertions(+), 128 deletions(-)

-- 
2.34.1

[RFC PATCH 1/2] xen/arm: Add DT reserve map regions to bootinfo.reserved_mem

[Luca Fancellu]

Currently the code is listing device tree reserve map regions
as reserved memory for Xen, but they are not added into
bootinfo.reserved_mem and they are fetched in multiple places
using the same code sequence, causing duplication. Fix this
by adding them to the bootinfo.reserved_mem at early stage.

Signed-off-by: Luca Fancellu 
---
 xen/arch/arm/arm32/mmu/mm.c  | 29 +
 xen/arch/arm/bootfdt.c   | 51 ++
 xen/arch/arm/domain_build.c  |  3 +-
 xen/arch/arm/include/asm/setup.h |  5 +++
 xen/arch/arm/setup.c | 53 +---
 5 files changed, 53 insertions(+), 88 deletions(-)

diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c
index 23150122f7c4..be480c31ea05 100644
--- a/xen/arch/arm/arm32/mmu/mm.c
+++ b/xen/arch/arm/arm32/mmu/mm.c
@@ -73,33 +73,6 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
 }
 }
 
-/* Now check any fdt reserved areas. */
-
-nr = fdt_num_mem_rsv(device_tree_flattened);
-
-for ( ; i < mi->nr_mods + nr; i++ )
-{
-paddr_t mod_s, mod_e;
-
-if ( fdt_get_mem_rsv_paddr(device_tree_flattened,
-   i - mi->nr_mods,
-   _s, _e ) < 0 )
-/* If we can't read it, pretend it doesn't exist... */
-continue;
-
-/* fdt_get_mem_rsv_paddr returns length */
-mod_e += mod_s;
-
-if ( s < mod_e && mod_s < e )
-{
-mod_e = consider_modules(mod_e, e, size, align, i+1);
-if ( mod_e )
-return mod_e;
-
-return consider_modules(s, mod_s, size, align, i+1);
-}
-}
-
 /*
  * i is the current bootmodule we are evaluating, across all
  * possible kinds of bootmodules.
@@ -108,7 +81,7 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
  * need to index the reserved_mem bank starting from 0, and only counting
  * the reserved-memory modules. Hence, we need to use i - nr.
  */
-nr += mi->nr_mods;
+nr = mi->nr_mods;
 for ( ; i - nr < reserved_mem->nr_banks; i++ )
 {
 paddr_t r_s = reserved_mem->bank[i - nr].start;
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 4d708442a19e..6e060111d95b 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -475,8 +475,7 @@ static void __init early_print_info(void)
 const struct membanks *mem_resv = bootinfo_get_reserved_mem();
 struct bootmodules *mods = 
 struct bootcmdlines *cmds = 
-unsigned int i, j;
-int nr_rsvd;
+unsigned int i;
 
 for ( i = 0; i < mi->nr_banks; i++ )
 printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
@@ -490,26 +489,11 @@ static void __init early_print_info(void)
 mods->module[i].start + mods->module[i].size,
 boot_module_kind_as_string(mods->module[i].kind));
 
-nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
-if ( nr_rsvd < 0 )
-panic("Parsing FDT memory reserve map failed (%d)\n", nr_rsvd);
-
-for ( i = 0; i < nr_rsvd; i++ )
-{
-paddr_t s, e;
-
-if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, , ) < 0 )
-continue;
-
-/* fdt_get_mem_rsv_paddr returns length */
-e += s;
-printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i, s, e);
-}
-for ( j = 0; j < mem_resv->nr_banks; j++, i++ )
+for ( i = 0; i < mem_resv->nr_banks; i++ )
 {
 printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i,
-   mem_resv->bank[j].start,
-   mem_resv->bank[j].start + mem_resv->bank[j].size - 1);
+   mem_resv->bank[i].start,
+   mem_resv->bank[i].start + mem_resv->bank[i].size - 1);
 }
 early_print_info_shmem();
 printk("\n");
@@ -550,7 +534,10 @@ static void __init swap_memory_node(void *_a, void *_b, 
size_t size)
  */
 size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
 {
+struct membanks *reserved_mem = bootinfo_get_reserved_mem();
 struct membanks *mem = bootinfo_get_mem();
+unsigned int i;
+int nr_rsvd;
 int ret;
 
 ret = fdt_check_header(fdt);
@@ -559,6 +546,30 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
 
 add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
 
+nr_rsvd = fdt_num_mem_rsv(fdt);
+if ( nr_rsvd < 0 )
+panic("Parsing FDT memory reserve map failed (%d)\n", nr_rsvd);
+
+for ( i = 0; i < nr_rsvd; i++ )
+{
+struct membank *bank;
+paddr_t s, sz;
+
+if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, , ) < 0 )
+continue;
+
+if ( reserved_mem->nr_banks < reserved_mem->max_banks )
+{
+bank = _mem->bank[reserved_mem->nr_banks];
+bank->start = s;
+bank->size = sz;
+bank->type = MEMBANK_FDT_RESVMEM;
+reserved_mem->nr_banks++;
+  

Re: [PATCH v2 1/2] x86/spec: fix reporting of BHB clearing usage from guest entry points

[Andrew Cooper]

On 18/04/2024 4:52 pm, Roger Pau Monne wrote:
> Reporting whether the BHB clearing on entry is done for the different domains
> types based on cpu_has_bhb_seq is incorrect, as that variable signals whether

I'd prefer s/incorrect/unhelpful/ here.

As pointed out, it's currently like IBPB-entry in this regard,
identifying whether the block is in place; not whether it's used by the
condition.

> there's a BHB clearing sequence selected, but that alone doesn't imply that
> such sequence is used from the PV and/or HVM entry points.
>
> Instead use opt_bhb_entry_{pv,hvm} which do signal whether BHB clearing is
> performed on entry from PV/HVM.

I was going to ask for a note about dom0, but instead I need to make a
correction to the cmdline docs.  I'll do that in a separate patch.


> Fixes: 689ad48ce9cf ('x86/spec-ctrl: Wire up the Native-BHI software 
> sequences')
> Signed-off-by: Roger Pau Monné 

Reviewed-by: Andrew Cooper 

Can make the one tweak on commit.

Re: [PATCH v4 3/4] livepatch: refuse to resolve symbols that belong to init sections

[Roger Pau Monné]

On Wed, Apr 24, 2024 at 10:19:56AM +0200, Roger Pau Monne wrote:
> Livepatch payloads containing symbols that belong to init sections can only
> lead to page faults later on, as by the time the livepatch is loaded init
> sections have already been freed.
> 
> Refuse to resolve such symbols and return an error instead.
> 
> Note such resolutions are only relevant for symbols that point to undefined
> sections (SHN_UNDEF), as that implies the symbol is not in the current payload
> and hence must either be a Xen or a different livepatch payload symbol.
> 
> Do not allow to resolve symbols that point to __init_begin, as that address is
> also unmapped.  On the other hand, __init_end is not unmapped, and hence allow
> resolutions against it.
> 
> Since __init_begin can alias other symbols (like _erodata for example)
> allow the force flag to override the check and resolve the symbol anyway.

I've been thinking more about this, and it has further corner cases,
for example with this patch applied attempting to livepatch a function
that contains a rune like:

if ( system_state < SYS_STATE_active )
call_init_function();

Will now fail as Xen will refuse to resolve the call_init_function
symbol.

I however don't have any better ideas about how to solve this.  Maybe
it's fine to require such patches to use the --force option, as
refusing to resolve init symbols adds a bit more safety to
livepatches.

Thanks, Roger.

Re: [PATCH v5 2/7] x86/msi: Extend per-domain/device warning mechanism

[Jan Beulich]

On 13.03.2024 16:16, Marek Marczykowski-Górecki wrote:
> The arch_msix struct had a single "warned" field with a domid for which
> warning was issued. Upcoming patch will need similar mechanism for few
> more warnings, so change it to save a bit field of issued warnings.
> 
> Signed-off-by: Marek Marczykowski-Górecki 
> ---
> Should I add also some helper for the boilerplate the handling requires
> now? I tried some macro that also sets the bit field, but I couldn't get
> it working. I guess I could make it working with a bitmask in a single
> uint8_t - would that be preferred?

Without you providing some hint as to where the problem was, it's hard to
see why something like this couldn't work:

#define MSIX_CHECK_WARN(msix, domid, which) ({ \
if ( (msix)->warned_domid != (domid) ) \
{ \
(msix)->warned_domid = (domid); \
(msix)->warned_kind.all = 0; \
} \
(msix)->warned_kind.which ? false : ((msix)->warned_kind.which = true); \
})

> --- a/xen/arch/x86/include/asm/msi.h
> +++ b/xen/arch/x86/include/asm/msi.h
> @@ -217,7 +217,13 @@ struct arch_msix {
>  int table_idx[MAX_MSIX_TABLE_PAGES];
>  spinlock_t table_lock;
>  bool host_maskall, guest_maskall;
> -domid_t warned;
> +domid_t warned_domid;
> +union {
> +uint8_t all;
> +struct {
> +bool maskall   : 1;
> +} u;

No need for giving this struct a name, I don't think.

Jan

Re: [PATCH v5 3/7] x86/hvm: Allow access to registers on the same page as MSI-X table

[Jan Beulich]

On 13.03.2024 16:16, Marek Marczykowski-Górecki wrote:
> Some devices (notably Intel Wifi 6 AX210 card) keep auxiliary registers
> on the same page as MSI-X table. Device model (especially one in
> stubdomain) cannot really handle those, as direct writes to that page is
> refused (page is on the mmio_ro_ranges list). Instead, extend
> msixtbl_mmio_ops to handle such accesses too.
> 
> Doing this, requires correlating read/write location with guest
> of MSI-X table address. Since QEMU doesn't map MSI-X table to the guest,
> it requires msixtbl_entry->gtable, which is HVM-only. Similar feature
> for PV would need to be done separately.
> 
> This will be also used to read Pending Bit Array, if it lives on the same
> page, making QEMU not needing /dev/mem access at all (especially helpful
> with lockdown enabled in dom0). If PBA lives on another page, QEMU will
> map it to the guest directly.
> If PBA lives on the same page, discard writes and log a message.
> Technically, writes outside of PBA could be allowed, but at this moment
> the precise location of PBA isn't saved, and also no known device abuses
> the spec in this way (at least yet).
> 
> To access those registers, msixtbl_mmio_ops need the relevant page
> mapped. MSI handling already has infrastructure for that, using fixmap,
> so try to map first/last page of the MSI-X table (if necessary) and save
> their fixmap indexes. Note that msix_get_fixmap() does reference
> counting and reuses existing mapping, so just call it directly, even if
> the page was mapped before. Also, it uses a specific range of fixmap
> indexes which doesn't include 0, so use 0 as default ("not mapped")
> value - which simplifies code a bit.
> 
> GCC 12.2.1 gets confused about 'desc' variable:
> 
> arch/x86/hvm/vmsi.c: In function ‘msixtbl_range’:
> arch/x86/hvm/vmsi.c:553:8: error: ‘desc’ may be used uninitialized 
> [-Werror=maybe-uninitialized]
>   553 | if ( desc )
>   |^
> arch/x86/hvm/vmsi.c:537:28: note: ‘desc’ was declared here
>   537 | const struct msi_desc *desc;
>   |^~~~
> 
> It's conditional initialization is actually correct (in the case where
> it isn't initialized, function returns early), but to avoid
> build failure initialize it explicitly to NULL anyway.
> 
> Signed-off-by: Marek Marczykowski-Górecki 

Sadly there are further more or less cosmetic issues. Plus, as indicated
before, I'm not really happy for us to gain all of this extra code. In
the end I may eventually give an R-b not including the usually implied
A-b, to indicate the code (then) looks okay to me but I still want
someone else to actually ack it to allow it going in.

> +static int adjacent_read(
> +unsigned int fixmap_idx,
> +paddr_t address, unsigned int len, uint64_t *pval)
> +{
> +const void __iomem *hwaddr;
> +
> +*pval = ~0UL;
> +
> +ASSERT(fixmap_idx != ADJACENT_DISCARD_WRITE);

Why only one of the special values? And before you add the other here:
Why not simply ASSERT(fixmap_idx <= FIX_MSIX_IO_RESERV_END)? (Could of
course bound at the other end, too, i.e. against FIX_MSIX_IO_RESERV_BASE.)

> +hwaddr = fix_to_virt(fixmap_idx) + PAGE_OFFSET(address);
> +
> +switch ( len )
> +{
> +case 1:
> +*pval = readb(hwaddr);
> +break;
> +
> +case 2:
> +*pval = readw(hwaddr);
> +break;
> +
> +case 4:
> +*pval = readl(hwaddr);
> +break;
> +
> +case 8:
> +*pval = readq(hwaddr);
> +break;
> +
> +default:
> +ASSERT_UNREACHABLE();

Misra demands "break;" to be here for release builds. In fact I wonder
why "*pval = ~0UL;" isn't put here, too. Question of course is whether
in such a case a true error indicator wouldn't be yet better.

> +}
> +return X86EMUL_OKAY;
> +}

Like in adjacent_handle(): Blank line please ahead of the function's main
"return".

> +static int adjacent_write(
> +unsigned int fixmap_idx,
> +paddr_t address, unsigned int len, uint64_t val)
> +{
> +void __iomem *hwaddr;
> +
> +if ( fixmap_idx == ADJACENT_DISCARD_WRITE )
> +return X86EMUL_OKAY;

Similar assert as suggested above below here then, too?

> @@ -622,12 +808,15 @@ void msix_write_completion(struct vcpu *v)
>   v->arch.hvm.hvm_io.msix_snoop_gpa )
>  {
>  unsigned int token = hvmemul_cache_disable(v);
> -const struct msi_desc *desc;
> +const struct msi_desc *desc = NULL;
> +const struct msixtbl_entry *entry;
>  uint32_t data;
>  
>  rcu_read_lock(_rcu_lock);
> -desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, snoop_addr),
> -snoop_addr);
> +entry = msixtbl_find_entry(v, snoop_addr);
> +if ( entry && snoop_addr >= entry->gtable &&
> +  snoop_addr < entry->gtable + entry->table_len )

Nit: Unexpected / unusual indentation. If you really want the two 

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 xen  524eee7ce019da69df356a7bdd2174b94a6787b1
baseline version:
 xen  31d6b5b4a7c84818294dacca7a07e92f52393c9d

Last test of basis   185790  2024-04-25 01:00:22 Z0 days
Testing same since   185795  2024-04-25 08:00:24 Z0 days1 attempts


People who touched revisions under test:
  Alessandro Zucchelli 
  Anthony PERARD 
  Jan Beulich 
  Jason Andryuk 
  Jason Andryuk 
  Nicola Vetrini 
  Petr Beneš 
  Roger Pau Monné 

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
   31d6b5b4a7..524eee7ce0  524eee7ce019da69df356a7bdd2174b94a6787b1 -> smoke

Re: [PATCH v5 1/7] x86/msi: passthrough all MSI-X vector ctrl writes to device model

[Jan Beulich]

On 13.03.2024 16:16, Marek Marczykowski-Górecki wrote:
> QEMU needs to know whether clearing maskbit of a vector is really
> clearing, or was already cleared before. Currently Xen sends only
> clearing that bit to the device model, but not setting it, so QEMU
> cannot detect it. Because of that, QEMU is working this around by
> checking via /dev/mem, but that isn't the proper approach.
> 
> Give all necessary information to QEMU by passing all ctrl writes,
> including masking a vector. Advertise the new behavior via
> XENVER_get_features, so QEMU can know it doesn't need to access /dev/mem
> anymore.
> 
> While this commit doesn't move the whole maskbit handling to QEMU (as
> discussed on xen-devel as one of the possibilities), it is a necessary
> first step anyway. Including telling QEMU it will get all the required
> information to do so. The actual implementation would need to include:
>  - a hypercall for QEMU to control just maskbit (without (re)binding the
>interrupt again
>  - a methor for QEMU to tell Xen it will actually do the work
> Those are not part of this series.
> 
> Signed-off-by: Marek Marczykowski-Górecki 

Reviewed-by: Jan Beulich 

> ---
> I did not added any control to enable/disable this new behavior (as
> Roger have suggested for possible non-QEMU ioreqs). I don't see how the
> new behavior could be problematic for some existing ioreq server (they
> already received writes to those addresses, just not all of them),
> but if that's really necessary, I can probably add a command line option
> to restore previous behavior system-wide.

Roger, please indicate whether you consider things to be okay to go in
as is, or whether you demand this earlier concern of yours to be addressed
by adding a command line option (or even finer-grained control).

Jan

[XEN PATCH] MAINTAINERS: Update my email address

[Anthony PERARD]

Signed-off-by: Anthony PERARD 
---
 MAINTAINERS | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index d1850c134d..6ba7d2765f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -208,7 +208,7 @@ Maintainers List (try to look for most precise areas first)
 
 9PFSD
 M: Juergen Gross 
-M: Anthony PERARD 
+M: Anthony PERARD 
 S: Supported
 F: tools/9pfsd/
 
@@ -381,7 +381,7 @@ F:  xen/arch/x86/machine_kexec.c
 F: xen/arch/x86/x86_64/kexec_reloc.S
 
 LIBS
-M: Anthony PERARD 
+M: Anthony PERARD 
 R: Juergen Gross 
 S: Supported
 F: tools/include/libxenvchan.h
@@ -427,7 +427,7 @@ S:  Supported
 F: tools/ocaml/
 
 OVMF UPSTREAM
-M: Anthony PERARD 
+M: Anthony PERARD 
 S: Supported
 T: git https://xenbits.xenproject.org/git-http/ovmf.git
 
@@ -460,7 +460,7 @@ T:  git 
https://xenbits.xenproject.org/git-http/qemu-xen-traditional.git
 
 QEMU UPSTREAM
 M: Stefano Stabellini 
-M: Anthony Perard 
+M: Anthony Perard 
 S: Supported
 T: git https://xenbits.xenproject.org/git-http/qemu-xen.git
 
@@ -512,7 +512,7 @@ F:  xen/arch/arm/include/asm/tee
 F: xen/arch/arm/tee/
 
 TOOLSTACK
-M: Anthony PERARD 
+M: Anthony PERARD 
 S: Supported
 F: autogen.sh
 F: config/*.in
-- 
Anthony PERARD

Re: [PATCH v1] tools/ocaml: fix warnings

[Christian Lindig]

> On 27 Mar 2024, at 16:30, Edwin Török  wrote:
> 
> Do not rely on the string values of the `Failure` exception,
> but use the `_opt` functions instead.
> 
> Signed-off-by: Edwin Török 
> ---
> tools/ocaml/xenstored/config.ml | 20 +++-
> 1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/ocaml/xenstored/config.ml b/tools/ocaml/xenstored/config.ml
> index 95ef745a54..e0df236f73 100644
> --- a/tools/ocaml/xenstored/config.ml
> +++ b/tools/ocaml/xenstored/config.ml
> @@ -83,25 +83,27 @@ let validate cf expected other =
>   let err = ref [] in
>   let append x = err := x :: !err in
>   List.iter (fun (k, v) ->
> +  let parse ~err_msg parser v f =
> +match parser v with
> +| None -> append (k, err_msg)
> +| Some r -> f r
> +  in
>   try
> if not (List.mem_assoc k expected) then
>   other k v
> else let ty = List.assoc k expected in
>   match ty with
>   | Unit f   -> f ()
> -  | Bool f   -> f (bool_of_string v)
> +  | Bool f   -> parse ~err_msg:"expect bool arg" 
> bool_of_string_opt v f
>   | String f -> f v
> -  | Int f-> f (int_of_string v)
> -  | Float f  -> f (float_of_string v)
> -  | Set_bool r   -> r := (bool_of_string v)
> +  | Int f-> parse ~err_msg:"expect int arg" 
> int_of_string_opt v f
> +  | Float f  -> parse ~err_msg:"expect float arg" 
> float_of_string_opt v f
> +  | Set_bool r   -> parse ~err_msg:"expect bool arg" 
> bool_of_string_opt v (fun x -> r := x)
>   | Set_string r -> r := v
> -  | Set_int r-> r := int_of_string v
> -  | Set_float r  -> r := (float_of_string v)
> +  | Set_int r-> parse ~err_msg:"expect int arg" 
> int_of_string_opt v (fun x -> r:= x)
> +  | Set_float r  -> parse ~err_msg:"expect float arg" 
> float_of_string_opt v (fun x -> r := x)
>   with
>   | Not_found -> append (k, "unknown key")
> -  | Failure "int_of_string"   -> append (k, "expect int arg")
> -  | Failure "bool_of_string"  -> append (k, "expect bool arg")
> -  | Failure "float_of_string" -> append (k, "expect float arg")
>   | exn   -> append (k, Printexc.to_string exn)
> ) cf;
>   if !err != [] then raise (Error !err)
> -- 
> 2.44.0
> 


Acked-by: Christian Lindig 

Re: [PATCH 3/7] tools/xl: Add max_altp2m parameter

[Jan Beulich]

On 25.04.2024 10:51, Petr Beneš wrote:
> On Thu, Apr 25, 2024 at 8:19 AM Jan Beulich  wrote:
>>
>> On 24.04.2024 22:42, Petr Beneš wrote:
>>> Introduce a new max_altp2m parameter to control the maximum number of altp2m
>>> views a domain can use. By default, if max_altp2m is unspecified and altp2m 
>>> is
>>> enabled, the value is set to 10, reflecting the legacy behavior.
>>
>> Apart from the public header you don't even touch hypervisor code here. What
>> you say above therefore is limited in scope to the tool stack. I question
>> though that adding a new field without respecting it constitutes an overall
>> consistent change. In particular I'm curious how you mean to deal with this
>> for other than x86, where altp2m as a concept doesn't exist (yet).
>>
>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -77,6 +77,7 @@ struct xen_domctl_createdomain {
>>>   */
>>>  uint32_t max_vcpus;
>>>  uint32_t max_evtchn_port;
>>> +uint32_t max_altp2m;
>>>  int32_t max_grant_frames;
>>>  int32_t max_maptrack_frames;
>>
>> Such a struct layout change needs to be accompanied by an interface version
>> bump (which hasn't happened so far in this release cycle, afaics).
> 
> So I should not include domctl.h changes in this commit and instead
> edit the commit message that it's merely a preparation for the
> following commit.
> Then, move the xen_domctl_createdomain field creation to the commit
> with the hypervisor changes (+ include interface version bump) and
> then create an additional commit that ties xl and xen together.
> 
> Do I understand it correctly?

That's one way of approaching it, yes.

Jan

Re: [PATCH] svm: Fix MISRA 8.2 violation

[Jan Beulich]

On 25.04.2024 11:12, George Dunlap wrote:
> Misra 8.2 requires named parameters in prototypes.  Use the name from
> the implementaiton.
> 
> Fixes 0d19d3aab0 ("svm/nestedsvm: Introduce nested capabilities bit").
> 
> Signed-off-by: George Dunlap 

Acked-by: Jan Beulich 

Re: [XEN PATCH] automation/eclair: reorganize pipelines

[Federico Serafini]

On 25/04/24 02:06, Stefano Stabellini wrote:

On Tue, 23 Apr 2024, Federico Serafini wrote:

From: Simone Ballarin 

Introduce accepted_guidelines.sh: a script to autogenerate the
configuration file accepted.ecl from docs/misra/rules.rst which enables
all accepted guidelines.

Introduce monitored.ecl: a manual selection of accepted guidelines
which are clean or almost clean, it is intended to be used for the
analyses triggered by commits.

Reorganize tagging.ecl:
   -Remove "accepted" tags: keeping track of accepted guidelines tagging
them as "accepted" in the configuration file tagging.ecl is no
longer needed since docs/rules.rst is keeping track of them.
   -Tag more guidelines as clean.

Reorganize eclair pipelines:
   - Set1, Set2, Set3 are now obsolete: remove the corresponding
 pipelines and ecl files.
   - Amend scheduled eclair pipeline to use accepted.ecl.
   - Amend triggered eclair pipeline to use monitored.ecl.

Rename and improve action_check_clean_regressions.sh to print a
diagnostic in case a commit introduces a violation of a clean guideline.

An example of diagnostic is the following:

Failure: 13 regressions found for clean guidelines
   service MC3R1.R8.2: (required) Function types shall be in prototype form 
with named parameters:
violation: 13

Signed-off-by: Simone Ballarin 
Signed-off-by: Federico Serafini 
Signed-off-by: Alessandro Zucchelli 


Fantastic work, thank you!

Reviewed-by: Stefano Stabellini 

Is this patch safe to commit now? Or would it cause gitlab-ci breakage?


Yes, it is safe because the ECLAIR analysis is still allowed to fail.
Committing this patch wouldn't break the CI but it will highlight some 
regressions with the orange badge and the following messages:


arm:

Failure: 5 regressions found for clean guidelines
  service MC3R1.R1.1: (required) The program shall contain no 
violations of the standard C syntax and constraints, and shall not 
exceed the implementation's translation limits:

   violation: 5

x86:

Failure: 2 regressions found for clean guidelines
  service MC3R1.R8.2: (required) Function types shall be in prototype 
form with named parameters:

   violation: 2

(George just sent a patch to address the regressions of Rule 8.2.)



One question below.



-
  
  # Clean guidelines #
  
  
  -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.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"

+-service_selector={clean_guidelines_common,"MC3R1.D1.1||MC3R1.D2.1||MC3R1.D4.1||MC3R1.D4.11||MC3R1.D4.14||MC3R1.R1.1||MC3R1.R11.7||MC3R1.R11.9||MC3R1.R12.5||MC3R1.R1.3||MC3R1.R1.4||MC3R1.R14.1||MC3R1.R16.7||MC3R1.R17.1||MC3R1.R17.3||MC3R1.R17.4||MC3R1.R17.5||MC3R1.R17.6||MC3R1.R20.13||MC3R1.R20.14||MC3R1.R20.4||MC3R1.R20.9||MC3R1.R21.13||MC3R1.R21.19||MC3R1.R21.21||MC3R1.R2.2||MC3R1.R22.2||MC3R1.R22.4||MC3R1.R22.5||MC3R1.R22.6||MC3R1.R2.6||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.R7.4||MC3R1.R8.1||MC3R1.R8.10||MC3R1.R8.12||MC3R1.R8.14||MC3R1.R8.2||MC3R1.R8.5||MC3R1.R8.6||MC3R1.R8.8||MC3R1.R9.2||MC3R1.R9.3||MC3R1.R9.4||MC3R1.R9.5"
  }


Is this list different from monitored.ecl? If so, why? If not, maybe we
don't need to repeat the list here as well?


Quick answer: this list is different from monitored.ecl and the two
lists must coexist.

Here, we are "tagging" some guidelines as "clean":
this list is crucial and will be (manually) updated every time a new
guideline reaches 0 violations, it shall not be removed because this tag
allows ECLAIR to print a diagnostic and fail in case unjustified
violations are found for the tagged guidelines.

The monitored.ecl is the list of guidelines which are analyzed at each
commit: the list shall include all the guidelines tagged as "clean"
(to do the proper regressions checks) but the monitored list can also
include some accepted guidelines for which it may be interesting to see
the number of violations at each commit, for example, we put there some
almost-clean guidelines (guidelines with few violations left but not yet
tagged as clean yet).
Introducing new violations of monitored but not-clean guidelines will
not cause a failure.

--
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

[PATCH] svm: Fix MISRA 8.2 violation

[George Dunlap]

Misra 8.2 requires named parameters in prototypes.  Use the name from
the implementaiton.

Fixes 0d19d3aab0 ("svm/nestedsvm: Introduce nested capabilities bit").

Signed-off-by: George Dunlap 
---
CC: Andrew Cooper 
CC: Jan Beulich 
CC: Roger Pau Monne 
CC: Nicola Vetrini 
---
 xen/arch/x86/include/asm/hvm/nestedhvm.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/include/asm/hvm/nestedhvm.h 
b/xen/arch/x86/include/asm/hvm/nestedhvm.h
index 0568acb25f..ea2c1bc328 100644
--- a/xen/arch/x86/include/asm/hvm/nestedhvm.h
+++ b/xen/arch/x86/include/asm/hvm/nestedhvm.h
@@ -83,7 +83,7 @@ static inline bool vvmcx_valid(const struct vcpu *v)
 }
 
 
-void start_nested_svm(struct hvm_function_table *);
-void start_nested_vmx(struct hvm_function_table *);
+void start_nested_svm(struct hvm_function_table *hvm_function_table);
+void start_nested_vmx(struct hvm_function_table *hvm_function_table);
 
 #endif /* _HVM_NESTEDHVM_H */
-- 
2.25.1

[ANNOUNCE] Call for agenda items - May 2024 Community Call

[Kelly Choi]

Hi all,

Please add your proposed agenda items below.

https://cryptpad.fr/pad/#/2/pad/edit/FluG1Qk1UTGr3CdvJBXTk6AF/

If any action items are missing or have been resolved, please add/remove
them from the sheet.

*CALL LINK: https://meet.jit.si/XenProjectCommunityCall
*

*DATE: Thursday 2nd May 2024*

*TIME: 1500 UTC (4 pm UK time)*
*Note the following administrative conventions for the call:*












** To allow time to switch between meetings, we plan on starting theagenda
at 16:05 UTC sharp.  Aim to join by 16:03 UTC if possible to allocatetime
to sort out technical difficulties.* If you want to be CC'ed please add or
remove yourself from thesign-up-sheet
at https://cryptpad.fr/pad/#/2/pad/edit/D9vGzihPxxAOe6RFPz0sRCf+/
== Dial-in
Information ==## Meeting time16:00 - 17:00 British timeFurther
International meeting times:*
https://www.timeanddate.com/worldclock/meetingdetails.html?year=2024=5=2=15=0=0=1234=37=224=179


## Dial in details
https://meet.jit.si/static/dialInInfo.html?room=XenProjectCommunityCall

Many thanks,
Kelly Choi

Community Manager
Xen Project

Re: [PATCH 4/7] tools/ocaml: Add max_altp2m parameter

[Christian Lindig]

> On 24 Apr 2024, at 21:42, Petr Beneš  wrote:
> 
> From: Petr Beneš 
> 
> Allow developers using the OCaml bindings to set the max_altp2m parameter.
> 
> Signed-off-by: Petr Beneš 
> ---
> tools/ocaml/libs/xc/xenctrl.ml  |  1 +
> tools/ocaml/libs/xc/xenctrl.mli |  1 +
> tools/ocaml/libs/xc/xenctrl_stubs.c | 17 ++---
> 3 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
> index 55923857ec..ed851bb071 100644
> --- a/tools/ocaml/libs/xc/xenctrl.ml
> +++ b/tools/ocaml/libs/xc/xenctrl.ml
> @@ -82,6 +82,7 @@ type domctl_create_config =
> iommu_opts: domain_create_iommu_opts list;
> max_vcpus: int;
> max_evtchn_port: int;
> +max_altp2m: int;
> max_grant_frames: int;
> max_maptrack_frames: int;
> max_grant_version: int;
> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
> index 9b4b45db3a..971b269d85 100644
> --- a/tools/ocaml/libs/xc/xenctrl.mli
> +++ b/tools/ocaml/libs/xc/xenctrl.mli
> @@ -74,6 +74,7 @@ type domctl_create_config = {
>   iommu_opts: domain_create_iommu_opts list;
>   max_vcpus: int;
>   max_evtchn_port: int;
> +  max_altp2m: int;
>   max_grant_frames: int;
>   max_maptrack_frames: int;
>   max_grant_version: int;
> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c 
> b/tools/ocaml/libs/xc/xenctrl_stubs.c
> index 2b6d3c09df..0b70cc9b08 100644
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -207,12 +207,13 @@ CAMLprim value stub_xc_domain_create(value xch_val, 
> value wanted_domid, value co
> #define VAL_IOMMU_OPTS  Field(config, 3)
> #define VAL_MAX_VCPUS   Field(config, 4)
> #define VAL_MAX_EVTCHN_PORT Field(config, 5)
> -#define VAL_MAX_GRANT_FRAMESField(config, 6)
> -#define VAL_MAX_MAPTRACK_FRAMES Field(config, 7)
> -#define VAL_MAX_GRANT_VERSION   Field(config, 8)
> -#define VAL_VMTRACE_BUF_KB  Field(config, 9)
> -#define VAL_CPUPOOL_ID  Field(config, 10)
> -#define VAL_ARCHField(config, 11)
> +#define VAL_MAX_ALTP2M  Field(config, 6)
> +#define VAL_MAX_GRANT_FRAMESField(config, 7)
> +#define VAL_MAX_MAPTRACK_FRAMES Field(config, 8)
> +#define VAL_MAX_GRANT_VERSION   Field(config, 9)
> +#define VAL_VMTRACE_BUF_KB  Field(config, 10)
> +#define VAL_CPUPOOL_ID  Field(config, 11)
> +#define VAL_ARCHField(config, 12)
> 
> uint32_t domid = Int_val(wanted_domid);
> uint64_t vmtrace_size = Int32_val(VAL_VMTRACE_BUF_KB);
> @@ -226,6 +227,7 @@ CAMLprim value stub_xc_domain_create(value xch_val, value 
> wanted_domid, value co
> .ssidref = Int32_val(VAL_SSIDREF),
> .max_vcpus = Int_val(VAL_MAX_VCPUS),
> .max_evtchn_port = Int_val(VAL_MAX_EVTCHN_PORT),
> + .max_altp2m = Int_val(VAL_MAX_ALTP2M),
> .max_grant_frames = Int_val(VAL_MAX_GRANT_FRAMES),
> .max_maptrack_frames = Int_val(VAL_MAX_MAPTRACK_FRAMES),
> .grant_opts =
> @@ -257,7 +259,7 @@ CAMLprim value stub_xc_domain_create(value xch_val, value 
> wanted_domid, value co
> #if defined(__i386__) || defined(__x86_64__)
> 
> /* Quick & dirty check for ABI changes. */
> - BUILD_BUG_ON(sizeof(cfg) != 64);
> + BUILD_BUG_ON(sizeof(cfg) != 68);
> 
> /* Mnemonics for the named fields inside xen_x86_arch_domainconfig */
> #define VAL_EMUL_FLAGS  Field(arch_domconfig, 0)
> @@ -291,6 +293,7 @@ CAMLprim value stub_xc_domain_create(value xch_val, value 
> wanted_domid, value co
> #undef VAL_MAX_GRANT_VERSION
> #undef VAL_MAX_MAPTRACK_FRAMES
> #undef VAL_MAX_GRANT_FRAMES
> +#undef VAL_MAX_ALTP2M
> #undef VAL_MAX_EVTCHN_PORT
> #undef VAL_MAX_VCPUS
> #undef VAL_IOMMU_OPTS
> -- 
> 2.34.1
> 

This looks correct from an OCaml perspective.

Acked-by: Christian Lindig 

Re: [PATCH 3/7] tools/xl: Add max_altp2m parameter

[Petr Beneš]

On Thu, Apr 25, 2024 at 8:19 AM Jan Beulich  wrote:
>
> On 24.04.2024 22:42, Petr Beneš wrote:
> > Introduce a new max_altp2m parameter to control the maximum number of altp2m
> > views a domain can use. By default, if max_altp2m is unspecified and altp2m 
> > is
> > enabled, the value is set to 10, reflecting the legacy behavior.
>
> Apart from the public header you don't even touch hypervisor code here. What
> you say above therefore is limited in scope to the tool stack. I question
> though that adding a new field without respecting it constitutes an overall
> consistent change. In particular I'm curious how you mean to deal with this
> for other than x86, where altp2m as a concept doesn't exist (yet).
>
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -77,6 +77,7 @@ struct xen_domctl_createdomain {
> >   */
> >  uint32_t max_vcpus;
> >  uint32_t max_evtchn_port;
> > +uint32_t max_altp2m;
> >  int32_t max_grant_frames;
> >  int32_t max_maptrack_frames;
>
> Such a struct layout change needs to be accompanied by an interface version
> bump (which hasn't happened so far in this release cycle, afaics).
>
> Jan

So I should not include domctl.h changes in this commit and instead
edit the commit message that it's merely a preparation for the
following commit.
Then, move the xen_domctl_createdomain field creation to the commit
with the hypervisor changes (+ include interface version bump) and
then create an additional commit that ties xl and xen together.

Do I understand it correctly?

Re: [PATCH 1/7] x86/p2m: Add braces for better code clarity

[Petr Beneš]

On Thu, Apr 25, 2024 at 8:21 AM Jan Beulich  wrote:
>
> On 24.04.2024 22:41, Petr Beneš wrote:
> > From: Petr Beneš 
> >
> > No functional change.
> >
> > Signed-off-by: Petr Beneš 
>
> Hmm. I don't really mind the extra braces, but I also don't really see a need.
> IOW this is not an objection, but it'll want to be someone else (if anyone) to
> ack this.
>
> Jan
>
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -106,6 +106,7 @@ void p2m_change_entry_type_global(struct domain *d,
> >  unsigned int i;
> >
> >  for ( i = 0; i < MAX_ALTP2M; i++ )
> > +{
> >  if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
> >  {
> >  struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
> > @@ -114,6 +115,7 @@ void p2m_change_entry_type_global(struct domain *d,
> >  change_entry_type_global(altp2m, ot, nt);
> >  p2m_unlock(altp2m);
> >  }
> > +}
> >  }
> >
> >  p2m_unlock(hostp2m);
> > @@ -139,6 +141,7 @@ void p2m_memory_type_changed(struct domain *d)
> >  unsigned int i;
> >
> >  for ( i = 0; i < MAX_ALTP2M; i++ )
> > +{
> >  if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
> >  {
> >  struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
> > @@ -147,6 +150,7 @@ void p2m_memory_type_changed(struct domain *d)
> >  _memory_type_changed(altp2m);
> >  p2m_unlock(altp2m);
> >  }
> > +}
> >  }
> >
> >  p2m_unlock(hostp2m);
>

I should have specified that it builds on commit
5205bda5f11cc03ca62ad2bb6c34bf738bbb3247, which did some coding style
cleanup and added braces to several places too, but missed these two.

Re: [PATCH] arm/vpci: make prefetchable mem 64 bit

[Rahul Singh]

Hi Stewart,

> On 24 Apr 2024, at 5:27 PM, Stewart Hildebrand  
> wrote:
> 
> The vPCI prefetchable memory range is >= 4GB, so the memory space flags
> should be set to 64-bit. See IEEE Std 1275-1994 [1] for a definition of
> the field.
> 
> [1] https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf
> 
> Signed-off-by: Stewart Hildebrand 

Reviewed-by: Rahul Singh 

Regards,
Rahul 
 

Re: [PATCH v3] xen/x86/pvh: handle ACPI RSDT table in PVH Dom0 build

[Roger Pau Monné]

On Thu, Apr 25, 2024 at 09:26:59AM +0200, Jan Beulich wrote:
> On 25.04.2024 09:10, Roger Pau Monné wrote:
> > On Thu, Apr 25, 2024 at 08:12:09AM +0200, Jan Beulich wrote:
> >> On 24.04.2024 21:18, Daniel P. Smith wrote:
> >>> @@ -1089,6 +1098,9 @@ static int __init pvh_setup_acpi_xsdt(struct domain 
> >>> *d, paddr_t madt_addr,
> >>>  xsdt->header = *table;
> >>>  acpi_os_unmap_memory(table, sizeof(*table));
> >>>  
> >>> +/* In case the header is an RSDT copy, blindly ensure it has an XSDT 
> >>> sig */
> >>> +xsdt->header.signature[0] = 'X';
> >>
> >> This is in no way "blindly". The size of the table elements being different
> >> between RSDT and XSDT makes it mandatory to have the correct signature. 
> >> Else
> >> the consumer of the struct is going to be misled.
> > 
> > The "blindly" IMO refers to the fact that the table might already have
> > the right signature, but this is not checked, IOW we could do:
> > 
> > if ( xsdt->header.signature[0] == 'R' )
> > xsdt->header.signature[0] = 'X';
> 
> Right, and indeed I was seeing this as a possible further interpretation.
> Yet given multiple ways of reading this, I'm of the opinion that it wants
> adjusting. ", ... unconditionally ensure it has ... " may already do.
> Simply dropping "blindly" would too be okay with me.

FWIW, I'm fine with any of the proposed adjustments.

Thanks, Roger.

Re: [PATCH v3] xen/x86/pvh: handle ACPI RSDT table in PVH Dom0 build

[Jan Beulich]

On 25.04.2024 09:10, Roger Pau Monné wrote:
> On Thu, Apr 25, 2024 at 08:12:09AM +0200, Jan Beulich wrote:
>> On 24.04.2024 21:18, Daniel P. Smith wrote:
>>> @@ -1089,6 +1098,9 @@ static int __init pvh_setup_acpi_xsdt(struct domain 
>>> *d, paddr_t madt_addr,
>>>  xsdt->header = *table;
>>>  acpi_os_unmap_memory(table, sizeof(*table));
>>>  
>>> +/* In case the header is an RSDT copy, blindly ensure it has an XSDT 
>>> sig */
>>> +xsdt->header.signature[0] = 'X';
>>
>> This is in no way "blindly". The size of the table elements being different
>> between RSDT and XSDT makes it mandatory to have the correct signature. Else
>> the consumer of the struct is going to be misled.
> 
> The "blindly" IMO refers to the fact that the table might already have
> the right signature, but this is not checked, IOW we could do:
> 
> if ( xsdt->header.signature[0] == 'R' )
> xsdt->header.signature[0] = 'X';

Right, and indeed I was seeing this as a possible further interpretation.
Yet given multiple ways of reading this, I'm of the opinion that it wants
adjusting. ", ... unconditionally ensure it has ... " may already do.
Simply dropping "blindly" would too be okay with me.

Jan

Re: [PATCH v3] xen/x86/pvh: handle ACPI RSDT table in PVH Dom0 build

[Roger Pau Monné]

On Thu, Apr 25, 2024 at 08:12:09AM +0200, Jan Beulich wrote:
> On 24.04.2024 21:18, Daniel P. Smith wrote:
> > @@ -1089,6 +1098,9 @@ static int __init pvh_setup_acpi_xsdt(struct domain 
> > *d, paddr_t madt_addr,
> >  xsdt->header = *table;
> >  acpi_os_unmap_memory(table, sizeof(*table));
> >  
> > +/* In case the header is an RSDT copy, blindly ensure it has an XSDT 
> > sig */
> > +xsdt->header.signature[0] = 'X';
> 
> This is in no way "blindly". The size of the table elements being different
> between RSDT and XSDT makes it mandatory to have the correct signature. Else
> the consumer of the struct is going to be misled.

The "blindly" IMO refers to the fact that the table might already have
the right signature, but this is not checked, IOW we could do:

if ( xsdt->header.signature[0] == 'R' )
xsdt->header.signature[0] = 'X';

Regards, Roger.

Re: [PATCH v3] xen/x86/pvh: handle ACPI RSDT table in PVH Dom0 build

[Roger Pau Monné]

On Wed, Apr 24, 2024 at 03:18:26PM -0400, Daniel P. Smith wrote:
> From: Stefano Stabellini 
> 
> Xen always generates as XSDT table even if the firmware provided an RSDT 
> table.
 ^ only

As providing an RSDT doesn't exclude from a XSDT also being provided.

> Copy the RSDT header from the firmware table, adjusting the signature, for the
> XSDT table when not provided by the firmware.

Either here, or in the code comment below (or in both places), I would
detail that this is required for QEMU, that only exposes RSDT but no
XSDT.

> Fixes: 1d74282c455f ('x86: setup PVHv2 Dom0 ACPI tables')
> Suggested-by: Roger Pau Monné 
> Signed-off-by: Stefano Stabellini 
> Signed-off-by: Daniel P. Smith 

Reviewed-by: Roger Pau Monné 

Thanks, Roger.

Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Henry Wang]

Hi Julien,

On 4/24/2024 8:58 PM, Julien Grall wrote:

Hi Henry,

On 24/04/2024 04:34, Henry Wang wrote:

From: Vikram Garhwal 

Enable interrupt assign/remove for running VMs in CONFIG_OVERLAY_DTB.

Currently, irq_route and mapping is only allowed at the domain 
creation. Adding

exception for CONFIG_OVERLAY_DTB.


AFAICT, this is mostly reverting b8577547236f ("xen/arm: Restrict when 
a physical IRQ can be routed/removed from/to a domain").




Signed-off-by: Vikram Garhwal 
Signed-off-by: Stefano Stabellini 
Signed-off-by: Henry Wang 
---
  xen/arch/arm/gic.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 44c40e86de..a775f886ed 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -140,8 +140,10 @@ int gic_route_irq_to_guest(struct domain *d, 
unsigned int virq,

   * back to the physical IRQ. To prevent get unsync, restrict the
   * routing to when the Domain is been created.
   */


The above comment explains why the check was added. But the commit 
message doesn't explain why this can be disregarded for your use-case.


Looking at the history, I don't think you can simply remove the checks.

Regardless that...


+#ifndef CONFIG_OVERLAY_DTB


... I am against such #ifdef. A distros may want to have OVERLAY_DTB 
enabled, yet the user will not use it.


Instead, you want to remove the check once the code can properly 
handle routing an IRQ the domain is created or ...



  if ( d->creation_finished )
  return -EBUSY;
+#endif
    ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
  if ( ret )
@@ -171,8 +173,10 @@ int gic_remove_irq_from_guest(struct domain *d, 
unsigned int virq,

   * Removing an interrupt while the domain is running may have
   * undesirable effect on the vGIC emulation.
   */
+#ifndef CONFIG_OVERLAY_DTB
  if ( !d->is_dying )
  return -EBUSY;
+#endif


... removed before they domain is destroyed.


Thanks for your feeedback. After checking the b8577547236f commit 
message I think I now understand your point. Do you have any suggestion 
about how can I properly add the support to route/remove the IRQ to 
running domains? Thanks.


Kind regards,
Henry




desc->handler->shutdown(desc);


Cheers,

Re: [PATCH 14/15] add a domU script to fetch overlays and applying them to linux

[Henry Wang]

Hi Jan,

On 4/25/2024 2:46 PM, Jan Beulich wrote:

On 25.04.2024 02:54, Henry Wang wrote:

On 4/24/2024 2:16 PM, Jan Beulich wrote:

On 24.04.2024 05:34, Henry Wang wrote:

From: Vikram Garhwal 

Introduce a shell script that runs in the background and calls
get_overlay to retrive overlays and add them (or remove them) to Linux
device tree (running as a domU).

Signed-off-by: Vikram Garhwal 
Signed-off-by: Stefano Stabellini 
Signed-off-by: Henry Wang 
---
   tools/helpers/Makefile   |  2 +-
   tools/helpers/get_overlay.sh | 81 
   2 files changed, 82 insertions(+), 1 deletion(-)
   create mode 100755 tools/helpers/get_overlay.sh

Besides the same naming issue as in the earlier patch, the script also
looks very Linux-ish. Yet ...

I will fix the naming issue in v2. Would you mind elaborating a bit more
about the "Linux-ish" concern? I guess this is because the original use
case is on Linux, should I do anything about this?

Well, the script won't work on other than Linux, will it? Therefore ...


--- a/tools/helpers/Makefile
+++ b/tools/helpers/Makefile
@@ -58,7 +58,6 @@ init-dom0less: $(INIT_DOM0LESS_OBJS)
   get_overlay: $(SHARE_OVERLAY_OBJS)
$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore) 
$(LDLIBS_libxenctrl) $(LDLIBS_libxengnttab) $(APPEND_LDFLAGS)
   
-

   .PHONY: install
   install: all
$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
@@ -67,6 +66,7 @@ install: all
   .PHONY: uninstall
   uninstall:
for i in $(TARGETS); do rm -f $(DESTDIR)$(LIBEXEC_BIN)/$$i; done
+   $(RM) $(DESTDIR)$(LIBEXEC_BIN)/get_overlay.sh
   
   .PHONY: clean

   clean:

... you touching only the uninstall target, it's not even clear to me
how (and under what conditions) the script is going to make it into
$(DESTDIR)$(LIBEXEC_BIN)/. Did you mean to add to $(TARGETS), perhaps,
alongside the earlier added get-overlay binary?

... it first of needs to become clear under what conditions it is actually
going to be installed.


You are right, I think the get-overlay binary and this script should be
installed if DTB overlay is supported. Checking the code, I found
LIBXL_HAVE_DT_OVERLAY which can indicate if we have this feature
supported in libxl. Do you think it is a good idea to use it to install
these two files in Makefile? Thanks.

Counter question: If it's not going to be installed, how are people going
to make use of it? If the script is intended for manual use only, I think
that would want saying in the description. Yet then I couldn't see why
the uninstall goal would need modifying.


Checking the code again, I feel like this is a mistake actually. I think 
this script should be installed together with the get-overlay 
application as the script actually calls get-overlay. The uninstall goal 
should remain untouched. I will fix this in v2.



As to LIBXL_HAVE_DT_OVERLAY - that's not accessible from a Makefile, I
guess?


Yes.

Kind regards,
Henry



Jan

Re: [PATCH 14/15] add a domU script to fetch overlays and applying them to linux

[Jan Beulich]

On 25.04.2024 02:54, Henry Wang wrote:
> On 4/24/2024 2:16 PM, Jan Beulich wrote:
>> On 24.04.2024 05:34, Henry Wang wrote:
>>> From: Vikram Garhwal 
>>>
>>> Introduce a shell script that runs in the background and calls
>>> get_overlay to retrive overlays and add them (or remove them) to Linux
>>> device tree (running as a domU).
>>>
>>> Signed-off-by: Vikram Garhwal 
>>> Signed-off-by: Stefano Stabellini 
>>> Signed-off-by: Henry Wang 
>>> ---
>>>   tools/helpers/Makefile   |  2 +-
>>>   tools/helpers/get_overlay.sh | 81 
>>>   2 files changed, 82 insertions(+), 1 deletion(-)
>>>   create mode 100755 tools/helpers/get_overlay.sh
>> Besides the same naming issue as in the earlier patch, the script also
>> looks very Linux-ish. Yet ...
> 
> I will fix the naming issue in v2. Would you mind elaborating a bit more 
> about the "Linux-ish" concern? I guess this is because the original use 
> case is on Linux, should I do anything about this?

Well, the script won't work on other than Linux, will it? Therefore ...

>>> --- a/tools/helpers/Makefile
>>> +++ b/tools/helpers/Makefile
>>> @@ -58,7 +58,6 @@ init-dom0less: $(INIT_DOM0LESS_OBJS)
>>>   get_overlay: $(SHARE_OVERLAY_OBJS)
>>> $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore) 
>>> $(LDLIBS_libxenctrl) $(LDLIBS_libxengnttab) $(APPEND_LDFLAGS)
>>>   
>>> -
>>>   .PHONY: install
>>>   install: all
>>> $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
>>> @@ -67,6 +66,7 @@ install: all
>>>   .PHONY: uninstall
>>>   uninstall:
>>> for i in $(TARGETS); do rm -f $(DESTDIR)$(LIBEXEC_BIN)/$$i; done
>>> +   $(RM) $(DESTDIR)$(LIBEXEC_BIN)/get_overlay.sh
>>>   
>>>   .PHONY: clean
>>>   clean:
>> ... you touching only the uninstall target, it's not even clear to me
>> how (and under what conditions) the script is going to make it into
>> $(DESTDIR)$(LIBEXEC_BIN)/. Did you mean to add to $(TARGETS), perhaps,
>> alongside the earlier added get-overlay binary?

... it first of needs to become clear under what conditions it is actually
going to be installed.

> You are right, I think the get-overlay binary and this script should be 
> installed if DTB overlay is supported. Checking the code, I found 
> LIBXL_HAVE_DT_OVERLAY which can indicate if we have this feature 
> supported in libxl. Do you think it is a good idea to use it to install 
> these two files in Makefile? Thanks.

Counter question: If it's not going to be installed, how are people going
to make use of it? If the script is intended for manual use only, I think
that would want saying in the description. Yet then I couldn't see why
the uninstall goal would need modifying.

As to LIBXL_HAVE_DT_OVERLAY - that's not accessible from a Makefile, I
guess?

Jan

Re: [linux-linus test] 185785: regressions - FAIL

[Jan Beulich]

On 25.04.2024 03:38, osstest service owner wrote:
> flight 185785 linux-linus real [real]
> http://logs.test-lab.xenproject.org/osstest/logs/185785/
> 
> Regressions :-(
> 
> Tests which did not succeed and are blocking,
> including tests which could not be run:
>  test-armhf-armhf-xl-raw   8 xen-boot fail REGR. vs. 
> 185768
>  test-armhf-armhf-libvirt-vhd  8 xen-boot fail REGR. vs. 
> 185768

This looks to be recurring. From one of the logs:

Apr 24 21:09:04.301751 Configuring network interfaces...RTNETLINK answers: 
Operation not supported
Apr 24 21:09:05.033646 done.
Apr 24 21:09:05.033646 Cleaning up temporary files
Apr 24 21:09:05.105744 Setting up X socket directories... /tmp/.X11-unix 
/tmp/.ICE-unix.
Apr 24 21:09:05.381747 Starting nftables: none
Apr 24 21:09:05.405781 �mnl.c:60: Unable to initialize Netlink socket: Protocol 
not supported
Apr 24 21:09:05.837763  failed!
Apr 24 21:09:05.837817 startpar: service(s) returned failure: nftables ... 
failed!

Hints at a possible network setup issue, which would explain the timeout
in trying to ping the host.

Jan

Re: [PATCH 2/7] x86/hap: Refactor boolean field assignments

[Jan Beulich]

On 24.04.2024 22:42, Petr Beneš wrote:
> From: Petr Beneš 
> 
> No functional change.
> 
> Signed-off-by: Petr Beneš 

Acked-by: Jan Beulich 

Re: [PATCH 1/7] x86/p2m: Add braces for better code clarity

[Jan Beulich]

On 24.04.2024 22:41, Petr Beneš wrote:
> From: Petr Beneš 
> 
> No functional change.
> 
> Signed-off-by: Petr Beneš 

Hmm. I don't really mind the extra braces, but I also don't really see a need.
IOW this is not an objection, but it'll want to be someone else (if anyone) to
ack this.

Jan

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -106,6 +106,7 @@ void p2m_change_entry_type_global(struct domain *d,
>  unsigned int i;
>  
>  for ( i = 0; i < MAX_ALTP2M; i++ )
> +{
>  if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
>  {
>  struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
> @@ -114,6 +115,7 @@ void p2m_change_entry_type_global(struct domain *d,
>  change_entry_type_global(altp2m, ot, nt);
>  p2m_unlock(altp2m);
>  }
> +}
>  }
>  
>  p2m_unlock(hostp2m);
> @@ -139,6 +141,7 @@ void p2m_memory_type_changed(struct domain *d)
>  unsigned int i;
>  
>  for ( i = 0; i < MAX_ALTP2M; i++ )
> +{
>  if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
>  {
>  struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
> @@ -147,6 +150,7 @@ void p2m_memory_type_changed(struct domain *d)
>  _memory_type_changed(altp2m);
>  p2m_unlock(altp2m);
>  }
> +}
>  }
>  
>  p2m_unlock(hostp2m);

Re: [PATCH 3/7] tools/xl: Add max_altp2m parameter

[Jan Beulich]

On 24.04.2024 22:42, Petr Beneš wrote:
> Introduce a new max_altp2m parameter to control the maximum number of altp2m
> views a domain can use. By default, if max_altp2m is unspecified and altp2m is
> enabled, the value is set to 10, reflecting the legacy behavior.

Apart from the public header you don't even touch hypervisor code here. What
you say above therefore is limited in scope to the tool stack. I question
though that adding a new field without respecting it constitutes an overall
consistent change. In particular I'm curious how you mean to deal with this
for other than x86, where altp2m as a concept doesn't exist (yet).

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -77,6 +77,7 @@ struct xen_domctl_createdomain {
>   */
>  uint32_t max_vcpus;
>  uint32_t max_evtchn_port;
> +uint32_t max_altp2m;
>  int32_t max_grant_frames;
>  int32_t max_maptrack_frames;

Such a struct layout change needs to be accompanied by an interface version
bump (which hasn't happened so far in this release cycle, afaics).

Jan

Re: [PATCH] arm/vpci: make prefetchable mem 64 bit

[Michal Orzel]

Hi Stewart,

On 24/04/2024 18:27, Stewart Hildebrand wrote:
> The vPCI prefetchable memory range is >= 4GB, so the memory space flags
> should be set to 64-bit. See IEEE Std 1275-1994 [1] for a definition of
NIT: would be beneficial to provide chapter (in this case 2.2.1.1)

> the field.
> 
> [1] https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf
> 
> Signed-off-by: Stewart Hildebrand 
Acked-by: Michal Orzel 

~Michal

> ---
>  xen/include/public/arch-arm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index e167e14f8df9..289af81bd69d 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -487,7 +487,7 @@ typedef uint64_t xen_callback_t;
>  #define GUEST_RAM0_SIZE   xen_mk_ullong(0xc000)
>  
>  /* 4GB @ 4GB Prefetch Memory for VPCI */
> -#define GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM   xen_mk_ullong(0x4200)
> +#define GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM   xen_mk_ullong(0x4300)
>  #define GUEST_VPCI_PREFETCH_MEM_ADDRxen_mk_ullong(0x1)
>  #define GUEST_VPCI_PREFETCH_MEM_SIZExen_mk_ullong(0x1)
>  
> 
> base-commit: 410ef3343924b5a3928bbe8e392491992b322cf0

Re: [PATCH v3] xen/x86/pvh: handle ACPI RSDT table in PVH Dom0 build

[Jan Beulich]

On 24.04.2024 21:18, Daniel P. Smith wrote:
> From: Stefano Stabellini 
> 
> Xen always generates as XSDT table even if the firmware provided an RSDT 
> table.
> Copy the RSDT header from the firmware table, adjusting the signature, for the
> XSDT table when not provided by the firmware.
> 
> Fixes: 1d74282c455f ('x86: setup PVHv2 Dom0 ACPI tables')

Especially with this tag the description really wants to say what the problem
is that is being solved here. XSDT having superseded RSDT for ages, it seems
unlikely that there might be systems around that are new enough to run PVH
Dom0, yet come without an XSDT. I can only guess ...

> Suggested-by: Roger Pau Monné 
> Signed-off-by: Stefano Stabellini 
> Signed-off-by: Daniel P. Smith 
> ---
> 
> This patch is used for development and testing of hyperlaunch using the QEMU
> emulator.

... that for whatever reason qemu doesn't surface an XSDT (at which point a
follow-on question would be why this wouldn't want dealing with in qemu
instead).

> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -1077,7 +1077,16 @@ static int __init pvh_setup_acpi_xsdt(struct domain 
> *d, paddr_t madt_addr,
>  rc = -EINVAL;
>  goto out;
>  }
> -xsdt_paddr = rsdp->xsdt_physical_address;
> +/*
> + * Note the header is the same for both RSDT and XSDT, so it's fine to
> + * copy the native RSDT header to the Xen crafted XSDT if no native
> + * XSDT is available.
> + */
> +if ( rsdp->revision > 1 && rsdp->xsdt_physical_address )
> +xsdt_paddr = rsdp->xsdt_physical_address;
> +else
> +xsdt_paddr = rsdp->rsdt_physical_address;
> +
>  acpi_os_unmap_memory(rsdp, sizeof(*rsdp));
>  table = acpi_os_map_memory(xsdt_paddr, sizeof(*table));
>  if ( !table )
> @@ -1089,6 +1098,9 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, 
> paddr_t madt_addr,
>  xsdt->header = *table;
>  acpi_os_unmap_memory(table, sizeof(*table));
>  
> +/* In case the header is an RSDT copy, blindly ensure it has an XSDT sig 
> */
> +xsdt->header.signature[0] = 'X';

This is in no way "blindly". The size of the table elements being different
between RSDT and XSDT makes it mandatory to have the correct signature. Else
the consumer of the struct is going to be misled.

Jan