| Message ID | 1457960012-29481-7-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <ard.biesheuvel@linaro.org> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> 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 >> >>
On 5 April 2016 at 18:08, Jordan Justen <jordan.l.justen@intel.com> wrote: > 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 <ard.biesheuvel@linaro.org> >> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> >> --- >> >> 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. > Isn't that used for device path structs all the time? -- Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
(I've snipped liberally below, but I've been careful to follow up on all of your comments.) On 04/05/16 18:08, Jordan Justen wrote: > 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 <ard.biesheuvel@linaro.org> >>>> Cc: Jordan Justen <jordan.l.justen@intel.com> >>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >>>> --- >>>> 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? Okay... What for? Definitions exposed by either header do not depend on definitions from the other header. [snip] >>>> 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? Sure... but why? The legacy interface defines the structure (VIRTIO_NET_REQ) that is re-used in the modern interface (VIRTIO_1_0_NET_REQ). > What about naming this VirtioNet10.h instead? I've been extremely concsious and careful to insert the "10" and "1_0" (as appropriate) strings right after the "virtio" prefix. The "modernity" (the 1.0 thing) characterizes the entire virtio spec first, and it may have consequences for various device types second. > >>>> + >>>> +// >>>> +// 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. Ard mentions the good example of device path protocols for nesting (thank you, Ard). Also, this stuff definitely doesn't target MdePkg; similarly to the other files under "OvmfPkg/Include/IndustryStandard". (Not the smallest of which is the entire Xen subdirectory, imported whole-sale and very lightly touched up from Linux, ignoring the edk2 traditions almost completely...) "ArmPkg/Include/IndustryStandard" is also separate. ... Clearly, I never try to "break" MdePkg requirements as some "sneaky goal" in OvmfPkg, but please let us not start applying MdePkg requirements to patches that do not target MdePkg. The virtio spec is a specification about virtualization. Whatever we need of it belongs under OvmfPkg. I believe the patch doesn't conflict with the edk2 coding style document. >>> 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. Yes, not sharing the initial fields in those structures (through a common sub-structure) seems more acceptable to me as well. Another example (for embedding) is PCI_DEVICE_INDEPENDENT_REGION, in "MdePkg/Include/IndustryStandard/Pci22.h". Said struct is embedded in both PCI_TYPE00 and PCI_TYPE01. And PCI_TYPE00 and PCI_TYPE01 don't even encode different versions of the same specification, they encode entirely separate specifications. Quoting their comments: - PCI_TYPE00: Section 6.1, PCI Local Bus Specification, 2.2 - PCI_TYPE01: Section 3.2, PCI-PCI Bridge Architecture, Version 1.2 [snip] >> 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. I think V0_95 is great. I'll rename the field in the next version of the series. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 04/05/16 19:26, Jordan Justen wrote: > On 2016-04-05 09:46:33, Laszlo Ersek wrote: >> (I've snipped liberally below, but I've been careful to follow up on all >> of your comments.) >> >> On 04/05/16 18:08, Jordan Justen wrote: >>> 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 <ard.biesheuvel@linaro.org> >>>>>> Cc: Jordan Justen <jordan.l.justen@intel.com> >>>>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >>>>>> --- >>>>>> 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? >> >> Okay... What for? Definitions exposed by either header do not depend on >> definitions from the other header. >> > > Once again, because this follows the example of Acpi.h and Pci.h... Wait a minute, that's not a good analogy. (a) Looking at Acpi.h and Pci.h, those are "catch-all" headers. They say, "if you include me, I'll give you everything ACPI (or everything PCI), across all versions of the relevant standards". (b) Furthermore, regarding any such header file there that concerns itself with a specific version of the PCI or ACPI specs, the header with the higher version number includes the header with the lower version number. Specifics: - Acpi.h -> Acpi61.h -> Acpi60.h -> Acpi51.h -> Acpi50.h -> ... - Pci.h -> Pci30.h -> Pci23.h -> Pci22.h -> PciExpress21.h -> PciExpress30.h -> PciExpress21.h (strange...) As I said, this is not a good analogy. Because: Virtio.h never wanted to be a catch-all header. To closely imitate the Acpi.h and Pci.h situation above, I would have to rename Virtio.h to Virtio095.h first, then include it in Virtio10.h, then introduce a *new* file called Virtio.h, which should include Virtio10.h. At the moment, Virtio.h is not the catch-all header, it is actually the base (= lowest version number) header. So your suggestion would establish the inverse direction, compared to Acpi.h and Pci.h. Now, if you indeed proposed Virtio.h [new] -> Virtio10.h -> Virtio095.h [to be renamed from the current Virtio.h] then I would say, "a lot of churn for nothing". >>> 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. >> >> Ard mentions the good example of device path protocols for nesting >> (thank you, Ard). >> > > No, this is not a good example. What seems odd (for EDK II) is > embedding structures to extend the fields for a newer version of the > spec. I named another example, PCI_TYPE00 and PCI_TYPE01, both including PCI_DEVICE_INDEPENDENT_REGION as first member, all the while being separated from each other by significantly more than a version change: they correspond to different specs. My point is, it doesn't seem consistent to forbid sub-structure extraction for inter-version sharing, when there is a (quite central) example for *inter-spec* sharing (i.e., bridging a much larger conceptual distance). Anyway, I don't want to obsess about this any longer; as far as I understand, you agreed to embedding VIRTIO_NET_REQ, conditional on my renaming of the Legacy field, which I will certainly do. Thanks. > I wonder if Mike or Andrew have any thoughts about why the ACPI > includes (and arguably EDK II) go out of their way to define flattened > (duplicated) structures for different spec versions. Yes, that would be interesting to know. I wouldn't generalize the question to the entirety of edk2 (see especially the PCI_TYPE00 and PCI_TYPE01 counter-example above), but the ACPI examples are strange. ... I can make one attempt at speculating: I guess each version of the ACPI spec defines the then-current ACPI structures as flat lists of members. In other words, they do not refer to earlier versions of the spec. In turn, MdePkg developers probably find it easier to trans-code the spec to C source by following the flat listing. Extracting a common sub-structure would be *more* work. This does not apply to the virtio specs however, as far as VIRTIO_1_0_NET_REQ and VIRTIO_NET_REQ. Both 0.9.5 and 1.0 have language that calls the "num_buffers" field an *addition* to VIRTIO_NET_REQ. > Once again, I personally don't mind the sub-structure based on > versions if Mike and Andrew don't have anything to add... Alright, let's see. Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 04/05/16 19:46, Andrew Fish wrote: > >> On Apr 5, 2016, at 10:26 AM, Jordan Justen <jordan.l.justen@intel.com> wrote: >> >> I don't think it conflicts either. >> >> I wonder if Mike or Andrew have any thoughts about why the ACPI >> includes (and arguably EDK II) go out of their way to define flattened >> (duplicated) structures for different spec versions. >> >> Once again, I personally don't mind the sub-structure based on >> versions if Mike and Andrew don't have anything to add... > > Jordan, > > I'm not sure why things like GENERIC_ADDRESS_STRUCTURE keep getting > redefined even though the definition > EFI_ACPI_5_0_GENERIC_ADDRESS_STRUCTURE is the same as > EFI_ACPI_5_0_GENERIC_ADDRESS_STRUCTURE. I apologize for repeating my speculation / suspicion from my other message, but, again, I believe it was coded up like this simply because a straight trans-coding of the flat member list, from the spec, into C source, was easier and less error-prone for the developer, than laborously locating inter-version identities, and extracting them as common sub-structs. Using your example, in ACPI 5.1, "Table 5-26 Generic Address Structure (GAS)" is just a list of fields; it doesn't say "this structure is unchanged relative to ACPI 5.0". So the developer goes ahead and codes up EFI_ACPI_5_1_GENERIC_ADDRESS_STRUCTURE straight from the 5.1 spec, without looking at EFI_ACPI_5_0_GENERIC_ADDRESS_STRUCTURE. I think. > Some things are actually different, for example > EFI_ACPI_5_0_FIXED_ACPI_DESCRIPTION_TABLE has more entries than > EFI_ACPI_4_0_FIXED_ACPI_DESCRIPTION_TABLE. > > Maybe Mike remembers the reason? > > Seems like changing it now risks breaking existing code. Absolutely. I think there is no suggestion to extract common sub-structures now. The question is if we should imitate this open-coding / duplication of fields in new code, when there is every reason not to. (The reasons including: sub-structure extraction is natural, frugal, and standard C, and in this specific case, it even reflects the wording of the underlying specs). Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 04/05/16 20:20, Jordan Justen wrote: > On 2016-04-05 11:06:39, Laszlo Ersek wrote: >> On 04/05/16 19:26, Jordan Justen wrote: >>> On 2016-04-05 09:46:33, Laszlo Ersek wrote: >>>> (I've snipped liberally below, but I've been careful to follow up on all >>>> of your comments.) >>>> >>>> On 04/05/16 18:08, Jordan Justen wrote: >>>>> 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 <ard.biesheuvel@linaro.org> >>>>>>>> Cc: Jordan Justen <jordan.l.justen@intel.com> >>>>>>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >>>>>>>> --- >>>>>>>> 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? >>>> >>>> Okay... What for? Definitions exposed by either header do not depend on >>>> definitions from the other header. >>>> >>> >>> Once again, because this follows the example of Acpi.h and Pci.h... >> >> Wait a minute, that's not a good analogy. >> >> (a) Looking at Acpi.h and Pci.h, those are "catch-all" headers. They >> say, "if you include me, I'll give you everything ACPI (or everything >> PCI), across all versions of the relevant standards". >> >> (b) Furthermore, regarding any such header file there that concerns >> itself with a specific version of the PCI or ACPI specs, the header with >> the higher version number includes the header with the lower version number. >> >> Specifics: >> - Acpi.h -> Acpi61.h -> Acpi60.h -> Acpi51.h -> Acpi50.h -> ... >> - Pci.h -> Pci30.h -> Pci23.h -> Pci22.h >> -> PciExpress21.h >> -> PciExpress30.h -> PciExpress21.h (strange...) >> >> As I said, this is not a good analogy. Because: Virtio.h never wanted to >> be a catch-all header. To closely imitate the Acpi.h and Pci.h situation >> above, I would have to rename Virtio.h to Virtio095.h first, then >> include it in Virtio10.h, then introduce a *new* file called Virtio.h, >> which should include Virtio10.h. >> >> At the moment, Virtio.h is not the catch-all header, it is actually the >> base (= lowest version number) header. > > Yes, it currently is a catch-all header. It just happens that there is > only 1 version of the spec in the tree at the moment. :) Good point. > >> So your suggestion would >> establish the inverse direction, compared to Acpi.h and Pci.h. >> >> Now, if you indeed proposed >> >> Virtio.h [new] -> Virtio10.h -> Virtio095.h [to be renamed from >> the current Virtio.h] >> >> then I would say, "a lot of churn for nothing". >> > > I think we should include 1.0 from Virtio.h, and determine if we want > to break out the Virtio095.h file at a later point. Works for me. > > Does it actually cause problems if we include Virtio10.h from > Virtio.h? No, it shouldn't; they are independent of each other. Note however that the same will not work for VirtioNet.h <-> Virtio10Net.h, because Virtio10Net.h already depends on (and includes) VirtioNet.h. Including Virtio10Net.h from VirtioNet.h would lead to a cycle. (The inclusion guards would prevent an actual inclusion loop, but the definition of VIRTIO_1_0_NET_REQ would not compile.) Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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; + UINT16 NumBuffers; +} VIRTIO_1_0_NET_REQ; +#pragma pack () + +#endif // _VIRTIO_1_0_NET_H_
These header files are intentionally minimal, and intentionally kept apart from the VirtIo 0.9.5 headers. Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Jordan Justen <jordan.l.justen@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- OvmfPkg/Include/IndustryStandard/Virtio10.h | 81 ++++++++++++++++++++ OvmfPkg/Include/IndustryStandard/Virtio10Net.h | 31 ++++++++ 2 files changed, 112 insertions(+) -- 1.8.3.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel