| Message ID | 20190215122808.22301-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] hw/block: report when pflash backing file isn't aligned | expand |
On 02/15/19 13:28, Alex Bennée wrote: > It looks like there was going to be code to check we had some sort of > alignment so lets replace it with an actual check. This is a bit more > useful than the enigmatic "failed to read the initial flash content" > when we attempt to read the number of bytes the device should have. > > This is a potential confusing stumbling block when you move from using > -bios to using -drive if=pflash,file=blob,format=raw,readonly for > loading your firmware code. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > --- > v2 > - use PRIu64 instead of PRId64 > - tweaked message output > --- > hw/block/pflash_cfi01.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index bffb4c40e7..7532c8d8e8 100644 > --- a/hw/block/pflash_cfi01.c > +++ b/hw/block/pflash_cfi01.c > @@ -722,12 +722,20 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) > } > device_len = sector_len_per_device * blocks_per_device; > > - /* XXX: to be fixed */ > -#if 0 > - if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) && > - total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024)) > - return NULL; > -#endif > + /* > + * Validate the backing store is the right size for pflash > + * devices. It has to be padded to a multiple of the flash block > + * size. > + */ > + if (pfl->blk) { > + uint64_t backing_len = blk_getlength(pfl->blk); > + if (device_len != backing_len) { > + error_setg(errp, "device needs %" PRIu64 " bytes, " > + "backing file provides only %" PRIu64 " bytes", > + device_len, backing_len); > + return; > + } > + } > > memory_region_init_rom_device( > &pfl->mem, OBJECT(dev), > The word "only" implies that the file is too small. It could be too large as well (the C expression is right, but the message doesn't reflect it). With the word "only" dropped, I think the message looks fine. Also, now I've checked blk_getlength(). First, it can directly return (-ENOMEDIUM). Second, it delegates the job to bdrv_getlength(), which itself can return (-EFBIG). Third, bdrv_nb_sectors(), used internally, can itself return (-ENOMEDIUM). For me this is pretty much impossible to follow. Can we: - use type "int64_t" for "backing_len" in the new code, AND - either prove (from the rest of pflash_cfi01_realize()) that "backing_len" is nonnegative, and then *assert* it, plus cast "backing_len" to uint64_t for the comparison; - or check for a negative "backing_len" explicitly, and if that happens, fail pflash_cfi01_realize() with an error message that reports *that* failure? Sorry about the pedantry; I've got no clue what's happening in blk_getlength() for real. Thanks! Laszlo
Laszlo Ersek <lersek@redhat.com> writes: > On 02/15/19 13:28, Alex Bennée wrote: >> It looks like there was going to be code to check we had some sort of >> alignment so lets replace it with an actual check. This is a bit more >> useful than the enigmatic "failed to read the initial flash content" >> when we attempt to read the number of bytes the device should have. >> >> This is a potential confusing stumbling block when you move from using >> -bios to using -drive if=pflash,file=blob,format=raw,readonly for >> loading your firmware code. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >> --- >> v2 >> - use PRIu64 instead of PRId64 >> - tweaked message output >> --- >> hw/block/pflash_cfi01.c | 20 ++++++++++++++------ >> 1 file changed, 14 insertions(+), 6 deletions(-) >> >> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >> index bffb4c40e7..7532c8d8e8 100644 >> --- a/hw/block/pflash_cfi01.c >> +++ b/hw/block/pflash_cfi01.c >> @@ -722,12 +722,20 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) >> } >> device_len = sector_len_per_device * blocks_per_device; >> >> - /* XXX: to be fixed */ >> -#if 0 >> - if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) && >> - total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024)) >> - return NULL; >> -#endif pflash_cfi02_realize() has the same XXX. There's a pair of related XXXs in taihu_405ep_init(). Related because @total_len is computed like this total_len = pfl->sector_len * pfl->nb_blocs; and the two factors come from callers of pflash_cfi01_realize(), pflash_cfi02_realize(), or from ve_pflash_cfi01_register(), xtfpga_flash_init(). Aside: the latter two are slight variations of pflash_cfi01_realize() which I'm going to clean up. Some of these use fixed sizes (good, real machines do that, too). Some of them compute them from blk_getlength(), with varying levels of care. I'm afraid we need to take all that into account. Let's take a step back and consider sane requirements. The size of the flash chip is a property of the machine. It is *fixed*. Using whatever size the image has is sloppy modelling. A machine may come in minor variations that aren't worth their own machine type. One such variation could be a different flash chip size. Using the image size to select one from the set of fixed sizes is tolerable. Aside: the image size can change between the place where we get it to pick a flash chip size and realize(). I guess that's a "don't do that then". If the image size matches the flash chip's size exactly, all is well. Should we require the size to match the flash chip's size? If we accept a smaller image, we want to pad with zeros. What about writes beyond the size of the image? Discard them, or let them extend the image file? If we accept a larger image, we want to ignore its tail. Sorry for turning the tiny issue your patch tries to address into a much larger one... >> + /* >> + * Validate the backing store is the right size for pflash >> + * devices. It has to be padded to a multiple of the flash block >> + * size. >> + */ >> + if (pfl->blk) { >> + uint64_t backing_len = blk_getlength(pfl->blk); >> + if (device_len != backing_len) { >> + error_setg(errp, "device needs %" PRIu64 " bytes, " >> + "backing file provides only %" PRIu64 " bytes", >> + device_len, backing_len); >> + return; >> + } >> + } >> >> memory_region_init_rom_device( >> &pfl->mem, OBJECT(dev), >> > > The word "only" implies that the file is too small. It could be too > large as well (the C expression is right, but the message doesn't > reflect it). > > With the word "only" dropped, I think the message looks fine. > > > Also, now I've checked blk_getlength(). First, it can directly return > (-ENOMEDIUM). Second, it delegates the job to bdrv_getlength(), which > itself can return (-EFBIG). Third, bdrv_nb_sectors(), used internally, > can itself return (-ENOMEDIUM). > > For me this is pretty much impossible to follow. Can we: > > - use type "int64_t" for "backing_len" in the new code, AND > > - either prove (from the rest of pflash_cfi01_realize()) that > "backing_len" is nonnegative, and then *assert* it, plus cast > "backing_len" to uint64_t for the comparison; > > - or check for a negative "backing_len" explicitly, and if that > happens, fail pflash_cfi01_realize() with an error message that > reports *that* failure? > > Sorry about the pedantry; I've got no clue what's happening in > blk_getlength() for real. László is right, blk_getlength() *can* fail. It doesn't fail often, so neglecting to check for failure can go undetected for quite a while.
Markus Armbruster <armbru@redhat.com> writes: > Laszlo Ersek <lersek@redhat.com> writes: > >> On 02/15/19 13:28, Alex Bennée wrote: >>> It looks like there was going to be code to check we had some sort of >>> alignment so lets replace it with an actual check. This is a bit more >>> useful than the enigmatic "failed to read the initial flash content" >>> when we attempt to read the number of bytes the device should have. >>> >>> This is a potential confusing stumbling block when you move from using >>> -bios to using -drive if=pflash,file=blob,format=raw,readonly for >>> loading your firmware code. >>> >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>> >>> --- >>> v2 >>> - use PRIu64 instead of PRId64 >>> - tweaked message output >>> --- >>> hw/block/pflash_cfi01.c | 20 ++++++++++++++------ >>> 1 file changed, 14 insertions(+), 6 deletions(-) >>> >>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >>> index bffb4c40e7..7532c8d8e8 100644 >>> --- a/hw/block/pflash_cfi01.c >>> +++ b/hw/block/pflash_cfi01.c >>> @@ -722,12 +722,20 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) >>> } >>> device_len = sector_len_per_device * blocks_per_device; >>> >>> - /* XXX: to be fixed */ >>> -#if 0 >>> - if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) && >>> - total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024)) >>> - return NULL; >>> -#endif > > pflash_cfi02_realize() has the same XXX. > > There's a pair of related XXXs in taihu_405ep_init(). Related because > @total_len is computed like this > > total_len = pfl->sector_len * pfl->nb_blocs; > > and the two factors come from callers of pflash_cfi01_realize(), > pflash_cfi02_realize(), or from ve_pflash_cfi01_register(), > xtfpga_flash_init(). Aside: the latter two are slight variations of > pflash_cfi01_realize() which I'm going to clean up. Some of these use > fixed sizes (good, real machines do that, too). Some of them compute > them from blk_getlength(), with varying levels of care. Missed one: create_one_flash(). Let's review flash size computation. Single fixed size per machine type: collie_init() 0x02000000 (two times) connex_init() 0x01000000 verdex_init() 0x02000000 mainstone_common_init() 0x02000000 sx1_init() (16 * 1024 * 1024) for sx1-v1 (32 * 1024 * 1024) for sx1 ( 8 * 1024 * 1024) for sx1-v1 versatile_init() (64 * 1024 * 1024) z2_init() 0x00800000 milkymist_init() 32 * MiB petalogix_ml605_init() (32 * MiB) petalogix_s3adsp1800_init() (16 * MiB) mips_malta_init() (4 * MiB) mips_r4k_init() 0x00400000 virtex_init() (16 * MiB) digic4_add_k8p3215uqb_rom() (4 * 1024 * 1024) zynq_init() (64 * 1024 * 1024) lm32_evr_init() 32 * MiB lm32_uclinux_init() 32 * MiB taihu_405ep_init() 32 * MiB (but see below) r2d_init() 0x02000000 ve_pflash_cfi01_register() (64 * 1024 * 1024) xtfpga_flash_init() 0x00400000 for lx60* 0x01000000 for lx200* 0x01000000 for ml605* 0x08000000 for kc705* create_one_flash() 0x08000000 / 2 (two times) Set of fixed sizes: musicpal_init() image size, either 8*1024*1024, 16*1024*1024 or 32*1024*1024 Troublemakers: pc_system_flash_init() sum of image sizes, at most 8MiB rejects images whose size is not a multiple of 4KiB sam460ex_load_uboot() image size rounded up to multiple of 64KiB ref405ep_init() image size rounded up to multiple of 64KiB taihu_405ep_init() image size rounded up to multiple of 64KiB > I'm afraid we need to take all that into account. We can ignore all but the four troublemakers. [...]
On 02/15/19 17:01, Markus Armbruster wrote: > The size of the flash chip is a property of the machine. It is *fixed*. I'll have to disagree on this one; in OVMF's case, you can build OVMF in 1MB, 2MB, and 4MB *cumulative* size (that is, the numbers I give here each refer to the sum of both pflash chips, namely varstore and executable). The cumulative size that is picked at OVMF build time influences (obviously) the amount of crap ^W firmware features that one can squeeze into the executable image, but it also determines other things. For example, the 4MB build has a different (incompatible!) internal structure than the 2MB does. See the small table/comparison in the following commit message: https://github.com/tianocore/edk2/commit/b24fca05751f In order to provide some numbers that one can observe simply on their host filesystem, the 2MB cumulative size consists of (128K varstore chip, 1920KB executable chip); while the 4MB one comprises (528K varstore chip, 3568KB executable chip). > Using whatever size the image has is sloppy modelling. Maybe so, but it's also very convenient, and also quite important, right now (given the multiple firmware image sizes used in the wild). > A machine may come in minor variations that aren't worth their own > machine type. One such variation could be a different flash chip size. > Using the image size to select one from the set of fixed sizes is > tolerable. The problem is that this requires coordination between QEMU and firmware development. (Well, I have to agree that the present patch is *already* that kind of coordination; my point is that when I introduced the 4MB build for OVMF, I didn't have to touch QEMU. In retrospect, I'm extremely thankful for that, as the introduction of the 4MB build was difficult in itself.) "Using the image size to flexibly dictate the pflash size, in board code" would be preferable, to "select from a pre-determined set". (A similarly flexible approach would be if we required the user or mgmt app to specify base & size as pflash device properties -- see your option #1 elsewhere --, but I digress.) > Aside: the image size can change between the place where we get it to > pick a flash chip size and realize(). Haha :) > I guess that's a "don't do that then". I think so! :) > If the image size matches the flash chip's size exactly, all is well. > > Should we require the size to match the flash chip's size? So, this is hard to answer generally for all targets / boards. pc_system_flash_init() (on i440fx/q35, i.e. when OVMF is used) will guarantee this equality -- ignoring the TOCTOU that you point out above, anyway. For "virt", the answer carries a lot more weight, because *that* board code does not size the pflash from the fw image found in the filesystem. (See create_flash(), and a15memmap[VIRT_FLASH].) IIUC, this patch suggests, "yes, we should require equality". A no-op for OVMF (= pc_system_flash_init()), and helpful against user mistakes with the ArmVirtQemu platform firmware (= create_flash()). > If we accept a smaller image, we want to pad with zeros. What about > writes beyond the size of the image? Discard them, or let them extend > the image file? > > If we accept a larger image, we want to ignore its tail. > > Sorry for turning the tiny issue your patch tries to address into a much > larger one... I think the following would be useful: - reject images that are larger than the pflash chip size - pad smaller images with zero (not on the disk) - ignore guest writes to padding Because, in case of the ArmVirtQemu firmware at least, the build process produces the following two files: 2.0M QEMU_EFI.fd 768K QEMU_VARS.fd And we've always had to manually pad each of these to 64MB, with zeroes, on-disk, for use with the "virt" board. If QEMU could do that automatically (in memory), that would be a win, in my book. If Alex thinks such padding is out of scope for now, I will certainly not insist; I think the present patch (enforcing equality) is already a significant usability improvement. I'd just like the error message to be precise, and the error checking to be (more) complete. Thanks! Laszlo
Laszlo Ersek <lersek@redhat.com> writes: > On 02/15/19 17:01, Markus Armbruster wrote: > >> Let's take a step back and consider sane requirements. >> >> The size of the flash chip is a property of the machine. It is *fixed*. > > I'll have to disagree on this one; in OVMF's case, you can build OVMF in > 1MB, 2MB, and 4MB *cumulative* size (that is, the numbers I give here > each refer to the sum of both pflash chips, namely varstore and executable). > > The cumulative size that is picked at OVMF build time influences > (obviously) the amount of crap ^W firmware features that one can squeeze > into the executable image, but it also determines other things. For > example, the 4MB build has a different (incompatible!) internal > structure than the 2MB does. See the small table/comparison in the > following commit message: > > https://github.com/tianocore/edk2/commit/b24fca05751f > > In order to provide some numbers that one can observe simply on their > host filesystem, the 2MB cumulative size consists of (128K varstore > chip, 1920KB executable chip); while the 4MB one comprises (528K > varstore chip, 3568KB executable chip). > >> Using whatever size the image has is sloppy modelling. > > Maybe so, but it's also very convenient, and also quite important, right > now (given the multiple firmware image sizes used in the wild). > >> A machine may come in minor variations that aren't worth their own >> machine type. One such variation could be a different flash chip size. >> Using the image size to select one from the set of fixed sizes is >> tolerable. > > The problem is that this requires coordination between QEMU and firmware > development. > > (Well, I have to agree that the present patch is *already* that kind of > coordination; We've always had that kind of coordination. It just happens to be less tight in the case of PC firmware in flash than in most other cases. > my point is that when I introduced the 4MB build for OVMF, > I didn't have to touch QEMU. In retrospect, I'm extremely thankful for > that, as the introduction of the 4MB build was difficult in itself.) You don't actually need "flash size is whatever the image size is" for that. "Use image size to select one from the set of fixed sizes" should suffice. Actually, the PC machines currently comply, just with a rather large set: { n * 4KiB | 1 <= n <= 2048 }. I very much doubt PC firmware sizes other than powers of two between 64KiB and 8MiB matter. Have you ever seen real flash chips with sizes like 64140KiB? For traditional (non-flash) firmware, these machines require the image size to be a multiple of 64KiB. There is no upper limit. Images 2GiB and larger cause integer overflow in old_pc_system_rom_init(). I think both mechanisms should accept the same set of sizes. > "Using the image size to flexibly dictate the pflash size, in board > code" would be preferable, to "select from a pre-determined set". (A > similarly flexible approach would be if we required the user or mgmt app > to specify base & size as pflash device properties -- see your option #1 > elsewhere --, but I digress.) Sticking to what real hardware does has had a much better track record in QEMU than doing things that don't exist in real hardware. That said, restricting our virtual pflash chips to sizes that can plausibly occur in real hardware is not a hill I'm prepared to die on. >> Aside: the image size can change between the place where we get it to >> pick a flash chip size and realize(). > > Haha :) > >> I guess that's a "don't do that then". > > I think so! :) > >> If the image size matches the flash chip's size exactly, all is well. >> >> Should we require the size to match the flash chip's size? > > So, this is hard to answer generally for all targets / boards. > > pc_system_flash_init() (on i440fx/q35, i.e. when OVMF is used) will > guarantee this equality -- ignoring the TOCTOU that you point out above, > anyway. > > For "virt", the answer carries a lot more weight, because *that* board > code does not size the pflash from the fw image found in the filesystem. > (See create_flash(), and a15memmap[VIRT_FLASH].) The vast majority of boards use *fixed* flash sizes, namely whatever the physical machine they're modelling has. We're discussing variable size only because a few boards (four, if I count correctly) emulate a family of rather diverse machines (such as PCs), or are perhaps just confused about what they emulate. > IIUC, this patch suggests, "yes, we should require equality". A no-op > for OVMF (= pc_system_flash_init()), and helpful against user mistakes > with the ArmVirtQemu platform firmware (= create_flash()). Requiring equality makes sense to me. >> If we accept a smaller image, we want to pad with zeros. What about >> writes beyond the size of the image? Discard them, or let them extend >> the image file? >> >> If we accept a larger image, we want to ignore its tail. >> >> Sorry for turning the tiny issue your patch tries to address into a much >> larger one... > > I think the following would be useful: > > - reject images that are larger than the pflash chip size > - pad smaller images with zero (not on the disk) > - ignore guest writes to padding The "ignore writes" part adds complexity. > Because, in case of the ArmVirtQemu firmware at least, the build process > produces the following two files: > > 2.0M QEMU_EFI.fd > 768K QEMU_VARS.fd > > And we've always had to manually pad each of these to 64MB, with zeroes, > on-disk, for use with the "virt" board. If QEMU could do that > automatically (in memory), that would be a win, in my book. Some code somewhere has to add padding. Moving it from A to B is not an overall win by itself. To avoid wasting actual disk, try padding with a hole. > If Alex thinks such padding is out of scope for now, I will certainly > not insist; I think the present patch (enforcing equality) is already a > significant usability improvement. I'd just like the error message to be > precise, and the error checking to be (more) complete.
Markus Armbruster <armbru@redhat.com> writes: > Markus Armbruster <armbru@redhat.com> writes: > >> Laszlo Ersek <lersek@redhat.com> writes: >> >>> On 02/15/19 13:28, Alex Bennée wrote: >>>> It looks like there was going to be code to check we had some sort of >>>> alignment so lets replace it with an actual check. This is a bit more >>>> useful than the enigmatic "failed to read the initial flash content" >>>> when we attempt to read the number of bytes the device should have. >>>> >>>> This is a potential confusing stumbling block when you move from using >>>> -bios to using -drive if=pflash,file=blob,format=raw,readonly for >>>> loading your firmware code. >>>> >>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>>> >>>> --- >>>> v2 >>>> - use PRIu64 instead of PRId64 >>>> - tweaked message output >>>> --- >>>> hw/block/pflash_cfi01.c | 20 ++++++++++++++------ >>>> 1 file changed, 14 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >>>> index bffb4c40e7..7532c8d8e8 100644 >>>> --- a/hw/block/pflash_cfi01.c >>>> +++ b/hw/block/pflash_cfi01.c >>>> @@ -722,12 +722,20 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) >>>> } >>>> device_len = sector_len_per_device * blocks_per_device; >>>> >>>> - /* XXX: to be fixed */ >>>> -#if 0 >>>> - if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) && >>>> - total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024)) >>>> - return NULL; >>>> -#endif >> >> pflash_cfi02_realize() has the same XXX. >> >> There's a pair of related XXXs in taihu_405ep_init(). Related because >> @total_len is computed like this >> >> total_len = pfl->sector_len * pfl->nb_blocs; >> >> and the two factors come from callers of pflash_cfi01_realize(), >> pflash_cfi02_realize(), or from ve_pflash_cfi01_register(), >> xtfpga_flash_init(). Aside: the latter two are slight variations of >> pflash_cfi01_realize() which I'm going to clean up. Some of these use >> fixed sizes (good, real machines do that, too). Some of them compute >> them from blk_getlength(), with varying levels of care. > > Missed one: create_one_flash(). > > Let's review flash size computation. > > Single fixed size per machine type: > > collie_init() 0x02000000 (two times) > connex_init() 0x01000000 > verdex_init() 0x02000000 > mainstone_common_init() 0x02000000 > sx1_init() (16 * 1024 * 1024) for sx1-v1 > (32 * 1024 * 1024) for sx1 > ( 8 * 1024 * 1024) for sx1-v1 > versatile_init() (64 * 1024 * 1024) > z2_init() 0x00800000 > milkymist_init() 32 * MiB > petalogix_ml605_init() (32 * MiB) > petalogix_s3adsp1800_init() (16 * MiB) > mips_malta_init() (4 * MiB) > mips_r4k_init() 0x00400000 > virtex_init() (16 * MiB) > digic4_add_k8p3215uqb_rom() (4 * 1024 * 1024) > zynq_init() (64 * 1024 * 1024) > lm32_evr_init() 32 * MiB > lm32_uclinux_init() 32 * MiB > taihu_405ep_init() 32 * MiB (but see below) > r2d_init() 0x02000000 > ve_pflash_cfi01_register() (64 * 1024 * 1024) > xtfpga_flash_init() 0x00400000 for lx60* > 0x01000000 for lx200* > 0x01000000 for ml605* > 0x08000000 for kc705* > create_one_flash() 0x08000000 / 2 (two times) > > Set of fixed sizes: > > musicpal_init() image size, either 8*1024*1024, > 16*1024*1024 or 32*1024*1024 > > Troublemakers: > > pc_system_flash_init() sum of image sizes, at most 8MiB > rejects images whose size is not a > multiple of 4KiB Image size must be one of n * 4KiB where 1 <= n <= 2048. Can be filed under "Set of fixed sizes" > sam460ex_load_uboot() image size rounded up to multiple of > 64KiB Looks broken. (1) The flash is mapped at address 0xFFF00000. When no image is supplied, it's size is 1MiB (0x100000). Else, it's size is the size of the image rounded up to the next multiple of 64KiB. I have no idea what happens when the size exceeds 1MiB. Does it wrap around the 32 bit address space? (2) Unless the image size is a multiple of 64KiB, pflash_cfi01_realize() fails with "failed to read the initial flash content". > ref405ep_init() image size rounded up to multiple of > 64KiB Looks broken. (1) The flash is mapped at address 2^32 - (image size). Its size is the size of the image rounded up to the next multiple of 64KiB. Unless the rounding is a no-op, the flash extends beyond the end of the 32 bit address space. Probably irrelevant due to (2). If the size exceeds 0x80000 Bytes, we overlap first SRAM, then other stuff. No idea how that would play out. I suspect we should either require the image to be 512KiB, or maybe also accept smaller ones and pad them to 512KiB. The latter would be unusual. (2) Unless the image size is a multiple of 64KiB, pflash_cfi01_realize() fails with "failed to read the initial flash content". > taihu_405ep_init() image size rounded up to multiple of > 64KiB Looks broken. Boot flash (unit 0) is just like ref405ep_init(), just with different addresses. (3) Application flash (unit 1) has a fixed size of 32 MiB. If the image is smaller, pflash_cfi01_realize() fails with "failed to read the initial flash content". I think we should require the image to match the real machine's flash size. We could also accept smaller ones and pad them to 1MiB, or larger ones ignoring the tail, but that would be unusual. >> I'm afraid we need to take all that into account. > > We can ignore all but the four troublemakers. > > [...] I'll post patches.
On 02/16/19 12:21, Markus Armbruster wrote: > Laszlo Ersek <lersek@redhat.com> writes: >> On 02/15/19 17:01, Markus Armbruster wrote: >>> Using whatever size the image has is sloppy modelling. >> >> Maybe so, but it's also very convenient, and also quite important, right >> now (given the multiple firmware image sizes used in the wild). >> >>> A machine may come in minor variations that aren't worth their own >>> machine type. One such variation could be a different flash chip size. >>> Using the image size to select one from the set of fixed sizes is >>> tolerable. >> >> The problem is that this requires coordination between QEMU and firmware >> development. >> >> (Well, I have to agree that the present patch is *already* that kind of >> coordination; > > We've always had that kind of coordination. It just happens to be less > tight in the case of PC firmware in flash than in most other cases. > >> my point is that when I introduced the 4MB build for OVMF, >> I didn't have to touch QEMU. In retrospect, I'm extremely thankful for >> that, as the introduction of the 4MB build was difficult in itself.) > > You don't actually need "flash size is whatever the image size is" for > that. "Use image size to select one from the set of fixed sizes" should > suffice. > > Actually, the PC machines currently comply, just with a rather large > set: { n * 4KiB | 1 <= n <= 2048 }. > > I very much doubt PC firmware sizes other than powers of two between > 64KiB and 8MiB matter. Have you ever seen real flash chips with sizes > like 64140KiB? Honestly, I wouldn't know. I haven't seen any physical flash chip (as in, directing my gaze at it). I also don't know what the usual flash chip sizes are on physical boards (e.g. I have no clue what my laptop uses). I think a jump from 4MB to 8MB would be too large. It gives too much of sudden convenience to firmware developers. I do agree "7MB" looks quite lame. I really wonder about the flash sizes used on physical UEFI boards. Thanks, Laszlo
Laszlo Ersek <lersek@redhat.com> writes: > On 02/15/19 17:01, Markus Armbruster wrote: > >> The size of the flash chip is a property of the machine. It is *fixed*. > > I'll have to disagree on this one; in OVMF's case, you can build OVMF in > 1MB, 2MB, and 4MB *cumulative* size (that is, the numbers I give here > each refer to the sum of both pflash chips, namely varstore and executable). > > The cumulative size that is picked at OVMF build time influences > (obviously) the amount of crap ^W firmware features that one can squeeze > into the executable image, but it also determines other things. For > example, the 4MB build has a different (incompatible!) internal > structure than the 2MB does. See the small table/comparison in the > following commit message: > > https://github.com/tianocore/edk2/commit/b24fca05751f > > In order to provide some numbers that one can observe simply on their > host filesystem, the 2MB cumulative size consists of (128K varstore > chip, 1920KB executable chip); while the 4MB one comprises (528K > varstore chip, 3568KB executable chip). > >> Using whatever size the image has is sloppy modelling. > > Maybe so, but it's also very convenient, and also quite important, right > now (given the multiple firmware image sizes used in the wild). > >> A machine may come in minor variations that aren't worth their own >> machine type. One such variation could be a different flash chip size. >> Using the image size to select one from the set of fixed sizes is >> tolerable. > > The problem is that this requires coordination between QEMU and firmware > development. > > (Well, I have to agree that the present patch is *already* that kind of > coordination; my point is that when I introduced the 4MB build for OVMF, > I didn't have to touch QEMU. In retrospect, I'm extremely thankful for > that, as the introduction of the 4MB build was difficult in itself.) > > "Using the image size to flexibly dictate the pflash size, in board > code" would be preferable, to "select from a pre-determined set". (A > similarly flexible approach would be if we required the user or mgmt app > to specify base & size as pflash device properties -- see your option #1 > elsewhere --, but I digress.) > >> Aside: the image size can change between the place where we get it to >> pick a flash chip size and realize(). > > Haha :) > >> I guess that's a "don't do that then". > > I think so! :) > >> If the image size matches the flash chip's size exactly, all is well. >> >> Should we require the size to match the flash chip's size? > > So, this is hard to answer generally for all targets / boards. > > pc_system_flash_init() (on i440fx/q35, i.e. when OVMF is used) will > guarantee this equality -- ignoring the TOCTOU that you point out above, > anyway. > > For "virt", the answer carries a lot more weight, because *that* board > code does not size the pflash from the fw image found in the filesystem. > (See create_flash(), and a15memmap[VIRT_FLASH].) > > IIUC, this patch suggests, "yes, we should require equality". A no-op > for OVMF (= pc_system_flash_init()), and helpful against user mistakes > with the ArmVirtQemu platform firmware (= create_flash()). > >> If we accept a smaller image, we want to pad with zeros. What about >> writes beyond the size of the image? Discard them, or let them extend >> the image file? >> >> If we accept a larger image, we want to ignore its tail. >> >> Sorry for turning the tiny issue your patch tries to address into a much >> larger one... > > I think the following would be useful: > > - reject images that are larger than the pflash chip size > - pad smaller images with zero (not on the disk) > - ignore guest writes to padding > > Because, in case of the ArmVirtQemu firmware at least, the build process > produces the following two files: > > 2.0M QEMU_EFI.fd > 768K QEMU_VARS.fd > > And we've always had to manually pad each of these to 64MB, with zeroes, > on-disk, for use with the "virt" board. If QEMU could do that > automatically (in memory), that would be a win, in my book. > > If Alex thinks such padding is out of scope for now, I will certainly > not insist; I think the present patch (enforcing equality) is already a > significant usability improvement. I'd just like the error message to be > precise, and the error checking to be (more) complete. I think padding should be in scope for read-only blobs. It does seem pointless forcing distros to pad blobs that are ultimately never going to be written to. The only reason I didn't look into it is because I was unfamiliar with the blk_* api but I guess we can do it all in the pflash code. I'll have a look at Markus's clean-ups first and see if I can re-spin on top of that. -- Alex Bennée
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c index bffb4c40e7..7532c8d8e8 100644 --- a/hw/block/pflash_cfi01.c +++ b/hw/block/pflash_cfi01.c @@ -722,12 +722,20 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) } device_len = sector_len_per_device * blocks_per_device; - /* XXX: to be fixed */ -#if 0 - if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) && - total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024)) - return NULL; -#endif + /* + * Validate the backing store is the right size for pflash + * devices. It has to be padded to a multiple of the flash block + * size. + */ + if (pfl->blk) { + uint64_t backing_len = blk_getlength(pfl->blk); + if (device_len != backing_len) { + error_setg(errp, "device needs %" PRIu64 " bytes, " + "backing file provides only %" PRIu64 " bytes", + device_len, backing_len); + return; + } + } memory_region_init_rom_device( &pfl->mem, OBJECT(dev),
It looks like there was going to be code to check we had some sort of alignment so lets replace it with an actual check. This is a bit more useful than the enigmatic "failed to read the initial flash content" when we attempt to read the number of bytes the device should have. This is a potential confusing stumbling block when you move from using -bios to using -drive if=pflash,file=blob,format=raw,readonly for loading your firmware code. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- v2 - use PRIu64 instead of PRId64 - tweaked message output --- hw/block/pflash_cfi01.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) -- 2.20.1