Re: [PATCH] drivers/xen/hypervisor: Expose VM SIF flags to userspace

[Andrew Cooper <Andrew.Cooper3@citrix.com>]

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