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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.05.16 at 16:45, <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:
>>> @@ -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?

IMO it's misleading here, and appropriate only in the other place
(where the altp2m-s get updated).

> 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...

I agree, and I meant to express this with the "btw".

Jan

>> 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