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:45, Jan Beulich wrote:
> > > > On 10.05.19 at 15:41, <julien.grall@xxxxxxx> wrote:
> > 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 attempt may result in a log message spit out.
I still can't see the issue here. This is nothing different than the frame were 
not setup beforehand.
> > 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...).
> Exactly, and hence INVALID_GFN should not become visible to the
> outside. Hence my request to use an all-ones value here.
It is only visible if you put an exact value in the documentation. Your 
suggestion is to put a exactly value and I would rather not see that.
> > > > > > --- 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...?
> Because of what I've said in my initial reply (still quoted above).
I still don't see the problem of unconditional log message. It is not really the 
first place we have that.
> > I hope you are aware, this is unlikely going to be printed as the code should
> > not be called.
> ASSERT_UNREACHABLE() then?
And still avoiding the printk?

Cheers,

--
Julien Grall

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