gnttab: misc fixes

[PATCH 2/3] gnttab: bypass IOMMU (un)mapping when a domain is (un)mapping its own grant

Posted by Jan Beulich 4 years, 11 months ago

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);
     if ( need_iommu )
     {
         unsigned int kind;
@@ -1493,7 +1493,7 @@ unmap_common(
     if ( put_handle )
         put_maptrack_handle(lgt, op->handle);
 
-    if ( rc == GNTST_okay && gnttab_need_iommu_mapping(ld) )
+    if ( rc == GNTST_okay && ld != rd && gnttab_need_iommu_mapping(ld) )
     {
         unsigned int kind;
         int err = 0;

Re: [PATCH 2/3] gnttab: bypass IOMMU (un)mapping when a domain is (un)mapping its own grant

Posted by Julien Grall 4 years, 11 months ago

Hi Jan,

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?

Cheers,

-- 
Julien Grall

Re: [PATCH 2/3] gnttab: bypass IOMMU (un)mapping when a domain is (un)mapping its own grant

Posted by Jan Beulich 4 years, 11 months ago

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?

Jan

Re: [PATCH 2/3] gnttab: bypass IOMMU (un)mapping when a domain is (un)mapping its own grant

Posted by Julien Grall 4 years, 11 months ago

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.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/3] gnttab: bypass IOMMU (un)mapping when a domain is (un)mapping its own grant

Posted by Jan Beulich 4 years, 11 months ago

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?

Jan

Re: [PATCH 2/3] gnttab: bypass IOMMU (un)mapping when a domain is (un)mapping its own grant

Posted by Julien Grall 4 years, 11 months ago

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