Re: [Xen-devel] [PATCH 1/2] IOMMU/MMU: Adjust top level functions for VT-d Device-TLB flush error.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.03.16 at 04:19, <quan.xu@xxxxxxxxx> wrote:
> On March 17, 2016 8:34pm, George Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote:
>> On Thu, Mar 17, 2016 at 12:30 PM, George Dunlap
>> <George.Dunlap@xxxxxxxxxxxxx> wrote:
>> > On Thu, Mar 17, 2016 at 6:54 AM, Quan Xu <quan.xu@xxxxxxxxx> wrote:
>> >> --- a/xen/arch/x86/mm/p2m-ept.c
>> >> +++ b/xen/arch/x86/mm/p2m-ept.c
>> >> @@ -830,7 +830,15 @@ out:
>> >>          {
>> >>              if ( iommu_flags )
>> >>                  for ( i = 0; i < (1 << order); i++ )
>> >> -                    iommu_map_page(d, gfn + i, mfn_x(mfn) + i,
>> iommu_flags);
>> >> +                {
>> >> +                    rc = iommu_map_page(d, gfn + i, mfn_x(mfn) + i,
>> iommu_flags);
>> >> +                    if ( rc )
>> >> +                    {
>> >> +                        while ( i-- > 0 )
>> >> +                            iommu_unmap_page(d, gfn + i);
>> >
>> > This won't unmap gfn+0 (since it will break out when i == 0 without
>> > calling unmap).
>> 
>> Oh, no it won't, because the decrement is postfix.
>> 
>> For us mere mortals, I'd appreciate a comment here like this:
>> 
>> /* Postfix operator means we will call unmap with i == 0 */
>> 
> Agreed.
> For these 2 points, to summarize:
>    - adding "unlikely()" to the if() condition, e.g. if ( unlikely(rc) )
>    - adding a comment:
>         /* Postfix operator means we will call unmap with i == 0 */

To be honest, I'm opposed to the addition of such comments.
See also the parallel discussion rooted at
http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg01779.html

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel