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

[Marcel Apfelbaum]

On 07/08/2017 19:02, Alexander Bezzubikov wrote:
> 2017-08-07 18:52 GMT+03:00 Marcel Apfelbaum <marcel at redhat.com>:
>> On 05/08/2017 23:29, 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 | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 50 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..2c8ddb0
>>
>>
>> Hi Aleksandr,
>>
> 
> Hi Marcel,
> 
>>> --- /dev/null
>>> +++ b/src/fw/dev-pci.h
>>> @@ -0,0 +1,50 @@
>>> +#ifndef _PCI_CAP_H
>>> +#define _PCI_CAP_H
>>> +
>>> +#include "types.h"
>>> +
>>> +/*
>>> +
>>
>>
>> Please use the standard comment:
>>
>> /*
>>   *
>>   *
>>   */
>>
>>
>>> +QEMU-specific vendor(Red Hat)-specific capability.
>>> +It's intended to provide some hints for firmware to init PCI devices.
>>> +
>>> +Its structure 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_CAP_TYP_QEMU=1 exists
>>
>>
>> Typo o the line before, but I think you don't need it there.
>>
>>> +Data:
>>> +
>>> +u32 bus_res;            minimum bus number to reserve;
>>> +                        this is necessary for PCI Express Root Ports
>>> +                        to support PCIE-to-PCI bridge hotplug
>>
>>
>> I would add a broader class of usage:
>>       necessary for nesting PCI bridges hotplug.
>>
>>> +u64 io;                 IO space to reserve
>>> +u64 mem;                non-prefetchable memory space to reserve
>>> +u64 prefetchable_mem;   prefetchable memory space to reserve
>>> +
>>
>>
>> Layout looks good to me.
>>
>>> +If any field value in Data section is -1,
>>> +it means that such kind of reservation
>>> +is not needed and must be ignored.
>>> +
>>
>>
>> -1 is not a valid value for unsigned fields, you may
>> want to say 0xff..f or some other way.
> 
> I meant cast to unsigned here (because we still use unsigned types),
> but if it can mislead someone I will change this.
> 
We should not document signed values for unsigned fields,
even if the reason is "best practices."

>>
>>> +*/
>>> +
>>> +/* Offset of vendor-specific capability type field */
>>> +#define PCI_CAP_REDHAT_TYPE  3
>>
>>
>> May I ask why why '3'? I am not against it, I just
>> want to understand the number.
>>
> 
> This is actually an offset to this field
> 

OK, so it should end with 'offset' to be clear.
I was mislead.

>>> +
>>> +/* List of valid Red Hat vendor-specific capability types */
>>> +#define REDHAT_CAP_TYPE_QEMU    1
>>
>>
>> I think I pointed this in another thread, the name is
>> too vague, please change it to something like:
>>     REDHAT_CAP_RES_RESERVE_QEMU
>> that narrows down the intend.
>>
> 
> What does the first 'RES' mean?

Resource.

I don't mind you change it how you seem fit,
just make it clear what it does. Is about
resource reserving, not a general capability.


Thanks,
Marcel

> 
>>> +
>>> +
>>> +/* Offsets of QEMU capability fields */
>>> +#define QEMU_PCI_CAP_BUS_RES        4
>>> +#define QEMU_PCI_CAP_LIMITS_OFFSET  8
>>> +#define QEMU_PCI_CAP_IO             8
>>> +#define QEMU_PCI_CAP_MEM            16
>>> +#define QEMU_PCI_CAP_PREF_MEM       24
>>> +#define QEMU_PCI_CAP_SIZE           32
>>> +
>>> +#endif /* _PCI_CAP_H */
>>>
>>
>> The layout looks good to me.
>>
>> Thanks,
>> Marcel
> 
> 
>