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

From: "Jan Beulich" <JBeulich@suse.com>
To: Julien Grall <julien.grall@arm.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	oleksandr_tyshchenko@epam.com,
	xen-devel <xen-devel@lists.xenproject.org>,
	"andrii_anisov@epam.com" <andrii_anisov@epam.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH 09/14] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call
Date: Fri, 10 May 2019 07:45:03 -0600	[thread overview]
Message-ID: <5CD5805F020000780022D939@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <3a359c40-ec6e-db96-b86e-17829c8ccf88@arm.com>

>>> On 10.05.19 at 15:41, <julien.grall@arm.com> wrote:
> On 10/05/2019 14:32, Jan Beulich wrote:
>>>>> On 10.05.19 at 15:22, <julien.grall@arm.com> wrote:
>>> On 10/05/2019 13:31, Jan Beulich wrote:
>>>>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel