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

[Tim Deegan <tim@xxxxxxx>]

At 10:11 +0100 on 24 Jul (1406193065), Andrew Cooper wrote:
> 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@xxxxxxxxx>
> >>> ---
> >>>   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.

Indeed -- outside code should not be touching the mm locks, and
furthermore the declaration of p2m_set_entry() has a comment right
beside it that says "only for use by p2m code".

I suppose you can't use the usual add-to-physmap operations
because they would try to call back into iommu code to keep the vtd
tables in sync?  In that case you will need some sort of new API to
the p2m code to handle this case, as Andrew suggested.  But OTOH 
from the commit message it looks like this path only ever needed
when vtd is using the p2m table directly?  In which case, maybe
set_mmio_p2m_entry() already does what you need.

I'm a bit surprised that there's no check for an existing mapping
here, btw -- what if the guest already has memory mapped at that
address?  That's going to go badly when it tries to use that memory
(either from the CPU or DMA).

Cheers,

Tim.

> ~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++;
> >>>       }
> >>
> >>
> >>
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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