[Qemu-devel] Testing sysbus devices

[Qemu-devel] Testing sysbus devices

[Stephen Checkoway]

Hi all,

I've been working on some improvements to the pflash_cfi02 block device (interleaved flash devices similar to pflash_cfi01, multi-sector erase, nonuniform sector sizes, and some bug fixes and I'm planning on implementing sector erase suspend/resume commands in the near future).

There appear to be no existing tests for this device and I'm unsure what the appropriate way to add tests for sysbus devices is. -device can't be used because sysbus devices aren't user-creatable (and even if they were, creating the device wouldn't be sufficient since it wouldn't connect it to the sysbus).

Any suggestions would be appreciated.

Thank you,

-- 
Stephen Checkoway

Re: [Qemu-devel] Testing sysbus devices

[Thomas Huth]

On 18/02/2019 07.07, Stephen Checkoway wrote:
> Hi all,
> 
> I've been working on some improvements to the pflash_cfi02 block device (interleaved flash devices similar to pflash_cfi01, multi-sector erase, nonuniform sector sizes, and some bug fixes and I'm planning on implementing sector erase suspend/resume commands in the near future).
> 
> There appear to be no existing tests for this device and I'm unsure what the appropriate way to add tests for sysbus devices is. -device can't be used because sysbus devices aren't user-creatable (and even if they were, creating the device wouldn't be sufficient since it wouldn't connect it to the sysbus).
> 
> Any suggestions would be appreciated.

I think you could use one of the machines that has a cfi02 on board. For
example: Write some random data to a temporary file. Run qemu with:

 QTestState *qts;
 qts = qtest_initf(" qemu-system-arm -M musicpal,accel=qtest "
                "-drive if=pflash,file=%s,format=raw", filename);

Then you should be able to access the device with the qtest_read/write
functions, e.g. use "qtest_memread(qts, 0x100000000ULL, ...)" to read
the contents of the device. I haven't tried that though, that's just my
quick assumption from looking at hw/arm/musicpal.c ...

 Thomas

Re: [Qemu-devel] Testing sysbus devices

[Stephen Checkoway]

On Feb 18, 2019, at 08:43, Thomas Huth <thuth@redhat.com> wrote:

> I think you could use one of the machines that has a cfi02 on board. For
> example: Write some random data to a temporary file. Run qemu with:
> 
> QTestState *qts;
> qts = qtest_initf(" qemu-system-arm -M musicpal,accel=qtest "
>                "-drive if=pflash,file=%s,format=raw", filename);

If I do that, will it be possible for the test to override the properties set by pflash_cfi02_register? It looks like I should be able to use -global to set properties that aren't set explicitly.

Thank you,

-- 
Stephen Checkoway

Re: [Qemu-devel] Testing sysbus devices

[Thomas Huth]

On 18/02/2019 17.02, Stephen Checkoway wrote:
> 
> 
> On Feb 18, 2019, at 08:43, Thomas Huth <thuth@redhat.com> wrote:
> 
>> I think you could use one of the machines that has a cfi02 on board. For
>> example: Write some random data to a temporary file. Run qemu with:
>>
>> QTestState *qts;
>> qts = qtest_initf(" qemu-system-arm -M musicpal,accel=qtest "
>>                "-drive if=pflash,file=%s,format=raw", filename);
> 
> If I do that, will it be possible for the test to override the properties set by pflash_cfi02_register? It looks like I should be able to use -global to set properties that aren't set explicitly.

Yes, I'm also not an expert in that area, but I think -global should do
the job here.

 Thomas

Re: [Qemu-devel] Testing sysbus devices

[Markus Armbruster]

Stephen Checkoway <stephen.checkoway@oberlin.edu> writes:

> On Feb 18, 2019, at 08:43, Thomas Huth <thuth@redhat.com> wrote:
>
>>On 18/02/2019 07.07, Stephen Checkoway wrote:
>>> Hi all,
>>> 
>>> I've been working on some improvements to the pflash_cfi02 block device (interleaved flash devices similar to pflash_cfi01, multi-sector erase, nonuniform sector sizes, and some bug fixes and I'm planning on implementing sector erase suspend/resume commands in the near future).

Any chance you could do multiple region support, too?

