[PATCH 00/16] x86: indirect call overhead reduction

[PATCH 00/16] x86: indirect call overhead reduction

[Jan Beulich]

While indirect calls have always been more expensive than direct ones,
their cost has further increased with the Spectre v2 mitigations. In a
number of cases we simply pointlessly use them in the first place. In
many other cases the indirection solely exists to abstract from e.g.
vendor specific hardware details, and hence the pointers used never
change once set. Here we can use alternatives patching to get rid of
the indirection.

From patch 8 onwards dependencies exist on earlier, yet to be reviewed
patches ("x86/alternatives: fully leverage automatic NOP filling" as well
as the "x86: improve PDX <-> PFN and alike translations" series at the
very least). I nevertheless wanted to enable a first round of review of
the series, the more that some of the patches (not just initial ones)
could perhaps be taken irrespective of those dependencies.

Further areas where indirect calls could be eliminated (and that I've put
on my todo list in case the general concept here is deemed reasonable)
are IOMMU, cpufreq, vPMU, and XSM. For some of these, the ARM side
would need dealing with as well - I'm not sure whether replacing indirect
calls by direct ones is worthwhile there as well; if not, the wrappers
would simply need to become function invocations in the ARM case.

01: VMX: reduce number of posted-interrupt hooks
02: VMX: don't unconditionally set the tsc_scaling.setup hook
03: x86/HVM: switch virtual_intr_delivery_enabled() hook to simple boolean
04: x86/HVM: drop vmfunc_intercept
05: x86/HVM: add wrapper for hvm_funcs.set_tsc_offset()
06: x86: allow producing .i or .s for multiply compiled files
07: x86/shadow: fetch CPL just once in sh_page_fault()
08: x86/alternatives: allow using assembler macros in favor of C ones
09: x86: infrastructure to allow converting certain indirect calls to direct ones
10: x86/HVM: patch indirect calls through hvm_funcs to direct ones
11: x86/HVM: patch vINTR indirect calls through hvm_funcs to direct ones
12: x86: patch ctxt_switch_masking() indirect call to direct one
13: x86/genapic: drop .target_cpus() hook
14: x86/genapic: remove indirection from genapic hook accesses
15: x86/genapic: patch indirect calls to direct ones
16: x86/cpuidle: patch some indirect calls to direct ones

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 01/16] VMX: reduce number of posted-interrupt hooks

[Jan Beulich]

