On 04/05/16 09:03, Jordan Justen wrote:
> On 2016-03-14 05:53:23, Laszlo Ersek wrote:
>> These header files are intentionally minimal, and intentionally kept apart
>> from the VirtIo 0.9.5 headers.
>>
>> Cc: Ard Biesheuvel <[email protected]>
>> Cc: Jordan Justen <[email protected]>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <[email protected]>
>> ---
>> OvmfPkg/Include/IndustryStandard/Virtio10.h | 81 ++++++++++++++++++++
>> OvmfPkg/Include/IndustryStandard/Virtio10Net.h | 31 ++++++++
>> 2 files changed, 112 insertions(+)
>>
>> diff --git a/OvmfPkg/Include/IndustryStandard/Virtio10.h
>> b/OvmfPkg/Include/IndustryStandard/Virtio10.h
>> new file mode 100644
>> index 000000000000..722475c4ea55
>> --- /dev/null
>> +++ b/OvmfPkg/Include/IndustryStandard/Virtio10.h
>> @@ -0,0 +1,81 @@
>> +/** @file
>> + Definitions from the VirtIo 1.0 specification (csprd05).
>> +
>> + Copyright (C) 2016, Red Hat, Inc.
>> +
>> + This program and the accompanying materials are licensed and made
>> available
>> + under the terms and conditions of the BSD License which accompanies this
>> + distribution. The full text of the license may be found at
>> + http://opensource.org/licenses/bsd-license.php
>> +
>> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> WITHOUT
>> + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +**/
>> +
>> +#ifndef _VIRTIO_1_0_H_
>> +#define _VIRTIO_1_0_H_
>> +
>> +#include <Base.h>
>> +
>> +//
>> +// Structures for parsing the VirtIo 1.0 specific PCI capabilities from the
>> +// config space
>> +//
>> +#pragma pack (1)
>> +typedef struct {
>> + UINT8 CapId; // Capability identifier (generic)
>> + UINT8 CapNext; // Link to next capability (generic)
>> +} VIRTIO_PCI_CAP_LINK;
>> +
>> +typedef struct {
>> + UINT8 ConfigType; // Identifies the specific VirtIo 1.0 config structure
>> + UINT8 Bar; // The BAR that contains the structure
>> + UINT8 Padding[3];
>> + UINT32 Offset; // Offset within Bar until the start of the structure
>> + UINT32 Length; // Length of the structure
>> +} VIRTIO_PCI_CAP;
>> +#pragma pack ()
>> +
>> +//
>> +// Values for the VIRTIO_PCI_CAP.ConfigType field
>> +//
>> +#define VIRTIO_PCI_CAP_COMMON_CFG 1 // Common configuration
>> +#define VIRTIO_PCI_CAP_NOTIFY_CFG 2 // Notifications
>> +#define VIRTIO_PCI_CAP_DEVICE_CFG 4 // Device specific configuration
>> +
>> +//
>> +// Structure pointed-to by Bar and Offset in VIRTIO_PCI_CAP when ConfigType
>> is
>> +// VIRTIO_PCI_CAP_COMMON_CFG
>> +//
>> +#pragma pack (1)
>> +typedef struct {
>> + UINT32 DeviceFeatureSelect;
>> + UINT32 DeviceFeature;
>> + UINT32 DriverFeatureSelect;
>> + UINT32 DriverFeature;
>> + UINT16 MsixConfig;
>> + UINT16 NumQueues;
>> + UINT8 DeviceStatus;
>> + UINT8 ConfigGeneration;
>> + UINT16 QueueSelect;
>> + UINT16 QueueSize;
>> + UINT16 QueueMsixVector;
>> + UINT16 QueueEnable;
>> + UINT16 QueueNotifyOff;
>> + UINT64 QueueDesc;
>> + UINT64 QueueAvail;
>> + UINT64 QueueUsed;
>> +} VIRTIO_PCI_COMMON_CFG;
>> +#pragma pack ()
>> +
>> +//
>> +// VirtIo 1.0 device status bits
>> +//
>> +#define VSTAT_FEATURES_OK BIT3
>> +
>> +//
>> +// VirtIo 1.0 reserved (device-independent) feature bits
>> +//
>> +#define VIRTIO_F_VERSION_1 BIT32
>> +
>> +#endif // _VIRTIO_1_0_H_
>> diff --git a/OvmfPkg/Include/IndustryStandard/Virtio10Net.h
>> b/OvmfPkg/Include/IndustryStandard/Virtio10Net.h
>> new file mode 100644
>> index 000000000000..2befc661dc62
>> --- /dev/null
>> +++ b/OvmfPkg/Include/IndustryStandard/Virtio10Net.h
>> @@ -0,0 +1,31 @@
>> +/** @file
>> + Definitions from the VirtIo 1.0 specification (csprd05), specifically for
>> the
>> + network device.
>> +
>> + Copyright (C) 2016, Red Hat, Inc.
>> +
>> + This program and the accompanying materials are licensed and made
>> available
>> + under the terms and conditions of the BSD License which accompanies this
>> + distribution. The full text of the license may be found at
>> + http://opensource.org/licenses/bsd-license.php
>> +
>> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> WITHOUT
>> + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +**/
>> +
>> +#ifndef _VIRTIO_1_0_NET_H_
>> +#define _VIRTIO_1_0_NET_H_
>> +
>> +#include <IndustryStandard/VirtioNet.h>
>> +
>> +//
>> +// VirtIo 1.0 packet header
>> +//
>> +#pragma pack (1)
>> +typedef struct {
>> + VIRTIO_NET_REQ Legacy;
>
> This naming seems a bit unfortunate.
I can change the field name, sure.
> I thought the nested version structures seemed a bit strange for edk2.
Why?
> Then I took a look at EFI_ACPI_4_0_FIXED_ACPI_DESCRIPTION_TABLE vs
> EFI_ACPI_5_0_FIXED_ACPI_DESCRIPTION_TABLE.
>
> What are your thoughts?
I'm not sure about the exact justification for defining those structures
the way they are defined. I can imagine that nesting would have made no
sense for them. Personally, I would have defined the 5.0 FADT on top of
the 4.0 FADT, if only to save some space (in the source code).
However, the main bonus of nesting is not just saving source code. It is
about convertibility between pointers to the container structure and the
contained (first member) structure. In C99, "6.7.2.1 Structure and union
specifiers" paragraph 13 says,
[...] A pointer to a structure object, suitably converted, points
to its initial member [...] and vice versa. There may be
unnamed padding within a structure object, but not at its beginning.
Without nesting, you can't access the initial portion of the 5.0 struct
through a pointer to 4.0; the compiler might even decide to insert
different padding between the same fields in 4.0 vs. 5.0.
(This holds for C89 too.)
Additionally, if you look at the strict aliasing (effective type) rules,
in C99 "6.5 Expressions", paragraph 7:
An object shall have its stored value accessed only by an lvalue
expression that has one of the following types:
— a type compatible with the effective type of the object,
— a qualified version of a type compatible with the effective type
of the object,
— a type that is the signed or unsigned type corresponding to the
effective type of the object,
— a type that is the signed or unsigned type corresponding to a
qualified version of the effective type of the object,
— an aggregate or union type that includes one of the
aforementioned types among its members (including, recursively, a
member of a subaggregate or contained union), or
— a character type.
This means that, without nesting the 4.0 FADT into the 5.0 FADT, a
compiler that is allowed to enforce strict aliasing will assume that
pointers to 4.0 FADT and 5.0 FADT never alias; in other words, that
their pointed-to objects never overlap. However, if you embed the 4.0
FADT into the 5.0 FADT, then the compiler will "keep in mind" (due to
the penultimate bullet) that the target of the pointer-to-4.0 could
actually be part of the target of the pointer-to-5.0.
(The effective type rules were not spelled out this clearly in C89.)
So, nesting is generally superior.
There is one alternative (that keeps the above benefits): placing both
structures into a union. See "6.5.2.3 Structure and union members",
paragraph 5:
One special guarantee is made in order to simplify the use of
unions: if a union contains several structures that share a common
initial sequence (see below), and if the union object currently
contains one of these structures, it is permitted to inspect the
common initial part of any of them anywhere that a declaration of
the complete type of the union is visible. Two structures share a
common initial sequence if corresponding members have compatible
types (and, for bit-fields, the same widths) for a sequence of one
or more initial members.
(This is also guaranteed by C89.)
In patch 12/15 ("OvmfPkg: VirtioNetDxe: adapt virtio-net packet header
size to virtio-1.0") you can see how I'm using the Legacy field.
Embedding VIRTIO_NET_REQ into VIRTIO_1_0_NET_REQ is the natural choice.
I guess I could define VIRTIO_1_0_NET_REQ independently, without
embedding VIRTIO_NET_REQ. In that case however I'd have to introduce
another type (a union type) for patch 12/15.
But that would be strictly worse: not only would I have to call the
union member representing the old header "Legacy", I would also have to
call the union member representing the new header *something*.
So, I think:
- Nesting is natural, frugal, and standard C; we should use it.
- If you dislike the field name "Legacy", I can certainly rename the
field to, say, Virtio095NetReq, or something else you'd recommend.
Thanks
Laszlo
>
> -Jordan
>
>> + UINT16 NumBuffers;
>> +} VIRTIO_1_0_NET_REQ;
>> +#pragma pack ()
>> +
>> +#endif // _VIRTIO_1_0_NET_H_
>> --
>> 1.8.3.1
>>
>>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