>>> There appear to be no existing tests for this device and I'm unsure what the appropriate way to add tests for sysbus devices is. -device can't be used because sysbus devices aren't user-creatable (and even if they were, creating the device wouldn't be sufficient since it wouldn't connect it to the sysbus).
>>> 
>>> Any suggestions would be appreciated.
>> 
>> I think you could use one of the machines that has a cfi02 on board. For
>> example: Write some random data to a temporary file. Run qemu with:
>> 
>> QTestState *qts;
>> qts = qtest_initf(" qemu-system-arm -M musicpal,accel=qtest "
>>                "-drive if=pflash,file=%s,format=raw", filename);
>
> If I do that, will it be possible for the test to override the properties set by pflash_cfi02_register? It looks like I should be able to use -global to set properties that aren't set explicitly.

Yes.

Won't work for properties set by pflash_cfi02_register(), though.  To
test the full range of values there, you'd have to make them
configurable somehow.  We currently don't have a good way to do that.
Please see

    Subject: Re: Configuring pflash devices for OVMF firmware
    Message-ID: <87mun8gd2x.fsf@dusky.pond.sub.org>
    https://lists.nongnu.org/archive/html/qemu-devel/2019-02/msg01734.html

Re: [Qemu-devel] Testing sysbus devices

[Stephen Checkoway]

On Feb 18, 2019, at 13:08, Markus Armbruster <armbru@redhat.com> wrote:

> Stephen Checkoway <stephen.checkoway@oberlin.edu> writes:
> 
>> On Feb 18, 2019, at 08:43, Thomas Huth <thuth@redhat.com> wrote:
>> 
>>> On 18/02/2019 07.07, Stephen Checkoway wrote:
>>>> Hi all,
>>>> 
>>>> I've been working on some improvements to the pflash_cfi02 block device (interleaved flash devices similar to pflash_cfi01, multi-sector erase, nonuniform sector sizes, and some bug fixes and I'm planning on implementing sector erase suspend/resume commands in the near future).
> 
> Any chance you could do multiple region support, too?

Can you point me at the data sheet for a flash chip with multiple region support? For my purposes, I only need the features I mentioned, but if it's a simple change, I'll consider it.

>>> QTestState *qts;
>>> qts = qtest_initf(" qemu-system-arm -M musicpal,accel=qtest "
>>>               "-drive if=pflash,file=%s,format=raw", filename);
>> 
>> If I do that, will it be possible for the test to override the properties set by pflash_cfi02_register? It looks like I should be able to use -global to set properties that aren't set explicitly.
> 
> Yes.
> 
> Won't work for properties set by pflash_cfi02_register(), though.  To
> test the full range of values there, you'd have to make them
> configurable somehow.  We currently don't have a good way to do that.
> Please see
> 
>    Subject: Re: Configuring pflash devices for OVMF firmware
>    Message-ID: <87mun8gd2x.fsf@dusky.pond.sub.org>
>    https://lists.nongnu.org/archive/html/qemu-devel/2019-02/msg01734.html

I see. That's too bad.

-- 
Stephen Checkoway

Re: [Qemu-devel] Testing sysbus devices

[Markus Armbruster]

Stephen Checkoway <stephen.checkoway@oberlin.edu> writes:

> On Feb 18, 2019, at 13:08, Markus Armbruster <armbru@redhat.com> wrote:
>
>> Stephen Checkoway <stephen.checkoway@oberlin.edu> writes:
>> 
>>> On Feb 18, 2019, at 08:43, Thomas Huth <thuth@redhat.com> wrote:
>>> 
>>>> On 18/02/2019 07.07, Stephen Checkoway wrote:
>>>>> Hi all,
>>>>> 
>>>>> I've been working on some improvements to the pflash_cfi02 block device (interleaved flash devices similar to pflash_cfi01, multi-sector erase, nonuniform sector sizes, and some bug fixes and I'm planning on implementing sector erase suspend/resume commands in the near future).
>> 
>> Any chance you could do multiple region support, too?
>
> Can you point me at the data sheet for a flash chip with multiple region support? For my purposes, I only need the features I mentioned, but if it's a simple change, I'll consider it.

I'm not familiar with CFI pflash, but I can operate a search engine.
Have a look at page 27 and 56 of

https://media.digikey.com/pdf/Data%20Sheets/Intel%20PDFs/28F160C3,320C3,640C3,800C3%20(x16).pdf

and tell us whether it's helpful.