Three of the four hooks are not exposed outside of vmx.c, and all of
them have only a single possible non-NULL value. So there's no reason to
use hooks here - a simple set of flag indicators is sufficient (and we
don't even need a flag for the VM entry one, as it's always
(de-)activated together the the vCPU blocking hook, which needs to
remain an actual function pointer). This is the more that with the
Spectre v2 workarounds indirect calls have become more expensive.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -78,6 +78,10 @@ static int vmx_msr_write_intercept(unsig
 static void vmx_invlpg(struct vcpu *v, unsigned long vaddr);
 static int vmx_vmfunc_intercept(struct cpu_user_regs *regs);
 
+/* Values for domain's ->arch.hvm_domain.pi_ops.flags. */
+#define PI_CSW_FROM (1u << 0)
+#define PI_CSW_TO   (1u << 1)
+
 struct vmx_pi_blocking_vcpu {
     struct list_head     list;
     spinlock_t           lock;
@@ -330,8 +334,7 @@ void vmx_pi_hooks_assign(struct domain *
      * This can make sure the PI (especially the NDST feild) is
      * in proper state when we call vmx_vcpu_block().
      */
-    d->arch.hvm_domain.pi_ops.switch_from = vmx_pi_switch_from;
-    d->arch.hvm_domain.pi_ops.switch_to = vmx_pi_switch_to;
+    d->arch.hvm_domain.pi_ops.flags = PI_CSW_FROM | PI_CSW_TO;
 
     for_each_vcpu ( d, v )
     {
@@ -347,7 +350,6 @@ void vmx_pi_hooks_assign(struct domain *
     }
 
     d->arch.hvm_domain.pi_ops.vcpu_block = vmx_vcpu_block;
-    d->arch.hvm_domain.pi_ops.do_resume = vmx_pi_do_resume;
 }
 
 /* This function is called when pcidevs_lock is held */
@@ -384,8 +386,7 @@ void vmx_pi_hooks_deassign(struct domain
      * 'switch_to' hook function.
      */
     d->arch.hvm_domain.pi_ops.vcpu_block = NULL;
-    d->arch.hvm_domain.pi_ops.switch_from = NULL;
-    d->arch.hvm_domain.pi_ops.do_resume = NULL;
+    d->arch.hvm_domain.pi_ops.flags = PI_CSW_FROM;
 
     for_each_vcpu ( d, v )
         vmx_pi_unblock_vcpu(v);
@@ -929,8 +930,8 @@ static void vmx_ctxt_switch_from(struct
     vmx_restore_host_msrs();
     vmx_save_dr(v);
 
-    if ( v->domain->arch.hvm_domain.pi_ops.switch_from )
-        v->domain->arch.hvm_domain.pi_ops.switch_from(v);
+    if ( v->domain->arch.hvm_domain.pi_ops.flags & PI_CSW_FROM )
+        vmx_pi_switch_from(v);
 }
 
 static void vmx_ctxt_switch_to(struct vcpu *v)
@@ -938,8 +939,8 @@ static void vmx_ctxt_switch_to(struct vc
     vmx_restore_guest_msrs(v);
     vmx_restore_dr(v);
 
-    if ( v->domain->arch.hvm_domain.pi_ops.switch_to )
-        v->domain->arch.hvm_domain.pi_ops.switch_to(v);
+    if ( v->domain->arch.hvm_domain.pi_ops.flags & PI_CSW_TO )
+        vmx_pi_switch_to(v);
 }
 
 
@@ -4307,8 +4308,8 @@ bool vmx_vmenter_helper(const struct cpu
      if ( nestedhvm_vcpu_in_guestmode(curr) && vcpu_nestedhvm(curr).stale_np2m )
          return false;
 
-    if ( curr->domain->arch.hvm_domain.pi_ops.do_resume )
-        curr->domain->arch.hvm_domain.pi_ops.do_resume(curr);
+    if ( curr->domain->arch.hvm_domain.pi_ops.vcpu_block )
+        vmx_pi_do_resume(curr);
 
     if ( !cpu_has_vmx_vpid )
         goto out;
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -80,20 +80,13 @@ struct hvm_ioreq_server {
  *     and actually has a physical device assigned .
  */
 struct hvm_pi_ops {
-    /* Hook into ctx_switch_from. */
-    void (*switch_from)(struct vcpu *v);
-
-    /* Hook into ctx_switch_to. */
-    void (*switch_to)(struct vcpu *v);
+    unsigned int flags;
 
     /*
      * Hook into arch_vcpu_block(), which is called
      * from vcpu_block() and vcpu_do_poll().
      */
     void (*vcpu_block)(struct vcpu *);
-
-    /* Hook into the vmentry path. */
-    void (*do_resume)(struct vcpu *v);
 };
 
 #define MAX_NR_IOREQ_SERVERS 8





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 01/16] VMX: reduce number of posted-interrupt hooks

[Andrew Cooper]

On 11/07/18 14:23, Jan Beulich wrote:
> Three of the four hooks are not exposed outside of vmx.c, and all of
> them have only a single possible non-NULL value. So there's no reason to
> use hooks here - a simple set of flag indicators is sufficient (and we
> don't even need a flag for the VM entry one, as it's always
> (de-)activated together the the vCPU blocking hook, which needs to
> remain an actual function pointer). This is the more that with the
> Spectre v2 workarounds indirect calls have become more expensive.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 01/16] VMX: reduce number of posted-interrupt hooks

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, July 11, 2018 9:23 PM
> 
> Three of the four hooks are not exposed outside of vmx.c, and all of
> them have only a single possible non-NULL value. So there's no reason to
> use hooks here - a simple set of flag indicators is sufficient (and we
> don't even need a flag for the VM entry one, as it's always
> (de-)activated together the the vCPU blocking hook, which needs to
> remain an actual function pointer). This is the more that with the
> Spectre v2 workarounds indirect calls have become more expensive.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -78,6 +78,10 @@ static int vmx_msr_write_intercept(unsig
>  static void vmx_invlpg(struct vcpu *v, unsigned long vaddr);
>  static int vmx_vmfunc_intercept(struct cpu_user_regs *regs);
> 
> +/* Values for domain's ->arch.hvm_domain.pi_ops.flags. */
> +#define PI_CSW_FROM (1u << 0)
> +#define PI_CSW_TO   (1u << 1)
> +
>  struct vmx_pi_blocking_vcpu {
>      struct list_head     list;
>      spinlock_t           lock;
> @@ -330,8 +334,7 @@ void vmx_pi_hooks_assign(struct domain *
>       * This can make sure the PI (especially the NDST feild) is
>       * in proper state when we call vmx_vcpu_block().
>       */
> -    d->arch.hvm_domain.pi_ops.switch_from = vmx_pi_switch_from;
> -    d->arch.hvm_domain.pi_ops.switch_to = vmx_pi_switch_to;
> +    d->arch.hvm_domain.pi_ops.flags = PI_CSW_FROM | PI_CSW_TO;
> 
>      for_each_vcpu ( d, v )
>      {
> @@ -347,7 +350,6 @@ void vmx_pi_hooks_assign(struct domain *
>      }
> 
>      d->arch.hvm_domain.pi_ops.vcpu_block = vmx_vcpu_block;
> -    d->arch.hvm_domain.pi_ops.do_resume = vmx_pi_do_resume;
>  }
> 
>  /* This function is called when pcidevs_lock is held */
> @@ -384,8 +386,7 @@ void vmx_pi_hooks_deassign(struct domain
>       * 'switch_to' hook function.
>       */
>      d->arch.hvm_domain.pi_ops.vcpu_block = NULL;
> -    d->arch.hvm_domain.pi_ops.switch_from = NULL;
> -    d->arch.hvm_domain.pi_ops.do_resume = NULL;
> +    d->arch.hvm_domain.pi_ops.flags = PI_CSW_FROM;

Doesn't it bring functional change ('from' hook was cleared but now
you still get the flag set)?

Thanks
Kevin

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 01/16] VMX: reduce number of posted-interrupt hooks

[Jan Beulich]

>>> On 19.07.18 at 03:51, <kevin.tian@intel.com> wrote:
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -78,6 +78,10 @@ static int vmx_msr_write_intercept(unsig
>>  static void vmx_invlpg(struct vcpu *v, unsigned long vaddr);
>>  static int vmx_vmfunc_intercept(struct cpu_user_regs *regs);
>> 
>> +/* Values for domain's ->arch.hvm_domain.pi_ops.flags. */
>> +#define PI_CSW_FROM (1u << 0)
>> +#define PI_CSW_TO   (1u << 1)
>> +
>>  struct vmx_pi_blocking_vcpu {
>>      struct list_head     list;
>>      spinlock_t           lock;
>> @@ -330,8 +334,7 @@ void vmx_pi_hooks_assign(struct domain *
>>       * This can make sure the PI (especially the NDST feild) is
>>       * in proper state when we call vmx_vcpu_block().
>>       */
>> -    d->arch.hvm_domain.pi_ops.switch_from = vmx_pi_switch_from;
>> -    d->arch.hvm_domain.pi_ops.switch_to = vmx_pi_switch_to;
>> +    d->arch.hvm_domain.pi_ops.flags = PI_CSW_FROM | PI_CSW_TO;
>> 
>>      for_each_vcpu ( d, v )
>>      {
>> @@ -347,7 +350,6 @@ void vmx_pi_hooks_assign(struct domain *
>>      }
>> 
>>      d->arch.hvm_domain.pi_ops.vcpu_block = vmx_vcpu_block;
>> -    d->arch.hvm_domain.pi_ops.do_resume = vmx_pi_do_resume;
>>  }
>> 
>>  /* This function is called when pcidevs_lock is held */
>> @@ -384,8 +386,7 @@ void vmx_pi_hooks_deassign(struct domain
>>       * 'switch_to' hook function.
>>       */
>>      d->arch.hvm_domain.pi_ops.vcpu_block = NULL;
>> -    d->arch.hvm_domain.pi_ops.switch_from = NULL;
>> -    d->arch.hvm_domain.pi_ops.do_resume = NULL;
>> +    d->arch.hvm_domain.pi_ops.flags = PI_CSW_FROM;
> 
> Doesn't it bring functional change ('from' hook was cleared but now
> you still get the flag set)?

Urgh - of course this was meant to be PI_CSW_TO.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 02/16] VMX: don't unconditionally set the tsc_scaling.setup hook

[Jan Beulich]

Instead of checking hvm_tsc_scaling_supported inside the hook function,
install the hook only when setting state such that said predicate
becomes true.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1283,7 +1283,7 @@ static void vmx_handle_cd(struct vcpu *v
 
 static void vmx_setup_tsc_scaling(struct vcpu *v)
 {
-    if ( !hvm_tsc_scaling_supported || v->domain->arch.vtsc )
+    if ( v->domain->arch.vtsc )
         return;
 
     vmx_vmcs_enter(v);
@@ -2346,7 +2346,6 @@ static struct hvm_function_table __initd
     .altp2m_vcpu_emulate_vmfunc = vmx_vcpu_emulate_vmfunc,
     .tsc_scaling = {
         .max_ratio = VMX_TSC_MULTIPLIER_MAX,
-        .setup     = vmx_setup_tsc_scaling,
     },
 };
 
@@ -2486,7 +2485,10 @@ const struct hvm_function_table * __init
     }
 
     if ( cpu_has_vmx_tsc_scaling )
+    {
         vmx_function_table.tsc_scaling.ratio_frac_bits = 48;
+        vmx_function_table.tsc_scaling.setup = vmx_setup_tsc_scaling;
+    }
 
     if ( cpu_has_mpx && cpu_has_vmx_mpx )
     {





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 02/16] VMX: don't unconditionally set the tsc_scaling.setup hook

[Andrew Cooper]

On 11/07/18 14:23, Jan Beulich wrote:
> Instead of checking hvm_tsc_scaling_supported inside the hook function,
> install the hook only when setting state such that said predicate
> becomes true.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 02/16] VMX: don't unconditionally set the tsc_scaling.setup hook

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, July 11, 2018 9:24 PM
> 
> Instead of checking hvm_tsc_scaling_supported inside the hook function,
> install the hook only when setting state such that said predicate
> becomes true.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 03/16] x86/HVM: switch virtual_intr_delivery_enabled() hook to simple boolean

[Jan Beulich]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

This patch modifies the hvm_funcs.virtual_intr_delivery_enabled()
to become a bool variable as VMX does and SVM will simply return a
static value.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Extracted from an SVM/AVIC series patch.

--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1258,14 +1258,6 @@ void vlapic_adjust_i8259_target(struct d
     pt_adjust_global_vcpu_target(v);
 }
 
-int vlapic_virtual_intr_delivery_enabled(void)
-{
-    if ( hvm_funcs.virtual_intr_delivery_enabled )
-        return hvm_funcs.virtual_intr_delivery_enabled();
-    else
-        return 0;
-}
-
 int vlapic_has_pending_irq(struct vcpu *v)
 {
     struct vlapic *vlapic = vcpu_vlapic(v);
@@ -1278,7 +1270,7 @@ int vlapic_has_pending_irq(struct vcpu *
     if ( irr == -1 )
         return -1;
 
-    if ( vlapic_virtual_intr_delivery_enabled() &&
+    if ( hvm_funcs.virtual_intr_delivery_enabled &&
          !nestedhvm_vcpu_in_guestmode(v) )
         return irr;
 
@@ -1316,7 +1308,7 @@ int vlapic_ack_pending_irq(struct vcpu *
     int isr;
 
     if ( !force_ack &&
-         vlapic_virtual_intr_delivery_enabled() )
+         hvm_funcs.virtual_intr_delivery_enabled )
         return 1;
 
     /* If there's no chance of using APIC assist then bail now. */
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1948,11 +1948,6 @@ static void vmx_update_eoi_exit_bitmap(s
         vmx_clear_eoi_exit_bitmap(v, vector);
 }
 
-static int vmx_virtual_intr_delivery_enabled(void)
-{
-    return cpu_has_vmx_virtual_intr_delivery;
-}
-
 static void vmx_process_isr(int isr, struct vcpu *v)
 {
     unsigned long status;
@@ -2331,7 +2326,6 @@ static struct hvm_function_table __initd
     .nhvm_intr_blocked    = nvmx_intr_blocked,
     .nhvm_domain_relinquish_resources = nvmx_domain_relinquish_resources,
     .update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap,
-    .virtual_intr_delivery_enabled = vmx_virtual_intr_delivery_enabled,
     .process_isr          = vmx_process_isr,
     .deliver_posted_intr  = vmx_deliver_posted_intr,
     .sync_pir_to_irr      = vmx_sync_pir_to_irr,
@@ -2470,6 +2464,8 @@ const struct hvm_function_table * __init
         vmx_function_table.process_isr = NULL;
         vmx_function_table.handle_eoi = NULL;
     }
+    else
+        vmx_function_table.virtual_intr_delivery_enabled = true;
 
     if ( cpu_has_vmx_posted_intr_processing )
     {
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -97,6 +97,9 @@ struct hvm_function_table {
     /* Necessary hardware support for alternate p2m's? */
     bool altp2m_supported;
 
+    /* Hardware virtual interrupt delivery enable? */
+    bool virtual_intr_delivery_enabled;
+
     /* Indicate HAP capabilities. */
     unsigned int hap_capabilities;
 
@@ -195,7 +198,6 @@ struct hvm_function_table {
 
     /* Virtual interrupt delivery */
     void (*update_eoi_exit_bitmap)(struct vcpu *v, u8 vector, u8 trig);
-    int (*virtual_intr_delivery_enabled)(void);
     void (*process_isr)(int isr, struct vcpu *v);
     void (*deliver_posted_intr)(struct vcpu *v, u8 vector);
     void (*sync_pir_to_irr)(struct vcpu *v);





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 03/16] x86/HVM: switch virtual_intr_delivery_enabled() hook to simple boolean

[Andrew Cooper]

On 11/07/18 14:24, Jan Beulich wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> This patch modifies the hvm_funcs.virtual_intr_delivery_enabled()
> to become a bool variable as VMX does and SVM will simply return a
> static value.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 03/16] x86/HVM: switch virtual_intr_delivery_enabled() hook to simple boolean

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, July 11, 2018 9:25 PM
> 
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> This patch modifies the hvm_funcs.virtual_intr_delivery_enabled()
> to become a bool variable as VMX does and SVM will simply return a
> static value.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Extracted from an SVM/AVIC series patch.
> 
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1258,14 +1258,6 @@ void vlapic_adjust_i8259_target(struct d
>      pt_adjust_global_vcpu_target(v);
>  }
> 
> -int vlapic_virtual_intr_delivery_enabled(void)
> -{
> -    if ( hvm_funcs.virtual_intr_delivery_enabled )
> -        return hvm_funcs.virtual_intr_delivery_enabled();
> -    else
> -        return 0;
> -}
> -
>  int vlapic_has_pending_irq(struct vcpu *v)
>  {
>      struct vlapic *vlapic = vcpu_vlapic(v);
> @@ -1278,7 +1270,7 @@ int vlapic_has_pending_irq(struct vcpu *
>      if ( irr == -1 )
>          return -1;
> 
> -    if ( vlapic_virtual_intr_delivery_enabled() &&
> +    if ( hvm_funcs.virtual_intr_delivery_enabled &&
>           !nestedhvm_vcpu_in_guestmode(v) )
>          return irr;
> 

above is another functional change. Can you explain and if making sense
then put the fix in separate patch?

Thanks
Kevin

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 03/16] x86/HVM: switch virtual_intr_delivery_enabled() hook to simple boolean

[Jan Beulich]

>>> On 19.07.18 at 03:56, <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Wednesday, July 11, 2018 9:25 PM
>> 
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> 
>> This patch modifies the hvm_funcs.virtual_intr_delivery_enabled()
>> to become a bool variable as VMX does and SVM will simply return a
>> static value.
>> 
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Extracted from an SVM/AVIC series patch.
>> 
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -1258,14 +1258,6 @@ void vlapic_adjust_i8259_target(struct d
>>      pt_adjust_global_vcpu_target(v);
>>  }
>> 
>> -int vlapic_virtual_intr_delivery_enabled(void)
>> -{
>> -    if ( hvm_funcs.virtual_intr_delivery_enabled )
>> -        return hvm_funcs.virtual_intr_delivery_enabled();
>> -    else
>> -        return 0;
>> -}
>> -
>>  int vlapic_has_pending_irq(struct vcpu *v)
>>  {
>>      struct vlapic *vlapic = vcpu_vlapic(v);
>> @@ -1278,7 +1270,7 @@ int vlapic_has_pending_irq(struct vcpu *
>>      if ( irr == -1 )
>>          return -1;
>> 
>> -    if ( vlapic_virtual_intr_delivery_enabled() &&
>> +    if ( hvm_funcs.virtual_intr_delivery_enabled &&
>>           !nestedhvm_vcpu_in_guestmode(v) )
>>          return irr;
>> 
> 
> above is another functional change. Can you explain and if making sense
> then put the fix in separate patch?

I'm afraid I don't understand: What functional change? All the patch
does is change from a hook based approach to a flag based one, as
said in the description.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 03/16] x86/HVM: switch virtual_intr_delivery_enabled() hook to simple boolean

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, July 19, 2018 2:16 PM
> 
> >>> On 19.07.18 at 03:56, <kevin.tian@intel.com> wrote:
> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Wednesday, July 11, 2018 9:25 PM
> >>
> >> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> >>
> >> This patch modifies the hvm_funcs.virtual_intr_delivery_enabled()
> >> to become a bool variable as VMX does and SVM will simply return a
> >> static value.
> >>
> >> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> Extracted from an SVM/AVIC series patch.
> >>
> >> --- a/xen/arch/x86/hvm/vlapic.c
> >> +++ b/xen/arch/x86/hvm/vlapic.c
> >> @@ -1258,14 +1258,6 @@ void vlapic_adjust_i8259_target(struct d
> >>      pt_adjust_global_vcpu_target(v);
> >>  }
> >>
> >> -int vlapic_virtual_intr_delivery_enabled(void)
> >> -{
> >> -    if ( hvm_funcs.virtual_intr_delivery_enabled )
> >> -        return hvm_funcs.virtual_intr_delivery_enabled();
> >> -    else
> >> -        return 0;
> >> -}
> >> -
> >>  int vlapic_has_pending_irq(struct vcpu *v)
> >>  {
> >>      struct vlapic *vlapic = vcpu_vlapic(v);
> >> @@ -1278,7 +1270,7 @@ int vlapic_has_pending_irq(struct vcpu *
> >>      if ( irr == -1 )
> >>          return -1;
> >>
> >> -    if ( vlapic_virtual_intr_delivery_enabled() &&
> >> +    if ( hvm_funcs.virtual_intr_delivery_enabled &&
> >>           !nestedhvm_vcpu_in_guestmode(v) )
> >>          return irr;
> >>
> >
> > above is another functional change. Can you explain and if making sense
> > then put the fix in separate patch?
> 
> I'm afraid I don't understand: What functional change? All the patch
> does is change from a hook based approach to a flag based one, as
> said in the description.
> 

forget it. I don't know why I thought nestedhvm check was added by your
patch. :/

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Thanks
Kevin

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 04/16] x86/HVM: drop vmfunc_intercept

[Jan Beulich]

Commit a1b1572833 ("VMX: add VMFUNC leaf 0 (EPTP switching) to
emulator") needlessly introduced it, and no user has appeared since.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -76,7 +76,6 @@ static void vmx_fpu_dirty_intercept(void
 static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content);
 static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content);
 static void vmx_invlpg(struct vcpu *v, unsigned long vaddr);
-static int vmx_vmfunc_intercept(struct cpu_user_regs *regs);
 
 /* Values for domain's ->arch.hvm_domain.pi_ops.flags. */
 #define PI_CSW_FROM (1u << 0)
@@ -2312,7 +2311,6 @@ static struct hvm_function_table __initd
     .fpu_dirty_intercept  = vmx_fpu_dirty_intercept,
     .msr_read_intercept   = vmx_msr_read_intercept,
     .msr_write_intercept  = vmx_msr_write_intercept,
-    .vmfunc_intercept     = vmx_vmfunc_intercept,
     .handle_cd            = vmx_handle_cd,
     .set_info_guest       = vmx_set_info_guest,
     .set_rdtsc_exiting    = vmx_set_rdtsc_exiting,
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -176,7 +176,6 @@ struct hvm_function_table {
     void (*fpu_dirty_intercept)(void);
     int (*msr_read_intercept)(unsigned int msr, uint64_t *msr_content);
     int (*msr_write_intercept)(unsigned int msr, uint64_t msr_content);
-    int (*vmfunc_intercept)(struct cpu_user_regs *regs);
     void (*handle_cd)(struct vcpu *v, unsigned long value);
     void (*set_info_guest)(struct vcpu *v);
     void (*set_rdtsc_exiting)(struct vcpu *v, bool_t);





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 04/16] x86/HVM: drop vmfunc_intercept

[Andrew Cooper]

On 11/07/18 14:25, Jan Beulich wrote:
> Commit a1b1572833 ("VMX: add VMFUNC leaf 0 (EPTP switching) to
> emulator") needlessly introduced it, and no user has appeared since.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 04/16] x86/HVM: drop vmfunc_intercept

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, July 11, 2018 9:26 PM
> 
> Commit a1b1572833 ("VMX: add VMFUNC leaf 0 (EPTP switching) to
> emulator") needlessly introduced it, and no user has appeared since.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 05/16] x86/HVM: add wrapper for hvm_funcs.set_tsc_offset()

[Jan Beulich]

It's used in quite a few places, and hence doing so eases subsequent
adjustment to how these (indirect) calls are carried out.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/domain.c
+++ b/xen/arch/x86/hvm/domain.c
@@ -317,9 +317,9 @@ int arch_set_info_hvm_guest(struct vcpu
 
     /* Sync AP's TSC with BSP's. */
     v->arch.hvm_vcpu.cache_tsc_offset =
-        v->domain->vcpu[0]->arch.hvm_vcpu.cache_tsc_offset;
-    hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset,
-                             v->domain->arch.hvm_domain.sync_tsc);
+        d->vcpu[0]->arch.hvm_vcpu.cache_tsc_offset;
+    hvm_set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset,
+                       d->arch.hvm_domain.sync_tsc);
 
     paging_update_paging_modes(v);
 
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -417,7 +417,7 @@ static void hvm_set_guest_tsc_fixed(stru
     delta_tsc = guest_tsc - tsc;
     v->arch.hvm_vcpu.cache_tsc_offset = delta_tsc;
 
-    hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset, at_tsc);
+    hvm_set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset, at_tsc);
 }
 
 #define hvm_set_guest_tsc(v, t) hvm_set_guest_tsc_fixed(v, t, 0)
@@ -435,7 +435,7 @@ static void hvm_set_guest_tsc_adjust(str
 {
     v->arch.hvm_vcpu.cache_tsc_offset += tsc_adjust
                             - v->arch.hvm_vcpu.msr_tsc_adjust;
-    hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset, 0);
+    hvm_set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset, 0);
     v->arch.hvm_vcpu.msr_tsc_adjust = tsc_adjust;
 }
 
@@ -3934,8 +3934,8 @@ void hvm_vcpu_reset_state(struct vcpu *v
     /* Sync AP's TSC with BSP's. */
     v->arch.hvm_vcpu.cache_tsc_offset =
         v->domain->vcpu[0]->arch.hvm_vcpu.cache_tsc_offset;
-    hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset,
-                             d->arch.hvm_domain.sync_tsc);
+    hvm_set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset,
+                       d->arch.hvm_domain.sync_tsc);
 
     v->arch.hvm_vcpu.msr_tsc_adjust = 0;
 
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1082,7 +1082,7 @@ static void load_shadow_guest_state(stru
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
     }
 
-    hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset, 0);
+    hvm_set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset, 0);
 
     vvmcs_to_shadow_bulk(v, ARRAY_SIZE(vmentry_fields), vmentry_fields);
 
@@ -1288,7 +1288,7 @@ static void load_vvmcs_host_state(struct
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
     }
 
-    hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset, 0);
+    hvm_set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset, 0);
 
     set_vvmcs(v, VM_ENTRY_INTR_INFO, 0);
 }
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -2198,9 +2198,9 @@ void tsc_set_info(struct domain *d,
              * will sync their TSC to BSP's sync_tsc.
              */
             d->arch.hvm_domain.sync_tsc = rdtsc();
-            hvm_funcs.set_tsc_offset(d->vcpu[0],
-                                     d->vcpu[0]->arch.hvm_vcpu.cache_tsc_offset,
-                                     d->arch.hvm_domain.sync_tsc);
+            hvm_set_tsc_offset(d->vcpu[0],
+                               d->vcpu[0]->arch.hvm_vcpu.cache_tsc_offset,
+                               d->arch.hvm_domain.sync_tsc);
         }
     }
 
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -347,6 +347,12 @@ static inline void hvm_cpuid_policy_chan
     hvm_funcs.cpuid_policy_changed(v);
 }
 
+static inline void hvm_set_tsc_offset(struct vcpu *v, uint64_t offset,
+                                      uint64_t at_tsc)
+{
+    hvm_funcs.set_tsc_offset(v, offset, at_tsc);
+}
+
 /*
  * Called to ensure than all guest-specific mappings in a tagged TLB are 
  * flushed; does *not* flush Xen's TLB entries, and on processors without a 





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 05/16] x86/HVM: add wrapper for hvm_funcs.set_tsc_offset()

[Andrew Cooper]

On 11/07/18 14:26, Jan Beulich wrote:
> It's used in quite a few places, and hence doing so eases subsequent
> adjustment to how these (indirect) calls are carried out.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Overall, I'd say that it would be good to have wrappers for all the
functions, to make a cleaner API.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 05/16] x86/HVM: add wrapper for hvm_funcs.set_tsc_offset()

[Jan Beulich]

>>> On 13.07.18 at 14:19, <andrew.cooper3@citrix.com> wrote:
> On 11/07/18 14:26, Jan Beulich wrote:
>> It's used in quite a few places, and hence doing so eases subsequent
>> adjustment to how these (indirect) calls are carried out.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

> Overall, I'd say that it would be good to have wrappers for all the
> functions, to make a cleaner API.

I had considered that, but decided that I'd leave the ones having just
a single use site.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 06/16] x86: allow producing .i or .s for multiply compiled files

[Jan Beulich]

Since the generic pattern rules don't match those, explicit rules need
to be put in place for this to work.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/Makefile
+++ b/xen/Makefile
@@ -249,6 +249,17 @@ FORCE:
 %/: FORCE
 	$(MAKE) -f $(BASEDIR)/Rules.mk -C $* built_in.o built_in_bin.o
 
+build-intermediate = $(eval $(call build-intermediate-closure,$(1)))
+define build-intermediate-closure
+$(1): FORCE
+	$(MAKE) -f $(BASEDIR)/Rules.mk -C $$(@D) $$(@F)
+endef
+
+$(foreach base,arch/x86/mm/guest_walk_% \
+               arch/x86/mm/hap/guest_walk_%level \
+               arch/x86/mm/shadow/guest_%, \
+    $(foreach ext,o i s,$(call build-intermediate,$(base).$(ext))))
+
 kconfig := silentoldconfig oldconfig config menuconfig defconfig \
 	nconfig xconfig gconfig savedefconfig listnewconfig olddefconfig \
 	randconfig
--- a/xen/arch/x86/mm/Makefile
+++ b/xen/arch/x86/mm/Makefile
@@ -13,3 +13,9 @@ obj-y += mem_access.o
 
 guest_walk_%.o: guest_walk.c Makefile
 	$(CC) $(CFLAGS) -DGUEST_PAGING_LEVELS=$* -c $< -o $@
+
+guest_walk_%.i: guest_walk.c Makefile
+	$(CPP) $(filter-out -Wa$(comma)%,$(CFLAGS)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@
+
+guest_walk_%.s: guest_walk.c Makefile
+	$(CC) $(filter-out -Wa$(comma)%,$(CFLAGS)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@
--- a/xen/arch/x86/mm/hap/Makefile
+++ b/xen/arch/x86/mm/hap/Makefile
@@ -7,3 +7,9 @@ obj-y += nested_ept.o
 
 guest_walk_%level.o: guest_walk.c Makefile
 	$(CC) $(CFLAGS) -DGUEST_PAGING_LEVELS=$* -c $< -o $@
+
+guest_walk_%level.i: guest_walk.c Makefile
+	$(CPP) $(filter-out -Wa$(comma)%,$(CFLAGS)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@
+
+guest_walk_%level.s: guest_walk.c Makefile
+	$(CC) $(filter-out -Wa$(comma)%,$(CFLAGS)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@
--- a/xen/arch/x86/mm/shadow/Makefile
+++ b/xen/arch/x86/mm/shadow/Makefile
@@ -6,3 +6,9 @@ endif
 
 guest_%.o: multi.c Makefile
 	$(CC) $(CFLAGS) -DGUEST_PAGING_LEVELS=$* -c $< -o $@
+
+guest_%.i: multi.c Makefile
+	$(CPP) $(filter-out -Wa$(comma)%,$(CFLAGS)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@
+
+guest_%.s: multi.c Makefile
+	$(CC) $(filter-out -Wa$(comma)%,$(CFLAGS)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 06/16] x86: allow producing .i or .s for multiply compiled files

[Andrew Cooper]

On 11/07/18 14:27, Jan Beulich wrote:
> Since the generic pattern rules don't match those, explicit rules need
> to be put in place for this to work.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 07/16] x86/shadow: fetch CPL just once in sh_page_fault()

[Jan Beulich]

This isn't as much of an optimization than to avoid triggering a gcc bug
affecting 5.x ... 7.x, triggered by any asm() put inside the ad hoc
"rewalk" loop and taking as an (output?) operand a register variable
tied to %rdx (an "rdx" clobber is fine). The issue is due to an apparent
collision in register use with the modulo operation in vtlb_hash(),
which (with optimization enabled) involves a multiplication of two
64-bit values with the upper half (in %rdx) of the 128-bit result being
of interest.

Such an asm() was originally meant to be implicitly introduced into the
code when converting most indirect calls through the hvm_funcs table to
direct calls (via alternative instruction patching); that model was
switched to clobbers due to further compiler problems, but I think the
change here is worthwhile nevertheless.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -2817,6 +2817,7 @@ static int sh_page_fault(struct vcpu *v,
     uint32_t rc, error_code;
     bool walk_ok;
     int version;
+    unsigned int cpl;
     const struct npfec access = {
          .read_access = 1,
          .write_access = !!(regs->error_code & PFEC_write_access),
@@ -2967,6 +2968,8 @@ static int sh_page_fault(struct vcpu *v,
         return 0;
     }
 
+    cpl = is_pv_vcpu(v) ? (regs->ss & 3) : hvm_get_cpl(v);
+
  rewalk:
 
     error_code = regs->error_code;
@@ -3023,8 +3026,7 @@ static int sh_page_fault(struct vcpu *v,
      * If this corner case comes about accidentally, then a security-relevant
      * bug has been tickled.
      */
-    if ( !(error_code & (PFEC_insn_fetch|PFEC_user_mode)) &&
-         (is_pv_vcpu(v) ? (regs->ss & 3) : hvm_get_cpl(v)) == 3 )
+    if ( !(error_code & (PFEC_insn_fetch|PFEC_user_mode)) && cpl == 3 )
         error_code |= PFEC_implicit;
 
     /* The walk is done in a lock-free style, with some sanity check





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 07/16] x86/shadow: fetch CPL just once in sh_page_fault()

[Andrew Cooper]

On 11/07/18 14:29, Jan Beulich wrote:
> This isn't as much of an optimization than to avoid triggering a gcc bug
> affecting 5.x ... 7.x, triggered by any asm() put inside the ad hoc
> "rewalk" loop and taking as an (output?) operand a register variable
> tied to %rdx (an "rdx" clobber is fine). The issue is due to an apparent
> collision in register use with the modulo operation in vtlb_hash(),
> which (with optimization enabled) involves a multiplication of two
> 64-bit values with the upper half (in %rdx) of the 128-bit result being
> of interest.
>
> Such an asm() was originally meant to be implicitly introduced into the
> code when converting most indirect calls through the hvm_funcs table to
> direct calls (via alternative instruction patching); that model was
> switched to clobbers due to further compiler problems, but I think the
> change here is worthwhile nevertheless.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

It scares me how many compiler bugs get found with work like this.

As for the change itself, I agree that it is worthwhile irrespective of
toolchain issues.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 07/16] x86/shadow: fetch CPL just once in sh_page_fault()

[Tim Deegan]

At 07:29 -0600 on 11 Jul (1531294179), Jan Beulich wrote:
> This isn't as much of an optimization than to avoid triggering a gcc bug
> affecting 5.x ... 7.x, triggered by any asm() put inside the ad hoc
> "rewalk" loop and taking as an (output?) operand a register variable
> tied to %rdx (an "rdx" clobber is fine). The issue is due to an apparent
> collision in register use with the modulo operation in vtlb_hash(),
> which (with optimization enabled) involves a multiplication of two
> 64-bit values with the upper half (in %rdx) of the 128-bit result being
> of interest.
> 
> Such an asm() was originally meant to be implicitly introduced into the
> code when converting most indirect calls through the hvm_funcs table to
> direct calls (via alternative instruction patching); that model was
> switched to clobbers due to further compiler problems, but I think the
> change here is worthwhile nevertheless.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I don't quite follow what the compiler bug does here -- it would be nice
to say what effect it has on the final code.  In any case, the code
change is fine.

Reviewed-by: Tim Deegan <tim@xen.org>

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 07/16] x86/shadow: fetch CPL just once in sh_page_fault()

[Jan Beulich]

>>> On 11.07.18 at 15:46, <tim@xen.org> wrote:
> At 07:29 -0600 on 11 Jul (1531294179), Jan Beulich wrote:
>> This isn't as much of an optimization than to avoid triggering a gcc bug
>> affecting 5.x ... 7.x, triggered by any asm() put inside the ad hoc
>> "rewalk" loop and taking as an (output?) operand a register variable
>> tied to %rdx (an "rdx" clobber is fine). The issue is due to an apparent
>> collision in register use with the modulo operation in vtlb_hash(),
>> which (with optimization enabled) involves a multiplication of two
>> 64-bit values with the upper half (in %rdx) of the 128-bit result being
>> of interest.
>> 
>> Such an asm() was originally meant to be implicitly introduced into the
>> code when converting most indirect calls through the hvm_funcs table to
>> direct calls (via alternative instruction patching); that model was
>> switched to clobbers due to further compiler problems, but I think the
>> change here is worthwhile nevertheless.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I don't quite follow what the compiler bug does here -- it would be nice
> to say what effect it has on the final code.  In any case, the code
> change is fine.

There was no final code - it was an Internal Compiler Error.

> Reviewed-by: Tim Deegan <tim@xen.org>

Thanks, Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 08/16] x86/alternatives: allow using assembler macros in favor of C ones

[Jan Beulich]

As was validly pointed out as motivation for similar Linux side changes
(https://lkml.org/lkml/2018/6/22/677), using long sequences of
directives and auxiliary instructions, like is commonly the case when
setting up an alternative patch site, gcc can be mislead into believing
an asm() to be more heavy weight than it really is. By presenting it
with an assembler macro invocation instead, this can be avoided.

Initially I wanted to outright change the C macros ALTERNATIVE() and
ALTERNATIVE_2() to invoke the respective assembler ones, but doing so
would require quite a bit of cleanup of some use sites, because of the
exra necessary quoting combined with the need that each assembler macro
argument must consist of just a single string literal. We can consider
working towards that subsequently.

For now, set the stage of using the assembler macros here by providing a
new generated header, being the slightly massaged pre-processor output
of (for now just) alternative-asm.h. The massaging is primarily to be
able to properly track the build dependency: For this, we need the C
compiler to see the inclusion, which means we shouldn't directly use an
asm(". include ...") directive.

The dependency added to asm-offsets.s is not a true one; it's just the
easiest approach I could think of to make sure the new header gets
generated early on, without having to fiddle with xen/Makefile (and
introducing some x86-specific construct there).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/.gitignore
+++ b/.gitignore
@@ -293,6 +293,7 @@ xen/.config.old
 xen/System.map
 xen/arch/arm/asm-offsets.s
 xen/arch/arm/xen.lds
+xen/arch/x86/asm-macros.i
 xen/arch/x86/asm-offsets.s
 xen/arch/x86/boot/mkelf32
 xen/arch/x86/xen.lds
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -209,9 +209,22 @@ $(TARGET).efi: prelink-efi.o $(note_file
 efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: $(BASEDIR)/arch/x86/efi/built_in.o
 efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: ;
 
-asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c
+asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c $(BASEDIR)/include/asm-x86/asm-macros.h
 	$(CC) $(filter-out -Wa$(comma)% -flto,$(CFLAGS)) -S -o $@ $<
 
+asm-macros.i: CFLAGS += -D__ASSEMBLY__ -P
+
+$(BASEDIR)/include/asm-x86/asm-macros.h: asm-macros.i Makefile
+	echo '#if 0' >$@.new
+	echo '.if 0' >>$@.new
+	echo '#endif' >>$@.new
+	echo 'asm ( ".include \"$@\"" );' >>$@.new
+	echo '#if 0' >>$@.new
+	echo '.endif' >>$@.new
+	cat $< >>$@.new
+	echo '#endif' >>$@.new
+	$(call move-if-changed,$@.new,$@)
+
 xen.lds: xen.lds.S
 	$(CC) -P -E -Ui386 $(filter-out -Wa$(comma)%,$(AFLAGS)) -o $@ $<
 	sed -e 's/xen\.lds\.o:/xen\.lds:/g' <.xen.lds.d >.xen.lds.d.new
@@ -231,6 +244,7 @@ efi/mkreloc: efi/mkreloc.c
 .PHONY: clean
 clean::
 	rm -f asm-offsets.s *.lds boot/*.o boot/*~ boot/core boot/mkelf32
+	rm -f asm-macros.i $(BASEDIR)/include/asm-x86/asm-macros.*
 	rm -f $(BASEDIR)/.xen-syms.[0-9]* boot/.*.d
 	rm -f $(BASEDIR)/.xen.efi.[0-9]* efi/*.efi efi/disabled efi/mkreloc
 	rm -f boot/cmdline.S boot/reloc.S boot/*.lnk boot/*.bin
--- /dev/null
+++ b/xen/arch/x86/asm-macros.c
@@ -0,0 +1 @@
+#include <asm/alternative-asm.h>
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -1,11 +1,12 @@
 #ifndef __X86_ALTERNATIVE_H__
 #define __X86_ALTERNATIVE_H__
 
+#ifdef __ASSEMBLY__
 #include <asm/alternative-asm.h>
-
-#ifndef __ASSEMBLY__
+#else
 #include <xen/stringify.h>
 #include <xen/types.h>
+#include <asm/asm-macros.h>
 
 struct __packed alt_instr {
     int32_t  orig_offset;   /* original instruction */





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 09/16] x86: infrastructure to allow converting certain indirect calls to direct ones

[Jan Beulich]

In a number of cases the targets of indirect calls get determined once
at boot time. In such cases we can replace those calls with direct ones
via our alternative instruction patching mechanism.

Some of the targets (in particular the hvm_funcs ones) get established
only in pre-SMP initcalls, making necessary a second passs through the
alternative patching code. Therefore some adjustments beyond the
recognition of the new special pattern are necessary there.

Note that patching such sites more than once is not supported (and the
supplied macros also don't provide any means to do so).

Also a note on alternative_call_clobber(): The particular compiler error
("unable to find a register to spill") was observed on gcc 7.x only,
other than the similar one in shadow code that was taken care of earlier
in the series. I hope it is acceptable to require invoking that macro in
a small set of places.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -160,8 +160,9 @@ text_poke(void *addr, const void *opcode
  * APs have less capabilities than the boot processor are not handled.
  * Tough. Make sure you disable such features by hand.
  */
-void init_or_livepatch apply_alternatives(struct alt_instr *start,
-                                          struct alt_instr *end)
+static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
+                                                  struct alt_instr *end,
+                                                  bool force)
 {
     struct alt_instr *a, *base;
 
@@ -200,6 +201,13 @@ void init_or_livepatch apply_alternative
         if ( ALT_ORIG_PTR(base) != orig )
             base = a;
 
+        /* Skip patch sites already handled during the first pass. */
+        if ( a->priv )
+        {
+            ASSERT(force);
+            continue;
+        }
+
         /* If there is no replacement to make, see about optimising the nops. */
         if ( !boot_cpu_has(a->cpuid) )
         {
@@ -207,7 +215,7 @@ void init_or_livepatch apply_alternative
             if ( base->priv )
                 continue;
 
-            base->priv = 1;
+            a->priv = 1;
 
             /* Nothing useful to do? */
             if ( a->pad_len <= 1 )
@@ -212,13 +220,64 @@ void init_or_livepatch apply_alternative
             continue;
         }
 
-        base->priv = 1;
-
         memcpy(buf, repl, a->repl_len);
 
         /* 0xe8/0xe9 are relative branches; fix the offset. */
         if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
-            *(int32_t *)(buf + 1) += repl - orig;
+        {
+            /*
+             * Detect the special case of indirect-to-direct branch patching:
+             * - replacement is a direct CALL/JMP (opcodes 0xE8/0xE9; already
+             *   checked above),
+             * - replacement's displacement is -5 (pointing back at the very
+             *   insn, which makes no sense in a real replacement insn),
+             * - original is an indirect CALL/JMP (opcodes 0xFF/2 or 0xFF/4)
+             *   using RIP-relative addressing.
+             * Some function targets may not be available when we come here
+             * the first time. Defer patching of those until the post-presmp-
+             * initcalls re-invocation. If at that point the target pointer is
+             * still NULL, insert "UD2; UD0" (for ease of recognition) instead
+             * of CALL/JMP.
+             */
+            if ( a->cpuid == X86_FEATURE_ALWAYS &&
+                 *(int32_t *)(buf + 1) == -5 &&
+                 a->orig_len >= 6 &&
+                 orig[0] == 0xff &&
+                 orig[1] == (*buf & 1 ? 0x25 : 0x15) )
+            {
+                long disp = *(int32_t *)(orig + 2);
+                const uint8_t *dest = *(void **)(orig + 6 + disp);
+
+                if ( dest )
+                {
+                    disp = dest - (orig + 5);
+                    ASSERT(disp == (int32_t)disp);
+                    *(int32_t *)(buf + 1) = disp;
+                }
+                else if ( force )
+                {
+                    buf[0] = 0x0f;
+                    buf[1] = 0x0b;
+                    buf[2] = 0x0f;
+                    buf[3] = 0xff;
+                    buf[4] = 0xff;
+                }
+                else
+                    continue;
+            }
+            else if ( force && system_state < SYS_STATE_active )
+                ASSERT_UNREACHABLE();
+            else
+                *(int32_t *)(buf + 1) += repl - orig;
+        }
+        else if ( force && system_state < SYS_STATE_active  )
+            ASSERT_UNREACHABLE();
         /* RIP-relative addressing is easy to check for in VEX-encoded insns. */
         else if ( a->repl_len >= 8 &&
                   (*buf & ~1) == 0xc4 &&
@@ -232,12 +291,21 @@ void init_or_livepatch apply_alternative
                   (buf[4 - (*buf & 1)] & ~0x38) == 0x05 )
             *(int32_t *)(buf + 5 - (*buf & 1)) += repl - orig;
 
+        a->priv = 1;
+
         add_nops(buf + a->repl_len, total_len - a->repl_len);
         text_poke(orig, buf, total_len);
     }
 }
 
-static bool __initdata alt_done;
+void init_or_livepatch apply_alternatives(struct alt_instr *start,
+                                          struct alt_instr *end)
+{
+    _apply_alternatives(start, end, true);
+}
+
+static unsigned int __initdata alt_todo;
+static unsigned int __initdata alt_done;
 
 /*
  * At boot time, we patch alternatives in NMI context.  This means that the
@@ -252,7 +320,7 @@ static int __init nmi_apply_alternatives
      * More than one NMI may occur between the two set_nmi_callback() below.
      * We only need to apply alternatives once.
      */
-    if ( !alt_done )
+    if ( !(alt_done & alt_todo) )
     {
         unsigned long cr0;
 
@@ -261,11 +329,12 @@ static int __init nmi_apply_alternatives
         /* Disable WP to allow patching read-only pages. */
         write_cr0(cr0 & ~X86_CR0_WP);
 
-        apply_alternatives(__alt_instructions, __alt_instructions_end);
+        _apply_alternatives(__alt_instructions, __alt_instructions_end,
+                            alt_done);
 
         write_cr0(cr0);
 
-        alt_done = true;
+        alt_done |= alt_todo;
     }
 
     return 1;
@@ -275,13 +344,11 @@ static int __init nmi_apply_alternatives
  * This routine is called with local interrupt disabled and used during
  * bootup.
  */
-void __init alternative_instructions(void)
+static void __init _alternative_instructions(bool force)
 {
     unsigned int i;
     nmi_callback_t *saved_nmi_callback;
 
-    arch_init_ideal_nops();
-
     /*
      * Don't stop machine check exceptions while patching.
      * MCEs only happen when something got corrupted and in this
@@ -294,6 +361,10 @@ void __init alternative_instructions(voi
      */
     ASSERT(!local_irq_is_enabled());
 
+    /* Set what operation to perform /before/ setting the callback. */
+    alt_todo = 1u << force;
+    barrier();
+
     /*
      * As soon as the callback is set up, the next NMI will trigger patching,
      * even an NMI ahead of our explicit self-NMI.
@@ -309,11 +380,24 @@ void __init alternative_instructions(voi
      * cover the (hopefully never) async case, poll alt_done for up to one
      * second.
      */
-    for ( i = 0; !ACCESS_ONCE(alt_done) && i < 1000; ++i )
+    for ( i = 0; !(ACCESS_ONCE(alt_done) & alt_todo) && i < 1000; ++i )
         mdelay(1);
 
-    if ( !ACCESS_ONCE(alt_done) )
+    if ( !(ACCESS_ONCE(alt_done) & alt_todo) )
         panic("Timed out waiting for alternatives self-NMI to hit");
 
     set_nmi_callback(saved_nmi_callback);
 }
+
+void __init alternative_instructions(void)
+{
+    arch_init_ideal_nops();
+    _alternative_instructions(false);
+}
+
+void __init alternative_branches(void)
+{
+    local_irq_disable();
+    _alternative_instructions(true);
+    local_irq_enable();
+}
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1619,6 +1619,8 @@ void __init noreturn __start_xen(unsigne
 
     do_presmp_initcalls();
 
+    alternative_branches();
+
     /*
      * NB: when running as a PV shim VCPUOP_up/down is wired to the shim
      * physical cpu_add/remove functions, so launch the guest with only
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -26,6 +26,7 @@ extern void add_nops(void *insns, unsign
 /* Similar to alternative_instructions except it can be run with IRQs enabled. */
 extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
 extern void alternative_instructions(void);
+extern void alternative_branches(void);
 
 #define alt_orig_len       "(.LXEN%=_orig_e - .LXEN%=_orig_s)"
 #define alt_pad_len        "(.LXEN%=_orig_p - .LXEN%=_orig_e)"
@@ -149,6 +150,190 @@ extern void alternative_instructions(voi
 /* Use this macro(s) if you need more than one output parameter. */
 #define ASM_OUTPUT2(a...) a
 
+/*
+ * Machinery to allow converting indirect to direct calls, when the called
+ * function is determined once at boot and later never changed.
+ */
+
+#define ALT_CALL_arg1 "rdi"
+#define ALT_CALL_arg2 "rsi"
+#define ALT_CALL_arg3 "rdx"
+#define ALT_CALL_arg4 "rcx"
+#define ALT_CALL_arg5 "r8"
+#define ALT_CALL_arg6 "r9"
+
+#define ALT_CALL_ARG(arg, n) \
+    register typeof((arg) ?: 0) a ## n ## _ asm(ALT_CALL_arg ## n) = (arg)
+#define ALT_CALL_NO_ARG(n) \
+    register unsigned long a ## n ## _ asm(ALT_CALL_arg ## n)
+
+#define ALT_CALL_NO_ARG6 ALT_CALL_NO_ARG(6)
+#define ALT_CALL_NO_ARG5 ALT_CALL_NO_ARG(5); ALT_CALL_NO_ARG6
+#define ALT_CALL_NO_ARG4 ALT_CALL_NO_ARG(4); ALT_CALL_NO_ARG5
+#define ALT_CALL_NO_ARG3 ALT_CALL_NO_ARG(3); ALT_CALL_NO_ARG4
+#define ALT_CALL_NO_ARG2 ALT_CALL_NO_ARG(2); ALT_CALL_NO_ARG3
+#define ALT_CALL_NO_ARG1 ALT_CALL_NO_ARG(1); ALT_CALL_NO_ARG2
+
+/*
+ * Unfortunately ALT_CALL_NO_ARG() above can't use a fake initializer (to
+ * suppress "uninitialized variable" warnings), as various versions of gcc
+ * older than 8.1 fall on the nose in various ways with that (always because
+ * of some other construct elsewhere in the same function needing to use the
+ * same hard register). Otherwise the asm() below could uniformly use "+r"
+ * output constraints, making unnecessary all these ALT_CALL<n>_OUT macros.
+ */
+#define ALT_CALL0_OUT "=r" (a1_), "=r" (a2_), "=r" (a3_), \
+                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
+#define ALT_CALL1_OUT "+r" (a1_), "=r" (a2_), "=r" (a3_), \
+                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
+#define ALT_CALL2_OUT "+r" (a1_), "+r" (a2_), "=r" (a3_), \
+                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
+#define ALT_CALL3_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
+                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
+#define ALT_CALL4_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
+                      "+r" (a4_), "=r" (a5_), "=r" (a6_)
+#define ALT_CALL5_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
+                      "+r" (a4_), "+r" (a5_), "=r" (a6_)
+#define ALT_CALL6_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
+                      "+r" (a4_), "+r" (a5_), "+r" (a6_)
+
+#define alternative_callN(n, rettype, func) ({                     \
+    rettype ret_;                                                  \
+    register unsigned long r10_ asm("r10");                        \
+    register unsigned long r11_ asm("r11");                        \
+    asm volatile (__stringify(ALTERNATIVE "call *%c[addr](%%rip)", \
+                                          "call .",                \
+                                          X86_FEATURE_ALWAYS)      \
+                  : ALT_CALL ## n ## _OUT, "=a" (ret_),            \
+                    "=r" (r10_), "=r" (r11_)                       \
+                  : [addr] "i" (&(func)), "g" (func)               \
+                  : "memory" );                                    \
+    ret_;                                                          \
+})
+
+#define alternative_vcall0(func) ({             \
+    ALT_CALL_NO_ARG1;                           \
+    ((void)alternative_callN(0, int, func));    \
+})
+
+#define alternative_call0(func) ({              \
+    ALT_CALL_NO_ARG1;                           \
+    alternative_callN(0, typeof(func()), func); \
+})
+
+#define alternative_vcall1(func, arg) ({           \
+    ALT_CALL_ARG(arg, 1);                          \
+    ALT_CALL_NO_ARG2;                              \
+    (void)sizeof(func(arg));                       \
+    (void)alternative_callN(1, int, func);         \
+})
+
+#define alternative_call1(func, arg) ({            \
+    ALT_CALL_ARG(arg, 1);                          \
+    ALT_CALL_NO_ARG2;                              \
+    alternative_callN(1, typeof(func(arg)), func); \
+})
+
+#define alternative_vcall2(func, arg1, arg2) ({           \
+    ALT_CALL_ARG(arg1, 1);                                \
+    ALT_CALL_ARG(arg2, 2);                                \
+    ALT_CALL_NO_ARG3;                                     \
+    (void)sizeof(func(arg1, arg2));                       \
+    (void)alternative_callN(2, int, func);                \
+})
+
+#define alternative_call2(func, arg1, arg2) ({            \
+    ALT_CALL_ARG(arg1, 1);                                \
+    ALT_CALL_ARG(arg2, 2);                                \
+    ALT_CALL_NO_ARG3;                                     \
+    alternative_callN(2, typeof(func(arg1, arg2)), func); \
+})
+
+#define alternative_vcall3(func, arg1, arg2, arg3) ({    \
+    ALT_CALL_ARG(arg1, 1);                               \
+    ALT_CALL_ARG(arg2, 2);                               \
+    ALT_CALL_ARG(arg3, 3);                               \
+    ALT_CALL_NO_ARG4;                                    \
+    (void)sizeof(func(arg1, arg2, arg3));                \
+    (void)alternative_callN(3, int, func);               \
+})
+
+#define alternative_call3(func, arg1, arg2, arg3) ({     \
+    ALT_CALL_ARG(arg1, 1);                               \
+    ALT_CALL_ARG(arg2, 2);                               \
+    ALT_CALL_ARG(arg3, 3);                               \
+    ALT_CALL_NO_ARG4;                                    \
+    alternative_callN(3, typeof(func(arg1, arg2, arg3)), \
+                      func);                             \
+})
+
+#define alternative_vcall4(func, arg1, arg2, arg3, arg4) ({ \
+    ALT_CALL_ARG(arg1, 1);                                  \
+    ALT_CALL_ARG(arg2, 2);                                  \
+    ALT_CALL_ARG(arg3, 3);                                  \
+    ALT_CALL_ARG(arg4, 4);                                  \
+    ALT_CALL_NO_ARG5;                                       \
+    (void)sizeof(func(arg1, arg2, arg3, arg4));             \
+    (void)alternative_callN(4, int, func);                  \
+})
+
+#define alternative_call4(func, arg1, arg2, arg3, arg4) ({  \
+    ALT_CALL_ARG(arg1, 1);                                  \
+    ALT_CALL_ARG(arg2, 2);                                  \
+    ALT_CALL_ARG(arg3, 3);                                  \
+    ALT_CALL_ARG(arg4, 4);                                  \
+    ALT_CALL_NO_ARG5;                                       \
+    alternative_callN(4, typeof(func(arg1, arg2,            \
+                                     arg3, arg4)),          \
+                      func);                                \
+})
+
+#define alternative_vcall5(func, arg1, arg2, arg3, arg4, arg5) ({ \
+    ALT_CALL_ARG(arg1, 1);                                        \
+    ALT_CALL_ARG(arg2, 2);                                        \
+    ALT_CALL_ARG(arg3, 3);                                        \
+    ALT_CALL_ARG(arg4, 4);                                        \
+    ALT_CALL_ARG(arg5, 5);                                        \
+    ALT_CALL_NO_ARG6;                                             \
+    (void)sizeof(func(arg1, arg2, arg3, arg4, arg5));             \
+    (void)alternative_callN(5, int, func, ALT_CALL_OUT5);         \
+})
+
+#define alternative_call5(func, arg1, arg2, arg3, arg4, arg5) ({  \
+    ALT_CALL_ARG(arg1, 1);                                        \
+    ALT_CALL_ARG(arg2, 2);                                        \
+    ALT_CALL_ARG(arg3, 3);                                        \
+    ALT_CALL_ARG(arg4, 4);                                        \
+    ALT_CALL_ARG(arg5, 5);                                        \
+    ALT_CALL_NO_ARG6;                                             \
+    alternative_callN(5, typeof(func(arg1, arg2, arg3,            \
+                                     arg4, arg5)),                \
+                      func, ALT_CALL_OUT5);                       \
+})
+
+#define alternative_vcall6(func, arg1, arg2, arg3, arg4, arg5, arg6) ({ \
+    ALT_CALL_ARG(arg1, 1);                                              \
+    ALT_CALL_ARG(arg2, 2);                                              \
+    ALT_CALL_ARG(arg3, 3);                                              \
+    ALT_CALL_ARG(arg4, 4);                                              \
+    ALT_CALL_ARG(arg5, 5);                                              \
+    ALT_CALL_ARG(arg6, 6);                                              \
+    (void)sizeof(func(arg1, arg2, arg3, arg4, arg5, arg6));             \
+    (void)alternative_callN(6, int, func);                              \
+})
+
+#define alternative_call6(func, arg1, arg2, arg3, arg4, arg5, arg6) ({  \
+    ALT_CALL_ARG(arg1, 1);                                              \
+    ALT_CALL_ARG(arg2, 2);                                              \
+    ALT_CALL_ARG(arg3, 3);                                              \
+    ALT_CALL_ARG(arg4, 4);                                              \
+    ALT_CALL_ARG(arg5, 5);                                              \
+    ALT_CALL_ARG(arg6, 6);                                              \
+    alternative_callN(6, typeof(func(arg1, arg2, arg3,                  \
+                                     arg4, arg5, arg6)),                \
+                      func, ALT_CALL_OUT6);                             \
+})
+
 #endif /*  !__ASSEMBLY__  */
 
 #endif /* __X86_ALTERNATIVE_H__ */




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 10/16] x86/HVM: patch indirect calls through hvm_funcs to direct ones

[Jan Beulich]

