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(>devs)) {
- intel_svm_free_pasid(mm);
if (svm->notifier.ops) {
mmu_notifier_unregister(>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, );
+ 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, , );
___
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, );
+ 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, , );
--
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
Re: [RFC 0/5] Support non-coherent DMA on RISC-V using a global pool
On Tue, Aug 17, 2021 at 11:28 AM Atish Patra wrote: > > On Mon, Aug 16, 2021 at 6:37 PM Guo Ren wrote: > > > > 1 > > > > On Thu, Jul 29, 2021 at 2:19 PM Atish Patra wrote: > > > > > > On Wed, Jul 28, 2021 at 9:30 PM Palmer Dabbelt wrote: > > > > > > > > On Fri, 23 Jul 2021 14:40:26 PDT (-0700), Atish Patra wrote: > > > > > RISC-V privilege specification doesn't define an IOMMU or any method > > > > > modify > > > > > PMA attributes or PTE entries to allow non-coherent mappings yet. In > > > > > the beginning, this approach was adopted assuming that most of the > > > > > RISC-V > > > > > platforms would support full cache-coherent IO. Here is excerpt from > > > > > the > > > > > priv spec section 3.6.5 > > > > > > > > > > "In RISC-V platforms, the use of hardware-incoherent regions is > > > > > discouraged > > > > > due to software complexity, performance, and energy impacts." > > > > > > > > > > While some of the RISC-V platforms adhere to the above suggestion, > > > > > not all > > > > > platforms can afford to build to fully cache-coherent I/O devices. To > > > > > address DMA for non-coherent I/O devices, we need to mark a region of > > > > > memory > > > > > as non-cacheable. Some of the platforms rely on a fixed region of > > > > > uncached > > > > > memory that is remapped to the system memory while some other modify > > > > > the PTE entries to achieve that. > > > > > > > > > > The patch3 solves the issue for the fist use case by using a global > > > > > pool of memory that is reserved for DMA. The device access the > > > > > reserved area > > > > > of the region while corresponding CPU address will be from uncached > > > > > region > > > > > As the uncached region is remapped to the beginning of the system > > > > > ram, both > > > > > CPU and device get the same view. > > > > > > > > > > To facilitate streaming DMA APIs, patch 1 introduces a set of generic > > > > > cache operations. Any platform can use the generic ops to provide > > > > > platform > > > > > specific cache management operations. Once the standard RISC-V CMO > > > > > extension > > > > > is available, it will also use these generic ops. > > > > > > > > > > To address the second use case, Page Based Memory Attribute (PBMT) > > > > > extension > > > > > is proposed. Once the extension is in good shape, we can leverage that > > > > > using CONFIG_DIRECT_REMAP. Currently, it is selected via a compile > > > > > time config > > > > > option. We will probably need another arch specific hooks to know if > > > > > the > > > > > platform supports direct remap at runtime. For RISC-V, it will check > > > > > the > > > > > presence of PBMT extension while other arch function will simply > > > > > return true > > > > > > > > IIUC this is another extension that's not yet frozen or implemented in > > > > hardware? Is this one compatible with what's in the c906, or is it > > > > doing things its own way? > > > > > > Hi Palmer, > > > This series doesn't implement the PBMT extension which is still in > > > discussion. > > > It simply reuse the existing non-coherent dma mapping support in > > > upstream kernel and enable > > > it for RISC-V. The current version uses a non-coherent global pool. I > > > will update it to use arch_set_uncached > > > as per Christoph's suggestion. It solves the non-coherent DMA problem > > > for beagleV and not c906. > > > > > > I briefly mentioned the PBMT extension just to provide an idea how the > > > RISC-V Linux kernel > > > can support both unached window and PBMT extension. PBMT extension is > > > planned to be frozen > > > by the end of this year and none of the hardware has implemented it. > > > > > > The implementation in c906 is a non-standard one and will not be > > > supported by the default PBMT > > > extension implementation. > > The default PBMT & c908 PBMT are similar, only BIT definitions are > > different. I propose to support default PBMT first and give the back > > door to modify the PBMT definition during boot. > > The "protection_map[] = (__P000, __P001 ..__S000, __S001)" in > > mm/mmap.c has been modified by arch/mips, arm, sparc, x86, so I think > > it's proper solution direction. > > > > The reset problem is how to passing custom PBMT at the very early boot > > stage. Now I'm trying to use the DTS parsing instead of boot param hdr > > which Anup objected to. > > > > IIRC, c906 has a compatible mode that has the compliant PTE bit modifications. > Can you use that mode iOn the Allwinner D1 board to boot Linux ? I am > not sure if you have any fallback method for non-coherent DMA > if custom DMA_COHERENT bits are not enabled through enhanced mode ? We need custom PBMT(enhanced mode) to enable the dma driver on D1 (GMAC, USB, EMMC) or these drivers couldn't work. D1 hasn't any uncached region in SOC design. > > > ref: > > https://lore.kernel.org/linux-riscv/1623693067-53886-1-git-send-email-guo...@kernel.org/ > > > > Any comments are welcome. > > > > > > > > > > > > > > > >
Re: [RFC 1/5] RISC-V: Implement arch_sync_dma* functions
On Tue, Aug 17, 2021 at 11:24 AM Atish Patra wrote:
>
> On Mon, Aug 16, 2021 at 6:48 PM Guo Ren wrote:
> >
> > On Sat, Jul 24, 2021 at 5:40 AM Atish Patra wrote:
> > >
> > > To facilitate streaming DMA APIs, this patch introduces a set of generic
> > > cache operations related dma sync. Any platform can use the generic ops
> > > to provide platform specific cache management operations. Once the
> > > standard RISC-V CMO extension is available, it can be built on top of it.
> > >
> > > Signed-off-by: Atish Patra
> > > ---
> > > arch/riscv/include/asm/dma-noncoherent.h | 19 +++
> > > arch/riscv/mm/Makefile | 1 +
> > > arch/riscv/mm/dma-noncoherent.c | 66
> > > 3 files changed, 86 insertions(+)
> > > create mode 100644 arch/riscv/include/asm/dma-noncoherent.h
> > > create mode 100644 arch/riscv/mm/dma-noncoherent.c
> > >
> > > diff --git a/arch/riscv/include/asm/dma-noncoherent.h
> > > b/arch/riscv/include/asm/dma-noncoherent.h
> > > new file mode 100644
> > > index ..5bdb03c9c427
> > > --- /dev/null
> > > +++ b/arch/riscv/include/asm/dma-noncoherent.h
> > > @@ -0,0 +1,19 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +/*
> > > + * Copyright (c) 2021 Western Digital Corporation or its affiliates.
> > > + */
> > > +
> > > +#ifndef __ASM_RISCV_DMA_NON_COHERENT_H
> > > +#define __ASM_RISCV_DMA_NON_COHERENT_H
> > > +
> > > +#ifdef CONFIG_RISCV_DMA_NONCOHERENT
> > > +struct riscv_dma_cache_sync {
> > > + void (*cache_invalidate)(phys_addr_t paddr, size_t size);
> > > + void (*cache_clean)(phys_addr_t paddr, size_t size);
> > > + void (*cache_flush)(phys_addr_t paddr, size_t size);
> > > +};
> > I like the style like this than my previous patch which using
> > sbi_call. The c906 has custom instructions that could be called in
> > S-mode directly.
> >
>
> How are you going to include the custom instructions in the upstream kernel ?
In errata, call set_ops? I'm headache with that issue.
>
> > Hope the patch could be soon merged, after correct the
> > DMA_FROM/TO_DEVICE/BIDIRECTIONAL and alternatives ops_set.
> >
> > > +
> > > +void riscv_dma_cache_sync_set(struct riscv_dma_cache_sync *ops);
> > > +#endif
> > > +
> > > +#endif
> > > diff --git a/arch/riscv/mm/Makefile b/arch/riscv/mm/Makefile
> > > index 7ebaef10ea1b..959bef49098b 100644
> > > --- a/arch/riscv/mm/Makefile
> > > +++ b/arch/riscv/mm/Makefile
> > > @@ -27,3 +27,4 @@ KASAN_SANITIZE_init.o := n
> > > endif
> > >
> > > obj-$(CONFIG_DEBUG_VIRTUAL) += physaddr.o
> > > +obj-$(CONFIG_RISCV_DMA_NONCOHERENT) += dma-noncoherent.o
> > > diff --git a/arch/riscv/mm/dma-noncoherent.c
> > > b/arch/riscv/mm/dma-noncoherent.c
> > > new file mode 100644
> > > index ..2f6e9627c4aa
> > > --- /dev/null
> > > +++ b/arch/riscv/mm/dma-noncoherent.c
> > > @@ -0,0 +1,66 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * RISC-V specific functions to support DMA for non-coherent devices
> > > + *
> > > + * Copyright (c) 2021 Western Digital Corporation or its affiliates.
> > > + */
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +static struct riscv_dma_cache_sync *dma_cache_sync;
> > > +unsigned long riscv_dma_uc_offset;
> > > +
> > > +static void __dma_sync(phys_addr_t paddr, size_t size, enum
> > > dma_data_direction dir)
> > > +{
> > > + if ((dir == DMA_FROM_DEVICE) &&
> > > (dma_cache_sync->cache_invalidate))
> > > + dma_cache_sync->cache_invalidate(paddr, size);
> > > + else if ((dir == DMA_TO_DEVICE) && (dma_cache_sync->cache_clean))
> > > + dma_cache_sync->cache_clean(paddr, size);
> > > + else if ((dir == DMA_BIDIRECTIONAL) &&
> > > dma_cache_sync->cache_flush)
> > > + dma_cache_sync->cache_flush(paddr, size);
> > > +}
> > > +
> > > +void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, enum
> > > dma_data_direction dir)
> > > +{
> > > + if (!dma_cache_sync)
> > > + return;
> > > +
> > > + __dma_sync(paddr, size, dir);
> > > +}
> > > +
> > > +void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size, enum
> > > dma_data_direction dir)
> > > +{
> > > + if (!dma_cache_sync)
> > > + return;
> > > +
> > > + __dma_sync(paddr, size, dir);
> > > +}
> > > +
> > > +void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> > > + const struct iommu_ops *iommu, bool coherent)
> > > +{
> > > + /* If a specific device is dma-coherent, set it here */
> > > + dev->dma_coherent = coherent;
> > > +}
> > > +
> > > +void arch_dma_prep_coherent(struct page *page, size_t size)
> > > +{
> > > + void *flush_addr = page_address(page);
> > > +
> > > + memset(flush_addr, 0, size);
> > > + if (dma_cache_sync && dma_cache_sync->cache_flush)
> > > +
