Re: [PATCH v7 8/8] xen/arm: mmu: move MMU specific P2M code to mmu/p2m.{c,h}
Hi Julien, > On Oct 14, 2023, at 02:22, Julien Grall wrote: > > Hi Henry, > > On 09/10/2023 02:03, Henry Wang wrote: >> >> diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h >> index 940495d42b..a9622dac9a 100644 >> --- a/xen/arch/arm/include/asm/p2m.h >> +++ b/xen/arch/arm/include/asm/p2m.h >> @@ -19,6 +19,22 @@ extern unsigned int p2m_root_level; >> #define P2M_ROOT_ORDERp2m_root_order > > You seem to use P2M_ROOT_ORDER to allocate p2m->root in arm/p2m.c. However, > as I mentioned before, I don't think the defintion of p2m->root is suitable > for the MPU. I think the two functions using p2m->root should be moved in > mmu/p2m.c and P2M_ROOT_ORDER should be moved in mmu/p2m.h. While working on this, I noticed that if we move p2m_final_teardown() (one of the two functions you mentioned that consuming p2m->root), we also need to move the static function p2m_free_vmid(). This seems reasonable as on MPU we only have dom0less domUs so we don’t really need to free vmid. However, if p2m_free_vmid() is moved to mmu/p2m.c, we have two consumers of "static spinlock_t vmid_alloc_lock” and "static unsigned long *vmid_mask". So I am writing this email just to confirm that are you fine with exposing these two global variables? Thanks! The other comments seem fine and I’ve fixed all of them. Kind regards, Henry
Re: [PATCH for-4.18] docs/sphinx: Lifecycle of a domid
On 16.10.23 18:24, Andrew Cooper wrote: Signed-off-by: Andrew Cooper --- CC: George Dunlap CC: Jan Beulich CC: Stefano Stabellini CC: Wei Liu CC: Julien Grall CC: Roger Pau Monné CC: Juergen Gross CC: Henry Wang Rendered form: https://andrewcoop-xen.readthedocs.io/en/docs-devel/hypervisor-guide/domid-lifecycle.html I'm not sure why it's using the alibaster theme and not RTD theme, but I don't have time to debug that further at this point. This was written mostly while sat waiting for flights in Nanjing and Beijing. If while reading this you spot a hole, congratulations. There are holes which need fixing... --- docs/glossary.rst | 9 ++ docs/hypervisor-guide/domid-lifecycle.rst | 164 ++ docs/hypervisor-guide/index.rst | 1 + 3 files changed, 174 insertions(+) create mode 100644 docs/hypervisor-guide/domid-lifecycle.rst diff --git a/docs/glossary.rst b/docs/glossary.rst index 8ddbdab160a1..1fd1de0f0e97 100644 --- a/docs/glossary.rst +++ b/docs/glossary.rst @@ -50,3 +50,12 @@ Glossary By default it gets all devices, including all disks and network cards, so is responsible for multiplexing guest I/O. + + system domain + Abstractions within Xen that are modelled in a similar way to regular + :term:`domains`. E.g. When there's no work to do, Xen schedules + ``DOMID_IDLE`` to put the CPU into a lower power state. + + System domains have :term:`domids` and are referenced by + privileged software for certain control operations, but they do not run + guest code. diff --git a/docs/hypervisor-guide/domid-lifecycle.rst b/docs/hypervisor-guide/domid-lifecycle.rst new file mode 100644 index ..d405a321f3c7 --- /dev/null +++ b/docs/hypervisor-guide/domid-lifecycle.rst @@ -0,0 +1,164 @@ +.. SPDX-License-Identifier: CC-BY-4.0 + +Lifecycle of a domid + + +Overview + + +A :term:`domid` is Xen's numeric identifier for a :term:`domain`. In any +operational Xen system, there are one or more domains running. + +Domids are 16-bit integers. Regular domids start from 0, but there are some +special identifiers, e.g. ``DOMID_SELF``, and :term:`system domains`, e.g. ``DOMID_IDLE`` starting from 0x7ff0. Therefore, a Xen system +can run a maximum of 32k domains concurrently. + +.. note:: + + Despite being exposed in the domid ABI, the system domains are internal to + Xen and do not have lifecycles like regular domains. Therefore, they are + not discussed further in this document. + +At system boot, Xen will construct one or more domains. Kernels and +configuration for these domains must be provided by the bootloader, or at +Xen's compile time for more highly integrated solutions. + +Correct functioning of the domain lifecycle involves ``xenstored``, and some +privileged entity which has bound the ``VIRQ_DOM_EXC`` global event channel. + +.. note:: + + While not a strict requirement for these to be the same entity, it is + ``xenstored`` which typically has ``VIRQ_DOM_EXC`` bound. This document is + written assuming the common case. + +Creation + + +Within Xen, the ``domain_create()`` function is used to allocate and perform +bare minimum construction of a domain. The :term:`control domain` accesses +this functionality via the ``DOMCTL_createdomain`` hypercall. + +The final action that ``domain_create()`` performs before returning +successfully is to enter the new domain into the domlist. This makes the +domain "visible" within Xen, allowing the new domid to be successfully +referenced by other hypercalls. + +At this point, the domain exists as far as Xen is concerned, but not usefully +as a VM yet. The toolstack performs further construction activities; +allocating vCPUs, RAM, copying in the initial executable code, etc. Domains +are automatically created with one "pause" reference count held, meaning that +it is not eligible for scheduling. + +When the toolstack has finished VM construction, it send an ``XS_INTRODUCE`` s/send/sends/ +command to ``xenstored``. This instructs ``xenstored`` to connect to the +guest's xenstore ring, and fire the ``@introduceDomain`` watch. The firing of +this watch is the signal to all other components which care that a new VM has +appeared and is about to start running. A note should be added that the control domain is introduced implicitly by xenstored, so no XS_INTRODUCE command is needed and no @introduceDomain watch is being sent for the control domain. All components interested in the @introduceDomain watch have to find out for themselves which new domain has appeared, as the watch event doesn't contain the domid of the new domain. + +When the ``XS_INTRODUCE`` command returns successfully, the final action the +toolstack performs is to unpause the guest, using the ``DOMCTL_unpausedomain`` +hypercall. This drops the "pause" reference the domain was originally created +with, meaning that the
Re: [PATCH v4 2/2] xen/arm: Enlarge identity map space to 10TB
Hi Julien,
> On Oct 16, 2023, at 22:46, Julien Grall wrote:
>
> CC Henry
Thanks.
> Hi Alexey,
>
> On 16/10/2023 15:24, Alexey Klimov wrote:
>> On Mon, 16 Oct 2023 at 15:13, Luca Fancellu wrote
>>>
> On 16/10/2023 09:44, Michal Orzel wrote:
>> On 13/10/2023 14:26, Leo Yan wrote:
>>>
>>>
>>> On ADLink AVA platform (Ampere Altra SoC with 32 Arm Neoverse N1 cores),
>>> the physical memory regions are:
>>>
>>> DRAM memory regions:
>>>Node[0] Region[0]: 0x8000 - 0x
>>>Node[0] Region[1]: 0x0800 - 0x08007fff
>>>Node[0] Region[2]: 0x0801 - 0x0807
>>>
>>> The UEFI loads Xen hypervisor and DTB into the high memory, the kernel
>>> and ramdisk images are loaded into the low memory space:
>>>
>>> (XEN) MODULE[0]: 0807f6df - 0807f6f3e000 Xen
>>> (XEN) MODULE[1]: 0807f8054000 - 0807f8056000 Device Tree
>>> (XEN) MODULE[2]: fa834000 - fc5de1d5 Ramdisk
>>> (XEN) MODULE[3]: fc5df000 - ffb3f810 Kernel
>>>
>>> In this case, the Xen binary is loaded above 8TB, which exceeds the
>>> maximum supported identity map space of 2TB in Xen. Consequently, the
>>> system fails to boot.
>>>
>>> This patch enlarges identity map space to 10TB, allowing module loading
>>> within the range of [0x0 .. 0x09ff__].
>>>
>>> Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to
>>> prepare/enable/disable")
>> I don't think a fixes tag applies here given that 2TB was just a number
>> we believed is enough
>> and all of this is platform dependent.
>> This can be dropped on commit if committer agrees
> Xen may have booted on that platform before hand. So this would be
> considered a regression and therefore a tag would be warrant.
>
> AFAICT, the commit is only present on the upcoming 4.18. So the question
> is whether Xen 4.17 booted out-of-the-box on ADLink? If the answer is
> yes, then we need to add a Fixes tag. But the correct one would be
>
@Rahul or Luca: could you give an answer here ?
I know you used Xen on an AVA platform but was it booting out of the box ?
>>>
>>> I can’t say for Xen 4.17, but our nightly job has run successfully on AVA
>>> for the commit 730406ab81094115d9fb5ca00ba8d53cec1279b3
>>> (docs/misra: add deviations.rst to document additional deviations.)
>>>
>>> We are not applying any patch for it to run on AVA.
>> Most likely it is because your UEFI/BIOS firmware is 2.x, for instance
>> 2.04.100.07.
>> This fix if for AVA machine with older UEFI firmware 1.07.300.03.
>
> OOI, why not updating your firmware? I was expecting that it would also
> contain other critical fixes.
Not sure what is the reason from Alexey’s side, but from my experience of
updating AVA firmware, it is sometimes tricky as I remember I did 5 boards
using the same set of upgrade instructions but one board was bricked
magically during this progress (Funny that the reason why we upgrade
the firmware was to fix this issue…). Once this happens, the only method to
recover is to have a dediprog or something similar to program the flash.
If there is no handy tools, then there would be a problem…So I understand
people may don’t really want to do the upgrading, but indeed I also recommend
to upgrade the firmware.
>
> With this in mind, I am now more in two mind to ask to merge this patch in
> Xen 4.18. On one hand, I understand it will help to boot on AVA machine with
> an older firmware. But on the other hand this is changing the memory layout
> quite late in the release. The risk seems limited because Xen is not loaded
> at the top of the virtual address space (there is the directmap afterwards).
>
> Henry (as the release manager) and others, any opinions?
I tested this series on top of today’s staging in our CI and it seems that this
series is not breaking any boards/emulators we used for testing, so:
Tested-by: Henry Wang
I saw you and Bertrand had a slightly different opinion on this and probably
we need Stefano’s input too, Any ideas Stefano?
But I personally won’t block this patch, so if the discussion is settled to
merge this to 4.18, feel free:
Release-acked-by: Henry Wang
Kind regards,
Henry
>
> Cheers,
>
> --
> Julien Grall
Re: [for-4.18][PATCH] xen/arm: Check return code from recursive calls to scan_pfdt_node()
Hi Michal,
> On Oct 16, 2023, at 21:28, Bertrand Marquis wrote:
>
> Hi Michal,
>
>> On 16 Oct 2023, at 14:45, Michal Orzel wrote:
>>
>> At the moment, we do not check a return code from scan_pfdt_node()
>> called recursively. This means that any issue that may occur while
>> parsing and copying the passthrough nodes is hidden and Xen continues
>> to boot a domain despite errors. This may lead to incorrect device tree
>> generation and various guest issues (e.g. trap on attempt to access MMIO
>> not mapped in P2M). Fix it.
>>
>> Fixes: 669ecdf8d6cd ("xen/arm: copy dtb fragment to guest dtb")
>> Signed-off-by: Michal Orzel
>
> Good finding :-)
>
> Reviewed-by: Bertrand Marquis
Release-acked-by: Henry Wang
Kind regards,
Henry
>
> Cheers
> Bertrand
>
>> ---
>> @Henry:
>> This is a bug fix, so I think we should have it in 4.18 given the possible
>> consequences I described in the commit msg. I don't see any risks as this
>> change
>> only checks the return code for an error.
>> ---
>> xen/arch/arm/domain_build.c | 7 +--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 24c9019cc43c..49792dd590ee 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -2872,8 +2872,11 @@ static int __init scan_pfdt_node(struct kernel_info
>> *kinfo, const void *pfdt,
>>node_next = fdt_first_subnode(pfdt, nodeoff);
>>while ( node_next > 0 )
>>{
>> -scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells,
>> - scan_passthrough_prop);
>> +rc = scan_pfdt_node(kinfo, pfdt, node_next, address_cells,
>> size_cells,
>> +scan_passthrough_prop);
>> +if ( rc )
>> +return rc;
>> +
>>node_next = fdt_next_subnode(pfdt, node_next);
>>}
>>
>> --
>> 2.25.1
>>
>
Xen 4.18 rc3
Hi all, Xen 4.18 rc3 is tagged. You can check that out from xen.git: git://xenbits.xen.org/xen.git 4.18.0-rc3 For your convenience there is also a tarball at: https://downloads.xenproject.org/release/xen/4.18.0-rc3/xen-4.18.0-rc3.tar.gz And the signature is at: https://downloads.xenproject.org/release/xen/4.18.0-rc3/xen-4.18.0-rc3.tar.gz.sig Please send bug reports and test reports to xen-devel@lists.xenproject.org. When sending bug reports, please CC relevant maintainers and me (henry.w...@arm.com). Kind regards, Henry
Re: SAF-x-safe rename
On Mon, 16 Oct 2023, Jan Beulich wrote: > On 06.10.2023 00:01, Andrew Cooper wrote: > > On 05/10/2023 10:38 pm, andrew.coop...@citrix.com wrote: > >> On 05/10/2023 8:43 am, Luca Fancellu wrote: > On 5 Oct 2023, at 00:46, Stefano Stabellini > wrote: > > Hi MISRA C working group (Jan, Roger, Andrew, Julien, Bertrand, George) > > in a recent thread Andrew pointed out that the SAF-2-safe tag is > confusing and requested a rename: > https://marc.info/?l=xen-devel=169634970821202 > > As documented by docs/misra/documenting-violations.rst: > > - SAF-X-safe: This tag means that the next line of code contains a > finding, but > the non compliance to the checker is analysed and demonstrated to be > safe. > - SAF-X-false-positive-: This tag means that the next line of code > contains a finding, but the finding is a bug of the tool. > > > Today we have already 28 instances of SAF tags in the Xen codebase. > > > Andrew suggested "ANALYSIS" instead of SAF so I would imagine: > - ANALYSIS-X-safe > - ANALYSIS-X-false-positive- > > If we really want a rename I suggest to rename SAF to SAFE: > - SAFE-X-safe > - SAFE-X-false-positive- > > Or maybe MISRA: > - MISRA-X-safe > - MISRA-X-false-positive- > > But I actually prefer to keep the tag as it is today. > >>> We chose a generic name instead of MISRA because the tag can potentially > >>> suppress findings > >>> of any checker, including MISRA checker. > >>> > >>> If SAF-* is confusing, what about FUSA-* ? > >>> > >>> Anyway I’m thinking that every name we could come up will be confusing at > >>> first, improving the > >>> documentation would mitigate it (by improving I mean to improve the > >>> fruition of it, for example a > >>> Read the docs documentation has the search bar, a quick copy paste of > >>> SAF- would make the > >>> documenting-violations page visible.) > >> > >> No - this is a problem *because* changing the documentation doesn't > >> help. (To be clear, updating the documentation is fine, but irrelevant > >> here.) > >> > >> > >> These are annotations in code. They need to be: > >> > >> 1) Short (obviously) > >> 2) Clear to someone who isn't you (the collective us of this group) > >> reading the code. > >> 3) Non-intrusive, so as not to get in the way of the code. > >> > >> and they must be all three. This was even a principle given at the > >> start of the MISRA work that we would not be deteriorating the quality > >> of the code just to comply. > >> > >> Point 3 is other thread about end-of-line, or block regions. Lets leave > >> that there because it's really a metadata transformation problem > >> constrained by where the comments can acceptably go. > >> > >> > >> Point 2 is the issue here, and "SAF-$N-safe" scores very highly on the > >> WTF-o-meter *even* for people who know that it's something related to > >> MISRA. > >> > >> Seriously it looks like someone couldn't spell, and everyone else went > >> with it (reflects poorly on everyone else). > >> > >> And yes, I know it's an initialisation for something, but it's not even > >> an industry standard term - it's a contraction of an intentionally > >> generic phrase, with substantial irony on an early MISRA call where > >> there was uncertainly between people as to what it even stood for. > >> > >> These are the thoughts running through the minds of people reading the > >> code when they don't understand what they're looking at. > >> > >> > >> Annotations for other static analysers intentionally use their own name > >> so they're googleable. > >> > >> Guess what SAF googles for? Sustainable Aviation Fuel, or Specialist > >> Automotive Finance. > >> > >> Fine, lets be more specific. How about "Xen SAF" ? Nope... > >> > >> "Did you mean to search for: > >> Xen SAVE Xen SAN Xen VIF Xenstaff" > >> > >> > >> Despite many of the search results referencing patches, or rendered > >> documents out of docs/, not a single one of them gets > >> documenting-violations.rst in any form, where the single definition of > >> this term is hiding in a paragraph which spends 90% of it's volume > >> describing a monotonically increasing number. > >> > >> Seriously, ChatGPT would struggle to make shit this good up. > >> > >> > >> The thing we tag with *must* be trivially recognisable as an analysis > >> tag in order for others to be able to read the code. Therefore, it > >> needs to be an actual full world (hence the ANALYSIS suggestion), or an > >> industry standard term (where MISRA does qualify). > > ANALYSIS imo gets in conflict with 1) above, considering that permitting > line length violations was already brought up with the shorter SAF-*. > > >> I don't exactly what it is - something else might turn out to be even > >> better, but it is very important to be not this, for everyone else to > >> have an easy time reading the
Re: [PATCH v7 8/8] xen/arm: mmu: move MMU specific P2M code to mmu/p2m.{c,h}
Hi Julien, > On Oct 17, 2023, at 00:41, Julien Grall wrote: > On 14/10/2023 02:26, Henry Wang wrote: >> Hi Julien, > > Hi Henry, > >>> On Oct 14, 2023, at 02:22, Julien Grall wrote: >>> >>> Hi Henry, >>> >>> On 09/10/2023 02:03, Henry Wang wrote: From: Penny Zheng Current P2M implementation is designed for MMU system only. We move the MMU-specific codes into mmu/p2m.c, and only keep generic codes in p2m.c, like VMID allocator, etc. We also move MMU-specific definitions and declarations to mmu/p2m.h, such as p2m_tlb_flush_sync(). Also expose previously static functions p2m_vmid_allocator_init(), p2m_alloc_vmid(), and setup_virt_paging_one() for further MPU usage. With the code movement, global variable max_vmid is used in multiple files instead of a single file (and will be used in MPU P2M implementation), declare it in the header and remove the "static" of this variable. Signed-off-by: Penny Zheng Signed-off-by: Wei Chen Signed-off-by: Henry Wang >>> >>> Some remarks about some of the code not moved: >>> * struct p2m_domain: The bulk of the fields seems to be MMU specific. So >>> depending on the number of common fields we either want to split or move >>> the structure to p2m_domain. I would be ok to wait until the MPU code is >>> present. >>> * p2m_type_t: It is not yet clear how this will apply to the MPU. I am ok >>> to wait before moving it. >> Agree with both here, let’s continue the discussion in the actual MPU patch >> for P2M >> then, but I am then a bit confused about... >>> * p2m_cache_flush_range(): I expect the code will need some change because >>> you may get large chunk of memory for the MPU. >>> * p2m_set_way_flush()/p2m_toggle_cache(): This was a giant hack to support >>> cache flush operations via set/way. To make it efficient, we track the >>> pages that have been touched and only flush them. For the MPU, this would >>> not work. Can we attempt to not emulate the instructions? >> …these two remarks here, do you expect me to do some changes with these three >> functions in this patch? Or we can defer the required changes to the MPU >> patch for >> P2M? > > My original intention was to ask to move them right now. But if it is unclear > whether they would be used, then it would be best to defer until we have a > better understanding. Sure, I think I will defer it then. > >> I think I am highly likely to make a mistake here but I took a look at the >> MPU >> implementation [1] and it looks like the MPU code can use these tree >> functions >> without changes - probably because these functions are simply used by >> (1) domctl and we only have dom0less DomUs on MPU >> (2) trap handlers >> which means these functions are simply not called… > > I am not sure I fully understand why would the trap handlers not called. Is > this suggesting that a dom0less domUs can not use set/way instructions? No I was wrong, sorry for the confusion, I somehow “mis-linked” the “trap” to “panic” :/ But as I saw your reply above, so I think probably we can defer the movement of these functions to further discussions in MPU series. Kind regards, Henry > > Cheers, > > -- > Julien Grall
[xen-unstable test] 183383: tolerable FAIL - PUSHED
flight 183383 xen-unstable real [real] flight 183391 xen-unstable real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/183383/ http://logs.test-lab.xenproject.org/osstest/logs/183391/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 7 xen-install fail pass in 183391-retest Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 183379 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 183379 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 183379 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 183379 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 183379 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 183379 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 183379 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 183379 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 183379 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 183379 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 183379 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 183379 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt-xsm 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-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-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-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass version targeted for testing: xen 6432228fb5808150d3c5c14affb3d46af81b3878 baseline version: xen 730406ab81094115d9fb5ca00ba8d53cec1279b3 Last test of basis 183379 2023-10-16
Re: [XEN PATCH 02/10] arm/cpufeature: address violations of MISRA C:2012 Rule 8.2
On Mon, 16 Oct 2023, Julien Grall wrote:
> On 14/10/2023 00:34, Stefano Stabellini wrote:
> > On Fri, 13 Oct 2023, Stefano Stabellini wrote:
> > > On Fri, 13 Oct 2023, Federico Serafini wrote:
> > > > Add missing parameter names, no functional change.
> > > >
> > > > Signed-off-by: Federico Serafini
> > > > ---
> > > > xen/arch/arm/include/asm/cpufeature.h | 8
> > > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/xen/arch/arm/include/asm/cpufeature.h
> > > > b/xen/arch/arm/include/asm/cpufeature.h
> > > > index 8011076b8c..41e97c23dd 100644
> > > > --- a/xen/arch/arm/include/asm/cpufeature.h
> > > > +++ b/xen/arch/arm/include/asm/cpufeature.h
> > > > @@ -127,8 +127,8 @@ static inline void cpus_set_cap(unsigned int num)
> > > > struct arm_cpu_capabilities {
> > > > const char *desc;
> > > > u16 capability;
> > > > -bool (*matches)(const struct arm_cpu_capabilities *);
> > > > -int (*enable)(void *); /* Called on every active CPUs */
> > > > +bool (*matches)(const struct arm_cpu_capabilities *caps);
> > >
> > > all the implementations of matches I found in xen/arch/arm/cpuerrata.c
> > > actually call the parameter "entry"
> > >
> > >
> > > > +int (*enable)(void *ptr); /* Called on every active CPUs */
> > >
> > > this one instead is "data"
> >
> > I committed all the other patches in this series to the my for-4.19 branch
>
> I have left some comments in patch #1. Given this is not the latest master, I
> think we should consider to remove/replace the patch rather than introducing a
> follow-up.
Makes sense. I took out patch #1 of this series and also took the
opportunity to rebase 4.19 on the latest staging
Re: [PATCH] cxenstored: wait until after reset to notify dom0less domains
Hi Stefano,
> On Oct 17, 2023, at 07:14, Stefano Stabellini wrote:
>
> +Henry for release ack
Thanks, I think we did have Juergen for reviewing this patch so:
Release-acked-by: Henry Wang
Kind regards,
Henry
>
>
> On Fri, 13 Oct 2023, Stefano Stabellini wrote:
>> From: George Dunlap
>>
>> Commit fc2b57c9a ("xenstored: send an evtchn notification on
>> introduce_domain") introduced the sending of an event channel to the
>> guest when first introduced, so that dom0less domains waiting for the
>> connection would know that xenstore was ready to use.
>>
>> Unfortunately, it was introduced in introduce_domain(), which 1) is
>> called by other functions, where such functionality is unneeded, and
>> 2) after the main XS_INTRODUCE call, calls domain_conn_reset(). This
>> introduces a race condition, whereby if xenstored is delayed, a domain
>> can wake up, send messages to the buffer, only to have them deleted by
>> xenstore before finishing its processing of the XS_INTRODUCE message.
>>
>> Move the connect-and-notfy call into do_introduce() instead, after the
>> domain_conn_rest(); predicated on the state being in the
>> XENSTORE_RECONNECT state.
>>
>> (We don't need to check for "restoring", since that value is always
>> passed as "false" from do_domain_introduce()).
>>
>> Also take the opportunity to add a missing wmb barrier after resetting
>> the indexes of the ring in domain_conn_reset.
>>
>> This change will also remove an extra event channel notification for
>> dom0 (because the notification is now done by do_introduce which is not
>> called for dom0.) The extra dom0 event channel notification was only
>> introduced by fc2b57c9a and was never present before. It is not needed
>> because dom0 is the one to tell xenstored the connection parameters, so
>> dom0 has to know that the ring page is setup correctly by the time
>> xenstored starts looking at it. It is dom0 that performs the ring page
>> init.
>>
>> Signed-off-by: George Dunlap
>> Signed-off-by: Stefano Stabellini
>> CC: jgr...@suse.com
>> CC: jul...@xen.org
>> CC: w...@xen.org
>> ---
>> tools/xenstored/domain.c | 14 --
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
>> index a6cd199fdc..f6ef37856c 100644
>> --- a/tools/xenstored/domain.c
>> +++ b/tools/xenstored/domain.c
>> @@ -923,6 +923,7 @@ static void domain_conn_reset(struct domain *domain)
>>
>> domain->interface->req_cons = domain->interface->req_prod = 0;
>> domain->interface->rsp_cons = domain->interface->rsp_prod = 0;
>> + xen_wmb();
>> }
>>
>> /*
>> @@ -988,12 +989,6 @@ static struct domain *introduce_domain(const void *ctx,
>> /* Now domain belongs to its connection. */
>> talloc_steal(domain->conn, domain);
>>
>> - if (!restore) {
>> - /* Notify the domain that xenstore is available */
>> - interface->connection = XENSTORE_CONNECTED;
>> - xenevtchn_notify(xce_handle, domain->port);
>> - }
>> -
>> if (!is_master_domain && !restore)
>> fire_special_watches("@introduceDomain");
>> } else {
>> @@ -1033,6 +1028,13 @@ int do_introduce(const void *ctx, struct connection
>> *conn,
>>
>> domain_conn_reset(domain);
>>
>> + if (domain->interface != NULL &&
>> + domain->interface->connection == XENSTORE_RECONNECT) {
>> + /* Notify the domain that xenstore is available */
>> + domain->interface->connection = XENSTORE_CONNECTED;
>> + xenevtchn_notify(xce_handle, domain->port);
>> + }
>> +
>> send_ack(conn, XS_INTRODUCE);
>>
>> return 0;
>> --
>> 2.25.1
>>
Re: [XEN PATCH] xen/irq: address violations of MISRA C:2012 Rule 8.2
> On Oct 16, 2023, at 23:14, Jan Beulich wrote: > > On 03.10.2023 02:19, Henry Wang wrote: >>> On Oct 3, 2023, at 06:45, Stefano Stabellini wrote: >>> >>> On Mon, 2 Oct 2023, Federico Serafini wrote: Add missing parameter names. No functional change. Signed-off-by: Federico Serafini >>> >>> Reviewed-by: Stefano Stabellini >> >> Release-acked-by: Henry Wang > > Same question here wrt applicability now that we're past RC3. Same answer here for this patch, as this is a simple one and nothing behavioral is expected to change. So I am ok to commit this. Kind regards, Henry > > Jan
Re: [XEN PATCH] x86/paging: address a violation of MISRA C:2012 Rule 8.3
Hi Jan, > On Oct 16, 2023, at 23:12, Jan Beulich wrote: > > On 03.10.2023 02:19, Henry Wang wrote: >>> On Oct 3, 2023, at 06:46, Stefano Stabellini wrote: >>> >>> On Mon, 2 Oct 2023, Federico Serafini wrote: Make function declaration and definition consistent. No functional change. Signed-off-by: Federico Serafini >>> >>> Reviewed-by: Stefano Stabellini >> >> Release-acked-by: Henry Wang > > We're past RC3, so I'd like to confirm whether this still holds. Thanks for reaching out. Although we reached an agreement that we need a case by case analysis for if merging MISRA patches or not to 4.18, and technically MISRA patches should be committed by RC2, to be honest for such a simple patch I am not going to block it because I really don’t think it will cause a behavioral change, so yes my release-ack tag still holds. Feel free to commit it. Kind regards, Henry > > Jan
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
On Mon, 16 Oct 2023, Jan Beulich wrote:
> On 16.10.2023 18:17, Nicola Vetrini wrote:
> > On 16/10/2023 17:33, Jan Beulich wrote:
> >> On 12.10.2023 17:28, Nicola Vetrini wrote:
> >>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> >>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> >>> @@ -274,6 +274,12 @@ still non-negative."
> >>> -config=MC3R1.R10.1,etypes+={safe,
> >>> "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))",
> >>>
> >>> "dst_type(ebool||boolean)"}
> >>> -doc_end
> >>>
> >>> +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern to
> >>> obtain the value
> >>> +2^ffs(x) for unsigned integers on two's complement architectures
> >>> +(all the architectures supported by Xen satisfy this requirement)."
> >>> +-config=MC3R1.R10.1,reports+={safe,
> >>> "any_area(any_loc(any_exp(macro(^LOWEST_BIT$"}
> >>> +-doc_end
> >>
> >> Why is this added here rather than by ...
> >>
> >>> --- a/xen/include/xen/macros.h
> >>> +++ b/xen/include/xen/macros.h
> >>> @@ -8,8 +8,10 @@
> >>> #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
> >>> #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
> >>>
> >>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
> >>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
> >>
> >> a SAF--safe comment here?
> >>
> >
> > One reason is that now that violations only belonging to tool
> > configurations
> > and similar are documented in docs/misra/deviations.rst (committed in
> > Stefano's
> > branch for-4.19 [1]).
>
> But tool configuration means every analysis tool needs configuring
> separately. That's why the comment tagging scheme was decided to be
> preferred, iirc.
>
> > Also, there were disagreements on the SAF naming
> > scheme, and
> > patches like those would not be accepted at the moment.
>
> Well, that needs resolving. The naming there shouldn't lead to patches
> being accepted that later may need redoing.
I agree
[ovmf test] 183390: all pass - PUSHED
flight 183390 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/183390/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 03d6569f70939d2a1653265367121212459a6b89 baseline version: ovmf 326b9e1d815c4ae4b0b207fcb0bfa16864af5400 Last test of basis 183355 2023-10-12 09:13:57 Z4 days Testing same since 183390 2023-10-16 23:10:46 Z0 days1 attempts People who touched revisions under test: Mike Maslenkin jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass test-amd64-i386-xl-qemuu-ovmf-amd64 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git 326b9e1d81..03d6569f70 03d6569f70939d2a1653265367121212459a6b89 -> xen-tested-master
Re: [PATCH] cxenstored: wait until after reset to notify dom0less domains
+Henry for release ack
On Fri, 13 Oct 2023, Stefano Stabellini wrote:
> From: George Dunlap
>
> Commit fc2b57c9a ("xenstored: send an evtchn notification on
> introduce_domain") introduced the sending of an event channel to the
> guest when first introduced, so that dom0less domains waiting for the
> connection would know that xenstore was ready to use.
>
> Unfortunately, it was introduced in introduce_domain(), which 1) is
> called by other functions, where such functionality is unneeded, and
> 2) after the main XS_INTRODUCE call, calls domain_conn_reset(). This
> introduces a race condition, whereby if xenstored is delayed, a domain
> can wake up, send messages to the buffer, only to have them deleted by
> xenstore before finishing its processing of the XS_INTRODUCE message.
>
> Move the connect-and-notfy call into do_introduce() instead, after the
> domain_conn_rest(); predicated on the state being in the
> XENSTORE_RECONNECT state.
>
> (We don't need to check for "restoring", since that value is always
> passed as "false" from do_domain_introduce()).
>
> Also take the opportunity to add a missing wmb barrier after resetting
> the indexes of the ring in domain_conn_reset.
>
> This change will also remove an extra event channel notification for
> dom0 (because the notification is now done by do_introduce which is not
> called for dom0.) The extra dom0 event channel notification was only
> introduced by fc2b57c9a and was never present before. It is not needed
> because dom0 is the one to tell xenstored the connection parameters, so
> dom0 has to know that the ring page is setup correctly by the time
> xenstored starts looking at it. It is dom0 that performs the ring page
> init.
>
> Signed-off-by: George Dunlap
> Signed-off-by: Stefano Stabellini
> CC: jgr...@suse.com
> CC: jul...@xen.org
> CC: w...@xen.org
> ---
> tools/xenstored/domain.c | 14 --
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
> index a6cd199fdc..f6ef37856c 100644
> --- a/tools/xenstored/domain.c
> +++ b/tools/xenstored/domain.c
> @@ -923,6 +923,7 @@ static void domain_conn_reset(struct domain *domain)
>
> domain->interface->req_cons = domain->interface->req_prod = 0;
> domain->interface->rsp_cons = domain->interface->rsp_prod = 0;
> + xen_wmb();
> }
>
> /*
> @@ -988,12 +989,6 @@ static struct domain *introduce_domain(const void *ctx,
> /* Now domain belongs to its connection. */
> talloc_steal(domain->conn, domain);
>
> - if (!restore) {
> - /* Notify the domain that xenstore is available */
> - interface->connection = XENSTORE_CONNECTED;
> - xenevtchn_notify(xce_handle, domain->port);
> - }
> -
> if (!is_master_domain && !restore)
> fire_special_watches("@introduceDomain");
> } else {
> @@ -1033,6 +1028,13 @@ int do_introduce(const void *ctx, struct connection
> *conn,
>
> domain_conn_reset(domain);
>
> + if (domain->interface != NULL &&
> + domain->interface->connection == XENSTORE_RECONNECT) {
> + /* Notify the domain that xenstore is available */
> + domain->interface->connection = XENSTORE_CONNECTED;
> + xenevtchn_notify(xce_handle, domain->port);
> + }
> +
> send_ack(conn, XS_INTRODUCE);
>
> return 0;
> --
> 2.25.1
>
Re: [XEN PATCH v2 3/7] x86: add deviation comments for asm-only functions
On Mon, 16 Oct 2023, Jan Beulich wrote: > On 09.10.2023 08:54, Nicola Vetrini wrote: > > As stated in rules.rst, functions used only in asm code > > are allowed to have no prior declaration visible when being > > defined, hence these functions are deviated. > > This also fixes violations of MISRA C:2012 Rule 8.4. > > > > Signed-off-by: Nicola Vetrini > > Reviewed-by: Stefano Stabellini > > --- > > xen/arch/x86/hvm/svm/intr.c | 1 + > > xen/arch/x86/hvm/svm/nestedsvm.c | 1 + > > xen/arch/x86/hvm/svm/svm.c | 2 ++ > > Once again - why are you not also adjusting the respective VMX code? > Iirc it was agreed long ago that scans should be extended to cover as > much of the code base as possible. Let me summarize here our past discussions on the subject to make sure we are all aligned. With my AMD hat on, of course we want to work with the upstream community as much as possible and improve the overall codebase. But it is not a goal for AMD to improve Intel-specific drivers (VMX and others). Our safety configuration for Xen, including the public ECLAIR instance currently sponsored by AMD, only includes SVM files, not VMX files. MISRA compliance costs time and effort; this was done both because of lack of interest in VMX and also as a cost saving measure. Upon maintainer's request we can expand the scope of individual patches. For example, AMD is not interested in ARM32 either, but in the past we did address certain MISRA C issues on ARM32 too, if nothing else for consistency of the code base. It comes down to a compromise what we should do for consistency of the codebase versus addressing things that makes sense for AMD business. For sure we could work on a few violations affecting Intel drivers, but overall I don't think AMD could be asked to make Intel drivers MISRA compliant. In addition to the above, we also discussed during one of the past MISRA C working group meetings to have larger-than-usual ECLAIR scans. ECLAIR takes a couple of hours to run, so it is a good idea to restrict its configuration in the usual runs. However, at least once a week maybe on a Sunday, it would be good to run a comprehensive scan including components that are not currently targeted for safety. This would help us detect regressions and in general spot potential bugs. As part of this larger-than-usual ECLAIR scan we could certainly include Intel drivers as well as other things currently unsupported. Now, concrete action items. Nicola, I think we should look into having a larger-than-usual ECLAIR scan every week which includes all of Intel files and in general as much as possible of the codebase. Jan, for this specific patch, I don't think we have the scan including Intel vmx files yet. Nicola please correct me if I am wrong. So Nicola wouldn't be able to easily expand this patch to also cover Intel vmx violations of this rule because we don't have the list of violations affecting those files.
Re: [XEN PATCH 10/10] arm/smmu: address violation of MISRA C:2012 Rule 8.2
On Mon, 16 Oct 2023, Bertrand Marquis wrote:
> > On 16 Oct 2023, at 15:38, Julien Grall wrote:
> >
> >
> >
> > On 16/10/2023 14:31, Bertrand Marquis wrote:
> >> Hi Julien,
> >
> > Hi Bertrand,
> >
> >>> On 16 Oct 2023, at 11:07, Julien Grall wrote:
> >>>
> >>> Hi,
> >>>
> >>> On 13/10/2023 16:24, Federico Serafini wrote:
> Add missing parameter names, no functional change.
> Signed-off-by: Federico Serafini
> ---
> xen/drivers/passthrough/arm/smmu.c | 6 +++---
> >>>
> >>> This file is using the Linux coding style because it is imported from
> >>> Linux. I was under the impression we would exclude such file for now.
> >>>
> >>> Looking at exclude-list.json, it doesn't seem to be present. I think this
> >>> patch should be replaced with adding a line in execlude-list.json.
> >> I think that during one of the discussions we said that this file already
> >> deviated quite a lot from the status in Linux and we wanted to turn it to
> >> Xen coding style in the future hence it is not listed in the exclude file.
> > AFAIK the SMMUv{1, 2} code didn't deviate very much from Linux. I can't
> > tell about the SMMUv3.
>
> True that i had the v3 code in mind, we might not want to do that for v1/2
> you are right.
>
> >
> >> At the end having a working smmu might be critical in a safety context so
> >> it will make sense to also check this part of xen.
> > I don't buy this argument right now. We have files in exclude-lists.json
> > that I would consider critical for Xen to run (e.g. common/bitmap.c,
> > common/libfdt). So if you want to use this argument then we need to move
> > critical components of Xen out of the exclusion list.
>
> I am sure we will at some point for runtime code but we cannot do everything
> on the first shot.
> I was not meaning to do that now but that it is something to consider.
Things that are in exclude-lists.json are source files that come from
other projects and also change very rarely. The argument that we don't
do MISRA C on the files in exclude-lists.json, it is not because those
files are unimportant, but because they change only once every many
years.
Of course the least we rely on exclude-lists.json the better.
For smmu.c, looking at the git history I think it is more actively
worked on than other files such as lib/rbtree.c or common/bitmap.c.
Given that backports from Linux to smmu.c are not straightforward anyway
(please correct me if I am wrong) then I think we should not add smmu.c
to exclude-lists.json and do MISRA for smmu.c.
On the other hand, if we think that doing MISRA for smmu.c is going to
make backports a lot harder, and we think that we want to do backports
"often" (every year or every couple of years) then maybe we shouldn't do
MISRA for smmu.c after all.
[xen-unstable-smoke test] 183389: tolerable all pass - PUSHED
flight 183389 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/183389/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen 0ce2ee7a16f2886c32b3f070bba3172f4a577aa4 baseline version: xen 618826f67306c32b2799e84ded9a8203f4109ccf Last test of basis 183385 2023-10-16 14:00:28 Z0 days Testing same since 183389 2023-10-16 18:03:55 Z0 days1 attempts People who touched revisions under test: Anthony PERARD George Dunlap 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 618826f673..0ce2ee7a16 0ce2ee7a16f2886c32b3f070bba3172f4a577aa4 -> smoke
Re: [XEN v1 3/4] xen/arm32: Split and move MMU-specific head.S to mmu/head.S
Hi,
On 11/09/2023 14:59, Ayan Kumar Halder wrote:
This is based on
"[PATCH v6 03/13] xen/arm64: Split and move MMU-specific head.S to mmu/head.S"
https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg151920.html
Same remark as patch #1. Please provide a proper commit message.
[...]
diff --git a/xen/arch/arm/include/asm/arm32/macros.h
b/xen/arch/arm/include/asm/arm32/macros.h
index a4e20aa520..64fbe4876a 100644
--- a/xen/arch/arm/include/asm/arm32/macros.h
+++ b/xen/arch/arm/include/asm/arm32/macros.h
@@ -1,8 +1,70 @@
#ifndef __ASM_ARM_ARM32_MACROS_H
#define __ASM_ARM_ARM32_MACROS_H
+/* Offset between the early boot xen mapping and the runtime xen mapping */
+#define XEN_TEMPORARY_OFFSET (TEMPORARY_XEN_VIRT_START - XEN_VIRT_START)
+
.macro ret
mov pc, lr
.endm
+/*
+ * Move an immediate constant into a 32-bit register using movw/movt
+ * instructions.
+ */
+.macro mov_w reg, word
+movw \reg, #:lower16:\word
+movt \reg, #:upper16:\word
+.endm
+
+/*
+ * Pseudo-op for PC relative adr , where is
+ * within the range +/- 4GB of the PC.
+ *
+ * @dst: destination register
+ * @sym: name of the symbol
+ */
+.macro adr_l, dst, sym
+mov_w \dst, \sym - .Lpc\@
+.set .Lpc\@, .+ 8 /* PC bias */
+add \dst, \dst, pc
+.endm
+
+.macro load_paddr rb, sym
+mov_w \rb, \sym
+add \rb, \rb, r10
+.endm
I see that we are still using load_paddr in arm64/head.S. But I don't
think it makes entirely sense as r10 (x20) would always be 0 for the MPU.
In fact all the use in arm{32, 64}/head.S after this patch could be
easily replaced with adr_l because they are called when the MMU is off
so the return address will be a physical address.
So can you add a prequisite patch for arm32 to replace the remaining
some of the load_paddr? With that, load_paddr can be moved in mmu/head.S.
I will take care of the arm64 part.
Cheers,
--
Julien Grall
Re: [XEN v1 2/4] xen/arm32: head.S: Introduce enable_{boot,secondary}_cpu_mm()
Hi Ayan,
On 11/09/2023 14:59, Ayan Kumar Halder wrote:
This is based on:-
"[PATCH v6 01/13] xen/arm64: head.S: Introduce enable_{boot,secondary}_cpu_mm()"
https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg151918.html
Most of the explanation in that commit message doesn't apply here. You
also move call like setup_fixmap without explaining why.
Also, if you want to refer to the arm64 patch in the commit message (I
think it would be better after ---), then it is best to point ot the now
committed patch.
This is being done for Arm32 as MPU support will be added for Arm32 as well.
Signed-off-by: Ayan Kumar Halder
---
xen/arch/arm/arm32/head.S | 90 +--
1 file changed, 67 insertions(+), 23 deletions(-)
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 057c44a5a2..c0c425eac6 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -201,13 +201,10 @@ past_zImage:
blcheck_cpu_mode
blcpu_init
NIT: I would add a newline to make it clearer.
-blcreate_page_tables
+ldr lr, =primary_switched
The existing code was using 'mov_w ...' to avoid load from memory. Can
you explain why you switched to 'ldr'?
+b enable_boot_cpu_mm
-/* Address in the runtime mapping to jump to after the MMU is enabled */
-mov_w lr, primary_switched
-b enable_mmu
primary_switched:
-blsetup_fixmap
#ifdef CONFIG_EARLY_PRINTK
/* Use a virtual address to access the UART. */
mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
@@ -249,27 +246,11 @@ GLOBAL(init_secondary)
#endif
blcheck_cpu_mode
blcpu_init
-blcreate_page_tables
-/* Address in the runtime mapping to jump to after the MMU is enabled */
mov_w lr, secondary_switched
-b enable_mmu
-secondary_switched:
-/*
- * Non-boot CPUs need to move on to the proper pagetables, which were
- * setup in prepare_secondary_mm.
- *
- * XXX: This is not compliant with the Arm Arm.
- */
-mov_w r4, init_ttbr /* VA of HTTBR value stashed by CPU 0 */
-ldrd r4, r5, [r4] /* Actual value */
-dsb
-mcrr CP64(r4, r5, HTTBR)
-dsb
-isb
-flush_xen_tlb_local r0
-pt_enforce_wxn r0
+b enable_secondary_cpu_mm
+secondary_switched:
#ifdef CONFIG_EARLY_PRINTK
/* Use a virtual address to access the UART. */
mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
@@ -692,6 +673,69 @@ ready_to_switch:
mov pc, lr
ENDPROC(switch_to_runtime_mapping)
+/*
+ * Enable mm (turn on the data cache and the MMU) for secondary CPUs.
+ * The function will return to the virtual address provided in LR (e.g. the
+ * runtime mapping).
+ *
+ * Inputs:
+ * lr : Virtual address to return to.
+ *
+ * Clobbers r0 - r6
+ */
+enable_secondary_cpu_mm:
+mov r6, lr
NIT: I would add a newline to make it clearer.
+blcreate_page_tables
+
I think the comment on top of "mov_w lr, secondary_switched" should be
moved here.
+mov_w lr, secondary_cpu_mmu_on
+b enable_mmu
+secondary_cpu_mmu_on:
I would prefer if we use 1 to avoid introducing local label.
+/*
+ * Non-boot CPUs need to move on to the proper pagetables, which were
+ * setup in prepare_secondary_mm.
+ *
+ * XXX: This is not compliant with the Arm Arm.
+ */
+mov_w r4, init_ttbr /* VA of HTTBR value stashed by CPU 0 */
+ldrd r4, r5, [r4] /* Actual value */
+dsb
+mcrr CP64(r4, r5, HTTBR)
+dsb
+isb
+flush_xen_tlb_local r0
+pt_enforce_wxn r0
+mov lr, r6
+
+/* Return to the virtual address requested by the caller. */
+mov pc, lr
The above two instructions can be combined to:
/* Return to the virtual address requested by the caller (r6). */
mov pc, r6
+ENDPROC(enable_secondary_cpu_mm)
+
+/*
+ * Enable mm (turn on the data cache and the MMU) for the boot CPU.
+ * The function will return to the virtual address provided in LR (e.g. the
+ * runtime mapping).
+ *
+ * Inputs:
+ * lr : Virtual address to return to.
+ *
+ * Clobbers r0 - r6
+ */
+enable_boot_cpu_mm:
+mov r6, lr
NIT: I would add a newline to make it clearer.
+blcreate_page_tables
+
+/* Address in the runtime mapping to jump to after the MMU is enabled
*/
+mov_w lr, boot_cpu_mmu_on
+b enable_mmu
+boot_cpu_mmu_on:
Same as the lable secondary_cpu_mmu_on.
+blsetup_fixmap
+
+mov lr, r6
+
+/* Return to the virtual address requested by the caller. */
+mov pc, lr
In this case the 3 instructions can be replaced to:
mov lr, r6
b setup_fixmap
Re: [XEN PATCH 10/10] arm/smmu: address violation of MISRA C:2012 Rule 8.2
Hi Bertrand > On 16 Oct 2023, at 2:31 pm, Bertrand Marquis wrote: > > Hi Julien, > >> On 16 Oct 2023, at 11:07, Julien Grall wrote: >> >> Hi, >> >> On 13/10/2023 16:24, Federico Serafini wrote: >>> Add missing parameter names, no functional change. >>> Signed-off-by: Federico Serafini >>> --- >>> xen/drivers/passthrough/arm/smmu.c | 6 +++--- >> >> This file is using the Linux coding style because it is imported from Linux. >> I was under the impression we would exclude such file for now. >> >> Looking at exclude-list.json, it doesn't seem to be present. I think this >> patch should be replaced with adding a line in execlude-list.json. > > I think that during one of the discussions we said that this file already > deviated quite a lot from the status in Linux and we wanted to turn it to Xen > coding style in the future hence it is not listed in the exclude file. > At the end having a working smmu might be critical in a safety context so it > will make sense to also check this part of xen. > > @Rahul: do you agree ? Yes, you are right current SMMUv3 code already deviates quite a lot from the status of Linux because Xen handles the command queue in a different way than the way Linux handles it. More detailed can be found at the start of the SMMUv3 file. I am pasting it here also. * 5. Latest version of the Linux SMMUv3 code implements the commands queue * access functions based on atomic operations implemented in Linux. * Atomic functions used by the commands queue access functions are not * implemented in XEN therefore we decided to port the earlier version * of the code. Atomic operations are introduced to fix the bottleneck of * the SMMU command queue insertion operation. A new algorithm for * inserting commands into the queue is introduced, which is * lock-free on the fast-path. * Consequence of reverting the patch is that the command queue insertion * will be slow for large systems as spinlock will be used to serializes * accesses from all CPUs to the single queue supported by the hardware. * Once the proper atomic operations will be available in XEN the driver * can be updated. Anyway because of above reason backporting SMMUv3 Linux driver patches to Xen are not straight forward. If making smmu-v3.c to Xen coding style helps in safety context I am okay with that. Regards, Rahul
Re: MISRA C:2012 D4.11 caution on staging
Hi Nicola,
On 13/10/2023 16:11, Nicola Vetrini wrote:
On 13/10/2023 11:27, Julien Grall wrote:
Hi Andrew,
On 11/10/2023 08:51, Andrew Cooper wrote:
On 11/10/2023 3:47 pm, Nicola Vetrini wrote:
On 11/10/2023 02:15, Andrew Cooper wrote:
On 10/10/2023 5:31 pm, Nicola Vetrini wrote:
Hi,
as you can see from [1], there's a MISRA C guideline, D4.11, that is
supposed to be clean
(i.e., have no reports), but has a caution on an argument to memcpy
(the second argument might be null according to the checker, given a
set of assumptions on
the control flow). To access the report just click on the second link
in the log, which should take you to a webpage with a list of
MISRA guidelines. Click on D4.11 and you'll see the full report,
which
I pasted below for convenience.
If the finding is genuine, then some countermeasure needs to be taken
against this
possible bug, otherwise it needs to be motivated why the field
config->handle can't
be null at that point.
The finding is likely the result of an improvement made to the
checker, because the first
analysis I can see that spots it happened when rc1 has been tagged,
but that commit does not
touch the involved files.
[1] https://gitlab.com/xen-project/xen/-/jobs/5251222578
That's a false positive, but I'm not entirely surprised that the
checker
struggled to see it.
First,
ASSERT(is_system_domain(d) ? config == NULL : config != NULL);
All system domains (domid >= 0x7ff0, inc IDLE) pass a NULL config. All
other domains pass a real config.
Next,
/* DOMID_{XEN,IO,etc} (other than IDLE) are sufficiently
constructed. */
if ( is_system_domain(d) && !is_idle_domain(d) )
return d;
So at this point we only have the IDLE domain and real domains.
And finally, the complained-about construct is inside an:
if ( !is_idle_domain(d) )
...
hence config cannot be NULL, but only because of the way in which
is_{system,idle}_domain() interact.
~Andrew
Ok. I think this should be documented as a false positive using the
comment deviation mechanism
in the false-positive-eclair.json file (once a name for the prefix is
settled on in the
other thread).
Yeah - I was expecting that.
This code is mid-cleanup (in my copious free time, so not for several
releases now...) but even when it is rearranged sufficiently for IDLE to
have an earlier exit, I still wouldn't expect a static analyser to be
able to recognise this as being safe.
Looking through the analysis, I think Eclair properly detect the IDLE
domain. But it thinks the domain ID has changed mid function (see my
other reply to Stefano).
So we can return early for the IDLE domain, then this should silence
Eclair. That said, it is unclear to me why it thinks the domain_id can
change...
Well, the implementation of the directive has best-effort precision,
therefore false positives
like this one are possible. Since Andrew argued that the path is indeed
safe, I think
it's best to deviate this as such, since other minimal changes could
also make this one
resurface in the future.
If Eclair always reported the false positive, then I would have agree
that deviating would make sense. But you said this was due to an
improvement in the checker.
I expect improvement in the checker to reduce the number of false
positive, not introducing new ones. So I think we need to understand
what changed.
A simple way to fix it would be to have a local boolean that will be
used in place of is_idle_domain(d). This should help Eclair to detect
that a domain cannot change its ID in the middle of domain
construction.
Cheers,
I think only conditions are checked to get the possible code paths, to
have a reasonable
tradeoff between speed and analysis precision. Therefore, it's quite
possible that it would
give the same caution.
What I want to avoid is adding a SAF-* every time there are an update to
Eclair that reports more false positive. This will make the code more
difficult to read.
Instead, it would be best to find a way to help Eclair to detect this is
not an issue and also improve readability. Would the following help Eclair?
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 30c227967345..ab16124eabd6 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -671,6 +671,8 @@ struct domain *domain_create(domid_t domid,
if ( !is_idle_domain(d) )
{
+ASSERT(config);
+
watchdog_domain_init(d);
init_status |= INIT_watchdog;
Cheers,
--
Julien Grall
[xen-unstable-smoke test] 183385: tolerable all pass - PUSHED
flight 183385 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/183385/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen 618826f67306c32b2799e84ded9a8203f4109ccf baseline version: xen 6432228fb5808150d3c5c14affb3d46af81b3878 Last test of basis 183382 2023-10-16 10:03:41 Z0 days Testing same since 183385 2023-10-16 14:00:28 Z0 days1 attempts People who touched revisions under test: George Dunlap Julien Grall jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 6432228fb5..618826f673 618826f67306c32b2799e84ded9a8203f4109ccf -> smoke
Re: [XEN PATCH v2 1/7] xen: add declarations for variables where needed
On 16/10/2023 16:50, Jan Beulich wrote:
On 09.10.2023 08:54, Nicola Vetrini wrote:
Some variables with external linkage used in C code do not have
a visible declaration where they are defined. Providing such
declaration also resolves violations of MISRA C:2012 Rule 8.4.
Signed-off-by: Nicola Vetrini
This is a mix of different approaches to the same underlying issue. I
think respectively splitting would have helped.
--- a/xen/arch/x86/include/asm/compat.h
+++ b/xen/arch/x86/include/asm/compat.h
@@ -13,6 +13,7 @@ typedef unsigned long full_ptr_t;
struct domain;
#ifdef CONFIG_PV32
+extern unsigned long cr4_pv32_mask;
Why is this needed? Didn't we say declarations aren't needed when the
only consumer is assembly code? (I also wonder how this header is any
more "appropriate" - see the changelog entry - than about any other
one.)
It was pointed out to me [1] that compat.h might be more appropriate
than setup.h
(probably because the asm code referencing it is under x86_64/compat).
Further, while it's true that this variable is used in asm, it's also
used in x86/setup.c, hence
the need for a declaration.
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -13,6 +13,7 @@ extern char __2M_rwdata_start[], __2M_rwdata_end[];
extern unsigned long xenheap_initial_phys_start;
extern uint64_t boot_tsc_stamp;
+extern char cpu0_stack[STACK_SIZE];
Same question here.
This one is a bit more nuanced (I wouldn't oppose deviating this), but
there is indeed one use.
--- a/xen/common/symbols.c
+++ b/xen/common/symbols.c
@@ -21,23 +21,6 @@
#include
#include
-#ifdef SYMBOLS_ORIGIN
-extern const unsigned int symbols_offsets[];
-#define symbols_address(n) (SYMBOLS_ORIGIN + symbols_offsets[n])
-#else
-extern const unsigned long symbols_addresses[];
-#define symbols_address(n) symbols_addresses[n]
-#endif
-extern const unsigned int symbols_num_syms;
-extern const u8 symbols_names[];
-
-extern const struct symbol_offset symbols_sorted_offsets[];
-
-extern const u8 symbols_token_table[];
-extern const u16 symbols_token_index[];
-
-extern const unsigned int symbols_markers[];
-
/* expand a compressed symbol data into the resulting uncompressed
string,
given the offset to where the symbol is in the compressed stream
*/
static unsigned int symbols_expand_symbol(unsigned int off, char
*result)
--- a/xen/include/xen/symbols.h
+++ b/xen/include/xen/symbols.h
@@ -33,4 +33,22 @@ struct symbol_offset {
uint32_t stream; /* .. in the compressed stream.*/
uint32_t addr; /* .. and in the fixed size address array. */
};
+
+#ifdef SYMBOLS_ORIGIN
+extern const unsigned int symbols_offsets[];
+#define symbols_address(n) (SYMBOLS_ORIGIN + symbols_offsets[n])
+#else
+extern const unsigned long symbols_addresses[];
+#define symbols_address(n) symbols_addresses[n]
+#endif
+extern const unsigned int symbols_num_syms;
+extern const u8 symbols_names[];
+
+extern const struct symbol_offset symbols_sorted_offsets[];
+
+extern const u8 symbols_token_table[];
+extern const u16 symbols_token_index[];
+
+extern const unsigned int symbols_markers[];
+
#endif /*_XEN_SYMBOLS_H*/
This change is even less clear to me: The producer is assembly code,
and the consumer already had appropriate declarations. Why would we
want to increase the scope of their visibility?
Jan
The missing decls are about common/symbols-dummy.c. Xen can choose that
this file does
not need to conform (to this guideline or any guideline), in which case
this change can be dropped.
[1] https://lore.kernel.org/xen-devel/ZRqkbeVUZbjizjNv@MacBookPdeRoger/
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH][for-4.19 v2 1/1] xen: introduce a deviation for Rule 11.9
On 16/10/2023 15:43, Jan Beulich wrote: On 11.10.2023 14:46, Nicola Vetrini wrote: --- a/xen/include/xen/compiler.h +++ b/xen/include/xen/compiler.h @@ -109,13 +109,16 @@ #define offsetof(a,b) __builtin_offsetof(a,b) +/* Access the field of structure type, without defining a local variable */ +#define access_field(type, member) (((type *)NULL)->member) This is not a field access, so I consider the macro misnamed. Question is whether such a helper macro is needed in the first place. +#define typeof_field(type, member) typeof(access_field(type, member)) If this needs adding, it wants to come ... /** * sizeof_field(TYPE, MEMBER) * * @TYPE: The structure containing the field of interest * @MEMBER: The field to return the size of */ -#define sizeof_field(TYPE, MEMBER) sizeofTYPE *)0)->MEMBER)) +#define sizeof_field(TYPE, MEMBER) sizeof(access_field(TYPE, MEMBER)) ... with a commend similar as this one has. (Or the commend could be slightly altered to cover both). I added access_field since it's possibly useful on its own in the future, but that may not be the case. Not a real field access, perhaps a fake_access_field? Ok about the missing comment for typeof_field. Further, if fiddling with these: Wouldn't they better move to macros.h? Jan That seems a good suggestion, as they are not compiler-specific. -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [PATCH v7 8/8] xen/arm: mmu: move MMU specific P2M code to mmu/p2m.{c,h}
On 14/10/2023 02:26, Henry Wang wrote: Hi Julien, Hi Henry, On Oct 14, 2023, at 02:22, Julien Grall wrote: Hi Henry, On 09/10/2023 02:03, Henry Wang wrote: From: Penny Zheng Current P2M implementation is designed for MMU system only. We move the MMU-specific codes into mmu/p2m.c, and only keep generic codes in p2m.c, like VMID allocator, etc. We also move MMU-specific definitions and declarations to mmu/p2m.h, such as p2m_tlb_flush_sync(). Also expose previously static functions p2m_vmid_allocator_init(), p2m_alloc_vmid(), and setup_virt_paging_one() for further MPU usage. With the code movement, global variable max_vmid is used in multiple files instead of a single file (and will be used in MPU P2M implementation), declare it in the header and remove the "static" of this variable. Signed-off-by: Penny Zheng Signed-off-by: Wei Chen Signed-off-by: Henry Wang Some remarks about some of the code not moved: * struct p2m_domain: The bulk of the fields seems to be MMU specific. So depending on the number of common fields we either want to split or move the structure to p2m_domain. I would be ok to wait until the MPU code is present. * p2m_type_t: It is not yet clear how this will apply to the MPU. I am ok to wait before moving it. Agree with both here, let’s continue the discussion in the actual MPU patch for P2M then, but I am then a bit confused about... * p2m_cache_flush_range(): I expect the code will need some change because you may get large chunk of memory for the MPU. * p2m_set_way_flush()/p2m_toggle_cache(): This was a giant hack to support cache flush operations via set/way. To make it efficient, we track the pages that have been touched and only flush them. For the MPU, this would not work. Can we attempt to not emulate the instructions? …these two remarks here, do you expect me to do some changes with these three functions in this patch? Or we can defer the required changes to the MPU patch for P2M? My original intention was to ask to move them right now. But if it is unclear whether they would be used, then it would be best to defer until we have a better understanding. I think I am highly likely to make a mistake here but I took a look at the MPU implementation [1] and it looks like the MPU code can use these tree functions without changes - probably because these functions are simply used by (1) domctl and we only have dom0less DomUs on MPU (2) trap handlers which means these functions are simply not called… I am not sure I fully understand why would the trap handlers not called. Is this suggesting that a dom0less domUs can not use set/way instructions? Cheers, -- Julien Grall
Re: [XEN PATCH][for-next v2 5/8] x86/io_apic: address violation of MISRA C:2012 Rule 10.1
On 16/10/2023 17:42, Jan Beulich wrote: On 12.10.2023 17:28, Nicola Vetrini wrote: The definition of IO_APIC_BASE contains a sum of an essentially enum value (FIX_IO_APIC_BASE_0) that is positive with an index that, in all instances, is unsigned, therefore the former is cast to unsigned, so that the operands are of the same essential type. No functional change. --- xen/arch/x86/include/asm/io_apic.h | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) Oh, also - there's no S-o-b here. Jan Ah, good catch. -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH][for-next v2 5/8] x86/io_apic: address violation of MISRA C:2012 Rule 10.1
On 16/10/2023 17:42, Jan Beulich wrote: On 12.10.2023 17:28, Nicola Vetrini wrote: The definition of IO_APIC_BASE contains a sum of an essentially enum value (FIX_IO_APIC_BASE_0) that is positive with an index that, in all instances, is unsigned, therefore the former is cast to unsigned, so that the operands are of the same essential type. No functional change. --- xen/arch/x86/include/asm/io_apic.h | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/xen/arch/x86/include/asm/io_apic.h b/xen/arch/x86/include/asm/io_apic.h index a7e4c9e146de..a0fc50d601fe 100644 --- a/xen/arch/x86/include/asm/io_apic.h +++ b/xen/arch/x86/include/asm/io_apic.h @@ -14,9 +14,10 @@ * Copyright (C) 1997, 1998, 1999, 2000 Ingo Molnar */ -#define IO_APIC_BASE(idx) \ -((volatile uint32_t *)(__fix_to_virt(FIX_IO_APIC_BASE_0 + (idx)) \ - + (mp_ioapics[idx].mpc_apicaddr & ~PAGE_MASK))) +#define IO_APIC_BASE(idx) \ +((volatile uint32_t *)\ + (__fix_to_virt((unsigned int)FIX_IO_APIC_BASE_0 + (idx)) \ + + (mp_ioapics[idx].mpc_apicaddr & ~PAGE_MASK))) Let's assume that down the road we want to make __fix_to_virt() an inline function (which perhaps it really ought to be already): Won't this trade one violation for another then? I don't think so: the violation is in the argument to the macro (i.e., the fact that idx is unsigned and FIX_IO_APIC_BASE_0 is a constant from a named enum). Do you see a way in which a MISRA rule is violated if __fix_to_virt becomes a function with an unsigned int argument? I wonder whether the better course of action wouldn't be to switch the two enums to be "anonymous", even if that means adjusting __set_fixmap() and __set_fixmap_x(). I don't know. It certainly would help from a violation count perspective, though that's more of a code style decision in my opinion. As an aside, since you touch the entire construct, it would be nice if the + was also moved to the end of the earlier line. Sure -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
On 16.10.2023 18:17, Nicola Vetrini wrote:
> On 16/10/2023 17:33, Jan Beulich wrote:
>> On 12.10.2023 17:28, Nicola Vetrini wrote:
>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> @@ -274,6 +274,12 @@ still non-negative."
>>> -config=MC3R1.R10.1,etypes+={safe,
>>> "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))",
>>>
>>> "dst_type(ebool||boolean)"}
>>> -doc_end
>>>
>>> +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern to
>>> obtain the value
>>> +2^ffs(x) for unsigned integers on two's complement architectures
>>> +(all the architectures supported by Xen satisfy this requirement)."
>>> +-config=MC3R1.R10.1,reports+={safe,
>>> "any_area(any_loc(any_exp(macro(^LOWEST_BIT$"}
>>> +-doc_end
>>
>> Why is this added here rather than by ...
>>
>>> --- a/xen/include/xen/macros.h
>>> +++ b/xen/include/xen/macros.h
>>> @@ -8,8 +8,10 @@
>>> #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
>>> #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
>>>
>>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
>>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
>>
>> a SAF--safe comment here?
>>
>
> One reason is that now that violations only belonging to tool
> configurations
> and similar are documented in docs/misra/deviations.rst (committed in
> Stefano's
> branch for-4.19 [1]).
But tool configuration means every analysis tool needs configuring
separately. That's why the comment tagging scheme was decided to be
preferred, iirc.
> Also, there were disagreements on the SAF naming
> scheme, and
> patches like those would not be accepted at the moment.
Well, that needs resolving. The naming there shouldn't lead to patches
being accepted that later may need redoing.
Jan
[PATCH for-4.18] docs/sphinx: Lifecycle of a domid
Signed-off-by: Andrew Cooper --- CC: George Dunlap CC: Jan Beulich CC: Stefano Stabellini CC: Wei Liu CC: Julien Grall CC: Roger Pau Monné CC: Juergen Gross CC: Henry Wang Rendered form: https://andrewcoop-xen.readthedocs.io/en/docs-devel/hypervisor-guide/domid-lifecycle.html I'm not sure why it's using the alibaster theme and not RTD theme, but I don't have time to debug that further at this point. This was written mostly while sat waiting for flights in Nanjing and Beijing. If while reading this you spot a hole, congratulations. There are holes which need fixing... --- docs/glossary.rst | 9 ++ docs/hypervisor-guide/domid-lifecycle.rst | 164 ++ docs/hypervisor-guide/index.rst | 1 + 3 files changed, 174 insertions(+) create mode 100644 docs/hypervisor-guide/domid-lifecycle.rst diff --git a/docs/glossary.rst b/docs/glossary.rst index 8ddbdab160a1..1fd1de0f0e97 100644 --- a/docs/glossary.rst +++ b/docs/glossary.rst @@ -50,3 +50,12 @@ Glossary By default it gets all devices, including all disks and network cards, so is responsible for multiplexing guest I/O. + + system domain + Abstractions within Xen that are modelled in a similar way to regular + :term:`domains`. E.g. When there's no work to do, Xen schedules + ``DOMID_IDLE`` to put the CPU into a lower power state. + + System domains have :term:`domids` and are referenced by + privileged software for certain control operations, but they do not run + guest code. diff --git a/docs/hypervisor-guide/domid-lifecycle.rst b/docs/hypervisor-guide/domid-lifecycle.rst new file mode 100644 index ..d405a321f3c7 --- /dev/null +++ b/docs/hypervisor-guide/domid-lifecycle.rst @@ -0,0 +1,164 @@ +.. SPDX-License-Identifier: CC-BY-4.0 + +Lifecycle of a domid + + +Overview + + +A :term:`domid` is Xen's numeric identifier for a :term:`domain`. In any +operational Xen system, there are one or more domains running. + +Domids are 16-bit integers. Regular domids start from 0, but there are some +special identifiers, e.g. ``DOMID_SELF``, and :term:`system domains`, e.g. ``DOMID_IDLE`` starting from 0x7ff0. Therefore, a Xen system +can run a maximum of 32k domains concurrently. + +.. note:: + + Despite being exposed in the domid ABI, the system domains are internal to + Xen and do not have lifecycles like regular domains. Therefore, they are + not discussed further in this document. + +At system boot, Xen will construct one or more domains. Kernels and +configuration for these domains must be provided by the bootloader, or at +Xen's compile time for more highly integrated solutions. + +Correct functioning of the domain lifecycle involves ``xenstored``, and some +privileged entity which has bound the ``VIRQ_DOM_EXC`` global event channel. + +.. note:: + + While not a strict requirement for these to be the same entity, it is + ``xenstored`` which typically has ``VIRQ_DOM_EXC`` bound. This document is + written assuming the common case. + +Creation + + +Within Xen, the ``domain_create()`` function is used to allocate and perform +bare minimum construction of a domain. The :term:`control domain` accesses +this functionality via the ``DOMCTL_createdomain`` hypercall. + +The final action that ``domain_create()`` performs before returning +successfully is to enter the new domain into the domlist. This makes the +domain "visible" within Xen, allowing the new domid to be successfully +referenced by other hypercalls. + +At this point, the domain exists as far as Xen is concerned, but not usefully +as a VM yet. The toolstack performs further construction activities; +allocating vCPUs, RAM, copying in the initial executable code, etc. Domains +are automatically created with one "pause" reference count held, meaning that +it is not eligible for scheduling. + +When the toolstack has finished VM construction, it send an ``XS_INTRODUCE`` +command to ``xenstored``. This instructs ``xenstored`` to connect to the +guest's xenstore ring, and fire the ``@introduceDomain`` watch. The firing of +this watch is the signal to all other components which care that a new VM has +appeared and is about to start running. + +When the ``XS_INTRODUCE`` command returns successfully, the final action the +toolstack performs is to unpause the guest, using the ``DOMCTL_unpausedomain`` +hypercall. This drops the "pause" reference the domain was originally created +with, meaning that the vCPU(s) are eligible for scheduling and the domain will +start executing its first instruction. + +.. note:: + + It is common for vCPUs other than 0 to be left in an offline state, to be + started by actions within the VM. + +Termination +--- + +The VM runs for a period of time, but eventually stops. It can stop for a +number of reasons, including: + + * Directly at the guest kernel's request, via the ``SCHEDOP_shutdown`` +
Re: [PATCH v4 2/2] xen/arm: Enlarge identity map space to 10TB
On 16/10/2023 16:39, Bertrand Marquis wrote:
On 16 Oct 2023, at 17:31, Julien Grall wrote:
On 16/10/2023 16:07, Bertrand Marquis wrote:
Hi,
On 16 Oct 2023, at 16:46, Julien Grall wrote:
CC Henry
Hi Alexey,
On 16/10/2023 15:24, Alexey Klimov wrote:
On Mon, 16 Oct 2023 at 15:13, Luca Fancellu wrote:
On 16 Oct 2023, at 15:00, Bertrand Marquis wrote:
Hi
+Luca and Rahul
On 16 Oct 2023, at 15:54, Julien Grall wrote:
On 16/10/2023 09:44, Michal Orzel wrote:
Hi,
Hi,
On 13/10/2023 14:26, Leo Yan wrote:
On ADLink AVA platform (Ampere Altra SoC with 32 Arm Neoverse N1 cores),
the physical memory regions are:
DRAM memory regions:
Node[0] Region[0]: 0x8000 - 0x
Node[0] Region[1]: 0x0800 - 0x08007fff
Node[0] Region[2]: 0x0801 - 0x0807
The UEFI loads Xen hypervisor and DTB into the high memory, the kernel
and ramdisk images are loaded into the low memory space:
(XEN) MODULE[0]: 0807f6df - 0807f6f3e000 Xen
(XEN) MODULE[1]: 0807f8054000 - 0807f8056000 Device Tree
(XEN) MODULE[2]: fa834000 - fc5de1d5 Ramdisk
(XEN) MODULE[3]: fc5df000 - ffb3f810 Kernel
In this case, the Xen binary is loaded above 8TB, which exceeds the
maximum supported identity map space of 2TB in Xen. Consequently, the
system fails to boot.
This patch enlarges identity map space to 10TB, allowing module loading
within the range of [0x0 .. 0x09ff__].
Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to prepare/enable/disable")
I don't think a fixes tag applies here given that 2TB was just a number we
believed is enough
and all of this is platform dependent.
This can be dropped on commit if committer agrees
Xen may have booted on that platform before hand. So this would be considered a
regression and therefore a tag would be warrant.
AFAICT, the commit is only present on the upcoming 4.18. So the question is
whether Xen 4.17 booted out-of-the-box on ADLink? If the answer is yes, then we
need to add a Fixes tag. But the correct one would be
@Rahul or Luca: could you give an answer here ?
I know you used Xen on an AVA platform but was it booting out of the box ?
I can’t say for Xen 4.17, but our nightly job has run successfully on AVA for
the commit 730406ab81094115d9fb5ca00ba8d53cec1279b3
(docs/misra: add deviations.rst to document additional deviations.)
We are not applying any patch for it to run on AVA.
Most likely it is because your UEFI/BIOS firmware is 2.x, for instance
2.04.100.07.
This fix if for AVA machine with older UEFI firmware 1.07.300.03.
OOI, why not updating your firmware? I was expecting that it would also contain
other critical fixes.
With this in mind, I am now more in two mind to ask to merge this patch in Xen
4.18. On one hand, I understand it will help to boot on AVA machine with an
older firmware. But on the other hand this is changing the memory layout quite
late in the release. The risk seems limited because Xen is not loaded at the
top of the virtual address space (there is the directmap afterwards).
Henry (as the release manager) and others, any opinions?
With the new information, I think it should be stated that it is fixing an
issue on AVA platforms using an old firmware and platforms with and up-to-date
firmware are not impacted.
It is an important information to keep in mind that this is not a fix to be
backported for example (and for me the fixes tag should not be kept).
IMHO, the fixes tag should not be based solely on the AVA platform. It should
be based on whether this could sensibly affect others. Right now, there is
nothing that would indicate either way.
And therefore a Fixes tag is sensible. This doesn't mean I would want to
backport it right now (note that only 4.18 is affected). But this could change
in the future if we get another report (post-4.18) on a platform where there
are no other workaround.
Stefano any opinions?
On whether or not it should go in the release, I am not quite sure that making
a change in the layout at that stage is a good idea unless it is fixing a
critical issue (which is not the case here).
So i would vote no but not go against if someone argue to have it in.
I agree with your statement with the information we have today. But it could
become a critical issue if another platform is hit by the same issue and have
no other workaround.
I am more seeing this as adding support for platforms with a topology that was
not supported until now (because it was not encountered) rather than fixing a
bug which is why i find it a bit odd to say that it is fixing some issue. But
definitely this is an opinion and nothing that i could argue on :-)
This is a valid point. The problem is SUPPORT.md doesn't say anything
about new HW. So in theory, if it works out-of-the-box on Xen 4.17, then
one could expect that it should work on 4.18.
I can see why the owner of the platform
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
On 16/10/2023 17:33, Jan Beulich wrote:
On 12.10.2023 17:28, Nicola Vetrini wrote:
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -274,6 +274,12 @@ still non-negative."
-config=MC3R1.R10.1,etypes+={safe,
"stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))",
"dst_type(ebool||boolean)"}
-doc_end
+-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern to
obtain the value
+2^ffs(x) for unsigned integers on two's complement architectures
+(all the architectures supported by Xen satisfy this requirement)."
+-config=MC3R1.R10.1,reports+={safe,
"any_area(any_loc(any_exp(macro(^LOWEST_BIT$"}
+-doc_end
Why is this added here rather than by ...
--- a/xen/include/xen/macros.h
+++ b/xen/include/xen/macros.h
@@ -8,8 +8,10 @@
#define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
-#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
-#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
a SAF--safe comment here?
One reason is that now that violations only belonging to tool
configurations
and similar are documented in docs/misra/deviations.rst (committed in
Stefano's
branch for-4.19 [1]). Also, there were disagreements on the SAF naming
scheme, and
patches like those would not be accepted at the moment.
+#define LOWEST_BIT(x) ((x) & -(x))
Personally I consider the name misleading: I'd expect a macro of this
name to return a bit number, not a mask with a single bit set. No
good, reasonably short name comes to mind - ISOLATE_LOW_BIT() is too
long for my taste.
Jan
[1]
https://gitlab.com/xen-project/people/sstabellini/xen/-/commits/for-4.19
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
[linux-linus test] 183380: tolerable FAIL - PUSHED
flight 183380 linux-linus real [real] flight 183386 linux-linus real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/183380/ http://logs.test-lab.xenproject.org/osstest/logs/183386/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-amd64-xl-credit1 22 guest-start/debian.repeat fail pass in 183386-retest Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 183378 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 183378 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 183378 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 183378 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 183378 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 183378 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 183378 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 183378 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-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-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-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-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-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-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-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-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass version targeted for testing: linux58720809f52779dc0f08e53e54b014209d13eebb baseline version: linuxfbe1bf1e5ff1e3b298420d7a8434983ef8d72bd1 Last test of basis 183378 2023-10-15 19:43:01 Z0 days Testing same since 183380 2023-10-16 02:44:29 Z0 days1 attempts People who touched revisions under test: Linus Torvalds jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf pass build-i386
Re: [XEN PATCH][for-next v2 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class
On 16/10/2023 17:45, Jan Beulich wrote: On 12.10.2023 17:28, Nicola Vetrini wrote: The definition of MC_NCLASSES contained a violation of MISRA C:2012 Rule 10.1, therefore by moving it as an enumeration constant resolves the violation and makes it more resilient to possible additions to that enum. And using an enumerator as array dimension specifier is okay for Misra? That would be odd when elsewhere named enums are treated specially. Jan Yes, the array subscript operator is one of the few places where an enum can be used as an operand (also because negative values wouldn't compile), as opposed to mixing them with ordinary integers. -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH][for-4.19 v2 7/8] xen/types: address Rule 10.1 for DECLARE_BITMAP use
On 12.10.2023 17:28, Nicola Vetrini wrote: > --- a/xen/include/xen/types.h > +++ b/xen/include/xen/types.h > @@ -22,6 +22,14 @@ typedef signed long ssize_t; > > typedef __PTRDIFF_TYPE__ ptrdiff_t; > > +/* > + * Users of this macro are expected to pass a positive value. Is passing 0 going to cause any issues? > + * Eventually, this should become an unsigned quantity, but this > + * requires fixing various uses of this macro and BITS_PER_LONG in signed > + * contexts, such as type-safe 'min' macro uses, which give rise to build > errors > + * when the arguments have differing signedness, due to the build flags used. > + */ I'm not convinced of the usefulness of this part of the comment. Jan > #define BITS_TO_LONGS(bits) \ > (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG) > #define DECLARE_BITMAP(name,bits) \
Re: [XEN PATCH][for-next v2 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class
On 12.10.2023 17:28, Nicola Vetrini wrote:
> The definition of MC_NCLASSES contained a violation of MISRA C:2012
> Rule 10.1, therefore by moving it as an enumeration constant resolves the
> violation and makes it more resilient to possible additions to that enum.
And using an enumerator as array dimension specifier is okay for Misra?
That would be odd when elsewhere named enums are treated specially.
Jan
> --- a/xen/arch/x86/cpu/mcheck/mctelem.c
> +++ b/xen/arch/x86/cpu/mcheck/mctelem.c
> @@ -64,8 +64,6 @@ struct mctelem_ent {
>
> #define MC_NENT (MC_URGENT_NENT + MC_NONURGENT_NENT)
>
> -#define MC_NCLASSES (MC_NONURGENT + 1)
> -
> #define COOKIE2MCTE(c) ((struct mctelem_ent *)(c))
> #define MCTE2COOKIE(tep)((mctelem_cookie_t)(tep))
>
> diff --git a/xen/arch/x86/cpu/mcheck/mctelem.h
> b/xen/arch/x86/cpu/mcheck/mctelem.h
> index d4eba53ae0e5..21b251847bc0 100644
> --- a/xen/arch/x86/cpu/mcheck/mctelem.h
> +++ b/xen/arch/x86/cpu/mcheck/mctelem.h
> @@ -55,8 +55,9 @@
> typedef struct mctelem_cookie *mctelem_cookie_t;
>
> typedef enum mctelem_class {
> - MC_URGENT,
> - MC_NONURGENT
> +MC_URGENT,
> +MC_NONURGENT,
> +MC_NCLASSES
> } mctelem_class_t;
>
> extern void mctelem_init(unsigned int);
Re: [XEN PATCH][for-next v2 5/8] x86/io_apic: address violation of MISRA C:2012 Rule 10.1
On 12.10.2023 17:28, Nicola Vetrini wrote: > The definition of IO_APIC_BASE contains a sum of an essentially enum > value (FIX_IO_APIC_BASE_0) that is positive with an index that, in all > instances, is unsigned, therefore the former is cast to unsigned, so that > the operands are of the same essential type. > > No functional change. > --- > xen/arch/x86/include/asm/io_apic.h | 7 --- > 1 file changed, 4 insertions(+), 3 deletions(-) Oh, also - there's no S-o-b here. Jan
Re: [XEN PATCH][for-next v2 5/8] x86/io_apic: address violation of MISRA C:2012 Rule 10.1
On 12.10.2023 17:28, Nicola Vetrini wrote: > The definition of IO_APIC_BASE contains a sum of an essentially enum > value (FIX_IO_APIC_BASE_0) that is positive with an index that, in all > instances, is unsigned, therefore the former is cast to unsigned, so that > the operands are of the same essential type. > > No functional change. > --- > xen/arch/x86/include/asm/io_apic.h | 7 --- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/include/asm/io_apic.h > b/xen/arch/x86/include/asm/io_apic.h > index a7e4c9e146de..a0fc50d601fe 100644 > --- a/xen/arch/x86/include/asm/io_apic.h > +++ b/xen/arch/x86/include/asm/io_apic.h > @@ -14,9 +14,10 @@ > * Copyright (C) 1997, 1998, 1999, 2000 Ingo Molnar > */ > > -#define IO_APIC_BASE(idx) \ > -((volatile uint32_t *)(__fix_to_virt(FIX_IO_APIC_BASE_0 + (idx))\ > - + (mp_ioapics[idx].mpc_apicaddr & ~PAGE_MASK))) > +#define IO_APIC_BASE(idx) \ > +((volatile uint32_t *)\ > + (__fix_to_virt((unsigned int)FIX_IO_APIC_BASE_0 + (idx)) \ > + + (mp_ioapics[idx].mpc_apicaddr & ~PAGE_MASK))) Let's assume that down the road we want to make __fix_to_virt() an inline function (which perhaps it really ought to be already): Won't this trade one violation for another then? I wonder whether the better course of action wouldn't be to switch the two enums to be "anonymous", even if that means adjusting __set_fixmap() and __set_fixmap_x(). As an aside, since you touch the entire construct, it would be nice if the + was also moved to the end of the earlier line. Jan
Re: [PATCH v4 2/2] xen/arm: Enlarge identity map space to 10TB
> On 16 Oct 2023, at 17:31, Julien Grall wrote:
>
>
>
> On 16/10/2023 16:07, Bertrand Marquis wrote:
>> Hi,
>>> On 16 Oct 2023, at 16:46, Julien Grall wrote:
>>>
>>> CC Henry
>>>
>>> Hi Alexey,
>>>
>>> On 16/10/2023 15:24, Alexey Klimov wrote:
On Mon, 16 Oct 2023 at 15:13, Luca Fancellu wrote:
>
>
>
>> On 16 Oct 2023, at 15:00, Bertrand Marquis
>> wrote:
>>
>> Hi
>>
>> +Luca and Rahul
>>
>>> On 16 Oct 2023, at 15:54, Julien Grall wrote:
>>>
>>>
>>>
>>> On 16/10/2023 09:44, Michal Orzel wrote:
Hi,
>>>
>>> Hi,
>>>
On 13/10/2023 14:26, Leo Yan wrote:
>
>
> On ADLink AVA platform (Ampere Altra SoC with 32 Arm Neoverse N1
> cores),
> the physical memory regions are:
>
> DRAM memory regions:
>Node[0] Region[0]: 0x8000 - 0x
>Node[0] Region[1]: 0x0800 - 0x08007fff
>Node[0] Region[2]: 0x0801 - 0x0807
>
> The UEFI loads Xen hypervisor and DTB into the high memory, the kernel
> and ramdisk images are loaded into the low memory space:
>
> (XEN) MODULE[0]: 0807f6df - 0807f6f3e000 Xen
> (XEN) MODULE[1]: 0807f8054000 - 0807f8056000 Device Tree
> (XEN) MODULE[2]: fa834000 - fc5de1d5 Ramdisk
> (XEN) MODULE[3]: fc5df000 - ffb3f810 Kernel
>
> In this case, the Xen binary is loaded above 8TB, which exceeds the
> maximum supported identity map space of 2TB in Xen. Consequently, the
> system fails to boot.
>
> This patch enlarges identity map space to 10TB, allowing module
> loading
> within the range of [0x0 .. 0x09ff__].
>
> Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to
> prepare/enable/disable")
I don't think a fixes tag applies here given that 2TB was just a
number we believed is enough
and all of this is platform dependent.
This can be dropped on commit if committer agrees
>>> Xen may have booted on that platform before hand. So this would be
>>> considered a regression and therefore a tag would be warrant.
>>>
>>> AFAICT, the commit is only present on the upcoming 4.18. So the
>>> question is whether Xen 4.17 booted out-of-the-box on ADLink? If the
>>> answer is yes, then we need to add a Fixes tag. But the correct one
>>> would be
>>>
>>
>> @Rahul or Luca: could you give an answer here ?
>> I know you used Xen on an AVA platform but was it booting out of the box
>> ?
>
> I can’t say for Xen 4.17, but our nightly job has run successfully on AVA
> for the commit 730406ab81094115d9fb5ca00ba8d53cec1279b3
> (docs/misra: add deviations.rst to document additional deviations.)
>
> We are not applying any patch for it to run on AVA.
Most likely it is because your UEFI/BIOS firmware is 2.x, for instance
2.04.100.07.
This fix if for AVA machine with older UEFI firmware 1.07.300.03.
>>>
>>> OOI, why not updating your firmware? I was expecting that it would also
>>> contain other critical fixes.
>>>
>>> With this in mind, I am now more in two mind to ask to merge this patch in
>>> Xen 4.18. On one hand, I understand it will help to boot on AVA machine
>>> with an older firmware. But on the other hand this is changing the memory
>>> layout quite late in the release. The risk seems limited because Xen is not
>>> loaded at the top of the virtual address space (there is the directmap
>>> afterwards).
>>>
>>> Henry (as the release manager) and others, any opinions?
>> With the new information, I think it should be stated that it is fixing an
>> issue on AVA platforms using an old firmware and platforms with and
>> up-to-date firmware are not impacted.
>> It is an important information to keep in mind that this is not a fix to be
>> backported for example (and for me the fixes tag should not be kept).
>
> IMHO, the fixes tag should not be based solely on the AVA platform. It should
> be based on whether this could sensibly affect others. Right now, there is
> nothing that would indicate either way.
>
> And therefore a Fixes tag is sensible. This doesn't mean I would want to
> backport it right now (note that only 4.18 is affected). But this could
> change in the future if we get another report (post-4.18) on a platform where
> there are no other workaround.
>
> Stefano any opinions?
>
>> On whether or not it should go in the release, I am not quite sure that
>> making a change in the layout at that stage is a good idea unless it is
>> fixing a critical issue (which is not the case here).
>> So i would vote no but not go against if someone argue to have it in.
>
> I agree with your statement
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
On 12.10.2023 17:28, Nicola Vetrini wrote:
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -274,6 +274,12 @@ still non-negative."
> -config=MC3R1.R10.1,etypes+={safe,
> "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))",
> "dst_type(ebool||boolean)"}
> -doc_end
>
> +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern to obtain
> the value
> +2^ffs(x) for unsigned integers on two's complement architectures
> +(all the architectures supported by Xen satisfy this requirement)."
> +-config=MC3R1.R10.1,reports+={safe,
> "any_area(any_loc(any_exp(macro(^LOWEST_BIT$"}
> +-doc_end
Why is this added here rather than by ...
> --- a/xen/include/xen/macros.h
> +++ b/xen/include/xen/macros.h
> @@ -8,8 +8,10 @@
> #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
> #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
>
> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
a SAF--safe comment here?
> +#define LOWEST_BIT(x) ((x) & -(x))
Personally I consider the name misleading: I'd expect a macro of this
name to return a bit number, not a mask with a single bit set. No
good, reasonably short name comes to mind - ISOLATE_LOW_BIT() is too
long for my taste.
Jan
Re: [PATCH v4 2/2] xen/arm: Enlarge identity map space to 10TB
On 16/10/2023 16:07, Bertrand Marquis wrote:
Hi,
On 16 Oct 2023, at 16:46, Julien Grall wrote:
CC Henry
Hi Alexey,
On 16/10/2023 15:24, Alexey Klimov wrote:
On Mon, 16 Oct 2023 at 15:13, Luca Fancellu wrote:
On 16 Oct 2023, at 15:00, Bertrand Marquis wrote:
Hi
+Luca and Rahul
On 16 Oct 2023, at 15:54, Julien Grall wrote:
On 16/10/2023 09:44, Michal Orzel wrote:
Hi,
Hi,
On 13/10/2023 14:26, Leo Yan wrote:
On ADLink AVA platform (Ampere Altra SoC with 32 Arm Neoverse N1 cores),
the physical memory regions are:
DRAM memory regions:
Node[0] Region[0]: 0x8000 - 0x
Node[0] Region[1]: 0x0800 - 0x08007fff
Node[0] Region[2]: 0x0801 - 0x0807
The UEFI loads Xen hypervisor and DTB into the high memory, the kernel
and ramdisk images are loaded into the low memory space:
(XEN) MODULE[0]: 0807f6df - 0807f6f3e000 Xen
(XEN) MODULE[1]: 0807f8054000 - 0807f8056000 Device Tree
(XEN) MODULE[2]: fa834000 - fc5de1d5 Ramdisk
(XEN) MODULE[3]: fc5df000 - ffb3f810 Kernel
In this case, the Xen binary is loaded above 8TB, which exceeds the
maximum supported identity map space of 2TB in Xen. Consequently, the
system fails to boot.
This patch enlarges identity map space to 10TB, allowing module loading
within the range of [0x0 .. 0x09ff__].
Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to prepare/enable/disable")
I don't think a fixes tag applies here given that 2TB was just a number we
believed is enough
and all of this is platform dependent.
This can be dropped on commit if committer agrees
Xen may have booted on that platform before hand. So this would be considered a
regression and therefore a tag would be warrant.
AFAICT, the commit is only present on the upcoming 4.18. So the question is
whether Xen 4.17 booted out-of-the-box on ADLink? If the answer is yes, then we
need to add a Fixes tag. But the correct one would be
@Rahul or Luca: could you give an answer here ?
I know you used Xen on an AVA platform but was it booting out of the box ?
I can’t say for Xen 4.17, but our nightly job has run successfully on AVA for
the commit 730406ab81094115d9fb5ca00ba8d53cec1279b3
(docs/misra: add deviations.rst to document additional deviations.)
We are not applying any patch for it to run on AVA.
Most likely it is because your UEFI/BIOS firmware is 2.x, for instance
2.04.100.07.
This fix if for AVA machine with older UEFI firmware 1.07.300.03.
OOI, why not updating your firmware? I was expecting that it would also contain
other critical fixes.
With this in mind, I am now more in two mind to ask to merge this patch in Xen
4.18. On one hand, I understand it will help to boot on AVA machine with an
older firmware. But on the other hand this is changing the memory layout quite
late in the release. The risk seems limited because Xen is not loaded at the
top of the virtual address space (there is the directmap afterwards).
Henry (as the release manager) and others, any opinions?
With the new information, I think it should be stated that it is fixing an
issue on AVA platforms using an old firmware and platforms with and up-to-date
firmware are not impacted.
It is an important information to keep in mind that this is not a fix to be
backported for example (and for me the fixes tag should not be kept).
IMHO, the fixes tag should not be based solely on the AVA platform. It
should be based on whether this could sensibly affect others. Right now,
there is nothing that would indicate either way.
And therefore a Fixes tag is sensible. This doesn't mean I would want to
backport it right now (note that only 4.18 is affected). But this could
change in the future if we get another report (post-4.18) on a platform
where there are no other workaround.
Stefano any opinions?
On whether or not it should go in the release, I am not quite sure that making
a change in the layout at that stage is a good idea unless it is fixing a
critical issue (which is not the case here).
So i would vote no but not go against if someone argue to have it in.
I agree with your statement with the information we have today. But it
could become a critical issue if another platform is hit by the same
issue and have no other workaround.
Cheers,
--
Julien Grall
Re: [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3
On 03.10.2023 17:24, Federico Serafini wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5901,17 +5901,17 @@ int destroy_xen_mappings(unsigned long s, unsigned
> long e)
> * a problem.
> */
> void init_or_livepatch modify_xen_mappings_lite(
> -unsigned long s, unsigned long e, unsigned int _nf)
> +unsigned long s, unsigned long e, unsigned int nf)
> {
> -unsigned long v = s, fm, nf;
> +unsigned long v = s, fm, flags;
While it looks correct, I consider this an unacceptably dangerous
change: What if by the time this is to be committed some new use of
the local "nf" appears, without resulting in fuzz while applying the
patch? Imo this needs doing in two steps: First nf -> flags, then
_nf -> nf.
Furthermore since you alter the local variable, is there any reason
not to also change it to be "unsigned int", matching the function
argument it's set from?
Yet then - can't we just delete "nf" and rename "_nf" to "nf"? The
function parameter is only used in
nf = put_pte_flags(_nf & FLAGS_MASK);
Jan
[PATCH 04/12] i386/xen: advertise XEN_HVM_CPUID_UPCALL_VECTOR in CPUID
From: David Woodhouse
This will allow Linux guests (since v6.0) to use the per-vCPU upcall
vector delivered as MSI through the local APIC.
Signed-off-by: David Woodhouse
---
target/i386/kvm/kvm.c | 4
1 file changed, 4 insertions(+)
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index f6c7f7e268..8bdbdcdffe 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1887,6 +1887,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
c->eax |= XEN_HVM_CPUID_VCPU_ID_PRESENT;
c->ebx = cs->cpu_index;
}
+
+if (cs->kvm_state->xen_version >= XEN_VERSION(4, 17)) {
+c->eax |= XEN_HVM_CPUID_UPCALL_VECTOR;
+}
}
r = kvm_xen_init_vcpu(cs);
--
2.40.1
[PATCH 06/12] hw/xen: add get_frontend_path() method to XenDeviceClass
From: David Woodhouse
The primary Xen console is special. The guest's side is set up for it by
the toolstack automatically and not by the standard PV init sequence.
Accordingly, its *frontend* doesn't appear in …/device/console/0 either;
instead it appears under …/console in the guest's XenStore node.
To allow the Xen console driver to override the frontend path for the
primary console, add a method to the XenDeviceClass which can be used
instead of the standard xen_device_get_frontend_path()
Signed-off-by: David Woodhouse
---
hw/xen/xen-bus.c | 10 +-
include/hw/xen/xen-bus.h | 2 ++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index ece8ec40cd..cc524ed92c 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -711,8 +711,16 @@ static void xen_device_frontend_create(XenDevice *xendev,
Error **errp)
{
ERRP_GUARD();
XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
+XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
-xendev->frontend_path = xen_device_get_frontend_path(xendev);
+if (xendev_class->get_frontend_path) {
+xendev->frontend_path = xendev_class->get_frontend_path(xendev, errp);
+if (!xendev->frontend_path) {
+return;
+}
+} else {
+xendev->frontend_path = xen_device_get_frontend_path(xendev);
+}
/*
* The frontend area may have already been created by a legacy
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index f435898164..eb440880b5 100644
--- a/include/hw/xen/xen-bus.h
+++ b/include/hw/xen/xen-bus.h
@@ -33,6 +33,7 @@ struct XenDevice {
};
typedef struct XenDevice XenDevice;
+typedef char *(*XenDeviceGetFrontendPath)(XenDevice *xendev, Error **errp);
typedef char *(*XenDeviceGetName)(XenDevice *xendev, Error **errp);
typedef void (*XenDeviceRealize)(XenDevice *xendev, Error **errp);
typedef void (*XenDeviceFrontendChanged)(XenDevice *xendev,
@@ -46,6 +47,7 @@ struct XenDeviceClass {
/*< public >*/
const char *backend;
const char *device;
+XenDeviceGetFrontendPath get_frontend_path;
XenDeviceGetName get_name;
XenDeviceRealize realize;
XenDeviceFrontendChanged frontend_changed;
--
2.40.1
[PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode
From: David Woodhouse
The primary console is special because the toolstack maps a page at a
fixed GFN and also allocates the guest-side event channel. Add support
for that in emulated mode, so that we can have a primary console.
Add a *very* rudimentary stub of foriegnmem ops for emulated mode, which
supports literally nothing except a single-page mapping of the console
page. This might as well have been a hack in the xen_console driver, but
this way at least the special-casing is kept within the Xen emulation
code, and it gives us a hook for a more complete implementation if/when
we ever do need one.
Now at last we can boot the Xen PV shim and run PV kernels in QEMU.
Signed-off-by: David Woodhouse
---
hw/char/xen_console.c | 12 ++-
hw/i386/kvm/meson.build | 1 +
hw/i386/kvm/trace-events | 2 +
hw/i386/kvm/xen-stubs.c | 5 +
hw/i386/kvm/xen_gnttab.c | 32 +-
hw/i386/kvm/xen_primary_console.c | 167 ++
hw/i386/kvm/xen_primary_console.h | 22
hw/i386/kvm/xen_xenstore.c| 9 ++
target/i386/kvm/xen-emu.c | 23 +++-
9 files changed, 266 insertions(+), 7 deletions(-)
create mode 100644 hw/i386/kvm/xen_primary_console.c
create mode 100644 hw/i386/kvm/xen_primary_console.h
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index 1a0f5ed3e1..dfc4be0aa1 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -32,6 +32,7 @@
#include "hw/qdev-properties.h"
#include "hw/qdev-properties-system.h"
#include "hw/xen/interface/io/console.h"
+#include "hw/i386/kvm/xen_primary_console.h"
#include "trace.h"
struct buffer {
@@ -334,8 +335,8 @@ static char *xen_console_get_name(XenDevice *xendev, Error
**errp)
XenConsole *con = XEN_CONSOLE_DEVICE(xendev);
if (con->dev == -1) {
+int idx = (xen_mode == XEN_EMULATE) ? 0 : 1;
char name[11];
-int idx = 1;
/* Theoretically we could go up to INT_MAX here but that's overkill */
while (idx < 100) {
@@ -386,10 +387,13 @@ static void xen_console_realize(XenDevice *xendev, Error
**errp)
* be mapped directly as foreignmem (not a grant ref), and the guest port
* was allocated *for* the guest by the toolstack. The guest gets these
* through HVMOP_get_param and can use the console long before it's got
- * XenStore up and running. We cannot create those for a Xen guest.
+ * XenStore up and running. We cannot create those for a true Xen guest,
+ * but we can for Xen emulation.
*/
if (!con->dev) {
-if (xen_device_frontend_scanf(xendev, "ring-ref", "%u", ) != 1 ||
+if (xen_mode == XEN_EMULATE) {
+xen_primary_console_create();
+} else if (xen_device_frontend_scanf(xendev, "ring-ref", "%u", ) !=
1 ||
xen_device_frontend_scanf(xendev, "port", "%u", ) != 1) {
error_setg(errp, "cannot create primary Xen console");
return;
@@ -404,7 +408,7 @@ static void xen_console_realize(XenDevice *xendev, Error
**errp)
}
/* No normal PV driver initialization for the primary console */
-if (!con->dev) {
+if (!con->dev && xen_mode != XEN_EMULATE) {
xen_console_connect(xendev, errp);
}
}
diff --git a/hw/i386/kvm/meson.build b/hw/i386/kvm/meson.build
index ab143d6474..a4a2e23c06 100644
--- a/hw/i386/kvm/meson.build
+++ b/hw/i386/kvm/meson.build
@@ -9,6 +9,7 @@ i386_kvm_ss.add(when: 'CONFIG_XEN_EMU', if_true: files(
'xen_evtchn.c',
'xen_gnttab.c',
'xen_xenstore.c',
+ 'xen_primary_console.c',
'xenstore_impl.c',
))
diff --git a/hw/i386/kvm/trace-events b/hw/i386/kvm/trace-events
index e4c82de6f3..67bf7f174e 100644
--- a/hw/i386/kvm/trace-events
+++ b/hw/i386/kvm/trace-events
@@ -18,3 +18,5 @@ xenstore_watch(const char *path, const char *token) "path %s
token %s"
xenstore_unwatch(const char *path, const char *token) "path %s token %s"
xenstore_reset_watches(void) ""
xenstore_watch_event(const char *path, const char *token) "path %s token %s"
+xen_primary_console_create(void) ""
+xen_primary_console_reset(int port) "port %u"
diff --git a/hw/i386/kvm/xen-stubs.c b/hw/i386/kvm/xen-stubs.c
index ae406e0b02..10068970fe 100644
--- a/hw/i386/kvm/xen-stubs.c
+++ b/hw/i386/kvm/xen-stubs.c
@@ -15,6 +15,7 @@
#include "qapi/qapi-commands-misc-target.h"
#include "xen_evtchn.h"
+#include "xen_primary_console.h"
void xen_evtchn_snoop_msi(PCIDevice *dev, bool is_msix, unsigned int vector,
uint64_t addr, uint32_t data, bool is_masked)
@@ -30,6 +31,10 @@ bool xen_evtchn_deliver_pirq_msi(uint64_t address, uint32_t
data)
return false;
}
+void xen_primary_console_create(void)
+{
+}
+
#ifdef TARGET_I386
EvtchnInfoList *qmp_xen_event_list(Error **errp)
{
diff --git a/hw/i386/kvm/xen_gnttab.c b/hw/i386/kvm/xen_gnttab.c
index 21c30e3659..ea201cd582 100644
--- a/hw/i386/kvm/xen_gnttab.c
+++ b/hw/i386/kvm/xen_gnttab.c
@@ -25,6
[PATCH 09/12] hw/xen: prevent duplicate device registrations
From: David Woodhouse
Ensure that we have a XenBackendInstance for every device regardless
of whether it was "discovered" in XenStore or created directly in QEMU.
This allows the backend_list to be a source of truth about whether a
given backend exists, and allows us to reject duplicates.
This also cleans up the fact that backend drivers were calling
xen_backend_set_device() with a XenDevice immediately after calling
qdev_realize_and_unref() on it, when it wasn't theirs to play with any
more.
Signed-off-by: David Woodhouse
---
hw/block/xen-block.c | 1 -
hw/char/xen_console.c| 2 +-
hw/xen/xen-backend.c | 78 ++--
hw/xen/xen-bus.c | 8
include/hw/xen/xen-backend.h | 3 ++
5 files changed, 69 insertions(+), 23 deletions(-)
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index a07cd7eb5d..9262338535 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -975,7 +975,6 @@ static void xen_block_device_create(XenBackendInstance
*backend,
goto fail;
}
-xen_backend_set_device(backend, xendev);
return;
fail:
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index bd20be116c..2825b8c511 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -468,7 +468,7 @@ static void xen_console_device_create(XenBackendInstance
*backend,
Chardev *cd = NULL;
struct qemu_xs_handle *xsh = xenbus->xsh;
-if (qemu_strtoul(name, NULL, 10, )) {
+if (qemu_strtoul(name, NULL, 10, ) || number >= INT_MAX) {
error_setg(errp, "failed to parse name '%s'", name);
goto fail;
}
diff --git a/hw/xen/xen-backend.c b/hw/xen/xen-backend.c
index b9bf70a9f5..dcb4329258 100644
--- a/hw/xen/xen-backend.c
+++ b/hw/xen/xen-backend.c
@@ -101,22 +101,28 @@ static XenBackendInstance
*xen_backend_list_find(XenDevice *xendev)
return NULL;
}
-bool xen_backend_exists(const char *type, const char *name)
+static XenBackendInstance *xen_backend_lookup(const XenBackendImpl *impl,
const char *name)
{
-const XenBackendImpl *impl = xen_backend_table_lookup(type);
XenBackendInstance *backend;
-if (!impl) {
-return false;
-}
-
QLIST_FOREACH(backend, _list, entry) {
if (backend->impl == impl && !strcmp(backend->name, name)) {
-return true;
+return backend;
}
}
-return false;
+return NULL;
+}
+
+bool xen_backend_exists(const char *type, const char *name)
+{
+const XenBackendImpl *impl = xen_backend_table_lookup(type);
+
+if (!impl) {
+return false;
+}
+
+return !!xen_backend_lookup(impl, name);
}
static void xen_backend_list_remove(XenBackendInstance *backend)
@@ -138,11 +144,10 @@ void xen_backend_device_create(XenBus *xenbus, const char
*type,
backend = g_new0(XenBackendInstance, 1);
backend->xenbus = xenbus;
backend->name = g_strdup(name);
-
-impl->create(backend, opts, errp);
-
backend->impl = impl;
xen_backend_list_add(backend);
+
+impl->create(backend, opts, errp);
}
XenBus *xen_backend_get_bus(XenBackendInstance *backend)
@@ -155,13 +160,6 @@ const char *xen_backend_get_name(XenBackendInstance
*backend)
return backend->name;
}
-void xen_backend_set_device(XenBackendInstance *backend,
-XenDevice *xendev)
-{
-g_assert(!backend->xendev);
-backend->xendev = xendev;
-}
-
XenDevice *xen_backend_get_device(XenBackendInstance *backend)
{
return backend->xendev;
@@ -178,9 +176,7 @@ bool xen_backend_try_device_destroy(XenDevice *xendev,
Error **errp)
}
impl = backend->impl;
-if (backend->xendev) {
-impl->destroy(backend, errp);
-}
+impl->destroy(backend, errp);
xen_backend_list_remove(backend);
g_free(backend->name);
@@ -188,3 +184,43 @@ bool xen_backend_try_device_destroy(XenDevice *xendev,
Error **errp)
return true;
}
+
+bool xen_backend_device_realized(XenDevice *xendev, Error **errp)
+{
+XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
+const char *type = xendev_class->backend ? :
object_get_typename(OBJECT(xendev));
+const XenBackendImpl *impl = xen_backend_table_lookup(type);
+XenBackendInstance *backend;
+
+if (!impl) {
+return false;
+}
+
+backend = xen_backend_lookup(impl, xendev->name);
+if (backend) {
+if (backend->xendev && backend->xendev != xendev) {
+error_setg(errp, "device %s/%s already exists", type,
xendev->name);
+return false;
+}
+backend->xendev = xendev;
+return true;
+}
+
+backend = g_new0(XenBackendInstance, 1);
+backend->xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
+backend->xendev = xendev;
+backend->name = g_strdup(xendev->name);
+backend->impl = impl;
+
+xen_backend_list_add(backend);
+return true;
+}
+
+void
[PATCH 0/12] Get Xen PV shim running in qemu
I hadn't got round to getting the PV shim running yet; I thought it would
need work on the multiboot loader. Turns out it doesn't. I *did* need to
fix a couple of brown-paper-bag bugs in the per-vCPU upcall vector support,
and implement Xen console support though. Now I can test PV guests:
$ qemu-system-x86_64 --accel kvm,xen-version=0x40011,kernel-irqchip=split \
-chardev stdio,mux=on,id=char0 -device xen-console,chardev=char0 \
-drive file=${GUEST_IMAGE},if=xen -display none -m 1G \
-kernel ~/git/xen/xen/xen -initrd ~/git/linux/arch/x86/boot/bzImage \
-append "loglvl=all -- console=hvc0 root=/dev/xvda1"
blockdev.c | 15 +++-
hw/block/xen-block.c | 26 +-
hw/char/trace-events | 8 ++
hw/char/xen_console.c | 522
--
hw/i386/kvm/meson.build| 1 +
hw/i386/kvm/trace-events | 2 +
hw/i386/kvm/xen-stubs.c| 5 ++
hw/i386/kvm/xen_evtchn.c | 6 ++
hw/i386/kvm/xen_gnttab.c | 32 ++-
hw/i386/kvm/xen_primary_console.c | 167
++
hw/i386/kvm/xen_primary_console.h | 22 +
hw/i386/kvm/xen_xenstore.c | 21 -
hw/xen/xen-backend.c | 81 +
hw/xen/xen-bus.c | 21 -
hw/xen/xen-legacy-backend.c| 1 -
include/hw/xen/interface/arch-arm.h| 37
include/hw/xen/interface/arch-x86/cpuid.h | 31 +++
include/hw/xen/interface/arch-x86/xen-x86_32.h | 19 +---
include/hw/xen/interface/arch-x86/xen-x86_64.h | 19 +---
include/hw/xen/interface/arch-x86/xen.h| 26 +-
include/hw/xen/interface/event_channel.h | 19 +---
include/hw/xen/interface/features.h| 19 +---
include/hw/xen/interface/grant_table.h | 19 +---
include/hw/xen/interface/hvm/hvm_op.h | 19 +---
include/hw/xen/interface/hvm/params.h | 19 +---
include/hw/xen/interface/io/blkif.h| 27 ++
include/hw/xen/interface/io/console.h | 19 +---
include/hw/xen/interface/io/fbif.h | 19 +---
include/hw/xen/interface/io/kbdif.h| 19 +---
include/hw/xen/interface/io/netif.h| 25 ++
include/hw/xen/interface/io/protocols.h| 19 +---
include/hw/xen/interface/io/ring.h | 49 +-
include/hw/xen/interface/io/usbif.h| 19 +---
include/hw/xen/interface/io/xenbus.h | 19 +---
include/hw/xen/interface/io/xs_wire.h | 36
include/hw/xen/interface/memory.h | 30 +++
include/hw/xen/interface/physdev.h | 23 +
include/hw/xen/interface/sched.h | 19 +---
include/hw/xen/interface/trace.h | 19 +---
include/hw/xen/interface/vcpu.h| 19 +---
include/hw/xen/interface/version.h | 19 +---
include/hw/xen/interface/xen-compat.h | 19 +---
include/hw/xen/interface/xen.h | 19 +---
include/hw/xen/xen-backend.h | 4 +
include/hw/xen/xen-bus.h | 2 +
include/sysemu/kvm_xen.h | 1 +
target/i386/kvm/kvm.c | 4 +
target/i386/kvm/xen-emu.c | 35 ++--
48 files changed, 941 insertions(+), 680 deletions(-)
[PATCH 02/12] hw/xen: select kernel mode for per-vCPU event channel upcall vector
From: David Woodhouse
A guest which has configured the per-vCPU upcall vector may set the
HVM_PARAM_CALLBACK_IRQ param to fairly much anything other than zero.
For example, Linux v6.0+ after commit b1c3497e604 ("x86/xen: Add support
for HVMOP_set_evtchn_upcall_vector") will just do this after setting the
vector:
/* Trick toolstack to think we are enlightened. */
if (!cpu)
rc = xen_set_callback_via(1);
That's explicitly setting the delivery to GSI#, but it's supposed to be
overridden by the per-vCPU vector setting. This mostly works in QEMU
*except* for the logic to enable the in-kernel handling of event channels,
which falsely determines that the kernel cannot accelerate GSI delivery
in this case.
Add a kvm_xen_has_vcpu_callback_vector() to report whether vCPU#0 has
the vector set, and use that in xen_evtchn_set_callback_param() to
enable the kernel acceleration features even when the param *appears*
to be set to target a GSI.
Preserve the Xen behaviour that when HVM_PARAM_CALLBACK_IRQ is set to
*zero* the event channel delivery is disabled completely. (Which is
what that bizarre guest behaviour is working round in the first place.)
Fixes: 91cce756179 ("hw/xen: Add xen_evtchn device for event channel emulation")
Signed-off-by: David Woodhouse
---
hw/i386/kvm/xen_evtchn.c | 6 ++
include/sysemu/kvm_xen.h | 1 +
target/i386/kvm/xen-emu.c | 7 +++
3 files changed, 14 insertions(+)
diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
index 4df973022c..d72dca6591 100644
--- a/hw/i386/kvm/xen_evtchn.c
+++ b/hw/i386/kvm/xen_evtchn.c
@@ -490,6 +490,12 @@ int xen_evtchn_set_callback_param(uint64_t param)
break;
}
+/* If the guest has set a per-vCPU callback vector, prefer that. */
+if (gsi && kvm_xen_has_vcpu_callback_vector()) {
+in_kernel = kvm_xen_has_cap(EVTCHN_SEND);
+gsi = 0;
+}
+
if (!ret) {
/* If vector delivery was turned *off* then tell the kernel */
if ((s->callback_param >> CALLBACK_VIA_TYPE_SHIFT) ==
diff --git a/include/sysemu/kvm_xen.h b/include/sysemu/kvm_xen.h
index 595abfbe40..961c702c4e 100644
--- a/include/sysemu/kvm_xen.h
+++ b/include/sysemu/kvm_xen.h
@@ -22,6 +22,7 @@
int kvm_xen_soft_reset(void);
uint32_t kvm_xen_get_caps(void);
void *kvm_xen_get_vcpu_info_hva(uint32_t vcpu_id);
+bool kvm_xen_has_vcpu_callback_vector(void);
void kvm_xen_inject_vcpu_callback_vector(uint32_t vcpu_id, int type);
void kvm_xen_set_callback_asserted(void);
int kvm_xen_set_vcpu_virq(uint32_t vcpu_id, uint16_t virq, uint16_t port);
diff --git a/target/i386/kvm/xen-emu.c b/target/i386/kvm/xen-emu.c
index b49a840438..477e93cd92 100644
--- a/target/i386/kvm/xen-emu.c
+++ b/target/i386/kvm/xen-emu.c
@@ -424,6 +424,13 @@ void kvm_xen_set_callback_asserted(void)
}
}
+bool kvm_xen_has_vcpu_callback_vector(void)
+{
+CPUState *cs = qemu_get_cpu(0);
+
+return cs && !!X86_CPU(cs)->env.xen_vcpu_callback_vector;
+}
+
void kvm_xen_inject_vcpu_callback_vector(uint32_t vcpu_id, int type)
{
CPUState *cs = qemu_get_cpu(vcpu_id);
--
2.40.1
[PATCH 03/12] include: update Xen public headers to Xen 4.17.2 release
From: David Woodhouse
... in order to advertise the XEN_HVM_CPUID_UPCALL_VECTOR feature,
which will come in a subsequent commit.
Signed-off-by: David Woodhouse
---
hw/i386/kvm/xen_xenstore.c| 2 +-
include/hw/xen/interface/arch-arm.h | 37 +++---
include/hw/xen/interface/arch-x86/cpuid.h | 31 +---
.../hw/xen/interface/arch-x86/xen-x86_32.h| 19 +--
.../hw/xen/interface/arch-x86/xen-x86_64.h| 19 +--
include/hw/xen/interface/arch-x86/xen.h | 26 ++
include/hw/xen/interface/event_channel.h | 19 +--
include/hw/xen/interface/features.h | 19 +--
include/hw/xen/interface/grant_table.h| 19 +--
include/hw/xen/interface/hvm/hvm_op.h | 19 +--
include/hw/xen/interface/hvm/params.h | 19 +--
include/hw/xen/interface/io/blkif.h | 27 --
include/hw/xen/interface/io/console.h | 19 +--
include/hw/xen/interface/io/fbif.h| 19 +--
include/hw/xen/interface/io/kbdif.h | 19 +--
include/hw/xen/interface/io/netif.h | 25 +++---
include/hw/xen/interface/io/protocols.h | 19 +--
include/hw/xen/interface/io/ring.h| 49 ++-
include/hw/xen/interface/io/usbif.h | 19 +--
include/hw/xen/interface/io/xenbus.h | 19 +--
include/hw/xen/interface/io/xs_wire.h | 36 ++
include/hw/xen/interface/memory.h | 30 +---
include/hw/xen/interface/physdev.h| 23 ++---
include/hw/xen/interface/sched.h | 19 +--
include/hw/xen/interface/trace.h | 19 +--
include/hw/xen/interface/vcpu.h | 19 +--
include/hw/xen/interface/version.h| 19 +--
include/hw/xen/interface/xen-compat.h | 19 +--
include/hw/xen/interface/xen.h| 19 +--
29 files changed, 124 insertions(+), 523 deletions(-)
diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
index 660d0b72f9..d2b311109b 100644
--- a/hw/i386/kvm/xen_xenstore.c
+++ b/hw/i386/kvm/xen_xenstore.c
@@ -331,7 +331,7 @@ static void xs_error(XenXenstoreState *s, unsigned int id,
const char *errstr = NULL;
for (unsigned int i = 0; i < ARRAY_SIZE(xsd_errors); i++) {
-struct xsd_errors *xsd_error = _errors[i];
+const struct xsd_errors *xsd_error = _errors[i];
if (xsd_error->errnum == errnum) {
errstr = xsd_error->errstring;
diff --git a/include/hw/xen/interface/arch-arm.h
b/include/hw/xen/interface/arch-arm.h
index 94b31511dd..1528ced509 100644
--- a/include/hw/xen/interface/arch-arm.h
+++ b/include/hw/xen/interface/arch-arm.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
/**
* arch-arm.h
*
* Guest OS interface to ARM Xen.
*
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
* Copyright 2011 (C) Citrix Systems
*/
@@ -361,6 +344,7 @@ typedef uint64_t xen_callback_t;
#define PSR_DBG_MASK(1<<9)/* arm64: Debug Exception mask */
#define PSR_IT_MASK (0x0600fc00) /* Thumb If-Then Mask */
#define PSR_JAZELLE (1<<24) /* Jazelle Mode */
+#define PSR_Z (1<<30) /* Zero condition flag */
/* 32 bit modes */
#define PSR_MODE_USR 0x10
@@ -383,7 +367,15 @@ typedef uint64_t xen_callback_t;
#define PSR_MODE_EL1t 0x04
#define PSR_MODE_EL0t 0x00
-#define PSR_GUEST32_INIT (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC)
+/*
+ * We set PSR_Z to be able to boot Linux kernel versions with an invalid
+ * encoding of the first 8 NOP instructions. See commit a92882a4d270 in
+ * Linux.
+ *
+ * Note that PSR_Z is also set by U-Boot and QEMU -kernel when loading
+ * zImage kernels on aarch32.
+ */
+#define PSR_GUEST32_INIT
[PATCH 07/12] hw/xen: update Xen console to XenDevice model
This allows (non-primary) console devices to be created on the command
line.
Signed-off-by: David Woodhouse
---
hw/char/trace-events| 8 +
hw/char/xen_console.c | 502 +++-
hw/xen/xen-legacy-backend.c | 1 -
3 files changed, 381 insertions(+), 130 deletions(-)
diff --git a/hw/char/trace-events b/hw/char/trace-events
index babf4d35ea..7a398c82a5 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -105,3 +105,11 @@ cadence_uart_baudrate(unsigned baudrate) "baudrate %u"
# sh_serial.c
sh_serial_read(char *id, unsigned size, uint64_t offs, uint64_t val) " %s size
%d offs 0x%02" PRIx64 " -> 0x%02" PRIx64
sh_serial_write(char *id, unsigned size, uint64_t offs, uint64_t val) "%s size
%d offs 0x%02" PRIx64 " <- 0x%02" PRIx64
+
+# xen_console.c
+xen_console_connect(unsigned int idx, unsigned int ring_ref, unsigned int
port, unsigned int limit) "idx %u ring_ref %u port %u limit %u"
+xen_console_disconnect(unsigned int idx) "idx %u"
+xen_console_unrealize(unsigned int idx) "idx %u"
+xen_console_realize(unsigned int idx, const char *chrdev) "idx %u chrdev %s"
+xen_console_device_create(unsigned int idx) "idx %u"
+xen_console_device_destroy(unsigned int idx) "idx %u"
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index 810dae3f44..bd20be116c 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -20,15 +20,19 @@
*/
#include "qemu/osdep.h"
+#include "qemu/cutils.h"
#include
#include
#include "qapi/error.h"
#include "sysemu/sysemu.h"
#include "chardev/char-fe.h"
-#include "hw/xen/xen-legacy-backend.h"
-
+#include "hw/xen/xen-backend.h"
+#include "hw/xen/xen-bus-helper.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
#include "hw/xen/interface/io/console.h"
+#include "trace.h"
struct buffer {
uint8_t *data;
@@ -39,16 +43,22 @@ struct buffer {
};
struct XenConsole {
-struct XenLegacyDevice xendev; /* must be first */
+struct XenDevice xendev; /* must be first */
+XenEventChannel *event_channel;
+int dev;
struct buffer buffer;
-char console[XEN_BUFSIZE];
-int ring_ref;
+char *fe_path;
+unsigned int ring_ref;
void *sring;
CharBackend chr;
int backlog;
};
+typedef struct XenConsole XenConsole;
+
+#define TYPE_XEN_CONSOLE_DEVICE "xen-console"
+OBJECT_DECLARE_SIMPLE_TYPE(XenConsole, XEN_CONSOLE_DEVICE)
-static void buffer_append(struct XenConsole *con)
+static bool buffer_append(XenConsole *con)
{
struct buffer *buffer = >buffer;
XENCONS_RING_IDX cons, prod, size;
@@ -60,7 +70,7 @@ static void buffer_append(struct XenConsole *con)
size = prod - cons;
if ((size == 0) || (size > sizeof(intf->out)))
-return;
+return false;
if ((buffer->capacity - buffer->size) < size) {
buffer->capacity += (size + 1024);
@@ -73,7 +83,7 @@ static void buffer_append(struct XenConsole *con)
xen_mb();
intf->out_cons = cons;
-xen_pv_send_notify(>xendev);
+xen_device_notify_event_channel(XEN_DEVICE(con), con->event_channel, NULL);
if (buffer->max_capacity &&
buffer->size > buffer->max_capacity) {
@@ -89,6 +99,7 @@ static void buffer_append(struct XenConsole *con)
if (buffer->consumed > buffer->max_capacity - over)
buffer->consumed = buffer->max_capacity - over;
}
+return true;
}
static void buffer_advance(struct buffer *buffer, size_t len)
@@ -100,7 +111,7 @@ static void buffer_advance(struct buffer *buffer, size_t
len)
}
}
-static int ring_free_bytes(struct XenConsole *con)
+static int ring_free_bytes(XenConsole *con)
{
struct xencons_interface *intf = con->sring;
XENCONS_RING_IDX cons, prod, space;
@@ -118,13 +129,13 @@ static int ring_free_bytes(struct XenConsole *con)
static int xencons_can_receive(void *opaque)
{
-struct XenConsole *con = opaque;
+XenConsole *con = opaque;
return ring_free_bytes(con);
}
static void xencons_receive(void *opaque, const uint8_t *buf, int len)
{
-struct XenConsole *con = opaque;
+XenConsole *con = opaque;
struct xencons_interface *intf = con->sring;
XENCONS_RING_IDX prod;
int i, max;
@@ -141,10 +152,10 @@ static void xencons_receive(void *opaque, const uint8_t
*buf, int len)
}
xen_wmb();
intf->in_prod = prod;
-xen_pv_send_notify(>xendev);
+xen_device_notify_event_channel(XEN_DEVICE(con), con->event_channel, NULL);
}
-static void xencons_send(struct XenConsole *con)
+static bool xencons_send(XenConsole *con)
{
ssize_t len, size;
@@ -159,174 +170,407 @@ static void xencons_send(struct XenConsole *con)
if (len < 1) {
if (!con->backlog) {
con->backlog = 1;
-xen_pv_printf(>xendev, 1,
- "backlog piling up, nobody listening?\n");
}
[PATCH 08/12] hw/xen: do not repeatedly try to create a failing backend device
From: David Woodhouse
If xen_backend_device_create() fails to instantiate a device, the XenBus
code will just keep trying over and over again each time the bus is
re-enumerated, as long as the backend appears online and in
XenbusStateInitialising.
The only thing which prevents the XenBus code from recreating duplicates
of devices which already exist, is the fact that xen_device_realize()
sets the backend state to XenbusStateInitWait. If the attempt to create
the device doesn't get *that* far, that's when it will keep getting
retried.
My first thought was to handle errors by setting the backend state to
XenbusStateClosed, but that doesn't work for XenConsole which wants to
*ignore* any device of type != "ioemu" completely.
So, make xen_backend_device_create() *keep* the XenBackendInstance for a
failed device, and provide a new xen_backend_exists() function to allow
xen_bus_type_enumerate() to check whether one already exists before
creating a new one.
Signed-off-by: David Woodhouse
---
hw/xen/xen-backend.c | 27 +--
hw/xen/xen-bus.c | 3 ++-
include/hw/xen/xen-backend.h | 1 +
3 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/hw/xen/xen-backend.c b/hw/xen/xen-backend.c
index 5b0fb76eae..b9bf70a9f5 100644
--- a/hw/xen/xen-backend.c
+++ b/hw/xen/xen-backend.c
@@ -101,6 +101,24 @@ static XenBackendInstance *xen_backend_list_find(XenDevice
*xendev)
return NULL;
}
+bool xen_backend_exists(const char *type, const char *name)
+{
+const XenBackendImpl *impl = xen_backend_table_lookup(type);
+XenBackendInstance *backend;
+
+if (!impl) {
+return false;
+}
+
+QLIST_FOREACH(backend, _list, entry) {
+if (backend->impl == impl && !strcmp(backend->name, name)) {
+return true;
+}
+}
+
+return false;
+}
+
static void xen_backend_list_remove(XenBackendInstance *backend)
{
QLIST_REMOVE(backend, entry);
@@ -122,11 +140,6 @@ void xen_backend_device_create(XenBus *xenbus, const char
*type,
backend->name = g_strdup(name);
impl->create(backend, opts, errp);
-if (*errp) {
-g_free(backend->name);
-g_free(backend);
-return;
-}
backend->impl = impl;
xen_backend_list_add(backend);
@@ -165,7 +178,9 @@ bool xen_backend_try_device_destroy(XenDevice *xendev,
Error **errp)
}
impl = backend->impl;
-impl->destroy(backend, errp);
+if (backend->xendev) {
+impl->destroy(backend, errp);
+}
xen_backend_list_remove(backend);
g_free(backend->name);
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index cc524ed92c..0da2aa219a 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -209,7 +209,8 @@ static void xen_bus_type_enumerate(XenBus *xenbus, const
char *type)
NULL, "%u", ) != 1)
online = 0;
-if (online && state == XenbusStateInitialising) {
+if (online && state == XenbusStateInitialising &&
+!xen_backend_exists(type, backend[i])) {
Error *local_err = NULL;
xen_bus_backend_create(xenbus, type, backend[i], backend_path,
diff --git a/include/hw/xen/xen-backend.h b/include/hw/xen/xen-backend.h
index aac2fd454d..0f01631ae7 100644
--- a/include/hw/xen/xen-backend.h
+++ b/include/hw/xen/xen-backend.h
@@ -33,6 +33,7 @@ XenDevice *xen_backend_get_device(XenBackendInstance
*backend);
void xen_backend_register(const XenBackendInfo *info);
const char **xen_backend_get_types(unsigned int *nr);
+bool xen_backend_exists(const char *type, const char *name);
void xen_backend_device_create(XenBus *xenbus, const char *type,
const char *name, QDict *opts, Error **errp);
bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp);
--
2.40.1
[PATCH 05/12] hw/xen: populate store frontend nodes with XenStore PFN/port
From: David Woodhouse
This is kind of redundant since without being able to get these through
ome other method (HVMOP_get_param) the guest wouldn't be able to access
XenStore in order to find them. But Xen populates them, and it does
allow guests to *rebind* to the event channel port after a reset.
Signed-off-by: David Woodhouse
---
hw/i386/kvm/xen_xenstore.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
index d2b311109b..3300e0614a 100644
--- a/hw/i386/kvm/xen_xenstore.c
+++ b/hw/i386/kvm/xen_xenstore.c
@@ -1432,6 +1432,7 @@ static void alloc_guest_port(XenXenstoreState *s)
int xen_xenstore_reset(void)
{
XenXenstoreState *s = xen_xenstore_singleton;
+GList *perms;
int err;
if (!s) {
@@ -1459,6 +1460,15 @@ int xen_xenstore_reset(void)
}
s->be_port = err;
+/* Create frontend store nodes */
+perms = g_list_append(NULL, xs_perm_as_string(XS_PERM_NONE, DOMID_QEMU));
+perms = g_list_append(perms, xs_perm_as_string(XS_PERM_READ, xen_domid));
+
+relpath_printf(s, perms, "store/ring-ref", "%lu",
XEN_SPECIAL_PFN(XENSTORE));
+relpath_printf(s, perms, "store/port", "%u", s->be_port);
+
+g_list_free_full(perms, g_free);
+
/*
* We don't actually access the guest's page through the grant, because
* this isn't real Xen, and we can just use the page we gave it in the
--
2.40.1
[PATCH 10/12] hw/xen: automatically assign device index to console devices
From: David Woodhouse
Now that we can reliably tell whether a given device already exists, we
can allow the user to add console devices on the command line with just
'-device xen-console,chardev=foo'.
Start at 1, because we can't add the *primary* console; that's special
because the toolstack has to set up the guest end of that.
Signed-off-by: David Woodhouse
---
hw/char/xen_console.c | 16
1 file changed, 16 insertions(+)
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index 2825b8c511..1a0f5ed3e1 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -333,6 +333,22 @@ static char *xen_console_get_name(XenDevice *xendev, Error
**errp)
{
XenConsole *con = XEN_CONSOLE_DEVICE(xendev);
+if (con->dev == -1) {
+char name[11];
+int idx = 1;
+
+/* Theoretically we could go up to INT_MAX here but that's overkill */
+while (idx < 100) {
+snprintf(name, sizeof(name), "%u", idx);
+if (!xen_backend_exists("console", name)) {
+con->dev = idx;
+return g_strdup(name);
+}
+idx++;
+}
+error_setg(errp, "cannot find device index for console device");
+return NULL;
+}
return g_strdup_printf("%u", con->dev);
}
--
2.40.1
[PATCH 11/12] hw/xen: automatically assign device index to block devices
From: David Woodhouse
There's no need to force the user to assign a vdev. We can automatically
assign one, starting at xvda and searching until we find the first disk
name that's unused.
This means we can now allow '-drive if=xen,file=xxx' to work without an
explicit separate -driver argument, just like if=virtio.
Signed-off-by: David Woodhouse
---
blockdev.c | 15 ---
hw/block/xen-block.c | 25 +
2 files changed, 37 insertions(+), 3 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 325b7a3bef..9dec4c9c74 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -255,13 +255,13 @@ void drive_check_orphaned(void)
* Ignore default drives, because we create certain default
* drives unconditionally, then leave them unclaimed. Not the
* users fault.
- * Ignore IF_VIRTIO, because it gets desugared into -device,
- * so we can leave failing to -device.
+ * Ignore IF_VIRTIO or IF_XEN, because it gets desugared into
+ * -device, so we can leave failing to -device.
* Ignore IF_NONE, because leaving unclaimed IF_NONE remains
* available for device_add is a feature.
*/
if (dinfo->is_default || dinfo->type == IF_VIRTIO
-|| dinfo->type == IF_NONE) {
+|| dinfo->type == IF_XEN || dinfo->type == IF_NONE) {
continue;
}
if (!blk_get_attached_dev(blk)) {
@@ -977,6 +977,15 @@ DriveInfo *drive_new(QemuOpts *all_opts,
BlockInterfaceType block_default_type,
qemu_opt_set(devopts, "driver", "virtio-blk", _abort);
qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
_abort);
+} else if (type == IF_XEN) {
+QemuOpts *devopts;
+devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
+ _abort);
+qemu_opt_set(devopts, "driver",
+ (media == MEDIA_CDROM) ? "xen-cdrom" : "xen-disk",
+ _abort);
+qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
+ _abort);
}
filename = qemu_opt_get(legacy_opts, "file");
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 9262338535..20fa783cbe 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -34,6 +34,31 @@ static char *xen_block_get_name(XenDevice *xendev, Error
**errp)
XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
XenBlockVdev *vdev = >props.vdev;
+if (blockdev->props.vdev.type == XEN_BLOCK_VDEV_TYPE_INVALID) {
+char name[11];
+int disk = 0;
+unsigned long idx;
+
+/* Find an unoccupied device name */
+while (disk < (1 << 20)) {
+if (disk < (1 << 4)) {
+idx = (202 << 8) | (disk << 4);
+} else {
+idx = (1 << 28) | (disk << 8);
+}
+snprintf(name, sizeof(name), "%lu", idx);
+if (!xen_backend_exists("qdisk", name)) {
+vdev->type = XEN_BLOCK_VDEV_TYPE_XVD;
+vdev->partition = 0;
+vdev->disk = disk;
+vdev->number = idx;
+return g_strdup(name);
+}
+disk++;
+}
+error_setg(errp, "cannot find device vdev for block device");
+return NULL;
+}
return g_strdup_printf("%lu", vdev->number);
}
--
2.40.1
[PATCH 01/12] i386/xen: fix per-vCPU upcall vector for Xen emulation
From: David Woodhouse
The per-vCPU upcall vector support had two problems. Firstly it was
using the wrong hypercall argument and would always return -EFAULT.
And secondly it was using the wrong ioctl() to pass the vector to
the kernel and thus the *kernel* would always return -EINVAL.
Linux doesn't (yet) use this mode so it went without decent testing
for a while.
Fixes: 105b47fdf2d0 ("i386/xen: implement HVMOP_set_evtchn_upcall_vector")
Signed-off-by: David Woodhouse
---
target/i386/kvm/xen-emu.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/target/i386/kvm/xen-emu.c b/target/i386/kvm/xen-emu.c
index c4fa84a982..b49a840438 100644
--- a/target/i386/kvm/xen-emu.c
+++ b/target/i386/kvm/xen-emu.c
@@ -306,7 +306,7 @@ static int kvm_xen_set_vcpu_callback_vector(CPUState *cs)
trace_kvm_xen_set_vcpu_callback(cs->cpu_index, vector);
-return kvm_vcpu_ioctl(cs, KVM_XEN_HVM_SET_ATTR, );
+return kvm_vcpu_ioctl(cs, KVM_XEN_VCPU_SET_ATTR, );
}
static void do_set_vcpu_callback_vector(CPUState *cs, run_on_cpu_data data)
@@ -849,8 +849,7 @@ static bool kvm_xen_hcall_hvm_op(struct kvm_xen_exit *exit,
X86CPU *cpu,
int ret = -ENOSYS;
switch (cmd) {
case HVMOP_set_evtchn_upcall_vector:
-ret = kvm_xen_hcall_evtchn_upcall_vector(exit, cpu,
- exit->u.hcall.params[0]);
+ret = kvm_xen_hcall_evtchn_upcall_vector(exit, cpu, arg);
break;
case HVMOP_pagetable_dying:
--
2.40.1
Re: [XEN PATCH] xen/irq: address violations of MISRA C:2012 Rule 8.2
On 03.10.2023 02:19, Henry Wang wrote: >> On Oct 3, 2023, at 06:45, Stefano Stabellini wrote: >> >> On Mon, 2 Oct 2023, Federico Serafini wrote: >>> Add missing parameter names. No functional change. >>> >>> Signed-off-by: Federico Serafini >> >> Reviewed-by: Stefano Stabellini > > Release-acked-by: Henry Wang Same question here wrt applicability now that we're past RC3. Jan
Re: [XEN PATCH] xen/irq: address violations of MISRA C:2012 Rule 8.2
On 03.10.2023 00:45, Stefano Stabellini wrote: > On Mon, 2 Oct 2023, Federico Serafini wrote: >> Add missing parameter names. No functional change. >> >> Signed-off-by: Federico Serafini > > Reviewed-by: Stefano Stabellini Acked-by: Jan Beulich
Re: [XEN PATCH] x86/paging: address a violation of MISRA C:2012 Rule 8.3
On 03.10.2023 02:19, Henry Wang wrote: >> On Oct 3, 2023, at 06:46, Stefano Stabellini wrote: >> >> On Mon, 2 Oct 2023, Federico Serafini wrote: >>> Make function declaration and definition consistent. >>> No functional change. >>> >>> Signed-off-by: Federico Serafini >> >> Reviewed-by: Stefano Stabellini > > Release-acked-by: Henry Wang We're past RC3, so I'd like to confirm whether this still holds. Jan
Re: [XEN PATCH] x86/paging: address a violation of MISRA C:2012 Rule 8.3
On 03.10.2023 00:46, Stefano Stabellini wrote: > On Mon, 2 Oct 2023, Federico Serafini wrote: >> Make function declaration and definition consistent. >> No functional change. >> >> Signed-off-by: Federico Serafini > > Reviewed-by: Stefano Stabellini Acked-by: Jan Beulich
Re: [PATCH v4 2/2] xen/arm: Enlarge identity map space to 10TB
Hi,
> On 16 Oct 2023, at 16:46, Julien Grall wrote:
>
> CC Henry
>
> Hi Alexey,
>
> On 16/10/2023 15:24, Alexey Klimov wrote:
>> On Mon, 16 Oct 2023 at 15:13, Luca Fancellu wrote:
>>>
>>>
>>>
On 16 Oct 2023, at 15:00, Bertrand Marquis
wrote:
Hi
+Luca and Rahul
> On 16 Oct 2023, at 15:54, Julien Grall wrote:
>
>
>
> On 16/10/2023 09:44, Michal Orzel wrote:
>> Hi,
>
> Hi,
>
>> On 13/10/2023 14:26, Leo Yan wrote:
>>>
>>>
>>> On ADLink AVA platform (Ampere Altra SoC with 32 Arm Neoverse N1 cores),
>>> the physical memory regions are:
>>>
>>> DRAM memory regions:
>>>Node[0] Region[0]: 0x8000 - 0x
>>>Node[0] Region[1]: 0x0800 - 0x08007fff
>>>Node[0] Region[2]: 0x0801 - 0x0807
>>>
>>> The UEFI loads Xen hypervisor and DTB into the high memory, the kernel
>>> and ramdisk images are loaded into the low memory space:
>>>
>>> (XEN) MODULE[0]: 0807f6df - 0807f6f3e000 Xen
>>> (XEN) MODULE[1]: 0807f8054000 - 0807f8056000 Device Tree
>>> (XEN) MODULE[2]: fa834000 - fc5de1d5 Ramdisk
>>> (XEN) MODULE[3]: fc5df000 - ffb3f810 Kernel
>>>
>>> In this case, the Xen binary is loaded above 8TB, which exceeds the
>>> maximum supported identity map space of 2TB in Xen. Consequently, the
>>> system fails to boot.
>>>
>>> This patch enlarges identity map space to 10TB, allowing module loading
>>> within the range of [0x0 .. 0x09ff__].
>>>
>>> Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to
>>> prepare/enable/disable")
>> I don't think a fixes tag applies here given that 2TB was just a number
>> we believed is enough
>> and all of this is platform dependent.
>> This can be dropped on commit if committer agrees
> Xen may have booted on that platform before hand. So this would be
> considered a regression and therefore a tag would be warrant.
>
> AFAICT, the commit is only present on the upcoming 4.18. So the question
> is whether Xen 4.17 booted out-of-the-box on ADLink? If the answer is
> yes, then we need to add a Fixes tag. But the correct one would be
>
@Rahul or Luca: could you give an answer here ?
I know you used Xen on an AVA platform but was it booting out of the box ?
>>>
>>> I can’t say for Xen 4.17, but our nightly job has run successfully on AVA
>>> for the commit 730406ab81094115d9fb5ca00ba8d53cec1279b3
>>> (docs/misra: add deviations.rst to document additional deviations.)
>>>
>>> We are not applying any patch for it to run on AVA.
>> Most likely it is because your UEFI/BIOS firmware is 2.x, for instance
>> 2.04.100.07.
>> This fix if for AVA machine with older UEFI firmware 1.07.300.03.
>
> OOI, why not updating your firmware? I was expecting that it would also
> contain other critical fixes.
>
> With this in mind, I am now more in two mind to ask to merge this patch in
> Xen 4.18. On one hand, I understand it will help to boot on AVA machine with
> an older firmware. But on the other hand this is changing the memory layout
> quite late in the release. The risk seems limited because Xen is not loaded
> at the top of the virtual address space (there is the directmap afterwards).
>
> Henry (as the release manager) and others, any opinions?
With the new information, I think it should be stated that it is fixing an
issue on AVA platforms using an old firmware and platforms with and up-to-date
firmware are not impacted.
It is an important information to keep in mind that this is not a fix to be
backported for example (and for me the fixes tag should not be kept).
On whether or not it should go in the release, I am not quite sure that making
a change in the layout at that stage is a good idea unless it is fixing a
critical issue (which is not the case here).
So i would vote no but not go against if someone argue to have it in.
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
Re: [XEN PATCH 4/4] x86/psr: address a violation of MISRA C:2012 Rule 8.3
On 29.09.2023 22:54, Stefano Stabellini wrote: > On Fri, 29 Sep 2023, Federico Serafini wrote: >> Make function declaration and definition consistent. >> No functional change. >> >> Signed-off-by: Federico Serafini > > Reviewed-by: Stefano Stabellini Acked-by: Jan Beulich
Re: [XEN PATCH 3/4] x86/xstate: address a violation of MISRA C:2012 Rule 8.3
On 29.09.2023 22:53, Stefano Stabellini wrote: > On Fri, 29 Sep 2023, Federico Serafini wrote: >> Make function declaration and definition consistent. >> No functional change. >> >> Signed-off-by: Federico Serafini > > Reviewed-by: Stefano Stabellini Acked-by: Jan Beulich
Re: [XEN PATCH 2/4] x86/uaccess: address violations of MISRA C:2012 Rule 8.3
On 29.09.2023 22:53, Stefano Stabellini wrote: > On Fri, 29 Sep 2023, Federico Serafini wrote: >> Make function declarations and definitions consistent. >> No functional change. >> >> Signed-off-by: Federico Serafini > > Reviewed-by: Stefano Stabellini Acked-by: Jan Beulich
Re: [RFC PATCH v3 08/78] hw/block: add fallthrough pseudo-keyword
On Fri, Oct 13, 2023 at 11:45:36AM +0300, Emmanouil Pitsidianakis wrote: > In preparation of raising -Wimplicit-fallthrough to 5, replace all > fall-through comments with the fallthrough attribute pseudo-keyword. > > Signed-off-by: Emmanouil Pitsidianakis > --- > hw/block/dataplane/xen-block.c | 4 ++-- > hw/block/m25p80.c | 2 +- > hw/block/onenand.c | 2 +- > hw/block/pflash_cfi01.c| 1 + > hw/block/pflash_cfi02.c| 6 -- > 5 files changed, 9 insertions(+), 6 deletions(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [XEN PATCH] x86/msi: address violations of MISRA C:2012 Rules 8.2 and 8.3
On 29.09.2023 22:51, Stefano Stabellini wrote: > On Fri, 29 Sep 2023, Federico Serafini wrote: >> Add missing parameter names and make function declarations and >> definitions consistent. >> No functional change. >> >> Signed-off-by: Federico Serafini > > Reviewed-by: Stefano Stabellini Acked-by: Jan Beulich
Re: [XEN PATCH v2 3/7] x86: add deviation comments for asm-only functions
On 09.10.2023 08:54, Nicola Vetrini wrote: > As stated in rules.rst, functions used only in asm code > are allowed to have no prior declaration visible when being > defined, hence these functions are deviated. > This also fixes violations of MISRA C:2012 Rule 8.4. > > Signed-off-by: Nicola Vetrini > Reviewed-by: Stefano Stabellini > --- > xen/arch/x86/hvm/svm/intr.c | 1 + > xen/arch/x86/hvm/svm/nestedsvm.c | 1 + > xen/arch/x86/hvm/svm/svm.c | 2 ++ Once again - why are you not also adjusting the respective VMX code? Iirc it was agreed long ago that scans should be extended to cover as much of the code base as possible. Jan
Re: [XEN PATCH v2 2/7] x86: add deviations for variables only used in asm code
On 09.10.2023 08:54, Nicola Vetrini wrote:
> These variables are only used by asm code, and therefore
> the lack of a declaration is justified by the corresponding
> deviation comment.
Hmm, you say "declaration" here, but according to my understanding ...
> --- a/xen/arch/x86/include/asm/asm_defns.h
> +++ b/xen/arch/x86/include/asm/asm_defns.h
> @@ -31,6 +31,7 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, "
> * gets set up by the containing function.
> */
> #ifdef CONFIG_FRAME_POINTER
> +/* SAF-1-safe */
> register unsigned long current_stack_pointer asm("rsp");
... this is a declaration, not a definition.
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -153,6 +153,7 @@ char __section(".init.bss.stack_aligned")
> __aligned(STACK_SIZE)
> void *stack_start = cpu0_stack + STACK_SIZE - sizeof(struct cpu_info);
>
> /* Used by the boot asm to stash the relocated multiboot info pointer. */
> +/* SAF-1-safe */
> unsigned int __initdata multiboot_ptr;
Imo such comments want folding; question is whether the tooling can
cope.
Jan
Re: [PATCH for-4.18 v2] x86/pvh: fix identity mapping of low 1MB
On 16.10.2023 16:51, Roger Pau Monné wrote:
> On Mon, Oct 16, 2023 at 04:07:22PM +0200, Jan Beulich wrote:
>> On 16.10.2023 15:51, Roger Pau Monné wrote:
>>> On Mon, Oct 16, 2023 at 03:32:54PM +0200, Jan Beulich wrote:
On 13.10.2023 10:56, Roger Pau Monne wrote:
> The mapping of memory regions below the 1MB mark was all done by the PVH
> dom0
> builder code, causing the region to be avoided by the arch specific IOMMU
> hardware domain initialization code. That lead to the IOMMU being enabled
> without reserved regions in the low 1MB identity mapped in the p2m for PVH
> hardware domains. Firmware which happens to be missing RMRR/IVMD ranges
> describing E820 reserved regions in the low 1MB would transiently trigger
> IOMMU
> faults until the p2m is populated by the PVH dom0 builder:
>
> AMD-Vi: IO_PAGE_FAULT: :00:13.1 d0 addr 000eb380 flags 0x20 RW
> AMD-Vi: IO_PAGE_FAULT: :00:13.1 d0 addr 000eb340 flags 0
> AMD-Vi: IO_PAGE_FAULT: :00:13.2 d0 addr 000ea1c0 flags 0
> AMD-Vi: IO_PAGE_FAULT: :00:14.5 d0 addr 000eb480 flags 0x20 RW
> AMD-Vi: IO_PAGE_FAULT: :00:12.0 d0 addr 000eb080 flags 0x20 RW
> AMD-Vi: IO_PAGE_FAULT: :00:14.5 d0 addr 000eb400 flags 0
> AMD-Vi: IO_PAGE_FAULT: :00:12.0 d0 addr 000eb040 flags 0
>
> Those errors have been observed on the osstest pinot{0,1} boxes (AMD
> Fam15h
> Opteron(tm) Processor 3350 HE).
>
> Mostly remove the special handling of the low 1MB done by the PVH dom0
> builder,
> leaving just the data copy between RAM regions. Otherwise rely on the
> IOMMU
> arch init code to create any identity mappings for reserved regions in
> that
> range (like it already does for reserved regions elsewhere).
>
> Note there's a small difference in behavior, as holes in the low 1MB will
> no
> longer be identity mapped to the p2m.
I certainly like the simplification, but I'm concerned by this: The BDA
is not normally reserved, yet may want accessing by Dom0 (to see the real
machine contents). We do access that first page of memory ourselves, so
I expect OSes may do so as well (even if the specific aspect I'm thinking
of - the warm/cold reboot field - is under Xen's control).
>>>
>>> The BDA on the systems I've checked falls into a RAM area on the
>>> memory map, but if you think it can be problematic I could arrange for
>>> arch_iommu_hwdom_init() to also identity map holes in the low 1MB.
>>
>> Hmm, this again is a case where I'd wish CPU and IOMMU mappings could
>> be different. I don't see reasons to try I/O to such holes, but I can
>> see reasons for CPU accesses (of more or less probing kind).
>
> Hm, while I agree devices have likely no reason to access holes (there
> or elsewhere) I don't see much benefit of having this differentiation,
> it's easier to just map everything for accesses from both device and
> CPU rather than us having to decide (and maybe get wrong) whether
> ranges should only be accessed by the CPU.
I understand that, and I also follow Andrew's arguments towards not
making such a distinction. The consequence though is that we need
to map more than possibly necessary, and never too little.
>>> Keep in mind this is only for PVH, it won't affect PV.
>>
>> Of course.
>
> Would you be willing to Ack it?
If "it" is the present version, then me doing so would be stretch.
How averse are you to re-adding the hole mappings?
Jan
Re: [PATCH v4 2/2] xen/arm: Enlarge identity map space to 10TB
Hi Leo,
On 16/10/2023 14:54, Leo Yan wrote:
On Mon, Oct 16, 2023 at 01:40:26PM +, Bertrand Marquis wrote:
[...]
This patch enlarges identity map space to 10TB, allowing module loading
within the range of [0x0 .. 0x09ff__].
Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to prepare/enable/disable")
I agree with Michal here, this is not a fix so this should be removed (can be
done
on commit).
This is fine for me.
I'd like to confirm with maintainers that should I spin a new patch
set to remove the fix tag? Or maintainers could help to remove it
when pick up this patch set.
I can do small changes while committing (updating the fixes tag is one).
That said, we are in the middle of the code freeze for Xen 4.18. Any
patch requires a release-ack from the release manager (Henry). I am a
bit split at the moment whether we want this patch for Xen 4.18. So I
have asked opinions from Henry (others can also provide some).
If it doesn't go for Xen 4.18, then Stefano might be able to queue it in
his for-4.19. Otherwise, it will be picked up when the tree re-open
hopefully in a couple of weeks time.
Cheers,
And thanks for review, Michal and Bertrand.
Leo
Reported-by: Alexey Klimov
Signed-off-by: Leo Yan
Reviewed-by: Bertrand Marquis
Cheers
Bertrand
--
Julien Grall
Re: [XEN PATCH][for-4.19 v2 6/7] xen/console: make function static inline
On 09.10.2023 08:54, Nicola Vetrini wrote:
> --- a/xen/include/xen/consoled.h
> +++ b/xen/include/xen/consoled.h
> @@ -12,7 +12,7 @@ size_t consoled_guest_tx(char c);
>
> #else
>
> -size_t consoled_guest_tx(char c) { return 0; }
> +static inline size_t consoled_guest_tx(char c) { return 0; }
Why inline? We do so in headers, but we generally avoid "inline" in
.c files.
Jan
Re: [PATCH for-4.18 v2] x86/pvh: fix identity mapping of low 1MB
On Mon, Oct 16, 2023 at 04:07:22PM +0200, Jan Beulich wrote:
> On 16.10.2023 15:51, Roger Pau Monné wrote:
> > On Mon, Oct 16, 2023 at 03:32:54PM +0200, Jan Beulich wrote:
> >> On 13.10.2023 10:56, Roger Pau Monne wrote:
> >>> The mapping of memory regions below the 1MB mark was all done by the PVH
> >>> dom0
> >>> builder code, causing the region to be avoided by the arch specific IOMMU
> >>> hardware domain initialization code. That lead to the IOMMU being enabled
> >>> without reserved regions in the low 1MB identity mapped in the p2m for PVH
> >>> hardware domains. Firmware which happens to be missing RMRR/IVMD ranges
> >>> describing E820 reserved regions in the low 1MB would transiently trigger
> >>> IOMMU
> >>> faults until the p2m is populated by the PVH dom0 builder:
> >>>
> >>> AMD-Vi: IO_PAGE_FAULT: :00:13.1 d0 addr 000eb380 flags 0x20 RW
> >>> AMD-Vi: IO_PAGE_FAULT: :00:13.1 d0 addr 000eb340 flags 0
> >>> AMD-Vi: IO_PAGE_FAULT: :00:13.2 d0 addr 000ea1c0 flags 0
> >>> AMD-Vi: IO_PAGE_FAULT: :00:14.5 d0 addr 000eb480 flags 0x20 RW
> >>> AMD-Vi: IO_PAGE_FAULT: :00:12.0 d0 addr 000eb080 flags 0x20 RW
> >>> AMD-Vi: IO_PAGE_FAULT: :00:14.5 d0 addr 000eb400 flags 0
> >>> AMD-Vi: IO_PAGE_FAULT: :00:12.0 d0 addr 000eb040 flags 0
> >>>
> >>> Those errors have been observed on the osstest pinot{0,1} boxes (AMD
> >>> Fam15h
> >>> Opteron(tm) Processor 3350 HE).
> >>>
> >>> Mostly remove the special handling of the low 1MB done by the PVH dom0
> >>> builder,
> >>> leaving just the data copy between RAM regions. Otherwise rely on the
> >>> IOMMU
> >>> arch init code to create any identity mappings for reserved regions in
> >>> that
> >>> range (like it already does for reserved regions elsewhere).
> >>>
> >>> Note there's a small difference in behavior, as holes in the low 1MB will
> >>> no
> >>> longer be identity mapped to the p2m.
> >>
> >> I certainly like the simplification, but I'm concerned by this: The BDA
> >> is not normally reserved, yet may want accessing by Dom0 (to see the real
> >> machine contents). We do access that first page of memory ourselves, so
> >> I expect OSes may do so as well (even if the specific aspect I'm thinking
> >> of - the warm/cold reboot field - is under Xen's control).
> >
> > The BDA on the systems I've checked falls into a RAM area on the
> > memory map, but if you think it can be problematic I could arrange for
> > arch_iommu_hwdom_init() to also identity map holes in the low 1MB.
>
> Hmm, this again is a case where I'd wish CPU and IOMMU mappings could
> be different. I don't see reasons to try I/O to such holes, but I can
> see reasons for CPU accesses (of more or less probing kind).
Hm, while I agree devices have likely no reason to access holes (there
or elsewhere) I don't see much benefit of having this differentiation,
it's easier to just map everything for accesses from both device and
CPU rather than us having to decide (and maybe get wrong) whether
ranges should only be accessed by the CPU.
> > Keep in mind this is only for PVH, it won't affect PV.
>
> Of course.
Would you be willing to Ack it?
Thanks, Roger.
Re: [XEN PATCH v2 1/7] xen: add declarations for variables where needed
On 09.10.2023 08:54, Nicola Vetrini wrote:
> Some variables with external linkage used in C code do not have
> a visible declaration where they are defined. Providing such
> declaration also resolves violations of MISRA C:2012 Rule 8.4.
>
> Signed-off-by: Nicola Vetrini
This is a mix of different approaches to the same underlying issue. I
think respectively splitting would have helped.
> --- a/xen/arch/x86/include/asm/compat.h
> +++ b/xen/arch/x86/include/asm/compat.h
> @@ -13,6 +13,7 @@ typedef unsigned long full_ptr_t;
>
> struct domain;
> #ifdef CONFIG_PV32
> +extern unsigned long cr4_pv32_mask;
Why is this needed? Didn't we say declarations aren't needed when the
only consumer is assembly code? (I also wonder how this header is any
more "appropriate" - see the changelog entry - than about any other
one.)
> --- a/xen/arch/x86/include/asm/setup.h
> +++ b/xen/arch/x86/include/asm/setup.h
> @@ -13,6 +13,7 @@ extern char __2M_rwdata_start[], __2M_rwdata_end[];
> extern unsigned long xenheap_initial_phys_start;
> extern uint64_t boot_tsc_stamp;
>
> +extern char cpu0_stack[STACK_SIZE];
Same question here.
> --- a/xen/common/symbols.c
> +++ b/xen/common/symbols.c
> @@ -21,23 +21,6 @@
> #include
> #include
>
> -#ifdef SYMBOLS_ORIGIN
> -extern const unsigned int symbols_offsets[];
> -#define symbols_address(n) (SYMBOLS_ORIGIN + symbols_offsets[n])
> -#else
> -extern const unsigned long symbols_addresses[];
> -#define symbols_address(n) symbols_addresses[n]
> -#endif
> -extern const unsigned int symbols_num_syms;
> -extern const u8 symbols_names[];
> -
> -extern const struct symbol_offset symbols_sorted_offsets[];
> -
> -extern const u8 symbols_token_table[];
> -extern const u16 symbols_token_index[];
> -
> -extern const unsigned int symbols_markers[];
> -
> /* expand a compressed symbol data into the resulting uncompressed string,
> given the offset to where the symbol is in the compressed stream */
> static unsigned int symbols_expand_symbol(unsigned int off, char *result)
> --- a/xen/include/xen/symbols.h
> +++ b/xen/include/xen/symbols.h
> @@ -33,4 +33,22 @@ struct symbol_offset {
> uint32_t stream; /* .. in the compressed stream.*/
> uint32_t addr; /* .. and in the fixed size address array. */
> };
> +
> +#ifdef SYMBOLS_ORIGIN
> +extern const unsigned int symbols_offsets[];
> +#define symbols_address(n) (SYMBOLS_ORIGIN + symbols_offsets[n])
> +#else
> +extern const unsigned long symbols_addresses[];
> +#define symbols_address(n) symbols_addresses[n]
> +#endif
> +extern const unsigned int symbols_num_syms;
> +extern const u8 symbols_names[];
> +
> +extern const struct symbol_offset symbols_sorted_offsets[];
> +
> +extern const u8 symbols_token_table[];
> +extern const u16 symbols_token_index[];
> +
> +extern const unsigned int symbols_markers[];
> +
> #endif /*_XEN_SYMBOLS_H*/
This change is even less clear to me: The producer is assembly code,
and the consumer already had appropriate declarations. Why would we
want to increase the scope of their visibility?
Jan
Re: [PATCH v4 2/2] xen/arm: Enlarge identity map space to 10TB
CC Henry
Hi Alexey,
On 16/10/2023 15:24, Alexey Klimov wrote:
On Mon, 16 Oct 2023 at 15:13, Luca Fancellu wrote:
On 16 Oct 2023, at 15:00, Bertrand Marquis wrote:
Hi
+Luca and Rahul
On 16 Oct 2023, at 15:54, Julien Grall wrote:
On 16/10/2023 09:44, Michal Orzel wrote:
Hi,
Hi,
On 13/10/2023 14:26, Leo Yan wrote:
On ADLink AVA platform (Ampere Altra SoC with 32 Arm Neoverse N1 cores),
the physical memory regions are:
DRAM memory regions:
Node[0] Region[0]: 0x8000 - 0x
Node[0] Region[1]: 0x0800 - 0x08007fff
Node[0] Region[2]: 0x0801 - 0x0807
The UEFI loads Xen hypervisor and DTB into the high memory, the kernel
and ramdisk images are loaded into the low memory space:
(XEN) MODULE[0]: 0807f6df - 0807f6f3e000 Xen
(XEN) MODULE[1]: 0807f8054000 - 0807f8056000 Device Tree
(XEN) MODULE[2]: fa834000 - fc5de1d5 Ramdisk
(XEN) MODULE[3]: fc5df000 - ffb3f810 Kernel
In this case, the Xen binary is loaded above 8TB, which exceeds the
maximum supported identity map space of 2TB in Xen. Consequently, the
system fails to boot.
This patch enlarges identity map space to 10TB, allowing module loading
within the range of [0x0 .. 0x09ff__].
Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to prepare/enable/disable")
I don't think a fixes tag applies here given that 2TB was just a number we
believed is enough
and all of this is platform dependent.
This can be dropped on commit if committer agrees
Xen may have booted on that platform before hand. So this would be considered a
regression and therefore a tag would be warrant.
AFAICT, the commit is only present on the upcoming 4.18. So the question is
whether Xen 4.17 booted out-of-the-box on ADLink? If the answer is yes, then we
need to add a Fixes tag. But the correct one would be
@Rahul or Luca: could you give an answer here ?
I know you used Xen on an AVA platform but was it booting out of the box ?
I can’t say for Xen 4.17, but our nightly job has run successfully on AVA for
the commit 730406ab81094115d9fb5ca00ba8d53cec1279b3
(docs/misra: add deviations.rst to document additional deviations.)
We are not applying any patch for it to run on AVA.
Most likely it is because your UEFI/BIOS firmware is 2.x, for instance
2.04.100.07.
This fix if for AVA machine with older UEFI firmware 1.07.300.03.
OOI, why not updating your firmware? I was expecting that it would also
contain other critical fixes.
With this in mind, I am now more in two mind to ask to merge this patch
in Xen 4.18. On one hand, I understand it will help to boot on AVA
machine with an older firmware. But on the other hand this is changing
the memory layout quite late in the release. The risk seems limited
because Xen is not loaded at the top of the virtual address space (there
is the directmap afterwards).
Henry (as the release manager) and others, any opinions?
Cheers,
--
Julien Grall
Re: [PATCH v2 2/2] domain: expose newly introduced hypercalls as XENFEAT
On Mon, Oct 16, 2023 at 04:04:34PM +0200, Jan Beulich wrote:
> On 16.10.2023 16:01, Roger Pau Monné wrote:
> > On Mon, Oct 16, 2023 at 03:58:22PM +0200, Jan Beulich wrote:
> >> On 16.10.2023 15:00, Roger Pau Monné wrote:
> >>> On Mon, Oct 16, 2023 at 02:35:44PM +0200, Jan Beulich wrote:
> On 06.10.2023 15:00, Roger Pau Monne wrote:
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -1998,6 +1998,10 @@ long common_vcpu_op(int cmd, struct vcpu *v,
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> > {
> > struct vcpu_register_runstate_memory_area area;
> >
> > +rc = -ENOSYS;
> > +if ( 0 /* TODO: Dom's XENFEAT_runstate_phys_area setting */ )
> > +break;
> > +
> > rc = -EFAULT;
> > if ( copy_from_guest(, arg, 1) )
> > break;
>
> ENOSYS is not correct here. EPERM, EACCES, or EOPNOTSUPP would all be
> more
> correct.
> >>>
> >>> I don't think so, common_vcpu_op() default case does return -ENOSYS,
> >>> and the point of this path is to mimic the behavior of an hypervisor
> >>> that doesn't have the hypercalls implemented, hence -ENOSYS is the
> >>> correct behavior.
> >>
> >> Besides that other ENOSYS being wrong too, I question such "mimic-ing".
> >> Imo error codes should be the best representation of the real reason,
> >> not be arbitrarily "made up".
> >
> > The point is for the guest to not take any action that it won't take
> > on an hypervisor that doesn't have the hypercalls implemented, and the
> > only way to be sure about that is to return the same exact error code.
>
> I don't follow this kind of reasoning. Why would a guest, whose code to
> use the new sub-functions has to be newly written, key its decision to
> the specific error code it gets, when at the same time you expose
> feature bits that it can use to decide whether to invoke the hypercall
> in the first place.
Because we don't control all possible guest implementations out there.
AIUI the point of introducing a way to disable the newly added
hypercalls is to make the interface between a Xen previous to the
introduction of the hypercalls vs a Xen that has the hypercalls
disabled equal, and that requires returning the same error code IMO,
or else interfaces are not equal.
Thanks, Roger.
Re: [XEN PATCH][for-next v2 7/7] x86/mem_access: make function static
On 09.10.2023 08:54, Nicola Vetrini wrote: > --- a/xen/arch/x86/mm/mem_access.c > +++ b/xen/arch/x86/mm/mem_access.c > @@ -249,7 +249,7 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla, > return (p2ma != p2m_access_n2rwx); > } > > -int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, > +static int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain > *hp2m, >struct p2m_domain *ap2m, p2m_access_t a, >gfn_t gfn) As mentioned before, when adding static (or whatever else) like this, the next line(s) need re-indenting. Jan
Re: [PATCH v4 2/2] xen/arm: Enlarge identity map space to 10TB
>>> >>> @Rahul or Luca: could you give an answer here ? >>> I know you used Xen on an AVA platform but was it booting out of the box ? >> >> I can’t say for Xen 4.17, but our nightly job has run successfully on AVA >> for the commit 730406ab81094115d9fb5ca00ba8d53cec1279b3 >> (docs/misra: add deviations.rst to document additional deviations.) >> >> We are not applying any patch for it to run on AVA. > > Most likely it is because your UEFI/BIOS firmware is 2.x, for instance > 2.04.100.07. > This fix if for AVA machine with older UEFI firmware 1.07.300.03. Yes indeed, all our AVA have a newer firmware, so I’m afraid we can’t check Cheers, Luca
Re: [PATCH for-4.19] x86/cpu-policy: Adjust CPUID_MAX_SERIALISED_LEAVES to placate MISRA
On 10.10.2023 11:57, Andrew Cooper wrote: > MISRA doesn't like !!CONST being used in place of a 1 (Rule 10.1). Update the > expression to just be a plain 1, which still matches the description. > > No functional change. > > Reported-by: Nicola Vetrini > Signed-off-by: Andrew Cooper Acked-by: Jan Beulich
Re: [PATCH v4 2/2] xen/arm: Enlarge identity map space to 10TB
On Mon, 16 Oct 2023 at 15:13, Luca Fancellu wrote:
>
>
>
> > On 16 Oct 2023, at 15:00, Bertrand Marquis wrote:
> >
> > Hi
> >
> > +Luca and Rahul
> >
> >> On 16 Oct 2023, at 15:54, Julien Grall wrote:
> >>
> >>
> >>
> >> On 16/10/2023 09:44, Michal Orzel wrote:
> >>> Hi,
> >>
> >> Hi,
> >>
> >>> On 13/10/2023 14:26, Leo Yan wrote:
>
>
> On ADLink AVA platform (Ampere Altra SoC with 32 Arm Neoverse N1 cores),
> the physical memory regions are:
>
> DRAM memory regions:
> Node[0] Region[0]: 0x8000 - 0x
> Node[0] Region[1]: 0x0800 - 0x08007fff
> Node[0] Region[2]: 0x0801 - 0x0807
>
> The UEFI loads Xen hypervisor and DTB into the high memory, the kernel
> and ramdisk images are loaded into the low memory space:
>
> (XEN) MODULE[0]: 0807f6df - 0807f6f3e000 Xen
> (XEN) MODULE[1]: 0807f8054000 - 0807f8056000 Device Tree
> (XEN) MODULE[2]: fa834000 - fc5de1d5 Ramdisk
> (XEN) MODULE[3]: fc5df000 - ffb3f810 Kernel
>
> In this case, the Xen binary is loaded above 8TB, which exceeds the
> maximum supported identity map space of 2TB in Xen. Consequently, the
> system fails to boot.
>
> This patch enlarges identity map space to 10TB, allowing module loading
> within the range of [0x0 .. 0x09ff__].
>
> Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to
> prepare/enable/disable")
> >>> I don't think a fixes tag applies here given that 2TB was just a number
> >>> we believed is enough
> >>> and all of this is platform dependent.
> >>> This can be dropped on commit if committer agrees
> >> Xen may have booted on that platform before hand. So this would be
> >> considered a regression and therefore a tag would be warrant.
> >>
> >> AFAICT, the commit is only present on the upcoming 4.18. So the question
> >> is whether Xen 4.17 booted out-of-the-box on ADLink? If the answer is yes,
> >> then we need to add a Fixes tag. But the correct one would be
> >>
> >
> > @Rahul or Luca: could you give an answer here ?
> > I know you used Xen on an AVA platform but was it booting out of the box ?
>
> I can’t say for Xen 4.17, but our nightly job has run successfully on AVA for
> the commit 730406ab81094115d9fb5ca00ba8d53cec1279b3
> (docs/misra: add deviations.rst to document additional deviations.)
>
> We are not applying any patch for it to run on AVA.
Most likely it is because your UEFI/BIOS firmware is 2.x, for instance
2.04.100.07.
This fix if for AVA machine with older UEFI firmware 1.07.300.03.
Best regards,
Alexey
Re: [PATCH v4 2/2] xen/arm: Enlarge identity map space to 10TB
> On 16 Oct 2023, at 15:00, Bertrand Marquis wrote:
>
> Hi
>
> +Luca and Rahul
>
>> On 16 Oct 2023, at 15:54, Julien Grall wrote:
>>
>>
>>
>> On 16/10/2023 09:44, Michal Orzel wrote:
>>> Hi,
>>
>> Hi,
>>
>>> On 13/10/2023 14:26, Leo Yan wrote:
On ADLink AVA platform (Ampere Altra SoC with 32 Arm Neoverse N1 cores),
the physical memory regions are:
DRAM memory regions:
Node[0] Region[0]: 0x8000 - 0x
Node[0] Region[1]: 0x0800 - 0x08007fff
Node[0] Region[2]: 0x0801 - 0x0807
The UEFI loads Xen hypervisor and DTB into the high memory, the kernel
and ramdisk images are loaded into the low memory space:
(XEN) MODULE[0]: 0807f6df - 0807f6f3e000 Xen
(XEN) MODULE[1]: 0807f8054000 - 0807f8056000 Device Tree
(XEN) MODULE[2]: fa834000 - fc5de1d5 Ramdisk
(XEN) MODULE[3]: fc5df000 - ffb3f810 Kernel
In this case, the Xen binary is loaded above 8TB, which exceeds the
maximum supported identity map space of 2TB in Xen. Consequently, the
system fails to boot.
This patch enlarges identity map space to 10TB, allowing module loading
within the range of [0x0 .. 0x09ff__].
Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to
prepare/enable/disable")
>>> I don't think a fixes tag applies here given that 2TB was just a number we
>>> believed is enough
>>> and all of this is platform dependent.
>>> This can be dropped on commit if committer agrees
>> Xen may have booted on that platform before hand. So this would be
>> considered a regression and therefore a tag would be warrant.
>>
>> AFAICT, the commit is only present on the upcoming 4.18. So the question is
>> whether Xen 4.17 booted out-of-the-box on ADLink? If the answer is yes, then
>> we need to add a Fixes tag. But the correct one would be
>>
>
> @Rahul or Luca: could you give an answer here ?
> I know you used Xen on an AVA platform but was it booting out of the box ?
I can’t say for Xen 4.17, but our nightly job has run successfully on AVA for
the commit 730406ab81094115d9fb5ca00ba8d53cec1279b3
(docs/misra: add deviations.rst to document additional deviations.)
We are not applying any patch for it to run on AVA.
>
>> 1c78d76b67e1 ("xen/arm64: mm: Introduce helpers to prepare/enable/disable
>> the identity mapping").
>>
>> We would also need to consider it as a candidate for Xen 4.18 because we
>> would regress boot on ADLink.
>
> Ack
>
> Cheers
> Bertrand
>
>>
>> Cheers,
>>
>> --
>> Julien Grall
>
>
Re: [PATCH for-4.18 v2] x86/pvh: fix identity mapping of low 1MB
On 16.10.2023 15:51, Roger Pau Monné wrote:
> On Mon, Oct 16, 2023 at 03:32:54PM +0200, Jan Beulich wrote:
>> On 13.10.2023 10:56, Roger Pau Monne wrote:
>>> The mapping of memory regions below the 1MB mark was all done by the PVH
>>> dom0
>>> builder code, causing the region to be avoided by the arch specific IOMMU
>>> hardware domain initialization code. That lead to the IOMMU being enabled
>>> without reserved regions in the low 1MB identity mapped in the p2m for PVH
>>> hardware domains. Firmware which happens to be missing RMRR/IVMD ranges
>>> describing E820 reserved regions in the low 1MB would transiently trigger
>>> IOMMU
>>> faults until the p2m is populated by the PVH dom0 builder:
>>>
>>> AMD-Vi: IO_PAGE_FAULT: :00:13.1 d0 addr 000eb380 flags 0x20 RW
>>> AMD-Vi: IO_PAGE_FAULT: :00:13.1 d0 addr 000eb340 flags 0
>>> AMD-Vi: IO_PAGE_FAULT: :00:13.2 d0 addr 000ea1c0 flags 0
>>> AMD-Vi: IO_PAGE_FAULT: :00:14.5 d0 addr 000eb480 flags 0x20 RW
>>> AMD-Vi: IO_PAGE_FAULT: :00:12.0 d0 addr 000eb080 flags 0x20 RW
>>> AMD-Vi: IO_PAGE_FAULT: :00:14.5 d0 addr 000eb400 flags 0
>>> AMD-Vi: IO_PAGE_FAULT: :00:12.0 d0 addr 000eb040 flags 0
>>>
>>> Those errors have been observed on the osstest pinot{0,1} boxes (AMD Fam15h
>>> Opteron(tm) Processor 3350 HE).
>>>
>>> Mostly remove the special handling of the low 1MB done by the PVH dom0
>>> builder,
>>> leaving just the data copy between RAM regions. Otherwise rely on the IOMMU
>>> arch init code to create any identity mappings for reserved regions in that
>>> range (like it already does for reserved regions elsewhere).
>>>
>>> Note there's a small difference in behavior, as holes in the low 1MB will no
>>> longer be identity mapped to the p2m.
>>
>> I certainly like the simplification, but I'm concerned by this: The BDA
>> is not normally reserved, yet may want accessing by Dom0 (to see the real
>> machine contents). We do access that first page of memory ourselves, so
>> I expect OSes may do so as well (even if the specific aspect I'm thinking
>> of - the warm/cold reboot field - is under Xen's control).
>
> The BDA on the systems I've checked falls into a RAM area on the
> memory map, but if you think it can be problematic I could arrange for
> arch_iommu_hwdom_init() to also identity map holes in the low 1MB.
Hmm, this again is a case where I'd wish CPU and IOMMU mappings could
be different. I don't see reasons to try I/O to such holes, but I can
see reasons for CPU accesses (of more or less probing kind).
> Keep in mind this is only for PVH, it won't affect PV.
Of course.
Jan
Re: [PATCH v4 2/2] xen/arm: Enlarge identity map space to 10TB
On Mon, 16 Oct 2023 at 14:54, Julien Grall wrote:
>
>
>
> On 16/10/2023 09:44, Michal Orzel wrote:
> > Hi,
>
> Hi,
>
> > On 13/10/2023 14:26, Leo Yan wrote:
> >>
> >>
> >> On ADLink AVA platform (Ampere Altra SoC with 32 Arm Neoverse N1 cores),
> >> the physical memory regions are:
> >>
> >>DRAM memory regions:
> >> Node[0] Region[0]: 0x8000 - 0x
> >> Node[0] Region[1]: 0x0800 - 0x08007fff
> >> Node[0] Region[2]: 0x0801 - 0x0807
> >>
> >> The UEFI loads Xen hypervisor and DTB into the high memory, the kernel
> >> and ramdisk images are loaded into the low memory space:
> >>
> >>(XEN) MODULE[0]: 0807f6df - 0807f6f3e000 Xen
> >>(XEN) MODULE[1]: 0807f8054000 - 0807f8056000 Device Tree
> >>(XEN) MODULE[2]: fa834000 - fc5de1d5 Ramdisk
> >>(XEN) MODULE[3]: fc5df000 - ffb3f810 Kernel
> >>
> >> In this case, the Xen binary is loaded above 8TB, which exceeds the
> >> maximum supported identity map space of 2TB in Xen. Consequently, the
> >> system fails to boot.
> >>
> >> This patch enlarges identity map space to 10TB, allowing module loading
> >> within the range of [0x0 .. 0x09ff__].
> >>
> >> Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to
> >> prepare/enable/disable")
> > I don't think a fixes tag applies here given that 2TB was just a number we
> > believed is enough
> > and all of this is platform dependent.
> > This can be dropped on commit if committer agrees
> Xen may have booted on that platform before hand. So this would be
> considered a regression and therefore a tag would be warrant.
>
> AFAICT, the commit is only present on the upcoming 4.18. So the question
> is whether Xen 4.17 booted out-of-the-box on ADLink? If the answer is
> yes, then we need to add a Fixes tag. But the correct one would be
Yes. The upstream xen 4.17 booted fine and the mentioned commit was
found during bisect, so it is introduced regression.
I'd personally say that "Fixes" tag is needed here.
Thanks,
Alexey
Re: [PATCH v2 2/2] domain: expose newly introduced hypercalls as XENFEAT
On 16.10.2023 16:01, Roger Pau Monné wrote:
> On Mon, Oct 16, 2023 at 03:58:22PM +0200, Jan Beulich wrote:
>> On 16.10.2023 15:00, Roger Pau Monné wrote:
>>> On Mon, Oct 16, 2023 at 02:35:44PM +0200, Jan Beulich wrote:
On 06.10.2023 15:00, Roger Pau Monne wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1998,6 +1998,10 @@ long common_vcpu_op(int cmd, struct vcpu *v,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> {
> struct vcpu_register_runstate_memory_area area;
>
> +rc = -ENOSYS;
> +if ( 0 /* TODO: Dom's XENFEAT_runstate_phys_area setting */ )
> +break;
> +
> rc = -EFAULT;
> if ( copy_from_guest(, arg, 1) )
> break;
ENOSYS is not correct here. EPERM, EACCES, or EOPNOTSUPP would all be more
correct.
>>>
>>> I don't think so, common_vcpu_op() default case does return -ENOSYS,
>>> and the point of this path is to mimic the behavior of an hypervisor
>>> that doesn't have the hypercalls implemented, hence -ENOSYS is the
>>> correct behavior.
>>
>> Besides that other ENOSYS being wrong too, I question such "mimic-ing".
>> Imo error codes should be the best representation of the real reason,
>> not be arbitrarily "made up".
>
> The point is for the guest to not take any action that it won't take
> on an hypervisor that doesn't have the hypercalls implemented, and the
> only way to be sure about that is to return the same exact error code.
I don't follow this kind of reasoning. Why would a guest, whose code to
use the new sub-functions has to be newly written, key its decision to
the specific error code it gets, when at the same time you expose
feature bits that it can use to decide whether to invoke the hypercall
in the first place.
Jan
Re: [PATCH for-4.18 v5] xen/pdx: Make CONFIG_PDX_COMPRESSION a common Kconfig option
On 16.10.2023 15:40, Roger Pau Monné wrote: > On Mon, Oct 16, 2023 at 03:19:22PM +0200, Jan Beulich wrote: >> On 06.10.2023 17:27, Roger Pau Monné wrote: >>> On Fri, Oct 06, 2023 at 04:09:19PM +0100, Julien Grall wrote: On 06/10/2023 15:44, Andrew Cooper wrote: > From: Alejandro Vallejo > > Adds a new compile-time flag to allow disabling PDX compression and > compiles out compression-related code/data. It also shorts the pdx<->pfn > conversion macros and creates stubs for masking functions. > > While at it, removes the old arch-defined CONFIG_HAS_PDX flag. Despite > the > illusion of choice, it was not optional. > > There are ARM and PPC platforms with sparse RAM banks - leave compression > active by default there. OTOH, there are no known production x86 systems > with > sparse RAM banks, so disable compression. This decision can be revisited > if > such a platform comes along. (Process remarks rather than the code itself) Jan is away this week so I want to make sure this doesn't go in without him having a say. While I don't particularly care about the approach taken for x86, Jan voiced concerned with this approach and so far I didn't see any conclusion. If there is any, then please point me to them. For the record, the objections from Jan are in [1]. If we want to ignore them, then I think we need a vote. Possibly only from the x86 folks (?). >>> >>> I would be fine in leaving the option to be selected if we knew that >>> such x86 systems might be feasible, but so far we have seen 0 x86 >>> systems with sparse RAM. That said, I don't have a strong opinion, but >>> the hiding on x86 seems fine to me. Interested parties can always >>> forcefully select the option, and a case can be made to make it >>> available again on Kconfig. >> >> I find it odd to demand people to change source code for aspects like >> this. The very least I'd expect is that BIGMEM configurations (which >> I've never seen any production use of) can actually also engage PDX. > > So we expect BIGMEM to have sparse RAM regions? I would have expected > systems with >16TB of RAM to still be contiguous. Well, the system kind I did the original work for were sparse for the purpose of allowing huge hotplug areas which would then be contiguous with the non-hotplugged memory on the same nodes. That said, me mentioning BIGMEM was merely yet another step in trying to find some compromise with Andrew. As pointed out before I'd really expect that finding compromises doesn't really mean only one side moves, yet here and elsewhere I can't help getting the impression that this is what's expected (of me). Jan
Re: [PATCH v2 2/2] domain: expose newly introduced hypercalls as XENFEAT
On Mon, Oct 16, 2023 at 03:58:22PM +0200, Jan Beulich wrote:
> On 16.10.2023 15:00, Roger Pau Monné wrote:
> > On Mon, Oct 16, 2023 at 02:35:44PM +0200, Jan Beulich wrote:
> >> On 06.10.2023 15:00, Roger Pau Monne wrote:
> >>> --- a/xen/common/domain.c
> >>> +++ b/xen/common/domain.c
> >>> @@ -1998,6 +1998,10 @@ long common_vcpu_op(int cmd, struct vcpu *v,
> >>> XEN_GUEST_HANDLE_PARAM(void) arg)
> >>> {
> >>> struct vcpu_register_runstate_memory_area area;
> >>>
> >>> +rc = -ENOSYS;
> >>> +if ( 0 /* TODO: Dom's XENFEAT_runstate_phys_area setting */ )
> >>> +break;
> >>> +
> >>> rc = -EFAULT;
> >>> if ( copy_from_guest(, arg, 1) )
> >>> break;
> >>
> >> ENOSYS is not correct here. EPERM, EACCES, or EOPNOTSUPP would all be more
> >> correct.
> >
> > I don't think so, common_vcpu_op() default case does return -ENOSYS,
> > and the point of this path is to mimic the behavior of an hypervisor
> > that doesn't have the hypercalls implemented, hence -ENOSYS is the
> > correct behavior.
>
> Besides that other ENOSYS being wrong too, I question such "mimic-ing".
> Imo error codes should be the best representation of the real reason,
> not be arbitrarily "made up".
The point is for the guest to not take any action that it won't take
on an hypervisor that doesn't have the hypercalls implemented, and the
only way to be sure about that is to return the same exact error code.
Thanks, Roger.
Re: [PATCH v4 2/2] xen/arm: Enlarge identity map space to 10TB
Hi
+Luca and Rahul
> On 16 Oct 2023, at 15:54, Julien Grall wrote:
>
>
>
> On 16/10/2023 09:44, Michal Orzel wrote:
>> Hi,
>
> Hi,
>
>> On 13/10/2023 14:26, Leo Yan wrote:
>>>
>>>
>>> On ADLink AVA platform (Ampere Altra SoC with 32 Arm Neoverse N1 cores),
>>> the physical memory regions are:
>>>
>>> DRAM memory regions:
>>> Node[0] Region[0]: 0x8000 - 0x
>>> Node[0] Region[1]: 0x0800 - 0x08007fff
>>> Node[0] Region[2]: 0x0801 - 0x0807
>>>
>>> The UEFI loads Xen hypervisor and DTB into the high memory, the kernel
>>> and ramdisk images are loaded into the low memory space:
>>>
>>> (XEN) MODULE[0]: 0807f6df - 0807f6f3e000 Xen
>>> (XEN) MODULE[1]: 0807f8054000 - 0807f8056000 Device Tree
>>> (XEN) MODULE[2]: fa834000 - fc5de1d5 Ramdisk
>>> (XEN) MODULE[3]: fc5df000 - ffb3f810 Kernel
>>>
>>> In this case, the Xen binary is loaded above 8TB, which exceeds the
>>> maximum supported identity map space of 2TB in Xen. Consequently, the
>>> system fails to boot.
>>>
>>> This patch enlarges identity map space to 10TB, allowing module loading
>>> within the range of [0x0 .. 0x09ff__].
>>>
>>> Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to
>>> prepare/enable/disable")
>> I don't think a fixes tag applies here given that 2TB was just a number we
>> believed is enough
>> and all of this is platform dependent.
>> This can be dropped on commit if committer agrees
> Xen may have booted on that platform before hand. So this would be considered
> a regression and therefore a tag would be warrant.
>
> AFAICT, the commit is only present on the upcoming 4.18. So the question is
> whether Xen 4.17 booted out-of-the-box on ADLink? If the answer is yes, then
> we need to add a Fixes tag. But the correct one would be
>
@Rahul or Luca: could you give an answer here ?
I know you used Xen on an AVA platform but was it booting out of the box ?
> 1c78d76b67e1 ("xen/arm64: mm: Introduce helpers to prepare/enable/disable
> the identity mapping").
>
> We would also need to consider it as a candidate for Xen 4.18 because we
> would regress boot on ADLink.
Ack
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
Re: [PATCH v2 2/2] domain: expose newly introduced hypercalls as XENFEAT
On 16.10.2023 15:00, Roger Pau Monné wrote:
> On Mon, Oct 16, 2023 at 02:35:44PM +0200, Jan Beulich wrote:
>> On 06.10.2023 15:00, Roger Pau Monne wrote:
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -1998,6 +1998,10 @@ long common_vcpu_op(int cmd, struct vcpu *v,
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>> {
>>> struct vcpu_register_runstate_memory_area area;
>>>
>>> +rc = -ENOSYS;
>>> +if ( 0 /* TODO: Dom's XENFEAT_runstate_phys_area setting */ )
>>> +break;
>>> +
>>> rc = -EFAULT;
>>> if ( copy_from_guest(, arg, 1) )
>>> break;
>>
>> ENOSYS is not correct here. EPERM, EACCES, or EOPNOTSUPP would all be more
>> correct.
>
> I don't think so, common_vcpu_op() default case does return -ENOSYS,
> and the point of this path is to mimic the behavior of an hypervisor
> that doesn't have the hypercalls implemented, hence -ENOSYS is the
> correct behavior.
Besides that other ENOSYS being wrong too, I question such "mimic-ing".
Imo error codes should be the best representation of the real reason,
not be arbitrarily "made up".
Jan
Re: [PATCH v4 2/2] xen/arm: Enlarge identity map space to 10TB
On Mon, Oct 16, 2023 at 01:40:26PM +, Bertrand Marquis wrote:
[...]
> > This patch enlarges identity map space to 10TB, allowing module loading
> > within the range of [0x0 .. 0x09ff__].
> >
> > Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to
> > prepare/enable/disable")
>
> I agree with Michal here, this is not a fix so this should be removed (can be
> done
> on commit).
This is fine for me.
I'd like to confirm with maintainers that should I spin a new patch
set to remove the fix tag? Or maintainers could help to remove it
when pick up this patch set.
And thanks for review, Michal and Bertrand.
Leo
> > Reported-by: Alexey Klimov
> > Signed-off-by: Leo Yan
>
> Reviewed-by: Bertrand Marquis
>
> Cheers
> Bertrand
Re: [PATCH v4 2/2] xen/arm: Enlarge identity map space to 10TB
On 16/10/2023 09:44, Michal Orzel wrote:
Hi,
Hi,
On 13/10/2023 14:26, Leo Yan wrote:
On ADLink AVA platform (Ampere Altra SoC with 32 Arm Neoverse N1 cores),
the physical memory regions are:
DRAM memory regions:
Node[0] Region[0]: 0x8000 - 0x
Node[0] Region[1]: 0x0800 - 0x08007fff
Node[0] Region[2]: 0x0801 - 0x0807
The UEFI loads Xen hypervisor and DTB into the high memory, the kernel
and ramdisk images are loaded into the low memory space:
(XEN) MODULE[0]: 0807f6df - 0807f6f3e000 Xen
(XEN) MODULE[1]: 0807f8054000 - 0807f8056000 Device Tree
(XEN) MODULE[2]: fa834000 - fc5de1d5 Ramdisk
(XEN) MODULE[3]: fc5df000 - ffb3f810 Kernel
In this case, the Xen binary is loaded above 8TB, which exceeds the
maximum supported identity map space of 2TB in Xen. Consequently, the
system fails to boot.
This patch enlarges identity map space to 10TB, allowing module loading
within the range of [0x0 .. 0x09ff__].
Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to prepare/enable/disable")
I don't think a fixes tag applies here given that 2TB was just a number we
believed is enough
and all of this is platform dependent.
This can be dropped on commit if committer agrees
Xen may have booted on that platform before hand. So this would be
considered a regression and therefore a tag would be warrant.
AFAICT, the commit is only present on the upcoming 4.18. So the question
is whether Xen 4.17 booted out-of-the-box on ADLink? If the answer is
yes, then we need to add a Fixes tag. But the correct one would be
1c78d76b67e1 ("xen/arm64: mm: Introduce helpers to
prepare/enable/disable the identity mapping").
We would also need to consider it as a candidate for Xen 4.18 because we
would regress boot on ADLink.
Cheers,
--
Julien Grall
Re: [PATCH for-4.18 v2] x86/pvh: fix identity mapping of low 1MB
On Mon, Oct 16, 2023 at 03:32:54PM +0200, Jan Beulich wrote:
> On 13.10.2023 10:56, Roger Pau Monne wrote:
> > The mapping of memory regions below the 1MB mark was all done by the PVH
> > dom0
> > builder code, causing the region to be avoided by the arch specific IOMMU
> > hardware domain initialization code. That lead to the IOMMU being enabled
> > without reserved regions in the low 1MB identity mapped in the p2m for PVH
> > hardware domains. Firmware which happens to be missing RMRR/IVMD ranges
> > describing E820 reserved regions in the low 1MB would transiently trigger
> > IOMMU
> > faults until the p2m is populated by the PVH dom0 builder:
> >
> > AMD-Vi: IO_PAGE_FAULT: :00:13.1 d0 addr 000eb380 flags 0x20 RW
> > AMD-Vi: IO_PAGE_FAULT: :00:13.1 d0 addr 000eb340 flags 0
> > AMD-Vi: IO_PAGE_FAULT: :00:13.2 d0 addr 000ea1c0 flags 0
> > AMD-Vi: IO_PAGE_FAULT: :00:14.5 d0 addr 000eb480 flags 0x20 RW
> > AMD-Vi: IO_PAGE_FAULT: :00:12.0 d0 addr 000eb080 flags 0x20 RW
> > AMD-Vi: IO_PAGE_FAULT: :00:14.5 d0 addr 000eb400 flags 0
> > AMD-Vi: IO_PAGE_FAULT: :00:12.0 d0 addr 000eb040 flags 0
> >
> > Those errors have been observed on the osstest pinot{0,1} boxes (AMD Fam15h
> > Opteron(tm) Processor 3350 HE).
> >
> > Mostly remove the special handling of the low 1MB done by the PVH dom0
> > builder,
> > leaving just the data copy between RAM regions. Otherwise rely on the IOMMU
> > arch init code to create any identity mappings for reserved regions in that
> > range (like it already does for reserved regions elsewhere).
> >
> > Note there's a small difference in behavior, as holes in the low 1MB will no
> > longer be identity mapped to the p2m.
>
> I certainly like the simplification, but I'm concerned by this: The BDA
> is not normally reserved, yet may want accessing by Dom0 (to see the real
> machine contents). We do access that first page of memory ourselves, so
> I expect OSes may do so as well (even if the specific aspect I'm thinking
> of - the warm/cold reboot field - is under Xen's control).
The BDA on the systems I've checked falls into a RAM area on the
memory map, but if you think it can be problematic I could arrange for
arch_iommu_hwdom_init() to also identity map holes in the low 1MB.
Keep in mind this is only for PVH, it won't affect PV.
Thanks, Roger.
Re: [XEN PATCH][for-4.19 v2 1/1] xen: introduce a deviation for Rule 11.9
On 11.10.2023 18:56, andrew.coop...@citrix.com wrote: > On 11/10/2023 8:46 pm, Nicola Vetrini wrote: >> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst >> index ee7aed0609d2..1b00e4e3e9b7 100644 >> --- a/docs/misra/deviations.rst >> +++ b/docs/misra/deviations.rst >> @@ -199,6 +199,11 @@ Deviations related to MISRA C:2012 Rules: >> See automation/eclair_analysis/deviations.ecl for the full >> explanation. >> - Tagged as `safe` for ECLAIR. >> >> + * - R11.9 >> + - __ACCESS_ONCE uses a 0 as a null pointer constant to check if a type >> is >> + scalar, therefore its usage for this purpose is allowed. >> + - Tagged as `deliberate` for ECLAIR. > > Really? > > #define __ACCESS_ONCE(x) > (void)(typeof(x))0; /* Scalar typecheck. */ > > That's not a pointer. > > If someone were to pass in an x who's type was pointer-to-object, then > yes it is technically a NULL pointer constant for long enough for the > build to error out. Why would it error out? ACCESS_ONCE() on a variable of pointer type is as okay as one on a variable of (some kind of) integer type. (Sadly the check as is would even let floating point types pass. At the same time the check is too strict to allow e.g. single-machine-word struct/union to also pass.) Jan
Re: [XEN PATCH 10/10] arm/smmu: address violation of MISRA C:2012 Rule 8.2
Hi Julien,
> On 16 Oct 2023, at 15:38, Julien Grall wrote:
>
>
>
> On 16/10/2023 14:31, Bertrand Marquis wrote:
>> Hi Julien,
>
> Hi Bertrand,
>
>>> On 16 Oct 2023, at 11:07, Julien Grall wrote:
>>>
>>> Hi,
>>>
>>> On 13/10/2023 16:24, Federico Serafini wrote:
Add missing parameter names, no functional change.
Signed-off-by: Federico Serafini
---
xen/drivers/passthrough/arm/smmu.c | 6 +++---
>>>
>>> This file is using the Linux coding style because it is imported from
>>> Linux. I was under the impression we would exclude such file for now.
>>>
>>> Looking at exclude-list.json, it doesn't seem to be present. I think this
>>> patch should be replaced with adding a line in execlude-list.json.
>> I think that during one of the discussions we said that this file already
>> deviated quite a lot from the status in Linux and we wanted to turn it to
>> Xen coding style in the future hence it is not listed in the exclude file.
> AFAIK the SMMUv{1, 2} code didn't deviate very much from Linux. I can't tell
> about the SMMUv3.
True that i had the v3 code in mind, we might not want to do that for v1/2 you
are right.
>
>> At the end having a working smmu might be critical in a safety context so it
>> will make sense to also check this part of xen.
> I don't buy this argument right now. We have files in exclude-lists.json that
> I would consider critical for Xen to run (e.g. common/bitmap.c,
> common/libfdt). So if you want to use this argument then we need to move
> critical components of Xen out of the exclusion list.
I am sure we will at some point for runtime code but we cannot do everything on
the first shot.
I was not meaning to do that now but that it is something to consider.
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
Re: [XEN PATCH][for-4.19 v2 1/1] xen: introduce a deviation for Rule 11.9
On 11.10.2023 14:46, Nicola Vetrini wrote: > --- a/xen/include/xen/compiler.h > +++ b/xen/include/xen/compiler.h > @@ -109,13 +109,16 @@ > > #define offsetof(a,b) __builtin_offsetof(a,b) > > +/* Access the field of structure type, without defining a local variable */ > +#define access_field(type, member) (((type *)NULL)->member) This is not a field access, so I consider the macro misnamed. Question is whether such a helper macro is needed in the first place. > +#define typeof_field(type, member) typeof(access_field(type, member)) If this needs adding, it wants to come ... > /** > * sizeof_field(TYPE, MEMBER) > * > * @TYPE: The structure containing the field of interest > * @MEMBER: The field to return the size of > */ > -#define sizeof_field(TYPE, MEMBER) sizeofTYPE *)0)->MEMBER)) > +#define sizeof_field(TYPE, MEMBER) sizeof(access_field(TYPE, MEMBER)) ... with a commend similar as this one has. (Or the commend could be slightly altered to cover both). Further, if fiddling with these: Wouldn't they better move to macros.h? Jan
Re: [PATCH v4 2/2] xen/arm: Enlarge identity map space to 10TB
Hi,
> On 13 Oct 2023, at 14:26, Leo Yan wrote:
>
> On ADLink AVA platform (Ampere Altra SoC with 32 Arm Neoverse N1 cores),
> the physical memory regions are:
>
> DRAM memory regions:
>Node[0] Region[0]: 0x8000 - 0x
>Node[0] Region[1]: 0x0800 - 0x08007fff
>Node[0] Region[2]: 0x0801 - 0x0807
>
> The UEFI loads Xen hypervisor and DTB into the high memory, the kernel
> and ramdisk images are loaded into the low memory space:
>
> (XEN) MODULE[0]: 0807f6df - 0807f6f3e000 Xen
> (XEN) MODULE[1]: 0807f8054000 - 0807f8056000 Device Tree
> (XEN) MODULE[2]: fa834000 - fc5de1d5 Ramdisk
> (XEN) MODULE[3]: fc5df000 - ffb3f810 Kernel
>
> In this case, the Xen binary is loaded above 8TB, which exceeds the
> maximum supported identity map space of 2TB in Xen. Consequently, the
> system fails to boot.
>
> This patch enlarges identity map space to 10TB, allowing module loading
> within the range of [0x0 .. 0x09ff__].
>
> Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to
> prepare/enable/disable")
I agree with Michal here, this is not a fix so this should be removed (can be
done
on commit).
> Reported-by: Alexey Klimov
> Signed-off-by: Leo Yan
Reviewed-by: Bertrand Marquis
Cheers
Bertrand
> ---
> xen/arch/arm/arm64/mm.c | 6 --
> xen/arch/arm/include/asm/mmu/layout.h | 8
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/mm.c b/xen/arch/arm/arm64/mm.c
> index 78b7c7eb00..cb69df0661 100644
> --- a/xen/arch/arm/arm64/mm.c
> +++ b/xen/arch/arm/arm64/mm.c
> @@ -41,7 +41,8 @@ static void __init prepare_boot_identity_mapping(void)
> clear_page(boot_third_id);
>
> if ( id_offsets[0] >= IDENTITY_MAPPING_AREA_NR_L0 )
> -panic("Cannot handle ID mapping above 2TB\n");
> +panic("Cannot handle ID mapping above %uTB\n",
> + IDENTITY_MAPPING_AREA_NR_L0 >> 1);
>
> /* Link first ID table */
> pte = mfn_to_xen_entry(virt_to_mfn(boot_first_id), MT_NORMAL);
> @@ -74,7 +75,8 @@ static void __init prepare_runtime_identity_mapping(void)
> DECLARE_OFFSETS(id_offsets, id_addr);
>
> if ( id_offsets[0] >= IDENTITY_MAPPING_AREA_NR_L0 )
> -panic("Cannot handle ID mapping above 2TB\n");
> +panic("Cannot handle ID mapping above %uTB\n",
> + IDENTITY_MAPPING_AREA_NR_L0 >> 1);
>
> /* Link first ID table */
> pte = pte_of_xenaddr((vaddr_t)xen_first_id);
> diff --git a/xen/arch/arm/include/asm/mmu/layout.h
> b/xen/arch/arm/include/asm/mmu/layout.h
> index 2cb2382fbf..eac7eef885 100644
> --- a/xen/arch/arm/include/asm/mmu/layout.h
> +++ b/xen/arch/arm/include/asm/mmu/layout.h
> @@ -19,11 +19,11 @@
> * 2G - 4G Domheap: on-demand-mapped
> *
> * ARM64 layout:
> - * 0x - 0x01ff (2TB, L0 slots [0..3])
> + * 0x - 0x09ff (10TB, L0 slots [0..19])
> *
> * Reserved to identity map Xen
> *
> - * 0x0200 - 0x027f (512GB, L0 slot [4])
> + * 0x0a00 - 0x0a7f (512GB, L0 slot [20])
> * (Relative offsets)
> * 0 - 2M Unmapped
> * 2M - 10M Xen text, data, bss
> @@ -35,7 +35,7 @@
> *
> * 32G - 64G Frametable: 56 bytes per page for 2TB of RAM
> *
> - * 0x0280 - 0x7fff (125TB, L0 slots [5..255])
> + * 0x0a80 - 0x7fff (512GB+117TB, L0 slots [21..255])
> * Unused
> *
> * 0x8000 - 0x84ff (5TB, L0 slots [256..265])
> @@ -49,7 +49,7 @@
> #define XEN_VIRT_START _AT(vaddr_t, MB(2))
> #else
>
> -#define IDENTITY_MAPPING_AREA_NR_L0 4
> +#define IDENTITY_MAPPING_AREA_NR_L0 20
> #define XEN_VM_MAPPING SLOT0(IDENTITY_MAPPING_AREA_NR_L0)
>
> #define SLOT0_ENTRY_BITS 39
> --
> 2.39.2
>
Re: [PATCH for-4.18 v5] xen/pdx: Make CONFIG_PDX_COMPRESSION a common Kconfig option
On Mon, Oct 16, 2023 at 03:19:22PM +0200, Jan Beulich wrote: > On 06.10.2023 17:27, Roger Pau Monné wrote: > > On Fri, Oct 06, 2023 at 04:09:19PM +0100, Julien Grall wrote: > >> On 06/10/2023 15:44, Andrew Cooper wrote: > >>> From: Alejandro Vallejo > >>> > >>> Adds a new compile-time flag to allow disabling PDX compression and > >>> compiles out compression-related code/data. It also shorts the pdx<->pfn > >>> conversion macros and creates stubs for masking functions. > >>> > >>> While at it, removes the old arch-defined CONFIG_HAS_PDX flag. Despite > >>> the > >>> illusion of choice, it was not optional. > >>> > >>> There are ARM and PPC platforms with sparse RAM banks - leave compression > >>> active by default there. OTOH, there are no known production x86 systems > >>> with > >>> sparse RAM banks, so disable compression. This decision can be revisited > >>> if > >>> such a platform comes along. > >> > >> (Process remarks rather than the code itself) > >> > >> Jan is away this week so I want to make sure this doesn't go in without him > >> having a say. > >> > >> While I don't particularly care about the approach taken for x86, Jan > >> voiced > >> concerned with this approach and so far I didn't see any conclusion. If > >> there is any, then please point me to them. > >> > >> For the record, the objections from Jan are in [1]. If we want to ignore > >> them, then I think we need a vote. Possibly only from the x86 folks (?). > > > > I would be fine in leaving the option to be selected if we knew that > > such x86 systems might be feasible, but so far we have seen 0 x86 > > systems with sparse RAM. That said, I don't have a strong opinion, but > > the hiding on x86 seems fine to me. Interested parties can always > > forcefully select the option, and a case can be made to make it > > available again on Kconfig. > > I find it odd to demand people to change source code for aspects like > this. The very least I'd expect is that BIGMEM configurations (which > I've never seen any production use of) can actually also engage PDX. So we expect BIGMEM to have sparse RAM regions? I would have expected systems with >16TB of RAM to still be contiguous. Thanks, Roger.
Re: [XEN PATCH 10/10] arm/smmu: address violation of MISRA C:2012 Rule 8.2
On 16/10/2023 14:31, Bertrand Marquis wrote:
Hi Julien,
Hi Bertrand,
On 16 Oct 2023, at 11:07, Julien Grall wrote:
Hi,
On 13/10/2023 16:24, Federico Serafini wrote:
Add missing parameter names, no functional change.
Signed-off-by: Federico Serafini
---
xen/drivers/passthrough/arm/smmu.c | 6 +++---
This file is using the Linux coding style because it is imported from Linux. I
was under the impression we would exclude such file for now.
Looking at exclude-list.json, it doesn't seem to be present. I think this patch
should be replaced with adding a line in execlude-list.json.
I think that during one of the discussions we said that this file already
deviated quite a lot from the status in Linux and we wanted to turn it to Xen
coding style in the future hence it is not listed in the exclude file.
AFAIK the SMMUv{1, 2} code didn't deviate very much from Linux. I can't
tell about the SMMUv3.
At the end having a working smmu might be critical in a safety context so it
will make sense to also check this part of xen.
I don't buy this argument right now. We have files in exclude-lists.json
that I would consider critical for Xen to run (e.g. common/bitmap.c,
common/libfdt). So if you want to use this argument then we need to move
critical components of Xen out of the exclusion list.
Cheers,
--
Julien Grall
Re: [PATCH for-4.18 v2] x86/pvh: fix identity mapping of low 1MB
On 13.10.2023 10:56, Roger Pau Monne wrote:
> The mapping of memory regions below the 1MB mark was all done by the PVH dom0
> builder code, causing the region to be avoided by the arch specific IOMMU
> hardware domain initialization code. That lead to the IOMMU being enabled
> without reserved regions in the low 1MB identity mapped in the p2m for PVH
> hardware domains. Firmware which happens to be missing RMRR/IVMD ranges
> describing E820 reserved regions in the low 1MB would transiently trigger
> IOMMU
> faults until the p2m is populated by the PVH dom0 builder:
>
> AMD-Vi: IO_PAGE_FAULT: :00:13.1 d0 addr 000eb380 flags 0x20 RW
> AMD-Vi: IO_PAGE_FAULT: :00:13.1 d0 addr 000eb340 flags 0
> AMD-Vi: IO_PAGE_FAULT: :00:13.2 d0 addr 000ea1c0 flags 0
> AMD-Vi: IO_PAGE_FAULT: :00:14.5 d0 addr 000eb480 flags 0x20 RW
> AMD-Vi: IO_PAGE_FAULT: :00:12.0 d0 addr 000eb080 flags 0x20 RW
> AMD-Vi: IO_PAGE_FAULT: :00:14.5 d0 addr 000eb400 flags 0
> AMD-Vi: IO_PAGE_FAULT: :00:12.0 d0 addr 000eb040 flags 0
>
> Those errors have been observed on the osstest pinot{0,1} boxes (AMD Fam15h
> Opteron(tm) Processor 3350 HE).
>
> Mostly remove the special handling of the low 1MB done by the PVH dom0
> builder,
> leaving just the data copy between RAM regions. Otherwise rely on the IOMMU
> arch init code to create any identity mappings for reserved regions in that
> range (like it already does for reserved regions elsewhere).
>
> Note there's a small difference in behavior, as holes in the low 1MB will no
> longer be identity mapped to the p2m.
I certainly like the simplification, but I'm concerned by this: The BDA
is not normally reserved, yet may want accessing by Dom0 (to see the real
machine contents). We do access that first page of memory ourselves, so
I expect OSes may do so as well (even if the specific aspect I'm thinking
of - the warm/cold reboot field - is under Xen's control).
Jan
Re: [XEN PATCH 10/10] arm/smmu: address violation of MISRA C:2012 Rule 8.2
Hi Julien, > On 16 Oct 2023, at 11:07, Julien Grall wrote: > > Hi, > > On 13/10/2023 16:24, Federico Serafini wrote: >> Add missing parameter names, no functional change. >> Signed-off-by: Federico Serafini >> --- >> xen/drivers/passthrough/arm/smmu.c | 6 +++--- > > This file is using the Linux coding style because it is imported from Linux. > I was under the impression we would exclude such file for now. > > Looking at exclude-list.json, it doesn't seem to be present. I think this > patch should be replaced with adding a line in execlude-list.json. I think that during one of the discussions we said that this file already deviated quite a lot from the status in Linux and we wanted to turn it to Xen coding style in the future hence it is not listed in the exclude file. At the end having a working smmu might be critical in a safety context so it will make sense to also check this part of xen. @Rahul: do you agree ? Cheers Bertrand > > Cheers, > > -- > Julien Grall
Re: [PATCH v3] xen-pciback: Consider INTx disabled when MSI/MSI-X is enabled
On Mon, Oct 16, 2023 at 03:04:36PM +0200, Marek Marczykowski-Górecki wrote:
> On Mon, Oct 16, 2023 at 11:05:10AM +0200, Roger Pau Monné wrote:
> > On Fri, Nov 18, 2022 at 04:49:23PM +0100, Marek Marczykowski-Górecki wrote:
> > > Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until
> > > the table is filled. Then it disables INTx just before clearing MASKALL
> > > bit. Currently this approach is rejected by xen-pciback.
> > > According to the PCIe spec, device cannot use INTx when MSI/MSI-X is
> > > enabled (in other words: enabling MSI/MSI-X implicitly disables INTx).
> > >
> > > Change the logic to consider INTx disabled if MSI/MSI-X is enabled. This
> > > applies to three places:
> > > - checking currently enabled interrupts type,
> > > - transition to MSI/MSI-X - where INTx would be implicitly disabled,
> > > - clearing INTx disable bit - which can be allowed even if MSI/MSI-X is
> > >enabled, as device should consider INTx disabled anyway in that case
> >
> > Is this last point strictly needed? From the description above it
> > seems Linux only cares about enabling MSI(-X) without the disable INTx
> > bit set.
>
> I'm not sure, but it seems logical to have it symmetric.
I don't have a strong opinion, but I would rather err on the cautious
side and just leave it more strict unless explicitly required.
> >
> > >
> > > Fixes: 5e29500eba2a ("xen-pciback: Allow setting PCI_MSIX_FLAGS_MASKALL
> > > too")
> > > Signed-off-by: Marek Marczykowski-Górecki
> > >
> > > ---
> > > Changes in v3:
> > > - allow clearing INTx regardless of MSI/MSI-X state, to be consistent
> > >with enabling MSI/MSI-X
> > > Changes in v2:
> > > - restructure the patch to consider not only MASKALL bit, but enabling
> > >MSI/MSI-X generally, without explicitly disabling INTx first
> > > ---
> > > drivers/xen/xen-pciback/conf_space.c | 19 +++--
> > > .../xen/xen-pciback/conf_space_capability.c | 3 ++-
> > > drivers/xen/xen-pciback/conf_space_header.c | 21 +++
> > > 3 files changed, 18 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/xen/xen-pciback/conf_space.c
> > > b/drivers/xen/xen-pciback/conf_space.c
> > > index 059de92aea7d..d47eee6c5143 100644
> > > --- a/drivers/xen/xen-pciback/conf_space.c
> > > +++ b/drivers/xen/xen-pciback/conf_space.c
> > > @@ -288,12 +288,6 @@ int xen_pcibk_get_interrupt_type(struct pci_dev *dev)
> > > u16 val;
> > > int ret = 0;
> > >
> > > - err = pci_read_config_word(dev, PCI_COMMAND, );
> > > - if (err)
> > > - return err;
> > > - if (!(val & PCI_COMMAND_INTX_DISABLE))
> > > - ret |= INTERRUPT_TYPE_INTX;
> > > -
> > > /*
> > >* Do not trust dev->msi(x)_enabled here, as enabling could be done
> > >* bypassing the pci_*msi* functions, by the qemu.
> > > @@ -316,6 +310,19 @@ int xen_pcibk_get_interrupt_type(struct pci_dev *dev)
> > > if (val & PCI_MSIX_FLAGS_ENABLE)
> > > ret |= INTERRUPT_TYPE_MSIX;
> > > }
> >
> > Since we are explicitly hiding INTx now, should we also do something
> > about MSI(X) being both enabled at the same time? The spec states:
> >
> > "System configuration software sets one of these bits to enable either
> > MSI or MSI-X, but never both simultaneously. Behavior is undefined if
> > both MSI and MSI-X are enabled simultaneously."
> >
> > So finding both MSI and MSI-X enabled likely means something has gone
> > very wrong? Likely to be done in a separate change, just realized
> > while looking at the patch context.
>
> Pciback try to prevent such situation (that's exactly the point of
> checking the current interrupt type). But if you get into such situation
> somehow anyway (likely bypassing pciback), then pciback will still allow
> to disable one of them, so you can fix the situation (the enforcement of
> "only one type at the time" is done setting the enable bit, but you can still
> clear it).
>
> If both MSI and MSI-X are enabled xen_pcibk_get_interrupt_type() will
> return both bits set.
>
> > > +
> > > + /*
> > > + * PCIe spec says device cannot use INTx if MSI/MSI-X is enabled,
> > > + * so check for INTx only when both are disabled.
> > > + */
> > > + if (!ret) {
> > > + err = pci_read_config_word(dev, PCI_COMMAND, );
> > > + if (err)
> > > + return err;
> > > + if (!(val & PCI_COMMAND_INTX_DISABLE))
> > > + ret |= INTERRUPT_TYPE_INTX;
> > > + }
> > > +
> > > return ret ?: INTERRUPT_TYPE_NONE;
> > > }
> > >
> > > diff --git a/drivers/xen/xen-pciback/conf_space_capability.c
> > > b/drivers/xen/xen-pciback/conf_space_capability.c
> > > index 097316a74126..eb4c1af44f5c 100644
> > > --- a/drivers/xen/xen-pciback/conf_space_capability.c
> > > +++ b/drivers/xen/xen-pciback/conf_space_capability.c
> > > @@ -236,10 +236,11 @@ static int msi_msix_flags_write(struct pci_dev
> > > *dev, int offset, u16 new_value,
> > > return
Re: [for-4.18][PATCH] xen/arm: Check return code from recursive calls to scan_pfdt_node()
Hi Michal,
> On 16 Oct 2023, at 14:45, Michal Orzel wrote:
>
> At the moment, we do not check a return code from scan_pfdt_node()
> called recursively. This means that any issue that may occur while
> parsing and copying the passthrough nodes is hidden and Xen continues
> to boot a domain despite errors. This may lead to incorrect device tree
> generation and various guest issues (e.g. trap on attempt to access MMIO
> not mapped in P2M). Fix it.
>
> Fixes: 669ecdf8d6cd ("xen/arm: copy dtb fragment to guest dtb")
> Signed-off-by: Michal Orzel
Good finding :-)
Reviewed-by: Bertrand Marquis
Cheers
Bertrand
> ---
> @Henry:
> This is a bug fix, so I think we should have it in 4.18 given the possible
> consequences I described in the commit msg. I don't see any risks as this
> change
> only checks the return code for an error.
> ---
> xen/arch/arm/domain_build.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 24c9019cc43c..49792dd590ee 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2872,8 +2872,11 @@ static int __init scan_pfdt_node(struct kernel_info
> *kinfo, const void *pfdt,
> node_next = fdt_first_subnode(pfdt, nodeoff);
> while ( node_next > 0 )
> {
> -scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells,
> - scan_passthrough_prop);
> +rc = scan_pfdt_node(kinfo, pfdt, node_next, address_cells,
> size_cells,
> +scan_passthrough_prop);
> +if ( rc )
> +return rc;
> +
> node_next = fdt_next_subnode(pfdt, node_next);
> }
>
> --
> 2.25.1
>
Re: [PATCH for-4.18 v5] xen/pdx: Make CONFIG_PDX_COMPRESSION a common Kconfig option
On 06.10.2023 18:01, Andrew Cooper wrote: > On 06/10/2023 4:09 pm, Julien Grall wrote: >> >> >> On 06/10/2023 15:44, Andrew Cooper wrote: >>> From: Alejandro Vallejo >>> >>> Adds a new compile-time flag to allow disabling PDX compression and >>> compiles out compression-related code/data. It also shorts the pdx<->pfn >>> conversion macros and creates stubs for masking functions. >>> >>> While at it, removes the old arch-defined CONFIG_HAS_PDX flag. >>> Despite the >>> illusion of choice, it was not optional. >>> >>> There are ARM and PPC platforms with sparse RAM banks - leave compression >>> active by default there. OTOH, there are no known production x86 >>> systems with >>> sparse RAM banks, so disable compression. This decision can be >>> revisited if >>> such a platform comes along. >> >> (Process remarks rather than the code itself) >> >> Jan is away this week so I want to make sure this doesn't go in without >> him having a say. >> >> While I don't particularly care about the approach taken for x86, Jan >> voiced concerned with this approach and so far I didn't see any >> conclusion. If there is any, then please point me to them. >> >> For the record, the objections from Jan are in [1]. If we want to ignore >> them, then I think we need a vote. Possibly only from the x86 folks (?). > > What do you think the 2 x86 maintainer tags on this patch in this exact > form, following far too much wasted time already, represents. The vote > has already concluded. In a reply separate from his R-b he also said "I would be fine in leaving the option to be selected if ...", so I don't think you can count tags as votes. As much as you apparently have a hard time seeing why I want the option to remain available (despite knowing why I introduced PDX back at the time), I'm having a hard time seeing why you want it unilaterally off (and I'm afraid I haven't seen any reasoning beyond you simply not liking that code, and you also not having liked my earlier attempts to overcome the undue overhead). Jan
