[PATCH 0/9] Miscellaneous CONFIG_HVM / CONFIG_PV clean up

[PATCH 0/9] Miscellaneous CONFIG_HVM / CONFIG_PV clean up

[Wei Liu]

Wei Liu (9):
  x86: put compat.o and x86_64/compat.o under CONFIG_PV
  x86: add missing "inline" keyword
  x86: enclose hypercall page initialisation code in CONFIG_{HVM,PV}
  x86/hvm: provide stubs for HVM guest accessors
  x86/hvm: put hvm_local_events_need_delivery into a header file
  x86: move memory_type_changed to mm.c
  x86: move arch_evtchn_inject to x86 common code
  x86/time: only HVM has RTC
  x86: move declaration of arch_set_info_hvm_guest and provide stub

 xen/arch/x86/Makefile                  |  2 +-
 xen/arch/x86/hvm/irq.c                 |  6 ------
 xen/arch/x86/hvm/mtrr.c                |  9 ---------
 xen/arch/x86/irq.c                     |  8 ++++++++
 xen/arch/x86/mm.c                      |  9 +++++++++
 xen/arch/x86/time.c                    |  4 ++++
 xen/arch/x86/x86_64/traps.c            | 15 +++++++++++----
 xen/include/asm-x86/domain.h           |  3 ---
 xen/include/asm-x86/event.h            |  3 ++-
 xen/include/asm-x86/hvm/domain.h       | 17 +++++++++++++++++
 xen/include/asm-x86/hvm/event.h        | 14 ++++++++++++++
 xen/include/asm-x86/hvm/guest_access.h | 24 ++++++++++++++++++++++++
 xen/include/asm-x86/pv/traps.h         |  2 +-
 13 files changed, 91 insertions(+), 25 deletions(-)
 create mode 100644 xen/include/asm-x86/hvm/event.h

base-commit: e752f28409678ce3ade49986b39309178fb2a6d6
-- 
git-series 0.9.1

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

[PATCH 1/9] x86: put compat.o and x86_64/compat.o under CONFIG_PV

[Wei Liu]

They contain code for compat hypercall for PV guests.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 17e7d3f..9b9b63a 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -17,7 +17,7 @@ obj-bin-y += bzimage.init.o
 obj-bin-y += clear_page.o
 obj-bin-y += copy_page.o
 obj-y += cpuid.o
-obj-y += compat.o x86_64/compat.o
+obj-$(CONFIG_PV) += compat.o x86_64/compat.o
 obj-$(CONFIG_KEXEC) += crash.o
 obj-y += debug.o
 obj-y += delay.o
-- 
git-series 0.9.1

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

[PATCH 2/9] x86: add missing "inline" keyword

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/include/asm-x86/pv/traps.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/asm-x86/pv/traps.h b/xen/include/asm-x86/pv/traps.h
index 7279d16..89985d1 100644
--- a/xen/include/asm-x86/pv/traps.h
+++ b/xen/include/asm-x86/pv/traps.h
@@ -47,7 +47,7 @@ static inline bool pv_trap_callback_registered(const struct vcpu *v,
 static inline void pv_trap_init(void) {}
 
 /* Deliver interrupt to PV guest. Return 0 on success. */
-static int pv_raise_interrupt(struct vcpu *v, uint8_t vector) { return -EOPNOTSUPP; }
+static inline int pv_raise_interrupt(struct vcpu *v, uint8_t vector) { return -EOPNOTSUPP; }
 
 static inline int pv_emulate_privileged_op(struct cpu_user_regs *regs) { return 0; }
 static inline void pv_emulate_gate_op(struct cpu_user_regs *regs) {}
-- 
git-series 0.9.1

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

[PATCH 3/9] x86: enclose hypercall page initialisation code in CONFIG_{HVM, PV}

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/x86_64/traps.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index ed02b78..87d5816 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -352,12 +352,19 @@ void subarch_percpu_traps_init(void)
 void hypercall_page_initialise(struct domain *d, void *hypercall_page)
 {
     memset(hypercall_page, 0xCC, PAGE_SIZE);
+#if CONFIG_HVM
     if ( is_hvm_domain(d) )
         hvm_hypercall_page_initialise(d, hypercall_page);
-    else if ( !is_pv_32bit_domain(d) )
-        hypercall_page_initialise_ring3_kernel(hypercall_page);
-    else
-        hypercall_page_initialise_ring1_kernel(hypercall_page);
+#endif
+#if CONFIG_PV
+    if ( is_pv_domain(d) )
+    {
+        if ( !is_pv_32bit_domain(d) )
+            hypercall_page_initialise_ring3_kernel(hypercall_page);
+        else
+            hypercall_page_initialise_ring1_kernel(hypercall_page);
+    }
+#endif
 }
 
 /*
-- 
git-series 0.9.1

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

[PATCH 4/9] x86/hvm: provide stubs for HVM guest accessors

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/include/asm-x86/hvm/guest_access.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/xen/include/asm-x86/hvm/guest_access.h b/xen/include/asm-x86/hvm/guest_access.h
index b92dbe9..c7e83dc 100644
--- a/xen/include/asm-x86/hvm/guest_access.h
+++ b/xen/include/asm-x86/hvm/guest_access.h
@@ -1,8 +1,32 @@
 #ifndef __ASM_X86_HVM_GUEST_ACCESS_H__
 #define __ASM_X86_HVM_GUEST_ACCESS_H__
 
+#if CONFIG_HVM
+
 unsigned long copy_to_user_hvm(void *to, const void *from, unsigned len);
 unsigned long clear_user_hvm(void *to, unsigned int len);
 unsigned long copy_from_user_hvm(void *to, const void *from, unsigned len);
 
+#else
+
+static inline
+unsigned long copy_to_user_hvm(void *to, const void *from, unsigned len)
+{
+    return len;
+}
+
+static inline
+unsigned long clear_user_hvm(void *to, unsigned int len)
+{
+    return len;
+}
+
+static inline
+unsigned long copy_from_user_hvm(void *to, const void *from, unsigned len)
+{
+    return len;
+}
+
+#endif
+
 #endif /* __ASM_X86_HVM_GUEST_ACCESS_H__ */
-- 
git-series 0.9.1

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

[PATCH 5/9] x86/hvm: put hvm_local_events_need_delivery into a header file

[Wei Liu]

This allows us to provide a stub for it in that header file.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/include/asm-x86/event.h     |  3 ++-
 xen/include/asm-x86/hvm/event.h | 14 ++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)
 create mode 100644 xen/include/asm-x86/hvm/event.h

