Re: [Qemu-devel] [libvirt] Libvirt support of mac-programming over macvtap planned?
On 11/04/2013 08:34 AM, Amos Kong wrote: (I'm curious where your original query was sent - I didn't see it in my libvir-list nor my qemu-devel folders...) currently, all the RedHat mailing lists seem to gobble up emails from the linux.vnet.ibm.com domain, I have tried to report that via the mailman interface with no luck so far -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [Qemu-devel] [PATCH 3/3] S390: Enable -cpu help and QMP query-cpu-definitions
On 12/12/2012 02:51 PM, Andreas Färber wrote: +(*cpu_fprintf)(f, s390 %16s\n, [host]); Note that the square-bracket notation was specific to x86 when it distinguished between built-in and config-based models. OK, since libvirt capable of dealing with s390 cpu models will never see this, we can change it any way that is wanted. So, host without brackets? host only makes sense for KVM, not for TCG. So we would need one other placeholder model for libvirt. see my reply to Alex' comment: the placeholder name must be chosen carefully, i.e. future-proof -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [Qemu-devel] [PATCH 3/3] S390: Enable -cpu help and QMP query-cpu-definitions
On 12/12/2012 03:03 PM, Alexander Graf wrote: The TCG model is a z900. We can just have a z900 model and a host model with tcg_enabled() vs kvm_enabled() checks on each. Alex I would advise to not define 'artificial' names, i.e. such that cannot be retrieved via a user-space interface on the host in order to make it easier for systems management tools to figure out the right CPU model in the future. Further, if 'z900' was the host CPU model, then it should also be accepted in kvm mode in order to behave as on x86. This can be deferred to a future point in time. Having said this, I have no idea whether 'z900' would be a correct choice for a CPU model or not. -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [Qemu-devel] [PATCH 0/5] s390x: network boot
On 20.02.2017 17:01, Alexander Graf wrote: > > > On 20/02/2017 07:43, Thomas Huth wrote: >> On 20.02.2017 15:19, Cornelia Huck wrote: >>> This patchset implements network boot for s390x. You need to build >>> a s390-netboot.img to actually perform the work (not included). >> >> Do you plan to include this into the upstream QEMU repository later, or >> will this stay an external binary? >> >>> Basically, we add support for virtio-net devices into the ipl code >>> and update the ccw bios to handle this as well. >> >> Just out of curiosity: What's the reason for shipping an additional >> binary here? Couldn't it be linked into the ccw bios as well (so that >> it's easier for the user to select a different version with the -bios >> parameter)? > > The "other binary" (s390-netboot.img) is the equivalent of grub or > pxelinux in an x86 network boot environment. That one usually comes from > a different entity and different department :). I'd rather compare the other binary to an option ROM. At this point in time we there's no netboot ROM, but a set of instructions on how to build a netboot binary from a kernel and a ramdisk, the series coverletter contains a brief version of that. > > I haven't looked at the patches in detail, but do they follow the > "normal" PXE boot flow? Do they do a DHCP request, send a proper > Vendor-Class-Identifier for s390x, fetch the binary described by the > "filename" property in the DHCP ack via TFTP from next-server and run > that one then? It's currently not described with that level of detail, but the idea is to skip the step of loading a syslinux equivalent, and to immediately fetch the PXELINUX config file from the TFT next server aka siaddr and load the kernel and ramdisk specified therein. I.e. a simulation of the PXELINUX process is done in a similar fashion to petitboot PXE boot. > > It would be very good to stick to that flow, so that you don't confuse > your network admins :). > > > Alex > -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [Qemu-devel] [PATCH v5 00/11] Implement network booting in the s390-ccw BIOS
On 12.07.2017 14:49, Thomas Huth wrote: > It's already possible to do a network boot of an s390x guest with an > external netboot image based on a Linux installation, but it would > be much more convenient if the s390-ccw firmware supported network > booting right out of the box, without the need to assemble such an > external image first. > > This patch series now introduces a s390-netboot.img that can be used > for network booting via DHCP and TFTP by re-using the networking stack > from the SLOF firmware (see https://github.com/aik/SLOF/ for details), > and adds a driver for virtio-net-ccw devices. > > The code can only be built if the roms/SLOF submodule has been checked > out (there is a sanity check for this in the Makefile). Once it has > been built, you can download a combined kernel + initrd image via TFTP > by starting QEMU for example with: > > qemu-system-s390x ... -device virtio-net,netdev=n1,bootindex=1 \ >-netdev user,id=n1,tftp=/path/to/tftp,bootfile=kernel.img > > Note that this version does not support downloading via config > files (i.e. pxelinux config files or .INS config files) yet. This > will be added later. > > v5: > - Fixed problem when compiling the code natively on a s390x machine >(do not try to re-use the Makefile.inc files from SLOF, and define >our own rules instead) > - Fixed typos that Cornelia discovered while reviewing v4 > > v4: > - Cosmetic clean-ups according to the review feedback from v3 > - Fixed bug in the find_dev() function (spotted by Cornelia in v3) > - Added an additional patch to remove some unused structs from >virtio.h > > v3: > - Adressed the review feedback from v2 > - The last remaining SLOF patch has now been merged (big thanks to >Alexey!), so not sending this as RFC anymore - it is ready now for >integration, I think. > > v2: > - Put the network boot loader into a separate s390-netboot.img >binary instead of linking it directly into the s390-ccw firmware. > - Use the SLOF sources from the roms/SLOF/ submodule instead of >copying them into the pc-bios/s390-ccw folder > - Removed the .INS config file loading code for now - only support >combined kernel + initrd images in this initial implementation. > > Thomas Huth (11): > pc-bios/s390-ccw: Move libc functions to separate header > pc-bios/s390-ccw: Move ebc2asc to sclp.c > pc-bios/s390-ccw: Move virtio-block related functions into a separate > file > pc-bios/s390-ccw: Add a write() function for stdio > pc-bios/s390-ccw: Move byteswap functions to a separate header > pc-bios/s390-ccw: Remove unused structs from virtio.h > pc-bios/s390-ccw: Add code for virtio feature negotiation > roms/SLOF: Update submodule to latest status > pc-bios/s390-ccw: Add core files for the network bootloading program > pc-bios/s390-ccw: Add virtio-net driver code > pc-bios/s390-ccw: Link libnet into the netboot image and do the TFTP > load > > pc-bios/s390-ccw/Makefile| 13 +- > pc-bios/s390-ccw/bootmap.c | 2 + > pc-bios/s390-ccw/bootmap.h | 26 --- > pc-bios/s390-ccw/bswap.h | 30 > pc-bios/s390-ccw/libc.h | 45 + > pc-bios/s390-ccw/main.c | 14 +- > pc-bios/s390-ccw/netboot.mak | 59 +++ > pc-bios/s390-ccw/netmain.c | 361 > +++ > pc-bios/s390-ccw/s390-ccw.h | 33 +--- > pc-bios/s390-ccw/sclp.c | 37 ++-- > pc-bios/s390-ccw/virtio-blkdev.c | 296 > pc-bios/s390-ccw/virtio-net.c| 135 +++ > pc-bios/s390-ccw/virtio-scsi.c | 1 + > pc-bios/s390-ccw/virtio.c| 306 - > pc-bios/s390-ccw/virtio.h| 46 ++--- > roms/SLOF| 2 +- > 16 files changed, 1018 insertions(+), 388 deletions(-) > create mode 100644 pc-bios/s390-ccw/bswap.h > create mode 100644 pc-bios/s390-ccw/libc.h > create mode 100644 pc-bios/s390-ccw/netboot.mak > create mode 100644 pc-bios/s390-ccw/netmain.c > create mode 100644 pc-bios/s390-ccw/virtio-blkdev.c > create mode 100644 pc-bios/s390-ccw/virtio-net.c > With the patches applied I could successfully perform a network boot using a libvirt-defined dnsmasq TFTP server (even emulating a PXELINUX setup with a special crafted boot image :-). So, FWIW: (Series) Tested-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com> -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [Qemu-devel] [PATCH v4 00/11] Implement network booting in the s390-ccw BIOS
ed the slof thing but building fails. Not >> sure yet why. >> >> In file included from >> /home/cborntra/REPOS/qemu/roms/SLOF/lib/libc/ctype/isdigit.c:13:0: >> /home/cborntra/REPOS/qemu/roms/SLOF/lib/libc/ctype/isdigit.c:15:17: error: >> expected ‘)’ before ‘ch’ >> int isdigit(int ch) > > What a bummer! Sorry, I've only cross-compiled the s390-ccw BIOS so far, > and that worked fine (since s390x machines are a little bit hard to get > here), but seems like there is still something ugly going on when you > build the code natively ... likely somethings wrong in the Makefile ... > I'll try to figure it out ... > > Thomas > I haven't tried recently, but you could consider using one of these here https://developer.ibm.com/linuxone/ -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
[Qemu-devel] [PATCH] s390: return unavailable features via query-cpu-definitions
The response for query-cpu-definitions didn't include the
unavailable-features field, which is used by libvirt to figure
out whether a certain cpu model is usable on the host.
The unavailable features are now computed by obtaining the host CPU
model and comparing its feature bitmap with the feature bitmaps of
the known CPU models.
I.e. the output of virsh domcapabilities would change from
...
z10EC-base
z9EC-base
z196.2-base
z900-base
z990
...
to
...
z10EC-base
z9EC-base
z196.2-base
z900-base
z990
...
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
target/s390x/cpu_models.c | 51 ++-
1 file changed, 46 insertions(+), 5 deletions(-)
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 63903c2..dc3371f 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -283,14 +283,43 @@ void s390_cpu_list(FILE *f, fprintf_function print)
}
}
+static S390CPUModel *get_max_cpu_model(Error **errp);
+
#ifndef CONFIG_USER_ONLY
+static void list_add_feat(const char *name, void *opaque);
+
+static void check_unavailable_features(const S390CPUModel *max_model,
+ const S390CPUModel *model,
+ strList **unavailable)
+{
+S390FeatBitmap missing;
+
+/* detect missing features if any to properly report them */
+bitmap_andnot(missing, model->features, max_model->features,
+ S390_FEAT_MAX);
+if (!bitmap_empty(missing, S390_FEAT_MAX)) {
+s390_feat_bitmap_to_ascii(missing,
+ unavailable,
+ list_add_feat);
+}
+}
+
+struct CpuDefinitionInfoListData {
+CpuDefinitionInfoList *list;
+Error **errp;
+};
+
static void create_cpu_model_list(ObjectClass *klass, void *opaque)
{
-CpuDefinitionInfoList **cpu_list = opaque;
+struct CpuDefinitionInfoListData *cpu_list_data = opaque;
+CpuDefinitionInfoList **cpu_list = _list_data->list;
CpuDefinitionInfoList *entry;
CpuDefinitionInfo *info;
char *name = g_strdup(object_class_get_name(klass));
S390CPUClass *scc = S390_CPU_CLASS(klass);
+Object *obj;
+S390CPU *sc;
+S390CPUModel *scm;
/* strip off the -s390-cpu */
g_strrstr(name, "-" TYPE_S390_CPU)[0] = 0;
@@ -300,21 +329,33 @@ static void create_cpu_model_list(ObjectClass *klass,
void *opaque)
info->migration_safe = scc->is_migration_safe;
info->q_static = scc->is_static;
info->q_typename = g_strdup(object_class_get_name(klass));
-
+/* check for unavailable features */
+obj = object_new(object_class_get_name(klass));
+sc = S390_CPU(obj);
+scm = get_max_cpu_model(cpu_list_data->errp);
+if (scm && sc->model) {
+info->has_unavailable_features = true;
+check_unavailable_features(scm, sc->model,
>unavailable_features);
+}
entry = g_malloc0(sizeof(*entry));
entry->value = info;
entry->next = *cpu_list;
*cpu_list = entry;
+object_unref(obj);
}
CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
{
-CpuDefinitionInfoList *list = NULL;
+struct CpuDefinitionInfoListData list_data = {
+.list = NULL,
+.errp = errp,
+};
-object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false, );
+object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false,
+ _data);
-return list;
+return list_data.list;
}
static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
--
1.9.1
Re: [Qemu-devel] [PATCH v3] s390: return unavailable features via query-cpu-definitions
On 04.07.2017 11:35, David Hildenbrand wrote:
>> +
>> static void create_cpu_model_list(ObjectClass *klass, void *opaque)
>> {
>> -CpuDefinitionInfoList **cpu_list = opaque;
>> +struct CpuDefinitionInfoListData *cpu_list_data = opaque;
>> +CpuDefinitionInfoList **cpu_list = _list_data->list;
>> CpuDefinitionInfoList *entry;
>> CpuDefinitionInfo *info;
>> char *name = g_strdup(object_class_get_name(klass));
>> @@ -300,7 +331,19 @@ static void create_cpu_model_list(ObjectClass *klass,
>> void *opaque)
>> info->migration_safe = scc->is_migration_safe;
>> info->q_static = scc->is_static;
>> info->q_typename = g_strdup(object_class_get_name(klass));
>> -
>
> Wonder if we should add the following:
>
> /* the host model can never be used under TCG */
> if (scc->kvm_required && !kvm_enabled())) {
> info->has_unavailable_features = true;
> list_add_feat("type", >unavailable_features);
> } else if (cpu_list_data->model ...
>
>> +/* check for unavailable features */
>> +if (cpu_list_data->model) {
>> +Object *obj;
>> +S390CPU *sc;
>> +obj = object_new(object_class_get_name(klass));
>> +sc = S390_CPU(obj);
>
> So we can drop the check here (if we have a host/max_model definition
> under KVM, then all models have sc->model set). But the current code
> should also work (it simply will not indicate runability information for
> the "host" model under TCG, as sc->model will be NULL).
Hmm ... that made me think that we could remove the check here and make
check_unavailable_features interpret a NULL model as not
usable/incompatible (a bit more robust in the unlikely event of a memory
allocation failure). Thoughts?
> >> +if (sc->model) {
>> +info->has_unavailable_features = true;
>> +check_unavailable_features(cpu_list_data->model, sc->model,
>> + >unavailable_features);
>> + }
>> + object_unref(obj);
>> +}
>>
> Looks good to me and produced the expected result under TCG.
>
> Feel free to add my
>
> Reviewed-by: David Hildenbrand <da...@redhat.com>
>
> with or without the modification.
>
--
Mit freundlichen Grüßen/Kind Regards
Viktor Mihajlovski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [Qemu-devel] [PATCH v3] s390: return unavailable features via query-cpu-definitions
On 04.07.2017 13:22, Viktor Mihajlovski wrote:
> On 04.07.2017 11:35, David Hildenbrand wrote:
>>> +
>>> static void create_cpu_model_list(ObjectClass *klass, void *opaque)
>>> {
>>> -CpuDefinitionInfoList **cpu_list = opaque;
>>> +struct CpuDefinitionInfoListData *cpu_list_data = opaque;
>>> +CpuDefinitionInfoList **cpu_list = _list_data->list;
>>> CpuDefinitionInfoList *entry;
>>> CpuDefinitionInfo *info;
>>> char *name = g_strdup(object_class_get_name(klass));
>>> @@ -300,7 +331,19 @@ static void create_cpu_model_list(ObjectClass *klass,
>>> void *opaque)
>>> info->migration_safe = scc->is_migration_safe;
>>> info->q_static = scc->is_static;
>>> info->q_typename = g_strdup(object_class_get_name(klass));
>>> -
>>
>> Wonder if we should add the following:
>>
>> /* the host model can never be used under TCG */
>> if (scc->kvm_required && !kvm_enabled())) {
>> info->has_unavailable_features = true;
>> list_add_feat("type", >unavailable_features);
>> } else if (cpu_list_data->model ...
>>
>>> +/* check for unavailable features */
>>> +if (cpu_list_data->model) {
>>> +Object *obj;
>>> +S390CPU *sc;
>>> +obj = object_new(object_class_get_name(klass));
>>> +sc = S390_CPU(obj);
>>
>> So we can drop the check here (if we have a host/max_model definition
>> under KVM, then all models have sc->model set). But the current code
>> should also work (it simply will not indicate runability information for
>> the "host" model under TCG, as sc->model will be NULL).
> Hmm ... that made me think that we could remove the check here and make
> check_unavailable_features interpret a NULL model as not
> usable/incompatible (a bit more robust in the unlikely event of a memory
> allocation failure). Thoughts?
In fact, all of this may be moot anyway: libvirt blacklists the host
model as I found out, after wondering why there was no "host" CPU with
unknown usability in the virsh domcapabilities output.
I'll take your r-b then, and abstain from posting my v4 :-).
>>>> +if (sc->model) {
>>> +info->has_unavailable_features = true;
>>> +check_unavailable_features(cpu_list_data->model, sc->model,
>>> + >unavailable_features);
>>> +}
>>> +object_unref(obj);
>>> +}
>>>
>> Looks good to me and produced the expected result under TCG.
>>
>> Feel free to add my
>>
>> Reviewed-by: David Hildenbrand <da...@redhat.com>
>>
>> with or without the modification.
>>
>
>
--
Mit freundlichen Grüßen/Kind Regards
Viktor Mihajlovski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [Qemu-devel] [PATCH] s390: return unavailable features via query-cpu-definitions
On 30.06.2017 18:47, David Hildenbrand wrote:
> On 30.06.2017 15:25, Viktor Mihajlovski wrote:
>> The response for query-cpu-definitions didn't include the
>> unavailable-features field, which is used by libvirt to figure
>> out whether a certain cpu model is usable on the host.
>>
>> The unavailable features are now computed by obtaining the host CPU
>> model and comparing its feature bitmap with the feature bitmaps of
>> the known CPU models.
>>
>> I.e. the output of virsh domcapabilities would change from
>> ...
>>
>> z10EC-base
>> z9EC-base
>> z196.2-base
>> z900-base
>> z990
>> ...
>> to
>> ...
>>
>> z10EC-base
>> z9EC-base
>> z196.2-base
>> z900-base
>> z990
>> ...
>>
>> Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
>> ---
>> target/s390x/cpu_models.c | 51
>> ++-
>> 1 file changed, 46 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>> index 63903c2..dc3371f 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -283,14 +283,43 @@ void s390_cpu_list(FILE *f, fprintf_function print)
>> }
>> }
>>
>> +static S390CPUModel *get_max_cpu_model(Error **errp);
>> +
>> #ifndef CONFIG_USER_ONLY
>> +static void list_add_feat(const char *name, void *opaque);
>> +
>> +static void check_unavailable_features(const S390CPUModel *max_model,
>> + const S390CPUModel *model,
>> + strList **unavailable)
>> +{
>> +S390FeatBitmap missing;
>> +
>> +/* detect missing features if any to properly report them */
>> +bitmap_andnot(missing, model->features, max_model->features,
>> + S390_FEAT_MAX);
>> +if (!bitmap_empty(missing, S390_FEAT_MAX)) {
>> +s390_feat_bitmap_to_ascii(missing,
>> + unavailable,
>> + list_add_feat);
>
> I remember discussing this with Eduardo when he introduced this.
>
> There is one additional case to handle here that might turn a CPU model
> not runnable. This can happen when trying to run a new CPU model on an
> old host, where the features are not the problem, but the model itself.
> E.g. running a GA2 version on a GA1 is not allowed. But this can happen
> more generally also between hardware generations.
>
> Therefore, whenever the model is never than the host model, we have to
> add here the special property "type" as missing feature. The
> documentation partly coveres what we had in mind.>
> qapi-schema.json:
> # @unavailable-features is a list of QOM property names that
>
> # represent CPU model attributes that prevent the CPU from running.
>
> # If the QOM property is read-only, that means there's no known
>
> # way to make the CPU model run in the current host. Implementations
>
> # that choose not to provide specific information return the
>
> # property name "type".
>
> We discussed that "type" should always be added if there is no way to
> make the model runnable (by simply removing features).
Thanks for the clarification.
I guess I was reading that too narrow-minded (no specific information vs
type), although I had the suspicion that features alone wouldn't
suffice. I will follow the query_cpu_model_comparison pattern and return
both "type" and the real features.
> > See check_compatibility() for details. For these cases, add "type" to
> the list. (you might be able to extend check_compatibility(), making
> e.g. the *errp and *unavailable parameters optional).
>
--
Mit freundlichen Grüßen/Kind Regards
Viktor Mihajlovski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
[Qemu-devel] [PATCH v2] s390: return unavailable features via query-cpu-definitions
The response for query-cpu-definitions didn't include the
unavailable-features field, which is used by libvirt to figure
out whether a certain cpu model is usable on the host.
The unavailable features are now computed by obtaining the host CPU
model and comparing it against the known CPU models. The comparison
takes into account the generation, the GA level and the feature
bitmaps. In the case of a CPU generation/GA level mismatch
a feature called "type" is reported to be missing.
As a result, the output of virsh domcapabilities would change
from something like
...
z10EC-base
z9EC-base
z196.2-base
z900-base
z990
...
to
...
z10EC-base
z9EC-base
z196.2-base
z900-base
z990
...
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
target/s390x/cpu_models.c | 58 +++
1 file changed, 53 insertions(+), 5 deletions(-)
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 63903c2..51440ff 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -283,14 +283,50 @@ void s390_cpu_list(FILE *f, fprintf_function print)
}
}
+static S390CPUModel *get_max_cpu_model(Error **errp);
+
#ifndef CONFIG_USER_ONLY
+static void list_add_feat(const char *name, void *opaque);
+
+static void check_unavailable_features(const S390CPUModel *max_model,
+ const S390CPUModel *model,
+ strList **unavailable)
+{
+S390FeatBitmap missing;
+
+/* check general model compatibility */
+if (max_model->def->gen < model->def->gen ||
+(max_model->def->gen == model->def->gen &&
+ max_model->def->ec_ga < model->def->ec_ga)) {
+list_add_feat("type", unavailable);
+}
+
+/* detect missing features if any to properly report them */
+bitmap_andnot(missing, model->features, max_model->features,
+ S390_FEAT_MAX);
+if (!bitmap_empty(missing, S390_FEAT_MAX)) {
+s390_feat_bitmap_to_ascii(missing,
+ unavailable,
+ list_add_feat);
+}
+}
+
+struct CpuDefinitionInfoListData {
+CpuDefinitionInfoList *list;
+Error **errp;
+};
+
static void create_cpu_model_list(ObjectClass *klass, void *opaque)
{
-CpuDefinitionInfoList **cpu_list = opaque;
+struct CpuDefinitionInfoListData *cpu_list_data = opaque;
+CpuDefinitionInfoList **cpu_list = _list_data->list;
CpuDefinitionInfoList *entry;
CpuDefinitionInfo *info;
char *name = g_strdup(object_class_get_name(klass));
S390CPUClass *scc = S390_CPU_CLASS(klass);
+Object *obj;
+S390CPU *sc;
+S390CPUModel *scm;
/* strip off the -s390-cpu */
g_strrstr(name, "-" TYPE_S390_CPU)[0] = 0;
@@ -300,21 +336,33 @@ static void create_cpu_model_list(ObjectClass *klass,
void *opaque)
info->migration_safe = scc->is_migration_safe;
info->q_static = scc->is_static;
info->q_typename = g_strdup(object_class_get_name(klass));
-
+/* check for unavailable features */
+obj = object_new(object_class_get_name(klass));
+sc = S390_CPU(obj);
+scm = get_max_cpu_model(cpu_list_data->errp);
+if (scm && sc->model) {
+info->has_unavailable_features = true;
+check_unavailable_features(scm, sc->model,
>unavailable_features);
+}
entry = g_malloc0(sizeof(*entry));
entry->value = info;
entry->next = *cpu_list;
*cpu_list = entry;
+object_unref(obj);
}
CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
{
-CpuDefinitionInfoList *list = NULL;
+struct CpuDefinitionInfoListData list_data = {
+.list = NULL,
+.errp = errp,
+};
-object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false, );
+object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false,
+ _data);
-return list;
+return list_data.list;
}
static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
--
1.9.1
Re: [Qemu-devel] [PATCH v2] s390: return unavailable features via query-cpu-definitions
On 03.07.2017 11:25, David Hildenbrand wrote:
>
>>
>> +static S390CPUModel *get_max_cpu_model(Error **errp);
>> +
>> #ifndef CONFIG_USER_ONLY
>> +static void list_add_feat(const char *name, void *opaque);
>
> Wonder if we should declare all these prototypes at the beginning of the
> file.
>
Don't know either. Looking around in QEMU, forward declarations for
static functions seem to be used sparsely (only when needed). I could
have reordered the functions to get around without forward decls but
this would have obscured the change. Maybe defer and clean up in a
general cleanup/refactoring?
>> +
>> +static void check_unavailable_features(const S390CPUModel *max_model,
>> + const S390CPUModel *model,
>> + strList **unavailable)
>> +{
>> +S390FeatBitmap missing;
>> +
>> +/* check general model compatibility */
>> +if (max_model->def->gen < model->def->gen ||
>> +(max_model->def->gen == model->def->gen &&
>> + max_model->def->ec_ga < model->def->ec_ga)) {
>> +list_add_feat("type", unavailable);
>> +}
>> +
>> +/* detect missing features if any to properly report them */
>> +bitmap_andnot(missing, model->features, max_model->features,
>> + S390_FEAT_MAX);
>> +if (!bitmap_empty(missing, S390_FEAT_MAX)) {
>> +s390_feat_bitmap_to_ascii(missing,
>> + unavailable,
>> + list_add_feat);
>
> This certainly fits into one line.
True, will change.
>
>> +}
>> +}
>> +
>> +struct CpuDefinitionInfoListData {
>> +CpuDefinitionInfoList *list;
>> +Error **errp;
>> +};
>> +
>> static void create_cpu_model_list(ObjectClass *klass, void *opaque)
>> {
>> -CpuDefinitionInfoList **cpu_list = opaque;
>> +struct CpuDefinitionInfoListData *cpu_list_data = opaque;
>> +CpuDefinitionInfoList **cpu_list = _list_data->list;
>> CpuDefinitionInfoList *entry;
>> CpuDefinitionInfo *info;
>> char *name = g_strdup(object_class_get_name(klass));
>> S390CPUClass *scc = S390_CPU_CLASS(klass);
>> +Object *obj;
>> +S390CPU *sc;
>> +S390CPUModel *scm;
>>
>> /* strip off the -s390-cpu */
>> g_strrstr(name, "-" TYPE_S390_CPU)[0] = 0;
>> @@ -300,21 +336,33 @@ static void create_cpu_model_list(ObjectClass *klass,
>> void *opaque)
>> info->migration_safe = scc->is_migration_safe;
>> info->q_static = scc->is_static;
>> info->q_typename = g_strdup(object_class_get_name(klass));
>> -
>> +/* check for unavailable features */
>> +obj = object_new(object_class_get_name(klass));
>> +sc = S390_CPU(obj);
>> +scm = get_max_cpu_model(cpu_list_data->errp);
>
> H, if this function fails, we will create the same error multiple
> times (as there is no way to stop this function from iterating). And we
> will fail to create a cpu model list in case there is no host cpu model,
> which is a change in behavior (as we will report an error).
>
> Would it be better to simply get the max model in
> arch_query_cpu_definitions() and pass it via CpuDefinitionInfoListData,
> instead of the errp variable?Simplifies things, I like it.
>
> Then you could simply skip the checks and set
> info->has_unavailable_features = false in case there is no max model
> (get_max_cpu_model() returned NULL / an error). (same behavior as for now)
>
> Errors from get_max_cpu_model() then should be ignored and not reported.
>
Just to be sure: you suggest that I should call error_free() after
calling get_max_cpu_model, in order to prevent that the QMP command
query-cpu-definitions fails, right?
>
>
>> +if (scm && sc->model) {
>> +info->has_unavailable_features = true;
>> +check_unavailable_features(scm, sc->model,
>> >unavailable_features);
>> +}
>>
>> entry = g_malloc0(sizeof(*entry));
>> entry->value = info;
>> entry->next = *cpu_list;
>> *cpu_list = entry;
>> +object_unref(obj);
>> }
>>
>> CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>> {
>> -CpuDefinitionInfoList *list = NULL;
>> +struct CpuDefinitionInfoListData list_data = {
>> +.list = NULL,
>> +.errp = errp,
>> +};
>>
>> -object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false,
>> );
>> +object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false,
>> + _data);
>>
>> -return list;
>> +return list_data.list;
>> }
>>
>> static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo
>> *info,
>>
>
>
--
Mit freundlichen Grüßen/Kind Regards
Viktor Mihajlovski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
[Qemu-devel] [PATCH v3] s390: return unavailable features via query-cpu-definitions
The response for query-cpu-definitions didn't include the
unavailable-features field, which is used by libvirt to figure
out whether a certain cpu model is usable on the host.
The unavailable features are now computed by obtaining the host CPU
model and comparing it against the known CPU models. The comparison
takes into account the generation, the GA level and the feature
bitmaps. In the case of a CPU generation/GA level mismatch
a feature called "type" is reported to be missing.
As a result, the output of virsh domcapabilities would change
from something like
...
z10EC-base
z9EC-base
z196.2-base
z900-base
z990
...
to
...
z10EC-base
z9EC-base
z196.2-base
z900-base
z990
...
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
v1 -> v2:
Account for model generation and GA level, not only on features.
v2 -> v3:
Prevent repetitive failures by calling get_max_cpu_model only once.
Ignore get_max_cpu_model failures in query_cpu_definitions in order
return CPU models even if the host CPU model is not available.
target/s390x/cpu_models.c | 62 +++
1 file changed, 57 insertions(+), 5 deletions(-)
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 63903c2..7cb55dc 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -283,10 +283,41 @@ void s390_cpu_list(FILE *f, fprintf_function print)
}
}
+static S390CPUModel *get_max_cpu_model(Error **errp);
+
#ifndef CONFIG_USER_ONLY
+static void list_add_feat(const char *name, void *opaque);
+
+static void check_unavailable_features(const S390CPUModel *max_model,
+ const S390CPUModel *model,
+ strList **unavailable)
+{
+S390FeatBitmap missing;
+
+/* check general model compatibility */
+if (max_model->def->gen < model->def->gen ||
+(max_model->def->gen == model->def->gen &&
+ max_model->def->ec_ga < model->def->ec_ga)) {
+list_add_feat("type", unavailable);
+}
+
+/* detect missing features if any to properly report them */
+bitmap_andnot(missing, model->features, max_model->features,
+ S390_FEAT_MAX);
+if (!bitmap_empty(missing, S390_FEAT_MAX)) {
+s390_feat_bitmap_to_ascii(missing, unavailable, list_add_feat);
+}
+}
+
+struct CpuDefinitionInfoListData {
+CpuDefinitionInfoList *list;
+S390CPUModel *model;
+};
+
static void create_cpu_model_list(ObjectClass *klass, void *opaque)
{
-CpuDefinitionInfoList **cpu_list = opaque;
+struct CpuDefinitionInfoListData *cpu_list_data = opaque;
+CpuDefinitionInfoList **cpu_list = _list_data->list;
CpuDefinitionInfoList *entry;
CpuDefinitionInfo *info;
char *name = g_strdup(object_class_get_name(klass));
@@ -300,7 +331,19 @@ static void create_cpu_model_list(ObjectClass *klass, void
*opaque)
info->migration_safe = scc->is_migration_safe;
info->q_static = scc->is_static;
info->q_typename = g_strdup(object_class_get_name(klass));
-
+/* check for unavailable features */
+if (cpu_list_data->model) {
+Object *obj;
+S390CPU *sc;
+obj = object_new(object_class_get_name(klass));
+sc = S390_CPU(obj);
+if (sc->model) {
+info->has_unavailable_features = true;
+check_unavailable_features(cpu_list_data->model, sc->model,
+ >unavailable_features);
+}
+object_unref(obj);
+}
entry = g_malloc0(sizeof(*entry));
entry->value = info;
@@ -310,11 +353,20 @@ static void create_cpu_model_list(ObjectClass *klass,
void *opaque)
CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
{
-CpuDefinitionInfoList *list = NULL;
+struct CpuDefinitionInfoListData list_data = {
+.list = NULL,
+};
+
+list_data.model = get_max_cpu_model(errp);
+if (*errp) {
+error_free(*errp);
+*errp = NULL;
+}
-object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false, );
+object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false,
+ _data);
-return list;
+return list_data.list;
}
static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
--
1.9.1
Re: [Qemu-devel] [RFC PATCH 00/14] Implement network booting directly into the s390-ccw BIOS
On 29.06.2017 09:58, Thomas Huth wrote: > On 28.06.2017 17:02, Viktor Mihajlovski wrote: >> On 28.06.2017 12:56, Thomas Huth wrote: >>> On 28.06.2017 10:02, Thomas Huth wrote: >>>> On 28.06.2017 09:28, Viktor Mihajlovski wrote: >>>>> On 27.06.2017 23:40, Thomas Huth wrote: >>>>> [...] >>>>>>>> - Is it OK to require loading an .INS file first? Or does anybody >>>>>>>> have a better idea how to load multiple files (kernel, initrd, >>>>>>>> etc. ...)? >>>>>>> It would be nice to support PXE-style boot, because the majority of boot >>>>>>> servers is set up that way. A straightforward way would be to do a PXE >>>>>>> emulation by attempting to download a pxelinux.cfg from the well-known >>>>>>> locations, parsing the content (menu) and finally load the kernel, >>>>>>> initrd and set the kernel command line as specified there. (I know, but >>>>>>> you're already parsing the INS-File). >>>>>> >>>>>> Please, don't mix up PXE and pxelinux (since you've used both terms in >>>>>> above paragraph). Assuming that you're only talking about pxlinux config >>>>>> files... are they that common on s390x already? Using the pxelinux >>>>>> config file syntax sounds like we would be completely bound to only >>>>>> loading Linux guests to me, since the boot loader has to know where to >>>>>> load the initrd and how to patch the kernel so that it can find the >>>>>> initrd. >>>>>> Using .INS files sounds more flexible to me instead, since you can also >>>>>> specify the addresses here - so you can theoretically also load other >>>>>> guest kernels, and that's IMHO the better approach since a firmware >>>>>> should stay as generic as possible. >>>>>> >>>>> In order to be consumable, the network boot should support the most >>>>> common configurations. I would think that most network boot servers are >>>>> setup as PXE boot servers using pxelinux configs. >>>> >>>> Are you really sure about the popularity of pxelinux? It's just one >>>> flavor of secondary stage network boot loaders - which also only exist >>>> on x86 so far, as far as I know. >>> >>> And it seems like it also only works with legacy BIOSes, i.e. you can >>> not use it on EFI-only systems, if I've got that right: >>> >>> https://github.com/openSUSE/kiwi/wiki/Setup-PXE-boot-with-EFI-Using-GRUB2 >>> >>> So I guess the significance of pxelinux will very likely decrease in >>> the next years... >>> >> Maybe, but supposed goners tend to linger more often than not. I.e., the >> syslinux project offers a EFI bootloader called syslinux.efi equivalent >> to the pexelinux.0 BIOS loader. >> Further, the OPAL firmware of newer POWER systems embeds petitboot and >> thus offers PXELINUX-compatible network boot as well. > > OK, true, ... you're slowly get me convinced that this pxelinux.cfg > stuff is maybe not such a bad idea after all ... So I guess supporting > at least the basic commands from the pxelinux config file would be > appropriate... (the full set of commands is huge, see > http://www.syslinux.org/wiki/index.php?title=Config ) > >> I appreciate the idea of a proper BOOTP implementation though, so maybe >> a compromise could be to start off with your proposal with the slight >> modification that the final boot action is controlled by the bootfile >> content (file magic), similar to what you suggested in order to support >> both insfile and binary kernel. PXELINUX emulation could be triggered by >> a specially crafted bootfile then. What do you think? > > Yes, something like this could work: > > 1) Do DHCP to get TFTP server address and boot file name > 2) Load the boot file from the TFTP server to address 0 > 3) If the boot file name ended with ".ins" or ".INS" (and the content >starts with the "* " magic), treat it as an .INS file and load the >files that are listed in there accordingly > 4) If the boot file looks like a kernel, start it directly > 5) If not successful in 3 or 4, start looking for a pxelinux config >file by trying to download the config files as specified in >http://www.syslinux.org/wiki/index.php?title=PXELINUX#Configuration >and then parse the file and load the kernel + initrd accordingly. > > Quite a bit of work, so I'll continue to ignore 5 for the first > versions, but I agree now that it can certainly be added later. That sounds more than fair. Thanks! > > Thomas > -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [Qemu-devel] [RFC PATCH 00/14] Implement network booting directly into the s390-ccw BIOS
On 28.06.2017 12:56, Thomas Huth wrote: > On 28.06.2017 10:02, Thomas Huth wrote: >> On 28.06.2017 09:28, Viktor Mihajlovski wrote: >>> On 27.06.2017 23:40, Thomas Huth wrote: >>> [...] >>>>>> - Is it OK to require loading an .INS file first? Or does anybody >>>>>> have a better idea how to load multiple files (kernel, initrd, >>>>>> etc. ...)? >>>>> It would be nice to support PXE-style boot, because the majority of boot >>>>> servers is set up that way. A straightforward way would be to do a PXE >>>>> emulation by attempting to download a pxelinux.cfg from the well-known >>>>> locations, parsing the content (menu) and finally load the kernel, >>>>> initrd and set the kernel command line as specified there. (I know, but >>>>> you're already parsing the INS-File). >>>> >>>> Please, don't mix up PXE and pxelinux (since you've used both terms in >>>> above paragraph). Assuming that you're only talking about pxlinux config >>>> files... are they that common on s390x already? Using the pxelinux >>>> config file syntax sounds like we would be completely bound to only >>>> loading Linux guests to me, since the boot loader has to know where to >>>> load the initrd and how to patch the kernel so that it can find the initrd. >>>> Using .INS files sounds more flexible to me instead, since you can also >>>> specify the addresses here - so you can theoretically also load other >>>> guest kernels, and that's IMHO the better approach since a firmware >>>> should stay as generic as possible. >>>> >>> In order to be consumable, the network boot should support the most >>> common configurations. I would think that most network boot servers are >>> setup as PXE boot servers using pxelinux configs. >> >> Are you really sure about the popularity of pxelinux? It's just one >> flavor of secondary stage network boot loaders - which also only exist >> on x86 so far, as far as I know. > > And it seems like it also only works with legacy BIOSes, i.e. you can > not use it on EFI-only systems, if I've got that right: > > https://github.com/openSUSE/kiwi/wiki/Setup-PXE-boot-with-EFI-Using-GRUB2 > > So I guess the significance of pxelinux will very likely decrease in > the next years... > Maybe, but supposed goners tend to linger more often than not. I.e., the syslinux project offers a EFI bootloader called syslinux.efi equivalent to the pexelinux.0 BIOS loader. Further, the OPAL firmware of newer POWER systems embeds petitboot and thus offers PXELINUX-compatible network boot as well. I appreciate the idea of a proper BOOTP implementation though, so maybe a compromise could be to start off with your proposal with the slight modification that the final boot action is controlled by the bootfile content (file magic), similar to what you suggested in order to support both insfile and binary kernel. PXELINUX emulation could be triggered by a specially crafted bootfile then. What do you think? > Thomas > -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [Qemu-devel] [RFC PATCH 00/14] Implement network booting directly into the s390-ccw BIOS
On 27.06.2017 13:48, Thomas Huth wrote: > It's already possible to do a network boot of an s390x guest with an > external netboot image (based on a Linux installation), but it would > be much more convenient if the s390-ccw firmware supported network > booting right out of the box, without the need to assemble such an > external image first. > > This patch series now introduces network booting via DHCP and TFTP > directly into the s390-ccw firmware by re-using the networking stack > from the SLOF firmware (see https://github.com/aik/SLOF/ for details), > and adds a driver for virtio-net-ccw devices.Neat :) > > Once the patches have been applied, you can download an .INS file > via TFTP which contains the information about the further files > that have to be loaded - kernel, initrd, etc. For example, you can > use the built-in TFTP and DHCP server of QEMU for this by starting > QEMU with: > > qemu-system-s390x ... -device virtio-net,netdev=n1,bootindex=1 \ >-netdev user,id=n1,tftp=/path/to/tftp,bootfile=generic.ins > or the dnsmasq tftp server configured by libvirt (SCNR) > The .INS file has to use the same syntax as the .INS files that can > already be found on s390x Linux distribution installation CD-ROMs. > > The patches are still in a rough shape, but before I continue here, > I though I'd get some feedback first. Specifically: > > - This adds a lot of additional code to the s390-ccw firmware (and > the binary is afterwards three times as big as before, 75k instead > of 25k) ... is that still acceptable? The size may be less of an issue than the general overhead for initialization. Since the current approach of the s390-ccw firmware of lazy loading (only if a network boot is requested) takes care of that, would it be conceivable that you build a standalone network boot firmware image instead of incorporating that into the base firmware? > > - Is it OK to require loading an .INS file first? Or does anybody > have a better idea how to load multiple files (kernel, initrd, > etc. ...)? It would be nice to support PXE-style boot, because the majority of boot servers is set up that way. A straightforward way would be to do a PXE emulation by attempting to download a pxelinux.cfg from the well-known locations, parsing the content (menu) and finally load the kernel, initrd and set the kernel command line as specified there. (I know, but you're already parsing the INS-File). Alternatively, one could load a single boot image (consisting of kernel and initrd concatenated, i.e. the bootable ISO format). This could serve as a more potent "stage 2" boot loader. > > - The code from SLOF uses a different coding style (TABs instead > of space) ... is it OK to keep that coding style here so we > can share patches between SLOF and s390-ccw more easil> > - The code only supports TFTP (via UDP) ... I think that is OK for > most use-cases, but if we ever want to support network booting > via HTTP or something else that is based on TCP, we would need to > use something else instead... Should we maybe rather head towards > grub2, petitboot or something different instead? I don't have an opinion on whether HTTP, FTP, etc is needed, but at some point in time it would definitely be cool to have IPv6 support. Not sure whether SLOF has that included. > > Thomas > > [...] -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [Qemu-devel] [RFC PATCH 00/14] Implement network booting directly into the s390-ccw BIOS
On 27.06.2017 23:40, Thomas Huth wrote: [...] >>> - Is it OK to require loading an .INS file first? Or does anybody >>> have a better idea how to load multiple files (kernel, initrd, >>> etc. ...)? >> It would be nice to support PXE-style boot, because the majority of boot >> servers is set up that way. A straightforward way would be to do a PXE >> emulation by attempting to download a pxelinux.cfg from the well-known >> locations, parsing the content (menu) and finally load the kernel, >> initrd and set the kernel command line as specified there. (I know, but >> you're already parsing the INS-File). > > Please, don't mix up PXE and pxelinux (since you've used both terms in > above paragraph). Assuming that you're only talking about pxlinux config > files... are they that common on s390x already? Using the pxelinux > config file syntax sounds like we would be completely bound to only > loading Linux guests to me, since the boot loader has to know where to > load the initrd and how to patch the kernel so that it can find the initrd. > Using .INS files sounds more flexible to me instead, since you can also > specify the addresses here - so you can theoretically also load other > guest kernels, and that's IMHO the better approach since a firmware > should stay as generic as possible. > In order to be consumable, the network boot should support the most common configurations. I would think that most network boot servers are setup as PXE boot servers using pxelinux configs. It will do no good to tell system administrators to have a totally different setup for s390 boot clients. If the firmware doesn't support it we will have to fall back to provide a Linux-based fat netboot image to the pxelinux handling :-(. >> Alternatively, one could load a single boot image (consisting of kernel >> and initrd concatenated, i.e. the bootable ISO format). This could serve >> as a more potent "stage 2" boot loader. > > Agreed, that's also a common practice when doing network booting. I > guess the firmware could also support both quite easily, direct single > boot images, and .INS files. The latter could be detected via the file > name or with the magic string "* " at the beginning. > >>> - The code from SLOF uses a different coding style (TABs instead >>> of space) ... is it OK to keep that coding style here so we >>> can share patches between SLOF and s390-ccw more easil> >>> - The code only supports TFTP (via UDP) ... I think that is OK for >>> most use-cases, but if we ever want to support network booting >>> via HTTP or something else that is based on TCP, we would need to >>> use something else instead... Should we maybe rather head towards >>> grub2, petitboot or something different instead? >> I don't have an opinion on whether HTTP, FTP, etc is needed, but at some >> point in time it would definitely be cool to have IPv6 support. Not sure >> whether SLOF has that included. > > Yes, IPv6 is included in this networking stack. > > Thomas > -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
[Qemu-devel] [PATCH] s390: set DHCP client architecure id for netboot
Setting the client architecture DHCP option to 0x001f (s390 Basic) [1] allows the DHCP server to return a s390-specific bootfile if wanted. DHCP servers not configured for the option (or not yet recognizing the option value) will continue to work as they have done before. [1] https://www.iana.org/assignments/dhcpv6-parameters Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com> --- pc-bios/s390-ccw/netboot.mak | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pc-bios/s390-ccw/netboot.mak b/pc-bios/s390-ccw/netboot.mak index a9e1374..a25d238 100644 --- a/pc-bios/s390-ccw/netboot.mak +++ b/pc-bios/s390-ccw/netboot.mak @@ -50,7 +50,7 @@ libc.a: $(LIBCOBJS) LIBNETOBJS := args.o dhcp.o dns.o icmpv6.o ipv6.o tcp.o udp.o bootp.o \ dhcpv6.o ethernet.o ipv4.o ndp.o tftp.o -LIBNETCFLAGS := $(QEMU_CFLAGS) $(LIBC_INC) $(LIBNET_INC) +LIBNETCFLAGS := $(QEMU_CFLAGS) -DDHCPARCH=0x1F $(LIBC_INC) $(LIBNET_INC) %.o : $(SLOF_DIR)/lib/libnet/%.c $(call quiet-command,$(CC) $(LIBNETCFLAGS) -c -o $@ $<,"CC","$(TARGET_DIR)$@") -- 1.9.1
Re: [Qemu-devel] [PATCH] s390: set DHCP client architecure id for netboot
On 11.09.2017 15:58, Thomas Huth wrote: > On 11.09.2017 12:33, Viktor Mihajlovski wrote: >> Setting the client architecture DHCP option to 0x001f (s390 Basic) [1] >> allows the DHCP server to return a s390-specific bootfile if wanted. >> DHCP servers not configured for the option (or not yet recognizing the >> option value) will continue to work as they have done before. >> >> [1] https://www.iana.org/assignments/dhcpv6-parameters > > Ah, nice, you already registered a value for s390x :-) > May I ask what's the difference between "Basic" and "Extended" there? > sure, "Basic" stands for the "classic netboot", i.e. a single binary file is designated by the bootfile in the DHCP reply. "Extended" is the mode implemented by Dynamic Partition Manager, where the bootfile is some sort of configuration file. [...] -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
[Qemu-devel] Suggested Wiki Update
Hi, the content of https://wiki.qemu.org/index.php/Features/S390xNetworkBoot is slightly outdated since proper DHCP based netboot support for s390x has been implemented by the series ending with commit 29d1221. As it is still desirable to provide some instructions on building network bootable images for the s390x architecture, I'd like to suggest that the text is replaced (see proposal below). Conny, could I once more request your help in that matter? == Building a s390 network bootable binary == Since 2.10 QEMU offers network boot support which allows to load a binary from a TFTP server and boot it. As s390 network bootable images are not very common, this document gives a brief overview on how to build one. A s390 network bootable image can be built by bundling some shell scripts, busybox and the kexec binary bundled into an initial ramdisk and append that to a kernel image. An existing s390 system can be used as source. One way to do that is to take a kernel and an installer initial ramdisk from a distribution's DVD/ISO and concatenate them. Some fixups are necessary int the new binary, which can be done by using the script in [https://github.com/ibm-s390-tools/s390-tools/blob/master/netboot/mk-s390image]. Booting this image would then start the installation process as if booted from the DVD. Another possible way is to build a binary that behaves similar to the PXELINUX boot loader. In this case an initial ramdisk with an init process triggering the PXELINUX-like processing has to be built as described in [https://github.com/ibm-s390-tools/s390-tools/tree/master/netboot]. The site also contains a script assisting in the creation of such an initial ramdisk. -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [Qemu-devel] Suggested Wiki Update
On 12.09.2017 13:25, Thomas Huth wrote:
> On 12.09.2017 13:18, Cornelia Huck wrote:
[...]
>>
>> Looks good.
>>
>> Can you perhaps add a sentence that the base support is available with
>> 2.9 and netboot with 2.10? ('Overview' looks like a good place for
>> that.)
>
> FWIW, I think it would also be good to add a sentence about the need to
> specify "bootindex" somewhere (I remember trying to use "-boot n" the
> first time I wanted to use it) ...
>
Good points, I'll add more context ... thanks for the review.
--
Mit freundlichen Grüßen/Kind Regards
Viktor Mihajlovski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
[Qemu-devel] [PATCH v2] S390: Expose s390-specific CPU info
Presently s390x is the only architecture not exposing specific
CPU information via QMP query-cpus. Upstream discussion has shown
that it could make sense to report the architecture specific CPU
state, e.g. to detect that a CPU has been stopped.
With this change the output of query-cpus will look like this on
s390:
[
{"arch": "s390", "current": true,
"props": {"core-id": 0}, "cpu-state": "operating", "CPU": 0,
"qom_path": "/machine/unattached/device[0]",
"halted": false, "thread_id": 63115},
{"arch": "s390", "current": false,
"props": {"core-id": 1}, "cpu-state": "stopped", "CPU": 1,
"qom_path": "/machine/unattached/device[1]",
"halted": true, "thread_id": 63116}
]
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
v1 -> v2:
- qapi-schema.json: fixed style issues, typos, improved enum naming
- hmp.c: added missing newline (printing cpu state)
cpus.c | 6 ++
hmp.c | 4
hw/s390x/s390-virtio-ccw.c | 2 +-
qapi-schema.json | 28 +++-
target/s390x/cpu.c | 24
target/s390x/cpu.h | 7 ++-
target/s390x/kvm.c | 8
target/s390x/sigp.c| 38 +++---
8 files changed, 75 insertions(+), 42 deletions(-)
diff --git a/cpus.c b/cpus.c
index 2cb0af9..39e46dd 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2033,6 +2033,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
#elif defined(TARGET_TRICORE)
TriCoreCPU *tricore_cpu = TRICORE_CPU(cpu);
CPUTriCoreState *env = _cpu->env;
+#elif defined(TARGET_S390X)
+S390CPU *s390_cpu = S390_CPU(cpu);
+CPUS390XState *env = _cpu->env;
#endif
cpu_synchronize_state(cpu);
@@ -2060,6 +2063,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
#elif defined(TARGET_TRICORE)
info->value->arch = CPU_INFO_ARCH_TRICORE;
info->value->u.tricore.PC = env->PC;
+#elif defined(TARGET_S390X)
+info->value->arch = CPU_INFO_ARCH_S390;
+info->value->u.s390.cpu_state = env->cpu_state;
#else
info->value->arch = CPU_INFO_ARCH_OTHER;
#endif
diff --git a/hmp.c b/hmp.c
index b3de32d..219f769 100644
--- a/hmp.c
+++ b/hmp.c
@@ -390,6 +390,10 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
case CPU_INFO_ARCH_TRICORE:
monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore.PC);
break;
+case CPU_INFO_ARCH_S390:
+monitor_printf(mon, " state=%s\n",
+ CpuS390State_str(cpu->value->u.s390.cpu_state));
+break;
default:
break;
}
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 3807dcb..a7ad7e7 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -373,7 +373,7 @@ static void s390_machine_reset(void)
/* all cpus are stopped - configure and start the ipl cpu only */
s390_ipl_prepare_cpu(ipl_cpu);
-s390_cpu_set_state(CPU_STATE_OPERATING, ipl_cpu);
+s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu);
}
static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
diff --git a/qapi-schema.json b/qapi-schema.json
index 5c06745..66e0927 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -410,10 +410,12 @@
# An enumeration of cpu types that enable additional information during
# @query-cpus.
#
+# @s390: since 2.12
+#
# Since: 2.6
##
{ 'enum': 'CpuInfoArch',
- 'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 'other' ] }
+ 'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 's390', 'other' ] }
##
# @CpuInfo:
@@ -452,6 +454,7 @@
'ppc': 'CpuInfoPPC',
'mips': 'CpuInfoMIPS',
'tricore': 'CpuInfoTricore',
+'s390': 'CpuInfoS390',
'other': 'CpuInfoOther' } }
##
@@ -522,6 +525,29 @@
{ 'struct': 'CpuInfoOther', 'data': { } }
##
+# @CpuS390State:
+#
+# An enumeration of cpu states that can be assumed by a virtual
+# S390 CPU
+#
+# Since: 2.12
+##
+{ 'enum': 'CpuS390State',
+ 'prefix': 'S390_CPU_STATE',
+ 'data': [ 'uninitialized', 'stopped', 'check-stop', 'operating', 'load' ] }
+
+##
+# @CpuInfoS390:
+#
+# Additional information about a virtual S390 CPU
+#
+# @cpu-state: the virtual CPU's state
+#
+# Since: 2.12
+##
+{ 'struct': 'CpuInfoS390', 'data': { 'cpu-state': 'CpuS390State' } }
+
+##
# @query-cpus:
#
# Returns a list of information about each virtual CPU.
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index d2e6b9f..4621eb4 100644
--- a/target/s390x/cpu.c
+++ b/targ
Re: [Qemu-devel] [PATCH] S390: Expose s390-specific CPU info
On 08.02.2018 17:22, Luiz Capitulino wrote:
> On Thu, 8 Feb 2018 16:52:28 +0100
> Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com> wrote:
>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 12c7dc8..0b36860 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -607,7 +607,27 @@
>> ##
>> { 'struct': 'CpuInfo2',
>>'data': {'cpu-index': 'int', '*halted': 'bool', 'qom-path': 'str',
>> - 'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
>> + 'thread-id': 'int', '*props': 'CpuInstanceProperties',
>> + '*archdata': 'CpuInfoArchData' } }
>> +
>> +##
>> +# @CpuInfoArchData:
>> +#
>> +# Architecure specific information about a virtual CPU
>> +#
>> +# Since: 2.12
>> +#
>> +##
>> +{ 'union': 'CpuInfoArchData',
>> + 'base': { 'arch': 'CpuInfoArch' },
>> + 'discriminator': 'arch',
>> + 'data': { 'x86': 'CpuInfoOther',
>> +'sparc': 'CpuInfoOther',
>> +'ppc': 'CpuInfoOther',
>> +'mips': 'CpuInfoOther',
>> +'tricore': 'CpuInfoOther',
>> +'s390': 'CpuInfoS390',
>> +'other': 'CpuInfoOther' } }
>>
>> ##
>> # @query-cpus-fast:
>
> I don't think you need CpuInfoArchData, you can have S390CpuState
> instead and ignore the other archs. It's not like all archs data
> can be returned at the same time, and also you start having to
> replicate that arch string list everywhere. Lastly, the arch name
> is returned by query-target, so no need to duplicate that one either.
>
I don't think I really understood your suggestion. Was it to assume that
only s390 will have arch-specific data?. I.e. something along the lines of
- 'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
+ 'thread-id': 'int', '*props': 'CpuInstanceProperties',
+ '*archdata': 'CpuInfoS390' } }
or some kind of in-line, anonymous union? I have to confess I'm pretty
QAPI-illiterate, so I may have done it overly complicated. At least it
feels that way.
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [PATCH v2] S390: Expose s390-specific CPU info
On 08.02.2018 17:43, Viktor Mihajlovski wrote:
> Presently s390x is the only architecture not exposing specific
> CPU information via QMP query-cpus. Upstream discussion has shown
> that it could make sense to report the architecture specific CPU
> state, e.g. to detect that a CPU has been stopped.
>
> With this change the output of query-cpus will look like this on
> s390:
>
>[
> {"arch": "s390", "current": true,
> "props": {"core-id": 0}, "cpu-state": "operating", "CPU": 0,
> "qom_path": "/machine/unattached/device[0]",
> "halted": false, "thread_id": 63115},
> {"arch": "s390", "current": false,
> "props": {"core-id": 1}, "cpu-state": "stopped", "CPU": 1,
> "qom_path": "/machine/unattached/device[1]",
> "halted": true, "thread_id": 63116}
>]
>
> Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
> ---
>
> v1 -> v2:
> - qapi-schema.json: fixed style issues, typos, improved enum naming
> - hmp.c: added missing newline (printing cpu state)
>
> cpus.c | 6 ++
> hmp.c | 4
> hw/s390x/s390-virtio-ccw.c | 2 +-
> qapi-schema.json | 28 +++-
> target/s390x/cpu.c | 24
> target/s390x/cpu.h | 7 ++-
> target/s390x/kvm.c | 8
> target/s390x/sigp.c| 38 +++---
> 8 files changed, 75 insertions(+), 42 deletions(-)
>
sorry, forgot to cc the maintainers
[...]
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [PATCH 1/2] qmp: add query-cpus-fast
On 07.02.2018 18:50, Luiz Capitulino wrote:
> The query-cpus command has an extremely serious side effect:
> it always interrupt all running vCPUs so that they can run
> ioctl calls. This can cause a huge performance degradation for
> some workloads. And most of the information retrieved by the
> ioctl calls are not even used by query-cpus.
>
> This commit introduces a replacement for query-cpus called
> query-cpus-fast, which has the following features:
>
> o Never interrupt vCPUs threads. query-cpus-fast only returns
>vCPU information maintained by QEMU itself, which should be
>sufficient for most management software needs
>
> o Make "halted" field optional: we only return it if the
>halted state is maintained by QEMU. But this also gives
>the option of dropping the field in the future (see below)
>
> o Drop irrelevant fields such as "current", "pc" and "arch"
I disagree that arch is irrelevant and would strongly suggest to keep
arch and arch-specific fields. At least in the case of s390 there's a
cpu_state field that can be obtained cheaply.
[...]
> diff --git a/cpus.c b/cpus.c
> index 2cb0af9b22..3b68a8146c 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -2083,6 +2083,50 @@ CpuInfoList *qmp_query_cpus(Error **errp)
> return head;
> }
>
> +/*
> + * fast means: we NEVER interrupt vCPU threads to retrieve
> + * information from KVM.
> + */
> +CpuInfo2List *qmp_query_cpus_fast(Error **errp)
> +{
> +MachineState *ms = MACHINE(qdev_get_machine());
> +MachineClass *mc = MACHINE_GET_CLASS(ms);
> +CpuInfo2List *head = NULL, *cur_item = NULL;
> +CPUState *cpu;
> +
> +CPU_FOREACH(cpu) {
> +CpuInfo2List *info = g_malloc0(sizeof(*info));
> +info->value = g_malloc0(sizeof(*info->value));
> +
> +info->value->cpu_index = cpu->cpu_index;
> +info->value->qom_path = object_get_canonical_path(OBJECT(cpu));
> +info->value->thread_id = cpu->thread_id;
> +
> +info->value->has_props = !!mc->cpu_index_to_instance_props;
> +if (info->value->has_props) {
> +CpuInstanceProperties *props;
> +props = g_malloc0(sizeof(*props));
> +*props = mc->cpu_index_to_instance_props(ms, cpu->cpu_index);
> +info->value->props = props;
> +}
> +
> +/* if in kernel irqchip is used, we don't have 'halted' */
> +info->value->has_halted = !kvm_irqchip_in_kernel();
This is definitely not true for s390. Externally observable CPU state
changes are handled by QEMU there. We may still drop halted if we add a
more appropriate arch-specific field.
> +if (info->value->has_halted) {
> +info->value->halted = cpu->halted;
> +}
[...]
--
Regards,
Viktor Mihajlovski
[Qemu-devel] [PATCH] S390: Expose s390-specific CPU info
Presently s390x is the only architecture not exposing specific
CPU information via QMP query-cpus. Upstream discussion has shown
that it could make sense to report the architecture specific CPU
state, e.g. to detect that a CPU has been stopped.
With this change the output of query-cpus will look like this on
s390:
[{"arch": "s390", "current": true,
"props": {"core-id": 0}, "cpu_state": "operating", "CPU": 0,
"qom_path": "/machine/unattached/device[0]",
"halted": false, "thread_id": 63115},
{"arch": "s390", "current": false,
"props": {"core-id": 1}, "cpu_state": "stopped", "CPU": 1,
"qom_path": "/machine/unattached/device[1]",
"halted": true, "thread_id": 63116}]
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
cpus.c | 6 ++
hmp.c | 4
hw/s390x/s390-virtio-ccw.c | 2 +-
qapi-schema.json | 25 -
target/s390x/cpu.c | 24
target/s390x/cpu.h | 7 ++-
target/s390x/kvm.c | 8
target/s390x/sigp.c| 38 +++---
8 files changed, 72 insertions(+), 42 deletions(-)
diff --git a/cpus.c b/cpus.c
index 2cb0af9..39e46dd 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2033,6 +2033,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
#elif defined(TARGET_TRICORE)
TriCoreCPU *tricore_cpu = TRICORE_CPU(cpu);
CPUTriCoreState *env = _cpu->env;
+#elif defined(TARGET_S390X)
+S390CPU *s390_cpu = S390_CPU(cpu);
+CPUS390XState *env = _cpu->env;
#endif
cpu_synchronize_state(cpu);
@@ -2060,6 +2063,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
#elif defined(TARGET_TRICORE)
info->value->arch = CPU_INFO_ARCH_TRICORE;
info->value->u.tricore.PC = env->PC;
+#elif defined(TARGET_S390X)
+info->value->arch = CPU_INFO_ARCH_S390;
+info->value->u.s390.cpu_state = env->cpu_state;
#else
info->value->arch = CPU_INFO_ARCH_OTHER;
#endif
diff --git a/hmp.c b/hmp.c
index b3de32d..37e04c3 100644
--- a/hmp.c
+++ b/hmp.c
@@ -390,6 +390,10 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
case CPU_INFO_ARCH_TRICORE:
monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore.PC);
break;
+case CPU_INFO_ARCH_S390:
+monitor_printf(mon, " state=%s",
+ CpuInfoS390State_str(cpu->value->u.s390.cpu_state));
+break;
default:
break;
}
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 3807dcb..3e6360e 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -373,7 +373,7 @@ static void s390_machine_reset(void)
/* all cpus are stopped - configure and start the ipl cpu only */
s390_ipl_prepare_cpu(ipl_cpu);
-s390_cpu_set_state(CPU_STATE_OPERATING, ipl_cpu);
+s390_cpu_set_state(CPU_INFOS390_STATE_OPERATING, ipl_cpu);
}
static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
diff --git a/qapi-schema.json b/qapi-schema.json
index 5c06745..c34880b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -413,7 +413,7 @@
# Since: 2.6
##
{ 'enum': 'CpuInfoArch',
- 'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 'other' ] }
+ 'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 's390', 'other' ] }
##
# @CpuInfo:
@@ -452,6 +452,7 @@
'ppc': 'CpuInfoPPC',
'mips': 'CpuInfoMIPS',
'tricore': 'CpuInfoTricore',
+'s390': 'CpuInfoS390',
'other': 'CpuInfoOther' } }
##
@@ -522,6 +523,28 @@
{ 'struct': 'CpuInfoOther', 'data': { } }
##
+# @CpuInfoS390State:
+#
+# An enumeration of cpu states that can be assumed by a virtual
+# S390 CPU
+#
+# Since: 2.12
+##
+{ 'enum': 'CpuInfoS390State',
+ 'data': [ 'uninitialized', 'stopped', 'check_stop', 'operating', 'load' ] }
+
+##
+# @CpuInfoS390:
+#
+# Additional information about a virtual S390 CPU
+#
+# @cpu_state: the CPUs state
+#
+# Since: 2.12
+##
+{ 'struct': 'CpuInfoS390', 'data': { 'cpu_state': 'CpuInfoS390State' } }
+
+##
# @query-cpus:
#
# Returns a list of information about each virtual CPU.
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index d2e6b9f..996cbc8 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -58,8 +58,8 @@ static bool s390_cpu_has_work(CPUState *cs)
S390CPU *cpu = S390_CPU(cs);
/* STOPPED cpus can never wake up */
-if (s390_cpu_get_state(cpu) != CPU_STATE_LOAD &&
-s390_cpu_get_state(cpu) != CPU_STATE_OPERATING) {
+if (s390_cpu_g
Re: [Qemu-devel] [PATCH 1/2] qmp: add query-cpus-fast
On 08.02.2018 08:41, Viktor Mihajlovski wrote: > On 07.02.2018 18:50, Luiz Capitulino wrote: >> The query-cpus command has an extremely serious side effect: >> it always interrupt all running vCPUs so that they can run >> ioctl calls. This can cause a huge performance degradation for >> some workloads. And most of the information retrieved by the >> ioctl calls are not even used by query-cpus. >> >> This commit introduces a replacement for query-cpus called >> query-cpus-fast, which has the following features: >> >> o Never interrupt vCPUs threads. query-cpus-fast only returns >>vCPU information maintained by QEMU itself, which should be >>sufficient for most management software needs >> >> o Make "halted" field optional: we only return it if the >>halted state is maintained by QEMU. But this also gives >>the option of dropping the field in the future (see below) >> >> o Drop irrelevant fields such as "current", "pc" and "arch" > I disagree that arch is irrelevant and would strongly suggest to keep > arch and arch-specific fields. At least in the case of s390 there's a > cpu_state field that can be obtained cheaply. I've posted a patch [1] to add s390-specific state info to the query-cpus output. This state *can* be obtained without kicking the CPU out of VM execution. With this info in the query-cpus-fast return data we can eventually get rid of halted and its ramifications. [...] [1] https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02032.html -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [PATCH v2] qmp: add query-cpus-fast
On 08.02.2018 21:33, Eduardo Habkost wrote: > On Thu, Feb 08, 2018 at 11:17:32AM -0500, Luiz Capitulino wrote: > [...] >> The "halted" field is somewhat controversial. On the one hand, >> it offers a convenient way to know if a guest CPU is idle or >> running. On the other hand, it's a field that can change many >> times a second. In fact, the halted state can change even >> before query-cpus-fast has returned. This makes one wonder if >> this field should be dropped all together. Having the "halted" >> field as optional gives a better option for dropping it in >> the future, since we can just stop returning it. > > I'd just drop it, unless we find a use case where it's really > useful. > > Also, the code that sets/clears cpu->halted is target-specific, > so I wouldn't be so sure that simply checking for > !kvm_irqchip_in_kernel() is enough on all targets. > Right, the present patch effectively disables halted anyway (including s390). So it may be cleaner to just drop it right now. Assuming the presence of architecure-specific data, libvirt can derive a halted state (or an equivalent thereof) from query-cpus-fast returned information. -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [qemu-s390x] [PATCH v5 05/12] s390-ccw: move auxiliary IPL data to separate location
On 06.02.2018 10:23, Thomas Huth wrote:
> On 05.02.2018 21:57, Collin L. Walling wrote:
>> The s390-ccw firmware needs some information in support of the
>> boot process which is not available on the native machine.
>> Examples are the netboot firmware load address and now the
>> boot menu parameters.
>>
>> While storing that data in unused fields of the IPL parameter block
>> works, that approach could create problems if the parameter block
>> definition should change in the future. Because then a guest could
>> overwrite these fields using the set IPLB diagnose.
>>
>> In fact the data in question is of more global nature and not really
>> tied to an IPL device, so separating it is rather logical.
>>
>> This commit introduces a new structure to hold firmware relevant
>> IPL parameters set by QEMU. The data is stored at location 204 (dec)
>> and can contain up to 7 32-bit words. This area is available to
>> programming in the z/Architecture Principles of Operation and
>> can thus safely be used by the firmware until the IPL has completed.
>
> Sounds like a good idea.
>
>> Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
>> ---
>> hw/s390x/ipl.c | 18 +-
>> hw/s390x/ipl.h | 11 +--
>> pc-bios/s390-ccw/iplb.h | 12 ++--
>> pc-bios/s390-ccw/main.c | 6 +-
>> 4 files changed, 41 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 0d06fc1..3e3c3b8 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -399,6 +399,20 @@ void s390_reipl_request(void)
>> qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>> }
>>
>> +static void s390_ipl_prepare_qipl(S390CPU *cpu)
>> +{
>> +S390IPLState *ipl = get_ipl_device();
>> +uint8_t *addr;
>> +uint64_t len = 4096;
>> +
>> +addr = cpu_physical_memory_map(cpu->env.psa, , 1);
>> +if (!addr || len < 204 + sizeof(QemuIplParameters)) {
>> +error_report("Cannot set QEMU IPL parameters");
>
> I think you should return or exit() here. Otherwise the memcpy below
> accesses an illegal memory range.Right, I have noticed and fixed that on my
> private branch, but forgot to
update the patch. Collin, could you squash in a
return;
>
>> +}
>> +memcpy(addr + 204, >iplb.qipl, sizeof(QemuIplParameters));
>> +cpu_physical_memory_unmap(addr, len, 1, len);
>> +}
>> +
[...]
>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>> index 8a705e0..68dcaf8 100644
>> --- a/hw/s390x/ipl.h
>> +++ b/hw/s390x/ipl.h
>> @@ -16,8 +16,7 @@
>> #include "cpu.h"
>>
>> struct IplBlockCcw {
>> -uint64_t netboot_start_addr;
>> -uint8_t reserved0[77];
>> +uint8_t reserved0[85];
>> uint8_t ssid;
>> uint16_t devno;
>> uint8_t vm_flags;
>> @@ -59,6 +58,13 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>>
>> #define DIAG308_FLAGS_LP_VALID 0x80
>>
>> +struct QemuIplParameters {
>> +uint8_t reserved1[4];
>> +uint64_t netboot_start_addr;
>> +uint8_t reserved2[16];
>> +} QEMU_PACKED;
>> +typedef struct QemuIplParameters QemuIplParameters;
>> +
>> union IplParameterBlock {
>> struct {
>> uint32_t len;
>> @@ -74,6 +80,7 @@ union IplParameterBlock {
>> IplBlockFcp fcp;
>> IplBlockQemuScsi scsi;
>> };
>> +QemuIplParameters qipl;
>> } QEMU_PACKED;
>
> Sorry if I get that wrong, but if you store that within
> IplParameterBlock, the information is still passed via DIAG 0x308, isn't
> it? (see handle_diag_308(), case 6). Is that really what was intended here?
>
Got me :).
The intention is to remove non-standard data from the IPL parameter
block. But I needed a place to store it in the s390-ipl context. The
IPLB conveniently has plenty of space following the standard data.
The alternative would be to have the QEMU IPL parameters explicitly
called out in the object. But they're only duplication information
available elsewhere (boot menu) or are computed anyway (netboot).
This may change in the future...
> [...]
>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>> index e857ce4..825a1a3 100644
>> --- a/pc-bios/s390-ccw/main.c
>> +++ b/pc-bios/s390-ccw/main.c
>> @@ -16,6 +16,7 @@ char stack[PAGE_SIZE * 8]
>> __attribute__((__aligned__(PAGE_SIZE)));
>> static SubChannelId blk_schid = { .one = 1 };
>> IplPar
Re: [Qemu-devel] [PATCH v5 05/12] s390-ccw: move auxiliary IPL data to separate location
On 06.02.2018 10:45, Christian Borntraeger wrote: > On 02/05/2018 09:57 PM, Collin L. Walling wrote: >> The s390-ccw firmware needs some information in support of the >> boot process which is not available on the native machine. >> Examples are the netboot firmware load address and now the >> boot menu parameters. >> >> While storing that data in unused fields of the IPL parameter block >> works, that approach could create problems if the parameter block >> definition should change in the future. Because then a guest could >> overwrite these fields using the set IPLB diagnose. >> >> In fact the data in question is of more global nature and not really >> tied to an IPL device, so separating it is rather logical. >> >> This commit introduces a new structure to hold firmware relevant >> IPL parameters set by QEMU. The data is stored at location 204 (dec) >> and can contain up to 7 32-bit words. This area is available to >> programming in the z/Architecture Principles of Operation and >> can thus safely be used by the firmware until the IPL has completed. > > This content is set by QEMU/the ccw bios on every IPL, so we do not > have to change anything for migration. Correct? > Hm ... this is true for the netboot start address, but I am not sure about the boot menu parameters added in a follow on patch. I'll comment there... [...] -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [PATCH v5 06/12] s390-ccw: parse and set boot menu options
On 05.02.2018 21:57, Collin L. Walling wrote:
> Set boot menu options for an s390 guest and store them in
> the iplb. These options are set via the QEMU command line
> option:
>
> -boot menu=on|off[,splash-time=X]
>
> or via the libvirt domain xml:
>
>
>
>
>
> Where X represents some positive integer representing
> milliseconds.
>
> Any value set for loadparm will override all boot menu options.
> If loadparm=PROMPT, then the menu will be enabled without a
> timeout.
>
> The absence of any boot options on the command line will flag
> to later use the zipl boot loader values.
>
> Signed-off-by: Collin L. Walling <wall...@linux.vnet.ibm.com>
> Reviewed-by: Janosch Frank <fran...@linux.vnet.ibm.com>
> Reviewed-by: Thomas Huth <th...@redhat.com>
> ---
> hw/s390x/ipl.c | 52
> +
> hw/s390x/ipl.h | 7 ++-
> pc-bios/s390-ccw/iplb.h | 4 +++-
> 3 files changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 3e3c3b8..227dccb 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -23,6 +23,8 @@
> #include "hw/s390x/ebcdic.h"
> #include "ipl.h"
> #include "qemu/error-report.h"
> +#include "qemu/config-file.h"
> +#include "qemu/cutils.h"
>
> #define KERN_IMAGE_START0x01UL
> #define KERN_PARM_AREA 0x010480UL
> @@ -219,6 +221,53 @@ static Property s390_ipl_properties[] = {
> DEFINE_PROP_END_OF_LIST(),
> };
>
> +static void s390_ipl_set_boot_menu(IplParameterBlock *iplb)
> +{
> +QemuOptsList *plist = qemu_find_opts("boot-opts");
> +QemuOpts *opts = QTAILQ_FIRST(>head);
> +uint8_t *flags;
> +uint16_t *timeout;
> +const char *tmp;
> +unsigned long result = 0;
> +
> +switch (iplb->pbt) {
> +case S390_IPL_TYPE_CCW:
> +case S390_IPL_TYPE_QEMU_SCSI:
> +flags = >qipl.boot_menu_flags;
> +timeout = >qipl.boot_menu_timeout;
> +break;
> +default:
> +error_report("boot menu is not supported for this device type.");
> +return;
> +}
> +
> +/* In the absence of -boot menu, use zipl parameters */
> +if (!qemu_opt_get(opts, "menu")) {
> +*flags = BOOT_MENU_FLAG_ZIPL_OPTS;
> +} else if (boot_menu) {
> +*flags = BOOT_MENU_FLAG_CMD_OPTS;
> +
> +tmp = qemu_opt_get(opts, "splash-time");
> +
> +if (tmp && qemu_strtoul(tmp, NULL, 10, )) {
> +error_report("splash-time value is invalid, forcing it to 0.");
> +*timeout = 0;
> +return;
> +}
> +
> +result = (result + 500) / 1000; /* Round and convert to seconds */
> +
> +if (result > 0x) {
> +error_report("splash-time value is greater than 65535000,"
> + " forcing it to 65535000.");
> +*timeout = 0x;
> +return;
> +}
> +
> +*timeout = cpu_to_be16(result);
> +}
> +}
> +
> static bool s390_gen_initial_iplb(S390IPLState *ipl)
> {
> DeviceState *dev_st;
> @@ -273,6 +322,9 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
> if (!s390_ipl_set_loadparm(ipl->iplb.loadparm)) {
> ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
> }
> +
> +s390_ipl_set_boot_menu(>iplb);> +
Maybe it would be safer wrt migration to move that to
s390_ipl_prepare_cpu()?
> return true;
> }
>
[...]
--
Regards,
Viktor Mihajlovski
[Qemu-devel] [PATCH 1/3] qmp: expose s390-specific CPU info
Presently s390x is the only architecture not exposing specific
CPU information via QMP query-cpus. Upstream discussion has shown
that it could make sense to report the architecture specific CPU
state, e.g. to detect that a CPU has been stopped.
With this change the output of query-cpus will look like this on
s390:
[
{"arch": "s390", "current": true,
"props": {"core-id": 0}, "cpu-state": "operating", "CPU": 0,
"qom_path": "/machine/unattached/device[0]",
"halted": false, "thread_id": 63115},
{"arch": "s390", "current": false,
"props": {"core-id": 1}, "cpu-state": "stopped", "CPU": 1,
"qom_path": "/machine/unattached/device[1]",
"halted": true, "thread_id": 63116}
]
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
Acked-by: Eric Blake <ebl...@redhat.com>
---
cpus.c | 6 ++
hmp.c | 4
hw/intc/s390_flic.c| 4 ++--
hw/s390x/s390-virtio-ccw.c | 2 +-
qapi-schema.json | 28 +++-
target/s390x/cpu.c | 24
target/s390x/cpu.h | 7 ++-
target/s390x/kvm.c | 8
target/s390x/sigp.c| 38 +++---
9 files changed, 77 insertions(+), 44 deletions(-)
diff --git a/cpus.c b/cpus.c
index f298b65..6006931 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2100,6 +2100,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
#elif defined(TARGET_TRICORE)
TriCoreCPU *tricore_cpu = TRICORE_CPU(cpu);
CPUTriCoreState *env = _cpu->env;
+#elif defined(TARGET_S390X)
+S390CPU *s390_cpu = S390_CPU(cpu);
+CPUS390XState *env = _cpu->env;
#endif
cpu_synchronize_state(cpu);
@@ -2127,6 +2130,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
#elif defined(TARGET_TRICORE)
info->value->arch = CPU_INFO_ARCH_TRICORE;
info->value->u.tricore.PC = env->PC;
+#elif defined(TARGET_S390X)
+info->value->arch = CPU_INFO_ARCH_S390;
+info->value->u.s390.cpu_state = env->cpu_state;
#else
info->value->arch = CPU_INFO_ARCH_OTHER;
#endif
diff --git a/hmp.c b/hmp.c
index 7870d6a..a6b94b7 100644
--- a/hmp.c
+++ b/hmp.c
@@ -392,6 +392,10 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
case CPU_INFO_ARCH_TRICORE:
monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore.PC);
break;
+case CPU_INFO_ARCH_S390:
+monitor_printf(mon, " state=%s",
+ CpuS390State_str(cpu->value->u.s390.cpu_state));
+break;
default:
break;
}
diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
index a85a149..5f8168f 100644
--- a/hw/intc/s390_flic.c
+++ b/hw/intc/s390_flic.c
@@ -192,8 +192,8 @@ static void qemu_s390_flic_notify(uint32_t type)
cs->interrupt_request |= CPU_INTERRUPT_HARD;
/* ignore CPUs that are not sleeping */
-if (s390_cpu_get_state(cpu) != CPU_STATE_OPERATING &&
-s390_cpu_get_state(cpu) != CPU_STATE_LOAD) {
+if (s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING &&
+s390_cpu_get_state(cpu) != S390_CPU_STATE_LOAD) {
continue;
}
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 4abbe89..4d0c3de 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -368,7 +368,7 @@ static void s390_machine_reset(void)
/* all cpus are stopped - configure and start the ipl cpu only */
s390_ipl_prepare_cpu(ipl_cpu);
-s390_cpu_set_state(CPU_STATE_OPERATING, ipl_cpu);
+s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu);
}
static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
diff --git a/qapi-schema.json b/qapi-schema.json
index 5c06745..66e0927 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -410,10 +410,12 @@
# An enumeration of cpu types that enable additional information during
# @query-cpus.
#
+# @s390: since 2.12
+#
# Since: 2.6
##
{ 'enum': 'CpuInfoArch',
- 'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 'other' ] }
+ 'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 's390', 'other' ] }
##
# @CpuInfo:
@@ -452,6 +454,7 @@
'ppc': 'CpuInfoPPC',
'mips': 'CpuInfoMIPS',
'tricore': 'CpuInfoTricore',
+'s390': 'CpuInfoS390',
'other': 'CpuInfoOther' } }
##
@@ -522,6 +525,29 @@
{ 'struct': 'CpuInfoOther', 'data': { } }
##
+# @CpuS390State:
+#
+# An enumeration of cpu states that can be assumed by a virtual
+# S390 CPU
+#
+# Since: 2.12
+
[Qemu-devel] [PATCH 0/3] add query-cpu-fast and related s390 changes
This series consolidates patches around a performance issue caused by the usage of QMP query-cpus. A performance issue was found in an OpenStack environment, where ceilometer was collecting domain statistics with libvirt. The domain statistics reported by libvirt include the vCPU halted state, which in turn is retrieved with QMP query-cpus. This causes two issues: 1. Performance: on most architectures query-cpus needs to issue a KVM ioctl to find out whether a vCPU was halted. This is not the case for s390 but query-cpus is always causing the vCPU to exit the VM. 2. Semantics: on x86 and other architectures, halted is a highly transient state, which is likely to have already changed shortly after the state information has been retrieved. This is not the case for s390, where halted is an indication that the vCPU is stopped, meaning its not available to the guest operating system until it has been restarted. The following patches help to alleviate the issues: Patch 1/3: Adds architecture specific data to the QMP CpuInfo type, exposing the existing s390 cpu-state in QMP. The cpu-state is a representation more adequate than the ambiguous 'halted' condition. Changes since original v2: - fixed cpu-state usage in hw/intc/s390_flic.c, necessary because master was updated in the meantime - removed superfluous newline while printing cpu-state Patch 2/3: Adds a new QMP function query-cpus-fast, which will only retrieve vCPU information that can be obtained without interrupting the vCPUs of a running VM. It introduces a new return type CpuInfoFast with the subset of fields meeting this condition. Specifically, the halted state is not part of CpuInfoFast. QMP clients like libvirt are encouraged to switch to the new API for vCPU information. Changes since original v2: - dropped optional halted state from CpuInfoFast Patch 3/3: Adds the s390-specific cpu state to CpuInfoFast, allowing management apps to find out whether a vCPU is in the stopped state. This extension leads to a partial duplication of field definitions from CpuInfo to CpuInfoFast. This should be tolerable if CpuInfo is deprecated and eventually removed. Luiz Capitulino (1): qmp: add query-cpus-fast Viktor Mihajlovski (2): qmp: expose s390-specific CPU info qmp: add architecture specific cpu data for query-cpus-fast cpus.c | 54 hmp-commands-info.hx | 14 ++ hmp.c | 33 + hmp.h | 1 + hw/intc/s390_flic.c| 4 +- hw/s390x/s390-virtio-ccw.c | 2 +- qapi-schema.json | 121 - target/s390x/cpu.c | 24 - target/s390x/cpu.h | 7 +-- target/s390x/kvm.c | 8 +-- target/s390x/sigp.c| 38 +++--- 11 files changed, 262 insertions(+), 44 deletions(-) -- 1.9.1
[Qemu-devel] [PATCH 3/3] qmp: add architecture specific cpu data for query-cpus-fast
The s390 CPU state can be retrieved without interrupting the
VM execution. Extendend the CpuInfoFast union with architecture
specific data and an implementation for s390.
Return data looks like this:
[
{"thread-id":64301,"props":{"core-id":0},
"arch":"s390","cpu-state":"operating",
"qom-path":"/machine/unattached/device[0]","cpu-index":0},
{"thread-id":64302,"props":{"core-id":1},
"arch":"s390","cpu-state":"operating",
"qom-path":"/machine/unattached/device[1]","cpu-index":1}
]
Currently there's a certain amount of duplication between
the definitions of CpuInfo and CpuInfoFast, both in the
base and variable areas, since there are data fields common
to the slow and fast variants.
A suggestion was made on the mailing list to enhance the QAPI
code generation to support two layers of unions. This would
allow to specify the common fields once and avoid the duplication
in the leaf unions.
On the other hand, the slow query-cpus should be deprecated
along with the slow CpuInfo type and eventually be removed.
Assuming that new architectures will not be added at high
rates, we could live with the duplication for the time being.
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
cpus.c | 10 ++
hmp.c| 8
qapi-schema.json | 35 +--
3 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/cpus.c b/cpus.c
index 6df6660..af67826 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2166,6 +2166,10 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
MachineClass *mc = MACHINE_GET_CLASS(ms);
CpuInfoFastList *head = NULL, *cur_item = NULL;
CPUState *cpu;
+#if defined(TARGET_S390X)
+S390CPU *s390_cpu;
+CPUS390XState *env;
+#endif
CPU_FOREACH(cpu) {
CpuInfoFastList *info = g_malloc0(sizeof(*info));
@@ -2183,6 +2187,12 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
info->value->props = props;
}
+#if defined(TARGET_S390X)
+s390_cpu = S390_CPU(cpu);
+env = _cpu->env;
+info->value->arch = CPU_INFO_ARCH_S390;
+info->value->u.s390.cpu_state = env->cpu_state;
+#endif
if (!cur_item) {
head = cur_item = info;
} else {
diff --git a/hmp.c b/hmp.c
index 598bfe5..94bfc7d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -425,6 +425,14 @@ void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
monitor_printf(mon, "CPU%" PRId64 "\n", cpu->value->cpu_index);
monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
monitor_printf(mon, " qom-path=%s\n", cpu->value->qom_path);
+switch (cpu->value->arch) {
+case CPU_INFO_ARCH_S390:
+monitor_printf(mon, " state=%s\n",
+ CpuS390State_str(cpu->value->u.s390.cpu_state));
+break;
+default:
+break;
+}
monitor_printf(mon, "\n");
}
diff --git a/qapi-schema.json b/qapi-schema.json
index 2a614af..a681d83 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -537,15 +537,26 @@
'data': [ 'uninitialized', 'stopped', 'check-stop', 'operating', 'load' ] }
##
-# @CpuInfoS390:
+# @CpuInfoS390Fast:
#
-# Additional information about a virtual S390 CPU
+# Additional information about a virtual S390 CPU which can be
+# obtained without a performance penalty for a running VM
#
# @cpu-state: the virtual CPU's state
#
# Since: 2.12
##
-{ 'struct': 'CpuInfoS390', 'data': { 'cpu-state': 'CpuS390State' } }
+{ 'struct': 'CpuInfoS390Fast', 'data': { 'cpu-state': 'CpuS390State' } }
+
+##
+# @CpuInfoS390:
+#
+# Additional information about a virtual S390 CPU, potentially expensive
+# to obtain
+#
+# Since: 2.12
+##
+{ 'struct': 'CpuInfoS390', 'base': 'CpuInfoS390Fast', 'data': { } }
##
# @query-cpus:
@@ -604,12 +615,24 @@
# @props: properties describing to which node/socket/core/thread
# virtual CPU belongs to, provided if supported by board
#
+# @arch: architecture of the cpu, which determines which additional fields
+#will be listed
+#
# Since: 2.12
#
##
-{ 'struct': 'CpuInfoFast',
- 'data': {'cpu-index': 'int', 'qom-path': 'str',
- 'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
+{ 'union': 'CpuInfoFast',
+ 'base': {'cpu-index': 'int', 'qom-path': 'str',
+ 'thread-id': 'int', '*props': 'CpuInstanceProperties',
+ 'arch': 'CpuInfoArch' },
+ 'discriminator': 'arch',
+ 'data': { 'x86': 'CpuInfoOther',
+'sparc': 'CpuInfoOther',
+'ppc': 'CpuInfoOther',
+'mips': 'CpuInfoOther',
+'tricore': 'CpuInfoOther',
+'s390': 'CpuInfoS390Fast',
+'other': 'CpuInfoOther' } }
##
# @query-cpus-fast:
--
1.9.1
[Qemu-devel] [PATCH 2/3] qmp: add query-cpus-fast
From: Luiz Capitulino <lcapitul...@redhat.com>
The query-cpus command has an extremely serious side effect:
it always interrupts all running vCPUs so that they can run
ioctl calls. This can cause a huge performance degradation for
some workloads. And most of the information retrieved by the
ioctl calls are not even used by query-cpus.
This commit introduces a replacement for query-cpus called
query-cpus-fast, which has the following features:
o Never interrupt vCPUs threads. query-cpus-fast only returns
vCPU information maintained by QEMU itself, which should be
sufficient for most management software needs
o Make "halted" field optional: we only return it if the
halted state is maintained by QEMU. But this also gives
the option of dropping the field in the future (see below)
o Drop irrelevant fields such as "current", "pc" and "arch"
o Drop field "halted" since it can't be provided fast reliably
and is too volatile on most architectures to be really useful
o Rename some fields for better clarification & proper naming
standard
Signed-off-by: Luiz Capitulino <lcapitul...@redhat.com>
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
cpus.c | 38
hmp-commands-info.hx | 14 +++
hmp.c| 21
hmp.h| 1 +
qapi-schema.json | 70
5 files changed, 144 insertions(+)
diff --git a/cpus.c b/cpus.c
index 6006931..6df6660 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2156,6 +2156,44 @@ CpuInfoList *qmp_query_cpus(Error **errp)
return head;
}
+/*
+ * fast means: we NEVER interrupt vCPU threads to retrieve
+ * information from KVM.
+ */
+CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
+{
+MachineState *ms = MACHINE(qdev_get_machine());
+MachineClass *mc = MACHINE_GET_CLASS(ms);
+CpuInfoFastList *head = NULL, *cur_item = NULL;
+CPUState *cpu;
+
+CPU_FOREACH(cpu) {
+CpuInfoFastList *info = g_malloc0(sizeof(*info));
+info->value = g_malloc0(sizeof(*info->value));
+
+info->value->cpu_index = cpu->cpu_index;
+info->value->qom_path = object_get_canonical_path(OBJECT(cpu));
+info->value->thread_id = cpu->thread_id;
+
+info->value->has_props = !!mc->cpu_index_to_instance_props;
+if (info->value->has_props) {
+CpuInstanceProperties *props;
+props = g_malloc0(sizeof(*props));
+*props = mc->cpu_index_to_instance_props(ms, cpu->cpu_index);
+info->value->props = props;
+}
+
+if (!cur_item) {
+head = cur_item = info;
+} else {
+cur_item->next = info;
+cur_item = info;
+}
+}
+
+return head;
+}
+
void qmp_memsave(int64_t addr, int64_t size, const char *filename,
bool has_cpu, int64_t cpu_index, Error **errp)
{
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index ad590a4..16ac602 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -160,6 +160,20 @@ Show infos for each CPU.
ETEXI
{
+.name = "cpus_fast",
+.args_type = "",
+.params = "",
+.help = "show information for each CPU without interrupting
them",
+.cmd= hmp_info_cpus_fast,
+},
+
+STEXI
+@item info cpus_fast
+@findex info cpus_fast
+Show infos for each CPU without performance penalty.
+ETEXI
+
+{
.name = "history",
.args_type = "",
.params = "",
diff --git a/hmp.c b/hmp.c
index a6b94b7..598bfe5 100644
--- a/hmp.c
+++ b/hmp.c
@@ -410,6 +410,27 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
qapi_free_CpuInfoList(cpu_list);
}
+void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
+{
+CpuInfoFastList *head, *cpu;
+TargetInfo *target;
+
+target = qmp_query_target(NULL);
+monitor_printf(mon, "CPU architecture is '%s'\n\n", target->arch);
+qapi_free_TargetInfo(target);
+
+head = qmp_query_cpus_fast(NULL);
+
+for (cpu = head; cpu; cpu = cpu->next) {
+monitor_printf(mon, "CPU%" PRId64 "\n", cpu->value->cpu_index);
+monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
+monitor_printf(mon, " qom-path=%s\n", cpu->value->qom_path);
+monitor_printf(mon, "\n");
+}
+
+qapi_free_CpuInfoFastList(head);
+}
+
static void print_block_info(Monitor *mon, BlockInfo *info,
BlockDeviceInfo *inserted, bool verbose)
{
diff --git a/hmp.h b/hmp.h
index 1143db4..93fb4e4 100644
--- a/hmp.h
+++ b/hmp.h
@@ -29,6 +29,7 @@ void
Re: [Qemu-devel] [PATCH v2] qmp: add query-cpus-fast
On 09.02.2018 15:27, Eduardo Habkost wrote: [...] >> I'm keeping it mainly for s390. Viktor, libvirt is still using >> this field in s390, no? >> >> Dropping halted and having management software still using query-cpus >> because of halted would be a total failure of query-cpus-fast. > > If I understood correctly, the CpuInfoS390::cpu_state field added > by Viktor in another patch[1] would replace "halted" for the s390 > case. Right, CPUState.halted is derived from CPUS390XState.cpu_state on s390: A cpu_state of CPU_STATE_STOPPED or CPU_STATE_CHECK_STOPPED results in halted = true. This derivation can be done by libvirt for s390. > > I'm assuming QEMU will be able to return that field without > interrupting the VCPUs. Viktor, is that correct? > > [1] https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02032.html > That's correct. >> >>>> Also, the code that sets/clears cpu->halted is target-specific, >>>> so I wouldn't be so sure that simply checking for >>>> !kvm_irqchip_in_kernel() is enough on all targets. >> >> I checked the code and had the impression it was enough, but >> I don't have experience with other archs. So, would be nice >> if other archs maintainers could review this. I'll try to ping them. > > I think we need to take a step back and rethink: > > 1) What the field is supposed to mean? The semantics of "halted" >are completely unclear. What exactly we want to communicate >to libvirt/management? > 2) On which cases the information (whatever it means) is really >useful/important? If you are excluding cases with in-kernel >irqchip, you are already excluding most users. > > Given that nobody (including myself) sees a need for halted we can remove it for the fast version of query-cpus without surprising anyone. [...] -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [PATCH 1/3] qmp: expose s390-specific CPU info
On 12.02.2018 16:52, Cornelia Huck wrote:
> On Mon, 12 Feb 2018 13:14:30 +0100
> Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com> wrote:
>
>> Presently s390x is the only architecture not exposing specific
>> CPU information via QMP query-cpus. Upstream discussion has shown
>> that it could make sense to report the architecture specific CPU
>> state, e.g. to detect that a CPU has been stopped.
>>
>> With this change the output of query-cpus will look like this on
>> s390:
>>
>>[
>> {"arch": "s390", "current": true,
>> "props": {"core-id": 0}, "cpu-state": "operating", "CPU": 0,
>> "qom_path": "/machine/unattached/device[0]",
>> "halted": false, "thread_id": 63115},
>> {"arch": "s390", "current": false,
>> "props": {"core-id": 1}, "cpu-state": "stopped", "CPU": 1,
>> "qom_path": "/machine/unattached/device[1]",
>> "halted": true, "thread_id": 63116}
>>]
>>
>> Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
>> Acked-by: Eric Blake <ebl...@redhat.com>
>> ---
>> cpus.c | 6 ++
>> hmp.c | 4
>> hw/intc/s390_flic.c| 4 ++--
>> hw/s390x/s390-virtio-ccw.c | 2 +-
>> qapi-schema.json | 28 +++-
>> target/s390x/cpu.c | 24
>> target/s390x/cpu.h | 7 ++-
>> target/s390x/kvm.c | 8
>> target/s390x/sigp.c| 38 +++---
>> 9 files changed, 77 insertions(+), 44 deletions(-)
>
> Patch looks good to me. I presume this should go through the s390 tree?
> Or do we want someone to pick up the whole series?
>
The main reason for adding this patch to the series is to ensure
everything is applied in proper order. This patch can stand for itself,
but it must be applied before 3/3.
Valid orders would be 1 - 2 - 3 or 2 - 1 - 3.
As long as this is observed, I'm fine with either way.
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [PATCH 0/3] add query-cpu-fast and related s390 changes
On 12.02.2018 16:38, Cornelia Huck wrote: > On Mon, 12 Feb 2018 13:14:29 +0100 > Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com> wrote: > >> This series consolidates patches around a performance issue >> caused by the usage of QMP query-cpus. > > Thank you for consolidating this; it was a bit hard to follow the > different discussions. > >> >> A performance issue was found in an OpenStack environment, where >> ceilometer was collecting domain statistics with libvirt. The domain >> statistics reported by libvirt include the vCPU halted state, which >> in turn is retrieved with QMP query-cpus. >> >> This causes two issues: >> 1. Performance: on most architectures query-cpus needs to issue a KVM ioctl >>to find out whether a vCPU was halted. This is not the case for s390 >>but query-cpus is always causing the vCPU to exit the VM. >> >> 2. Semantics: on x86 and other architectures, halted is a highly transient >>state, which is likely to have already changed shortly after the state >>information has been retrieved. This is not the case for s390, where >>halted is an indication that the vCPU is stopped, meaning its not >>available to the guest operating system until it has been restarted. >> >> The following patches help to alleviate the issues: >> >> Patch 1/3: >> Adds architecture specific data to the QMP CpuInfo type, exposing >> the existing s390 cpu-state in QMP. The cpu-state is a representation >> more adequate than the ambiguous 'halted' condition. >> >> Changes since original v2: >> - fixed cpu-state usage in hw/intc/s390_flic.c, necessary because >> master was updated in the meantime >> - removed superfluous newline while printing cpu-state > > So this is adding s390x info in query-cpus (presumably for legacy > callers?)... > Yeah, as long as we have the slow call, the fast data should be in there as well. [...] > > ...this is adding the new query-cpus-fast... > [...] > > ...and this is adding the s390x state to query-cpus-fast, right? > Exactly. -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [PATCH 3/3] qmp: add architecture specific cpu data for query-cpus-fast
On 12.02.2018 19:15, Luiz Capitulino wrote:
> On Mon, 12 Feb 2018 13:14:32 +0100
> Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com> wrote:
>
>> -{ 'struct': 'CpuInfoFast',
>> - 'data': {'cpu-index': 'int', 'qom-path': 'str',
>> - 'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
>> +{ 'union': 'CpuInfoFast',
>> + 'base': {'cpu-index': 'int', 'qom-path': 'str',
>> + 'thread-id': 'int', '*props': 'CpuInstanceProperties',
>> + 'arch': 'CpuInfoArch' },
>> + 'discriminator': 'arch',
>> + 'data': { 'x86': 'CpuInfoOther',
>> +'sparc': 'CpuInfoOther',
>> +'ppc': 'CpuInfoOther',
>> +'mips': 'CpuInfoOther',
>> +'tricore': 'CpuInfoOther',
>> +'s390': 'CpuInfoS390Fast',
>> +'other': 'CpuInfoOther' } }
>
> Consider this a minor comment (and QMP maintainers can give a much
> better advice than me), but I think this arch list has problems. For
> one thing, it's incomplete. And the second problem is the 'other'
> field. What happens when QEMU starts supporting a new arch? 'other'
> becomes the new arch. Is this compatible? I don't know.
This seems to be the customary approach, see
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg01986.html
>
> I don't know if this would work with the QAPI, but you could have
> a '*arch-data' field in the CpuInfoFast definition, like:
>
> 'data': { ..., '*arch-data': 'CpuInfoFastArchData' }
>
> where 'CpuInfoFastArchData' is defined by each arch that supports
> the field. An arch supporting the field could also export a
> query_cpus_fast_arch() function, which is called by qmp_query_cpus_fast().
>
I had it like this in my first reply to your initial patch. However,
that adds an additional hierarchy level in the return data. This
prevents clients like libvirt to reuse the data extraction code when
they switch over to using query-cpus-fast.
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [qemu-s390x] [PATCH 1/3] qmp: expose s390-specific CPU info
On 13.02.2018 12:16, David Hildenbrand wrote:
> On 12.02.2018 19:03, Luiz Capitulino wrote:
>> On Mon, 12 Feb 2018 13:14:30 +0100
>> Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com> wrote:
>>
>>> Presently s390x is the only architecture not exposing specific
>>> CPU information via QMP query-cpus. Upstream discussion has shown
>>> that it could make sense to report the architecture specific CPU
>>> state, e.g. to detect that a CPU has been stopped.
>>>
>>> With this change the output of query-cpus will look like this on
>>> s390:
>>>
>>>[
>>> {"arch": "s390", "current": true,
>>> "props": {"core-id": 0}, "cpu-state": "operating", "CPU": 0,
>>> "qom_path": "/machine/unattached/device[0]",
>>> "halted": false, "thread_id": 63115},
>>> {"arch": "s390", "current": false,
>>> "props": {"core-id": 1}, "cpu-state": "stopped", "CPU": 1,
>>> "qom_path": "/machine/unattached/device[1]",
>>> "halted": true, "thread_id": 63116}
>>>]
>>
>> We're adding the same information to query-cpus-fast. Why do we
>> need to duplicate this into query-cpus? Do you plan to keep using
>> query-cpus? If yes, why?
>
> Wonder if we could simply pass a flag to query-cpus "fast=true", that
> makes it behave differently. (either not indicate the critical values or
> simply provide dummy values - e.g. simply halted=false)
>
That was one of the ideas in the earlier stages of this discussion (and
I was inclined to go that way initially). The major drawback of this
approach that the slow call is hard to deprecate (OK, one could change
the default to fast=true over time). It's easier to deprecate the entire
query-cpus function. The other issue, maybe not as bad, is that one has
to deal with fields that are suddenly optional or obsolete in way not
confusing every one.
Bottom line is that I'm convinced it's better to have both APIs and to
deprecate the slow one over time. But I have to confess I'm not familiar
with QAPI deprecation rules, maybe Eric can shed some light on this...
>>
>> Libvirt for one, should move away from it. We don't want to run
>> into the risk of having the same issue we had with x86 in other
>> archs.
>>
>>>
>
>
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [PATCH 2/3] qmp: add query-cpus-fast
On 12.02.2018 17:50, Dr. David Alan Gilbert wrote:
[...]
>> diff --git a/hmp.c b/hmp.c
>> index a6b94b7..598bfe5 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -410,6 +410,27 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
>> qapi_free_CpuInfoList(cpu_list);
>> }
>>
>> +void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
>> +{
>> +CpuInfoFastList *head, *cpu;
>> +TargetInfo *target;
>> +
>> +target = qmp_query_target(NULL);
>> +monitor_printf(mon, "CPU architecture is '%s'\n\n", target->arch);
>> +qapi_free_TargetInfo(target);
>> +
>> +head = qmp_query_cpus_fast(NULL);
>> +
>> +for (cpu = head; cpu; cpu = cpu->next) {
>> +monitor_printf(mon, "CPU%" PRId64 "\n", cpu->value->cpu_index);
>> +monitor_printf(mon, " thread-id=%" PRId64 "\n",
>> cpu->value->thread_id);
>> +monitor_printf(mon, " qom-path=%s\n", cpu->value->qom_path);
>> +monitor_printf(mon, "\n");
>> +}
I started some prototyping in libvirt and stumbled over the changed
layout of "info cpus_fast" vs. "info cpus". IMHO it would be better to
stick with the original format (one line per CPU). I'll post a v2.
>> +
>> +qapi_free_CpuInfoFastList(head);
>> +}
>> +
>> static void print_block_info(Monitor *mon, BlockInfo *info,
>> BlockDeviceInfo *inserted, bool verbose)
>> {
>> diff --git a/hmp.h b/hmp.h
>> index 1143db4..93fb4e4 100644
>> --- a/hmp.h
>> +++ b/hmp.h
>> @@ -29,6 +29,7 @@ void hmp_info_migrate_capabilities(Monitor *mon, const
>> QDict *qdict);
>> void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict);
>> void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict);
>> void hmp_info_cpus(Monitor *mon, const QDict *qdict);
>> +void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict);
>> void hmp_info_block(Monitor *mon, const QDict *qdict);
>> void hmp_info_blockstats(Monitor *mon, const QDict *qdict);
>> void hmp_info_vnc(Monitor *mon, const QDict *qdict);
>
> For HMP:
>
> Acked-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
>
[...]
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [PATCH 3/3] qmp: add architecture specific cpu data for query-cpus-fast
On 12.02.2018 17:23, Cornelia Huck wrote:
[...]
>> diff --git a/cpus.c b/cpus.c
>> index 6df6660..af67826 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -2166,6 +2166,10 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
>> MachineClass *mc = MACHINE_GET_CLASS(ms);
>> CpuInfoFastList *head = NULL, *cur_item = NULL;
>> CPUState *cpu;
>> +#if defined(TARGET_S390X)
>> +S390CPU *s390_cpu;
>> +CPUS390XState *env;
>> +#endif
>>
>> CPU_FOREACH(cpu) {
>> CpuInfoFastList *info = g_malloc0(sizeof(*info));
>> @@ -2183,6 +2187,12 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
>> info->value->props = props;
>> }
>>
>> +#if defined(TARGET_S390X)
>> +s390_cpu = S390_CPU(cpu);
>> +env = _cpu->env;
>
> You should be able to omit the s390_cpu variable by using
>
>env = _CPU(cpu)->env;
>
True, but I wanted to stay in style with the code in qmp_query_cpus.
>> +info->value->arch = CPU_INFO_ARCH_S390;
>> +info->value->u.s390.cpu_state = env->cpu_state;
>> +#endif
>> if (!cur_item) {
>> head = cur_item = info;
>> } else {
>
> As you mentioned in the patch description, the duplication is a bit
> awkward. I'll let the QAPI experts judge that; otherwise, this looks
> fine to me.
>
--
Regards,
Viktor Mihajlovski
[Qemu-devel] [PATCHv2 0/3] add query-cpu-fast and related s390 changes
This series consolidates patches around a performance issue caused by the usage of QMP query-cpus. A performance issue was found in an OpenStack environment, where ceilometer was collecting domain statistics with libvirt. The domain statistics reported by libvirt include the vCPU halted state, which in turn is retrieved with QMP query-cpus. This causes two issues: 1. Performance: on most architectures query-cpus needs to issue a KVM ioctl to find out whether a vCPU was halted. This is not the case for s390 but query-cpus is always causing the vCPU to exit the VM. 2. Semantics: on x86 and other architectures, halted is a highly transient state, which is likely to have already changed shortly after the state information has been retrieved. This is not the case for s390, where halted is an indication that the vCPU is stopped, meaning its not available to the guest operating system until it has been restarted. The following patches help to alleviate the issues: Patch 1/3: Adds architecture specific data to the QMP CpuInfo type, exposing the existing s390 cpu-state in QMP. The cpu-state is a representation more adequate than the ambiguous 'halted' condition. Patch 2/3: Adds a new QMP function query-cpus-fast, which will only retrieve vCPU information that can be obtained without interrupting the vCPUs of a running VM. It introduces a new return type CpuInfoFast with the subset of fields meeting this condition. Specifically, the halted state is not part of CpuInfoFast. QMP clients like libvirt are encouraged to switch to the new API for vCPU information. Patch 3/3: Adds the s390-specific cpu state to CpuInfoFast, allowing management apps to find out whether a vCPU is in the stopped state. This extension leads to a partial duplication of field definitions from CpuInfo to CpuInfoFast. This should be tolerable if CpuInfo is deprecated and eventually removed. Series v1 -> v2: Patch 2/3: o Changed formatting of hmp info cpus_fast to match that of info cpus. This makes it easier for clients to switch to the fast call. Patch 3/3: o Same formatting change for info cpus_fast as in 2/3, only for s390-specific cpu state. Luiz Capitulino (1): qmp: add query-cpus-fast Viktor Mihajlovski (2): qmp: expose s390-specific CPU info qmp: add architecture specific cpu data for query-cpus-fast cpus.c | 54 hmp-commands-info.hx | 14 ++ hmp.c | 28 +++ hmp.h | 1 + hw/intc/s390_flic.c| 4 +- hw/s390x/s390-virtio-ccw.c | 2 +- qapi-schema.json | 121 - target/s390x/cpu.c | 24 - target/s390x/cpu.h | 7 +-- target/s390x/kvm.c | 8 +-- target/s390x/sigp.c| 38 +++--- 11 files changed, 257 insertions(+), 44 deletions(-) -- 1.9.1
[Qemu-devel] [PATCHv2 2/3] qmp: add query-cpus-fast
From: Luiz Capitulino <lcapitul...@redhat.com>
The query-cpus command has an extremely serious side effect:
it always interrupts all running vCPUs so that they can run
ioctl calls. This can cause a huge performance degradation for
some workloads. And most of the information retrieved by the
ioctl calls are not even used by query-cpus.
This commit introduces a replacement for query-cpus called
query-cpus-fast, which has the following features:
o Never interrupt vCPUs threads. query-cpus-fast only returns
vCPU information maintained by QEMU itself, which should be
sufficient for most management software needs
o Make "halted" field optional: we only return it if the
halted state is maintained by QEMU. But this also gives
the option of dropping the field in the future (see below)
o Drop irrelevant fields such as "current", "pc", "arch"
and "halted"
o Rename some fields for better clarification & proper naming
standard
Signed-off-by: Luiz Capitulino <lcapitul...@redhat.com>
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
cpus.c | 38
hmp-commands-info.hx | 14 +++
hmp.c| 14 +++
hmp.h| 1 +
qapi-schema.json | 70
5 files changed, 137 insertions(+)
diff --git a/cpus.c b/cpus.c
index 6006931..6df6660 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2156,6 +2156,44 @@ CpuInfoList *qmp_query_cpus(Error **errp)
return head;
}
+/*
+ * fast means: we NEVER interrupt vCPU threads to retrieve
+ * information from KVM.
+ */
+CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
+{
+MachineState *ms = MACHINE(qdev_get_machine());
+MachineClass *mc = MACHINE_GET_CLASS(ms);
+CpuInfoFastList *head = NULL, *cur_item = NULL;
+CPUState *cpu;
+
+CPU_FOREACH(cpu) {
+CpuInfoFastList *info = g_malloc0(sizeof(*info));
+info->value = g_malloc0(sizeof(*info->value));
+
+info->value->cpu_index = cpu->cpu_index;
+info->value->qom_path = object_get_canonical_path(OBJECT(cpu));
+info->value->thread_id = cpu->thread_id;
+
+info->value->has_props = !!mc->cpu_index_to_instance_props;
+if (info->value->has_props) {
+CpuInstanceProperties *props;
+props = g_malloc0(sizeof(*props));
+*props = mc->cpu_index_to_instance_props(ms, cpu->cpu_index);
+info->value->props = props;
+}
+
+if (!cur_item) {
+head = cur_item = info;
+} else {
+cur_item->next = info;
+cur_item = info;
+}
+}
+
+return head;
+}
+
void qmp_memsave(int64_t addr, int64_t size, const char *filename,
bool has_cpu, int64_t cpu_index, Error **errp)
{
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index ad590a4..16ac602 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -160,6 +160,20 @@ Show infos for each CPU.
ETEXI
{
+.name = "cpus_fast",
+.args_type = "",
+.params = "",
+.help = "show information for each CPU without interrupting
them",
+.cmd= hmp_info_cpus_fast,
+},
+
+STEXI
+@item info cpus_fast
+@findex info cpus_fast
+Show infos for each CPU without performance penalty.
+ETEXI
+
+{
.name = "history",
.args_type = "",
.params = "",
diff --git a/hmp.c b/hmp.c
index a6b94b7..0bd3b3a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -410,6 +410,20 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
qapi_free_CpuInfoList(cpu_list);
}
+void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
+{
+CpuInfoFastList *head, *cpu;
+
+head = qmp_query_cpus_fast(NULL);
+
+for (cpu = head; cpu; cpu = cpu->next) {
+monitor_printf(mon, " CPU #%" PRId64 ":", cpu->value->cpu_index);
+monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
+}
+
+qapi_free_CpuInfoFastList(head);
+}
+
static void print_block_info(Monitor *mon, BlockInfo *info,
BlockDeviceInfo *inserted, bool verbose)
{
diff --git a/hmp.h b/hmp.h
index 1143db4..93fb4e4 100644
--- a/hmp.h
+++ b/hmp.h
@@ -29,6 +29,7 @@ void hmp_info_migrate_capabilities(Monitor *mon, const QDict
*qdict);
void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict);
void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict);
void hmp_info_cpus(Monitor *mon, const QDict *qdict);
+void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict);
void hmp_info_block(Monitor *mon, const QDict *qdict);
void hmp_info_blockstats(Monitor *mon, const QDict *qdict);
[Qemu-devel] [PATCHv2 1/3] qmp: expose s390-specific CPU info
Presently s390x is the only architecture not exposing specific
CPU information via QMP query-cpus. Upstream discussion has shown
that it could make sense to report the architecture specific CPU
state, e.g. to detect that a CPU has been stopped.
With this change the output of query-cpus will look like this on
s390:
[
{"arch": "s390", "current": true,
"props": {"core-id": 0}, "cpu-state": "operating", "CPU": 0,
"qom_path": "/machine/unattached/device[0]",
"halted": false, "thread_id": 63115},
{"arch": "s390", "current": false,
"props": {"core-id": 1}, "cpu-state": "stopped", "CPU": 1,
"qom_path": "/machine/unattached/device[1]",
"halted": true, "thread_id": 63116}
]
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
Acked-by: Eric Blake <ebl...@redhat.com>
---
cpus.c | 6 ++
hmp.c | 4
hw/intc/s390_flic.c| 4 ++--
hw/s390x/s390-virtio-ccw.c | 2 +-
qapi-schema.json | 28 +++-
target/s390x/cpu.c | 24
target/s390x/cpu.h | 7 ++-
target/s390x/kvm.c | 8
target/s390x/sigp.c| 38 +++---
9 files changed, 77 insertions(+), 44 deletions(-)
diff --git a/cpus.c b/cpus.c
index f298b65..6006931 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2100,6 +2100,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
#elif defined(TARGET_TRICORE)
TriCoreCPU *tricore_cpu = TRICORE_CPU(cpu);
CPUTriCoreState *env = _cpu->env;
+#elif defined(TARGET_S390X)
+S390CPU *s390_cpu = S390_CPU(cpu);
+CPUS390XState *env = _cpu->env;
#endif
cpu_synchronize_state(cpu);
@@ -2127,6 +2130,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
#elif defined(TARGET_TRICORE)
info->value->arch = CPU_INFO_ARCH_TRICORE;
info->value->u.tricore.PC = env->PC;
+#elif defined(TARGET_S390X)
+info->value->arch = CPU_INFO_ARCH_S390;
+info->value->u.s390.cpu_state = env->cpu_state;
#else
info->value->arch = CPU_INFO_ARCH_OTHER;
#endif
diff --git a/hmp.c b/hmp.c
index 7870d6a..a6b94b7 100644
--- a/hmp.c
+++ b/hmp.c
@@ -392,6 +392,10 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
case CPU_INFO_ARCH_TRICORE:
monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore.PC);
break;
+case CPU_INFO_ARCH_S390:
+monitor_printf(mon, " state=%s",
+ CpuS390State_str(cpu->value->u.s390.cpu_state));
+break;
default:
break;
}
diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
index a85a149..5f8168f 100644
--- a/hw/intc/s390_flic.c
+++ b/hw/intc/s390_flic.c
@@ -192,8 +192,8 @@ static void qemu_s390_flic_notify(uint32_t type)
cs->interrupt_request |= CPU_INTERRUPT_HARD;
/* ignore CPUs that are not sleeping */
-if (s390_cpu_get_state(cpu) != CPU_STATE_OPERATING &&
-s390_cpu_get_state(cpu) != CPU_STATE_LOAD) {
+if (s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING &&
+s390_cpu_get_state(cpu) != S390_CPU_STATE_LOAD) {
continue;
}
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 4abbe89..4d0c3de 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -368,7 +368,7 @@ static void s390_machine_reset(void)
/* all cpus are stopped - configure and start the ipl cpu only */
s390_ipl_prepare_cpu(ipl_cpu);
-s390_cpu_set_state(CPU_STATE_OPERATING, ipl_cpu);
+s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu);
}
static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
diff --git a/qapi-schema.json b/qapi-schema.json
index 0262b9f..94d560e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -410,10 +410,12 @@
# An enumeration of cpu types that enable additional information during
# @query-cpus.
#
+# @s390: since 2.12
+#
# Since: 2.6
##
{ 'enum': 'CpuInfoArch',
- 'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 'other' ] }
+ 'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 's390', 'other' ] }
##
# @CpuInfo:
@@ -452,6 +454,7 @@
'ppc': 'CpuInfoPPC',
'mips': 'CpuInfoMIPS',
'tricore': 'CpuInfoTricore',
+'s390': 'CpuInfoS390',
'other': 'CpuInfoOther' } }
##
@@ -522,6 +525,29 @@
{ 'struct': 'CpuInfoOther', 'data': { } }
##
+# @CpuS390State:
+#
+# An enumeration of cpu states that can be assumed by a virtual
+# S390 CPU
+#
+# Since: 2.12
+
[Qemu-devel] [PATCHv2 3/3] qmp: add architecture specific cpu data for query-cpus-fast
The s390 CPU state can be retrieved without interrupting the
VM execution. Extendend the CpuInfoFast union with architecture
specific data and an implementation for s390.
Return data looks like this:
[
{"thread-id":64301,"props":{"core-id":0},
"arch":"s390","cpu-state":"operating",
"qom-path":"/machine/unattached/device[0]","cpu-index":0},
{"thread-id":64302,"props":{"core-id":1},
"arch":"s390","cpu-state":"operating",
"qom-path":"/machine/unattached/device[1]","cpu-index":1}
]
Currently there's a certain amount of duplication between
the definitions of CpuInfo and CpuInfoFast, both in the
base and variable areas, since there are data fields common
to the slow and fast variants.
A suggestion was made on the mailing list to enhance the QAPI
code generation to support two layers of unions. This would
allow to specify the common fields once and avoid the duplication
in the leaf unions.
On the other hand, the slow query-cpus should be deprecated
along with the slow CpuInfo type and eventually be removed.
Assuming that new architectures will not be added at high
rates, we could live with the duplication for the time being.
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
cpus.c | 10 ++
hmp.c| 10 ++
qapi-schema.json | 35 +--
3 files changed, 49 insertions(+), 6 deletions(-)
diff --git a/cpus.c b/cpus.c
index 6df6660..af67826 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2166,6 +2166,10 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
MachineClass *mc = MACHINE_GET_CLASS(ms);
CpuInfoFastList *head = NULL, *cur_item = NULL;
CPUState *cpu;
+#if defined(TARGET_S390X)
+S390CPU *s390_cpu;
+CPUS390XState *env;
+#endif
CPU_FOREACH(cpu) {
CpuInfoFastList *info = g_malloc0(sizeof(*info));
@@ -2183,6 +2187,12 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
info->value->props = props;
}
+#if defined(TARGET_S390X)
+s390_cpu = S390_CPU(cpu);
+env = _cpu->env;
+info->value->arch = CPU_INFO_ARCH_S390;
+info->value->u.s390.cpu_state = env->cpu_state;
+#endif
if (!cur_item) {
head = cur_item = info;
} else {
diff --git a/hmp.c b/hmp.c
index 0bd3b3a..e27433e 100644
--- a/hmp.c
+++ b/hmp.c
@@ -418,6 +418,16 @@ void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
for (cpu = head; cpu; cpu = cpu->next) {
monitor_printf(mon, " CPU #%" PRId64 ":", cpu->value->cpu_index);
+
+switch (cpu->value->arch) {
+case CPU_INFO_ARCH_S390:
+monitor_printf(mon, " state=%s",
+ CpuS390State_str(cpu->value->u.s390.cpu_state));
+break;
+default:
+break;
+}
+
monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
}
diff --git a/qapi-schema.json b/qapi-schema.json
index dc07729..b71f8cc 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -537,15 +537,26 @@
'data': [ 'uninitialized', 'stopped', 'check-stop', 'operating', 'load' ] }
##
-# @CpuInfoS390:
+# @CpuInfoS390Fast:
#
-# Additional information about a virtual S390 CPU
+# Additional information about a virtual S390 CPU which can be
+# obtained without a performance penalty for a running VM
#
# @cpu-state: the virtual CPU's state
#
# Since: 2.12
##
-{ 'struct': 'CpuInfoS390', 'data': { 'cpu-state': 'CpuS390State' } }
+{ 'struct': 'CpuInfoS390Fast', 'data': { 'cpu-state': 'CpuS390State' } }
+
+##
+# @CpuInfoS390:
+#
+# Additional information about a virtual S390 CPU, potentially expensive
+# to obtain
+#
+# Since: 2.12
+##
+{ 'struct': 'CpuInfoS390', 'base': 'CpuInfoS390Fast', 'data': { } }
##
# @query-cpus:
@@ -604,12 +615,24 @@
# @props: properties describing to which node/socket/core/thread
# virtual CPU belongs to, provided if supported by board
#
+# @arch: architecture of the cpu, which determines which additional fields
+#will be listed
+#
# Since: 2.12
#
##
-{ 'struct': 'CpuInfoFast',
- 'data': {'cpu-index': 'int', 'qom-path': 'str',
- 'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
+{ 'union': 'CpuInfoFast',
+ 'base': {'cpu-index': 'int', 'qom-path': 'str',
+ 'thread-id': 'int', '*props': 'CpuInstanceProperties',
+ 'arch': 'CpuInfoArch' },
+ 'discriminator': 'arch',
+ 'data': { 'x86': 'CpuInfoOther',
+'sparc': 'CpuInfoOther',
+'ppc': 'CpuInfoOther',
+'mips': 'CpuInfoOther',
+'tricore': 'CpuInfoOther',
+'s390': 'CpuInfoS390Fast',
+'other': 'CpuInfoOther' } }
##
# @query-cpus-fast:
--
1.9.1
Re: [Qemu-devel] [PATCHv2 0/3] add query-cpu-fast and related s390 changes
On 14.02.2018 11:57, Cornelia Huck wrote: [...] > > How shall we proceed with this series? Patch 3 depends upon patch 1, so > I think it makes sense to merge this in one go. > > I can give my R-b on patch 1 and Someone(tm) can merge this, or I can > take the whole series through the s390 tree (with some further > reviews/acks on patches 2/3). > Let's wait for the outstanding reviews (QAPI/HMP). If no major concerns are voiced, the series could go in via the s390 tree. As David H. pointed out, I've missed his r-b on patch 1, so you could add that along with Christian's if no v3 is required. -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [PATCHv2 3/3] qmp: add architecture specific cpu data for query-cpus-fast
On 14.02.2018 16:27, Cornelia Huck wrote: > On Wed, 14 Feb 2018 09:15:23 -0600 > Eric Blake <ebl...@redhat.com> wrote: > >> On 02/14/2018 04:15 AM, Cornelia Huck wrote: >>> On Tue, 13 Feb 2018 18:18:48 +0100 >>> Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com> wrote: >>> >> >>>> A suggestion was made on the mailing list to enhance the QAPI >>>> code generation to support two layers of unions. This would >>>> allow to specify the common fields once and avoid the duplication >>>> in the leaf unions. >>>> >>>> On the other hand, the slow query-cpus should be deprecated >>>> along with the slow CpuInfo type and eventually be removed. >>>> Assuming that new architectures will not be added at high >>>> rates, we could live with the duplication for the time being. >>> >>> What would be a realistic timeframe for deprecation/removal of >>> query-cpus, considering the libvirt usage? Are we aware of other users? >> >> Well, if we want to start the clock ticking, this series also needs to >> touch qemu-doc.texi to start the deprecation clock, then we go at least >> two more releases with support for both old and new commands. >> > > I'd like to know first what libvirt plans to do -- no sense in starting > deprecation if we're still stuck with it for a while. > FWIW, I'm currently preparing libvirt patches to use query-cpus-fast if available. I wouldn't see why it would take more than one libvirt release to get them in. The question might rather be, which combinations of qemu and libvirt are considered useful. E.g., I didn't upgrade libvirt in a while on my test system but am using bleeding edge QEMU. But that doesn't necessarily resemble a valid setup. -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [PATCH 1/2] qmp: add query-cpus-fast
On 08.02.2018 22:41, Eduardo Habkost wrote:
> On Thu, Feb 08, 2018 at 02:59:17PM -0600, Eric Blake wrote:
>> On 02/08/2018 01:59 PM, Eduardo Habkost wrote:
>>> On Wed, Feb 07, 2018 at 12:50:13PM -0500, Luiz Capitulino wrote:
>>>> The query-cpus command has an extremely serious side effect:
>>>> it always interrupt all running vCPUs so that they can run
>>>> ioctl calls. This can cause a huge performance degradation for
>>>> some workloads. And most of the information retrieved by the
>>>> ioctl calls are not even used by query-cpus.
>>>>
>>>> This commit introduces a replacement for query-cpus called
>>>> query-cpus-fast, which has the following features:
>>>>
>>
>>>> +# Notes: @halted is a transient state that changes frequently. By the
>>>> time the
>>>> +#data is sent to the client, the guest may no longer be halted.
>>>> +##
>>>> +{ 'struct': 'CpuInfo2',
>>>> + 'data': {'cpu-index': 'int', '*halted': 'bool', 'qom-path': 'str',
>>>> + 'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
>>>
>>> This will require duplicating struct fields every time we add a
>>> new field to query-cpus-fast (e.g. how would VIktor's
>>> CpuInfoS390State patch[1] look like if rebased on top of yours?).
>>>
>>> One way to avoid that is to use CpuInfo for both, and make all
>>> "slow" fields optional. Another option is to use QAPI
>>> inheritance, but it could be a little complicated if unions are
>>> involved?
>>
>> Inheritance is better than optional fields for the sake of introspection
>> learning which fields to expect.
>>
>> Put the common fields to both interfaces in the base class, then have the
>> slower (older) CpuInfo class extend the base class to add the additional
>> fields.
>>
>> Unions should be able to inherit just fine from structs (after all, a flat
>> union requires a struct base); but if we need two layers of unions, we'll
>> need to enhance QAPI code generation first.
>
> If we can't do union-union inheritance yet, maybe we can work around it this
> way:
>
> # fields that are always returned by both query-cpus and query-cpus-fast
> { 'struct': 'BothCpuInfoBase',
> 'data': {'cpu': 'int', 'qom_path': 'str', 'thread_id': 'int',
>'*props': 'CpuInstanceProperties' } }
>
> # fields that are always returned by query-cpus
> { 'struct': 'CpuInfoBase',
> 'base': 'BothCpuInfoBase',
> 'data': {'current': 'bool', 'halted': 'bool', 'arch': 'CpuInfoArch' } }
>
> # query-cpus return value
> { 'union': 'CpuInfo',
> 'base': 'CpuInfoBase',
> 'discriminator': 'arch',
> 'data': { 'x86': 'CpuInfoX86',
> 's390': 'CpuInfoS390',
> 'sparc': 'CpuInfoSPARC',
> 'ppc': 'CpuInfoPPC',
> 'mips': 'CpuInfoMIPS',
> 'tricore': 'CpuInfoTricore',
> 'other': 'CpuInfoOther' } }
>
> # fields that are always returned by query-cpus-fast
> { 'struct': 'FastCpuInfoBase',
> 'base': 'BothCpuInfoBase',
> 'data': { 'arch': 'CpuInfoArch' } }
>
> # return value of query-cpus-fast
> { 'union': 'FastCpuInfo',
> 'base': 'FastCpuInfoBase',
> 'discriminator': 'arch',
> 'data': { 'x86': 'CpuInfoOther',
> 's390': 'FastCpuInfoS390',
> 'sparc': 'CpuInfoOther',
> 'ppc': 'CpuInfoOther',
> 'mips': 'CpuInfoOther',
> 'tricore': 'CpuInfoOther',
> 'other': 'CpuInfoOther' } }
>
> # fields returned by both query-cpus and query-cpus-fast on s390
> { 'struct': 'FastCpuInfoS390',
> 'data': { 'fast_field': 'int' } }
>
> # fields returned by query-cpus on s390
> { 'struct': 'CpuInfoS390',
> 'base': 'FastCpuInfoS390',
> 'data': { 'slow_field': 'int' } }
>
This is not exactly pretty, but would provide the functionality (and
flexibility) I'd expect.
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [PATCH v2] qmp: add query-cpus-fast
On 09.02.2018 14:49, Luiz Capitulino wrote: > On Fri, 9 Feb 2018 08:56:19 +0100 > Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com> wrote: > >> On 08.02.2018 21:33, Eduardo Habkost wrote: >>> On Thu, Feb 08, 2018 at 11:17:32AM -0500, Luiz Capitulino wrote: >>> [...] >>>> The "halted" field is somewhat controversial. On the one hand, >>>> it offers a convenient way to know if a guest CPU is idle or >>>> running. On the other hand, it's a field that can change many >>>> times a second. In fact, the halted state can change even >>>> before query-cpus-fast has returned. This makes one wonder if >>>> this field should be dropped all together. Having the "halted" >>>> field as optional gives a better option for dropping it in >>>> the future, since we can just stop returning it. >>> >>> I'd just drop it, unless we find a use case where it's really >>> useful. > > I don't think there's any, unless for debugging purposes. > > I'm keeping it mainly for s390. Viktor, libvirt is still using > this field in s390, no? see below > > Dropping halted and having management software still using query-cpus > because of halted would be a total failure of query-cpus-fast. > >>> Also, the code that sets/clears cpu->halted is target-specific, >>> so I wouldn't be so sure that simply checking for >>> !kvm_irqchip_in_kernel() is enough on all targets. > > I checked the code and had the impression it was enough, but > I don't have experience with other archs. So, would be nice > if other archs maintainers could review this. I'll try to ping them. > >> Right, the present patch effectively disables halted anyway (including >> s390). > > No, it doesn't. It only disables halted for archs that require going > to the kernel to get it.> >> So it may be cleaner to just drop it right now. >> Assuming the presence of architecure-specific data, libvirt can derive a >> halted state (or an equivalent thereof) from query-cpus-fast returned >> information. > > This is a different proposal. You're proposing moving the halted state > to a CPU-specific field. This is doable if that's what we want. > In order to use query-cpus-fast, libvirt has to change code anyway. Since libvirt is only providing cpu.n.halted for s390, and this value can be derived from the cpu-state, there's no need for QEMU to return halted in query-cpus-fast any more. -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [PATCH] S390: Expose s390-specific CPU info
On 08.02.2018 16:19, Eric Blake wrote:
>
> Missing a documentation line that mentions when the enum grew. Also, has
> a conflict with this other proposed addition, which demonstrates what
> the documentation should look like (should be easy to resolve, though):
> https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg01986.html
>
Good pointer, thanks. So the enum conflict would be resolved on a
first-to-ack base?
>
>> ##
>> +# @CpuInfoS390State:
>> +#
>> +# An enumeration of cpu states that can be assumed by a virtual
>> +# S390 CPU
>> +#
>> +# Since: 2.12
>> +##
>> +{ 'enum': 'CpuInfoS390State',
>> + 'data': [ 'uninitialized', 'stopped', 'check_stop', 'operating',
>> 'load' ] }
>> +
>
> Is there a consistency reason for naming this 'check_stop', or can we go
> with our preference for using dash 'check-stop'?
No specific reason, I've based that on the definitions previously in
target/s390x/cpu.h, same thing for cpu-state. Will update.
>
>> +##
>> +# @CpuInfoS390:
>> +#
>> +# Additional information about a virtual S390 CPU
>> +#
>> +# @cpu_state: the CPUs state
>> +#
>> +# Since: 2.12
>> +##
>> +{ 'struct': 'CpuInfoS390', 'data': { 'cpu_state': 'CpuInfoS390State' } }
>
> Likewise for 'cpu-state'
>
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [PATCH] S390: Expose s390-specific CPU info
On 08.02.2018 16:30, Luiz Capitulino wrote:
> On Thu, 8 Feb 2018 16:21:26 +0100
> Cornelia Huck <coh...@redhat.com> wrote:
>
>> On Thu, 8 Feb 2018 09:09:04 -0500
>> Luiz Capitulino <lcapitul...@redhat.com> wrote:
>>
>>> On Thu, 8 Feb 2018 10:48:08 +0100
>>> Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com> wrote:
>>>
>>>> Presently s390x is the only architecture not exposing specific
>>>> CPU information via QMP query-cpus. Upstream discussion has shown
>>>> that it could make sense to report the architecture specific CPU
>>>> state, e.g. to detect that a CPU has been stopped.
>>>
>>> I'd very strongly advise against extending query-cpus. Note that the
>>> latency problems with query-cpus exists in all archs, it's just a
>>> matter of time for it to pop up for s390 use-cases too.
>>>
>>> I think there's three options for this change:
>>>
>>> 1. If this doesn't require interrupting vCPU threads, then you
>>> could rebase this on top of query-cpus-fast
>>
>> From my perspective, rebasing on top of query-cpus-fast looks like a
>> good idea. This would imply that we need architecture-specific fields
>> for the new interface as well, though.
>
> That's not a problem. I mean, to be honest I think I'd slightly prefer
> to keep things separate and add a new command for each arch that needs
> its specific information, but that's just personal preference. The only
> strong requirement for query-cpus-fast is that it doesn't interrupt
> vCPU threads.
>
While it's a reasonable idea to deprecate query-cpus it will not go away
in a while, if ever. Reason is that there's a significant number of
libvirt release out there using it to probe the VCPUs of a domain.
It would be more than strange if the fast-but-slim version of query-cpus
would report a superset of the slow-but-thorough version.
Therefore, while query-cpus is available, it has to have all the
cpu info.
I'm going to spin a new version of the patch with the changes suggested
by Eric. Additionaly, see the patch below, which can be applied on top
Luiz' and my patch to provide the s390 cpu state with both query types.
[PATCH] S390: Add architecture specific cpu data for query-cpus-fast
The s390 CPU state can be retrieved without interrupting the
VM execution. Extendend the CpuInfo2 with optional architecture
specific data and an implementation for s390.
Return data looks like this:
[
{"thread-id":64301,"props":{"core-id":0},
"archdata":{"arch":"s390","cpu_state":"operating"},
"qom-path":"/machine/unattached/device[0]","cpu-index":0},
{"thread-id":64302,"props":{"core-id":1},
"archdata":{"arch":"s390","cpu_state":"operating"},
"qom-path":"/machine/unattached/device[1]","cpu-index":1}
]
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
cpus.c | 13 +
hmp.c| 11 +++
qapi-schema.json | 22 +-
3 files changed, 45 insertions(+), 1 deletion(-)
diff --git a/cpus.c b/cpus.c
index cb04b2c..a972378 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2099,6 +2099,10 @@ CpuInfo2List *qmp_query_cpus_fast(Error **errp)
MachineClass *mc = MACHINE_GET_CLASS(ms);
CpuInfo2List *head = NULL, *cur_item = NULL;
CPUState *cpu;
+#if defined(TARGET_S390X)
+S390CPU *s390_cpu;
+CPUS390XState *env;
+#endif
CPU_FOREACH(cpu) {
CpuInfo2List *info = g_malloc0(sizeof(*info));
@@ -2122,6 +2126,15 @@ CpuInfo2List *qmp_query_cpus_fast(Error **errp)
info->value->halted = cpu->halted;
}
+/* add architecture specific data if available */
+#if defined(TARGET_S390X)
+s390_cpu = S390_CPU(cpu);
+env = _cpu->env;
+info->value->has_archdata = true;
+info->value->archdata = g_malloc0(sizeof(*info->value->archdata));
+info->value->archdata->arch = CPU_INFO_ARCH_S390;
+info->value->archdata->u.s390.cpu_state = env->cpu_state;
+#endif
if (!cur_item) {
head = cur_item = info;
} else {
diff --git a/hmp.c b/hmp.c
index 0c36864..bfd1038 100644
--- a/hmp.c
+++ b/hmp.c
@@ -427,6 +427,17 @@ void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
}
monitor_printf(mon, " qom-path=%s\n", cpu->value->qom_path);
monitor_printf(mon, "\n");
+if (cpu->value->has_archdata) {
+switch (cpu->value->archdata->arch) {
+case CPU_INFO_ARCH_S390:
+mo
Re: [Qemu-devel] [qemu-s390x] [PATCH v5 06/12] s390-ccw: parse and set boot menu options
On 15.02.2018 07:38, Thomas Huth wrote: > On 14.02.2018 18:46, Collin L. Walling wrote: >> I'm beginning to like the usage of splash-time to represent a timeout >> for the boot menu >> less and less. It is really meant for how long a _splash_ _image_ >> should appear during boot. >> >> I'd like to suggest adding a new boot option "menu-timeout". An >> alternative would be >> documenting in qemu-options.hx that s390 treats "splash-time" as the >> menu-timeout. >> >> Thoughts? > > I think you should keep splash-time and not introduce a new option. > Libvirt seems to map the timeout from timeout='X'/> to the splash-time option, and according to the libvirt > documentation: "Additional attribute timeout takes the number of > milliseconds the boot menu should wait until it times out." > > So it seems like splash-time is already expected to define the amount of > time for the boot menu. We should not confuse libvirt or the users by > introducing yet another option here. I agree, the QEMU option name was probably poorly chosen, as it really describes the time a user has to interact with the BIOS before it starts booting the OS. BTW: we could have a nice ASCII art splash image (nah ... just kidding) ## # #### # # # # # # # # # # # # # # # # # # # ## # # # # # # # # # # # # # # # # # # ## #### [...] -- Regards, Viktor Mihajlovski
[Qemu-devel] [PATCHv4 0/4] add query-cpu-fast and related s390 changes
tl;dr v4: Drop HMP "info cpus_fast" and use query-cpus-fast in HMP "info cpus" instead. Requires HMP maintainer re-review of Patch 2/4. This series consolidates patches around a performance issue caused by the usage of QMP query-cpus. A performance issue was found in an OpenStack environment, where ceilometer was collecting domain statistics with libvirt. The domain statistics reported by libvirt include the vCPU halted state, which in turn is retrieved with QMP query-cpus. This causes two issues: 1. Performance: on most architectures query-cpus needs to issue a KVM ioctl to find out whether a vCPU was halted. This is not the case for s390 but query-cpus is always causing the vCPU to exit the VM. 2. Semantics: on x86 and other architectures, halted is a highly transient state, which is likely to have already changed shortly after the state information has been retrieved. This is not the case for s390, where halted is an indication that the vCPU is stopped, meaning its not available to the guest operating system until it has been restarted. The following patches help to alleviate the issues: Patch 1/4: Adds architecture specific data to the QMP CpuInfo type, exposing the existing s390 cpu-state in QMP. The cpu-state is a representation more adequate than the ambiguous 'halted' condition. Patch 2/4: Adds a new QMP function query-cpus-fast, which will only retrieve vCPU information that can be obtained without interrupting the vCPUs of a running VM. It introduces a new return type CpuInfoFast with the subset of fields meeting this condition. Specifically, the halted state is not part of CpuInfoFast. QMP clients like libvirt are encouraged to switch to the new API for vCPU information. Patch 3/4: Adds the s390-specific cpu state to CpuInfoFast, allowing management apps to find out whether a vCPU is in the stopped state. This extension leads to a partial duplication of field definitions from CpuInfo to CpuInfoFast. This should be tolerable if CpuInfo is deprecated and eventually removed. Patch 4/4 (NEW): Starts the deprecation of query-cpus. Series v3 -> v4: Overall: Instead of adding a new HMP 'info cpus_fast', changed 'info cpus' to use query-cpus-fast directly. Patch 1/4: o Don't report s390-specific data in HMP 'info cpus' Patch 2/4: o Change HMP 'info cpus' to use query-cpus-fast and to return only basic information (no arch-specific data) o Drop HMP 'info cpus_fast' o Fixed typo in commit message Patch 3/4: o Drop HMP-related changes Patch 4/4: o Drop HMP-related changes Series v2 -> v3: Overall: Added r-b's and a-b's. Patch 2/4: o Fixed commit message with respect to the halted field disposition. o Fixed grammar in qapi-schema documentation. Patch 3/4: o Use CpuInfoS390 type for both query-cpus and query-cpus-fast per Eric Blake's comment. o Dropped 'duplication blurb' from commit message as it doesn't provide relevant information other than query-cpus should be deprecated, which is done in the next patch now. Series v1 -> v2: Patch 2/3: o Changed formatting of hmp info cpus_fast to match that of info cpus. This makes it easier for clients to switch to the fast call. Patch 3/3: o Same formatting change for info cpus_fast as in 2/3, only for s390-specific cpu state. Luiz Capitulino (1): qmp: add query-cpus-fast Viktor Mihajlovski (3): qmp: expose s390-specific CPU info qmp: add architecture specific cpu data for query-cpus-fast qemu-doc: deprecate query-cpus cpus.c | 54 + hmp.c | 46 +++--- hw/intc/s390_flic.c| 4 +- hw/s390x/s390-virtio-ccw.c | 2 +- qapi-schema.json | 115 - qemu-doc.texi | 4 ++ target/s390x/cpu.c | 24 +- target/s390x/cpu.h | 7 +-- target/s390x/kvm.c | 8 ++-- target/s390x/sigp.c| 38 +++ 10 files changed, 217 insertions(+), 85 deletions(-) -- 1.9.1
[Qemu-devel] [PATCHv4 4/4] qemu-doc: deprecate query-cpus
Start the deprecation period for QAPI query-cpus (replaced by
query-cpus-fast) beginning with 2.12.0.
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
qapi-schema.json | 4
qemu-doc.texi| 4
2 files changed, 8 insertions(+)
diff --git a/qapi-schema.json b/qapi-schema.json
index e6ca63f..cd98a94 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -587,6 +587,10 @@
# ]
#}
#
+# Notes: This interface is deprecated (since 2.12.0), and it is strongly
+#recommended that you avoid using it. Use @query-cpus-fast to
+#obtain information about virtual CPUs.
+#
##
{ 'command': 'query-cpus', 'returns': ['CpuInfo'] }
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 137f581..221f565 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2764,6 +2764,10 @@ by the ``convert -l snapshot_param'' argument instead.
"autoload" parameter is now ignored. All bitmaps are automatically loaded
from qcow2 images.
+@subsection query-cpus (since 2.12.0)
+
+The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
+
@section System emulator human monitor commands
@subsection host_net_add (since 2.10.0)
--
1.9.1
[Qemu-devel] [PATCHv4 1/4] qmp: expose s390-specific CPU info
Presently s390x is the only architecture not exposing specific
CPU information via QMP query-cpus. Upstream discussion has shown
that it could make sense to report the architecture specific CPU
state, e.g. to detect that a CPU has been stopped.
With this change the output of query-cpus will look like this on
s390:
[
{"arch": "s390", "current": true,
"props": {"core-id": 0}, "cpu-state": "operating", "CPU": 0,
"qom_path": "/machine/unattached/device[0]",
"halted": false, "thread_id": 63115},
{"arch": "s390", "current": false,
"props": {"core-id": 1}, "cpu-state": "stopped", "CPU": 1,
"qom_path": "/machine/unattached/device[1]",
"halted": true, "thread_id": 63116}
]
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
Acked-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: David Hildenbrand <da...@redhat.com>
Reviewed-by: Christian Borntraeger <borntrae...@de.ibm.com>
---
cpus.c | 6 ++
hw/intc/s390_flic.c| 4 ++--
hw/s390x/s390-virtio-ccw.c | 2 +-
qapi-schema.json | 28 +++-
target/s390x/cpu.c | 24
target/s390x/cpu.h | 7 ++-
target/s390x/kvm.c | 8
target/s390x/sigp.c| 38 +++---
8 files changed, 73 insertions(+), 44 deletions(-)
diff --git a/cpus.c b/cpus.c
index f298b65..6006931 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2100,6 +2100,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
#elif defined(TARGET_TRICORE)
TriCoreCPU *tricore_cpu = TRICORE_CPU(cpu);
CPUTriCoreState *env = _cpu->env;
+#elif defined(TARGET_S390X)
+S390CPU *s390_cpu = S390_CPU(cpu);
+CPUS390XState *env = _cpu->env;
#endif
cpu_synchronize_state(cpu);
@@ -2127,6 +2130,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
#elif defined(TARGET_TRICORE)
info->value->arch = CPU_INFO_ARCH_TRICORE;
info->value->u.tricore.PC = env->PC;
+#elif defined(TARGET_S390X)
+info->value->arch = CPU_INFO_ARCH_S390;
+info->value->u.s390.cpu_state = env->cpu_state;
#else
info->value->arch = CPU_INFO_ARCH_OTHER;
#endif
diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
index a85a149..5f8168f 100644
--- a/hw/intc/s390_flic.c
+++ b/hw/intc/s390_flic.c
@@ -192,8 +192,8 @@ static void qemu_s390_flic_notify(uint32_t type)
cs->interrupt_request |= CPU_INTERRUPT_HARD;
/* ignore CPUs that are not sleeping */
-if (s390_cpu_get_state(cpu) != CPU_STATE_OPERATING &&
-s390_cpu_get_state(cpu) != CPU_STATE_LOAD) {
+if (s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING &&
+s390_cpu_get_state(cpu) != S390_CPU_STATE_LOAD) {
continue;
}
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 4abbe89..4d0c3de 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -368,7 +368,7 @@ static void s390_machine_reset(void)
/* all cpus are stopped - configure and start the ipl cpu only */
s390_ipl_prepare_cpu(ipl_cpu);
-s390_cpu_set_state(CPU_STATE_OPERATING, ipl_cpu);
+s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu);
}
static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
diff --git a/qapi-schema.json b/qapi-schema.json
index 0262b9f..94d560e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -410,10 +410,12 @@
# An enumeration of cpu types that enable additional information during
# @query-cpus.
#
+# @s390: since 2.12
+#
# Since: 2.6
##
{ 'enum': 'CpuInfoArch',
- 'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 'other' ] }
+ 'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 's390', 'other' ] }
##
# @CpuInfo:
@@ -452,6 +454,7 @@
'ppc': 'CpuInfoPPC',
'mips': 'CpuInfoMIPS',
'tricore': 'CpuInfoTricore',
+'s390': 'CpuInfoS390',
'other': 'CpuInfoOther' } }
##
@@ -522,6 +525,29 @@
{ 'struct': 'CpuInfoOther', 'data': { } }
##
+# @CpuS390State:
+#
+# An enumeration of cpu states that can be assumed by a virtual
+# S390 CPU
+#
+# Since: 2.12
+##
+{ 'enum': 'CpuS390State',
+ 'prefix': 'S390_CPU_STATE',
+ 'data': [ 'uninitialized', 'stopped', 'check-stop', 'operating', 'load' ] }
+
+##
+# @CpuInfoS390:
+#
+# Additional information about a virtual S390 CPU
+#
+# @cpu-state: the virtual CPU's state
+#
+# Since: 2.12
+##
+{ 'struct': 'CpuInfoS390', 'data': { 'cpu-state': 'CpuS390State' } }
+
+##
# @query-cpus:
#
# Returns a list of information about each virtual CPU.
diff --git a/target/s390x/cpu.c b
Re: [Qemu-devel] [qemu-s390x] [PATCH v6 01/12] s390-ccw: refactor boot map table code
On 16.02.2018 11:42, Thomas Huth wrote:
> On 15.02.2018 23:54, Collin L. Walling wrote:
>> Some ECKD bootmap code was using structs designed for SCSI.
>> Even though this works, it confuses readability. Add a new
>> BootMapTable struct to assist with readability in bootmap
>> entry code. Also:
>>
>> - replace ScsiMbr in ECKD code with appropriate structs
>> - fix read_block messages to reflect BootMapTable
>> - fixup ipl_scsi to use BootMapTable (referred to as Program Table)
>> - defined value for maximum table entries
>>
>> Signed-off-by: Collin L. Walling <wall...@linux.vnet.ibm.com>
>> ---
>> pc-bios/s390-ccw/bootmap.c | 60
>> +-
>> pc-bios/s390-ccw/bootmap.h | 14 +--
>> 2 files changed, 39 insertions(+), 35 deletions(-)
> [...]
>> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
>> index cf99a4c..850b655 100644
>> --- a/pc-bios/s390-ccw/bootmap.h
>> +++ b/pc-bios/s390-ccw/bootmap.h
>> @@ -53,6 +53,15 @@ typedef union BootMapPointer {
>> ExtEckdBlockPtr xeckd;
>> } __attribute__ ((packed)) BootMapPointer;
>>
>> +#define MAX_TABLE_ENTRIES 30
>> +
>> +/* aka Program Table */
>> +typedef struct BootMapTable {
>> +uint8_t magic[4];
>> +uint8_t reserved[12];
>> +BootMapPointer entry[];
>> +} __attribute__ ((packed)) BootMapTable;
>> +
>> typedef struct ComponentEntry {
>> ScsiBlockPtr data;
>> uint8_t pad[7];
>> @@ -69,8 +78,9 @@ typedef struct ComponentHeader {
>> typedef struct ScsiMbr {
>> uint8_t magic[4];
>> uint32_t version_id;
>> -uint8_t reserved[8];
>> -ScsiBlockPtr blockptr[];
>> +uint8_t reserved1[8];
>> +ScsiBlockPtr pt; /* block pointer to program table */
>> +uint8_t reserved2[120];
>
> Did you want to pad the struct to 512 bytes here? If so, I think that
> should have been "uint32_t" instead of "uint8_t" or "480" instead of "120" ?
>
Should probably be uint8_t[480] then to stay in style with other
reserved field definitions.
>> } __attribute__ ((packed)) ScsiMbr;
>>
>> #define ZIPL_MAGIC "zIPL"
>>
>
> Thomas
>
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [PATCH v6 05/12] s390-ccw: move auxiliary IPL data to separate location
On 15.02.2018 23:54, Collin L. Walling wrote: > The s390-ccw firmware needs some information in support of the > boot process which is not available on the native machine. > Examples are the netboot firmware load address and now the > boot menu parameters. > > While storing that data in unused fields of the IPL parameter block > works, that approach could create problems if the parameter block > definition should change in the future. Because then a guest could > overwrite these fields using the set IPLB diagnose. > > In fact the data in question is of more global nature and not really > tied to an IPL device, so separating it is rather logical. > > This commit introduces a new structure to hold firmware relevant > IPL parameters set by QEMU. The data is stored at location 204 (dec) > and can contain up to 7 32-bit words. This area is available to > programming in the z/Architecture Principles of Operation and > can thus safely be used by the firmware until the IPL has completed. > > Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com> you might want to add your s-o-b here ... [...] -- Regards, Viktor Mihajlovski
[Qemu-devel] [PATCHv4 3/4] qmp: add architecture specific cpu data for query-cpus-fast
The s390 CPU state can be retrieved without interrupting the
VM execution. Extendend the CpuInfoFast union with architecture
specific data and an implementation for s390.
Return data looks like this:
[
{"thread-id":64301,"props":{"core-id":0},
"arch":"s390","cpu-state":"operating",
"qom-path":"/machine/unattached/device[0]","cpu-index":0},
{"thread-id":64302,"props":{"core-id":1},
"arch":"s390","cpu-state":"operating",
"qom-path":"/machine/unattached/device[1]","cpu-index":1}
]
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <coh...@redhat.com>
Acked-by: Eric Blake <ebl...@redhat.com>
---
cpus.c | 10 ++
qapi-schema.json | 25 ++---
2 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/cpus.c b/cpus.c
index 6df6660..af67826 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2166,6 +2166,10 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
MachineClass *mc = MACHINE_GET_CLASS(ms);
CpuInfoFastList *head = NULL, *cur_item = NULL;
CPUState *cpu;
+#if defined(TARGET_S390X)
+S390CPU *s390_cpu;
+CPUS390XState *env;
+#endif
CPU_FOREACH(cpu) {
CpuInfoFastList *info = g_malloc0(sizeof(*info));
@@ -2183,6 +2187,12 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
info->value->props = props;
}
+#if defined(TARGET_S390X)
+s390_cpu = S390_CPU(cpu);
+env = _cpu->env;
+info->value->arch = CPU_INFO_ARCH_S390;
+info->value->u.s390.cpu_state = env->cpu_state;
+#endif
if (!cur_item) {
head = cur_item = info;
} else {
diff --git a/qapi-schema.json b/qapi-schema.json
index 815f072..e6ca63f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -408,7 +408,7 @@
# @CpuInfoArch:
#
# An enumeration of cpu types that enable additional information during
-# @query-cpus.
+# @query-cpus and @query-cpus-fast.
#
# @s390: since 2.12
#
@@ -604,12 +604,24 @@
# @props: properties describing to which node/socket/core/thread
# virtual CPU belongs to, provided if supported by board
#
+# @arch: architecture of the cpu, which determines which additional fields
+#will be listed
+#
# Since: 2.12
#
##
-{ 'struct': 'CpuInfoFast',
- 'data': {'cpu-index': 'int', 'qom-path': 'str',
- 'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
+{ 'union': 'CpuInfoFast',
+ 'base': {'cpu-index': 'int', 'qom-path': 'str',
+ 'thread-id': 'int', '*props': 'CpuInstanceProperties',
+ 'arch': 'CpuInfoArch' },
+ 'discriminator': 'arch',
+ 'data': { 'x86': 'CpuInfoOther',
+'sparc': 'CpuInfoOther',
+'ppc': 'CpuInfoOther',
+'mips': 'CpuInfoOther',
+'tricore': 'CpuInfoOther',
+'s390': 'CpuInfoS390',
+'other': 'CpuInfoOther' } }
##
# @query-cpus-fast:
@@ -620,9 +632,6 @@
#
# Returns: list of @CpuInfoFast
#
-# Notes: The CPU architecture name is not returned by query-cpus-fast.
-#Use query-target to retrieve that information.
-#
# Since: 2.12
#
# Example:
@@ -637,6 +646,7 @@
# "socket-id": 0
# },
# "qom-path": "/machine/unattached/device[0]",
+# "arch":"x86",
# "cpu-index": 0
# },
# {
@@ -647,6 +657,7 @@
# "socket-id": 1
# },
# "qom-path": "/machine/unattached/device[2]",
+# "arch":"x86",
# "cpu-index": 1
# }
# ]
--
1.9.1
[Qemu-devel] [PATCHv4 2/4] qmp: add query-cpus-fast
From: Luiz Capitulino <lcapitul...@redhat.com>
The query-cpus command has an extremely serious side effect:
it always interrupts all running vCPUs so that they can run
ioctl calls. This can cause a huge performance degradation for
some workloads. And most of the information retrieved by the
ioctl calls are not even used by query-cpus.
This commit introduces a replacement for query-cpus called
query-cpus-fast, which has the following features:
o Never interrupt vCPUs threads. query-cpus-fast only returns
vCPU information maintained by QEMU itself, which should be
sufficient for most management software needs
o Drop "halted" field as it can not be retrieved in a fast
way on most architectures
o Drop irrelevant fields such as "current", "pc" and "arch"
o Rename some fields for better clarification & proper naming
standard
Further, the HMP "info cpus" implementation is changed to
use the new QMP interface to avoid the side effects caused
by query-cpus. This means that only a reduced subset of cpu
information will be reported, which is acceptable as
the content of "info cpus" is not documented or guaranteed
to be stable.
Signed-off-by: Luiz Capitulino <lcapitul...@redhat.com>
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <coh...@redhat.com>
Acked-by: Eric Blake <ebl...@redhat.com>
---
cpus.c | 38 ++
hmp.c| 46 +
qapi-schema.json | 70
3 files changed, 114 insertions(+), 40 deletions(-)
diff --git a/cpus.c b/cpus.c
index 6006931..6df6660 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2156,6 +2156,44 @@ CpuInfoList *qmp_query_cpus(Error **errp)
return head;
}
+/*
+ * fast means: we NEVER interrupt vCPU threads to retrieve
+ * information from KVM.
+ */
+CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
+{
+MachineState *ms = MACHINE(qdev_get_machine());
+MachineClass *mc = MACHINE_GET_CLASS(ms);
+CpuInfoFastList *head = NULL, *cur_item = NULL;
+CPUState *cpu;
+
+CPU_FOREACH(cpu) {
+CpuInfoFastList *info = g_malloc0(sizeof(*info));
+info->value = g_malloc0(sizeof(*info->value));
+
+info->value->cpu_index = cpu->cpu_index;
+info->value->qom_path = object_get_canonical_path(OBJECT(cpu));
+info->value->thread_id = cpu->thread_id;
+
+info->value->has_props = !!mc->cpu_index_to_instance_props;
+if (info->value->has_props) {
+CpuInstanceProperties *props;
+props = g_malloc0(sizeof(*props));
+*props = mc->cpu_index_to_instance_props(ms, cpu->cpu_index);
+info->value->props = props;
+}
+
+if (!cur_item) {
+head = cur_item = info;
+} else {
+cur_item->next = info;
+cur_item = info;
+}
+}
+
+return head;
+}
+
void qmp_memsave(int64_t addr, int64_t size, const char *filename,
bool has_cpu, int64_t cpu_index, Error **errp)
{
diff --git a/hmp.c b/hmp.c
index 7870d6a..28b1070 100644
--- a/hmp.c
+++ b/hmp.c
@@ -360,50 +360,16 @@ void hmp_info_migrate_cache_size(Monitor *mon, const
QDict *qdict)
void hmp_info_cpus(Monitor *mon, const QDict *qdict)
{
-CpuInfoList *cpu_list, *cpu;
+CpuInfoFastList *head, *cpu;
-cpu_list = qmp_query_cpus(NULL);
+head = qmp_query_cpus_fast(NULL);
-for (cpu = cpu_list; cpu; cpu = cpu->next) {
-int active = ' ';
-
-if (cpu->value->CPU == monitor_get_cpu_index()) {
-active = '*';
-}
-
-monitor_printf(mon, "%c CPU #%" PRId64 ":", active, cpu->value->CPU);
-
-switch (cpu->value->arch) {
-case CPU_INFO_ARCH_X86:
-monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->u.x86.pc);
-break;
-case CPU_INFO_ARCH_PPC:
-monitor_printf(mon, " nip=0x%016" PRIx64, cpu->value->u.ppc.nip);
-break;
-case CPU_INFO_ARCH_SPARC:
-monitor_printf(mon, " pc=0x%016" PRIx64,
- cpu->value->u.q_sparc.pc);
-monitor_printf(mon, " npc=0x%016" PRIx64,
- cpu->value->u.q_sparc.npc);
-break;
-case CPU_INFO_ARCH_MIPS:
-monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.q_mips.PC);
-break;
-case CPU_INFO_ARCH_TRICORE:
-monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore.PC);
-break;
-default:
-break;
-}
-
-if (cpu->value->halted) {
-monitor_printf(mon, " (hal
[Qemu-devel] [PATCHv3 0/4] ] add query-cpu-fast and related s390 changes
This series consolidates patches around a performance issue caused by the usage of QMP query-cpus. A performance issue was found in an OpenStack environment, where ceilometer was collecting domain statistics with libvirt. The domain statistics reported by libvirt include the vCPU halted state, which in turn is retrieved with QMP query-cpus. This causes two issues: 1. Performance: on most architectures query-cpus needs to issue a KVM ioctl to find out whether a vCPU was halted. This is not the case for s390 but query-cpus is always causing the vCPU to exit the VM. 2. Semantics: on x86 and other architectures, halted is a highly transient state, which is likely to have already changed shortly after the state information has been retrieved. This is not the case for s390, where halted is an indication that the vCPU is stopped, meaning its not available to the guest operating system until it has been restarted. The following patches help to alleviate the issues: Patch 1/4: Adds architecture specific data to the QMP CpuInfo type, exposing the existing s390 cpu-state in QMP. The cpu-state is a representation more adequate than the ambiguous 'halted' condition. Patch 2/4: Adds a new QMP function query-cpus-fast, which will only retrieve vCPU information that can be obtained without interrupting the vCPUs of a running VM. It introduces a new return type CpuInfoFast with the subset of fields meeting this condition. Specifically, the halted state is not part of CpuInfoFast. QMP clients like libvirt are encouraged to switch to the new API for vCPU information. Patch 3/4: Adds the s390-specific cpu state to CpuInfoFast, allowing management apps to find out whether a vCPU is in the stopped state. This extension leads to a partial duplication of field definitions from CpuInfo to CpuInfoFast. This should be tolerable if CpuInfo is deprecated and eventually removed. Patch 4/4 (NEW): Starts the deprecation of query-cpus and hmp 'info cpus'. It wouldn't hurt to have QAPI and HMP maintainer reviews for this. Series v2 -> v3: Overall: Added r-b's and a-b's. Patch 2/4: o Fixed commit message with respect to the halted field disposition. o Fixed grammar in qapi-schema documentation. Patch 3/4: o Use CpuInfoS390 type for both query-cpus and query-cpus-fast per Eric Blake's comment. o Dropped 'duplication blurb' from commit message as it doesn't provide relevant information other than query-cpus should be deprecated, which is done in the next patch now. Series v1 -> v2: Patch 2/3: o Changed formatting of hmp info cpus_fast to match that of info cpus. This makes it easier for clients to switch to the fast call. Patch 3/3: o Same formatting change for info cpus_fast as in 2/3, only for s390-specific cpu state. Luiz Capitulino (1): qmp: add query-cpus-fast Viktor Mihajlovski (3): qmp: expose s390-specific CPU info qmp: add architecture specific cpu data for query-cpus-fast qemu-doc: deprecate query-cpus and info cpus cpus.c | 54 + hmp-commands-info.hx | 18 ++- hmp.c | 28 +++ hmp.h | 1 + hw/intc/s390_flic.c| 4 +- hw/s390x/s390-virtio-ccw.c | 2 +- qapi-schema.json | 115 - qemu-doc.texi | 10 target/s390x/cpu.c | 24 +- target/s390x/cpu.h | 7 +-- target/s390x/kvm.c | 8 ++-- target/s390x/sigp.c| 38 +++ 12 files changed, 262 insertions(+), 47 deletions(-) -- 1.9.1
[Qemu-devel] [PATCHv3 3/4] qmp: add architecture specific cpu data for query-cpus-fast
The s390 CPU state can be retrieved without interrupting the
VM execution. Extendend the CpuInfoFast union with architecture
specific data and an implementation for s390.
Return data looks like this:
[
{"thread-id":64301,"props":{"core-id":0},
"arch":"s390","cpu-state":"operating",
"qom-path":"/machine/unattached/device[0]","cpu-index":0},
{"thread-id":64302,"props":{"core-id":1},
"arch":"s390","cpu-state":"operating",
"qom-path":"/machine/unattached/device[1]","cpu-index":1}
]
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <coh...@redhat.com>
Acked-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
Acked-by: Eric Blake <ebl...@redhat.com>
---
cpus.c | 10 ++
hmp.c| 10 ++
qapi-schema.json | 25 ++---
3 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/cpus.c b/cpus.c
index 6df6660..af67826 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2166,6 +2166,10 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
MachineClass *mc = MACHINE_GET_CLASS(ms);
CpuInfoFastList *head = NULL, *cur_item = NULL;
CPUState *cpu;
+#if defined(TARGET_S390X)
+S390CPU *s390_cpu;
+CPUS390XState *env;
+#endif
CPU_FOREACH(cpu) {
CpuInfoFastList *info = g_malloc0(sizeof(*info));
@@ -2183,6 +2187,12 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
info->value->props = props;
}
+#if defined(TARGET_S390X)
+s390_cpu = S390_CPU(cpu);
+env = _cpu->env;
+info->value->arch = CPU_INFO_ARCH_S390;
+info->value->u.s390.cpu_state = env->cpu_state;
+#endif
if (!cur_item) {
head = cur_item = info;
} else {
diff --git a/hmp.c b/hmp.c
index 0bd3b3a..e27433e 100644
--- a/hmp.c
+++ b/hmp.c
@@ -418,6 +418,16 @@ void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
for (cpu = head; cpu; cpu = cpu->next) {
monitor_printf(mon, " CPU #%" PRId64 ":", cpu->value->cpu_index);
+
+switch (cpu->value->arch) {
+case CPU_INFO_ARCH_S390:
+monitor_printf(mon, " state=%s",
+ CpuS390State_str(cpu->value->u.s390.cpu_state));
+break;
+default:
+break;
+}
+
monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
}
diff --git a/qapi-schema.json b/qapi-schema.json
index 815f072..e6ca63f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -408,7 +408,7 @@
# @CpuInfoArch:
#
# An enumeration of cpu types that enable additional information during
-# @query-cpus.
+# @query-cpus and @query-cpus-fast.
#
# @s390: since 2.12
#
@@ -604,12 +604,24 @@
# @props: properties describing to which node/socket/core/thread
# virtual CPU belongs to, provided if supported by board
#
+# @arch: architecture of the cpu, which determines which additional fields
+#will be listed
+#
# Since: 2.12
#
##
-{ 'struct': 'CpuInfoFast',
- 'data': {'cpu-index': 'int', 'qom-path': 'str',
- 'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
+{ 'union': 'CpuInfoFast',
+ 'base': {'cpu-index': 'int', 'qom-path': 'str',
+ 'thread-id': 'int', '*props': 'CpuInstanceProperties',
+ 'arch': 'CpuInfoArch' },
+ 'discriminator': 'arch',
+ 'data': { 'x86': 'CpuInfoOther',
+'sparc': 'CpuInfoOther',
+'ppc': 'CpuInfoOther',
+'mips': 'CpuInfoOther',
+'tricore': 'CpuInfoOther',
+'s390': 'CpuInfoS390',
+'other': 'CpuInfoOther' } }
##
# @query-cpus-fast:
@@ -620,9 +632,6 @@
#
# Returns: list of @CpuInfoFast
#
-# Notes: The CPU architecture name is not returned by query-cpus-fast.
-#Use query-target to retrieve that information.
-#
# Since: 2.12
#
# Example:
@@ -637,6 +646,7 @@
# "socket-id": 0
# },
# "qom-path": "/machine/unattached/device[0]",
+# "arch":"x86",
# "cpu-index": 0
# },
# {
@@ -647,6 +657,7 @@
# "socket-id": 1
# },
# "qom-path": "/machine/unattached/device[2]",
+# "arch":"x86",
# "cpu-index": 1
# }
# ]
--
1.9.1
[Qemu-devel] [PATCHv3 1/4] qmp: expose s390-specific CPU info
Presently s390x is the only architecture not exposing specific
CPU information via QMP query-cpus. Upstream discussion has shown
that it could make sense to report the architecture specific CPU
state, e.g. to detect that a CPU has been stopped.
With this change the output of query-cpus will look like this on
s390:
[
{"arch": "s390", "current": true,
"props": {"core-id": 0}, "cpu-state": "operating", "CPU": 0,
"qom_path": "/machine/unattached/device[0]",
"halted": false, "thread_id": 63115},
{"arch": "s390", "current": false,
"props": {"core-id": 1}, "cpu-state": "stopped", "CPU": 1,
"qom_path": "/machine/unattached/device[1]",
"halted": true, "thread_id": 63116}
]
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
Acked-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: David Hildenbrand <da...@redhat.com>
Reviewed-by: Christian Borntraeger <borntrae...@de.ibm.com>
---
cpus.c | 6 ++
hmp.c | 4
hw/intc/s390_flic.c| 4 ++--
hw/s390x/s390-virtio-ccw.c | 2 +-
qapi-schema.json | 28 +++-
target/s390x/cpu.c | 24
target/s390x/cpu.h | 7 ++-
target/s390x/kvm.c | 8
target/s390x/sigp.c| 38 +++---
9 files changed, 77 insertions(+), 44 deletions(-)
diff --git a/cpus.c b/cpus.c
index f298b65..6006931 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2100,6 +2100,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
#elif defined(TARGET_TRICORE)
TriCoreCPU *tricore_cpu = TRICORE_CPU(cpu);
CPUTriCoreState *env = _cpu->env;
+#elif defined(TARGET_S390X)
+S390CPU *s390_cpu = S390_CPU(cpu);
+CPUS390XState *env = _cpu->env;
#endif
cpu_synchronize_state(cpu);
@@ -2127,6 +2130,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
#elif defined(TARGET_TRICORE)
info->value->arch = CPU_INFO_ARCH_TRICORE;
info->value->u.tricore.PC = env->PC;
+#elif defined(TARGET_S390X)
+info->value->arch = CPU_INFO_ARCH_S390;
+info->value->u.s390.cpu_state = env->cpu_state;
#else
info->value->arch = CPU_INFO_ARCH_OTHER;
#endif
diff --git a/hmp.c b/hmp.c
index 7870d6a..a6b94b7 100644
--- a/hmp.c
+++ b/hmp.c
@@ -392,6 +392,10 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
case CPU_INFO_ARCH_TRICORE:
monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore.PC);
break;
+case CPU_INFO_ARCH_S390:
+monitor_printf(mon, " state=%s",
+ CpuS390State_str(cpu->value->u.s390.cpu_state));
+break;
default:
break;
}
diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
index a85a149..5f8168f 100644
--- a/hw/intc/s390_flic.c
+++ b/hw/intc/s390_flic.c
@@ -192,8 +192,8 @@ static void qemu_s390_flic_notify(uint32_t type)
cs->interrupt_request |= CPU_INTERRUPT_HARD;
/* ignore CPUs that are not sleeping */
-if (s390_cpu_get_state(cpu) != CPU_STATE_OPERATING &&
-s390_cpu_get_state(cpu) != CPU_STATE_LOAD) {
+if (s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING &&
+s390_cpu_get_state(cpu) != S390_CPU_STATE_LOAD) {
continue;
}
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 4abbe89..4d0c3de 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -368,7 +368,7 @@ static void s390_machine_reset(void)
/* all cpus are stopped - configure and start the ipl cpu only */
s390_ipl_prepare_cpu(ipl_cpu);
-s390_cpu_set_state(CPU_STATE_OPERATING, ipl_cpu);
+s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu);
}
static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
diff --git a/qapi-schema.json b/qapi-schema.json
index 0262b9f..94d560e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -410,10 +410,12 @@
# An enumeration of cpu types that enable additional information during
# @query-cpus.
#
+# @s390: since 2.12
+#
# Since: 2.6
##
{ 'enum': 'CpuInfoArch',
- 'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 'other' ] }
+ 'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 's390', 'other' ] }
##
# @CpuInfo:
@@ -452,6 +454,7 @@
'ppc': 'CpuInfoPPC',
'mips': 'CpuInfoMIPS',
'tricore': 'CpuInfoTricore',
+'s390': 'CpuInfoS390',
'other': 'CpuInfoOther' } }
##
@@ -522,6 +525,29 @@
{ 'struct': 'CpuInfoOther', 'data': { } }
[Qemu-devel] [PATCHv3 4/4] qemu-doc: deprecate query-cpus and info cpus
Start the deprecation period for QAPI query-cpus (replaced by
query-cpus-fast) and HMP 'info cpus' (replaced by 'info cpus_fast')
beginning with 2.12.0.
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
hmp-commands-info.hx | 4 ++--
qapi-schema.json | 4
qemu-doc.texi| 10 ++
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 16ac602..2ccb9c7 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -149,14 +149,14 @@ ETEXI
.name = "cpus",
.args_type = "",
.params = "",
-.help = "show infos for each CPU",
+.help = "show infos for each CPU (deprecated, use info cpus_fast
instead)",
.cmd= hmp_info_cpus,
},
STEXI
@item info cpus
@findex info cpus
-Show infos for each CPU.
+Show infos for each CPU. Deprecated, please use @code{info cpus_fast} instead.
ETEXI
{
diff --git a/qapi-schema.json b/qapi-schema.json
index e6ca63f..cd98a94 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -587,6 +587,10 @@
# ]
#}
#
+# Notes: This interface is deprecated (since 2.12.0), and it is strongly
+#recommended that you avoid using it. Use @query-cpus-fast to
+#obtain information about virtual CPUs.
+#
##
{ 'command': 'query-cpus', 'returns': ['CpuInfo'] }
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 769968a..46aacb6 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2757,6 +2757,12 @@ used and it will be removed with no replacement.
The ``convert -s snapshot_id_or_name'' argument is obsoleted
by the ``convert -l snapshot_param'' argument instead.
+@section System emulator monitor commands
+
+@subsection query-cpus (since 2.12.0)
+
+The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
+
@section System emulator human monitor commands
@subsection host_net_add (since 2.10.0)
@@ -2767,6 +2773,10 @@ The ``host_net_add'' command is replaced by the
``netdev_add'' command.
The ``host_net_remove'' command is replaced by the ``netdev_del'' command.
+@subsection info cpus (since 2.12.0)
+
+The ``info cpus'' command is replaced by the ``info cpus_fast'' command.
+
@section System emulator devices
@subsection ivshmem (since 2.6.0)
--
1.9.1
[Qemu-devel] [PATCHv3 2/4] qmp: add query-cpus-fast
From: Luiz Capitulino <lcapitul...@redhat.com>
The query-cpus command has an extremely serious side effect:
it always interrupts all running vCPUs so that they can run
ioctl calls. This can cause a huge performance degradation for
some workloads. And most of the information retrieved by the
ioctl calls are not even used by query-cpus.
This commit introduces a replacement for query-cpus called
query-cpus-fast, which has the following features:
o Never interrupt vCPUs threads. query-cpus-fast only returns
vCPU information maintained by QEMU itself, which should be
sufficient for most management software needs
o Drop "halted" field as it can not retrieved in a fast
way on most architectures
o Drop irrelevant fields such as "current", "pc" and "arch"
o Rename some fields for better clarification & proper naming
standard
Signed-off-by: Luiz Capitulino <lcapitul...@redhat.com>
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <coh...@redhat.com>
Acked-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
Acked-by: Eric Blake <ebl...@redhat.com>
---
cpus.c | 38
hmp-commands-info.hx | 14 +++
hmp.c| 14 +++
hmp.h| 1 +
qapi-schema.json | 70
5 files changed, 137 insertions(+)
diff --git a/cpus.c b/cpus.c
index 6006931..6df6660 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2156,6 +2156,44 @@ CpuInfoList *qmp_query_cpus(Error **errp)
return head;
}
+/*
+ * fast means: we NEVER interrupt vCPU threads to retrieve
+ * information from KVM.
+ */
+CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
+{
+MachineState *ms = MACHINE(qdev_get_machine());
+MachineClass *mc = MACHINE_GET_CLASS(ms);
+CpuInfoFastList *head = NULL, *cur_item = NULL;
+CPUState *cpu;
+
+CPU_FOREACH(cpu) {
+CpuInfoFastList *info = g_malloc0(sizeof(*info));
+info->value = g_malloc0(sizeof(*info->value));
+
+info->value->cpu_index = cpu->cpu_index;
+info->value->qom_path = object_get_canonical_path(OBJECT(cpu));
+info->value->thread_id = cpu->thread_id;
+
+info->value->has_props = !!mc->cpu_index_to_instance_props;
+if (info->value->has_props) {
+CpuInstanceProperties *props;
+props = g_malloc0(sizeof(*props));
+*props = mc->cpu_index_to_instance_props(ms, cpu->cpu_index);
+info->value->props = props;
+}
+
+if (!cur_item) {
+head = cur_item = info;
+} else {
+cur_item->next = info;
+cur_item = info;
+}
+}
+
+return head;
+}
+
void qmp_memsave(int64_t addr, int64_t size, const char *filename,
bool has_cpu, int64_t cpu_index, Error **errp)
{
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index ad590a4..16ac602 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -160,6 +160,20 @@ Show infos for each CPU.
ETEXI
{
+.name = "cpus_fast",
+.args_type = "",
+.params = "",
+.help = "show information for each CPU without interrupting
them",
+.cmd= hmp_info_cpus_fast,
+},
+
+STEXI
+@item info cpus_fast
+@findex info cpus_fast
+Show infos for each CPU without performance penalty.
+ETEXI
+
+{
.name = "history",
.args_type = "",
.params = "",
diff --git a/hmp.c b/hmp.c
index a6b94b7..0bd3b3a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -410,6 +410,20 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
qapi_free_CpuInfoList(cpu_list);
}
+void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
+{
+CpuInfoFastList *head, *cpu;
+
+head = qmp_query_cpus_fast(NULL);
+
+for (cpu = head; cpu; cpu = cpu->next) {
+monitor_printf(mon, " CPU #%" PRId64 ":", cpu->value->cpu_index);
+monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
+}
+
+qapi_free_CpuInfoFastList(head);
+}
+
static void print_block_info(Monitor *mon, BlockInfo *info,
BlockDeviceInfo *inserted, bool verbose)
{
diff --git a/hmp.h b/hmp.h
index 1143db4..93fb4e4 100644
--- a/hmp.h
+++ b/hmp.h
@@ -29,6 +29,7 @@ void hmp_info_migrate_capabilities(Monitor *mon, const QDict
*qdict);
void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict);
void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict);
void hmp_info_cpus(Monitor *mon, const QDict *qdict);
+void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict);
void hmp_info_block(Monitor *mon, const QDict *qdict);
void hmp_info_b
Re: [Qemu-devel] [PATCHv3 2/4] qmp: add query-cpus-fast
On 15.02.2018 15:53, Eric Blake wrote: > On 02/15/2018 08:40 AM, Viktor Mihajlovski wrote: >> On 15.02.2018 15:19, Eric Blake wrote: >>> On 02/15/2018 04:20 AM, Viktor Mihajlovski wrote: >>>> From: Luiz Capitulino <lcapitul...@redhat.com> >>>> >>>> The query-cpus command has an extremely serious side effect: >>>> it always interrupts all running vCPUs so that they can run >>>> ioctl calls. This can cause a huge performance degradation for >>>> some workloads. And most of the information retrieved by the >>>> ioctl calls are not even used by query-cpus. >>>> >>>> This commit introduces a replacement for query-cpus called > >>> You know, we have no back-compat guarantees on HMP. We could make 'info >>> cpu' just ALWAYS call query-cpus-fast, with no HMP counterpart for the >>> slower query-cpus, and without needing a deprecation period. But I'll >>> leave that up to David if that makes more sense. >> Ditching info cpus_fast would make me happy as well, because it would >> cause less headache on the libvirt side of things. > > Why is libvirt using HMP in the first place? Libvirt should always be > using the QMP command, when one exists. > Which it does, but there's still a lot of fallback code including HMP "info cpus". In real life this should of course never be used, because the QMP is always there. -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [PATCHv3 2/4] qmp: add query-cpus-fast
On 15.02.2018 15:19, Eric Blake wrote:
> On 02/15/2018 04:20 AM, Viktor Mihajlovski wrote:
>> From: Luiz Capitulino <lcapitul...@redhat.com>
>>
>> The query-cpus command has an extremely serious side effect:
>> it always interrupts all running vCPUs so that they can run
>> ioctl calls. This can cause a huge performance degradation for
>> some workloads. And most of the information retrieved by the
>> ioctl calls are not even used by query-cpus.
>>
>> This commit introduces a replacement for query-cpus called
>> query-cpus-fast, which has the following features:
>>
>> o Never interrupt vCPUs threads. query-cpus-fast only returns
>> vCPU information maintained by QEMU itself, which should be
>> sufficient for most management software needs
>>
>> o Drop "halted" field as it can not retrieved in a fast
>
> s/can not retrieved/cannot be retrieved/
>
>> way on most architectures
>>
>> o Drop irrelevant fields such as "current", "pc" and "arch"
>>
>> o Rename some fields for better clarification & proper naming
>> standard
>>
>> Signed-off-by: Luiz Capitulino <lcapitul...@redhat.com>
>> Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
>> Reviewed-by: Cornelia Huck <coh...@redhat.com>
>> Acked-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
>> Acked-by: Eric Blake <ebl...@redhat.com>
>> ---
>
>> +++ b/hmp-commands-info.hx
>> @@ -160,6 +160,20 @@ Show infos for each CPU.
>
> Pre-existing, but while we are here, it wouldn't hurt to do
> s/infos/information/
>
>> ETEXI
>> {
>> + .name = "cpus_fast",
>> + .args_type = "",
>> + .params = "",
>> + .help = "show information for each CPU without
>> interrupting them",
>> + .cmd = hmp_info_cpus_fast,
>> + },
>> +
>> +STEXI
>> +@item info cpus_fast
>> +@findex info cpus_fast
>> +Show infos for each CPU without performance penalty.
>
> and again here, where you copied and pasted.
>
> You know, we have no back-compat guarantees on HMP. We could make 'info
> cpu' just ALWAYS call query-cpus-fast, with no HMP counterpart for the
> slower query-cpus, and without needing a deprecation period. But I'll
> leave that up to David if that makes more sense.
Ditching info cpus_fast would make me happy as well, because it would
cause less headache on the libvirt side of things.
>
>> +++ b/hmp.c
>> @@ -410,6 +410,20 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
>> qapi_free_CpuInfoList(cpu_list);
>> }
>> +void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
>> +{
>> + CpuInfoFastList *head, *cpu;
>> +
>> + head = qmp_query_cpus_fast(NULL);
>> +
>> + for (cpu = head; cpu; cpu = cpu->next) {
>> + monitor_printf(mon, " CPU #%" PRId64 ":",
>> cpu->value->cpu_index);
>> + monitor_printf(mon, " thread-id=%" PRId64 "\n",
>> cpu->value->thread_id);
>> + }
>
> This is particularly true since your HMP implementation is not even
> printing all the information returned by query-cpus-fast!
>
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [qemu-s390x] [PATCH v8 05/13] s390-ccw: move auxiliary IPL data to separate location
On 22.02.2018 05:40, Thomas Huth wrote:
> On 21.02.2018 20:35, Collin L. Walling wrote:
>> The s390-ccw firmware needs some information in support of the
>> boot process which is not available on the native machine.
>> Examples are the netboot firmware load address and now the
>> boot menu parameters.
>>
>> While storing that data in unused fields of the IPL parameter block
>> works, that approach could create problems if the parameter block
>> definition should change in the future. Because then a guest could
>> overwrite these fields using the set IPLB diagnose.
>>
>> In fact the data in question is of more global nature and not really
>> tied to an IPL device, so separating it is rather logical.
>>
>> This commit introduces a new structure to hold firmware relevant
>> IPL parameters set by QEMU. The data is stored at location 204 (dec)
>> and can contain up to 7 32-bit words. This area is available to
>> programming in the z/Architecture Principles of Operation and
>> can thus safely be used by the firmware until the IPL has completed.
>>
>> Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
>> Signed-off-by: Collin L. Walling <wall...@linux.vnet.ibm.com>
>> ---
>> hw/s390x/ipl.c | 18 +-
>> hw/s390x/ipl.h | 25 +++--
>> pc-bios/s390-ccw/iplb.h | 18 --
>> pc-bios/s390-ccw/main.c | 6 +-
>> 4 files changed, 61 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 0d06fc1..79f5a58 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -399,6 +399,21 @@ void s390_reipl_request(void)
>> qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>> }
>>
>> +static void s390_ipl_prepare_qipl(S390CPU *cpu)
>> +{
>> +S390IPLState *ipl = get_ipl_device();
>> +uint8_t *addr;
>> +uint64_t len = 4096;
>> +
>> +addr = cpu_physical_memory_map(cpu->env.psa, , 1);
>> +if (!addr || len < QIPL_ADDRESS + sizeof(QemuIplParameters)) {
>> +error_report("Cannot set QEMU IPL parameters");
>> +return;
>> +}
>> +memcpy(addr + QIPL_ADDRESS, >qipl, sizeof(QemuIplParameters));
>> +cpu_physical_memory_unmap(addr, len, 1, len);
>> +}
>> +
>> void s390_ipl_prepare_cpu(S390CPU *cpu)
>> {
>> S390IPLState *ipl = get_ipl_device();
>> @@ -418,8 +433,9 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
>> error_report_err(err);
>> vm_stop(RUN_STATE_INTERNAL_ERROR);
>> }
>> -ipl->iplb.ccw.netboot_start_addr = cpu_to_be64(ipl->start_addr);
>> +ipl->qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr);
>> }
>> +s390_ipl_prepare_qipl(cpu);
>> }
>>
>> static void s390_ipl_reset(DeviceState *dev)
>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>> index 8a705e0..08926a3 100644
>> --- a/hw/s390x/ipl.h
>> +++ b/hw/s390x/ipl.h
>> @@ -16,8 +16,7 @@
>> #include "cpu.h"
>>
>> struct IplBlockCcw {
>> -uint64_t netboot_start_addr;
>> -uint8_t reserved0[77];
>> +uint8_t reserved0[85];
>> uint8_t ssid;
>> uint16_t devno;
>> uint8_t vm_flags;
>> @@ -90,6 +89,27 @@ void s390_ipl_prepare_cpu(S390CPU *cpu);
>> IplParameterBlock *s390_ipl_get_iplb(void);
>> void s390_reipl_request(void);
>>
>> +#define QIPL_ADDRESS 0xcc
>> +
>> +/*
>> + * The QEMU IPL Parameters will be stored at absolute address
>> + * 204 (0xcc) which means it is 32-bit word aligned but not
>> + * double-word aligned.
>> + * Placement of data fields in this area must account for
>> + * their alignment needs. E.g., netboot_start_address must
>> + * have an offset of n * 8 bytes within the struct in order
>> + * to keep it double-word aligned.
>
> Should that rather be "4 + n * 8" instead of "n * 8" ?
I wonder if I ever get that comment right. You're correct of course.
>
> Apart from that, patch looks good to me now, so once you've fixed the
> comment (if necessary):
>
> Reviewed-by: Thomas Huth <th...@redhat.com>
>
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [PATCH v8 00/13] Interactive Boot Menu for DASD and SCSI Guests on s390x
On 22.02.2018 12:51, Christian Borntraeger wrote: > Series > Acked-by: Christian Borntraeger <borntrae...@de.ibm.com> > > > menu on scsi and dasd bootmaps tested successfully. > > There is one thing that we might want to fix (can be an addon patch since > this is a non-customer > scenario (no libvirt)). > > If you start QEMU manually without a bootindex, the -boot menu=on is ignored > if no drive has a bootindex. > > For example: > > -drive file=/dev/dasda,if=none,id=d1 -device > virtio-blk-ccw,drive=d1,bootindex=1 -boot menu=on > does work > > -drive file=/dev/dasda -boot menu=on > does not > > instead it prints: > qemu-system-s390x: boot menu is not supported for this device type. > > and the boots up the default entry. > That should indeed be a separate patch, as it would move logic currently in the BIOS up to QEMU (find the first defined virtio disk and select it as boot disk). In fact it's more complicated than that, because it would have to properly account for -boot order=[acdn] and produce the respective IPLB. While it makes sense, I wouldn't rush that in but rather change the error message to indicate that -device bootindex is needed to activate the menu, at least for the time being. [...] -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [PATCH v7 06/12] s390-ccw: parse and set boot menu options
On 19.02.2018 21:39, Collin L. Walling wrote:
> On 02/19/2018 10:52 AM, Viktor Mihajlovski wrote:
>> On 16.02.2018 23:07, Collin L. Walling wrote:
>> [...]
>>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>>> index 74469b1..f632c59 100644
>>> --- a/hw/s390x/ipl.h
>>> +++ b/hw/s390x/ipl.h
>>> @@ -60,6 +60,9 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>>>
>>> #define QIPL_ADDRESS 0xcc
>>>
>>> +#define BOOT_MENU_FLAG_CMD_OPTS 0x80
>>> +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
>>> +
>>> /*
>>> * The QEMU IPL Parameters will be stored 32-bit word aligned.
>>> * Placement of data fields in this area must account for
>>> @@ -67,9 +70,11 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>>> * The entire structure must not be larger than 28 bytes.
>>> */
>>> struct QemuIplParameters {
>>> - uint8_t reserved1[4];
>>> + uint8_t boot_menu_flags;
>>> + uint8_t reserved1[3];
>>> + uint32_t boot_menu_timeout;
>>> uint64_t netboot_start_addr;
>>> - uint8_t reserved2[16];
>>> + uint8_t reserved2[12];
>>> } QEMU_PACKED;Since this has to be touched anyway to re-establish
>>> proper alignment, I
>> could also imagine to define the struct as
>> struct QemuIplParameters {
>> struct {
>> uint32_t flags:8;
>> uint32_t timeout:24;
>> } QEMU_PACKED boot_menu;
>> uint64_t netboot_start_addr;
>> uint8_t reserved2[16];
>> } QEMU_PACKED;
>> would allow to keep the boot menu stuff together without creating
>> unnecessary holes.
>> It would allow for a timeout value of more than 4 hours. The code to set
>> the boot menu would have to be adapted though to properly deal with the
>> bitfields.
>
> I'm currently trying to wrap my brain aroundendian conversion with bit
> fields.
> I'll investigate the best way to handle this in the mean time, but we
> could also
> consider the following:
>
> If neighboring related fields is important, how about moving the fields
> below netboot?
>
> struct QemuIplParameters {
> uint8_t reserved1[4];
> uint64_t netboot_start_addr;
> uint32_t boot_menu_timeout;
> uint8_t boot_menu_flags;
> uint8_t reserved2[11];
> } QEMU_PACKED;
>
I didn't consider the le/be ramifications. They can be dealt with, but
simple is definitely better as we could see in the discussion. No
concerns from my side regarding space.
Another possibility is having a uint8_t field (qipl_flags?) at the
beginning of the struct that could hold the boot menu and other QEMU IPL
flags to come (if any). I.e.
uint8_t qipl_flags;
uint8_t reserved1[3];
uint64_t netboot_start_addr;
uint32_t boot_menu_timeout;
...
and then use a prefix of QIPL_FLAG_ or so. But that's really only a
matter of taste, so whatever you decide is OK for me.
But while examining this file I noticed that I've put the
QemuIplParameters just before the IplParamenterBlock, since I planned to
use it as a member there. As the intention is to use it stand-alone now,
it could be moved somewhere else, e.g. trailing the IplParameterBlock,
which would improve the readability.
>
> If we're concerned about space, we could retreat to timeout as a 16-bit
> field
> (and also bring back the ms -> seconds conversion business)
>
> struct QemuIplParameters {
> uint8_t boot_menu_flags;
> uint8_t reserved;
> uint16_t boot_menu_timeout;
> uint64_t netboot_start_addr;
> uint8_t reserved2[16];
> } QEMU_PACKED;
> [...]
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [qemu-s390x] [PATCH v7 05/12] s390-ccw: move auxiliary IPL data to separate location
On 17.02.2018 09:11, Thomas Huth wrote: [...] > > I still think that the information should *not* be stored within the > IplParameterBlock to avoid that we pass it via DIAG 0x308, too. > If we do it like this, I'm pretty sure that we will look at this code in > a couple of years and wonder whether we can change it again or whether > this is an established interface between the host and the guest. So > please, let's avoid establishing such "hidden" interfaces just out of > current convenience. There must be a better location for this. > Christian, do you have an idea? > > Thomas > In principle I do agree, although I think we can manage the host/guest boundary. If we believe we can't, we have a much bigger problem (looking at the position of the iplb smack in the middle of the s390-ipl state). The main reason why I didn't bother to introduce a new private field in s390-ipl was that I expect more changes to the IPL area in the future (earlier than in a couple of years) and am not really sure whether this QEMU <-> BIOS interface will remain the same and how much effort to spend on it. The major point of this change is to move non-standard data out of the guest-visible IPLB to avoid compatibility problems in the future, while still catering for features like network boot and boot menus. I have no bias against other solutions achieving this objective. If you and Christian think we need a new field, it's all right with me. -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [qemu-s390x] [PATCH v7 06/12] s390-ccw: parse and set boot menu options
On 17.02.2018 09:26, Thomas Huth wrote:
[...]
>> struct QemuIplParameters {
>> -uint8_t reserved1[4];
>> +uint8_t boot_menu_flags;
>> +uint8_t reserved1[3];
>> +uint32_t boot_menu_timeout;
>> uint64_t netboot_start_addr;
>> -uint8_t reserved2[16];
>> +uint8_t reserved2[12];
>> } __attribute__ ((packed));
>> typedef struct QemuIplParameters QemuIplParameters;
>
> I think Victor's original intention was to get netboot_start_addr
> aligned in the lowcore memory. Now it's rather aligned in the host
> memory. Quite confusing, but I think I'd rather prefer Victor's idea to
> keep it aligned in the lowcore (since that's the "architected" part).
>
> Maybe it's better if we do not declare this as a packed struct at all,
> and then instead of doing a memcpy of the whole struct, we set the
> fields manually one by one on the host side into the lowcore, and read
> the fields manually one by one on the guest side? That's more
> cumbersome, but avoids future confusion about the alignments here...
>
> Thomas
>
Hm ... I would prefer to keep it all together and perhaps come up with
better comments (for the fields). BTW: I think it would make sense to
reserve the last 8 bytes 'seriously': in case more global configuration
data is needed in the future, we should have the possibility to install
a pointer to an extension block in there.
Anyway, here's the follup squash-in for a qipl-free IPLB.
---
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 3c6a411..fe70008 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -222,7 +222,7 @@ static Property s390_ipl_properties[] = {
DEFINE_PROP_END_OF_LIST(),
};
-static void s390_ipl_set_boot_menu(IplParameterBlock *iplb)
+static void s390_ipl_set_boot_menu(S390IPLState *ipl)
{
QemuOptsList *plist = qemu_find_opts("boot-opts");
QemuOpts *opts = QTAILQ_FIRST(>head);
@@ -231,11 +231,11 @@ static void s390_ipl_set_boot_menu(IplParameterBlock
*iplb)
const char *tmp;
unsigned long splash_time = 0;
-switch (iplb->pbt) {
+switch (ipl->iplb.pbt) {
case S390_IPL_TYPE_CCW:
case S390_IPL_TYPE_QEMU_SCSI:
-flags = >qipl.boot_menu_flags;
-timeout = >qipl.boot_menu_timeout;
+flags = >qipl.boot_menu_flags;
+timeout = >qipl.boot_menu_timeout;
break;
default:
error_report("boot menu is not supported for this device type.");
@@ -482,7 +482,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
}
ipl->qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr);
}
-s390_ipl_set_boot_menu(>iplb);
+s390_ipl_set_boot_menu(ipl);
s390_ipl_prepare_qipl(cpu);
}
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [qemu-s390x] [PATCH v7 05/12] s390-ccw: move auxiliary IPL data to separate location
On 19.02.2018 09:50, Viktor Mihajlovski wrote:
> On 17.02.2018 09:11, Thomas Huth wrote:
> [...]
>>
>> I still think that the information should *not* be stored within the
>> IplParameterBlock to avoid that we pass it via DIAG 0x308, too.
>> If we do it like this, I'm pretty sure that we will look at this code in
>> a couple of years and wonder whether we can change it again or whether
>> this is an established interface between the host and the guest. So
>> please, let's avoid establishing such "hidden" interfaces just out of
>> current convenience. There must be a better location for this.
>> Christian, do you have an idea?
>>
>> Thomas
>>
> In principle I do agree, although I think we can manage the host/guest
> boundary. If we believe we can't, we have a much bigger problem (looking
> at the position of the iplb smack in the middle of the s390-ipl state).
>
> The main reason why I didn't bother to introduce a new private field in
> s390-ipl was that I expect more changes to the IPL area in the future
> (earlier than in a couple of years) and am not really sure whether this
> QEMU <-> BIOS interface will remain the same and how much effort to
> spend on it.
>
> The major point of this change is to move non-standard data out of the
> guest-visible IPLB to avoid compatibility problems in the future, while
> still catering for features like network boot and boot menus. I have no
> bias against other solutions achieving this objective. If you and
> Christian think we need a new field, it's all right with me.
>
With below squashed in (and a subsequent fixup for the next patch), we can
logically separate the QEMU IPL parameter from the IPLB. Better?
---
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 31565ce..c5923b5 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -410,7 +410,7 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
error_report("Cannot set QEMU IPL parameters");
return;
}
-memcpy(addr + QIPL_ADDRESS, >iplb.qipl, sizeof(QemuIplParameters));
+memcpy(addr + QIPL_ADDRESS, >qipl, sizeof(QemuIplParameters));
cpu_physical_memory_unmap(addr, len, 1, len);
}
@@ -433,7 +433,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
error_report_err(err);
vm_stop(RUN_STATE_INTERNAL_ERROR);
}
-ipl->iplb.qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr);
+ipl->qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr);
}
s390_ipl_prepare_qipl(cpu);
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index 74469b1..775d363 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -88,7 +88,6 @@ union IplParameterBlock {
IplBlockFcp fcp;
IplBlockQemuScsi scsi;
};
-QemuIplParameters qipl;
} QEMU_PACKED;
struct {
uint8_t reserved1[110];
@@ -120,6 +119,7 @@ struct S390IPLState {
bool iplb_valid;
bool reipl_requested;
bool netboot;
+QemuIplParameters qipl;
/*< public >*/
char *kernel;
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [PATCH v7 06/12] s390-ccw: parse and set boot menu options
On 16.02.2018 23:07, Collin L. Walling wrote:
[...]
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index 74469b1..f632c59 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -60,6 +60,9 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>
> #define QIPL_ADDRESS 0xcc
>
> +#define BOOT_MENU_FLAG_CMD_OPTS 0x80
> +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
> +
> /*
> * The QEMU IPL Parameters will be stored 32-bit word aligned.
> * Placement of data fields in this area must account for
> @@ -67,9 +70,11 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
> * The entire structure must not be larger than 28 bytes.
> */
> struct QemuIplParameters {
> -uint8_t reserved1[4];
> +uint8_t boot_menu_flags;
> +uint8_t reserved1[3];
> +uint32_t boot_menu_timeout;
> uint64_t netboot_start_addr;
> -uint8_t reserved2[16];
> +uint8_t reserved2[12];
> } QEMU_PACKED;Since this has to be touched anyway to re-establish proper
> alignment, I
could also imagine to define the struct as
struct QemuIplParameters {
struct {
uint32_t flags:8;
uint32_t timeout:24;
} QEMU_PACKED boot_menu;
uint64_t netboot_start_addr;
uint8_t reserved2[16];
} QEMU_PACKED;
would allow to keep the boot menu stuff together without creating
unnecessary holes.
It would allow for a timeout value of more than 4 hours. The code to set
the boot menu would have to be adapted though to properly deal with the
bitfields.
> typedef struct QemuIplParameters QemuIplParameters;
>
> diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
> index a23237e..0e39aa0 100644
> --- a/pc-bios/s390-ccw/iplb.h
> +++ b/pc-bios/s390-ccw/iplb.h
> @@ -81,9 +81,11 @@ extern IplParameterBlock iplb
> __attribute__((__aligned__(PAGE_SIZE)));
> * The entire structure must not be larger than 28 bytes.
> */
> struct QemuIplParameters {
> -uint8_t reserved1[4];
> +uint8_t boot_menu_flags;
> +uint8_t reserved1[3];
> +uint32_t boot_menu_timeout;
> uint64_t netboot_start_addr;
> -uint8_t reserved2[16];
> +uint8_t reserved2[12];
> } __attribute__ ((packed));
> typedef struct QemuIplParameters QemuIplParameters;
>
same here.
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [PATCH v7 05/12] s390-ccw: move auxiliary IPL data to separate location
On 16.02.2018 23:07, Collin L. Walling wrote:
[...]
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index 8a705e0..74469b1 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -16,8 +16,7 @@
> #include "cpu.h"
>
> struct IplBlockCcw {
> -uint64_t netboot_start_addr;
> -uint8_t reserved0[77];
> +uint8_t reserved0[85];
> uint8_t ssid;
> uint16_t devno;
> uint8_t vm_flags;
> @@ -59,6 +58,21 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>
> #define DIAG308_FLAGS_LP_VALID 0x80
>
> +#define QIPL_ADDRESS 0xcc
> +
> +/*
> + * The QEMU IPL Parameters will be stored 32-bit word aligned.
> + * Placement of data fields in this area must account for
> + * their alignment needs.
> + * The entire structure must not be larger than 28 bytes.
> + */
I realize that my initial suggestion was flawed. Hopefully better:
/*
* The QEMU IPL Parameters will be stored at absolute address
* 204 (0xcc) which means it is 32-bit word aligned but not
* double-word aligned.
* Placement of data fields in this area must account for
* their alignment needs. E.g., netboot_start_address must
* have an offset of n * 8 bytes within the struct in order
* to keep it double-word aligned.
* The total size of the struct must never exceed 28 bytes.
* This definition must be kept in sync with the defininition
* in pc-bios/s390-ccw/iplb.h.
*/
> +struct QemuIplParameters {
> +uint8_t reserved1[4];
> +uint64_t netboot_start_addr;
> +uint8_t reserved2[16];
> +} QEMU_PACKED;
> +typedef struct QemuIplParameters QemuIplParameters;
> +
> union IplParameterBlock {
> struct {
> uint32_t len;
[...]
> diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
> index 890aed9..a23237e 100644
> --- a/pc-bios/s390-ccw/iplb.h
> +++ b/pc-bios/s390-ccw/iplb.h
> @@ -13,8 +13,7 @@
> #define IPLB_H
>
> struct IplBlockCcw {
> -uint64_t netboot_start_addr;
> -uint8_t reserved0[77];
> +uint8_t reserved0[85];
> uint8_t ssid;
> uint16_t devno;
> uint8_t vm_flags;
> @@ -73,6 +72,23 @@ typedef struct IplParameterBlock IplParameterBlock;
>
> extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
>
> +#define QIPL_ADDRESS 0xcc
> +
> +/*
> + * The QEMU IPL Parameters will be stored 32-bit word aligned.
> + * Placement of data fields in this area must account for
> + * their alignment needs.
> + * The entire structure must not be larger than 28 bytes.
> + */
The comment can probably be a bit more terse, avoiding text duplication
and potential inconsistencies arising thereof.
/*
* This definition must be kept in sync with the defininition
* in hw/s390x/ipl.h
*/
> +struct QemuIplParameters {
> +uint8_t reserved1[4];
> +uint64_t netboot_start_addr;
> +uint8_t reserved2[16];
> +} __attribute__ ((packed));
> +typedef struct QemuIplParameters QemuIplParameters;
> +
> +extern QemuIplParameters qipl;
> +
> #define S390_IPL_TYPE_FCP 0x00
> #define S390_IPL_TYPE_CCW 0x02
> #define S390_IPL_TYPE_QEMU_SCSI 0xff
[...]
--
Regards,
Viktor Mihajlovski
[Qemu-devel] [PATCHv5 4/5] qemu-doc: deprecate query-cpus
Start the deprecation period for QAPI query-cpus (replaced by
query-cpus-fast) beginning with 2.12.0.
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
---
qapi-schema.json | 4
qemu-doc.texi| 4
2 files changed, 8 insertions(+)
diff --git a/qapi-schema.json b/qapi-schema.json
index e6ca63f..cd98a94 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -587,6 +587,10 @@
# ]
#}
#
+# Notes: This interface is deprecated (since 2.12.0), and it is strongly
+#recommended that you avoid using it. Use @query-cpus-fast to
+#obtain information about virtual CPUs.
+#
##
{ 'command': 'query-cpus', 'returns': ['CpuInfo'] }
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 137f581..221f565 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2764,6 +2764,10 @@ by the ``convert -l snapshot_param'' argument instead.
"autoload" parameter is now ignored. All bitmaps are automatically loaded
from qcow2 images.
+@subsection query-cpus (since 2.12.0)
+
+The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
+
@section System emulator human monitor commands
@subsection host_net_add (since 2.10.0)
--
1.9.1
[Qemu-devel] [PATCHv5 1/5] qmp: expose s390-specific CPU info
Presently s390x is the only architecture not exposing specific
CPU information via QMP query-cpus. Upstream discussion has shown
that it could make sense to report the architecture specific CPU
state, e.g. to detect that a CPU has been stopped.
With this change the output of query-cpus will look like this on
s390:
[
{"arch": "s390", "current": true,
"props": {"core-id": 0}, "cpu-state": "operating", "CPU": 0,
"qom_path": "/machine/unattached/device[0]",
"halted": false, "thread_id": 63115},
{"arch": "s390", "current": false,
"props": {"core-id": 1}, "cpu-state": "stopped", "CPU": 1,
"qom_path": "/machine/unattached/device[1]",
"halted": true, "thread_id": 63116}
]
This change doesn't add the s390-specific data to HMP 'info cpus'.
A follow-on patch will remove all architecture specific information
from there.
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
Reviewed-by: David Hildenbrand <da...@redhat.com>
Reviewed-by: Christian Borntraeger <borntrae...@de.ibm.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
---
cpus.c | 6 ++
hw/intc/s390_flic.c| 4 ++--
hw/s390x/s390-virtio-ccw.c | 2 +-
qapi-schema.json | 28 +++-
target/s390x/cpu.c | 24
target/s390x/cpu.h | 7 ++-
target/s390x/kvm.c | 8
target/s390x/sigp.c| 38 +++---
8 files changed, 73 insertions(+), 44 deletions(-)
diff --git a/cpus.c b/cpus.c
index f298b65..6006931 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2100,6 +2100,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
#elif defined(TARGET_TRICORE)
TriCoreCPU *tricore_cpu = TRICORE_CPU(cpu);
CPUTriCoreState *env = _cpu->env;
+#elif defined(TARGET_S390X)
+S390CPU *s390_cpu = S390_CPU(cpu);
+CPUS390XState *env = _cpu->env;
#endif
cpu_synchronize_state(cpu);
@@ -2127,6 +2130,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
#elif defined(TARGET_TRICORE)
info->value->arch = CPU_INFO_ARCH_TRICORE;
info->value->u.tricore.PC = env->PC;
+#elif defined(TARGET_S390X)
+info->value->arch = CPU_INFO_ARCH_S390;
+info->value->u.s390.cpu_state = env->cpu_state;
#else
info->value->arch = CPU_INFO_ARCH_OTHER;
#endif
diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
index a85a149..5f8168f 100644
--- a/hw/intc/s390_flic.c
+++ b/hw/intc/s390_flic.c
@@ -192,8 +192,8 @@ static void qemu_s390_flic_notify(uint32_t type)
cs->interrupt_request |= CPU_INTERRUPT_HARD;
/* ignore CPUs that are not sleeping */
-if (s390_cpu_get_state(cpu) != CPU_STATE_OPERATING &&
-s390_cpu_get_state(cpu) != CPU_STATE_LOAD) {
+if (s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING &&
+s390_cpu_get_state(cpu) != S390_CPU_STATE_LOAD) {
continue;
}
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 4abbe89..4d0c3de 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -368,7 +368,7 @@ static void s390_machine_reset(void)
/* all cpus are stopped - configure and start the ipl cpu only */
s390_ipl_prepare_cpu(ipl_cpu);
-s390_cpu_set_state(CPU_STATE_OPERATING, ipl_cpu);
+s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu);
}
static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
diff --git a/qapi-schema.json b/qapi-schema.json
index 0262b9f..94d560e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -410,10 +410,12 @@
# An enumeration of cpu types that enable additional information during
# @query-cpus.
#
+# @s390: since 2.12
+#
# Since: 2.6
##
{ 'enum': 'CpuInfoArch',
- 'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 'other' ] }
+ 'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 's390', 'other' ] }
##
# @CpuInfo:
@@ -452,6 +454,7 @@
'ppc': 'CpuInfoPPC',
'mips': 'CpuInfoMIPS',
'tricore': 'CpuInfoTricore',
+'s390': 'CpuInfoS390',
'other': 'CpuInfoOther' } }
##
@@ -522,6 +525,29 @@
{ 'struct': 'CpuInfoOther', 'data': { } }
##
+# @CpuS390State:
+#
+# An enumeration of cpu states that can be assumed by a virtual
+# S390 CPU
+#
+# Since: 2.12
+##
+{ 'enum': 'CpuS390State',
+ 'prefix': 'S390_CPU_STATE',
+ 'data': [ 'uninitialized', 'stopped', 'check-stop', 'operating', 'load' ] }
+
+##
+# @CpuInfoS390:
+#
+# Additional information about a virtual S390 CPU
+#
+# @cpu-state: the virtual CPU's state
+#
+# Since: 2.12
+##
+{ 'struct': 'CpuInfoS390', 'data'
[Qemu-devel] [PATCHv5 3/5] qmp: add architecture specific cpu data for query-cpus-fast
The s390 CPU state can be retrieved without interrupting the
VM execution. Extendend the CpuInfoFast union with architecture
specific data and an implementation for s390.
Return data looks like this:
[
{"thread-id":64301,"props":{"core-id":0},
"arch":"s390","cpu-state":"operating",
"qom-path":"/machine/unattached/device[0]","cpu-index":0},
{"thread-id":64302,"props":{"core-id":1},
"arch":"s390","cpu-state":"operating",
"qom-path":"/machine/unattached/device[1]","cpu-index":1}
]
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <coh...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
---
cpus.c | 10 ++
qapi-schema.json | 25 ++---
2 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/cpus.c b/cpus.c
index 6df6660..af67826 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2166,6 +2166,10 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
MachineClass *mc = MACHINE_GET_CLASS(ms);
CpuInfoFastList *head = NULL, *cur_item = NULL;
CPUState *cpu;
+#if defined(TARGET_S390X)
+S390CPU *s390_cpu;
+CPUS390XState *env;
+#endif
CPU_FOREACH(cpu) {
CpuInfoFastList *info = g_malloc0(sizeof(*info));
@@ -2183,6 +2187,12 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
info->value->props = props;
}
+#if defined(TARGET_S390X)
+s390_cpu = S390_CPU(cpu);
+env = _cpu->env;
+info->value->arch = CPU_INFO_ARCH_S390;
+info->value->u.s390.cpu_state = env->cpu_state;
+#endif
if (!cur_item) {
head = cur_item = info;
} else {
diff --git a/qapi-schema.json b/qapi-schema.json
index 815f072..e6ca63f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -408,7 +408,7 @@
# @CpuInfoArch:
#
# An enumeration of cpu types that enable additional information during
-# @query-cpus.
+# @query-cpus and @query-cpus-fast.
#
# @s390: since 2.12
#
@@ -604,12 +604,24 @@
# @props: properties describing to which node/socket/core/thread
# virtual CPU belongs to, provided if supported by board
#
+# @arch: architecture of the cpu, which determines which additional fields
+#will be listed
+#
# Since: 2.12
#
##
-{ 'struct': 'CpuInfoFast',
- 'data': {'cpu-index': 'int', 'qom-path': 'str',
- 'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
+{ 'union': 'CpuInfoFast',
+ 'base': {'cpu-index': 'int', 'qom-path': 'str',
+ 'thread-id': 'int', '*props': 'CpuInstanceProperties',
+ 'arch': 'CpuInfoArch' },
+ 'discriminator': 'arch',
+ 'data': { 'x86': 'CpuInfoOther',
+'sparc': 'CpuInfoOther',
+'ppc': 'CpuInfoOther',
+'mips': 'CpuInfoOther',
+'tricore': 'CpuInfoOther',
+'s390': 'CpuInfoS390',
+'other': 'CpuInfoOther' } }
##
# @query-cpus-fast:
@@ -620,9 +632,6 @@
#
# Returns: list of @CpuInfoFast
#
-# Notes: The CPU architecture name is not returned by query-cpus-fast.
-#Use query-target to retrieve that information.
-#
# Since: 2.12
#
# Example:
@@ -637,6 +646,7 @@
# "socket-id": 0
# },
# "qom-path": "/machine/unattached/device[0]",
+# "arch":"x86",
# "cpu-index": 0
# },
# {
@@ -647,6 +657,7 @@
# "socket-id": 1
# },
# "qom-path": "/machine/unattached/device[2]",
+# "arch":"x86",
# "cpu-index": 1
# }
# ]
--
1.9.1
[Qemu-devel] [PATCHv5 5/5] hmp: change hmp_info_cpus to use query-cpus-fast
Changing the implementation of hmp_info_cpus() to call
qmp_query_cpus_fast() instead of qmp_query_cpus. This has the
following consequences:
o No further code change required for qmp_query_cpus deprecation
o HMP profits from the less disruptive cpu information retrieval
o HMP 'info cpus' won't display architecture specific data anymore,
which should be tolerable in the light of the deprecation of
query-cpus.
In order to allow 'info cpus' to be executed completely on the
fast path, monitor_get_cpu_index() has been adapted to not synchronize
the cpu state.
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
hmp.c | 41 +++--
monitor.c | 13 ++---
2 files changed, 17 insertions(+), 37 deletions(-)
diff --git a/hmp.c b/hmp.c
index 7870d6a..ae86bfb 100644
--- a/hmp.c
+++ b/hmp.c
@@ -360,50 +360,23 @@ void hmp_info_migrate_cache_size(Monitor *mon, const
QDict *qdict)
void hmp_info_cpus(Monitor *mon, const QDict *qdict)
{
-CpuInfoList *cpu_list, *cpu;
+CpuInfoFastList *cpu_list, *cpu;
-cpu_list = qmp_query_cpus(NULL);
+cpu_list = qmp_query_cpus_fast(NULL);
for (cpu = cpu_list; cpu; cpu = cpu->next) {
int active = ' ';
-if (cpu->value->CPU == monitor_get_cpu_index()) {
+if (cpu->value->cpu_index == monitor_get_cpu_index()) {
active = '*';
}
-monitor_printf(mon, "%c CPU #%" PRId64 ":", active, cpu->value->CPU);
-
-switch (cpu->value->arch) {
-case CPU_INFO_ARCH_X86:
-monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->u.x86.pc);
-break;
-case CPU_INFO_ARCH_PPC:
-monitor_printf(mon, " nip=0x%016" PRIx64, cpu->value->u.ppc.nip);
-break;
-case CPU_INFO_ARCH_SPARC:
-monitor_printf(mon, " pc=0x%016" PRIx64,
- cpu->value->u.q_sparc.pc);
-monitor_printf(mon, " npc=0x%016" PRIx64,
- cpu->value->u.q_sparc.npc);
-break;
-case CPU_INFO_ARCH_MIPS:
-monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.q_mips.PC);
-break;
-case CPU_INFO_ARCH_TRICORE:
-monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore.PC);
-break;
-default:
-break;
-}
-
-if (cpu->value->halted) {
-monitor_printf(mon, " (halted)");
-}
-
-monitor_printf(mon, " thread_id=%" PRId64 "\n", cpu->value->thread_id);
+monitor_printf(mon, "%c CPU #%" PRId64 ":", active,
+ cpu->value->cpu_index);
+monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
}
-qapi_free_CpuInfoList(cpu_list);
+qapi_free_CpuInfoFastList(cpu_list);
}
static void print_block_info(Monitor *mon, BlockInfo *info,
diff --git a/monitor.c b/monitor.c
index f499250..db57241 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1055,7 +1055,7 @@ int monitor_set_cpu(int cpu_index)
return 0;
}
-CPUState *mon_get_cpu(void)
+static CPUState *mon_get_cpu_sync(bool synchronize)
{
CPUState *cpu;
@@ -1074,10 +1074,17 @@ CPUState *mon_get_cpu(void)
monitor_set_cpu(first_cpu->cpu_index);
cpu = first_cpu;
}
-cpu_synchronize_state(cpu);
+if (synchronize) {
+cpu_synchronize_state(cpu);
+}
return cpu;
}
+CPUState *mon_get_cpu(void)
+{
+return mon_get_cpu_sync(true);
+}
+
CPUArchState *mon_get_cpu_env(void)
{
CPUState *cs = mon_get_cpu();
@@ -1087,7 +1094,7 @@ CPUArchState *mon_get_cpu_env(void)
int monitor_get_cpu_index(void)
{
-CPUState *cs = mon_get_cpu();
+CPUState *cs = mon_get_cpu_sync(false);
return cs ? cs->cpu_index : UNASSIGNED_CPU_INDEX;
}
--
1.9.1
Re: [Qemu-devel] [qemu-s390x] [PATCH v6 06/12] s390-ccw: parse and set boot menu options
On 16.02.2018 17:20, Thomas Huth wrote:
[...]
>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>> index cab8a97..7c3cab8 100644
>> --- a/hw/s390x/ipl.h
>> +++ b/hw/s390x/ipl.h
>> @@ -60,10 +60,15 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>>
>> #define QIPL_ADDRESS 0xcc
>>
>> +#define BOOT_MENU_FLAG_CMD_OPTS 0x80
>> +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
>> +
>> struct QemuIplParameters {
>> -uint8_t reserved1[4];
>> +uint8_t boot_menu_flags;
>> +uint8_t reserved1;
>> +uint32_t boot_menu_timeout;
>> uint64_t netboot_start_addr;
>
> The netboot_start_addr field is now never aligned anymore, neither on
> the host side, nor in guest memory. Not a big problem since the struct
> is declared with "QEMU_PACKED", but still ... it's always nicer to try
> to align fields to their natural boundaries. So maybe move
> boot_menu_flags and reserved1 after netboot_start_addr ?
>
Good catch ... we probably should document the alignment needs and state
that the ipl parameters starts on a word boundary (and that the block
may not be larger than 28 bytes) in a comment block.
>> -uint8_t reserved2[16];
>> +uint8_t reserved2[14];
>> } QEMU_PACKED;
>> typedef struct QemuIplParameters QemuIplParameters;
>
> Thomas
>
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [qemu-s390x] [PATCH v6 06/12] s390-ccw: parse and set boot menu options
On 16.02.2018 17:44, Collin L. Walling wrote:
> On 02/16/2018 11:36 AM, Viktor Mihajlovski wrote:
>> On 16.02.2018 17:20, Thomas Huth wrote:
>> [...]
>>>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>>>> index cab8a97..7c3cab8 100644
>>>> --- a/hw/s390x/ipl.h
>>>> +++ b/hw/s390x/ipl.h
>>>> @@ -60,10 +60,15 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>>>> #define QIPL_ADDRESS 0xcc
>>>> +#define BOOT_MENU_FLAG_CMD_OPTS 0x80
>>>> +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
>>>> +
>>>> struct QemuIplParameters {
>>>> - uint8_t reserved1[4];
>>>> + uint8_t boot_menu_flags;
>>>> + uint8_t reserved1;
>>>> + uint32_t boot_menu_timeout;
>>>> uint64_t netboot_start_addr;
>>> The netboot_start_addr field is now never aligned anymore, neither on
>>> the host side, nor in guest memory. Not a big problem since the struct
>>> is declared with "QEMU_PACKED", but still ... it's always nicer to try
>>> to align fields to their natural boundaries. So maybe move
>>> boot_menu_flags and reserved1 after netboot_start_addr ?
>>>
>> Good catch ... we probably should document the alignment needs and state
>> that the ipl parameters starts on a word boundary (and that the block
>> may not be larger than 28 bytes) in a comment block.
>
> How does this sound?
>
> /* word aligned and cannot exceed 28 bytes */
>
Maybe:
/*
* The Qemu IPL Parameters will be stored 32-bit word aligned.
* Placement of data fields in this area must account for
* their alignment needs.
* The entire structure must not be larger than 28 bytes.
*/
>
>>>> - uint8_t reserved2[16];
>>>> + uint8_t reserved2[14];
>>>> } QEMU_PACKED;
>>>> typedef struct QemuIplParameters QemuIplParameters;
>>> Thomas
>>>
>>
>
>
--
Regards,
Viktor Mihajlovski
[Qemu-devel] [PATCHv5 2/5] qmp: add query-cpus-fast
From: Luiz Capitulino <lcapitul...@redhat.com>
The query-cpus command has an extremely serious side effect:
it always interrupts all running vCPUs so that they can run
ioctl calls. This can cause a huge performance degradation for
some workloads. And most of the information retrieved by the
ioctl calls are not even used by query-cpus.
This commit introduces a replacement for query-cpus called
query-cpus-fast, which has the following features:
o Never interrupt vCPUs threads. query-cpus-fast only returns
vCPU information maintained by QEMU itself, which should be
sufficient for most management software needs
o Drop "halted" field as it can not be retrieved in a fast
way on most architectures
o Drop irrelevant fields such as "current", "pc" and "arch"
o Rename some fields for better clarification & proper naming
standard
Signed-off-by: Luiz Capitulino <lcapitul...@redhat.com>
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
cpus.c | 38 ++
qapi-schema.json | 70
2 files changed, 108 insertions(+)
diff --git a/cpus.c b/cpus.c
index 6006931..6df6660 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2156,6 +2156,44 @@ CpuInfoList *qmp_query_cpus(Error **errp)
return head;
}
+/*
+ * fast means: we NEVER interrupt vCPU threads to retrieve
+ * information from KVM.
+ */
+CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
+{
+MachineState *ms = MACHINE(qdev_get_machine());
+MachineClass *mc = MACHINE_GET_CLASS(ms);
+CpuInfoFastList *head = NULL, *cur_item = NULL;
+CPUState *cpu;
+
+CPU_FOREACH(cpu) {
+CpuInfoFastList *info = g_malloc0(sizeof(*info));
+info->value = g_malloc0(sizeof(*info->value));
+
+info->value->cpu_index = cpu->cpu_index;
+info->value->qom_path = object_get_canonical_path(OBJECT(cpu));
+info->value->thread_id = cpu->thread_id;
+
+info->value->has_props = !!mc->cpu_index_to_instance_props;
+if (info->value->has_props) {
+CpuInstanceProperties *props;
+props = g_malloc0(sizeof(*props));
+*props = mc->cpu_index_to_instance_props(ms, cpu->cpu_index);
+info->value->props = props;
+}
+
+if (!cur_item) {
+head = cur_item = info;
+} else {
+cur_item->next = info;
+cur_item = info;
+}
+}
+
+return head;
+}
+
void qmp_memsave(int64_t addr, int64_t size, const char *filename,
bool has_cpu, int64_t cpu_index, Error **errp)
{
diff --git a/qapi-schema.json b/qapi-schema.json
index 94d560e..815f072 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -552,6 +552,12 @@
#
# Returns a list of information about each virtual CPU.
#
+# This command causes vCPU threads to exit to userspace, which causes
+# a small interruption to guest CPU execution. This will have a negative
+# impact on realtime guests and other latency sensitive guest workloads.
+# It is recommended to use @query-cpus-fast instead of this command to
+# avoid the vCPU interruption.
+#
# Returns: a list of @CpuInfo for each virtual CPU
#
# Since: 0.14.0
@@ -585,6 +591,70 @@
{ 'command': 'query-cpus', 'returns': ['CpuInfo'] }
##
+# @CpuInfoFast:
+#
+# Information about a virtual CPU
+#
+# @cpu-index: index of the virtual CPU
+#
+# @qom-path: path to the CPU object in the QOM tree
+#
+# @thread-id: ID of the underlying host thread
+#
+# @props: properties describing to which node/socket/core/thread
+# virtual CPU belongs to, provided if supported by board
+#
+# Since: 2.12
+#
+##
+{ 'struct': 'CpuInfoFast',
+ 'data': {'cpu-index': 'int', 'qom-path': 'str',
+ 'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
+
+##
+# @query-cpus-fast:
+#
+# Returns information about all virtual CPUs. This command does not
+# incur a performance penalty and should be used in production
+# instead of query-cpus.
+#
+# Returns: list of @CpuInfoFast
+#
+# Notes: The CPU architecture name is not returned by query-cpus-fast.
+#Use query-target to retrieve that information.
+#
+# Since: 2.12
+#
+# Example:
+#
+# -> { "execute": "query-cpus-fast" }
+# <- { "return": [
+# {
+# "thread-id": 25627,
+# "props": {
+# "core-id": 0,
+# "thread-id": 0,
+# "socket-id": 0
+# },
+# "qom-path": "/machine/unattached/device[0]",
+# "cpu-index": 0
+# },
+# {
+# "thread-id": 25628,
+# "props": {
+# "core-id": 0,
+# "thread-id": 0,
+#
[Qemu-devel] [PATCHv5 0/5] add query-cpu-fast and related s390 changes
v5 synopsis: Split out HMP changes from Patch 2 into Patch 5. Please re-review, as I've removed the a-b/r-b from Patch 2 as well. This series consolidates patches around a performance issue caused by the usage of QMP query-cpus. A performance issue was found in an OpenStack environment, where ceilometer was collecting domain statistics with libvirt. The domain statistics reported by libvirt include the vCPU halted state, which in turn is retrieved with QMP query-cpus. This causes two issues: 1. Performance: on most architectures query-cpus needs to issue a KVM ioctl to find out whether a vCPU was halted. This is not the case for s390 but query-cpus is always causing the vCPU to exit the VM. 2. Semantics: on x86 and other architectures, halted is a highly transient state, which is likely to have already changed shortly after the state information has been retrieved. This is not the case for s390, where halted is an indication that the vCPU is stopped, meaning its not available to the guest operating system until it has been restarted. The following patches help to alleviate the issues: Patch 1/5: Adds architecture specific data to the QMP CpuInfo type, exposing the existing s390 cpu-state in QMP. The cpu-state is a representation more adequate than the ambiguous 'halted' condition. Patch 2/5: Adds a new QMP function query-cpus-fast, which will only retrieve vCPU information that can be obtained without interrupting the vCPUs of a running VM. It introduces a new return type CpuInfoFast with the subset of fields meeting this condition. Specifically, the halted state is not part of CpuInfoFast. QMP clients like libvirt are encouraged to switch to the new API for vCPU information. Patch 3/5: Adds the s390-specific cpu state to CpuInfoFast, allowing management apps to find out whether a vCPU is in the stopped state. This extension leads to a partial duplication of field definitions from CpuInfo to CpuInfoFast. This should be tolerable if CpuInfo is deprecated and eventually removed. Patch 4/5: Starts the deprecation of query-cpus. Patch 5/5 (NEW): Changes HMP 'info cpus' to use query-cpus-fast. Was part of 2/5 initially. Series v4 -> v5: Overall: Updated r-b's and moved HMP changes into a seperate patch. Patch 1/5: o Updated commit message to clarify why no HMP change Patch 2/5: o Split out HMP changes o Removed r-b cohuck, a-b eblake Series v3 -> v4: Overall: Instead of adding a new HMP 'info cpus_fast', changed 'info cpus' to use query-cpus-fast directly. Patch 1/4: o Don't report s390-specific data in HMP 'info cpus' Patch 2/4: o Change HMP 'info cpus' to use query-cpus-fast and to return only basic information (no arch-specific data) o Drop HMP 'info cpus_fast' o Fixed typo in commit message Patch 3/4: o Drop HMP-related changes Patch 4/4: o Drop HMP-related changes Series v2 -> v3: Overall: Added r-b's and a-b's. Patch 2/4: o Fixed commit message with respect to the halted field disposition. o Fixed grammar in qapi-schema documentation. Patch 3/4: o Use CpuInfoS390 type for both query-cpus and query-cpus-fast per Eric Blake's comment. o Dropped 'duplication blurb' from commit message as it doesn't provide relevant information other than query-cpus should be deprecated, which is done in the next patch now. Series v1 -> v2: Patch 2/3: o Changed formatting of hmp info cpus_fast to match that of info cpus. This makes it easier for clients to switch to the fast call. Patch 3/3: o Same formatting change for info cpus_fast as in 2/3, only for s390-specific cpu state. Luiz Capitulino (1): qmp: add query-cpus-fast Viktor Mihajlovski (4): qmp: expose s390-specific CPU info qmp: add architecture specific cpu data for query-cpus-fast qemu-doc: deprecate query-cpus hmp: change hmp_info_cpus to use query-cpus-fast cpus.c | 54 + hmp.c | 41 +++- hw/intc/s390_flic.c| 4 +- hw/s390x/s390-virtio-ccw.c | 2 +- monitor.c | 13 +++-- qapi-schema.json | 115 - qemu-doc.texi | 4 ++ target/s390x/cpu.c | 24 +- target/s390x/cpu.h | 7 +-- target/s390x/kvm.c | 8 ++-- target/s390x/sigp.c| 38 +++ 11 files changed, 228 insertions(+), 82 deletions(-) -- 1.9.1
Re: [Qemu-devel] [PATCHv4 2/4] qmp: add query-cpus-fast
On 16.02.2018 15:03, Eric Blake wrote:
> On 02/16/2018 06:04 AM, Viktor Mihajlovski wrote:
>> From: Luiz Capitulino <lcapitul...@redhat.com>
>>
>> The query-cpus command has an extremely serious side effect:
>> it always interrupts all running vCPUs so that they can run
>> ioctl calls. This can cause a huge performance degradation for
>> some workloads. And most of the information retrieved by the
>> ioctl calls are not even used by query-cpus.
>>
>> This commit introduces a replacement for query-cpus called
>> query-cpus-fast, which has the following features:
>>
>> o Never interrupt vCPUs threads. query-cpus-fast only returns
>> vCPU information maintained by QEMU itself, which should be
>> sufficient for most management software needs
>>
>> o Drop "halted" field as it can not be retrieved in a fast
>> way on most architectures
>>
>> o Drop irrelevant fields such as "current", "pc" and "arch"
>>
>> o Rename some fields for better clarification & proper naming
>> standard
>>
>> Further, the HMP "info cpus" implementation is changed to
>> use the new QMP interface to avoid the side effects caused
>> by query-cpus. This means that only a reduced subset of cpu
>> information will be reported, which is acceptable as
>> the content of "info cpus" is not documented or guaranteed
>> to be stable.
>>
>> Signed-off-by: Luiz Capitulino <lcapitul...@redhat.com>
>> Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
>> Reviewed-by: Cornelia Huck <coh...@redhat.com>
>> Acked-by: Eric Blake <ebl...@redhat.com>
>
> The HMP changes are non-trivial compared to v3, so I might have dropped
> all R-b and Acked-by to ensure they are looked at again.
>
> In fact,...
>
>> ---
>> cpus.c | 38 ++
>> hmp.c | 46 +
>> qapi-schema.json | 70
>>
>> 3 files changed, 114 insertions(+), 40 deletions(-)
>>
>
>> +++ b/hmp.c
>> @@ -360,50 +360,16 @@ void hmp_info_migrate_cache_size(Monitor *mon,
>> const QDict *qdict)
>> void hmp_info_cpus(Monitor *mon, const QDict *qdict)
>> {
>> - CpuInfoList *cpu_list, *cpu;
>> + CpuInfoFastList *head, *cpu;
>> - cpu_list = qmp_query_cpus(NULL);
>> + head = qmp_query_cpus_fast(NULL);
>> - for (cpu = cpu_list; cpu; cpu = cpu->next) {
>> - int active = ' ';
>> -
>> - if (cpu->value->CPU == monitor_get_cpu_index()) {
>> - active = '*';
>> - }
>
> The old code was doing multiple things - it was telling you the current
> HMP cpu (HMP has 'cpu' with no QMP counterpart, but if you are using
> HMP, knowing which cpu is the active target for future HMP commands that
> depend on the active target, this bit of information is important), as
> well as...
>
>> - monitor_printf(mon, "%c CPU #%" PRId64 ":", active,
>> cpu->value->CPU);
>> -
>> - switch (cpu->value->arch) {
>> - case CPU_INFO_ARCH_X86:
>> - monitor_printf(mon, " pc=0x%016" PRIx64,
>> cpu->value->u.x86.pc);
>> - break;
>
> ...inundating you with the arch-specific slow stuff. I'm in agreement
> that dropping the slow stuff is okay, but...
>
>> + for (cpu = head; cpu; cpu = cpu->next) {
>> + monitor_printf(mon, " CPU #%" PRId64 ":",
>> cpu->value->cpu_index);
>
> ...unilaterally dropping the '*' active indicator, which has no bearing
> on the QMP changes, and thus is not an appropriate change for this patch>
> I've now gone through the entire patch (and not just the UI), so on the
> next revision, I'll be prepared to give a full R-b, rather than just
> Acked-by. But I do think this means we need a v5.
>
Thanks for the patience. I'll respin with the r/a-b's on patch 2/4
removed, but want to verify first that I can get the active cpu without
triggering cpu_synchronize_state via monitor_get_cpu_index...
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [PATCH v9 13/13] s390-ccw: interactive boot menu for scsi
On 23.02.2018 16:43, Collin L. Walling wrote:
> Interactive boot menu for scsi. This follows a similar procedure
> as the interactive menu for eckd dasd. An example follows:
>
> s390x Enumerated Boot Menu.
>
> 3 entries detected. Select from index 0 to 2.
>
> Signed-off-by: Collin L. Walling <wall...@linux.vnet.ibm.com>
> Reviewed-by: Thomas Huth <th...@redhat.com>
> ---
> hw/s390x/ipl.c | 1 +
> pc-bios/s390-ccw/bootmap.c | 4
> pc-bios/s390-ccw/main.c | 1 +
> pc-bios/s390-ccw/menu.c | 20
> pc-bios/s390-ccw/s390-ccw.h | 2 ++
> 5 files changed, 28 insertions(+)
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index c12e460..edbec90 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -246,6 +246,7 @@ static void s390_ipl_set_boot_menu(S390IPLState *ipl)
> *flags |= QIPL_FLAG_BM_OPTS_ZIPL;
> return;
> }
break still missing here ...
> +case S390_IPL_TYPE_QEMU_SCSI:
> break;
> default:
> error_report("boot menu is not supported for this device type.");
otherwise, LGTM
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [PATCH v9 00/13] Interactive Boot Menu for DASD and SCSI Guests on s390x
On 23.02.2018 16:43, Collin L. Walling wrote: > --- [v9] --- > > - only set boot menu opts if a bootindex was specified on cmd > > - Menus for guests with an IPL device that chooses to use the SCSI scheme are >only enabled explicitly for -boot menu=on options (i.e. it will > appropriately >and cleanly ignore the zipl flag) > > --- [Summary] --- > > These patches implement a boot menu for ECKD DASD and SCSI guests on s390x. > The menu will only appear if the disk has been configured for IPL with the > zIPL tool and with the following QEMU command line options: > > -boot menu=on[,splash-time=X] and/or -machine loadparm='prompt' > > The following must be specified for the device to be IPL'd from: > > -device ...,bootindex=1 > > or via the following libvirt domain xml: > > > > > > or > > > ... > > > > Where X is some positive integer representing time in milliseconds. > > must be specified for the device to be IPL'd from > > A loadparm other than 'prompt' will disable the menu and just boot > the specified entry. > > If no boot options are specified, we will attempt to use the values > provided by zipl (ECKD DASD only). > > Collin L. Walling (13): > s390-ccw: refactor boot map table code > s390-ccw: refactor eckd_block_num to use CHS > s390-ccw: refactor IPL structs > s390-ccw: update libc > s390-ccw: move auxiliary IPL data to separate location > s390-ccw: parse and set boot menu options > s390-ccw: set up interactive boot menu parameters > s390-ccw: read stage2 boot loader data to find menu > s390-ccw: print zipl boot menu > s390-ccw: read user input for boot index via the SCLP console > s390-ccw: set cp_receive mask only when needed and consume pending > service irqs > s390-ccw: use zipl values when no boot menu options are present > s390-ccw: interactive boot menu for scsi > > hw/s390x/ipl.c | 76 +- > hw/s390x/ipl.h | 31 +- > pc-bios/s390-ccw/Makefile | 2 +- > pc-bios/s390-ccw/bootmap.c | 184 +++- > pc-bios/s390-ccw/bootmap.h | 91 ++-- > pc-bios/s390-ccw/iplb.h | 24 - > pc-bios/s390-ccw/libc.c | 88 > pc-bios/s390-ccw/libc.h | 37 ++- > pc-bios/s390-ccw/main.c | 49 ++--- > pc-bios/s390-ccw/menu.c | 249 > > pc-bios/s390-ccw/s390-ccw.h | 10 ++ > pc-bios/s390-ccw/sclp.c | 39 --- > pc-bios/s390-ccw/virtio.c | 2 +- > 13 files changed, 756 insertions(+), 126 deletions(-) > create mode 100644 pc-bios/s390-ccw/libc.c > create mode 100644 pc-bios/s390-ccw/menu.c > Series looks good to me (the break nit in 13/13 notwithstanding). -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [PATCH v8 00/13] Interactive Boot Menu for DASD and SCSI Guests on s390x
On 23.02.2018 11:17, Thomas Huth wrote:
> On 23.02.2018 09:53, Christian Borntraeger wrote:
>> Hmmm, on my ubuntu 16.04 guest, I get the boot menu with no timeout even if
>> I do not
>> specify loadparm or boot menu:
>>
>> qemu-kvm -drive file=/var/lib/libvirt/qemu/image.zhyp140,if=none,id=d1
>> -device virtio-blk-ccw,drive=d1,bootindex=1
>
> I can reproduce this now, too. FWIW: It only happens with
> virtio-blk-ccw, not with virtio-scsi-ccw (that's why I did not notice it
> first). Collin, can you reproduce this, too?
>
> Thomas
> The idea is to support the zipl boot menu on ECKD disks (which have a timeout
> by themselves)
even without switching the boot menu on. This is to have the same behavior as
one would see
on LPAR or z/VM.
The issue is that the boot code tries to interpret the program table of SCSI
bootmaps as if
a boot menu has been specified.
The following patch fixes this. Collin might want to have a say in this matter
as well.
The scsi program table was erroneously evaluated as implicit
boot menu. This patch adds a per-bootmap-type menu_is_enabled
function.
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
pc-bios/s390-ccw/bootmap.c | 4 ++--
pc-bios/s390-ccw/menu.c | 7 ++-
pc-bios/s390-ccw/s390-ccw.h | 3 ++-
3 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 81dbd4a..b6fd77e 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -265,7 +265,7 @@ static void run_eckd_boot_script(block_number_t
bmt_block_nr,
BootMapTable *bmt = (void *)sec;
BootMapScript *bms = (void *)sec;
-if (menu_is_enabled()) {
+if (menu_is_enabled_for_zipl()) {
loadparm = eckd_get_boot_menu_index(s1b_block_nr);
}
@@ -568,7 +568,7 @@ static void ipl_scsi(void)
debug_print_int("program table entries", program_table_entries);
IPL_assert(program_table_entries != 0, "Empty Program Table");
-if (menu_is_enabled()) {
+if (menu_is_enabled_for_enum()) {
loadparm = menu_get_enum_boot_index(program_table_entries);
}
diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index 16b5310..ebe5e9f 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -237,7 +237,12 @@ void menu_set_parms(uint8_t boot_menu_flag, uint32_t
boot_menu_timeout)
timeout = boot_menu_timeout;
}
-bool menu_is_enabled(void)
+bool menu_is_enabled_for_zipl(void)
{
return flag;
}
+
+bool menu_is_enabled_for_enum(void)
+{
+return flag & ~QIPL_FLAG_BM_OPTS_ZIPL;
+}
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index cfcaf3c..a18381f 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -89,7 +89,8 @@ void zipl_load(void);
/* menu.c */
void menu_set_parms(uint8_t boot_menu_flag, uint32_t boot_menu_timeout);
-bool menu_is_enabled(void);
+bool menu_is_enabled_for_zipl(void);
+bool menu_is_enabled_for_enum(void);
int menu_get_zipl_boot_index(const char *menu_data);
int menu_get_enum_boot_index(int entries);
--
1.9.1
--
Regards,
Viktor Mihajlovski
[Qemu-devel] [PATCH 2/3] s390: Ensure IPL from SCSI works as expected
Operating systems may request an IPL from a virtio-scsi device
by specifying an IPL parameter type of CCW. In this case QEMU
won't set up the IPLB correctly. The BIOS will still detect
it's a SCSI device to boot from, but it will now have to search
for the first LUN and attempt to boot from there.
However this may not be the original boot LUN if there's more than
one SCSI disk attached to the HBA.
With this change QEMU will detect that the request is for a
SCSI device and will rebuild the initial IPL parameter info
if it's the SCSI device used for the first boot. In consequence
the BIOS can use the boot LUN from the IPL information block.
In case a different SCSI device has been set, the BIOS will find
and use the first available LUN.
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
hw/s390x/ipl.c | 31 +--
1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 58e33c5..fb554ab 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -427,7 +427,8 @@ unref_mr:
return img_size;
}
-static bool is_virtio_net_device(IplParameterBlock *iplb)
+static bool is_virtio_ccw_device_of_type(IplParameterBlock *iplb,
+ int virtio_id)
{
uint8_t cssid;
uint8_t ssid;
@@ -447,13 +448,23 @@ static bool is_virtio_net_device(IplParameterBlock *iplb)
sch = css_find_subch(1, cssid, ssid, schid);
if (sch && sch->devno == devno) {
-return sch->id.cu_model == VIRTIO_ID_NET;
+return sch->id.cu_model == virtio_id;
}
}
}
return false;
}
+static bool is_virtio_net_device(IplParameterBlock *iplb)
+{
+return is_virtio_ccw_device_of_type(iplb, VIRTIO_ID_NET);
+}
+
+static bool is_virtio_scsi_device(IplParameterBlock *iplb)
+{
+return is_virtio_ccw_device_of_type(iplb, VIRTIO_ID_SCSI);
+}
+
void s390_ipl_update_diag308(IplParameterBlock *iplb)
{
S390IPLState *ipl = get_ipl_device();
@@ -478,6 +489,22 @@ void s390_reipl_request(void)
S390IPLState *ipl = get_ipl_device();
ipl->reipl_requested = true;
+if (ipl->iplb_valid &&
+!ipl->netboot &&
+ipl->iplb.pbt == S390_IPL_TYPE_CCW &&
+is_virtio_scsi_device(>iplb)) {
+CcwDevice *ccw_dev = s390_get_ccw_device(get_boot_device(0));
+
+if (ccw_dev &&
+cpu_to_be16(ccw_dev->sch->devno) == ipl->iplb.ccw.devno &&
+(ccw_dev->sch->ssid & 3) == ipl->iplb.ccw.ssid) {
+/*
+ * this is the original boot device's SCSI
+ * so restore IPL parameter info from it
+ */
+ipl->iplb_valid = s390_gen_initial_iplb(ipl);
+}
+}
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
}
--
1.9.1
[Qemu-devel] [PATCH 3/3] s390: Do not pass inofficial IPL type to the guest
IPL over a virtio-scsi device requires special handling not
available in the real architecture. For this purpose the IPL
type 0xFF has been chosen as means of communication between
QEMU and the pc-bios. However, a guest OS could be confused
by seeing an unknown IPL type.
This change sets the IPL parameter type to 0x02 (CCW) to prevent
this. Pre-existing Linux has looked up the IPL parameters only in
the case of FCP IPL. This means that the behavior should stay
the same even if Linux checks for the IPL type unconditionally.
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
pc-bios/s390-ccw/bootmap.c | 7 +++
pc-bios/s390-ccw/iplb.h| 15 +--
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index fc2a9fe..9287b7a 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -70,6 +70,13 @@ static void jump_to_IPL_code(uint64_t address)
{
/* store the subsystem information _after_ the bootmap was loaded */
write_subsystem_identification();
+
+/* prevent unknown IPL types in the guest */
+if (iplb.pbt == S390_IPL_TYPE_QEMU_SCSI) {
+iplb.pbt = S390_IPL_TYPE_CCW;
+set_iplb();
+}
+
/*
* The IPL PSW is at address 0. We also must not overwrite the
* content of non-BIOS memory after we loaded the guest, so we
diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
index 7dfce4f..5357a36 100644
--- a/pc-bios/s390-ccw/iplb.h
+++ b/pc-bios/s390-ccw/iplb.h
@@ -97,16 +97,27 @@ extern QemuIplParameters qipl;
#define S390_IPL_TYPE_CCW 0x02
#define S390_IPL_TYPE_QEMU_SCSI 0xff
-static inline bool store_iplb(IplParameterBlock *iplb)
+static inline bool manage_iplb(IplParameterBlock *iplb, bool store)
{
register unsigned long addr asm("0") = (unsigned long) iplb;
register unsigned long rc asm("1") = 0;
asm volatile ("diag %0,%2,0x308\n"
: "+d" (addr), "+d" (rc)
- : "d" (6)
+ : "d" (store ? 6 : 5)
: "memory", "cc");
return rc == 0x01;
}
+
+static inline bool store_iplb(IplParameterBlock *iplb)
+{
+return manage_iplb(iplb, true);
+}
+
+static inline bool set_iplb(IplParameterBlock *iplb)
+{
+return manage_iplb(iplb, false);
+}
+
#endif /* IPLB_H */
--
1.9.1
[Qemu-devel] [PATCH 1/3] s390: Refactor IPL parameter block generation
Splitting out the the CCW device extraction allows reuse.
Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com>
---
hw/s390x/ipl.c | 81 --
1 file changed, 51 insertions(+), 30 deletions(-)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index fdeaec3..58e33c5 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -279,44 +279,52 @@ static void s390_ipl_set_boot_menu(S390IPLState *ipl)
*timeout = cpu_to_be32(splash_time);
}
+static CcwDevice *s390_get_ccw_device(DeviceState *dev_st)
+{
+CcwDevice *ccw_dev = NULL;
+
+if (dev_st) {
+VirtioCcwDevice *virtio_ccw_dev = (VirtioCcwDevice *)
+object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->parent),
+TYPE_VIRTIO_CCW_DEVICE);
+if (virtio_ccw_dev) {
+ccw_dev = CCW_DEVICE(virtio_ccw_dev);
+} else {
+SCSIDevice *sd = (SCSIDevice *)
+object_dynamic_cast(OBJECT(dev_st),
+TYPE_SCSI_DEVICE);
+if (sd) {
+SCSIBus *bus = scsi_bus_from_device(sd);
+VirtIOSCSI *vdev = container_of(bus, VirtIOSCSI, bus);
+VirtIOSCSICcw *scsi_ccw = container_of(vdev, VirtIOSCSICcw,
+ vdev);
+
+ccw_dev = (CcwDevice *)object_dynamic_cast(OBJECT(scsi_ccw),
+ TYPE_CCW_DEVICE);
+}
+}
+}
+return ccw_dev;
+}
+
static bool s390_gen_initial_iplb(S390IPLState *ipl)
{
DeviceState *dev_st;
+CcwDevice *ccw_dev = NULL;
dev_st = get_boot_device(0);
if (dev_st) {
-VirtioCcwDevice *virtio_ccw_dev = (VirtioCcwDevice *)
-object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->parent),
-TYPE_VIRTIO_CCW_DEVICE);
+ccw_dev = s390_get_ccw_device(dev_st);
+}
+
+/*
+ * Currently allow IPL only from CCW devices.
+ */
+if (ccw_dev) {
SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st),
TYPE_SCSI_DEVICE);
-VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st),
- TYPE_VIRTIO_NET);
-
-if (vn) {
-ipl->netboot = true;
-}
-if (virtio_ccw_dev) {
-CcwDevice *ccw_dev = CCW_DEVICE(virtio_ccw_dev);
-
-ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
-ipl->iplb.blk0_len =
-cpu_to_be32(S390_IPLB_MIN_CCW_LEN - S390_IPLB_HEADER_LEN);
-ipl->iplb.pbt = S390_IPL_TYPE_CCW;
-ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
-ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3;
-} else if (sd) {
-SCSIBus *bus = scsi_bus_from_device(sd);
-VirtIOSCSI *vdev = container_of(bus, VirtIOSCSI, bus);
-VirtIOSCSICcw *scsi_ccw = container_of(vdev, VirtIOSCSICcw, vdev);
-CcwDevice *ccw_dev;
-
-ccw_dev = (CcwDevice *)object_dynamic_cast(OBJECT(scsi_ccw),
- TYPE_CCW_DEVICE);
-if (!ccw_dev) { /* It might be a PCI device instead */
-return false;
-}
+if (sd) {
ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN);
ipl->iplb.blk0_len =
cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN -
S390_IPLB_HEADER_LEN);
@@ -327,12 +335,25 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno);
ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3;
} else {
-return false; /* unknown device */
+VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st),
+ TYPE_VIRTIO_NET);
+
+ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
+ipl->iplb.blk0_len =
+cpu_to_be32(S390_IPLB_MIN_CCW_LEN - S390_IPLB_HEADER_LEN);
+ipl->iplb.pbt = S390_IPL_TYPE_CCW;
+ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
+ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3;
+
+if (vn) {
+ipl->netboot = true;
+}
}
if (!s390_ipl_set_loadparm(ipl->iplb.loadparm)) {
ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
}
+
return true;
}
--
1.9.1
[Qemu-devel] [PATCH 0/3] s390: Fix virtio-scsi IPL quirks
IPL from virtio-scsi currently uses a non-standard parameter type definition to pass boot parameters from QEMU to the BIOS. There are two potential issues with this approach: o If the guest operating systems requests a re-ipl of type CCW where the boot device is a virtio-scsi HBA, this goes unnoticed by QEMU. The BIOS will detect that it's IPLing from a SCSI device, but it will boot the first LUN found, which might not be the one used for the initial boot. o The guest operating system can be confused by an unknown IPL parameter block type. If the OS hasn't previously used diag308 to store the IPL info but is changed to do so, a user-observable change in behavior will happen. The following patches address the issues above. Viktor Mihajlovski (3): s390: Refactor IPL parameter block generation s390: Ensure IPL from SCSI works as expected s390: Do not pass inofficial IPL type to the guest hw/s390x/ipl.c | 112 - pc-bios/s390-ccw/bootmap.c | 7 +++ pc-bios/s390-ccw/iplb.h| 15 +- 3 files changed, 100 insertions(+), 34 deletions(-) -- 1.9.1
Re: [Qemu-devel] [PATCH for-2.12] hmp.c: Revert hmp_info_cpus output format
On 27.03.2018 16:25, Cornelia Huck wrote: > On Tue, 27 Mar 2018 08:59:04 -0500 > Eric Blake <ebl...@redhat.com> wrote: > >> On 03/27/2018 07:38 AM, sathn...@linux.vnet.ibm.com wrote: >>> From: Satheesh Rajendran <sathn...@linux.vnet.ibm.com> >>> >>> This commit 137b5cb6ab565cb3781d5337591e155932b4230e >>> refactors info cpus output and changes output format from >>> 'thread_id' to 'thread-id', this would break parsing >>> of output in above layers like libvirt, test framework etc. >>> >>> This patch just reverts back output format to 'thread_id'. >>> >>> CC: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com> >>> Signed-off-by: Satheesh Rajendran <sathn...@linux.vnet.ibm.com> >>> --- >>> hmp.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> I'm not opposed to this patch (because it is a trivial way to keep older >> stuff working), but agree with Daniel that HMP output can change at any >> time, so other stuff relying on HMP should be fixed. Libvirt is not >> affected, so if we DO take this into 2.12, we should update the commit >> message to drop mention of libvirt being impacted, maybe along the lines of: >> >> Commit 137b5cb6 refactored 'info cpus' output, changing 'thread_id' to >> 'thread-id'. While HMP is not a stable interface, it is trivial to keep >> the spelling consistent for test frameworks that have not yet updated to >> using QMP. >> >> With the improved commit message, >> Reviewed-by: Eric Blake <ebl...@redhat.com> >> > > Yes, that certainly makes sense. > > Reviewed-by: Cornelia Huck <coh...@redhat.com> > Right. Seems I got carried away during the thread_id to thread-id conversion. -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [PATCH] hw/s390x/ipl: Bail out if the network bootloader can not be found
On 27.02.2018 11:05, Thomas Huth wrote:
> If QEMU fails to load 's390-netboot.img', the guest firmware currently
> loops forever and just floods the console with "Network boot device
> detected" messages. The code in ipl.c apparently already tried to stop
> the VM with vm_stop() in this case, but this is in vain since the run
> state is later reset due to a call to vm_start() from vl.c again.
> To avoid the ugly firmware loop, let's simply exit QEMU directly instead
> since it just does not make sense to continue if the required firmware
> image can not be loaded. While we're at it, also add the file name of
> the netboot binary to the error message, so that the user has a better
> hint about what is missing.
>
> Signed-off-by: Thomas Huth <th...@redhat.com>
> ---
> hw/s390x/ipl.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0d06fc1..ff8308e 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -322,7 +322,8 @@ static int load_netboot_image(Error **errp)
>
> netboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, ipl->netboot_fw);
> if (netboot_filename == NULL) {
> -error_setg(errp, "Could not find network bootloader");
> +error_setg(errp, "Could not find network bootloader '%s'",> +
>ipl->netboot_fw);
> goto unref_mr;
> }
>
> @@ -416,7 +417,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
> if (ipl->netboot) {
> if (load_netboot_image() < 0) {
> error_report_err(err);
> -vm_stop(RUN_STATE_INTERNAL_ERROR);
Should we print something like 'exiting' or 'terminating' here, to make
clear that the situation is terminal? Sometimes errors are reported and
processing continues nonetheless.
> +exit(1);
> }
> ipl->iplb.ccw.netboot_start_addr = cpu_to_be64(ipl->start_addr);
> }
>
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [PATCH] hw/s390x: Add the possibility to specify the netboot image on the command line
On 27.02.2018 12:32, Thomas Huth wrote:
> The file name of the netboot binary is currently hard-coded to
> "s390-netboot.img", without a possibility for the user to select
> an alternative firmware image here. That's unfortunate, especially
> since the basics are already there: The filename is a property of
> the s390-ipl device. So we just have to add a check whether the user
> already provided the property and only set the default if the string
> is still empty. Now it is possible to select a different firmware
> image with "-global s390-ipl.netboot_fw=/path/to/s390-netboot.img".
> While I sympathize with the concept, it will be a bit hard to consume
since most of the QEMU instances will be started by libvirt and that has
no provisions for this kind of firmware replacement. We could craft some
s390-specific variety of the element.
Another thing to consider is that, while the current netboot firmware
doesn't rely on any special QEMU <-> loader interfaces, it might happen
that QEMU and the network firmware must match similar to QEMU and the
s390-ccw firmware image and I have no idea on how to ensure that.
> Signed-off-by: Thomas Huth <th...@redhat.com>
> ---
> hw/s390x/s390-virtio-ccw.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4abbe89..7b3fb5f 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -254,8 +254,10 @@ static void s390_init_ipl_dev(const char
> *kernel_filename,
> }
> qdev_prop_set_string(dev, "cmdline", kernel_cmdline);
> qdev_prop_set_string(dev, "firmware", firmware);
Wouldn't it be consequent to allow firmware replacement? But then, see
above.
> -qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
> qdev_prop_set_bit(dev, "enforce_bios", enforce_bios);
> +if (!strlen(object_property_get_str(new, "netboot_fw", _abort))) {
> +qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
> +}
> object_property_add_child(qdev_get_machine(), TYPE_S390_IPL,
>new, NULL);
> object_unref(new);
>
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [PATCHv5 4/5] qemu-doc: deprecate query-cpus
On 19.02.2018 15:57, Cornelia Huck wrote: > On Fri, 16 Feb 2018 17:08:40 +0100 > Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com> wrote: > >> Start the deprecation period for QAPI query-cpus (replaced by >> query-cpus-fast) beginning with 2.12.0. >> >> Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com> >> Reviewed-by: Eric Blake <ebl...@redhat.com> >> --- >> qapi-schema.json | 4 >> qemu-doc.texi| 4 >> 2 files changed, 8 insertions(+) > > We need to remember updating the deprecation notes in the wiki as well. > > Reviewed-by: Cornelia Huck <coh...@redhat.com> > Let me know whether you plan on doing that or I should take a crack at it. -- Regards, Viktor Mihajlovski
Re: Arch info lost in "info cpus"
On 9/30/19 10:45 AM, Dr. David Alan Gilbert wrote: > * Sergio Lopez (s...@redhat.com) wrote: >> Hi, >> >> Commit 137b5cb6ab565cb3781d5337591e155932b4230e (hmp: change >> hmp_info_cpus to use query-cpus-fast) updated the "info cpus" commit to >> make it more lightweight, but also removed the ability to get the >> architecture specific status of each vCPU. >> >> This information was really useful to diagnose certain Guest issues, >> without the need of using GDB, which is more intrusive and requires >> enabling it in advance. >> >> Is there an alternative way of getting something equivalent to what >> "info cpus" provided previously (in 2.10)? > Even the qemp equivalent, query-cpus is deprecated. > (Although we do call the underlying query-cpus in 'info numa' as well) This obviously went by unnoticed back then. Query-cpus-fast should serve the same purpose as query-cpus there, being less intrusive on the VM and allow to complete the deprecation process, if this is still wanted. If not, adding an option that lets hmp 'info cpus' call the old API doesn't seem unreasonable. > > Dave > >> Thanks, >> Sergio. > > -- > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK > -- Kind Regards, Viktor
Re: [PATCH 00/15] s390x: Protected Virtualization support
On 11/29/19 3:02 PM, Janosch Frank wrote: [...] As a mgmt app I think there will be a need to be able to determine whether a host + QEMU combo is actually able to support protected machines. If the mgmt app is given an image and the users says it required protected mode, then the mgmt app needs to know which host(s) are able to run it. Doing version number checks is not particularly desirable, so is there a way libvirt can determine if QEMU + host in general supports protected machines, so that we can report this feature to mgmt apps ? I thought that would be visible via the cpu model by checking for the unpack facility (161)? Time for somebody else to explain that. @Viktor @Boris: This one's for you. Right, a management app could check the supported CPU model, with something like virsh domcapabilities. The domain's CPU model would have to require the 'unpack' facility. So, in theory any management app establishing CPU model compatibility using the libvirt APIs should be able to find appropriate hosts. [...] -- Kind Regards, Viktor
Re: [PATCH 1/1] s390/ipl: sync back loadparm
On 2/25/20 12:56 PM, Halil Pasic wrote:
On Tue, 25 Feb 2020 10:39:40 +0100
David Hildenbrand wrote:
On 24.02.20 16:02, Halil Pasic wrote:
We expose loadparm as a r/w machine property, but if loadparm is set by
the guest via DIAG 308, we don't update the property. Having a
disconnect between the guest view and the QEMU property is not nice in
itself, but things get even worse for SCSI, where under certain
circumstances (see 789b5a401b "s390: Ensure IPL from SCSI works as
expected" for details) we call s390_gen_initial_iplb() on resets
effectively overwriting the guest/user supplied loadparm with the stale
value.
Signed-off-by: Halil Pasic
Fixes: 7104bae9de "hw/s390x: provide loadparm property for the machine"
Reported-by: Marc Hartmayer
Reviewed-by: Janosch Frank
Reviewed-by: Viktor Mihajlovski
Tested-by: Marc Hartmayer
---
hw/s390x/ipl.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
[...]
+
+/* Sync loadparm */
+if (iplb->flags & DIAG308_FLAGS_LP_VALID) {
+char ascii_loadparm[8];
+uint8_t *ebcdic_loadparm = iplb->loadparm;
+int i;
+
+for (i = 0; i < 8 && ebcdic_loadparm[i]; i++) {
+ascii_loadparm[i] = ebcdic2ascii[(uint8_t) ebcdic_loadparm[i]];
+}
+ascii_loadparm[i] = 0;
+object_property_set_str(mo, ascii_loadparm, "loadparm", NULL);
+} else {
+object_property_set_str(mo, "", "loadparm", NULL);
+}
_abort instead of NULL, we certainly want to know if this would
ever surprisingly fail.
IMHO this is a typical assert() situation where one would like to have
a fast and obvious failure when testing, but not in production.
AFAIU the guest can trigger this code at any time, and crashing the
whole (production) system seems a bit heavy handed to me. The setter
should only fail if something is buggy.
But if the majority says _abort I can certainly do. Other opinions?
We might consider to return 0x0402 (invalid parameter) from the diag308
"set", which is less drastic and would allow the OS to do whatever it
finds appropriate to deal with the failure. Not that Linux would care
about that today :-).
[...]
--
Kind Regards,
Viktor
Re: [PATCH v7 02/15] s390x: protvirt: Support unpack facility
On 3/9/20 12:21 PM, Janosch Frank wrote:
The unpack facility provides the means to setup a protected guest. A
protected guest can not be introspected by the hypervisor or any
user/administrator of the machine it is running on.
Protected guests are encrypted at rest and need a special boot
mechanism via diag308 subcode 8 and 10.
Code 8 sets the PV specific IPLB which is retained seperately from
those set via code 5.
Code 10 is used to unpack the VM into protected memory, verify its
integrity and start it.
Signed-off-by: Janosch Frank
Signed-off-by: Christian Borntraeger [Changes
to machine]
A nit: [Changes...] should go before the s-o-b.
---
hw/s390x/Makefile.objs | 1 +
hw/s390x/ipl.c | 59 -
hw/s390x/ipl.h | 72 ++--
hw/s390x/pv.c | 104 +++
hw/s390x/pv.h | 34
hw/s390x/s390-virtio-ccw.c | 127 +++-
include/hw/s390x/s390-virtio-ccw.h | 1 +
target/s390x/cpu.c | 17
target/s390x/cpu.h | 1 +
target/s390x/cpu_features_def.inc.h | 1 +
target/s390x/diag.c | 34 +++-
11 files changed, 436 insertions(+), 15 deletions(-)
create mode 100644 hw/s390x/pv.c
create mode 100644 hw/s390x/pv.h
diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index e02ed80b68..a46a1c7894 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -31,6 +31,7 @@ obj-y += tod-qemu.o
obj-$(CONFIG_KVM) += tod-kvm.o
obj-$(CONFIG_KVM) += s390-skeys-kvm.o
obj-$(CONFIG_KVM) += s390-stattrib-kvm.o
+obj-$(CONFIG_KVM) += pv.o
obj-y += s390-ccw.o
obj-y += ap-device.o
obj-y += ap-bridge.o
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 9c1ecd423c..ba3eac30c6 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -33,6 +33,7 @@
#include "qemu/cutils.h"
#include "qemu/option.h"
#include "exec/exec-all.h"
+#include "pv.h"
#define KERN_IMAGE_START0x01UL
#define LINUX_MAGIC_ADDR0x010008UL
@@ -542,11 +543,30 @@ void s390_ipl_update_diag308(IplParameterBlock *iplb)
{
S390IPLState *ipl = get_ipl_device();
-ipl->iplb = *iplb;
-ipl->iplb_valid = true;
+/*
+ * The IPLB set and retrieved by subcodes 8/9 is completely
+ * separate from the one managed via subcodes 5/6.
+ */
+if (iplb->pbt == S390_IPL_TYPE_PV) {
+ipl->iplb_pv = *iplb;
+ipl->iplb_valid_pv = true;
+} else {
+ipl->iplb = *iplb;
+ipl->iplb_valid = true;
+}
ipl->netboot = is_virtio_net_device(iplb);
}
+IplParameterBlock *s390_ipl_get_iplb_pv(void)
+{
+S390IPLState *ipl = get_ipl_device();
+
+if (!ipl->iplb_valid_pv) {
+return NULL;
+}
+return >iplb_pv;
+}
+
IplParameterBlock *s390_ipl_get_iplb(void)
{
S390IPLState *ipl = get_ipl_device();
@@ -561,7 +581,8 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset
reset_type)
{
S390IPLState *ipl = get_ipl_device();
-if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL) {
+if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL ||
+reset_type == S390_RESET_PV) {
/* use CPU 0 for full resets */
ipl->reset_cpu_index = 0;
} else {
@@ -635,6 +656,38 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
cpu_physical_memory_unmap(addr, len, 1, len);
}
+int s390_ipl_prepare_pv_header(void)
+{
+S390IPLState *ipl = get_ipl_device();
+IPLBlockPV *ipib_pv = >iplb_pv.pv;
+void *hdr = g_malloc(ipib_pv->pv_header_len);
+int rc;
+
+cpu_physical_memory_read(ipib_pv->pv_header_addr, hdr,
+ ipib_pv->pv_header_len);
+rc = s390_pv_set_sec_parms((uint64_t)hdr,
+ ipib_pv->pv_header_len);
+g_free(hdr);
+return rc;
+}
+
+int s390_ipl_pv_unpack(void)
+{
+int i, rc = 0;
+S390IPLState *ipl = get_ipl_device();
+IPLBlockPV *ipib_pv = >iplb_pv.pv;
+
+for (i = 0; i < ipib_pv->num_comp; i++) {
+rc = s390_pv_unpack(ipib_pv->components[i].addr,
+TARGET_PAGE_ALIGN(ipib_pv->components[i].size),
+ipib_pv->components[i].tweak_pref);
+if (rc) {
+break;
+}
+}
+return rc;
+}
+
void s390_ipl_prepare_cpu(S390CPU *cpu)
{
S390IPLState *ipl = get_ipl_device();
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index d4813105db..b2ccdd9dae 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -15,6 +15,24 @@
#include "cpu.h"
#include "hw/qdev-core.h"
+struct IPLBlockPVComp {
+uint64_t tweak_pref;
+uint64_t addr;
+uint64_t size;
+} QEMU_PACKED;
+typedef struct IPLBlockPVComp IPLBlockPVComp;
+
+struct IPLBlockPV {
+uint8_t reserved18[87];/* 0x18 */
+uint8_t version; /* 0x6f */
+
Re: [PATCH v2 1/1] virtio-ccw: auto-manage VIRTIO_F_IOMMU_PLATFORM if PV
On 6/10/20 12:24 PM, David Hildenbrand wrote:
On 10.06.20 12:07, David Gibson wrote:
On Wed, Jun 10, 2020 at 09:22:45AM +0200, David Hildenbrand wrote:
On 10.06.20 06:31, David Gibson wrote:
On Tue, Jun 09, 2020 at 12:44:39PM -0400, Michael S. Tsirkin wrote:
On Tue, Jun 09, 2020 at 06:28:39PM +0200, Halil Pasic wrote:
On Tue, 9 Jun 2020 17:47:47 +0200
Claudio Imbrenda wrote:
On Tue, 9 Jun 2020 11:41:30 +0200
Halil Pasic wrote:
[...]
I don't know. Janosch could answer that, but he is on vacation. Adding
Claudio maybe he can answer. My understanding is, that while it might
be possible, it is ugly at best. The ability to do a transition is
indicated by a CPU model feature. Indicating the feature to the guest
and then failing the transition sounds wrong to me.
I agree. If the feature is advertised, then it has to work. I don't
think we even have an architected way to fail the transition for that
reason.
What __could__ be done is to prevent qemu from even starting if an
incompatible device is specified together with PV.
AFAIU, the "specified together with PV" is the problem here. Currently
we don't "specify PV" but PV is just a capability that is managed by the
CPU model (like so many other).
So if we want to keep it user friendly, there could be
protection property with values on/off/auto, and auto
would poke at host capability to figure out whether
it's supported.
Both virtio and CPU would inherit from that.
Right, that's what I have in mind for my 'host-trust-limitation'
property (a generalized version of the existing 'memory-encryption'
machine option). My draft patches already set virtio properties
accordingly, it should be possible to set (default) cpu properties as
well.
No crazy CPU model hacks please (at least speaking for the s390x).
Uh... I'm not really sure what you have in mind here.
Reading along I got the impression that we want to glue the availability
of CPU features to other QEMU cmdline parameters (besides the
accelerator). ("to set (default) cpu properties as well"). If we are
talking about other CPU properties not expressed as CPU features (e.g.,
-cpu X,Y=on ...), then there is no issue.
The reason that the capability to run in PV mode is expressed in the CPU
model is that this capability *is* provided by the CPU in terms of
available instructions. I wouldn't see a benefit in providing
a meta-property that needs to be synced with the CPU model.
So, if something has to be concluded from the fact that a VM
could run in PV mode, that decision should be derived from the
CPU model.
--
Kind Regards,
Viktor
