Re: [PATCH 0/4] Introduce a battery, AC adapter, and lid button

[Michael S. Tsirkin]

On Thu, Jan 21, 2021 at 07:38:46AM +0200, Leonid Bloch wrote:
> Hi Phil,
> 
> Thanks for your feedback! Please see below.
> 
> On Wed, Jan 20, 2021 at 11:52 PM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> wrote:
> >
> > Hi Leonid, Marcel,
> >
> > On 1/20/21 9:54 PM, Leonid Bloch wrote:
> > > This series introduces the following ACPI devices:
> > >
> > > * Battery
> > > * AC adapter
> > > * Laptop lid button
> > >
> > > When running QEMU on a laptop, these paravirtualized devices reflect the
> > > state of these physical devices onto the guest. This functionality is
> > > relevant not only for laptops, but also for any other device which has 
> > > e.g.
> > > a battery. This even allows to insert a ``fake'' battery to the
> > > guest, in a form of a file which emulates the behavior of the actual
> > > battery in sysfs. A possible use case for such a ``fake'' battery can be
> > > limiting the budget of VM usage to a subscriber, in a naturally-visible 
> > > way.
> >
> > Your series looks good. Now for this feature to be even more useful for
> > the community, it would be better to
> >
> > 1/ Have a generic (kind of abstract QDev) battery model.
> >    Your model would be the ISA implementation. But we could add LPC,
> >    SPI or I2C implementations for example.
> 
> It definitely feels that it needs to be more generic, and I thought
> how to make it so, but so far it is what I came up with. I'll think
> some more, but any ideas are welcome, cause nowadays I'm doing this in
> my free time only.
> 
> > 2/ Make it a model backend accepting various kind of frontends:
> >    - host Linux sysfs mirroring is a particular frontend implementation
> >    - mirroring on Windows would be another
> >    - any connection (TCP) to battery simulator (Octave, ...)
> 
> Well, it does accept an arbitrary file to represent a battery, so this
> covers the battery simulator, does it? As for Windows - indeed, it
> would be nice to have.

Poking at sysfs from QEMU poses a bunch of issues, for example,
security, migration, etc. Running timers on the host is also not nice
since it causes exits from VM ...

So I agree, as a starting point let's just let user
control the battery level through QMP.



> > Meanwhile 2/ is not available, it would be useful to have QMP commands
> > to set the battery charge and state (also max capacity).
> 
> But the battery state is determined by the physical battery, or by an
> externally provided file. Do you mean introducing another source for
> battery information which will be controlled by QMP commands?
> As for the max capacity, as with an actual battery, the "QEMU battery"
> has it set "by the manufacturer". It is not passed through from the
> host, for simplicity sake, and only the percentage is passed. How will
> it help if we allow to set the max capacity? It's something pretty
> much transparent to the user. (But if there is a use case, of course
> it can be done.)
> 
> > Ditto QMP command to set the LID/AC adapter state.
> >
> > > But of course, the main purpose here is addressing the desktop users.
> > >
> > > This series was tested with Windows and (desktop) Linux guests, on which
> > > indeed the battery icon appears in the corresponding state (full,
> > > charging, discharging, time remaining to empty, etc.) and the AC adapter
> > > plugging/unplugging behaves as expected. So is the laptop lid button.
> > [...]
> >
> > In patch #2 you comment 'if a "fake" host battery is to be provided,
> > a 'sysfs_path' property allows to override the default one.'.
> >
> > Eventually you'd provide a such fake file as example, ideally used
> > by a QTest.
> 
> Sure! I will - it's definitely a good idea.
> 
> > Another question. If the battery is disconnected, is there an event
> > propagated to the guest?
> 
> No. I definitely need to add! Thanks!
> 
> > Thanks for contributing these patches :)
> 
> Thank you!
> Leonid.
> 
> > Phil.