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

From: Julien Grall <julien.grall@arm.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	"andrii_anisov@epam.com" <andrii_anisov@epam.com>,
	WeiLiu <wl@xen.org>,
	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>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v3 07/14] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call
Date: Tue, 4 Jun 2019 17:22:29 +0100	[thread overview]
Message-ID: <b85178eb-05ad-da33-3f97-9fdcdf65727b@arm.com> (raw)
In-Reply-To: <5CF698EB0200007800235319@prv1-mh.provo.novell.com>

Hi Jan,

On 6/4/19 5:14 PM, Jan Beulich wrote:
>>>> On 03.06.19 at 18:03, <julien.grall@arm.com> wrote:
>> While Arm never had a M2P, the implementation of mfn_to_gmfn is pretty
>> bogus as we directly return the MFN passed in parameter.
>>
>> Thankfully, the use of mfn_to_gmfn is pretty limited on Arm today. There
>> are only 3 callers:
>>      - iommu_hwdom_init: mfn_to_gmfn is used for creating IOMMU
>>      page-tables when the P2M is not shared with the IOMMU. No issues so
>>      far as Arm does not yet support non-shared P2M case.
>>      - memory_exchange: Arm cannot not use it because steal_page is not
>>      implemented.
>>      - getdomaininfo: Toolstack may map the shared page. It looks like
>>      this is mostly used for mapping the P2M of PV guest. Therefore the
>>      issue might be minor.
>>
>> Implementing the M2P on Arm is not planned. The M2P would require
>> significant
>> amount of VA address (very tough on 32-bit) that can hardly be justified with
>> the current use of mfn_to_gmfn.
>>      - iommu_hwdom_init: mfn_to_gmfn is used because the creating of the
>>      IOMMU page-tables is delayed until the first device is assigned.
>>      In the embedded case, we will known in most of the times what
>>      devices are assigned during the domain creation. So it is possible
>>      to take to enable the IOMMU from start. See [1] for the patch.
>>      - memory_exchange: This does not work and I haven't seen any
>>      request for it so far.
>>      - getdomaininfo: The structure on Arm does not seem to contain a lot
>>      of useful information on Arm. It is unclear whether we want to
>>      allow the toolstack mapping it on Arm.
>>
>> This patch introduces a config option HAS_M2P to tell whether an
>> architecture implements the M2P.
>>      - iommu_hwdom_init: For now, we require the M2P support when the IOMMU
>>      is not sharing the P2M.
>>      - memory_exchange: The hypercall is marked as not supported when the
>>      M2P does not exist.
> 
> But where's the connection between there being M2P and the
> availability of this operation? I think I've suggested so before:
> Why don't we simply disable this operation for translated
> guests (in an independent patch)?

And I answered that mfn_to_gmfn() is used in the function. I really 
don't want to implement the macro on Arm (even if it is dummy).

You haven't answered back to that comment and I assumed this was fine 
with you...

> 
>>      - getdomaininfo: A new helper is introduced to wrap the call to
>>      mfn_to_gfn/mfn_to_gmfn. For Arm, a fixed value will be provided that will
>>      fail on mapping if used.
> 
> This reads slightly stale now.

ok.

> 
>> --- 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 )
>>           {
>> @@ -221,6 +222,11 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>>           if ( rc )
>>               printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
>>                      d->domain_id, rc);
>> +#else
>> +        ASSERT_UNREACHABLE();
>> +        rc = -EOPNOTSUPP;
>> +#endif
>> +
>>       }
> 
> Please don't add a blank line ahead of a closing brace.

ok.

Cheers,

-- 
Julien Grall

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