Re: [PATCH v3 1/3] xen/igd: get PCH info from host sysfs

On 7/23/2026 9:09 AM, Tomita Moeko wrote:
> Sorry I am not the maintainer/reviewer of Xen IGD passthrough, just out
> of my personal interest

I appreciate your review. Thanks!

> 
> On 2026-07-10 02:35, Chuck Zmudzinski wrote:
>> The igd_combo_id_infos[] data is more than 10 years
>> out of date with many Intel IGD devices missing from
>> igd_combo_id_infos[]. This means that many devices
>> that could be supported will not work with the
>> current implementation.
>> 
>> For newer devices not listed in igd_combo_id_infos[],
>> get infos from the host sysfs to enable support for the
>> newer devices not listed in igd_combo_id_infos[].
>> 
>> Introduce the helper function xen_pt_get_host_pch_info
>> to facilitate getting the necessary information from
>> sysfs.
>> 
>> Also, use errp in xen_igd_passthrough_isa_bridge_create
>> to set errors from xen_pt_get_host_pch_info.
>> 
>> Signed-off-by: Chuck Zmudzinski <[email protected]>
>> ---
>> Changes in v2:
>>   - call error_setg* after closing files instead of before closing
>>     files
>>   - in last line of commit message change "to propagate errors" to
>>     "to set errors"
>>   - add stable to Cc list
>> 
>> Changes in v3:
>>   - whitespace fix at line 380 of xen_pt_graphics.c
>>   - fix Cc address for qemu-stable
>> 
>>  hw/xen/xen_pt.c          |  2 +-
>>  hw/xen/xen_pt_graphics.c | 82 ++++++++++++++++++++++++++++++++++++++--
>>  include/hw/xen/xen_igd.h |  3 +-
>>  3 files changed, 82 insertions(+), 5 deletions(-)
>> 
>> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
>> index 0fe9c0a..474606e 100644
>> --- a/hw/xen/xen_pt.c
>> +++ b/hw/xen/xen_pt.c
>> @@ -867,7 +867,7 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
>>          }
>>  
>>          /* Register ISA bridge for passthrough GFX. */
>> -        xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
>> +        xen_igd_passthrough_isa_bridge_create(s, &s->real_device, errp);
> 
> The `errp` need to be handled here if any error occurs.
> 
>>      }
>>  
>>      /* Handle real device's MMIO/PIO BARs */
>> diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c
>> index 7df9344..2ef941e 100644
>> --- a/hw/xen/xen_pt_graphics.c
>> +++ b/hw/xen/xen_pt_graphics.c
>> @@ -376,8 +376,75 @@ static void pt_graphics_register_types(void)
>>  }
>>  type_init(pt_graphics_register_types)
>>  
>> +static void xen_pt_get_host_pch_info(PCIDevice *dev, uint16_t *pch_dev_id,
>> +                                     uint8_t *pch_rev_id, Error **errp)
>> +{
>> +    FILE *fp1, *fp2;
>> +    char *endptr;
>> +    char device_id[7], rev[5];
>> +    size_t len;
>> +    const char *device = "/sys/bus/pci/devices/0000:00:1f.0/device";
>> +    const char *revision = "/sys/bus/pci/devices/0000:00:1f.0/revision";
>> +    unsigned long val;
>> +
>> +    fp1 = fopen(device, "r");
>> +    if (fp1 == NULL) {
>> +        error_setg_errno(errp, errno, "Cannot open %s", device);
>> +        return;
>> +    }
>> +    fp2 = fopen(revision, "r");
>> +    if (fp2 == NULL) {
>> +        fclose(fp1);
>> +        error_setg_errno(errp, errno, "Cannot open %s", revision);
>> +        return;
>> +    }
>> +
>> +    len = fread(device_id, 1, 7, fp1);
>> +    if (!len) {
>> +        fclose(fp1);
>> +        fclose(fp2);
>> +        error_setg(errp, "Cannot read %s", device);
>> +        return;
>> +    }
>> +    len = fread(rev, 1, 5, fp2);
>> +    if (!len) {
>> +        fclose(fp1);
>> +        fclose(fp2);
>> +        error_setg(errp, "Cannot read %s", revision);
>> +        return;
>> +    }
>> +    fclose(fp1);
>> +    fclose(fp2);
>> +
>> +    val = strtoul(device_id, &endptr, 16);
>> +    if (val > 0xffff) {
>> +        error_setg(errp, "PCH device id is out of range: 0x%lx", val);
>> +        return;
>> +    }
>> +    if ((endptr > device_id) && (errno != ERANGE) &&
>> +        (errno != EINVAL)) {
>> +        *pch_dev_id = (uint16_t)val;
>> +    } else {
>> +        error_setg_errno(errp, errno, "device id strtoul "
>> +                                      "conversion failed");
>> +        return;
>> +    }
>> +    val = strtoul(rev, &endptr, 16);
>> +    if (val > 0xff) {
>> +        error_setg(errp, "PCH revision is out of range: 0x%lx", val);
>> +        return;
>> +    }
>> +    if ((endptr > rev) && (errno != ERANGE) && (errno != EINVAL)) {
>> +        *pch_rev_id = (uint8_t)val;
>> +    } else {
>> +        error_setg_errno(errp, errno, "revision strtoul "
>> +                                      "conversion failed");
>> +    }
>> +}
>> +
> 
> It would be better to reuse existing xen_host_pci_device_get() to read host
> LPC bridge ids instead of parsing it on our own here.

