From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
George Dunlap <george.dunlap@citrix.com>,
Ian Jackson <iwj@xenproject.org>,
Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
Rahul Singh <Rahul.Singh@arm.com>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 2/3] gnttab: bypass IOMMU (un)mapping when a domain is (un)mapping its own grant
Date: Wed, 17 Feb 2021 14:27:22 +0000 [thread overview]
Message-ID: <c9e48ef3-7de3-f6c8-7976-a574b7b8be90@xen.org> (raw)
In-Reply-To: <ef8991ad-ba9e-c4f0-e4cc-fb4655608549@suse.com>
Hi Jan,
On 17/02/2021 13:16, Jan Beulich wrote:
> On 17.02.2021 12:41, Julien Grall wrote:
>> Hi Jan,
>>
>> On 17/02/2021 11:38, Jan Beulich wrote:
>>> On 17.02.2021 12:03, Julien Grall wrote:
>>>> On 17/02/2021 10:46, Jan Beulich wrote:
>>>>> Mappings for a domain's own pages should already be present in the
>>>>> IOMMU. While installing the same mapping again is merely redundant (and
>>>>> inefficient), removing the mapping when the grant mapping gets removed
>>>>> is outright wrong in this case: The mapping was there before the map, so
>>>>> should remain in place after unmapping.
>>>>>
>>>>> This affects
>>>>> - Arm Dom0 in the direct mapped case,
>>>>> - x86 PV Dom0 in the "iommu=dom0-strict" / "dom0-iommu=strict" cases,
>>>>> - all x86 PV DomU-s, including driver domains.
>>>>>
>>>>> Reported-by: Rahul Singh <Rahul.Singh@arm.com>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>
>>>>> --- a/xen/common/grant_table.c
>>>>> +++ b/xen/common/grant_table.c
>>>>> @@ -1243,7 +1243,7 @@ map_grant_ref(
>>>>> goto undo_out;
>>>>> }
>>>>>
>>>>> - need_iommu = gnttab_need_iommu_mapping(ld);
>>>>> + need_iommu = ld != rd && gnttab_need_iommu_mapping(ld);
>>>>
>>>> AFAICT, the owner of the page may not always be rd. So do we want to
>>>> check against the owner instead?
>>>
>>> For the DomIO case - specifically not. And the DomCOW case can't
>>> happen when an IOMMU is in use. Did I overlook any other cases
>>> where the page may not be owned by rd?
>>
>> For the current code, it looks like not. But it feels to me this code is
>> fragile as we are assuming that other cases should never happen.
>>
>> I think it would be worth explaining in a comment and the commit message
>> why check rd rather than the page owner is sufficient.
>
> Well, I've added
>
> /*
> * This is deliberately not checking the page's owner: get_paged_frame()
> * explicitly rejects foreign pages, and all success paths above yield
> * either owner == rd or owner == dom_io (the dom_cow case is irrelevant
> * as mem-sharing and IOMMU use are incompatible). The dom_io case would
> * need checking separately if we compared against owner here.
> */
>
> to map_grant_ref(), and a reference to this comment to both
> unmap_common() and the commit message. Will this do?
LGTM. With that, you can add:
Reviewed-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
next prev parent reply other threads:[~2021-02-17 14:27 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-17 10:42 [PATCH 0/3] gnttab: misc fixes Jan Beulich
2021-02-17 10:46 ` [PATCH 1/3] gnttab: never permit mapping transitive grants Jan Beulich
2021-02-18 10:25 ` Julien Grall
2021-02-18 11:31 ` Jan Beulich
2021-02-17 10:46 ` [PATCH 2/3] gnttab: bypass IOMMU (un)mapping when a domain is (un)mapping its own grant Jan Beulich
2021-02-17 11:03 ` Julien Grall
2021-02-17 11:38 ` Jan Beulich
2021-02-17 11:41 ` Julien Grall
2021-02-17 13:16 ` Jan Beulich
2021-02-17 14:27 ` Julien Grall [this message]
2021-02-17 10:46 ` [PATCH 3/3] gnttab: GTF_sub_page is a v2-only flag Jan Beulich
2021-02-18 14:22 ` Andrew Cooper
2021-02-18 15:02 ` Jan Beulich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c9e48ef3-7de3-f6c8-7976-a574b7b8be90@xen.org \
--to=julien@xen.org \
--cc=Rahul.Singh@arm.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=iwj@xenproject.org \
--cc=jbeulich@suse.com \
--cc=sstabellini@kernel.org \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).