Re: [PATCH v3 2/2] xen: modify kernel mappings corresponding togranted pages

[Stefano Stabellini]

On Wed, 7 Sep 2011, Jeremy Fitzhardinge wrote:
> On 09/07/2011 09:39 AM, stefano.stabellini@xxxxxxxxxxxxx wrote:
> > From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> >
> > If we want to use granted pages for AIO, changing the mappings of a user
> > vma and the corresponding p2m is not enough, we also need to update the
> > kernel mappings accordingly.
> > In order to avoid the complexity of dealing with highmem, we allocated
> > the pages lowmem.
> > We issue a HYPERVISOR_grant_table_op right away in
> > m2p_add_override and we remove the mappings using another
> > HYPERVISOR_grant_table_op in m2p_remove_override.
> > Considering that m2p_add_override and m2p_remove_override are called
> > once per page we use multicalls and hypercall batching.
> >
> > Use the kmap_op pointer directly as argument to do the mapping as it is
> > guaranteed to be present up until the unmapping is done.
> > Before issuing any unmapping multicalls, we need to make sure that the
> > mapping has already being done, because we need the kmap->handle to be
> > set correctly.
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > ---
> >  arch/x86/include/asm/xen/page.h |    5 ++-
> >  arch/x86/xen/p2m.c              |   67 ++++++++++++++++++++++++++++++++------
> >  drivers/xen/gntdev.c            |   27 +++++++++++++++-
> >  drivers/xen/grant-table.c       |    4 +-
> >  include/xen/grant_table.h       |    1 +
> >  5 files changed, 89 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
> > index 7ff4669..0ce1884 100644
> > --- a/arch/x86/include/asm/xen/page.h
> > +++ b/arch/x86/include/asm/xen/page.h
> > @@ -12,6 +12,7 @@
> >  #include <asm/pgtable.h>
> >  
> >  #include <xen/interface/xen.h>
> > +#include <xen/grant_table.h>
> >  #include <xen/features.h>
> >  
> >  /* Xen machine address */
> > @@ -31,8 +32,10 @@ typedef struct xpaddr {
> >  #define INVALID_P2M_ENTRY	(~0UL)
> >  #define FOREIGN_FRAME_BIT	(1UL<<(BITS_PER_LONG-1))
> >  #define IDENTITY_FRAME_BIT	(1UL<<(BITS_PER_LONG-2))
> > +#define GRANT_FRAME_BIT	(1UL<<(BITS_PER_LONG-3))
> >  #define FOREIGN_FRAME(m)	((m) | FOREIGN_FRAME_BIT)
> >  #define IDENTITY_FRAME(m)	((m) | IDENTITY_FRAME_BIT)
> > +#define GRANT_FRAME(m)	((m) | GRANT_FRAME_BIT)
> >  
> >  /* Maximum amount of memory we can handle in a domain in pages */
> >  #define MAX_DOMAIN_PAGES						\
> > @@ -48,7 +51,7 @@ extern unsigned long set_phys_range_identity(unsigned long pfn_s,
> >  					     unsigned long pfn_e);
> >  
> >  extern int m2p_add_override(unsigned long mfn, struct page *page,
> > -			    bool clear_pte);
> > +			    struct gnttab_map_grant_ref *kmap_op);
> >  extern int m2p_remove_override(struct page *page, bool clear_pte);
> >  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 58efeb9..6c26ac80 100644
> > --- a/arch/x86/xen/p2m.c
> > +++ b/arch/x86/xen/p2m.c
> > @@ -161,7 +161,9 @@
> >  #include <asm/xen/page.h>
> >  #include <asm/xen/hypercall.h>
> >  #include <asm/xen/hypervisor.h>
> > +#include <xen/grant_table.h>
> >  
> > +#include "multicalls.h"
> >  #include "xen-ops.h"
> >  
> >  static void __init m2p_override_init(void);
> > @@ -676,7 +678,8 @@ static unsigned long mfn_hash(unsigned long mfn)
> >  }
> >  
> >  /* Add an MFN override for a particular page */
> > -int m2p_add_override(unsigned long mfn, struct page *page, bool clear_pte)
> > +int m2p_add_override(unsigned long mfn, struct page *page,
> > +		struct gnttab_map_grant_ref *kmap_op)
> >  {
> >  	unsigned long flags;
> >  	unsigned long pfn;
> > @@ -699,9 +702,20 @@ int m2p_add_override(unsigned long mfn, struct page *page, bool clear_pte)
> >  	if (unlikely(!set_phys_to_machine(pfn, FOREIGN_FRAME(mfn))))
> >  		return -ENOMEM;
> >  
> > -	if (clear_pte && !PageHighMem(page))
> > -		/* Just zap old mapping for now */
> > -		pte_clear(&init_mm, address, ptep);
> > +	if (kmap_op != NULL) {
> > +		if (!PageHighMem(page)) {
> > +			struct multicall_space mcs = xen_mc_entry(sizeof(*kmap_op));
> > +
> > +			MULTI_grant_table_op(mcs.mc,
> > +					GNTTABOP_map_grant_ref, kmap_op, 1);
> > +
> > +			xen_mc_issue(PARAVIRT_LAZY_MMU);
> > +		}
> > +		page->private |= GRANT_FRAME_BIT;
> > +		/* 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)]);
> >  	spin_unlock_irqrestore(&m2p_override_lock, flags);
> > @@ -735,13 +749,44 @@ int m2p_remove_override(struct page *page, bool clear_pte)
> >  	spin_lock_irqsave(&m2p_override_lock, flags);
> >  	list_del(&page->lru);
> >  	spin_unlock_irqrestore(&m2p_override_lock, flags);
> > -	set_phys_to_machine(pfn, page->index);
> >  
> > -	if (clear_pte && !PageHighMem(page))
> > -		set_pte_at(&init_mm, address, ptep,
> > -				pfn_pte(pfn, PAGE_KERNEL));
> > -		/* No tlb flush necessary because the caller already
> > -		 * left the pte unmapped. */
> > +	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);
> > +		if (!PageHighMem(page)) {
> > +			struct multicall_space mcs =
> > +				xen_mc_entry(sizeof(struct gnttab_unmap_grant_ref));
> > +			struct gnttab_unmap_grant_ref *unmap_op = mcs.args;
> > +
> > +			/* 
> > +			 * Has the grant_op mapping multicall being issued? If not,
> > +			 * make sure it is called now.
> > +			 */
> > +			if (map_op->handle == -1)
> > +				xen_mc_flush();
> 
> Hm, this will end up flushing the empty uninitialized entry you just
> added, which also implicitly frees the space, so all the stuff below is
> - at best - a noop.

I'll have to move it earlier.


> But also, this pattern of getting results back from batched calls is
> unusual - actually, I think this is unique.  If you have batched up a
> map operation which has its map_op args allocated from the multicall
> buffer, then the flush will implicitly free them as well, so it isn't
> valid to read back from the structure later on.  If you want to have an
> args structure you can use once the hypercall has been issued, you need
> to manually manage its lifetime.

But I am not using the multicall buffer as argument, I am using the
kmap_op passed to m2p_add_override.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/