Re: [Xen-devel] [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall <julien.grall@xxxxxxx>]

Hi Jan,

On 13/03/2019 15:20, Jan Beulich wrote:
> > > > On 18.02.19 at 12:35, <julien.grall@xxxxxxx> wrote:
> > --- a/xen/common/domctl.c
> > +++ b/xen/common/domctl.c
> > @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct 
> > xen_domctl_getdomaininfo *info)
> >       info->outstanding_pages = d->outstanding_pages;
> >       info->shr_pages         = atomic_read(&d->shr_pages);
> >       info->paged_pages       = atomic_read(&d->paged_pages);
> > -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
> > +    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));
> I think this change wants to be accompanied by a warning attached
> to the field declaration in the public header.
Make sense.

> But I'd also like to have the tool stack maintainers' view on making
> this field effectively unusable for Arm.
The value in shared_info_frame was plain wrong since the creation of Xen Arm. So 
this is just making the error more obvious. I don't expect any user of it on Arm.
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -510,6 +510,7 @@ static bool propagate_node(unsigned int xmf, unsigned int 
> > *memflags)
> >       return true;
> >   }
> >   
> > +#ifdef CONFIG_M2P
> >   static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
> >   {
> >       struct xen_memory_exchange exch;
> > @@ -802,6 +803,7 @@ static long 
> > memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
> >           rc = -EFAULT;
> >       return rc;
> >   }
> > +#endif
> Please can you move the #ifdef inside the function body, add #else
> to return -EOPNOTSUPP, and ...
Sure.

> > @@ -1233,12 +1235,14 @@ long do_memory_op(unsigned long cmd, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >   
> >           break;
> >   
> > +#ifdef CONFIG_M2P
> >       case XENMEM_exchange:
> >           if ( unlikely(start_extent) )
> >               return -EINVAL;
> >   
> >           rc = memory_exchange(guest_handle_cast(arg, xen_memory_exchange_t));
> >           break;
> > +#endif
> ... avoid the extra #ifdef-ary here altogether?
>
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -186,9 +186,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
> >       hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
> >       if ( need_iommu_pt_sync(d) )
> >       {
> > +        int rc = 0;
> > +#ifdef CONFIG_HAS_M2P
> >           struct page_info *page;
> >           unsigned int i = 0, flush_flags = 0;
> > -        int rc = 0;
> >   
> >           page_list_for_each ( page, &d->page_list )
> >           {
> > @@ -215,6 +216,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
> >           /* Use while-break to avoid compiler warning */
> >           while ( iommu_iotlb_flush_all(d, flush_flags) )
> >               break;
> > +#else
> > +        rc = -ENOSYS;
> -EOPNOTSUPP please.
Sure.

> > --- a/xen/include/asm-arm/domain.h
> > +++ b/xen/include/asm-arm/domain.h
> > @@ -267,6 +267,11 @@ static inline void free_vcpu_guest_context(struct
> > vcpu_guest_context *vgc)
> >   
> >   static inline void arch_vcpu_block(struct vcpu *v) {}
> >   
> > +static inline gfn_t domain_shared_info_gfn(struct domain *d)
> > +{
> > +    return INVALID_GFN;
> > +}
> > +
> >   #endif /* __ASM_DOMAIN_H__ */
> >   
> >   /*
> > diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> > index d1bfc82f57..00d8b09794 100644
> > --- a/xen/include/xen/domain.h
> > +++ b/xen/include/xen/domain.h
> > @@ -118,4 +118,13 @@ struct vnuma_info {
> >   
> >   void vnuma_destroy(struct vnuma_info *vnuma);
> >   
> > +#ifdef CONFIG_HAS_M2P
> > +#define domain_shared_info_gfn(d_) ({                           \
> > +    struct domain *d = (d_);                                    \
> const
>
> Also the naming needs to be the other way around: The macro
> parameter should be named d (all instances will get substituted
> anyway, and hence name collisions are impossible) while the
> local variable should be named d_, to avoid collisions with names
> in the outer scopes.
I will do.

> > +                                                                \
> I won't insist, but especially small macros are often an exception
> to the blank-line-between-declarations-and-statements rule. IOW
> I'd prefer if you dropped this line, unless others correct my view
> here.
The newline is here for clarity. I am not keen to drop it unless someone else 
agrees with the removal.
Cheers,

--
Julien Grall

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