diff --git a/xen/include/asm-x86/event.h b/xen/include/asm-x86/event.h
index 2f6ea54..b4472da 100644
--- a/xen/include/asm-x86/event.h
+++ b/xen/include/asm-x86/event.h
@@ -11,6 +11,8 @@
 
 #include <xen/shared.h>
 
+#include <asm/hvm/event.h>
+
 void vcpu_kick(struct vcpu *v);
 void vcpu_mark_events_pending(struct vcpu *v);
 
@@ -19,7 +21,6 @@ static inline int vcpu_event_delivery_is_enabled(struct vcpu *v)
     return !vcpu_info(v, evtchn_upcall_mask);
 }
 
-int hvm_local_events_need_delivery(struct vcpu *v);
 static inline int local_events_need_delivery(void)
 {
     struct vcpu *v = current;
diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h
new file mode 100644
index 0000000..f4781a9
--- /dev/null
+++ b/xen/include/asm-x86/hvm/event.h
@@ -0,0 +1,14 @@
+#ifndef ASM_HVM_EVENT_H
+#define ASM_HVM_EVENT_H
+
+#if CONFIG_HVM
+
+int hvm_local_events_need_delivery(struct vcpu *v);
+
+#else
+
+static inline int hvm_local_events_need_delivery(struct vcpu *v) { return 0; }
+
+#endif
+
+#endif	/* ASM_HVM_EVENT_H */
-- 
git-series 0.9.1

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

[PATCH 6/9] x86: move memory_type_changed to mm.c

[Wei Liu]

This function is common to both PV and HVM. Move it to x86 common
code.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/hvm/mtrr.c |  9 ---------
 xen/arch/x86/mm.c       |  9 +++++++++
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index c502dda..f33d3b5 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -822,15 +822,6 @@ static int hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
 HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, hvm_load_mtrr_msr,
                           1, HVMSR_PER_VCPU);
 
-void memory_type_changed(struct domain *d)
-{
-    if ( need_iommu(d) && d->vcpu && d->vcpu[0] )
-    {
-        p2m_memory_type_changed(d);
-        flush_all(FLUSH_CACHE);
-    }
-}
-
 int epte_get_entry_emt(struct domain *d, unsigned long gfn, mfn_t mfn,
                        unsigned int order, uint8_t *ipat, bool_t direct_mmio)
 {
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index a1a1f5f..f251bb3 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5761,6 +5761,15 @@ unsigned long get_upper_mfn_bound(void)
     return min(max_mfn, 1UL << (paddr_bits - PAGE_SHIFT)) - 1;
 }
 
