Re: [PATCH 1/1] drm/ttm: Set memory as decrypted for ttm framebuffer mappings
On 08/24/2018 02:21 AM, Christian König wrote: > Am 24.08.2018 um 01:05 schrieb Jiandi An: >> >> >> On 08/23/2018 01:47 AM, Christian König wrote: >>> Am 22.08.2018 um 22:57 schrieb Jiandi An: >>>> >>>> On 08/22/2018 02:09 PM, Christian König wrote: >>>>> Am 22.08.2018 um 20:57 schrieb Jiandi An: >>>>>> Framebuffer memory needs to be accessed decrypted. Ensure the >>>>>> memory encryption mask is not set for the ttm framebuffer mappings. >>>>> NAK, the memory not only needs to be decrypted while CPU accessed but all >>>>> the time. >>>>> >>>>> ttm_page_alloc.c and ttm_page_alloc_dma.c should already take care of >>>>> that while mapping the pages. >>>>> >>>>> Regards, >>>>> Christian. >>>>> >>>> The path where the issue comes in is booting guest with AMD SEV enabled >>>> while using virtio-vga device. >>>> The ttm->pages doesn't go through ttm_populate_and_map_pages(). >>> >>> And that is the point where you need to jump in and fix the problem. >>> >>> When a driver implements the populate() and unpopulate() callbacks >>> themselves it needs to take care of all that handling. >> Thanks for the suggestion and guidance. So you want me to register the >> callbacks something like virtio_gpu_ttm_populate() and >> virtio_gpu_ttm_unpopulate() in the virtio_gpu_bo_driver, > > Yes, that is one possibility. The alternative is directly hack that into the > ttm_tt_populate() and ttm_tt_unpopulate() functions. > >> and perform mapping by calling ttm_bo_kmap()? Then bring setting memory >> decrypted/encryped by calling set_memory_decrypted()/set_memory_encrypted() >> outside of ttm_bo_kmap(), do it within virtio_gpu_ttm_populate() and >> virtio_gpu_ttm_unpopulate() callbacks? > > No, don't call ttm_bo_kmap() here. I think I now understand where we have a > misunderstanding. > > ttm_bo_kmap() just creates a vmap of the original pages to make them look > linear to the kernel space. > > What you need to do instead is to change the attributes of the original page > mapping, not the copy in the vmap. > > In other words you should call set_memory_decrypted() for each individually > allocated page. > >> >> Then get rid of the separate call of virtio_gpu_vmap_fb() the virtio_gpu >> driver does? >> >>> >>>> >>>> In the kernel path, the virtio_gpu driver calls virtio_gpu_alloc_object() >>>> and goes down to ttm to >>>> allocate pages in ttm_pool_populate(). Driver in guest then does >>>> virtio_gpu_vmap_fb() which goes >>>> down to ttm_bo_kmap_ttm() and does a vmap() for GVA to GPA for those >>>> pages. If SEV is enabled, >>>> decryption should be set in this GVA to GPA mapping. >>> >>> That assumption is what is incorrect here. >>> >>> TTM can't work with encrypted pages, so you need to set the pages as >>> decrypted directly after calling ttm_pool_populate(). >>> >>> And obviously set it to encrypted again before calling >>> ttm_pool_unpopulate(). >>> >>> Probably best if we add that to ttm_tt_populate()/ttm_tt_unpopulate(). >> >> Here when you say ttm_tt_populate() and ttm_tt_unpopulate() you mean the >> virtio_gpu_ttm_populate() and virtio_gpu_ttm_unpopulate() that I register in >> virtio_gpu_bo_driver right? > > Either as driver specific callbacks or directly in the fallback path of > ttm_tt_populate() and ttm_tt_unpopulate(). > After working and testing with Gerd Hoffmann's patches, getting virtio-gpu framebuffer pages to go through dma-api is more appropriate fix and is what we would like to do for SEV as well. Gerd Hoffmann's patches to add iommu support for virtio-gpu in QEMU and kernel together to allow iommu_platform attribute to be specified for virtio-gpu devices in qemu command line and the ttm pages for virtio-gpu framebuffers to go through dma-api. This solves the issue for SEV as SEV forces dma-api to go to swiotlb for bounce buffer and that's set as decrypted. Gerd Hoffmann QEMU patch https://git.kraxel.org/cgit/qemu/commit/?h=sirius/virtio-gpu-iommu&id=588ebd02e9daedd6e1c5453ef828c064a16e43c3 https://git.kraxel.org/cgit/qemu/commit/?h=sirius/virtio-gpu-iommu&id=86f9d30e4a44ca47f007e51ab5744f87e79fb83e Gerd Hoffmann Kernel patch https://lore.kernel.org/lkml/20180829122026.27012-2-kra...@redhat.com/ https://lore.kernel.org/lkml/20180829122026.27012-3-kra...@redhat.com/ Gerd Hoffmann's kernel patch is missing dma_sync, graphical screen turns black blank screen. Fix is the following two patches https://lore.kernel.org/lkml/20180919113406.ukhgbiukx7vsu...@sirius.home.kraxel.org/ https://lore.kernel.org/lkml/ffc08d4e-5da8-7631-438f-311fd0564...@amd.com/ - Jiandi > Regards, > Christian. >
Re: [PATCH] ipmi: Remove ACPI SPMI probing from the SSIF (I2C) driver
On 03/09/2018 08:42 PM, Corey Minyard wrote:
On 03/09/2018 04:28 PM, Jiandi An wrote:
On 03/08/2018 03:08 PM, miny...@acm.org wrote:
From: Corey Minyard
The IPMI spec states:
The purpose of the SPMI Table is to provide a mechanism that can
be used by the OSPM (an ACPI term for “OS Operating System-directed
configuration and Power Management” essentially meaning an ACPI-aware
OS or OS loader) very early in the boot process, e.g., before the
ability to execute ACPI control methods in the OS is available.
When we are probing IPMI in Linux, ACPI control methods are available,
so we shouldn't be probing using SPMI. It could cause some confusion
during the probing process.
Signed-off-by: Corey Minyard
Cc: Jiandi An
---
Jiandi, this just yanks out the SPMI code. Your platform
should have an ACPI control method in it's namespace specifying the
IPMI interface. If it doesn't, that's a bug in your platform and
really needs to be fixed. Finding IPMI devices with SMBIOS tables
is kind of risky since there is no real way to know which I2C bus
has the IPMI device if your system has more than one I2C bus. With
an ACPI control method, the IPMI control method will be inside the
I2C bus control method, so it will be unambiguous.
Thanks Corey. I tested this patch. It works for us through the ACPI
control method.
Thanks. May I add a "Tested-by" for you?
Tested-by: Jiandi An
By the way, FYI the ipmi_si driver is also having the
SPMI probe code.
[ 17.370835] ipmi_si: probing via SPMI
Yes, I've removed that in a separate patch.
-corey
Thanks
- Jiandi
drivers/char/ipmi/ipmi_ssif.c | 105
--
1 file changed, 105 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_ssif.c
b/drivers/char/ipmi/ipmi_ssif.c
index 4cff4cd..206257b 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -1994,108 +1994,6 @@ static const struct acpi_device_id
ssif_acpi_match[] = {
{ },
};
MODULE_DEVICE_TABLE(acpi, ssif_acpi_match);
-
-/*
- * Once we get an ACPI failure, we don't try any more, because we go
- * through the tables sequentially. Once we don't find a table, there
- * are no more.
- */
-static int acpi_failure;
-
-/*
- * Defined in the IPMI 2.0 spec.
- */
-struct SPMITable {
- s8 Signature[4];
- u32 Length;
- u8 Revision;
- u8 Checksum;
- s8 OEMID[6];
- s8 OEMTableID[8];
- s8 OEMRevision[4];
- s8 CreatorID[4];
- s8 CreatorRevision[4];
- u8 InterfaceType;
- u8 IPMIlegacy;
- s16 SpecificationRevision;
-
- /*
- * Bit 0 - SCI interrupt supported
- * Bit 1 - I/O APIC/SAPIC
- */
- u8 InterruptType;
-
- /*
- * If bit 0 of InterruptType is set, then this is the SCI
- * interrupt in the GPEx_STS register.
- */
- u8 GPE;
-
- s16 Reserved;
-
- /*
- * If bit 1 of InterruptType is set, then this is the I/O
- * APIC/SAPIC interrupt.
- */
- u32 GlobalSystemInterrupt;
-
- /* The actual register address. */
- struct acpi_generic_address addr;
-
- u8 UID[4];
-
- s8 spmi_id[1]; /* A '\0' terminated array starts here. */
-};
-
-static int try_init_spmi(struct SPMITable *spmi)
-{
- unsigned short myaddr;
-
- if (num_addrs >= MAX_SSIF_BMCS)
- return -1;
-
- if (spmi->IPMIlegacy != 1) {
- pr_warn("IPMI: Bad SPMI legacy: %d\n", spmi->IPMIlegacy);
- return -ENODEV;
- }
-
- if (spmi->InterfaceType != 4)
- return -ENODEV;
-
- if (spmi->addr.space_id != ACPI_ADR_SPACE_SMBUS) {
- pr_warn(PFX "Invalid ACPI SSIF I/O Address type: %d\n",
- spmi->addr.space_id);
- return -EIO;
- }
-
- myaddr = spmi->addr.address & 0x7f;
-
- return new_ssif_client(myaddr, NULL, 0, 0, SI_SPMI, NULL);
-}
-
-static void spmi_find_bmc(void)
-{
- acpi_status status;
- struct SPMITable *spmi;
- int i;
-
- if (acpi_disabled)
- return;
-
- if (acpi_failure)
- return;
-
- for (i = 0; ; i++) {
- status = acpi_get_table(ACPI_SIG_SPMI, i+1,
- (struct acpi_table_header **)&spmi);
- if (status != AE_OK)
- return;
-
- try_init_spmi(spmi);
- }
-}
-#else
-static void spmi_find_bmc(void) { }
#endif
#ifdef CONFIG_DMI
@@ -2200,9 +2098,6 @@ static int init_ipmi_ssif(void)
ssif_i2c_driver.driver.acpi_match_table =
ACPI_PTR(ssif_acpi_match);
- if (ssif_tryacpi)
- spmi_find_bmc();
-
if (ssif_trydmi) {
rv = platform_driver_register(&ipmi_driver);
if (rv)
--
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project.
Re: [PATCH] ipmi: Remove ACPI SPMI probing from the SSIF (I2C) driver
On 03/08/2018 03:08 PM, miny...@acm.org wrote:
From: Corey Minyard
The IPMI spec states:
The purpose of the SPMI Table is to provide a mechanism that can
be used by the OSPM (an ACPI term for “OS Operating System-directed
configuration and Power Management” essentially meaning an ACPI-aware
OS or OS loader) very early in the boot process, e.g., before the
ability to execute ACPI control methods in the OS is available.
When we are probing IPMI in Linux, ACPI control methods are available,
so we shouldn't be probing using SPMI. It could cause some confusion
during the probing process.
Signed-off-by: Corey Minyard
Cc: Jiandi An
---
Jiandi, this just yanks out the SPMI code. Your platform
should have an ACPI control method in it's namespace specifying the
IPMI interface. If it doesn't, that's a bug in your platform and
really needs to be fixed. Finding IPMI devices with SMBIOS tables
is kind of risky since there is no real way to know which I2C bus
has the IPMI device if your system has more than one I2C bus. With
an ACPI control method, the IPMI control method will be inside the
I2C bus control method, so it will be unambiguous.
Thanks Corey. I tested this patch. It works for us through the ACPI
control method. By the way, FYI the ipmi_si driver is also having the
SPMI probe code.
[ 17.370835] ipmi_si: probing via SPMI
Thanks
- Jiandi
drivers/char/ipmi/ipmi_ssif.c | 105 --
1 file changed, 105 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 4cff4cd..206257b 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -1994,108 +1994,6 @@ static const struct acpi_device_id ssif_acpi_match[] = {
{ },
};
MODULE_DEVICE_TABLE(acpi, ssif_acpi_match);
-
-/*
- * Once we get an ACPI failure, we don't try any more, because we go
- * through the tables sequentially. Once we don't find a table, there
- * are no more.
- */
-static int acpi_failure;
-
-/*
- * Defined in the IPMI 2.0 spec.
- */
-struct SPMITable {
- s8 Signature[4];
- u32 Length;
- u8 Revision;
- u8 Checksum;
- s8 OEMID[6];
- s8 OEMTableID[8];
- s8 OEMRevision[4];
- s8 CreatorID[4];
- s8 CreatorRevision[4];
- u8 InterfaceType;
- u8 IPMIlegacy;
- s16 SpecificationRevision;
-
- /*
-* Bit 0 - SCI interrupt supported
-* Bit 1 - I/O APIC/SAPIC
-*/
- u8 InterruptType;
-
- /*
-* If bit 0 of InterruptType is set, then this is the SCI
-* interrupt in the GPEx_STS register.
-*/
- u8 GPE;
-
- s16 Reserved;
-
- /*
-* If bit 1 of InterruptType is set, then this is the I/O
-* APIC/SAPIC interrupt.
-*/
- u32 GlobalSystemInterrupt;
-
- /* The actual register address. */
- struct acpi_generic_address addr;
-
- u8 UID[4];
-
- s8 spmi_id[1]; /* A '\0' terminated array starts here. */
-};
-
-static int try_init_spmi(struct SPMITable *spmi)
-{
- unsigned short myaddr;
-
- if (num_addrs >= MAX_SSIF_BMCS)
- return -1;
-
- if (spmi->IPMIlegacy != 1) {
- pr_warn("IPMI: Bad SPMI legacy: %d\n", spmi->IPMIlegacy);
- return -ENODEV;
- }
-
- if (spmi->InterfaceType != 4)
- return -ENODEV;
-
- if (spmi->addr.space_id != ACPI_ADR_SPACE_SMBUS) {
- pr_warn(PFX "Invalid ACPI SSIF I/O Address type: %d\n",
- spmi->addr.space_id);
- return -EIO;
- }
-
- myaddr = spmi->addr.address & 0x7f;
-
- return new_ssif_client(myaddr, NULL, 0, 0, SI_SPMI, NULL);
-}
-
-static void spmi_find_bmc(void)
-{
- acpi_status status;
- struct SPMITable *spmi;
- int i;
-
- if (acpi_disabled)
- return;
-
- if (acpi_failure)
- return;
-
- for (i = 0; ; i++) {
- status = acpi_get_table(ACPI_SIG_SPMI, i+1,
- (struct acpi_table_header **)&spmi);
- if (status != AE_OK)
- return;
-
- try_init_spmi(spmi);
- }
-}
-#else
-static void spmi_find_bmc(void) { }
#endif
#ifdef CONFIG_DMI
@@ -2200,9 +2098,6 @@ static int init_ipmi_ssif(void)
ssif_i2c_driver.driver.acpi_match_table =
ACPI_PTR(ssif_acpi_match);
- if (ssif_tryacpi)
- spmi_find_bmc();
-
if (ssif_trydmi) {
rv = platform_driver_register(&ipmi_driver);
if (rv)
--
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project.
Re: [PATCH] security: Fix IMA Kconfig for dependencies on ARM64
On 03/07/2018 04:19 PM, Mimi Zohar wrote: On Wed, 2018-03-07 at 11:41 -0800, James Bottomley wrote: On Wed, 2018-03-07 at 14:21 -0500, Mimi Zohar wrote: On Wed, 2018-03-07 at 11:08 -0800, James Bottomley wrote: On Wed, 2018-03-07 at 13:55 -0500, Mimi Zohar wrote: On Wed, 2018-03-07 at 11:51 -0700, Jason Gunthorpe wrote: On Tue, Mar 06, 2018 at 11:26:26PM -0600, Jiandi An wrote: TPM_CRB driver is the TPM support for ARM64. If it is built as module, TPM chip is registered after IMA init. tpm_pcr_read() in IMA driver would fail and display the following message even though eventually there is TPM chip on the system: ima: No TPM chip found, activating TPM-bypass! (rc=-19) Fix IMA Kconfig to select TPM_CRB so TPM_CRB driver is built in kernel and initializes before IMA driver. Signed-off-by: Jiandi An security/integrity/ima/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index 35ef693..6a8f677 100644 +++ b/security/integrity/ima/Kconfig @@ -10,6 +10,7 @@ config IMA select CRYPTO_HASH_INFO select TCG_TPM if HAS_IOMEM && !UML select TCG_TIS if TCG_TPM && X86 Well, this explains why IMA doesn't work on one of my X86 systems: it's got a non i2c infineon TPM. + select TCG_CRB if TCG_TPM && ACPI select TCG_IBMVTPM if TCG_TPM && PPC_PSERIES help The Trusted Computing Group(TCG) runtime Integrity This seems really weird, why are any specific TPM drivers linked to IMA config, we have lots of drivers.. I don't think I've ever seen this pattern in Kconfig before? As you've seen by the current discussions, the TPM driver needs to be initialized prior to IMA. Otherwise IMA goes into TPM- bypass mode. That implies that the TPM must be builtin to the kernel, and not as a kernel module. Actually, that's not necessarily true: If we don't begin appraisal until after the initrd phase, then the initrd can load TPM modules before IMA starts. This would involve a bit of code rejigging to not require a TPM until IMA wants to write its first measurement, but it looks doable and would get us out of having to second guess TPM selections. The question is about measurement, not appraisal. Although the initramfs might be measured, the initramfs can access files on the real root filesystem. Those files need to be measured, before they are used/accessed. Isn't it a question of threat model? Because the initrd is measured, you know it's the one you specified and you should know its security properties, so measurement doesn't really need to begin until the root pivots. Perhaps in the case where the initramfs is signed and the signature is verified, I would agree that I know the security properties of the initramfs. That still doesn't negate the fact that the initramfs could access files on real root, without first measuring them. At that point you pick up the boot aggregate so the log now is tied to the initrd measurement. Conversely, I can't really see a threat model where you could trick a correctly measured initrd into subverting IMA, especially because listening network daemons aren't usually active at this stage. Linux based boot loaders can be configured to download remote kernel images and initramfs files - network boot. I'm not saying there isn't a use case for wanting your TPM built in, I'm just saying I don't think it needs to be required for everyone who uses IMA. If the TPM module is not builtin, there are no guarantees when it was loaded. There could be a disconnect between the IMA measurement list and the TPM PCRs. If someone has a special use case, then I agree with you, that we could theoretically support it, but I don't think we want to confuse distros or anyone else. The TPM should be builtin, so that IMA measurements can begin before accessing real root. Mimi So from the discussion, I hear James suggests to overhaul the current IMA driver to not do measurement (calling tpm_pcr_read(), etc) until after initrd phase so TPM drivers can be built as modules. I hear Mimi insists TPM drivers should be built-in when IMA driver is enabled and set to Y in Kconfig. Do we have a consensus on which way we should go? I'm no expert on IMA and its driver. James, will you be kind enough to look into overhauling the IMA driver to not measure until after initrd phase if that's the consensus on resolving this? Thanks -Jiandi -- Jiandi An Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH] ipmi:ssif: Fix double probe from tryacpi and trydmi
On 03/08/2018 08:10 AM, Corey Minyard wrote:
On 03/07/2018 05:59 PM, Jiandi An wrote:
On 03/07/2018 07:34 AM, Corey Minyard wrote:
On 03/06/2018 11:49 PM, Jiandi An wrote:
IPMI SSIF driver's parameter tryacpi and trydmi both
are set to true. The addition of IPMI DMI driver to
create platform device for each IPMI device causes
SSIF probe to be done twice on the same SMB I2C address
for BMC. Fix is to not call trydmi if tryacpi is able
to find I2C address for BMC from SPMI ACPI table and
probe successfully.
Why are you trying to do this? Is something bad happening?
SPMI is not the preferred mechanism for detecting a device,
the preferred mechanism should be the acpi match table or
OF, followed by DMI, followed by SPMI. In fact, it might be
best to just remove SPMI.
-corey
On our ARM64 platform, SSIF is the IPMI interface for host to
BMC communication and it is described in ACPI SPMI table including
the I2C address. The driver would get the SSIF device from
IPI0001 ssif_acpi_match[] and probe. It worked fine with no issues.
Then it was reported dmidecode does not show the correct SSIF
device information including correct I2C address. So SSIF device
description is also added in SMBIOS table. It worked fine with no
issues until this patch.
0944d88 ipmi: Convert DMI handling over to a platform device
We would see error message indicating trydmi via
platform_driver_register fails with -EEXIST during boot.
[ 9.385758] ipmi_ssif: probe of dmi-ipmi-ssif.0 failed with error -17
This is because tryacpi ran first and successfully completed
new_ssif_client() (which adds address to ssif_info) and eventually
ssif_probe()
ssif_tryacpi
spmi_find_bmc()
try_init_spmi()
new_ssif_client()
Since both tryacpi and trydmi are set to true as module parameter
with no permission and not exposed to /sys/module/ipmi_ssif/parameters,
it triggers the following path down to dmi_ipmi_probe() and
new_ssif_client() which fails ssif_info_find() finds the address
added to ssif_info already in the ssif_tryacpi path.
ssif_trydmi
platform_driver_register(&ipmi_driver)
__platform_driver_register()
driver_register()
bus_add_driver()
driver_attach()
bus_for_each_dev()
__driver_attach()
driver_probe_device()
ssif_platform_probe()
dmi_ipmi_probe()
new_ssif_client()
Are you suggesting to not do tryacpi at all and just rely on
trydmi?
Basically, yes. SPMI is really designed for early detection of interfaces
before ACPI proper comes up. You should have the IPMI device in your
ACPI tree.
You meant to say I should have the IPMI SSIF device in my SMBIOS table?
Or do you mean to say I should have the IPMI SSIF device in my ACPI SPMI
table but you will remove SPMI support from the IPMI driver?
Do you want me to remove the ssif_tryacpi logic and tryacpi module
parameter all together in that patch?
Thanks
-Jiandi
My inclination is to remove SPMI support from the IPMI driver.
-corey
>>
I was looking at the following patch to understand more about
the added ipmi_dmi driver.
9f88145 ipmi: Create a platform device for a DMI-specified IPMI interface
It's creating a platform device for each IPMI device in the DMI
table including SSIF device, for auto-loading IPMI devices from
SMBIOS tables.
Are you suggesting removing SSIF device description from ACPI
SPMI table and remove ssif_tryacpi logic all together?
But the commit description mentions ...
"This also adds the ability to extract the slave address from
the SMBIOS tables, so that when the driver uses ACPI-specified
interfaces, it can still extract the slave address from SMBIOS."
This made me think SSIF driver can still use ACPI-specified
interface. It made me think it implies SSIF device can be
described in ACPI SPMI table and SMBIOS table. Both spec
did not say they cannot.
What's your recommended way of fixing this double probing?
Thanks
Signed-off-by: Jiandi An
---
drivers/char/ipmi/ipmi_ssif.c | 35
---
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_ssif.c
b/drivers/char/ipmi/ipmi_ssif.c
index 9d3b0fa..5c57363 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -1981,29 +1981,41 @@ static int try_init_spmi(struct SPMITable
*spmi)
return new_ssif_client(myaddr, NULL, 0, 0, SI_SPMI, NULL);
}
-static void spmi_find_bmc(void)
+static int spmi_find_bmc(void)
{
acpi_status status;
struct SPMITable *spmi;
int i;
+ int rc = 0;
if (acpi_disabled)
- return;
+ return -EPERM;
if (acpi_failure)
- return;
+ return -ENODEV;
fo
Re: [PATCH] ipmi:ssif: Fix double probe from tryacpi and trydmi
On 03/07/2018 07:34 AM, Corey Minyard wrote:
On 03/06/2018 11:49 PM, Jiandi An wrote:
IPMI SSIF driver's parameter tryacpi and trydmi both
are set to true. The addition of IPMI DMI driver to
create platform device for each IPMI device causes
SSIF probe to be done twice on the same SMB I2C address
for BMC. Fix is to not call trydmi if tryacpi is able
to find I2C address for BMC from SPMI ACPI table and
probe successfully.
Why are you trying to do this? Is something bad happening?
SPMI is not the preferred mechanism for detecting a device,
the preferred mechanism should be the acpi match table or
OF, followed by DMI, followed by SPMI. In fact, it might be
best to just remove SPMI.
-corey
On our ARM64 platform, SSIF is the IPMI interface for host to
BMC communication and it is described in ACPI SPMI table including
the I2C address. The driver would get the SSIF device from
IPI0001 ssif_acpi_match[] and probe. It worked fine with no issues.
Then it was reported dmidecode does not show the correct SSIF
device information including correct I2C address. So SSIF device
description is also added in SMBIOS table. It worked fine with no
issues until this patch.
0944d88 ipmi: Convert DMI handling over to a platform device
We would see error message indicating trydmi via
platform_driver_register fails with -EEXIST during boot.
[9.385758] ipmi_ssif: probe of dmi-ipmi-ssif.0 failed with error -17
This is because tryacpi ran first and successfully completed
new_ssif_client() (which adds address to ssif_info) and eventually
ssif_probe()
ssif_tryacpi
spmi_find_bmc()
try_init_spmi()
new_ssif_client()
Since both tryacpi and trydmi are set to true as module parameter
with no permission and not exposed to /sys/module/ipmi_ssif/parameters,
it triggers the following path down to dmi_ipmi_probe() and
new_ssif_client() which fails ssif_info_find() finds the address
added to ssif_info already in the ssif_tryacpi path.
ssif_trydmi
platform_driver_register(&ipmi_driver)
__platform_driver_register()
driver_register()
bus_add_driver()
driver_attach()
bus_for_each_dev()
__driver_attach()
driver_probe_device()
ssif_platform_probe()
dmi_ipmi_probe()
new_ssif_client()
Are you suggesting to not do tryacpi at all and just rely on
trydmi?
I was looking at the following patch to understand more about
the added ipmi_dmi driver.
9f88145 ipmi: Create a platform device for a DMI-specified IPMI interface
It's creating a platform device for each IPMI device in the DMI
table including SSIF device, for auto-loading IPMI devices from
SMBIOS tables.
Are you suggesting removing SSIF device description from ACPI
SPMI table and remove ssif_tryacpi logic all together?
But the commit description mentions ...
"This also adds the ability to extract the slave address from
the SMBIOS tables, so that when the driver uses ACPI-specified
interfaces, it can still extract the slave address from SMBIOS."
This made me think SSIF driver can still use ACPI-specified
interface. It made me think it implies SSIF device can be
described in ACPI SPMI table and SMBIOS table. Both spec
did not say they cannot.
What's your recommended way of fixing this double probing?
Thanks
Signed-off-by: Jiandi An
---
drivers/char/ipmi/ipmi_ssif.c | 35 ---
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_ssif.c
b/drivers/char/ipmi/ipmi_ssif.c
index 9d3b0fa..5c57363 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -1981,29 +1981,41 @@ static int try_init_spmi(struct SPMITable *spmi)
return new_ssif_client(myaddr, NULL, 0, 0, SI_SPMI, NULL);
}
-static void spmi_find_bmc(void)
+static int spmi_find_bmc(void)
{
acpi_status status;
struct SPMITable *spmi;
int i;
+ int rc = 0;
if (acpi_disabled)
- return;
+ return -EPERM;
if (acpi_failure)
- return;
+ return -ENODEV;
for (i = 0; ; i++) {
status = acpi_get_table(ACPI_SIG_SPMI, i+1,
(struct acpi_table_header **)&spmi);
- if (status != AE_OK)
- return;
+ if (status != AE_OK) {
+ if (i == 0)
+ return -ENODEV;
+ else
+ return 0;
+ }
- try_init_spmi(spmi);
+ rc = try_init_spmi(spmi);
+ if (rc)
+ return rc;
}
+
+ return 0;
}
#else
-static void spmi_find_bmc(void) { }
+static int spmi_find_bmc(void)
+{
+ return -ENODEV;
+}
#endif
#ifdef CONFIG_DMI
@@ -2104,12 +2116,13 @@ static int init_ipmi_ssif(
Re: [PATCH] security: Fix IMA Kconfig for dependencies on ARM64
On 03/07/2018 01:41 PM, James Bottomley wrote: On Wed, 2018-03-07 at 14:21 -0500, Mimi Zohar wrote: On Wed, 2018-03-07 at 11:08 -0800, James Bottomley wrote: On Wed, 2018-03-07 at 13:55 -0500, Mimi Zohar wrote: On Wed, 2018-03-07 at 11:51 -0700, Jason Gunthorpe wrote: On Tue, Mar 06, 2018 at 11:26:26PM -0600, Jiandi An wrote: TPM_CRB driver is the TPM support for ARM64. If it is built as module, TPM chip is registered after IMA init. tpm_pcr_read() in IMA driver would fail and display the following message even though eventually there is TPM chip on the system: ima: No TPM chip found, activating TPM-bypass! (rc=-19) Fix IMA Kconfig to select TPM_CRB so TPM_CRB driver is built in kernel and initializes before IMA driver. Signed-off-by: Jiandi An security/integrity/ima/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index 35ef693..6a8f677 100644 +++ b/security/integrity/ima/Kconfig @@ -10,6 +10,7 @@ config IMA select CRYPTO_HASH_INFO select TCG_TPM if HAS_IOMEM && !UML select TCG_TIS if TCG_TPM && X86 Well, this explains why IMA doesn't work on one of my X86 systems: it's got a non i2c infineon TPM. + select TCG_CRB if TCG_TPM && ACPI select TCG_IBMVTPM if TCG_TPM && PPC_PSERIES help The Trusted Computing Group(TCG) runtime Integrity This seems really weird, why are any specific TPM drivers linked to IMA config, we have lots of drivers.. I don't think I've ever seen this pattern in Kconfig before? As you've seen by the current discussions, the TPM driver needs to be initialized prior to IMA. Otherwise IMA goes into TPM- bypass mode. That implies that the TPM must be builtin to the kernel, and not as a kernel module. Actually, that's not necessarily true: If we don't begin appraisal until after the initrd phase, then the initrd can load TPM modules before IMA starts. This would involve a bit of code rejigging to not require a TPM until IMA wants to write its first measurement, but it looks doable and would get us out of having to second guess TPM selections. The question is about measurement, not appraisal. Although the initramfs might be measured, the initramfs can access files on the real root filesystem. Those files need to be measured, before they are used/accessed. Isn't it a question of threat model? Because the initrd is measured, you know it's the one you specified and you should know its security properties, so measurement doesn't really need to begin until the root pivots. At that point you pick up the boot aggregate so the log now is tied to the initrd measurement. Conversely, I can't really see a threat model where you could trick a correctly measured initrd into subverting IMA, especially because listening network daemons aren't usually active at this stage. I'm not saying there isn't a use case for wanting your TPM built in, I'm just saying I don't think it needs to be required for everyone who uses IMA. James ima_init() first calls tpm_pcr_read() which tries to use underlying registered TPM chip driver and send read PCR TPM command to the TPM chip. If IMA driver is enabled and ima_init() happens, we see this. In security/integrity/ima/ima_main.c, init_ima() is in late_initcall. And it calls ima_init(). late_initcall(init_ima); /* Start IMA after the TPM is available */ So as long as init_ima() is called, need to at least enable the TPM driver for the platform right? I'm just following current IMA Kconfig where it's selecting different underlying TPM chip drivers for various platforms respectively when CONFIG_IMA is set to Y because IMA driver init calls tpm_pcr_read() which needs to use TPM. -- Jiandi An Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
[PATCH] ipmi:ssif: Fix double probe from tryacpi and trydmi
IPMI SSIF driver's parameter tryacpi and trydmi both
are set to true. The addition of IPMI DMI driver to
create platform device for each IPMI device causes
SSIF probe to be done twice on the same SMB I2C address
for BMC. Fix is to not call trydmi if tryacpi is able
to find I2C address for BMC from SPMI ACPI table and
probe successfully.
Signed-off-by: Jiandi An
---
drivers/char/ipmi/ipmi_ssif.c | 35 ---
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 9d3b0fa..5c57363 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -1981,29 +1981,41 @@ static int try_init_spmi(struct SPMITable *spmi)
return new_ssif_client(myaddr, NULL, 0, 0, SI_SPMI, NULL);
}
-static void spmi_find_bmc(void)
+static int spmi_find_bmc(void)
{
acpi_status status;
struct SPMITable *spmi;
int i;
+ int rc = 0;
if (acpi_disabled)
- return;
+ return -EPERM;
if (acpi_failure)
- return;
+ return -ENODEV;
for (i = 0; ; i++) {
status = acpi_get_table(ACPI_SIG_SPMI, i+1,
(struct acpi_table_header **)&spmi);
- if (status != AE_OK)
- return;
+ if (status != AE_OK) {
+ if (i == 0)
+ return -ENODEV;
+ else
+ return 0;
+ }
- try_init_spmi(spmi);
+ rc = try_init_spmi(spmi);
+ if (rc)
+ return rc;
}
+
+ return 0;
}
#else
-static void spmi_find_bmc(void) { }
+static int spmi_find_bmc(void)
+{
+ return -ENODEV;
+}
#endif
#ifdef CONFIG_DMI
@@ -2104,12 +2116,13 @@ static int init_ipmi_ssif(void)
addr[i]);
}
- if (ssif_tryacpi)
+ if (ssif_tryacpi) {
ssif_i2c_driver.driver.acpi_match_table =
ACPI_PTR(ssif_acpi_match);
-
- if (ssif_tryacpi)
- spmi_find_bmc();
+ rv = spmi_find_bmc();
+ if (!rv)
+ ssif_trydmi = false;
+ }
if (ssif_trydmi) {
rv = platform_driver_register(&ipmi_driver);
--
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
Foundation Collaborative Project.
[PATCH] security: Fix IMA Kconfig for dependencies on ARM64
TPM_CRB driver is the TPM support for ARM64. If it is built as module, TPM chip is registered after IMA init. tpm_pcr_read() in IMA driver would fail and display the following message even though eventually there is TPM chip on the system: ima: No TPM chip found, activating TPM-bypass! (rc=-19) Fix IMA Kconfig to select TPM_CRB so TPM_CRB driver is built in kernel and initializes before IMA driver. Signed-off-by: Jiandi An --- security/integrity/ima/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index 35ef693..6a8f677 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -10,6 +10,7 @@ config IMA select CRYPTO_HASH_INFO select TCG_TPM if HAS_IOMEM && !UML select TCG_TIS if TCG_TPM && X86 + select TCG_CRB if TCG_TPM && ACPI select TCG_IBMVTPM if TCG_TPM && PPC_PSERIES help The Trusted Computing Group(TCG) runtime Integrity -- Jiandi An Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
[PATCH] tpm/tpm_crb: Use start method value from ACPI table directly
This patch gets rid of dealing with intermediate flag for start method
and use start method value from ACPI table directly.
For ARM64, the locality is handled by Trust Zone in FW. The layout
does not have crb_regs_head. It is hitting the following line.
dev_warn(dev, FW_BUG "Bad ACPI memory layout");
Current code excludes CRB_FL_ACPI_START for this check. Now since
ARM64 support for TPM CRB is added, CRB_FL_CRB_SMC_START should also be
excluded from this check.
For goIdle and cmdReady where code was excluding CRB_FL_ACPI_START only
(do nothing for ACPI start method), CRB_FL_CRB_SMC_START was also
excluded as ARM64 SMC start method does not have TPM_CRB_CTRL_REQ.
However with special PPT workaround requiring CRB_FL_CRB_START to be
set in addition to CRB_FL_ACPI_START and the addition flag of SMC
start method CRB_FL_CRB_SMC_START, the code has become difficult to
maintain and undrestand. It is better to make code deal with start
method value from ACPI table directly.
Signed-off-by: Jiandi An
---
drivers/char/tpm/tpm_crb.c | 59 +++---
1 file changed, 29 insertions(+), 30 deletions(-)
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 8f0a98d..7b3c2a8 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -92,14 +92,9 @@ enum crb_status {
CRB_DRV_STS_COMPLETE= BIT(0),
};
-enum crb_flags {
- CRB_FL_ACPI_START = BIT(0),
- CRB_FL_CRB_START= BIT(1),
- CRB_FL_CRB_SMC_START= BIT(2),
-};
-
struct crb_priv {
- unsigned int flags;
+ u32 sm;
+ const char *hid;
void __iomem *iobase;
struct crb_regs_head __iomem *regs_h;
struct crb_regs_tail __iomem *regs_t;
@@ -128,14 +123,16 @@ struct tpm2_crb_smc {
* Anyhow, we do not wait here as a consequent CMD_READY request
* will be handled correctly even if idle was not completed.
*
- * The function does nothing for devices with ACPI-start method.
+ * The function does nothing for devices with ACPI-start method
+ * or SMC-start method.
*
* Return: 0 always
*/
static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv
*priv)
{
- if ((priv->flags & CRB_FL_ACPI_START) ||
- (priv->flags & CRB_FL_CRB_SMC_START))
+ if ((priv->sm == ACPI_TPM2_START_METHOD) ||
+ (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
+ (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC))
return 0;
iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
@@ -174,14 +171,16 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32
mask, u32 value,
* The device should respond within TIMEOUT_C.
*
* The function does nothing for devices with ACPI-start method
+ * or SMC-start method.
*
* Return: 0 on success -ETIME on timeout;
*/
static int __maybe_unused crb_cmd_ready(struct device *dev,
struct crb_priv *priv)
{
- if ((priv->flags & CRB_FL_ACPI_START) ||
- (priv->flags & CRB_FL_CRB_SMC_START))
+ if ((priv->sm == ACPI_TPM2_START_METHOD) ||
+ (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
+ (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC))
return 0;
iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
@@ -325,13 +324,20 @@ static int crb_send(struct tpm_chip *chip, u8 *buf,
size_t len)
/* Make sure that cmd is populated before issuing start. */
wmb();
- if (priv->flags & CRB_FL_CRB_START)
+ /* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
+* report only ACPI start but in practice seems to require both
+* CRB start, hence invoking CRB start method if hid == MSFT0101.
+*/
+ if ((priv->sm == ACPI_TPM2_COMMAND_BUFFER) ||
+ (priv->sm == ACPI_TPM2_MEMORY_MAPPED) ||
+ (!strcmp(priv->hid, "MSFT0101")))
iowrite32(CRB_START_INVOKE, &priv->regs_t->ctrl_start);
- if (priv->flags & CRB_FL_ACPI_START)
+ if ((priv->sm == ACPI_TPM2_START_METHOD) ||
+ (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD))
rc = crb_do_acpi_start(chip);
- if (priv->flags & CRB_FL_CRB_SMC_START) {
+ if (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {
iowrite32(CRB_START_INVOKE, &priv->regs_t->ctrl_start);
rc = tpm_crb_smc_start(&chip->dev, priv->smc_func_id);
}
@@ -345,7 +351,9 @@ static void crb_cancel(struct tpm_chip *chip)
iowrite32(CRB_CANCEL_INVOKE, &priv->regs_t->ctrl_cancel);
- if ((priv->flags & CRB_FL_ACPI_START) && crb_do_acpi_start(chip))
+ if (((priv->sm == ACPI_TPM2_S
Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method
On 08/25/2017 11:53 AM, Jason Gunthorpe wrote: On Fri, Aug 25, 2017 at 07:21:39PM +0300, Jarkko Sakkinen wrote: As I said before it would make much more sense to make code always deal with sm and remove flags completely. That would help maintaining code easier as new logic for TZ is introduced. Yes please Jason Okay, I'll work on moving sm to priv and getting rid of flags from priv. Will submit a new patch with appropriate patch name and not a v2 of this patch since the scope of changes have changed. Thanks for your comments and feedback. -- Jiandi An Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method
On 08/24/2017 07:26 AM, Jarkko Sakkinen wrote:
On Tue, Aug 22, 2017 at 04:28:54PM -0500, Jiandi An wrote:
> > {
- if ((priv->flags & CRB_FL_ACPI_START) ||
- (priv->flags & CRB_FL_CRB_SMC_START))
- return 0;
-
- iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
- /* we don't really care when this settles */
+ if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0)
+ iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
+ /* we don't really care when this settles */
It's *exactly* the same condition expessed in different form.
I'm sorry perhaps I didn't fully understand the workaround specific to Intel
PPT. In previous patch thread, you mentioned the following where
a platform could report to require start method 2 (ACPI start) which is
sm = ACPI_TPM2_START_METHOD, and actually requires start method 8, which
is sm = ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD.
What this has to do with the above code change? Those two versions
compile most likely to almost if not exactly same machine code.
Both the original code and your updated blacklist cases where either
of those flags is set.
I know they don't change the logic. This was to address comment from
Jason. He wanted to express the exact condition which crb_go_idle(),
crb_cmd_ready(), and the check for "Bad ACPI memory layout" in
crb_map_io() should run, instead of the if not the condition, return.
Since you want it as is, I'll change it back. It's already excluding
CRB_FL_CRB_SMC_START in addition to CRB_FL_ACPI_START which is what's
intended.
Like I said the goal for this patch is to really further exclude
CRB_FL_CRB_SMC_START from the check for "Bad ACPI memory layout"
in crb_map_io() in the code below.
@@ -458,7 +455,7 @@ static int crb_map_io(struct acpi_device *device,
struct crb_priv *priv,
* the control area, as one nice sane region except for some older
* stuff that puts the control area outside the ACPI IO region.
*/
-if (!(priv->flags & CRB_FL_ACPI_START)) {
+if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0) {
if (buf->control_address == io_res.start +
sizeof(*priv->regs_h))
priv->regs_h = priv->iobase;
else
dev_warn(dev, FW_BUG "Bad ACPI memory layout");
}
I'll submit v2 with only this change. Are you okay which this change?
Thanks
- Jiandi
But you listed the following code logic which for either sm = 2 or
sm = 8, CRB_FL_ACPI_START flag is set.
if (sm == ACPI_TPM2_START_METHOD ||
sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
priv->flags |= CRB_FL_ACPI_START;
So I'm a little confused about the PPT workaround you mentioned
/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
* report only ACPI start but in practice seems to require both
* ACPI start and CRB start.
*/
if (sm == ACPI_TPM2_COMMAND_BUFFER || sm == ACPI_TPM2_MEMORY_MAPPED ||
!strcmp(acpi_device_hid(device), "MSFT0101"))
priv->flags |= CRB_FL_CRB_START;
I took it as some platform sm = 2 or sm = 8 which CRB_FL_ACPI_START
flag is set, additionally, if HID = MSFT0101, CRB_FL_CRB_START flag
is set.
Yes.
I'm starting to think that the code might be easier to follow if we
removed 'flags' and store sm to the priv struct and put conditionals in
places where we need them based on sm.
I think the 'flags' field was not a good idea in the first place.
/Jarkko
--
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project.
Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method
On 08/22/2017 12:39 PM, Jarkko Sakkinen wrote:
On Thu, Aug 17, 2017 at 11:15:36PM -0500, Jiandi An wrote:
For ARM64, the locality is handled by Trust Zone in FW.
The layout does not have crb_regs_head. It is hitting
the following line.
dev_warn(dev, FW_BUG "Bad ACPI memory layout");
Current code excludes CRB_FL_ACPI_START and when
CRB_FL_CRB_SMC_START is added around the same time
locality support is added, it should also be excluded.
For goIdle and cmdReady where code was excluding
CRB_FL_ACPI_START only (do nothing for ACPI start method),
CRB_FL_CRB_SMC_START was also excluded as ARM64 SMC start
method does not have TPM_CRB_CTRL_REQ.
Change if confition to white list instead of black list.
Signed-off-by: Jiandi An
---
drivers/char/tpm/tpm_crb.c | 35 ---
1 file changed, 16 insertions(+), 19 deletions(-)
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 8f0a98d..cbfdbdde 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -128,18 +128,16 @@ struct tpm2_crb_smc {
* Anyhow, we do not wait here as a consequent CMD_READY request
* will be handled correctly even if idle was not completed.
*
- * The function does nothing for devices with ACPI-start method.
+ * The function does nothing for devices with ACPI-start method
+ * or SMC-start method.
*
* Return: 0 always
*/
static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv
*priv)
{
- if ((priv->flags & CRB_FL_ACPI_START) ||
- (priv->flags & CRB_FL_CRB_SMC_START))
- return 0;
-
- iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
- /* we don't really care when this settles */
+ if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0)
+ iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
+ /* we don't really care when this settles */
It's *exactly* the same condition expessed in different form.
I'm sorry perhaps I didn't fully understand the workaround specific to
Intel PPT. In previous patch thread, you mentioned the following where
a platform could report to require start method 2 (ACPI start) which is
sm = ACPI_TPM2_START_METHOD, and actually requires start method 8, which
is sm = ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD.
But you listed the following code logic which for either sm = 2 or
sm = 8, CRB_FL_ACPI_START flag is set.
if (sm == ACPI_TPM2_START_METHOD ||
sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
priv->flags |= CRB_FL_ACPI_START;
So I'm a little confused about the PPT workaround you mentioned
/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
* report only ACPI start but in practice seems to require both
* ACPI start and CRB start.
*/
if (sm == ACPI_TPM2_COMMAND_BUFFER || sm == ACPI_TPM2_MEMORY_MAPPED ||
!strcmp(acpi_device_hid(device), "MSFT0101"))
priv->flags |= CRB_FL_CRB_START;
I took it as some platform sm = 2 or sm = 8 which CRB_FL_ACPI_START
flag is set, additionally, if HID = MSFT0101, CRB_FL_CRB_START flag
is set.
So I took it as the PPT workaround you mentioned is an instance where
both CRB_FL_ACPI_START and CRB_FL_CRB_START are set.
The original code logic for crb_go_idle() and crb_cmd_ready() only
exclude CRB_FL_ACPI_START, saying crb_go_idle() and crb_cmd_ready()
do nothing if CRB_FL_ACPI_START is set.
static int __maybe_unused crb_go_idle(struct device *dev,
struct crb_priv *priv)
{
if (priv->flags & CRB_FL_ACPI_START)
return 0;
So even in the case when both CRB_FL_ACPI_START and CRB_FL_CRB_START
are set, crb_go_idle() and crb_cmd_ready() will not do anything because
CRB_FL_ACPI_START is set.
And when SMC start method was enabled for ARM64, I further excluded
CRB_FL_CRB_SMC_START
static int __maybe_unused crb_go_idle(struct device *dev,
struct crb_priv *priv)
{
if ((priv->flags & CRB_FL_ACPI_START) ||
(prive->flags & CRB_FL_CRB_SMC_START))
return 0;
And Jason's comment was telling me to have these list the cases where
crb_go_idle() and crb_cmd_ready() are known to be required. Less
brittle that way. And he gave example...
if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_START)) == 0)
return 0
The following is your comment and Jason's comment in the other patch
thread discussion here
https://sourceforge.net/p/tpmdd/mailman/message/35984747/
*
What about platforms with firmware bug that they report only requiring
ACPI start but actually also require CRB start.
if (sm == ACPI_TPM2_START_METHOD ||
sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
priv->flags |= CRB_FL_ACPI_START;
I've encountered a platform that reports to require start
Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method
On 08/19/2017 12:05 PM, Jarkko Sakkinen wrote: On Thu, Aug 17, 2017 at 11:15:36PM -0500, Jiandi An wrote: For ARM64, the locality is handled by Trust Zone in FW. The layout does not have crb_regs_head. It is hitting the following line. dev_warn(dev, FW_BUG "Bad ACPI memory layout"); Current code excludes CRB_FL_ACPI_START and when CRB_FL_CRB_SMC_START is added around the same time locality support is added, it should also be excluded. For goIdle and cmdReady where code was excluding CRB_FL_ACPI_START only (do nothing for ACPI start method), CRB_FL_CRB_SMC_START was also excluded as ARM64 SMC start method does not have TPM_CRB_CTRL_REQ. Change if confition to white list instead of black list. Signed-off-by: Jiandi An Is this v2? If so, where is the change log? Based on the previous comments, I now understand that because of Intel PTT bug workaround, it is not appropriate for the patch to have title/subject "Access locality for only CRB_START method" So the more descriptive patch title is "Access locality for only non-ACPI and non-SMC start method". Because the patch is changed, I thought I start a new series. Would you like me to tag this v3 and put change log even though patch title has changed? Thanks - Jiandi /Jarkko -- Jiandi An Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
[PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method
For ARM64, the locality is handled by Trust Zone in FW.
The layout does not have crb_regs_head. It is hitting
the following line.
dev_warn(dev, FW_BUG "Bad ACPI memory layout");
Current code excludes CRB_FL_ACPI_START and when
CRB_FL_CRB_SMC_START is added around the same time
locality support is added, it should also be excluded.
For goIdle and cmdReady where code was excluding
CRB_FL_ACPI_START only (do nothing for ACPI start method),
CRB_FL_CRB_SMC_START was also excluded as ARM64 SMC start
method does not have TPM_CRB_CTRL_REQ.
Change if confition to white list instead of black list.
Signed-off-by: Jiandi An
---
drivers/char/tpm/tpm_crb.c | 35 ---
1 file changed, 16 insertions(+), 19 deletions(-)
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 8f0a98d..cbfdbdde 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -128,18 +128,16 @@ struct tpm2_crb_smc {
* Anyhow, we do not wait here as a consequent CMD_READY request
* will be handled correctly even if idle was not completed.
*
- * The function does nothing for devices with ACPI-start method.
+ * The function does nothing for devices with ACPI-start method
+ * or SMC-start method.
*
* Return: 0 always
*/
static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv
*priv)
{
- if ((priv->flags & CRB_FL_ACPI_START) ||
- (priv->flags & CRB_FL_CRB_SMC_START))
- return 0;
-
- iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
- /* we don't really care when this settles */
+ if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0)
+ iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
+ /* we don't really care when this settles */
return 0;
}
@@ -174,23 +172,22 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32
mask, u32 value,
* The device should respond within TIMEOUT_C.
*
* The function does nothing for devices with ACPI-start method
+ * or SMC-start method.
*
* Return: 0 on success -ETIME on timeout;
*/
static int __maybe_unused crb_cmd_ready(struct device *dev,
struct crb_priv *priv)
{
- if ((priv->flags & CRB_FL_ACPI_START) ||
- (priv->flags & CRB_FL_CRB_SMC_START))
- return 0;
-
- iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
- if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req,
-CRB_CTRL_REQ_CMD_READY /* mask */,
-0, /* value */
-TPM2_TIMEOUT_C)) {
- dev_warn(dev, "cmdReady timed out\n");
- return -ETIME;
+ if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0) {
+ iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
+ if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req,
+CRB_CTRL_REQ_CMD_READY /* mask */,
+0, /* value */
+TPM2_TIMEOUT_C)) {
+ dev_warn(dev, "cmdReady timed out\n");
+ return -ETIME;
+ }
}
return 0;
@@ -458,7 +455,7 @@ static int crb_map_io(struct acpi_device *device, struct
crb_priv *priv,
* the control area, as one nice sane region except for some older
* stuff that puts the control area outside the ACPI IO region.
*/
- if (!(priv->flags & CRB_FL_ACPI_START)) {
+ if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0) {
if (buf->control_address == io_res.start +
sizeof(*priv->regs_h))
priv->regs_h = priv->iobase;
--
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
Foundation Collaborative Project.
[PATCH v2] tpm/tpm_crb: Access locality for only CRB_START method
For ARM64, the locality is handled by Trust Zone in FW.
The layout does not have crb_regs_head. It is hitting
the following line.
dev_warn(dev, FW_BUG "Bad ACPI memory layout");
Current code excludes CRB_FL_ACPI_START and when
CRB_FL_CRB_SMC_START is added around the same time
locality support is added, it should also be excluded.
Change the if condition to be only for CRB_FL_CRB_START.
For goIdle and cmdReady where code was excluding
CRB_FL_ACPI_START only (do nothing for ACPI start method),
CRB_FL_CRB_SMC_START was also excluded as ARM64 SMC start
method does not have TPM_CRB_CTRL_REQ.
Change the if condition to be only for CRB_FL_CRB_START
in crb_go_idle() and crb_cmd_ready().
Signed-off-by: Jiandi An
---
v2
Changed if condition to CRB_FL_CRB_START for go idle
and crb_cmd_ready per maintainer's comment.
drivers/char/tpm/tpm_crb.c | 36 +---
1 file changed, 17 insertions(+), 19 deletions(-)
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 8f0a98d..7a6735d 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -128,18 +128,16 @@ struct tpm2_crb_smc {
* Anyhow, we do not wait here as a consequent CMD_READY request
* will be handled correctly even if idle was not completed.
*
- * The function does nothing for devices with ACPI-start method.
+ * The function does nothing for devices with ACPI-start method
+ * and SMC-start method.
*
* Return: 0 always
*/
static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv
*priv)
{
- if ((priv->flags & CRB_FL_ACPI_START) ||
- (priv->flags & CRB_FL_CRB_SMC_START))
- return 0;
-
- iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
- /* we don't really care when this settles */
+ if (priv->flags & CRB_FL_CRB_START)
+ iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
+ /* we don't really care when this settles */
return 0;
}
@@ -174,23 +172,23 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32
mask, u32 value,
* The device should respond within TIMEOUT_C.
*
* The function does nothing for devices with ACPI-start method
+ * and SMC-start method.
*
* Return: 0 on success -ETIME on timeout;
*/
static int __maybe_unused crb_cmd_ready(struct device *dev,
struct crb_priv *priv)
{
- if ((priv->flags & CRB_FL_ACPI_START) ||
- (priv->flags & CRB_FL_CRB_SMC_START))
- return 0;
-
- iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
- if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req,
-CRB_CTRL_REQ_CMD_READY /* mask */,
-0, /* value */
-TPM2_TIMEOUT_C)) {
- dev_warn(dev, "cmdReady timed out\n");
- return -ETIME;
+ if (priv->flags & CRB_FL_CRB_START)
+ {
+ iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
+ if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req,
+CRB_CTRL_REQ_CMD_READY /* mask */,
+0, /* value */
+TPM2_TIMEOUT_C)) {
+ dev_warn(dev, "cmdReady timed out\n");
+ return -ETIME;
+ }
}
return 0;
@@ -458,7 +456,7 @@ static int crb_map_io(struct acpi_device *device, struct
crb_priv *priv,
* the control area, as one nice sane region except for some older
* stuff that puts the control area outside the ACPI IO region.
*/
- if (!(priv->flags & CRB_FL_ACPI_START)) {
+ if (priv->flags & CRB_FL_CRB_START) {
if (buf->control_address == io_res.start +
sizeof(*priv->regs_h))
priv->regs_h = priv->iobase;
--
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
Foundation Collaborative Project.
[PATCH] tpm/tpm_crb: Access locality for only CRB_START method
For ARM64, the locality is handled by Trust Zone in FW.
The layout does not have crb_regs_head. It is hitting
the following line.
dev_warn(dev, FW_BUG "Bad ACPI memory layout");
Current code excludes CRB_FL_ACPI_START and when
CRB_FL_CRB_SMC_START is added around the same time
locality support is added, it should also be excluded.
Change the if condition to be only for CRB_FL_CRB_START.
For goIdle and cmdReady where code was excluding
CRB_FL_ACPI_START only (do nothing for ACPI start method),
CRB_FL_CRB_SMC_START was also excluded as ARM64 SMC start
method does not have TPM_CRB_CTRL_REQ.
Change the if condition to be only for CRB_FL_CRB_START
in crb_go_idle() and crb_cmd_ready().
Signed-off-by: Jiandi An
---
drivers/char/tpm/tpm_crb.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 8f0a98d..d583c50 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -128,14 +128,14 @@ struct tpm2_crb_smc {
* Anyhow, we do not wait here as a consequent CMD_READY request
* will be handled correctly even if idle was not completed.
*
- * The function does nothing for devices with ACPI-start method.
+ * The function does nothing for devices with ACPI-start method
+ * and SMC-start method.
*
* Return: 0 always
*/
static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv
*priv)
{
- if ((priv->flags & CRB_FL_ACPI_START) ||
- (priv->flags & CRB_FL_CRB_SMC_START))
+ if (!(priv->flags & CRB_FL_CRB_START))
return 0;
iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
@@ -174,14 +174,14 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32
mask, u32 value,
* The device should respond within TIMEOUT_C.
*
* The function does nothing for devices with ACPI-start method
+ * and SMC-start method.
*
* Return: 0 on success -ETIME on timeout;
*/
static int __maybe_unused crb_cmd_ready(struct device *dev,
struct crb_priv *priv)
{
- if ((priv->flags & CRB_FL_ACPI_START) ||
- (priv->flags & CRB_FL_CRB_SMC_START))
+ if (!(priv->flags & CRB_FL_CRB_START))
return 0;
iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
@@ -458,7 +458,7 @@ static int crb_map_io(struct acpi_device *device, struct
crb_priv *priv,
* the control area, as one nice sane region except for some older
* stuff that puts the control area outside the ACPI IO region.
*/
- if (!(priv->flags & CRB_FL_ACPI_START)) {
+ if (priv->flags & CRB_FL_CRB_START) {
if (buf->control_address == io_res.start +
sizeof(*priv->regs_h))
priv->regs_h = priv->iobase;
--
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
Foundation Collaborative Project.
Re: [PATCH v2] ACPI : Update platform device numa node based on _PXM method
On 03/29/2017 06:39 AM, Shanker Donthineni wrote:
The optional _PXM method evaluates to an integer that identifies the
proximity domain of a device object. On ACPI based kernel boot, the
field numa_node in 'struct device' is always set to -1 irrespective
of _PXM value that is specified in the ACPI device object. But in
case of device-tree based kernel boot the numa_node field is populated
and reflects to a DT property that is specified in DTS according to
the below document.
https://www.kernel.org/doc/Documentation/devicetree/bindings/numa.txt
http://lxr.free-electrons.com/source/drivers/of/device.c#L54
Without this patch dev_to_node() always returns a value of -1 for all
platform devices. This patch implements support for ACPI _PXM method
and updates the platform device numa node id using acpi_get_node(),
which provides the PXM to NUMA mapping information. The individual
platform device drivers should be able to use the NUMA aware memory
allocation functions kmalloc_node() and alloc_pages_node() to improve
the performance.
Signed-off-by: Shanker Donthineni
Tested-by: Jiandi An
---
Changes since v1: Edited commit text.
Not all the platform devices are attached to NUMA node 0 on Qualcomm
Datacenter Technologies server chips. Platform device drivers needs
to be aware of NUMA information to improve performance.
drivers/acpi/acpi_platform.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index b4c1a6a..83d953e 100644
--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -119,11 +119,14 @@ struct platform_device
*acpi_create_platform_device(struct acpi_device *adev,
if (IS_ERR(pdev))
dev_err(&adev->dev, "platform device creation failed:
%ld\n",
PTR_ERR(pdev));
- else
+ else {
+ set_dev_node(&pdev->dev, acpi_get_node(adev->handle));
dev_dbg(&adev->dev, "created platform device %s\n",
dev_name(&pdev->dev));
+ }
kfree(resources);
+
return pdev;
}
EXPORT_SYMBOL_GPL(acpi_create_platform_device);
--
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project.
[PATCH V2] Xen: ARM: Zero reserved fields of xatp before making hypervisor call
Ensure all reserved fields of xatp are zero before making
hypervisor call to XEN in xen_map_device_mmio().
xenmem_add_to_physmap_one() in XEN fails the mapping request if
extra.res reserved field in xatp is not zero for XENMAPSPACE_dev_mmio
request.
Signed-off-by: Jiandi An
---
Changed zeroing xatp to a static initialization
of .domid and .space for xatp.
drivers/xen/arm-device.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/xen/arm-device.c b/drivers/xen/arm-device.c
index 778acf8..85dd20e 100644
--- a/drivers/xen/arm-device.c
+++ b/drivers/xen/arm-device.c
@@ -58,9 +58,13 @@ static int xen_map_device_mmio(const struct resource
*resources,
xen_pfn_t *gpfns;
xen_ulong_t *idxs;
int *errs;
- struct xen_add_to_physmap_range xatp;
for (i = 0; i < count; i++) {
+ struct xen_add_to_physmap_range xatp = {
+ .domid = DOMID_SELF,
+ .space = XENMAPSPACE_dev_mmio
+ };
+
r = &resources[i];
nr = DIV_ROUND_UP(resource_size(r), XEN_PAGE_SIZE);
if ((resource_type(r) != IORESOURCE_MEM) || (nr == 0))
@@ -87,9 +91,7 @@ static int xen_map_device_mmio(const struct resource
*resources,
idxs[j] = XEN_PFN_DOWN(r->start) + j;
}
- xatp.domid = DOMID_SELF;
xatp.size = nr;
- xatp.space = XENMAPSPACE_dev_mmio;
set_xen_guest_handle(xatp.gpfns, gpfns);
set_xen_guest_handle(xatp.idxs, idxs);
--
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
Foundation Collaborative Project.
Re: [tpmdd-devel] [PATCH] tpm, tpm_crb: Handle 64-bit resource in crb_check_resource()
On 12/19/16 07:56, Jarkko Sakkinen wrote:
> On Sun, Dec 18, 2016 at 10:20:53PM -0600, Jiandi An wrote:
>> crb_check_resource() in TPM CRB driver calls
>> acpi_dev_resource_memory() which only handles 32-bit resources.
>> Adding a call to acpi_dev_resource_address_space() in TPM CRB
>> driver which handles 64-bit resources.
>>
>> Signed-off-by: Jiandi An
>
> 1. Is there a platform in existence where this change fixes a problem?
> 2. What is difference between "memory" and "address space" conceptually?
>Just wondering why 32-bit stuff is "memory" and 64-bit stuff is
>"address space". Could there be a one function that would work both
>for 32-bit and 64-bit cases?
>
> Yeah, I do not know this API too well. That's why I'm asking.
>
> /Jarkko
>
>
> If this is the right thing it also needs to be done in tpm_tis.
>
> I will point out that this driver only works with memory, so using a
> generic decoder without checking for IO maps may not be correct..
>
> Jason
>
Hi Jarkko and Jason,
Thanks for your comments.
I am a developer at Qualcomm and we are trying to enable TPM CRB driver
on ARM64 for Qualcomm Centriq 2400. Spec changes to ACPI table for TPM 2.0
have been submitted and approved by TCG and are currently in the 60 day waiting
period for release. I have a series of patches that do this TPM CRB driver
enablement for ARM64 that I'll be submitting.
But first, for our platform the control area buffer address is greater than
32-bit.
It is memory with no IO effects. I ran into this issue first when I use
QWordMemory in ACPI ASL to describe the resource. MemoryRangeType is specified
as
AddressRangeMemory.
The QWordMemory macro evaluates to a buffer which contains a 64-bit memory
resource descriptor, which describes a range of memory addresses. It is
a QWord Address Space Descriptor. acpi_dev_resource_address_space()
handles the 64-bit memory described using QWordMemory macro in ACPI ASL.
The Memory32Fixed macro evaluates to a buffer which contains a 32-bit memory
descriptor, which describes a fixed range of memory addresses.
acpi_dev_resource_memory() handles the 32-bit memory described using
Memory32Fixed
in ACPI ASL.
I did not see a specific acpi_dev_resource_ service that handles 64-bit resource
address and doesn't use the generic acpi_decode_space() decoder.
I do have a question about having to specify _CRS method in ACPI ASL.
When I was doing early prototyping for this enablement work on ARM64 back
during the time with 4.5 kernel, it was before the introduction of the following
two patches:
commit 1bd047be37d95bf65a219f4931215f71878ac060
tpm_crb: Use devm_ioremap_resource
commit 51dd43dff74b0547ad844638f6910ca29c956819
tpm_tis: Use devm_ioremap_resource
The control area buffer is specified in the TPM2.0 static ACPI table. TPM
CRB driver maps the control area address and reads out cmd and rsp buffer
addresses and maps them. There is no requirement in the TCG TPM ACPI spec
for specifying _CRS to describe the control area buffer. When I rebased
the early prototype for CRB driver ARM64 enablement to the latest kernel,
I hit this issue where I have to specify _CRS method. So in _CRS method
I specify the same control area address that's in the TPM2.0 static ACPI table.
I see the _CRS requirement in the CRB driver is for resource synchronization
between the TIS and CRB drivers to support force mode in TIS. Could I get some
background on that so I could be more careful not breaking TIS while making
changes to CRB driver for ARM64 enablement?
Thanks in advance.
>> ---
>> drivers/char/tpm/tpm_crb.c | 8 +---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
>> index 717b6b4..86f355b 100644
>> --- a/drivers/char/tpm/tpm_crb.c
>> +++ b/drivers/char/tpm/tpm_crb.c
>> @@ -264,10 +264,12 @@ static bool crb_req_canceled(struct tpm_chip *chip, u8
>> status)
>> static int crb_check_resource(struct acpi_resource *ares, void *data)
>> {
>> struct resource *io_res = data;
>> -struct resource res;
>> +struct resource_win win;
>> +struct resource *res = &(win.res);
>>
>> -if (acpi_dev_resource_memory(ares, &res)) {
>> -*io_res = res;
>> +if (acpi_dev_resource_memory(ares, res) ||
>> +acpi_dev_resource_address_space(ares, &win)) {
>> +*io_res = *res;
>> io_res->name = NULL;
>> }
>>
>> --
>> Jiandi An
>> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
>> Technologies, Inc.
>> Qualcomm Technologies, Inc. is a member of the Code Aurora
Re: [PATCH] Xen: ARM: Zero reserved fields of xatp before making hypervisor call
On 12/19/16 12:49, Stefano Stabellini wrote:
> On Mon, 19 Dec 2016, Juergen Gross wrote:
>> On 19/12/16 03:56, Jiandi An wrote:
>>> Ensure all reserved fields of xatp are zero before making hypervisor
>>> call to XEN in xen_map_device_mmio(). xenmem_add_to_physmap_one() in
>>> XEN fails the mapping request if extra.res reserved field in xatp is
>>> not zero for XENMAPSPACE_dev_mmio request.
>>>
>>> Signed-off-by: Jiandi An
>>> ---
>>> drivers/xen/arm-device.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/xen/arm-device.c b/drivers/xen/arm-device.c
>>> index 778acf8..208273b 100644
>>> --- a/drivers/xen/arm-device.c
>>> +++ b/drivers/xen/arm-device.c
>>> @@ -87,6 +87,9 @@ static int xen_map_device_mmio(const struct resource
>>> *resources,
>>> idxs[j] = XEN_PFN_DOWN(r->start) + j;
>>> }
>>>
>>> + /* Ensure reserved fields are set to zero */
>>> + memset(&xatp, 0, sizeof(xatp));
>>> +
>>> xatp.domid = DOMID_SELF;
>>> xatp.size = nr;
>>> xatp.space = XENMAPSPACE_dev_mmio;
>>
>> Instead of setting xatp to 0 in each iteration I'd prefer a static
>> initialization of .domid and .space:
>>
>> struct xen_add_to_physmap_range xatp = {
>> .domid = DOMID_SELF,
>> .space = XENMAPSPACE_dev_mmio
>> };
>
> +1
>
Hi Juergen,
Thanks for you comments. xatp is passed to XEN via the hypervisor call in each
loop.
XEN touches xatp and returns it back. For example XEN returns error of
underlying mapping call in the err[] array in xatp. (The err[] is not checked
after the hypervisor call returns and it's a bug to be addressed in a separate
patch) XEN could theoretically corrupt xatp when it's returned. And the loop
would go on to the next iteration passing in whatever that's in xatp returned
by the previous hypervisor call. Harder to debug in my opinion if xatp get
corrupted by XEN somehow when a bug is introduced in XEN. At first I put the
memset of xatp at the beginning outside of the loop. But I thought it's better
to initialize xatp that's passed in each time a hypervisor call is made so we
know exactly we set going into the hypervisor call.
--
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
Foundation Collaborative Project.
[PATCH] tpm, tpm_crb: Handle 64-bit resource in crb_check_resource()
crb_check_resource() in TPM CRB driver calls
acpi_dev_resource_memory() which only handles 32-bit resources.
Adding a call to acpi_dev_resource_address_space() in TPM CRB
driver which handles 64-bit resources.
Signed-off-by: Jiandi An
---
drivers/char/tpm/tpm_crb.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 717b6b4..86f355b 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -264,10 +264,12 @@ static bool crb_req_canceled(struct tpm_chip *chip, u8
status)
static int crb_check_resource(struct acpi_resource *ares, void *data)
{
struct resource *io_res = data;
- struct resource res;
+ struct resource_win win;
+ struct resource *res = &(win.res);
- if (acpi_dev_resource_memory(ares, &res)) {
- *io_res = res;
+ if (acpi_dev_resource_memory(ares, res) ||
+ acpi_dev_resource_address_space(ares, &win)) {
+ *io_res = *res;
io_res->name = NULL;
}
--
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
Foundation Collaborative Project.
[PATCH] Xen: ARM: Zero reserved fields of xatp before making hypervisor call
Ensure all reserved fields of xatp are zero before making hypervisor call to XEN in xen_map_device_mmio(). xenmem_add_to_physmap_one() in XEN fails the mapping request if extra.res reserved field in xatp is not zero for XENMAPSPACE_dev_mmio request. Signed-off-by: Jiandi An --- drivers/xen/arm-device.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/xen/arm-device.c b/drivers/xen/arm-device.c index 778acf8..208273b 100644 --- a/drivers/xen/arm-device.c +++ b/drivers/xen/arm-device.c @@ -87,6 +87,9 @@ static int xen_map_device_mmio(const struct resource *resources, idxs[j] = XEN_PFN_DOWN(r->start) + j; } + /* Ensure reserved fields are set to zero */ + memset(&xatp, 0, sizeof(xatp)); + xatp.domid = DOMID_SELF; xatp.size = nr; xatp.space = XENMAPSPACE_dev_mmio; -- Jiandi An Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
