| Message ID | 1512031988-32490-4-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 30 November 2017 at 08:53, Thomas Huth <thuth@redhat.com> wrote: > We can output a character quite easily here with some few lines of > assembly that we provide as a mini-kernel for this board. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > tests/boot-serial-test.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c > index d997269..dd3828c 100644 > --- a/tests/boot-serial-test.c > +++ b/tests/boot-serial-test.c > @@ -16,6 +16,14 @@ > #include "qemu/osdep.h" > #include "libqtest.h" > > +static const uint8_t kernel_mcf5208[] = { > + 0x41, 0xf9, 0xfc, 0x06, 0x00, 0x00, /* lea 0xfc060000,%a0 */ > + 0x10, 0x3c, 0x00, 0x54, /* move.b #'T',%d0 */ > + 0x11, 0x7c, 0x00, 0x04, 0x00, 0x08, /* move.b #4,8(%a0) Enable TX */ > + 0x11, 0x40, 0x00, 0x0c, /* move.b %d0,12(%a0) Print 'T' */ > + 0x60, 0xfa /* bra.s loop */ > +}; This approach doesn't seem to be scalable to me -- are we really going to have 50 or more fragments of hand-coded hex in this file to cover the various board models? I'd much rather see us have a framework for being able to build test blobs from source using a cross compiler setup (and docker or similar so anybody can rebuild the test blobs). That will be much easier to maintain and easier to extend to having tests that test other parts of the board or other aspects of TCG emulation. thanks -- PMM
On 30.11.2017 13:14, Peter Maydell wrote: > On 30 November 2017 at 08:53, Thomas Huth <thuth@redhat.com> wrote: >> We can output a character quite easily here with some few lines of >> assembly that we provide as a mini-kernel for this board. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> tests/boot-serial-test.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c >> index d997269..dd3828c 100644 >> --- a/tests/boot-serial-test.c >> +++ b/tests/boot-serial-test.c >> @@ -16,6 +16,14 @@ >> #include "qemu/osdep.h" >> #include "libqtest.h" >> >> +static const uint8_t kernel_mcf5208[] = { >> + 0x41, 0xf9, 0xfc, 0x06, 0x00, 0x00, /* lea 0xfc060000,%a0 */ >> + 0x10, 0x3c, 0x00, 0x54, /* move.b #'T',%d0 */ >> + 0x11, 0x7c, 0x00, 0x04, 0x00, 0x08, /* move.b #4,8(%a0) Enable TX */ >> + 0x11, 0x40, 0x00, 0x0c, /* move.b %d0,12(%a0) Print 'T' */ >> + 0x60, 0xfa /* bra.s loop */ >> +}; > > This approach doesn't seem to be scalable to me -- are we > really going to have 50 or more fragments of hand-coded hex in > this file to cover the various board models? No, since this only works for certain few boards with these criteria: 1) There must be a way to load such small blobs either with -kernel or -bios. Many boards only support loading kernels in the ELF format, and we certainly don't want to encode the hex-dump of such a kernel here. Or some boards / CPUs also support -bios, but the entry point must be at the end of a large image, so that also does not make sense to include them here. 2) The UART must be usable with some few lines of assembly. That's also not always the case. So no, this is really not meant to scale to all boards, it's just meant to get at least some few more test coverage, especially while we're not having other test mechanisms in place yet. (e.g. how long are we talking about reviving tests/tcg/ again already? ... some attempts have been made to make it compilable again, but non of the patch series has been included yet so far) > I'd much rather see us have a framework for being able > to build test blobs from source using a cross compiler > setup (and docker or similar so anybody can rebuild > the test blobs). That will be much easier to maintain > and easier to extend to having tests that test other > parts of the board or other aspects of TCG emulation. I agree that this idea is way more flexible and could cover many more boards, but there are also things that need to be solved first: Not everybody has docker / cross-compilers installed or can use them, so we likely need a set of pre-build binary images somewhere. But we certainly don't want to include megabytes of blobs in the git repository ... so this would need to go into an external repository instead. Then you likely can not include this in "make check" so easily anymore (unless you force everybody to check out the external repository with the git-submodule.sh script - but I really dislike this idea) ... So yes, we should have better test coverage for almost all machines if possible, but I don't see that this is happening soon or could be really tightly integrated into "make check". I.e. the boot-serial tester could fill at least parts of this gap, I think. Thomas
On 30/11/2017 13:14, Peter Maydell wrote: > On 30 November 2017 at 08:53, Thomas Huth <thuth@redhat.com> wrote: >> +static const uint8_t kernel_mcf5208[] = { >> + 0x41, 0xf9, 0xfc, 0x06, 0x00, 0x00, /* lea 0xfc060000,%a0 */ >> + 0x10, 0x3c, 0x00, 0x54, /* move.b #'T',%d0 */ >> + 0x11, 0x7c, 0x00, 0x04, 0x00, 0x08, /* move.b #4,8(%a0) Enable TX */ >> + 0x11, 0x40, 0x00, 0x0c, /* move.b %d0,12(%a0) Print 'T' */ >> + 0x60, 0xfa /* bra.s loop */ >> +}; > > This approach doesn't seem to be scalable to me -- are we > really going to have 50 or more fragments of hand-coded hex in > this file to cover the various board models? > > I'd much rather see us have a framework for being able > to build test blobs from source using a cross compiler > setup (and docker or similar so anybody can rebuild > the test blobs). That will be much easier to maintain > and easier to extend to having tests that test other > parts of the board or other aspects of TCG emulation. It seems a bit overkill, as these snippets are ~16 bytes long. However, it would be useful to have a basic patching mechanism so that board descriptions could include a common hand-coded const array and place an address at a given offset. So you'd have struct HexFirmware { int patch_offset; short patch_size; bool patch_bigendian; uint8_t data[32]; } and microblaze boards could have: struct HexFirmware kernel_microblaze = { .patch_offset = 0, .patch_size = 2, .patch_bigendian = false, .data = { 0xaa, 0xaa, 0x00, 0xb0, /* imm 0x???? */ 0x00, 0x10, 0x60, 0x30, /* addik r3,r0,0x1000 */ 0x54, 0x00, 0x80, 0x30, /* addik r4,r0,'T' */ 0x00, 0x00, 0x83, 0xf0, /* sbi r4,r3,0 */ 0xfc, 0xff, 0x00, 0xb8, /* bri -4 loop */ } }; ... { "microblaze", "petalogix-s3adsp1800", "", "TT", kernel_microblaze, 0x8400 }, { "microblazeel", "petalogix-ml605", "", "TT", kernel_microblaze, 0x83a0 }, Likewise, you could have just two copies of the code for all ARM boards that have a pl011 (or any other UART with a simple byte-long transmit register), one 32-bit and one 64-bit. Thanks, Paolo
On 30.11.2017 13:40, Paolo Bonzini wrote: > On 30/11/2017 13:14, Peter Maydell wrote: >> On 30 November 2017 at 08:53, Thomas Huth <thuth@redhat.com> wrote: >>> +static const uint8_t kernel_mcf5208[] = { >>> + 0x41, 0xf9, 0xfc, 0x06, 0x00, 0x00, /* lea 0xfc060000,%a0 */ >>> + 0x10, 0x3c, 0x00, 0x54, /* move.b #'T',%d0 */ >>> + 0x11, 0x7c, 0x00, 0x04, 0x00, 0x08, /* move.b #4,8(%a0) Enable TX */ >>> + 0x11, 0x40, 0x00, 0x0c, /* move.b %d0,12(%a0) Print 'T' */ >>> + 0x60, 0xfa /* bra.s loop */ >>> +}; >> >> This approach doesn't seem to be scalable to me -- are we >> really going to have 50 or more fragments of hand-coded hex in >> this file to cover the various board models? >> >> I'd much rather see us have a framework for being able >> to build test blobs from source using a cross compiler >> setup (and docker or similar so anybody can rebuild >> the test blobs). That will be much easier to maintain >> and easier to extend to having tests that test other >> parts of the board or other aspects of TCG emulation. > > It seems a bit overkill, as these snippets are ~16 bytes long. > > However, it would be useful to have a basic patching mechanism so that > board descriptions could include a common hand-coded const array and > place an address at a given offset. So you'd have > > struct HexFirmware { > int patch_offset; > short patch_size; > bool patch_bigendian; > uint8_t data[32]; > } > > and microblaze boards could have: > > struct HexFirmware kernel_microblaze = { > .patch_offset = 0, > .patch_size = 2, > .patch_bigendian = false, > .data = { > 0xaa, 0xaa, 0x00, 0xb0, /* imm 0x???? */ > 0x00, 0x10, 0x60, 0x30, /* addik r3,r0,0x1000 */ > 0x54, 0x00, 0x80, 0x30, /* addik r4,r0,'T' */ > 0x00, 0x00, 0x83, 0xf0, /* sbi r4,r3,0 */ > 0xfc, 0xff, 0x00, 0xb8, /* bri -4 loop */ > } > }; > > ... > > { "microblaze", "petalogix-s3adsp1800", "", "TT", > kernel_microblaze, 0x8400 }, > { "microblazeel", "petalogix-ml605", "", "TT", > kernel_microblaze, 0x83a0 }, The two micrablaze data arrays are completely different, since one is big endian, the other is little, so I'd need to byte-swap the whole array on the fly, too. Not sure whether it makes sense to add such complex code just to safe 16 bytes of blob data...? > Likewise, you could have just two copies of the code for all ARM boards > that have a pl011 (or any other UART with a simple byte-long transmit > register), one 32-bit and one 64-bit. OK, having a patching mechanism in place likely makes sense as soon as we want to include multiple ARM boards here. I can do that, but I'd rather like to do it as a second step. It was already quite hard work to come up with all these assembler snippets (from CPUs where I don't have a clue of) and to determine which machines could be tested this way at all, so I'd first like to wait and see whether this patch series is acceptable at all or not, since Peter has objections. Thomas
On 30 November 2017 at 12:40, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 30/11/2017 13:14, Peter Maydell wrote: >> On 30 November 2017 at 08:53, Thomas Huth <thuth@redhat.com> wrote: >>> +static const uint8_t kernel_mcf5208[] = { >>> + 0x41, 0xf9, 0xfc, 0x06, 0x00, 0x00, /* lea 0xfc060000,%a0 */ >>> + 0x10, 0x3c, 0x00, 0x54, /* move.b #'T',%d0 */ >>> + 0x11, 0x7c, 0x00, 0x04, 0x00, 0x08, /* move.b #4,8(%a0) Enable TX */ >>> + 0x11, 0x40, 0x00, 0x0c, /* move.b %d0,12(%a0) Print 'T' */ >>> + 0x60, 0xfa /* bra.s loop */ >>> +}; >> >> This approach doesn't seem to be scalable to me -- are we >> really going to have 50 or more fragments of hand-coded hex in >> this file to cover the various board models? >> >> I'd much rather see us have a framework for being able >> to build test blobs from source using a cross compiler >> setup (and docker or similar so anybody can rebuild >> the test blobs). That will be much easier to maintain >> and easier to extend to having tests that test other >> parts of the board or other aspects of TCG emulation. > > It seems a bit overkill, as these snippets are ~16 bytes long. They're 16 bytes long because that's about the limit of what you can do with this approach. The consequence is that they barely test anything at all. A more sensible framework would allow: * better testing of TCG instructions more generally * writing your test cases in C * more interesting board dependent tests * "integration test" setups like 'boot entire kernel' * etc thanks -- PMM
On 30/11/2017 13:51, Peter Maydell wrote: > On 30 November 2017 at 12:40, Paolo Bonzini <pbonzini@redhat.com> wrote: >> On 30/11/2017 13:14, Peter Maydell wrote: >>> On 30 November 2017 at 08:53, Thomas Huth <thuth@redhat.com> wrote: >>>> +static const uint8_t kernel_mcf5208[] = { >>>> + 0x41, 0xf9, 0xfc, 0x06, 0x00, 0x00, /* lea 0xfc060000,%a0 */ >>>> + 0x10, 0x3c, 0x00, 0x54, /* move.b #'T',%d0 */ >>>> + 0x11, 0x7c, 0x00, 0x04, 0x00, 0x08, /* move.b #4,8(%a0) Enable TX */ >>>> + 0x11, 0x40, 0x00, 0x0c, /* move.b %d0,12(%a0) Print 'T' */ >>>> + 0x60, 0xfa /* bra.s loop */ >>>> +}; >>> >>> This approach doesn't seem to be scalable to me -- are we >>> really going to have 50 or more fragments of hand-coded hex in >>> this file to cover the various board models? >>> >>> I'd much rather see us have a framework for being able >>> to build test blobs from source using a cross compiler >>> setup (and docker or similar so anybody can rebuild >>> the test blobs). That will be much easier to maintain >>> and easier to extend to having tests that test other >>> parts of the board or other aspects of TCG emulation. >> >> It seems a bit overkill, as these snippets are ~16 bytes long. > > They're 16 bytes long because that's about the limit of > what you can do with this approach. The consequence is that > they barely test anything at all. Certainly they are an awful test for boards, but they are a great smoke test for TCG changes that require modifications in all target/ subdirectories. The infrastructure you want for integration tests is already provided by kvm-unit-tests, which satisfies at least bullets 2-4 from your list below (the first is unclear to me). Thanks, Paolo > A more sensible framework > would allow: > * better testing of TCG instructions more generally > * writing your test cases in C > * more interesting board dependent tests > * "integration test" setups like 'boot entire kernel' > * etc > > thanks > -- PMM >
On 30/11/2017 13:51, Thomas Huth wrote: > The two micrablaze data arrays are completely different, since one is > big endian, the other is little, so I'd need to byte-swap the whole > array on the fly, too. Not sure whether it makes sense to add such > complex code just to safe 16 bytes of blob data...? *facepalm* :) >> Likewise, you could have just two copies of the code for all ARM boards >> that have a pl011 (or any other UART with a simple byte-long transmit >> register), one 32-bit and one 64-bit. > > OK, having a patching mechanism in place likely makes sense as soon as > we want to include multiple ARM boards here. I can do that, but I'd > rather like to do it as a second step. It was already quite hard work to > come up with all these assembler snippets (from CPUs where I don't have > a clue of) and to determine which machines could be tested this way at > all, so I'd first like to wait and see whether this patch series is > acceptable at all or not, since Peter has objections. Given your remark on endianness then no, having one snippet per binary is enough. Paolo
diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c index d997269..dd3828c 100644 --- a/tests/boot-serial-test.c +++ b/tests/boot-serial-test.c @@ -16,6 +16,14 @@ #include "qemu/osdep.h" #include "libqtest.h" +static const uint8_t kernel_mcf5208[] = { + 0x41, 0xf9, 0xfc, 0x06, 0x00, 0x00, /* lea 0xfc060000,%a0 */ + 0x10, 0x3c, 0x00, 0x54, /* move.b #'T',%d0 */ + 0x11, 0x7c, 0x00, 0x04, 0x00, 0x08, /* move.b #4,8(%a0) Enable TX */ + 0x11, 0x40, 0x00, 0x0c, /* move.b %d0,12(%a0) Print 'T' */ + 0x60, 0xfa /* bra.s loop */ +}; + typedef struct testdef { const char *arch; /* Target architecture */ const char *machine; /* Name of the machine */ @@ -41,6 +49,8 @@ static testdef_t tests[] = { { "x86_64", "q35", "-device sga", "SGABIOS" }, { "s390x", "s390-ccw-virtio", "-nodefaults -device sclpconsole,chardev=serial0", "virtio device" }, + { "m68k", "mcf5208evb", "", "TT", sizeof(kernel_mcf5208), kernel_mcf5208 }, + { NULL } };
We can output a character quite easily here with some few lines of assembly that we provide as a mini-kernel for this board. Signed-off-by: Thomas Huth <thuth@redhat.com> --- tests/boot-serial-test.c | 10 ++++++++++ 1 file changed, 10 insertions(+)