From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44905) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UGAZ1-0005Cv-LQ for qemu-devel@nongnu.org; Thu, 14 Mar 2013 11:59:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UGAYu-0000nn-Np for qemu-devel@nongnu.org; Thu, 14 Mar 2013 11:59:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:5589) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UGAYu-0000nS-Fr for qemu-devel@nongnu.org; Thu, 14 Mar 2013 11:59:24 -0400 Date: Thu, 14 Mar 2013 17:59:10 +0200 From: Gleb Natapov Message-ID: <20130314155910.GB11223@redhat.com> References: <5d85837fd58683b7e13ac78d6d468af6e152a512.1363243596.git.hutao@cn.fujitsu.com> <514194E4.3040208@redhat.com> <20130314091919.GD11223@redhat.com> <51419BCF.8030003@redhat.com> <20130314123440.GP11223@redhat.com> <5141D57C.7000303@redhat.com> <20130314135658.GA13274@redhat.com> <5141D922.1080508@redhat.com> <20130314142353.GU11223@redhat.com> <5141F1D0.50509@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5141F1D0.50509@redhat.com> Subject: Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Peter Maydell , "Michael S. Tsirkin" , Hu Tao , qemu-devel , Markus Armbruster , Blue Swirl , Orit Wasserman , Juan Quintela , Alexander Graf , Christian Borntraeger , Jan Kiszka , Andrew Jones , Alex Williamson , Sasha Levin , Stefan Hajnoczi , Luiz Capitulino , KAMEZAWA Hiroyuki , Kevin Wolf , Anthony Liguori , Marcelo Tosatti On Thu, Mar 14, 2013 at 04:50:40PM +0100, Paolo Bonzini wrote: > 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). > We are not there yet :) > 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. > Yes, if you do not allow changing IO port patching _STA is enough, but if you already patching it is easy to patch both. > >>>> I think it's a nice compromise. > > ^^^ This still holds. :) If we would have found a reasonable way to go without patching at all then it would have been worthwhile to consider compromises, but if patching is inevitable I honestly do not see big difference between patching one place or two. > > >>>>>> * 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. > Fine with me. That was just a suggestion. I thought we had singleton qdev flag. -- Gleb.