Re: [PATCH v2 12/23] x86: monitor.o is currently HVM only

[Wei Liu <wei.liu2@citrix.com>]

On Wed, Aug 29, 2018 at 09:09:03PM +0300, Razvan Cojocaru wrote:
> On 8/29/18 8:43 PM, Tamas K Lengyel wrote:
> > On Wed, Aug 29, 2018 at 10:42 AM Wei Liu <wei.liu2@citrix.com> wrote:
> >>
> >> On Mon, Aug 27, 2018 at 09:18:29AM -0600, Jan Beulich wrote:
> >>>>>> On 26.08.18 at 14:19, <wei.liu2@citrix.com> wrote:
> >>>> There has been plan to make PV work, but it is not yet there.  Provide
> >>>> stubs to make it build with !CONFIG_HVM.
> >>>>
> >>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> >>>> ---
> >>>>  xen/arch/x86/Makefile         |  2 +-
> >>>>  xen/include/asm-x86/monitor.h | 14 ++++++++++++++
> >>>>  2 files changed, 15 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> >>>> index 9b9b63a..43f9189 100644
> >>>> --- a/xen/arch/x86/Makefile
> >>>> +++ b/xen/arch/x86/Makefile
> >>>> @@ -45,7 +45,7 @@ obj-y += microcode_amd.o
> >>>>  obj-y += microcode_intel.o
> >>>>  obj-y += microcode.o
> >>>>  obj-y += mm.o x86_64/mm.o
> >>>> -obj-y += monitor.o
> >>>> +obj-$(CONFIG_HVM) += monitor.o
> >>>>  obj-y += mpparse.o
> >>>>  obj-y += nmi.o
> >>>>  obj-y += numa.o
> >>>> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
> >>>> index 4988903..09f7f8a 100644
> >>>> --- a/xen/include/asm-x86/monitor.h
> >>>> +++ b/xen/include/asm-x86/monitor.h
> >>>> @@ -99,10 +99,24 @@ static inline uint32_t
> >>>> arch_monitor_get_capabilities(struct domain *d)
> >>>>  int arch_monitor_domctl_event(struct domain *d,
> >>>>                                struct xen_domctl_monitor_op *mop);
> >>>>
> >>>> +#ifdef CONFIG_HVM
> >>>> +
> >>>>  int arch_monitor_init_domain(struct domain *d);
> >>>>
> >>>>  void arch_monitor_cleanup_domain(struct domain *d);
> >>>>
> >>>> +#else
> >>>> +
> >>>> +static inline int arch_monitor_init_domain(struct domain *d)
> >>>> +{
> >>>> +    return 0;
> >>>> +}
> >>>> +
> >>>> +static inline void arch_monitor_cleanup_domain(struct domain *d)
> >>>> +{}
> >>>> +
> >>>> +#endif
> >>>
> >>> Wouldn't the entire XEN_DOMCTL_VM_EVENT_OP_MONITOR case
> >>> in vm_event_domctl() then better be put in an #ifdef instead?
> >>
> >> I didn't do that because that was common to both ARM and x86.
> >>
> >> But now looking at the ARM counterpart, it is not supported either. When
> >> it is eventually supported on ARM, it will be likely to be dependent on
> >> CONFIG_HVM anyway. So I think I can put XEN_DOMCTL_VM_EVENT_OP_MONITOR
> >> under CONFIG_HVM.
> >>
> > 
> > It is not that it is not supported, it is that it's not (yet) needed.
> > I think it would be better to have ifdef CONFIG_HVM only in code
> > that's reached on x86 and not common ones.
> 
> FWIW, I agree with Tamas here.

Alright, in that case I will keep this patch, with the style issues
addressed.

Wei.

> 
> 
> Thanks,
> Razvan

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