Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table

[Liav Albani <liavalb@gmail.com>]

On 3/2/22 14:42, Michael S. Tsirkin wrote:
> On Wed, Mar 02, 2022 at 10:44:03AM +0530, Ani Sinha wrote:
>> On Wed, Mar 2, 2022 at 12:50 AM Liav Albani <liavalb@gmail.com> wrote:
>>>
>>> On 3/1/22 11:52, Ani Sinha wrote:
>>>> On Tue, 1 Mar 2022, Igor Mammedov wrote:
>>>>
>>>>> On Mon, 28 Feb 2022 22:17:32 +0200
>>>>> Liav Albani <liavalb@gmail.com> wrote:
>>>>>
>>>>>> This can allow the guest OS to determine more easily if i8042 controller
>>>>>> is present in the system or not, so it doesn't need to do probing of the
>>>>>> controller, but just initialize it immediately, before enumerating the
>>>>>> ACPI AML namespace.
>>>>>>
>>>>>> This change only applies to the x86/q35 machine type, as it uses FACP
>>>>>> ACPI table with revision higher than 1, which should implement at least
>>>>>> ACPI 2.0 features within the table, hence it can also set the IA-PC boot
>>>>>> flags register according to the ACPI 2.0 specification.
>>>>>>
>>>>>> Signed-off-by: Liav Albani <liavalb@gmail.com>
>>>>>> ---
>>>>>>    hw/acpi/aml-build.c         | 11 ++++++++++-
>>>>>>    hw/i386/acpi-build.c        |  9 +++++++++
>>>>>>    hw/i386/acpi-microvm.c      |  9 +++++++++
>>>>> commit message says it's q35 specific, so wy it touched microvm anc piix4?
>>>> Igor is correct. Although I see that currently there are no 8042 devices
>>>> for microvms, maybe we should be conservative and add the code to detect
>>>> the device anyway. In that case, the change could affect microvms too when
>>>> such devices get added in the future.
>>>>
>>>>
>>>> echo -e "info qtree\r\nquit\r\n" | ./qemu-system-x86_64 -machine microvm
>>>> -monitor stdio 2>/dev/null | grep 8042
>>>>
>>>> <empty>
>>> What about this?
>>>
>>> echo -e "info qtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm
>>> -device i8042 -monitor stdio 2>/dev/null | grep 8042
>>>
>>> Or this?
>>>
>>> echo -e "info mtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm
>>> -device i8042 -monitor stdio 2>/dev/null | grep 8042
>> On both occasions you are explicitly adding the device.
> Yes of course. It seems a bit cleaner to have -device i8042 -monitor
> stdio give us the correct ACPI table even if there's no pressing need
> for this ATM, simply because it's not much more code, and because if we
> don't we risk guests trying to work around incorrect ACPI tables.
> Let us however do this in a patch by its own with proper
> documentation and motivation.
>
So if I understand how to do this now - I should drop the code for the 
MicroVM ACPI for now, letting only to change the Q35 FACP table, right? 
So if that's the case I should send it in a separate patch?

If that's the case, as suggested by you and Ani, I'll not add a separate 
function to reduce code duplication as there's no such thing in such case...