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

[Alexander Bezzubikov]

2017-08-07 18:52 GMT+03:00 Marcel Apfelbaum <address@hidden>:
> 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 <address@hidden>
>> ---
>>   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.

>
>> +*/
>> +
>> +/* 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

>> +
>> +/* 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?

>> +
>> +
>> +/* 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



-- 
Aleksandr Bezzubikov