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

[Chuck Zmudzinski <brchuckz@xxxxxxxxxxxx>]

On 7/23/2026 9:09 AM, Tomita Moeko wrote:
>> ...
>> 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 <brchuckz@xxxxxxx>
>> ---
>> 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.

Do you mean calling it like this as a supported way to handle an error:

    xen_igd_passthrough_isa_bridge_create(s, &s->real_device, &error_fatal);

IIUC, that would cause Qemu to exit(1) here if there was any error.

I didn't do that because I thought if there was an error one of the
parents (pci or qdev) would handle the error appropriately since we
are passing 'errp' from xen_pt_realize which I think comes from pci
and qdev, but maybe not. I have not tested how an error is handled
with this version of the patch and I am certainly no expert in how
error handling should be done here, so thanks for alerting me to this
question.

I have also seen in the Qemu source the use of local_err and maybe
using &error_fatal is something like using a local_err instead of the
errp from xen_pt_realize.

I will accept any suggestions from more knowledgeable people about
how best to handle the errors here. I can also do some tests by
faking an error here and make sure the the error does get handled.
I do think if the LPC bridge creation fails it should be a fatal error
and if am reading the current code we have correctly, we currently
just print a message to stderr if bridge creation fails without doing
anything about that error. I also think maybe if for some reason we
can't get a revision number for the LPC bridge, maybe that should
not be a fatal error.

I will make sure the next version of the patch will not be posted until
I have verified the errors are handled appropriately.

Cheers,

Chuck