On Wed, Jul 10, 2024 at 07:48:14AM +0000, Dexuan Cui wrote:
It's ok to me it will be after -rc1. I just thought the patch would get more testing if it could be on some branch (e.g., x86/tdx ?) in the tip.git tree, e.g., if the patch is on some tip.git branch, I suppose the linux-next tree would merge the patch so the patch will get more testing automatically.
Yes, it will get more testing automatically but the period is important: if I rush it now, it goes to Linus next week and then any fallout it causes needs to be dealt with in mainline.
If I queue it after -rc1, it'll be only in tip and linux-next for an additional 7 week cycle and I can always whack it if it breaks something. If it doesn't, I can send it mainline in the 6.12 merge window.
But we won't have to revert it mainline.
See the difference?
I guess we have different options on whether "the patch has changed substantially". My impression is that it hasn't.
If you're calling the difference between what I reverted and what you're sending now unsubstantial:
--- /tmp/old 2024-07-10 10:03:20.016629439 +0200 +++ /tmp/new 2024-07-10 10:02:23.696872729 +0200 diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c -index c1cb90369915..abf3cd591afd 100644 +index 078e2bac25531..8f471260924f7 100644 --- a/arch/x86/coco/tdx/tdx.c +++ b/arch/x86/coco/tdx/tdx.c -@@ -7,6 +7,7 @@ - #include <linux/cpufeature.h> +@@ -8,6 +8,7 @@ #include <linux/export.h> #include <linux/io.h> + #include <linux/kexec.h> +#include <linux/mm.h> #include <asm/coco.h> #include <asm/tdx.h> #include <asm/vmx.h> -@@ -778,6 +779,19 @@ static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc) +@@ -782,6 +783,19 @@ static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc) return false; }
@@ -53,7 +86,7 @@ index c1cb90369915..abf3cd591afd 100644 /* * Inform the VMM of the guest's intent for this physical page: shared with * the VMM or private to the guest. The VMM is expected to change its mapping -@@ -785,15 +799,22 @@ static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc) +@@ -789,15 +803,30 @@ static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc) */ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc) { @@ -63,23 +96,34 @@ index c1cb90369915..abf3cd591afd 100644 + unsigned long end = start + numpages * PAGE_SIZE; + unsigned long step = end - start; + unsigned long addr; - -- if (!tdx_map_gpa(start, end, enc)) -- return false; ++ + /* Step through page-by-page for vmalloc() mappings */ + if (is_vmalloc_addr((void *)vaddr)) + step = PAGE_SIZE; ++ ++ for (addr = start; addr < end; addr += step) { ++ phys_addr_t start_pa; ++ phys_addr_t end_pa; ++ ++ /* The check fails on vmalloc() mappings */ ++ if (virt_addr_valid(addr)) ++ start_pa = __pa(addr); ++ else ++ start_pa = slow_virt_to_phys((void *)addr); + +- if (!tdx_map_gpa(start, end, enc)) +- return false; ++ end_pa = start_pa + step;
- /* shared->private conversion requires memory to be accepted before use */ - if (enc) - return tdx_accept_memory(start, end); -+ for (addr = start; addr < end; addr += step) { -+ phys_addr_t start_pa = slow_virt_to_phys((void *)addr); -+ phys_addr_t end_pa = start_pa + step; -+ + if (!tdx_enc_status_changed_phys(start_pa, end_pa, enc)) + return false; + }
return true; }
especially for a patch which is already known to break things and where we're especially careful, then yes, we strongly disagree here.
So yes, it will definitely not go in now.
I started to add people's tags since v4 and my impression is that since then it's rebasing and minor changes.
When version N introduces changes like above in what is already non-trivial code, you drop all tags. And if people want to review it again, then they should give you those R-by tags.
Also, think about it: your patch broke a use case. How much are those R-by tags worth if the patch is broken? And why do you want to hold on to them so badly?
If a patch needs to be reverted because it breaks a use case, all reviewed and acked tags should simply be removed too. It is that simple.