From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:55427)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <tiejun.chen@intel.com>) id 1ZAq97-00018r-AV
	for qemu-devel@nongnu.org; Thu, 02 Jul 2015 21:52:06 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <tiejun.chen@intel.com>) id 1ZAq92-0008Ej-5q
	for qemu-devel@nongnu.org; Thu, 02 Jul 2015 21:52:05 -0400
Received: from mga11.intel.com ([192.55.52.93]:36074)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <tiejun.chen@intel.com>) id 1ZAq92-0008EY-11
	for qemu-devel@nongnu.org; Thu, 02 Jul 2015 21:52:00 -0400
Message-ID: <5595EABB.1000808@intel.com>
Date: Fri, 03 Jul 2015 09:51:55 +0800
From: "Chen, Tiejun" <tiejun.chen@intel.com>
MIME-Version: 1.0
References: <1433493901-9332-1-git-send-email-tiejun.chen@intel.com>
	<1433493901-9332-9-git-send-email-tiejun.chen@intel.com>
	<alpine.DEB.2.02.1507011653190.17378@kaball.uk.xensource.com>
In-Reply-To: <alpine.DEB.2.02.1507011653190.17378@kaball.uk.xensource.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [v8][RESEND][PATCH 08/10] xen,
 gfx passthrough: register a isa bridge
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: qemu-devel@nongnu.org, pbonzini@redhat.com, rth@twiddle.net, ehabkost@redhat.com, mst@redhat.com

>> +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