Re: [PATCH v1 06/14] s390x: introduce s390_get_memory_limit()

[Nina Schoetterl-Glausch]

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.