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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Aug 16, 2018 at 07:48:43AM -0600, Jan Beulich wrote:
> >>> On 16.08.18 at 14:59, <wei.liu2@xxxxxxxxxx> wrote:
> > On Thu, Aug 16, 2018 at 05:24:15AM -0600, Jan Beulich wrote:
> >> >>> On 16.08.18 at 12:42, <wei.liu2@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel