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

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxx>
> > > > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > > >
> > > > > --- 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@xxxxxxxxxx>

Cheers,

--
Julien Grall