+void memory_type_changed(struct domain *d)
+{
+    if ( need_iommu(d) && d->vcpu && d->vcpu[0] )
+    {
+        p2m_memory_type_changed(d);
+        flush_all(FLUSH_CACHE);
+    }
+}
+
 /*
  * Local variables:
  * mode: C
-- 
git-series 0.9.1

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

[PATCH 7/9] x86: move arch_evtchn_inject to x86 common code

[Wei Liu]

It is not specific to HVM. It just so happens that PV doesn't need
special handling. Also enclose the code in CONFIG_HVM.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/hvm/irq.c | 6 ------
 xen/arch/x86/irq.c     | 8 ++++++++
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 8095c82..dfe8ed6 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -577,12 +577,6 @@ int hvm_local_events_need_delivery(struct vcpu *v)
     return !hvm_interrupt_blocked(v, intack);
 }
 
-void arch_evtchn_inject(struct vcpu *v)
-{
-    if ( is_hvm_vcpu(v) )
-        hvm_assert_evtchn_irq(v);
-}
-
 static void irq_dump(struct domain *d)
 {
     struct hvm_irq *hvm_irq = hvm_domain_irq(d);
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 7d0b19f..1aeb02f 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2732,3 +2732,11 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
 
     return ret;
 }
+
+void arch_evtchn_inject(struct vcpu *v)
+{
+#if CONFIG_HVM
+    if ( is_hvm_vcpu(v) )
+        hvm_assert_evtchn_irq(v);
+#endif
+}
-- 
git-series 0.9.1

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

[PATCH 8/9] x86/time: only HVM has RTC

[Wei Liu]

PV doesn't have RTC. Enclose relevant code in CONFIG_HVM.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/time.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 536449b..8d74210 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1144,6 +1144,7 @@ void force_update_vcpu_system_time(struct vcpu *v)
 
 static void update_domain_rtc(void)
 {
+#if CONFIG_HVM
     struct domain *d;
 
     rcu_read_lock(&domlist_read_lock);
@@ -1153,13 +1154,16 @@ static void update_domain_rtc(void)
             rtc_update_clock(d);
 
     rcu_read_unlock(&domlist_read_lock);
+#endif
 }
 
 void domain_set_time_offset(struct domain *d, int64_t time_offset_seconds)
 {
     d->time_offset_seconds = time_offset_seconds;
+#if CONFIG_HVM
     if ( is_hvm_domain(d) )
         rtc_update_clock(d);
+#endif
     update_domain_wallclock_time(d);
 }
 
-- 
git-series 0.9.1

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

[PATCH 9/9] x86: move declaration of arch_set_info_hvm_guest and provide stub

[Wei Liu]

While at it, remove declaration of vcpu_hvm_context and use the proper
header.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/include/asm-x86/domain.h     |  3 ---
 xen/include/asm-x86/hvm/domain.h | 17 +++++++++++++++++
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index e2442f4..d3bc5dc 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -631,9 +631,6 @@ static inline void free_vcpu_guest_context(struct vcpu_guest_context *vgc)
     vfree(vgc);
 }
 
-struct vcpu_hvm_context;
-int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context *ctx);
-
 void pv_inject_event(const struct x86_event *event);
 
 static inline void pv_inject_hw_exception(unsigned int vector, int errcode)
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 5885950..231b424 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -34,6 +34,7 @@
 #include <public/hvm/save.h>
 #include <public/hvm/hvm_op.h>
 #include <public/hvm/dm_op.h>
+#include <public/hvm/hvm_vcpu.h>
 
 struct hvm_ioreq_page {
     gfn_t gfn;
@@ -204,6 +205,22 @@ struct hvm_domain {
 
 #define hap_enabled(d)  ((d)->arch.hvm_domain.hap_enabled)
 
+#if CONFIG_HVM
+
+int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context *ctx);
+
+#else
+
+#include <xen/errno.h>
+
+static inline
+int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context *ctx)
+{
+    return -EINVAL;
+}
+
+#endif
+
 #endif /* __ASM_X86_HVM_DOMAIN_H__ */
 
 /*
-- 
git-series 0.9.1

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

Re: [PATCH 1/9] x86: put compat.o and x86_64/compat.o under CONFIG_PV

[Jan Beulich]

>>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
> They contain code for compat hypercall for PV guests.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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



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

Re: [PATCH 2/9] x86: add missing "inline" keyword

[Jan Beulich]

>>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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



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

Re: [PATCH 3/9] x86: enclose hypercall page initialisation code in CONFIG_{HVM, PV}

[Jan Beulich]

>>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -352,12 +352,19 @@ void subarch_percpu_traps_init(void)
>  void hypercall_page_initialise(struct domain *d, void *hypercall_page)
>  {
>      memset(hypercall_page, 0xCC, PAGE_SIZE);
> +#if CONFIG_HVM
>      if ( is_hvm_domain(d) )
>          hvm_hypercall_page_initialise(d, hypercall_page);
> -    else if ( !is_pv_32bit_domain(d) )
> -        hypercall_page_initialise_ring3_kernel(hypercall_page);
> -    else
> -        hypercall_page_initialise_ring1_kernel(hypercall_page);
> +#endif
> +#if CONFIG_PV
> +    if ( is_pv_domain(d) )
> +    {
> +        if ( !is_pv_32bit_domain(d) )
> +            hypercall_page_initialise_ring3_kernel(hypercall_page);
> +        else
> +            hypercall_page_initialise_ring1_kernel(hypercall_page);
> +    }
> +#endif
>  }

I'm not convinced: This is uglier to read, and things like is_hvm_domain()
should evaluate to constant false anyway without the respective
CONFIG_* setting enabled.

Jan



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

Re: [PATCH 4/9] x86/hvm: provide stubs for HVM guest accessors

[Jan Beulich]

>>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
> --- a/xen/include/asm-x86/hvm/guest_access.h
> +++ b/xen/include/asm-x86/hvm/guest_access.h
> @@ -1,8 +1,32 @@
>  #ifndef __ASM_X86_HVM_GUEST_ACCESS_H__
>  #define __ASM_X86_HVM_GUEST_ACCESS_H__
>  
> +#if CONFIG_HVM
> +
>  unsigned long copy_to_user_hvm(void *to, const void *from, unsigned len);
>  unsigned long clear_user_hvm(void *to, unsigned int len);
>  unsigned long copy_from_user_hvm(void *to, const void *from, unsigned len);
>  
> +#else
> +
> +static inline
> +unsigned long copy_to_user_hvm(void *to, const void *from, unsigned len)
> +{
> +    return len;
> +}
> +
> +static inline
> +unsigned long clear_user_hvm(void *to, unsigned int len)
> +{
> +    return len;
> +}
> +
> +static inline
> +unsigned long copy_from_user_hvm(void *to, const void *from, unsigned len)
> +{
> +    return len;
> +}
> +
> +#endif

With BUG() or at least ASSERT_UNREACHABLE() put into all of them
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH 3/9] x86: enclose hypercall page initialisation code in CONFIG_{HVM, PV}

[Andrew Cooper]

On 07/08/18 11:44, Jan Beulich wrote:
>>>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
>> --- a/xen/arch/x86/x86_64/traps.c
>> +++ b/xen/arch/x86/x86_64/traps.c
>> @@ -352,12 +352,19 @@ void subarch_percpu_traps_init(void)
>>  void hypercall_page_initialise(struct domain *d, void *hypercall_page)
>>  {
>>      memset(hypercall_page, 0xCC, PAGE_SIZE);
>> +#if CONFIG_HVM
>>      if ( is_hvm_domain(d) )
>>          hvm_hypercall_page_initialise(d, hypercall_page);
>> -    else if ( !is_pv_32bit_domain(d) )
>> -        hypercall_page_initialise_ring3_kernel(hypercall_page);
>> -    else
>> -        hypercall_page_initialise_ring1_kernel(hypercall_page);
>> +#endif
>> +#if CONFIG_PV
>> +    if ( is_pv_domain(d) )
>> +    {
>> +        if ( !is_pv_32bit_domain(d) )
>> +            hypercall_page_initialise_ring3_kernel(hypercall_page);
>> +        else
>> +            hypercall_page_initialise_ring1_kernel(hypercall_page);
>> +    }
>> +#endif
>>  }
> I'm not convinced: This is uglier to read, and things like is_hvm_domain()
> should evaluate to constant false anyway without the respective
> CONFIG_* setting enabled.

I agree.

Each of the 4 initialisation functions could be static inlines, because
there is nothing complicated about them, and the static evaluation of
is_*_domain() would cause the properly unused ones to be omitted.

~Andrew

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

Re: [PATCH 5/9] x86/hvm: put hvm_local_events_need_delivery into a header file

[Jan Beulich]

>>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
> --- /dev/null
> +++ b/xen/include/asm-x86/hvm/event.h
> @@ -0,0 +1,14 @@
> +#ifndef ASM_HVM_EVENT_H
> +#define ASM_HVM_EVENT_H
> +
> +#if CONFIG_HVM
> +
> +int hvm_local_events_need_delivery(struct vcpu *v);
> +
> +#else
> +
> +static inline int hvm_local_events_need_delivery(struct vcpu *v) { return 0; }
> +
> +#endif
> +
> +#endif	/* ASM_HVM_EVENT_H */

