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

[Aleksandr Bezzubikov]

2017-08-09 13:18 GMT+03:00 Laszlo Ersek <address@hidden>:
> On 08/08/17 21:21, Aleksandr Bezzubikov wrote:
>> 2017-08-08 18:11 GMT+03:00 Laszlo Ersek <address@hidden>:
>>> 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?

>
> Thanks
> Laszlo
>
>>
>>>
>>> This is based on an earlier email from Alex to me:
>>>
>>> On 10/03/16 18:01, Alex Williamson wrote:
>>>> I don't think there's such a thing as a 64-bit non-prefetchable
>>>> aperture.  In fact, there are not separate 32 and 64 bit
>>>> prefetchable apertures.  The apertures are:
>>>>
>>>> I/O base/limit - (default 16bit, may be 32bit)
>>>> Memory base/limit - (32bit only, non-prefetchable)
>>>> Prefetchable Memory base/limit - (default 32bit, may be 64bit)
>>>>
>>>> This is according to Table 3-2 in the PCI-to-PCI bridge spec rev
>>>> 1.2.
>>>
>>> I don't care much about the 16-bit vs. 32-bit IO difference (that's
>>> entirely academic and the Platform Spec init doesn't even provide a
>>> way for OVMF to express such a difference). However, the optional
>>> restriction to 32-bit matters for the prefetchable MMIO aperture.
>>>
>>> Other than this, the patch looks good to me, and I'm ready to R-b.
>>>
>>> Thanks!
>>> Laszlo



-- 
Aleksandr Bezzubikov