Re: [PATCH v2 2/3] OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O.

[Laszlo Ersek <lersek@redhat.com>]

On 04/05/17 11:16, Phil Dennis-Jordan wrote:
> On Tue, Apr 4, 2017 at 8:19 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 04/03/17 00:44, Phil Dennis-Jordan wrote:

>>> +/**
>>> +  Performs a 32-bit write to the specified, possibly unaligned I/O-type address.
>>> +
>>> +  Writes the 32-bit I/O port specified by Port with the value specified by Value
>>> +  and returns Value. This function must guarantee that all I/O read and write
>>> +  operations are serialized.
>>> +
>>> +  If 32-bit unaligned I/O port operations are not supported, then ASSERT().
>>> +
>>> +  @param[in]  Port   I/O port address
>>> +  @param[in]  Value  32-bit word to write
>>> +
>>> +  @return The value written to the I/O port.
>>> +
>>> +**/
>>> +UINT32
>>> +UnalignedIoWrite32 (
>>> +  IN      UINTN                     Port,
>>> +  IN      UINT32                    Value
>>> +  )
>>> +{
>>> +  __asm__ __volatile__ ( "outl %0, %1" : : "a"(Value), "Nd"((UINT16)Port) );
>>
>> Please insert a space after each second quote character:
>>
>> "a" (Value)
>>
>> "Nd" ((UINT16)Port)
>>
>> Also, a question: what does the N character (constraint?) do in the
>> input operand specification? I tried to check the gcc inline assembly
>> docs at <https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html>, and I
>> couldn't find it. Thanks.
> 
> The N constraint is x86-specific and indicates an 8-bit unsigned
> immediate value can be used as the operand. This enables emitting the
> OUT  imm8, EAX
> instruction variant, see
> https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html
> In practice, this is most likely not going to happen here, even if
> link time optimisation happens to inline the call, as the port number
> isn't hardcoded. I can remove it if you like.

Either a comment or removal would be fine, I think.

I feel slightly more attracted to removal because the "N" diverges from
IoWrite32() in "MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c", and that
difference seems to deserve justification.

Thanks
Laszlo