Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Paolo Bonzini]

Il 14/03/2013 15:23, Gleb Natapov ha scritto:
> On Thu, Mar 14, 2013 at 03:05:22PM +0100, Paolo Bonzini wrote:
>> Il 14/03/2013 14:56, Gleb Natapov ha scritto:
>>> On Thu, Mar 14, 2013 at 02:49:48PM +0100, Paolo Bonzini wrote:
>>>> Il 14/03/2013 13:34, Gleb Natapov ha scritto:
>>>>>> * it can be an ISA device; the interface is the I/O port and ACPI
>>>>>> support is provided just for convenience of the OSPM.  In this case,
>>>>>> "-device pvevent" should just add handlers for the port.  The ACPI
>>>>>> support is similar to what we do for other on-board ISA devices, for
>>>>>> example serial ports (the serial ports use PIIX PCI configuration
>>>>>> instead of fw-cfg, but that's a minor detail).  It only needs to work
>>>>>> for port 0x505, so the fw-cfg data can be a single yes/no value and only
>>>>>> the _STA method needs patching.  See piix4_pm_machine_ready in
>>>>>> hw/acpi_piix4.c.
>>>>>
>>>>> Again I think there is a big difference between well knows device and
>>>>> PV devices that we add at random location. And if we make the later
>>>>> configurable i.e it may or may not be present and location where it is
>>>>> present can be changed then we better not make a guest to do guesses.
>>>>
>>>> No guesses here on part of the guest, and no probing in the firmware
>>>> two.  The same number is hard-coded in QEMU and the DSDT, which go in
>>>> pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
>>>> trick with the methods).
>>>
>>> That's the problem. The number is not hard coded in QEMU only DSDT.
>>
>> It is hard-coded where the board creates it, or at least as the default
>> value of the qdev property.
>
> Default value that can be changes is not hard coded.
> Why do you allow change in one place, but not the other?

I'm just following the model of other ISA devices, I don't think there's
any difference in this respect between well-known and pv devices (also
because in the end all modern guests will use ACPI to discover even
well-known devices).

The board hardcodes 0x505 for pvpanic just like it hardcodes 0x3f8 for
serial ports.

>>> If you hard code it in QEMU (make it non configurable) and make device 
>>> mandatory
>>> static DSDT make sense if provided by QEMU.
>>
>> You cannot make it mandatory due to versioned machine types, but my plan
>> would be to make it mandatory on "pc" and "pc-1.5".  For that plan it
>> makes sense to have a static DSDT.  Sorry if it was unclear.
> 
> And then you will have to have different DSDT for pre pc-1.5. Dynamic
> patching solves exactly that problem.

Yes, but it's enough to patch _STA.  Easier in both QEMU and the BIOS.

>>>> I think it's a nice compromise.

^^^ This still holds. :)

>>>>>> * ACPI support is a first-class part of the device.  Each instance of
>>>>>> the device should be there in the ACPI tables.  In this case the fw-cfg
>>>>>> data needs to be a list of ports, and it is probably simpler to combine
>>>>>> all the definitions in an SSDT that is dynamically-built (similar to
>>>>>> what we do for PCI hotplug slots).  Or even provide a separate SSDT for
>>>>>> each instance of the device.
>>>>>>
>>>>>> I prefer the first, the second seems to be over-engineered.
>>>>>>
>>>>> Second is over-engineering indeed. The device should be singleton and
>>>>> fail if second instance is created. Do we have such capability in qdev?
>>>>
>>>> No, but why should it fail?
>>>>
>>> Why should it not? Guest cannot use more than on of them, why allow to
>>> create insane configs?
>>
>> Who cares?  Insane ISA device configs anyway are not discoverable by
>> guests, you need to teach the guest about the device manually.
>>
> With proper ACPI they are discoverable. Since writing ACPI support for
> multiple pvpanic devices is clear case of over-engineering it is a
> courtesy to QEMU users to fail machine creation that cannot be properly
> described by ACPI.

We don't fail machine creation if someone wants to place a serial port
at 0x5678.  With ISA it's basically garbage-in, garbage-out, I don't see
a reason to make pvpanic special in this respect.

Paolo