Re: [PATCH v4 04/10] x86/vmx: implement processor tracing for VMX
On Tue, Jun 30, 2020 at 02:33:47PM +0200, Michał Leszczyński wrote:
> From: Michal Leszczynski
>
> Use Intel Processor Trace feature in order to
> provision vmtrace_pt_* features.
>
> Signed-off-by: Michal Leszczynski
> ---
> xen/arch/x86/hvm/vmx/vmx.c | 89 ++
> xen/include/asm-x86/hvm/hvm.h | 38 +
> xen/include/asm-x86/hvm/vmx/vmcs.h | 3 +
> xen/include/asm-x86/hvm/vmx/vmx.h | 14 +
> 4 files changed, 144 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index ab19d9424e..db3f051b40 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -508,11 +508,24 @@ static void vmx_restore_host_msrs(void)
>
> static void vmx_save_guest_msrs(struct vcpu *v)
> {
> +uint64_t rtit_ctl;
> +
> /*
> * We cannot cache SHADOW_GS_BASE while the VCPU runs, as it can
> * be updated at any time via SWAPGS, which we cannot trap.
> */
> v->arch.hvm.vmx.shadow_gs = rdgsshadow();
> +
> +if ( unlikely(v->arch.hvm.vmx.pt_state &&
> + v->arch.hvm.vmx.pt_state->active) )
> +{
Nit: define rtit_ctl here to reduce the scope.
> +rdmsrl(MSR_RTIT_CTL, rtit_ctl);
> +BUG_ON(rtit_ctl & RTIT_CTL_TRACEEN);
> +
> +rdmsrl(MSR_RTIT_STATUS, v->arch.hvm.vmx.pt_state->status);
> +rdmsrl(MSR_RTIT_OUTPUT_MASK,
> + v->arch.hvm.vmx.pt_state->output_mask.raw);
> +}
> }
>
> static void vmx_restore_guest_msrs(struct vcpu *v)
> @@ -524,6 +537,17 @@ static void vmx_restore_guest_msrs(struct vcpu *v)
>
> if ( cpu_has_msr_tsc_aux )
> wrmsr_tsc_aux(v->arch.msrs->tsc_aux);
> +
> +if ( unlikely(v->arch.hvm.vmx.pt_state &&
> + v->arch.hvm.vmx.pt_state->active) )
> +{
> +wrmsrl(MSR_RTIT_OUTPUT_BASE,
> + v->arch.hvm.vmx.pt_state->output_base);
> +wrmsrl(MSR_RTIT_OUTPUT_MASK,
> + v->arch.hvm.vmx.pt_state->output_mask.raw);
> +wrmsrl(MSR_RTIT_STATUS,
> + v->arch.hvm.vmx.pt_state->status);
> +}
> }
>
> void vmx_update_cpu_exec_control(struct vcpu *v)
> @@ -2240,6 +2264,60 @@ static bool vmx_get_pending_event(struct vcpu *v,
> struct x86_event *info)
> return true;
> }
>
> +static int vmx_init_pt(struct vcpu *v)
> +{
> +v->arch.hvm.vmx.pt_state = xzalloc(struct pt_state);
> +
> +if ( !v->arch.hvm.vmx.pt_state )
> +return -EFAULT;
-ENOMEM
> +
> +if ( !v->arch.vmtrace.pt_buf )
Agian, I'm quite sure this doesn't build, since pt_buf is introduced
in patch 5.
I will try to continue to review, but it's quite hard when fields not
yet introduced are used in the code, as I have no idea what that is.
> +return -EINVAL;
> +
> +if ( !v->domain->vmtrace_pt_size )
> + return -EINVAL;
Indentation (hard tab), and could be joined with the previous check,
since both return -EINVAL.
> +
> +v->arch.hvm.vmx.pt_state->output_base =
> page_to_maddr(v->arch.vmtrace.pt_buf);
> +v->arch.hvm.vmx.pt_state->output_mask.raw = v->domain->vmtrace_pt_size -
> 1;
> +
> +if ( vmx_add_host_load_msr(v, MSR_RTIT_CTL, 0) )
> +return -EFAULT;
> +
> +if ( vmx_add_guest_msr(v, MSR_RTIT_CTL,
> + RTIT_CTL_TRACEEN | RTIT_CTL_OS |
> + RTIT_CTL_USR | RTIT_CTL_BRANCH_EN) )
> +return -EFAULT;
I think I've already pointed this out before (in v2), but please don't
drop the returned error codes from vmx_add_host_load_msr and
vmx_add_guest_msr. Please store them in a local variable and return
those if != 0.
> +
> +return 0;
> +}
> +
> +static int vmx_destroy_pt(struct vcpu* v)
> +{
> +if ( v->arch.hvm.vmx.pt_state )
> +xfree(v->arch.hvm.vmx.pt_state);
> +
> +v->arch.hvm.vmx.pt_state = NULL;
> +return 0;
> +}
I think those should be port of vmx_vcpu_{initialise/destroy}, there's
no need to introduce new hooks for it? As the allocation size will be
known at domain creation already.
> +static int vmx_control_pt(struct vcpu *v, bool_t enable)
Plain bool.
> +{
> +if ( !v->arch.hvm.vmx.pt_state )
> +return -EINVAL;
> +
> +v->arch.hvm.vmx.pt_state->active = enable;
> +return 0;
> +}
> +
> +static int vmx_get_pt_offset(struct vcpu *v, uint64_t *offset)
> +{
> +if ( !v->arch.hvm.vmx.pt_state )
> +return -EINVAL;
> +
> +*offset = v->arch.hvm.vmx.pt_state->output_mask.offset;
> +return 0;
> +}
> +
> static struct hvm_function_table __initdata vmx_function_table = {
> .name = "VMX",
> .cpu_up_prepare = vmx_cpu_up_prepare,
> @@ -2295,6 +2373,10 @@ static struct hvm_function_table __initdata
> vmx_function_table = {
> .altp2m_vcpu_update_vmfunc_ve = vmx_vcpu_update_vmfunc_ve,
> .altp2m_vcpu_emulate_ve = vmx_vcpu_emulate_ve,
> .altp2m_vcpu_emulate_vmfunc = vmx_vcpu_emulate_vmfunc,
> +.vmtrace_init_pt = vmx_init_pt,
> +
Re: [PATCH 2/2] xen: cleanup unrealized flash devices
On Wed, Jul 1, 2020 at 3:03 AM Paul Durrant wrote:
>
> > -Original Message-
> > From: Philippe Mathieu-Daudé
> > Sent: 30 June 2020 18:27
> > To: p...@xen.org; xen-devel@lists.xenproject.org; qemu-de...@nongnu.org
> > Cc: 'Eduardo Habkost' ; 'Michael S. Tsirkin'
> > ; 'Paul Durrant'
> > ; 'Jason Andryuk' ; 'Paolo
> > Bonzini' ;
> > 'Richard Henderson'
> > Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> >
> > On 6/30/20 5:44 PM, Paul Durrant wrote:
> > >> -Original Message-
> > >> From: Philippe Mathieu-Daudé
> > >> Sent: 30 June 2020 16:26
> > >> To: Paul Durrant ; xen-devel@lists.xenproject.org;
> > >> qemu-de...@nongnu.org
> > >> Cc: Eduardo Habkost ; Michael S. Tsirkin
> > >> ; Paul Durrant
> > >> ; Jason Andryuk ; Paolo Bonzini
> > >> ;
> > >> Richard Henderson
> > >> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> > >>
> > >> On 6/24/20 2:18 PM, Paul Durrant wrote:
> > >>> From: Paul Durrant
> > >>>
> > >>> The generic pc_machine_initfn() calls pc_system_flash_create() which
> > >>> creates
> > >>> 'system.flash0' and 'system.flash1' devices. These devices are then
> > >>> realized
> > >>> by pc_system_flash_map() which is called from pc_system_firmware_init()
> > >>> which
> > >>> itself is called via pc_memory_init(). The latter however is not called
> > >>> when
> > >>> xen_enable() is true and hence the following assertion fails:
> > >>>
> > >>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> > >>> Assertion `dev->realized' failed
> > >>>
> > >>> These flash devices are unneeded when using Xen so this patch avoids the
> > >>> assertion by simply removing them using
> > >>> pc_system_flash_cleanup_unused().
> > >>>
> > >>> Reported-by: Jason Andryuk
> > >>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with
> > >>> -blockdev")
> > >>> Signed-off-by: Paul Durrant
> > >>> Tested-by: Jason Andryuk
> > >>> ---
> > >>> Cc: Paolo Bonzini
> > >>> Cc: Richard Henderson
> > >>> Cc: Eduardo Habkost
> > >>> Cc: "Michael S. Tsirkin"
> > >>> Cc: Marcel Apfelbaum
> > >>> ---
> > >>> hw/i386/pc_piix.c| 9 ++---
> > >>> hw/i386/pc_sysfw.c | 2 +-
> > >>> include/hw/i386/pc.h | 1 +
> > >>> 3 files changed, 8 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > >>> index 1497d0e4ae..977d40afb8 100644
> > >>> --- a/hw/i386/pc_piix.c
> > >>> +++ b/hw/i386/pc_piix.c
> > >>> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
> > >>> if (!xen_enabled()) {
> > >>> pc_memory_init(pcms, system_memory,
> > >>> rom_memory, _memory);
> > >>> -} else if (machine->kernel_filename != NULL) {
> > >>> -/* For xen HVM direct kernel boot, load linux here */
> > >>> -xen_load_linux(pcms);
> > >>> +} else {
> > >>> +pc_system_flash_cleanup_unused(pcms);
> > >>
> > >> TIL pc_system_flash_cleanup_unused().
> > >>
> > >> What about restricting at the source?
> > >>
> > >
> > > And leave the devices in place? They are not relevant for Xen, so why not
> > > clean up?
> >
> > No, I meant to not create them in the first place, instead of
> > create+destroy.
> >
> > Anyway what you did works, so I don't have any problem.
>
> IIUC Jason originally tried restricting creation but encountered a problem
> because xen_enabled() would always return false at that point, because
> machine creation occurs before accelerators are initialized.
Correct. Quoting my previous email:
"""
Removing the call to pc_system_flash_create() from pc_machine_initfn()
lets QEMU startup and run a Xen HVM again. xen_enabled() doesn't work
there since accelerators have not been initialized yes, I guess?
"""
If you want to remove the creation in the first place, then I have two
questions. Why does pc_system_flash_create()/pc_pflash_create() get
called so early creating the pflash devices? Why aren't they just
created as needed in pc_system_flash_map()?
Regards,
Jason
> Paul
>
> >
> > Reviewed-by: Philippe Mathieu-Daudé
> >
> > >
> > > Paul
> > >
> > >> -- >8 --
> > >> --- a/hw/i386/pc.c
> > >> +++ b/hw/i386/pc.c
> > >> @@ -1004,24 +1004,26 @@ void pc_memory_init(PCMachineState *pcms,
> > >> >device_memory->mr);
> > >> }
> > >>
> > >> -/* Initialize PC system firmware */
> > >> -pc_system_firmware_init(pcms, rom_memory);
> > >> -
> > >> -option_rom_mr = g_malloc(sizeof(*option_rom_mr));
> > >> -memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
> > >> - _fatal);
> > >> -if (pcmc->pci_enabled) {
> > >> -memory_region_set_readonly(option_rom_mr, true);
> > >> -}
> > >> -memory_region_add_subregion_overlap(rom_memory,
> > >> -PC_ROM_MIN_VGA,
> > >> -option_rom_mr,
> > >> -1);
>
[PATCH v2 3/7] x86/mce: bring hypercall subop compat checking in sync again
Use a typedef in struct xen_mc also for the two subops "manually"
translated in the handler, just for consistency. No functional
change.
Signed-off-by: Jan Beulich
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -1307,16 +1307,16 @@ CHECK_mcinfo_common;
CHECK_FIELD_(struct, mc_fetch, flags);
CHECK_FIELD_(struct, mc_fetch, fetch_id);
-# define CHECK_compat_mc_fetch struct mc_fetch
+# define CHECK_mc_fetch struct mc_fetch
CHECK_FIELD_(struct, mc_physcpuinfo, ncpus);
-# define CHECK_compat_mc_physcpuinfo struct mc_physcpuinfo
+# define CHECK_mc_physcpuinfostruct mc_physcpuinfo
# define xen_ctl_bitmap xenctl_bitmap
CHECK_mc;
-# undef CHECK_compat_mc_fetch
-# undef CHECK_compat_mc_physcpuinfo
+# undef CHECK_mc_fetch
+# undef CHECK_mc_physcpuinfo
# undef xen_ctl_bitmap
# define xen_mc_info mc_info
--- a/xen/include/public/arch-x86/xen-mca.h
+++ b/xen/include/public/arch-x86/xen-mca.h
@@ -391,6 +391,7 @@ struct xen_mc_physcpuinfo {
/* OUT */
XEN_GUEST_HANDLE(xen_mc_logical_cpu_t) info;
};
+typedef struct xen_mc_physcpuinfo xen_mc_physcpuinfo_t;
#define XEN_MC_msrinject4
#define MC_MSRINJ_MAXMSRS 8
@@ -436,9 +437,9 @@ struct xen_mc {
uint32_t cmd;
uint32_t interface_version; /* XEN_MCA_INTERFACE_VERSION */
union {
-struct xen_mc_fetchmc_fetch;
+xen_mc_fetch_t mc_fetch;
xen_mc_notifydomain_t mc_notifydomain;
-struct xen_mc_physcpuinfo mc_physcpuinfo;
+xen_mc_physcpuinfo_t mc_physcpuinfo;
xen_mc_msrinject_t mc_msrinject;
xen_mc_mceinject_t mc_mceinject;
#if defined(__XEN__) || defined(__XEN_TOOLS__)
[PATCH v2 4/7] x86/dmop: add compat struct checking for XEN_DMOP_map_mem_type_to_ioreq_server
This was forgotten when the subop was added.
Also take the opportunity and move the dm_op_relocate_memory entry in
xlat.lst to its designated place.
No change in the resulting generated code.
Fixes: ca2b511d3ff4 ("x86/ioreq server: add DMOP to map guest ram with
p2m_ioreq_server to an ioreq server")
Signed-off-by: Jan Beulich
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -730,6 +730,7 @@ CHECK_dm_op_modified_memory;
CHECK_dm_op_set_mem_type;
CHECK_dm_op_inject_event;
CHECK_dm_op_inject_msi;
+CHECK_dm_op_map_mem_type_to_ioreq_server;
CHECK_dm_op_remote_shutdown;
CHECK_dm_op_relocate_memory;
CHECK_dm_op_pin_memory_cacheattr;
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -67,15 +67,16 @@
? grant_entry_v2 grant_table.h
? gnttab_swap_grant_ref grant_table.h
! dm_op_buf hvm/dm_op.h
-? dm_op_relocate_memory hvm/dm_op.h
? dm_op_create_ioreq_server hvm/dm_op.h
? dm_op_destroy_ioreq_server hvm/dm_op.h
? dm_op_get_ioreq_server_info hvm/dm_op.h
? dm_op_inject_event hvm/dm_op.h
? dm_op_inject_msihvm/dm_op.h
? dm_op_ioreq_server_rangehvm/dm_op.h
+? dm_op_map_mem_type_to_ioreq_server hvm/dm_op.h
? dm_op_modified_memory hvm/dm_op.h
? dm_op_pin_memory_cacheattr hvm/dm_op.h
+? dm_op_relocate_memory hvm/dm_op.h
? dm_op_remote_shutdown hvm/dm_op.h
? dm_op_set_ioreq_server_statehvm/dm_op.h
? dm_op_set_isa_irq_level hvm/dm_op.h
[PATCH v2 5/7] x86: generalize padding field handling
The original intention was to ignore padding fields, but the pattern matched only ones whose names started with an underscore. Also match fields whose names are in line with the C spec by not having a leading underscore. (Note that the leading ^ in the sed regexps was pointless and hence get dropped.) This requires adjusting some vNUMA macros, to avoid triggering "enumeration value ... not handled in switch" warnings, which - due to -Werror - would cause the build to fail. (I have to admit that I find these padding fields odd, when translation of the containing structure is needed anyway.) Signed-off-by: Jan Beulich --- While for translation macros skipping padding fields pretty surely is a reasonable thing to do, we may want to consider not ignoring them when generating checking macros. --- a/xen/common/compat/memory.c +++ b/xen/common/compat/memory.c @@ -354,10 +354,13 @@ int compat_memory_op(unsigned int cmd, X return -EFAULT; #define XLAT_vnuma_topology_info_HNDL_vdistance_h(_d_, _s_)\ +case XLAT_vnuma_topology_info_vdistance_pad:\ guest_from_compat_handle((_d_)->vdistance.h, (_s_)->vdistance.h) #define XLAT_vnuma_topology_info_HNDL_vcpu_to_vnode_h(_d_, _s_) \ +case XLAT_vnuma_topology_info_vcpu_to_vnode_pad:\ guest_from_compat_handle((_d_)->vcpu_to_vnode.h, (_s_)->vcpu_to_vnode.h) #define XLAT_vnuma_topology_info_HNDL_vmemrange_h(_d_, _s_)\ +case XLAT_vnuma_topology_info_vmemrange_pad:\ guest_from_compat_handle((_d_)->vmemrange.h, (_s_)->vmemrange.h) XLAT_vnuma_topology_info(nat.vnuma, ); --- a/xen/tools/get-fields.sh +++ b/xen/tools/get-fields.sh @@ -218,7 +218,7 @@ for line in sys.stdin.readlines(): fi ;; [\,\;]) - if [ $level = 2 -a -n "$(echo $id | $SED 's,^_pad[[:digit:]]*,,')" ] + if [ $level = 2 -a -n "$(echo $id | $SED 's,_\?pad[[:digit:]]*,,')" ] then if [ $kind = union ] then @@ -347,7 +347,7 @@ build_body () fi ;; [\,\;]) - if [ $level = 2 -a -n "$(echo $id | $SED 's,^_pad[[:digit:]]*,,')" ] + if [ $level = 2 -a -n "$(echo $id | $SED 's,_\?pad[[:digit:]]*,,')" ] then if [ -z "$array" -a -z "$array_type" ] then @@ -437,7 +437,7 @@ check_field () id=$token ;; [\,\;]) - if [ $level = 2 -a -n "$(echo $id | $SED 's,^_pad[[:digit:]]*,,')" ] + if [ $level = 2 -a -n "$(echo $id | $SED 's,_\?pad[[:digit:]]*,,')" ] then check_field $1 $2 $3.$id "$fields" test "$token" != ";" || fields= id= @@ -491,7 +491,7 @@ build_check () test $level != 2 -o $arrlvl != 1 || id=$token ;; [\,\;]) - if [ $level = 2 -a -n "$(echo $id | $SED 's,^_pad[[:digit:]]*,,')" ] + if [ $level = 2 -a -n "$(echo $id | $SED 's,_\?pad[[:digit:]]*,,')" ] then check_field $kind $1 $id "$fields" test "$token" != ";" || fields= id=
[PATCH v2 6/7] flask: drop dead compat translation code
Translation macros aren't needed at all (or else a devicetree_label entry would have been missing), and userlist has been removed quite some time ago. No functional change. Signed-off-by: Jan Beulich --- a/xen/include/xlat.lst +++ b/xen/include/xlat.lst @@ -148,14 +148,11 @@ ? xenoprof_init xenoprof.h ? xenoprof_passivexenoprof.h ? flask_accessxsm/flask_op.h -! flask_boolean xsm/flask_op.h ? flask_cache_stats xsm/flask_op.h ? flask_hash_statsxsm/flask_op.h -! flask_load xsm/flask_op.h ? flask_ocontext xsm/flask_op.h ? flask_peersid xsm/flask_op.h ? flask_relabel xsm/flask_op.h ? flask_setavc_threshold xsm/flask_op.h ? flask_setenforcexsm/flask_op.h -! flask_sid_context xsm/flask_op.h ? flask_transitionxsm/flask_op.h --- a/xen/xsm/flask/flask_op.c +++ b/xen/xsm/flask/flask_op.c @@ -790,8 +790,6 @@ CHECK_flask_transition; #define xen_flask_load compat_flask_load #define flask_security_load compat_security_load -#define xen_flask_userlist compat_flask_userlist - #define xen_flask_sid_context compat_flask_sid_context #define flask_security_context compat_security_context #define flask_security_sid compat_security_sid
[PATCH v2 7/7] x86: only generate compat headers actually needed
As was already the case for XSM/Flask, avoid generating compat headers
when they're not going to be needed. To address resulting build issues
- move compat/hvm/dm_op.h inclusion to the only source file needing it,
- add a little bit of #ifdef-ary.
Signed-off-by: Jan Beulich
---
Alternatively we could consistently drop conditionals (except for per-
arch cases perhaps).
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -717,6 +717,8 @@ static int dm_op(const struct dmop_args
return rc;
}
+#include
+
CHECK_dm_op_create_ioreq_server;
CHECK_dm_op_get_ioreq_server_info;
CHECK_dm_op_ioreq_server_range;
--- a/xen/common/compat/domain.c
+++ b/xen/common/compat/domain.c
@@ -11,7 +11,6 @@ EMIT_FILE;
#include
#include
#include
-#include
#define xen_vcpu_set_periodic_timer vcpu_set_periodic_timer
CHECK_vcpu_set_periodic_timer;
@@ -25,6 +24,10 @@ CHECK_SIZE_(struct, vcpu_info);
CHECK_vcpu_register_vcpu_info;
#undef xen_vcpu_register_vcpu_info
+#ifdef CONFIG_HVM
+
+#include
+
#define xen_vcpu_hvm_context vcpu_hvm_context
#define xen_vcpu_hvm_x86_32 vcpu_hvm_x86_32
#define xen_vcpu_hvm_x86_64 vcpu_hvm_x86_64
@@ -33,6 +36,8 @@ CHECK_vcpu_hvm_context;
#undef xen_vcpu_hvm_x86_32
#undef xen_vcpu_hvm_context
+#endif
+
int compat_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void)
arg)
{
struct domain *d = current->domain;
@@ -49,6 +54,7 @@ int compat_vcpu_op(int cmd, unsigned int
if ( v->vcpu_info == _vcpu_info )
return -EINVAL;
+#ifdef CONFIG_HVM
if ( is_hvm_vcpu(v) )
{
struct vcpu_hvm_context ctxt;
@@ -61,6 +67,7 @@ int compat_vcpu_op(int cmd, unsigned int
domain_unlock(d);
}
else
+#endif
{
struct compat_vcpu_guest_context *ctxt;
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -3,32 +3,34 @@ ifneq ($(CONFIG_COMPAT),)
compat-arch-$(CONFIG_X86) := x86_32
headers-y := \
-compat/argo.h \
-compat/callback.h \
+compat/arch-$(compat-arch-y).h \
compat/elfnote.h \
compat/event_channel.h \
compat/features.h \
-compat/grant_table.h \
-compat/hypfs.h \
-compat/kexec.h \
compat/memory.h \
compat/nmi.h \
compat/physdev.h \
compat/platform.h \
+compat/pmu.h \
compat/sched.h \
-compat/trace.h \
compat/vcpu.h \
compat/version.h \
compat/xen.h \
-compat/xenoprof.h
+compat/xlat.h
headers-$(CONFIG_X86) += compat/arch-x86/pmu.h
headers-$(CONFIG_X86) += compat/arch-x86/xen-mca.h
headers-$(CONFIG_X86) += compat/arch-x86/xen.h
headers-$(CONFIG_X86) += compat/arch-x86/xen-$(compat-arch-y).h
-headers-$(CONFIG_X86) += compat/hvm/dm_op.h
-headers-$(CONFIG_X86) += compat/hvm/hvm_op.h
-headers-$(CONFIG_X86) += compat/hvm/hvm_vcpu.h
-headers-y += compat/arch-$(compat-arch-y).h compat/pmu.h
compat/xlat.h
+headers-$(CONFIG_ARGO)+= compat/argo.h
+headers-$(CONFIG_PV) += compat/callback.h
+headers-$(CONFIG_GRANT_TABLE) += compat/grant_table.h
+headers-$(CONFIG_HVM) += compat/hvm/dm_op.h
+headers-$(CONFIG_HVM) += compat/hvm/hvm_op.h
+headers-$(CONFIG_HVM) += compat/hvm/hvm_vcpu.h
+headers-$(CONFIG_HYPFS) += compat/hypfs.h
+headers-$(CONFIG_KEXEC) += compat/kexec.h
+headers-$(CONFIG_TRACEBUFFER) += compat/trace.h
+headers-$(CONFIG_XENOPROF) += compat/xenoprof.h
headers-$(CONFIG_XSM_FLASK) += compat/xsm/flask_op.h
cppflags-y:= -include public/xen-compat.h
-DXEN_GENERATING_COMPAT_HEADERS
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -216,8 +216,6 @@ extern long compat_argo_op(
unsigned long arg4);
#endif
-#include
-
extern int
compat_dm_op(
domid_t domid,
Re: [PATCH for-4.14] x86/spec-ctrl: Protect against CALL/JMP straight-line speculation
On 01.07.2020 13:58, Andrew Cooper wrote: > Some x86 CPUs speculatively execute beyond indirect CALL/JMP instructions. > > With CONFIG_INDIRECT_THUNK / Retpolines, indirect CALL/JMP instructions are > converted to direct CALL/JMP's to __x86_indirect_thunk_REG(), leaving just a > handful of indirect JMPs implementing those stubs. > > There is no architectrual execution beyond an indirect JMP, so use INT3 as > recommended by vendors to halt speculative execution. This is shorter than > LFENCE (which would also work fine), but also shows up in logs if we do > unexpected execute them. > > Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich
[PATCH v2 1/7] x86: fix compat header generation
As was pointed out by 0e2e54966af5 ("mm: fix public declaration of
struct xen_mem_acquire_resource"), we're not currently handling structs
correctly that have uint64_aligned_t fields. #pragma pack(4) suppresses
the necessary alignment even if the type did properly survive (which
it also didn't) in the process of generating the headers. Overall,
with the above mentioned change applied, there's only a latent issue
here afaict, i.e. no other of our interface structs is currently
affected.
As a result it is clear that using #pragma pack(4) is not an option.
Drop all uses from compat header generation. Make sure
{,u}int64_aligned_t actually survives, such that explicitly aligned
fields will remain aligned. Arrange for {,u}int64_t to be transformed
into a type that's 64 bits wide and 4-byte aligned, by utilizing that
in typedef-s the "aligned" attribute can be used to reduce alignment.
Additionally, for the cases where native structures get re-used,
enforce suitable alignment via typedef-s (which allow alignment to be
reduced).
This use of typedef-s makes necessary changes to CHECK_*() macro
generation: Previously get-fields.sh relied on finding struct/union
keywords when other compound types were used. We now need to use the
typedef-s (guaranteeing suitable alignment) now, and hence the script
has to recognize those cases, too. (Unfortunately there are a few
special cases to be dealt with, but this is really not much different
from e.g. the pre-existing compat_domain_handle_t special case.)
This need to use typedef-s is certainly somewhat fragile going forward,
as in similar future cases it is imperative to also use typedef-s, or
else the CHECK_*() macros won't check what they're supposed to check. I
don't currently see any means to avoid this fragility, though.
There's one change to generated code according to my observations: In
arch_compat_vcpu_op() the runstate area "area" variable would previously
have been put in a just 4-byte aligned stack slot (despite being 8 bytes
in size), whereas now it gets put in an 8-byte aligned location.
There also results some curious inconsistency in struct xen_mc from
these changes - I intend to clean this up later on. Otherwise unrelated
code would also need adjustment right here.
Signed-off-by: Jan Beulich
---
v2: Different approach, addressing the latent alignment issues in v1.
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -34,15 +34,6 @@ headers-$(CONFIG_XSM_FLASK) += compat/xs
cppflags-y:= -include public/xen-compat.h
-DXEN_GENERATING_COMPAT_HEADERS
cppflags-$(CONFIG_X86)+= -m32
-# 8-byte types are 4-byte aligned on x86_32 ...
-ifeq ($(CONFIG_CC_IS_CLANG),y)
-prefix-$(CONFIG_X86) := \#pragma pack(push, 4)
-suffix-$(CONFIG_X86) := \#pragma pack(pop)
-else
-prefix-$(CONFIG_X86) := \#pragma pack(4)
-suffix-$(CONFIG_X86) := \#pragma pack()
-endif
-
endif
public-$(CONFIG_X86) := $(wildcard public/arch-x86/*.h public/arch-x86/*/*.h)
@@ -57,10 +48,8 @@ compat/%.h: compat/%.i Makefile $(BASEDI
echo "#define $$id" >>$@.new; \
echo "#include " >>$@.new; \
$(if $(filter-out compat/arch-%.h,$@),echo "#include <$(patsubst
compat/%,public/%,$@)>" >>$@.new;) \
- $(if $(prefix-y),echo "$(prefix-y)" >>$@.new;) \
grep -v '^# [0-9]' $< | \
$(PYTHON) $(BASEDIR)/tools/compat-build-header.py | uniq >>$@.new; \
- $(if $(suffix-y),echo "$(suffix-y)" >>$@.new;) \
echo "#endif /* $$id */" >>$@.new
mv -f $@.new $@
--- a/xen/include/public/arch-x86/pmu.h
+++ b/xen/include/public/arch-x86/pmu.h
@@ -105,7 +105,7 @@ struct xen_pmu_arch {
* Processor's registers at the time of interrupt.
* WO for hypervisor, RO for guests.
*/
-struct xen_pmu_regs regs;
+xen_pmu_regs_t regs;
/* Padding for adding new registers to xen_pmu_regs in the future */
#define XENPMU_REGS_PAD_SZ 64
uint8_t pad[XENPMU_REGS_PAD_SZ];
@@ -132,8 +132,8 @@ struct xen_pmu_arch {
* hypervisor into hardware during XENPMU_flush
*/
union {
-struct xen_pmu_amd_ctxt amd;
-struct xen_pmu_intel_ctxt intel;
+xen_pmu_amd_ctxt_t amd;
+xen_pmu_intel_ctxt_t intel;
/*
* Padding for contexts (fixed parts only, does not include MSR banks
--- a/xen/include/public/arch-x86/xen-mca.h
+++ b/xen/include/public/arch-x86/xen-mca.h
@@ -112,7 +112,7 @@ struct mcinfo_common {
uint16_t type; /* structure type */
uint16_t size; /* size of this struct in bytes */
};
-
+typedef struct mcinfo_common xen_mcinfo_common_t;
#define MC_FLAG_CORRECTABLE (1 << 0)
#define MC_FLAG_UNCORRECTABLE (1 << 1)
@@ -123,7 +123,7 @@ struct mcinfo_common {
#define MC_FLAG_MCE(1 << 6)
/* contains global x86 mc information */
struct mcinfo_global {
-struct mcinfo_common common;
+xen_mcinfo_common_t common;
/* running domain at the time in error (most likely
[PATCH v2 2/7] x86/mce: add compat struct checking for XEN_MC_inject_v2
84e364f2eda2 ("x86: add CMCI software injection interface") merely made
sure things would build, without any concern about things actually
working:
- despite the addition of xenctl_bitmap to xlat.lst, the resulting macro
wasn't invoked anywhere (which would have lead to recognizing that the
structure appeared to have no fully compatible layout, despite the use
of a 64-bit handle),
- the interface struct itself was neither added to xlat.lst (and the
resulting macro then invoked) nor was any manual checking of
individual fields added.
Adjust compat header generation logic to retain XEN_GUEST_HANDLE_64(),
which is intentionally layed out to be compatible between different size
guests. Invoke the missing checking (implicitly through CHECK_mc).
No change in the resulting generated code.
Fixes: 84e364f2eda2 ("x86: add CMCI software injection interface")
Signed-off-by: Jan Beulich
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -1312,10 +1312,12 @@ CHECK_FIELD_(struct, mc_fetch, fetch_id)
CHECK_FIELD_(struct, mc_physcpuinfo, ncpus);
# define CHECK_compat_mc_physcpuinfo struct mc_physcpuinfo
-#define CHECK_compat_mc_inject_v2 struct mc_inject_v2
+# define xen_ctl_bitmap xenctl_bitmap
+
CHECK_mc;
# undef CHECK_compat_mc_fetch
# undef CHECK_compat_mc_physcpuinfo
+# undef xen_ctl_bitmap
# define xen_mc_info mc_info
CHECK_mc_info;
--- a/xen/include/public/arch-x86/xen-mca.h
+++ b/xen/include/public/arch-x86/xen-mca.h
@@ -429,6 +429,7 @@ struct xen_mc_inject_v2 {
uint32_t flags;
xenctl_bitmap_t cpumap;
};
+typedef struct xen_mc_inject_v2 xen_mc_inject_v2_t;
#endif
struct xen_mc {
@@ -441,7 +442,7 @@ struct xen_mc {
xen_mc_msrinject_t mc_msrinject;
xen_mc_mceinject_t mc_mceinject;
#if defined(__XEN__) || defined(__XEN_TOOLS__)
-struct xen_mc_inject_v2mc_inject_v2;
+xen_mc_inject_v2_t mc_inject_v2;
#endif
} u;
};
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -25,6 +25,7 @@
? mcinfo_recovery arch-x86/xen-mca.h
! mc_fetcharch-x86/xen-mca.h
? mc_info arch-x86/xen-mca.h
+? mc_inject_v2arch-x86/xen-mca.h
? mc_mceinjectarch-x86/xen-mca.h
? mc_msrinjectarch-x86/xen-mca.h
? mc_notifydomain arch-x86/xen-mca.h
--- a/xen/tools/compat-build-header.py
+++ b/xen/tools/compat-build-header.py
@@ -19,6 +19,7 @@ pats = [
[ r"(^|[^\w])xen_?(\w*)_compat_t([^\w]|$$)", r"\1compat_\2_t\3" ],
[ r"(^|[^\w])XEN_?", r"\1COMPAT_" ],
[ r"(^|[^\w])Xen_?", r"\1Compat_" ],
+ [ r"(^|[^\w])COMPAT_HANDLE_64\(", r"\1XEN_GUEST_HANDLE_64(" ],
[ r"(^|[^\w])long([^\w]|$$)", r"\1int\2" ]
];
--- a/xen/tools/compat-build-source.py
+++ b/xen/tools/compat-build-source.py
@@ -10,7 +10,7 @@ pats = [
[ r"^\s*#\s*define\s+([A-Z_]*_GUEST_HANDLE)", r"#define HIDE_\1" ],
[ r"^\s*#\s*define\s+([a-z_]*_guest_handle)", r"#define hide_\1" ],
[ r"^\s*#\s*define\s+(u?int64)_aligned_t\s.*", r"typedef \1_T
__attribute__((aligned(4))) \1_compat_T;" ],
- [ r"XEN_GUEST_HANDLE(_[0-9A-Fa-f]+)?", r"COMPAT_HANDLE" ],
+ [ r"XEN_GUEST_HANDLE", r"COMPAT_HANDLE" ],
];
xlatf = open('xlat.lst', 'r')
Re: [PATCH 2/2] xen: cleanup unrealized flash devices
On 7/1/20 2:40 PM, Philippe Mathieu-Daudé wrote:
> On 7/1/20 2:25 PM, Jason Andryuk wrote:
>> On Wed, Jul 1, 2020 at 3:03 AM Paul Durrant wrote:
>>>
-Original Message-
From: Philippe Mathieu-Daudé
Sent: 30 June 2020 18:27
To: p...@xen.org; xen-devel@lists.xenproject.org; qemu-de...@nongnu.org
Cc: 'Eduardo Habkost' ; 'Michael S. Tsirkin'
; 'Paul Durrant'
; 'Jason Andryuk' ; 'Paolo
Bonzini' ;
'Richard Henderson'
Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
On 6/30/20 5:44 PM, Paul Durrant wrote:
>> -Original Message-
>> From: Philippe Mathieu-Daudé
>> Sent: 30 June 2020 16:26
>> To: Paul Durrant ; xen-devel@lists.xenproject.org;
>> qemu-de...@nongnu.org
>> Cc: Eduardo Habkost ; Michael S. Tsirkin
>> ; Paul Durrant
>> ; Jason Andryuk ; Paolo Bonzini
>> ;
>> Richard Henderson
>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>>
>> On 6/24/20 2:18 PM, Paul Durrant wrote:
>>> From: Paul Durrant
>>>
>>> The generic pc_machine_initfn() calls pc_system_flash_create() which
>>> creates
>>> 'system.flash0' and 'system.flash1' devices. These devices are then
>>> realized
>>> by pc_system_flash_map() which is called from pc_system_firmware_init()
>>> which
>>> itself is called via pc_memory_init(). The latter however is not called
>>> when
>>> xen_enable() is true and hence the following assertion fails:
>>>
>>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
>>> Assertion `dev->realized' failed
>>>
>>> These flash devices are unneeded when using Xen so this patch avoids the
>>> assertion by simply removing them using
>>> pc_system_flash_cleanup_unused().
>>>
>>> Reported-by: Jason Andryuk
>>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with
>>> -blockdev")
>>> Signed-off-by: Paul Durrant
>>> Tested-by: Jason Andryuk
>>> ---
>>> Cc: Paolo Bonzini
>>> Cc: Richard Henderson
>>> Cc: Eduardo Habkost
>>> Cc: "Michael S. Tsirkin"
>>> Cc: Marcel Apfelbaum
>>> ---
>>> hw/i386/pc_piix.c| 9 ++---
>>> hw/i386/pc_sysfw.c | 2 +-
>>> include/hw/i386/pc.h | 1 +
>>> 3 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>> index 1497d0e4ae..977d40afb8 100644
>>> --- a/hw/i386/pc_piix.c
>>> +++ b/hw/i386/pc_piix.c
>>> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
>>> if (!xen_enabled()) {
>>> pc_memory_init(pcms, system_memory,
>>> rom_memory, _memory);
>>> -} else if (machine->kernel_filename != NULL) {
>>> -/* For xen HVM direct kernel boot, load linux here */
>>> -xen_load_linux(pcms);
>>> +} else {
>>> +pc_system_flash_cleanup_unused(pcms);
>>
>> TIL pc_system_flash_cleanup_unused().
>>
>> What about restricting at the source?
>>
>
> And leave the devices in place? They are not relevant for Xen, so why not
> clean up?
No, I meant to not create them in the first place, instead of
create+destroy.
Anyway what you did works, so I don't have any problem.
>>>
>>> IIUC Jason originally tried restricting creation but encountered a problem
>>> because xen_enabled() would always return false at that point, because
>>> machine creation occurs before accelerators are initialized.
>>
>> Correct. Quoting my previous email:
>> """
>> Removing the call to pc_system_flash_create() from pc_machine_initfn()
>> lets QEMU startup and run a Xen HVM again. xen_enabled() doesn't work
>> there since accelerators have not been initialized yes, I guess?
>
> Ah, I missed that. You pointed at the bug here :)
>
> I think pc_system_flash_create() shouldn't be called in init() but
> realize().
Hmm this is a MachineClass, not qdev, so no realize().
In softmmu/vl.c we have:
4152 configure_accelerators(argv[0]);
4327 if (!xen_enabled()) { // so xen_enable() is working
4328 /* On 32-bit hosts, QEMU is limited by virtual address space */
4329 if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
4330 error_report("at most 2047 MB RAM can be simulated");
4331 exit(1);
4332 }
4333 }
4348 machine_run_board_init(current_machine);
which calls in hw/core/machine.c:
1089 void machine_run_board_init(MachineState *machine)
1090 {
1091 MachineClass *machine_class = MACHINE_GET_CLASS(machine);
1138 machine_class->init(machine);
1139 }
-> pc_machine_class_init()
This should come after:
-> pc_machine_initfn()
-> pc_system_flash_create(pcms)
>
>> """
>>
>> If you want to remove the creation in the first place, then I have two
>>
Re: [PATCH v4 02/10] x86/vmx: add IPT cpu feature
On Tue, Jun 30, 2020 at 02:33:45PM +0200, Michał Leszczyński wrote:
> From: Michal Leszczynski
>
> Check if Intel Processor Trace feature is supported by current
> processor. Define vmtrace_supported global variable.
>
> Signed-off-by: Michal Leszczynski
> ---
> xen/arch/x86/hvm/vmx/vmcs.c | 7 ++-
> xen/common/domain.c | 2 ++
> xen/include/asm-x86/cpufeature.h| 1 +
> xen/include/asm-x86/hvm/vmx/vmcs.h | 1 +
> xen/include/public/arch-x86/cpufeatureset.h | 1 +
> xen/include/xen/domain.h| 2 ++
> 6 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index ca94c2bedc..b73d824357 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -291,6 +291,12 @@ static int vmx_init_vmcs_config(void)
> _vmx_cpu_based_exec_control &=
> ~(CPU_BASED_CR8_LOAD_EXITING | CPU_BASED_CR8_STORE_EXITING);
>
> +rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap);
> +
> +/* Check whether IPT is supported in VMX operation. */
> +vmtrace_supported = cpu_has_ipt &&
> +(_vmx_misc_cap & VMX_MISC_PT_SUPPORTED);
This function gets called for every CPU that's brought up, so you need
to set it on the BSP, and then check that the APs also support the
feature or else fail to bring them up AFAICT. If not you could end up
with a non working system.
I agree it's very unlikely to boot on a system with such differences
between CPUs, but better be safe than sorry.
> +
> if ( _vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS
> )
> {
> min = 0;
> @@ -305,7 +311,6 @@ static int vmx_init_vmcs_config(void)
> SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS |
> SECONDARY_EXEC_XSAVES |
> SECONDARY_EXEC_TSC_SCALING);
> -rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap);
> if ( _vmx_misc_cap & VMX_MISC_VMWRITE_ALL )
> opt |= SECONDARY_EXEC_ENABLE_VMCS_SHADOWING;
> if ( opt_vpid_enabled )
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 7cc9526139..0a33e0dfd6 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -82,6 +82,8 @@ struct vcpu *idle_vcpu[NR_CPUS] __read_mostly;
>
> vcpu_info_t dummy_vcpu_info;
>
> +bool_t vmtrace_supported;
Plain bool, and I think it wants to be __read_mostly.
I'm also unsure whether this is the best place to put such variable,
since there are no users introduced on this patch it's hard to tell.
Thanks, Roger.
Re: [PATCH v4 07/10] x86/mm: add vmtrace_buf resource type
On Tue, Jun 30, 2020 at 02:33:50PM +0200, Michał Leszczyński wrote:
> From: Michal Leszczynski
>
> Allow to map processor trace buffer using
> acquire_resource().
>
> Signed-off-by: Michal Leszczynski
> ---
> xen/arch/x86/mm.c | 25 +
> xen/include/public/memory.h | 1 +
> 2 files changed, 26 insertions(+)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index e376fc7e8f..bb781bd90c 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4624,6 +4624,31 @@ int arch_acquire_resource(struct domain *d, unsigned
> int type,
> }
> break;
> }
> +
> +case XENMEM_resource_vmtrace_buf:
> +{
> +mfn_t mfn;
> +unsigned int i;
> +struct vcpu *v = domain_vcpu(d, id);
Missing blank newline between variable definitions and code.
> +rc = -EINVAL;
> +
> +if ( !v )
> +break;
> +
> +if ( !v->arch.vmtrace.pt_buf )
> +break;
> +
> +mfn = page_to_mfn(v->arch.vmtrace.pt_buf);
> +
> +if ( frame + nr_frames > (v->domain->vmtrace_pt_size >> PAGE_SHIFT) )
> +break;
You can place all the checks done above in a single if.
Thanks, Roger.
[xen-unstable test] 151491: tolerable FAIL - PUSHED
flight 151491 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/151491/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10 fail REGR. vs. 151461 Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 151461 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 151461 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 151461 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 151461 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 151461 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 151461 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 151461 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 151461 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 151461 test-amd64-i386-xl-pvshim12 guest-start fail 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-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-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-thunderx 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-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-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-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-amd64-amd64-libvirt-vhd 12 migrate-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-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 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 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail 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-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-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-libvirt-raw 12 migrate-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass version targeted for testing: xen 23ca7ec0ba620db52a646d80e22f9703a6589f66 baseline version: xen da53345dd5ff7d3a34e83587fd375c0b7722f46c Last test of basis 151461 2020-06-29 19:39:16 Z1 days Failing since151473 2020-06-30 08:47:12 Z1 days2 attempts Testing same since 151491 2020-06-30 22:36:56 Z0 days1 attempts People who touched revisions under test: Olaf Hering Roger Pau Monné jobs: build-amd64-xsm
Re: [PATCH v4 08/10] x86/domctl: add XEN_DOMCTL_vmtrace_op
On Tue, Jun 30, 2020 at 02:33:51PM +0200, Michał Leszczyński wrote:
> From: Michal Leszczynski
>
> Implement domctl to manage the runtime state of
> processor trace feature.
>
> Signed-off-by: Michal Leszczynski
> ---
> xen/arch/x86/domctl.c | 48 +
> xen/include/public/domctl.h | 26
> 2 files changed, 74 insertions(+)
>
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 6f2c69788d..a041b724d8 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -322,6 +322,48 @@ void arch_get_domain_info(const struct domain *d,
> info->arch_config.emulation_flags = d->arch.emulation_flags;
> }
>
> +static int do_vmtrace_op(struct domain *d, struct xen_domctl_vmtrace_op *op,
> + XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> +{
> +int rc;
> +struct vcpu *v;
> +
> +if ( !vmtrace_supported )
> +return -EOPNOTSUPP;
> +
> +if ( !is_hvm_domain(d) )
> +return -EOPNOTSUPP;
You can join both checks.
> +
> +if ( op->vcpu >= d->max_vcpus )
> +return -EINVAL;
> +
> +v = domain_vcpu(d, op->vcpu);
> +rc = 0;
No need to init rc to zero, after the switch below it will always be
initialized.
> +
> +switch ( op->cmd )
> +{
> +case XEN_DOMCTL_vmtrace_pt_enable:
> +case XEN_DOMCTL_vmtrace_pt_disable:
> +vcpu_pause(v);
> +spin_lock(>vmtrace_lock);
> +
> +rc = vmtrace_control_pt(v, op->cmd == XEN_DOMCTL_vmtrace_pt_enable);
> +
> +spin_unlock(>vmtrace_lock);
> +vcpu_unpause(v);
> +break;
> +
> +case XEN_DOMCTL_vmtrace_pt_get_offset:
> +rc = vmtrace_get_pt_offset(v, >offset);
Since you don't pause the vcpu here, I think you want to use atomic
operations to update v->arch.hvm.vmx.pt_state->output_mask.raw, or
else you could see inconsistent results if a vmexit is updating it in
parallel? (since you don't pause the target vcpu).
Thanks, Roger.
Re: [PATCH v3 7/7] tools/proctrace: add proctrace tool
Tamas K Lengyel writes ("Re: [PATCH v3 7/7] tools/proctrace: add proctrace
tool"):
> On Fri, Jun 26, 2020 at 7:26 AM Ian Jackson wrote:
> > Wei Liu writes ("Re: [PATCH v3 7/7] tools/proctrace: add proctrace tool"):
> > > On Mon, Jun 22, 2020 at 08:12:56PM +0200, Michał Leszczyński wrote:
> > > > Add an demonstration tool that uses xc_vmtrace_* calls in order
> > > > to manage external IPT monitoring for DomU.
> > ...
> > > > +if (rc) {
> > > > +fprintf(stderr, "Failed to call xc_vmtrace_pt_disable\n");
> > > > +return 1;
> > > > +}
> > > > +
> > >
> > > You should close fmem and xc in the exit path.
> >
> > Thanks for reviewing this. I agree with your comments. But I
> > disagree with this one.
> >
> > This is in main(). When the program exits, the xc handle and memory
> > mappings will go away as the kernel tears down the process.
> >
> > Leaving out this kind of cleanup in standalone command-line programs
> > is fine, I think. It can make the code simpler - and it avoids the
> > risk of doing it wrong, which can turn errors into crashes.
>
> Hi Ian,
> while I agree that this particular code would not be an issue,
> consider that these tools are often taken as sample codes to be reused
> in other contexts as well. As such, I think it should include the
> close bits as well and exhibit all the "best practices" we would like
> to see anyone else building tools for Xen.
Well, you're the author if this and I think you get to decide this
question (which is one of style). If that is your view then you Wei's
comment is certainly right, as far as it goes.
But looking at this program it seems to me that there is a great deal
of other stuff it allocates, one way or another, which it doesn't
free.
Is your intent that this program has this coding style ?
wombat = xc_allocate_wombat();
if (bad(wombat)) {
print_error("wombat");
exit(-1);
}
hippo = xc_allocate_hippo();
if (bad(hippo)) {
print_error("hippo");
xc_free_wombat(wombat);
exit(-1);
}
zebra = xc_allocate_zebra();
if (bad(zebra)) {
print_error("zebra");
xc_free_wombat(wombat);
xc_free_hippo(hippo);
exit(-1);
}
...
I think this is an unhelpful coding style. It inevitably leads to
leaks. IMO if you are going to try to tear down all things, you
should do it like this:
xc_wombat *wombat = NULL:
xc_hippo *hippo = NULL;
xc_hippo *zebra = NULL;
wombat = xc_allocate_wombat();
if (bad(wombat)) {
print_error("wombat");
goto exit_error;
}
hippo = xc_allocate_hippo();
if (bad(hipp)) {
print_error("hippo");
goto exit_error;
}
zebra = xc_allocate_zebra();
if (bad(hipp)) {
print_error("zebra");
goto exit_error;
}
...
exit_error:
if (some(wombat)) xc_free_wombat(wombat);
if (some(hippo)) xc_free_hippo(hippo);
if (some(zebra)) xc_free_zebra(zebra);
exit(-1);
or some similar approach that makes it easier to see that the code is
correct and leak-free.
But as I say I think as the author you get to decide.
Regards,
Ian.
[PATCH v2 3/4] x86/paravirt: cleanup paravirt macros
Some paravirt macros are no longer used, delete them. Signed-off-by: Juergen Gross --- arch/x86/include/asm/paravirt.h | 15 --- 1 file changed, 15 deletions(-) diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h index cfe9f6e472b5..cff2fbd1edd5 100644 --- a/arch/x86/include/asm/paravirt.h +++ b/arch/x86/include/asm/paravirt.h @@ -609,16 +609,9 @@ bool __raw_callee_save___native_vcpu_is_preempted(long cpu); #endif /* SMP && PARAVIRT_SPINLOCKS */ #ifdef CONFIG_X86_32 -#define PV_SAVE_REGS "pushl %ecx; pushl %edx;" -#define PV_RESTORE_REGS "popl %edx; popl %ecx;" - /* save and restore all caller-save registers, except return value */ #define PV_SAVE_ALL_CALLER_REGS"pushl %ecx;" #define PV_RESTORE_ALL_CALLER_REGS "popl %ecx;" - -#define PV_FLAGS_ARG "0" -#define PV_EXTRA_CLOBBERS -#define PV_VEXTRA_CLOBBERS #else /* save and restore all caller-save registers, except return value */ #define PV_SAVE_ALL_CALLER_REGS \ @@ -639,14 +632,6 @@ bool __raw_callee_save___native_vcpu_is_preempted(long cpu); "pop %rsi;" \ "pop %rdx;" \ "pop %rcx;" - -/* We save some registers, but all of them, that's too much. We clobber all - * caller saved registers but the argument parameter */ -#define PV_SAVE_REGS "pushq %%rdi;" -#define PV_RESTORE_REGS "popq %%rdi;" -#define PV_EXTRA_CLOBBERS EXTRA_CLOBBERS, "rcx" , "rdx", "rsi" -#define PV_VEXTRA_CLOBBERS EXTRA_CLOBBERS, "rdi", "rcx" , "rdx", "rsi" -#define PV_FLAGS_ARG "D" #endif /* -- 2.26.2
[PATCH v2 1/4] x86/xen: remove 32-bit Xen PV guest support
Xen is requiring 64-bit machines today and since Xen 4.14 it can be built without 32-bit PV guest support. There is no need to carry the burden of 32-bit PV guest support in the kernel any longer, as new guests can be either HVM or PVH, or they can use a 64 bit kernel. Remove the 32-bit Xen PV support from the kernel. Signed-off-by: Juergen Gross --- arch/x86/entry/entry_32.S | 109 +-- arch/x86/include/asm/proto.h | 2 +- arch/x86/include/asm/segment.h | 2 +- arch/x86/kernel/head_32.S | 31 --- arch/x86/xen/Kconfig | 3 +- arch/x86/xen/Makefile | 3 +- arch/x86/xen/apic.c| 17 -- arch/x86/xen/enlighten_pv.c| 48 + arch/x86/xen/mmu_pv.c | 340 - arch/x86/xen/p2m.c | 6 +- arch/x86/xen/setup.c | 35 +--- arch/x86/xen/smp_pv.c | 18 -- arch/x86/xen/xen-asm.S | 182 -- arch/x86/xen/xen-asm_32.S | 185 -- arch/x86/xen/xen-asm_64.S | 181 -- arch/x86/xen/xen-head.S| 6 - drivers/xen/Kconfig| 4 +- 17 files changed, 216 insertions(+), 956 deletions(-) delete mode 100644 arch/x86/xen/xen-asm_32.S delete mode 100644 arch/x86/xen/xen-asm_64.S diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S index 024d7d276cd4..70efe6d072f1 100644 --- a/arch/x86/entry/entry_32.S +++ b/arch/x86/entry/entry_32.S @@ -449,8 +449,6 @@ .macro SWITCH_TO_KERNEL_STACK - ALTERNATIVE "", "jmp .Lend_\@", X86_FEATURE_XENPV - BUG_IF_WRONG_CR3 SWITCH_TO_KERNEL_CR3 scratch_reg=%eax @@ -599,8 +597,6 @@ */ .macro SWITCH_TO_ENTRY_STACK - ALTERNATIVE "", "jmp .Lend_\@", X86_FEATURE_XENPV - /* Bytes to copy */ movl$PTREGS_SIZE, %ecx @@ -872,17 +868,6 @@ SYM_ENTRY(__begin_SYSENTER_singlestep_region, SYM_L_GLOBAL, SYM_A_NONE) * will ignore all of the single-step traps generated in this range. */ -#ifdef CONFIG_XEN_PV -/* - * Xen doesn't set %esp to be precisely what the normal SYSENTER - * entry point expects, so fix it up before using the normal path. - */ -SYM_CODE_START(xen_sysenter_target) - addl$5*4, %esp /* remove xen-provided frame */ - jmp .Lsysenter_past_esp -SYM_CODE_END(xen_sysenter_target) -#endif - /* * 32-bit SYSENTER entry. * @@ -966,9 +951,8 @@ SYM_FUNC_START(entry_SYSENTER_32) movl%esp, %eax calldo_fast_syscall_32 - /* XEN PV guests always use IRET path */ - ALTERNATIVE "testl %eax, %eax; jz .Lsyscall_32_done", \ - "jmp .Lsyscall_32_done", X86_FEATURE_XENPV + testl %eax, %eax + jz .Lsyscall_32_done STACKLEAK_ERASE @@ -1166,95 +1150,6 @@ SYM_FUNC_END(entry_INT80_32) #endif .endm -#ifdef CONFIG_PARAVIRT -SYM_CODE_START(native_iret) - iret - _ASM_EXTABLE(native_iret, asm_iret_error) -SYM_CODE_END(native_iret) -#endif - -#ifdef CONFIG_XEN_PV -/* - * See comment in entry_64.S for further explanation - * - * Note: This is not an actual IDT entry point. It's a XEN specific entry - * point and therefore named to match the 64-bit trampoline counterpart. - */ -SYM_FUNC_START(xen_asm_exc_xen_hypervisor_callback) - /* -* Check to see if we got the event in the critical -* region in xen_iret_direct, after we've reenabled -* events and checked for pending events. This simulates -* iret instruction's behaviour where it delivers a -* pending interrupt when enabling interrupts: -*/ - cmpl$xen_iret_start_crit, (%esp) - jb 1f - cmpl$xen_iret_end_crit, (%esp) - jae 1f - callxen_iret_crit_fixup -1: - pushl $-1 /* orig_ax = -1 => not a system call */ - SAVE_ALL - ENCODE_FRAME_POINTER - - mov %esp, %eax - callxen_pv_evtchn_do_upcall - jmp handle_exception_return -SYM_FUNC_END(xen_asm_exc_xen_hypervisor_callback) - -/* - * Hypervisor uses this for application faults while it executes. - * We get here for two reasons: - * 1. Fault while reloading DS, ES, FS or GS - * 2. Fault while executing IRET - * Category 1 we fix up by reattempting the load, and zeroing the segment - * register if the load fails. - * Category 2 we fix up by jumping to do_iret_error. We cannot use the - * normal Linux return path in this case because if we use the IRET hypercall - * to pop the stack frame we end up in an infinite loop of failsafe callbacks. - * We distinguish between categories by maintaining a status value in EAX. - */ -SYM_FUNC_START(xen_failsafe_callback) - pushl %eax - movl$1, %eax -1: mov 4(%esp), %ds -2: mov 8(%esp), %es -3: mov 12(%esp), %fs -4: mov 16(%esp), %gs - /* EAX == 0 => Category 1 (Bad segment) - EAX != 0 => Category 2 (Bad IRET) */
[PATCH v2 4/4] x86/paravirt: use CONFIG_PARAVIRT_XXL instead of CONFIG_PARAVIRT
There are some code parts using CONFIG_PARAVIRT for Xen pvops related
issues instead of the more stringent CONFIG_PARAVIRT_XXL.
Signed-off-by: Juergen Gross
---
arch/x86/entry/entry_64.S| 4 ++--
arch/x86/include/asm/fixmap.h| 2 +-
arch/x86/include/asm/required-features.h | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index d2a00c97e53f..cb715d2b357d 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -45,13 +45,13 @@
.code64
.section .entry.text, "ax"
-#ifdef CONFIG_PARAVIRT
+#ifdef CONFIG_PARAVIRT_XXL
SYM_CODE_START(native_usergs_sysret64)
UNWIND_HINT_EMPTY
swapgs
sysretq
SYM_CODE_END(native_usergs_sysret64)
-#endif /* CONFIG_PARAVIRT */
+#endif /* CONFIG_PARAVIRT_XXL */
/*
* 64-bit SYSCALL instruction entry. Up to 6 arguments in registers.
diff --git a/arch/x86/include/asm/fixmap.h b/arch/x86/include/asm/fixmap.h
index b9527a54db99..f1422ada4ffe 100644
--- a/arch/x86/include/asm/fixmap.h
+++ b/arch/x86/include/asm/fixmap.h
@@ -99,7 +99,7 @@ enum fixed_addresses {
FIX_PCIE_MCFG,
#endif
#endif
-#ifdef CONFIG_PARAVIRT
+#ifdef CONFIG_PARAVIRT_XXL
FIX_PARAVIRT_BOOTMAP,
#endif
#ifdef CONFIG_X86_INTEL_MID
diff --git a/arch/x86/include/asm/required-features.h
b/arch/x86/include/asm/required-features.h
index 6847d85400a8..3ff0d48469f2 100644
--- a/arch/x86/include/asm/required-features.h
+++ b/arch/x86/include/asm/required-features.h
@@ -54,7 +54,7 @@
#endif
#ifdef CONFIG_X86_64
-#ifdef CONFIG_PARAVIRT
+#ifdef CONFIG_PARAVIRT_XXL
/* Paravirtualized systems may not have PSE or PGE available */
#define NEED_PSE 0
#define NEED_PGE 0
--
2.26.2
[PATCH v2 0/4] Remove 32-bit Xen PV guest support
The long term plan has been to replace Xen PV guests by PVH. The first victim of that plan are now 32-bit PV guests, as those are used only rather seldom these days. Xen on x86 requires 64-bit support and with Grub2 now supporting PVH officially since version 2.04 there is no need to keep 32-bit PV guest support alive in the Linux kernel. Additionally Meltdown mitigation is not available in the kernel running as 32-bit PV guest, so dropping this mode makes sense from security point of view, too. Changes in V2: - rebase to 5.8 kernel - addressed comments to V1 - new patches 3 and 4 Juergen Gross (4): x86/xen: remove 32-bit Xen PV guest support x86/paravirt: remove 32-bit support from PARAVIRT_XXL x86/paravirt: cleanup paravirt macros x86/paravirt: use CONFIG_PARAVIRT_XXL instead of CONFIG_PARAVIRT arch/x86/entry/entry_32.S | 109 +-- arch/x86/entry/entry_64.S | 4 +- arch/x86/entry/vdso/vdso32/vclock_gettime.c | 1 + arch/x86/include/asm/fixmap.h | 2 +- arch/x86/include/asm/paravirt.h | 107 +- arch/x86/include/asm/paravirt_types.h | 21 -- arch/x86/include/asm/pgtable-3level_types.h | 5 - arch/x86/include/asm/proto.h| 2 +- arch/x86/include/asm/required-features.h| 2 +- arch/x86/include/asm/segment.h | 6 +- arch/x86/kernel/cpu/common.c| 8 - arch/x86/kernel/head_32.S | 31 -- arch/x86/kernel/kprobes/core.c | 1 - arch/x86/kernel/kprobes/opt.c | 1 - arch/x86/kernel/paravirt.c | 18 -- arch/x86/kernel/paravirt_patch.c| 17 - arch/x86/xen/Kconfig| 3 +- arch/x86/xen/Makefile | 3 +- arch/x86/xen/apic.c | 17 - arch/x86/xen/enlighten_pv.c | 52 +-- arch/x86/xen/mmu_pv.c | 340 +++- arch/x86/xen/p2m.c | 6 +- arch/x86/xen/setup.c| 35 +- arch/x86/xen/smp_pv.c | 18 -- arch/x86/xen/xen-asm.S | 182 ++- arch/x86/xen/xen-asm_32.S | 185 --- arch/x86/xen/xen-asm_64.S | 181 --- arch/x86/xen/xen-head.S | 6 - drivers/xen/Kconfig | 4 +- 29 files changed, 232 insertions(+), 1135 deletions(-) delete mode 100644 arch/x86/xen/xen-asm_32.S delete mode 100644 arch/x86/xen/xen-asm_64.S -- 2.26.2
[PATCH v2 2/2] xen/xenbus: let xenbus_map_ring_valloc() return errno values only
Today xenbus_map_ring_valloc() can return either a negative errno
value (-ENOMEM or -EINVAL) or a grant status value. This is a mess as
e.g -ENOMEM and GNTST_eagain have the same numeric value.
Fix that by turning all grant mapping errors into -ENOENT. This is
no problem as all callers of xenbus_map_ring_valloc() only use the
return value to print an error message, and in case of mapping errors
the grant status value has already been printed by __xenbus_map_ring()
before.
Signed-off-by: Juergen Gross
Reviewed-by: Boris Ostrovsky
---
drivers/xen/xenbus/xenbus_client.c | 22 ++
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/drivers/xen/xenbus/xenbus_client.c
b/drivers/xen/xenbus/xenbus_client.c
index 9f8372079ecf..4f168b46fbca 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -456,8 +456,7 @@ EXPORT_SYMBOL_GPL(xenbus_free_evtchn);
* Map @nr_grefs pages of memory into this domain from another
* domain's grant table. xenbus_map_ring_valloc allocates @nr_grefs
* pages of virtual address space, maps the pages to that address, and
- * sets *vaddr to that address. Returns 0 on success, and GNTST_*
- * (see xen/include/interface/grant_table.h) or -ENOMEM / -EINVAL on
+ * sets *vaddr to that address. Returns 0 on success, and -errno on
* error. If an error is returned, device will switch to
* XenbusStateClosing and the error message will be saved in XenStore.
*/
@@ -477,18 +476,11 @@ int xenbus_map_ring_valloc(struct xenbus_device *dev,
grant_ref_t *gnt_refs,
return -ENOMEM;
info->node = kzalloc(sizeof(*info->node), GFP_KERNEL);
- if (!info->node) {
+ if (!info->node)
err = -ENOMEM;
- goto out;
- }
-
- err = ring_ops->map(dev, info, gnt_refs, nr_grefs, vaddr);
-
- /* Some hypervisors are buggy and can return 1. */
- if (err > 0)
- err = GNTST_general_error;
+ else
+ err = ring_ops->map(dev, info, gnt_refs, nr_grefs, vaddr);
- out:
kfree(info->node);
kfree(info);
return err;
@@ -507,7 +499,6 @@ static int __xenbus_map_ring(struct xenbus_device *dev,
bool *leaked)
{
int i, j;
- int err = GNTST_okay;
if (nr_grefs > XENBUS_MAX_RING_GRANTS)
return -EINVAL;
@@ -522,7 +513,6 @@ static int __xenbus_map_ring(struct xenbus_device *dev,
for (i = 0; i < nr_grefs; i++) {
if (info->map[i].status != GNTST_okay) {
- err = info->map[i].status;
xenbus_dev_fatal(dev, info->map[i].status,
"mapping in shared page %d from domain
%d",
gnt_refs[i], dev->otherend_id);
@@ -531,7 +521,7 @@ static int __xenbus_map_ring(struct xenbus_device *dev,
handles[i] = info->map[i].handle;
}
- return GNTST_okay;
+ return 0;
fail:
for (i = j = 0; i < nr_grefs; i++) {
@@ -554,7 +544,7 @@ static int __xenbus_map_ring(struct xenbus_device *dev,
}
}
- return err;
+ return -ENOENT;
}
/**
--
2.26.2
[PATCH v2 0/2] xen/xenbus: some cleanups
Avoid allocating large amount of data on the stack in xenbus_map_ring_valloc() and some related return value cleanups. Juergen Gross (2): xen/xenbus: avoid large structs and arrays on the stack xen/xenbus: let xenbus_map_ring_valloc() return errno values only drivers/xen/xenbus/xenbus_client.c | 167 ++--- 1 file changed, 81 insertions(+), 86 deletions(-) -- 2.26.2
Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address
On 29/06/2020 08:42, Paul Durrant wrote:
-Original Message-
From: Julien Grall
Sent: 26 June 2020 18:54
To: Stefano Stabellini ; p...@xen.org
Cc: Volodymyr Babchuk ;
xen-devel@lists.xenproject.org; op-
t...@lists.trustedfirmware.org
Subject: Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address
(using paul xen.org's email)
Thanks. Avoids annoying warning banners :-)
Hi,
Apologies for the late answer.
On 23/06/2020 22:09, Stefano Stabellini wrote:
On Tue, 23 Jun 2020, Julien Grall wrote:
On 23/06/2020 03:49, Volodymyr Babchuk wrote:
Hi Stefano,
Stefano Stabellini writes:
On Fri, 19 Jun 2020, Volodymyr Babchuk wrote:
Trusted Applications use popular approach to determine required size
of buffer: client provides a memory reference with the NULL pointer to
a buffer. This is so called "Null memory reference". TA updates the
reference with the required size and returns it back to the
client. Then client allocates buffer of needed size and repeats the
operation.
This behavior is described in TEE Client API Specification, paragraph
3.2.5. Memory References.
OP-TEE represents this null memory reference as a TMEM parameter with
buf_ptr = 0x0. This is the only case when we should allow TMEM
buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.
This could lead to a potential issue, because IPA 0x0 is a valid
address, but OP-TEE will treat it as a special case. So, care should
be taken when construction OP-TEE enabled guest to make sure that such
guest have no memory at IPA 0x0 and none of its memory is mapped at PA
0x0.
Signed-off-by: Volodymyr Babchuk
---
Changes from v1:
- Added comment with TODO about possible PA/IPA 0x0 issue
- The same is described in the commit message
- Added check in translate_noncontig() for the NULL ptr buffer
---
xen/arch/arm/tee/optee.c | 27 ---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index 6963238056..70bfef7e5f 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -215,6 +215,15 @@ static bool optee_probe(void)
return true;
}
+/*
+ * TODO: There is a potential issue with guests that either have RAM
+ * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
^ their
+ * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
+ * not be able to map buffer with such pointer to TA address space, or
+ * use such buffer for communication with the guest. We either need to
+ * check that guest have no such mappings or ensure that OP-TEE
+ * enabled guest will not be created with such mappings.
+ */
static int optee_domain_init(struct domain *d)
{
struct arm_smccc_res resp;
@@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain
*ctx,
uint64_t next_page_data;
} *guest_data, *xen_data;
+/*
+ * Special case: buffer with buf_ptr == 0x0 is considered as NULL
+ * pointer by OP-TEE. No translation is needed. This can lead to
+ * an issue as IPA 0x0 is a valid address for Xen. See the comment
+ * near optee_domain_init()
+ */
+if ( !param->u.tmem.buf_ptr )
+return 0;
Given that today it is not possible for this to happen, it could even be
an ASSERT. But I think I would just return an error, maybe -EINVAL?
Hmm, looks like my comment is somewhat misleading :(
How about the following comment:
We don't want to translate NULL (0) as it can be used by the guest to fetch
the size of the buffer to allocate. This behavior depends on TA, but there is
a guarantee that OP-TEE will not try to map it (see more details on top of
optee_domain_init()).
What I mean, is that param->u.tmem.buf_ptr == 0 is the normal situation.
This is the special case, when OP-TEE treats this buffer as a NULL. So
we are doing nothing there. Thus, "return 0".
But, as Julien pointed out, we can have machine where 0x0 is the valid
memory address and there is a chance, that some guest will use it as a
pointer to buffer.
Aside from this, and the small grammar issue, everything else looks fine
to me.
Let's wait for Julien's reply, but if this is the only thing I could fix
on commit.
I agree with Volodymyr, this is the normal case here. There are more work to
prevent MFN 0 to be mapped in the guest but this shouldn't be an issue today.
Let's put the MFN 0 issue aside for a moment.
From the commit message I thought that if the guest wanted to pass a
NULL buffer ("Null memory reference") then it would also *not* set
OPTEE_MSG_ATTR_NONCONTIG, which would be handled by the "else" statement
also modified by this patch. Thus, I thought that reaching
translate_noncontig with buf_ptr == NULL would always be an error.
But re-reading the commit message and from both your answers it is not
the case: a "Null
Re: [PATCH v4 05/10] common/domain: allocate vmtrace_pt_buffer
On Tue, Jun 30, 2020 at 02:33:48PM +0200, Michał Leszczyński wrote:
> From: Michal Leszczynski
>
> Allocate processor trace buffer for each vCPU when the domain
> is created, deallocate trace buffers on domain destruction.
>
> Signed-off-by: Michal Leszczynski
> ---
> xen/arch/x86/domain.c| 11 +++
> xen/common/domain.c | 32
> xen/include/asm-x86/domain.h | 4
> xen/include/xen/sched.h | 4
> 4 files changed, 51 insertions(+)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index fee6c3931a..0d79fd390c 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -2199,6 +2199,17 @@ int domain_relinquish_resources(struct domain *d)
> altp2m_vcpu_disable_ve(v);
> }
>
> +for_each_vcpu ( d, v )
> +{
> +if ( !v->arch.vmtrace.pt_buf )
> +continue;
> +
> +vmtrace_destroy_pt(v);
> +
> +free_domheap_pages(v->arch.vmtrace.pt_buf,
> +get_order_from_bytes(v->domain->vmtrace_pt_size));
> +}
> +
> if ( is_pv_domain(d) )
> {
> for_each_vcpu ( d, v )
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 27dcfbac8c..8513659ef8 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -137,6 +137,31 @@ static void vcpu_destroy(struct vcpu *v)
> free_vcpu_struct(v);
> }
>
> +static int vmtrace_alloc_buffers(struct vcpu *v)
> +{
> +struct page_info *pg;
> +uint64_t size = v->domain->vmtrace_pt_size;
IMO you would be better by just storing an order here (like it's
passed from the toolstack), that would avoid the checks and conversion
to an order. Also vmtrace_pt_size could be of type unsigned int
instead of uint64_t.
> +
> +if ( size < PAGE_SIZE || size > GB(4) || (size & (size - 1)) )
> +{
> +/*
> + * We don't accept trace buffer size smaller than single page
> + * and the upper bound is defined as 4GB in the specification.
> + * The buffer size must be also a power of 2.
> + */
> +return -EINVAL;
> +}
> +
> +pg = alloc_domheap_pages(v->domain, get_order_from_bytes(size),
> + MEMF_no_refcount);
> +
> +if ( !pg )
> +return -ENOMEM;
> +
> +v->arch.vmtrace.pt_buf = pg;
You can assign to pt_buf directly IMO, no need for the pg local
variable.
> +return 0;
> +}
> +
> struct vcpu *vcpu_create(struct domain *d, unsigned int vcpu_id)
> {
> struct vcpu *v;
> @@ -162,6 +187,9 @@ struct vcpu *vcpu_create(struct domain *d, unsigned int
> vcpu_id)
> v->vcpu_id = vcpu_id;
> v->dirty_cpu = VCPU_CPU_CLEAN;
>
> +if ( d->vmtrace_pt_size && vmtrace_alloc_buffers(v) != 0 )
> +return NULL;
You are leaking the allocated v here, see other error paths below in
the function.
> +
> spin_lock_init(>virq_lock);
>
> tasklet_init(>continue_hypercall_tasklet, NULL, NULL);
> @@ -188,6 +216,9 @@ struct vcpu *vcpu_create(struct domain *d, unsigned int
> vcpu_id)
> if ( arch_vcpu_create(v) != 0 )
> goto fail_sched;
>
> +if ( d->vmtrace_pt_size && vmtrace_init_pt(v) != 0 )
> +goto fail_sched;
> +
> d->vcpu[vcpu_id] = v;
> if ( vcpu_id != 0 )
> {
> @@ -422,6 +453,7 @@ struct domain *domain_create(domid_t domid,
> d->shutdown_code = SHUTDOWN_CODE_INVALID;
>
> spin_lock_init(>pbuf_lock);
> +spin_lock_init(>vmtrace_lock);
>
> rwlock_init(>vnuma_rwlock);
>
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 6fd94c2e14..b01c107f5c 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -627,6 +627,10 @@ struct arch_vcpu
> struct {
> bool next_interrupt_enabled;
> } monitor;
> +
> +struct {
> +struct page_info *pt_buf;
> +} vmtrace;
> };
>
> struct guest_memory_policy
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index ac53519d7f..48f0a61bbd 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -457,6 +457,10 @@ struct domain
> unsignedpbuf_idx;
> spinlock_t pbuf_lock;
>
> +/* Used by vmtrace features */
> +spinlock_t vmtrace_lock;
Does this need to be per domain or rather per-vcpu? It's hard to tell
because there's no user of it in the patch.
Thanks, Roger.
[PATCH for-4.14] x86/spec-ctrl: Protect against CALL/JMP straight-line speculation
Some x86 CPUs speculatively execute beyond indirect CALL/JMP instructions. With CONFIG_INDIRECT_THUNK / Retpolines, indirect CALL/JMP instructions are converted to direct CALL/JMP's to __x86_indirect_thunk_REG(), leaving just a handful of indirect JMPs implementing those stubs. There is no architectrual execution beyond an indirect JMP, so use INT3 as recommended by vendors to halt speculative execution. This is shorter than LFENCE (which would also work fine), but also shows up in logs if we do unexpected execute them. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné CC: Paul Durrant This wants backporting to all release, possibly even into the security trees, and should therefore be considered for 4.14 at this point. --- xen/arch/x86/indirect-thunk.S | 2 ++ 1 file changed, 2 insertions(+) diff --git a/xen/arch/x86/indirect-thunk.S b/xen/arch/x86/indirect-thunk.S index 3c17f75c23..7392aee127 100644 --- a/xen/arch/x86/indirect-thunk.S +++ b/xen/arch/x86/indirect-thunk.S @@ -24,10 +24,12 @@ .macro IND_THUNK_LFENCE reg:req lfence jmp *%\reg +int3 /* Halt straight-line speculation */ .endm .macro IND_THUNK_JMP reg:req jmp *%\reg +int3 /* Halt straight-line speculation */ .endm /* -- 2.11.0
Re: [PATCH v2] xen/displif: Protocol version 2
On 7/1/20 1:46 PM, Jürgen Groß wrote: > On 01.07.20 09:19, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko >> >> 1. Add protocol version as an integer >> >> Version string, which is in fact an integer, is hard to handle in the >> code that supports different protocol versions. To simplify that >> also add the version as an integer. >> >> 2. Pass buffer offset with XENDISPL_OP_DBUF_CREATE >> >> There are cases when display data buffer is created with non-zero >> offset to the data start. Handle such cases and provide that offset >> while creating a display buffer. >> >> 3. Add XENDISPL_OP_GET_EDID command >> >> Add an optional request for reading Extended Display Identification >> Data (EDID) structure which allows better configuration of the >> display connectors over the configuration set in XenStore. >> With this change connectors may have multiple resolutions defined >> with respect to detailed timing definitions and additional properties >> normally provided by displays. >> >> If this request is not supported by the backend then visible area >> is defined by the relevant XenStore's "resolution" property. >> >> If backend provides extended display identification data (EDID) with >> XENDISPL_OP_GET_EDID request then EDID values must take precedence >> over the resolutions defined in XenStore. >> >> 4. Bump protocol version to 2. >> >> Signed-off-by: Oleksandr Andrushchenko > > Reviewed-by: Juergen Gross Thank you, do you want me to prepare the same for the kernel so you have it at hand when the time comes? > > > Juergen
Re: [PATCH 2/2] xen: cleanup unrealized flash devices
On 7/1/20 2:25 PM, Jason Andryuk wrote:
> On Wed, Jul 1, 2020 at 3:03 AM Paul Durrant wrote:
>>
>>> -Original Message-
>>> From: Philippe Mathieu-Daudé
>>> Sent: 30 June 2020 18:27
>>> To: p...@xen.org; xen-devel@lists.xenproject.org; qemu-de...@nongnu.org
>>> Cc: 'Eduardo Habkost' ; 'Michael S. Tsirkin'
>>> ; 'Paul Durrant'
>>> ; 'Jason Andryuk' ; 'Paolo
>>> Bonzini' ;
>>> 'Richard Henderson'
>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>>>
>>> On 6/30/20 5:44 PM, Paul Durrant wrote:
> -Original Message-
> From: Philippe Mathieu-Daudé
> Sent: 30 June 2020 16:26
> To: Paul Durrant ; xen-devel@lists.xenproject.org;
> qemu-de...@nongnu.org
> Cc: Eduardo Habkost ; Michael S. Tsirkin
> ; Paul Durrant
> ; Jason Andryuk ; Paolo Bonzini
> ;
> Richard Henderson
> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>
> On 6/24/20 2:18 PM, Paul Durrant wrote:
>> From: Paul Durrant
>>
>> The generic pc_machine_initfn() calls pc_system_flash_create() which
>> creates
>> 'system.flash0' and 'system.flash1' devices. These devices are then
>> realized
>> by pc_system_flash_map() which is called from pc_system_firmware_init()
>> which
>> itself is called via pc_memory_init(). The latter however is not called
>> when
>> xen_enable() is true and hence the following assertion fails:
>>
>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
>> Assertion `dev->realized' failed
>>
>> These flash devices are unneeded when using Xen so this patch avoids the
>> assertion by simply removing them using pc_system_flash_cleanup_unused().
>>
>> Reported-by: Jason Andryuk
>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
>> Signed-off-by: Paul Durrant
>> Tested-by: Jason Andryuk
>> ---
>> Cc: Paolo Bonzini
>> Cc: Richard Henderson
>> Cc: Eduardo Habkost
>> Cc: "Michael S. Tsirkin"
>> Cc: Marcel Apfelbaum
>> ---
>> hw/i386/pc_piix.c| 9 ++---
>> hw/i386/pc_sysfw.c | 2 +-
>> include/hw/i386/pc.h | 1 +
>> 3 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 1497d0e4ae..977d40afb8 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
>> if (!xen_enabled()) {
>> pc_memory_init(pcms, system_memory,
>> rom_memory, _memory);
>> -} else if (machine->kernel_filename != NULL) {
>> -/* For xen HVM direct kernel boot, load linux here */
>> -xen_load_linux(pcms);
>> +} else {
>> +pc_system_flash_cleanup_unused(pcms);
>
> TIL pc_system_flash_cleanup_unused().
>
> What about restricting at the source?
>
And leave the devices in place? They are not relevant for Xen, so why not
clean up?
>>>
>>> No, I meant to not create them in the first place, instead of
>>> create+destroy.
>>>
>>> Anyway what you did works, so I don't have any problem.
>>
>> IIUC Jason originally tried restricting creation but encountered a problem
>> because xen_enabled() would always return false at that point, because
>> machine creation occurs before accelerators are initialized.
>
> Correct. Quoting my previous email:
> """
> Removing the call to pc_system_flash_create() from pc_machine_initfn()
> lets QEMU startup and run a Xen HVM again. xen_enabled() doesn't work
> there since accelerators have not been initialized yes, I guess?
Ah, I missed that. You pointed at the bug here :)
I think pc_system_flash_create() shouldn't be called in init() but
realize().
> """
>
> If you want to remove the creation in the first place, then I have two
> questions. Why does pc_system_flash_create()/pc_pflash_create() get
> called so early creating the pflash devices? Why aren't they just
> created as needed in pc_system_flash_map()?
>
> Regards,
> Jason
>
>> Paul
>>
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé
>>>
Paul
> -- >8 --
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1004,24 +1004,26 @@ void pc_memory_init(PCMachineState *pcms,
> >device_memory->mr);
> }
>
> -/* Initialize PC system firmware */
> -pc_system_firmware_init(pcms, rom_memory);
> -
> -option_rom_mr = g_malloc(sizeof(*option_rom_mr));
> -memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
> - _fatal);
> -if (pcmc->pci_enabled) {
> -memory_region_set_readonly(option_rom_mr, true);
> -}
> -memory_region_add_subregion_overlap(rom_memory,
> -PC_ROM_MIN_VGA,
> -
Re: [PATCH v4 03/10] tools/libxl: add vmtrace_pt_size parameter
On Tue, Jun 30, 2020 at 02:33:46PM +0200, Michał Leszczyński wrote:
> From: Michal Leszczynski
>
> Allow to specify the size of per-vCPU trace buffer upon
> domain creation. This is zero by default (meaning: not enabled).
>
> Signed-off-by: Michal Leszczynski
> ---
> docs/man/xl.cfg.5.pod.in | 10 ++
> tools/golang/xenlight/helpers.gen.go | 2 ++
> tools/golang/xenlight/types.gen.go | 1 +
> tools/libxl/libxl.h | 8
> tools/libxl/libxl_create.c | 1 +
> tools/libxl/libxl_types.idl | 2 ++
> tools/xl/xl_parse.c | 20
> xen/common/domain.c | 12
> xen/include/public/domctl.h | 1 +
> 9 files changed, 57 insertions(+)
>
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 0532739c1f..78f434b722 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -278,6 +278,16 @@ memory=8096 will report significantly less memory
> available for use
> than a system with maxmem=8096 memory=8096 due to the memory overhead
> of having to track the unused pages.
>
> +=item B
> +
> +Specifies the size of processor trace buffer that would be allocated
> +for each vCPU belonging to this domain. Disabled (i.e. B
> +by default. This must be set to non-zero value in order to be able to
> +use processor tracing features with this domain.
> +
> +B: The size value must be between 4 kB and 4 GB and it must
I think the minimum value is 8kB, since 4kB would be order 0, which
is used to signal that the feature is disabled?
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 61b4ef7b7e..4eba224590 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1861,6 +1861,26 @@ void parse_config_data(const char *config_source,
> }
> }
>
> +if (!xlu_cfg_get_long(config, "vmtrace_pt_size", , 1) && l) {
> +int32_t shift = 0;
unsigned int? I don't think there's a reason for this to be a fixed
width signed integer.
> +
> +if (l & (l - 1))
> +{
> +fprintf(stderr, "ERROR: pt buffer size must be a power of 2\n");
> +exit(1);
> +}
> +
> +while (l >>= 1) ++shift;
> +
> +if (shift <= XEN_PAGE_SHIFT)
> +{
> +fprintf(stderr, "ERROR: too small pt buffer\n");
> +exit(1);
> +}
> +
> +b_info->vmtrace_pt_order = shift - XEN_PAGE_SHIFT;
> +}
> +
> if (!xlu_cfg_get_list(config, "ioports", , _ioports, 0)) {
> b_info->num_ioports = num_ioports;
> b_info->ioports = calloc(num_ioports, sizeof(*b_info->ioports));
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 0a33e0dfd6..27dcfbac8c 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -338,6 +338,12 @@ static int sanitise_domain_config(struct
> xen_domctl_createdomain *config)
> return -EINVAL;
> }
>
> +if ( config->vmtrace_pt_order && !vmtrace_supported )
> +{
> +dprintk(XENLOG_INFO, "Processor tracing is not supported\n");
> +return -EINVAL;
> +}
> +
> return arch_sanitise_domain_config(config);
> }
>
> @@ -443,6 +449,12 @@ struct domain *domain_create(domid_t domid,
> d->nr_pirqs = min(d->nr_pirqs, nr_irqs);
>
> radix_tree_init(>pirq_tree);
> +
> +if ( config->vmtrace_pt_order )
> +{
> +uint32_t shift_val = config->vmtrace_pt_order + PAGE_SHIFT;
> +d->vmtrace_pt_size = (1ULL << shift_val);
I don't think the vmtrace_pt_size domain field has been introduced
yet?
Please check that each patch builds on it's own, or else we would
break bisectability of the tree.
Also I would consider just storing this directly as an order, there's
no reason to convert it back to a size?
Thanks, Roger.
[xen-unstable-coverity test] 151504: all pass - PUSHED
flight 151504 xen-unstable-coverity real [real] http://logs.test-lab.xenproject.org/osstest/logs/151504/ Perfect :-) All tests in this flight passed as required version targeted for testing: xen 23ca7ec0ba620db52a646d80e22f9703a6589f66 baseline version: xen 88cfd062e8318dfeb67c7d2eb50b6cd224b0738a Last test of basis 151428 2020-06-28 09:23:57 Z3 days Testing same since 151504 2020-07-01 09:23:05 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Jan Beulich Olaf Hering Roger Pau Monné jobs: coverity-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/xen.git 88cfd062e8..23ca7ec0ba 23ca7ec0ba620db52a646d80e22f9703a6589f66 -> coverity-tested/smoke
[PATCH v2 0/7] x86: compat header generation and checking adjustments
As was pointed out by 0e2e54966af5 ("mm: fix public declaration of
struct xen_mem_acquire_resource"), we're not currently handling structs
correctly that have uint64_aligned_t fields. Patch 2 demonstrates that
there was also an issue with XEN_GUEST_HANDLE_64().
Only the 1st patch was previously sent, but the approach chosen has
been changed altogether. All later patches are new. For 4.14 I think
at least patch 1 should be considered.
1: x86: fix compat header generation
2: x86/mce: add compat struct checking for XEN_MC_inject_v2
3: x86/mce: bring hypercall subop compat checking in sync again
4: x86/dmop: add compat struct checking for
XEN_DMOP_map_mem_type_to_ioreq_server
5: x86: generalize padding field handling
6: flask: drop dead compat translation code
7: x86: only generate compat headers actually needed
Jan
Re: [PATCH v2] xen/displif: Protocol version 2
On 01.07.20 09:19, Oleksandr Andrushchenko wrote: From: Oleksandr Andrushchenko 1. Add protocol version as an integer Version string, which is in fact an integer, is hard to handle in the code that supports different protocol versions. To simplify that also add the version as an integer. 2. Pass buffer offset with XENDISPL_OP_DBUF_CREATE There are cases when display data buffer is created with non-zero offset to the data start. Handle such cases and provide that offset while creating a display buffer. 3. Add XENDISPL_OP_GET_EDID command Add an optional request for reading Extended Display Identification Data (EDID) structure which allows better configuration of the display connectors over the configuration set in XenStore. With this change connectors may have multiple resolutions defined with respect to detailed timing definitions and additional properties normally provided by displays. If this request is not supported by the backend then visible area is defined by the relevant XenStore's "resolution" property. If backend provides extended display identification data (EDID) with XENDISPL_OP_GET_EDID request then EDID values must take precedence over the resolutions defined in XenStore. 4. Bump protocol version to 2. Signed-off-by: Oleksandr Andrushchenko Reviewed-by: Juergen Gross Juergen
Re: [PATCH v4 06/10] memory: batch processing in acquire_resource()
On Tue, Jun 30, 2020 at 02:33:49PM +0200, Michał Leszczyński wrote:
> From: Michal Leszczynski
>
> Allow to acquire large resources by allowing acquire_resource()
> to process items in batches, using hypercall continuation.
This patch should be the first of thew series IMO, since it can go in
independently of the rest, as it's a general improvement to
XENMEM_acquire_resource.
> Signed-off-by: Michal Leszczynski
> ---
> xen/common/memory.c | 32 +---
> 1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 714077c1e5..3ab06581a2 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1046,10 +1046,12 @@ static int acquire_grant_table(struct domain *d,
> unsigned int id,
> }
>
> static int acquire_resource(
> -XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
> +XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg,
> +unsigned long *start_extent)
> {
> struct domain *d, *currd = current->domain;
> xen_mem_acquire_resource_t xmar;
> +uint32_t total_frames;
> /*
> * The mfn_list and gfn_list (below) arrays are ok on stack for the
> * moment since they are small, but if they need to grow in future
> @@ -1077,8 +1079,17 @@ static int acquire_resource(
> return 0;
> }
>
> +total_frames = xmar.nr_frames;
> +
> +if ( *start_extent )
> +{
> +xmar.frame += *start_extent;
> +xmar.nr_frames -= *start_extent;
> +guest_handle_add_offset(xmar.frame_list, *start_extent);
> +}
> +
> if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
> -return -E2BIG;
> +xmar.nr_frames = ARRAY_SIZE(mfn_list);
>
> rc = rcu_lock_remote_domain_by_id(xmar.domid, );
> if ( rc )
> @@ -1135,6 +1146,14 @@ static int acquire_resource(
> }
> }
>
> +if ( !rc )
> +{
> +*start_extent += xmar.nr_frames;
> +
> +if ( *start_extent != total_frames )
> +rc = -ERESTART;
> +}
I think you should add some kind of loop here, processing just 32
frames and preempting might be too low. You generally want to loop
doing batches of 32 entries until hypercall_preempt_check() returns
true.
Thanks, Roger.
[PATCH v2 2/4] x86/paravirt: remove 32-bit support from PARAVIRT_XXL
The last 32-bit user of stuff under CONFIG_PARAVIRT_XXL is gone.
Remove 32-bit specific parts.
Signed-off-by: Juergen Gross
---
arch/x86/entry/vdso/vdso32/vclock_gettime.c | 1 +
arch/x86/include/asm/paravirt.h | 92 +++--
arch/x86/include/asm/paravirt_types.h | 21 -
arch/x86/include/asm/pgtable-3level_types.h | 5 --
arch/x86/include/asm/segment.h | 4 -
arch/x86/kernel/cpu/common.c| 8 --
arch/x86/kernel/kprobes/core.c | 1 -
arch/x86/kernel/kprobes/opt.c | 1 -
arch/x86/kernel/paravirt.c | 18
arch/x86/kernel/paravirt_patch.c| 17
arch/x86/xen/enlighten_pv.c | 6 --
11 files changed, 13 insertions(+), 161 deletions(-)
diff --git a/arch/x86/entry/vdso/vdso32/vclock_gettime.c
b/arch/x86/entry/vdso/vdso32/vclock_gettime.c
index 84a4a73f77f7..283ed9d00426 100644
--- a/arch/x86/entry/vdso/vdso32/vclock_gettime.c
+++ b/arch/x86/entry/vdso/vdso32/vclock_gettime.c
@@ -14,6 +14,7 @@
#undef CONFIG_ILLEGAL_POINTER_VALUE
#undef CONFIG_SPARSEMEM_VMEMMAP
#undef CONFIG_NR_CPUS
+#undef CONFIG_PARAVIRT_XXL
#define CONFIG_X86_32 1
#define CONFIG_PGTABLE_LEVELS 2
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 5ca5d297df75..cfe9f6e472b5 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -160,8 +160,6 @@ static inline void wbinvd(void)
PVOP_VCALL0(cpu.wbinvd);
}
-#define get_kernel_rpl() (pv_info.kernel_rpl)
-
static inline u64 paravirt_read_msr(unsigned msr)
{
return PVOP_CALL1(u64, cpu.read_msr, msr);
@@ -277,12 +275,10 @@ static inline void load_TLS(struct thread_struct *t,
unsigned cpu)
PVOP_VCALL2(cpu.load_tls, t, cpu);
}
-#ifdef CONFIG_X86_64
static inline void load_gs_index(unsigned int gs)
{
PVOP_VCALL1(cpu.load_gs_index, gs);
}
-#endif
static inline void write_ldt_entry(struct desc_struct *dt, int entry,
const void *desc)
@@ -372,10 +368,7 @@ static inline pte_t __pte(pteval_t val)
{
pteval_t ret;
- if (sizeof(pteval_t) > sizeof(long))
- ret = PVOP_CALLEE2(pteval_t, mmu.make_pte, val, (u64)val >> 32);
- else
- ret = PVOP_CALLEE1(pteval_t, mmu.make_pte, val);
+ ret = PVOP_CALLEE1(pteval_t, mmu.make_pte, val);
return (pte_t) { .pte = ret };
}
@@ -384,11 +377,7 @@ static inline pteval_t pte_val(pte_t pte)
{
pteval_t ret;
- if (sizeof(pteval_t) > sizeof(long))
- ret = PVOP_CALLEE2(pteval_t, mmu.pte_val,
- pte.pte, (u64)pte.pte >> 32);
- else
- ret = PVOP_CALLEE1(pteval_t, mmu.pte_val, pte.pte);
+ ret = PVOP_CALLEE1(pteval_t, mmu.pte_val, pte.pte);
return ret;
}
@@ -397,10 +386,7 @@ static inline pgd_t __pgd(pgdval_t val)
{
pgdval_t ret;
- if (sizeof(pgdval_t) > sizeof(long))
- ret = PVOP_CALLEE2(pgdval_t, mmu.make_pgd, val, (u64)val >> 32);
- else
- ret = PVOP_CALLEE1(pgdval_t, mmu.make_pgd, val);
+ ret = PVOP_CALLEE1(pgdval_t, mmu.make_pgd, val);
return (pgd_t) { ret };
}
@@ -409,11 +395,7 @@ static inline pgdval_t pgd_val(pgd_t pgd)
{
pgdval_t ret;
- if (sizeof(pgdval_t) > sizeof(long))
- ret = PVOP_CALLEE2(pgdval_t, mmu.pgd_val,
- pgd.pgd, (u64)pgd.pgd >> 32);
- else
- ret = PVOP_CALLEE1(pgdval_t, mmu.pgd_val, pgd.pgd);
+ ret = PVOP_CALLEE1(pgdval_t, mmu.pgd_val, pgd.pgd);
return ret;
}
@@ -433,51 +415,32 @@ static inline void ptep_modify_prot_commit(struct
vm_area_struct *vma, unsigned
pte_t *ptep, pte_t old_pte, pte_t
pte)
{
- if (sizeof(pteval_t) > sizeof(long))
- /* 5 arg words */
- pv_ops.mmu.ptep_modify_prot_commit(vma, addr, ptep, pte);
- else
- PVOP_VCALL4(mmu.ptep_modify_prot_commit,
- vma, addr, ptep, pte.pte);
+ PVOP_VCALL4(mmu.ptep_modify_prot_commit, vma, addr, ptep, pte.pte);
}
static inline void set_pte(pte_t *ptep, pte_t pte)
{
- if (sizeof(pteval_t) > sizeof(long))
- PVOP_VCALL3(mmu.set_pte, ptep, pte.pte, (u64)pte.pte >> 32);
- else
- PVOP_VCALL2(mmu.set_pte, ptep, pte.pte);
+ PVOP_VCALL2(mmu.set_pte, ptep, pte.pte);
}
static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
pte_t *ptep, pte_t pte)
{
- if (sizeof(pteval_t) > sizeof(long))
- /* 5 arg words */
- pv_ops.mmu.set_pte_at(mm, addr, ptep, pte);
- else
- PVOP_VCALL4(mmu.set_pte_at, mm, addr, ptep, pte.pte);
+ PVOP_VCALL4(mmu.set_pte_at, mm, addr, ptep, pte.pte);
}
static inline void
[PATCH v2 1/2] xen/xenbus: avoid large structs and arrays on the stack
xenbus_map_ring_valloc() and its sub-functions are putting quite large
structs and arrays on the stack. This is problematic at runtime, but
might also result in build failures (e.g. with clang due to the option
-Werror,-Wframe-larger-than=... used).
Fix that by moving most of the data from the stack into a dynamically
allocated struct. Performance is no issue here, as
xenbus_map_ring_valloc() is used only when adding a new PV device to
a backend driver.
While at it move some duplicated code from pv/hvm specific mapping
functions to the single caller.
Reported-by: Arnd Bergmann
Signed-off-by: Juergen Gross
---
V2:
- shorten internal function names (Boris Ostrovsky)
---
drivers/xen/xenbus/xenbus_client.c | 161 +++--
1 file changed, 83 insertions(+), 78 deletions(-)
diff --git a/drivers/xen/xenbus/xenbus_client.c
b/drivers/xen/xenbus/xenbus_client.c
index 040d2a43e8e3..9f8372079ecf 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -69,11 +69,27 @@ struct xenbus_map_node {
unsigned int nr_handles;
};
+struct map_ring_valloc {
+ struct xenbus_map_node *node;
+
+ /* Why do we need two arrays? See comment of __xenbus_map_ring */
+ union {
+ unsigned long addrs[XENBUS_MAX_RING_GRANTS];
+ pte_t *ptes[XENBUS_MAX_RING_GRANTS];
+ };
+ phys_addr_t phys_addrs[XENBUS_MAX_RING_GRANTS];
+
+ struct gnttab_map_grant_ref map[XENBUS_MAX_RING_GRANTS];
+ struct gnttab_unmap_grant_ref unmap[XENBUS_MAX_RING_GRANTS];
+
+ unsigned int idx; /* HVM only. */
+};
+
static DEFINE_SPINLOCK(xenbus_valloc_lock);
static LIST_HEAD(xenbus_valloc_pages);
struct xenbus_ring_ops {
- int (*map)(struct xenbus_device *dev,
+ int (*map)(struct xenbus_device *dev, struct map_ring_valloc *info,
grant_ref_t *gnt_refs, unsigned int nr_grefs,
void **vaddr);
int (*unmap)(struct xenbus_device *dev, void *vaddr);
@@ -449,12 +465,32 @@ int xenbus_map_ring_valloc(struct xenbus_device *dev,
grant_ref_t *gnt_refs,
unsigned int nr_grefs, void **vaddr)
{
int err;
+ struct map_ring_valloc *info;
+
+ *vaddr = NULL;
+
+ if (nr_grefs > XENBUS_MAX_RING_GRANTS)
+ return -EINVAL;
+
+ info = kzalloc(sizeof(*info), GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
+
+ info->node = kzalloc(sizeof(*info->node), GFP_KERNEL);
+ if (!info->node) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ err = ring_ops->map(dev, info, gnt_refs, nr_grefs, vaddr);
- err = ring_ops->map(dev, gnt_refs, nr_grefs, vaddr);
/* Some hypervisors are buggy and can return 1. */
if (err > 0)
err = GNTST_general_error;
+ out:
+ kfree(info->node);
+ kfree(info);
return err;
}
EXPORT_SYMBOL_GPL(xenbus_map_ring_valloc);
@@ -466,12 +502,10 @@ static int __xenbus_map_ring(struct xenbus_device *dev,
grant_ref_t *gnt_refs,
unsigned int nr_grefs,
grant_handle_t *handles,
-phys_addr_t *addrs,
+struct map_ring_valloc *info,
unsigned int flags,
bool *leaked)
{
- struct gnttab_map_grant_ref map[XENBUS_MAX_RING_GRANTS];
- struct gnttab_unmap_grant_ref unmap[XENBUS_MAX_RING_GRANTS];
int i, j;
int err = GNTST_okay;
@@ -479,23 +513,22 @@ static int __xenbus_map_ring(struct xenbus_device *dev,
return -EINVAL;
for (i = 0; i < nr_grefs; i++) {
- memset([i], 0, sizeof(map[i]));
- gnttab_set_map_op([i], addrs[i], flags, gnt_refs[i],
- dev->otherend_id);
+ gnttab_set_map_op(>map[i], info->phys_addrs[i], flags,
+ gnt_refs[i], dev->otherend_id);
handles[i] = INVALID_GRANT_HANDLE;
}
- gnttab_batch_map(map, i);
+ gnttab_batch_map(info->map, i);
for (i = 0; i < nr_grefs; i++) {
- if (map[i].status != GNTST_okay) {
- err = map[i].status;
- xenbus_dev_fatal(dev, map[i].status,
+ if (info->map[i].status != GNTST_okay) {
+ err = info->map[i].status;
+ xenbus_dev_fatal(dev, info->map[i].status,
"mapping in shared page %d from domain
%d",
gnt_refs[i], dev->otherend_id);
goto fail;
} else
- handles[i] = map[i].handle;
+ handles[i] = info->map[i].handle;
}
return GNTST_okay;
@@ -503,19 +536,19 @@ static int
RE: [PATCH for-4.14] x86/spec-ctrl: Protect against CALL/JMP straight-line speculation
> -Original Message- > From: Jan Beulich > Sent: 01 July 2020 13:27 > To: Andrew Cooper > Cc: Xen-devel ; Wei Liu ; Roger > Pau Monné > ; Paul Durrant > Subject: Re: [PATCH for-4.14] x86/spec-ctrl: Protect against CALL/JMP > straight-line speculation > > On 01.07.2020 13:58, Andrew Cooper wrote: > > Some x86 CPUs speculatively execute beyond indirect CALL/JMP instructions. > > > > With CONFIG_INDIRECT_THUNK / Retpolines, indirect CALL/JMP instructions are > > converted to direct CALL/JMP's to __x86_indirect_thunk_REG(), leaving just a > > handful of indirect JMPs implementing those stubs. > > > > There is no architectrual execution beyond an indirect JMP, so use INT3 as > > recommended by vendors to halt speculative execution. This is shorter than > > LFENCE (which would also work fine), but also shows up in logs if we do > > unexpected execute them. > > > > Signed-off-by: Andrew Cooper > > Reviewed-by: Jan Beulich Release-acked-by: Paul Durrant
Re: [PATCH] xen: introduce xen_vring_use_dma
On Mon, Jun 29, 2020 at 04:46:09PM -0700, Stefano Stabellini wrote: > > I could imagine some future Xen hosts setting a flag somewhere in the > > platform capability saying "no xen specific flag, rely on > > "VIRTIO_F_ACCESS_PLATFORM". Then you set that accordingly in QEMU. > > How about that? > > Yes, that would be fine and there is no problem implementing something > like that when we get virtio support in Xen. Today there are still no > virtio interfaces provided by Xen to ARM guests (no virtio-block/net, > etc.) > > In fact, in both cases we are discussing virtio is *not* provided by > Xen; it is a firmware interface to something entirely different: > > 1) virtio is used to talk to a remote AMP processor (RPMesg) > 2) virtio is used to talk to a secure-world firmware/OS (Trusty) > > VIRTIO_F_ACCESS_PLATFORM is not set by Xen in these cases but by RPMesg > and by Trusty respectively. I don't know if Trusty should or should not > set VIRTIO_F_ACCESS_PLATFORM, but I think Linux should still work > without issues. > Any virtio implementation that is not in control of the memory map (aka not the hypervisor) absolutely must set VIRTIO_F_ACCESS_PLATFORM, else it is completely broken. > The xen_domain() check in Linux makes it so that vring_use_dma_api > returns the opposite value on native Linux compared to Linux as Xen/ARM > DomU by "accident". By "accident" because there is no architectural > reason why Linux Xen/ARM DomU should behave differently compared to > native Linux in this regard. > > I hope that now it is clearer why I think the if (xen_domain()) check > needs to be improved anyway, even if we fix generic dma_ops with virtio > interfaces missing VIRTIO_F_ACCESS_PLATFORM. IMHO that Xen quirk should never have been added in this form..
Re: [PATCH v2 1/7] x86: fix compat header generation
On Wed, Jul 01, 2020 at 12:25:15PM +0200, Jan Beulich wrote:
> As was pointed out by 0e2e54966af5 ("mm: fix public declaration of
> struct xen_mem_acquire_resource"), we're not currently handling structs
> correctly that have uint64_aligned_t fields. #pragma pack(4) suppresses
> the necessary alignment even if the type did properly survive (which
> it also didn't) in the process of generating the headers. Overall,
> with the above mentioned change applied, there's only a latent issue
> here afaict, i.e. no other of our interface structs is currently
> affected.
>
> As a result it is clear that using #pragma pack(4) is not an option.
> Drop all uses from compat header generation. Make sure
> {,u}int64_aligned_t actually survives, such that explicitly aligned
> fields will remain aligned. Arrange for {,u}int64_t to be transformed
> into a type that's 64 bits wide and 4-byte aligned, by utilizing that
> in typedef-s the "aligned" attribute can be used to reduce alignment.
> Additionally, for the cases where native structures get re-used,
> enforce suitable alignment via typedef-s (which allow alignment to be
> reduced).
>
> This use of typedef-s makes necessary changes to CHECK_*() macro
> generation: Previously get-fields.sh relied on finding struct/union
> keywords when other compound types were used. We now need to use the
> typedef-s (guaranteeing suitable alignment) now, and hence the script
Extra now before the comma I think.
> has to recognize those cases, too. (Unfortunately there are a few
> special cases to be dealt with, but this is really not much different
> from e.g. the pre-existing compat_domain_handle_t special case.)
>
> This need to use typedef-s is certainly somewhat fragile going forward,
> as in similar future cases it is imperative to also use typedef-s, or
> else the CHECK_*() macros won't check what they're supposed to check. I
> don't currently see any means to avoid this fragility, though.
>
> There's one change to generated code according to my observations: In
> arch_compat_vcpu_op() the runstate area "area" variable would previously
> have been put in a just 4-byte aligned stack slot (despite being 8 bytes
> in size), whereas now it gets put in an 8-byte aligned location.
>
> There also results some curious inconsistency in struct xen_mc from
> these changes - I intend to clean this up later on. Otherwise unrelated
> code would also need adjustment right here.
Oh, so that's the reason fields in xen_mc are not all switched to use
their typedef equivalent I guess?
> --- a/xen/tools/get-fields.sh
> +++ b/xen/tools/get-fields.sh
> @@ -418,6 +418,21 @@ check_field ()
> "}")
> level=$(expr $level - 1) id=
> ;;
> + compat_*_t)
> + if [ $level = 2 ]
> + then
> + fields=" "
> + token="${token%_t}"
> + token="${token#compat_}"
> + fi
> + ;;
> + evtchn_*_compat_t)
> + if [ $level = 2 -a $token !=
> evtchn_port_compat_t ]
> + then
> + fields=" "
> + token="${token%_compat_t}"
> + fi
> + ;;
Likely related to the above, but I assume we might want to add a check
here to assert no struct fields are used?
I assume this is not added here in order to prevent exploding due to
the xen_mc issues.
Thanks, Roger.
Re: [PATCH 2/2] xen: cleanup unrealized flash devices
On Wed, Jul 1, 2020 at 8:55 AM Philippe Mathieu-Daudé wrote:
>
> On 7/1/20 2:40 PM, Philippe Mathieu-Daudé wrote:
> > On 7/1/20 2:25 PM, Jason Andryuk wrote:
> >> On Wed, Jul 1, 2020 at 3:03 AM Paul Durrant wrote:
> >>>
> -Original Message-
> From: Philippe Mathieu-Daudé
> Sent: 30 June 2020 18:27
> To: p...@xen.org; xen-devel@lists.xenproject.org; qemu-de...@nongnu.org
> Cc: 'Eduardo Habkost' ; 'Michael S. Tsirkin'
> ; 'Paul Durrant'
> ; 'Jason Andryuk' ; 'Paolo
> Bonzini' ;
> 'Richard Henderson'
> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>
> On 6/30/20 5:44 PM, Paul Durrant wrote:
> >> -Original Message-
> >> From: Philippe Mathieu-Daudé
> >> Sent: 30 June 2020 16:26
> >> To: Paul Durrant ; xen-devel@lists.xenproject.org;
> >> qemu-de...@nongnu.org
> >> Cc: Eduardo Habkost ; Michael S. Tsirkin
> >> ; Paul Durrant
> >> ; Jason Andryuk ; Paolo
> >> Bonzini ;
> >> Richard Henderson
> >> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> >>
> >> On 6/24/20 2:18 PM, Paul Durrant wrote:
> >>> From: Paul Durrant
> >>>
> >>> The generic pc_machine_initfn() calls pc_system_flash_create() which
> >>> creates
> >>> 'system.flash0' and 'system.flash1' devices. These devices are then
> >>> realized
> >>> by pc_system_flash_map() which is called from
> >>> pc_system_firmware_init() which
> >>> itself is called via pc_memory_init(). The latter however is not
> >>> called when
> >>> xen_enable() is true and hence the following assertion fails:
> >>>
> >>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> >>> Assertion `dev->realized' failed
> >>>
> >>> These flash devices are unneeded when using Xen so this patch avoids
> >>> the
> >>> assertion by simply removing them using
> >>> pc_system_flash_cleanup_unused().
> >>>
> >>> Reported-by: Jason Andryuk
> >>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with
> >>> -blockdev")
> >>> Signed-off-by: Paul Durrant
> >>> Tested-by: Jason Andryuk
> >>> ---
> >>> Cc: Paolo Bonzini
> >>> Cc: Richard Henderson
> >>> Cc: Eduardo Habkost
> >>> Cc: "Michael S. Tsirkin"
> >>> Cc: Marcel Apfelbaum
> >>> ---
> >>> hw/i386/pc_piix.c| 9 ++---
> >>> hw/i386/pc_sysfw.c | 2 +-
> >>> include/hw/i386/pc.h | 1 +
> >>> 3 files changed, 8 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >>> index 1497d0e4ae..977d40afb8 100644
> >>> --- a/hw/i386/pc_piix.c
> >>> +++ b/hw/i386/pc_piix.c
> >>> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
> >>> if (!xen_enabled()) {
> >>> pc_memory_init(pcms, system_memory,
> >>> rom_memory, _memory);
> >>> -} else if (machine->kernel_filename != NULL) {
> >>> -/* For xen HVM direct kernel boot, load linux here */
> >>> -xen_load_linux(pcms);
> >>> +} else {
> >>> +pc_system_flash_cleanup_unused(pcms);
> >>
> >> TIL pc_system_flash_cleanup_unused().
> >>
> >> What about restricting at the source?
> >>
> >
> > And leave the devices in place? They are not relevant for Xen, so why
> > not clean up?
>
> No, I meant to not create them in the first place, instead of
> create+destroy.
>
> Anyway what you did works, so I don't have any problem.
> >>>
> >>> IIUC Jason originally tried restricting creation but encountered a
> >>> problem because xen_enabled() would always return false at that point,
> >>> because machine creation occurs before accelerators are initialized.
> >>
> >> Correct. Quoting my previous email:
> >> """
> >> Removing the call to pc_system_flash_create() from pc_machine_initfn()
> >> lets QEMU startup and run a Xen HVM again. xen_enabled() doesn't work
> >> there since accelerators have not been initialized yes, I guess?
> >
> > Ah, I missed that. You pointed at the bug here :)
> >
> > I think pc_system_flash_create() shouldn't be called in init() but
> > realize().
>
> Hmm this is a MachineClass, not qdev, so no realize().
>
> In softmmu/vl.c we have:
>
> 4152 configure_accelerators(argv[0]);
>
> 4327 if (!xen_enabled()) { // so xen_enable() is working
> 4328 /* On 32-bit hosts, QEMU is limited by virtual address space */
> 4329 if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
> 4330 error_report("at most 2047 MB RAM can be simulated");
> 4331 exit(1);
> 4332 }
> 4333 }
>
> 4348 machine_run_board_init(current_machine);
>
> which calls in hw/core/machine.c:
>
> 1089 void machine_run_board_init(MachineState *machine)
> 1090 {
> 1091 MachineClass
Re: [PATCH v4 02/10] x86/vmx: add IPT cpu feature
On 30/06/2020 13:33, Michał Leszczyński wrote: @@ -305,7 +311,6 @@ static int vmx_init_vmcs_config(void) SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS | SECONDARY_EXEC_XSAVES | SECONDARY_EXEC_TSC_SCALING); -rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap); if ( _vmx_misc_cap & VMX_MISC_VMWRITE_ALL ) opt |= SECONDARY_EXEC_ENABLE_VMCS_SHADOWING; if ( opt_vpid_enabled ) diff --git a/xen/common/domain.c b/xen/common/domain.c index 7cc9526139..0a33e0dfd6 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -82,6 +82,8 @@ struct vcpu *idle_vcpu[NR_CPUS] __read_mostly; vcpu_info_t dummy_vcpu_info; +bool_t vmtrace_supported; All the code looks x86 specific. So may I ask why this was implemented in common code? Cheers, -- Julien Grall
Re: [PATCH 2/2] xen: cleanup unrealized flash devices
On 7/1/20 4:59 PM, Jason Andryuk wrote:
> On Wed, Jul 1, 2020 at 8:55 AM Philippe Mathieu-Daudé
> wrote:
>> On 7/1/20 2:40 PM, Philippe Mathieu-Daudé wrote:
>>> On 7/1/20 2:25 PM, Jason Andryuk wrote:
On Wed, Jul 1, 2020 at 3:03 AM Paul Durrant wrote:
>
>> -Original Message-
>> From: Philippe Mathieu-Daudé
>> Sent: 30 June 2020 18:27
>> To: p...@xen.org; xen-devel@lists.xenproject.org; qemu-de...@nongnu.org
>> Cc: 'Eduardo Habkost' ; 'Michael S. Tsirkin'
>> ; 'Paul Durrant'
>> ; 'Jason Andryuk' ; 'Paolo
>> Bonzini' ;
>> 'Richard Henderson'
>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>>
>> On 6/30/20 5:44 PM, Paul Durrant wrote:
-Original Message-
From: Philippe Mathieu-Daudé
Sent: 30 June 2020 16:26
To: Paul Durrant ; xen-devel@lists.xenproject.org;
qemu-de...@nongnu.org
Cc: Eduardo Habkost ; Michael S. Tsirkin
; Paul Durrant
; Jason Andryuk ; Paolo
Bonzini ;
Richard Henderson
Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
On 6/24/20 2:18 PM, Paul Durrant wrote:
> From: Paul Durrant
>
> The generic pc_machine_initfn() calls pc_system_flash_create() which
> creates
> 'system.flash0' and 'system.flash1' devices. These devices are then
> realized
> by pc_system_flash_map() which is called from
> pc_system_firmware_init() which
> itself is called via pc_memory_init(). The latter however is not
> called when
> xen_enable() is true and hence the following assertion fails:
>
> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> Assertion `dev->realized' failed
>
> These flash devices are unneeded when using Xen so this patch avoids
> the
> assertion by simply removing them using
> pc_system_flash_cleanup_unused().
>
> Reported-by: Jason Andryuk
> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with
> -blockdev")
> Signed-off-by: Paul Durrant
> Tested-by: Jason Andryuk
> ---
> Cc: Paolo Bonzini
> Cc: Richard Henderson
> Cc: Eduardo Habkost
> Cc: "Michael S. Tsirkin"
> Cc: Marcel Apfelbaum
> ---
> hw/i386/pc_piix.c| 9 ++---
> hw/i386/pc_sysfw.c | 2 +-
> include/hw/i386/pc.h | 1 +
> 3 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 1497d0e4ae..977d40afb8 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
> if (!xen_enabled()) {
> pc_memory_init(pcms, system_memory,
> rom_memory, _memory);
> -} else if (machine->kernel_filename != NULL) {
> -/* For xen HVM direct kernel boot, load linux here */
> -xen_load_linux(pcms);
> +} else {
> +pc_system_flash_cleanup_unused(pcms);
TIL pc_system_flash_cleanup_unused().
What about restricting at the source?
>>>
>>> And leave the devices in place? They are not relevant for Xen, so why
>>> not clean up?
>>
>> No, I meant to not create them in the first place, instead of
>> create+destroy.
>>
>> Anyway what you did works, so I don't have any problem.
>
> IIUC Jason originally tried restricting creation but encountered a
> problem because xen_enabled() would always return false at that point,
> because machine creation occurs before accelerators are initialized.
Correct. Quoting my previous email:
"""
Removing the call to pc_system_flash_create() from pc_machine_initfn()
lets QEMU startup and run a Xen HVM again. xen_enabled() doesn't work
there since accelerators have not been initialized yes, I guess?
>>>
>>> Ah, I missed that. You pointed at the bug here :)
>>>
>>> I think pc_system_flash_create() shouldn't be called in init() but
>>> realize().
>>
>> Hmm this is a MachineClass, not qdev, so no realize().
>>
>> In softmmu/vl.c we have:
>>
>> 4152 configure_accelerators(argv[0]);
>>
>> 4327 if (!xen_enabled()) { // so xen_enable() is working
>> 4328 /* On 32-bit hosts, QEMU is limited by virtual address space */
>> 4329 if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
>> 4330 error_report("at most 2047 MB RAM can be simulated");
>> 4331 exit(1);
>> 4332 }
>> 4333 }
>>
>> 4348 machine_run_board_init(current_machine);
>>
>> which calls in hw/core/machine.c:
>>
>> 1089 void
Re: [PATCH v4 02/10] x86/vmx: add IPT cpu feature
On 01/07/2020 16:12, Julien Grall wrote: > On 30/06/2020 13:33, Michał Leszczyński wrote: >> @@ -305,7 +311,6 @@ static int vmx_init_vmcs_config(void) >> SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS | >> SECONDARY_EXEC_XSAVES | >> SECONDARY_EXEC_TSC_SCALING); >> - rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap); >> if ( _vmx_misc_cap & VMX_MISC_VMWRITE_ALL ) >> opt |= SECONDARY_EXEC_ENABLE_VMCS_SHADOWING; >> if ( opt_vpid_enabled ) >> diff --git a/xen/common/domain.c b/xen/common/domain.c >> index 7cc9526139..0a33e0dfd6 100644 >> --- a/xen/common/domain.c >> +++ b/xen/common/domain.c >> @@ -82,6 +82,8 @@ struct vcpu *idle_vcpu[NR_CPUS] __read_mostly; >> vcpu_info_t dummy_vcpu_info; >> +bool_t vmtrace_supported; > > All the code looks x86 specific. So may I ask why this was implemented > in common code? There were some questions directed specifically at the ARM maintainers about CoreSight, which have gone unanswered. ~Andrew
Re: [PATCH v2 1/4] x86/xen: remove 32-bit Xen PV guest support
On Wed, Jul 1, 2020 at 7:08 AM Juergen Gross wrote: > > Xen is requiring 64-bit machines today and since Xen 4.14 it can be > built without 32-bit PV guest support. There is no need to carry the > burden of 32-bit PV guest support in the kernel any longer, as new > guests can be either HVM or PVH, or they can use a 64 bit kernel. > > Remove the 32-bit Xen PV support from the kernel. If you send a v3, it would be better to split the move of the 64-bit code into xen-asm.S into a separate patch. -- Brian Gerst
Re: [PATCH v2 1/7] x86: fix compat header generation
On 01.07.2020 18:10, Roger Pau Monné wrote:
> On Wed, Jul 01, 2020 at 12:25:15PM +0200, Jan Beulich wrote:
>> As was pointed out by 0e2e54966af5 ("mm: fix public declaration of
>> struct xen_mem_acquire_resource"), we're not currently handling structs
>> correctly that have uint64_aligned_t fields. #pragma pack(4) suppresses
>> the necessary alignment even if the type did properly survive (which
>> it also didn't) in the process of generating the headers. Overall,
>> with the above mentioned change applied, there's only a latent issue
>> here afaict, i.e. no other of our interface structs is currently
>> affected.
>>
>> As a result it is clear that using #pragma pack(4) is not an option.
>> Drop all uses from compat header generation. Make sure
>> {,u}int64_aligned_t actually survives, such that explicitly aligned
>> fields will remain aligned. Arrange for {,u}int64_t to be transformed
>> into a type that's 64 bits wide and 4-byte aligned, by utilizing that
>> in typedef-s the "aligned" attribute can be used to reduce alignment.
>> Additionally, for the cases where native structures get re-used,
>> enforce suitable alignment via typedef-s (which allow alignment to be
>> reduced).
>>
>> This use of typedef-s makes necessary changes to CHECK_*() macro
>> generation: Previously get-fields.sh relied on finding struct/union
>> keywords when other compound types were used. We now need to use the
>> typedef-s (guaranteeing suitable alignment) now, and hence the script
>
> Extra now before the comma I think.
>
>> has to recognize those cases, too. (Unfortunately there are a few
>> special cases to be dealt with, but this is really not much different
>> from e.g. the pre-existing compat_domain_handle_t special case.)
>>
>> This need to use typedef-s is certainly somewhat fragile going forward,
>> as in similar future cases it is imperative to also use typedef-s, or
>> else the CHECK_*() macros won't check what they're supposed to check. I
>> don't currently see any means to avoid this fragility, though.
>>
>> There's one change to generated code according to my observations: In
>> arch_compat_vcpu_op() the runstate area "area" variable would previously
>> have been put in a just 4-byte aligned stack slot (despite being 8 bytes
>> in size), whereas now it gets put in an 8-byte aligned location.
>>
>> There also results some curious inconsistency in struct xen_mc from
>> these changes - I intend to clean this up later on. Otherwise unrelated
>> code would also need adjustment right here.
>
> Oh, so that's the reason fields in xen_mc are not all switched to use
> their typedef equivalent I guess?
Yes - see patches later in the series, which take care of the anomaly.
>> --- a/xen/tools/get-fields.sh
>> +++ b/xen/tools/get-fields.sh
>> @@ -418,6 +418,21 @@ check_field ()
>> "}")
>> level=$(expr $level - 1) id=
>> ;;
>> +compat_*_t)
>> +if [ $level = 2 ]
>> +then
>> +fields=" "
>> +token="${token%_t}"
>> +token="${token#compat_}"
>> +fi
>> +;;
>> +evtchn_*_compat_t)
>> +if [ $level = 2 -a $token !=
>> evtchn_port_compat_t ]
>> +then
>> +fields=" "
>> +token="${token%_compat_t}"
>> +fi
>> +;;
>
> Likely related to the above, but I assume we might want to add a check
> here to assert no struct fields are used?
I think we could, but have you found similar assertions
elsewhere? There being any fields would, aiui, indicate a syntax
violation (or else $level can't be 2), and I'd rather leave
catching these to the compiler.
> I assume this is not added here in order to prevent exploding due to
> the xen_mc issues.
I don't think it would, as it continues handling struct/union
just fine. (We may want to drop this support, to enforce no
use of only typedef-s, but I'm not sure _that_ wouldn't
explode.)
Jan
Re: [PATCH v4 02/10] x86/vmx: add IPT cpu feature
On 01/07/2020 17:06, Andrew Cooper wrote: On 01/07/2020 16:12, Julien Grall wrote: On 30/06/2020 13:33, Michał Leszczyński wrote: @@ -305,7 +311,6 @@ static int vmx_init_vmcs_config(void) SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS | SECONDARY_EXEC_XSAVES | SECONDARY_EXEC_TSC_SCALING); - rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap); if ( _vmx_misc_cap & VMX_MISC_VMWRITE_ALL ) opt |= SECONDARY_EXEC_ENABLE_VMCS_SHADOWING; if ( opt_vpid_enabled ) diff --git a/xen/common/domain.c b/xen/common/domain.c index 7cc9526139..0a33e0dfd6 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -82,6 +82,8 @@ struct vcpu *idle_vcpu[NR_CPUS] __read_mostly; vcpu_info_t dummy_vcpu_info; +bool_t vmtrace_supported; All the code looks x86 specific. So may I ask why this was implemented in common code? There were some questions directed specifically at the ARM maintainers about CoreSight, which have gone unanswered. I can only find one question related to the size. Is there any other? I don't know how the interface will look like given that AFAICT the buffer may be embedded in the HW. We would need to investigate how to differentiate between two domUs in this case without impacting the performance in the common code. So I think it is a little premature to implement this in common code and always compiled in for Arm. It would be best if this stay in x86 code. Cheers, -- Julien Grall
[libvirt test] 151496: tolerable all pass - PUSHED
flight 151496 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/151496/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 151469 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 151469 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-xsm 13 migrate-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-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-qcow2 12 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 13 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-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-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass version targeted for testing: libvirt e7998ebeaf15e4e8825be0dd97aa1316f194f00d baseline version: libvirt 4268e187531eb370bc6fbac4496018bb7fef6716 Last test of basis 151469 2020-06-30 04:19:06 Z1 days Testing same since 151496 2020-07-01 04:23:43 Z0 days1 attempts People who touched revisions under test: Daniel P. Berrangé Fedora Weblate Translation Weblate Yuri Chornoivan 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 4268e18753..e7998ebeaf e7998ebeaf15e4e8825be0dd97aa1316f194f00d -> xen-tested-master
Re: [PATCH v4 05/10] common/domain: allocate vmtrace_pt_buffer
Hi,
On 30/06/2020 13:33, Michał Leszczyński wrote:
+static int vmtrace_alloc_buffers(struct vcpu *v)
+{
+struct page_info *pg;
+uint64_t size = v->domain->vmtrace_pt_size;
+
+if ( size < PAGE_SIZE || size > GB(4) || (size & (size - 1)) )
+{
+/*
+ * We don't accept trace buffer size smaller than single page
+ * and the upper bound is defined as 4GB in the specification.
This is common code, so what specification are you talking about?
I am guessing this is an Intel one, but I don't think Intel should
dictate the common code implementation.
+ * The buffer size must be also a power of 2.
+ */
+return -EINVAL;
+}
+
+pg = alloc_domheap_pages(v->domain, get_order_from_bytes(size),
+ MEMF_no_refcount);
+
+if ( !pg )
+return -ENOMEM;
+
+v->arch.vmtrace.pt_buf = pg;
v->arch.vmtrace.pt_buf is not defined on Arm. Please make sure common
code build on all arch.
Cheers,
--
Julien Grall
Re: [PATCH v4 02/10] x86/vmx: add IPT cpu feature
On 01/07/2020 17:17, Julien Grall wrote: On 01/07/2020 17:06, Andrew Cooper wrote: On 01/07/2020 16:12, Julien Grall wrote: On 30/06/2020 13:33, Michał Leszczyński wrote: @@ -305,7 +311,6 @@ static int vmx_init_vmcs_config(void) SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS | SECONDARY_EXEC_XSAVES | SECONDARY_EXEC_TSC_SCALING); - rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap); if ( _vmx_misc_cap & VMX_MISC_VMWRITE_ALL ) opt |= SECONDARY_EXEC_ENABLE_VMCS_SHADOWING; if ( opt_vpid_enabled ) diff --git a/xen/common/domain.c b/xen/common/domain.c index 7cc9526139..0a33e0dfd6 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -82,6 +82,8 @@ struct vcpu *idle_vcpu[NR_CPUS] __read_mostly; vcpu_info_t dummy_vcpu_info; +bool_t vmtrace_supported; All the code looks x86 specific. So may I ask why this was implemented in common code? There were some questions directed specifically at the ARM maintainers about CoreSight, which have gone unanswered. I can only find one question related to the size. Is there any other? I don't know how the interface will look like given that AFAICT the buffer may be embedded in the HW. We would need to investigate how to differentiate between two domUs in this case without impacting the performance in the common code. s/in the common code/during the context switch/ So I think it is a little premature to implement this in common code and always compiled in for Arm. It would be best if this stay in x86 code. Cheers, -- Julien Grall
Re: [PATCH 3/6] x86/entry/64/compat: Fix Xen PV SYSENTER frame setup
On Fri, Jun 26, 2020 at 1:30 PM Andy Lutomirski wrote:
>
> The SYSENTER frame setup was nonsense. It worked by accident
> because the normal code into which the Xen asm jumped
> (entry_SYSENTER_32/compat) threw away SP without touching the stack.
> entry_SYSENTER_compat was recently modified such that it relied on
> having a valid stack pointer, so now the Xen asm needs to invoke it
> with a valid stack.
>
> Fix it up like SYSCALL: use the Xen-provided frame and skip the bare
> metal prologue.
>
> Cc: Boris Ostrovsky
> Cc: Juergen Gross
> Cc: Stefano Stabellini
> Cc: xen-devel@lists.xenproject.org
> Fixes: 1c3e5d3f60e2 ("x86/entry: Make entry_64_compat.S objtool clean")
> Signed-off-by: Andy Lutomirski
> ---
> arch/x86/entry/entry_64_compat.S | 1 +
> arch/x86/xen/xen-asm_64.S| 20
> 2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/entry/entry_64_compat.S
> b/arch/x86/entry/entry_64_compat.S
> index 7b9d8150f652..381a6de7de9c 100644
> --- a/arch/x86/entry/entry_64_compat.S
> +++ b/arch/x86/entry/entry_64_compat.S
> @@ -79,6 +79,7 @@ SYM_CODE_START(entry_SYSENTER_compat)
> pushfq /* pt_regs->flags (except IF = 0) */
> pushq $__USER32_CS/* pt_regs->cs */
> pushq $0 /* pt_regs->ip = 0 (placeholder) */
> +SYM_INNER_LABEL(entry_SYSENTER_compat_after_hwframe, SYM_L_GLOBAL)
This skips over the section that truncates the syscall number to
32-bits. The comments present some doubt that it is actually
necessary, but the Xen path shouldn't differ from native. That code
should be moved after this new label.
--
Brian Gerst
Re: [PATCH v4 02/10] x86/vmx: add IPT cpu feature
On 01/07/2020 17:18, Julien Grall wrote: > > > On 01/07/2020 17:17, Julien Grall wrote: >> >> >> On 01/07/2020 17:06, Andrew Cooper wrote: >>> On 01/07/2020 16:12, Julien Grall wrote: On 30/06/2020 13:33, Michał Leszczyński wrote: > @@ -305,7 +311,6 @@ static int vmx_init_vmcs_config(void) > SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS | > SECONDARY_EXEC_XSAVES | > SECONDARY_EXEC_TSC_SCALING); > - rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap); > if ( _vmx_misc_cap & VMX_MISC_VMWRITE_ALL ) > opt |= SECONDARY_EXEC_ENABLE_VMCS_SHADOWING; > if ( opt_vpid_enabled ) > diff --git a/xen/common/domain.c b/xen/common/domain.c > index 7cc9526139..0a33e0dfd6 100644 > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -82,6 +82,8 @@ struct vcpu *idle_vcpu[NR_CPUS] __read_mostly; > vcpu_info_t dummy_vcpu_info; > +bool_t vmtrace_supported; All the code looks x86 specific. So may I ask why this was implemented in common code? >>> >>> There were some questions directed specifically at the ARM maintainers >>> about CoreSight, which have gone unanswered. >> >> I can only find one question related to the size. Is there any other? >> >> I don't know how the interface will look like given that AFAICT the >> buffer may be embedded in the HW. We would need to investigate how to >> differentiate between two domUs in this case without impacting the >> performance in the common code. > > s/in the common code/during the context switch/ > >> So I think it is a little premature to implement this in common code >> and always compiled in for Arm. It would be best if this stay in x86 >> code. I've just checked with a colleague. CoreSight can dump to a memory buffer - there's even a decode library for the packet stream https://github.com/Linaro/OpenCSD, although ultimately it is platform specific as to whether the feature is supported. Furthermore, the choice isn't "x86 vs ARM", now that RISCv support is on-list, and Power9 is floating on the horizon. For the sake of what is literally just one byte in common code, I stand my original suggestion of this being a common interface. It is not something which should be x86 specific. ~Andrew
Re: [PATCH] xen: introduce xen_vring_use_dma
On Wed, 1 Jul 2020, Christoph Hellwig wrote: > On Mon, Jun 29, 2020 at 04:46:09PM -0700, Stefano Stabellini wrote: > > > I could imagine some future Xen hosts setting a flag somewhere in the > > > platform capability saying "no xen specific flag, rely on > > > "VIRTIO_F_ACCESS_PLATFORM". Then you set that accordingly in QEMU. > > > How about that? > > > > Yes, that would be fine and there is no problem implementing something > > like that when we get virtio support in Xen. Today there are still no > > virtio interfaces provided by Xen to ARM guests (no virtio-block/net, > > etc.) > > > > In fact, in both cases we are discussing virtio is *not* provided by > > Xen; it is a firmware interface to something entirely different: > > > > 1) virtio is used to talk to a remote AMP processor (RPMesg) > > 2) virtio is used to talk to a secure-world firmware/OS (Trusty) > > > > VIRTIO_F_ACCESS_PLATFORM is not set by Xen in these cases but by RPMesg > > and by Trusty respectively. I don't know if Trusty should or should not > > set VIRTIO_F_ACCESS_PLATFORM, but I think Linux should still work > > without issues. > > > > Any virtio implementation that is not in control of the memory map > (aka not the hypervisor) absolutely must set VIRTIO_F_ACCESS_PLATFORM, > else it is completely broken. Lots of broken virtio implementations out there it would seem :-( > > The xen_domain() check in Linux makes it so that vring_use_dma_api > > returns the opposite value on native Linux compared to Linux as Xen/ARM > > DomU by "accident". By "accident" because there is no architectural > > reason why Linux Xen/ARM DomU should behave differently compared to > > native Linux in this regard. > > > > I hope that now it is clearer why I think the if (xen_domain()) check > > needs to be improved anyway, even if we fix generic dma_ops with virtio > > interfaces missing VIRTIO_F_ACCESS_PLATFORM. > > IMHO that Xen quirk should never have been added in this form.. Would you be in favor of a more flexible check along the lines of the one proposed in the patch that started this thread: if (xen_vring_use_dma()) return true; xen_vring_use_dma would be implemented so that it returns true when xen_swiotlb is required and false otherwise.
Re: [PATCH] xen: introduce xen_vring_use_dma
On Wed, Jul 01, 2020 at 10:34:53AM -0700, Stefano Stabellini wrote: > Would you be in favor of a more flexible check along the lines of the > one proposed in the patch that started this thread: > > if (xen_vring_use_dma()) > return true; > > > xen_vring_use_dma would be implemented so that it returns true when > xen_swiotlb is required and false otherwise. Just to stress - with a patch like this virtio can *still* use DMA API if PLATFORM_ACCESS is set. So if DMA API is broken on some platforms as you seem to be saying, you guys should fix it before doing something like this.. -- MST
Re: [PATCH v4 01/10] x86/vmx: add Intel PT MSR definitions
On 30/06/2020 13:33, Michał Leszczyński wrote: > From: Michal Leszczynski > > Define constants related to Intel Processor Trace features. > > Signed-off-by: Michal Leszczynski Acked-by: Andrew Cooper I wanted to have a play with the series, and have ended up having to do the rebase anyway. As we're in code freeze for 4.14, I've started x86-next in its usual location (https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/x86-next) and will commit this (and any other accumulated patches) once 4.15 opens. ~Andrew
Re: [PATCH v4 02/10] x86/vmx: add IPT cpu feature
On 01/07/2020 19:02, Julien Grall wrote: > Hi, > > On 01/07/2020 18:26, Andrew Cooper wrote: >> On 01/07/2020 17:18, Julien Grall wrote: >>> >>> >>> On 01/07/2020 17:17, Julien Grall wrote: On 01/07/2020 17:06, Andrew Cooper wrote: > On 01/07/2020 16:12, Julien Grall wrote: >> On 30/06/2020 13:33, Michał Leszczyński wrote: >>> @@ -305,7 +311,6 @@ static int vmx_init_vmcs_config(void) >>> SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS | >>> SECONDARY_EXEC_XSAVES | >>> SECONDARY_EXEC_TSC_SCALING); >>> - rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap); >>> if ( _vmx_misc_cap & VMX_MISC_VMWRITE_ALL ) >>> opt |= SECONDARY_EXEC_ENABLE_VMCS_SHADOWING; >>> if ( opt_vpid_enabled ) >>> diff --git a/xen/common/domain.c b/xen/common/domain.c >>> index 7cc9526139..0a33e0dfd6 100644 >>> --- a/xen/common/domain.c >>> +++ b/xen/common/domain.c >>> @@ -82,6 +82,8 @@ struct vcpu *idle_vcpu[NR_CPUS] __read_mostly; >>> vcpu_info_t dummy_vcpu_info; >>> +bool_t vmtrace_supported; >> >> All the code looks x86 specific. So may I ask why this was >> implemented >> in common code? > > There were some questions directed specifically at the ARM > maintainers > about CoreSight, which have gone unanswered. I can only find one question related to the size. Is there any other? I don't know how the interface will look like given that AFAICT the buffer may be embedded in the HW. We would need to investigate how to differentiate between two domUs in this case without impacting the performance in the common code. >>> >>> s/in the common code/during the context switch/ >>> So I think it is a little premature to implement this in common code and always compiled in for Arm. It would be best if this stay in x86 code. >> >> I've just checked with a colleague. CoreSight can dump to a memory >> buffer - there's even a decode library for the packet stream >> https://github.com/Linaro/OpenCSD, although ultimately it is platform >> specific as to whether the feature is supported. >> >> Furthermore, the choice isn't "x86 vs ARM", now that RISCv support is >> on-list, and Power9 is floating on the horizon. >> >> For the sake of what is literally just one byte in common code, I stand >> my original suggestion of this being a common interface. It is not >> something which should be x86 specific. > > This argument can also be used against putting in common code. What I > am the most concern of is we are trying to guess how the interface > will look like for another architecture. Your suggested interface may > work, but this also may end up to be a complete mess. > > So I think we want to wait for a new architecture to use vmtrace > before moving to common code. This is not going to be a massive effort > to move that bit in common if needed. I suggest you read the series. The only thing in common code is the bit of the interface saying "I'd like buffers this big please". ~Andrew
[linux-linus test] 151494: regressions - FAIL
flight 151494 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/151494/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-arm64-arm64-libvirt-xsm 16 guest-start/debian.repeat fail REGR. vs. 151214 Tests which are failing intermittently (not blocking): test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 guest-saverestore fail pass in 151480 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 151214 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 151214 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 151214 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 151214 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 151214 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 151214 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 151214 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 151214 test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 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-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 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-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-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 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-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-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 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-amd64-amd64-libvirt-vhd 12 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 test-armhf-armhf-xl 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-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail 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-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-libvirt 13 migrate-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-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass version targeted for testing: linux7c30b859a947535f2213277e827d7ac7dcff9c84 baseline version: linux1b5044021070efa3259f3e9548dc35d1eb6aa844 Last test of basis 151214 2020-06-18 02:27:46 Z 13 days Failing since151236 2020-06-19 19:10:35 Z 11 days 15 attempts Testing same since 151467 2020-06-30 02:29:41 Z1 days3 attempts
[linux-5.4 test] 151503: regressions - FAIL
flight 151503 linux-5.4 real [real] http://logs.test-lab.xenproject.org/osstest/logs/151503/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-examine 4 memdisk-try-append fail REGR. vs. 151339 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-amd64-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-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-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-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-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-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 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-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-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stop fail 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 13 migrate-support-checkfail never pass test-armhf-armhf-xl 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-libvirt 14 saverestore-support-checkfail never pass test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail 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-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail never pass test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass version targeted for testing: linuxe75220890bf6b37c5f7b1dbd81d8292ed6d96643 baseline version: linux4e9688ad3d36e8f73c73e435f53da5ae1cd91a70 Last test of basis 151339 2020-06-24 16:09:27 Z7 days Testing same since 151503 2020-07-01 09:18:19 Z0 days1 attempts People who touched revisions under test: Aaron Plattner Aditya Pakki Al Cooper Al Viro Alan Stern Alex Deucher Alexander Lobakin Alexei Starovoitov Andrew Morton
Re: [PATCH v4 02/10] x86/vmx: add IPT cpu feature
Hi, On 01/07/2020 18:26, Andrew Cooper wrote: On 01/07/2020 17:18, Julien Grall wrote: On 01/07/2020 17:17, Julien Grall wrote: On 01/07/2020 17:06, Andrew Cooper wrote: On 01/07/2020 16:12, Julien Grall wrote: On 30/06/2020 13:33, Michał Leszczyński wrote: @@ -305,7 +311,6 @@ static int vmx_init_vmcs_config(void) SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS | SECONDARY_EXEC_XSAVES | SECONDARY_EXEC_TSC_SCALING); - rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap); if ( _vmx_misc_cap & VMX_MISC_VMWRITE_ALL ) opt |= SECONDARY_EXEC_ENABLE_VMCS_SHADOWING; if ( opt_vpid_enabled ) diff --git a/xen/common/domain.c b/xen/common/domain.c index 7cc9526139..0a33e0dfd6 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -82,6 +82,8 @@ struct vcpu *idle_vcpu[NR_CPUS] __read_mostly; vcpu_info_t dummy_vcpu_info; +bool_t vmtrace_supported; All the code looks x86 specific. So may I ask why this was implemented in common code? There were some questions directed specifically at the ARM maintainers about CoreSight, which have gone unanswered. I can only find one question related to the size. Is there any other? I don't know how the interface will look like given that AFAICT the buffer may be embedded in the HW. We would need to investigate how to differentiate between two domUs in this case without impacting the performance in the common code. s/in the common code/during the context switch/ So I think it is a little premature to implement this in common code and always compiled in for Arm. It would be best if this stay in x86 code. I've just checked with a colleague. CoreSight can dump to a memory buffer - there's even a decode library for the packet stream https://github.com/Linaro/OpenCSD, although ultimately it is platform specific as to whether the feature is supported. Furthermore, the choice isn't "x86 vs ARM", now that RISCv support is on-list, and Power9 is floating on the horizon. For the sake of what is literally just one byte in common code, I stand my original suggestion of this being a common interface. It is not something which should be x86 specific. This argument can also be used against putting in common code. What I am the most concern of is we are trying to guess how the interface will look like for another architecture. Your suggested interface may work, but this also may end up to be a complete mess. So I think we want to wait for a new architecture to use vmtrace before moving to common code. This is not going to be a massive effort to move that bit in common if needed. Cheers, -- Julien Grall
Re: [PATCH v4 02/10] x86/vmx: add IPT cpu feature
On 30/06/2020 13:33, Michał Leszczyński wrote:
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index ca94c2bedc..b73d824357 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -291,6 +291,12 @@ static int vmx_init_vmcs_config(void)
> _vmx_cpu_based_exec_control &=
> ~(CPU_BASED_CR8_LOAD_EXITING | CPU_BASED_CR8_STORE_EXITING);
>
> +rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap);
> +
> +/* Check whether IPT is supported in VMX operation. */
> +vmtrace_supported = cpu_has_ipt &&
> +(_vmx_misc_cap & VMX_MISC_PT_SUPPORTED);
There is a subtle corner case here. vmx_init_vmcs_config() is called on
all CPUs, and is supposed to level things down safely if we find any
asymmetry.
If instead you go with something like this:
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index b73d824357..6960109183 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -294,8 +294,8 @@ static int vmx_init_vmcs_config(void)
rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap);
/* Check whether IPT is supported in VMX operation. */
- vmtrace_supported = cpu_has_ipt &&
- (_vmx_misc_cap & VMX_MISC_PT_SUPPORTED);
+ if ( !(_vmx_misc_cap & VMX_MISC_PT_SUPPORTED) )
+ vmtrace_supported = false;
if ( _vmx_cpu_based_exec_control &
CPU_BASED_ACTIVATE_SECONDARY_CONTROLS )
{
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index c9b6af826d..9d7822e006 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1092,6 +1092,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
#endif
}
+ /* Set a default for VMTrace before HVM setup occurs. */
+ vmtrace_supported = cpu_has_ipt;
+
/* Sanitise the raw E820 map to produce a final clean version. */
max_page = raw_max_page = init_e820(memmap_type, _raw);
Then you'll also get a vmtrace_supported=true which works correctly in
the Broadwell and no-VT-x case as well.
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 7cc9526139..0a33e0dfd6 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -82,6 +82,8 @@ struct vcpu *idle_vcpu[NR_CPUS] __read_mostly;
>
> vcpu_info_t dummy_vcpu_info;
>
> +bool_t vmtrace_supported;
bool please. We're in the process of converting over to C99 bools, and
objection was taken to a tree-wide cleanup.
> +
> static void __domain_finalise_shutdown(struct domain *d)
> {
> struct vcpu *v;
> diff --git a/xen/include/asm-x86/cpufeature.h
> b/xen/include/asm-x86/cpufeature.h
> index f790d5c1f8..8d7955dd87 100644
> --- a/xen/include/asm-x86/cpufeature.h
> +++ b/xen/include/asm-x86/cpufeature.h
> @@ -104,6 +104,7 @@
> #define cpu_has_clwbboot_cpu_has(X86_FEATURE_CLWB)
> #define cpu_has_avx512erboot_cpu_has(X86_FEATURE_AVX512ER)
> #define cpu_has_avx512cdboot_cpu_has(X86_FEATURE_AVX512CD)
> +#define cpu_has_ipt boot_cpu_has(X86_FEATURE_IPT)
> #define cpu_has_sha boot_cpu_has(X86_FEATURE_SHA)
> #define cpu_has_avx512bwboot_cpu_has(X86_FEATURE_AVX512BW)
> #define cpu_has_avx512vlboot_cpu_has(X86_FEATURE_AVX512VL)
> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h
> b/xen/include/asm-x86/hvm/vmx/vmcs.h
> index 906810592f..0e9a0b8de6 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -283,6 +283,7 @@ extern u32 vmx_secondary_exec_control;
> #define VMX_VPID_INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL 0x800ULL
> extern u64 vmx_ept_vpid_cap;
>
> +#define VMX_MISC_PT_SUPPORTED 0x4000
VMX_MISC_PROC_TRACE, and ...
> #define VMX_MISC_CR3_TARGET 0x01ff
> #define VMX_MISC_VMWRITE_ALL0x2000
>
> diff --git a/xen/include/public/arch-x86/cpufeatureset.h
> b/xen/include/public/arch-x86/cpufeatureset.h
> index 5ca35d9d97..0d3f15f628 100644
> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -217,6 +217,7 @@ XEN_CPUFEATURE(SMAP, 5*32+20) /*S Supervisor
> Mode Access Prevention */
> XEN_CPUFEATURE(AVX512_IFMA, 5*32+21) /*A AVX-512 Integer Fused Multiply
> Add */
> XEN_CPUFEATURE(CLFLUSHOPT,5*32+23) /*A CLFLUSHOPT instruction */
> XEN_CPUFEATURE(CLWB, 5*32+24) /*A CLWB instruction */
> +XEN_CPUFEATURE(IPT, 5*32+25) /* Intel Processor Trace */
.. any chance we can spell this out as PROC_TRACE? The "Intel" part
won't be true if any of the other vendors choose to implement this
interface to the spec.
Otherwise, LGTM.
~Andrew
Re: [PATCH v4 02/10] x86/vmx: add IPT cpu feature
On 01/07/2020 19:06, Andrew Cooper wrote: On 01/07/2020 19:02, Julien Grall wrote: Hi, On 01/07/2020 18:26, Andrew Cooper wrote: On 01/07/2020 17:18, Julien Grall wrote: On 01/07/2020 17:17, Julien Grall wrote: On 01/07/2020 17:06, Andrew Cooper wrote: On 01/07/2020 16:12, Julien Grall wrote: On 30/06/2020 13:33, Michał Leszczyński wrote: @@ -305,7 +311,6 @@ static int vmx_init_vmcs_config(void) SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS | SECONDARY_EXEC_XSAVES | SECONDARY_EXEC_TSC_SCALING); - rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap); if ( _vmx_misc_cap & VMX_MISC_VMWRITE_ALL ) opt |= SECONDARY_EXEC_ENABLE_VMCS_SHADOWING; if ( opt_vpid_enabled ) diff --git a/xen/common/domain.c b/xen/common/domain.c index 7cc9526139..0a33e0dfd6 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -82,6 +82,8 @@ struct vcpu *idle_vcpu[NR_CPUS] __read_mostly; vcpu_info_t dummy_vcpu_info; +bool_t vmtrace_supported; All the code looks x86 specific. So may I ask why this was implemented in common code? There were some questions directed specifically at the ARM maintainers about CoreSight, which have gone unanswered. I can only find one question related to the size. Is there any other? I don't know how the interface will look like given that AFAICT the buffer may be embedded in the HW. We would need to investigate how to differentiate between two domUs in this case without impacting the performance in the common code. s/in the common code/during the context switch/ So I think it is a little premature to implement this in common code and always compiled in for Arm. It would be best if this stay in x86 code. I've just checked with a colleague. CoreSight can dump to a memory buffer - there's even a decode library for the packet stream https://github.com/Linaro/OpenCSD, although ultimately it is platform specific as to whether the feature is supported. Furthermore, the choice isn't "x86 vs ARM", now that RISCv support is on-list, and Power9 is floating on the horizon. For the sake of what is literally just one byte in common code, I stand my original suggestion of this being a common interface. It is not something which should be x86 specific. This argument can also be used against putting in common code. What I am the most concern of is we are trying to guess how the interface will look like for another architecture. Your suggested interface may work, but this also may end up to be a complete mess. So I think we want to wait for a new architecture to use vmtrace before moving to common code. This is not going to be a massive effort to move that bit in common if needed. I suggest you read the series. Already went through the series and ... The only thing in common code is the bit of the interface saying "I'd like buffers this big please". ... I stand by my point. There is no need to have this code in common code until someone else need it. This code can be easily implemented in arch_domain_create(). Cheers, -- Julien Grall
Re: [PATCH 3/6] x86/entry/64/compat: Fix Xen PV SYSENTER frame setup
On Wed, Jul 1, 2020 at 8:42 AM Brian Gerst wrote:
>
> On Fri, Jun 26, 2020 at 1:30 PM Andy Lutomirski wrote:
> >
> > The SYSENTER frame setup was nonsense. It worked by accident
> > because the normal code into which the Xen asm jumped
> > (entry_SYSENTER_32/compat) threw away SP without touching the stack.
> > entry_SYSENTER_compat was recently modified such that it relied on
> > having a valid stack pointer, so now the Xen asm needs to invoke it
> > with a valid stack.
> >
> > Fix it up like SYSCALL: use the Xen-provided frame and skip the bare
> > metal prologue.
> >
> > Cc: Boris Ostrovsky
> > Cc: Juergen Gross
> > Cc: Stefano Stabellini
> > Cc: xen-devel@lists.xenproject.org
> > Fixes: 1c3e5d3f60e2 ("x86/entry: Make entry_64_compat.S objtool clean")
> > Signed-off-by: Andy Lutomirski
> > ---
> > arch/x86/entry/entry_64_compat.S | 1 +
> > arch/x86/xen/xen-asm_64.S| 20
> > 2 files changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/entry/entry_64_compat.S
> > b/arch/x86/entry/entry_64_compat.S
> > index 7b9d8150f652..381a6de7de9c 100644
> > --- a/arch/x86/entry/entry_64_compat.S
> > +++ b/arch/x86/entry/entry_64_compat.S
> > @@ -79,6 +79,7 @@ SYM_CODE_START(entry_SYSENTER_compat)
> > pushfq /* pt_regs->flags (except IF = 0) */
> > pushq $__USER32_CS/* pt_regs->cs */
> > pushq $0 /* pt_regs->ip = 0 (placeholder) */
> > +SYM_INNER_LABEL(entry_SYSENTER_compat_after_hwframe, SYM_L_GLOBAL)
>
> This skips over the section that truncates the syscall number to
> 32-bits. The comments present some doubt that it is actually
> necessary, but the Xen path shouldn't differ from native. That code
> should be moved after this new label.
Whoops. I thought I caught that myself, but apparently not. I'll fix it.
>
> --
> Brian Gerst
[qemu-mainline test] 151500: regressions - FAIL
flight 151500 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/151500/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-win7-amd64 10 windows-install fail REGR. vs. 151065 test-amd64-amd64-libvirt-vhd 10 debian-di-installfail REGR. vs. 151065 test-amd64-i386-xl-qemuu-win7-amd64 10 windows-install fail REGR. vs. 151065 test-amd64-amd64-qemuu-nested-intel 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-libvirt-xsm 12 guest-start fail REGR. vs. 151065 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-qemuu-nested-amd 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-xl-qcow210 debian-di-installfail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-qemuu-rhel6hvm-amd 10 redhat-install fail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-xl-qemuu-ws16-amd64 10 windows-install fail REGR. vs. 151065 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-libvirt-pair 21 guest-start/debian fail REGR. vs. 151065 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-freebsd10-i386 11 guest-startfail REGR. vs. 151065 test-amd64-i386-qemuu-rhel6hvm-intel 10 redhat-install fail REGR. vs. 151065 test-amd64-amd64-libvirt-xsm 12 guest-start fail REGR. vs. 151065 test-amd64-amd64-libvirt 12 guest-start fail REGR. vs. 151065 test-amd64-i386-freebsd10-amd64 11 guest-start fail REGR. vs. 151065 test-amd64-i386-libvirt 12 guest-start fail REGR. vs. 151065 test-amd64-amd64-libvirt-pair 21 guest-start/debian fail REGR. vs. 151065 test-armhf-armhf-xl-vhd 10 debian-di-installfail REGR. vs. 151065 test-arm64-arm64-libvirt-xsm 12 guest-start fail REGR. vs. 151065 test-armhf-armhf-libvirt-raw 10 debian-di-installfail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-ws16-amd64 10 windows-install fail REGR. vs. 151065 test-armhf-armhf-libvirt 12 guest-start fail REGR. vs. 151065 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-pvshim12 guest-start fail 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-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-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 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-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 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-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-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 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-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
[xen-unstable-smoke test] 151515: tolerable all pass - PUSHED
flight 151515 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/151515/ 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 0dbed3ad3366734fd23ee3fd1f9989c8c96b6052 baseline version: xen 3b7dab93f2401b08c673244c9ae0f92e08bd03ba Last test of basis 151511 2020-07-01 17:01:00 Z0 days Testing same since 151515 2020-07-01 21:04:44 Z0 days1 attempts People who touched revisions under test: Stefano Stabellini Volodymyr Babchuk 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 3b7dab93f2..0dbed3ad33 0dbed3ad3366734fd23ee3fd1f9989c8c96b6052 -> smoke
[Xen ARM64] Save coredump log when xen/dom0 crash on ARM64?
Hello xen experts: Is there any way to save xen and dom0 core dump log when xen or dom0 crash on ARM64 platform? I find that the kdump seems can'trun on ARM64 platform? Are there anypatchesor other way to achive this goal? Thank you very much!
[qemu-mainline bisection] complete test-amd64-i386-libvirt-xsm
branch xen-unstable xenbranch xen-unstable job test-amd64-i386-libvirt-xsm testid guest-start Tree: libvirt git://xenbits.xen.org/libvirt.git Tree: libvirt_keycodemapdb https://gitlab.com/keycodemap/keycodemapdb.git Tree: linux git://xenbits.xen.org/linux-pvops.git Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git Tree: qemuu git://git.qemu.org/qemu.git Tree: seabios git://xenbits.xen.org/osstest/seabios.git Tree: xen git://xenbits.xen.org/xen.git *** Found and reproduced problem changeset *** Bug is in tree: qemuu git://git.qemu.org/qemu.git Bug introduced: 81cb05732efb36971901c515b007869cc1d3a532 Bug not present: d6b78ac8ecf94f56dbfbecc23fb4365d8772a41a Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/151514/ commit 81cb05732efb36971901c515b007869cc1d3a532 Author: Markus Armbruster Date: Tue Jun 9 14:23:37 2020 +0200 qdev: Assert devices are plugged into a bus that can take them This would have caught some of the bugs I just fixed. Signed-off-by: Markus Armbruster Reviewed-by: Mark Cave-Ayland Message-Id: <20200609122339.937862-23-arm...@redhat.com> For bisection revision-tuple graph see: http://logs.test-lab.xenproject.org/osstest/results/bisect/qemu-mainline/test-amd64-i386-libvirt-xsm.guest-start.html Revision IDs in each graph node refer, respectively, to the Trees above. Running cs-bisection-step --graph-out=/home/logs/results/bisect/qemu-mainline/test-amd64-i386-libvirt-xsm.guest-start --summary-out=tmp/151514.bisection-summary --basis-template=151065 --blessings=real,real-bisect qemu-mainline test-amd64-i386-libvirt-xsm guest-start Searching for failure / basis pass: 151500 fail [host=huxelrebe0] / 151149 [host=huxelrebe1] 151101 [host=albana0] 151065 [host=albana1] 151047 [host=fiano0] 150970 [host=fiano1] 150930 [host=elbling1] 150916 [host=chardonnay0] 150895 [host=elbling0] 150831 [host=pinot0] 150694 [host=rimava1] 150631 [host=debina1] 150608 [host=pinot1] 150593 [host=italia0] 150585 [host=chardonnay1] 150532 [host=debina0] 150492 [host=fiano0] 150457 ok. Failure / basis pass flights: 151500 / 150457 (tree with no url: minios) (tree in basispass but not in latest: libvirt_gnulib) Tree: libvirt git://xenbits.xen.org/libvirt.git Tree: libvirt_keycodemapdb https://gitlab.com/keycodemap/keycodemapdb.git Tree: linux git://xenbits.xen.org/linux-pvops.git Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git Tree: qemuu git://git.qemu.org/qemu.git Tree: seabios git://xenbits.xen.org/osstest/seabios.git Tree: xen git://xenbits.xen.org/xen.git Latest 4268e187531eb370bc6fbac4496018bb7fef6716 27acf0ef828bf719b2053ba398b195829413dbdd c3038e718a19fc596f7b1baba0f83d5146dc7784 c530a75c1e6a472b0eb9558310b518f0dfcd8860 00217f1919270007d7a911f89b32e39b9dcaa907 3c659044118e34603161457db9934a34f816d78b fc1bff958998910ec8d25db86cd2f53ff125f7ab 88ab0c15525ced2eefe39220742efe4769089ad8 da53345dd5ff7d3a34e83587fd375c0b7722f46c Basis pass a1cd25b919509be2645dbe6f952d5263e0d4e4e5 317d3eeb963a515e15a63fa356d8ebcda7041a51 c3038e718a19fc596f7b1baba0f83d5146dc7784 c530a75c1e6a472b0eb9558310b518f0dfcd8860 568eee7cf319fa95183c8d3b5e8dcf6e078ab8b3 3c659044118e34603161457db9934a34f816d78b a20ab81d22300cca80325c284f21eefee99aa740 2e3de6253422112ae43e608661ba94ea6b345694 9f3e9139fa6c3d620eb08dff927518fc88200b8d Generating revisions with ./adhoc-revtuple-generator git://xenbits.xen.org/libvirt.git#a1cd25b919509be2645dbe6f952d5263e0d4e4e5-4268e187531eb370bc6fbac4496018bb7fef6716 https://gitlab.com/keycodemap/keycodemapdb.git#317d3eeb963a515e15a63fa356d8ebcda7041a51-27acf0ef828bf719b2053ba398b195829413dbdd git://xenbits.xen.org/linux-pvops.git#c3038e718a19fc596f7b1baba0f83d5146dc7784-c3038e718a19fc596f7b1baba0f83d5146dc7784 git://xenbits.xen.org/osstest/linux-firmware.git#c530a75c1e6a472b0eb9558310b518f0\ dfcd8860-c530a75c1e6a472b0eb9558310b518f0dfcd8860 git://xenbits.xen.org/osstest/ovmf.git#568eee7cf319fa95183c8d3b5e8dcf6e078ab8b3-00217f1919270007d7a911f89b32e39b9dcaa907 git://xenbits.xen.org/qemu-xen-traditional.git#3c659044118e34603161457db9934a34f816d78b-3c659044118e34603161457db9934a34f816d78b git://git.qemu.org/qemu.git#a20ab81d22300cca80325c284f21eefee99aa740-fc1bff958998910ec8d25db86cd2f53ff125f7ab git://xenbits.xen.org/osstest/seabios.git#2e3de6253422112ae43e608661ba94ea6b345694-88ab0c1\ 5525ced2eefe39220742efe4769089ad8 git://xenbits.xen.org/xen.git#9f3e9139fa6c3d620eb08dff927518fc88200b8d-da53345dd5ff7d3a34e83587fd375c0b7722f46c Auto packing the repository in background for optimum performance. See "git help gc" for manual housekeeping. error: The last gc run reported the following. Please
RE: [PATCH 2/2] xen: cleanup unrealized flash devices
> -Original Message-
> From: Philippe Mathieu-Daudé
> Sent: 30 June 2020 18:27
> To: p...@xen.org; xen-devel@lists.xenproject.org; qemu-de...@nongnu.org
> Cc: 'Eduardo Habkost' ; 'Michael S. Tsirkin'
> ; 'Paul Durrant'
> ; 'Jason Andryuk' ; 'Paolo Bonzini'
> ;
> 'Richard Henderson'
> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>
> On 6/30/20 5:44 PM, Paul Durrant wrote:
> >> -Original Message-
> >> From: Philippe Mathieu-Daudé
> >> Sent: 30 June 2020 16:26
> >> To: Paul Durrant ; xen-devel@lists.xenproject.org;
> >> qemu-de...@nongnu.org
> >> Cc: Eduardo Habkost ; Michael S. Tsirkin
> >> ; Paul Durrant
> >> ; Jason Andryuk ; Paolo Bonzini
> >> ;
> >> Richard Henderson
> >> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> >>
> >> On 6/24/20 2:18 PM, Paul Durrant wrote:
> >>> From: Paul Durrant
> >>>
> >>> The generic pc_machine_initfn() calls pc_system_flash_create() which
> >>> creates
> >>> 'system.flash0' and 'system.flash1' devices. These devices are then
> >>> realized
> >>> by pc_system_flash_map() which is called from pc_system_firmware_init()
> >>> which
> >>> itself is called via pc_memory_init(). The latter however is not called
> >>> when
> >>> xen_enable() is true and hence the following assertion fails:
> >>>
> >>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> >>> Assertion `dev->realized' failed
> >>>
> >>> These flash devices are unneeded when using Xen so this patch avoids the
> >>> assertion by simply removing them using pc_system_flash_cleanup_unused().
> >>>
> >>> Reported-by: Jason Andryuk
> >>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
> >>> Signed-off-by: Paul Durrant
> >>> Tested-by: Jason Andryuk
> >>> ---
> >>> Cc: Paolo Bonzini
> >>> Cc: Richard Henderson
> >>> Cc: Eduardo Habkost
> >>> Cc: "Michael S. Tsirkin"
> >>> Cc: Marcel Apfelbaum
> >>> ---
> >>> hw/i386/pc_piix.c| 9 ++---
> >>> hw/i386/pc_sysfw.c | 2 +-
> >>> include/hw/i386/pc.h | 1 +
> >>> 3 files changed, 8 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >>> index 1497d0e4ae..977d40afb8 100644
> >>> --- a/hw/i386/pc_piix.c
> >>> +++ b/hw/i386/pc_piix.c
> >>> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
> >>> if (!xen_enabled()) {
> >>> pc_memory_init(pcms, system_memory,
> >>> rom_memory, _memory);
> >>> -} else if (machine->kernel_filename != NULL) {
> >>> -/* For xen HVM direct kernel boot, load linux here */
> >>> -xen_load_linux(pcms);
> >>> +} else {
> >>> +pc_system_flash_cleanup_unused(pcms);
> >>
> >> TIL pc_system_flash_cleanup_unused().
> >>
> >> What about restricting at the source?
> >>
> >
> > And leave the devices in place? They are not relevant for Xen, so why not
> > clean up?
>
> No, I meant to not create them in the first place, instead of
> create+destroy.
>
> Anyway what you did works, so I don't have any problem.
IIUC Jason originally tried restricting creation but encountered a problem
because xen_enabled() would always return false at that point, because machine
creation occurs before accelerators are initialized.
Paul
>
> Reviewed-by: Philippe Mathieu-Daudé
>
> >
> > Paul
> >
> >> -- >8 --
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -1004,24 +1004,26 @@ void pc_memory_init(PCMachineState *pcms,
> >> >device_memory->mr);
> >> }
> >>
> >> -/* Initialize PC system firmware */
> >> -pc_system_firmware_init(pcms, rom_memory);
> >> -
> >> -option_rom_mr = g_malloc(sizeof(*option_rom_mr));
> >> -memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
> >> - _fatal);
> >> -if (pcmc->pci_enabled) {
> >> -memory_region_set_readonly(option_rom_mr, true);
> >> -}
> >> -memory_region_add_subregion_overlap(rom_memory,
> >> -PC_ROM_MIN_VGA,
> >> -option_rom_mr,
> >> -1);
> >> -
> >> fw_cfg = fw_cfg_arch_create(machine,
> >> x86ms->boot_cpus, x86ms->apic_id_limit);
> >>
> >> -rom_set_fw(fw_cfg);
> >> +/* Initialize PC system firmware */
> >> +if (!xen_enabled()) {
> >> +pc_system_firmware_init(pcms, rom_memory);
> >> +
> >> +option_rom_mr = g_malloc(sizeof(*option_rom_mr));
> >> +memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
> >> + _fatal);
> >> +if (pcmc->pci_enabled) {
> >> +memory_region_set_readonly(option_rom_mr, true);
> >> +}
> >> +memory_region_add_subregion_overlap(rom_memory,
> >> +PC_ROM_MIN_VGA,
> >> +option_rom_mr,
> >> +
[qemu-mainline test] 151485: regressions - FAIL
flight 151485 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/151485/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-win7-amd64 10 windows-install fail REGR. vs. 151065 test-amd64-amd64-libvirt-vhd 10 debian-di-installfail REGR. vs. 151065 test-amd64-i386-xl-qemuu-win7-amd64 10 windows-install fail REGR. vs. 151065 test-amd64-amd64-qemuu-nested-intel 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-libvirt-xsm 12 guest-start fail REGR. vs. 151065 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-qemuu-nested-amd 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-xl-qcow210 debian-di-installfail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-qemuu-rhel6hvm-amd 10 redhat-install fail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-xl-qemuu-ws16-amd64 10 windows-install fail REGR. vs. 151065 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-libvirt-pair 21 guest-start/debian fail REGR. vs. 151065 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-freebsd10-i386 11 guest-startfail REGR. vs. 151065 test-amd64-i386-qemuu-rhel6hvm-intel 10 redhat-install fail REGR. vs. 151065 test-amd64-amd64-libvirt 12 guest-start fail REGR. vs. 151065 test-amd64-amd64-libvirt-xsm 12 guest-start fail REGR. vs. 151065 test-amd64-i386-freebsd10-amd64 11 guest-start fail REGR. vs. 151065 test-amd64-i386-libvirt 12 guest-start fail REGR. vs. 151065 test-amd64-amd64-libvirt-pair 21 guest-start/debian fail REGR. vs. 151065 test-armhf-armhf-xl-vhd 10 debian-di-installfail REGR. vs. 151065 test-arm64-arm64-libvirt-xsm 12 guest-start fail REGR. vs. 151065 test-armhf-armhf-libvirt-raw 10 debian-di-installfail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-ws16-amd64 10 windows-install fail REGR. vs. 151065 test-armhf-armhf-libvirt 12 guest-start fail REGR. vs. 151065 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-pvshim12 guest-start fail 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-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-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-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-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 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 13 migrate-support-checkfail never pass test-arm64-arm64-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-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 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
[PATCH v2] xen/displif: Protocol version 2
From: Oleksandr Andrushchenko
1. Add protocol version as an integer
Version string, which is in fact an integer, is hard to handle in the
code that supports different protocol versions. To simplify that
also add the version as an integer.
2. Pass buffer offset with XENDISPL_OP_DBUF_CREATE
There are cases when display data buffer is created with non-zero
offset to the data start. Handle such cases and provide that offset
while creating a display buffer.
3. Add XENDISPL_OP_GET_EDID command
Add an optional request for reading Extended Display Identification
Data (EDID) structure which allows better configuration of the
display connectors over the configuration set in XenStore.
With this change connectors may have multiple resolutions defined
with respect to detailed timing definitions and additional properties
normally provided by displays.
If this request is not supported by the backend then visible area
is defined by the relevant XenStore's "resolution" property.
If backend provides extended display identification data (EDID) with
XENDISPL_OP_GET_EDID request then EDID values must take precedence
over the resolutions defined in XenStore.
4. Bump protocol version to 2.
Signed-off-by: Oleksandr Andrushchenko
---
xen/include/public/io/displif.h | 91 +++--
1 file changed, 88 insertions(+), 3 deletions(-)
diff --git a/xen/include/public/io/displif.h b/xen/include/public/io/displif.h
index cc5de9cb1f35..0055895510f7 100644
--- a/xen/include/public/io/displif.h
+++ b/xen/include/public/io/displif.h
@@ -38,7 +38,8 @@
* Protocol version
**
*/
-#define XENDISPL_PROTOCOL_VERSION "1"
+#define XENDISPL_PROTOCOL_VERSION "2"
+#define XENDISPL_PROTOCOL_VERSION_INT 2
/*
**
@@ -202,6 +203,9 @@
* Width and height of the connector in pixels separated by
* XENDISPL_RESOLUTION_SEPARATOR. This defines visible area of the
* display.
+ * If backend provides extended display identification data (EDID) with
+ * XENDISPL_OP_GET_EDID request then EDID values must take precedence
+ * over the resolutions defined here.
*
*-- Connector Request Transport Parameters ---
*
@@ -349,6 +353,8 @@
#define XENDISPL_OP_FB_DETACH 0x13
#define XENDISPL_OP_SET_CONFIG0x14
#define XENDISPL_OP_PG_FLIP 0x15
+/* The below command is available in protocol version 2 and above. */
+#define XENDISPL_OP_GET_EDID 0x16
/*
**
@@ -377,6 +383,10 @@
#define XENDISPL_FIELD_BE_ALLOC "be-alloc"
#define XENDISPL_FIELD_UNIQUE_ID "unique-id"
+#define XENDISPL_EDID_BLOCK_SIZE 128
+#define XENDISPL_EDID_BLOCK_COUNT 256
+#define XENDISPL_EDID_MAX_SIZE(XENDISPL_EDID_BLOCK_SIZE *
XENDISPL_EDID_BLOCK_COUNT)
+
/*
**
* STATUS RETURN CODES
@@ -451,7 +461,9 @@
* +++++
* | gref_directory | 40
* +++++
- * | reserved | 44
+ * | data_ofs | 44
+ * +++++
+ * | reserved | 48
* +++++
* |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
* +++++
@@ -494,6 +506,7 @@
* buffer size (buffer_sz) exceeds what can be addressed by this single page,
* then reference to the next page must be supplied (see gref_dir_next_page
* below)
+ * data_ofs - uint32_t, offset of the data in the buffer, octets
*/
#define XENDISPL_DBUF_FLG_REQ_ALLOC (1 << 0)
@@ -506,6 +519,7 @@ struct xendispl_dbuf_create_req {
uint32_t buffer_sz;
uint32_t flags;
grant_ref_t gref_directory;
+uint32_t data_ofs;
};
/*
@@ -731,6 +745,44 @@ struct xendispl_page_flip_req {
uint64_t fb_cookie;
};
+/*
+ * Request EDID - request EDID describing current connector:
+ * 01 2 3octet
+ * +++++
+ * | id| _OP_GET_EDID | reserved | 4
+ * +++++
+ * | buffer_sz | 8
+ *
[xen-unstable test] 151506: tolerable FAIL
flight 151506 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/151506/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-amd64-qemuu-nested-amd 14 xen-boot/l1 fail pass in 151491 Tests which did not succeed, but are not blocking: test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail in 151491 never pass test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10 fail like 151491 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 151491 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 151491 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 151491 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 151491 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 151491 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 151491 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 151491 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 151491 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 151491 test-amd64-i386-xl-pvshim12 guest-start fail 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-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-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 13 migrate-support-checkfail never pass test-arm64-arm64-xl 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-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 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-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-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-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-amd64-amd64-libvirt-vhd 12 migrate-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-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 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 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail 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-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-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-libvirt-raw 12 migrate-support-checkfail never pass version targeted for testing: xen 23ca7ec0ba620db52a646d80e22f9703a6589f66 baseline version: xen 23ca7ec0ba620db52a646d80e22f9703a6589f66 Last test of basis 151506 2020-07-01 10:55:16 Z0 days Testing same since (not found) 0 attempts jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm
Re: [PATCH 2/2] xen: cleanup unrealized flash devices
Jason Andryuk writes:
> On Wed, Jul 1, 2020 at 3:03 AM Paul Durrant wrote:
>>
>> > -Original Message-
>> > From: Philippe Mathieu-Daudé
>> > Sent: 30 June 2020 18:27
>> > To: p...@xen.org; xen-devel@lists.xenproject.org; qemu-de...@nongnu.org
>> > Cc: 'Eduardo Habkost' ; 'Michael S. Tsirkin'
>> > ; 'Paul Durrant'
>> > ; 'Jason Andryuk' ; 'Paolo
>> > Bonzini' ;
>> > 'Richard Henderson'
>> > Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>> >
>> > On 6/30/20 5:44 PM, Paul Durrant wrote:
>> > >> -Original Message-
>> > >> From: Philippe Mathieu-Daudé
>> > >> Sent: 30 June 2020 16:26
>> > >> To: Paul Durrant ; xen-devel@lists.xenproject.org;
>> > >> qemu-de...@nongnu.org
>> > >> Cc: Eduardo Habkost ; Michael S. Tsirkin
>> > >> ; Paul Durrant
>> > >> ; Jason Andryuk ; Paolo
>> > >> Bonzini ;
>> > >> Richard Henderson
>> > >> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>> > >>
>> > >> On 6/24/20 2:18 PM, Paul Durrant wrote:
>> > >>> From: Paul Durrant
>> > >>>
>> > >>> The generic pc_machine_initfn() calls pc_system_flash_create() which
>> > >>> creates
>> > >>> 'system.flash0' and 'system.flash1' devices. These devices are then
>> > >>> realized
>> > >>> by pc_system_flash_map() which is called from
>> > >>> pc_system_firmware_init() which
>> > >>> itself is called via pc_memory_init(). The latter however is not
>> > >>> called when
>> > >>> xen_enable() is true and hence the following assertion fails:
>> > >>>
>> > >>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
>> > >>> Assertion `dev->realized' failed
>> > >>>
>> > >>> These flash devices are unneeded when using Xen so this patch avoids
>> > >>> the
>> > >>> assertion by simply removing them using
>> > >>> pc_system_flash_cleanup_unused().
>> > >>>
>> > >>> Reported-by: Jason Andryuk
>> > >>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with
>> > >>> -blockdev")
>> > >>> Signed-off-by: Paul Durrant
>> > >>> Tested-by: Jason Andryuk
>> > >>> ---
>> > >>> Cc: Paolo Bonzini
>> > >>> Cc: Richard Henderson
>> > >>> Cc: Eduardo Habkost
>> > >>> Cc: "Michael S. Tsirkin"
>> > >>> Cc: Marcel Apfelbaum
>> > >>> ---
>> > >>> hw/i386/pc_piix.c| 9 ++---
>> > >>> hw/i386/pc_sysfw.c | 2 +-
>> > >>> include/hw/i386/pc.h | 1 +
>> > >>> 3 files changed, 8 insertions(+), 4 deletions(-)
>> > >>>
>> > >>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> > >>> index 1497d0e4ae..977d40afb8 100644
>> > >>> --- a/hw/i386/pc_piix.c
>> > >>> +++ b/hw/i386/pc_piix.c
>> > >>> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
>> > >>> if (!xen_enabled()) {
>> > >>> pc_memory_init(pcms, system_memory,
>> > >>> rom_memory, _memory);
>> > >>> -} else if (machine->kernel_filename != NULL) {
>> > >>> -/* For xen HVM direct kernel boot, load linux here */
>> > >>> -xen_load_linux(pcms);
>> > >>> +} else {
>> > >>> +pc_system_flash_cleanup_unused(pcms);
>> > >>
>> > >> TIL pc_system_flash_cleanup_unused().
>> > >>
>> > >> What about restricting at the source?
>> > >>
>> > >
>> > > And leave the devices in place? They are not relevant for Xen, so why
>> > > not clean up?
>> >
>> > No, I meant to not create them in the first place, instead of
>> > create+destroy.
>> >
>> > Anyway what you did works, so I don't have any problem.
>>
>> IIUC Jason originally tried restricting creation but encountered a problem
>> because xen_enabled() would always return false at that point, because
>> machine creation occurs before accelerators are initialized.
>
> Correct. Quoting my previous email:
> """
> Removing the call to pc_system_flash_create() from pc_machine_initfn()
> lets QEMU startup and run a Xen HVM again. xen_enabled() doesn't work
> there since accelerators have not been initialized yes, I guess?
> """
>
> If you want to remove the creation in the first place, then I have two
> questions. Why does pc_system_flash_create()/pc_pflash_create() get
> called so early creating the pflash devices? Why aren't they just
> created as needed in pc_system_flash_map()?
commit ebc29e1beab02646702c8cb9a1d29b68f72ad503
pc: Support firmware configuration with -blockdev
[...]
Properties need to be created in .instance_init() methods. For PC
machines, that's pc_machine_initfn(). To make alias properties work,
we need to create the onboard flash devices there, too. [...]
For context, read the entire commit message. If you have questions
then, don't hesitate to ask them here.
[xen-unstable-smoke test] 151511: tolerable all pass - PUSHED
flight 151511 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/151511/ 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 3b7dab93f2401b08c673244c9ae0f92e08bd03ba baseline version: xen 23ca7ec0ba620db52a646d80e22f9703a6589f66 Last test of basis 151476 2020-06-30 11:00:51 Z1 days Testing same since 151511 2020-07-01 17:01:00 Z0 days1 attempts People who touched revisions under test: Andrew Cooper 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 23ca7ec0ba..3b7dab93f2 3b7dab93f2401b08c673244c9ae0f92e08bd03ba -> smoke
Re: [PATCH] xen: introduce xen_vring_use_dma
On Wed, Jul 01, 2020 at 10:34:53AM -0700, Stefano Stabellini wrote: > On Wed, 1 Jul 2020, Christoph Hellwig wrote: > > On Mon, Jun 29, 2020 at 04:46:09PM -0700, Stefano Stabellini wrote: > > > > I could imagine some future Xen hosts setting a flag somewhere in the > > > > platform capability saying "no xen specific flag, rely on > > > > "VIRTIO_F_ACCESS_PLATFORM". Then you set that accordingly in QEMU. > > > > How about that? > > > > > > Yes, that would be fine and there is no problem implementing something > > > like that when we get virtio support in Xen. Today there are still no > > > virtio interfaces provided by Xen to ARM guests (no virtio-block/net, > > > etc.) > > > > > > In fact, in both cases we are discussing virtio is *not* provided by > > > Xen; it is a firmware interface to something entirely different: > > > > > > 1) virtio is used to talk to a remote AMP processor (RPMesg) > > > 2) virtio is used to talk to a secure-world firmware/OS (Trusty) > > > > > > VIRTIO_F_ACCESS_PLATFORM is not set by Xen in these cases but by RPMesg > > > and by Trusty respectively. I don't know if Trusty should or should not > > > set VIRTIO_F_ACCESS_PLATFORM, but I think Linux should still work > > > without issues. > > > > > > > Any virtio implementation that is not in control of the memory map > > (aka not the hypervisor) absolutely must set VIRTIO_F_ACCESS_PLATFORM, > > else it is completely broken. > > Lots of broken virtio implementations out there it would seem :-( Not really, most of virtio implementations are in full control of memory, being part of the hypervisor. > > > > The xen_domain() check in Linux makes it so that vring_use_dma_api > > > returns the opposite value on native Linux compared to Linux as Xen/ARM > > > DomU by "accident". By "accident" because there is no architectural > > > reason why Linux Xen/ARM DomU should behave differently compared to > > > native Linux in this regard. > > > > > > I hope that now it is clearer why I think the if (xen_domain()) check > > > needs to be improved anyway, even if we fix generic dma_ops with virtio > > > interfaces missing VIRTIO_F_ACCESS_PLATFORM. > > > > IMHO that Xen quirk should never have been added in this form.. > > Would you be in favor of a more flexible check along the lines of the > one proposed in the patch that started this thread: > > if (xen_vring_use_dma()) > return true; > > > xen_vring_use_dma would be implemented so that it returns true when > xen_swiotlb is required and false otherwise. I'll need to think about it. Sounds reasonable on the surface ... -- MST
Re: [XEN PATCH] hvmloader: Fix reading ACPI PM1 CNT value
On Tue, Jun 30, 2020 at 06:09:13PM +0100, Anthony PERARD wrote:
> In order to get the CNT value from QEMU, we were supposed to read a
> word, according to the implementation in QEMU. But it has been lax and
> allowed to read a single byte. This has changed with commit
> 5d971f9e6725 ("memory: Revert "memory: accept mismatching sizes in
> memory_region_access_valid"") and result in hvmloader crashing on
> the BUG_ON.
This is a bug on the QEMU side, the ACPI spec states: "Accesses to PM1
control registers are accessed through byte and word accesses.".
That's on section 4.8.3.2.1 PM1 Control Registers of my copy of the
ACPI spec (6.2A).
>
> Signed-off-by: Anthony PERARD
I'm fine with this if such bogus behavior has made it's way into a
release version of QEMU, but it needs to state it's a workaround for a
QEMU bug, not a bug in hvmloader.
IMO the QEMU change should be reverted.
Thanks, Roger.
vPT rework (and timer mode)
Hello, I've been doing some work with the virtual timers infrastructure in order to improve some of it's shortcomings. See: https://lists.xenproject.org/archives/html/xen-devel/2020-06/msg00919.html For an example of such issues, and how the emulated timers are not architecturally correct. It's my understanding that the purpose of pt_update_irq and pt_intr_post is to attempt to implement the "delay for missed ticks" mode, where Xen will accumulate timer interrupts if they cannot be injected. As shown by the patch above, this is all broken when the timer is added to a vCPU (pt->vcpu) different than the actual target vCPU where the interrupt gets delivered (note this can also be a list of vCPUs if routed from the IO-APIC using Fixed mode). I'm at lost at how to fix this so that virtual timers work properly and we also keep the "delay for missed ticks" mode without doing a massive rework and somehow keeping track of where injected interrupts originated, which seems an overly complicated solution. My proposal hence would be to completely remove the timer_mode, and just treat virtual timer interrupts as other interrupts, ie: they will be injected from the callback (pt_timer_fn) and the vCPU(s) would be kicked. Whether interrupts would get lost (ie: injected when a previous one is still pending) depends on the contention on the system. I'm not aware of any current OS that uses timer interrupts as a way to track time. I think current OSes know the differences between a timer counter and an event timer, and will use them appropriately. This would allow to get rid of pt_update_irq and pt_intr_post calls in the VMX/SVM interrupt injection paths, and likely simplify the virtual timers code quite a lot. Note the guest would also always track the real wallclock. AFAICT such change would also allow to get rid of the per-vCPU vpt lists. Wanted to get some feedback on this approach before starting to do the work, since as said above it will involve dropping the timer modes. Thanks, Roger.
Re: [XEN PATCH] hvmloader: Fix reading ACPI PM1 CNT value
On Wed, Jul 01, 2020 at 09:52:57AM +0200, Roger Pau Monné wrote:
> On Tue, Jun 30, 2020 at 06:09:13PM +0100, Anthony PERARD wrote:
> > In order to get the CNT value from QEMU, we were supposed to read a
> > word, according to the implementation in QEMU. But it has been lax and
> > allowed to read a single byte. This has changed with commit
> > 5d971f9e6725 ("memory: Revert "memory: accept mismatching sizes in
> > memory_region_access_valid"") and result in hvmloader crashing on
> > the BUG_ON.
>
> This is a bug on the QEMU side, the ACPI spec states: "Accesses to PM1
> control registers are accessed through byte and word accesses.".
> That's on section 4.8.3.2.1 PM1 Control Registers of my copy of the
> ACPI spec (6.2A).
I guess we can ignore this patch then, and I should write a patch for
QEMU instead.
> I'm fine with this if such bogus behavior has made it's way into a
> release version of QEMU, but it needs to state it's a workaround for a
> QEMU bug, not a bug in hvmloader.
It hasn't, but might.
> IMO the QEMU change should be reverted.
The change can't be reverted, it is to fix a CVE and isn't related to
ACPI. But we can fix the emulator.
> Thanks, Roger.
Thanks,
--
Anthony PERARD