Are you expecting more stuff to go into this header? If not, I don't
think a separate header is really warranted here.

Did you consider taking the opportunity and switching the function
to have bool return value at the same time? (Seeing whether the
parameter could also be constified is probably more involved a
task, and hence not suitable here.)

Jan



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

Re: [PATCH 6/9] x86: move memory_type_changed to mm.c

[Jan Beulich]

>>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
> This function is common to both PV and HVM. Move it to x86 common
> code.

I'm afraid that's the wrong way round: p2m_memory_type_changed()
is HVM (in fact EPT) specific. The only uses of the function that aren't
already HVM-specific are from domctl.c and from iommu_construct(),
yet I doubt the flush_all(FLUSH_CACHE) has any relevance there in
the PV case.

Jan



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

Re: [PATCH 7/9] x86: move arch_evtchn_inject to x86 common code

[Jan Beulich]

>>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2732,3 +2732,11 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
>  
>      return ret;
>  }
> +
> +void arch_evtchn_inject(struct vcpu *v)
> +{
> +#if CONFIG_HVM
> +    if ( is_hvm_vcpu(v) )
> +        hvm_assert_evtchn_irq(v);
> +#endif
> +}

Without the #ifdef
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH 8/9] x86/time: only HVM has RTC

[Jan Beulich]

>>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1144,6 +1144,7 @@ void force_update_vcpu_system_time(struct vcpu *v)
>  
>  static void update_domain_rtc(void)
>  {
> +#if CONFIG_HVM
>      struct domain *d;
>  
>      rcu_read_lock(&domlist_read_lock);
> @@ -1153,13 +1154,16 @@ static void update_domain_rtc(void)
>              rtc_update_clock(d);
>  
>      rcu_read_unlock(&domlist_read_lock);
> +#endif
>  }

I think the function should gain a hvm_ prefix and move to
hvm/rtc.c.

Jan



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

Re: [PATCH 9/9] x86: move declaration of arch_set_info_hvm_guest and provide stub

[Jan Beulich]

>>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -34,6 +34,7 @@
>  #include <public/hvm/save.h>
>  #include <public/hvm/hvm_op.h>
>  #include <public/hvm/dm_op.h>
> +#include <public/hvm/hvm_vcpu.h>

Why? struct vcpu_hvm_context only needs a forward declaration, just
like was the case originally. Full structure/union definitions are needed
only if you de-reference the type, instantiate it, or apply sizeof() and
alike to it.

In fact I suspect we could reduce header dependencies quite a bit if
we followed that model more widely.

> @@ -204,6 +205,22 @@ struct hvm_domain {
>  
>  #define hap_enabled(d)  ((d)->arch.hvm_domain.hap_enabled)
>  
> +#if CONFIG_HVM
> +
> +int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context *ctx);
> +
> +#else
> +
> +#include <xen/errno.h>
> +
> +static inline
> +int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context *ctx)
> +{
> +    return -EINVAL;
> +}
> +
> +#endif

All uses sit either in HVM-specific code or inside is_hvm_...()
conditionals: Why do we need the inline stub? If the declaration
was visible independent of CONFIG_HVM, code would compile
fine, and references to the function would be removed by the
compiler, so linking would also succeed despite there not being
any definition of the function.

Jan



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

Re: [PATCH 3/9] x86: enclose hypercall page initialisation code in CONFIG_{HVM, PV}

[Wei Liu]

On Tue, Aug 07, 2018 at 04:44:13AM -0600, Jan Beulich wrote:
> >>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
> > --- a/xen/arch/x86/x86_64/traps.c
> > +++ b/xen/arch/x86/x86_64/traps.c
> > @@ -352,12 +352,19 @@ void subarch_percpu_traps_init(void)
> >  void hypercall_page_initialise(struct domain *d, void *hypercall_page)
> >  {
> >      memset(hypercall_page, 0xCC, PAGE_SIZE);
> > +#if CONFIG_HVM
> >      if ( is_hvm_domain(d) )
> >          hvm_hypercall_page_initialise(d, hypercall_page);
> > -    else if ( !is_pv_32bit_domain(d) )
> > -        hypercall_page_initialise_ring3_kernel(hypercall_page);
> > -    else
> > -        hypercall_page_initialise_ring1_kernel(hypercall_page);
> > +#endif
> > +#if CONFIG_PV
> > +    if ( is_pv_domain(d) )
> > +    {
> > +        if ( !is_pv_32bit_domain(d) )
> > +            hypercall_page_initialise_ring3_kernel(hypercall_page);
> > +        else
> > +            hypercall_page_initialise_ring1_kernel(hypercall_page);
> > +    }
> > +#endif
> >  }
> 
> I'm not convinced: This is uglier to read, and things like is_hvm_domain()
> should evaluate to constant false anyway without the respective
> CONFIG_* setting enabled.

I take it you are against adding CONFIG_* here?

The addition of is_pv_domain branch is still needed because both sets
might get compiled out.

Wei.

> 
> Jan
> 
> 

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

