Re: [Xen-devel] [RFC PATCH for-next 04/18] x86/mem_sharing: cleanup code in various locations

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Sep 25, 2019 at 10:15 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 25.09.2019 17:48, Tamas K Lengyel wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -1879,12 +1879,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> > long gla,
> >      if ( npfec.write_access && (p2mt == p2m_ram_shared) )
> >      {
> >          ASSERT(p2m_is_hostp2m(p2m));
> > -        sharing_enomem =
> > -            (mem_sharing_unshare_page(currd, gfn, 0) < 0);
> > +        sharing_enomem = mem_sharing_unshare_page(currd, gfn, 0);
>
> I guess the implication here is that the function can only return
> -ENOMEM? Not very forward compatible, but well. However, if you
> touch this already, shouldn't you at least make "sharing_enomem"
> bool?

Correct, there is a BUG_ON for every other rc value but ENOMEM. We
could turn it into a bool but I don't see a reason for it, perhaps
there will be another rc value in the future that we want to handle
gracefully.

>
> > @@ -1240,11 +1277,11 @@ int __mem_sharing_unshare_page(struct domain *d,
> >      mem_sharing_page_unlock(old_page);
> >      put_page_and_type(old_page);
> >
> > -private_page_found:
> > +private_page_found:
>
> Please also indent this label by (at least) one blank.

OK

>
> > @@ -57,34 +59,34 @@ struct page_sharing_info
> >      };
> >  };
> >
> > -#define sharing_supported(_d) \
> > -    (is_hvm_domain(_d) && paging_mode_hap(_d))
> > -
> >  unsigned int mem_sharing_get_nr_saved_mfns(void);
> >  unsigned int mem_sharing_get_nr_shared_mfns(void);
> >
> >  #define MEM_SHARING_DESTROY_GFN       (1<<1)
> >  /* Only fails with -ENOMEM. Enforce it with a BUG_ON wrapper. */
> >  int __mem_sharing_unshare_page(struct domain *d,
> > -                             unsigned long gfn,
> > -                             uint16_t flags);
> > -static inline int mem_sharing_unshare_page(struct domain *d,
> > -                                           unsigned long gfn,
> > -                                           uint16_t flags)
> > +                               unsigned long gfn,
> > +                               uint16_t flags);
> > +
> > +static inline
> > +int mem_sharing_unshare_page(struct domain *d,
> > +                             unsigned long gfn,
> > +                             uint16_t flags)
> >  {
> >      int rc = __mem_sharing_unshare_page(d, gfn, flags);
> >      BUG_ON( rc && (rc != -ENOMEM) );
>
> Would you mind also dropping the stray blanks here?
>

Sure.

Thanks,
Tamas

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