| Message ID | cc4323b2-e074-f86f-eea0-9cd6a802bed8@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IOMMU: superpage support when not sharing pagetables | expand |
On 25/04/2022 09:32, Jan Beulich wrote: > As of 68a8aa5d7264 ("iommu: make map and unmap take a page count, > similar to flush") there's no need anymore to have a loop here. > > Suggested-by: Roger Pau Monné <roger.pau@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v3: New. > > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -308,11 +308,9 @@ int iommu_map(struct domain *d, dfn_t df > d->domain_id, dfn_x(dfn_add(dfn, i)), > mfn_x(mfn_add(mfn, i)), rc); > > - while ( i-- ) > - /* if statement to satisfy __must_check */ > - if ( iommu_call(hd->platform_ops, unmap_page, d, dfn_add(dfn, i), > - flush_flags) ) > - continue; > + /* while statement to satisfy __must_check */ > + while ( iommu_unmap(d, dfn, i, flush_flags) ) > + break; How can this possibly be correct? The map_page() calls are made one 4k page at a time, and this while loop is undoing every iteration, one 4k page at a time. Without this while loop, any failure after the first page will end up not being unmapped. ~Andrew
On 27.04.2022 15:16, Andrew Cooper wrote: > On 25/04/2022 09:32, Jan Beulich wrote: >> --- a/xen/drivers/passthrough/iommu.c >> +++ b/xen/drivers/passthrough/iommu.c >> @@ -308,11 +308,9 @@ int iommu_map(struct domain *d, dfn_t df >> d->domain_id, dfn_x(dfn_add(dfn, i)), >> mfn_x(mfn_add(mfn, i)), rc); >> >> - while ( i-- ) >> - /* if statement to satisfy __must_check */ >> - if ( iommu_call(hd->platform_ops, unmap_page, d, dfn_add(dfn, i), >> - flush_flags) ) >> - continue; >> + /* while statement to satisfy __must_check */ >> + while ( iommu_unmap(d, dfn, i, flush_flags) ) >> + break; > > How can this possibly be correct? > > The map_page() calls are made one 4k page at a time, and this while loop > is undoing every iteration, one 4k page at a time. > > Without this while loop, any failure after the first page will end up > not being unmapped. There's no real "while loop" here, it's effectively if ( iommu_unmap(d, dfn, i, flush_flags) ) /* nothing */; just that I wanted to avoid the empty body (but I could switch if that's preferred). Note that the 3rd argument to iommu_unmap() is i, not 1. But I have to admit that I also have trouble interpreting your last sentence - how would it matter if there was no code here at all? Or did you maybe mean "With ..." instead of "Without ..."? Jan
On Mon, Apr 25, 2022 at 10:32:10AM +0200, Jan Beulich wrote: > As of 68a8aa5d7264 ("iommu: make map and unmap take a page count, > similar to flush") there's no need anymore to have a loop here. > > Suggested-by: Roger Pau Monné <roger.pau@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> I wonder whether we should have a macro to ignore returns from __must_check attributed functions. Ie: #define IGNORE_RETURN(exp) while ( exp ) break; As to avoid confusion (and having to reason) whether the usage of while is correct. I always find it confusing to assert such loop expressions are correct. Thanks, Roger.
On 03.05.2022 12:25, Roger Pau Monné wrote: > On Mon, Apr 25, 2022 at 10:32:10AM +0200, Jan Beulich wrote: >> As of 68a8aa5d7264 ("iommu: make map and unmap take a page count, >> similar to flush") there's no need anymore to have a loop here. >> >> Suggested-by: Roger Pau Monné <roger.pau@citrix.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. > I wonder whether we should have a macro to ignore returns from > __must_check attributed functions. Ie: > > #define IGNORE_RETURN(exp) while ( exp ) break; > > As to avoid confusion (and having to reason) whether the usage of > while is correct. I always find it confusing to assert such loop > expressions are correct. I've been considering some form of wrapper macro (not specifically the one you suggest), but I'm of two minds: On one hand I agree it would help readers, but otoh I fear it may make it more attractive to actually override the __must_check (which really ought to be an exception). Jan
On Tue, May 03, 2022 at 04:37:29PM +0200, Jan Beulich wrote: > On 03.05.2022 12:25, Roger Pau Monné wrote: > > On Mon, Apr 25, 2022 at 10:32:10AM +0200, Jan Beulich wrote: > >> As of 68a8aa5d7264 ("iommu: make map and unmap take a page count, > >> similar to flush") there's no need anymore to have a loop here. > >> > >> Suggested-by: Roger Pau Monné <roger.pau@citrix.com> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks. > > > I wonder whether we should have a macro to ignore returns from > > __must_check attributed functions. Ie: > > > > #define IGNORE_RETURN(exp) while ( exp ) break; > > > > As to avoid confusion (and having to reason) whether the usage of > > while is correct. I always find it confusing to assert such loop > > expressions are correct. > > I've been considering some form of wrapper macro (not specifically > the one you suggest), but I'm of two minds: On one hand I agree it > would help readers, but otoh I fear it may make it more attractive > to actually override the __must_check (which really ought to be an > exception). Well, I think anyone reviewing the code would realize that the error is being ignored, and hence check that this is actually intended. Thanks, Roger.
--- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -308,11 +308,9 @@ int iommu_map(struct domain *d, dfn_t df d->domain_id, dfn_x(dfn_add(dfn, i)), mfn_x(mfn_add(mfn, i)), rc); - while ( i-- ) - /* if statement to satisfy __must_check */ - if ( iommu_call(hd->platform_ops, unmap_page, d, dfn_add(dfn, i), - flush_flags) ) - continue; + /* while statement to satisfy __must_check */ + while ( iommu_unmap(d, dfn, i, flush_flags) ) + break; if ( !is_hardware_domain(d) ) domain_crash(d);
As of 68a8aa5d7264 ("iommu: make map and unmap take a page count, similar to flush") there's no need anymore to have a loop here. Suggested-by: Roger Pau Monné <roger.pau@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v3: New.