Re: [Qemu-devel] [PATCH for 1.4] block/vpc: Fix size calculation

[Jeff Cody]

On Mon, Feb 11, 2013 at 09:54:56AM +0100, Kevin Wolf wrote:
> Am 08.02.2013 18:43, schrieb Stefan Weil:
> > Am 08.02.2013 13:14, schrieb Jeff Cody:
> >> On Fri, Feb 08, 2013 at 09:38:47AM +0100, Kevin Wolf wrote:
> >>> Am 07.02.2013 20:26, schrieb Stefan Weil:
> >>>> From: Stefan Weil <address@hidden(none)>
> >>>>
> >>>> The size calculated from the CHS values is not the real image (disk) 
> >>>> size,
> >>>> but usually a smaller value. This is caused by rounding effects.
> >>>>
> >>>> Only older operating systems use CHS. Such guests won't be able to use
> >>>> the whole disk. All modern operating systems use the real size.
> >>>>
> >>>> This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/.
> >>>>
> >>>> Signed-off-by: Stefan Weil <address@hidden>
> >>>> ---
> >>>>
> >>>> This is a rebased extract from my patch series for block/vpc.c.
> >>>> It's the minimum needed to fix the open bug for QEMU 1.4.
> >>>>
> >>>> The rest of the series can be discussed and applied after 1.4.
> >>>>
> >>>> Regards
> >>>>
> >>>> Stefan W.
> >>>>
> >>>> PS. Please excuse a previous personal mail which I had sent
> >>>> with a wrong signature and without addressing qemu-devel.
> >>>>
> >>>>  block/vpc.c |   14 +++++++++-----
> >>>>  1 file changed, 9 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/block/vpc.c b/block/vpc.c
> >>>> index 82229ef..b4ff564 100644
> >>>> --- a/block/vpc.c
> >>>> +++ b/block/vpc.c
> >>>> @@ -34,6 +34,8 @@
> >>>>  
> >>>>  #define HEADER_SIZE 512
> >>>>  
> >>>> +#define VHD_SECTOR_SIZE 512
> >>>> +
> >>>>  //#define CACHE
> >>>>  
> >>>>  enum vhd_type {
> >>>> @@ -204,11 +206,13 @@ static int vpc_open(BlockDriverState *bs, int 
> >>>> flags)
> >>>>      /* Write 'checksum' back to footer, or else will leave it with 
> >>>> zero. */
> >>>>      footer->checksum = be32_to_cpu(checksum);
> >>>>  
> >>>> -    // The visible size of a image in Virtual PC depends on the geometry
> >>>> -    // rather than on the size stored in the footer (the size in the 
> >>>> footer
> >>>> -    // is too large usually)
> >>>> -    bs->total_sectors = (int64_t)
> >>>> -        be16_to_cpu(footer->cyls) * footer->heads * 
> >>>> footer->secs_per_cyl;
> >>>> +    /* The visible size of a image in Virtual PC depends on the guest:
> >>>> +     * QEMU and other emulators report the real size (here in sectors).
> >>>> +     * All modern operating systems use this real size.
> >>>> +     * Very old operating systems use CHS values to calculate the total 
> >>>> size.
> >>>> +     * This calculated size is usually smaller than the real size.
> >>>> +     */
> >>>> +    bs->total_sectors = be64_to_cpu(footer->size) / VHD_SECTOR_SIZE;
> >>> It's unfortunate that I don't have my old Virtual PC installation around
> >>> any more so I could prove that you're wrong for at least some versions.
> >>> Or does a Linux of 2009 already count as "very old"?
> > 
> > Linux is a modern OS per definition :-)
> > 
> > I made an additional test with Knoppix (from December 2006),
> > and it worked as expected.
> 
> So I'm afraid that the reason isn't the guest OS but the Virtual PC
> version. I guess supporting newer versions of it is more important, so
> if Jeff's testing passes, I'm not against taking the patch, even though
> I don't feel completely comfortable about it. But I don't see how we
> could support both ways at the same time.
> 

I've done some testing back and forth between VPC, Win Server 2012
Hyper-V, and qemu/kvm with and without this patch applied.

The short: this patch will break Virtual PC VHD compatibility (as it
exists under Win7), but allow compatibility for VHD files created by
Hyper-V (at least under Server 2012).

VPC and Server 2012 Hyper-V are also incompatible with each other,
when it comes to VHD files, if that is any consolation.

I used sha1sums between the different combination of VHD files filled to
capacity with random data under VPC, qemu/kvm (with & without the
patch), and Hyper-V.  Without this patch we are an exact math with
VPC, and with this patch we are an exact match with Hyper-V.

However, perhaps we can support both, depending on who created the VHD
file.  There is a "Creator Application" field in the VHD header - for
the VPC-created VHD file, that field is ASCII "vpc ".  For the Hyper-V
created VHD file, that field is ASCII "win ".  If we see the field as
"vpc ", we could default to using the CHS geometry found in the
header - otherwise, do as this patch does, and go by the virtual disk
size.

Jeff