Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210623141033.27475-1-emmanuel.grumbach@intel.com> <87bl7vi3o1.fsf@codeaurora.org>
In-Reply-To: <87bl7vi3o1.fsf@codeaurora.org>
From:   Emmanuel Grumbach <egrumbach@gmail.com>
Date:   Thu, 24 Jun 2021 23:04:56 +0300
Message-ID: <CANUX_P3QE9xNnQLT=mHNDp4VCv37Bcjuvn9O1wQ4Btejjkbrvg@mail.gmail.com>
Subject: Re: [PATCH v3 1/4] iwlwifi: mei: add the driver to allow cooperation
 with CSME
To:     Kalle Valo <kvalo@codeaurora.org>
Cc:     Emmanuel Grumbach <emmanuel.grumbach@intel.com>,
        "Coelho, Luciano" <luciano.coelho@intel.com>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        Ayala Beker <ayala.beker@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jun 24, 2021 at 8:18 PM Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Emmanuel Grumbach <emmanuel.grumbach@intel.com> writes:
>
> > CSME in two words
> > -----------------
> > CSME stands for Converged Security and Management Engine. It is
> > a CPU on the chipset and runs a dedicated firmware.
> > AMT (Active Management Technology) is one of the applications
> > that run on that CPU. AMT allows to control the platform remotely.
> > Here is a partial list of the use cases:
> > * View the screen of the plaform, with keyboard and mouse (KVM)
> > * Attach a remote IDE device
> > * Have a serial console to the device
> > * Query the state of the platform
> > * Reset / shut down / boot the platform
> >
> > Networking in CSME
> > ------------------
> > For those uses cases, CSME's firmware has an embedded network
> > stack and is able to use the network devices of the system: LAN
> > and WLAN. This is thanks to the CSME's firmware WLAN driver.
> >
> > One can add a profile (SSID / key / certificate) to the CSME's OS
> > and CSME will connect to that profile. Then, one can use the WLAN
> > link to access the applications that run on CSME (AMT is one of
> > them). Note that CSME is active during power state and power state
> > transitions. For example, it is possible to have a KVM session
> > open to the system while the system is rebooting and actually
> > configure the BIOS remotely over WLAN thanks to AMT.
> >
> > How all this is related to Linux
> > --------------------------------
> > In Linux, there is a driver that allows the OS to talk to the
> > CSME firmware, this driver is drivers/misc/mei. This driver
> > advertises a bus that allows other kernel drivers or even user
> > space) to talk to components inside the CSME firmware.
> > In practice, the system advertises a PCI device that allows
> > to send / receive data to / from the CSME firmware. The mei
> > bus drivers in drivers/misc/mei is an abstration on top of
> > this PCI device.
> > The driver being added here is called iwlmei and talks to the
> > WLAN driver inside the CSME firmware through the mei bus driver.
> > Note that the mei bus driver only gives bus services, it doesn't
> > define the content of the communication.
> >
> > Why do we need this driver?
> > --------------------------
> > CSME uses the same WLAN device that the OS is expecting to see
> > hence we need an arbitration mechanism. This is what iwlmei is
> > in charge of. iwlmei maintains the communication with the CSME
> > firmware's WLAN driver. The language / protocol that is used
> > between the CSME's firmware WLAN driver and iwlmei is OS agnostic
> > and is called SAP which stands for Software Abritration Protocol.
> > With SAP, iwlmei will be able to tell the CSME firmware's WLAN
> > driver:
> > 1) Please give me the device.
> > 2) Please note that the SW/HW rfkill state change.
> > 3) Please note that I am now associated to X.
> > 4) Please note that I received this packet.
> > etc...
> >
> > There are messages that go the opposite direction as well:
> > 1) Please note that AMT is en/disable.
> > 2) Please note that I believe the OS is broken and hence I'll take
> >    the device *now*, whether you like it or not, to make sure that
> >    connectivity is preserved.
> > 3) Please note that I am willing to give the device if the OS
> >    needs it.
> > 4) Please give me any packet that is sent on UDP / TCP on IP address
> >    XX.XX.XX.XX and an port ZZ.
> > 5) Please send this packet.
> > etc...
> >
> > Please check drivers/net/wireless/intel/iwlwifi/mei/sap.h for the
> > full protocol specification.
> >
> > Arbitration is not the only purpose of iwlmei and SAP. SAP also
> > allows to maintain the AMT's functionality even when the OS owns
> > the device. To connect to AMT, one needs to initiate an HTTP
> > connection to port 16992. iwlmei will listen to the Rx path and
> > forward (through SAP) to the CSME firmware the data it got. Then,
> > the embedded HTTP server in the chipset will reply to the request
> > and send a SAP notification to ask iwlmei to send the reply.
> > This way, AMT running on the CSME can still work.
> >
> > In practice this means that all the use cases quoted above (KVM,
> > remote IDE device, etc...) will work even when the OS uses the
> > WLAN device.
> >
> > How to disable all this?
> > ---------------------------
> > iwlmei won't be able to do anything if the CSME's networking stack
> > is not enabled. By default, CSME's networking stack is disabled (this
> > is a BIOS setting).
> > In case the CSME's networking stack is disabled, iwlwifi will just
> > get access to the device because there is no contention with any other
> > actor and, hence, no arbitration is needed.
> >
> > In this patch, I only add the iwlmei driver. Integration with
> > iwlwifi will be implemented in the next one.
> >
> > Co-Developed-by: Ayala Beker <ayala.beker@intel.com>
> > Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > ---
> > v2: fix a few warnings raised by the different bots
> > v3: rewrite the commit message
>
> Thank you for the great commit log, now I understand what this is about.
> I'm not able to do thorough review but my first quick comments.

