[SeaBIOS] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure

[Marcel Apfelbaum]

On 31/07/2017 22:01, Alexander Bezzubikov wrote:
> 2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin <mst at redhat.com>:
>> On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote:
>>> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum <marcel at redhat.com>:
>>>> On 31/07/2017 17:00, Michael S. Tsirkin wrote:
>>>>>
>>>>> On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote:
>>>>>>
>>>>>> On PCI init PCI bridge devices may need some
>>>>>> extra info about bus number to reserve, IO, memory and
>>>>>> prefetchable memory limits. QEMU can provide this
>>>>>> with special vendor-specific PCI capability.
>>>>>>
>>>>>> This capability is intended to be used only
>>>>>> for Red Hat PCI bridges, i.e. QEMU cooperation.
>>>>>>
>>>>>> Signed-off-by: Aleksandr Bezzubikov <zuban32s at gmail.com>
>>>>>> ---
>>>>>>    src/fw/dev-pci.h | 62
>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>    1 file changed, 62 insertions(+)
>>>>>>    create mode 100644 src/fw/dev-pci.h
>>>>>>
>>>>>> diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h
>>>>>> new file mode 100644
>>>>>> index 0000000..fbd49ed
>>>>>> --- /dev/null
>>>>>> +++ b/src/fw/dev-pci.h
>>>>>> @@ -0,0 +1,62 @@
>>>>>> +#ifndef _PCI_CAP_H
>>>>>> +#define _PCI_CAP_H
>>>>>> +
>>>>>> +#include "types.h"
>>>>>> +
>>>>>> +/*
>>>>>> +
>>>>>> +QEMU-specific vendor(Red Hat)-specific capability.
>>>>>> +It's intended to provide some hints for firmware to init PCI devices.
>>>>>> +
>>>>>> +Its is shown below:
>>>>>> +
>>>>>> +Header:
>>>>>> +
>>>>>> +u8 id;       Standard PCI Capability Header field
>>>>>> +u8 next;     Standard PCI Capability Header field
>>>>>> +u8 len;      Standard PCI Capability Header field
>>>>>> +u8 type;     Red Hat vendor-specific capability type:
>>>>>> +               now only REDHAT_QEMU_CAP 1 exists
>>>>>> +Data:
>>>>>> +
>>>>>> +u16 non_prefetchable_16;     non-prefetchable memory limit
>>>>>> +
>>>>>> +u8 bus_res;  minimum bus number to reserve;
>>>>>> +             this is necessary for PCI Express Root Ports
>>>>>> +             to support PCIE-to-PCI bridge hotplug
>>>>>> +
>>>>>> +u8 io_8;     IO limit in case of 8-bit limit value
>>>>>> +u32 io_32;   IO limit in case of 16-bit limit value
>>>>>> +             io_8 and io_16 are mutually exclusive, in other words,
>>>>>> +             they can't be non-zero simultaneously
>>>>>> +
>>>>>> +u32 prefetchable_32;         non-prefetchable memory limit
>>>>>> +                             in case of 32-bit limit value
>>>>>> +u64 prefetchable_64;         non-prefetchable memory limit
>>>>>> +                             in case of 64-bit limit value
>>>>>> +                             prefetachable_32 and prefetchable_64 are
>>>>>> +                             mutually exclusive, in other words,
>>>>>> +                             they can't be non-zero simultaneously
>>>>>> +If any field in Data section is 0,
>>>>>> +it means that such kind of reservation
>>>>>> +is not needed.
>>>
>>> I really don't like this 'mutually exclusive' fields approach because
>>> IMHO it increases confusion level when undertanding this capability structure.
>>> But - if we came to consensus on that, then IO fields should be used
>>> in the same way,
>>> because as I understand, this 'mutual exclusivity' serves to distinguish cases
>>> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT
>>> registers.
>>> And this is how both IO and PREFETCHABLE works, isn't it?
>>
>> I would just use simeple 64 bit registers. PCI spec has an ugly format
>> with fields spread all over the place but that is because of
>> compatibility concerns. It makes not sense to spend cycles just
>> to be similarly messy.
> 
> Then I suggest to use exactly one field of a maximum possible size
> for each reserving object, and get rid of mutually exclusive fields.
> Then it can be something like that (order and names can be changed):
> u8 bus;
> u16 non_pref;
> u32 io;
> u64 pref;
> 

I think Michael suggested:

     u64 bus_res;
     u64 mem_res;
     u64 io_res;
     u64 mem_pref_res;

OR:
     u32 bus_res;
     u32 mem_res;
     u32 io_res;
     u64 mem_pref_res;


We can use 0XFFF..F as "not-set" value "merging" Gerd's and Michael's
requests.

Thanks,
Marcel

> 
>>
>>
>>>>>
>>>>>
>>>>
>>>> Hi Michael,
>>>>
>>>>> We also want a way to say "no hint for this type".
>>>>>
>>>>> One way to achive this would be to have instead multiple
>>>>> vendor specific capabilities, one for each of
>>>>> bus#/io/mem/prefetch. 0 would mean do not reserve anything,
>>>>> absence of capability would mean "no info, up to firmware".
>>>>>
>>>>
>>>> First version of the series was implemented exactly like you propose,
>>>> however Gerd preferred only one capability with multiple fields.
>>>>
>>>> I personally like the simplicity of vendor cap per io/mem/bus,
>>>> even if it is on the expense of the limited PCI Config space.
>>>>
>>>
>>> Personally I agree with Marcel since all this fields express
>>> reservations of some objects.
>>>
>>>> We need a consensus here :)
>>>>
>>>
>>> Absolutely :)
>>>
>>>> Thanks,
>>>> Marcel
>>>>
>>>>
>>>>>
>>>>>> +
>>>>>> +*/
>>>>>> +
>>>>>> +/* Offset of vendor-specific capability type field */
>>>>>> +#define PCI_CAP_VNDR_SPEC_TYPE  3
>>>>>
>>>>>
>>>>> This is a QEMU specific thing. Please name it as such.
>>>>>
>>>>>> +
>>>>>> +/* List of valid Red Hat vendor-specific capability types */
>>>>>> +#define REDHAT_CAP_TYPE_QEMU    1
>>>>>> +
>>>>>> +
>>>>>> +/* Offsets of QEMU capability fields */
>>>>>> +#define QEMU_PCI_CAP_NON_PREF   4
>>>>>> +#define QEMU_PCI_CAP_BUS_RES    6
>>>>>> +#define QEMU_PCI_CAP_IO_8       7
>>>>>> +#define QEMU_PCI_CAP_IO_32      8
>>>>>> +#define QEMU_PCI_CAP_PREF_32    12
>>>>>> +#define QEMU_PCI_CAP_PREF_64    16
>>>>>> +#define QEMU_PCI_CAP_SIZE       24
>>>>>> +
>>>>>> +#endif /* _PCI_CAP_H */
>>>>>> --
>>>>>> 2.7.4
>>>>
>>>>
>>>
>>>
>>>
>>> --
>>> Alexander Bezzubikov
> 
> 
>