Re: [PATCH 5/9] x86/hvm: put hvm_local_events_need_delivery into a header file

[Wei Liu]

On Tue, Aug 07, 2018 at 04:50:21AM -0600, Jan Beulich wrote:
> >>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
> > --- /dev/null
> > +++ b/xen/include/asm-x86/hvm/event.h
> > @@ -0,0 +1,14 @@
> > +#ifndef ASM_HVM_EVENT_H
> > +#define ASM_HVM_EVENT_H
> > +
> > +#if CONFIG_HVM
> > +
> > +int hvm_local_events_need_delivery(struct vcpu *v);
> > +
> > +#else
> > +
> > +static inline int hvm_local_events_need_delivery(struct vcpu *v) { return 0; }
> > +
> > +#endif
> > +
> > +#endif	/* ASM_HVM_EVENT_H */
> 
> Are you expecting more stuff to go into this header? If not, I don't
> think a separate header is really warranted here.

Not at this stage. I have mostly been using the following structure
while trying to split things:

  asm-x86/THING.h
  asm-x86/hvm/THING.h
  asm-x86/pv/THING.h

Then THING.h includes both pv and hvm THING.h's.  It is just more
structural to me.

Since you object here, I guess you will be happy with moving the content
of this new file to asm-x86/event.h?

> 
> Did you consider taking the opportunity and switching the function
> to have bool return value at the same time? (Seeing whether the
> parameter could also be constified is probably more involved a
> task, and hence not suitable here.)
> 

Switching to bool -- sure.

Wei.

> Jan
> 
> 

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

Re: [PATCH 3/9] x86: enclose hypercall page initialisation code in CONFIG_{HVM, PV}

[Jan Beulich]

>>> On 07.08.18 at 18:08, <wei.liu2@citrix.com> wrote:
> On Tue, Aug 07, 2018 at 04:44:13AM -0600, Jan Beulich wrote:
>> >>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
>> > --- a/xen/arch/x86/x86_64/traps.c
>> > +++ b/xen/arch/x86/x86_64/traps.c
>> > @@ -352,12 +352,19 @@ void subarch_percpu_traps_init(void)
>> >  void hypercall_page_initialise(struct domain *d, void *hypercall_page)
>> >  {
>> >      memset(hypercall_page, 0xCC, PAGE_SIZE);
>> > +#if CONFIG_HVM
>> >      if ( is_hvm_domain(d) )
>> >          hvm_hypercall_page_initialise(d, hypercall_page);
>> > -    else if ( !is_pv_32bit_domain(d) )
>> > -        hypercall_page_initialise_ring3_kernel(hypercall_page);
>> > -    else
>> > -        hypercall_page_initialise_ring1_kernel(hypercall_page);
>> > +#endif
>> > +#if CONFIG_PV
>> > +    if ( is_pv_domain(d) )
>> > +    {
>> > +        if ( !is_pv_32bit_domain(d) )
>> > +            hypercall_page_initialise_ring3_kernel(hypercall_page);
>> > +        else
>> > +            hypercall_page_initialise_ring1_kernel(hypercall_page);
>> > +    }
>> > +#endif
>> >  }
>> 
>> I'm not convinced: This is uglier to read, and things like is_hvm_domain()
>> should evaluate to constant false anyway without the respective
>> CONFIG_* setting enabled.
> 
> I take it you are against adding CONFIG_* here?
> 
> The addition of is_pv_domain branch is still needed because both sets
> might get compiled out.

But that can be done by making the sequence

	if ( is_hvm_... )
	else if ( is_pv_32bit_... )
	else if ( is_pv_... )

I.e. in particular without any changes to the nesting.

Jan



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

Re: [PATCH 6/9] x86: move memory_type_changed to mm.c

[Wei Liu]

On Tue, Aug 07, 2018 at 05:33:35AM -0600, Jan Beulich wrote:
> >>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
> > This function is common to both PV and HVM. Move it to x86 common
> > code.
> 
> I'm afraid that's the wrong way round: p2m_memory_type_changed()
> is HVM (in fact EPT) specific. The only uses of the function that aren't
> already HVM-specific are from domctl.c and from iommu_construct(),
> yet I doubt the flush_all(FLUSH_CACHE) has any relevance there in
> the PV case.

Yeah, I got the impression that flushing is relevant to PV from reading
existing code.

If it is HVM only I suggest we add hvm prefix to it and only call it
when the domain in question is hvm. This follows what we already do in
other places.

   if ( is_hvm_domain(d) )
       hvm_memory_type_change(...)

Wei.

> 
> Jan
> 
> 

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

Re: [PATCH 3/9] x86: enclose hypercall page initialisation code in CONFIG_{HVM, PV}

[Wei Liu]

On Tue, Aug 07, 2018 at 10:18:36AM -0600, Jan Beulich wrote:
> >>> On 07.08.18 at 18:08, <wei.liu2@citrix.com> wrote:
> > On Tue, Aug 07, 2018 at 04:44:13AM -0600, Jan Beulich wrote:
> >> >>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
> >> > --- a/xen/arch/x86/x86_64/traps.c
> >> > +++ b/xen/arch/x86/x86_64/traps.c
> >> > @@ -352,12 +352,19 @@ void subarch_percpu_traps_init(void)
> >> >  void hypercall_page_initialise(struct domain *d, void *hypercall_page)
> >> >  {
> >> >      memset(hypercall_page, 0xCC, PAGE_SIZE);
> >> > +#if CONFIG_HVM
> >> >      if ( is_hvm_domain(d) )
> >> >          hvm_hypercall_page_initialise(d, hypercall_page);
> >> > -    else if ( !is_pv_32bit_domain(d) )
> >> > -        hypercall_page_initialise_ring3_kernel(hypercall_page);
> >> > -    else
> >> > -        hypercall_page_initialise_ring1_kernel(hypercall_page);
> >> > +#endif
> >> > +#if CONFIG_PV
> >> > +    if ( is_pv_domain(d) )
> >> > +    {
> >> > +        if ( !is_pv_32bit_domain(d) )
> >> > +            hypercall_page_initialise_ring3_kernel(hypercall_page);
> >> > +        else
> >> > +            hypercall_page_initialise_ring1_kernel(hypercall_page);
> >> > +    }
> >> > +#endif
> >> >  }
> >> 
> >> I'm not convinced: This is uglier to read, and things like is_hvm_domain()
> >> should evaluate to constant false anyway without the respective
> >> CONFIG_* setting enabled.
> > 
> > I take it you are against adding CONFIG_* here?
> > 
> > The addition of is_pv_domain branch is still needed because both sets
> > might get compiled out.
> 
> But that can be done by making the sequence
> 
> 	if ( is_hvm_... )
> 	else if ( is_pv_32bit_... )
> 	else if ( is_pv_... )
> 
> I.e. in particular without any changes to the nesting.

