(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 <[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? 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 [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
