Re: [Qemu-devel] [PATCH 2/4] xen:hw:pci-host:piix: create host bridge to passthrough

[Chen, Tiejun]

On 2014/7/29 19:17, Michael S. Tsirkin wrote:
> On Thu, Jul 24, 2014 at 07:30:27PM +0800, Tiejun Chen wrote:
> > Implement that pci
>
> s/that/a/

Fixed.

> > host bridge to specific
>
> s/to specific/specific/

Fixed.

> > to passthrough. Actually
> > this just inherit
>
> s/inherit/inherits/

Fixed.

> > the standard one.
> >
> > This is based on http://patchwork.ozlabs.org/patch/363810/.
> >
> > Signed-off-by: Tiejun Chen <address@hidden>
> > ---
> >   hw/pci-host/piix.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 43 insertions(+)
> >
> > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > index e0e0946..9feddf5 100644
> > --- a/hw/pci-host/piix.c
> > +++ b/hw/pci-host/piix.c
> > @@ -34,6 +34,7 @@
> >   #include "sysemu/sysemu.h"
> >   #include "hw/i386/ioapic.h"
> >   #include "qapi/visitor.h"
> > +#include "hw/xen/xen_pt.h"
>
> What call needs this, exactly?

Another fault when I grab something from the original patches.

> >   /*
> >    * I440FX chipset data sheet.
> > @@ -44,6 +45,10 @@
> >   #define I440FX_PCI_HOST_BRIDGE(obj) \
> >       OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE)
>
> OK cool, but don't you want to put "igd" and "passthrough"
> somewhere in the name? Maybe "legacy" as well since
> future drivers will work with existing machine type, right?

Looks good so what about this prefix, IGD_PT_XXX/igd_pt_xxx?

> Applies to functions and macro names as well.
>
> > +#define TYPE_I440FX_XEN_PCI_DEVICE "i440FX-xen"
> > +#define I440FX_XEN_PCI_DEVICE(obj) \
> > +    OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_XEN_PCI_DEVICE)
> > +
> >   typedef struct I440FXState {
> >       PCIHostState parent_obj;
> >       PcPciInfo pci_info;
> > @@ -305,6 +310,16 @@ static int i440fx_initfn(PCIDevice *dev)
> >       return 0;
> >   }
> >
> > +static int i440fx_xen_initfn(PCIDevice *dev)
> > +{
> > +    PCII440FXState *d = I440FX_XEN_PCI_DEVICE(dev);
> > +
> > +    dev->config[I440FX_SMRAM] = 0x02;
> > +
> > +    cpu_smm_register(&i440fx_set_smm, d);
> > +    return 0;
> > +}
> > +
> >   PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> >                       int *piix3_devfn,
> >                       ISABus **isa_bus, qemu_irq *pic,
> > @@ -704,6 +719,33 @@ static const TypeInfo i440fx_info = {
> >       .class_init    = i440fx_class_init,
> >   };
> >
> > +static void i440fx_xen_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > +
> > +    k->init = i440fx_xen_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_ISA;
> > +    dc->desc = "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 i440fx_xen_info = {
> > +    .name          = TYPE_I440FX_XEN_PCI_DEVICE,
> > +    .parent        = TYPE_PCI_DEVICE,
> > +    .instance_size = sizeof(PCII440FXState),
> > +    .class_init    = i440fx_xen_class_init,
> > +};
> > +
> >   static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
> >                                                   PCIBus *rootbus)
> >   {
> > @@ -745,6 +787,7 @@ static const TypeInfo i440fx_pcihost_info = {
> >   static void i440fx_register_types(void)
> >   {
> >       type_register_static(&i440fx_info);
> > +    type_register_static(&i440fx_xen_info);
> >       type_register_static(&piix3_info);
> >       type_register_static(&piix3_xen_info);
> >       type_register_static(&i440fx_pcihost_info);
> > --
> > 1.9.1