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

[Nicola Vetrini]

On 2024-02-19 16:14, Nicola Vetrini wrote:

The cache clearing and invalidation helpers in x86 and Arm didn't
comply with MISRA C Rule 17.7: "The value returned by a function
having non-void return type shall be used". On Arm they
were always returning 0, while some in x86 returned -EOPNOTSUPP
and in common/grant_table the return value is saved.

As a consequence, a common helper arch_grant_cache_flush that returns
an integer is introduced, so that each architecture can choose whether 
to
return an error value on certain conditions, and the helpers have 
either

been changed to return void (on Arm) or deleted entirely (on x86).

Signed-off-by: Julien Grall 
Signed-off-by: Nicola Vetrini 
---
The original refactor idea came from Julien Grall in [1]; I edited that 
proposal

to fix build errors.

I did introduce a cast to void for the call to flush_area_local on x86, 
because
even before this patch the return value of that function wasn't checked 
in all
but one use in x86/smp.c, and in this context the helper (perhaps 
incidentally)

ignored the return value of flush_area_local.

[1] 
https://lore.kernel.org/xen-devel/09589e8f-77b6-47f7-b5bd-cf485e4b6...@xen.org/

---
 xen/arch/arm/include/asm/page.h | 33 ++---
 xen/arch/x86/include/asm/flushtlb.h | 23 ++--
 xen/common/grant_table.c|  9 +---
 3 files changed, 34 insertions(+), 31 deletions(-)



I'll put this patch in the backlog at the moment: too many intricacies 
while trying to untangle xen/flushtlb from xen/mm.h, and there are 
easier cases that can be done faster. If someone is interested I can 
post the partial work I've done so far, even though it doesn't

build on x86.

diff --git a/xen/arch/arm/include/asm/page.h 
b/xen/arch/arm/include/asm/page.h

index 69f817d1e68a..e90c9de3616e 100644
--- a/xen/arch/arm/include/asm/page.h
+++ b/xen/arch/arm/include/asm/page.h
@@ -123,6 +123,7 @@

 #ifndef __ASSEMBLY__

+#include 
 #include 
 #include 
 #include 
@@ -159,13 +160,13 @@ static inline size_t read_dcache_line_bytes(void)
  * if 'range' is large enough we might want to use model-specific
  * full-cache flushes. */

-static inline int invalidate_dcache_va_range(const void *p, unsigned 
long size)
+static inline void invalidate_dcache_va_range(const void *p, unsigned 
long size)

 {
 size_t cacheline_mask = dcache_line_bytes - 1;
 unsigned long idx = 0;

 if ( !size )
-return 0;
+return;

 /* Passing a region that wraps around is illegal */
 ASSERT(((uintptr_t)p + size - 1) >= (uintptr_t)p);
@@ -188,17 +189,15 @@ static inline int 
invalidate_dcache_va_range(const void *p, unsigned long size)
 asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p 
+ idx));


 dsb(sy);   /* So we know the flushes happen before 
continuing */

-
-return 0;
 }

-static inline int clean_dcache_va_range(const void *p, unsigned long 
size)
+static inline void clean_dcache_va_range(const void *p, unsigned long 
size)

 {
 size_t cacheline_mask = dcache_line_bytes - 1;
 unsigned long idx = 0;

 if ( !size )
-return 0;
+return;

 /* Passing a region that wraps around is illegal */
 ASSERT(((uintptr_t)p + size - 1) >= (uintptr_t)p);
@@ -211,18 +210,16 @@ static inline int clean_dcache_va_range(const 
void *p, unsigned long size)

 idx += dcache_line_bytes, size -= dcache_line_bytes )
 asm volatile (__clean_dcache_one(0) : : "r" (p + idx));
 dsb(sy);   /* So we know the flushes happen before 
continuing */

-/* ARM callers assume that dcache_* functions cannot fail. */
-return 0;
 }

-static inline int clean_and_invalidate_dcache_va_range
+static inline void clean_and_invalidate_dcache_va_range
 (const void *p, unsigned long size)
 {
 size_t cacheline_mask = dcache_line_bytes - 1;
 unsigned long idx = 0;

 if ( !size )
-return 0;
+return;

 /* Passing a region that wraps around is illegal */
 ASSERT(((uintptr_t)p + size - 1) >= (uintptr_t)p);
@@ -235,8 +232,6 @@ static inline int 
clean_and_invalidate_dcache_va_range

 idx += dcache_line_bytes, size -= dcache_line_bytes )
 asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p 
+ idx));
 dsb(sy); /* So we know the flushes happen before 
continuing */

-/* ARM callers assume that dcache_* functions cannot fail. */
-return 0;
 }

 /* Macros for flushing a single small item.  The predicate is always
@@ -266,6 +261,20 @@ static inline int 
clean_and_invalidate_dcache_va_range
 : : "r" (_p), "m" (*_p));  
 \

 } while (0)

+static inline int arch_grant_cache_flush(unsigned int op, const void 
*p,

+ unsigned long size)
+{
+if ( (op & GNTTAB_CACHE_INVAL) && (op & GNTTAB_CACHE_CLEAN) )
+clean_and_invalidate_dcache_va_range(p, size);
+else if ( op & 

Re: [PATCH 1/2] x86/memsharing: use an atomic add instead of a cmpxchg loop

[Jan Beulich]

On 22.02.2024 19:03, Tamas K Lengyel wrote:
> On Thu, Feb 22, 2024 at 5:06 AM Jan Beulich  wrote:
>> On 22.02.2024 10:05, Roger Pau Monne wrote:
>>> The usage of a cmpxchg loop in get_next_handle() is unnecessary, as the same
>>> can be achieved with an atomic increment, which is both simpler to read, and
>>> avoid any need for a loop.
>>>
>>> The cmpxchg usage is likely a remnant of 32bit support, which didn't have an
>>> instruction to do an atomic 64bit add, and instead a cmpxchg had to be used.
>>>
>>> Suggested-by: Andrew Cooper 
>>> Signed-of-by: Roger Pau Monné 
>>
>> Reviewed-by: Jan Beulich 
>> albeit ...
>>
>>> --- a/xen/arch/x86/mm/mem_sharing.c
>>> +++ b/xen/arch/x86/mm/mem_sharing.c
>>> @@ -179,13 +179,7 @@ static void mem_sharing_page_unlock(struct page_info 
>>> *pg)
>>>
>>>  static shr_handle_t get_next_handle(void)
>>>  {
>>> -/* Get the next handle get_page style */
>>> -uint64_t x, y = next_handle;
>>> -do {
>>> -x = y;
>>> -}
>>> -while ( (y = cmpxchg(_handle, x, x + 1)) != x );
>>> -return x + 1;
>>> +return arch_fetch_and_add(_handle, 1) + 1;
>>>  }
>>
>> ... the adding of 1 here is a little odd when taken together with
>> next_handle's initializer. Tamas, you've not written that code, but do
>> you have any thoughts towards the possible removal of either the
>> initializer or the adding here? Plus that variable of course could
>> very well do with moving into this function.
> 
> I have to say I find the existing logic here hard to parse but by the
> looks I don't think we need the + 1 once we switch to
> arch_fetch_and_add. Also could go without initializing next_handle to
> 1. Moving it into the function would not really accomplish anything
> other than style AFAICT?

Well, limiting scope of things can be viewed as purely style, but I
think it's more than that: It makes intentions more clear and reduces
the chance of abuse (deliberate or unintentional).

Jan

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

[Jan Beulich]

On 23.02.2024 02:32, Stefano Stabellini wrote:
> On Tue, 24 Oct 2023, Jan Beulich wrote:
>> On 24.10.2023 00:05, Stefano Stabellini wrote:
>>> On Mon, 23 Oct 2023, Jan Beulich wrote:
 On 23.10.2023 17:23, Simone Ballarin wrote:
> On 23/10/23 15:34, Jan Beulich wrote:
>> On 18.10.2023 16:18, Simone Ballarin wrote:
>>> --- a/xen/include/xen/pdx.h
>>> +++ b/xen/include/xen/pdx.h
>>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned 
>>> long pfn)
>>>* @param pdx Page index
>>>* @return Obtained pfn after decompressing the pdx
>>>*/
>>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
>>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned 
>>> long pdx)
>>>   {
>>>   return (pdx & pfn_pdx_bottom_mask) |
>>>  ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
>>
>> Taking this as an example for what I've said above: The compiler can't
>> know that the globals used by the functions won't change value. Even
>> within Xen it is only by convention that these variables are assigned
>> their values during boot, and then aren't changed anymore. Which makes
>> me wonder: Did you check carefully that around the time the variables
>> have their values established, no calls to the functions exist (which
>> might then be subject to folding)?
>
> There is no need to check that, the GCC documentation explicitly says:
>
> However, functions declared with the pure attribute *can safely read any 
> non-volatile objects*, and modify the value of objects in a way that 
> does not affect their return value or the observable state of the program.

 I did quote this same text in response to what Andrew has said, but I also
 did note there that this needs to be taken with a grain of salt: The
 compiler generally assumes a single-threaded environment, i.e. no changes
 to globals behind the back of the code it is processing.
>>>
>>> Let's start from the beginning. The reason for Simone to add
>>> __attribute_pure__ to pdx_to_pfn and other functions is for
>>> documentation purposes. It is OK if it doesn't serve any purpose other
>>> than documentation.
>>>
>>> Andrew, for sure we do not want to lie to the compiler and introduce
>>> undefined behavior. If we think there is a risk of it, we should not do
>>> it.
>>>
>>> So, what do we want to document? We want to document that the function
>>> does not have side effects according to MISRA's definition of it, which
>>> might subtly differ from GCC's definition.
>>>
>>> Looking at GCC's definition of __attribute_pure__, with the
>>> clarification statement copy/pasted above by both Simone and Jan, it
>>> seems that __attribute_pure__ matches MISRA's definition of a function
>>> without side effects. It also seems that pdx_to_pfn abides to that
>>> definition.
>>>
>>> Jan has a point that GCC might be making other assumptions
>>> (single-thread execution) that might not hold true in our case. Given
>>> the way the GCC statement is written I think this is low risk. But maybe
>>> not all GCC versions we want to support in the project might have the
>>> same definition of __attribute_pure__. So we could end up using
>>> __attribute_pure__ correctly for the GCC version used for safety (GCC
>>> 12.1, see docs/misra/C-language-toolchain.rst) but it might actually
>>> break an older GCC version.
>>>
>>>
>>> So Option#1 is to use __attribute_pure__ taking the risk that a GCC or
>>> Clang version might interpret __attribute_pure__ differently and
>>> potentially misbehave.
>>>
>>> Option#2 is to avoid this risk, by not using __attribute_pure__.
>>> Instead, we can use SAF-xx-safe or deviations.rst to document that
>>> pdx_to_pfn and other functions like it are without side effects
>>> according to MISRA's definition.
>>>
>>>
>>> Both options have pros and cons. To me the most important factor is how
>>> many GCC versions come with the statement "pure attribute can safely
>>> read any non-volatile objects, and modify the value of objects in a way
>>> that does not affect their return value or the observable state of the
>>> program".
>>>
>>> I checked and these are the results:
>>> - gcc 4.0.2: no statement
>>> - gcc 5.1.0: no statement
>>> - gcc 6.1.0: no statement
>>> - gcc 7.1.0: no statement
>>> - gcc 8.1.0: alternative statement "The pure attribute imposes similar
>>>   but looser restrictions on a function’s definition than the const
>>>   attribute: it allows the function to read global variables."
>>> - gcc 9.1.0: yes statement
>>>
>>>
>>> So based on the above, __attribute_pure__ comes with its current
>>> definition only from gcc 9 onward. I don't know if as a Xen community we
>>> clearly declare a range of supported compilers, but I would imagine we
>>> would still want to support gcc versions older than 9? (Not to mention
>>> clang, which I haven't checked.)
>>>
>>> It doesn't seem 

Re: [PATCH v6 07/11] virtio-gpu: Handle resource blob commands

[Huang Rui]

On Wed, Jan 10, 2024 at 04:51:31PM +0800, Pierre-Eric Pelloux-Prayer wrote:
> 
> 
> Le 09/01/2024 à 17:50, Pierre-Eric Pelloux-Prayer a écrit :
> > 
> > 
> > Le 19/12/2023 à 08:53, Huang Rui a écrit :
> >> From: Antonio Caggiano 
> >>
> >> Support BLOB resources creation, mapping and unmapping by calling the
> >> new stable virglrenderer 0.10 interface. Only enabled when available and
> >> via the blob config. E.g. -device virtio-vga-gl,blob=true
> >>
> >> Signed-off-by: Antonio Caggiano 
> >> Signed-off-by: Dmitry Osipenko 
> >> Signed-off-by: Xenia Ragiadakou 
> >> Signed-off-by: Huang Rui 
> >> ---
> >>
> >> Changes in v6:
> >> - Use new struct virgl_gpu_resource.
> >> - Unmap, unref and destroy the resource only after the memory region
> >>    has been completely removed.
> >> - In unref check whether the resource is still mapped.
> >> - In unmap_blob check whether the resource has been already unmapped.
> >> - Fix coding style
> >>
> >>   hw/display/virtio-gpu-virgl.c | 274 +-
> >>   hw/display/virtio-gpu.c   |   4 +-
> >>   meson.build   |   4 +
> >>   3 files changed, 276 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> >> index faab374336..5a3a292f79 100644
> >> --- a/hw/display/virtio-gpu-virgl.c
> >> +++ b/hw/display/virtio-gpu-virgl.c
> >> @@ -17,6 +17,7 @@
> >>   #include "trace.h"
> >>   #include "hw/virtio/virtio.h"
> >>   #include "hw/virtio/virtio-gpu.h"
> >> +#include "hw/virtio/virtio-gpu-bswap.h"
> >>   #include "ui/egl-helpers.h"
> >> @@ -24,8 +25,62 @@
> >>   struct virgl_gpu_resource {
> >>   struct virtio_gpu_simple_resource res;
> >> +    uint32_t ref;
> >> +    VirtIOGPU *g;
> >> +
> >> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> >> +    /* only blob resource needs this region to be mapped as guest mmio */
> >> +    MemoryRegion *region;
> >> +#endif
> >>   };
> >> +static void vres_get_ref(struct virgl_gpu_resource *vres)
> >> +{
> >> +    uint32_t ref;
> >> +
> >> +    ref = qatomic_fetch_inc(>ref);
> >> +    g_assert(ref < INT_MAX);
> >> +}
> >> +
> >> +static void virgl_resource_destroy(struct virgl_gpu_resource *vres)
> >> +{
> >> +    struct virtio_gpu_simple_resource *res;
> >> +    VirtIOGPU *g;
> >> +
> >> +    if (!vres) {
> >> +    return;
> >> +    }
> >> +
> >> +    g = vres->g;
> >> +    res = >res;
> >> +    QTAILQ_REMOVE(>reslist, res, next);
> >> +    virtio_gpu_cleanup_mapping(g, res);
> >> +    g_free(vres);
> >> +}
> >> +
> >> +static void virgl_resource_unref(struct virgl_gpu_resource *vres)
> >> +{
> >> +    struct virtio_gpu_simple_resource *res;
> >> +
> >> +    if (!vres) {
> >> +    return;
> >> +    }
> >> +
> >> +    res = >res;
> >> +    virgl_renderer_resource_detach_iov(res->resource_id, NULL, NULL);
> >> +    virgl_renderer_resource_unref(res->resource_id);
> >> +}
> >> +
> >> +static void vres_put_ref(struct virgl_gpu_resource *vres)
> >> +{
> >> +    g_assert(vres->ref > 0);
> >> +
> >> +    if (qatomic_fetch_dec(>ref) == 1) {
> >> +    virgl_resource_unref(vres);
> >> +    virgl_resource_destroy(vres);
> >> +    }
> >> +}
> >> +
> >>   static struct virgl_gpu_resource *
> >>   virgl_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
> >>   {
> >> @@ -59,6 +114,8 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
> >>  c2d.width, c2d.height);
> >>   vres = g_new0(struct virgl_gpu_resource, 1);
> >> +    vres_get_ref(vres);
> >> +    vres->g = g;
> >>   vres->res.width = c2d.width;
> >>   vres->res.height = c2d.height;
> >>   vres->res.format = c2d.format;
> >> @@ -91,6 +148,8 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
> >>  c3d.width, c3d.height, c3d.depth);
> >>   vres = g_new0(struct virgl_gpu_resource, 1);
> >> +    vres_get_ref(vres);
> >> +    vres->g = g;
> >>   vres->res.width = c3d.width;
> >>   vres->res.height = c3d.height;
> >>   vres->res.format = c3d.format;
> >> @@ -126,12 +185,21 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
> >>   return;
> >>   }
> >> -    virgl_renderer_resource_detach_iov(unref.resource_id, NULL, NULL);
> >> -    virgl_renderer_resource_unref(unref.resource_id);
> >> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> >> +    if (vres->region) {
> >> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> >> +    MemoryRegion *mr = vres->region;
> >> +
> >> +    warn_report("%s: blob resource %d not unmapped",
> >> +    __func__, unref.resource_id);
> >> +    vres->region = NULL;
> > 
> > Shouldn't there be a call to memory_region_unref(mr)?
> > 
> >> +    memory_region_set_enabled(mr, false);
> >> +    memory_region_del_subregion(>hostmem, mr);
> >> +    object_unparent(OBJECT(mr));
> >> +    }
> >> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
> >> -    QTAILQ_REMOVE(>reslist, >res, next);
> >> -    

