| Message ID | 1426669601-13891-4-git-send-email-tiejun.chen@intel.com |
|---|---|
| State | New |
| Headers | show |
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 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 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? > +#define TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE "xen-igd-passthrough-i440FX" One xen leftover slipped through here. cheers, Gerd
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. Thanks Tiejun
Michael and Gerd, I don't see any more comments for this revision, so what's next that I should do? Thanks Tiejun On 2015/3/19 9:01, 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. > > Thanks > Tiejun > > >
On Thu, Mar 26, 2015 at 03:57:18PM +0800, Chen, Tiejun wrote: > Michael and Gerd, > > I don't see any more comments for this revision, so what's next that I > should do? > > Thanks > Tiejun > > On 2015/3/19 9:01, Chen, Tiejun wrote: It's only been a week, and we are busy finalizing 2.3. So please give people time to review.
On 2015/3/26 16:15, Michael S. Tsirkin wrote: > On Thu, Mar 26, 2015 at 03:57:18PM +0800, Chen, Tiejun wrote: >> Michael and Gerd, >> >> I don't see any more comments for this revision, so what's next that I >> should do? >> >> Thanks >> Tiejun >> >> On 2015/3/19 9:01, Chen, Tiejun wrote: > > It's only been a week, and we are busy finalizing 2.3. So please give Oh, understood. Thanks Tiejun
Chen, Tiejun wrote > +/* 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 */ > +}; > + I tested this patch and found that the registers 0xa0(top of memory) and 0xb0(ILK: BSM) were necessary for Win XP. Both of them are 4 bytes. Chen, Tiejun wrote > +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); Here, when we call function host_pci_config_read, the parameter val is passed by value not address, so the value of val will not be changed after call host_pci_config_read. So I think host_pci_config_read need update and the third parameter should be an address. Thanks -- View this message in context: http://qemu.11.n7.nabble.com/v7-PATCH-00-10-xen-add-Intel-IGD-passthrough-support-tp319698p323854.html Sent from the Developer mailing list archive at Nabble.com.
On 2015/3/26 16:15, Michael S. Tsirkin wrote: > On Thu, Mar 26, 2015 at 03:57:18PM +0800, Chen, Tiejun wrote: >> Michael and Gerd, >> >> I don't see any more comments for this revision, so what's next that I >> should do? >> >> Thanks >> Tiejun >> >> On 2015/3/19 9:01, Chen, Tiejun wrote: > > It's only been a week, and we are busy finalizing 2.3. So please give > people time to review. Ping Thanks Tiejun
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? > Thanks > Tiejun
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
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 fixed in the next revision of the patchset"?
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"? >
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c index c812eaa..0906ba5 100644 --- a/hw/pci-host/piix.c +++ b/hw/pci-host/piix.c @@ -728,6 +728,87 @@ static const TypeInfo i440fx_info = { .class_init = i440fx_class_init, }; +/* IGD Passthrough Host Bridge. */ +typedef struct { + uint8_t offset; + uint8_t len; +} IGDHostInfo; + +/* 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 */ +}; + +static int host_pci_config_read(int pos, int len, uint32_t val) +{ + char path[PATH_MAX]; + int config_fd; + ssize_t size = sizeof(path); + /* Access real host bridge. */ + int rc = snprintf(path, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s", + 0, 0, 0, 0, "config"); + + if (rc >= size || rc < 0) { + return -ENODEV; + } + + config_fd = open(path, O_RDWR); + if (config_fd < 0) { + return -ENODEV; + } + + do { + rc = pread(config_fd, (uint8_t *)&val, len, pos); + } while (rc < 0 && (errno == EINTR || errno == EAGAIN)); + if (rc != len) { + return -errno; + } + + return 0; +} + +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; +} + +static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); + + k->init = igd_pt_i440fx_initfn; + dc->desc = "IGD Passthrough Host bridge"; +} + +static const TypeInfo igd_passthrough_i440fx_info = { + .name = TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE, + .parent = TYPE_I440FX_PCI_DEVICE, + .instance_size = sizeof(PCII440FXState), + .class_init = igd_passthrough_i440fx_class_init, +}; + static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge, PCIBus *rootbus) { @@ -769,6 +850,7 @@ static const TypeInfo i440fx_pcihost_info = { static void i440fx_register_types(void) { type_register_static(&i440fx_info); + type_register_static(&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 99dc26b..ec3ca88 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -233,6 +233,8 @@ typedef struct PCII440FXState PCII440FXState; #define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost" #define TYPE_I440FX_PCI_DEVICE "i440FX" +#define TYPE_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,
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. Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> --- hw/pci-host/piix.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++ include/hw/i386/pc.h | 2 ++ 2 files changed, 84 insertions(+)