[edk2,wave,3,06/15] OvmfPkg: IndustryStandard: add definitions from the VirtIo 1.0 spec

Message ID 1457960012-29481-7-git-send-email-lersek@redhat.com
State New
Headers show

Commit Message

Laszlo Ersek March 14, 2016, 12:53 p.m.
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

Comments

Laszlo Ersek April 5, 2016, 8:31 a.m. | #1
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
>>
>>
Ard Biesheuvel April 5, 2016, 4:10 p.m. | #2
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
Laszlo Ersek April 5, 2016, 4:46 p.m. | #3
(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
Laszlo Ersek April 5, 2016, 6:06 p.m. | #4
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
Laszlo Ersek April 5, 2016, 6:18 p.m. | #5
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
Laszlo Ersek April 5, 2016, 6:26 p.m. | #6
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

Patch

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_