[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V6 for-2.3 13/26] hw/pci-host: introduce TYPE_PC
|
From: |
Marcel Apfelbaum |
|
Subject: |
Re: [Qemu-devel] [PATCH V6 for-2.3 13/26] hw/pci-host: introduce TYPE_PCI_HOST_BRIDGE_SNOOPED interface |
|
Date: |
Tue, 28 Apr 2015 13:21:14 +0300 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 |
On 04/28/2015 11:43 AM, Michael S. Tsirkin wrote:
On Tue, Apr 28, 2015 at 11:39:42AM +0300, Marcel Apfelbaum wrote:
On 04/27/2015 05:45 PM, Michael S. Tsirkin wrote:
On Mon, Apr 27, 2015 at 04:01:16PM +0300, Marcel Apfelbaum wrote:
On 04/27/2015 03:14 PM, Michael S. Tsirkin wrote:
On Thu, Mar 19, 2015 at 08:52:48PM +0200, Marcel Apfelbaum wrote:
TYPE_PCI_HOST_BRIDGE_SNOOPED is a special case of host bridge
whose configuration registers are snooped by other host bridges
to complete their configuration cycles.
The interface exposes a list of snooping host bridges that
shall be used by the hosts implementing this interface
in order to emulate a snooping mechanism.
The way that the snooping hosts are registered or how
the snooping is implemented are out of the interface scope,
it only provides a way to determine if a host bridge has
snooping hosts and list them.
Signed-off-by: Marcel Apfelbaum <address@hidden>
---
hw/pci/pci_host.c | 8 ++++++++
include/hw/pci/pci_host.h | 24 ++++++++++++++++++++++++
2 files changed, 32 insertions(+)
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 87180c8..288e74c 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -180,6 +180,12 @@ static const TypeInfo pci_main_host_interface_info = {
.parent = TYPE_INTERFACE,
};
+static const TypeInfo pci_host_bridge_snooped_interface_info = {
+ .name = TYPE_PCI_HOST_BRIDGE_SNOOPED,
+ .parent = TYPE_INTERFACE,
+ .class_size = sizeof(PCIHostBridgeSnoopedClass),
+};
+
static const TypeInfo pci_host_type_info = {
.name = TYPE_PCI_HOST_BRIDGE,
.parent = TYPE_SYS_BUS_DEVICE,
@@ -191,7 +197,9 @@ static const TypeInfo pci_host_type_info = {
static void pci_host_register_types(void)
{
type_register_static(&pci_main_host_interface_info);
+ type_register_static(&pci_host_bridge_snooped_interface_info);
type_register_static(&pci_host_type_info);
+
extra empty line
Will take care of it, thanks.
}
type_init(pci_host_register_types)
diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index 3c72e26..a041919 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -63,6 +63,30 @@ typedef struct PCIHostBridgeClass {
const char *(*root_bus_path)(PCIHostState *, PCIBus *);
} PCIHostBridgeClass;
+/**
+ * A special case of host bridge whose configuration registers
+ * are snooped by other host bridges to complete their
+ * configuration cycles.
+ */
+#define TYPE_PCI_HOST_BRIDGE_SNOOPED "pci-host-bridge-snooped"
+#define TYPE_PCI_HOST_BRIDGE_SNOOPED_CLASS(klass) \
+ OBJECT_CLASS_CHECK(PCIHostBridgeSnoopedClass, (klass), \
+ TYPE_PCI_HOST_BRIDGE_SNOOPED)
+#define PCI_HOST_BRIDGE_SNOOPED_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(PCIHostBridgeSnoopedClass, (obj), \
+ TYPE_PCI_HOST_BRIDGE_SNOOPED)
+#define PCI_HOST_BRIDGE_SNOOPED(obj) \
+ INTERFACE_CHECK(PCIHostState, (obj), \
+ TYPE_PCI_HOST_BRIDGE_SNOOPED)
+
+typedef struct PCIHostBridgeSnoopedClass {
+ /* <private> */
+ InterfaceClass parent_class;
+
+ /* <public> */
+ GPtrArray *(*snooping_hosts)(PCIHostState *);
Why not just add GPtrArray here directly, and add an API to
register/deregister?
The interface concentrates on usage, letting the way to fill the snooping
hosts to the specific implementation.
Thanks,
Marcel
IMO this is overdoing abstractions.
It will provide us with enough elasticity when we'll try the same for Q35.
Also this interface is used in several places and it will serve us when we'll
go for Q35.
I don't see how it helps, we don't just stick a virtual function
in front of all new code we write on the assumption it
helps elasticity.
It helps by letting us querying the interface instead of asking:
Is this host-bridge PIIX ? look for child buses and see if there are roots
else: is this host-bridge ...
But all this might be moot, it's probably better to just
add everyone to the child bus list, and drop the
concept of snooping config cycles.
Well, this will be a problem, since the whole concept of the PXB is to snoop
on the configuration cycles and this interface gives us a more easy
way to understand the implementation.
We already discussed this approach and was accepted.
The code is small and concise and the use of QOM makes sense in my opinion.
I prefer not to re-open it now.
Thanks,
Marcel
I'm talking about the implementation here.
We already scan the child bus list on config
transactions. Why not add your entry there,
then you can snoop to your heart's content.
Maybe I didn't fully understand what you want.
Do you want to add the new root buses as child buses of root bus 0?
This interface is not used in this context, is used in other scenarios:
[PATCH V6 for-2.3 21/26] hw/pci: inform bios if the system has extra pci root
buses
[PATCH V6 for-2.3 15/26] hw/acpi: add support for i440fx 'snooping' root busses
I can look for other usages, can you tell me how it will look without this
interface?
Other thing, in config we have pci_data_write calling (in the end)
pci_find_bus_nr that does not support
multiple root buses.
Changing it could affect a lot of functionality and also looking at the
function it will not be pretty or clear.
Keeping it at host-bridge level (see [PATCH V6 for-2.3 14/26]
hw/pci-host/piix: implement TYPE_PCI_HOST_BRIDGE_SNOOPED interface)
and having piix query its pxbs for the extra root buses seems a good fit.
Thanks,
Marce
+} PCIHostBridgeSnoopedClass;
+
/* common internal helpers for PCI/PCIe hosts, cut off overflows */
void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
uint32_t limit, uint32_t val, uint32_t len);
--
2.1.0