On 08/09/17 18:52, Aleksandr Bezzubikov wrote: > 2017-08-09 13:18 GMT+03:00 Laszlo Ersek <ler...@redhat.com>: >> On 08/08/17 21:21, Aleksandr Bezzubikov wrote: >>> 2017-08-08 18:11 GMT+03:00 Laszlo Ersek <ler...@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