Re: [PATCH 11/14] OvmfPkg/MptScsiDxe: Initialize hardware

[Laszlo Ersek <lersek@redhat.com>]

Hi Nikita,

I've jumped ahead a little bit to point out other style issues, so you
can address them at once in v2. These are again general comments, likely
applying to several patches in the series.

On 01/31/19 11:07, Nikita Leshenko wrote:

> +typedef struct {
> +  UINT8     WhoInit;
> +  UINT8     Reserved1;
> +  UINT8     ChainOffset;
> +  UINT8     Function;
> +  UINT8     Flags;
> +  UINT8     MaxDevices;
> +  UINT8     MaxBuses;
> +  UINT8     MessageFlags;
> +  UINT32    MessageContext;
> +  UINT16    ReplyFrameSize;
> +  UINT16    Reserved2;
> +  UINT32    HostMfaHighAddr;
> +  UINT32    SenseBufferHighAddr;
> +} __attribute__((aligned(8), packed)) MPT_IO_CONTROLLER_INIT_REQUEST; // Align required by HW


(1) For packing, you need to say:

#pragma pack (1)
...
#pragma pack ()


(2) For enforcing an alignment of 8 bytes, I think you'll have to create
an outer union first, and place a UINT64 member into it, as one member.
The other member can be the structure you actually need. And, all
further variable definitions (with static or auto storage duration) will
have to occur through the union type. For dynamically allocated objects,
the maximum alignment is ensured anyway.

Perhaps other edk2 maintainers can give you a better tip for enforcing a
minimum alignment for local & global variables; I'm not aware of any
other portable method than the union.

> +STATIC
> +EFI_STATUS
> +MptDoorbell (
> +  IN MPT_SCSI_DEV       *Dev,
> +  IN UINT8              DoorbellFunc,
> +  IN UINT8              DoorbellArg
> +  )
> +{
> +  return Out32 (Dev, MPT_REG_DOORBELL, (DoorbellFunc << 24) | (DoorbellArg << 16));
> +}

(3) The DoorbellFunc variable is of type UINT8, which (in practice) is
"unsigned char". As the operand of the << operator, it is promoted to
"int" (because "int" can represent all values of "unsigned char", on all
the platforms that we care about). In practice, our "int" representation
is "31 value bits, 1 sign bit, no padding bits, and two's complement
representation". When you left shift a UINT8 value by 24 bits, it's
possible that you sign a nonzero bit into the sign bit -- more
precisely, the mathematical result of the bitwise left shift cannot be
represented in the signed int result. According to the specification of
the << operator, this is undefined behavior.

In other words, please cast DoorbellFunc to UINT32 before applying the
shift.

> +  MPT_IO_CONTROLLER_INIT_REQUEST Req = {
> +    .WhoInit = MPT_IOC_WHOINIT_ROM_BIOS,
> +    .Function = MPT_MESSAGE_HDR_FUNCTION_IOC_INIT,
> +    .MaxDevices = 1,
> +    .MaxBuses = 1,
> +    .ReplyFrameSize = sizeof (MPT_SCSI_IO_ERROR_REPLY),
> +  };

(4) Okay, so this uses two language features simultaneously that are not
permitted in edk2 code:

- declaration that is not at the top of a block,
- designated initializer.

You'll have to set these fields one by one (with assignments), or else
introduce a static constant object as a "template", and then set the
local variable with CopyMem().

(In edk2 you can't use structure assignment either, you can only assign
scalars. This limitation is not from the C language of course; it is
because otherwise some compilers cannot be prevented from generating
calls to intrinsics, which are not available in the firmware execution
environment.)

I guess I'll also mention that compound literals are forbidden too.
Basically, stick with C89.

Thanks
Laszlo