Re: [RFC KERNEL PATCH v4 2/3] xen/pvh: Setup gsi for passthrough device

[Chen, Jiqian]

On 2024/2/23 08:23, Stefano Stabellini wrote:
> On Fri, 5 Jan 2024, Jiqian Chen wrote:
>> In PVH dom0, the gsis don't get registered, but the gsi of
>> a passthrough device must be configured for it to be able to be
>> mapped into a domU.
>>
>> When assign a device to passthrough, proactively setup the gsi
>> of the device during that process.
>>
>> Co-developed-by: Huang Rui 
>> Signed-off-by: Jiqian Chen 
>> ---
>>  arch/x86/xen/enlighten_pvh.c   | 90 ++
>>  drivers/acpi/pci_irq.c |  2 +-
>>  drivers/xen/xen-pciback/pci_stub.c |  8 +++
>>  include/linux/acpi.h   |  1 +
>>  include/xen/acpi.h |  6 ++
>>  5 files changed, 106 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
>> index ada3868c02c2..ecadd966c684 100644
>> --- a/arch/x86/xen/enlighten_pvh.c
>> +++ b/arch/x86/xen/enlighten_pvh.c
>> @@ -1,6 +1,7 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>  #include 
>>  #include 
>> +#include 
>>  
>>  #include 
>>  
>> @@ -25,6 +26,95 @@
>>  bool __ro_after_init xen_pvh;
>>  EXPORT_SYMBOL_GPL(xen_pvh);
>>  
>> +typedef struct gsi_info {
>> +int gsi;
>> +int trigger;
>> +int polarity;
>> +} gsi_info_t;
>> +
>> +struct acpi_prt_entry {
>> +struct acpi_pci_id  id;
>> +u8  pin;
>> +acpi_handle link;
>> +u32 index;  /* GSI, or link _CRS index */
>> +};
>> +
>> +static int xen_pvh_get_gsi_info(struct pci_dev *dev,
>> +gsi_info_t 
>> *gsi_info)
>> +{
>> +int gsi;
>> +u8 pin = 0;
>> +struct acpi_prt_entry *entry;
>> +int trigger = ACPI_LEVEL_SENSITIVE;
>> +int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ?
>> +  ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW;
>> +
>> +if (dev)
>> +pin = dev->pin;
> 
> This is minor, but you can just move the pin = dev->pin after the !dev
> check below.
Will change in next version.

> 
> With that change, and assuming the Xen-side and QEMU-side patches are
> accepted:
> 
> Reviewed-by: Stefano Stabellini 
Thank you very much!

> 
> 
> 
> 
>> +if (!dev || !pin || !gsi_info)
>> +return -EINVAL;
>> +
>> +entry = acpi_pci_irq_lookup(dev, pin);
>> +if (entry) {
>> +if (entry->link)
>> +gsi = acpi_pci_link_allocate_irq(entry->link,
>> + entry->index,
>> + , ,
>> + NULL);
>> +else
>> +gsi = entry->index;
>> +} else
>> +gsi = -1;
>> +
>> +if (gsi < 0)
>> +return -EINVAL;
>> +
>> +gsi_info->gsi = gsi;
>> +gsi_info->trigger = trigger;
>> +gsi_info->polarity = polarity;
>> +
>> +return 0;
>> +}
>> +
>> +static int xen_pvh_setup_gsi(gsi_info_t *gsi_info)
>> +{
>> +struct physdev_setup_gsi setup_gsi;
>> +
>> +if (!gsi_info)
>> +return -EINVAL;
>> +
>> +setup_gsi.gsi = gsi_info->gsi;
>> +setup_gsi.triggering = (gsi_info->trigger == ACPI_EDGE_SENSITIVE ? 0 : 
>> 1);
>> +setup_gsi.polarity = (gsi_info->polarity == ACPI_ACTIVE_HIGH ? 0 : 1);
>> +
>> +return HYPERVISOR_physdev_op(PHYSDEVOP_setup_gsi, _gsi);
>> +}
>> +
>> +int xen_pvh_passthrough_gsi(struct pci_dev *dev)
>> +{
>> +int ret;
>> +gsi_info_t gsi_info;
>> +
>> +if (!dev)
>> +return -EINVAL;
>> +
>> +ret = xen_pvh_get_gsi_info(dev, _info);
>> +if (ret) {
>> +xen_raw_printk("Fail to get gsi info!\n");
>> +return ret;
>> +}
>> +
>> +ret = xen_pvh_setup_gsi(_info);
>> +if (ret == -EEXIST) {
>> +xen_raw_printk("Already setup the GSI :%d\n", gsi_info.gsi);
>> +ret = 0;
>> +} else if (ret)
>> +xen_raw_printk("Fail to setup GSI (%d)!\n", gsi_info.gsi);
>> +
>> +return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(xen_pvh_passthrough_gsi);
>> +
>>  void __init xen_pvh_init(struct boot_params *boot_params)
>>  {
>>  u32 msr;
>> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
>> index ff30ceca2203..630fe0a34bc6 100644
>> --- a/drivers/acpi/pci_irq.c
>> +++ b/drivers/acpi/pci_irq.c
>> @@ -288,7 +288,7 @@ static int acpi_reroute_boot_interrupt(struct pci_dev 
>> *dev,
>>  }
>>  #endif /* CONFIG_X86_IO_APIC */
>>  
>> -static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int 
>> pin)
>> +struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin)
>>  {
>>  struct acpi_prt_entry *entry = NULL;
>>  struct pci_dev *bridge;
>> diff --git a/drivers/xen/xen-pciback/pci_stub.c 
>> b/drivers/xen/xen-pciback/pci_stub.c
>> index 46c40ec8a18e..22d4380d2b04 100644
>> --- a/drivers/xen/xen-pciback/pci_stub.c
>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>> @@ 

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

[Chen, Jiqian]

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

> 
> 
>> +{
>> +rcu_unlock_domain(d);
>> +return -EOPNOTSUPP;
>> +}
>> +rcu_unlock_domain(d);
>> +
>>  switch ( map.type )
>>  {
>>  case MAP_PIRQ_TYPE_MSI_SEG:
>> @@ -341,11 +352,22 @@ ret_t do_physdev_op(int cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>  
>>  case PHYSDEVOP_unmap_pirq: {
>>  struct physdev_unmap_pirq unmap;
>> +struct domain *d;
>>  
>>  ret = -EFAULT;
>>  if ( copy_from_guest(, arg, 1) != 0 )
>>  break;
>>  
>> +d = rcu_lock_domain_by_any_id(unmap.domid);
>> +if ( d == NULL )
>> +return -ESRCH;
>> +if ( !is_pv_domain(d) && !has_pirq(d) )
> 
> same here
> 
> 
> Other than that, everything looks fine to me
> 
> 
>> +{
>> +rcu_unlock_domain(d);
>> +return -EOPNOTSUPP;
>> +}
>> +rcu_unlock_domain(d);
>> +
>>  ret = physdev_unmap_pirq(unmap.domid, unmap.pirq);
>>  break;
>>  }
>> -- 
>> 2.34.1
>>

-- 
Best regards,
Jiqian Chen.

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 xen  9ee7dc877b8754ce2fc82500feea52c04d4e6409
baseline version:
 xen  f8791d0fd3adbda3701e7eb9db63a9351b478365

Last test of basis   184723  2024-02-22 01:52:14 Z1 days
Testing same since   184729  2024-02-22 14:07:38 Z0 days1 attempts


People 

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

[Stefano Stabellini]

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

Re: [XEN PATCH v2] automation/eclair_analysis: deviate certain macros for Rule 20.12

[Stefano Stabellini]

On Thu, 15 Feb 2024, Nicola Vetrini wrote:
> Certain macros are allowed to violate the Rule, since their meaning and
> intended use is well-known to all Xen developers.
> 
> Variadic macros that rely on the GCC extension for removing a trailing
> comma when token pasting the variable argument are similarly
> well-understood and therefore allowed.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini 

Reviewed-by: Stefano Stabellini 



> ---
> Changes in v2:
> - Restrict deviation for GENERATE_CASE to vcpreg.c.
> - Improve deviation justifications.
> ---
>  .../eclair_analysis/ECLAIR/deviations.ecl | 20 +
>  docs/misra/deviations.rst | 22 +++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
> b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index fd32ff8a9cae..04cb41e16a50 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -387,6 +387,26 @@ in assignments."
>  {safe, "left_right(^[(,\\[]$,^[),\\]]$)"}
>  -doc_end
>  
> +-doc_begin="Uses of variadic macros that have one of their arguments defined 
> as
> +a macro and used within the body for both ordinary parameter expansion and 
> as an
> +operand to the # or ## operators have a behavior that is well-understood and
> +deliberate."
> +-config=MC3R1.R20.12,macros+={deliberate, "variadic()"}
> +-doc_end
> +
> +-doc_begin="Uses of a macro parameter for ordinary expansion and as an 
> operand
> +to the # or ## operators within the following macros are deliberate, to 
> provide
> +useful diagnostic messages to the user."
> +-config=MC3R1.R20.12,macros+={deliberate, 
> "name(ASSERT||BUILD_BUG_ON||BUILD_BUG_ON_ZERO)"}
> +-doc_end
> +
> +-doc_begin="The helper macro GENERATE_CASE may use a macro parameter for 
> ordinary
> +expansion and token pasting to improve readability. Only instances where this
> +leads to a violation of the Rule are deviated."
> +-file_tag+={deliberate_generate_case, "^xen/arch/arm/vcpreg\\.c$"}
> +-config=MC3R1.R20.12,macros+={deliberate, 
> "name(GENERATE_CASE)&(file(deliberate_generate_case))"}
> +-doc_end
> +
>  #
>  # General
>  #
> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
> index 123c78e20a01..84da637ef888 100644
> --- a/docs/misra/deviations.rst
> +++ b/docs/misra/deviations.rst
> @@ -330,6 +330,28 @@ Deviations related to MISRA C:2012 Rules:
> (4) as lhs in assignments.
>   - Tagged as `safe` for ECLAIR.
>  
> +   * - R20.12
> + - Variadic macros that use token pasting often employ the gcc extension
> +   `ext_paste_comma`, as detailed in `C-language-toolchain.rst`, which is
> +   not easily replaceable; macros that in addition perform regular 
> argument
> +   expansion on the same argument subject to the # or ## operators 
> violate
> +   the Rule if the argument is a macro. 
> + - Tagged as `deliberate` for ECLAIR.
> +
> +   * - R20.12
> + - Macros that are used for runtime or build-time assertions contain
> +   deliberate uses of an argument as both a regular argument and a
> +   stringification token, to provide useful diagnostic messages.
> + - Tagged as `deliberate` for ECLAIR.
> +
> +   * - R20.12
> + - GENERATE_CASE is a local helper macro that allows some selected switch
> +   statements to be more compact and readable. As such, the risk of
> +   developer confusion in using such macro is deemed negligible. This
> +   construct is deviated only in Translation Units that present a 
> violation
> +   of the Rule due to uses of this macro.
> + - Tagged as `deliberate` for ECLAIR.
> +
>  Other deviations:
>  -
>  
> -- 
> 2.34.1
> 

Re: [PATCH RFC] x86/xen: attempt to inflate the memory balloon on PVH

[Stefano Stabellini]

On Tue, 20 Feb 2024, Roger Pau Monne wrote:
> When running as PVH or HVM Linux will use holes in the memory map as scratch
> space to map grants, foreign domain pages and possibly miscellaneous other
> stuff.  However the usage of such memory map holes for Xen purposes can be
> problematic.  The request of holesby Xen happen quite early in the kernel boot
> process (grant table setup already uses scratch map space), and it's possible
> that by then not all devices have reclaimed their MMIO space.  It's not
> unlikely for chunks of Xen scratch map space to end up using PCI bridge MMIO
> window memory, which (as expected) causes quite a lot of issues in the system.

Am I understanding correctly that XEN_BALLOON_MEMORY_HOTPLUG doesn't
help because it becomes available too late in the PVH boot sequence? 



> At least for PVH dom0 we have the possibility of using regions marked as
> UNUSABLE in the e820 memory map.  Either if the region is UNUSABLE in the
> native memory map, or it has been converted into UNUSABLE in order to hide RAM
> regions from dom0, the second stage translation page-tables can populate those
> areas without issues.
> 
> PV already has this kind of logic, where the balloon driver is inflated at
> boot.  Re-use the current logic in order to also inflate it when running as
> PVH.  onvert UNUSABLE regions up to the ratio specified in EXTRA_MEM_RATIO to
> RAM, while reserving them using xen_add_extra_mem() (which is also moved so
> it's no longer tied to CONFIG_PV).
> 
> Signed-off-by: Roger Pau Monné 
> ---
> RFC reasons:
> 
>  * Note that it would be preferred for the hypervisor to provide an explicit
>range to be used as scratch mapping space, but that requires changes to 
> Xen,
>and it's not fully clear whether Xen can figure out the position of all 
> MMIO
>regions at boot in order to suggest a scratch mapping region for dom0.
> 
>  * Should the whole set of xen_{add,del,chk,inv}_extra_mem() functions be 
> moved
>to a different file?  For the purposes of PVH only xen_add_extra_mem() is
>moved and the chk and inv ones are PV specific and might not want moving to
>a separate file just to guard them with CONFIG_PV.
> ---
>  arch/x86/include/asm/xen/hypervisor.h |  1 +
>  arch/x86/platform/pvh/enlighten.c |  3 ++
>  arch/x86/xen/enlighten.c  | 32 +
>  arch/x86/xen/enlighten_pvh.c  | 68 +++
>  arch/x86/xen/setup.c  | 44 -
>  arch/x86/xen/xen-ops.h| 14 ++
>  drivers/xen/balloon.c |  2 -
>  7 files changed, 118 insertions(+), 46 deletions(-)
> 
> diff --git a/arch/x86/include/asm/xen/hypervisor.h 
> b/arch/x86/include/asm/xen/hypervisor.h
> index a9088250770f..31e2bf8d5db7 100644
> --- a/arch/x86/include/asm/xen/hypervisor.h
> +++ b/arch/x86/include/asm/xen/hypervisor.h
> @@ -62,6 +62,7 @@ void xen_arch_unregister_cpu(int num);
>  #ifdef CONFIG_PVH
>  void __init xen_pvh_init(struct boot_params *boot_params);
>  void __init mem_map_via_hcall(struct boot_params *boot_params_p);
> +void __init xen_reserve_extra_memory(struct boot_params *bootp);
>  #endif
>  
>  /* Lazy mode for batching updates / context switch */
> diff --git a/arch/x86/platform/pvh/enlighten.c 
> b/arch/x86/platform/pvh/enlighten.c
> index 00a92cb2c814..a12117f3d4de 100644
> --- a/arch/x86/platform/pvh/enlighten.c
> +++ b/arch/x86/platform/pvh/enlighten.c
> @@ -74,6 +74,9 @@ static void __init init_pvh_bootparams(bool xen_guest)
>   } else
>   xen_raw_printk("Warning: Can fit ISA range into e820\n");
>  
> + if (xen_guest)
> + xen_reserve_extra_memory(_bootparams);
> +
>   pvh_bootparams.hdr.cmd_line_ptr =
>   pvh_start_info.cmdline_paddr;
>  
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index 3c61bb98c10e..a01ca255b0c6 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -6,6 +6,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -350,3 +351,34 @@ void xen_arch_unregister_cpu(int num)
>  }
>  EXPORT_SYMBOL(xen_arch_unregister_cpu);
>  #endif
> +
> +/* Amount of extra memory space we add to the e820 ranges */
> +struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] __initdata;
> +
> +void __init xen_add_extra_mem(unsigned long start_pfn, unsigned long n_pfns)
> +{
> + unsigned int i;
> +
> + /*
> +  * No need to check for zero size, should happen rarely and will only
> +  * write a new entry regarded to be unused due to zero size.
> +  */
> + for (i = 0; i < XEN_EXTRA_MEM_MAX_REGIONS; i++) {
> + /* Add new region. */
> + if (xen_extra_mem[i].n_pfns == 0) {
> + xen_extra_mem[i].start_pfn = start_pfn;
> + xen_extra_mem[i].n_pfns = n_pfns;
> + break;
> + }
> + /* Append to existing region. */
> +   

Re: [XEN PATCH] automation: Rework "build-each-commit-gcc" test

[Stefano Stabellini]

On Tue, 20 Feb 2024, Anthony PERARD wrote:

> Current issues with this test are:
> - when the job timeout, the log file is lost as there is no chance to
>   run the `mv` command.
> - GitLab job log is limited in size, so one usually have to download
>   the artifacts, which may be missing.
> 
> Use $GITLAB_CI to detect when the script is run as part of a GitLab
> pipeline. GitLab will add "GITLAB_CI=true" in the environment
> variables.
> 
> When run as part of $GITLAB_CI, ignore "dirty" worktree to allow to
> write "build-each-commit-gcc.log", which can then be grabbed as
> artifacts, even when the job timeout. The `git clean` command is
> changed to keep those build logs.
> 
> When run as part of $GITLAB_CI, we will also store the build output in
> a log file instead of writing it to stdout, because GitLab's job log
> is limited. But we will write the log to stdout in case of error, so
> we can find out more quickly why there's been an error.
> 
> This patch also make use of a GitLab feature, "log sections", which we
> will collapse by default. One section per commit been built.
> 
> There's a bit of colour added to the logs.
> 
> Signed-off-by: Anthony PERARD 

Acked-by: Stefano Stabellini 


> ---
> 
> Notes:
> Examples:
> - on success:
>   https://gitlab.com/xen-project/people/anthonyper/xen/-/jobs/6212972041
> - on failure:
>   https://gitlab.com/xen-project/people/anthonyper/xen/-/jobs/6212993231
> 
>  automation/gitlab-ci/build-each-commit.sh |  2 +-
>  automation/gitlab-ci/test.yaml|  4 +-
>  automation/scripts/build-test.sh  | 55 ---
>  3 files changed, 50 insertions(+), 11 deletions(-)
> 
> diff --git a/automation/gitlab-ci/build-each-commit.sh 
> b/automation/gitlab-ci/build-each-commit.sh
> index 19e337b468..08fddecbbe 100755
> --- a/automation/gitlab-ci/build-each-commit.sh
> +++ b/automation/gitlab-ci/build-each-commit.sh
> @@ -15,4 +15,4 @@ fi
>  echo "Building ${BASE}..${TIP}"
>  
>  NON_SYMBOLIC_REF=1 ./automation/scripts/build-test.sh ${BASE} ${TIP} \
> -bash -c "git clean -ffdx && ./automation/scripts/build"
> +bash -c "git clean -ffdx -e '/build-*.log' && ./automation/scripts/build"
> diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
> index 3b27cc9f41..50056c1372 100644
> --- a/automation/gitlab-ci/test.yaml
> +++ b/automation/gitlab-ci/test.yaml
> @@ -136,9 +136,7 @@ build-each-commit-gcc:
>  XEN_TARGET_ARCH: x86_64
>  CC: gcc
>script:
> -- BASE=${BASE_SHA:-${CI_COMMIT_BEFORE_SHA}} 
> TIP=${TIP_SHA:-${CI_COMMIT_SHA}} ./automation/gitlab-ci/build-each-commit.sh 
> 2>&1 | tee ../build-each-commit-gcc.log
> -  after_script:
> -- mv ../build-each-commit-gcc.log .
> +- BASE=${BASE_SHA:-${CI_COMMIT_BEFORE_SHA}} 
> TIP=${TIP_SHA:-${CI_COMMIT_SHA}} ./automation/gitlab-ci/build-each-commit.sh 
> 2>&1 | tee build-each-commit-gcc.log
>artifacts:
>  paths:
>- '*.log'
> diff --git a/automation/scripts/build-test.sh 
> b/automation/scripts/build-test.sh
> index da643adc01..355d4bf7f6 100755
> --- a/automation/scripts/build-test.sh
> +++ b/automation/scripts/build-test.sh
> @@ -9,6 +9,37 @@
>  # Set NON_SYMBOLIC_REF=1 if you want to use this script in detached HEAD 
> state.
>  # This is currently used by automated test system.
>  
> +# Colors with ANSI escape sequences
> +txt_info=''
> +txt_err=''
> +txt_clr=''
> +
> +# $GITLAB_CI should be "true" or "false".
> +if [ "$GITLAB_CI" != true ]; then
> +GITLAB_CI=false
> +fi
> +
> +gitlab_log_section() {
> +if $GITLAB_CI; then
> +echo -n "section_$1:$(date +%s):$2"
> +fi
> +if [ $# -ge 3 ]; then
> +echo "$3"
> +fi
> +}
> +log_section_last=
> +log_section_start() {
> +log_section_last="${1%\[collapsed=true\]}"
> +gitlab_log_section 'start' "$1" "${txt_info}$2${txt_clr}"
> +}
> +log_section_end() {
> +if [ "$log_section_last" ]; then
> +gitlab_log_section 'end' "$log_section_last"
> +log_section_last=
> +fi
> +}
> +
> +
>  if test $# -lt 2 ; then
>  echo "Usage:"
>  echo " $0   [CMD]"
> @@ -19,10 +50,12 @@ fi
>  
>  pushd `git rev-parse --show-toplevel`
>  
> -status=`git status -s`
> -if test -n "$status"; then
> -echo "Tree is dirty, aborted"
> -exit 1
> +if ! $GITLAB_CI; then
> +status=`git status -s`
> +if test -n "$status"; then
> +echo "Tree is dirty, aborted"
> +exit 1
> +fi
>  fi
>  
>  BASE=$1; shift
> @@ -40,26 +73,34 @@ fi
>  
>  ret=1
>  while read num rev; do
> -echo "Testing $num $rev"
> +log_section_start "commit_$rev[collapsed=true]" "Testing #$num $(git log 
> -1 --abbrev=12 --format=tformat:'%h ("%s")' $rev)"
>  
>  git checkout $rev
>  ret=$?
>  if test $ret -ne 0; then
> -echo "Failed to checkout $num $rev with $ret"
> +log_section_end
> +echo "${txt_err}Failed to checkout $num $rev with $ret${txt_clr}"
>  break
>  

Re: [RFC XEN PATCH v5 3/5] x86/pvh: Add PHYSDEVOP_setup_gsi for PVH dom0

[Stefano Stabellini]

On Fri, 12 Jan 2024, Jiqian Chen wrote:
> On PVH dom0, the gsis don't get registered, but
> the gsi of a passthrough device must be configured for it to
> be able to be mapped into a hvm domU.
> On Linux kernel side, it calles PHYSDEVOP_setup_gsi for
> passthrough devices to register gsi when dom0 is PVH.
> So, add PHYSDEVOP_setup_gsi for above purpose.
> 
> Co-developed-by: Huang Rui 
> Signed-off-by: Jiqian Chen 
> ---
>  xen/arch/x86/hvm/hypercall.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> index 493998b42ec5..46f51ee459f6 100644
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -76,6 +76,12 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> arg)
>  case PHYSDEVOP_unmap_pirq:
>  break;
>  
> +case PHYSDEVOP_setup_gsi:
> +if ( !is_hardware_domain(currd) )
> +return -EOPNOTSUPP;
> +ASSERT(!has_pirq(currd));

Do we really need this assert? I understand that the use case right now
is for !has_pirq(currd) but in general it doesn't seem to me that
PHYSDEVOP_setup_gsi and !has_pirq should be tied together.

Aside from that, it looks fine.


> +break;
> +
>  case PHYSDEVOP_eoi:
>  case PHYSDEVOP_irq_status_query:
>  case PHYSDEVOP_get_free_pirq:
> -- 
> 2.34.1
> 

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

[Stefano Stabellini]

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

I think this could just be:

if ( !has_pirq(d) )

Right?


> +{
> +rcu_unlock_domain(d);
> +return -EOPNOTSUPP;
> +}
> +rcu_unlock_domain(d);
> +
>  switch ( map.type )
>  {
>  case MAP_PIRQ_TYPE_MSI_SEG:
> @@ -341,11 +352,22 @@ ret_t do_physdev_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>  case PHYSDEVOP_unmap_pirq: {
>  struct physdev_unmap_pirq unmap;
> +struct domain *d;
>  
>  ret = -EFAULT;
>  if ( copy_from_guest(, arg, 1) != 0 )
>  break;
>  
> +d = rcu_lock_domain_by_any_id(unmap.domid);
> +if ( d == NULL )
> +return -ESRCH;
> +if ( !is_pv_domain(d) && !has_pirq(d) )

same here


Other than that, everything looks fine to me


> +{
> +rcu_unlock_domain(d);
> +return -EOPNOTSUPP;
> +}
> +rcu_unlock_domain(d);
> +
>  ret = physdev_unmap_pirq(unmap.domid, unmap.pirq);
>  break;
>  }
> -- 
> 2.34.1
> 

Re: [RFC XEN PATCH v5 1/5] xen/vpci: Clear all vpci status of device

[Stefano Stabellini]

On Fri, 9 Feb 2024, Stewart Hildebrand wrote:
> On 1/12/24 01:13, Jiqian Chen wrote:
> > When a device has been reset on dom0 side, the vpci on Xen
> > side won't get notification, so the cached state in vpci is
> > all out of date compare with the real device state.
> > To solve that problem, add a new hypercall to clear all vpci
> > device state. When the state of device is reset on dom0 side,
> > dom0 can call this hypercall to notify vpci.
> > 
> > Co-developed-by: Huang Rui 
> > Signed-off-by: Jiqian Chen 
> 
> Reviewed-by: Stewart Hildebrand 

Reviewed-by: Stefano Stabellini 

Re: [RFC QEMU PATCH v4 1/1] xen: Use gsi instead of irq for mapping pirq

[Stefano Stabellini]

On Fri, 5 Jan 2024, Jiqian Chen wrote:
> In PVH dom0, it uses the linux local interrupt mechanism,
> when it allocs irq for a gsi, it is dynamic, and follow
> the principle of applying first, distributing first. And
> the irq number is alloced from small to large, but the
> applying gsi number is not, may gsi 38 comes before gsi
> 28, that causes the irq number is not equal with the gsi
> number. And when passthrough a device, qemu wants to use
> gsi to map pirq, xen_pt_realize->xc_physdev_map_pirq, but
> the gsi number is got from file
> /sys/bus/pci/devices//irq in current code, so it
> will fail when mapping.
> 
> Add gsi into XenHostPCIDevice and use gsi number that
> read from gsi sysfs if it exists.
> 
> Co-developed-by: Huang Rui 
> Signed-off-by: Jiqian Chen 

Reviewed-by: Stefano Stabellini 


> ---
>  hw/xen/xen-host-pci-device.c | 7 +++
>  hw/xen/xen-host-pci-device.h | 1 +
>  hw/xen/xen_pt.c  | 6 +-
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 8c6e9a1716a2..5be3279aa25b 100644
> --- a/hw/xen/xen-host-pci-device.c
> +++ b/hw/xen/xen-host-pci-device.c
> @@ -370,6 +370,13 @@ void xen_host_pci_device_get(XenHostPCIDevice *d, 
> uint16_t domain,
>  }
>  d->irq = v;
>  
> +xen_host_pci_get_dec_value(d, "gsi", , errp);
> +if (*errp) {
> +d->gsi = -1;
> +} else {
> +d->gsi = v;
> +}
> +
>  xen_host_pci_get_hex_value(d, "class", , errp);
>  if (*errp) {
>  goto error;
> diff --git a/hw/xen/xen-host-pci-device.h b/hw/xen/xen-host-pci-device.h
> index 4d8d34ecb024..74c552bb5548 100644
> --- a/hw/xen/xen-host-pci-device.h
> +++ b/hw/xen/xen-host-pci-device.h
> @@ -27,6 +27,7 @@ typedef struct XenHostPCIDevice {
>  uint16_t device_id;
>  uint32_t class_code;
>  int irq;
> +int gsi;
>  
>  XenHostPCIIORegion io_regions[PCI_NUM_REGIONS - 1];
>  XenHostPCIIORegion rom;
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 36e6f93c372f..d448f3a17306 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -839,7 +839,11 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
>  goto out;
>  }
>  
> -machine_irq = s->real_device.irq;
> +if (s->real_device.gsi < 0) {
> +machine_irq = s->real_device.irq;
> +} else {
> +machine_irq = s->real_device.gsi;
> +}
>  if (machine_irq == 0) {
>  XEN_PT_LOG(d, "machine irq is 0\n");
>  cmd |= PCI_COMMAND_INTX_DISABLE;
> -- 
> 2.34.1
> 

Re: [RFC KERNEL PATCH v4 2/3] xen/pvh: Setup gsi for passthrough device

[Stefano Stabellini]

On Fri, 5 Jan 2024, Jiqian Chen wrote:
> In PVH dom0, the gsis don't get registered, but the gsi of
> a passthrough device must be configured for it to be able to be
> mapped into a domU.
> 
> When assign a device to passthrough, proactively setup the gsi
> of the device during that process.
> 
> Co-developed-by: Huang Rui 
> Signed-off-by: Jiqian Chen 
> ---
>  arch/x86/xen/enlighten_pvh.c   | 90 ++
>  drivers/acpi/pci_irq.c |  2 +-
>  drivers/xen/xen-pciback/pci_stub.c |  8 +++
>  include/linux/acpi.h   |  1 +
>  include/xen/acpi.h |  6 ++
>  5 files changed, 106 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
> index ada3868c02c2..ecadd966c684 100644
> --- a/arch/x86/xen/enlighten_pvh.c
> +++ b/arch/x86/xen/enlighten_pvh.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  
> @@ -25,6 +26,95 @@
>  bool __ro_after_init xen_pvh;
>  EXPORT_SYMBOL_GPL(xen_pvh);
>  
> +typedef struct gsi_info {
> + int gsi;
> + int trigger;
> + int polarity;
> +} gsi_info_t;
> +
> +struct acpi_prt_entry {
> + struct acpi_pci_id  id;
> + u8  pin;
> + acpi_handle link;
> + u32 index;  /* GSI, or link _CRS index */
> +};
> +
> +static int xen_pvh_get_gsi_info(struct pci_dev *dev,
> + gsi_info_t 
> *gsi_info)
> +{
> + int gsi;
> + u8 pin = 0;
> + struct acpi_prt_entry *entry;
> + int trigger = ACPI_LEVEL_SENSITIVE;
> + int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ?
> +   ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW;
> +
> + if (dev)
> + pin = dev->pin;

This is minor, but you can just move the pin = dev->pin after the !dev
check below.

With that change, and assuming the Xen-side and QEMU-side patches are
accepted:

Reviewed-by: Stefano Stabellini 




> + if (!dev || !pin || !gsi_info)
> + return -EINVAL;
> +
> + entry = acpi_pci_irq_lookup(dev, pin);
> + if (entry) {
> + if (entry->link)
> + gsi = acpi_pci_link_allocate_irq(entry->link,
> +  entry->index,
> +  , ,
> +  NULL);
> + else
> + gsi = entry->index;
> + } else
> + gsi = -1;
> +
> + if (gsi < 0)
> + return -EINVAL;
> +
> + gsi_info->gsi = gsi;
> + gsi_info->trigger = trigger;
> + gsi_info->polarity = polarity;
> +
> + return 0;
> +}
> +
> +static int xen_pvh_setup_gsi(gsi_info_t *gsi_info)
> +{
> + struct physdev_setup_gsi setup_gsi;
> +
> + if (!gsi_info)
> + return -EINVAL;
> +
> + setup_gsi.gsi = gsi_info->gsi;
> + setup_gsi.triggering = (gsi_info->trigger == ACPI_EDGE_SENSITIVE ? 0 : 
> 1);
> + setup_gsi.polarity = (gsi_info->polarity == ACPI_ACTIVE_HIGH ? 0 : 1);
> +
> + return HYPERVISOR_physdev_op(PHYSDEVOP_setup_gsi, _gsi);
> +}
> +
> +int xen_pvh_passthrough_gsi(struct pci_dev *dev)
> +{
> + int ret;
> + gsi_info_t gsi_info;
> +
> + if (!dev)
> + return -EINVAL;
> +
> + ret = xen_pvh_get_gsi_info(dev, _info);
> + if (ret) {
> + xen_raw_printk("Fail to get gsi info!\n");
> + return ret;
> + }
> +
> + ret = xen_pvh_setup_gsi(_info);
> + if (ret == -EEXIST) {
> + xen_raw_printk("Already setup the GSI :%d\n", gsi_info.gsi);
> + ret = 0;
> + } else if (ret)
> + xen_raw_printk("Fail to setup GSI (%d)!\n", gsi_info.gsi);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(xen_pvh_passthrough_gsi);
> +
>  void __init xen_pvh_init(struct boot_params *boot_params)
>  {
>   u32 msr;
> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
> index ff30ceca2203..630fe0a34bc6 100644
> --- a/drivers/acpi/pci_irq.c
> +++ b/drivers/acpi/pci_irq.c
> @@ -288,7 +288,7 @@ static int acpi_reroute_boot_interrupt(struct pci_dev 
> *dev,
>  }
>  #endif /* CONFIG_X86_IO_APIC */
>  
> -static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int 
> pin)
> +struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin)
>  {
>   struct acpi_prt_entry *entry = NULL;
>   struct pci_dev *bridge;
> diff --git a/drivers/xen/xen-pciback/pci_stub.c 
> b/drivers/xen/xen-pciback/pci_stub.c
> index 46c40ec8a18e..22d4380d2b04 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -435,6 +436,13 @@ static int pcistub_init_device(struct pci_dev *dev)
>   

Re: [RFC KERNEL PATCH v4 1/3] xen/pci: Add xen_reset_device_state function

[Stefano Stabellini]

On Fri, 5 Jan 2024, Jiqian Chen wrote:
> When device on dom0 side has been reset, the vpci on Xen side
> won't get notification, so that the cached state in vpci is
> all out of date with the real device state.
> To solve that problem, add a new function to clear all vpci
> device state when device is reset on dom0 side.
> 
> And call that function in pcistub_init_device. Because when
> using "pci-assignable-add" to assign a passthrough device in
> Xen, it will reset passthrough device and the vpci state will
> out of date, and then device will fail to restore bar state.
> 
> Co-developed-by: Huang Rui 
> Signed-off-by: Jiqian Chen 

Reviewed-by: Stefano Stabellini 


> ---
>  drivers/xen/pci.c  | 12 
>  drivers/xen/xen-pciback/pci_stub.c | 18 +++---
>  include/xen/interface/physdev.h|  7 +++
>  include/xen/pci.h  |  6 ++
>  4 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
> index 72d4e3f193af..e9b30bc09139 100644
> --- a/drivers/xen/pci.c
> +++ b/drivers/xen/pci.c
> @@ -177,6 +177,18 @@ static int xen_remove_device(struct device *dev)
>   return r;
>  }
>  
> +int xen_reset_device_state(const struct pci_dev *dev)
> +{
> + struct physdev_pci_device device = {
> + .seg = pci_domain_nr(dev->bus),
> + .bus = dev->bus->number,
> + .devfn = dev->devfn
> + };
> +
> + return HYPERVISOR_physdev_op(PHYSDEVOP_pci_device_state_reset, );
> +}
> +EXPORT_SYMBOL_GPL(xen_reset_device_state);
> +
>  static int xen_pci_notifier(struct notifier_block *nb,
>   unsigned long action, void *data)
>  {
> diff --git a/drivers/xen/xen-pciback/pci_stub.c 
> b/drivers/xen/xen-pciback/pci_stub.c
> index e34b623e4b41..46c40ec8a18e 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -89,6 +89,16 @@ static struct pcistub_device *pcistub_device_alloc(struct 
> pci_dev *dev)
>   return psdev;
>  }
>  
> +static int pcistub_reset_device_state(struct pci_dev *dev)
> +{
> + __pci_reset_function_locked(dev);
> +
> + if (!xen_pv_domain())
> + return xen_reset_device_state(dev);
> + else
> + return 0;
> +}
> +
>  /* Don't call this directly as it's called by pcistub_device_put */
>  static void pcistub_device_release(struct kref *kref)
>  {
> @@ -107,7 +117,7 @@ static void pcistub_device_release(struct kref *kref)
>   /* Call the reset function which does not take lock as this
>* is called from "unbind" which takes a device_lock mutex.
>*/
> - __pci_reset_function_locked(dev);
> + pcistub_reset_device_state(dev);
>   if (dev_data &&
>   pci_load_and_free_saved_state(dev, _data->pci_saved_state))
>   dev_info(>dev, "Could not reload PCI state\n");
> @@ -284,7 +294,7 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
>* (so it's ready for the next domain)
>*/
>   device_lock_assert(>dev);
> - __pci_reset_function_locked(dev);
> + pcistub_reset_device_state(dev);
>  
>   dev_data = pci_get_drvdata(dev);
>   ret = pci_load_saved_state(dev, dev_data->pci_saved_state);
> @@ -420,7 +430,9 @@ static int pcistub_init_device(struct pci_dev *dev)
>   dev_err(>dev, "Could not store PCI conf saved state!\n");
>   else {
>   dev_dbg(>dev, "resetting (FLR, D3, etc) the device\n");
> - __pci_reset_function_locked(dev);
> + err = pcistub_reset_device_state(dev);
> + if (err)
> + goto config_release;
>   pci_restore_state(dev);
>   }
>   /* Now disable the device (this also ensures some private device
> diff --git a/include/xen/interface/physdev.h b/include/xen/interface/physdev.h
> index a237af867873..8609770e28f5 100644
> --- a/include/xen/interface/physdev.h
> +++ b/include/xen/interface/physdev.h
> @@ -256,6 +256,13 @@ struct physdev_pci_device_add {
>   */
>  #define PHYSDEVOP_prepare_msix  30
>  #define PHYSDEVOP_release_msix  31
> +/*
> + * Notify the hypervisor that a PCI device has been reset, so that any
> + * internally cached state is regenerated.  Should be called after any
> + * device reset performed by the hardware domain.
> + */
> +#define PHYSDEVOP_pci_device_state_reset 32
> +
>  struct physdev_pci_device {
>  /* IN */
>  uint16_t seg;
> diff --git a/include/xen/pci.h b/include/xen/pci.h
> index b8337cf85fd1..b2e2e856efd6 100644
> --- a/include/xen/pci.h
> +++ b/include/xen/pci.h
> @@ -4,10 +4,16 @@
>  #define __XEN_PCI_H__
>  
>  #if defined(CONFIG_XEN_DOM0)
> +int xen_reset_device_state(const struct pci_dev *dev);
>  int xen_find_device_domain_owner(struct pci_dev *dev);
>  int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
>  int xen_unregister_device_domain_owner(struct pci_dev *dev);
>  #else
> +static inline int 

Re: AMD Working On VirtIO GPU Support For Xen Virtualization issues

[Stefano Stabellini]

Hi Zewei,

Did you manage to make progress on this issue?

I apologize for answering so late, but this email fell through the
cracks,

Cheers,

Stefano


On Mon, 20 Nov 2023, ZHANG Zewei wrote:
> INTERNAL & PARTNERS
> 
> 
> Hi，Xen-devel：
> 
>    We are trying to implement the igpu sharing function for domU on the 
> AMD (v2000 series) development board environment, and the
> software architecture is xen + ubuntu 22.04 (dom0) + Ubuntu 22.04 (Dom U). We 
> found that AMD engineers like Mr. Huang @ray.hu...@amd.com
> have posted information about virtio-gpu  for Xen virtualization to the open 
> source community and uploaded the relevant code.
> 
>  
> 
>    And We refer to the relevant information linked below to deploy on my 
> AMD hardware platform:
> 
> [RFC PATCH 0/5] Add Xen PVH dom0 support for GPU - Huang Rui (kernel.org)
> 
>  
> 
>    However, when we deploy virtio-GPU related software, we encounter the 
> following issues:
> 
> 1.    My code of xen /qemu /Virglrenderer checkout to the upstream-for-xen or 
> upstream-for-xen-v2 branch, the xl configuration file is set
> as follows, see< ubuntuhvm_virtIO-gpu.cfg>:
> 
>       device_model_args_hvm= ["-display", "sdl,gl=on", "-device", 
> "virtio-vga-gl,context_init=true,blob=true,hostmem=4G"]
> 
>  
> 
> 2.    After the domU is started with the xl command, qemu and xen hypervisor 
> will report the following errors:
> 
>  xen hypervisor report < xl_dmesg_upstream-for-xen-v2.txt>:
> 
>       (XEN) d0v8 Over-allocation for d1: 1048833 > 1048832
> 
> (XEN) common/memory.c:277:d0v8 Could not allocate order=0 extent: id=1 
> memflags=0xc0 (192 of 2048)
> 
>       qemu report  in log file < qemu-dm-ubuntuU_b.hvm.log>:
> 
>       qemu-system-i386: -device 
> virtio-vga-gl,context_init=true,blob=true,hostmem=4G: xen: failed to populate 
> ram at 11008
> 
>  
> 
> 3.    I tried to start pvh dom0 but it didn't work, it looks the same 
> regardless of whether I'm using upstream-for-xen-v2 or
> upstream-for-xen branch linux ：
> 
> Logs in attachment:< serial_xen_hypervisor-linux-upstream-for-xen-v2.txt>
> 
>  
> 
>  
> 
>    So I have some questions for you :
> 
> 1. Does Dom 0 need to be configured with PVH for VirtIO-GPU?
> 
> 2. What may cause the above issues?
> 
>  
> 
> We look forward to hearing from you!
> 
>  
> 
> Thanks & Best regards!
> 
>  
> 
>  
> 
>  
> 
>  
> 
>  
> 
> 
> 5acXjzUk
> 
> 
> 

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

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-amd64-xl-credit1 22 guest-start/debian.repeat fail in 184722 pass 
in 184725
 test-amd64-amd64-freebsd12-amd64 21 guest-start/freebsd.repeat fail pass in 
184722

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

version targeted for testing:
 linux39133352cbed6626956d38ed72012f49b0421e7b
baseline version:
 linux9fc1fd8d53dc7936fe6d633f2373fc9f62e8

Last test of basis   184719  2024-02-21 06:34:23 Z1 days
Testing same since   184721  2024-02-21 17:43:51 Z1 days3 attempts


People who touched revisions under test:
  David Sterba 
  Herbert Xu 
  Jason Wang 
  Josef Bacik 
  Linus Torvalds 
  Marc Zyngier 
  Michael S. Tsirkin 
  Nathan Chancellor  # build
  Oliver Upton 
  Paolo Bonzini 
  Qu Wenruo 
  zhenwei pi 

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

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 xen  812bdc99f4c5d05d20b6fac03b90920c0dbf9a2b
baseline version:
 xen  8c5e4ce14509ca85b3f5faa298a766b8ffdd4f85

Last test of basis   184728  2024-02-22 14:04:05 Z0 days
Testing same since   184730  2024-02-22 17:02:09 Z0 days1 attempts


People who touched revisions under test:
  Luca Fancellu 
  Stefano Stabellini 

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



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

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

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

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


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   8c5e4ce145..812bdc99f4  812bdc99f4c5d05d20b6fac03b90920c0dbf9a2b -> smoke

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

[Andrew Cooper]

On 22/02/2024 4:55 pm, Jan Beulich wrote:
> On 22.02.2024 17:44, Roger Pau Monne wrote:
>> --- a/xen/arch/x86/include/asm/alternative.h
>> +++ b/xen/arch/x86/include/asm/alternative.h
>> @@ -167,9 +167,25 @@ extern void alternative_branches(void);
>>  #define ALT_CALL_arg5 "r8"
>>  #define ALT_CALL_arg6 "r9"
>>  
>> +#ifdef CONFIG_CC_IS_CLANG
>> +/*
>> + * Use an union with an unsigned long in order to prevent clang from 
>> skipping a
>> + * possible truncation of the value.  By using the union any truncation is
>> + * carried before the call instruction.
>> + * https://github.com/llvm/llvm-project/issues/82598
>> + */
> I think it needs saying that this is relying on compiler behavior not
> mandated by the standard, thus explaining why it's restricted to
> Clang (down the road we may even want to restrict to old versions,
> assuming they fix the issue at some point). Plus also giving future
> readers a clear understanding that if something breaks with this, it's
> not really a surprise.
>
> Aiui this bug is only a special case of the other, much older one, so
> referencing that one here too would seem advisable.
>
> As a nit: I think it is "a union" (spelling according to pronunciation),
> and I guess the title could now do with saying "optionally" or
> mentioning Clang or some such.

Yes.  "a union" is the right form here.

~Andrew

P.S. Spoken, "an union" would come across as "an onion", which is
something very different.

P.P.S.  As I noted to Matthew concerning
https://mjg59.dreamwidth.org/66907.html

"There's an interesting sidechannel in your blog post.  You seem to call
it an S-O-C and not a soc(k), based on the "an""

Re: [PATCH 1/2] x86/memsharing: use an atomic add instead of a cmpxchg loop

[Tamas K Lengyel]

On Thu, Feb 22, 2024 at 5:06 AM Jan Beulich  wrote:
>
> On 22.02.2024 10:05, Roger Pau Monne wrote:
> > The usage of a cmpxchg loop in get_next_handle() is unnecessary, as the same
> > can be achieved with an atomic increment, which is both simpler to read, and
> > avoid any need for a loop.
> >
> > The cmpxchg usage is likely a remnant of 32bit support, which didn't have an
> > instruction to do an atomic 64bit add, and instead a cmpxchg had to be used.
> >
> > Suggested-by: Andrew Cooper 
> > Signed-of-by: Roger Pau Monné 
>
> Reviewed-by: Jan Beulich 
> albeit ...
>
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -179,13 +179,7 @@ static void mem_sharing_page_unlock(struct page_info 
> > *pg)
> >
> >  static shr_handle_t get_next_handle(void)
> >  {
> > -/* Get the next handle get_page style */
> > -uint64_t x, y = next_handle;
> > -do {
> > -x = y;
> > -}
> > -while ( (y = cmpxchg(_handle, x, x + 1)) != x );
> > -return x + 1;
> > +return arch_fetch_and_add(_handle, 1) + 1;
> >  }
>
> ... the adding of 1 here is a little odd when taken together with
> next_handle's initializer. Tamas, you've not written that code, but do
> you have any thoughts towards the possible removal of either the
> initializer or the adding here? Plus that variable of course could
> very well do with moving into this function.

I have to say I find the existing logic here hard to parse but by the
looks I don't think we need the + 1 once we switch to
arch_fetch_and_add. Also could go without initializing next_handle to
1. Moving it into the function would not really accomplish anything
other than style AFAICT?

Tamas

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

[Jan Beulich]

On 22.02.2024 17:44, Roger Pau Monne wrote:
> --- a/xen/arch/x86/include/asm/alternative.h
> +++ b/xen/arch/x86/include/asm/alternative.h
> @@ -167,9 +167,25 @@ extern void alternative_branches(void);
>  #define ALT_CALL_arg5 "r8"
>  #define ALT_CALL_arg6 "r9"
>  
> +#ifdef CONFIG_CC_IS_CLANG
> +/*
> + * Use an union with an unsigned long in order to prevent clang from 
> skipping a
> + * possible truncation of the value.  By using the union any truncation is
> + * carried before the call instruction.
> + * https://github.com/llvm/llvm-project/issues/82598
> + */

I think it needs saying that this is relying on compiler behavior not
mandated by the standard, thus explaining why it's restricted to
Clang (down the road we may even want to restrict to old versions,
assuming they fix the issue at some point). Plus also giving future
readers a clear understanding that if something breaks with this, it's
not really a surprise.

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

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

Jan

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

[Roger Pau Monne]

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

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

Would expand roughly into:

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

However under certain circumstances clang >= 16.0.0 with -O2 can generate
incorrect code, given the following example:

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

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

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

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

Clang generates the following code:

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

Note the truncation of the unsigned int parameter 'a' of foo() to uint8_t when
passed into bar() is lost.

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

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

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

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

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

Reported-by: Matthew Grooms 
Link: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=277200
Link: https://github.com/llvm/llvm-project/issues/82598
Signed-off-by: Roger Pau Monné 
---
Changes since v1:
 - Only apply the union workaround with clang.

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

diff --git a/xen/arch/x86/include/asm/alternative.h 
b/xen/arch/x86/include/asm/alternative.h
index a1cd6a9fe5b8..3fe27ea791bf 100644
--- a/xen/arch/x86/include/asm/alternative.h
+++ b/xen/arch/x86/include/asm/alternative.h
@@ -167,9 +167,25 @@ extern void alternative_branches(void);
 #define ALT_CALL_arg5 "r8"
 #define ALT_CALL_arg6 "r9"
 
+#ifdef CONFIG_CC_IS_CLANG
+/*
+ * Use an union with an unsigned long in order to prevent clang from skipping a
+ * possible truncation of the value.  By using the union any truncation is
+ * carried before the call instruction.
+ * https://github.com/llvm/llvm-project/issues/82598
+ */
+#define ALT_CALL_ARG(arg, n)\
+register union {\
+typeof(arg) e;  \
+unsigned long r;\
+} a ## n ## _ asm ( ALT_CALL_arg ## n ) = { \
+.e = ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })   \
+}
+#else
 #define ALT_CALL_ARG(arg, n) \
 register typeof(arg) a ## n ## _ asm ( ALT_CALL_arg ## n ) = \
 ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })
+#endif
 #define ALT_CALL_NO_ARG(n) \
 register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n )
 
-- 
2.43.0

Re: [PATCH v8 3/8] stubdom: extend xenstore stubdom configs

[Samuel Thibault]

Juergen Gross, le jeu. 22 févr. 2024 17:06:09 +0100, a ecrit:
> On 16.02.24 17:31, Juergen Gross wrote:
> > Extend the config files of the Xenstore stubdoms to include XENBUS
> > and 9PFRONT items in order to support file based logging.
> > 
> > Signed-off-by: Juergen Gross 
> > Reviewed-by: Jason Andryuk 
> 
> Samuel, are you fine with this patch?

Yes!

Acked-by: Samuel Thibault 

Samuel

> 
> 
> Juergen
> 
> > ---
> >   stubdom/xenstore-minios.cfg| 2 +-
> >   stubdom/xenstorepvh-minios.cfg | 2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/stubdom/xenstore-minios.cfg b/stubdom/xenstore-minios.cfg
> > index a41704bb6b..239da519b9 100644
> > --- a/stubdom/xenstore-minios.cfg
> > +++ b/stubdom/xenstore-minios.cfg
> > @@ -3,7 +3,7 @@ CONFIG_NETFRONT=n
> >   CONFIG_FBFRONT=n
> >   CONFIG_KBDFRONT=n
> >   CONFIG_CONSFRONT=n
> > -CONFIG_XENBUS=n
> >   CONFIG_LWIP=n
> > +CONFIG_9PFRONT=y
> >   CONFIG_BALLOON=y
> >   XEN_INTERFACE_VERSION=__XEN_LATEST_INTERFACE_VERSION__
> > diff --git a/stubdom/xenstorepvh-minios.cfg b/stubdom/xenstorepvh-minios.cfg
> > index 6af51f5753..752b90d7d3 100644
> > --- a/stubdom/xenstorepvh-minios.cfg
> > +++ b/stubdom/xenstorepvh-minios.cfg
> > @@ -4,7 +4,7 @@ CONFIG_NETFRONT=n
> >   CONFIG_FBFRONT=n
> >   CONFIG_KBDFRONT=n
> >   CONFIG_CONSFRONT=n
> > -CONFIG_XENBUS=n
> >   CONFIG_LWIP=n
> > +CONFIG_9PFRONT=y
> >   CONFIG_BALLOON=y
> >   XEN_INTERFACE_VERSION=__XEN_LATEST_INTERFACE_VERSION__
> 

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 xen  8c5e4ce14509ca85b3f5faa298a766b8ffdd4f85
baseline version:
 xen  9ee7dc877b8754ce2fc82500feea52c04d4e6409

Last test of basis   184727  2024-02-22 11:00:37 Z0 days
Testing same since   184728  2024-02-22 14:04:05 Z0 days1 attempts


People who touched revisions under test:
  Jan Beulich 
  Julien Grall 

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
   9ee7dc877b..8c5e4ce145  8c5e4ce14509ca85b3f5faa298a766b8ffdd4f85 -> smoke

Re: [PATCH 0/3] xen-analysis.py: Enable Xen deviation tag at the end of the line

[Julien Grall]

Hi Stefano,

On 15/02/2024 01:08, Stefano Stabellini wrote:

On Wed, 31 Jan 2024, Luca Fancellu wrote:

This serie is enabling the xen-analysis tool to parse and substitute correctly
a deviation tag put at the end of the line containing a deviation to be 
deviated.

Before this serie the only way to deviate a violation was to put the tag in the
line above:

/* SAF--safe deviate the bla bla bla */


But there are places in the code base where using the tag in the line above is
not convinient, for example:

if ( (expression) &&
  ((expression with violation) ||
  (expression) )
{
   [...]
}

In the above example is better to have the suppression comment at the end of the
line:

if ( (expression) &&
  ((expression with violation) || /* SAF--safe deviate the bla bla bla 
*/
  (expression) )
{
   [...]
}

This clearly brings up the question about the code style line length, which in
this case needs to be amended for Xen deviation tags that goes above the limit.



Hi Luca,

I tested the series in a number of configurations and everything works
as expected. Great!

For the whole series:

Acked-by: Stefano Stabellini 


This is now committed.

Cheers,


--
Julien Grall

Re: [PATCH v8 3/8] stubdom: extend xenstore stubdom configs

[Juergen Gross]

On 16.02.24 17:31, Juergen Gross wrote:

Extend the config files of the Xenstore stubdoms to include XENBUS
and 9PFRONT items in order to support file based logging.

Signed-off-by: Juergen Gross 
Reviewed-by: Jason Andryuk 


Samuel, are you fine with this patch?


Juergen


---
  stubdom/xenstore-minios.cfg| 2 +-
  stubdom/xenstorepvh-minios.cfg | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/stubdom/xenstore-minios.cfg b/stubdom/xenstore-minios.cfg
index a41704bb6b..239da519b9 100644
--- a/stubdom/xenstore-minios.cfg
+++ b/stubdom/xenstore-minios.cfg
@@ -3,7 +3,7 @@ CONFIG_NETFRONT=n
  CONFIG_FBFRONT=n
  CONFIG_KBDFRONT=n
  CONFIG_CONSFRONT=n
-CONFIG_XENBUS=n
  CONFIG_LWIP=n
+CONFIG_9PFRONT=y
  CONFIG_BALLOON=y
  XEN_INTERFACE_VERSION=__XEN_LATEST_INTERFACE_VERSION__
diff --git a/stubdom/xenstorepvh-minios.cfg b/stubdom/xenstorepvh-minios.cfg
index 6af51f5753..752b90d7d3 100644
--- a/stubdom/xenstorepvh-minios.cfg
+++ b/stubdom/xenstorepvh-minios.cfg
@@ -4,7 +4,7 @@ CONFIG_NETFRONT=n
  CONFIG_FBFRONT=n
  CONFIG_KBDFRONT=n
  CONFIG_CONSFRONT=n
-CONFIG_XENBUS=n
  CONFIG_LWIP=n
+CONFIG_9PFRONT=y
  CONFIG_BALLOON=y
  XEN_INTERFACE_VERSION=__XEN_LATEST_INTERFACE_VERSION__




OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

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

[Jan Beulich]

On 22.02.2024 15:43, Nicola Vetrini wrote:
>>> In passing it should be noted that the header ordering in
>>> x86/alternative.c is not the one usually prescribed, so that may be
>>> taken care of as well.
>>
>> I'm afraid I don't understand this remark.
>>
> 
> I just meant to say that this
> 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> 
> is not the usual order of xen/*.h then asm/*.h and there is no comment 
> justifying that ordering.

Well, you'll find such in many other places. It hasn't been for that long
that we've been trying to get #include-s into a more predictable shape.

> So in the process of including asm/flushtlb.h 
> here the inclusion order can be tidied up (or also indipendently), 
> unless there is some reason I'm missing that disallows it.

Independently, if at all possible, would be better. Unless of course you
need to touch almost all of that block anyway.

Jan

Re: [PATCH 6/6] svm/nestedvm: Introduce nested capabilities bit

[Jan Beulich]

On 22.02.2024 15:33, George Dunlap wrote:
> On Tue, Feb 20, 2024 at 12:25 AM Jan Beulich  wrote:
>> On 06.02.2024 02:20, George Dunlap wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -3021,6 +3021,9 @@ const struct hvm_function_table * __init 
>>> start_vmx(void)
>>>  if ( cpu_has_vmx_tsc_scaling )
>>>  vmx_function_table.tsc_scaling.ratio_frac_bits = 48;
>>>
>>> +/* TODO: Require hardware support before enabling nested virt */
>>> +vmx_function_table.caps.nested_virt = vmx_function_table.caps.hap;
>>
>> This won't have the intended effect if hap_supported() ends up clearing
>> the bit (used as input here) due to a command line option override. I
>> wonder if instead this wants doing e.g. in a new hook to be called from
>> nestedhvm_setup(). On the SVM side the hook function would then be the
>> start_nested_svm() that you already introduce, with a caps.hap check
>> added.
> 
> I take it presmp_initcall()'s are guaranteed to run before __initcall()'s?

Yes - the former happen ahead of AP bringup, the latter after.

>> Since you leave the other nested-related if() in place in
>> arch_sanitise_domain_config(), all ought to be well, but I think that
>> other if() really wants replacing by the one you presently add.
> 
> Ack.
> 
> I'll probably check in patches 1,2,3, and 5, and resend the other two,
> unless you'd like to see all the changes again...

No need imo to re-post anything that was agreed upon.

Jan

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

[Nicola Vetrini]

--- a/xen/arch/arm/include/asm/page.h
+++ b/xen/arch/arm/include/asm/page.h
@@ -123,6 +123,7 @@

 #ifndef __ASSEMBLY__

+#include 


This is a no-go, imo (also on x86): Adding this include here
effectively
means that nearly every CU will have a dependency on that header, 
no

matter that most are entirely agnostic of grants. Each arch has a
grant_table.h - is there any reason the new, grant-specific helper
can't
be put there?



I would have to test, but I think that can be done



The only blocker so far is that this triggers a build error due to a
circular dependency between xen/mm.h and asm/flushtlb.h on x86. Also
found some earlier evidence [1] that there are some oddities around
asm/flushtlb's inclusion.

[1]
https://lore.kernel.org/xen-devel/20200318210540.5602-1-andrew.coop...@citrix.com/


There could be a way of untangling asm/flushtlb.h from xen/mm.h, by
moving "accumulate_tlbflush" and "filtered_flush_tlb_mask" introduced 
by

commit 80943aa40e30 ("replace tlbflush check and operation with inline
functions") [1].
However, these function should then be part of a generic 
xen/flushtlb.h
header, since they are used in common code (e.g., common/page_alloc) 
and
a bunch of common code source files should move their includes (see 
[2]

for a partial non-working patch). Do you feel that this is a feasible
route?


Yeah, introducing xen/flushtlb.h to hold these sounds pretty sensible.



There is some shuffling of includes to be done to get it to build, which 
I'm still trying to address. Most problems are down to the use of struct 
page_info in page_set_tlbflush_timestamp in x86/.*/asm/flushtlb.h which 
then prompts the inclusion asm/mm.h probably.



In passing it should be noted that the header ordering in
x86/alternative.c is not the one usually prescribed, so that may be
taken care of as well.


I'm afraid I don't understand this remark.



I just meant to say that this

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

is not the usual order of xen/*.h then asm/*.h and there is no comment 
justifying that ordering. So in the process of including asm/flushtlb.h 
here the inclusion order can be tidied up (or also indipendently), 
unless there is some reason I'm missing that disallows it.


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

Re: [RFC XEN PATCH v5 1/5] xen/vpci: Clear all vpci status of device

[Stewart Hildebrand]

On 2/22/24 01:22, Chen, Jiqian wrote:
> Hi Stewart,
> 
> On 2024/2/10 02:02, Stewart Hildebrand wrote:
>> On 1/12/24 01:13, Jiqian Chen wrote:
>>> When a device has been reset on dom0 side, the vpci on Xen
>>> side won't get notification, so the cached state in vpci is
>>> all out of date compare with the real device state.
>>> To solve that problem, add a new hypercall to clear all vpci
>>> device state. When the state of device is reset on dom0 side,
>>> dom0 can call this hypercall to notify vpci.
>>>
>>> Co-developed-by: Huang Rui 
>>> Signed-off-by: Jiqian Chen 
>>
>> Reviewed-by: Stewart Hildebrand 
> Thanks, I will add in next version.
> 
>>
>> If you send another version, the RFC tag may be dropped.
> Does only this one patch, or all patches of this series, need to drop RFC tag?

In my opinion at least patches 1, 2, and 3. If you decide to retain the
RFC tag on e.g. patches 4 and/or 5, you may want to include after the
commit description a scissors line --- followed by a brief explanation
for the RFC tag. For example:

---
RFC: discussions ongoing on the Linux side where/how to expose the gsi

>>
>> One thing to keep an eye out for below (not requesting any changes).
> Thanks for reminding me, I will always keep rebasing my code from latest 
> staging branch before sending new version.
> 
>>
>>> ---
>>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>>> index 72ef277c4f8e..c6df2c6a9561 100644
>>> --- a/xen/drivers/vpci/vpci.c
>>> +++ b/xen/drivers/vpci/vpci.c
>>> @@ -107,6 +107,16 @@ int vpci_add_handlers(struct pci_dev *pdev)
>>>  
>>>  return rc;
>>>  }
>>> +
>>> +int vpci_reset_device_state(struct pci_dev *pdev)
>>> +{
>>> +ASSERT(pcidevs_locked());
>>> +ASSERT(rw_is_write_locked(>domain->pci_lock));
>>> +
>>> +vpci_remove_device(pdev);
>>> +return vpci_add_handlers(pdev);
>>
>> Note that these two functions may be renamed soon by the patch at [1].
>> Whichever patch goes in later will need to be rebased to account for the
>> rename.
>>
>> [1] 
>> https://lists.xenproject.org/archives/html/xen-devel/2024-02/msg00134.html
>>
>>> +}
>>> +
>>>  #endif /* __XEN__ */
>>>  
>>>  static int vpci_register_cmp(const struct vpci_register *r1,
>>
> 

Re: [PATCH 6/6] svm/nestedvm: Introduce nested capabilities bit

[George Dunlap]

On Tue, Feb 20, 2024 at 12:25 AM Jan Beulich  wrote:
>
> On 06.02.2024 02:20, George Dunlap wrote:
> > --- /dev/null
> > +++ b/docs/designs/nested-svm-cpu-features.md
> > @@ -0,0 +1,110 @@
> > +# Nested SVM (AMD) CPUID requirements
> > +
> > +The first step in making nested SVM production-ready is to make sure
> > +that all features are implemented and well-tested.  To make this
> > +tractable, we will initially be limiting the "supported" range of
> > +nested virt to a specific subset of host and guest features.  This
> > +document describes the criteria for deciding on features, and the
> > +rationale behind each feature.
> > +
> > +For AMD, all virtualization-related features can be found in CPUID
> > +leaf 800A:edx
> > +
> > +# Criteria
> > +
> > +- Processor support: At a minimum we want to support processors from
> > +  the last 5 years.  All things being equal, older processors are
> > +  better.
>
> Nit: Perhaps missing "covering"? Generally I hope newer processors are
> "better".

I've reworded it.

> > +  For L0 this is required for performance: There's no way to tell the
> > +  guests not to use the LBR-related registers; and if the guest does,
> > +  then you have to save and restore all LBR-related registers on
> > +  context switch, which is prohibitive.
>
> "prohibitive" is too strong imo; maybe "undesirable"?

Prohibitive sounds strong, but I don't think it really is; here's the
dictionary definition:

"(of a price or charge) so high as to prevent something being done or bought."

The cost of saving the registers on every context switch is so high as
to prevent us from wanting to do it.

I could change it to "too expensive".

> > --- a/xen/arch/x86/hvm/svm/nestedhvm.h
> > +++ b/xen/arch/x86/hvm/svm/nestedhvm.h
> > @@ -35,6 +35,7 @@ enum nestedhvm_vmexits
> >  nestedsvm_check_intercepts(struct vcpu *v, struct cpu_user_regs *regs,
> >  uint64_t exitcode);
> >  void svm_nested_features_on_efer_update(struct vcpu *v);
> > +void __init start_nested_svm(struct hvm_function_table 
> > *svm_function_table);
>
> No section placement attributes on declarations, please.

Ack.

> > --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> > +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> > @@ -1666,3 +1666,17 @@ void svm_nested_features_on_efer_update(struct vcpu 
> > *v)
> >  }
> >  }
> >  }
> > +
> > +void __init start_nested_svm(struct hvm_function_table *svm_function_table)
> > +{
> > +/*
> > + * Required host functionality to support nested virt.  See
> > + * docs/designs/nested-svm-cpu-features.md for rationale.
> > + */
> > +svm_function_table->caps.nested_virt =
> > +cpu_has_svm_nrips &&
> > +cpu_has_svm_lbrv &&
> > +cpu_has_svm_nrips &&
>
> nrips twice? Was the earlier one meant to be npt?

Er, yes exactly.



> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -3021,6 +3021,9 @@ const struct hvm_function_table * __init 
> > start_vmx(void)
> >  if ( cpu_has_vmx_tsc_scaling )
> >  vmx_function_table.tsc_scaling.ratio_frac_bits = 48;
> >
> > +/* TODO: Require hardware support before enabling nested virt */
> > +vmx_function_table.caps.nested_virt = vmx_function_table.caps.hap;
>
> This won't have the intended effect if hap_supported() ends up clearing
> the bit (used as input here) due to a command line option override. I
> wonder if instead this wants doing e.g. in a new hook to be called from
> nestedhvm_setup(). On the SVM side the hook function would then be the
> start_nested_svm() that you already introduce, with a caps.hap check
> added.

I take it presmp_initcall()'s are guaranteed to run before __initcall()'s?

If so that's probably a good idea.

> Since you leave the other nested-related if() in place in
> arch_sanitise_domain_config(), all ought to be well, but I think that
> other if() really wants replacing by the one you presently add.

Ack.

I'll probably check in patches 1,2,3, and 5, and resend the other two,
unless you'd like to see all the changes again...

 -George




 -George

Need help on USB port virtualization with Xen hypervisor

[LARRIEU Dominique]

Dear all,

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

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

The Software used are :

-Debian 11 version 5.10.0-20

-Xen version 4.14

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

Thanks in advance for your help.

Best regards,




Dominique LARRIEU

Cyber Project Manager

Thales


4, avenue des Louvresses
92230 Gennevilliers, France

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

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

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

[Jan Beulich]

On 21.02.2024 16:46, Nicola Vetrini wrote:
> On 2024-02-21 13:08, Nicola Vetrini wrote:
>> On 2024-02-20 09:14, Nicola Vetrini wrote:
>>> On 2024-02-20 08:45, Jan Beulich wrote:
 On 19.02.2024 16:14, Nicola Vetrini wrote:
> The cache clearing and invalidation helpers in x86 and Arm didn't
> comply with MISRA C Rule 17.7: "The value returned by a function
> having non-void return type shall be used". On Arm they
> were always returning 0, while some in x86 returned -EOPNOTSUPP
> and in common/grant_table the return value is saved.
>
> As a consequence, a common helper arch_grant_cache_flush that 
> returns
> an integer is introduced, so that each architecture can choose 
> whether to
> return an error value on certain conditions, and the helpers have 
> either
> been changed to return void (on Arm) or deleted entirely (on x86).
>
> Signed-off-by: Julien Grall 
> Signed-off-by: Nicola Vetrini 
> ---
> The original refactor idea came from Julien Grall in [1]; I edited 
> that proposal
> to fix build errors.
>
> I did introduce a cast to void for the call to flush_area_local on 
> x86, because
> even before this patch the return value of that function wasn't 
> checked in all
> but one use in x86/smp.c, and in this context the helper (perhaps 
> incidentally)
> ignored the return value of flush_area_local.

 I object to such casting to void, at least until there's an 
 overriding
 decision that for Misra purposes such casts may be needed.

>>>
>>> There are three choices here:
>>> 1. cast to void
>>> 2. deviation for flush_area_local, which for the case of the cache 
>>> helpers is what led to this patch; it may still be a viable option, if 
>>> other maintainers agree
>>> 3. refactor of flush_area_local; this is not viable here because the 
>>> return value is actually used and useful, as far as I can tell, in 
>>> smp.c
>>>
> --- a/xen/arch/arm/include/asm/page.h
> +++ b/xen/arch/arm/include/asm/page.h
> @@ -123,6 +123,7 @@
>
>  #ifndef __ASSEMBLY__
>
> +#include 

 This is a no-go, imo (also on x86): Adding this include here 
 effectively
 means that nearly every CU will have a dependency on that header, no
 matter that most are entirely agnostic of grants. Each arch has a
 grant_table.h - is there any reason the new, grant-specific helper 
 can't
 be put there?

>>>
>>> I would have to test, but I think that can be done
>>>
>>
>> The only blocker so far is that this triggers a build error due to a 
>> circular dependency between xen/mm.h and asm/flushtlb.h on x86. Also 
>> found some earlier evidence [1] that there are some oddities around 
>> asm/flushtlb's inclusion.
>>
>> [1] 
>> https://lore.kernel.org/xen-devel/20200318210540.5602-1-andrew.coop...@citrix.com/
> 
> There could be a way of untangling asm/flushtlb.h from xen/mm.h, by 
> moving "accumulate_tlbflush" and "filtered_flush_tlb_mask" introduced by 
> commit 80943aa40e30 ("replace tlbflush check and operation with inline 
> functions") [1].
> However, these function should then be part of a generic xen/flushtlb.h 
> header, since they are used in common code (e.g., common/page_alloc) and 
> a bunch of common code source files should move their includes (see [2] 
> for a partial non-working patch). Do you feel that this is a feasible 
> route?

Yeah, introducing xen/flushtlb.h to hold these sounds pretty sensible.

> In passing it should be noted that the header ordering in 
> x86/alternative.c is not the one usually prescribed, so that may be 
> taken care of as well.

I'm afraid I don't understand this remark.

Jan

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 xen  9ee7dc877b8754ce2fc82500feea52c04d4e6409
baseline version:
 xen  f8791d0fd3adbda3701e7eb9db63a9351b478365

Last test of basis   184713  2024-02-20 15:00:29 Z1 days
Testing same since   184727  2024-02-22 11:00:37 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Anthony PERARD 
  Daniel P. Smith  # XSM
  Federico Serafini 
  Frediano Ziglio 
  George Dunlap  # sched
  Jan Beulich 
  Juergen Gross 
  Marek Marczykowski-Górecki 
  Oleksii Kurochko 

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
   f8791d0fd3..9ee7dc877b  9ee7dc877b8754ce2fc82500feea52c04d4e6409 -> smoke

[xen-unstable test] 184723: tolerable FAIL

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 12 debian-hvm-install fail in 
184720 pass in 184723
 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 12 debian-hvm-install fail pass 
in 184720

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

version targeted for testing:
 xen  f8791d0fd3adbda3701e7eb9db63a9351b478365
baseline version:
 xen  f8791d0fd3adbda3701e7eb9db63a9351b478365

Last test of basis   184723  2024-02-22 01:52:14 Z 

Re: [PATCH 5/6] nestedsvm: Remove bogus debug message from nestedsvm_check_intercepts

[George Dunlap]

On Mon, Feb 19, 2024 at 11:56 PM Jan Beulich  wrote:
>
> On 06.02.2024 02:20, George Dunlap wrote:
> > Changeset ef3e8db8068 ("x86/hvm: Corrections and improvements to
> > unhandled vmexit logging") introduced a printk to the default path of
> > the switch statement in nestedsvm_check_intercepts(), complaining of
> > an unknown exit reason.
> >
> > Unfortunately, the "core" switch statement which is meant to handle
> > all vmexit reasons is in nsvm_vmcb_guest_intercepts_exitcode(); the
> > switch statement in nestedsvm_check_intercepts() is only meant to
> > superimpose on top of that some special-casing for how to interaction
> > between L1 and L0 vmexits.
> >
> > Remove the printk, and add a comment to prevent future confusion.
> >
> > Signed-off-by: George Dunlap 
>
> Reviewed-by: Jan Beulich 
>
> I wonder if a Fixes: tag is warranted here.

Yes, I think probably so.


 -George

Re: [PATCH 4/6] nestedsvm: Disable TscRateMSR

[Jan Beulich]

On 22.02.2024 12:00, George Dunlap wrote:
> On Thu, Feb 22, 2024 at 5:50 PM Jan Beulich  wrote:
>>
>> On 22.02.2024 10:30, George Dunlap wrote:
>>> On Wed, Feb 21, 2024 at 6:52 PM Jan Beulich  wrote:
>> But then of course Andrew may know of reasons why all of this is done
>> in calculate_host_policy() in the first place, rather than in HVM
>> policy calculation.
>
> It sounds like maybe you're confusing host_policy with
> x86_capabilities?  From what I can tell:
>
> *  the "basic" cpu_has_X macros resolve to boot_cpu_has(), which
> resolves to cpu_has(_cpu_data, ...), which is completely
> independent of the cpu-policy.c:host_cpu_policy
>
> * cpu-policy.c:host_cpu_policy only affects what is advertised to
> guests, via {pv,hvm}_cpu_policy and featureset bits.  Most notably a
> quick skim doesn't show any mechanism by which host_cpu_policy could
> affect what features Xen itself decides to use.

 I'm not mixing the two, no; the two are still insufficiently disentangled.
 There's really no reason (long term) to have both host policy and
 x86_capabilities. Therefore I'd prefer if new code (including a basically
 fundamental re-write as is going to be needed for nested) to avoid
 needlessly further extending x86_capabilities. Unless of course there's
 something fundamentally wrong with eliminating the redundancy, which
 likely Andrew would be in the best position to point out.
>>>
>>> So I don't know the history of how things got to be the way they are,
>>> nor really much about the code but what I've gathered from skimming
>>> through while creating this patch series.  But from that impression,
>>> the only issue I really see with the current code is the confusing
>>> naming.  The cpufeature.h code has this nice infrastructure to allow
>>> you to, for instance, enable or disable certain bits on the
>>> command-line; and the interface for querying all the different bits of
>>> functionality is all nicely put in one place.  Moving the
>>> svm_feature_flags into x86_capabilities would immediately allow SVM to
>>> take advantage of this infrastructure; it's not clear to me how this
>>> would be "needless".
>>>
>>> Furthermore, it looks to me like host_cpu_policy is used as a starting
>>> point for generating pv_cpu_policy and hvm_cpu_policy, both of which
>>> are only used for guest cpuid generation.  Given that the format of
>>> those policies is fixed, and there's a lot of "copy this bit from the
>>> host policy wholesale", it seems like no matter what, you'd want a
>>> host_cpu_policy.
>>>
>>> And in any case -- all that is kind of moot.  *Right now*,
>>> host_cpu_policy is only used for guest cpuid policy creation; *right
>>> now*, the nested virt features of AMD are handled in the
>>> host_cpu_policy; *right now*, we're advertising to guests bits which
>>> are not properly virtualized; *right now* these bits are actually set
>>> unconditionally, regardless of whether they're even available on the
>>> hardware; *right now*, Xen uses svm_feature_flags to determine its own
>>> use of TscRateMSR; so *right now*, removing this bit from
>>> host_cpu_policy won't prevent Xen from using TscRateMSR itself.
>>>
>>> (Unless my understanding of the code is wrong, in which case I'd
>>> appreciate a correction.)
>>
>> There's nothing wrong afaics, just missing at least one aspect: Did you
>> see all the featureset <-> policy conversions in cpu-policy.c? That (to
>> me at least) clearly is a sign of unnecessary duplication of the same
>> data. This goes as far as seeding the host policy from the raw one, just
>> to then immediately run x86_cpu_featureset_to_policy(), thus overwriting
>> a fair part of what was first taken from the raw policy. That's necessary
>> right now, because setup_{force,clear}_cpu_cap() act on
>> boot_cpu_data.x86_capability[], not the host policy.
>>
>> As to the "needless" further up, it's only as far as moving those bits
>> into x86_capability[] would further duplicate information, rather than
>> (for that piece at least) putting them into the policies right away. But
>> yes, if the goal is to have setup_{force,clear}_cpu_cap() be able to
>> control those bits as well, then going the intermediate step would be
>> unavoidable at this point in time.
> 
> I'm still not sure of what needs to happen to move this forward.
> 
> As I said, I'm not opposed to doing some prep work; but I don't want
> to randomly guess as to what kinds of clean-up needs to be done, only
> to be told it was wrong (either by you when I post it or by Andy
> sometime after it's checked in).
> 
> I could certainly move svm_feature_flags into host_cpu_policy, and
> have cpu_svm_feature_* reference host_cpu_policy instead (after moving
> the nested virt "guest policy" tweaks into hvm_cpu_policy); but as far
> as I can tell, that would be the *very first* instance of Xen using
> host_cpu_policy in that manner.  I'd like more clarity that 

Re: [PATCH 2/2] x86/hpet: use an atomic add instead of a cmpxchg loop

[Julien Grall]

Hi Roger,

On 22/02/2024 09:05, Roger Pau Monne wrote:

The usage of a cmpxchg loop in hpet_get_channel() is unnecessary, as the same
can be achieved with an atomic increment, which is both simpler to read, and
avoid any need for a loop.

Note there can be a small divergence in the channel returned if next_channel
overflows, but returned channel will always be in the [0, num_hpets_used)
range, and that's fine for the purpose of balancing HPET channels across CPUs.
This is also theoretical, as there's no system currently with 2^32 CPUs (as
long as next_channel is 32bit width).


This would also happen if we decide to reduce the size next_channel.

Reviewed-by: Julien Grall 

Cheers,

--
Julien Grall

Re: [PATCH 1/2] x86/memsharing: use an atomic add instead of a cmpxchg loop

[Julien Grall]

Hi Roger,

On 22/02/2024 09:05, Roger Pau Monne wrote:

The usage of a cmpxchg loop in get_next_handle() is unnecessary, as the same
can be achieved with an atomic increment, which is both simpler to read, and
avoid any need for a loop.

The cmpxchg usage is likely a remnant of 32bit support, which didn't have an
instruction to do an atomic 64bit add, and instead a cmpxchg had to be used.

Suggested-by: Andrew Cooper 
Signed-of-by: Roger Pau Monné 

Reviewed-by: Julien Grall 

Cheers,

--
Julien Grall

Re: [PATCH 2/2] x86/hpet: use an atomic add instead of a cmpxchg loop

[Jan Beulich]

On 22.02.2024 11:58, Roger Pau Monné wrote:
> On Thu, Feb 22, 2024 at 11:10:54AM +0100, Jan Beulich wrote:
>> On 22.02.2024 10:05, Roger Pau Monne wrote:
>>> The usage of a cmpxchg loop in hpet_get_channel() is unnecessary, as the 
>>> same
>>> can be achieved with an atomic increment, which is both simpler to read, and
>>> avoid any need for a loop.
>>>
>>> Note there can be a small divergence in the channel returned if next_channel
>>> overflows, but returned channel will always be in the [0, num_hpets_used)
>>> range, and that's fine for the purpose of balancing HPET channels across 
>>> CPUs.
>>> This is also theoretical, as there's no system currently with 2^32 CPUs (as
>>> long as next_channel is 32bit width).
>>
>> The code change looks good to me, but I fail to see the connection to
>> 2^32 CPUs. So it feels like I'm missing something, in which case I'd
>> rather not offer any R-b.
> 
> The purpose of hpet_get_channel() is to distribute HPET channels
> across CPUs, so that each CPU gets assigned an HPET channel, balancing
> the number of CPUs that use each channel.
> 
> This is done by (next_channel++ % num_hpets_used), so the value of
> next_channel after this change can be > num_hpets_used, which
> previously wasn't.  This can lead to a different channel returned for
> the 2^32 call to the function, as the counter would overflow.  Note
> calls to the function are restricted to the number of CPUs in the
> host, as per-CPU channel assignment is done only once (and the channel
> is then stored in a per-cpu variable).

That's once per CPU being brought up, not once per CPU in the system.

> Feel free to adjust the commit message if you think all this is too
> much data, or too confusing.

I'll simply drop that last sentence then, without which
Reviewed-by: Jan Beulich 

Jan

Re: [PATCH 4/6] nestedsvm: Disable TscRateMSR

[George Dunlap]

On Thu, Feb 22, 2024 at 5:50 PM Jan Beulich  wrote:
>
> On 22.02.2024 10:30, George Dunlap wrote:
> > On Wed, Feb 21, 2024 at 6:52 PM Jan Beulich  wrote:
>  But then of course Andrew may know of reasons why all of this is done
>  in calculate_host_policy() in the first place, rather than in HVM
>  policy calculation.
> >>>
> >>> It sounds like maybe you're confusing host_policy with
> >>> x86_capabilities?  From what I can tell:
> >>>
> >>> *  the "basic" cpu_has_X macros resolve to boot_cpu_has(), which
> >>> resolves to cpu_has(_cpu_data, ...), which is completely
> >>> independent of the cpu-policy.c:host_cpu_policy
> >>>
> >>> * cpu-policy.c:host_cpu_policy only affects what is advertised to
> >>> guests, via {pv,hvm}_cpu_policy and featureset bits.  Most notably a
> >>> quick skim doesn't show any mechanism by which host_cpu_policy could
> >>> affect what features Xen itself decides to use.
> >>
> >> I'm not mixing the two, no; the two are still insufficiently disentangled.
> >> There's really no reason (long term) to have both host policy and
> >> x86_capabilities. Therefore I'd prefer if new code (including a basically
> >> fundamental re-write as is going to be needed for nested) to avoid
> >> needlessly further extending x86_capabilities. Unless of course there's
> >> something fundamentally wrong with eliminating the redundancy, which
> >> likely Andrew would be in the best position to point out.
> >
> > So I don't know the history of how things got to be the way they are,
> > nor really much about the code but what I've gathered from skimming
> > through while creating this patch series.  But from that impression,
> > the only issue I really see with the current code is the confusing
> > naming.  The cpufeature.h code has this nice infrastructure to allow
> > you to, for instance, enable or disable certain bits on the
> > command-line; and the interface for querying all the different bits of
> > functionality is all nicely put in one place.  Moving the
> > svm_feature_flags into x86_capabilities would immediately allow SVM to
> > take advantage of this infrastructure; it's not clear to me how this
> > would be "needless".
> >
> > Furthermore, it looks to me like host_cpu_policy is used as a starting
> > point for generating pv_cpu_policy and hvm_cpu_policy, both of which
> > are only used for guest cpuid generation.  Given that the format of
> > those policies is fixed, and there's a lot of "copy this bit from the
> > host policy wholesale", it seems like no matter what, you'd want a
> > host_cpu_policy.
> >
> > And in any case -- all that is kind of moot.  *Right now*,
> > host_cpu_policy is only used for guest cpuid policy creation; *right
> > now*, the nested virt features of AMD are handled in the
> > host_cpu_policy; *right now*, we're advertising to guests bits which
> > are not properly virtualized; *right now* these bits are actually set
> > unconditionally, regardless of whether they're even available on the
> > hardware; *right now*, Xen uses svm_feature_flags to determine its own
> > use of TscRateMSR; so *right now*, removing this bit from
> > host_cpu_policy won't prevent Xen from using TscRateMSR itself.
> >
> > (Unless my understanding of the code is wrong, in which case I'd
> > appreciate a correction.)
>
> There's nothing wrong afaics, just missing at least one aspect: Did you
> see all the featureset <-> policy conversions in cpu-policy.c? That (to
> me at least) clearly is a sign of unnecessary duplication of the same
> data. This goes as far as seeding the host policy from the raw one, just
> to then immediately run x86_cpu_featureset_to_policy(), thus overwriting
> a fair part of what was first taken from the raw policy. That's necessary
> right now, because setup_{force,clear}_cpu_cap() act on
> boot_cpu_data.x86_capability[], not the host policy.
>
> As to the "needless" further up, it's only as far as moving those bits
> into x86_capability[] would further duplicate information, rather than
> (for that piece at least) putting them into the policies right away. But
> yes, if the goal is to have setup_{force,clear}_cpu_cap() be able to
> control those bits as well, then going the intermediate step would be
> unavoidable at this point in time.

I'm still not sure of what needs to happen to move this forward.

As I said, I'm not opposed to doing some prep work; but I don't want
to randomly guess as to what kinds of clean-up needs to be done, only
to be told it was wrong (either by you when I post it or by Andy
sometime after it's checked in).

I could certainly move svm_feature_flags into host_cpu_policy, and
have cpu_svm_feature_* reference host_cpu_policy instead (after moving
the nested virt "guest policy" tweaks into hvm_cpu_policy); but as far
as I can tell, that would be the *very first* instance of Xen using
host_cpu_policy in that manner.  I'd like more clarity that this is
the long-term direction that things are going before then.

If you (plural) don't 

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

[Jan Beulich]

On 22.02.2024 11:50, Roger Pau Monné wrote:
> On Thu, Feb 22, 2024 at 11:32:14AM +0100, Jan Beulich wrote:
>> On 21.02.2024 18:03, Roger Pau Monne wrote:
>>> The above can be worked around by using an union when defining the register
>>> variables, so that `di` becomes:
>>>
>>> register union {
>>> uint8_t e;
>>> unsigned long r;
>>> } di asm("rdi") = { .e = b };
>>>
>>> Which results in following code generated for `foo()`:
>>>
>>> foo:# @foo
>>> movzbl  %dil, %edi
>>> callq   func
>>> retq
>>>
>>> So the truncation is not longer lost.
>>
>> But how do you explain this behavior? I see absolutely no reason why filling
>> the one union field should lead to zero-extension. If I'm not mistaken the
>> language allows the rest of the union to retain undefined contents. So to me
>> this looks like you're converting something that failed to build due to a
>> (presumed) bug in Clang to something that any compiler would be permitted to
>> translate to other than what we want.
> 
> Oh, right, I was expecting the compiler to zero extend it, confabulating
> how unmentioned fields are initialized in structs.
> 
> However, if as mentioned in the psABI thread, the callee is required
> to do any zero extension as necessary, using the union shouldn't cause
> issues for compilers that implement the ABI properly.
> 
> IOW: I don't think the proposed workaround would cause issues for gcc.

Well, that's making a lot of assumptions then. There may indeed be little
reason for the compiler to emit different code, but it is absolutely free
to do so. I continue to think that we want to limit this workaround to as
narrow a set of compilers as possible. Clearly for Clang the code is
better than without the workaround, so there's no reason to take this as
an improvement even if in some obscure case it would still cause an issue.
But for a compiler that produces correct code without this workaround, we
shouldn't chance the workaround breaking some case somewhere.

Jan

Re: [PATCH 2/2] x86/hpet: use an atomic add instead of a cmpxchg loop

[Roger Pau Monné]

On Thu, Feb 22, 2024 at 11:10:54AM +0100, Jan Beulich wrote:
> On 22.02.2024 10:05, Roger Pau Monne wrote:
> > The usage of a cmpxchg loop in hpet_get_channel() is unnecessary, as the 
> > same
> > can be achieved with an atomic increment, which is both simpler to read, and
> > avoid any need for a loop.
> > 
> > Note there can be a small divergence in the channel returned if next_channel
> > overflows, but returned channel will always be in the [0, num_hpets_used)
> > range, and that's fine for the purpose of balancing HPET channels across 
> > CPUs.
> > This is also theoretical, as there's no system currently with 2^32 CPUs (as
> > long as next_channel is 32bit width).
> 
> The code change looks good to me, but I fail to see the connection to
> 2^32 CPUs. So it feels like I'm missing something, in which case I'd
> rather not offer any R-b.

The purpose of hpet_get_channel() is to distribute HPET channels
across CPUs, so that each CPU gets assigned an HPET channel, balancing
the number of CPUs that use each channel.

This is done by (next_channel++ % num_hpets_used), so the value of
next_channel after this change can be > num_hpets_used, which
previously wasn't.  This can lead to a different channel returned for
the 2^32 call to the function, as the counter would overflow.  Note
calls to the function are restricted to the number of CPUs in the
host, as per-CPU channel assignment is done only once (and the channel
is then stored in a per-cpu variable).

Feel free to adjust the commit message if you think all this is too
much data, or too confusing.

Thanks, Roger.

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

[Roger Pau Monné]

On Thu, Feb 22, 2024 at 11:32:14AM +0100, Jan Beulich wrote:
> On 21.02.2024 18:03, Roger Pau Monne wrote:
> > The current code for alternative calls uses the caller parameter types as 
> > the
> > types for the register variables that serve as function parameters:
> > 
> > uint8_t foo;
> > [...]
> > alternative_call(myfunc, foo);
> > 
> > Would expand roughly into:
> > 
> > register unint8_t a1_ asm("rdi") = foo;
> > register unsigned long a2_ asm("rsi");
> > [...]
> > asm volatile ("call *%c[addr](%%rip)"...);
> > 
> > However under certain circumstances clang >= 16.0.0 with -O2 can generate
> > incorrect code,
> 
> Considering that the related (wider) ABI issue looks to also be present on
> Clang5, is the more specific issue here really limited to >= 16?

No, this is wrong.  I did check clang 15.0.0 and I messed up the
output, all clang versions (on godbolt) seem to be affected.

> > given the following example:
> > 
> > unsigned int func(uint8_t t)
> > {
> > return t;
> > }
> > 
> > static void bar(uint8_t b)
> > {
> > int ret_;
> > register uint8_t di asm("rdi") = b;
> > register unsigned long si asm("rsi");
> > register unsigned long dx asm("rdx");
> > register unsigned long cx asm("rcx");
> > register unsigned long r8 asm("r8");
> > register unsigned long r9 asm("r9");
> > register unsigned long r10 asm("r10");
> > register unsigned long r11 asm("r11");
> > 
> > asm volatile ( "call %c[addr]"
> >: "+r" (di), "=r" (si), "=r" (dx),
> >  "=r" (cx), "=r" (r8), "=r" (r9),
> >  "=r" (r10), "=r" (r11), "=a" (ret_)
> >: [addr] "i" (&(func)), "g" (func)
> >: "memory" );
> > }
> > 
> > void foo(unsigned int a)
> > {
> > bar(a);
> > }
> > 
> > Clang generates the following code:
> > 
> > func:   # @func
> > movl%edi, %eax
> > retq
> > foo:# @foo
> > callq   func
> > retq
> > 
> > Note the truncation of the unsigned int parameter 'a' of foo() to uint8_t 
> > when
> > passed into bar() is lost.
> > 
> > The above can be worked around by using an union when defining the register
> > variables, so that `di` becomes:
> > 
> > register union {
> > uint8_t e;
> > unsigned long r;
> > } di asm("rdi") = { .e = b };
> > 
> > Which results in following code generated for `foo()`:
> > 
> > foo:# @foo
> > movzbl  %dil, %edi
> > callq   func
> > retq
> > 
> > So the truncation is not longer lost.
> 
> But how do you explain this behavior? I see absolutely no reason why filling
> the one union field should lead to zero-extension. If I'm not mistaken the
> language allows the rest of the union to retain undefined contents. So to me
> this looks like you're converting something that failed to build due to a
> (presumed) bug in Clang to something that any compiler would be permitted to
> translate to other than what we want.

Oh, right, I was expecting the compiler to zero extend it, confabulating
how unmentioned fields are initialized in structs.

However, if as mentioned in the psABI thread, the callee is required
to do any zero extension as necessary, using the union shouldn't cause
issues for compilers that implement the ABI properly.

IOW: I don't think the proposed workaround would cause issues for gcc.

Thanks, Roger.

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

[Jan Beulich]

On 21.02.2024 18:03, Roger Pau Monne wrote:
> The current code for alternative calls uses the caller parameter types as the
> types for the register variables that serve as function parameters:
> 
> uint8_t foo;
> [...]
> alternative_call(myfunc, foo);
> 
> Would expand roughly into:
> 
> register unint8_t a1_ asm("rdi") = foo;
> register unsigned long a2_ asm("rsi");
> [...]
> asm volatile ("call *%c[addr](%%rip)"...);
> 
> However under certain circumstances clang >= 16.0.0 with -O2 can generate
> incorrect code,

Considering that the related (wider) ABI issue looks to also be present on
Clang5, is the more specific issue here really limited to >= 16?

> given the following example:
> 
> unsigned int func(uint8_t t)
> {
> return t;
> }
> 
> static void bar(uint8_t b)
> {
> int ret_;
> register uint8_t di asm("rdi") = b;
> register unsigned long si asm("rsi");
> register unsigned long dx asm("rdx");
> register unsigned long cx asm("rcx");
> register unsigned long r8 asm("r8");
> register unsigned long r9 asm("r9");
> register unsigned long r10 asm("r10");
> register unsigned long r11 asm("r11");
> 
> asm volatile ( "call %c[addr]"
>: "+r" (di), "=r" (si), "=r" (dx),
>  "=r" (cx), "=r" (r8), "=r" (r9),
>  "=r" (r10), "=r" (r11), "=a" (ret_)
>: [addr] "i" (&(func)), "g" (func)
>: "memory" );
> }
> 
> void foo(unsigned int a)
> {
> bar(a);
> }
> 
> Clang generates the following code:
> 
> func:   # @func
> movl%edi, %eax
> retq
> foo:# @foo
> callq   func
> retq
> 
> Note the truncation of the unsigned int parameter 'a' of foo() to uint8_t when
> passed into bar() is lost.
> 
> The above can be worked around by using an union when defining the register
> variables, so that `di` becomes:
> 
> register union {
> uint8_t e;
> unsigned long r;
> } di asm("rdi") = { .e = b };
> 
> Which results in following code generated for `foo()`:
> 
> foo:# @foo
> movzbl  %dil, %edi
> callq   func
> retq
> 
> So the truncation is not longer lost.

But how do you explain this behavior? I see absolutely no reason why filling
the one union field should lead to zero-extension. If I'm not mistaken the
language allows the rest of the union to retain undefined contents. So to me
this looks like you're converting something that failed to build due to a
(presumed) bug in Clang to something that any compiler would be permitted to
translate to other than what we want.

In this context note in particular that the spec distinguishes aggregates
from unions, and the clause regarding filling unmentioned fields is limited
to aggregates:

"If there are fewer initializers in a brace-enclosed list than there are
 elements or members of an aggregate, or fewer characters in a string
 literal used to initialize an array of known size than there are elements
 in the array, the remainder of the aggregate shall be initialized
 implicitly the same as objects that have static storage duration."

Which makes sense, as a union initializer shall mention a single of the
members only anyway.

All of this of course doesn't invalidate your approach as a possible
workaround, but it then needs limiting to Clang versions where we are sure
the more strict behavior than demanded by the standard actually applies.

Jan

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

[Anthony PERARD]

On Thu, Feb 22, 2024 at 8:28 AM Juergen Gross  wrote:
> > Tests which did not succeed and are blocking,
> > including tests which could not be run:
> >   build-arm64-pvops 6 kernel-build   fail in 184721 REGR. vs. 
> > 184719
>
> Log says:
>
> gcc: internal compiler error: Segmentation fault signal terminated program cc1
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See  for instructions.
> make[5]: *** [scripts/Makefile.build:243: drivers/iio/adc/max9611.o] Error 4
> make[5]: *** Waiting for unfinished jobs

Yeah, that happens from time to time. My plan to fix this is to
upgrade to a newer version of Debian, it just takes time.

Cheers,

-- 
Anthony PERARD

Re: [PATCH 2/2] x86/hpet: use an atomic add instead of a cmpxchg loop

[Jan Beulich]

On 22.02.2024 10:05, Roger Pau Monne wrote:
> The usage of a cmpxchg loop in hpet_get_channel() is unnecessary, as the same
> can be achieved with an atomic increment, which is both simpler to read, and
> avoid any need for a loop.
> 
> Note there can be a small divergence in the channel returned if next_channel
> overflows, but returned channel will always be in the [0, num_hpets_used)
> range, and that's fine for the purpose of balancing HPET channels across CPUs.
> This is also theoretical, as there's no system currently with 2^32 CPUs (as
> long as next_channel is 32bit width).

The code change looks good to me, but I fail to see the connection to
2^32 CPUs. So it feels like I'm missing something, in which case I'd
rather not offer any R-b.

Jan

> Signed-of-by: Roger Pau Monné 
> ---
>  xen/arch/x86/hpet.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
> index d982b0f6b2c9..4777dc859d96 100644
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -458,11 +458,7 @@ static struct hpet_event_channel 
> *hpet_get_channel(unsigned int cpu)
>  if ( num_hpets_used >= nr_cpu_ids )
>  return _events[cpu];
>  
> -do {
> -next = next_channel;
> -if ( (i = next + 1) == num_hpets_used )
> -i = 0;
> -} while ( cmpxchg(_channel, next, i) != next );
> +next = arch_fetch_and_add(_channel, 1) % num_hpets_used;
>  
>  /* try unused channel first */
>  for ( i = next; i < next + num_hpets_used; i++ )

Re: [PATCH 1/2] x86/memsharing: use an atomic add instead of a cmpxchg loop

[Jan Beulich]

On 22.02.2024 10:05, Roger Pau Monne wrote:
> The usage of a cmpxchg loop in get_next_handle() is unnecessary, as the same
> can be achieved with an atomic increment, which is both simpler to read, and
> avoid any need for a loop.
> 
> The cmpxchg usage is likely a remnant of 32bit support, which didn't have an
> instruction to do an atomic 64bit add, and instead a cmpxchg had to be used.
> 
> Suggested-by: Andrew Cooper 
> Signed-of-by: Roger Pau Monné 

Reviewed-by: Jan Beulich 
albeit ...

> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -179,13 +179,7 @@ static void mem_sharing_page_unlock(struct page_info *pg)
>  
>  static shr_handle_t get_next_handle(void)
>  {
> -/* Get the next handle get_page style */
> -uint64_t x, y = next_handle;
> -do {
> -x = y;
> -}
> -while ( (y = cmpxchg(_handle, x, x + 1)) != x );
> -return x + 1;
> +return arch_fetch_and_add(_handle, 1) + 1;
>  }

... the adding of 1 here is a little odd when taken together with
next_handle's initializer. Tamas, you've not written that code, but do
you have any thoughts towards the possible removal of either the
initializer or the adding here? Plus that variable of course could
very well do with moving into this function.

Jan

Re: Stats on Xen tarball downloads

[Juergen Gross]

On 22.02.24 10:49, Roger Pau Monné wrote:

On Wed, Feb 21, 2024 at 10:53:49PM +, Julien Grall wrote:

Hi George,

On 21/02/2024 02:55, George Dunlap wrote:

On Mon, Feb 19, 2024 at 6:38 PM Jan Beulich  wrote:


On 19.02.2024 11:31, Roger Pau Monné wrote:

On Mon, Feb 19, 2024 at 06:01:54PM +0800, George Dunlap wrote:

One of the questions we had with respect to changing our release
practice (for instance, making the process more light-weight so that
we could do a point release after every XSA) was, "How many people are
actually using the tarballs?"


What would this more lightweight process involve from a downstream
PoV?  IOW: in what would the contents of the tarball change compared
to the current releases?


  From all prior discussion my conclusion was "no tarball at all".


Or at very least, the tarball would be a simple `git archive` of a
release tag.   Right now the tarball creation has a number of
annoyingly manual parts about it.

At the moment we have the following steps:

1) Checkout tag
2) Create the tarball
3) Check the source tarball can build
4) Sign the tarball
5) Upload it

