Re: [Xen-devel] [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Tue, May 10, 2016 at 9:44 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 06.05.16 at 10:54, <quan.xu@xxxxxxxxx> wrote:
>> When IOMMU mapping is failed, we issue a best effort rollback, stopping
>> IOMMU mapping, unmapping the previous IOMMU maps and then reporting the
>> error up to the call trees. When rollback is not feasible (in early
>> initialization phase or trade-off of complexity) for the hardware domain,
>> we do things on a best effort basis, only throwing out an error message.
>>
>> IOMMU unmapping should perhaps continue despite an error, in an attempt
>> to do best effort cleanup.
>>
>> Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
>>
>> CC: Keir Fraser <keir@xxxxxxx>
>> CC: Jan Beulich <jbeulich@xxxxxxxx>
>> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
>> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
>> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>> ---
>
> Somewhere here I continue to miss a summary on what has changed
> compared to the previous version. For review especially of larger
> patches (where preferably one wouldn't want to re-review the entire
> thing) this is more than just a nice-to-have.
>
>> @@ -812,17 +813,22 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long 
>> gfn, mfn_t mfn,
>>      rc = atomic_write_ept_entry(ept_entry, new_entry, target);
>>      if ( unlikely(rc) )
>>          old_entry.epte = 0;
>> -    else if ( p2mt != p2m_invalid &&
>> -              (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
>> -        /* Track the highest gfn for which we have ever had a valid mapping 
>> */
>> -        p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
>> +    else
>> +    {
>> +        entry_written = 1;
>> +
>> +        if ( p2mt != p2m_invalid &&
>> +             (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
>> +            /* Track the highest gfn for which we have ever had a valid 
>> mapping */
>> +            p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
>> +    }
>>
>>  out:
>>      if ( needs_sync )
>>          ept_sync_domain(p2m);
>>
>>      /* For host p2m, may need to change VT-d page table.*/
>> -    if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) &&
>> +    if ( entry_written && p2m_is_hostp2m(p2m) && need_iommu(d) &&
>>           need_modify_vtd_table )
>>      {
>
> I'd prefer this conditional to remain untouched, but I'll leave the
> decision to the maintainers of the file.

Any particular reason you think it would be better untouched?

I asked for it to be changed to "entry_written", because it seemed to
me that's what was actually wanted (i.e., you're checking whether rc
== 0 to determine whether the entry was written or not).  At the
moment the checks will be identical, but if someone changed something
later, rc might be non-zero even though the entry had been written, in
which case (I think) you'd want the iommu update to happen.

It's not that big a deal to me, but I do prefer it this way (unless
I've misunderstood something).

>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -638,13 +638,20 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long 
>> gfn, unsigned long mfn,
>>      mfn_t mfn_return;
>>      p2m_type_t t;
>>      p2m_access_t a;
>> +    int rc = 0, ret;
>>
>>      if ( !paging_mode_translate(p2m->domain) )
>>      {
>>          if ( need_iommu(p2m->domain) )
>>              for ( i = 0; i < (1 << page_order); i++ )
>> -                iommu_unmap_page(p2m->domain, mfn + i);
>> -        return 0;
>> +            {
>> +               ret = iommu_unmap_page(p2m->domain, mfn + i);
>> +
>> +               if ( !rc )
>> +                   rc = ret;
>> +            }
>> +
>> +        return rc;
>>      }
>
> In code like this, btw., restricting the scope of "ret" to the innermost
> block would help future readers see immediately that the value of
> "ret" is of no further interest outside of that block.

I wouldn't ask for re-send just for this, but...

> Having reached the end of the patch, I'm missing the __must_check
> additions that you said you would do in this new iteration. Is there
> any reason for their absence? Did I overlook something?

If it's going to be re-sent anyway, moving the ret declaration inside
the loop might as well be done.

Other than that, it looks good to me, thanks.

 -George

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