On 2016-04-05 01:31:27, Laszlo Ersek wrote: > 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
How about including Virtio10.h from Virtio.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> How about instead we have VirtioNet.h include Virtio10Net.h? What about naming this VirtioNet10.h instead? > >> + > >> +// > >> +// 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? > For some reason, it just seems out of place in EDK II. I don't personally have a problem with nesting structures, but I'm just trying to think about what this would look like if it lived in MdePkg instead. > > Then I took a look at EFI_ACPI_4_0_FIXED_ACPI_DESCRIPTION_TABLE vs > > EFI_ACPI_5_0_FIXED_ACPI_DESCRIPTION_TABLE. > > PCI_3_0_DATA_STRUCTURE and PCI_DATA_STRUCTURE seem to be another example, but in that case the structure is not just being extended, so it seems more natural to not embed v2.2 of the structure. > > 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. > Virtio095NetReq, Virtio095, V0_95, _0_95, or Base all seem better to me. I guess Base or V0_95 would be my preference. -Jordan > > > > > -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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
