[Xen-devel] [qemu-mainline test] 144984: regressions - FAIL
flight 144984 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/144984/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-freebsd10-i386 14 guest-saverestore fail REGR. vs. 144861 test-amd64-i386-freebsd10-amd64 14 guest-saverestore fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-debianhvm-amd64 13 guest-saverestore fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-win7-amd64 13 guest-saverestore fail REGR. vs. 144861 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 guest-saverestore fail REGR. vs. 144861 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 guest-saverestore fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 13 guest-saverestore fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 13 guest-saverestore fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 13 guest-saverestore fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-ovmf-amd64 13 guest-saverestore fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-debianhvm-amd64 13 guest-saverestore fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-ovmf-amd64 13 guest-saverestore fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 13 guest-saverestore fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-win7-amd64 13 guest-saverestore fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-ws16-amd64 13 guest-saverestore fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-ws16-amd64 13 guest-saverestore fail REGR. vs. 144861 Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-rtds16 guest-start/debian.repeat fail REGR. vs. 144861 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10 fail like 144861 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 144861 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 144861 test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass
[Xen-devel] [xen-unstable-smoke bisection] complete build-arm64-xsm
branch xen-unstable-smoke xenbranch xen-unstable-smoke job build-arm64-xsm testid xen-build/dist-test Tree: qemuu git://xenbits.xen.org/qemu-xen.git Tree: xen git://xenbits.xen.org/xen.git *** Found and reproduced problem changeset *** Bug is in tree: xen git://xenbits.xen.org/xen.git Bug introduced: 25164571fc11ed3010c5885a98a68fac3b891d33 Bug not present: 0cd791c499bdc698d14a24050ec56d60b45732e0 Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/145004/ commit 25164571fc11ed3010c5885a98a68fac3b891d33 Merge: 0cd791c499 5083e0ff93 Author: Konrad Rzeszutek Wilk Date: Thu Dec 19 20:16:43 2019 -0500 Merge branch 'livepatch.aws.v6' into staging * livepatch.aws.v6: livepatch: Add metadata runtime retrieval mechanism livepatch: Handle arbitrary size names with the list operation livepatch: Add support for modules .modinfo section metadata livepatch: Add support for inline asm livepatching expectations livepatch: Add per-function applied/reverted state tracking marker livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence livepatch: Add support for apply|revert action replacement hooks livepatch: Implement pre-|post- apply|revert hooks livepatch: Export payload structure via livepatch_payload.h livepatch: Allow to override inter-modules buildid dependency livepatch: Always check hypervisor build ID upon livepatch upload Signed-off-by: Konrad Rzeszutek Wilk commit 5083e0ff939d149860db40e0da54ea2048749471 Author: Pawel Wieczorkiewicz Date: Tue Nov 26 10:08:00 2019 + livepatch: Add metadata runtime retrieval mechanism Extend the livepatch list operation to fetch also payloads' metadata. This is achieved by extending the sysctl list interface with 2 extra guest handles: * metadata - an array of arbitrary size strings * metadata_len - an array of metadata strings' lengths (uin32_t each) Payloads' metadata is a string of arbitrary size and does not have an upper bound limit. It may also vary in size between payloads. In order to let the userland allocate enough space for the incoming data add a metadata total size field to the list sysctl operation and fill it with total size of all payloads' metadata. Extend the libxc to handle the metadata back-to-back data transfers as well as metadata length array data transfers. The xen-livepatch userland tool is extended to always display the metadata for each received module. The metadata is received with the following format: key=value\0key=value\0...key=value\0. The format is modified to the following one: key=value;key=value;...key=value. The new format allows to easily parse the metadata for a given module by a machine. Signed-off-by: Pawel Wieczorkiewicz Reviewed-by: Andra-Irina Paraschiv Reviewed-by: Martin Pohlack Reviewed-by: Norbert Manthey Signed-off-by: Konrad Rzeszutek Wilk Reviewed-by: Ross Lagerwall commit b145b4a39c1324186b1b43313a9fefc19b7aa43f Author: Pawel Wieczorkiewicz Date: Tue Nov 26 10:07:59 2019 + livepatch: Handle arbitrary size names with the list operation The payloads' name strings can be of arbitrary size (typically small with an upper bound of XEN_LIVEPATCH_NAME_SIZE). Current implementation of the list operation interface allows to copy names in the XEN_LIVEPATCH_NAME_SIZE chunks regardless of its actual size and enforces space allocation requirements on userland tools. To unify and simplify the interface, handle the name strings of arbitrary size by copying them in adhering chunks to the userland. In order to let the userland allocate enough space for the incoming data add an auxiliary interface xc_livepatch_list_get_sizes() that provides the current number of payload entries and the total size of all name strings. This is achieved by extending the sysctl list interface with an extra fields: name_total_size. The xc_livepatch_list_get_sizes() issues the livepatch sysctl list operation with the nr field set to 0. In this mode the operation returns the number of payload entries and calculates the total sizes for all payloads' names. When the sysctl operation is issued with a non-zero nr field (for instance with a value obtained earlier with the prior call to the xc_livepatch_list_get_sizes()) the new field name_total_size provides the total size of actually copied data. Extend the libxc to handle the name back-to-back data transfers. The xen-livepatch tool is modified to start the list operation with a call to the xc_livepatch_list_get_sizes() to obtain the actual
Re: [Xen-devel] Recent cores-scheduling failures
On 19.12.19 13:45, Sergey Dyasli wrote: Hi Juergen, We recently did another quick test of core scheduling mode, and the following failures were found: 1. live-patch apply failures: (XEN) [ 1058.751974] livepatch: lp_1_1: Timed out on semaphore in CPU quiesce phase 30/31 (XEN) [ 1058.751982] livepatch: lp_1_1 finished REPLACE with rc=-16 2. ACPI S5 crash: https://paste.debian.net/1121748/ Are there any XenServer patches in your hypervisor? I'm asking because I don't see why a vcpu would be freed when shutting down the host (other than by any shutdown scripts, but those should be long finished when trying to enter S5). Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [xen-unstable-smoke test] 144991: regressions - all pass
flight 144991 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/144991/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-arm64-xsm 7 xen-build/dist-test fail REGR. vs. 144983 build-amd64 7 xen-build/dist-test fail REGR. vs. 144983 build-armhf 7 xen-build/dist-test fail REGR. vs. 144983 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass version targeted for testing: xen 25164571fc11ed3010c5885a98a68fac3b891d33 baseline version: xen 0cd791c499bdc698d14a24050ec56d60b45732e0 Last test of basis 144983 2019-12-19 19:08:42 Z0 days Testing same since 144991 2019-12-20 02:01:01 Z0 days1 attempts People who touched revisions under test: Julien Grall Konrad Rzeszutek Wilk Pawel Wieczorkiewicz 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 Not pushing. (No revision log; it would be 436 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [xen-unstable test] 144972: tolerable FAIL - PUSHED
flight 144972 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/144972/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-xtf-amd64-amd64-5 72 xtf/test-hvm64-xsa-308 fail in 144959 pass in 144972 test-armhf-armhf-xl-rtds 12 guest-startfail pass in 144959 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10 fail blocked in 144936 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeat fail in 144959 like 144936 test-armhf-armhf-xl-rtds13 migrate-support-check fail in 144959 never pass test-armhf-armhf-xl-rtds 14 saverestore-support-check fail in 144959 never pass test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 144936 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 144936 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 144936 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 144936 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 144936 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 144936 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 144936 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 144936 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 144936 test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass version targeted for testing: xen 5c13ed79f3cba200f21e7dfd6ed7f3aa08e4dada baseline version: xen 0e7c69bd3c0b35a677d73843b39522787ccf5a3f Last test of basis 144936 2019-12-18 16:07:31 Z1 days Testing same since 144959 2019-12-19 04:58:26 Z0 days2 attempts
Re: [Xen-devel] REGRESSION: Xen 4.13 RC5 fails to bootstrap Dom0 on ARM
On Thu, 19 Dec 2019, Julien Grall wrote:
> > > In fact most of people on Arm are using GRUB rather than EFI directly as
> > > this is more friendly to use.
> > >
> > > Regarding the devicetree, Xen and Linux will completely ignore the
> > > memory nodes in Xen if using EFI. This because the EFI memory map will
> > > give you an overview of the platform with the EFI regions included.
> >
> > Aha! So in that sense it is a bug in Xen after all, right? (that's what
> > you're
> > referring to when you say you now understand what needs to get fixed).
>
> Yes. The EFI memory map is a list of existing memory with a type associated to
> it (Conventional, BootServiceCodes, MemoryMappedIO...).
>
> The OS/Hypervisor will have to go through them and check which regions are
> usuable. Compare to Linux, Xen has limited itself to only a few types.
>
> However, I think we can be on a par with Linux here.
I gave a look at the Linux implementation, the interesting bit is
drivers/firmware/efi/arm-init.c:is_usable_memory as far as I can tell.
I also gave a look at the Xen side, which is
xen/arch/arm/efi/efi-boot.h:efi_process_memory_map_bootinfo. As guessed,
the two are not quite the same.
One of the main differences is that Linux uses as "System RAM" even
regions that were marked as EFI_BOOT_SERVICES_CODE/DATA and
EFI_LOADER_CODE/DATA because they will get freed anyway. Xen doesn't
do that unless map_bs is set.
I wrote a quick patch to implement the Linux behavior on Xen, only
lightly tested. I can confirm that I see more memory this way. However,
I am not sure we actually want to import the Linux behavior wholesale.
Anyway, Roman, could you please let me know if this patch solves the
issue?
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index ca655ff003..ad18ff3669 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -149,10 +149,14 @@ static EFI_STATUS __init
efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
{
-if ( desc_ptr->Type == EfiConventionalMemory ||
- (!map_bs &&
- (desc_ptr->Type == EfiBootServicesCode ||
- desc_ptr->Type == EfiBootServicesData)) )
+if ( desc_ptr->Attribute == EFI_MEMORY_WB &&
+ (desc_ptr->Type == EfiConventionalMemory ||
+ desc_ptr->Type == EfiLoaderCode ||
+ desc_ptr->Type == EfiLoaderData ||
+ desc_ptr->Type == EfiACPIReclaimMemory ||
+ desc_ptr->Type == EfiPersistentMemory ||
+ desc_ptr->Type == EfiBootServicesCode ||
+ desc_ptr->Type == EfiBootServicesData) )
{
if ( !meminfo_add_bank(, desc_ptr) )
{
diff --git a/xen/include/efi/efidef.h b/xen/include/efi/efidef.h
index 86a7e111bf..f46207840f 100644
--- a/xen/include/efi/efidef.h
+++ b/xen/include/efi/efidef.h
@@ -147,6 +147,7 @@ typedef enum {
EfiMemoryMappedIO,
EfiMemoryMappedIOPortSpace,
EfiPalCode,
+EfiPersistentMemory,
EfiMaxMemoryType
} EFI_MEMORY_TYPE;
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests
On 19/12/2019 23:15, Andrew Cooper wrote: > On 19/12/2019 11:35, Jan Beulich wrote: > XENVER_changeset > XENVER_commandline > XENVER_build_id > > Return a more customer friendly empty string instead of "" > which would be shown in tools like dmidecode.> I think "" is quite fine for many of the original purposes. Maybe it would be better to filter for this when populating guest DMI tables? >>> I don't know how DMI tables are populated, but nothing stops a guest >>> from using these hypercalls directly. >> And this is precisely the case where I think "" is better >> than an empty string. > "" was a terrible choice back when it was introduced, and its > still a terrible choice today. > > These are ASCII string fields, and the empty string is a perfectly good > string. Nothing is going to break, because it would have broken the > first time around. Sorry - send just too early. This has shipped in several versions of XenServer already. It is part of a effort to prevent easy fingerprinting of the binary hypervisor - a security measure which was requested specifically by a number of customers. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests
On 19/12/2019 11:35, Jan Beulich wrote: XENVER_changeset XENVER_commandline XENVER_build_id Return a more customer friendly empty string instead of "" which would be shown in tools like dmidecode.> >>> I think "" is quite fine for many of the original purposes. >>> Maybe it would be better to filter for this when populating guest >>> DMI tables? >> I don't know how DMI tables are populated, but nothing stops a guest >> from using these hypercalls directly. > And this is precisely the case where I think "" is better > than an empty string. "" was a terrible choice back when it was introduced, and its still a terrible choice today. These are ASCII string fields, and the empty string is a perfectly good string. Nothing is going to break, because it would have broken the first time around. The end result without denied sprayed all over this interface is much cleaner overall. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests
Hi, On 19/12/2019 11:35, Jan Beulich wrote: On 19.12.2019 12:23, Sergey Dyasli wrote: On 18/12/2019 11:06, Jan Beulich wrote: On 17.12.2019 16:46, Sergey Dyasli wrote: Hide the following information that can help identify the running Xen binary version: XENVER_extraversion XENVER_compile_info XENVER_capabilities What's wrong with exposing this one? extraversion can help identify the exact running Xen binary (and I must say that at Citrix we add some additional version information there). compile_info will identify compiler and many speculative mitigations are provided by compilers these days. Not sure if it's worth hiding capabilities, but OTOH I don't see how guests could meaningfully use it. Well, my question (using "this", not "these") was really mainly on the last item. I can see how extraversion can provide clues. I'm having difficulty seeing how the compiler (little bit of) details can provide sufficient information to become leveragable. XENVER_changeset XENVER_commandline XENVER_build_id Return a more customer friendly empty string instead of "" which would be shown in tools like dmidecode.> I think "" is quite fine for many of the original purposes. Maybe it would be better to filter for this when populating guest DMI tables? I don't know how DMI tables are populated, but nothing stops a guest from using these hypercalls directly. And this is precisely the case where I think "" is better than an empty string. +1. The more the behavior would change for tools checking whether the string is valid (i.e != ""). return xsm_default_action(XSM_HOOK, current->domain, NULL); + +case XENVER_extraversion: +case XENVER_compile_info: +case XENVER_capabilities: +case XENVER_changeset: +case XENVER_commandline: +case XENVER_build_id: default: There's no need to add all of these next to the default case. Note how commandline and build_id have been coming here already (and there would need to be further justification for exposing them on debug builds, should this questionable behavior - see above - be retained). I find that explicitly listing all the cases is better for code readability, but I don't have a strong opinion here. Well, I'm viewing it kind of the opposite, as being unnecessary clutter (and hence harming readability). We'll see what others think. I am on Sergey side here, with a "default" label you have to check what falls under it. So explicit case makes easier to find out how each hypercalls are dealt with. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 4/4] x86/microcode: Support builtin CPU microcode
On 18.12.19 13:42, Jan Beulich wrote:
On 18.12.2019 02:32, Eslam Elnikety wrote:
+
+
+Xen can bundle microcode updates within its image. This support is conditional
+on the build configuration BUILTIN_UCODE being enabled. Builtin microcode is
+useful to ensure that, by default, a minimum microcode patch level will be
+applied to the underlying CPU.
+
+To use microcode updates available on the build system as builtin,
+use BUILTIN_UCODE_DIR to refer to the directory containing the firmware updates
+and specify the individual microcode patches via either BUILTIN_UCODE_AMD or
+BUILTIN_UCODE_INTEL for AMD microcode or INTEL microcode, respectively. For
+instance, the configuration below is suitable for a build system which has a
+``/lib/firmware/`` directory which, in turn, includes the individual microcode
+patches ``amd-ucode/microcode_amd_fam15h.bin``, ``intel-ucode/06-3a-09``, and
+``intel-ucode/06-2f-02``.
+
+ CONFIG_BUILTIN_UCODE=y
+ CONFIG_BUILTIN_UCODE_DIR="/lib/firmware/"
+ CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin"
+ CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09 intel-ucode/06-2f-02"
Rather than a blank as separator, the more conventional one on
Unix and alike would be : I think. Of course ideally there wouldn't
be any restriction at all on the characters usable here for file
names.
It would be great if there is a particular convention. The blank
separator is aligned with Linux way of doing builtin microcode.
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -218,6 +218,36 @@ config MEM_SHARING
bool "Xen memory sharing support" if EXPERT = "y"
depends on HVM
+config BUILTIN_UCODE
+ bool "Support for Builtin Microcode"
+ ---help---
+ Include the CPU microcode update in the Xen image itself. With this
+ support, Xen can update the CPU microcode upon boot using the builtin
+ microcode, with no need for an additional microcode boot modules.
+
+ If unsure, say N.
I continue to be unconvinced that this separate option is needed.
Albeit compared to the v1 approach I will agree that handling
would become more complicated without.
Any particular preference between the v1 vs v2 approach?
@@ -701,7 +747,13 @@ static int __init microcode_init(void)
*/
if ( ucode_blob.size )
{
+#ifdef CONFIG_BUILTIN_UCODE
+/* No need to destroy module mappings if builtin was used */
+if ( !ucode_builtin )
+bootstrap_map(NULL);
+#else
bootstrap_map(NULL);
+#endif
First of all - is there no ucode unrelated side effect of this
invocation? I.e. can it safely be skipped?
Maybe I am missing something. Are you asking if we can safely skip the
bootstrap_map(NULL)? (Quoting your response on PATCH v2 2/4 "And of
course we really want these mappings to be gone")
If yes, then I think
you want to get away without #ifdef here, by having a suitably
placed
#define ucode_builtin false
somewhere up the file.
Agreed. That will make the code snippet more readable indeed.
--- /dev/null
+++ b/xen/arch/x86/microcode/Makefile
@@ -0,0 +1,46 @@
+# Copyright (C) 2019 Amazon.com, Inc. or its affiliates.
+# Author: Eslam Elnikety
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+
+# Remove quotes and excess spaces from configuration strings
+UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR)))
+UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD)))
+UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL)))
+
+# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs.
+amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD)))
+intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL)))
+
+ifneq ($(amd-blobs),)
+obj-y += ucode_amd.o
+endif
+
+ifneq ($(intel-blobs),)
+obj-y += ucode_intel.o
+endif
+
+ifeq ($(amd-blobs)$(intel-blobs),)
+obj-y += ucode_dummy.o
+endif
+
+ucode_amd.o: Makefile $(amd-blobs)
+ cat $(amd-blobs) > $@.bin
+ $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section
.data=.builtin_amd_ucode,alloc,load,readonly,data,contents $@.bin $@
+ rm -f $@.bin
+
+ucode_intel.o: Makefile $(intel-blobs)
+ cat $(intel-blobs) > $@.bin
+ $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section
.data=.builtin_intel_ucode,alloc,load,readonly,data,contents $@.bin $@
+ rm -f $@.bin
This can be had with a pattern rule (with the vendor being the stem)
and hence without
[Xen-devel] [xen-unstable-smoke test] 144983: tolerable all pass - PUSHED
flight 144983 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/144983/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass version targeted for testing: xen 0cd791c499bdc698d14a24050ec56d60b45732e0 baseline version: xen 5c13ed79f3cba200f21e7dfd6ed7f3aa08e4dada Last test of basis 144934 2019-12-18 15:01:21 Z1 days Testing same since 144983 2019-12-19 19:08:42 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Tamas K Lengyel Wei Liu 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 5c13ed79f3..0cd791c499 0cd791c499bdc698d14a24050ec56d60b45732e0 -> smoke ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/4] x86/microcode: avoid unnecessary xmalloc/memcpy of ucode data
On 18.12.19 13:05, Jan Beulich wrote:
On 18.12.2019 02:32, Eslam Elnikety wrote:
@@ -725,7 +701,7 @@ static int __init microcode_init(void)
*/
if ( ucode_blob.size )
{
-xfree(ucode_blob.data);
+bootstrap_map(NULL);
As much as I like the change, I wholeheartedly disagree with this
aspect of it: You make it largely unpredictable when the boot
mappings will go away - it becomes entirely dependent upon link
order. And of course we really want these mappings to be gone,
the very latest (I think), by the time we start bringing up APs
(but generally the sooner the better). This is (one of?) the main
reason(s) why it hadn't been done this way to begin with. The
alternative is more complicated (set up a proper, long term
mapping), but it's going to be more clean (including the mapping
then also being suitable to post-boot CPU onlining).
This change is an improvement on the current status. We get to avoid
xmalloc/memcpy in the case of a successful ucode=scan. The problematic
aspect you highlight is anyway there regardless of this patch (ref. to
the "else if ( ucode_mod.mod_end )" in microcode_init). The proper, long
term mapping would be a welcome change, but is otherwise independent of
this patch imo.
Thanks,
Eslam
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/4] x86/microcode: Improve documentation and parsing for ucode=
On 18.12.19 12:49, Jan Beulich wrote:
On 18.12.2019 02:32, Eslam Elnikety wrote:
Decouple the microcode referencing mechanism when using GRUB to that
when using EFI. This allows us to avoid the "unspecified effect" of
using ` | scan` along xen.efi.
I guess "unspecified effect" in the doc was pretty pointless - such
options have been ignored before; in fact ...
With that, Xen can explicitly
ignore those named options when using EFI.
... I don't see things becoming any more explicit (not even parsing
the options was quite explicit to me).
I agree that those options have been ignored so far in the case of EFI.
The documentation contradicts that however. The documentation:
* says has unspecified effect.
* does not mention anything about scan being ignored.
With this patch, it is explicit in code and in documentation that both
options are ignored in case of EFI.
As an added benefit,
we get a straightfoward parsing of the ucode parameter.
It's a single if() you eliminate - for me this doesn't make it
meaningfully more straightforward.
As we decouple ucode_mod_idx and ucode_mod_efi_idx, the parsing of the
ucode= just follows the pattern "[ | scan=, nmi=
]" and it does not have to cater for corner cases. In either case, if
you strongly disagree with the wording, I can drop that sentence.
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2128,7 +2128,13 @@ logic applies:
### ucode (x86)
> `= List of [ | scan=, nmi= ]`
-Specify how and where to find CPU microcode update blob.
+Applicability: x86
+Default: `nmi`
+
+Controls for CPU microcode loading. For early loading, this parameter can
+specify how and where to find the microcode update blob. For late loading,
+this parameter specifies if the update happens within a NMI handler or in
+a stop_machine context.
It's always stop_machine context, isn't it? I also don't think this
implementation detail belongs here.
Needs a better wording indeed. Let me know if you have particular
suggestions, and I will incorporate in v3.
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -60,7 +60,7 @@
static module_t __initdata ucode_mod;
static signed int __initdata ucode_mod_idx;
-static bool_t __initdata ucode_mod_forced;
+static signed int __initdata ucode_mod_efi_idx;
I don't see anything negative be put into here - should be unsigned
int then.
Correct! The type just carried over from the coupling with
ucode_mod_idx. Will adjust in v3.
@@ -105,16 +105,10 @@ static struct microcode_patch *microcode_cache;
void __init microcode_set_module(unsigned int idx)
{
-ucode_mod_idx = idx;
-ucode_mod_forced = 1;
+ucode_mod_efi_idx = idx;
Is it guaranteed (now and forever) that the index passed in is
non-zero? You changes to microcode_grab_module() imply so, but
just looking at the call site of the function I can't convince
myself this is the case. _If_ it is (thought to be) guaranteed,
then I think you at least want to ASSERT() here, perhaps with
a comment.
For x86, it seems we have that guarantee (given that we must have a
dom0). Right?
}
-/*
- * The format is '[|scan=, nmi=]'. Both options are
- * optional. If the EFI has forced which of the multiboot payloads is to be
- * used, only nmi= is parsed.
- */
-static int __init parse_ucode(const char *s)
+static int __init parse_ucode_param(const char *s)
Any particular reason for the renaming? The function name was quite
fine imo.
To me, "parse_ucode" is a misnomer.
Thanks for your comments, Jan.
-- Eslam
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] arm64: xen: Use modern annotations for assembly functions
On Thu, 19 Dec 2019, Mark Brown wrote: > In an effort to clarify and simplify the annotation of assembly functions > in the kernel new macros have been introduced. These replace ENTRY and > ENDPROC. Update the annotations in the xen code to the new macros. > > Signed-off-by: Mark Brown > Reviewed-by: Julien Grall > Reviewed-by: Stefano Stabellini Thank you! > --- > arch/arm64/xen/hypercall.S | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/xen/hypercall.S b/arch/arm64/xen/hypercall.S > index c5f05c4a4d00..5b09aca55108 100644 > --- a/arch/arm64/xen/hypercall.S > +++ b/arch/arm64/xen/hypercall.S > @@ -56,11 +56,11 @@ > #define XEN_IMM 0xEA1 > > #define HYPERCALL_SIMPLE(hypercall) \ > -ENTRY(HYPERVISOR_##hypercall)\ > +SYM_FUNC_START(HYPERVISOR_##hypercall) \ > mov x16, #__HYPERVISOR_##hypercall; \ > hvc XEN_IMM;\ > ret;\ > -ENDPROC(HYPERVISOR_##hypercall) > +SYM_FUNC_END(HYPERVISOR_##hypercall) > > #define HYPERCALL0 HYPERCALL_SIMPLE > #define HYPERCALL1 HYPERCALL_SIMPLE > @@ -86,7 +86,7 @@ HYPERCALL2(multicall); > HYPERCALL2(vm_assist); > HYPERCALL3(dm_op); > > -ENTRY(privcmd_call) > +SYM_FUNC_START(privcmd_call) > mov x16, x0 > mov x0, x1 > mov x1, x2 > @@ -109,4 +109,4 @@ ENTRY(privcmd_call) >*/ > uaccess_ttbr0_disable x6, x7 > ret > -ENDPROC(privcmd_call); > +SYM_FUNC_END(privcmd_call); > -- > 2.20.1 > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [ovmf test] 144974: all pass - PUSHED
flight 144974 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/144974/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 665afccc52e1a02ee329147e02f04b8e9cf1d571 baseline version: ovmf 95bb203861c5e19b7b7d5d9318e16d82108f2134 Last test of basis 144962 2019-12-19 06:46:07 Z0 days Testing same since 144974 2019-12-19 15:09:19 Z0 days1 attempts People who touched revisions under test: Daniel Pawel Banaszek 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 95bb203861..665afccc52 665afccc52e1a02ee329147e02f04b8e9cf1d571 -> xen-tested-master ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 01/20] x86: make hvm_{get/set}_param accessible
On Thu, Dec 19, 2019 at 12:57 PM Andrew Cooper wrote: > > On 19/12/2019 19:49, Tamas K Lengyel wrote: > > On Thu, Dec 19, 2019 at 12:41 PM Andrew Cooper > > wrote: > >> On 19/12/2019 19:38, Tamas K Lengyel wrote: > > --- a/xen/include/asm-x86/hvm/hvm.h > > +++ b/xen/include/asm-x86/hvm/hvm.h > > @@ -335,6 +335,10 @@ unsigned long hvm_cr4_guest_valid_bits(const > > struct domain *d, bool restore); > > bool hvm_flush_vcpu_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v), > > void *ctxt); > > > > +/* Caller must hold domain locks */ > How about asserting so? > >>> AFAICT there is no "domain_locked_by_me" function, only > >>> paging/p2m/gfn_locked_by_me. So any suggestion on how to achieve that? > >> spin_is_locked() gets you most of the way, and would be a start. > >> > >> But yes - now you say this, I remember that we don't currently have > >> suitable infrastructure. > > Is the domain lock even a spin lock (the on you use by > > rcu_lock_live_remote_domain_by_id)? Looks to me it just goes down to > > "rcu_read_lock" which only does a preempt_disable() call o.O > > /sigh - of course we have two things called the domain lock. > > The RCU one is to ensure that the struct domain can't get freed behind > our back, and shouldn't be interesting in this context (obtaining the d > pointer in the first place causes it to be safe). If that is the only > one which matters, I would drop the comment. Yes, only the RCU lock gets taken right now by all callers, so I'll drop the comment. Thanks, Tamas ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 01/20] x86: make hvm_{get/set}_param accessible
On 19/12/2019 19:49, Tamas K Lengyel wrote: > On Thu, Dec 19, 2019 at 12:41 PM Andrew Cooper > wrote: >> On 19/12/2019 19:38, Tamas K Lengyel wrote: > --- a/xen/include/asm-x86/hvm/hvm.h > +++ b/xen/include/asm-x86/hvm/hvm.h > @@ -335,6 +335,10 @@ unsigned long hvm_cr4_guest_valid_bits(const struct > domain *d, bool restore); > bool hvm_flush_vcpu_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v), > void *ctxt); > > +/* Caller must hold domain locks */ How about asserting so? >>> AFAICT there is no "domain_locked_by_me" function, only >>> paging/p2m/gfn_locked_by_me. So any suggestion on how to achieve that? >> spin_is_locked() gets you most of the way, and would be a start. >> >> But yes - now you say this, I remember that we don't currently have >> suitable infrastructure. > Is the domain lock even a spin lock (the on you use by > rcu_lock_live_remote_domain_by_id)? Looks to me it just goes down to > "rcu_read_lock" which only does a preempt_disable() call o.O /sigh - of course we have two things called the domain lock. The RCU one is to ensure that the struct domain can't get freed behind our back, and shouldn't be interesting in this context (obtaining the d pointer in the first place causes it to be safe). If that is the only one which matters, I would drop the comment. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 01/20] x86: make hvm_{get/set}_param accessible
On Thu, Dec 19, 2019 at 12:41 PM Andrew Cooper wrote: > > On 19/12/2019 19:38, Tamas K Lengyel wrote: > >>> --- a/xen/include/asm-x86/hvm/hvm.h > >>> +++ b/xen/include/asm-x86/hvm/hvm.h > >>> @@ -335,6 +335,10 @@ unsigned long hvm_cr4_guest_valid_bits(const struct > >>> domain *d, bool restore); > >>> bool hvm_flush_vcpu_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v), > >>> void *ctxt); > >>> > >>> +/* Caller must hold domain locks */ > >> How about asserting so? > > AFAICT there is no "domain_locked_by_me" function, only > > paging/p2m/gfn_locked_by_me. So any suggestion on how to achieve that? > > spin_is_locked() gets you most of the way, and would be a start. > > But yes - now you say this, I remember that we don't currently have > suitable infrastructure. Is the domain lock even a spin lock (the on you use by rcu_lock_live_remote_domain_by_id)? Looks to me it just goes down to "rcu_read_lock" which only does a preempt_disable() call o.O Tamas ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 01/20] x86: make hvm_{get/set}_param accessible
On 19/12/2019 19:38, Tamas K Lengyel wrote: >>> --- a/xen/include/asm-x86/hvm/hvm.h >>> +++ b/xen/include/asm-x86/hvm/hvm.h >>> @@ -335,6 +335,10 @@ unsigned long hvm_cr4_guest_valid_bits(const struct >>> domain *d, bool restore); >>> bool hvm_flush_vcpu_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v), >>> void *ctxt); >>> >>> +/* Caller must hold domain locks */ >> How about asserting so? > AFAICT there is no "domain_locked_by_me" function, only > paging/p2m/gfn_locked_by_me. So any suggestion on how to achieve that? spin_is_locked() gets you most of the way, and would be a start. But yes - now you say this, I remember that we don't currently have suitable infrastructure. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 01/20] x86: make hvm_{get/set}_param accessible
> > --- a/xen/include/asm-x86/hvm/hvm.h > > +++ b/xen/include/asm-x86/hvm/hvm.h > > @@ -335,6 +335,10 @@ unsigned long hvm_cr4_guest_valid_bits(const struct > > domain *d, bool restore); > > bool hvm_flush_vcpu_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v), > > void *ctxt); > > > > +/* Caller must hold domain locks */ > > How about asserting so? AFAICT there is no "domain_locked_by_me" function, only paging/p2m/gfn_locked_by_me. So any suggestion on how to achieve that? Thanks! Tamas ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 04/20] x86/mem_sharing: cleanup code and comments in various locations
On Thu, Dec 19, 2019 at 11:51 AM Andrew Cooper wrote: > > On 19/12/2019 16:21, Tamas K Lengyel wrote: > >>> @@ -437,25 +441,29 @@ static inline void mem_sharing_gfn_destroy(struct > >>> page_info *page, > >>> xfree(gfn_info); > >>> } > >>> > >>> -static struct page_info* mem_sharing_lookup(unsigned long mfn) > >>> +static inline struct page_info* mem_sharing_lookup(unsigned long mfn) > >> Seeing as this is cleanup, the position of the * can move. Similarly, > >> it shouldn't gain an inline. > >> > >> I can fix all of this up on commit (and a few other brace position > >> issues) if you want, to save reworking a trivial v2. > >> > > Provided nothing else is outstanding with the patch and it can be > > committed I would certainly appreciate that. > > I've pushed this and the previous patch, with some further fixups that I > spotted. > > Hopefully the rebase won't be an issue. Great! There were quite a few conflicts in the rebase so I hope I don't accidentally revert some of your new fixes :) Tamas ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 07/20] x86/mem_sharing: don't try to unshare twice during page fault
On 18/12/2019 19:40, Tamas K Lengyel wrote:
> @@ -1959,19 +1965,21 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned
> long gla,
> */
> if ( paged )
> p2m_mem_paging_populate(currd, gfn);
> +
> +#ifdef CONFIG_MEM_SHARING
> if ( sharing_enomem )
> {
> -int rv;
> -
> -if ( (rv = mem_sharing_notify_enomem(currd, gfn, true)) < 0 )
> +if ( !vm_event_check_ring(currd->vm_event_share) )
> {
> gdprintk(XENLOG_ERR, "Domain %hu attempt to unshare "
> - "gfn %lx, ENOMEM and no helper (rc %d)\n",
> - currd->domain_id, gfn, rv);
> + "gfn %lx, ENOMEM and no helper\n",
> + currd->domain_id, gfn);
As observations on this and later patches. Use %pd (gets d%u or d[NAME]
as applicable), especially were d[COW] is a liable domid to get.
Also, any action which crashes a domain must not be a gdprintk(),
because otherwise you end up with a domain_crash() and no
context/symptoms in release builds of Xen.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 05/20] x86/mem_sharing: make get_two_gfns take locks conditionally
On 18/12/2019 19:40, Tamas K Lengyel wrote: > During VM forking the client lock will already be taken. > > Signed-off-by: Tamas K Lengyel Acked-by: Andrew Cooper ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 02/20] xen/x86: Make hap_get_allocation accessible
On 18/12/2019 19:40, Tamas K Lengyel wrote: > During VM forking we'll copy the parent domain's parameters to the client, > including the HAP shadow memory setting that is used for storing the domain's > EPT. We'll copy this in the hypervisor instead doing it during toolstack > launch > to allow the domain to start executing and unsharing memory before (or > even completely without) the toolstack. > > Signed-off-by: Tamas K Lengyel Acked-by: Andrew Cooper ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [qemu-mainline test] 144964: regressions - FAIL
flight 144964 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/144964/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-freebsd10-i386 14 guest-saverestore fail REGR. vs. 144861 test-amd64-i386-freebsd10-amd64 14 guest-saverestore fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-debianhvm-amd64 13 guest-saverestore fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-win7-amd64 13 guest-saverestore fail REGR. vs. 144861 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 guest-saverestore fail REGR. vs. 144861 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 guest-saverestore fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 13 guest-saverestore fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 13 guest-saverestore fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 13 guest-saverestore fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-ovmf-amd64 13 guest-saverestore fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-debianhvm-amd64 13 guest-saverestore fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-ovmf-amd64 13 guest-saverestore fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 13 guest-saverestore fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-win7-amd64 13 guest-saverestore fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-ws16-amd64 13 guest-saverestore fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-ws16-amd64 13 guest-saverestore fail REGR. vs. 144861 Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-rtds16 guest-start/debian.repeat fail REGR. vs. 144861 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10 fail like 144861 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 144861 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 144861 test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass
Re: [Xen-devel] [PATCH v2 01/20] x86: make hvm_{get/set}_param accessible
On 18/12/2019 19:40, Tamas K Lengyel wrote:
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 614ed60fe4..5a3a962fbb 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4072,16 +4072,17 @@ static int hvmop_set_evtchn_upcall_vector(
> }
>
> static int hvm_allow_set_param(struct domain *d,
> - const struct xen_hvm_param *a)
> + uint32_t index,
> + uint64_t new_value)
These two lines can be folded together.
> @@ -4134,13 +4135,11 @@ static int hvm_allow_set_param(struct domain *d,
> return rc;
> }
>
> -static int hvmop_set_param(
> +int hvmop_set_param(
> XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
and here.
> @@ -4160,23 +4159,42 @@ static int hvmop_set_param(
> if ( !is_hvm_domain(d) )
> goto out;
>
> -rc = hvm_allow_set_param(d, );
> +rc = hvm_set_param(d, a.index, a.value);
> +
> + out:
> +rcu_unlock_domain(d);
> +return rc;
> +}
> +
> +int hvm_set_param(
> +struct domain *d,
> +uint32_t index,
> +uint64_t value)
and again.
> @@ -4340,34 +4358,33 @@ static int hvmop_set_param(
> * 256 bits interrupt redirection bitmap + 64k bits I/O bitmap
> * plus one padding byte).
> */
> -if ( (a.value >> 32) > sizeof(struct tss32) +
> +if ( (value >> 32) > sizeof(struct tss32) +
> (0x100 / 8) + (0x1 / 8) + 1 )
> -a.value = (uint32_t)a.value |
> +value = (uint32_t)value |
>((sizeof(struct tss32) + (0x100 / 8) +
> (0x1 / 8) + 1) << 32);
Can you reindent the surrounding lines so they line up again?
> @@ -4429,42 +4446,60 @@ static int hvmop_get_param(
> if ( !is_hvm_domain(d) )
> goto out;
>
> -rc = hvm_allow_get_param(d, );
> +rc = hvm_get_param(d, a.index, );
> if ( rc )
> goto out;
>
> -switch ( a.index )
> +rc = __copy_to_guest(arg, , 1) ? -EFAULT : 0;
> +
> +HVM_DBG_LOG(DBG_LEVEL_HCALL, "get param %u = %"PRIx64,
> +a.index, a.value);
> +
> + out:
> +rcu_unlock_domain(d);
> +return rc;
> +}
> +
> +int hvm_get_param(
> +struct domain *d,
> +uint32_t index,
> +uint64_t *value)
Fold.
> +{
> +int rc;
> +
> +if ( index >= HVM_NR_PARAMS || !value )
No need for !value. It had better only ever point onto the hypervisor
stack now that this is an internal function.
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 1d7b66f927..a6f4ae76a1 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -335,6 +335,10 @@ unsigned long hvm_cr4_guest_valid_bits(const struct
> domain *d, bool restore);
> bool hvm_flush_vcpu_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v),
> void *ctxt);
>
> +/* Caller must hold domain locks */
How about asserting so?
No major problems so Acked-by: Andrew Cooper
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 04/20] x86/mem_sharing: cleanup code and comments in various locations
On 19/12/2019 16:21, Tamas K Lengyel wrote: >>> @@ -437,25 +441,29 @@ static inline void mem_sharing_gfn_destroy(struct >>> page_info *page, >>> xfree(gfn_info); >>> } >>> >>> -static struct page_info* mem_sharing_lookup(unsigned long mfn) >>> +static inline struct page_info* mem_sharing_lookup(unsigned long mfn) >> Seeing as this is cleanup, the position of the * can move. Similarly, >> it shouldn't gain an inline. >> >> I can fix all of this up on commit (and a few other brace position >> issues) if you want, to save reworking a trivial v2. >> > Provided nothing else is outstanding with the patch and it can be > committed I would certainly appreciate that. I've pushed this and the previous patch, with some further fixups that I spotted. Hopefully the rebase won't be an issue. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 2/1] libxc: Drop other examples of the 'goto x; } else if' antipattern
None of these are buggy, but the resulting code is more robust.
No functional change.
Signed-off-by: Andrew Cooper
---
CC: Ian Jackson
CC: Wei Liu
---
tools/libxc/xc_dom_core.c | 3 ++-
tools/libxc/xc_misc.c | 3 ++-
tools/libxc/xc_resource.c | 7 ---
tools/libxc/xc_sr_common.c | 3 ++-
tools/libxc/xc_sr_restore.c | 18 --
tools/libxc/xc_sr_restore_x86_hvm.c | 4 +++-
tools/libxc/xc_sr_restore_x86_pv.c | 33 ++---
7 files changed, 43 insertions(+), 28 deletions(-)
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index 9bd04cb2d5..73fe09fe18 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -225,7 +225,8 @@ void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
"tried to map file which is too large");
goto err;
}
-else if ( !*size )
+
+if ( !*size )
{
xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
"'%s': zero length file", filename);
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 8e60b6e9f0..b8eebd91e4 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -794,7 +794,8 @@ int xc_livepatch_list(xc_interface *xch, unsigned int max,
unsigned int start,
xc_hypercall_bounce_post(xch, len);
continue;
}
-else if ( rc < 0 ) /* For all other errors we bail out. */
+
+if ( rc < 0 ) /* For all other errors we bail out. */
break;
if ( !version )
diff --git a/tools/libxc/xc_resource.c b/tools/libxc/xc_resource.c
index 3abadbdcfc..3394cc1833 100644
--- a/tools/libxc/xc_resource.c
+++ b/tools/libxc/xc_resource.c
@@ -133,10 +133,11 @@ int xc_resource_op(xc_interface *xch, uint32_t nr_ops,
xc_resource_op_t *ops)
{
if ( nr_ops == 1 )
return xc_resource_op_one(xch, ops);
-else if ( nr_ops > 1 )
+
+if ( nr_ops > 1 )
return xc_resource_op_multi(xch, nr_ops, ops);
-else
-return -1;
+
+return -1;
}
/*
diff --git a/tools/libxc/xc_sr_common.c b/tools/libxc/xc_sr_common.c
index 79b9c3e940..6b887b3053 100644
--- a/tools/libxc/xc_sr_common.c
+++ b/tools/libxc/xc_sr_common.c
@@ -102,7 +102,8 @@ int read_record(struct xc_sr_context *ctx, int fd, struct
xc_sr_record *rec)
PERROR("Failed to read Record Header from stream");
return -1;
}
-else if ( rhdr.length > REC_LENGTH_MAX )
+
+if ( rhdr.length > REC_LENGTH_MAX )
{
ERROR("Record (0x%08x, %s) length %#x exceeds max (%#x)", rhdr.type,
rec_type_to_str(rhdr.type), rhdr.length, REC_LENGTH_MAX);
diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c
index 1ac404b97b..98038096c7 100644
--- a/tools/libxc/xc_sr_restore.c
+++ b/tools/libxc/xc_sr_restore.c
@@ -28,18 +28,21 @@ static int read_headers(struct xc_sr_context *ctx)
ERROR("Invalid marker: Got 0x%016"PRIx64, ihdr.marker);
return -1;
}
-else if ( ihdr.id != IHDR_ID )
+
+if ( ihdr.id != IHDR_ID )
{
ERROR("Invalid ID: Expected 0x%08x, Got 0x%08x", IHDR_ID, ihdr.id);
return -1;
}
-else if ( ihdr.version != IHDR_VERSION )
+
+if ( ihdr.version != IHDR_VERSION )
{
ERROR("Invalid Version: Expected %d, Got %d",
IHDR_VERSION, ihdr.version);
return -1;
}
-else if ( ihdr.options & IHDR_OPT_BIG_ENDIAN )
+
+if ( ihdr.options & IHDR_OPT_BIG_ENDIAN )
{
ERROR("Unable to handle big endian streams");
return -1;
@@ -345,12 +348,14 @@ static int handle_page_data(struct xc_sr_context *ctx,
struct xc_sr_record *rec)
rec->length, sizeof(*pages));
goto err;
}
-else if ( pages->count < 1 )
+
+if ( pages->count < 1 )
{
ERROR("Expected at least 1 pfn in PAGE_DATA record");
goto err;
}
-else if ( rec->length < sizeof(*pages) + (pages->count * sizeof(uint64_t))
)
+
+if ( rec->length < sizeof(*pages) + (pages->count * sizeof(uint64_t)) )
{
ERROR("PAGE_DATA record (length %u) too short to contain %u"
" pfns worth of information", rec->length, pages->count);
@@ -383,7 +388,8 @@ static int handle_page_data(struct xc_sr_context *ctx,
struct xc_sr_record *rec)
type, pfn, i);
goto err;
}
-else if ( type < XEN_DOMCTL_PFINFO_BROKEN )
+
+if ( type < XEN_DOMCTL_PFINFO_BROKEN )
/* NOTAB and all L1 through L4 tables (including pinned) should
* have a page worth of data in the record. */
pages_of_data++;
diff --git a/tools/libxc/xc_sr_restore_x86_hvm.c
b/tools/libxc/xc_sr_restore_x86_hvm.c
index 4765a52f33..9763aaa8dc 100644
--- a/tools/libxc/xc_sr_restore_x86_hvm.c
+++ b/tools/libxc/xc_sr_restore_x86_hvm.c
@@ -142,12 +142,14 @@ static int x86_hvm_setup(struct xc_sr_context *ctx)
[Xen-devel] [PATCH] tools/libxc: Drop unused xc_compression_*()
There have been no users of the xc_compression_*() interface since Migration v2 replaced legacy migration (2016, c/s b15bc4345). It would need adjusting to fit into migration v2, and can be pulled out of git history if someone wants to resurrect it in the future. Signed-off-by: Andrew Cooper --- CC: Ian Jackson CC: Wei Liu --- tools/libxc/Makefile | 2 +- tools/libxc/include/xenctrl.h | 60 - tools/libxc/xc_compression.c | 545 -- 3 files changed, 1 insertion(+), 606 deletions(-) delete mode 100644 tools/libxc/xc_compression.c diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile index cbc30001f6..6d0af563c0 100644 --- a/tools/libxc/Makefile +++ b/tools/libxc/Makefile @@ -63,7 +63,7 @@ GUEST_SRCS-$(CONFIG_X86) += xc_sr_save_x86_pv.c GUEST_SRCS-$(CONFIG_X86) += xc_sr_save_x86_hvm.c GUEST_SRCS-y += xc_sr_restore.c GUEST_SRCS-y += xc_sr_save.c -GUEST_SRCS-y += xc_offline_page.c xc_compression.c +GUEST_SRCS-y += xc_offline_page.c else GUEST_SRCS-y += xc_nomigrate.c endif diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index f4431687b3..983f5ac6b0 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -2315,66 +2315,6 @@ void xc_elf_set_logfile(xc_interface *xch, struct elf_binary *elf, int verbose); /* Useful for callers who also use libelf. */ -/** - * Checkpoint Compression - */ -typedef struct compression_ctx comp_ctx; -comp_ctx *xc_compression_create_context(xc_interface *xch, - unsigned long p2m_size); -void xc_compression_free_context(xc_interface *xch, comp_ctx *ctx); - -/** - * Add a page to compression page buffer, to be compressed later. - * - * returns 0 if the page was successfully added to the page buffer - * - * returns -1 if there is no space in buffer. In this case, the - * application should call xc_compression_compress_pages to compress - * the buffer (or atleast part of it), thereby freeing some space in - * the page buffer. - * - * returns -2 if the pfn is out of bounds, where the bound is p2m_size - * parameter passed during xc_compression_create_context. - */ -int xc_compression_add_page(xc_interface *xch, comp_ctx *ctx, char *page, - unsigned long pfn, int israw); - -/** - * Delta compress pages in the compression buffer and inserts the - * compressed data into the supplied compression buffer compbuf, whose - * size is compbuf_size. - * After compression, the pages are copied to the internal LRU cache. - * - * This function compresses as many pages as possible into the - * supplied compression buffer. It maintains an internal iterator to - * keep track of pages in the input buffer that are yet to be compressed. - * - * returns -1 if the compression buffer has run out of space. - * returns 1 on success. - * returns 0 if no more pages are left to be compressed. - * When the return value is non-zero, compbuf_len indicates the actual - * amount of data present in compbuf (<=compbuf_size). - */ -int xc_compression_compress_pages(xc_interface *xch, comp_ctx *ctx, - char *compbuf, unsigned long compbuf_size, - unsigned long *compbuf_len); - -/** - * Resets the internal page buffer that holds dirty pages before compression. - * Also resets the iterators. - */ -void xc_compression_reset_pagebuf(xc_interface *xch, comp_ctx *ctx); - -/** - * Caller must supply the compression buffer (compbuf), - * its size (compbuf_size) and a reference to index variable (compbuf_pos) - * that is used internally. Each call pulls out one page from the compressed - * chunk and copies it to dest. - */ -int xc_compression_uncompress_page(xc_interface *xch, char *compbuf, - unsigned long compbuf_size, - unsigned long *compbuf_pos, char *dest); - /* * Execute an image previously loaded with xc_kexec_load(). * diff --git a/tools/libxc/xc_compression.c b/tools/libxc/xc_compression.c deleted file mode 100644 index 89c1114ebb..00 --- a/tools/libxc/xc_compression.c +++ /dev/null @@ -1,545 +0,0 @@ -/** - * xc_compression.c - * - * Checkpoint Compression using Page Delta Algorithm. - * - A LRU cache of recently dirtied guest pages is maintained. - * - For each dirty guest page in the checkpoint, if a previous version of the - * page exists in the cache, XOR both pages and send the non-zero sections - * to the receiver. The cache is then updated with the newer copy of guest page. - * - The receiver will XOR the non-zero sections against its copy of the guest - * page, thereby bringing the guest page up-to-date with the sender side. - * - * Copyright (c) 2011 Shriram Rajagopalan (rshri...@cs.ubc.ca). - * - * This library is free software; you can redistribute it and/or - * modify it under
Re: [Xen-devel] [OSSTEST PATCH 3/3] microcode: Install Debian microcode packages and add ucode=scan
On 19/12/2019 17:57, Ian Jackson wrote: > We are no longer using the frozen-in-amber microcode from 2015. Now > we use current microcode from Debian (or hopefully in future via other > distros). > > Empirically this fixes the XSA-308 test on rimava1, which was failing > and producing very strange symptoms. > > CC: Andrew Cooper > Signed-off-by: Ian Jackson Acked-by: Andrew Cooper (for both) This is a definite improvement over the status quo. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 04/20] x86/mem_sharing: cleanup code and comments in various locations
On Thu, Dec 19, 2019 at 4:19 AM Andrew Cooper wrote:
>
> On 18/12/2019 19:40, Tamas K Lengyel wrote:
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index 5a3a962fbb..1e888b403b 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -1902,12 +1902,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned
> > long gla,
> > if ( npfec.write_access && (p2mt == p2m_ram_shared) )
> > {
> > ASSERT(p2m_is_hostp2m(p2m));
> > -sharing_enomem =
> > -(mem_sharing_unshare_page(currd, gfn, 0) < 0);
> > +sharing_enomem = mem_sharing_unshare_page(currd, gfn, 0);
>
> This is a logical change. Is it intended to be in a later patch?
While it may look like one it's actually not. The variable
sharing_enomem is declared as an int and the function only has two
possible return values, 0 and -ENOMEM.
Tamas
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 19/20] x86/mem_sharing: reset a fork
> >>> The alternative would be that we just release a fork (or just > >>> the patches) and walk away. > >>> If the Xen community wants to make the > >>> announcement that only code that will have long term support and is > >>> "stable" is accepted upstream that's IMHO drastically going to reduce > >>> people's interest to share anything. > >> > >> Sharing is one thing, but if this code is not at least a minimum > >> maintained then it is likely the code will not be functional in a year > >> time. > > > > Surprisingly mem_sharing had only minor bitrots in the last ~5 years > > in which time it has been pretty much abandoned. > This falls under a "minimum maintained". This wasn't clear from your > previous statement stating there will be no support. Sure, I meant there is no support from Intel (ie. it's not part of my job-description nor do I get payed to support this long-term). I usually check during the RC test days that it's at least functional by doing some testing manually. But it's pretty ad-hoc when and if I do that (hence the Odd fixes status). > > > > But if others feel that strongly as well about > > having to have continuation for this I don't really mind adding it. > > I don't think the continuation work is going to be difficult, but if you > want to delay it, then the minimum is to document such assumptions for > your users. > >>> > >>> I just don't see a use for it because it will never actually execute. > >> > >> That's a very narrow view of how your hypercall can be used. I know that > >> you said that should only be called early, but imagine for a moment the > >> user decide to call it much later in the fork process. > >> > >>> So to me it just looks like unnecessary dead glue. > >> > >> Try to call the hypercall after enough deduplication happen (maybe > >> 20min). Alternatively, give me access to your machine with the code and > >> I can show how it can be misused ;). > > > > It will hang for a bit for sure and Linux in dom0 will complain that a > > CPU is stuck. But it will eventually finish. It's not like it's doing > > all that much. And anyway, if you notice that happening when you call > > it it will be an obvious clue that you shouldn't be using it under the > > situation you are using it under. Having continuation would hide that. > > I am not going to argue more as this is an experimental feature. But > this will be a showstopper if we ever consider mem_sharing to be > supported (or even security supported). > > Meanwhile please document the assumption. Ack, already did. Thanks, Tamas ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [OSSTEST PATCH 1/3] Provide target_install_packages_nonfree_nonconcurrent
Right now this is only useful with Debian. We will call it from
ts-host-install to install the microcode.
We enable and then disable non-free so that we don't install non-free
packages unintentionally.
Signed-off-by: Ian Jackson
---
Osstest/TestSupport.pm | 16
1 file changed, 16 insertions(+)
diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 44f01a86..63087260 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -71,6 +71,7 @@ BEGIN {
target_editfile_kvp_replace
target_run_pkgmanager_install
target_install_packages target_install_packages_norec
+ target_install_packages_nonfree_nonconcurrent
target_jobdir target_extract_jobdistpath_subdir
target_extract_jobdistpath target_extract_distpart
target_tftp_prefix
@@ -627,6 +628,21 @@ sub target_install_packages_norec ($@) {
my ($ho, @packages) = @_;
target_run_pkgmanager_install($ho,\@packages,1);
}
+sub target_install_packages_nonfree_nonconcurrent ($@) {
+# inadequate locking, should be called during installation only
+my ($ho, @packages) = @_;
+my $slist= '/etc/apt/sources.list';
+my $xsuites= 'contrib non-free';
+target_cmd_root($ho,
[Xen-devel] [OSSTEST PATCH 2/3] Revert "Arrange to upgrade microcode on x86 test hosts."
We are going to do this differently, by installing the packages from
Debian. We couldn't do that in 2015 because not every version of Xen
supported scanning the dom0 initrd for microcode, but now all
supported versions do. So we will do that, in a moment.
This reverts commit 6cedb8c7d76a0ab3820ed0a48a3aebcb18fa0813.
CC: Andrew Cooper
Signed-off-by: Ian Jackson
---
Osstest/Debian.pm | 44
mg-cpu-microcode-update | 83 -
production-config | 5 ---
production-config-cambridge | 4 ---
ts-xen-install | 7
5 files changed, 143 deletions(-)
delete mode 100755 mg-cpu-microcode-update
diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
index c22aaadf..6e9d2072 100644
--- a/Osstest/Debian.pm
+++ b/Osstest/Debian.pm
@@ -753,48 +753,6 @@ chroot /target chown -R \$u.\$u \$h/.ssh
END
}
-sub preseed_microcode($$)
-{
-my ($ho,$sfx) = @_;
-my $prop = "MicrocodeUpdate".ucfirst($ho->{Arch});
-return unless $c{$prop};
-logm("ucode=$prop $c{$prop}");
-my $ucode = get_filecontents("$c{Images}/$c{$prop}");
-my $cpio_url = create_webfile($ho, "microcode-cpio",
- sub {
- my $f = "$c{Images}/$c{$prop}";
- copy($f, $_[0]) or die "Copy $f failed: $!";
-});
-
-# The ability to prepend from an initramfs-hook was not added
-# until Jessie, therefore for Wheezy we use a custom compression
-# method which sneaks the necessary cpio onto the front.
-my $gzip_url = create_webfile($ho, "microcode-gzip",<> \\
- /target/etc/initramfs-tools/conf.d/osstest-initramfs-gzip.conf
-END
-}
-
sub debian_overlays ($$) {
my ($ho, $func) = @_;
# calls $func->($local_dir, $tarball_leafname);
@@ -1503,8 +1461,6 @@ d-i partman-auto/expert_recipe string
\\
END
-preseed_microcode($ho,$sfx);
-
if (get_host_property($ho, "firmware") eq "uefi") {
die unless $ho->{Suite} =~ m/jessie|stretch/;
# Prevent grub-install from making a new Debian boot entry, so
diff --git a/mg-cpu-microcode-update b/mg-cpu-microcode-update
deleted file mode 100755
index 1032025f..
--- a/mg-cpu-microcode-update
+++ /dev/null
@@ -1,83 +0,0 @@
-#!/bin/bash
-
-set -e -o posix
-
-. ./cri-getconfig
-. ./mgi-common
-
-# iucode_tool is in /usr/sbin, see #788459.
-export PATH="/usr/local/sbin:$PATH:/sbin:/usr/sbin"
-
-images=`getconfig Images`
-gitproxy=`getconfig GitCacheProxy`
-date=`date +%Y-%m-%d`
-
-cpiodir=cpio.x86
-ucodepath=kernel/x86/microcode
-
-intelbin=GenuineIntel.bin
-amdbin=AuthenticAMD.bin
-
-ucodecpio=$images/microcode.x86.$date.cpio
-
-linuxfw=git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
-
-rm -rf $ucodecpio.tmp
-mkdir $ucodecpio.tmp
-cd $ucodecpio.tmp
-
-mkdir -p $cpiodir/$ucodepath/
-
-# Intel
-#
-# From http://feeds.downloadcenter.intel.com/rss/?p=483=eng look for
-# Linux* Processor Microcode Data File which will take you to e.g.
-#
https://downloadcenter.intel.com/downloads/eula/24661/Linux-Processor-Microcode-Data-File?httpDown=http%3A%2F%2Fdownloadmirror.intel.com%2F24661%2Feng%2Fmicrocode-20150121.tgz'
which redirects to the following:
-INTEL_TGZ='http://downloadmirror.intel.com/24661/eng/microcode-20150121.tgz'
-
-# The microcode-MMDD.tgz contains a microcode.dat which we must
-# then convert to the appropriate binary format using iucode_tool
-# (available in Debian).
-
-mkdir intel-ucode
-
-echo >&2 "Fetching Intel ucode"
-fetch $INTEL_TGZ > intel-ucode/microcode.tgz
-
-tar -C intel-ucode -xaf intel-ucode/microcode.tgz microcode.dat
-
-echo >&2 "Converting Intel ucode"
-iucode_tool -t d -q \
---write-to=$cpiodir/$ucodepath/$intelbin \
-intel-ucode/microcode.dat
-
-# AMD
-#
-# http://xenbits.xen.org/docs/unstable/misc/amd-ucode-container.txt
-#
-# From linux-firmware.git:
-AMD_BINS='
- amd-ucode/microcode_amd.bin
- amd-ucode/microcode_amd_fam15h.bin
- amd-ucode/microcode_amd_fam16h.bin
-'
-
-echo >&2 "Cloning $gitproxy$linuxfw"
-git clone --quiet --depth=1 $gitproxy$linuxfw linux-firmware
-
-# Concatenate into $ucodepath/$amdbin within the
-# cpio.
-echo >&2 "Conveting AMD ucode"
-( cd linux-firmware && cat $AMD_BINS ) > $cpiodir/$ucodepath/$amdbin
-
-# Ensure the cpio is reproducible, we don't care about timestamps
-# inside the archive.
-find $cpiodir -exec touch -d @0 {} \;
-
-echo >&2 "Building cpio archive"
-( cd $cpiodir && find . | cpio -o -H newc --quiet > $ucodecpio )
-
-echo "New x86 microcode: $ucodecpio"
-
-cd $images # Get out of $ucodecpio.tmp
-rm -rf $ucodecpio.tmp
diff --git a/production-config b/production-config
index bd200182..41f68409 100644
--- a/production-config
+++ b/production-config
@@ -99,11 +99,6 @@ DebianImageVersion_wheezy 7.2.0
DebianImageVersion_jessie 8.2.0
DebianImageVersion_stretch 9.4.0
-# These should normally be the same.
-# Update with ./mg-cpu-microcode-update
-MicrocodeUpdateAmd64
[Xen-devel] [OSSTEST PATCH 3/3] microcode: Install Debian microcode packages and add ucode=scan
We are no longer using the frozen-in-amber microcode from 2015. Now
we use current microcode from Debian (or hopefully in future via other
distros).
Empirically this fixes the XSA-308 test on rimava1, which was failing
and producing very strange symptoms.
CC: Andrew Cooper
Signed-off-by: Ian Jackson
---
ts-host-install | 6 ++
ts-xen-install | 2 ++
2 files changed, 8 insertions(+)
diff --git a/ts-host-install b/ts-host-install
index 4bfa2f5e..7a72a867 100755
--- a/ts-host-install
+++ b/ts-host-install
@@ -139,6 +139,12 @@ END
target_cmd_root($ho, "chmod 2775 /root");
target_install_packages($ho, qw(ed));
+if ($ho->{Arch} =~ m/^(?:i386|amd64)$/) {
+ # We don't necessarily know the CPU vendor, but the Debian
+ # packaged microcode doesn't mind us installing both.
+ target_install_packages_nonfree_nonconcurrent($ho,
+qw(amd64-microcode intel-microcode));
+}
my $ntpserver = get_target_property($ho, 'NtpServer');
if ($ntpserver) {
diff --git a/ts-xen-install b/ts-xen-install
index 9113f318..08b4ea23 100755
--- a/ts-xen-install
+++ b/ts-xen-install
@@ -209,6 +209,8 @@ sub setupboot () {
my $mem = $r{'dom0_mem'} // 512;
$xenhopt .= " dom0_mem=${mem}M,max:${mem}M";
}
+$xenhopt .= " ucode=scan";
+
my $append= $r{xen_boot_append};
$xenhopt .= " $append" if defined $append;
$append = get_host_property($ho, 'xen-commandline-append', undef);
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 19/20] x86/mem_sharing: reset a fork
On Thu, Dec 19, 2019 at 9:58 AM Julien Grall wrote: > > Hi, > > On 19/12/2019 16:11, Tamas K Lengyel wrote: > >>> Well, this is only an experimental system that's completely disabled > >>> by default. Making the assumption that people who make use of it will > >>> know what they are doing I think is fair. > >> > >> I assume that if you submit to upstream this new hypercall then there is > >> longer plan to have more people to use it and potentially making > >> "stable". If not, then it raises the question why this is pushed > >> upstream... > > > > It's being pushed upstream so other people can make use of it, even if > > it's not "production quality". Beyond what is being sent here there > > are no longer term plans from Intel (at this point) to support this in > > any way. > > So if this is merged, who is going to maintain it? It falls under mem_sharing for which I'm listed as "Odd Fixes" maintainer, which I do in my personal free time. The status there isn't changing with this new feature. > > > The alternative would be that we just release a fork (or just > > the patches) and walk away. > > If the Xen community wants to make the > > announcement that only code that will have long term support and is > > "stable" is accepted upstream that's IMHO drastically going to reduce > > people's interest to share anything. > > Sharing is one thing, but if this code is not at least a minimum > maintained then it is likely the code will not be functional in a year time. Surprisingly mem_sharing had only minor bitrots in the last ~5 years in which time it has been pretty much abandoned. > > Don't get me wrong, this is a cool feature to have but you make it > sounds like this is going to be dumped in Xen and never going to be > touched again. How is this going to be beneficial for the community? It adds an experimental feature to Xen that people can try out and well experiment with! It may be useful, it may not be. You yourself said that this is a cool feature. The fact that there is a JIRA ticket tracking this also shows there is community interest in having such a feature available. If down the road that changes and this proves to be dead code, it can be removed. I don't think that will be necessary since this isn't even compiled by default anymore though. But anyway, it makes more sense to get it upstream then carry it out of tree because it gets more exposure that way, more people may try it out. Having it in-tree also means that in a couple releases people who are interested in the feature don't have to go through the process of rebasing the patchset on newer versions of Xen since it's already in-tree. > >>> But if others feel that strongly as well about > >>> having to have continuation for this I don't really mind adding it. > >> > >> I don't think the continuation work is going to be difficult, but if you > >> want to delay it, then the minimum is to document such assumptions for > >> your users. > > > > I just don't see a use for it because it will never actually execute. > > That's a very narrow view of how your hypercall can be used. I know that > you said that should only be called early, but imagine for a moment the > user decide to call it much later in the fork process. > > > So to me it just looks like unnecessary dead glue. > > Try to call the hypercall after enough deduplication happen (maybe > 20min). Alternatively, give me access to your machine with the code and > I can show how it can be misused ;). It will hang for a bit for sure and Linux in dom0 will complain that a CPU is stuck. But it will eventually finish. It's not like it's doing all that much. And anyway, if you notice that happening when you call it it will be an obvious clue that you shouldn't be using it under the situation you are using it under. Having continuation would hide that. > > > But documenting the > > assumption under which this hypercall should execute is perfectly > > fair. > > You might want to think how the interface would look like with the > preemption. So the day you decide to introduce preemption you don't have > to create a new hypercall. Why would you need to introduce a new hypercall if preemption becomes necessary? This is not a stable interfaces. It can be removed/changed completely from one Xen release to the next. Tamas ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v3] x86/save: reserve HVM save record numbers that have been consumed...
...for patches not (yet) upstream.
This patch is simply adding a comment to reserve save record number space
to avoid the risk of clashes between existent downstream changes made by
Amazon and future upstream changes which may be incompatible.
Signed-off-by: Paul Durrant
Reviewed-by: Wei Liu
---
Cc: Jan Beulich
Cc: Andrew Cooper
Cc: "Roger Pau Monné"
v3:
- Reduced range and clarified inclusivity
v2:
- Reduce patch to just a comment
---
xen/include/public/arch-x86/hvm/save.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/xen/include/public/arch-x86/hvm/save.h
b/xen/include/public/arch-x86/hvm/save.h
index b2ad3fcd74..468c28dedb 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -639,6 +639,8 @@ struct hvm_msr {
#define CPU_MSR_CODE 20
+/* Range 22 - 34 (inclusive) reserved for Amazon */
+
/*
* Largest type-code in use
*/
--
2.20.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 19/20] x86/mem_sharing: reset a fork
On 19/12/2019 17:23, Tamas K Lengyel wrote: On Thu, Dec 19, 2019 at 9:58 AM Julien Grall wrote: Hi, On 19/12/2019 16:11, Tamas K Lengyel wrote: Well, this is only an experimental system that's completely disabled by default. Making the assumption that people who make use of it will know what they are doing I think is fair. I assume that if you submit to upstream this new hypercall then there is longer plan to have more people to use it and potentially making "stable". If not, then it raises the question why this is pushed upstream... It's being pushed upstream so other people can make use of it, even if it's not "production quality". Beyond what is being sent here there are no longer term plans from Intel (at this point) to support this in any way. So if this is merged, who is going to maintain it? It falls under mem_sharing for which I'm listed as "Odd Fixes" maintainer, which I do in my personal free time. The status there isn't changing with this new feature. The alternative would be that we just release a fork (or just the patches) and walk away. If the Xen community wants to make the announcement that only code that will have long term support and is "stable" is accepted upstream that's IMHO drastically going to reduce people's interest to share anything. Sharing is one thing, but if this code is not at least a minimum maintained then it is likely the code will not be functional in a year time. Surprisingly mem_sharing had only minor bitrots in the last ~5 years in which time it has been pretty much abandoned. This falls under a "minimum maintained". This wasn't clear from your previous statement stating there will be no support. [...] But if others feel that strongly as well about having to have continuation for this I don't really mind adding it. I don't think the continuation work is going to be difficult, but if you want to delay it, then the minimum is to document such assumptions for your users. I just don't see a use for it because it will never actually execute. That's a very narrow view of how your hypercall can be used. I know that you said that should only be called early, but imagine for a moment the user decide to call it much later in the fork process. So to me it just looks like unnecessary dead glue. Try to call the hypercall after enough deduplication happen (maybe 20min). Alternatively, give me access to your machine with the code and I can show how it can be misused ;). It will hang for a bit for sure and Linux in dom0 will complain that a CPU is stuck. But it will eventually finish. It's not like it's doing all that much. And anyway, if you notice that happening when you call it it will be an obvious clue that you shouldn't be using it under the situation you are using it under. Having continuation would hide that. I am not going to argue more as this is an experimental feature. But this will be a showstopper if we ever consider mem_sharing to be supported (or even security supported). Meanwhile please document the assumption. But documenting the assumption under which this hypercall should execute is perfectly fair. You might want to think how the interface would look like with the preemption. So the day you decide to introduce preemption you don't have to create a new hypercall. Why would you need to introduce a new hypercall if preemption becomes necessary? This is not a stable interfaces. It can be removed/changed completely from one Xen release to the next. Sorry, I didn't realize the mem_sharing code was not a stable interfaces. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 04/20] x86/mem_sharing: cleanup code and comments in various locations
On Thu, Dec 19, 2019 at 4:19 AM Andrew Cooper wrote:
>
> On 18/12/2019 19:40, Tamas K Lengyel wrote:
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index 5a3a962fbb..1e888b403b 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -1902,12 +1902,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned
> > long gla,
> > if ( npfec.write_access && (p2mt == p2m_ram_shared) )
> > {
> > ASSERT(p2m_is_hostp2m(p2m));
> > -sharing_enomem =
> > -(mem_sharing_unshare_page(currd, gfn, 0) < 0);
> > +sharing_enomem = mem_sharing_unshare_page(currd, gfn, 0);
>
> This is a logical change. Is it intended to be in a later patch?
>
> > diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> > index efb8821768..319aaf3074 100644
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -198,24 +200,26 @@ static inline shr_handle_t get_next_handle(void)
> > #define mem_sharing_enabled(d) \
> > (is_hvm_domain(d) && (d)->arch.hvm.mem_sharing_enabled)
> >
> > -static atomic_t nr_saved_mfns = ATOMIC_INIT(0);
> > +static atomic_t nr_saved_mfns = ATOMIC_INIT(0);
> > static atomic_t nr_shared_mfns = ATOMIC_INIT(0);
> >
> > -/** Reverse map **/
> > -/* Every shared frame keeps a reverse map (rmap) of tuples
> > that
> > +/*
> > + * Reverse map
> > + *
> > + * Every shared frame keeps a reverse map (rmap) of tuples
> > that
> > * this shared frame backs. For pages with a low degree of sharing, a O(n)
> > * search linked list is good enough. For pages with higher degree of
> > sharing,
> > - * we use a hash table instead. */
> > + * we use a hash table instead.
> > + */
> >
> > typedef struct gfn_info
> > {
> > unsigned long gfn;
> > -domid_t domain;
> > +domid_t domain;
> > struct list_head list;
> > } gfn_info_t;
> >
> > -static inline void
> > -rmap_init(struct page_info *page)
> > +static inline void rmap_init(struct page_info *page)
>
> Seeing as you're folding this, the inline can be dropped. In .c files,
> functions should just be static.
>
> > @@ -437,25 +441,29 @@ static inline void mem_sharing_gfn_destroy(struct
> > page_info *page,
> > xfree(gfn_info);
> > }
> >
> > -static struct page_info* mem_sharing_lookup(unsigned long mfn)
> > +static inline struct page_info* mem_sharing_lookup(unsigned long mfn)
>
> Seeing as this is cleanup, the position of the * can move. Similarly,
> it shouldn't gain an inline.
>
> I can fix all of this up on commit (and a few other brace position
> issues) if you want, to save reworking a trivial v2.
>
Provided nothing else is outstanding with the patch and it can be
committed I would certainly appreciate that.
Thanks,
Tamas
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 18/20] xen/mem_access: Use __get_gfn_type_access in set_mem_access
On Thu, Dec 19, 2019 at 12:59 AM Alexandru Stefan ISAILA
wrote:
>
>
>
> On 18.12.2019 21:40, Tamas K Lengyel wrote:
> > Use __get_gfn_type_access instead of p2m->get_entry to trigger page-forking
> > when the mem_access permission is being set on a page that has not yet been
> > copied over from the parent.
> >
> > Signed-off-by: Tamas K Lengyel > Alexandru Isaila
>
> > ---
> > xen/arch/x86/mm/mem_access.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> > index 320b9fe621..9caf08a5b2 100644
> > --- a/xen/arch/x86/mm/mem_access.c
> > +++ b/xen/arch/x86/mm/mem_access.c
> > @@ -303,11 +303,10 @@ static int set_mem_access(struct domain *d, struct
> > p2m_domain *p2m,
> > ASSERT(!ap2m);
> > #endif
> > {
> > -mfn_t mfn;
> > p2m_access_t _a;
> > p2m_type_t t;
> > -
> > -mfn = p2m->get_entry(p2m, gfn, , &_a, 0, NULL, NULL);
> > +mfn_t mfn = __get_gfn_type_access(p2m, gfn_x(gfn), , &_a,
> > + P2M_ALLOC, NULL, false);
>
> Don't you want p2m_query_t to be 0 as it was in the p2m->get_entry() call ?
No, the entire point of the patch is to have the P2M_ALLOC query flag
set. That triggers the fork's p2m population.
Tamas
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen-bus/block: explicitly assign event channels to an AioContext
On Mon, Dec 16, 2019 at 02:34:51PM +, Paul Durrant wrote: > It is not safe to close an event channel from the QEMU main thread when > that channel's poller is running in IOThread context. > > This patch adds a new xen_device_set_event_channel_context() function > to explicitly assign the channel AioContext, and modifies > xen_device_bind_event_channel() to initially assign the channel's poller > to the QEMU main thread context. The code in xen-block's dataplane is > then modified to assign the channel to IOThread context during > xen_block_dataplane_start() and de-assign it during in > xen_block_dataplane_stop(), such that the channel is always assigned > back to main thread context before it is closed. aio_set_fd_handler() > already deals with all the necessary synchronization when moving an fd > between AioContext-s so no extra code is needed to manage this. > > Reported-by: Julien Grall > Signed-off-by: Paul Durrant Reviewed-by: Anthony PERARD > Tested against an HVM debian guest with a QCOW2 image as system disk, and > as a hot-plugged/unplgged secondary disk. And I've run an osstest flight with the patch. Thanks, -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 19/20] x86/mem_sharing: reset a fork
Hi, On 19/12/2019 16:11, Tamas K Lengyel wrote: Well, this is only an experimental system that's completely disabled by default. Making the assumption that people who make use of it will know what they are doing I think is fair. I assume that if you submit to upstream this new hypercall then there is longer plan to have more people to use it and potentially making "stable". If not, then it raises the question why this is pushed upstream... It's being pushed upstream so other people can make use of it, even if it's not "production quality". Beyond what is being sent here there are no longer term plans from Intel (at this point) to support this in any way. So if this is merged, who is going to maintain it? The alternative would be that we just release a fork (or just the patches) and walk away. If the Xen community wants to make the announcement that only code that will have long term support and is "stable" is accepted upstream that's IMHO drastically going to reduce people's interest to share anything. Sharing is one thing, but if this code is not at least a minimum maintained then it is likely the code will not be functional in a year time. Don't get me wrong, this is a cool feature to have but you make it sounds like this is going to be dumped in Xen and never going to be touched again. How is this going to be beneficial for the community? In any case, all the known assumptions should be documented so they can be fixed rather than forgotten until it is rediscovered via an XSA. Fair enough. Granted the list can grow larger, but in those cases its likely better to just discard the fork and create a new one. So in my opinion adding a hypercall continuation to this not needed How would the caller know it? What would happen if the caller ends up to call this with a growing list. The caller knows by virtue of knowing how long the VM was executed for. In the usecase this is targeted at the VM was executing only for a couple seconds at most. Usually much less then that (we get about ~80 resets/s with AFL). During that time its extremely unlikely you get more then a ~100 pages deduplicated (that is, written to). But even if there are more pages, it just means the hypercall might take a bit longer to run for that iteration. I assume if you upstream the code then you want more people to use it (otherwise what's the point?). In this case, you will likely have people that heard about the feature, wants to test but don't know the internal. Such users need to know how this can be call safely without reading the implementation. In other words, some documentation for your hypercall is needed. Sure. I don't see any issue with not breaking up this hypercall with continuation even under the worst case situation though. Xen only supports voluntary preemption, this means that an hypercall can only be preempted if there is code for it. Otherwise the preemption will mostly only happen when returning to the guest. In other words, the vCPU executing the hypercall may go past its timeslice and prevent other vCPU to run. There are other problem with long running hypercalls. Anyway, in short, if you ever want to get you code supported then you will need the hypercall to be broken down. But if others feel that strongly as well about having to have continuation for this I don't really mind adding it. I don't think the continuation work is going to be difficult, but if you want to delay it, then the minimum is to document such assumptions for your users. I just don't see a use for it because it will never actually execute. That's a very narrow view of how your hypercall can be used. I know that you said that should only be called early, but imagine for a moment the user decide to call it much later in the fork process. So to me it just looks like unnecessary dead glue. Try to call the hypercall after enough deduplication happen (maybe 20min). Alternatively, give me access to your machine with the code and I can show how it can be misused ;). But documenting the assumption under which this hypercall should execute is perfectly fair. You might want to think how the interface would look like with the preemption. So the day you decide to introduce preemption you don't have to create a new hypercall. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [XEN PATCH v4] x86/vm_event: add short-circuit for breakpoints (aka "fast single step")
On Thu, Dec 19, 2019 at 8:47 AM Sergey Kovalev wrote: > > When using DRAKVUF (or another system using altp2m with shadow pages similar > to what is described in > https://xenproject.org/2016/04/13/stealthy-monitoring-with-xen-altp2m), > after a breakpoint is hit the system switches to the default > unrestricted altp2m view with singlestep enabled. When the singlestep > traps to Xen another vm_event is sent to the monitor agent, which then > normally disables singlestepping and switches the altp2m view back to > the restricted view. > > This patch short-circuiting that last part so that it doesn't need to send the > vm_event out for the singlestep event and should switch back to the restricted > view in Xen automatically. > > This optimization gains about 35% speed-up. > > Was tested on Debian branch of Xen 4.12. See at: > https://github.com/skvl/xen/tree/debian/knorrie/4.12/fast-singlestep > > Rebased on master: > https://github.com/skvl/xen/tree/fast-singlestep > > Signed-off-by: Sergey Kovalev Acked-by: Tamas K Lengyel ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 19/20] x86/mem_sharing: reset a fork
> > Well, this is only an experimental system that's completely disabled > > by default. Making the assumption that people who make use of it will > > know what they are doing I think is fair. > > I assume that if you submit to upstream this new hypercall then there is > longer plan to have more people to use it and potentially making > "stable". If not, then it raises the question why this is pushed upstream... It's being pushed upstream so other people can make use of it, even if it's not "production quality". Beyond what is being sent here there are no longer term plans from Intel (at this point) to support this in any way. The alternative would be that we just release a fork (or just the patches) and walk away. If the Xen community wants to make the announcement that only code that will have long term support and is "stable" is accepted upstream that's IMHO drastically going to reduce people's interest to share anything. > In any case, all the known assumptions should be documented so they can > be fixed rather than forgotten until it is rediscovered via an XSA. Fair enough. > > > >> > >>> Granted the list can grow larger, but in those cases its likely better > >>> to just discard the fork and create a new one. So in my opinion adding > >>> a hypercall continuation to this not needed > >> > >> How would the caller know it? What would happen if the caller ends up to > >> call this with a growing list. > > > > The caller knows by virtue of knowing how long the VM was executed > > for. In the usecase this is targeted at the VM was executing only for > > a couple seconds at most. Usually much less then that (we get about > > ~80 resets/s with AFL). During that time its extremely unlikely you > > get more then a ~100 pages deduplicated (that is, written to). But > > even if there are more pages, it just means the hypercall might take a > > bit longer to run for that iteration. > > I assume if you upstream the code then you want more people to use it > (otherwise what's the point?). In this case, you will likely have people > that heard about the feature, wants to test but don't know the internal. > > Such users need to know how this can be call safely without reading the > implementation. In other words, some documentation for your hypercall is > needed. Sure. > > > I don't see any issue with not > > breaking up this hypercall with continuation even under the worst case > > situation though. > > Xen only supports voluntary preemption, this means that an hypercall can > only be preempted if there is code for it. Otherwise the preemption will > mostly only happen when returning to the guest. > > In other words, the vCPU executing the hypercall may go past its > timeslice and prevent other vCPU to run. > > There are other problem with long running hypercalls. Anyway, in short, > if you ever want to get you code supported then you will need the > hypercall to be broken down. > > > But if others feel that strongly as well about > > having to have continuation for this I don't really mind adding it. > > I don't think the continuation work is going to be difficult, but if you > want to delay it, then the minimum is to document such assumptions for > your users. I just don't see a use for it because it will never actually execute. So to me it just looks like unnecessary dead glue. But documenting the assumption under which this hypercall should execute is perfectly fair. Tamas ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 1/3] x86/xen: add basic KASAN support for PV kernel
On 18/12/2019 09:24, Jürgen Groß wrote:
> On 17.12.19 15:08, Sergey Dyasli wrote:
>> This enables to use Outline instrumentation for Xen PV kernels.
>>
>> KASAN_INLINE and KASAN_VMALLOC options currently lead to boot crashes
>> and hence disabled.
>>
>> Rough edges in the patch are marked with XXX.
>>
>> Signed-off-by: Sergey Dyasli
>> ---
>> arch/x86/mm/init.c | 14 ++
>> arch/x86/mm/kasan_init_64.c | 28
>> arch/x86/xen/Makefile | 7 +++
>> arch/x86/xen/enlighten_pv.c | 3 +++
>> arch/x86/xen/mmu_pv.c | 13 +++--
>> arch/x86/xen/multicalls.c | 10 ++
>> drivers/xen/Makefile| 2 ++
>> kernel/Makefile | 2 ++
>> lib/Kconfig.kasan | 3 ++-
>> 9 files changed, 79 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
>> index e7bb483557c9..0c98a45eec6c 100644
>> --- a/arch/x86/mm/init.c
>> +++ b/arch/x86/mm/init.c
>> @@ -8,6 +8,8 @@
>> #include
>> #include
>> +#include
>> +
>> #include
>> #include
>> #include
>> @@ -835,6 +837,18 @@ void free_kernel_image_pages(const char *what, void
>> *begin, void *end)
>> unsigned long end_ul = (unsigned long)end;
>> unsigned long len_pages = (end_ul - begin_ul) >> PAGE_SHIFT;
>> +/*
>> + * XXX: skip this for now. Otherwise it leads to:
>> + *
>> + * (XEN) mm.c:2713:d157v0 Bad type (saw 8c01 != exp
>> e000) for mfn 36f40 (pfn 02f40)
>> + * (XEN) mm.c:1043:d157v0 Could not get page type PGT_writable_page
>> + * (XEN) mm.c:1096:d157v0 Error getting mfn 36f40 (pfn 02f40) from L1
>> entry 801036f40067 for l1e_owner d157, pg_owner d157
>> + *
>> + * and further #PF error: [PROT] [WRITE] in the kernel.
>> + */
>> +if (xen_pv_domain() && IS_ENABLED(CONFIG_KASAN))
>> +return;
>> +
>
> I guess this is related to freeing some kasan page tables without
> unpinning them?
Your guess was correct. Turned out that early_top_pgt which I pinned and made RO
is located in .init section and that was causing issues. Unpinning it and making
RW again right after kasan_init() switches to use init_top_pgt seem to fix this
issue.
>
>> free_init_pages(what, begin_ul, end_ul);
>> /*
>> diff --git a/arch/x86/mm/kasan_init_64.c b/arch/x86/mm/kasan_init_64.c
>> index cf5bc37c90ac..caee2022f8b0 100644
>> --- a/arch/x86/mm/kasan_init_64.c
>> +++ b/arch/x86/mm/kasan_init_64.c
>> @@ -13,6 +13,8 @@
>> #include
>> #include
>> +#include
>> +
>> #include
>> #include
>> #include
>> @@ -20,6 +22,9 @@
>> #include
>> #include
>> +#include
>> +#include
>> +
>> extern struct range pfn_mapped[E820_MAX_ENTRIES];
>> static p4d_t tmp_p4d_table[MAX_PTRS_PER_P4D] __initdata
>> __aligned(PAGE_SIZE);
>> @@ -305,6 +310,12 @@ static struct notifier_block kasan_die_notifier = {
>> };
>> #endif
>> +#ifdef CONFIG_XEN
>> +/* XXX: this should go to some header */
>> +void __init set_page_prot(void *addr, pgprot_t prot);
>> +void __init pin_pagetable_pfn(unsigned cmd, unsigned long pfn);
>> +#endif
>> +
>
> Instead of exporting those, why don't you ...
>
>> void __init kasan_early_init(void)
>> {
>> int i;
>> @@ -332,6 +343,16 @@ void __init kasan_early_init(void)
>> for (i = 0; pgtable_l5_enabled() && i < PTRS_PER_P4D; i++)
>> kasan_early_shadow_p4d[i] = __p4d(p4d_val);
>> +if (xen_pv_domain()) {
>> +/* PV page tables must have PAGE_KERNEL_RO */
>> +set_page_prot(kasan_early_shadow_pud, PAGE_KERNEL_RO);
>> +set_page_prot(kasan_early_shadow_pmd, PAGE_KERNEL_RO);
>> +set_page_prot(kasan_early_shadow_pte, PAGE_KERNEL_RO);
>
> add a function doing that to mmu_pv.c (e.g. xen_pv_kasan_early_init())?
Sounds like a good suggestion, but new functions still need some header for
declarations (xen/xen.h?). And kasan_map_early_shadow() will need exporting
through kasan.h as well, but that's probably not an issue.
>
>> +
>> +/* Add mappings to the initial PV page tables */
>> +kasan_map_early_shadow((pgd_t *)xen_start_info->pt_base);
>> +}
>> +
>> kasan_map_early_shadow(early_top_pgt);
>> kasan_map_early_shadow(init_top_pgt);
>> }
>> @@ -369,6 +390,13 @@ void __init kasan_init(void)
>> __pgd(__pa(tmp_p4d_table) | _KERNPG_TABLE));
>> }
>> +if (xen_pv_domain()) {
>> +/* PV page tables must be pinned */
>> +set_page_prot(early_top_pgt, PAGE_KERNEL_RO);
>> +pin_pagetable_pfn(MMUEXT_PIN_L4_TABLE,
>> + PFN_DOWN(__pa_symbol(early_top_pgt)));
>
> and another one like xen_pv_kasan_init() here.
Now there needs to be a 3rd function to unpin early_top_pgt.
>
>> +}
>> +
>> load_cr3(early_top_pgt);
>> __flush_tlb_all();
>> diff --git a/arch/x86/xen/Makefile b/arch/x86/xen/Makefile
>> index 084de77a109e..102fad0b0bca 100644
>> ---
Re: [Xen-devel] Recent cores-scheduling failures
On 19.12.19 13:45, Sergey Dyasli wrote: Hi Juergen, We recently did another quick test of core scheduling mode, and the following failures were found: 1. live-patch apply failures: (XEN) [ 1058.751974] livepatch: lp_1_1: Timed out on semaphore in CPU quiesce phase 30/31 (XEN) [ 1058.751982] livepatch: lp_1_1 finished REPLACE with rc=-16 2. ACPI S5 crash: https://paste.debian.net/1121748/ So in sched_slave() *vprev is already scrubbed. I have currently no idea how that could happen, is vprev->is_running should be cleared only a little bit later. Will look into it more.. Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] x86/time: update vtsc_last with cmpxchg and drop vtsc_lock
Now that vtsc_last is the only entity protected by vtsc_lock we can
simply update it using a single atomic operation and drop the spinlock
entirely. This is extremely important for the case of running nested
(e.g. shim instance with lots of vCPUs assigned) since if preemption
happens somewhere inside the critical section that would immediately
mean that other vCPU stop progressing (and probably being preempted
as well) waiting for the spinlock to be freed.
This fixes constant shim guest boot lockups with ~32 vCPUs if there is
vCPU overcommit present (which increases the likelihood of preemption).
Signed-off-by: Igor Druzhinin
---
v2: simplify the condition as suggested
---
xen/arch/x86/domain.c| 1 -
xen/arch/x86/time.c | 16 ++--
xen/include/asm-x86/domain.h | 1 -
3 files changed, 6 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 7cb7fd3..d9c6337 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -539,7 +539,6 @@ int arch_domain_create(struct domain *d,
INIT_PAGE_LIST_HEAD(>arch.relmem_list);
spin_lock_init(>arch.e820_lock);
-spin_lock_init(>arch.vtsc_lock);
/* Minimal initialisation for the idle domain. */
if ( unlikely(is_idle_domain(d)) )
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 216169a..63dd5a2 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -2130,19 +2130,15 @@ u64 gtsc_to_gtime(struct domain *d, u64 tsc)
uint64_t pv_soft_rdtsc(const struct vcpu *v, const struct cpu_user_regs *regs)
{
-s_time_t now = get_s_time();
+s_time_t old, new, now = get_s_time();
struct domain *d = v->domain;
-spin_lock(>arch.vtsc_lock);
-
-if ( (int64_t)(now - d->arch.vtsc_last) > 0 )
-d->arch.vtsc_last = now;
-else
-now = ++d->arch.vtsc_last;
-
-spin_unlock(>arch.vtsc_lock);
+do {
+old = d->arch.vtsc_last;
+new = now > d->arch.vtsc_last ? now : old + 1;
+} while ( cmpxchg(>arch.vtsc_last, old, new) != old );
-return gtime_to_gtsc(d, now);
+return gtime_to_gtsc(d, new);
}
bool clocksource_is_tsc(void)
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 3780287..e4da373 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -364,7 +364,6 @@ struct arch_domain
int tsc_mode;/* see include/asm-x86/time.h */
bool_t vtsc; /* tsc is emulated (may change after migrate) */
s_time_t vtsc_last; /* previous TSC value (guarantee monotonicity) */
-spinlock_t vtsc_lock;
uint64_t vtsc_offset;/* adjustment for save/restore/migrate */
uint32_t tsc_khz;/* cached guest khz for certain emulated or
hardware TSC scaling cases */
--
2.7.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 19/20] x86/mem_sharing: reset a fork
On Thu, Dec 19, 2019 at 4:05 AM Jan Beulich wrote: > > On 19.12.2019 01:15, Tamas K Lengyel wrote: > > On Wed, Dec 18, 2019 at 4:02 PM Julien Grall wrote: > >> On 18/12/2019 22:33, Tamas K Lengyel wrote: > >>> On Wed, Dec 18, 2019 at 3:00 PM Julien Grall wrote: > You also have multiple loop on the page_list in this function. Given the > number of page_list can be quite big, this is a call for hogging the > pCPU and an RCU lock on the domain vCPU running this call. > >>> > >>> There is just one loop over page_list itself, the second loop is on > >>> the internal list that is being built here which will be a subset. The > >>> list itself in fact should be small (in our tests usually <100). > >> > >> For a first, nothing in this function tells me that there will be only > >> 100 pages. But then, I don't think this is right to implement your > >> hypercall based only the "normal" scenario. You should also think about > >> the "worst" case scenario. > >> > >> In this case the worst case scenario is have hundreds of page in page_list. > > > > Well, this is only an experimental system that's completely disabled > > by default. Making the assumption that people who make use of it will > > know what they are doing I think is fair. > > FWIW I'm with Julien here: The preferred course of action is to make > the operation safe against abuse. The minimum requirement is to > document obvious missing pieces for this to become supported code. That's perfectly fine by me. Tamas ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 00/20] VM forking
On Thu, Dec 19, 2019 at 2:48 AM Roger Pau Monné wrote: > > On Wed, Dec 18, 2019 at 11:40:37AM -0800, Tamas K Lengyel wrote: > > The following series implements VM forking for Intel HVM guests to allow for > > the fast creation of identical VMs without the assosciated high startup > > costs > > of booting or restoring the VM from a savefile. > > > > JIRA issue: https://xenproject.atlassian.net/browse/XEN-89 > > > > The main design goal with this series has been to reduce the time of > > creating > > the VM fork as much as possible. To achieve this the VM forking process is > > split into two steps: > > 1) forking the VM on the hypervisor side; > > 2) starting QEMU to handle the backed for emulated devices. > > > > Step 1) involves creating a VM using the new "xl fork-vm" command. The > > parent VM is expected to remain paused after forks are created from it > > (which > > is different then what process forking normally entails). During this > > forking >^ than > > operation the HVM context and VM settings are copied over to the new forked > > VM. > > This operation is fast and it allows the forked VM to be unpaused and to be > > monitored and accessed via VMI. Note however that without its device model > > running (depending on what is executing in the VM) it is bound to > > misbehave/crash when its trying to access devices that would be emulated by > > QEMU. We anticipate that for certain use-cases this would be an acceptable > > situation, in case for example when fuzzing is performed of code segments > > that > > don't access such devices. > > > > Step 2) involves launching QEMU to support the forked VM, which requires the > > QEMU Xen savefile to be generated manually from the parent VM. This can be > > accomplished simply by connecting to its QMP socket and issuing the > > "xen-save-devices-state" command as documented by QEMU: > > https://github.com/qemu/qemu/blob/master/docs/xen-save-devices-state.txt > > Once the QEMU Xen savefile is generated the new "xl fork-launch-dm" command > > is > > used to launch QEMU and load the specified savefile for it. > > IMO having two different commands is confusing for the end user, I > would rather have something like: > > xl fork-vm [-d] ... > > Where '-d' would prevent forking any user-space emulators. I don't > thinks there's a need for a separate command to fork the underlying > user-space emulators. Keeping it as two commands allows you to start up the fork and let it run immediately and only start up QEMU when you notice it is needed. The idea being that you can monitor the kernel and see when it tries to do some I/O that would require the QEMU backend. If you combine the commands that option goes away. Also, QEMU itself isn't getting forked right now, we just start a new QEMU process with the saved-state getting loaded into it. I looked into implementing a QEMU fork command but it turns out that for the vast majority of our use-cases QEMU isn't needed at all, so developing that addition was tabled. Tamas ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [XEN PATCH v4] x86/vm_event: add short-circuit for breakpoints (aka "fast single step")
On 19.12.2019 16:47, Sergey Kovalev wrote: > When using DRAKVUF (or another system using altp2m with shadow pages similar > to what is described in > https://xenproject.org/2016/04/13/stealthy-monitoring-with-xen-altp2m), > after a breakpoint is hit the system switches to the default > unrestricted altp2m view with singlestep enabled. When the singlestep > traps to Xen another vm_event is sent to the monitor agent, which then > normally disables singlestepping and switches the altp2m view back to > the restricted view. > > This patch short-circuiting that last part so that it doesn't need to send the > vm_event out for the singlestep event and should switch back to the restricted > view in Xen automatically. > > This optimization gains about 35% speed-up. > > Was tested on Debian branch of Xen 4.12. See at: > https://github.com/skvl/xen/tree/debian/knorrie/4.12/fast-singlestep > > Rebased on master: > https://github.com/skvl/xen/tree/fast-singlestep > > Signed-off-by: Sergey Kovalev You've lost Acked-by: Jan Beulich for applicable bits, as given on v3 with the one issue addressed. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [XEN PATCH v4] x86/vm_event: add short-circuit for breakpoints (aka "fast single step")
When using DRAKVUF (or another system using altp2m with shadow pages similar
to what is described in
https://xenproject.org/2016/04/13/stealthy-monitoring-with-xen-altp2m),
after a breakpoint is hit the system switches to the default
unrestricted altp2m view with singlestep enabled. When the singlestep
traps to Xen another vm_event is sent to the monitor agent, which then
normally disables singlestepping and switches the altp2m view back to
the restricted view.
This patch short-circuiting that last part so that it doesn't need to send the
vm_event out for the singlestep event and should switch back to the restricted
view in Xen automatically.
This optimization gains about 35% speed-up.
Was tested on Debian branch of Xen 4.12. See at:
https://github.com/skvl/xen/tree/debian/knorrie/4.12/fast-singlestep
Rebased on master:
https://github.com/skvl/xen/tree/fast-singlestep
Signed-off-by: Sergey Kovalev
---
v1 Basic implementation
v2 Fix coding style issues and commit message
v3 Add check for P2M index
v4 Simplify code
---
xen/arch/x86/hvm/hvm.c | 15 +++
xen/arch/x86/hvm/monitor.c | 9 +
xen/arch/x86/vm_event.c| 8 ++--
xen/include/asm-x86/hvm/hvm.h | 1 +
xen/include/asm-x86/hvm/vcpu.h | 4
xen/include/public/vm_event.h | 14 ++
6 files changed, 49 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 47573f71b8..cb3aa06fd2 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5126,6 +5126,21 @@ void hvm_toggle_singlestep(struct vcpu *v)
v->arch.hvm.single_step = !v->arch.hvm.single_step;
}
+void hvm_fast_singlestep(struct vcpu *v, uint16_t p2midx)
+{
+ASSERT(atomic_read(>pause_count));
+
+if ( !hvm_is_singlestep_supported() )
+return;
+
+if ( p2midx >= MAX_ALTP2M )
+return;
+
+v->arch.hvm.single_step = true;
+v->arch.hvm.fast_single_step.enabled = true;
+v->arch.hvm.fast_single_step.p2midx = p2midx;
+}
+
/*
* Segment caches in VMCB/VMCS are inconsistent about which bits are checked,
* important, and preserved across vmentry/exit. Cook the values to make them
diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
index 1f23fe25e8..85996a3edd 100644
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -159,6 +160,14 @@ int hvm_monitor_debug(unsigned long rip, enum
hvm_monitor_debug_type type,
case HVM_MONITOR_SINGLESTEP_BREAKPOINT:
if ( !ad->monitor.singlestep_enabled )
return 0;
+if ( curr->arch.hvm.fast_single_step.enabled )
+{
+p2m_altp2m_check(curr, curr->arch.hvm.fast_single_step.p2midx);
+curr->arch.hvm.single_step = false;
+curr->arch.hvm.fast_single_step.enabled = false;
+curr->arch.hvm.fast_single_step.p2midx = 0;
+return 0;
+}
req.reason = VM_EVENT_REASON_SINGLESTEP;
req.u.singlestep.gfn = gfn_of_rip(rip);
sync = true;
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 52c2a71fa0..848d69c1b0 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -61,7 +61,8 @@ void vm_event_cleanup_domain(struct domain *d)
void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v,
vm_event_response_t *rsp)
{
-if ( !(rsp->flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP) )
+if ( !(rsp->flags & (VM_EVENT_FLAG_TOGGLE_SINGLESTEP |
+ VM_EVENT_FLAG_FAST_SINGLESTEP)) )
return;
if ( !is_hvm_domain(d) )
@@ -69,7 +70,10 @@ void vm_event_toggle_singlestep(struct domain *d, struct
vcpu *v,
ASSERT(atomic_read(>vm_event_pause_count));
-hvm_toggle_singlestep(v);
+if ( rsp->flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
+hvm_toggle_singlestep(v);
+else
+hvm_fast_singlestep(v, rsp->u.fast_singlestep.p2midx);
}
void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 1d7b66f927..09793c12e9 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -323,6 +323,7 @@ int hvm_debug_op(struct vcpu *v, int32_t op);
/* Caller should pause vcpu before calling this function */
void hvm_toggle_singlestep(struct vcpu *v);
+void hvm_fast_singlestep(struct vcpu *v, uint16_t p2midx);
int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
struct npfec npfec);
diff --git a/xen/include/asm-x86/hvm/vcpu.h b/xen/include/asm-x86/hvm/vcpu.h
index 38f5c2bb9b..8b8494 100644
--- a/xen/include/asm-x86/hvm/vcpu.h
+++ b/xen/include/asm-x86/hvm/vcpu.h
@@ -172,6 +172,10 @@ struct hvm_vcpu {
boolflag_dr_dirty;
booldebug_state_latch;
bool
Re: [Xen-devel] [OSSTEST PATCH] ts-xen-install: Drop gdb= parameter
On 19/12/2019 15:07, Ian Jackson wrote: > This has been there forever and I doubt anyone has ever used it. > > Andrew Cooper tells me that it needs an L or H suffix so probably > doesn't work. Recent changes to more conspicuously report command > line parsing failures highlighted this issue. > > Suggested-by: Andrew Cooper > Signed-off-by: Ian Jackson http://xenbits.xen.org/docs/unstable/misc/xen-command-line.html#gdb It needs an L or H suffix to instruct Xen how to mux the console with the gdb remote protocol based on the high bit on 8-bit clean line. Acked-by: Andrew Cooper ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [OSSTEST PATCH] ts-xen-install: Drop gdb= parameter
This has been there forever and I doubt anyone has ever used it.
Andrew Cooper tells me that it needs an L or H suffix so probably
doesn't work. Recent changes to more conspicuously report command
line parsing failures highlighted this issue.
Suggested-by: Andrew Cooper
Signed-off-by: Ian Jackson
---
ts-xen-install | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ts-xen-install b/ts-xen-install
index 154f78c7..d8e9d7e6 100755
--- a/ts-xen-install
+++ b/ts-xen-install
@@ -197,7 +197,7 @@ sub setupboot () {
: 'com1');
if ( $cons eq "com1" ) {
- $xenhopt .= " com1=$c{Baud},8n1 console=com1,vga gdb=com1";
+ $xenhopt .= " com1=$c{Baud},8n1 console=com1,vga";
} elsif ( $cons eq "dtuart" ) {
$xenhopt .= " console=dtuart";
my $dtuart= get_host_property($ho, 'XenDTUARTPath', undef);
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
Hi Lars, On 18/12/2019 17:09, Lars Kurth wrote: On 18/12/2019, 14:29, "Julien Grall" wrote: Hi Lars, On 12/12/2019 21:14, Lars Kurth wrote: > +### Workflow from an Author's Perspective > + > +When code authors receive feedback on their patches, they typically first try > +to clarify feedback they do not understand. For smaller patches or patch series > +it makes sense to wait until receiving feedback on the entire series before > +sending out a new version addressing the changes. For larger series, it may > +make sense to send out a new revision earlier. > + > +As a reviewer, you need some system that he;ps ensure that you address all Just a small typo: I think you meant "helps" rather than "he;ps". Cheers, Thank you: fixed in my working copy. One thing which occurred to me for reviews like these, where there is no ACK's or Reviewed-by's is that I don't actually know whether you as reviewer is otherwise happy with the remainder of patch. Normally the ACKed-by or Reviewed-by is a signal that it is > I am assuming it is, but I think it may be worthwhile pointing this out in the document, that unless stated otherwise, the reviewer is happy with the patch I don't think you can always assume this. There are case where I don't give a reviewed-by yet because I want to understand the follow-up patches first. I think what Ian described correspond the best to my view here. And I agree that we probably want to be more explicit in the review to avoid confusion. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [XEN PATCH 1/2] tools: Allow to make *-dir-force-update without ./configure
This also allows to run `make src-tarball` without first having to run `./configure`. Signed-off-by: Anthony PERARD --- tools/Rules.mk | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/Rules.mk b/tools/Rules.mk index cf8935d6a3ea..31cf419ef4f5 100644 --- a/tools/Rules.mk +++ b/tools/Rules.mk @@ -239,7 +239,8 @@ subdir-all-% subdir-clean-% subdir-install-% subdir-uninstall-%: .phony subdir-distclean-%: .phony $(MAKE) -C $* distclean -ifeq (,$(findstring clean,$(MAKECMDGOALS))) +no-configure-targets := clean subtree-force-update-all %-dir-force-update +ifeq (,$(filter $(no-configure-targets),$(MAKECMDGOALS))) $(XEN_ROOT)/config/Tools.mk: $(error You have to run ./configure before building or installing the tools) endif -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [XEN PATCH 2/2] automation: Cache sub-project git tree in build jobs
GitLab have a caching capability, see [1]. Let's use it to avoid using
Internet too often.
The cache is setup so that when xen.git/Config.mk is changed, the
cache will need to be recreated. This has been chosen because that is
where the information about how to clone sub-project trees is encoded
(revisions). That may not work for qemu-xen tree which usually is
`master', but that should be fine for now.
The cache is populated of "git bundle" which will contain a mirror of
the original repo, and can be cloned from. If the bundle exist, the
script have the Xen makefiles clone from it, otherwise it will clone
from the original URL and the bundles will be created just after.
We have more than one runner in GitLab, and no shared cache between
them, so every build jobs will be responsible to create the cache.
[1] https://docs.gitlab.com/ee/ci/yaml/README.html#cache
Signed-off-by: Anthony PERARD
---
automation/gitlab-ci/build.yaml | 8 +
automation/scripts/prepare-cache.sh | 52 +
2 files changed, 60 insertions(+)
create mode 100755 automation/scripts/prepare-cache.sh
diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index 1e61d30c8545..8f9f53a4222f 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -1,6 +1,14 @@
.build-tmpl:
stage: build
image: registry.gitlab.com/xen-project/xen/${CONTAINER}
+ cache:
+key:
+ files:
+- Config.mk
+paths:
+ - ci_cache
+ before_script:
+- ./automation/scripts/prepare-cache.sh
script:
- ./automation/scripts/build 2>&1 | tee build.log
artifacts:
diff --git a/automation/scripts/prepare-cache.sh
b/automation/scripts/prepare-cache.sh
new file mode 100755
index ..017f1b8f0672
--- /dev/null
+++ b/automation/scripts/prepare-cache.sh
@@ -0,0 +1,52 @@
+#!/bin/bash
+
+set -ex
+
+cachedir="${CI_PROJECT_DIR:=`pwd`}/ci_cache"
+mkdir -p "$cachedir"
+
+declare -A r
+r[extras/mini-os]=MINIOS_UPSTREAM_URL
+r[tools/qemu-xen-dir]=QEMU_UPSTREAM_URL
+r[tools/qemu-xen-traditional-dir]=QEMU_TRADITIONAL_URL
+r[tools/firmware/ovmf-dir]=OVMF_UPSTREAM_URL
+r[tools/firmware/seabios-dir]=SEABIOS_UPSTREAM_URL
+
+bundle_loc() {
+echo "$cachedir/${1//\//_}.git.bundle"
+}
+for d in ${!r[@]}; do
+if [ -e $(bundle_loc $d) ]; then
+export ${r[$d]}=$(bundle_loc $d)
+fi
+done
+
+if ! make subtree-force-update-all; then
+# There's maybe an issue with one of the git bundle, just clear the cache
+# and allow it to be rebuilt by a different jobs.
+# Make will reclone missing clones from original URLs instead of from the
+# bundle.
+for d in ${!r[@]}; do
+rm -f "$(bundle_loc $d)"
+done
+exit
+fi
+
+
+tmpdir=$(mktemp -d "$CI_PROJECT_DIR/ci-tmp.XXX")
+for d in ${!r[@]}; do
+bundle=$(bundle_loc $d)
+if [ -e $bundle ]; then
+# We didn't download anything new
+continue
+fi
+# We create a mirror to be able to create a bundle that is a mirror of
+# upstream. Otherwise, the bundle may not have refs that the build system
+# will want, i.e. refs/heads/master would be missing from the bundle.
+url=$(git --git-dir=$d/.git config remote.origin.url)
+repo_mirrored="$tmpdir/${d//\//_}"
+git clone --bare --mirror --reference "$d" "$url" "$repo_mirrored"
+git --git-dir="$repo_mirrored" bundle create $bundle --all
+rm -rf "$repo_mirrored"
+done
+rmdir "$tmpdir"
--
Anthony PERARD
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [XEN PATCH 0/2] Start using GitLab caching capability
Patch series available in this git branch: https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git br.ci-caching-v1 This is only a small improvement to the GitLab CI. It only caches git clone that Xen's makefile creates and not submodules needed by those clones. It doesn't cache the different tarballs that the makefiles downloads, that could be another improvement. Cheers, Anthony PERARD (2): tools: Allow to make *-dir-force-update without ./configure automation: Cache sub-project git tree in build jobs automation/gitlab-ci/build.yaml | 8 + automation/scripts/prepare-cache.sh | 52 + tools/Rules.mk | 3 +- 3 files changed, 62 insertions(+), 1 deletion(-) create mode 100755 automation/scripts/prepare-cache.sh -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [xen-unstable test] 144959: regressions - FAIL
flight 144959 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/144959/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-xtf-amd64-amd64-5 72 xtf/test-hvm64-xsa-308 fail REGR. vs. 144936 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10 fail blocked in 144936 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 144936 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 144936 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 144936 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 144936 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 144936 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail like 144936 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 144936 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 144936 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 144936 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 144936 test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass version targeted for testing: xen 5c13ed79f3cba200f21e7dfd6ed7f3aa08e4dada baseline version: xen 0e7c69bd3c0b35a677d73843b39522787ccf5a3f Last test of basis 144936 2019-12-18 16:07:31 Z0 days Testing same since 144959 2019-12-19 04:58:26 Z0 days1 attempts People who touched
[Xen-devel] [PATCH] libxc/restore: Fix data auditing in handle_x86_pv_info()
handle_x86_pv_info() has a subtle bug. It uses an 'else if' chain with a
clause in the middle which doesn't exit unconditionally. In practice, this
means that when restoring a 32bit PV guest, later sanity checks are skipped.
Rework the logic a little to be simpler. There are exactly two valid
combinations of fields in X86_PV_INFO, so factor this out and check them all
in one go, before making adjustments to the current domain.
Once adjustments have been completed successfully, sanity check the result
against the X86_PV_INFO settings in one go, rather than piecewise.
Signed-off-by: Andrew Cooper
---
CC: Ian Jackson
CC: Wei Liu
---
tools/libxc/xc_sr_restore_x86_pv.c | 69 ++
1 file changed, 41 insertions(+), 28 deletions(-)
diff --git a/tools/libxc/xc_sr_restore_x86_pv.c
b/tools/libxc/xc_sr_restore_x86_pv.c
index a2dbf85157..9e9ff32d47 100644
--- a/tools/libxc/xc_sr_restore_x86_pv.c
+++ b/tools/libxc/xc_sr_restore_x86_pv.c
@@ -582,6 +582,21 @@ static int update_guest_p2m(struct xc_sr_context *ctx)
}
/*
+ * The valid width/pt_levels values in X86_PV_INFO are inextricably linked.
+ * Cross-check the legitimate combinations.
+ */
+static bool valid_x86_pv_info_combination(
+const struct xc_sr_rec_x86_pv_info *info)
+{
+switch ( info->guest_width )
+{
+case 4: return info->pt_levels == 3;
+case 8: return info->pt_levels == 4;
+default: return false;
+}
+}
+
+/*
* Process an X86_PV_INFO record.
*/
static int handle_x86_pv_info(struct xc_sr_context *ctx,
@@ -602,29 +617,31 @@ static int handle_x86_pv_info(struct xc_sr_context *ctx,
rec->length, sizeof(*info));
return -1;
}
-else if ( info->guest_width != 4 &&
- info->guest_width != 8 )
+
+if ( !valid_x86_pv_info_combination(info) )
{
-ERROR("Unexpected guest width %u, Expected 4 or 8",
- info->guest_width);
+ERROR("Invalid X86_PV_INFO combination: width %u, pt_levels %u",
+ info->guest_width, info->pt_levels);
return -1;
}
-else if ( info->guest_width != ctx->x86_pv.width )
+
+/*
+ * PV domains default to native width. For an incomming compat domain, we
+ * will typically be the first entity to inform Xen.
+ */
+if ( info->guest_width != ctx->x86_pv.width )
{
-int rc;
-struct xen_domctl domctl;
-
-/* Try to set address size, domain is always created 64 bit. */
-memset(, 0, sizeof(domctl));
-domctl.domain = ctx->domid;
-domctl.cmd= XEN_DOMCTL_set_address_size;
-domctl.u.address_size.size = info->guest_width * 8;
-rc = do_domctl(xch, );
+struct xen_domctl domctl = {
+.domain = ctx->domid,
+.cmd= XEN_DOMCTL_set_address_size,
+.u.address_size.size = info->guest_width * 8,
+};
+int rc = do_domctl(xch, );
+
if ( rc != 0 )
{
-ERROR("Width of guest in stream (%u"
- " bits) differs with existing domain (%u bits)",
- info->guest_width * 8, ctx->x86_pv.width * 8);
+ERROR("Failed to update d%d address size to %u",
+ ctx->domid, info->guest_width * 8);
return -1;
}
@@ -636,18 +653,14 @@ static int handle_x86_pv_info(struct xc_sr_context *ctx,
return -1;
}
}
-else if ( info->pt_levels != 3 &&
- info->pt_levels != 4 )
-{
-ERROR("Unexpected guest levels %u, Expected 3 or 4",
- info->pt_levels);
-return -1;
-}
-else if ( info->pt_levels != ctx->x86_pv.levels )
+
+/* Sanity check (possibly new) domain settings. */
+if ( (info->guest_width != ctx->x86_pv.width) ||
+ (info->pt_levels != ctx->x86_pv.levels) )
{
-ERROR("Levels of guest in stream (%u"
- ") differs with existing domain (%u)",
- info->pt_levels, ctx->x86_pv.levels);
+ERROR("X86_PV_INFO width/pt_levels settings %u/%u mismatch with d%d
%u/%u",
+ info->guest_width, info->pt_levels, ctx->domid,
+ ctx->x86_pv.width, ctx->x86_pv.levels);
return -1;
}
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/save: reserve HVM save record numbers that have been consumed...
> -Original Message-
> From: Jan Beulich
> Sent: 19 December 2019 13:25
> To: Durrant, Paul
> Cc: Andrew Cooper ; xen-
> de...@lists.xenproject.org; Wei Liu ; Roger Pau Monné
>
> Subject: Re: [PATCH v2] x86/save: reserve HVM save record numbers that
> have been consumed...
>
> On 19.12.2019 14:15, Durrant, Paul wrote:
> >> From: Andrew Cooper
> >> Sent: 19 December 2019 13:08
> >>
> >> On 19/12/2019 12:37, Durrant, Paul wrote:
> From: Andrew Cooper
> Sent: 19 December 2019 12:16
>
> On 19/12/2019 12:04, Paul Durrant wrote:
> > --- a/xen/include/public/arch-x86/hvm/save.h
> > +++ b/xen/include/public/arch-x86/hvm/save.h
> > @@ -639,6 +639,8 @@ struct hvm_msr {
> >
> > #define CPU_MSR_CODE 20
> >
> > +/* Range 22 - 40 reserved for Amazon */
> What about 21 and 22? And where does the actual number stop, because
> based on v1, its not 40.
>
> >>> The range is inclusive (maybe that's not obvious?). For some reason 21
> >> was skipped but why do you say the top is not 40? That was what I set
> >> HVM_SAVE_CODE_MAX to in v1.
> >>
> >> You also said that included prospective headroom, which definitely
> isn't
> >> fair to reserve for ABI breakage reasons.
> >>
> >> Which numbers have actually been allocated?
> >>
> >
> > The problem is that I don't yet know for sure. I have definitely seen
> > patches using 22 thru 34. It is *probably* safe to restrict to that but
> > does it really cost that much more to reserve some extra space just in
> > case? I.e. if someone else adds 41 vs. 35 is it going to make much of a
> > difference?
>
> Not _that much_, but still - it's a bodge, so let's try to limit it as
> much as we can.
>
OK, I'll send a v3 using 34 as the limit.
Paul
> Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/save: reserve HVM save record numbers that have been consumed...
On 19.12.2019 14:15, Durrant, Paul wrote:
>> From: Andrew Cooper
>> Sent: 19 December 2019 13:08
>>
>> On 19/12/2019 12:37, Durrant, Paul wrote:
From: Andrew Cooper
Sent: 19 December 2019 12:16
On 19/12/2019 12:04, Paul Durrant wrote:
> --- a/xen/include/public/arch-x86/hvm/save.h
> +++ b/xen/include/public/arch-x86/hvm/save.h
> @@ -639,6 +639,8 @@ struct hvm_msr {
>
> #define CPU_MSR_CODE 20
>
> +/* Range 22 - 40 reserved for Amazon */
What about 21 and 22? And where does the actual number stop, because
based on v1, its not 40.
>>> The range is inclusive (maybe that's not obvious?). For some reason 21
>> was skipped but why do you say the top is not 40? That was what I set
>> HVM_SAVE_CODE_MAX to in v1.
>>
>> You also said that included prospective headroom, which definitely isn't
>> fair to reserve for ABI breakage reasons.
>>
>> Which numbers have actually been allocated?
>>
>
> The problem is that I don't yet know for sure. I have definitely seen
> patches using 22 thru 34. It is *probably* safe to restrict to that but
> does it really cost that much more to reserve some extra space just in
> case? I.e. if someone else adds 41 vs. 35 is it going to make much of a
> difference?
Not _that much_, but still - it's a bodge, so let's try to limit it as
much as we can.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/save: reserve HVM save record numbers that have been consumed...
> -Original Message-
> From: Andrew Cooper
> Sent: 19 December 2019 13:08
> To: Durrant, Paul ; xen-devel@lists.xenproject.org
> Cc: Wei Liu ; Jan Beulich ; Roger Pau Monné
>
> Subject: Re: [PATCH v2] x86/save: reserve HVM save record numbers that
> have been consumed...
>
> On 19/12/2019 12:37, Durrant, Paul wrote:
> >> -Original Message-
> >> From: Andrew Cooper
> >> Sent: 19 December 2019 12:16
> >> To: Durrant, Paul ; xen-devel@lists.xenproject.org
> >> Cc: Wei Liu ; Jan Beulich ; Roger Pau
> Monné
> >>
> >> Subject: Re: [PATCH v2] x86/save: reserve HVM save record numbers that
> >> have been consumed...
> >>
> >> On 19/12/2019 12:04, Paul Durrant wrote:
> >>> ...for patches not (yet) upstream.
> >>>
> >>> This patch is simply adding a comment to reserve save record number
> >> space
> >>> to avoid the risk of clashes between existent downstream changes made
> by
> >>> Amazon and future upstream changes which may be incompatible.
> >>>
> >>> Signed-off-by: Paul Durrant
> >>> Reviewed-by: Wei Liu
> >>> ---
> >>> Cc: Jan Beulich
> >>> Cc: Andrew Cooper
> >>> Cc: "Roger Pau Monné"
> >>>
> >>> v2:
> >>> - Reduce patch to just a comment
> >>> ---
> >>> xen/include/public/arch-x86/hvm/save.h | 2 ++
> >>> 1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/xen/include/public/arch-x86/hvm/save.h
> >> b/xen/include/public/arch-x86/hvm/save.h
> >>> index b2ad3fcd74..b3d445acf7 100644
> >>> --- a/xen/include/public/arch-x86/hvm/save.h
> >>> +++ b/xen/include/public/arch-x86/hvm/save.h
> >>> @@ -639,6 +639,8 @@ struct hvm_msr {
> >>>
> >>> #define CPU_MSR_CODE 20
> >>>
> >>> +/* Range 22 - 40 reserved for Amazon */
> >> What about 21 and 22? And where does the actual number stop, because
> >> based on v1, its not 40.
> >>
> > The range is inclusive (maybe that's not obvious?). For some reason 21
> was skipped but why do you say the top is not 40? That was what I set
> HVM_SAVE_CODE_MAX to in v1.
>
> You also said that included prospective headroom, which definitely isn't
> fair to reserve for ABI breakage reasons.
>
> Which numbers have actually been allocated?
>
The problem is that I don't yet know for sure. I have definitely seen patches
using 22 thru 34. It is *probably* safe to restrict to that but does it really
cost that much more to reserve some extra space just in case? I.e. if someone
else adds 41 vs. 35 is it going to make much of a difference?
Paul
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Recent cores-scheduling failures
On 19/12/2019 12:45, Sergey Dyasli wrote: > 2. ACPI S5 crash: > > https://paste.debian.net/1121748/ cmpw $0x7fff,(%rax) with %rax as 0xc2c2c2c2c2c2c2c2 Looks like a use-after-free checking for the idle domain. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/save: reserve HVM save record numbers that have been consumed...
On 19/12/2019 12:37, Durrant, Paul wrote:
>> -Original Message-
>> From: Andrew Cooper
>> Sent: 19 December 2019 12:16
>> To: Durrant, Paul ; xen-devel@lists.xenproject.org
>> Cc: Wei Liu ; Jan Beulich ; Roger Pau Monné
>>
>> Subject: Re: [PATCH v2] x86/save: reserve HVM save record numbers that
>> have been consumed...
>>
>> On 19/12/2019 12:04, Paul Durrant wrote:
>>> ...for patches not (yet) upstream.
>>>
>>> This patch is simply adding a comment to reserve save record number
>> space
>>> to avoid the risk of clashes between existent downstream changes made by
>>> Amazon and future upstream changes which may be incompatible.
>>>
>>> Signed-off-by: Paul Durrant
>>> Reviewed-by: Wei Liu
>>> ---
>>> Cc: Jan Beulich
>>> Cc: Andrew Cooper
>>> Cc: "Roger Pau Monné"
>>>
>>> v2:
>>> - Reduce patch to just a comment
>>> ---
>>> xen/include/public/arch-x86/hvm/save.h | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/xen/include/public/arch-x86/hvm/save.h
>> b/xen/include/public/arch-x86/hvm/save.h
>>> index b2ad3fcd74..b3d445acf7 100644
>>> --- a/xen/include/public/arch-x86/hvm/save.h
>>> +++ b/xen/include/public/arch-x86/hvm/save.h
>>> @@ -639,6 +639,8 @@ struct hvm_msr {
>>>
>>> #define CPU_MSR_CODE 20
>>>
>>> +/* Range 22 - 40 reserved for Amazon */
>> What about 21 and 22? And where does the actual number stop, because
>> based on v1, its not 40.
>>
> The range is inclusive (maybe that's not obvious?). For some reason 21 was
> skipped but why do you say the top is not 40? That was what I set
> HVM_SAVE_CODE_MAX to in v1.
You also said that included prospective headroom, which definitely isn't
fair to reserve for ABI breakage reasons.
Which numbers have actually been allocated?
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] tools/python: Python 3 compatibility
convert-legacy-stream is only used for incomming migration from pre Xen 4.7,
and verify-stream-v2 appears to only be used by me during migration
development - it is little surprise that they missed the main converstion
effort in Xen 4.13.
Fix it all up.
Move open_file_or_fd() into a new util.py to avoid duplication, making it a
more generic wrapper around open() or fdopen().
In libxc.py, drop all long() conversion. Python 2 will DTRT with int => long
promotion, even on 32bit builds.
In convert-legacy-stream, don't pass empty strings to write_record(). Join on
the empty argl will do the right thing.
Signed-off-by: Andrew Cooper
---
CC: Ian Jackson
CC: Wei Liu
CC: Marek Marczykowski-Górecki
v2:
* Drop int/long in libxc.py. Python 2 will DTRT with int turning into long.
* More b prefixes in convert-legacy-stream. Drop empty string passing
This needs backporting to 4.13 ASAP
---
tools/python/scripts/convert-legacy-stream | 67 +-
tools/python/scripts/verify-stream-v2 | 42 ---
tools/python/xen/__init__.py | 1 -
tools/python/xen/lowlevel/__init__.py | 1 -
tools/python/xen/migration/libxc.py| 34 +++
tools/python/xen/migration/libxl.py| 2 +-
tools/python/xen/migration/verify.py | 4 +-
tools/python/xen/util.py | 23 ++
8 files changed, 69 insertions(+), 105 deletions(-)
create mode 100644 tools/python/xen/util.py
diff --git a/tools/python/scripts/convert-legacy-stream
b/tools/python/scripts/convert-legacy-stream
index 5f80f13654..d316ae16f0 100755
--- a/tools/python/scripts/convert-legacy-stream
+++ b/tools/python/scripts/convert-legacy-stream
@@ -5,6 +5,8 @@
Convert a legacy migration stream to a v2 stream.
"""
+from __future__ import print_function
+
import sys
import os, os.path
import syslog
@@ -12,6 +14,7 @@ import traceback
from struct import calcsize, unpack, pack
+from xen.util import open_file_or_fd as open_file_or_fd
from xen.migration import legacy, public, libxc, libxl, xl
__version__ = 1
@@ -39,16 +42,16 @@ def info(msg):
for line in msg.split("\n"):
syslog.syslog(syslog.LOG_INFO, line)
else:
-print msg
+print(msg)
def err(msg):
"""Error message, routed to appropriate destination"""
if log_to_syslog:
for line in msg.split("\n"):
syslog.syslog(syslog.LOG_ERR, line)
-print >> sys.stderr, msg
+print(msg, file = sys.stderr)
-class StreamError(StandardError):
+class StreamError(Exception):
"""Error with the incoming migration stream"""
pass
@@ -70,7 +73,7 @@ class VM(object):
# libxl
self.libxl = fmt == "libxl"
-self.emu_xenstore = "" # NUL terminated key pairs from "toolstack"
records
+self.emu_xenstore = b"" # NUL terminated key pairs from
"toolstack" records
def write_libxc_ihdr():
stream_write(pack(libxc.IHDR_FORMAT,
@@ -102,12 +105,12 @@ def write_libxl_hdr():
))
def write_record(rt, *argl):
-alldata = ''.join(argl)
+alldata = b''.join(argl)
length = len(alldata)
record = pack(libxc.RH_FORMAT, rt, length) + alldata
plen = (8 - (length & 7)) & 7
-record += '\x00' * plen
+record += b'\x00' * plen
stream_write(record)
@@ -164,10 +167,10 @@ def write_libxc_hvm_params(params):
pack("Q" * len(params), *params))
def write_libxl_end():
-write_record(libxl.REC_TYPE_end, "")
+write_record(libxl.REC_TYPE_end)
def write_libxl_libxc_context():
-write_record(libxl.REC_TYPE_libxc_context, "")
+write_record(libxl.REC_TYPE_libxc_context)
def write_libxl_emulator_xenstore_data(data):
write_record(libxl.REC_TYPE_emulator_xenstore_data,
@@ -225,7 +228,7 @@ def read_pv_extended_info(vm):
so_far += datasz
# Eww, but this is how it is done :(
-if blkid == "vcpu":
+if blkid == b"vcpu":
vm.basic_len = datasz
@@ -242,10 +245,10 @@ def read_pv_extended_info(vm):
write_libxc_pv_info(vm)
-elif blkid == "extv":
+elif blkid == b"extv":
vm.extd = True
-elif blkid == "xcnt":
+elif blkid == b"xcnt":
vm.xsave_len, = unpack("I", data[:4])
info("xcnt sz 0x%x" % (vm.xsave_len, ))
@@ -296,7 +299,7 @@ def read_pv_tail(vm):
info("Got shinfo")
write_record(libxc.REC_TYPE_shared_info, shinfo)
-write_record(libxc.REC_TYPE_end, "")
+write_record(libxc.REC_TYPE_end)
def read_libxl_toolstack(vm, data):
@@ -336,7 +339,7 @@ def read_libxl_toolstack(vm, data):
if twidth == 64:
name = name[:-4]
-if name[-1] != '\x00':
+if name[-1] != b'\x00':
raise StreamError("physmap name not NUL terminated")
root = "physmap/%x" % (phys,)
@@ -347,7 +350,7 @@ def read_libxl_toolstack(vm,
Re: [Xen-devel] [PATCH v2] xen-pciback: optionally allow interrupt enable flag writes
On Thu, Dec 19, 2019 at 12:20:24PM +0100, Jan Beulich wrote:
> On 19.12.2019 04:49, Marek Marczykowski-Górecki wrote:
> > +enum interrupt_type xen_pcibk_get_interrupt_type(struct pci_dev *dev)
> > +{
> > + int err;
> > + u16 val;
> > +
> > + err = pci_read_config_word(dev, PCI_COMMAND, );
> > + if (err)
> > + return INTERRUPT_TYPE_ERR;
> > + if (!(val & PCI_COMMAND_INTX_DISABLE))
> > + return INTERRUPT_TYPE_INTX;
> > +
> > + /* Do not trust dev->msi(x)_enabled here, as enabling could be done
> > +* bypassing the pci_*msi* functions, by the qemu.
> > +*/
>
> Judging from this comment, how can you assume only one of the
> three variants is actually enabled? It's against the spec, yes,
> but it's not at all impossible afaict. I think you want the
> return value here to be
> - negative errno values (no need to discard the actual error
> codes) or
> - a non-negative bitmap indicating which of the interrupt types
> is/are currently enabled.
Good idea, I'll change that.
> That way ...
>
> > +static int msi_msix_flags_write(struct pci_dev *dev, int offset, u16
> > new_value,
> > + void *data)
> > +{
> > + int err;
> > + u16 old_value;
> > + const struct msi_msix_field_config *field_config = data;
> > + const struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(dev);
> > +
> > + if (xen_pcibk_permissive || dev_data->permissive)
> > + goto write;
> > +
> > + err = pci_read_config_word(dev, offset, _value);
> > + if (err)
> > + return err;
> > +
> > + if (new_value == old_value)
> > + return 0;
> > +
> > + if (!dev_data->allow_interrupt_control ||
> > + (new_value ^ old_value) & ~field_config->enable_bit)
> > + return PCIBIOS_SET_FAILED;
> > +
> > + if (new_value & field_config->enable_bit) {
> > + /* don't allow enabling together with other interrupt types */
> > + const enum interrupt_type int_type =
> > xen_pcibk_get_interrupt_type(dev);
> > + if (int_type == INTERRUPT_TYPE_NONE ||
> > + int_type == field_config->int_type)
>
> ... equality comparisons like this one will actually become safe.
>
> Jan
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
signature.asc
Description: PGP signature
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] Recent cores-scheduling failures
Hi Juergen, We recently did another quick test of core scheduling mode, and the following failures were found: 1. live-patch apply failures: (XEN) [ 1058.751974] livepatch: lp_1_1: Timed out on semaphore in CPU quiesce phase 30/31 (XEN) [ 1058.751982] livepatch: lp_1_1 finished REPLACE with rc=-16 2. ACPI S5 crash: https://paste.debian.net/1121748/ -- Thanks, Sergey ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/save: reserve HVM save record numbers that have been consumed...
> -Original Message-
> From: Andrew Cooper
> Sent: 19 December 2019 12:16
> To: Durrant, Paul ; xen-devel@lists.xenproject.org
> Cc: Wei Liu ; Jan Beulich ; Roger Pau Monné
>
> Subject: Re: [PATCH v2] x86/save: reserve HVM save record numbers that
> have been consumed...
>
> On 19/12/2019 12:04, Paul Durrant wrote:
> > ...for patches not (yet) upstream.
> >
> > This patch is simply adding a comment to reserve save record number
> space
> > to avoid the risk of clashes between existent downstream changes made by
> > Amazon and future upstream changes which may be incompatible.
> >
> > Signed-off-by: Paul Durrant
> > Reviewed-by: Wei Liu
> > ---
> > Cc: Jan Beulich
> > Cc: Andrew Cooper
> > Cc: "Roger Pau Monné"
> >
> > v2:
> > - Reduce patch to just a comment
> > ---
> > xen/include/public/arch-x86/hvm/save.h | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/xen/include/public/arch-x86/hvm/save.h
> b/xen/include/public/arch-x86/hvm/save.h
> > index b2ad3fcd74..b3d445acf7 100644
> > --- a/xen/include/public/arch-x86/hvm/save.h
> > +++ b/xen/include/public/arch-x86/hvm/save.h
> > @@ -639,6 +639,8 @@ struct hvm_msr {
> >
> > #define CPU_MSR_CODE 20
> >
> > +/* Range 22 - 40 reserved for Amazon */
>
> What about 21 and 22? And where does the actual number stop, because
> based on v1, its not 40.
>
The range is inclusive (maybe that's not obvious?). For some reason 21 was
skipped but why do you say the top is not 40? That was what I set
HVM_SAVE_CODE_MAX to in v1.
Paul
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] arm64: xen: Use modern annotations for assembly functions
In an effort to clarify and simplify the annotation of assembly functions in the kernel new macros have been introduced. These replace ENTRY and ENDPROC. Update the annotations in the xen code to the new macros. Signed-off-by: Mark Brown Reviewed-by: Julien Grall Reviewed-by: Stefano Stabellini --- arch/arm64/xen/hypercall.S | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/arm64/xen/hypercall.S b/arch/arm64/xen/hypercall.S index c5f05c4a4d00..5b09aca55108 100644 --- a/arch/arm64/xen/hypercall.S +++ b/arch/arm64/xen/hypercall.S @@ -56,11 +56,11 @@ #define XEN_IMM 0xEA1 #define HYPERCALL_SIMPLE(hypercall)\ -ENTRY(HYPERVISOR_##hypercall) \ +SYM_FUNC_START(HYPERVISOR_##hypercall) \ mov x16, #__HYPERVISOR_##hypercall; \ hvc XEN_IMM;\ ret;\ -ENDPROC(HYPERVISOR_##hypercall) +SYM_FUNC_END(HYPERVISOR_##hypercall) #define HYPERCALL0 HYPERCALL_SIMPLE #define HYPERCALL1 HYPERCALL_SIMPLE @@ -86,7 +86,7 @@ HYPERCALL2(multicall); HYPERCALL2(vm_assist); HYPERCALL3(dm_op); -ENTRY(privcmd_call) +SYM_FUNC_START(privcmd_call) mov x16, x0 mov x0, x1 mov x1, x2 @@ -109,4 +109,4 @@ ENTRY(privcmd_call) */ uaccess_ttbr0_disable x6, x7 ret -ENDPROC(privcmd_call); +SYM_FUNC_END(privcmd_call); -- 2.20.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/save: reserve HVM save record numbers that have been consumed...
On 19/12/2019 12:04, Paul Durrant wrote:
> ...for patches not (yet) upstream.
>
> This patch is simply adding a comment to reserve save record number space
> to avoid the risk of clashes between existent downstream changes made by
> Amazon and future upstream changes which may be incompatible.
>
> Signed-off-by: Paul Durrant
> Reviewed-by: Wei Liu
> ---
> Cc: Jan Beulich
> Cc: Andrew Cooper
> Cc: "Roger Pau Monné"
>
> v2:
> - Reduce patch to just a comment
> ---
> xen/include/public/arch-x86/hvm/save.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/xen/include/public/arch-x86/hvm/save.h
> b/xen/include/public/arch-x86/hvm/save.h
> index b2ad3fcd74..b3d445acf7 100644
> --- a/xen/include/public/arch-x86/hvm/save.h
> +++ b/xen/include/public/arch-x86/hvm/save.h
> @@ -639,6 +639,8 @@ struct hvm_msr {
>
> #define CPU_MSR_CODE 20
>
> +/* Range 22 - 40 reserved for Amazon */
What about 21 and 22? And where does the actual number stop, because
based on v1, its not 40.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/save: reserve HVM save record numbers that have been consumed...
On 19/12/2019 11:45, Durrant, Paul wrote: >> -Original Message- >> From: Andrew Cooper >> Sent: 19 December 2019 11:30 >> To: Durrant, Paul ; xen-devel@lists.xenproject.org >> Cc: Wei Liu ; Jan Beulich ; Roger Pau Monné >> >> Subject: Re: [Xen-devel] [PATCH] x86/save: reserve HVM save record numbers >> that have been consumed... >> >> On 19/12/2019 11:06, Durrant, Paul wrote: It is not fair or reasonable to include extra headroom in a "oh dear we screwed up - will the community be kind enough to help us work around our ABI problems" scenario. >>> I would have thought all the pain you went through to keep XenServer >> going with its non-upstreamed hypercall numbers would have made you a >> little more sympathetic to dealing with past mistakes. >> >> I could object to the principle of the patch, if you'd prefer :) >> >> If you recall for the legacy window PV driver ABI breakages, I didn't >> actually reserve any numbers upstream in the end. All compatibility was >> handled locally. > And I remember how nasty the hacks were ;-) Like I'm ever going to forget those... For anyone else reading this thread and is a little confused, https://github.com/xenserver/xen.pg/blob/XS-7.1.x/master/xen-legacy-win-xenmapspace-quirks.patch ought to clear some things up. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] x86/save: reserve HVM save record numbers that have been consumed...
...for patches not (yet) upstream.
This patch is simply adding a comment to reserve save record number space
to avoid the risk of clashes between existent downstream changes made by
Amazon and future upstream changes which may be incompatible.
Signed-off-by: Paul Durrant
Reviewed-by: Wei Liu
---
Cc: Jan Beulich
Cc: Andrew Cooper
Cc: "Roger Pau Monné"
v2:
- Reduce patch to just a comment
---
xen/include/public/arch-x86/hvm/save.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/xen/include/public/arch-x86/hvm/save.h
b/xen/include/public/arch-x86/hvm/save.h
index b2ad3fcd74..b3d445acf7 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -639,6 +639,8 @@ struct hvm_msr {
#define CPU_MSR_CODE 20
+/* Range 22 - 40 reserved for Amazon */
+
/*
* Largest type-code in use
*/
--
2.20.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [ovmf test] 144962: all pass - PUSHED
flight 144962 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/144962/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 95bb203861c5e19b7b7d5d9318e16d82108f2134 baseline version: ovmf c7a0aca0ed0e9b51efe0c437ff77b30cf1457f8a Last test of basis 144957 2019-12-19 04:17:39 Z0 days Testing same since 144962 2019-12-19 06:46:07 Z0 days1 attempts People who touched revisions under test: Zhichao Gao 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 c7a0aca0ed..95bb203861 95bb203861c5e19b7b7d5d9318e16d82108f2134 -> xen-tested-master ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/save: reserve HVM save record numbers that have been consumed...
> -Original Message- > From: Andrew Cooper > Sent: 19 December 2019 11:30 > To: Durrant, Paul ; xen-devel@lists.xenproject.org > Cc: Wei Liu ; Jan Beulich ; Roger Pau Monné > > Subject: Re: [Xen-devel] [PATCH] x86/save: reserve HVM save record numbers > that have been consumed... > > On 19/12/2019 11:06, Durrant, Paul wrote: > >> It is not fair or reasonable to include extra headroom in a "oh dear we > >> screwed up - will the community be kind enough to help us work around > >> our ABI problems" scenario. > >> > > I would have thought all the pain you went through to keep XenServer > going with its non-upstreamed hypercall numbers would have made you a > little more sympathetic to dealing with past mistakes. > > I could object to the principle of the patch, if you'd prefer :) > > If you recall for the legacy window PV driver ABI breakages, I didn't > actually reserve any numbers upstream in the end. All compatibility was > handled locally. And I remember how nasty the hacks were ;-) Given we don't yet have a clash that requires such nastiness, I just want to avoid it happening before we manage to dispense with the downstream-only legacy code. > > >> For now, I'd just leave it as a comment, and strictly only covering the > >> ones you have used. There is no need to actually bump the structure > >> sizes in xen for now - simply that the ones you have currently used > >> don't get allocated for something different in the future. > >> > > Ok, we can defer actually bumping HVM_SAVE_CODE_MAX, but it's almost > certain to happen eventually. > > That's fine. > Ok. I'll send a v2 with just the comment (and assume Wei's R-b still stands). Paul ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
Ian Jackson writes ("Re: [PATCH v3 5/7] Add Code Review Guide"):
> Lars Kurth writes ("Re: [PATCH v3 5/7] Add Code Review Guide"):
> > In a nutshell, in such a review the possible interpretations are
> > * I reviewed, but didn't feel qualified to do the rest
> > * I reviewed, but did not get round to give it full attention
> > * I reviewed, but before I make a final decision want to look at the next
> > version
> > ...
> > * I reviewed and don't object the rest
> > * I reviewed and agreed with the rest
> > The latter two are equivalent to Ack/R-b
> >
> > That is quite a large range of possibilities and kind of leaves the author
> > guessing what state the review is in
...
> IOW: if you do not get an A-b or R-b, then the person writing is not
> necessarily making any of the statements you posit which start "I
> reviewed".
The other point it occurs to me to make here is that whether someone
reviewed something is not of formal importance to the process,
although it is of course very relevant information.
With my maintainer hat on I frequently approve patches which I have
not reviewed.
Likewise I sometimes read and even review in detail patches where I
have no formal authority. My comments are then intended (in a formal
sense) as input to the maintainers' decisionmaking.
(In practice, since we are all working together to make the best
software, maintainers generally expect a submitter to address issues
raised by a review from a non-maintainer, and strong objections from
non-maintainer stakeholders usually lead to a discussion about how to
resolve matters. This is all cooperation and common courtesy I
think.)
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/hvm/rtc: preserved guest RTC offset during suspend/resume/migrate
> -Original Message-
> From: Jan Beulich
> Sent: 19 December 2019 11:30
> To: Durrant, Paul
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini
> ; Julien Grall ; Volodymyr Babchuk
> ; Andrew Cooper ;
> George Dunlap ; Ian Jackson
> ; Konrad Rzeszutek Wilk
> ; Wei Liu ; Roger Pau Monné
>
> Subject: Re: [PATCH] x86/hvm/rtc: preserved guest RTC offset during
> suspend/resume/migrate
>
> On 18.12.2019 15:41, Paul Durrant wrote:
> > The emulated RTC is synchronized with the PV wallclock; any write to the
> > RTC will update struct domain's 'time_offset_seconds' field and call
> > update_domain_wallclock().
> >
> > However, the value of 'time_offset_seconds' is not preserved in any save
> > record and indeed, when the RTC save record is loaded, the CMOS values
> > will be updated based on an offset value which may or may not have been
> > set by the toolstack [1]. This may result in making bogus values
> available
> > to the guest and messing up any calculations done in the call to
> > alarm_timer_update() at the end of rtc_load().
> >
> > This patch extends the RTC save record to contain an offset value, which
> > will be zero filled on load of an older record. The
> 'time_offset_secoonds'
> > field in struct domain is also modified into a 'time_offset' struct,
> > containing a 'seconds' field and a boolean 'set' field.
> >
> > The code in rtc_load() then uses the new value in the save record to
> > update the value of struct domain's 'time_offset.seconds' unless
> > 'time_offset.set' is true, which will only be the case if the toolstack
> has
> > already performed a XEN_DOMCTL_settimeoffset.
> >
> > [1] There is currently no way for a toolstack to read the value of
> > 'time_offset_seconds' from struct domain. In the past, any hope of
> > preservation of the value across a guest life-cycle operation was
> based
> > on relying on qemu-dm to write a value into xenstore whenever the
> RTC
> > was updated, in response to an IOREQ with type IOREQ_TYPE_TIMEOFFSET
> > being sent by Xen; see:
> >
> > https://xenbits.xen.org/gitweb/?p=qemu-xen-
> traditional.git;a=blob;f=i386-dm/helper2.c#l457
> >
> > but this behaviour was never forward-ported into upstream QEMU,
> which
> > completely ignores that IOREQ type.
> > In either case, nothing in xl or libxl ever samples the value of
> > RTC offset from xenstore so any offset adjustment to a non-zero
> value
> > performed by the guest (which in the case of Windows is highly
> likely
> > as it normally writes RTC in local time, whereas Xen maintains time
> in
> > UTC) is completely lost with the de-facto toolstack, and always has
> > been. Instead, PV drivers are relied upon to paper over this gaping
> > hole.
> >
> > Signed-off-by: Paul Durrant
>
> Reviewed-by: Jan Beulich
Thanks.
> with one remark:
>
> > @@ -771,6 +773,12 @@ static int rtc_load(struct domain *d,
> hvm_domain_context_t *h)
> >
> > /* Reset the wall-clock time. In normal running, this runs with
> host
> > * time, so let's keep doing that. */
> > +if ( !d->time_offset.set )
> > +{
> > +d->time_offset.seconds = s->hw.rtc_offset;
> > +update_domain_wallclock_time(d);
> > +}
>
> It's not really clear to me which of the possible behaviors is the
> better one - overriding a tool stack set value with what the save
> record says, or (like you do) the other way around.
It's not clear to me either, which is why I erred on the side of caution. I
didn't want to break any out-of-tree toolstacks that might be overriding the
offset early in the restore phase from a value acquired via QEMU hackery. I
guess the boolean could be dispensed with if we retire the IOREQ and silently
ignore the DOMCTL for HVM guests (so overrides from the aforementioned
toolstacks simply have no effect).
Paul
>
> Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
Lars Kurth writes ("Re: [PATCH v3 5/7] Add Code Review Guide"):
> In a nutshell, in such a review the possible interpretations are
> * I reviewed, but didn't feel qualified to do the rest
> * I reviewed, but did not get round to give it full attention
> * I reviewed, but before I make a final decision want to look at the next
> version
> ...
> * I reviewed and don't object the rest
> * I reviewed and agreed with the rest
> The latter two are equivalent to Ack/R-b
>
> That is quite a large range of possibilities and kind of leaves the author
> guessing what state the review is in
There are only three possibilities:
Acked-by:
I hereby bless this with my maintainer powers.
I may or may not have reviewed it. The body text may contain
more information about that.
Reviewed-by:
I have reviewed this to my own satisfaction and this mail contains
all the comments I have on it. If I am a maintainer, I hereby
bless it with my maintainer powers. (This is a weird use of the
word "Reviewed" since in usual usage one can review something and
end up disapproving of it; nevertheless this is the convention.)
In both of the above cases, additionally
If my approval is conditional (eg on changes) this will be
stated explicitly in the body text of my message.
Neither of the above:
I have read at least some of this and felt motivated to make some
observations. If I have reviewed it properly this would usually be
stated in the body text. If I am taking enough of an interest in
this patch, the body text may also say what I think the current
state and/or next steps should be. In any case, I do *not* bless
this patch (in its current form) with any maintainer powers I may
have.
IOW: if you do not get an A-b or R-b, then the person writing is not
necessarily making any of the statements you posit which start "I
reviewed".
Having said that it is a good idea for people commenting on patches to
make clear what they have and haven't done. I often start a message
with "Thanks for this patch which I have reviewed. I have some
comments" or words to that effect.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests
On 19.12.2019 12:23, Sergey Dyasli wrote: > On 18/12/2019 11:06, Jan Beulich wrote: >> On 17.12.2019 16:46, Sergey Dyasli wrote: >>> Hide the following information that can help identify the running Xen >>> binary version: >>> >>> XENVER_extraversion >>> XENVER_compile_info >>> XENVER_capabilities >> >> What's wrong with exposing this one? > > extraversion can help identify the exact running Xen binary (and I must > say that at Citrix we add some additional version information there). > compile_info will identify compiler and many speculative mitigations > are provided by compilers these days. Not sure if it's worth hiding > capabilities, but OTOH I don't see how guests could meaningfully use it. Well, my question (using "this", not "these") was really mainly on the last item. I can see how extraversion can provide clues. I'm having difficulty seeing how the compiler (little bit of) details can provide sufficient information to become leveragable. >>> XENVER_changeset >>> XENVER_commandline >>> XENVER_build_id >>> >>> Return a more customer friendly empty string instead of "" >>> which would be shown in tools like dmidecode.> >> I think "" is quite fine for many of the original purposes. >> Maybe it would be better to filter for this when populating guest >> DMI tables? > > I don't know how DMI tables are populated, but nothing stops a guest > from using these hypercalls directly. And this is precisely the case where I think "" is better than an empty string. >>> return xsm_default_action(XSM_HOOK, current->domain, NULL); >>> + >>> +case XENVER_extraversion: >>> +case XENVER_compile_info: >>> +case XENVER_capabilities: >>> +case XENVER_changeset: >>> +case XENVER_commandline: >>> +case XENVER_build_id: >>> default: >> >> There's no need to add all of these next to the default case. >> Note how commandline and build_id have been coming here already >> (and there would need to be further justification for exposing >> them on debug builds, should this questionable behavior - see >> above - be retained). > > I find that explicitly listing all the cases is better for code > readability, but I don't have a strong opinion here. Well, I'm viewing it kind of the opposite, as being unnecessary clutter (and hence harming readability). We'll see what others think. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/save: reserve HVM save record numbers that have been consumed...
On 19/12/2019 11:06, Durrant, Paul wrote: >> It is not fair or reasonable to include extra headroom in a "oh dear we >> screwed up - will the community be kind enough to help us work around >> our ABI problems" scenario. >> > I would have thought all the pain you went through to keep XenServer going > with its non-upstreamed hypercall numbers would have made you a little more > sympathetic to dealing with past mistakes. I could object to the principle of the patch, if you'd prefer :) If you recall for the legacy window PV driver ABI breakages, I didn't actually reserve any numbers upstream in the end. All compatibility was handled locally. >> For now, I'd just leave it as a comment, and strictly only covering the >> ones you have used. There is no need to actually bump the structure >> sizes in xen for now - simply that the ones you have currently used >> don't get allocated for something different in the future. >> > Ok, we can defer actually bumping HVM_SAVE_CODE_MAX, but it's almost certain > to happen eventually. That's fine. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/hvm/rtc: preserved guest RTC offset during suspend/resume/migrate
On 18.12.2019 15:41, Paul Durrant wrote:
> The emulated RTC is synchronized with the PV wallclock; any write to the
> RTC will update struct domain's 'time_offset_seconds' field and call
> update_domain_wallclock().
>
> However, the value of 'time_offset_seconds' is not preserved in any save
> record and indeed, when the RTC save record is loaded, the CMOS values
> will be updated based on an offset value which may or may not have been
> set by the toolstack [1]. This may result in making bogus values available
> to the guest and messing up any calculations done in the call to
> alarm_timer_update() at the end of rtc_load().
>
> This patch extends the RTC save record to contain an offset value, which
> will be zero filled on load of an older record. The 'time_offset_secoonds'
> field in struct domain is also modified into a 'time_offset' struct,
> containing a 'seconds' field and a boolean 'set' field.
>
> The code in rtc_load() then uses the new value in the save record to
> update the value of struct domain's 'time_offset.seconds' unless
> 'time_offset.set' is true, which will only be the case if the toolstack has
> already performed a XEN_DOMCTL_settimeoffset.
>
> [1] There is currently no way for a toolstack to read the value of
> 'time_offset_seconds' from struct domain. In the past, any hope of
> preservation of the value across a guest life-cycle operation was based
> on relying on qemu-dm to write a value into xenstore whenever the RTC
> was updated, in response to an IOREQ with type IOREQ_TYPE_TIMEOFFSET
> being sent by Xen; see:
>
>
> https://xenbits.xen.org/gitweb/?p=qemu-xen-traditional.git;a=blob;f=i386-dm/helper2.c#l457
>
> but this behaviour was never forward-ported into upstream QEMU, which
> completely ignores that IOREQ type.
> In either case, nothing in xl or libxl ever samples the value of
> RTC offset from xenstore so any offset adjustment to a non-zero value
> performed by the guest (which in the case of Windows is highly likely
> as it normally writes RTC in local time, whereas Xen maintains time in
> UTC) is completely lost with the de-facto toolstack, and always has
> been. Instead, PV drivers are relied upon to paper over this gaping
> hole.
>
> Signed-off-by: Paul Durrant
Reviewed-by: Jan Beulich
with one remark:
> @@ -771,6 +773,12 @@ static int rtc_load(struct domain *d,
> hvm_domain_context_t *h)
>
> /* Reset the wall-clock time. In normal running, this runs with host
> * time, so let's keep doing that. */
> +if ( !d->time_offset.set )
> +{
> +d->time_offset.seconds = s->hw.rtc_offset;
> +update_domain_wallclock_time(d);
> +}
It's not really clear to me which of the possible behaviors is the
better one - overriding a tool stack set value with what the save
record says, or (like you do) the other way around.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests
On 18/12/2019 11:06, Jan Beulich wrote: > On 17.12.2019 16:46, Sergey Dyasli wrote: >> Hide the following information that can help identify the running Xen >> binary version: >> >> XENVER_extraversion >> XENVER_compile_info >> XENVER_capabilities > > What's wrong with exposing this one? extraversion can help identify the exact running Xen binary (and I must say that at Citrix we add some additional version information there). compile_info will identify compiler and many speculative mitigations are provided by compilers these days. Not sure if it's worth hiding capabilities, but OTOH I don't see how guests could meaningfully use it. > >> XENVER_changeset >> XENVER_commandline >> XENVER_build_id >> >> Return a more customer friendly empty string instead of "" >> which would be shown in tools like dmidecode.> > I think "" is quite fine for many of the original purposes. > Maybe it would be better to filter for this when populating guest > DMI tables? I don't know how DMI tables are populated, but nothing stops a guest from using these hypercalls directly. > >> But allow guests to see this information in Debug builds of Xen. > > Behavior like this would imo better not differ between debug and > release builds, or else guest software may behave entirely > differently once you put it on a production build. I agree on this one, it's not much worth providing this information in debug builds. > >> --- a/xen/include/xsm/dummy.h >> +++ b/xen/include/xsm/dummy.h >> @@ -750,16 +750,21 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG >> uint32_t op) >> case XENVER_get_features: >> /* These sub-ops ignore the permission checks and return data. */ >> return 0; >> -case XENVER_extraversion: >> -case XENVER_compile_info: >> -case XENVER_capabilities: >> -case XENVER_changeset: >> case XENVER_pagesize: >> case XENVER_guest_handle: >> /* These MUST always be accessible to any guest by default. */ > > This comment, not the least because of its use of capitals, > suggests to me that there's further justification needed for > your change, including discussion of why there's no risk of > breaking existing guests. Not sure about this comment, maybe the author (Konrad) remembers? We had this change in production for very long time now and haven't seen any guest regressions. > >> return xsm_default_action(XSM_HOOK, current->domain, NULL); >> + >> +case XENVER_extraversion: >> +case XENVER_compile_info: >> +case XENVER_capabilities: >> +case XENVER_changeset: >> +case XENVER_commandline: >> +case XENVER_build_id: >> default: > > There's no need to add all of these next to the default case. > Note how commandline and build_id have been coming here already > (and there would need to be further justification for exposing > them on debug builds, should this questionable behavior - see > above - be retained). I find that explicitly listing all the cases is better for code readability, but I don't have a strong opinion here. -- Thanks, Sergey ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen-pciback: optionally allow interrupt enable flag writes
On 19.12.2019 04:49, Marek Marczykowski-Górecki wrote:
> +enum interrupt_type xen_pcibk_get_interrupt_type(struct pci_dev *dev)
> +{
> + int err;
> + u16 val;
> +
> + err = pci_read_config_word(dev, PCI_COMMAND, );
> + if (err)
> + return INTERRUPT_TYPE_ERR;
> + if (!(val & PCI_COMMAND_INTX_DISABLE))
> + return INTERRUPT_TYPE_INTX;
> +
> + /* Do not trust dev->msi(x)_enabled here, as enabling could be done
> + * bypassing the pci_*msi* functions, by the qemu.
> + */
Judging from this comment, how can you assume only one of the
three variants is actually enabled? It's against the spec, yes,
but it's not at all impossible afaict. I think you want the
return value here to be
- negative errno values (no need to discard the actual error
codes) or
- a non-negative bitmap indicating which of the interrupt types
is/are currently enabled.
That way ...
> +static int msi_msix_flags_write(struct pci_dev *dev, int offset, u16
> new_value,
> + void *data)
> +{
> + int err;
> + u16 old_value;
> + const struct msi_msix_field_config *field_config = data;
> + const struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(dev);
> +
> + if (xen_pcibk_permissive || dev_data->permissive)
> + goto write;
> +
> + err = pci_read_config_word(dev, offset, _value);
> + if (err)
> + return err;
> +
> + if (new_value == old_value)
> + return 0;
> +
> + if (!dev_data->allow_interrupt_control ||
> + (new_value ^ old_value) & ~field_config->enable_bit)
> + return PCIBIOS_SET_FAILED;
> +
> + if (new_value & field_config->enable_bit) {
> + /* don't allow enabling together with other interrupt types */
> + const enum interrupt_type int_type =
> xen_pcibk_get_interrupt_type(dev);
> + if (int_type == INTERRUPT_TYPE_NONE ||
> + int_type == field_config->int_type)
... equality comparisons like this one will actually become safe.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 04/20] x86/mem_sharing: cleanup code and comments in various locations
On 18/12/2019 19:40, Tamas K Lengyel wrote:
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 5a3a962fbb..1e888b403b 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1902,12 +1902,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned
> long gla,
> if ( npfec.write_access && (p2mt == p2m_ram_shared) )
> {
> ASSERT(p2m_is_hostp2m(p2m));
> -sharing_enomem =
> -(mem_sharing_unshare_page(currd, gfn, 0) < 0);
> +sharing_enomem = mem_sharing_unshare_page(currd, gfn, 0);
This is a logical change. Is it intended to be in a later patch?
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index efb8821768..319aaf3074 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -198,24 +200,26 @@ static inline shr_handle_t get_next_handle(void)
> #define mem_sharing_enabled(d) \
> (is_hvm_domain(d) && (d)->arch.hvm.mem_sharing_enabled)
>
> -static atomic_t nr_saved_mfns = ATOMIC_INIT(0);
> +static atomic_t nr_saved_mfns = ATOMIC_INIT(0);
> static atomic_t nr_shared_mfns = ATOMIC_INIT(0);
>
> -/** Reverse map **/
> -/* Every shared frame keeps a reverse map (rmap) of tuples that
> +/*
> + * Reverse map
> + *
> + * Every shared frame keeps a reverse map (rmap) of tuples that
> * this shared frame backs. For pages with a low degree of sharing, a O(n)
> * search linked list is good enough. For pages with higher degree of
> sharing,
> - * we use a hash table instead. */
> + * we use a hash table instead.
> + */
>
> typedef struct gfn_info
> {
> unsigned long gfn;
> -domid_t domain;
> +domid_t domain;
> struct list_head list;
> } gfn_info_t;
>
> -static inline void
> -rmap_init(struct page_info *page)
> +static inline void rmap_init(struct page_info *page)
Seeing as you're folding this, the inline can be dropped. In .c files,
functions should just be static.
> @@ -437,25 +441,29 @@ static inline void mem_sharing_gfn_destroy(struct
> page_info *page,
> xfree(gfn_info);
> }
>
> -static struct page_info* mem_sharing_lookup(unsigned long mfn)
> +static inline struct page_info* mem_sharing_lookup(unsigned long mfn)
Seeing as this is cleanup, the position of the * can move. Similarly,
it shouldn't gain an inline.
I can fix all of this up on commit (and a few other brace position
issues) if you want, to save reworking a trivial v2.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/save: reserve HVM save record numbers that have been consumed...
On 19.12.2019 12:06, Durrant, Paul wrote: >> -Original Message- >> From: Andrew Cooper >> Sent: 19 December 2019 10:52 >> To: Durrant, Paul ; xen-devel@lists.xenproject.org >> Cc: Wei Liu ; Jan Beulich ; Roger Pau Monné >> >> Subject: Re: [Xen-devel] [PATCH] x86/save: reserve HVM save record numbers >> that have been consumed... >> >> On 19/12/2019 08:52, Durrant, Paul wrote: -Original Message- From: Xen-devel On Behalf Of Andrew Cooper Sent: 18 December 2019 19:45 To: Durrant, Paul ; xen-devel@lists.xenproject.org Cc: Wei Liu ; Jan Beulich ; Roger Pau >> Monné Subject: Re: [Xen-devel] [PATCH] x86/save: reserve HVM save record >> numbers that have been consumed... On 18/12/2019 16:09, Paul Durrant wrote: > ...for patches not (yet) upstream. > > This patch is simply reserving save record number space to avoid the > risk of clashes between existent downstream changes made by Amazon and > future upstream changes which may be incompatible. > > Signed-off-by: Paul Durrant Is this "you've already used some of these", or you plan to? >>> Already used in code that has been deployed, although I have left some >> headroom because I know there is code in development which is using new >> ones. >>> >>> Where records can be upstreamed in a way that is compatible with >> downstream use, we will keep the existing number. If incompatible changes >> are necessary to get the code upstream then we will have to use a new >> number and maintain downstream compatibility patches. >> >> Every bump to this number is more wasted memory in Xen. > > How much more memory? It is, btw, not just memory, but also a higher number of loop iterations. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
On 19.12.2019 11:03, Lars Kurth wrote: > > >> On 19 Dec 2019, at 09:56, Jan Beulich wrote: >> >> On 18.12.2019 18:09, Lars Kurth wrote: >>> >>> >>> On 18/12/2019, 14:29, "Julien Grall" wrote: >>> >>>Hi Lars, >>> >>>On 12/12/2019 21:14, Lars Kurth wrote: +### Workflow from an Author's Perspective + +When code authors receive feedback on their patches, they typically first try +to clarify feedback they do not understand. For smaller patches or patch series +it makes sense to wait until receiving feedback on the entire series before +sending out a new version addressing the changes. For larger series, it may +make sense to send out a new revision earlier. + +As a reviewer, you need some system that he;ps ensure that you address all >>> >>>Just a small typo: I think you meant "helps" rather than "he;ps". >>> >>>Cheers, >>> >>> Thank you: fixed in my working copy. >>> >>> One thing which occurred to me for reviews like these, where there is no >>> ACK's or Reviewed-by's is that I don't actually know whether you as >>> reviewer is otherwise happy with the remainder of the patch. >>> Normally the ACKed-by or Reviewed-by is a signal that it is >>> >>> I am assuming it is, but I think it may be worthwhile pointing this out in >>> the document, that unless stated otherwise, the reviewer is happy with the >>> patch >> >> I don't think there should ever be such an implication. Afaic there >> are patches I comment upon, but that I either don't feel qualified >> to give an ack/R-b on, or that I simply don't want to for whatever >> reason. At best, no other comment (as in the above example) may be >> taken as "I don't object". > > > If that is the case, would there be a reasonable convention to make this > clear? > > In a nutshell, in such a review the possible interpretations are > * I reviewed, but didn't feel qualified to do the rest > * I reviewed, but did not get round to give it full attention > * I reviewed, but before I make a final decision want to look at the next > version > ... > * I reviewed and don't object the rest > * I reviewed and agreed with the rest > The latter two are equivalent to Ack/R-b > > That is quite a large range of possibilities and kind of leaves the author > guessing what state the review is in Well, I though the convention is to give A-b / R-b explicitly. In a few overly ambiguous cases we tend to simply ask back whether a given reply can be transformed into a tag. I don't see any need for further formalization here. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/save: reserve HVM save record numbers that have been consumed...
> -Original Message- > From: Andrew Cooper > Sent: 19 December 2019 10:52 > To: Durrant, Paul ; xen-devel@lists.xenproject.org > Cc: Wei Liu ; Jan Beulich ; Roger Pau Monné > > Subject: Re: [Xen-devel] [PATCH] x86/save: reserve HVM save record numbers > that have been consumed... > > On 19/12/2019 08:52, Durrant, Paul wrote: > >> -Original Message- > >> From: Xen-devel On Behalf Of > >> Andrew Cooper > >> Sent: 18 December 2019 19:45 > >> To: Durrant, Paul ; xen-devel@lists.xenproject.org > >> Cc: Wei Liu ; Jan Beulich ; Roger Pau > Monné > >> > >> Subject: Re: [Xen-devel] [PATCH] x86/save: reserve HVM save record > numbers > >> that have been consumed... > >> > >> On 18/12/2019 16:09, Paul Durrant wrote: > >>> ...for patches not (yet) upstream. > >>> > >>> This patch is simply reserving save record number space to avoid the > >>> risk of clashes between existent downstream changes made by Amazon and > >>> future upstream changes which may be incompatible. > >>> > >>> Signed-off-by: Paul Durrant > >> Is this "you've already used some of these", or you plan to? > > Already used in code that has been deployed, although I have left some > headroom because I know there is code in development which is using new > ones. > > > > Where records can be upstreamed in a way that is compatible with > downstream use, we will keep the existing number. If incompatible changes > are necessary to get the code upstream then we will have to use a new > number and maintain downstream compatibility patches. > > Every bump to this number is more wasted memory in Xen. > How much more memory? > It is not fair or reasonable to include extra headroom in a "oh dear we > screwed up - will the community be kind enough to help us work around > our ABI problems" scenario. > I would have thought all the pain you went through to keep XenServer going with its non-upstreamed hypercall numbers would have made you a little more sympathetic to dealing with past mistakes. > For now, I'd just leave it as a comment, and strictly only covering the > ones you have used. There is no need to actually bump the structure > sizes in xen for now - simply that the ones you have currently used > don't get allocated for something different in the future. > Ok, we can defer actually bumping HVM_SAVE_CODE_MAX, but it's almost certain to happen eventually. Paul ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 19/20] x86/mem_sharing: reset a fork
On 19.12.2019 01:15, Tamas K Lengyel wrote: > On Wed, Dec 18, 2019 at 4:02 PM Julien Grall wrote: >> On 18/12/2019 22:33, Tamas K Lengyel wrote: >>> On Wed, Dec 18, 2019 at 3:00 PM Julien Grall wrote: You also have multiple loop on the page_list in this function. Given the number of page_list can be quite big, this is a call for hogging the pCPU and an RCU lock on the domain vCPU running this call. >>> >>> There is just one loop over page_list itself, the second loop is on >>> the internal list that is being built here which will be a subset. The >>> list itself in fact should be small (in our tests usually <100). >> >> For a first, nothing in this function tells me that there will be only >> 100 pages. But then, I don't think this is right to implement your >> hypercall based only the "normal" scenario. You should also think about >> the "worst" case scenario. >> >> In this case the worst case scenario is have hundreds of page in page_list. > > Well, this is only an experimental system that's completely disabled > by default. Making the assumption that people who make use of it will > know what they are doing I think is fair. FWIW I'm with Julien here: The preferred course of action is to make the operation safe against abuse. The minimum requirement is to document obvious missing pieces for this to become supported code. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/save: reserve HVM save record numbers that have been consumed...
> -Original Message-
> From: Jan Beulich
> Sent: 19 December 2019 10:33
> To: Durrant, Paul
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> ; Wei Liu ; Roger Pau Monné
>
> Subject: Re: [PATCH] x86/save: reserve HVM save record numbers that have
> been consumed...
>
> On 18.12.2019 17:09, Paul Durrant wrote:
> > --- a/xen/include/public/arch-x86/hvm/save.h
> > +++ b/xen/include/public/arch-x86/hvm/save.h
> > @@ -639,10 +639,12 @@ struct hvm_msr {
> >
> > #define CPU_MSR_CODE 20
> >
> > +/* Range 22 - 40 reserved for Amazon */
> > +
> > /*
> > * Largest type-code in use
> > */
> > -#define HVM_SAVE_CODE_MAX 20
> > +#define HVM_SAVE_CODE_MAX 40
>
> I'm not overly happy to see the respective in-Xen array double its
> size for no use at all. I also don't think out-of-tree extensions
> should have been added using numbers consecutive to the upstream
> ones. Instead, an Amazon range should have been picked high up in
> the number space (e.g. with the upper byte being ASCII 'A').
>
Totally agreed, but we don't have a time machine handy.
Paul
> Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/save: reserve HVM save record numbers that have been consumed...
On 19/12/2019 08:52, Durrant, Paul wrote: >> -Original Message- >> From: Xen-devel On Behalf Of >> Andrew Cooper >> Sent: 18 December 2019 19:45 >> To: Durrant, Paul ; xen-devel@lists.xenproject.org >> Cc: Wei Liu ; Jan Beulich ; Roger Pau Monné >> >> Subject: Re: [Xen-devel] [PATCH] x86/save: reserve HVM save record numbers >> that have been consumed... >> >> On 18/12/2019 16:09, Paul Durrant wrote: >>> ...for patches not (yet) upstream. >>> >>> This patch is simply reserving save record number space to avoid the >>> risk of clashes between existent downstream changes made by Amazon and >>> future upstream changes which may be incompatible. >>> >>> Signed-off-by: Paul Durrant >> Is this "you've already used some of these", or you plan to? > Already used in code that has been deployed, although I have left some > headroom because I know there is code in development which is using new ones. > > Where records can be upstreamed in a way that is compatible with downstream > use, we will keep the existing number. If incompatible changes are necessary > to get the code upstream then we will have to use a new number and maintain > downstream compatibility patches. Every bump to this number is more wasted memory in Xen. It is not fair or reasonable to include extra headroom in a "oh dear we screwed up - will the community be kind enough to help us work around our ABI problems" scenario. For now, I'd just leave it as a comment, and strictly only covering the ones you have used. There is no need to actually bump the structure sizes in xen for now - simply that the ones you have currently used don't get allocated for something different in the future. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V5 2/4] x86/altp2m: Add hypercall to set a range of sve bits
On 19.12.2019 10:42, Alexandru Stefan ISAILA wrote:
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -46,6 +46,16 @@ struct xen_hvm_altp2m_suppress_ve {
> uint64_t gfn;
> };
>
> +struct xen_hvm_altp2m_suppress_ve_multi {
> +uint16_t view;
> +uint8_t suppress_ve; /* Boolean type. */
> +uint8_t pad1;
> +int32_t first_error_code; /* Must be set to 0 . */
> +uint64_t first_gfn; /* Value will be updated */
s/will/may/
> +uint64_t last_gfn;
> +uint64_t first_error; /* Gfn of the first error. Must be set to 0. */
There's no real "must" here. Please at most say "should", but I'd
even consider dropping that part of the comment altogether. The
consumer logic needs to key off of the error code anyway. Even
for the error code field I'd suggest saying just "should": You
don't check it (because you can't), and the caller only shoots
itself in the foot if it doesn't do so.
Also looking at this yet again - I think field names would better
be "first_error" for the error code and "first_error_gfn" for the
GFN.
Anyway, preferably with the suggested adjustments, applicable
hypervisor parts
Acked-by: Jan Beulich
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V5 1/4] x86/mm: Add array_index_nospec to guest provided index values
On 19.12.2019 10:42, Alexandru Stefan ISAILA wrote:
> This patch aims to sanitize indexes, potentially guest provided
> values, for altp2m_eptp[] and altp2m_p2m[] arrays.
>
> Requested-by: Jan Beulich
> Signed-off-by: Alexandru Isaila
> ---
> CC: Razvan Cojocaru
> CC: Tamas K Lengyel
> CC: Petre Pircalabu
> CC: George Dunlap
> CC: Jan Beulich
> CC: Andrew Cooper
> CC: Wei Liu
> CC: "Roger Pau Monné"
> CC: Jun Nakajima
> CC: Kevin Tian
> ---
> Changes since V4:
> - Change bounds check from MAX_EPTP to MAX_ALTP2M
> - Move array_index_nospec() closer to the bounds check.
> ---
> xen/arch/x86/mm/mem_access.c | 15 +--
> xen/arch/x86/mm/p2m.c| 20 ++--
> 2 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 320b9fe621..33e379db8f 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -367,10 +367,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn,
> uint32_t nr,
> if ( altp2m_idx )
> {
> if ( altp2m_idx >= MAX_ALTP2M ||
> - d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
> + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_ALTP2M)]
> ==
As implied by a reply to v4, this is still latently buggy: There's
no guarantee anyone will notice the issue here when bumping
MAX_ALTP2M past MAX_EPTP. The only future proof thing to do here
is, as suggested, using some form of min(MAX_ALTP2M, MAX_EPTP) in
the actual bounds check. Then each array access itself can be made
use the correct bound. In fact you'd probably have noticed this if
you had made use of array_access_nospec() where possible (which
also would help readability) - apparently not here, but ...
> + mfn_x(INVALID_MFN) )
> return -EINVAL;
>
> -ap2m = d->arch.altp2m_p2m[altp2m_idx];
> +ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx,
> MAX_ALTP2M)];
... here. The min() suggested above would then better become
min(ARRAY_SIZE(d->arch.altp2m_eptp), MAX_EPTP), which I think
would then even compile cleanly (the apparently simpler form
above wouldn't as is afaict).
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2574,6 +2574,7 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned
> int idx)
> if ( idx >= MAX_ALTP2M )
> return rc;
>
> +idx = array_index_nospec(idx, MAX_ALTP2M);
> altp2m_list_lock(d);
I wouldn't object to there being no blank line between the if()
and the line you add, but you surely want a blank line ahead of
the unrelated lock acquire (similarly at least once more below).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/save: reserve HVM save record numbers that have been consumed...
On 18.12.2019 17:09, Paul Durrant wrote:
> --- a/xen/include/public/arch-x86/hvm/save.h
> +++ b/xen/include/public/arch-x86/hvm/save.h
> @@ -639,10 +639,12 @@ struct hvm_msr {
>
> #define CPU_MSR_CODE 20
>
> +/* Range 22 - 40 reserved for Amazon */
> +
> /*
> * Largest type-code in use
> */
> -#define HVM_SAVE_CODE_MAX 20
> +#define HVM_SAVE_CODE_MAX 40
I'm not overly happy to see the respective in-Xen array double its
size for no use at all. I also don't think out-of-tree extensions
should have been added using numbers consecutive to the upstream
ones. Instead, an Amazon range should have been picked high up in
the number space (e.g. with the upper byte being ASCII 'A').
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
> On 19 Dec 2019, at 09:56, Jan Beulich wrote: > > On 18.12.2019 18:09, Lars Kurth wrote: >> >> >> On 18/12/2019, 14:29, "Julien Grall" wrote: >> >>Hi Lars, >> >>On 12/12/2019 21:14, Lars Kurth wrote: >>> +### Workflow from an Author's Perspective >>> + >>> +When code authors receive feedback on their patches, they typically first >>> try >>> +to clarify feedback they do not understand. For smaller patches or patch >>> series >>> +it makes sense to wait until receiving feedback on the entire series before >>> +sending out a new version addressing the changes. For larger series, it may >>> +make sense to send out a new revision earlier. >>> + >>> +As a reviewer, you need some system that he;ps ensure that you address all >> >>Just a small typo: I think you meant "helps" rather than "he;ps". >> >>Cheers, >> >> Thank you: fixed in my working copy. >> >> One thing which occurred to me for reviews like these, where there is no >> ACK's or Reviewed-by's is that I don't actually know whether you as reviewer >> is otherwise happy with the remainder of the patch. >> Normally the ACKed-by or Reviewed-by is a signal that it is >> >> I am assuming it is, but I think it may be worthwhile pointing this out in >> the document, that unless stated otherwise, the reviewer is happy with the >> patch > > I don't think there should ever be such an implication. Afaic there > are patches I comment upon, but that I either don't feel qualified > to give an ack/R-b on, or that I simply don't want to for whatever > reason. At best, no other comment (as in the above example) may be > taken as "I don't object". If that is the case, would there be a reasonable convention to make this clear? In a nutshell, in such a review the possible interpretations are * I reviewed, but didn't feel qualified to do the rest * I reviewed, but did not get round to give it full attention * I reviewed, but before I make a final decision want to look at the next version ... * I reviewed and don't object the rest * I reviewed and agreed with the rest The latter two are equivalent to Ack/R-b That is quite a large range of possibilities and kind of leaves the author guessing what state the review is in Regards Lars ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
On 18.12.2019 18:09, Lars Kurth wrote: > > > On 18/12/2019, 14:29, "Julien Grall" wrote: > > Hi Lars, > > On 12/12/2019 21:14, Lars Kurth wrote: > > +### Workflow from an Author's Perspective > > + > > +When code authors receive feedback on their patches, they typically > first try > > +to clarify feedback they do not understand. For smaller patches or > patch series > > +it makes sense to wait until receiving feedback on the entire series > before > > +sending out a new version addressing the changes. For larger series, > it may > > +make sense to send out a new revision earlier. > > + > > +As a reviewer, you need some system that he;ps ensure that you address > all > > Just a small typo: I think you meant "helps" rather than "he;ps". > > Cheers, > > Thank you: fixed in my working copy. > > One thing which occurred to me for reviews like these, where there is no > ACK's or Reviewed-by's is that I don't actually know whether you as reviewer > is otherwise happy with the remainder of patch. > Normally the ACKed-by or Reviewed-by is a signal that it is > > I am assuming it is, but I think it may be worthwhile pointing this out in > the document, that unless stated otherwise, the reviewer is happy with the > patch I don't think there should ever be such an implication. Afaic there are patches I comment upon, but that I either don't feel qualified to give an ack/R-b on, or that I simply don't want to for whatever reason. At best, no other comment (as in the above example) may be taken as "I don't object". Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [libvirt test] 144958: tolerable all pass - PUSHED
flight 144958 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/144958/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 144920 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 144920 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 12 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass version targeted for testing: libvirt ce7af78e3907c7462aa88d58facdc4ce7e0acd8d baseline version: libvirt 6c17606b7cce7bf77baef956bde8a0b05011 Last test of basis 144920 2019-12-18 04:18:43 Z1 days Testing same since 144958 2019-12-19 04:19:03 Z0 days1 attempts People who touched revisions under test: Cole Robinson Daniel Henrique Barboza Daniel P. Berrangé Fabiano Fidêncio John Ferlan Michal Privoznik Peter Krempa jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-arm64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-arm64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass test-amd64-amd64-libvirt-xsm pass test-arm64-arm64-libvirt-xsm pass test-amd64-i386-libvirt-xsm pass test-amd64-amd64-libvirt pass test-arm64-arm64-libvirt pass test-armhf-armhf-libvirt pass test-amd64-i386-libvirt pass test-amd64-amd64-libvirt-pairpass test-amd64-i386-libvirt-pair pass test-arm64-arm64-libvirt-qcow2 pass test-armhf-armhf-libvirt-raw pass test-amd64-amd64-libvirt-vhd pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/libvirt.git 6c17606b7c..ce7af78e39 ce7af78e3907c7462aa88d58facdc4ce7e0acd8d -> xen-tested-master ___ Xen-devel mailing list
Re: [Xen-devel] [PATCH v2 00/20] VM forking
On Wed, Dec 18, 2019 at 11:40:37AM -0800, Tamas K Lengyel wrote: > The following series implements VM forking for Intel HVM guests to allow for > the fast creation of identical VMs without the assosciated high startup costs > of booting or restoring the VM from a savefile. > > JIRA issue: https://xenproject.atlassian.net/browse/XEN-89 > > The main design goal with this series has been to reduce the time of creating > the VM fork as much as possible. To achieve this the VM forking process is > split into two steps: > 1) forking the VM on the hypervisor side; > 2) starting QEMU to handle the backed for emulated devices. > > Step 1) involves creating a VM using the new "xl fork-vm" command. The > parent VM is expected to remain paused after forks are created from it (which > is different then what process forking normally entails). During this forking ^ than > operation the HVM context and VM settings are copied over to the new forked > VM. > This operation is fast and it allows the forked VM to be unpaused and to be > monitored and accessed via VMI. Note however that without its device model > running (depending on what is executing in the VM) it is bound to > misbehave/crash when its trying to access devices that would be emulated by > QEMU. We anticipate that for certain use-cases this would be an acceptable > situation, in case for example when fuzzing is performed of code segments that > don't access such devices. > > Step 2) involves launching QEMU to support the forked VM, which requires the > QEMU Xen savefile to be generated manually from the parent VM. This can be > accomplished simply by connecting to its QMP socket and issuing the > "xen-save-devices-state" command as documented by QEMU: > https://github.com/qemu/qemu/blob/master/docs/xen-save-devices-state.txt > Once the QEMU Xen savefile is generated the new "xl fork-launch-dm" command is > used to launch QEMU and load the specified savefile for it. IMO having two different commands is confusing for the end user, I would rather have something like: xl fork-vm [-d] ... Where '-d' would prevent forking any user-space emulators. I don't thinks there's a need for a separate command to fork the underlying user-space emulators. Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH V5 4/4] x86/mm: Make use of the default access param from xc_altp2m_create_view
At this moment the default_access param from xc_altp2m_create_view is
not used.
This patch assigns default_access to p2m->default_access at the time of
initializing a new altp2m view.
Signed-off-by: Alexandru Isaila
---
CC: Jan Beulich
CC: Andrew Cooper
CC: Wei Liu
CC: "Roger Pau Monné"
CC: George Dunlap
CC: Ian Jackson
CC: Julien Grall
CC: Konrad Rzeszutek Wilk
CC: Stefano Stabellini
CC: Razvan Cojocaru
CC: Tamas K Lengyel
CC: Petre Pircalabu
CC: George Dunlap
---
Changes since V4:
- Add const struct p2m_domain *p2m to
xenmem_access_to_p2m_access()
- Pull xenmem_access_to_p2m_access() out of the locked area
- Add a check for NULL p2m in xenmem_access_to_p2m_access().
---
xen/arch/x86/hvm/hvm.c | 3 ++-
xen/arch/x86/mm/mem_access.c| 11 +++
xen/arch/x86/mm/p2m.c | 21 -
xen/include/asm-x86/p2m.h | 3 ++-
xen/include/public/hvm/hvm_op.h | 2 --
xen/include/xen/mem_access.h| 4
6 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 98d1d9788b..d7a55568c9 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4687,7 +4687,8 @@ static int do_altp2m_op(
}
case HVMOP_altp2m_create_p2m:
-if ( !(rc = p2m_init_next_altp2m(d, )) )
+if ( !(rc = p2m_init_next_altp2m(d, ,
+ a.u.view.hvmmem_default_access)) )
rc = __copy_to_guest(arg, , 1) ? -EFAULT : 0;
break;
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 33e379db8f..5b74a6898b 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -314,9 +314,9 @@ static int set_mem_access(struct domain *d, struct
p2m_domain *p2m,
return rc;
}
-static bool xenmem_access_to_p2m_access(struct p2m_domain *p2m,
-xenmem_access_t xaccess,
-p2m_access_t *paccess)
+bool xenmem_access_to_p2m_access(const struct p2m_domain *p2m,
+ xenmem_access_t xaccess,
+ p2m_access_t *paccess)
{
static const p2m_access_t memaccess[] = {
#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
@@ -340,7 +340,10 @@ static bool xenmem_access_to_p2m_access(struct p2m_domain
*p2m,
*paccess = memaccess[xaccess];
break;
case XENMEM_access_default:
-*paccess = p2m->default_access;
+if ( !p2m )
+*paccess = p2m_access_rwx;
+else
+*paccess = p2m->default_access;
break;
default:
return false;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index cb5b8d67d1..2774811bb8 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -25,6 +25,7 @@
#include /* copy_from_guest() */
#include
+#include
#include
#include
#include
@@ -2533,7 +2534,8 @@ void p2m_flush_altp2m(struct domain *d)
altp2m_list_unlock(d);
}
-static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
+static int p2m_activate_altp2m(struct domain *d, unsigned int idx,
+ p2m_access_t hvmmem_default_access)
{
struct p2m_domain *hostp2m, *p2m;
int rc;
@@ -2559,7 +2561,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned
int idx)
goto out;
}
-p2m->default_access = hostp2m->default_access;
+p2m->default_access = hvmmem_default_access;
p2m->domain = hostp2m->domain;
p2m->global_logdirty = hostp2m->global_logdirty;
p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
@@ -2576,6 +2578,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned
int idx)
int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
{
int rc = -EINVAL;
+struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
if ( idx >= MAX_ALTP2M )
return rc;
@@ -2584,16 +2587,23 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned
int idx)
altp2m_list_lock(d);
if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
-rc = p2m_activate_altp2m(d, idx);
+rc = p2m_activate_altp2m(d, idx, hostp2m->default_access);
altp2m_list_unlock(d);
return rc;
}
-int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
+int p2m_init_next_altp2m(struct domain *d, uint16_t *idx,
+ xenmem_access_t hvmmem_default_access)
{
int rc = -EINVAL;
unsigned int i;
+p2m_access_t a;
+struct p2m_domain *p2m;
+
+if ( hvmmem_default_access > XENMEM_access_default ||
+ !xenmem_access_to_p2m_access(NULL, hvmmem_default_access, ) )
+return rc;
altp2m_list_lock(d);
@@ -2602,7 +2612,8 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
continue;
-rc = p2m_activate_altp2m(d, i);
+p2m =
[Xen-devel] [PATCH V5 3/4] x86/mm: Pull out the p2m specifics from p2m_init_altp2m_ept
Requested-by: Jan Beulich Signed-off-by: Alexandru Isaila Reviewed-by: Jan Beulich --- CC: Jun Nakajima CC: Kevin Tian CC: George Dunlap CC: Jan Beulich CC: Andrew Cooper CC: Wei Liu CC: "Roger Pau Monné" --- xen/arch/x86/mm/p2m-ept.c | 6 -- xen/arch/x86/mm/p2m.c | 6 ++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index b5517769c9..d861cd7b51 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -1357,13 +1357,7 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i) struct p2m_domain *hostp2m = p2m_get_hostp2m(d); struct ept_data *ept; -p2m->default_access = hostp2m->default_access; -p2m->domain = hostp2m->domain; - -p2m->global_logdirty = hostp2m->global_logdirty; p2m->ept.ad = hostp2m->ept.ad; -p2m->min_remapped_gfn = gfn_x(INVALID_GFN); -p2m->max_mapped_pfn = p2m->max_remapped_gfn = 0; ept = >ept; ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m)); d->arch.altp2m_eptp[i] = ept->eptp; diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index d92613ebe4..cb5b8d67d1 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -2559,6 +2559,12 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx) goto out; } +p2m->default_access = hostp2m->default_access; +p2m->domain = hostp2m->domain; +p2m->global_logdirty = hostp2m->global_logdirty; +p2m->min_remapped_gfn = gfn_x(INVALID_GFN); +p2m->max_mapped_pfn = p2m->max_remapped_gfn = 0; + p2m_init_altp2m_ept(d, idx); out: -- 2.17.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH V5 2/4] x86/altp2m: Add hypercall to set a range of sve bits
By default the sve bits are not set.
This patch adds a new hypercall, xc_altp2m_set_supress_ve_multi(),
to set a range of sve bits.
The core function, p2m_set_suppress_ve_multi(), does not brake in case
of a error and it is doing a best effort for setting the bits in the
given range. A check for continuation is made in order to have
preemption on big ranges.
The gfn of the first error is stored in
xen_hvm_altp2m_suppress_ve_multi.first_error and the error code is
stored in xen_hvm_altp2m_suppress_ve_multi.first_error_code.
If no error occurred the values will be 0.
Signed-off-by: Alexandru Isaila
---
CC: Ian Jackson
CC: Wei Liu
CC: Andrew Cooper
CC: George Dunlap
CC: Jan Beulich
CC: Julien Grall
CC: Konrad Rzeszutek Wilk
CC: Stefano Stabellini
CC: "Roger Pau Monné"
CC: George Dunlap
CC: Razvan Cojocaru
CC: Tamas K Lengyel
CC: Petre Pircalabu
---
Changes since V4:
- Remove ->first_error and first_error_code from
HVMOP_altp2m_set_suppress_ve_multi check
- Check ->first_error_code so that first_error on gfn 0 can be
saved
- Chage type of first_error_code to int32_t
- Clip ->last_gfn before sanity check.
---
tools/libxc/include/xenctrl.h | 4 +++
tools/libxc/xc_altp2m.c | 33 +
xen/arch/x86/hvm/hvm.c | 20 +++
xen/arch/x86/mm/p2m.c | 64 +
xen/include/public/hvm/hvm_op.h | 13 +++
xen/include/xen/mem_access.h| 3 ++
6 files changed, 137 insertions(+)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index f4431687b3..21a333f2c4 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1923,6 +1923,10 @@ int xc_altp2m_switch_to_view(xc_interface *handle,
uint32_t domid,
uint16_t view_id);
int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
uint16_t view_id, xen_pfn_t gfn, bool sve);
+int xc_altp2m_set_supress_ve_multi(xc_interface *handle, uint32_t domid,
+ uint16_t view_id, xen_pfn_t first_gfn,
+ xen_pfn_t last_gfn, bool sve,
+ xen_pfn_t *error_gfn, int32_t *error_code);
int xc_altp2m_get_suppress_ve(xc_interface *handle, uint32_t domid,
uint16_t view_id, xen_pfn_t gfn, bool *sve);
int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index 09dad0355e..4ba930666a 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -234,6 +234,39 @@ int xc_altp2m_set_suppress_ve(xc_interface *handle,
uint32_t domid,
return rc;
}
+int xc_altp2m_set_supress_ve_multi(xc_interface *handle, uint32_t domid,
+ uint16_t view_id, xen_pfn_t first_gfn,
+ xen_pfn_t last_gfn, bool sve,
+ xen_pfn_t *error_gfn, int32_t *error_code)
+{
+int rc;
+DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
+
+arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
+if ( arg == NULL )
+return -1;
+
+arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
+arg->cmd = HVMOP_altp2m_set_suppress_ve_multi;
+arg->domain = domid;
+arg->u.suppress_ve_multi.view = view_id;
+arg->u.suppress_ve_multi.first_gfn = first_gfn;
+arg->u.suppress_ve_multi.last_gfn = last_gfn;
+arg->u.suppress_ve_multi.suppress_ve = sve;
+
+rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
+ HYPERCALL_BUFFER_AS_ARG(arg));
+
+if ( arg->u.suppress_ve_multi.first_error )
+{
+*error_gfn = arg->u.suppress_ve_multi.first_error;
+*error_code = arg->u.suppress_ve_multi.first_error_code;
+}
+
+xc_hypercall_buffer_free(handle, arg);
+return rc;
+}
+
int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
uint16_t view_id, xen_pfn_t gfn,
xenmem_access_t access)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 47573f71b8..98d1d9788b 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4553,6 +4553,7 @@ static int do_altp2m_op(
case HVMOP_altp2m_destroy_p2m:
case HVMOP_altp2m_switch_p2m:
case HVMOP_altp2m_set_suppress_ve:
+case HVMOP_altp2m_set_suppress_ve_multi:
case HVMOP_altp2m_get_suppress_ve:
case HVMOP_altp2m_set_mem_access:
case HVMOP_altp2m_set_mem_access_multi:
@@ -4711,6 +4712,25 @@ static int do_altp2m_op(
}
break;
+case HVMOP_altp2m_set_suppress_ve_multi:
+{
+uint64_t max_phys_addr = (1UL << d->arch.cpuid->extd.maxphysaddr) - 1;
+
+a.u.suppress_ve_multi.last_gfn = min(a.u.suppress_ve_multi.last_gfn,
+ max_phys_addr);
+
+if (
[Xen-devel] [PATCH V5 1/4] x86/mm: Add array_index_nospec to guest provided index values
This patch aims to sanitize indexes, potentially guest provided
values, for altp2m_eptp[] and altp2m_p2m[] arrays.
Requested-by: Jan Beulich
Signed-off-by: Alexandru Isaila
---
CC: Razvan Cojocaru
CC: Tamas K Lengyel
CC: Petre Pircalabu
CC: George Dunlap
CC: Jan Beulich
CC: Andrew Cooper
CC: Wei Liu
CC: "Roger Pau Monné"
CC: Jun Nakajima
CC: Kevin Tian
---
Changes since V4:
- Change bounds check from MAX_EPTP to MAX_ALTP2M
- Move array_index_nospec() closer to the bounds check.
---
xen/arch/x86/mm/mem_access.c | 15 +--
xen/arch/x86/mm/p2m.c| 20 ++--
2 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 320b9fe621..33e379db8f 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -367,10 +367,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn,
uint32_t nr,
if ( altp2m_idx )
{
if ( altp2m_idx >= MAX_ALTP2M ||
- d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
+ d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_ALTP2M)] ==
+ mfn_x(INVALID_MFN) )
return -EINVAL;
-ap2m = d->arch.altp2m_p2m[altp2m_idx];
+ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];
}
#else
ASSERT(!altp2m_idx);
@@ -426,10 +427,11 @@ long p2m_set_mem_access_multi(struct domain *d,
if ( altp2m_idx )
{
if ( altp2m_idx >= MAX_ALTP2M ||
- d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
+ d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_ALTP2M)] ==
+ mfn_x(INVALID_MFN) )
return -EINVAL;
-ap2m = d->arch.altp2m_p2m[altp2m_idx];
+ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];
}
#else
ASSERT(!altp2m_idx);
@@ -492,10 +494,11 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn,
xenmem_access_t *access,
else if ( altp2m_idx ) /* altp2m view 0 is treated as the hostp2m */
{
if ( altp2m_idx >= MAX_ALTP2M ||
- d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
+ d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_ALTP2M)] ==
+ mfn_x(INVALID_MFN) )
return -EINVAL;
-p2m = d->arch.altp2m_p2m[altp2m_idx];
+p2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];
}
#else
ASSERT(!altp2m_idx);
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index ba126f790a..16039c7a57 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2574,6 +2574,7 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int
idx)
if ( idx >= MAX_ALTP2M )
return rc;
+idx = array_index_nospec(idx, MAX_ALTP2M);
altp2m_list_lock(d);
if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
@@ -2615,6 +2616,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned
int idx)
if ( !idx || idx >= MAX_ALTP2M )
return rc;
+idx = array_index_nospec(idx, MAX_ALTP2M);
rc = domain_pause_except_self(d);
if ( rc )
return rc;
@@ -2686,11 +2688,13 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned
int idx,
mfn_t mfn;
int rc = -EINVAL;
-if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
+if ( idx >= MAX_ALTP2M ||
+ d->arch.altp2m_eptp[array_index_nospec(idx, MAX_ALTP2M)] ==
+ mfn_x(INVALID_MFN) )
return rc;
hp2m = p2m_get_hostp2m(d);
-ap2m = d->arch.altp2m_p2m[idx];
+ap2m = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)];
p2m_lock(hp2m);
p2m_lock(ap2m);
@@ -3030,10 +3034,12 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn,
bool suppress_ve,
if ( altp2m_idx > 0 )
{
if ( altp2m_idx >= MAX_ALTP2M ||
- d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
+ d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_ALTP2M)] ==
+ mfn_x(INVALID_MFN) )
return -EINVAL;
-p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx];
+p2m = ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx,
+ MAX_ALTP2M)];
}
else
p2m = host_p2m;
@@ -3073,10 +3079,12 @@ int p2m_get_suppress_ve(struct domain *d, gfn_t gfn,
bool *suppress_ve,
if ( altp2m_idx > 0 )
{
if ( altp2m_idx >= MAX_ALTP2M ||
- d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
+ d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_ALTP2M)] ==
+ mfn_x(INVALID_MFN) )
return -EINVAL;
-p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx];
+p2m = ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx,
+ MAX_ALTP2M)];
}
