Re: [Qemu-devel] [v7][PATCH 03/10] piix: create host bridge to passthrough

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2015/6/2 17:17, Michael S. Tsirkin wrote:
> On Tue, Jun 02, 2015 at 08:50:58AM +0800, Chen, Tiejun wrote:
>> On 2015/6/1 2:11, Michael S. Tsirkin wrote:
>>> On Thu, Mar 19, 2015 at 09:01:27AM +0800, Chen, Tiejun wrote:
>>>> On 2015/3/18 18:21, Gerd Hoffmann wrote:
>>>>> On Mi, 2015-03-18 at 17:06 +0800, Tiejun Chen wrote:
>>>>>> Implement a pci host bridge specific to passthrough. Actually
>>>>>> this just inherits the standard one. And we also just expose
>>>>>> a minimal real host bridge pci configuration subset.
>>>>>
>>>>>> +/* Here we just expose minimal host bridge offset subset. */
>>>>>> +static const IGDHostInfo igd_host_bridge_infos[] = {
>>>>>> +    {0x08, 2},  /* revision id */
>>>>>> +    {0x2c, 2},  /* sybsystem vendor id */
>>>>>> +    {0x2e, 2},  /* sybsystem id */
>>>>>> +    {0x50, 2},  /* SNB: processor graphics control register */
>>>>>> +    {0x52, 2},  /* processor graphics control register */
>>>>>> +    {0xa4, 4},  /* SNB: graphics base of stolen memory */
>>>>>> +    {0xa8, 4},  /* SNB: base of GTT stolen memory */
>>>>>> +};
>>>>>
>>>>> Hmm, no vendor/device id here?  Will the idg guest drivers happily read
>>>>
>>>> These two emulated values are already fine.
>>>>
>>>> And this is a long story. At the beginning, we were initially trying to
>>>> expose more,
>>>>
>>>> +    case 0x00:        /* vendor id */
>>>> +    case 0x02:        /* device id */
>>>> +    case 0x08:        /* revision id */
>>>> +    case 0x2c:        /* sybsystem vendor id */
>>>> +    case 0x2e:        /* sybsystem id */
>>>> +    case 0x50:        /* SNB: processor graphics control register */
>>>> +    case 0x52:        /* processor graphics control register */
>>>> +    case 0xa0:        /* top of memory */
>>>> +    case 0xb0:        /* ILK: BSM: should read from dev 2 offset 0x5c */
>>>> +    case 0x58:        /* SNB: PAVPC Offset */
>>>> +    case 0xa4:        /* SNB: graphics base of stolen memory */
>>>> +    case 0xa8:        /* SNB: base of GTT stolen memory */
>>>>
>>>> But this brought some discussions with Paolo and Michael, and then plus our
>>>> further experiment, now as everyone expect, this is a minimal real host
>>>> bridge pci configuration subset which we need to expose.
>>>>
>>>>> graphics control registers from i440fx even though this chipset never
>>>>> existed in a igd variant?
>>>>>
>>>>> [ just wondering, if it works that way fine, it certainly makes things
>>>>
>>>> Yes, it works fine.
>>>>
>>>>>    easier for the firmware which uses host bridge pci id to figure
>>>>>    whenever it is running @ i440fx or q35 ].
>>>>>
>>>>>> +static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
>>>>>> +{
>>>>>> +    uint32_t val = 0;
>>>>>> +    int rc, i, num;
>>>>>> +    int pos, len;
>>>>>> +
>>>>>> +    num = ARRAY_SIZE(igd_host_bridge_infos);
>>>>>> +    for (i = 0; i < num; i++) {
>>>>>> +        pos = igd_host_bridge_infos[i].offset;
>>>>>> +        len = igd_host_bridge_infos[i].len;
>>>>>> +        rc = host_pci_config_read(pos, len, val);
>>>>>> +        if (rc) {
>>>>>> +            return -ENODEV;
>>>>>> +        }
>>>>>> +        pci_default_write_config(pci_dev, pos, val, len);
>>>>>> +    }
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>
>>>>> Nothing i440fx specific in here, just copying some host bridge pci
>>>>> config space bits.  I guess we can sub-class the q35 host bridge the
>>>>> same way and even reuse the init function?
>>>>
>>>> This is our another discussion long time ago :)
>>>>
>>>> Currently Xen don't run with q35. If I remember those discussions correctly,
>>>> something is still restricted to Windows.
>>>>
>>>>>
>>>>>> +#define TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE "xen-igd-passthrough-i440FX"
>>>>>
>>>>> One xen leftover slipped through here.
>>>>
>>>> Fixed.
>>>
>>> So should I expect v8 then?
>>>
>>
>> For this revision we just had only this valuable comment, and that is just
>> about to rename a macro, so I think its not worth resend this, right?
>>
>> If I'm wrong let me know :)
>>
>> Thanks
>> Tiejun
>
> I didn't follow closely so I have no idea how valuable was the last
> round of feedback, but when you say "Fixed" don't you mean "will be

It should be, but in this revision, I just received this sole comment 
until now so I mean its not worth resending a new review just with this 
fix :)

Anyway, its not big deal, just let me send this out as you expect.

Thanks
Tiejun

> fixed in the next revision of the patchset"?
>