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

[Tomita Moeko <tomitamoeko@xxxxxxxxx>]

On 2026-07-25 12:19, Chuck Zmudzinski wrote:
> 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.

Here I mean, since you've changed the function to use errp, then it should be
handled as other function calls in xen_pt_realize() like

    xen_igd_passthrough_isa_bridge_create(s, &s->real_device, errp);
    if (errp) {
        xen_host_pci_device_put(&s->real_device);  // cleanup
        return;
    }

The realized function is called in device_set_realized() with

    dc->realize(dev, &local_err);
    if (local_err != NULL) {
        goto fail;
    }
    
In your code, if any error occurs, though it contiunes, it would be still
treated as a failure. I'm not sure if this failure would have any impact
to further steps in xen_pt_realize(), but since it ultimately fails, fail
fast is always preferred.

If you think the error can be ignored, then keep using current fprintf()
or error_report() would be the best choice.

Thanks,
Moeko

> 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