[Qemu-devel] [PATCH 0/1] multiboot.c: Document as fixed against CVE-2018-7550

2018-03-21 Thread Jack Schwartz 
Some changes were delivered a few weeks ago that fixed CVE-2018-7550
before that CVE was created.  Changeset:
2a8fcd119eb7 ("multiboot: bss_end_addr can be zero")

Shortly after that delivery, a different changeset claimed in its commit
message to fix that CVE, but it really fixed a different one.  Changeset:
7cdc61becd09 ("vga: fix region calculation")
PJP says the proper CVE is CVE-2018-7858
https://bugzilla.redhat.com/show_bug.cgi?id=1553402

Not sure what the proper protocol is for getting this all straightened
out, but I would like to help since I delivered the multiboot changes.

This patch set contains an empty patch with a commit message to document
the multiboot changes.  Please add it to the repo as you see fit, or let
me know if you need something else from me to resolve this differently.

Note that unless the vga CVE is explained/corrected, people may get
confused when they see different fixes associated with the same CVE.

Thanks,
    Jack

Jack Schwartz (1):
  multiboot.c: Document as fixed against CVE-2018-7550

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/1] multiboot.c: Document as fixed against CVE-2018-7550

2018-03-21 Thread Jack Schwartz 
This empty commit documents multiboot.c as fixed against CVE-2018-7550.
The fix, dated Dec 21 2017, went in before the CVE was created, as:
2a8fcd119eb7 ("multiboot: bss_end_addr can be zero")

Fixes: CVE-2018-7550
Signed-off-by: Jack Schwartz <jack.schwa...@oracle.com>
Reviewed-by: Mark Kanda <mark.ka...@oracle.com>
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/5] multiboot: Reject kernels exceeding the address space

2018-03-15 Thread Jack Schwartz 

On 03/15/18 10:18, Kevin Wolf wrote:

Am 15.03.2018 um 17:55 hat Jack Schwartz geschrieben:

On 03/15/18 08:54, Kevin Wolf wrote:

Am 15.03.2018 um 06:19 hat Jack Schwartz geschrieben:

Hi Kevin.

My comments are inline...

On 2018-03-14 10:32, Kevin Wolf wrote:

The code path with a manually set mh_load_addr in the Multiboot header
checks that load_end_addr <= load_addr, but the path where load_end_addr
is automatically detected if 0 is given in the header misses the
corresponding check.

1) The code checks that load_end_addr >= load_addr (before letting it
through).

2) load_end_addr is used only when it is read in as non-zero, so no check is
needed if zero.  (It gets debug-printed even when zero, but is used only to
calculate mb_load_size and only when non-zero.)

Oops, good point. I'll change the start of the commit message as follows:

  The code path with a manually set mh_load_end_addr in the Multiboot
  header checks that mh_load_end_addr >= mh_load_addr, but the path where
  mh_load_end_addr is 0 in the header and therefore automatically
  calculated from the file size misses the corresponding check.

Does this look better?

mb_load_size is calculated from the file size, not mh_load_end_addr, so
I think you mean mb_load_size rather than mh_load_end_addr.  Do you intend
to say:

   The code path where mh_load_end_addr is non-zero in the Multiboot
   header checks that mh_load_end_addr >= mh_load_addr and so
   mb_load_size is checked.  However, mb_load_size is not checked when
   calculated from the file size, when mh_load_end_addr is 0.

Ok, technically that's more accurate.
OK.  Note that I fixed a couple of missing spaces if you decide to use 
it...

Also, if this is what you intend to say, would the following code change be
more ofwhat you want:

Remove this:

     mb_load_size = kernel_file_size - mb_kernel_text_offset;
     }
-   if (mb_load_size > UINT32_MAX - mh_load_addr) {
-   error_report("kernel does not fit in address space");
-   exit(1);
-   }
     if (mh_bss_end_addr) {

and instead do this a few lines further down:

    mb_kernel_size = mh_bss_end_addr - mh_load_addr;
     } else {
     mb_kernel_size = mb_load_size;
     }

+   if (mb_kernel_size > UINT32_MAX - mh_load_addr) {
+   error_report("kernel does not fit in address space");
+   exit(1);
+   }

     mb_debug("multiboot: header_addr = %#x", mh_header_addr);
     mb_debug("multiboot: load_addr = %#x", mh_load_addr);

The reason would be to include the bss area in the calculation, when
mh_bss_end_addr is non-zero.

Ultimately, mb_load_size > mb_kernel_size is what kills us,

Right.

  so maybe we
could add an assertion for that.

But the reason why this condition could ever be true is the integer
overflow in this line:

 if (mh_bss_end_addr < (mh_load_addr + mb_load_size))

It's generally better to check for integer overflows before they happen
than trying to infer them from the result. In fact, your condition
wouldn't catch the error of test scenario 9:

 kernel_file_size= 0x2035
 mh_header_addr  = 0xf000
 mh_load_addr= 0xf000
 mh_load_end_addr= 0
 mh_bss_end_addr = 0xf001

 mb_kernel_text_offset   = i - (mh_header_addr - mh_load_addr)
 = 0

 mb_load_size= kernel_file_size - mb_kernel_text_offset
 = 0x2035
 > UINT32_MAX - mh_load_addr

 mb_kernel_size  = mh_bss_end_addr - mh_load_addr
 = 1
 < UINT32_MAX - mh_load_addr

OK, fair enough...

One other suggestion, would be to move what you added up by one line, 
into the "else" clause.  This moves the check to run only where it is 
needed.  This is cleaner as it does the check only where mb_load_size is 
based on the kernel_file_size.


Reason: it doesn't make sense to do it for the "if" clause as it will 
never be true anyway:

  if (mh_load_end_addr) {
   ...
   mb_load_size = mh_load_end_addr - mh_load_addr;
       if (mb_load_size > UINT32_MAX - mh_load_addr) {
                bomb();

    Thanks,
    Jack


Kevin

Re: [Qemu-devel] [PATCH 1/5] multiboot: Reject kernels exceeding the address space

2018-03-15 Thread Jack Schwartz 

On 03/15/18 08:54, Kevin Wolf wrote:

Am 15.03.2018 um 06:19 hat Jack Schwartz geschrieben:

Hi Kevin.

My comments are inline...

On 2018-03-14 10:32, Kevin Wolf wrote:

The code path with a manually set mh_load_addr in the Multiboot header
checks that load_end_addr <= load_addr, but the path where load_end_addr
is automatically detected if 0 is given in the header misses the
corresponding check.

1) The code checks that load_end_addr >= load_addr (before letting it
through).

2) load_end_addr is used only when it is read in as non-zero, so no check is
needed if zero.  (It gets debug-printed even when zero, but is used only to
calculate mb_load_size and only when non-zero.)

Oops, good point. I'll change the start of the commit message as follows:

 The code path with a manually set mh_load_end_addr in the Multiboot
 header checks that mh_load_end_addr >= mh_load_addr, but the path where
 mh_load_end_addr is 0 in the header and therefore automatically
 calculated from the file size misses the corresponding check.

Does this look better?


mb_load_size is calculated from the file size, not mh_load_end_addr, so
I think you mean mb_load_size rather than mh_load_end_addr.  Do you 
intend to say:


  The code path where mh_load_end_addr is non-zero in the Multiboot
  header checks that mh_load_end_addr >= mh_load_addr and so
  mb_load_size ischecked.  However, mb_load_size is not checked when
  calculated from thefile size, when mh_load_end_addr is 0.

Also, if this is what you intend to say, would the following code change 
be more ofwhat you want:


Remove this:

    mb_load_size = kernel_file_size - mb_kernel_text_offset;
    }
-   if (mb_load_size > UINT32_MAX - mh_load_addr) {
-   error_report("kernel does not fit in address space");
-   exit(1);
-   }
    if (mh_bss_end_addr) {

and instead do this a few lines further down:

   mb_kernel_size = mh_bss_end_addr - mh_load_addr;
    } else {
    mb_kernel_size = mb_load_size;
    }

+   if (mb_kernel_size > UINT32_MAX - mh_load_addr) {
+   error_report("kernel does not fit in address space");
+   exit(1);
+   }

    mb_debug("multiboot: header_addr = %#x", mh_header_addr);
    mb_debug("multiboot: load_addr = %#x", mh_load_addr);

The reason would be to include the bss area in the calculation, when
mh_bss_end_addr is non-zero.

    Thanks,
    Jack




If the kernel binary size is larger than can fit in
the address space after load_addr, we ended up with a kernel_size that
is smaller than load_size, which means that we read the file into a too
small buffer.

Add a check to reject kernel files with such Multiboot headers.

Code itself looks fine.

Modulo above comments:
     Reviewed-by: Jack Schwartz <jack.schwa...@oracle.com>

Thanks for your review of the series!

Kevin

Re: [Qemu-devel] [PATCH 1/5] multiboot: Reject kernels exceeding the address space

2018-03-14 Thread Jack Schwartz 

Hi Kevin.

My comments are inline...

On 2018-03-14 10:32, Kevin Wolf wrote:

The code path with a manually set mh_load_addr in the Multiboot header
checks that load_end_addr <= load_addr, but the path where load_end_addr
is automatically detected if 0 is given in the header misses the
corresponding check.

1) The code checks that load_end_addr >= load_addr (before letting it
through).

2) load_end_addr is used only when it is read in as non-zero, so no 
check is needed if zero.  (It gets debug-printed even when zero, but is 
used only to calculate mb_load_size and only when non-zero.)

If the kernel binary size is larger than can fit in
the address space after load_addr, we ended up with a kernel_size that
is smaller than load_size, which means that we read the file into a too
small buffer.

Add a check to reject kernel files with such Multiboot headers.

Code itself looks fine.

Modulo above comments:
    Reviewed-by: Jack Schwartz <jack.schwa...@oracle.com>

    Thanks,
    Jack


Signed-off-by: Kevin Wolf <kw...@redhat.com>
---
  hw/i386/multiboot.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index b9064264d8..1e215bf8d3 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -247,6 +247,10 @@ int load_multiboot(FWCfgState *fw_cfg,
  }
  mb_load_size = kernel_file_size - mb_kernel_text_offset;
  }
+if (mb_load_size > UINT32_MAX - mh_load_addr) {
+error_report("kernel does not fit in address space");
+exit(1);
+}
  if (mh_bss_end_addr) {
  if (mh_bss_end_addr < (mh_load_addr + mb_load_size)) {
  error_report("invalid bss_end_addr address");

Re: [Qemu-devel] [PATCH 0/5] multiboot: Fix buffer overflow on invalid kernels

2018-03-14 Thread Jack Schwartz 

Hi Kevin.

I see an issue with the commit message of patch 1; please see my reply 
to that patch for details.  I fully understand patches 1,2,3, patch 4 
except for some of the Makefile black magic, and patch 5 looks 
reasonable to me.


So, for patches 2,3,4,5:
    Reviewed-by: Jack Schwartz <jack.schwa...@oracle.com>

    Thanks,
    Jack

On 2018-03-14 10:32, Kevin Wolf wrote:

Patch 1 fixes another Multiboot kernel validation bug that could cause
QEMU to load the kernel image file into a too small buffer. Patch 2 adds
another check to harden the code. The rest of the series adds Multiboot
test cases for kernels using the a.out kludge, which is where the recent
bugs were found.

Kevin Wolf (5):
   multiboot: Reject kernels exceeding the address space
   multiboot: Check validity of mh_header_addr
   tests/multiboot: Test exit code for every qemu run
   tests/multiboot: Add tests for the a.out kludge
   tests/multiboot: Add .gitignore

  hw/i386/multiboot.c |   8 +++
  tests/multiboot/.gitignore  |   3 +
  tests/multiboot/Makefile|  22 +--
  tests/multiboot/aout_kludge.S   | 138 
  tests/multiboot/aout_kludge.out |  42 
  tests/multiboot/run_test.sh |  34 ++
  6 files changed, 227 insertions(+), 20 deletions(-)
  create mode 100644 tests/multiboot/.gitignore
  create mode 100644 tests/multiboot/aout_kludge.S
  create mode 100644 tests/multiboot/aout_kludge.out

Re: [Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

2018-03-06 Thread Jack Schwartz 

Hi Kevin and everyone.

On 2018-03-05 00:13, Kevin Wolf wrote:

Am 02.03.2018 um 20:32 hat Jack Schwartz geschrieben:

Hi Kevin.

On 2018-01-15 07:54, Kevin Wolf wrote:

Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:

Properly account for the possibility of multiboot kernels with a zero
bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
kernels without a bss section, by allowing a zeroed bss_end_addr multiboot
header field.

Do some cleanup to multiboot.c as well:
- Remove some unused variables.
- Use more intuitive header names when displaying fields in messages.
- Change fprintf(stderr...) to error_report

There are some conflicts with Anatol's (CCed) multiboot series:
https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html

None if these should be hard to resolve, but it would be good if you
could agree with each other whose patch series should come first, and
then the other one should be rebased on top of that.


Testing:
1) Ran the "make check" test suite.
2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
   grub multiboot.elf test "kernel" by modifying source.)  Verified
   with gdb that new code that reads addresses/offsets from multiboot
   header was accessed.
3) Booted multiboot kernel with non-zero bss_end_addr.
4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked.
5) Code has soaked in an internal repo for two months.

Can you integrate your test kernel from 2) in tests/multiboot/ so we can
keep this as a regression test?

If need be, would you be willing to accept updated versions of these patches
(with another review, of course) without the test file?  I will deliver the
test file later once I get company approvals.  I don't want the test file to
continue holding everything up in the meantime.

Sure, let's move forward with what we have now. Please keep me CCed when
you send a new version and I'll give it a review and hopeuflly get it
merged.

Kevin

Thanks, Kevin.

Patches have not changed, and I verified they still work on a current 
repo.  (Multiboot.c has had a one-line change regarding a header file, 
so I rebuilt and re-tested to make sure.)


Links again, for your reference:

1/4 multiboot: bss_end_addr can be zero
http://patchwork.ozlabs.org/patch/852049/

2/4 multiboot: Remove unused variables from multiboot.c
http://patchwork.ozlabs.org/patch/852045/

3/4 multiboot: Use header names when displaying fields
http://patchwork.ozlabs.org/patch/852046/

4/4 multiboot: fprintf(stderr...) -> error_report()
http://patchwork.ozlabs.org/patch/852051/

    Thanks,
    Jack

Re: [Qemu-devel] [PATCH] multiboot: check mh_load_end_addr address field

2018-03-06 Thread Jack Schwartz 

Hi Prasad.

The Multiboot Spec will allow for a zero bss end address.  (Please see 
section 3.1.3 at https://www.gnu.org/software/grub/manual/multiboot/  . 
) For a zero bss end address, this patch will not do the right thing.


I had proposed some patches to properly handle zero bss end address, but 
they got delayed.  However, I will be re-sending updated patches out 
later today.  Please stay tuned...


    Thanks,
    Jack

On 02/27/18 11:48, P J P wrote:

From: Prasad J Pandit 

While loading kernel via multiboot-v1 image, (flags & 0x0001)
indicates that multiboot header contains valid addresses to load
the kernel image. In that, end of the data segment address
'mh_load_end_addr' should be less than the bss segment address
'mh_bss_end_addr'. Add check to validate that.

Reported-by: CERT CC 
Signed-off-by: Prasad J Pandit 
---
  hw/i386/multiboot.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 46d9c68bf5..d16e32bf4a 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -227,6 +227,10 @@ int load_multiboot(FWCfgState *fw_cfg,
  fprintf(stderr, "invalid mh_load_addr address\n");
  exit(1);
  }
+if (mh_load_end_addr > mh_bss_end_addr) {
+fprintf(stderr, "invalid mh_load_end_addr address\n");
+exit(1);
+}
  
  uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);

  uint32_t mb_load_size = 0;

Re: [Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

2018-03-02 Thread Jack Schwartz 

Hi Kevin.

On 2018-01-15 07:54, Kevin Wolf wrote:

Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:

Properly account for the possibility of multiboot kernels with a zero
bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
kernels without a bss section, by allowing a zeroed bss_end_addr multiboot
header field.

Do some cleanup to multiboot.c as well:
- Remove some unused variables.
- Use more intuitive header names when displaying fields in messages.
- Change fprintf(stderr...) to error_report

There are some conflicts with Anatol's (CCed) multiboot series:
https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html

None if these should be hard to resolve, but it would be good if you
could agree with each other whose patch series should come first, and
then the other one should be rebased on top of that.


Testing:
   1) Ran the "make check" test suite.
   2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
  grub multiboot.elf test "kernel" by modifying source.)  Verified
  with gdb that new code that reads addresses/offsets from multiboot
  header was accessed.
   3) Booted multiboot kernel with non-zero bss_end_addr.
   4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked.
   5) Code has soaked in an internal repo for two months.

Can you integrate your test kernel from 2) in tests/multiboot/ so we can
keep this as a regression test?
If need be, would you be willing to accept updated versions of these 
patches (with another review, of course) without the test file?  I will 
deliver the test file later once I get company approvals.  I don't want 
the test file to continue holding everything up in the meantime.


Patchwork links, for reference:

1/4 multiboot: bss_end_addr can be zero
http://patchwork.ozlabs.org/patch/852049/

2/4 multiboot: Remove unused variables from multiboot.c
http://patchwork.ozlabs.org/patch/852045/

3/4 multiboot: Use header names when displaying fields
http://patchwork.ozlabs.org/patch/852046/

4/4 multiboot: fprintf(stderr...) -> error_report()
http://patchwork.ozlabs.org/patch/852051/


    Thanks,
    Jack



Jack Schwartz (4):
   multiboot: bss_end_addr can be zero
   multiboot: Remove unused variables from multiboot.c
   multiboot: Use header names when displaying fields
   multiboot: fprintf(stderr...) -> error_report()

Apart from the conflicts, the patches look good to me.

Kevin

Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct

2018-02-06 Thread Jack Schwartz 

Hi Anatol and Kevin.

Kevin and Anatol, thanks for your replies.

A few comments inline close to the bottom...

On 2018-02-05 13:43, Anatol Pomozov wrote:

Hi

On Wed, Jan 31, 2018 at 1:12 AM, Kevin Wolf <kw...@redhat.com> wrote:

Am 31.01.2018 um 00:15 hat Jack Schwartz geschrieben:

Hi Anatol and Kevin.

Even though I am new to Qemu, I have a patch to deliver for
multiboot.c (as you know) and so I feel familiar enough to do a review
of this patch.  One comment is probably more for maintainers.

The Multiboot code is essentially unmaintained. It's technically part of
the PC/x86 subsystem, but their maintainers don't seem to care at all.
So in order to make any progress here, I decided that I will send a
pull request for Multiboot once we have the patches ready (as a one-time
thing, without officially making myself the maintainer).

So I am the closest thing to a maintainer that we have here, and while
I'm familiar with some of the Multiboot-specific code, I don't really
know the ELF code and don't have a lot of time to spend here.

Therefore it's very welcome if you review the patches of each other,
even if you're not perfectly familiar with the code, as there is
probably noone else who could do a better review.


On 01/29/18 12:43, Anatol Pomozov wrote:

@@ -253,11 +228,11 @@ int load_multiboot(FWCfgState *fw_cfg,
   mb_load_size = mb_kernel_size;
   }
-/* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
-uint32_t mh_mode_type = ldl_p(header+i+32);
-uint32_t mh_width = ldl_p(header+i+36);
-uint32_t mh_height = ldl_p(header+i+40);
-uint32_t mh_depth = ldl_p(header+i+44); */
+/* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
+uint32_t mh_mode_type = ldl_p(_header->mode_type);
+uint32_t mh_width = ldl_p(_header->width);
+uint32_t mh_height = ldl_p(_header->height);
+uint32_t mh_depth = ldl_p(_header->depth); */

This question is probably more for maintainers...

In the patch series I submitted, people were OK that I was going to delete
these lines since they were only comments anyway.  Your approach leaves
these lines in and updates them even though they are comments.  Which way is
preferred?

As far as I am concerned, I honestly don't mind either way. It's
trivial code, so we won't lose anything by removing it.

This change suppose to be a refactoring and tries to avoid functional
changes. I can remove it in a separate change.


The ideal solution would be just implementing support for it, of course.
If we wanted to do that, I think we would have to pass the values
through fw_cfg and then set the VBE mode in the Mutiboot option rom.


   mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
   mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
@@ -295,14 +270,15 @@ int load_multiboot(FWCfgState *fw_cfg,
   }
   mbs.mb_buf_size += cmdline_len;
-mbs.mb_buf_size += MB_MOD_SIZE * mbs.mb_mods_avail;
+mbs.mb_buf_size += sizeof(multiboot_module_t) * mbs.mb_mods_avail;
   mbs.mb_buf_size += strlen(bootloader_name) + 1;
   mbs.mb_buf_size = TARGET_PAGE_ALIGN(mbs.mb_buf_size);
   /* enlarge mb_buf to hold cmdlines, bootloader, mb-info structs */
   mbs.mb_buf= g_realloc(mbs.mb_buf, mbs.mb_buf_size);
-mbs.offset_cmdlines   = mbs.offset_mbinfo + mbs.mb_mods_avail * 
MB_MOD_SIZE;
+mbs.offset_cmdlines   = mbs.offset_mbinfo +
+mbs.mb_mods_avail * sizeof(multiboot_module_t);
   mbs.offset_bootloader = mbs.offset_cmdlines + cmdline_len;
   if (initrd_filename) {
@@ -348,22 +324,22 @@ int load_multiboot(FWCfgState *fw_cfg,
   char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2];
   snprintf(kcmdline, sizeof(kcmdline), "%s %s",
kernel_filename, kernel_cmdline);
-stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(, kcmdline));
+stl_p(, mb_add_cmdline(, kcmdline));
-stl_p(bootinfo + MBI_BOOTLOADER, mb_add_bootloader(, bootloader_name));
+stl_p(_loader_name, mb_add_bootloader(, 
bootloader_name));
-stl_p(bootinfo + MBI_MODS_ADDR,  mbs.mb_buf_phys + mbs.offset_mbinfo);
-stl_p(bootinfo + MBI_MODS_COUNT, mbs.mb_mods_count); /* mods_count */
+stl_p(_addr,  mbs.mb_buf_phys + mbs.offset_mbinfo);
+stl_p(_count, mbs.mb_mods_count); /* mods_count */
   /* the kernel is where we want it to be now */