This is intentionally not touching hooks used rarely (or not at all)
during the lifetime of a VM, like {domain,vcpu}_initialise or cpu_up,
as well as nested, VM event, and altp2m ones (they can all be done
later, if so desired). Virtual Interrupt delivery ones will be dealt
with in a subsequent patch.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Needless to say that I'm pretty unhappy about the workaround I had to
add to hvm_set_rdtsc_exiting(). Improvement suggestions welcome.

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2017,7 +2017,7 @@ static int hvmemul_write_msr(
 static int hvmemul_wbinvd(
     struct x86_emulate_ctxt *ctxt)
 {
-    hvm_funcs.wbinvd_intercept();
+    alternative_vcall0(hvm_funcs.wbinvd_intercept);
     return X86EMUL_OKAY;
 }
 
@@ -2035,7 +2035,7 @@ static int hvmemul_get_fpu(
     struct vcpu *curr = current;
 
     if ( !curr->fpu_dirtied )
-        hvm_funcs.fpu_dirty_intercept();
+        alternative_vcall0(hvm_funcs.fpu_dirty_intercept);
     else if ( type == X86EMUL_FPU_fpu )
     {
         const typeof(curr->arch.xsave_area->fpu_sse) *fpu_ctxt =
@@ -2152,7 +2152,7 @@ static void hvmemul_put_fpu(
         {
             curr->fpu_dirtied = false;
             stts();
-            hvm_funcs.fpu_leave(curr);
+            alternative_vcall1(hvm_funcs.fpu_leave, curr);
         }
     }
 }
@@ -2314,7 +2314,8 @@ static int _hvm_emulate_one(struct hvm_e
     if ( hvmemul_ctxt->intr_shadow != new_intr_shadow )
     {
         hvmemul_ctxt->intr_shadow = new_intr_shadow;
-        hvm_funcs.set_interrupt_shadow(curr, new_intr_shadow);
+        alternative_vcall2(hvm_funcs.set_interrupt_shadow,
+                           curr, new_intr_shadow);
     }
 
     if ( hvmemul_ctxt->ctxt.retire.hlt &&
@@ -2451,7 +2452,8 @@ void hvm_emulate_init_once(
 
     memset(hvmemul_ctxt, 0, sizeof(*hvmemul_ctxt));
 
-    hvmemul_ctxt->intr_shadow = hvm_funcs.get_interrupt_shadow(curr);
+    hvmemul_ctxt->intr_shadow =
+        alternative_call1(hvm_funcs.get_interrupt_shadow, curr);
     hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt);
     hvmemul_get_seg_reg(x86_seg_ss, hvmemul_ctxt);
 
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -271,13 +271,24 @@ void hvm_set_rdtsc_exiting(struct domain
 {
     struct vcpu *v;
 
+#if __GNUC__ >= 7
+/*
+ * gcc from 7.x onwards warns about ternary operators with their middle operand
+ * omitted and the controlling expression itself being of _Bool type.
+ */
+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Wparentheses"
+#endif
     for_each_vcpu ( d, v )
-        hvm_funcs.set_rdtsc_exiting(v, enable);
+        alternative_vcall2(hvm_funcs.set_rdtsc_exiting, v, enable);
+#if __GNUC__ >= 7
+# pragma GCC diagnostic pop
+#endif
 }
 
 void hvm_get_guest_pat(struct vcpu *v, u64 *guest_pat)
 {
-    if ( !hvm_funcs.get_guest_pat(v, guest_pat) )
+    if ( !alternative_call2(hvm_funcs.get_guest_pat, v, guest_pat) )
         *guest_pat = v->arch.hvm_vcpu.pat_cr;
 }
 
@@ -302,7 +313,7 @@ int hvm_set_guest_pat(struct vcpu *v, u6
             return 0;
         }
 
-    if ( !hvm_funcs.set_guest_pat(v, guest_pat) )
+    if ( !alternative_call2(hvm_funcs.set_guest_pat, v, guest_pat) )
         v->arch.hvm_vcpu.pat_cr = guest_pat;
 
     return 1;
@@ -342,7 +353,7 @@ bool hvm_set_guest_bndcfgs(struct vcpu *
             /* nothing, best effort only */;
     }
 
-    return hvm_funcs.set_guest_bndcfgs(v, val);
+    return alternative_call2(hvm_funcs.set_guest_bndcfgs, v, val);
 }
 
 /*
@@ -502,7 +513,8 @@ void hvm_migrate_pirqs(struct vcpu *v)
 static bool hvm_get_pending_event(struct vcpu *v, struct x86_event *info)
 {
     info->cr2 = v->arch.hvm_vcpu.guest_cr[2];
-    return hvm_funcs.get_pending_event(v, info);
+
+    return alternative_call2(hvm_funcs.get_pending_event, v, info);
 }
 
 void hvm_do_resume(struct vcpu *v)
@@ -1684,7 +1696,7 @@ void hvm_inject_event(const struct x86_e
         }
     }
 
-    hvm_funcs.inject_event(event);
+    alternative_vcall1(hvm_funcs.inject_event, event);
 }
 
 int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
@@ -2271,7 +2283,7 @@ int hvm_set_cr0(unsigned long value, boo
          (!rangeset_is_empty(d->iomem_caps) ||
           !rangeset_is_empty(d->arch.ioport_caps) ||
           has_arch_pdevs(d)) )
-        hvm_funcs.handle_cd(v, value);
+        alternative_vcall2(hvm_funcs.handle_cd, v, value);
 
     hvm_update_cr(v, 0, value);
 
@@ -3515,7 +3527,8 @@ int hvm_msr_read_intercept(unsigned int
             goto gp_fault;
         /* If ret == 0 then this is not an MCE MSR, see other MSRs. */
         ret = ((ret == 0)
-               ? hvm_funcs.msr_read_intercept(msr, msr_content)
+               ? alternative_call2(hvm_funcs.msr_read_intercept,
+                                   msr, msr_content)
                : X86EMUL_OKAY);
         break;
     }
@@ -3672,7 +3685,8 @@ int hvm_msr_write_intercept(unsigned int
             goto gp_fault;
         /* If ret == 0 then this is not an MCE MSR, see other MSRs. */
         ret = ((ret == 0)
-               ? hvm_funcs.msr_write_intercept(msr, msr_content)
+               ? alternative_call2(hvm_funcs.msr_write_intercept,
+                                   msr, msr_content)
                : X86EMUL_OKAY);
         break;
     }
@@ -3864,7 +3878,7 @@ void hvm_hypercall_page_initialise(struc
                                    void *hypercall_page)
 {
     hvm_latch_shinfo_size(d);
-    hvm_funcs.init_hypercall_page(d, hypercall_page);
+    alternative_vcall2(hvm_funcs.init_hypercall_page, d, hypercall_page);
 }
 
 void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
@@ -4988,7 +5002,7 @@ void hvm_domain_soft_reset(struct domain
 void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
                               struct segment_register *reg)
 {
-    hvm_funcs.get_segment_register(v, seg, reg);
+    alternative_vcall3(hvm_funcs.get_segment_register, v, seg, reg);
 
     switch ( seg )
     {
@@ -5134,7 +5148,7 @@ void hvm_set_segment_register(struct vcp
         return;
     }
 
-    hvm_funcs.set_segment_register(v, seg, reg);
+    alternative_vcall3(hvm_funcs.set_segment_register, v, seg, reg);
 }
 
 /*
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5785,7 +5785,7 @@ void paging_invlpg(struct vcpu *v, unsig
     if ( is_pv_vcpu(v) )
         flush_tlb_one_local(va);
     else
-        hvm_funcs.invlpg(v, va);
+        alternative_vcall2(hvm_funcs.invlpg, v, va);
 }
 
 /* Build a 32bit PSE page table using 4MB pages. */
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -317,42 +317,42 @@ static inline int
 hvm_guest_x86_mode(struct vcpu *v)
 {
     ASSERT(v == current);
-    return hvm_funcs.guest_x86_mode(v);
+    return alternative_call1(hvm_funcs.guest_x86_mode, v);
 }
 
 static inline void
 hvm_update_host_cr3(struct vcpu *v)
 {
     if ( hvm_funcs.update_host_cr3 )
-        hvm_funcs.update_host_cr3(v);
+        alternative_vcall1(hvm_funcs.update_host_cr3, v);
 }
 
 static inline void hvm_update_guest_cr(struct vcpu *v, unsigned int cr)
 {
-    hvm_funcs.update_guest_cr(v, cr, 0);
+    alternative_vcall3(hvm_funcs.update_guest_cr, v, cr, 0);
 }
 
 static inline void hvm_update_guest_cr3(struct vcpu *v, bool noflush)
 {
     unsigned int flags = noflush ? HVM_UPDATE_GUEST_CR3_NOFLUSH : 0;
 
-    hvm_funcs.update_guest_cr(v, 3, flags);
+    alternative_vcall3(hvm_funcs.update_guest_cr, v, 3, flags);
 }
 
 static inline void hvm_update_guest_efer(struct vcpu *v)
 {
-    hvm_funcs.update_guest_efer(v);
+    alternative_vcall1(hvm_funcs.update_guest_efer, v);
 }
 
 static inline void hvm_cpuid_policy_changed(struct vcpu *v)
 {
-    hvm_funcs.cpuid_policy_changed(v);
+    alternative_vcall1(hvm_funcs.cpuid_policy_changed, v);
 }
 
 static inline void hvm_set_tsc_offset(struct vcpu *v, uint64_t offset,
                                       uint64_t at_tsc)
 {
-    hvm_funcs.set_tsc_offset(v, offset, at_tsc);
+    alternative_vcall3(hvm_funcs.set_tsc_offset, v, offset, at_tsc);
 }
 
 /*
@@ -372,7 +372,7 @@ void hvm_hypercall_page_initialise(struc
 static inline unsigned int
 hvm_get_cpl(struct vcpu *v)
 {
-    return hvm_funcs.get_cpl(v);
+    return alternative_call1(hvm_funcs.get_cpl, v);
 }
 
 void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
@@ -382,13 +382,13 @@ void hvm_set_segment_register(struct vcp
 
 static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
 {
-    return hvm_funcs.get_shadow_gs_base(v);
+    return alternative_call1(hvm_funcs.get_shadow_gs_base, v);
 }
 
 static inline bool hvm_get_guest_bndcfgs(struct vcpu *v, u64 *val)
 {
     return hvm_funcs.get_guest_bndcfgs &&
-           hvm_funcs.get_guest_bndcfgs(v, val);
+           alternative_call2(hvm_funcs.get_guest_bndcfgs, v, val);
 }
 
 bool hvm_set_guest_bndcfgs(struct vcpu *v, u64 val);
@@ -454,7 +454,7 @@ static inline void hvm_inject_page_fault
 
 static inline int hvm_event_pending(struct vcpu *v)
 {
-    return hvm_funcs.event_pending(v);
+    return alternative_call1(hvm_funcs.event_pending, v);
 }
 
 /* These bits in CR4 are owned by the host. */
@@ -485,7 +485,8 @@ static inline void hvm_cpu_down(void)
 
 static inline unsigned int hvm_get_insn_bytes(struct vcpu *v, uint8_t *buf)
 {
-    return (hvm_funcs.get_insn_bytes ? hvm_funcs.get_insn_bytes(v, buf) : 0);
+    return (hvm_funcs.get_insn_bytes
+            ? alternative_call2(hvm_funcs.get_insn_bytes, v, buf) : 0);
 }
 
 enum hvm_task_switch_reason { TSW_jmp, TSW_iret, TSW_call_or_int };
@@ -517,7 +518,7 @@ void hvm_mapped_guest_frames_mark_dirty(
 static inline void hvm_set_info_guest(struct vcpu *v)
 {
     if ( hvm_funcs.set_info_guest )
-        return hvm_funcs.set_info_guest(v);
+        alternative_vcall1(hvm_funcs.set_info_guest, v);
 }
 
 int hvm_debug_op(struct vcpu *v, int32_t op);




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 10/16] x86/HVM: patch indirect calls through hvm_funcs to direct ones

[Jan Beulich]

>>> On 11.07.18 at 15:42, <JBeulich@suse.com> wrote:
> This is intentionally not touching hooks used rarely (or not at all)
> during the lifetime of a VM, like {domain,vcpu}_initialise or cpu_up,
> as well as nested, VM event, and altp2m ones (they can all be done
> later, if so desired). Virtual Interrupt delivery ones will be dealt
> with in a subsequent patch.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Should have Cc-ed you, Paul, right away.

Jan

> ---
> Needless to say that I'm pretty unhappy about the workaround I had to
> add to hvm_set_rdtsc_exiting(). Improvement suggestions welcome.
> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2017,7 +2017,7 @@ static int hvmemul_write_msr(
>  static int hvmemul_wbinvd(
>      struct x86_emulate_ctxt *ctxt)
>  {
> -    hvm_funcs.wbinvd_intercept();
> +    alternative_vcall0(hvm_funcs.wbinvd_intercept);
>      return X86EMUL_OKAY;
>  }
>  
> @@ -2035,7 +2035,7 @@ static int hvmemul_get_fpu(
>      struct vcpu *curr = current;
>  
>      if ( !curr->fpu_dirtied )
> -        hvm_funcs.fpu_dirty_intercept();
> +        alternative_vcall0(hvm_funcs.fpu_dirty_intercept);
>      else if ( type == X86EMUL_FPU_fpu )
>      {
>          const typeof(curr->arch.xsave_area->fpu_sse) *fpu_ctxt =
> @@ -2152,7 +2152,7 @@ static void hvmemul_put_fpu(
>          {
>              curr->fpu_dirtied = false;
>              stts();
> -            hvm_funcs.fpu_leave(curr);
> +            alternative_vcall1(hvm_funcs.fpu_leave, curr);
>          }
>      }
>  }
> @@ -2314,7 +2314,8 @@ static int _hvm_emulate_one(struct hvm_e
>      if ( hvmemul_ctxt->intr_shadow != new_intr_shadow )
>      {
>          hvmemul_ctxt->intr_shadow = new_intr_shadow;
> -        hvm_funcs.set_interrupt_shadow(curr, new_intr_shadow);
> +        alternative_vcall2(hvm_funcs.set_interrupt_shadow,
> +                           curr, new_intr_shadow);
>      }
>  
>      if ( hvmemul_ctxt->ctxt.retire.hlt &&
> @@ -2451,7 +2452,8 @@ void hvm_emulate_init_once(
>  
>      memset(hvmemul_ctxt, 0, sizeof(*hvmemul_ctxt));
>  
> -    hvmemul_ctxt->intr_shadow = hvm_funcs.get_interrupt_shadow(curr);
> +    hvmemul_ctxt->intr_shadow =
> +        alternative_call1(hvm_funcs.get_interrupt_shadow, curr);
>      hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt);
>      hvmemul_get_seg_reg(x86_seg_ss, hvmemul_ctxt);
>  
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -271,13 +271,24 @@ void hvm_set_rdtsc_exiting(struct domain
>  {
>      struct vcpu *v;
>  
> +#if __GNUC__ >= 7
> +/*
> + * gcc from 7.x onwards warns about ternary operators with their middle 
> operand
> + * omitted and the controlling expression itself being of _Bool type.
> + */
> +# pragma GCC diagnostic push
> +# pragma GCC diagnostic ignored "-Wparentheses"
> +#endif
>      for_each_vcpu ( d, v )
> -        hvm_funcs.set_rdtsc_exiting(v, enable);
> +        alternative_vcall2(hvm_funcs.set_rdtsc_exiting, v, enable);
> +#if __GNUC__ >= 7
> +# pragma GCC diagnostic pop
> +#endif
>  }
>  
>  void hvm_get_guest_pat(struct vcpu *v, u64 *guest_pat)
>  {
> -    if ( !hvm_funcs.get_guest_pat(v, guest_pat) )
> +    if ( !alternative_call2(hvm_funcs.get_guest_pat, v, guest_pat) )
>          *guest_pat = v->arch.hvm_vcpu.pat_cr;
>  }
>  
> @@ -302,7 +313,7 @@ int hvm_set_guest_pat(struct vcpu *v, u6
>              return 0;
>          }
>  
> -    if ( !hvm_funcs.set_guest_pat(v, guest_pat) )
> +    if ( !alternative_call2(hvm_funcs.set_guest_pat, v, guest_pat) )
>          v->arch.hvm_vcpu.pat_cr = guest_pat;
>  
>      return 1;
> @@ -342,7 +353,7 @@ bool hvm_set_guest_bndcfgs(struct vcpu *
>              /* nothing, best effort only */;
>      }
>  
> -    return hvm_funcs.set_guest_bndcfgs(v, val);
> +    return alternative_call2(hvm_funcs.set_guest_bndcfgs, v, val);
>  }
>  
>  /*
> @@ -502,7 +513,8 @@ void hvm_migrate_pirqs(struct vcpu *v)
>  static bool hvm_get_pending_event(struct vcpu *v, struct x86_event *info)
>  {
>      info->cr2 = v->arch.hvm_vcpu.guest_cr[2];
> -    return hvm_funcs.get_pending_event(v, info);
> +
> +    return alternative_call2(hvm_funcs.get_pending_event, v, info);
>  }
>  
>  void hvm_do_resume(struct vcpu *v)
> @@ -1684,7 +1696,7 @@ void hvm_inject_event(const struct x86_e
>          }
>      }
>  
> -    hvm_funcs.inject_event(event);
> +    alternative_vcall1(hvm_funcs.inject_event, event);
>  }
>  
>  int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> @@ -2271,7 +2283,7 @@ int hvm_set_cr0(unsigned long value, boo
>           (!rangeset_is_empty(d->iomem_caps) ||
>            !rangeset_is_empty(d->arch.ioport_caps) ||
>            has_arch_pdevs(d)) )
> -        hvm_funcs.handle_cd(v, value);
> +        alternative_vcall2(hvm_funcs.handle_cd, v, value);
>  
>      hvm_update_cr(v, 0, value);
>  
> @@ -3515,7 +3527,8 @@ int hvm_msr_read_intercept(unsigned int
>              goto gp_fault;
>          /* If ret == 0 then this is not an MCE MSR, see other MSRs. */
>          ret = ((ret == 0)
> -               ? hvm_funcs.msr_read_intercept(msr, msr_content)
> +               ? alternative_call2(hvm_funcs.msr_read_intercept,
> +                                   msr, msr_content)
>                 : X86EMUL_OKAY);
>          break;
>      }
> @@ -3672,7 +3685,8 @@ int hvm_msr_write_intercept(unsigned int
>              goto gp_fault;
>          /* If ret == 0 then this is not an MCE MSR, see other MSRs. */
>          ret = ((ret == 0)
> -               ? hvm_funcs.msr_write_intercept(msr, msr_content)
> +               ? alternative_call2(hvm_funcs.msr_write_intercept,
> +                                   msr, msr_content)
>                 : X86EMUL_OKAY);
>          break;
>      }
> @@ -3864,7 +3878,7 @@ void hvm_hypercall_page_initialise(struc
>                                     void *hypercall_page)
>  {
>      hvm_latch_shinfo_size(d);
> -    hvm_funcs.init_hypercall_page(d, hypercall_page);
> +    alternative_vcall2(hvm_funcs.init_hypercall_page, d, hypercall_page);
>  }
>  
>  void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
> @@ -4988,7 +5002,7 @@ void hvm_domain_soft_reset(struct domain
>  void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
>                                struct segment_register *reg)
>  {
> -    hvm_funcs.get_segment_register(v, seg, reg);
> +    alternative_vcall3(hvm_funcs.get_segment_register, v, seg, reg);
>  
>      switch ( seg )
>      {
> @@ -5134,7 +5148,7 @@ void hvm_set_segment_register(struct vcp
>          return;
>      }
>  
> -    hvm_funcs.set_segment_register(v, seg, reg);
> +    alternative_vcall3(hvm_funcs.set_segment_register, v, seg, reg);
>  }
>  
>  /*
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5785,7 +5785,7 @@ void paging_invlpg(struct vcpu *v, unsig
>      if ( is_pv_vcpu(v) )
>          flush_tlb_one_local(va);
>      else
> -        hvm_funcs.invlpg(v, va);
> +        alternative_vcall2(hvm_funcs.invlpg, v, va);
>  }
>  
>  /* Build a 32bit PSE page table using 4MB pages. */
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -317,42 +317,42 @@ static inline int
>  hvm_guest_x86_mode(struct vcpu *v)
>  {
>      ASSERT(v == current);
> -    return hvm_funcs.guest_x86_mode(v);
> +    return alternative_call1(hvm_funcs.guest_x86_mode, v);
>  }
>  
>  static inline void
>  hvm_update_host_cr3(struct vcpu *v)
>  {
>      if ( hvm_funcs.update_host_cr3 )
> -        hvm_funcs.update_host_cr3(v);
> +        alternative_vcall1(hvm_funcs.update_host_cr3, v);
>  }
>  
>  static inline void hvm_update_guest_cr(struct vcpu *v, unsigned int cr)
>  {
> -    hvm_funcs.update_guest_cr(v, cr, 0);
> +    alternative_vcall3(hvm_funcs.update_guest_cr, v, cr, 0);
>  }
>  
>  static inline void hvm_update_guest_cr3(struct vcpu *v, bool noflush)
>  {
>      unsigned int flags = noflush ? HVM_UPDATE_GUEST_CR3_NOFLUSH : 0;
>  
> -    hvm_funcs.update_guest_cr(v, 3, flags);
> +    alternative_vcall3(hvm_funcs.update_guest_cr, v, 3, flags);
>  }
>  
>  static inline void hvm_update_guest_efer(struct vcpu *v)
>  {
> -    hvm_funcs.update_guest_efer(v);
> +    alternative_vcall1(hvm_funcs.update_guest_efer, v);
>  }
>  
>  static inline void hvm_cpuid_policy_changed(struct vcpu *v)
>  {
> -    hvm_funcs.cpuid_policy_changed(v);
> +    alternative_vcall1(hvm_funcs.cpuid_policy_changed, v);
>  }
>  
>  static inline void hvm_set_tsc_offset(struct vcpu *v, uint64_t offset,
>                                        uint64_t at_tsc)
>  {
> -    hvm_funcs.set_tsc_offset(v, offset, at_tsc);
> +    alternative_vcall3(hvm_funcs.set_tsc_offset, v, offset, at_tsc);
>  }
>  
>  /*
> @@ -372,7 +372,7 @@ void hvm_hypercall_page_initialise(struc
>  static inline unsigned int
>  hvm_get_cpl(struct vcpu *v)
>  {
> -    return hvm_funcs.get_cpl(v);
> +    return alternative_call1(hvm_funcs.get_cpl, v);
>  }
>  
>  void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
> @@ -382,13 +382,13 @@ void hvm_set_segment_register(struct vcp
>  
>  static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
>  {
> -    return hvm_funcs.get_shadow_gs_base(v);
> +    return alternative_call1(hvm_funcs.get_shadow_gs_base, v);
>  }
>  
>  static inline bool hvm_get_guest_bndcfgs(struct vcpu *v, u64 *val)
>  {
>      return hvm_funcs.get_guest_bndcfgs &&
> -           hvm_funcs.get_guest_bndcfgs(v, val);
> +           alternative_call2(hvm_funcs.get_guest_bndcfgs, v, val);
>  }
>  
>  bool hvm_set_guest_bndcfgs(struct vcpu *v, u64 val);
> @@ -454,7 +454,7 @@ static inline void hvm_inject_page_fault
>  
>  static inline int hvm_event_pending(struct vcpu *v)
>  {
> -    return hvm_funcs.event_pending(v);
> +    return alternative_call1(hvm_funcs.event_pending, v);
>  }
>  
>  /* These bits in CR4 are owned by the host. */
> @@ -485,7 +485,8 @@ static inline void hvm_cpu_down(void)
>  
>  static inline unsigned int hvm_get_insn_bytes(struct vcpu *v, uint8_t *buf)
>  {
> -    return (hvm_funcs.get_insn_bytes ? hvm_funcs.get_insn_bytes(v, buf) : 
> 0);
> +    return (hvm_funcs.get_insn_bytes
> +            ? alternative_call2(hvm_funcs.get_insn_bytes, v, buf) : 0);
>  }
>  
>  enum hvm_task_switch_reason { TSW_jmp, TSW_iret, TSW_call_or_int };
> @@ -517,7 +518,7 @@ void hvm_mapped_guest_frames_mark_dirty(
>  static inline void hvm_set_info_guest(struct vcpu *v)
>  {
>      if ( hvm_funcs.set_info_guest )
> -        return hvm_funcs.set_info_guest(v);
> +        alternative_vcall1(hvm_funcs.set_info_guest, v);
>  }
>  
>  int hvm_debug_op(struct vcpu *v, int32_t op);
> 




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 10/16] x86/HVM: patch indirect calls through hvm_funcs to direct ones

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Jan Beulich
> Sent: 11 July 2018 14:43
> To: xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
> Subject: [Xen-devel] [PATCH 10/16] x86/HVM: patch indirect calls through
> hvm_funcs to direct ones
> 
> This is intentionally not touching hooks used rarely (or not at all)
> during the lifetime of a VM, like {domain,vcpu}_initialise or cpu_up,
> as well as nested, VM event, and altp2m ones (they can all be done
> later, if so desired). Virtual Interrupt delivery ones will be dealt
> with in a subsequent patch.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Needless to say that I'm pretty unhappy about the workaround I had to
> add to hvm_set_rdtsc_exiting(). Improvement suggestions welcome.

Presumably the workaround is needed because of the ?: in ALT_CALL_ARG() and will occur for any bool argument to an alternatives patched call? If so, and ad hoc workaround seems undesirable. Is it possible to suppress it by dropping the gcc-ism from the ternary operator?

  Paul

> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2017,7 +2017,7 @@ static int hvmemul_write_msr(
>  static int hvmemul_wbinvd(
>      struct x86_emulate_ctxt *ctxt)
>  {
> -    hvm_funcs.wbinvd_intercept();
> +    alternative_vcall0(hvm_funcs.wbinvd_intercept);
>      return X86EMUL_OKAY;
>  }
> 
> @@ -2035,7 +2035,7 @@ static int hvmemul_get_fpu(
>      struct vcpu *curr = current;
> 
>      if ( !curr->fpu_dirtied )
> -        hvm_funcs.fpu_dirty_intercept();
> +        alternative_vcall0(hvm_funcs.fpu_dirty_intercept);
>      else if ( type == X86EMUL_FPU_fpu )
>      {
>          const typeof(curr->arch.xsave_area->fpu_sse) *fpu_ctxt =
> @@ -2152,7 +2152,7 @@ static void hvmemul_put_fpu(
>          {
>              curr->fpu_dirtied = false;
>              stts();
> -            hvm_funcs.fpu_leave(curr);
> +            alternative_vcall1(hvm_funcs.fpu_leave, curr);
>          }
>      }
>  }
> @@ -2314,7 +2314,8 @@ static int _hvm_emulate_one(struct hvm_e
>      if ( hvmemul_ctxt->intr_shadow != new_intr_shadow )
>      {
>          hvmemul_ctxt->intr_shadow = new_intr_shadow;
> -        hvm_funcs.set_interrupt_shadow(curr, new_intr_shadow);
> +        alternative_vcall2(hvm_funcs.set_interrupt_shadow,
> +                           curr, new_intr_shadow);
>      }
> 
>      if ( hvmemul_ctxt->ctxt.retire.hlt &&
> @@ -2451,7 +2452,8 @@ void hvm_emulate_init_once(
> 
>      memset(hvmemul_ctxt, 0, sizeof(*hvmemul_ctxt));
> 
> -    hvmemul_ctxt->intr_shadow = hvm_funcs.get_interrupt_shadow(curr);
> +    hvmemul_ctxt->intr_shadow =
> +        alternative_call1(hvm_funcs.get_interrupt_shadow, curr);
>      hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt);
>      hvmemul_get_seg_reg(x86_seg_ss, hvmemul_ctxt);
> 
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -271,13 +271,24 @@ void hvm_set_rdtsc_exiting(struct domain
>  {
>      struct vcpu *v;
> 
> +#if __GNUC__ >= 7
> +/*
> + * gcc from 7.x onwards warns about ternary operators with their middle
> operand
> + * omitted and the controlling expression itself being of _Bool type.
> + */
> +# pragma GCC diagnostic push
> +# pragma GCC diagnostic ignored "-Wparentheses"
> +#endif
>      for_each_vcpu ( d, v )
> -        hvm_funcs.set_rdtsc_exiting(v, enable);
> +        alternative_vcall2(hvm_funcs.set_rdtsc_exiting, v, enable);
> +#if __GNUC__ >= 7
> +# pragma GCC diagnostic pop
> +#endif
>  }
> 
>  void hvm_get_guest_pat(struct vcpu *v, u64 *guest_pat)
>  {
> -    if ( !hvm_funcs.get_guest_pat(v, guest_pat) )
> +    if ( !alternative_call2(hvm_funcs.get_guest_pat, v, guest_pat) )
>          *guest_pat = v->arch.hvm_vcpu.pat_cr;
>  }
> 
> @@ -302,7 +313,7 @@ int hvm_set_guest_pat(struct vcpu *v, u6
>              return 0;
>          }
> 
> -    if ( !hvm_funcs.set_guest_pat(v, guest_pat) )
> +    if ( !alternative_call2(hvm_funcs.set_guest_pat, v, guest_pat) )
>          v->arch.hvm_vcpu.pat_cr = guest_pat;
> 
>      return 1;
> @@ -342,7 +353,7 @@ bool hvm_set_guest_bndcfgs(struct vcpu *
>              /* nothing, best effort only */;
>      }
> 
> -    return hvm_funcs.set_guest_bndcfgs(v, val);
> +    return alternative_call2(hvm_funcs.set_guest_bndcfgs, v, val);
>  }
> 
>  /*
> @@ -502,7 +513,8 @@ void hvm_migrate_pirqs(struct vcpu *v)
>  static bool hvm_get_pending_event(struct vcpu *v, struct x86_event *info)
>  {
>      info->cr2 = v->arch.hvm_vcpu.guest_cr[2];
> -    return hvm_funcs.get_pending_event(v, info);
> +
> +    return alternative_call2(hvm_funcs.get_pending_event, v, info);
>  }
> 
>  void hvm_do_resume(struct vcpu *v)
> @@ -1684,7 +1696,7 @@ void hvm_inject_event(const struct x86_e
>          }
>      }
> 
> -    hvm_funcs.inject_event(event);
> +    alternative_vcall1(hvm_funcs.inject_event, event);
>  }
> 
>  int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> @@ -2271,7 +2283,7 @@ int hvm_set_cr0(unsigned long value, boo
>           (!rangeset_is_empty(d->iomem_caps) ||
>            !rangeset_is_empty(d->arch.ioport_caps) ||
>            has_arch_pdevs(d)) )
> -        hvm_funcs.handle_cd(v, value);
> +        alternative_vcall2(hvm_funcs.handle_cd, v, value);
> 
>      hvm_update_cr(v, 0, value);
> 
> @@ -3515,7 +3527,8 @@ int hvm_msr_read_intercept(unsigned int
>              goto gp_fault;
>          /* If ret == 0 then this is not an MCE MSR, see other MSRs. */
>          ret = ((ret == 0)
> -               ? hvm_funcs.msr_read_intercept(msr, msr_content)
> +               ? alternative_call2(hvm_funcs.msr_read_intercept,
> +                                   msr, msr_content)
>                 : X86EMUL_OKAY);
>          break;
>      }
> @@ -3672,7 +3685,8 @@ int hvm_msr_write_intercept(unsigned int
>              goto gp_fault;
>          /* If ret == 0 then this is not an MCE MSR, see other MSRs. */
>          ret = ((ret == 0)
> -               ? hvm_funcs.msr_write_intercept(msr, msr_content)
> +               ? alternative_call2(hvm_funcs.msr_write_intercept,
> +                                   msr, msr_content)
>                 : X86EMUL_OKAY);
>          break;
>      }
> @@ -3864,7 +3878,7 @@ void hvm_hypercall_page_initialise(struc
>                                     void *hypercall_page)
>  {
>      hvm_latch_shinfo_size(d);
> -    hvm_funcs.init_hypercall_page(d, hypercall_page);
> +    alternative_vcall2(hvm_funcs.init_hypercall_page, d, hypercall_page);
>  }
> 
>  void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
> @@ -4988,7 +5002,7 @@ void hvm_domain_soft_reset(struct domain
>  void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
>                                struct segment_register *reg)
>  {
> -    hvm_funcs.get_segment_register(v, seg, reg);
> +    alternative_vcall3(hvm_funcs.get_segment_register, v, seg, reg);
> 
>      switch ( seg )
>      {
> @@ -5134,7 +5148,7 @@ void hvm_set_segment_register(struct vcp
>          return;
>      }
> 
> -    hvm_funcs.set_segment_register(v, seg, reg);
> +    alternative_vcall3(hvm_funcs.set_segment_register, v, seg, reg);
>  }
> 
>  /*
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5785,7 +5785,7 @@ void paging_invlpg(struct vcpu *v, unsig
>      if ( is_pv_vcpu(v) )
>          flush_tlb_one_local(va);
>      else
> -        hvm_funcs.invlpg(v, va);
> +        alternative_vcall2(hvm_funcs.invlpg, v, va);
>  }
> 
>  /* Build a 32bit PSE page table using 4MB pages. */
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -317,42 +317,42 @@ static inline int
>  hvm_guest_x86_mode(struct vcpu *v)
>  {
>      ASSERT(v == current);
> -    return hvm_funcs.guest_x86_mode(v);
> +    return alternative_call1(hvm_funcs.guest_x86_mode, v);
>  }
> 
>  static inline void
>  hvm_update_host_cr3(struct vcpu *v)
>  {
>      if ( hvm_funcs.update_host_cr3 )
> -        hvm_funcs.update_host_cr3(v);
> +        alternative_vcall1(hvm_funcs.update_host_cr3, v);
>  }
> 
>  static inline void hvm_update_guest_cr(struct vcpu *v, unsigned int cr)
>  {
> -    hvm_funcs.update_guest_cr(v, cr, 0);
> +    alternative_vcall3(hvm_funcs.update_guest_cr, v, cr, 0);
>  }
> 
>  static inline void hvm_update_guest_cr3(struct vcpu *v, bool noflush)
>  {
>      unsigned int flags = noflush ? HVM_UPDATE_GUEST_CR3_NOFLUSH : 0;
> 
> -    hvm_funcs.update_guest_cr(v, 3, flags);
> +    alternative_vcall3(hvm_funcs.update_guest_cr, v, 3, flags);
>  }
> 
>  static inline void hvm_update_guest_efer(struct vcpu *v)
>  {
> -    hvm_funcs.update_guest_efer(v);
> +    alternative_vcall1(hvm_funcs.update_guest_efer, v);
>  }
> 
>  static inline void hvm_cpuid_policy_changed(struct vcpu *v)
>  {
> -    hvm_funcs.cpuid_policy_changed(v);
> +    alternative_vcall1(hvm_funcs.cpuid_policy_changed, v);
>  }
> 
>  static inline void hvm_set_tsc_offset(struct vcpu *v, uint64_t offset,
>                                        uint64_t at_tsc)
>  {
> -    hvm_funcs.set_tsc_offset(v, offset, at_tsc);
> +    alternative_vcall3(hvm_funcs.set_tsc_offset, v, offset, at_tsc);
>  }
> 
>  /*
> @@ -372,7 +372,7 @@ void hvm_hypercall_page_initialise(struc
>  static inline unsigned int
>  hvm_get_cpl(struct vcpu *v)
>  {
> -    return hvm_funcs.get_cpl(v);
> +    return alternative_call1(hvm_funcs.get_cpl, v);
>  }
> 
>  void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
> @@ -382,13 +382,13 @@ void hvm_set_segment_register(struct vcp
> 
>  static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
>  {
> -    return hvm_funcs.get_shadow_gs_base(v);
> +    return alternative_call1(hvm_funcs.get_shadow_gs_base, v);
>  }
> 
>  static inline bool hvm_get_guest_bndcfgs(struct vcpu *v, u64 *val)
>  {
>      return hvm_funcs.get_guest_bndcfgs &&
> -           hvm_funcs.get_guest_bndcfgs(v, val);
> +           alternative_call2(hvm_funcs.get_guest_bndcfgs, v, val);
>  }
> 
>  bool hvm_set_guest_bndcfgs(struct vcpu *v, u64 val);
> @@ -454,7 +454,7 @@ static inline void hvm_inject_page_fault
> 
>  static inline int hvm_event_pending(struct vcpu *v)
>  {
> -    return hvm_funcs.event_pending(v);
> +    return alternative_call1(hvm_funcs.event_pending, v);
>  }
> 
>  /* These bits in CR4 are owned by the host. */
> @@ -485,7 +485,8 @@ static inline void hvm_cpu_down(void)
> 
>  static inline unsigned int hvm_get_insn_bytes(struct vcpu *v, uint8_t *buf)
>  {
> -    return (hvm_funcs.get_insn_bytes ? hvm_funcs.get_insn_bytes(v, buf) :
> 0);
> +    return (hvm_funcs.get_insn_bytes
> +            ? alternative_call2(hvm_funcs.get_insn_bytes, v, buf) : 0);
>  }
> 
>  enum hvm_task_switch_reason { TSW_jmp, TSW_iret, TSW_call_or_int };
> @@ -517,7 +518,7 @@ void hvm_mapped_guest_frames_mark_dirty(
>  static inline void hvm_set_info_guest(struct vcpu *v)
>  {
>      if ( hvm_funcs.set_info_guest )
> -        return hvm_funcs.set_info_guest(v);
> +        alternative_vcall1(hvm_funcs.set_info_guest, v);
>  }
> 
>  int hvm_debug_op(struct vcpu *v, int32_t op);
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 10/16] x86/HVM: patch indirect calls through hvm_funcs to direct ones

[Jan Beulich]

>>> On 13.07.18 at 12:06, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
>> Of Jan Beulich
>> Sent: 11 July 2018 14:43
>> To: xen-devel <xen-devel@lists.xenproject.org>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
>> Subject: [Xen-devel] [PATCH 10/16] x86/HVM: patch indirect calls through
>> hvm_funcs to direct ones
>> 
>> This is intentionally not touching hooks used rarely (or not at all)
>> during the lifetime of a VM, like {domain,vcpu}_initialise or cpu_up,
>> as well as nested, VM event, and altp2m ones (they can all be done
>> later, if so desired). Virtual Interrupt delivery ones will be dealt
>> with in a subsequent patch.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Needless to say that I'm pretty unhappy about the workaround I had to
>> add to hvm_set_rdtsc_exiting(). Improvement suggestions welcome.
> 
> Presumably the workaround is needed because of the ?: in ALT_CALL_ARG() and 
> will occur for any bool argument to an alternatives patched call?

Yes.

> If so, and 
> ad hoc workaround seems undesirable. Is it possible to suppress it by 
> dropping the gcc-ism from the ternary operator?

Hmm, yes, that looks to work. Makes the code slightly more ugly,
though. And I dislike this because of the warning being pointless
when such an expression occurs in sizeof(), alignof(), or typeof()
(i.e. when the expression isn't really evaluated). But perhaps
that's less ugly than the current warning suppression.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 11/16] x86/HVM: patch vINTR indirect calls through hvm_funcs to direct ones

[Jan Beulich]

While not strictly necessary, change the VMX initialization logic to
update the function table in start_vmx() from NULL rather than to NULL,
to make more obvious that we won't ever change an already (explictly)
initialized function pointer.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -111,10 +111,15 @@ static void vlapic_clear_irr(int vector,
     vlapic_clear_vector(vector, &vlapic->regs->data[APIC_IRR]);
 }
 
-static int vlapic_find_highest_irr(struct vlapic *vlapic)
+static void sync_pir_to_irr(struct vcpu *v)
 {
     if ( hvm_funcs.sync_pir_to_irr )
-        hvm_funcs.sync_pir_to_irr(vlapic_vcpu(vlapic));
+        alternative_vcall1(hvm_funcs.sync_pir_to_irr, v);
+}
+
+static int vlapic_find_highest_irr(struct vlapic *vlapic)
+{
+    sync_pir_to_irr(vlapic_vcpu(vlapic));
 
     return vlapic_find_highest_vector(&vlapic->regs->data[APIC_IRR]);
 }
@@ -143,7 +148,7 @@ bool vlapic_test_irq(const struct vlapic
         return false;
 
     if ( hvm_funcs.test_pir &&
-         hvm_funcs.test_pir(const_vlapic_vcpu(vlapic), vec) )
+         alternative_call2(hvm_funcs.test_pir, const_vlapic_vcpu(vlapic), vec) )
         return true;
 
     return vlapic_test_vector(vec, &vlapic->regs->data[APIC_IRR]);
@@ -165,10 +170,10 @@ void vlapic_set_irq(struct vlapic *vlapi
         vlapic_clear_vector(vec, &vlapic->regs->data[APIC_TMR]);
 
     if ( hvm_funcs.update_eoi_exit_bitmap )
-        hvm_funcs.update_eoi_exit_bitmap(target, vec, trig);
+        alternative_vcall3(hvm_funcs.update_eoi_exit_bitmap, target, vec, trig);
 
     if ( hvm_funcs.deliver_posted_intr )
-        hvm_funcs.deliver_posted_intr(target, vec);
+        alternative_vcall2(hvm_funcs.deliver_posted_intr, target, vec);
     else if ( !vlapic_test_and_set_irr(vec, vlapic) )
         vcpu_kick(target);
 }
@@ -448,7 +453,7 @@ void vlapic_EOI_set(struct vlapic *vlapi
     vlapic_clear_vector(vector, &vlapic->regs->data[APIC_ISR]);
 
     if ( hvm_funcs.handle_eoi )
-        hvm_funcs.handle_eoi(vector);
+        alternative_vcall1(hvm_funcs.handle_eoi, vector);
 
     vlapic_handle_EOI(vlapic, vector);
 
@@ -1457,8 +1462,7 @@ static int lapic_save_regs(struct domain
 
     for_each_vcpu ( d, v )
     {
-        if ( hvm_funcs.sync_pir_to_irr )
-            hvm_funcs.sync_pir_to_irr(v);
+        sync_pir_to_irr(v);
 
         s = vcpu_vlapic(v);
         if ( (rc = hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, s->regs)) != 0 )
@@ -1561,7 +1565,8 @@ static int lapic_load_regs(struct domain
         lapic_load_fixup(s);
 
     if ( hvm_funcs.process_isr )
-        hvm_funcs.process_isr(vlapic_find_highest_isr(s), v);
+        alternative_vcall2(hvm_funcs.process_isr,
+                           vlapic_find_highest_isr(s), v);
 
     vlapic_adjust_i8259_target(d);
     lapic_rearm(s);
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2280,12 +2280,6 @@ static struct hvm_function_table __initd
     .nhvm_vcpu_vmexit_event = nvmx_vmexit_event,
     .nhvm_intr_blocked    = nvmx_intr_blocked,
     .nhvm_domain_relinquish_resources = nvmx_domain_relinquish_resources,
-    .update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap,
-    .process_isr          = vmx_process_isr,
-    .deliver_posted_intr  = vmx_deliver_posted_intr,
-    .sync_pir_to_irr      = vmx_sync_pir_to_irr,
-    .test_pir             = vmx_test_pir,
-    .handle_eoi           = vmx_handle_eoi,
     .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m,
     .enable_msr_interception = vmx_enable_msr_interception,
     .is_singlestep_supported = vmx_is_singlestep_supported,
@@ -2413,26 +2407,23 @@ const struct hvm_function_table * __init
         setup_ept_dump();
     }
 
-    if ( !cpu_has_vmx_virtual_intr_delivery )
+    if ( cpu_has_vmx_virtual_intr_delivery )
     {
-        vmx_function_table.update_eoi_exit_bitmap = NULL;
-        vmx_function_table.process_isr = NULL;
-        vmx_function_table.handle_eoi = NULL;
-    }
-    else
+        vmx_function_table.update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap;
+        vmx_function_table.process_isr = vmx_process_isr;
+        vmx_function_table.handle_eoi = vmx_handle_eoi;
         vmx_function_table.virtual_intr_delivery_enabled = true;
+    }
 
     if ( cpu_has_vmx_posted_intr_processing )
     {
         alloc_direct_apic_vector(&posted_intr_vector, pi_notification_interrupt);
         if ( iommu_intpost )
             alloc_direct_apic_vector(&pi_wakeup_vector, pi_wakeup_interrupt);
-    }
-    else
-    {
-        vmx_function_table.deliver_posted_intr = NULL;
-        vmx_function_table.sync_pir_to_irr = NULL;
-        vmx_function_table.test_pir = NULL;
+
+        vmx_function_table.deliver_posted_intr = vmx_deliver_posted_intr;
+        vmx_function_table.sync_pir_to_irr     = vmx_sync_pir_to_irr;
+        vmx_function_table.test_pir            = vmx_test_pir;
     }
 
     if ( cpu_has_vmx_tsc_scaling )




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 11/16] x86/HVM: patch vINTR indirect calls through hvm_funcs to direct ones

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, July 11, 2018 9:44 PM
> 
> While not strictly necessary, change the VMX initialization logic to
> update the function table in start_vmx() from NULL rather than to NULL,
> to make more obvious that we won't ever change an already (explictly)
> initialized function pointer.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 12/16] x86: patch ctxt_switch_masking() indirect call to direct one

[Jan Beulich]

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -196,7 +196,7 @@ void ctxt_switch_levelling(const struct
 	}
 
 	if (ctxt_switch_masking)
-		ctxt_switch_masking(next);
+		alternative_vcall1(ctxt_switch_masking, next);
 }
 
 bool_t opt_cpu_info;



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 13/16] x86/genapic: drop .target_cpus() hook

[Jan Beulich]

All flavors specify target_cpus_all() anyway - replace use of the hook
by &cpu_online_map.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/genapic/delivery.c
+++ b/xen/arch/x86/genapic/delivery.c
@@ -5,12 +5,6 @@
 #include <asm/hardirq.h>
 #include <mach_apic.h>
 
-
-const cpumask_t *target_cpus_all(void)
-{
-	return &cpu_online_map;
-}
-
 /*
  * LOGICAL FLAT DELIVERY MODE (multicast via bitmask to <= 8 logical APIC IDs).
  */
--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -169,7 +169,6 @@ static const struct genapic apic_x2apic_
     .int_dest_mode = 0 /* physical delivery */,
     .init_apic_ldr = init_apic_ldr_x2apic_phys,
     .clustered_apic_check = clustered_apic_check_x2apic,
-    .target_cpus = target_cpus_all,
     .vector_allocation_cpumask = vector_allocation_cpumask_phys,
     .cpu_mask_to_apicid = cpu_mask_to_apicid_phys,
     .send_IPI_mask = send_IPI_mask_x2apic_phys,
@@ -182,7 +181,6 @@ static const struct genapic apic_x2apic_
     .int_dest_mode = 1 /* logical delivery */,
     .init_apic_ldr = init_apic_ldr_x2apic_cluster,
     .clustered_apic_check = clustered_apic_check_x2apic,
-    .target_cpus = target_cpus_all,
     .vector_allocation_cpumask = vector_allocation_cpumask_x2apic_cluster,
     .cpu_mask_to_apicid = cpu_mask_to_apicid_x2apic_cluster,
     .send_IPI_mask = send_IPI_mask_x2apic_cluster,
--- a/xen/include/asm-x86/genapic.h
+++ b/xen/include/asm-x86/genapic.h
@@ -33,7 +33,6 @@ struct genapic {
 	int int_dest_mode;
 	void (*init_apic_ldr)(void);
 	void (*clustered_apic_check)(void);
-	const cpumask_t *(*target_cpus)(void);
 	const cpumask_t *(*vector_allocation_cpumask)(int cpu);
 	unsigned int (*cpu_mask_to_apicid)(const cpumask_t *cpumask);
 	void (*send_IPI_mask)(const cpumask_t *mask, int vector);
@@ -51,7 +50,6 @@ struct genapic {
 extern const struct genapic *genapic;
 extern const struct genapic apic_default;
 
-const cpumask_t *target_cpus_all(void);
 void send_IPI_self_legacy(uint8_t vector);
 
 void init_apic_ldr_flat(void);
@@ -64,7 +62,6 @@ const cpumask_t *vector_allocation_cpuma
 	.int_dest_mode = 1 /* logical delivery */, \
 	.init_apic_ldr = init_apic_ldr_flat, \
 	.clustered_apic_check = clustered_apic_check_flat, \
-	.target_cpus = target_cpus_all, \
 	.vector_allocation_cpumask = vector_allocation_cpumask_flat, \
 	.cpu_mask_to_apicid = cpu_mask_to_apicid_flat, \
 	.send_IPI_mask = send_IPI_mask_flat, \
@@ -80,7 +77,6 @@ const cpumask_t *vector_allocation_cpuma
 	.int_dest_mode = 0 /* physical delivery */, \
 	.init_apic_ldr = init_apic_ldr_phys, \
 	.clustered_apic_check = clustered_apic_check_phys, \
-	.target_cpus = target_cpus_all, \
 	.vector_allocation_cpumask = vector_allocation_cpumask_phys, \
 	.cpu_mask_to_apicid = cpu_mask_to_apicid_phys, \
 	.send_IPI_mask = send_IPI_mask_phys, \
--- a/xen/include/asm-x86/mach-generic/mach_apic.h
+++ b/xen/include/asm-x86/mach-generic/mach_apic.h
@@ -12,7 +12,7 @@
 /* The following are dependent on APIC delivery mode (logical vs. physical). */
 #define INT_DELIVERY_MODE (genapic->int_delivery_mode)
 #define INT_DEST_MODE (genapic->int_dest_mode)
-#define TARGET_CPUS	  (genapic->target_cpus())
+#define TARGET_CPUS ((const typeof(cpu_online_map) *)&cpu_online_map)
 #define init_apic_ldr (genapic->init_apic_ldr)
 #define clustered_apic_check (genapic->clustered_apic_check) 
 #define cpu_mask_to_apicid (genapic->cpu_mask_to_apicid)





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 14/16] x86/genapic: remove indirection from genapic hook accesses

[Jan Beulich]

Instead of loading a pointer at each use site, have a single runtime
instance of struct genapic, copying into it from the individual
instances. The individual instances can this way also be moved to .init
(also adjust apic_probe[] at this occasion).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -944,8 +944,8 @@ void __init x2apic_bsp_setup(void)
 
     force_iommu = 1;
 
-    genapic = apic_x2apic_probe();
-    printk("Switched to APIC driver %s.\n", genapic->name);
+    genapic = *apic_x2apic_probe();
+    printk("Switched to APIC driver %s.\n", genapic.name);
 
     if ( !x2apic_enabled )
     {
--- a/xen/arch/x86/genapic/bigsmp.c
+++ b/xen/arch/x86/genapic/bigsmp.c
@@ -42,7 +42,7 @@ static __init int probe_bigsmp(void)
 	return def_to_bigsmp;
 } 
 
-const struct genapic apic_bigsmp = {
+const struct genapic __initconstrel apic_bigsmp = {
 	APIC_INIT("bigsmp", probe_bigsmp),
 	GENAPIC_PHYS
 };
--- a/xen/arch/x86/genapic/default.c
+++ b/xen/arch/x86/genapic/default.c
@@ -20,7 +20,7 @@ static __init int probe_default(void)
 	return 1;
 } 
 
-const struct genapic apic_default = {
+const struct genapic __initconstrel apic_default = {
 	APIC_INIT("default", probe_default),
 	GENAPIC_FLAT
 };
--- a/xen/arch/x86/genapic/probe.c
+++ b/xen/arch/x86/genapic/probe.c
@@ -15,11 +15,9 @@
 #include <asm/mach-generic/mach_apic.h>
 #include <asm/setup.h>
 
-extern const struct genapic apic_bigsmp;
+struct genapic __read_mostly genapic;
 
-const struct genapic *__read_mostly genapic;
-
-const struct genapic *apic_probe[] __initdata = {
+const struct genapic *const __initconstrel apic_probe[] = {
 	&apic_bigsmp, 
 	&apic_default,	/* must be last */
 	NULL,
@@ -36,11 +34,11 @@ void __init generic_bigsmp_probe(void)
 	 * - we find more than 8 CPUs in acpi LAPIC listing with xAPIC support
 	 */
 
-	if (!cmdline_apic && genapic == &apic_default)
+	if (!cmdline_apic && genapic.name == apic_default.name)
 		if (apic_bigsmp.probe()) {
-			genapic = &apic_bigsmp;
+			genapic = apic_bigsmp;
 			printk(KERN_INFO "Overriding APIC driver with %s\n",
-			       genapic->name);
+			       genapic.name);
 		}
 }
 
@@ -50,7 +48,7 @@ static int __init genapic_apic_force(con
 
 	for (i = 0; apic_probe[i]; i++)
 		if (!strcmp(apic_probe[i]->name, str)) {
-			genapic = apic_probe[i];
+			genapic = *apic_probe[i];
 			rc = 0;
 		}
 
@@ -66,18 +64,18 @@ void __init generic_apic_probe(void)
 	record_boot_APIC_mode();
 
 	check_x2apic_preenabled();
-	cmdline_apic = changed = (genapic != NULL);
+	cmdline_apic = changed = !!genapic.name;
 
 	for (i = 0; !changed && apic_probe[i]; i++) { 
 		if (apic_probe[i]->probe()) {
 			changed = 1;
-			genapic = apic_probe[i];
+			genapic = *apic_probe[i];
 		} 
 	}
 	if (!changed) 
-		genapic = &apic_default;
+		genapic = apic_default;
 
-	printk(KERN_INFO "Using APIC driver %s\n", genapic->name);
+	printk(KERN_INFO "Using APIC driver %s\n", genapic.name);
 } 
 
 /* These functions can switch the APIC even after the initial ->probe() */
@@ -88,9 +86,9 @@ int __init mps_oem_check(struct mp_confi
 	for (i = 0; apic_probe[i]; ++i) { 
 		if (apic_probe[i]->mps_oem_check(mpc,oem,productid)) { 
 			if (!cmdline_apic) {
-				genapic = apic_probe[i];
+				genapic = *apic_probe[i];
 				printk(KERN_INFO "Switched to APIC driver `%s'.\n", 
-				       genapic->name);
+				       genapic.name);
 			}
 			return 1;
 		} 
@@ -104,9 +102,9 @@ int __init acpi_madt_oem_check(char *oem
 	for (i = 0; apic_probe[i]; ++i) { 
 		if (apic_probe[i]->acpi_madt_oem_check(oem_id, oem_table_id)) { 
 			if (!cmdline_apic) {
-				genapic = apic_probe[i];
+				genapic = *apic_probe[i];
 				printk(KERN_INFO "Switched to APIC driver `%s'.\n", 
-				       genapic->name);
+				       genapic.name);
 			}
 			return 1;
 		} 
--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -163,7 +163,7 @@ static void send_IPI_mask_x2apic_cluster
     local_irq_restore(flags);
 }
 
-static const struct genapic apic_x2apic_phys = {
+static const struct genapic __initconstrel apic_x2apic_phys = {
     APIC_INIT("x2apic_phys", NULL),
     .int_delivery_mode = dest_Fixed,
     .int_dest_mode = 0 /* physical delivery */,
@@ -175,7 +175,7 @@ static const struct genapic apic_x2apic_
     .send_IPI_self = send_IPI_self_x2apic
 };
 
-static const struct genapic apic_x2apic_cluster = {
+static const struct genapic __initconstrel apic_x2apic_cluster = {
     APIC_INIT("x2apic_cluster", NULL),
     .int_delivery_mode = dest_LowestPrio,
     .int_dest_mode = 1 /* logical delivery */,
@@ -263,6 +263,6 @@ void __init check_x2apic_preenabled(void
     {
         printk("x2APIC mode is already enabled by BIOS.\n");
         x2apic_enabled = 1;
-        genapic = apic_x2apic_probe();
+        genapic = *apic_x2apic_probe();
     }
 }
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -163,7 +163,8 @@ static int MP_processor_info_x(struct mp
 		return -ENOSPC;
 	}
 
-	if (num_processors >= 8 && hotplug && genapic == &apic_default) {
+	if (num_processors >= 8 && hotplug
+	    && genapic.name == apic_default.name) {
 		printk(KERN_WARNING "WARNING: CPUs limit of 8 reached."
 			" Processor ignored.\n");
 		return -ENOSPC;
--- a/xen/arch/x86/smp.c
+++ b/xen/arch/x86/smp.c
@@ -29,12 +29,12 @@
 
 void send_IPI_mask(const cpumask_t *mask, int vector)
 {
-    genapic->send_IPI_mask(mask, vector);
+    genapic.send_IPI_mask(mask, vector);
 }
 
 void send_IPI_self(int vector)
 {
-    genapic->send_IPI_self(vector);
+    genapic.send_IPI_self(vector);
 }
 
 /*
--- a/xen/include/asm-x86/genapic.h
+++ b/xen/include/asm-x86/genapic.h
@@ -47,8 +47,9 @@ struct genapic {
 	APICFUNC(mps_oem_check), \
 	APICFUNC(acpi_madt_oem_check)
 
-extern const struct genapic *genapic;
+extern struct genapic genapic;
 extern const struct genapic apic_default;
+extern const struct genapic apic_bigsmp;
 
 void send_IPI_self_legacy(uint8_t vector);
 
--- a/xen/include/asm-x86/mach-generic/mach_apic.h
+++ b/xen/include/asm-x86/mach-generic/mach_apic.h
@@ -10,13 +10,13 @@
 #define esr_disable (0)
 
 /* The following are dependent on APIC delivery mode (logical vs. physical). */
-#define INT_DELIVERY_MODE (genapic->int_delivery_mode)
-#define INT_DEST_MODE (genapic->int_dest_mode)
+#define INT_DELIVERY_MODE (genapic.int_delivery_mode)
+#define INT_DEST_MODE (genapic.int_dest_mode)
 #define TARGET_CPUS ((const typeof(cpu_online_map) *)&cpu_online_map)
-#define init_apic_ldr (genapic->init_apic_ldr)
-#define clustered_apic_check (genapic->clustered_apic_check) 
-#define cpu_mask_to_apicid (genapic->cpu_mask_to_apicid)
-#define vector_allocation_cpumask(cpu) (genapic->vector_allocation_cpumask(cpu))
+#define init_apic_ldr (genapic.init_apic_ldr)
+#define clustered_apic_check (genapic.clustered_apic_check)
+#define cpu_mask_to_apicid (genapic.cpu_mask_to_apicid)
+#define vector_allocation_cpumask(cpu) (genapic.vector_allocation_cpumask(cpu))
 
 static inline void enable_apic_mode(void)
 {




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 15/16] x86/genapic: patch indirect calls to direct ones

[Jan Beulich]

For (I hope) obvious reasons only the ones used at runtime get
converted.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/smp.c
+++ b/xen/arch/x86/smp.c
@@ -29,12 +29,12 @@
 
 void send_IPI_mask(const cpumask_t *mask, int vector)
 {
-    genapic.send_IPI_mask(mask, vector);
+    alternative_vcall2(genapic.send_IPI_mask, mask, vector);
 }
 
 void send_IPI_self(int vector)
 {
-    genapic.send_IPI_self(vector);
+    alternative_vcall1(genapic.send_IPI_self, vector);
 }
 
 /*
--- a/xen/include/asm-x86/mach-generic/mach_apic.h
+++ b/xen/include/asm-x86/mach-generic/mach_apic.h
@@ -15,8 +15,18 @@
 #define TARGET_CPUS ((const typeof(cpu_online_map) *)&cpu_online_map)
 #define init_apic_ldr (genapic.init_apic_ldr)
 #define clustered_apic_check (genapic.clustered_apic_check)
-#define cpu_mask_to_apicid (genapic.cpu_mask_to_apicid)
-#define vector_allocation_cpumask(cpu) (genapic.vector_allocation_cpumask(cpu))
+#define cpu_mask_to_apicid(mask) ({ \
+	/* \
+	 * There are a number of places where the address of a local variable \
+	 * gets passed here. The use of ?: in alternative_call1() triggers an \
+	 * "address of ... is always true" warning in such a case with at least \
+	 * gcc 7. Hence the seemingly pointless local variable here. \
+	 */ \
+	const cpumask_t *m_ = (mask); \
+	alternative_call1(genapic.cpu_mask_to_apicid, m_); \
+})
+#define vector_allocation_cpumask(cpu) \
+	alternative_call1(genapic.vector_allocation_cpumask, cpu)
 
 static inline void enable_apic_mode(void)
 {





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 16/16] x86/cpuidle: patch some indirect calls to direct ones

[Jan Beulich]

For now only the ones used during entering/exiting of idle states are
converted. Additionally pm_idle{,_save} and lapic_timer_{on,off} can't
be converted, as they may get established rather late (when Dom0 is
already active).

Note that for patching to be deferred until after the pre-SMP initcalls
(from where cpuidle_init_cpu() runs the first time) the pointers need to
start out as NULL.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -102,8 +102,6 @@ bool lapic_timer_init(void)
     return true;
 }
 
-static uint64_t (*__read_mostly tick_to_ns)(uint64_t) = acpi_pm_tick_to_ns;
-
 void (*__read_mostly pm_idle_save)(void);
 unsigned int max_cstate __read_mostly = ACPI_PROCESSOR_MAX_POWER - 1;
 integer_param("max_cstate", max_cstate);
@@ -289,9 +287,9 @@ static uint64_t acpi_pm_ticks_elapsed(ui
         return ((0xFFFFFFFF - t1) + t2 +1);
 }
 
-uint64_t (*__read_mostly cpuidle_get_tick)(void) = get_acpi_pm_tick;
-static uint64_t (*__read_mostly ticks_elapsed)(uint64_t, uint64_t)
-    = acpi_pm_ticks_elapsed;
+uint64_t (*__read_mostly cpuidle_get_tick)(void);
+static uint64_t (*__read_mostly tick_to_ns)(uint64_t);
+static uint64_t (*__read_mostly ticks_elapsed)(uint64_t, uint64_t);
 
 static void print_acpi_power(uint32_t cpu, struct acpi_processor_power *power)
 {
@@ -547,7 +545,7 @@ void update_idle_stats(struct acpi_proce
                        struct acpi_processor_cx *cx,
                        uint64_t before, uint64_t after)
 {
-    int64_t sleep_ticks = ticks_elapsed(before, after);
+    int64_t sleep_ticks = alternative_call2(ticks_elapsed, before, after);
     /* Interrupts are disabled */
 
     spin_lock(&power->stat_lock);
@@ -555,7 +553,8 @@ void update_idle_stats(struct acpi_proce
     cx->usage++;
     if ( sleep_ticks > 0 )
     {
-        power->last_residency = tick_to_ns(sleep_ticks) / 1000UL;
+        power->last_residency = alternative_call1(tick_to_ns, sleep_ticks) /
+                                1000UL;
         cx->time += sleep_ticks;
     }
     power->last_state = &power->states[0];
@@ -635,7 +634,7 @@ static void acpi_processor_idle(void)
         if ( cx->type == ACPI_STATE_C1 || local_apic_timer_c2_ok )
         {
             /* Get start time (ticks) */
-            t1 = cpuidle_get_tick();
+            t1 = alternative_call0(cpuidle_get_tick);
             /* Trace cpu idle entry */
             TRACE_4D(TRC_PM_IDLE_ENTRY, cx->idx, t1, exp, pred);
 
@@ -644,7 +643,7 @@ static void acpi_processor_idle(void)
             /* Invoke C2 */
             acpi_idle_do_entry(cx);
             /* Get end time (ticks) */
-            t2 = cpuidle_get_tick();
+            t2 = alternative_call0(cpuidle_get_tick);
             trace_exit_reason(irq_traced);
             /* Trace cpu idle exit */
             TRACE_6D(TRC_PM_IDLE_EXIT, cx->idx, t2,
@@ -666,7 +665,7 @@ static void acpi_processor_idle(void)
         lapic_timer_off();
 
         /* Get start time (ticks) */
-        t1 = cpuidle_get_tick();
+        t1 = alternative_call0(cpuidle_get_tick);
         /* Trace cpu idle entry */
         TRACE_4D(TRC_PM_IDLE_ENTRY, cx->idx, t1, exp, pred);
 
@@ -717,7 +716,7 @@ static void acpi_processor_idle(void)
         }
 
         /* Get end time (ticks) */
-        t2 = cpuidle_get_tick();
+        t2 = alternative_call0(cpuidle_get_tick);
 
         /* recovering TSC */
         cstate_restore_tsc();
@@ -827,11 +826,20 @@ int cpuidle_init_cpu(unsigned int cpu)
     {
         unsigned int i;
 
-        if ( cpu == 0 && boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
+        if ( cpu == 0 && system_state < SYS_STATE_active )
         {
-            cpuidle_get_tick = get_stime_tick;
-            ticks_elapsed = stime_ticks_elapsed;
-            tick_to_ns = stime_tick_to_ns;
+            if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
+            {
+                cpuidle_get_tick = get_stime_tick;
+                ticks_elapsed = stime_ticks_elapsed;
+                tick_to_ns = stime_tick_to_ns;
+            }
+            else
+            {
+                cpuidle_get_tick = get_acpi_pm_tick;
+                ticks_elapsed = acpi_pm_ticks_elapsed;
+                tick_to_ns = acpi_pm_tick_to_ns;
+            }
         }
 
         acpi_power = xzalloc(struct acpi_processor_power);
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -778,7 +778,7 @@ static void mwait_idle(void)
 	if (!(lapic_timer_reliable_states & (1 << cstate)))
 		lapic_timer_off();
 
-	before = cpuidle_get_tick();
+	before = alternative_call0(cpuidle_get_tick);
 	TRACE_4D(TRC_PM_IDLE_ENTRY, cx->type, before, exp, pred);
 
 	update_last_cx_stat(power, cx, before);
@@ -786,7 +786,7 @@ static void mwait_idle(void)
 	if (cpu_is_haltable(cpu))
 		mwait_idle_with_hints(eax, MWAIT_ECX_INTERRUPT_BREAK);
 
-	after = cpuidle_get_tick();
+	after = alternative_call0(cpuidle_get_tick);
 
 	cstate_restore_tsc();
 	trace_exit_reason(irq_traced);




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/16] x86: indirect call overhead reduction

[Jan Beulich]

>>> On 11.07.18 at 15:15, <JBeulich@suse.com> wrote:
> While indirect calls have always been more expensive than direct ones,
> their cost has further increased with the Spectre v2 mitigations. In a
> number of cases we simply pointlessly use them in the first place. In
> many other cases the indirection solely exists to abstract from e.g.
> vendor specific hardware details, and hence the pointers used never
> change once set. Here we can use alternatives patching to get rid of
> the indirection.
> 
> From patch 8 onwards dependencies exist on earlier, yet to be reviewed
> patches ("x86/alternatives: fully leverage automatic NOP filling" as well
> as the "x86: improve PDX <-> PFN and alike translations" series at the
> very least). I nevertheless wanted to enable a first round of review of
> the series, the more that some of the patches (not just initial ones)
> could perhaps be taken irrespective of those dependencies.
> 
> Further areas where indirect calls could be eliminated (and that I've put
> on my todo list in case the general concept here is deemed reasonable)
> are IOMMU, cpufreq, vPMU, and XSM. For some of these, the ARM side
> would need dealing with as well - I'm not sure whether replacing indirect
> calls by direct ones is worthwhile there as well; if not, the wrappers
> would simply need to become function invocations in the ARM case.

Btw, I didn't want to Cc you on the whole series, but input on the above
would certainly be helpful to decide how to deal with indirect calls in code
shared between the architectures.

Thanks, Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/16] x86: indirect call overhead reduction

[Julien Grall]

Hi Jan,

On 13/07/18 09:10, Jan Beulich wrote:
>>>> On 11.07.18 at 15:15, <JBeulich@suse.com> wrote:
>> While indirect calls have always been more expensive than direct ones,
>> their cost has further increased with the Spectre v2 mitigations. In a
>> number of cases we simply pointlessly use them in the first place. In
>> many other cases the indirection solely exists to abstract from e.g.
>> vendor specific hardware details, and hence the pointers used never
>> change once set. Here we can use alternatives patching to get rid of
>> the indirection.
>>
>>  From patch 8 onwards dependencies exist on earlier, yet to be reviewed
>> patches ("x86/alternatives: fully leverage automatic NOP filling" as well
>> as the "x86: improve PDX <-> PFN and alike translations" series at the
>> very least). I nevertheless wanted to enable a first round of review of
>> the series, the more that some of the patches (not just initial ones)
>> could perhaps be taken irrespective of those dependencies.
>>
>> Further areas where indirect calls could be eliminated (and that I've put
>> on my todo list in case the general concept here is deemed reasonable)
>> are IOMMU, cpufreq, vPMU, and XSM. For some of these, the ARM side
>> would need dealing with as well - I'm not sure whether replacing indirect
>> calls by direct ones is worthwhile there as well; if not, the wrappers
>> would simply need to become function invocations in the ARM case.
> 
> Btw, I didn't want to Cc you on the whole series, but input on the above
> would certainly be helpful to decide how to deal with indirect calls in code
> shared between the architectures.

For the IOMMU, we want to keep the indirect call as it might be possible 
to have a platform support different IOMMUs (not yet supported in Xen).

For CPUFreq and vPMU, they are not yet supported on Arm and I haven't 
spent much time looking at them. So I can't confirm whether indirect 
call could be dropped.

For XSM, I guess indirect call could be dropped.

What would be the generic interface here? I saw it was based on 
alternative for the plumbing.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/16] x86: indirect call overhead reduction

[Jan Beulich]

>>> On 13.07.18 at 15:00, <julien.grall@arm.com> wrote:
> Hi Jan,
> 
> On 13/07/18 09:10, Jan Beulich wrote:
>>>>> On 11.07.18 at 15:15, <JBeulich@suse.com> wrote:
>>> While indirect calls have always been more expensive than direct ones,
>>> their cost has further increased with the Spectre v2 mitigations. In a
>>> number of cases we simply pointlessly use them in the first place. In
>>> many other cases the indirection solely exists to abstract from e.g.
>>> vendor specific hardware details, and hence the pointers used never
>>> change once set. Here we can use alternatives patching to get rid of
>>> the indirection.
>>>
>>>  From patch 8 onwards dependencies exist on earlier, yet to be reviewed
>>> patches ("x86/alternatives: fully leverage automatic NOP filling" as well
>>> as the "x86: improve PDX <-> PFN and alike translations" series at the
>>> very least). I nevertheless wanted to enable a first round of review of
>>> the series, the more that some of the patches (not just initial ones)
>>> could perhaps be taken irrespective of those dependencies.
>>>
>>> Further areas where indirect calls could be eliminated (and that I've put
>>> on my todo list in case the general concept here is deemed reasonable)
>>> are IOMMU, cpufreq, vPMU, and XSM. For some of these, the ARM side
>>> would need dealing with as well - I'm not sure whether replacing indirect
>>> calls by direct ones is worthwhile there as well; if not, the wrappers
>>> would simply need to become function invocations in the ARM case.
>> 
>> Btw, I didn't want to Cc you on the whole series, but input on the above
>> would certainly be helpful to decide how to deal with indirect calls in code
>> shared between the architectures.
> 
> For the IOMMU, we want to keep the indirect call as it might be possible 
> to have a platform support different IOMMUs (not yet supported in Xen).
> 
> For CPUFreq and vPMU, they are not yet supported on Arm and I haven't 
> spent much time looking at them. So I can't confirm whether indirect 
> call could be dropped.
> 
> For XSM, I guess indirect call could be dropped.

But you didn't answer the fundamental question: Is this worthwhile on
ARM?

> What would be the generic interface here? I saw it was based on 
> alternative for the plumbing.

Yes, I'd prefer to use the same mechanism as presented in the series.
As per above for the IOMMU case we'd then need another abstraction
layer put in the middle (to produce a patch site on x86, but a normal
[indirect] call on ARM).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/16] x86: indirect call overhead reduction

[Julien Grall]

Hi,

On 13/07/18 14:27, Jan Beulich wrote:
>>>> On 13.07.18 at 15:00, <julien.grall@arm.com> wrote:
>> Hi Jan,
>>
>> On 13/07/18 09:10, Jan Beulich wrote:
>>>>>> On 11.07.18 at 15:15, <JBeulich@suse.com> wrote:
>>>> While indirect calls have always been more expensive than direct ones,
>>>> their cost has further increased with the Spectre v2 mitigations. In a
>>>> number of cases we simply pointlessly use them in the first place. In
>>>> many other cases the indirection solely exists to abstract from e.g.
>>>> vendor specific hardware details, and hence the pointers used never
>>>> change once set. Here we can use alternatives patching to get rid of
>>>> the indirection.
>>>>
>>>>   From patch 8 onwards dependencies exist on earlier, yet to be reviewed
>>>> patches ("x86/alternatives: fully leverage automatic NOP filling" as well
>>>> as the "x86: improve PDX <-> PFN and alike translations" series at the
>>>> very least). I nevertheless wanted to enable a first round of review of
>>>> the series, the more that some of the patches (not just initial ones)
>>>> could perhaps be taken irrespective of those dependencies.
>>>>
>>>> Further areas where indirect calls could be eliminated (and that I've put
>>>> on my todo list in case the general concept here is deemed reasonable)
>>>> are IOMMU, cpufreq, vPMU, and XSM. For some of these, the ARM side
>>>> would need dealing with as well - I'm not sure whether replacing indirect
>>>> calls by direct ones is worthwhile there as well; if not, the wrappers
>>>> would simply need to become function invocations in the ARM case.
>>>
>>> Btw, I didn't want to Cc you on the whole series, but input on the above
>>> would certainly be helpful to decide how to deal with indirect calls in code
>>> shared between the architectures.
>>
>> For the IOMMU, we want to keep the indirect call as it might be possible
>> to have a platform support different IOMMUs (not yet supported in Xen).
>>
>> For CPUFreq and vPMU, they are not yet supported on Arm and I haven't
>> spent much time looking at them. So I can't confirm whether indirect
>> call could be dropped.
>>
>> For XSM, I guess indirect call could be dropped.
> 
> But you didn't answer the fundamental question: Is this worthwhile on
> ARM?
I can't tell, that will depend on the implementation. Although such 
changes should not hurt.

> 
>> What would be the generic interface here? I saw it was based on
>> alternative for the plumbing.
> 
> Yes, I'd prefer to use the same mechanism as presented in the series.
> As per above for the IOMMU case we'd then need another abstraction
> layer put in the middle (to produce a patch site on x86, but a normal
> [indirect] call on ARM).

I will have a look. Could you point to the patch adding the abstraction?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/16] x86: indirect call overhead reduction

[Jan Beulich]

>>> On 13.07.18 at 15:39, <julien.grall@arm.com> wrote:
> On 13/07/18 14:27, Jan Beulich wrote:
>>>>> On 13.07.18 at 15:00, <julien.grall@arm.com> wrote:
>>> What would be the generic interface here? I saw it was based on
>>> alternative for the plumbing.
>> 
>> Yes, I'd prefer to use the same mechanism as presented in the series.
>> As per above for the IOMMU case we'd then need another abstraction
>> layer put in the middle (to produce a patch site on x86, but a normal
>> [indirect] call on ARM).
> 
> I will have a look. Could you point to the patch adding the abstraction?

Patch 9 is where the x86 side infrastructure gets added. Typical uses
would then be (taken from patch 10)

 hvm_guest_x86_mode(struct vcpu *v)
 {
     ASSERT(v == current);
-    return hvm_funcs.guest_x86_mode(v);
+    return alternative_call1(hvm_funcs.guest_x86_mode, v);
 }
 
 static inline void
 hvm_update_host_cr3(struct vcpu *v)
 {
     if ( hvm_funcs.update_host_cr3 )
-        hvm_funcs.update_host_cr3(v);
+        alternative_vcall1(hvm_funcs.update_host_cr3, v);
 }

And actually there was one more question I meant to ask: Are all
locations within Xen reachable by direct calls on ARM? I.e. can an
indirect call, when we know its target, always be replaced, no
matter at what address (within Xen) it lives?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/16] x86: indirect call overhead reduction

[Julien Grall]

Hi Jan,

On 13/07/18 15:27, Jan Beulich wrote:
>>>> On 13.07.18 at 15:39, <julien.grall@arm.com> wrote:
>> On 13/07/18 14:27, Jan Beulich wrote:
>>>>>> On 13.07.18 at 15:00, <julien.grall@arm.com> wrote:
>>>> What would be the generic interface here? I saw it was based on
>>>> alternative for the plumbing.
>>>
>>> Yes, I'd prefer to use the same mechanism as presented in the series.
>>> As per above for the IOMMU case we'd then need another abstraction
>>> layer put in the middle (to produce a patch site on x86, but a normal
>>> [indirect] call on ARM).
>>
>> I will have a look. Could you point to the patch adding the abstraction?
> 
> Patch 9 is where the x86 side infrastructure gets added. Typical uses
> would then be (taken from patch 10)
> 
>   hvm_guest_x86_mode(struct vcpu *v)
>   {
>       ASSERT(v == current);
> -    return hvm_funcs.guest_x86_mode(v);
> +    return alternative_call1(hvm_funcs.guest_x86_mode, v);
OOI, is there any reason for you to define alternative_callN and not 
providing a generic version (similar to arm_smccc_1_1_smc in 
include/asm-arm/smccc.h)?

>   }
>   
>   static inline void
>   hvm_update_host_cr3(struct vcpu *v)
>   {
>       if ( hvm_funcs.update_host_cr3 )
> -        hvm_funcs.update_host_cr3(v);
> +        alternative_vcall1(hvm_funcs.update_host_cr3, v);
>   }
> 
> And actually there was one more question I meant to ask: Are all
> locations within Xen reachable by direct calls on ARM? I.e. can an
> indirect call, when we know its target, always be replaced, no
> matter at what address (within Xen) it lives?

For direct branch, the range supported is +/- 32MB for 32-bit Arm, +/- 
128MB for 64-bit Arm. With the current layout all Xen text (including 
livepatch payload) will always be below 12MB.

So we should be able to reach all Xen by direct calls.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/16] x86: indirect call overhead reduction

[Jan Beulich]

>>> On 13.07.18 at 19:15, <julien.grall@arm.com> wrote:
> On 13/07/18 15:27, Jan Beulich wrote:
>>>>> On 13.07.18 at 15:39, <julien.grall@arm.com> wrote:
>>> On 13/07/18 14:27, Jan Beulich wrote:
>>>>>>> On 13.07.18 at 15:00, <julien.grall@arm.com> wrote:
>>>>> What would be the generic interface here? I saw it was based on
>>>>> alternative for the plumbing.
>>>>
>>>> Yes, I'd prefer to use the same mechanism as presented in the series.
>>>> As per above for the IOMMU case we'd then need another abstraction
>>>> layer put in the middle (to produce a patch site on x86, but a normal
>>>> [indirect] call on ARM).
>>>
>>> I will have a look. Could you point to the patch adding the abstraction?
>> 
>> Patch 9 is where the x86 side infrastructure gets added. Typical uses
>> would then be (taken from patch 10)
>> 
>>   hvm_guest_x86_mode(struct vcpu *v)
>>   {
>>       ASSERT(v == current);
>> -    return hvm_funcs.guest_x86_mode(v);
>> +    return alternative_call1(hvm_funcs.guest_x86_mode, v);
> OOI, is there any reason for you to define alternative_callN and not 
> providing a generic version (similar to arm_smccc_1_1_smc in 
> include/asm-arm/smccc.h)?

Oh, that's a nice approach - I'll try to switch to that.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/16] x86: indirect call overhead reduction

[Jan Beulich]

>>> On 11.07.18 at 15:15, <JBeulich@suse.com> wrote:
> While indirect calls have always been more expensive than direct ones,
> their cost has further increased with the Spectre v2 mitigations. In a
> number of cases we simply pointlessly use them in the first place. In
> many other cases the indirection solely exists to abstract from e.g.
> vendor specific hardware details, and hence the pointers used never
> change once set. Here we can use alternatives patching to get rid of
> the indirection.
> 
> From patch 8 onwards dependencies exist on earlier, yet to be reviewed
> patches ("x86/alternatives: fully leverage automatic NOP filling" as well
> as the "x86: improve PDX <-> PFN and alike translations" series at the
> very least). I nevertheless wanted to enable a first round of review of
> the series, the more that some of the patches (not just initial ones)
> could perhaps be taken irrespective of those dependencies.
> 
> Further areas where indirect calls could be eliminated (and that I've put
> on my todo list in case the general concept here is deemed reasonable)
> are IOMMU, cpufreq, vPMU, and XSM. For some of these, the ARM side
> would need dealing with as well - I'm not sure whether replacing indirect
> calls by direct ones is worthwhile there as well; if not, the wrappers
> would simply need to become function invocations in the ARM case.
> 
> 01: VMX: reduce number of posted-interrupt hooks
> 02: VMX: don't unconditionally set the tsc_scaling.setup hook
> 03: x86/HVM: switch virtual_intr_delivery_enabled() hook to simple boolean
> 04: x86/HVM: drop vmfunc_intercept
> 05: x86/HVM: add wrapper for hvm_funcs.set_tsc_offset()
> 06: x86: allow producing .i or .s for multiply compiled files
> 07: x86/shadow: fetch CPL just once in sh_page_fault()
> 08: x86/alternatives: allow using assembler macros in favor of C ones
> 09: x86: infrastructure to allow converting certain indirect calls to direct ones
> 10: x86/HVM: patch indirect calls through hvm_funcs to direct ones
> 11: x86/HVM: patch vINTR indirect calls through hvm_funcs to direct ones
> 12: x86: patch ctxt_switch_masking() indirect call to direct one
> 13: x86/genapic: drop .target_cpus() hook
> 14: x86/genapic: remove indirection from genapic hook accesses
> 15: x86/genapic: patch indirect calls to direct ones
> 16: x86/cpuidle: patch some indirect calls to direct ones

The last patch is wrong for the non-mwait-idle case, and I've
therefore dropped it.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 00/12] x86: indirect call overhead reduction

[Jan Beulich]

While indirect calls have always been more expensive than direct ones,
their cost has further increased with the Spectre v2 mitigations. In a
number of cases we simply pointlessly use them in the first place. In
many other cases the indirection solely exists to abstract from e.g.
vendor specific hardware details, and hence the pointers used never
change once set. Here we can use alternatives patching to get rid of
the indirection.

From patch 2 onwards dependencies exist on earlier, yet to be reviewed
patches ("x86/alternatives: fully leverage automatic NOP filling" as well
as the "x86: improve PDX <-> PFN and alike translations" series at the
very least). I nevertheless wanted to enable a first round of review of
the series, the more that some of the patches (not just initial ones)
could perhaps be taken irrespective of those dependencies. The first
two of the three genapic patches, otoh, are entirely independent and
could go in right away afaict if they were ack-ed.

Further areas where indirect calls could be eliminated (and that I've put
on my todo list in case the general concept here is deemed reasonable)
are IOMMU, vPMU, and XSM. For some of these, the ARM side would
need dealing with as well - I'm not sure whether replacing indirect calls
by direct ones is worthwhile there as well; if not, the wrappers would
simply need to become function invocations in the ARM case (as was
already asked for in the IOMMU case).

01: VMX: reduce number of posted-interrupt hooks
02: x86/alternatives: allow using assembler macros in favor of C ones
03: x86: infrastructure to allow converting certain indirect calls to direct ones
04: x86/HVM: patch indirect calls through hvm_funcs to direct ones
05: x86/HVM: patch vINTR indirect calls through hvm_funcs to direct ones
06: x86: patch ctxt_switch_masking() indirect call to direct one
07: x86/genapic: drop .target_cpus() hook
08: x86/genapic: remove indirection from genapic hook accesses
09: x86/genapic: patch indirect calls to direct ones
10: x86/cpuidle: patch some indirect calls to direct ones
11: cpufreq: convert to a single post-init driver (hooks) instance
12: cpufreq: patch target() indirect call to direct one

Jan




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 01/12] VMX: reduce number of posted-interrupt hooks

[Jan Beulich]

Three of the four hooks are not exposed outside of vmx.c, and all of
them have only a single possible non-NULL value. So there's no reason to
use hooks here - a simple set of flag indicators is sufficient (and we
don't even need a flag for the VM entry one, as it's always
(de-)activated together the the vCPU blocking hook, which needs to
remain an actual function pointer). This is the more that with the
Spectre v2 workarounds indirect calls have become more expensive.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Set PI_CSW_TO instead of PI_CSW_FROM in vmx_pi_hooks_deassign().

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -77,6 +77,10 @@ static int vmx_msr_read_intercept(unsign
 static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content);
 static void vmx_invlpg(struct vcpu *v, unsigned long vaddr);
 
+/* Values for domain's ->arch.hvm_domain.pi_ops.flags. */
+#define PI_CSW_FROM (1u << 0)
+#define PI_CSW_TO   (1u << 1)
+
 struct vmx_pi_blocking_vcpu {
     struct list_head     list;
     spinlock_t           lock;
@@ -329,8 +333,7 @@ void vmx_pi_hooks_assign(struct domain *
      * This can make sure the PI (especially the NDST feild) is
      * in proper state when we call vmx_vcpu_block().
      */
-    d->arch.hvm_domain.pi_ops.switch_from = vmx_pi_switch_from;
-    d->arch.hvm_domain.pi_ops.switch_to = vmx_pi_switch_to;
+    d->arch.hvm_domain.pi_ops.flags = PI_CSW_FROM | PI_CSW_TO;
 
     for_each_vcpu ( d, v )
     {
@@ -346,7 +349,6 @@ void vmx_pi_hooks_assign(struct domain *
     }
 
     d->arch.hvm_domain.pi_ops.vcpu_block = vmx_vcpu_block;
-    d->arch.hvm_domain.pi_ops.do_resume = vmx_pi_do_resume;
 }
 
 /* This function is called when pcidevs_lock is held */
@@ -383,8 +385,7 @@ void vmx_pi_hooks_deassign(struct domain
      * 'switch_to' hook function.
      */
     d->arch.hvm_domain.pi_ops.vcpu_block = NULL;
-    d->arch.hvm_domain.pi_ops.switch_from = NULL;
-    d->arch.hvm_domain.pi_ops.do_resume = NULL;
+    d->arch.hvm_domain.pi_ops.flags = PI_CSW_TO;
 
     for_each_vcpu ( d, v )
         vmx_pi_unblock_vcpu(v);
@@ -934,8 +935,8 @@ static void vmx_ctxt_switch_from(struct
     vmx_restore_host_msrs();
     vmx_save_dr(v);
 
-    if ( v->domain->arch.hvm_domain.pi_ops.switch_from )
-        v->domain->arch.hvm_domain.pi_ops.switch_from(v);
+    if ( v->domain->arch.hvm_domain.pi_ops.flags & PI_CSW_FROM )
+        vmx_pi_switch_from(v);
 }
 
 static void vmx_ctxt_switch_to(struct vcpu *v)
@@ -943,8 +944,8 @@ static void vmx_ctxt_switch_to(struct vc
     vmx_restore_guest_msrs(v);
     vmx_restore_dr(v);
 
-    if ( v->domain->arch.hvm_domain.pi_ops.switch_to )
-        v->domain->arch.hvm_domain.pi_ops.switch_to(v);
+    if ( v->domain->arch.hvm_domain.pi_ops.flags & PI_CSW_TO )
+        vmx_pi_switch_to(v);
 }
 
 
@@ -4330,8 +4331,8 @@ bool vmx_vmenter_helper(const struct cpu
      if ( nestedhvm_vcpu_in_guestmode(curr) && vcpu_nestedhvm(curr).stale_np2m )
          return false;
 
-    if ( curr->domain->arch.hvm_domain.pi_ops.do_resume )
-        curr->domain->arch.hvm_domain.pi_ops.do_resume(curr);
+    if ( curr->domain->arch.hvm_domain.pi_ops.vcpu_block )
+        vmx_pi_do_resume(curr);
 
     if ( !cpu_has_vmx_vpid )
         goto out;
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -80,20 +80,13 @@ struct hvm_ioreq_server {
  *     and actually has a physical device assigned .
  */
 struct hvm_pi_ops {
-    /* Hook into ctx_switch_from. */
-    void (*switch_from)(struct vcpu *v);
-
-    /* Hook into ctx_switch_to. */
-    void (*switch_to)(struct vcpu *v);
+    unsigned int flags;
 
     /*
      * Hook into arch_vcpu_block(), which is called
      * from vcpu_block() and vcpu_do_poll().
      */
     void (*vcpu_block)(struct vcpu *);
-
-    /* Hook into the vmentry path. */
-    void (*do_resume)(struct vcpu *v);
 };
 
 #define MAX_NR_IOREQ_SERVERS 8






_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 01/12] VMX: reduce number of posted-interrupt hooks

[Andrew Cooper]

On 29/08/18 14:59, Jan Beulich wrote:
> Three of the four hooks are not exposed outside of vmx.c, and all of
> them have only a single possible non-NULL value. So there's no reason to
> use hooks here - a simple set of flag indicators is sufficient (and we
> don't even need a flag for the VM entry one, as it's always
> (de-)activated together the the vCPU blocking hook, which needs to
> remain an actual function pointer). This is the more that with the
> Spectre v2 workarounds indirect calls have become more expensive.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 01/12] VMX: reduce number of posted-interrupt hooks

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, August 29, 2018 9:59 PM
> 
> Three of the four hooks are not exposed outside of vmx.c, and all of
> them have only a single possible non-NULL value. So there's no reason to
> use hooks here - a simple set of flag indicators is sufficient (and we
> don't even need a flag for the VM entry one, as it's always
> (de-)activated together the the vCPU blocking hook, which needs to
> remain an actual function pointer). This is the more that with the
> Spectre v2 workarounds indirect calls have become more expensive.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 02/12] x86/alternatives: allow using assembler macros in favor of C ones

[Jan Beulich]

As was validly pointed out as motivation for similar Linux side changes
(https://lkml.org/lkml/2018/6/22/677), using long sequences of
directives and auxiliary instructions, like is commonly the case when
setting up an alternative patch site, gcc can be mislead into believing
an asm() to be more heavy weight than it really is. By presenting it
with an assembler macro invocation instead, this can be avoided.

Initially I wanted to outright change the C macros ALTERNATIVE() and
ALTERNATIVE_2() to invoke the respective assembler ones, but doing so
would require quite a bit of cleanup of some use sites, because of the
exra necessary quoting combined with the need that each assembler macro
argument must consist of just a single string literal. We can consider
working towards that subsequently.

For now, set the stage of using the assembler macros here by providing a
new generated header, being the slightly massaged pre-processor output
of (for now just) alternative-asm.h. The massaging is primarily to be
able to properly track the build dependency: For this, we need the C
compiler to see the inclusion, which means we shouldn't directly use an
asm(". include ...") directive.

The dependency added to asm-offsets.s is not a true one; it's just the
easiest approach I could think of to make sure the new header gets
generated early on, without having to fiddle with xen/Makefile (and
introducing some x86-specific construct there).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Re-base.

--- a/.gitignore
+++ b/.gitignore
@@ -293,6 +293,7 @@ xen/.config.old
 xen/System.map
 xen/arch/arm/asm-offsets.s
 xen/arch/arm/xen.lds
+xen/arch/x86/asm-macros.i
 xen/arch/x86/asm-offsets.s
 xen/arch/x86/boot/mkelf32
 xen/arch/x86/xen.lds
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -215,9 +215,22 @@ $(TARGET).efi: prelink-efi.o $(note_file
 efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: $(BASEDIR)/arch/x86/efi/built_in.o
 efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: ;
 
-asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c
+asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c $(BASEDIR)/include/asm-x86/asm-macros.h
 	$(CC) $(filter-out -Wa$(comma)% -flto,$(CFLAGS)) -S -o $@ $<
 
+asm-macros.i: CFLAGS += -D__ASSEMBLY__ -P
+
+$(BASEDIR)/include/asm-x86/asm-macros.h: asm-macros.i Makefile
+	echo '#if 0' >$@.new
+	echo '.if 0' >>$@.new
+	echo '#endif' >>$@.new
+	echo 'asm ( ".include \"$@\"" );' >>$@.new
+	echo '#if 0' >>$@.new
+	echo '.endif' >>$@.new
+	cat $< >>$@.new
+	echo '#endif' >>$@.new
+	$(call move-if-changed,$@.new,$@)
+
 xen.lds: xen.lds.S
 	$(CC) -P -E -Ui386 $(filter-out -Wa$(comma)%,$(AFLAGS)) -o $@ $<
 	sed -e 's/xen\.lds\.o:/xen\.lds:/g' <.xen.lds.d >.xen.lds.d.new
@@ -237,6 +250,7 @@ efi/mkreloc: efi/mkreloc.c
 .PHONY: clean
 clean::
 	rm -f asm-offsets.s *.lds boot/*.o boot/*~ boot/core boot/mkelf32
+	rm -f asm-macros.i $(BASEDIR)/include/asm-x86/asm-macros.*
 	rm -f $(BASEDIR)/.xen-syms.[0-9]* boot/.*.d
 	rm -f $(BASEDIR)/.xen.efi.[0-9]* efi/*.efi efi/mkreloc
 	rm -f boot/cmdline.S boot/reloc.S boot/*.lnk boot/*.bin
--- /dev/null
+++ b/xen/arch/x86/asm-macros.c
@@ -0,0 +1 @@
+#include <asm/alternative-asm.h>
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -1,11 +1,12 @@
 #ifndef __X86_ALTERNATIVE_H__
 #define __X86_ALTERNATIVE_H__
 
+#ifdef __ASSEMBLY__
 #include <asm/alternative-asm.h>
-
-#ifndef __ASSEMBLY__
+#else
 #include <xen/stringify.h>
 #include <xen/types.h>
+#include <asm/asm-macros.h>
 
 struct __packed alt_instr {
     int32_t  orig_offset;   /* original instruction */
@@ -26,14 +27,6 @@ extern void add_nops(void *insns, unsign
 extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
 extern void alternative_instructions(void);
 
-asm ( ".macro mknops nr_bytes\n\t"
-#ifdef HAVE_AS_NOPS_DIRECTIVE
-      ".nops \\nr_bytes, " __stringify(ASM_NOP_MAX) "\n\t"
-#else
-      ".skip \\nr_bytes, 0x90\n\t"
-#endif
-      ".endm\n\t" );
-
 #define alt_orig_len       "(.LXEN%=_orig_e - .LXEN%=_orig_s)"
 #define alt_pad_len        "(.LXEN%=_orig_p - .LXEN%=_orig_e)"
 #define alt_total_len      "(.LXEN%=_orig_p - .LXEN%=_orig_s)"




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 02/12] x86/alternatives: allow using assembler macros in favor of C ones

[Andrew Cooper]

On 29/08/18 15:00, Jan Beulich wrote:
> As was validly pointed out as motivation for similar Linux side changes
> (https://lkml.org/lkml/2018/6/22/677), using long sequences of
> directives and auxiliary instructions, like is commonly the case when
> setting up an alternative patch site, gcc can be mislead into believing
> an asm() to be more heavy weight than it really is. By presenting it
> with an assembler macro invocation instead, this can be avoided.
>
> Initially I wanted to outright change the C macros ALTERNATIVE() and
> ALTERNATIVE_2() to invoke the respective assembler ones, but doing so
> would require quite a bit of cleanup of some use sites, because of the
> exra necessary quoting combined with the need that each assembler macro
> argument must consist of just a single string literal. We can consider
> working towards that subsequently.
>
> For now, set the stage of using the assembler macros here by providing a
> new generated header, being the slightly massaged pre-processor output
> of (for now just) alternative-asm.h. The massaging is primarily to be
> able to properly track the build dependency: For this, we need the C
> compiler to see the inclusion, which means we shouldn't directly use an
> asm(". include ...") directive.
>
> The dependency added to asm-offsets.s is not a true one; it's just the
> easiest approach I could think of to make sure the new header gets
> generated early on, without having to fiddle with xen/Makefile (and
> introducing some x86-specific construct there).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 03/12] x86: infrastructure to allow converting certain indirect calls to direct ones

[Jan Beulich]

In a number of cases the targets of indirect calls get determined once
at boot time. In such cases we can replace those calls with direct ones
via our alternative instruction patching mechanism.

Some of the targets (in particular the hvm_funcs ones) get established
only in pre-SMP initcalls, making necessary a second passs through the
alternative patching code. Therefore some adjustments beyond the
recognition of the new special pattern are necessary there.

Note that patching such sites more than once is not supported (and the
supplied macros also don't provide any means to do so).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Introduce and use count_va_arg(). Don't omit middle operand from
    ?: in ALT_CALL_ARG(). Re-base.

--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -177,8 +177,9 @@ text_poke(void *addr, const void *opcode
  * APs have less capabilities than the boot processor are not handled.
  * Tough. Make sure you disable such features by hand.
  */
-void init_or_livepatch apply_alternatives(struct alt_instr *start,
-                                          struct alt_instr *end)
+static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
+                                                  struct alt_instr *end,
+                                                  bool force)
 {
     struct alt_instr *a, *base;
 
@@ -217,6 +218,13 @@ void init_or_livepatch apply_alternative
         if ( ALT_ORIG_PTR(base) != orig )
             base = a;
 
+        /* Skip patch sites already handled during the first pass. */
+        if ( a->priv )
+        {
+            ASSERT(force);
+            continue;
+        }
+
         /* If there is no replacement to make, see about optimising the nops. */
         if ( !boot_cpu_has(a->cpuid) )
         {
@@ -224,7 +232,7 @@ void init_or_livepatch apply_alternative
             if ( base->priv )
                 continue;
 
-            base->priv = 1;
+            a->priv = 1;
 
             /* Nothing useful to do? */
             if ( toolchain_nops_are_ideal || a->pad_len <= 1 )
@@ -235,13 +243,58 @@ void init_or_livepatch apply_alternative
             continue;
         }
 
-        base->priv = 1;
-
         memcpy(buf, repl, a->repl_len);
 
         /* 0xe8/0xe9 are relative branches; fix the offset. */
         if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
-            *(int32_t *)(buf + 1) += repl - orig;
+        {
+            /*
+             * Detect the special case of indirect-to-direct branch patching:
+             * - replacement is a direct CALL/JMP (opcodes 0xE8/0xE9; already
+             *   checked above),
+             * - replacement's displacement is -5 (pointing back at the very
+             *   insn, which makes no sense in a real replacement insn),
+             * - original is an indirect CALL/JMP (opcodes 0xFF/2 or 0xFF/4)
+             *   using RIP-relative addressing.
+             * Some function targets may not be available when we come here
+             * the first time. Defer patching of those until the post-presmp-
+             * initcalls re-invocation. If at that point the target pointer is
+             * still NULL, insert "UD2; UD0" (for ease of recognition) instead
+             * of CALL/JMP.
+             */
+            if ( a->cpuid == X86_FEATURE_ALWAYS &&
+                 *(int32_t *)(buf + 1) == -5 &&
+                 a->orig_len >= 6 &&
+                 orig[0] == 0xff &&
+                 orig[1] == (*buf & 1 ? 0x25 : 0x15) )
+            {
+                long disp = *(int32_t *)(orig + 2);
+                const uint8_t *dest = *(void **)(orig + 6 + disp);
+
+                if ( dest )
+                {
+                    disp = dest - (orig + 5);
+                    ASSERT(disp == (int32_t)disp);
+                    *(int32_t *)(buf + 1) = disp;
+                }
+                else if ( force )
+                {
+                    buf[0] = 0x0f;
+                    buf[1] = 0x0b;
+                    buf[2] = 0x0f;
+                    buf[3] = 0xff;
+                    buf[4] = 0xff;
+                }
+                else
+                    continue;
+            }
+            else if ( force && system_state < SYS_STATE_active )
+                ASSERT_UNREACHABLE();
+            else
+                *(int32_t *)(buf + 1) += repl - orig;
+        }
+        else if ( force && system_state < SYS_STATE_active  )
+            ASSERT_UNREACHABLE();
         /* RIP-relative addressing is easy to check for in VEX-encoded insns. */
         else if ( a->repl_len >= 8 &&
                   (*buf & ~1) == 0xc4 &&
@@ -249,12 +302,21 @@ void init_or_livepatch apply_alternative
                   (buf[4 - (*buf & 1)] & ~0x38) == 0x05 )
             *(int32_t *)(buf + 5 - (*buf & 1)) += repl - orig;
 
+        a->priv = 1;
+
         add_nops(buf + a->repl_len, total_len - a->repl_len);
         text_poke(orig, buf, total_len);
     }
 }
 
-static bool __initdata alt_done;
+void init_or_livepatch apply_alternatives(struct alt_instr *start,
+                                          struct alt_instr *end)
+{
+    _apply_alternatives(start, end, true);
+}
+
+static unsigned int __initdata alt_todo;
+static unsigned int __initdata alt_done;
 
 /*
  * At boot time, we patch alternatives in NMI context.  This means that the
@@ -269,7 +331,7 @@ static int __init nmi_apply_alternatives
      * More than one NMI may occur between the two set_nmi_callback() below.
      * We only need to apply alternatives once.
      */
-    if ( !alt_done )
+    if ( !(alt_done & alt_todo) )
     {
         unsigned long cr0;
 
@@ -278,11 +340,12 @@ static int __init nmi_apply_alternatives
         /* Disable WP to allow patching read-only pages. */
         write_cr0(cr0 & ~X86_CR0_WP);
 
-        apply_alternatives(__alt_instructions, __alt_instructions_end);
+        _apply_alternatives(__alt_instructions, __alt_instructions_end,
+                            alt_done);
 
         write_cr0(cr0);
 
-        alt_done = true;
+        alt_done |= alt_todo;
     }
 
     return 1;
@@ -292,13 +355,11 @@ static int __init nmi_apply_alternatives
  * This routine is called with local interrupt disabled and used during
  * bootup.
  */
-void __init alternative_instructions(void)
+static void __init _alternative_instructions(bool force)
 {
     unsigned int i;
     nmi_callback_t *saved_nmi_callback;
 
-    arch_init_ideal_nops();
-
     /*
      * Don't stop machine check exceptions while patching.
      * MCEs only happen when something got corrupted and in this
@@ -311,6 +372,10 @@ void __init alternative_instructions(voi
      */
     ASSERT(!local_irq_is_enabled());
 
+    /* Set what operation to perform /before/ setting the callback. */
+    alt_todo = 1u << force;
+    barrier();
+
     /*
      * As soon as the callback is set up, the next NMI will trigger patching,
      * even an NMI ahead of our explicit self-NMI.
@@ -326,11 +391,24 @@ void __init alternative_instructions(voi
      * cover the (hopefully never) async case, poll alt_done for up to one
      * second.
      */
-    for ( i = 0; !ACCESS_ONCE(alt_done) && i < 1000; ++i )
+    for ( i = 0; !(ACCESS_ONCE(alt_done) & alt_todo) && i < 1000; ++i )
         mdelay(1);
 
-    if ( !ACCESS_ONCE(alt_done) )
+    if ( !(ACCESS_ONCE(alt_done) & alt_todo) )
         panic("Timed out waiting for alternatives self-NMI to hit");
 
     set_nmi_callback(saved_nmi_callback);
 }
+
+void __init alternative_instructions(void)
+{
+    arch_init_ideal_nops();
+    _alternative_instructions(false);
+}
+
+void __init alternative_branches(void)
+{
+    local_irq_disable();
+    _alternative_instructions(true);
+    local_irq_enable();
+}
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1634,6 +1634,8 @@ void __init noreturn __start_xen(unsigne
 
     do_presmp_initcalls();
 
+    alternative_branches();
+
     /*
      * NB: when running as a PV shim VCPUOP_up/down is wired to the shim
      * physical cpu_add/remove functions, so launch the guest with only
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -4,8 +4,8 @@
 #ifdef __ASSEMBLY__
 #include <asm/alternative-asm.h>
 #else
+#include <xen/lib.h>
 #include <xen/stringify.h>
-#include <xen/types.h>
 #include <asm/asm-macros.h>
 
 struct __packed alt_instr {
@@ -26,6 +26,7 @@ extern void add_nops(void *insns, unsign
 /* Similar to alternative_instructions except it can be run with IRQs enabled. */
 extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
 extern void alternative_instructions(void);
+extern void alternative_branches(void);
 
 #define alt_orig_len       "(.LXEN%=_orig_e - .LXEN%=_orig_s)"
 #define alt_pad_len        "(.LXEN%=_orig_p - .LXEN%=_orig_e)"
@@ -149,6 +150,203 @@ extern void alternative_instructions(voi
 /* Use this macro(s) if you need more than one output parameter. */
 #define ASM_OUTPUT2(a...) a
 
+/*
+ * Machinery to allow converting indirect to direct calls, when the called
+ * function is determined once at boot and later never changed.
+ */
+
+#define ALT_CALL_arg1 "rdi"
+#define ALT_CALL_arg2 "rsi"
+#define ALT_CALL_arg3 "rdx"
+#define ALT_CALL_arg4 "rcx"
+#define ALT_CALL_arg5 "r8"
+#define ALT_CALL_arg6 "r9"
+
+#define ALT_CALL_ARG(arg, n) \
+    register typeof((arg) ? (arg) : 0) a ## n ## _ \
+    asm ( ALT_CALL_arg ## n ) = (arg)
+#define ALT_CALL_NO_ARG(n) \
+    register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n )
+
+#define ALT_CALL_NO_ARG6 ALT_CALL_NO_ARG(6)
+#define ALT_CALL_NO_ARG5 ALT_CALL_NO_ARG(5); ALT_CALL_NO_ARG6
+#define ALT_CALL_NO_ARG4 ALT_CALL_NO_ARG(4); ALT_CALL_NO_ARG5
+#define ALT_CALL_NO_ARG3 ALT_CALL_NO_ARG(3); ALT_CALL_NO_ARG4
+#define ALT_CALL_NO_ARG2 ALT_CALL_NO_ARG(2); ALT_CALL_NO_ARG3
+#define ALT_CALL_NO_ARG1 ALT_CALL_NO_ARG(1); ALT_CALL_NO_ARG2
+
+/*
+ * Unfortunately ALT_CALL_NO_ARG() above can't use a fake initializer (to
+ * suppress "uninitialized variable" warnings), as various versions of gcc
+ * older than 8.1 fall on the nose in various ways with that (always because
+ * of some other construct elsewhere in the same function needing to use the
+ * same hard register). Otherwise the asm() below could uniformly use "+r"
+ * output constraints, making unnecessary all these ALT_CALL<n>_OUT macros.
+ */
+#define ALT_CALL0_OUT "=r" (a1_), "=r" (a2_), "=r" (a3_), \
+                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
+#define ALT_CALL1_OUT "+r" (a1_), "=r" (a2_), "=r" (a3_), \
+                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
+#define ALT_CALL2_OUT "+r" (a1_), "+r" (a2_), "=r" (a3_), \
+                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
+#define ALT_CALL3_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
+                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
+#define ALT_CALL4_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
+                      "+r" (a4_), "=r" (a5_), "=r" (a6_)
+#define ALT_CALL5_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
+                      "+r" (a4_), "+r" (a5_), "=r" (a6_)
+#define ALT_CALL6_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
+                      "+r" (a4_), "+r" (a5_), "+r" (a6_)
+
+#define alternative_callN(n, rettype, func) ({                     \
+    rettype ret_;                                                  \
+    register unsigned long r10_ asm("r10");                        \
+    register unsigned long r11_ asm("r11");                        \
+    asm volatile (__stringify(ALTERNATIVE "call *%c[addr](%%rip)", \
+                                          "call .",                \
+                                          X86_FEATURE_ALWAYS)      \
+                  : ALT_CALL ## n ## _OUT, "=a" (ret_),            \
+                    "=r" (r10_), "=r" (r11_)                       \
+                  : [addr] "i" (&(func)), "g" (func)               \
+                  : "memory" );                                    \
+    ret_;                                                          \
+})
+
+#define alternative_vcall0(func) ({             \
+    ALT_CALL_NO_ARG1;                           \
+    ((void)alternative_callN(0, int, func));    \
+})
+
+#define alternative_call0(func) ({              \
+    ALT_CALL_NO_ARG1;                           \
+    alternative_callN(0, typeof(func()), func); \
+})
+
+#define alternative_vcall1(func, arg) ({           \
+    ALT_CALL_ARG(arg, 1);                          \
+    ALT_CALL_NO_ARG2;                              \
+    (void)sizeof(func(arg));                       \
+    (void)alternative_callN(1, int, func);         \
+})
+
+#define alternative_call1(func, arg) ({            \
+    ALT_CALL_ARG(arg, 1);                          \
+    ALT_CALL_NO_ARG2;                              \
+    alternative_callN(1, typeof(func(arg)), func); \
+})
+
+#define alternative_vcall2(func, arg1, arg2) ({           \
+    ALT_CALL_ARG(arg1, 1);                                \
+    ALT_CALL_ARG(arg2, 2);                                \
+    ALT_CALL_NO_ARG3;                                     \
+    (void)sizeof(func(arg1, arg2));                       \
+    (void)alternative_callN(2, int, func);                \
+})
+
+#define alternative_call2(func, arg1, arg2) ({            \
+    ALT_CALL_ARG(arg1, 1);                                \
+    ALT_CALL_ARG(arg2, 2);                                \
+    ALT_CALL_NO_ARG3;                                     \
+    alternative_callN(2, typeof(func(arg1, arg2)), func); \
+})
+
+#define alternative_vcall3(func, arg1, arg2, arg3) ({    \
+    ALT_CALL_ARG(arg1, 1);                               \
+    ALT_CALL_ARG(arg2, 2);                               \
+    ALT_CALL_ARG(arg3, 3);                               \
+    ALT_CALL_NO_ARG4;                                    \
+    (void)sizeof(func(arg1, arg2, arg3));                \
+    (void)alternative_callN(3, int, func);               \
+})
+
+#define alternative_call3(func, arg1, arg2, arg3) ({     \
+    ALT_CALL_ARG(arg1, 1);                               \
+    ALT_CALL_ARG(arg2, 2);                               \
+    ALT_CALL_ARG(arg3, 3);                               \
+    ALT_CALL_NO_ARG4;                                    \
+    alternative_callN(3, typeof(func(arg1, arg2, arg3)), \
+                      func);                             \
+})
+
+#define alternative_vcall4(func, arg1, arg2, arg3, arg4) ({ \
+    ALT_CALL_ARG(arg1, 1);                                  \
+    ALT_CALL_ARG(arg2, 2);                                  \
+    ALT_CALL_ARG(arg3, 3);                                  \
+    ALT_CALL_ARG(arg4, 4);                                  \
+    ALT_CALL_NO_ARG5;                                       \
+    (void)sizeof(func(arg1, arg2, arg3, arg4));             \
+    (void)alternative_callN(4, int, func);                  \
+})
+
+#define alternative_call4(func, arg1, arg2, arg3, arg4) ({  \
+    ALT_CALL_ARG(arg1, 1);                                  \
+    ALT_CALL_ARG(arg2, 2);                                  \
+    ALT_CALL_ARG(arg3, 3);                                  \
+    ALT_CALL_ARG(arg4, 4);                                  \
+    ALT_CALL_NO_ARG5;                                       \
+    alternative_callN(4, typeof(func(arg1, arg2,            \
+                                     arg3, arg4)),          \
+                      func);                                \
+})
+
+#define alternative_vcall5(func, arg1, arg2, arg3, arg4, arg5) ({ \
+    ALT_CALL_ARG(arg1, 1);                                        \
+    ALT_CALL_ARG(arg2, 2);                                        \
+    ALT_CALL_ARG(arg3, 3);                                        \
+    ALT_CALL_ARG(arg4, 4);                                        \
+    ALT_CALL_ARG(arg5, 5);                                        \
+    ALT_CALL_NO_ARG6;                                             \
+    (void)sizeof(func(arg1, arg2, arg3, arg4, arg5));             \
+    (void)alternative_callN(5, int, func, ALT_CALL_OUT5);         \
+})
+
+#define alternative_call5(func, arg1, arg2, arg3, arg4, arg5) ({  \
+    ALT_CALL_ARG(arg1, 1);                                        \
+    ALT_CALL_ARG(arg2, 2);                                        \
+    ALT_CALL_ARG(arg3, 3);                                        \
+    ALT_CALL_ARG(arg4, 4);                                        \
+    ALT_CALL_ARG(arg5, 5);                                        \
+    ALT_CALL_NO_ARG6;                                             \
+    alternative_callN(5, typeof(func(arg1, arg2, arg3,            \
+                                     arg4, arg5)),                \
+                      func, ALT_CALL_OUT5);                       \
+})
+
+#define alternative_vcall6(func, arg1, arg2, arg3, arg4, arg5, arg6) ({ \
+    ALT_CALL_ARG(arg1, 1);                                              \
+    ALT_CALL_ARG(arg2, 2);                                              \
+    ALT_CALL_ARG(arg3, 3);                                              \
+    ALT_CALL_ARG(arg4, 4);                                              \
+    ALT_CALL_ARG(arg5, 5);                                              \
+    ALT_CALL_ARG(arg6, 6);                                              \
+    (void)sizeof(func(arg1, arg2, arg3, arg4, arg5, arg6));             \
+    (void)alternative_callN(6, int, func);                              \
+})
+
+#define alternative_call6(func, arg1, arg2, arg3, arg4, arg5, arg6) ({  \
+    ALT_CALL_ARG(arg1, 1);                                              \
+    ALT_CALL_ARG(arg2, 2);                                              \
+    ALT_CALL_ARG(arg3, 3);                                              \
+    ALT_CALL_ARG(arg4, 4);                                              \
+    ALT_CALL_ARG(arg5, 5);                                              \
+    ALT_CALL_ARG(arg6, 6);                                              \
+    alternative_callN(6, typeof(func(arg1, arg2, arg3,                  \
+                                     arg4, arg5, arg6)),                \
+                      func, ALT_CALL_OUT6);                             \
+})
+
+#define alternative_vcall__(nr) alternative_vcall ## nr
+#define alternative_call__(nr)  alternative_call ## nr
+
+#define alternative_vcall_(nr) alternative_vcall__(nr)
+#define alternative_call_(nr)  alternative_call__(nr)
+
+#define alternative_vcall(func, args...) \
+    alternative_vcall_(count_va_arg(args))(func, ## args)
+
+#define alternative_call(func, args...) \
+    alternative_call_(count_va_arg(args))(func, ## args)
+
 #endif /*  !__ASSEMBLY__  */
 
 #endif /* __X86_ALTERNATIVE_H__ */
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -66,6 +66,10 @@
 
 #define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
 
+#define count_va_arg_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x
+#define count_va_arg(args...) \
+    count_va_arg_(., ## args, 8, 7, 6, 5, 4, 3, 2, 1, 0)
+
 struct domain;
 
 void cmdline_parse(const char *cmdline);


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 03/12] x86: infrastructure to allow converting certain indirect calls to direct ones

[Julien Grall]

Hi Jan,

On 08/29/2018 03:02 PM, Jan Beulich wrote:
> +#define alternative_vcall2(func, arg1, arg2) ({           \
> +    ALT_CALL_ARG(arg1, 1);                                \
> +    ALT_CALL_ARG(arg2, 2);                                \

I believe this code has the same issue Stefano recently discovered on 
the SMCCC.

Using explicit register variable will not reserve the register. So if 
arg* is a function call, the register you have just assigned will get 
clobbered (see [1]).

The solution to this is evaluating all the arguments before declaring 
the variable with explicit registers. See the patch [2] for an example.


> +    ALT_CALL_NO_ARG3;                                     \
> +    (void)sizeof(func(arg1, arg2));                       \
> +    (void)alternative_callN(2, int, func);                \
> +})

