| Message ID | 20171213232025.24503-2-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | [01/26] sdbus: add a QMP command to access a SDBus | expand |
Am 14.12.2017 um 00:20 hat Philippe Mathieu-Daudé geschrieben: > Use Base64 to serialize the binary blobs in JSON. > So far at most 512 bytes will be transfered, which result > in a 684 bytes payload. > Since this command is intented for qtesting, this is acceptable. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Doing this kind of thing over QMP doesn't look right to me. qtests should access hardware the same way as real guests access the hardware (i.e. MMIO and I/O ports). But if for some reason the QMP maintainers were to think that this is acceptable in QMP, I'd argue it should at least get an x-debug- prefix to avoid making it a stable API that management tools may rely on. Kevin
On 14/12/2017 10:06, Kevin Wolf wrote: > Am 14.12.2017 um 00:20 hat Philippe Mathieu-Daudé geschrieben: >> Use Base64 to serialize the binary blobs in JSON. >> So far at most 512 bytes will be transfered, which result >> in a 684 bytes payload. >> Since this command is intented for qtesting, this is acceptable. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > Doing this kind of thing over QMP doesn't look right to me. qtests > should access hardware the same way as real guests access the hardware > (i.e. MMIO and I/O ports). > > But if for some reason the QMP maintainers were to think that this is > acceptable in QMP, I'd argue it should at least get an x-debug- prefix > to avoid making it a stable API that management tools may rely on. Yeah, what we usually do is not test the device (e.g. SCSI) directly, but only through the HBA (e.g. virtio-scsi or AHCI, it would be SDHCI in this case). Thanks, Paolo
Hi Kevin, On 12/14/2017 06:06 AM, Kevin Wolf wrote: > Am 14.12.2017 um 00:20 hat Philippe Mathieu-Daudé geschrieben: >> Use Base64 to serialize the binary blobs in JSON. >> So far at most 512 bytes will be transfered, which result >> in a 684 bytes payload. >> Since this command is intented for qtesting, this is acceptable. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > Doing this kind of thing over QMP doesn't look right to me. qtests > should access hardware the same way as real guests access the hardware > (i.e. MMIO and I/O ports). Yes, I agree with you, however this command does not implement a guest access behavior (MMIO and I/O ports) but a _bus_ access. Guests access buses via MMIO/IOP hardware frontend (bus master), bus slave devices might be considered as backend, like the BlockBackend. As the current iotests are meant for backend testing, this command is meant for SDBus backend testing. Actually with SDHCI I started to qtest the hardware frontend then realized the backend was incorrect, so I had to go this way to fix it. Later series do test the HCI using C qtests. This approach should works for any buses, and start to be quite interesting with: - hot-plug buses to unplug/plug slaves - multi-master buses like I2C to inject noise on the bus and see if the host can recover/continue - testing slave failures like a bricked SPI slave keeping some bus lines held and checking if the HCI expose this failure to the guest (or the guest checking the HCI for failures) > But if for some reason the QMP maintainers were to think that this is > acceptable in QMP, I'd argue it should at least get an x-debug- prefix > to avoid making it a stable API that management tools may rely on. I'd rather have the qtests using this command always run (if they take too long they might be tagged as 'slow' tests), so I'd keep this stable. Maybe we can prefix the qtests related QMP commands as "x-qtest-"? Else your suggestion is fine. Regards, Phil.
Hi Paolo, On 12/14/2017 06:34 AM, Paolo Bonzini wrote: > On 14/12/2017 10:06, Kevin Wolf wrote: [...] >> Doing this kind of thing over QMP doesn't look right to me. qtests >> should access hardware the same way as real guests access the hardware >> (i.e. MMIO and I/O ports). [...] > > Yeah, what we usually do is not test the device (e.g. SCSI) directly, > but only through the HBA (e.g. virtio-scsi or AHCI, it would be SDHCI in > this case). Yes, a SDHCI qtest is added in a later series (in C) using MMIO access: http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg02391.html But to be sure the SDHCI is correct I needed a SD slave to behave correctly ;) Hence this "bus test". Regards, Phil.
On 14/12/2017 14:25, Philippe Mathieu-Daudé wrote: > Yes, a SDHCI qtest is added in a later series (in C) using MMIO access: > http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg02391.html > > But to be sure the SDHCI is correct I needed a SD slave to behave > correctly ;) Hence this "bus test". You could also use the SPI interface for a lower-level test. Paolo
On 12/14/2017 03:06 AM, Kevin Wolf wrote: > Am 14.12.2017 um 00:20 hat Philippe Mathieu-Daudé geschrieben: >> Use Base64 to serialize the binary blobs in JSON. >> So far at most 512 bytes will be transfered, which result s/transfered/transferred/ >> in a 684 bytes payload. >> Since this command is intented for qtesting, this is acceptable. s/intented/intended/ >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > Doing this kind of thing over QMP doesn't look right to me. qtests > should access hardware the same way as real guests access the hardware > (i.e. MMIO and I/O ports). > > But if for some reason the QMP maintainers were to think that this is > acceptable in QMP, I'd argue it should at least get an x-debug- prefix > to avoid making it a stable API that management tools may rely on. I'm not convinced the command is needed, but agree that if we want it, it should have an 'x-debug-' prefix.
diff --git a/qapi-schema.json b/qapi-schema.json index 18457954a8..9b0fd90fed 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3200,3 +3200,44 @@ # Since: 2.11 ## { 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} } + +## +# @SDBusCommandResponse: +# +# SD Bus command response. +# +# @base64: the command response encoded as a Base64 string, if any (optional) +# +# Since: 2.11 +## +{ 'struct': 'SDBusCommandResponse', 'data': {'*base64': 'str'} } + +## +# @sdbus-command: +# +# Execute a command on a SD Bus return the response (if any). +# +# @qom-path: the SD Bus path +# @command: the SD protocol command to execute in the bus +# @arg: a 64-bit command argument (optional) +# @crc: the command/argument CRC (optional) +# +# Returns: the response of the command encoded as a Base64 string +# +# Since: 2.11 +# +# -> { "execute": "sdbus-command", +# "arguments": { "qom-path": "/machine/unattached/device[32]/sd.0", +# "command": 0x01 +# } +# } +# <- { "return": {'base64': 'A='} } +# +## +{ 'command': 'sdbus-command', + 'data': { 'qom-path': 'str', + 'command': 'uint8', + '*arg': 'uint64', + '*crc': 'uint16' }, + 'returns': 'SDBusCommandResponse' +} diff --git a/hw/sd/core.c b/hw/sd/core.c index da3a7e0efa..c546db2b22 100644 --- a/hw/sd/core.c +++ b/hw/sd/core.c @@ -22,6 +22,7 @@ #include "qemu/osdep.h" #include "hw/sd/sd.h" #include "qemu/cutils.h" +#include "qmp-commands.h" #include "sd-internal.h" #include "trace.h" @@ -220,3 +221,45 @@ SDBus *sdbus_create_bus(DeviceState *parent, const char *name) { return SD_BUS(qbus_create(TYPE_SD_BUS, parent, name)); } + +SDBusCommandResponse *qmp_sdbus_command(const char *qom_path, + uint8_t command, + bool has_arg, uint64_t arg, + bool has_crc, uint16_t crc, + Error **errp) +{ + uint8_t response[16 + 1]; + SDBusCommandResponse *res; + bool ambiguous = false; + Object *obj; + SDBus *sdbus; + int sz; + + obj = object_resolve_path(qom_path, &ambiguous); + if (obj == NULL) { + if (ambiguous) { + error_setg(errp, "Path '%s' is ambiguous", qom_path); + } else { + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", qom_path); + } + return NULL; + } + sdbus = (SDBus *)object_dynamic_cast(obj, TYPE_SD_BUS); + if (sdbus == NULL) { + error_set(errp, ERROR_CLASS_GENERIC_ERROR, + "Device '%s' not a sd-bus", qom_path); + return NULL; + } + + res = g_new0(SDBusCommandResponse, 1); + sz = sdbus_do_command(sdbus, + &(SDRequest){ command, arg, has_crc ? crc : -1 }, + response); + if (sz > 0) { + res->has_base64 = true; + res->base64 = g_base64_encode(response, sz); + } + + return res; +} diff --git a/stubs/qmp_sdbus.c b/stubs/qmp_sdbus.c new file mode 100644 index 0000000000..50f9f1410d --- /dev/null +++ b/stubs/qmp_sdbus.c @@ -0,0 +1,11 @@ +#include "qemu/osdep.h" +#include "qmp-commands.h" +#include "hw/sd/sd.h" + +SDBusCommandResponse *qmp_sdbus_command(const char *qom_path, uint8_t command, + bool has_arg, uint64_t arg, + bool has_crc, uint16_t crc, + Error **errp) +{ + return NULL; +} diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs index 8cfe34328a..a46cb3b992 100644 --- a/stubs/Makefile.objs +++ b/stubs/Makefile.objs @@ -35,6 +35,7 @@ stub-obj-y += vm-stop.o stub-obj-y += vmstate.o stub-obj-$(CONFIG_WIN32) += fd-register.o stub-obj-y += qmp_pc_dimm.o +stub-obj-y += qmp_sdbus.o stub-obj-y += target-monitor-defs.o stub-obj-y += target-get-monitor-def.o stub-obj-y += pc_madt_cpu_entry.o
Use Base64 to serialize the binary blobs in JSON. So far at most 512 bytes will be transfered, which result in a 684 bytes payload. Since this command is intented for qtesting, this is acceptable. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- qapi-schema.json | 41 +++++++++++++++++++++++++++++++++++++++++ hw/sd/core.c | 43 +++++++++++++++++++++++++++++++++++++++++++ stubs/qmp_sdbus.c | 11 +++++++++++ stubs/Makefile.objs | 1 + 4 files changed, 96 insertions(+) create mode 100644 stubs/qmp_sdbus.c