Ah yes, I see how that can be done and I agree that would be better. I will
re-write xen_pt_get_host_pch_info() using the functions declared in
xen-host-pci-device.h for the next version of this patch.

> 
>>  void xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
>> -                                           XenHostPCIDevice *dev)
>> +                                           XenHostPCIDevice *dev,
>> +                                           Error **errp)
>>  {
>>      PCIBus *bus = pci_get_bus(&s->dev);
>>      struct PCIDevice *bridge_dev;
>> @@ -394,7 +461,16 @@ void xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
>>          }
>>      }
>>  
>> -    if (pch_dev_id == 0xffff) {
>> +    /* Newer devices get PCH infos from host sysfs */
>> +    if ((pch_dev_id == 0xffff) || !pch_rev_id) {
>> +        xen_pt_get_host_pch_info(&s->dev, &pch_dev_id, &pch_rev_id, errp);
>> +    }
>> +
>> +    XEN_PT_LOG(&s->dev, "PCH device id: 0x%x\n", pch_dev_id);
>> +    XEN_PT_LOG(&s->dev, "PCH revision: 0x%x\n", pch_rev_id);
>> +
>> +    if ((pch_dev_id == 0xffff) || !pch_rev_id) {
>> +        error_setg(errp, "failed to get PCH device id or revision");
>>          return;
>>      }
>>  
>> @@ -406,7 +482,7 @@ void xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
>>       * Note that vendor id is always PCI_VENDOR_ID_INTEL.
>>       */
>>      if (!bridge_dev) {
>> -        fprintf(stderr, "set igd-passthrough-isa-bridge failed!\n");
>> +        error_setg(errp, "set igd-passthrough-isa-bridge failed!");
>>          return;
>>      }
>>      pci_config_set_device_id(bridge_dev->config, pch_dev_id);
>> diff --git a/include/hw/xen/xen_igd.h b/include/hw/xen/xen_igd.h
>> index 7ffca06..da51f09 100644
>> --- a/include/hw/xen/xen_igd.h
>> +++ b/include/hw/xen/xen_igd.h
>> @@ -22,7 +22,8 @@ uint32_t igd_read_opregion(XenPCIPassthroughState *s);
>>  void xen_igd_reserve_slot(PCIBus *pci_bus);
>>  void igd_write_opregion(XenPCIPassthroughState *s, uint32_t val);
>>  void xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
>> -                                           XenHostPCIDevice *dev);
>> +                                           XenHostPCIDevice *dev,
>> +                                           Error **errp);
>>  
>>  static inline bool is_igd_vga_passthrough(XenHostPCIDevice *dev)
>>  {
>