[...]

> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -66,6 +66,10 @@
>   
>   #define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
>   
> +#define count_va_arg_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x > +#define count_va_arg(args...) \
> +    count_va_arg_(., ## args, 8, 7, 6, 5, 4, 3, 2, 1, 0)

We have a similar function in SMCCC where only the arguments except the 
first one (Function ID) and last one (Result pointer). I believe 
different context will require different way to count argument in order 
to keep the code readable.

So I am not entirely sure if there are a benefit to have this function 
in common.

Cheers,

[1] 
https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html#Local-Register-Variables

[2] https://lists.xen.org/archives/html/xen-devel/2018-08/msg02139.html

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 03/12] x86: infrastructure to allow converting certain indirect calls to direct ones

[Jan Beulich]

>>> On 29.08.18 at 16:37, <julien.grall@arm.com> wrote:
> On 08/29/2018 03:02 PM, Jan Beulich wrote:
>> +#define alternative_vcall2(func, arg1, arg2) ({           \
>> +    ALT_CALL_ARG(arg1, 1);                                \
>> +    ALT_CALL_ARG(arg2, 2);                                \
> 
> I believe this code has the same issue Stefano recently discovered on 
> the SMCCC.
> 
> Using explicit register variable will not reserve the register. So if 
> arg* is a function call, the register you have just assigned will get 
> clobbered (see [1]).
> 
> The solution to this is evaluating all the arguments before declaring 
> the variable with explicit registers. See the patch [2] for an example.

