| Message ID | 20240910175809.2135596-7-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: virtio-mem support | expand |
On 10/09/2024 19.58, David Hildenbrand wrote: > Let's add s390_get_memory_limit(), to query what has been successfully > set via s390_set_memory_limit(). Allow setting the limit only once. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/cpu-sysemu.c | 19 +++++++++++++++++-- > target/s390x/cpu.h | 1 + > 2 files changed, 18 insertions(+), 2 deletions(-) Reviewed-by: Thomas Huth <thuth@redhat.com>
On Tue, 2024-09-10 at 19:58 +0200, David Hildenbrand wrote: > Let's add s390_get_memory_limit(), to query what has been successfully > set via s390_set_memory_limit(). Allow setting the limit only once. > > Signed-off-by: David Hildenbrand <david@redhat.com> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> Comment below. > --- > target/s390x/cpu-sysemu.c | 19 +++++++++++++++++-- > target/s390x/cpu.h | 1 + > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c > index 1cd30c1d84..1915567b3a 100644 > --- a/target/s390x/cpu-sysemu.c > +++ b/target/s390x/cpu-sysemu.c > @@ -255,12 +255,27 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu) > return s390_count_running_cpus(); > } > > +static uint64_t memory_limit; > + > int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit) > { > + int ret = 0; > + > + if (memory_limit) { > + return -EBUSY; > + } > if (kvm_enabled()) { > - return kvm_s390_set_mem_limit(new_limit, hw_limit); > + ret = kvm_s390_set_mem_limit(new_limit, hw_limit); > + } > + if (!ret) { > + memory_limit = new_limit; > } > - return 0; > + return ret; > +} > + > +uint64_t s390_get_memory_limit(void) > +{ Might be nice to guard/warn against s390_set_memory_limit not having been called before. > + return memory_limit; > } > > void s390_set_max_pagesize(uint64_t pagesize, Error **errp) > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > index d6b75ad0e0..7a51b606ed 100644 > --- a/target/s390x/cpu.h > +++ b/target/s390x/cpu.h > @@ -895,6 +895,7 @@ static inline void s390_do_cpu_load_normal(CPUState *cs, run_on_cpu_data arg) > /* cpu.c */ > void s390_crypto_reset(void); > int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit); > +uint64_t s390_get_memory_limit(void); > void s390_set_max_pagesize(uint64_t pagesize, Error **errp); > void s390_cmma_reset(void); > void s390_enable_css_support(S390CPU *cpu);
On 16.09.24 15:20, Nina Schoetterl-Glausch wrote: > On Tue, 2024-09-10 at 19:58 +0200, David Hildenbrand wrote: >> Let's add s390_get_memory_limit(), to query what has been successfully >> set via s390_set_memory_limit(). Allow setting the limit only once. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> > > Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > Comment below. >> --- >> target/s390x/cpu-sysemu.c | 19 +++++++++++++++++-- >> target/s390x/cpu.h | 1 + >> 2 files changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c >> index 1cd30c1d84..1915567b3a 100644 >> --- a/target/s390x/cpu-sysemu.c >> +++ b/target/s390x/cpu-sysemu.c >> @@ -255,12 +255,27 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu) >> return s390_count_running_cpus(); >> } >> >> +static uint64_t memory_limit; >> + >> int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit) >> { >> + int ret = 0; >> + >> + if (memory_limit) { >> + return -EBUSY; >> + } >> if (kvm_enabled()) { >> - return kvm_s390_set_mem_limit(new_limit, hw_limit); >> + ret = kvm_s390_set_mem_limit(new_limit, hw_limit); >> + } >> + if (!ret) { >> + memory_limit = new_limit; >> } >> - return 0; >> + return ret; >> +} >> + >> +uint64_t s390_get_memory_limit(void) >> +{ > > Might be nice to guard/warn against s390_set_memory_limit not having been called before. > What about the following on top: diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c index 1915567b3a..07cb85103a 100644 --- a/target/s390x/cpu-sysemu.c +++ b/target/s390x/cpu-sysemu.c @@ -263,6 +263,8 @@ int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit) if (memory_limit) { return -EBUSY; + } else if (!new_limit) { + return -EINVAL; } if (kvm_enabled()) { ret = kvm_s390_set_mem_limit(new_limit, hw_limit); @@ -275,6 +277,8 @@ int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit) uint64_t s390_get_memory_limit(void) { + /* We expect to be called after s390_set_memory_limit(). */ + assert(memory_limit); return memory_limit; }
On Tue, 2024-09-17 at 13:23 +0200, David Hildenbrand wrote: > On 16.09.24 15:20, Nina Schoetterl-Glausch wrote: > > On Tue, 2024-09-10 at 19:58 +0200, David Hildenbrand wrote: > > > Let's add s390_get_memory_limit(), to query what has been successfully > > > set via s390_set_memory_limit(). Allow setting the limit only once. > > > > > > Signed-off-by: David Hildenbrand <david@redhat.com> > > > > Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > > > Comment below. > > > --- > > > target/s390x/cpu-sysemu.c | 19 +++++++++++++++++-- > > > target/s390x/cpu.h | 1 + > > > 2 files changed, 18 insertions(+), 2 deletions(-) [...] > > > + > > > +uint64_t s390_get_memory_limit(void) > > > +{ > > > > Might be nice to guard/warn against s390_set_memory_limit not having been called before. > > > > What about the following on top: > > diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c > index 1915567b3a..07cb85103a 100644 > --- a/target/s390x/cpu-sysemu.c > +++ b/target/s390x/cpu-sysemu.c > @@ -263,6 +263,8 @@ int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit) > > if (memory_limit) { > return -EBUSY; > + } else if (!new_limit) { > + return -EINVAL; > } > if (kvm_enabled()) { > ret = kvm_s390_set_mem_limit(new_limit, hw_limit); > @@ -275,6 +277,8 @@ int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit) > > uint64_t s390_get_memory_limit(void) > { > + /* We expect to be called after s390_set_memory_limit(). */ > + assert(memory_limit); > return memory_limit; > } Looks good. Looking at the patch again I'm wondering if using globals in qemu is still encouraged. I know it's a common pattern today, but seeing efforts like the multiarch binary or Unicorn I'm wondering if there is aspirations to do things more "cleanly", in general, for far out benefits? I.e. memory_limit could be a machine property instead.
> > Looks good. > > Looking at the patch again I'm wondering if using globals in qemu is still encouraged. > I know it's a common pattern today, but seeing efforts like the multiarch binary or Unicorn > I'm wondering if there is aspirations to do things more "cleanly", in general, for far out benefits? > I.e. memory_limit could be a machine property instead. Yes, I'll rework that code to simply store it in the machine, moving that code out of cpu-sysemu.c int s390-virtio-ccw.c. Same for patch #12, thanks!
diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c index 1cd30c1d84..1915567b3a 100644 --- a/target/s390x/cpu-sysemu.c +++ b/target/s390x/cpu-sysemu.c @@ -255,12 +255,27 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu) return s390_count_running_cpus(); } +static uint64_t memory_limit; + int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit) { + int ret = 0; + + if (memory_limit) { + return -EBUSY; + } if (kvm_enabled()) { - return kvm_s390_set_mem_limit(new_limit, hw_limit); + ret = kvm_s390_set_mem_limit(new_limit, hw_limit); + } + if (!ret) { + memory_limit = new_limit; } - return 0; + return ret; +} + +uint64_t s390_get_memory_limit(void) +{ + return memory_limit; } void s390_set_max_pagesize(uint64_t pagesize, Error **errp) diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index d6b75ad0e0..7a51b606ed 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -895,6 +895,7 @@ static inline void s390_do_cpu_load_normal(CPUState *cs, run_on_cpu_data arg) /* cpu.c */ void s390_crypto_reset(void); int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit); +uint64_t s390_get_memory_limit(void); void s390_set_max_pagesize(uint64_t pagesize, Error **errp); void s390_cmma_reset(void); void s390_enable_css_support(S390CPU *cpu);
Let's add s390_get_memory_limit(), to query what has been successfully set via s390_set_memory_limit(). Allow setting the limit only once. Signed-off-by: David Hildenbrand <david@redhat.com> --- target/s390x/cpu-sysemu.c | 19 +++++++++++++++++-- target/s390x/cpu.h | 1 + 2 files changed, 18 insertions(+), 2 deletions(-)