Re: [Xen-devel] [PATCH v12 14/18] xen/grant: Implement an grant frame array struct.

[Konrad Rzeszutek Wilk <konrad@darnok.org>]

On Fri, Jan 03, 2014 at 04:53:59PM +0000, Stefano Stabellini wrote:
> On Tue, 31 Dec 2013, Konrad Rzeszutek Wilk wrote:
> > The 'xen_hvm_resume_frames' used to be an 'unsigned long'
> > and contain the virtual address of the grants. That was OK
> > for most architectures (PVHVM, ARM) were the grants are contingous
> > in memory. That however is not the case for PVH - in which case
> > we will have to do a lookup for each virtual address for the PFN.
> > 
> > Instead of doing that, lets make it a structure which will contain
> > the array of PFNs, the virtual address and the count of said PFNs.
> > 
> > Also provide a generic functions: gnttab_setup_auto_xlat_frames and
> > gnttab_free_auto_xlat_frames to populate said structure with
> > appropiate values for PVHVM and ARM.
>      ^appropriate
> 
> 
> > To round it off, change the name from 'xen_hvm_resume_frames' to
> > a more descriptive one - 'xen_auto_xlat_grant_frames'.
> > 
> > For PVH, in patch "xen/pvh: Piggyback on PVHVM for grant driver"
> > we will populate the 'xen_auto_xlat_grant_frames' by ourselves.
> > 
> > Suggested-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  arch/arm/xen/enlighten.c   |  9 +++++++--
> >  drivers/xen/grant-table.c  | 45 ++++++++++++++++++++++++++++++++++++++++-----
> >  drivers/xen/platform-pci.c | 10 +++++++---
> >  include/xen/grant_table.h  |  9 ++++++++-
> >  4 files changed, 62 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > index 8550123..2162172 100644
> > --- a/arch/arm/xen/enlighten.c
> > +++ b/arch/arm/xen/enlighten.c
> > @@ -208,6 +208,7 @@ static int __init xen_guest_init(void)
> >  	const char *version = NULL;
> >  	const char *xen_prefix = "xen,xen-";
> >  	struct resource res;
> > +	unsigned long grant_frames;
> >  
> >  	node = of_find_compatible_node(NULL, NULL, "xen,xen");
> >  	if (!node) {
> > @@ -224,10 +225,10 @@ static int __init xen_guest_init(void)
> >  	}
> >  	if (of_address_to_resource(node, GRANT_TABLE_PHYSADDR, &res))
> >  		return 0;
> > -	xen_hvm_resume_frames = res.start;
> > +	grant_frames = res.start;
> >  	xen_events_irq = irq_of_parse_and_map(node, 0);
> >  	pr_info("Xen %s support found, events_irq=%d gnttab_frame_pfn=%lx\n",
> > -			version, xen_events_irq, (xen_hvm_resume_frames >> PAGE_SHIFT));
> > +			version, xen_events_irq, (grant_frames >> PAGE_SHIFT));
> >  	xen_domain_type = XEN_HVM_DOMAIN;
> >  
> >  	xen_setup_features();
> > @@ -265,6 +266,10 @@ static int __init xen_guest_init(void)
> >  	if (xen_vcpu_info == NULL)
> >  		return -ENOMEM;
> >  
> > +	if (gnttab_setup_auto_xlat_frames(grant_frames)) {
> > +		free_percpu(xen_vcpu_info);
> > +		return -ENOMEM;
> > +	}
> >  	gnttab_init();
> >  	if (!xen_initial_domain())
> >  		xenbus_probe(NULL);
> > diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> > index cc1b4fa..b117fd6 100644
> > --- a/drivers/xen/grant-table.c
> > +++ b/drivers/xen/grant-table.c
> > @@ -65,8 +65,8 @@ static unsigned int nr_grant_frames;
> >  static int gnttab_free_count;
> >  static grant_ref_t gnttab_free_head;
> >  static DEFINE_SPINLOCK(gnttab_list_lock);
> > -unsigned long xen_hvm_resume_frames;
> > -EXPORT_SYMBOL_GPL(xen_hvm_resume_frames);
> > +struct grant_frames xen_auto_xlat_grant_frames;
> > +EXPORT_SYMBOL_GPL(xen_auto_xlat_grant_frames);
> 
> it should be static now

Can't be. The arch/x86/xen/grant-table.c has to use it.

I can drop the 'EXPORT_SYMBOL_GPL' though.

> 
> 
> >  static union {
> >  	struct grant_entry_v1 *v1;
> > @@ -838,6 +838,40 @@ unsigned int gnttab_max_grant_frames(void)
> >  }
> >  EXPORT_SYMBOL_GPL(gnttab_max_grant_frames);
> >  
> > +int gnttab_setup_auto_xlat_frames(unsigned long addr)
> > +{
> > +	xen_pfn_t *pfn;
> > +	unsigned int max_nr_gframes = __max_nr_grant_frames();
> > +	int i;
> > +
> > +	if (xen_auto_xlat_grant_frames.count)
> > +		return -EINVAL;
> > +
> > +	pfn = kcalloc(max_nr_gframes, sizeof(pfn[0]), GFP_KERNEL);
> > +	if (!pfn)
> > +		return -ENOMEM;
> > +	for (i = 0; i < max_nr_gframes; i++)
> > +		pfn[i] = PFN_DOWN(addr + (i * PAGE_SIZE));
> > +
> > +	xen_auto_xlat_grant_frames.vaddr = addr;
> > +	xen_auto_xlat_grant_frames.pfn = pfn;
> > +	xen_auto_xlat_grant_frames.count = max_nr_gframes;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(gnttab_setup_auto_xlat_frames);
> > +
> > +void gnttab_free_auto_xlat_frames(void)
> > +{
> > +	if (!xen_auto_xlat_grant_frames.count)
> > +		return;
> > +	kfree(xen_auto_xlat_grant_frames.pfn);
> > +	xen_auto_xlat_grant_frames.pfn = NULL;
> > +	xen_auto_xlat_grant_frames.count = 0;
> > +	xen_auto_xlat_grant_frames.vaddr = 0;
> > +}
> > +EXPORT_SYMBOL_GPL(gnttab_free_auto_xlat_frames);
> 
> I would leave vaddr alone in gnttab_setup_auto_xlat_frames and
> gnttab_free_auto_xlat_frames

Actually, I like David's suggestion. Patch coming out soon.