From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37153) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XH8KL-0005fd-Jl for qemu-devel@nongnu.org; Tue, 12 Aug 2014 05:25:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XH8KG-0005Y8-Nc for qemu-devel@nongnu.org; Tue, 12 Aug 2014 05:25:09 -0400 Received: from mga11.intel.com ([192.55.52.93]:17007) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XH8KG-0005VN-Db for qemu-devel@nongnu.org; Tue, 12 Aug 2014 05:25:04 -0400 Message-ID: <53E9DD6C.5070304@intel.com> Date: Tue, 12 Aug 2014 17:25:00 +0800 From: "Chen, Tiejun" MIME-Version: 1.0 References: <1407307835-9692-1-git-send-email-tiejun.chen@intel.com> <1407307835-9692-4-git-send-email-tiejun.chen@intel.com> <20140806094509.GB22307@redhat.com> <53E2009E.4080107@intel.com> <20140806210714.GD22307@redhat.com> <53E2D921.8040501@intel.com> <20140810202759.GA5199@redhat.com> <53E82F88.5020209@intel.com> <20140812085435.GA6440@redhat.com> In-Reply-To: <20140812085435.GA6440@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v4][PATCH 3/5] I440FX_PCI_DEVICE: add pci_type to index List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: pbonzini@redhat.com, xen-devel@lists.xensource.com, qemu-devel@nongnu.org, stefano.stabellini@eu.citrix.com On 2014/8/12 16:54, Michael S. Tsirkin wrote: > On Mon, Aug 11, 2014 at 10:50:48AM +0800, Chen, Tiejun wrote: >> On 2014/8/11 4:27, Michael S. Tsirkin wrote: >>> On Thu, Aug 07, 2014 at 09:40:49AM +0800, Chen, Tiejun wrote: >>>> >>>> >>>> On 2014/8/7 5:07, Michael S. Tsirkin wrote: >>>>> On Wed, Aug 06, 2014 at 06:17:02PM +0800, Chen, Tiejun wrote: >>>>>> On 2014/8/6 17:45, Michael S. Tsirkin wrote: >>>>>>> On Wed, Aug 06, 2014 at 02:50:33PM +0800, Tiejun Chen wrote: >>>>>>>> We need to use this index to reuse this macro later >>>>>>>> >>>>>>>> Signed-off-by: Tiejun Chen >>>>>>> >>>>>>> Which index? >>>>>>> Most users don't need to change. >>>>>>> Just open-code OBJECT_CHECK where necessary, or add >>>>>>> a new wrapper. >>>>>> >>>>>> Okay so what about this? >>>>>> >>>>>> hw:pci-host:piix: define I440FX_PCI_DEVICE_FROM_TYPE >>>>>> >>>>>> We need to introduce I440FX_PCI_DEVICE_FROM_TYPE to get >>>>>> object with type, then we can reuse i440fx_init() simply. >>>>>> >>>>>> Signed-off-by: Tiejun Chen >>>>>> >>>>>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c >>>>>> index 0cd82b8..8c74653 100644 >>>>>> --- a/hw/pci-host/piix.c >>>>>> +++ b/hw/pci-host/piix.c >>>>>> @@ -93,6 +93,9 @@ typedef struct PIIX3State { >>>>>> #define I440FX_PCI_DEVICE(obj) \ >>>>>> OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE) >>>>>> >>>>>> +#define I440FX_PCI_DEVICE_FROM_TYPE(obj, type) \ >>>>>> + OBJECT_CHECK(PCII440FXState, (obj), type) >>>>> >>>>> This is just wrong. If you are casting to PCII440FXState, >>>> >>>> Why? We will have two different QOM typenames of PCII440FXStates. >>>> >>>>> there is no reason not to use TYPE_I440FX_PCI_DEVICE. >>>> >>>> As you know we already have this original, >>>> >>>> static const TypeInfo i440fx_info = { >>>> .name = TYPE_I440FX_PCI_DEVICE, >>>> .parent = TYPE_PCI_DEVICE, >>>> .instance_size = sizeof(PCII440FXState), >>>> .class_init = i440fx_class_init, >>>> }; >>>> >>>> and in patch #4, we will register that new host bridge to IGD passthrough: >>>> >>>> static const TypeInfo xen_igd_passthrough_i440fx_info = { >>>> .name = TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE, >>>> .parent = TYPE_PCI_DEVICE, >>>> .instance_size = sizeof(PCII440FXState), >>>> .class_init = xen_igd_passthrough_i440fx_class_init, >>>> }; >>> >>> My idea is to inherit TYPE_I440FX_PCI_DEVICE instead. >> >> So here, this mean xen_igd_passthrough_i440fx_info's parent should be >> TYPE_I440FX_PCI_DEVICE, right? >> >>> Then you can reuse regular piix code which casts to >>> TYPE_I440FX_PCI_DEVICE because >>> TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE would >>> be a subclass of TYPE_I440FX_PCI_DEVICE. >>> >> >> So since then, we can reuse i440fx_initfn. >> >> As a summary, what we should do is like the following, >> >> xen:hw:pci-host:piix: create host bridge to passthrough >> >> Implement a pci host bridge specific to passthrough. Actually >> this just inherits the standard one. >> >> This is based on http://patchwork.ozlabs.org/patch/363810/. >> >> Signed-off-by: Tiejun Chen >> --- >> hw/pci-host/piix.c | 28 ++++++++++++++++++++++++++++ >> include/hw/i386/pc.h | 2 ++ >> 2 files changed, 30 insertions(+) >> >> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c >> index 0cd82b8..2ccd9ee 100644 >> --- a/hw/pci-host/piix.c >> +++ b/hw/pci-host/piix.c >> @@ -703,6 +703,33 @@ static const TypeInfo i440fx_info = { >> .class_init = i440fx_class_init, >> }; >> >> +static void xen_igd_passthrough_i440fx_class_init(ObjectClass *klass, void >> *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >> + >> + k->init = i440fx_initfn; >> + k->vendor_id = PCI_VENDOR_ID_INTEL; >> + k->device_id = PCI_DEVICE_ID_INTEL_82441; >> + k->revision = 0x02; >> + k->class_id = PCI_CLASS_BRIDGE_HOST; >> + dc->desc = "IGD PT XEN Host bridge"; >> + dc->vmsd = &vmstate_i440fx; >> + /* >> + * PCI-facing part of the host bridge, not usable without the >> + * host-facing part, which can't be device_add'ed, yet. >> + */ >> + dc->cannot_instantiate_with_device_add_yet = true; >> + dc->hotpluggable = false; >> +} >> + > > A bunch of code still duplicated here. Only override what you have to. You mean all stuffs can be inherited from its own parent, even including vendor_id/device_id? If yes, currently the follows should be enough, static void xen_igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); dc->desc = "IGD PT XEN Host bridge"; } We will add k->config_write/k->config_read here when we introduce IGD passthrough later. Thanks Tiejun > > >> +static const TypeInfo xen_igd_passthrough_i440fx_info = { >> + .name = TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE, >> + .parent = TYPE_I440FX_PCI_DEVICE, >> + .instance_size = sizeof(PCII440FXState), >> + .class_init = xen_igd_passthrough_i440fx_class_init, >> +}; >> + >> static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge, >> PCIBus *rootbus) >> { >> @@ -744,6 +771,7 @@ static const TypeInfo i440fx_pcihost_info = { >> static void i440fx_register_types(void) >> { >> type_register_static(&i440fx_info); >> + type_register_static(&xen_igd_passthrough_i440fx_info); >> type_register_static(&piix3_info); >> type_register_static(&piix3_xen_info); >> type_register_static(&i440fx_pcihost_info); >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h >> index 11fb72f..de34aa6 100644 >> --- a/include/hw/i386/pc.h >> +++ b/include/hw/i386/pc.h >> @@ -238,6 +238,8 @@ typedef struct PCII440FXState PCII440FXState; >> #define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost" >> #define TYPE_I440FX_PCI_DEVICE "i440FX" >> >> +#define TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE >> "xen-igd-passthrough-i440FX" >> + >> PCIBus *i440fx_init(const char *host_type, const char *pci_type, >> PCII440FXState **pi440fx_state, int *piix_devfn, >> ISABus **isa_bus, qemu_irq *pic, >> -- >> 1.9.1 >> >> Thanks >> Tiejun > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v4][PATCH 3/5] I440FX_PCI_DEVICE: add pci_type to index Date: Tue, 12 Aug 2014 17:25:00 +0800 Message-ID: <53E9DD6C.5070304@intel.com> References: <1407307835-9692-1-git-send-email-tiejun.chen@intel.com> <1407307835-9692-4-git-send-email-tiejun.chen@intel.com> <20140806094509.GB22307@redhat.com> <53E2009E.4080107@intel.com> <20140806210714.GD22307@redhat.com> <53E2D921.8040501@intel.com> <20140810202759.GA5199@redhat.com> <53E82F88.5020209@intel.com> <20140812085435.GA6440@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140812085435.GA6440@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org To: "Michael S. Tsirkin" Cc: pbonzini@redhat.com, xen-devel@lists.xensource.com, qemu-devel@nongnu.org, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On 2014/8/12 16:54, Michael S. Tsirkin wrote: > On Mon, Aug 11, 2014 at 10:50:48AM +0800, Chen, Tiejun wrote: >> On 2014/8/11 4:27, Michael S. Tsirkin wrote: >>> On Thu, Aug 07, 2014 at 09:40:49AM +0800, Chen, Tiejun wrote: >>>> >>>> >>>> On 2014/8/7 5:07, Michael S. Tsirkin wrote: >>>>> On Wed, Aug 06, 2014 at 06:17:02PM +0800, Chen, Tiejun wrote: >>>>>> On 2014/8/6 17:45, Michael S. Tsirkin wrote: >>>>>>> On Wed, Aug 06, 2014 at 02:50:33PM +0800, Tiejun Chen wrote: >>>>>>>> We need to use this index to reuse this macro later >>>>>>>> >>>>>>>> Signed-off-by: Tiejun Chen >>>>>>> >>>>>>> Which index? >>>>>>> Most users don't need to change. >>>>>>> Just open-code OBJECT_CHECK where necessary, or add >>>>>>> a new wrapper. >>>>>> >>>>>> Okay so what about this? >>>>>> >>>>>> hw:pci-host:piix: define I440FX_PCI_DEVICE_FROM_TYPE >>>>>> >>>>>> We need to introduce I440FX_PCI_DEVICE_FROM_TYPE to get >>>>>> object with type, then we can reuse i440fx_init() simply. >>>>>> >>>>>> Signed-off-by: Tiejun Chen >>>>>> >>>>>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c >>>>>> index 0cd82b8..8c74653 100644 >>>>>> --- a/hw/pci-host/piix.c >>>>>> +++ b/hw/pci-host/piix.c >>>>>> @@ -93,6 +93,9 @@ typedef struct PIIX3State { >>>>>> #define I440FX_PCI_DEVICE(obj) \ >>>>>> OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE) >>>>>> >>>>>> +#define I440FX_PCI_DEVICE_FROM_TYPE(obj, type) \ >>>>>> + OBJECT_CHECK(PCII440FXState, (obj), type) >>>>> >>>>> This is just wrong. If you are casting to PCII440FXState, >>>> >>>> Why? We will have two different QOM typenames of PCII440FXStates. >>>> >>>>> there is no reason not to use TYPE_I440FX_PCI_DEVICE. >>>> >>>> As you know we already have this original, >>>> >>>> static const TypeInfo i440fx_info = { >>>> .name = TYPE_I440FX_PCI_DEVICE, >>>> .parent = TYPE_PCI_DEVICE, >>>> .instance_size = sizeof(PCII440FXState), >>>> .class_init = i440fx_class_init, >>>> }; >>>> >>>> and in patch #4, we will register that new host bridge to IGD passthrough: >>>> >>>> static const TypeInfo xen_igd_passthrough_i440fx_info = { >>>> .name = TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE, >>>> .parent = TYPE_PCI_DEVICE, >>>> .instance_size = sizeof(PCII440FXState), >>>> .class_init = xen_igd_passthrough_i440fx_class_init, >>>> }; >>> >>> My idea is to inherit TYPE_I440FX_PCI_DEVICE instead. >> >> So here, this mean xen_igd_passthrough_i440fx_info's parent should be >> TYPE_I440FX_PCI_DEVICE, right? >> >>> Then you can reuse regular piix code which casts to >>> TYPE_I440FX_PCI_DEVICE because >>> TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE would >>> be a subclass of TYPE_I440FX_PCI_DEVICE. >>> >> >> So since then, we can reuse i440fx_initfn. >> >> As a summary, what we should do is like the following, >> >> xen:hw:pci-host:piix: create host bridge to passthrough >> >> Implement a pci host bridge specific to passthrough. Actually >> this just inherits the standard one. >> >> This is based on http://patchwork.ozlabs.org/patch/363810/. >> >> Signed-off-by: Tiejun Chen >> --- >> hw/pci-host/piix.c | 28 ++++++++++++++++++++++++++++ >> include/hw/i386/pc.h | 2 ++ >> 2 files changed, 30 insertions(+) >> >> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c >> index 0cd82b8..2ccd9ee 100644 >> --- a/hw/pci-host/piix.c >> +++ b/hw/pci-host/piix.c >> @@ -703,6 +703,33 @@ static const TypeInfo i440fx_info = { >> .class_init = i440fx_class_init, >> }; >> >> +static void xen_igd_passthrough_i440fx_class_init(ObjectClass *klass, void >> *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >> + >> + k->init = i440fx_initfn; >> + k->vendor_id = PCI_VENDOR_ID_INTEL; >> + k->device_id = PCI_DEVICE_ID_INTEL_82441; >> + k->revision = 0x02; >> + k->class_id = PCI_CLASS_BRIDGE_HOST; >> + dc->desc = "IGD PT XEN Host bridge"; >> + dc->vmsd = &vmstate_i440fx; >> + /* >> + * PCI-facing part of the host bridge, not usable without the >> + * host-facing part, which can't be device_add'ed, yet. >> + */ >> + dc->cannot_instantiate_with_device_add_yet = true; >> + dc->hotpluggable = false; >> +} >> + > > A bunch of code still duplicated here. Only override what you have to. You mean all stuffs can be inherited from its own parent, even including vendor_id/device_id? If yes, currently the follows should be enough, static void xen_igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); dc->desc = "IGD PT XEN Host bridge"; } We will add k->config_write/k->config_read here when we introduce IGD passthrough later. Thanks Tiejun > > >> +static const TypeInfo xen_igd_passthrough_i440fx_info = { >> + .name = TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE, >> + .parent = TYPE_I440FX_PCI_DEVICE, >> + .instance_size = sizeof(PCII440FXState), >> + .class_init = xen_igd_passthrough_i440fx_class_init, >> +}; >> + >> static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge, >> PCIBus *rootbus) >> { >> @@ -744,6 +771,7 @@ static const TypeInfo i440fx_pcihost_info = { >> static void i440fx_register_types(void) >> { >> type_register_static(&i440fx_info); >> + type_register_static(&xen_igd_passthrough_i440fx_info); >> type_register_static(&piix3_info); >> type_register_static(&piix3_xen_info); >> type_register_static(&i440fx_pcihost_info); >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h >> index 11fb72f..de34aa6 100644 >> --- a/include/hw/i386/pc.h >> +++ b/include/hw/i386/pc.h >> @@ -238,6 +238,8 @@ typedef struct PCII440FXState PCII440FXState; >> #define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost" >> #define TYPE_I440FX_PCI_DEVICE "i440FX" >> >> +#define TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE >> "xen-igd-passthrough-i440FX" >> + >> PCIBus *i440fx_init(const char *host_type, const char *pci_type, >> PCII440FXState **pi440fx_state, int *piix_devfn, >> ISABus **isa_bus, qemu_irq *pic, >> -- >> 1.9.1 >> >> Thanks >> Tiejun > >