Re: [PATCH 2/3] Grant tables v2 implementation.

[annie li <annie.li@oracle.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4862 bytes --]



On 2011-11-9 21:32, Ian Campbell wrote:
> On Wed, 2011-11-09 at 08:15 +0000, annie.li@oracle.com wrote:
>   
>> From: Annie Li <annie.li@oracle.com>
>>
>> Receiver-side copying of packets is based on this implementation, it gives
>> better performance and better CPU accounting. It totally supports three types:
>> full-page, sub-page and transitive grants.
>>
>> However this patch does not cover sub-page and transitive grants, it mainly
>> focus on Full-page part and implements grant table V2 interfaces corresponding
>> to what already exists in grant table V1, such as: grant table V2
>> initialization, mapping, releasing and exported interfaces.
>>
>> Each guest can only supports one type of grant table type, every entry in grant
>> table should be the same version. It is necessary to set V1 or V2 version before
>> initializing the grant table.
>>
>> Grant table exported interfaces of V2 are same with those of V1, Xen is
>> responsible to judge what grant table version guests are using in every grant
>> operation.
>>
>> V2 fulfills the same role of V1, and it is totally backwards compitable with V1.
>> If dom0 support grant table V2, the guests runing on it can run with either V1
>> or V2.
>>
>> Signed-off-by: Annie Li <annie.li@oracle.com>
>> ---
>>  arch/x86/xen/grant-table.c |   34 ++++++++-
>>  drivers/xen/grant-table.c  |  194 +++++++++++++++++++++++++++++++++++++++++---
>>  include/xen/grant_table.h  |    6 +-
>>  3 files changed, 220 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c
>> index 65ce508..3e9f936 100644
>> --- a/arch/x86/xen/grant-table.c
>> +++ b/arch/x86/xen/grant-table.c
>> @@ -54,6 +54,17 @@ static int map_pte_fn(pte_t *pte, struct page *pmd_page,
>>         return 0;
>>  }
>>
>> +/*32bits is not enough since Xen supports sparse physical memory*/
>>     
>
> What does this mean?
>   
Sorry for the confusion, my comment seems not so precise.
This function is used to map the status frames of grant table 2, and the 
status definition is uint64_t. So it requires 64bits even on both 32bit 
guest.
>   
>> +static int map_pte_fn_status(pte_t *pte, struct page *pmd_page,
>> +                            unsigned long addr, void *data)
>> +{
>> +       uint64_t **frames = (uint64_t **)data;
>> +
>> +       set_pte_at(&init_mm, addr, pte, mfn_pte((*frames)[0], PAGE_KERNEL));
>> +       (*frames)++;
>> +       return 0;
>> +}
>>     
>
> Isn't this function identical to map_pte_fn?
>
>   
Yes, map_pte_fn already exists, but the frames definition is different.

uint64_t **frames in map_pte_fn_status  VS unsigned long **frames in 
map_pte_fn

Grant table 2 added status frames which is defined as uint64_t, so I 
kept both map_pte_fn and map_pte_fn_status.
>> +
>>  static int unmap_pte_fn(pte_t *pte, struct page *pmd_page,
>>                         unsigned long addr, void *data)
>>  {
>>     
>
>   
>> @@ -630,6 +756,44 @@ static int gnttab_map_status_v1(unsigned int nr_gframes)
>>         return 0;
>>  }
>>
>> +static int gnttab_map_status_v2(unsigned int nr_gframes)
>> +{
>> +       uint64_t *sframes;
>> +       unsigned int nr_sframes;
>> +       struct gnttab_get_status_frames getframes;
>> +       int rc;
>> +
>> +       nr_sframes = nr_status_frames(nr_gframes);
>> +
>> +       /* No need for kzalloc as it is initialized in following hyercall
>>     
>
>                                                                    hypercall
>   
Thanks, will fix it.
>   
>> +        * GNTTABOP_get_status_frames.
>> +        */
>> +       sframes = kmalloc(nr_sframes  * sizeof(uint64_t), GFP_ATOMIC);
>> +       if (!sframes)
>> +               return -ENOMEM;
>> +
>> +       getframes.dom        = DOMID_SELF;
>> +       getframes.nr_frames  = nr_sframes;
>> +       set_xen_guest_handle(getframes.frame_list, sframes);
>> +
>> +       rc = HYPERVISOR_grant_table_op(GNTTABOP_get_status_frames,
>> +                                      &getframes, 1);
>> +       if (rc == -ENOSYS) {
>> +               kfree(sframes);
>> +               return -ENOSYS;
>> +       }
>> +
>> +       BUG_ON(rc || getframes.status);
>> +
>> +       rc = arch_gnttab_map_status(sframes, nr_sframes,
>> +                                   nr_status_frames(gnttab_max_grant_frames()),
>> +                                   &grstatus);
>> +       BUG_ON(rc);
>> +       kfree(sframes);
>> +
>> +       return 0;
>> +}
>> +
>>  static int gnttab_map_status(unsigned int nr_gframes)
>>  {
>>         return gnttab_interface._gnttab_map_status(nr_gframes);
>> @@ -666,6 +830,9 @@ static int gnttab_map(unsigned int start_idx, unsigned int end_idx)
>>                 return rc;
>>         }
>>
>> +       /* No need for kzalloc as it is initialized in following hyercall
>>     
>
> "hypercall" again
>   
Thanks, will fix it.

Thanks
Annie
> Ian.
>
>
>   

[-- Attachment #1.2: Type: text/html, Size: 6084 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel