On 11/08/2017 2:31, Aleksandr Bezzubikov wrote:
On PCI init PCI bridges may need some extra info about bus number, IO, memory and prefetchable memory to reserve. QEMU can provide this with a special vendor-specific PCI capability.
Hi Aleksandr,
I only have a few very small comments, other than that it looks OK to me.
Signed-off-by: Aleksandr Bezzubikov zuban32s@gmail.com Reviewed-by: Marcel Apfelbaum marcel@redhat.com
hw/pci/pci_bridge.c | 54 +++++++++++++++++++++++++++++++++++++++++++++ include/hw/pci/pci_bridge.h | 24 ++++++++++++++++++++ 2 files changed, 78 insertions(+)
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c index 720119b..2495a51 100644 --- a/hw/pci/pci_bridge.c +++ b/hw/pci/pci_bridge.c @@ -408,6 +408,60 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name, br->bus_name = bus_name; }
+int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset, > + uint32_t bus_reserve, uint64_t io_reserve,
Please pay attention to indentation, the above line should be aligned with the above ("
uint32_t mem_non_pref_reserve,uint32_t mem_pref_32_reserve,uint64_t mem_pref_64_reserve,Error **errp)+{
- if (mem_pref_32_reserve != (uint32_t)-1 &&
mem_pref_64_reserve != (uint64_t) -1) {
Same here
error_setg(errp,"PCI resource reserve cap: PREF32 and PREF64 conflict");return -EINVAL;- }
- if (bus_reserve == (uint32_t)-1 &&
io_reserve == (uint64_t)-1 &&mem_non_pref_reserve == (uint32_t)-1 &&mem_pref_32_reserve == (uint32_t)-1 &&mem_pref_64_reserve == (uint64_t)-1) {
and here (please go over all the file)
return 0;- }
- size_t cap_len = sizeof(PCIBridgeQemuCap);
- PCIBridgeQemuCap cap = {
.len = cap_len,.type = REDHAT_PCI_CAP_RESOURCE_RESERVE,.bus_res = bus_reserve,.io = io_reserve,.mem = mem_non_pref_reserve,.mem_pref_32 = (uint32_t)-1,.mem_pref_64 = (uint64_t)-1
Why not use the values of mem_pref_32_reserve and mem_pref_64_reserve ? You already have checked they are mutually exclusive.
- };
- if (mem_pref_32_reserve != (uint32_t)-1 &&
mem_pref_64_reserve == (uint64_t)-1) {cap.mem_pref_32 = mem_pref_32_reserve;- } else if (mem_pref_32_reserve == (uint32_t)-1 &&
mem_pref_64_reserve != (uint64_t)-1) {cap.mem_pref_64 = mem_pref_64_reserve;- }
So it seems you don't need the above code at all, right?
With the above minor comments, please keep my R-b tag. Thanks, Marcel
- int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
cap_offset, cap_len, errp);- if (offset < 0) {
return offset;- }
- memcpy(dev->config + offset + PCI_CAP_FLAGS,
(char *)&cap + PCI_CAP_FLAGS,cap_len - PCI_CAP_FLAGS);- return 0;
+}
- static const TypeInfo pci_bridge_type_info = { .name = TYPE_PCI_BRIDGE, .parent = TYPE_PCI_DEVICE,
diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h index ff7cbaa..2d8c635 100644 --- a/include/hw/pci/pci_bridge.h +++ b/include/hw/pci/pci_bridge.h @@ -67,4 +67,28 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name, #define PCI_BRIDGE_CTL_DISCARD_STATUS 0x400 /* Discard timer status */ #define PCI_BRIDGE_CTL_DISCARD_SERR 0x800 /* Discard timer SERR# enable */
+typedef struct PCIBridgeQemuCap {
- uint8_t id; /* Standard PCI capability header field */
- uint8_t next; /* Standard PCI capability header field */
- uint8_t len; /* Standard PCI vendor-specific capability header field */
- uint8_t type; /* Red Hat vendor-specific capability type.
Types are defined with REDHAT_PCI_CAP_ prefix */- uint32_t bus_res; /* Minimum number of buses to reserve */
- uint64_t io; /* IO space to reserve */
- uint32_t mem; /* Non-prefetchable memory to reserve */
- /* This two fields are mutually exclusive */
- uint32_t mem_pref_32; /* Prefetchable memory to reserve (32-bit MMIO) */
- uint64_t mem_pref_64; /* Prefetchable memory to reserve (64-bit MMIO) */
+} PCIBridgeQemuCap;
+#define REDHAT_PCI_CAP_RESOURCE_RESERVE 1
+int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset,
uint32_t bus_reserve, uint64_t io_reserve,uint32_t mem_non_pref_reserve,uint32_t mem_pref_32_reserve,uint64_t mem_pref_64_reserve,Error **errp);- #endif /* QEMU_PCI_BRIDGE_H */