Oh, I see - I should have made the connection when reading through
that part of the gcc doc the other day. The current construct is safe
for all current uses, but I agree I'd better follow the model you point
out.

>> --- a/xen/include/xen/lib.h
>> +++ b/xen/include/xen/lib.h
>> @@ -66,6 +66,10 @@
>>   
>>   #define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
>>   
>> +#define count_va_arg_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x
>> +#define count_va_arg(args...) \
>> +    count_va_arg_(., ## args, 8, 7, 6, 5, 4, 3, 2, 1, 0)
> 
> We have a similar function in SMCCC where only the arguments except the 
> first one (Function ID) and last one (Result pointer). I believe 
> different context will require different way to count argument in order 
> to keep the code readable.
> 
> So I am not entirely sure if there are a benefit to have this function 
> in common.

From an initial look it seemed to me that the SMCCC code could be
converted to use the macros here. In any event, other than the
macros used there the ones here are generic in that they simply
could arguments, without and special treatment of any of them.
Hence the generic name and placement.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 03/12] x86: infrastructure to allow converting certain indirect calls to direct ones

[Andrew Cooper]

On 29/08/18 15:02, Jan Beulich wrote:
> @@ -235,13 +243,58 @@ void init_or_livepatch apply_alternative
>              continue;
>          }
>  
> -        base->priv = 1;
> -
>          memcpy(buf, repl, a->repl_len);
>  
>          /* 0xe8/0xe9 are relative branches; fix the offset. */
>          if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
> -            *(int32_t *)(buf + 1) += repl - orig;
> +        {
> +            /*
> +             * Detect the special case of indirect-to-direct branch patching:
> +             * - replacement is a direct CALL/JMP (opcodes 0xE8/0xE9; already
> +             *   checked above),
> +             * - replacement's displacement is -5 (pointing back at the very
> +             *   insn, which makes no sense in a real replacement insn),
> +             * - original is an indirect CALL/JMP (opcodes 0xFF/2 or 0xFF/4)
> +             *   using RIP-relative addressing.
> +             * Some function targets may not be available when we come here
> +             * the first time. Defer patching of those until the post-presmp-
> +             * initcalls re-invocation.

Which calls?  That smells like a bug which should be fixed rather than
worked around.  As for the other complexity here...

>  If at that point the target pointer is
> +             * still NULL, insert "UD2; UD0" (for ease of recognition) instead
> +             * of CALL/JMP.
> +             */
> +            if ( a->cpuid == X86_FEATURE_ALWAYS &&
> +                 *(int32_t *)(buf + 1) == -5 &&
> +                 a->orig_len >= 6 &&
> +                 orig[0] == 0xff &&
> +                 orig[1] == (*buf & 1 ? 0x25 : 0x15) )
> +            {
> +                long disp = *(int32_t *)(orig + 2);
> +                const uint8_t *dest = *(void **)(orig + 6 + disp);
> +
> +                if ( dest )
> +                {
> +                    disp = dest - (orig + 5);
> +                    ASSERT(disp == (int32_t)disp);
> +                    *(int32_t *)(buf + 1) = disp;
> +                }
> +                else if ( force )
> +                {
> +                    buf[0] = 0x0f;
> +                    buf[1] = 0x0b;
> +                    buf[2] = 0x0f;
> +                    buf[3] = 0xff;
> +                    buf[4] = 0xff;
> +                }
> +                else
> +                    continue;
> +            }
> +            else if ( force && system_state < SYS_STATE_active )
> +                ASSERT_UNREACHABLE();
> +            else
> +                *(int32_t *)(buf + 1) += repl - orig;
> +        }
> +        else if ( force && system_state < SYS_STATE_active  )
> +            ASSERT_UNREACHABLE();
>          /* RIP-relative addressing is easy to check for in VEX-encoded insns. */
>          else if ( a->repl_len >= 8 &&
>                    (*buf & ~1) == 0xc4 &&
> @@ -149,6 +150,203 @@ extern void alternative_instructions(voi
>  /* Use this macro(s) if you need more than one output parameter. */
>  #define ASM_OUTPUT2(a...) a
>  
> +/*
> + * Machinery to allow converting indirect to direct calls, when the called
> + * function is determined once at boot and later never changed.
> + */
> +
> +#define ALT_CALL_arg1 "rdi"
> +#define ALT_CALL_arg2 "rsi"
> +#define ALT_CALL_arg3 "rdx"
> +#define ALT_CALL_arg4 "rcx"
> +#define ALT_CALL_arg5 "r8"
> +#define ALT_CALL_arg6 "r9"
> +
> +#define ALT_CALL_ARG(arg, n) \
> +    register typeof((arg) ? (arg) : 0) a ## n ## _ \
> +    asm ( ALT_CALL_arg ## n ) = (arg)
> +#define ALT_CALL_NO_ARG(n) \
> +    register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n )
> +
> +#define ALT_CALL_NO_ARG6 ALT_CALL_NO_ARG(6)
> +#define ALT_CALL_NO_ARG5 ALT_CALL_NO_ARG(5); ALT_CALL_NO_ARG6
> +#define ALT_CALL_NO_ARG4 ALT_CALL_NO_ARG(4); ALT_CALL_NO_ARG5
> +#define ALT_CALL_NO_ARG3 ALT_CALL_NO_ARG(3); ALT_CALL_NO_ARG4
> +#define ALT_CALL_NO_ARG2 ALT_CALL_NO_ARG(2); ALT_CALL_NO_ARG3
> +#define ALT_CALL_NO_ARG1 ALT_CALL_NO_ARG(1); ALT_CALL_NO_ARG2
> +
> +/*
> + * Unfortunately ALT_CALL_NO_ARG() above can't use a fake initializer (to
> + * suppress "uninitialized variable" warnings), as various versions of gcc
> + * older than 8.1 fall on the nose in various ways with that (always because
> + * of some other construct elsewhere in the same function needing to use the
> + * same hard register). Otherwise the asm() below could uniformly use "+r"
> + * output constraints, making unnecessary all these ALT_CALL<n>_OUT macros.
> + */
> +#define ALT_CALL0_OUT "=r" (a1_), "=r" (a2_), "=r" (a3_), \
> +                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
> +#define ALT_CALL1_OUT "+r" (a1_), "=r" (a2_), "=r" (a3_), \
> +                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
> +#define ALT_CALL2_OUT "+r" (a1_), "+r" (a2_), "=r" (a3_), \
> +                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
> +#define ALT_CALL3_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
> +                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
> +#define ALT_CALL4_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
> +                      "+r" (a4_), "=r" (a5_), "=r" (a6_)
> +#define ALT_CALL5_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
> +                      "+r" (a4_), "+r" (a5_), "=r" (a6_)
> +#define ALT_CALL6_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
> +                      "+r" (a4_), "+r" (a5_), "+r" (a6_)
> +
> +#define alternative_callN(n, rettype, func) ({                     \
> +    rettype ret_;                                                  \
> +    register unsigned long r10_ asm("r10");                        \
> +    register unsigned long r11_ asm("r11");                        \
> +    asm volatile (__stringify(ALTERNATIVE "call *%c[addr](%%rip)", \
> +                                          "call .",                \
> +                                          X86_FEATURE_ALWAYS)      \
> +                  : ALT_CALL ## n ## _OUT, "=a" (ret_),            \
> +                    "=r" (r10_), "=r" (r11_)                       \
> +                  : [addr] "i" (&(func)), "g" (func)               \

There was a Linux thread (which I've lost track of) which went around
replacing "i" with "P" for labels like this, which behaves better for
relocatable targets.  Furthermore, I can't work out what the "g"
constraint is for, but if it is simply for making the reference visible,
I'd suggest "X" instead which avoids any interference with register
scheduling.

As for the complexity, how about implementing this as an unlikely block
with a single call in?  That way, patching turns the entry jmp imm32
into a call imm32 and we lose the complicated decoding and nop padding
from the result.

~Andrew

> +                  : "memory" );                                    \
> +    ret_;                                                          \
> +})
> +
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 03/12] x86: infrastructure to allow converting certain indirect calls to direct ones

