Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Laszlo Ersek <lersek@redhat.com>]

On 05/02/17 23:45, Jordan Justen wrote:
> On 2017-05-02 12:31:39, Laszlo Ersek wrote:
>> On 05/02/17 20:22, Jordan Justen wrote:
>>> On 2017-05-02 07:39:04, Laszlo Ersek wrote:
>>>>
>>>> I wouldn't mind if we made more room for the varstore in the 2MB build,
>>>> even at the expense of FVMAIN_COMPACT, if we also kept the current 2MB
>>>> build the default, so that the "new" (incompatible) 2MB build doesn't
>>>> come as a surprise to unsuspecting downstreams.
>>>>
>>>> Regarding the 4MB build:
>>>> - we can discuss that on top of the above "new" 2MB build,
>>>> - we can discuss it instead of the above "new" 2MB build,
>>>> - we can postpone it for now, for upstream.
>>>
>>> I was hoping there was a way to avoid the need to add 4MB, but you
>>> needing to support the layout until 2024 really adds risk to the 2MB
>>> image. I think there is a decent chance 2MB would work until then, but
>>> I can also see how it adds significant risk.
>>>
>>> If we are adding the 4MB layout, then we may as well make it the
>>> default for debug builds.
>>
>> OK, I think that's technically doable. Based on your commit e3dca1859b24
>> ("OvmfPkg: Increase default RELEASE build image size to 2MB",
>> 2016-01-29), the $(TARGET) macro can be used in FDF files.
>>
>>> I'm not sure what to do about 2MB then. In
>>> the short term, I say we do nothing.
>>
>> Do you mean "do nothing about 2MB", or "do nothing at all in the fdf.inc"?
>>
>> (You have to be really specific with me these days, sorry...)
>>
>> If I understand correctly, we'd have to reinstate FD_SIZE_2MB then, so
>> that people that want to stick with the 2MB build for DEBUG (and NOOPT)
>> can select it. Given that 4MB would become the new default for those
>> targets.
> 
> Ah. I guess I dropped FD_SIZE_2MB in e3dca1859b24, which I don't think
> I should have done. Going forward, I think we should allow
> FD_SIZE_1/2/4MB.

Agreed.

> 
> Regarding RELEASE builds, I'm not sure what we should do. Should we
> just change it to 4MB as well? In the past, I preferred to allow
> release builds to use the smaller size, since it fit. But, in this
> case we also know that leaving 2MB size will mean a known test will
> fail. The test failing doesn't mean a real user is likely to be
> impacted, but I guess Microsoft feels the larger size may be required
> in some scenarios.
> 
> What do you think? (Maybe not a fair question since you don't use the
> release build.) I guess the safe option is to just bump the default
> for both the debug and release builds to the ridiculously large (er, I
> mean luxuriously spacious :) 4MB image.

Will do that in v2. (Maybe we should call the 4MB build
FD_SIZE_LUXURIOUS_FRIDGE. :) )

> 
>>> I feel fairly confident of the 4MB image supporting your code size
>>> needs until 2024. What seems less certain in future varstore
>>> requirements. Right now the requirement is 120~128k. I think rather
>>> than 248k in the 4MB layout, we should make it 256k. (Since these
>>> kinds of things often progress in powers-of-two.) It will make for a
>>> couple unfriendly offsets, but it seems worth it to avoid being 8k shy
>>> of the next power-of-two size.
>>>
>>> In my other email, I mentioned the event-log. I did ask around a bit
>>> about this, but I didn't find anyone willing to fight for more space.
>>> Therefore, I think we should just keep it at 4k.
>>
>> That means 256K for the varstore, 4K for the event log, 4K for the FTW
>> working block.
>>
>> How much for the spare area? Currently the spare area equals the sum of
>> the former three. The spare area is used both while reclaiming the
>> varstore, and while reclaiming the FTW working block. (Not sure about
>> the event log.) So I'd say we should stick with our tradition, and make
>> the spare area 256K + 4K + 4K = 264K in size. That would result in a
>> 528K NVRAM. (Which is 16K larger than in the current patch.)
>>
>> In turn, I wouldn't increase FVMAIN_COMPACT by 1664K, to 3376K, but by
>> 16K less (1648K) to 3360K. The full FD size would remain 4M.
>>
>> Does this sound okay?
> 
> Yes.

Yay! \o/

> This will leave the split rom sizes being a multiple of 16k rather
> than 512k. Today they are a multiple of 128k. I don't expect this
> would be an issue for qemu/kvm. Do you agree?

I agree. In "hw/i386/pc_sysfw.c", QEMU expects the pflash chip sizes to
be multiples of 4KB (1 << 12).

I'll attempt to write, test and post v2 today.

Thank you,
Laszlo