Re: [Xen-devel] [PATCH] vtpmmgr: fix 32-bit compilation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.04.14 at 22:39, <dgdegra@xxxxxxxxxxxxx> wrote:
> --- a/stubdom/vtpmmgr/vtpm_cmd_handler.c
> +++ b/stubdom/vtpmmgr/vtpm_cmd_handler.c
> @@ -451,6 +451,7 @@ static TPM_RESULT vtpmmgr_GroupActivate(tpmcmd_t* tpmcmd)
>   * mpi objects use little endian word ordering
>   */
>  static t_uint Pp[256 / sizeof(t_uint)] = {
> +#ifdef __x86_64__
>       0xFFFFFFFFFFFFFFFFUL, 0x15728E5A8AACAA68UL, 0x15D2261898FA0510UL,
>       0x3995497CEA956AE5UL, 0xDE2BCBF695581718UL, 0xB5C55DF06F4C52C9UL,
>       0x9B2783A2EC07A28FUL, 0xE39E772C180E8603UL, 0x32905E462E36CE3BUL,

Isn't looking for just a single architecture a little too specific here?
What about other 64-bit architectures, namely aarch64?

> @@ -462,6 +463,19 @@ static t_uint Pp[256 / sizeof(t_uint)] = {
>       0x302B0A6DF25F1437UL, 0xEF9519B3CD3A431BUL, 0x514A08798E3404DDUL,
>       0x020BBEA63B139B22UL, 0x29024E088A67CC74UL, 0xC4C6628B80DC1CD1UL,
>       0xC90FDAA22168C234UL, 0xFFFFFFFFFFFFFFFFUL,
> +#else
> +     0xFFFFFFFF, 0xFFFFFFFF, 0x8AACAA68, 0x15728E5A, 0x98FA0510, 0x15D22618,
> +     0xEA956AE5, 0x3995497C, 0x95581718, 0xDE2BCBF6, 0x6F4C52C9, 0xB5C55DF0,
> +     0xEC07A28F, 0x9B2783A2, 0x180E8603, 0xE39E772C, 0x2E36CE3B, 0x32905E46,
> +     0xCA18217C, 0xF1746C08, 0x4ABC9804, 0x670C354E, 0x7096966D, 0x9ED52907,
> +     0x208552BB, 0x1C62F356, 0xDCA3AD96, 0x83655D23, 0xFD24CF5F, 0x69163FA8,
> +     0x1C55D39A, 0x98DA4836, 0xA163BF05, 0xC2007CB8, 0xECE45B3D, 0x49286651,
> +     0x7C4B1FE6, 0xAE9F2411, 0x5A899FA5, 0xEE386BFB, 0xF406B7ED, 0x0BFF5CB6,
> +     0xA637ED6B, 0xF44C42E9, 0x625E7EC6, 0xE485B576, 0x6D51C245, 0x4FE1356D,
> +     0xF25F1437, 0x302B0A6D, 0xCD3A431B, 0xEF9519B3, 0x8E3404DD, 0x514A0879,
> +     0x3B139B22, 0x020BBEA6, 0x8A67CC74, 0x29024E08, 0x80DC1CD1, 0xC4C6628B,
> +     0x2168C234, 0xC90FDAA2, 0xFFFFFFFF, 0xFFFFFFFF,
> +#endif

And the resulting redundancy doesn't seem desirable either. Perhaps
this ought to be initialized as an array of bytes?

> @@ -469,15 +483,16 @@ static void tm_dhkx_gen(void* dhkx1, void* dhkx2, void* 
> out)
>  {
>       mpi GX = { 0 }, GY = { 0 }, K = { 0 }, RP = { 0 };
>  
> -     t_uint Xp[256 / sizeof(t_uint)];
> +     int XpElts = 256 / sizeof(t_uint);

If the two arrays need to be the same size (not sure whether that's
the case), an equivalent of ARRAY_SIZE(Pp) (open coded in the
current code as seen below) would seem to be the better alternative.

> +     t_uint Xp[XpElts];

Or maybe simply typeof(Pp) here, ...

>       mpi X = {
>               .s = 1,
> -             .n = sizeof(Xp)/sizeof(Xp[0]),
> +             .n = XpElts,
>               .p = Xp
>       };
>       mpi P = {
>               .s = 1,
> -             .n = sizeof(Pp)/sizeof(Pp[0]),
> +             .n = XpElts,

... and these two left as they were?

> @@ -487,8 +502,8 @@ static void tm_dhkx_gen(void* dhkx1, void* dhkx2, void* 
> out)
>       };
>  
>       do_random(Xp, sizeof(Xp));
> -     while (Xp[31] == 0 || Xp[31] == -1UL)
> -             do_random(Xp + 31, sizeof(Xp[31]));
> +     while (Xp[XpElts - 1] == 0 || Xp[XpElts - 1] == -1UL)

Wouldn't that better be ~(t_uint)0, or - without any cast -
!(Xp[XpElts - 1] + 1)?

> +             do_random(Xp + XpElts - 1, sizeof(Xp[0]));

Also it looks odd to me that this deals with a different number of
trailing bit depending on architecture - is that really correct?

Jan


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