|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] ARM fixes for my improved privcmd patch.
On Tue, 2012-12-18 at 19:34 +0000, Mats Petersson wrote:
> >> I think I'll do the minimal patch first, then, if I find some spare
> >> time, work on the "batching" variant.
> > OK. The batching is IMHO just using the multicall variant.
The XENMEM_add_to_physmap_range is itself batched (it contains ranges of
mfns etc), so we don't just want to batch the actual hypercalls we
really want to make sure each hypercall processes a batch, this will
lets us optimise the flushes in the hypervisor.
I don't know if they mutlicall infrastructure allows for that but it
would be pretty trivial to do it explicitly
I expect these patches will need to be folded into one to avoid a
bisecability hazard?
xen-privcmd-remap-array-arm.patch:
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 7a32976..dc50a53 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -73,7 +73,7 @@ static int map_foreign_page(unsigned long lpfn, unsigned
> long fgmfn,
> }
>
> struct remap_data {
> - xen_pfn_t fgmfn; /* foreign domain's gmfn */
> + xen_pfn_t *fgmfn; /* foreign domain's gmfn */
> pgprot_t prot;
> domid_t domid;
> struct vm_area_struct *vma;
> @@ -90,38 +90,120 @@ static int remap_pte_fn(pte_t *ptep, pgtable_t token,
> unsigned long addr,
> unsigned long pfn = page_to_pfn(page);
> pte_t pte = pfn_pte(pfn, info->prot);
>
> - if (map_foreign_page(pfn, info->fgmfn, info->domid))
> + // TODO: We should really batch these updates
> + if (map_foreign_page(pfn, *info->fgmfn, info->domid))
> return -EFAULT;
> set_pte_at(info->vma->vm_mm, addr, ptep, pte);
> + info->fgmfn++;
Looks good.
> return 0;
> }
>
> -int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
> - unsigned long addr,
> - xen_pfn_t mfn, int nr,
> - pgprot_t prot, unsigned domid,
> - struct page **pages)
> +/* do_remap_mfn() - helper function to map foreign pages
> + * @vma: the vma for the pages to be mapped into
> + * @addr: the address at which to map the pages
> + * @mfn: pointer to array of MFNs to map
> + * @nr: the number entries in the MFN array
> + * @prot: page protection mask
> + * @domid: id of the domain that we are mapping from
> + * @pages: page information (for PVH, not used currently)
> + *
> + * This function takes an array of mfns and maps nr pages from that into
> + * this kernel's memory. The owner of the pages is defined by domid. Where
> the
> + * pages are mapped is determined by addr, and vma is used for "accounting"
> of
> + * the pages.
> + *
> + * Return value is zero for success, negative for failure.
> + */
> +static int do_remap_mfn(struct vm_area_struct *vma,
> + unsigned long addr,
> + xen_pfn_t *mfn, int nr,
> + pgprot_t prot, unsigned domid,
> + struct page **pages)
Since xen_remap_domain_mfn_range isn't implemented on ARM the only
caller is xen_remap_domain_mfn_array so you might as well just call this
function ..._array.
>
> {
> int err;
> struct remap_data data;
>
> - /* TBD: Batching, current sole caller only does page at a time */
> - if (nr > 1)
> - return -EINVAL;
> + BUG_ON(!((vma->vm_flags & (VM_PFNMAP | VM_IO)) == (VM_PFNMAP |
> VM_IO)));
Where does this restriction come from?
I think it is true of X86 PV MMU (which has certain requirements about
how and when PTEs are changed) but I don't think ARM or PVH need it
because they use two level paging so the PTEs are just normal native
ones with no extra restrictions.
Maybe it is useful to enforce similarity between PV and PVH/ARM though?
> data.fgmfn = mfn;
> - data.prot = prot;
> + data.prot = prot;
> data.domid = domid;
> - data.vma = vma;
> - data.index = 0;
> + data.vma = vma;
>
>
> data.pages = pages;
> - err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
> - remap_pte_fn, &data);
> - return err;
> + data.index = 0;
> +
> + while (nr) {
> + unsigned long range = 1 << PAGE_SHIFT;
> +
> + err = apply_to_page_range(vma->vm_mm, addr, range,
> + remap_pte_fn, &data);
> + /* Warning: We do probably need to care about what error we
> + get here. However, currently, the remap_area_mfn_pte_fn is
^ this isn't the name of
the fn
>
> + only likely to return EFAULT or some other "things are very
> + bad" error code, which the rest of the calling code won't
> + be able to fix up. So we just exit with the error we got.
It expect it is more important to accumulate the individual errors from
remap_pte_fn into err_ptr.
> + */
> + if (err)
> + return err;
> +
> + nr--;
> + addr += range;
> + }
>
This while loop (and therefore this change) is unnecessary. The single
call to apply_to_page_range is sufficient and as your TODO notes any
batching should happen in remap_pte_fn (which can handle both
accumulation and flushing when the batch is large enough).
> + return 0;
> +}
> +
> +/* xen_remap_domain_mfn_array() - Used to map an array of foreign pages
> + * @vma: the vma for the pages to be mapped into
> + * @addr: the address at which to map the pages
physical address, right?
> + * @mfn: pointer to array of MFNs to map
> + * @nr: the number entries in the MFN array
> + * @err_ptr: pointer to array of integers, one per MFN, for an error
> + * value for each page. The err_ptr must not be NULL.
Nothing seems to be filling this in?
> + * @prot: page protection mask
> + * @domid: id of the domain that we are mapping from
> + * @pages: page information (for PVH, not used currently)
No such thing as PVH on ARM. Also pages is used by this code.
>
> + *
> + * This function takes an array of mfns and maps nr pages from that into this
> + * kernel's memory. The owner of the pages is defined by domid. Where the
> pages
> + * are mapped is determined by addr, and vma is used for "accounting" of the
> + * pages. The err_ptr array is filled in on any page that is not sucessfully
>
> successfully
>
> + * mapped in.
> + *
> + * Return value is zero for success, negative ERRNO value for failure.
> + * Note that the error value -ENOENT is considered a "retry", so when this
> + * error code is seen, another call should be made with the list of pages
> that
> + * are marked as -ENOENT in the err_ptr array.
> + */
> +int xen_remap_domain_mfn_array(struct vm_area_struct *vma,
> + unsigned long addr,
> + xen_pfn_t *mfn, int nr,
> + int *err_ptr, pgprot_t prot,
> + unsigned domid,
> + struct page **pages)
> +{
> + /* We BUG_ON because it's a programmer error to pass a NULL err_ptr,
> + * and the consequences later is quite hard to detect what the actual
> + * cause of "wrong memory was mapped in".
> + * Note: This variant doesn't actually use err_ptr at the moment.
True ;-)
> + */
> + BUG_ON(err_ptr == NULL);
> + return do_remap_mfn(vma, addr, mfn, nr, prot, domid, pages);
> +}
> +EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_array);
> +
> +/* Not used in ARM. Use xen_remap_domain_mfn_array(). */
> +int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
> + unsigned long addr,
> + xen_pfn_t mfn, int nr,
> + pgprot_t prot, unsigned domid,
> + struct page **pages)
> +{
> + return -ENOSYS;
> }
> EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
>
> +
> int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> int nr, struct page **pages)
> {
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |