Re: [Qemu-devel] [PATCH 12/12 v9] target-tilegx: Generate tcg instructions to execute to 1st system call

[Chen Gang]

On 4/11/15 05:56, Peter Maydell wrote:
> On 10 April 2015 at 22:28, Chen Gang <address@hidden> wrote:
>> On 4/10/15 06:19, Peter Maydell wrote:
>>> On 27 March 2015 at 11:07, Chen Gang <address@hidden> wrote:
>>>> +/*
>>>> + * The related functional description for bfextu in isa document:
>>>> + *
>>>> + * uint64_t mask = 0;
>>>> + * mask = (-1ULL) ^ ((-1ULL << ((BFEnd - BFStart) & 63)) << 1);
>>>> + * uint64_t rot_src = (((uint64_t) rf[SrcA]) >> BFStart)
>>>> + *                    | (rf[SrcA] << (64 - BFStart));
>>>> + * rf[Dest] = rot_src & mask;
>>>> + */
>>>> +static void gen_bfextu(struct DisasContext *dc, uint8_t rdst, uint8_t 
>>>> rsrc,
>>>> +                       int8_t start, int8_t end)
>>>> +{
>>>> +    uint64_t mask = (-1ULL) ^ ((-1ULL << ((end - start) & 63)) << 1);
>>>> +    TCGv tmp = dest_gr(dc, rdst);
>>>
>>> Are the start and end immediates here limited such that we're
>>> guaranteed not to hit any of C's undefined behaviour for out
>>> of range shifts, and that we don't hit TCG's undefined-value
>>> behaviour on bad rotates?
>>>
>>
>> For me, it is correct, it is only the copy of the document, which has
>> already considered about any cases (include C's undefined behaviour).
> 
> Even if the ISA document implicitly (or explicitly) permits
> some values of start and end to be undefined behaviour of the
> CPU, you must not either have C code in translate.c that does
> undefined behaviour, or emit generated code that is undefined
> behaviour by TCG's rules (undefined values may be OK). You
> need to make sure QEMU can't crash or misbehave if the guest
> passes us badly encoded or meaningless instructions.
> 
> You need to work through the possibilities and convince
> yourselves (and us) that everything is correctly handled.
> It's not enough to just say "it's a copy of the C code from
> the ISA so it must be OK".
> 

Oh, maybe we misunderstood the "C's undefined behaviour". in our case:

 - if end < start, it is OK (it is also one of important cases).

 - start and end are within 0 - 63 (they have 6 bits), but they still
   need "& 63" for end < start case. (I guess, it will be better to use
   uint8_t instead of my original int8_t for start and end). 

The related introduction is:

  bfextu

      Bit field Extract Unsigned

      Syntax

             bfextu Dest, SrcA, BFStart, BFEnd

      Example

             bfextu r5, r6, 5, 7

      Description

             Extract and right justify the specified bit field of the
             destination operand. The bit field is specified by the
             BFStart and BFEnd immediate operands, which contain the bit
             fields starting and ending bit positions. If the start
             position is less than or equal to the end position, then
             the field contains bits from start bit position up to and
             including the ending bit position. If the start position is
             greater than the end position, then the field contains the
             bits start bit position up to the WORD_SIZE bit position,
             and from the zero bit position up to the end bit position.

      Functional Description

             uint64_t mask = 0;
             mask = ((-1ULL) ^ ((-1ULL << ((BFEnd - BFStart) & 63)) << 1));
             uint64_t rot_src =
               (((uint64_t) rf[SrcA]) >> BFStart) | (rf[SrcA] << (64 - 
BFStart));
             rf[Dest] = rot_src & mask;


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed