Re: [PATCH 10/28] qapi: Rework name checking in preparation of stricter checking
John Snow writes:
> On 3/23/21 5:40 AM, Markus Armbruster wrote:
>> Naming rules differ for the various kinds of names. To prepare
>> enforcing them, define functions to check them: check_name_upper(),
>> check_name_lower(), and check_name_camel(). For now, these merely
>> wrap around check_name_str(), but that will change shortly. Replace
>> the other uses of check_name_str() by appropriate uses of the
>> wrappers. No change in behavior just yet.
>> Signed-off-by: Markus Armbruster
>> ---
>> scripts/qapi/expr.py | 51 +++-
>> 1 file changed, 36 insertions(+), 15 deletions(-)
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index e00467636c..30285fe334 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -21,11 +21,12 @@
>> from .error import QAPISemError
>>
>> -# Names must be letters, numbers, -, and _. They must start with letter,
>> -# except for downstream extensions which must start with __RFQDN_.
>> -# Dots are only valid in the downstream extension prefix.
>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
>> -'[a-zA-Z][a-zA-Z0-9_-]*$')
>> +# Names consist of letters, digits, -, and _, starting with a letter.
>> +# An experimental name is prefixed with x-. A name of a downstream
>> +# extension is prefixed with __RFQDN_. The latter prefix goes first.
>> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
>> +r'(x-)?'
>> +r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)
>>
>> def check_name_is_str(name, info, source):
>> @@ -37,16 +38,38 @@ def check_name_str(name, info, source,
>> permit_upper=False):
>> # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
>> # and 'q_obj_*' implicit type names.
>> -if not valid_name.match(name) or \
>> - c_name(name, False).startswith('q_'):
>> +match = valid_name.match(name)
>> +if not match or c_name(name, False).startswith('q_'):
>> raise QAPISemError(info, "%s has an invalid name" % source)
>> if not permit_upper and name.lower() != name:
>> raise QAPISemError(
>> info, "%s uses uppercase in name" % source)
>> +return match.group(3)
>> +
>> +
>> +def check_name_upper(name, info, source):
>> +stem = check_name_str(name, info, source, permit_upper=True)
>> +# TODO reject '[a-z-]' in @stem
>> +
>
> Creates (presumably) temporary errors in flake8 for the dead
> assignment here and below.
All gone by the end of the series.
"make check" and checkpatch were content. Anything else you'd like me
to run?
Re: [PATCH 09/28] qapi: Lift enum-specific code out of check_name_str()
John Snow writes:
> On 3/23/21 5:40 AM, Markus Armbruster wrote:
>> check_name_str() masks leading digits when passed enum_member=True.
>> Only check_enum() does. Lift the masking into check_enum().
>> Signed-off-by: Markus Armbruster
>> ---
>> scripts/qapi/expr.py | 23 ++-
>> 1 file changed, 10 insertions(+), 13 deletions(-)
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index 507550c340..e00467636c 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -34,18 +34,11 @@ def check_name_is_str(name, info, source):
>>
>> def check_name_str(name, info, source,
>> - enum_member=False,
>> permit_upper=False):
>> -membername = name
>> -
>> -# Enum members can start with a digit, because the generated C
>> -# code always prefixes it with the enum name
>> -if enum_member and membername[0].isdigit():
>> -membername = 'D' + membername
>> # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
>> # and 'q_obj_*' implicit type names.
>> -if not valid_name.match(membername) or \
>> - c_name(membername, False).startswith('q_'):
>> +if not valid_name.match(name) or \
>> + c_name(name, False).startswith('q_'):
>> raise QAPISemError(info, "%s has an invalid name" % source)
>> if not permit_upper and name.lower() != name:
>> raise QAPISemError(
>> @@ -213,11 +206,15 @@ def check_enum(expr, info):
>> for m in members]
>> for member in members:
>> source = "'data' member"
>> +member_name = member['name']
>> check_keys(member, info, source, ['name'], ['if'])
>> -check_name_is_str(member['name'], info, source)
>> -source = "%s '%s'" % (source, member['name'])
>> -check_name_str(member['name'], info, source,
>> - enum_member=True, permit_upper=permit_upper)
>> +check_name_is_str(member_name, info, source)
>> +source = "%s '%s'" % (source, member_name)
>> +# Enum members may start with a digit
>> +if member_name[0].isdigit():
>> +member_name = 'd' + member_name # Hack: hide the digit
>
> Actually, can you put in one more space here? ^
Too late, pull request is out, I should've waited for your review.
We'll need to tidy up on top. I'm prone to this style mistake, because
Emacs M-; makes it.
>> +check_name_str(member_name, info, source,
>> + permit_upper=permit_upper)
>> check_if(member, info, source)
>>
>>
Re: [RFC PATCH 10/13] blobs: Only install s390x firmwares if s390x system target is built
On 23/03/2021 16.51, Philippe Mathieu-Daudé wrote:
Signed-off-by: Philippe Mathieu-Daudé
---
Cc: Cornelia Huck
Cc: Thomas Huth
Cc: qemu-s3...@nongnu.org
---
meson.build | 2 ++
pc-bios/meson.build | 9 +++--
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/meson.build b/meson.build
index 4b614b30ed9..fccda30ffc7 100644
--- a/meson.build
+++ b/meson.build
@@ -99,6 +99,7 @@
install_blobs_microblaze = false
install_blobs_ppc = false
install_blobs_riscv = false
+install_blobs_s390x = false
if get_option('install_blobs')
foreach target : target_dirs
install_edk2_blobs = install_edk2_blobs or target in edk2_targets
@@ -108,6 +109,7 @@
install_blobs_microblaze = install_blobs_microblaze or target in
['microblaze-softmmu', 'microblazeel-softmmu']
install_blobs_ppc = install_blobs_ppc or (target.startswith('ppc') and
target.endswith('softmmu'))
install_blobs_riscv = install_blobs_riscv or target in
['riscv32-softmmu', 'riscv64-softmmu']
+install_blobs_s390x = install_blobs_s390x or target in ['s390x-softmmu']
endforeach
endif
diff --git a/pc-bios/meson.build b/pc-bios/meson.build
index 504f03ec925..27c0f316dee 100644
--- a/pc-bios/meson.build
+++ b/pc-bios/meson.build
@@ -64,8 +64,6 @@
'linuxboot_dma.bin',
'kvmvapic.bin',
'pvh.bin',
- 's390-ccw.img',
- 's390-netboot.img',
))
if host_machine.system() == 'windows'
@@ -121,6 +119,13 @@
))
endif
+if install_blobs_s390x
+ blobs_ss.add(files(
+'s390-ccw.img',
+'s390-netboot.img',
+ ))
+endif
+
blobs_ss = blobs_ss.apply(config_host, strict: false)
if get_option('install_blobs')
Reviewed-by: Thomas Huth
Re: [PATCH] tests/migration: fix parameter of auto-converge migration
On 23/03/2021 20.40, Dr. David Alan Gilbert wrote:
* huang...@chinatelecom.cn (huang...@chinatelecom.cn) wrote:
From: Hyman Huang(黄勇)
when execute the following test command:
$ ./guestperf-batch.py --auto-converge \
--auto-converge-step {percent} ...
test aborts and error message be throwed as the following:
"Parameter 'x-cpu-throttle-increment' is unexpected"
The reason is that 'x-cpu-throttle-increment' has been
deprecated and 'cpu-throttle-increment' was introduced
Since v2.7. Use the new parameter instead.
Signed-off-by: Hyman Huang(黄勇)
Reviewed-by: Dr. David Alan Gilbert
Please cc th...@redhat.com and berra...@redhat.com on fixes to this
file.
This not really my turf, since it's not directly related to the qtests.
But I wonder why tests/migration/ is not listed in the "Migration" sections
in the MAINTAINERS file ... care to send a patch?
Thomas
Re: [PATCH qemu] spapr: Fix typo in the patb_entry comment
On 22/03/2021 16:44, David Gibson wrote:
On Thu, Feb 25, 2021 at 02:23:35PM +1100, Alexey Kardashevskiy wrote:
There is no H_REGISTER_PROCESS_TABLE, it is H_REGISTER_PROC_TBL handler
for which is still called h_register_process_table() though.
Signed-off-by: Alexey Kardashevskiy
Applied to ppc-for-6.0.
In future, best to CC me directly, since I only occasionally peruse
the lists.
even the qemu-...@nongnu.org list? okay :)
---
include/hw/ppc/spapr.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index ccbeeca1de84..8dceace4b442 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -168,7 +168,7 @@ struct SpaprMachineState {
SpaprResizeHpt resize_hpt;
void *htab;
uint32_t htab_shift;
-uint64_t patb_entry; /* Process tbl registed in H_REGISTER_PROCESS_TABLE */
+uint64_t patb_entry; /* Process tbl registed in H_REGISTER_PROC_TBL */
SpaprPendingHpt *pending_hpt; /* in-progress resize */
hwaddr rma_size;
--
Alexey
Re: [PATCH v3 3/3] spapr: nvdimm: Enable sync-dax device property for nvdimm
On Tue, Mar 23, 2021 at 09:47:55AM -0400, Shivaprasad G Bhat wrote:
> The patch adds the 'sync-dax' property to the nvdimm device.
>
> When the sync-dax is 'off', the device tree property
> "hcall-flush-required" is added to the nvdimm node which makes the
> guest to issue H_SCM_FLUSH hcalls to request for flushes explicitly.
> This would be the default behaviour without sync-dax property set
> for the nvdimm device.
>
> The sync-dax="on" would mean the guest need not make flush requests
> to the qemu. On previous machine versions the sync-dax is set to be
> "on" by default using the hw_compat magic.
>
> Signed-off-by: Shivaprasad G Bhat
> ---
> hw/core/machine.c |1 +
> hw/mem/nvdimm.c |1 +
> hw/ppc/spapr_nvdimm.c | 17 +
> include/hw/mem/nvdimm.h | 10 ++
> include/hw/ppc/spapr.h |1 +
> 5 files changed, 30 insertions(+)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 257a664ea2..f843643574 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -41,6 +41,7 @@ GlobalProperty hw_compat_5_2[] = {
> { "PIIX4_PM", "smm-compat", "on"},
> { "virtio-blk-device", "report-discard-granularity", "off" },
> { "virtio-net-pci", "vectors", "3"},
> +{ "nvdimm", "sync-dax", "on" },
> };
> const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
>
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 7397b67156..8f0e29b191 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -229,6 +229,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm,
> const void *buf,
>
> static Property nvdimm_properties[] = {
> DEFINE_PROP_BOOL(NVDIMM_UNARMED_PROP, NVDIMMDevice, unarmed, false),
> +DEFINE_PROP_BOOL(NVDIMM_SYNC_DAX_PROP, NVDIMMDevice, sync_dax, false),
I'm a bit uncomfortable adding this base NVDIMM property without at
least some logic about how it's handled on non-PAPR platforms.
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index 883317c1ed..dd1c90251b 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -125,6 +125,9 @@ static int spapr_dt_nvdimm(SpaprMachineState *spapr, void
> *fdt,
> uint64_t lsize = nvdimm->label_size;
> uint64_t size = object_property_get_int(OBJECT(nvdimm),
> PC_DIMM_SIZE_PROP,
> NULL);
> +bool sync_dax = object_property_get_bool(OBJECT(nvdimm),
> + NVDIMM_SYNC_DAX_PROP,
> + _abort);
>
> drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PMEM, slot);
> g_assert(drc);
> @@ -159,6 +162,11 @@ static int spapr_dt_nvdimm(SpaprMachineState *spapr,
> void *fdt,
> "operating-system")));
> _FDT(fdt_setprop(fdt, child_offset, "ibm,cache-flush-required", NULL,
> 0));
>
> +if (!sync_dax) {
> +_FDT(fdt_setprop(fdt, child_offset, "ibm,hcall-flush-required",
> + NULL, 0));
> +}
> +
> return child_offset;
> }
>
> @@ -567,10 +575,12 @@ static target_ulong h_scm_flush(PowerPCCPU *cpu,
> SpaprMachineState *spapr,
>target_ulong opcode, target_ulong
> *args)
> {
> int ret;
> +bool sync_dax;
> uint32_t drc_index = args[0];
> uint64_t continue_token = args[1];
> SpaprDrc *drc = spapr_drc_by_index(drc_index);
> PCDIMMDevice *dimm;
> +NVDIMMDevice *nvdimm;
> HostMemoryBackend *backend = NULL;
> SpaprNVDIMMDeviceFlushState *state;
> ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
> @@ -580,6 +590,13 @@ static target_ulong h_scm_flush(PowerPCCPU *cpu,
> SpaprMachineState *spapr,
> return H_PARAMETER;
> }
>
> +nvdimm = NVDIMM(drc->dev);
> +sync_dax = object_property_get_bool(OBJECT(nvdimm), NVDIMM_SYNC_DAX_PROP,
> +_abort);
> +if (sync_dax) {
> +return H_UNSUPPORTED;
Do you want to return UNSUPPORTED here, or just H_SUCCESS, since the
flush should be a no-op in this case.
> +}
> +
> if (continue_token != 0) {
> ret = spapr_nvdimm_get_flush_status(continue_token);
> if (H_IS_LONG_BUSY(ret)) {
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index bcf62f825c..f82979cf2f 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -51,6 +51,7 @@ OBJECT_DECLARE_TYPE(NVDIMMDevice, NVDIMMClass, NVDIMM)
> #define NVDIMM_LABEL_SIZE_PROP "label-size"
> #define NVDIMM_UUID_PROP "uuid"
> #define NVDIMM_UNARMED_PROP"unarmed"
> +#define NVDIMM_SYNC_DAX_PROP "sync-dax"
>
> struct NVDIMMDevice {
> /* private */
> @@ -85,6 +86,15 @@ struct NVDIMMDevice {
> */
> bool unarmed;
>
> +/*
> + * On PPC64,
> + * The 'off' value results in the hcall-flush-required property set
> + * in the device tree for
Re: [PATCH v3 2/3] spapr: nvdimm: Implement H_SCM_FLUSH hcall
On Tue, Mar 23, 2021 at 09:47:38AM -0400, Shivaprasad G Bhat wrote:
> The patch adds support for the SCM flush hcall for the nvdimm devices.
> To be available for exploitation by guest through the next patch.
>
> The hcall expects the semantics such that the flush to return
> with H_BUSY when the operation is expected to take longer time along
> with a continue_token. The hcall to be called again providing the
> continue_token to get the status. So, all fresh requsts are put into
> a 'pending' list and flush worker is submitted to the thread pool.
> The thread pool completion callbacks move the requests to 'completed'
> list, which are cleaned up after reporting to guest in subsequent
> hcalls to get the status.
>
> The semantics makes it necessary to preserve the continue_tokens
> and their return status even across migrations. So, the pre_save
> handler for the device waits for the flush worker to complete and
> collects all the hcall states from 'completed' list. The necessary
> nvdimm flush specific vmstate structures are added to the spapr
> machine vmstate.
>
> Signed-off-by: Shivaprasad G Bhat
An overal question: surely the same issue must arise on x86 with
file-backed NVDIMMs. How do they handle this case?
> ---
> hw/ppc/spapr.c|6 +
> hw/ppc/spapr_nvdimm.c | 240
> +
> include/hw/ppc/spapr.h| 11 ++
> include/hw/ppc/spapr_nvdimm.h | 12 ++
> 4 files changed, 268 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d56418ca29..fdb0c73a2c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1607,6 +1607,8 @@ static void spapr_machine_reset(MachineState *machine)
> spapr->ov5_cas = spapr_ovec_clone(spapr->ov5);
> }
>
> +spapr_nvdimm_finish_flushes();
> +
> /* DRC reset may cause a device to be unplugged. This will cause troubles
> * if this device is used by another device (eg, a running vhost backend
> * will crash QEMU if the DIMM holding the vring goes away). To avoid
> such
> @@ -2003,6 +2005,7 @@ static const VMStateDescription vmstate_spapr = {
> _spapr_cap_ccf_assist,
> _spapr_cap_fwnmi,
> _spapr_fwnmi,
> +_spapr_nvdimm_flush_states,
> NULL
> }
> };
> @@ -2997,6 +3000,9 @@ static void spapr_machine_init(MachineState *machine)
> }
>
> qemu_cond_init(>fwnmi_machine_check_interlock_cond);
> +qemu_mutex_init(>spapr_nvdimm_flush_states_lock);
Do you actually need an extra mutex, or can you rely on the BQL?
> +QLIST_INIT(>pending_flush_states);
> +QLIST_INIT(>completed_flush_states);
> }
>
> #define DEFAULT_KVM_TYPE "auto"
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index 8cf3fb2ffb..883317c1ed 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -22,14 +22,17 @@
> * THE SOFTWARE.
> */
> #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
> #include "qapi/error.h"
> #include "hw/ppc/spapr_drc.h"
> #include "hw/ppc/spapr_nvdimm.h"
> #include "hw/mem/nvdimm.h"
> +#include "qemu/guest-random.h"
> #include "qemu/nvdimm-utils.h"
> #include "hw/ppc/fdt.h"
> #include "qemu/range.h"
> #include "hw/ppc/spapr_numa.h"
> +#include "block/thread-pool.h"
>
> /*
> * The nvdimm size should be aligned to SCM block size.
> @@ -371,6 +374,242 @@ static target_ulong h_scm_bind_mem(PowerPCCPU *cpu,
> SpaprMachineState *spapr,
> return H_SUCCESS;
> }
>
> +static const VMStateDescription vmstate_spapr_nvdimm_entry = {
> + .name = "spapr_nvdimm_states",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT64(continue_token, SpaprNVDIMMDeviceFlushState),
> + VMSTATE_INT64(hcall_ret, SpaprNVDIMMDeviceFlushState),
> + VMSTATE_END_OF_LIST()
> + },
> +};
> +
> +static bool spapr_nvdimm_states_needed(void *opaque)
> +{
> + SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +
> + return (!QLIST_EMPTY(>pending_flush_states) ||
> + !QLIST_EMPTY(>completed_flush_states));
> +}
> +
> +static int spapr_nvdimm_pre_save(void *opaque)
> +{
> +SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +
> +while (!QLIST_EMPTY(>pending_flush_states)) {
> +aio_poll(qemu_get_aio_context(), true);
Hmm... how long could waiting for all the pending flushes to complete
take? This could add substanially to the guest's migration downtime,
couldn't it?
> +}
> +
> +return 0;
> +}
> +
> +const VMStateDescription vmstate_spapr_nvdimm_flush_states = {
> +.name = "spapr_nvdimm_hcall_states",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.needed = spapr_nvdimm_states_needed,
> +.pre_save = spapr_nvdimm_pre_save,
> +.fields = (VMStateField[]) {
> +VMSTATE_QLIST_V(completed_flush_states, SpaprMachineState, 1,
> +
Re: [PATCH v3 1/3] spapr: nvdimm: Forward declare and move the definitions
On Tue, Mar 23, 2021 at 09:47:23AM -0400, Shivaprasad G Bhat wrote:
> The subsequent patches add definitions which tend to
> get the compilation to cyclic dependency. So, prepare
> with forward declarations, move the defitions and clean up.
>
> Signed-off-by: Shivaprasad G Bhat
> ---
> hw/ppc/spapr_nvdimm.c | 12
> include/hw/ppc/spapr_nvdimm.h | 21 ++---
> 2 files changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index b46c36917c..8cf3fb2ffb 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -31,6 +31,18 @@
> #include "qemu/range.h"
> #include "hw/ppc/spapr_numa.h"
>
> +/*
> + * The nvdimm size should be aligned to SCM block size.
> + * The SCM block size should be aligned to SPAPR_MEMORY_BLOCK_SIZE
> + * inorder to have SCM regions not to overlap with dimm memory regions.
> + * The SCM devices can have variable block sizes. For now, fixing the
> + * block size to the minimum value.
> + */
> +#define SPAPR_MINIMUM_SCM_BLOCK_SIZE SPAPR_MEMORY_BLOCK_SIZE
> +
> +/* Have an explicit check for alignment */
> +QEMU_BUILD_BUG_ON(SPAPR_MINIMUM_SCM_BLOCK_SIZE % SPAPR_MEMORY_BLOCK_SIZE);
> +
> bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
> uint64_t size, Error **errp)
> {
> diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
> index 73be250e2a..abcacda5d7 100644
> --- a/include/hw/ppc/spapr_nvdimm.h
> +++ b/include/hw/ppc/spapr_nvdimm.h
> @@ -11,23 +11,14 @@
> #define HW_SPAPR_NVDIMM_H
>
> #include "hw/mem/nvdimm.h"
> -#include "hw/ppc/spapr.h"
>
> -/*
> - * The nvdimm size should be aligned to SCM block size.
> - * The SCM block size should be aligned to SPAPR_MEMORY_BLOCK_SIZE
> - * inorder to have SCM regions not to overlap with dimm memory regions.
> - * The SCM devices can have variable block sizes. For now, fixing the
> - * block size to the minimum value.
> - */
> -#define SPAPR_MINIMUM_SCM_BLOCK_SIZE SPAPR_MEMORY_BLOCK_SIZE
> -
> -/* Have an explicit check for alignment */
> -QEMU_BUILD_BUG_ON(SPAPR_MINIMUM_SCM_BLOCK_SIZE % SPAPR_MEMORY_BLOCK_SIZE);
> +struct SpaprDrc;
> +struct SpaprMachineState;
>
> -int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
> - void *fdt, int *fdt_start_offset, Error **errp);
> -void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt);
> +int spapr_pmem_dt_populate(struct SpaprDrc *drc,
Using explicit struct tags is against qemu coding style. You should
put a typedef on the forward decl so you don't need to do it here (see
examples in spapr_pci.c amongst other places).
> + struct SpaprMachineState *spapr, void *fdt,
> + int *fdt_start_offset, Error **errp);
> +void spapr_dt_persistent_memory(struct SpaprMachineState *spapr, void *fdt);
> bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
> uint64_t size, Error **errp);
> void spapr_add_nvdimm(DeviceState *dev, uint64_t slot);
>
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH v5 00/10] KVM: Dirty ring support (QEMU part)
Hi Peter, On 2021/3/23 22:34, Peter Xu wrote: > Keqian, > > On Tue, Mar 23, 2021 at 02:40:43PM +0800, Keqian Zhu wrote: The second question is that you observed longer migration time (55s->73s) when guest has 24G ram and dirty rate is 800M/s. I am not clear about the reason. As with dirty ring enabled, Qemu can get dirty info faster which means it handles dirty page more quick, and guest can be throttled which means dirty page is generated slower. What's the rationale for the longer migration time? >>> >>> Because dirty ring is more sensitive to dirty rate, while dirty bitmap is >>> more >> Emm... Sorry that I'm very clear about this... I think that higher dirty >> rate doesn't cause >> slower dirty_log_sync compared to that of legacy bitmap mode. Besides, >> higher dirty rate >> means we may have more full-exit, which can properly limit the dirty rate. >> So it seems that >> dirty ring "prefers" higher dirty rate. > > When I measured the 800MB/s it's in the guest, after throttling. > > Imagine another example: a VM has 1G memory keep dirtying with 10GB/s. Dirty > logging will need to collect even less for each iteration because memory size > shrinked, collect even less frequent due to the high dirty rate, however dirty > ring will use 100% cpu power to collect dirty pages because the ring keeps > full. Looks good. We have many places to collect dirty pages: the background reaper, vCPU exit handler, and the migration thread. I think migration time is closely related to the migration thread. The migration thread calls kvm_dirty_ring_flush(). 1. kvm_cpu_synchronize_kick_all() will wait vcpu handles full-exit. 2. kvm_dirty_ring_reap() collects and resets dirty pages. The above two operation will spend more time with higher dirty rate. But I suddenly realize that the key problem maybe not at this. Though we have separate "reset" operation for dirty ring, actually it is performed right after we collect dirty ring to kvmslot. So in dirty ring mode, it likes legacy bitmap mode without manual_dirty_clear. If we can "reset" dirty ring just before we really handle the dirty pages, we can have shorter migration time. But the design of dirty ring doesn't allow this, because we must perform reset to make free space... > >> >>> sensitive to memory footprint. In above 24G mem + 800MB/s dirty rate >>> condition, dirty bitmap seems to be more efficient, say, collecting dirty >>> bitmap of 24G mem (24G/4K/8=0.75MB) for each migration cycle is fast enough. >>> >>> Not to mention that current implementation of dirty ring in QEMU is not >>> complete - we still have two more layers of dirty bitmap, so it's actually a >>> mixture of dirty bitmap and dirty ring. This series is more like a POC on >>> dirty ring interface, so as to let QEMU be able to run on KVM dirty ring. >>> E.g., we won't have hang issue when getting dirty pages since it's totally >>> async, however we'll still have some legacy dirty bitmap issues e.g. memory >>> consumption of userspace dirty bitmaps are still linear to memory footprint. >> The plan looks good and coordinated, but I have a concern. Our dirty ring >> actually depends >> on the structure of hardware logging buffer (PML buffer). We can't say it >> can be properly >> adapted to all kinds of hardware design in the future. > > Sorry I don't get it - dirty ring can work with pure page wr-protect too? Sure, it can. I just want to discuss many possible kinds of hardware logging buffer. However, I'd like to stop at this, at least dirty ring works well with PML. :) > >> >>> >>> Moreover, IMHO another important feature that dirty ring provided is >>> actually >>> the full-exit, where we can pause a vcpu when it dirties too fast, while >>> other >> I think a proper pause time is hard to decide. Short time may have little >> effect >> of throttle, but long time may have heavy effect on guest. Do you have a >> good algorithm? > > That's the next thing we can discuss. IMHO I think the dirty ring is nice > already because we can measure dirty rate per-vcpu, also we can throttle in > vcpu granule. That's something required for a good algorithm, say we > shouldn't > block vcpu when there's small dirty rate, and in many cases that's the case > for > e.g. UI threads. Any algorithm should be based on these facts. OK. Thanks, Keqian
Re: [PATCH V13 2/9] meson.build: Re-enable KVM support for MIPS
Jiaxun Yang 于2021年3月24日周三 上午9:29写道： > > > > On Tue, Mar 23, 2021, at 9:56 PM, Philippe Mathieu-Daudé wrote: > > Hi Huacai, > > > > We are going to tag QEMU v6.0-rc0 today. > > > > I only have access to a 64-bit MIPS in little-endian to > > test KVM. > > > > Can you test the other configurations please? > > - 32-bit BE > > - 32-bit LE > > - 64-bit BE > How to run the test? just run a VM with KVM support on these kernel? > +syq > As Loongson doesn't have Big-Endian processor and Loongson 3A won't run 32bit > kernel. > > Probably wecan test on boston or malta board? > > Thanks. > > > > > > Thanks! > > > > Phil. > > > > > [...] > > -- > - Jiaxun
Re: [PATCH v4 07/17] target/ppc: Disconnect hflags from MSR
On Tue, Mar 23, 2021 at 11:04:03AM -0600, Richard Henderson wrote: > On 3/22/21 5:54 PM, David Gibson wrote: > > On Mon, Mar 22, 2021 at 10:55:46AM -0600, Richard Henderson wrote: > > > On 3/21/21 9:52 PM, David Gibson wrote: > > > > > +/* > > > > > + * Bits for env->hflags. > > > > > + * > > > > > + * Most of these bits overlap with corresponding bits in MSR, > > > > > + * but some come from other sources. Be cautious when modifying. > > > > > > > > Yeah.. I'm not sure "be cautious" is enough of a warning. The exact > > > > value of some but not all of these flags must equal that for the > > > > corresponding MSR bits, which is terrifyingly subtle. > > > > > > Fair. How about, for the comment here, "This is validated in > > > hreg_compute_hflags." > > > > > > > > +/* Some bits come straight across from MSR. */ > > > > > +msr_mask = ((1 << MSR_LE) | (1 << MSR_PR) | > > > > > +(1 << MSR_DR) | (1 << MSR_IR) | > > > > > +(1 << MSR_FP) | (1 << MSR_SA) | (1 << MSR_AP)); > > > > > > Here, and in every other spot within this function where we manipulate > > > msr_mask, > > > > > > QEMU_BUILD_BUG_ON(MSR_LE != HFLAGS_LE); > > > > Seems reasonable. > > Hah. Your paranoia was well-founded. Typo in HFLAGS_PR. :-) Heh :). -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH v10 6/7] hw/pci-host: Add emulation of Marvell MV64361 PPC system controller
On Tue, Mar 23, 2021 at 02:31:07PM +0100, BALATON Zoltan wrote:
> On Tue, 23 Mar 2021, David Gibson wrote:
> > On Wed, Mar 17, 2021 at 02:17:51AM +0100, BALATON Zoltan wrote:
[snip]
> > > +static void setup_mem_windows(MV64361State *s, uint32_t val)
> > > +{
> > > +MV64361PCIState *p;
> > > +MemoryRegion *mr;
> > > +uint32_t mask;
> > > +int i;
> > > +
> > > +val &= 0x1f;
> > > +for (mask = 1, i = 0; i < 21; i++, mask <<= 1) {
> >
> > Having a loop, where nearly the entire body is a switch over the loop
> > variable seems a rather odd choice to me, compared to just unrolling
> > the whole thing. Or alternatively, maybe more can be be factored out
> > of the switch into common body code.
>
> The loop is really over the bits in val that say which memory regions to
I see that, but it doesn't change the point.
> enable so depending on this we need to touch different mem regions. For
> selecting those memory regions and what to do with them a switch seems to be
> obvious choice. I probably can't factor out anything as these lines in
> switch cases are similar but all differ in some little details (like which
> PCI bus, name of the region, etc.). Unrolling could be possible but would
> just add lines between cases that's now in the loop head so I really don't
I see only 2 common lines, which basically balances about the case and
break lines in every switchcase.
> see why would that be better. Maybe renaming the loop variable from i to
> something that makes the intent clearer could help but I don't know what
> you'd like better. Can you suggest something?
>
> > > +if ((val & mask) != (s->base_addr_enable & mask)) {
> > > +trace_mv64361_region_enable(!(val & mask) ? "enable" :
> > > "disable", i);
> > > +switch (i) {
> > > +/*
> > > + * 0-3 are SDRAM chip selects but we map all RAM directly
> > > + * 4-7 are device chip selects (not sure what those are)
> > > + * 8 is Boot device (ROM) chip select but we map that
> > > directly too
> > > + */
> > > +case 9:
> > > +p = >pci[0];
> > > +mr = >cpu_win[i];
> > > +unmap_region(mr);
> > > +if (!(val & mask)) {
> > > +map_pci_region(mr, >io, OBJECT(s), "pci0-io-win",
> > > + p->remap[4], (p->io_size + 1) << 16,
> > > + (p->io_base & 0xf) << 16);
> > > +}
> > > +break;
> > > +case 10:
> > > +p = >pci[0];
> > > +mr = >cpu_win[i];
> > > +unmap_region(mr);
> > > +if (!(val & mask)) {
> > > +map_pci_region(mr, >mem, OBJECT(s),
> > > "pci0-mem0-win",
> > > + p->remap[0], (p->mem_size[0] + 1) <<
> > > 16,
> > > + (p->mem_base[0] & 0xf) << 16);
> > > +}
> > > +break;
> > > +case 11:
> > > +p = >pci[0];
> > > +mr = >cpu_win[i];
> > > +unmap_region(mr);
> > > +if (!(val & mask)) {
> > > +map_pci_region(mr, >mem, OBJECT(s),
> > > "pci0-mem1-win",
> > > + p->remap[1], (p->mem_size[1] + 1) <<
> > > 16,
> > > + (p->mem_base[1] & 0xf) << 16);
> > > +}
> > > +break;
> > > +case 12:
> > > +p = >pci[0];
> > > +mr = >cpu_win[i];
> > > +unmap_region(mr);
> > > +if (!(val & mask)) {
> > > +map_pci_region(mr, >mem, OBJECT(s),
> > > "pci0-mem2-win",
> > > + p->remap[2], (p->mem_size[2] + 1) <<
> > > 16,
> > > + (p->mem_base[2] & 0xf) << 16);
> > > +}
> > > +break;
> > > +case 13:
> > > +p = >pci[0];
> > > +mr = >cpu_win[i];
> > > +unmap_region(mr);
> > > +if (!(val & mask)) {
> > > +map_pci_region(mr, >mem, OBJECT(s),
> > > "pci0-mem3-win",
> > > + p->remap[3], (p->mem_size[3] + 1) <<
> > > 16,
> > > + (p->mem_base[3] & 0xf) << 16);
> > > +}
> > > +break;
> > > +case 14:
> > > +p = >pci[1];
> > > +mr = >cpu_win[i];
> > > +unmap_region(mr);
> > > +if (!(val & mask)) {
> > > +map_pci_region(mr, >io, OBJECT(s), "pci1-io-win",
> > > + p->remap[4], (p->io_size + 1) << 16,
> > > + (p->io_base & 0xf) << 16);
> > > +
[PULL 1/5] tcg: Do not set guard pages on the rx portion of code_gen_buffer
The rw portion of the buffer is the only one in which overruns
can be generated. Allow the rx portion to be more completely
covered by huge pages.
Signed-off-by: Richard Henderson
Tested-by: Roman Bolshakov
Reviewed-by: Roman Bolshakov
Message-Id: <20210320165720.1813545-2-richard.hender...@linaro.org>
---
tcg/tcg.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/tcg/tcg.c b/tcg/tcg.c
index de91bb6e9e..88c9e6f8a4 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -828,7 +828,6 @@ void tcg_region_init(void)
size_t region_size;
size_t n_regions;
size_t i;
-uintptr_t splitwx_diff;
n_regions = tcg_n_regions();
@@ -858,8 +857,11 @@ void tcg_region_init(void)
/* account for that last guard page */
region.end -= page_size;
-/* set guard pages */
-splitwx_diff = tcg_splitwx_diff;
+/*
+ * Set guard pages in the rw buffer, as that's the one into which
+ * buffer overruns could occur. Do not set guard pages in the rx
+ * buffer -- let that one use hugepages throughout.
+ */
for (i = 0; i < region.n; i++) {
void *start, *end;
int rc;
@@ -867,10 +869,6 @@ void tcg_region_init(void)
tcg_region_bounds(i, , );
rc = qemu_mprotect_none(end, page_size);
g_assert(!rc);
-if (splitwx_diff) {
-rc = qemu_mprotect_none(end + splitwx_diff, page_size);
-g_assert(!rc);
-}
}
tcg_region_trees_init();
--
2.25.1
Re: [PATCH v10 7/7] hw/ppc: Add emulation of Genesi/bPlan Pegasos II
On Tue, Mar 23, 2021 at 02:01:27PM +0100, BALATON Zoltan wrote: > On Tue, 23 Mar 2021, David Gibson wrote: > > On Wed, Mar 17, 2021 at 02:17:51AM +0100, BALATON Zoltan wrote: > > > Add new machine called pegasos2 emulating the Genesi/bPlan Pegasos II, > > > a PowerPC board based on the Marvell MV64361 system controller and the > > > VIA VT8231 integrated south bridge/superio chips. It can run Linux, > > > AmigaOS and a wide range of MorphOS versions. Currently a firmware ROM > > > image is needed to boot and only MorphOS has a video driver to produce > > > graphics output. Linux could work too but distros that supported this > > > machine don't include usual video drivers so those only run with > > > serial console for now. > > > > > > Signed-off-by: BALATON Zoltan > > > Reviewed-by: Philippe Mathieu-Daudé > > > --- > > > MAINTAINERS | 10 ++ > > > default-configs/devices/ppc-softmmu.mak | 2 + > > > hw/ppc/Kconfig | 9 ++ > > > hw/ppc/meson.build | 2 + > > > hw/ppc/pegasos2.c | 144 > > > 5 files changed, 167 insertions(+) > > > create mode 100644 hw/ppc/pegasos2.c > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > index b6ab3d25a7..1c3c55ef09 100644 > > > --- a/MAINTAINERS > > > +++ b/MAINTAINERS > > > @@ -1353,6 +1353,16 @@ F: pc-bios/canyonlands.dt[sb] > > > F: pc-bios/u-boot-sam460ex-20100605.bin > > > F: roms/u-boot-sam460ex > > > > > > +pegasos2 > > > +M: BALATON Zoltan > > > +R: David Gibson > > > +L: qemu-...@nongnu.org > > > +S: Maintained > > > +F: hw/ppc/pegasos2.c > > > +F: hw/pci-host/mv64361.c > > > +F: hw/pci-host/mv643xx.h > > > +F: include/hw/pci-host/mv64361.h > > > > Oh, sorry about the comment in the previous patch. > > > > > RISC-V Machines > > > --- > > > OpenTitan > > > diff --git a/default-configs/devices/ppc-softmmu.mak > > > b/default-configs/devices/ppc-softmmu.mak > > > index 61b78b844d..4535993d8d 100644 > > > --- a/default-configs/devices/ppc-softmmu.mak > > > +++ b/default-configs/devices/ppc-softmmu.mak > > > @@ -14,5 +14,7 @@ CONFIG_SAM460EX=y > > > CONFIG_MAC_OLDWORLD=y > > > CONFIG_MAC_NEWWORLD=y > > > > > > +CONFIG_PEGASOS2=y > > > > I don't think we can have this default to enabled while it requires a > > non-free ROM to start. > > Not having it enabled though does not help those who might want to use it as > they are not people who can compile their own QEMU but rely on binaries so > adding it without also enabling it is like it wasn't there at all in > practice. Not convinced, sorry. If it's not usable out of the box, having to build from source is kind of expected. Or you could convince someone (or do it yourself) to provide prebuild binaries for this purpose that have the right things enabled. > I can attempt to make some guests boot without a ROM but since > guests expect an OpenFirmware client interface, I'll need something to > provide that. I'm waiting for VOF to be merged for this. > > Regards, > BALATON Zoltan -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
[PULL 2/5] tcg: Workaround macOS 11.2 mprotect bug
There's a change in mprotect() behaviour [1] in the latest macOS
on M1 and it's not yet clear if it's going to be fixed by Apple.
As a short-term fix, ignore failures setting up the guard pages.
[1] https://gist.github.com/hikalium/75ae822466ee4da13cbbe486498a191f
Signed-off-by: Richard Henderson
Tested-by: Roman Bolshakov
Reviewed-by: Roman Bolshakov
Buglink: https://bugs.launchpad.net/qemu/+bug/1914849
Message-Id: <20210320165720.1813545-3-richard.hender...@linaro.org>
---
tcg/tcg.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 88c9e6f8a4..1fbe0b686d 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -864,11 +864,15 @@ void tcg_region_init(void)
*/
for (i = 0; i < region.n; i++) {
void *start, *end;
-int rc;
tcg_region_bounds(i, , );
-rc = qemu_mprotect_none(end, page_size);
-g_assert(!rc);
+
+/*
+ * macOS 11.2 has a bug (Apple Feedback FB8994773) in which mprotect
+ * rejects a permission change from RWX -> NONE. Guard pages are
+ * nice for bug detection but are not essential; ignore any failure.
+ */
+(void)qemu_mprotect_none(end, page_size);
}
tcg_region_trees_init();
--
2.25.1
Re: [PATCH 0/4] DEVICE_NOT_DELETED/DEVICE_UNPLUG_ERROR QAPI events
On Tue, Mar 23, 2021 at 02:10:22PM -0300, Daniel Henrique Barboza wrote: > > > On 3/22/21 10:12 PM, David Gibson wrote: > > On Fri, Mar 12, 2021 at 05:07:36PM -0300, Daniel Henrique Barboza wrote: > > > Hi, > > > > > > This series adds 2 new QAPI events, DEVICE_NOT_DELETED and > > > DEVICE_UNPLUG_ERROR. They were (and are still being) discussed in [1]. > > > > > > Patches 1 and 3 are independent of the ppc patches and can be applied > > > separately. Patches 2 and 4 are based on David's ppc-for-6.0 branch and > > > are dependent on the QAPI patches. > > > > Implementation looks fine, but I think there's a bit more to discuss > > before we can apply. > > > > I think it would make sense to re-order this and put UNPLUG_ERROR > > first. Its semantics are clearer, and I think there's a stronger case > > for it. > > Alright > > > > > I'm a bit less sold on DEVICE_NOT_DELETED, after consideration. Does > > it really tell the user/management anything useful beyond what > > receiving neither a DEVICE_DELETED nor a DEVICE_UNPLUG_ERROR does? > > > It informs that the hotunplug operation exceed the timeout that QEMU > internals considers adequate, but QEMU can't assert that it was caused > by an error or an unexpected delay. The end result is that the device > is not going to be deleted from QMP, so DEVICE_NOT_DELETED. Is it, though? I mean, it is with this implementation for papr: because we clear the unplug_requested flag, even if the guest later tries to complete the unplug, it will fail. But if I understand what Markus was saying correctly, that might not be possible for all hotplug systems. I believe Markus was suggesting that DEVICE_NOT_DELETED could just mean that we haven't deleted the device yet, but it could still happen later. And in that case, I'm not yet sold on the value of a message that essentially just means "Ayup, still dunno what's happening, sorry". > Perhaps we should just be straightforward and create a DEVICE_UNPLUG_TIMEOUT > event. Hm... what if we added a "reason" field to UNPLUG_ERROR. That could be "guest rejected hotplug", or something more specific, in the rare case that the guest has a way of signalling something more specific, or "timeout" - but the later *only* to be sent in cases where on the timeout we're able to block any later completion of the unplug (as we can on papr). Thoughs, Markus? -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
[PULL 0/5] tcg patch queue for 6.0
The following changes since commit 266469947161aa10b1d36843580d369d5aa38589: Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2021-03-23' into staging (2021-03-23 22:28:58 +) are available in the Git repository at: https://github.com/rth7680/qemu.git tags/pull-tcg-20210323 for you to fetch changes up to 44b99a6d5f24afcd8476d0d2701e1ca4ab9b35c1: exec: Build page-vary-common.c with -fno-lto (2021-03-23 19:36:47 -0600) Workaround for macos mprotect Workaround for target_page vs -flto Richard Henderson (5): tcg: Do not set guard pages on the rx portion of code_gen_buffer tcg: Workaround macOS 11.2 mprotect bug exec: Rename exec-vary.c as page-vary.c exec: Extract 'page-vary.h' header exec: Build page-vary-common.c with -fno-lto configure| 19 - meson.build | 21 - include/exec/cpu-all.h | 15 ++- include/exec/page-vary.h | 34 +++ exec-vary.c | 108 --- page-vary-common.c | 54 page-vary.c | 41 ++ tcg/tcg.c| 22 +- MAINTAINERS | 2 + 9 files changed, 167 insertions(+), 149 deletions(-) create mode 100644 include/exec/page-vary.h delete mode 100644 exec-vary.c create mode 100644 page-vary-common.c create mode 100644 page-vary.c
[PULL 5/5] exec: Build page-vary-common.c with -fno-lto
In bbc17caf81f, we used an alias attribute to allow target_page
to be declared const, and yet be initialized late.
This fails when using LTO with several versions of gcc.
The compiler looks through the alias and decides that the const
variable is statically initialized to zero, then propagates that
zero to many uses of the variable.
This can be avoided by compiling one object file with -fno-lto.
In this way, any initializer cannot be seen, and the constant
propagation does not occur.
Since we are certain to have this separate compilation unit, we
can drop the alias attribute as well. We simply have differing
declarations for target_page in different compilation units.
Drop the use of init_target_page, and drop the configure detection
for CONFIG_ATTRIBUTE_ALIAS.
In order to change the compilation flags for a file with meson,
we must use a static_library. This runs into specific_ss, where
we would need to create many static_library instances.
Fix this by splitting page-vary.c: the page-vary-common.c part is
compiled once as a static_library, while the page-vary.c part is
left in specific_ss in order to handle the target-specific value
of TARGET_PAGE_BITS_MIN.
Reported-by: Gavin Shan
Signed-off-by: Richard Henderson
Message-Id: <20210321211534.2101231-1-richard.hender...@linaro.org>
[PMD: Fix typo in subject, split original patch in 3]
Signed-off-by: Philippe Mathieu-Daudé
Tested-by: Gavin Shan
Message-Id: <20210322112427.4045204-4-f4...@amsat.org>
[rth: Update MAINTAINERS]
Signed-off-by: Richard Henderson
---
configure| 19 --
meson.build | 18 +
include/exec/cpu-all.h | 4 --
include/exec/page-vary.h | 5 +++
page-vary-common.c | 54 +++
page-vary.c | 79 +++-
MAINTAINERS | 1 +
7 files changed, 84 insertions(+), 96 deletions(-)
create mode 100644 page-vary-common.c
diff --git a/configure b/configure
index 61872096a8..edf9dc8985 100755
--- a/configure
+++ b/configure
@@ -4889,21 +4889,6 @@ if test "$plugins" = "yes" &&
"for this purpose. You can't build with --static."
fi
-
-# See if __attribute__((alias)) is supported.
-# This false for Xcode 9, but has been remedied for Xcode 10.
-# Unfortunately, travis uses Xcode 9 by default.
-
-attralias=no
-cat > $TMPC << EOF
-int x = 1;
-extern const int y __attribute__((alias("x")));
-int main(void) { return 0; }
-EOF
-if compile_prog "" "" ; then
-attralias=yes
-fi
-
# check if getauxval is available.
@@ -5935,10 +5920,6 @@ if test "$atomic64" = "yes" ; then
echo "CONFIG_ATOMIC64=y" >> $config_host_mak
fi
-if test "$attralias" = "yes" ; then
- echo "CONFIG_ATTRIBUTE_ALIAS=y" >> $config_host_mak
-fi
-
if test "$getauxval" = "yes" ; then
echo "CONFIG_GETAUXVAL=y" >> $config_host_mak
fi
diff --git a/meson.build b/meson.build
index f0dd8aa089..c6f4b0cf5e 100644
--- a/meson.build
+++ b/meson.build
@@ -1944,6 +1944,24 @@ specific_ss.add(when: 'CONFIG_TCG', if_true: files(
))
specific_ss.add(when: 'CONFIG_TCG_INTERPRETER', if_true: files('tcg/tci.c'))
+# Work around a gcc bug/misfeature wherein constant propagation looks
+# through an alias:
+# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99696
+# to guess that a const variable is always zero. Without lto, this is
+# impossible, as the alias is restricted to page-vary-common.c. Indeed,
+# without lto, not even the alias is required -- we simply use different
+# declarations in different compilation units.
+pagevary = files('page-vary-common.c')
+if get_option('b_lto')
+ pagevary_flags = ['-fno-lto']
+ if get_option('cfi')
+pagevary_flags += '-fno-sanitize=cfi-icall'
+ endif
+ pagevary = static_library('page-vary-common', sources: pagevary,
+c_args: pagevary_flags)
+ pagevary = declare_dependency(link_with: pagevary)
+endif
+common_ss.add(pagevary)
specific_ss.add(files('page-vary.c'))
subdir('backends')
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index b0a422c7b6..d76b0b9e02 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -216,11 +216,7 @@ static inline void stl_phys_notdirty(AddressSpace *as,
hwaddr addr, uint32_t val
#ifdef TARGET_PAGE_BITS_VARY
# include "exec/page-vary.h"
-#if defined(CONFIG_ATTRIBUTE_ALIAS) || !defined(IN_EXEC_VARY)
extern const TargetPageBits target_page;
-#else
-extern TargetPageBits target_page;
-#endif
#ifdef CONFIG_DEBUG_TCG
#define TARGET_PAGE_BITS ({ assert(target_page.decided); target_page.bits; })
#define TARGET_PAGE_MASK ({ assert(target_page.decided); \
diff --git a/include/exec/page-vary.h b/include/exec/page-vary.h
index 799d6310d6..c22a7a742e 100644
--- a/include/exec/page-vary.h
+++ b/include/exec/page-vary.h
@@ -26,4 +26,9 @@ typedef struct {
uint64_t mask;
} TargetPageBits;
+#ifdef IN_PAGE_VARY
+extern bool
[PULL 4/5] exec: Extract 'page-vary.h' header
In the next commit we will extract the generic code out of
page-vary.c, only keeping the target specific code. Both
files will use the same TargetPageBits structure, so make
its declaration in a shared header.
As the common header can not use target specific types,
use a uint64_t to hold the page mask value, and add a
cast back to target_long in the TARGET_PAGE_MASK definitions.
Signed-off-by: Philippe Mathieu-Daudé
Message-Id: <20210322112427.4045204-3-f4...@amsat.org>
Signed-off-by: Richard Henderson
---
include/exec/cpu-all.h | 11 ---
include/exec/page-vary.h | 29 +
2 files changed, 33 insertions(+), 7 deletions(-)
create mode 100644 include/exec/page-vary.h
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 76443eb11d..b0a422c7b6 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -215,11 +215,7 @@ static inline void stl_phys_notdirty(AddressSpace *as,
hwaddr addr, uint32_t val
/* page related stuff */
#ifdef TARGET_PAGE_BITS_VARY
-typedef struct {
-bool decided;
-int bits;
-target_long mask;
-} TargetPageBits;
+# include "exec/page-vary.h"
#if defined(CONFIG_ATTRIBUTE_ALIAS) || !defined(IN_EXEC_VARY)
extern const TargetPageBits target_page;
#else
@@ -227,10 +223,11 @@ extern TargetPageBits target_page;
#endif
#ifdef CONFIG_DEBUG_TCG
#define TARGET_PAGE_BITS ({ assert(target_page.decided); target_page.bits; })
-#define TARGET_PAGE_MASK ({ assert(target_page.decided); target_page.mask; })
+#define TARGET_PAGE_MASK ({ assert(target_page.decided); \
+ (target_long)target_page.mask; })
#else
#define TARGET_PAGE_BITS target_page.bits
-#define TARGET_PAGE_MASK target_page.mask
+#define TARGET_PAGE_MASK ((target_long)target_page.mask)
#endif
#define TARGET_PAGE_SIZE (-(int)TARGET_PAGE_MASK)
#else
diff --git a/include/exec/page-vary.h b/include/exec/page-vary.h
new file mode 100644
index 00..799d6310d6
--- /dev/null
+++ b/include/exec/page-vary.h
@@ -0,0 +1,29 @@
+/*
+ * Definitions for cpus with variable page sizes.
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see
[PULL 3/5] exec: Rename exec-vary.c as page-vary.c
exec-vary.c is about variable page size handling,
rename it page-vary.c. Currently this file is target
specific (built once for each target), comment this.
Signed-off-by: Philippe Mathieu-Daudé
Message-Id: <20210322112427.4045204-2-f4...@amsat.org>
[rth: Update MAINTAINERS]
Signed-off-by: Richard Henderson
---
meson.build| 3 ++-
exec-vary.c => page-vary.c | 2 +-
MAINTAINERS| 1 +
3 files changed, 4 insertions(+), 2 deletions(-)
rename exec-vary.c => page-vary.c (98%)
diff --git a/meson.build b/meson.build
index 5c85a15364..f0dd8aa089 100644
--- a/meson.build
+++ b/meson.build
@@ -1933,7 +1933,6 @@ subdir('softmmu')
common_ss.add(capstone)
specific_ss.add(files('cpu.c', 'disas.c', 'gdbstub.c'), capstone)
-specific_ss.add(files('exec-vary.c'))
specific_ss.add(when: 'CONFIG_TCG', if_true: files(
'fpu/softfloat.c',
'tcg/optimize.c',
@@ -1945,6 +1944,8 @@ specific_ss.add(when: 'CONFIG_TCG', if_true: files(
))
specific_ss.add(when: 'CONFIG_TCG_INTERPRETER', if_true: files('tcg/tci.c'))
+specific_ss.add(files('page-vary.c'))
+
subdir('backends')
subdir('disas')
subdir('migration')
diff --git a/exec-vary.c b/page-vary.c
similarity index 98%
rename from exec-vary.c
rename to page-vary.c
index a603b1b433..344f9fcf76 100644
--- a/exec-vary.c
+++ b/page-vary.c
@@ -1,5 +1,5 @@
/*
- * Variable page size handling
+ * Variable page size handling -- target specific part.
*
* Copyright (c) 2003 Fabrice Bellard
*
diff --git a/MAINTAINERS b/MAINTAINERS
index 9147e9a429..ed68de3cec 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -117,6 +117,7 @@ R: Paolo Bonzini
S: Maintained
F: softmmu/cpus.c
F: cpus-common.c
+F: page-vary.c
F: accel/tcg/
F: accel/stubs/tcg-stub.c
F: util/cacheinfo.c
--
2.25.1
Re: [PATCH v5 10/10] target/ppc: Validate hflags with CONFIG_DEBUG_TCG
On Tue, Mar 23, 2021 at 12:43:40PM -0600, Richard Henderson wrote:
> Verify that hflags was updated correctly whenever we change
> cpu state that is used by hflags.
>
> Signed-off-by: Richard Henderson
Applied to ppc-for-6.0, thanks.
> ---
> target/ppc/cpu.h | 5 +
> target/ppc/helper_regs.c | 29 +++--
> 2 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 3d021f61f3..69fc9a2831 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2425,6 +2425,10 @@ void cpu_write_xer(CPUPPCState *env, target_ulong xer);
> */
> #define is_book3s_arch2x(ctx) (!!((ctx)->insns_flags & PPC_SEGMENT_64B))
>
> +#ifdef CONFIG_DEBUG_TCG
> +void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
> + target_ulong *cs_base, uint32_t *flags);
> +#else
> static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
> target_ulong *cs_base, uint32_t
> *flags)
> {
> @@ -2432,6 +2436,7 @@ static inline void cpu_get_tb_cpu_state(CPUPPCState
> *env, target_ulong *pc,
> *cs_base = 0;
> *flags = env->hflags;
> }
> +#endif
>
> void QEMU_NORETURN raise_exception(CPUPPCState *env, uint32_t exception);
> void QEMU_NORETURN raise_exception_ra(CPUPPCState *env, uint32_t exception,
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 5411a67e9a..3723872aa6 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -43,7 +43,7 @@ void hreg_swap_gpr_tgpr(CPUPPCState *env)
> env->tgpr[3] = tmp;
> }
>
> -void hreg_compute_hflags(CPUPPCState *env)
> +static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
> {
> target_ulong msr = env->msr;
> uint32_t ppc_flags = env->flags;
> @@ -155,9 +155,34 @@ void hreg_compute_hflags(CPUPPCState *env)
> hflags |= dmmu_idx << HFLAGS_DMMU_IDX;
> #endif
>
> -env->hflags = hflags | (msr & msr_mask);
> +return hflags | (msr & msr_mask);
> }
>
> +void hreg_compute_hflags(CPUPPCState *env)
> +{
> +env->hflags = hreg_compute_hflags_value(env);
> +}
> +
> +#ifdef CONFIG_DEBUG_TCG
> +void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
> + target_ulong *cs_base, uint32_t *flags)
> +{
> +uint32_t hflags_current = env->hflags;
> +uint32_t hflags_rebuilt;
> +
> +*pc = env->nip;
> +*cs_base = 0;
> +*flags = hflags_current;
> +
> +hflags_rebuilt = hreg_compute_hflags_value(env);
> +if (unlikely(hflags_current != hflags_rebuilt)) {
> +cpu_abort(env_cpu(env),
> + "TCG hflags mismatch (current:0x%08x rebuilt:0x%08x)\n",
> + hflags_current, hflags_rebuilt);
> +}
> +}
> +#endif
> +
> void cpu_interrupt_exittb(CPUState *cs)
> {
> if (!kvm_enabled()) {
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH v5 07/10] target/ppc: Remove MSR_SA and MSR_AP from hflags
On Tue, Mar 23, 2021 at 12:43:37PM -0600, Richard Henderson wrote:
> Nothing within the translator -- or anywhere else for that
> matter -- checks MSR_SA or MSR_AP on the 602. This may be
> a mistake. However, for the moment, we need not record these
> bits in hflags.
And frankly, even if it's wrong, I suspect the chances of someone
caring enough to fix 602 logic are pretty slim.
> This allows us to simplify HFLAGS_VSX computation by moving
> it to overlap with MSR_VSX.
>
> Reviewed-by: David Gibson
> Signed-off-by: Richard Henderson
Applied to ppc-for-6.0.
> ---
> target/ppc/cpu.h | 4 +---
> target/ppc/helper_regs.c | 10 --
> 2 files changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 3c28ddb331..2f72f83ee3 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -600,14 +600,12 @@ enum {
> HFLAGS_DR = 4, /* MSR_DR */
> HFLAGS_IR = 5, /* MSR_IR */
> HFLAGS_SPE = 6, /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR
> */
> -HFLAGS_VSX = 7, /* from MSR_VSX if cpu has VSX; avoid overlap w/ MSR_AP
> */
> HFLAGS_TM = 8, /* computed from MSR_TM */
> HFLAGS_BE = 9, /* MSR_BE -- from elsewhere on embedded ppc */
> HFLAGS_SE = 10, /* MSR_SE -- from elsewhere on embedded ppc */
> HFLAGS_FP = 13, /* MSR_FP */
> HFLAGS_PR = 14, /* MSR_PR */
> -HFLAGS_SA = 22, /* MSR_SA */
> -HFLAGS_AP = 23, /* MSR_AP */
> +HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
> HFLAGS_VR = 25, /* MSR_VR if cpu has VRE */
> };
>
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index f85bb14d1d..dd3cd770a3 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -99,11 +99,8 @@ void hreg_compute_hflags(CPUPPCState *env)
> QEMU_BUILD_BUG_ON(MSR_DR != HFLAGS_DR);
> QEMU_BUILD_BUG_ON(MSR_IR != HFLAGS_IR);
> QEMU_BUILD_BUG_ON(MSR_FP != HFLAGS_FP);
> -QEMU_BUILD_BUG_ON(MSR_SA != HFLAGS_SA);
> -QEMU_BUILD_BUG_ON(MSR_AP != HFLAGS_AP);
> msr_mask = ((1 << MSR_LE) | (1 << MSR_PR) |
> -(1 << MSR_DR) | (1 << MSR_IR) |
> -(1 << MSR_FP) | (1 << MSR_SA) | (1 << MSR_AP));
> +(1 << MSR_DR) | (1 << MSR_IR) | (1 << MSR_FP));
>
> if (ppc_flags & POWERPC_FLAG_HID0_LE) {
> /*
> @@ -143,8 +140,9 @@ void hreg_compute_hflags(CPUPPCState *env)
> QEMU_BUILD_BUG_ON(MSR_VR != HFLAGS_VR);
> msr_mask |= 1 << MSR_VR;
> }
> -if ((ppc_flags & POWERPC_FLAG_VSX) && (msr & (1 << MSR_VSX))) {
> -hflags |= 1 << HFLAGS_VSX;
> +if (ppc_flags & POWERPC_FLAG_VSX) {
> +QEMU_BUILD_BUG_ON(MSR_VSX != HFLAGS_VSX);
> +msr_mask |= 1 << MSR_VSX;
> }
> if ((ppc_flags & POWERPC_FLAG_TM) && (msr & (1ull << MSR_TM))) {
> hflags |= 1 << HFLAGS_TM;
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH v5 08/10] target/ppc: Remove env->immu_idx and env->dmmu_idx
On Tue, Mar 23, 2021 at 12:43:38PM -0600, Richard Henderson wrote:
> We weren't recording MSR_GS in hflags, which means that BookE
> memory accesses were essentially random vs Guest State.
>
> Instead of adding this bit directly, record the completed mmu
> indexes instead. This makes it obvious that we are recording
> exactly the information that we need.
>
> This also means that we can stop directly recording MSR_IR.
>
> Reviewed-by: David Gibson
> Signed-off-by: Richard Henderson
Applied to ppc-for-6.0.
> ---
> target/ppc/cpu.h | 12 --
> target/ppc/helper_regs.h | 1 -
> target/ppc/helper_regs.c | 89 +++-
> target/ppc/machine.c | 2 +-
> target/ppc/mem_helper.c | 2 +-
> target/ppc/translate.c | 6 +--
> 6 files changed, 56 insertions(+), 56 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 2f72f83ee3..3d021f61f3 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -598,7 +598,6 @@ enum {
> HFLAGS_64 = 2, /* computed from MSR_CE and MSR_SF */
> HFLAGS_GTSE = 3, /* computed from SPR_LPCR[GTSE] */
> HFLAGS_DR = 4, /* MSR_DR */
> -HFLAGS_IR = 5, /* MSR_IR */
> HFLAGS_SPE = 6, /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR
> */
> HFLAGS_TM = 8, /* computed from MSR_TM */
> HFLAGS_BE = 9, /* MSR_BE -- from elsewhere on embedded ppc */
> @@ -607,6 +606,9 @@ enum {
> HFLAGS_PR = 14, /* MSR_PR */
> HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
> HFLAGS_VR = 25, /* MSR_VR if cpu has VRE */
> +
> +HFLAGS_IMMU_IDX = 26, /* 26..28 -- the composite immu_idx */
> +HFLAGS_DMMU_IDX = 29, /* 29..31 -- the composite dmmu_idx */
> };
>
>
> /*/
> @@ -1131,8 +1133,6 @@ struct CPUPPCState {
> /* These resources are used only in TCG */
> uint32_t hflags;
> target_ulong hflags_compat_nmsr; /* for migration compatibility */
> -int immu_idx; /* precomputed MMU index to speed up insn accesses */
> -int dmmu_idx; /* precomputed MMU index to speed up data accesses */
>
> /* Power management */
> int (*check_pow)(CPUPPCState *env);
> @@ -1368,7 +1368,11 @@ int ppc_dcr_write(ppc_dcr_t *dcr_env, int dcrn,
> uint32_t val);
> #define MMU_USER_IDX 0
> static inline int cpu_mmu_index(CPUPPCState *env, bool ifetch)
> {
> -return ifetch ? env->immu_idx : env->dmmu_idx;
> +#ifdef CONFIG_USER_ONLY
> +return MMU_USER_IDX;
> +#else
> +return (env->hflags >> (ifetch ? HFLAGS_IMMU_IDX : HFLAGS_DMMU_IDX)) & 7;
> +#endif
> }
>
> /* Compatibility modes */
> diff --git a/target/ppc/helper_regs.h b/target/ppc/helper_regs.h
> index 4148a442b3..42f26870b9 100644
> --- a/target/ppc/helper_regs.h
> +++ b/target/ppc/helper_regs.h
> @@ -21,7 +21,6 @@
> #define HELPER_REGS_H
>
> void hreg_swap_gpr_tgpr(CPUPPCState *env);
> -void hreg_compute_mem_idx(CPUPPCState *env);
> void hreg_compute_hflags(CPUPPCState *env);
> void cpu_interrupt_exittb(CPUState *cs);
> int hreg_store_msr(CPUPPCState *env, target_ulong value, int alter_hv);
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index dd3cd770a3..5411a67e9a 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -43,49 +43,6 @@ void hreg_swap_gpr_tgpr(CPUPPCState *env)
> env->tgpr[3] = tmp;
> }
>
> -void hreg_compute_mem_idx(CPUPPCState *env)
> -{
> -/*
> - * This is our encoding for server processors. The architecture
> - * specifies that there is no such thing as userspace with
> - * translation off, however it appears that MacOS does it and some
> - * 32-bit CPUs support it. Weird...
> - *
> - * 0 = Guest User space virtual mode
> - * 1 = Guest Kernel space virtual mode
> - * 2 = Guest User space real mode
> - * 3 = Guest Kernel space real mode
> - * 4 = HV User space virtual mode
> - * 5 = HV Kernel space virtual mode
> - * 6 = HV User space real mode
> - * 7 = HV Kernel space real mode
> - *
> - * For BookE, we need 8 MMU modes as follow:
> - *
> - * 0 = AS 0 HV User space
> - * 1 = AS 0 HV Kernel space
> - * 2 = AS 1 HV User space
> - * 3 = AS 1 HV Kernel space
> - * 4 = AS 0 Guest User space
> - * 5 = AS 0 Guest Kernel space
> - * 6 = AS 1 Guest User space
> - * 7 = AS 1 Guest Kernel space
> - */
> -if (env->mmu_model & POWERPC_MMU_BOOKE) {
> -env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1;
> -env->immu_idx += msr_is ? 2 : 0;
> -env->dmmu_idx += msr_ds ? 2 : 0;
> -env->immu_idx += msr_gs ? 4 : 0;
> -env->dmmu_idx += msr_gs ? 4 : 0;
> -} else {
> -env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1;
> -env->immu_idx += msr_ir ? 0 : 2;
> -env->dmmu_idx += msr_dr ? 0 : 2;
> -env->immu_idx += msr_hv ? 4 : 0;
> -
Re: [PATCH v5 06/10] target/ppc: Put LPCR[GTSE] in hflags
On Tue, Mar 23, 2021 at 12:43:36PM -0600, Richard Henderson wrote:
> Because this bit was not in hflags, the privilege check
> for tlb instructions was essentially random.
> Recompute hflags when storing to LPCR.
>
> Reviewed-by: David Gibson
> Signed-off-by: Richard Henderson
Applied to ppc-for-6.0, thanks.
> ---
> target/ppc/cpu.h | 1 +
> target/ppc/helper_regs.c | 3 +++
> target/ppc/mmu-hash64.c | 3 +++
> target/ppc/translate.c | 2 +-
> 4 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index d5f362506a..3c28ddb331 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -596,6 +596,7 @@ enum {
> HFLAGS_LE = 0, /* MSR_LE -- comes from elsewhere on 601 */
> HFLAGS_HV = 1, /* computed from MSR_HV and other state */
> HFLAGS_64 = 2, /* computed from MSR_CE and MSR_SF */
> +HFLAGS_GTSE = 3, /* computed from SPR_LPCR[GTSE] */
> HFLAGS_DR = 4, /* MSR_DR */
> HFLAGS_IR = 5, /* MSR_IR */
> HFLAGS_SPE = 6, /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR
> */
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index e345966b6b..f85bb14d1d 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -149,6 +149,9 @@ void hreg_compute_hflags(CPUPPCState *env)
> if ((ppc_flags & POWERPC_FLAG_TM) && (msr & (1ull << MSR_TM))) {
> hflags |= 1 << HFLAGS_TM;
> }
> +if (env->spr[SPR_LPCR] & LPCR_GTSE) {
> +hflags |= 1 << HFLAGS_GTSE;
> +}
>
> #ifndef CONFIG_USER_ONLY
> if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 0fabc10302..d517a99832 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -30,6 +30,7 @@
> #include "exec/log.h"
> #include "hw/hw.h"
> #include "mmu-book3s-v3.h"
> +#include "helper_regs.h"
>
> /* #define DEBUG_SLB */
>
> @@ -1125,6 +1126,8 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
> CPUPPCState *env = >env;
>
> env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
> +/* The gtse bit affects hflags */
> +hreg_compute_hflags(env);
> }
>
> void helper_store_lpcr(CPUPPCState *env, target_ulong val)
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index d48c554290..5e629291d3 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7908,7 +7908,7 @@ static void ppc_tr_init_disas_context(DisasContextBase
> *dcbase, CPUState *cs)
> ctx->altivec_enabled = (hflags >> HFLAGS_VR) & 1;
> ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
> ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
> -ctx->gtse = !!(env->spr[SPR_LPCR] & LPCR_GTSE);
> +ctx->gtse = (hflags >> HFLAGS_GTSE) & 1;
>
> ctx->singlestep_enabled = 0;
> if ((hflags >> HFLAGS_SE) & 1) {
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH v5 04/10] target/ppc: Put dbcr0 single-step bits into hflags
On Tue, Mar 23, 2021 at 12:43:34PM -0600, Richard Henderson wrote:
> Because these bits were not in hflags, the code generated
> for single-stepping on BookE was essentially random.
> Recompute hflags when storing to dbcr0.
>
> Reviewed-by: David Gibson
> Signed-off-by: Richard Henderson
Applied to ppc-for-6.0.
> ---
> target/ppc/helper_regs.c | 24 +---
> target/ppc/misc_helper.c | 3 +++
> target/ppc/translate.c | 11 ---
> 3 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index df9673b90f..e345966b6b 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -114,13 +114,23 @@ void hreg_compute_hflags(CPUPPCState *env)
> hflags |= le << MSR_LE;
> }
>
> -if (ppc_flags & POWERPC_FLAG_BE) {
> -QEMU_BUILD_BUG_ON(MSR_BE != HFLAGS_BE);
> -msr_mask |= 1 << MSR_BE;
> -}
> -if (ppc_flags & POWERPC_FLAG_SE) {
> -QEMU_BUILD_BUG_ON(MSR_SE != HFLAGS_SE);
> -msr_mask |= 1 << MSR_SE;
> +if (ppc_flags & POWERPC_FLAG_DE) {
> +target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0];
> +if (dbcr0 & DBCR0_ICMP) {
> +hflags |= 1 << HFLAGS_SE;
> +}
> +if (dbcr0 & DBCR0_BRT) {
> +hflags |= 1 << HFLAGS_BE;
> +}
> +} else {
> +if (ppc_flags & POWERPC_FLAG_BE) {
> +QEMU_BUILD_BUG_ON(MSR_BE != HFLAGS_BE);
> +msr_mask |= 1 << MSR_BE;
> +}
> +if (ppc_flags & POWERPC_FLAG_SE) {
> +QEMU_BUILD_BUG_ON(MSR_SE != HFLAGS_SE);
> +msr_mask |= 1 << MSR_SE;
> +}
> }
>
> if (msr_is_64bit(env, msr)) {
> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> index b04b4d7c6e..002958be26 100644
> --- a/target/ppc/misc_helper.c
> +++ b/target/ppc/misc_helper.c
> @@ -215,6 +215,9 @@ void helper_store_403_pbr(CPUPPCState *env, uint32_t num,
> target_ulong value)
>
> void helper_store_40x_dbcr0(CPUPPCState *env, target_ulong val)
> {
> +/* Bits 26 & 27 affect single-stepping. */
> +hreg_compute_hflags(env);
> +/* Bits 28 & 29 affect reset or shutdown. */
> store_40x_dbcr0(env, val);
> }
>
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index a85b890bb0..7912495f28 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7923,17 +7923,6 @@ static void ppc_tr_init_disas_context(DisasContextBase
> *dcbase, CPUState *cs)
> if ((hflags >> HFLAGS_BE) & 1) {
> ctx->singlestep_enabled |= CPU_BRANCH_STEP;
> }
> -if ((env->flags & POWERPC_FLAG_DE) && msr_de) {
> -ctx->singlestep_enabled = 0;
> -target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0];
> -if (dbcr0 & DBCR0_ICMP) {
> -ctx->singlestep_enabled |= CPU_SINGLE_STEP;
> -}
> -if (dbcr0 & DBCR0_BRT) {
> -ctx->singlestep_enabled |= CPU_BRANCH_STEP;
> -}
> -
> -}
> if (unlikely(ctx->base.singlestep_enabled)) {
> ctx->singlestep_enabled |= GDBSTUB_SINGLE_STEP;
> }
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH v5 09/10] linux-user/ppc: Fix msr updates for signal handling
On Tue, Mar 23, 2021 at 12:43:39PM -0600, Richard Henderson wrote:
> In save_user_regs, there are two bugs where we OR in a bit number
> instead of the bit, clobbering the low bits of MSR. However:
>
> The MSR_VR and MSR_SPE bits control the availability of the insns.
> If the bits were not already set in MSR, then any attempt to access
> those registers would result in SIGILL.
>
> For linux-user, we always initialize MSR to the capabilities
> of the cpu. We *could* add checks vs MSR where we currently
> check insn_flags and insn_flags2, but we know they match.
>
> Also, there's a stray cut-and-paste comment in restore.
>
> Then, do not force little-endian binaries into big-endian mode.
>
> Finally, use ppc_store_msr for the update to affect hflags.
> Which is the reason none of these bugs were previously noticed.
>
> Signed-off-by: Richard Henderson
Applied to ppc-for-6.0.
> ---
> linux-user/ppc/cpu_loop.c | 5 +++--
> linux-user/ppc/signal.c | 23 +++
> 2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/linux-user/ppc/cpu_loop.c b/linux-user/ppc/cpu_loop.c
> index df71e15a25..4a0f6c8dc2 100644
> --- a/linux-user/ppc/cpu_loop.c
> +++ b/linux-user/ppc/cpu_loop.c
> @@ -492,11 +492,12 @@ void target_cpu_copy_regs(CPUArchState *env, struct
> target_pt_regs *regs)
> #if defined(TARGET_PPC64)
> int flag = (env->insns_flags2 & PPC2_BOOKE206) ? MSR_CM : MSR_SF;
> #if defined(TARGET_ABI32)
> -env->msr &= ~((target_ulong)1 << flag);
> +ppc_store_msr(env, env->msr & ~((target_ulong)1 << flag));
> #else
> -env->msr |= (target_ulong)1 << flag;
> +ppc_store_msr(env, env->msr | (target_ulong)1 << flag);
> #endif
> #endif
> +
> env->nip = regs->nip;
> for(i = 0; i < 32; i++) {
> env->gpr[i] = regs->gpr[i];
> diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
> index b78613f7c8..bad38f8ed9 100644
> --- a/linux-user/ppc/signal.c
> +++ b/linux-user/ppc/signal.c
> @@ -261,9 +261,6 @@ static void save_user_regs(CPUPPCState *env, struct
> target_mcontext *frame)
> __put_user(avr->u64[PPC_VEC_HI], >u64[0]);
> __put_user(avr->u64[PPC_VEC_LO], >u64[1]);
> }
> -/* Set MSR_VR in the saved MSR value to indicate that
> - frame->mc_vregs contains valid data. */
> -msr |= MSR_VR;
> #if defined(TARGET_PPC64)
> vrsave = (uint32_t *)>mc_vregs.altivec[33];
> /* 64-bit needs to put a pointer to the vectors in the frame */
> @@ -300,9 +297,6 @@ static void save_user_regs(CPUPPCState *env, struct
> target_mcontext *frame)
> for (i = 0; i < ARRAY_SIZE(env->gprh); i++) {
> __put_user(env->gprh[i], >mc_vregs.spe[i]);
> }
> -/* Set MSR_SPE in the saved MSR value to indicate that
> - frame->mc_vregs contains valid data. */
> -msr |= MSR_SPE;
> __put_user(env->spe_fscr, >mc_vregs.spe[32]);
> }
> #endif
> @@ -354,8 +348,10 @@ static void restore_user_regs(CPUPPCState *env,
> __get_user(msr, >mc_gregs[TARGET_PT_MSR]);
>
> /* If doing signal return, restore the previous little-endian mode. */
> -if (sig)
> -env->msr = (env->msr & ~(1ull << MSR_LE)) | (msr & (1ull << MSR_LE));
> +if (sig) {
> +ppc_store_msr(env, ((env->msr & ~(1ull << MSR_LE)) |
> +(msr & (1ull << MSR_LE;
> +}
>
> /* Restore Altivec registers if necessary. */
> if (env->insns_flags & PPC_ALTIVEC) {
> @@ -376,8 +372,6 @@ static void restore_user_regs(CPUPPCState *env,
> __get_user(avr->u64[PPC_VEC_HI], >u64[0]);
> __get_user(avr->u64[PPC_VEC_LO], >u64[1]);
> }
> -/* Set MSR_VEC in the saved MSR value to indicate that
> - frame->mc_vregs contains valid data. */
> #if defined(TARGET_PPC64)
> vrsave = (uint32_t *)_regs[33];
> #else
> @@ -468,7 +462,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
> env->nip = (target_ulong) ka->_sa_handler;
>
> /* Signal handlers are entered in big-endian mode. */
> -env->msr &= ~(1ull << MSR_LE);
> +ppc_store_msr(env, env->msr & ~(1ull << MSR_LE));
>
> unlock_user_struct(frame, frame_addr, 1);
> return;
> @@ -563,8 +557,13 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
> env->nip = (target_ulong) ka->_sa_handler;
> #endif
>
> +#ifdef TARGET_WORDS_BIGENDIAN
> /* Signal handlers are entered in big-endian mode. */
> -env->msr &= ~(1ull << MSR_LE);
> +ppc_store_msr(env, env->msr & ~(1ull << MSR_LE));
> +#else
> +/* Signal handlers are entered in little-endian mode. */
> +ppc_store_msr(env, env->msr | (1ull << MSR_LE));
> +#endif
>
> unlock_user_struct(rt_sf, rt_sf_addr, 1);
> return;
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
Re: [PATCH v5 01/10] target/ppc: Extract post_load_update_msr
On Tue, Mar 23, 2021 at 12:43:31PM -0600, Richard Henderson wrote:
> Extract post_load_update_msr to share between cpu_load_old
> and cpu_post_load in updating the msr.
>
> Suggested-by: Cédric Le Goater
> Signed-off-by: Richard Henderson
Applied to ppc-for-6.0.
> ---
> target/ppc/machine.c | 30 ++
> 1 file changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 1f7a353c78..09c5765a87 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -10,6 +10,18 @@
> #include "kvm_ppc.h"
> #include "exec/helper-proto.h"
>
> +static void post_load_update_msr(CPUPPCState *env)
> +{
> +target_ulong msr = env->msr;
> +
> +/*
> + * Invalidate all supported msr bits except MSR_TGPR/MSR_HVB
> + * before restoring. Note that this recomputes hflags and mem_idx.
> + */
> +env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
> +ppc_store_msr(env, msr);
> +}
> +
> static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
> {
> PowerPCCPU *cpu = opaque;
> @@ -21,7 +33,6 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int
> version_id)
> int32_t slb_nr;
> #endif
> target_ulong xer;
> -target_ulong msr;
>
> for (i = 0; i < 32; i++) {
> qemu_get_betls(f, >gpr[i]);
> @@ -117,13 +128,7 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int
> version_id)
> qemu_get_sbe32(f); /* Discard unused mmu_idx */
> qemu_get_sbe32(f); /* Discard unused power_mode */
>
> -/*
> - * Invalidate all supported msr bits except MSR_TGPR/MSR_HVB
> - * before restoring. Note that this recomputes hflags and mem_idx.
> - */
> -msr = env->msr;
> -env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
> -ppc_store_msr(env, msr);
> +post_load_update_msr(env);
>
> return 0;
> }
> @@ -343,7 +348,6 @@ static int cpu_post_load(void *opaque, int version_id)
> PowerPCCPU *cpu = opaque;
> CPUPPCState *env = >env;
> int i;
> -target_ulong msr;
>
> /*
> * If we're operating in compat mode, we should be ok as long as
> @@ -417,13 +421,7 @@ static int cpu_post_load(void *opaque, int version_id)
> ppc_store_sdr1(env, env->spr[SPR_SDR1]);
> }
>
> -/*
> - * Invalidate all supported msr bits except MSR_TGPR/MSR_HVB
> - * before restoring. Note that this recomputes hflags and mem_idx.
> - */
> -msr = env->msr;
> -env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
> -ppc_store_msr(env, msr);
> +post_load_update_msr(env);
>
> return 0;
> }
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH v5 05/10] target/ppc: Create helper_scv
On Tue, Mar 23, 2021 at 12:43:35PM -0600, Richard Henderson wrote:
> Perform the test against FSCR_SCV at runtime, in the helper.
>
> This means we can remove the incorrect set against SCV in
> ppc_tr_init_disas_context and do not need to add an HFLAGS bit.
>
> Signed-off-by: Richard Henderson
Applied to ppc-for-6.0.
> ---
> target/ppc/helper.h | 1 +
> target/ppc/excp_helper.c | 9 +
> target/ppc/translate.c | 20 +++-
> 3 files changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 6a4dccf70c..513066d54d 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -13,6 +13,7 @@ DEF_HELPER_1(rfci, void, env)
> DEF_HELPER_1(rfdi, void, env)
> DEF_HELPER_1(rfmci, void, env)
> #if defined(TARGET_PPC64)
> +DEF_HELPER_2(scv, noreturn, env, i32)
> DEF_HELPER_2(pminsn, void, env, i32)
> DEF_HELPER_1(rfid, void, env)
> DEF_HELPER_1(rfscv, void, env)
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 85de7e6c90..5c95e0c103 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1130,6 +1130,15 @@ void helper_store_msr(CPUPPCState *env, target_ulong
> val)
> }
>
> #if defined(TARGET_PPC64)
> +void helper_scv(CPUPPCState *env, uint32_t lev)
> +{
> +if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
> +raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
> +} else {
> +raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
> +}
> +}
> +
> void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
> {
> CPUState *cs;
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 7912495f28..d48c554290 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -173,7 +173,6 @@ struct DisasContext {
> bool vsx_enabled;
> bool spe_enabled;
> bool tm_enabled;
> -bool scv_enabled;
> bool gtse;
> ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
> int singlestep_enabled;
> @@ -4081,15 +4080,16 @@ static void gen_sc(DisasContext *ctx)
> #if !defined(CONFIG_USER_ONLY)
> static void gen_scv(DisasContext *ctx)
> {
> -uint32_t lev;
> +uint32_t lev = (ctx->opcode >> 5) & 0x7F;
>
> -if (unlikely(!ctx->scv_enabled)) {
> -gen_exception_err(ctx, POWERPC_EXCP_FU, FSCR_IC_SCV);
> -return;
> +/* Set the PC back to the faulting instruction. */
> +if (ctx->exception == POWERPC_EXCP_NONE) {
> +gen_update_nip(ctx, ctx->base.pc_next - 4);
> }
> +gen_helper_scv(cpu_env, tcg_constant_i32(lev));
>
> -lev = (ctx->opcode >> 5) & 0x7F;
> -gen_exception_err(ctx, POWERPC_SYSCALL_VECTORED, lev);
> +/* This need not be exact, just not POWERPC_EXCP_NONE */
> +ctx->exception = POWERPC_SYSCALL_VECTORED;
> }
> #endif
> #endif
> @@ -7907,12 +7907,6 @@ static void ppc_tr_init_disas_context(DisasContextBase
> *dcbase, CPUState *cs)
> ctx->spe_enabled = (hflags >> HFLAGS_SPE) & 1;
> ctx->altivec_enabled = (hflags >> HFLAGS_VR) & 1;
> ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
> -if ((env->flags & POWERPC_FLAG_SCV)
> -&& (env->spr[SPR_FSCR] & (1ull << FSCR_SCV))) {
> -ctx->scv_enabled = true;
> -} else {
> -ctx->scv_enabled = false;
> -}
> ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
> ctx->gtse = !!(env->spr[SPR_LPCR] & LPCR_GTSE);
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH v5 02/10] target/ppc: Disconnect hflags from MSR
On Tue, Mar 23, 2021 at 12:43:32PM -0600, Richard Henderson wrote:
> Copying flags directly from msr has drawbacks: (1) msr bits
> mean different things per cpu, (2) msr has 64 bits on 64 cpus
> while tb->flags has only 32 bits.
>
> Create a enum to define these bits. Document the origin of each bit
> and validate those bits that must match MSR. This fixes the
> truncation of env->hflags to tb->flags, because we no longer
> have hflags bits set above bit 31.
>
> Most of the code in ppc_tr_init_disas_context is moved over to
> hreg_compute_hflags. Some of it is simple extractions from msr,
> some requires examining other cpu flags. Anything that is moved
> becomes a simple extract from hflags in ppc_tr_init_disas_context.
>
> Several existing bugs are left in ppc_tr_init_disas_context, where
> additional changes are required -- to be addressed in future patches.
>
> Remove a broken #if 0 block.
>
> Reported-by: Ivan Warren
> Signed-off-by: Richard Henderson
Applied to ppc-for-6.0.
> ---
> target/ppc/cpu.h | 25
> target/ppc/helper_regs.c | 65 +---
> target/ppc/translate.c | 55 ++
> 3 files changed, 95 insertions(+), 50 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index fd13489dce..fe6c3f815d 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -585,6 +585,31 @@ enum {
> POWERPC_FLAG_HID0_LE = 0x0040,
> };
>
> +/*
> + * Bits for env->hflags.
> + *
> + * Most of these bits overlap with corresponding bits in MSR,
> + * but some come from other sources. Those that do come from
> + * the MSR are validated in hreg_compute_hflags.
> + */
> +enum {
> +HFLAGS_LE = 0, /* MSR_LE -- comes from elsewhere on 601 */
> +HFLAGS_HV = 1, /* computed from MSR_HV and other state */
> +HFLAGS_64 = 2, /* computed from MSR_CE and MSR_SF */
> +HFLAGS_DR = 4, /* MSR_DR */
> +HFLAGS_IR = 5, /* MSR_IR */
> +HFLAGS_SPE = 6, /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR
> */
> +HFLAGS_VSX = 7, /* from MSR_VSX if cpu has VSX; avoid overlap w/ MSR_AP
> */
> +HFLAGS_TM = 8, /* computed from MSR_TM */
> +HFLAGS_BE = 9, /* MSR_BE -- from elsewhere on embedded ppc */
> +HFLAGS_SE = 10, /* MSR_SE -- from elsewhere on embedded ppc */
> +HFLAGS_FP = 13, /* MSR_FP */
> +HFLAGS_PR = 14, /* MSR_PR */
> +HFLAGS_SA = 22, /* MSR_SA */
> +HFLAGS_AP = 23, /* MSR_AP */
> +HFLAGS_VR = 25, /* MSR_VR if cpu has VRE */
> +};
> +
>
> /*/
> /* Floating point status and control register
> */
> #define FPSCR_DRN2 34 /* Decimal Floating-Point rounding control
> */
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index a87e354ca2..df9673b90f 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -18,6 +18,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "cpu.h"
> #include "qemu/main-loop.h"
> #include "exec/exec-all.h"
> #include "sysemu/kvm.h"
> @@ -87,24 +88,66 @@ void hreg_compute_mem_idx(CPUPPCState *env)
>
> void hreg_compute_hflags(CPUPPCState *env)
> {
> -target_ulong hflags_mask;
> +target_ulong msr = env->msr;
> +uint32_t ppc_flags = env->flags;
> +uint32_t hflags = 0;
> +uint32_t msr_mask;
>
> -/* We 'forget' FE0 & FE1: we'll never generate imprecise exceptions */
> -hflags_mask = (1 << MSR_VR) | (1 << MSR_AP) | (1 << MSR_SA) |
> -(1 << MSR_PR) | (1 << MSR_FP) | (1 << MSR_SE) | (1 << MSR_BE) |
> -(1 << MSR_LE) | (1 << MSR_VSX) | (1 << MSR_IR) | (1 << MSR_DR);
> -hflags_mask |= (1ULL << MSR_CM) | (1ULL << MSR_SF) | MSR_HVB;
> -hreg_compute_mem_idx(env);
> -env->hflags = env->msr & hflags_mask;
> +/* Some bits come straight across from MSR. */
> +QEMU_BUILD_BUG_ON(MSR_LE != HFLAGS_LE);
> +QEMU_BUILD_BUG_ON(MSR_PR != HFLAGS_PR);
> +QEMU_BUILD_BUG_ON(MSR_DR != HFLAGS_DR);
> +QEMU_BUILD_BUG_ON(MSR_IR != HFLAGS_IR);
> +QEMU_BUILD_BUG_ON(MSR_FP != HFLAGS_FP);
> +QEMU_BUILD_BUG_ON(MSR_SA != HFLAGS_SA);
> +QEMU_BUILD_BUG_ON(MSR_AP != HFLAGS_AP);
> +msr_mask = ((1 << MSR_LE) | (1 << MSR_PR) |
> +(1 << MSR_DR) | (1 << MSR_IR) |
> +(1 << MSR_FP) | (1 << MSR_SA) | (1 << MSR_AP));
>
> -if (env->flags & POWERPC_FLAG_HID0_LE) {
> +if (ppc_flags & POWERPC_FLAG_HID0_LE) {
> /*
> * Note that MSR_LE is not set in env->msr_mask for this cpu,
> - * and so will never be set in msr or hflags at this point.
> + * and so will never be set in msr.
> */
> uint32_t le = extract32(env->spr[SPR_HID0], 3, 1);
> -env->hflags |= le << MSR_LE;
> +hflags |= le << MSR_LE;
> }
> +
> +if (ppc_flags & POWERPC_FLAG_BE) {
> +QEMU_BUILD_BUG_ON(MSR_BE
Re: [PATCH v5 03/10] target/ppc: Reduce env->hflags to uint32_t
On Tue, Mar 23, 2021 at 12:43:33PM -0600, Richard Henderson wrote:
> It will be stored in tb->flags, which is also uint32_t,
> so let's use the correct size.
>
> Reviewed-by: Cédric Le Goater
> Reviewed-by: David Gibson
> Signed-off-by: Richard Henderson
Applied to ppc-for-6.0.
> ---
> target/ppc/cpu.h | 4 ++--
> target/ppc/misc_helper.c | 2 +-
> target/ppc/translate.c | 2 +-
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index fe6c3f815d..d5f362506a 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1129,8 +1129,8 @@ struct CPUPPCState {
> bool resume_as_sreset;
> #endif
>
> -/* These resources are used only in QEMU core */
> -target_ulong hflags;
> +/* These resources are used only in TCG */
> +uint32_t hflags;
> target_ulong hflags_compat_nmsr; /* for migration compatibility */
> int immu_idx; /* precomputed MMU index to speed up insn accesses */
> int dmmu_idx; /* precomputed MMU index to speed up data accesses */
> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> index 63e3147eb4..b04b4d7c6e 100644
> --- a/target/ppc/misc_helper.c
> +++ b/target/ppc/misc_helper.c
> @@ -199,7 +199,7 @@ void helper_store_hid0_601(CPUPPCState *env, target_ulong
> val)
> if ((val ^ hid0) & 0x0008) {
> /* Change current endianness */
> hreg_compute_hflags(env);
> -qemu_log("%s: set endianness to %c => " TARGET_FMT_lx "\n", __func__,
> +qemu_log("%s: set endianness to %c => %08x\n", __func__,
> val & 0x8 ? 'l' : 'b', env->hflags);
> }
> }
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index a9325a12e5..a85b890bb0 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7657,7 +7657,7 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int
> flags)
> env->nip, env->lr, env->ctr, cpu_read_xer(env),
> cs->cpu_index);
> qemu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx " HF "
> - TARGET_FMT_lx " iidx %d didx %d\n",
> + "%08x iidx %d didx %d\n",
> env->msr, env->spr[SPR_HID0],
> env->hflags, env->immu_idx, env->dmmu_idx);
> #if !defined(NO_TIMER_DUMP)
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH V13 2/9] meson.build: Re-enable KVM support for MIPS
On Tue, Mar 23, 2021, at 9:56 PM, Philippe Mathieu-Daudé wrote: > Hi Huacai, > > We are going to tag QEMU v6.0-rc0 today. > > I only have access to a 64-bit MIPS in little-endian to > test KVM. > > Can you test the other configurations please? > - 32-bit BE > - 32-bit LE > - 64-bit BE +syq As Loongson doesn't have Big-Endian processor and Loongson 3A won't run 32bit kernel. Probably wecan test on boston or malta board? Thanks. > > Thanks! > > Phil. > > [...] -- - Jiaxun
Re: [PATCH v1] tests/migration: introduce multifd into guestperf
cc th...@redhat.com and berra...@redhat.com
Please review, thanks
在 2021/3/20 1:04, huang...@chinatelecom.cn 写道:
From: Hyman
Guestperf tool does not cover the multifd-enabled migration
currently, it is worth supporting so that developers can
analysis the migration performance with all kinds of
migration.
To request that multifd is enabled, with 4 channels:
$ ./tests/migration/guestperf.py \
--multifd --multifd-channels 4 --output output.json
To run the entire standardized set of multifd-enabled
comparisons, with unix migration:
$ ./tests/migration/guestperf-batch.py \
--dst-host localhost --transport unix \
--filter compr-multifd* --output outputdir
Signed-off-by: Hyman Huang(黄勇)
---
tests/migration/guestperf/comparison.py | 14 ++
tests/migration/guestperf/engine.py | 16
tests/migration/guestperf/scenario.py | 12 ++--
tests/migration/guestperf/shell.py | 10 +-
4 files changed, 49 insertions(+), 3 deletions(-)
diff --git a/tests/migration/guestperf/comparison.py
b/tests/migration/guestperf/comparison.py
index ba2edbe..c03b3f6 100644
--- a/tests/migration/guestperf/comparison.py
+++ b/tests/migration/guestperf/comparison.py
@@ -121,4 +121,18 @@ def __init__(self, name, scenarios):
Scenario("compr-xbzrle-cache-50",
compression_xbzrle=True, compression_xbzrle_cache=50),
]),
+
+
+# Looking at effect of multifd with
+# varying numbers of channels
+Comparison("compr-multifd", scenarios = [
+Scenario("compr-multifd-channels-4",
+ multifd=True, multifd_channels=2),
+Scenario("compr-multifd-channels-8",
+ multifd=True, multifd_channels=8),
+Scenario("compr-multifd-channels-32",
+ multifd=True, multifd_channels=32),
+Scenario("compr-multifd-channels-64",
+ multifd=True, multifd_channels=64),
+]),
]
diff --git a/tests/migration/guestperf/engine.py
b/tests/migration/guestperf/engine.py
index e399447..fab3957 100644
--- a/tests/migration/guestperf/engine.py
+++ b/tests/migration/guestperf/engine.py
@@ -188,6 +188,22 @@ def _migrate(self, hardware, scenario, src, dst,
connect_uri):
1024 * 1024 * 1024 / 100 *
scenario._compression_xbzrle_cache))
+if scenario._multifd:
+resp = src.command("migrate-set-capabilities",
+ capabilities = [
+ { "capability": "multifd",
+ "state": True }
+ ])
+resp = src.command("migrate-set-parameters",
+ multifd_channels=scenario._multifd_channels)
+resp = dst.command("migrate-set-capabilities",
+ capabilities = [
+ { "capability": "multifd",
+ "state": True }
+ ])
+resp = dst.command("migrate-set-parameters",
+ multifd_channels=scenario._multifd_channels)
+
resp = src.command("migrate", uri=connect_uri)
post_copy = False
diff --git a/tests/migration/guestperf/scenario.py
b/tests/migration/guestperf/scenario.py
index 28ef36c..de70d9b 100644
--- a/tests/migration/guestperf/scenario.py
+++ b/tests/migration/guestperf/scenario.py
@@ -29,7 +29,8 @@ def __init__(self, name,
post_copy=False, post_copy_iters=5,
auto_converge=False, auto_converge_step=10,
compression_mt=False, compression_mt_threads=1,
- compression_xbzrle=False, compression_xbzrle_cache=10):
+ compression_xbzrle=False, compression_xbzrle_cache=10,
+ multifd=False, multifd_channels=2):
self._name = name
@@ -56,6 +57,9 @@ def __init__(self, name,
self._compression_xbzrle = compression_xbzrle
self._compression_xbzrle_cache = compression_xbzrle_cache #
percentage of guest RAM
+self._multifd = multifd
+self._multifd_channels = multifd_channels
+
def serialize(self):
return {
"name": self._name,
@@ -73,6 +77,8 @@ def serialize(self):
"compression_mt_threads": self._compression_mt_threads,
"compression_xbzrle": self._compression_xbzrle,
"compression_xbzrle_cache": self._compression_xbzrle_cache,
+"multifd": self._multifd,
+"multifd_channels": self._multifd_channels,
}
@classmethod
@@ -92,4 +98,6 @@ def deserialize(cls, data):
data["compression_mt"],
data["compression_mt_threads"],
data["compression_xbzrle"],
-data["compression_xbzrle_cache"])
+data["compression_xbzrle_cache"],
+
[Bug 1921061] [NEW] Corsair iCUE Install Fails, qemu VM Reboots
Public bug reported: Hi, I had this working before, but in the latest version of QEMU (built from master), when I try to install Corsair iCUE, and it gets to the driver install point => my Windows 10 VM just reboots! I would be happy to capture logs, but ... what logs exist for an uncontrolled reboot? Thinking they are lost in the reboot :-(. Thanks! ** Affects: qemu Importance: Undecided Status: New -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1921061 Title: Corsair iCUE Install Fails, qemu VM Reboots Status in QEMU: New Bug description: Hi, I had this working before, but in the latest version of QEMU (built from master), when I try to install Corsair iCUE, and it gets to the driver install point => my Windows 10 VM just reboots! I would be happy to capture logs, but ... what logs exist for an uncontrolled reboot? Thinking they are lost in the reboot :-(. Thanks! To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1921061/+subscriptions
RE: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition
> -Original Message- > From: Markus Armbruster > Sent: Tuesday, March 23, 2021 5:55 PM > To: Zhang, Chen > Cc: Lukas Straub ; Li Zhijian > ; Jason Wang ; qemu- > dev ; Dr. David Alan Gilbert > ; Zhang Chen > Subject: Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition > > "Zhang, Chen" writes: > > >> -Original Message- > >> From: Markus Armbruster > [...] > >> Naming the argument type L4_Connection is misleading. > >> > >> Even naming the match arguments L4_Connection would be misleading. > >> "Connection" has a specific meaning in networking. There are TCP > >> connections. There is no such thing as an UDP connection. > >> > >> A TCP connection is uniquely identified by a pair of endpoints, i.e. > >> by source address, source port, destination address, destination port. > >> Same for other connection-oriented protocols. The protocol is not > >> part of the connection. Thus, L4_Connection would be misleading even > >> for the connection-oriented case. > >> > >> You need a named type for colo-passthrough-add's argument because > you > >> share it with colo-passthrough-del. I'm not sure that's what we want > >> (I'm going to write more on that in a moment). If it is what we > >> want, then please pick a another, descriptive name. > > > > What do you think the "L4BypassRule" or "NetworkRule" ? > > NetworkRule is too generic. > > What about ColoPassthroughRule? It can be used by net filter modules(filter mirror,filter-dump) in the future, that's not just for COLO. PassthroughRule is better? Thanks Chen
Re: [PULL 00/29] QAPI patches patches for 2021-03-23
On Tue, 23 Mar 2021 at 21:57, Markus Armbruster wrote: > > The following changes since commit 9950da284fa5e2ea9ab57d87e05b693fb60c79ce: > > Merge remote-tracking branch > 'remotes/alistair/tags/pull-riscv-to-apply-20210322-2' into staging > (2021-03-23 15:30:46 +) > > are available in the Git repository at: > > git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2021-03-23 > > for you to fetch changes up to bdabafc6836edc0f34732cac473899cb4e77a296: > > block: Remove monitor command block_passwd (2021-03-23 22:31:56 +0100) > > > QAPI patches patches for 2021-03-23 > > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0 for any user-visible changes. -- PMM
Re: [PATCH 5/6] hw/isa/vt82c686: Simplify removing unuseful qemu_allocate_irqs() call
On Wed, 24 Mar 2021, Philippe Mathieu-Daudé wrote:
Instead of creating an input IRQ with qemu_allocate_irqs()
to pass it as output IRQ of the PIC, with its handler simply
dispatching into the "intr" output IRQ, simplify by directly
connecting the PIC to the "intr" named output.
I think I've tried to do it that way first but it did not work for some
reason, that's why I had to add the additional handler, but this was about
a year ago so I don't remember the details. Did you test it still works or
expect me to test it? (Note that testing with firmware only may not be
enough as some firmwares don't use interrupts so only booting a guest
might reveal a problem. Not sure about pegasos2 firmware but sam460ex
U-Boot seems to poll instead of using IRQs.)
Regards,
BALATON Zoltan
Fixes: 3dc31cb8490 ("vt82c686: Move creation of ISA devices to the ISA bridge")
Signed-off-by: Philippe Mathieu-Daudé
---
hw/isa/vt82c686.c | 10 +-
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 87473ec121f..3dc3454858e 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -323,12 +323,6 @@ struct VT82C686BISAState {
SuperIOConfig superio_cfg;
};
-static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
-{
-VT82C686BISAState *s = opaque;
-qemu_set_irq(s->cpu_intr, level);
-}
-
static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
uint32_t val, int len)
{
@@ -384,14 +378,12 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
VT82C686BISAState *s = VT82C686B_ISA(d);
DeviceState *dev = DEVICE(d);
ISABus *isa_bus;
-qemu_irq *isa_irq;
int i;
qdev_init_gpio_out_named(dev, >cpu_intr, "intr", 1);
-isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
_fatal);
-isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
+isa_bus_irqs(isa_bus, i8259_init(isa_bus, s->cpu_intr));
i8254_pit_init(isa_bus, 0x40, 0, NULL);
i8257_dma_init(isa_bus, 0);
isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
[Bug 1920784] Re: qemu-system-ppc64le fails with kvm acceleration
The guys on the Fedora side seem to have found the patch to fix this: https://bugzilla.redhat.com/show_bug.cgi?id=1941652#c6 Apparently it will go upstream in Linux 5.11, but earlier versions also need the fix, specifically 5.9 and 5.10 Thank you! ** Bug watch added: Red Hat Bugzilla #1941652 https://bugzilla.redhat.com/show_bug.cgi?id=1941652 -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1920784 Title: qemu-system-ppc64le fails with kvm acceleration Status in QEMU: New Status in The Ubuntu-power-systems project: Confirmed Status in glibc package in Ubuntu: New Status in linux package in Ubuntu: Confirmed Status in qemu package in Ubuntu: Confirmed Bug description: (Suspected glibc issue!) qemu-system-ppc64(le) fails when invoked with kvm acceleration with error "illegal instruction" > qemu-system-ppc64(le) -M pseries,accel=kvm Illegal instruction (core dumped) In dmesg: Facility 'SCV' unavailable (12), exception at 0x7624f8134c0c, MSR=9280f033 Version-Release number of selected component (if applicable): qemu 5.2.0 Linux kernel 5.11 glibc 2.33 all latest updates as of submitting the bug report How reproducible: Always Steps to Reproduce: 1. Run qemu with kvm acceleration Actual results: Illegal instruction Expected results: Normal VM execution Additional info: The machine is a Raptor Talos II Lite with a Sforza V1 8-core, but was also observed on a Raptor Blackbird with the same processor. This was also observed on Fedora 34 beta, which uses glibc 2.33 Also tested on ArchPOWER (unofficial port of Arch Linux for ppc64le) with glibc 2.33 Fedora 33 and Ubuntu 20.10, both using glibc 2.32 do not have this issue, and downgrading the Linux kernel from 5.11 to 5.4 LTS on ArchPOWER solved the problem. Kernel 5.9 and 5.10 have the same issue when combined with glibc2.33 ProblemType: Bug DistroRelease: Ubuntu 21.04 Package: qemu-system 1:5.2+dfsg-6ubuntu2 ProcVersionSignature: Ubuntu 5.11.0-11.12-generic 5.11.0 Uname: Linux 5.11.0-11-generic ppc64le .sys.firmware.opal.msglog: Error: [Errno 13] Permission denied: '/sys/firmware/opal/msglog' ApportVersion: 2.20.11-0ubuntu60 Architecture: ppc64el CasperMD5CheckResult: pass CurrentDesktop: Unity:Unity7:ubuntu Date: Mon Mar 22 14:48:39 2021 InstallationDate: Installed on 2021-03-22 (0 days ago) InstallationMedia: Ubuntu-Server 21.04 "Hirsute Hippo" - Alpha ppc64el (20210321) KvmCmdLine: COMMAND STAT EUID RUID PIDPPID %CPU COMMAND ProcKernelCmdLine: root=UUID=f3d03315-0944-4a02-9c87-09c00eba9fa1 ro ProcLoadAvg: 1.20 0.73 0.46 1/1054 6071 ProcSwaps: Filename TypeSizeUsed Priority /swap.img file 8388544 0 -2 ProcVersion: Linux version 5.11.0-11-generic (buildd@bos02-ppc64el-002) (gcc (Ubuntu 10.2.1-20ubuntu1) 10.2.1 20210220, GNU ld (GNU Binutils for Ubuntu) 2.36.1) #12-Ubuntu SMP Mon Mar 1 19:26:20 UTC 2021 SourcePackage: qemu UpgradeStatus: No upgrade log present (probably fresh install) VarLogDump_list: total 0 acpidump: cpu_cores: Number of cores present = 8 cpu_coreson: Number of cores online = 8 cpu_smt: SMT=4 To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1920784/+subscriptions
Re: Crashes with qemu-system-ppc64
On 3/24/21 12:00 AM, Greg Kurz wrote:
> Cc'ing David
>
> On Tue, 23 Mar 2021 17:48:36 +0100
> Thomas Huth wrote:
>
>>
>> In case anyone is interested in fixing those, there are two regressions with
>> qemu-system-ppc64 in the current master branch:
>>
>> $ ./qemu-system-ppc64 -M ppce500 -device macio-oldworld
>> qemu-system-ppc64: ../../devel/qemu/softmmu/memory.c:2443:
>> memory_region_add_subregion_common: Assertion `!subregion->container' failed.
>>
>> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
>> /home/thuth/devel/qemu/include/hw/boards.h:24:MACHINE: Object 0x5635bd53af10
>> is not an instance of type machine
>> Aborted (core dumped)
>>
>
> I've bisected this one to:
>
> 3df261b6676b5850e93d6fab3f7a98f8ee8f19c5 is the first bad commit
> commit 3df261b6676b5850e93d6fab3f7a98f8ee8f19c5
> Author: Peter Maydell
> Date: Fri Mar 13 17:24:47 2020 +
>
> softmmu/vl.c: Handle '-cpu help' and '-device help' before 'no default
> machine'
>
> Currently if you try to ask for the list of CPUs for a target
> architecture which does not specify a default machine type
> you just get an error:
>
> $ qemu-system-arm -cpu help
> qemu-system-arm: No machine specified, and there is no default
> Use -machine help to list supported machines
>
> Since the list of CPUs doesn't depend on the machine, this is
> unnecessarily unhelpful. "-device help" has a similar problem.
>
> Move the checks for "did the user ask for -cpu help or -device help"
> up so they precede the select_machine() call which checks that the
> user specified a valid machine type.
>
> Signed-off-by: Peter Maydell
> Signed-off-by: Paolo Bonzini
>
> softmmu/vl.c | 26 --
> 1 file changed, 16 insertions(+), 10 deletions(-)
> bisect run success
>
> This change is fine but it unveils a bad assumption.
>
> 0 0x764a3708 in raise () at /lib64/power9/libc.so.6
> #1 0x76483bcc in abort () at /lib64/power9/libc.so.6
> #2 0x0001008db940 in object_dynamic_cast_assert
> (obj=0x10126f670, typename=0x100c20380 "machine", file=0x100b34878
> "/home/greg/Work/qemu/qemu-ppc/include/hw/boards.h", line=,
> func=0x100bcd320 <__func__.30338> "MACHINE") at ../../qom/object.c:883
> #3 0x000100456e00 in MACHINE (obj=) at
> /home/greg/Work/qemu/qemu-ppc/include/hw/boards.h:24
> #4 0x000100456e00 in cpu_core_instance_init (obj=0x10118e2c0) at
> ../../hw/cpu/core.c:69
> #5 0x0001008d9f44 in object_init_with_type (obj=obj@entry=0x10118e2c0,
> ti=0x1011fd470) at ../../qom/object.c:375
> #6 0x0001008d9f24 in object_init_with_type (obj=obj@entry=0x10118e2c0,
> ti=0x101211ad0) at ../../qom/object.c:371
> #7 0x0001008d9f24 in object_init_with_type (obj=obj@entry=0x10118e2c0,
> ti=ti@entry=0x101212760) at ../../qom/object.c:371
> #8 0x0001008dc474 in object_initialize_with_type
> (obj=obj@entry=0x10118e2c0, size=size@entry=160, type=type@entry=0x101212760)
> at ../../qom/object.c:517
> #9 0x0001008dc678 in object_new_with_type (type=0x101212760) at
> ../../qom/object.c:732
> #10 0x0001009fbad8 in qmp_device_list_properties (typename= out>, errp=) at ../../qom/qom-qmp-cmds.c:146
> #11 0x0001005a4bf0 in qdev_device_help (opts=0x10126c200) at
> ../../softmmu/qdev-monitor.c:285
> #12 0x000100760afc in device_help_func (opaque=,
> opts=, errp=) at ../../softmmu/vl.c:1204
> #13 0x000100ad1050 in qemu_opts_foreach (list=,
> func=0x100760ae0 , opaque=0x0, errp=0x0) at
> ../../util/qemu-option.c:1167
> #14 0x0001007653cc in qemu_process_help_options () at
> ../../softmmu/vl.c:2451
> #15 0x0001007653cc in qemu_init (argc=, argv= out>, envp=) at ../../softmmu/vl.c:3521
> #16 0x0001002f4f88 in main (argc=, argv=,
> envp=) at ../../softmmu/main.c:49
>
> Basically, "-device power8_v2.0-spapr-cpu-core,help" ends up
> instantiating an object of the "power8_v2.0-spapr-cpu-core" type,
> which derives from "cpu-core". The "cpu-core" type has an instance
> init function that assumes that qdev_get_machine() returns an object
> of type "machine"...
>
> static void cpu_core_instance_init(Object *obj)
> {
> MachineState *ms = MACHINE(qdev_get_machine());
> ^^
> ...here.
>
> qdev_get_machine() cannot return a valid machine type since
> select_machine() hasn't been called yet... an instance init
> function is probably not the best place to use qdev_get_machine()
> if any.
Hmmm does this assert() matches your comment?
-- >8 --
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a9..41cbee77d14 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1130,6 +1130,8 @@ Object *qdev_get_machine(void)
{
static Object *dev;
+assert(phase_check(PHASE_MACHINE_CREATED));
+
if (dev == NULL) {
dev = container_get(object_get_root(), "/machine");
}
---
>
> CPUCore *core = CPU_CORE(obj);
Re: [RFC PATCH 08/13] blobs: Only install blobs if powerpc system targets are built
On Tue, Mar 23, 2021 at 04:51:27PM +0100, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> Cc: David Gibson
> Cc: Greg Kurz
> Cc: qemu-...@nongnu.org
> Cc: Mark Cave-Ayland
> Cc: Hervé Poussineau
> Cc: Cédric Le Goater
> Cc: BALATON Zoltan
> ---
> meson.build | 2 ++
> pc-bios/meson.build | 21 +
> 2 files changed, 15 insertions(+), 8 deletions(-)
Acked-by: David Gibson
>
> diff --git a/meson.build b/meson.build
> index e3418815b04..6f5561c2212 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -97,6 +97,7 @@
> install_blobs_arm = false
> install_blobs_hppa = false
> install_blobs_microblaze = false
> +install_blobs_ppc = false
> if get_option('install_blobs')
>foreach target : target_dirs
> install_edk2_blobs = install_edk2_blobs or target in edk2_targets
> @@ -104,6 +105,7 @@
> install_blobs_arm = install_blobs_hppa or target in ['arm-softmmu',
> 'aarch64-softmmu']
> install_blobs_hppa = install_blobs_hppa or target in ['hppa-softmmu']
> install_blobs_microblaze = install_blobs_microblaze or target in
> ['microblaze-softmmu', 'microblazeel-softmmu']
> +install_blobs_ppc = install_blobs_ppc or (target.startswith('ppc') and
> target.endswith('softmmu'))
>endforeach
> endif
>
> diff --git a/pc-bios/meson.build b/pc-bios/meson.build
> index a6185feff58..73d02a57628 100644
> --- a/pc-bios/meson.build
> +++ b/pc-bios/meson.build
> @@ -43,7 +43,6 @@
>'vgabios-ati.bin',
>'openbios-sparc32',
>'openbios-sparc64',
> - 'openbios-ppc',
>'QEMU,tcx.bin',
>'QEMU,cgthree.bin',
>'pxe-e1000.rom',
> @@ -60,8 +59,6 @@
>'efi-virtio.rom',
>'efi-e1000e.rom',
>'efi-vmxnet3.rom',
> - 'bamboo.dtb',
> - 'canyonlands.dtb',
>'multiboot.bin',
>'linuxboot.bin',
>'linuxboot_dma.bin',
> @@ -69,11 +66,6 @@
>'pvh.bin',
>'s390-ccw.img',
>'s390-netboot.img',
> - 'slof.bin',
> - 'skiboot.lid',
> - 'u-boot.e500',
> - 'u-boot-sam460-20100605.bin',
> - 'qemu_vga.ndrv',
>'opensbi-riscv32-generic-fw_dynamic.bin',
>'opensbi-riscv64-generic-fw_dynamic.bin',
>'opensbi-riscv32-generic-fw_dynamic.elf',
> @@ -111,6 +103,19 @@
>))
> endif
>
> +if install_blobs_ppc
> + blobs_ss.add(files(
> +'bamboo.dtb',
> +'canyonlands.dtb',
> +'openbios-ppc',
> +'qemu_vga.ndrv',
> +'slof.bin',
> +'skiboot.lid',
> +'u-boot.e500',
> +'u-boot-sam460-20100605.bin',
> + ))
> +endif
> +
> blobs_ss = blobs_ss.apply(config_host, strict: false)
>
> if get_option('install_blobs')
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH] Document qemu-img options data_file and data_file_raw
On 3/1/21 12:28 PM, Connor Kuehl wrote: The contents of this patch were initially developed and posted by Han Han[1], however, it appears the original patch was not applied. Since then, the relevant documentation has been moved and adapted to a new format. I've taken most of the original wording and tweaked it according to some of the feedback from the original patch submission. I've also adapted it to restructured text, which is the format the documentation currently uses. [1] https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg01253.html Reported-by: Han Han Co-developed-by: Han Han I think it's okay to just keep the "Signed-off-by" from Han Han here, and the implication is that you are signing off on modifications you've made since. Fixes: https://bugzilla.redhat.com/1763105 Signed-off-by: Connor Kuehl --- docs/tools/qemu-img.rst | 12 1 file changed, 12 insertions(+) diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst index b615aa8419..5cc585dc27 100644 --- a/docs/tools/qemu-img.rst +++ b/docs/tools/qemu-img.rst @@ -866,6 +866,18 @@ Supported image file formats: issue ``lsattr filename`` to check if the NOCOW flag is set or not (Capital 'C' is NOCOW flag). + ``data_file`` +Pathname that refers to a file that will store all guest data. If +this option is used, the qcow2 file will only contain the image's +metadata. + Might recommend "filename" simply for parity with *FILENAME* argument. (This is the first appearance of "Pathname" in this file without spaces, though "Path name" is indeed used several times.) + ``data_file_raw`` +If this option is set to ``on``, QEMU will always keep the external +data file consistent as a standalone read-only raw image. The default +value is ``off``. + +This option can only be enabled if ``data_file`` is set. + How does this interact with caching options, if it does? What happens in the negative case -- how does the file become inconsistent? ``Other`` QEMU also supports various other image file formats for
Re: [PATCH 1/1] iotests: fix 051.out expected output after error text touchups
On 3/18/21 4:09 PM, Connor Kuehl wrote:
A patch was recently applied that touched up some error messages that
pertained to key names like 'node-name'. The trouble is it only updated
tests/qemu-iotests/051.pc.out and not tests/qemu-iotests/051.out as
well.
Do that now.
Fixes: 785ec4b1b9 ("block: Clarify error messages pertaining to
'node-name'")
Signed-off-by: Connor Kuehl
Reviewed-by: John Snow
---
tests/qemu-iotests/051.out | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index de4771bcb3..db8c14b903 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -61,13 +61,13 @@ QEMU X.Y.Z monitor - type 'help' for more information
(qemu) quit
Testing: -drive file=TEST_DIR/t.qcow2,node-name=123foo
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=123foo: Invalid node name
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=123foo: Invalid node-name:
'123foo'
Testing: -drive file=TEST_DIR/t.qcow2,node-name=_foo
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=_foo: Invalid node name
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=_foo: Invalid node-name:
'_foo'
Testing: -drive file=TEST_DIR/t.qcow2,node-name=foo#12
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=foo#12: Invalid node name
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=foo#12: Invalid node-name:
'foo#12'
=== Device without drive ===
[PATCH 6/6] hw/isa/piix4: Fix leak removing unuseful qemu_allocate_irqs() call
We locally create an input IRQ with qemu_allocate_irqs() to
pass it as output IRQ of the PIC, but its handler simply dispatch
into another of our output IRQ ("intr" output).
Simplify by directly connecting the PIC output to our "intr"
output.
This fixes when using QEMU built with --enable-sanitizers:
==338425==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 8 byte(s) in 1 object(s) allocated from:
#0 0x5641b361e1df in malloc (qemu-system-mips+0x1b201df)
#1 0x7f995e683958 in g_malloc (/lib64/libglib-2.0.so.0+0x58958)
#2 0x5641b5558e36 in qemu_allocate_irqs hw/core/irq.c:66:12
#3 0x5641b4161817 in piix4_realize hw/isa/piix4.c:171:21
#4 0x5641b42f077a in pci_qdev_realize hw/pci/pci.c:2114:9
#5 0x5641b554c802 in device_set_realized hw/core/qdev.c:761:13
#6 0x5641b5578458 in property_set_bool qom/object.c:2257:5
#7 0x5641b55709e2 in object_property_set qom/object.c:1402:5
#8 0x5641b55861c9 in object_property_set_qobject qom/qom-qobject.c:28:10
#9 0x5641b5571831 in object_property_set_bool qom/object.c:1472:15
#10 0x5641b55410fd in qdev_realize hw/core/qdev.c:389:12
Fixes: 078778c5a55 ("piix4: Add an i8259 Interrupt Controller as specified in
datasheet")
Signed-off-by: Philippe Mathieu-Daudé
---
hw/isa/piix4.c | 10 +-
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index a50d97834c7..79ed20e2a1a 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -103,12 +103,6 @@ static const VMStateDescription vmstate_piix4 = {
}
};
-static void piix4_request_i8259_irq(void *opaque, int irq, int level)
-{
-PIIX4State *s = opaque;
-qemu_set_irq(s->cpu_intr, level);
-}
-
static void piix4_set_i8259_irq(void *opaque, int irq, int level)
{
PIIX4State *s = opaque;
@@ -149,7 +143,6 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
{
PIIX4State *s = PIIX4_PCI_DEVICE(dev);
ISABus *isa_bus;
-qemu_irq *i8259_out_irq;
isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
pci_address_space_io(dev), errp);
@@ -168,8 +161,7 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
PIIX_RCR_IOPORT, >rcr_mem, 1);
/* initialize i8259 pic */
-i8259_out_irq = qemu_allocate_irqs(piix4_request_i8259_irq, s, 1);
-s->isa = i8259_init(isa_bus, *i8259_out_irq);
+s->isa = i8259_init(isa_bus, s->cpu_intr);
/* initialize ISA irqs */
isa_bus_irqs(isa_bus, s->isa);
--
2.26.2
[PATCH 5/6] hw/isa/vt82c686: Simplify removing unuseful qemu_allocate_irqs() call
Instead of creating an input IRQ with qemu_allocate_irqs()
to pass it as output IRQ of the PIC, with its handler simply
dispatching into the "intr" output IRQ, simplify by directly
connecting the PIC to the "intr" named output.
Fixes: 3dc31cb8490 ("vt82c686: Move creation of ISA devices to the ISA bridge")
Signed-off-by: Philippe Mathieu-Daudé
---
hw/isa/vt82c686.c | 10 +-
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 87473ec121f..3dc3454858e 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -323,12 +323,6 @@ struct VT82C686BISAState {
SuperIOConfig superio_cfg;
};
-static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
-{
-VT82C686BISAState *s = opaque;
-qemu_set_irq(s->cpu_intr, level);
-}
-
static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
uint32_t val, int len)
{
@@ -384,14 +378,12 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
VT82C686BISAState *s = VT82C686B_ISA(d);
DeviceState *dev = DEVICE(d);
ISABus *isa_bus;
-qemu_irq *isa_irq;
int i;
qdev_init_gpio_out_named(dev, >cpu_intr, "intr", 1);
-isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
_fatal);
-isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
+isa_bus_irqs(isa_bus, i8259_init(isa_bus, s->cpu_intr));
i8254_pit_init(isa_bus, 0x40, 0, NULL);
i8257_dma_init(isa_bus, 0);
isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
--
2.26.2
[PATCH 4/6] hw/isa/vt82c686: Name output IRQ as 'intr'
Named IRQs are easier to understand in the monitor. Name the single output interrupt as 'intr'. Signed-off-by: Philippe Mathieu-Daudé --- hw/isa/vt82c686.c | 2 +- hw/mips/fuloong2e.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index 05d084f6982..87473ec121f 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -387,7 +387,7 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp) qemu_irq *isa_irq; int i; -qdev_init_gpio_out(dev, >cpu_intr, 1); +qdev_init_gpio_out_named(dev, >cpu_intr, "intr", 1); isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d), _fatal); diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c index 4f61f2c873b..931385c760f 100644 --- a/hw/mips/fuloong2e.c +++ b/hw/mips/fuloong2e.c @@ -206,7 +206,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc, dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true, TYPE_VT82C686B_ISA); -qdev_connect_gpio_out(DEVICE(dev), 0, intc); +qdev_connect_gpio_out_named(DEVICE(dev), "intr", 0, intc); dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide"); pci_ide_create_devs(dev); -- 2.26.2
[PATCH 2/6] hw/isa/i82378: Simplify removing unuseful qemu_allocate_irqs() call
When the i82378 model was added in commit a04ff940974 ("prep:
Add i82378 PCI-to-ISA bridge emulation") the i8259 model was
not yet QOM'ified. This happened later in commit 747c70af78f
("i8259: Convert to qdev").
Instead of creating an input IRQ with qemu_allocate_irqs()
to pass it as output IRQ of the PIC, with its handler simply
dispatching into the "intr" output IRQ, we can now simplify
and directly connect the PIC to the "intr" named output.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/isa/i82378.c | 9 +
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
index fd296c8ed7a..817eca47053 100644
--- a/hw/isa/i82378.c
+++ b/hw/isa/i82378.c
@@ -47,12 +47,6 @@ static const VMStateDescription vmstate_i82378 = {
},
};
-static void i82378_request_out0_irq(void *opaque, int irq, int level)
-{
-I82378State *s = opaque;
-qemu_set_irq(s->out[0], level);
-}
-
static void i82378_request_pic_irq(void *opaque, int irq, int level)
{
DeviceState *dev = opaque;
@@ -94,8 +88,7 @@ static void i82378_realize(PCIDevice *pci, Error **errp)
*/
/* 2 82C59 (irq) */
-s->i8259 = i8259_init(isabus,
- qemu_allocate_irq(i82378_request_out0_irq, s, 0));
+s->i8259 = i8259_init(isabus, s->out[0]);
isa_bus_irqs(isabus, s->i8259);
/* 1 82C54 (pit) */
--
2.26.2
[PATCH 1/6] hw/isa/i82378: Name output IRQ as 'intr'
Named IRQs are easier to understand in the monitor. Name the single output interrupt as 'intr'. Signed-off-by: Philippe Mathieu-Daudé --- hw/isa/i82378.c | 2 +- hw/ppc/prep.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c index 2a2ff05b937..fd296c8ed7a 100644 --- a/hw/isa/i82378.c +++ b/hw/isa/i82378.c @@ -113,7 +113,7 @@ static void i82378_init(Object *obj) DeviceState *dev = DEVICE(obj); I82378State *s = I82378(obj); -qdev_init_gpio_out(dev, s->out, 1); +qdev_init_gpio_out_named(dev, s->out, "intr", 1); qdev_init_gpio_in(dev, i82378_request_pic_irq, 16); } diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c index f1b1efdcef9..50d9b6f0d54 100644 --- a/hw/ppc/prep.c +++ b/hw/ppc/prep.c @@ -284,8 +284,8 @@ static void ibm_40p_init(MachineState *machine) /* PCI -> ISA bridge */ i82378_dev = DEVICE(pci_create_simple(pci_bus, PCI_DEVFN(11, 0), "i82378")); -qdev_connect_gpio_out(i82378_dev, 0, - cpu->env.irq_inputs[PPC6xx_INPUT_INT]); +qdev_connect_gpio_out_named(i82378_dev, "intr", 0, +cpu->env.irq_inputs[PPC6xx_INPUT_INT]); sysbus_connect_irq(pcihost, 0, qdev_get_gpio_in(i82378_dev, 15)); isa_bus = ISA_BUS(qdev_get_child_bus(i82378_dev, "isa.0")); -- 2.26.2
[PATCH 3/6] hw/isa/i82378: Rename output IRQ variable
The i82378 has 2 output IRQs: "INT" and "NMI".
We do not model the NMI, so simplify I82378State by
removing the unused IRQ. To avoid keeping an array of
one element, remove the array and rename the variable.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/isa/i82378.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
index 817eca47053..164d6c65f64 100644
--- a/hw/isa/i82378.c
+++ b/hw/isa/i82378.c
@@ -32,7 +32,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(I82378State, I82378)
struct I82378State {
PCIDevice parent_obj;
-qemu_irq out[2];
+qemu_irq intr;
qemu_irq *i8259;
MemoryRegion io;
};
@@ -88,7 +88,7 @@ static void i82378_realize(PCIDevice *pci, Error **errp)
*/
/* 2 82C59 (irq) */
-s->i8259 = i8259_init(isabus, s->out[0]);
+s->i8259 = i8259_init(isabus, s->intr);
isa_bus_irqs(isabus, s->i8259);
/* 1 82C54 (pit) */
@@ -106,7 +106,7 @@ static void i82378_init(Object *obj)
DeviceState *dev = DEVICE(obj);
I82378State *s = I82378(obj);
-qdev_init_gpio_out_named(dev, s->out, "intr", 1);
+qdev_init_gpio_out_named(dev, >intr, "intr", 1);
qdev_init_gpio_in(dev, i82378_request_pic_irq, 16);
}
--
2.26.2
Re: [PATCH v7 1/8] vt82c686: Implement control of serial port io ranges via config regs
On Tue, 23 Mar 2021, Mark Cave-Ayland wrote: On 23/03/2021 12:54, BALATON Zoltan wrote: On Wed, 10 Mar 2021, BALATON Zoltan wrote: In VIA super south bridge the io ranges of superio components (parallel and serial ports and FDC) can be controlled by superio config registers to set their base address and enable/disable them. This is not easy to implement in QEMU because ISA emulation is only designed to set io base address once on creating the device and io ranges are registered at creation and cannot easily be disabled or moved later. In this patch we hack around that but only for serial ports because those have a single io range at port base that's relatively easy to handle and it's what guests actually use and set address different than the default. We do not attempt to handle controlling the parallel and FDC regions because those have multiple io ranges so handling them would be messy and guests either don't change their deafult or don't care. We could even get away with disabling and not emulating them, but since they are already there, this patch leaves them mapped at their default address just in case this could be useful for a guest in the future. Signed-off-by: BALATON Zoltan Could this patch be reviewed now please? I've dropped it from later versions to avoid this holding back the series but now that it won't be in 6.0 I'd like to go back to this. This is implementing the behaviour of the real hardware better than the unsettable default value we have as a replacement. That approach also works for the guests I've tried (MorphOS and Linux) but if we can do better than why not do it? Do bear in mind that several people have already looked at this patch and haven't felt comfortable enough to review it, and I've also said in a I haven't heard back from many people (mostly only Philippe and you and maybe David looked at it but I did not expect him to review it as it's not his area) so I had the impression nobody cared or had time to look at it. That's why I've raised this again to get a clear answer about it. Now that at least you dislike this patch it's good enough reason to drop it for now and go with the default value instead until a guest is found to need this functionality. previous email that this isn't the right approach. Given that the ISA bus partly uses the ioport address to reference the device, manually moving the memory regions around for devices on the bus without the ISA bus seeing those changes is going to cause issues. The ioport_id you refer to is only used to print device name so the only issue is that the device may be currently mapped to a different address than its name might suggest when you query it but that should not break any functionality so maybe only cosmetic. The current ISA bus emulation in QEMU does not care about ISA devices after they are created, they are just initialised once and then the bus seems to forget about them. At least there are no functions to enable/disable or control them afterwards, only to register them at startup. So it's static and does not allow dynamically changing devices like we have in these VTxxx chips. (By the way, this is the same problem via-ide has that required hacks instead of actually emulating what the chip does because we can't have both ISA IDE and PCI IDE as the ISA one cannot be moved or disabled once created.) That's what this patch tried to circumvent at least for serial ports but without completly changing ISA emulation which might be a better approach but well beyond the effort I'm willing to put into this. IIRC the ability to dynamically change the standard ISA addresses was present in several motherboards from that era, and given that this functionality hasn't been implemented in QEMU this already tells us that no firmware is currently is using it. QEMU only supports SeaBIOS and on PC the ports are usually at their default address at startup so it's probably not firmware but guests that could change it. I don't know what other BIOSes might do as those don't run on QEMU. But these VTxxx chips are not used on PC machine models. The fuloong2e PMON and pegasos2 SmartFirtmware seem to poke the regs enabling and setting port address of these at startup (otherwise the chip seems to start without these enabled) so I've tried to emulate that. On pegasos2 it even uses different than usual standard value (as it only has one serial port) and guests boot with that so we can use that default. It's not exactly how the real device works but satisfies guests that rely on firmware to set up these and don't touch it later. I don't understand why using the hard-coded addresses in the v10 is a problem here? The addresses you added in the comments representing the programmed values are the standard ISA device addresses, so if those are what the firmware is programming then there will be no change. You also reported that it works fine with both your MorphOS and Linux test images,
[PATCH 0/6] hw/isa: Remove unuseful qemu_allocate_irqs() call
I started to fix the LeakSanitizer error in piix4_realize(), then looked for similar pattern and found 2 other places. The older is i82378 (historically the first one) which then spread. Philippe Mathieu-Daudé (6): hw/isa/i82378: Name output IRQ as 'intr' hw/isa/i82378: Simplify removing unuseful qemu_allocate_irqs() call hw/isa/i82378: Rename output IRQ variable hw/isa/vt82c686: Name output IRQ as 'intr' hw/isa/vt82c686: Simplify removing unuseful qemu_allocate_irqs() call hw/isa/piix4: Fix leak removing unuseful qemu_allocate_irqs() call hw/isa/i82378.c | 13 +++-- hw/isa/piix4.c | 10 +- hw/isa/vt82c686.c | 12 ++-- hw/mips/fuloong2e.c | 2 +- hw/ppc/prep.c | 4 ++-- 5 files changed, 9 insertions(+), 32 deletions(-) -- 2.26.2
Re: [RFC PATCH 06/13] blobs: Only install PA-RISC SeaBIOS if hppa system target is built
On 3/23/21 4:51 PM, Philippe Mathieu-Daudé wrote:
Signed-off-by: Philippe Mathieu-Daudé
---
Cc: Richard Henderson
Cc: Helge Deller
Acked-by: Helge Deller
---
meson.build | 2 ++
pc-bios/meson.build | 7 ++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index 10c21ef52af..a7f3b60d08e 100644
--- a/meson.build
+++ b/meson.build
@@ -95,11 +95,13 @@
install_edk2_blobs = false
install_blobs_alpha = false
install_blobs_arm = false
+install_blobs_hppa = false
if get_option('install_blobs')
foreach target : target_dirs
install_edk2_blobs = install_edk2_blobs or target in edk2_targets
install_blobs_alpha = install_blobs_alpha or target in ['alpha-softmmu']
install_blobs_arm = install_blobs_hppa or target in ['arm-softmmu',
'aarch64-softmmu']
+install_blobs_hppa = install_blobs_hppa or target in ['hppa-softmmu']
endforeach
endif
diff --git a/pc-bios/meson.build b/pc-bios/meson.build
index 635485931b9..c494c334e26 100644
--- a/pc-bios/meson.build
+++ b/pc-bios/meson.build
@@ -76,7 +76,6 @@
'u-boot.e500',
'u-boot-sam460-20100605.bin',
'qemu_vga.ndrv',
- 'hppa-firmware.img',
'opensbi-riscv32-generic-fw_dynamic.bin',
'opensbi-riscv64-generic-fw_dynamic.bin',
'opensbi-riscv32-generic-fw_dynamic.elf',
@@ -101,6 +100,12 @@
))
endif
+if install_blobs_hppa
+ blobs_ss.add(files(
+'hppa-firmware.img',
+ ))
+endif
+
blobs_ss = blobs_ss.apply(config_host, strict: false)
if get_option('install_blobs')
Re: Crashes with qemu-system-ppc64
Cc'ing David
On Tue, 23 Mar 2021 17:48:36 +0100
Thomas Huth wrote:
>
> In case anyone is interested in fixing those, there are two regressions with
> qemu-system-ppc64 in the current master branch:
>
> $ ./qemu-system-ppc64 -M ppce500 -device macio-oldworld
> qemu-system-ppc64: ../../devel/qemu/softmmu/memory.c:2443:
> memory_region_add_subregion_common: Assertion `!subregion->container' failed.
>
> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> /home/thuth/devel/qemu/include/hw/boards.h:24:MACHINE: Object 0x5635bd53af10
> is not an instance of type machine
> Aborted (core dumped)
>
I've bisected this one to:
3df261b6676b5850e93d6fab3f7a98f8ee8f19c5 is the first bad commit
commit 3df261b6676b5850e93d6fab3f7a98f8ee8f19c5
Author: Peter Maydell
Date: Fri Mar 13 17:24:47 2020 +
softmmu/vl.c: Handle '-cpu help' and '-device help' before 'no default
machine'
Currently if you try to ask for the list of CPUs for a target
architecture which does not specify a default machine type
you just get an error:
$ qemu-system-arm -cpu help
qemu-system-arm: No machine specified, and there is no default
Use -machine help to list supported machines
Since the list of CPUs doesn't depend on the machine, this is
unnecessarily unhelpful. "-device help" has a similar problem.
Move the checks for "did the user ask for -cpu help or -device help"
up so they precede the select_machine() call which checks that the
user specified a valid machine type.
Signed-off-by: Peter Maydell
Signed-off-by: Paolo Bonzini
softmmu/vl.c | 26 --
1 file changed, 16 insertions(+), 10 deletions(-)
bisect run success
This change is fine but it unveils a bad assumption.
0 0x764a3708 in raise () at /lib64/power9/libc.so.6
#1 0x76483bcc in abort () at /lib64/power9/libc.so.6
#2 0x0001008db940 in object_dynamic_cast_assert
(obj=0x10126f670, typename=0x100c20380 "machine", file=0x100b34878
"/home/greg/Work/qemu/qemu-ppc/include/hw/boards.h", line=,
func=0x100bcd320 <__func__.30338> "MACHINE") at ../../qom/object.c:883
#3 0x000100456e00 in MACHINE (obj=) at
/home/greg/Work/qemu/qemu-ppc/include/hw/boards.h:24
#4 0x000100456e00 in cpu_core_instance_init (obj=0x10118e2c0) at
../../hw/cpu/core.c:69
#5 0x0001008d9f44 in object_init_with_type (obj=obj@entry=0x10118e2c0,
ti=0x1011fd470) at ../../qom/object.c:375
#6 0x0001008d9f24 in object_init_with_type (obj=obj@entry=0x10118e2c0,
ti=0x101211ad0) at ../../qom/object.c:371
#7 0x0001008d9f24 in object_init_with_type (obj=obj@entry=0x10118e2c0,
ti=ti@entry=0x101212760) at ../../qom/object.c:371
#8 0x0001008dc474 in object_initialize_with_type
(obj=obj@entry=0x10118e2c0, size=size@entry=160, type=type@entry=0x101212760)
at ../../qom/object.c:517
#9 0x0001008dc678 in object_new_with_type (type=0x101212760) at
../../qom/object.c:732
#10 0x0001009fbad8 in qmp_device_list_properties (typename=,
errp=) at ../../qom/qom-qmp-cmds.c:146
#11 0x0001005a4bf0 in qdev_device_help (opts=0x10126c200) at
../../softmmu/qdev-monitor.c:285
#12 0x000100760afc in device_help_func (opaque=,
opts=, errp=) at ../../softmmu/vl.c:1204
#13 0x000100ad1050 in qemu_opts_foreach (list=,
func=0x100760ae0 , opaque=0x0, errp=0x0) at
../../util/qemu-option.c:1167
#14 0x0001007653cc in qemu_process_help_options () at
../../softmmu/vl.c:2451
#15 0x0001007653cc in qemu_init (argc=, argv=, envp=) at ../../softmmu/vl.c:3521
#16 0x0001002f4f88 in main (argc=, argv=,
envp=) at ../../softmmu/main.c:49
Basically, "-device power8_v2.0-spapr-cpu-core,help" ends up
instantiating an object of the "power8_v2.0-spapr-cpu-core" type,
which derives from "cpu-core". The "cpu-core" type has an instance
init function that assumes that qdev_get_machine() returns an object
of type "machine"...
static void cpu_core_instance_init(Object *obj)
{
MachineState *ms = MACHINE(qdev_get_machine());
^^
...here.
qdev_get_machine() cannot return a valid machine type since
select_machine() hasn't been called yet... an instance init
function is probably not the best place to use qdev_get_machine()
if any.
CPUCore *core = CPU_CORE(obj);
core->nr_threads = ms->smp.threads;
}
It seems that this should rather sit in a device realize function,
when the machine type is known.
> Thomas
>
>
Re: Crashes with qemu-system-ppc64
On Tue, 23 Mar 2021 at 21:21, Mark Cave-Ayland wrote: > I'm not sure what the right solution is here. In my mind there hasn't really > been any > difference between TYPE_DEVICE and TYPE_SYS_BUS_DEVICE other than the APIs > that > expose the memory regions and IRQs are different, but clearly platform bus > expects/defines a different behaviour here. > > Probably the quickest solution for now would be to change the DBDMA device so > that it > is derived from TYPE_DEVICE rather than TYPE_SYS_BUS_DEVICE and make the > relevant > changes if everyone agrees? You want to be at least cautious about doing that. TYPE_DEVICE objects don't get reset usually, because they are not part of the qbus hierarchy (all TYPE_SYS_BUS_DEVICE devices live on the sysbus and get reset when the sysbus is reset). So you would need the (currently nonexistent) reset function of the containing macio device to manually reset any TYPE_DEVICE children it has. (This is one of the areas where reset is a mess, incidentally: logically speaking if you have a PCI device then you want its children to all get reset when the PCI device itself is reset; as it stands that doesn't happen, because its sysbus children are on the sysbus, and a pci-bus-reset won't touch them.) I forget how the platform bus stuff is supposed to work, but something should be arranging that it only happens for a pretty restrictive subset of devices -- in general it should certainly not be firing for random sysbus devices that are part of something else. It's a pretty old commit (from 2018, one of Igor's), but I wonder if this was broken by commit a3fc8396352e945f9. The original design of the platform bus was that the "grab unassigned IRQs and MMIO regions" hook got run as a machine-init-done hook. That meant that by definition the board code had finished setting up all its sysbus devices, and anything still unconnected must be (assuming not a bug) something the user requested via -device to be put on the platform bus. But in commit a3fc8396352e945f9 we changed this to use the hotplug-handler callbacks, which happen when the device is realized. So it stopped being true that we would only find loose MMIOs and IRQs on the user's sysbus devices and now we're also incorrectly grabbing parts of devices that are supposed to be being wired up by QEMU code before that code has a chance to do that wiring. There must still be something causing this not to happen literally for every sysbus device, or we'd have noticed a lot earlier. I'm not sure what's specifically different here, but I think it is that: (1) we only create the platform bus itself as pretty much the last thing we do in machine init. This (accidentally?) means it doesn't get to see most of the sysbus devices in the board itself (2) macio-oldworld is a pluggable PCI device which happens to use a sysbus device as part of its implementation, which is probably not very common I think the long term fix is that we either need to undo a3fc8396352e945f9 so that we only run after all other device creation code has run and the unassigned IRQs and MMIOs really are just the loose ends, or alternatively we need to make the hooks much more restrictive about identifying what devices are supposed to go into the platform bus. Second note: does it actually make sense for a user to create a macio-oldworld device and plug it into anything? It's a PCI device, but is it really a generally usable device rather than a specific built-into-the-board part of the g3beige machine ? If it isn't actually useful for a user to create it on the command line with -device, we could sidestep the whole thing for 6.0 by marking it dc->user_creatable = false ... thanks -- PMM
Re: [RFC v11 30/55] target/arm: wrap call to aarch64_sve_change_el in tcg_enabled()
Claudio Fontana writes:
> After this patch it is possible to build only kvm:
>
> ./configure --disable-tcg --enable-kvm
FWIW at this point we get a different failure than later on:
21:10:25 [alex@aarch64-new:~/l/q/b/disable.tcg] (94e2abe0…)|… + make
check-qtest
GIT ui/keycodemapdb tests/fp/berkeley-testfloat-3
tests/fp/berkeley-softfloat-3 meson dtc capstone slirp
[1/19] Generating qemu-version.h with a meson_exe.py custom command
Running test qtest-aarch64/qom-test
qemu-system-aarch64: missing interface 'idau-interface' for object 'machine'
socket_accept failed: Resource temporarily unavailable
**
ERROR:../../tests/qtest/libqtest.c:319:qtest_init_without_qmp_handshake:
assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
ERROR qtest-aarch64/qom-test - Bail out!
ERROR:../../tests/qtest/libqtest.c:319:qtest_init_without_qmp_handshake:
assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
make: *** [Makefile.mtest:24: run-test-1] Error 1
>
> Signed-off-by: Claudio Fontana
> ---
> target/arm/cpu-sysemu.c | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/target/arm/cpu-sysemu.c b/target/arm/cpu-sysemu.c
> index eb928832a9..05d6e79ad9 100644
> --- a/target/arm/cpu-sysemu.c
> +++ b/target/arm/cpu-sysemu.c
> @@ -820,11 +820,13 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
> unsigned int cur_el = arm_current_el(env);
> int rt;
>
> -/*
> - * Note that new_el can never be 0. If cur_el is 0, then
> - * el0_a64 is is_a64(), else el0_a64 is ignored.
> - */
> -aarch64_sve_change_el(env, cur_el, new_el, is_a64(env));
> +if (tcg_enabled()) {
> +/*
> + * Note that new_el can never be 0. If cur_el is 0, then
> + * el0_a64 is is_a64(), else el0_a64 is ignored.
> + */
> +aarch64_sve_change_el(env, cur_el, new_el, is_a64(env));
> +}
>
> if (cur_el < new_el) {
> /* Entry vector offset depends on whether the implemented EL
--
Alex Bennée
Re: [PATCH v2] docs/devel/testing.rst: Fix references to unit tests
On 3/18/21 1:44 PM, Wainer dos Santos Moschetta wrote:
With the recent move of the unit tests to tests/unit directory some
instructions under the "Unit tests" section became imprecise, which
are fixed by this change.
Fixes: da668aa15b99 ("tests: Move unit tests into a separate directory")
Signed-off-by: Wainer dos Santos Moschetta
Reviewed-by: Thomas Huth
Reviewed-by: John Snow
---
v1->v2:
* Fixed typo on subject [jsnow]
* Replaced Related-to with Fixes [jsnow]
docs/devel/testing.rst | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 1434a50cc4..1da4c4e4c4 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -34,17 +34,17 @@ If you are writing new code in QEMU, consider adding a unit
test, especially
for utility modules that are relatively stateless or have few dependencies. To
add a new unit test:
-1. Create a new source file. For example, ``tests/foo-test.c``.
+1. Create a new source file. For example, ``tests/unit/foo-test.c``.
2. Write the test. Normally you would include the header file which exports
the module API, then verify the interface behaves as expected from your
test. The test code should be organized with the glib testing framework.
Copying and modifying an existing test is usually a good idea.
-3. Add the test to ``tests/meson.build``. The unit tests are listed in a
+3. Add the test to ``tests/unit/meson.build``. The unit tests are listed in a
dictionary called ``tests``. The values are any additional sources and
dependencies to be linked with the test. For a simple test whose source
- is in ``tests/foo-test.c``, it is enough to add an entry like::
+ is in ``tests/unit/foo-test.c``, it is enough to add an entry like::
{
...
Re: [PATCH v3 1/1] acpi: Consolidate the handling of OEM ID and OEM Table ID fields
"Michael S. Tsirkin" writes:
>> +#include "qemu/cutils.h"
>> +
>> +#define ACPI_BUILD_APPNAME6 "BOCHS "
>> +#define ACPI_BUILD_APPNAME8 "BXPC"
>
> A single user for each of these now ... drop the defines?
>
Unfortunately ACPI_BUILD_APPNAME8 is still used in build_header() in
aml-build.c, and to me it didn't look nice for one to have a define and
the other not, but if you prefer with only ACPI_BUILD_APPNAME8 as a
define, I can do the change.
>> +#define ACPI_INIT_DEFAULT_BUILD_OEM(__bld_oem) do { \
>> +ACPI_INIT_BUILD_OEM(__bld_oem, \
>> +ACPI_BUILD_APPNAME6, ACPI_BUILD_APPNAME8); \
>> +} while (0)
>
> OK but ... why are these macros? Won't inline functions
> work just as well with more type safety?
>
Didn't know what was the attitude in this project to inlined functions
in headers and went with macros. I will change to inlined functions.
Re: [PULL 0/7] target-arm queue
On Tue, 23 Mar 2021 at 14:26, Peter Maydell wrote: > > Small pullreq with some bug fixes to go into rc1. > > -- PMM > > The following changes since commit 5ca634afcf83215a9a54ca6e66032325b5ffb5f6: > > Merge remote-tracking branch 'remotes/philmd/tags/sdmmc-20210322' into > staging (2021-03-22 18:50:25 +) > > are available in the Git repository at: > > https://git.linaro.org/people/pmaydell/qemu-arm.git > tags/pull-target-arm-20210323 > > for you to fetch changes up to dad90de78e9e9d47cefcbcd30115706b98e6ec87: > > target/arm: Set ARMMMUFaultInfo.level in user-only arm_cpu_tlb_fill > (2021-03-23 14:07:55 +) > > > target-arm queue: > * hw/arm/virt: Disable pl011 clock migration if needed > * target/arm: Make M-profile VTOR loads on reset handle memory aliasing > * target/arm: Set ARMMMUFaultInfo.level in user-only arm_cpu_tlb_fill > > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0 for any user-visible changes. -- PMM
Re: [PATCH 12/28] qapi: Consistently permit any case in downstream prefixes
On 3/23/21 5:40 AM, Markus Armbruster wrote: We require lowercase __RFQDN_ downstream prefixes only where we require the prefixed name to be lowercase. Don't; permit any case in __RFQDN_ prefixes anywhere. Signed-off-by: Markus Armbruster Reviewed-by: John Snow --- scripts/qapi/expr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index a815060ee2..b5fb0be48b 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -51,7 +51,7 @@ def check_name_upper(name, info, source): def check_name_lower(name, info, source, permit_upper=False): stem = check_name_str(name, info, source) -if not permit_upper and name.lower() != name: +if not permit_upper and re.search(r'[A-Z]', stem): raise QAPISemError( info, "%s uses uppercase in name" % source) # TODO reject '_' in stem
Re: [PATCH v2 1/3] vt82c686.c: don't raise SCI when PCI_INTERRUPT_PIN isn't setup
On Tue, 23 Mar 2021, Philippe Mathieu-Daudé wrote:
Hi Isaku,
On 3/23/21 6:24 PM, Isaku Yamahata wrote:
Without this patch, the following patch will triger clan runtime
sanitizer warnings as follows. This patch proactively works around it.
I let v582c686.c maintainer address a correct fix as I'm not sure
about fuloong2e device model.
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
QTEST_QEMU_IMG=./qemu-img
G_TEST_DBUS_DAEMON=/home/petmay01/linaro/qemu-for-merges/tests/dbus-vmstate-daemon.sh
QTEST_QEMU_BINARY=./qemu-system-mips64el tests/qtest/qom-test --tap -k
PASS 1 qtest-mips64el/qom-test /mips64el/qom/loongson3-virt
PASS 2 qtest-mips64el/qom-test /mips64el/qom/none
PASS 3 qtest-mips64el/qom-test /mips64el/qom/magnum
PASS 4 qtest-mips64el/qom-test /mips64el/qom/mipssim
PASS 5 qtest-mips64el/qom-test /mips64el/qom/malta
../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative
PASS 6 qtest-mips64el/qom-test /mips64el/qom/fuloong2e
PASS 7 qtest-mips64el/qom-test /mips64el/qom/boston
PASS 8 qtest-mips64el/qom-test /mips64el/qom/pica61
and similarly for eg
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
QTEST_QEMU_IMG=./qemu-img
G_TEST_DBUS_DAEMON=/home/petmay01/linaro/qemu-for-merges/tests/dbus-vmstate-daemon.sh
QTEST_QEMU_BINARY=./qemu-system-mips64el tests/qtest/endianness-test
--tap -k
../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative
PASS 1 qtest-mips64el/endianness-test /mips64el/endianness/fuloong2e
../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative
PASS 2 qtest-mips64el/endianness-test /mips64el/endianness/split/fuloong2e
../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative
PASS 3 qtest-mips64el/endianness-test /mips64el/endianness/combine/fuloong2e
Cc: Huacai Chen
Cc: "Philippe Mathieu-Daudé"
Cc: Jiaxun Yang
Reported-by: Peter Maydell
Signed-off-by: Isaku Yamahata
---
hw/isa/vt82c686.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 05d084f698..f0fb309f12 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -144,7 +144,18 @@ static void pm_update_sci(ViaPMState *s)
ACPI_BITMASK_POWER_BUTTON_ENABLE |
ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
ACPI_BITMASK_TIMER_ENABLE)) != 0);
-pci_set_irq(>dev, sci_level);
+if (pci_get_byte(s->dev.config + PCI_INTERRUPT_PIN)) {
+/*
+ * FIXME:
+ * Fix device model that realizes this PM device and remove
+ * this work around.
+ * The device model should wire SCI and setup
+ * PCI_INTERRUPT_PIN properly.
+ * If PIN# = 0(interrupt pin isn't used), don't raise SCI as
+ * work around.
+ */
+pci_set_irq(>dev, sci_level);
I'll defer this to Zoltan.
I don't know anything about this, this was there well before I've touched
this device model:
https://git.qemu.org/?p=qemu.git;a=blame;f=hw/isa/vt82c686.c;hb=8063396bf3459a810d24e3efd6110b8480f0de5b
Personally I wouldn't care about SCI_EN on the vt82c686, as
it is not used by x86 machines (IOW, I'd not modify via_pm_reset
and KISS).
I'm not sure but maybe then you could also just remove the PM parts from
the device model as it probably does not work correctly anyway at the
moment as it may not be correctly wired up to config registers. I'm not
sure if it's needed by any guests but it was there for some reason and
maybe better to fix it if possible than dropping it. As a workaround I'm
OK with the proposed patch, I don't think it would break anything but
haven't tested it.
Regards,
BALATON Zoltan
+}
/* schedule a timer interruption if needed */
acpi_pm_tmr_update(>ar, (s->ar.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE)
&&
!(pmsts & ACPI_BITMASK_TIMER_STATUS));
[PATCH v2 07/10] Acceptance Tests: set up SSH connection by default after boot for LinuxTest
The LinuxTest specifically targets users that need to interact with Linux
guests. So, it makes sense to give a connection by default, and avoid
requiring it as boiler-plate code.
Signed-off-by: Cleber Rosa
---
tests/acceptance/avocado_qemu/__init__.py | 5 -
tests/acceptance/virtiofs_submounts.py| 1 -
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/tests/acceptance/avocado_qemu/__init__.py
b/tests/acceptance/avocado_qemu/__init__.py
index 535f63a48d..4960142bcc 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -390,7 +390,7 @@ def set_up_cloudinit(self, ssh_pubkey=None):
cloudinit_iso = self.prepare_cloudinit(ssh_pubkey)
self.vm.add_args('-drive', 'file=%s,format=raw' % cloudinit_iso)
-def launch_and_wait(self):
+def launch_and_wait(self, set_up_ssh_connection=True):
self.vm.set_console()
self.vm.launch()
console_drainer =
datadrainer.LineLogger(self.vm.console_socket.fileno(),
@@ -398,3 +398,6 @@ def launch_and_wait(self):
console_drainer.start()
self.log.info('VM launched, waiting for boot confirmation from guest')
cloudinit.wait_for_phone_home(('0.0.0.0', self.phone_home_port),
self.name)
+if set_up_ssh_connection:
+self.log.info('Setting up the SSH connection')
+self.ssh_connect(self.username, self.ssh_key)
diff --git a/tests/acceptance/virtiofs_submounts.py
b/tests/acceptance/virtiofs_submounts.py
index e10a935ac4..e019d3b896 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -136,7 +136,6 @@ def set_up_virtiofs(self):
def launch_vm(self):
self.launch_and_wait()
-self.ssh_connect('root', self.ssh_key)
def set_up_nested_mounts(self):
scratch_dir = os.path.join(self.shared_dir, 'scratch')
--
2.25.4
Re: [PATCH 13/28] qapi: Enforce event naming rules
On 3/23/21 5:40 AM, Markus Armbruster wrote:
Event names should be ALL_CAPS with words separated by underscore.
Enforce this. The only offenders are in tests/. Fix them. Existing
test event-case covers the new error.
Signed-off-by: Markus Armbruster
---
tests/unit/test-qmp-event.c | 6 +++---
scripts/qapi/expr.py | 4 +++-
tests/qapi-schema/doc-good.json | 4 ++--
tests/qapi-schema/doc-good.out| 4 ++--
tests/qapi-schema/doc-good.txt| 2 +-
tests/qapi-schema/doc-invalid-return.json | 4 ++--
tests/qapi-schema/event-case.err | 2 ++
tests/qapi-schema/event-case.json | 2 --
tests/qapi-schema/event-case.out | 14 --
tests/qapi-schema/qapi-schema-test.json | 6 +++---
tests/qapi-schema/qapi-schema-test.out| 8
11 files changed, 22 insertions(+), 34 deletions(-)
diff --git a/tests/unit/test-qmp-event.c b/tests/unit/test-qmp-event.c
index 047f44ff9a..d58c3b78f2 100644
--- a/tests/unit/test-qmp-event.c
+++ b/tests/unit/test-qmp-event.c
@@ -143,7 +143,7 @@ static void test_event_d(TestEventData *data,
static void test_event_deprecated(TestEventData *data, const void *unused)
{
-data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST-EVENT-FEATURES1'
}");
+data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES1'
}");
memset(_policy, 0, sizeof(compat_policy));
@@ -163,7 +163,7 @@ static void test_event_deprecated_data(TestEventData *data, const void *unused)
{
memset(_policy, 0, sizeof(compat_policy));
-data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST-EVENT-FEATURES0',"
+data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES0',"
" 'data': { 'foo': 42 } }");
qapi_event_send_test_event_features0(42);
g_assert(data->emitted);
@@ -172,7 +172,7 @@ static void test_event_deprecated_data(TestEventData *data,
const void *unused)
compat_policy.has_deprecated_output = true;
compat_policy.deprecated_output = COMPAT_POLICY_OUTPUT_HIDE;
-data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST-EVENT-FEATURES0'
}");
+data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES0'
}");
qapi_event_send_test_event_features0(42);
g_assert(data->emitted);
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index b5fb0be48b..c065505b27 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -45,7 +45,9 @@ def check_name_str(name, info, source):
def check_name_upper(name, info, source):
stem = check_name_str(name, info, source)
-# TODO reject '[a-z-]' in @stem
+if re.search(r'[a-z-]', stem):
+raise QAPISemError(
+info, "name of %s must not use lowercase or '-'" % source)
Does a little bit more than check_name_upper. Is this only used for
event names? I guess so. Should it be inlined into check_defn_name_str
instead in this case, or nah?
[PATCH v2 08/10] tests/acceptance/virtiofs_submounts.py: remove launch_vm()
The LinuxTest class' launch_and_wait() method now behaves the same way as this test's custom launch_vm(), so let's just use the upper layer (common) method. Signed-off-by: Cleber Rosa --- tests/acceptance/virtiofs_submounts.py | 13 + 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py index e019d3b896..d77ee35674 100644 --- a/tests/acceptance/virtiofs_submounts.py +++ b/tests/acceptance/virtiofs_submounts.py @@ -134,9 +134,6 @@ def set_up_virtiofs(self): '-numa', 'node,memdev=mem') -def launch_vm(self): -self.launch_and_wait() - def set_up_nested_mounts(self): scratch_dir = os.path.join(self.shared_dir, 'scratch') try: @@ -225,7 +222,7 @@ def test_pre_virtiofsd_set_up(self): self.set_up_nested_mounts() self.set_up_virtiofs() -self.launch_vm() +self.launch_and_wait() self.mount_in_guest() self.check_in_guest() @@ -235,14 +232,14 @@ def test_pre_launch_set_up(self): self.set_up_nested_mounts() -self.launch_vm() +self.launch_and_wait() self.mount_in_guest() self.check_in_guest() def test_post_launch_set_up(self): self.set_up_shared_dir() self.set_up_virtiofs() -self.launch_vm() +self.launch_and_wait() self.set_up_nested_mounts() @@ -252,7 +249,7 @@ def test_post_launch_set_up(self): def test_post_mount_set_up(self): self.set_up_shared_dir() self.set_up_virtiofs() -self.launch_vm() +self.launch_and_wait() self.mount_in_guest() self.set_up_nested_mounts() @@ -265,7 +262,7 @@ def test_two_runs(self): self.set_up_nested_mounts() self.set_up_virtiofs() -self.launch_vm() +self.launch_and_wait() self.mount_in_guest() self.check_in_guest() -- 2.25.4
[PATCH v2 04/10] Acceptance Tests: move useful ssh methods to base class
Both the virtiofs submounts and the linux ssh mips malta tests
contains useful methods related to ssh that deserve to be made
available to other tests. Let's move them to the base LinuxTest
class.
The method that helps with setting up an ssh connection will now
support both key and password based authentication, defaulting to key
based.
Signed-off-by: Cleber Rosa
Reviewed-by: Wainer dos Santos Moschetta
Reviewed-by: Willian Rampazzo
---
tests/acceptance/avocado_qemu/__init__.py | 48 ++-
tests/acceptance/linux_ssh_mips_malta.py | 38 ++
tests/acceptance/virtiofs_submounts.py| 37 -
3 files changed, 50 insertions(+), 73 deletions(-)
diff --git a/tests/acceptance/avocado_qemu/__init__.py
b/tests/acceptance/avocado_qemu/__init__.py
index 83b1741ec8..67f75f66e5 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -20,6 +20,7 @@
from avocado.utils import cloudinit
from avocado.utils import datadrainer
from avocado.utils import network
+from avocado.utils import ssh
from avocado.utils import vmimage
from avocado.utils.path import find_command
@@ -43,6 +44,8 @@
from qemu.accel import kvm_available
from qemu.accel import tcg_available
from qemu.machine import QEMUMachine
+from qemu.utils import get_info_usernet_hostfwd_port
+
def is_readable_executable_file(path):
return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
@@ -253,7 +256,50 @@ def fetch_asset(self, name,
cancel_on_missing=cancel_on_missing)
-class LinuxTest(Test):
+class LinuxSSHMixIn:
+"""Contains utility methods for interacting with a guest via SSH."""
+
+def ssh_connect(self, username, credential, credential_is_key=True):
+self.ssh_logger = logging.getLogger('ssh')
+res = self.vm.command('human-monitor-command',
+ command_line='info usernet')
+port = get_info_usernet_hostfwd_port(res)
+self.assertIsNotNone(port)
+self.assertGreater(port, 0)
+self.log.debug('sshd listening on port: %d', port)
+if credential_is_key:
+self.ssh_session = ssh.Session('127.0.0.1', port=port,
+ user=username, key=credential)
+else:
+self.ssh_session = ssh.Session('127.0.0.1', port=port,
+ user=username, password=credential)
+for i in range(10):
+try:
+self.ssh_session.connect()
+return
+except:
+time.sleep(4)
+pass
+self.fail('ssh connection timeout')
+
+def ssh_command(self, command):
+self.ssh_logger.info(command)
+result = self.ssh_session.cmd(command)
+stdout_lines = [line.rstrip() for line
+in result.stdout_text.splitlines()]
+for line in stdout_lines:
+self.ssh_logger.info(line)
+stderr_lines = [line.rstrip() for line
+in result.stderr_text.splitlines()]
+for line in stderr_lines:
+self.ssh_logger.warning(line)
+
+self.assertEqual(result.exit_status, 0,
+ f'Guest command failed: {command}')
+return stdout_lines, stderr_lines
+
+
+class LinuxTest(Test, LinuxSSHMixIn):
"""Facilitates having a cloud-image Linux based available.
For tests that indend to interact with guests, this is a better choice
diff --git a/tests/acceptance/linux_ssh_mips_malta.py
b/tests/acceptance/linux_ssh_mips_malta.py
index 052008f02d..3f590a081f 100644
--- a/tests/acceptance/linux_ssh_mips_malta.py
+++ b/tests/acceptance/linux_ssh_mips_malta.py
@@ -12,7 +12,7 @@
import time
from avocado import skipUnless
-from avocado_qemu import Test
+from avocado_qemu import Test, LinuxSSHMixIn
from avocado_qemu import wait_for_console_pattern
from avocado.utils import process
from avocado.utils import archive
@@ -21,7 +21,7 @@
from qemu.utils import get_info_usernet_hostfwd_port
-class LinuxSSH(Test):
+class LinuxSSH(Test, LinuxSSHMixIn):
timeout = 150 # Not for 'configure --enable-debug --enable-debug-tcg'
@@ -72,41 +72,9 @@ def get_kernel_info(self, endianess, wordsize):
def setUp(self):
super(LinuxSSH, self).setUp()
-def ssh_connect(self, username, password):
-self.ssh_logger = logging.getLogger('ssh')
-res = self.vm.command('human-monitor-command',
- command_line='info usernet')
-port = get_info_usernet_hostfwd_port(res)
-if not port:
-self.cancel("Failed to retrieve SSH port")
-self.log.debug("sshd listening on port:" + port)
-self.ssh_session = ssh.Session(self.VM_IP, port=int(port),
- user=username, password=password)
-for i in range(10):
-try:
-
Re: [PATCH 11/28] qapi: Move uppercase rejection to check_name_lower()
On 3/23/21 5:40 AM, Markus Armbruster wrote:
check_name_lower() is the only user of check_name_str() using
permit_upper=False. Move the associated code from check_name_str() to
check_name_lower(), and drop the parameter.
Signed-off-by: Markus Armbruster
Reviewed-by: John Snow
---
scripts/qapi/expr.py | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 30285fe334..a815060ee2 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -34,32 +34,31 @@ def check_name_is_str(name, info, source):
raise QAPISemError(info, "%s requires a string name" % source)
-def check_name_str(name, info, source,
- permit_upper=False):
+def check_name_str(name, info, source):
# Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
# and 'q_obj_*' implicit type names.
match = valid_name.match(name)
if not match or c_name(name, False).startswith('q_'):
raise QAPISemError(info, "%s has an invalid name" % source)
-if not permit_upper and name.lower() != name:
-raise QAPISemError(
-info, "%s uses uppercase in name" % source)
return match.group(3)
def check_name_upper(name, info, source):
-stem = check_name_str(name, info, source, permit_upper=True)
+stem = check_name_str(name, info, source)
# TODO reject '[a-z-]' in @stem
def check_name_lower(name, info, source,
permit_upper=False):
-stem = check_name_str(name, info, source, permit_upper)
+stem = check_name_str(name, info, source)
+if not permit_upper and name.lower() != name:
+raise QAPISemError(
+info, "%s uses uppercase in name" % source)
# TODO reject '_' in stem
def check_name_camel(name, info, source):
-stem = check_name_str(name, info, source, permit_upper=True)
+stem = check_name_str(name, info, source)
# TODO reject '[_-]' in stem, require CamelCase
[PATCH v2 03/10] Python: add utility function for retrieving port redirection
Slightly different versions for the same utility code are currently
present on different locations. This unifies them all, giving
preference to the version from virtiofs_submounts.py, because of the
last tweaks added to it.
While at it, this adds a "qemu.utils" module to host the utility
function and a test.
Signed-off-by: Cleber Rosa
Reviewed-by: Wainer dos Santos Moschetta
---
python/qemu/utils.py | 35
tests/acceptance/info_usernet.py | 29
tests/acceptance/linux_ssh_mips_malta.py | 16 +--
tests/acceptance/virtiofs_submounts.py | 21 --
tests/vm/basevm.py | 7 ++---
5 files changed, 78 insertions(+), 30 deletions(-)
create mode 100644 python/qemu/utils.py
create mode 100644 tests/acceptance/info_usernet.py
diff --git a/python/qemu/utils.py b/python/qemu/utils.py
new file mode 100644
index 00..89a246ab30
--- /dev/null
+++ b/python/qemu/utils.py
@@ -0,0 +1,35 @@
+"""
+QEMU utility library
+
+This offers miscellaneous utility functions, which may not be easily
+distinguishable or numerous to be in their own module.
+"""
+
+# Copyright (C) 2021 Red Hat Inc.
+#
+# Authors:
+# Cleber Rosa
+#
+# This work is licensed under the terms of the GNU GPL, version 2. See
+# the COPYING file in the top-level directory.
+#
+
+import re
+from typing import Optional
+
+
+def get_info_usernet_hostfwd_port(info_usernet_output: str) -> Optional[int]:
+"""
+Returns the port given to the hostfwd parameter via info usernet
+
+:param info_usernet_output: output generated by hmp command "info usernet"
+:param info_usernet_output: str
+:return: the port number allocated by the hostfwd option
+:rtype: int
+"""
+for line in info_usernet_output.split('\r\n'):
+regex = r'TCP.HOST_FORWARD.*127\.0\.0\.1\s+(\d+)\s+10\.'
+match = re.search(regex, line)
+if match is not None:
+return int(match[1])
+return None
diff --git a/tests/acceptance/info_usernet.py b/tests/acceptance/info_usernet.py
new file mode 100644
index 00..9c1fd903a0
--- /dev/null
+++ b/tests/acceptance/info_usernet.py
@@ -0,0 +1,29 @@
+# Test for the hmp command "info usernet"
+#
+# Copyright (c) 2021 Red Hat, Inc.
+#
+# Author:
+# Cleber Rosa
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later. See the COPYING file in the top-level directory.
+
+from avocado_qemu import Test
+
+from qemu.utils import get_info_usernet_hostfwd_port
+
+
+class InfoUsernet(Test):
+
+def test_hostfwd(self):
+self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22')
+self.vm.launch()
+res = self.vm.command('human-monitor-command',
+ command_line='info usernet')
+port = get_info_usernet_hostfwd_port(res)
+self.assertIsNotNone(port,
+ ('"info usernet" output content does not seem to '
+ 'contain the redirected port'))
+self.assertGreater(port, 0,
+ ('Found a redirected port that is not greater than'
+' zero'))
diff --git a/tests/acceptance/linux_ssh_mips_malta.py
b/tests/acceptance/linux_ssh_mips_malta.py
index 6dbd02d49d..052008f02d 100644
--- a/tests/acceptance/linux_ssh_mips_malta.py
+++ b/tests/acceptance/linux_ssh_mips_malta.py
@@ -18,6 +18,8 @@
from avocado.utils import archive
from avocado.utils import ssh
+from qemu.utils import get_info_usernet_hostfwd_port
+
class LinuxSSH(Test):
@@ -70,18 +72,14 @@ def get_kernel_info(self, endianess, wordsize):
def setUp(self):
super(LinuxSSH, self).setUp()
-def get_portfwd(self):
+def ssh_connect(self, username, password):
+self.ssh_logger = logging.getLogger('ssh')
res = self.vm.command('human-monitor-command',
command_line='info usernet')
-line = res.split('\r\n')[2]
-port = re.split(r'.*TCP.HOST_FORWARD.*127\.0\.0\.1 (\d+)\s+10\..*',
-line)[1]
+port = get_info_usernet_hostfwd_port(res)
+if not port:
+self.cancel("Failed to retrieve SSH port")
self.log.debug("sshd listening on port:" + port)
-return port
-
-def ssh_connect(self, username, password):
-self.ssh_logger = logging.getLogger('ssh')
-port = self.get_portfwd()
self.ssh_session = ssh.Session(self.VM_IP, port=int(port),
user=username, password=password)
for i in range(10):
diff --git a/tests/acceptance/virtiofs_submounts.py
b/tests/acceptance/virtiofs_submounts.py
index ca64b76301..57a7047342 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -9,6 +9,8 @@
from avocado_qemu import wait_for_console_pattern
from avocado.utils import ssh
+from qemu.utils
Re: [PATCH v7 1/8] vt82c686: Implement control of serial port io ranges via config regs
On 23/03/2021 12:54, BALATON Zoltan wrote: On Wed, 10 Mar 2021, BALATON Zoltan wrote: In VIA super south bridge the io ranges of superio components (parallel and serial ports and FDC) can be controlled by superio config registers to set their base address and enable/disable them. This is not easy to implement in QEMU because ISA emulation is only designed to set io base address once on creating the device and io ranges are registered at creation and cannot easily be disabled or moved later. In this patch we hack around that but only for serial ports because those have a single io range at port base that's relatively easy to handle and it's what guests actually use and set address different than the default. We do not attempt to handle controlling the parallel and FDC regions because those have multiple io ranges so handling them would be messy and guests either don't change their deafult or don't care. We could even get away with disabling and not emulating them, but since they are already there, this patch leaves them mapped at their default address just in case this could be useful for a guest in the future. Signed-off-by: BALATON Zoltan Could this patch be reviewed now please? I've dropped it from later versions to avoid this holding back the series but now that it won't be in 6.0 I'd like to go back to this. This is implementing the behaviour of the real hardware better than the unsettable default value we have as a replacement. That approach also works for the guests I've tried (MorphOS and Linux) but if we can do better than why not do it? Do bear in mind that several people have already looked at this patch and haven't felt comfortable enough to review it, and I've also said in a previous email that this isn't the right approach. Given that the ISA bus partly uses the ioport address to reference the device, manually moving the memory regions around for devices on the bus without the ISA bus seeing those changes is going to cause issues. IIRC the ability to dynamically change the standard ISA addresses was present in several motherboards from that era, and given that this functionality hasn't been implemented in QEMU this already tells us that no firmware is currently is using it. I don't understand why using the hard-coded addresses in the v10 is a problem here? The addresses you added in the comments representing the programmed values are the standard ISA device addresses, so if those are what the firmware is programming then there will be no change. You also reported that it works fine with both your MorphOS and Linux test images, indicating that neither of these guest OSs require the feature. If you find a guest OS that needs the functionality then certainly we can talk about trying to come up with a solution, but for me the extra complexity of this approach and the fact that you're overriding the management of the device by the ISA bus is why I haven't given it a R-B tag (I should add that my R-B for v10 using the hard-coded ISA addresses still stands). ATB, Mark.
Re: [PATCH v1 0/3] migration: Fixes to the 'background-snapshot' code
On Fri, Mar 19, 2021 at 05:52:46PM +0300, Andrey Gruzdev wrote: > Changes v0->v1: > * Using qemu_real_host_page_size instead of TARGET_PAGE_SIZE for host >page size in ram_block_populate_pages() > * More elegant implementation of ram_block_populate_pages() > > This patch series contains: > * Fix to the issue with occasionally truncated non-iterable device state > * Solution to compatibility issues with virtio-balloon device > * Fix to the issue when discarded or never populated pages miss UFFD >write protection and get into migration stream in dirty state > > Andrey Gruzdev (3): > migration: Fix missing qemu_fflush() on buffer file in > bg_migration_thread > migration: Inhibit virtio-balloon for the duration of background > snapshot > migration: Pre-fault memory before starting background snasphot Unless Andrey would like to respin a new version, this version looks good to me (I don't think the adding new helper issue in patch 1 is a blocker): Reviewed-by: Peter Xu I'm also looking into introducing UFFD_FEATURE_WP_UNALLOCATED so as to wr-protect page holes too for a uffd-wp region when the feature bit is set. With that feature we should be able to avoid pre-fault as what we do in the last patch of this series. However even if that can work out, we'll still need this for old kernel anyways. Thanks, -- Peter Xu
[PATCH v2 00/10] Acceptance Test: introduce base class for Linux based tests
This introduces a base class for tests that need to interact with a Linux guest. It generalizes the "boot_linux.py" code, already been used by the "virtiofs_submounts.py" and also SSH related code being used by that and "linux_ssh_mips_malta.py". While at it, a number of fixes on hopeful improvements to those tests were added. Changes from v1: * Majority of v1 patches have been merged. * New patches: - Acceptance Tests: make username/password configurable - Acceptance Tests: set up SSH connection by default after boot for LinuxTest - tests/acceptance/virtiofs_submounts.py: remove launch_vm() * Allowed for the configuration of the network device type (defaulting to virtio-net) [Phil] * Fix module name typo (s/qemu.util/qemu.utils/) in the commit message [John] * Tests based on LinuxTest will have the SSH connection already prepared Cleber Rosa (10): tests/acceptance/virtiofs_submounts.py: add missing accel tag tests/acceptance/virtiofs_submounts.py: evaluate string not length Python: add utility function for retrieving port redirection Acceptance Tests: move useful ssh methods to base class Acceptance Tests: add port redirection for ssh by default Acceptance Tests: make username/password configurable Acceptance Tests: set up SSH connection by default after boot for LinuxTest tests/acceptance/virtiofs_submounts.py: remove launch_vm() Acceptance Tests: add basic documentation on LinuxTest base class Acceptance Tests: introduce CPU hotplug test docs/devel/testing.rst| 25 python/qemu/utils.py | 35 tests/acceptance/avocado_qemu/__init__.py | 63 +++-- tests/acceptance/hotplug_cpu.py | 37 tests/acceptance/info_usernet.py | 29 ++ tests/acceptance/linux_ssh_mips_malta.py | 44 ++- tests/acceptance/virtiofs_submounts.py| 69 +++ tests/vm/basevm.py| 7 +-- 8 files changed, 198 insertions(+), 111 deletions(-) create mode 100644 python/qemu/utils.py create mode 100644 tests/acceptance/hotplug_cpu.py create mode 100644 tests/acceptance/info_usernet.py -- 2.25.4
[PATCH v2 10/10] Acceptance Tests: introduce CPU hotplug test
Even though there are qtest based tests for hotplugging CPUs (from
which this test took some inspiration from), this one adds checks
from a Linux guest point of view.
It should also serve as an example for tests that follow a similar
pattern and need to interact with QEMU (via qmp) and with the Linux
guest via SSH.
Signed-off-by: Cleber Rosa
Reviewed-by: Marc-André Lureau
Reviewed-by: Willian Rampazzo
---
tests/acceptance/hotplug_cpu.py | 37 +
1 file changed, 37 insertions(+)
create mode 100644 tests/acceptance/hotplug_cpu.py
diff --git a/tests/acceptance/hotplug_cpu.py b/tests/acceptance/hotplug_cpu.py
new file mode 100644
index 00..6374bf1b54
--- /dev/null
+++ b/tests/acceptance/hotplug_cpu.py
@@ -0,0 +1,37 @@
+# Functional test that hotplugs a CPU and checks it on a Linux guest
+#
+# Copyright (c) 2021 Red Hat, Inc.
+#
+# Author:
+# Cleber Rosa
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later. See the COPYING file in the top-level directory.
+
+from avocado_qemu import LinuxTest
+
+
+class HotPlugCPU(LinuxTest):
+
+def test(self):
+"""
+:avocado: tags=arch:x86_64
+:avocado: tags=machine:q35
+:avocado: tags=accel:kvm
+"""
+self.require_accelerator('kvm')
+self.vm.add_args('-accel', 'kvm')
+self.vm.add_args('-cpu', 'Haswell')
+self.vm.add_args('-smp', '1,sockets=1,cores=2,threads=1,maxcpus=2')
+self.launch_and_wait()
+
+self.ssh_command('test -e /sys/devices/system/cpu/cpu0')
+with self.assertRaises(AssertionError):
+self.ssh_command('test -e /sys/devices/system/cpu/cpu1')
+
+self.vm.command('device_add',
+driver='Haswell-x86_64-cpu',
+socket_id=0,
+core_id=1,
+thread_id=0)
+self.ssh_command('test -e /sys/devices/system/cpu/cpu1')
--
2.25.4
[PATCH v2 05/10] Acceptance Tests: add port redirection for ssh by default
For users of the LinuxTest class, let's set up the VM with the port
redirection for SSH, instead of requiring each test to set the same
arguments.
Signed-off-by: Cleber Rosa
---
tests/acceptance/avocado_qemu/__init__.py | 4 +++-
tests/acceptance/virtiofs_submounts.py| 4
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/tests/acceptance/avocado_qemu/__init__.py
b/tests/acceptance/avocado_qemu/__init__.py
index 67f75f66e5..e75b002c70 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -309,10 +309,12 @@ class LinuxTest(Test, LinuxSSHMixIn):
timeout = 900
chksum = None
-def setUp(self, ssh_pubkey=None):
+def setUp(self, ssh_pubkey=None, network_device_type='virtio-net'):
super(LinuxTest, self).setUp()
self.vm.add_args('-smp', '2')
self.vm.add_args('-m', '1024')
+self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22',
+ '-device', '%s,netdev=vnet' % network_device_type)
self.set_up_boot()
if ssh_pubkey is None:
ssh_pubkey, self.ssh_key = self.set_up_existing_ssh_keys()
diff --git a/tests/acceptance/virtiofs_submounts.py
b/tests/acceptance/virtiofs_submounts.py
index bed8ce44df..e10a935ac4 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -207,10 +207,6 @@ def setUp(self):
self.vm.add_args('-kernel', vmlinuz,
'-append', 'console=ttyS0 root=/dev/sda1')
-# Allow us to connect to SSH
-self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22',
- '-device', 'virtio-net,netdev=vnet')
-
self.require_accelerator("kvm")
self.vm.add_args('-accel', 'kvm')
--
2.25.4
Re: [PATCH 10/28] qapi: Rework name checking in preparation of stricter checking
On 3/23/21 5:40 AM, Markus Armbruster wrote:
Naming rules differ for the various kinds of names. To prepare
enforcing them, define functions to check them: check_name_upper(),
check_name_lower(), and check_name_camel(). For now, these merely
wrap around check_name_str(), but that will change shortly. Replace
the other uses of check_name_str() by appropriate uses of the
wrappers. No change in behavior just yet.
Signed-off-by: Markus Armbruster
---
scripts/qapi/expr.py | 51 +++-
1 file changed, 36 insertions(+), 15 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index e00467636c..30285fe334 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -21,11 +21,12 @@
from .error import QAPISemError
-# Names must be letters, numbers, -, and _. They must start with letter,
-# except for downstream extensions which must start with __RFQDN_.
-# Dots are only valid in the downstream extension prefix.
-valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
-'[a-zA-Z][a-zA-Z0-9_-]*$')
+# Names consist of letters, digits, -, and _, starting with a letter.
+# An experimental name is prefixed with x-. A name of a downstream
+# extension is prefixed with __RFQDN_. The latter prefix goes first.
+valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
+r'(x-)?'
+r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)
def check_name_is_str(name, info, source):
@@ -37,16 +38,38 @@ def check_name_str(name, info, source,
permit_upper=False):
# Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
# and 'q_obj_*' implicit type names.
-if not valid_name.match(name) or \
- c_name(name, False).startswith('q_'):
+match = valid_name.match(name)
+if not match or c_name(name, False).startswith('q_'):
raise QAPISemError(info, "%s has an invalid name" % source)
if not permit_upper and name.lower() != name:
raise QAPISemError(
info, "%s uses uppercase in name" % source)
+return match.group(3)
+
+
+def check_name_upper(name, info, source):
+stem = check_name_str(name, info, source, permit_upper=True)
+# TODO reject '[a-z-]' in @stem
+
Creates (presumably) temporary errors in flake8 for the dead assignment
here and below.
+
+def check_name_lower(name, info, source,
+ permit_upper=False):
+stem = check_name_str(name, info, source, permit_upper)
+# TODO reject '_' in stem
+
+
+def check_name_camel(name, info, source):
+stem = check_name_str(name, info, source, permit_upper=True)
+# TODO reject '[_-]' in stem, require CamelCase
def check_defn_name_str(name, info, meta):
-check_name_str(name, info, meta, permit_upper=True)
+if meta == 'event':
+check_name_upper(name, info, meta)
+elif meta == 'command':
+check_name_lower(name, info, meta, permit_upper=True)
+else:
+check_name_camel(name, info, meta)
if name.endswith('Kind') or name.endswith('List'):
raise QAPISemError(
info, "%s name should not end in '%s'" % (meta, name[-4:]))
@@ -163,8 +186,7 @@ def check_type(value, info, source,
key_source = "%s member '%s'" % (source, key)
if key.startswith('*'):
key = key[1:]
-check_name_str(key, info, key_source,
- permit_upper=permit_upper)
+check_name_lower(key, info, key_source, permit_upper)
if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'):
raise QAPISemError(info, "%s uses reserved name" % key_source)
check_keys(arg, info, key_source, ['type'], ['if', 'features'])
@@ -186,7 +208,7 @@ def check_features(features, info):
check_keys(f, info, source, ['name'], ['if'])
check_name_is_str(f['name'], info, source)
source = "%s '%s'" % (source, f['name'])
-check_name_str(f['name'], info, source)
+check_name_lower(f['name'], info, source)
check_if(f, info, source)
@@ -213,8 +235,7 @@ def check_enum(expr, info):
# Enum members may start with a digit
if member_name[0].isdigit():
member_name = 'd' + member_name # Hack: hide the digit
-check_name_str(member_name, info, source,
- permit_upper=permit_upper)
+check_name_lower(member_name, info, source, permit_upper)
check_if(member, info, source)
@@ -244,7 +265,7 @@ def check_union(expr, info):
for (key, value) in members.items():
source = "'data' member '%s'" % key
if discriminator is None:
-check_name_str(key, info, source)
+check_name_lower(key, info, source)
# else: name is in discriminator enum, which gets checked
check_keys(value, info, source, ['type'], ['if'])
check_if(value, info,
[PATCH v2 09/10] Acceptance Tests: add basic documentation on LinuxTest base class
Signed-off-by: Cleber Rosa
Reviewed-by: Marc-André Lureau
Reviewed-by: Willian Rampazzo
---
docs/devel/testing.rst | 25 +
1 file changed, 25 insertions(+)
diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 1da4c4e4c4..ed2a06db28 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -810,6 +810,31 @@ and hypothetical example follows:
At test "tear down", ``avocado_qemu.Test`` handles all the QEMUMachines
shutdown.
+The ``avocado_qemu.LinuxTest`` base test class
+~~
+
+The ``avocado_qemu.LinuxTest`` is further specialization of the
+``avocado_qemu.Test`` class, so it contains all the characteristics of
+the later plus some extra features.
+
+First of all, this base class is intended for tests that need to
+interact with a fully booted and operational Linux guest. The most
+basic example looks like this:
+
+.. code::
+
+ from avocado_qemu import LinuxTest
+
+
+ class SomeTest(LinuxTest):
+
+ def test(self):
+ self.launch_and_wait()
+ self.ssh_command('some_command_to_be_run_in_the_guest')
+
+Please refer to tests that use ``avocado_qemu.LinuxTest`` under
+``tests/acceptance`` for more examples.
+
QEMUMachine
~~~
--
2.25.4
[PULL 19/29] qapi: Rename pragma *-whitelist to *-exceptions
Rename pragma returns-whitelist to command-returns-exceptions, and
name-case-whitelist to member-name-case-exceptions.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-20-arm...@redhat.com>
Reviewed-by: Eric Blake
---
docs/devel/qapi-code-gen.txt | 21 ++-
qapi/pragma.json | 4 ++--
qga/qapi-schema.json | 2 +-
scripts/qapi/expr.py | 4 ++--
scripts/qapi/parser.py| 8 +++
scripts/qapi/schema.py| 2 +-
scripts/qapi/source.py| 8 +++
tests/qapi-schema/enum-member-case.json | 2 +-
.../pragma-value-not-list-of-str.err | 2 +-
.../pragma-value-not-list-of-str.json | 2 +-
tests/qapi-schema/pragma-value-not-list.err | 2 +-
tests/qapi-schema/pragma-value-not-list.json | 3 +--
tests/qapi-schema/qapi-schema-test.json | 2 +-
tests/qapi-schema/returns-bad-type.json | 2 +-
14 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 6906a06ad2..23e9823019 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -147,9 +147,10 @@ prevent incomplete include files.
=== Pragma directives ===
Syntax:
-PRAGMA = { 'pragma': { '*doc-required': BOOL,
- '*returns-whitelist': [ STRING, ... ],
- '*name-case-whitelist': [ STRING, ... ] } }
+PRAGMA = { 'pragma': {
+ '*doc-required': BOOL,
+ '*command-returns-exceptions': [ STRING, ... ],
+ '*member-name-exceptions': [ STRING, ... ] } }
The pragma directive lets you control optional generator behavior.
@@ -159,11 +160,11 @@ pragma to different values in parts of the schema doesn't
work.
Pragma 'doc-required' takes a boolean value. If true, documentation
is required. Default is false.
-Pragma 'returns-whitelist' takes a list of command names that may
+Pragma 'command-returns-exceptions' takes a list of commands that may
violate the rules on permitted return types. Default is none.
-Pragma 'name-case-whitelist' takes a list of names that may violate
-rules on use of upper- vs. lower-case letters. Default is none.
+Pragma 'member-name-exceptions' takes a list of types whose member
+names may contain uppercase letters. Default is none.
=== Enumeration types ===
@@ -490,9 +491,9 @@ are the arguments. A union type requires 'boxed': true.
Member 'returns' defines the command's return type. It defaults to an
empty struct type. It must normally be a complex type or an array of
a complex type. To return anything else, the command must be listed
-in pragma 'returns-whitelist'. If you do this, extending the command
-to return additional information will be harder. Use of
-'returns-whitelist' for new commands is strongly discouraged.
+in pragma 'commands-returns-exceptions'. If you do this, extending
+the command to return additional information will be harder. Use of
+the pragma for new commands is strongly discouraged.
A command's error responses are not specified in the QAPI schema.
Error conditions should be documented in comments.
@@ -755,7 +756,7 @@ Any name (command, event, type, member, or enum value)
beginning with
"x-" is marked experimental, and may be withdrawn or changed
incompatibly in a future release.
-Pragma 'name-case-whitelist' lets you violate the rules on use of
+Pragma 'member-name-exceptions' lets you violate the rules on use of
upper and lower case. Use for new code is strongly discouraged.
diff --git a/qapi/pragma.json b/qapi/pragma.json
index 7f158e183d..4895848c5e 100644
--- a/qapi/pragma.json
+++ b/qapi/pragma.json
@@ -4,13 +4,13 @@
# add to them!
{ 'pragma': {
# Commands allowed to return a non-dictionary:
-'returns-whitelist': [
+'command-returns-exceptions': [
'human-monitor-command',
'qom-get',
'query-tpm-models',
'query-tpm-types',
'ringbuf-read' ],
-'name-case-whitelist': [
+'member-name-exceptions': [
'ACPISlotType', # DIMM, visible through
query-acpi-ospm-status
'BlockdevVmdkSubformat',# all members, to match VMDK spec spellings
'BlockdevVmdkAdapterType', # legacyESX, to match VMDK spec spellings
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 9a82b7e952..2b08b761c2 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -20,7 +20,7 @@
# add to them!
{ 'pragma': {
# Commands allowed to return a non-dictionary:
-'returns-whitelist': [
+'command-returns-exceptions': [
'guest-file-open',
'guest-fsfreeze-freeze',
'guest-fsfreeze-freeze-list',
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 7bd15559de..85493efbac 100644
--- a/scripts/qapi/expr.py
+++
[PATCH v2 06/10] Acceptance Tests: make username/password configurable
This makes the username/password used for authentication configurable, because some guest operating systems may have restrictions on accounts to be used for logins, and it just makes it better documented. Signed-off-by: Cleber Rosa --- tests/acceptance/avocado_qemu/__init__.py | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py index e75b002c70..535f63a48d 100644 --- a/tests/acceptance/avocado_qemu/__init__.py +++ b/tests/acceptance/avocado_qemu/__init__.py @@ -308,6 +308,8 @@ class LinuxTest(Test, LinuxSSHMixIn): timeout = 900 chksum = None +username = 'root' +password = 'password' def setUp(self, ssh_pubkey=None, network_device_type='virtio-net'): super(LinuxTest, self).setUp() @@ -370,8 +372,8 @@ def prepare_cloudinit(self, ssh_pubkey=None): with open(ssh_pubkey) as pubkey: pubkey_content = pubkey.read() cloudinit.iso(cloudinit_iso, self.name, - username='root', - password='password', + username=self.username, + password=self.password, # QEMU's hard coded usermode router address phone_home_host='10.0.2.2', phone_home_port=self.phone_home_port, -- 2.25.4
[PULL 26/29] qapi: Enforce struct member naming rules
Struct members, including command arguments, event data, and union
inline base members, should use '-', not '_'. Enforce this. Fix the
fixable offenders (all in tests/), and add the remainder to pragma
member-name-exceptions.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-27-arm...@redhat.com>
Reviewed-by: Eric Blake
---
docs/devel/qapi-code-gen.txt| 3 ++-
qapi/pragma.json| 17 +
qga/qapi-schema.json| 4
scripts/qapi/expr.py| 5 +++--
tests/qapi-schema/qapi-schema-test.json | 8 ++--
tests/qapi-schema/qapi-schema-test.out | 4 ++--
tests/qapi-schema/struct-member-name-clash.err | 2 +-
tests/qapi-schema/struct-member-name-clash.json | 1 +
8 files changed, 36 insertions(+), 8 deletions(-)
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 10e9a0f829..c1cb6f987d 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -168,7 +168,8 @@ Pragma 'command-returns-exceptions' takes a list of
commands that may
violate the rules on permitted return types. Default is none.
Pragma 'member-name-exceptions' takes a list of types whose member
-names may contain uppercase letters. Default is none.
+names may contain uppercase letters, and '_' instead of '-'. Default
+is none.
=== Enumeration types ===
diff --git a/qapi/pragma.json b/qapi/pragma.json
index 339f067943..f422a1a2ac 100644
--- a/qapi/pragma.json
+++ b/qapi/pragma.json
@@ -31,10 +31,27 @@
# Externally visible types whose member names may use uppercase
'member-name-exceptions': [ # visible in:
'ACPISlotType', # query-acpi-ospm-status
+'AcpiTableOptions', # -acpitable
+'BlkdebugSetStateOptions', # blockdev-add, -blockdev
+'BlockDeviceInfo', # query-block
+'BlockDeviceStats', # query-blockstats
+'BlockDeviceTimedStats',# query-blockstats
+'BlockIOThrottle', # block_set_io_throttle
+'BlockInfo',# query-block
'BlockdevVmdkAdapterType', # blockdev-create (to match VMDK spec)
'BlockdevVmdkSubformat',# blockdev-create (to match VMDK spec)
+'ColoCompareProperties',# object_add, -object
+'FilterMirrorProperties', # object_add, -object
+'FilterRedirectorProperties', # object_add, -object
+'FilterRewriterProperties', # object_add, -object
+'InputLinuxProperties', # object_add, -object
+'NetdevTapOptions', # netdev_add, query-netdev, -netdev
+'PciBusInfo', # query-pci
+'PciDeviceInfo',# query-pci
+'PciMemoryRegion', # query-pci
'QapiErrorClass', # QMP error replies
'UuidInfo', # query-uuid
+'VncClientInfo',# query-vnc, query-vnc-servers, ...
'X86CPURegister32' # qom-get of x86 CPU properties
# feature-words, filtered-features
] } }
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 2b08b761c2..fb17eebde3 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -19,6 +19,10 @@
# Whitelists to permit QAPI rule violations; think twice before you
# add to them!
{ 'pragma': {
+# Types whose member names may use '_'
+'member-name-exceptions': [
+'GuestAgentInfo'
+],
# Commands allowed to return a non-dictionary:
'command-returns-exceptions': [
'guest-file-open',
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 9193e68763..ba9f7ad350 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -184,7 +184,7 @@ def check_type(value, info, source,
raise QAPISemError(info,
"%s should be an object or type name" % source)
-permit_upper = allow_dict in info.pragma.member_name_exceptions
+permissive = allow_dict in info.pragma.member_name_exceptions
# value is a dictionary, check that each member is okay
for (key, arg) in value.items():
@@ -192,7 +192,8 @@ def check_type(value, info, source,
if key.startswith('*'):
key = key[1:]
check_name_lower(key, info, key_source,
- permit_upper, permit_underscore=True)
+ permit_upper=permissive,
+ permit_underscore=permissive)
if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'):
raise QAPISemError(info, "%s uses reserved name" % key_source)
check_keys(arg, info, key_source, ['type'], ['if', 'features'])
diff --git a/tests/qapi-schema/qapi-schema-test.json
b/tests/qapi-schema/qapi-schema-test.json
index e635db4a35..387678acbb 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++
Re: [PATCH 09/28] qapi: Lift enum-specific code out of check_name_str()
On 3/23/21 5:40 AM, Markus Armbruster wrote:
check_name_str() masks leading digits when passed enum_member=True.
Only check_enum() does. Lift the masking into check_enum().
Signed-off-by: Markus Armbruster
---
scripts/qapi/expr.py | 23 ++-
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 507550c340..e00467636c 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -34,18 +34,11 @@ def check_name_is_str(name, info, source):
def check_name_str(name, info, source,
- enum_member=False,
permit_upper=False):
-membername = name
-
-# Enum members can start with a digit, because the generated C
-# code always prefixes it with the enum name
-if enum_member and membername[0].isdigit():
-membername = 'D' + membername
# Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
# and 'q_obj_*' implicit type names.
-if not valid_name.match(membername) or \
- c_name(membername, False).startswith('q_'):
+if not valid_name.match(name) or \
+ c_name(name, False).startswith('q_'):
raise QAPISemError(info, "%s has an invalid name" % source)
if not permit_upper and name.lower() != name:
raise QAPISemError(
@@ -213,11 +206,15 @@ def check_enum(expr, info):
for m in members]
for member in members:
source = "'data' member"
+member_name = member['name']
check_keys(member, info, source, ['name'], ['if'])
-check_name_is_str(member['name'], info, source)
-source = "%s '%s'" % (source, member['name'])
-check_name_str(member['name'], info, source,
- enum_member=True, permit_upper=permit_upper)
+check_name_is_str(member_name, info, source)
+source = "%s '%s'" % (source, member_name)
+# Enum members may start with a digit
+if member_name[0].isdigit():
+member_name = 'd' + member_name # Hack: hide the digit
Actually, can you put in one more space here? ^
+check_name_str(member_name, info, source,
+ permit_upper=permit_upper)
check_if(member, info, source)
[PATCH v2 01/10] tests/acceptance/virtiofs_submounts.py: add missing accel tag
The tag is useful to select tests that depend/use a particular feature. Signed-off-by: Cleber Rosa Reviewed-by: Wainer dos Santos Moschetta Reviewed-by: Willian Rampazzo --- tests/acceptance/virtiofs_submounts.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py index 46fa65392a..5b74ce2929 100644 --- a/tests/acceptance/virtiofs_submounts.py +++ b/tests/acceptance/virtiofs_submounts.py @@ -70,6 +70,7 @@ def test_something_that_needs_cmd1_and_cmd2(self): class VirtiofsSubmountsTest(LinuxTest): """ :avocado: tags=arch:x86_64 +:avocado: tags=accel:kvm """ def get_portfwd(self): -- 2.25.4
[PATCH v2 02/10] tests/acceptance/virtiofs_submounts.py: evaluate string not length
If the vmlinuz variable is set to anything that evaluates to True,
then the respective arguments should be set. If the variable contains
an empty string, than it will evaluate to False, and the extra
arguments will not be set.
This keeps the same logic, but improves readability a bit.
Signed-off-by: Cleber Rosa
Reviewed-by: Beraldo Leal
---
tests/acceptance/virtiofs_submounts.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/acceptance/virtiofs_submounts.py
b/tests/acceptance/virtiofs_submounts.py
index 5b74ce2929..ca64b76301 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -251,7 +251,7 @@ def setUp(self):
super(VirtiofsSubmountsTest, self).setUp(pubkey)
-if len(vmlinuz) > 0:
+if vmlinuz:
self.vm.add_args('-kernel', vmlinuz,
'-append', 'console=ttyS0 root=/dev/sda1')
--
2.25.4
[PULL 17/29] tests/qapi-schema: Rename pragma-*-crap to pragma-value-not-*
Rename pragma-doc-required-crap to pragma-not-bool,
pragma-returns-whitelist-crap to pragma-value-not-list, and
pragma-name-case-whitelist-crap to pragma-value-not-list-of-str.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-18-arm...@redhat.com>
Reviewed-by: Eric Blake
---
tests/qapi-schema/meson.build | 6 +++---
tests/qapi-schema/pragma-doc-required-crap.err | 1 -
tests/qapi-schema/pragma-name-case-whitelist-crap.err | 1 -
tests/qapi-schema/pragma-returns-whitelist-crap.err | 1 -
tests/qapi-schema/pragma-value-not-bool.err | 1 +
...ma-doc-required-crap.json => pragma-value-not-bool.json} | 2 +-
...agma-doc-required-crap.out => pragma-value-not-bool.out} | 0
tests/qapi-schema/pragma-value-not-list-of-str.err | 1 +
...hitelist-crap.json => pragma-value-not-list-of-str.json} | 2 +-
...-whitelist-crap.out => pragma-value-not-list-of-str.out} | 0
tests/qapi-schema/pragma-value-not-list.err | 1 +
...-case-whitelist-crap.json => pragma-value-not-list.json} | 2 +-
...returns-whitelist-crap.out => pragma-value-not-list.out} | 0
13 files changed, 9 insertions(+), 9 deletions(-)
delete mode 100644 tests/qapi-schema/pragma-doc-required-crap.err
delete mode 100644 tests/qapi-schema/pragma-name-case-whitelist-crap.err
delete mode 100644 tests/qapi-schema/pragma-returns-whitelist-crap.err
create mode 100644 tests/qapi-schema/pragma-value-not-bool.err
rename tests/qapi-schema/{pragma-doc-required-crap.json =>
pragma-value-not-bool.json} (55%)
rename tests/qapi-schema/{pragma-doc-required-crap.out =>
pragma-value-not-bool.out} (100%)
create mode 100644 tests/qapi-schema/pragma-value-not-list-of-str.err
rename tests/qapi-schema/{pragma-returns-whitelist-crap.json =>
pragma-value-not-list-of-str.json} (57%)
rename tests/qapi-schema/{pragma-name-case-whitelist-crap.out =>
pragma-value-not-list-of-str.out} (100%)
create mode 100644 tests/qapi-schema/pragma-value-not-list.err
rename tests/qapi-schema/{pragma-name-case-whitelist-crap.json =>
pragma-value-not-list.json} (50%)
rename tests/qapi-schema/{pragma-returns-whitelist-crap.out =>
pragma-value-not-list.out} (100%)
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index 664f9ee22d..ffc276b765 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -144,12 +144,12 @@ schemas = [
'oob-coroutine.json',
'oob-test.json',
'allow-preconfig-test.json',
- 'pragma-doc-required-crap.json',
'pragma-extra-junk.json',
- 'pragma-name-case-whitelist-crap.json',
'pragma-non-dict.json',
'pragma-unknown.json',
- 'pragma-returns-whitelist-crap.json',
+ 'pragma-value-not-bool.json',
+ 'pragma-value-not-list-of-str.json',
+ 'pragma-value-not-list.json',
'qapi-schema-test.json',
'quoted-structural-chars.json',
'redefined-command.json',
diff --git a/tests/qapi-schema/pragma-doc-required-crap.err
b/tests/qapi-schema/pragma-doc-required-crap.err
deleted file mode 100644
index 717062cb14..00
--- a/tests/qapi-schema/pragma-doc-required-crap.err
+++ /dev/null
@@ -1 +0,0 @@
-pragma-doc-required-crap.json:3: pragma 'doc-required' must be boolean
diff --git a/tests/qapi-schema/pragma-name-case-whitelist-crap.err
b/tests/qapi-schema/pragma-name-case-whitelist-crap.err
deleted file mode 100644
index fbea90d6c5..00
--- a/tests/qapi-schema/pragma-name-case-whitelist-crap.err
+++ /dev/null
@@ -1 +0,0 @@
-pragma-name-case-whitelist-crap.json:3: pragma name-case-whitelist must be a
list of strings
diff --git a/tests/qapi-schema/pragma-returns-whitelist-crap.err
b/tests/qapi-schema/pragma-returns-whitelist-crap.err
deleted file mode 100644
index 69784259df..00
--- a/tests/qapi-schema/pragma-returns-whitelist-crap.err
+++ /dev/null
@@ -1 +0,0 @@
-pragma-returns-whitelist-crap.json:3: pragma returns-whitelist must be a list
of strings
diff --git a/tests/qapi-schema/pragma-value-not-bool.err
b/tests/qapi-schema/pragma-value-not-bool.err
new file mode 100644
index 00..6247539616
--- /dev/null
+++ b/tests/qapi-schema/pragma-value-not-bool.err
@@ -0,0 +1 @@
+pragma-value-not-bool.json:3: pragma 'doc-required' must be boolean
diff --git a/tests/qapi-schema/pragma-doc-required-crap.json
b/tests/qapi-schema/pragma-value-not-bool.json
similarity index 55%
rename from tests/qapi-schema/pragma-doc-required-crap.json
rename to tests/qapi-schema/pragma-value-not-bool.json
index ed763c5ffc..feb489f15b 100644
--- a/tests/qapi-schema/pragma-doc-required-crap.json
+++ b/tests/qapi-schema/pragma-value-not-bool.json
@@ -1,3 +1,3 @@
-# 'doc-required' must be bool
+# pragma value must be bool
{ 'pragma': { 'doc-required': {} } }
diff --git a/tests/qapi-schema/pragma-doc-required-crap.out
b/tests/qapi-schema/pragma-value-not-bool.out
similarity index 100%
rename from tests/qapi-schema/pragma-doc-required-crap.out
rename to
[Bug 1915027] Re: RISC-V 64, CPUs do ilegal 0x00 write with SMP
Even with -smp 1 you will see the same errors. The problem is because there is nothing to run after OpenSBI jumps to the next stage. If you load a kernel you will not see the error messages. ** Changed in: qemu Status: New => Invalid -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1915027 Title: RISC-V 64, CPUs do ilegal 0x00 write with SMP Status in QEMU: Invalid Bug description: When QEMU is runt like this: qemu-system-riscv64 -d unimp,guest_errors -smp 8 Other harts will do a illegal write on address 0x00. This could be mostly (i think) because the initial assembly code is only loaded on the first hart and the others do a mess because there is no code to execute. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1915027/+subscriptions
[PULL 28/29] qapi: Enforce union and alternate branch naming rules
Union branch names should use '-', not '_'. Enforce this. The only
offenders are in tests/. Fix them.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-29-arm...@redhat.com>
Reviewed-by: Eric Blake
[Commit message typo fixed]
---
scripts/qapi/expr.py| 4 ++--
tests/qapi-schema/alternate-clash.err | 2 +-
tests/qapi-schema/alternate-clash.json | 6 --
tests/qapi-schema/qapi-schema-test.json | 2 +-
tests/qapi-schema/qapi-schema-test.out | 4 ++--
tests/qapi-schema/union-clash-branches.err | 2 +-
tests/qapi-schema/union-clash-branches.json | 6 --
7 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index d968609c48..540b3982b1 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -274,7 +274,7 @@ def check_union(expr, info):
for (key, value) in members.items():
source = "'data' member '%s'" % key
if discriminator is None:
-check_name_lower(key, info, source, permit_underscore=True)
+check_name_lower(key, info, source)
# else: name is in discriminator enum, which gets checked
check_keys(value, info, source, ['type'], ['if'])
check_if(value, info, source)
@@ -288,7 +288,7 @@ def check_alternate(expr, info):
raise QAPISemError(info, "'data' must not be empty")
for (key, value) in members.items():
source = "'data' member '%s'" % key
-check_name_lower(key, info, source, permit_underscore=True)
+check_name_lower(key, info, source)
check_keys(value, info, source, ['type'], ['if'])
check_if(value, info, source)
check_type(value['type'], info, source)
diff --git a/tests/qapi-schema/alternate-clash.err
b/tests/qapi-schema/alternate-clash.err
index 0fe02f2c99..caa2d42e3f 100644
--- a/tests/qapi-schema/alternate-clash.err
+++ b/tests/qapi-schema/alternate-clash.err
@@ -1,2 +1,2 @@
alternate-clash.json: In alternate 'Alt1':
-alternate-clash.json:4: branch 'a_b' collides with branch 'a-b'
+alternate-clash.json:6: name of 'data' member 'a_b' must not use uppercase or
'_'
diff --git a/tests/qapi-schema/alternate-clash.json
b/tests/qapi-schema/alternate-clash.json
index 039c4be658..87f061a74a 100644
--- a/tests/qapi-schema/alternate-clash.json
+++ b/tests/qapi-schema/alternate-clash.json
@@ -1,5 +1,7 @@
# Alternate branch name collision
-# Reject an alternate that would result in a collision in generated C
-# names (this would try to generate two union members named 'a_b').
+# Naming rules make collision impossible (even with the pragma). If
+# that wasn't the case, then we'd get a collision in generated C: two
+# union members a_b.
+{ 'pragma': { 'member-name-exceptions': [ 'Alt1' ] } }
{ 'alternate': 'Alt1',
'data': { 'a-b': 'bool', 'a_b': 'int' } }
diff --git a/tests/qapi-schema/qapi-schema-test.json
b/tests/qapi-schema/qapi-schema-test.json
index 387678acbb..84b9d41f15 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -231,7 +231,7 @@
{ 'union': 'TestIfUnion', 'data':
{ 'foo': 'TestStruct',
-'union_bar': { 'type': 'str', 'if': 'defined(TEST_IF_UNION_BAR)'} },
+'bar': { 'type': 'str', 'if': 'defined(TEST_IF_UNION_BAR)'} },
'if': 'defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)' }
{ 'command': 'test-if-union-cmd',
diff --git a/tests/qapi-schema/qapi-schema-test.out
b/tests/qapi-schema/qapi-schema-test.out
index 51efe5d7cd..e0b8a5f0b6 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -309,14 +309,14 @@ object q_obj_TestStruct-wrapper
member data: TestStruct optional=False
enum TestIfUnionKind
member foo
-member union_bar
+member bar
if ['defined(TEST_IF_UNION_BAR)']
if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)']
object TestIfUnion
member type: TestIfUnionKind optional=False
tag type
case foo: q_obj_TestStruct-wrapper
-case union_bar: q_obj_str-wrapper
+case bar: q_obj_str-wrapper
if ['defined(TEST_IF_UNION_BAR)']
if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)']
object q_obj_test-if-union-cmd-arg
diff --git a/tests/qapi-schema/union-clash-branches.err
b/tests/qapi-schema/union-clash-branches.err
index 73bbc2cabd..ef53645728 100644
--- a/tests/qapi-schema/union-clash-branches.err
+++ b/tests/qapi-schema/union-clash-branches.err
@@ -1,2 +1,2 @@
union-clash-branches.json: In union 'TestUnion':
-union-clash-branches.json:4: branch 'a_b' collides with branch 'a-b'
+union-clash-branches.json:6: name of 'data' member 'a_b' must not use
uppercase or '_'
diff --git a/tests/qapi-schema/union-clash-branches.json
b/tests/qapi-schema/union-clash-branches.json
index 3bece8c948..7bdda0b0da 100644
--- a/tests/qapi-schema/union-clash-branches.json
+++ b/tests/qapi-schema/union-clash-branches.json
@@ -1,5 +1,7 @@
# Union branch name
[PULL 10/29] qapi: Rework name checking in preparation of stricter checking
Naming rules differ for the various kinds of names. To prepare
enforcing them, define functions to check them: check_name_upper(),
check_name_lower(), and check_name_camel(). For now, these merely
wrap around check_name_str(), but that will change shortly. Replace
the other uses of check_name_str() by appropriate uses of the
wrappers. No change in behavior just yet.
check_name_str() now returns the name without downstream and x-
prefix, for use by the wrappers in later patches. Requires tweaking
regexp @valid_name. It accepts the same strings as before.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-11-arm...@redhat.com>
Reviewed-by: Eric Blake
[Commit message improved]
---
scripts/qapi/expr.py | 51 +++-
1 file changed, 36 insertions(+), 15 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index e00467636c..30285fe334 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -21,11 +21,12 @@
from .error import QAPISemError
-# Names must be letters, numbers, -, and _. They must start with letter,
-# except for downstream extensions which must start with __RFQDN_.
-# Dots are only valid in the downstream extension prefix.
-valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
-'[a-zA-Z][a-zA-Z0-9_-]*$')
+# Names consist of letters, digits, -, and _, starting with a letter.
+# An experimental name is prefixed with x-. A name of a downstream
+# extension is prefixed with __RFQDN_. The latter prefix goes first.
+valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
+r'(x-)?'
+r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)
def check_name_is_str(name, info, source):
@@ -37,16 +38,38 @@ def check_name_str(name, info, source,
permit_upper=False):
# Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
# and 'q_obj_*' implicit type names.
-if not valid_name.match(name) or \
- c_name(name, False).startswith('q_'):
+match = valid_name.match(name)
+if not match or c_name(name, False).startswith('q_'):
raise QAPISemError(info, "%s has an invalid name" % source)
if not permit_upper and name.lower() != name:
raise QAPISemError(
info, "%s uses uppercase in name" % source)
+return match.group(3)
+
+
+def check_name_upper(name, info, source):
+stem = check_name_str(name, info, source, permit_upper=True)
+# TODO reject '[a-z-]' in @stem
+
+
+def check_name_lower(name, info, source,
+ permit_upper=False):
+stem = check_name_str(name, info, source, permit_upper)
+# TODO reject '_' in stem
+
+
+def check_name_camel(name, info, source):
+stem = check_name_str(name, info, source, permit_upper=True)
+# TODO reject '[_-]' in stem, require CamelCase
def check_defn_name_str(name, info, meta):
-check_name_str(name, info, meta, permit_upper=True)
+if meta == 'event':
+check_name_upper(name, info, meta)
+elif meta == 'command':
+check_name_lower(name, info, meta, permit_upper=True)
+else:
+check_name_camel(name, info, meta)
if name.endswith('Kind') or name.endswith('List'):
raise QAPISemError(
info, "%s name should not end in '%s'" % (meta, name[-4:]))
@@ -163,8 +186,7 @@ def check_type(value, info, source,
key_source = "%s member '%s'" % (source, key)
if key.startswith('*'):
key = key[1:]
-check_name_str(key, info, key_source,
- permit_upper=permit_upper)
+check_name_lower(key, info, key_source, permit_upper)
if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'):
raise QAPISemError(info, "%s uses reserved name" % key_source)
check_keys(arg, info, key_source, ['type'], ['if', 'features'])
@@ -186,7 +208,7 @@ def check_features(features, info):
check_keys(f, info, source, ['name'], ['if'])
check_name_is_str(f['name'], info, source)
source = "%s '%s'" % (source, f['name'])
-check_name_str(f['name'], info, source)
+check_name_lower(f['name'], info, source)
check_if(f, info, source)
@@ -213,8 +235,7 @@ def check_enum(expr, info):
# Enum members may start with a digit
if member_name[0].isdigit():
member_name = 'd' + member_name # Hack: hide the digit
-check_name_str(member_name, info, source,
- permit_upper=permit_upper)
+check_name_lower(member_name, info, source, permit_upper)
check_if(member, info, source)
@@ -244,7 +265,7 @@ def check_union(expr, info):
for (key, value) in members.items():
source = "'data' member '%s'" % key
if discriminator is None:
-check_name_str(key, info, source)
+check_name_lower(key, info, source)
# else: name is in discriminator enum,
[PULL 22/29] qapi: Prepare for rejecting underscore in command and member names
Command names and member names within a type should be all lower case
with words separated by a hyphen. We also accept underscore. Rework
check_name_lower() to optionally reject underscores, but don't use
that option, yet.
Update expected test output for the changed error message.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-23-arm...@redhat.com>
Reviewed-by: Eric Blake
---
scripts/qapi/expr.py| 24 ++--
tests/qapi-schema/args-member-case.err | 2 +-
tests/qapi-schema/enum-member-case.err | 2 +-
tests/qapi-schema/union-branch-case.err | 2 +-
4 files changed, 17 insertions(+), 13 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 85493efbac..e416cc5162 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -51,12 +51,13 @@ def check_name_upper(name, info, source):
def check_name_lower(name, info, source,
- permit_upper=False):
+ permit_upper=False,
+ permit_underscore=False):
stem = check_name_str(name, info, source)
-if not permit_upper and re.search(r'[A-Z]', stem):
+if ((not permit_upper and re.search(r'[A-Z]', stem))
+or (not permit_underscore and '_' in stem)):
raise QAPISemError(
-info, "%s uses uppercase in name" % source)
-# TODO reject '_' in stem
+info, "name of %s must not use uppercase or '_'" % source)
def check_name_camel(name, info, source):
@@ -69,7 +70,8 @@ def check_defn_name_str(name, info, meta):
if meta == 'event':
check_name_upper(name, info, meta)
elif meta == 'command':
-check_name_lower(name, info, meta, permit_upper=True)
+check_name_lower(name, info, meta,
+ permit_upper=True, permit_underscore=True)
else:
check_name_camel(name, info, meta)
if name.endswith('Kind') or name.endswith('List'):
@@ -188,7 +190,8 @@ def check_type(value, info, source,
key_source = "%s member '%s'" % (source, key)
if key.startswith('*'):
key = key[1:]
-check_name_lower(key, info, key_source, permit_upper)
+check_name_lower(key, info, key_source,
+ permit_upper, permit_underscore=True)
if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'):
raise QAPISemError(info, "%s uses reserved name" % key_source)
check_keys(arg, info, key_source, ['type'], ['if', 'features'])
@@ -210,7 +213,7 @@ def check_features(features, info):
check_keys(f, info, source, ['name'], ['if'])
check_name_is_str(f['name'], info, source)
source = "%s '%s'" % (source, f['name'])
-check_name_lower(f['name'], info, source)
+check_name_lower(f['name'], info, source, permit_underscore=True)
check_if(f, info, source)
@@ -237,7 +240,8 @@ def check_enum(expr, info):
# Enum members may start with a digit
if member_name[0].isdigit():
member_name = 'd' + member_name # Hack: hide the digit
-check_name_lower(member_name, info, source, permit_upper)
+check_name_lower(member_name, info, source,
+ permit_upper, permit_underscore=True)
check_if(member, info, source)
@@ -267,7 +271,7 @@ def check_union(expr, info):
for (key, value) in members.items():
source = "'data' member '%s'" % key
if discriminator is None:
-check_name_lower(key, info, source)
+check_name_lower(key, info, source, permit_underscore=True)
# else: name is in discriminator enum, which gets checked
check_keys(value, info, source, ['type'], ['if'])
check_if(value, info, source)
@@ -281,7 +285,7 @@ def check_alternate(expr, info):
raise QAPISemError(info, "'data' must not be empty")
for (key, value) in members.items():
source = "'data' member '%s'" % key
-check_name_lower(key, info, source)
+check_name_lower(key, info, source, permit_underscore=True)
check_keys(value, info, source, ['type'], ['if'])
check_if(value, info, source)
check_type(value['type'], info, source)
diff --git a/tests/qapi-schema/args-member-case.err
b/tests/qapi-schema/args-member-case.err
index 4f33dbbc38..25c3910a17 100644
--- a/tests/qapi-schema/args-member-case.err
+++ b/tests/qapi-schema/args-member-case.err
@@ -1,2 +1,2 @@
args-member-case.json: In command 'no-way-this-will-get-whitelisted':
-args-member-case.json:2: 'data' member 'Arg' uses uppercase in name
+args-member-case.json:2: name of 'data' member 'Arg' must not use uppercase or
'_'
diff --git a/tests/qapi-schema/enum-member-case.err
b/tests/qapi-schema/enum-member-case.err
index 8b3aefe37a..c813522c50 100644
--- a/tests/qapi-schema/enum-member-case.err
+++ b/tests/qapi-schema/enum-member-case.err
@@ -1,2 +1,2 @@
enum-member-case.json: In
[PULL 11/29] qapi: Move uppercase rejection to check_name_lower()
check_name_lower() is the only user of check_name_str() using
permit_upper=False. Move the associated code from check_name_str() to
check_name_lower(), and drop the parameter.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-12-arm...@redhat.com>
Reviewed-by: Eric Blake
---
scripts/qapi/expr.py | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 30285fe334..a815060ee2 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -34,32 +34,31 @@ def check_name_is_str(name, info, source):
raise QAPISemError(info, "%s requires a string name" % source)
-def check_name_str(name, info, source,
- permit_upper=False):
+def check_name_str(name, info, source):
# Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
# and 'q_obj_*' implicit type names.
match = valid_name.match(name)
if not match or c_name(name, False).startswith('q_'):
raise QAPISemError(info, "%s has an invalid name" % source)
-if not permit_upper and name.lower() != name:
-raise QAPISemError(
-info, "%s uses uppercase in name" % source)
return match.group(3)
def check_name_upper(name, info, source):
-stem = check_name_str(name, info, source, permit_upper=True)
+stem = check_name_str(name, info, source)
# TODO reject '[a-z-]' in @stem
def check_name_lower(name, info, source,
permit_upper=False):
-stem = check_name_str(name, info, source, permit_upper)
+stem = check_name_str(name, info, source)
+if not permit_upper and name.lower() != name:
+raise QAPISemError(
+info, "%s uses uppercase in name" % source)
# TODO reject '_' in stem
def check_name_camel(name, info, source):
-stem = check_name_str(name, info, source, permit_upper=True)
+stem = check_name_str(name, info, source)
# TODO reject '[_-]' in stem, require CamelCase
--
2.26.3
[PULL 27/29] qapi: Enforce enum member naming rules
Enum members should use '-', not '_'. Enforce this. Fix the fixable
offenders (all in tests/), and add the remainder to pragma
member-name-exceptions.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-28-arm...@redhat.com>
Reviewed-by: Eric Blake
---
qapi/pragma.json | 8
scripts/qapi/expr.py | 5 +++--
tests/qapi-schema/enum-clash-member.err | 2 +-
tests/qapi-schema/enum-clash-member.json | 1 +
4 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/qapi/pragma.json b/qapi/pragma.json
index f422a1a2ac..b4e17167e1 100644
--- a/qapi/pragma.json
+++ b/qapi/pragma.json
@@ -32,12 +32,15 @@
'member-name-exceptions': [ # visible in:
'ACPISlotType', # query-acpi-ospm-status
'AcpiTableOptions', # -acpitable
+'BlkdebugEvent',# blockdev-add, -blockdev
'BlkdebugSetStateOptions', # blockdev-add, -blockdev
'BlockDeviceInfo', # query-block
'BlockDeviceStats', # query-blockstats
'BlockDeviceTimedStats',# query-blockstats
'BlockIOThrottle', # block_set_io_throttle
'BlockInfo',# query-block
+'BlockdevAioOptions', # blockdev-add, -blockdev
+'BlockdevDriver', # blockdev-add, query-blockstats, ...
'BlockdevVmdkAdapterType', # blockdev-create (to match VMDK spec)
'BlockdevVmdkSubformat',# blockdev-create (to match VMDK spec)
'ColoCompareProperties',# object_add, -object
@@ -46,10 +49,15 @@
'FilterRewriterProperties', # object_add, -object
'InputLinuxProperties', # object_add, -object
'NetdevTapOptions', # netdev_add, query-netdev, -netdev
+'ObjectType', # object-add, -object
+'PCIELinkSpeed',# internal only
'PciBusInfo', # query-pci
'PciDeviceInfo',# query-pci
'PciMemoryRegion', # query-pci
+'QKeyCode', # send-key, input-sent-event
'QapiErrorClass', # QMP error replies
+'SshHostKeyCheckMode', # blockdev-add, -blockdev
+'SysEmuTarget', # query-cpu-fast, query-target
'UuidInfo', # query-uuid
'VncClientInfo',# query-vnc, query-vnc-servers, ...
'X86CPURegister32' # qom-get of x86 CPU properties
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index ba9f7ad350..d968609c48 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -229,7 +229,7 @@ def check_enum(expr, info):
if prefix is not None and not isinstance(prefix, str):
raise QAPISemError(info, "'prefix' must be a string")
-permit_upper = name in info.pragma.member_name_exceptions
+permissive = name in info.pragma.member_name_exceptions
members[:] = [m if isinstance(m, dict) else {'name': m}
for m in members]
@@ -243,7 +243,8 @@ def check_enum(expr, info):
if member_name[0].isdigit():
member_name = 'd' + member_name # Hack: hide the digit
check_name_lower(member_name, info, source,
- permit_upper, permit_underscore=True)
+ permit_upper=permissive,
+ permit_underscore=permissive)
check_if(member, info, source)
diff --git a/tests/qapi-schema/enum-clash-member.err
b/tests/qapi-schema/enum-clash-member.err
index 5986571427..e4eb102ae2 100644
--- a/tests/qapi-schema/enum-clash-member.err
+++ b/tests/qapi-schema/enum-clash-member.err
@@ -1,2 +1,2 @@
enum-clash-member.json: In enum 'MyEnum':
-enum-clash-member.json:2: value 'one_two' collides with value 'one-two'
+enum-clash-member.json:3: value 'one_two' collides with value 'one-two'
diff --git a/tests/qapi-schema/enum-clash-member.json
b/tests/qapi-schema/enum-clash-member.json
index b6928b8bfd..82bcbf724b 100644
--- a/tests/qapi-schema/enum-clash-member.json
+++ b/tests/qapi-schema/enum-clash-member.json
@@ -1,2 +1,3 @@
# we reject enums where members will clash when mapped to C enum
+{ 'pragma': { 'member-name-exceptions': [ 'MyEnum' ] } }
{ 'enum': 'MyEnum', 'data': [ 'one-two', 'one_two' ] }
--
2.26.3
[PULL 15/29] tests/qapi-schema: Rename redefined-builtin to redefined-predefined
The previous commit changed this test to clash with a predefined enum
type, not a built-in type. Adjust its name.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-16-arm...@redhat.com>
Reviewed-by: Eric Blake
---
tests/qapi-schema/meson.build | 2 +-
tests/qapi-schema/redefined-builtin.err | 2 --
tests/qapi-schema/redefined-predefined.err | 2 ++
.../{redefined-builtin.json => redefined-predefined.json} | 0
.../{redefined-builtin.out => redefined-predefined.out} | 0
5 files changed, 3 insertions(+), 3 deletions(-)
delete mode 100644 tests/qapi-schema/redefined-builtin.err
create mode 100644 tests/qapi-schema/redefined-predefined.err
rename tests/qapi-schema/{redefined-builtin.json => redefined-predefined.json}
(100%)
rename tests/qapi-schema/{redefined-builtin.out => redefined-predefined.out}
(100%)
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index ba11cb76ac..664f9ee22d 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -152,9 +152,9 @@ schemas = [
'pragma-returns-whitelist-crap.json',
'qapi-schema-test.json',
'quoted-structural-chars.json',
- 'redefined-builtin.json',
'redefined-command.json',
'redefined-event.json',
+ 'redefined-predefined.json',
'redefined-type.json',
'reserved-command-q.json',
'reserved-enum-q.json',
diff --git a/tests/qapi-schema/redefined-builtin.err
b/tests/qapi-schema/redefined-builtin.err
deleted file mode 100644
index 92bc62dc76..00
--- a/tests/qapi-schema/redefined-builtin.err
+++ /dev/null
@@ -1,2 +0,0 @@
-redefined-builtin.json: In struct 'QType':
-redefined-builtin.json:2: enum type 'QType' is already defined
diff --git a/tests/qapi-schema/redefined-predefined.err
b/tests/qapi-schema/redefined-predefined.err
new file mode 100644
index 00..2924dde60b
--- /dev/null
+++ b/tests/qapi-schema/redefined-predefined.err
@@ -0,0 +1,2 @@
+redefined-predefined.json: In struct 'QType':
+redefined-predefined.json:2: enum type 'QType' is already defined
diff --git a/tests/qapi-schema/redefined-builtin.json
b/tests/qapi-schema/redefined-predefined.json
similarity index 100%
rename from tests/qapi-schema/redefined-builtin.json
rename to tests/qapi-schema/redefined-predefined.json
diff --git a/tests/qapi-schema/redefined-builtin.out
b/tests/qapi-schema/redefined-predefined.out
similarity index 100%
rename from tests/qapi-schema/redefined-builtin.out
rename to tests/qapi-schema/redefined-predefined.out
--
2.26.3
[PULL 20/29] qapi/pragma: Streamline comments on member-name-exceptions
Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-21-arm...@redhat.com> Reviewed-by: Eric Blake --- qapi/pragma.json | 16 +--- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/qapi/pragma.json b/qapi/pragma.json index 4895848c5e..4c47c802d1 100644 --- a/qapi/pragma.json +++ b/qapi/pragma.json @@ -10,11 +10,13 @@ 'query-tpm-models', 'query-tpm-types', 'ringbuf-read' ], -'member-name-exceptions': [ -'ACPISlotType', # DIMM, visible through query-acpi-ospm-status -'BlockdevVmdkSubformat',# all members, to match VMDK spec spellings -'BlockdevVmdkAdapterType', # legacyESX, to match VMDK spec spellings -'QapiErrorClass', # all members, visible through errors -'UuidInfo', # UUID, visible through query-uuid -'X86CPURegister32' # all members, visible indirectly through qom-get +# Externally visible types whose member names may use uppercase +'member-name-exceptions': [ # visible in: +'ACPISlotType', # query-acpi-ospm-status +'BlockdevVmdkAdapterType', # blockdev-create (to match VMDK spec) +'BlockdevVmdkSubformat',# blockdev-create (to match VMDK spec) +'QapiErrorClass', # QMP error replies +'UuidInfo', # query-uuid +'X86CPURegister32' # qom-get of x86 CPU properties +# feature-words, filtered-features ] } } -- 2.26.3
[PULL 08/29] qapi: Permit flat union members for any tag value
Flat union branch names match the tag enum's member names. Omitted
branches default to "no members for this tag value".
Branch names starting with a digit get rejected like "'data' member
'0' has an invalid name". However, omitting the branch works.
This is because flat union tag values get checked twice: as enum
member name, and as union branch name. The former accepts leading
digits, the latter doesn't.
Branches whose names start with a digit therefore cannot have members.
Feels wrong. Get rid of the restriction by skipping the latter check.
This can expose c_name() to input it can't handle: a name starting
with a digit. Improve it to return a valid C identifier for any
input.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-9-arm...@redhat.com>
Reviewed-by: Eric Blake
[Commit message rewritten]
---
scripts/qapi/common.py | 8
scripts/qapi/expr.py | 4 +++-
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 11b86beeab..cbd3fd81d3 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -18,7 +18,6 @@
#: Magic string that gets removed along with all space to its right.
EATSPACE = '\033EATSPACE.'
POINTER_SUFFIX = ' *' + EATSPACE
-_C_NAME_TRANS = str.maketrans('.-', '__')
def camel_to_upper(value: str) -> str:
@@ -109,9 +108,10 @@ def c_name(name: str, protect: bool = True) -> str:
'not_eq', 'or', 'or_eq', 'xor', 'xor_eq'])
# namespace pollution:
polluted_words = set(['unix', 'errno', 'mips', 'sparc', 'i386'])
-name = name.translate(_C_NAME_TRANS)
-if protect and (name in c89_words | c99_words | c11_words | gcc_words
-| cpp_words | polluted_words):
+name = re.sub(r'[^A-Za-z0-9_]', '_', name)
+if protect and (name in (c89_words | c99_words | c11_words | gcc_words
+ | cpp_words | polluted_words)
+or name[0].isdigit()):
return 'q_' + name
return name
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index cf09fa9fd3..507550c340 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -246,7 +246,9 @@ def check_union(expr, info):
for (key, value) in members.items():
source = "'data' member '%s'" % key
-check_name_str(key, info, source)
+if discriminator is None:
+check_name_str(key, info, source)
+# else: name is in discriminator enum, which gets checked
check_keys(value, info, source, ['type'], ['if'])
check_if(value, info, source)
check_type(value['type'], info, source, allow_array=not base)
--
2.26.3
[PULL 16/29] qapi: Factor out QAPISchemaParser._check_pragma_list_of_str()
Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-17-arm...@redhat.com> Reviewed-by: Eric Blake --- scripts/qapi/parser.py | 19 +-- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 116afe549a..8eed69333f 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -119,6 +119,13 @@ def _include(self, include, info, incl_fname, previously_included): return QAPISchemaParser(incl_fname, previously_included, info) +def _check_pragma_list_of_str(self, name, value, info): +if (not isinstance(value, list) +or any([not isinstance(elt, str) for elt in value])): +raise QAPISemError( +info, +"pragma %s must be a list of strings" % name) + def _pragma(self, name, value, info): if name == 'doc-required': if not isinstance(value, bool): @@ -126,18 +133,10 @@ def _pragma(self, name, value, info): "pragma 'doc-required' must be boolean") info.pragma.doc_required = value elif name == 'returns-whitelist': -if (not isinstance(value, list) -or any([not isinstance(elt, str) for elt in value])): -raise QAPISemError( -info, -"pragma returns-whitelist must be a list of strings") +self._check_pragma_list_of_str(name, value, info) info.pragma.returns_whitelist = value elif name == 'name-case-whitelist': -if (not isinstance(value, list) -or any([not isinstance(elt, str) for elt in value])): -raise QAPISemError( -info, -"pragma name-case-whitelist must be a list of strings") +self._check_pragma_list_of_str(name, value, info) info.pragma.name_case_whitelist = value else: raise QAPISemError(info, "unknown pragma '%s'" % name) -- 2.26.3
[PULL 29/29] block: Remove monitor command block_passwd
Command block_passwd always fails since
Commit c01c214b69 "block: remove all encryption handling APIs"
(v2.10.0) turned block_passwd into a stub that always fails, and
hardcoded encryption_key_missing to false in query-named-block-nodes
and query-block.
Commit ad1324e044 "block: remove 'encryption_key_missing' flag from
QAPI" just landed. Complete the cleanup job: remove block_passwd.
Cc: Daniel P. Berrangé
Signed-off-by: Markus Armbruster
Message-Id: <20210323101951.3686029-1-arm...@redhat.com>
Reviewed-by: Daniel P. Berrangé
---
qapi/block-core.json | 14 --
qapi/pragma.json | 1 -
block/monitor/block-hmp-cmds.c | 10 --
blockdev.c | 8
hmp-commands.hx| 15 ---
5 files changed, 48 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1c3f1deb03..6d227924d0 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1207,20 +1207,6 @@
##
{ 'command': 'query-block-jobs', 'returns': ['BlockJobInfo'] }
-##
-# @block_passwd:
-#
-# This command sets the password of a block device that has not been open
-# with a password and requires one.
-#
-# This command is now obsolete and will always return an error since 2.10
-#
-##
-{ 'command': 'block_passwd',
- 'data': { '*device': 'str',
-'*node-name': 'str',
-'password': 'str' } }
-
##
# @block_resize:
#
diff --git a/qapi/pragma.json b/qapi/pragma.json
index b4e17167e1..3bc0335d1f 100644
--- a/qapi/pragma.json
+++ b/qapi/pragma.json
@@ -6,7 +6,6 @@
# Commands allowed to return a non-dictionary:
'command-name-exceptions': [
'add_client',
-'block_passwd',
'block_resize',
'block_set_io_throttle',
'client_migrate_info',
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 75d7fa9510..ebf1033f31 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -515,16 +515,6 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, error);
}
-void hmp_block_passwd(Monitor *mon, const QDict *qdict)
-{
-const char *device = qdict_get_str(qdict, "device");
-const char *password = qdict_get_str(qdict, "password");
-Error *err = NULL;
-
-qmp_block_passwd(true, device, false, NULL, password, );
-hmp_handle_error(mon, err);
-}
-
void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
{
Error *err = NULL;
diff --git a/blockdev.c b/blockdev.c
index cf70bb4e43..621cc3b7c4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2407,14 +2407,6 @@ exit:
job_txn_unref(block_job_txn);
}
-void qmp_block_passwd(bool has_device, const char *device,
- bool has_node_name, const char *node_name,
- const char *password, Error **errp)
-{
-error_setg(errp,
- "Setting block passwords directly is no longer supported");
-}
-
BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
const char *name,
Error **errp)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9b88c45174..435c591a1c 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1493,21 +1493,6 @@ SRST
used by another monitor command.
ERST
-{
-.name = "block_passwd",
-.args_type = "device:B,password:s",
-.params = "block_passwd device password",
-.help = "set the password of encrypted block devices",
-.cmd= hmp_block_passwd,
-},
-
-SRST
-``block_passwd`` *device* *password*
- Set the encrypted device *device* password to *password*
-
- This command is now obsolete and will always return an error since 2.10
-ERST
-
{
.name = "block_set_io_throttle",
.args_type =
"device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l",
--
2.26.3
[PULL 12/29] qapi: Consistently permit any case in downstream prefixes
We require lowercase __RFQDN_ downstream prefixes only where we require the prefixed name to be lowercase. Don't; permit any case in __RFQDN_ prefixes anywhere. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-13-arm...@redhat.com> Reviewed-by: Eric Blake --- scripts/qapi/expr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index a815060ee2..b5fb0be48b 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -51,7 +51,7 @@ def check_name_upper(name, info, source): def check_name_lower(name, info, source, permit_upper=False): stem = check_name_str(name, info, source) -if not permit_upper and name.lower() != name: +if not permit_upper and re.search(r'[A-Z]', stem): raise QAPISemError( info, "%s uses uppercase in name" % source) # TODO reject '_' in stem -- 2.26.3
[PULL 21/29] tests-qmp-cmds: Drop unused and incorrect qmp_TestIfCmd()
Commit 967c885108 "qapi: add 'if' to top-level expressions" added
command TestIfCmd with an 'if' condition. It also added the
qmp_TestIfCmd() to go with it, guarded by the corresponding #if.
Commit ccadd6bcba "qapi: Add 'if' to implicit struct members" changed
the command, but not the function. Compiles only because we don't
satisfy the #if. Instead of fixing the function, simply drop it.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-22-arm...@redhat.com>
Reviewed-by: Eric Blake
---
tests/unit/test-qmp-cmds.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/tests/unit/test-qmp-cmds.c b/tests/unit/test-qmp-cmds.c
index 266db074b4..99973dde7c 100644
--- a/tests/unit/test-qmp-cmds.c
+++ b/tests/unit/test-qmp-cmds.c
@@ -13,13 +13,6 @@
static QmpCommandList qmp_commands;
-#if defined(TEST_IF_STRUCT) && defined(TEST_IF_CMD)
-UserDefThree *qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
-{
-return NULL;
-}
-#endif
-
UserDefThree *qmp_TestCmdReturnDefThree(Error **errp)
{
return NULL;
--
2.26.3
[PULL 24/29] qapi: Enforce command naming rules
Command names should be lower-case. Enforce this. Fix the fixable
offenders (all in tests/), and add the remainder to pragma
command-name-exceptions.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-25-arm...@redhat.com>
Reviewed-by: Eric Blake
---
docs/devel/qapi-code-gen.txt| 8 ++--
qapi/pragma.json| 18 +
tests/unit/test-qmp-cmds.c | 10 +-
scripts/qapi/expr.py| 5 +++--
scripts/qapi/parser.py | 3 +++
scripts/qapi/source.py | 2 ++
tests/qapi-schema/qapi-schema-test.json | 21 +++-
tests/qapi-schema/qapi-schema-test.out | 26 -
8 files changed, 62 insertions(+), 31 deletions(-)
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 23e9823019..10e9a0f829 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -149,6 +149,7 @@ prevent incomplete include files.
Syntax:
PRAGMA = { 'pragma': {
'*doc-required': BOOL,
+ '*command-name-exceptions': [ STRING, ... ],
'*command-returns-exceptions': [ STRING, ... ],
'*member-name-exceptions': [ STRING, ... ] } }
@@ -160,6 +161,9 @@ pragma to different values in parts of the schema doesn't
work.
Pragma 'doc-required' takes a boolean value. If true, documentation
is required. Default is false.
+Pragma 'command-name-exceptions' takes a list of commands whose names
+may contain '_' instead of '-'. Default is none.
+
Pragma 'command-returns-exceptions' takes a list of commands that may
violate the rules on permitted return types. Default is none.
@@ -756,8 +760,8 @@ Any name (command, event, type, member, or enum value)
beginning with
"x-" is marked experimental, and may be withdrawn or changed
incompatibly in a future release.
-Pragma 'member-name-exceptions' lets you violate the rules on use of
-upper and lower case. Use for new code is strongly discouraged.
+Pragmas 'command-name-exceptions' and 'member-name-exceptions' let you
+violate naming rules. Use for new code is strongly discouraged.
=== Downstream extensions ===
diff --git a/qapi/pragma.json b/qapi/pragma.json
index 4c47c802d1..339f067943 100644
--- a/qapi/pragma.json
+++ b/qapi/pragma.json
@@ -4,6 +4,24 @@
# add to them!
{ 'pragma': {
# Commands allowed to return a non-dictionary:
+'command-name-exceptions': [
+'add_client',
+'block_passwd',
+'block_resize',
+'block_set_io_throttle',
+'client_migrate_info',
+'device_add',
+'device_del',
+'expire_password',
+'migrate_cancel',
+'netdev_add',
+'netdev_del',
+'qmp_capabilities',
+'set_link',
+'set_password',
+'system_powerdown',
+'system_reset',
+'system_wakeup' ],
'command-returns-exceptions': [
'human-monitor-command',
'qom-get',
diff --git a/tests/unit/test-qmp-cmds.c b/tests/unit/test-qmp-cmds.c
index 99973dde7c..1b0b7d99df 100644
--- a/tests/unit/test-qmp-cmds.c
+++ b/tests/unit/test-qmp-cmds.c
@@ -13,7 +13,7 @@
static QmpCommandList qmp_commands;
-UserDefThree *qmp_TestCmdReturnDefThree(Error **errp)
+UserDefThree *qmp_test_cmd_return_def_three(Error **errp)
{
return NULL;
}
@@ -199,7 +199,7 @@ static void test_dispatch_cmd(void)
ret = qobject_to(QDict,
do_qmp_dispatch(false,
- "{ 'execute': 'user_def_cmd' }"));
+ "{ 'execute': 'user-def-cmd' }"));
assert(ret && qdict_size(ret) == 0);
qobject_unref(ret);
}
@@ -220,11 +220,11 @@ static void test_dispatch_cmd_failure(void)
{
/* missing arguments */
do_qmp_dispatch_error(false, ERROR_CLASS_GENERIC_ERROR,
- "{ 'execute': 'user_def_cmd2' }");
+ "{ 'execute': 'user-def-cmd2' }");
/* extra arguments */
do_qmp_dispatch_error(false, ERROR_CLASS_GENERIC_ERROR,
- "{ 'execute': 'user_def_cmd',"
+ "{ 'execute': 'user-def-cmd',"
" 'arguments': { 'a': 66 } }");
}
@@ -248,7 +248,7 @@ static void test_dispatch_cmd_io(void)
int64_t val;
ret = qobject_to(QDict, do_qmp_dispatch(false,
-"{ 'execute': 'user_def_cmd2', 'arguments': {"
+"{ 'execute': 'user-def-cmd2', 'arguments': {"
" 'ud1a': { 'integer': 42, 'string': 'hello' },"
" 'ud1b': { 'integer': 422, 'string': 'hello2' } } }"));
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index d778107c18..9193e68763 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -70,8 +70,9 @@ def check_defn_name_str(name, info, meta):
if meta == 'event':
check_name_upper(name, info, meta)
elif meta ==
[PULL 18/29] tests/qapi-schema: Rename returns-whitelist to returns-bad-type
This test covers returning "bad" types. Pragma returns-whitelist is
just one aspect. Naming it returns-whitelist is suboptimal. Rename
to returns-bad-type.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-19-arm...@redhat.com>
Reviewed-by: Eric Blake
---
tests/qapi-schema/meson.build | 2 +-
tests/qapi-schema/returns-bad-type.err | 2 ++
.../{returns-whitelist.json => returns-bad-type.json} | 0
.../qapi-schema/{returns-whitelist.out => returns-bad-type.out} | 0
tests/qapi-schema/returns-whitelist.err | 2 --
5 files changed, 3 insertions(+), 3 deletions(-)
create mode 100644 tests/qapi-schema/returns-bad-type.err
rename tests/qapi-schema/{returns-whitelist.json => returns-bad-type.json}
(100%)
rename tests/qapi-schema/{returns-whitelist.out => returns-bad-type.out} (100%)
delete mode 100644 tests/qapi-schema/returns-whitelist.err
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index ffc276b765..4e7635f0a8 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -166,9 +166,9 @@ schemas = [
'reserved-type-list.json',
'returns-alternate.json',
'returns-array-bad.json',
+ 'returns-bad-type.json',
'returns-dict.json',
'returns-unknown.json',
- 'returns-whitelist.json',
'string-code-point-31.json',
'string-code-point-127.json',
'struct-base-clash-deep.json',
diff --git a/tests/qapi-schema/returns-bad-type.err
b/tests/qapi-schema/returns-bad-type.err
new file mode 100644
index 00..2c270de9ad
--- /dev/null
+++ b/tests/qapi-schema/returns-bad-type.err
@@ -0,0 +1,2 @@
+returns-bad-type.json: In command 'no-way-this-will-get-whitelisted':
+returns-bad-type.json:14: command's 'returns' cannot take array type ['int']
diff --git a/tests/qapi-schema/returns-whitelist.json
b/tests/qapi-schema/returns-bad-type.json
similarity index 100%
rename from tests/qapi-schema/returns-whitelist.json
rename to tests/qapi-schema/returns-bad-type.json
diff --git a/tests/qapi-schema/returns-whitelist.out
b/tests/qapi-schema/returns-bad-type.out
similarity index 100%
rename from tests/qapi-schema/returns-whitelist.out
rename to tests/qapi-schema/returns-bad-type.out
diff --git a/tests/qapi-schema/returns-whitelist.err
b/tests/qapi-schema/returns-whitelist.err
deleted file mode 100644
index c6e46b9b86..00
--- a/tests/qapi-schema/returns-whitelist.err
+++ /dev/null
@@ -1,2 +0,0 @@
-returns-whitelist.json: In command 'no-way-this-will-get-whitelisted':
-returns-whitelist.json:14: command's 'returns' cannot take array type ['int']
--
2.26.3
[PULL 04/29] tests/qapi-schema: Belatedly update comment on alternate clash
Commit 0426d53c65 "qapi: Simplify visiting of alternate types"
eliminated the implicit alternate enum, but neglected to update a
comment about it in a test. Do that now.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-5-arm...@redhat.com>
Reviewed-by: John Snow
---
tests/qapi-schema/alternate-clash.err | 2 +-
tests/qapi-schema/alternate-clash.json | 5 +
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/tests/qapi-schema/alternate-clash.err
b/tests/qapi-schema/alternate-clash.err
index f58b977f7b..0fe02f2c99 100644
--- a/tests/qapi-schema/alternate-clash.err
+++ b/tests/qapi-schema/alternate-clash.err
@@ -1,2 +1,2 @@
alternate-clash.json: In alternate 'Alt1':
-alternate-clash.json:7: branch 'a_b' collides with branch 'a-b'
+alternate-clash.json:4: branch 'a_b' collides with branch 'a-b'
diff --git a/tests/qapi-schema/alternate-clash.json
b/tests/qapi-schema/alternate-clash.json
index 9a59b88ced..039c4be658 100644
--- a/tests/qapi-schema/alternate-clash.json
+++ b/tests/qapi-schema/alternate-clash.json
@@ -1,8 +1,5 @@
# Alternate branch name collision
# Reject an alternate that would result in a collision in generated C
-# names (this would try to generate two enum values 'ALT1_KIND_A_B').
-# TODO: In the future, if alternates are simplified to not generate
-# the implicit Alt1Kind enum, we would still have a collision with the
-# resulting C union trying to have two members named 'a_b'.
+# names (this would try to generate two union members named 'a_b').
{ 'alternate': 'Alt1',
'data': { 'a-b': 'bool', 'a_b': 'int' } }
--
2.26.3
[PULL 02/29] tests/qapi-schema: Drop redundant flat-union-inline test
flat-union-inline.json covers longhand branch definition with an
invalid type value. It's redundant: longhand branch definition is
covered by flat-union-inline-invalid-dict.json, and invalid type value
is covered by nested-struct-data.json. Drop the test.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-3-arm...@redhat.com>
Reviewed-by: John Snow
---
tests/qapi-schema/flat-union-inline.err | 2 --
tests/qapi-schema/flat-union-inline.json | 11 ---
tests/qapi-schema/flat-union-inline.out | 0
tests/qapi-schema/meson.build| 1 -
4 files changed, 14 deletions(-)
delete mode 100644 tests/qapi-schema/flat-union-inline.err
delete mode 100644 tests/qapi-schema/flat-union-inline.json
delete mode 100644 tests/qapi-schema/flat-union-inline.out
diff --git a/tests/qapi-schema/flat-union-inline.err
b/tests/qapi-schema/flat-union-inline.err
deleted file mode 100644
index 538283f5db..00
--- a/tests/qapi-schema/flat-union-inline.err
+++ /dev/null
@@ -1,2 +0,0 @@
-flat-union-inline.json: In union 'TestUnion':
-flat-union-inline.json:7: 'data' member 'value1' should be a type name
diff --git a/tests/qapi-schema/flat-union-inline.json
b/tests/qapi-schema/flat-union-inline.json
deleted file mode 100644
index a9b3ce3f0d..00
--- a/tests/qapi-schema/flat-union-inline.json
+++ /dev/null
@@ -1,11 +0,0 @@
-# we require branches to be a struct name
-# TODO: should we allow anonymous inline branch types?
-{ 'enum': 'TestEnum',
- 'data': [ 'value1', 'value2' ] }
-{ 'struct': 'Base',
- 'data': { 'enum1': 'TestEnum', 'kind': 'str' } }
-{ 'union': 'TestUnion',
- 'base': 'Base',
- 'discriminator': 'enum1',
- 'data': { 'value1': { 'type': {} },
-'value2': { 'integer': 'int' } } }
diff --git a/tests/qapi-schema/flat-union-inline.out
b/tests/qapi-schema/flat-union-inline.out
deleted file mode 100644
index e69de29bb2..00
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index 304ef939bd..d5fa035507 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -111,7 +111,6 @@ schemas = [
'flat-union-clash-member.json',
'flat-union-discriminator-bad-name.json',
'flat-union-empty.json',
- 'flat-union-inline.json',
'flat-union-inline-invalid-dict.json',
'flat-union-int-branch.json',
'flat-union-invalid-branch-key.json',
--
2.26.3
[PULL 14/29] qapi: Enforce type naming rules
Type names should be CamelCase. Enforce this. The only offenders are
in tests/. Fix them. Add test type-case to cover the new error.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-15-arm...@redhat.com>
Reviewed-by: Eric Blake
[Regexp simplified, new test made more robust]
---
scripts/qapi/expr.py | 3 ++-
tests/qapi-schema/doc-bad-union-member.json | 4 ++--
tests/qapi-schema/double-type.err | 2 +-
tests/qapi-schema/double-type.json| 2 +-
tests/qapi-schema/features-deprecated-type.err| 2 +-
tests/qapi-schema/features-deprecated-type.json | 2 +-
tests/qapi-schema/meson.build | 1 +
tests/qapi-schema/redefined-builtin.err | 4 ++--
tests/qapi-schema/redefined-builtin.json | 4 ++--
tests/qapi-schema/redefined-type.err | 6 +++---
tests/qapi-schema/redefined-type.json | 4 ++--
tests/qapi-schema/struct-data-invalid.err | 2 +-
tests/qapi-schema/struct-data-invalid.json| 2 +-
tests/qapi-schema/struct-member-invalid-dict.err | 2 +-
tests/qapi-schema/struct-member-invalid-dict.json | 2 +-
tests/qapi-schema/struct-member-invalid.err | 2 +-
tests/qapi-schema/struct-member-invalid.json | 2 +-
tests/qapi-schema/type-case.err | 2 ++
tests/qapi-schema/type-case.json | 2 ++
tests/qapi-schema/type-case.out | 0
tests/qapi-schema/unknown-expr-key.err| 2 +-
tests/qapi-schema/unknown-expr-key.json | 2 +-
22 files changed, 30 insertions(+), 24 deletions(-)
create mode 100644 tests/qapi-schema/type-case.err
create mode 100644 tests/qapi-schema/type-case.json
create mode 100644 tests/qapi-schema/type-case.out
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index c065505b27..7bd15559de 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -61,7 +61,8 @@ def check_name_lower(name, info, source,
def check_name_camel(name, info, source):
stem = check_name_str(name, info, source)
-# TODO reject '[_-]' in stem, require CamelCase
+if not re.match(r'[A-Z][A-Za-z0-9]*[a-z][A-Za-z0-9]*$', stem):
+raise QAPISemError(info, "name of %s must use CamelCase" % source)
def check_defn_name_str(name, info, meta):
diff --git a/tests/qapi-schema/doc-bad-union-member.json
b/tests/qapi-schema/doc-bad-union-member.json
index d611435f6a..bd231a0109 100644
--- a/tests/qapi-schema/doc-bad-union-member.json
+++ b/tests/qapi-schema/doc-bad-union-member.json
@@ -11,9 +11,9 @@
'data': { 'nothing': 'Empty' } }
{ 'struct': 'Base',
- 'data': { 'type': 'T' } }
+ 'data': { 'type': 'FrobType' } }
{ 'struct': 'Empty',
'data': { } }
-{ 'enum': 'T', 'data': ['nothing'] }
+{ 'enum': 'FrobType', 'data': ['nothing'] }
diff --git a/tests/qapi-schema/double-type.err
b/tests/qapi-schema/double-type.err
index 71fc4dbb52..576e716197 100644
--- a/tests/qapi-schema/double-type.err
+++ b/tests/qapi-schema/double-type.err
@@ -1,3 +1,3 @@
-double-type.json: In struct 'bar':
+double-type.json: In struct 'Bar':
double-type.json:2: struct has unknown key 'command'
Valid keys are 'base', 'data', 'features', 'if', 'struct'.
diff --git a/tests/qapi-schema/double-type.json
b/tests/qapi-schema/double-type.json
index 911fa7af50..2c0809f38d 100644
--- a/tests/qapi-schema/double-type.json
+++ b/tests/qapi-schema/double-type.json
@@ -1,2 +1,2 @@
# we reject an expression with ambiguous metatype
-{ 'command': 'foo', 'struct': 'bar', 'data': { } }
+{ 'command': 'foo', 'struct': 'Bar', 'data': { } }
diff --git a/tests/qapi-schema/features-deprecated-type.err
b/tests/qapi-schema/features-deprecated-type.err
index af4ffe20aa..ddaedf604e 100644
--- a/tests/qapi-schema/features-deprecated-type.err
+++ b/tests/qapi-schema/features-deprecated-type.err
@@ -1,2 +1,2 @@
-features-deprecated-type.json: In struct 'S':
+features-deprecated-type.json: In struct 'Foo':
features-deprecated-type.json:2: feature 'deprecated' is not supported for
types
diff --git a/tests/qapi-schema/features-deprecated-type.json
b/tests/qapi-schema/features-deprecated-type.json
index 4b5bf5b86e..265849b1f7 100644
--- a/tests/qapi-schema/features-deprecated-type.json
+++ b/tests/qapi-schema/features-deprecated-type.json
@@ -1,3 +1,3 @@
# Feature 'deprecated' is not supported for types
-{ 'struct': 'S', 'data': {},
+{ 'struct': 'Foo', 'data': {},
'features': [ 'deprecated' ] }
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index d5fa035507..ba11cb76ac 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -180,6 +180,7 @@ schemas = [
'trailing-comma-list.json',
'trailing-comma-object.json',
'type-bypass-bad-gen.json',
+ 'type-case.json',
'unclosed-list.json',
'unclosed-object.json',
'unclosed-string.json',
diff --git a/tests/qapi-schema/redefined-builtin.err
[PULL 23/29] qapi: Enforce feature naming rules
Feature names should use '-', not '_'. Enforce this. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-24-arm...@redhat.com> Reviewed-by: Eric Blake --- scripts/qapi/expr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index e416cc5162..d778107c18 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -213,7 +213,7 @@ def check_features(features, info): check_keys(f, info, source, ['name'], ['if']) check_name_is_str(f['name'], info, source) source = "%s '%s'" % (source, f['name']) -check_name_lower(f['name'], info, source, permit_underscore=True) +check_name_lower(f['name'], info, source) check_if(f, info, source) -- 2.26.3
[PULL 25/29] tests/qapi-schema: Switch member name clash test to struct
Test args-name-clash covers command parameter name clash. This
effectively covers struct member name clash as well. The next commit
will make parameter name clash impossible. Convert args-name-clash
from testing command to testing a struct, and rename it to
struct-member-name-clash.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-26-arm...@redhat.com>
Reviewed-by: Eric Blake
[Commit message typo fixed]
---
tests/qapi-schema/args-name-clash.err | 2 --
tests/qapi-schema/meson.build | 2 +-
tests/qapi-schema/struct-member-name-clash.err | 2 ++
.../{args-name-clash.json => struct-member-name-clash.json} | 2 +-
.../{args-name-clash.out => struct-member-name-clash.out} | 0
5 files changed, 4 insertions(+), 4 deletions(-)
delete mode 100644 tests/qapi-schema/args-name-clash.err
create mode 100644 tests/qapi-schema/struct-member-name-clash.err
rename tests/qapi-schema/{args-name-clash.json =>
struct-member-name-clash.json} (64%)
rename tests/qapi-schema/{args-name-clash.out => struct-member-name-clash.out}
(100%)
diff --git a/tests/qapi-schema/args-name-clash.err
b/tests/qapi-schema/args-name-clash.err
deleted file mode 100644
index 3e04817bc0..00
--- a/tests/qapi-schema/args-name-clash.err
+++ /dev/null
@@ -1,2 +0,0 @@
-args-name-clash.json: In command 'oops':
-args-name-clash.json:4: parameter 'a_b' collides with parameter 'a-b'
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index 4e7635f0a8..8ba6917132 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -30,7 +30,6 @@ schemas = [
'args-member-array-bad.json',
'args-member-case.json',
'args-member-unknown.json',
- 'args-name-clash.json',
'args-union.json',
'args-unknown.json',
'bad-base.json',
@@ -177,6 +176,7 @@ schemas = [
'struct-member-if-invalid.json',
'struct-member-invalid-dict.json',
'struct-member-invalid.json',
+ 'struct-member-name-clash.json',
'trailing-comma-list.json',
'trailing-comma-object.json',
'type-bypass-bad-gen.json',
diff --git a/tests/qapi-schema/struct-member-name-clash.err
b/tests/qapi-schema/struct-member-name-clash.err
new file mode 100644
index 00..6ac042d59d
--- /dev/null
+++ b/tests/qapi-schema/struct-member-name-clash.err
@@ -0,0 +1,2 @@
+struct-member-name-clash.json: In struct 'Oops':
+struct-member-name-clash.json:4: member 'a_b' collides with member 'a-b'
diff --git a/tests/qapi-schema/args-name-clash.json
b/tests/qapi-schema/struct-member-name-clash.json
similarity index 64%
rename from tests/qapi-schema/args-name-clash.json
rename to tests/qapi-schema/struct-member-name-clash.json
index 61423cb893..3fb69cc2ce 100644
--- a/tests/qapi-schema/args-name-clash.json
+++ b/tests/qapi-schema/struct-member-name-clash.json
@@ -1,4 +1,4 @@
# C member name collision
# Reject members that clash when mapped to C names (we would have two 'a_b'
# members).
-{ 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }
+{ 'struct': 'Oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }
diff --git a/tests/qapi-schema/args-name-clash.out
b/tests/qapi-schema/struct-member-name-clash.out
similarity index 100%
rename from tests/qapi-schema/args-name-clash.out
rename to tests/qapi-schema/struct-member-name-clash.out
--
2.26.3
[PULL 03/29] tests/qapi-schema: Rework comments on longhand member definitions
A few old comments talk about "desired future use of defaults" and
"anonymous inline branch types". Kind of misleading since commit
87adbbffd4 "qapi: add a dictionary form for TYPE" added longhand
member definitions. Talk about that instead.
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-4-arm...@redhat.com>
Reviewed-by: Eric Blake
Reviewed-by: John Snow
---
tests/qapi-schema/event-member-invalid-dict.err| 2 +-
tests/qapi-schema/event-member-invalid-dict.json | 2 ++
tests/qapi-schema/flat-union-inline-invalid-dict.json | 4 ++--
tests/qapi-schema/nested-struct-data-invalid-dict.err | 2 +-
tests/qapi-schema/nested-struct-data-invalid-dict.json | 3 ++-
tests/qapi-schema/nested-struct-data.json | 2 +-
tests/qapi-schema/struct-member-invalid-dict.err | 2 +-
tests/qapi-schema/struct-member-invalid-dict.json | 3 ++-
8 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/tests/qapi-schema/event-member-invalid-dict.err
b/tests/qapi-schema/event-member-invalid-dict.err
index c7a6a24305..82f8989344 100644
--- a/tests/qapi-schema/event-member-invalid-dict.err
+++ b/tests/qapi-schema/event-member-invalid-dict.err
@@ -1,2 +1,2 @@
event-member-invalid-dict.json: In event 'EVENT_A':
-event-member-invalid-dict.json:1: 'data' member 'a' misses key 'type'
+event-member-invalid-dict.json:3: 'data' member 'a' misses key 'type'
diff --git a/tests/qapi-schema/event-member-invalid-dict.json
b/tests/qapi-schema/event-member-invalid-dict.json
index ee6f3ecb6f..e58560abca 100644
--- a/tests/qapi-schema/event-member-invalid-dict.json
+++ b/tests/qapi-schema/event-member-invalid-dict.json
@@ -1,2 +1,4 @@
+# event 'data' member with dict value is (longhand) argument
+# definition, not inline complex type
{ 'event': 'EVENT_A',
'data': { 'a' : { 'string' : 'str', 'integer': 'int' }, 'b' : 'str' } }
diff --git a/tests/qapi-schema/flat-union-inline-invalid-dict.json
b/tests/qapi-schema/flat-union-inline-invalid-dict.json
index 62c7cda617..1779712795 100644
--- a/tests/qapi-schema/flat-union-inline-invalid-dict.json
+++ b/tests/qapi-schema/flat-union-inline-invalid-dict.json
@@ -1,5 +1,5 @@
-# we require branches to be a struct name
-# TODO: should we allow anonymous inline branch types?
+# union 'data' member with dict value is (longhand) branch
+# definition, not inline complex type
{ 'enum': 'TestEnum',
'data': [ 'value1', 'value2' ] }
{ 'struct': 'Base',
diff --git a/tests/qapi-schema/nested-struct-data-invalid-dict.err
b/tests/qapi-schema/nested-struct-data-invalid-dict.err
index c044b2b17a..375e155fe6 100644
--- a/tests/qapi-schema/nested-struct-data-invalid-dict.err
+++ b/tests/qapi-schema/nested-struct-data-invalid-dict.err
@@ -1,2 +1,2 @@
nested-struct-data-invalid-dict.json: In command 'foo':
-nested-struct-data-invalid-dict.json:2: 'data' member 'a' misses key 'type'
+nested-struct-data-invalid-dict.json:3: 'data' member 'a' misses key 'type'
diff --git a/tests/qapi-schema/nested-struct-data-invalid-dict.json
b/tests/qapi-schema/nested-struct-data-invalid-dict.json
index efbe773ded..aa37b85e19 100644
--- a/tests/qapi-schema/nested-struct-data-invalid-dict.json
+++ b/tests/qapi-schema/nested-struct-data-invalid-dict.json
@@ -1,3 +1,4 @@
-# inline subtypes collide with our desired future use of defaults
+# command 'data' member with dict value is (longhand) argument
+# definition, not inline complex type
{ 'command': 'foo',
'data': { 'a' : { 'string' : 'str', 'integer': 'int' }, 'b' : 'str' } }
diff --git a/tests/qapi-schema/nested-struct-data.json
b/tests/qapi-schema/nested-struct-data.json
index 5b8a40cca3..2980d45d05 100644
--- a/tests/qapi-schema/nested-struct-data.json
+++ b/tests/qapi-schema/nested-struct-data.json
@@ -1,3 +1,3 @@
-# inline subtypes collide with our desired future use of defaults
+# {} is not a valid type reference
{ 'command': 'foo',
'data': { 'a' : { 'type': {} }, 'b' : 'str' } }
diff --git a/tests/qapi-schema/struct-member-invalid-dict.err
b/tests/qapi-schema/struct-member-invalid-dict.err
index 0621aecfbd..f9b3f33551 100644
--- a/tests/qapi-schema/struct-member-invalid-dict.err
+++ b/tests/qapi-schema/struct-member-invalid-dict.err
@@ -1,2 +1,2 @@
struct-member-invalid-dict.json: In struct 'foo':
-struct-member-invalid-dict.json:2: 'data' member '*a' misses key 'type'
+struct-member-invalid-dict.json:3: 'data' member '*a' misses key 'type'
diff --git a/tests/qapi-schema/struct-member-invalid-dict.json
b/tests/qapi-schema/struct-member-invalid-dict.json
index 9fe0d455a9..bc3d62ae63 100644
--- a/tests/qapi-schema/struct-member-invalid-dict.json
+++ b/tests/qapi-schema/struct-member-invalid-dict.json
@@ -1,3 +1,4 @@
-# Long form of member must have a value member 'type'
+# struct 'data' member with dict value is (longhand) member
+# definition, not inline complex type
{ 'struct': 'foo',
'data': { '*a': { 'case': 'foo' } } }
--
2.26.3
[PULL 09/29] qapi: Lift enum-specific code out of check_name_str()
check_name_str() masks leading digits when passed enum_member=True.
Only check_enum() does. Lift the masking into check_enum().
Signed-off-by: Markus Armbruster
Message-Id: <20210323094025.3569441-10-arm...@redhat.com>
Reviewed-by: Eric Blake
---
scripts/qapi/expr.py | 23 ++-
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 507550c340..e00467636c 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -34,18 +34,11 @@ def check_name_is_str(name, info, source):
def check_name_str(name, info, source,
- enum_member=False,
permit_upper=False):
-membername = name
-
-# Enum members can start with a digit, because the generated C
-# code always prefixes it with the enum name
-if enum_member and membername[0].isdigit():
-membername = 'D' + membername
# Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
# and 'q_obj_*' implicit type names.
-if not valid_name.match(membername) or \
- c_name(membername, False).startswith('q_'):
+if not valid_name.match(name) or \
+ c_name(name, False).startswith('q_'):
raise QAPISemError(info, "%s has an invalid name" % source)
if not permit_upper and name.lower() != name:
raise QAPISemError(
@@ -213,11 +206,15 @@ def check_enum(expr, info):
for m in members]
for member in members:
source = "'data' member"
+member_name = member['name']
check_keys(member, info, source, ['name'], ['if'])
-check_name_is_str(member['name'], info, source)
-source = "%s '%s'" % (source, member['name'])
-check_name_str(member['name'], info, source,
- enum_member=True, permit_upper=permit_upper)
+check_name_is_str(member_name, info, source)
+source = "%s '%s'" % (source, member_name)
+# Enum members may start with a digit
+if member_name[0].isdigit():
+member_name = 'd' + member_name # Hack: hide the digit
+check_name_str(member_name, info, source,
+ permit_upper=permit_upper)
check_if(member, info, source)
--
2.26.3
