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

[Gleb Natapov]

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?

> > 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.

> 
> >> I think it's a nice compromise.
> >>
> >>>> * 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.

--
                        Gleb.