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 Jan,

On 10/05/2019 14:32, Jan Beulich wrote:
> > > > On 10.05.19 at 15:22, <julien.grall@xxxxxxx> wrote:
> > On 10/05/2019 13:31, Jan Beulich wrote:
> > > > > > On 07.05.19 at 17:14, <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));
> > > 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.
> That's not helpful - how would a consumer know to avoid the mapping
> attempt in the first place?
I can't see any issue with the consumer to try to map it. Ok, you will waste a 
couple of cycles, but that should be pretty rare.
The point here, we keep within the hypervisor the idea of what's valid or 
invalid. This allows us more flexibility on the value here (imagine we decide to 
change the value of GFN_INVALID in the future...).
> > > > --- 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.
> I disagree.
You disagree because...?

I hope you are aware, this is unlikely going to be printed as the code should 
not be called. If it ever happens, it is easier for a user to grep the code for 
the message rather than having to add some to find out where the -EOPNOTSUPP is 
coming from.
Cheers,

--
Julien Grall

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