Re: [Xen-devel] [PATCH 16/22] vixen: pass grant table operations through to the outer Xen

[Anthony Liguori <anthony@xxxxxxxxxxxxx>]

On Sun, Jan 7, 2018 at 12:36 AM, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> On Sat, Jan 06, 2018 at 02:54:31PM -0800, Anthony Liguori wrote:
>> From: Anthony Liguori <aliguori@xxxxxxxxxx>
>>
>> The grant table is a region of guest memory that contains GMFNs
>> which in PV are MFNs but are PFNs in HVM.  Since a Vixen guest MFN
>> is an HVM PFN, we can pass this table directly through to the outer
>> Xen which cuts down considerably on overhead.
>>
>> We do not forward most of the hypercalls since we only intend on
>> Vixen to be used for normal guests, not driver domains.
>>
>> Signed-off-by: Anthony Liguori <aliguori@xxxxxxxxxx>
>> ---
>>  xen/common/grant_table.c | 131 
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 131 insertions(+)
>>
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index 250450b..b302fd0 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -39,6 +39,7 @@
>>  #include <xen/vmap.h>
>>  #include <xsm/xsm.h>
>>  #include <asm/flushtlb.h>
>> +#include <asm/guest.h>
>>
>>  /* Per-domain grant information. */
>>  struct grant_table {
>> @@ -1199,6 +1200,9 @@ gnttab_map_grant_ref(
>>      int i;
>>      struct gnttab_map_grant_ref op;
>>
>> +    if ( is_vixen() )
>> +        return -ENOSYS;
>
> Here and below: instead of adding all those is_vixen calls in a bunch
> of gnttab functions, why don't you just replace the whole
> do_grant_table_op function? That's cleaner and less intrusive.

Ack.  That's what we did for event channels and I like it better too.

>>  static long
>> +vixen_gnttab_setup_table(
>> +    XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count)
>> +{
>> +    long rc;
>> +
>> +    struct gnttab_setup_table op;
>> +    xen_pfn_t *frame_list = NULL;
>> +    static void *grant_table;
>> +    XEN_GUEST_HANDLE(xen_pfn_t) old_frame_list;
>> +
>> +    if ( count != 1 )
>> +        return -EINVAL;
>> +
>> +    if ( unlikely(copy_from_guest(&op, uop, 1) != 0) )
>> +    {
>> +        gdprintk(XENLOG_INFO, "Fault while reading 
>> gnttab_setup_table_t.\n");
>> +        return -EFAULT;
>> +    }
>> +
>> +    if ( grant_table == NULL ) {
>> +        struct xen_add_to_physmap xatp;
>> +        struct domain *d;
>> +        int i;
>> +
>> +        for ( i = 0; i < max_grant_frames; i++ )
>> +        {
>> +             grant_table = alloc_xenheap_page();
>
> This is wasting one memory page, grant table frames don't need to be
> populated.

Well they have to have a valid struct page_info in order for the guest
to map it within its address space.

Or did you have something else in mind?

>> +             BUG_ON(grant_table == NULL);
>> +             xatp.domid = DOMID_SELF;
>> +             xatp.idx = i;
>> +             xatp.space = XENMAPSPACE_grant_table;
>> +             xatp.gpfn = virt_to_mfn(grant_table);
>> +             rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
>> +             if ( rc != 0 )
>> +                 printk("Add to physmap failed! %ld\n", rc);
>> +
>> +             d = rcu_lock_current_domain();
>> +             share_xen_page_with_guest(mfn_to_page(xatp.gpfn), d, 
>> XENSHARE_writable);
>> +             rcu_unlock_domain(d);
>> +        }
>> +    }
>> +
>> +    if ( op.nr_frames > 0 ) {
>> +        frame_list = xzalloc_array(xen_pfn_t, op.nr_frames);
>> +        if ( frame_list == NULL )
>> +            return -ENOMEM;
>> +    }
>> +
>> +    old_frame_list = op.frame_list;
>> +    op.frame_list.p = frame_list;
>> +
>> +    rc = HYPERVISOR_grant_table_op(GNTTABOP_setup_table, &op, count);
>
> On HVM you don't need to use the GNTTABOP_setup_table hypercall,
> XENMEM_add_to_physmap already does all the needed setup AFAICT.

I'll double check this, thanks.

>> +    op.frame_list = old_frame_list;
>> +
>> +    if ( rc >= 0 ) {
>> +        if ( op.status == 0 && op.nr_frames &&
>> +             copy_to_guest(old_frame_list, frame_list, op.nr_frames) != 0 ) 
>> {
>> +            rc = -EFAULT;
>> +            goto out;
>> +        }
>> +
>> +        if ( unlikely(copy_to_guest(uop, &op, 1)) != 0 ) {
>> +            rc = -EFAULT;
>> +            goto out;
>> +        }
>> +    }
>> +
>> + out:
>> +    xfree(frame_list);
>> +
>> +    return rc;
>> +}
>> +
>> +static long
>>  gnttab_setup_table(
>>      XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count,
>>      unsigned int limit_max)
>> @@ -1811,6 +1895,9 @@ gnttab_setup_table(
>>      struct grant_table *gt;
>>      unsigned int i;
>>
>> +    if ( is_vixen() )
>> +        return vixen_gnttab_setup_table(uop, count);
>> +
>>      if ( count != 1 )
>>          return -EINVAL;
>>
>> @@ -1892,6 +1979,26 @@ gnttab_setup_table(
>>  }
>>
>>  static long
>> +vixen_gnttab_query_size(
>> +    XEN_GUEST_HANDLE_PARAM(gnttab_query_size_t) uop, unsigned int count)
>> +{
>> +    struct gnttab_query_size op;
>> +    int rc;
>> +
>> +    if ( count != 1 )
>> +        return -EINVAL;
>> +
>> +    if ( unlikely(copy_from_guest(&op, uop, 1)) != 0)
>> +        return -EFAULT;
>> +
>> +    rc = HYPERVISOR_grant_table_op(GNTTABOP_query_size, &op, count);
>> +    if (rc == 0 && unlikely(__copy_to_guest(uop, &op, 1)) )
>            ^ nit: missing space

Ack.

Regards,

Anthony Liguori

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

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