Re: [Xen-devel] Xen-unstable-staging: Xen BUG at iommu_map.c:455

[Sander Eikelenboom <linux@xxxxxxxxxxxxxx>]

Monday, April 20, 2015, 6:11:42 PM, you wrote:

>>>> On 16.04.15 at 11:28, <tim@xxxxxxx> wrote:
>> At 22:35 +0100 on 11 Apr (1428791713), Andrew Cooper wrote:
>>> At least we now understand why it happens.  I will defer to others CC'd
>>> on this thread for their opinions in the matter.
>> 
>> The patch semes like a pretty good check to me, though I'm not
>> convinced it's race-free.  At the least I'd cache the m2p lookup so we
>> use the same value for the checks and the map_page() call. 
>> 
>> And IMO update_paging_mode() ought to log and reject bogus GFNs as
>> well.

> Sander,

> could you give the patch below a try, namely also in the context
> of seeing again the issue originally fixed by Andrew's initial patch?
> Please make sure you try a debug build and you have
> "iommu=debug" on the Xen command line.

> Jan

Hi Jan,

I'm running with it now, have seen no issues so far !

--
Sander

> --- unstable.orig/xen/drivers/passthrough/amd/iommu_map.c
> +++ unstable/xen/drivers/passthrough/amd/iommu_map.c
> @@ -557,6 +557,10 @@ static int update_paging_mode(struct dom
>      unsigned long old_root_mfn;
>      struct hvm_iommu *hd = domain_hvm_iommu(d);
>  
> +    if ( gfn == INVALID_MFN )
> +        return -EADDRNOTAVAIL;
+    ASSERT(!(gfn >>> DEFAULT_DOMAIN_ADDRESS_WIDTH));
> +
>      level = hd->arch.paging_mode;
>      old_root = hd->arch.root_table;
>      offset = gfn >> (PTE_PER_TABLE_SHIFT * (level - 1));
> @@ -729,12 +733,15 @@ int amd_iommu_unmap_page(struct domain *
>       * we might need a deeper page table for lager gfn now */
>      if ( is_hvm_domain(d) )
>      {
> -        if ( update_paging_mode(d, gfn) )
> +        int rc = update_paging_mode(d, gfn);
> +
> +        if ( rc )
>          {
>              spin_unlock(&hd->arch.mapping_lock);
>              AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn);
> -            domain_crash(d);
> -            return -EFAULT;
> +            if ( rc != -EADDRNOTAVAIL )
> +                domain_crash(d);
> +            return rc;
>          }
>      }
>  
> --- unstable.orig/xen/drivers/passthrough/x86/iommu.c
> +++ unstable/xen/drivers/passthrough/x86/iommu.c
> @@ -59,10 +59,17 @@ int arch_iommu_populate_page_table(struc
>          if ( has_hvm_container_domain(d) ||
>              (page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page )
>          {
> -            BUG_ON(SHARED_M2P(mfn_to_gmfn(d, page_to_mfn(page))));
> -            rc = hd->platform_ops->map_page(
> -                d, mfn_to_gmfn(d, page_to_mfn(page)), page_to_mfn(page),
> -                IOMMUF_readable|IOMMUF_writable);
> +            unsigned long mfn = page_to_mfn(page);
> +            unsigned long gfn = mfn_to_gmfn(d, mfn);
> +
> +            if ( gfn != INVALID_MFN )
> +            {
> +                ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
> +                BUG_ON(SHARED_M2P(gfn));
> +                rc = hd->platform_ops->map_page(d, gfn, mfn,
> +                                                IOMMUF_readable |
> +                                                IOMMUF_writable);
> +            }
>              if ( rc )
>              {
>                  page_list_add(page, &d->page_list);
> --- unstable.orig/xen/drivers/passthrough/vtd/iommu.h
> +++ unstable/xen/drivers/passthrough/vtd/iommu.h
> @@ -482,7 +482,6 @@ struct qinval_entry {
>  #define VTD_PAGE_TABLE_LEVEL_3  3
>  #define VTD_PAGE_TABLE_LEVEL_4  4
>  
> -#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
>  #define MAX_IOMMU_REGS 0xc0
>  
>  extern struct list_head acpi_drhd_units;
> --- unstable.orig/xen/include/asm-x86/hvm/iommu.h
> +++ unstable/xen/include/asm-x86/hvm/iommu.h
> @@ -46,6 +46,8 @@ struct g2m_ioport {
>      unsigned int np;
>  };
>  
> +#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48
> +
>  struct arch_hvm_iommu
>  {
>      u64 pgd_maddr;                 /* io page directory machine address */
> --- unstable.orig/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> +++ unstable/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> @@ -464,8 +464,6 @@
>  #define IOMMU_CONTROL_DISABLED       0
>  #define IOMMU_CONTROL_ENABLED  1
>  
> -#define DEFAULT_DOMAIN_ADDRESS_WIDTH    48
> -
>  /* interrupt remapping table */
>  #define INT_REMAP_ENTRY_REMAPEN_MASK    0x00000001
>  #define INT_REMAP_ENTRY_REMAPEN_SHIFT   0





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