| Message ID | 20221129150058.266943-1-per.bilse@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drivers/xen/hypervisor: Expose VM SIF flags to userspace | expand |
On 29.11.22 16:00, Per Bilse wrote: > /proc/xen is a legacy pseudo filesystem which predates Xen support > getting merged into Linux. It has largely been replaced with more > normal locations for data (/sys/hypervisor/ for info, /dev/xen/ for > user devices). We want to compile xenfs support out of the dom0 kernel. > > There is one item which only exists in /proc/xen, namely > /proc/xen/capabilities with "control_d" being the signal of "you're in > the control domain". This ultimately comes from the SIF flags provided > at VM start. > > This patch exposes all SIF flags in /sys/hypervisor/properties/flags, > which will coexist with /proc/xen while dependencies are being migrated. > Possible values are "privileged", "initdomain", "multiboot", > "mod_start_pfn", and "virtmap", with "initdomain" being the equivalent > of "control_d". > > Signed-off-by: Per Bilse <per.bilse@citrix.com> > --- > drivers/xen/sys-hypervisor.c | 26 ++++++++++++++++++++++++++ > include/xen/interface/xen.h | 13 ++++++++----- > 2 files changed, 34 insertions(+), 5 deletions(-) > > diff --git a/drivers/xen/sys-hypervisor.c b/drivers/xen/sys-hypervisor.c > index fcb0792f090e..7393e04bdb6d 100644 > --- a/drivers/xen/sys-hypervisor.c > +++ b/drivers/xen/sys-hypervisor.c > @@ -379,6 +379,31 @@ static ssize_t buildid_show(struct hyp_sysfs_attr *attr, char *buffer) > > HYPERVISOR_ATTR_RO(buildid); > > +static ssize_t flags_show(struct hyp_sysfs_attr *attr, char *buffer) > +{ > + static char const *const sifstr[SIFN_NUM_SIFN] = { > + [SIFN_PRIV] = "privileged", > + [SIFN_INIT] = "initdomain", > + [SIFN_MULTI] = "multiboot", > + [SIFN_PFN] = "mod_start_pfn", > + [SIFN_VIRT] = "virtmap" > + }; > + unsigned sifnum, sifmask; > + ssize_t ret = 0; > + > + sifmask = ~(~0U << SIFN_NUM_SIFN); // ...0000111... > + if (xen_domain() && (xen_start_flags & sifmask) != 0) { > + for (sifnum = 0; sifnum != SIFN_NUM_SIFN; sifnum++) { > + if ((xen_start_flags & (1<<sifnum)) != 0) > + ret += sprintf(buffer+ret, "%s ", sifstr[sifnum]); > + } > + buffer[ret-1] = '\n'; > + } > + return ret; > +} > + > +HYPERVISOR_ATTR_RO(flags); > + > static struct attribute *xen_properties_attrs[] = { > &capabilities_attr.attr, > &changeset_attr.attr, > @@ -386,6 +411,7 @@ static struct attribute *xen_properties_attrs[] = { > &pagesize_attr.attr, > &features_attr.attr, > &buildid_attr.attr, > + &flags_attr.attr, > NULL > }; > > diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h > index 0ca23eca2a9c..762a348abe3e 100644 > --- a/include/xen/interface/xen.h > +++ b/include/xen/interface/xen.h > @@ -648,11 +648,14 @@ struct start_info { > }; > > /* These flags are passed in the 'flags' field of start_info_t. */ > -#define SIF_PRIVILEGED (1<<0) /* Is the domain privileged? */ > -#define SIF_INITDOMAIN (1<<1) /* Is this the initial control domain? */ > -#define SIF_MULTIBOOT_MOD (1<<2) /* Is mod_start a multiboot module? */ > -#define SIF_MOD_START_PFN (1<<3) /* Is mod_start a PFN? */ > -#define SIF_VIRT_P2M_4TOOLS (1<<4) /* Do Xen tools understand a virt. mapped */ > +/* Text strings are printed out in sys-hypervisor.c, we guard */ > +/* against mix-ups and errors by enumerating the flags. */ > +enum { SIFN_PRIV, SIFN_INIT, SIFN_MULTI, SIFN_PFN, SIFN_VIRT, SIFN_NUM_SIFN }; > +#define SIF_PRIVILEGED (1<<SIFN_PRIV) /* Is the domain privileged? */ > +#define SIF_INITDOMAIN (1<<SIFN_INIT) /* Is this the initial control domain? */ > +#define SIF_MULTIBOOT_MOD (1<<SIFN_MULTI) /* Is mod_start a multiboot module? */ > +#define SIF_MOD_START_PFN (1<<SIFN_PFN) /* Is mod_start a PFN? */ > +#define SIF_VIRT_P2M_4TOOLS (1<<SIFN_VIRT) /* Do Xen tools understand a virt. mapped */ Please don't change this header, as it is based on its master located in the Xen repository. An acceptable solution would be to send a Xen patch first doing the xen.h changes, and when that patch has been taken to modify the related Linux header accordingly. In case you want to go that route, please add a "XEN_" prefix to the enum members. Juergen
On 11/29/22 10:00 AM, Per Bilse wrote: > > +static ssize_t flags_show(struct hyp_sysfs_attr *attr, char *buffer) > +{ > + static char const *const sifstr[SIFN_NUM_SIFN] = { > + [SIFN_PRIV] = "privileged", > + [SIFN_INIT] = "initdomain", > + [SIFN_MULTI] = "multiboot", > + [SIFN_PFN] = "mod_start_pfn", > + [SIFN_VIRT] = "virtmap" > + }; > + unsigned sifnum, sifmask; > + ssize_t ret = 0; > + > + sifmask = ~(~0U << SIFN_NUM_SIFN); // ...0000111... > + if (xen_domain() && (xen_start_flags & sifmask) != 0) { > + for (sifnum = 0; sifnum != SIFN_NUM_SIFN; sifnum++) { > + if ((xen_start_flags & (1<<sifnum)) != 0) > + ret += sprintf(buffer+ret, "%s ", sifstr[sifnum]); > + } > + buffer[ret-1] = '\n'; > + } > + return ret; > +} Why not simply show unprocessed xen_start_flags as a hex value? -boris
On 29/11/2022 15:27, Juergen Gross wrote: > On 29.11.22 16:00, Per Bilse wrote: >> /proc/xen is a legacy pseudo filesystem which predates Xen support >> getting merged into Linux. It has largely been replaced with more >> normal locations for data (/sys/hypervisor/ for info, /dev/xen/ for >> user devices). We want to compile xenfs support out of the dom0 kernel. >> >> There is one item which only exists in /proc/xen, namely >> /proc/xen/capabilities with "control_d" being the signal of "you're in >> the control domain". This ultimately comes from the SIF flags provided >> at VM start. >> >> This patch exposes all SIF flags in /sys/hypervisor/properties/flags, >> which will coexist with /proc/xen while dependencies are being migrated. >> Possible values are "privileged", "initdomain", "multiboot", >> "mod_start_pfn", and "virtmap", with "initdomain" being the equivalent >> of "control_d". >> >> Signed-off-by: Per Bilse <per.bilse@citrix.com> >> --- >> drivers/xen/sys-hypervisor.c | 26 ++++++++++++++++++++++++++ >> include/xen/interface/xen.h | 13 ++++++++----- >> 2 files changed, 34 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/xen/sys-hypervisor.c b/drivers/xen/sys-hypervisor.c >> index fcb0792f090e..7393e04bdb6d 100644 >> --- a/drivers/xen/sys-hypervisor.c >> +++ b/drivers/xen/sys-hypervisor.c >> @@ -379,6 +379,31 @@ static ssize_t buildid_show(struct >> hyp_sysfs_attr *attr, char *buffer) >> HYPERVISOR_ATTR_RO(buildid); >> +static ssize_t flags_show(struct hyp_sysfs_attr *attr, char *buffer) >> +{ >> + static char const *const sifstr[SIFN_NUM_SIFN] = { >> + [SIFN_PRIV] = "privileged", >> + [SIFN_INIT] = "initdomain", >> + [SIFN_MULTI] = "multiboot", >> + [SIFN_PFN] = "mod_start_pfn", >> + [SIFN_VIRT] = "virtmap" >> + }; >> + unsigned sifnum, sifmask; >> + ssize_t ret = 0; >> + >> + sifmask = ~(~0U << SIFN_NUM_SIFN); // ...0000111... >> + if (xen_domain() && (xen_start_flags & sifmask) != 0) { >> + for (sifnum = 0; sifnum != SIFN_NUM_SIFN; sifnum++) { >> + if ((xen_start_flags & (1<<sifnum)) != 0) >> + ret += sprintf(buffer+ret, "%s ", sifstr[sifnum]); >> + } >> + buffer[ret-1] = '\n'; >> + } >> + return ret; >> +} >> + >> +HYPERVISOR_ATTR_RO(flags); >> + >> static struct attribute *xen_properties_attrs[] = { >> &capabilities_attr.attr, >> &changeset_attr.attr, >> @@ -386,6 +411,7 @@ static struct attribute *xen_properties_attrs[] = { >> &pagesize_attr.attr, >> &features_attr.attr, >> &buildid_attr.attr, >> + &flags_attr.attr, >> NULL >> }; >> diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h >> index 0ca23eca2a9c..762a348abe3e 100644 >> --- a/include/xen/interface/xen.h >> +++ b/include/xen/interface/xen.h >> @@ -648,11 +648,14 @@ struct start_info { >> }; >> /* These flags are passed in the 'flags' field of start_info_t. */ >> -#define SIF_PRIVILEGED (1<<0) /* Is the domain privileged? */ >> -#define SIF_INITDOMAIN (1<<1) /* Is this the initial control >> domain? */ >> -#define SIF_MULTIBOOT_MOD (1<<2) /* Is mod_start a multiboot >> module? */ >> -#define SIF_MOD_START_PFN (1<<3) /* Is mod_start a PFN? */ >> -#define SIF_VIRT_P2M_4TOOLS (1<<4) /* Do Xen tools understand a >> virt. mapped */ >> +/* Text strings are printed out in sys-hypervisor.c, we guard */ >> +/* against mix-ups and errors by enumerating the flags. */ >> +enum { SIFN_PRIV, SIFN_INIT, SIFN_MULTI, SIFN_PFN, SIFN_VIRT, >> SIFN_NUM_SIFN }; >> +#define SIF_PRIVILEGED (1<<SIFN_PRIV) /* Is the domain >> privileged? */ >> +#define SIF_INITDOMAIN (1<<SIFN_INIT) /* Is this the initial >> control domain? */ >> +#define SIF_MULTIBOOT_MOD (1<<SIFN_MULTI) /* Is mod_start a >> multiboot module? */ >> +#define SIF_MOD_START_PFN (1<<SIFN_PFN) /* Is mod_start a PFN? */ >> +#define SIF_VIRT_P2M_4TOOLS (1<<SIFN_VIRT) /* Do Xen tools >> understand a virt. mapped */ > > Please don't change this header, as it is based on its master > located in the Xen repository. > > An acceptable solution would be to send a Xen patch first doing the > xen.h changes, and when that patch has been taken to modify the > related Linux header accordingly. > > In case you want to go that route, please add a "XEN_" prefix to the > enum members. You can't use enums in the public headers because they have implementation defined behaviour (including the constants themselves). Also, you cant just rename them to XEN because the API is stable. For Linux, just use ilog2() when constructing the table, and don't modify the header at all. As for the actual flags exposed, it would be very beneficial not to copy the exist proc interface. It would be better to expose a subdir that had files containing booleans, because that also gives userspace an easy way to figure out if the particular flag is known to Linux, independently of whether the flag is set for a specific VM. Also, I think you only care about exposing PRIV and INITDOM. MULTIBOOT_MOD, START_PFN and VIRT_P2M_4TOOLS are all details specific to the kernel itself, and not relevant for userspace to know. You also can't make any calculation SIFN_NUM_SIFN because this version of Linux needs to support running on a newer Xen when more flags have been specified. Options are to either ignore unknown bits, or (slightly more helpful) expose them as "unknown$BIT" boolean files. Finally, any changes to the sys ABI needs to patch Documentation/ABI/stable/sysfs-hypervisor-xen ~Andrew
On 29/11/2022 16:29, Boris Ostrovsky wrote:
> Why not simply show unprocessed xen_start_flags as a hex value?
Providing a text representation follows what is currently the case:
[root@lcy2-dt128 /proc/xen]# cat capabilities
control_d
[root@lcy2-dt128 /proc/xen]#
The migrated form would yield:
root@dt90:/sys/hypervisor/properties# cat flags
privileged initdomain mod_start_pfn
root@dt90:/sys/hypervisor/properties#
There is good precedence for being more helpful than printing hex
values, but I have no objections if there is consensus that a hex value
is better.
Best,
-- Per
On 29/11/2022 16:41, Andrew Cooper wrote: > As for the actual flags exposed, it would be very beneficial not to copy > the exist proc interface. It would be better to expose a subdir that > had files containing booleans, because that also gives userspace an easy > way to figure out if the particular flag is known to Linux, > independently of whether the flag is set for a specific VM. OK, I'll do that instead; I thought it was the single "control_d" that shouldn't be perpetuated. Best, -- Per
diff --git a/drivers/xen/sys-hypervisor.c b/drivers/xen/sys-hypervisor.c index fcb0792f090e..7393e04bdb6d 100644 --- a/drivers/xen/sys-hypervisor.c +++ b/drivers/xen/sys-hypervisor.c @@ -379,6 +379,31 @@ static ssize_t buildid_show(struct hyp_sysfs_attr *attr, char *buffer) HYPERVISOR_ATTR_RO(buildid); +static ssize_t flags_show(struct hyp_sysfs_attr *attr, char *buffer) +{ + static char const *const sifstr[SIFN_NUM_SIFN] = { + [SIFN_PRIV] = "privileged", + [SIFN_INIT] = "initdomain", + [SIFN_MULTI] = "multiboot", + [SIFN_PFN] = "mod_start_pfn", + [SIFN_VIRT] = "virtmap" + }; + unsigned sifnum, sifmask; + ssize_t ret = 0; + + sifmask = ~(~0U << SIFN_NUM_SIFN); // ...0000111... + if (xen_domain() && (xen_start_flags & sifmask) != 0) { + for (sifnum = 0; sifnum != SIFN_NUM_SIFN; sifnum++) { + if ((xen_start_flags & (1<<sifnum)) != 0) + ret += sprintf(buffer+ret, "%s ", sifstr[sifnum]); + } + buffer[ret-1] = '\n'; + } + return ret; +} + +HYPERVISOR_ATTR_RO(flags); + static struct attribute *xen_properties_attrs[] = { &capabilities_attr.attr, &changeset_attr.attr, @@ -386,6 +411,7 @@ static struct attribute *xen_properties_attrs[] = { &pagesize_attr.attr, &features_attr.attr, &buildid_attr.attr, + &flags_attr.attr, NULL }; diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h index 0ca23eca2a9c..762a348abe3e 100644 --- a/include/xen/interface/xen.h +++ b/include/xen/interface/xen.h @@ -648,11 +648,14 @@ struct start_info { }; /* These flags are passed in the 'flags' field of start_info_t. */ -#define SIF_PRIVILEGED (1<<0) /* Is the domain privileged? */ -#define SIF_INITDOMAIN (1<<1) /* Is this the initial control domain? */ -#define SIF_MULTIBOOT_MOD (1<<2) /* Is mod_start a multiboot module? */ -#define SIF_MOD_START_PFN (1<<3) /* Is mod_start a PFN? */ -#define SIF_VIRT_P2M_4TOOLS (1<<4) /* Do Xen tools understand a virt. mapped */ +/* Text strings are printed out in sys-hypervisor.c, we guard */ +/* against mix-ups and errors by enumerating the flags. */ +enum { SIFN_PRIV, SIFN_INIT, SIFN_MULTI, SIFN_PFN, SIFN_VIRT, SIFN_NUM_SIFN }; +#define SIF_PRIVILEGED (1<<SIFN_PRIV) /* Is the domain privileged? */ +#define SIF_INITDOMAIN (1<<SIFN_INIT) /* Is this the initial control domain? */ +#define SIF_MULTIBOOT_MOD (1<<SIFN_MULTI) /* Is mod_start a multiboot module? */ +#define SIF_MOD_START_PFN (1<<SIFN_PFN) /* Is mod_start a PFN? */ +#define SIF_VIRT_P2M_4TOOLS (1<<SIFN_VIRT) /* Do Xen tools understand a virt. mapped */ /* P->M making the 3 level tree obsolete? */ #define SIF_PM_MASK (0xFF<<8) /* reserve 1 byte for xen-pm options */
/proc/xen is a legacy pseudo filesystem which predates Xen support getting merged into Linux. It has largely been replaced with more normal locations for data (/sys/hypervisor/ for info, /dev/xen/ for user devices). We want to compile xenfs support out of the dom0 kernel. There is one item which only exists in /proc/xen, namely /proc/xen/capabilities with "control_d" being the signal of "you're in the control domain". This ultimately comes from the SIF flags provided at VM start. This patch exposes all SIF flags in /sys/hypervisor/properties/flags, which will coexist with /proc/xen while dependencies are being migrated. Possible values are "privileged", "initdomain", "multiboot", "mod_start_pfn", and "virtmap", with "initdomain" being the equivalent of "control_d". Signed-off-by: Per Bilse <per.bilse@citrix.com> --- drivers/xen/sys-hypervisor.c | 26 ++++++++++++++++++++++++++ include/xen/interface/xen.h | 13 ++++++++----- 2 files changed, 34 insertions(+), 5 deletions(-)