Re: [Xen-devel] [PATCH] x86/pv: Export pv_hypercall_table[] rather than working around it in several ways

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jan 24, 2018 at 12:16:09PM +0000, Andrew Cooper wrote:
> The functions in compat.c are thing wrappers around the main hypercalls,
> massaging certain parameters.  However, they second-guess the content of
> pv_hypercall_table[], which is problematic for the shim case.  Instead,
> arrange for them to call via function pointer, which removes the need for
> pv_get_hypercall_handler().
> 
> With pv_hypercall_table[] exported, there is no need for
> pv_hypercall_table_replace(), so its single callsite gets modified to cope.
> The backing code behind __va(__pa()) is substantial, and there is no need to
> calculate it repeatedly (Xen's .rodata is also contiguous in the directmap).
> 
> While adjusting the declarations, guard content in arch/x86/pv with CONFIG_PV.
> 
> The net difference is:
>   add/remove: 0/2 grow/shrink: 4/1 up/down: 176/-321 (-145)
>   function                                     old     new   delta
>   pv_shim_setup_dom                           1130    1266    +136
>   do_sched_op_compat                           176     192     +16
>   compat_physdev_op_compat                      90     106     +16
>   do_physdev_op_compat                          98     106      +8
>   do_event_channel_op_compat                   145     123     -22
>   pv_get_hypercall_handler                      28       -     -28
>   pv_hypercall_table_replace                   271       -    -271
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

LGTM:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
> index d5383dc..9651952 100644
> --- a/xen/arch/x86/pv/shim.c
> +++ b/xen/arch/x86/pv/shim.c
> @@ -160,6 +160,7 @@ void __init pv_shim_setup_dom(struct domain *d, 
> l4_pgentry_t *l4start,
>                                unsigned long console_va, unsigned long 
> vphysmap,
>                                start_info_t *si)
>  {
> +    hypercall_table_t *rw_pv_hypercall_table;
>      uint64_t param = 0;
>      long rc;
>  
> @@ -205,12 +206,20 @@ void __init pv_shim_setup_dom(struct domain *d, 
> l4_pgentry_t *l4start,
>                              mfn_x(console_mfn), vphysmap);
>          consoled_set_ring_addr(page);
>      }
> -    pv_hypercall_table_replace(__HYPERVISOR_event_channel_op,
> -                               (hypercall_fn_t *)pv_shim_event_channel_op,
> -                               (hypercall_fn_t *)pv_shim_event_channel_op);
> -    pv_hypercall_table_replace(__HYPERVISOR_grant_table_op,
> -                               (hypercall_fn_t *)pv_shim_grant_table_op,
> -                               (hypercall_fn_t *)pv_shim_grant_table_op);
> +
> +    /*
> +     * Find a writeable mapping to pv_hypercall_table[] in the directmap
> +     * insert some shim-specific hypercall handlers.
         ^ and

Maybe add something like:

"The hypercall table lives in .rodata, and so it's contiguous in
physical address space, which makes it also contiguous in VA space in
the direct map"

Or better worded.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel