Re: [SeaBIOS] [Qemu-devel] [PATCH v4 5/5] docs: update documentation considering PCIE-PCI bridge - SeaBIOS

9 Aug 2017

      On 08/09/17 18:52, Aleksandr Bezzubikov wrote:
...
2017-08-09 13:18 GMT+03:00 Laszlo Ersek lersek@redhat.com:
...
On 08/08/17 21:21, Aleksandr Bezzubikov wrote:
...
2017-08-08 18:11 GMT+03:00 Laszlo Ersek lersek@redhat.com:
...
one comment below
On 08/05/17 22:27, Aleksandr Bezzubikov wrote:
...
+Capability layout (defined in include/hw/pci/pci_bridge.h):

uint8_t id;     Standard PCI capability header field
uint8_t next;   Standard PCI capability header field
uint8_t len;    Standard PCI vendor-specific capability header field

uint8_t type;   Red Hat vendor-specific capability type
               List of currently existing types:

                   QEMU_RESERVE = 1

uint32_t bus_res;   Minimum number of buses to reserve

uint64_t io;        IO space to reserve
uint64_t mem        Non-prefetchable memory to reserve
uint64_t mem_pref;  Prefetchable memory to reserve

(I apologize if I missed any concrete points from the past messages
regarding this structure.)
How is the firmware supposed to know whether the prefetchable MMIO
reservation should be made in 32-bit or 64-bit address space? If we
reserve prefetchable MMIO outside of the 32-bit address space, then
hot-plugging a device without 64-bit MMIO support could fail.
My earlier request, to distinguish "prefetchable_32" from
"prefetchable_64" (mutually exclusively), was so that firmware would
know whether to restrict the MMIO reservation to 32-bit address
space.
IIUC now (in SeaBIOS at least) we just assign this PREF registers
unconditionally,
so the decision about the mode can be made basing on !=0
UPPER_PREF_LIMIT register.
My idea was the same - we can just check if the value doesn't fit into
16-bit (PREF_LIMIT reg size, 32-bit MMIO). Do we really need separate
fields for that?
The PciBusDxe driver in edk2 tracks 32-bit and 64-bit MMIO resources
separately from each other, and other (independent) logic exists in it
that, on some conditions, allocates 64-bit MMIO BARs from 32-bit address
space. This is just to say that the distinction is intentional in
PciBusDxe.
Furthermore, the Platform Init spec v1.6 says the following (this is
what OVMF will have to comply with, in the "platform hook" called by
PciBusDxe):
...
12.6 PCI Hot Plug PCI Initialization Protocol
EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding()
...
Padding  The amount of resource padding that is required by the PCI
         bus under the control of the specified HPC. Because the
         caller does not know the size of this buffer, this buffer is
         allocated by the callee and freed by the caller.
...
The padding is returned in the form of ACPI (2.0 & 3.0) resource
descriptors. The exact definition of each of the fields is the same as
in the
EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL.SubmitResources()
function. See the section 10.8 for the definition of this function.
Following that pointer:
...
10.8 PCI HostBridge Code Definitions
10.8.2 PCI Host Bridge Resource Allocation Protocol
Table 8. ACPI 2.0 & 3.0 QWORD Address Space Descriptor Usage
Byte    Byte    Data  Description
Offset  Length
...
0x03    0x01          Resource type:
                        0: Memory range
                        1: I/O range
                        2: Bus number range
...
0x05    0x01          Type-specific flags. Ignored except as defined
                      in Table 3-3 and Table 3-4 below.
0x06    0x08          Address Space Granularity. Used to differentiate
                      between a 32-bit memory request and a 64-bit
                      memory request. For a 32-bit memory request,
                      this field should be set to 32. For a 64-bit
                      memory request, this field should be set to 64.
                      Ignored for I/O and bus resource requests.
                      Ignored during GetProposedResources().
The "Table 3-3" and "Table 3-4" references under "Type-specific flags"
are out of date (spec bug); in reality those are:

Table 10. I/O Resource Flag (Resource Type = 1) Usage,
Table 11. Memory Resource Flag (Resource Type = 0) Usage.

The latter is relevant here:
...
Table 11. Memory Resource Flag (Resource Type = 0) Usage
Bits      Meaning
...
Bit[2:1]  _MEM. Memory attributes.
          Value and Meaning:
            0 The memory is nonprefetchable.
            1 Invalid.
            2 Invalid.
            3 The memory is prefetchable.
          Note: The interpretation of these bits is somewhat different
          from the ACPI Specification. According to the ACPI
          Specification, a value of 0 implies noncacheable memory and
          the value of 3 indicates prefetchable and cacheable memory.
So whatever OVMF sees in the capability, it must be able to translate to
the above representation.
OK, I got it.
Then I suggest this part of the cap look like
uint64_t mem_pref_32;
uint64_t mem_pref_64;
'mem_pref_32' field can be uint32_t, but this will require 4-byte padding,
so what looks more preferable here - uint64_t for 32-bit value or
4-byte padding in the middle
of the capapbility?
The last field before this part is "uint64_t io", and it is naturally
aligned. So, how about:
- uint32_t mem;         /* non-prefetchable, 32-bit only */
- uint32_t mem_pref_32; /* prefetchable, 32-bit,
                         * mutually exclusive with mem_pref_64
                         */
- uint64_t mem_pref_64; /* prefetchable, 64-bit,
                         * mutually exclusive with mem_pref_32
                         */
Again, the comments to the right come from the email that I got earlier
from Alex Williamson (which he wrote "according to Table 3-2 in the
PCI-to-PCI bridge spec rev 1.2").
IOW, "mem" need not be uint64_t, it can be uint32_t just as well, and
then we don't need padding for "mem_pref_32" either.
(I also think "uint64_t io" is overkill, but I care precious little
about IO reservation, beyond *disabling* it :) I intend to implement
"io" as well, of course.)
Thanks!
Laszlo