| Message ID | 1456412174-20162-10-git-send-email-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote: > --- a/tools/firmware/hvmloader/hvmloader.c > +++ b/tools/firmware/hvmloader/hvmloader.c > @@ -303,6 +303,15 @@ int main(void) > > smp_initialise(); > > + /* Check that tests does not use memory where modules are stored */ > + BUG_ON( ((uint32_t)hvm_start_info + sizeof(struct hvm_start_info)) >= 4 << 20 ); > + for ( unsigned i = 0; i < hvm_start_info->nr_modules; i++ ) > + { > + const struct hvm_modlist_entry *modlist = > + (struct hvm_modlist_entry *)hvm_start_info->modlist_paddr; > + if ( modlist[i].size ) > + BUG_ON( modlist[i].paddr + modlist[i].size >= 4ul << 20 ); > + } First of all both checks should use > instead of >=. And then expecting all modules to live below 4Mb doesn't seem very reasonable. Jan
On Mon, Feb 29, 2016 at 09:58:29AM -0700, Jan Beulich wrote: > >>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote: > > --- a/tools/firmware/hvmloader/hvmloader.c > > +++ b/tools/firmware/hvmloader/hvmloader.c > > @@ -303,6 +303,15 @@ int main(void) > > > > smp_initialise(); > > > > + /* Check that tests does not use memory where modules are stored */ > > + BUG_ON( ((uint32_t)hvm_start_info + sizeof(struct hvm_start_info)) >= 4 << 20 ); > > + for ( unsigned i = 0; i < hvm_start_info->nr_modules; i++ ) > > + { > > + const struct hvm_modlist_entry *modlist = > > + (struct hvm_modlist_entry *)hvm_start_info->modlist_paddr; > > + if ( modlist[i].size ) > > + BUG_ON( modlist[i].paddr + modlist[i].size >= 4ul << 20 ); > > + } > > First of all both checks should use > instead of >=. And then > expecting all modules to live below 4Mb doesn't seem very > reasonable. Yes, and this can be an issue with OVMF, it's a 2MB binary. I'm not sure what to do about this, should I ignore perform_tests() if it going to clear the memory used by the modules? I should probably check that the modules are outside the memory region that is going to be used by perform_tests(), instead of just checking that they are bellow 4MB.
>>> On 03.03.16 at 17:00, <anthony.perard@citrix.com> wrote: > On Mon, Feb 29, 2016 at 09:58:29AM -0700, Jan Beulich wrote: >> >>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote: >> > --- a/tools/firmware/hvmloader/hvmloader.c >> > +++ b/tools/firmware/hvmloader/hvmloader.c >> > @@ -303,6 +303,15 @@ int main(void) >> > >> > smp_initialise(); >> > >> > + /* Check that tests does not use memory where modules are stored */ >> > + BUG_ON( ((uint32_t)hvm_start_info + sizeof(struct hvm_start_info)) >= 4 > << 20 ); >> > + for ( unsigned i = 0; i < hvm_start_info->nr_modules; i++ ) >> > + { >> > + const struct hvm_modlist_entry *modlist = >> > + (struct hvm_modlist_entry *)hvm_start_info->modlist_paddr; >> > + if ( modlist[i].size ) >> > + BUG_ON( modlist[i].paddr + modlist[i].size >= 4ul << 20 ); >> > + } >> >> First of all both checks should use > instead of >=. And then >> expecting all modules to live below 4Mb doesn't seem very >> reasonable. > > Yes, and this can be an issue with OVMF, it's a 2MB binary. I'm not sure > what to do about this, should I ignore perform_tests() if it going to > clear the memory used by the modules? Temporarily this might be an option, but ultimately the tests shouldn't assume a fixed memory location to be available. > I should probably check that the modules are outside the memory region that > is going to be used by perform_tests(), instead of just checking that they > are bellow 4MB. Yes. Jan
On 03/03/16 16:00, Anthony PERARD wrote: > On Mon, Feb 29, 2016 at 09:58:29AM -0700, Jan Beulich wrote: >>>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote: >>> --- a/tools/firmware/hvmloader/hvmloader.c >>> +++ b/tools/firmware/hvmloader/hvmloader.c >>> @@ -303,6 +303,15 @@ int main(void) >>> >>> smp_initialise(); >>> >>> + /* Check that tests does not use memory where modules are stored */ >>> + BUG_ON( ((uint32_t)hvm_start_info + sizeof(struct hvm_start_info)) >= 4 << 20 ); >>> + for ( unsigned i = 0; i < hvm_start_info->nr_modules; i++ ) >>> + { >>> + const struct hvm_modlist_entry *modlist = >>> + (struct hvm_modlist_entry *)hvm_start_info->modlist_paddr; >>> + if ( modlist[i].size ) >>> + BUG_ON( modlist[i].paddr + modlist[i].size >= 4ul << 20 ); >>> + } >> First of all both checks should use > instead of >=. And then >> expecting all modules to live below 4Mb doesn't seem very >> reasonable. > Yes, and this can be an issue with OVMF, it's a 2MB binary. I'm not sure > what to do about this, should I ignore perform_tests() if it going to > clear the memory used by the modules? > > I should probably check that the modules are outside the memory region that > is going to be used by perform_tests(), instead of just checking that they > are bellow 4MB. Now that I have published my Xen Test Framework, these hvmloader tests should be moved across and removed entirely from hvmloader. This was one of the original intentions. ~Andrew
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c index d319de0..a40503d 100644 --- a/tools/firmware/hvmloader/hvmloader.c +++ b/tools/firmware/hvmloader/hvmloader.c @@ -303,6 +303,15 @@ int main(void) smp_initialise(); + /* Check that tests does not use memory where modules are stored */ + BUG_ON( ((uint32_t)hvm_start_info + sizeof(struct hvm_start_info)) >= 4 << 20 ); + for ( unsigned i = 0; i < hvm_start_info->nr_modules; i++ ) + { + const struct hvm_modlist_entry *modlist = + (struct hvm_modlist_entry *)hvm_start_info->modlist_paddr; + if ( modlist[i].size ) + BUG_ON( modlist[i].paddr + modlist[i].size >= 4ul << 20 ); + } perform_tests(); if ( bios->bios_info_setup )
As perform_tests() is going to clear memory past 4MB, we check that the memory can be use. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- New in V3 --- tools/firmware/hvmloader/hvmloader.c | 9 +++++++++ 1 file changed, 9 insertions(+)