Re: [Xen-devel] [PATCH v5 10/28] xsplice: Implement payload loading

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

> > -void *vm_alloc(unsigned int nr, unsigned int align)
> > +void __init vm_init(void)
> > +{
> > +    vm_init_type(VMAP_VIRT);
> > +#ifdef CONFIG_XSPLICE
> > +    vm_init_type(XEN_VIRT);
> > +#endif
> > +}
> 
> I think we should leave it to the arch to call vm_init_type() for
> the non-default type(s) it cares about, namely allowing for this to
> be done at a different (later) time. Which means vm_init() could
> simply become an inline/macro wrapper of vm_init_type(VMAP_VIRT).

Yup, and then I've an arch_xsplice_init which nicely calls vm_init_type
for the VMAP_VIRT_XEN.

> 
> > +void *vm_alloc(unsigned int nr, unsigned int align)
> > +{
> > +    return vm_alloc_type(nr, align, VMAP_VIRT);
> > +}
> 
> Inline/macro wrapper?
> 
> > +void vm_free(const void *va)
> > +{
> > +    vm_free_type(va, VMAP_VIRT);
> > +}
> 
> Again.
> 
> >  void vunmap(const void *va)
> >  {
> > +    enum vmap_type type = VMAP_VIRT;
> > +    unsigned int size = vm_size(va, type);
> > +#ifndef _PAGE_NONE
> > +    unsigned long addr;
> > +#endif
> > +
> > +    if ( !size )
> > +    {
> > +        type = XEN_VIRT;
> > +        size = vm_size(va, type);
> > +    }
> 
> I don't think such automatic fallback should be tried - the caller ought
> to know which region it mapped, so it could call vunmap_type().

OK.
> 
> > @@ -238,11 +288,15 @@ void *vmalloc(size_t size)
> >          mfn[i] = _mfn(page_to_mfn(pg));
> >      }
> >  
> > -    va = vmap(mfn, pages);
> > +    va = __vmap(mfn, 1, pages, 1, PAGE_HYPERVISOR, type);
> >      if ( va == NULL )
> >          goto error;
> >  
> > -    xfree(mfn);
> > +    if ( mfn_array )
> > +        *mfn_array = mfn;
> > +    else
> > +        xfree(mfn);
> 
> What's this? I certainly assumed this wouldn't be needed anymore
> now.

I still need the MFNs so I can change the page table attributes once
I've finished copying the ELF.

I can walk the virtual addresses and gather them from the PTE,
but I figured it would be easier to have them stashed away somewhere?

> 
> > @@ -275,7 +334,10 @@ void vfree(void *va)
> >      if ( !va )
> >          return;
> >  
> > -    pages = vm_size(va);
> > +    pages = vm_size(va, VMAP_VIRT);
> > +    if ( !pages )
> > +        pages = vm_size(va, XEN_VIRT);
> 
> Same comment as as for vunmap().
> 
> > +enum vmap_type {
> > +    VMAP_VIRT,
> > +    XEN_VIRT,
> > +    VMAP_TYPE_MAX,
> > +};
> 
> I think these would benefit from using a common prefix, e.g.
> 
> enum vmap_type {
>     VMAP_DEFAULT,
>     VMAP_XEN,
>     VMAP_nr
> };

Thanks!
> 
> Jan

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