RE: [RFC PATCH 10/10] acpi: add support for loading SSDTs via configfs

[Zheng, Lv]

Hi,

> From: Octavian Purdila [mailto:octavian.purdila@xxxxxxxxx]
> Subject: Re: [RFC PATCH 10/10] acpi: add support for loading SSDTs via configfs
> 
> On Tue, Apr 5, 2016 at 6:11 AM, Zheng, Lv <lv.zheng@xxxxxxxxx> wrote:
> > Hi,
> >
> >> From: Octavian Purdila [mailto:octavian.purdila@xxxxxxxxx]
> >> Subject: Re: [RFC PATCH 10/10] acpi: add support for loading SSDTs via
> configfs
> >>
> >> On Fri, Apr 1, 2016 at 7:55 AM, Zheng, Lv <lv.zheng@xxxxxxxxx> wrote:
> >> > Hi,
> >>
> >> Hi Lv,
> >>
> >> >> Add support for acpi_user_table configfs items that allows the user to
> >> >> load new tables. The data attributes contains the table data and once it
> >> >> is filled from userspace the table is loaded and ACPI devices are
> >> >> enumerated.
> >> > [Lv Zheng]
> >> > We've been considering to implement this facility before.
> >> > The 2 alternative solutions are:
> >> > 1. implement LOAD/UNLOAD ioctl for /sys/kernel/debug/acpi/acpidbg -
> this
> >> will be useful for extracting AML byte stream from kernel to be used by a
> >> userspace disassembler.
> >>
> >> AFAIK adding new ioctls is discouraged.
> > [Lv Zheng]
> > Tools/power/acpi/tools/acpidbg is a file descriptor based utility.
> > And it needs a method to obtain an AML byte stream from kernel.
> > Using ioctl is a best fit design for acpidbg so that it needn't to access any
> other files.
> >
> >>
> >> > 2. transition /sys/firmware/acpi/tables/xxx into directory and implement
> >> /sys/firmware/acpi/tables/load, /sys/firmware/acpi/tables/unload - this
> should
> >> be able to meet your requirement.
> >>
> >> We can't do that as it would break the ABI.
> > [Lv Zheng]
> > The only user of this directory hierarchy is acpidump.
> 
> Some systems (e.g. Android, Brillo) do not ship acpidump and in this
> case the only way to dump tables is by accessing sysfs directly.
> 
> > And the user of this tool are all developers/reporters on the kernel bugzilla.
> > We've been asking the Bugzilla users to use the up-to-date acpidump instead
> of the distribution vendor provided one for so many years.
> > So IMO, this is not a serious problem you should consider.
> > You only need to think about an acceptable way for the distribution vendors
> to synchronize the kernel change and the acpidump change.
> >
> 
> I guess the perf model where you have a perf package build together
> with the kernel will work. But right now distributions are not using
> this model and a kernel change will break the userspace tool.
> 
> > IMO:
> > You may expose a version file from /sys/firmware/acpi.
> > acpidump can be changed accordingly by referencing the version file.
> > And old directory hierarchy support could be kept in acpidump.
> >
> 
> That will still break if you upgrade the kernel and use the old tool.
> 
> > Note that acpidump is also a part of the kernel, so your change could be
> consistent.
> > For example,
> > If you changed acpidump prior than making the kernel change, the
> distribution vendors might have already released the new acpidump for all old
> kernels before you transitioned the directory hierarchy.
> 
> To me this still looks like breaking userspace, patches have been
> reverted for less :) But I can see how this could be considered a gray
> area.
[Lv Zheng] 
For the "userspace breaking" concern.
IMO, we needn't worry about that too much.

> 
> ABI aside, see below for why configfs is better then sysfs.
> 
> >
> >>
> >> > So my first question is:
> >> > Why do you use configfs rather than the existing mechanisms?
> >>
> >> sysfs is not a good choice for dealing with objects created from
> >> userspace, configfs was created to address this specific need. Since
> >> we want to be able to create and load new tables from userspace this
> >> use-case fits very well with configfs.
> > [Lv Zheng]
> > Was the table binary stream still maintained by the userspace?
> > If not, I couldn't see the difference/advantages from using
> /sys/firmware/acpi/tables to using configfs.
> >
> 
> It is hard to create new kernel objects from sysfs. You need to resort
> to hacks like using new_table sysfs entries which does not map to a
> kernel object. Writes larger then PAGE_SIZE are impossible to handle
> with multiple open files because you have no open callback to create a
> file context. It is also not possible to do any clean-up because there
> is no close callback and if something goes wrong for example when
> trying to install the table you will leak the allocated memory.
> 
> configfs was designed for the specific purpose of creating kernel
> objects from userspace and addresses all of the limitations above (and
> some more).
> 
> Initially I started to implement this functionality via sysfs but I
> run into the issues mentioned above and decided to use configfs.
[Lv Zheng] 
I can sense different difficulties from your descriptions.
Let me break it down into details.

We already have acpi_table_handler working there for creating new ACPI table entries for us.
Based on this facility, let's think about the following solution:
1. sysfs presenting change
We can change the table file to a table directory whose name is in the following format:
TableSignature-OemId-OemTableId
Then we can get rid of the annoying numbered table name suffix first.
The numbered table name suffix cannot be kept consistent to reflect the real index if we allow tables to be dynamically loaded/unloaded.

This is the first design difficulty we need to solve.

2. acpi_table_handler change
Now we can append 2 new events to acpi_table_handler - ACPI_TABLE_INSTALL/ACPI_TABLE_UNINSTALL.
With which, the sysfs entries can be created/deleted when the table is added to/removed from the global table list.
And this should be the working mechanism for us
So we actually don't have the trouble to deal with the new kernel object creation/deletion from sysfs.

I agree the dynamic kernel object creation/deletion need special care.
But this actually is what a kernel engineer should do because this kind of things happen here and there throughout the kernel.
We should have already been used to that.

This is the second engineering difficulty we need to face.

3. load/unload commanding
Now we need a character device in sysfs to handle load/unload command.
Well, there are many such kind of files in sysfs, for example, device nodes.
So this is not a non-achievable task, but just a difficult engineering task.
The system engineers need to be skillful enough to implement this.
Like the dynamic kernel object handling, we should have already been used to this.

If you still think this is difficult, the alternative choice is to use acpidbg char device's ioctl interface.
That could simplifies this task.
And since the ioctl interface is required by ACPICA disassembler, the work on that will be inherited by the disassembler porting.

This is the last engineering difficulty we need to face.

So why can't these solutions work for us?

Thanks and best regards
-Lv