Re: [Xen-devel] [v4][PATCH 03/19] xen/vtd: create RMRR mapping

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.06.15 at 03:11, <tiejun.chen@xxxxxxxxx> wrote:
> On 2015/6/23 18:12, Jan Beulich wrote:
>>>>> On 23.06.15 at 11:57, <tiejun.chen@xxxxxxxxx> wrote:
>>> --- a/xen/drivers/passthrough/vtd/iommu.c
>>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>>> @@ -1839,7 +1839,7 @@ static int rmrr_identity_mapping(struct domain *d, 
>>> bool_t map,
>>>
>>>               while ( base_pfn < end_pfn )
>>>               {
>>> -                if ( intel_iommu_unmap_page(d, base_pfn) )
>>> +                if ( guest_physmap_remove_page(d, base_pfn, base_pfn, 0) )
> 
> Yeah, I also thought this may bring some confusions in this context.
> 
>>>                       ret = -ENXIO;
>>>                   base_pfn++;
>>>               }
>>> @@ -1855,8 +1855,7 @@ static int rmrr_identity_mapping(struct domain *d, 
>>> bool_t map,
>>>
>>>       while ( base_pfn < end_pfn )
>>>       {
>>> -        int err = intel_iommu_map_page(d, base_pfn, base_pfn,
>>> -                                       IOMMUF_readable|IOMMUF_writable);
>>> +        int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw);
>>
>> Shouldn't the two continue to be the inverse of one another?
> 
> Initially, instead of using guest_physmap_remove_page, I was trying to 
> introduce a new, clear_identity_p2m_entry, which can wrapper 
> p2m_remove_page().
> 
> But Tim just thought we'd better avoid duplicating code,
> 
> http://lists.xenproject.org/archives/html/xen-devel/2015-06/msg02970.html 
> 
>> Maybe guest_physmap_remove_page() does what you want,
> 
> Note actually we just need p2m_remove_page() to unmap these mapping on 
> both ept and vt-d sides, and guest_physmap_remove_page is really a 
> wrapper of p2m_remove_page().

And I agree with Tim regarding the desire to avoid code duplication.
Yet that's no reason to do it asymmetrically:
clear_identity_p2m_entry() could still be an inline (or macro) wrapper
around guest_physmap_remove_page(). That way, apart from making
the code above look nicer, if the latter needs extending in the future
for some reason, simply converting the wrapper to a real function is
possible without needing to touch the call site(s).

This would need to go into patch 2; I wonder whether folding that
and this one wouldn't be warranted, avoiding the former adding
(at that point) dead code.

Jan


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