Re: [v2][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 24/07/14 10:03, Chen, Tiejun wrote:
> On 2014/7/24 16:57, Andrew Cooper wrote:
>> On 24/07/14 09:50, Tiejun Chen wrote:
>>> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
>>> So rmrr_identity_mapping() never create RMRR mapping but in some
>>> cases like some GFX drivers it still need to access RMRR.
>>>
>>> Here we will create those RMRR mappings even in shared EPT case.
>>>
>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>> ---
>>>   xen/drivers/passthrough/vtd/iommu.c | 18 ++++++++++++++++--
>>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>>
>>> v2:
>>>
>>> * Fix coding style.
>>> * Still need to abide intel_iommu_map_page(), so we should do nothing
>>>    if dom0 and iommu supports pass thru.
>>>
>>> diff --git a/xen/drivers/passthrough/vtd/iommu.c
>>> b/xen/drivers/passthrough/vtd/iommu.c
>>> index 042b882..aca79db 100644
>>> --- a/xen/drivers/passthrough/vtd/iommu.c
>>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>>> @@ -41,6 +41,7 @@
>>>   #include "extern.h"
>>>   #include "vtd.h"
>>>   #include "../ats.h"
>>> +#include "../../../arch/x86/mm/mm-locks.h"
>>
>> <asm/mm/mm-locks.h>
>
> iommu.c:38:29: fatal error: asm/mm/mm-locks.h: No such file or directory
>  #include <asm/mm/mm-locks.h>
>                              ^
> compilation terminated.
>
> Tiejun

Hmm - my mistake.  The lack of easy access to this header file goes to
emphasise that it is private to arch/x86/mm, and there are indeed no
current users outside of that part of the tree.

Would it not be better have some public p2m_set_identity() function in
p2m.c ?

~Andrew

>
>>
>> ~Andrew
>>
>>>
>>>   struct mapped_rmrr {
>>>       struct list_head list;
>>> @@ -1842,6 +1843,7 @@ static int rmrr_identity_mapping(struct domain
>>> *d,
>>>       unsigned long base_pfn, end_pfn;
>>>       struct mapped_rmrr *mrmrr;
>>>       struct hvm_iommu *hd = domain_hvm_iommu(d);
>>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>
>>>       ASSERT(spin_is_locked(&pcidevs_lock));
>>>       ASSERT(rmrr->base_address < rmrr->end_address);
>>> @@ -1867,8 +1869,20 @@ static int rmrr_identity_mapping(struct
>>> domain *d,
>>>
>>>       while ( base_pfn < end_pfn )
>>>       {
>>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>> -                                  IOMMUF_readable|IOMMUF_writable) )
>>> +        if ( iommu_use_hap_pt(d) && (!iommu_passthrough ||
>>> +                                     !is_hardware_domain(d)) )
>>> +        {
>>> +            p2m_lock(p2m);
>>> +            if ( p2m_set_entry(p2m, base_pfn, _mfn(base_pfn),
>>> PAGE_ORDER_4K,
>>> +                               p2m_mmio_direct, p2m_access_rw) )
>>> +            {
>>> +                p2m_unlock(p2m);
>>> +                return -1;
>>> +            }
>>> +            p2m_unlock(p2m);
>>> +        }
>>> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>> +                                      
>>> IOMMUF_readable|IOMMUF_writable) )
>>>               return -1;
>>>           base_pfn++;
>>>       }
>>
>>
>>