Re: [Xen-devel] [PATCH v3] x86/hvm/viridian: flush remote tlbs by hypercall

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.11.15 at 10:15, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
>> Sent: 19 November 2015 17:09
>> On 19/11/15 16:57, Paul Durrant wrote:
>> >>> +        /*
>> >>> +         * For each specified virtual CPU flush all ASIDs to invalidate
>> >>> +         * TLB entries the next time it is scheduled and then, if it
>> >>> +         * is currently running, add its physical CPU to a mask of
>> >>> +         * those which need to be interrupted to force a flush.
>> >>> +         */
>> >>> +        for_each_vcpu ( currd, v )
>> >>> +        {
>> >>> +            if ( !(input_params.vcpu_mask & (1ul << v->vcpu_id)) )
>> >>> +                continue;
>> >> You need to cap this loop at a vcpu_id of 63, or the above conditional
>> >> will become undefined.
>> > The guest should not issue the hypercall if it has more than 64 vCPUs so to
>> some extend I don't care what happens, as long as it is not harmful to the
>> hypervisor in general, and I don't think that it is in this case.
>> 
>> The compiler is free to do anything it wishes when encountering
>> undefined behaviour, including crash the hypervisor.
>> 
> 
> I don't believe so. See section 6.5.7 of the C99 spec. (top pf page 85):
> 
> "The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are 
> filled with
> zeros. If E1 has an unsigned type, the value of the result is E1 x 2^E2, 
> reduced modulo
> one more than the maximum value representable in the result type. If E1 has 
> a signed
> type and nonnegative value, and E1 x 2^E2 is representable in the result 
> type, then that is
> the resulting value; otherwise, the behavior is undefined"
> 
> So, the undefined behaviour you refer to is only in the case that E1 is 
> signed and in this case it is most definitely unsigned, so the modulo rule 
> applies.

Looks like you missed the earlier clause 3 (at the bottom of page 84):
"If the value of the right operand is negative or is greater than or
 equal to the width of the promoted left operand, the behavior is
 undefined." And if you look at generated code, I think you'll find
that the result is not zero for too large shift counts, as would need
to be the case if that clause wasn't there.

I'll actually make my previous Reviewed-by dependent on this issue
getting fixed.

>> Any undefined-behaviour which can be triggered by a guest action
>> warrants an XSA, because there is no telling what might happen.

Well, it warrants fixing, but I don't think it unconditionally would
be XSA-worthy. After all, the compiler (normally) does sane
things in response - in order to crash the hypervisor or produce
truly undefined behavior, it would need to generate extra code
(since other than in C, at the assembly level the instruction's
behavior is defined), which I think we can take for granted it
won't.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel