[PATCH v2 13/63] iommu/amd: Use struct_group() for memcpy() region
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct ivhd_entry around members ext and hidh, so
they can be referenced together. This will allow memcpy() and sizeof()
to more easily reason about sizes, improve readability, and avoid future
warnings about writing beyond the end of ext.
"pahole" shows no size nor member offset changes to struct ivhd_entry.
"objdump -d" shows no object code changes.
Cc: Joerg Roedel
Cc: Will Deacon
Cc: iommu@lists.linux-foundation.org
Signed-off-by: Kees Cook
---
drivers/iommu/amd/init.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index bdcf167b4afe..70506d6175e9 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -121,8 +121,10 @@ struct ivhd_entry {
u8 type;
u16 devid;
u8 flags;
- u32 ext;
- u32 hidh;
+ struct_group(ext_hid,
+ u32 ext;
+ u32 hidh;
+ );
u64 cid;
u8 uidf;
u8 uidl;
@@ -1377,7 +1379,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
break;
}
- memcpy(hid, (u8 *)(&e->ext), ACPIHID_HID_LEN - 1);
+ BUILD_BUG_ON(sizeof(e->ext_hid) != ACPIHID_HID_LEN - 1);
+ memcpy(hid, &e->ext_hid, ACPIHID_HID_LEN - 1);
hid[ACPIHID_HID_LEN - 1] = '\0';
if (!(*hid)) {
--
2.30.2
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu-v3: Simplify useless instructions in arm_smmu_cmdq_build_cmd()
On 2021/8/17 21:23, Robin Murphy wrote:
> On 2021-08-17 12:34, Zhen Lei wrote:
>> Although the parameter 'cmd' is always passed by a local array variable,
>> and only this function modifies it, the compiler does not know this. The
>> compiler almost always reads the value of cmd[i] from memory rather than
>> directly using the value cached in the register. This generates many
>> useless instruction operations and affects the performance to some extent.
>
> Which compiler? GCC 4.9 does not make the same codegen decisions that GCC 10
> does; Clang is different again. There are also various config options which
> affect a compiler's inlining/optimisation choices either directly or
> indirectly.
gcc version 7.3.1 20180425 [linaro-7.3-2018.05 revision
d29120a424ecfbc167ef90065c0eeb7f91977701] (Linaro GCC 7.3-2018.05)
In addition, yesterday morning I also purposely compiled a compiler with the
latest
GCC source code. The result is the same.
gcc version 11.2.0 (GCC)
>
> If it's something that newer compilers can get right anyway, then
> micro-optimising just for older ones might warrant a bit more justification.
>
>> To guide the compiler for proper optimization, 'cmd' is defined as a local
>> array variable, marked as register, and copied to the output parameter at
>> a time when the function is returned.
>>
>> The optimization effect can be viewed by running the "size arm-smmu-v3.o"
>> command.
>>
>> Before:
>> text data bss dec hex
>> 27602 1348 56 29006 714e
>>
>> After:
>> text data bss dec hex
>> 27402 1348 56 28806 7086
>>
>> Signed-off-by: Zhen Lei
>> ---
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 +++---
>> 1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> index d76bbbde558b776..50a9db5bac466c7 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -233,11 +233,19 @@ static int queue_remove_raw(struct arm_smmu_queue *q,
>> u64 *ent)
>> return 0;
>> }
>> +#define arm_smmu_cmdq_copy_cmd(dst, src) \
>> + do { \
>> + dst[0] = src[0]; \
>> + dst[1] = src[1]; \
>> + } while (0)
>> +
>> /* High-level queue accessors */
>> -static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
>> +static int arm_smmu_cmdq_build_cmd(u64 *out_cmd, struct arm_smmu_cmdq_ent
>> *ent)
>> {
>> - memset(cmd, 0, 1 << CMDQ_ENT_SZ_SHIFT);
>> - cmd[0] |= FIELD_PREP(CMDQ_0_OP, ent->opcode);
>> + register u64 cmd[CMDQ_ENT_DWORDS];
>> +
>> + cmd[0] = FIELD_PREP(CMDQ_0_OP, ent->opcode);
>> + cmd[1] = 0;
>> switch (ent->opcode) {
>> case CMDQ_OP_TLBI_EL2_ALL:
>> @@ -309,6 +317,7 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct
>> arm_smmu_cmdq_ent *ent)
>> case PRI_RESP_SUCC:
>> break;
>> default:
>> + arm_smmu_cmdq_copy_cmd(out_cmd, cmd);
>
> Why bother writing back a partial command when we're telling the caller it's
> invalid anyway?
Some callers do not check the return value of arm_smmu_cmdq_build_cmd().
In particular, the arm_smmu_cmdq_batch_add has no judgment. Yes, I should
add judgment there.
>
>> return -EINVAL;
>> }
>> cmd[1] |= FIELD_PREP(CMDQ_PRI_1_RESP, ent->pri.resp);
>> @@ -329,9 +338,12 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct
>> arm_smmu_cmdq_ent *ent)
>> cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB);
>> break;
>> default:
>> + arm_smmu_cmdq_copy_cmd(out_cmd, cmd);
>
> Ditto.
>
>> return -ENOENT;
>> }
>> + arm_smmu_cmdq_copy_cmd(out_cmd, cmd);
>
> ...and then it would be simpler to open-code the assignment here.
Right, I'll modify it in v2. I also don't like the addition of
arm_smmu_cmdq_copy_cmd().
>
> I guess if you're really concerned with avoiding temporary commands being
> written back to the stack and reloaded, it might be worth experimenting with
> wrapping them in a struct which can be passed around by value - AAPCS64
> allows passing a 16-byte composite type purely in registers.
'out_cmd' is an output parameter. Use 16-byte composite type need to modify
many functions, this may not be good..
>
> Robin.
>
>> +
>> return 0;
>> }
>>
> .
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 02/29] dt-bindings: mediatek: mt8195: Add binding for infra IOMMU
On Fri, 13 Aug 2021 14:52:57 +0800, Yong Wu wrote: > In mt8195, we have a new IOMMU that is for INFRA IOMMU. its masters > mainly are PCIe and USB. Different with MM IOMMU, all these masters > connect with IOMMU directly, there is no mediatek,larbs property for > infra IOMMU. > > Another thing is about PCIe ports. currently the function > "of_iommu_configure_dev_id" only support the id number is 1, But our > PCIe have two ports, one is for reading and the other is for writing. > see more about the PCIe patch in this patchset. Thus, I only list > the reading id here and add the other id in our driver. > > Signed-off-by: Yong Wu > Acked-by: Krzysztof Kozlowski > --- > Change note: use "contains" commented from Rob. > --- > .../bindings/iommu/mediatek,iommu.yaml | 13 - > .../dt-bindings/memory/mt8195-memory-port.h| 18 ++ > include/dt-bindings/memory/mtk-memory-port.h | 2 ++ > 3 files changed, 32 insertions(+), 1 deletion(-) > Reviewed-by: Rob Herring ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/13] dt-bindings: memory: mediatek: Add mt8195 smi sub common
On Tue, Aug 10, 2021 at 04:08:48PM +0800, Yong Wu wrote: > Add the binding for smi-sub-common. The SMI block diagram like this: > > IOMMU > | | > smi-common > -- > | | > larb0 larb7 <-max is 8 > > The smi-common connects with smi-larb and IOMMU. The maximum larbs number > that connects with a smi-common is 8. If the engines number is over 8, > sometimes we use a smi-sub-common which is nearly same with smi-common. > It supports up to 8 input and 1 output(smi-common has 2 output) > > Something like: > > IOMMU > | | > smi-common > - > | | ... > larb0 sub-common ... <-max is 8 > --- >||... <-max is 8 too. > larb2 larb5 > > We don't need extra SW setting for smi-sub-common, only the sub-common has > special clocks need to enable when the engines access dram. > > If it is sub-common, it should have a "mediatek,smi" phandle to point to > its smi-common. meanwhile the sub-common only has one gals clock. > > Additionally, add a new property "mediatek,smi_sub_common" for this > sub-common, this is needed in the IOMMU driver in which we create a device > link between smi-common and the IOMMU device. If we add the smi-sub-common > here, the IOMMU driver still need to find the smi-common device. thus, > add this bool property to indicate if it is sub-common. > > Signed-off-by: Yong Wu > --- > change note: > a. change mediatek, smi type from phandle-array to phandle from Rob. > b. Add a new bool property (mediatek,smi_sub_common) to indicate this is >smi-sub-common. the reason is as above. > --- > .../mediatek,smi-common.yaml | 30 +++ > 1 file changed, 30 insertions(+) > > diff --git > a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml > > b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml > index 602592b6c3f5..26bb9903864b 100644 > --- > a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml > +++ > b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml > @@ -38,6 +38,7 @@ properties: >- mediatek,mt8192-smi-common >- mediatek,mt8195-smi-common-vdo >- mediatek,mt8195-smi-common-vpp > + - mediatek,mt8195-smi-sub-common > >- description: for mt7623 > items: > @@ -67,6 +68,14 @@ properties: > minItems: 2 > maxItems: 4 > > + mediatek,smi: > +$ref: /schemas/types.yaml#/definitions/phandle > +description: a phandle to the smi-common node above. Only for sub-common. > + > + mediatek,smi_sub_common: s/_/-/ > +type: boolean > +description: Indicate if this is smi-sub-common. > + > required: >- compatible >- reg > @@ -93,6 +102,27 @@ allOf: > - const: smi > - const: async > > + - if: # only for sub common > + properties: > +compatible: > + contains: > +enum: > + - mediatek,mt8195-smi-sub-common > +then: > + required: > +- mediatek,smi > +- mediatek,smi_sub_common > + properties: > +clock: > + items: > +minItems: 3 > +maxItems: 3 > +clock-names: > + items: > +- const: apb > +- const: smi > +- const: gals0 If not allowed for other compatibles, you need: else: properties: mediatek,sni: false mediatek,smi_sub_common: false > + >- if: # for gen2 HW that have gals >properties: > compatible: > -- > 2.18.0 > > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 06/12] x86/sev: Replace occurrences of sev_active() with prot_guest_has()
On Tue, Aug 17, 2021 at 10:26:18AM -0500, Tom Lendacky wrote: > >>/* > >> - * If SME is active we need to be sure that kexec pages are > >> - * not encrypted because when we boot to the new kernel the > >> + * If host memory encryption is active we need to be sure that kexec > >> + * pages are not encrypted because when we boot to the new kernel the > >> * pages won't be accessed encrypted (initially). > >> */ > > > > That hunk belongs logically into the previous patch which removes > > sme_active(). > > I was trying to keep the sev_active() changes separate... so even though > it's an SME thing, I kept it here. But I can move it to the previous > patch, it just might look strange. Oh I meant only the comment because it is a SME-related change. But not too important so whatever. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 05/12] x86/sme: Replace occurrences of sme_active() with prot_guest_has()
On Tue, Aug 17, 2021 at 09:46:58AM -0500, Tom Lendacky wrote: > I'm ok with letting the TDX folks make changes to these calls to be SME or > SEV specific, if necessary, later. Yap, exactly. Let's add the specific stuff only when really needed. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 03/12] x86/sev: Add an x86 version of prot_guest_has()
On Tue, Aug 17, 2021 at 10:22:52AM -0500, Tom Lendacky wrote: > I can change it to be an AMD/HYGON check... although, I'll have to check > to see if any (very) early use of the function will work with that. We can always change it later if really needed. It is just that I'm not a fan of such "preemptive" changes. > At a minimum, the check in arch/x86/kernel/head64.c will have to be > changed or removed. I'll take a closer look. Yeah, sme_me_mask, already discussed on IRC. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V3 08/13] HV/Vmbus: Initialize VMbus ring buffer for Isolation VM
On 8/17/2021 1:28 AM, Michael Kelley wrote: This patch does the following: 1) The existing ring buffer wrap-around mapping functionality is still executed in hv_ringbuffer_init() when not doing SNP isolation. This mapping is based on an array of struct page's that describe the contiguous physical memory. 2) New ring buffer wrap-around mapping functionality is added in hv_ringbuffer_post_init() for the SNP isolation case. The case is handled in hv_ringbuffer_post_init() because it must be done after the GPADL is established, since that's where the host visibility is set. What's interesting is that this case is exactly the same as #1 above, except that the mapping is based on physical memory addresses instead of struct page's. We have to use physical addresses because of applying the GPA boundary, and there are no struct page's for those physical addresses. Unfortunately, this duplicates a lot of logic in #1 and #2, except for the struct page vs. physical address difference. Proposal: Couldn't we always do #2, even for the normal case where SNP isolation is not being used? The difference would only be in whether the GPA boundary is added. And it looks like the normal case could be done after the GPADL is established, as setting up the GPADL doesn't have any dependencies on having the ring buffer mapped. This approach would remove a lot of duplication. Just move the calls to hv_ringbuffer_init() to after the GPADL is established, and do all the work there for both cases. Hi Michael: Thanks for suggestion. I just keep the original logic in current code. I will try combining these two functions and report back. Thanks. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 09/12] mm: Remove the now unused mem_encrypt_active() function
On 8/17/21 5:24 AM, Borislav Petkov wrote: > On Tue, Aug 17, 2021 at 12:22:33PM +0200, Borislav Petkov wrote: >> This one wants to be part of the previous patch. > > ... and the three following patches too - the treewide patch does a > single atomic :) replacement and that's it. Ok, I'll squash those all together. Thanks, Tom > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 06/12] x86/sev: Replace occurrences of sev_active() with prot_guest_has()
On 8/17/21 5:02 AM, Borislav Petkov wrote:
> On Fri, Aug 13, 2021 at 11:59:25AM -0500, Tom Lendacky wrote:
>> diff --git a/arch/x86/kernel/machine_kexec_64.c
>> b/arch/x86/kernel/machine_kexec_64.c
>> index 8e7b517ad738..66ff788b79c9 100644
>> --- a/arch/x86/kernel/machine_kexec_64.c
>> +++ b/arch/x86/kernel/machine_kexec_64.c
>> @@ -167,7 +167,7 @@ static int init_transition_pgtable(struct kimage *image,
>> pgd_t *pgd)
>> }
>> pte = pte_offset_kernel(pmd, vaddr);
>>
>> -if (sev_active())
>> +if (prot_guest_has(PATTR_GUEST_MEM_ENCRYPT))
>> prot = PAGE_KERNEL_EXEC;
>>
>> set_pte(pte, pfn_pte(paddr >> PAGE_SHIFT, prot));
>> @@ -207,7 +207,7 @@ static int init_pgtable(struct kimage *image, unsigned
>> long start_pgtable)
>> level4p = (pgd_t *)__va(start_pgtable);
>> clear_page(level4p);
>>
>> -if (sev_active()) {
>> +if (prot_guest_has(PATTR_GUEST_MEM_ENCRYPT)) {
>> info.page_flag |= _PAGE_ENC;
>> info.kernpg_flag |= _PAGE_ENC;
>> }
>> @@ -570,12 +570,12 @@ void arch_kexec_unprotect_crashkres(void)
>> */
>> int arch_kexec_post_alloc_pages(void *vaddr, unsigned int pages, gfp_t gfp)
>> {
>> -if (sev_active())
>> +if (!prot_guest_has(PATTR_HOST_MEM_ENCRYPT))
>> return 0;
>>
>> /*
>> - * If SME is active we need to be sure that kexec pages are
>> - * not encrypted because when we boot to the new kernel the
>> + * If host memory encryption is active we need to be sure that kexec
>> + * pages are not encrypted because when we boot to the new kernel the
>> * pages won't be accessed encrypted (initially).
>> */
>
> That hunk belongs logically into the previous patch which removes
> sme_active().
I was trying to keep the sev_active() changes separate... so even though
it's an SME thing, I kept it here. But I can move it to the previous
patch, it just might look strange.
>
>> return set_memory_decrypted((unsigned long)vaddr, pages);
>> @@ -583,12 +583,12 @@ int arch_kexec_post_alloc_pages(void *vaddr, unsigned
>> int pages, gfp_t gfp)
>>
>> void arch_kexec_pre_free_pages(void *vaddr, unsigned int pages)
>> {
>> -if (sev_active())
>> +if (!prot_guest_has(PATTR_HOST_MEM_ENCRYPT))
>> return;
>>
>> /*
>> - * If SME is active we need to reset the pages back to being
>> - * an encrypted mapping before freeing them.
>> + * If host memory encryption is active we need to reset the pages back
>> + * to being an encrypted mapping before freeing them.
>> */
>> set_memory_encrypted((unsigned long)vaddr, pages);
>> }
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index e8ccab50ebf6..b69f5ac622d5 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -25,6 +25,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include
>> #include
>> @@ -457,7 +458,7 @@ static int has_svm(void)
>> return 0;
>> }
>>
>> -if (sev_active()) {
>> +if (prot_guest_has(PATTR_SEV)) {
>> pr_info("KVM is unsupported when running as an SEV guest\n");
>> return 0;
>
> Same question as for PATTR_SME. PATTR_GUEST_MEM_ENCRYPT should be enough.
Yup, I'll change them all.
>
>> @@ -373,7 +373,7 @@ int __init early_set_memory_encrypted(unsigned long
>> vaddr, unsigned long size)
>> * up under SME the trampoline area cannot be encrypted, whereas under SEV
>> * the trampoline area must be encrypted.
>> */
>> -bool sev_active(void)
>> +static bool sev_active(void)
>> {
>> return sev_status & MSR_AMD64_SEV_ENABLED;
>> }
>> @@ -382,7 +382,6 @@ static bool sme_active(void)
>> {
>> return sme_me_mask && !sev_active();
>> }
>> -EXPORT_SYMBOL_GPL(sev_active);
>
> Just get rid of it altogether.
Ok.
Thanks,
Tom
>
> Thx.
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 03/12] x86/sev: Add an x86 version of prot_guest_has()
On 8/15/21 9:39 AM, Borislav Petkov wrote: > On Sun, Aug 15, 2021 at 08:53:31AM -0500, Tom Lendacky wrote: >> It's not a cross-vendor thing as opposed to a KVM or other hypervisor >> thing where the family doesn't have to be reported as AMD or HYGON. > > What would be the use case? A HV starts a guest which is supposed to be > encrypted using the AMD's confidential guest technology but the HV tells > the guest that it is not running on an AMD SVM HV but something else? > > Is that even an actual use case? > > Or am I way off? > > I know we have talked about this in the past but this still sounds > insane. Maybe the KVM folks have a better understanding of it... I can change it to be an AMD/HYGON check... although, I'll have to check to see if any (very) early use of the function will work with that. At a minimum, the check in arch/x86/kernel/head64.c will have to be changed or removed. I'll take a closer look. Thanks, Tom > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 05/12] x86/sme: Replace occurrences of sme_active() with prot_guest_has()
On 8/17/21 4:00 AM, Borislav Petkov wrote:
> On Fri, Aug 13, 2021 at 11:59:24AM -0500, Tom Lendacky wrote:
>> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
>> index edc67ddf065d..5635ca9a1fbe 100644
>> --- a/arch/x86/mm/mem_encrypt.c
>> +++ b/arch/x86/mm/mem_encrypt.c
>> @@ -144,7 +144,7 @@ void __init sme_unmap_bootdata(char *real_mode_data)
>> struct boot_params *boot_data;
>> unsigned long cmdline_paddr;
>>
>> -if (!sme_active())
>> +if (!amd_prot_guest_has(PATTR_SME))
>> return;
>>
>> /* Get the command line address before unmapping the real_mode_data */
>> @@ -164,7 +164,7 @@ void __init sme_map_bootdata(char *real_mode_data)
>> struct boot_params *boot_data;
>> unsigned long cmdline_paddr;
>>
>> -if (!sme_active())
>> +if (!amd_prot_guest_has(PATTR_SME))
>> return;
>>
>> __sme_early_map_unmap_mem(real_mode_data, sizeof(boot_params), true);
>> @@ -378,7 +378,7 @@ bool sev_active(void)
>> return sev_status & MSR_AMD64_SEV_ENABLED;
>> }
>>
>> -bool sme_active(void)
>> +static bool sme_active(void)
>
> Just get rid of it altogether. Also, there's an
>
> EXPORT_SYMBOL_GPL(sev_active);
> > which needs to go under the actual function. Here's a diff ontop:
Will do.
>
> ---
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 5635ca9a1fbe..a3a2396362a5 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -364,8 +364,9 @@ int __init early_set_memory_encrypted(unsigned long
> vaddr, unsigned long size)
> /*
> * SME and SEV are very similar but they are not the same, so there are
> * times that the kernel will need to distinguish between SME and SEV. The
> - * sme_active() and sev_active() functions are used for this. When a
> - * distinction isn't needed, the mem_encrypt_active() function can be used.
> + * PATTR_HOST_MEM_ENCRYPT and PATTR_GUEST_MEM_ENCRYPT flags to
> + * amd_prot_guest_has() are used for this. When a distinction isn't needed,
> + * the mem_encrypt_active() function can be used.
> *
> * The trampoline code is a good example for this requirement. Before
> * paging is activated, SME will access all memory as decrypted, but SEV
> @@ -377,11 +378,6 @@ bool sev_active(void)
> {
> return sev_status & MSR_AMD64_SEV_ENABLED;
> }
> -
> -static bool sme_active(void)
> -{
> - return sme_me_mask && !sev_active();
> -}
> EXPORT_SYMBOL_GPL(sev_active);
>
> /* Needs to be called from non-instrumentable code */
> @@ -398,7 +394,7 @@ bool amd_prot_guest_has(unsigned int attr)
>
> case PATTR_SME:
> case PATTR_HOST_MEM_ENCRYPT:
> - return sme_active();
> + return sme_me_mask && !sev_active();
>
> case PATTR_SEV:
> case PATTR_GUEST_MEM_ENCRYPT:
>
>> {
>> return sme_me_mask && !sev_active();
>> }
>> @@ -428,7 +428,7 @@ bool force_dma_unencrypted(struct device *dev)
>> * device does not support DMA to addresses that include the
>> * encryption mask.
>> */
>> -if (sme_active()) {
>> +if (amd_prot_guest_has(PATTR_SME)) {
>
> So I'm not sure: you add PATTR_SME which you call with
> amd_prot_guest_has() and PATTR_HOST_MEM_ENCRYPT which you call with
> prot_guest_has() and they both end up being the same thing on AMD.
>
> So why even bother with PATTR_SME?
>
> This is only going to cause confusion later and I'd say let's simply use
> prot_guest_has(PATTR_HOST_MEM_ENCRYPT) everywhere...
Ok, I can do that. I was trying to ensure that anything that is truly SME
or SEV specific would be called out now.
I'm ok with letting the TDX folks make changes to these calls to be SME or
SEV specific, if necessary, later.
Thanks,
Tom
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 04/12] powerpc/pseries/svm: Add a powerpc version of prot_guest_has()
On 8/17/21 3:35 AM, Borislav Petkov wrote: > On Fri, Aug 13, 2021 at 11:59:23AM -0500, Tom Lendacky wrote: >> Introduce a powerpc version of the prot_guest_has() function. This will >> be used to replace the powerpc mem_encrypt_active() implementation, so >> the implementation will initially only support the PATTR_MEM_ENCRYPT >> attribute. >> >> Cc: Michael Ellerman >> Cc: Benjamin Herrenschmidt >> Cc: Paul Mackerras >> Signed-off-by: Tom Lendacky >> --- >> arch/powerpc/include/asm/protected_guest.h | 30 ++ >> arch/powerpc/platforms/pseries/Kconfig | 1 + >> 2 files changed, 31 insertions(+) >> create mode 100644 arch/powerpc/include/asm/protected_guest.h >> >> diff --git a/arch/powerpc/include/asm/protected_guest.h >> b/arch/powerpc/include/asm/protected_guest.h >> new file mode 100644 >> index ..ce55c2c7e534 >> --- /dev/null >> +++ b/arch/powerpc/include/asm/protected_guest.h >> @@ -0,0 +1,30 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Protected Guest (and Host) Capability checks >> + * >> + * Copyright (C) 2021 Advanced Micro Devices, Inc. >> + * >> + * Author: Tom Lendacky >> + */ >> + >> +#ifndef _POWERPC_PROTECTED_GUEST_H >> +#define _POWERPC_PROTECTED_GUEST_H >> + >> +#include >> + >> +#ifndef __ASSEMBLY__ > > Same thing here. Pls audit the whole set whether those __ASSEMBLY__ > guards are really needed and remove them if not. Will do. Thanks, Tom > > Thx. > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu-v3: Simplify useless instructions in arm_smmu_cmdq_build_cmd()
On 2021-08-17 12:34, Zhen Lei wrote:
Although the parameter 'cmd' is always passed by a local array variable,
and only this function modifies it, the compiler does not know this. The
compiler almost always reads the value of cmd[i] from memory rather than
directly using the value cached in the register. This generates many
useless instruction operations and affects the performance to some extent.
Which compiler? GCC 4.9 does not make the same codegen decisions that
GCC 10 does; Clang is different again. There are also various config
options which affect a compiler's inlining/optimisation choices either
directly or indirectly.
If it's something that newer compilers can get right anyway, then
micro-optimising just for older ones might warrant a bit more justification.
To guide the compiler for proper optimization, 'cmd' is defined as a local
array variable, marked as register, and copied to the output parameter at
a time when the function is returned.
The optimization effect can be viewed by running the "size arm-smmu-v3.o"
command.
Before:
textdata bss dec hex
276021348 56 29006714e
After:
textdata bss dec hex
274021348 56 288067086
Signed-off-by: Zhen Lei
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index d76bbbde558b776..50a9db5bac466c7 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -233,11 +233,19 @@ static int queue_remove_raw(struct arm_smmu_queue *q, u64
*ent)
return 0;
}
+#define arm_smmu_cmdq_copy_cmd(dst, src) \
+ do {\
+ dst[0] = src[0];\
+ dst[1] = src[1];\
+ } while (0)
+
/* High-level queue accessors */
-static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
+static int arm_smmu_cmdq_build_cmd(u64 *out_cmd, struct arm_smmu_cmdq_ent *ent)
{
- memset(cmd, 0, 1 << CMDQ_ENT_SZ_SHIFT);
- cmd[0] |= FIELD_PREP(CMDQ_0_OP, ent->opcode);
+ register u64 cmd[CMDQ_ENT_DWORDS];
+
+ cmd[0] = FIELD_PREP(CMDQ_0_OP, ent->opcode);
+ cmd[1] = 0;
switch (ent->opcode) {
case CMDQ_OP_TLBI_EL2_ALL:
@@ -309,6 +317,7 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct
arm_smmu_cmdq_ent *ent)
case PRI_RESP_SUCC:
break;
default:
+ arm_smmu_cmdq_copy_cmd(out_cmd, cmd);
Why bother writing back a partial command when we're telling the caller
it's invalid anyway?
return -EINVAL;
}
cmd[1] |= FIELD_PREP(CMDQ_PRI_1_RESP, ent->pri.resp);
@@ -329,9 +338,12 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct
arm_smmu_cmdq_ent *ent)
cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSIATTR,
ARM_SMMU_MEMATTR_OIWB);
break;
default:
+ arm_smmu_cmdq_copy_cmd(out_cmd, cmd);
Ditto.
return -ENOENT;
}
+ arm_smmu_cmdq_copy_cmd(out_cmd, cmd);
...and then it would be simpler to open-code the assignment here.
I guess if you're really concerned with avoiding temporary commands
being written back to the stack and reloaded, it might be worth
experimenting with wrapping them in a struct which can be passed around
by value - AAPCS64 allows passing a 16-byte composite type purely in
registers.
Robin.
+
return 0;
}
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/vt-d: Fix PASID reference leak
On 2021/8/14 2:13, Fenghua Yu wrote: A PASID reference is increased whenever a device is bound to an mm (and its PASID) successfully (i.e. the device's sdev user count is increased). But the reference is not dropped every time the device is unbound successfully from the mm (i.e. the device's sdev user count is decreased). The reference is dropped only once by calling intel_svm_free_pasid() when there isn't any device bound to the mm. intel_svm_free_pasid() drops the reference and only frees the PASID on zero reference. Fix the issue by dropping the PASID reference and freeing the PASID when no reference on successful unbinding the device by calling intel_svm_free_pasid() . Signed-off-by: Fenghua Yu Patch has been queued for iommu/fix. Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 3/3] iommu/vt-d: Fix Unexpected page request in Privilege Mode
On 2021/8/17 12:24, Liu Yi L wrote:
This patch fixes the below error. This is due to improper iotlb invalidation
in intel_pasid_tear_down_entry().
[ 180.187556] Unexpected page request in Privilege Mode
[ 180.187565] Unexpected page request in Privilege Mode
[ 180.279933] Unexpected page request in Privilege Mode
[ 180.279937] Unexpected page request in Privilege Mode
Per chapter 6.5.3.3 of VT-d spec 3.3, when tear down a pasid entry, software
should use Domain selective IOTLB flush if the PGTT of the pasid entry is
SL only or Nested, while for the pasid entries whose PGTT is FL only or PT
using PASID-based IOTLB flush is enough.
Fixes: 1c4f88b7f1f9 ("iommu/vt-d: Shared virtual address in scalable mode")
Cc: Lu Baolu
Signed-off-by: Kumar Sanjay K
Signed-off-by: Liu Yi L
Tested-by: Yi Sun
Good catch! Thanks!
It has been queued for iommu/fix.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 2/2] iommu/vt-d: Fix incomplete cache flush in intel_pasid_tear_down_entry()
From: Liu Yi L
This fixes improper iotlb invalidation in intel_pasid_tear_down_entry().
When a PASID was used as nested mode, released and reused, the following
error message will appear:
[ 180.187556] Unexpected page request in Privilege Mode
[ 180.187565] Unexpected page request in Privilege Mode
[ 180.279933] Unexpected page request in Privilege Mode
[ 180.279937] Unexpected page request in Privilege Mode
Per chapter 6.5.3.3 of VT-d spec 3.3, when tear down a pasid entry, the
software should use Domain selective IOTLB flush if the PGTT of the pasid
entry is SL only or Nested, while for the pasid entries whose PGTT is FL
only or PT using PASID-based IOTLB flush is enough.
Fixes: 2cd1311a26673 ("iommu/vt-d: Add set domain DOMAIN_ATTR_NESTING attr")
Signed-off-by: Kumar Sanjay K
Signed-off-by: Liu Yi L
Tested-by: Yi Sun
Link: https://lore.kernel.org/r/20210817042425.1784279-1-yi.l@intel.com
Signed-off-by: Lu Baolu
---
drivers/iommu/intel/pasid.h | 6 ++
drivers/iommu/intel/pasid.c | 10 --
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
index 5ff61c3d401f..c11bc8b833b8 100644
--- a/drivers/iommu/intel/pasid.h
+++ b/drivers/iommu/intel/pasid.h
@@ -99,6 +99,12 @@ static inline bool pasid_pte_is_present(struct pasid_entry
*pte)
return READ_ONCE(pte->val[0]) & PASID_PTE_PRESENT;
}
+/* Get PGTT field of a PASID table entry */
+static inline u16 pasid_pte_get_pgtt(struct pasid_entry *pte)
+{
+ return (u16)((READ_ONCE(pte->val[0]) >> 6) & 0x7);
+}
+
extern unsigned int intel_pasid_max_id;
int intel_pasid_alloc_table(struct device *dev);
void intel_pasid_free_table(struct device *dev);
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index c6cf44a6c923..9ec374e17469 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -511,7 +511,7 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
struct device *dev,
u32 pasid, bool fault_ignore)
{
struct pasid_entry *pte;
- u16 did;
+ u16 did, pgtt;
pte = intel_pasid_get_entry(dev, pasid);
if (WARN_ON(!pte))
@@ -521,13 +521,19 @@ void intel_pasid_tear_down_entry(struct intel_iommu
*iommu, struct device *dev,
return;
did = pasid_get_domain_id(pte);
+ pgtt = pasid_pte_get_pgtt(pte);
+
intel_pasid_clear_entry(dev, pasid, fault_ignore);
if (!ecap_coherent(iommu->ecap))
clflush_cache_range(pte, sizeof(*pte));
pasid_cache_invalidation_with_pasid(iommu, did, pasid);
- qi_flush_piotlb(iommu, did, pasid, 0, -1, 0);
+
+ if (pgtt == PASID_ENTRY_PGTT_PT || pgtt == PASID_ENTRY_PGTT_FL_ONLY)
+ qi_flush_piotlb(iommu, did, pasid, 0, -1, 0);
+ else
+ iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);
/* Device IOTLB doesn't need to be flushed in caching mode. */
if (!cap_caching_mode(iommu->cap))
--
2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 1/2] iommu/vt-d: Fix PASID reference leak
From: Fenghua Yu
A PASID reference is increased whenever a device is bound to an mm (and
its PASID) successfully (i.e. the device's sdev user count is increased).
But the reference is not dropped every time the device is unbound
successfully from the mm (i.e. the device's sdev user count is decreased).
The reference is dropped only once by calling intel_svm_free_pasid() when
there isn't any device bound to the mm. intel_svm_free_pasid() drops the
reference and only frees the PASID on zero reference.
Fix the issue by dropping the PASID reference and freeing the PASID when
no reference on successful unbinding the device by calling
intel_svm_free_pasid() .
Fixes: 4048377414162 ("iommu/vt-d: Use iommu_sva_alloc(free)_pasid() helpers")
Signed-off-by: Fenghua Yu
Link: https://lore.kernel.org/r/20210813181345.1870742-1-fenghua...@intel.com
Signed-off-by: Lu Baolu
---
drivers/iommu/intel/svm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 9b0f22bc0514..4b9b3f35ba0e 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -675,7 +675,6 @@ static int intel_svm_unbind_mm(struct device *dev, u32
pasid)
kfree_rcu(sdev, rcu);
if (list_empty(&svm->devs)) {
- intel_svm_free_pasid(mm);
if (svm->notifier.ops) {
mmu_notifier_unregister(&svm->notifier,
mm);
/* Clear mm's pasid. */
@@ -690,6 +689,8 @@ static int intel_svm_unbind_mm(struct device *dev, u32
pasid)
kfree(svm);
}
}
+ /* Drop a PASID reference and free it if no reference. */
+ intel_svm_free_pasid(mm);
}
out:
return ret;
--
2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 0/2] [PULL REQUEST] iommu/vt-d: Fixes for v5.14-rc7
Hi Joerg, Two small fixes are queued for v5.14. They aim to fix: - PASID reference leakage in intel_svm_unbind_mm(); - An improper iotlb invalidation in intel_pasid_tear_down_entry(). Please consider it for your iommu/fix branch. Best regards, Lu Baolu Fenghua Yu (1): iommu/vt-d: Fix PASID reference leak Liu Yi L (1): iommu/vt-d: Fix incomplete cache flush in intel_pasid_tear_down_entry() drivers/iommu/intel/pasid.h | 6 ++ drivers/iommu/intel/pasid.c | 10 -- drivers/iommu/intel/svm.c | 3 ++- 3 files changed, 16 insertions(+), 3 deletions(-) -- 2.25.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 04/12] powerpc/pseries/svm: Add a powerpc version of prot_guest_has()
Tom Lendacky writes: > Introduce a powerpc version of the prot_guest_has() function. This will > be used to replace the powerpc mem_encrypt_active() implementation, so > the implementation will initially only support the PATTR_MEM_ENCRYPT > attribute. > > Cc: Michael Ellerman > Cc: Benjamin Herrenschmidt > Cc: Paul Mackerras > Signed-off-by: Tom Lendacky > --- > arch/powerpc/include/asm/protected_guest.h | 30 ++ > arch/powerpc/platforms/pseries/Kconfig | 1 + > 2 files changed, 31 insertions(+) > create mode 100644 arch/powerpc/include/asm/protected_guest.h > > diff --git a/arch/powerpc/include/asm/protected_guest.h > b/arch/powerpc/include/asm/protected_guest.h > new file mode 100644 > index ..ce55c2c7e534 > --- /dev/null > +++ b/arch/powerpc/include/asm/protected_guest.h > @@ -0,0 +1,30 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Protected Guest (and Host) Capability checks > + * > + * Copyright (C) 2021 Advanced Micro Devices, Inc. > + * > + * Author: Tom Lendacky > + */ > + > +#ifndef _POWERPC_PROTECTED_GUEST_H > +#define _POWERPC_PROTECTED_GUEST_H Minor nit, we would usually use _ASM_POWERPC_PROTECTED_GUEST_H Otherwise looks OK to me. Acked-by: Michael Ellerman cheers ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu-v3: Stop pre-zeroing batch commands in arm_smmu_atc_inv_master()
On 17/08/2021 12:34, Zhen Lei wrote:
Pre-zeroing the batched commands structure is inefficient, as individual
commands are zeroed later in arm_smmu_cmdq_build_cmd(). Therefore, only
the member 'num' needs to be initialized to 0.
Signed-off-by: Zhen Lei
Reviewed-by: John Garry
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 50a9db5bac466c7..e6882ae81fd08f6 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1776,10 +1776,11 @@ static int arm_smmu_atc_inv_master(struct
arm_smmu_master *master)
{
int i;
struct arm_smmu_cmdq_ent cmd;
- struct arm_smmu_cmdq_batch cmds = {};
+ struct arm_smmu_cmdq_batch cmds;
arm_smmu_atc_inv_to_cmd(0, 0, 0, &cmd);
+ cmds.num = 0;
We prob should have added a comment why we do this (and at the other
sites). I think Robin said something similar in another patch.
for (i = 0; i < master->num_streams; i++) {
cmd.atc.sid = master->streams[i].id;
arm_smmu_cmdq_batch_add(master->smmu, &cmds, &cmd);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/arm-smmu-v3: Simplify useless instructions in arm_smmu_cmdq_build_cmd()
Although the parameter 'cmd' is always passed by a local array variable,
and only this function modifies it, the compiler does not know this. The
compiler almost always reads the value of cmd[i] from memory rather than
directly using the value cached in the register. This generates many
useless instruction operations and affects the performance to some extent.
To guide the compiler for proper optimization, 'cmd' is defined as a local
array variable, marked as register, and copied to the output parameter at
a time when the function is returned.
The optimization effect can be viewed by running the "size arm-smmu-v3.o"
command.
Before:
textdata bss dec hex
276021348 56 29006714e
After:
textdata bss dec hex
274021348 56 288067086
Signed-off-by: Zhen Lei
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index d76bbbde558b776..50a9db5bac466c7 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -233,11 +233,19 @@ static int queue_remove_raw(struct arm_smmu_queue *q, u64
*ent)
return 0;
}
+#define arm_smmu_cmdq_copy_cmd(dst, src) \
+ do {\
+ dst[0] = src[0];\
+ dst[1] = src[1];\
+ } while (0)
+
/* High-level queue accessors */
-static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
+static int arm_smmu_cmdq_build_cmd(u64 *out_cmd, struct arm_smmu_cmdq_ent *ent)
{
- memset(cmd, 0, 1 << CMDQ_ENT_SZ_SHIFT);
- cmd[0] |= FIELD_PREP(CMDQ_0_OP, ent->opcode);
+ register u64 cmd[CMDQ_ENT_DWORDS];
+
+ cmd[0] = FIELD_PREP(CMDQ_0_OP, ent->opcode);
+ cmd[1] = 0;
switch (ent->opcode) {
case CMDQ_OP_TLBI_EL2_ALL:
@@ -309,6 +317,7 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct
arm_smmu_cmdq_ent *ent)
case PRI_RESP_SUCC:
break;
default:
+ arm_smmu_cmdq_copy_cmd(out_cmd, cmd);
return -EINVAL;
}
cmd[1] |= FIELD_PREP(CMDQ_PRI_1_RESP, ent->pri.resp);
@@ -329,9 +338,12 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct
arm_smmu_cmdq_ent *ent)
cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSIATTR,
ARM_SMMU_MEMATTR_OIWB);
break;
default:
+ arm_smmu_cmdq_copy_cmd(out_cmd, cmd);
return -ENOENT;
}
+ arm_smmu_cmdq_copy_cmd(out_cmd, cmd);
+
return 0;
}
--
2.26.0.106.g9fadedd
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/arm-smmu-v3: Stop pre-zeroing batch commands in arm_smmu_atc_inv_master()
Pre-zeroing the batched commands structure is inefficient, as individual
commands are zeroed later in arm_smmu_cmdq_build_cmd(). Therefore, only
the member 'num' needs to be initialized to 0.
Signed-off-by: Zhen Lei
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 50a9db5bac466c7..e6882ae81fd08f6 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1776,10 +1776,11 @@ static int arm_smmu_atc_inv_master(struct
arm_smmu_master *master)
{
int i;
struct arm_smmu_cmdq_ent cmd;
- struct arm_smmu_cmdq_batch cmds = {};
+ struct arm_smmu_cmdq_batch cmds;
arm_smmu_atc_inv_to_cmd(0, 0, 0, &cmd);
+ cmds.num = 0;
for (i = 0; i < master->num_streams; i++) {
cmd.atc.sid = master->streams[i].id;
arm_smmu_cmdq_batch_add(master->smmu, &cmds, &cmd);
--
2.26.0.106.g9fadedd
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 09/12] mm: Remove the now unused mem_encrypt_active() function
On Tue, Aug 17, 2021 at 12:22:33PM +0200, Borislav Petkov wrote: > This one wants to be part of the previous patch. ... and the three following patches too - the treewide patch does a single atomic :) replacement and that's it. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 09/12] mm: Remove the now unused mem_encrypt_active() function
On Fri, Aug 13, 2021 at 11:59:28AM -0500, Tom Lendacky wrote:
> The mem_encrypt_active() function has been replaced by prot_guest_has(),
> so remove the implementation.
>
> Reviewed-by: Joerg Roedel
> Signed-off-by: Tom Lendacky
> ---
> include/linux/mem_encrypt.h | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/include/linux/mem_encrypt.h b/include/linux/mem_encrypt.h
> index 5c4a18a91f89..ae4526389261 100644
> --- a/include/linux/mem_encrypt.h
> +++ b/include/linux/mem_encrypt.h
> @@ -16,10 +16,6 @@
>
> #include
>
> -#else/* !CONFIG_ARCH_HAS_MEM_ENCRYPT */
> -
> -static inline bool mem_encrypt_active(void) { return false; }
> -
> #endif /* CONFIG_ARCH_HAS_MEM_ENCRYPT */
>
> #ifdef CONFIG_AMD_MEM_ENCRYPT
> --
This one wants to be part of the previous patch.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 07/12] x86/sev: Replace occurrences of sev_es_active() with prot_guest_has()
On Fri, Aug 13, 2021 at 11:59:26AM -0500, Tom Lendacky wrote: > Replace occurrences of sev_es_active() with the more generic > prot_guest_has() using PATTR_GUEST_PROT_STATE, except for in > arch/x86/kernel/sev*.c and arch/x86/mm/mem_encrypt*.c where PATTR_SEV_ES > will be used. If future support is added for other memory encyrption > techonologies, the use of PATTR_GUEST_PROT_STATE can be updated, as > required, to specifically use PATTR_SEV_ES. > > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: Borislav Petkov > Signed-off-by: Tom Lendacky > --- > arch/x86/include/asm/mem_encrypt.h | 2 -- > arch/x86/kernel/sev.c | 6 +++--- > arch/x86/mm/mem_encrypt.c | 7 +++ > arch/x86/realmode/init.c | 3 +-- > 4 files changed, 7 insertions(+), 11 deletions(-) Same comments to this one as for the previous two. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 06/12] x86/sev: Replace occurrences of sev_active() with prot_guest_has()
On Fri, Aug 13, 2021 at 11:59:25AM -0500, Tom Lendacky wrote:
> diff --git a/arch/x86/kernel/machine_kexec_64.c
> b/arch/x86/kernel/machine_kexec_64.c
> index 8e7b517ad738..66ff788b79c9 100644
> --- a/arch/x86/kernel/machine_kexec_64.c
> +++ b/arch/x86/kernel/machine_kexec_64.c
> @@ -167,7 +167,7 @@ static int init_transition_pgtable(struct kimage *image,
> pgd_t *pgd)
> }
> pte = pte_offset_kernel(pmd, vaddr);
>
> - if (sev_active())
> + if (prot_guest_has(PATTR_GUEST_MEM_ENCRYPT))
> prot = PAGE_KERNEL_EXEC;
>
> set_pte(pte, pfn_pte(paddr >> PAGE_SHIFT, prot));
> @@ -207,7 +207,7 @@ static int init_pgtable(struct kimage *image, unsigned
> long start_pgtable)
> level4p = (pgd_t *)__va(start_pgtable);
> clear_page(level4p);
>
> - if (sev_active()) {
> + if (prot_guest_has(PATTR_GUEST_MEM_ENCRYPT)) {
> info.page_flag |= _PAGE_ENC;
> info.kernpg_flag |= _PAGE_ENC;
> }
> @@ -570,12 +570,12 @@ void arch_kexec_unprotect_crashkres(void)
> */
> int arch_kexec_post_alloc_pages(void *vaddr, unsigned int pages, gfp_t gfp)
> {
> - if (sev_active())
> + if (!prot_guest_has(PATTR_HOST_MEM_ENCRYPT))
> return 0;
>
> /*
> - * If SME is active we need to be sure that kexec pages are
> - * not encrypted because when we boot to the new kernel the
> + * If host memory encryption is active we need to be sure that kexec
> + * pages are not encrypted because when we boot to the new kernel the
>* pages won't be accessed encrypted (initially).
>*/
That hunk belongs logically into the previous patch which removes
sme_active().
> return set_memory_decrypted((unsigned long)vaddr, pages);
> @@ -583,12 +583,12 @@ int arch_kexec_post_alloc_pages(void *vaddr, unsigned
> int pages, gfp_t gfp)
>
> void arch_kexec_pre_free_pages(void *vaddr, unsigned int pages)
> {
> - if (sev_active())
> + if (!prot_guest_has(PATTR_HOST_MEM_ENCRYPT))
> return;
>
> /*
> - * If SME is active we need to reset the pages back to being
> - * an encrypted mapping before freeing them.
> + * If host memory encryption is active we need to reset the pages back
> + * to being an encrypted mapping before freeing them.
>*/
> set_memory_encrypted((unsigned long)vaddr, pages);
> }
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index e8ccab50ebf6..b69f5ac622d5 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -25,6 +25,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -457,7 +458,7 @@ static int has_svm(void)
> return 0;
> }
>
> - if (sev_active()) {
> + if (prot_guest_has(PATTR_SEV)) {
> pr_info("KVM is unsupported when running as an SEV guest\n");
> return 0;
Same question as for PATTR_SME. PATTR_GUEST_MEM_ENCRYPT should be enough.
> @@ -373,7 +373,7 @@ int __init early_set_memory_encrypted(unsigned long
> vaddr, unsigned long size)
> * up under SME the trampoline area cannot be encrypted, whereas under SEV
> * the trampoline area must be encrypted.
> */
> -bool sev_active(void)
> +static bool sev_active(void)
> {
> return sev_status & MSR_AMD64_SEV_ENABLED;
> }
> @@ -382,7 +382,6 @@ static bool sme_active(void)
> {
> return sme_me_mask && !sev_active();
> }
> -EXPORT_SYMBOL_GPL(sev_active);
Just get rid of it altogether.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 05/12] x86/sme: Replace occurrences of sme_active() with prot_guest_has()
On Fri, Aug 13, 2021 at 11:59:24AM -0500, Tom Lendacky wrote:
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index edc67ddf065d..5635ca9a1fbe 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -144,7 +144,7 @@ void __init sme_unmap_bootdata(char *real_mode_data)
> struct boot_params *boot_data;
> unsigned long cmdline_paddr;
>
> - if (!sme_active())
> + if (!amd_prot_guest_has(PATTR_SME))
> return;
>
> /* Get the command line address before unmapping the real_mode_data */
> @@ -164,7 +164,7 @@ void __init sme_map_bootdata(char *real_mode_data)
> struct boot_params *boot_data;
> unsigned long cmdline_paddr;
>
> - if (!sme_active())
> + if (!amd_prot_guest_has(PATTR_SME))
> return;
>
> __sme_early_map_unmap_mem(real_mode_data, sizeof(boot_params), true);
> @@ -378,7 +378,7 @@ bool sev_active(void)
> return sev_status & MSR_AMD64_SEV_ENABLED;
> }
>
> -bool sme_active(void)
> +static bool sme_active(void)
Just get rid of it altogether. Also, there's an
EXPORT_SYMBOL_GPL(sev_active);
which needs to go under the actual function. Here's a diff ontop:
---
diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index 5635ca9a1fbe..a3a2396362a5 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -364,8 +364,9 @@ int __init early_set_memory_encrypted(unsigned long vaddr,
unsigned long size)
/*
* SME and SEV are very similar but they are not the same, so there are
* times that the kernel will need to distinguish between SME and SEV. The
- * sme_active() and sev_active() functions are used for this. When a
- * distinction isn't needed, the mem_encrypt_active() function can be used.
+ * PATTR_HOST_MEM_ENCRYPT and PATTR_GUEST_MEM_ENCRYPT flags to
+ * amd_prot_guest_has() are used for this. When a distinction isn't needed,
+ * the mem_encrypt_active() function can be used.
*
* The trampoline code is a good example for this requirement. Before
* paging is activated, SME will access all memory as decrypted, but SEV
@@ -377,11 +378,6 @@ bool sev_active(void)
{
return sev_status & MSR_AMD64_SEV_ENABLED;
}
-
-static bool sme_active(void)
-{
- return sme_me_mask && !sev_active();
-}
EXPORT_SYMBOL_GPL(sev_active);
/* Needs to be called from non-instrumentable code */
@@ -398,7 +394,7 @@ bool amd_prot_guest_has(unsigned int attr)
case PATTR_SME:
case PATTR_HOST_MEM_ENCRYPT:
- return sme_active();
+ return sme_me_mask && !sev_active();
case PATTR_SEV:
case PATTR_GUEST_MEM_ENCRYPT:
> {
> return sme_me_mask && !sev_active();
> }
> @@ -428,7 +428,7 @@ bool force_dma_unencrypted(struct device *dev)
>* device does not support DMA to addresses that include the
>* encryption mask.
>*/
> - if (sme_active()) {
> + if (amd_prot_guest_has(PATTR_SME)) {
So I'm not sure: you add PATTR_SME which you call with
amd_prot_guest_has() and PATTR_HOST_MEM_ENCRYPT which you call with
prot_guest_has() and they both end up being the same thing on AMD.
So why even bother with PATTR_SME?
This is only going to cause confusion later and I'd say let's simply use
prot_guest_has(PATTR_HOST_MEM_ENCRYPT) everywhere...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 04/12] powerpc/pseries/svm: Add a powerpc version of prot_guest_has()
On Fri, Aug 13, 2021 at 11:59:23AM -0500, Tom Lendacky wrote: > Introduce a powerpc version of the prot_guest_has() function. This will > be used to replace the powerpc mem_encrypt_active() implementation, so > the implementation will initially only support the PATTR_MEM_ENCRYPT > attribute. > > Cc: Michael Ellerman > Cc: Benjamin Herrenschmidt > Cc: Paul Mackerras > Signed-off-by: Tom Lendacky > --- > arch/powerpc/include/asm/protected_guest.h | 30 ++ > arch/powerpc/platforms/pseries/Kconfig | 1 + > 2 files changed, 31 insertions(+) > create mode 100644 arch/powerpc/include/asm/protected_guest.h > > diff --git a/arch/powerpc/include/asm/protected_guest.h > b/arch/powerpc/include/asm/protected_guest.h > new file mode 100644 > index ..ce55c2c7e534 > --- /dev/null > +++ b/arch/powerpc/include/asm/protected_guest.h > @@ -0,0 +1,30 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Protected Guest (and Host) Capability checks > + * > + * Copyright (C) 2021 Advanced Micro Devices, Inc. > + * > + * Author: Tom Lendacky > + */ > + > +#ifndef _POWERPC_PROTECTED_GUEST_H > +#define _POWERPC_PROTECTED_GUEST_H > + > +#include > + > +#ifndef __ASSEMBLY__ Same thing here. Pls audit the whole set whether those __ASSEMBLY__ guards are really needed and remove them if not. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
