| Message ID | 53E82F88.5020209@intel.com |
|---|---|
| State | New |
| Headers | show |
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 <tiejun.chen@intel.com> > >>>>> > >>>>>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 <tiejun.chen@intel.com> > >>>> > >>>>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 <tiejun.chen@intel.com> > --- > 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. > +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
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 <tiejun.chen@intel.com> >>>>>>> >>>>>>> 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 <tiejun.chen@intel.com> >>>>>> >>>>>> 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 <tiejun.chen@intel.com> >> --- >> 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 > >
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 <tiejun.chen@intel.com> >>>>>>>> >>>>>>>> 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 <tiejun.chen@intel.com> >>>>>>> >>>>>>> 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 <tiejun.chen@intel.com> >>> --- >>> 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
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; +} + +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,