Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751445AbdFFNtz (ORCPT <rfc822;w@1wt.eu>);
        Tue, 6 Jun 2017 09:49:55 -0400
Received: from esa2.dell-outbound.iphmx.com ([68.232.149.220]:26443 "EHLO
        esa2.dell-outbound.iphmx.com" rhost-flags-OK-OK-OK-OK)
        by vger.kernel.org with ESMTP id S1751399AbdFFNtx (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 6 Jun 2017 09:49:53 -0400
From: <Mario.Limonciello@dell.com>
X-LoopCount0: from 10.175.216.249
X-IronPort-AV: E=Sophos;i="5.39,306,1493701200"; 
   d="scan'208";a="951093449"
To: <pali.rohar@gmail.com>, <luto@amacapital.net>
CC: <dvhart@infradead.org>, <platform-driver-x86@vger.kernel.org>,
        <andriy.shevchenko@linux.intel.com>, <luto@kernel.org>,
        <rjw@rjwysocki.net>, <linux-kernel@vger.kernel.org>,
        <linux-acpi@vger.kernel.org>
Subject: RE: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose
 embedded WMI MOF metadata
Thread-Topic: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose
 embedded WMI MOF metadata
Thread-Index: AQHS3kk7xMJsawcrI0WF/5NgAk77dKIXKtQAgAAFrACAANB6AP//0/SQ
Date: Tue, 6 Jun 2017 13:46:16 +0000
Message-ID: <8aa135f430b441eb82614b22d662d1ff@ausx13mpc120.AMER.DELL.COM>
References: <cover.1495862272.git.dvhart@infradead.org>
 <201705271314.16241@pali> <20170605221456.GA27270@fury>
 <201706060019.26462@pali>
 <CALCETrXTejMgbQZaHOKuEM_rF4bMduEzwWZ6+7R2M+gbFo5LMg@mail.gmail.com>
 <20170606110554.GB4690@pali>
In-Reply-To: <20170606110554.GB4690@pali>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
x-ms-exchange-transport-fromentityheader: Hosted
x-originating-ip: [10.143.18.86]
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v56Do6lh016129
Content-Length: 6438
Lines: 158

Pali,

Amazing job with what you've done so far.  A few comments I wanted to share from my taking look at your bmf2mof and comparing to "source" MOF.
1) At least in source the case used for String and Boolean is lower case.  I'm unsure if that actually matters for any MOF parsing tools, but I wanted to FYI in case it does.
2) On my system when you expand the arguments for "void DoBFn" the source doesn't describe individual arguments like you do.  
Again this might not matter to MOF parsing tools but wanted to let you know in case it does.

source:
	void DoBFn([in, out, Description("Fn buf")] BDat Data);
bmf2mof:
	void doBFn([in, Description("Fn buf"), ID(0)] BDat Data, [out, Description("Fn buf"), ID(0)] BDat Data);

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Tuesday, June 6, 2017 6:06 AM
> To: Andy Lutomirski <luto@amacapital.net>
> Cc: Darren Hart <dvhart@infradead.org>; platform-driver-x86@vger.kernel.org;
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Andy Lutomirski
> <luto@kernel.org>; Limonciello, Mario <Mario_Limonciello@Dell.com>; Rafael
> Wysocki <rjw@rjwysocki.net>; linux-kernel@vger.kernel.org; Linux ACPI <linux-
> acpi@vger.kernel.org>
> Subject: Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose
> embedded WMI MOF metadata
> 
> On Monday 05 June 2017 15:39:44 Andy Lutomirski wrote:
> > On Mon, Jun 5, 2017 at 3:19 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > > On Tuesday 06 June 2017 00:14:56 Darren Hart wrote:
> > >> On Sat, May 27, 2017 at 01:14:15PM +0200, Pali Rohár wrote:
> > >> > > metadata. I think that Samba has tools to interpret it, but there
> > >> > > is currently no interface to get the data in the first place.
> > >> >
> > >> > No, there is no non-ms-windows tool for interpreting those binary
> > >> > MOF (BMF) data yet.
> > >>
> > >> Good point. Updated.
> > >
> > > You are too late :-) Now there is my at https://github.com/pali/bmfdec
> > > See my email "Binary MOF buffer in WMI is finally decoded!".
> > >
> >
> > It comes out like this on my laptop.  I don't know enough about MOF to
> > know what we're supposed to do with this, but I suspect it at least
> > gives us the sizes of buffers that we should be passing to the various
> > methods.
> 

Yes, the size of the buffers and the format of the data you can pass in them is 
what is most important bit of information that comes out of this.

