On 2/22/19 16:08, Jan Beulich wrote:
>>>> On 21.02.19 at 09:16, <nmant...@amazon.de> wrote:
>> @@ -226,10 +228,18 @@ nr_maptrack_frames(struct grant_table *t)
>> static grant_entry_header_t *
>> shared_entry_header(struct grant_table *t, grant_ref_t ref)
>> {
>> - if ( t->gt_version == 1 )
>> + switch ( t->gt_version )
>> + {
>> + case 1:
>> + /* Make sure we return a value independently of speculative
>> execution */
>> + block_speculation();
>> return (grant_entry_header_t*)&shared_entry_v1(t, ref);
>> - else
>> + case 2:
>> + /* Make sure we return a value independently of speculative
>> execution */
>> + block_speculation();
>> return &shared_entry_v2(t, ref).hdr;
>> + }
>> + return NULL;
>> }
> I'm not happy with the comment wording, as to me it reads ambiguously
> at best. How about "Make sure the value returned is independent of
> speculative execution"? I'm of course open to even better suggestions,
> in particular by native speakers.
>
> Also please add a blank line
> - between the individual case blocks,
> - before the final (main) return statement.
>
> Plus please add ASSERT_UNREACHABLE() ahead of the NULL return.
I will adapt the function accordingly.
>
>> @@ -963,9 +979,15 @@ map_grant_ref(
>> PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>> op->ref, rgt->domain->domain_id);
>>
>> + /* Make sure the above bound check cannot be bypassed speculatively */
>> + block_speculation();
>> +
>> act = active_entry_acquire(rgt, op->ref);
>> shah = shared_entry_header(rgt, op->ref);
> So shared_entry_header() now has a fence before consuming op->ref.
> Is there anything wrong with swapping these two and dropping the
> extra fence you add above, like you do in acquire_grant_for_copy()? All
> this would seem to require is adding block_speculation() also onto the
> "return NULL" path of shared_entry_header() (where it shouldn't hurt
> at all).
I will swap the two function calls, I missed that in the previous
iteration. I will add the block_speculation() and ASSERT_UNREACHABLE()
at the end of the shared_entry_header function.
>
>> @@ -2946,6 +2981,7 @@
>> gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
>> grant_entry_v1_t reserved_entries[GNTTAB_NR_RESERVED_ENTRIES];
>> int res;
>> unsigned int i;
>> + unsigned int gt_nr_grant_entries;
> Rather then lengthening the name by adding a disambiguating prefix,
> how about shortening it to "nr" or "nr_ents" (also elsewhere)? Also
> please add onto the line declaring i instead of adding yet another line
> with the same type.
I will rename the variable to nr_ents and move it to the line above.
>
>> @@ -2969,7 +3005,8 @@
>> gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
>> * are allowed to be in use (xenstore/xenconsole keeps them mapped).
>> * (You need to change the version number for e.g. kexec.)
>> */
>> - for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < nr_grant_entries(gt); i++ )
>> + gt_nr_grant_entries = nr_grant_entries(gt);
>> + for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < gt_nr_grant_entries; i++ )
> This then also calls for a 3rd block_speculation() in nr_grant_entries(),
> I think.
I'll add that one as well.
Best,
Norbert
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