Glad it helped.

>
>
> [...]
>
> > +     BUILD_BUG_ON((u32)IWL_MEI_AKM_AUTH_OPEN !=
> > +                  (u32)SAP_WIFI_AUTH_TYPE_OPEN);
> > +     BUILD_BUG_ON((u32)IWL_MEI_AKM_AUTH_RSNA !=
> > +                  (u32)SAP_WIFI_AUTH_TYPE_RSNA);
> > +     BUILD_BUG_ON((u32)IWL_MEI_AKM_AUTH_RSNA_PSK !=
> > +                  (u32)SAP_WIFI_AUTH_TYPE_RSNA_PSK);
> > +     BUILD_BUG_ON((u32)IWL_MEI_AKM_AUTH_SAE !=
> > +                  (u32)SAP_WIFI_AUTH_TYPE_SAE);
> > +
> > +     BUILD_BUG_ON((u32)IWL_MEI_CIPHER_NONE !=
> > +                  (u32)SAP_WIFI_CIPHER_ALG_NONE);
> > +     BUILD_BUG_ON((u32)IWL_MEI_CIPHER_CCMP !=
> > +                  (u32)SAP_WIFI_CIPHER_ALG_CCMP);
> > +     BUILD_BUG_ON((u32)IWL_MEI_CIPHER_GCMP !=
> > +                  (u32)SAP_WIFI_CIPHER_ALG_GCMP);
> > +     BUILD_BUG_ON((u32)IWL_MEI_CIPHER_GCMP_256 !=
> > +                  (u32)SAP_WIFI_CIPHER_ALG_GCMP_256);
>
> These look just weird, and suspicious. You are using two different enums
> but they have to be same values, or what?

Exactly. I don't want the userspace to have to include all the SAP
protocol header file. OTOH, I don't want to have to translate between
vendor commands attributes values and the SAP values. So I want them
to be exactly the same. Note that the SAP values are not contiguous:
enum iwl_sap_wifi_cipher_alg {
        SAP_WIFI_CIPHER_ALG_NONE        = 0,
        SAP_WIFI_CIPHER_ALG_CCMP        = 4,
        SAP_WIFI_CIPHER_ALG_GCMP        = 8,
        SAP_WIFI_CIPHER_ALG_GCMP_256    = 9,
};

Hint, hint, there are ciphers that we don't want to support, like...
WEP, TKIP and alike. There are also values here that are really
surprising, but I didn't write the API... It is the CSME firmware's.
Making sure that the vendor commands attributes values and the SAP
values are the same here allows me to externalize only the vendor
command attribute enumeratoin to the userspace and be safe that we
won't need to translate any value before sending those through SAP.

>
> > +static void iwl_mei_dbgfs_register(struct iwl_mei *mei)
> > +{
> > +     mei->dbgfs_dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
> > +
> > +     if (!mei->dbgfs_dir)
> > +             return;
> > +
> > +     init_waitqueue_head(&mei->debugfs_wq);
> > +
> > +     debugfs_create_ulong("status", S_IRUSR,
> > +                          mei->dbgfs_dir, &iwl_mei_status);
> > +     debugfs_create_file("send_start_message", S_IWUSR, mei->dbgfs_dir,
> > +                         mei, &iwl_mei_dbgfs_send_start_message_ops);
> > +     debugfs_create_file("req_ownserhip", S_IWUSR, mei->dbgfs_dir,
> > +                         mei, &iwl_mei_dbgfs_req_ownership_ops);
> > +}
>
> The commit log mentions nothing about debugfs interface. I recommend
> moving them to a separate patch for easier review and explaining what
> these do.

Doable I guess.
Next week.