Re: [PATCH v13 0/8] pv event interface between host and guest

[Hu Tao <hutao@cn.fujitsu.com>]

Hi,

On Mon, Mar 04, 2013 at 11:05:37AM +0100, Paolo Bonzini wrote:
> Il 03/03/2013 10:17, Gleb Natapov ha scritto:
> > On Thu, Feb 28, 2013 at 08:13:10PM +0800, Hu Tao wrote:
> >> This series implements a new interface, kvm pv event, to notify host when
> >> some events happen in guest. Right now there is one supported event: guest
> >> panic.
> >>
> > What other event do you have in mind? Is interface generic enough to
> > accommodate future, yet unknown, events. It allows to pass only one
> > integer specifying even type, what if additional info is needed? My be
> > stop pretending that device is generic and make it do once thing but do
> > it well? For generic even passing interface (whatever it may be needed
> > for) much more powerful virtio should be used.
> > 
> > On implementation itself I do not understand why is this kvm specific.
> > The only thing that makes it so is that you hook device initialization
> > into guest kvm initialization code, but this is obviously incorrect.
> > What stops QEMU tcg or Xen from reusing the same device for the same
> > purpose except the artificial limitation in a guest.
> 
> Agreed.
> 
> > Reading data from a random ioports is not how you discover platform
> > devices in 21 century (and the data you read from unassigned port is not
> > guarantied to be zero, it may depend on QEMU version), you use ACPI for
> > that and Marcelo already pointed that to you. Having little knowledge of
> > ACPI (we all do) is not a good reason to not doing it. We probably need
> > to reserve QEMU specific ACPI Plug and Play hardware ID to define our own
> > devices. After that you will be able to create device with _HID(QEMU0001)
> > in DSDT that supplies address information (ioport to use) and capability
> > supported.
> 
> Please also document this HID in a new file in the QEMU docs/ directory.
> 
> > Guest uses acpi_get_devices() to discover a platform device by
> > its name (QEMU0001).  Then you put the driver for the platform device
> > into drivers/platform/x86/ and QEMU/kvm/Xen all will be able to use it.
> 
> Just to clarify it for Hu Tao, the read from a random ioport is how the
> ACPI code will detect presence of the device.
> 
> Something like this should work (in SeaBIOS's src/acpi-dsdt-isa.dsl):
> 
>     Device(PEVT) {
>         Name(_HID, EisaId("QEMU0001"))
>         OperationRegion(PEOR, SystemIO, 0x505, 0x01)
>         Field(PEOR, ByteAcc, NoLock, Preserve) {
>             PEPT,   8,
>         }
> 
>         Method(_STA, 0, NotSerialized) {
>             Store(PEPT, Local0)
>             If (LEqual(Local0, Zero)) {
>                 Return (0x00)
>             } Else {
>                 Return (0x0F)
>             }
>         }

IIUC, here _STA reads from ioport 0x505, if the result is 0, then the
device is not present. Otherwise, the device is present. But as Gleb
said, ''the data you read from unassigned port is not guarantied to be
zero, it may depend on QEMU version''. What should I do to tell if the
device is present or not correctly?

> 
>         Name(_CRS, ResourceTemplate() {
>             IO(Decode16, 0x505, 0x505, 0x01, 0x01)
>         })
>     }
> 
> Please test this with a QEMU option like "-M pc-1.4".  The device should
> _not_ be detected if you're doing it right.

because there is no corresponding device driver?

> 
> > On QEMU side of things I cannot comment much on how QOMified the device
> > is (it should be),
> 
> Please make the device target-independent.  It can be used on non-x86
> architectures that have I/O ports.  You should make the port
> configurable using a property (DEFINE_PROPERTY_INT16 or something like
> that), with a default of 0x505.

OK.

> 
> All the panicked_action is not necessary in my opinion.  We have it for
> watchdogs, but that's really a legacy thing.  Let libvirt do it, and
> always make the guest panic perform the PANICKED_PAUSE action.

OK.

> 
> If you do it properly, a lot (really a lot) of code will go away.

Thanks!

> 
> > I hope other reviews will verify it, but I noticed
> > that device is only initialized for PIIX, what about Q35?
> 
> Yup.
> 
> Paolo