| Message ID | 1340984094-5451-5-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 29.06.2012 17:34, schrieb Markus Armbruster: > vvfat creates a virtual VFAT filesystem with a certain logical > geometry that depends on its options. It sets the "geometry hint" to > this geometry. It is the only block driver to do this. > > The geometry hint is about about *physical* geometry, and used only by > certain hard disk device models. > > vvfat's hint is normally invisible for device models, because > bdrv_open() puts a raw format on top of vvfat's fat protocol. That > raw format is where drive_init() puts the user's geometry (if any), > and where the device model gets it from. > > Nobody complained, because the default physical geometry is the same > as vvfat's logical geometry: > > opts LCHS def. PCHS > 1024,16,63 same > :32: 1024,16,63 same > :16: 1024,16,63 same > :12: 64,16,63 same > > Except when you specify :floppy: > > opts LCHS def. PCHS > :floppy: 80, 2,36 5,16,63 > :32:floppy: 80, 2,36 5,16,63 > :16:floppy: 80, 2,36 5,16,63 > :12:floppy: 80, 2,18 2,16,63 > > Silly thing to do for use with a hard disk. > > However, the "raw" format can be suppressed by adding an > redundant-looking "format=vvfat" to "file=fat:FOO". Then, vvfat's > hint clobbers the user's geometry, i.e. -drive options cyls, heads, > secs get silently ignored. Don't do that. > > No change without format=vvfat. With it, the user's hard disk > geometry (-drive options cyls, heads, secs) is now obeyed, and the > default hard disk geometry with :floppy: now matches the one without > format=vvfat. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> For some values of "obeyed". If I understand correctly, the user defined geometry will indeed by visible for the device emulation now, but this still doesn't mean that vvfat also provides an image that matches this geometry. Not sure if it is a good idea to allow such mismatches. > @@ -1067,19 +1074,16 @@ DLOG(if (stderr == NULL) { > else > dirname += i+1; > > - bs->total_sectors=bs->cyls*bs->heads*bs->secs; > + bs->total_sectors = cyls * heads * secs; > > - if(init_directories(s, dirname)) > + if (init_directories(s, dirname, heads, secs)) { > return -1; > + } > > s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; > > if(s->first_sectors_number==0x40) > - init_mbr(s); > - else { > - /* MS-DOS does not like to know about CHS (?). */ > - bs->heads = bs->cyls = bs->secs = 0; > - } > + init_mbr(s, cyls, heads, secs); You can add braces here while touching the code. Kevin
Il 04/07/2012 17:23, Kevin Wolf ha scritto: >> > opts LCHS def. PCHS >> > :floppy: 80, 2,36 5,16,63 >> > :32:floppy: 80, 2,36 5,16,63 >> > :16:floppy: 80, 2,36 5,16,63 >> > :12:floppy: 80, 2,18 2,16,63 >> > >> > Silly thing to do for use with a hard disk. >> > >> > However, the "raw" format can be suppressed by adding an >> > redundant-looking "format=vvfat" to "file=fat:FOO". Then, vvfat's >> > hint clobbers the user's geometry, i.e. -drive options cyls, heads, >> > secs get silently ignored. Don't do that. >> > >> > No change without format=vvfat. With it, the user's hard disk >> > geometry (-drive options cyls, heads, secs) is now obeyed, and the >> > default hard disk geometry with :floppy: now matches the one without >> > format=vvfat. >> > >> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > For some values of "obeyed". If I understand correctly, the user defined > geometry will indeed by visible for the device emulation now, but this > still doesn't mean that vvfat also provides an image that matches this > geometry. Not sure if it is a good idea to allow such mismatches. > Does this only matter if you declare an image with :floppy: and pass it as a hard disk? Then we can honestly say we don't care... Paolo
Am 04.07.2012 18:25, schrieb Paolo Bonzini: > Il 04/07/2012 17:23, Kevin Wolf ha scritto: >>>> opts LCHS def. PCHS >>>> :floppy: 80, 2,36 5,16,63 >>>> :32:floppy: 80, 2,36 5,16,63 >>>> :16:floppy: 80, 2,36 5,16,63 >>>> :12:floppy: 80, 2,18 2,16,63 >>>> >>>> Silly thing to do for use with a hard disk. >>>> >>>> However, the "raw" format can be suppressed by adding an >>>> redundant-looking "format=vvfat" to "file=fat:FOO". Then, vvfat's >>>> hint clobbers the user's geometry, i.e. -drive options cyls, heads, >>>> secs get silently ignored. Don't do that. >>>> >>>> No change without format=vvfat. With it, the user's hard disk >>>> geometry (-drive options cyls, heads, secs) is now obeyed, and the >>>> default hard disk geometry with :floppy: now matches the one without >>>> format=vvfat. >>>> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> For some values of "obeyed". If I understand correctly, the user defined >> geometry will indeed by visible for the device emulation now, but this >> still doesn't mean that vvfat also provides an image that matches this >> geometry. Not sure if it is a good idea to allow such mismatches. >> > > Does this only matter if you declare an image with :floppy: and pass it > as a hard disk? Then we can honestly say we don't care... As I understand it, the patch has two effect: 1. the user's hard disk geometry (-drive options cyls, heads, secs) is now obeyed 2. the default hard disk geometry with :floppy: now matches the one without format=vvfat I don't care about 2. indeed, but doing 1. right would be nice. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 04.07.2012 18:25, schrieb Paolo Bonzini: >> Il 04/07/2012 17:23, Kevin Wolf ha scritto: >>>>> opts LCHS def. PCHS >>>>> :floppy: 80, 2,36 5,16,63 >>>>> :32:floppy: 80, 2,36 5,16,63 >>>>> :16:floppy: 80, 2,36 5,16,63 >>>>> :12:floppy: 80, 2,18 2,16,63 >>>>> >>>>> Silly thing to do for use with a hard disk. >>>>> >>>>> However, the "raw" format can be suppressed by adding an >>>>> redundant-looking "format=vvfat" to "file=fat:FOO". Then, vvfat's >>>>> hint clobbers the user's geometry, i.e. -drive options cyls, heads, >>>>> secs get silently ignored. Don't do that. >>>>> >>>>> No change without format=vvfat. With it, the user's hard disk >>>>> geometry (-drive options cyls, heads, secs) is now obeyed, and the >>>>> default hard disk geometry with :floppy: now matches the one without >>>>> format=vvfat. >>>>> >>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> For some values of "obeyed". If I understand correctly, the user defined >>> geometry will indeed by visible for the device emulation now, but this >>> still doesn't mean that vvfat also provides an image that matches this >>> geometry. Not sure if it is a good idea to allow such mismatches. This patch doesn't let the user do anything he couldn't do before. It only makes the special case "vvfat without raw on top" consistent with all other block backend configurations. With "raw over vvfat" in particular. So, if anything needs additional user parameter validation, it's the common case. Note that any other image type could contain the very same FAT filesystem as a vvfat one, with the very same impact on a guest vulnerable to mismatches between geometry and block device contents. To avoid confusion, let me recap geometry. Physical geometry is a property of the disk. Or rather, a property of the disk's legacy CHS command interface; it ceased to have any useful relation to platters and heads long ago. Non-legacy commands use LBA to address blocks. Logical geometry is a property of the PC BIOS legacy CHS interface (int 13h). It was invented when disks outgrew the BIOS's CHS limits. It need not match physical geometry (*not* matching is its purpose). The BIOS translates logical geometry to something the disk understands. Long ago, this meant translating to physical CHS. Nowadays, it means translating to LBA. As far as I can tell, SeaBIOS always translates to LBA. SeaBIOS picks a logical geometry based on what it finds in CMOS. Logical geometry matters only for software that uses legacy int 13h. DOS, basically. Physical geometry matters only for software that talks to the disk directly, using legacy CHS commands. Sufficently old versions of Unix, perhaps? The choice of geometry isn't really important, as long as: 1. The mapping to LBA is sane: lba = ((c * #h) + h * #s) + (s-1) where c, h, s is the address, and #c, #h, #s is the geometry. Increasing first s, then h, then c runs through the lbas in order, regardless of what you picked for geometry. For physical geometry, the device model maps. As far as I can tell, they all map sanely. For logical geometry, SeaBIOS maps. As far as I can tell, it maps sanely. Actually, even an insane mapping would work for block drivers that only schlep blocks, as long as it doesn't change. Same as with physical disks. 2. The OS is happy with the partition table. Some OSs want each partition to start and end on a cylinder boundary. For them, whatever geometry they use should be consistent with the partition table. Note that before PATCH 03/32, vvfat's partition end was screwed up, and nobody noticed. >> Does this only matter if you declare an image with :floppy: and pass it >> as a hard disk? Then we can honestly say we don't care... > > As I understand it, the patch has two effect: > > 1. the user's hard disk geometry (-drive options cyls, heads, secs) is > now obeyed ... even when the user suppresses raw by specifying format=vvfat with file=fat:... Consistent with how all other block drivers work. > 2. the default hard disk geometry with :floppy: now matches the one > without format=vvfat Yes. > I don't care about 2. indeed, but doing 1. right would be nice.
Kevin Wolf <kwolf@redhat.com> writes: > Am 29.06.2012 17:34, schrieb Markus Armbruster: [...] >> @@ -1067,19 +1074,16 @@ DLOG(if (stderr == NULL) { >> else >> dirname += i+1; >> >> - bs->total_sectors=bs->cyls*bs->heads*bs->secs; >> + bs->total_sectors = cyls * heads * secs; >> >> - if(init_directories(s, dirname)) >> + if (init_directories(s, dirname, heads, secs)) { >> return -1; >> + } >> >> s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; >> >> if(s->first_sectors_number==0x40) >> - init_mbr(s); >> - else { >> - /* MS-DOS does not like to know about CHS (?). */ >> - bs->heads = bs->cyls = bs->secs = 0; >> - } >> + init_mbr(s, cyls, heads, secs); > > You can add braces here while touching the code. I'm not touching the if... I'll do it if you insist.
diff --git a/block/vvfat.c b/block/vvfat.c index 62745b5..ef89f71 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -359,11 +359,12 @@ typedef struct BDRVVVFATState { * if the position is outside the specified geometry, fill maximum value for CHS * and return 1 to signal overflow. */ -static int sector2CHS(BlockDriverState* bs, mbr_chs_t * chs, int spos){ +static int sector2CHS(mbr_chs_t *chs, int spos, int cyls, int heads, int secs) +{ int head,sector; - sector = spos % (bs->secs); spos/= bs->secs; - head = spos % (bs->heads); spos/= bs->heads; - if(spos >= bs->cyls){ + sector = spos % secs; spos /= secs; + head = spos % heads; spos /= heads; + if (spos >= cyls) { /* Overflow, it happens if 32bit sector positions are used, while CHS is only 24bit. Windows/Dos is said to take 1023/255/63 as nonrepresentable CHS */ @@ -378,7 +379,7 @@ static int sector2CHS(BlockDriverState* bs, mbr_chs_t * chs, int spos){ return 0; } -static void init_mbr(BDRVVVFATState* s) +static void init_mbr(BDRVVVFATState *s, int cyls, int heads, int secs) { /* TODO: if the files mbr.img and bootsect.img exist, use them */ mbr_t* real_mbr=(mbr_t*)s->first_sectors; @@ -393,8 +394,10 @@ static void init_mbr(BDRVVVFATState* s) partition->attributes=0x80; /* bootable */ /* LBA is used when partition is outside the CHS geometry */ - lba = sector2CHS(s->bs, &partition->start_CHS, s->first_sectors_number-1); - lba |= sector2CHS(s->bs, &partition->end_CHS, s->bs->total_sectors - 1); + lba = sector2CHS(&partition->start_CHS, s->first_sectors_number - 1, + cyls, heads, secs); + lba |= sector2CHS(&partition->end_CHS, s->bs->total_sectors - 1, + cyls, heads, secs); /*LBA partitions are identified only by start/length_sector_long not by CHS*/ partition->start_sector_long = cpu_to_le32(s->first_sectors_number-1); @@ -831,7 +834,7 @@ static inline off_t cluster2sector(BDRVVVFATState* s, uint32_t cluster_num) } static int init_directories(BDRVVVFATState* s, - const char* dirname) + const char *dirname, int heads, int secs) { bootsector_t* bootsector; mapping_t* mapping; @@ -958,8 +961,8 @@ static int init_directories(BDRVVVFATState* s, bootsector->media_type=(s->first_sectors_number>1?0xf8:0xf0); /* media descriptor (f8=hd, f0=3.5 fd)*/ s->fat.pointer[0] = bootsector->media_type; bootsector->sectors_per_fat=cpu_to_le16(s->sectors_per_fat); - bootsector->sectors_per_track=cpu_to_le16(s->bs->secs); - bootsector->number_of_heads=cpu_to_le16(s->bs->heads); + bootsector->sectors_per_track = cpu_to_le16(secs); + bootsector->number_of_heads = cpu_to_le16(heads); bootsector->hidden_sectors=cpu_to_le32(s->first_sectors_number==1?0:0x3f); bootsector->total_sectors=cpu_to_le32(s->sector_count>0xffff?s->sector_count:0); @@ -992,7 +995,7 @@ static void vvfat_rebind(BlockDriverState *bs) static int vvfat_open(BlockDriverState *bs, const char* dirname, int flags) { BDRVVVFATState *s = bs->opaque; - int i; + int i, cyls, heads, secs; #ifdef DEBUG vvv = s; @@ -1034,24 +1037,28 @@ DLOG(if (stderr == NULL) { /* 1.44MB or 2.88MB floppy. 2.88MB can be FAT12 (default) or FAT16. */ if (!s->fat_type) { s->fat_type = 12; - bs->secs = 36; + secs = 36; s->sectors_per_cluster=2; } else { - bs->secs=(s->fat_type == 12 ? 18 : 36); + secs = s->fat_type == 12 ? 18 : 36; s->sectors_per_cluster=1; } s->first_sectors_number = 1; - bs->cyls=80; bs->heads=2; + cyls = 80; + heads = 2; } else { /* 32MB or 504MB disk*/ if (!s->fat_type) { s->fat_type = 16; } - bs->cyls=(s->fat_type == 12 ? 64 : 1024); - bs->heads=16; bs->secs=63; + cyls = s->fat_type == 12 ? 64 : 1024; + heads = 16; + secs = 63; } + fprintf(stderr, "vvfat %s chs %d,%d,%d\n", + dirname, cyls, heads, secs); - s->sector_count=bs->cyls*bs->heads*bs->secs-(s->first_sectors_number-1); + s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1); if (strstr(dirname, ":rw:")) { if (enable_write_target(s)) @@ -1067,19 +1074,16 @@ DLOG(if (stderr == NULL) { else dirname += i+1; - bs->total_sectors=bs->cyls*bs->heads*bs->secs; + bs->total_sectors = cyls * heads * secs; - if(init_directories(s, dirname)) + if (init_directories(s, dirname, heads, secs)) { return -1; + } s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; if(s->first_sectors_number==0x40) - init_mbr(s); - else { - /* MS-DOS does not like to know about CHS (?). */ - bs->heads = bs->cyls = bs->secs = 0; - } + init_mbr(s, cyls, heads, secs); // assert(is_consistent(s)); qemu_co_mutex_init(&s->lock);
vvfat creates a virtual VFAT filesystem with a certain logical geometry that depends on its options. It sets the "geometry hint" to this geometry. It is the only block driver to do this. The geometry hint is about about *physical* geometry, and used only by certain hard disk device models. vvfat's hint is normally invisible for device models, because bdrv_open() puts a raw format on top of vvfat's fat protocol. That raw format is where drive_init() puts the user's geometry (if any), and where the device model gets it from. Nobody complained, because the default physical geometry is the same as vvfat's logical geometry: opts LCHS def. PCHS 1024,16,63 same :32: 1024,16,63 same :16: 1024,16,63 same :12: 64,16,63 same Except when you specify :floppy: opts LCHS def. PCHS :floppy: 80, 2,36 5,16,63 :32:floppy: 80, 2,36 5,16,63 :16:floppy: 80, 2,36 5,16,63 :12:floppy: 80, 2,18 2,16,63 Silly thing to do for use with a hard disk. However, the "raw" format can be suppressed by adding an redundant-looking "format=vvfat" to "file=fat:FOO". Then, vvfat's hint clobbers the user's geometry, i.e. -drive options cyls, heads, secs get silently ignored. Don't do that. No change without format=vvfat. With it, the user's hard disk geometry (-drive options cyls, heads, secs) is now obeyed, and the default hard disk geometry with :floppy: now matches the one without format=vvfat. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block/vvfat.c | 52 ++++++++++++++++++++++++++++------------------------ 1 files changed, 28 insertions(+), 24 deletions(-)