Re: [RFC KERNEL PATCH v4 3/3] PCI/sysfs: Add gsi sysfs for pci_dev

[Chen, Jiqian]

Hi Bjorn,
Looking forward to getting your more inputs and suggestions.
It seems /sys/bus/acpi/devices/PNP0A03:00/ is not a good place to create gsi 
sysfs.

On 2024/2/15 16:37, Roger Pau Monné wrote:
> On Mon, Feb 12, 2024 at 01:18:58PM -0600, Bjorn Helgaas wrote:
>> On Mon, Feb 12, 2024 at 10:13:28AM +0100, Roger Pau Monné wrote:
>>> On Fri, Feb 09, 2024 at 03:05:49PM -0600, Bjorn Helgaas wrote:
 On Thu, Feb 01, 2024 at 09:39:49AM +0100, Roger Pau Monné wrote:
> On Wed, Jan 31, 2024 at 01:00:14PM -0600, Bjorn Helgaas wrote:
>> On Wed, Jan 31, 2024 at 09:58:19AM +0100, Roger Pau Monné wrote:
>>> On Tue, Jan 30, 2024 at 02:44:03PM -0600, Bjorn Helgaas wrote:
 On Tue, Jan 30, 2024 at 10:07:36AM +0100, Roger Pau Monné wrote:
> On Mon, Jan 29, 2024 at 04:01:13PM -0600, Bjorn Helgaas wrote:
>> On Thu, Jan 25, 2024 at 07:17:24AM +, Chen, Jiqian wrote:
>>> On 2024/1/24 00:02, Bjorn Helgaas wrote:
 On Tue, Jan 23, 2024 at 10:13:52AM +, Chen, Jiqian wrote:
> On 2024/1/23 07:37, Bjorn Helgaas wrote:
>> On Fri, Jan 05, 2024 at 02:22:17PM +0800, Jiqian Chen wrote:
>>> There is a need for some scenarios to use gsi sysfs.
>>> For example, when xen passthrough a device to dumU, it will
>>> use gsi to map pirq, but currently userspace can't get gsi
>>> number.
>>> So, add gsi sysfs for that and for other potential scenarios.
> ...

>> I don't know enough about Xen to know why it needs the GSI in
>> userspace.  Is this passthrough brand new functionality that 
>> can't be
>> done today because we don't expose the GSI yet?
>>
>> I assume this must be new functionality, i.e., this kind of
>> passthrough does not work today, right?
>>
> has ACPI support and is responsible for detecting and controlling
> the hardware, also it performs privileged operations such as the
> creation of normal (unprivileged) domains DomUs. When we give to a
> DomU direct access to a device, we need also to route the physical
> interrupts to the DomU. In order to do so Xen needs to setup and 
> map
> the interrupts appropriately.

 What kernel interfaces are used for this setup and mapping?
>>>
>>> For passthrough devices, the setup and mapping of routing physical
>>> interrupts to DomU are done on Xen hypervisor side, hypervisor only
>>> need userspace to provide the GSI info, see Xen code:
>>> xc_physdev_map_pirq require GSI and then will call hypercall to pass
>>> GSI into hypervisor and then hypervisor will do the mapping and
>>> routing, kernel doesn't do the setup and mapping.
>>
>> So we have to expose the GSI to userspace not because userspace 
>> itself
>> uses it, but so userspace can turn around and pass it back into the
>> kernel?
>
> No, the point is to pass it back to Xen, which doesn't know the
> mapping between GSIs and PCI devices because it can't execute the ACPI
> AML resource methods that provide such information.
>
> The (Linux) kernel is just a proxy that forwards the hypercalls from
> user-space tools into Xen.

 But I guess Xen knows how to interpret a GSI even though it doesn't
 have access to AML?
>>>
>>> On x86 Xen does know how to map a GSI into an IO-APIC pin, in order
>>> configure the RTE as requested.
>>
>> IIUC, mapping a GSI to an IO-APIC pin requires information from the
>> MADT.  So I guess Xen does use the static ACPI tables, but not the AML
>> _PRT methods that would connect a GSI with a PCI device?
>
> Yes, Xen can parse the static tables, and knows the base GSI of
> IO-APICs from the MADT.
>
>> I guess this means Xen would not be able to deal with _MAT methods,
>> which also contains MADT entries?  I don't know the implications of
>> this -- maybe it means Xen might not be able to use with hot-added
>> devices?
>
> It's my understanding _MAT will only be present on some very specific
> devices (IO-APIC or CPU objects).  Xen doesn't support hotplug of
> IO-APICs, but hotplug of CPUs should in principle be supported with
> cooperation from the control domain OS (albeit it's not something that
> we tests on our CI).  I don't expect however that a CPU object _MAT
> method will return IO APIC entries.
>
>> The tables (including DSDT and SSDTS that contain the AML) are exposed
>> to userspace via /sys/firmware/acpi/tables/, but of course that
>> doesn't mean Xen knows how to interpret the AML, and even if it did,
>> Xen probably wouldn't be able to *evaluate* it since that could

Re: [XEN PATCH 04/10] xen/public: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 17:49, Nicola Vetrini wrote:
> On 2024-02-29 17:40, Jan Beulich wrote:
>> On 29.02.2024 16:27, Nicola Vetrini wrote:
>>> --- a/xen/include/public/xen.h
>>> +++ b/xen/include/public/xen.h
>>> @@ -988,7 +988,7 @@ typedef struct {
>>>((b) >>  8) & 0xFF, ((b) >>  0) & 0xFF, 
>>>   \
>>>((c) >>  8) & 0xFF, ((c) >>  0) & 0xFF, 
>>>   \
>>>((d) >>  8) & 0xFF, ((d) >>  0) & 0xFF, 
>>>   \
>>> -e1, e2, e3, e4, e5, e6}}
>>> +(e1), (e2), (e3), (e4), (e5), (e6)}}
>>
>> Why? Wasn't it agreed already that long macro arguments passed on
>> (no matter whether to a function, a macro, or like used here) don't
>> need parenthesizing?
>>
> 
> That applies to all outermost macro invocations, but not to the 
> innermost one. If you want also aggregate initalizers to be deviated, 
> that could be done (provided that the macro arg is not included in some 
> expression, such as "{..., e1 + 1, ...}"

Sure, this obviously needs to be "{..., (e1) + 1, ...}" then. But yes,
the full scope of the underlying pattern ought to be excluded from
needing (pointless) parentheses added. Even in a plain comma expression
you don't need them - to pass in a comma expression the invocation site
of such a macro would then need to parenthesize the respective operand
(or else it would be two separate operands).

The one case we're leaving aside here anyway is use of odd things like

#define M a, b

and then passing M into another macro.

Jan

Re: [PATCH v2 3/3] docs/misra/rules.rst: add rule 14.4

[Bertrand Marquis]

Hi Stefano,


> On 13 Feb 2024, at 23:33, Stefano Stabellini  
> wrote:
> 
> Signed-off-by: Stefano Stabellini 

Coherent with what was discussed during the Misra meeting so:

Acked-by: Bertrand Marquis 

Cheers
Bertrand

> ---
> docs/misra/rules.rst | 9 +
> 1 file changed, 9 insertions(+)
> 
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> index 931158b354..3e6f94d7bd 100644
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -468,6 +468,15 @@ maintainers if you want to suggest a change.
> 
>while(0) and while(1) and alike are allowed.
> 
> +   * - `Rule 14.4 
> `_
> + - Required
> + - The controlling expression of an if-statement and the controlling
> +   expression of an iteration-statement shall have essentially
> +   Boolean type
> + - Automatic conversions of integer types to bool are permitted.
> +   Automatic conversions of pointer types to bool are permitted.
> +   This rule still applies to enum types.
> +
>* - `Rule 16.3 
> `_
>  - Required
>  - An unconditional break statement shall terminate every
> -- 
> 2.25.1
> 
> 

[linux-linus test] 184825: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 8 xen-boot fail REGR. 
vs. 184816
 test-amd64-amd64-xl-qemuu-debianhvm-amd64  8 xen-bootfail REGR. vs. 184816
 test-amd64-amd64-xl   8 xen-boot fail REGR. vs. 184816
 test-amd64-amd64-xl-qemut-debianhvm-i386-xsm  8 xen-boot fail REGR. vs. 184816
 test-amd64-amd64-freebsd11-amd64  8 xen-boot fail REGR. vs. 184816
 test-amd64-amd64-pygrub   8 xen-boot fail REGR. vs. 184816
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 8 xen-boot fail REGR. vs. 
184816
 test-amd64-amd64-xl-qemuu-ovmf-amd64  8 xen-boot fail REGR. vs. 184816
 build-arm64-xsm   6 xen-buildfail REGR. vs. 184816

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

version targeted for testing:
 linux87adedeba51a822533649b143232418b9e26d08b
baseline version:
 linux805d849d7c3cc1f38efefd48b2480d62b7b5dcb7

Last test of basis   184816  2024-02-29 05:22:18 Z1 days
Testing same since   184825  2024-02-29 21:14:29 Z0 days1 attempts


People who touched revisions under test:
  Alexander Ofitserov 
  Amritha Nambiar 
  Andre Werner 
  Arkadiusz 

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

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-xl-vhd  13 guest-start fail pass in 184827-retest

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

Last test of basis   184811  2024-02-28 19:28:00 Z

Re: [PATCH] xen/common: Do not allocate magic pages 1:1 for direct mapped domains

[Henry Wang]

Hi Julien,

On 2/28/2024 8:27 PM, Julien Grall wrote:

Hi Henry,
...here basically means we want to do the finding of the unused 
region in toolstack. Since currently what we care about is only a 
couple of pages instead of the whole memory map, could it be possible 
that we do the opposite: in alloc_xs_page(), we issue a domctl 
hypercall to Xen and do the finding work in Xen and return with the 
found gfn? Then the page can be mapped by populate_physmap() from 
alloc_xs_page() and used for XenStore.


I know that DOMCTL hypercalls are not stable. But I am not overly 
happy with creating an hypercall which is just "fetch the magic 
regions". I think it need to be a more general one that would expose 
all the regions.


Also, you can't really find the magic regions when the hypercall is 
done as we don't have the guest memory layout. This will need to be 
done in advance.


Overall, I think it would be better if we provide an hypercall listing 
the regions currently occupied (similar to e820). One will have the 
type "magic pages".


Would below design make sense to you:

(1) Similarly as the e820, we can firstly introduce some data structures 
in struct arch_domain:


#define MEM_REGIONS_MAX 4

// For now we only care the magic regions, but in the future this can be 
easily extended with other types such as RAM, MMIO etc.

enum mem_region_type {
    MEM_REGION_MAGIC,
};

struct mem_region {
    paddr_t start;
    paddr_t size;
    enum mem_region_type type;
};

struct domain_mem_map {
    unsigned int nr_mem_regions;
    struct mem_region region[MEM_REGIONS_MAX];
};

struct arch_domain {
...
    struct domain_mem_map mem_map;
};

(2) Then in domain creation time, for normal and static non-directmapped 
Dom0less DomU, set d->arch.mem_map.region[0].start = GUEST_MAGIC_BASE 
and the size to 4 pages. For static and directmapped Dom0less DomU, find 
a free region of 4 pages for magic pages. The finding of a free region 
can reuse the approach for extended region and be called before 
make_hypervisor_node(), and when make_hypervisor_node() is called. We 
carve the magic pages out from the actual extended region.


(3) Add a new hypercall XENMEM_memory_map_domid (or a domctl). Input 
will be domid and output will be the memory map of the domain recorded 
in struct arch_domain.


(4) In alloc_xs_page() and alloc_magic_pages, replace the hardcoded base 
address with the new address found by the hypercall.


Kind regards,
Henry

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

[osstest service owner]

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

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  4cac80e22600d5a38d77c65e9a6507c752efc155
baseline version:
 xen  635dd1120a01961a39dce6ad3f09692681379378

Last test of basis   184823  2024-02-29 17:00:27 Z0 days
Testing same since   184826  2024-03-01 00:00:25 Z0 days1 attempts


People who touched revisions under test:
  Federico Serafini 
  Julien Grall 
  Stefano Stabellini 
  Stefano Stabellini 

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



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

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

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

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


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   635dd1120a..4cac80e226  4cac80e22600d5a38d77c65e9a6507c752efc155 -> smoke

Re: [PATCH 2/2] libxl: devd: Spawn QEMU for 9pfs

[Jason Andryuk]

On Wed, Feb 28, 2024 at 11:01 AM Anthony PERARD
 wrote:
>
> On Tue, Jan 09, 2024 at 12:05:40PM -0500, Jason Andryuk wrote:
> > @@ -3430,7 +3431,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, 
> > libxl__dm_spawn_state *dmss)
> >   * because we will call this from unprivileged driver domains,
> >   * so save it in the current domain libxl private dir.
> >   */
> > -dmss->spawn.pidpath = GCSPRINTF("libxl/%u/qdisk-backend-pid", domid);
> > +dmss->spawn.pidpath = GCSPRINTF("libxl/%u/qemu-xenpv-backend-pid", 
> > domid);
>
> This path is documented in "docs/misc/xenstore-paths.pandoc", I'm not
> sure it's such a good idea to change it.
>
> But maybe it's ok to change the path because xl devd is expected add
> then remove this path? And that pid isn't going to be useful to anything
> else?
>
> I'd rather not change the path, but if you still think it's ok, we can
> go for it. Thought?

I changed the path to keep it consistent with the internal libxl
names.  The node is for libxl's internal use, so it seemed okay to
change.  But it is externally visible, so there could be other
software looking at it.  I'm fine with leaving the xenstore name
unchanged.

(I didn't want to rename all the qdisk stuff, but it seemed like it
should be renamed since it would also handle 9pfs.)

Thanks,
Jason

Re: [PATCH v2 1/3] docs/misra/rules.rst: add rule 16.6 and 20.12

[Stefano Stabellini]

Hi all,

This patch broke gitlab-ci. The jobs failing are the cppcheck jobs.

xen/scripts/xen-analysis.py --run-cppcheck --cppcheck-misra -- -j80
No summary for rule 20.12
WARNING: Can't open 
/builds/xen-project/hardware/xen/xen/drivers/video/font_8x14.c: 'utf-8' codec 
can't decode byte 0x9f in position 7228: invalid start byte
WARNING: Can't open 
/builds/xen-project/hardware/xen/xen/drivers/video/font_8x16.c: 'utf-8' codec 
can't decode byte 0x80 in position 5436: invalid start byte
WARNING: Can't open 
/builds/xen-project/hardware/xen/xen/drivers/video/font_8x8.c: 'utf-8' codec 
can't decode byte 0x80 in position 4410: invalid start byte
ERROR: An error occured when running:
/builds/xen-project/hardware/xen/xen/tools/convert_misra_doc.py -i 
/builds/xen-project/hardware/xen/docs/misra/rules.rst -o 
/builds/xen-project/hardware/xen/xen/cppcheck-misra.txt -j 
/builds/xen-project/hardware/xen/xen/cppcheck-misra.json 

I'll look into it as soon as possible.


On Tue, 13 Feb 2024, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini 
> ---
>  docs/misra/rules.rst | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> index 182331089d..c185366966 100644
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -478,6 +478,12 @@ maintainers if you want to suggest a change.
> adhere to Rule 16.2 would result in increased complexity and
> maintenance difficulty, and could potentially introduce bugs. 
>  
> +   * - `Rule 16.6 
> `_
> + - Required
> + - Every switch statement shall have at least two switch-clauses
> + - Single-clause switches are allowed when they do not involve a
> +   default label.
> +
> * - `Rule 16.7 
> `_
>   - Required
>   - A switch-expression shall not have essentially Boolean type
> @@ -554,6 +560,13 @@ maintainers if you want to suggest a change.
> evaluation
>   -
>  
> +   * - `Rule 20.12 
> `_
> + - A macro parameter used as an operand to the # or ## operators,
> +   which is itself subject to further macro replacement, shall only
> +   be used as an operand to these operators
> + - Required
> + - Variadic macros are allowed to violate the rule.
> +
> * - `Rule 20.13 
> `_
>   - Required
>   - A line whose first token is # shall be a valid preprocessing
> -- 
> 2.25.1
> 
> 

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

[Alexei Starovoitov]

From: Alexei Starovoitov 

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

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

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

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

  // request virtual memory region during bpf prog verification
  area = get_vm_area(area_size, VM_SPARSE);

  // on demand
  vm_area_map_pages(area, kaddr, kend, pages);
  vm_area_unmap_pages(area, kaddr, kend);

  // after bpf program is detached and unloaded
  free_vm_area(area);

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

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

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

[Alexei Starovoitov]

From: Alexei Starovoitov 

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

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

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

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

[Alexei Starovoitov]

From: Alexei Starovoitov 

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

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

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

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

[Alexei Starovoitov]

From: Alexei Starovoitov 

v2 -> v3
- added Christoph's reviewed-by to patch 1
- cap commit log lines to 75 chars
- factored out common checks in patch 3 into helper
- made vm_area_unmap_pages() return void

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

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

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

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

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

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

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

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

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

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

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

-- 
2.34.1

Re: [PATCH v2 2/3] docs/misra/rules.rst: add rule 5.5

[Stefano Stabellini]

On Wed, 14 Feb 2024, Federico Serafini wrote:
> On 14/02/24 14:15, Jan Beulich wrote:
> > On 14.02.2024 12:27, Federico Serafini wrote:
> > > On 14/02/24 09:28, Jan Beulich wrote:
> > > > On 13.02.2024 23:33, Stefano Stabellini wrote:
> > > > > Signed-off-by: Stefano Stabellini 
> > > > > ---
> > > > >docs/misra/rules.rst | 6 ++
> > > > >1 file changed, 6 insertions(+)
> > > > > 
> > > > > diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> > > > > index c185366966..931158b354 100644
> > > > > --- a/docs/misra/rules.rst
> > > > > +++ b/docs/misra/rules.rst
> > > > > @@ -181,6 +181,12 @@ maintainers if you want to suggest a change.
> > > > >   headers (xen/include/public/) are allowed to retain longer
> > > > >   identifiers for backward compatibility.
> > > > >+   * - `Rule 5.5
> > > > > `_
> > > > > + - Required
> > > > > + - Identifiers shall be distinct from macro names
> > > > > + - Clashes between function-like macros and non-callable entities
> > > > > +   are allowed. The pattern #define x x is also allowed.
> > > > 
> > > > Just for me to know what exactly is covered (hence also a question
> > > > to Roberto as to [to be] implemented Eclair behavior): Even when
> > > > the above would be sufficient (and imo better) people frequently
> > > > write
> > > > 
> > > > #define a(x, y) b(x, y)
> > > > 
> > > > which, transformed to the specific case here, would then be
> > > > 
> > > > #define a(x, y) a(x, y)
> > > > 
> > > > I'd assume such ought to also be covered, but that's not clear
> > > > from the spelling above.
> > > 
> > > I list what happens in some different situations,
> > > then we can find the right words for the documentation and/or
> > > refine the configuration:
> > > 
> > > If you
> > > #define x x
> > > and then use `x' as identifier,
> > > the resulting violation is deviated (allowed pattern).
> > > 
> > > If you
> > > #define a(x, y) a(x, y)
> > > and then use `a' as identifier for a non-callable entity,
> > > the resulting violation is deviated (no clash with non-callable
> > > entities).
> > > If you use identifier `a' for a callable entity, the resulting violation
> > > is reported: the allowed pattern covers only macros expanding to their
> > > own name, in this case the macro name is considered to be
> > > `a' only, not a(x, y).
> > > 
> > > If you
> > > #define a(x, y) b(x, y)
> > > and then use `a' as identifier for a non-callable entity,
> > > the resulting violation is deviated (no clash with non-callable
> > > entities).
> > 
> > I'm afraid I don't see what violation there is in this case, to
> > deviate. As a result I'm also not sure I correctly understand the
> > rest of your reply.
> 
> #define a(x, y) b(x, y)
> 
> int a; // Violation of Rule 5.5.
> 
> The macro name `a' that exist before the preprocessing phase,
> still exists after the preprocessing phase as identifier for the integer
> variable and this is a violation.
> 
> > > If you use `a' as identifier for a callable entity,
> > > this is not a violation because after the preprocessing phase,
> > > identifier `a' no longer exists.
> I correct myself:
> if you use `a' as identifier for a *function*,
> it is not a violation because after the preprocessing phase
> the identifier `a' no longer exists, for example:
> 
> #define a(x, y) b(x, y)
> 
> void a(int x, int y); // Ok.

Federico, do you have a better wording suggestion for this rule?

Jan, any further requests here? What would you like to see as next step?

Re: [QEMU][PATCH v3 6/7] xen: add map and unmap callbacks for grant region

[Stefano Stabellini]

On Tue, 27 Feb 2024, Vikram Garhwal wrote:
> From: Juergen Gross 
> 
> Add the callbacks for mapping/unmapping guest memory via grants to the
> special grant memory region.
> 
> Signed-off-by: Juergen Gross 
> Signed-off-by: Vikram Garhwal 

Reviewed-by: Stefano Stabellini 


> ---
>  hw/xen/xen-mapcache.c | 176 +-
>  system/physmem.c  |  11 ++-
>  2 files changed, 182 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
> index 179b7e95b2..2e4c9b4947 100644
> --- a/hw/xen/xen-mapcache.c
> +++ b/hw/xen/xen-mapcache.c
> @@ -9,6 +9,8 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/queue.h"
> +#include "qemu/thread.h"
>  #include "qemu/units.h"
>  #include "qemu/error-report.h"
>  
> @@ -23,6 +25,8 @@
>  #include "sysemu/xen-mapcache.h"
>  #include "trace.h"
>  
> +#include 
> +#include 
>  
>  #if HOST_LONG_BITS == 32
>  #  define MCACHE_BUCKET_SHIFT 16
> @@ -377,7 +381,7 @@ uint8_t *xen_map_cache(hwaddr phys_addr, hwaddr size,
>  return p;
>  }
>  
> -ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
> +static ram_addr_t xen_ram_addr_from_mapcache_try(void *ptr)
>  {
>  MapCacheEntry *entry = NULL;
>  MapCacheRev *reventry;
> @@ -588,10 +592,179 @@ uint8_t *xen_replace_cache_entry(hwaddr old_phys_addr,
>  return p;
>  }
>  
> +struct XENMappedGrantRegion {
> +void *addr;
> +unsigned int pages;
> +unsigned int refs;
> +unsigned int prot;
> +uint32_t idx;
> +QLIST_ENTRY(XENMappedGrantRegion) list;
> +};
> +
> +static xengnttab_handle *xen_region_gnttabdev;
> +static QLIST_HEAD(GrantRegionList, XENMappedGrantRegion) xen_grant_mappings =
> +QLIST_HEAD_INITIALIZER(xen_grant_mappings);
> +static QemuMutex xen_map_mutex;
> +
> +static void *xen_map_grant_dyn(MemoryRegion **mr, hwaddr addr, hwaddr *plen,
> +   bool is_write, MemTxAttrs attrs)
> +{
> +unsigned int page_off = addr & (XC_PAGE_SIZE - 1);
> +unsigned int i;
> +unsigned int total_grants = 0;
> +unsigned int nrefs = (page_off + *plen + XC_PAGE_SIZE - 1) >> 
> XC_PAGE_SHIFT;
> +uint32_t ref = (addr - XEN_GRANT_ADDR_OFF) >> XC_PAGE_SHIFT;
> +uint32_t *refs = NULL;
> +unsigned int prot = PROT_READ;
> +struct XENMappedGrantRegion *mgr = NULL;
> +
> +if (is_write) {
> +prot |= PROT_WRITE;
> +}
> +
> +qemu_mutex_lock(_map_mutex);
> +
> +QLIST_FOREACH(mgr, _grant_mappings, list) {
> +if (mgr->idx == ref &&
> +mgr->pages == nrefs &&
> +(mgr->prot & prot) == prot) {
> +break;
> +}
> +
> +total_grants += mgr->pages;
> +}
> +
> +if (!mgr) {
> +if (nrefs + total_grants >= XEN_MAX_VIRTIO_GRANTS) {
> +qemu_mutex_unlock(_map_mutex);
> +return NULL;
> +}
> +
> +mgr = g_new(struct XENMappedGrantRegion, 1);
> +
> +if (nrefs == 1) {
> +refs = 
> +} else {
> +refs = g_new(uint32_t, nrefs);
> +for (i = 0; i < nrefs; i++) {
> +refs[i] = ref + i;
> +}
> +}
> +mgr->addr = xengnttab_map_domain_grant_refs(xen_region_gnttabdev, 
> nrefs,
> +xen_domid, refs, prot);
> +if (mgr->addr) {
> +mgr->pages = nrefs;
> +mgr->refs = 1;
> +mgr->prot = prot;
> +mgr->idx = ref;
> +
> +QLIST_INSERT_HEAD(_grant_mappings, mgr, list);
> +} else {
> +g_free(mgr);
> +mgr = NULL;
> +}
> +} else {
> +mgr->refs++;
> +}
> +
> +qemu_mutex_unlock(_map_mutex);
> +
> +if (nrefs > 1) {
> +g_free(refs);
> +}
> +
> +return mgr ? mgr->addr + page_off : NULL;
> +}
> +
> +static void xen_unmap_grant_dyn(MemoryRegion *mr, void *buffer, ram_addr_t 
> addr,
> +hwaddr len, bool is_write, hwaddr access_len)
> +{
> +unsigned int page_off = (unsigned long)buffer & (XC_PAGE_SIZE - 1);
> +unsigned int nrefs = (page_off + len + XC_PAGE_SIZE - 1) >> 
> XC_PAGE_SHIFT;
> +unsigned int prot = PROT_READ;
> +struct XENMappedGrantRegion *mgr = NULL;
> +
> +if (is_write) {
> +prot |= PROT_WRITE;
> +}
> +
> +qemu_mutex_lock(_map_mutex);
> +
> +QLIST_FOREACH(mgr, _grant_mappings, list) {
> +if (mgr->addr == buffer - page_off &&
> +mgr->pages == nrefs &&
> +(mgr->prot & prot) == prot) {
> +break;
> +}
> +}
> +if (mgr) {
> +mgr->refs--;
> +if (!mgr->refs) {
> +xengnttab_unmap(xen_region_gnttabdev, mgr->addr, nrefs);
> +
> +QLIST_REMOVE(mgr, list);
> +g_free(mgr);
> +}
> +} else {
> +error_report("xen_unmap_grant_dyn() trying to unmap unknown buffer");
> +}
> +
> +qemu_mutex_unlock(_map_mutex);
> +}
> +
> 

Re: [XEN PATCH] automation/eclair: extend deviations of MISRA C:2012 Rule 16.3

[Stefano Stabellini]

On Thu, 29 Feb 2024, Jan Beulich wrote:
> On 29.02.2024 09:01, Federico Serafini wrote:
> > On 28/02/24 10:06, Jan Beulich wrote:
> >> On 28.02.2024 09:53, Federico Serafini wrote:
> >>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> >>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> >>
> >> Comments below apply similarly to text added to this file.
> >>
> >>> --- a/docs/misra/deviations.rst
> >>> +++ b/docs/misra/deviations.rst
> >>> @@ -291,7 +291,14 @@ Deviations related to MISRA C:2012 Rules:
> >>>- Project-wide deviation; tagged as `deliberate` for ECLAIR.
> >>>   
> >>>  * - R16.3
> >>> - - Switch clauses ending with continue, goto, return statements are 
> >>> safe.
> >>> + - Switch clauses ending with an unconditional flow control statement
> >>> +   (i.e., continue, goto, or return) are safe.
> >>> + - Tagged as `safe` for ECLAIR.
> >>
> >> With this edit (unmentioned in the description, btw) ...
> >>
> >>> +   * - R16.3
> >>> + - Switch clauses ending with an if-else statement are safe if both
> >>> +   branches consist of a flow control statement (i.e., continue, 
> >>> break,
> >>> +   goto, return).
> >>
> >> ... why is it not also "ending with" here?
> > 
> > Because the allowed pattern is:
> > 
> > if ( cond )
> >   return; /* Or continue / break / goto */
> > else
> >   break;  /* Or continue / goto / return */
> > 
> > See below for more information.
> > 
> >>
> >> Also what about either situation ending with a call to a noreturn function?
> > 
> > This can be added.
> > 
> >>
> >>> @@ -307,6 +314,16 @@ Deviations related to MISRA C:2012 Rules:
> >>>- Switch clauses ending with failure method \"BUG()\" are safe.
> >>>- Tagged as `safe` for ECLAIR.
> >>>   
> >>> +   * - R16.3
> >>> + - On X86, switch clauses ending generating an exception through
> >>> +   \"generate_exception()\" are safe.
> >>> + - Tagged as `safe` for ECLAIR.
> >>
> >> This macro is limited to the emulator, so shouldn't be deviated globally.
> > 
> > Noted.
> > 
> >> Furthermore - why does the special case need mentioning here? Shouldn't
> >> it be the underlying pattern which is deviated (along the lines of the
> >> earlier ones):
> >>
> >>  if ( true )
> >>  {
> >>  ...
> >>  goto ...; /* Or break / continue / return */
> >>  }
> > 
> > This pattern that involves a compound statement for the true branch
> > is not deviated by this configuration.
> > 
> > See below for more information.
> > 
> >>
> >>> +   * - R16.3
> >>> + - Switch clauses ending generating a parse error through
> >>> +   \"PARSE_ERR_RET()\" are safe.
> >>> + - Tagged as `safe` for ECLAIR.
> >>
> >> Again this isn't a global scope macro, so shouldn't be deviated globally.
> > 
> > Noted.
> > 
> >> Plus it ends in "return", so ought to be covered by the earlier clause.
> >> The fact that the return is in a body of do {} while(0) shouldn't matter
> >> at all - that's purely syntactic sugar.
> > 
> > I gather from your comments/questions that you would like to deviate
> > *all* the patterns where an unintentional fall through can not happen.
> > 
> > Rule 16.3 is a purely syntactic rule, and, as a consequence,
> > in the current version of ECLAIR additional "allowed pattern" (aka
> > deviations) for that rule need to be described through AST nodes,
> > meaning that all what you consider as syntactic sugar cannot be ignored.
> > 
> > A deviation that covers all the pattern you are asking for could be
> > done, but it will result in a complex and quite long expression
> > (not easy to read and justify in front of an assessor).
> > 
> > Hence, what I am proposing is to deviate only the the simplest and
> > most readable cases, such as:
> > 
> > if ( cond )
> >return x;
> > else
> >return y;
> > 
> > without involving compound statements, fake do-wile and fake if
> > statements but rather deviating the macro inside of which are used
> > (as I did).
> 
> I see. Problem is that this isn't sufficient for the code we have, and
> the seemingly random deviation of certain constructs by name looks to
> me as pretty undesirable.

Yeah, I also think it is not ideal. At the same time among all options,
it is probably the best way forward (better than in-code comments or
better than leaving the violations outstanding).

I think we should go for it.

Re: [XEN PATCH 10/10] xen/keyhandler: address violations of MISRA C Rule 20.7

[Stefano Stabellini]

On Thu, 29 Feb 2024, Nicola Vetrini wrote:
> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> of macro parameters shall be enclosed in parentheses". Therefore, some
> macro definitions should gain additional parentheses to ensure that all
> current and future users will be safe with respect to expansions that
> can possibly alter the semantics of the passed-in macro parameter.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini 

Reviewed-by: Stefano Stabellini 


> ---
>  xen/common/keyhandler.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
> index 127ca506965c..4c1ce007870f 100644
> --- a/xen/common/keyhandler.c
> +++ b/xen/common/keyhandler.c
> @@ -42,10 +42,10 @@ static struct keyhandler {
>  } key_table[128] __read_mostly =
>  {
>  #define KEYHANDLER(k, f, desc, diag)\
> -[k] = { { .fn = (f) }, desc, 0, diag }
> +[k] = { { .fn = (f) }, (desc), 0, (diag) }
>  
>  #define IRQ_KEYHANDLER(k, f, desc, diag)\
> -[k] = { { .irq_fn = (f) }, desc, 1, diag }
> +[k] = { { .irq_fn = (f) }, (desc), 1, (diag) }
>  
>  IRQ_KEYHANDLER('A', do_toggle_alt_key, "toggle alternative key 
> handling", 0),
>  IRQ_KEYHANDLER('d', dump_registers, "dump registers", 1),
> -- 
> 2.34.1
> 

Re: [XEN PATCH 09/10] xen/include: tasklet: address violations of MISRA C Rule 20.7

[Stefano Stabellini]

On Thu, 29 Feb 2024, Nicola Vetrini wrote:
> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> of macro parameters shall be enclosed in parentheses". Therefore, some
> macro definitions should gain additional parentheses to ensure that all
> current and future users will be safe with respect to expansions that
> can possibly alter the semantics of the passed-in macro parameter.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini 
> ---
>  xen/include/xen/tasklet.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/xen/tasklet.h b/xen/include/xen/tasklet.h
> index 593d6a2400fb..78760b694a39 100644
> --- a/xen/include/xen/tasklet.h
> +++ b/xen/include/xen/tasklet.h
> @@ -27,7 +27,7 @@ struct tasklet
>  
>  #define _DECLARE_TASKLET(name, func, data, softirq) \
>  struct tasklet name = { \
> -LIST_HEAD_INIT(name.list), -1, softirq, 0, 0, func, data }
> +LIST_HEAD_INIT((name).list), -1, softirq, 0, 0, func, data }
>  #define DECLARE_TASKLET(name, func, data)   \
>  _DECLARE_TASKLET(name, func, data, 0)
>  #define DECLARE_SOFTIRQ_TASKLET(name, func, data)   \

In reality this is not required due to "struct tasklet name", but for
uniformity:

Reviewed-by: Stefano Stabellini 

Re: [XEN PATCH 08/10] xen/errno: address violations of MISRA C Rule 20.7

[Stefano Stabellini]

On Thu, 29 Feb 2024, Nicola Vetrini wrote:
> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> of macro parameters shall be enclosed in parentheses". Therefore, some
> macro definitions should gain additional parentheses to ensure that all
> current and future users will be safe with respect to expansions that
> can possibly alter the semantics of the passed-in macro parameter.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini 
> ---
>  xen/include/xen/errno.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/xen/errno.h b/xen/include/xen/errno.h
> index 69b28dd3c6c5..506674701fae 100644
> --- a/xen/include/xen/errno.h
> +++ b/xen/include/xen/errno.h
> @@ -3,7 +3,7 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define XEN_ERRNO(name, value) name = value,
> +#define XEN_ERRNO(name, value) name = (value),

I see this and the fact that "name" was not parenthesized and it would
deliberate right? So I guess the left side of an assignment doesn't need
parenthesis?

Re: [XEN PATCH 07/10] xen/arm: smmuv3: address violations of MISRA C Rule 20.7

[Stefano Stabellini]

On Thu, 29 Feb 2024, Nicola Vetrini wrote:
> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> of macro parameters shall be enclosed in parentheses". Therefore, some
> macro definitions should gain additional parentheses to ensure that all
> current and future users will be safe with respect to expansions that
> can possibly alter the semantics of the passed-in macro parameter.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini 

Reviewed-by: Stefano Stabellini 


> ---
>  xen/drivers/passthrough/arm/smmu-v3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> b/xen/drivers/passthrough/arm/smmu-v3.c
> index c3ac6d17d1c8..b1c40c2c0ae7 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -111,7 +111,7 @@
>  #define GFP_KERNEL   0
>  
>  /* Device logger functions */
> -#define dev_name(dev)dt_node_full_name(dev->of_node)
> +#define dev_name(dev)dt_node_full_name((dev)->of_node)
>  #define dev_dbg(dev, fmt, ...)   \
>   printk(XENLOG_DEBUG "SMMUv3: %s: " fmt, dev_name(dev), ## __VA_ARGS__)
>  #define dev_notice(dev, fmt, ...)\
> -- 
> 2.34.1
> 

Re: [XEN PATCH 06/10] arm/smmu: address some violations of MISRA C Rule 20.7

[Stefano Stabellini]

On Thu, 29 Feb 2024, Nicola Vetrini wrote:
> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> of macro parameters shall be enclosed in parentheses". Therefore, some
> macro definitions should gain additional parentheses to ensure that all
> current and future users will be safe with respect to expansions that
> can possibly alter the semantics of the passed-in macro parameter.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini 
> ---
>  xen/drivers/passthrough/arm/smmu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c 
> b/xen/drivers/passthrough/arm/smmu.c
> index 625ed0e41961..83196057a937 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -242,7 +242,7 @@ struct arm_smmu_xen_device {
>   struct iommu_group *group;
>  };
>  
> -#define dev_archdata(dev) ((struct arm_smmu_xen_device *)dev->iommu)
> +#define dev_archdata(dev) ((struct arm_smmu_xen_device *)(dev)->iommu)
>  #define dev_iommu_domain(dev) (dev_archdata(dev)->domain)
>  #define dev_iommu_group(dev) (dev_archdata(dev)->group)

this is OK


> @@ -627,7 +627,7 @@ struct arm_smmu_master_cfg {
>  };
>  #define INVALID_SMENDX   -1
>  #define for_each_cfg_sme(cfg, i, idx, num) \
> - for (i = 0; idx = cfg->smendx[i], i < num; ++i)
> + for (i = 0; idx = (cfg)->smendx[i], (i) < (num); ++(i))

The first i = 0 is missing parentheses?
Also idx misses parentheses?

Re: [XEN PATCH 04/10] xen/public: address violations of MISRA C Rule 20.7

[Stefano Stabellini]

On Thu, 29 Feb 2024, Nicola Vetrini wrote:
> On 2024-02-29 17:40, Jan Beulich wrote:
> > On 29.02.2024 16:27, Nicola Vetrini wrote:
> > > --- a/xen/include/public/xen.h
> > > +++ b/xen/include/public/xen.h
> > > @@ -988,7 +988,7 @@ typedef struct {
> > >((b) >>  8) & 0xFF, ((b) >>  0) & 0xFF,   \
> > >((c) >>  8) & 0xFF, ((c) >>  0) & 0xFF,   \
> > >((d) >>  8) & 0xFF, ((d) >>  0) & 0xFF,   \
> > > -e1, e2, e3, e4, e5, e6}}
> > > +(e1), (e2), (e3), (e4), (e5), (e6)}}
> > 
> > Why? Wasn't it agreed already that long macro arguments passed on
> > (no matter whether to a function, a macro, or like used here) don't
> > need parenthesizing?
> > 
> 
> That applies to all outermost macro invocations, but not to the innermost one.

I don't understand what you mean. Maybe a couple of trivial examples
would help.