>>>> QTestState *qts;
>>>> qts = qtest_initf(" qemu-system-arm -M musicpal,accel=qtest "
>>>>               "-drive if=pflash,file=%s,format=raw", filename);
>>> 
>>> If I do that, will it be possible for the test to override the properties set by pflash_cfi02_register? It looks like I should be able to use -global to set properties that aren't set explicitly.
>> 
>> Yes.
>> 
>> Won't work for properties set by pflash_cfi02_register(), though.  To
>> test the full range of values there, you'd have to make them
>> configurable somehow.  We currently don't have a good way to do that.
>> Please see
>> 
>>    Subject: Re: Configuring pflash devices for OVMF firmware
>>    Message-ID: <87mun8gd2x.fsf@dusky.pond.sub.org>
>>    https://lists.nongnu.org/archive/html/qemu-devel/2019-02/msg01734.html
>
> I see. That's too bad.

I think a test would be quite welcome even if it only tests what's
testable now with reasonable effort.

Re: [Qemu-devel] Testing sysbus devices

[Stephen Checkoway]

On Feb 19, 2019, at 01:09, Markus Armbruster <armbru@redhat.com> wrote:

> Stephen Checkoway <stephen.checkoway@oberlin.edu> writes:
> 
>> On Feb 18, 2019, at 13:08, Markus Armbruster <armbru@redhat.com> wrote:
>> 
>>> Any chance you could do multiple region support, too?
>> 
>> Can you point me at the data sheet for a flash chip with multiple region support? For my purposes, I only need the features I mentioned, but if it's a simple change, I'll consider it.
> 
> I'm not familiar with CFI pflash, but I can operate a search engine.
> Have a look at page 27 and 56 of
> 
> https://media.digikey.com/pdf/Data%20Sheets/Intel%20PDFs/28F160C3,320C3,640C3,800C3%20(x16).pdf
> 
> and tell us whether it's helpful.

That's a flash chip that uses the Intel command set (pflash_cfi01.c in qemu), I'm only touching the AMD command set (pflash_cfi02.c). Also those pages seem to be about block locking, not multiple regions.

I'm not entirely sure what you meant by multiple regions. If you meant blocks* having different sizes (usually in the top or bottom of the flash as boot blocks), then I've implemented that (the nonuniform sectors* I mentioned in my first email).

* Some flash chips differentiate between sectors and blocks. E.g., the flash chip used by hw/arm/musicpal.c <http://ww1.microchip.com/downloads/en/DeviceDoc/SST39VF6401B-SST39VF6402B-64-Mbit-x16-Multi-Purpose-Flash-Plus-Data-Sheet-DS20005008C.pdf> has sectors and blocks with separate erase commands. Qemu treats these the same and does not support separate erase commands.

> I think a test would be quite welcome even if it only tests what's
> testable now with reasonable effort.

I used Thomas Huth's suggestion for testing the autoselect, CFI, sector erase, chip erase, programming, and reset commands. I'll see how much I can test.

-- 
Stephen Checkoway

Re: [Qemu-devel] Testing sysbus devices

[Markus Armbruster]

Stephen Checkoway <stephen.checkoway@oberlin.edu> writes:

> On Feb 19, 2019, at 01:09, Markus Armbruster <armbru@redhat.com> wrote:
>
>> Stephen Checkoway <stephen.checkoway@oberlin.edu> writes:
>> 
>>> On Feb 18, 2019, at 13:08, Markus Armbruster <armbru@redhat.com> wrote:
>>> 
>>>> Any chance you could do multiple region support, too?
>>> 
>>> Can you point me at the data sheet for a flash chip with multiple region support? For my purposes, I only need the features I mentioned, but if it's a simple change, I'll consider it.
>> 
>> I'm not familiar with CFI pflash, but I can operate a search engine.
>> Have a look at page 27 and 56 of
>> 
>> https://media.digikey.com/pdf/Data%20Sheets/Intel%20PDFs/28F160C3,320C3,640C3,800C3%20(x16).pdf
>> 
>> and tell us whether it's helpful.
>
> That's a flash chip that uses the Intel command set (pflash_cfi01.c in
> qemu), I'm only touching the AMD command set (pflash_cfi02.c). Also
> those pages seem to be about block locking, not multiple regions.
>
> I'm not entirely sure what you meant by multiple regions. If you meant
> blocks* having different sizes (usually in the top or bottom of the
> flash as boot blocks), then I've implemented that (the nonuniform
> sectors* I mentioned in my first email).
>
> * Some flash chips differentiate between sectors and blocks. E.g., the
> flash chip used by hw/arm/musicpal.c
> <http://ww1.microchip.com/downloads/en/DeviceDoc/SST39VF6401B-SST39VF6402B-64-Mbit-x16-Multi-Purpose-Flash-Plus-Data-Sheet-DS20005008C.pdf>
> has sectors and blocks with separate erase commands. Qemu treats these
> the same and does not support separate erase commands.

My terminology might be confused...

Let me backtrack a bit an explain my use case.  On physical PCs, the
single flash chip is commonly configured to have a read-only part and a
read/write part.  The read-only part holds UEFI code, and the read-write
part holds its persistent state.

Since our virtual flash chips lack this feature, our virtual PCs have
*two* of them: one configured read-only, and one configured read/write.
Cleaning that up would be nice.

The comment "It does not implement software data protection as found in
many real chips" in both pflash_cfi0*.c might be referring to this
missing feature.

>> I think a test would be quite welcome even if it only tests what's
>> testable now with reasonable effort.
>
> I used Thomas Huth's suggestion for testing the autoselect, CFI,
> sector erase, chip erase, programming, and reset commands. I'll see
> how much I can test.

Sounds good!

Re: [Qemu-devel] Testing sysbus devices

[Stephen Checkoway]

> On Feb 19, 2019, at 10:28, Markus Armbruster <armbru@redhat.com> wrote:
> 
> My terminology might be confused...
> 
> Let me backtrack a bit an explain my use case.  On physical PCs, the
> single flash chip is commonly configured to have a read-only part and a
> read/write part.  The read-only part holds UEFI code, and the read-write
> part holds its persistent state.
> 
> Since our virtual flash chips lack this feature, our virtual PCs have
> *two* of them: one configured read-only, and one configured read/write.
> Cleaning that up would be nice.
> 
> The comment "It does not implement software data protection as found in
> many real chips" in both pflash_cfi0*.c might be referring to this
> missing feature.

I understand now, thank you for explaining. I noticed the comments about software data protection in the code, but I didn't investigate.

From a quick look at <https://www.cypress.com/file/195291/download> Table 27 on page 8, I see there are at least 4 different protection modes. I think the most common one (based on my reading of a handful of data sheets for flash chips) is the high voltage one. Essentially, there are sector groups that can be locked/unlocked using high voltage. It seems easy enough to model this by configuring sectors as locked and refusing to erase or program them.

Software command locking would probably involve implementing a few additional commands.

I'm not sure what the others are.

Which locking method do you need?

-- 
Stephen Checkoway

Re: [Qemu-devel] Testing sysbus devices

[Markus Armbruster]

Stephen Checkoway <stephen.checkoway@oberlin.edu> writes:

>> On Feb 19, 2019, at 10:28, Markus Armbruster <armbru@redhat.com> wrote:
>> 
>> My terminology might be confused...
>> 
>> Let me backtrack a bit an explain my use case.  On physical PCs, the
>> single flash chip is commonly configured to have a read-only part and a
>> read/write part.  The read-only part holds UEFI code, and the read-write
>> part holds its persistent state.
>> 
>> Since our virtual flash chips lack this feature, our virtual PCs have
>> *two* of them: one configured read-only, and one configured read/write.
>> Cleaning that up would be nice.
>> 
>> The comment "It does not implement software data protection as found in
>> many real chips" in both pflash_cfi0*.c might be referring to this
>> missing feature.
>
> I understand now, thank you for explaining. I noticed the comments about software data protection in the code, but I didn't investigate.
>
>>From a quick look at <https://www.cypress.com/file/195291/download> Table 27 on page 8, I see there are at least 4 different protection modes. I think the most common one (based on my reading of a handful of data sheets for flash chips) is the high voltage one. Essentially, there are sector groups that can be locked/unlocked using high voltage. It seems easy enough to model this by configuring sectors as locked and refusing to erase or program them.
>
> Software command locking would probably involve implementing a few additional commands.
>
> I'm not sure what the others are.
>
> Which locking method do you need?

László, Philippe, what would you prefer to work with in OVMF?

Re: [Qemu-devel] Testing sysbus devices

[Laszlo Ersek]

On 02/19/19 18:55, Markus Armbruster wrote:
> Stephen Checkoway <stephen.checkoway@oberlin.edu> writes:
> 
>>> On Feb 19, 2019, at 10:28, Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> My terminology might be confused...
>>>
>>> Let me backtrack a bit an explain my use case.  On physical PCs, the
>>> single flash chip is commonly configured to have a read-only part and a
>>> read/write part.  The read-only part holds UEFI code, and the read-write
>>> part holds its persistent state.
>>>
>>> Since our virtual flash chips lack this feature, our virtual PCs have
>>> *two* of them: one configured read-only, and one configured read/write.
>>> Cleaning that up would be nice.
>>>
>>> The comment "It does not implement software data protection as found in
>>> many real chips" in both pflash_cfi0*.c might be referring to this
>>> missing feature.
>>
>> I understand now, thank you for explaining. I noticed the comments about software data protection in the code, but I didn't investigate.
>>
>> >From a quick look at <https://www.cypress.com/file/195291/download> Table 27 on page 8, I see there are at least 4 different protection modes. I think the most common one (based on my reading of a handful of data sheets for flash chips) is the high voltage one. Essentially, there are sector groups that can be locked/unlocked using high voltage. It seems easy enough to model this by configuring sectors as locked and refusing to erase or program them.
>>
>> Software command locking would probably involve implementing a few additional commands.
>>
>> I'm not sure what the others are.
>>
>> Which locking method do you need?
> 
> László, Philippe, what would you prefer to work with in OVMF?

I would strongly prefer if the guest-side view wouldn't change at all.

IOW, I don't have any useful input on extensions to the current command
set; what matters to me is that OVMF please not be forced to make use of
the new commands (and that the privilege differences wrt. SMM remain
functional). We've avoided version lock-in between OVMF and QEMU for a
great long time now, thanks to the ACPI linker/loader; I wouldn't like
to see version dependencies reintroduced in other areas.

Thanks
Laszlo

Re: [Qemu-devel] Testing sysbus devices

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> On 02/19/19 18:55, Markus Armbruster wrote:
>> Stephen Checkoway <stephen.checkoway@oberlin.edu> writes:
>> 
>>>> On Feb 19, 2019, at 10:28, Markus Armbruster <armbru@redhat.com> wrote:
>>>>
>>>> My terminology might be confused...
>>>>
>>>> Let me backtrack a bit an explain my use case.  On physical PCs, the
>>>> single flash chip is commonly configured to have a read-only part and a
>>>> read/write part.  The read-only part holds UEFI code, and the read-write
>>>> part holds its persistent state.
>>>>
>>>> Since our virtual flash chips lack this feature, our virtual PCs have
>>>> *two* of them: one configured read-only, and one configured read/write.
>>>> Cleaning that up would be nice.
>>>>
>>>> The comment "It does not implement software data protection as found in
>>>> many real chips" in both pflash_cfi0*.c might be referring to this
>>>> missing feature.
>>>
>>> I understand now, thank you for explaining. I noticed the comments about software data protection in the code, but I didn't investigate.
>>>
>>> >From a quick look at <https://www.cypress.com/file/195291/download> Table 27 on page 8, I see there are at least 4 different protection modes. I think the most common one (based on my reading of a handful of data sheets for flash chips) is the high voltage one. Essentially, there are sector groups that can be locked/unlocked using high voltage. It seems easy enough to model this by configuring sectors as locked and refusing to erase or program them.
>>>
>>> Software command locking would probably involve implementing a few additional commands.
>>>
>>> I'm not sure what the others are.
>>>
>>> Which locking method do you need?
>> 
>> László, Philippe, what would you prefer to work with in OVMF?
>
> I would strongly prefer if the guest-side view wouldn't change at all.
>
> IOW, I don't have any useful input on extensions to the current command
> set; what matters to me is that OVMF please not be forced to make use of
> the new commands (and that the privilege differences wrt. SMM remain
> functional). We've avoided version lock-in between OVMF and QEMU for a
> great long time now, thanks to the ACPI linker/loader; I wouldn't like
> to see version dependencies reintroduced in other areas.

My grasp on CFI pflash is somewhat shaky.  Philippe, Stephen, please
correct misunderstandings in the following.

We could improve the device model to let us configure a part of the
flash memory read-only.  We could use that to have just one pflash
device suitably configured instead of two.

For guest software that merely reads and writes the memory, no visible
change.

To support updating firmware from the guest, we'd have to implement a
suitable guest-controlled protection mode, but that's not on the table
right now.

Re: [Qemu-devel] Testing sysbus devices

[Stephen Checkoway]

> On Feb 20, 2019, at 03:55, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> I would strongly prefer if the guest-side view wouldn't change at all.

It sounds like sector protection isn't something you want and it's not something I currently need so unless that changes, I probably won't do anything with it.

My goal is merely to implement some missing flash functionality that I need to emulate some hardware that I have. My plan for doing this is to not change any defaults (except for a few bug fixes) while doing so. I'm happy for the qemu community to take as much or as little as it finds useful.

I'll send a patch series for review in the normal fashion, but if anyone wants to see my in-progress work, including tests, the diff is available here <https://github.com/qemu/qemu/compare/master...stevecheckoway:pflash02>.

For my own edification, I'm curious how you're currently dealing with some regions of flash that are protected. I believe Markus mentioned using multiple flash devices. Are you overlapping the address ranges?

The current pflash_cfi02.c code assumes, but doesn't check that both the total size of the chip as well as the size of each sector is a power of two. If you wanted say 7 MB of read/write flash and 1 MB of read-only flash, qemu might be willing to create a device with say 7 MB of storage, but it will definitely misbehave. (I added a check for that here <https://github.com/qemu/qemu/compare/master...stevecheckoway:pflash02#diff-d33881bd0ef099e2f46ebd4797c653bcR738>.)

Cheers,

Steve

-- 
Stephen Checkoway

Re: [Qemu-devel] Testing sysbus devices

[Markus Armbruster]

Stephen Checkoway <stephen.checkoway@oberlin.edu> writes:

>> On Feb 20, 2019, at 03:55, Laszlo Ersek <lersek@redhat.com> wrote:
>> 
>> I would strongly prefer if the guest-side view wouldn't change at all.
>
> It sounds like sector protection isn't something you want and it's not

László is content with the status quo, but I'm not.

> something I currently need so unless that changes, I probably won't do
> anything with it.

Pity.

> My goal is merely to implement some missing flash functionality that I
> need to emulate some hardware that I have. My plan for doing this is
> to not change any defaults (except for a few bug fixes) while doing
> so. I'm happy for the qemu community to take as much or as little as
> it finds useful.

Understand.

> I'll send a patch series for review in the normal fashion, but if
> anyone wants to see my in-progress work, including tests, the diff is
> available here
> <https://github.com/qemu/qemu/compare/master...stevecheckoway:pflash02>.
>
> For my own edification, I'm curious how you're currently dealing with
> some regions of flash that are protected. I believe Markus mentioned
> using multiple flash devices. Are you overlapping the address ranges?

UEFI wants to store some persistent state in flash memory.  Real PCs
have a single flash chip with a suitable part configured to be writable
for firmware.

Since our flash device models can't do that (yet?), we worked around the
missing functionality by exposing two separate flash chips to guests,
one read-only, one writable for firmware.  The two are adjacent, no gap,
with the boundary aligned to 4KiB (page size).

Our track record for doing whatever real hardware does has been okay.
The track record for our own good-enough inventions less so.  I'm not
claiming this one is about to explode into our faces.  Still, I'd like
to clean it up if practical.  If not for PCs (say because complications
for OVMF render that less than practical), then at least for other, less
encumbered machines.

Would be nice if you could pitch in a bit.

Way, way more than you ever wanted to know on configuring flash for PCs:

    Subject: Configuring pflash devices for OVMF firmware
    Message-ID: <87y378n5iy.fsf@dusky.pond.sub.org>
    https://lists.nongnu.org/archive/html/qemu-devel/2019-01/msg06606.html