[Jan Beulich]

>>> On 29.08.18 at 18:01, <andrew.cooper3@citrix.com> wrote:
> On 29/08/18 15:02, Jan Beulich wrote:
>> @@ -235,13 +243,58 @@ void init_or_livepatch apply_alternative
>>              continue;
>>          }
>>  
>> -        base->priv = 1;
>> -
>>          memcpy(buf, repl, a->repl_len);
>>  
>>          /* 0xe8/0xe9 are relative branches; fix the offset. */
>>          if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
>> -            *(int32_t *)(buf + 1) += repl - orig;
>> +        {
>> +            /*
>> +             * Detect the special case of indirect-to-direct branch patching:
>> +             * - replacement is a direct CALL/JMP (opcodes 0xE8/0xE9; already
>> +             *   checked above),
>> +             * - replacement's displacement is -5 (pointing back at the very
>> +             *   insn, which makes no sense in a real replacement insn),
>> +             * - original is an indirect CALL/JMP (opcodes 0xFF/2 or 0xFF/4)
>> +             *   using RIP-relative addressing.
>> +             * Some function targets may not be available when we come here
>> +             * the first time. Defer patching of those until the post-presmp-
>> +             * initcalls re-invocation.
> 
> Which calls?  That smells like a bug which should be fixed rather than
> worked around.

Well, hvm_funcs being the primary goal of all this work - HVM
init is a pre-SMP initcall.

>  As for the other complexity here...
> 
>>  If at that point the target pointer is
>> +             * still NULL, insert "UD2; UD0" (for ease of recognition) instead
>> +             * of CALL/JMP.
>> +             */
>> +            if ( a->cpuid == X86_FEATURE_ALWAYS &&
>> +                 *(int32_t *)(buf + 1) == -5 &&
>> +                 a->orig_len >= 6 &&
>> +                 orig[0] == 0xff &&
>> +                 orig[1] == (*buf & 1 ? 0x25 : 0x15) )
>> +            {
>> +                long disp = *(int32_t *)(orig + 2);
>> +                const uint8_t *dest = *(void **)(orig + 6 + disp);
>> +
>> +                if ( dest )
>> +                {
>> +                    disp = dest - (orig + 5);
>> +                    ASSERT(disp == (int32_t)disp);
>> +                    *(int32_t *)(buf + 1) = disp;
>> +                }
>> +                else if ( force )
>> +                {
>> +                    buf[0] = 0x0f;
>> +                    buf[1] = 0x0b;
>> +                    buf[2] = 0x0f;
>> +                    buf[3] = 0xff;
>> +                    buf[4] = 0xff;
>> +                }
>> +                else
>> +                    continue;
>> +            }
>> +            else if ( force && system_state < SYS_STATE_active )
>> +                ASSERT_UNREACHABLE();
>> +            else
>> +                *(int32_t *)(buf + 1) += repl - orig;
>> +        }
>> +        else if ( force && system_state < SYS_STATE_active  )
>> +            ASSERT_UNREACHABLE();
>>          /* RIP-relative addressing is easy to check for in VEX-encoded insns. */
>>          else if ( a->repl_len >= 8 &&
>>                    (*buf & ~1) == 0xc4 &&
>> @@ -149,6 +150,203 @@ extern void alternative_instructions(voi
>>  /* Use this macro(s) if you need more than one output parameter. */
>>  #define ASM_OUTPUT2(a...) a
>>  
>> +/*
>> + * Machinery to allow converting indirect to direct calls, when the called
>> + * function is determined once at boot and later never changed.
>> + */
>> +
>> +#define ALT_CALL_arg1 "rdi"
>> +#define ALT_CALL_arg2 "rsi"
>> +#define ALT_CALL_arg3 "rdx"
>> +#define ALT_CALL_arg4 "rcx"
>> +#define ALT_CALL_arg5 "r8"
>> +#define ALT_CALL_arg6 "r9"
>> +
>> +#define ALT_CALL_ARG(arg, n) \
>> +    register typeof((arg) ? (arg) : 0) a ## n ## _ \
>> +    asm ( ALT_CALL_arg ## n ) = (arg)
>> +#define ALT_CALL_NO_ARG(n) \
>> +    register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n )
>> +
>> +#define ALT_CALL_NO_ARG6 ALT_CALL_NO_ARG(6)
>> +#define ALT_CALL_NO_ARG5 ALT_CALL_NO_ARG(5); ALT_CALL_NO_ARG6
>> +#define ALT_CALL_NO_ARG4 ALT_CALL_NO_ARG(4); ALT_CALL_NO_ARG5
>> +#define ALT_CALL_NO_ARG3 ALT_CALL_NO_ARG(3); ALT_CALL_NO_ARG4
>> +#define ALT_CALL_NO_ARG2 ALT_CALL_NO_ARG(2); ALT_CALL_NO_ARG3
>> +#define ALT_CALL_NO_ARG1 ALT_CALL_NO_ARG(1); ALT_CALL_NO_ARG2
>> +
>> +/*
>> + * Unfortunately ALT_CALL_NO_ARG() above can't use a fake initializer (to
>> + * suppress "uninitialized variable" warnings), as various versions of gcc
>> + * older than 8.1 fall on the nose in various ways with that (always because
>> + * of some other construct elsewhere in the same function needing to use the
>> + * same hard register). Otherwise the asm() below could uniformly use "+r"
>> + * output constraints, making unnecessary all these ALT_CALL<n>_OUT macros.
>> + */
>> +#define ALT_CALL0_OUT "=r" (a1_), "=r" (a2_), "=r" (a3_), \
>> +                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
>> +#define ALT_CALL1_OUT "+r" (a1_), "=r" (a2_), "=r" (a3_), \
>> +                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
>> +#define ALT_CALL2_OUT "+r" (a1_), "+r" (a2_), "=r" (a3_), \
>> +                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
>> +#define ALT_CALL3_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
>> +                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
>> +#define ALT_CALL4_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
>> +                      "+r" (a4_), "=r" (a5_), "=r" (a6_)
>> +#define ALT_CALL5_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
>> +                      "+r" (a4_), "+r" (a5_), "=r" (a6_)
>> +#define ALT_CALL6_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
>> +                      "+r" (a4_), "+r" (a5_), "+r" (a6_)
>> +
>> +#define alternative_callN(n, rettype, func) ({                     \
>> +    rettype ret_;                                                  \
>> +    register unsigned long r10_ asm("r10");                        \
>> +    register unsigned long r11_ asm("r11");                        \
>> +    asm volatile (__stringify(ALTERNATIVE "call *%c[addr](%%rip)", \
>> +                                          "call .",                \
>> +                                          X86_FEATURE_ALWAYS)      \
>> +                  : ALT_CALL ## n ## _OUT, "=a" (ret_),            \
>> +                    "=r" (r10_), "=r" (r11_)                       \
>> +                  : [addr] "i" (&(func)), "g" (func)               \
> 
> There was a Linux thread (which I've lost track of) which went around
> replacing "i" with "P" for labels like this, which behaves better for
> relocatable targets.

Looking at Linux sources, do you mean the P operand modifier
instead of a P constraint (which I can't find any reference to)?
I.e. to replace "%c[addr]" by "%P[addr]"? But all P does,
according to gcc sources, is to add @plt() around the symbol.
We definitely don't want this here - it's only applicable to direct
branches.

>  Furthermore, I can't work out what the "g"
> constraint is for, but if it is simply for making the reference visible,
> I'd suggest "X" instead which avoids any interference with register
> scheduling.

I don't think there's a big difference between "Any register, memory
or immediate integer operand is allowed, except for registers that are
not general registers" and "Any operand whatsoever is allowed" in
the our context, but yes, I can switch to this.

> As for the complexity, how about implementing this as an unlikely block
> with a single call in?  That way, patching turns the entry jmp imm32
> into a call imm32 and we lose the complicated decoding and nop padding
> from the result.

I don't understand this at all: It would further complicate decoding, as
then I also need to follow the JMP. Please carefully look at the logic in
_apply_alternatives() again: I need to decode the RIP-relative
operand in order to determine the CALL target. Your suggestion has
the only benefit of reducing main line code size (as we'd then not need
to patch in any NOP anymore). That _could_ be considered as an
add-on optimization (provided it is one in the first place, considering
the larger overall code size, but perhaps the "CALL <imm32>" could
be put in .init.text, as we guarantee to patch all instances), but not
something I'd be willing to do right in this patch.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 04/12] x86/HVM: patch indirect calls through hvm_funcs to direct ones

[Jan Beulich]

This is intentionally not touching hooks used rarely (or not at all)
during the lifetime of a VM, like {domain,vcpu}_initialise or cpu_up,
as well as nested, VM event, and altp2m ones (they can all be done
later, if so desired). Virtual Interrupt delivery ones will be dealt
with in a subsequent patch.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Drop open-coded numbers from macro invocations. Re-base.

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2031,7 +2031,7 @@ static int hvmemul_write_msr(
 static int hvmemul_wbinvd(
     struct x86_emulate_ctxt *ctxt)
 {
-    hvm_funcs.wbinvd_intercept();
+    alternative_vcall(hvm_funcs.wbinvd_intercept);
     return X86EMUL_OKAY;
 }
 
@@ -2049,7 +2049,7 @@ static int hvmemul_get_fpu(
     struct vcpu *curr = current;
 
     if ( !curr->fpu_dirtied )
-        hvm_funcs.fpu_dirty_intercept();
+        alternative_vcall(hvm_funcs.fpu_dirty_intercept);
     else if ( type == X86EMUL_FPU_fpu )
     {
         const typeof(curr->arch.xsave_area->fpu_sse) *fpu_ctxt =
@@ -2166,7 +2166,7 @@ static void hvmemul_put_fpu(
         {
             curr->fpu_dirtied = false;
             stts();
-            hvm_funcs.fpu_leave(curr);
+            alternative_vcall(hvm_funcs.fpu_leave, curr);
         }
     }
 }
@@ -2328,7 +2328,8 @@ static int _hvm_emulate_one(struct hvm_e
     if ( hvmemul_ctxt->intr_shadow != new_intr_shadow )
     {
         hvmemul_ctxt->intr_shadow = new_intr_shadow;
-        hvm_funcs.set_interrupt_shadow(curr, new_intr_shadow);
+        alternative_vcall(hvm_funcs.set_interrupt_shadow,
+                          curr, new_intr_shadow);
     }
 
     if ( hvmemul_ctxt->ctxt.retire.hlt &&
@@ -2465,7 +2466,8 @@ void hvm_emulate_init_once(
 
     memset(hvmemul_ctxt, 0, sizeof(*hvmemul_ctxt));
 
-    hvmemul_ctxt->intr_shadow = hvm_funcs.get_interrupt_shadow(curr);
+    hvmemul_ctxt->intr_shadow =
+        alternative_call(hvm_funcs.get_interrupt_shadow, curr);
     hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt);
     hvmemul_get_seg_reg(x86_seg_ss, hvmemul_ctxt);
 
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -272,12 +272,12 @@ void hvm_set_rdtsc_exiting(struct domain
     struct vcpu *v;
 
     for_each_vcpu ( d, v )
-        hvm_funcs.set_rdtsc_exiting(v, enable);
+        alternative_vcall(hvm_funcs.set_rdtsc_exiting, v, enable);
 }
 
 void hvm_get_guest_pat(struct vcpu *v, u64 *guest_pat)
 {
-    if ( !hvm_funcs.get_guest_pat(v, guest_pat) )
+    if ( !alternative_call(hvm_funcs.get_guest_pat, v, guest_pat) )
         *guest_pat = v->arch.hvm_vcpu.pat_cr;
 }
 
@@ -302,7 +302,7 @@ int hvm_set_guest_pat(struct vcpu *v, u6
             return 0;
         }
 
-    if ( !hvm_funcs.set_guest_pat(v, guest_pat) )
+    if ( !alternative_call(hvm_funcs.set_guest_pat, v, guest_pat) )
         v->arch.hvm_vcpu.pat_cr = guest_pat;
 
     return 1;
@@ -342,7 +342,7 @@ bool hvm_set_guest_bndcfgs(struct vcpu *
             /* nothing, best effort only */;
     }
 
-    return hvm_funcs.set_guest_bndcfgs(v, val);
+    return alternative_call(hvm_funcs.set_guest_bndcfgs, v, val);
 }
 
 /*
@@ -502,7 +502,8 @@ void hvm_migrate_pirqs(struct vcpu *v)
 static bool hvm_get_pending_event(struct vcpu *v, struct x86_event *info)
 {
     info->cr2 = v->arch.hvm_vcpu.guest_cr[2];
-    return hvm_funcs.get_pending_event(v, info);
+
+    return alternative_call(hvm_funcs.get_pending_event, v, info);
 }
 
 void hvm_do_resume(struct vcpu *v)
@@ -1683,7 +1684,7 @@ void hvm_inject_event(const struct x86_e
         }
     }
 
-    hvm_funcs.inject_event(event);
+    alternative_vcall(hvm_funcs.inject_event, event);
 }
 
 int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
@@ -2270,7 +2271,7 @@ int hvm_set_cr0(unsigned long value, boo
          (!rangeset_is_empty(d->iomem_caps) ||
           !rangeset_is_empty(d->arch.ioport_caps) ||
           has_arch_pdevs(d)) )
-        hvm_funcs.handle_cd(v, value);
+        alternative_vcall(hvm_funcs.handle_cd, v, value);
 
     hvm_update_cr(v, 0, value);
 
@@ -3512,7 +3513,8 @@ int hvm_msr_read_intercept(unsigned int
             goto gp_fault;
         /* If ret == 0 then this is not an MCE MSR, see other MSRs. */
         ret = ((ret == 0)
-               ? hvm_funcs.msr_read_intercept(msr, msr_content)
+               ? alternative_call(hvm_funcs.msr_read_intercept,
+                                  msr, msr_content)
                : X86EMUL_OKAY);
         break;
     }
@@ -3672,7 +3674,8 @@ int hvm_msr_write_intercept(unsigned int
             goto gp_fault;
         /* If ret == 0 then this is not an MCE MSR, see other MSRs. */
         ret = ((ret == 0)
-               ? hvm_funcs.msr_write_intercept(msr, msr_content)
+               ? alternative_call(hvm_funcs.msr_write_intercept,
+                                  msr, msr_content)
                : X86EMUL_OKAY);
         break;
     }
@@ -3864,7 +3867,7 @@ void hvm_hypercall_page_initialise(struc
                                    void *hypercall_page)
 {
     hvm_latch_shinfo_size(d);
-    hvm_funcs.init_hypercall_page(d, hypercall_page);
+    alternative_vcall(hvm_funcs.init_hypercall_page, d, hypercall_page);
 }
 
 void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
@@ -5019,7 +5022,7 @@ void hvm_domain_soft_reset(struct domain
 void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
                               struct segment_register *reg)
 {
-    hvm_funcs.get_segment_register(v, seg, reg);
+    alternative_vcall(hvm_funcs.get_segment_register, v, seg, reg);
 
     switch ( seg )
     {
@@ -5165,7 +5168,7 @@ void hvm_set_segment_register(struct vcp
         return;
     }
 
-    hvm_funcs.set_segment_register(v, seg, reg);
+    alternative_vcall(hvm_funcs.set_segment_register, v, seg, reg);
 }
 
 /*
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -314,42 +314,42 @@ static inline int
 hvm_guest_x86_mode(struct vcpu *v)
 {
     ASSERT(v == current);
-    return hvm_funcs.guest_x86_mode(v);
+    return alternative_call(hvm_funcs.guest_x86_mode, v);
 }
 
 static inline void
 hvm_update_host_cr3(struct vcpu *v)
 {
     if ( hvm_funcs.update_host_cr3 )
-        hvm_funcs.update_host_cr3(v);
+        alternative_vcall(hvm_funcs.update_host_cr3, v);
 }
 
 static inline void hvm_update_guest_cr(struct vcpu *v, unsigned int cr)
 {
-    hvm_funcs.update_guest_cr(v, cr, 0);
+    alternative_vcall(hvm_funcs.update_guest_cr, v, cr, 0);
 }
 
 static inline void hvm_update_guest_cr3(struct vcpu *v, bool noflush)
 {
     unsigned int flags = noflush ? HVM_UPDATE_GUEST_CR3_NOFLUSH : 0;
 
-    hvm_funcs.update_guest_cr(v, 3, flags);
+    alternative_vcall(hvm_funcs.update_guest_cr, v, 3, flags);
 }
 
 static inline void hvm_update_guest_efer(struct vcpu *v)
 {
-    hvm_funcs.update_guest_efer(v);
+    alternative_vcall(hvm_funcs.update_guest_efer, v);
 }
 
 static inline void hvm_cpuid_policy_changed(struct vcpu *v)
 {
-    hvm_funcs.cpuid_policy_changed(v);
+    alternative_vcall(hvm_funcs.cpuid_policy_changed, v);
 }
 
 static inline void hvm_set_tsc_offset(struct vcpu *v, uint64_t offset,
                                       uint64_t at_tsc)
 {
-    hvm_funcs.set_tsc_offset(v, offset, at_tsc);
+    alternative_vcall(hvm_funcs.set_tsc_offset, v, offset, at_tsc);
 }
 
 /*
@@ -369,7 +369,7 @@ void hvm_hypercall_page_initialise(struc
 static inline unsigned int
 hvm_get_cpl(struct vcpu *v)
 {
-    return hvm_funcs.get_cpl(v);
+    return alternative_call(hvm_funcs.get_cpl, v);
 }
 
 void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
@@ -379,13 +379,13 @@ void hvm_set_segment_register(struct vcp
 
 static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
 {
-    return hvm_funcs.get_shadow_gs_base(v);
+    return alternative_call(hvm_funcs.get_shadow_gs_base, v);
 }
 
 static inline bool hvm_get_guest_bndcfgs(struct vcpu *v, u64 *val)
 {
     return hvm_funcs.get_guest_bndcfgs &&
-           hvm_funcs.get_guest_bndcfgs(v, val);
+           alternative_call(hvm_funcs.get_guest_bndcfgs, v, val);
 }
 
 bool hvm_set_guest_bndcfgs(struct vcpu *v, u64 val);
@@ -451,12 +451,12 @@ static inline void hvm_inject_page_fault
 
 static inline int hvm_event_pending(struct vcpu *v)
 {
-    return hvm_funcs.event_pending(v);
+    return alternative_call(hvm_funcs.event_pending, v);
 }
 
 static inline void hvm_invlpg(struct vcpu *v, unsigned long va)
 {
-    hvm_funcs.invlpg(v, va);
+    alternative_vcall(hvm_funcs.invlpg, v, va);
 }
 
 /* These bits in CR4 are owned by the host. */
@@ -487,7 +487,8 @@ static inline void hvm_cpu_down(void)
 
 static inline unsigned int hvm_get_insn_bytes(struct vcpu *v, uint8_t *buf)
 {
-    return (hvm_funcs.get_insn_bytes ? hvm_funcs.get_insn_bytes(v, buf) : 0);
+    return (hvm_funcs.get_insn_bytes
+            ? alternative_call(hvm_funcs.get_insn_bytes, v, buf) : 0);
 }
 
 enum hvm_task_switch_reason { TSW_jmp, TSW_iret, TSW_call_or_int };
@@ -519,7 +520,7 @@ void hvm_mapped_guest_frames_mark_dirty(
 static inline void hvm_set_info_guest(struct vcpu *v)
 {
     if ( hvm_funcs.set_info_guest )
-        return hvm_funcs.set_info_guest(v);
+        alternative_vcall(hvm_funcs.set_info_guest, v);
 }
 
 int hvm_debug_op(struct vcpu *v, int32_t op);




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 05/12] x86/HVM: patch vINTR indirect calls through hvm_funcs to direct ones

[Jan Beulich]

While not strictly necessary, change the VMX initialization logic to
update the function table in start_vmx() from NULL rather than to NULL,
to make more obvious that we won't ever change an already (explictly)
initialized function pointer.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
---
v2: Drop open-coded numbers from macro invocations.

--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -111,10 +111,15 @@ static void vlapic_clear_irr(int vector,
     vlapic_clear_vector(vector, &vlapic->regs->data[APIC_IRR]);
 }
 
-static int vlapic_find_highest_irr(struct vlapic *vlapic)
+static void sync_pir_to_irr(struct vcpu *v)
 {
     if ( hvm_funcs.sync_pir_to_irr )
-        hvm_funcs.sync_pir_to_irr(vlapic_vcpu(vlapic));
+        alternative_vcall(hvm_funcs.sync_pir_to_irr, v);
+}
+
+static int vlapic_find_highest_irr(struct vlapic *vlapic)
+{
+    sync_pir_to_irr(vlapic_vcpu(vlapic));
 
     return vlapic_find_highest_vector(&vlapic->regs->data[APIC_IRR]);
 }
@@ -143,7 +148,7 @@ bool vlapic_test_irq(const struct vlapic
         return false;
 
     if ( hvm_funcs.test_pir &&
-         hvm_funcs.test_pir(const_vlapic_vcpu(vlapic), vec) )
+         alternative_call(hvm_funcs.test_pir, const_vlapic_vcpu(vlapic), vec) )
         return true;
 
     return vlapic_test_vector(vec, &vlapic->regs->data[APIC_IRR]);
@@ -165,10 +170,10 @@ void vlapic_set_irq(struct vlapic *vlapi
         vlapic_clear_vector(vec, &vlapic->regs->data[APIC_TMR]);
 
     if ( hvm_funcs.update_eoi_exit_bitmap )
-        hvm_funcs.update_eoi_exit_bitmap(target, vec, trig);
+        alternative_vcall(hvm_funcs.update_eoi_exit_bitmap, target, vec, trig);
 
     if ( hvm_funcs.deliver_posted_intr )
-        hvm_funcs.deliver_posted_intr(target, vec);
+        alternative_vcall(hvm_funcs.deliver_posted_intr, target, vec);
     else if ( !vlapic_test_and_set_irr(vec, vlapic) )
         vcpu_kick(target);
 }
@@ -448,7 +453,7 @@ void vlapic_EOI_set(struct vlapic *vlapi
     vlapic_clear_vector(vector, &vlapic->regs->data[APIC_ISR]);
 
     if ( hvm_funcs.handle_eoi )
-        hvm_funcs.handle_eoi(vector);
+        alternative_vcall(hvm_funcs.handle_eoi, vector);
 
     vlapic_handle_EOI(vlapic, vector);
 
@@ -1429,8 +1434,7 @@ static int lapic_save_regs(struct domain
 
     for_each_vcpu ( d, v )
     {
-        if ( hvm_funcs.sync_pir_to_irr )
-            hvm_funcs.sync_pir_to_irr(v);
+        sync_pir_to_irr(v);
 
         s = vcpu_vlapic(v);
         if ( (rc = hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, s->regs)) != 0 )
@@ -1531,7 +1535,8 @@ static int lapic_load_regs(struct domain
         lapic_load_fixup(s);
 
     if ( hvm_funcs.process_isr )
-        hvm_funcs.process_isr(vlapic_find_highest_isr(s), v);
+        alternative_vcall(hvm_funcs.process_isr,
+                           vlapic_find_highest_isr(s), v);
 
     vlapic_adjust_i8259_target(d);
     lapic_rearm(s);
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2336,12 +2336,6 @@ static struct hvm_function_table __initd
     .nhvm_vcpu_vmexit_event = nvmx_vmexit_event,
     .nhvm_intr_blocked    = nvmx_intr_blocked,
     .nhvm_domain_relinquish_resources = nvmx_domain_relinquish_resources,
-    .update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap,
-    .process_isr          = vmx_process_isr,
-    .deliver_posted_intr  = vmx_deliver_posted_intr,
-    .sync_pir_to_irr      = vmx_sync_pir_to_irr,
-    .test_pir             = vmx_test_pir,
-    .handle_eoi           = vmx_handle_eoi,
     .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m,
     .enable_msr_interception = vmx_enable_msr_interception,
     .is_singlestep_supported = vmx_is_singlestep_supported,
@@ -2469,26 +2463,23 @@ const struct hvm_function_table * __init
         setup_ept_dump();
     }
 
-    if ( !cpu_has_vmx_virtual_intr_delivery )
+    if ( cpu_has_vmx_virtual_intr_delivery )
     {
-        vmx_function_table.update_eoi_exit_bitmap = NULL;
-        vmx_function_table.process_isr = NULL;
-        vmx_function_table.handle_eoi = NULL;
-    }
-    else
+        vmx_function_table.update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap;
+        vmx_function_table.process_isr = vmx_process_isr;
+        vmx_function_table.handle_eoi = vmx_handle_eoi;
         vmx_function_table.virtual_intr_delivery_enabled = true;
+    }
 
     if ( cpu_has_vmx_posted_intr_processing )
     {
         alloc_direct_apic_vector(&posted_intr_vector, pi_notification_interrupt);
         if ( iommu_intpost )
             alloc_direct_apic_vector(&pi_wakeup_vector, pi_wakeup_interrupt);
-    }
-    else
-    {
-        vmx_function_table.deliver_posted_intr = NULL;
-        vmx_function_table.sync_pir_to_irr = NULL;
-        vmx_function_table.test_pir = NULL;
+
+        vmx_function_table.deliver_posted_intr = vmx_deliver_posted_intr;
+        vmx_function_table.sync_pir_to_irr     = vmx_sync_pir_to_irr;
+        vmx_function_table.test_pir            = vmx_test_pir;
     }
 
     if ( cpu_has_vmx_tsc_scaling )




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 06/12] x86: patch ctxt_switch_masking() indirect call to direct one

[Jan Beulich]

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Drop open-coded number from macro invocation.

--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -184,7 +184,7 @@ void ctxt_switch_levelling(const struct
 	}
 
 	if (ctxt_switch_masking)
-		ctxt_switch_masking(next);
+		alternative_vcall(ctxt_switch_masking, next);
 }
 
 bool_t opt_cpu_info;



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 07/12] x86/genapic: drop .target_cpus() hook

[Jan Beulich]

All flavors specify target_cpus_all() anyway - replace use of the hook
by &cpu_online_map.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/genapic/delivery.c
+++ b/xen/arch/x86/genapic/delivery.c
@@ -5,12 +5,6 @@
 #include <asm/hardirq.h>
 #include <mach_apic.h>
 
-
-const cpumask_t *target_cpus_all(void)
-{
-	return &cpu_online_map;
-}
-
 /*
  * LOGICAL FLAT DELIVERY MODE (multicast via bitmask to <= 8 logical APIC IDs).
  */
--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -169,7 +169,6 @@ static const struct genapic apic_x2apic_
     .int_dest_mode = 0 /* physical delivery */,
     .init_apic_ldr = init_apic_ldr_x2apic_phys,
     .clustered_apic_check = clustered_apic_check_x2apic,
-    .target_cpus = target_cpus_all,
     .vector_allocation_cpumask = vector_allocation_cpumask_phys,
     .cpu_mask_to_apicid = cpu_mask_to_apicid_phys,
     .send_IPI_mask = send_IPI_mask_x2apic_phys,
@@ -182,7 +181,6 @@ static const struct genapic apic_x2apic_
     .int_dest_mode = 1 /* logical delivery */,
     .init_apic_ldr = init_apic_ldr_x2apic_cluster,
     .clustered_apic_check = clustered_apic_check_x2apic,
-    .target_cpus = target_cpus_all,
     .vector_allocation_cpumask = vector_allocation_cpumask_x2apic_cluster,
     .cpu_mask_to_apicid = cpu_mask_to_apicid_x2apic_cluster,
     .send_IPI_mask = send_IPI_mask_x2apic_cluster,
--- a/xen/include/asm-x86/genapic.h
+++ b/xen/include/asm-x86/genapic.h
@@ -33,7 +33,6 @@ struct genapic {
 	int int_dest_mode;
 	void (*init_apic_ldr)(void);
 	void (*clustered_apic_check)(void);
-	const cpumask_t *(*target_cpus)(void);
 	const cpumask_t *(*vector_allocation_cpumask)(int cpu);
 	unsigned int (*cpu_mask_to_apicid)(const cpumask_t *cpumask);
 	void (*send_IPI_mask)(const cpumask_t *mask, int vector);
@@ -51,7 +50,6 @@ struct genapic {
 extern const struct genapic *genapic;
 extern const struct genapic apic_default;
 
-const cpumask_t *target_cpus_all(void);
 void send_IPI_self_legacy(uint8_t vector);
 
 void init_apic_ldr_flat(void);
@@ -64,7 +62,6 @@ const cpumask_t *vector_allocation_cpuma
 	.int_dest_mode = 1 /* logical delivery */, \
 	.init_apic_ldr = init_apic_ldr_flat, \
 	.clustered_apic_check = clustered_apic_check_flat, \
-	.target_cpus = target_cpus_all, \
 	.vector_allocation_cpumask = vector_allocation_cpumask_flat, \
 	.cpu_mask_to_apicid = cpu_mask_to_apicid_flat, \
 	.send_IPI_mask = send_IPI_mask_flat, \
@@ -80,7 +77,6 @@ const cpumask_t *vector_allocation_cpuma
 	.int_dest_mode = 0 /* physical delivery */, \
 	.init_apic_ldr = init_apic_ldr_phys, \
 	.clustered_apic_check = clustered_apic_check_phys, \
-	.target_cpus = target_cpus_all, \
 	.vector_allocation_cpumask = vector_allocation_cpumask_phys, \
 	.cpu_mask_to_apicid = cpu_mask_to_apicid_phys, \
 	.send_IPI_mask = send_IPI_mask_phys, \
--- a/xen/include/asm-x86/mach-generic/mach_apic.h
+++ b/xen/include/asm-x86/mach-generic/mach_apic.h
@@ -12,7 +12,7 @@
 /* The following are dependent on APIC delivery mode (logical vs. physical). */
 #define INT_DELIVERY_MODE (genapic->int_delivery_mode)
 #define INT_DEST_MODE (genapic->int_dest_mode)
-#define TARGET_CPUS	  (genapic->target_cpus())
+#define TARGET_CPUS ((const typeof(cpu_online_map) *)&cpu_online_map)
 #define init_apic_ldr (genapic->init_apic_ldr)
 #define clustered_apic_check (genapic->clustered_apic_check) 
 #define cpu_mask_to_apicid (genapic->cpu_mask_to_apicid)





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 07/12] x86/genapic: drop .target_cpus() hook

[Andrew Cooper]

On 29/08/18 15:06, Jan Beulich wrote:
> All flavors specify target_cpus_all() anyway - replace use of the hook
> by &cpu_online_map.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 08/12] x86/genapic: remove indirection from genapic hook accesses

[Jan Beulich]

Instead of loading a pointer at each use site, have a single runtime
instance of struct genapic, copying into it from the individual
instances. The individual instances can this way also be moved to .init
(also adjust apic_probe[] at this occasion).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -944,8 +944,8 @@ void __init x2apic_bsp_setup(void)
 
     force_iommu = 1;
 
-    genapic = apic_x2apic_probe();
-    printk("Switched to APIC driver %s.\n", genapic->name);
+    genapic = *apic_x2apic_probe();
+    printk("Switched to APIC driver %s.\n", genapic.name);
 
     if ( !x2apic_enabled )
     {
--- a/xen/arch/x86/genapic/bigsmp.c
+++ b/xen/arch/x86/genapic/bigsmp.c
@@ -42,7 +42,7 @@ static __init int probe_bigsmp(void)
 	return def_to_bigsmp;
 } 
 
-const struct genapic apic_bigsmp = {
+const struct genapic __initconstrel apic_bigsmp = {
 	APIC_INIT("bigsmp", probe_bigsmp),
 	GENAPIC_PHYS
 };
--- a/xen/arch/x86/genapic/default.c
+++ b/xen/arch/x86/genapic/default.c
@@ -20,7 +20,7 @@ static __init int probe_default(void)
 	return 1;
 } 
 
-const struct genapic apic_default = {
+const struct genapic __initconstrel apic_default = {
 	APIC_INIT("default", probe_default),
 	GENAPIC_FLAT
 };
--- a/xen/arch/x86/genapic/probe.c
+++ b/xen/arch/x86/genapic/probe.c
@@ -15,11 +15,9 @@
 #include <asm/mach-generic/mach_apic.h>
 #include <asm/setup.h>
 
-extern const struct genapic apic_bigsmp;
+struct genapic __read_mostly genapic;
 
-const struct genapic *__read_mostly genapic;
-
-const struct genapic *apic_probe[] __initdata = {
+const struct genapic *const __initconstrel apic_probe[] = {
 	&apic_bigsmp, 
 	&apic_default,	/* must be last */
 	NULL,
@@ -36,11 +34,11 @@ void __init generic_bigsmp_probe(void)
 	 * - we find more than 8 CPUs in acpi LAPIC listing with xAPIC support
 	 */
 
-	if (!cmdline_apic && genapic == &apic_default)
+	if (!cmdline_apic && genapic.name == apic_default.name)
 		if (apic_bigsmp.probe()) {
-			genapic = &apic_bigsmp;
+			genapic = apic_bigsmp;
 			printk(KERN_INFO "Overriding APIC driver with %s\n",
-			       genapic->name);
+			       genapic.name);
 		}
 }
 
@@ -50,7 +48,7 @@ static int __init genapic_apic_force(con
 
 	for (i = 0; apic_probe[i]; i++)
 		if (!strcmp(apic_probe[i]->name, str)) {
-			genapic = apic_probe[i];
+			genapic = *apic_probe[i];
 			rc = 0;
 		}
 
@@ -66,18 +64,18 @@ void __init generic_apic_probe(void)
 	record_boot_APIC_mode();
 
 	check_x2apic_preenabled();
-	cmdline_apic = changed = (genapic != NULL);
+	cmdline_apic = changed = !!genapic.name;
 
 	for (i = 0; !changed && apic_probe[i]; i++) { 
 		if (apic_probe[i]->probe()) {
 			changed = 1;
-			genapic = apic_probe[i];
+			genapic = *apic_probe[i];
 		} 
 	}
 	if (!changed) 
-		genapic = &apic_default;
+		genapic = apic_default;
 
-	printk(KERN_INFO "Using APIC driver %s\n", genapic->name);
+	printk(KERN_INFO "Using APIC driver %s\n", genapic.name);
 } 
 
 /* These functions can switch the APIC even after the initial ->probe() */
@@ -88,9 +86,9 @@ int __init mps_oem_check(struct mp_confi
 	for (i = 0; apic_probe[i]; ++i) { 
 		if (apic_probe[i]->mps_oem_check(mpc,oem,productid)) { 
 			if (!cmdline_apic) {
-				genapic = apic_probe[i];
+				genapic = *apic_probe[i];
 				printk(KERN_INFO "Switched to APIC driver `%s'.\n", 
-				       genapic->name);
+				       genapic.name);
 			}
 			return 1;
 		} 
@@ -104,9 +102,9 @@ int __init acpi_madt_oem_check(char *oem
 	for (i = 0; apic_probe[i]; ++i) { 
 		if (apic_probe[i]->acpi_madt_oem_check(oem_id, oem_table_id)) { 
 			if (!cmdline_apic) {
-				genapic = apic_probe[i];
+				genapic = *apic_probe[i];
 				printk(KERN_INFO "Switched to APIC driver `%s'.\n", 
-				       genapic->name);
+				       genapic.name);
 			}
 			return 1;
 		} 
--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -163,7 +163,7 @@ static void send_IPI_mask_x2apic_cluster
     local_irq_restore(flags);
 }
 
-static const struct genapic apic_x2apic_phys = {
+static const struct genapic __initconstrel apic_x2apic_phys = {
     APIC_INIT("x2apic_phys", NULL),
     .int_delivery_mode = dest_Fixed,
     .int_dest_mode = 0 /* physical delivery */,
@@ -175,7 +175,7 @@ static const struct genapic apic_x2apic_
     .send_IPI_self = send_IPI_self_x2apic
 };
 
-static const struct genapic apic_x2apic_cluster = {
+static const struct genapic __initconstrel apic_x2apic_cluster = {
     APIC_INIT("x2apic_cluster", NULL),
     .int_delivery_mode = dest_LowestPrio,
     .int_dest_mode = 1 /* logical delivery */,
@@ -259,6 +259,6 @@ void __init check_x2apic_preenabled(void
     {
         printk("x2APIC mode is already enabled by BIOS.\n");
         x2apic_enabled = 1;
-        genapic = apic_x2apic_probe();
+        genapic = *apic_x2apic_probe();
     }
 }
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -162,7 +162,8 @@ static int MP_processor_info_x(struct mp
 		return -ENOSPC;
 	}
 
-	if (num_processors >= 8 && hotplug && genapic == &apic_default) {
+	if (num_processors >= 8 && hotplug
+	    && genapic.name == apic_default.name) {
 		printk(KERN_WARNING "WARNING: CPUs limit of 8 reached."
 			" Processor ignored.\n");
 		return -ENOSPC;
--- a/xen/arch/x86/smp.c
+++ b/xen/arch/x86/smp.c
@@ -29,12 +29,12 @@
 
 void send_IPI_mask(const cpumask_t *mask, int vector)
 {
-    genapic->send_IPI_mask(mask, vector);
+    genapic.send_IPI_mask(mask, vector);
 }
 
 void send_IPI_self(int vector)
 {
-    genapic->send_IPI_self(vector);
+    genapic.send_IPI_self(vector);
 }
 
 /*
--- a/xen/include/asm-x86/genapic.h
+++ b/xen/include/asm-x86/genapic.h
@@ -47,8 +47,9 @@ struct genapic {
 	APICFUNC(mps_oem_check), \
 	APICFUNC(acpi_madt_oem_check)
 
-extern const struct genapic *genapic;
+extern struct genapic genapic;
 extern const struct genapic apic_default;
+extern const struct genapic apic_bigsmp;
 
 void send_IPI_self_legacy(uint8_t vector);
 
--- a/xen/include/asm-x86/mach-generic/mach_apic.h
+++ b/xen/include/asm-x86/mach-generic/mach_apic.h
@@ -10,13 +10,13 @@
 #define esr_disable (0)
 
 /* The following are dependent on APIC delivery mode (logical vs. physical). */
-#define INT_DELIVERY_MODE (genapic->int_delivery_mode)
-#define INT_DEST_MODE (genapic->int_dest_mode)
+#define INT_DELIVERY_MODE (genapic.int_delivery_mode)
+#define INT_DEST_MODE (genapic.int_dest_mode)
 #define TARGET_CPUS ((const typeof(cpu_online_map) *)&cpu_online_map)
-#define init_apic_ldr (genapic->init_apic_ldr)
-#define clustered_apic_check (genapic->clustered_apic_check) 
-#define cpu_mask_to_apicid (genapic->cpu_mask_to_apicid)
-#define vector_allocation_cpumask(cpu) (genapic->vector_allocation_cpumask(cpu))
+#define init_apic_ldr (genapic.init_apic_ldr)
+#define clustered_apic_check (genapic.clustered_apic_check)
+#define cpu_mask_to_apicid (genapic.cpu_mask_to_apicid)
+#define vector_allocation_cpumask(cpu) (genapic.vector_allocation_cpumask(cpu))
 
 static inline void enable_apic_mode(void)
 {




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 09/12] x86/genapic: patch indirect calls to direct ones

[Jan Beulich]

For (I hope) obvious reasons only the ones used at runtime get
converted.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Drop open-coded numbers from macro invocations.

--- a/xen/arch/x86/smp.c
+++ b/xen/arch/x86/smp.c
@@ -29,12 +29,12 @@
 
 void send_IPI_mask(const cpumask_t *mask, int vector)
 {
-    genapic.send_IPI_mask(mask, vector);
+    alternative_vcall(genapic.send_IPI_mask, mask, vector);
 }
 
 void send_IPI_self(int vector)
 {
-    genapic.send_IPI_self(vector);
+    alternative_vcall(genapic.send_IPI_self, vector);
 }
 
 /*
--- a/xen/include/asm-x86/mach-generic/mach_apic.h
+++ b/xen/include/asm-x86/mach-generic/mach_apic.h
@@ -15,8 +15,18 @@
 #define TARGET_CPUS ((const typeof(cpu_online_map) *)&cpu_online_map)
 #define init_apic_ldr (genapic.init_apic_ldr)
 #define clustered_apic_check (genapic.clustered_apic_check)
-#define cpu_mask_to_apicid (genapic.cpu_mask_to_apicid)
-#define vector_allocation_cpumask(cpu) (genapic.vector_allocation_cpumask(cpu))
+#define cpu_mask_to_apicid(mask) ({ \
+	/* \
+	 * There are a number of places where the address of a local variable \
+	 * gets passed here. The use of ?: in alternative_call<N>() triggers an \
+	 * "address of ... is always true" warning in such a case with at least \
+	 * gcc 7 and 8. Hence the seemingly pointless local variable here. \
+	 */ \
+	const cpumask_t *m_ = (mask); \
+	alternative_call(genapic.cpu_mask_to_apicid, m_); \
+})
+#define vector_allocation_cpumask(cpu) \
+	alternative_call(genapic.vector_allocation_cpumask, cpu)
 
 static inline void enable_apic_mode(void)
 {





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 10/12] x86/cpuidle: patch some indirect calls to direct ones

[Jan Beulich]

For now only the ones used during entering/exiting of idle states are
converted. Additionally pm_idle{,_save} and lapic_timer_{on,off} can't
be converted, as they may get established rather late (when Dom0 is
already active).

Note that for patching to be deferred until after the pre-SMP initcalls
(from where cpuidle_init_cpu() runs the first time) the pointers need to
start out as NULL.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Drop open-coded numbers from macro invocations.

--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -102,8 +102,6 @@ bool lapic_timer_init(void)
     return true;
 }
 
-static uint64_t (*__read_mostly tick_to_ns)(uint64_t) = acpi_pm_tick_to_ns;
-
 void (*__read_mostly pm_idle_save)(void);
 unsigned int max_cstate __read_mostly = ACPI_PROCESSOR_MAX_POWER - 1;
 integer_param("max_cstate", max_cstate);
@@ -289,9 +287,9 @@ static uint64_t acpi_pm_ticks_elapsed(ui
         return ((0xFFFFFFFF - t1) + t2 +1);
 }
 
-uint64_t (*__read_mostly cpuidle_get_tick)(void) = get_acpi_pm_tick;
-static uint64_t (*__read_mostly ticks_elapsed)(uint64_t, uint64_t)
-    = acpi_pm_ticks_elapsed;
+uint64_t (*__read_mostly cpuidle_get_tick)(void);
+static uint64_t (*__read_mostly tick_to_ns)(uint64_t);
+static uint64_t (*__read_mostly ticks_elapsed)(uint64_t, uint64_t);
 
 static void print_acpi_power(uint32_t cpu, struct acpi_processor_power *power)
 {
@@ -547,7 +545,7 @@ void update_idle_stats(struct acpi_proce
                        struct acpi_processor_cx *cx,
                        uint64_t before, uint64_t after)
 {
-    int64_t sleep_ticks = ticks_elapsed(before, after);
+    int64_t sleep_ticks = alternative_call(ticks_elapsed, before, after);
     /* Interrupts are disabled */
 
     spin_lock(&power->stat_lock);
@@ -555,7 +553,8 @@ void update_idle_stats(struct acpi_proce
     cx->usage++;
     if ( sleep_ticks > 0 )
     {
-        power->last_residency = tick_to_ns(sleep_ticks) / 1000UL;
+        power->last_residency = alternative_call(tick_to_ns, sleep_ticks) /
+                                1000UL;
         cx->time += sleep_ticks;
     }
     power->last_state = &power->states[0];
@@ -635,7 +634,7 @@ static void acpi_processor_idle(void)
         if ( cx->type == ACPI_STATE_C1 || local_apic_timer_c2_ok )
         {
             /* Get start time (ticks) */
-            t1 = cpuidle_get_tick();
+            t1 = alternative_call(cpuidle_get_tick);
             /* Trace cpu idle entry */
             TRACE_4D(TRC_PM_IDLE_ENTRY, cx->idx, t1, exp, pred);
 
@@ -644,7 +643,7 @@ static void acpi_processor_idle(void)
             /* Invoke C2 */
             acpi_idle_do_entry(cx);
             /* Get end time (ticks) */
-            t2 = cpuidle_get_tick();
+            t2 = alternative_call(cpuidle_get_tick);
             trace_exit_reason(irq_traced);
             /* Trace cpu idle exit */
             TRACE_6D(TRC_PM_IDLE_EXIT, cx->idx, t2,
@@ -666,7 +665,7 @@ static void acpi_processor_idle(void)
         lapic_timer_off();
 
         /* Get start time (ticks) */
-        t1 = cpuidle_get_tick();
+        t1 = alternative_call(cpuidle_get_tick);
         /* Trace cpu idle entry */
         TRACE_4D(TRC_PM_IDLE_ENTRY, cx->idx, t1, exp, pred);
 
@@ -717,7 +716,7 @@ static void acpi_processor_idle(void)
         }
 
         /* Get end time (ticks) */
-        t2 = cpuidle_get_tick();
+        t2 = alternative_call(cpuidle_get_tick);
 
         /* recovering TSC */
         cstate_restore_tsc();
@@ -827,11 +826,20 @@ int cpuidle_init_cpu(unsigned int cpu)
     {
         unsigned int i;
 
-        if ( cpu == 0 && boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
+        if ( cpu == 0 && system_state < SYS_STATE_active )
         {
-            cpuidle_get_tick = get_stime_tick;
-            ticks_elapsed = stime_ticks_elapsed;
-            tick_to_ns = stime_tick_to_ns;
+            if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
+            {
+                cpuidle_get_tick = get_stime_tick;
+                ticks_elapsed = stime_ticks_elapsed;
+                tick_to_ns = stime_tick_to_ns;
+            }
+            else
+            {
+                cpuidle_get_tick = get_acpi_pm_tick;
+                ticks_elapsed = acpi_pm_ticks_elapsed;
+                tick_to_ns = acpi_pm_tick_to_ns;
+            }
         }
 
         acpi_power = xzalloc(struct acpi_processor_power);
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -778,7 +778,7 @@ static void mwait_idle(void)
 	if (!(lapic_timer_reliable_states & (1 << cstate)))
 		lapic_timer_off();
 
-	before = cpuidle_get_tick();
+	before = alternative_call(cpuidle_get_tick);
 	TRACE_4D(TRC_PM_IDLE_ENTRY, cx->type, before, exp, pred);
 
 	update_last_cx_stat(power, cx, before);
@@ -786,7 +786,7 @@ static void mwait_idle(void)
 	if (cpu_is_haltable(cpu))
 		mwait_idle_with_hints(eax, MWAIT_ECX_INTERRUPT_BREAK);
 
-	after = cpuidle_get_tick();
+	after = alternative_call(cpuidle_get_tick);
 
 	cstate_restore_tsc();
 	trace_exit_reason(irq_traced);




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 11/12] cpufreq: convert to a single post-init driver (hooks) instance

[Jan Beulich]

This reduces the post-init memory footprint, eliminates a pointless
level of indirection at the use sites, and allows for subsequent
alternatives call patching.

Take the opportunity and also add a name to the PowerNow! instance.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -53,8 +53,6 @@ enum {
 
 struct acpi_cpufreq_data *cpufreq_drv_data[NR_CPUS];
 
-static struct cpufreq_driver acpi_cpufreq_driver;
-
 static bool __read_mostly acpi_pstate_strict;
 boolean_param("acpi_pstate_strict", acpi_pstate_strict);
 
@@ -355,7 +353,7 @@ static void feature_detect(void *info)
     if ( cpu_has_aperfmperf )
     {
         policy->aperf_mperf = 1;
-        acpi_cpufreq_driver.getavg = get_measured_perf;
+        cpufreq_driver.getavg = get_measured_perf;
     }
 
     eax = cpuid_eax(6);
@@ -593,7 +591,7 @@ acpi_cpufreq_cpu_init(struct cpufreq_pol
         policy->cur = acpi_cpufreq_guess_freq(data, policy->cpu);
         break;
     case ACPI_ADR_SPACE_FIXED_HARDWARE:
-        acpi_cpufreq_driver.get = get_cur_freq_on_cpu;
+        cpufreq_driver.get = get_cur_freq_on_cpu;
         policy->cur = get_cur_freq_on_cpu(cpu);
         break;
     default:
@@ -635,7 +633,7 @@ static int acpi_cpufreq_cpu_exit(struct
     return 0;
 }
 
-static struct cpufreq_driver acpi_cpufreq_driver = {
+static const struct cpufreq_driver __initconstrel acpi_cpufreq_driver = {
     .name   = "acpi-cpufreq",
     .verify = acpi_cpufreq_verify,
     .target = acpi_cpufreq_target,
@@ -656,7 +654,7 @@ static int __init cpufreq_driver_init(vo
 
     return ret;
 }
-__initcall(cpufreq_driver_init);
+presmp_initcall(cpufreq_driver_init);
 
 int cpufreq_cpu_init(unsigned int cpuid)
 {
--- a/xen/arch/x86/acpi/cpufreq/powernow.c
+++ b/xen/arch/x86/acpi/cpufreq/powernow.c
@@ -52,8 +52,6 @@
 
 #define ARCH_CPU_FLAG_RESUME	1
 
-static struct cpufreq_driver powernow_cpufreq_driver;
-
 static void transition_pstate(void *pstate)
 {
     wrmsrl(MSR_PSTATE_CTRL, *(unsigned int *)pstate);
@@ -215,7 +213,7 @@ static void feature_detect(void *info)
     if ( cpu_has_aperfmperf )
     {
         policy->aperf_mperf = 1;
-        powernow_cpufreq_driver.getavg = get_measured_perf;
+        cpufreq_driver.getavg = get_measured_perf;
     }
 
     edx = cpuid_edx(CPUID_FREQ_VOLT_CAPABILITIES);
@@ -347,7 +345,8 @@ static int powernow_cpufreq_cpu_exit(str
     return 0;
 }
 
-static struct cpufreq_driver powernow_cpufreq_driver = {
+static const struct cpufreq_driver __initconstrel powernow_cpufreq_driver = {
+    .name   = "powernow",
     .verify = powernow_cpufreq_verify,
     .target = powernow_cpufreq_target,
     .init   = powernow_cpufreq_cpu_init,
--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -64,7 +64,7 @@ int do_get_pm_info(struct xen_sysctl_get
     case PMSTAT_PX:
         if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_PX) )
             return -ENODEV;
-        if ( !cpufreq_driver )
+        if ( !cpufreq_driver.init )
             return -ENODEV;
         if ( !pmpt || !(pmpt->perf.init & XEN_PX_INIT) )
             return -EINVAL;
@@ -255,16 +255,16 @@ static int get_cpufreq_para(struct xen_s
         return ret;
 
     op->u.get_para.cpuinfo_cur_freq =
-        cpufreq_driver->get ? cpufreq_driver->get(op->cpuid) : policy->cur;
+        cpufreq_driver.get ? cpufreq_driver.get(op->cpuid) : policy->cur;
     op->u.get_para.cpuinfo_max_freq = policy->cpuinfo.max_freq;
     op->u.get_para.cpuinfo_min_freq = policy->cpuinfo.min_freq;
     op->u.get_para.scaling_cur_freq = policy->cur;
     op->u.get_para.scaling_max_freq = policy->max;
     op->u.get_para.scaling_min_freq = policy->min;
 
-    if ( cpufreq_driver->name[0] )
+    if ( cpufreq_driver.name[0] )
         strlcpy(op->u.get_para.scaling_driver, 
-            cpufreq_driver->name, CPUFREQ_NAME_LEN);
+            cpufreq_driver.name, CPUFREQ_NAME_LEN);
     else
         strlcpy(op->u.get_para.scaling_driver, "Unknown", CPUFREQ_NAME_LEN);
 
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -172,7 +172,7 @@ int cpufreq_add_cpu(unsigned int cpu)
     if ( !(perf->init & XEN_PX_INIT) )
         return -EINVAL;
 
-    if (!cpufreq_driver)
+    if (!cpufreq_driver.init)
         return 0;
 
     if (per_cpu(cpufreq_cpu_policy, cpu))
@@ -239,7 +239,7 @@ int cpufreq_add_cpu(unsigned int cpu)
         policy->cpu = cpu;
         per_cpu(cpufreq_cpu_policy, cpu) = policy;
 
-        ret = cpufreq_driver->init(policy);
+        ret = cpufreq_driver.init(policy);
         if (ret) {
             free_cpumask_var(policy->cpus);
             xfree(policy);
@@ -298,7 +298,7 @@ err1:
     cpumask_clear_cpu(cpu, cpufreq_dom->map);
 
     if (cpumask_empty(policy->cpus)) {
-        cpufreq_driver->exit(policy);
+        cpufreq_driver.exit(policy);
         free_cpumask_var(policy->cpus);
         xfree(policy);
     }
@@ -362,7 +362,7 @@ int cpufreq_del_cpu(unsigned int cpu)
     cpumask_clear_cpu(cpu, cpufreq_dom->map);
 
     if (cpumask_empty(policy->cpus)) {
-        cpufreq_driver->exit(policy);
+        cpufreq_driver.exit(policy);
         free_cpumask_var(policy->cpus);
         xfree(policy);
     }
@@ -663,17 +663,17 @@ static int __init cpufreq_presmp_init(vo
 }
 presmp_initcall(cpufreq_presmp_init);
 
-int __init cpufreq_register_driver(struct cpufreq_driver *driver_data)
+int __init cpufreq_register_driver(const struct cpufreq_driver *driver_data)
 {
    if ( !driver_data || !driver_data->init ||
         !driver_data->verify || !driver_data->exit ||
         (!driver_data->target == !driver_data->setpolicy) )
         return -EINVAL;
 
-    if ( cpufreq_driver )
+    if ( cpufreq_driver.init )
         return -EBUSY;
 
-    cpufreq_driver = driver_data;
+    cpufreq_driver = *driver_data;
 
     return 0;
 }
--- a/xen/drivers/cpufreq/utility.c
+++ b/xen/drivers/cpufreq/utility.c
@@ -31,7 +31,7 @@
 #include <acpi/cpufreq/cpufreq.h>
 #include <public/sysctl.h>
 
-struct cpufreq_driver   *cpufreq_driver;
+struct cpufreq_driver __read_mostly cpufreq_driver;
 struct processor_pminfo *__read_mostly processor_pminfo[NR_CPUS];
 DEFINE_PER_CPU_READ_MOSTLY(struct cpufreq_policy *, cpufreq_cpu_policy);
 
@@ -360,11 +360,11 @@ int __cpufreq_driver_target(struct cpufr
 {
     int retval = -EINVAL;
 
-    if (cpu_online(policy->cpu) && cpufreq_driver->target)
+    if (cpu_online(policy->cpu) && cpufreq_driver.target)
     {
         unsigned int prev_freq = policy->cur;
 
-        retval = cpufreq_driver->target(policy, target_freq, relation);
+        retval = cpufreq_driver.target(policy, target_freq, relation);
         if ( retval == 0 )
             TRACE_2D(TRC_PM_FREQ_CHANGE, prev_freq/1000, policy->cur/1000);
     }
@@ -380,9 +380,9 @@ int cpufreq_driver_getavg(unsigned int c
     if (!cpu_online(cpu) || !(policy = per_cpu(cpufreq_cpu_policy, cpu)))
         return 0;
 
-    if (cpufreq_driver->getavg)
+    if (cpufreq_driver.getavg)
     {
-        freq_avg = cpufreq_driver->getavg(cpu, flag);
+        freq_avg = cpufreq_driver.getavg(cpu, flag);
         if (freq_avg > 0)
             return freq_avg;
     }
@@ -412,9 +412,9 @@ int cpufreq_update_turbo(int cpuid, int
         return 0;
 
     policy->turbo = new_state;
-    if (cpufreq_driver->update)
+    if (cpufreq_driver.update)
     {
-        ret = cpufreq_driver->update(cpuid, policy);
+        ret = cpufreq_driver.update(cpuid, policy);
         if (ret)
             policy->turbo = curr_state;
     }
@@ -450,15 +450,15 @@ int __cpufreq_set_policy(struct cpufreq_
         return -EINVAL;
 
     /* verify the cpu speed can be set within this limit */
-    ret = cpufreq_driver->verify(policy);
+    ret = cpufreq_driver.verify(policy);
     if (ret)
         return ret;
 
     data->min = policy->min;
     data->max = policy->max;
     data->limits = policy->limits;
-    if (cpufreq_driver->setpolicy)
-        return cpufreq_driver->setpolicy(data);
+    if (cpufreq_driver.setpolicy)
+        return cpufreq_driver.setpolicy(data);
 
     if (policy->governor != data->governor) {
         /* save old, working values */
--- a/xen/include/acpi/cpufreq/cpufreq.h
+++ b/xen/include/acpi/cpufreq/cpufreq.h
@@ -153,7 +153,7 @@ __cpufreq_governor(struct cpufreq_policy
 #define CPUFREQ_RELATION_H 1  /* highest frequency below or at target */
 
 struct cpufreq_driver {
-    char   name[CPUFREQ_NAME_LEN];
+    const char *name;
     int    (*init)(struct cpufreq_policy *policy);
     int    (*verify)(struct cpufreq_policy *policy);
     int    (*setpolicy)(struct cpufreq_policy *policy);
@@ -166,9 +166,9 @@ struct cpufreq_driver {
     int    (*exit)(struct cpufreq_policy *policy);
 };
 
-extern struct cpufreq_driver *cpufreq_driver;
+extern struct cpufreq_driver cpufreq_driver;
 
-int cpufreq_register_driver(struct cpufreq_driver *);
+int cpufreq_register_driver(const struct cpufreq_driver *);
 
 static __inline__
 void cpufreq_verify_within_limits(struct cpufreq_policy *policy,




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 12/12] cpufreq: patch target() indirect call to direct one

[Jan Beulich]

This looks to be the only frequently executed hook; don't bother
patching any other ones.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/drivers/cpufreq/utility.c
+++ b/xen/drivers/cpufreq/utility.c
@@ -364,7 +364,8 @@ int __cpufreq_driver_target(struct cpufr
     {
         unsigned int prev_freq = policy->cur;
 
-        retval = cpufreq_driver.target(policy, target_freq, relation);
+        retval = alternative_call(cpufreq_driver.target,
+                                  policy, target_freq, relation);
         if ( retval == 0 )
             TRACE_2D(TRC_PM_FREQ_CHANGE, prev_freq/1000, policy->cur/1000);
     }



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 00/12] x86: indirect call overhead reduction

[Jan Beulich]

>>> On 29.08.18 at 15:55, <JBeulich@suse.com> wrote:
> While indirect calls have always been more expensive than direct ones,
> their cost has further increased with the Spectre v2 mitigations. In a
> number of cases we simply pointlessly use them in the first place. In
> many other cases the indirection solely exists to abstract from e.g.
> vendor specific hardware details, and hence the pointers used never
> change once set. Here we can use alternatives patching to get rid of
> the indirection.
> 
> From patch 2 onwards dependencies exist on earlier, yet to be reviewed

Oh, this was definitely meant to be "From patch 3 onwards ...".

Jan

> patches ("x86/alternatives: fully leverage automatic NOP filling" as well
> as the "x86: improve PDX <-> PFN and alike translations" series at the
> very least). I nevertheless wanted to enable a first round of review of
> the series, the more that some of the patches (not just initial ones)
> could perhaps be taken irrespective of those dependencies. The first
> two of the three genapic patches, otoh, are entirely independent and
> could go in right away afaict if they were ack-ed.
> 
> Further areas where indirect calls could be eliminated (and that I've put
> on my todo list in case the general concept here is deemed reasonable)
> are IOMMU, vPMU, and XSM. For some of these, the ARM side would
> need dealing with as well - I'm not sure whether replacing indirect calls
> by direct ones is worthwhile there as well; if not, the wrappers would
> simply need to become function invocations in the ARM case (as was
> already asked for in the IOMMU case).
> 
> 01: VMX: reduce number of posted-interrupt hooks
> 02: x86/alternatives: allow using assembler macros in favor of C ones
> 03: x86: infrastructure to allow converting certain indirect calls to direct ones
> 04: x86/HVM: patch indirect calls through hvm_funcs to direct ones
> 05: x86/HVM: patch vINTR indirect calls through hvm_funcs to direct ones
> 06: x86: patch ctxt_switch_masking() indirect call to direct one
> 07: x86/genapic: drop .target_cpus() hook
> 08: x86/genapic: remove indirection from genapic hook accesses
> 09: x86/genapic: patch indirect calls to direct ones
> 10: x86/cpuidle: patch some indirect calls to direct ones
> 11: cpufreq: convert to a single post-init driver (hooks) instance
> 12: cpufreq: patch target() indirect call to direct one
> 
> Jan
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org 
> https://lists.xenproject.org/mailman/listinfo/xen-devel 





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel