diff mbox

[RFC,v2,3/4] pci: add QEMU-specific PCI capability structure

Message ID 1500761510-1556-4-git-send-email-zuban32s@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Aleksandr Bezzubikov July 22, 2017, 10:11 p.m. UTC 
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.

Sizes of limits match ones from 
PCI Type 1 Configuration Space Header,
number of buses to reserve occupies only 1 byte 
since it is the size of Subordinate Bus Number register.

Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
---
 src/hw/pci_cap.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 src/hw/pci_cap.h

Comments

Michael S. Tsirkin July 23, 2017, 2:44 a.m. UTC | #1 
On Sun, Jul 23, 2017 at 01:11:49AM +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.
> 
> Sizes of limits match ones from 
> PCI Type 1 Configuration Space Header,
> number of buses to reserve occupies only 1 byte 
> since it is the size of Subordinate Bus Number register.
> 
> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> ---
>  src/hw/pci_cap.h | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>  create mode 100644 src/hw/pci_cap.h
> 
> diff --git a/src/hw/pci_cap.h b/src/hw/pci_cap.h
> new file mode 100644
> index 0000000..1382b0b
> --- /dev/null
> +++ b/src/hw/pci_cap.h
> @@ -0,0 +1,23 @@
> +#ifndef _PCI_CAP_H
> +#define _PCI_CAP_H
> +
> +#include "types.h"
> +
> +struct vendor_pci_cap {
> +    u8 id;
> +    u8 next;
> +    u8 len;
> +};
> +
> +struct redhat_pci_bridge_cap {
> +    struct vendor_pci_cap hdr;

You want to add some kind of identifier here after
the header, such that more capabilities can be added
in future without breaking this one.

> +    u8 bus_res;
> +    u32 pref_lim_upper;
> +    u16 pref_lim;
> +    u16 mem_lim;
> +    u16 io_lim_upper;
> +    u8 io_lim;
> +    u8 padd;

Please add documentation.


> +};
> +
> +#endif /* _PCI_CAP_H */
> -- 
> 2.7.4
Aleksandr Bezzubikov July 23, 2017, 4:12 p.m. UTC | #2 
2017-07-23 5:44 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:

> On Sun, Jul 23, 2017 at 01:11:49AM +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.
> >
> > Sizes of limits match ones from
> > PCI Type 1 Configuration Space Header,
> > number of buses to reserve occupies only 1 byte
> > since it is the size of Subordinate Bus Number register.
> >
> > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> > ---
> >  src/hw/pci_cap.h | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >  create mode 100644 src/hw/pci_cap.h
> >
> > diff --git a/src/hw/pci_cap.h b/src/hw/pci_cap.h
> > new file mode 100644
> > index 0000000..1382b0b
> > --- /dev/null
> > +++ b/src/hw/pci_cap.h
> > @@ -0,0 +1,23 @@
> > +#ifndef _PCI_CAP_H
> > +#define _PCI_CAP_H
> > +
> > +#include "types.h"
> > +
> > +struct vendor_pci_cap {
> > +    u8 id;
> > +    u8 next;
> > +    u8 len;
> > +};
> > +
> > +struct redhat_pci_bridge_cap {
> > +    struct vendor_pci_cap hdr;
>
> Hi Michael,
Thanks for the quick reply.


> You want to add some kind of identifier here after
> the header, such that more capabilities can be added
> in future without breaking this one.
>

You mean to distinguish different vendor-specific capabilities?
Agreed if so, will add it in the next version.


>
> > +    u8 bus_res;
> > +    u32 pref_lim_upper;
> > +    u16 pref_lim;
> > +    u16 mem_lim;
> > +    u16 io_lim_upper;
> > +    u8 io_lim;
> > +    u8 padd;
>
> Please add documentation.
>
>
> > +};
> > +
> > +#endif /* _PCI_CAP_H */
> > --
> > 2.7.4
>
Kevin O'Connor July 23, 2017, 4:30 p.m. UTC | #3 
On Sun, Jul 23, 2017 at 01:11:49AM +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.
> 
> Sizes of limits match ones from 
> PCI Type 1 Configuration Space Header,
> number of buses to reserve occupies only 1 byte 
> since it is the size of Subordinate Bus Number register.
> 
> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> ---
>  src/hw/pci_cap.h | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>  create mode 100644 src/hw/pci_cap.h
> 
> diff --git a/src/hw/pci_cap.h b/src/hw/pci_cap.h
> new file mode 100644
> index 0000000..1382b0b
> --- /dev/null
> +++ b/src/hw/pci_cap.h
> @@ -0,0 +1,23 @@
> +#ifndef _PCI_CAP_H
> +#define _PCI_CAP_H
> +
> +#include "types.h"
> +
> +struct vendor_pci_cap {
> +    u8 id;
> +    u8 next;
> +    u8 len;
> +};

Thanks.  If you respin this series, please add this header to the
src/fw/ directory instead of src/hw/.  Also, I'd prefer to avoid a
"pci_" prefix on the header as it makes it seem similar to the
existing pci_regs.h and pci_ids.h headers which are a bit different -
how about src/fw/dev-pci.h instead?

-Kevin
Aleksandr Bezzubikov July 23, 2017, 4:47 p.m. UTC | #4 
2017-07-23 19:30 GMT+03:00 Kevin O'Connor <kevin@koconnor.net>:

> On Sun, Jul 23, 2017 at 01:11:49AM +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.
> >
> > Sizes of limits match ones from
> > PCI Type 1 Configuration Space Header,
> > number of buses to reserve occupies only 1 byte
> > since it is the size of Subordinate Bus Number register.
> >
> > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> > ---
> >  src/hw/pci_cap.h | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >  create mode 100644 src/hw/pci_cap.h
> >
> > diff --git a/src/hw/pci_cap.h b/src/hw/pci_cap.h
> > new file mode 100644
> > index 0000000..1382b0b
> > --- /dev/null
> > +++ b/src/hw/pci_cap.h
> > @@ -0,0 +1,23 @@
> > +#ifndef _PCI_CAP_H
> > +#define _PCI_CAP_H
> > +
> > +#include "types.h"
> > +
> > +struct vendor_pci_cap {
> > +    u8 id;
> > +    u8 next;
> > +    u8 len;
> > +};
>
> Thanks.

Thanks for the review.


> If you respin this series, please add this header to the
> src/fw/ directory instead of src/hw/.  Also, I'd prefer to avoid a
> "pci_" prefix on the header as it makes it seem similar to the
> existing pci_regs.h and pci_ids.h headers which are a bit different -
> how about src/fw/dev-pci.h instead?
>
> -Kevin
>

No objections, Will do that in v3.
Except 'pci_' prefix - it's still a PCI capability, isn't it?
diff mbox

Patch

diff --git a/src/hw/pci_cap.h b/src/hw/pci_cap.h
new file mode 100644
index 0000000..1382b0b
--- /dev/null
+++ b/src/hw/pci_cap.h
@@ -0,0 +1,23 @@ 
+#ifndef _PCI_CAP_H
+#define _PCI_CAP_H
+
+#include "types.h"
+
+struct vendor_pci_cap {
+    u8 id;
+    u8 next;
+    u8 len;
+};
+
+struct redhat_pci_bridge_cap {
+    struct vendor_pci_cap hdr;
+    u8 bus_res;
+    u32 pref_lim_upper;
+    u16 pref_lim;
+    u16 mem_lim;
+    u16 io_lim_upper;
+    u8 io_lim;
+    u8 padd;
+};
+
+#endif /* _PCI_CAP_H */