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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20130306085628.GB4719@localhost.localdomain>
Date:	Wed, 6 Mar 2013 16:56:28 +0800
From:	Hu Tao <hutao@...fujitsu.com>
To:	Paolo Bonzini <pbonzini@...hat.com>
Cc:	Gleb Natapov <gleb@...hat.com>, kvm list <kvm@...r.kernel.org>,
	qemu-devel <qemu-devel@...gnu.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Daniel P. Berrange" <berrange@...hat.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Jan Kiszka <jan.kiszka@...mens.com>,
	Blue Swirl <blauwirbel@...il.com>,
	Eric Blake <eblake@...hat.com>,
	Andrew Jones <drjones@...hat.com>,
	Marcelo Tosatti <mtosatti@...hat.com>,
	Sasha Levin <levinsasha928@...il.com>,
	Luiz Capitulino <lcapitulino@...hat.com>,
	Anthony Liguori <aliguori@...ibm.com>,
	Markus Armbruster <armbru@...hat.com>,
	Stefan Hajnoczi <stefanha@...hat.com>,
	Juan Quintela <quintela@...hat.com>,
	Orit Wasserman <owasserm@...hat.com>,
	Kevin Wolf <kwolf@...hat.com>,
	Wen Congyang <wency@...fujitsu.com>,
	"Michael S. Tsirkin" <mst@...hat.com>,
	Alexander Graf <agraf@...e.de>,
	Alex Williamson <alex.williamson@...hat.com>,
	Peter Maydell <peter.maydell@...aro.org>
Subject: Re: [PATCH v13 0/8] pv event interface between host and guest

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/