From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39814) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XH8YI-0005PF-GD for qemu-devel@nongnu.org; Tue, 12 Aug 2014 05:39:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XH8YD-0001m9-Uu for qemu-devel@nongnu.org; Tue, 12 Aug 2014 05:39:34 -0400 Received: from mga01.intel.com ([192.55.52.88]:24908) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XH8YD-0001lx-N5 for qemu-devel@nongnu.org; Tue, 12 Aug 2014 05:39:29 -0400 Message-ID: <53E9E0CD.7090409@intel.com> Date: Tue, 12 Aug 2014 17:39:25 +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> <53E9DD6C.5070304@intel.com> In-Reply-To: <53E9DD6C.5070304@intel.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 17:25, Chen, Tiejun wrote: > 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. > Looks this is true after a quick test, so I will update this then send v5. Thanks Tiejun