On 15/01/2021 16:12, Jan Beulich wrote: > On 12.01.2021 20:48, Andrew Cooper wrote: >> --- a/xen/arch/x86/mm.c >> +++ b/xen/arch/x86/mm.c >> @@ -4628,7 +4628,6 @@ int arch_acquire_resource(struct domain *d, unsigned >> int type, >> if ( id != (unsigned int)ioservid ) >> break; >> >> - rc = 0; >> for ( i = 0; i < nr_frames; i++ ) >> { >> mfn_t mfn; > How "good" are our chances that older gcc won't recognize that > without this initialization ... > >> @@ -4639,6 +4638,9 @@ int arch_acquire_resource(struct domain *d, unsigned >> int type, >> >> mfn_list[i] = mfn_x(mfn); >> } >> + if ( i == nr_frames ) >> + /* Success. Passed nr_frames back to the caller. */ >> + rc = nr_frames; > ... rc will nevertheless not remain uninitialized when nr_frames > is zero?
I don't anticipate this function getting complicated enough for us to need to rely on tricks like that to spot bugs. AFAICT, it would take a rather larger diffstat to make it "uninitialised clean" again. >> @@ -432,6 +419,28 @@ int compat_memory_op(unsigned int cmd, >> XEN_GUEST_HANDLE_PARAM(void) compat) >> >> if ( xen_frame_list && cmp.mar.nr_frames ) >> { >> + unsigned int xlat_max_frames = >> + (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.mar)) / >> + sizeof(*xen_frame_list); >> + >> + if ( start_extent >= nat.mar->nr_frames ) > Comparing with the enclosing if() I find it slightly confusing > that different instances of nr_frames get used. Perhaps best to > adjust the earlier patch to also use nat.mar->nr_frames? Or is > this perhaps on purpose? Good point - this is before the shuffle to avoid double accounting, and the code gen will be better by using cmp consistently. ~Andrew