Re: [Xen-devel] dom0 linux 3.6.0-rc4, crash due to ballooning althoug dom0_mem=X, max:X set

[Sander Eikelenboom <linux@xxxxxxxxxxxxxx>]

Tuesday, September 11, 2012, 6:02:47 PM, you wrote:

> On Wed, 5 Sep 2012, Konrad Rzeszutek Wilk wrote:
>> On Tue, Sep 04, 2012 at 04:27:20PM -0400, Robert Phillips wrote:
>> > Ben,
>> > 
>> > You have asked me to provide the rationale behind the gnttab_old_mfn 
>> > patch, which you emailed to Sander earlier today. 
>> > Here are my findings.
>> > 
>> > I found that xen_blkbk_map() in drivers/block/xen-blkback/blkback.c has 
>> > changed from our previous version.  It now calls gnttab_map_refs() in 
>> > drivers/xen/grant-table.c.
>> > 
>> > That function first calls 
>> > HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, ... ) and then calls 
>> > m2p_add_override() in p2m.c
>> 
>> And HYPERVISOR_grant_table_op .. would populate map_ops[i].bus_addr with the 
>> machine address..
>> 
>> > which is where I made my change.
>> > 
>> > The unpatched code was saving the pfn's old mfn in kmap_op->dev_bus_addr.  
>> > 
>> > kmap_op is of type struct gnttab_map_grant_ref.  That data type is used to 
>> > record grant table mappings so later they can be unmapped correctly.
>> 
>> Right, but the blkback makes a distinction by passing NULL as kmap_op, which 
>> means it should
>> use the old mechanism. Meaning that once the hypercall is done, the 
>> map_ops[i].bus_addr is not
>> used anymore..
>> 
>> > 
>> > The problem with saving the old mfn in kmap_op->dev_bus_addr is that it is 
>> > later overwritten by __gnttab_map_grant_ref() in xen/common/grant_table.c
>> 
>> Uh, so the problem of saving the old mfn in dev_bus_addr has been there for 
>> a long long time then?
>> Even before this patch set?

> I think that Robert identified the real problem: dev_bus_addr shouldn't
> have been used here. However the bug only shows up if we are batching
> the grant table operations, that we started doing since
> f62805f1f30a40e354bd036b4cb799863a39be4b.
> That's why Sander's bisection found that
> f62805f1f30a40e354bd036b4cb799863a39be4b is the culprit.

> However the fix is incorrect because it is modifying a struct that is
> part of the Xen ABI.
> I am appending an alternative fix that doesn't need any changes to
> public headers.

> Sander, could you please let me know if it fixes the problem for you?

It does !

Tested-By: Sander Eikelenboom <linux@xxxxxxxxxxxxxx>

> ---


> diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
> index 93971e8..472b9b7 100644
> --- a/arch/x86/include/asm/xen/page.h
> +++ b/arch/x86/include/asm/xen/page.h
> @@ -51,7 +51,8 @@ extern unsigned long set_phys_range_identity(unsigned long 
> pfn_s,
>  
>  extern int m2p_add_override(unsigned long mfn, struct page *page,
>                             struct gnttab_map_grant_ref *kmap_op);
> -extern int m2p_remove_override(struct page *page, bool clear_pte);
> +extern int m2p_remove_override(struct page *page,
> +                               struct gnttab_map_grant_ref *kmap_op);
>  extern struct page *m2p_find_override(unsigned long mfn);
>  extern unsigned long m2p_find_override_pfn(unsigned long mfn, unsigned long 
> pfn);
>  
> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> index 64effdc..2825594 100644
> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -734,9 +734,6 @@ int m2p_add_override(unsigned long mfn, struct page *page,
>  
>                         xen_mc_issue(PARAVIRT_LAZY_MMU);
>                 }
> -               /* let's use dev_bus_addr to record the old mfn instead */
> -               kmap_op->dev_bus_addr = page->index;
> -               page->index = (unsigned long) kmap_op;
>         }
>         spin_lock_irqsave(&m2p_override_lock, flags);
>         list_add(&page->lru,  &m2p_overrides[mfn_hash(mfn)]);
> @@ -763,7 +760,8 @@ int m2p_add_override(unsigned long mfn, struct page *page,
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(m2p_add_override);
> -int m2p_remove_override(struct page *page, bool clear_pte)
> +int m2p_remove_override(struct page *page,
> +               struct gnttab_map_grant_ref *kmap_op)
>  {
>         unsigned long flags;
>         unsigned long mfn;
> @@ -793,10 +791,8 @@ int m2p_remove_override(struct page *page, bool 
> clear_pte)
>         WARN_ON(!PagePrivate(page));
>         ClearPagePrivate(page);
>  
> -       if (clear_pte) {
> -               struct gnttab_map_grant_ref *map_op =
> -                       (struct gnttab_map_grant_ref *) page->index;
> -               set_phys_to_machine(pfn, map_op->dev_bus_addr);
> +       set_phys_to_machine(pfn, page->index);
> +       if (kmap_op != NULL) {
>                 if (!PageHighMem(page)) {
>                         struct multicall_space mcs;
>                         struct gnttab_unmap_grant_ref *unmap_op;
> @@ -808,13 +804,13 @@ int m2p_remove_override(struct page *page, bool 
> clear_pte)
>                          * issued. In this case handle is going to -1 because
>                          * it hasn't been modified yet.
>                          */
> -                       if (map_op->handle == -1)
> +                       if (kmap_op->handle == -1)
>                                 xen_mc_flush();
>                         /*
> -                        * Now if map_op->handle is negative it means that the
> +                        * Now if kmap_op->handle is negative it means that 
> the
>                          * hypercall actually returned an error.
>                          */
> -                       if (map_op->handle == GNTST_general_error) {
> +                       if (kmap_op->handle == GNTST_general_error) {
>                                 printk(KERN_WARNING "m2p_remove_override: "
>                                                 "pfn %lx mfn %lx, failed to 
> modify kernel mappings",
>                                                 pfn, mfn);
> @@ -824,8 +820,8 @@ int m2p_remove_override(struct page *page, bool clear_pte)
>                         mcs = xen_mc_entry(
>                                         sizeof(struct 
> gnttab_unmap_grant_ref));
>                         unmap_op = mcs.args;
> -                       unmap_op->host_addr = map_op->host_addr;
> -                       unmap_op->handle = map_op->handle;
> +                       unmap_op->host_addr = kmap_op->host_addr;
> +                       unmap_op->handle = kmap_op->handle;
>                         unmap_op->dev_bus_addr = 0;
>  
>                         MULTI_grant_table_op(mcs.mc,
> @@ -836,10 +832,9 @@ int m2p_remove_override(struct page *page, bool 
> clear_pte)
>                         set_pte_at(&init_mm, address, ptep,
>                                         pfn_pte(pfn, PAGE_KERNEL));
>                         __flush_tlb_single(address);
> -                       map_op->host_addr = 0;
> +                       kmap_op->host_addr = 0;
>                 }
> -       } else
> -               set_phys_to_machine(pfn, page->index);
> +       }
>  
>         /* p2m(m2p(mfn)) == FOREIGN_FRAME(mfn): the mfn is already present
>          * somewhere in this domain, even before being added to the
> diff --git a/drivers/block/xen-blkback/blkback.c 
> b/drivers/block/xen-blkback/blkback.c
> index 73f196c..c6decb9 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -337,7 +337,7 @@ static void xen_blkbk_unmap(struct pending_req *req)
>                 invcount++;
>         }
>  
> -       ret = gnttab_unmap_refs(unmap, pages, invcount, false);
> +       ret = gnttab_unmap_refs(unmap, NULL, pages, invcount);
>         BUG_ON(ret);
>  }
>  
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index 1ffd03b..7f12416 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -314,8 +314,9 @@ static int __unmap_grant_pages(struct grant_map *map, int 
> offset, int pages)
>                 }
>         }
>  
> -       err = gnttab_unmap_refs(map->unmap_ops + offset, map->pages + offset,
> -                               pages, true);
> +       err = gnttab_unmap_refs(map->unmap_ops + offset,
> +                       use_ptemod ? map->kmap_ops + offset : NULL, 
> map->pages + offset,
> +                       pages);
>         if (err)
>                 return err;
>  
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index 0bfc1ef..0067266 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -870,7 +870,8 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
>  EXPORT_SYMBOL_GPL(gnttab_map_refs);
>  
>  int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
> -                     struct page **pages, unsigned int count, bool clear_pte)
> +                     struct gnttab_map_grant_ref *kmap_ops,
> +                     struct page **pages, unsigned int count)
>  {
>         int i, ret;
>         bool lazy = false;
> @@ -888,7 +889,8 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref 
> *unmap_ops,
>         }
>  
>         for (i = 0; i < count; i++) {
> -               ret = m2p_remove_override(pages[i], clear_pte);
> +               ret = m2p_remove_override(pages[i], kmap_ops ?
> +                                      &kmap_ops[i] : NULL);
>                 if (ret)
>                         return ret;
>         }
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index 11e27c3..f19fff8 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -187,6 +187,7 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
>                     struct gnttab_map_grant_ref *kmap_ops,
>                     struct page **pages, unsigned int count);
>  int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
> -                     struct page **pages, unsigned int count, bool 
> clear_pte);
> +                     struct gnttab_map_grant_ref *kunmap_ops,
> +                     struct page **pages, unsigned int count);
>  
>  #endif /* __ASM_GNTTAB_H__ */



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