| Message ID | bf03f851-2fb4-3de1-7d72-b0ac15b2d488@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/shadow: sh_page_fault() adjustments | expand |
On 19/01/2023 1:19 pm, Jan Beulich wrote: > Clearly within the for_each_vcpu() the vCPU of this loop is meant, not > the (loop invariant) one the fault occurred on. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Wow that's been broken for the entire lifetime of the pagetable_dying op 0 3d5e6a3ff38 from 2010, but it still deserves a fixes tag. Preferably with that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > Quitle likely this mistake would have been avoided if the function scope > variable was named "curr", leaving "v" available for purposes likethe > one here. Doing the rename now would, however, be quite a bit of code > churn. Perhaps, but that pattern was far less prevalent back then, and the real cause of this bug is sh_page_fault() being far too big and sprawling. ~Andrew
On 19.01.2023 19:19, Andrew Cooper wrote: > On 19/01/2023 1:19 pm, Jan Beulich wrote: >> Clearly within the for_each_vcpu() the vCPU of this loop is meant, not >> the (loop invariant) one the fault occurred on. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Wow that's been broken for the entire lifetime of the pagetable_dying op > 0 3d5e6a3ff38 from 2010, but it still deserves a fixes tag. Oh, yes, of course. But then it'll really need two, as ef3b0d8d2c39 ("x86/shadow: shadow_table[] needs only one entry for PV-only configs") was also flawed, and I guess I really should have spotted the issue back then already. > Preferably with that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks. Jan
--- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -2672,10 +2672,10 @@ static int cf_check sh_page_fault( #if GUEST_PAGING_LEVELS == 3 unsigned int i; - for_each_shadow_table(v, i) + for_each_shadow_table(tmp, i) { mfn_t smfn = pagetable_get_mfn( - v->arch.paging.shadow.shadow_table[i]); + tmp->arch.paging.shadow.shadow_table[i]); if ( mfn_x(smfn) ) {
Clearly within the for_each_vcpu() the vCPU of this loop is meant, not the (loop invariant) one the fault occurred on. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Quitle likely this mistake would have been avoided if the function scope variable was named "curr", leaving "v" available for purposes likethe one here. Doing the rename now would, however, be quite a bit of code churn.