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

Marcel Apfelbaum marcel at redhat.com
Mon Aug 7 17:52:18 CEST 2017


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,

> --- /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.

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

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

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



More information about the SeaBIOS mailing list