Re: [Xen-devel] [PATCH 09/14] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 10/05/2019 13:31, Jan Beulich wrote:
> > > > On 07.05.19 at 17:14, <julien.grall@xxxxxxx> wrote:
> > This patch introduces a config option HAS_M2P to tell whether an
> > architecture implements the M2P.
> >      - iommu_hwdom_init: For now, we require the M2P support when the IOMMU
> >      is not sharing the P2M.
> >      - memory_exchange: The hypercall is marked as not supported when the
> >      M2P does not exist.
> Was it you or someone else to suggest it be restricted to non-translated
> guests in the first place? I'd prefer this over the #ifdef-ary you add.
I never suggested that as I have no idea who is using it on x86.

But then, we would still need to implement mfn_to_gfn on Arm to make it compile. 
I really want to avoid such macro on Arm.
> >      - getdomaininfo: A new helper is introduced to wrap the call to
> >      mfn_to_gfn/mfn_to_gmfn. For Arm, it returns an invalid GFN so the mapping
> >      will fail.
> There's no use of mfn_to_gmfn() in either of the wrappers, so why
> mention this to-be-removed one?
Because code has been changed over the revision and I forgot to update the 
commit message.
> > --- 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));
> What is the intended behavior on 32-bit Arm here? Do you really
> mean to return a value with 32 bits of ones (instead of 64 bits of
> them) in this 64-bit field?
It does not matter as long as the GFN is invalid so it can't be mapped 
afterwards. The exact value is not documented in the header to avoid any assumption.
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -188,9 +188,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 )
> >           {
> > @@ -217,6 +218,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 = -EOPNOTSUPP;
> > +#endif
> >   
> >           if ( rc )
> >               printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
> Would you mind extending the scope of the #ifdef beyond this printk()?
> It seems pretty pointless to me to unconditionally emit a log message
> here.
Well, it at least tell you the function can't work. So I think it is still makes 
sense to have it.
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -118,6 +118,10 @@ struct xen_domctl_getdomaininfo {
> >       uint64_aligned_t outstanding_pages;
> >       uint64_aligned_t shr_pages;
> >       uint64_aligned_t paged_pages;
> > +    /*
> > +     * GFN of shared_info struct. Some architectures (e.g Arm) may not
> > +     * provide a valid GFN.
> > +     */
> Along the lines of what I've said above, I think you want to spell out
> here what the value is going to be in this case.
Spelling out the exact value gives us less freedom. What matters here is the GFN 
return can't be mapped.
> > --- a/xen/include/xen/domain.h
> > +++ b/xen/include/xen/domain.h
> > @@ -118,4 +118,12 @@ struct vnuma_info {
> >   
> >   void vnuma_destroy(struct vnuma_info *vnuma);
> >   
> > +#ifdef CONFIG_HAS_M2P
> > +#define domain_shared_info_gfn(d) ({                            \
> > +    const struct domain *d_ = (d);                              \
> > +                                                                \
> > +    mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));       \
> > +})
> > +#endif
> And an inline function doesn't work here?
With enough time to rework the headers maybe. At the moment, no because 
mfn_to_gfn is implemented in p2m.h which depends on domain.h for the definition 
of struct domain d:
In file included from /home/julieng/works/xen/xen/include/xen/sched.h:11:0,
                 from x86_64/asm-offsets.c:9:
/home/julieng/works/xen/xen/include/xen/domain.h: In function 
‘domain_shared_info_gfn’:
/home/julieng/works/xen/xen/include/xen/domain.h:124:12: error: implicit 
declaration of function ‘mfn_to_gfn’ [-Werror=implicit-function-declaration]
     return mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info)));
            ^~~~~~~~~~
/home/julieng/works/xen/xen/include/xen/domain.h:124:5: error: nested extern 
declaration of ‘mfn_to_gfn’ [-Werror=nested-externs]
     return mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info)));
     ^~~~~~
In file included from /home/julieng/works/xen/xen/include/asm/current.h:12:0,
                 from /home/julieng/works/xen/xen/include/asm/smp.h:10,
                 from /home/julieng/works/xen/xen/include/xen/smp.h:4,
                 from /home/julieng/works/xen/xen/include/asm/processor.h:10,
                 from /home/julieng/works/xen/xen/include/asm/system.h:6,
                 from /home/julieng/works/xen/xen/include/xen/spinlock.h:4,
                 from /home/julieng/works/xen/xen/include/xen/sched.h:6,
                 from x86_64/asm-offsets.c:9:
/home/julieng/works/xen/xen/include/xen/domain.h:124:46: error: dereferencing 
pointer to incomplete type ‘struct domain’
     return mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info)));
                                              ^
/home/julieng/works/xen/xen/include/asm/page.h:265:61: note: in definition of 
macro ‘virt_to_maddr’
 #define virt_to_maddr(va)   __virt_to_maddr((unsigned long)(va))
                                                             ^~
/home/julieng/works/xen/xen/include/xen/domain.h:124:31: note: in expansion of 
macro ‘__virt_to_mfn’
     return mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info)));
                               ^~~~~~~~~~~~~
cc1: all warnings being treated as errors
Makefile:217: recipe for target 'asm-offsets.s' failed
make[2]: *** [asm-offsets.s] Error 1
make[2]: Leaving directory '/home/julieng/works/xen/xen/arch/x86'
Makefile:136: recipe for target '/home/julieng/works/xen/xen/xen' failed
make[1]: *** [/home/julieng/works/xen/xen/xen] Error 2
make[1]: Leaving directory '/home/julieng/works/xen/xen'
Makefile:45: recipe for target 'build' failed
make: *** [build] Error 2

Cheers,

--
Julien Grall

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