Yeah, sure. I just wanted to make sure PV is explicitly tested. The
above structure is fine by me.

Wei.

> 
> Jan
> 
> 

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

Re: [PATCH 5/9] x86/hvm: put hvm_local_events_need_delivery into a header file

[Jan Beulich]

>>> On 07.08.18 at 18:13, <wei.liu2@citrix.com> wrote:
> On Tue, Aug 07, 2018 at 04:50:21AM -0600, Jan Beulich wrote:
>> >>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
>> > --- /dev/null
>> > +++ b/xen/include/asm-x86/hvm/event.h
>> > @@ -0,0 +1,14 @@
>> > +#ifndef ASM_HVM_EVENT_H
>> > +#define ASM_HVM_EVENT_H
>> > +
>> > +#if CONFIG_HVM
>> > +
>> > +int hvm_local_events_need_delivery(struct vcpu *v);
>> > +
>> > +#else
>> > +
>> > +static inline int hvm_local_events_need_delivery(struct vcpu *v) { return 0; }
>> > +
>> > +#endif
>> > +
>> > +#endif	/* ASM_HVM_EVENT_H */
>> 
>> Are you expecting more stuff to go into this header? If not, I don't
>> think a separate header is really warranted here.
> 
> Not at this stage. I have mostly been using the following structure
> while trying to split things:
> 
>   asm-x86/THING.h
>   asm-x86/hvm/THING.h
>   asm-x86/pv/THING.h
> 
> Then THING.h includes both pv and hvm THING.h's.  It is just more
> structural to me.
> 
> Since you object here, I guess you will be happy with moving the content
> of this new file to asm-x86/event.h?

Yes, but that's where it is now. And together with the question
whether an inline stub is needed in the first place, I wonder
whether a change is necessary here at all.

Jan



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

Re: [PATCH 9/9] x86: move declaration of arch_set_info_hvm_guest and provide stub

[Wei Liu]

On Tue, Aug 07, 2018 at 05:46:06AM -0600, Jan Beulich wrote:
> >>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
> > --- a/xen/include/asm-x86/hvm/domain.h
> > +++ b/xen/include/asm-x86/hvm/domain.h
> > @@ -34,6 +34,7 @@
> >  #include <public/hvm/save.h>
> >  #include <public/hvm/hvm_op.h>
> >  #include <public/hvm/dm_op.h>
> > +#include <public/hvm/hvm_vcpu.h>
> 
> Why? struct vcpu_hvm_context only needs a forward declaration, just
> like was the case originally. Full structure/union definitions are needed
> only if you de-reference the type, instantiate it, or apply sizeof() and
> alike to it.

Alright, I'm not too fussed about this.

> 
> In fact I suspect we could reduce header dependencies quite a bit if
> we followed that model more widely.
> 
> > @@ -204,6 +205,22 @@ struct hvm_domain {
> >  
> >  #define hap_enabled(d)  ((d)->arch.hvm_domain.hap_enabled)
> >  
> > +#if CONFIG_HVM
> > +
> > +int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context *ctx);
> > +
> > +#else
> > +
> > +#include <xen/errno.h>
> > +
> > +static inline
> > +int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context *ctx)
> > +{
> > +    return -EINVAL;
> > +}
> > +
> > +#endif
> 
> All uses sit either in HVM-specific code or inside is_hvm_...()
> conditionals: Why do we need the inline stub? If the declaration
> was visible independent of CONFIG_HVM, code would compile
> fine, and references to the function would be removed by the
> compiler, so linking would also succeed despite there not being
> any definition of the function.

Last time I tried DCE wasn't working so well. Let me try again and if
DCE works it would save me a lot of effort to provide stubs.

Wei.

> 
> Jan
> 
> 

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

Re: [PATCH 6/9] x86: move memory_type_changed to mm.c

[Jan Beulich]

>>> On 07.08.18 at 18:19, <wei.liu2@citrix.com> wrote:
> On Tue, Aug 07, 2018 at 05:33:35AM -0600, Jan Beulich wrote:
>> >>> On 07.08.18 at 12:00, <wei.liu2@citrix.com> wrote:
>> > This function is common to both PV and HVM. Move it to x86 common
>> > code.
>> 
>> I'm afraid that's the wrong way round: p2m_memory_type_changed()
>> is HVM (in fact EPT) specific. The only uses of the function that aren't
>> already HVM-specific are from domctl.c and from iommu_construct(),
>> yet I doubt the flush_all(FLUSH_CACHE) has any relevance there in
>> the PV case.
> 
> Yeah, I got the impression that flushing is relevant to PV from reading
> existing code.

Well, I would have thought so as well, but the few places where the
function is possibly used on a PV domain all don't look as if the cache
flush could matter there (quite possibly it could even be avoided in
those specific cases for HVM, at the very least in iommu_construct()).

