Re: [Xen-devel] [PATCH v3 4/4] x86/mem_sharing: compile mem_sharing subsystem only when kconfig is enabled

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Apr 29, 2019 at 9:32 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> On 26.04.19 at 19:21, <tamas@xxxxxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/Kconfig
> > +++ b/xen/arch/x86/Kconfig
> > @@ -17,7 +17,6 @@ config X86
> >       select HAS_KEXEC
> >       select MEM_ACCESS_ALWAYS_ON
> >       select HAS_MEM_PAGING
> > -     select HAS_MEM_SHARING
> >       select HAS_NS16550
> >       select HAS_PASSTHROUGH
> >       select HAS_PCI
> > @@ -198,6 +197,13 @@ config PV_SHIM_EXCLUSIVE
> >         firmware, and will not function correctly in other scenarios.
> >
> >         If unsure, say N.
> > +
> > +config MEM_SHARING
> > +    bool
> > +    default n
> > +    depends on HVM
> > +    prompt "Xen memory sharing support" if EXPERT = "y"
> > +
>
> As per all the context above, please use proper (tab) indentation.
> Also please omit the pointless "default n". And then the "bool"
> and "prompt ..." can (and hence should) be combined into a
> single line.

Sure.

>
> I also wonder whether this shouldn't be in common/Kconfig, but
> then again it can be moved there if Arm would ever gain
> mem-sharing support.

It is currently only supported for x86 HVM guests. There are no plans
for adding ARM support. If and when that happens, it could be moved to
common. I don't expect that will ever happen.

>
> > @@ -98,4 +100,33 @@ void mem_sharing_init(void);
> >   */
> >  int relinquish_shared_pages(struct domain *d);
> >
> > +#else
> > +
> > +static inline unsigned int mem_sharing_get_nr_saved_mfns(void)
> > +{
> > +    return 0;
> > +}
> > +static inline unsigned int mem_sharing_get_nr_shared_mfns(void)
> > +{
> > +    return 0;
> > +}
>
> I follow you for these.
>
> > +static inline int mem_sharing_unshare_page(struct domain *d,
> > +                                           unsigned long gfn,
> > +                                           uint16_t flags)
> > +{
> > +    return 0;
> > +}
>
> But shouldn't this one (just as an example, there may be more
> below) return -EOPNOTSUPP?

It really doesn't matter. No caller ever reaches this function when
mem_sharing is compiled out since it's gated on the page type being
p2m_ram_shared. You can't set that page type when the memop/domctl to
set it is gone.

>
> And in general - if these inline functions are needed, shouldn't
> all of them (or at least some) gain ASSERT_UNREACHABLE(), as we
> do elsewhere?

Yes, since unshare_page is unreachable adding that assert would be
appropriate. It would probably be appropriate for the others too
(except get_nr_saved/shared_mfns).

Tamas

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