I managed to script it so I have only two commands to execute (mostly
because I build and sign on a different host).

AFAIU, your command 'git archive' will only replace 2. Am I correct? If so,
it is not entirely clear how your proposal is going to make it better.


IMO building for release tarballs is easier than from a git checkout
(or archive).  It's a bit annoying to have to pre-download the
external project sources, now even more as QEMU is using git
submodules.

Most distro binary builders have infrastructure to deal with all this,
but requires a bit more logic in the recipe than a plain just fetch a
tarball and build from it.


I have an unfinished patch series lying around doing the download steps
_before_ starting the build. This includes make targets for downloading
the required components, or all components if configure should be called
afterwards.

Creating the tarball after having downloaded all components is trivial.

There are a few bugs in the series I didn't have time yet to fix. If someone
is interested in working on it, I can post the series.


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH 4/6] nestedsvm: Disable TscRateMSR

[Jan Beulich]

On 22.02.2024 10:30, George Dunlap wrote:
> On Wed, Feb 21, 2024 at 6:52 PM Jan Beulich  wrote:
 But then of course Andrew may know of reasons why all of this is done
 in calculate_host_policy() in the first place, rather than in HVM
 policy calculation.
>>>
>>> It sounds like maybe you're confusing host_policy with
>>> x86_capabilities?  From what I can tell:
>>>
>>> *  the "basic" cpu_has_X macros resolve to boot_cpu_has(), which
>>> resolves to cpu_has(_cpu_data, ...), which is completely
>>> independent of the cpu-policy.c:host_cpu_policy
>>>
>>> * cpu-policy.c:host_cpu_policy only affects what is advertised to
>>> guests, via {pv,hvm}_cpu_policy and featureset bits.  Most notably a
>>> quick skim doesn't show any mechanism by which host_cpu_policy could
>>> affect what features Xen itself decides to use.
>>
>> I'm not mixing the two, no; the two are still insufficiently disentangled.
>> There's really no reason (long term) to have both host policy and
>> x86_capabilities. Therefore I'd prefer if new code (including a basically
>> fundamental re-write as is going to be needed for nested) to avoid
>> needlessly further extending x86_capabilities. Unless of course there's
>> something fundamentally wrong with eliminating the redundancy, which
>> likely Andrew would be in the best position to point out.
> 
> So I don't know the history of how things got to be the way they are,
> nor really much about the code but what I've gathered from skimming
> through while creating this patch series.  But from that impression,
> the only issue I really see with the current code is the confusing
> naming.  The cpufeature.h code has this nice infrastructure to allow
> you to, for instance, enable or disable certain bits on the
> command-line; and the interface for querying all the different bits of
> functionality is all nicely put in one place.  Moving the
> svm_feature_flags into x86_capabilities would immediately allow SVM to
> take advantage of this infrastructure; it's not clear to me how this
> would be "needless".
> 
> Furthermore, it looks to me like host_cpu_policy is used as a starting
> point for generating pv_cpu_policy and hvm_cpu_policy, both of which
> are only used for guest cpuid generation.  Given that the format of
> those policies is fixed, and there's a lot of "copy this bit from the
> host policy wholesale", it seems like no matter what, you'd want a
> host_cpu_policy.
> 
> And in any case -- all that is kind of moot.  *Right now*,
> host_cpu_policy is only used for guest cpuid policy creation; *right
> now*, the nested virt features of AMD are handled in the
> host_cpu_policy; *right now*, we're advertising to guests bits which
> are not properly virtualized; *right now* these bits are actually set
> unconditionally, regardless of whether they're even available on the
> hardware; *right now*, Xen uses svm_feature_flags to determine its own
> use of TscRateMSR; so *right now*, removing this bit from
> host_cpu_policy won't prevent Xen from using TscRateMSR itself.
> 
> (Unless my understanding of the code is wrong, in which case I'd
> appreciate a correction.)