-stl_p(bootinfo + MBI_FLAGS, MULTIBOOT_FLAGS_MEMORY
-| MULTIBOOT_FLAGS_BOOT_DEVICE
-| MULTIBOOT_FLAGS_CMDLINE
-| MULTIBOOT_FLAGS_MODULES
-| MULTIBOOT_FLAGS_MMAP
-| MULTIBOOT_FLAGS_BOOTLOADER);
-stl_p(bootinfo + MBI_BOOT_DEVICE, 0x8000); /* XXX: use the -boot 
switch? */
-stl_p(bootinfo + MBI_MMAP_ADDR,   AD

Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct

2018-01-30 Thread Jack Schwartz 

Hi Anatol and Kevin.

Even though I am new to Qemu, I have a patch to deliver for multiboot.c 
(as you know) and so I feel familiar enough to do a review of this 
patch.  One comment is probably more for maintainers.


Comments inline...


On 01/29/18 12:43, Anatol Pomozov wrote:

Using C structs makes the code more readable and prevents type conversion
errors.

Borrow multiboot1 header from GRUB project.

Signed-off-by: Anatol Pomozov 
---
  hw/i386/multiboot.c | 124 +
  hw/i386/multiboot_header.h  | 254 
  tests/multiboot/mmap.c  |  14 +--
  tests/multiboot/mmap.out|  10 ++
  tests/multiboot/modules.c   |  12 ++-
  tests/multiboot/modules.out |  10 ++
  tests/multiboot/multiboot.h |  66 
  7 files changed, 339 insertions(+), 151 deletions(-)
  create mode 100644 hw/i386/multiboot_header.h
  delete mode 100644 tests/multiboot/multiboot.h

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index c7b70c91d5..c6d05ca46b 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -28,6 +28,7 @@
  #include "hw/hw.h"
  #include "hw/nvram/fw_cfg.h"
  #include "multiboot.h"
+#include "multiboot_header.h"
  #include "hw/loader.h"
  #include "elf.h"
  #include "sysemu/sysemu.h"
@@ -47,39 +48,9 @@
  #error multiboot struct needs to fit in 16 bit real mode
  #endif
  
-enum {

-/* Multiboot info */
-MBI_FLAGS   = 0,
-MBI_MEM_LOWER   = 4,
-MBI_MEM_UPPER   = 8,
-MBI_BOOT_DEVICE = 12,
-MBI_CMDLINE = 16,
-MBI_MODS_COUNT  = 20,
-MBI_MODS_ADDR   = 24,
-MBI_MMAP_ADDR   = 48,
-MBI_BOOTLOADER  = 64,
-
-MBI_SIZE= 88,
-
-/* Multiboot modules */
-MB_MOD_START= 0,
-MB_MOD_END  = 4,
-MB_MOD_CMDLINE  = 8,
-
-MB_MOD_SIZE = 16,
-
-/* Region offsets */
-ADDR_E820_MAP = MULTIBOOT_STRUCT_ADDR + 0,
-ADDR_MBI  = ADDR_E820_MAP + 0x500,
-
-/* Multiboot flags */
-MULTIBOOT_FLAGS_MEMORY  = 1 << 0,
-MULTIBOOT_FLAGS_BOOT_DEVICE = 1 << 1,
-MULTIBOOT_FLAGS_CMDLINE = 1 << 2,
-MULTIBOOT_FLAGS_MODULES = 1 << 3,
-MULTIBOOT_FLAGS_MMAP= 1 << 6,
-MULTIBOOT_FLAGS_BOOTLOADER  = 1 << 9,
-};
+/* Region offsets */
+#define ADDR_E820_MAP MULTIBOOT_STRUCT_ADDR
+#define ADDR_MBI  (ADDR_E820_MAP + 0x500)
  
  typedef struct {

  /* buffer holding kernel, cmdlines and mb_infos */
@@ -128,14 +99,15 @@ static void mb_add_mod(MultibootState *s,
 hwaddr start, hwaddr end,
 hwaddr cmdline_phys)
  {
-char *p;
+multiboot_module_t *mod;
  assert(s->mb_mods_count < s->mb_mods_avail);
  
-p = (char *)s->mb_buf + s->offset_mbinfo + MB_MOD_SIZE * s->mb_mods_count;

+mod = s->mb_buf + s->offset_mbinfo +
+  sizeof(multiboot_module_t) * s->mb_mods_count;
  
-stl_p(p + MB_MOD_START,   start);

-stl_p(p + MB_MOD_END, end);
-stl_p(p + MB_MOD_CMDLINE, cmdline_phys);
+stl_p(>mod_start, start);
+stl_p(>mod_end,   end);
+stl_p(>cmdline,   cmdline_phys);
  
  mb_debug("mod%02d: "TARGET_FMT_plx" - "TARGET_FMT_plx"\n",

   s->mb_mods_count, start, end);
@@ -151,26 +123,29 @@ int load_multiboot(FWCfgState *fw_cfg,
 int kernel_file_size,
 uint8_t *header)
  {
-int i, is_multiboot = 0;
+int i;
+bool is_multiboot = false;
  uint32_t flags = 0;
  uint32_t mh_entry_addr;
  uint32_t mh_load_addr;
  uint32_t mb_kernel_size;
  MultibootState mbs;
-uint8_t bootinfo[MBI_SIZE];
+multiboot_info_t bootinfo;
  uint8_t *mb_bootinfo_data;
  uint32_t cmdline_len;
+struct multiboot_header *multiboot_header;
  
  /* Ok, let's see if it is a multiboot image.

 The header is 12x32bit long, so the latest entry may be 8192 - 48. */
  for (i = 0; i < (8192 - 48); i += 4) {
-if (ldl_p(header+i) == 0x1BADB002) {
-uint32_t checksum = ldl_p(header+i+8);
-flags = ldl_p(header+i+4);
+multiboot_header = (struct multiboot_header *)(header + i);
+if (ldl_p(_header->magic) == MULTIBOOT_HEADER_MAGIC) {
+uint32_t checksum = ldl_p(_header->checksum);
+flags = ldl_p(_header->flags);
  checksum += flags;
-checksum += (uint32_t)0x1BADB002;
+checksum += (uint32_t)MULTIBOOT_HEADER_MAGIC;
  if (!checksum) {
-is_multiboot = 1;
+is_multiboot = true;
  break;
  }
  }
@@ -180,13 +155,13 @@ int load_multiboot(FWCfgState *fw_cfg,
  return 0; /* no multiboot */
  
  mb_debug("qemu: I believe we found a multiboot image!\n");

-memset(bootinfo, 0, sizeof(bootinfo));
+memset(, 0, sizeof(bootinfo));
  memset(, 0, sizeof(mbs));
  
-if (flags & 0x0004) { /* MULTIBOOT_HEADER_HAS_VBE */

+if (flags &

Re: [Qemu-devel] [PATCH 2/4] multiboot: load elf sections and section headers

2018-01-30 Thread Jack Schwartz 

Hi Anatol.

I have one comment about sections.c headers (and didn't really look 
further in that file), and start.S.


Comments inline...

On 01/29/18 12:43, Anatol Pomozov wrote:

Multiboot may load section headers and all sections (even those that are
not part of any segment) to target memory.

Tested with an ELF application that uses data from strings table
section.

Signed-off-by: Anatol Pomozov 
---
  hw/core/loader.c |   8 +--
  hw/i386/multiboot.c  |  17 +++--
  hw/s390x/ipl.c   |   2 +-
  include/hw/elf_ops.h | 110 +--
  include/hw/loader.h  |  11 +++-
  tests/multiboot/Makefile |   8 ++-
  tests/multiboot/generate_sections_out.py |  33 ++
  tests/multiboot/modules.out  |  22 +++
  tests/multiboot/run_test.sh  |   6 +-
  tests/multiboot/sections.c   |  57 
  tests/multiboot/start.S  |   2 +-
  11 files changed, 248 insertions(+), 28 deletions(-)
  create mode 100755 tests/multiboot/generate_sections_out.py
  create mode 100644 tests/multiboot/sections.c




diff --git a/tests/multiboot/sections.c b/tests/multiboot/sections.c
new file mode 100644
index 00..64060510ce
--- /dev/null
+++ b/tests/multiboot/sections.c
@@ -0,0 +1,57 @@
+/*
+ * Copyright (c) 2017 Anatol Pomozov 
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "libc.h"
+#include "../../hw/i386/multiboot_header.h"
As like the first patch, I suggest creating a "multiboot" subdir in the 
"include" subtree and putting all multiboot header files there. 
Including "multiboot/multiboot_header.h" would be cleaner.

+#include "../../include/elf.h"
+
+int test_main(uint32_t magic, struct multiboot_info *mbi)
+{
+void *p;
+unsigned int i;
+
+(void) magic;
+multiboot_elf_section_header_table_t shdr;
+
+printf("Multiboot header at %x, ELF section headers %s\n\n", mbi,
+mbi->flags & MULTIBOOT_INFO_ELF_SHDR ? "present" : "doesn't present");
+
+shdr = mbi->u.elf_sec;
+printf("Sections list with %d entries of size %d at %x, string index %d\n",
+shdr.num, shdr.size, shdr.addr, shdr.shndx);
+
+const char *string_table = (char *)((Elf32_Shdr *)(uintptr_t)(shdr.addr +
+shdr.shndx * shdr.size))->sh_addr;
+
+for (i = 0, p = (void *)shdr.addr;
+ i < shdr.num;
+ i++, p += shdr.size)
+{
+Elf32_Shdr *sec;
+
+sec = (Elf32_Shdr *)p;
+printf("Elf section name=%s addr=%lx size=%ld\n",
+string_table + sec->sh_name, sec->sh_addr, sec->sh_size);
+}
+
+return 0;
+}
diff --git a/tests/multiboot/start.S b/tests/multiboot/start.S
index 7d33959650..bd404100c2 100644
--- a/tests/multiboot/start.S
+++ b/tests/multiboot/start.S
@@ -23,7 +23,7 @@
  .section multiboot
  
  #define MB_MAGIC 0x1badb002

-#define MB_FLAGS 0x0
+#define MB_FLAGS 0x2
  #define MB_CHECKSUM -(MB_MAGIC + MB_FLAGS)
C headers can be included in assembly files.  Since you are bringing 
over multiboot_header.h, why not include it and use its values instead 
of re-defining them here?


    Thanks,
    Jack
  
  .align  4

Re: [Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

2018-01-19 Thread Jack Schwartz 

Hi Anatol, Daniel and Kevin.

On 01/19/18 10:36, Anatol Pomozov wrote:

Hello Jack

On Wed, Jan 17, 2018 at 12:06 PM, Jack Schwartz
<jack.schwa...@oracle.com> wrote:

Hi Kevin and Anatol.

Kevin, thanks for your review.

More inline below...

On 01/15/18 07:54, Kevin Wolf wrote:

Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:

Properly account for the possibility of multiboot kernels with a zero
bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
kernels without a bss section, by allowing a zeroed bss_end_addr
multiboot
header field.

Do some cleanup to multiboot.c as well:
- Remove some unused variables.
- Use more intuitive header names when displaying fields in messages.
- Change fprintf(stderr...) to error_report

There are some conflicts with Anatol's (CCed) multiboot series:
https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html

None if these should be hard to resolve, but it would be good if you
could agree with each other whose patch series should come first, and
then the other one should be rebased on top of that.

Anatol,

from my side, there are pros and cons to either patch set going in first,
but advantages to either are pretty negligible.  Pro for you going first: I
can use the constants you will define in header files.  Pro for me going
first: your merge should be about the same as if you went first (since my
changes are small, more localized and affect only multiboot.c) and my merge
will be easier.

What are your thoughts?

Please move ahead with your patches. I'll rebase my changes on top of yours.
OK.  I'm consulting with my company's legal department and waiting for 
their approvals for delivery of a test "kernel".  I'll get in touch will 
everyone once I have an answer about that.  I anticipate about a week 
before taking next steps to deliver.


Kevin and Daniel, thanks for your inputs on this issue (different 
subthread), which I have forwarded to our legal department for review.


    Thanks,
    Jack



Testing:
1) Ran the "make check" test suite.
2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
   grub multiboot.elf test "kernel" by modifying source.)  Verified
   with gdb that new code that reads addresses/offsets from multiboot
   header was accessed.
3) Booted multiboot kernel with non-zero bss_end_addr.
4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages
worked.
5) Code has soaked in an internal repo for two months.
Can you integrate your test kernel from 2) in tests/multiboot/ so we can
keep this as a regression test?

Kevin and alias,

Before I proceed with adding my multiboot test file, I'll clarify here that
I started with a version from the grub2 tree.  In that file I expanded a
header file, also from the same tree.  Neither file had any license header,
though the tree I got them from (Dated October 2017) contains the GNU GPLv3
license file.

I'll need to check with my company before I can say I can deliver this file.
If I deliver it, I'll add a header stating the GPL license, that it came
from grub2 and to check its repository for contributors.

Jack Schwartz (4):
multiboot: bss_end_addr can be zero
multiboot: Remove unused variables from multiboot.c
multiboot: Use header names when displaying fields
multiboot: fprintf(stderr...) -> error_report()

Apart from the conflicts, the patches look good to me.

 Thanks,
 Jack


Kevin

Re: [Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

2018-01-17 Thread Jack Schwartz 

Hi Kevin and Anatol.

Kevin, thanks for your review.

More inline below...

On 01/15/18 07:54, Kevin Wolf wrote:

Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:

Properly account for the possibility of multiboot kernels with a zero
bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
kernels without a bss section, by allowing a zeroed bss_end_addr multiboot
header field.

Do some cleanup to multiboot.c as well:
- Remove some unused variables.
- Use more intuitive header names when displaying fields in messages.
- Change fprintf(stderr...) to error_report

There are some conflicts with Anatol's (CCed) multiboot series:
https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html

None if these should be hard to resolve, but it would be good if you
could agree with each other whose patch series should come first, and
then the other one should be rebased on top of that.

Anatol,

from my side, there are pros and cons to either patch set going in 
first, but advantages to either are pretty negligible.  Pro for you 
going first: I can use the constants you will define in header files.  
Pro for me going first: your merge should be about the same as if you 
went first (since my changes are small, more localized and affect only 
multiboot.c) and my merge will be easier.


What are your thoughts?

Testing:
   1) Ran the "make check" test suite.
   2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
  grub multiboot.elf test "kernel" by modifying source.)  Verified
  with gdb that new code that reads addresses/offsets from multiboot
  header was accessed.
   3) Booted multiboot kernel with non-zero bss_end_addr.
   4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked.
   5) Code has soaked in an internal repo for two months.
Can you integrate your test kernel from 2) in tests/multiboot/ so we can
keep this as a regression test?

Kevin and alias,

Before I proceed with adding my multiboot test file, I'll clarify here 
that I started with a version from the grub2 tree.  In that file I 
expanded a header file, also from the same tree.  Neither file had any 
license header, though the tree I got them from (Dated October 2017) 
contains the GNU GPLv3 license file.


I'll need to check with my company before I can say I can deliver this 
file.  If I deliver it, I'll add a header stating the GPL license, that 
it came from grub2 and to check its repository for contributors.

Jack Schwartz (4):
   multiboot: bss_end_addr can be zero
   multiboot: Remove unused variables from multiboot.c
   multiboot: Use header names when displaying fields
   multiboot: fprintf(stderr...) -> error_report()

Apart from the conflicts, the patches look good to me.

    Thanks,
    Jack


Kevin

[Qemu-devel] ping: Re: [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

2018-01-12 Thread Jack Schwartz 

Hi everyone.

Ping!  Please review.

Patchwork links:

1/4 multiboot: bss_end_addr can be zero
http://patchwork.ozlabs.org/patch/852049/

2/4 multiboot: Remove unused variables from multiboot.c
http://patchwork.ozlabs.org/patch/852045/

3/4 multiboot: Use header names when displaying fields
http://patchwork.ozlabs.org/patch/852046/

4/4 multiboot: fprintf(stderr...) -> error_report()
http://patchwork.ozlabs.org/patch/852051/

    Thanks,
    Jack


On 12/21/17 09:25, Jack Schwartz wrote:

Properly account for the possibility of multiboot kernels with a zero
bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
kernels without a bss section, by allowing a zeroed bss_end_addr multiboot
header field.

Do some cleanup to multiboot.c as well:
- Remove some unused variables.
- Use more intuitive header names when displaying fields in messages.
- Change fprintf(stderr...) to error_report

Testing:
   1) Ran the "make check" test suite.
   2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
  grub multiboot.elf test "kernel" by modifying source.)  Verified
  with gdb that new code that reads addresses/offsets from multiboot
  header was accessed.
   3) Booted multiboot kernel with non-zero bss_end_addr.
   4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked.
   5) Code has soaked in an internal repo for two months.

    Thanks,
Jack

Jack Schwartz (4):
   multiboot: bss_end_addr can be zero
   multiboot: Remove unused variables from multiboot.c
   multiboot: Use header names when displaying fields
   multiboot: fprintf(stderr...) -> error_report()

  hw/i386/multiboot.c | 77 ++---
  1 file changed, 38 insertions(+), 39 deletions(-)

Re: [Qemu-devel] [PATCH v1 1/1] block: Add numeric errno field to BLOCK_IO_ERROR events

2018-01-10 Thread Jack Schwartz 

Hi Kevin.

Thanks for your feedback.

Looks like my team's project plans have changed, and there is no need to 
pursue this further.  We can work with the existing reason string.


    Thanks,
    Jack

On 01/09/18 02:24, Kevin Wolf wrote:

Am 08.01.2018 um 20:57 hat Jack Schwartz geschrieben:

Hi Kevin.

On 2017-12-22 05:52, Kevin Wolf wrote:

Am 22.12.2017 um 01:11 hat Jack Schwartz geschrieben:

BLOCK_IO_ERROR events currently contain a "reason" string which is
strerror(errno) of the error.  This enhancement provides those events with
the numeric errno value as well, since it is easier to parse for error type
than a string.

Signed-off-by: Jack Schwartz<jack.schwa...@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk<konrad.w...@oracle.com>
Reviewed-by: Karl Heubaum<karl.heub...@oracle.com>

Apart from the technical details that Eric mentioed, I wonder what is
your use case for this?

We have thousands of servers in our cloud, and would like to closely monitor
for different kinds of disk errors without parsing the non-machine-readable
error string.

So do you actually care about the semantical difference between, say,
EINVAL and EIO, and treat them differently in the monitoring? To be
honest, I can't see anything useful you could do with this information
because there are so many possible causes for each of the error codes.

Because if the only thing you want to do with them is to log them in
different categories, you can use the error strings without parsing
them.


Exposing errors in a machine readable form was discussed earlier,

OK, found it.  April of 2010.

Upshot of discussion: exposing naked errnos are platform dependent.

Right, that's what Eric mentioned.


   and
the result was that nobody had an actual use for error codes other than
presenting the right error message to the user - which the error string
already achieves.

Given the platform independence requirement, exposing errors to clients is
not that simple given that different OSs use different errno values.  Other
options/considerations than exposing naked errno values:

- Having a platform-independent enumeration of errors, as Eric suggested.
This would have to explicitly set an enumerated value for each individual
errno we are interested in.  It would be returned in a field that ~parallels
the "reason" string.  This should be OK since for BLOCK_IO_ERROR events we
could limit values to just storage device errors plus a default "other";
otherwise this could be hard to maintain.

But what are "storage device errors"? Can't we get more or less any
error while processing an I/O request?


- The strerror strings cannot be used because they can change with locale.
(This also assumes the strings are identical for given errnos
cross-platform, and that there are no typos - which are not automatically
checked-for.)

You mean when you aggregate errors from multiple different hosts running
on different platforms and where you don't control the locale?

But cross-platform, even the exact numeric error codes you get may
differ, so they become even less meaningful than they already are on a
single platform.

Kevin

Re: [Qemu-devel] [PATCH v1 1/1] block: Add numeric errno field to BLOCK_IO_ERROR events

2018-01-08 Thread Jack Schwartz 

Hi Kevin.

On 2017-12-22 05:52, Kevin Wolf wrote:

Am 22.12.2017 um 01:11 hat Jack Schwartz geschrieben:

BLOCK_IO_ERROR events currently contain a "reason" string which is
strerror(errno) of the error.  This enhancement provides those events with
the numeric errno value as well, since it is easier to parse for error type
than a string.

Signed-off-by: Jack Schwartz<jack.schwa...@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk<konrad.w...@oracle.com>
Reviewed-by: Karl Heubaum<karl.heub...@oracle.com>

Apart from the technical details that Eric mentioed, I wonder what is
your use case for this?
We have thousands of servers in our cloud, and would like to closely 
monitor for different kinds of disk errors without parsing the 
non-machine-readable error string.

Exposing errors in a machine readable form was discussed earlier,

OK, found it.  April of 2010.

Upshot of discussion: exposing naked errnos are platform dependent.

  and
the result was that nobody had an actual use for error codes other than
presenting the right error message to the user - which the error string
already achieves.
Given the platform independence requirement, exposing errors to clients 
is not that simple given that different OSs use different errno values.  
Other options/considerations than exposing naked errno values:


- Having a platform-independent enumeration of errors, as Eric 
suggested.  This would have to explicitly set an enumerated value for 
each individual errno we are interested in.  It would be returned in a 
field that ~parallels the "reason" string.  This should be OK since for 
BLOCK_IO_ERROR events we could limit values to just storage device 
errors plus a default "other"; otherwise this could be hard to maintain.


- The strerror strings cannot be used because they can change with 
locale. (This also assumes the strings are identical for given errnos 
cross-platform, and that there are no typos - which are not 
automatically checked-for.)


    Thanks,
    Jack

P.S.  Please excuse the delayed reply due to vacation / company shutdown.

The only exception so far was ENOSPC, which some management tools like
oVirt respond to by increasing the volume size, so this was mapped into
a bool.

Kevin

[Qemu-devel] [PATCH v1 1/1] block: Add numeric errno field to BLOCK_IO_ERROR events

2017-12-21 Thread Jack Schwartz 
BLOCK_IO_ERROR events currently contain a "reason" string which is
strerror(errno) of the error.  This enhancement provides those events with
the numeric errno value as well, since it is easier to parse for error type
than a string.

Signed-off-by: Jack Schwartz <jack.schwa...@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
Reviewed-by: Karl Heubaum <karl.heub...@oracle.com>
---
 block/block-backend.c |  2 +-
 qapi/block-core.json  | 12 ++--
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index baef8e7..f628668 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1572,7 +1572,7 @@ static void send_qmp_error_event(BlockBackend *blk,
 qapi_event_send_block_io_error(blk_name(blk),
bdrv_get_node_name(blk_bs(blk)), optype,
action, blk_iostatus_is_enabled(blk),
-   error == ENOSPC, strerror(error),
+   error == ENOSPC, error, strerror(error),
_abort);
 }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a8cdbc3..b7beca7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3660,6 +3660,11 @@
 #   io-status is present, please see query-block documentation
 #   for more information (since: 2.2)
 #
+# @errno: int describing the error cause, provided for applications.
+# (Note: while most errnos are posix compliant between OSs, it
+# is possible some errno values can vary among different OSs.)
+# (since 2.12)
+#
 # @reason: human readable string describing the error cause.
 #  (This field is a debugging aid for humans, it should not
 #   be parsed by applications) (since: 2.2)
@@ -3675,14 +3680,17 @@
 #  "data": { "device": "ide0-hd1",
 #"node-name": "#block212",
 #"operation": "write",
-#"action": "stop" },
+#"action": "stop",
+#"nospace": false,
+#"errno": 5,
+#"reason": "Input/output error" },
 #  "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
 #
 ##
 { 'event': 'BLOCK_IO_ERROR',
   'data': { 'device': 'str', 'node-name': 'str', 'operation': 
'IoOperationType',
 'action': 'BlockErrorAction', '*nospace': 'bool',
-'reason': 'str' } }
+'errno': 'int', 'reason': 'str' } }
 
 ##
 # @BLOCK_JOB_COMPLETED:
-- 
1.8.3.1

[Qemu-devel] [PATCH v1 0/1] block: Add numeric errno field to BLOCK_IO_ERROR events

2017-12-21 Thread Jack Schwartz 
Currently, BLOCK_IO_ERROR events have a string error "reason" field
which is derived from errno.  The proposed change adds errno itself
as a field to these events.  Figuring out the error by comparing the
(int) errno itself is easier than comparing a string.  There is also
a comment in the code that the reason field should not be parsed by
applications.

Sample QMP output of modified events adds the errno field as follows (see
last line):

{"timestamp": {"seconds": 1509071709, "microseconds": 563303}, "event":
"BLOCK_IO_ERROR", "data": {"device": "ide0-hd0", "node-name": "#block128",
"reason": "Input/output error", "operation": "write", "action": "ignore",
"errno": 5}}

Testing:
- Artificially created error conditions that emit BLOCK_IO_ERROR events.
Verified those events could be viewed by the QMP monitor and by the
qmp-shell; and that event behavior with those two utilities was identical.
- Ran tests via "make check" from the build root.  There were no changes
from vanilla build when building or running.

Homework:
- Looked through source and build trees for tests and scripts which
reference BLOCK_IO_ERROR events.  No direct references to such events were
found.  No direct references to BLOCK_IO_ERROR events implies there won't be
references to specific fields within those events.

- What about Windows?
  - The file block/block-backend.c is the only C file with a code change.
The file block/Makefile brings block-backend.o into both Windows and Linux
compilations.  The change introduces an additional reference to errno, which
strerror already calls, even with Windows.  That file's prior reference to
errno confirms that Windows will work with the code change.
  - If there would be a Linux vs Windows difference in mapping of errno to
error string values, that difference would have been in place before my
changes.

  Thanks,
  Jack

Jack Schwartz (1):
  block: Add numeric errno field to BLOCK_IO_ERROR events

 block/block-backend.c |  2 +-
 qapi/block-core.json  | 12 ++--
 2 files changed, 11 insertions(+), 3 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH QEMU v1 4/4] multiboot: fprintf(stderr...) -> error_report()

2017-12-21 Thread Jack Schwartz 
Change all fprintf(stderr...) calls in hw/i386/multiboot.c to call
error_report() instead, including the mb_debug macro.  Remove the "\n"
from strings passed to all modified calls, since error_report() appends
one.

Signed-off-by: Jack Schwartz <jack.schwa...@oracle.com>
Reviewed-by: Daniel Kiper <daniel.ki...@oracle.com>
---
 hw/i386/multiboot.c | 55 -
 1 file changed, 29 insertions(+), 26 deletions(-)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 818728b..d9a0a95 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -31,12 +31,13 @@
 #include "hw/loader.h"
 #include "elf.h"
 #include "sysemu/sysemu.h"
+#include "qemu/error-report.h"
 
 /* Show multiboot debug output */
 //#define DEBUG_MULTIBOOT
 
 #ifdef DEBUG_MULTIBOOT
-#define mb_debug(a...) fprintf(stderr, ## a)
+#define mb_debug(a...) error_report(a)
 #else
 #define mb_debug(a...)
 #endif
@@ -137,7 +138,7 @@ static void mb_add_mod(MultibootState *s,
 stl_p(p + MB_MOD_END, end);
 stl_p(p + MB_MOD_CMDLINE, cmdline_phys);
 
-mb_debug("mod%02d: "TARGET_FMT_plx" - "TARGET_FMT_plx"\n",
+mb_debug("mod%02d: "TARGET_FMT_plx" - "TARGET_FMT_plx,
  s->mb_mods_count, start, end);
 
 s->mb_mods_count++;
@@ -179,12 +180,12 @@ int load_multiboot(FWCfgState *fw_cfg,
 if (!is_multiboot)
 return 0; /* no multiboot */
 
-mb_debug("qemu: I believe we found a multiboot image!\n");
+mb_debug("qemu: I believe we found a multiboot image!");
 memset(bootinfo, 0, sizeof(bootinfo));
 memset(, 0, sizeof(mbs));
 
 if (flags & 0x0004) { /* MULTIBOOT_HEADER_HAS_VBE */
-fprintf(stderr, "qemu: multiboot knows VBE. we don't.\n");
+error_report("qemu: multiboot knows VBE. we don't.");
 }
 if (!(flags & 0x0001)) { /* MULTIBOOT_HEADER_HAS_ADDR */
 uint64_t elf_entry;
@@ -193,7 +194,7 @@ int load_multiboot(FWCfgState *fw_cfg,
 fclose(f);
 
 if (((struct elf64_hdr*)header)->e_machine == EM_X86_64) {
-fprintf(stderr, "Cannot load x86-64 image, give a 32bit one.\n");
+error_report("Cannot load x86-64 image, give a 32bit one.");
 exit(1);
 }
 
@@ -201,7 +202,7 @@ int load_multiboot(FWCfgState *fw_cfg,
_low, _high, 0, I386_ELF_MACHINE,
0, 0);
 if (kernel_size < 0) {
-fprintf(stderr, "Error while loading elf kernel\n");
+error_report("Error while loading elf kernel");
 exit(1);
 }
 mh_load_addr = elf_low;
@@ -210,12 +211,13 @@ int load_multiboot(FWCfgState *fw_cfg,
 
 mbs.mb_buf = g_malloc(mb_kernel_size);
 if (rom_copy(mbs.mb_buf, mh_load_addr, mb_kernel_size) != 
mb_kernel_size) {
-fprintf(stderr, "Error while fetching elf kernel from rom\n");
+error_report("Error while fetching elf kernel from rom");
 exit(1);
 }
 
-mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry 
%#zx\n",
-  mb_kernel_size, (size_t)mh_entry_addr);
+mb_debug("qemu: loading multiboot-elf kernel "
+ "(%#x bytes) with entry %#zx",
+ mb_kernel_size, (size_t)mh_entry_addr);
 } else {
 /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_ADDR. */
 uint32_t mh_header_addr = ldl_p(header+i+12);
@@ -224,7 +226,7 @@ int load_multiboot(FWCfgState *fw_cfg,
 
 mh_load_addr = ldl_p(header+i+16);
 if (mh_header_addr < mh_load_addr) {
-fprintf(stderr, "invalid load_addr address\n");
+error_report("invalid load_addr address");
 exit(1);
 }
 
@@ -234,20 +236,20 @@ int load_multiboot(FWCfgState *fw_cfg,
 
 if (mh_load_end_addr) {
 if (mh_load_end_addr < mh_load_addr) {
-fprintf(stderr, "invalid load_end_addr address\n");
+error_report("invalid load_end_addr address");
 exit(1);
 }
 mb_load_size = mh_load_end_addr - mh_load_addr;
 } else {
 if (kernel_file_size < mb_kernel_text_offset) {
-fprintf(stderr, "invalid kernel_file_size\n");
+error_report("invalid kernel_file_size");
 exit(1);
 }
 mb_load_size = kernel_file_size - mb_kernel_text_offset;
 }
 if (mh_bss_end_addr) {
 if (mh_bss_end_addr < (mh_load_addr + mb_load_size)) {
-fprintf(stderr, "invalid bss_end_addr address\n");
+error_rep

[Qemu-devel] [PATCH QEMU v1 1/4] multiboot: bss_end_addr can be zero

2017-12-21 Thread Jack Schwartz 
The multiboot spec (https://www.gnu.org/software/grub/manual/multiboot/),
section 3.1.3, allows for bss_end_addr to be zero.

A zero bss_end_addr signifies there is no .bss section.

Suggested-by: Daniel Kiper <daniel.ki...@oracle.com>
Signed-off-by: Jack Schwartz <jack.schwa...@oracle.com>
Reviewed-by: Daniel Kiper <daniel.ki...@oracle.com>
---
 hw/i386/multiboot.c | 18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index c7b70c9..ff2733d 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -233,12 +233,6 @@ int load_multiboot(FWCfgState *fw_cfg,
 mh_entry_addr = ldl_p(header+i+28);
 
 if (mh_load_end_addr) {
-if (mh_bss_end_addr < mh_load_addr) {
-fprintf(stderr, "invalid mh_bss_end_addr address\n");
-exit(1);
-}
-mb_kernel_size = mh_bss_end_addr - mh_load_addr;
-
 if (mh_load_end_addr < mh_load_addr) {
 fprintf(stderr, "invalid mh_load_end_addr address\n");
 exit(1);
@@ -249,8 +243,16 @@ int load_multiboot(FWCfgState *fw_cfg,
 fprintf(stderr, "invalid kernel_file_size\n");
 exit(1);
 }
-mb_kernel_size = kernel_file_size - mb_kernel_text_offset;
-mb_load_size = mb_kernel_size;
+mb_load_size = kernel_file_size - mb_kernel_text_offset;
+}
+if (mh_bss_end_addr) {
+if (mh_bss_end_addr < (mh_load_addr + mb_load_size)) {
+fprintf(stderr, "invalid mh_bss_end_addr address\n");
+exit(1);
+}
+mb_kernel_size = mh_bss_end_addr - mh_load_addr;
+} else {
+mb_kernel_size = mb_load_size;
 }
 
 /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
-- 
1.8.3.1

[Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

2017-12-21 Thread Jack Schwartz 
Properly account for the possibility of multiboot kernels with a zero
bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
kernels without a bss section, by allowing a zeroed bss_end_addr multiboot
header field.

Do some cleanup to multiboot.c as well:
- Remove some unused variables.
- Use more intuitive header names when displaying fields in messages.
- Change fprintf(stderr...) to error_report

Testing:
  1) Ran the "make check" test suite.
  2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
 grub multiboot.elf test "kernel" by modifying source.)  Verified
 with gdb that new code that reads addresses/offsets from multiboot
 header was accessed.
  3) Booted multiboot kernel with non-zero bss_end_addr.
  4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked.
  5) Code has soaked in an internal repo for two months.

Thanks,
    Jack

Jack Schwartz (4):
  multiboot: bss_end_addr can be zero
  multiboot: Remove unused variables from multiboot.c
  multiboot: Use header names when displaying fields
  multiboot: fprintf(stderr...) -> error_report()

 hw/i386/multiboot.c | 77 ++---
 1 file changed, 38 insertions(+), 39 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH QEMU v1 3/4] multiboot: Use header names when displaying fields

2017-12-21 Thread Jack Schwartz 
Refer to field names when displaying fields in printf and debug statements.

Signed-off-by: Jack Schwartz <jack.schwa...@oracle.com>
Reviewed-by: Daniel Kiper <daniel.ki...@oracle.com>
---
 hw/i386/multiboot.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 964feaf..818728b 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -224,7 +224,7 @@ int load_multiboot(FWCfgState *fw_cfg,
 
 mh_load_addr = ldl_p(header+i+16);
 if (mh_header_addr < mh_load_addr) {
-fprintf(stderr, "invalid mh_load_addr address\n");
+fprintf(stderr, "invalid load_addr address\n");
 exit(1);
 }
 
@@ -234,7 +234,7 @@ int load_multiboot(FWCfgState *fw_cfg,
 
 if (mh_load_end_addr) {
 if (mh_load_end_addr < mh_load_addr) {
-fprintf(stderr, "invalid mh_load_end_addr address\n");
+fprintf(stderr, "invalid load_end_addr address\n");
 exit(1);
 }
 mb_load_size = mh_load_end_addr - mh_load_addr;
@@ -247,7 +247,7 @@ int load_multiboot(FWCfgState *fw_cfg,
 }
 if (mh_bss_end_addr) {
 if (mh_bss_end_addr < (mh_load_addr + mb_load_size)) {
-fprintf(stderr, "invalid mh_bss_end_addr address\n");
+fprintf(stderr, "invalid bss_end_addr address\n");
 exit(1);
 }
 mb_kernel_size = mh_bss_end_addr - mh_load_addr;
@@ -255,10 +255,10 @@ int load_multiboot(FWCfgState *fw_cfg,
 mb_kernel_size = mb_load_size;
 }
 
-mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
-mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
-mb_debug("multiboot: mh_load_end_addr = %#x\n", mh_load_end_addr);
-mb_debug("multiboot: mh_bss_end_addr = %#x\n", mh_bss_end_addr);
+mb_debug("multiboot: header_addr = %#x\n", mh_header_addr);
+mb_debug("multiboot: load_addr = %#x\n", mh_load_addr);
+mb_debug("multiboot: load_end_addr = %#x\n", mh_load_end_addr);
+mb_debug("multiboot: bss_end_addr = %#x\n", mh_bss_end_addr);
 mb_debug("qemu: loading multiboot kernel (%#x bytes) at %#x\n",
  mb_load_size, mh_load_addr);
 
@@ -361,7 +361,7 @@ int load_multiboot(FWCfgState *fw_cfg,
 stl_p(bootinfo + MBI_BOOT_DEVICE, 0x8000); /* XXX: use the -boot 
switch? */
 stl_p(bootinfo + MBI_MMAP_ADDR,   ADDR_E820_MAP);
 
-mb_debug("multiboot: mh_entry_addr = %#x\n", mh_entry_addr);
+mb_debug("multiboot: entry_addr = %#x\n", mh_entry_addr);
 mb_debug("   mb_buf_phys   = "TARGET_FMT_plx"\n", mbs.mb_buf_phys);
 mb_debug("   mod_start = "TARGET_FMT_plx"\n", mbs.mb_buf_phys 
+ mbs.offset_mods);
 mb_debug("   mb_mods_count = %d\n", mbs.mb_mods_count);
-- 
1.8.3.1

[Qemu-devel] [PATCH QEMU v1 2/4] multiboot: Remove unused variables from multiboot.c

2017-12-21 Thread Jack Schwartz 
Remove unused variables: mh_mode_type, mh_width, mh_height, mh_depth

Signed-off-by: Jack Schwartz <jack.schwa...@oracle.com>
Reviewed-by: Daniel Kiper <daniel.ki...@oracle.com>
---
 hw/i386/multiboot.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index ff2733d..964feaf 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -255,12 +255,6 @@ int load_multiboot(FWCfgState *fw_cfg,
 mb_kernel_size = mb_load_size;
 }
 
-/* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
-uint32_t mh_mode_type = ldl_p(header+i+32);
-uint32_t mh_width = ldl_p(header+i+36);
-uint32_t mh_height = ldl_p(header+i+40);
-uint32_t mh_depth = ldl_p(header+i+44); */
-
 mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
 mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
 mb_debug("multiboot: mh_load_end_addr = %#x\n", mh_load_end_addr);
-- 
1.8.3.1

[Qemu-devel] query-block io-status display

2017-09-04 Thread Jack Schwartz 

Hi Luiz, Markus and everyone.

I am working on a qemu enhancement to display io-status in each 
query-block command, not just those for devices which have werror and/or 
rerror set to stop on error.


I'd like to verify the reasons behind the query-block command not 
reporting io-status if errors were reported to the guest or ignored.  A 
clue may come from the original code review email[1] for when this code 
was implemented:


  "In case of multiple errors being triggered in sequence only the first
   one is stored. The I/O status is always reset to BDRV_IOS_OK when the
   'cont' command is issued."

From this I infer:
- io-status is shown when qemu is stopped onerror so errors can be seen 
in cases where a guest does not handle them.

- io-status is not shown when errors are already being handled by a guest
- io-status is not shown when errors are ignored

Is this correct?  Are there other subtleties/reasons as well?

Thanks,
Jack

[1] http://lists.nongnu.org/archive/html/qemu-devel/2011-09/msg02940.html