Re: [PATCH v8 08/27] ARM: introduce vgic_access_guest_memory()

[Julien Grall <julien.grall@arm.com>]

Hi Andre,

On 12/04/17 01:44, Andre Przywara wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> This function allows to copy a chunk of data from and to guest physical
> memory. It looks up the associated page from the guest's p2m tree
> and maps this page temporarily for the time of the access.
> This function was originally written by Vijaya as part of an earlier series:
> https://patchwork.kernel.org/patch/8177251
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/vgic.c        | 50 ++++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/vgic.h |  3 +++
>  2 files changed, 53 insertions(+)
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index b6fe34f..e385b43 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -20,6 +20,7 @@
>  #include <xen/bitops.h>
>  #include <xen/lib.h>
>  #include <xen/init.h>
> +#include <xen/domain_page.h>
>  #include <xen/softirq.h>
>  #include <xen/irq.h>
>  #include <xen/sched.h>
> @@ -602,6 +603,55 @@ void vgic_free_virq(struct domain *d, unsigned int virq)
>  }
>
>  /*
> + * Temporarily map one physical guest page and copy data to or from it.
> + * The data to be copied cannot cross a page boundary.
> + */
> +int vgic_access_guest_memory(struct domain *d, paddr_t gpa, void *buf,
> +                             uint32_t size, bool_t is_write)
> +{
> +    struct page_info *page;
> +    uint64_t offset;
> +    p2m_type_t p2mt;
> +    int ret = 0;
> +    void *p;
> +
> +    page = get_page_from_gfn(d, paddr_to_pfn(gpa), &p2mt, P2M_ALLOC);
> +    if ( !page )
> +    {
> +        printk(XENLOG_G_ERR "d%d: vITS: Failed to get table entry\n",
> +               d->domain_id);
> +        return -EINVAL;
> +    }
> +
> +    if ( !p2m_is_ram(p2mt) )
> +    {
> +        put_page(page);
> +        printk(XENLOG_G_ERR "d%d: vITS: memory used by the ITS should be RAM.",
> +               d->domain_id);
> +        return -EINVAL;
> +    }
> +
> +    p = __map_domain_page(page);
> +    /* Offset within the mapped page */
> +    offset = gpa & ~PAGE_MASK;
> +    /* Do not cross a page boundary. */
> +    if ( size <= (PAGE_SIZE - offset) )
> +    {
> +        if ( is_write )
> +            memcpy(p + offset, buf, size);
> +        else
> +            memcpy(buf, p + offset, size);
> +    }
> +    else
> +        ret = -ENOSPC;

Why ENOSPC? The buffer still have space we are just not able to handle 
the value, so it should be -EINVAL as I suggested on the previous version.

Also, I don't understand why you didn't move the check at the beginning 
of the function. It would make the code more readable. By that I mean:


int vgic_access_....

{
    offset = gpa & ~PAGE_MASK;

    if ( size > (PAGE_SIZE - offset) )
      return -EINVAL;

    /* Rest of the code */

}


> +
> +    unmap_domain_page(p);
> +    put_page(page);
> +
> +    return ret;
> +}
> +
> +/*
>   * Local variables:
>   * mode: C
>   * c-file-style: "BSD"
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 7efa164..6b17802 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -313,6 +313,9 @@ extern void register_vgic_ops(struct domain *d, const struct vgic_ops *ops);
>  int vgic_v2_init(struct domain *d, int *mmio_count);
>  int vgic_v3_init(struct domain *d, int *mmio_count);
>
> +int vgic_access_guest_memory(struct domain *d, paddr_t gpa, void *buf,
> +                             uint32_t size, bool_t is_write);
> +
>  extern int domain_vgic_register(struct domain *d, int *mmio_count);
>  extern int vcpu_vgic_free(struct vcpu *v);
>  extern bool vgic_to_sgi(struct vcpu *v, register_t sgir,
>

Cheers,

-- 
Julien Grall

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