Re: [Qemu-devel] [v8][RESEND][PATCH 08/10] xen, gfx passthrough: register a isa bridge

[Chen, Tiejun]

> > +static void
> > +xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
> > +                                      XenHostPCIDevice *dev)
> > +{
> > +    uint16_t gpu_dev_id;
> > +    PCIDevice *d = &s->dev;
> > +
> > +    if (!is_igd_vga_passthrough(dev)) {
> > +        return;
> > +    }
>
> I would rather move the if into xen_pt_initfn, otherwise reading
> xen_pt_initfn it looks like we are going to create an isa bridge
> regardless.

This makes sense.

> > +    gpu_dev_id = dev->device_id;
> > +    igd_passthrough_isa_bridge_create(d->bus, gpu_dev_id);
> > +}
> > +
> >   /* init */
> >
> >   static int xen_pt_initfn(PCIDevice *d)
> > @@ -725,6 +740,9 @@ static int xen_pt_initfn(PCIDevice *d)

I'd like to add something like,

if (!is_igd_vga_passthrough(&s->real_device)) {
    XEN_PT_ERR(d, "Need to enable igd-passthru if you're trying"
               " to passthrough IGD GFX.\n");
    xen_host_pci_device_put(&s->real_device); 

    return -1; 

        }

> >               xen_host_pci_device_put(&s->real_device);
> >               return -1;
> >           }
> > +
> > +        /* Register ISA bridge for passthrough GFX. */
> > +        xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
> >       }
> >
> >       /* Handle real device's MMIO/PIO BARs */
> > diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> > index 4356af4..703148e 100644
> > --- a/include/hw/xen/xen.h
> > +++ b/include/hw/xen/xen.h
> > @@ -51,4 +51,5 @@ void xen_register_framebuffer(struct MemoryRegion *mr);
> >   #  define HVM_MAX_VCPUS 32
> >   #endif
> >
> > +extern void igd_passthrough_isa_bridge_create(PCIBus *bus, uint16_t 
> > gpu_dev_id);
> >   #endif /* QEMU_HW_XEN_H */
>
> Either static or extern. You probably want to drop this declaration.

I just guess you're confused between igd_passthrough_isa_bridge_create() 
and xen_igd_passthrough_isa_bridge_create(), or am I wrong?

Note igd_passthrough_isa_bridge_create() is defined in the pc_piix.c file.

Thanks
Tiejun