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 3:45 PM, George Dunlap
<George.Dunlap@xxxxxxxxxxxxx> wrote:
> 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).

See the discussion on patch 8 regarding why I now think Jan is probably right.

 -George

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