Re: [Xen-devel] [PATCH 05/24] xen: Preserve reserved grant entries when switching versions

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 01/27/2012 04:54 AM, Ian Campbell wrote:
> It would be worth CCing the relevant maintainers for each patch in the
> series. e.g. Keir in this case.

OK, will do that for v6.

> On Thu, 2012-01-26 at 19:44 +0000, Daniel De Graaf wrote:
>> In order for the toolstack to use reserved grant table entries, the
>> grant table for a guest must be initialized prior to the guest's boot.
>> When the guest switches grant table versions (necessary if the guest is
>> using v2 grant tables, or on kexec if switching grant versions), these
>> initial grants will be cleared. Instead of clearing them, preserve
>> the grants across the type change.
>>
>> Attempting to preserve v2-only features such as sub-page grants will
>> produce a warning and invalidate the resulting v1 grant entry.
>>
>> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>> ---
>>  xen/common/grant_table.c         |   48 
>> +++++++++++++++++++++++++++++++++----
>>  xen/include/public/grant_table.h |    7 +++++
>>  2 files changed, 49 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index 0c55fd1..6f24a94 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -2111,6 +2111,7 @@ 
>> gnttab_set_version(XEN_GUEST_HANDLE(gnttab_set_version_t uop))
>>      struct domain *d = current->domain;
>>      struct grant_table *gt = d->grant_table;
>>      struct active_grant_entry *act;
>> +    grant_entry_v1_t reserved_entries[GNTTAB_NR_RESERVED_ENTRIES];
>>      long res;
>>      int i;
>>  
>> @@ -2131,7 +2132,7 @@ 
>> gnttab_set_version(XEN_GUEST_HANDLE(gnttab_set_version_t uop))
>>      /* (You need to change the version number for e.g. kexec.) */
>>      if ( gt->gt_version != 0 )
>>      {
>> -        for ( i = 0; i < nr_grant_entries(gt); i++ )
>> +        for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < nr_grant_entries(gt); i++ 
>> )
> 
> The comment just prior says:
>     /* Make sure that the grant table isn't currently in use when we
>        change the version number. */
> I think this needs updating to note that we do allow reserved entries to
> be active during the switch over and we will correctly preserve
> flags/status/mapped-ness etc.

Right.

>>          {
>>              act = &active_entry(gt, i);
>>              if ( act->pin != 0 )
>> @@ -2156,15 +2157,50 @@ 
>> gnttab_set_version(XEN_GUEST_HANDLE(gnttab_set_version_t uop))
>>              goto out_unlock;
>>      }
>>  
>> +    /* Preserve the first 8 entries (toolstack reserved grants) */
>> +    if (gt->gt_version == 1)
> 
> Xen coding style has extra spaces just inside the braces (and again
> below a few more times).
> 
>> +    {
>> +        memcpy(reserved_entries, gt->shared_v1[0], 
>> sizeof(reserved_entries));
> 
> Shouldn't that be either "gt->shared_v1" or "&gt->shared_v1[0]" ?
> 

No, [0] means this is copying from the first page; the first entry would be
gt->shared_v1[0][0]. &shared_entry_v1(gt, 0) may be clearer here; I'll use that.

>> +    }
>> +    else if (gt->gt_version == 2)
>> +    {
>> +        for ( i = 0; i < GNTTAB_NR_RESERVED_ENTRIES && i < 
>> nr_grant_entries(gt); i++ )
>> +        {
>> +            reserved_entries[i].flags = shared_entry_v2(gt, i).hdr.flags;
>> +            reserved_entries[i].domid = shared_entry_v2(gt, i).hdr.domid;
>> +            reserved_entries[i].frame = shared_entry_v2(gt, 
>> i).full_page.frame;
>> +            reserved_entries[i].flags |= status_entry(gt, i);
>> +            if ((reserved_entries[i].flags & GTF_type_mask) > 
>> GTF_permit_access)
> 
> In effect this only allows GTF_permit_access or GTF_invalid, which is
> good. It would be more obvious/explicit to do
>       if ((shared_entry_v2(gt, i).hdr.flags & GTF_type_mask) != GTF_invalid &&
>           (shared_entry_v2(gt, i).hdr.flags & GTF_type_mask) != 
> GTF_permit_access)
>               memset-whole-entry and continue;
> at the top or even a switch().

In that case I think it would be clearer to only populate the entry if 
GTF_permit_access
and clear it otherwise (warning if not already GTF_invalid).

>> +            {
>> +                gdprintk(XENLOG_INFO, "d%d: bad flags %x in grant %d when 
>> switching grant version\n",
>> +                       d->domain_id, reserved_entries[i].flags, i);
>> +                reserved_entries[i].flags = GTF_invalid;
>> +            }
>> +        }
>> +    }
>> +
>>      if ( op.version < 2 && gt->gt_version == 2 )
>>          gnttab_unpopulate_status_frames(d, gt);
>>  
>> -    if ( op.version != gt->gt_version )
>> +    /* Make sure there's no crud left over in the table from the
>> +       old version. */
>> +    for ( i = 0; i < nr_grant_frames(gt); i++ )
>> +        memset(gt->shared_raw[i], 0, PAGE_SIZE);
>> +
>> +    /* Restore the first 8 entries (toolstack reserved grants) */
>> +    if (gt->gt_version != 0 && op.version == 1)
>>      {
>> -        /* Make sure there's no crud left over in the table from the
>> -           old version. */
>> -        for ( i = 0; i < nr_grant_frames(gt); i++ )
>> -            memset(gt->shared_raw[i], 0, PAGE_SIZE);
>> +        memcpy(gt->shared_v1[0], reserved_entries, 
>> sizeof(reserved_entries));
>> +    }
>> +    else if (gt->gt_version != 0 && op.version == 2)
>> +    {
>> +        for ( i = 0; i < GNTTAB_NR_RESERVED_ENTRIES; i++ )
>> +        {
>> +            status_entry(gt, i) = reserved_entries[i].flags & 
>> (GTF_reading|GTF_writing);
>> +            shared_entry_v2(gt, i).hdr.flags = reserved_entries[i].flags & 
>> ~(GTF_reading|GTF_writing);
>> +            shared_entry_v2(gt, i).hdr.domid = reserved_entries[i].domid;
>> +            shared_entry_v2(gt, i).full_page.frame = 
>> reserved_entries[i].frame;
>> +        }
>>      }
>>  
>>      gt->gt_version = op.version;
>> diff --git a/xen/include/public/grant_table.h 
>> b/xen/include/public/grant_table.h
>> index 54d9551..292d724 100644
>> --- a/xen/include/public/grant_table.h
>> +++ b/xen/include/public/grant_table.h
>> @@ -117,6 +117,13 @@ struct grant_entry_v1 {
>>  };
>>  typedef struct grant_entry_v1 grant_entry_v1_t;
>>  
>> +/* The first few grant table entries will be preserved across grant table
>> + * version changes and may be pre-populated at domain creation by tools.
>> + */
>> +#define GNTTAB_NR_RESERVED_ENTRIES     8
>> +#define GNTTAB_RESERVED_CONSOLE        0
>> +#define GNTTAB_RESERVED_XENSTORE       1
>> +
>>  /*
>>   * Type of grant entry.
>>   *  GTF_invalid: This grant entry grants no privileges.
> 

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