There's nothing wrong afaics, just missing at least one aspect: Did you
see all the featureset <-> policy conversions in cpu-policy.c? That (to
me at least) clearly is a sign of unnecessary duplication of the same
data. This goes as far as seeding the host policy from the raw one, just
to then immediately run x86_cpu_featureset_to_policy(), thus overwriting
a fair part of what was first taken from the raw policy. That's necessary
right now, because setup_{force,clear}_cpu_cap() act on
boot_cpu_data.x86_capability[], not the host policy.

As to the "needless" further up, it's only as far as moving those bits
into x86_capability[] would further duplicate information, rather than
(for that piece at least) putting them into the policies right away. But
yes, if the goal is to have setup_{force,clear}_cpu_cap() be able to
control those bits as well, then going the intermediate step would be
unavoidable at this point in time.

Jan

Re: Stats on Xen tarball downloads

[Roger Pau Monné]

On Wed, Feb 21, 2024 at 10:53:49PM +, Julien Grall wrote:
> Hi George,
> 
> On 21/02/2024 02:55, George Dunlap wrote:
> > On Mon, Feb 19, 2024 at 6:38 PM Jan Beulich  wrote:
> > > 
> > > On 19.02.2024 11:31, Roger Pau Monné wrote:
> > > > On Mon, Feb 19, 2024 at 06:01:54PM +0800, George Dunlap wrote:
> > > > > One of the questions we had with respect to changing our release
> > > > > practice (for instance, making the process more light-weight so that
> > > > > we could do a point release after every XSA) was, "How many people are
> > > > > actually using the tarballs?"
> > > > 
> > > > What would this more lightweight process involve from a downstream
> > > > PoV?  IOW: in what would the contents of the tarball change compared
> > > > to the current releases?
> > > 
> > >  From all prior discussion my conclusion was "no tarball at all".
> > 
> > Or at very least, the tarball would be a simple `git archive` of a
> > release tag.   Right now the tarball creation has a number of
> > annoyingly manual parts about it.
> At the moment we have the following steps:
> 
> 1) Checkout tag
> 2) Create the tarball
> 3) Check the source tarball can build
> 4) Sign the tarball
> 5) Upload it
> 
> I managed to script it so I have only two commands to execute (mostly
> because I build and sign on a different host).
> 
> AFAIU, your command 'git archive' will only replace 2. Am I correct? If so,
> it is not entirely clear how your proposal is going to make it better.

IMO building for release tarballs is easier than from a git checkout
(or archive).  It's a bit annoying to have to pre-download the
external project sources, now even more as QEMU is using git
submodules.

Most distro binary builders have infrastructure to deal with all this,
but requires a bit more logic in the recipe than a plain just fetch a
tarball and build from it.

Thanks, Roger.

Re: [PATCH 4/6] nestedsvm: Disable TscRateMSR

[George Dunlap]

On Wed, Feb 21, 2024 at 6:52 PM Jan Beulich  wrote:
> >> But then of course Andrew may know of reasons why all of this is done
> >> in calculate_host_policy() in the first place, rather than in HVM
> >> policy calculation.
> >
> > It sounds like maybe you're confusing host_policy with
> > x86_capabilities?  From what I can tell:
> >
> > *  the "basic" cpu_has_X macros resolve to boot_cpu_has(), which
> > resolves to cpu_has(_cpu_data, ...), which is completely
> > independent of the cpu-policy.c:host_cpu_policy
> >
> > * cpu-policy.c:host_cpu_policy only affects what is advertised to
> > guests, via {pv,hvm}_cpu_policy and featureset bits.  Most notably a
> > quick skim doesn't show any mechanism by which host_cpu_policy could
> > affect what features Xen itself decides to use.
>
> I'm not mixing the two, no; the two are still insufficiently disentangled.
> There's really no reason (long term) to have both host policy and
> x86_capabilities. Therefore I'd prefer if new code (including a basically
> fundamental re-write as is going to be needed for nested) to avoid
> needlessly further extending x86_capabilities. Unless of course there's
> something fundamentally wrong with eliminating the redundancy, which
> likely Andrew would be in the best position to point out.

So I don't know the history of how things got to be the way they are,
nor really much about the code but what I've gathered from skimming
through while creating this patch series.  But from that impression,
the only issue I really see with the current code is the confusing
naming.  The cpufeature.h code has this nice infrastructure to allow
you to, for instance, enable or disable certain bits on the
command-line; and the interface for querying all the different bits of
functionality is all nicely put in one place.  Moving the
svm_feature_flags into x86_capabilities would immediately allow SVM to
take advantage of this infrastructure; it's not clear to me how this
would be "needless".

Furthermore, it looks to me like host_cpu_policy is used as a starting
point for generating pv_cpu_policy and hvm_cpu_policy, both of which
are only used for guest cpuid generation.  Given that the format of
those policies is fixed, and there's a lot of "copy this bit from the
host policy wholesale", it seems like no matter what, you'd want a
host_cpu_policy.

And in any case -- all that is kind of moot.  *Right now*,
host_cpu_policy is only used for guest cpuid policy creation; *right
now*, the nested virt features of AMD are handled in the
host_cpu_policy; *right now*, we're advertising to guests bits which
are not properly virtualized; *right now* these bits are actually set
unconditionally, regardless of whether they're even available on the
hardware; *right now*, Xen uses svm_feature_flags to determine its own
use of TscRateMSR; so *right now*, removing this bit from
host_cpu_policy won't prevent Xen from using TscRateMSR itself.

