Re: [RFC QEMU PATCH] pc-bios/s390-ccw: Add zipl-like "BOOT_IMAGE=x" to the kernel parameters
On 16.12.19 12:24, Thomas Huth wrote: > Note: I've marked the patch as RFC since I'm not quite sure whether > this is really the right way to address this issue: It's unfortunate > that we have to mess with different location in ZIPL which might also > change again in the future. As suggested by Christian on IRC last week, > maybe it would make more sense to change ZIPL to add this parameter > already when zipl is installed (i.e. by the Linux userspace "zipl" pro- > gram), instead of adding it during boot time? Also, the BOOT_IMAGE para- > meter on s390x is quite different from the BOOT_IMAGE paramter that is > used on x86 - while s390x only uses one single number here, the x86 > variant (added by grub2, I guess) uses the boot device + full filename > of the kernel on the boot partition. Should we maybe make the s390x > variant more conform to x86? If so, I think this really has to be fixed > in zipl userspace tool, and not in the s390-ccw bios (and zipl stage3 > bootloader). Yes, I actually think we should revisit the whole BOOT_IMAGE scheme on s390. Maybe we should use the kernel name, or the name of the boot menu entry. And maybe we should not use 0 (when the default is running) but instead really use to what 0 points to.
Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask
On 03.12.19 14:28, Janosch Frank wrote:
> We need to set the short psw indication bit in the reset psw, as it is
> a short psw.
>
> fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the
> guest")
> Signed-off-by: Janosch Frank
Acked-by: Christian Borntraeger
> ---
> pc-bios/s390-ccw/jump2ipl.c | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index 266f1502b9..da13c43cc0 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -12,11 +12,11 @@
> #define KERN_IMAGE_START 0x01UL
> #define PSW_MASK_64 0x0001ULL
> #define PSW_MASK_32 0x8000ULL
> -#define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64)
> +#define PSW_MASK_SHORTPSW 0x0008ULL
> +#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_32 | PSW_MASK_64)
>
> typedef struct ResetInfo {
> -uint32_t ipl_mask;
> -uint32_t ipl_addr;
> +uint64_t ipl_psw;
> uint32_t ipl_continue;
> } ResetInfo;
>
> @@ -50,7 +50,9 @@ void jump_to_IPL_code(uint64_t address)
> ResetInfo *current = 0;
>
> save = *current;
> -current->ipl_addr = (uint32_t) (uint64_t) _to_IPL_2;
> +
> +current->ipl_psw = (uint64_t) _to_IPL_2;
> +current->ipl_psw |= RESET_PSW_MASK;
> current->ipl_continue = address & 0x7fff;
>
> debug_print_int("set IPL addr to", current->ipl_continue);
> @@ -82,7 +84,7 @@ void jump_to_low_kernel(void)
> }
>
> /* Trying to get PSW at zero address */
> -if (*((uint64_t *)0) & IPL_PSW_MASK) {
> +if (*((uint64_t *)0) & RESET_PSW_MASK) {
> jump_to_IPL_code((*((uint64_t *)0)) & 0x7fff);
> }
>
>
Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask
On 03.12.19 14:28, Janosch Frank wrote:
> We need to set the short psw indication bit in the reset psw, as it is
> a short psw.
>
> fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the
> guest")
> Signed-off-by: Janosch Frank
We should also add
commit 24bb1fa36ff7b25ee774dbe4a18830dc782b54bf (HEAD, github-cohuck/s390-next)
Author: Janosch Frank
AuthorDate: Fri Nov 29 09:20:23 2019 -0500
Commit: Cornelia Huck
CommitDate: Mon Dec 2 09:58:57 2019 +0100
s390x: Properly fetch and test the short psw on diag308 subc 0/1
or whatever the final commit id will be. While this patch is not "broken"
it exposes the bug.
> ---
> pc-bios/s390-ccw/jump2ipl.c | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index 266f1502b9..da13c43cc0 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -12,11 +12,11 @@
> #define KERN_IMAGE_START 0x01UL
> #define PSW_MASK_64 0x0001ULL
> #define PSW_MASK_32 0x8000ULL
> -#define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64)
> +#define PSW_MASK_SHORTPSW 0x0008ULL
> +#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_32 | PSW_MASK_64)
>
> typedef struct ResetInfo {
> -uint32_t ipl_mask;
> -uint32_t ipl_addr;
> +uint64_t ipl_psw;
> uint32_t ipl_continue;
> } ResetInfo;
>
> @@ -50,7 +50,9 @@ void jump_to_IPL_code(uint64_t address)
> ResetInfo *current = 0;
>
> save = *current;
> -current->ipl_addr = (uint32_t) (uint64_t) _to_IPL_2;
> +
> +current->ipl_psw = (uint64_t) _to_IPL_2;
> +current->ipl_psw |= RESET_PSW_MASK;
> current->ipl_continue = address & 0x7fff;
>
> debug_print_int("set IPL addr to", current->ipl_continue);
> @@ -82,7 +84,7 @@ void jump_to_low_kernel(void)
> }
>
> /* Trying to get PSW at zero address */
> -if (*((uint64_t *)0) & IPL_PSW_MASK) {
> +if (*((uint64_t *)0) & RESET_PSW_MASK) {
> jump_to_IPL_code((*((uint64_t *)0)) & 0x7fff);
> }
>
>
Re: [PATCH v2 3/3] s390x: Fix cpu normal reset ri clearing
On 02.12.19 15:01, Janosch Frank wrote:
> As it turns out we need to clear the ri controls and PSW enablement
> bit to be architecture compliant.
>
> Signed-off-by: Janosch Frank
> ---
> target/s390x/cpu.c | 5 +
> target/s390x/cpu.h | 7 ++-
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 906285888e..c192e6b3b9 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -131,6 +131,11 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type
> type)
>>fpu_status);
> /* fall through */
As this is a fall through, do we want to change the INITIAL RESET to only clear
up to
start_normal_reset_fields, e.g. something like this on top
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 829ce6ad5491..58ac721687a9 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -108,7 +108,7 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
case S390_CPU_RESET_INITIAL:
/* initial reset does not clear everything! */
memset(>start_initial_reset_fields, 0,
- offsetof(CPUS390XState, end_reset_fields) -
+ offsetof(CPUS390XState, start_normal_reset_fields) -
offsetof(CPUS390XState, start_initial_reset_fields));
/* architectured initial value for Breaking-Event-Address register */
to avoid double memsetting.
Other than this question
Reviewed-by: Christian Borntraeger
> case S390_CPU_RESET_NORMAL:
> +env->psw.mask &= ~PSW_MASK_RI;
> +memset(>start_normal_reset_fields, 0,
> + offsetof(CPUS390XState, end_reset_fields) -
> + offsetof(CPUS390XState, start_normal_reset_fields));
> +
> env->pfault_token = -1UL;
> env->bpbc = false;
> break;
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index d5e18b096e..7f5fa1d35b 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -58,7 +58,6 @@ struct CPUS390XState {
> */
> uint64_t vregs[32][2] QEMU_ALIGNED(16); /* vector registers */
> uint32_t aregs[16];/* access registers */
> -uint8_t riccb[64]; /* runtime instrumentation control */
> uint64_t gscb[4]; /* guarded storage control */
> uint64_t etoken; /* etoken */
> uint64_t etoken_extension; /* etoken extension */
> @@ -114,6 +113,10 @@ struct CPUS390XState {
> uint64_t gbea;
> uint64_t pp;
>
> +/* Fields up to this point are not cleared by normal CPU reset */
> +struct {} start_normal_reset_fields;
> +uint8_t riccb[64]; /* runtime instrumentation control */
> +
> /* Fields up to this point are cleared by a CPU reset */
> struct {} end_reset_fields;
>
> @@ -252,6 +255,7 @@ extern const VMStateDescription vmstate_s390_cpu;
> #undef PSW_SHIFT_ASC
> #undef PSW_MASK_CC
> #undef PSW_MASK_PM
> +#undef PSW_MASK_RI
> #undef PSW_SHIFT_MASK_PM
> #undef PSW_MASK_64
> #undef PSW_MASK_32
> @@ -274,6 +278,7 @@ extern const VMStateDescription vmstate_s390_cpu;
> #define PSW_MASK_CC 0x3000ULL
> #define PSW_MASK_PM 0x0F00ULL
> #define PSW_SHIFT_MASK_PM 40
> +#define PSW_MASK_RI 0x0080ULL
> #define PSW_MASK_64 0x0001ULL
> #define PSW_MASK_32 0x8000ULL
> #define PSW_MASK_ESA_ADDR 0x7fffULL
>
Re: [PATCH v1 1/1] pc-bios/s390-ccw: fix sclp_get_loadparm_ascii
On 28.11.19 16:05, Peter Maydell wrote: > On Thu, 28 Nov 2019 at 12:48, Christian Borntraeger > wrote: >> >> >> >> On 28.11.19 13:45, Cornelia Huck wrote: >>> On Thu, 28 Nov 2019 13:35:29 +0100 >>> Christian Borntraeger wrote: >>> >>>> Ack. >>>> >>>> Conny, I think this would be really nice to have for 4.2 (together with a >>>> bios rebuild) >>>> as this fixes a regression. Opinions? >>> >>> I fear that this is a bit late for 4.2... but this should get a >>> cc:stable. >> >> So we would rather ship a qemu regression instead of pushing a 1 line fixup? >> Peter, what is the current state of 4.2? does it look like we will have an >> rc4 >> or is everything else silent. > > There isn't currently anything else that would need an rc4. I would vote for getting this patch still in. And I think we probably do not need an rc4 for that, the fix seems pretty straight forward.
Re: [PATCH v1 1/1] pc-bios/s390-ccw: fix sclp_get_loadparm_ascii
On 28.11.19 14:16, Cornelia Huck wrote:
> On Thu, 28 Nov 2019 14:11:38 +0100
> Thomas Huth wrote:
>
>> On 28/11/2019 13.35, Christian Borntraeger wrote:
>>> Ack.
>>>
>>> Conny, I think this would be really nice to have for 4.2 (together with a
>>> bios rebuild)
>>> as this fixes a regression. Opinions?
>>
>> If we do another rc of 4.2, I think we definitely want this to be
>> included, otherwise quite a bunch of things don't work anymore as
>> expected, e.g. "-boot menu=on"...
>
> I do agree we want this if possible; the question is really the
> "possible" part...
Given the issues that we see without that fix, I think this would qualify do
an rc4 (or even to apply it on top of rc3 to become 4.2)
Maybe just do a pull request?
>
>>
>>>> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
>>>> index c0223fa..7251f9a 100644
>>>> --- a/pc-bios/s390-ccw/sclp.c
>>>> +++ b/pc-bios/s390-ccw/sclp.c
>>>> @@ -112,7 +112,7 @@ void sclp_get_loadparm_ascii(char *loadparm)
>>>> ReadInfo *sccb = (void *)_sccb;
>>>>
>>>> memset((char *)_sccb, 0, sizeof(ReadInfo));
>>>> -sccb->h.length = sizeof(ReadInfo);
>>>> +sccb->h.length = SCCB_SIZE;
>>>> if (!sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb)) {
>>>> ebcdic_to_ascii((char *) sccb->loadparm, loadparm, LOADPARM_LEN);
>>>> }
>>
>> I gave it a quick try, and this fixes "-boot menu=on" for me, so:
>>
>> Tested-by: Thomas Huth
>
> Thanks.
>
> FWIW, I'm currently working to put this + the rebuild on my s390-fixes
> branch.
>
Re: [PATCH v1 1/1] pc-bios/s390-ccw: fix sclp_get_loadparm_ascii
On 28.11.19 13:45, Cornelia Huck wrote:
> On Thu, 28 Nov 2019 13:35:29 +0100
> Christian Borntraeger wrote:
>
>> Ack.
>>
>> Conny, I think this would be really nice to have for 4.2 (together with a
>> bios rebuild)
>> as this fixes a regression. Opinions?
>
> I fear that this is a bit late for 4.2... but this should get a
> cc:stable.
So we would rather ship a qemu regression instead of pushing a 1 line fixup?
Peter, what is the current state of 4.2? does it look like we will have an rc4
or is everything else silent.
>
>>
>>
>>
>> On 28.11.19 13:33, Claudio Imbrenda wrote:
>>> The existing s390 bios gets the LOADPARM information from the system using
>>> an SCLP call that specifies a buffer length too small to contain all the
>>> output.
>>>
>>> The recent fixes in the SCLP code have exposed this bug, since now the
>>> SCLP call will return an error (as per architecture) instead of
>>> writing partially and completing successfully.
>>>
>>> The solution is simply to specify the full page length as the SCCB
>>> length instead of a smaller size.
>>>
>>> Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length")
>>> Fixes: 9a22473c70f3 ("pc-bios/s390-ccw: get LOADPARM stored in SCP Read
>>> Info")
>>>
>>> Signed-off-by: Claudio Imbrenda
>>> ---
>>> pc-bios/s390-ccw/sclp.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
>>> index c0223fa..7251f9a 100644
>>> --- a/pc-bios/s390-ccw/sclp.c
>>> +++ b/pc-bios/s390-ccw/sclp.c
>>> @@ -112,7 +112,7 @@ void sclp_get_loadparm_ascii(char *loadparm)
>>> ReadInfo *sccb = (void *)_sccb;
>>>
>>> memset((char *)_sccb, 0, sizeof(ReadInfo));
>>> -sccb->h.length = sizeof(ReadInfo);
>>> +sccb->h.length = SCCB_SIZE;
>>> if (!sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb)) {
>>> ebcdic_to_ascii((char *) sccb->loadparm, loadparm, LOADPARM_LEN);
>>> }
>>>
>
> The change seems sane.
>
Re: [PATCH v1 1/1] pc-bios/s390-ccw: fix sclp_get_loadparm_ascii
Ack.
Conny, I think this would be really nice to have for 4.2 (together with a bios
rebuild)
as this fixes a regression. Opinions?
On 28.11.19 13:33, Claudio Imbrenda wrote:
> The existing s390 bios gets the LOADPARM information from the system using
> an SCLP call that specifies a buffer length too small to contain all the
> output.
>
> The recent fixes in the SCLP code have exposed this bug, since now the
> SCLP call will return an error (as per architecture) instead of
> writing partially and completing successfully.
>
> The solution is simply to specify the full page length as the SCCB
> length instead of a smaller size.
>
> Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length")
> Fixes: 9a22473c70f3 ("pc-bios/s390-ccw: get LOADPARM stored in SCP Read Info")
>
> Signed-off-by: Claudio Imbrenda
> ---
> pc-bios/s390-ccw/sclp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
> index c0223fa..7251f9a 100644
> --- a/pc-bios/s390-ccw/sclp.c
> +++ b/pc-bios/s390-ccw/sclp.c
> @@ -112,7 +112,7 @@ void sclp_get_loadparm_ascii(char *loadparm)
> ReadInfo *sccb = (void *)_sccb;
>
> memset((char *)_sccb, 0, sizeof(ReadInfo));
> -sccb->h.length = sizeof(ReadInfo);
> +sccb->h.length = SCCB_SIZE;
> if (!sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb)) {
> ebcdic_to_ascii((char *) sccb->loadparm, loadparm, LOADPARM_LEN);
> }
>
Re: [PATCH v2 2/2] s390x/cpumodel: Introduce dynamic feature groups
re-adding ccs from the cover-letter
>>> On 25.11.19 18:20, David Hildenbrand wrote:
>>>
>>> As soon as dynamic feature groups are used, the CPU model becomes
>>> migration-unsafe. Upper layers can expand these models to migration-safe
>>> and static variants, allowing them to be migrated.
>>
>> I really dislike that. I am trying to get rid of the unsafe variants (e.g.
>> now
>> defaulting to host-model instead of host-passthrough). I do not want to give
>> users new ways of hurting themselves.
>>
>
> Please note that this is just on the bare command line. Libvirt and friends
> will expand the model and have proper migration in place. What exactly is
> your concern in that regard?
What is then the value? libvirt can also use host-model or baselining if
necessary.
And for the command line this will just add more opportunity to shot yourself
in the
foot, no?
Let me put it this way, I might have misunderstood what you are trying to do
here,
but if I do not get, then others (e.g. users) will also not get it.
Maybe its just the interface or the name. But I find this very non-intuitive
e.g. you wrote
Get the maximum possible feature set (e.g., including deprecated
features) for a CPU definition in the configuration ("everything that
could be enabled"):
-cpu z14,all-features=off,available-features=on
Get all valid features for a CPU definition:
-cpu z14,all-features=on
What is the point of this? It is either the same as the one before, or it wont
be able to start.
>
>> Unless I misunderstood Eduardo, I think his versioning approach is actually
>> better
>> in regard to migration, no?
>> I z terms, you can still say -cpu z13 which is just an alias to z13v1 z13v2
>> etc.
>> Assuming that the version is checked this will be safe.
>>
>
> It‘s even worse AFAIKS. A „-cpu z13“ would dynamically map to whatever is
> best on the host.
>
Re: [PATCH v2 2/2] s390x/cpumodel: Introduce dynamic feature groups
On 25.11.19 18:20, David Hildenbrand wrote: > > As soon as dynamic feature groups are used, the CPU model becomes > migration-unsafe. Upper layers can expand these models to migration-safe > and static variants, allowing them to be migrated. I really dislike that. I am trying to get rid of the unsafe variants (e.g. now defaulting to host-model instead of host-passthrough). I do not want to give users new ways of hurting themselves. Unless I misunderstood Eduardo, I think his versioning approach is actually better in regard to migration, no? I z terms, you can still say -cpu z13 which is just an alias to z13v1 z13v2 etc. Assuming that the version is checked this will be safe.
Re: [PATCH 06/15] s390x: protvirt: Support unpack facility
On 21.11.19 15:28, David Hildenbrand wrote: >> And please trim your emails. >> > > If you use Thunderbird I suggest QuoteCollapse ... because nobody got time > for that ;) neat.
Re: [PATCH 0/7] i386: Add `machine` parameter to query-cpu-definitions
On 25.10.19 09:17, David Hildenbrand wrote:
> On 25.10.19 04:25, Eduardo Habkost wrote:
>> We had introduced versioned CPU models in QEMU 4.1, including a
>> method for querying for CPU model versions using
>
> Interesting, I was not aware of that.
>
> On s390x, we somewhat have versioned CPU models, but we decided against
> giving them explicit names (e.g., z13-v1 or z13-4.1.0), because it didn't
> really seem to be necessary. (and we often implement/add features for older
> CPU models, there is a lot of fluctuation) Actually, you would have had to
> add "z13-z/VM-x.x.x" or "z13-LPAR-x.x.x" or "z13-KVM-x.x.x" to model the
> features you actually see in all the different virtual environments ("what a
> CPU looks like"). Not to talk about QEMU versions in addition to that. So we
> decided to always model what you would see under LPAR and are able to specify
> for a KVM guest. So you can use "z13" in an up-to-date LPAR environment, but
> not in a z/VM environment (you would have to disable features).
>
> Each (!base) CPU model has a specific feature set per machine. Libvirt uses
> query-cpu-model-expansion() to convert this model+machine to a
> machine-independent representation. That representation is sufficient for all
> use cases we were aware of (esp. "virsh domcapabilities", the host CPU model,
> migration).
>
> While s390x has versioned CPU models, we have no explicit way of specifying
> them for older machines, besides going over query-cpu-model-expansion and
> using expanded "base model + features".
>
> I can see that this might make sense on x86-64, where you only have maybe 3
> versions of a CPU (e.g., the one Intel messed up first - Haswell, the one
> Intel messed up next - Haswell-noTSX, and the one that Intel eventually did
> right - Haswell-noTSX-IBRS) and you might want to specify "Haswell" vs.
> "Haswell-IBRS" vs. "Haswell-noTSX-IBRS". But actually, you will always want
> to go for "Haswell-noTSX-IBRS", because you can expect to run in updated
> environments if I am not wrong, everything else are corner cases.
>
> Of course, versioned CPU model are neat to avoid "base model + list of
> features", but at least for expanding the host model on s390x, it is not
> really helpful. When migrating, the model expansion does the trick.
>
> I haven't looked into details of "how to specify or model versions". Maybe
> IBM wants to look into creating versions for all the old models we had. But
> again, not sure if that is of any help for s390x. CCing IBM.
I agree that this does not look very helpful.
Especially as several things depend on the kernel version a QEMU version is
not sufficient to be guarantee construction success.
So we would need something like z14-qemu4.0-kernel-5.2-suse-flavour-onLPAR
Instead we do check if we can construct an equivalent model on the migration
target.
And that model is precise. We do even have versions.
Right now with QEMU on s390 our models are versioned in a way that we fence of
facilities for old machine versions.
For example
-machine s390-virtio-ccw-3.1 -cpu z14 will not have the multiple epoch facility
and
-machine s390-virtio-ccw-4.0 -cpu z14 will have the multiple epoch facility.
As migration does always require the tuple of machine and cpu this is save. I
fail
to see what the benefit of an explicit z14-3.1 would be.
Re: Default CPU models on s390x and ppc64
On 17.10.19 17:18, David Hildenbrand wrote: > On 17.10.19 17:16, Jiri Denemark wrote: >> Hi David and David, >> >> I'm working on libvirt's support [1] for query-machines' >> default-cpu-type, which is supposed to return the type of the default >> CPU model that QEMU uses for each machine type. Rather than hard coding >> the default in libvirt (which we currently do on x86), we ask QEMU for >> the default CPU model and use it unless a user asks for a specific CPU >> model explicitly. >> >> We use query-cpu-definitions for translating the default CPU type to the >> actual CPU model we need to pass to -cpu by looking up the CPU model >> with matching typename. >> >> However, all this seems to work only with TCG on both s390x and ppc64. >> The issues I met with KVM on each architecture are described below. >> >> On ppc64 the default CPU type reported by query-machines is >> power*-powerpc64-cpu, while IIUC QEMU would effectively use -cpu host by >> default. In fact -cpu power8 is mostly just a fancy alias to -cpu host >> on a Power8 machine. But QEMU even rewrites typename of the current host >> CPU model to host-powerpc64-cpu. Which means on a Power8 host the power8 >> CPU model would have typename=host-powerpc64-cpu while the default CPU >> type would still use power8*-powerpc64-cpu. Thus we may just fail to >> find any CPU model corresponding to the default CPU model. >> >> And to make it even worse, the default CPU model changes with machine >> type. E.g., pseries-3.1 uses power8_v2.0-powerpc64-cpu while pseries-4.2 >> uses power9_v2.0-powerpc64-cpu. However, starting QEMU with pseries-4.2 >> machine type and the reported default -cpu power9 fails on any >> non-Power9 host. That said QEMU just lies about the default CPU model. >> >> So for all combinations of (pseries-3.1, pseries-4.2) machine types and >> (Power8, Power9) hosts, one will get the following mixed results: >> >> - pseries-3.1 on Power8: no default CPU model will be detected, QEMU >> starts fine with it's own default >> - pseries-4.2 on Power9: same as above >> - pseries-3.1 on Power9: -cpu power8 (not sure if this works, though) >> - pseries-4.2 on Power8: -cpu power9, QEMU doesn't start >> >> >> This situation on s390x is not so complicated, but not really better. >> The default CPU is said to be "qemu" for all machine types, which works >> fine for TCG domains, but it doesn't work on KVM because QEMU complains >> that some features requested in the CPU model are not available. In >> other words the "qemu" CPU model is not runnable on KVM. This a bit >> similar to what happens on x86_64, but QEMU just ignores missing >> features and starts happily there. > > The default model under KVM is "host", under TCG it's "qemu". We should not > use "qemu" under KVM, although it might work on some setups ... > > Where/how is this default model detected? I would prefer it libvirt would use the equivalent of mode=host-model by default. Is this what this series is all about?
Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
On 17.10.19 16:21, Thomas Huth wrote:
> There is no USB on s390x, so running qemu-system-s390x with
> "-machine ...,usb=on" is certainly wrong. Emit a warning to make
> the users aware of their misconfiguration.
>
> Signed-off-by: Thomas Huth
> ---
> After a year or two, we could finally turn this into a hard error,
> but I think we should give the users some time to fix their command
> lines first, so I'm initially only emitting a warning here.
I think a warn message is ok, but we should never make this a hard
error.
I am pretty sure that there are some tools in the wild that create xmls
or qemu commands lines cross-platform and deploy those dynamically.
These tools have probably been fixed to work good enough with s390x
but nobody with qemu clue has ever looked at these command lines. And
I am pretty sure that no user will actually see the command like nor
the error message.
So this warning will stay unnoticed until we make this a hard error. And
then we have broken a previously working setup.
In other words, I appreciate the willingness to detect mis-uses but I
fear that we will never be able to assume that everything is fixed.
>
> hw/s390x/s390-virtio-ccw.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index d3edeef0ad..af8c4c0daf 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -243,6 +243,10 @@ static void ccw_init(MachineState *machine)
> VirtualCssBus *css_bus;
> DeviceState *dev;
>
> +if (machine->usb) {
> +warn_report("This machine does not support USB");
> +}
> +
> s390_sclp_init();
> /* init memory + setup max page size. Required for the CPU model */
> s390_memory_init(machine->ram_size);
>
Re: libvirt? qemu change that mmaps ELF files breaks libvirt svirt handling for os.kernel
On 04.10.19 14:36, Daniel P. Berrangé wrote:
> On Fri, Oct 04, 2019 at 02:18:49PM +0200, Christian Borntraeger wrote:
>>
>>
>> On 04.10.19 14:13, Paolo Bonzini wrote:
>>> On 04/10/19 14:03, Christian Borntraeger wrote:
>>>> Stefano, Paolo,
>>>>
>>>> I have an interesting fail in QEMU
>>>>
>>>> 2019-10-04T12:00:32.675188Z qemu-system-s390x: GLib: g_mapped_file_unref:
>>>> assertion 'file != NULL' failed
>>>> that bisected to
>>>> commit 816b9fe450220e19acb91a0ce4a8ade7000648d1 (refs/bisect/bad)
>>>> elf-ops.h: Map into memory the ELF to load
>>>>
>>>> strace tells that I can read the ELF file, but not mmap
>>>> strace:
>>>> 214365 openat(AT_FDCWD, "/var/lib/libvirt/images/test_cpu_timer.elf",
>>>> O_RDONLY) = 36
>>>> 214365 read(46, "\177ELF\2\2\1\0\0\0\0\0\0\0\0\0", 16) = 16
>>>> 214365 lseek(46, 0, SEEK_SET) = 0
>>>> [...]
>>>> 214365 fstat(46, {st_mode=S_IFREG|0755, st_size=168176, ...}) = 0
>>>> 214365 mmap(NULL, 168176, PROT_READ|PROT_WRITE, MAP_PRIVATE, 46, 0) = -1
>>>> EACCES (Permission denied)
>>>>
>>>> So reading from /var/lib/libvirt/images/test_cpu_timer.elf does work,
>>>> mmaping does not.
>>>> setenforce 0 makes the problem go away.
>>>>
>>>> This might be more of an issue in libvirt, setting the svirt context too
>>>> restrictive, but I am not too deep into the svirt part of libvirt.
>>>> Reverting the qemu commit makes the problem go away.
>>>
>>> Yes, the policy is too restrictive in my opinion.
>>>
>>> Can you include the output of "audit2allow" and/or "audit2allow -R"?
>>>
>>> Thanks,
>>>
>>> Paolo
>>>
>>
>> require {
>> type unconfined_t;
>> type virt_content_t;
>> type svirt_t;
>> type systemd_tmpfiles_t;
>> type user_home_t;
>> type NetworkManager_t;
>> class file { entrypoint execute ioctl lock map open read write };
>> class bpf prog_run;
>> }
>>
>> #= svirt_t ==
>> allow svirt_t user_home_t:file { entrypoint execute ioctl lock open read
>> write };
>>
>> # This avc can be allowed using the boolean 'domain_can_mmap_files'
>
> This is an unrelated boolean and we don't want to use that so ignore
> this suggestion !
>
>> allow svirt_t virt_content_t:file map;
>
> This matches your strace. virt_content_t is the label we use on
> files that are exposed to QEMU read-only.
>
> The svirt policy only allows mmap on files that re exposed read-write
> currently.
>
> I believe we can safely allow this mmap on read-only files too
> because the read-only restriction is enforced at time of open,
> and any writes to the mapped file will result in a private
> copy on write.
>
> Please file a bz entry against the selinux-policy component for
> whatever distro you're using, and/or Fedora rawhide, and CC me
> on it too.
Done. This was on Fedora 30.
https://bugzilla.redhat.com/show_bug.cgi?id=1758525
Now sure about others like RHEL. RHV.
libvirt? qemu change that mmaps ELF files breaks libvirt svirt handling for os.kernel
Stefano, Paolo,
I have an interesting fail in QEMU
2019-10-04T12:00:32.675188Z qemu-system-s390x: GLib: g_mapped_file_unref:
assertion 'file != NULL' failed
that bisected to
commit 816b9fe450220e19acb91a0ce4a8ade7000648d1 (refs/bisect/bad)
elf-ops.h: Map into memory the ELF to load
strace tells that I can read the ELF file, but not mmap
strace:
214365 openat(AT_FDCWD, "/var/lib/libvirt/images/test_cpu_timer.elf", O_RDONLY)
= 36
214365 read(46, "\177ELF\2\2\1\0\0\0\0\0\0\0\0\0", 16) = 16
214365 lseek(46, 0, SEEK_SET) = 0
[...]
214365 fstat(46, {st_mode=S_IFREG|0755, st_size=168176, ...}) = 0
214365 mmap(NULL, 168176, PROT_READ|PROT_WRITE, MAP_PRIVATE, 46, 0) = -1 EACCES
(Permission denied)
So reading from /var/lib/libvirt/images/test_cpu_timer.elf does work, mmaping
does not.
setenforce 0 makes the problem go away.
This might be more of an issue in libvirt, setting the svirt context too
restrictive, but I am not too deep into the svirt part of libvirt.
Reverting the qemu commit makes the problem go away.
Re: libvirt? qemu change that mmaps ELF files breaks libvirt svirt handling for os.kernel
On 04.10.19 14:13, Paolo Bonzini wrote:
> On 04/10/19 14:03, Christian Borntraeger wrote:
>> Stefano, Paolo,
>>
>> I have an interesting fail in QEMU
>>
>> 2019-10-04T12:00:32.675188Z qemu-system-s390x: GLib: g_mapped_file_unref:
>> assertion 'file != NULL' failed
>> that bisected to
>> commit 816b9fe450220e19acb91a0ce4a8ade7000648d1 (refs/bisect/bad)
>> elf-ops.h: Map into memory the ELF to load
>>
>> strace tells that I can read the ELF file, but not mmap
>> strace:
>> 214365 openat(AT_FDCWD, "/var/lib/libvirt/images/test_cpu_timer.elf",
>> O_RDONLY) = 36
>> 214365 read(46, "\177ELF\2\2\1\0\0\0\0\0\0\0\0\0", 16) = 16
>> 214365 lseek(46, 0, SEEK_SET) = 0
>> [...]
>> 214365 fstat(46, {st_mode=S_IFREG|0755, st_size=168176, ...}) = 0
>> 214365 mmap(NULL, 168176, PROT_READ|PROT_WRITE, MAP_PRIVATE, 46, 0) = -1
>> EACCES (Permission denied)
>>
>> So reading from /var/lib/libvirt/images/test_cpu_timer.elf does work,
>> mmaping does not.
>> setenforce 0 makes the problem go away.
>>
>> This might be more of an issue in libvirt, setting the svirt context too
>> restrictive, but I am not too deep into the svirt part of libvirt.
>> Reverting the qemu commit makes the problem go away.
>
> Yes, the policy is too restrictive in my opinion.
>
> Can you include the output of "audit2allow" and/or "audit2allow -R"?
>
> Thanks,
>
> Paolo
>
require {
type unconfined_t;
type virt_content_t;
type svirt_t;
type systemd_tmpfiles_t;
type user_home_t;
type NetworkManager_t;
class file { entrypoint execute ioctl lock map open read write };
class bpf prog_run;
}
#= svirt_t ==
allow svirt_t user_home_t:file { entrypoint execute ioctl lock open read write
};
# This avc can be allowed using the boolean 'domain_can_mmap_files'
allow svirt_t virt_content_t:file map;
corecmd_bin_entry_type(svirt_t)
userdom_manage_user_home_content_dirs(svirt_t)
userdom_map_user_home_files(svirt_t)
virt_rw_svirt_image(svirt_t)
#= systemd_tmpfiles_t ==
kernel_read_usermodehelper_state(systemd_tmpfiles_t)
#= unconfined_t ==
allow unconfined_t NetworkManager_t:bpf prog_run;
Re: [PULL 09/12] kvm: clear dirty bitmaps from all overlapping memslots
On 02.10.19 18:01, Kevin Wolf wrote:
> Am 30.09.2019 um 15:19 hat Christian Borntraeger geschrieben:
>> From: Paolo Bonzini
>>
>> Currently MemoryRegionSection has 1:1 mapping to KVMSlot.
>> However next patch will allow splitting MemoryRegionSection into
>> several KVMSlot-s, make sure that kvm_physical_log_slot_clear()
>> is able to handle such 1:N mapping.
>>
>> Signed-off-by: Paolo Bonzini
>> Signed-off-by: Igor Mammedov
>> Reviewed-by: Peter Xu
>> Message-Id: <20190924144751.24149-3-imamm...@redhat.com>
>> Acked-by: Paolo Bonzini
>> Signed-off-by: Christian Borntraeger
>
> This broke the build for me on F29:
>
> CC x86_64-softmmu/accel/kvm/kvm-all.o
> /tmp/qemu/accel/kvm/kvm-all.c: In function 'kvm_log_clear':
> /tmp/qemu/accel/kvm/kvm-all.c:1086:8: error: 'ret' may be used uninitialized
> in this function [-Werror=maybe-uninitialized]
> if (r < 0) {
> ^
> cc1: all warnings being treated as errors
>
Does
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index aabe097c410f..e2605da22e7e 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -712,7 +712,7 @@ static int kvm_physical_log_clear(KVMMemoryListener *kml,
KVMState *s = kvm_state;
uint64_t start, size, offset, count;
KVMSlot *mem;
-int ret, i;
+int ret = 0, i;
if (!s->manual_dirty_log_protect) {
/* No need to do explicit clear */
fix the message?
[PULL 02/12] s390: PCI: fix IOMMU region init
From: Matthew Rosato
The fix in dbe9cf606c shrinks the IOMMU memory region to a size
that seems reasonable on the surface, however is actually too
small as it is based against a 0-mapped address space. This
causes breakage with small guests as they can overrun the IOMMU window.
Let's go back to the prior method of initializing iommu for now.
Fixes: dbe9cf606c ("s390x/pci: Set the iommu region size mpcifc request")
Cc: qemu-sta...@nongnu.org
Reviewed-by: Pierre Morel
Reported-by: Boris Fiuczynski
Tested-by: Boris Fiuczynski
Reported-by: Stefan Zimmerman
Signed-off-by: Matthew Rosato
Message-Id: <1569507036-15314-1-git-send-email-mjros...@linux.ibm.com>
Signed-off-by: Christian Borntraeger
---
hw/s390x/s390-pci-bus.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 963a41c7f532..2d2f4a7c419c 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -695,10 +695,15 @@ static const MemoryRegionOps s390_msi_ctrl_ops = {
void s390_pci_iommu_enable(S390PCIIOMMU *iommu)
{
+/*
+ * The iommu region is initialized against a 0-mapped address space,
+ * so the smallest IOMMU region we can define runs from 0 to the end
+ * of the PCI address space.
+ */
char *name = g_strdup_printf("iommu-s390-%04x", iommu->pbdev->uid);
memory_region_init_iommu(>iommu_mr, sizeof(iommu->iommu_mr),
TYPE_S390_IOMMU_MEMORY_REGION, OBJECT(>mr),
- name, iommu->pal - iommu->pba + 1);
+ name, iommu->pal + 1);
iommu->enabled = true;
memory_region_add_subregion(>mr, 0,
MEMORY_REGION(>iommu_mr));
g_free(name);
--
2.21.0
[PULL 03/12] s390x: sclp: refactor invalid command check
From: Janosch Frank
Invalid command checking has to be done before the boundary check,
refactoring it now allows to insert the boundary check at the correct
place later.
Signed-off-by: Janosch Frank
Reviewed-by: Jason J. Herne
Message-Id: <1569591203-15258-2-git-send-email-imbre...@linux.ibm.com>
Reviewed-by: David Hildenbrand
Signed-off-by: Christian Borntraeger
---
hw/s390x/event-facility.c | 3 ---
hw/s390x/sclp.c | 17 -
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 797ecbb7a9c8..66205697ae75 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -377,9 +377,6 @@ static void command_handler(SCLPEventFacility *ef, SCCB
*sccb, uint64_t code)
case SCLP_CMD_WRITE_EVENT_MASK:
write_event_mask(ef, sccb);
break;
-default:
-sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
-break;
}
}
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index fac7c3bb6c02..95ebfe7bd2f1 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -219,8 +219,23 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb,
uint32_t code)
goto out;
}
-sclp_c->execute(sclp, _sccb, code);
+switch (code & SCLP_CMD_CODE_MASK) {
+case SCLP_CMDW_READ_SCP_INFO:
+case SCLP_CMDW_READ_SCP_INFO_FORCED:
+case SCLP_CMDW_READ_CPU_INFO:
+case SCLP_CMDW_CONFIGURE_IOA:
+case SCLP_CMDW_DECONFIGURE_IOA:
+case SCLP_CMD_READ_EVENT_DATA:
+case SCLP_CMD_WRITE_EVENT_DATA:
+case SCLP_CMD_WRITE_EVENT_MASK:
+break;
+default:
+work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
+goto out_write;
+}
+sclp_c->execute(sclp, _sccb, code);
+out_write:
cpu_physical_memory_write(sccb, _sccb,
be16_to_cpu(work_sccb.h.length));
--
2.21.0
[PULL 08/12] kvm: extract kvm_log_clear_one_slot
From: Paolo Bonzini
We may need to clear the dirty bitmap for more than one KVM memslot.
First do some code movement with no semantic change.
Signed-off-by: Paolo Bonzini
Signed-off-by: Igor Mammedov
Reviewed-by: Peter Xu
Message-Id: <20190924144751.24149-2-imamm...@redhat.com>
Acked-by: Paolo Bonzini
Signed-off-by: Christian Borntraeger
[fixup line break]
---
accel/kvm/kvm-all.c | 103
1 file changed, 57 insertions(+), 46 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index b09bad08048d..a85ec09486dd 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -575,55 +575,14 @@ out:
#define KVM_CLEAR_LOG_ALIGN (qemu_real_host_page_size << KVM_CLEAR_LOG_SHIFT)
#define KVM_CLEAR_LOG_MASK (-KVM_CLEAR_LOG_ALIGN)
-/**
- * kvm_physical_log_clear - Clear the kernel's dirty bitmap for range
- *
- * NOTE: this will be a no-op if we haven't enabled manual dirty log
- * protection in the host kernel because in that case this operation
- * will be done within log_sync().
- *
- * @kml: the kvm memory listener
- * @section: the memory range to clear dirty bitmap
- */
-static int kvm_physical_log_clear(KVMMemoryListener *kml,
- MemoryRegionSection *section)
+static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
+ uint64_t size)
{
KVMState *s = kvm_state;
+uint64_t end, bmap_start, start_delta, bmap_npages;
struct kvm_clear_dirty_log d;
-uint64_t start, end, bmap_start, start_delta, bmap_npages, size;
unsigned long *bmap_clear = NULL, psize = qemu_real_host_page_size;
-KVMSlot *mem = NULL;
-int ret, i;
-
-if (!s->manual_dirty_log_protect) {
-/* No need to do explicit clear */
-return 0;
-}
-
-start = section->offset_within_address_space;
-size = int128_get64(section->size);
-
-if (!size) {
-/* Nothing more we can do... */
-return 0;
-}
-
-kvm_slots_lock(kml);
-
-/* Find any possible slot that covers the section */
-for (i = 0; i < s->nr_slots; i++) {
-mem = >slots[i];
-if (mem->start_addr <= start &&
-start + size <= mem->start_addr + mem->memory_size) {
-break;
-}
-}
-
-/*
- * We should always find one memslot until this point, otherwise
- * there could be something wrong from the upper layer
- */
-assert(mem && i != s->nr_slots);
+int ret;
/*
* We need to extend either the start or the size or both to
@@ -694,7 +653,7 @@ static int kvm_physical_log_clear(KVMMemoryListener *kml,
/* It should never overflow. If it happens, say something */
assert(bmap_npages <= UINT32_MAX);
d.num_pages = bmap_npages;
-d.slot = mem->slot | (kml->as_id << 16);
+d.slot = mem->slot | (as_id << 16);
if (kvm_vm_ioctl(s, KVM_CLEAR_DIRTY_LOG, ) == -1) {
ret = -errno;
@@ -717,6 +676,58 @@ static int kvm_physical_log_clear(KVMMemoryListener *kml,
size / psize);
/* This handles the NULL case well */
g_free(bmap_clear);
+return ret;
+}
+
+
+/**
+ * kvm_physical_log_clear - Clear the kernel's dirty bitmap for range
+ *
+ * NOTE: this will be a no-op if we haven't enabled manual dirty log
+ * protection in the host kernel because in that case this operation
+ * will be done within log_sync().
+ *
+ * @kml: the kvm memory listener
+ * @section: the memory range to clear dirty bitmap
+ */
+static int kvm_physical_log_clear(KVMMemoryListener *kml,
+ MemoryRegionSection *section)
+{
+KVMState *s = kvm_state;
+uint64_t start, size;
+KVMSlot *mem = NULL;
+int ret, i;
+
+if (!s->manual_dirty_log_protect) {
+/* No need to do explicit clear */
+return 0;
+}
+
+start = section->offset_within_address_space;
+size = int128_get64(section->size);
+
+if (!size) {
+/* Nothing more we can do... */
+return 0;
+}
+
+kvm_slots_lock(kml);
+
+/* Find any possible slot that covers the section */
+for (i = 0; i < s->nr_slots; i++) {
+mem = >slots[i];
+if (mem->start_addr <= start &&
+start + size <= mem->start_addr + mem->memory_size) {
+break;
+}
+}
+
+/*
+ * We should always find one memslot until this point, otherwise
+ * there could be something wrong from the upper layer
+ */
+assert(mem && i != s->nr_slots);
+ret = kvm_log_clear_one_slot(mem, kml->as_id, start, size);
kvm_slots_unlock(kml);
--
2.21.0
[PULL 06/12] s390x: sclp: Report insufficient SCCB length
From: Claudio Imbrenda
Return the correct error code when the SCCB buffer is too small to
contain all of the output, for the Read SCP Information and
Read CPU Information commands.
Signed-off-by: Claudio Imbrenda
Reviewed-by: Jason J. Herne
Message-Id: <1569591203-15258-5-git-send-email-imbre...@linux.ibm.com>
Reviewed-by: David Hildenbrand
Signed-off-by: Christian Borntraeger
---
hw/s390x/sclp.c | 12
1 file changed, 12 insertions(+)
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index abb6e5011f9c..f57ce7b73943 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -68,6 +68,12 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
+if (be16_to_cpu(sccb->h.length) <
+(sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {
+sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+return;
+}
+
/* Configuration Characteristic (Extension) */
s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR,
read_info->conf_char);
@@ -118,6 +124,12 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB
*sccb)
cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
cpu_info->nr_standby = cpu_to_be16(0);
+if (be16_to_cpu(sccb->h.length) <
+(sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
+sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+return;
+}
+
/* The standby offset is 16-byte for each CPU */
cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured
+ cpu_info->nr_configured*sizeof(CPUEntry));
--
2.21.0
[PULL 09/12] kvm: clear dirty bitmaps from all overlapping memslots
From: Paolo Bonzini
Currently MemoryRegionSection has 1:1 mapping to KVMSlot.
However next patch will allow splitting MemoryRegionSection into
several KVMSlot-s, make sure that kvm_physical_log_slot_clear()
is able to handle such 1:N mapping.
Signed-off-by: Paolo Bonzini
Signed-off-by: Igor Mammedov
Reviewed-by: Peter Xu
Message-Id: <20190924144751.24149-3-imamm...@redhat.com>
Acked-by: Paolo Bonzini
Signed-off-by: Christian Borntraeger
---
accel/kvm/kvm-all.c | 36 ++--
1 file changed, 22 insertions(+), 14 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index a85ec09486dd..ff9b95c0d103 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -589,8 +589,8 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id,
uint64_t start,
* satisfy the KVM interface requirement. Firstly, do the start
* page alignment on 64 host pages
*/
-bmap_start = (start - mem->start_addr) & KVM_CLEAR_LOG_MASK;
-start_delta = start - mem->start_addr - bmap_start;
+bmap_start = start & KVM_CLEAR_LOG_MASK;
+start_delta = start - bmap_start;
bmap_start /= psize;
/*
@@ -694,8 +694,8 @@ static int kvm_physical_log_clear(KVMMemoryListener *kml,
MemoryRegionSection *section)
{
KVMState *s = kvm_state;
-uint64_t start, size;
-KVMSlot *mem = NULL;
+uint64_t start, size, offset, count;
+KVMSlot *mem;
int ret, i;
if (!s->manual_dirty_log_protect) {
@@ -713,22 +713,30 @@ static int kvm_physical_log_clear(KVMMemoryListener *kml,
kvm_slots_lock(kml);
-/* Find any possible slot that covers the section */
for (i = 0; i < s->nr_slots; i++) {
mem = >slots[i];
-if (mem->start_addr <= start &&
-start + size <= mem->start_addr + mem->memory_size) {
+/* Discard slots that are empty or do not overlap the section */
+if (!mem->memory_size ||
+mem->start_addr > start + size - 1 ||
+start > mem->start_addr + mem->memory_size - 1) {
+continue;
+}
+
+if (start >= mem->start_addr) {
+/* The slot starts before section or is aligned to it. */
+offset = start - mem->start_addr;
+count = MIN(mem->memory_size - offset, size);
+} else {
+/* The slot starts after section. */
+offset = 0;
+count = MIN(mem->memory_size, size - (mem->start_addr - start));
+}
+ret = kvm_log_clear_one_slot(mem, kml->as_id, offset, count);
+if (ret < 0) {
break;
}
}
-/*
- * We should always find one memslot until this point, otherwise
- * there could be something wrong from the upper layer
- */
-assert(mem && i != s->nr_slots);
-ret = kvm_log_clear_one_slot(mem, kml->as_id, start, size);
-
kvm_slots_unlock(kml);
return ret;
--
2.21.0
[PULL 01/12] MAINTAINERS: Update S390 PCI Maintainer
From: Matthew Rosato As discussed previously with Collin, I will take over maintaining s390 pci. Signed-off-by: Matthew Rosato Message-Id: <1569590461-12562-1-git-send-email-mjros...@linux.ibm.com> Acked-by: Collin Walling Signed-off-by: Christian Borntraeger --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index bd7ee2310184..21264eae9c43 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1206,7 +1206,7 @@ T: git https://github.com/borntraeger/qemu.git s390-next L: qemu-s3...@nongnu.org S390 PCI -M: Collin Walling +M: Matthew Rosato S: Supported F: hw/s390x/s390-pci* L: qemu-s3...@nongnu.org -- 2.21.0
[PULL 04/12] s390x: sclp: boundary check
From: Janosch Frank
All sclp codes need to be checked for page boundary violations.
Signed-off-by: Janosch Frank
Reviewed-by: Jason J. Herne
Message-Id: <1569591203-15258-3-git-send-email-imbre...@linux.ibm.com>
Reviewed-by: David Hildenbrand
Signed-off-by: Christian Borntraeger
---
hw/s390x/sclp.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 95ebfe7bd2f1..73244c938b10 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -234,6 +234,11 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb,
uint32_t code)
goto out_write;
}
+if ((sccb + be16_to_cpu(work_sccb.h.length)) > ((sccb & PAGE_MASK) +
PAGE_SIZE)) {
+work_sccb.h.response_code =
cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
+goto out_write;
+}
+
sclp_c->execute(sclp, _sccb, code);
out_write:
cpu_physical_memory_write(sccb, _sccb,
--
2.21.0
[PULL 00/12] s390x qemu updates 20190930
Peter, The following changes since commit 786d36ad416c6c199b18b78cc31eddfb784fe15d: Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20190927' into staging (2019-09-30 11:02:22 +0100) are available in the Git repository at: git://github.com/borntraeger/qemu.git tags/s390x-20190930 for you to fetch changes up to c5b9ce518c0551d0198bcddadc82e03de9ac8de9: s390/kvm: split kvm mem slots at 4TB (2019-09-30 13:51:50 +0200) - do not abuse memory_region_allocate_system_memory and split the memory according to KVM memslots in KVM code instead (Paolo, Igor) - change splitting to split at 4TB (Christian) - do not claim s390 (31bit) support in configure (Thomas) - sclp error checking (Janosch, Claudio) - new s390 pci maintainer (Matt, Collin) - fix s390 pci (again) (Matt) Christian Borntraeger (1): s390/kvm: split kvm mem slots at 4TB Claudio Imbrenda (1): s390x: sclp: Report insufficient SCCB length Igor Mammedov (2): kvm: split too big memory section on several memslots s390: do not call memory_region_allocate_system_memory() multiple times Janosch Frank (3): s390x: sclp: refactor invalid command check s390x: sclp: boundary check s390x: sclp: fix error handling for oversize control blocks Matthew Rosato (2): MAINTAINERS: Update S390 PCI Maintainer s390: PCI: fix IOMMU region init Paolo Bonzini (2): kvm: extract kvm_log_clear_one_slot kvm: clear dirty bitmaps from all overlapping memslots Thomas Huth (1): configure: Remove s390 (31-bit mode) from the list of supported CPUs MAINTAINERS| 2 +- accel/kvm/kvm-all.c| 237 - configure | 2 +- hw/s390x/event-facility.c | 3 - hw/s390x/s390-pci-bus.c| 7 +- hw/s390x/s390-virtio-ccw.c | 30 +- hw/s390x/sclp.c| 37 ++- include/sysemu/kvm_int.h | 1 + target/s390x/kvm.c | 10 ++ 9 files changed, 202 insertions(+), 127 deletions(-)
[PULL 10/12] kvm: split too big memory section on several memslots
From: Igor Mammedov
Max memslot size supported by kvm on s390 is 8Tb,
move logic of splitting RAM in chunks upto 8T to KVM code.
This way it will hide KVM specific restrictions in KVM code
and won't affect board level design decisions. Which would allow
us to avoid misusing memory_region_allocate_system_memory() API
and eventually use a single hostmem backend for guest RAM.
Signed-off-by: Igor Mammedov
Message-Id: <20190924144751.24149-4-imamm...@redhat.com>
Reviewed-by: Peter Xu
Acked-by: Paolo Bonzini
Signed-off-by: Christian Borntraeger
---
accel/kvm/kvm-all.c | 122 +--
include/sysemu/kvm_int.h | 1 +
2 files changed, 80 insertions(+), 43 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index ff9b95c0d103..aabe097c410f 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -140,6 +140,7 @@ bool kvm_direct_msi_allowed;
bool kvm_ioeventfd_any_length_allowed;
bool kvm_msi_use_devid;
static bool kvm_immediate_exit;
+static hwaddr kvm_max_slot_size = ~0;
static const KVMCapabilityInfo kvm_required_capabilites[] = {
KVM_CAP_INFO(USER_MEMORY),
@@ -437,7 +438,7 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml,
KVMSlot *mem,
static int kvm_section_update_flags(KVMMemoryListener *kml,
MemoryRegionSection *section)
{
-hwaddr start_addr, size;
+hwaddr start_addr, size, slot_size;
KVMSlot *mem;
int ret = 0;
@@ -448,13 +449,18 @@ static int kvm_section_update_flags(KVMMemoryListener
*kml,
kvm_slots_lock(kml);
-mem = kvm_lookup_matching_slot(kml, start_addr, size);
-if (!mem) {
-/* We don't have a slot if we want to trap every access. */
-goto out;
-}
+while (size && !ret) {
+slot_size = MIN(kvm_max_slot_size, size);
+mem = kvm_lookup_matching_slot(kml, start_addr, slot_size);
+if (!mem) {
+/* We don't have a slot if we want to trap every access. */
+goto out;
+}
-ret = kvm_slot_update_flags(kml, mem, section->mr);
+ret = kvm_slot_update_flags(kml, mem, section->mr);
+start_addr += slot_size;
+size -= slot_size;
+}
out:
kvm_slots_unlock(kml);
@@ -527,11 +533,15 @@ static int
kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
struct kvm_dirty_log d = {};
KVMSlot *mem;
hwaddr start_addr, size;
+hwaddr slot_size, slot_offset = 0;
int ret = 0;
size = kvm_align_section(section, _addr);
-if (size) {
-mem = kvm_lookup_matching_slot(kml, start_addr, size);
+while (size) {
+MemoryRegionSection subsection = *section;
+
+slot_size = MIN(kvm_max_slot_size, size);
+mem = kvm_lookup_matching_slot(kml, start_addr, slot_size);
if (!mem) {
/* We don't have a slot if we want to trap every access. */
goto out;
@@ -549,11 +559,11 @@ static int
kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
* So for now, let's align to 64 instead of HOST_LONG_BITS here, in
* a hope that sizeof(long) won't become >8 any time soon.
*/
-size = ALIGN(((mem->memory_size) >> TARGET_PAGE_BITS),
- /*HOST_LONG_BITS*/ 64) / 8;
if (!mem->dirty_bmap) {
+hwaddr bitmap_size = ALIGN(((mem->memory_size) >>
TARGET_PAGE_BITS),
+/*HOST_LONG_BITS*/ 64) / 8;
/* Allocate on the first log_sync, once and for all */
-mem->dirty_bmap = g_malloc0(size);
+mem->dirty_bmap = g_malloc0(bitmap_size);
}
d.dirty_bitmap = mem->dirty_bmap;
@@ -564,7 +574,13 @@ static int
kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
goto out;
}
-kvm_get_dirty_pages_log_range(section, d.dirty_bitmap);
+subsection.offset_within_region += slot_offset;
+subsection.size = int128_make64(slot_size);
+kvm_get_dirty_pages_log_range(, d.dirty_bitmap);
+
+slot_offset += slot_size;
+start_addr += slot_size;
+size -= slot_size;
}
out:
return ret;
@@ -972,6 +988,14 @@ kvm_check_extension_list(KVMState *s, const
KVMCapabilityInfo *list)
return NULL;
}
+void kvm_set_max_memslot_size(hwaddr max_slot_size)
+{
+g_assert(
+ROUND_UP(max_slot_size, qemu_real_host_page_size) == max_slot_size
+);
+kvm_max_slot_size = max_slot_size;
+}
+
static void kvm_set_phys_mem(KVMMemoryListener *kml,
MemoryRegionSection *section, bool add)
{
@@ -979,7 +1003,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
int err;
MemoryRegion *mr = section->mr;
bool writeable = !mr->readonly && !mr->rom_device;
-hwaddr start_addr, size;
+hwaddr start_addr, size, slot_size;
void *ram;
if (!memory_reg
[PULL 12/12] s390/kvm: split kvm mem slots at 4TB
Instead of splitting at an unaligned address, we can simply split at 4TB. Signed-off-by: Christian Borntraeger Acked-by: Igor Mammedov --- target/s390x/kvm.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index 54864c259c5e..c24c869e7703 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -126,12 +126,11 @@ /* * KVM does only support memory slots up to KVM_MEM_MAX_NR_PAGES pages * as the dirty bitmap must be managed by bitops that take an int as - * position indicator. If we have a guest beyond that we will split off - * new subregions. The split must happen on a segment boundary (1MB). + * position indicator. This would end at an unaligned address + * (0x7f0). As future variants might provide larger pages + * and to make all addresses properly aligned, let us split at 4TB. */ -#define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1) -#define SEG_MSK (~0xfULL) -#define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) & SEG_MSK) +#define KVM_SLOT_MAX_BYTES (4UL * TiB) static CPUWatchpoint hw_watchpoint; /* -- 2.21.0
[PULL 11/12] s390: do not call memory_region_allocate_system_memory() multiple times
From: Igor Mammedov
s390 was trying to solve limited KVM memslot size issue by abusing
memory_region_allocate_system_memory(), which breaks API contract
where the function might be called only once.
Beside an invalid use of API, the approach also introduced migration
issue, since RAM chunks for each KVM_SLOT_MAX_BYTES are transferred in
migration stream as separate RAMBlocks.
After discussion [1], it was agreed to break migration from older
QEMU for guest with RAM >8Tb (as it was relatively new (since 2.12)
and considered to be not actually used downstream).
Migration should keep working for guests with less than 8TB and for
more than 8TB with QEMU 4.2 and newer binary.
In case user tries to migrate more than 8TB guest, between incompatible
QEMU versions, migration should fail gracefully due to non-exiting
RAMBlock ID or RAMBlock size mismatch.
Taking in account above and that now KVM code is able to split too
big MemorySection into several memslots, partially revert commit
(bb223055b s390-ccw-virtio: allow for systems larger that 7.999TB)
and use kvm_set_max_memslot_size() to set KVMSlot size to
KVM_SLOT_MAX_BYTES.
1) [PATCH RFC v2 4/4] s390: do not call memory_region_allocate_system_memory()
multiple times
Signed-off-by: Igor Mammedov
Message-Id: <20190924144751.24149-5-imamm...@redhat.com>
Acked-by: Peter Xu
Signed-off-by: Christian Borntraeger
---
hw/s390x/s390-virtio-ccw.c | 30 +++---
target/s390x/kvm.c | 11 +++
2 files changed, 14 insertions(+), 27 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 8bfb6684cb72..18ad279a00a3 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -154,39 +154,15 @@ static void virtio_ccw_register_hcalls(void)
virtio_ccw_hcall_early_printk);
}
-/*
- * KVM does only support memory slots up to KVM_MEM_MAX_NR_PAGES pages
- * as the dirty bitmap must be managed by bitops that take an int as
- * position indicator. If we have a guest beyond that we will split off
- * new subregions. The split must happen on a segment boundary (1MB).
- */
-#define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1)
-#define SEG_MSK (~0xfULL)
-#define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) &
SEG_MSK)
static void s390_memory_init(ram_addr_t mem_size)
{
MemoryRegion *sysmem = get_system_memory();
-ram_addr_t chunk, offset = 0;
-unsigned int number = 0;
+MemoryRegion *ram = g_new(MemoryRegion, 1);
Error *local_err = NULL;
-gchar *name;
/* allocate RAM for core */
-name = g_strdup_printf("s390.ram");
-while (mem_size) {
-MemoryRegion *ram = g_new(MemoryRegion, 1);
-uint64_t size = mem_size;
-
-/* KVM does not allow memslots >= 8 TB */
-chunk = MIN(size, KVM_SLOT_MAX_BYTES);
-memory_region_allocate_system_memory(ram, NULL, name, chunk);
-memory_region_add_subregion(sysmem, offset, ram);
-mem_size -= chunk;
-offset += chunk;
-g_free(name);
-name = g_strdup_printf("s390.ram.%u", ++number);
-}
-g_free(name);
+memory_region_allocate_system_memory(ram, NULL, "s390.ram", mem_size);
+memory_region_add_subregion(sysmem, 0, ram);
/*
* Configure the maximum page size. As no memory devices were created
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 97a662ad0ebf..54864c259c5e 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -28,6 +28,7 @@
#include "cpu.h"
#include "internal.h"
#include "kvm_s390x.h"
+#include "sysemu/kvm_int.h"
#include "qapi/error.h"
#include "qemu/error-report.h"
#include "qemu/timer.h"
@@ -122,6 +123,15 @@
*/
#define VCPU_IRQ_BUF_SIZE(max_cpus) (sizeof(struct kvm_s390_irq) * \
(max_cpus + NR_LOCAL_IRQS))
+/*
+ * KVM does only support memory slots up to KVM_MEM_MAX_NR_PAGES pages
+ * as the dirty bitmap must be managed by bitops that take an int as
+ * position indicator. If we have a guest beyond that we will split off
+ * new subregions. The split must happen on a segment boundary (1MB).
+ */
+#define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1)
+#define SEG_MSK (~0xfULL)
+#define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) &
SEG_MSK)
static CPUWatchpoint hw_watchpoint;
/*
@@ -355,6 +365,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
*/
/* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
+kvm_set_max_memslot_size(KVM_SLOT_MAX_BYTES);
return 0;
}
--
2.21.0
[PULL 07/12] configure: Remove s390 (31-bit mode) from the list of supported CPUs
From: Thomas Huth On IBM Z, KVM in the kernel is only implemented for 64-bit mode, and with regards to TCG, we also only support 64-bit host CPUs (see the check at the beginning of tcg/s390/tcg-target.inc.c), so we should remove s390 (without "x", i.e. the old 31-bit mode CPUs) from the list of supported CPUs. Signed-off-by: Thomas Huth Message-Id: <20190928190334.6897-1-th...@redhat.com> Reviewed-by: David Hildenbrand Signed-off-by: Christian Borntraeger --- configure | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure b/configure index 542f6aea3f61..8f8446f52b92 100755 --- a/configure +++ b/configure @@ -728,7 +728,7 @@ ARCH= # Normalise host CPU name and set ARCH. # Note that this case should only have supported host CPUs, not guests. case "$cpu" in - ppc|ppc64|s390|s390x|sparc64|x32|riscv32|riscv64) + ppc|ppc64|s390x|sparc64|x32|riscv32|riscv64) supported_cpu="yes" ;; ppc64le) -- 2.21.0
[PULL 05/12] s390x: sclp: fix error handling for oversize control blocks
From: Janosch Frank
Requests over 4k are not a spec exception.
Signed-off-by: Janosch Frank
Reviewed-by: Jason J. Herne
Message-Id: <1569591203-15258-4-git-send-email-imbre...@linux.ibm.com>
Acked-by: David Hildenbrand
Signed-off-by: Christian Borntraeger
---
hw/s390x/sclp.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 73244c938b10..abb6e5011f9c 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -213,8 +213,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb,
uint32_t code)
cpu_physical_memory_read(sccb, _sccb, sccb_len);
/* Valid sccb sizes */
-if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader) ||
-be16_to_cpu(work_sccb.h.length) > SCCB_SIZE) {
+if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
r = -PGM_SPECIFICATION;
goto out;
}
--
2.21.0
Re: [PATCH v7 0/4] s390: stop abusing memory_region_allocate_system_memory()
On 24.09.19 16:47, Igor Mammedov wrote: > Changelog: > since v6: > - include and rebase on top of >[PATCH 0/2] kvm: clear dirty bitmaps from all overlapping memslots > https://www.mail-archive.com/qemu-devel@nongnu.org/msg646200.html > - minor fixups suggested during v6 review > - more testing incl. hacked x86 > since v5: > - [1/2] fix migration that wasn't starting and make sure that KVM part > is able to handle 1:n MemorySection:memslot arrangement > since v3: > - fix compilation issue > - advance HVA along with GPA in kvm_set_phys_mem() > since v2: > - break migration from old QEMU (since 2.12-4.1) for guest with >8TB RAM > and drop migratable aliases patch as was agreed during v2 review > - drop 4.2 machines patch as it's not prerequisite anymore > since v1: > - include 4.2 machines patch for adding compat RAM layout on top > - 2/4 add missing in v1 patch for splitting too big MemorySection on > several memslots > - 3/4 amend code path on alias destruction to ensure that RAMBlock is > cleaned properly > - 4/4 add compat machine code to keep old layout (migration-wise) for > 4.1 and older machines > > > While looking into unifying guest RAM allocation to use hostmem backends > for initial RAM (especially when -mempath is used) and retiring > memory_region_allocate_system_memory() API, leaving only single hostmem > backend, > I was inspecting how currently it is used by boards and it turns out several > boards abuse it by calling the function several times (despite documented > contract > forbiding it). > > s390 is one of such boards where KVM limitation on memslot size got propagated > to board design and memory_region_allocate_system_memory() was abused to > satisfy > KVM requirement for max RAM chunk where memory region alias would suffice. > > Unfortunately, memory_region_allocate_system_memory() usage created migration > dependency where guest RAM is transferred in migration stream as several > RAMBlocks > if it's more than KVM_SLOT_MAX_BYTES. During v2 review it was agreed to ignore > migration breakage (documenting it in release notes) and leaving only KVM fix. > > In order to replace these several RAM chunks with a single memdev and keep it > working with KVM memslot size limit, the later was modified to deal with > memory section split on several KVMSlots and manual RAM splitting in s390 > was replace by single memory_region_allocate_system_memory() call. > > Tested: > * s390 with hacked KVM_SLOT_MAX_BYTES = 128Mb > - guest reboot cycle in ping-pong migration > * x86 with hacke max memslot = 128 and manual_dirty_log_protect enabled > - ping-pong migration with workload dirtying RAM around a split area > Thanks, v7 applied to s390-next.
Re: [PATCH v7 1/4] kvm: extract kvm_log_clear_one_slot
On 24.09.19 16:47, Igor Mammedov wrote: > From: Paolo Bonzini > > We may need to clear the dirty bitmap for more than one KVM memslot. > First do some code movement with no semantic change. > > Signed-off-by: Paolo Bonzini > Signed-off-by: Igor Mammedov > Reviewed-by: Peter Xu > --- > accel/kvm/kvm-all.c | 102 > 1 file changed, 56 insertions(+), 46 deletions(-) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index b09bad0804..e9e6086c09 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -575,55 +575,13 @@ out: > #define KVM_CLEAR_LOG_ALIGN (qemu_real_host_page_size << > KVM_CLEAR_LOG_SHIFT) > #define KVM_CLEAR_LOG_MASK (-KVM_CLEAR_LOG_ALIGN) > > -/** > - * kvm_physical_log_clear - Clear the kernel's dirty bitmap for range > - * > - * NOTE: this will be a no-op if we haven't enabled manual dirty log > - * protection in the host kernel because in that case this operation > - * will be done within log_sync(). > - * > - * @kml: the kvm memory listener > - * @section: the memory range to clear dirty bitmap > - */ > -static int kvm_physical_log_clear(KVMMemoryListener *kml, > - MemoryRegionSection *section) > +static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start, > uint64_t size) When applying, I will split this to fit within 80 chars. ok?
Re: [PATCH] configure: Remove s390 (31-bit mode) from the list of supported CPUs
On 28.09.19 21:03, Thomas Huth wrote: > On IBM Z, KVM in the kernel is only implemented for 64-bit mode, and > with regards to TCG, we also only support 64-bit host CPUs (see the > check at the beginning of tcg/s390/tcg-target.inc.c), so we should > remove s390 (without "x", i.e. the old 31-bit mode CPUs) from the > list of supported CPUs. > > Signed-off-by: Thomas Huth > --- > configure | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/configure b/configure > index 397bb476e1..a4488c6705 100755 > --- a/configure > +++ b/configure > @@ -728,7 +728,7 @@ ARCH= > # Normalise host CPU name and set ARCH. > # Note that this case should only have supported host CPUs, not guests. > case "$cpu" in > - ppc|ppc64|s390|s390x|sparc64|x32|riscv32|riscv64) > + ppc|ppc64|s390x|sparc64|x32|riscv32|riscv64) > supported_cpu="yes" >;; >ppc64le) > Thanks applied to s390-next.
Re: [PATCH v7 4/4] s390: do not call memory_region_allocate_system_memory() multiple times
On 30.09.19 11:33, Igor Mammedov wrote: > On Mon, 30 Sep 2019 09:09:59 +0200 > Christian Borntraeger wrote: > >> On 28.09.19 03:28, Peter Xu wrote: >>> On Fri, Sep 27, 2019 at 03:33:20PM +0200, Igor Mammedov wrote: >>>> On Thu, 26 Sep 2019 07:52:35 +0800 >>>> Peter Xu wrote: >>>> >>>>> On Wed, Sep 25, 2019 at 01:51:05PM +0200, Igor Mammedov wrote: >>>>>> On Wed, 25 Sep 2019 11:27:00 +0800 >>>>>> Peter Xu wrote: >>>>>> >>>>>>> On Tue, Sep 24, 2019 at 10:47:51AM -0400, Igor Mammedov wrote: >>>>>>>> s390 was trying to solve limited KVM memslot size issue by abusing >>>>>>>> memory_region_allocate_system_memory(), which breaks API contract >>>>>>>> where the function might be called only once. >>>>>>>> >>>>>>>> Beside an invalid use of API, the approach also introduced migration >>>>>>>> issue, since RAM chunks for each KVM_SLOT_MAX_BYTES are transferred in >>>>>>>> migration stream as separate RAMBlocks. >>>>>>>> >>>>>>>> After discussion [1], it was agreed to break migration from older >>>>>>>> QEMU for guest with RAM >8Tb (as it was relatively new (since 2.12) >>>>>>>> and considered to be not actually used downstream). >>>>>>>> Migration should keep working for guests with less than 8TB and for >>>>>>>> more than 8TB with QEMU 4.2 and newer binary. >>>>>>>> In case user tries to migrate more than 8TB guest, between incompatible >>>>>>>> QEMU versions, migration should fail gracefully due to non-exiting >>>>>>>> RAMBlock ID or RAMBlock size mismatch. >>>>>>>> >>>>>>>> Taking in account above and that now KVM code is able to split too >>>>>>>> big MemorySection into several memslots, partially revert commit >>>>>>>> (bb223055b s390-ccw-virtio: allow for systems larger that 7.999TB) >>>>>>>> and use kvm_set_max_memslot_size() to set KVMSlot size to >>>>>>>> KVM_SLOT_MAX_BYTES. >>>>>>>> >>>>>>>> 1) [PATCH RFC v2 4/4] s390: do not call >>>>>>>> memory_region_allocate_system_memory() multiple times >>>>>>>> >>>>>>>> Signed-off-by: Igor Mammedov >>>>>>> >>>>>>> Acked-by: Peter Xu >>>>>>> >>>>>>> IMHO it would be good to at least mention bb223055b9 in the commit >>>>>>> message even if not with a "Fixed:" tag. May be amended during commit >>>>>>> if anyone prefers. >>>>>> >>>>>> /me confused, bb223055b9 is mentioned in commit message >>>>> >>>>> I'm sorry, I overlooked that. >>>>> >>>>>> >>>>>>> Also, this only applies the split limitation to s390. Would that be a >>>>>>> good thing to some other archs as well? >>>>>> >>>>>> Don't we have the similar bitmap size issue in KVM for other archs? >>>>> >>>>> Yes I thought we had. So I feel like it would be good to also allow >>>>> other archs to support >8TB mem as well. Thanks, >>>> Another question, Is there another archs with that much RAM that are >>>> available/used in real life (if not I'd wait for demand to arise first)? >>> >>> I don't know, so it was a pure question besides the series. Sorry if >>> that holds your series somehow, it was not my intention. >>> >>>> >>>> If we are to generalize it to other targets, then instead of using >>>> arbitrary memslot max size per target, we could just hardcode or get >>>> from KVM, max supported size of bitmap and use that to calculate >>>> kvm_max_slot_size depending on target page size. >>> >>> Right, I think if so hard code would be fine for now, and probably can >>> with a smallest one across all archs (should depend on the smallest >>> page size, I guess). >>> >>>> >>>> Then there wouldn't be need for having machine specific code >>>> to care about it and pick/set arbitrary values. >>>> >>>> Another aspect to think about if we are to enable it for >>>> other targets is memslot accounting. It doesn't affect s390 >>>> but other targets that support memory hotplug now assume 1:1 >>>> relation between memoryregion:memslot, which currently holds >>>> true but would need to amended in case split is enabled there. >>> >>> I didn't know this. So maybe it makes more sense to have s390 only >>> here. Thanks, >> >> OK. So shall I take the series as is via the s390 tree? > Yes, I'd appreciate it. Paolo, ok it I pick the first 3 patches as well? Can you ack?
Re: [PATCH v7 4/4] s390: do not call memory_region_allocate_system_memory() multiple times
On 28.09.19 03:28, Peter Xu wrote: > On Fri, Sep 27, 2019 at 03:33:20PM +0200, Igor Mammedov wrote: >> On Thu, 26 Sep 2019 07:52:35 +0800 >> Peter Xu wrote: >> >>> On Wed, Sep 25, 2019 at 01:51:05PM +0200, Igor Mammedov wrote: >>>> On Wed, 25 Sep 2019 11:27:00 +0800 >>>> Peter Xu wrote: >>>> >>>>> On Tue, Sep 24, 2019 at 10:47:51AM -0400, Igor Mammedov wrote: >>>>>> s390 was trying to solve limited KVM memslot size issue by abusing >>>>>> memory_region_allocate_system_memory(), which breaks API contract >>>>>> where the function might be called only once. >>>>>> >>>>>> Beside an invalid use of API, the approach also introduced migration >>>>>> issue, since RAM chunks for each KVM_SLOT_MAX_BYTES are transferred in >>>>>> migration stream as separate RAMBlocks. >>>>>> >>>>>> After discussion [1], it was agreed to break migration from older >>>>>> QEMU for guest with RAM >8Tb (as it was relatively new (since 2.12) >>>>>> and considered to be not actually used downstream). >>>>>> Migration should keep working for guests with less than 8TB and for >>>>>> more than 8TB with QEMU 4.2 and newer binary. >>>>>> In case user tries to migrate more than 8TB guest, between incompatible >>>>>> QEMU versions, migration should fail gracefully due to non-exiting >>>>>> RAMBlock ID or RAMBlock size mismatch. >>>>>> >>>>>> Taking in account above and that now KVM code is able to split too >>>>>> big MemorySection into several memslots, partially revert commit >>>>>> (bb223055b s390-ccw-virtio: allow for systems larger that 7.999TB) >>>>>> and use kvm_set_max_memslot_size() to set KVMSlot size to >>>>>> KVM_SLOT_MAX_BYTES. >>>>>> >>>>>> 1) [PATCH RFC v2 4/4] s390: do not call >>>>>> memory_region_allocate_system_memory() multiple times >>>>>> >>>>>> Signed-off-by: Igor Mammedov >>>>> >>>>> Acked-by: Peter Xu >>>>> >>>>> IMHO it would be good to at least mention bb223055b9 in the commit >>>>> message even if not with a "Fixed:" tag. May be amended during commit >>>>> if anyone prefers. >>>> >>>> /me confused, bb223055b9 is mentioned in commit message >>> >>> I'm sorry, I overlooked that. >>> >>>> >>>>> Also, this only applies the split limitation to s390. Would that be a >>>>> good thing to some other archs as well? >>>> >>>> Don't we have the similar bitmap size issue in KVM for other archs? >>> >>> Yes I thought we had. So I feel like it would be good to also allow >>> other archs to support >8TB mem as well. Thanks, >> Another question, Is there another archs with that much RAM that are >> available/used in real life (if not I'd wait for demand to arise first)? > > I don't know, so it was a pure question besides the series. Sorry if > that holds your series somehow, it was not my intention. > >> >> If we are to generalize it to other targets, then instead of using >> arbitrary memslot max size per target, we could just hardcode or get >> from KVM, max supported size of bitmap and use that to calculate >> kvm_max_slot_size depending on target page size. > > Right, I think if so hard code would be fine for now, and probably can > with a smallest one across all archs (should depend on the smallest > page size, I guess). > >> >> Then there wouldn't be need for having machine specific code >> to care about it and pick/set arbitrary values. >> >> Another aspect to think about if we are to enable it for >> other targets is memslot accounting. It doesn't affect s390 >> but other targets that support memory hotplug now assume 1:1 >> relation between memoryregion:memslot, which currently holds >> true but would need to amended in case split is enabled there. > > I didn't know this. So maybe it makes more sense to have s390 only > here. Thanks, OK. So shall I take the series as is via the s390 tree? I would like to add the following patch on top if nobody minds: Subject: [PATCH 1/1] s390/kvm: split kvm mem slots at 4TB Instead of splitting at an unaligned address, we can simply split at 4TB. Signed-off-by: Christian Borntraeger --- target/s390x/kvm
Re: [PATCH] s390: PCI: fix IOMMU region init
On 26.09.19 16:10, Matthew Rosato wrote:
> The fix in dbe9cf606c shrinks the IOMMU memory region to a size
> that seems reasonable on the surface, however is actually too
> small as it is based against a 0-mapped address space. This
> causes breakage with small guests as they can overrun the IOMMU window.
>
> Let's go back to the prior method of initializing iommu for now.
>
> Fixes: dbe9cf606c ("s390x/pci: Set the iommu region size mpcifc request")
> Reported-by: Boris Fiuczynski
> Reported-by: Stefan Zimmerman
> Signed-off-by: Matthew Rosato
> ---
> hw/s390x/s390-pci-bus.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 963a41c..2d2f4a7 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -695,10 +695,15 @@ static const MemoryRegionOps s390_msi_ctrl_ops = {
>
> void s390_pci_iommu_enable(S390PCIIOMMU *iommu)
> {
> +/*
> + * The iommu region is initialized against a 0-mapped address space,
> + * so the smallest IOMMU region we can define runs from 0 to the end
> + * of the PCI address space.
> + */
> char *name = g_strdup_printf("iommu-s390-%04x", iommu->pbdev->uid);
> memory_region_init_iommu(>iommu_mr, sizeof(iommu->iommu_mr),
> TYPE_S390_IOMMU_MEMORY_REGION,
> OBJECT(>mr),
> - name, iommu->pal - iommu->pba + 1);
> + name, iommu->pal + 1);
> iommu->enabled = true;
> memory_region_add_subregion(>mr, 0,
> MEMORY_REGION(>iommu_mr));
> g_free(name);
>
#
Thanks applied.
Re: [PATCH v2 3/4] s390x: sclp: fix error handling for oversize control blocks
On 27.09.19 15:33, Claudio Imbrenda wrote:
> From: Janosch Frank
>
> Requests over 4k are not a spec exception.
>
> Signed-off-by: Janosch Frank
> Reviewed-by: Jason J. Herne
> ---
> hw/s390x/sclp.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 73244c9..abb6e50 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -213,8 +213,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb,
> uint32_t code)
> cpu_physical_memory_read(sccb, _sccb, sccb_len);
>
> /* Valid sccb sizes */
> -if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader) ||
> -be16_to_cpu(work_sccb.h.length) > SCCB_SIZE) {
> +if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
> r = -PGM_SPECIFICATION;
> goto out;
> }
>
Thanks applied.
Re: [PATCH v2 4/4] s390x: Fix SCLP return code when buffer too small
On 27.09.19 15:33, Claudio Imbrenda wrote:
> Return the correct error code when the SCCB buffer is too small to
> contain all of the output, for the Read SCP Information and
> Read CPU Information commands.
>
> Signed-off-by: Claudio Imbrenda
> Reviewed-by: Jason J. Herne
> ---
> hw/s390x/sclp.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index abb6e50..f57ce7b 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -68,6 +68,12 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>
> read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
>
> +if (be16_to_cpu(sccb->h.length) <
> +(sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {
> +sccb->h.response_code =
> cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +return;
> +}
> +
> /* Configuration Characteristic (Extension) */
> s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR,
> read_info->conf_char);
> @@ -118,6 +124,12 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB
> *sccb)
> cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo,
> entries));
> cpu_info->nr_standby = cpu_to_be16(0);
>
> +if (be16_to_cpu(sccb->h.length) <
> +(sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
> +sccb->h.response_code =
> cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +return;
> +}
> +
> /* The standby offset is 16-byte for each CPU */
> cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured
> + cpu_info->nr_configured*sizeof(CPUEntry));
>
Thanks applied.
Re: [PATCH v2 2/4] s390x: sclp: boundary check
On 27.09.19 15:33, Claudio Imbrenda wrote:
> From: Janosch Frank
>
> All sclp codes need to be checked for page boundary violations.
>
> Signed-off-by: Janosch Frank
> Reviewed-by: Jason J. Herne
> ---
> hw/s390x/sclp.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 95ebfe7..73244c9 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -234,6 +234,11 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb,
> uint32_t code)
> goto out_write;
> }
>
> +if ((sccb + be16_to_cpu(work_sccb.h.length)) > ((sccb & PAGE_MASK) +
> PAGE_SIZE)) {
> +work_sccb.h.response_code =
> cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> +goto out_write;
> +}
> +
> sclp_c->execute(sclp, _sccb, code);
> out_write:
> cpu_physical_memory_write(sccb, _sccb,
>
checkpatch complains about the line length, but splitting makes it really
less readable.
Thanks applied.
Re: [PATCH v2 1/4] s390x: sclp: refactor invalid command check
On 27.09.19 15:33, Claudio Imbrenda wrote:
> From: Janosch Frank
>
> Invalid command checking has to be done before the boundary check,
> refactoring it now allows to insert the boundary check at the correct
> place later.
>
> Signed-off-by: Janosch Frank
> Reviewed-by: Jason J. Herne
> ---
> hw/s390x/event-facility.c | 3 ---
> hw/s390x/sclp.c | 17 -
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> index 797ecbb..6620569 100644
> --- a/hw/s390x/event-facility.c
> +++ b/hw/s390x/event-facility.c
> @@ -377,9 +377,6 @@ static void command_handler(SCLPEventFacility *ef, SCCB
> *sccb, uint64_t code)
> case SCLP_CMD_WRITE_EVENT_MASK:
> write_event_mask(ef, sccb);
> break;
> -default:
> -sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> -break;
> }
> }
>
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index fac7c3b..95ebfe7 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -219,8 +219,23 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb,
> uint32_t code)
> goto out;
> }
>
> -sclp_c->execute(sclp, _sccb, code);
> +switch (code & SCLP_CMD_CODE_MASK) {
> +case SCLP_CMDW_READ_SCP_INFO:
> +case SCLP_CMDW_READ_SCP_INFO_FORCED:
> +case SCLP_CMDW_READ_CPU_INFO:
> +case SCLP_CMDW_CONFIGURE_IOA:
> +case SCLP_CMDW_DECONFIGURE_IOA:
> +case SCLP_CMD_READ_EVENT_DATA:
> +case SCLP_CMD_WRITE_EVENT_DATA:
> +case SCLP_CMD_WRITE_EVENT_MASK:
> +break;
> +default:
> +work_sccb.h.response_code =
> cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> +goto out_write;
> +}
>
> +sclp_c->execute(sclp, _sccb, code);
> +out_write:
> cpu_physical_memory_write(sccb, _sccb,
>be16_to_cpu(work_sccb.h.length));
>
Thanks applied.
Re: [PATCH] MAINTAINERS: Update S390 PCI Maintainer
On 27.09.19 15:21, Matthew Rosato wrote: > As discussed previously with Collin, I will take over maintaining > s390 pci. > > Signed-off-by: Matthew Rosato > --- > MAINTAINERS | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index bd7ee23..21264ea 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1206,7 +1206,7 @@ T: git https://github.com/borntraeger/qemu.git s390-next > L: qemu-s3...@nongnu.org > > S390 PCI > -M: Collin Walling > +M: Matthew Rosato > S: Supported > F: hw/s390x/s390-pci* > L: qemu-s3...@nongnu.org > Thanks applied.
Re: [PATCH] s390: PCI: fix IOMMU region init
On 26.09.19 16:10, Matthew Rosato wrote:
> The fix in dbe9cf606c shrinks the IOMMU memory region to a size
> that seems reasonable on the surface, however is actually too
> small as it is based against a 0-mapped address space. This
> causes breakage with small guests as they can overrun the IOMMU window.
>
> Let's go back to the prior method of initializing iommu for now.
>
> Fixes: dbe9cf606c ("s390x/pci: Set the iommu region size mpcifc request")
> Reported-by: Boris Fiuczynski
> Reported-by: Stefan Zimmerman
> Signed-off-by: Matthew Rosato
Matt can you also send a patch adding you as the PCI maintainer now
that you have taken over from Collin?
> ---
> hw/s390x/s390-pci-bus.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 963a41c..2d2f4a7 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -695,10 +695,15 @@ static const MemoryRegionOps s390_msi_ctrl_ops = {
>
> void s390_pci_iommu_enable(S390PCIIOMMU *iommu)
> {
> +/*
> + * The iommu region is initialized against a 0-mapped address space,
> + * so the smallest IOMMU region we can define runs from 0 to the end
> + * of the PCI address space.
> + */
> char *name = g_strdup_printf("iommu-s390-%04x", iommu->pbdev->uid);
> memory_region_init_iommu(>iommu_mr, sizeof(iommu->iommu_mr),
> TYPE_S390_IOMMU_MEMORY_REGION,
> OBJECT(>mr),
> - name, iommu->pal - iommu->pba + 1);
> + name, iommu->pal + 1);
> iommu->enabled = true;
> memory_region_add_subregion(>mr, 0,
> MEMORY_REGION(>iommu_mr));
> g_free(name);
>
Re: [PATCH] s390: PCI: fix IOMMU region init
On 26.09.19 16:34, Peter Maydell wrote:
> On Thu, 26 Sep 2019 at 15:12, Matthew Rosato wrote:
>>
>> The fix in dbe9cf606c shrinks the IOMMU memory region to a size
>> that seems reasonable on the surface, however is actually too
>> small as it is based against a 0-mapped address space. This
>> causes breakage with small guests as they can overrun the IOMMU window.
>>
>> Let's go back to the prior method of initializing iommu for now.
>>
>> Fixes: dbe9cf606c ("s390x/pci: Set the iommu region size mpcifc request")
>> Reported-by: Boris Fiuczynski
>> Reported-by: Stefan Zimmerman
>> Signed-off-by: Matthew Rosato
>
> So in commit f0a399dbae6a2d0e2 (Nov 2015) we used "pal - pba + 1".
> In commit f7c40aa1e7feb50bc4 (June 2016) we switched to "pal + 1".
> In commit dbe9cf606c (Jan 2019) we went back to "pal - pba + 1"
> Now we're on "pal + 1" again...
>
> Are we really sure that this is correct and that we're not
> just going to keep looping around between these two formations
> forever? :-)
As Matt and Pierre outlined this is indeed the variant that works
reliably. I will add
Cc: qemu-sta...@nongnu.org
and apply.
Re: [RFC PATCH] configure: deprecate 32 bit build hosts
On 26.09.19 14:58, Daniel P. Berrangé wrote: > On Thu, Sep 26, 2019 at 08:50:36AM +0100, Peter Maydell wrote: >> On Thu, 26 Sep 2019 at 00:31, Alex Bennée wrote: >>> >>> The 32 bit hosts are already a second class citizen especially with >>> support for running 64 bit guests under TCG. We are also limited by >>> testing as actual working 32 bit machines are getting quite rare in >>> developers personal menageries. For TCG supporting newer types like >>> Int128 is a lot harder with 32 bit calling conventions compared to >>> their larger bit sized cousins. Fundamentally address space is the >>> most useful thing for the translator to have even for a 32 bit guest a >>> 32 bit host is quite constrained. >>> >>> As far as I'm aware 32 bit KVM users are even less numerous. Even >>> ILP32 doesn't make much sense given the address space QEMU needs to >>> manage. >> >> For KVM we should wait until the kernel chooses to drop support, >> I think. > > What if the kernel is waiting for QEMU to drop support too ;-P For what its worth on kvm/s390 we never cared about implementing 32 bit. > >>> @@ -745,19 +744,22 @@ case "$cpu" in >>>;; >>>armv*b|armv*l|arm) >>> cpu="arm" >>> -supported_cpu="yes" >>>;; >> >> I'll leave others to voice opinions about their architectures, >> but I still have 32-bit arm in my test set for builds, and >> I'm pretty sure we have users (raspi users, for a start). > > RHEL dropped all 32-bit host support a long time ago, so Red Hat > don't care for our products. > > Fedora has recently stopped building i686 kernels and thus also no > long composes i686 installs. Some users complained, but ultimately > no one cares enough to step forward as maintainers. > > That leaves armv7 as the only 32-bit arch in Fedora that is somewhat > active & maintained. I don't have any real insight on whether any > armv7 (Fedora) users are making much use of QEMU/KVM though, either > system or user emulation. > > Our preference in Fedora is to have things built on every architecture > that the distro targets, but if upstream developers explicitly drop an > architecture we're not going to try to add it back. > > Regards, > Daniel >
Re: [PATCH v4 0/8] Introduce the microvm machine type
On 24.09.19 14:44, Sergio Lopez wrote: > Microvm is a machine type inspired by both NEMU and Firecracker, and > constructed after the machine model implemented by the latter. > > It's main purpose is providing users a minimalist machine type free > from the burden of legacy compatibility, serving as a stepping stone > for future projects aiming at improving boot times, reducing the > attack surface and slimming down QEMU's footprint. > > The microvm machine type supports the following devices: > > - ISA bus > - i8259 PIC > - LAPIC (implicit if using KVM) > - IOAPIC (defaults to kernel_irqchip_split = true) > - i8254 PIT > - MC146818 RTC (optional) > - kvmclock (if using KVM) > - fw_cfg > - One ISA serial port (optional) > - Up to eight virtio-mmio devices (configured by the user) Just out of curiosity. What is the reason for not going virtio-pci? Is the PCI bus really that expensive and complicated? FWIW, I do not complain. When people start using virtio-mmio more often this would also help virtio-ccw (which I am interested in) as this forces people to think beyond virtio-pci.
Re: [PATCH v7 0/4] s390: stop abusing memory_region_allocate_system_memory()
On 24.09.19 16:47, Igor Mammedov wrote: > Changelog: > since v6: > - include and rebase on top of >[PATCH 0/2] kvm: clear dirty bitmaps from all overlapping memslots > https://www.mail-archive.com/qemu-devel@nongnu.org/msg646200.html > - minor fixups suggested during v6 review > - more testing incl. hacked x86 > since v5: > - [1/2] fix migration that wasn't starting and make sure that KVM part > is able to handle 1:n MemorySection:memslot arrangement > since v3: > - fix compilation issue > - advance HVA along with GPA in kvm_set_phys_mem() > since v2: > - break migration from old QEMU (since 2.12-4.1) for guest with >8TB RAM > and drop migratable aliases patch as was agreed during v2 review > - drop 4.2 machines patch as it's not prerequisite anymore > since v1: > - include 4.2 machines patch for adding compat RAM layout on top > - 2/4 add missing in v1 patch for splitting too big MemorySection on > several memslots > - 3/4 amend code path on alias destruction to ensure that RAMBlock is > cleaned properly > - 4/4 add compat machine code to keep old layout (migration-wise) for > 4.1 and older machines > > > While looking into unifying guest RAM allocation to use hostmem backends > for initial RAM (especially when -mempath is used) and retiring > memory_region_allocate_system_memory() API, leaving only single hostmem > backend, > I was inspecting how currently it is used by boards and it turns out several > boards abuse it by calling the function several times (despite documented > contract > forbiding it). > > s390 is one of such boards where KVM limitation on memslot size got propagated > to board design and memory_region_allocate_system_memory() was abused to > satisfy > KVM requirement for max RAM chunk where memory region alias would suffice. > > Unfortunately, memory_region_allocate_system_memory() usage created migration > dependency where guest RAM is transferred in migration stream as several > RAMBlocks > if it's more than KVM_SLOT_MAX_BYTES. During v2 review it was agreed to ignore > migration breakage (documenting it in release notes) and leaving only KVM fix. > > In order to replace these several RAM chunks with a single memdev and keep it > working with KVM memslot size limit, the later was modified to deal with > memory section split on several KVMSlots and manual RAM splitting in s390 > was replace by single memory_region_allocate_system_memory() call. > > Tested: > * s390 with hacked KVM_SLOT_MAX_BYTES = 128Mb > - guest reboot cycle in ping-pong migration > * x86 with hacke max memslot = 128 and manual_dirty_log_protect enabled > - ping-pong migration with workload dirtying RAM around a split area > > > > Igor Mammedov (2): > kvm: split too big memory section on several memslots > s390: do not call memory_region_allocate_system_memory() multiple > times > > Paolo Bonzini (2): > kvm: extract kvm_log_clear_one_slot > kvm: clear dirty bitmaps from all overlapping memslots > > include/sysemu/kvm_int.h | 1 + > accel/kvm/kvm-all.c | 238 +++-- > hw/s390x/s390-virtio-ccw.c | 30 + > target/s390x/kvm.c | 11 ++ > 4 files changed, 161 insertions(+), 119 deletions(-) > Series Tested-by: Christian Borntraeger --- target/s390x/kvm.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index ad2dd14f7e78..611f56f4b5ac 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -126,12 +126,11 @@ /* * KVM does only support memory slots up to KVM_MEM_MAX_NR_PAGES pages * as the dirty bitmap must be managed by bitops that take an int as - * position indicator. If we have a guest beyond that we will split off - * new subregions. The split must happen on a segment boundary (1MB). + * position indicator. This would end at an unaligned address + * (0x7f0). As future variants might provide larger pages + * and to make all addresses properly aligned, let us split at 4TB. */ -#define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1) -#define SEG_MSK (~0xfULL) -#define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) & SEG_MSK) +#define KVM_SLOT_MAX_BYTES 4096UL*1024*1024*1024 static CPUWatchpoint hw_watchpoint; /* -- 2.21.0
[PULL 3/5] pc-bios/s390-ccw: Rebuild the s390-netboot.img firmware image
From: Thomas Huth
The new image now contains the "pc-bios/s390-ccw/net: fix a possible
memory leak in get_uuid()" patch.
Signed-off-by: Thomas Huth
---
pc-bios/s390-netboot.img | Bin 67232 -> 67232 bytes
1 file changed, 0 insertions(+), 0 deletions(-)
diff --git a/pc-bios/s390-netboot.img b/pc-bios/s390-netboot.img
index
aa90fbccb142470924ad5f0de5a4c6337916c63a..b984ad0da08f1401978b173d44e935d5563dde70
100644
GIT binary patch
literal 67232
zcmeEvd3;pW75{yc$-+JfNtgtM7ZM;W!h|^G>hfXj%W!HtNhxTB_OtE~e@#ibge
zv~H~f5+7=9he#FNh^C+-jnci%DIYlde;y&=zsB$c<~pn?kYxZlw+@zBFpmP^+0@~%=t
z`Yj$R_1Q)p6Au)BWh50u%)Hwtxnos%9J9J1n7qb39IYT3&;-T<`w8i;S
zbqQ60j!%T{B~_oEfVfmAzPo`3<>n!?Q(tGg;&_l^S3?RL0l~#k9!X=X?jJf=h
zF-4L}7@7Db%{p#@de1%VPr0T4dFj@Y9*ZwM_4*A>y*_(h!;p3-Z`d9wtK6o`e^`15
z&-};uw6FfPc-$=w-=uum^Yzj1e=_#N;>{D@xaO|g`aH4e?K>pbLHHPPRT^
zvJ;<>XC`H)E7hA1sq6mI$PoW1mxpzTUn#L#u16rg7vEEtC*;_~>%==
zGOTJ3D9|Pf*kmXfLnFDX~sB~{$g{MZuV)vB8g8FOPc6
zi%W*`^<|gOuwodX73^n*%@_Y1q1SS0ihJ~v|s;xnunNy|W`vFJv@Z9>#vldNi@z8W|G#=@!~>W{6*-(Zak
zG5#7uQWgGI)+DK#pr_?3!an;}he|K2HBx4ZHO98RE39f`jFcHD
zWvVk)S~HCK706``LYr0_OAw}5)vi*>B{iLWX@~SGCp{-n)4ViJL?#E}OaZ(q
zV_YKsR)u)!Q10cF$Bp0vXLqSC925+^*m5KO`eUCX#uxh#fBh{tAja2nJ^q@pcM#)=HR7+?QjHi-
z%XRo0j5RQ4Y!m+aViBf_+^E<~c=m`SRko~P(o)>xX32un%59c`(ixODfLT*{}@H
zBLd(d$^aJnC+H@;0AvYIkVPveO=*arepW*zN{q4=s!TvLnww0?#)Yn{~
zGMmO@rK}Ir0^`HdQ_RoeQs6}^p{1|13XEQqk_)~dE_u(QWm4E)pz|t~8JE0?Jpn~z
zk-3WZWDF50ETBAERc%i}E+!Mo`A=KFux;j9Qeyq4?f$rQpTAM!#!8;Q$@4;~)y3z$
z>j1q((!Q3ofmUnVmZ;fWsIvKf)X8zIQP-W(%H~nzX!A*n`jE3m%CBSov-Y5T
zHS$fDd}Wd^Ao=`GzI!F#e~~ZL;8E(AbF2~*{KX@CUBH_Ap#n`a$U8`m2f7t-2me#_
[PULL 4/5] s390x/kvm: Officially require at least kernel 3.15
From: Thomas Huth
Since QEMU v2.10, the KVM acceleration does not work on older kernels
anymore since the code accidentally requires the KVM_CAP_DEVICE_CTRL
capability now - it should have been optional instead.
Instead of fixing the bug, we asked in the ChangeLog of QEMU 2.11 - 3.0
that people should speak up if they still need support of QEMU running
with KVM on older kernels, but seems like nobody really complained.
Thus let's make this official now and turn it into a proper error
message, telling the users to use at least kernel 3.15 now.
Signed-off-by: Thomas Huth
Message-Id: <20190913091443.27565-1-th...@redhat.com>
Reviewed-by: David Hildenbrand
Signed-off-by: Christian Borntraeger
---
hw/intc/s390_flic_kvm.c | 6 --
hw/intc/trace-events| 1 -
target/s390x/kvm.c | 7 +++
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index 819aa5e198b7..cedccba8a9c7 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -589,12 +589,6 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error
**errp)
goto fail;
}
flic_state->fd = -1;
-if (!kvm_check_extension(kvm_state, KVM_CAP_DEVICE_CTRL)) {
-error_setg_errno(_local, errno, "KVM is missing capability"
- " KVM_CAP_DEVICE_CTRL");
-trace_flic_no_device_api(errno);
-goto fail;
-}
cd.type = KVM_DEV_TYPE_FLIC;
ret = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, );
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index 90c9d07c1a66..719f46b51628 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -75,7 +75,6 @@ xics_ics_simple_eoi(int nr) "ics_eoi: irq 0x%x"
# s390_flic_kvm.c
flic_create_device(int err) "flic: create device failed %d"
-flic_no_device_api(int err) "flic: no Device Contral API support %d"
flic_reset_failed(int err) "flic: reset failed %d"
# s390_flic.c
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index cea71ac7c3dd..97a662ad0ebf 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -316,6 +316,13 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
MachineClass *mc = MACHINE_GET_CLASS(ms);
mc->default_cpu_type = S390_CPU_TYPE_NAME("host");
+
+if (!kvm_check_extension(kvm_state, KVM_CAP_DEVICE_CTRL)) {
+error_report("KVM is missing capability KVM_CAP_DEVICE_CTRL - "
+ "please use kernel 3.15 or newer");
+return -1;
+}
+
cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
--
2.21.0
[PULL 5/5] s390x/cpumodel: Add the z15 name to the description of gen15a
We now know that gen15a is called z15.
Reviewed-by: David Hildenbrand
Signed-off-by: Christian Borntraeger
---
target/s390x/cpu_models.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 1d16d7d5e794..009afc38b92d 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -84,7 +84,7 @@ static S390CPUDef s390_cpu_defs[] = {
CPUDEF_INIT(0x3906, 14, 1, 47, 0x0800U, "z14", "IBM z14 GA1"),
CPUDEF_INIT(0x3906, 14, 2, 47, 0x0800U, "z14.2", "IBM z14 GA2"),
CPUDEF_INIT(0x3907, 14, 1, 47, 0x0800U, "z14ZR1", "IBM z14 Model ZR1
GA1"),
-CPUDEF_INIT(0x8561, 15, 1, 47, 0x0800U, "gen15a", "IBM 8561 GA1"),
+CPUDEF_INIT(0x8561, 15, 1, 47, 0x0800U, "gen15a", "IBM z15 GA1"),
CPUDEF_INIT(0x8562, 15, 1, 47, 0x0800U, "gen15b", "IBM 8562 GA1"),
};
--
2.21.0
[PULL 2/5] pc-bios/s390-ccw/net: fix a possible memory leak in get_uuid()
From: Yifan Luo
There is a possible memory leak in get_uuid(). Should free allocated mem
before
return NULL.
Signed-off-by: Yifan Luo
Message-Id: <02cf01d55267$86cf2850$946d78f0$@cmss.chinamobile.com>
Reviewed-by: Thomas Huth
Reviewed-by: Cornelia Huck
Signed-off-by: Thomas Huth
---
pc-bios/s390-ccw/netmain.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index f3542cb2cf11..f2dcc01e2725 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -269,6 +269,7 @@ static const char *get_uuid(void)
: "d" (r0), "d" (r1), [addr] "a" (buf)
: "cc", "memory");
if (cc) {
+free(mem);
return NULL;
}
--
2.21.0
[PULL 0/5] s390x update
Peter, here is the non-tcg subset of the s390x updates. The following changes since commit 4300b7c2cd9f3f273804e8cca325842ccb93b1ad: Merge remote-tracking branch 'remotes/cleber/tags/python-next-pull-request' into staging (2019-09-20 17:28:43 +0100) are available in the Git repository at: git://github.com/borntraeger/qemu.git tags/s390x-20190923 for you to fetch changes up to 7505deca0bfa859136ec6419dbafc504f22fcac2: s390x/cpumodel: Add the z15 name to the description of gen15a (2019-09-23 09:15:28 +0200) - bugfixes in ccw bios - gen15a is called z15 - officially require a 3.15 kernel or later for kvm Christian Borntraeger (2): Merge tag 's390-ccw-bios-2019-09-18' of https://gitlab.com/huth/qemu into s390-next s390x/cpumodel: Add the z15 name to the description of gen15a Thomas Huth (3): pc-bios/s390-ccw: Do not pre-initialize empty array pc-bios/s390-ccw: Rebuild the s390-netboot.img firmware image s390x/kvm: Officially require at least kernel 3.15 Yifan Luo (1): pc-bios/s390-ccw/net: fix a possible memory leak in get_uuid() hw/intc/s390_flic_kvm.c| 6 -- hw/intc/trace-events | 1 - pc-bios/s390-ccw/main.c| 2 +- pc-bios/s390-ccw/netmain.c | 1 + pc-bios/s390-netboot.img | Bin 67232 -> 67232 bytes target/s390x/cpu_models.c | 2 +- target/s390x/kvm.c | 7 +++ 7 files changed, 10 insertions(+), 9 deletions(-)
[PULL 1/5] pc-bios/s390-ccw: Do not pre-initialize empty array
From: Thomas Huth
Since commit 339686a358b11a231aa5b6d1424e7a1460d7f277 ("pc-bios/s390-ccw:
zero out bss section"), we are clearing now the BSS in start.S, so there
is no need to pre-initialize the loadparm_str array with zeroes anymore.
Reviewed-by: Cornelia Huck
Signed-off-by: Thomas Huth
---
pc-bios/s390-ccw/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index a69c73349e8f..a21b38628075 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -17,7 +17,7 @@
char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
static SubChannelId blk_schid = { .one = 1 };
-static char loadparm_str[LOADPARM_LEN + 1] = { 0, 0, 0, 0, 0, 0, 0, 0, 0 };
+static char loadparm_str[LOADPARM_LEN + 1];
QemuIplParameters qipl;
IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
static bool have_iplb;
--
2.21.0
[Qemu-devel] [PATCH 1/1] s390x/cpumodel: Add the z15 name to the description of gen15a
We now know that gen15a is called z15.
Signed-off-by: Christian Borntraeger
---
target/s390x/cpu_models.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 1d16d7d5e794..009afc38b92d 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -84,7 +84,7 @@ static S390CPUDef s390_cpu_defs[] = {
CPUDEF_INIT(0x3906, 14, 1, 47, 0x0800U, "z14", "IBM z14 GA1"),
CPUDEF_INIT(0x3906, 14, 2, 47, 0x0800U, "z14.2", "IBM z14 GA2"),
CPUDEF_INIT(0x3907, 14, 1, 47, 0x0800U, "z14ZR1", "IBM z14 Model ZR1
GA1"),
-CPUDEF_INIT(0x8561, 15, 1, 47, 0x0800U, "gen15a", "IBM 8561 GA1"),
+CPUDEF_INIT(0x8561, 15, 1, 47, 0x0800U, "gen15a", "IBM z15 GA1"),
CPUDEF_INIT(0x8562, 15, 1, 47, 0x0800U, "gen15b", "IBM 8562 GA1"),
};
--
2.21.0
Re: [Qemu-devel] [qemu-s390x] [PATCH for-4.2 v5 1/2] kvm: s390: split too big memory section on several memslots
On 02.09.19 15:49, Igor Mammedov wrote:
> On Fri, 30 Aug 2019 18:19:29 +0200
> Christian Borntraeger wrote:
>
>> On 30.08.19 11:41, Igor Mammedov wrote:
>>> On Thu, 29 Aug 2019 14:41:13 +0200
>>> Christian Borntraeger wrote:
>>>
>>>> On 29.08.19 14:31, Igor Mammedov wrote:
>>>>> On Thu, 29 Aug 2019 14:07:44 +0200
>>>>> Christian Borntraeger wrote:
>>>>>
>>>>>> On 29.08.19 14:04, Igor Mammedov wrote:
>>>>>>> On Thu, 29 Aug 2019 08:47:49 +0200
>>>>>>> Christian Borntraeger wrote:
>>>>>>>
>>>>>>>> On 27.08.19 14:56, Igor Mammedov wrote:
>>>>>>>>> On Tue, 20 Aug 2019 18:07:27 +0200
>>>>>>>>> Cornelia Huck wrote:
>>>>>>>>>
>>>>>>>>>> On Wed, 7 Aug 2019 11:32:41 -0400
>>>>>>>>>> Igor Mammedov wrote:
>>>>>>>>>>
>>>>>>>>>>> Max memslot size supported by kvm on s390 is 8Tb,
>>>>>>>>>>> move logic of splitting RAM in chunks upto 8T to KVM code.
>>>>>>>>>>>
>>>>>>>>>>> This way it will hide KVM specific restrictions in KVM code
>>>>>>>>>>> and won't affect baord level design decisions. Which would allow
>>>>>>>>>>> us to avoid misusing memory_region_allocate_system_memory() API
>>>>>>>>>>> and eventually use a single hostmem backend for guest RAM.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Igor Mammedov
>>>>>>>>>>> ---
>>>>>>>>>>> v5:
>>>>>>>>>>> * move computation 'size -= slot_size' inside of loop body
>>>>>>>>>>> (David Hildenbrand )
>>>>>>>>>>> v4:
>>>>>>>>>>> * fix compilation issue
>>>>>>>>>>> (Christian Borntraeger )
>>>>>>>>>>> * advance HVA along with GPA in kvm_set_phys_mem()
>>>>>>>>>>> (Christian Borntraeger )
>>>>>>>>>>>
>>>>>>>>>>> patch prepares only KVM side for switching to single RAM memory
>>>>>>>>>>> region
>>>>>>>>>>> another patch will take care of dropping manual RAM partitioning in
>>>>>>>>>>> s390 code.
>>>>>>>>>>
>>>>>>>>>> I may have lost track a bit -- what is the status of this patch (and
>>>>>>>>>> the series)?
>>>>>>>>>
>>>>>>>>> Christian,
>>>>>>>>>
>>>>>>>>> could you test it on a host that have sufficient amount of RAM?
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> This version looks good. I was able to start a 9TB guest.
>>>>>>>> [pid 215723] ioctl(10, KVM_SET_USER_MEMORY_REGION, {slot=0, flags=0,
>>>>>>>> guest_phys_addr=0, memory_size=8796091973632,
>>>>>>>> userspace_addr=0x3ffee70}) = 0
>>>>>>>> [pid 215723] ioctl(10, KVM_SET_USER_MEMORY_REGION, {slot=1, flags=0,
>>>>>>>> guest_phys_addr=0x7f0, memory_size=1099512676352,
>>>>>>>> userspace_addr=0xbffee60}) = 0
>>>>>>
>>>>>>>> The only question is if we want to fix the weird alignment
>>>>>>>> (0x7f0) when
>>>>>>>> we already add a migration barrier for uber-large guests.
>>>>>>>> Maybe we could split at 4TB to avoid future problem with larger page
>>>>>>>> sizes?
>>>>>>> That probably should be a separate patch on top.
>>>>>>
>>>>>> Right. The split in KVM code is transparent to migration and other parts
>>>>>> of QEMU, correct?
>>>>>
>>>>> it should not affect other QEMU parts and migration (to my limited
>>>>> understanding of it),
>>>>> we are passing to KVM memory slots upto KVM_SLOT_MAX_BYTES as we were
>>>>> doing before by
>>>>> creating several memory regions instead of one as described in [2/2]
>>>>> commit message.
>>>>>
>>>>> Also could you also test migration of +9Tb guest, to check that nothing
>>>>> where broken by
>>>>> accident in QEMU migration code?
>>>>
>>>> I only have one server that is large enough :-/
>>> Could you test offline migration on it (to a file and restore from it)?
>>
>> I tested migration with a hacked QEMU (basically split in KVM code at 1GB
>> instead of 8TB) and
>> the restore from file failed with data corruption in the guest. The current
>> code
>> does work when I use small memslots. No idea yet what is wrong.
>
> I've tested 2Gb (max, I can test) guest (also hacked up version)
> and it worked for me.
> How do you test it and detect corruption so I could try to reproduce it
> locally?
> (given it worked before, there is no much hope but I could try)
I basically started a guest with just kernel and ramdisk on the command line and
then in the monitor I did
migrate "exec: cat > savefile"
and then I restarted the guest with
-incoming "exec: cat savefile"
the guest then very quickly crashed with random kernel oopses.
Using libvirts managedsave should work as well.
Re: [Qemu-devel] [qemu-s390x] [PATCH for-4.2 v5 1/2] kvm: s390: split too big memory section on several memslots
On 30.08.19 11:41, Igor Mammedov wrote:
> On Thu, 29 Aug 2019 14:41:13 +0200
> Christian Borntraeger wrote:
>
>> On 29.08.19 14:31, Igor Mammedov wrote:
>>> On Thu, 29 Aug 2019 14:07:44 +0200
>>> Christian Borntraeger wrote:
>>>
>>>> On 29.08.19 14:04, Igor Mammedov wrote:
>>>>> On Thu, 29 Aug 2019 08:47:49 +0200
>>>>> Christian Borntraeger wrote:
>>>>>
>>>>>> On 27.08.19 14:56, Igor Mammedov wrote:
>>>>>>> On Tue, 20 Aug 2019 18:07:27 +0200
>>>>>>> Cornelia Huck wrote:
>>>>>>>
>>>>>>>> On Wed, 7 Aug 2019 11:32:41 -0400
>>>>>>>> Igor Mammedov wrote:
>>>>>>>>
>>>>>>>>> Max memslot size supported by kvm on s390 is 8Tb,
>>>>>>>>> move logic of splitting RAM in chunks upto 8T to KVM code.
>>>>>>>>>
>>>>>>>>> This way it will hide KVM specific restrictions in KVM code
>>>>>>>>> and won't affect baord level design decisions. Which would allow
>>>>>>>>> us to avoid misusing memory_region_allocate_system_memory() API
>>>>>>>>> and eventually use a single hostmem backend for guest RAM.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Igor Mammedov
>>>>>>>>> ---
>>>>>>>>> v5:
>>>>>>>>> * move computation 'size -= slot_size' inside of loop body
>>>>>>>>> (David Hildenbrand )
>>>>>>>>> v4:
>>>>>>>>> * fix compilation issue
>>>>>>>>> (Christian Borntraeger )
>>>>>>>>> * advance HVA along with GPA in kvm_set_phys_mem()
>>>>>>>>> (Christian Borntraeger )
>>>>>>>>>
>>>>>>>>> patch prepares only KVM side for switching to single RAM memory region
>>>>>>>>> another patch will take care of dropping manual RAM partitioning in
>>>>>>>>> s390 code.
>>>>>>>>
>>>>>>>> I may have lost track a bit -- what is the status of this patch (and
>>>>>>>> the series)?
>>>>>>>
>>>>>>> Christian,
>>>>>>>
>>>>>>> could you test it on a host that have sufficient amount of RAM?
>>>>>>
>>>>>>
>>>>>> This version looks good. I was able to start a 9TB guest.
>>>>>> [pid 215723] ioctl(10, KVM_SET_USER_MEMORY_REGION, {slot=0, flags=0,
>>>>>> guest_phys_addr=0, memory_size=8796091973632,
>>>>>> userspace_addr=0x3ffee70}) = 0
>>>>>> [pid 215723] ioctl(10, KVM_SET_USER_MEMORY_REGION, {slot=1, flags=0,
>>>>>> guest_phys_addr=0x7f0, memory_size=1099512676352,
>>>>>> userspace_addr=0xbffee60}) = 0
>>>>
>>>>>> The only question is if we want to fix the weird alignment
>>>>>> (0x7f0) when
>>>>>> we already add a migration barrier for uber-large guests.
>>>>>> Maybe we could split at 4TB to avoid future problem with larger page
>>>>>> sizes?
>>>>> That probably should be a separate patch on top.
>>>>
>>>> Right. The split in KVM code is transparent to migration and other parts
>>>> of QEMU, correct?
>>>
>>> it should not affect other QEMU parts and migration (to my limited
>>> understanding of it),
>>> we are passing to KVM memory slots upto KVM_SLOT_MAX_BYTES as we were doing
>>> before by
>>> creating several memory regions instead of one as described in [2/2] commit
>>> message.
>>>
>>> Also could you also test migration of +9Tb guest, to check that nothing
>>> where broken by
>>> accident in QEMU migration code?
>>
>> I only have one server that is large enough :-/
> Could you test offline migration on it (to a file and restore from it)?
I tested migration with a hacked QEMU (basically split in KVM code at 1GB
instead of 8TB) and
the restore from file failed with data corruption in the guest. The current code
does work when I use small memslots. No idea yet what is wrong.
Re: [Qemu-devel] [qemu-s390x] [PATCH for-4.2 v5 1/2] kvm: s390: split too big memory section on several memslots
On 29.08.19 14:31, Igor Mammedov wrote:
> On Thu, 29 Aug 2019 14:07:44 +0200
> Christian Borntraeger wrote:
>
>> On 29.08.19 14:04, Igor Mammedov wrote:
>>> On Thu, 29 Aug 2019 08:47:49 +0200
>>> Christian Borntraeger wrote:
>>>
>>>> On 27.08.19 14:56, Igor Mammedov wrote:
>>>>> On Tue, 20 Aug 2019 18:07:27 +0200
>>>>> Cornelia Huck wrote:
>>>>>
>>>>>> On Wed, 7 Aug 2019 11:32:41 -0400
>>>>>> Igor Mammedov wrote:
>>>>>>
>>>>>>> Max memslot size supported by kvm on s390 is 8Tb,
>>>>>>> move logic of splitting RAM in chunks upto 8T to KVM code.
>>>>>>>
>>>>>>> This way it will hide KVM specific restrictions in KVM code
>>>>>>> and won't affect baord level design decisions. Which would allow
>>>>>>> us to avoid misusing memory_region_allocate_system_memory() API
>>>>>>> and eventually use a single hostmem backend for guest RAM.
>>>>>>>
>>>>>>> Signed-off-by: Igor Mammedov
>>>>>>> ---
>>>>>>> v5:
>>>>>>> * move computation 'size -= slot_size' inside of loop body
>>>>>>> (David Hildenbrand )
>>>>>>> v4:
>>>>>>> * fix compilation issue
>>>>>>> (Christian Borntraeger )
>>>>>>> * advance HVA along with GPA in kvm_set_phys_mem()
>>>>>>> (Christian Borntraeger )
>>>>>>>
>>>>>>> patch prepares only KVM side for switching to single RAM memory region
>>>>>>> another patch will take care of dropping manual RAM partitioning in
>>>>>>> s390 code.
>>>>>>
>>>>>> I may have lost track a bit -- what is the status of this patch (and
>>>>>> the series)?
>>>>>
>>>>> Christian,
>>>>>
>>>>> could you test it on a host that have sufficient amount of RAM?
>>>>
>>>>
>>>> This version looks good. I was able to start a 9TB guest.
>>>> [pid 215723] ioctl(10, KVM_SET_USER_MEMORY_REGION, {slot=0, flags=0,
>>>> guest_phys_addr=0, memory_size=8796091973632,
>>>> userspace_addr=0x3ffee70}) = 0
>>>> [pid 215723] ioctl(10, KVM_SET_USER_MEMORY_REGION, {slot=1, flags=0,
>>>> guest_phys_addr=0x7f0, memory_size=1099512676352,
>>>> userspace_addr=0xbffee60}) = 0
>>
>>>> The only question is if we want to fix the weird alignment (0x7f0)
>>>> when
>>>> we already add a migration barrier for uber-large guests.
>>>> Maybe we could split at 4TB to avoid future problem with larger page
>>>> sizes?
>>> That probably should be a separate patch on top.
>>
>> Right. The split in KVM code is transparent to migration and other parts of
>> QEMU, correct?
>
> it should not affect other QEMU parts and migration (to my limited
> understanding of it),
> we are passing to KVM memory slots upto KVM_SLOT_MAX_BYTES as we were doing
> before by
> creating several memory regions instead of one as described in [2/2] commit
> message.
>
> Also could you also test migration of +9Tb guest, to check that nothing where
> broken by
> accident in QEMU migration code?
I only have one server that is large enough :-/
Re: [Qemu-devel] [qemu-s390x] [PATCH for-4.2 v5 1/2] kvm: s390: split too big memory section on several memslots
On 29.08.19 14:04, Igor Mammedov wrote:
> On Thu, 29 Aug 2019 08:47:49 +0200
> Christian Borntraeger wrote:
>
>> On 27.08.19 14:56, Igor Mammedov wrote:
>>> On Tue, 20 Aug 2019 18:07:27 +0200
>>> Cornelia Huck wrote:
>>>
>>>> On Wed, 7 Aug 2019 11:32:41 -0400
>>>> Igor Mammedov wrote:
>>>>
>>>>> Max memslot size supported by kvm on s390 is 8Tb,
>>>>> move logic of splitting RAM in chunks upto 8T to KVM code.
>>>>>
>>>>> This way it will hide KVM specific restrictions in KVM code
>>>>> and won't affect baord level design decisions. Which would allow
>>>>> us to avoid misusing memory_region_allocate_system_memory() API
>>>>> and eventually use a single hostmem backend for guest RAM.
>>>>>
>>>>> Signed-off-by: Igor Mammedov
>>>>> ---
>>>>> v5:
>>>>> * move computation 'size -= slot_size' inside of loop body
>>>>> (David Hildenbrand )
>>>>> v4:
>>>>> * fix compilation issue
>>>>> (Christian Borntraeger )
>>>>> * advance HVA along with GPA in kvm_set_phys_mem()
>>>>> (Christian Borntraeger )
>>>>>
>>>>> patch prepares only KVM side for switching to single RAM memory region
>>>>> another patch will take care of dropping manual RAM partitioning in
>>>>> s390 code.
>>>>
>>>> I may have lost track a bit -- what is the status of this patch (and
>>>> the series)?
>>>
>>> Christian,
>>>
>>> could you test it on a host that have sufficient amount of RAM?
>>
>>
>> This version looks good. I was able to start a 9TB guest.
>> [pid 215723] ioctl(10, KVM_SET_USER_MEMORY_REGION, {slot=0, flags=0,
>> guest_phys_addr=0, memory_size=8796091973632, userspace_addr=0x3ffee70})
>> = 0
>> [pid 215723] ioctl(10, KVM_SET_USER_MEMORY_REGION, {slot=1, flags=0,
>> guest_phys_addr=0x7f0, memory_size=1099512676352,
>> userspace_addr=0xbffee60}) = 0
>>
>> The only question is if we want to fix the weird alignment (0x7f0)
>> when
>> we already add a migration barrier for uber-large guests.
>> Maybe we could split at 4TB to avoid future problem with larger page sizes?
> That probably should be a separate patch on top.
Right. The split in KVM code is transparent to migration and other parts of
QEMU, correct?
Re: [Qemu-devel] [PATCH for-4.2 v5 1/2] kvm: s390: split too big memory section on several memslots
On 27.08.19 14:56, Igor Mammedov wrote:
> On Tue, 20 Aug 2019 18:07:27 +0200
> Cornelia Huck wrote:
>
>> On Wed, 7 Aug 2019 11:32:41 -0400
>> Igor Mammedov wrote:
>>
>>> Max memslot size supported by kvm on s390 is 8Tb,
>>> move logic of splitting RAM in chunks upto 8T to KVM code.
>>>
>>> This way it will hide KVM specific restrictions in KVM code
>>> and won't affect baord level design decisions. Which would allow
>>> us to avoid misusing memory_region_allocate_system_memory() API
>>> and eventually use a single hostmem backend for guest RAM.
>>>
>>> Signed-off-by: Igor Mammedov
>>> ---
>>> v5:
>>> * move computation 'size -= slot_size' inside of loop body
>>> (David Hildenbrand )
>>> v4:
>>> * fix compilation issue
>>> (Christian Borntraeger )
>>> * advance HVA along with GPA in kvm_set_phys_mem()
>>> (Christian Borntraeger )
>>>
>>> patch prepares only KVM side for switching to single RAM memory region
>>> another patch will take care of dropping manual RAM partitioning in
>>> s390 code.
>>
>> I may have lost track a bit -- what is the status of this patch (and
>> the series)?
>
> Christian,
>
> could you test it on a host that have sufficient amount of RAM?
This version looks good. I was able to start a 9TB guest.
[pid 215723] ioctl(10, KVM_SET_USER_MEMORY_REGION, {slot=0, flags=0,
guest_phys_addr=0, memory_size=8796091973632, userspace_addr=0x3ffee70}) = 0
[pid 215723] ioctl(10, KVM_SET_USER_MEMORY_REGION, {slot=1, flags=0,
guest_phys_addr=0x7f0, memory_size=1099512676352,
userspace_addr=0xbffee60}) = 0
The only question is if we want to fix the weird alignment (0x7f0) when
we already add a migration barrier for uber-large guests.
Maybe we could split at 4TB to avoid future problem with larger page sizes?
Re: [Qemu-devel] [qemu-s390x] [PATCH] pc-bios/s390-ccw: Do not pre-initialize empty array
On 28.08.19 15:47, Cornelia Huck wrote:
> On Wed, 28 Aug 2019 15:42:37 +0200
> Thomas Huth wrote:
>
>> On 28/08/2019 15.27, Christian Borntraeger wrote:
>>> On 28.08.19 14:33, Thomas Huth wrote:
>>>> We're clearing the BSS in start.S now, so there is no need to
>>>> pre-initialize the loadparm_str array with zeroes anymore.
>>>
>>> Can you add a link to the commit that does the bss clearing?
>>
>> Sure, I'll change the description to:
>>
>> "
>> Since commit 339686a358b11a231aa5b6d1424e7a1460d7f277 ("pc-bios/s390-ccw:
>> zero out bss section"), we are clearing now the BSS in start.S, so there
>> is no need to pre-initialize the loadparm_str array with zeroes anymore.
>> "
>>
>> Thomas
>
> With that:
>
> Reviewed-by: Cornelia Huck
Acked-by: Christian Borntraeger
Thomas are you going to pick this up or shall I do it?
Re: [Qemu-devel] [PATCH] pc-bios/s390-ccw: Do not pre-initialize empty array
On 28.08.19 14:33, Thomas Huth wrote:
> We're clearing the BSS in start.S now, so there is no need to
> pre-initialize the loadparm_str array with zeroes anymore.
Can you add a link to the commit that does the bss clearing?
I think it was
commit 339686a358b11a231aa5b6d1424e7a1460d7f277
Author: Christian Borntraeger
AuthorDate: Wed Nov 22 15:26:27 2017 +0100
Commit: Cornelia Huck
CommitDate: Thu Dec 14 17:56:54 2017 +0100
pc-bios/s390-ccw: zero out bss section
>
> Signed-off-by: Thomas Huth
> ---
> pc-bios/s390-ccw/main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index a69c73349e..a21b386280 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -17,7 +17,7 @@
>
> char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
> static SubChannelId blk_schid = { .one = 1 };
> -static char loadparm_str[LOADPARM_LEN + 1] = { 0, 0, 0, 0, 0, 0, 0, 0, 0 };
> +static char loadparm_str[LOADPARM_LEN + 1];
> QemuIplParameters qipl;
> IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
> static bool have_iplb;
>
Re: [Qemu-devel] [PATCH for-4.2 v3 0/2] s390: stop abusing memory_region_allocate_system_memory()
On 02.08.19 16:59, Christian Borntraeger wrote:
>
>
> On 02.08.19 16:42, Christian Borntraeger wrote:
>> On 02.08.19 15:32, Igor Mammedov wrote:
>>> Changelog:
>>> since v2:
>>> - break migration from old QEMU (since 2.12-4.1) for guest with >8TB RAM
>>> and drop migratable aliases patch as was agreed during v2 review
>>> - drop 4.2 machines patch as it's not prerequisite anymore
>>> since v1:
>>> - include 4.2 machines patch for adding compat RAM layout on top
>>> - 2/4 add missing in v1 patch for splitting too big MemorySection on
>>> several memslots
>>> - 3/4 amend code path on alias destruction to ensure that RAMBlock is
>>> cleaned properly
>>> - 4/4 add compat machine code to keep old layout (migration-wise) for
>>> 4.1 and older machines
>>>
>>>
>>> While looking into unifying guest RAM allocation to use hostmem backends
>>> for initial RAM (especially when -mempath is used) and retiring
>>> memory_region_allocate_system_memory() API, leaving only single hostmem
>>> backend,
>>> I was inspecting how currently it is used by boards and it turns out several
>>> boards abuse it by calling the function several times (despite documented
>>> contract
>>> forbiding it).
>>>
>>> s390 is one of such boards where KVM limitation on memslot size got
>>> propagated
>>> to board design and memory_region_allocate_system_memory() was abused to
>>> satisfy
>>> KVM requirement for max RAM chunk where memory region alias would suffice.
>>>
>>> Unfortunately, memory_region_allocate_system_memory() usage created
>>> migration
>>> dependency where guest RAM is transferred in migration stream as several
>>> RAMBlocks
>>> if it's more than KVM_SLOT_MAX_BYTES. During v2 review it was agreed to
>>> ignore
>>> migration breakage (documenting it in release notes) and leaving only KVM
>>> fix.
>>>
>>> In order to replace these several RAM chunks with a single memdev and keep
>>> it
>>> working with KVM memslot size limit, following was done:
>>>* [1/2] split too big RAM chunk inside of KVM code on several memory
>>> slots
>>>if necessary
>>>* [2/2] drop manual ram splitting in s390 code
>>>
>>>
>>> CC: pbonz...@redhat.com
>>> CC: qemu-s3...@nongnu.org
>>> CC: borntrae...@de.ibm.com
>>> CC: th...@redhat.com
>>> CC: da...@redhat.com
>>> CC: coh...@redhat.com
>>
>> With the fixup this patch set seems to work on s390. I can start 9TB guests
>> and
>> I can migrate smaller guests between 4.1+patch and 4.0 and 3.1. I currently
>> can
>> not test migration for the 9TB guest due to lack of a 2nd system.
>
> I have to correct myself. The 9TB guest started up but it does not seem to do
> anything useful (it hangs).
Seems that the userspace addr is wrong (its the same).
[pid 258234] ioctl(10, KVM_SET_USER_MEMORY_REGION, {slot=0, flags=0,
guest_phys_addr=0, memory_size=8796091973632, userspace_addr=0x3fff7d0}) = 0
[pid 258234] ioctl(10, KVM_SET_USER_MEMORY_REGION, {slot=1, flags=0,
guest_phys_addr=0x7f0, memory_size=1099512676352,
userspace_addr=0x3fff7d0}) = 0
Re: [Qemu-devel] [PATCH for-4.2 v3 0/2] s390: stop abusing memory_region_allocate_system_memory()
On 02.08.19 16:42, Christian Borntraeger wrote: > On 02.08.19 15:32, Igor Mammedov wrote: >> Changelog: >> since v2: >> - break migration from old QEMU (since 2.12-4.1) for guest with >8TB RAM >> and drop migratable aliases patch as was agreed during v2 review >> - drop 4.2 machines patch as it's not prerequisite anymore >> since v1: >> - include 4.2 machines patch for adding compat RAM layout on top >> - 2/4 add missing in v1 patch for splitting too big MemorySection on >> several memslots >> - 3/4 amend code path on alias destruction to ensure that RAMBlock is >> cleaned properly >> - 4/4 add compat machine code to keep old layout (migration-wise) for >> 4.1 and older machines >> >> >> While looking into unifying guest RAM allocation to use hostmem backends >> for initial RAM (especially when -mempath is used) and retiring >> memory_region_allocate_system_memory() API, leaving only single hostmem >> backend, >> I was inspecting how currently it is used by boards and it turns out several >> boards abuse it by calling the function several times (despite documented >> contract >> forbiding it). >> >> s390 is one of such boards where KVM limitation on memslot size got >> propagated >> to board design and memory_region_allocate_system_memory() was abused to >> satisfy >> KVM requirement for max RAM chunk where memory region alias would suffice. >> >> Unfortunately, memory_region_allocate_system_memory() usage created migration >> dependency where guest RAM is transferred in migration stream as several >> RAMBlocks >> if it's more than KVM_SLOT_MAX_BYTES. During v2 review it was agreed to >> ignore >> migration breakage (documenting it in release notes) and leaving only KVM >> fix. >> >> In order to replace these several RAM chunks with a single memdev and keep it >> working with KVM memslot size limit, following was done: >>* [1/2] split too big RAM chunk inside of KVM code on several memory slots >>if necessary >>* [2/2] drop manual ram splitting in s390 code >> >> >> CC: pbonz...@redhat.com >> CC: qemu-s3...@nongnu.org >> CC: borntrae...@de.ibm.com >> CC: th...@redhat.com >> CC: da...@redhat.com >> CC: coh...@redhat.com > > With the fixup this patch set seems to work on s390. I can start 9TB guests > and > I can migrate smaller guests between 4.1+patch and 4.0 and 3.1. I currently > can > not test migration for the 9TB guest due to lack of a 2nd system. I have to correct myself. The 9TB guest started up but it does not seem to do anything useful (it hangs). I will try to investigate. >> >> Igor Mammedov (2): >> kvm: s390: split too big memory section on several memslots >> s390: do not call memory_region_allocate_system_memory() multiple >> times >> >> include/hw/s390x/s390-virtio-ccw.h | 10 >> include/sysemu/kvm_int.h | 1 + >> accel/kvm/kvm-all.c| 79 ++ >> hw/s390x/s390-virtio-ccw.c | 30 ++-- >> target/s390x/kvm.c | 1 + >> 5 files changed, 63 insertions(+), 58 deletions(-) >>
Re: [Qemu-devel] [PATCH for-4.2 v3 0/2] s390: stop abusing memory_region_allocate_system_memory()
On 02.08.19 15:32, Igor Mammedov wrote: > Changelog: > since v2: > - break migration from old QEMU (since 2.12-4.1) for guest with >8TB RAM > and drop migratable aliases patch as was agreed during v2 review > - drop 4.2 machines patch as it's not prerequisite anymore > since v1: > - include 4.2 machines patch for adding compat RAM layout on top > - 2/4 add missing in v1 patch for splitting too big MemorySection on > several memslots > - 3/4 amend code path on alias destruction to ensure that RAMBlock is > cleaned properly > - 4/4 add compat machine code to keep old layout (migration-wise) for > 4.1 and older machines > > > While looking into unifying guest RAM allocation to use hostmem backends > for initial RAM (especially when -mempath is used) and retiring > memory_region_allocate_system_memory() API, leaving only single hostmem > backend, > I was inspecting how currently it is used by boards and it turns out several > boards abuse it by calling the function several times (despite documented > contract > forbiding it). > > s390 is one of such boards where KVM limitation on memslot size got propagated > to board design and memory_region_allocate_system_memory() was abused to > satisfy > KVM requirement for max RAM chunk where memory region alias would suffice. > > Unfortunately, memory_region_allocate_system_memory() usage created migration > dependency where guest RAM is transferred in migration stream as several > RAMBlocks > if it's more than KVM_SLOT_MAX_BYTES. During v2 review it was agreed to ignore > migration breakage (documenting it in release notes) and leaving only KVM fix. > > In order to replace these several RAM chunks with a single memdev and keep it > working with KVM memslot size limit, following was done: >* [1/2] split too big RAM chunk inside of KVM code on several memory slots >if necessary >* [2/2] drop manual ram splitting in s390 code > > > CC: pbonz...@redhat.com > CC: qemu-s3...@nongnu.org > CC: borntrae...@de.ibm.com > CC: th...@redhat.com > CC: da...@redhat.com > CC: coh...@redhat.com With the fixup this patch set seems to work on s390. I can start 9TB guests and I can migrate smaller guests between 4.1+patch and 4.0 and 3.1. I currently can not test migration for the 9TB guest due to lack of a 2nd system. > > Igor Mammedov (2): > kvm: s390: split too big memory section on several memslots > s390: do not call memory_region_allocate_system_memory() multiple > times > > include/hw/s390x/s390-virtio-ccw.h | 10 > include/sysemu/kvm_int.h | 1 + > accel/kvm/kvm-all.c| 79 ++ > hw/s390x/s390-virtio-ccw.c | 30 ++-- > target/s390x/kvm.c | 1 + > 5 files changed, 63 insertions(+), 58 deletions(-) >
Re: [Qemu-devel] [PATCH for-4.2 v3 1/2] kvm: s390: split too big memory section on several memslots
On 02.08.19 15:32, Igor Mammedov wrote:
> Max memslot size supported by kvm on s390 is 8Tb,
> move logic of splitting RAM in chunks upto 8T to KVM code.
>
> This way it will hide KVM specific restrictions in KVM code
> and won't affect baord level design decisions. Which would allow
> us to avoid misusing memory_region_allocate_system_memory() API
> and eventually use a single hostmem backend for guest RAM.
>
> Signed-off-by: Igor Mammedov
> ---
> patch prepares only KVM side for switching to single RAM memory region
> another patch will take care of dropping manual RAM partitioning in
> s390 code.
> ---
/home/cborntra/REPOS/qemu/target/s390x/kvm.c: In function ‘kvm_arch_init’:
/home/cborntra/REPOS/qemu/target/s390x/kvm.c:350:5: error: implicit declaration
of function ‘kvm_set_max_memslot_size’; did you mean ‘kvm_get_max_memslots’?
[-Werror=implicit-function-declaration]
350 | kvm_set_max_memslot_size(KVM_SLOT_MAX_BYTES);
| ^~~~
| kvm_get_max_memslots
/home/cborntra/REPOS/qemu/target/s390x/kvm.c:350:5: error: nested extern
declaration of ‘kvm_set_max_memslot_size’ [-Werror=nested-externs]
cc1: all warnings being treated as errors
make[1]: *** [/home/cborntra/REPOS/qemu/rules.mak:69: target/s390x/kvm.o] Error
1
make: *** [Makefile:472: s390x-softmmu/all] Error 2
We probably need an include of sysemu/kvm_int.h in target/s390x/kvm.c
I will test with that fixup
> include/hw/s390x/s390-virtio-ccw.h | 10
> include/sysemu/kvm_int.h | 1 +
> accel/kvm/kvm-all.c| 79 ++
> hw/s390x/s390-virtio-ccw.c | 9
> target/s390x/kvm.c | 1 +
> 5 files changed, 60 insertions(+), 40 deletions(-)
>
> diff --git a/include/hw/s390x/s390-virtio-ccw.h
> b/include/hw/s390x/s390-virtio-ccw.h
> index 8aa27199c9..00632f94b4 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -21,6 +21,16 @@
> #define S390_MACHINE_CLASS(klass) \
> OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE)
>
> +/*
> + * KVM does only support memory slots up to KVM_MEM_MAX_NR_PAGES pages
> + * as the dirty bitmap must be managed by bitops that take an int as
> + * position indicator. If we have a guest beyond that we will split off
> + * new subregions. The split must happen on a segment boundary (1MB).
> + */
> +#define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1)
> +#define SEG_MSK (~0xfULL)
> +#define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) &
> SEG_MSK)
> +
> typedef struct S390CcwMachineState {
> /*< private >*/
> MachineState parent_obj;
> diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
> index 31df465fdc..7f7520bce2 100644
> --- a/include/sysemu/kvm_int.h
> +++ b/include/sysemu/kvm_int.h
> @@ -41,4 +41,5 @@ typedef struct KVMMemoryListener {
> void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,
>AddressSpace *as, int as_id);
>
> +void kvm_set_max_memslot_size(hwaddr max_slot_size);
> #endif
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index f450f25295..7a6efb9612 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -138,6 +138,7 @@ bool kvm_direct_msi_allowed;
> bool kvm_ioeventfd_any_length_allowed;
> bool kvm_msi_use_devid;
> static bool kvm_immediate_exit;
> +static hwaddr kvm_max_slot_size = ~0;
>
> static const KVMCapabilityInfo kvm_required_capabilites[] = {
> KVM_CAP_INFO(USER_MEMORY),
> @@ -951,6 +952,14 @@ kvm_check_extension_list(KVMState *s, const
> KVMCapabilityInfo *list)
> return NULL;
> }
>
> +void kvm_set_max_memslot_size(hwaddr max_slot_size)
> +{
> +g_assert(
> +ROUND_UP(max_slot_size, qemu_real_host_page_size) == max_slot_size
> +);
> +kvm_max_slot_size = max_slot_size;
> +}
> +
> static void kvm_set_phys_mem(KVMMemoryListener *kml,
> MemoryRegionSection *section, bool add)
> {
> @@ -958,7 +967,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
> int err;
> MemoryRegion *mr = section->mr;
> bool writeable = !mr->readonly && !mr->rom_device;
> -hwaddr start_addr, size;
> +hwaddr start_addr, size, slot_size;
> void *ram;
>
> if (!memory_region_is_ram(mr)) {
> @@ -983,41 +992,49 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
> kvm_slots_lock(kml);
>
> if (!add) {
> -mem = kvm_lookup_matching_slot(kml, start_addr, size);
> -if (!mem) {
> -goto out;
> -}
> -if (mem->flags & KVM_MEM_LOG_DIRTY_PAGES) {
> -kvm_physical_sync_dirty_bitmap(kml, section);
> -}
> +do {
> +slot_size = kvm_max_slot_size < size ? kvm_max_slot_size : size;
> +mem = kvm_lookup_matching_slot(kml, start_addr, slot_size);
> +if (!mem) {
> +goto out;
> +
Re: [Qemu-devel] [PATCH for-4.2 v3 2/2] s390: do not call memory_region_allocate_system_memory() multiple times
On 02.08.19 15:36, David Hildenbrand wrote:
> On 02.08.19 15:32, Igor Mammedov wrote:
>> s390 was trying to solve limited KVM memslot size issue by abusing
>> memory_region_allocate_system_memory(), which breaks API contract
>> where the function might be called only once.
>>
>> Beside an invalid use of API, the approach also introduced migration
>> issue, since RAM chunks for each KVM_SLOT_MAX_BYTES are transferred in
>> migration stream as separate RAMBlocks.
>>
>> After discussion [1], it was agreed to break migration from older
>> QEMU for guest with RAM >8Tb (as it was relatively new (since 2.12)
>> and considered to be not actually used downstream).
>> Migration should keep working for guests with less than 8TB and for
>> more than 8TB with QEMU 4.2 and newer binary.
>> In case user tries to migrate more than 8TB guest, between incompatible
>> QEMU versions, migration should fail gracefully due to non-exiting
>> RAMBlock ID or RAMBlock size mismatch.
>>
>> Taking in account above and that now KVM code is able to split too
>> big MemorySection into several memslots, stop abusing
>> memory_region_allocate_system_memory()
>> and use only one memory region for RAM.
>>
>> 1) [PATCH RFC v2 4/4] s390: do not call
>> memory_region_allocate_system_memory() multiple times
>>
>> Signed-off-by: Igor Mammedov
>> ---
>> v3:
>> - drop migration compat code
>>
>> PS:
>> I don't have access to a suitable system to test it.
>> ---
>> hw/s390x/s390-virtio-ccw.c | 21 +++--
>> 1 file changed, 3 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 0c03ffb7c7..e30058df38 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -154,27 +154,12 @@ static void virtio_ccw_register_hcalls(void)
>> static void s390_memory_init(ram_addr_t mem_size)
>> {
>> MemoryRegion *sysmem = get_system_memory();
>> -ram_addr_t chunk, offset = 0;
>> -unsigned int number = 0;
>> +MemoryRegion *ram = g_new(MemoryRegion, 1);
>> Error *local_err = NULL;
>> -gchar *name;
>>
>> /* allocate RAM for core */
>> -name = g_strdup_printf("s390.ram");
>> -while (mem_size) {
>> -MemoryRegion *ram = g_new(MemoryRegion, 1);
>> -uint64_t size = mem_size;
>> -
>> -/* KVM does not allow memslots >= 8 TB */
>> -chunk = MIN(size, KVM_SLOT_MAX_BYTES);
>> -memory_region_allocate_system_memory(ram, NULL, name, chunk);
>> -memory_region_add_subregion(sysmem, offset, ram);
>> -mem_size -= chunk;
>> -offset += chunk;
>> -g_free(name);
>> -name = g_strdup_printf("s390.ram.%u", ++number);
>> -}
>> -g_free(name);
>> +memory_region_allocate_system_memory(ram, NULL, "s390.ram", mem_size);
>> +memory_region_add_subregion(sysmem, 0, ram);
>>
>> /*
>> * Configure the maximum page size. As no memory devices were created
>>
>
> Reviewed-by: David Hildenbrand
>
> We have to remember putting it into the next release notes. (or do we
> even want to get this into v4.1 ?)
This is too invasive for 4.1
Re: [Qemu-devel] [PATCH RFC v2 4/4] s390: do not call memory_region_allocate_system_memory() multiple times
On 02.08.19 13:40, Igor Mammedov wrote:
> On Fri, 2 Aug 2019 12:27:50 +0200
> Christian Borntraeger wrote:
>
>> On 02.08.19 12:25, David Hildenbrand wrote:
>>> On 02.08.19 11:38, Igor Mammedov wrote:
>>>> s390 was trying to solve limited KVM memslot size issue by abusing
>>>> memory_region_allocate_system_memory(), which breaks API contract
>>>> where the function might be called only once.
>>>>
>>>> s390 should have used memory aliases to fragment inital memory into
>>>> smaller chunks to satisfy KVM's memslot limitation. But its a bit
>>>> late now, since allocated pieces are transfered in migration stream
>>>> separately, so it's not possible to just replace broken layout with
>>>> correct one. To workaround issue, MemoryRegion alases are made
>>>> migratable and this patch switches to use them to split big initial
>>>> RAM chunk into smaller pieces (KVM_SLOT_MAX_BYTES max) and registers
>>>> aliases for migration. That should keep migration compatible with
>>>> previous QEMU versions.
>>>>
>>>> New machine types (since 4.2) will use single memory region, which
>>>> will get transimitted in migration stream as a whole RAMBlock.
>>>>
>>>> Signed-off-by: Igor Mammedov
>>>> ---
>>>> I don't have access to a suitable system to test it, so I've simulated
>>>> it with smaller chunks on x84 host. Ping-pong migration between old
>>>> and new QEMU worked fine. KVM part should be fine as memslots
>>>> using mapped MemoryRegions (in this case it would be aliases) as
>>>> far as I know but is someone could test it on big enough host it
>>>> would be nice.
>>>> ---
>>>> include/hw/s390x/s390-virtio-ccw.h | 4 +++
>>>> hw/s390x/s390-virtio-ccw.c | 56 +-
>>>> 2 files changed, 43 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/include/hw/s390x/s390-virtio-ccw.h
>>>> b/include/hw/s390x/s390-virtio-ccw.h
>>>> index 00632f94b4..f9ed3737f8 100644
>>>> --- a/include/hw/s390x/s390-virtio-ccw.h
>>>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>>>> @@ -21,6 +21,9 @@
>>>> #define S390_MACHINE_CLASS(klass) \
>>>> OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass),
>>>> TYPE_S390_CCW_MACHINE)
>>>>
>>>> +#define S390_MACHINE_GET_CLASS(obj) \
>>>> +OBJECT_GET_CLASS(S390CcwMachineClass, (obj), TYPE_S390_CCW_MACHINE)
>>>> +
>>>> /*
>>>> * KVM does only support memory slots up to KVM_MEM_MAX_NR_PAGES pages
>>>> * as the dirty bitmap must be managed by bitops that take an int as
>>>> @@ -50,6 +53,7 @@ typedef struct S390CcwMachineClass {
>>>> bool cpu_model_allowed;
>>>> bool css_migration_enabled;
>>>> bool hpage_1m_allowed;
>>>> +bool split_ram_layout;
>>>> } S390CcwMachineClass;
>>>>
>>>> /* runtime-instrumentation allowed by the machine */
>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>> index 073672f9cb..9160c1ed0a 100644
>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>> @@ -151,28 +151,47 @@ static void virtio_ccw_register_hcalls(void)
>>>> virtio_ccw_hcall_early_printk);
>>>> }
>>>>
>>>> -static void s390_memory_init(ram_addr_t mem_size)
>>>> +static void s390_memory_init(MachineState *ms)
>>>> {
>>>> +S390CcwMachineClass *s390mc = S390_MACHINE_GET_CLASS(ms);
>>>> MemoryRegion *sysmem = get_system_memory();
>>>> -ram_addr_t chunk, offset = 0;
>>>> +MemoryRegion *ram = g_new(MemoryRegion, 1);
>>>> unsigned int number = 0;
>>>> Error *local_err = NULL;
>>>> -gchar *name;
>>>> +ram_addr_t mem_size = ms->ram_size;
>>>> +gchar *name = g_strdup_printf(s390mc->split_ram_layout ?
>>>> + "s390.whole.ram" : "s390.ram");
>>>>
>>>> /* allocate RAM for core */
>>>> -name = g_strdup_printf("s390.ram");
>>>> -while (mem_size) {
>>>> -MemoryRegion *ram = g_new(MemoryRegion, 1);
>>>> -uint64_t size = mem_size;
>>>> -
>>>>
Re: [Qemu-devel] [PATCH RFC v2 4/4] s390: do not call memory_region_allocate_system_memory() multiple times
On 02.08.19 12:25, David Hildenbrand wrote:
> On 02.08.19 11:38, Igor Mammedov wrote:
>> s390 was trying to solve limited KVM memslot size issue by abusing
>> memory_region_allocate_system_memory(), which breaks API contract
>> where the function might be called only once.
>>
>> s390 should have used memory aliases to fragment inital memory into
>> smaller chunks to satisfy KVM's memslot limitation. But its a bit
>> late now, since allocated pieces are transfered in migration stream
>> separately, so it's not possible to just replace broken layout with
>> correct one. To workaround issue, MemoryRegion alases are made
>> migratable and this patch switches to use them to split big initial
>> RAM chunk into smaller pieces (KVM_SLOT_MAX_BYTES max) and registers
>> aliases for migration. That should keep migration compatible with
>> previous QEMU versions.
>>
>> New machine types (since 4.2) will use single memory region, which
>> will get transimitted in migration stream as a whole RAMBlock.
>>
>> Signed-off-by: Igor Mammedov
>> ---
>> I don't have access to a suitable system to test it, so I've simulated
>> it with smaller chunks on x84 host. Ping-pong migration between old
>> and new QEMU worked fine. KVM part should be fine as memslots
>> using mapped MemoryRegions (in this case it would be aliases) as
>> far as I know but is someone could test it on big enough host it
>> would be nice.
>> ---
>> include/hw/s390x/s390-virtio-ccw.h | 4 +++
>> hw/s390x/s390-virtio-ccw.c | 56 +-
>> 2 files changed, 43 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/hw/s390x/s390-virtio-ccw.h
>> b/include/hw/s390x/s390-virtio-ccw.h
>> index 00632f94b4..f9ed3737f8 100644
>> --- a/include/hw/s390x/s390-virtio-ccw.h
>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>> @@ -21,6 +21,9 @@
>> #define S390_MACHINE_CLASS(klass) \
>> OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE)
>>
>> +#define S390_MACHINE_GET_CLASS(obj) \
>> +OBJECT_GET_CLASS(S390CcwMachineClass, (obj), TYPE_S390_CCW_MACHINE)
>> +
>> /*
>> * KVM does only support memory slots up to KVM_MEM_MAX_NR_PAGES pages
>> * as the dirty bitmap must be managed by bitops that take an int as
>> @@ -50,6 +53,7 @@ typedef struct S390CcwMachineClass {
>> bool cpu_model_allowed;
>> bool css_migration_enabled;
>> bool hpage_1m_allowed;
>> +bool split_ram_layout;
>> } S390CcwMachineClass;
>>
>> /* runtime-instrumentation allowed by the machine */
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 073672f9cb..9160c1ed0a 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -151,28 +151,47 @@ static void virtio_ccw_register_hcalls(void)
>> virtio_ccw_hcall_early_printk);
>> }
>>
>> -static void s390_memory_init(ram_addr_t mem_size)
>> +static void s390_memory_init(MachineState *ms)
>> {
>> +S390CcwMachineClass *s390mc = S390_MACHINE_GET_CLASS(ms);
>> MemoryRegion *sysmem = get_system_memory();
>> -ram_addr_t chunk, offset = 0;
>> +MemoryRegion *ram = g_new(MemoryRegion, 1);
>> unsigned int number = 0;
>> Error *local_err = NULL;
>> -gchar *name;
>> +ram_addr_t mem_size = ms->ram_size;
>> +gchar *name = g_strdup_printf(s390mc->split_ram_layout ?
>> + "s390.whole.ram" : "s390.ram");
>>
>> /* allocate RAM for core */
>> -name = g_strdup_printf("s390.ram");
>> -while (mem_size) {
>> -MemoryRegion *ram = g_new(MemoryRegion, 1);
>> -uint64_t size = mem_size;
>> -
>> -/* KVM does not allow memslots >= 8 TB */
>> -chunk = MIN(size, KVM_SLOT_MAX_BYTES);
>> -memory_region_allocate_system_memory(ram, NULL, name, chunk);
>> -memory_region_add_subregion(sysmem, offset, ram);
>> -mem_size -= chunk;
>> -offset += chunk;
>> -g_free(name);
>> -name = g_strdup_printf("s390.ram.%u", ++number);
>> +memory_region_allocate_system_memory(ram, NULL, name, mem_size);
>> +
>> +/* migration compatible RAM handling for 4.1 and older machines */
>> +if (s390mc->split_ram_layout) {
>> + ram_addr_t chunk, offset = 0;
>> + /*
>> +* memory_region_allocate_system_memory() registers allocated RAM for
>> +* migration, however for compat reasons the RAM should be passed
>> over
>> +* as RAMBlocks of the size upto KVM_SLOT_MAX_BYTES. So unregister
>> just
>> +* allocated RAM so it won't be migrated directly. Aliases will take
>> care
>> +* of segmenting RAM into legacy chunks that migration compatible.
>> +*/
>> + vmstate_unregister_ram(ram, NULL);
>> + name = g_strdup_printf("s390.ram");
>> + while (mem_size) {
>> + MemoryRegion *alias = g_new(MemoryRegion, 1);
>> +
>> + /* KVM does not allow memslots >= 8 TB */
>> + chunk =
Re: [Qemu-devel] [qemu-s390x] [PATCH RFC 0/2] s390: stop abusing memory_region_allocate_system_memory()
On 02.08.19 10:37, David Hildenbrand wrote: > On 02.08.19 10:29, Christian Borntraeger wrote: >> >> >> On 02.08.19 10:04, David Hildenbrand wrote: >>> On 29.07.19 16:52, Igor Mammedov wrote: >>>> While looking into unifying guest RAM allocation to use hostmem backends >>>> for initial RAM (especially when -mempath is used) and retiring >>>> memory_region_allocate_system_memory() API, leaving only single hostmem >>>> backend, >>>> I was inspecting how currently it is used by boards and it turns out >>>> several >>>> boards abuse it by calling the function several times (despite documented >>>> contract >>>> forbiding it). >>>> >>>> s390 is one of such boards where KVM limitation on memslot size got >>>> propagated >>>> to board design and memory_region_allocate_system_memory() was abused to >>>> satisfy >>>> KVM requirement for max RAM chunk where memory region alias would suffice. >>>> >>>> Unfortunately, memory_region_allocate_system_memory() usage created >>>> migration >>>> dependency where guest RAM is transferred in migration stream as several >>>> RAMBlocks >>>> if it's more than KVM_SLOT_MAX_BYTES. >>> >>> So if I understand it correctly, we only call >>> memory_region_allocate_system_memory() in case the guest initial memory >>> size exceeds KVM_SLOT_MAX_BYTES - ~8TB. >> >> We always call it. We just call it twice for > 8TB > > Yeah, that's what I meant. > >>> >>> Do we *really* care about keeping migration of systems running that most >>> probably nobody (except Christian ;) ) really uses? (especially not in >>> production). >>> >>> I am fine keeping migration running if it's easy, but introducing hacks >>> (reading below) for such obscure use cases - I don't know. >>> >>> @Christian: Please prove me wrong. :) >> >> For the time being we can block migration for guests > 8TB if that helps (it >> should not >> fail in a guest killing fashion), but we should >> 1. continue to be able to migrate guests < 8TB >> 2. continue to be >> >> On the other hand I find "and suddenly it fails if you go beyond this" really >> unpleasant. So it would be interesting to see the next round of patches to >> check how "hacky" those really are. > > I mean migration will work perfectly fine once we fixed it for new QEMU > versions. It's only the older QEMU versions to/from the > fixed one. I think that would be fine. We just have to make sure that migration really fails in a way that the system continues to run. > > Looking at the log I can see that this was introduced with v2.12.0. > > I would document this in the next release notes: "Migration of unusual > big VMs (>= 8TB) will not work from/to previous QEMU versions (up to > v2.12, before that starting such guests didn't even work)." Yes, sounds good.
Re: [Qemu-devel] [qemu-s390x] [PATCH RFC 0/2] s390: stop abusing memory_region_allocate_system_memory()
On 02.08.19 10:04, David Hildenbrand wrote: > On 29.07.19 16:52, Igor Mammedov wrote: >> While looking into unifying guest RAM allocation to use hostmem backends >> for initial RAM (especially when -mempath is used) and retiring >> memory_region_allocate_system_memory() API, leaving only single hostmem >> backend, >> I was inspecting how currently it is used by boards and it turns out several >> boards abuse it by calling the function several times (despite documented >> contract >> forbiding it). >> >> s390 is one of such boards where KVM limitation on memslot size got >> propagated >> to board design and memory_region_allocate_system_memory() was abused to >> satisfy >> KVM requirement for max RAM chunk where memory region alias would suffice. >> >> Unfortunately, memory_region_allocate_system_memory() usage created migration >> dependency where guest RAM is transferred in migration stream as several >> RAMBlocks >> if it's more than KVM_SLOT_MAX_BYTES. > > So if I understand it correctly, we only call > memory_region_allocate_system_memory() in case the guest initial memory > size exceeds KVM_SLOT_MAX_BYTES - ~8TB. We always call it. We just call it twice for > 8TB > > Do we *really* care about keeping migration of systems running that most > probably nobody (except Christian ;) ) really uses? (especially not in > production). > > I am fine keeping migration running if it's easy, but introducing hacks > (reading below) for such obscure use cases - I don't know. > > @Christian: Please prove me wrong. :) For the time being we can block migration for guests > 8TB if that helps (it should not fail in a guest killing fashion), but we should 1. continue to be able to migrate guests < 8TB 2. continue to be On the other hand I find "and suddenly it fails if you go beyond this" really unpleasant. So it would be interesting to see the next round of patches to check how "hacky" those really are. > >> >> In order to replace these several RAM chunks with a single memdev and keep it >> working with KVM memslot size limit and migration compatible, following was >> done: >>* [2/2] use memory region aliases to partition hostmem backend RAM on >>KVM_SLOT_MAX_BYTES chunks, which should keep KVM side working >>* [1/2] hacked memory region aliases (to ram memory regions only) to have >>its own RAMBlocks pointing to RAM chunks owned by aliased memory >>region. While it's admittedly a hack, but it's relatively simple >> and >>allows board code rashape migration stream as necessary >> >>I haven't tried to use migratable aliases on x86 machines, but >> with it >>it could be possible to drop legacy RAM allocation and compat knob >>(cd5ff8333a) dropping '-numa node,mem' completely even for old >> machines. >> >> PS: >>Tested with ping pong cross version migration on s390 machine >>(with reduced KVM_SLOT_MAX_BYTES since I don't have access to large >> enough host) >> >> >> Igor Mammedov (2): >> memory: make MemoryRegion alias migratable >> s390: do not call memory_region_allocate_system_memory() multiple >> times >> >> exec.c | 7 --- >> hw/s390x/s390-virtio-ccw.c | 20 +++- >> memory.c | 5 + >> 3 files changed, 24 insertions(+), 8 deletions(-) >> > >
Re: [Qemu-devel] [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call
On 31.07.19 14:28, Christian Borntraeger wrote:
>
>
> On 31.07.19 14:04, Andrey Shinkevich wrote:
>> On 31/07/2019 10:24, Christian Borntraeger wrote:
>>>
>>>
>>> On 30.07.19 21:20, Paolo Bonzini wrote:
>>>> On 30/07/19 18:01, Andrey Shinkevich wrote:
>>>>> Not the whole structure is initialized before passing it to the KVM.
>>>>> Reduce the number of Valgrind reports.
>>>>>
>>>>> Signed-off-by: Andrey Shinkevich
>>>>
>>>> Christian, is this the right fix? It's not expensive so it wouldn't be
>>>> an issue, just checking if there's any better alternative.
>>>
>>> I think all of these variants are valid with pros and cons
>>> 1. teach valgrind about this:
>>> Add to coregrind/m_syswrap/syswrap-linux.c (and the relevant header files)
>>> knowledge about which parts are actually touched.
>>> 2. use designated initializers
>>> 3. use memset
>>> 3. use a valgrind callback VG_USERREQ__MAKE_MEM_DEFINED to tell that this
>>> memory is defined
>>>
>>
>> Thank you all very much for taking part in the discussion.
>> Also, one may use the Valgrind technology to suppress the unwanted
>> reports by adding the Valgrind specific format file valgrind.supp to the
>> QEMU project. The file content is extendable for future needs.
>> All the cases we like to suppress will be recounted in that file.
>> A case looks like the stack fragments. For instance, from QEMU block:
>>
>> {
>> hw/block/hd-geometry.c
>> Memcheck:Cond
>> fun:guess_disk_lchs
>> fun:hd_geometry_guess
>> fun:blkconf_geometry
>> ...
>> fun:device_set_realized
>> fun:property_set_bool
>> fun:object_property_set
>> fun:object_property_set_qobject
>> fun:object_property_set_bool
>> }
>>
>> The number of suppressed cases are reported by the Valgrind with every
>> run: "ERROR SUMMARY: 5 errors from 3 contexts (suppressed: 0 from 0)"
>>
>> Andrey
>
> Yes, indeed that would be another variant. How performance critical are
> the fixed locations? That might have an impact on what is the best solution.
> From a cleanliness approach doing 1 (adding the ioctl definition to valgrind)
> is certainly the most beautiful way. I did that in the past, look for example
> at
>
> https://sourceware.org/git/?p=valgrind.git;a=commitdiff;h=c2baee9b7bf043702c130de0771a4df439fcf403
> or
> https://sourceware.org/git/?p=valgrind.git;a=commitdiff;h=00a31dd3d1e7101b331c2c83fca6c666ba35d910
>
> for examples.
>
>
>>
>>>>
>>>> Paolo
>>>>
>>>>> ---
>>>>> target/i386/kvm.c | 3 +++
>>>>> 1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>>>>> index dbbb137..ed57e31 100644
>>>>> --- a/target/i386/kvm.c
>>>>> +++ b/target/i386/kvm.c
>>>>> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +memset(_data, 0, sizeof(msr_data));
>>>>> msr_data.info.nmsrs = 1;
>>>>> msr_data.entries[0].index = MSR_IA32_TSC;
>>>>> env->tsc_valid = !runstate_is_running();
>>>>> @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>>>>
>>>>> if (has_xsave) {
>>>>> env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
>>>>> +memset(env->xsave_buf, 0, sizeof(struct kvm_xsave));
This is memsetting 4k?
Yet another variant would be to use the RUNNING_ON_VALGRIND macro from
valgrind/valgrind.h to only memset for valgrind. But just using MAKE_MEM_DEFINED
from memcheck.h is simpler.
Re: [Qemu-devel] [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call
On 31.07.19 14:04, Andrey Shinkevich wrote:
> On 31/07/2019 10:24, Christian Borntraeger wrote:
>>
>>
>> On 30.07.19 21:20, Paolo Bonzini wrote:
>>> On 30/07/19 18:01, Andrey Shinkevich wrote:
>>>> Not the whole structure is initialized before passing it to the KVM.
>>>> Reduce the number of Valgrind reports.
>>>>
>>>> Signed-off-by: Andrey Shinkevich
>>>
>>> Christian, is this the right fix? It's not expensive so it wouldn't be
>>> an issue, just checking if there's any better alternative.
>>
>> I think all of these variants are valid with pros and cons
>> 1. teach valgrind about this:
>> Add to coregrind/m_syswrap/syswrap-linux.c (and the relevant header files)
>> knowledge about which parts are actually touched.
>> 2. use designated initializers
>> 3. use memset
>> 3. use a valgrind callback VG_USERREQ__MAKE_MEM_DEFINED to tell that this
>> memory is defined
>>
>
> Thank you all very much for taking part in the discussion.
> Also, one may use the Valgrind technology to suppress the unwanted
> reports by adding the Valgrind specific format file valgrind.supp to the
> QEMU project. The file content is extendable for future needs.
> All the cases we like to suppress will be recounted in that file.
> A case looks like the stack fragments. For instance, from QEMU block:
>
> {
> hw/block/hd-geometry.c
> Memcheck:Cond
> fun:guess_disk_lchs
> fun:hd_geometry_guess
> fun:blkconf_geometry
> ...
> fun:device_set_realized
> fun:property_set_bool
> fun:object_property_set
> fun:object_property_set_qobject
> fun:object_property_set_bool
> }
>
> The number of suppressed cases are reported by the Valgrind with every
> run: "ERROR SUMMARY: 5 errors from 3 contexts (suppressed: 0 from 0)"
>
> Andrey
Yes, indeed that would be another variant. How performance critical are
the fixed locations? That might have an impact on what is the best solution.
>From a cleanliness approach doing 1 (adding the ioctl definition to valgrind)
is certainly the most beautiful way. I did that in the past, look for example at
https://sourceware.org/git/?p=valgrind.git;a=commitdiff;h=c2baee9b7bf043702c130de0771a4df439fcf403
or
https://sourceware.org/git/?p=valgrind.git;a=commitdiff;h=00a31dd3d1e7101b331c2c83fca6c666ba35d910
for examples.
>
>>>
>>> Paolo
>>>
>>>> ---
>>>> target/i386/kvm.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>>>> index dbbb137..ed57e31 100644
>>>> --- a/target/i386/kvm.c
>>>> +++ b/target/i386/kvm.c
>>>> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs)
>>>> return 0;
>>>> }
>>>>
>>>> +memset(_data, 0, sizeof(msr_data));
>>>> msr_data.info.nmsrs = 1;
>>>> msr_data.entries[0].index = MSR_IA32_TSC;
>>>> env->tsc_valid = !runstate_is_running();
>>>> @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>>>
>>>> if (has_xsave) {
>>>> env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
>>>> +memset(env->xsave_buf, 0, sizeof(struct kvm_xsave));
>>>> }
>>>>
>>>> max_nested_state_len = kvm_max_nested_state_length();
>>>> @@ -3477,6 +3479,7 @@ static int kvm_put_debugregs(X86CPU *cpu)
>>>> return 0;
>>>> }
>>>>
>>>> +memset(, 0, sizeof(dbgregs));
>>>> for (i = 0; i < 4; i++) {
>>>> dbgregs.db[i] = env->dr[i];
>>>> }
>>>> --
>>>> 1.8.3.1
>>>>
>>>
>>>
>>
>
Re: [Qemu-devel] [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call
On 30.07.19 21:20, Paolo Bonzini wrote:
> On 30/07/19 18:01, Andrey Shinkevich wrote:
>> Not the whole structure is initialized before passing it to the KVM.
>> Reduce the number of Valgrind reports.
>>
>> Signed-off-by: Andrey Shinkevich
>
> Christian, is this the right fix? It's not expensive so it wouldn't be
> an issue, just checking if there's any better alternative.
I think all of these variants are valid with pros and cons
1. teach valgrind about this:
Add to coregrind/m_syswrap/syswrap-linux.c (and the relevant header files)
knowledge about which parts are actually touched.
2. use designated initializers
3. use memset
3. use a valgrind callback VG_USERREQ__MAKE_MEM_DEFINED to tell that this
memory is defined
>
> Paolo
>
>> ---
>> target/i386/kvm.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>> index dbbb137..ed57e31 100644
>> --- a/target/i386/kvm.c
>> +++ b/target/i386/kvm.c
>> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs)
>> return 0;
>> }
>>
>> +memset(_data, 0, sizeof(msr_data));
>> msr_data.info.nmsrs = 1;
>> msr_data.entries[0].index = MSR_IA32_TSC;
>> env->tsc_valid = !runstate_is_running();
>> @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>
>> if (has_xsave) {
>> env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
>> +memset(env->xsave_buf, 0, sizeof(struct kvm_xsave));
>> }
>>
>> max_nested_state_len = kvm_max_nested_state_length();
>> @@ -3477,6 +3479,7 @@ static int kvm_put_debugregs(X86CPU *cpu)
>> return 0;
>> }
>>
>> +memset(, 0, sizeof(dbgregs));
>> for (i = 0; i < 4; i++) {
>> dbgregs.db[i] = env->dr[i];
>> }
>> --
>> 1.8.3.1
>>
>
>
Re: [Qemu-devel] [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call
On 30.07.19 19:14, Philippe Mathieu-Daudé wrote: > On 7/30/19 7:05 PM, Christian Borntraeger wrote: >> On 30.07.19 18:44, Philippe Mathieu-Daudé wrote: >>> On 7/30/19 6:01 PM, Andrey Shinkevich wrote: >>>> Not the whole structure is initialized before passing it to the KVM. >>>> Reduce the number of Valgrind reports. >>>> >>>> Signed-off-by: Andrey Shinkevich >>>> --- >>>> target/i386/kvm.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c >>>> index dbbb137..ed57e31 100644 >>>> --- a/target/i386/kvm.c >>>> +++ b/target/i386/kvm.c >>>> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs) >>>> return 0; >>>> } >>>> >>>> + memset(_data, 0, sizeof(msr_data)); >>> >>> I wonder the overhead of this one... >> >> Cant we use designated initializers like in >> >> commit bdfc8480c50a53d91aa9a513d23a84de0d5fbc86 >> Author: Christian Borntraeger >> AuthorDate: Thu Oct 30 09:23:41 2014 +0100 >> Commit: Paolo Bonzini >> CommitDate: Mon Dec 15 12:21:01 2014 +0100 >> >> valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl >> >> and others? > > Is the compiler smart enough to figure out it doesn't need to zeroes in > case env->tsc_valid is true and the function returns? Good question, we would need to double check with objdump.
Re: [Qemu-devel] [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call
On 30.07.19 18:46, Peter Maydell wrote: > On Tue, 30 Jul 2019 at 17:05, Andrey Shinkevich > wrote: >> >> Not the whole structure is initialized before passing it to the KVM. >> Reduce the number of Valgrind reports. >> >> Signed-off-by: Andrey Shinkevich > > Does it even make sense to try to valgrind a KVM-enabled run > of QEMU? As soon as we run the guest it will make modifications > to memory which Valgrind can't track; and I don't think > Valgrind supports the KVM_RUN ioctl anyway... As long as we do not care about the guest memory, it does make sense and it does find bugs. See also https://www.linux-kvm.org/page/KVM_Forum_2014 https://www.linux-kvm.org/images/d/d2/03x07-Valgrind.pdf Unfortunately I wasnt able to follow up on those.
Re: [Qemu-devel] [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call
On 30.07.19 18:44, Philippe Mathieu-Daudé wrote:
> On 7/30/19 6:01 PM, Andrey Shinkevich wrote:
>> Not the whole structure is initialized before passing it to the KVM.
>> Reduce the number of Valgrind reports.
>>
>> Signed-off-by: Andrey Shinkevich
>> ---
>> target/i386/kvm.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>> index dbbb137..ed57e31 100644
>> --- a/target/i386/kvm.c
>> +++ b/target/i386/kvm.c
>> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs)
>> return 0;
>> }
>>
>> +memset(_data, 0, sizeof(msr_data));
>
> I wonder the overhead of this one...
Cant we use designated initializers like in
commit bdfc8480c50a53d91aa9a513d23a84de0d5fbc86
Author: Christian Borntraeger
AuthorDate: Thu Oct 30 09:23:41 2014 +0100
Commit: Paolo Bonzini
CommitDate: Mon Dec 15 12:21:01 2014 +0100
valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl
and others?
This should minimize the impact.
>
>> msr_data.info.nmsrs = 1;
>> msr_data.entries[0].index = MSR_IA32_TSC;
>> env->tsc_valid = !runstate_is_running();
>> @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>
>> if (has_xsave) {
>> env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
>> +memset(env->xsave_buf, 0, sizeof(struct kvm_xsave));
>
> OK
>
>> }
>>
>> max_nested_state_len = kvm_max_nested_state_length();
>> @@ -3477,6 +3479,7 @@ static int kvm_put_debugregs(X86CPU *cpu)
>> return 0;
>> }
>>
>> +memset(, 0, sizeof(dbgregs));
>
> OK
>
>> for (i = 0; i < 4; i++) {
>> dbgregs.db[i] = env->dr[i];
>> }
>
> We could remove 'dbgregs.flags = 0;'
>
> Reviewed-by: Philippe Mathieu-Daudé
>
Re: [Qemu-devel] [qemu-s390x] [PATCH RFC 0/2] s390: stop abusing memory_region_allocate_system_memory()
I remember that you send a similar patch a while ago and something broke on s390x. Have you changed something from the old patchs set? On 29.07.19 16:52, Igor Mammedov wrote: > While looking into unifying guest RAM allocation to use hostmem backends > for initial RAM (especially when -mempath is used) and retiring > memory_region_allocate_system_memory() API, leaving only single hostmem > backend, > I was inspecting how currently it is used by boards and it turns out several > boards abuse it by calling the function several times (despite documented > contract > forbiding it). > > s390 is one of such boards where KVM limitation on memslot size got propagated > to board design and memory_region_allocate_system_memory() was abused to > satisfy > KVM requirement for max RAM chunk where memory region alias would suffice. > > Unfortunately, memory_region_allocate_system_memory() usage created migration > dependency where guest RAM is transferred in migration stream as several > RAMBlocks > if it's more than KVM_SLOT_MAX_BYTES. > > In order to replace these several RAM chunks with a single memdev and keep it > working with KVM memslot size limit and migration compatible, following was > done: >* [2/2] use memory region aliases to partition hostmem backend RAM on >KVM_SLOT_MAX_BYTES chunks, which should keep KVM side working >* [1/2] hacked memory region aliases (to ram memory regions only) to have >its own RAMBlocks pointing to RAM chunks owned by aliased memory >region. While it's admittedly a hack, but it's relatively simple > and >allows board code rashape migration stream as necessary > >I haven't tried to use migratable aliases on x86 machines, but > with it >it could be possible to drop legacy RAM allocation and compat knob >(cd5ff8333a) dropping '-numa node,mem' completely even for old > machines. > > PS: >Tested with ping pong cross version migration on s390 machine >(with reduced KVM_SLOT_MAX_BYTES since I don't have access to large > enough host) > > > Igor Mammedov (2): > memory: make MemoryRegion alias migratable > s390: do not call memory_region_allocate_system_memory() multiple > times > > exec.c | 7 --- > hw/s390x/s390-virtio-ccw.c | 20 +++- > memory.c | 5 + > 3 files changed, 24 insertions(+), 8 deletions(-) >
Re: [Qemu-devel] [PATCH] Fixes: a6862418fec4072 iotests change in 051.out
On 24.07.19 10:25, Andrey Shinkevich wrote: > The patch "iotests: Set read-zeroes on in null block driver for Valgrind" > needs the change in 051.out when compared against on the s390 system. > > Reported-by: Christian Borntraeger Tested-by: Christian Borntraeger FWIW, the Fixes tag should be inside the patch description. Maybe Kevin will fix this up when applying? > Signed-off-by: Andrey Shinkevich > --- > tests/qemu-iotests/051.out | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out > index 8993835..554c5ca 100644 > --- a/tests/qemu-iotests/051.out > +++ b/tests/qemu-iotests/051.out > @@ -149,23 +149,23 @@ QEMU X.Y.Z monitor - type 'help' for more information > > === Cache modes === > > -Testing: -drive driver=null-co,cache=none > +Testing: -drive driver=null-co,read-zeroes=on,cache=none > QEMU X.Y.Z monitor - type 'help' for more information > (qemu) quit > > -Testing: -drive driver=null-co,cache=directsync > +Testing: -drive driver=null-co,read-zeroes=on,cache=directsync > QEMU X.Y.Z monitor - type 'help' for more information > (qemu) quit > > -Testing: -drive driver=null-co,cache=writeback > +Testing: -drive driver=null-co,read-zeroes=on,cache=writeback > QEMU X.Y.Z monitor - type 'help' for more information > (qemu) quit > > -Testing: -drive driver=null-co,cache=writethrough > +Testing: -drive driver=null-co,read-zeroes=on,cache=writethrough > QEMU X.Y.Z monitor - type 'help' for more information > (qemu) quit > > -Testing: -drive driver=null-co,cache=unsafe > +Testing: -drive driver=null-co,read-zeroes=on,cache=unsafe > QEMU X.Y.Z monitor - type 'help' for more information > (qemu) quit > >
Re: [Qemu-devel] [PULL 01/13] iotests: Set read-zeroes on in null block driver for Valgrind
On 24.07.19 09:30, Andrey Shinkevich wrote: > > > On 24/07/2019 10:18, Christian Borntraeger wrote: >> >> On 19.07.19 15:43, Kevin Wolf wrote: >>> From: Andrey Shinkevich >>> >>> The Valgrind tool reports about the uninitialised buffer 'buf' >>> instantiated on the stack of the function guess_disk_lchs(). >>> Pass 'read-zeroes=on' to the null block driver to make it deterministic. >>> The output of the tests 051, 186 and 227 now includes the parameter >>> 'read-zeroes'. So, the benchmark output files are being changed too. >>> >>> Suggested-by: Kevin Wolf >>> Signed-off-by: Andrey Shinkevich >>> Signed-off-by: Kevin Wolf >>> --- >>> tests/qemu-iotests/051| 10 +-- >>> tests/qemu-iotests/051.pc.out | 10 +-- >>> tests/qemu-iotests/093| 9 +- >> [...9 >> >> >> I now get the following on s390. >> Seems that you only fixed 051.pc.out but not 051.out >> >> 051 ...[09:01:49] ... 051 >> [1m[31mfail [0m [09:01:49] [09:01:50]output >> mismatch (see 051.out.bad) >> --- tests/qemu-iotests/051.out 2019-07-09 18:34:26.734654933 +0200 >> +++ build/tests/qemu-iotests/051.out.bad2019-07-24 >> 09:01:50.015024901 +0200 >> @@ -149,23 +149,23 @@ >> >> === Cache modes === >> >> -Testing: -drive driver=null-co,cache=none >> +Testing: -drive driver=null-co,read-zeroes=on,cache=none >> QEMU X.Y.Z monitor - type 'help' for more information >> (qemu) quit >> >> -Testing: -drive driver=null-co,cache=directsync >> +Testing: -drive driver=null-co,read-zeroes=on,cache=directsync >> QEMU X.Y.Z monitor - type 'help' for more information >> (qemu) quit >> >> -Testing: -drive driver=null-co,cache=writeback >> +Testing: -drive driver=null-co,read-zeroes=on,cache=writeback >> QEMU X.Y.Z monitor - type 'help' for more information >> (qemu) quit >> >> -Testing: -drive driver=null-co,cache=writethrough >> +Testing: -drive driver=null-co,read-zeroes=on,cache=writethrough >> QEMU X.Y.Z monitor - type 'help' for more information >> (qemu) quit >> >> -Testing: -drive driver=null-co,cache=unsafe >> +Testing: -drive driver=null-co,read-zeroes=on,cache=unsafe >> QEMU X.Y.Z monitor - type 'help' for more information >> (qemu) quit >> >> > > Thank you Christian for your report. > Would you please send the command line you ran the 051 test with? just calling check with -qcow2 and 051 on an s390 system: ./check -qcow2 051 QEMU -- "/home/cborntra/REPOS/qemu/build/tests/qemu-iotests/../../s390x-softmmu/qemu-system-s390x" -nodefaults -machine accel=qtest QEMU_IMG -- "/home/cborntra/REPOS/qemu/build/tests/qemu-iotests/../../qemu-img" QEMU_IO -- "/home/cborntra/REPOS/qemu/build/tests/qemu-iotests/../../qemu-io" --cache writeback -f qcow2 QEMU_NBD -- "/home/cborntra/REPOS/qemu/build/tests/qemu-iotests/../../qemu-nbd" IMGFMT-- qcow2 (compat=1.1) IMGPROTO -- file PLATFORM -- Linux/s390x s38lp08 5.2.0+ TEST_DIR -- /home/cborntra/REPOS/qemu/build/tests/qemu-iotests/scratch SOCKET_SCM_HELPER -- /home/cborntra/REPOS/qemu/build/tests/qemu-iotests/socket_scm_helper 051 fail [09:32:20] [09:32:30]output mismatch (see 051.out.bad) --- /home/cborntra/REPOS/qemu/tests/qemu-iotests/051.out2019-07-11 18:31:29.440732461 +0200 +++ /home/cborntra/REPOS/qemu/build/tests/qemu-iotests/051.out.bad 2019-07-24 09:32:30.390620548 +0200 @@ -149,23 +149,23 @@ === Cache modes === -Testing: -drive driver=null-co,cache=none +Testing: -drive driver=null-co,read-zeroes=on,cache=none QEMU X.Y.Z monitor - type 'help' for more information (qemu) quit -Testing: -drive driver=null-co,cache=directsync +Testing: -drive driver=null-co,read-zeroes=on,cache=directsync QEMU X.Y.Z monitor - type 'help' for more information (qemu) quit -Testing: -drive driver=null-co,cache=writeback +Testing: -drive driver=null-co,read-zeroes=on,cache=writeback QEMU X.Y.Z monitor - type 'help' for more information (qemu) quit -Testing: -drive driver=null-co,cache=writethrough +Testing: -drive driver=null-co,read-zeroes=on,cache=writethrough QEMU X.Y.Z monitor - type 'help' for more information (qemu) quit -Testing: -drive driver=null-co,cache=unsafe +Testing: -drive driver=null-co,read-zeroes=on,cache=unsafe QEMU X.Y.Z monitor - type 'help' for
Re: [Qemu-devel] [PULL 01/13] iotests: Set read-zeroes on in null block driver for Valgrind
On 19.07.19 15:43, Kevin Wolf wrote: > From: Andrey Shinkevich > > The Valgrind tool reports about the uninitialised buffer 'buf' > instantiated on the stack of the function guess_disk_lchs(). > Pass 'read-zeroes=on' to the null block driver to make it deterministic. > The output of the tests 051, 186 and 227 now includes the parameter > 'read-zeroes'. So, the benchmark output files are being changed too. > > Suggested-by: Kevin Wolf > Signed-off-by: Andrey Shinkevich > Signed-off-by: Kevin Wolf > --- > tests/qemu-iotests/051| 10 +-- > tests/qemu-iotests/051.pc.out | 10 +-- > tests/qemu-iotests/093| 9 +- [...9 I now get the following on s390. Seems that you only fixed 051.pc.out but not 051.out 051 ...[09:01:49] ... 051 [1m[31mfail [0m [09:01:49] [09:01:50]output mismatch (see 051.out.bad) --- tests/qemu-iotests/051.out 2019-07-09 18:34:26.734654933 +0200 +++ build/tests/qemu-iotests/051.out.bad2019-07-24 09:01:50.015024901 +0200 @@ -149,23 +149,23 @@ === Cache modes === -Testing: -drive driver=null-co,cache=none +Testing: -drive driver=null-co,read-zeroes=on,cache=none QEMU X.Y.Z monitor - type 'help' for more information (qemu) quit -Testing: -drive driver=null-co,cache=directsync +Testing: -drive driver=null-co,read-zeroes=on,cache=directsync QEMU X.Y.Z monitor - type 'help' for more information (qemu) quit -Testing: -drive driver=null-co,cache=writeback +Testing: -drive driver=null-co,read-zeroes=on,cache=writeback QEMU X.Y.Z monitor - type 'help' for more information (qemu) quit -Testing: -drive driver=null-co,cache=writethrough +Testing: -drive driver=null-co,read-zeroes=on,cache=writethrough QEMU X.Y.Z monitor - type 'help' for more information (qemu) quit -Testing: -drive driver=null-co,cache=unsafe +Testing: -drive driver=null-co,read-zeroes=on,cache=unsafe QEMU X.Y.Z monitor - type 'help' for more information (qemu) quit > > diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051 > index 200660f977..ce942a5444 100755 > --- a/tests/qemu-iotests/051 > +++ b/tests/qemu-iotests/051 > @@ -251,11 +251,11 @@ echo > # Cannot use the test image because cache=none might not work on the host FS > # Use cdrom so that we won't get errors about missing media > > -run_qemu -drive driver=null-co,cache=none > -run_qemu -drive driver=null-co,cache=directsync > -run_qemu -drive driver=null-co,cache=writeback > -run_qemu -drive driver=null-co,cache=writethrough > -run_qemu -drive driver=null-co,cache=unsafe > +run_qemu -drive driver=null-co,read-zeroes=on,cache=none > +run_qemu -drive driver=null-co,read-zeroes=on,cache=directsync > +run_qemu -drive driver=null-co,read-zeroes=on,cache=writeback > +run_qemu -drive driver=null-co,read-zeroes=on,cache=writethrough > +run_qemu -drive driver=null-co,read-zeroes=on,cache=unsafe > run_qemu -drive driver=null-co,cache=invalid_value > > # Can't test direct=on here because O_DIRECT might not be supported on this > FS > diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out > index 2d811c166c..000557c7c8 100644 > --- a/tests/qemu-iotests/051.pc.out > +++ b/tests/qemu-iotests/051.pc.out > @@ -245,23 +245,23 @@ QEMU X.Y.Z monitor - type 'help' for more information > > === Cache modes === > > -Testing: -drive driver=null-co,cache=none > +Testing: -drive driver=null-co,read-zeroes=on,cache=none > QEMU X.Y.Z monitor - type 'help' for more information > (qemu) quit > > -Testing: -drive driver=null-co,cache=directsync > +Testing: -drive driver=null-co,read-zeroes=on,cache=directsync > QEMU X.Y.Z monitor - type 'help' for more information > (qemu) quit > > -Testing: -drive driver=null-co,cache=writeback > +Testing: -drive driver=null-co,read-zeroes=on,cache=writeback > QEMU X.Y.Z monitor - type 'help' for more information > (qemu) quit > > -Testing: -drive driver=null-co,cache=writethrough > +Testing: -drive driver=null-co,read-zeroes=on,cache=writethrough > QEMU X.Y.Z monitor - type 'help' for more information > (qemu) quit > > -Testing: -drive driver=null-co,cache=unsafe > +Testing: -drive driver=null-co,read-zeroes=on,cache=unsafe > QEMU X.Y.Z monitor - type 'help' for more information > (qemu) quit > > diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093 > index d88fbc182e..4b2cac1d0c 100755 > --- a/tests/qemu-iotests/093 > +++ b/tests/qemu-iotests/093 > @@ -38,7 +38,7 @@ class ThrottleTestCase(iotests.QMPTestCase): > def setUp(self): > self.vm = iotests.VM() > for i in range(0, self.max_drives): > -self.vm.add_drive(self.test_img) > +self.vm.add_drive(self.test_img, "file.read-zeroes=on") > self.vm.launch() > > def tearDown(self): > @@ -273,7 +273,8 @@ class
Re: [Qemu-devel] [qemu-s390x] [PATCH 1/3] qmp: don't emit the RESET event on wakeup
On 19.07.19 01:24, Nicholas Piggin wrote:
> Christian Borntraeger's on July 18, 2019 9:27 pm:
>>
>>
>> On 18.07.19 13:06, Paolo Bonzini wrote:
>>> On 18/07/19 12:39, Nicholas Piggin wrote:
>>>> Commit 1405819637f53 ("qmp: don't emit the RESET event on wakeup from
>>>> S3") changed system wakeup to avoid calling qapi_event_send_reset.
>>>> Commit 76ed4b18debfe ("s390/ipl: fix ipl with -no-reboot") appears to
>>>> have inadvertently broken that logic.
>>>>
>>>> Signed-off-by: Nicholas Piggin
>>>> ---
>>>> I'm not quite sure if this patch is correct and haven't tested it, I
>>>> found it by inspection. If this patch is incorrect, I will have to
>>>> adjust patch 2.
>>>>
>>>> vl.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/vl.c b/vl.c
>>>> index 5089fce6c5..ef3c7ab8b8 100644
>>>> --- a/vl.c
>>>> +++ b/vl.c
>>>> @@ -1550,7 +1550,7 @@ void qemu_system_reset(ShutdownCause reason)
>>>> } else {
>>>> qemu_devices_reset();
>>>> }
>>>> -if (reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) {
>>>> +if (reason && reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) {
>>>> qapi_event_send_reset(shutdown_caused_by_guest(reason), reason);
>>>> }
>>>> cpu_synchronize_all_post_reset();
>>>>
>>>
>>> Yes, it seems correct and I've queued it for 4.1.
>>
>> Yes makes sense.
>> Would it be better to write out if(reason) e.g. replace "reason" with
>> "reason != SHUTDOWN_CAUSE_NONE" ?
>
> I guess it's a matter of preference so I won't weigh in :) My patch
> did try to revert back to the previous form but I'm happy to change
> it.
>
>> Going even further, I am asking myself if something like
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 60bd081d3ef3..406743566869 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -577,7 +577,7 @@ void s390_ipl_reset_request(CPUState *cs, enum
>> s390_reset reset_type)
>> if (reset_type == S390_RESET_MODIFIED_CLEAR ||
>> reset_type == S390_RESET_LOAD_NORMAL) {
>> /* ignore -no-reboot, send no event */
>> -qemu_system_reset_request(SHUTDOWN_CAUSE_SUBSYSTEM_RESET);
>> +qemu_system_reset_request(SHUTDOWN_CAUSE_NONE);
>> } else {
>> qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>> }
>>
>> would also work.
>
> If that works for you, then you could take out the test in the reset
> code. You would have to modify qemu_system_reset_request too of course.
>
> But it seems a bit unsatisfactory to change the reason for the request
> so as to influence behaviour. Either the requests should ask for
> particular behaviour, or the logic for determining how to handle
> the type of request should remain in the reset logic, I would say.
I agree.
Anyway I think your patch is good as is.
Acked-by: Christian Borntraeger
Re: [Qemu-devel] [PATCH v6] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
On 18.07.19 15:06, Laurent Vivier wrote:
> From: Daniel P. Berrangé
>
> The SIOCGSTAMP symbol was previously defined in the
> asm-generic/sockios.h header file. QEMU sees that header
> indirectly via sys/socket.h
>
> In linux kernel commit 0768e17073dc527ccd18ed5f96ce85f9985e9115
> the asm-generic/sockios.h header no longer defines SIOCGSTAMP.
> Instead it provides only SIOCGSTAMP_OLD, which only uses a
> 32-bit time_t on 32-bit architectures.
>
> The linux/sockios.h header then defines SIOCGSTAMP using
> either SIOCGSTAMP_OLD or SIOCGSTAMP_NEW as appropriate. If
> SIOCGSTAMP_NEW is used, then the tv_sec field is 64-bit even
> on 32-bit architectures
>
> To cope with this we must now convert the old and new type from
> the target to the host one.
>
> Signed-off-by: Daniel P. Berrangé
> Signed-off-by: Laurent Vivier
> Reviewed-by: Arnd Bergmann
I can confirm that this fixes the compile errors with newer kernel headers
and I think we should have this for 4.1.
> ---
>
> Notes:
> v6: [lv] fix IOCTLEntry entries (host_cmd and name)
> v5: [lv] define special _OLD values for sh
> define special case for SPARC64
> define ioctl helpers
> define target__kernel_sock_timeval and target__kernel_timespec and
> converters to the host type
> always use SIOCGSTAMP and SIOCGSTAMPNS on the host
>
> v4: [lv] timeval64 and timespec64 are { long long , long }
>
> v3: [lv] redefine TARGET_SIOCGSTAMP_NEW, TARGET_SIOCGSTAMPNS_NEW,
> timeval64 and timespec64 to use 0x89 type and abi_llong[2]
>
> v2: [dpb] implement _NEW and _OLD variants
>
> linux-user/ioctls.h| 21 +-
> linux-user/syscall.c | 140 +
> linux-user/syscall_defs.h | 30 +++-
> linux-user/syscall_types.h | 6 --
> 4 files changed, 159 insertions(+), 38 deletions(-)
>
> diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
> index 5e84dc7c3a77..3281c97ca263 100644
> --- a/linux-user/ioctls.h
> +++ b/linux-user/ioctls.h
> @@ -222,8 +222,25 @@
>IOCTL(SIOCGIWNAME, IOC_W | IOC_R, MK_PTR(MK_STRUCT(STRUCT_char_ifreq)))
>IOCTL(SIOCSPGRP, IOC_W, MK_PTR(TYPE_INT)) /* pid_t */
>IOCTL(SIOCGPGRP, IOC_R, MK_PTR(TYPE_INT)) /* pid_t */
> - IOCTL(SIOCGSTAMP, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval)))
> - IOCTL(SIOCGSTAMPNS, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec)))
> +
> + /*
> + * We can't use IOCTL_SPECIAL() because it will set
> + * host_cmd to XXX_OLD and XXX_NEW and these macros
> + * are not defined with kernel prior to 5.2.
> + * We must set host_cmd to the same value as in target_cmd
> + * otherwise the consistency check in syscall_init()
> + * will trigger an error.
> + * host_cmd is ignored by the do_ioctl_XXX() helpers.
> + * FIXME: create a macro to define this kind of entry
> + */
> + { TARGET_SIOCGSTAMP_OLD, TARGET_SIOCGSTAMP_OLD,
> +"SIOCGSTAMP_OLD", IOC_R, do_ioctl_SIOCGSTAMP },
> + { TARGET_SIOCGSTAMPNS_OLD, TARGET_SIOCGSTAMPNS_OLD,
> +"SIOCGSTAMPNS_OLD", IOC_R, do_ioctl_SIOCGSTAMPNS },
> + { TARGET_SIOCGSTAMP_NEW, TARGET_SIOCGSTAMP_NEW,
> +"SIOCGSTAMP_NEW", IOC_R, do_ioctl_SIOCGSTAMP },
> + { TARGET_SIOCGSTAMPNS_NEW, TARGET_SIOCGSTAMPNS_NEW,
> +"SIOCGSTAMPNS_NEW", IOC_R, do_ioctl_SIOCGSTAMPNS },
>
>IOCTL(RNDGETENTCNT, IOC_R, MK_PTR(TYPE_INT))
>IOCTL(RNDADDTOENTCNT, IOC_W, MK_PTR(TYPE_INT))
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 39a37496fed5..8367cb138dfe 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -37,6 +37,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -1126,8 +1127,9 @@ static inline abi_long copy_from_user_timeval(struct
> timeval *tv,
> {
> struct target_timeval *target_tv;
>
> -if (!lock_user_struct(VERIFY_READ, target_tv, target_tv_addr, 1))
> +if (!lock_user_struct(VERIFY_READ, target_tv, target_tv_addr, 1)) {
> return -TARGET_EFAULT;
> +}
>
> __get_user(tv->tv_sec, _tv->tv_sec);
> __get_user(tv->tv_usec, _tv->tv_usec);
> @@ -1142,8 +1144,26 @@ static inline abi_long copy_to_user_timeval(abi_ulong
> target_tv_addr,
> {
> struct target_timeval *target_tv;
>
> -if (!lock_user_struct(VERIFY_WRITE, target_tv, target_tv_addr, 0))
> +if (!lock_user_struct(VERIFY_WRITE, target_tv, target_tv_addr, 0)) {
> +return -TARGET_EFAULT;
> +}
> +
> +__put_user(tv->tv_sec, _tv->tv_sec);
> +__put_user(tv->tv_usec, _tv->tv_usec);
> +
> +unlock_user_struct(target_tv, target_tv_addr, 1);
> +
> +return 0;
> +}
> +
> +static inline abi_long copy_to_user_timeval64(abi_ulong target_tv_addr,
> + const struct timeval *tv)
> +{
> +struct target__kernel_sock_timeval *target_tv;
> +
> +if (!lock_user_struct(VERIFY_WRITE, target_tv, target_tv_addr, 0)) {
> return -TARGET_EFAULT;
> +}
>
>
Re: [Qemu-devel] [PATCH 1/3] qmp: don't emit the RESET event on wakeup
On 18.07.19 13:06, Paolo Bonzini wrote:
> On 18/07/19 12:39, Nicholas Piggin wrote:
>> Commit 1405819637f53 ("qmp: don't emit the RESET event on wakeup from
>> S3") changed system wakeup to avoid calling qapi_event_send_reset.
>> Commit 76ed4b18debfe ("s390/ipl: fix ipl with -no-reboot") appears to
>> have inadvertently broken that logic.
>>
>> Signed-off-by: Nicholas Piggin
>> ---
>> I'm not quite sure if this patch is correct and haven't tested it, I
>> found it by inspection. If this patch is incorrect, I will have to
>> adjust patch 2.
>>
>> vl.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 5089fce6c5..ef3c7ab8b8 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1550,7 +1550,7 @@ void qemu_system_reset(ShutdownCause reason)
>> } else {
>> qemu_devices_reset();
>> }
>> -if (reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) {
>> +if (reason && reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) {
>> qapi_event_send_reset(shutdown_caused_by_guest(reason), reason);
>> }
>> cpu_synchronize_all_post_reset();
>>
>
> Yes, it seems correct and I've queued it for 4.1.
Yes makes sense.
Would it be better to write out if(reason) e.g. replace "reason" with "reason
!= SHUTDOWN_CAUSE_NONE" ?
Going even further, I am asking myself if something like
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 60bd081d3ef3..406743566869 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -577,7 +577,7 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset
reset_type)
if (reset_type == S390_RESET_MODIFIED_CLEAR ||
reset_type == S390_RESET_LOAD_NORMAL) {
/* ignore -no-reboot, send no event */
-qemu_system_reset_request(SHUTDOWN_CAUSE_SUBSYSTEM_RESET);
+qemu_system_reset_request(SHUTDOWN_CAUSE_NONE);
} else {
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
}
would also work.
Re: [Qemu-devel] [qemu-s390x] [PATCH for-4.1 1/2] s390x/pci: add some fallthrough annotations
On 17.07.19 10:54, Cornelia Huck wrote:
> On Tue, 16 Jul 2019 14:34:22 -0400
> Collin Walling wrote:
>
>> On 7/16/19 11:20 AM, Cornelia Huck wrote:
>>> On Wed, 10 Jul 2019 10:20:41 +0200
>>> Cornelia Huck wrote:
>>>
>>>> On Tue, 9 Jul 2019 18:55:34 -0400
>>>> Collin Walling wrote:
>>>>
>>>>> On 7/8/19 9:23 AM, Christian Borntraeger wrote:
>>>>>>
>>>>>>
>>>>>> On 08.07.19 14:54, Cornelia Huck wrote:
>>>>>>> According to the comment, the bits are supposed to accumulate.
>>>>>>>
>>>>>>> Reported-by: Stefan Weil
>>>>>>> Fixes: 5d1abf234462 ("s390x/pci: enforce zPCI state checking")
>>>>>>> Signed-off-by: Cornelia Huck
>>>>>>
>>>>>> This patch does not change behaviour, so it is certainly not wrong.
>>>>>>
>>>>>> So lets have a look at if the bug report was actually a real bug or
>>>>>> just a missing annotation.
>>>>>>
>>>>>>> ---
>>>>>>>hw/s390x/s390-pci-inst.c | 2 ++
>>>>>>>1 file changed, 2 insertions(+)
>>>>>>>
>>>>>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>>>>>>> index 61f30b8e55d2..00235148bed7 100644
>>>>>>> --- a/hw/s390x/s390-pci-inst.c
>>>>>>> +++ b/hw/s390x/s390-pci-inst.c
>>>>>>> @@ -1209,8 +1209,10 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t
>>>>>>> r1, uint64_t fiba, uint8_t ar,
>>>>>>> * FH Enabled bit is set to one in states of ENABLED, BLOCKED or
>>>>>>> ERROR. */
>>>>>>>case ZPCI_FS_ERROR:
>>>>>>>fib.fc |= 0x20;
>>>>>>> +/* fallthrough */
>>>>>>
>>>>>> This is correct, in case of an error we are also blocked.
>>>>>>
>>>>>
>>>>> Agreed. This is definitely correct based on our architecture.
>>>>>
>>>>>>>case ZPCI_FS_BLOCKED:
>>>>>>>fib.fc |= 0x40;
>>>>>>> +/* fallthrough */
>>>>>>
>>>>>> I think this is also correct, but it would be good if Collin could
>>>>>> verify.
>>>>>>
>>>>>
>>>>> I failed to find anything to support setting the function control
>>>>> enabled bit when the function state is in error / blocked. I'm
>>>>> assuming this might be some QEMU hack to get things working? I'll have
>>>>> to dive further to understand why this was done this way, as it doesn't
>>>>> align with how the s390x architecture is documented. It's confusing.
>>>>
>>>> Might this also be a real issue? Not matching the architecture is not a
>>>> good sign...
>>>
>>> Friendly ping. If we still want to have this patch or a fix in 4.1, we
>>> need to find out soon...
>>>
>>
>> Let's take it for now.
>>
>> Acked-by: Collin Walling
>>
>
> Just to be clear: You think that the current code is correct AFAYCS?
I also looked into this again.
There is a possibility to also be in disabled state.
>From what I can see, it makes sense that blocked and error belong to the
>enable state
so the patch seems correct.
Re: [Qemu-devel] [PATCH] s390: support EDAT-2 in mmu_translate_region
On 16.07.19 15:04, Cornelia Huck wrote: > On Tue, 16 Jul 2019 14:52:03 +0200 > Ilya Leoshkevich wrote: > >>> Am 16.07.2019 um 14:41 schrieb David Hildenbrand : >>> >>> How urgent is this? If this can wait, I can polish and send my series I >>> have here >>> instead, which also implents >>> - IEP support >>> - access-exception-fetch/store-indication facility >>> - ESOP-1, ESOP-2 >> >> This is not urgent, I can live with my patch for now. >> It’s good to know that proper EDAT-2 support is being worked on. >> >> Thanks! >> Ilya > > Ok, so I will not queue this patch right now (I assume you're fine with > keeping this locally for now?), but wait for David's series for 4.2. > > Sounds reasonable? While not complete, Ilyas patch clearly improves the situation (and it is pretty similar to the EDAT-1 support). I think the question is: are there other instruction that we emulate in qemu via the page table walker even for KVM? I believe we always go via the kvm memory ioctl for page table access via instructions so the patch is not critical for KVM. So unless we have something I think Connys proposal is reasonable.
Re: [Qemu-devel] [qemu-s390x] [PATCH 2/3] s390x/cpumodel: also change name of vxbeh
On 16.07.19 10:30, Cornelia Huck wrote:
> On Tue, 16 Jul 2019 09:25:42 +0200
> Christian Borntraeger wrote:
>
>> On 16.07.19 09:24, David Hildenbrand wrote:
>
>>> We also have
>>>
>>> sortl vs. sort
>>> vxe vs. vxeh
>>> vxe2 vs. vxeh2
>>>
>>> So I tend to prefer "vxpde", or rather "vxpdeh".
>>>
>>> (all other enhancement facilities have "eh", so we should actually use
>>> "vxpdeh")
>>
>> Fine with me. Conny, shall I resend or can you fixup everything?
>>
>
> I now have the following; can you please double check?
looks good.
>
> commit a02c8264b7219bc30ec258f068c89b93ad244c36
> Author: Christian Borntraeger
> Date: Mon Jul 15 16:23:03 2019 +0200
>
> s390x/cpumodel: also change name of vxbeh
>
> David suggested to keep everything in sync as 4.1 is not yet released.
> This patch fixes the name "vxbeh" into "vxpdeh".
>
> To simplify the backports this patch will not change VECTOR_BCD_ENH as
> this is just an internal name. That will be done by an extra patch that
> does not need to be backported.
>
> Suggested-by: David Hildenbrand
> Fixes: d05be57ddc2e ("s390: cpumodel: fix description for the new vector
> facility")
> Fixes: 54d65de0b525 ("s390x/cpumodel: vector enhancements")
> Signed-off-by: Christian Borntraeger
> Message-Id: <20190715142304.215018-3-borntrae...@de.ibm.com>
> Reviewed-by: David Hildenbrand
> Signed-off-by: Cornelia Huck
>
> diff --git a/target/s390x/cpu_features_def.inc.h
> b/target/s390x/cpu_features_def.inc.h
> index 3118a9f89228..05b7674affe6 100644
> --- a/target/s390x/cpu_features_def.inc.h
> +++ b/target/s390x/cpu_features_def.inc.h
> @@ -104,7 +104,7 @@ DEF_FEAT(CMM_NT, "cmmnt", STFL, 147, "CMM:
> ESSA-enhancement (no translate) facil
> DEF_FEAT(VECTOR_ENH2, "vxeh2", STFL, 148, "Vector Enhancements facility 2")
> DEF_FEAT(ESORT_BASE, "esort-base", STFL, 150, "Enhanced-sort facility
> (excluding subfunctions)")
> DEF_FEAT(DEFLATE_BASE, "deflate-base", STFL, 151, "Deflate-conversion
> facility (excluding subfunctions)")
> -DEF_FEAT(VECTOR_BCD_ENH, "vxbeh", STFL, 152,
> "Vector-Packed-Decimal-Enhancement Facility")
> +DEF_FEAT(VECTOR_BCD_ENH, "vxpdeh", STFL, 152,
> "Vector-Packed-Decimal-Enhancement Facility")
> DEF_FEAT(MSA_EXT_9, "msa9-base", STFL, 155,
> "Message-security-assist-extension-9 facility (excluding subfunctions)")
> DEF_FEAT(ETOKEN, "etoken", STFL, 156, "Etoken facility")
>
>
Re: [Qemu-devel] [qemu-s390x] [PATCH 2/3] s390x/cpumodel: also change name of vxbeh
On 16.07.19 09:24, David Hildenbrand wrote:
> On 15.07.19 18:12, Christian Borntraeger wrote:
>>
>>
>> On 15.07.19 17:50, Christian Borntraeger wrote:
>>>
>>>
>>> On 15.07.19 17:02, Thomas Huth wrote:
>>>> On 15/07/2019 16.23, Christian Borntraeger wrote:
>>>>> David suggested to keep everything in sync as 4.1 is not yet released.
>>>>> This patch fixes the name "vxbeh" into "vxp".
>>>>>
>>>>> To simplify the backports this patch will not change VECTOR_BCD_ENH as
>>>>> this is just an internal name. That will be done by an extra patch that
>>>>> does not need to be backported.
>>>>>
>>>>> Suggested-by: David Hildenbrand
>>>>> Fixes: d05be57ddc2e ("s390: cpumodel: fix description for the new vector
>>>>> facility")
>>>>> Fixes: 54d65de0b525 ("s390x/cpumodel: vector enhancements")
>>>>> Signed-off-by: Christian Borntraeger
>>>>> ---
>>>>> target/s390x/cpu_features_def.inc.h | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/target/s390x/cpu_features_def.inc.h
>>>>> b/target/s390x/cpu_features_def.inc.h
>>>>> index 3118a9f89228..99f58a89318a 100644
>>>>> --- a/target/s390x/cpu_features_def.inc.h
>>>>> +++ b/target/s390x/cpu_features_def.inc.h
>>>>> @@ -104,7 +104,7 @@ DEF_FEAT(CMM_NT, "cmmnt", STFL, 147, "CMM:
>>>>> ESSA-enhancement (no translate) facil
>>>>> DEF_FEAT(VECTOR_ENH2, "vxeh2", STFL, 148, "Vector Enhancements facility
>>>>> 2")
>>>>> DEF_FEAT(ESORT_BASE, "esort-base", STFL, 150, "Enhanced-sort facility
>>>>> (excluding subfunctions)")
>>>>> DEF_FEAT(DEFLATE_BASE, "deflate-base", STFL, 151, "Deflate-conversion
>>>>> facility (excluding subfunctions)")
>>>>> -DEF_FEAT(VECTOR_BCD_ENH, "vxbeh", STFL, 152,
>>>>> "Vector-Packed-Decimal-Enhancement Facility")
>>>>> +DEF_FEAT(VECTOR_BCD_ENH, "vxp", STFL, 152,
>>>>> "Vector-Packed-Decimal-Enhancement Facility")
>>>>
>>>> We already have:
>>>>
>>>> DEF_FEAT(VECTOR_PACKED_DECIMAL, "vxpd", STFL, 134, "Vector packed decimal
>>>> facility")
>>>>
>>>> ... so I rather expected something like "vxpde" here instead? Or is there
>>>> a reason
>>>>
>>> for just using "vxp"?
>>>
>>> Matching what the Linux kernel has.
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/s390/kernel/processor.c?id=a8fd61688dfad6fdce95fa64cacd8a66595697b8
>>>>
>>
>> Since we differ from the kernel in other places as well we might use
>> something else, of course.
>>
>
> We also have
>
> sortl vs. sort
> vxe vs. vxeh
> vxe2 vs. vxeh2
>
> So I tend to prefer "vxpde", or rather "vxpdeh".
>
> (all other enhancement facilities have "eh", so we should actually use
> "vxpdeh")
Fine with me. Conny, shall I resend or can you fixup everything?
Re: [Qemu-devel] [qemu-s390x] [PATCH 2/3] s390x/cpumodel: also change name of vxbeh
On 15.07.19 17:50, Christian Borntraeger wrote:
>
>
> On 15.07.19 17:02, Thomas Huth wrote:
>> On 15/07/2019 16.23, Christian Borntraeger wrote:
>>> David suggested to keep everything in sync as 4.1 is not yet released.
>>> This patch fixes the name "vxbeh" into "vxp".
>>>
>>> To simplify the backports this patch will not change VECTOR_BCD_ENH as
>>> this is just an internal name. That will be done by an extra patch that
>>> does not need to be backported.
>>>
>>> Suggested-by: David Hildenbrand
>>> Fixes: d05be57ddc2e ("s390: cpumodel: fix description for the new vector
>>> facility")
>>> Fixes: 54d65de0b525 ("s390x/cpumodel: vector enhancements")
>>> Signed-off-by: Christian Borntraeger
>>> ---
>>> target/s390x/cpu_features_def.inc.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target/s390x/cpu_features_def.inc.h
>>> b/target/s390x/cpu_features_def.inc.h
>>> index 3118a9f89228..99f58a89318a 100644
>>> --- a/target/s390x/cpu_features_def.inc.h
>>> +++ b/target/s390x/cpu_features_def.inc.h
>>> @@ -104,7 +104,7 @@ DEF_FEAT(CMM_NT, "cmmnt", STFL, 147, "CMM:
>>> ESSA-enhancement (no translate) facil
>>> DEF_FEAT(VECTOR_ENH2, "vxeh2", STFL, 148, "Vector Enhancements facility 2")
>>> DEF_FEAT(ESORT_BASE, "esort-base", STFL, 150, "Enhanced-sort facility
>>> (excluding subfunctions)")
>>> DEF_FEAT(DEFLATE_BASE, "deflate-base", STFL, 151, "Deflate-conversion
>>> facility (excluding subfunctions)")
>>> -DEF_FEAT(VECTOR_BCD_ENH, "vxbeh", STFL, 152,
>>> "Vector-Packed-Decimal-Enhancement Facility")
>>> +DEF_FEAT(VECTOR_BCD_ENH, "vxp", STFL, 152,
>>> "Vector-Packed-Decimal-Enhancement Facility")
>>
>> We already have:
>>
>> DEF_FEAT(VECTOR_PACKED_DECIMAL, "vxpd", STFL, 134, "Vector packed decimal
>> facility")
>>
>> ... so I rather expected something like "vxpde" here instead? Or is there a
>> reason
>>
> for just using "vxp"?
>
> Matching what the Linux kernel has.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/s390/kernel/processor.c?id=a8fd61688dfad6fdce95fa64cacd8a66595697b8
>>
Since we differ from the kernel in other places as well we might use something
else, of course.
Re: [Qemu-devel] [PATCH 3/3] s390x/cpumodel: change internal name of vxp to make description
On 15.07.19 17:35, Cornelia Huck wrote: > On Mon, 15 Jul 2019 16:23:04 +0200 > Christian Borntraeger wrote: > >> The internal macro name VECTOR_BCD_ENH does not match the actual >> description. Fix this. >> >> Signed-off-by: Christian Borntraeger >> --- >> target/s390x/cpu_features_def.inc.h | 2 +- >> target/s390x/gen-features.c | 4 ++-- >> 2 files changed, 3 insertions(+), 3 deletions(-) > > s/make/match/ in $SUBJECT? > yep.
Re: [Qemu-devel] [qemu-s390x] [PATCH 2/3] s390x/cpumodel: also change name of vxbeh
On 15.07.19 17:02, Thomas Huth wrote:
> On 15/07/2019 16.23, Christian Borntraeger wrote:
>> David suggested to keep everything in sync as 4.1 is not yet released.
>> This patch fixes the name "vxbeh" into "vxp".
>>
>> To simplify the backports this patch will not change VECTOR_BCD_ENH as
>> this is just an internal name. That will be done by an extra patch that
>> does not need to be backported.
>>
>> Suggested-by: David Hildenbrand
>> Fixes: d05be57ddc2e ("s390: cpumodel: fix description for the new vector
>> facility")
>> Fixes: 54d65de0b525 ("s390x/cpumodel: vector enhancements")
>> Signed-off-by: Christian Borntraeger
>> ---
>> target/s390x/cpu_features_def.inc.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/s390x/cpu_features_def.inc.h
>> b/target/s390x/cpu_features_def.inc.h
>> index 3118a9f89228..99f58a89318a 100644
>> --- a/target/s390x/cpu_features_def.inc.h
>> +++ b/target/s390x/cpu_features_def.inc.h
>> @@ -104,7 +104,7 @@ DEF_FEAT(CMM_NT, "cmmnt", STFL, 147, "CMM:
>> ESSA-enhancement (no translate) facil
>> DEF_FEAT(VECTOR_ENH2, "vxeh2", STFL, 148, "Vector Enhancements facility 2")
>> DEF_FEAT(ESORT_BASE, "esort-base", STFL, 150, "Enhanced-sort facility
>> (excluding subfunctions)")
>> DEF_FEAT(DEFLATE_BASE, "deflate-base", STFL, 151, "Deflate-conversion
>> facility (excluding subfunctions)")
>> -DEF_FEAT(VECTOR_BCD_ENH, "vxbeh", STFL, 152,
>> "Vector-Packed-Decimal-Enhancement Facility")
>> +DEF_FEAT(VECTOR_BCD_ENH, "vxp", STFL, 152,
>> "Vector-Packed-Decimal-Enhancement Facility")
>
> We already have:
>
> DEF_FEAT(VECTOR_PACKED_DECIMAL, "vxpd", STFL, 134, "Vector packed decimal
> facility")
>
> ... so I rather expected something like "vxpde" here instead? Or is there a
> reason
>
for just using "vxp"?
Matching what the Linux kernel has.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/s390/kernel/processor.c?id=a8fd61688dfad6fdce95fa64cacd8a66595697b8
>
> Thomas
>
[Qemu-devel] [PATCH 1/3] s390x/cpumodel: remove esort from the default model
esort might not be available on all models.
Fixes: caef62430fed6e73 ("s390x/cpumodel: add gen15 defintions")
Signed-off-by: Christian Borntraeger
---
target/s390x/gen-features.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 9f216219ff53..6debfc1d217e 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -642,7 +642,6 @@ static uint16_t default_GEN14_GA1[] = {
static uint16_t default_GEN15_GA1[] = {
S390_FEAT_VECTOR_ENH2,
-S390_FEAT_GROUP_ENH_SORT,
S390_FEAT_GROUP_DEFLATE_CONVERSION,
S390_FEAT_VECTOR_BCD_ENH,
S390_FEAT_GROUP_MSA_EXT_9,
--
2.21.0
[Qemu-devel] [PATCH 3/3] s390x/cpumodel: change internal name of vxp to make description
The internal macro name VECTOR_BCD_ENH does not match the actual
description. Fix this.
Signed-off-by: Christian Borntraeger
---
target/s390x/cpu_features_def.inc.h | 2 +-
target/s390x/gen-features.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/target/s390x/cpu_features_def.inc.h
b/target/s390x/cpu_features_def.inc.h
index 99f58a89318a..f7c52bc17c2e 100644
--- a/target/s390x/cpu_features_def.inc.h
+++ b/target/s390x/cpu_features_def.inc.h
@@ -104,7 +104,7 @@ DEF_FEAT(CMM_NT, "cmmnt", STFL, 147, "CMM: ESSA-enhancement
(no translate) facil
DEF_FEAT(VECTOR_ENH2, "vxeh2", STFL, 148, "Vector Enhancements facility 2")
DEF_FEAT(ESORT_BASE, "esort-base", STFL, 150, "Enhanced-sort facility
(excluding subfunctions)")
DEF_FEAT(DEFLATE_BASE, "deflate-base", STFL, 151, "Deflate-conversion facility
(excluding subfunctions)")
-DEF_FEAT(VECTOR_BCD_ENH, "vxp", STFL, 152, "Vector-Packed-Decimal-Enhancement
Facility")
+DEF_FEAT(VECTOR_PACKED_DECIMAL_ENH, "vxp", STFL, 152,
"Vector-Packed-Decimal-Enhancement Facility")
DEF_FEAT(MSA_EXT_9, "msa9-base", STFL, 155,
"Message-security-assist-extension-9 facility (excluding subfunctions)")
DEF_FEAT(ETOKEN, "etoken", STFL, 156, "Etoken facility")
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 6debfc1d217e..49a650ac52d0 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -558,7 +558,7 @@ static uint16_t full_GEN15_GA1[] = {
S390_FEAT_VECTOR_ENH2,
S390_FEAT_GROUP_ENH_SORT,
S390_FEAT_GROUP_DEFLATE_CONVERSION,
-S390_FEAT_VECTOR_BCD_ENH,
+S390_FEAT_VECTOR_PACKED_DECIMAL_ENH,
S390_FEAT_GROUP_MSA_EXT_9,
S390_FEAT_GROUP_MSA_EXT_9_PCKMO,
S390_FEAT_ETOKEN,
@@ -643,7 +643,7 @@ static uint16_t default_GEN14_GA1[] = {
static uint16_t default_GEN15_GA1[] = {
S390_FEAT_VECTOR_ENH2,
S390_FEAT_GROUP_DEFLATE_CONVERSION,
-S390_FEAT_VECTOR_BCD_ENH,
+S390_FEAT_VECTOR_PACKED_DECIMAL_ENH,
S390_FEAT_GROUP_MSA_EXT_9,
S390_FEAT_GROUP_MSA_EXT_9_PCKMO,
S390_FEAT_ETOKEN,
--
2.21.0
[Qemu-devel] [PATCH 2/3] s390x/cpumodel: also change name of vxbeh
David suggested to keep everything in sync as 4.1 is not yet released.
This patch fixes the name "vxbeh" into "vxp".
To simplify the backports this patch will not change VECTOR_BCD_ENH as
this is just an internal name. That will be done by an extra patch that
does not need to be backported.
Suggested-by: David Hildenbrand
Fixes: d05be57ddc2e ("s390: cpumodel: fix description for the new vector
facility")
Fixes: 54d65de0b525 ("s390x/cpumodel: vector enhancements")
Signed-off-by: Christian Borntraeger
---
target/s390x/cpu_features_def.inc.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/s390x/cpu_features_def.inc.h
b/target/s390x/cpu_features_def.inc.h
index 3118a9f89228..99f58a89318a 100644
--- a/target/s390x/cpu_features_def.inc.h
+++ b/target/s390x/cpu_features_def.inc.h
@@ -104,7 +104,7 @@ DEF_FEAT(CMM_NT, "cmmnt", STFL, 147, "CMM: ESSA-enhancement
(no translate) facil
DEF_FEAT(VECTOR_ENH2, "vxeh2", STFL, 148, "Vector Enhancements facility 2")
DEF_FEAT(ESORT_BASE, "esort-base", STFL, 150, "Enhanced-sort facility
(excluding subfunctions)")
DEF_FEAT(DEFLATE_BASE, "deflate-base", STFL, 151, "Deflate-conversion facility
(excluding subfunctions)")
-DEF_FEAT(VECTOR_BCD_ENH, "vxbeh", STFL, 152,
"Vector-Packed-Decimal-Enhancement Facility")
+DEF_FEAT(VECTOR_BCD_ENH, "vxp", STFL, 152, "Vector-Packed-Decimal-Enhancement
Facility")
DEF_FEAT(MSA_EXT_9, "msa9-base", STFL, 155,
"Message-security-assist-extension-9 facility (excluding subfunctions)")
DEF_FEAT(ETOKEN, "etoken", STFL, 156, "Etoken facility")
--
2.21.0
[Qemu-devel] [PATCH 0/3] s390x/cpumodel fixes for 4.1
Some fallout of the gen15 cpu model. As this is new in 4.1 it is still time to fixup some aspects. Christian Borntraeger (3): s390x/cpumodel: remove esort from the default model s390x/cpumodel: also change name of vxbeh s390x/cpumodel: change internal name of vxp to make description target/s390x/cpu_features_def.inc.h | 2 +- target/s390x/gen-features.c | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) -- 2.21.0
Re: [Qemu-devel] [PULL 00/19] Migration patches
On 11.07.19 12:43, Juan Quintela wrote: > The following changes since commit 6df2cdf44a82426f7a59dcb03f0dd2181ed7fdfa: > > Update version for v4.1.0-rc0 release (2019-07-09 17:21:53 +0100) > > are available in the Git repository at: > > https://github.com/juanquintela/qemu.git tags/migration-pull-request > > for you to fetch changes up to 0b47e79b3d04f500b6f3490628905ec5884133df: > > migration: allow private destination ram with x-ignore-shared (2019-07-11 > 12:30:40 +0200) > > > Migration pull request > > > [...] > include/exec/memory.h| 19 + > include/exec/memory.h.rej| 26 + > include/exec/ram_addr.h | 92 +- > include/exec/ram_addr.h.orig | 488 +++ [...] > migration/ram.c | 93 +- > migration/ram.c.orig | 4599 ++ > migration/ram.c.rej | 33 + The .ref and .orig look odd. And they are is not part of the patches.
Re: [Qemu-devel] [PATCH v2] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
On 11.07.19 11:24, Daniel P. Berrangé wrote:
> On Thu, Jul 11, 2019 at 10:02:01AM +0200, Christian Ehrhardt wrote:
>> On Mon, Jun 17, 2019 at 5:35 PM Laurent Vivier wrote:
>>>
>>> Le 17/06/2019 à 15:11, Daniel P. Berrangé a écrit :
The SIOCGSTAMP symbol was previously defined in the
asm-generic/sockios.h header file. QEMU sees that header
indirectly via sys/socket.h
In linux kernel commit 0768e17073dc527ccd18ed5f96ce85f9985e9115
the asm-generic/sockios.h header no longer defines SIOCGSTAMP.
Instead it provides only SIOCGSTAMP_OLD, which only uses a
32-bit time_t on 32-bit architectures.
The linux/sockios.h header then defines SIOCGSTAMP using
either SIOCGSTAMP_OLD or SIOCGSTAMP_NEW as appropriate. If
SIOCGSTAMP_NEW is used, then the tv_sec field is 64-bit even
on 32-bit architectures
To cope with this we must now define two separate syscalls,
with corresponding old and new sizes, as well as including
the new linux/sockios.h header.
Signed-off-by: Daniel P. Berrangé
---
linux-user/ioctls.h| 15 +++
linux-user/syscall.c | 1 +
linux-user/syscall_defs.h | 5 +
linux-user/syscall_types.h | 4
4 files changed, 25 insertions(+)
diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
index 5e84dc7c3a..5a6d6def7e 100644
--- a/linux-user/ioctls.h
+++ b/linux-user/ioctls.h
@@ -222,8 +222,23 @@
IOCTL(SIOCGIWNAME, IOC_W | IOC_R, MK_PTR(MK_STRUCT(STRUCT_char_ifreq)))
IOCTL(SIOCSPGRP, IOC_W, MK_PTR(TYPE_INT)) /* pid_t */
IOCTL(SIOCGPGRP, IOC_R, MK_PTR(TYPE_INT)) /* pid_t */
+
+#ifdef SIOCGSTAMP_OLD
+ IOCTL(SIOCGSTAMP_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval)))
+#else
IOCTL(SIOCGSTAMP, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval)))
+#endif
+#ifdef SIOCGSTAMPNS_OLD
+ IOCTL(SIOCGSTAMPNS_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec)))
+#else
IOCTL(SIOCGSTAMPNS, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec)))
+#endif
+#ifdef SIOCGSTAMP_NEW
+ IOCTL(SIOCGSTAMP_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval64)))
+#endif
+#ifdef SIOCGSTAMPNS_NEW
+ IOCTL(SIOCGSTAMPNS_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec64)))
+#endif
IOCTL(RNDGETENTCNT, IOC_R, MK_PTR(TYPE_INT))
IOCTL(RNDADDTOENTCNT, IOC_W, MK_PTR(TYPE_INT))
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index b187c1281d..f13e260b02 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -37,6 +37,7 @@
#include
#include
#include
+#include
#include
#include
#include
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 7f141f699c..7830b600e7 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -750,6 +750,11 @@ struct target_pollfd {
#define TARGET_SIOCGSTAMP 0x8906 /* Get stamp (timeval) */
#define TARGET_SIOCGSTAMPNS0x8907 /* Get stamp (timespec) */
+#define TARGET_SIOCGSTAMP_OLD 0x8906 /* Get stamp (timeval) */
+#define TARGET_SIOCGSTAMPNS_OLD 0x8907 /* Get stamp (timespec) */
+#define TARGET_SIOCGSTAMP_NEW TARGET_IOC(TARGET_IOC_READ, 's', 6,
sizeof(long long) + sizeof(long)) /* Get stamp (timeval64) */
+#define TARGET_SIOCGSTAMPNS_NEW TARGET_IOC(TARGET_IOC_READ, 's', 7,
sizeof(long long) + sizeof(long)) /* Get stamp (timespec64) */
>>> kernel defines:
>>>
>>> #define SIOCGSTAMP_NEW _IOR(SOCK_IOC_TYPE, 0x06, long long[2])
>>> #define SIOCGSTAMPNS_NEW _IOR(SOCK_IOC_TYPE, 0x07, long long[2])
>>>
>>> So it should be TARGET_IOR(0x89, 0x6, abi_llong[2])
>>>
>>> Their codes are 0x80108906 and 80108907.
>>
>> Hi,
>> I found the discussion around this topic being almost a month old.
>> And related to this fedora bug [1] was closed by adding [2] which
>> matches [3] that was nacked in the discussion here.
>>
>> Since I found nothing later (neither qemu commits nor further
>> discussions) I wonder if it has fallen through the cracks OR if there
>> was a kernel fix/change to resolve it (if that is the case a pointer
>> to the related kernel change would be nice)?
>
> I didn't have time to address the feedback to this v2, and since the
> immediate problem for Fedora has a workaround, its been lower priority
> especially since my understanding of linux-iser is limited.
>
> IOW, If anyone wants to take over my patch proposal here feel free. It
> would obviously be nice to fix for this 4.1 release if practical.
Same for me. I have never dealt with linux-user and my workaround was the
simplest thing that I could come up with. Would be good it Laurent or other
linux-user experts could do the "right thing".
Adding Peter, for your awareness. qemu does not build with newer kernel headers.
>
>>
>> [1]:
Re: [Qemu-devel] [PATCH] Makefile: Fix "make clean" in "unconfigured" source directory
On 09.07.19 16:38, Markus Armbruster wrote: > Recent commit "Makefile: Reuse all's recursion machinery for clean and > install" broke targets clean and distclean in the source directory > before running configure: > > $ make clean > LD recurse-clean.mo > cc: fatal error: no input files > compilation terminated. > make: *** [rules.mak:118: recurse-clean.mo] Error 1 > > Root cause is missing .PHONY. Fix that. > > Fixes: 1338a4b72659ce08eacb9de0205fe16202a22d9c > Reported-by: Christian Borntraeger > Signed-off-by: Markus Armbruster Tested-by: Christian Borntraeger > --- > Makefile | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Makefile b/Makefile > index c63de4e36c..1fcbaed62c 100644 > --- a/Makefile > +++ b/Makefile > @@ -518,6 +518,7 @@ ROM_DIRS_RULES=$(foreach t, all clean, $(addsuffix /$(t), > $(ROM_DIRS))) > $(ROM_DIRS_RULES): > $(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C $(dir $@) V="$(V)" > TARGET_DIR="$(dir $@)" CFLAGS="$(filter -O% -g%,$(CFLAGS))" $(notdir $@),) > > +.PHONY: recurse-all recurse-clean recurse-install > recurse-all: $(addsuffix /all, $(TARGET_DIRS) $(ROM_DIRS)) > recurse-clean: $(addsuffix /clean, $(TARGET_DIRS) $(ROM_DIRS)) > recurse-install: $(addsuffix /install, $(TARGET_DIRS)) >
[Qemu-devel] make distclean /clean does not work on fresh repository (was: Re: Makefile: Reuse all's recursion machinery for clean and install)
This is still broken with qemu/master. make clean/distclean no longer works on a fresh repository. As the make will return an error this can break any kind of scripts. On 05.07.19 23:31, Christian Borntraeger wrote: > This seems to break "make clean" and "make distclean" in the source directory > if there was never > a configure. > > qemu]$ make clean > LD recurse-clean.mo > cc: fatal error: no input files > compilation terminated. > make: *** [rules.mak:118: recurse-clean.mo] Error 1 > > > > On 02.07.19 13:34, Markus Armbruster wrote: >> Targets "clean" and "install" run make recursively in a for loop. >> This ignores -j and -k. Target "all" depends on SUBDIR/all to recurse >> into each SUBDIR. Behaves nicely with -j and -k. Put that to use for >> "clean" and "install": depend on SUBDIR/clean or SUBDIR/install, >> respectively, and delete the loop. >> >> Signed-off-by: Markus Armbruster >> Reviewed-by: Richard Henderson >> Message-Id: <20190528082308.22032-5-arm...@redhat.com> >> Reviewed-by: Philippe Mathieu-Daudé >> --- >> Makefile | 53 + >> 1 file changed, 25 insertions(+), 28 deletions(-) >> >> diff --git a/Makefile b/Makefile >> index e68982bd99..8cf6cbc4c4 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -454,20 +454,22 @@ config-host.h-timestamp: config-host.mak >> qemu-options.def: $(SRC_PATH)/qemu-options.hx $(SRC_PATH)/scripts/hxtool >> $(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > >> $@,"GEN","$@") >> >> -SUBDIR_RULES=$(addsuffix /all, $(TARGET_DIRS)) >> -SOFTMMU_SUBDIR_RULES=$(filter %-softmmu/all,$(SUBDIR_RULES)) >> +TARGET_DIRS_RULES := $(foreach t, all clean install, $(addsuffix /$(t), >> $(TARGET_DIRS))) >> >> -$(SOFTMMU_SUBDIR_RULES): $(authz-obj-y) >> -$(SOFTMMU_SUBDIR_RULES): $(block-obj-y) >> -$(SOFTMMU_SUBDIR_RULES): $(chardev-obj-y) >> -$(SOFTMMU_SUBDIR_RULES): $(crypto-obj-y) >> -$(SOFTMMU_SUBDIR_RULES): $(io-obj-y) >> -$(SOFTMMU_SUBDIR_RULES): config-all-devices.mak >> -$(SOFTMMU_SUBDIR_RULES): $(edk2-decompressed) >> +SOFTMMU_ALL_RULES=$(filter %-softmmu/all, $(TARGET_DIRS_RULES)) >> +$(SOFTMMU_ALL_RULES): $(authz-obj-y) >> +$(SOFTMMU_ALL_RULES): $(block-obj-y) >> +$(SOFTMMU_ALL_RULES): $(chardev-obj-y) >> +$(SOFTMMU_ALL_RULES): $(crypto-obj-y) >> +$(SOFTMMU_ALL_RULES): $(io-obj-y) >> +$(SOFTMMU_ALL_RULES): config-all-devices.mak >> +$(SOFTMMU_ALL_RULES): $(edk2-decompressed) >> >> -.PHONY: $(SUBDIR_RULES) >> -$(SUBDIR_RULES): >> -$(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C $(dir $@) V="$(V)" >> TARGET_DIR="$(dir $@)" all,) >> +.PHONY: $(TARGET_DIRS_RULES) >> +# The $(TARGET_DIRS_RULES) are of the form SUBDIR/GOAL, so that >> +# $(dir $@) yields the sub-directory, and $(notdir $@) yields the sub-goal >> +$(TARGET_DIRS_RULES): >> +$(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C $(dir $@) V="$(V)" >> TARGET_DIR="$(dir $@)" $(notdir $@),) >> >> DTC_MAKE_ARGS=-I$(SRC_PATH)/dtc VPATH=$(SRC_PATH)/dtc -C dtc V="$(V)" >> LIBFDT_srcdir=$(SRC_PATH)/dtc/libfdt >> DTC_CFLAGS=$(CFLAGS) $(QEMU_CFLAGS) >> @@ -500,19 +502,19 @@ capstone/all: .git-submodule-status >> slirp/all: .git-submodule-status >> $(call quiet-command,$(MAKE) -C $(SRC_PATH)/slirp >> BUILD_DIR="$(BUILD_DIR)/slirp" CC="$(CC)" AR="$(AR)" LD="$(LD)" >> RANLIB="$(RANLIB)" CFLAGS="$(QEMU_CFLAGS) $(CFLAGS)" LDFLAGS="$(LDFLAGS)") >> >> -$(SUBDIR_RULES): libqemuutil.a $(common-obj-y) \ >> +$(filter %/all, $(TARGET_DIRS_RULES)): libqemuutil.a $(common-obj-y) \ >> $(qom-obj-y) $(crypto-user-obj-$(CONFIG_USER_ONLY)) >> >> ROM_DIRS = $(addprefix pc-bios/, $(ROMS)) >> -ROMSUBDIR_RULES=$(addsuffix /all, $(ROM_DIRS)) >> +ROM_DIRS_RULES=$(foreach t, all clean, $(addsuffix /$(t), $(ROM_DIRS))) >> # Only keep -O and -g cflags >> -.PHONY: $(ROMSUBDIR_RULES) >> -$(ROMSUBDIR_RULES): >> -$(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C $(dir $@) V="$(V)" >> TARGET_DIR="$(dir $@)" CFLAGS="$(filter -O% -g%,$(CFLAGS))",) >> +.PHONY: $(ROM_DIRS_RULES) >> +$(ROM_DIRS_RULES): >> +$(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C $(dir $@) V="$(V)" >> TARGET_DIR="$(dir $@)" CFLAGS="$(filter -O% -g%,
[Qemu-devel] [PATCH v2] s390: cpumodel: fix description for the new vector facility
The new facility is called "Vector-Packed-Decimal-Enhancement Facility"
and not "Vector BCD enhancements facility 1". As the shortname might
have already found its way into some backports lets keep vxbeh.
Fixes: 54d65de0b525 ("s390x/cpumodel: vector enhancements")
Signed-off-by: Christian Borntraeger
---
target/s390x/cpu_features_def.inc.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/s390x/cpu_features_def.inc.h
b/target/s390x/cpu_features_def.inc.h
index ef190e2fc783..3118a9f89228 100644
--- a/target/s390x/cpu_features_def.inc.h
+++ b/target/s390x/cpu_features_def.inc.h
@@ -104,7 +104,7 @@ DEF_FEAT(CMM_NT, "cmmnt", STFL, 147, "CMM: ESSA-enhancement
(no translate) facil
DEF_FEAT(VECTOR_ENH2, "vxeh2", STFL, 148, "Vector Enhancements facility 2")
DEF_FEAT(ESORT_BASE, "esort-base", STFL, 150, "Enhanced-sort facility
(excluding subfunctions)")
DEF_FEAT(DEFLATE_BASE, "deflate-base", STFL, 151, "Deflate-conversion facility
(excluding subfunctions)")
-DEF_FEAT(VECTOR_BCD_ENH, "vxbeh", STFL, 152, "Vector BCD enhancements facility
1")
+DEF_FEAT(VECTOR_BCD_ENH, "vxbeh", STFL, 152,
"Vector-Packed-Decimal-Enhancement Facility")
DEF_FEAT(MSA_EXT_9, "msa9-base", STFL, 155,
"Message-security-assist-extension-9 facility (excluding subfunctions)")
DEF_FEAT(ETOKEN, "etoken", STFL, 156, "Etoken facility")
--
2.21.0
Re: [Qemu-devel] [PATCH] s390: cpumodel: fix name and description for the new vector facility
As this might have already sneaked into some distribution backports just fixing the description would be a "play-safe" variant. -DEF_FEAT(VECTOR_BCD_ENH, "vxbeh", STFL, 152, "Vector BCD enhancements facility 1") +DEF_FEAT(VECTOR_BCD_ENH, "vxbeh", STFL, 152, "Vector-Packed-Decimal-Enhancement Facility")