> If it is HVM only I suggest we add hvm prefix to it and only call it
> when the domain in question is hvm. This follows what we already do in
> other places.
> 
>    if ( is_hvm_domain(d) )
>        hvm_memory_type_change(...)

Yeah, that would probably be fine if we indeed settle on the function
being meaningless for PV.

Jan



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

Re: [PATCH 9/9] x86: move declaration of arch_set_info_hvm_guest and provide stub

[Wei Liu]

On Tue, Aug 07, 2018 at 05:24:15PM +0100, Wei Liu wrote:
 > 
> > All uses sit either in HVM-specific code or inside is_hvm_...()
> > conditionals: Why do we need the inline stub? If the declaration
> > was visible independent of CONFIG_HVM, code would compile
> > fine, and references to the function would be removed by the
> > compiler, so linking would also succeed despite there not being
> > any definition of the function.
> 
> Last time I tried DCE wasn't working so well. Let me try again and if
> DCE works it would save me a lot of effort to provide stubs.
> 

DCE seems to work better this time.

The only problem is that some ASSERTs will need to turn into panic or
BUG() if we want to fully utilise DCE. Is that okay?

To give you an example, after making is_hvm_domain evaluate to 0 when
!CONFIG_HVM, vm_event_fill_regs + !CONFIG_HVM compiles fine for debug
build because ASSERT hints the compiler that the rest of the function is
never going to be reachable. But for non-debug build, ASSERT is empty,
so compiler will not eliminate the rest of the function, complaining
hvm_get_segment_register is not available. It is solvable by adding
panic or BUG.

There is going to be quite a few cases like that. I haven't gone through
all of them.

Let me know your opinion.

Wei.

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

Re: [PATCH 9/9] x86: move declaration of arch_set_info_hvm_guest and provide stub

[Jan Beulich]

>>> On 16.08.18 at 12:42, <wei.liu2@citrix.com> wrote:
> On Tue, Aug 07, 2018 at 05:24:15PM +0100, Wei Liu wrote:
>  > 
>> > All uses sit either in HVM-specific code or inside is_hvm_...()
>> > conditionals: Why do we need the inline stub? If the declaration
>> > was visible independent of CONFIG_HVM, code would compile
>> > fine, and references to the function would be removed by the
>> > compiler, so linking would also succeed despite there not being
>> > any definition of the function.
>> 
>> Last time I tried DCE wasn't working so well. Let me try again and if
>> DCE works it would save me a lot of effort to provide stubs.
>> 
> 
> DCE seems to work better this time.
> 
> The only problem is that some ASSERTs will need to turn into panic or
> BUG() if we want to fully utilise DCE. Is that okay?

In general yes, I think so.

> To give you an example, after making is_hvm_domain evaluate to 0 when
> !CONFIG_HVM, vm_event_fill_regs + !CONFIG_HVM compiles fine for debug
> build because ASSERT hints the compiler that the rest of the function is
> never going to be reachable. But for non-debug build, ASSERT is empty,
> so compiler will not eliminate the rest of the function, complaining
> hvm_get_segment_register is not available. It is solvable by adding
> panic or BUG.
> 
> There is going to be quite a few cases like that. I haven't gone through
> all of them.

In cases like the example you give I'm not convinced of the
suggested conversion though - the entire function should then
live inside CONFIG_HVM (or in a file built for HVM only).

Jan



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

Re: [PATCH 9/9] x86: move declaration of arch_set_info_hvm_guest and provide stub

[Wei Liu]

On Thu, Aug 16, 2018 at 05:24:15AM -0600, Jan Beulich wrote:
> >>> On 16.08.18 at 12:42, <wei.liu2@citrix.com> wrote:
> > On Tue, Aug 07, 2018 at 05:24:15PM +0100, Wei Liu wrote:
> >  > 
> >> > All uses sit either in HVM-specific code or inside is_hvm_...()
> >> > conditionals: Why do we need the inline stub? If the declaration
> >> > was visible independent of CONFIG_HVM, code would compile
> >> > fine, and references to the function would be removed by the
> >> > compiler, so linking would also succeed despite there not being
> >> > any definition of the function.
> >> 
> >> Last time I tried DCE wasn't working so well. Let me try again and if
> >> DCE works it would save me a lot of effort to provide stubs.
> >> 
> > 
> > DCE seems to work better this time.
> > 
> > The only problem is that some ASSERTs will need to turn into panic or
> > BUG() if we want to fully utilise DCE. Is that okay?
> 
> In general yes, I think so.
> 
> > To give you an example, after making is_hvm_domain evaluate to 0 when
> > !CONFIG_HVM, vm_event_fill_regs + !CONFIG_HVM compiles fine for debug
> > build because ASSERT hints the compiler that the rest of the function is
> > never going to be reachable. But for non-debug build, ASSERT is empty,
> > so compiler will not eliminate the rest of the function, complaining
> > hvm_get_segment_register is not available. It is solvable by adding
> > panic or BUG.
> > 
> > There is going to be quite a few cases like that. I haven't gone through
> > all of them.
> 
> In cases like the example you give I'm not convinced of the
> suggested conversion though - the entire function should then
> live inside CONFIG_HVM (or in a file built for HVM only).
> 

This will do, too -- if you don't mind littering CONFIG_HVM in files.

VM event subsystem is entangled with other subsystems, too, so it will
take some time to clean that up. I haven't got to that part yet. At the
moment I have accumulated ~25 patches to almost make !CONFIG_HVM work
for debug build. I will go through all patches later to make them work
with non-debug build.

One thing I haven't decided what to do is hvm/i8254.c, which is used by
both PV and HVM. I'm thinking about moving that file under arch/x86 and
rename it emul-i8254.c. Is that a sensible thing to do?

Wei.

> Jan
> 
> 

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

Re: [PATCH 9/9] x86: move declaration of arch_set_info_hvm_guest and provide stub

[Jan Beulich]

>>> On 16.08.18 at 14:59, <wei.liu2@citrix.com> wrote:
> On Thu, Aug 16, 2018 at 05:24:15AM -0600, Jan Beulich wrote:
>> >>> On 16.08.18 at 12:42, <wei.liu2@citrix.com> wrote:
>> > On Tue, Aug 07, 2018 at 05:24:15PM +0100, Wei Liu wrote:
>> >  > 
>> >> > All uses sit either in HVM-specific code or inside is_hvm_...()
>> >> > conditionals: Why do we need the inline stub? If the declaration
>> >> > was visible independent of CONFIG_HVM, code would compile
>> >> > fine, and references to the function would be removed by the
>> >> > compiler, so linking would also succeed despite there not being
>> >> > any definition of the function.
>> >> 
>> >> Last time I tried DCE wasn't working so well. Let me try again and if
>> >> DCE works it would save me a lot of effort to provide stubs.
>> >> 
>> > 
>> > DCE seems to work better this time.
>> > 
>> > The only problem is that some ASSERTs will need to turn into panic or
>> > BUG() if we want to fully utilise DCE. Is that okay?
>> 
>> In general yes, I think so.
>> 
>> > To give you an example, after making is_hvm_domain evaluate to 0 when
>> > !CONFIG_HVM, vm_event_fill_regs + !CONFIG_HVM compiles fine for debug
>> > build because ASSERT hints the compiler that the rest of the function is
>> > never going to be reachable. But for non-debug build, ASSERT is empty,
>> > so compiler will not eliminate the rest of the function, complaining
>> > hvm_get_segment_register is not available. It is solvable by adding
>> > panic or BUG.
>> > 
>> > There is going to be quite a few cases like that. I haven't gone through
>> > all of them.
>> 
>> In cases like the example you give I'm not convinced of the
>> suggested conversion though - the entire function should then
>> live inside CONFIG_HVM (or in a file built for HVM only).
>> 
> 
> This will do, too -- if you don't mind littering CONFIG_HVM in files.
> 
> VM event subsystem is entangled with other subsystems, too, so it will
> take some time to clean that up. I haven't got to that part yet. At the
> moment I have accumulated ~25 patches to almost make !CONFIG_HVM work
> for debug build. I will go through all patches later to make them work
> with non-debug build.

That'll be fine for now, I think. Eventually the HVM pieces should be
moved to hvm/ of course.

> One thing I haven't decided what to do is hvm/i8254.c, which is used by
> both PV and HVM. I'm thinking about moving that file under arch/x86 and
> rename it emul-i8254.c. Is that a sensible thing to do?

Any chance you could leave HVM-only parts where they are? Or
would that make more of a mess than moving the entire file?

Jan



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

Re: [PATCH 9/9] x86: move declaration of arch_set_info_hvm_guest and provide stub

[Wei Liu]

On Thu, Aug 16, 2018 at 07:48:43AM -0600, Jan Beulich wrote:
> >>> On 16.08.18 at 14:59, <wei.liu2@citrix.com> wrote:
> > On Thu, Aug 16, 2018 at 05:24:15AM -0600, Jan Beulich wrote:
> >> >>> On 16.08.18 at 12:42, <wei.liu2@citrix.com> wrote:
> >> > On Tue, Aug 07, 2018 at 05:24:15PM +0100, Wei Liu wrote:
> >> >  > 
> >> >> > All uses sit either in HVM-specific code or inside is_hvm_...()
> >> >> > conditionals: Why do we need the inline stub? If the declaration
> >> >> > was visible independent of CONFIG_HVM, code would compile
> >> >> > fine, and references to the function would be removed by the
> >> >> > compiler, so linking would also succeed despite there not being
> >> >> > any definition of the function.
> >> >> 
> >> >> Last time I tried DCE wasn't working so well. Let me try again and if
> >> >> DCE works it would save me a lot of effort to provide stubs.
> >> >> 
> >> > 
> >> > DCE seems to work better this time.
> >> > 
> >> > The only problem is that some ASSERTs will need to turn into panic or
> >> > BUG() if we want to fully utilise DCE. Is that okay?
> >> 
> >> In general yes, I think so.
> >> 
> >> > To give you an example, after making is_hvm_domain evaluate to 0 when
> >> > !CONFIG_HVM, vm_event_fill_regs + !CONFIG_HVM compiles fine for debug
> >> > build because ASSERT hints the compiler that the rest of the function is
> >> > never going to be reachable. But for non-debug build, ASSERT is empty,
> >> > so compiler will not eliminate the rest of the function, complaining
> >> > hvm_get_segment_register is not available. It is solvable by adding
> >> > panic or BUG.
> >> > 
> >> > There is going to be quite a few cases like that. I haven't gone through
> >> > all of them.
> >> 
> >> In cases like the example you give I'm not convinced of the
> >> suggested conversion though - the entire function should then
> >> live inside CONFIG_HVM (or in a file built for HVM only).
> >> 
> > 
> > This will do, too -- if you don't mind littering CONFIG_HVM in files.
> > 
> > VM event subsystem is entangled with other subsystems, too, so it will
> > take some time to clean that up. I haven't got to that part yet. At the
> > moment I have accumulated ~25 patches to almost make !CONFIG_HVM work
> > for debug build. I will go through all patches later to make them work
> > with non-debug build.
> 
> That'll be fine for now, I think. Eventually the HVM pieces should be
> moved to hvm/ of course.
> 

Ack.

> > One thing I haven't decided what to do is hvm/i8254.c, which is used by
> > both PV and HVM. I'm thinking about moving that file under arch/x86 and
> > rename it emul-i8254.c. Is that a sensible thing to do?
> 
> Any chance you could leave HVM-only parts where they are? Or
> would that make more of a mess than moving the entire file?

Basically everything in that file is used by both HVM and PV. I will
leave the HVM-only parts under hvm/ if there is any.

Wei.

> 
> Jan
> 
> 

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