Re: [Xen-devel] [PATCH 9/9] x86: move declaration of arch_set_info_hvm_guest and provide stub
On Thu, Aug 16, 2018 at 07:48:43AM -0600, Jan Beulich wrote: > >>> On 16.08.18 at 14:59, wrote: > > On Thu, Aug 16, 2018 at 05:24:15AM -0600, Jan Beulich wrote: > >> >>> On 16.08.18 at 12:42, 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
Re: [Xen-devel] [PATCH 9/9] x86: move declaration of arch_set_info_hvm_guest and provide stub
>>> On 16.08.18 at 14:59, wrote: > On Thu, Aug 16, 2018 at 05:24:15AM -0600, Jan Beulich wrote: >> >>> On 16.08.18 at 12:42, 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: [Xen-devel] [PATCH 9/9] x86: move declaration of arch_set_info_hvm_guest and provide stub
On Thu, Aug 16, 2018 at 05:24:15AM -0600, Jan Beulich wrote: > >>> On 16.08.18 at 12:42, 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: [Xen-devel] [PATCH 9/9] x86: move declaration of arch_set_info_hvm_guest and provide stub
>>> On 16.08.18 at 12:42, 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: [Xen-devel] [PATCH 9/9] x86: move declaration of arch_set_info_hvm_guest and provide stub
On Tue, Aug 07, 2018 at 05:46:06AM -0600, Jan Beulich wrote:
> >>> On 07.08.18 at 12:00, wrote:
> > --- a/xen/include/asm-x86/hvm/domain.h
> > +++ b/xen/include/asm-x86/hvm/domain.h
> > @@ -34,6 +34,7 @@
> > #include
> > #include
> > #include
> > +#include
>
> 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
> > +
> > +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: [Xen-devel] [PATCH 9/9] x86: move declaration of arch_set_info_hvm_guest and provide stub
>>> On 07.08.18 at 12:00, wrote:
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -34,6 +34,7 @@
> #include
> #include
> #include
> +#include
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
> +
> +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
[Xen-devel] [PATCH 9/9] x86: move declaration of arch_set_info_hvm_guest and provide stub
While at it, remove declaration of vcpu_hvm_context and use the proper
header.
Signed-off-by: Wei Liu
---
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
#include
#include
+#include
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
+
+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
