[SeaBIOS] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure
Alexander Bezzubikov
zuban32s at gmail.com
Tue Aug 1 19:28:45 CEST 2017
2017-08-01 16:38 GMT+03:00 Marcel Apfelbaum <marcel at redhat.com>:
> 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.
Let's dwell on the second option (with -1 as 'ignore' sign), if no new
objections.
>
> 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
>>
>>
>>
>>
>
--
Aleksandr Bezzubikov
More information about the SeaBIOS
mailing list