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

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

>>>>> --- 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 hope you are aware, this is unlikely going to be printed as the code should 
> not be called.

ASSERT_UNREACHABLE() then?

Jan



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