> If you want also aggregate initalizers to be deviated, that could be done
> (provided that the macro arg is not included in some expression, such as
> "{..., e1 + 1, ...}"

My gut feeling tells me that probably this is what we want but I'd
rather first understand exactly what you meant above

[PATCH] x86/cpu-policy: Hide x2APIC from PV guests

[Andrew Cooper]

PV guests can't write to MSR_APIC_BASE (in order to set EXTD), nor can they
access any of the x2APIC MSR range.  Therefore they mustn't see the x2APIC
CPUID bit saying that they can.

Right now, the host x2APIC flag filters into PV guests, meaning that PV guests
generally see x2APIC except on Zen1-and-older AMD systems.

Linux works around this by explicitly hiding the bit itself, and filtering
EXTD out of MSR_APIC_BASE reads.  NetBSD behaves more in the spirit of PV
guests, and entirely ignores the APIC when built as a PV guest.

Change the annotation from !A to !S.  This has a consequence of stripping it
out of both PV featuremasks.  However, as existing guests may have seen the
bit, set it back into the PV Max policy; a VM which saw the bit and is alive
enough to migrate will have ignored it one way or another.

Hiding x2APIC does also change the contents of leaf 0xb, but as the
information is nonsense to begin with, this is likely an improvement on the
status quo.  The blind reporting of APIC_ID = vCPU_ID * 2 isn't interlinked
with the host's topology structure, and the APIC_IDs are useless without an
MADT to start with.  Dom0 is the only PV VM to get an MADT but it's the host
one, meaning the two sets of APIC_IDs are from different address spaces.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Wei Liu 

v2:
 * Set x2APIC in PV max after applying the featuremask.
 * Drop the hunk for the default policy as it's handled by the A->S transform.
 * Rewrite the commit message.
---
 xen/arch/x86/cpu-policy.c   | 11 +--
 xen/include/public/arch-x86/cpufeatureset.h |  2 +-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
index 15b49048fd55..c9b32bc17849 100644
--- a/xen/arch/x86/cpu-policy.c
+++ b/xen/arch/x86/cpu-policy.c
@@ -561,6 +561,14 @@ static void __init calculate_pv_max_policy(void)
 for ( i = 0; i < ARRAY_SIZE(fs); ++i )
 fs[i] &= pv_max_featuremask[i];
 
+/*
+ * Xen at the time of writing (Feb 2024, 4.19 dev cycle) used to leak the
+ * host x2APIC capability into PV guests, but never supported the guest
+ * trying to turn x2APIC mode on.  Tolerate an incoming VM which saw the
+ * x2APIC CPUID bit and is alive enough to migrate.
+ */
+__set_bit(X86_FEATURE_X2APIC, fs);
+
 /*
  * If Xen isn't virtualising MSR_SPEC_CTRL for PV guests (functional
  * availability, or admin choice), hide the feature.
@@ -854,11 +862,10 @@ void recalculate_cpuid_policy(struct domain *d)
 }
 
 /*
- * Allow the toolstack to set HTT, X2APIC and CMP_LEGACY.  These bits
+ * Allow the toolstack to set HTT and CMP_LEGACY.  These bits
  * affect how to interpret topology information in other cpuid leaves.
  */
 __set_bit(X86_FEATURE_HTT, max_fs);
-__set_bit(X86_FEATURE_X2APIC, max_fs);
 __set_bit(X86_FEATURE_CMP_LEGACY, max_fs);
 
 /*
diff --git a/xen/include/public/arch-x86/cpufeatureset.h 
b/xen/include/public/arch-x86/cpufeatureset.h
index 7e184ce0e3f4..0374cec3a2af 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -123,7 +123,7 @@ XEN_CPUFEATURE(PCID,  1*32+17) /*H  Process Context 
ID */
 XEN_CPUFEATURE(DCA,   1*32+18) /*   Direct Cache Access */
 XEN_CPUFEATURE(SSE4_1,1*32+19) /*A  Streaming SIMD Extensions 4.1 */
 XEN_CPUFEATURE(SSE4_2,1*32+20) /*A  Streaming SIMD Extensions 4.2 */
-XEN_CPUFEATURE(X2APIC,1*32+21) /*!A Extended xAPIC */
+XEN_CPUFEATURE(X2APIC,1*32+21) /*!S Extended xAPIC */
 XEN_CPUFEATURE(MOVBE, 1*32+22) /*A  movbe instruction */
 XEN_CPUFEATURE(POPCNT,1*32+23) /*A  POPCNT instruction */
 XEN_CPUFEATURE(TSC_DEADLINE,  1*32+24) /*S  TSC Deadline Timer */
-- 
2.30.2

Re: [PATCH] SUPPORT.md: clarify support of booting 32-bit Xen on ARMv8

[Stefano Stabellini]

On Thu, 29 Feb 2024, Michal Orzel wrote:
> 
> On 29/02/2024 13:40, Julien Grall wrote:
> > 
> > 
> > On 29/02/2024 12:37, Michal Orzel wrote:
> >> Hi Julien,
> > 
> > Hi Michal,
> > 
> >>
> >> On 29/02/2024 13:35, Julien Grall wrote:
> >>> On 29/02/2024 12:13, Michal Orzel wrote:
>  Since commit bd1001db0af1 ("xen/arm: arm32: Allow Xen to boot on
>  unidentified CPUs"), it's been possible to boot 32-bit Xen on ARMv8A CPUs
>  in AArch32 state (assuming HW supports EL2 execution in AArch32). Clarify
>  the support statement and mark it as Tech Preview, as this use case is
>  uncommon and hasn't really been tested/hardened.
> 
>  Signed-off-by: Michal Orzel 
>  ---
> SUPPORT.md | 1 +
> 1 file changed, 1 insertion(+)
> 
>  diff --git a/SUPPORT.md b/SUPPORT.md
>  index a90d1108c9d9..acc61230bb5e 100644
>  --- a/SUPPORT.md
>  +++ b/SUPPORT.md
>  @@ -40,6 +40,7 @@ supported in this document.
> Status: Supported
> >>> I would consider to use 'Status, Xen in aarch64 mode: Supported' and 
> >>> then...
> >>>
> Status, Cortex A57 r0p0-r1p1: Supported, not security supported
> Status, Cortex A77 r0p0-r1p0: Supported, not security supported
>  +Status, Xen in AArch32 mode: Tech Preview
> >>>
> >>> ... move this line closer. What do you think?
> >> That works for me too (+AArch64 instead of aarch64).
> > 
> > Ah yes. I keep forgetting capitalizing properly :).
> > 
> >> Shall I respin the patch?
> > 
> > Up to you. I am happy to fix it. But I will wait a day or two just to
> > give a chance for the others to comment.
> That works for me, let's wait for Bertrand and Stefano.

I am fine with it

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 linux805d849d7c3cc1f38efefd48b2480d62b7b5dcb7
baseline version:
 linuxcf1182944c7cc9f1c21a8a44e0d29abe12527412

Last test of basis   184802  2024-02-28 01:14:44 Z1 days
Testing same since   184816  2024-02-29 05:22:18 Z0 days1 attempts


People who touched revisions under test:
  Ard Biesheuvel 
  Doug Smythies 
  Herbert Xu 
  Linus Torvalds 
  Lukas Bulwahn 
  Mark Brown 
  Naresh Solanki 
  Rafael J. Wysocki 
  Théo Lebrun 

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

[PATCH] tests/resource: Fix HVM guest in !SHADOW builds

[Andrew Cooper]

Right now, test-resource always creates HVM Shadow guests.  But if Xen has
SHADOW compiled out, running the test yields:

  $./test-resource
  XENMEM_acquire_resource tests
  Test x86 PV
Created d1
Test grant table
  Test x86 PVH
Skip: 95 - Operation not supported

and doesn't really test HVM guests, but doesn't fail either.

There's nothing paging-mode-specific about this test, so default to HAP if
possible and provide a more specific message if neither HAP or Shadow are
available.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Wei Liu 
---
 tools/tests/resource/test-resource.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/tools/tests/resource/test-resource.c 
b/tools/tests/resource/test-resource.c
index 7ae88ea34807..2796053588d3 100644
--- a/tools/tests/resource/test-resource.c
+++ b/tools/tests/resource/test-resource.c
@@ -20,6 +20,8 @@ static xc_interface *xch;
 static xenforeignmemory_handle *fh;
 static xengnttab_handle *gh;
 
+static xc_physinfo_t physinfo;
+
 static void test_gnttab(uint32_t domid, unsigned int nr_frames,
 unsigned long gfn)
 {
@@ -172,6 +174,23 @@ static void test_domain_configurations(void)
 
 printf("Test %s\n", t->name);
 
+#if defined(__x86_64__) || defined(__i386__)
+/*
+ * On x86, use HAP guests if possible, but skip if neither HAP nor
+ * SHADOW is available.
+ */
+if ( t->create.flags & XEN_DOMCTL_CDF_hvm )
+{
+if ( physinfo.capabilities & XEN_SYSCTL_PHYSCAP_hap )
+t->create.flags |= XEN_DOMCTL_CDF_hap;
+else if ( !(physinfo.capabilities & XEN_SYSCTL_PHYSCAP_shadow) )
+{
+printf("  Skip: Neither HAP or SHADOW available\n");
+continue;
+}
+}
+#endif
+
 rc = xc_domain_create(xch, , >create);
 if ( rc )
 {
@@ -214,6 +233,8 @@ static void test_domain_configurations(void)
 
 int main(int argc, char **argv)
 {
+int rc;
+
 printf("XENMEM_acquire_resource tests\n");
 
 xch = xc_interface_open(NULL, NULL, 0);
@@ -227,6 +248,10 @@ int main(int argc, char **argv)
 if ( !gh )
 err(1, "xengnttab_open");
 
+rc = xc_physinfo(xch, );
+if ( rc )
+err(1, "Failed to obtain physinfo");
+
 test_domain_configurations();
 
 return !!nr_failures;

base-commit: 635dd1120a01961a39dce6ad3f09692681379378
prerequisite-patch-id: 1fddefae616fa9a57ddc85bbf770e3c5bdb47b1f
prerequisite-patch-id: 50c4a201d8fa10c1797a1b17f3f2729b5787da84
prerequisite-patch-id: 4b4799fae62b5f41b9b0d2078e8b081605341a0a
-- 
2.30.2

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

[osstest service owner]

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

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  635dd1120a01961a39dce6ad3f09692681379378
baseline version:
 xen  c20850540ad6a32f4fc17bde9b01c92b0df18bf0

Last test of basis   184817  2024-02-29 08:00:28 Z0 days
Testing same since   184823  2024-02-29 17:00:27 Z0 days1 attempts


People who touched revisions under test:
  Anthony PERARD 
  Jan Beulich 
  Juergen Gross 
  Oleksii Kurochko 
  Roger Pau Monné 
  Samuel Thibault 

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
   c20850540a..635dd1120a  635dd1120a01961a39dce6ad3f09692681379378 -> smoke

Re: [PATCH] x86/cpu-policy: Fix x2APIC visibility for PV guests

[Andrew Cooper]

On 29/02/2024 12:47 pm, Jan Beulich wrote:
>> @@ -830,11 +846,10 @@ void recalculate_cpuid_policy(struct domain *d)
>>  }
>>  
>>  /*
>> - * Allow the toolstack to set HTT, X2APIC and CMP_LEGACY.  These bits
>> + * Allow the toolstack to set HTT and CMP_LEGACY.  These bits
>>   * affect how to interpret topology information in other cpuid leaves.
>>   */
>>  __set_bit(X86_FEATURE_HTT, max_fs);
>> -__set_bit(X86_FEATURE_X2APIC, max_fs);
>>  __set_bit(X86_FEATURE_CMP_LEGACY, max_fs);
>>  
>>  /*
> ... these adjustments, just still in calculate_pv_featureset(). I
> haven't gone further backwards to check if/when this exposure has
> really appeared. I wouldn't be surprised if it's been like that
> for all the time since we gained x2APIC support in the hypervisor.

4.7 has:

cpufeatureset.h:140:XEN_CPUFEATURE(X2APIC,    1*32+21) /*!A Extended
xAPIC */

so will expose it to PV guests.

4.6 has this gem I'd forgotten, in pv_cpuid():

    if ( !cpu_has_apic )
   __clear_bit(X86_FEATURE_X2APIC % 32, );

but I don't see any (sensible) logic to hide x2APIC from the view of a
PV guest.

So PV guests really have see x2APIC from the host forever (and
irrespective of Xen's x2APIC support).

~Andrew

Re: [PATCH] xen/arm: Fix arm32 build failure when early printk is enabled

[Stefano Stabellini]

On Thu, 29 Feb 2024, Michal Orzel wrote:
> On 29/02/2024 11:10, Julien Grall wrote:
> > 
> > 
> > Hi,
> > 
> > On 29/02/2024 10:07, Michal Orzel wrote:
> >>
> >>
> >> On 28/02/2024 23:27, Stefano Stabellini wrote:
> >>>
> >>>
> >>> On Wed, 28 Feb 2024, Michal Orzel wrote:
>  Hi Julien,
> 
>  On 28/02/2024 12:42, Julien Grall wrote:
> >
> >
> > Hi Michal,
> >
> > On 28/02/2024 10:35, Michal Orzel wrote:
> >> Commit 0441c3acc7e9 forgot to rename FIXMAP_CONSOLE to FIX_CONSOLE in
> >> TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS macro. This results in a build
> >> failure on arm32, when early printk is enabled:
> >> arch/arm/arm32/mmu/head.S:311: Error: invalid operands (*UND* and 
> >> *ABS* sections) for `*'
> >
> > Good catch! Somewhat related I wonder whether we should add earlyprintk
> > testing in gitlab?
>  I thought about adding this and I think we should at least have build 
>  jobs (hypervisor only, no toolstack)
>  selecting early printk. When it comes to testing if early printk works, 
>  I'm not sure. It'd be nice
>  but FWIR we have limited bandwidth.
> 
>  @Stefano, what's your opinion?
> >>>
> >>> I think it would be a good and quick test to have. To save testing
> >>> bandwidth I think we should reduce the amount of debug/non-debug
> >>> variations of the same tests that we have.
> >> Yes, I suggested that some time ago. We could keep both versions for 
> >> generic tests,
> >> but remove the non-debug version (unless you prefer to do the opposite) 
> >> for:
> > 
> > I think it makes sense during development window to use the debug
> > version. However, I think we want some non-debug testing during the
> > hardening phase.
> > 
> > Can gitlab read CONFIG_DEBUG from Config.mk?
> At the moment, we have 2 types of jobs - non debug and debug (with -debug 
> suffix).
> They set "debug" variable accordingly, which is used later on to modify 
> .config:
> echo "CONFIG_DEBUG=${debug}" >> xen/.config
> 
> Without this line, Xen would be built according to default value of 
> CONFIG_DEBUG.
> That said, I don't think we want to get back to this behavior.
> 
> If we want to save some bandwidth, we should make a decision whether to keep 
> debug or non-debug versions.
> x86 has both versions for build jobs and mostly debug test jobs.

It is good to have some debug and non-debug jobs, but we probably don't
need both versions of every job.

[PATCH] x86/cpu-policy: Allow for levelling of VERW side effects

[Andrew Cooper]

MD_CLEAR and FB_CLEAR need OR-ing across a migrate pool.  Allow this, by
having them unconditinally set in max, with the host values reflected in
default.  Annotate the bits as having special properies.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Wei Liu 
---
 xen/arch/x86/cpu-policy.c   | 24 +
 xen/include/public/arch-x86/cpufeatureset.h |  4 ++--
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
index f3ed2d3a3227..41123e6cf778 100644
--- a/xen/arch/x86/cpu-policy.c
+++ b/xen/arch/x86/cpu-policy.c
@@ -442,6 +442,16 @@ static void __init 
guest_common_max_feature_adjustments(uint32_t *fs)
 __set_bit(X86_FEATURE_RSBA, fs);
 __set_bit(X86_FEATURE_RRSBA, fs);
 
+/*
+ * These bits indicate that the VERW instruction may have gained
+ * scrubbing side effects.  With pooling, they mean "you might migrate
+ * somewhere where scrubbing is necessary", and may need exposing on
+ * unaffected hardware.  This is fine, because the VERW instruction
+ * has been around since the 286.
+ */
+__set_bit(X86_FEATURE_MD_CLEAR, fs);
+__set_bit(X86_FEATURE_FB_CLEAR, fs);
+
 /*
  * The Gather Data Sampling microcode mitigation (August 2023) has an
  * adverse performance impact on the CLWB instruction on SKX/CLX/CPX.
@@ -476,6 +486,20 @@ static void __init 
guest_common_default_feature_adjustments(uint32_t *fs)
  cpu_has_rdrand && !is_forced_cpu_cap(X86_FEATURE_RDRAND) )
 __clear_bit(X86_FEATURE_RDRAND, fs);
 
+/*
+ * These bits indicate that the VERW instruction may have gained
+ * scrubbing side effects.  The max policy has them set for migration
+ * reasons, so reset the default policy back to the host values in
+ * case we're unaffected.
+ */
+fs[FEATURESET_7d0]   &= ~cpufeat_mask(X86_FEATURE_MD_CLEAR);
+fs[FEATURESET_m10Al] &= ~cpufeat_mask(X86_FEATURE_FB_CLEAR);
+
+fs[FEATURESET_7d0]   |= (boot_cpu_data.x86_capability[FEATURESET_7d0] &
+ cpufeat_mask(X86_FEATURE_MD_CLEAR));
+fs[FEATURESET_m10Al] |= 
(boot_cpu_data.x86_capability[FEATURESET_m10Al] &
+ cpufeat_mask(X86_FEATURE_FB_CLEAR));
+
 /*
  * The Gather Data Sampling microcode mitigation (August 2023) has an
  * adverse performance impact on the CLWB instruction on SKX/CLX/CPX.
diff --git a/xen/include/public/arch-x86/cpufeatureset.h 
b/xen/include/public/arch-x86/cpufeatureset.h
index b230d3a6907d..0374cec3a2af 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -262,7 +262,7 @@ XEN_CPUFEATURE(AVX512_4FMAPS, 9*32+ 3) /*A  AVX512 Multiply 
Accumulation Single
 XEN_CPUFEATURE(FSRM,  9*32+ 4) /*A  Fast Short REP MOVS */
 XEN_CPUFEATURE(AVX512_VP2INTERSECT, 9*32+8) /*a  VP2INTERSECT{D,Q} insns */
 XEN_CPUFEATURE(SRBDS_CTRL,9*32+ 9) /*   MSR_MCU_OPT_CTRL and 
RNGDS_MITG_DIS. */
-XEN_CPUFEATURE(MD_CLEAR,  9*32+10) /*A  VERW clears microarchitectural 
buffers */
+XEN_CPUFEATURE(MD_CLEAR,  9*32+10) /*!A VERW clears microarchitectural 
buffers */
 XEN_CPUFEATURE(RTM_ALWAYS_ABORT, 9*32+11) /*! June 2021 TSX defeaturing in 
microcode. */
 XEN_CPUFEATURE(TSX_FORCE_ABORT, 9*32+13) /* MSR_TSX_FORCE_ABORT.RTM_ABORT */
 XEN_CPUFEATURE(SERIALIZE, 9*32+14) /*A  SERIALIZE insn */
@@ -334,7 +334,7 @@ XEN_CPUFEATURE(DOITM,  16*32+12) /*   Data 
Operand Invariant Timing
 XEN_CPUFEATURE(SBDR_SSDP_NO,   16*32+13) /*A  No Shared Buffer Data Read 
or Sideband Stale Data Propagation */
 XEN_CPUFEATURE(FBSDP_NO,   16*32+14) /*A  No Fill Buffer Stale Data 
Propagation */
 XEN_CPUFEATURE(PSDP_NO,16*32+15) /*A  No Primary Stale Data 
Propagation */
-XEN_CPUFEATURE(FB_CLEAR,   16*32+17) /*A  Fill Buffers cleared by VERW 
*/
+XEN_CPUFEATURE(FB_CLEAR,   16*32+17) /*!A Fill Buffers cleared by VERW 
*/
 XEN_CPUFEATURE(FB_CLEAR_CTRL,  16*32+18) /*   
MSR_OPT_CPU_CTRL.FB_CLEAR_DIS */
 XEN_CPUFEATURE(RRSBA,  16*32+19) /*!  Restricted RSB Alternative */
 XEN_CPUFEATURE(BHI_NO, 16*32+20) /*A  No Branch History Injection  
*/

base-commit: 54fd7b997470e6686667ca8e18f9ba6139efcdea
prerequisite-patch-id: d2cbc8f341e98ccfd66016f19532df3ddbfc68a4
prerequisite-patch-id: 4b4799fae62b5f41b9b0d2078e8b081605341a0a
-- 
2.30.2

Re: preparations for 4.18.1

[Stefano Stabellini]

On Thu, 29 Feb 2024, Jan Beulich wrote:
> On 28.02.2024 23:45, Stefano Stabellini wrote:
> > On Wed, 28 Feb 2024, Julien Grall wrote:
> >> On 28/02/2024 12:58, Jan Beulich wrote:
> >>> On 28.02.2024 12:50, Julien Grall wrote:
>  On 27/02/2024 13:19, Jan Beulich wrote:
> > the release is due in two to three weeks. Please point out backports you
> > find
> > missing from the respective staging branch, but which you consider
> > relevant.
> 
>  For Arm:
> 
>  e11f576650 ("xen/arm: Fix UBSAN failure in start_xen()")
> >>>
> >>> Which I assume you or Stefano will take care of?
> >>
> >> I was expecting Stefano would do it as he did the backports in the past.
> 
> Was it deliberate that you did not also put it on the 4.17 branch?

Yes, I wasn't sure how far back we want to backport so I thought it
would be better to check with Julien and Michal.

Re: [PATCH v5 23/23] xen/README: add compiler and binutils versions for RISC-V64

[Stefano Stabellini]

On Thu, 29 Feb 2024, Julien Grall wrote:
> On 29/02/2024 14:07, Jan Beulich wrote:
> > On 29.02.2024 14:44, Julien Grall wrote:
> > > Hi Jan,
> > > 
> > > On 29/02/2024 12:51, Jan Beulich wrote:
> > > > On 29.02.2024 13:32, Julien Grall wrote:
> > > > > On 29/02/2024 12:17, Jan Beulich wrote:
> > > > > > On 29.02.2024 13:05, Andrew Cooper wrote:
> > > > > > > On 29/02/2024 10:23 am, Julien Grall wrote:
> > > > > > > > > > > IOW it is hard for me to see why RISC-V needs stronger
> > > > > > > > > > > restrictions
> > > > > > > > > > > here
> > > > > > > > > > > than other architectures. It ought to be possible to
> > > > > > > > > > > determine a
> > > > > > > > > > > baseline
> > > > > > > > > > > version. Even if taking the desire to have "pause"
> > > > > > > > > > > available as a
> > > > > > > > > > > requirement, gas (and presumably gld) 2.36.1 would already
> > > > > > > > > > > suffice.
> > > > > > > > > > 
> > > > > > > > > > I think we want to bump it on Arm. There are zero reasons to
> > > > > > > > > > try to
> > > > > > > > > > keep
> > > > > > > > > > a lower versions if nobody tests/use it in production.
> > > > > > > > > > 
> > > > > > > > > > I would suggest to do the same on x86. What's the point of
> > > > > > > > > > try to
> > > > > > > > > > support Xen with a 15+ years old compiler?
> > > > > > > > > 
> > > > > > > > > It could have long been bumped if only a proper scheme to
> > > > > > > > > follow for
> > > > > > > > > this and future bumping would have been put forward by anyone
> > > > > > > > > keen on
> > > > > > > > > such bumping, like - see his reply - e.g. Andrew. You may
> > > > > > > > > recall that
> > > > > > > > > this was discussed more than once on meetings, with no real
> > > > > > > > > outcome.
> > > > > > > > > I'm personally not meaning to stand in the way of such bumping
> > > > > > > > > as long
> > > > > > > > > as it's done in a predictable manner, but I'm not keen on
> > > > > > > > > doing so and
> > > > > > > > > hence I don't view it as my obligation to try to invent a
> > > > > > > > > reasonable
> > > > > > > > > scheme. (My personal view is that basic functionality should
> > > > > > > > > be
> > > > > > > > > possible to have virtually everywhere, whereas for advanced
> > > > > > > > > stuff it
> > > > > > > > > is fine to require a more modern tool chain.)
> > > > > > > > 
> > > > > > > > That's one way to see it. The problem with this statement is a
> > > > > > > > user
> > > > > > > > today is mislead to think you can build Xen with any GCC
> > > > > > > > versions
> > > > > > > > since 4.1. I don't believe we can guarantee that and we are
> > > > > > > > exposing
> > > > > > > > our users to unnecessary risk.
> > > > > > > > 
> > > > > > > > In addition to that, I agree with Andrew. This is preventing us
> > > > > > > > to
> > > > > > > > improve our code base and we have to carry hacks for older
> > > > > > > > compilers.
> > > > > > > 
> > > > > > > I don't think anyone here is suggesting that we switch to a
> > > > > > > bleeding-edge-only policy.  But 15y of support is extreme in the
> > > > > > > opposite direction.
> > > > > > > 
> > > > > > > Xen ought to be buildable in the contemporary distros of the day,
> > > > > > > and I
> > > > > > > don't think anyone is going to credibly argue otherwise.
> > > > > > > 
> > > > > > > But, it's also fine for new things to have newer requirements.
> > > > > > > 
> > > > > > > Take CET for example.  I know we have disagreements on exactly how
> > > > > > > it's
> > > > > > > toolchain-conditionalness is implemented, but the basic principle
> > > > > > > of "If
> > > > > > > you want shiny new optional feature $X, you need newer toolchain
> > > > > > > $Y" is
> > > > > > > entirely fine.
> > > > > > > 
> > > > > > > A brand new architecture is exactly the same.  Saying "this is the
> > > > > > > minimum, because it's what we test" doesn't preclude someone
> > > > > > > coming
> > > > > > > along and saying "can we use $N-1 ?  See here it works, and here's
> > > > > > > a
> > > > > > > change to CI test it".
> > > > > > > 
> > > > > > > 
> > > > > > > Anyway, its clear we need to write some policy on this, before
> > > > > > > making
> > > > > > > specific adjustments.  To get started, is there going to be any
> > > > > > > objection whatsoever on some principles which begin as follows:
> > > > > > 
> > > > > > Largely not, but one aspect needs clarifying up front:
> > > > > > 
> > > > > > > * For established architectures, we expect Xen to be buildable on
> > > > > > > the
> > > > > > > common contemporary distros.  (i.e. minima is not newer than
> > > > > > > what's
> > > > > > > available in contemporary distros, without a good reason)
> > > > > > 
> > > > > > What counts as contemporary distro? Still in normal support? LTS?
> > > > > > Yet
> > > > > > more extreme forms?
> > > > > 
> > > > > LTS makes sense. More I am not sure. I am under the impression that
> > > > > people using older distros are 

[xen-4.18-testing test] 184813: tolerable trouble: fail/pass/starved - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-i386-qemuu-rhel6hvm-amd 12 redhat-install fail pass in 184821-retest
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 12 debian-hvm-install fail pass in 
184821-retest
 test-amd64-i386-xl-qemuu-win7-amd64 12 windows-install fail pass in 
184821-retest

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

version targeted for testing:
 xen  498b3624d0ecc1267773e6482fd0b732e90c4511
baseline version:
 xen

Re: [PATCH v4 12/12] xen/spinlock: support higher number of cpus

[Jan Beulich]

On 29.02.2024 18:04, Jürgen Groß wrote:
> On 29.02.24 17:54, Jan Beulich wrote:
>> On 29.02.2024 17:45, Juergen Gross wrote:
>>> On 29.02.24 17:31, Jan Beulich wrote:
 On 29.02.2024 17:29, Jürgen Groß wrote:
> On 29.02.24 16:46, Jan Beulich wrote:
>> On 12.12.2023 10:47, Juergen Gross wrote:
>>> Allow 16 bits per cpu number, which is the limit imposed by
>>> spinlock_tickets_t.
>>>
>>> This will allow up to 65535 cpus, while increasing only the size of
>>> recursive spinlocks in debug builds from 8 to 12 bytes.
>>
>> I think we want to be more conservative here, for the case of there
>> being bugs: The CPU holding a lock may wrongly try to acquire it a
>> 2nd time. That's the 65536th ticket then, wrapping the value.
>
> Is this really a problem? There will be no other cpu left seeing the lock
> as "free" in this case, as all others will be waiting for the head to 
> reach
> their private tail value.

 But isn't said CPU then going to make progress, rather than indefinitely
 spinning on the lock?
>>>
>>> No, I don't think so.
>>
>> Hmm. If CPU0 takes a pristine lock, it'll get ticket 0x. All other
>> CPUs will get tickets 0x0001 ... 0x. Then CPU0 trying to take the lock
> 
> No, they'll get 0x0001 ... 0xfffe (only 65535 cpus are supported).
> 
>> again will get ticket 0x again, which equals what .head still has (no
> 
> And the first cpu will get 0x when trying to get the lock again.

Oh, right. Still a little too close to the boundary for my taste ...

Jan

Re: [XEN PATCH 03/10] x86: address some violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 17:45, Nicola Vetrini wrote:
> On 2024-02-29 17:37, Jan Beulich wrote:
>> On 29.02.2024 16:27, Nicola Vetrini wrote:
>>> --- a/xen/arch/x86/include/asm/irq.h
>>> +++ b/xen/arch/x86/include/asm/irq.h
>>> @@ -179,9 +179,9 @@ void cleanup_domain_irq_mapping(struct domain *d);
>>>  void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, 
>>> emuirq);\
>>>  __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND;   
>>>   \
>>>  })
>>> -#define IRQ_UNBOUND -1
>>> -#define IRQ_PT -2
>>> -#define IRQ_MSI_EMU -3
>>> +#define IRQ_UNBOUND (-1)
>>> +#define IRQ_PT  (-2)
>>> +#define IRQ_MSI_EMU (-3)
>>>
>>>  bool cpu_has_pending_apic_eoi(void);
>>>
>>
>> I'd be happy to ack this change right away.
>>
>>> --- a/xen/arch/x86/usercopy.c
>>> +++ b/xen/arch/x86/usercopy.c
>>> @@ -106,7 +106,7 @@ unsigned int copy_from_guest_ll(void *to, const 
>>> void __user *from, unsigned int
>>>  return n;
>>>  }
>>>
>>> -#if GUARD(1) + 0
>>> +#if GUARD((1)) + 0
>>
>> I don't even understand the need for this one, and nothing is said in
>> the description in that regard. Generally I'm afraid I'm averse to
>> such (seemingly) redundant parentheses in macro invocations.
>>
> 
> It's because
> #define UA_KEEP(args...) args
> #define GUARD UA_KEEP
> 
> which would expand to #if 1 + 0, while the rule demands #if (1) + 0
> I did note in the message after --- that I didn't wanna touch UA_KEEP so 
> I did this instead, which I'm not particularly happy about either. I can 
> remove this and deviate, there is no other issue with GUARD.

Or

#if (GUARD(1) + 0)

? To me at least that's quite a bit less odd. But I guess that still
wouldn't satisfy the rule. Perhaps even

#if (GUARD(1)) + 0

would be a little less odd, albeit there I'd already be on the edge.

Jan

Re: [PATCH v4 12/12] xen/spinlock: support higher number of cpus

[Jürgen Groß]

On 29.02.24 17:54, Jan Beulich wrote:

On 29.02.2024 17:45, Juergen Gross wrote:

On 29.02.24 17:31, Jan Beulich wrote:

On 29.02.2024 17:29, Jürgen Groß wrote:

On 29.02.24 16:46, Jan Beulich wrote:

On 12.12.2023 10:47, Juergen Gross wrote:

Allow 16 bits per cpu number, which is the limit imposed by
spinlock_tickets_t.

This will allow up to 65535 cpus, while increasing only the size of
recursive spinlocks in debug builds from 8 to 12 bytes.


I think we want to be more conservative here, for the case of there
being bugs: The CPU holding a lock may wrongly try to acquire it a
2nd time. That's the 65536th ticket then, wrapping the value.


Is this really a problem? There will be no other cpu left seeing the lock
as "free" in this case, as all others will be waiting for the head to reach
their private tail value.


But isn't said CPU then going to make progress, rather than indefinitely
spinning on the lock?


No, I don't think so.


Hmm. If CPU0 takes a pristine lock, it'll get ticket 0x. All other
CPUs will get tickets 0x0001 ... 0x. Then CPU0 trying to take the lock


No, they'll get 0x0001 ... 0xfffe (only 65535 cpus are supported).


again will get ticket 0x again, which equals what .head still has (no


And the first cpu will get 0x when trying to get the lock again.


unlocks so far), thus signalling the lock to be free when it isn't.


The limit isn't 65535 because of the ticket mechanism, but because of the
rspin mechanism, where we need a "no cpu is owning the lock" value. Without
the recursive locks the limit would be 65536 (or 4096 today).


I think this rather belongs ...


No, that was meant to tell you that without programming bug 65536 cpus would
be perfectly fine for the ticket mechanism, and with bug 65535 cpus are fine.




Therefore my suggestion would be to only (mention) go(ing) up to 32k.


Signed-off-by: Juergen Gross 
---
xen/common/spinlock.c  |  1 +
xen/include/xen/spinlock.h | 18 +-
2 files changed, 10 insertions(+), 9 deletions(-)


Shouldn't this also bump the upper bound of the NR_CPUS range then
in xen/arch/Kconfig?


Fine with me, I can add another patch to the series doing that.


Why not do it right here? The upper bound there is like it is only
because of the restriction that's lifted here.


... here (for having nothing to do with the supposed lack of hanging
that I'm seeing)?


I'd prefer splitting the two instances, but if you prefer it to be in a
single patch, so be it.


I'm not going to insist - if want to do it separately, please do.
Perhaps others would actually prefer it that way ...


Okay. I'll do it in another patch.


Juergen

Re: [PATCH v5 23/23] xen/README: add compiler and binutils versions for RISC-V64

[Oleksii]

On Wed, 2024-02-28 at 23:11 +, Andrew Cooper wrote:
> Furthermore, Linux has regularly been bumping minimum toolchain
> versions
> due to code generation issues, and we'd be foolish not pay attention.
Do they document that?

It looks like their doc is pretty old, because in Documentation/Changes
it is mentioned:
   GNU C  5.1  gcc --version

And RISC-V support in GCC wad added after 7.0 or so...

But there is also the following words:

   This document is originally based on my "Changes" file for 2.0.x
   kernels
   and therefore owes credit to the same people as that file (Jared
   Mauch,
   Axel Boldt, Alessandro Sigala, and countless other users all over
   the
   'net).

Probably the doc wasn't updated for a long time, but at the same time
there is a line with Rust:
   
   Rust (optional)1.62.0   rustc --version
   
~ Oleksii

Re: [PATCH v4 12/12] xen/spinlock: support higher number of cpus

[Jan Beulich]

On 29.02.2024 17:45, Juergen Gross wrote:
> On 29.02.24 17:31, Jan Beulich wrote:
>> On 29.02.2024 17:29, Jürgen Groß wrote:
>>> On 29.02.24 16:46, Jan Beulich wrote:
 On 12.12.2023 10:47, Juergen Gross wrote:
> Allow 16 bits per cpu number, which is the limit imposed by
> spinlock_tickets_t.
>
> This will allow up to 65535 cpus, while increasing only the size of
> recursive spinlocks in debug builds from 8 to 12 bytes.

 I think we want to be more conservative here, for the case of there
 being bugs: The CPU holding a lock may wrongly try to acquire it a
 2nd time. That's the 65536th ticket then, wrapping the value.
>>>
>>> Is this really a problem? There will be no other cpu left seeing the lock
>>> as "free" in this case, as all others will be waiting for the head to reach
>>> their private tail value.
>>
>> But isn't said CPU then going to make progress, rather than indefinitely
>> spinning on the lock?
> 
> No, I don't think so.

Hmm. If CPU0 takes a pristine lock, it'll get ticket 0x. All other
CPUs will get tickets 0x0001 ... 0x. Then CPU0 trying to take the lock
again will get ticket 0x again, which equals what .head still has (no
unlocks so far), thus signalling the lock to be free when it isn't.

> The limit isn't 65535 because of the ticket mechanism, but because of the
> rspin mechanism, where we need a "no cpu is owning the lock" value. Without
> the recursive locks the limit would be 65536 (or 4096 today).

I think this rather belongs ...

 Therefore my suggestion would be to only (mention) go(ing) up to 32k.

> Signed-off-by: Juergen Gross 
> ---
>xen/common/spinlock.c  |  1 +
>xen/include/xen/spinlock.h | 18 +-
>2 files changed, 10 insertions(+), 9 deletions(-)

 Shouldn't this also bump the upper bound of the NR_CPUS range then
 in xen/arch/Kconfig?
>>>
>>> Fine with me, I can add another patch to the series doing that.
>>
>> Why not do it right here? The upper bound there is like it is only
>> because of the restriction that's lifted here.

... here (for having nothing to do with the supposed lack of hanging
that I'm seeing)?

> I'd prefer splitting the two instances, but if you prefer it to be in a
> single patch, so be it.

I'm not going to insist - if want to do it separately, please do.
Perhaps others would actually prefer it that way ...

Jan

Re: [PATCH v5 23/23] xen/README: add compiler and binutils versions for RISC-V64

[Oleksii]

On Thu, 2024-02-29 at 08:58 +0100, Jan Beulich wrote:
> On 28.02.2024 23:58, Julien Grall wrote:
> > On 27/02/2024 07:55, Jan Beulich wrote:
> > > On 26.02.2024 18:39, Oleksii Kurochko wrote:
> > > > This patch doesn't represent a strict lower bound for GCC and
> > > > GNU Binutils; rather, these versions are specifically employed
> > > > by
> > > > the Xen RISC-V container and are anticipated to undergo
> > > > continuous
> > > > testing.
> > > 
> > > Up and until that container would be updated to a newer gcc. I'm
> > > afraid I view this as too weak a criteria,
> > 
> > I disagree. We have to decide a limit at some point. It is sensible
> > to 
> > say that we are only supporting what we can tests. AFAIK, this is
> > what 
> > QEMU has been doing.
> 
> I view qemu as a particularly bad example. They raise their baselines
> far too aggressively for my taste.
> 
> > > IOW it is hard for me to see why RISC-V needs stronger
> > > restrictions here
> > > than other architectures. It ought to be possible to determine a
> > > baseline
> > > version. Even if taking the desire to have "pause" available as a
> > > requirement, gas (and presumably gld) 2.36.1 would already
> > > suffice.
> > 
> > I think we want to bump it on Arm. There are zero reasons to try to
> > keep 
> > a lower versions if nobody tests/use it in production.
> > 
> > I would suggest to do the same on x86. What's the point of try to 
> > support Xen with a 15+ years old compiler?
> 
> It could have long been bumped if only a proper scheme to follow for
> this and future bumping would have been put forward by anyone keen on
> such bumping, like - see his reply - e.g. Andrew. You may recall that
> this was discussed more than once on meetings, with no real outcome.
> I'm personally not meaning to stand in the way of such bumping as
> long
> as it's done in a predictable manner, but I'm not keen on doing so
> and
> hence I don't view it as my obligation to try to invent a reasonable
> scheme. (My personal view is that basic functionality should be
> possible to have virtually everywhere, whereas for advanced stuff it
> is fine to require a more modern tool chain.)
> 
> The one additional concern I've raised in the past is that in the end
> it's not just minimal tool chain versions we rely on, but also other
> core system tools (see the recent move from "which" to "command -v"
> for an example of such a dependency, where luckily it turned out to
> not be an issue that the -v had only become a standard thing at some
> point). While for the tool chain I can arrange for making newer
> versions available, for core system tools I can't. 
Can't we identify the top X most popular Linux distributions ( LTS
versions ) and align Xen's minimal toolchain version with the selected
distributions' minimum toolchain versions?

> Therefore being too
> eager there would mean I can't really / easily (smoke) test Xen
> anymore on ancient hardware every once in a while. When afaict we do
> too little of such testing already anyway, despite not having any
> lower bound on hardware that formally we support running Xen on. (And
> no, upgrading the ancient distros on that ancient hardware is not an
> option for me.)
It seems there is no room for upgrading the toolchain version. This
leads to the question of determining the threshold between maintaining
the version as minimal as possible and deciding to upgrade it.
I understand your situation where you have an ancient hardware that
necessitates the use of older Linux distributions. However, is this a
common use case?

~ Oleksii

Re: [XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-02-29 17:47, Andrew Cooper wrote:

On 29/02/2024 4:21 pm, Nicola Vetrini wrote:

On 2024-02-29 17:10, Andrew Cooper wrote:

On 29/02/2024 3:27 pm, Nicola Vetrini wrote:

diff --git a/xen/include/xen/kconfig.h b/xen/include/xen/kconfig.h
index c25dc0f6c2a9..b7e70289737b 100644
--- a/xen/include/xen/kconfig.h
+++ b/xen/include/xen/kconfig.h
@@ -25,7 +25,7 @@
 #define __ARG_PLACEHOLDER_1 0,
 #define config_enabled(cfg) _config_enabled(cfg)
 #define _config_enabled(value)
__config_enabled(__ARG_PLACEHOLDER_##value)
-#define __config_enabled(arg1_or_junk)
___config_enabled(arg1_or_junk 1, 0)
+#define __config_enabled(arg1_or_junk)
___config_enabled(arg1_or_junk (1), (0))
 #define ___config_enabled(__ignored, val, ...) val


This one hunk I suggest we deviate rather than adjust.  You've subtly
broken it, and it's extreme preprocessor magic in the first place to
turn an absent symbol into a 0.



How so? I did test this because I was very wary of it, but it seemed
to expand fine (either if ((0)) or if ((1)) ). I may of course be
wrong, and it could be deviated regardless.



arg1_or_junk(1) can now be a function or macro expansion depending on
context, where previously it couldn't be.



I see, that would be a latent bug. I do agree on deviating then.

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

Re: [XEN PATCH 05/10] xen/perfc: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-02-29 17:42, Jan Beulich wrote:

On 29.02.2024 16:27, Nicola Vetrini wrote:

--- a/xen/common/perfc.c
+++ b/xen/common/perfc.c
@@ -10,10 +10,10 @@
 #include 
 #include 

-#define PERFCOUNTER( var, name )  { name, TYPE_SINGLE, 0 
},
-#define PERFCOUNTER_ARRAY( var, name, size )  { name, TYPE_ARRAY,  
size },
-#define PERFSTATUS( var, name )   { name, TYPE_S_SINGLE, 
0 },
-#define PERFSTATUS_ARRAY( var, name, size )   { name, TYPE_S_ARRAY,  
size },
+#define PERFCOUNTER( var, name )  { (name), TYPE_SINGLE, 
0 },
+#define PERFCOUNTER_ARRAY( var, name, size )  { (name), TYPE_ARRAY,  
(size) },
+#define PERFSTATUS( var, name )   { (name), 
TYPE_S_SINGLE, 0 },
+#define PERFSTATUS_ARRAY( var, name, size )   { (name), TYPE_S_ARRAY, 
 (size) },


Same question as for patch 4. Plus if this needed touching, then the 
stray
blanks immediately inside the parentheses would please be dropped as 
well.




Noted

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

Re: [XEN PATCH 04/10] xen/public: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-02-29 17:40, Jan Beulich wrote:

On 29.02.2024 16:27, Nicola Vetrini wrote:

--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -988,7 +988,7 @@ typedef struct {
   ((b) >>  8) & 0xFF, ((b) >>  0) & 0xFF, 
  \
   ((c) >>  8) & 0xFF, ((c) >>  0) & 0xFF, 
  \
   ((d) >>  8) & 0xFF, ((d) >>  0) & 0xFF, 
  \

-e1, e2, e3, e4, e5, e6}}
+(e1), (e2), (e3), (e4), (e5), (e6)}}


Why? Wasn't it agreed already that long macro arguments passed on
(no matter whether to a function, a macro, or like used here) don't
need parenthesizing?



That applies to all outermost macro invocations, but not to the 
innermost one. If you want also aggregate initalizers to be deviated, 
that could be done (provided that the macro arg is not included in some 
expression, such as "{..., e1 + 1, ...}"


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

Re: [XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

[Andrew Cooper]

On 29/02/2024 4:21 pm, Nicola Vetrini wrote:
> On 2024-02-29 17:10, Andrew Cooper wrote:
>> On 29/02/2024 3:27 pm, Nicola Vetrini wrote:
>>> diff --git a/xen/include/xen/kconfig.h b/xen/include/xen/kconfig.h
>>> index c25dc0f6c2a9..b7e70289737b 100644
>>> --- a/xen/include/xen/kconfig.h
>>> +++ b/xen/include/xen/kconfig.h
>>> @@ -25,7 +25,7 @@
>>>  #define __ARG_PLACEHOLDER_1 0,
>>>  #define config_enabled(cfg) _config_enabled(cfg)
>>>  #define _config_enabled(value)
>>> __config_enabled(__ARG_PLACEHOLDER_##value)
>>> -#define __config_enabled(arg1_or_junk)
>>> ___config_enabled(arg1_or_junk 1, 0)
>>> +#define __config_enabled(arg1_or_junk)
>>> ___config_enabled(arg1_or_junk (1), (0))
>>>  #define ___config_enabled(__ignored, val, ...) val
>>
>> This one hunk I suggest we deviate rather than adjust.  You've subtly
>> broken it, and it's extreme preprocessor magic in the first place to
>> turn an absent symbol into a 0.
>>
>
> How so? I did test this because I was very wary of it, but it seemed
> to expand fine (either if ((0)) or if ((1)) ). I may of course be
> wrong, and it could be deviated regardless.
>

arg1_or_junk(1) can now be a function or macro expansion depending on
context, where previously it couldn't be.

~Andrew

Re: [XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 17:40, Nicola Vetrini wrote:
> On 2024-02-29 17:25, Jan Beulich wrote:
>> On 29.02.2024 16:27, Nicola Vetrini wrote:
>>> --- a/xen/include/xen/kconfig.h
>>> +++ b/xen/include/xen/kconfig.h
>>> @@ -25,7 +25,7 @@
>>>  #define __ARG_PLACEHOLDER_1 0,
>>>  #define config_enabled(cfg) _config_enabled(cfg)
>>>  #define _config_enabled(value) 
>>> __config_enabled(__ARG_PLACEHOLDER_##value)
>>> -#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 
>>> 1, 0)
>>> +#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 
>>> (1), (0))
>>>  #define ___config_enabled(__ignored, val, ...) val
>>
>> In addition to what Andrew said, would you mind clarifying what exactly 
>> the
>> violation is here? I find it questionable that numeric literals need
>> parenthesizing; they don't normally need to, aynwhere.
>>
> 
> This one's a little special. I couldn't parenthesize val further down, 
> because then it would break the build:
> 
> In file included from ././include/xen/config.h:14,
>   from :
> ./include/xen/kconfig.h:29:52: error: expected identifier or ‘(’ before 
> ‘)’ token
> 29 | #define ___config_enabled(__ignored, val, ...) (val)
>|^
> ./include/xen/kconfig.h:39:34: note: in expansion of macro 
> ‘___config_enabled’
> 39 | #define _static_if(arg1_or_junk) ___config_enabled(arg1_or_junk 
> static,)
>|  ^
> ./include/xen/kconfig.h:38:26: note: in expansion of macro ‘_static_if’
> 38 | #define static_if(value) _static_if(__ARG_PLACEHOLDER_##value)
>|  ^~
> ./include/xen/kconfig.h:41:27: note: in expansion of macro ‘static_if’
> 41 | #define STATIC_IF(option) static_if(option)
>|   ^
> common/page_alloc.c:260:1: note: in expansion of macro ‘STATIC_IF’
>260 | STATIC_IF(CONFIG_NUMA) mfn_t first_valid_mfn = 
> INVALID_MFN_INITIALIZER;
>| ^
> make[2]: *** [Rules.mk:249: common/page_alloc.o] Error 1

So if we're not gong to deviate the construct, then this change needs to
come entirely on its own, with a really good description.

>>> --- a/xen/include/xen/list.h
>>> +++ b/xen/include/xen/list.h
>>> @@ -490,9 +490,9 @@ static inline void list_splice_init(struct 
>>> list_head *list,
>>>   * @member: the name of the list_struct within the struct.
>>>   */
>>>  #define list_for_each_entry(pos, head, member)
>>>   \
>>> -for (pos = list_entry((head)->next, typeof(*pos), member);
>>>   \
>>> - >member != (head);  
>>>   \
>>> - pos = list_entry(pos->member.next, typeof(*pos), member))
>>> +for (pos = list_entry((head)->next, typeof(*(pos)), member);  
>>> \
>>> + &(pos)->member != (head);
>>>   \
>>> + pos = list_entry((pos)->member.next, typeof(*(pos)), 
>>> member))
>>
>> this ends up inconsistent, which I think isn't nice: Some uses of "pos"
>> are now parenthesized, while others aren't. Applies further down as 
>> well.
>>
> 
> Yeah, the inconsistency is due to the fact that a non-parenthesized 
> parameter as lhs is already deviated. To keep it consistent here I can 
> add parentheses, but then the deviation would be kind of pointless, 
> wouldn't it?

I don't think so: It would still be useful for cases where a macro
parameter is used solely as the lhs of some kind of assignment
operator. But yes, before making adjustments you will want to wait
for possible further comments, especially from e.g. Julien, who iirc
was primarily against this kind of parenthesization.

Jan

Re: [PATCH v4 12/12] xen/spinlock: support higher number of cpus

[Juergen Gross]

On 29.02.24 17:31, Jan Beulich wrote:

On 29.02.2024 17:29, Jürgen Groß wrote:

On 29.02.24 16:46, Jan Beulich wrote:

On 12.12.2023 10:47, Juergen Gross wrote:

Allow 16 bits per cpu number, which is the limit imposed by
spinlock_tickets_t.

This will allow up to 65535 cpus, while increasing only the size of
recursive spinlocks in debug builds from 8 to 12 bytes.


I think we want to be more conservative here, for the case of there
being bugs: The CPU holding a lock may wrongly try to acquire it a
2nd time. That's the 65536th ticket then, wrapping the value.


Is this really a problem? There will be no other cpu left seeing the lock
as "free" in this case, as all others will be waiting for the head to reach
their private tail value.


But isn't said CPU then going to make progress, rather than indefinitely
spinning on the lock?


No, I don't think so.

The limit isn't 65535 because of the ticket mechanism, but because of the
rspin mechanism, where we need a "no cpu is owning the lock" value. Without
the recursive locks the limit would be 65536 (or 4096 today).




Therefore my suggestion would be to only (mention) go(ing) up to 32k.


Signed-off-by: Juergen Gross 
---
   xen/common/spinlock.c  |  1 +
   xen/include/xen/spinlock.h | 18 +-
   2 files changed, 10 insertions(+), 9 deletions(-)


Shouldn't this also bump the upper bound of the NR_CPUS range then
in xen/arch/Kconfig?


Fine with me, I can add another patch to the series doing that.


Why not do it right here? The upper bound there is like it is only
because of the restriction that's lifted here.


I'd prefer splitting the two instances, but if you prefer it to be in a
single patch, so be it.


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [XEN PATCH 03/10] x86: address some violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-02-29 17:37, Jan Beulich wrote:

On 29.02.2024 16:27, Nicola Vetrini wrote:

--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -179,9 +179,9 @@ void cleanup_domain_irq_mapping(struct domain *d);
 void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, 
emuirq);\
 __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND;   
  \

 })
-#define IRQ_UNBOUND -1
-#define IRQ_PT -2
-#define IRQ_MSI_EMU -3
+#define IRQ_UNBOUND (-1)
+#define IRQ_PT  (-2)
+#define IRQ_MSI_EMU (-3)

 bool cpu_has_pending_apic_eoi(void);



I'd be happy to ack this change right away.


--- a/xen/arch/x86/usercopy.c
+++ b/xen/arch/x86/usercopy.c
@@ -106,7 +106,7 @@ unsigned int copy_from_guest_ll(void *to, const 
void __user *from, unsigned int

 return n;
 }

-#if GUARD(1) + 0
+#if GUARD((1)) + 0


I don't even understand the need for this one, and nothing is said in
the description in that regard. Generally I'm afraid I'm averse to
such (seemingly) redundant parentheses in macro invocations.



It's because
#define UA_KEEP(args...) args
#define GUARD UA_KEEP

which would expand to #if 1 + 0, while the rule demands #if (1) + 0
I did note in the message after --- that I didn't wanna touch UA_KEEP so 
I did this instead, which I'm not particularly happy about either. I can 
remove this and deviate, there is no other issue with GUARD.

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

Re: [XEN PATCH 05/10] xen/perfc: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 16:27, Nicola Vetrini wrote:
> --- a/xen/common/perfc.c
> +++ b/xen/common/perfc.c
> @@ -10,10 +10,10 @@
>  #include 
>  #include 
>  
> -#define PERFCOUNTER( var, name )  { name, TYPE_SINGLE, 0 },
> -#define PERFCOUNTER_ARRAY( var, name, size )  { name, TYPE_ARRAY,  size },
> -#define PERFSTATUS( var, name )   { name, TYPE_S_SINGLE, 0 },
> -#define PERFSTATUS_ARRAY( var, name, size )   { name, TYPE_S_ARRAY,  size },
> +#define PERFCOUNTER( var, name )  { (name), TYPE_SINGLE, 0 },
> +#define PERFCOUNTER_ARRAY( var, name, size )  { (name), TYPE_ARRAY,  (size) 
> },
> +#define PERFSTATUS( var, name )   { (name), TYPE_S_SINGLE, 0 },
> +#define PERFSTATUS_ARRAY( var, name, size )   { (name), TYPE_S_ARRAY,  
> (size) },

Same question as for patch 4. Plus if this needed touching, then the stray
blanks immediately inside the parentheses would please be dropped as well.

Jan

Re: [XEN PATCH 04/10] xen/public: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 16:27, Nicola Vetrini wrote:
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -988,7 +988,7 @@ typedef struct {
>((b) >>  8) & 0xFF, ((b) >>  0) & 0xFF,   \
>((c) >>  8) & 0xFF, ((c) >>  0) & 0xFF,   \
>((d) >>  8) & 0xFF, ((d) >>  0) & 0xFF,   \
> -e1, e2, e3, e4, e5, e6}}
> +(e1), (e2), (e3), (e4), (e5), (e6)}}

Why? Wasn't it agreed already that long macro arguments passed on
(no matter whether to a function, a macro, or like used here) don't
need parenthesizing?

Jan

Re: [XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-02-29 17:25, Jan Beulich wrote:

On 29.02.2024 16:27, Nicola Vetrini wrote:

--- a/xen/include/xen/kconfig.h
+++ b/xen/include/xen/kconfig.h
@@ -25,7 +25,7 @@
 #define __ARG_PLACEHOLDER_1 0,
 #define config_enabled(cfg) _config_enabled(cfg)
 #define _config_enabled(value) 
__config_enabled(__ARG_PLACEHOLDER_##value)
-#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 
1, 0)
+#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 
(1), (0))

 #define ___config_enabled(__ignored, val, ...) val


In addition to what Andrew said, would you mind clarifying what exactly 
the

violation is here? I find it questionable that numeric literals need
parenthesizing; they don't normally need to, aynwhere.



This one's a little special. I couldn't parenthesize val further down, 
because then it would break the build:


In file included from ././include/xen/config.h:14,
 from :
./include/xen/kconfig.h:29:52: error: expected identifier or ‘(’ before 
‘)’ token

   29 | #define ___config_enabled(__ignored, val, ...) (val)
  |^
./include/xen/kconfig.h:39:34: note: in expansion of macro 
‘___config_enabled’
   39 | #define _static_if(arg1_or_junk) ___config_enabled(arg1_or_junk 
static,)

  |  ^
./include/xen/kconfig.h:38:26: note: in expansion of macro ‘_static_if’
   38 | #define static_if(value) _static_if(__ARG_PLACEHOLDER_##value)
  |  ^~
./include/xen/kconfig.h:41:27: note: in expansion of macro ‘static_if’
   41 | #define STATIC_IF(option) static_if(option)
  |   ^
common/page_alloc.c:260:1: note: in expansion of macro ‘STATIC_IF’
  260 | STATIC_IF(CONFIG_NUMA) mfn_t first_valid_mfn = 
INVALID_MFN_INITIALIZER;

  | ^
make[2]: *** [Rules.mk:249: common/page_alloc.o] Error 1



--- a/xen/include/xen/list.h
+++ b/xen/include/xen/list.h
@@ -490,9 +490,9 @@ static inline void list_splice_init(struct 
list_head *list,

  * @member: the name of the list_struct within the struct.
  */
 #define list_for_each_entry(pos, head, member)
  \
-for (pos = list_entry((head)->next, typeof(*pos), member);
  \
- >member != (head);  
  \

- pos = list_entry(pos->member.next, typeof(*pos), member))
+for (pos = list_entry((head)->next, typeof(*(pos)), member);  
\
+ &(pos)->member != (head);
  \
+ pos = list_entry((pos)->member.next, typeof(*(pos)), 
member))


this ends up inconsistent, which I think isn't nice: Some uses of "pos"
are now parenthesized, while others aren't. Applies further down as 
well.




Yeah, the inconsistency is due to the fact that a non-parenthesized 
parameter as lhs is already deviated. To keep it consistent here I can 
add parentheses, but then the deviation would be kind of pointless, 
wouldn't it?


You may also want to take this as a strong suggestion to split 
dissimilar

changes, so uncontroversial parts can go in.



Ok, that was an oversight.

@@ -977,4 +977,3 @@ static inline void hlist_add_after_rcu(struct 
hlist_node *prev,

   pos = pos->next)

 #endif /* __XEN_LIST_H__ */
-


Unrelated change?



Oh, yes. Will drop it.


--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -94,7 +94,7 @@ struct lock_profile_qhead {
 int32_t   idx; /* index for printout */
 };

-#define _LOCK_PROFILE(lockname) { .name = #lockname, .lock = 
, }
+#define _LOCK_PROFILE(lockname) { .name = #lockname, .lock = 
&(lockname), }


This also may be viewed as falling in the same category, but is less
problematic because the other use is stringification, when in principle
some kind of expression would be passed in (albeit in practice I don't
expect anyone would do that).



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

Re: [XEN PATCH 03/10] x86: address some violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 16:27, Nicola Vetrini wrote:
> --- a/xen/arch/x86/include/asm/irq.h
> +++ b/xen/arch/x86/include/asm/irq.h
> @@ -179,9 +179,9 @@ void cleanup_domain_irq_mapping(struct domain *d);
>  void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, emuirq);\
>  __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND; \
>  })
> -#define IRQ_UNBOUND -1
> -#define IRQ_PT -2
> -#define IRQ_MSI_EMU -3
> +#define IRQ_UNBOUND (-1)
> +#define IRQ_PT  (-2)
> +#define IRQ_MSI_EMU (-3)
>  
>  bool cpu_has_pending_apic_eoi(void);
>  

I'd be happy to ack this change right away.

> --- a/xen/arch/x86/usercopy.c
> +++ b/xen/arch/x86/usercopy.c
> @@ -106,7 +106,7 @@ unsigned int copy_from_guest_ll(void *to, const void 
> __user *from, unsigned int
>  return n;
>  }
>  
> -#if GUARD(1) + 0
> +#if GUARD((1)) + 0

I don't even understand the need for this one, and nothing is said in
the description in that regard. Generally I'm afraid I'm averse to
such (seemingly) redundant parentheses in macro invocations.

Jan

Re: [XEN PATCH 02/10] xen/arm: address some violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 16:27, Nicola Vetrini wrote:
> --- a/xen/arch/arm/cpuerrata.c
> +++ b/xen/arch/arm/cpuerrata.c
> @@ -462,8 +462,8 @@ static bool has_ssbd_mitigation(const struct 
> arm_cpu_capabilities *entry)
>  #define MIDR_RANGE(model, min, max) \
>  .matches = is_affected_midr_range,  \
>  .midr_model = model,\
> -.midr_range_min = min,  \
> -.midr_range_max = max
> +.midr_range_min = (min),\
> +.midr_range_max = (max)

Why min and max, but not model?

> --- a/xen/arch/arm/include/asm/smccc.h
> +++ b/xen/arch/arm/include/asm/smccc.h
> @@ -122,7 +122,7 @@ struct arm_smccc_res {
>  #define __constraint_read_7 __constraint_read_6, "r" (r7)
>  
>  #define __declare_arg_0(a0, res)\
> -struct arm_smccc_res*___res = res;  \
> +struct arm_smccc_res*___res = (res);\
>  register unsigned long  r0 ASM_REG(0) = (uint32_t)a0;   \

Why res but not a0?

> --- a/xen/arch/arm/include/asm/vgic-emul.h
> +++ b/xen/arch/arm/include/asm/vgic-emul.h
> @@ -6,11 +6,11 @@
>   * a range of registers
>   */
>  
> -#define VREG32(reg) reg ... reg + 3
> -#define VREG64(reg) reg ... reg + 7
> +#define VREG32(reg) (reg) ... (reg) + 3
> +#define VREG64(reg) (reg) ... (reg) + 7

#define VREG32(reg) (reg) ... ((reg) + 3)
#define VREG64(reg) (reg) ... ((reg) + 7)

?

Jan

Re: [PATCH v4 12/12] xen/spinlock: support higher number of cpus

[Jan Beulich]

On 29.02.2024 17:29, Jürgen Groß wrote:
> On 29.02.24 16:46, Jan Beulich wrote:
>> On 12.12.2023 10:47, Juergen Gross wrote:
>>> Allow 16 bits per cpu number, which is the limit imposed by
>>> spinlock_tickets_t.
>>>
>>> This will allow up to 65535 cpus, while increasing only the size of
>>> recursive spinlocks in debug builds from 8 to 12 bytes.
>>
>> I think we want to be more conservative here, for the case of there
>> being bugs: The CPU holding a lock may wrongly try to acquire it a
>> 2nd time. That's the 65536th ticket then, wrapping the value.
> 
> Is this really a problem? There will be no other cpu left seeing the lock
> as "free" in this case, as all others will be waiting for the head to reach
> their private tail value.

But isn't said CPU then going to make progress, rather than indefinitely
spinning on the lock?

>> Therefore my suggestion would be to only (mention) go(ing) up to 32k.
>>
>>> Signed-off-by: Juergen Gross 
>>> ---
>>>   xen/common/spinlock.c  |  1 +
>>>   xen/include/xen/spinlock.h | 18 +-
>>>   2 files changed, 10 insertions(+), 9 deletions(-)
>>
>> Shouldn't this also bump the upper bound of the NR_CPUS range then
>> in xen/arch/Kconfig?
> 
> Fine with me, I can add another patch to the series doing that.

Why not do it right here? The upper bound there is like it is only
because of the restriction that's lifted here.

Jan

Re: [PATCH v4 12/12] xen/spinlock: support higher number of cpus

[Jürgen Groß]

On 29.02.24 16:46, Jan Beulich wrote:

On 12.12.2023 10:47, Juergen Gross wrote:

Allow 16 bits per cpu number, which is the limit imposed by
spinlock_tickets_t.

This will allow up to 65535 cpus, while increasing only the size of
recursive spinlocks in debug builds from 8 to 12 bytes.


I think we want to be more conservative here, for the case of there
being bugs: The CPU holding a lock may wrongly try to acquire it a
2nd time. That's the 65536th ticket then, wrapping the value.


Is this really a problem? There will be no other cpu left seeing the lock
as "free" in this case, as all others will be waiting for the head to reach
their private tail value.


Therefore my suggestion would be to only (mention) go(ing) up to 32k.


Signed-off-by: Juergen Gross 
---
  xen/common/spinlock.c  |  1 +
  xen/include/xen/spinlock.h | 18 +-
  2 files changed, 10 insertions(+), 9 deletions(-)


Shouldn't this also bump the upper bound of the NR_CPUS range then
in xen/arch/Kconfig?


Fine with me, I can add another patch to the series doing that.


Juergen

Re: [PATCH v5 03/23] xen/riscv: introduce nospec.h

[Jan Beulich]

On 26.02.2024 18:38, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/nospec.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2024 Vates */
> +
> +#ifndef _ASM_GENERIC_NOSPEC_H
> +#define _ASM_GENERIC_NOSPEC_H

Btw, at the very last second I noticed the GENERIC in here, which I
took the liberty to replace. But please be more careful when moving
files around in the tree.

Jan

Re: [PATCH v5 04/23] xen/asm-generic: introduce generic fls() and flsl() functions

[Andrew Cooper]

On 26/02/2024 5:38 pm, Oleksii Kurochko wrote:
> These functions can be useful for architectures that don't
> have corresponding arch-specific instructions.
>
> Signed-off-by: Oleksii Kurochko 
> ---
>  Changes in V5:
>- new patch
> ---
>  xen/include/asm-generic/bitops/fls.h  | 18 ++
>  xen/include/asm-generic/bitops/flsl.h | 10 ++
>  2 files changed, 28 insertions(+)
>  create mode 100644 xen/include/asm-generic/bitops/fls.h
>  create mode 100644 xen/include/asm-generic/bitops/flsl.h
>
> diff --git a/xen/include/asm-generic/bitops/fls.h 
> b/xen/include/asm-generic/bitops/fls.h
> new file mode 100644
> index 00..369a4c790c
> --- /dev/null
> +++ b/xen/include/asm-generic/bitops/fls.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_GENERIC_BITOPS_FLS_H_
> +#define _ASM_GENERIC_BITOPS_FLS_H_
> +
> +/**
> + * fls - find last (most-significant) bit set
> + * @x: the word to search
> + *
> + * This is defined the same way as ffs.
> + * Note fls(0) = 0, fls(1) = 1, fls(0x8000) = 32.
> + */
> +
> +static inline int fls(unsigned int x)
> +{
> +return generic_fls(x);
> +}
> +
> +#endif /* _ASM_GENERIC_BITOPS_FLS_H_ */
> diff --git a/xen/include/asm-generic/bitops/flsl.h 
> b/xen/include/asm-generic/bitops/flsl.h
> new file mode 100644
> index 00..d0a2e9c729
> --- /dev/null
> +++ b/xen/include/asm-generic/bitops/flsl.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_GENERIC_BITOPS_FLSL_H_
> +#define _ASM_GENERIC_BITOPS_FLSL_H_
> +
> +static inline int flsl(unsigned long x)
> +{
> +return generic_flsl(x);
> +}
> +
> +#endif /* _ASM_GENERIC_BITOPS_FLSL_H_ */

Please don't do this.  It's compounding existing problems we have with
bitops, and there's a way to simplify things instead.

If you can wait a couple of days, I'll see about finishing and posting
my prototype demonstrating a simplification across all architectures,
and a reduction of code overall.

~Andrew

Re: [XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 16:27, Nicola Vetrini wrote:
> --- a/xen/include/xen/kconfig.h
> +++ b/xen/include/xen/kconfig.h
> @@ -25,7 +25,7 @@
>  #define __ARG_PLACEHOLDER_1 0,
>  #define config_enabled(cfg) _config_enabled(cfg)
>  #define _config_enabled(value) __config_enabled(__ARG_PLACEHOLDER_##value)
> -#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 1, 0)
> +#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk (1), 
> (0))
>  #define ___config_enabled(__ignored, val, ...) val

In addition to what Andrew said, would you mind clarifying what exactly the
violation is here? I find it questionable that numeric literals need
parenthesizing; they don't normally need to, aynwhere.

> --- a/xen/include/xen/list.h
> +++ b/xen/include/xen/list.h
> @@ -490,9 +490,9 @@ static inline void list_splice_init(struct list_head 
> *list,
>   * @member: the name of the list_struct within the struct.
>   */
>  #define list_for_each_entry(pos, head, member)  \
> -for (pos = list_entry((head)->next, typeof(*pos), member);  \
> - >member != (head);\
> - pos = list_entry(pos->member.next, typeof(*pos), member))
> +for (pos = list_entry((head)->next, typeof(*(pos)), member);  \
> + &(pos)->member != (head);  \
> + pos = list_entry((pos)->member.next, typeof(*(pos)), member))

this ends up inconsistent, which I think isn't nice: Some uses of "pos"
are now parenthesized, while others aren't. Applies further down as well.

You may also want to take this as a strong suggestion to split dissimilar
changes, so uncontroversial parts can go in.

> @@ -977,4 +977,3 @@ static inline void hlist_add_after_rcu(struct hlist_node 
> *prev,
>pos = pos->next)
>  
>  #endif /* __XEN_LIST_H__ */
> -

Unrelated change?

> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> @@ -94,7 +94,7 @@ struct lock_profile_qhead {
>  int32_t   idx; /* index for printout */
>  };
>  
> -#define _LOCK_PROFILE(lockname) { .name = #lockname, .lock = , }
> +#define _LOCK_PROFILE(lockname) { .name = #lockname, .lock = &(lockname), }

This also may be viewed as falling in the same category, but is less
problematic because the other use is stringification, when in principle
some kind of expression would be passed in (albeit in practice I don't
expect anyone would do that).

Jan

Re: [XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-02-29 17:10, Andrew Cooper wrote:

On 29/02/2024 3:27 pm, Nicola Vetrini wrote:

diff --git a/xen/include/xen/kconfig.h b/xen/include/xen/kconfig.h
index c25dc0f6c2a9..b7e70289737b 100644
--- a/xen/include/xen/kconfig.h
+++ b/xen/include/xen/kconfig.h
@@ -25,7 +25,7 @@
 #define __ARG_PLACEHOLDER_1 0,
 #define config_enabled(cfg) _config_enabled(cfg)
 #define _config_enabled(value) 
__config_enabled(__ARG_PLACEHOLDER_##value)
-#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 
1, 0)
+#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 
(1), (0))

 #define ___config_enabled(__ignored, val, ...) val


This one hunk I suggest we deviate rather than adjust.  You've subtly
broken it, and it's extreme preprocessor magic in the first place to
turn an absent symbol into a 0.



How so? I did test this because I was very wary of it, but it seemed to 
expand fine (either if ((0)) or if ((1)) ). I may of course be wrong, 
and it could be deviated regardless.


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

Re: [PATCH v5 04/23] xen/asm-generic: introduce generic fls() and flsl() functions

[Oleksii]

Hi Julien,

On Thu, 2024-02-29 at 13:54 +, Julien Grall wrote:
> Hi Oleksii,
> 
> On 26/02/2024 17:38, Oleksii Kurochko wrote:
> > These functions can be useful for architectures that don't
> > have corresponding arch-specific instructions.
> > 
> > Signed-off-by: Oleksii Kurochko 
> > ---
> >   Changes in V5:
> >     - new patch
> > ---
> >   xen/include/asm-generic/bitops/fls.h  | 18 ++
> >   xen/include/asm-generic/bitops/flsl.h | 10 ++
> 
> One header per function seems a little bit excessive to me. Do you
> have 
> any pointer where this request is coming from?
The goal was to be in sync with Linux kernel as Jan mentioned.
I will update the commit message as you suggested in one of replies.

> 
> Why not using the pattern.
> 
> In arch implementation:
> 
> #define fls
> static inline ...
> 
> In the generic header (asm-generic or xen/):
> 
> #ifndef fls
> static inline ...
> #endif
> 
> >   2 files changed, 28 insertions(+)
> >   create mode 100644 xen/include/asm-generic/bitops/fls.h
> >   create mode 100644 xen/include/asm-generic/bitops/flsl.h
> > 
> > diff --git a/xen/include/asm-generic/bitops/fls.h
> > b/xen/include/asm-generic/bitops/fls.h
> > new file mode 100644
> > index 00..369a4c790c
> > --- /dev/null
> > +++ b/xen/include/asm-generic/bitops/fls.h
> > @@ -0,0 +1,18 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> 
> You should use GPL-2.0-only.
Sure, I'll update the license here and in other files. I automatically
copied this SPDX from Linux kernel.

> 
> > +#ifndef _ASM_GENERIC_BITOPS_FLS_H_
> > +#define _ASM_GENERIC_BITOPS_FLS_H_
> > +
> > +/**
> > + * fls - find last (most-significant) bit set
> > + * @x: the word to search
> > + *
> > + * This is defined the same way as ffs.
> > + * Note fls(0) = 0, fls(1) = 1, fls(0x8000) = 32.
> > + */
> > +
> > +static inline int fls(unsigned int x)
> > +{
> > +    return generic_fls(x);
> > +}
> > +
> > +#endif /* _ASM_GENERIC_BITOPS_FLS_H_ */
> 
> Missing emacs magic. I am probably not going to repeat this remark
> and 
> the one above again. So please have a look.
Sure, I'll update files with emacs magic.

~ Oleksii
> 
> > diff --git a/xen/include/asm-generic/bitops/flsl.h
> > b/xen/include/asm-generic/bitops/flsl.h
> > new file mode 100644
> > index 00..d0a2e9c729
> > --- /dev/null
> > +++ b/xen/include/asm-generic/bitops/flsl.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _ASM_GENERIC_BITOPS_FLSL_H_
> > +#define _ASM_GENERIC_BITOPS_FLSL_H_
> > +
> > +static inline int flsl(unsigned long x)
> > +{
> > +    return generic_flsl(x);
> > +}
> > +
> > +#endif /* _ASM_GENERIC_BITOPS_FLSL_H_ */
> 
> Cheers,
> 

Re: [XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

[Andrew Cooper]

On 29/02/2024 3:27 pm, Nicola Vetrini wrote:
> diff --git a/xen/include/xen/kconfig.h b/xen/include/xen/kconfig.h
> index c25dc0f6c2a9..b7e70289737b 100644
> --- a/xen/include/xen/kconfig.h
> +++ b/xen/include/xen/kconfig.h
> @@ -25,7 +25,7 @@
>  #define __ARG_PLACEHOLDER_1 0,
>  #define config_enabled(cfg) _config_enabled(cfg)
>  #define _config_enabled(value) __config_enabled(__ARG_PLACEHOLDER_##value)
> -#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 1, 0)
> +#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk (1), 
> (0))
>  #define ___config_enabled(__ignored, val, ...) val

This one hunk I suggest we deviate rather than adjust.  You've subtly
broken it, and it's extreme preprocessor magic in the first place to
turn an absent symbol into a 0.

~Andrew

Re: [PATCH v5 03/23] xen/riscv: introduce nospec.h

[Oleksii]

On Thu, 2024-02-29 at 15:01 +0100, Jan Beulich wrote:
> On 29.02.2024 14:49, Julien Grall wrote:
> > On 26/02/2024 17:38, Oleksii Kurochko wrote:
> > > --- /dev/null
> > > +++ b/xen/arch/riscv/include/asm/nospec.h
> > > @@ -0,0 +1,25 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > 
> > New file should use the SPDX tag GPL-2.0-only. I guess this could
> > be 
> > fixed on commit?
> 
> I wouldn't mind doing so.
I would happy with that. Thanks.

~ Oleksii

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

[Christoph Hellwig]

On Tue, Feb 27, 2024 at 05:31:28PM -0800, Alexei Starovoitov wrote:
> What would it look like with a cookie?
> A static inline wrapper around get_vm_area() that returns area->addr ?
> And the start address of vmap range will be such a cookie?

Hmm, just making the kernel virtual address the cookie actually
sounds pretty neat indeed even if I did not have that in mind.

> I guess I don't understand the motivation to hide 'struct vm_struct *'.

The prime reason is that then people will try to start random APIs that
work on it.  But let's give it a try without the wrappers and see how
things go.

Re: [PATCH v5 04/23] xen/asm-generic: introduce generic fls() and flsl() functions

[Jan Beulich]

On 26.02.2024 18:38, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/include/asm-generic/bitops/fls.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_GENERIC_BITOPS_FLS_H_
> +#define _ASM_GENERIC_BITOPS_FLS_H_
> +
> +/**
> + * fls - find last (most-significant) bit set
> + * @x: the word to search
> + *
> + * This is defined the same way as ffs.
> + * Note fls(0) = 0, fls(1) = 1, fls(0x8000) = 32.
> + */
> +
> +static inline int fls(unsigned int x)
> +{
> +return generic_fls(x);
> +}

This being an inline function, it requires generic_fls() to be declared.
Yet there's no other header included here. I think these headers would
better be self-contained. Or else (e.g. because of this leading to an
#include cycle) something needs saying somewhere.

The other thing here that worries me is the use of plain int as return
type. Yes, generic_fls() is declared like that, too. But no, the return
value there or here cannot be negative.

Jan

[PATCH AUTOSEL 4.19 2/3] x86/xen: Add some null pointer checking to smp.c

[Sasha Levin]

From: Kunwu Chan 

[ Upstream commit 3693bb4465e6e32a204a5b86d3ec7e6b9f7e67c2 ]

kasprintf() returns a pointer to dynamically allocated memory
which can be NULL upon failure. Ensure the allocation was successful
by checking the pointer validity.

Signed-off-by: Kunwu Chan 
Reported-by: kernel test robot 
Closes: 
https://lore.kernel.org/oe-kbuild-all/202401161119.iof6bqsf-...@intel.com/
Suggested-by: Markus Elfring 
Reviewed-by: Juergen Gross 
Link: https://lore.kernel.org/r/20240119094948.275390-1-chen...@kylinos.cn
Signed-off-by: Juergen Gross 
Signed-off-by: Sasha Levin 
---
 arch/x86/xen/smp.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index a1cc855c539c1..a76ba342a6695 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -65,6 +65,8 @@ int xen_smp_intr_init(unsigned int cpu)
char *resched_name, *callfunc_name, *debug_name;
 
resched_name = kasprintf(GFP_KERNEL, "resched%d", cpu);
+   if (!resched_name)
+   goto fail_mem;
per_cpu(xen_resched_irq, cpu).name = resched_name;
rc = bind_ipi_to_irqhandler(XEN_RESCHEDULE_VECTOR,
cpu,
@@ -77,6 +79,8 @@ int xen_smp_intr_init(unsigned int cpu)
per_cpu(xen_resched_irq, cpu).irq = rc;
 
callfunc_name = kasprintf(GFP_KERNEL, "callfunc%d", cpu);
+   if (!callfunc_name)
+   goto fail_mem;
per_cpu(xen_callfunc_irq, cpu).name = callfunc_name;
rc = bind_ipi_to_irqhandler(XEN_CALL_FUNCTION_VECTOR,
cpu,
@@ -90,6 +94,9 @@ int xen_smp_intr_init(unsigned int cpu)
 
if (!xen_fifo_events) {
debug_name = kasprintf(GFP_KERNEL, "debug%d", cpu);
+   if (!debug_name)
+   goto fail_mem;
+
per_cpu(xen_debug_irq, cpu).name = debug_name;
rc = bind_virq_to_irqhandler(VIRQ_DEBUG, cpu,
 xen_debug_interrupt,
@@ -101,6 +108,9 @@ int xen_smp_intr_init(unsigned int cpu)
}
 
callfunc_name = kasprintf(GFP_KERNEL, "callfuncsingle%d", cpu);
+   if (!callfunc_name)
+   goto fail_mem;
+
per_cpu(xen_callfuncsingle_irq, cpu).name = callfunc_name;
rc = bind_ipi_to_irqhandler(XEN_CALL_FUNCTION_SINGLE_VECTOR,
cpu,
@@ -114,6 +124,8 @@ int xen_smp_intr_init(unsigned int cpu)
 
return 0;
 
+ fail_mem:
+   rc = -ENOMEM;
  fail:
xen_smp_intr_free(cpu);
return rc;
-- 
2.43.0

[PATCH AUTOSEL 5.10 4/7] x86/xen: Add some null pointer checking to smp.c

[Sasha Levin]

From: Kunwu Chan 

[ Upstream commit 3693bb4465e6e32a204a5b86d3ec7e6b9f7e67c2 ]

kasprintf() returns a pointer to dynamically allocated memory
which can be NULL upon failure. Ensure the allocation was successful
by checking the pointer validity.

Signed-off-by: Kunwu Chan 
Reported-by: kernel test robot 
Closes: 
https://lore.kernel.org/oe-kbuild-all/202401161119.iof6bqsf-...@intel.com/
Suggested-by: Markus Elfring 
Reviewed-by: Juergen Gross 
Link: https://lore.kernel.org/r/20240119094948.275390-1-chen...@kylinos.cn
Signed-off-by: Juergen Gross 
Signed-off-by: Sasha Levin 
---
 arch/x86/xen/smp.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index cdec892b28e2e..a641e0d452194 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -65,6 +65,8 @@ int xen_smp_intr_init(unsigned int cpu)
char *resched_name, *callfunc_name, *debug_name;
 
resched_name = kasprintf(GFP_KERNEL, "resched%d", cpu);
+   if (!resched_name)
+   goto fail_mem;
per_cpu(xen_resched_irq, cpu).name = resched_name;
rc = bind_ipi_to_irqhandler(XEN_RESCHEDULE_VECTOR,
cpu,
@@ -77,6 +79,8 @@ int xen_smp_intr_init(unsigned int cpu)
per_cpu(xen_resched_irq, cpu).irq = rc;
 
callfunc_name = kasprintf(GFP_KERNEL, "callfunc%d", cpu);
+   if (!callfunc_name)
+   goto fail_mem;
per_cpu(xen_callfunc_irq, cpu).name = callfunc_name;
rc = bind_ipi_to_irqhandler(XEN_CALL_FUNCTION_VECTOR,
cpu,
@@ -90,6 +94,9 @@ int xen_smp_intr_init(unsigned int cpu)
 
if (!xen_fifo_events) {
debug_name = kasprintf(GFP_KERNEL, "debug%d", cpu);
+   if (!debug_name)
+   goto fail_mem;
+
per_cpu(xen_debug_irq, cpu).name = debug_name;
rc = bind_virq_to_irqhandler(VIRQ_DEBUG, cpu,
 xen_debug_interrupt,
@@ -101,6 +108,9 @@ int xen_smp_intr_init(unsigned int cpu)
}
 
callfunc_name = kasprintf(GFP_KERNEL, "callfuncsingle%d", cpu);
+   if (!callfunc_name)
+   goto fail_mem;
+
per_cpu(xen_callfuncsingle_irq, cpu).name = callfunc_name;
rc = bind_ipi_to_irqhandler(XEN_CALL_FUNCTION_SINGLE_VECTOR,
cpu,
@@ -114,6 +124,8 @@ int xen_smp_intr_init(unsigned int cpu)
 
return 0;
 
+ fail_mem:
+   rc = -ENOMEM;
  fail:
xen_smp_intr_free(cpu);
return rc;
-- 
2.43.0

[PATCH AUTOSEL 5.4 3/5] x86/xen: Add some null pointer checking to smp.c

[Sasha Levin]

From: Kunwu Chan 

[ Upstream commit 3693bb4465e6e32a204a5b86d3ec7e6b9f7e67c2 ]

kasprintf() returns a pointer to dynamically allocated memory
which can be NULL upon failure. Ensure the allocation was successful
by checking the pointer validity.

Signed-off-by: Kunwu Chan 
Reported-by: kernel test robot 
Closes: 
https://lore.kernel.org/oe-kbuild-all/202401161119.iof6bqsf-...@intel.com/
Suggested-by: Markus Elfring 
Reviewed-by: Juergen Gross 
Link: https://lore.kernel.org/r/20240119094948.275390-1-chen...@kylinos.cn
Signed-off-by: Juergen Gross 
Signed-off-by: Sasha Levin 
---
 arch/x86/xen/smp.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index a1cc855c539c1..a76ba342a6695 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -65,6 +65,8 @@ int xen_smp_intr_init(unsigned int cpu)
char *resched_name, *callfunc_name, *debug_name;
 
resched_name = kasprintf(GFP_KERNEL, "resched%d", cpu);
+   if (!resched_name)
+   goto fail_mem;
per_cpu(xen_resched_irq, cpu).name = resched_name;
rc = bind_ipi_to_irqhandler(XEN_RESCHEDULE_VECTOR,
cpu,
@@ -77,6 +79,8 @@ int xen_smp_intr_init(unsigned int cpu)
per_cpu(xen_resched_irq, cpu).irq = rc;
 
callfunc_name = kasprintf(GFP_KERNEL, "callfunc%d", cpu);
+   if (!callfunc_name)
+   goto fail_mem;
per_cpu(xen_callfunc_irq, cpu).name = callfunc_name;
rc = bind_ipi_to_irqhandler(XEN_CALL_FUNCTION_VECTOR,
cpu,
@@ -90,6 +94,9 @@ int xen_smp_intr_init(unsigned int cpu)
 
if (!xen_fifo_events) {
debug_name = kasprintf(GFP_KERNEL, "debug%d", cpu);
+   if (!debug_name)
+   goto fail_mem;
+
per_cpu(xen_debug_irq, cpu).name = debug_name;
rc = bind_virq_to_irqhandler(VIRQ_DEBUG, cpu,
 xen_debug_interrupt,
@@ -101,6 +108,9 @@ int xen_smp_intr_init(unsigned int cpu)
}
 
callfunc_name = kasprintf(GFP_KERNEL, "callfuncsingle%d", cpu);
+   if (!callfunc_name)
+   goto fail_mem;
+
per_cpu(xen_callfuncsingle_irq, cpu).name = callfunc_name;
rc = bind_ipi_to_irqhandler(XEN_CALL_FUNCTION_SINGLE_VECTOR,
cpu,
@@ -114,6 +124,8 @@ int xen_smp_intr_init(unsigned int cpu)
 
return 0;
 
+ fail_mem:
+   rc = -ENOMEM;
  fail:
xen_smp_intr_free(cpu);
return rc;
-- 
2.43.0

[PATCH AUTOSEL 5.15 4/7] x86/xen: Add some null pointer checking to smp.c

[Sasha Levin]

From: Kunwu Chan 

[ Upstream commit 3693bb4465e6e32a204a5b86d3ec7e6b9f7e67c2 ]

kasprintf() returns a pointer to dynamically allocated memory
which can be NULL upon failure. Ensure the allocation was successful
by checking the pointer validity.

Signed-off-by: Kunwu Chan 
Reported-by: kernel test robot 
Closes: 
https://lore.kernel.org/oe-kbuild-all/202401161119.iof6bqsf-...@intel.com/
Suggested-by: Markus Elfring 
Reviewed-by: Juergen Gross 
Link: https://lore.kernel.org/r/20240119094948.275390-1-chen...@kylinos.cn
Signed-off-by: Juergen Gross 
Signed-off-by: Sasha Levin 
---
 arch/x86/xen/smp.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index cdec892b28e2e..a641e0d452194 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -65,6 +65,8 @@ int xen_smp_intr_init(unsigned int cpu)
char *resched_name, *callfunc_name, *debug_name;
 
resched_name = kasprintf(GFP_KERNEL, "resched%d", cpu);
+   if (!resched_name)
+   goto fail_mem;
per_cpu(xen_resched_irq, cpu).name = resched_name;
rc = bind_ipi_to_irqhandler(XEN_RESCHEDULE_VECTOR,
cpu,
@@ -77,6 +79,8 @@ int xen_smp_intr_init(unsigned int cpu)
per_cpu(xen_resched_irq, cpu).irq = rc;
 
callfunc_name = kasprintf(GFP_KERNEL, "callfunc%d", cpu);
+   if (!callfunc_name)
+   goto fail_mem;
per_cpu(xen_callfunc_irq, cpu).name = callfunc_name;
rc = bind_ipi_to_irqhandler(XEN_CALL_FUNCTION_VECTOR,
cpu,
@@ -90,6 +94,9 @@ int xen_smp_intr_init(unsigned int cpu)
 
if (!xen_fifo_events) {
debug_name = kasprintf(GFP_KERNEL, "debug%d", cpu);
+   if (!debug_name)
+   goto fail_mem;
+
per_cpu(xen_debug_irq, cpu).name = debug_name;
rc = bind_virq_to_irqhandler(VIRQ_DEBUG, cpu,
 xen_debug_interrupt,
@@ -101,6 +108,9 @@ int xen_smp_intr_init(unsigned int cpu)
}
 
callfunc_name = kasprintf(GFP_KERNEL, "callfuncsingle%d", cpu);
+   if (!callfunc_name)
+   goto fail_mem;
+
per_cpu(xen_callfuncsingle_irq, cpu).name = callfunc_name;
rc = bind_ipi_to_irqhandler(XEN_CALL_FUNCTION_SINGLE_VECTOR,
cpu,
@@ -114,6 +124,8 @@ int xen_smp_intr_init(unsigned int cpu)
 
return 0;
 
+ fail_mem:
+   rc = -ENOMEM;
  fail:
xen_smp_intr_free(cpu);
return rc;
-- 
2.43.0

[PATCH AUTOSEL 6.1 08/12] x86/xen: Add some null pointer checking to smp.c

[Sasha Levin]

From: Kunwu Chan 

[ Upstream commit 3693bb4465e6e32a204a5b86d3ec7e6b9f7e67c2 ]

kasprintf() returns a pointer to dynamically allocated memory
which can be NULL upon failure. Ensure the allocation was successful
by checking the pointer validity.

Signed-off-by: Kunwu Chan 
Reported-by: kernel test robot 
Closes: 
https://lore.kernel.org/oe-kbuild-all/202401161119.iof6bqsf-...@intel.com/
Suggested-by: Markus Elfring 
Reviewed-by: Juergen Gross 
Link: https://lore.kernel.org/r/20240119094948.275390-1-chen...@kylinos.cn
Signed-off-by: Juergen Gross 
Signed-off-by: Sasha Levin 
---
 arch/x86/xen/smp.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index 4b0d6fff88de5..1fb9a1644d944 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -65,6 +65,8 @@ int xen_smp_intr_init(unsigned int cpu)
char *resched_name, *callfunc_name, *debug_name;
 
resched_name = kasprintf(GFP_KERNEL, "resched%d", cpu);
+   if (!resched_name)
+   goto fail_mem;
per_cpu(xen_resched_irq, cpu).name = resched_name;
rc = bind_ipi_to_irqhandler(XEN_RESCHEDULE_VECTOR,
cpu,
@@ -77,6 +79,8 @@ int xen_smp_intr_init(unsigned int cpu)
per_cpu(xen_resched_irq, cpu).irq = rc;
 
callfunc_name = kasprintf(GFP_KERNEL, "callfunc%d", cpu);
+   if (!callfunc_name)
+   goto fail_mem;
per_cpu(xen_callfunc_irq, cpu).name = callfunc_name;
rc = bind_ipi_to_irqhandler(XEN_CALL_FUNCTION_VECTOR,
cpu,
@@ -90,6 +94,9 @@ int xen_smp_intr_init(unsigned int cpu)
 
if (!xen_fifo_events) {
debug_name = kasprintf(GFP_KERNEL, "debug%d", cpu);
+   if (!debug_name)
+   goto fail_mem;
+
per_cpu(xen_debug_irq, cpu).name = debug_name;
rc = bind_virq_to_irqhandler(VIRQ_DEBUG, cpu,
 xen_debug_interrupt,
@@ -101,6 +108,9 @@ int xen_smp_intr_init(unsigned int cpu)
}
 
callfunc_name = kasprintf(GFP_KERNEL, "callfuncsingle%d", cpu);
+   if (!callfunc_name)
+   goto fail_mem;
+
per_cpu(xen_callfuncsingle_irq, cpu).name = callfunc_name;
rc = bind_ipi_to_irqhandler(XEN_CALL_FUNCTION_SINGLE_VECTOR,
cpu,
@@ -114,6 +124,8 @@ int xen_smp_intr_init(unsigned int cpu)
 
return 0;
 
+ fail_mem:
+   rc = -ENOMEM;
  fail:
xen_smp_intr_free(cpu);
return rc;
-- 
2.43.0

[PATCH AUTOSEL 6.6 16/21] x86/xen: Add some null pointer checking to smp.c

[Sasha Levin]

From: Kunwu Chan 

[ Upstream commit 3693bb4465e6e32a204a5b86d3ec7e6b9f7e67c2 ]

kasprintf() returns a pointer to dynamically allocated memory
which can be NULL upon failure. Ensure the allocation was successful
by checking the pointer validity.

Signed-off-by: Kunwu Chan 
Reported-by: kernel test robot 
Closes: 
https://lore.kernel.org/oe-kbuild-all/202401161119.iof6bqsf-...@intel.com/
Suggested-by: Markus Elfring 
Reviewed-by: Juergen Gross 
Link: https://lore.kernel.org/r/20240119094948.275390-1-chen...@kylinos.cn
Signed-off-by: Juergen Gross 
Signed-off-by: Sasha Levin 
---
 arch/x86/xen/smp.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index 4b0d6fff88de5..1fb9a1644d944 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -65,6 +65,8 @@ int xen_smp_intr_init(unsigned int cpu)
char *resched_name, *callfunc_name, *debug_name;
 
resched_name = kasprintf(GFP_KERNEL, "resched%d", cpu);
+   if (!resched_name)
+   goto fail_mem;
per_cpu(xen_resched_irq, cpu).name = resched_name;
rc = bind_ipi_to_irqhandler(XEN_RESCHEDULE_VECTOR,
cpu,
@@ -77,6 +79,8 @@ int xen_smp_intr_init(unsigned int cpu)
per_cpu(xen_resched_irq, cpu).irq = rc;
 
callfunc_name = kasprintf(GFP_KERNEL, "callfunc%d", cpu);
+   if (!callfunc_name)
+   goto fail_mem;
per_cpu(xen_callfunc_irq, cpu).name = callfunc_name;
rc = bind_ipi_to_irqhandler(XEN_CALL_FUNCTION_VECTOR,
cpu,
@@ -90,6 +94,9 @@ int xen_smp_intr_init(unsigned int cpu)
 
if (!xen_fifo_events) {
debug_name = kasprintf(GFP_KERNEL, "debug%d", cpu);
+   if (!debug_name)
+   goto fail_mem;
+
per_cpu(xen_debug_irq, cpu).name = debug_name;
rc = bind_virq_to_irqhandler(VIRQ_DEBUG, cpu,
 xen_debug_interrupt,
@@ -101,6 +108,9 @@ int xen_smp_intr_init(unsigned int cpu)
}
 
callfunc_name = kasprintf(GFP_KERNEL, "callfuncsingle%d", cpu);
+   if (!callfunc_name)
+   goto fail_mem;
+
per_cpu(xen_callfuncsingle_irq, cpu).name = callfunc_name;
rc = bind_ipi_to_irqhandler(XEN_CALL_FUNCTION_SINGLE_VECTOR,
cpu,
@@ -114,6 +124,8 @@ int xen_smp_intr_init(unsigned int cpu)
 
return 0;
 
+ fail_mem:
+   rc = -ENOMEM;
  fail:
xen_smp_intr_free(cpu);
return rc;
-- 
2.43.0

[PATCH AUTOSEL 6.7 18/26] x86/xen: Add some null pointer checking to smp.c

[Sasha Levin]

From: Kunwu Chan 

[ Upstream commit 3693bb4465e6e32a204a5b86d3ec7e6b9f7e67c2 ]

kasprintf() returns a pointer to dynamically allocated memory
which can be NULL upon failure. Ensure the allocation was successful
by checking the pointer validity.

Signed-off-by: Kunwu Chan 
Reported-by: kernel test robot 
Closes: 
https://lore.kernel.org/oe-kbuild-all/202401161119.iof6bqsf-...@intel.com/
Suggested-by: Markus Elfring 
Reviewed-by: Juergen Gross 
Link: https://lore.kernel.org/r/20240119094948.275390-1-chen...@kylinos.cn
Signed-off-by: Juergen Gross 
Signed-off-by: Sasha Levin 
---
 arch/x86/xen/smp.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index 4b0d6fff88de5..1fb9a1644d944 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -65,6 +65,8 @@ int xen_smp_intr_init(unsigned int cpu)
char *resched_name, *callfunc_name, *debug_name;
 
resched_name = kasprintf(GFP_KERNEL, "resched%d", cpu);
+   if (!resched_name)
+   goto fail_mem;
per_cpu(xen_resched_irq, cpu).name = resched_name;
rc = bind_ipi_to_irqhandler(XEN_RESCHEDULE_VECTOR,
cpu,
@@ -77,6 +79,8 @@ int xen_smp_intr_init(unsigned int cpu)
per_cpu(xen_resched_irq, cpu).irq = rc;
 
callfunc_name = kasprintf(GFP_KERNEL, "callfunc%d", cpu);
+   if (!callfunc_name)
+   goto fail_mem;
per_cpu(xen_callfunc_irq, cpu).name = callfunc_name;
rc = bind_ipi_to_irqhandler(XEN_CALL_FUNCTION_VECTOR,
cpu,
@@ -90,6 +94,9 @@ int xen_smp_intr_init(unsigned int cpu)
 
if (!xen_fifo_events) {
debug_name = kasprintf(GFP_KERNEL, "debug%d", cpu);
+   if (!debug_name)
+   goto fail_mem;
+
per_cpu(xen_debug_irq, cpu).name = debug_name;
rc = bind_virq_to_irqhandler(VIRQ_DEBUG, cpu,
 xen_debug_interrupt,
@@ -101,6 +108,9 @@ int xen_smp_intr_init(unsigned int cpu)
}
 
callfunc_name = kasprintf(GFP_KERNEL, "callfuncsingle%d", cpu);
+   if (!callfunc_name)
+   goto fail_mem;
+
per_cpu(xen_callfuncsingle_irq, cpu).name = callfunc_name;
rc = bind_ipi_to_irqhandler(XEN_CALL_FUNCTION_SINGLE_VECTOR,
cpu,
@@ -114,6 +124,8 @@ int xen_smp_intr_init(unsigned int cpu)
 
return 0;
 
+ fail_mem:
+   rc = -ENOMEM;
  fail:
xen_smp_intr_free(cpu);
return rc;
-- 
2.43.0

Re: [PATCH v4 12/12] xen/spinlock: support higher number of cpus

[Jan Beulich]

On 12.12.2023 10:47, Juergen Gross wrote:
> Allow 16 bits per cpu number, which is the limit imposed by
> spinlock_tickets_t.
> 
> This will allow up to 65535 cpus, while increasing only the size of
> recursive spinlocks in debug builds from 8 to 12 bytes.

I think we want to be more conservative here, for the case of there
being bugs: The CPU holding a lock may wrongly try to acquire it a
2nd time. That's the 65536th ticket then, wrapping the value.
Therefore my suggestion would be to only (mention) go(ing) up to 32k.

> Signed-off-by: Juergen Gross 
> ---
>  xen/common/spinlock.c  |  1 +
>  xen/include/xen/spinlock.h | 18 +-
>  2 files changed, 10 insertions(+), 9 deletions(-)

Shouldn't this also bump the upper bound of the NR_CPUS range then
in xen/arch/Kconfig?

Jan

Re: [PATCH v4 10/12] xen/spinlock: split recursive spinlocks from normal ones

[Jürgen Groß]

On 29.02.24 16:32, Jan Beulich wrote:

On 12.12.2023 10:47, Juergen Gross wrote:

--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -541,6 +541,55 @@ void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned 
long flags)
  local_irq_restore(flags);
  }
  
+int nrspin_trylock(rspinlock_t *lock)

+{
+check_lock(>debug, true);
+
+if ( unlikely(lock->recurse_cpu != SPINLOCK_NO_CPU) )
+return 0;
+
+return spin_trylock_common(>tickets, >debug, LOCK_PROFILE_PAR);
+}


I wonder if we shouldn't take the opportunity and stop having trylock
functions have "int" return type. Perhaps already spin_trylock_common()
when introduced could use "bool" instead, then here following suit.


Fine with me.




+void nrspin_lock(rspinlock_t *lock)
+{
+spin_lock_common(>tickets, >debug, LOCK_PROFILE_PAR, NULL,
+ NULL);
+}
+
+void nrspin_unlock(rspinlock_t *lock)
+{
+spin_unlock_common(>tickets, >debug, LOCK_PROFILE_PAR);
+}
+
+void nrspin_lock_irq(rspinlock_t *lock)
+{
+ASSERT(local_irq_is_enabled());
+local_irq_disable();
+nrspin_lock(lock);
+}
+
+void nrspin_unlock_irq(rspinlock_t *lock)
+{
+nrspin_unlock(lock);
+local_irq_enable();
+}
+
+unsigned long __nrspin_lock_irqsave(rspinlock_t *lock)
+{
+unsigned long flags;
+
+local_irq_save(flags);
+nrspin_lock(lock);
+return flags;


Nit: Strictly speaking we want a blank line ahead of this "return".


Okay, will add it.




@@ -166,11 +172,15 @@ struct lock_profile_qhead { };
  struct lock_profile { };
  
  #define SPIN_LOCK_UNLOCKED {  \

+.debug =_LOCK_DEBUG,  \
+}
+#define RSPIN_LOCK_UNLOCKED { \
+.debug =_LOCK_DEBUG,  \
  .recurse_cpu = SPINLOCK_NO_CPU,   
\
  .debug =_LOCK_DEBUG,  
\
  }


Initializing .debug twice?


Oh, right. Will drop one.




@@ -180,7 +190,6 @@ struct lock_profile { };
  
  #endif
  
-

  typedef union {
  uint32_t head_tail;
  struct {


Looks like this might be undoing what the earlier patch isn't going to
do anymore?


Yes, seen it already.




@@ -242,6 +257,19 @@ void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned 
long flags);
  int rspin_is_locked(const rspinlock_t *lock);
  void rspin_barrier(rspinlock_t *lock);
  
+int nrspin_trylock(rspinlock_t *lock);

+void nrspin_lock(rspinlock_t *lock);
+void nrspin_unlock(rspinlock_t *lock);
+void nrspin_lock_irq(rspinlock_t *lock);
+void nrspin_unlock_irq(rspinlock_t *lock);
+#define nrspin_lock_irqsave(l, f)   \
+({  \
+BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long));   \
+((f) = __nrspin_lock_irqsave(l));   \


I don't think the outer pair of parentheses is needed here. As to the
leading double underscores, see comments elsewhere.


Okay.


Juergen

Re: [PATCH v4 11/12] xen/spinlock: remove indirection through macros for spin_*() functions

[Jan Beulich]

On 12.12.2023 10:47, Juergen Gross wrote:
> In reality all spin_*() functions are macros which are defined to just
> call a related real function.
> 
> Remove this macro layer, as it is adding complexity without any gain.
> 
> Signed-off-by: Juergen Gross 

Acked-by: Jan Beulich 
with the same remark as on patch 04.

Jan

Re: [PATCH v4 10/12] xen/spinlock: split recursive spinlocks from normal ones

[Jan Beulich]

On 12.12.2023 10:47, Juergen Gross wrote:
> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -541,6 +541,55 @@ void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned 
> long flags)
>  local_irq_restore(flags);
>  }
>  
> +int nrspin_trylock(rspinlock_t *lock)
> +{
> +check_lock(>debug, true);
> +
> +if ( unlikely(lock->recurse_cpu != SPINLOCK_NO_CPU) )
> +return 0;
> +
> +return spin_trylock_common(>tickets, >debug, 
> LOCK_PROFILE_PAR);
> +}

I wonder if we shouldn't take the opportunity and stop having trylock
functions have "int" return type. Perhaps already spin_trylock_common()
when introduced could use "bool" instead, then here following suit.

> +void nrspin_lock(rspinlock_t *lock)
> +{
> +spin_lock_common(>tickets, >debug, LOCK_PROFILE_PAR, NULL,
> + NULL);
> +}
> +
> +void nrspin_unlock(rspinlock_t *lock)
> +{
> +spin_unlock_common(>tickets, >debug, LOCK_PROFILE_PAR);
> +}
> +
> +void nrspin_lock_irq(rspinlock_t *lock)
> +{
> +ASSERT(local_irq_is_enabled());
> +local_irq_disable();
> +nrspin_lock(lock);
> +}
> +
> +void nrspin_unlock_irq(rspinlock_t *lock)
> +{
> +nrspin_unlock(lock);
> +local_irq_enable();
> +}
> +
> +unsigned long __nrspin_lock_irqsave(rspinlock_t *lock)
> +{
> +unsigned long flags;
> +
> +local_irq_save(flags);
> +nrspin_lock(lock);
> +return flags;

Nit: Strictly speaking we want a blank line ahead of this "return".

> @@ -166,11 +172,15 @@ struct lock_profile_qhead { };
>  struct lock_profile { };
>  
>  #define SPIN_LOCK_UNLOCKED { 
>  \
> +.debug =_LOCK_DEBUG, 
>  \
> +}
> +#define RSPIN_LOCK_UNLOCKED {
>  \
> +.debug =_LOCK_DEBUG, 
>  \
>  .recurse_cpu = SPINLOCK_NO_CPU,  
>  \
>  .debug =_LOCK_DEBUG, 
>  \
>  }

Initializing .debug twice?

> @@ -180,7 +190,6 @@ struct lock_profile { };
>  
>  #endif
>  
> -
>  typedef union {
>  uint32_t head_tail;
>  struct {

Looks like this might be undoing what the earlier patch isn't going to
do anymore?

> @@ -242,6 +257,19 @@ void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned 
> long flags);
>  int rspin_is_locked(const rspinlock_t *lock);
>  void rspin_barrier(rspinlock_t *lock);
>  
> +int nrspin_trylock(rspinlock_t *lock);
> +void nrspin_lock(rspinlock_t *lock);
> +void nrspin_unlock(rspinlock_t *lock);
> +void nrspin_lock_irq(rspinlock_t *lock);
> +void nrspin_unlock_irq(rspinlock_t *lock);
> +#define nrspin_lock_irqsave(l, f)   \
> +({  \
> +BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long));   \
> +((f) = __nrspin_lock_irqsave(l));   \

I don't think the outer pair of parentheses is needed here. As to the
leading double underscores, see comments elsewhere.

Jan

[XEN PATCH 07/10] xen/arm: smmuv3: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

Signed-off-by: Nicola Vetrini 
---
 xen/drivers/passthrough/arm/smmu-v3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
b/xen/drivers/passthrough/arm/smmu-v3.c
index c3ac6d17d1c8..b1c40c2c0ae7 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -111,7 +111,7 @@
 #define GFP_KERNEL 0
 
 /* Device logger functions */
-#define dev_name(dev)  dt_node_full_name(dev->of_node)
+#define dev_name(dev)  dt_node_full_name((dev)->of_node)
 #define dev_dbg(dev, fmt, ...) \
printk(XENLOG_DEBUG "SMMUv3: %s: " fmt, dev_name(dev), ## __VA_ARGS__)
 #define dev_notice(dev, fmt, ...)  \
-- 
2.34.1

[XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

Signed-off-by: Nicola Vetrini 
---
 xen/include/xen/bug.h  |  2 +-
 xen/include/xen/init.h |  4 +--
 xen/include/xen/kconfig.h  |  2 +-
 xen/include/xen/list.h | 59 +++---
 xen/include/xen/param.h| 22 +++---
 xen/include/xen/spinlock.h |  2 +-
 6 files changed, 45 insertions(+), 46 deletions(-)

diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
index 2c45c462fc63..77fe1e1ba840 100644
--- a/xen/include/xen/bug.h
+++ b/xen/include/xen/bug.h
@@ -80,7 +80,7 @@ struct bug_frame {
 [bf_type]"i" (type), \
 [bf_ptr] "i" (ptr),  \
 [bf_msg] "i" (msg),  \
-[bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1))\
+[bf_line_lo] "i" (((line) & ((1 << BUG_LINE_LO_WIDTH) - 1))  \
   << BUG_DISP_WIDTH),\
 [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)
 
diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
index 1d7c0216bc80..0a4223833755 100644
--- a/xen/include/xen/init.h
+++ b/xen/include/xen/init.h
@@ -63,9 +63,9 @@ typedef int (*initcall_t)(void);
 typedef void (*exitcall_t)(void);
 
 #define presmp_initcall(fn) \
-const static initcall_t __initcall_##fn __init_call("presmp") = fn
+const static initcall_t __initcall_##fn __init_call("presmp") = (fn)
 #define __initcall(fn) \
-const static initcall_t __initcall_##fn __init_call("1") = fn
+const static initcall_t __initcall_##fn __init_call("1") = (fn)
 #define __exitcall(fn) \
 static exitcall_t __exitcall_##fn __exit_call = fn
 
diff --git a/xen/include/xen/kconfig.h b/xen/include/xen/kconfig.h
index c25dc0f6c2a9..b7e70289737b 100644
--- a/xen/include/xen/kconfig.h
+++ b/xen/include/xen/kconfig.h
@@ -25,7 +25,7 @@
 #define __ARG_PLACEHOLDER_1 0,
 #define config_enabled(cfg) _config_enabled(cfg)
 #define _config_enabled(value) __config_enabled(__ARG_PLACEHOLDER_##value)
-#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 1, 0)
+#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk (1), (0))
 #define ___config_enabled(__ignored, val, ...) val
 
 /*
diff --git a/xen/include/xen/list.h b/xen/include/xen/list.h
index b5eab3a1eb6c..d803e7848cad 100644
--- a/xen/include/xen/list.h
+++ b/xen/include/xen/list.h
@@ -490,9 +490,9 @@ static inline void list_splice_init(struct list_head *list,
  * @member: the name of the list_struct within the struct.
  */
 #define list_for_each_entry(pos, head, member)  \
-for (pos = list_entry((head)->next, typeof(*pos), member);  \
- >member != (head);\
- pos = list_entry(pos->member.next, typeof(*pos), member))
+for (pos = list_entry((head)->next, typeof(*(pos)), member);  \
+ &(pos)->member != (head);  \
+ pos = list_entry((pos)->member.next, typeof(*(pos)), member))
 
 /**
  * list_for_each_entry_reverse - iterate backwards over list of given type.
@@ -501,9 +501,9 @@ static inline void list_splice_init(struct list_head *list,
  * @member: the name of the list_struct within the struct.
  */
 #define list_for_each_entry_reverse(pos, head, member)  \
-for (pos = list_entry((head)->prev, typeof(*pos), member);  \
- >member != (head);\
- pos = list_entry(pos->member.prev, typeof(*pos), member))
+for (pos = list_entry((head)->prev, typeof(*(pos)), member);  \
+ &(pos)->member != (head);  \
+ pos = list_entry((pos)->member.prev, typeof(*(pos)), member))
 
 /**
  * list_prepare_entry - prepare a pos entry for use in
@@ -516,7 +516,7 @@ static inline void list_splice_init(struct list_head *list,
  * list_for_each_entry_continue.
  */
 #define list_prepare_entry(pos, head, member)   \
-((pos) ? : list_entry(head, typeof(*pos), member))
+((pos) ? : list_entry(head, typeof(*(pos)), member))
 
 /**
  * list_for_each_entry_continue - continue iteration over list of given type
@@ -528,9 +528,9 @@ static inline void list_splice_init(struct list_head *list,
  * the current position.
  */
 #define list_for_each_entry_continue(pos, head, member) \
-for (pos = list_entry(pos->member.next, typeof(*pos), member);  \
- >member != (head); 

[XEN PATCH 09/10] xen/include: tasklet: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

Signed-off-by: Nicola Vetrini 
---
 xen/include/xen/tasklet.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/xen/tasklet.h b/xen/include/xen/tasklet.h
index 593d6a2400fb..78760b694a39 100644
--- a/xen/include/xen/tasklet.h
+++ b/xen/include/xen/tasklet.h
@@ -27,7 +27,7 @@ struct tasklet
 
 #define _DECLARE_TASKLET(name, func, data, softirq) \
 struct tasklet name = { \
-LIST_HEAD_INIT(name.list), -1, softirq, 0, 0, func, data }
+LIST_HEAD_INIT((name).list), -1, softirq, 0, 0, func, data }
 #define DECLARE_TASKLET(name, func, data)   \
 _DECLARE_TASKLET(name, func, data, 0)
 #define DECLARE_SOFTIRQ_TASKLET(name, func, data)   \
-- 
2.34.1

[XEN PATCH 04/10] xen/public: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore
the macro XEN_DEFINE_UUID_ should wrap its parameters in parentheses.

No functional change.

Signed-off-by: Nicola Vetrini 
---
 xen/include/public/xen.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index b47d48d0e2d6..fa23080bd7af 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -988,7 +988,7 @@ typedef struct {
   ((b) >>  8) & 0xFF, ((b) >>  0) & 0xFF,   \
   ((c) >>  8) & 0xFF, ((c) >>  0) & 0xFF,   \
   ((d) >>  8) & 0xFF, ((d) >>  0) & 0xFF,   \
-e1, e2, e3, e4, e5, e6}}
+(e1), (e2), (e3), (e4), (e5), (e6)}}
 
 #if defined(__STDC_VERSION__) ? __STDC_VERSION__ >= 199901L : defined(__GNUC__)
 #define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6) \
-- 
2.34.1

[XEN PATCH 05/10] xen/perfc: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

Signed-off-by: Nicola Vetrini 
---
 xen/common/perfc.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/common/perfc.c b/xen/common/perfc.c
index 7400667bf0c4..02f4fc8fe032 100644
--- a/xen/common/perfc.c
+++ b/xen/common/perfc.c
@@ -10,10 +10,10 @@
 #include 
 #include 
 
-#define PERFCOUNTER( var, name )  { name, TYPE_SINGLE, 0 },
-#define PERFCOUNTER_ARRAY( var, name, size )  { name, TYPE_ARRAY,  size },
-#define PERFSTATUS( var, name )   { name, TYPE_S_SINGLE, 0 },
-#define PERFSTATUS_ARRAY( var, name, size )   { name, TYPE_S_ARRAY,  size },
+#define PERFCOUNTER( var, name )  { (name), TYPE_SINGLE, 0 },
+#define PERFCOUNTER_ARRAY( var, name, size )  { (name), TYPE_ARRAY,  (size) },
+#define PERFSTATUS( var, name )   { (name), TYPE_S_SINGLE, 0 },
+#define PERFSTATUS_ARRAY( var, name, size )   { (name), TYPE_S_ARRAY,  (size) 
},
 static const struct {
 const char *name;
 enum { TYPE_SINGLE, TYPE_ARRAY,
-- 
2.34.1

[XEN PATCH 10/10] xen/keyhandler: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

Signed-off-by: Nicola Vetrini 
---
 xen/common/keyhandler.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index 127ca506965c..4c1ce007870f 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -42,10 +42,10 @@ static struct keyhandler {
 } key_table[128] __read_mostly =
 {
 #define KEYHANDLER(k, f, desc, diag)\
-[k] = { { .fn = (f) }, desc, 0, diag }
+[k] = { { .fn = (f) }, (desc), 0, (diag) }
 
 #define IRQ_KEYHANDLER(k, f, desc, diag)\
-[k] = { { .irq_fn = (f) }, desc, 1, diag }
+[k] = { { .irq_fn = (f) }, (desc), 1, (diag) }
 
 IRQ_KEYHANDLER('A', do_toggle_alt_key, "toggle alternative key handling", 
0),
 IRQ_KEYHANDLER('d', dump_registers, "dump registers", 1),
-- 
2.34.1

[XEN PATCH 02/10] xen/arm: address some violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

Signed-off-by: Nicola Vetrini 
---
Style in arm64/cpufeature.c has not been amended, because this file seems
to be kept in sync with its Linux counterpart.
---
 xen/arch/arm/arm64/cpufeature.c  | 14 +++---
 xen/arch/arm/cpuerrata.c |  4 ++--
 xen/arch/arm/include/asm/arm64/sysregs.h |  2 +-
 xen/arch/arm/include/asm/guest_atomics.h |  4 ++--
 xen/arch/arm/include/asm/mm.h|  2 +-
 xen/arch/arm/include/asm/smccc.h |  8 
 xen/arch/arm/include/asm/vgic-emul.h |  8 
 xen/arch/arm/vcpreg.c|  5 +++--
 8 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
index 864413d9cc03..6fb8974ade7f 100644
--- a/xen/arch/arm/arm64/cpufeature.c
+++ b/xen/arch/arm/arm64/cpufeature.c
@@ -78,13 +78,13 @@
 
 #define __ARM64_FTR_BITS(SIGNED, VISIBLE, STRICT, TYPE, SHIFT, WIDTH, 
SAFE_VAL) \
{   \
-   .sign = SIGNED, \
-   .visible = VISIBLE, \
-   .strict = STRICT,   \
-   .type = TYPE,   \
-   .shift = SHIFT, \
-   .width = WIDTH, \
-   .safe_val = SAFE_VAL,   \
+   .sign = (SIGNED),   \
+   .visible = (VISIBLE),   \
+   .strict = (STRICT), \
+   .type = (TYPE), \
+   .shift = (SHIFT),   \
+   .width = (WIDTH),   \
+   .safe_val = (SAFE_VAL), \
}
 
 /* Define a feature with unsigned values */
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index a28fa6ac78cc..c678a555910f 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -462,8 +462,8 @@ static bool has_ssbd_mitigation(const struct 
arm_cpu_capabilities *entry)
 #define MIDR_RANGE(model, min, max) \
 .matches = is_affected_midr_range,  \
 .midr_model = model,\
-.midr_range_min = min,  \
-.midr_range_max = max
+.midr_range_min = (min),\
+.midr_range_max = (max)
 
 #define MIDR_ALL_VERSIONS(model)\
 .matches = is_affected_midr_range,  \
diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h 
b/xen/arch/arm/include/asm/arm64/sysregs.h
index 3fdeb9d8cdef..b593e4028b53 100644
--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -465,7 +465,7 @@
 /* Access to system registers */
 
 #define WRITE_SYSREG64(v, name) do {\
-uint64_t _r = v;\
+uint64_t _r = (v);  \
 asm volatile("msr "__stringify(name)", %0" : : "r" (_r));   \
 } while (0)
 #define READ_SYSREG64(name) ({  \
diff --git a/xen/arch/arm/include/asm/guest_atomics.h 
b/xen/arch/arm/include/asm/guest_atomics.h
index a1745f8613f6..8893eb9a55d7 100644
--- a/xen/arch/arm/include/asm/guest_atomics.h
+++ b/xen/arch/arm/include/asm/guest_atomics.h
@@ -32,7 +32,7 @@ static inline void guest_##name(struct domain *d, int nr, 
volatile void *p) \
 perfc_incr(atomics_guest_paused);   \
 \
 domain_pause_nosync(d); \
-name(nr, p);\
+(name)(nr, p);  \
 domain_unpause(d);  \
 }
 
@@ -52,7 +52,7 @@ static inline int guest_##name(struct domain *d, int nr, 
volatile void *p)  \
 perfc_incr(atomics_guest_paused);   \
 \
 domain_pause_nosync(d); \
-oldbit = name(nr, p);   \
+oldbit = (name)(nr, p); \
 domain_unpause(d);  \
 \
 return oldbit; 

[XEN PATCH 08/10] xen/errno: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

Signed-off-by: Nicola Vetrini 
---
 xen/include/xen/errno.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/xen/errno.h b/xen/include/xen/errno.h
index 69b28dd3c6c5..506674701fae 100644
--- a/xen/include/xen/errno.h
+++ b/xen/include/xen/errno.h
@@ -3,7 +3,7 @@
 
 #ifndef __ASSEMBLY__
 
-#define XEN_ERRNO(name, value) name = value,
+#define XEN_ERRNO(name, value) name = (value),
 enum {
 #include 
 };
-- 
2.34.1

[XEN PATCH 06/10] arm/smmu: address some violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

Signed-off-by: Nicola Vetrini 
---
 xen/drivers/passthrough/arm/smmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c 
b/xen/drivers/passthrough/arm/smmu.c
index 625ed0e41961..83196057a937 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -242,7 +242,7 @@ struct arm_smmu_xen_device {
struct iommu_group *group;
 };
 
-#define dev_archdata(dev) ((struct arm_smmu_xen_device *)dev->iommu)
+#define dev_archdata(dev) ((struct arm_smmu_xen_device *)(dev)->iommu)
 #define dev_iommu_domain(dev) (dev_archdata(dev)->domain)
 #define dev_iommu_group(dev) (dev_archdata(dev)->group)
 
@@ -627,7 +627,7 @@ struct arm_smmu_master_cfg {
 };
 #define INVALID_SMENDX -1
 #define for_each_cfg_sme(cfg, i, idx, num) \
-   for (i = 0; idx = cfg->smendx[i], i < num; ++i)
+   for (i = 0; idx = (cfg)->smendx[i], (i) < (num); ++(i))
 
 struct arm_smmu_master {
struct device_node  *of_node;
-- 
2.34.1

[XEN PATCH 00/10] address some violations of MISRA C Rule 20.7

[Nicola Vetrini]

Hi all,

this series aims to refactor some macros that cause violations of MISRA C Rule
20.7 ("Expressions resulting from the expansion of macro parameters shall be
enclosed in parentheses"). All the macros touched by these patches are in some
way involved in violations, and the strategy adopted to bring them into
compliance is to add parentheses around macro arguments where needed.

Given that the community has previously requested a deviation from the rule, as
stated in docs/misra/deviations.rst, and reported below for convenience [1],
some macro parameters do not need any adjusting (e.g., when used as lhs to
an assignment in statement expressions).

This series fixes a significant portion of the violations on Arm
(from ~14000 to ~2500). On x86, though there is one patch touching it, there are
still many more; they will be part of a later series.

[1] - Code violating Rule 20.7 is safe when macro parameters are used:
   (1) as function arguments;
   (2) as macro arguments;
   (3) as array indices;
   (4) as lhs in assignments.

Nicola Vetrini (10):
  xen/include: address violations of MISRA C Rule 20.7
  xen/arm: address some violations of MISRA C Rule 20.7
  x86: address some violations of MISRA C Rule 20.7
  xen/public: address violations of MISRA C Rule 20.7
  xen/perfc: address violations of MISRA C Rule 20.7
  arm/smmu: address some violations of MISRA C Rule 20.7
  xen/arm: smmuv3: address violations of MISRA C Rule 20.7
  xen/errno: address violations of MISRA C Rule 20.7
  xen/include: tasklet: address violations of MISRA C Rule 20.7
  xen/keyhandler: address violations of MISRA C Rule 20.7

 xen/arch/arm/arm64/cpufeature.c  | 14 +++---
 xen/arch/arm/cpuerrata.c |  4 +-
 xen/arch/arm/include/asm/arm64/sysregs.h |  2 +-
 xen/arch/arm/include/asm/guest_atomics.h |  4 +-
 xen/arch/arm/include/asm/mm.h|  2 +-
 xen/arch/arm/include/asm/smccc.h |  8 ++--
 xen/arch/arm/include/asm/vgic-emul.h |  8 ++--
 xen/arch/arm/vcpreg.c|  5 +-
 xen/arch/x86/include/asm/irq.h   |  6 +--
 xen/arch/x86/usercopy.c  |  2 +-
 xen/common/keyhandler.c  |  4 +-
 xen/common/perfc.c   |  8 ++--
 xen/drivers/passthrough/arm/smmu-v3.c|  2 +-
 xen/drivers/passthrough/arm/smmu.c   |  4 +-
 xen/include/public/xen.h |  2 +-
 xen/include/xen/bug.h|  2 +-
 xen/include/xen/errno.h  |  2 +-
 xen/include/xen/init.h   |  4 +-
 xen/include/xen/kconfig.h|  2 +-
 xen/include/xen/list.h   | 59 
 xen/include/xen/param.h  | 22 -
 xen/include/xen/spinlock.h   |  2 +-
 xen/include/xen/tasklet.h|  2 +-
 23 files changed, 85 insertions(+), 85 deletions(-)

-- 
2.34.1

[XEN PATCH 03/10] x86: address some violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

GUARD(1) is also amended to avoid modifying UA_KEEP or its definition.

No functional change.

Signed-off-by: Nicola Vetrini 
---
I wasn't very sure whether touching the definition of UA_KEEP would be a good
idea, so I added parentheses in the only user I've seen so far that causes a
violation.
---
 xen/arch/x86/include/asm/irq.h | 6 +++---
 xen/arch/x86/usercopy.c| 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
index 082a3d6bbc6a..5c722848e8ce 100644
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -179,9 +179,9 @@ void cleanup_domain_irq_mapping(struct domain *d);
 void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, emuirq);\
 __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND; \
 })
-#define IRQ_UNBOUND -1
-#define IRQ_PT -2
-#define IRQ_MSI_EMU -3
+#define IRQ_UNBOUND (-1)
+#define IRQ_PT  (-2)
+#define IRQ_MSI_EMU (-3)
 
 bool cpu_has_pending_apic_eoi(void);
 
diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c
index b8c2d1cc0bed..b0b55398e968 100644
--- a/xen/arch/x86/usercopy.c
+++ b/xen/arch/x86/usercopy.c
@@ -106,7 +106,7 @@ unsigned int copy_from_guest_ll(void *to, const void __user 
*from, unsigned int
 return n;
 }
 
-#if GUARD(1) + 0
+#if GUARD((1)) + 0
 
 /**
  * copy_to_guest_pv: - Copy a block of data into PV guest space.
-- 
2.34.1

Re: [PATCH 1/2] README: bump minimum required clang/llvm version

[Jan Beulich]

On 29.02.2024 15:21, Roger Pau Monné wrote:
> On Thu, Feb 29, 2024 at 02:12:26PM +0100, Jan Beulich wrote:
>> Bah, that's not even Clang, only LLVM.
> 
> I'm confused by this, doesn't your llvm package include clang?

No, there are quite a few RPMs in general in SLES to cover everything,
yet on the distro in question the clang* one(s) are (is) missing.

Jan

Re: [PATCH] x86/cpu-policy: Fix x2APIC visibility for PV guests

[Andrew Cooper]

On 29/02/2024 1:29 pm, Jan Beulich wrote:
> On 29.02.2024 14:23, Andrew Cooper wrote:
>> On 29/02/2024 12:47 pm, Jan Beulich wrote:
>>> On 29.02.2024 11:43, Andrew Cooper wrote:
 Right now, the host x2APIC setting filters into the PV max and default
 policies, yet PV guests cannot set MSR_APIC_BASE.EXTD or access any of the
 x2APIC MSR range.  Therefore they absolutely shouldn't see the x2APIC bit.

 Linux has workarounds for the collateral damage caused by this leakage; it
 unconditionally filters out the x2APIC CPUID bit, and EXTD when reading
 MSR_APIC_BASE.

 Hide the x2APIC bit in the PV default policy, but for compatibility, 
 tolerate
 incoming VMs which already saw the bit.  This is logic from before the
 default/max split in Xen 4.14 which wasn't correctly adjusted at the time.
>>> What about guest_cpuid()'s handling of leaf 0xb then? The %edx value
>>> will change once a guest is rebooted, aiui. The comment in
>>> recalculate_cpuid_policy() that you update refers to that.
>> That comment is going in the next patch irrespective.
>>
>> But yes - this will change leaf 0xb from being
>> host-conditionally-visible to always hidden.
> Imo this wants saying explicitly,

Yeah - already started that for v2.

>  including why that's okay to do,
> especially since ...
>
>> PV guests don't have any coherent idea of topology.  Linux (with the
>> topo fixes) now explicitly ignores everything it can see and just fakes
>> up a flat non-SMT topology in a single package.
> ... you validly use "now" here. Plus Linux isn't the only PV guest we
> need to care about.

As I said on the other thread, NetBSD works in the spirit in which PV
guests were intended and completely ignores x2APIC in XENPV builds.

>
> What's wrong (more wrong than the present putting of vCPU ID * 2 there)
> with retaining the population of that leaf (by dropping the x2apic
> dependency there)?

Without an MADT it's meaningless.   For PV dom0, it's actively wrong
because there is an MADT and it's in the wrong space.

libxc has never written anything coherent in here, because it's never
had any coherent idea about topology.  Alejandro is working on that, and
I believe one of the prep series has been posted.  There's a lot more to go.

Even today, we end up overlaying the host's APIC_ID space layout over
the blind vCPU_ID * 2, which makes the result still nonsense.

Various versions of Xen have tried playing with this, without
understanding properly what they're doing, and XenServer still has a
revert of a Xen 3.4 patch in the patch, as it broken migration of guests
at the time...


There is going to be a future (hopefully soon) where HVM guests get to
see something which conforms to the architectural specs, and is sane.

But doing the same for PV guests is more complicated, because of the
conflicting requirements between PV guests not really having an APIC,
but APIC being the x86 architectural expression of topology.


 This wants backporting as far as people can tollerate, but it's really not
 obvious which commit in 4.14 should be referenced in a Fixes: tag.
>>> Why 4.14? In 4.7.0 I see ...
>>>
 @@ -830,11 +846,10 @@ void recalculate_cpuid_policy(struct domain *d)
  }
  
  /*
 - * Allow the toolstack to set HTT, X2APIC and CMP_LEGACY.  These bits
 + * Allow the toolstack to set HTT and CMP_LEGACY.  These bits
   * affect how to interpret topology information in other cpuid leaves.
   */
  __set_bit(X86_FEATURE_HTT, max_fs);
 -__set_bit(X86_FEATURE_X2APIC, max_fs);
  __set_bit(X86_FEATURE_CMP_LEGACY, max_fs);
  
  /*
>>> ... these adjustments, just still in calculate_pv_featureset(). I
>>> haven't gone further backwards to check if/when this exposure has
>>> really appeared. I wouldn't be surprised if it's been like that
>>> for all the time since we gained x2APIC support in the hypervisor.
>> 4.14 was when we got the proper default vs max split.  Before then, this
>> block of logic was an opencoded "max(ish) for tookstacks which know
>> about it" kind of thing.
> Except it was also affecting what guests get to see, afaict.

No - this hunk explicitly doesn't.

What this hunk says is "don't override the toolstack's choice based on
what the host can see".

Because even today, Xen is still blindly zeroing toolstack settings it
doesn't like, because my series fixing this is still sat on the mailing
list from years and years ago over and argument over whether a function
lives in libx86 or elsewhere...

~Andrew

Re: [PATCH 1/2] README: bump minimum required clang/llvm version

[Roger Pau Monné]

On Thu, Feb 29, 2024 at 02:12:26PM +0100, Jan Beulich wrote:
> On 29.02.2024 14:01, Jan Beulich wrote:
> > On 29.02.2024 13:40, Roger Pau Monné wrote:
> >> On Thu, Feb 29, 2024 at 01:11:55PM +0100, Jan Beulich wrote:
> >>> On 29.02.2024 10:55, Roger Pau Monne wrote:
>  --- a/README
>  +++ b/README
>  @@ -41,7 +41,7 @@ provided by your OS distributor:
>   - GCC 4.1.2_20070115 or later
>   - GNU Binutils 2.16.91.0.5 or later
>   or
>  -- Clang/LLVM 3.5 or later
>  +- Clang/LLVM 14.0.0 or later
> >>>
> >>> Wow, that's a big step. I'm build-testing with Clang7 on one system and
> >>> with Clang5 on another (and the latter more frequently than the former).
> >>> If any real dependency on this new a version (about 3 years old?) was
> >>> introduced, I would then no longer be able to locally test any Clang
> >>> builds (and hence the risk would again increase that I introduce issues
> >>> that affect just Clang builds).
> >>
> >> Would it be possible for you to update to a newer version?  I see both
> >> the OpenSUSE containers in Gitlab have newer versions of Clang.
> > 
> > No. These are SLES versions which I'm not intending to touch. See
> > 
> > https://lists.xen.org/archives/html/xen-devel/2024-02/msg01793.html
> > 
> > and
> > 
> > https://lists.xen.org/archives/html/xen-devel/2024-02/msg01795.html
> > 
> > for why. The most recent piece of hardware I've installed a fresh (but
> > not exactly new, yet still fully supported) SLES version on would
> > apparently offer Clang7 only, either.

Hm, OK.  Would containers be an option?

I don't know for gcc, but for clang I'm quite sure the only run time
tested builds are the ones used by FreeBSD.  I'm hesitant to claim
clang versions as supported when they are only tested to generate an
output without triggering any errors, but there's no testing at all
that the generated code is functional.

> Bah, that's not even Clang, only LLVM.

I'm confused by this, doesn't your llvm package include clang?

Thanks, Roger.

Re: [PATCH v4 09/12] xen/spinlock: add missing rspin_is_locked() and rspin_barrier()

[Jürgen Groß]

On 29.02.24 15:14, Jan Beulich wrote:

On 12.12.2023 10:47, Juergen Gross wrote:

--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -458,6 +458,23 @@ void _spin_barrier(spinlock_t *lock)
  spin_barrier_common(>tickets, >debug, LOCK_PROFILE_PAR);
  }
  
+int rspin_is_locked(const rspinlock_t *lock)

+{
+/*
+ * Recursive locks may be locked by another CPU, yet we return
+ * "false" here, making this function suitable only for use in
+ * ASSERT()s and alike.
+ */
+return lock->recurse_cpu == SPINLOCK_NO_CPU
+   ? spin_is_locked_common(>tickets)
+   : lock->recurse_cpu == smp_processor_id();
+}
+
+void rspin_barrier(rspinlock_t *lock)
+{
+spin_barrier_common(>tickets, >debug, LOCK_PROFILE_PAR);
+}


Ah, here we go. Looks all okay to me, but needs re-ordering such that the
earlier patch won't transiently introduce a regression.


Yes, just wanted to answer something similar to your remark on patch 8.


Juergen

Re: [PATCH v5 23/23] xen/README: add compiler and binutils versions for RISC-V64

[Julien Grall]

On 29/02/2024 14:07, Jan Beulich wrote:

On 29.02.2024 14:44, Julien Grall wrote:

Hi Jan,

On 29/02/2024 12:51, Jan Beulich wrote:

On 29.02.2024 13:32, Julien Grall wrote:

On 29/02/2024 12:17, Jan Beulich wrote:

On 29.02.2024 13:05, Andrew Cooper wrote:

On 29/02/2024 10:23 am, Julien Grall wrote:

IOW it is hard for me to see why RISC-V needs stronger restrictions
here
than other architectures. It ought to be possible to determine a
baseline
version. Even if taking the desire to have "pause" available as a
requirement, gas (and presumably gld) 2.36.1 would already suffice.


I think we want to bump it on Arm. There are zero reasons to try to
keep
a lower versions if nobody tests/use it in production.

I would suggest to do the same on x86. What's the point of try to
support Xen with a 15+ years old compiler?


It could have long been bumped if only a proper scheme to follow for
this and future bumping would have been put forward by anyone keen on
such bumping, like - see his reply - e.g. Andrew. You may recall that
this was discussed more than once on meetings, with no real outcome.
I'm personally not meaning to stand in the way of such bumping as long
as it's done in a predictable manner, but I'm not keen on doing so and
hence I don't view it as my obligation to try to invent a reasonable
scheme. (My personal view is that basic functionality should be
possible to have virtually everywhere, whereas for advanced stuff it
is fine to require a more modern tool chain.)


That's one way to see it. The problem with this statement is a user
today is mislead to think you can build Xen with any GCC versions
since 4.1. I don't believe we can guarantee that and we are exposing
our users to unnecessary risk.

In addition to that, I agree with Andrew. This is preventing us to
improve our code base and we have to carry hacks for older compilers.


I don't think anyone here is suggesting that we switch to a
bleeding-edge-only policy.  But 15y of support is extreme in the
opposite direction.

Xen ought to be buildable in the contemporary distros of the day, and I
don't think anyone is going to credibly argue otherwise.

But, it's also fine for new things to have newer requirements.

Take CET for example.  I know we have disagreements on exactly how it's
toolchain-conditionalness is implemented, but the basic principle of "If
you want shiny new optional feature $X, you need newer toolchain $Y" is
entirely fine.

A brand new architecture is exactly the same.  Saying "this is the
minimum, because it's what we test" doesn't preclude someone coming
along and saying "can we use $N-1 ?  See here it works, and here's a
change to CI test it".


Anyway, its clear we need to write some policy on this, before making
specific adjustments.  To get started, is there going to be any
objection whatsoever on some principles which begin as follows:


Largely not, but one aspect needs clarifying up front:


* For established architectures, we expect Xen to be buildable on the
common contemporary distros.  (i.e. minima is not newer than what's
available in contemporary distros, without a good reason)


What counts as contemporary distro? Still in normal support? LTS? Yet
more extreme forms?


LTS makes sense. More I am not sure. I am under the impression that
people using older distros are those that wants a stable system. So they
would unlikely try to upgrade the hypervisor.

Even for LTS, I would argue that if it has been released 5 years ago,
then you probably want to update it at the same time as moving to a
newer Xen version.


For the purposes of distros I agree. For the purposes of individuals
I don't: What's wrong with running a newer hypervisor and/or kernel
underneath an older distro?


There is nothing wrong. I just don't understand the benefits for us to
support that use case. To me there are two sorts of individuals:
   1. The ones that are using distro packages. They will unlikely want to
switch to a newer hypervisor
   2. The ones that are happy to compile and hack their system. Fairly
likely they will use a more distros and/or would not be put up by
upgrading it.

What individuals do you have in mind?


People like me.


Which means? From what I read you mostly use an older distros for smoke 
testing/convenience.



Also, for me, the minimum doesn't prevent anyone to try to compile with
an older compiler. It is only here to say that as a community we will
not investigate or trying to workaround bugs in those compilers.


Besides this also allowing to use functionality you won't have an easy
way of replacing, what you say also doesn't make clear whether - for
cases where the issue can be (reasonably easily) worked around - patches
would be accepted, or rejected on the basis of only helping a below-the-
line compiler.


I would not accept them.

Cheers,

--
Julien Grall

Re: [PATCH v4 09/12] xen/spinlock: add missing rspin_is_locked() and rspin_barrier()

[Jan Beulich]

On 12.12.2023 10:47, Juergen Gross wrote:
> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -458,6 +458,23 @@ void _spin_barrier(spinlock_t *lock)
>  spin_barrier_common(>tickets, >debug, LOCK_PROFILE_PAR);
>  }
>  
> +int rspin_is_locked(const rspinlock_t *lock)
> +{
> +/*
> + * Recursive locks may be locked by another CPU, yet we return
> + * "false" here, making this function suitable only for use in
> + * ASSERT()s and alike.
> + */
> +return lock->recurse_cpu == SPINLOCK_NO_CPU
> +   ? spin_is_locked_common(>tickets)
> +   : lock->recurse_cpu == smp_processor_id();
> +}
> +
> +void rspin_barrier(rspinlock_t *lock)
> +{
> +spin_barrier_common(>tickets, >debug, LOCK_PROFILE_PAR);
> +}

Ah, here we go. Looks all okay to me, but needs re-ordering such that the
earlier patch won't transiently introduce a regression.

Jan

Re: [PATCH v5 04/23] xen/asm-generic: introduce generic fls() and flsl() functions

[Julien Grall]

Hi Jan,

On 29/02/2024 14:03, Jan Beulich wrote:

On 29.02.2024 14:54, Julien Grall wrote:

On 26/02/2024 17:38, Oleksii Kurochko wrote:

These functions can be useful for architectures that don't
have corresponding arch-specific instructions.

Signed-off-by: Oleksii Kurochko 
---
   Changes in V5:
 - new patch
---
   xen/include/asm-generic/bitops/fls.h  | 18 ++
   xen/include/asm-generic/bitops/flsl.h | 10 ++


One header per function seems a little bit excessive to me. Do you have
any pointer where this request is coming from?


That's in an attempt to follow Linux'es way of having this, aiui. This way
an arch can mix and match header inclusions and private definitions without
any #ifdef-ary.


Ok. I am not going to oppose it if the goal is to keep the headers in 
sync with Linux.


Although, it might have been useful to mention it in the commit message.

Cheers,

--
Julien Grall

Re: [PATCH v5 23/23] xen/README: add compiler and binutils versions for RISC-V64

[Jan Beulich]

On 29.02.2024 14:44, Julien Grall wrote:
> Hi Jan,
> 
> On 29/02/2024 12:51, Jan Beulich wrote:
>> On 29.02.2024 13:32, Julien Grall wrote:
>>> On 29/02/2024 12:17, Jan Beulich wrote:
 On 29.02.2024 13:05, Andrew Cooper wrote:
> On 29/02/2024 10:23 am, Julien Grall wrote:
> IOW it is hard for me to see why RISC-V needs stronger restrictions
> here
> than other architectures. It ought to be possible to determine a
> baseline
> version. Even if taking the desire to have "pause" available as a
> requirement, gas (and presumably gld) 2.36.1 would already suffice.

 I think we want to bump it on Arm. There are zero reasons to try to
 keep
 a lower versions if nobody tests/use it in production.

 I would suggest to do the same on x86. What's the point of try to
 support Xen with a 15+ years old compiler?
>>>
>>> It could have long been bumped if only a proper scheme to follow for
>>> this and future bumping would have been put forward by anyone keen on
>>> such bumping, like - see his reply - e.g. Andrew. You may recall that
>>> this was discussed more than once on meetings, with no real outcome.
>>> I'm personally not meaning to stand in the way of such bumping as long
>>> as it's done in a predictable manner, but I'm not keen on doing so and
>>> hence I don't view it as my obligation to try to invent a reasonable
>>> scheme. (My personal view is that basic functionality should be
>>> possible to have virtually everywhere, whereas for advanced stuff it
>>> is fine to require a more modern tool chain.)
>>
>> That's one way to see it. The problem with this statement is a user
>> today is mislead to think you can build Xen with any GCC versions
>> since 4.1. I don't believe we can guarantee that and we are exposing
>> our users to unnecessary risk.
>>
>> In addition to that, I agree with Andrew. This is preventing us to
>> improve our code base and we have to carry hacks for older compilers.
>
> I don't think anyone here is suggesting that we switch to a
> bleeding-edge-only policy.  But 15y of support is extreme in the
> opposite direction.
>
> Xen ought to be buildable in the contemporary distros of the day, and I
> don't think anyone is going to credibly argue otherwise.
>
> But, it's also fine for new things to have newer requirements.
>
> Take CET for example.  I know we have disagreements on exactly how it's
> toolchain-conditionalness is implemented, but the basic principle of "If
> you want shiny new optional feature $X, you need newer toolchain $Y" is
> entirely fine.
>
> A brand new architecture is exactly the same.  Saying "this is the
> minimum, because it's what we test" doesn't preclude someone coming
> along and saying "can we use $N-1 ?  See here it works, and here's a
> change to CI test it".
>
>
> Anyway, its clear we need to write some policy on this, before making
> specific adjustments.  To get started, is there going to be any
> objection whatsoever on some principles which begin as follows:

 Largely not, but one aspect needs clarifying up front:

> * For established architectures, we expect Xen to be buildable on the
> common contemporary distros.  (i.e. minima is not newer than what's
> available in contemporary distros, without a good reason)

 What counts as contemporary distro? Still in normal support? LTS? Yet
 more extreme forms?
>>>
>>> LTS makes sense. More I am not sure. I am under the impression that
>>> people using older distros are those that wants a stable system. So they
>>> would unlikely try to upgrade the hypervisor.
>>>
>>> Even for LTS, I would argue that if it has been released 5 years ago,
>>> then you probably want to update it at the same time as moving to a
>>> newer Xen version.
>>
>> For the purposes of distros I agree. For the purposes of individuals
>> I don't: What's wrong with running a newer hypervisor and/or kernel
>> underneath an older distro?
> 
> There is nothing wrong. I just don't understand the benefits for us to 
> support that use case. To me there are two sorts of individuals:
>   1. The ones that are using distro packages. They will unlikely want to 
> switch to a newer hypervisor
>   2. The ones that are happy to compile and hack their system. Fairly 
> likely they will use a more distros and/or would not be put up by 
> upgrading it.
> 
> What individuals do you have in mind?

People like me.

> Also, for me, the minimum doesn't prevent anyone to try to compile with 
> an older compiler. It is only here to say that as a community we will 
> not investigate or trying to workaround bugs in those compilers.

Besides this also allowing to use functionality you won't have an easy
way of replacing, what you say also doesn't make clear 

Re: [PATCH v5 04/23] xen/asm-generic: introduce generic fls() and flsl() functions

[Jan Beulich]

On 29.02.2024 14:54, Julien Grall wrote:
> On 26/02/2024 17:38, Oleksii Kurochko wrote:
>> These functions can be useful for architectures that don't
>> have corresponding arch-specific instructions.
>>
>> Signed-off-by: Oleksii Kurochko 
>> ---
>>   Changes in V5:
>> - new patch
>> ---
>>   xen/include/asm-generic/bitops/fls.h  | 18 ++
>>   xen/include/asm-generic/bitops/flsl.h | 10 ++
> 
> One header per function seems a little bit excessive to me. Do you have 
> any pointer where this request is coming from?

That's in an attempt to follow Linux'es way of having this, aiui. This way
an arch can mix and match header inclusions and private definitions without
any #ifdef-ary.

Jan

Re: [PATCH] pci: fix locking around vPCI removal in pci_remove_device()

[Stewart Hildebrand]

On 2/29/24 08:23, Jan Beulich wrote:
> On 29.02.2024 14:15, Roger Pau Monne wrote:
>> Currently vpci_deassign_device() is called without holding the per-domain
>> pci_lock in pci_remove_device(), which leads to:
>>
>> Assertion 'rw_is_write_locked(>domain->pci_lock)' failed at 
>> ../drivers/vpci/vpci.c:47
>> [...]
>> Xen call trace:
>>[] R vpci_deassign_device+0x10d/0x1b9
>>[] S pci_remove_device+0x2b1/0x380
>>[] F pci_physdev_op+0x197/0x19e
>>[] F do_physdev_op+0x342/0x12aa
>>[] F pv_hypercall+0x58e/0x62b
>>[] F lstar_enter+0x13a/0x140
>>
>> Move the existing block that removes the device from the domain pdev_list 
>> ahead
>> and also issue the call to vpci_deassign_device() there.  It's fine to remove
>> the device from the domain list of assigned devices, as further functions 
>> only
>> care that the pdev domain field is correctly set to the owner of the device
>> about to be removed.
>>
>> Moving the vpci_deassign_device() past the pci_cleanup_msi() call can be
>> dangerous, as doing the MSI cleanup ahead of having removed the vPCI handlers
>> could lead to stale data in vPCI MSI(-X) internal structures.
>>
>> Fixes: 4f78438b45e2 ('vpci: use per-domain PCI lock to protect vpci 
>> structure')
>> Signed-off-by: Roger Pau Monné 
> 
> Reviewed-by: Jan Beulich 

Reviewed-by: Stewart Hildebrand 

Thanks

Re: [PATCH v5 03/23] xen/riscv: introduce nospec.h

[Jan Beulich]

On 29.02.2024 14:49, Julien Grall wrote:
> On 26/02/2024 17:38, Oleksii Kurochko wrote:
>> --- /dev/null
>> +++ b/xen/arch/riscv/include/asm/nospec.h
>> @@ -0,0 +1,25 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
> 
> New file should use the SPDX tag GPL-2.0-only. I guess this could be 
> fixed on commit?

I wouldn't mind doing so.

Jan

Re: [PATCH v4 08/12] xen/spinlock: add another function level

[Jan Beulich]

On 12.12.2023 10:47, Juergen Gross wrote:
> @@ -377,25 +388,25 @@ void _spin_unlock_irqrestore(spinlock_t *lock, unsigned 
> long flags)
>  local_irq_restore(flags);
>  }
>  
> +static int always_inline spin_is_locked_common(const spinlock_tickets_t *t)
> +{
> +return t->head != t->tail;
> +}
> +
>  int _spin_is_locked(const spinlock_t *lock)
>  {
> -/*
> - * Recursive locks may be locked by another CPU, yet we return
> - * "false" here, making this function suitable only for use in
> - * ASSERT()s and alike.
> - */
> -return lock->recurse_cpu == SPINLOCK_NO_CPU
> -   ? lock->tickets.head != lock->tickets.tail
> -   : lock->recurse_cpu == smp_processor_id();
> +return spin_is_locked_common(>tickets);
>  }

This looks like a functional change. I haven't spotted an adjustment in an
earlier patch that would make the lost case unnecessary, but even if there
was one, the removal thereof would then also want doing there, I think.

Jan

Re: [PATCH v4 07/12] xen/spinlock: add explicit non-recursive locking functions

[Juergen Gross]

On 29.02.24 14:49, Jan Beulich wrote:

On 12.12.2023 10:47, Juergen Gross wrote:

In order to prepare a type-safe recursive spinlock structure, add
explicitly non-recursive locking functions to be used for non-recursive
locking of spinlocks, which are used recursively, too.

Signed-off-by: Juergen Gross 


Acked-by: Jan Beulich 
preferably with ...


--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -101,6 +101,8 @@ struct lock_profile_qhead {
  };
  
  #define _LOCK_PROFILE(lockname) { .name = #lockname, .lock = , }

+#define _RLOCK_PROFILE(lockname) { .name = #lockname, .rlock = , \
+.is_rlock = 1, }


... "true" used here, ...


@@ -133,13 +135,16 @@ struct lock_profile_qhead {
  break;
\
  } 
\
  prof->name = #l;  
\
-prof->lock = &(s)->l; \
+prof->lockptr = &(s)->l;  \
+prof->is_rlock = isr; \
  prof->next = (s)->profile_head.elem_q;
\
  (s)->profile_head.elem_q = prof;  
\
  } while( 0 )
  
-#define spin_lock_init_prof(s, l) __spin_lock_init_prof(s, l, spinlock_t)

-#define rspin_lock_init_prof(s, l) __spin_lock_init_prof(s, l, rspinlock_t)
+#define spin_lock_init_prof(s, l) \
+__spin_lock_init_prof(s, l, lock, spinlock_t, 0)


... "false" here, ...


+#define rspin_lock_init_prof(s, l)\
+__spin_lock_init_prof(s, l, rlock, rspinlock_t, 1)


... "true" again here, and ...


@@ -174,6 +179,7 @@ struct lock_profile_qhead { };
  
  #endif
  
+

  typedef union {
  uint32_t head_tail;
  struct {


... definitely with this hunk dropped.


I'm fine with all of above.


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH v5 04/23] xen/asm-generic: introduce generic fls() and flsl() functions

[Julien Grall]

Hi Oleksii,

On 26/02/2024 17:38, Oleksii Kurochko wrote:

These functions can be useful for architectures that don't
have corresponding arch-specific instructions.

Signed-off-by: Oleksii Kurochko 
---
  Changes in V5:
- new patch
---
  xen/include/asm-generic/bitops/fls.h  | 18 ++
  xen/include/asm-generic/bitops/flsl.h | 10 ++


One header per function seems a little bit excessive to me. Do you have 
any pointer where this request is coming from?


Why not using the pattern.

In arch implementation:

#define fls
static inline ...

In the generic header (asm-generic or xen/):

#ifndef fls
static inline ...
#endif


  2 files changed, 28 insertions(+)
  create mode 100644 xen/include/asm-generic/bitops/fls.h
  create mode 100644 xen/include/asm-generic/bitops/flsl.h

diff --git a/xen/include/asm-generic/bitops/fls.h 
b/xen/include/asm-generic/bitops/fls.h
new file mode 100644
index 00..369a4c790c
--- /dev/null
+++ b/xen/include/asm-generic/bitops/fls.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */


You should use GPL-2.0-only.


+#ifndef _ASM_GENERIC_BITOPS_FLS_H_
+#define _ASM_GENERIC_BITOPS_FLS_H_
+
+/**
+ * fls - find last (most-significant) bit set
+ * @x: the word to search
+ *
+ * This is defined the same way as ffs.
+ * Note fls(0) = 0, fls(1) = 1, fls(0x8000) = 32.
+ */
+
+static inline int fls(unsigned int x)
+{
+return generic_fls(x);
+}
+
+#endif /* _ASM_GENERIC_BITOPS_FLS_H_ */


Missing emacs magic. I am probably not going to repeat this remark and 
the one above again. So please have a look.



diff --git a/xen/include/asm-generic/bitops/flsl.h 
b/xen/include/asm-generic/bitops/flsl.h
new file mode 100644
index 00..d0a2e9c729
--- /dev/null
+++ b/xen/include/asm-generic/bitops/flsl.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_BITOPS_FLSL_H_
+#define _ASM_GENERIC_BITOPS_FLSL_H_
+
+static inline int flsl(unsigned long x)
+{
+return generic_flsl(x);
+}
+
+#endif /* _ASM_GENERIC_BITOPS_FLSL_H_ */


Cheers,

--
Julien Grall

Re: [PATCH v4 07/12] xen/spinlock: add explicit non-recursive locking functions

[Jan Beulich]

On 12.12.2023 10:47, Juergen Gross wrote:
> In order to prepare a type-safe recursive spinlock structure, add
> explicitly non-recursive locking functions to be used for non-recursive
> locking of spinlocks, which are used recursively, too.
> 
> Signed-off-by: Juergen Gross 

Acked-by: Jan Beulich 
preferably with ...

> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> @@ -101,6 +101,8 @@ struct lock_profile_qhead {
>  };
>  
>  #define _LOCK_PROFILE(lockname) { .name = #lockname, .lock = , }
> +#define _RLOCK_PROFILE(lockname) { .name = #lockname, .rlock = ,
>  \
> +.is_rlock = 1, }

... "true" used here, ...

> @@ -133,13 +135,16 @@ struct lock_profile_qhead {
>  break;   
>  \
>  }
>  \
>  prof->name = #l; 
>  \
> -prof->lock = &(s)->l;
>  \
> +prof->lockptr = &(s)->l; 
>  \
> +prof->is_rlock = isr;
>  \
>  prof->next = (s)->profile_head.elem_q;   
>  \
>  (s)->profile_head.elem_q = prof; 
>  \
>  } while( 0 )
>  
> -#define spin_lock_init_prof(s, l) __spin_lock_init_prof(s, l, spinlock_t)
> -#define rspin_lock_init_prof(s, l) __spin_lock_init_prof(s, l, rspinlock_t)
> +#define spin_lock_init_prof(s, l)
>  \
> +__spin_lock_init_prof(s, l, lock, spinlock_t, 0)

... "false" here, ...

> +#define rspin_lock_init_prof(s, l)   
>  \
> +__spin_lock_init_prof(s, l, rlock, rspinlock_t, 1)

... "true" again here, and ...

> @@ -174,6 +179,7 @@ struct lock_profile_qhead { };
>  
>  #endif
>  
> +
>  typedef union {
>  uint32_t head_tail;
>  struct {

... definitely with this hunk dropped.

Jan

Re: [PATCH v5 03/23] xen/riscv: introduce nospec.h

[Julien Grall]

Hi Oleksii,

On 26/02/2024 17:38, Oleksii Kurochko wrote:

 From the unpriviliged doc:
   No standard hints are presently defined.
   We anticipate standard hints to eventually include memory-system spatial
   and temporal locality hints, branch prediction hints, thread-scheduling
   hints, security tags, and instrumentation flags for simulation/emulation.

Also, there are no speculation execution barriers.

Therefore, functions evaluate_nospec() and block_speculation() should
remain empty until a specific platform has an extension to deal with
speculation execution.

Signed-off-by: Oleksii Kurochko 
---
  Changes in V5:
- new patch
---
  xen/arch/riscv/include/asm/nospec.h | 25 +
  1 file changed, 25 insertions(+)
  create mode 100644 xen/arch/riscv/include/asm/nospec.h

diff --git a/xen/arch/riscv/include/asm/nospec.h 
b/xen/arch/riscv/include/asm/nospec.h
new file mode 100644
index 00..4fb404a0a2
--- /dev/null
+++ b/xen/arch/riscv/include/asm/nospec.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */


New file should use the SPDX tag GPL-2.0-only. I guess this could be 
fixed on commit?


Cheers,

--
Julien Grall