> The current pflash_cfi02.c code assumes, but doesn't check that both
> the total size of the chip as well as the size of each sector is a
> power of two. If you wanted say 7 MB of read/write flash and 1 MB of
> read-only flash, qemu might be willing to create a device with say 7
> MB of storage, but it will definitely misbehave. (I added a check for
> that here
> <https://github.com/qemu/qemu/compare/master...stevecheckoway:pflash02#diff-d33881bd0ef099e2f46ebd4797c653bcR738>.)

Awesome.  The magic setup code in hw/i386/pc_sysfw.c will happily create
any size that's a multiple of 4KiB.  The current sizes are 128KiB
writable (power of two, good) and 2MiB - 128KiB for read-only (very much
not a power of two, possibly bad).

Can you tell us a bit more about what exactly can go wrong?

Re: [Qemu-devel] Testing sysbus devices

[Laszlo Ersek]

On 02/22/19 08:42, Markus Armbruster wrote:
> Stephen Checkoway <stephen.checkoway@oberlin.edu> writes:
> 
>>> On Feb 20, 2019, at 03:55, Laszlo Ersek <lersek@redhat.com> wrote:
>>>
>>> I would strongly prefer if the guest-side view wouldn't change at all.
>>
>> It sounds like sector protection isn't something you want and it's not
> 
> László is content with the status quo, but I'm not.

I think that's an accurate description. It means that I don't personally
pursue cleaning up this stuff. Obviously I'm also not trying to prevent
a cleanup! The only things I'd like to prevent are (a) regressions, (b)
introducing a version dependency between QEMU and OVMF in this area.

In the past we've dealt with similar issues via feature negotiation, as
long as the feature can be dynamically configured in the guest.
Unfortunately, pflash size / structure are one set of things that the
firmware can't configure dynamically.

>> something I currently need so unless that changes, I probably won't do
>> anything with it.
> 
> Pity.
> 
>> My goal is merely to implement some missing flash functionality that I
>> need to emulate some hardware that I have. My plan for doing this is
>> to not change any defaults (except for a few bug fixes) while doing
>> so. I'm happy for the qemu community to take as much or as little as
>> it finds useful.
> 
> Understand.
> 
>> I'll send a patch series for review in the normal fashion, but if
>> anyone wants to see my in-progress work, including tests, the diff is
>> available here
>> <https://github.com/qemu/qemu/compare/master...stevecheckoway:pflash02>.
>>
>> For my own edification, I'm curious how you're currently dealing with
>> some regions of flash that are protected. I believe Markus mentioned
>> using multiple flash devices. Are you overlapping the address ranges?
> 
> UEFI wants to store some persistent state in flash memory.  Real PCs
> have a single flash chip with a suitable part configured to be writable
> for firmware.
> 
> Since our flash device models can't do that (yet?), we worked around the
> missing functionality by exposing two separate flash chips to guests,
> one read-only, one writable for firmware.  The two are adjacent, no gap,
> with the boundary aligned to 4KiB (page size).
> 
> Our track record for doing whatever real hardware does has been okay.
> The track record for our own good-enough inventions less so.  I'm not
> claiming this one is about to explode into our faces.  Still, I'd like
> to clean it up if practical.  If not for PCs (say because complications
> for OVMF render that less than practical), then at least for other, less
> encumbered machines.
> 
> Would be nice if you could pitch in a bit.
> 
> Way, way more than you ever wanted to know on configuring flash for PCs:
> 
>     Subject: Configuring pflash devices for OVMF firmware
>     Message-ID: <87y378n5iy.fsf@dusky.pond.sub.org>
>     https://lists.nongnu.org/archive/html/qemu-devel/2019-01/msg06606.html
> 
>> The current pflash_cfi02.c code assumes, but doesn't check that both
>> the total size of the chip as well as the size of each sector is a
>> power of two. If you wanted say 7 MB of read/write flash and 1 MB of
>> read-only flash, qemu might be willing to create a device with say 7
>> MB of storage, but it will definitely misbehave. (I added a check for
>> that here
>> <https://github.com/qemu/qemu/compare/master...stevecheckoway:pflash02#diff-d33881bd0ef099e2f46ebd4797c653bcR738>.)
> 
> Awesome.  The magic setup code in hw/i386/pc_sysfw.c will happily create
> any size that's a multiple of 4KiB.  The current sizes are 128KiB
> writable (power of two, good) and 2MiB - 128KiB for read-only (very much
> not a power of two, possibly bad).

Correct, wrt. the images shipped in Fedora. The upstream edk2/OVMF
default (also used in RHEL) is 528KB + 3568KB = 4MB. Upstream also
supports building for 128KB + 896KB = 1MB.

Thanks!
Laszlo

> 
> Can you tell us a bit more about what exactly can go wrong?
> 

Re: [Qemu-devel] Testing sysbus devices

[Stephen Checkoway]

> On Feb 22, 2019, at 02:42, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Awesome.  The magic setup code in hw/i386/pc_sysfw.c will happily create
> any size that's a multiple of 4KiB.  The current sizes are 128KiB
> writable (power of two, good) and 2MiB - 128KiB for read-only (very much
> not a power of two, possibly bad).

As far as I can tell, the code I'm touching does not even compile by default for the i386 which only supports flash chips with the Intel command set hw/block/pflash_cfi01.c and not the flash chips with the AMD command set hw/block/pflash_cfi02.c.

> Can you tell us a bit more about what exactly can go wrong?

I don't know anything at all about the pflash_cfi01.c code. So if it's working now, then it's probably fine.

If you had been using pflash_cfi02.c, the problem would be that it has support for mapping the chip at multiple, consecutive locations (I'm not sure why that's part of the device itself and not something setup by code that's using the device). So device accesses mask offsets in the read and write functions by pfl->chip_len - 1 (here <https://github.com/qemu/qemu/blob/master/hw/block/pflash_cfi02.c#L154> and here <https://github.com/qemu/qemu/blob/master/hw/block/pflash_cfi02.c#L279>). If the chip size isn't a power of two, this breaks.

-- 
Stephen Checkoway

Re: [Qemu-devel] Testing sysbus devices

[Laszlo Ersek]

On 02/21/19 20:57, Stephen Checkoway wrote:
> 
> 
>> On Feb 20, 2019, at 03:55, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> I would strongly prefer if the guest-side view wouldn't change at
>> all.
> 
> It sounds like sector protection isn't something you want and it's not
> something I currently need so unless that changes, I probably won't do
> anything with it.
> 
> My goal is merely to implement some missing flash functionality that I
> need to emulate some hardware that I have. My plan for doing this is
> to not change any defaults (except for a few bug fixes) while doing
> so. I'm happy for the qemu community to take as much or as little as
> it finds useful.
> 
> I'll send a patch series for review in the normal fashion, but if
> anyone wants to see my in-progress work, including tests, the diff is
> available here
> <https://github.com/qemu/qemu/compare/master...stevecheckoway:pflash02>.
> 
> For my own edification, I'm curious how you're currently dealing with
> some regions of flash that are protected.

We aren't. The firmware has build-time constants that describe (a) what
GPA range stands for the varstore area (so that it is writeable at least
in SMM), and (b) what other range stands for the code area (which the
firmware never attempts to write). These ranges are adjacent.

> I believe Markus mentioned using multiple flash devices.

Yes. The firmware only has assumptions about GPA ranges, and
expectations about writeability. Originally it targeted a single r/w
chip, but it was possible to split that chip in two (one r/w and another
r/o) without the firmware noticing or caring.

> Are you overlapping the address ranges?

No, we're not.

> The current pflash_cfi02.c code assumes, but doesn't check that both
> the total size of the chip as well as the size of each sector is a
> power of two. If you wanted say 7 MB of read/write flash and 1 MB of
> read-only flash, qemu might be willing to create a device with say 7
> MB of storage, but it will definitely misbehave. (I added a check for
> that here
> <https://github.com/qemu/qemu/compare/master...stevecheckoway:pflash02#diff-d33881bd0ef099e2f46ebd4797c653bcR738>.)

OVMF and the q35/i440fx boards use cfi01. The 4KB sector size is assumed
by both QEMU board code and OVMF. The 4KB sector size is not assumed (to
my knowledge) by cfi01.pflash device model code.

Regarding the full size of each cfi01.pflash chip, i440fx/q35 board code
determines that dynamically from the file size (the only requirement is
that the size be a multiple of the sector size). There is no assumption
in the device model. In OVMF, the assumed/expected full size is
hard-coded (build-time constant).

Thanks
Laszlo

Re: [Qemu-devel] Testing sysbus devices

[Stephen Checkoway]

> On Feb 22, 2019, at 02:55, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> OVMF and the q35/i440fx boards use cfi01. The 4KB sector size is assumed
> by both QEMU board code and OVMF. The 4KB sector size is not assumed (to
> my knowledge) by cfi01.pflash device model code.
> 
> Regarding the full size of each cfi01.pflash chip, i440fx/q35 board code
> determines that dynamically from the file size (the only requirement is
> that the size be a multiple of the sector size). There is no assumption
> in the device model. In OVMF, the assumed/expected full size is
> hard-coded (build-time constant).

Gotcha, thanks for the info. It sounds like the code for the Intel command set chips (cfi01) and the AMD command set chips (cfi02) have different requirements for their use.

Regards,

Steve

-- 
Stephen Checkoway