> There are two tools bmfparse and bmf2mof. Difference is just output
> format.
> 
> Important are WmiDataId and WmiMethodId qualifiers. Those define ids are
> what are passed to kernel function wmi_evaluate_method(). Ids are
> processed by corresponding WMxx ACPI function.
> 
> So instead of
> 
>   wmi_evaluate_method(guid, instance, method_id, acpi_in, acpi_out);
> 
> it should be possible to call:
> 
>   wmi_evaluate_method_name(class, name, input_params, output_params);
> 
> (once somebody implement wmi_evaluate_method_name function)
> 
Now that you have managed to decode binary MOF, it might actually change
what makes sense to export to userspace.  We were previously
discussing exporting GUID's (eg  something like /dev/wmi-$GUID)
but with readable names that actually map to the GUID's, it makes more sense
to export the classes.  Userspace tools can parse the exported MOF to know
how to interact with those classes, and the classes can then map to chardevices.
For example on a Dell system today /dev/wmi-BFn would be the most important
to export (this will change in the future).

> It is more readable in code to use class and function names instead of
> some guids and random meaningless numbers. Also it would allow to check
> size of input buffer (or types).
Completely agree.  Once the parser can be brought into kernel, this is a very
good next change for existing WMI drivers.

> 
> E.g.
> 
>   BDat data_in;
>   BDat data_out;
>   // fill data_in.Bytes
>   wmi_evaluate_method_name("BFn", "DoBFn", &data_in, &data_out);
>   // output is in data_out.Bytes
> 
> could be translated to:
> 
>   wmi_evaluate_method("A80593CE-A997-11DA-B012-B622A1EF5492", 0, 1,
> acpi_in, acpi_out);
> 
> Sometimes method_id is random number and hard to guess it. One of
> possible solution is to trace ACPI calls on Windows, another is decode
> that BMOF buffer and take correct method_id.
> 
> I will probably write another one tool which prints just important WMI
> functions and their mapping to WMI ids + input/output parameters.
> Without all other MOF data which are not relevant to ACPI-WMI.

When this goes into the kernel, I think it would be ideal to export MOF
exactly like your bmf2mof does.
The userspace MOF parsing tools that already exist will be able to process 
it more effectively this way.
Perhaps internally in the kernel this mapping information will be useful to 
be able to  create wmi_evaluate_method_name.

> 
> > class WMIEvent : __ExtrinsicEvent {
> > };
> >
> > [WMI, Locale("MS\0x409"), Description("QDATA"),
> > guid("{ABBC0F60-8EA1-11d1-00A0-C90629100000}")]
> > class QDat {
> >   [WmiDataId(1), read, write, Description("qdata")] uint8 Bytes[128];
> > };
> >
> > [WMI, Dynamic, Provider("WmiProv"), Locale("MS\0x409"),
> > Description("BIOS WMI Query"),
> > guid("{8D9DDCBC-A997-11DA-B012-B622A1EF5492}")]
> > class WMI_Query {
> >   [key, read] String InstanceName;
> >   [read] Boolean Active;
> >   [WmiDataId(1), read, write, Description("BIOS WMI info")] QDat QDATA;
> > };
> >
> > [WMI, Locale("MS\0x409"), Description("Data"),
> > guid("{a3776ce0-1e88-11db-a98b-0800200c9a66}")]
> > class BDat {
> >   [WmiDataId(1), read, write, Description("data")] uint8 Bytes[4096];
> > };
> >
> > [WMI, Dynamic, Provider("WmiProv"), Locale("MS\0x409"),
> > Description("Interface"),
> > guid("{A80593CE-A997-11DA-B012-B622A1EF5492}")]
> > class BFn {
> >   [key, read] String InstanceName;
> >   [read] Boolean Active;
> >
> >   [WmiMethodId(1), Implemented, read, write, Description("Do BFn")]
> > void DoBFn([in, Description("Fn buf"), ID(0)] BDat Data, [out,
> > Description("Fn buf"), ID(0)] BDat Data);
> > };
> >
> > [WMI, Dynamic, Provider("WmiProv"), Locale("MS\0x409"),
> > Description("Event"), guid("{9DBB5994-A997-11DA-B012-B622A1EF5492}")]
> > class BIOSEvent : WmiEvent {
> >   [key, read] String InstanceName;
> >   [read] Boolean Active;
> >   [WmiDataId(1), read, write, Description("Ev buf")] QDat Data;
> > };
> 
> --
> Pali Rohár
> pali.rohar@gmail.com