(Unless my understanding of the code is wrong, in which case I'd
appreciate a correction.)

If at some point in the future x86_capabilities and host_cpu_policy
were merged somehow, whoever did the merging would have to untangle
the twiddling of these bits anyway.  What I'm changing in this patch
wouldn't make that any harder.

> > Not sure exactly why the nested virt stuff is done at the
> > host_cpu_policy level rather than the hvm_cpu_policy level, but since
> > that's where it is, that's where we need to change it.
> >
> > FWIW, as I said in response to your comment on 2/6, it would be nicer
> > if we moved svm_feature_flags into the "capabilities" section; but
> > that's a different set of work.
>
> Can as well reply here then, rather than there: If the movement from
> host to HVM policy was done first, the patch here could more sanely go
> on top, and patch 2 could then also go on top, converting the separate
> variable to host policy accesses, quite possibly introducing a similar
> wrapper as you introduce there right now.
>
> But no, I'm not meaning to make this a requirement; this would merely be
> an imo better approach. My ack there stands, in case you want to keep
> (and commit) the change as is.

I mean, I don't mind doing a bit more prep work, if I know that's the
direction we want to go in.  "Actually, since you're doing a bit of
clean-up anyway -- right now host_cpu_policy is only used to calculate
guest policy, but we'd like to move over to being the Source of Truth
for the host instead of x86_capabilities.  While you're here, would
you mind moving the nested virt policy stuff into hvm_cpu_policy
instead?"

