* [PATCH v3 1/7] x86: make traps.c build with !CONFIG_PV
2018-11-02 15:55 [PATCH v3 0/7] Make CONFIG_PV work on x86 Wei Liu
@ 2018-11-02 15:55 ` Wei Liu
2018-11-05 14:07 ` Andrew Cooper
2018-11-02 15:55 ` [PATCH v3 2/7] x86/domctl: rework XEN_DOMCTL_{set, get}_address_size Wei Liu
` (5 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Wei Liu @ 2018-11-02 15:55 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
Provide a stub for pv_inject_event, put code that accesses PV fields
and GDT / LDT fault handling code under CONFIG_PV.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v3:
1. don't skip updating dr_masks in activate_debugregs.
2. remove ASSERT_UNREACHABLE in fixup_page_fault.
v2: reduce the amount of ifdefs
---
xen/arch/x86/traps.c | 16 ++++++++++++++++
xen/include/asm-x86/domain.h | 7 +++++++
2 files changed, 23 insertions(+)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index c60c8f5..69449e0 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1115,6 +1115,7 @@ static void reserved_bit_page_fault(unsigned long addr,
show_execution_state(regs);
}
+#ifdef CONFIG_PV
static int handle_ldt_mapping_fault(unsigned int offset,
struct cpu_user_regs *regs)
{
@@ -1185,6 +1186,7 @@ static int handle_gdt_ldt_mapping_fault(unsigned long offset,
return EXCRET_fault_fixed;
}
+#endif
#define IN_HYPERVISOR_RANGE(va) \
(((va) >= HYPERVISOR_VIRT_START) && ((va) < HYPERVISOR_VIRT_END))
@@ -1335,10 +1337,12 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
if ( unlikely(IN_HYPERVISOR_RANGE(addr)) )
{
+#ifdef CONFIG_PV
if ( !(regs->error_code & (PFEC_user_mode | PFEC_reserved_bit)) &&
(addr >= GDT_LDT_VIRT_START) && (addr < GDT_LDT_VIRT_END) )
return handle_gdt_ldt_mapping_fault(
addr - GDT_LDT_VIRT_START, regs);
+#endif
return 0;
}
@@ -1494,7 +1498,9 @@ void __init do_early_page_fault(struct cpu_user_regs *regs)
void do_general_protection(struct cpu_user_regs *regs)
{
+#ifdef CONFIG_PV
struct vcpu *v = current;
+#endif
unsigned long fixup;
if ( debugger_trap_entry(TRAP_gp_fault, regs) )
@@ -1506,6 +1512,7 @@ void do_general_protection(struct cpu_user_regs *regs)
if ( !guest_mode(regs) )
goto gp_in_kernel;
+#ifdef CONFIG_PV
/*
* Cunning trick to allow arbitrary "INT n" handling.
*
@@ -1557,6 +1564,7 @@ void do_general_protection(struct cpu_user_regs *regs)
/* Pass on GPF as is. */
pv_inject_hw_exception(TRAP_gp_fault, regs->error_code);
return;
+#endif
gp_in_kernel:
@@ -1744,7 +1752,9 @@ void unset_nmi_callback(void)
void do_device_not_available(struct cpu_user_regs *regs)
{
+#ifdef CONFIG_PV
struct vcpu *curr = current;
+#endif
if ( !guest_mode(regs) )
{
@@ -1762,6 +1772,7 @@ void do_device_not_available(struct cpu_user_regs *regs)
return;
}
+#ifdef CONFIG_PV
vcpu_restore_fpu_lazy(curr);
if ( curr->arch.pv.ctrlreg[0] & X86_CR0_TS )
@@ -1771,6 +1782,9 @@ void do_device_not_available(struct cpu_user_regs *regs)
}
else
TRACE_0D(TRC_PV_MATH_STATE_RESTORE);
+#else
+ ASSERT_UNREACHABLE();
+#endif
return;
}
@@ -2078,6 +2092,7 @@ void activate_debugregs(const struct vcpu *curr)
}
}
+#ifdef CONFIG_PV
/*
* Used by hypercalls and the emulator.
* -ENODEV => #UD
@@ -2193,6 +2208,7 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
return 0;
}
+#endif /* CONFIG_PV */
void asm_domain_crash_synchronous(unsigned long addr)
{
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 7214037..643e69a 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -681,7 +681,14 @@ void arch_vcpu_regs_init(struct vcpu *v);
struct vcpu_hvm_context;
int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context *ctx);
+#ifdef CONFIG_PV
void pv_inject_event(const struct x86_event *event);
+#else
+static inline void pv_inject_event(const struct x86_event *event)
+{
+ ASSERT_UNREACHABLE();
+}
+#endif
static inline void pv_inject_hw_exception(unsigned int vector, int errcode)
{
--
git-series 0.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 1/7] x86: make traps.c build with !CONFIG_PV
2018-11-02 15:55 ` [PATCH v3 1/7] x86: make traps.c build with !CONFIG_PV Wei Liu
@ 2018-11-05 14:07 ` Andrew Cooper
0 siblings, 0 replies; 29+ messages in thread
From: Andrew Cooper @ 2018-11-05 14:07 UTC (permalink / raw)
To: Wei Liu, xen-devel; +Cc: Jan Beulich
On 02/11/18 15:55, Wei Liu wrote:
> @@ -2078,6 +2092,7 @@ void activate_debugregs(const struct vcpu *curr)
> }
> }
>
> +#ifdef CONFIG_PV
> /*
> * Used by hypercalls and the emulator.
> * -ENODEV => #UD
> @@ -2193,6 +2208,7 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
>
> return 0;
> }
> +#endif /* CONFIG_PV */
set_debugreg() doesn't really belong here. How about moving it to
pv/misc-hypercalls.c ?
Either way, 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 2/7] x86/domctl: rework XEN_DOMCTL_{set, get}_address_size
2018-11-02 15:55 [PATCH v3 0/7] Make CONFIG_PV work on x86 Wei Liu
2018-11-02 15:55 ` [PATCH v3 1/7] x86: make traps.c build with !CONFIG_PV Wei Liu
@ 2018-11-02 15:55 ` Wei Liu
2018-11-05 14:08 ` Andrew Cooper
2018-11-02 15:55 ` [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV Wei Liu
` (4 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Wei Liu @ 2018-11-02 15:55 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
Going through toolstack code, they are used for PV guests only.
Tighten their access to PV only. Return -EOPNOTSUPP if they are called
on HVM guests. Rewrite the code in a pattern that makes DCE work.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v3: rewritten
---
xen/arch/x86/domctl.c | 32 +++++++++++++++++++++++---------
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index f79827e..33f9a86 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -609,19 +609,33 @@ long arch_do_domctl(
break;
case XEN_DOMCTL_set_address_size:
- if ( ((domctl->u.address_size.size == 64) && !d->arch.is_32bit_pv) ||
- ((domctl->u.address_size.size == 32) && d->arch.is_32bit_pv) )
- ret = 0;
- else if ( domctl->u.address_size.size == 32 )
- ret = switch_compat(d);
+ if ( is_hvm_domain(d) )
+ ret = -EOPNOTSUPP;
+ else if ( is_pv_domain(d) )
+ {
+ if ( ((domctl->u.address_size.size == 64) && !d->arch.is_32bit_pv) ||
+ ((domctl->u.address_size.size == 32) && d->arch.is_32bit_pv) )
+ ret = 0;
+ else if ( domctl->u.address_size.size == 32 )
+ ret = switch_compat(d);
+ else
+ ret = -EINVAL;
+ }
else
- ret = -EINVAL;
+ ASSERT_UNREACHABLE();
break;
case XEN_DOMCTL_get_address_size:
- domctl->u.address_size.size = is_pv_32bit_domain(d) ? 32 :
- BITS_PER_LONG;
- copyback = true;
+ if ( is_hvm_domain(d) )
+ ret = -EOPNOTSUPP;
+ else if ( is_pv_domain(d) )
+ {
+ domctl->u.address_size.size =
+ is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG;
+ copyback = true;
+ }
+ else
+ ASSERT_UNREACHABLE();
break;
case XEN_DOMCTL_set_machine_address_size:
--
git-series 0.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV
2018-11-02 15:55 [PATCH v3 0/7] Make CONFIG_PV work on x86 Wei Liu
2018-11-02 15:55 ` [PATCH v3 1/7] x86: make traps.c build with !CONFIG_PV Wei Liu
2018-11-02 15:55 ` [PATCH v3 2/7] x86/domctl: rework XEN_DOMCTL_{set, get}_address_size Wei Liu
@ 2018-11-02 15:55 ` Wei Liu
2018-11-05 14:13 ` Andrew Cooper
2018-11-05 15:04 ` Jan Beulich
2018-11-02 15:55 ` [PATCH v3 4/7] x86: rearrange x86_64/entry.S Wei Liu
` (3 subsequent siblings)
6 siblings, 2 replies; 29+ messages in thread
From: Wei Liu @ 2018-11-02 15:55 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Kevin Tian, Wei Liu, Jan Beulich, Jun Nakajima
We want Xen to crash if we hit these paths when PV is disabled.
For syscall, we provide stubs for {l,c}star_enter which end up calling
panic. For sysenter, we initialise CS to 0 so that #GP can be raised.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v3: rewrite
---
xen/arch/x86/hvm/vmx/vmcs.c | 5 +++--
xen/arch/x86/x86_64/traps.c | 19 +++++++++++++++++--
2 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index d9747b4..dec21d1 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1160,8 +1160,9 @@ static int construct_vmcs(struct vcpu *v)
__vmwrite(HOST_RIP, (unsigned long)vmx_asm_vmexit_handler);
/* Host SYSENTER CS:RIP. */
- __vmwrite(HOST_SYSENTER_CS, __HYPERVISOR_CS);
- __vmwrite(HOST_SYSENTER_EIP, (unsigned long)sysenter_entry);
+ __vmwrite(HOST_SYSENTER_CS, IS_ENABLED(CONFIG_PV) ? __HYPERVISOR_CS : 0);
+ __vmwrite(HOST_SYSENTER_EIP,
+ IS_ENABLED(CONFIG_PV) ? (unsigned long)sysenter_entry : 0);
/* MSR intercepts. */
__vmwrite(VM_EXIT_MSR_LOAD_COUNT, 0);
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index 27154f2..35a60d4 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
}
DEFINE_PER_CPU(struct stubs, stubs);
+
+#ifdef CONFIG_PV
void lstar_enter(void);
void cstar_enter(void);
+#else
+static inline void lstar_enter(void)
+{
+ panic("%s called", __func__);
+}
+
+static inline void cstar_enter(void)
+{
+ panic("%s called", __func__);
+}
+#endif /* CONFIG_PV */
void subarch_percpu_traps_init(void)
{
@@ -329,8 +342,10 @@ void subarch_percpu_traps_init(void)
{
/* SYSENTER entry. */
wrmsrl(MSR_IA32_SYSENTER_ESP, stack_bottom);
- wrmsrl(MSR_IA32_SYSENTER_EIP, (unsigned long)sysenter_entry);
- wrmsr(MSR_IA32_SYSENTER_CS, __HYPERVISOR_CS, 0);
+ wrmsrl(MSR_IA32_SYSENTER_EIP,
+ IS_ENABLED(CONFIG_PV) ? (unsigned long)sysenter_entry : 0);
+ wrmsr(MSR_IA32_SYSENTER_CS,
+ IS_ENABLED(CONFIG_PV) ? __HYPERVISOR_CS : 0, 0);
}
/* Trampoline for SYSCALL entry from compatibility mode. */
--
git-series 0.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV
2018-11-02 15:55 ` [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV Wei Liu
@ 2018-11-05 14:13 ` Andrew Cooper
2018-11-05 15:04 ` Jan Beulich
1 sibling, 0 replies; 29+ messages in thread
From: Andrew Cooper @ 2018-11-05 14:13 UTC (permalink / raw)
To: Wei Liu, xen-devel; +Cc: Kevin Tian, Jan Beulich, Jun Nakajima
On 02/11/18 15:55, Wei Liu wrote:
> We want Xen to crash if we hit these paths when PV is disabled.
>
> For syscall, we provide stubs for {l,c}star_enter which end up calling
> panic. For sysenter, we initialise CS to 0 so that #GP can be raised.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> v3: rewrite
> ---
> xen/arch/x86/hvm/vmx/vmcs.c | 5 +++--
> xen/arch/x86/x86_64/traps.c | 19 +++++++++++++++++--
> 2 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index d9747b4..dec21d1 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1160,8 +1160,9 @@ static int construct_vmcs(struct vcpu *v)
> __vmwrite(HOST_RIP, (unsigned long)vmx_asm_vmexit_handler);
>
> /* Host SYSENTER CS:RIP. */
> - __vmwrite(HOST_SYSENTER_CS, __HYPERVISOR_CS);
> - __vmwrite(HOST_SYSENTER_EIP, (unsigned long)sysenter_entry);
> + __vmwrite(HOST_SYSENTER_CS, IS_ENABLED(CONFIG_PV) ? __HYPERVISOR_CS : 0);
> + __vmwrite(HOST_SYSENTER_EIP,
> + IS_ENABLED(CONFIG_PV) ? (unsigned long)sysenter_entry : 0);
Jun/Kevin: Given that the VMCS backing page is zeroed, is it safe to
omit these VMWRITE's entirely in the !CONFIG_PV case, rather than
explicitly writing 0?
It would be more efficient, especially when nested, if it is safe to do so.
>
> /* MSR intercepts. */
> __vmwrite(VM_EXIT_MSR_LOAD_COUNT, 0);
> diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
> index 27154f2..35a60d4 100644
> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
> }
>
> DEFINE_PER_CPU(struct stubs, stubs);
> +
> +#ifdef CONFIG_PV
> void lstar_enter(void);
> void cstar_enter(void);
> +#else
> +static inline void lstar_enter(void)
> +{
> + panic("%s called", __func__);
\n
With this fixed, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> +}
> +
> +static inline void cstar_enter(void)
> +{
> + panic("%s called", __func__);
> +}
> +#endif /* CONFIG_PV */
>
> void subarch_percpu_traps_init(void)
> {
> @@ -329,8 +342,10 @@ void subarch_percpu_traps_init(void)
> {
> /* SYSENTER entry. */
> wrmsrl(MSR_IA32_SYSENTER_ESP, stack_bottom);
> - wrmsrl(MSR_IA32_SYSENTER_EIP, (unsigned long)sysenter_entry);
> - wrmsr(MSR_IA32_SYSENTER_CS, __HYPERVISOR_CS, 0);
> + wrmsrl(MSR_IA32_SYSENTER_EIP,
> + IS_ENABLED(CONFIG_PV) ? (unsigned long)sysenter_entry : 0);
> + wrmsr(MSR_IA32_SYSENTER_CS,
> + IS_ENABLED(CONFIG_PV) ? __HYPERVISOR_CS : 0, 0);
> }
>
> /* Trampoline for SYSCALL entry from compatibility mode. */
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV
2018-11-02 15:55 ` [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV Wei Liu
2018-11-05 14:13 ` Andrew Cooper
@ 2018-11-05 15:04 ` Jan Beulich
2018-11-05 15:48 ` Wei Liu
1 sibling, 1 reply; 29+ messages in thread
From: Jan Beulich @ 2018-11-05 15:04 UTC (permalink / raw)
To: Wei Liu; +Cc: Andrew Cooper, Kevin Tian, Jun Nakajima, xen-devel
>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
> }
>
> DEFINE_PER_CPU(struct stubs, stubs);
> +
> +#ifdef CONFIG_PV
> void lstar_enter(void);
> void cstar_enter(void);
> +#else
> +static inline void lstar_enter(void)
> +{
> + panic("%s called", __func__);
> +}
> +
> +static inline void cstar_enter(void)
> +{
> + panic("%s called", __func__);
> +}
> +#endif /* CONFIG_PV */
Do we really need two separate stubs (and two separate string literals)
here?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV
2018-11-05 15:04 ` Jan Beulich
@ 2018-11-05 15:48 ` Wei Liu
2018-11-05 15:49 ` Andrew Cooper
0 siblings, 1 reply; 29+ messages in thread
From: Wei Liu @ 2018-11-05 15:48 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Kevin Tian, Wei Liu, Jun Nakajima, xen-devel
On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
> >>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
> > --- a/xen/arch/x86/x86_64/traps.c
> > +++ b/xen/arch/x86/x86_64/traps.c
> > @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
> > }
> >
> > DEFINE_PER_CPU(struct stubs, stubs);
> > +
> > +#ifdef CONFIG_PV
> > void lstar_enter(void);
> > void cstar_enter(void);
> > +#else
> > +static inline void lstar_enter(void)
> > +{
> > + panic("%s called", __func__);
> > +}
> > +
> > +static inline void cstar_enter(void)
> > +{
> > + panic("%s called", __func__);
> > +}
> > +#endif /* CONFIG_PV */
>
> Do we really need two separate stubs (and two separate string literals)
> here?
I think it is clearer if we have two distinct messages. But I'm not too
fussed either way really. If you feel strongly about this, I'm happy to
change it to only one function.
Wei.
>
> 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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV
2018-11-05 15:48 ` Wei Liu
@ 2018-11-05 15:49 ` Andrew Cooper
2018-11-05 15:51 ` Wei Liu
2018-11-05 16:11 ` Jan Beulich
0 siblings, 2 replies; 29+ messages in thread
From: Andrew Cooper @ 2018-11-05 15:49 UTC (permalink / raw)
To: Wei Liu, Jan Beulich; +Cc: xen-devel, Kevin Tian, Jun Nakajima
On 05/11/18 15:48, Wei Liu wrote:
> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
>>> --- a/xen/arch/x86/x86_64/traps.c
>>> +++ b/xen/arch/x86/x86_64/traps.c
>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
>>> }
>>>
>>> DEFINE_PER_CPU(struct stubs, stubs);
>>> +
>>> +#ifdef CONFIG_PV
>>> void lstar_enter(void);
>>> void cstar_enter(void);
>>> +#else
>>> +static inline void lstar_enter(void)
>>> +{
>>> + panic("%s called", __func__);
>>> +}
>>> +
>>> +static inline void cstar_enter(void)
>>> +{
>>> + panic("%s called", __func__);
>>> +}
>>> +#endif /* CONFIG_PV */
>> Do we really need two separate stubs (and two separate string literals)
>> here?
> I think it is clearer if we have two distinct messages. But I'm not too
> fussed either way really. If you feel strongly about this, I'm happy to
> change it to only one function.
This is the correct way to do it. __func__ will already be in the
string table, and the format string (being identical) will be merged.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV
2018-11-05 15:49 ` Andrew Cooper
@ 2018-11-05 15:51 ` Wei Liu
2018-11-05 16:11 ` Jan Beulich
1 sibling, 0 replies; 29+ messages in thread
From: Wei Liu @ 2018-11-05 15:51 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Kevin Tian, Wei Liu, Jun Nakajima, Jan Beulich
On Mon, Nov 05, 2018 at 03:49:40PM +0000, Andrew Cooper wrote:
> On 05/11/18 15:48, Wei Liu wrote:
> > On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
> >>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
> >>> --- a/xen/arch/x86/x86_64/traps.c
> >>> +++ b/xen/arch/x86/x86_64/traps.c
> >>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
> >>> }
> >>>
> >>> DEFINE_PER_CPU(struct stubs, stubs);
> >>> +
> >>> +#ifdef CONFIG_PV
> >>> void lstar_enter(void);
> >>> void cstar_enter(void);
> >>> +#else
> >>> +static inline void lstar_enter(void)
> >>> +{
> >>> + panic("%s called", __func__);
> >>> +}
> >>> +
> >>> +static inline void cstar_enter(void)
> >>> +{
> >>> + panic("%s called", __func__);
> >>> +}
> >>> +#endif /* CONFIG_PV */
> >> Do we really need two separate stubs (and two separate string literals)
> >> here?
> > I think it is clearer if we have two distinct messages. But I'm not too
> > fussed either way really. If you feel strongly about this, I'm happy to
> > change it to only one function.
>
> This is the correct way to do it. __func__ will already be in the
> string table, and the format string (being identical) will be merged.
I think Jan was complaining two __func__'s (two string literals).
Wei.
>
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV
2018-11-05 15:49 ` Andrew Cooper
2018-11-05 15:51 ` Wei Liu
@ 2018-11-05 16:11 ` Jan Beulich
2018-11-08 15:33 ` Wei Liu
1 sibling, 1 reply; 29+ messages in thread
From: Jan Beulich @ 2018-11-05 16:11 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Kevin Tian, Wei Liu, Jun Nakajima
>>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
> On 05/11/18 15:48, Wei Liu wrote:
>> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
>>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
>>>> --- a/xen/arch/x86/x86_64/traps.c
>>>> +++ b/xen/arch/x86/x86_64/traps.c
>>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
>>>> }
>>>>
>>>> DEFINE_PER_CPU(struct stubs, stubs);
>>>> +
>>>> +#ifdef CONFIG_PV
>>>> void lstar_enter(void);
>>>> void cstar_enter(void);
>>>> +#else
>>>> +static inline void lstar_enter(void)
>>>> +{
>>>> + panic("%s called", __func__);
>>>> +}
>>>> +
>>>> +static inline void cstar_enter(void)
>>>> +{
>>>> + panic("%s called", __func__);
>>>> +}
>>>> +#endif /* CONFIG_PV */
>>> Do we really need two separate stubs (and two separate string literals)
>>> here?
>> I think it is clearer if we have two distinct messages. But I'm not too
>> fussed either way really. If you feel strongly about this, I'm happy to
>> change it to only one function.
>
> This is the correct way to do it. __func__ will already be in the
> string table, and the format string (being identical) will be merged.
Why would __func__ be in the string table already, for functions
containing no other references to it?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV
2018-11-05 16:11 ` Jan Beulich
@ 2018-11-08 15:33 ` Wei Liu
2018-11-08 15:36 ` Andrew Cooper
2018-11-08 15:39 ` Jan Beulich
0 siblings, 2 replies; 29+ messages in thread
From: Wei Liu @ 2018-11-08 15:33 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Kevin Tian, Wei Liu, Jun Nakajima, xen-devel
On Mon, Nov 05, 2018 at 09:11:44AM -0700, Jan Beulich wrote:
> >>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
> > On 05/11/18 15:48, Wei Liu wrote:
> >> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
> >>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
> >>>> --- a/xen/arch/x86/x86_64/traps.c
> >>>> +++ b/xen/arch/x86/x86_64/traps.c
> >>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
> >>>> }
> >>>>
> >>>> DEFINE_PER_CPU(struct stubs, stubs);
> >>>> +
> >>>> +#ifdef CONFIG_PV
> >>>> void lstar_enter(void);
> >>>> void cstar_enter(void);
> >>>> +#else
> >>>> +static inline void lstar_enter(void)
> >>>> +{
> >>>> + panic("%s called", __func__);
> >>>> +}
> >>>> +
> >>>> +static inline void cstar_enter(void)
> >>>> +{
> >>>> + panic("%s called", __func__);
> >>>> +}
> >>>> +#endif /* CONFIG_PV */
> >>> Do we really need two separate stubs (and two separate string literals)
> >>> here?
> >> I think it is clearer if we have two distinct messages. But I'm not too
> >> fussed either way really. If you feel strongly about this, I'm happy to
> >> change it to only one function.
> >
> > This is the correct way to do it. __func__ will already be in the
> > string table, and the format string (being identical) will be merged.
>
> Why would __func__ be in the string table already, for functions
> containing no other references to it?
What is the way forward? Do we really care if there is one more string
literal in the binary?
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV
2018-11-08 15:33 ` Wei Liu
@ 2018-11-08 15:36 ` Andrew Cooper
2018-11-08 15:49 ` Jan Beulich
2018-11-08 15:39 ` Jan Beulich
1 sibling, 1 reply; 29+ messages in thread
From: Andrew Cooper @ 2018-11-08 15:36 UTC (permalink / raw)
To: Wei Liu, Jan Beulich; +Cc: xen-devel, Kevin Tian, Jun Nakajima
On 08/11/18 15:33, Wei Liu wrote:
> On Mon, Nov 05, 2018 at 09:11:44AM -0700, Jan Beulich wrote:
>>>>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
>>> On 05/11/18 15:48, Wei Liu wrote:
>>>> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
>>>>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
>>>>>> --- a/xen/arch/x86/x86_64/traps.c
>>>>>> +++ b/xen/arch/x86/x86_64/traps.c
>>>>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
>>>>>> }
>>>>>>
>>>>>> DEFINE_PER_CPU(struct stubs, stubs);
>>>>>> +
>>>>>> +#ifdef CONFIG_PV
>>>>>> void lstar_enter(void);
>>>>>> void cstar_enter(void);
>>>>>> +#else
>>>>>> +static inline void lstar_enter(void)
>>>>>> +{
>>>>>> + panic("%s called", __func__);
>>>>>> +}
>>>>>> +
>>>>>> +static inline void cstar_enter(void)
>>>>>> +{
>>>>>> + panic("%s called", __func__);
>>>>>> +}
>>>>>> +#endif /* CONFIG_PV */
>>>>> Do we really need two separate stubs (and two separate string literals)
>>>>> here?
>>>> I think it is clearer if we have two distinct messages. But I'm not too
>>>> fussed either way really. If you feel strongly about this, I'm happy to
>>>> change it to only one function.
>>> This is the correct way to do it. __func__ will already be in the
>>> string table, and the format string (being identical) will be merged.
>> Why would __func__ be in the string table already, for functions
>> containing no other references to it?
> What is the way forward? Do we really care if there is one more string
> literal in the binary?
No. One extra string like this is not something which needs caring
about in the slightest.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV
2018-11-08 15:36 ` Andrew Cooper
@ 2018-11-08 15:49 ` Jan Beulich
2018-11-08 16:09 ` Wei Liu
0 siblings, 1 reply; 29+ messages in thread
From: Jan Beulich @ 2018-11-08 15:49 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Kevin Tian, Wei Liu, Jun Nakajima
>>> On 08.11.18 at 16:36, <andrew.cooper3@citrix.com> wrote:
> On 08/11/18 15:33, Wei Liu wrote:
>> On Mon, Nov 05, 2018 at 09:11:44AM -0700, Jan Beulich wrote:
>>>>>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
>>>> On 05/11/18 15:48, Wei Liu wrote:
>>>>> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
>>>>>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
>>>>>>> --- a/xen/arch/x86/x86_64/traps.c
>>>>>>> +++ b/xen/arch/x86/x86_64/traps.c
>>>>>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
>>>>>>> }
>>>>>>>
>>>>>>> DEFINE_PER_CPU(struct stubs, stubs);
>>>>>>> +
>>>>>>> +#ifdef CONFIG_PV
>>>>>>> void lstar_enter(void);
>>>>>>> void cstar_enter(void);
>>>>>>> +#else
>>>>>>> +static inline void lstar_enter(void)
>>>>>>> +{
>>>>>>> + panic("%s called", __func__);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static inline void cstar_enter(void)
>>>>>>> +{
>>>>>>> + panic("%s called", __func__);
>>>>>>> +}
>>>>>>> +#endif /* CONFIG_PV */
>>>>>> Do we really need two separate stubs (and two separate string literals)
>>>>>> here?
>>>>> I think it is clearer if we have two distinct messages. But I'm not too
>>>>> fussed either way really. If you feel strongly about this, I'm happy to
>>>>> change it to only one function.
>>>> This is the correct way to do it. __func__ will already be in the
>>>> string table, and the format string (being identical) will be merged.
>>> Why would __func__ be in the string table already, for functions
>>> containing no other references to it?
>> What is the way forward? Do we really care if there is one more string
>> literal in the binary?
>
> No. One extra string like this is not something which needs caring
> about in the slightest.
So just to clarify - the unnecessary redundancy in the source is of more
relevance to me than the resulting slightly larger binary.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV
2018-11-08 15:49 ` Jan Beulich
@ 2018-11-08 16:09 ` Wei Liu
2018-11-08 16:22 ` Jan Beulich
0 siblings, 1 reply; 29+ messages in thread
From: Wei Liu @ 2018-11-08 16:09 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Kevin Tian, Wei Liu, Jun Nakajima, xen-devel
On Thu, Nov 08, 2018 at 08:49:44AM -0700, Jan Beulich wrote:
> >>> On 08.11.18 at 16:36, <andrew.cooper3@citrix.com> wrote:
> > On 08/11/18 15:33, Wei Liu wrote:
> >> On Mon, Nov 05, 2018 at 09:11:44AM -0700, Jan Beulich wrote:
> >>>>>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
> >>>> On 05/11/18 15:48, Wei Liu wrote:
> >>>>> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
> >>>>>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
> >>>>>>> --- a/xen/arch/x86/x86_64/traps.c
> >>>>>>> +++ b/xen/arch/x86/x86_64/traps.c
> >>>>>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
> >>>>>>> }
> >>>>>>>
> >>>>>>> DEFINE_PER_CPU(struct stubs, stubs);
> >>>>>>> +
> >>>>>>> +#ifdef CONFIG_PV
> >>>>>>> void lstar_enter(void);
> >>>>>>> void cstar_enter(void);
> >>>>>>> +#else
> >>>>>>> +static inline void lstar_enter(void)
> >>>>>>> +{
> >>>>>>> + panic("%s called", __func__);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static inline void cstar_enter(void)
> >>>>>>> +{
> >>>>>>> + panic("%s called", __func__);
> >>>>>>> +}
> >>>>>>> +#endif /* CONFIG_PV */
> >>>>>> Do we really need two separate stubs (and two separate string literals)
> >>>>>> here?
> >>>>> I think it is clearer if we have two distinct messages. But I'm not too
> >>>>> fussed either way really. If you feel strongly about this, I'm happy to
> >>>>> change it to only one function.
> >>>> This is the correct way to do it. __func__ will already be in the
> >>>> string table, and the format string (being identical) will be merged.
> >>> Why would __func__ be in the string table already, for functions
> >>> containing no other references to it?
> >> What is the way forward? Do we really care if there is one more string
> >> literal in the binary?
> >
> > No. One extra string like this is not something which needs caring
> > about in the slightest.
>
> So just to clarify - the unnecessary redundancy in the source is of more
> relevance to me than the resulting slightly larger binary.
So you would rather have
static inline void lcstar_enter(void)
{
panic("lstar/cstar\n");
}
?
TBH I think this is not as good as using two functions. I'm up for
sending a followup patch to do whatever it is agreed upon, but for now
I'm intending to commit this patch as-is.
Wei.
>
> Jan
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV
2018-11-08 16:09 ` Wei Liu
@ 2018-11-08 16:22 ` Jan Beulich
2018-11-08 16:42 ` Wei Liu
0 siblings, 1 reply; 29+ messages in thread
From: Jan Beulich @ 2018-11-08 16:22 UTC (permalink / raw)
To: Wei Liu; +Cc: Andrew Cooper, Kevin Tian, Jun Nakajima, xen-devel
>>> On 08.11.18 at 17:09, <wei.liu2@citrix.com> wrote:
> On Thu, Nov 08, 2018 at 08:49:44AM -0700, Jan Beulich wrote:
>> >>> On 08.11.18 at 16:36, <andrew.cooper3@citrix.com> wrote:
>> > On 08/11/18 15:33, Wei Liu wrote:
>> >> On Mon, Nov 05, 2018 at 09:11:44AM -0700, Jan Beulich wrote:
>> >>>>>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
>> >>>> On 05/11/18 15:48, Wei Liu wrote:
>> >>>>> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
>> >>>>>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
>> >>>>>>> --- a/xen/arch/x86/x86_64/traps.c
>> >>>>>>> +++ b/xen/arch/x86/x86_64/traps.c
>> >>>>>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
>> >>>>>>> }
>> >>>>>>>
>> >>>>>>> DEFINE_PER_CPU(struct stubs, stubs);
>> >>>>>>> +
>> >>>>>>> +#ifdef CONFIG_PV
>> >>>>>>> void lstar_enter(void);
>> >>>>>>> void cstar_enter(void);
>> >>>>>>> +#else
>> >>>>>>> +static inline void lstar_enter(void)
>> >>>>>>> +{
>> >>>>>>> + panic("%s called", __func__);
>> >>>>>>> +}
>> >>>>>>> +
>> >>>>>>> +static inline void cstar_enter(void)
>> >>>>>>> +{
>> >>>>>>> + panic("%s called", __func__);
>> >>>>>>> +}
>> >>>>>>> +#endif /* CONFIG_PV */
>> >>>>>> Do we really need two separate stubs (and two separate string literals)
>> >>>>>> here?
>> >>>>> I think it is clearer if we have two distinct messages. But I'm not too
>> >>>>> fussed either way really. If you feel strongly about this, I'm happy to
>> >>>>> change it to only one function.
>> >>>> This is the correct way to do it. __func__ will already be in the
>> >>>> string table, and the format string (being identical) will be merged.
>> >>> Why would __func__ be in the string table already, for functions
>> >>> containing no other references to it?
>> >> What is the way forward? Do we really care if there is one more string
>> >> literal in the binary?
>> >
>> > No. One extra string like this is not something which needs caring
>> > about in the slightest.
>>
>> So just to clarify - the unnecessary redundancy in the source is of more
>> relevance to me than the resulting slightly larger binary.
>
> So you would rather have
>
> static inline void lcstar_enter(void)
> {
> panic("lstar/cstar\n");
> }
>
> ?
Yes. Let's not forget that this function is never to be invoked
anyway, so the less dead code and data, the better. (As a
side note, I don't understand why they're inline when you need
a pointer to them anyway. Instead they should go into
.text.cold or whatever it is.)
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV
2018-11-08 16:22 ` Jan Beulich
@ 2018-11-08 16:42 ` Wei Liu
0 siblings, 0 replies; 29+ messages in thread
From: Wei Liu @ 2018-11-08 16:42 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Kevin Tian, Wei Liu, Jun Nakajima, xen-devel
On Thu, Nov 08, 2018 at 09:22:08AM -0700, Jan Beulich wrote:
> >>> On 08.11.18 at 17:09, <wei.liu2@citrix.com> wrote:
> > On Thu, Nov 08, 2018 at 08:49:44AM -0700, Jan Beulich wrote:
> >> >>> On 08.11.18 at 16:36, <andrew.cooper3@citrix.com> wrote:
> >> > On 08/11/18 15:33, Wei Liu wrote:
> >> >> On Mon, Nov 05, 2018 at 09:11:44AM -0700, Jan Beulich wrote:
> >> >>>>>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
> >> >>>> On 05/11/18 15:48, Wei Liu wrote:
> >> >>>>> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
> >> >>>>>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
> >> >>>>>>> --- a/xen/arch/x86/x86_64/traps.c
> >> >>>>>>> +++ b/xen/arch/x86/x86_64/traps.c
> >> >>>>>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
> >> >>>>>>> }
> >> >>>>>>>
> >> >>>>>>> DEFINE_PER_CPU(struct stubs, stubs);
> >> >>>>>>> +
> >> >>>>>>> +#ifdef CONFIG_PV
> >> >>>>>>> void lstar_enter(void);
> >> >>>>>>> void cstar_enter(void);
> >> >>>>>>> +#else
> >> >>>>>>> +static inline void lstar_enter(void)
> >> >>>>>>> +{
> >> >>>>>>> + panic("%s called", __func__);
> >> >>>>>>> +}
> >> >>>>>>> +
> >> >>>>>>> +static inline void cstar_enter(void)
> >> >>>>>>> +{
> >> >>>>>>> + panic("%s called", __func__);
> >> >>>>>>> +}
> >> >>>>>>> +#endif /* CONFIG_PV */
> >> >>>>>> Do we really need two separate stubs (and two separate string literals)
> >> >>>>>> here?
> >> >>>>> I think it is clearer if we have two distinct messages. But I'm not too
> >> >>>>> fussed either way really. If you feel strongly about this, I'm happy to
> >> >>>>> change it to only one function.
> >> >>>> This is the correct way to do it. __func__ will already be in the
> >> >>>> string table, and the format string (being identical) will be merged.
> >> >>> Why would __func__ be in the string table already, for functions
> >> >>> containing no other references to it?
> >> >> What is the way forward? Do we really care if there is one more string
> >> >> literal in the binary?
> >> >
> >> > No. One extra string like this is not something which needs caring
> >> > about in the slightest.
> >>
> >> So just to clarify - the unnecessary redundancy in the source is of more
> >> relevance to me than the resulting slightly larger binary.
> >
> > So you would rather have
> >
> > static inline void lcstar_enter(void)
> > {
> > panic("lstar/cstar\n");
> > }
> >
> > ?
>
> Yes. Let's not forget that this function is never to be invoked
> anyway, so the less dead code and data, the better. (As a
> side note, I don't understand why they're inline when you need
> a pointer to them anyway. Instead they should go into
> .text.cold or whatever it is.)
OK. I will submit a followup patch for that.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV
2018-11-08 15:33 ` Wei Liu
2018-11-08 15:36 ` Andrew Cooper
@ 2018-11-08 15:39 ` Jan Beulich
2018-11-08 15:49 ` Wei Liu
1 sibling, 1 reply; 29+ messages in thread
From: Jan Beulich @ 2018-11-08 15:39 UTC (permalink / raw)
To: Wei Liu; +Cc: Andrew Cooper, Kevin Tian, Jun Nakajima, xen-devel
>>> On 08.11.18 at 16:33, <wei.liu2@citrix.com> wrote:
> On Mon, Nov 05, 2018 at 09:11:44AM -0700, Jan Beulich wrote:
>> >>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
>> > On 05/11/18 15:48, Wei Liu wrote:
>> >> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
>> >>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
>> >>>> --- a/xen/arch/x86/x86_64/traps.c
>> >>>> +++ b/xen/arch/x86/x86_64/traps.c
>> >>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
>> >>>> }
>> >>>>
>> >>>> DEFINE_PER_CPU(struct stubs, stubs);
>> >>>> +
>> >>>> +#ifdef CONFIG_PV
>> >>>> void lstar_enter(void);
>> >>>> void cstar_enter(void);
>> >>>> +#else
>> >>>> +static inline void lstar_enter(void)
>> >>>> +{
>> >>>> + panic("%s called", __func__);
>> >>>> +}
>> >>>> +
>> >>>> +static inline void cstar_enter(void)
>> >>>> +{
>> >>>> + panic("%s called", __func__);
>> >>>> +}
>> >>>> +#endif /* CONFIG_PV */
>> >>> Do we really need two separate stubs (and two separate string literals)
>> >>> here?
>> >> I think it is clearer if we have two distinct messages. But I'm not too
>> >> fussed either way really. If you feel strongly about this, I'm happy to
>> >> change it to only one function.
>> >
>> > This is the correct way to do it. __func__ will already be in the
>> > string table, and the format string (being identical) will be merged.
>>
>> Why would __func__ be in the string table already, for functions
>> containing no other references to it?
>
> What is the way forward? Do we really care if there is one more string
> literal in the binary?
Well, if I understood what Andrew means, I could decide whether to
give up my objection. Yes, a single string literal and a few insn bytes
don't matter all that much. But nowadays everyone thinks this way
everywhere, and one has to load dozens if not hundreds of megabytes
of data just to get a basic OS booted.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV
2018-11-08 15:39 ` Jan Beulich
@ 2018-11-08 15:49 ` Wei Liu
0 siblings, 0 replies; 29+ messages in thread
From: Wei Liu @ 2018-11-08 15:49 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Kevin Tian, Wei Liu, Jun Nakajima, xen-devel
On Thu, Nov 08, 2018 at 08:39:36AM -0700, Jan Beulich wrote:
> >>> On 08.11.18 at 16:33, <wei.liu2@citrix.com> wrote:
> > On Mon, Nov 05, 2018 at 09:11:44AM -0700, Jan Beulich wrote:
> >> >>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
> >> > On 05/11/18 15:48, Wei Liu wrote:
> >> >> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
> >> >>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
> >> >>>> --- a/xen/arch/x86/x86_64/traps.c
> >> >>>> +++ b/xen/arch/x86/x86_64/traps.c
> >> >>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
> >> >>>> }
> >> >>>>
> >> >>>> DEFINE_PER_CPU(struct stubs, stubs);
> >> >>>> +
> >> >>>> +#ifdef CONFIG_PV
> >> >>>> void lstar_enter(void);
> >> >>>> void cstar_enter(void);
> >> >>>> +#else
> >> >>>> +static inline void lstar_enter(void)
> >> >>>> +{
> >> >>>> + panic("%s called", __func__);
> >> >>>> +}
> >> >>>> +
> >> >>>> +static inline void cstar_enter(void)
> >> >>>> +{
> >> >>>> + panic("%s called", __func__);
> >> >>>> +}
> >> >>>> +#endif /* CONFIG_PV */
> >> >>> Do we really need two separate stubs (and two separate string literals)
> >> >>> here?
> >> >> I think it is clearer if we have two distinct messages. But I'm not too
> >> >> fussed either way really. If you feel strongly about this, I'm happy to
> >> >> change it to only one function.
> >> >
> >> > This is the correct way to do it. __func__ will already be in the
> >> > string table, and the format string (being identical) will be merged.
> >>
> >> Why would __func__ be in the string table already, for functions
> >> containing no other references to it?
> >
> > What is the way forward? Do we really care if there is one more string
> > literal in the binary?
>
> Well, if I understood what Andrew means, I could decide whether to
> give up my objection. Yes, a single string literal and a few insn bytes
> don't matter all that much. But nowadays everyone thinks this way
> everywhere, and one has to load dozens if not hundreds of megabytes
> of data just to get a basic OS booted.
Can we please do this sort of things systematically? Like, have a list
of criteria that state which when and where should we pay attention for
adding string literals. Arguing over this on a case by case basis is not
very productive.
BTW I don't think Xen is too bad in that regard -- it certainly doesn't
need hundreds of megabytes of data to boot.
Wei.
>
> Jan
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 4/7] x86: rearrange x86_64/entry.S
2018-11-02 15:55 [PATCH v3 0/7] Make CONFIG_PV work on x86 Wei Liu
` (2 preceding siblings ...)
2018-11-02 15:55 ` [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV Wei Liu
@ 2018-11-02 15:55 ` Wei Liu
2018-11-05 14:20 ` Andrew Cooper
2018-11-02 15:55 ` [PATCH v3 5/7] x86: make entry point code build when !CONFIG_PV Wei Liu
` (2 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Wei Liu @ 2018-11-02 15:55 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
Split the file into two halves. The first half pertains to PV guest
code while the second half is mostly used by the hypervisor itself to
handle interrupts and exceptions.
No functional change intended.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v3: remove self_ipi_restore_all_guest
v2: new, requested by Andrew
---
xen/arch/x86/x86_64/entry.S | 65 ++++++++++++++++++++------------------
1 file changed, 35 insertions(+), 30 deletions(-)
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 48cb96c..9b02899 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -121,16 +121,6 @@ process_trap:
call create_bounce_frame
jmp test_all_events
-/* No special register assumptions. */
-ENTRY(ret_from_intr)
- GET_CURRENT(bx)
- testb $3, UREGS_cs(%rsp)
- jz restore_all_xen
- movq VCPU_domain(%rbx), %rax
- cmpb $0, DOMAIN_is_32bit_pv(%rax)
- je test_all_events
- jmp compat_test_all_events
-
.section .text.entry, "ax", @progbits
/* %rbx: struct vcpu, interrupts disabled */
@@ -211,26 +201,6 @@ iret_exit_to_guest:
.Lft0: iretq
_ASM_PRE_EXTABLE(.Lft0, handle_exception)
- ALIGN
-/* No special register assumptions. */
-restore_all_xen:
- /*
- * Check whether we need to switch to the per-CPU page tables, in
- * case we return to late PV exit code (from an NMI or #MC).
- */
- GET_STACK_END(bx)
- cmpb $0, STACK_CPUINFO_FIELD(use_pv_cr3)(%rbx)
-UNLIKELY_START(ne, exit_cr3)
- mov STACK_CPUINFO_FIELD(pv_cr3)(%rbx), %rax
- mov %rax, %cr3
-UNLIKELY_END(exit_cr3)
-
- /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
- SPEC_CTRL_EXIT_TO_XEN_IST /* Req: %rbx=end, Clob: acd */
-
- RESTORE_ALL adj=8
- iretq
-
/*
* When entering SYSCALL from kernel mode:
* %rax = hypercall vector
@@ -553,8 +523,43 @@ ENTRY(dom_crash_sync_extable)
jmp asm_domain_crash_synchronous /* Does not return */
.popsection
+/* --- CODE BELOW THIS LINE (MOSTLY) NOT GUEST RELATED --- */
+
+ .text
+
+ ALIGN
+/* No special register assumptions. */
+ENTRY(ret_from_intr)
+ GET_CURRENT(bx)
+ testb $3, UREGS_cs(%rsp)
+ jz restore_all_xen
+ movq VCPU_domain(%rbx), %rax
+ cmpb $0, DOMAIN_is_32bit_pv(%rax)
+ je test_all_events
+ jmp compat_test_all_events
+
.section .text.entry, "ax", @progbits
+ ALIGN
+/* No special register assumptions. */
+restore_all_xen:
+ /*
+ * Check whether we need to switch to the per-CPU page tables, in
+ * case we return to late PV exit code (from an NMI or #MC).
+ */
+ GET_STACK_END(bx)
+ cmpb $0, STACK_CPUINFO_FIELD(use_pv_cr3)(%rbx)
+UNLIKELY_START(ne, exit_cr3)
+ mov STACK_CPUINFO_FIELD(pv_cr3)(%rbx), %rax
+ mov %rax, %cr3
+UNLIKELY_END(exit_cr3)
+
+ /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
+ SPEC_CTRL_EXIT_TO_XEN_IST /* Req: %rbx=end, Clob: acd */
+
+ RESTORE_ALL adj=8
+ iretq
+
ENTRY(common_interrupt)
SAVE_ALL CLAC
--
git-series 0.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 4/7] x86: rearrange x86_64/entry.S
2018-11-02 15:55 ` [PATCH v3 4/7] x86: rearrange x86_64/entry.S Wei Liu
@ 2018-11-05 14:20 ` Andrew Cooper
0 siblings, 0 replies; 29+ messages in thread
From: Andrew Cooper @ 2018-11-05 14:20 UTC (permalink / raw)
To: Wei Liu, xen-devel; +Cc: Jan Beulich
On 02/11/18 15:55, Wei Liu wrote:
> @@ -553,8 +523,43 @@ ENTRY(dom_crash_sync_extable)
> jmp asm_domain_crash_synchronous /* Does not return */
> .popsection
>
> +/* --- CODE BELOW THIS LINE (MOSTLY) NOT GUEST RELATED --- */
> +
> + .text
> +
> + ALIGN
No need for an ALIGN here. There is one inside ENTRY().
Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> +/* No special register assumptions. */
> +ENTRY(ret_from_intr)
> + GET_CURRENT(bx)
> + testb $3, UREGS_cs(%rsp)
> + jz restore_all_xen
> + movq VCPU_domain(%rbx), %rax
> + cmpb $0, DOMAIN_is_32bit_pv(%rax)
> + je test_all_events
> + jmp compat_test_all_events
> +
> .section .text.entry, "ax", @progbits
>
> + ALIGN
> +/* No special register assumptions. */
> +restore_all_xen:
> + /*
> + * Check whether we need to switch to the per-CPU page tables, in
> + * case we return to late PV exit code (from an NMI or #MC).
> + */
> + GET_STACK_END(bx)
> + cmpb $0, STACK_CPUINFO_FIELD(use_pv_cr3)(%rbx)
> +UNLIKELY_START(ne, exit_cr3)
> + mov STACK_CPUINFO_FIELD(pv_cr3)(%rbx), %rax
> + mov %rax, %cr3
> +UNLIKELY_END(exit_cr3)
> +
> + /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
> + SPEC_CTRL_EXIT_TO_XEN_IST /* Req: %rbx=end, Clob: acd */
> +
> + RESTORE_ALL adj=8
> + iretq
> +
> ENTRY(common_interrupt)
> SAVE_ALL CLAC
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 5/7] x86: make entry point code build when !CONFIG_PV
2018-11-02 15:55 [PATCH v3 0/7] Make CONFIG_PV work on x86 Wei Liu
` (3 preceding siblings ...)
2018-11-02 15:55 ` [PATCH v3 4/7] x86: rearrange x86_64/entry.S Wei Liu
@ 2018-11-02 15:55 ` Wei Liu
2018-11-05 14:33 ` Andrew Cooper
2018-11-02 15:55 ` [PATCH v3 6/7] x86: expose CONFIG_PV Wei Liu
2018-11-02 15:55 ` [PATCH v3 7/7] x86: update help string for CONFIG_HVM Wei Liu
6 siblings, 1 reply; 29+ messages in thread
From: Wei Liu @ 2018-11-02 15:55 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
Skip building x86_64/compat/entry.S and put CONFIG_PV in
x86_64/entry.S.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v3:
1. make CR4_PV32_RESTORE expand to nothing when !PV
2. use unconditional jmp and add assertions
v2: new
---
xen/arch/x86/x86_64/Makefile | 2 +-
xen/arch/x86/x86_64/entry.S | 39 +++++++++++++++++++++++++++++++++-
xen/include/asm-x86/asm_defns.h | 4 +++-
3 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/x86_64/Makefile b/xen/arch/x86/x86_64/Makefile
index f336a6a..4bfa148 100644
--- a/xen/arch/x86/x86_64/Makefile
+++ b/xen/arch/x86/x86_64/Makefile
@@ -1,4 +1,4 @@
-subdir-y += compat
+subdir-$(CONFIG_PV) += compat
obj-bin-y += entry.o
obj-y += traps.o
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 9b02899..975ed6a 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -15,6 +15,18 @@
#include <public/xen.h>
#include <irq_vectors.h>
+#ifndef NDEBUG
+/* %rsp: struct cpu_user_regs */
+#define ASSERT_INTERRUPTED_XEN_CONTEXT \
+ testb $3, UREGS_cs(%rsp); \
+ jz 1f; \
+ ASSERT_FAILED("INTERRUPTED XEN CONTEXT"); \
+1:
+#else
+#define ASSERT_INTERRUPTED_XEN_CONTEXT
+#endif
+
+#ifdef CONFIG_PV
/* %rbx: struct vcpu */
ENTRY(switch_to_kernel)
leaq VCPU_trap_bounce(%rbx),%rdx
@@ -522,6 +534,7 @@ ENTRY(dom_crash_sync_extable)
xorl %edi,%edi
jmp asm_domain_crash_synchronous /* Does not return */
.popsection
+#endif /* CONFIG_PV */
/* --- CODE BELOW THIS LINE (MOSTLY) NOT GUEST RELATED --- */
@@ -530,6 +543,7 @@ ENTRY(dom_crash_sync_extable)
ALIGN
/* No special register assumptions. */
ENTRY(ret_from_intr)
+#ifdef CONFIG_PV
GET_CURRENT(bx)
testb $3, UREGS_cs(%rsp)
jz restore_all_xen
@@ -537,6 +551,10 @@ ENTRY(ret_from_intr)
cmpb $0, DOMAIN_is_32bit_pv(%rax)
je test_all_events
jmp compat_test_all_events
+#else
+ ASSERT_INTERRUPTED_XEN_CONTEXT
+ jmp restore_all_xen
+#endif
.section .text.entry, "ax", @progbits
@@ -619,6 +637,7 @@ handle_exception_saved:
testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
jz exception_with_ints_disabled
+#ifdef CONFIG_PV
ALTERNATIVE_2 "jmp .Lcr4_pv32_done", \
__stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMEP, \
__stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMAP
@@ -658,6 +677,9 @@ handle_exception_saved:
test $~(PFEC_write_access|PFEC_insn_fetch),%eax
jz compat_test_all_events
.Lcr4_pv32_done:
+#else
+ ASSERT_INTERRUPTED_XEN_CONTEXT
+#endif /* CONFIG_PV */
sti
1: movq %rsp,%rdi
movzbl UREGS_entry_vector(%rsp),%eax
@@ -667,12 +689,17 @@ handle_exception_saved:
INDIRECT_CALL %rdx
mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
mov %r13b, STACK_CPUINFO_FIELD(use_pv_cr3)(%r14)
+#ifdef CONFIG_PV
testb $3,UREGS_cs(%rsp)
jz restore_all_xen
movq VCPU_domain(%rbx),%rax
cmpb $0, DOMAIN_is_32bit_pv(%rax)
jne compat_test_all_events
jmp test_all_events
+#else
+ ASSERT_INTERRUPTED_XEN_CONTEXT
+ jmp restore_all_xen
+#endif
/* No special register assumptions. */
exception_with_ints_disabled:
@@ -823,6 +850,7 @@ handle_ist_exception:
mov %r12, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
.List_cr3_okay:
+#ifdef CONFIG_PV
CR4_PV32_RESTORE
testb $3,UREGS_cs(%rsp)
jz 1f
@@ -837,7 +865,11 @@ handle_ist_exception:
movl $UREGS_kernel_sizeof/8,%ecx
movq %rdi,%rsp
rep movsq
-1: movq %rsp,%rdi
+1:
+#else
+ ASSERT_INTERRUPTED_XEN_CONTEXT
+#endif
+ movq %rsp,%rdi
movzbl UREGS_entry_vector(%rsp),%eax
leaq exception_table(%rip),%rdx
mov (%rdx, %rax, 8), %rdx
@@ -848,6 +880,7 @@ handle_ist_exception:
jne ret_from_intr
/* We want to get straight to the IRET on the NMI exit path. */
+#ifdef CONFIG_PV
testb $3,UREGS_cs(%rsp)
jz restore_all_xen
GET_CURRENT(bx)
@@ -863,6 +896,10 @@ handle_ist_exception:
cmpb $0,DOMAIN_is_32bit_pv(%rax)
je restore_all_guest
jmp compat_restore_all_guest
+#else
+ ASSERT_INTERRUPTED_XEN_CONTEXT
+ jmp restore_all_xen
+#endif
ENTRY(machine_check)
pushq $0
diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index da504ea..e688cf1 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -321,10 +321,14 @@ static always_inline void stac(void)
subq $-(UREGS_error_code-UREGS_r15+\adj), %rsp
.endm
+#ifdef CONFIG_PV
#define CR4_PV32_RESTORE \
ALTERNATIVE_2 "", \
"call cr4_pv32_restore", X86_FEATURE_XEN_SMEP, \
"call cr4_pv32_restore", X86_FEATURE_XEN_SMAP
+#else
+#define CR4_PV32_RESTORE
+#endif
#include <asm/spec_ctrl_asm.h>
--
git-series 0.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 5/7] x86: make entry point code build when !CONFIG_PV
2018-11-02 15:55 ` [PATCH v3 5/7] x86: make entry point code build when !CONFIG_PV Wei Liu
@ 2018-11-05 14:33 ` Andrew Cooper
2018-11-05 17:08 ` Wei Liu
0 siblings, 1 reply; 29+ messages in thread
From: Andrew Cooper @ 2018-11-05 14:33 UTC (permalink / raw)
To: Wei Liu, xen-devel; +Cc: Jan Beulich
On 02/11/18 15:55, Wei Liu wrote:
> Skip building x86_64/compat/entry.S and put CONFIG_PV in
> x86_64/entry.S.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> v3:
> 1. make CR4_PV32_RESTORE expand to nothing when !PV
> 2. use unconditional jmp and add assertions
>
> v2: new
> ---
> xen/arch/x86/x86_64/Makefile | 2 +-
> xen/arch/x86/x86_64/entry.S | 39 +++++++++++++++++++++++++++++++++-
> xen/include/asm-x86/asm_defns.h | 4 +++-
> 3 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/x86_64/Makefile b/xen/arch/x86/x86_64/Makefile
> index f336a6a..4bfa148 100644
> --- a/xen/arch/x86/x86_64/Makefile
> +++ b/xen/arch/x86/x86_64/Makefile
> @@ -1,4 +1,4 @@
> -subdir-y += compat
> +subdir-$(CONFIG_PV) += compat
>
> obj-bin-y += entry.o
> obj-y += traps.o
> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index 9b02899..975ed6a 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -15,6 +15,18 @@
> #include <public/xen.h>
> #include <irq_vectors.h>
>
> +#ifndef NDEBUG
> +/* %rsp: struct cpu_user_regs */
> +#define ASSERT_INTERRUPTED_XEN_CONTEXT \
Could I suggest ASSERT_CONTEXT_IS_XEN which I think fits better in some
of the cases below?
> + testb $3, UREGS_cs(%rsp); \
> + jz 1f; \
> + ASSERT_FAILED("INTERRUPTED XEN CONTEXT"); \
This probably wants to be:
.macro ASSERT_CONTEXT_IS_XEN
#ifndef NDEBUG
testb $3, UREGS_cs(%rsp);
UNLIKELY_START(nz, ASSERT_XEN_\@);
ASSERT_FAILED("CONTEXT IS XEN");
__UNLIKELY_END(ASSERT_XEN_\@);
#endif
.endm
which will assemble in the fastpath to:
testb $3, UREGS_cs(%rsp);
jne <unlikely>
jmp restore_all_xen
And avoid a conditional jump in the expected path. (Code completely
untested.)
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 5/7] x86: make entry point code build when !CONFIG_PV
2018-11-05 14:33 ` Andrew Cooper
@ 2018-11-05 17:08 ` Wei Liu
2018-11-05 17:10 ` Andrew Cooper
0 siblings, 1 reply; 29+ messages in thread
From: Wei Liu @ 2018-11-05 17:08 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Wei Liu, Jan Beulich
On Mon, Nov 05, 2018 at 02:33:23PM +0000, Andrew Cooper wrote:
> On 02/11/18 15:55, Wei Liu wrote:
> > Skip building x86_64/compat/entry.S and put CONFIG_PV in
> > x86_64/entry.S.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > v3:
> > 1. make CR4_PV32_RESTORE expand to nothing when !PV
> > 2. use unconditional jmp and add assertions
> >
> > v2: new
> > ---
> > xen/arch/x86/x86_64/Makefile | 2 +-
> > xen/arch/x86/x86_64/entry.S | 39 +++++++++++++++++++++++++++++++++-
> > xen/include/asm-x86/asm_defns.h | 4 +++-
> > 3 files changed, 43 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/x86/x86_64/Makefile b/xen/arch/x86/x86_64/Makefile
> > index f336a6a..4bfa148 100644
> > --- a/xen/arch/x86/x86_64/Makefile
> > +++ b/xen/arch/x86/x86_64/Makefile
> > @@ -1,4 +1,4 @@
> > -subdir-y += compat
> > +subdir-$(CONFIG_PV) += compat
> >
> > obj-bin-y += entry.o
> > obj-y += traps.o
> > diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> > index 9b02899..975ed6a 100644
> > --- a/xen/arch/x86/x86_64/entry.S
> > +++ b/xen/arch/x86/x86_64/entry.S
> > @@ -15,6 +15,18 @@
> > #include <public/xen.h>
> > #include <irq_vectors.h>
> >
> > +#ifndef NDEBUG
> > +/* %rsp: struct cpu_user_regs */
> > +#define ASSERT_INTERRUPTED_XEN_CONTEXT \
>
> Could I suggest ASSERT_CONTEXT_IS_XEN which I think fits better in some
> of the cases below?
>
> > + testb $3, UREGS_cs(%rsp); \
> > + jz 1f; \
> > + ASSERT_FAILED("INTERRUPTED XEN CONTEXT"); \
>
> This probably wants to be:
>
> .macro ASSERT_CONTEXT_IS_XEN
> #ifndef NDEBUG
> testb $3, UREGS_cs(%rsp);
> UNLIKELY_START(nz, ASSERT_XEN_\@);
> ASSERT_FAILED("CONTEXT IS XEN");
> __UNLIKELY_END(ASSERT_XEN_\@);
> #endif
> .endm
This works. I have deleted all semicolons though.
(To avoid posting other patches)
---8<---
From 8f3ce0b0a5b6a215d4309d77d2c63229fb4af900 Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Fri, 19 Oct 2018 12:32:12 +0100
Subject: [PATCH] x86: make entry point code build when !CONFIG_PV
Skip building x86_64/compat/entry.S and put CONFIG_PV in
x86_64/entry.S.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v4:
1. Change name to ASSERT_CONTEXT_IS_XEN
2. Use UNLIKELY
v3:
1. make CR4_PV32_RESTORE expand to nothing when !PV
2. use unconditional jmp and add assertions
v2: new
---
xen/arch/x86/x86_64/Makefile | 2 +-
xen/arch/x86/x86_64/entry.S | 38 +++++++++++++++++++++++++++++++++++++-
xen/include/asm-x86/asm_defns.h | 4 ++++
3 files changed, 42 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/x86_64/Makefile b/xen/arch/x86/x86_64/Makefile
index f336a6ae65..4bfa1480eb 100644
--- a/xen/arch/x86/x86_64/Makefile
+++ b/xen/arch/x86/x86_64/Makefile
@@ -1,4 +1,4 @@
-subdir-y += compat
+subdir-$(CONFIG_PV) += compat
obj-bin-y += entry.o
obj-y += traps.o
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 8e12decea8..e8eae3b08d 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -15,6 +15,17 @@
#include <public/xen.h>
#include <irq_vectors.h>
+/* %rsp: struct cpu_user_regs */
+.macro ASSERT_CONTEXT_IS_XEN
+#ifndef NDEBUG
+ testb $3, UREGS_cs(%rsp)
+ UNLIKELY_START(nz, ASSERT_XEN_\@)
+ ASSERT_FAILED("INTERRUPTED XEN CONTEXT")
+ __UNLIKELY_END(ASSERT_XEN_\@)
+#endif
+.endm
+
+#ifdef CONFIG_PV
/* %rbx: struct vcpu */
ENTRY(switch_to_kernel)
leaq VCPU_trap_bounce(%rbx),%rdx
@@ -522,6 +533,7 @@ ENTRY(dom_crash_sync_extable)
xorl %edi,%edi
jmp asm_domain_crash_synchronous /* Does not return */
.popsection
+#endif /* CONFIG_PV */
/* --- CODE BELOW THIS LINE (MOSTLY) NOT GUEST RELATED --- */
@@ -529,6 +541,7 @@ ENTRY(dom_crash_sync_extable)
/* No special register assumptions. */
ENTRY(ret_from_intr)
+#ifdef CONFIG_PV
GET_CURRENT(bx)
testb $3, UREGS_cs(%rsp)
jz restore_all_xen
@@ -536,6 +549,10 @@ ENTRY(ret_from_intr)
cmpb $0, DOMAIN_is_32bit_pv(%rax)
je test_all_events
jmp compat_test_all_events
+#else
+ ASSERT_CONTEXT_IS_XEN
+ jmp restore_all_xen
+#endif
.section .text.entry, "ax", @progbits
@@ -618,6 +635,7 @@ handle_exception_saved:
testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
jz exception_with_ints_disabled
+#ifdef CONFIG_PV
ALTERNATIVE_2 "jmp .Lcr4_pv32_done", \
__stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMEP, \
__stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMAP
@@ -657,6 +675,9 @@ handle_exception_saved:
test $~(PFEC_write_access|PFEC_insn_fetch),%eax
jz compat_test_all_events
.Lcr4_pv32_done:
+#else
+ ASSERT_CONTEXT_IS_XEN
+#endif /* CONFIG_PV */
sti
1: movq %rsp,%rdi
movzbl UREGS_entry_vector(%rsp),%eax
@@ -666,12 +687,17 @@ handle_exception_saved:
INDIRECT_CALL %rdx
mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
mov %r13b, STACK_CPUINFO_FIELD(use_pv_cr3)(%r14)
+#ifdef CONFIG_PV
testb $3,UREGS_cs(%rsp)
jz restore_all_xen
movq VCPU_domain(%rbx),%rax
cmpb $0, DOMAIN_is_32bit_pv(%rax)
jne compat_test_all_events
jmp test_all_events
+#else
+ ASSERT_CONTEXT_IS_XEN
+ jmp restore_all_xen
+#endif
/* No special register assumptions. */
exception_with_ints_disabled:
@@ -822,6 +848,7 @@ handle_ist_exception:
mov %r12, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
.List_cr3_okay:
+#ifdef CONFIG_PV
CR4_PV32_RESTORE
testb $3,UREGS_cs(%rsp)
jz 1f
@@ -836,7 +863,11 @@ handle_ist_exception:
movl $UREGS_kernel_sizeof/8,%ecx
movq %rdi,%rsp
rep movsq
-1: movq %rsp,%rdi
+1:
+#else
+ ASSERT_CONTEXT_IS_XEN
+#endif
+ movq %rsp,%rdi
movzbl UREGS_entry_vector(%rsp),%eax
leaq exception_table(%rip),%rdx
mov (%rdx, %rax, 8), %rdx
@@ -847,6 +878,7 @@ handle_ist_exception:
jne ret_from_intr
/* We want to get straight to the IRET on the NMI exit path. */
+#ifdef CONFIG_PV
testb $3,UREGS_cs(%rsp)
jz restore_all_xen
GET_CURRENT(bx)
@@ -862,6 +894,10 @@ handle_ist_exception:
cmpb $0,DOMAIN_is_32bit_pv(%rax)
je restore_all_guest
jmp compat_restore_all_guest
+#else
+ ASSERT_CONTEXT_IS_XEN
+ jmp restore_all_xen
+#endif
ENTRY(machine_check)
pushq $0
diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index da504eaec2..e688cf1c16 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -321,10 +321,14 @@ static always_inline void stac(void)
subq $-(UREGS_error_code-UREGS_r15+\adj), %rsp
.endm
+#ifdef CONFIG_PV
#define CR4_PV32_RESTORE \
ALTERNATIVE_2 "", \
"call cr4_pv32_restore", X86_FEATURE_XEN_SMEP, \
"call cr4_pv32_restore", X86_FEATURE_XEN_SMAP
+#else
+#define CR4_PV32_RESTORE
+#endif
#include <asm/spec_ctrl_asm.h>
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 5/7] x86: make entry point code build when !CONFIG_PV
2018-11-05 17:08 ` Wei Liu
@ 2018-11-05 17:10 ` Andrew Cooper
0 siblings, 0 replies; 29+ messages in thread
From: Andrew Cooper @ 2018-11-05 17:10 UTC (permalink / raw)
To: Wei Liu; +Cc: xen-devel, Jan Beulich
On 05/11/18 17:08, Wei Liu wrote:
> On Mon, Nov 05, 2018 at 02:33:23PM +0000, Andrew Cooper wrote:
>> On 02/11/18 15:55, Wei Liu wrote:
>>> Skip building x86_64/compat/entry.S and put CONFIG_PV in
>>> x86_64/entry.S.
>>>
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>> ---
>>> v3:
>>> 1. make CR4_PV32_RESTORE expand to nothing when !PV
>>> 2. use unconditional jmp and add assertions
>>>
>>> v2: new
>>> ---
>>> xen/arch/x86/x86_64/Makefile | 2 +-
>>> xen/arch/x86/x86_64/entry.S | 39 +++++++++++++++++++++++++++++++++-
>>> xen/include/asm-x86/asm_defns.h | 4 +++-
>>> 3 files changed, 43 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/x86_64/Makefile b/xen/arch/x86/x86_64/Makefile
>>> index f336a6a..4bfa148 100644
>>> --- a/xen/arch/x86/x86_64/Makefile
>>> +++ b/xen/arch/x86/x86_64/Makefile
>>> @@ -1,4 +1,4 @@
>>> -subdir-y += compat
>>> +subdir-$(CONFIG_PV) += compat
>>>
>>> obj-bin-y += entry.o
>>> obj-y += traps.o
>>> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
>>> index 9b02899..975ed6a 100644
>>> --- a/xen/arch/x86/x86_64/entry.S
>>> +++ b/xen/arch/x86/x86_64/entry.S
>>> @@ -15,6 +15,18 @@
>>> #include <public/xen.h>
>>> #include <irq_vectors.h>
>>>
>>> +#ifndef NDEBUG
>>> +/* %rsp: struct cpu_user_regs */
>>> +#define ASSERT_INTERRUPTED_XEN_CONTEXT \
>> Could I suggest ASSERT_CONTEXT_IS_XEN which I think fits better in some
>> of the cases below?
>>
>>> + testb $3, UREGS_cs(%rsp); \
>>> + jz 1f; \
>>> + ASSERT_FAILED("INTERRUPTED XEN CONTEXT"); \
>> This probably wants to be:
>>
>> .macro ASSERT_CONTEXT_IS_XEN
>> #ifndef NDEBUG
>> testb $3, UREGS_cs(%rsp);
>> UNLIKELY_START(nz, ASSERT_XEN_\@);
>> ASSERT_FAILED("CONTEXT IS XEN");
>> __UNLIKELY_END(ASSERT_XEN_\@);
>> #endif
>> .endm
> This works. I have deleted all semicolons though.
Nice!
>
> (To avoid posting other patches)
>
> ---8<---
> From 8f3ce0b0a5b6a215d4309d77d2c63229fb4af900 Mon Sep 17 00:00:00 2001
> From: Wei Liu <wei.liu2@citrix.com>
> Date: Fri, 19 Oct 2018 12:32:12 +0100
> Subject: [PATCH] x86: make entry point code build when !CONFIG_PV
>
> Skip building x86_64/compat/entry.S and put CONFIG_PV in
> x86_64/entry.S.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 6/7] x86: expose CONFIG_PV
2018-11-02 15:55 [PATCH v3 0/7] Make CONFIG_PV work on x86 Wei Liu
` (4 preceding siblings ...)
2018-11-02 15:55 ` [PATCH v3 5/7] x86: make entry point code build when !CONFIG_PV Wei Liu
@ 2018-11-02 15:55 ` Wei Liu
2018-11-02 15:55 ` [PATCH v3 7/7] x86: update help string for CONFIG_HVM Wei Liu
6 siblings, 0 replies; 29+ messages in thread
From: Wei Liu @ 2018-11-02 15:55 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v3: update text
v2: guest -> domain
---
xen/arch/x86/Kconfig | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 548cbf9..37af728 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -37,6 +37,14 @@ source "arch/Kconfig"
config PV
def_bool y
+ prompt "PV support"
+ ---help---
+ Interfaces to support PV domains. These require guest kernel support
+ to run as a PV guest, but don't require any specific hardware support.
+
+ This option is needed if you want to run PV domains.
+
+ If unsure, say Y.
config PV_LINEAR_PT
bool "Support for PV linear pagetables"
--
git-series 0.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 7/7] x86: update help string for CONFIG_HVM
2018-11-02 15:55 [PATCH v3 0/7] Make CONFIG_PV work on x86 Wei Liu
` (5 preceding siblings ...)
2018-11-02 15:55 ` [PATCH v3 6/7] x86: expose CONFIG_PV Wei Liu
@ 2018-11-02 15:55 ` Wei Liu
2018-11-05 15:20 ` Jan Beulich
6 siblings, 1 reply; 29+ messages in thread
From: Wei Liu @ 2018-11-02 15:55 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
Update text. Change "guest" to "domain" where appropriate because
"guest" doesn't include Domain 0.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v3: update text
v2: new
---
xen/arch/x86/Kconfig | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 37af728..5c2d107 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -70,12 +70,11 @@ config HVM
def_bool !PV_SHIM_EXCLUSIVE
prompt "HVM support"
---help---
- Interfaces to support HVM guests which require hardware
- support like Intel's VT-x or AMD's SVM. Note the hypervisor
- doesn't distinguish HVM or PVH guest types. PVH guest type
- is only a concept for end users.
+ Interfaces to support HVM domains. HVM domains require hardware
+ virtualisation extensions (e.g. Intel VT-x, AMD SVM), but can boot
+ guests which have no specific Xen knowledge.
- This option is needed if you want to run HVM or PVH guests.
+ This option is needed if you want to run HVM or PVH domains.
If unsure, say Y.
--
git-series 0.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread