Re: [PATCH] hw/vfio/pci-quirks: Fix broken legacy IGD passthrough

[Thomas Huth]

On 10/06/2020 10.25, Philippe Mathieu-Daudé wrote:
> On 6/10/20 9:59 AM, Thomas Huth wrote:
>> On 10/06/2020 09.53, Philippe Mathieu-Daudé wrote:
>>> On 6/10/20 9:50 AM, Thomas Huth wrote:
>>>> On 10/06/2020 09.31, Philippe Mathieu-Daudé wrote:
>>>>> On 6/10/20 5:51 AM, Thomas Huth wrote:
>>>>>> The #ifdef CONFIG_VFIO_IGD in pci-quirks.c is not working since the
>>>>>> required header config-devices.h is not included, so that the legacy
>>>>>> IGD passthrough is currently broken. Let's include the right header
>>>>>> to fix this issue.
>>>>>>
>>>>>> Buglink: https://bugs.launchpad.net/qemu/+bug/1882784
>>>>>> Fixes: 29d62771c81d8fd244a67c14a1d968c268d3fb19
>>>>>>        ("hw/vfio: Move the IGD quirk code to a separate file")
>>>>>
>>>>> What about shorter tag?
>>>>>
>>>>> Fixes: 29d62771c81 ("vfio: Move the IGD quirk code to a separate file")
>>>>
>>>> I always forget whether to use the short or the long version for
>>>> "Fixes:" ... this can hopefully be fixed (if necessary) when the patch
>>>> gets picked up.
>>>>
>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>>> ---
>>>>>>  hw/vfio/pci-quirks.c | 1 +
>>>>>>  1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
>>>>>> index f2155ddb1d..3158390db1 100644
>>>>>> --- a/hw/vfio/pci-quirks.c
>>>>>> +++ b/hw/vfio/pci-quirks.c
>>>>>> @@ -11,6 +11,7 @@
>>>>>>   */
>>>>>>  
>>>>>>  #include "qemu/osdep.h"
>>>>>> +#include "config-devices.h"
>>>>>
>>>>> I've been wondering how we can avoid that mistake in the
>>>>> future, but can find anything beside human review.
>>>>
>>>> I think in the long term, we should include config-devices.h in osdep.h,
>>>> just like config-host.h and config-target.h is already included there.
>>>> Everything else is just too confusing. But then we should also add a
>>>> mechanism to poison the switches from config-devices.h in common code...
>>>
>>> We only need it for the files under hw/, right?
>>
>> qtest.c in the main directory includes it, too.
> 
> hw/ and qtests could include "hw/hw.h" instead of affecting all the
> codebase via "qemu/osdep.h".

I don't think that's a good idea - in that case, you have to make sure
to include hw/hw.h everywhere again, so you don't gain that much
compared to including config-devices.h directly everywhere. osdep.h is
our header that has to be included everywhere, so if we want to make
sure that these defines are available everywhere, we have to include it
from osdep.h.
Apart from that, hw/hw.h just contains one more prototype - which likely
should be renamed to cpu_hw_error() and moved to a cpu header instead,
so that we can finally delete hw/hw.h completely.

 Thomas