I certainly wouldn't want to move svm_feature_flags into
host_cpu_policy while it's still got random other "guest-only" policy
bits in it; and auditing it for such policy bits is out of the scope
of this work.

 -George

[PATCH 0/2] xen/x86: cmpxchg cleanup

[Roger Pau Monne]

Hello,

Following series replace a couple of cmpxchg loops with an atomic inc.
The usage of such loops probably dates back to 32bit support, which
didn't have an instruction to do an atomic 64bit addition.

Thanks, Roger.

Roger Pau Monne (2):
  x86/memsharing: use an atomic add instead of a cmpxchg loop
  x86/hpet: use an atomic add instead of a cmpxchg loop

 xen/arch/x86/hpet.c   | 6 +-
 xen/arch/x86/mm/mem_sharing.c | 8 +---
 2 files changed, 2 insertions(+), 12 deletions(-)

-- 
2.43.0

[PATCH 2/2] x86/hpet: use an atomic add instead of a cmpxchg loop

[Roger Pau Monne]

The usage of a cmpxchg loop in hpet_get_channel() is unnecessary, as the same
can be achieved with an atomic increment, which is both simpler to read, and
avoid any need for a loop.

Note there can be a small divergence in the channel returned if next_channel
overflows, but returned channel will always be in the [0, num_hpets_used)
range, and that's fine for the purpose of balancing HPET channels across CPUs.
This is also theoretical, as there's no system currently with 2^32 CPUs (as
long as next_channel is 32bit width).

Signed-of-by: Roger Pau Monné 
---
 xen/arch/x86/hpet.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index d982b0f6b2c9..4777dc859d96 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -458,11 +458,7 @@ static struct hpet_event_channel 
*hpet_get_channel(unsigned int cpu)
 if ( num_hpets_used >= nr_cpu_ids )
 return _events[cpu];
 
-do {
-next = next_channel;
-if ( (i = next + 1) == num_hpets_used )
-i = 0;
-} while ( cmpxchg(_channel, next, i) != next );
+next = arch_fetch_and_add(_channel, 1) % num_hpets_used;
 
 /* try unused channel first */
 for ( i = next; i < next + num_hpets_used; i++ )
-- 
2.43.0

[PATCH 1/2] x86/memsharing: use an atomic add instead of a cmpxchg loop

[Roger Pau Monne]

The usage of a cmpxchg loop in get_next_handle() is unnecessary, as the same
can be achieved with an atomic increment, which is both simpler to read, and
avoid any need for a loop.

The cmpxchg usage is likely a remnant of 32bit support, which didn't have an
instruction to do an atomic 64bit add, and instead a cmpxchg had to be used.

Suggested-by: Andrew Cooper 
Signed-of-by: Roger Pau Monné 
---
 xen/arch/x86/mm/mem_sharing.c | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 4f810706a315..fe299a2bf9aa 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -179,13 +179,7 @@ static void mem_sharing_page_unlock(struct page_info *pg)
 
 static shr_handle_t get_next_handle(void)
 {
-/* Get the next handle get_page style */
-uint64_t x, y = next_handle;
-do {
-x = y;
-}
-while ( (y = cmpxchg(_handle, x, x + 1)) != x );
-return x + 1;
+return arch_fetch_and_add(_handle, 1) + 1;
 }
 
 static atomic_t nr_saved_mfns   = ATOMIC_INIT(0);
-- 
2.43.0

Re: [XEN PATCH v2] automation/eclair_analysis: deviate certain macros for Rule 20.12

[Nicola Vetrini]

On 2024-02-15 14:06, Nicola Vetrini wrote:

Certain macros are allowed to violate the Rule, since their meaning and
intended use is well-known to all Xen developers.

Variadic macros that rely on the GCC extension for removing a trailing
comma when token pasting the variable argument are similarly
well-understood and therefore allowed.

No functional change.

Signed-off-by: Nicola Vetrini 
---
Changes in v2:
- Restrict deviation for GENERATE_CASE to vcpreg.c.
- Improve deviation justifications.
---
 .../eclair_analysis/ECLAIR/deviations.ecl | 20 +
 docs/misra/deviations.rst | 22 +++
 2 files changed, 42 insertions(+)



Ping?

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

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

[Roger Pau Monné]

On Wed, Feb 21, 2024 at 06:03:31PM +0100, Roger Pau Monne wrote:
> The current code for alternative calls uses the caller parameter types as the
> types for the register variables that serve as function parameters:
> 
> uint8_t foo;
> [...]
> alternative_call(myfunc, foo);
> 
> Would expand roughly into:
> 
> register unint8_t a1_ asm("rdi") = foo;
> register unsigned long a2_ asm("rsi");
> [...]
> asm volatile ("call *%c[addr](%%rip)"...);
> 
> However under certain circumstances clang >= 16.0.0 with -O2 can generate
> incorrect code, given the following example:
> 
> unsigned int func(uint8_t t)
> {
> return t;
> }
> 
> static void bar(uint8_t b)
> {
> int ret_;
> register uint8_t di asm("rdi") = b;
> register unsigned long si asm("rsi");
> register unsigned long dx asm("rdx");
> register unsigned long cx asm("rcx");
> register unsigned long r8 asm("r8");
> register unsigned long r9 asm("r9");
> register unsigned long r10 asm("r10");
> register unsigned long r11 asm("r11");
> 
> asm volatile ( "call %c[addr]"
>: "+r" (di), "=r" (si), "=r" (dx),
>  "=r" (cx), "=r" (r8), "=r" (r9),
>  "=r" (r10), "=r" (r11), "=a" (ret_)
>: [addr] "i" (&(func)), "g" (func)
>: "memory" );
> }
> 
> void foo(unsigned int a)
> {
> bar(a);
> }
> 
> Clang generates the following code:
> 
> func:   # @func
> movl%edi, %eax
> retq
> foo:# @foo
> callq   func
> retq
> 
> Note the truncation of the unsigned int parameter 'a' of foo() to uint8_t when
> passed into bar() is lost.
> 
> The above can be worked around by using an union when defining the register
> variables, so that `di` becomes:
> 
> register union {
> uint8_t e;
> unsigned long r;
> } di asm("rdi") = { .e = b };
> 
> Which results in following code generated for `foo()`:
> 
> foo:# @foo
> movzbl  %dil, %edi
> callq   func
> retq
> 
> So the truncation is not longer lost.
> 

This is missing:

Reported-by: Matthew Grooms 
Link: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=277200
Link: https://github.com/llvm/llvm-project/issues/82598

Last one is the bug report against llvm.

Thanks, Roger.

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

[Juergen Gross]

On 22.02.24 09:21, osstest service owner wrote:

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

Regressions :-(

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


Log says:

gcc: internal compiler error: Segmentation fault signal terminated program cc1
Please submit a full bug report,
with preprocessed source if appropriate.
See  for instructions.
make[5]: *** [scripts/Makefile.build:243: drivers/iio/adc/max9611.o] Error 4
make[5]: *** Waiting for unfinished jobs


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

[linux-linus test] 184722: regressions - FAIL

[osstest service owner]

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

Regressions :-(

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

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

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

version targeted for testing:
 linux39133352cbed6626956d38ed72012f49b0421e7b
baseline version: