RE: [PATCH v4 04/20] xen: introduce CONFIG_SYSCTL

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

Hi

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, June 10, 2025 9:05 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Stabellini, Stefano
> <stefano.stabellini@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>;
> Anthony PERARD <anthony.perard@xxxxxxxxxx>; Orzel, Michal
> <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Roger Pau Monné
> <roger.pau@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Sergiy 
> Kibrik
> <Sergiy_Kibrik@xxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4 04/20] xen: introduce CONFIG_SYSCTL
>
> On 28.05.2025 11:16, Penny Zheng wrote:
> > From: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> >
> > We introduce a new Kconfig CONFIG_SYSCTL, which shall only be disabled
> > on some dom0less systems or PV shim on x86, to reduce Xen footprint.
> >
> > Making SYSCTL without prompt is transient and it will be fixed in the
> > final
>
> Nit: s/fixed/adjusted/ ? It's not a bug, after all.
>

Understood.

> > patch. Also, we will also state unsetting SYSCTL in pvshim_defconfig
> > to explicitly make it unavailable for PV shim in the final patch.
>
> Even without the double "also" this reads odd. But it's also unclear what it 
> has to do
> here, nor whether what is being said is actually correct.
>

Hmmm, How about  "
The consequences of introducing "CONFIG_SYSCTL=y" in .config file generated 
from pvshim_defconfig
is transient and will be also fixed in the final."

> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -579,4 +579,15 @@ config BUDDY_ALLOCATOR_SIZE
> >       Amount of memory reserved for the buddy allocator to serve Xen heap,
> >       working alongside the colored one.
> >
> > +menu "Supported hypercall interfaces"
> > +   visible if EXPERT
> > +
> > +config SYSCTL
> > +   bool "Enable sysctl hypercall"
> > +   def_bool y
>
> Why def_bool when you already have bool on the earlier line?
>

Ack, then here maybe a simple
"
config SYSCTL
        def_bool y
"
 is enough.

> > +   help
> > +     This option shall only be disabled on some dom0less systems,
> > +     to reduce Xen footprint.
>
> This isn't overly useful (but possibly misleading) as long as the prompt 
> isn#t going
> to be visible, yet.
>

Understood, I'll remove the description here and add it in the final when 
prompt is visible

> > +endmenu
>
> Blank line please ahead of this one.
>

Ack

> Jan