[Public]
Hi,
Sorry for the late reply, got side-tracked for a while
> -----Original Message-----
> From: Alejandro Vallejo <alejandro.vall...@cloud.com>
> Sent: Thursday, March 13, 2025 8:03 PM
> To: Penny, Zheng <penny.zh...@amd.com>; xen-devel@lists.xenproject.org
> Cc: Huang, Ray <ray.hu...@amd.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Anthony PERARD <anthony.per...@vates.tech>;
> Orzel, Michal <michal.or...@amd.com>; Jan Beulich <jbeul...@suse.com>;
> Julien Grall <jul...@xen.org>; Roger Pau Monné <roger....@citrix.com>; Stefano
> Stabellini <sstabell...@kernel.org>; Daniel P. Smith
> <dpsm...@apertussolutions.com>
> Subject: Re: [PATCH v1 03/19] xen/sysctl: wrap around
> XEN_SYSCTL_readconsole
>
> Hi,
>
> Ok, so readconsole is done here. I see how if you're also removing the console
> handler for the sysctl that's a bit unwiledly to do in one go.
>
> I think my earlier remarks still hold in terms of removal of else branches of
> ifdefs.
>
> On Wed Mar 12, 2025 at 4:06 AM GMT, Penny Zheng wrote:
> > The following functions is to deal with XEN_SYSCTL_readconsole sub-op,
> > and shall be wrapped:
> > - xsm_readconsole
> > - read_console_ring
> >
> > Signed-off-by: Penny Zheng <penny.zh...@amd.com>
> > ---
> > xen/drivers/char/console.c | 2 ++
> > xen/include/xen/console.h | 8 ++++++++
> > xen/include/xsm/dummy.h | 11 ++++++++---
> > xen/include/xsm/xsm.h | 11 ++++++++---
> > xen/xsm/dummy.c | 2 +-
> > xen/xsm/flask/hooks.c | 4 ++--
> > 6 files changed, 29 insertions(+), 9 deletions(-)
> >
> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > index 2f028c5d44..6e4f3c4659 100644
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -336,6 +336,7 @@ static void conring_puts(const char *str, size_t len)
> > conringc = conringp - conring_size; }
> >
> > +#ifdef CONFIG_SYSCTL
> > long read_console_ring(struct xen_sysctl_readconsole *op) {
> > XEN_GUEST_HANDLE_PARAM(char) str; @@ -378,6 +379,7 @@ long
> > read_console_ring(struct xen_sysctl_readconsole *op)
> >
> > return 0;
> > }
> > +#endif /* CONFIG_SYSCTL */
> >
> >
> > /*
> > diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
> > index 83cbc9fbda..e7d5063d82 100644
> > --- a/xen/include/xen/console.h
> > +++ b/xen/include/xen/console.h
> > @@ -7,12 +7,20 @@
> > #ifndef __CONSOLE_H__
> > #define __CONSOLE_H__
> >
> > +#include <xen/errno.h>
> > #include <xen/inttypes.h>
> > #include <xen/ctype.h>
> > #include <public/xen.h>
> >
> > struct xen_sysctl_readconsole;
>
> That forward declaration should probably be inside the ifdef
>
> > +#ifdef CONFIG_SYSCTL
> > long read_console_ring(struct xen_sysctl_readconsole *op);
> > +#else
> > +static inline long read_console_ring(struct xen_sysctl_readconsole
> > +*op) {
> > + return -EOPNOTSUPP;
> > +}
> > +#endif
>
> This is only called from sysctl.c, which will be compiled out. Why is the else
> needed?
>
Because I wrapped the sysctl.c in the last commit.
If removing the else condition here, the compilation will fail on this commit.
So either I add #ifdef into read_console_ring function body, or in the last
commit,
I draw back all these unnecessary else conditions, or combine all commits into
one
Any preference? Or any other suggestion?
> >
> > void console_init_preirq(void);
> > void console_init_ring(void);
> > diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index
> > afc54a0b2f..35d084aca7 100644
> > --- a/xen/include/xsm/dummy.h
> > +++ b/xen/include/xsm/dummy.h
> > @@ -186,18 +186,23 @@ static XSM_INLINE int cf_check
> xsm_sysctl(XSM_DEFAULT_ARG int cmd)
> > XSM_ASSERT_ACTION(XSM_PRIV);
> > return xsm_default_action(action, current->domain, NULL); }
> > +
> > +static XSM_INLINE int cf_check xsm_readconsole(XSM_DEFAULT_ARG
> > +uint32_t clear) {
> > + XSM_ASSERT_ACTION(XSM_HOOK);
> > + return xsm_default_action(action, current->domain, NULL); }
> > #else
> > static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd) {
> > return -EOPNOTSUPP;
> > }
> > -#endif
> >
> > static XSM_INLINE int cf_check xsm_readconsole(XSM_DEFAULT_ARG
> > uint32_t clear) {
> > - XSM_ASSERT_ACTION(XSM_HOOK);
> > - return xsm_default_action(action, current->domain, NULL);
> > + return -EOPNOTSUPP;
> > }
> > +#endif /* CONFIG_SYSCTL */
> >
> > static XSM_INLINE int cf_check xsm_alloc_security_domain(struct
> > domain *d) { diff --git a/xen/include/xsm/xsm.h
> > b/xen/include/xsm/xsm.h index 276507b515..d322740de1 100644
> > --- a/xen/include/xsm/xsm.h
> > +++ b/xen/include/xsm/xsm.h
> > @@ -62,8 +62,8 @@ struct xsm_ops {
> > int (*domctl)(struct domain *d, unsigned int cmd, uint32_t
> > ssidref); #ifdef CONFIG_SYSCTL
> > int (*sysctl)(int cmd);
> > -#endif
> > int (*readconsole)(uint32_t clear);
> > +#endif
> >
> > int (*evtchn_unbound)(struct domain *d, struct evtchn *chn, domid_t
> > id2);
> > int (*evtchn_interdomain)(struct domain *d1, struct evtchn *chn1,
> > @@ -266,17 +266,22 @@ static inline int xsm_sysctl(xsm_default_t def,
> > int cmd) {
> > return alternative_call(xsm_ops.sysctl, cmd); }
> > +
> > +static inline int xsm_readconsole(xsm_default_t def, uint32_t clear)
> > +{
> > + return alternative_call(xsm_ops.readconsole, clear); }
> > #else
> > static inline int xsm_sysctl(xsm_default_t def, int cmd) {
> > return -EOPNOTSUPP;
> > }
> > -#endif
> >
> > static inline int xsm_readconsole(xsm_default_t def, uint32_t clear)
> > {
> > - return alternative_call(xsm_ops.readconsole, clear);
> > + return -EOPNOTSUPP;
> > }
> > +#endif
> >
> > static inline int xsm_evtchn_unbound(
> > xsm_default_t def, struct domain *d1, struct evtchn *chn, domid_t
> > id2) diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c index
> > 0a5fc06bbf..4c97db0c48 100644
> > --- a/xen/xsm/dummy.c
> > +++ b/xen/xsm/dummy.c
> > @@ -24,8 +24,8 @@ static const struct xsm_ops __initconst_cf_clobber
> dummy_ops = {
> > .domctl = xsm_domctl,
> > #ifdef CONFIG_SYSCTL
> > .sysctl = xsm_sysctl,
> > -#endif
> > .readconsole = xsm_readconsole,
> > +#endif
> >
> > .evtchn_unbound = xsm_evtchn_unbound,
> > .evtchn_interdomain = xsm_evtchn_interdomain,
> > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index
> > 7c5e7f5879..7c46657d97 100644
> > --- a/xen/xsm/flask/hooks.c
> > +++ b/xen/xsm/flask/hooks.c
> > @@ -934,7 +934,6 @@ static int cf_check flask_sysctl(int cmd)
> > return avc_unknown_permission("sysctl", cmd);
> > }
> > }
> > -#endif
> >
> > static int cf_check flask_readconsole(uint32_t clear) { @@ -945,6
> > +944,7 @@ static int cf_check flask_readconsole(uint32_t clear)
> >
> > return domain_has_xen(current->domain, perms); }
> > +#endif /* CONFIG_SYSCTL */
> >
> > static inline uint32_t resource_to_perm(uint8_t access) { @@ -1888,8
> > +1888,8 @@ static const struct xsm_ops __initconst_cf_clobber flask_ops = {
> > .domctl = flask_domctl,
> > #ifdef CONFIG_SYSCTL
> > .sysctl = flask_sysctl,
> > -#endif
> > .readconsole = flask_readconsole,
> > +#endif
> >
> > .evtchn_unbound = flask_evtchn_unbound,
> > .evtchn_interdomain = flask_evtchn_interdomain,
>
> Otherwise, same remarks as in the sysctl hooks for xsm.
>
> Cheers,
> Alejandro
