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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

>>> --- 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.

Jan



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