| Message ID | 20200402154616.16927-9-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add hypervisor sysfs-like support | expand |
On 02.04.2020 17:46, Juergen Gross wrote: > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -353,6 +353,16 @@ config DOM0_MEM > > Leave empty if you are not sure what to specify. > > +config HYPFS_CONFIG > + bool "Provide hypervisor .config via hypfs entry" > + default y My initial reaction was to ask for "depends on HYPFS", but then I noticed the earlier patch doesn't introduce such. Am I mis-remembering that it was agreed to make the whole thing possible to disable at least in EXPERT mode? > --- a/xen/common/kernel.c > +++ b/xen/common/kernel.c > @@ -389,6 +389,16 @@ static HYPFS_STRING_INIT(compile_date, "compile_date"); > static HYPFS_STRING_INIT(compile_domain, "compile_domain"); > static HYPFS_STRING_INIT(extra, "extra"); > > +#ifdef CONFIG_HYPFS_CONFIG > +static struct hypfs_entry_leaf config = { > + .e.type = XEN_HYPFS_TYPE_STRING, > + .e.encoding = XEN_HYPFS_ENC_GZIP, > + .e.name = "config", > + .e.read = hypfs_read_leaf, > + .content = &xen_config_data > +}; > +#endif Would be really good if no open-coded instantiations like this one would ever have to appear, i.e. if suitable macros were available. What's preventing use of one of the available ones here? Jan
On 03.04.20 16:31, Jan Beulich wrote: > On 02.04.2020 17:46, Juergen Gross wrote: >> --- a/xen/common/Kconfig >> +++ b/xen/common/Kconfig >> @@ -353,6 +353,16 @@ config DOM0_MEM >> >> Leave empty if you are not sure what to specify. >> >> +config HYPFS_CONFIG >> + bool "Provide hypervisor .config via hypfs entry" >> + default y > > My initial reaction was to ask for "depends on HYPFS", but then > I noticed the earlier patch doesn't introduce such. Am I > mis-remembering that it was agreed to make the whole thing > possible to disable at least in EXPERT mode? No, I don't remember that agreement. And TBH I'm not sure this is a good idea, as that would at once make the plan to replace at least some sysctl and/or domctl interfaces impossible (like e.g. the last 3 patches of the series are doing already), or at least would tie the related functionality to CONFIG_HYPFS. > >> --- a/xen/common/kernel.c >> +++ b/xen/common/kernel.c >> @@ -389,6 +389,16 @@ static HYPFS_STRING_INIT(compile_date, "compile_date"); >> static HYPFS_STRING_INIT(compile_domain, "compile_domain"); >> static HYPFS_STRING_INIT(extra, "extra"); >> >> +#ifdef CONFIG_HYPFS_CONFIG >> +static struct hypfs_entry_leaf config = { >> + .e.type = XEN_HYPFS_TYPE_STRING, >> + .e.encoding = XEN_HYPFS_ENC_GZIP, >> + .e.name = "config", >> + .e.read = hypfs_read_leaf, >> + .content = &xen_config_data >> +}; >> +#endif > > Would be really good if no open-coded instantiations like this > one would ever have to appear, i.e. if suitable macros were > available. What's preventing use of one of the available ones > here? Right now it is the only case for a non plain encoding. The alternative would have been to either specify the encoding explicitly at all other node definitions, or to have a macro for this single instance. Juergen
On 03.04.2020 17:12, Jürgen Groß wrote: > On 03.04.20 16:31, Jan Beulich wrote: >> On 02.04.2020 17:46, Juergen Gross wrote: >>> --- a/xen/common/Kconfig >>> +++ b/xen/common/Kconfig >>> @@ -353,6 +353,16 @@ config DOM0_MEM >>> Leave empty if you are not sure what to specify. >>> +config HYPFS_CONFIG >>> + bool "Provide hypervisor .config via hypfs entry" >>> + default y >> >> My initial reaction was to ask for "depends on HYPFS", but then >> I noticed the earlier patch doesn't introduce such. Am I >> mis-remembering that it was agreed to make the whole thing >> possible to disable at least in EXPERT mode? > > No, I don't remember that agreement. > > And TBH I'm not sure this is a good idea, as that would at once make the > plan to replace at least some sysctl and/or domctl interfaces impossible > (like e.g. the last 3 patches of the series are doing already), or at > least would tie the related functionality to CONFIG_HYPFS. I think that would be fine - that's what config setting are for. Someone caring about space may not care about runtime setting of parameters. >>> --- a/xen/common/kernel.c >>> +++ b/xen/common/kernel.c >>> @@ -389,6 +389,16 @@ static HYPFS_STRING_INIT(compile_date, "compile_date"); >>> static HYPFS_STRING_INIT(compile_domain, "compile_domain"); >>> static HYPFS_STRING_INIT(extra, "extra"); >>> +#ifdef CONFIG_HYPFS_CONFIG >>> +static struct hypfs_entry_leaf config = { >>> + .e.type = XEN_HYPFS_TYPE_STRING, >>> + .e.encoding = XEN_HYPFS_ENC_GZIP, >>> + .e.name = "config", >>> + .e.read = hypfs_read_leaf, >>> + .content = &xen_config_data >>> +}; >>> +#endif >> >> Would be really good if no open-coded instantiations like this >> one would ever have to appear, i.e. if suitable macros were >> available. What's preventing use of one of the available ones >> here? > > Right now it is the only case for a non plain encoding. The alternative > would have been to either specify the encoding explicitly at all other > node definitions, or to have a macro for this single instance. Or set the encoding alongside e.size in the init function? Jan
On 03.04.20 17:33, Jan Beulich wrote: > On 03.04.2020 17:12, Jürgen Groß wrote: >> On 03.04.20 16:31, Jan Beulich wrote: >>> On 02.04.2020 17:46, Juergen Gross wrote: >>>> --- a/xen/common/Kconfig >>>> +++ b/xen/common/Kconfig >>>> @@ -353,6 +353,16 @@ config DOM0_MEM >>>> Leave empty if you are not sure what to specify. >>>> +config HYPFS_CONFIG >>>> + bool "Provide hypervisor .config via hypfs entry" >>>> + default y >>> >>> My initial reaction was to ask for "depends on HYPFS", but then >>> I noticed the earlier patch doesn't introduce such. Am I >>> mis-remembering that it was agreed to make the whole thing >>> possible to disable at least in EXPERT mode? >> >> No, I don't remember that agreement. >> >> And TBH I'm not sure this is a good idea, as that would at once make the >> plan to replace at least some sysctl and/or domctl interfaces impossible >> (like e.g. the last 3 patches of the series are doing already), or at >> least would tie the related functionality to CONFIG_HYPFS. > > I think that would be fine - that's what config setting are for. > Someone caring about space may not care about runtime setting of > parameters. So right now it would start with a plain hypfs available or not. The next step would be in patch 12 to tell the user he will lose the capability of setting runtime parameters. Another planned extension would be to control per-cpupool settings, which would the go away (possibly functionality being unconditionally available today). Next would be the lack of being able to control per-domain mitigations like XPTI or L1TF, which I'd like to add. Another thing I wanted to add is some debugging stuff (e.g. to switch lock profiling using hypfs). And the list will go on. Does it really make sense to make a central control and information interface conditional? I'd like at least a second opinion on that topic. > >>>> --- a/xen/common/kernel.c >>>> +++ b/xen/common/kernel.c >>>> @@ -389,6 +389,16 @@ static HYPFS_STRING_INIT(compile_date, "compile_date"); >>>> static HYPFS_STRING_INIT(compile_domain, "compile_domain"); >>>> static HYPFS_STRING_INIT(extra, "extra"); >>>> +#ifdef CONFIG_HYPFS_CONFIG >>>> +static struct hypfs_entry_leaf config = { >>>> + .e.type = XEN_HYPFS_TYPE_STRING, >>>> + .e.encoding = XEN_HYPFS_ENC_GZIP, >>>> + .e.name = "config", >>>> + .e.read = hypfs_read_leaf, >>>> + .content = &xen_config_data >>>> +}; >>>> +#endif >>> >>> Would be really good if no open-coded instantiations like this >>> one would ever have to appear, i.e. if suitable macros were >>> available. What's preventing use of one of the available ones >>> here? >> >> Right now it is the only case for a non plain encoding. The alternative >> would have been to either specify the encoding explicitly at all other >> node definitions, or to have a macro for this single instance. > > Or set the encoding alongside e.size in the init function? Hmm, that's an option, yes. Juergen
On 03.04.2020 17:45, Jürgen Groß wrote: > On 03.04.20 17:33, Jan Beulich wrote: >> On 03.04.2020 17:12, Jürgen Groß wrote: >>> On 03.04.20 16:31, Jan Beulich wrote: >>>> On 02.04.2020 17:46, Juergen Gross wrote: >>>>> --- a/xen/common/Kconfig >>>>> +++ b/xen/common/Kconfig >>>>> @@ -353,6 +353,16 @@ config DOM0_MEM >>>>> Leave empty if you are not sure what to specify. >>>>> +config HYPFS_CONFIG >>>>> + bool "Provide hypervisor .config via hypfs entry" >>>>> + default y >>>> >>>> My initial reaction was to ask for "depends on HYPFS", but then >>>> I noticed the earlier patch doesn't introduce such. Am I >>>> mis-remembering that it was agreed to make the whole thing >>>> possible to disable at least in EXPERT mode? >>> >>> No, I don't remember that agreement. >>> >>> And TBH I'm not sure this is a good idea, as that would at once make the >>> plan to replace at least some sysctl and/or domctl interfaces impossible >>> (like e.g. the last 3 patches of the series are doing already), or at >>> least would tie the related functionality to CONFIG_HYPFS. >> >> I think that would be fine - that's what config setting are for. >> Someone caring about space may not care about runtime setting of >> parameters. > > So right now it would start with a plain hypfs available or not. > > The next step would be in patch 12 to tell the user he will lose the > capability of setting runtime parameters. > > Another planned extension would be to control per-cpupool settings, > which would the go away (possibly functionality being unconditionally > available today). > > Next would be the lack of being able to control per-domain mitigations > like XPTI or L1TF, which I'd like to add. > > Another thing I wanted to add is some debugging stuff (e.g. to switch > lock profiling using hypfs). > > And the list will go on. Understood. > Does it really make sense to make a central control and information > interface conditional? None of the above may be of interest to e.g. embedded use cases. > I'd like at least a second opinion on that topic. Yes, further opinions would surely help. Jan
Stefano, On 06.04.20 14:29, Jan Beulich wrote: > On 03.04.2020 17:45, Jürgen Groß wrote: >> On 03.04.20 17:33, Jan Beulich wrote: >>> On 03.04.2020 17:12, Jürgen Groß wrote: >>>> On 03.04.20 16:31, Jan Beulich wrote: >>>>> On 02.04.2020 17:46, Juergen Gross wrote: >>>>>> --- a/xen/common/Kconfig >>>>>> +++ b/xen/common/Kconfig >>>>>> @@ -353,6 +353,16 @@ config DOM0_MEM >>>>>> Leave empty if you are not sure what to specify. >>>>>> +config HYPFS_CONFIG >>>>>> + bool "Provide hypervisor .config via hypfs entry" >>>>>> + default y >>>>> >>>>> My initial reaction was to ask for "depends on HYPFS", but then >>>>> I noticed the earlier patch doesn't introduce such. Am I >>>>> mis-remembering that it was agreed to make the whole thing >>>>> possible to disable at least in EXPERT mode? >>>> >>>> No, I don't remember that agreement. >>>> >>>> And TBH I'm not sure this is a good idea, as that would at once make the >>>> plan to replace at least some sysctl and/or domctl interfaces impossible >>>> (like e.g. the last 3 patches of the series are doing already), or at >>>> least would tie the related functionality to CONFIG_HYPFS. >>> >>> I think that would be fine - that's what config setting are for. >>> Someone caring about space may not care about runtime setting of >>> parameters. >> >> So right now it would start with a plain hypfs available or not. >> >> The next step would be in patch 12 to tell the user he will lose the >> capability of setting runtime parameters. >> >> Another planned extension would be to control per-cpupool settings, >> which would the go away (possibly functionality being unconditionally >> available today). >> >> Next would be the lack of being able to control per-domain mitigations >> like XPTI or L1TF, which I'd like to add. >> >> Another thing I wanted to add is some debugging stuff (e.g. to switch >> lock profiling using hypfs). >> >> And the list will go on. > > Understood. > >> Does it really make sense to make a central control and information >> interface conditional? > > None of the above may be of interest to e.g. embedded use cases. > >> I'd like at least a second opinion on that topic. > > Yes, further opinions would surely help. Any opinion on making hypfs configurable? It would be quite some code churn and I want to avoid it in case you see no benefit in configuring it out for embedded. Juergen
> On Apr 27, 2020, at 4:40 PM, Jürgen Groß <jgross@suse.com> wrote: > > Stefano, > > On 06.04.20 14:29, Jan Beulich wrote: >> On 03.04.2020 17:45, Jürgen Groß wrote: >>> On 03.04.20 17:33, Jan Beulich wrote: >>>> On 03.04.2020 17:12, Jürgen Groß wrote: >>>>> On 03.04.20 16:31, Jan Beulich wrote: >>>>>> On 02.04.2020 17:46, Juergen Gross wrote: >>>>>>> --- a/xen/common/Kconfig >>>>>>> +++ b/xen/common/Kconfig >>>>>>> @@ -353,6 +353,16 @@ config DOM0_MEM >>>>>>> Leave empty if you are not sure what to specify. >>>>>>> +config HYPFS_CONFIG >>>>>>> + bool "Provide hypervisor .config via hypfs entry" >>>>>>> + default y >>>>>> >>>>>> My initial reaction was to ask for "depends on HYPFS", but then >>>>>> I noticed the earlier patch doesn't introduce such. Am I >>>>>> mis-remembering that it was agreed to make the whole thing >>>>>> possible to disable at least in EXPERT mode? >>>>> >>>>> No, I don't remember that agreement. >>>>> >>>>> And TBH I'm not sure this is a good idea, as that would at once make the >>>>> plan to replace at least some sysctl and/or domctl interfaces impossible >>>>> (like e.g. the last 3 patches of the series are doing already), or at >>>>> least would tie the related functionality to CONFIG_HYPFS. >>>> >>>> I think that would be fine - that's what config setting are for. >>>> Someone caring about space may not care about runtime setting of >>>> parameters. >>> >>> So right now it would start with a plain hypfs available or not. >>> >>> The next step would be in patch 12 to tell the user he will lose the >>> capability of setting runtime parameters. >>> >>> Another planned extension would be to control per-cpupool settings, >>> which would the go away (possibly functionality being unconditionally >>> available today). >>> >>> Next would be the lack of being able to control per-domain mitigations >>> like XPTI or L1TF, which I'd like to add. >>> >>> Another thing I wanted to add is some debugging stuff (e.g. to switch >>> lock profiling using hypfs). >>> >>> And the list will go on. >> Understood. >>> Does it really make sense to make a central control and information >>> interface conditional? >> None of the above may be of interest to e.g. embedded use cases. >>> I'd like at least a second opinion on that topic. >> Yes, further opinions would surely help. > > Any opinion on making hypfs configurable? > > It would be quite some code churn and I want to avoid it in case you > see no benefit in configuring it out for embedded. Just to reply with what I said on IRC: First of all, if it were free, I think that having CONFIG_HYPFS would be valuable. Sub-options should be made available on a case-by-case basis; I think CONFIG_HYPFS_CONFIG would be valuable, but I don’t see any point in having CONFIG_HYPFS_RUNTIME_PARAMETER. However, Juergen seems to think it would require a lot of churn to the current series. I don’t quite understand why; but supposing that’s so: In general, the people who want a feature should be the ones who do the work to make it. I think it would be perfectly reasonable for Juergen to say, “This is a lot of work that I don’t have time to do before the 4.14 release; if people want to be able to disable this feature, they can post their own patches.” (It’s also perfectly reasonable for him to do the work himself just to be helpful.) The discussion about CONFIG_EXPERT is sort of the same. If we have CONFIG_HYPFS, it would obviously be more valuable if it were *not* in CONFIG_EXPERT, so that people could change it while still being security supported. If Jan is OK with it simply being outside CONFIG_EXPERT, then great. But if he insists on some kind of testing for it to be outside of CONFIG_EXPERT, then again, the people who want it to be security supported should be the ones who do the work to make it happen. Hope that makes sense. :-) -George
On 27.04.2020 18:25, George Dunlap wrote:
> If Jan is OK with it simply being outside CONFIG_EXPERT, then great. But if he insists on some kind of testing for it to be outside of CONFIG_EXPERT, then again, the people who want it to be security supported should be the ones who do the work to make it happen.
I don't understand this part, I'm afraid: Without a config option,
the code is going to be security supported as long as it doesn't
get marked otherwise (experimental or what not). With an option
depending on EXPERT, what would become security unsupported is the
non-default (i.e. disabled) setting. There's not a whole lot to
test there, it's merely a formal consequence of our general rules.
(Of course, over time dependencies of other code may develop on
the information being available e.g. to Dom0 userland. Just like
there's Linux userland code assuming the kernel config is
available in certain ways [I don't necessarily mean the equivalent
of hypfs here], to then use it in what I'd call abusive ways in at
least some cases.)
Jan
> On Apr 28, 2020, at 8:20 AM, Jan Beulich <jbeulich@suse.com> wrote: > > On 27.04.2020 18:25, George Dunlap wrote: >> If Jan is OK with it simply being outside CONFIG_EXPERT, then great. But if he insists on some kind of testing for it to be outside of CONFIG_EXPERT, then again, the people who want it to be security supported should be the ones who do the work to make it happen. > > I don't understand this part, I'm afraid: Without a config option, > the code is going to be security supported as long as it doesn't > get marked otherwise (experimental or what not). With an option > depending on EXPERT, what would become security unsupported is the > non-default (i.e. disabled) setting. There's not a whole lot to > test there, it's merely a formal consequence of our general rules. > (Of course, over time dependencies of other code may develop on > the information being available e.g. to Dom0 userland. Just like > there's Linux userland code assuming the kernel config is > available in certain ways [I don't necessarily mean the equivalent > of hypfs here], to then use it in what I'd call abusive ways in at > least some cases.) Here’s an argument you might make: “As a member of the security team, I don’t want to be on the hook for issuing XSAs for code which isn’t at least smoke-tested. Therefore, I oppose any patch adding CONFIG_HYPFS outside of CONFIG_EXPERT, *unless* there is a concrete plan for getting regular testing for CONFIG_HYPFS=n.” I’m not saying that’s an argument you *should* make. But personally I don’t have a strong argument against such an argument. So, it seems to me, if you did make it, you have a reasonable chance of carrying your point. Now consider this hypothetical universe where you made that argument and nobody opposed it. In order to get a particular feature (CONFIG_HYPFS=n security supported), there is extra work that needs to be done (getting CONFIG_HYPFS=n tested regularly). My point was, the expectation should be that the extra work will be done by the people who want or benefit from the feature; the series shouldn’t be blocked until Juergen implements CONFIG_HYPFS=n testing (since he doesn’t personally have a stake in that feature). Now obviously, doing work to help someone else out in the community is of course a good thing to do; it builds goodwill, uses our aggregate resources more efficiently, and makes our community more enjoyable to work with. But the goodwill primarily comes from the fact that it was done as a voluntary choice, not as a requirement. Juergen was balking at having to do what he saw as extra work to implement CONFIG_HYPFS. I wanted to make it clear that even though I see value in having CONFIG_HYPFS, *he* doesn’t have to do the work if he doesn’t want to (although it would certainly be appreciated if he did). And this paragraph was extending the same principle into the hypothetical universe where someone insisted that CONFIG_HYPFS=n had to be tested before being security supported. Hope that makes sense. :-) -George
On 28.04.2020 10:24, George Dunlap wrote: >> On Apr 28, 2020, at 8:20 AM, Jan Beulich <jbeulich@suse.com> wrote: >> On 27.04.2020 18:25, George Dunlap wrote: >>> If Jan is OK with it simply being outside CONFIG_EXPERT, then great. But if he insists on some kind of testing for it to be outside of CONFIG_EXPERT, then again, the people who want it to be security supported should be the ones who do the work to make it happen. >> >> I don't understand this part, I'm afraid: Without a config option, >> the code is going to be security supported as long as it doesn't >> get marked otherwise (experimental or what not). With an option >> depending on EXPERT, what would become security unsupported is the >> non-default (i.e. disabled) setting. There's not a whole lot to >> test there, it's merely a formal consequence of our general rules. >> (Of course, over time dependencies of other code may develop on >> the information being available e.g. to Dom0 userland. Just like >> there's Linux userland code assuming the kernel config is >> available in certain ways [I don't necessarily mean the equivalent >> of hypfs here], to then use it in what I'd call abusive ways in at >> least some cases.) > > Here’s an argument you might make: > > “As a member of the security team, I don’t want to be on the hook for issuing XSAs for code which isn’t at least smoke-tested. Therefore, I oppose any patch adding CONFIG_HYPFS outside of CONFIG_EXPERT, *unless* there is a concrete plan for getting regular testing for CONFIG_HYPFS=n.” > > I’m not saying that’s an argument you *should* make. But personally I don’t have a strong argument against such an argument. So, it seems to me, if you did make it, you have a reasonable chance of carrying your point. > > Now consider this hypothetical universe where you made that argument and nobody opposed it. In order to get a particular feature (CONFIG_HYPFS=n security supported), there is extra work that needs to be done (getting CONFIG_HYPFS=n tested regularly). My point was, the expectation should be that the extra work will be done by the people who want or benefit from the feature; the series shouldn’t be blocked until Juergen implements CONFIG_HYPFS=n testing (since he doesn’t personally have a stake in that feature). > > Now obviously, doing work to help someone else out in the community is of course a good thing to do; it builds goodwill, uses our aggregate resources more efficiently, and makes our community more enjoyable to work with. But the goodwill primarily comes from the fact that it was done as a voluntary choice, not as a requirement. > > Juergen was balking at having to do what he saw as extra work to implement CONFIG_HYPFS. I wanted to make it clear that even though I see value in having CONFIG_HYPFS, *he* doesn’t have to do the work if he doesn’t want to (although it would certainly be appreciated if he did). And this paragraph was extending the same principle into the hypothetical universe where someone insisted that CONFIG_HYPFS=n had to be tested before being security supported. > > Hope that makes sense. :-) Yes, it does, thanks for the clarification. I can see what you describe as a valid perspective to take, but really in my request to Jürgen I took another: Now that we have Kconfig, additions of larger bodies of code (possibly also just in terms of binary size) should imo generally be questioned whether they want/need to be built for everyone. I.e. it is not to be left to people being worried about binary sizes to arrange for things to not be built, but for people contributing new but not entirely essential code to consider making it option from the very beginning. In particular, seeing which patch we're discussing, I find it far less helpful to have CONFIG_HYPFS_CONFIG when there's no CONFIG_HYPFS, at least as long as our .config-s are still tiny compared to Linux'es. Jan
Hi Jan, On 28/04/2020 09:39, Jan Beulich wrote: > On 28.04.2020 10:24, George Dunlap wrote: >>> On Apr 28, 2020, at 8:20 AM, Jan Beulich <jbeulich@suse.com> wrote: >>> On 27.04.2020 18:25, George Dunlap wrote: >>>> If Jan is OK with it simply being outside CONFIG_EXPERT, then great. But if he insists on some kind of testing for it to be outside of CONFIG_EXPERT, then again, the people who want it to be security supported should be the ones who do the work to make it happen. >>> >>> I don't understand this part, I'm afraid: Without a config option, >>> the code is going to be security supported as long as it doesn't >>> get marked otherwise (experimental or what not). With an option >>> depending on EXPERT, what would become security unsupported is the >>> non-default (i.e. disabled) setting. There's not a whole lot to >>> test there, it's merely a formal consequence of our general rules. >>> (Of course, over time dependencies of other code may develop on >>> the information being available e.g. to Dom0 userland. Just like >>> there's Linux userland code assuming the kernel config is >>> available in certain ways [I don't necessarily mean the equivalent >>> of hypfs here], to then use it in what I'd call abusive ways in at >>> least some cases.) >> >> Here’s an argument you might make: >> >> “As a member of the security team, I don’t want to be on the hook for issuing XSAs for code which isn’t at least smoke-tested. Therefore, I oppose any patch adding CONFIG_HYPFS outside of CONFIG_EXPERT, *unless* there is a concrete plan for getting regular testing for CONFIG_HYPFS=n.” >> >> I’m not saying that’s an argument you *should* make. But personally I don’t have a strong argument against such an argument. So, it seems to me, if you did make it, you have a reasonable chance of carrying your point. >> >> Now consider this hypothetical universe where you made that argument and nobody opposed it. In order to get a particular feature (CONFIG_HYPFS=n security supported), there is extra work that needs to be done (getting CONFIG_HYPFS=n tested regularly). My point was, the expectation should be that the extra work will be done by the people who want or benefit from the feature; the series shouldn’t be blocked until Juergen implements CONFIG_HYPFS=n testing (since he doesn’t personally have a stake in that feature). >> >> Now obviously, doing work to help someone else out in the community is of course a good thing to do; it builds goodwill, uses our aggregate resources more efficiently, and makes our community more enjoyable to work with. But the goodwill primarily comes from the fact that it was done as a voluntary choice, not as a requirement. >> >> Juergen was balking at having to do what he saw as extra work to implement CONFIG_HYPFS. I wanted to make it clear that even though I see value in having CONFIG_HYPFS, *he* doesn’t have to do the work if he doesn’t want to (although it would certainly be appreciated if he did). And this paragraph was extending the same principle into the hypothetical universe where someone insisted that CONFIG_HYPFS=n had to be tested before being security supported. >> >> Hope that makes sense. :-) > > Yes, it does, thanks for the clarification. I can see what you describe > as a valid perspective to take, but really in my request to Jürgen I > took another: Now that we have Kconfig, additions of larger bodies of > code (possibly also just in terms of binary size) should imo generally > be questioned whether they want/need to be built for everyone. I.e. it > is not to be left to people being worried about binary sizes to arrange > for things to not be built, but for people contributing new but not > entirely essential code to consider making it option from the very > beginning. I like the idea to have a more configurable Xen but this also comes at the expense of the testing/support. At the moment, we are getting around the problem by gating the new config options with CONFIG_EXPERT. I have stoppped counting the number of time I sweared because my config got rewritten when using 'make clean' or explain to someone else how to use it. As it stands, CONFIG_EXPERT is unusable and most likely anything behind it will rot quite quickly. So if we want to add more stuff behind it, then I would suggest to make it more accessible so any developper can experiment with it. Going forward, I would expect the embedded folks to want more part of Xen configurable. Requesting them to use CONFIG_EXPERT may be an issue as this means we would not security support them. At the same time, I understand that exposing a CONFIG increase the testing matrix. How about declaring we are supporting/testing a given set of .config? On Arm it would be defconfig and tiny. Cheers,
On 28.04.2020 11:43, Julien Grall wrote: > Hi Jan, > > On 28/04/2020 09:39, Jan Beulich wrote: >> On 28.04.2020 10:24, George Dunlap wrote: >>>> On Apr 28, 2020, at 8:20 AM, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 27.04.2020 18:25, George Dunlap wrote: >>>>> If Jan is OK with it simply being outside CONFIG_EXPERT, then great. But if he insists on some kind of testing for it to be outside of CONFIG_EXPERT, then again, the people who want it to be security supported should be the ones who do the work to make it happen. >>>> >>>> I don't understand this part, I'm afraid: Without a config option, >>>> the code is going to be security supported as long as it doesn't >>>> get marked otherwise (experimental or what not). With an option >>>> depending on EXPERT, what would become security unsupported is the >>>> non-default (i.e. disabled) setting. There's not a whole lot to >>>> test there, it's merely a formal consequence of our general rules. >>>> (Of course, over time dependencies of other code may develop on >>>> the information being available e.g. to Dom0 userland. Just like >>>> there's Linux userland code assuming the kernel config is >>>> available in certain ways [I don't necessarily mean the equivalent >>>> of hypfs here], to then use it in what I'd call abusive ways in at >>>> least some cases.) >>> >>> Here’s an argument you might make: >>> >>> “As a member of the security team, I don’t want to be on the hook for issuing XSAs for code which isn’t at least smoke-tested. Therefore, I oppose any patch adding CONFIG_HYPFS outside of CONFIG_EXPERT, *unless* there is a concrete plan for getting regular testing for CONFIG_HYPFS=n.” >>> >>> I’m not saying that’s an argument you *should* make. But personally I don’t have a strong argument against such an argument. So, it seems to me, if you did make it, you have a reasonable chance of carrying your point. >>> >>> Now consider this hypothetical universe where you made that argument and nobody opposed it. In order to get a particular feature (CONFIG_HYPFS=n security supported), there is extra work that needs to be done (getting CONFIG_HYPFS=n tested regularly). My point was, the expectation should be that the extra work will be done by the people who want or benefit from the feature; the series shouldn’t be blocked until Juergen implements CONFIG_HYPFS=n testing (since he doesn’t personally have a stake in that feature). >>> >>> Now obviously, doing work to help someone else out in the community is of course a good thing to do; it builds goodwill, uses our aggregate resources more efficiently, and makes our community more enjoyable to work with. But the goodwill primarily comes from the fact that it was done as a voluntary choice, not as a requirement. >>> >>> Juergen was balking at having to do what he saw as extra work to implement CONFIG_HYPFS. I wanted to make it clear that even though I see value in having CONFIG_HYPFS, *he* doesn’t have to do the work if he doesn’t want to (although it would certainly be appreciated if he did). And this paragraph was extending the same principle into the hypothetical universe where someone insisted that CONFIG_HYPFS=n had to be tested before being security supported. >>> >>> Hope that makes sense. :-) >> >> Yes, it does, thanks for the clarification. I can see what you describe >> as a valid perspective to take, but really in my request to Jürgen I >> took another: Now that we have Kconfig, additions of larger bodies of >> code (possibly also just in terms of binary size) should imo generally >> be questioned whether they want/need to be built for everyone. I.e. it >> is not to be left to people being worried about binary sizes to arrange >> for things to not be built, but for people contributing new but not >> entirely essential code to consider making it option from the very >> beginning. > > I like the idea to have a more configurable Xen but this also comes at the expense of the testing/support. > > At the moment, we are getting around the problem by gating the new config options with CONFIG_EXPERT. I have stoppped counting the number of time I sweared because my config got rewritten when using 'make clean' or explain to someone else how to use it. > > As it stands, CONFIG_EXPERT is unusable and most likely anything behind it will rot quite quickly. So if we want to add more stuff behind it, then I would suggest to make it more accessible so any developper can experiment with it. This complaint is not new; what I'm missing are concrete suggestions on how to improve the situation. > Going forward, I would expect the embedded folks to want more part of Xen configurable. Requesting them to use CONFIG_EXPERT may be an issue as this means we would not security support them. At the same time, I understand that exposing a CONFIG increase the testing matrix. How about declaring we are supporting/testing a given set of .config? On Arm it would be defconfig and tiny. We could do this, sure, but it would end up being rather limiting at least on the x86 side. Considering how frequently this is coming up, perhaps instead we should drop use of EXPERT mostly or altogether, and declare that we're willing to live with the fallout? We could document options or option combinations we specifically exclude from being supported then ... Jan
Hi Jan, On 28/04/2020 10:59, Jan Beulich wrote: > On 28.04.2020 11:43, Julien Grall wrote: >> Hi Jan, >> >> On 28/04/2020 09:39, Jan Beulich wrote: >>> On 28.04.2020 10:24, George Dunlap wrote: >>>>> On Apr 28, 2020, at 8:20 AM, Jan Beulich <jbeulich@suse.com> wrote: >>>>> On 27.04.2020 18:25, George Dunlap wrote: >>>>>> If Jan is OK with it simply being outside CONFIG_EXPERT, then great. But if he insists on some kind of testing for it to be outside of CONFIG_EXPERT, then again, the people who want it to be security supported should be the ones who do the work to make it happen. >>>>> >>>>> I don't understand this part, I'm afraid: Without a config option, >>>>> the code is going to be security supported as long as it doesn't >>>>> get marked otherwise (experimental or what not). With an option >>>>> depending on EXPERT, what would become security unsupported is the >>>>> non-default (i.e. disabled) setting. There's not a whole lot to >>>>> test there, it's merely a formal consequence of our general rules. >>>>> (Of course, over time dependencies of other code may develop on >>>>> the information being available e.g. to Dom0 userland. Just like >>>>> there's Linux userland code assuming the kernel config is >>>>> available in certain ways [I don't necessarily mean the equivalent >>>>> of hypfs here], to then use it in what I'd call abusive ways in at >>>>> least some cases.) >>>> >>>> Here’s an argument you might make: >>>> >>>> “As a member of the security team, I don’t want to be on the hook for issuing XSAs for code which isn’t at least smoke-tested. Therefore, I oppose any patch adding CONFIG_HYPFS outside of CONFIG_EXPERT, *unless* there is a concrete plan for getting regular testing for CONFIG_HYPFS=n.” >>>> >>>> I’m not saying that’s an argument you *should* make. But personally I don’t have a strong argument against such an argument. So, it seems to me, if you did make it, you have a reasonable chance of carrying your point. >>>> >>>> Now consider this hypothetical universe where you made that argument and nobody opposed it. In order to get a particular feature (CONFIG_HYPFS=n security supported), there is extra work that needs to be done (getting CONFIG_HYPFS=n tested regularly). My point was, the expectation should be that the extra work will be done by the people who want or benefit from the feature; the series shouldn’t be blocked until Juergen implements CONFIG_HYPFS=n testing (since he doesn’t personally have a stake in that feature). >>>> >>>> Now obviously, doing work to help someone else out in the community is of course a good thing to do; it builds goodwill, uses our aggregate resources more efficiently, and makes our community more enjoyable to work with. But the goodwill primarily comes from the fact that it was done as a voluntary choice, not as a requirement. >>>> >>>> Juergen was balking at having to do what he saw as extra work to implement CONFIG_HYPFS. I wanted to make it clear that even though I see value in having CONFIG_HYPFS, *he* doesn’t have to do the work if he doesn’t want to (although it would certainly be appreciated if he did). And this paragraph was extending the same principle into the hypothetical universe where someone insisted that CONFIG_HYPFS=n had to be tested before being security supported. >>>> >>>> Hope that makes sense. :-) >>> >>> Yes, it does, thanks for the clarification. I can see what you describe >>> as a valid perspective to take, but really in my request to Jürgen I >>> took another: Now that we have Kconfig, additions of larger bodies of >>> code (possibly also just in terms of binary size) should imo generally >>> be questioned whether they want/need to be built for everyone. I.e. it >>> is not to be left to people being worried about binary sizes to arrange >>> for things to not be built, but for people contributing new but not >>> entirely essential code to consider making it option from the very >>> beginning. >> >> I like the idea to have a more configurable Xen but this also comes at the expense of the testing/support. >> >> At the moment, we are getting around the problem by gating the new config options with CONFIG_EXPERT. I have stoppped counting the number of time I sweared because my config got rewritten when using 'make clean' or explain to someone else how to use it. >> >> As it stands, CONFIG_EXPERT is unusable and most likely anything behind it will rot quite quickly. So if we want to add more stuff behind it, then I would suggest to make it more accessible so any developper can experiment with it. > > This complaint is not new; what I'm missing are concrete suggestions > on how to improve the situation. It is quite easy to improve, rather than specifying on the command line we could introduce a new Kconfig option so the user can select it normally. This would not be very different compare to what Linux does. > >> Going forward, I would expect the embedded folks to want more part of Xen configurable. Requesting them to use CONFIG_EXPERT may be an issue as this means we would not security support them. At the same time, I understand that exposing a CONFIG increase the testing matrix. How about declaring we are supporting/testing a given set of .config? On Arm it would be defconfig and tiny. > > We could do this, sure, but it would end up being rather limiting at > least on the x86 side. It would not be worse than where we are today, right? > > Considering how frequently this is coming up, perhaps instead we > should drop use of EXPERT mostly or altogether, and declare that > we're willing to live with the fallout? We could document options > or option combinations we specifically exclude from being supported > then ... I would not suggest to remove EXPERT so far, just to make easier to use of EXPERT. Cheers,
> On Apr 28, 2020, at 9:39 AM, Jan Beulich <jbeulich@suse.com> wrote: > > On 28.04.2020 10:24, George Dunlap wrote: >>> On Apr 28, 2020, at 8:20 AM, Jan Beulich <jbeulich@suse.com> wrote: >>> On 27.04.2020 18:25, George Dunlap wrote: >>>> If Jan is OK with it simply being outside CONFIG_EXPERT, then great. But if he insists on some kind of testing for it to be outside of CONFIG_EXPERT, then again, the people who want it to be security supported should be the ones who do the work to make it happen. >>> >>> I don't understand this part, I'm afraid: Without a config option, >>> the code is going to be security supported as long as it doesn't >>> get marked otherwise (experimental or what not). With an option >>> depending on EXPERT, what would become security unsupported is the >>> non-default (i.e. disabled) setting. There's not a whole lot to >>> test there, it's merely a formal consequence of our general rules. >>> (Of course, over time dependencies of other code may develop on >>> the information being available e.g. to Dom0 userland. Just like >>> there's Linux userland code assuming the kernel config is >>> available in certain ways [I don't necessarily mean the equivalent >>> of hypfs here], to then use it in what I'd call abusive ways in at >>> least some cases.) >> >> Here’s an argument you might make: >> >> “As a member of the security team, I don’t want to be on the hook for issuing XSAs for code which isn’t at least smoke-tested. Therefore, I oppose any patch adding CONFIG_HYPFS outside of CONFIG_EXPERT, *unless* there is a concrete plan for getting regular testing for CONFIG_HYPFS=n.” >> >> I’m not saying that’s an argument you *should* make. But personally I don’t have a strong argument against such an argument. So, it seems to me, if you did make it, you have a reasonable chance of carrying your point. >> >> Now consider this hypothetical universe where you made that argument and nobody opposed it. In order to get a particular feature (CONFIG_HYPFS=n security supported), there is extra work that needs to be done (getting CONFIG_HYPFS=n tested regularly). My point was, the expectation should be that the extra work will be done by the people who want or benefit from the feature; the series shouldn’t be blocked until Juergen implements CONFIG_HYPFS=n testing (since he doesn’t personally have a stake in that feature). >> >> Now obviously, doing work to help someone else out in the community is of course a good thing to do; it builds goodwill, uses our aggregate resources more efficiently, and makes our community more enjoyable to work with. But the goodwill primarily comes from the fact that it was done as a voluntary choice, not as a requirement. >> >> Juergen was balking at having to do what he saw as extra work to implement CONFIG_HYPFS. I wanted to make it clear that even though I see value in having CONFIG_HYPFS, *he* doesn’t have to do the work if he doesn’t want to (although it would certainly be appreciated if he did). And this paragraph was extending the same principle into the hypothetical universe where someone insisted that CONFIG_HYPFS=n had to be tested before being security supported. >> >> Hope that makes sense. :-) > > Yes, it does, thanks for the clarification. I can see what you describe > as a valid perspective to take, but really in my request to Jürgen I > took another: Now that we have Kconfig, additions of larger bodies of > code (possibly also just in terms of binary size) should imo generally > be questioned whether they want/need to be built for everyone. I.e. it > is not to be left to people being worried about binary sizes to arrange > for things to not be built, but for people contributing new but not > entirely essential code to consider making it option from the very > beginning. I think that’s a reasonable position to take, but needs to be balanced on the amount of work that this would actually require. If it only requires adding a handful of #ifdef’s and maybe making a few stubs, then yes, asking the submitter to make the change makes sense. But if it requires three dozen #ifdef’s throughout the code and a fairly major architectural change, then I think it’s reasonable for a submitter to push back. I don’t really understand why Juergen thinks adding CONFIG_HYPFS would cause a lot of code churn; my argumentation here is based on the assumption that his assessment is correct. -George
On 28.04.20 13:23, George Dunlap wrote: > > >> On Apr 28, 2020, at 9:39 AM, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 28.04.2020 10:24, George Dunlap wrote: >>>> On Apr 28, 2020, at 8:20 AM, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 27.04.2020 18:25, George Dunlap wrote: >>>>> If Jan is OK with it simply being outside CONFIG_EXPERT, then great. But if he insists on some kind of testing for it to be outside of CONFIG_EXPERT, then again, the people who want it to be security supported should be the ones who do the work to make it happen. >>>> >>>> I don't understand this part, I'm afraid: Without a config option, >>>> the code is going to be security supported as long as it doesn't >>>> get marked otherwise (experimental or what not). With an option >>>> depending on EXPERT, what would become security unsupported is the >>>> non-default (i.e. disabled) setting. There's not a whole lot to >>>> test there, it's merely a formal consequence of our general rules. >>>> (Of course, over time dependencies of other code may develop on >>>> the information being available e.g. to Dom0 userland. Just like >>>> there's Linux userland code assuming the kernel config is >>>> available in certain ways [I don't necessarily mean the equivalent >>>> of hypfs here], to then use it in what I'd call abusive ways in at >>>> least some cases.) >>> >>> Here’s an argument you might make: >>> >>> “As a member of the security team, I don’t want to be on the hook for issuing XSAs for code which isn’t at least smoke-tested. Therefore, I oppose any patch adding CONFIG_HYPFS outside of CONFIG_EXPERT, *unless* there is a concrete plan for getting regular testing for CONFIG_HYPFS=n.” >>> >>> I’m not saying that’s an argument you *should* make. But personally I don’t have a strong argument against such an argument. So, it seems to me, if you did make it, you have a reasonable chance of carrying your point. >>> >>> Now consider this hypothetical universe where you made that argument and nobody opposed it. In order to get a particular feature (CONFIG_HYPFS=n security supported), there is extra work that needs to be done (getting CONFIG_HYPFS=n tested regularly). My point was, the expectation should be that the extra work will be done by the people who want or benefit from the feature; the series shouldn’t be blocked until Juergen implements CONFIG_HYPFS=n testing (since he doesn’t personally have a stake in that feature). >>> >>> Now obviously, doing work to help someone else out in the community is of course a good thing to do; it builds goodwill, uses our aggregate resources more efficiently, and makes our community more enjoyable to work with. But the goodwill primarily comes from the fact that it was done as a voluntary choice, not as a requirement. >>> >>> Juergen was balking at having to do what he saw as extra work to implement CONFIG_HYPFS. I wanted to make it clear that even though I see value in having CONFIG_HYPFS, *he* doesn’t have to do the work if he doesn’t want to (although it would certainly be appreciated if he did). And this paragraph was extending the same principle into the hypothetical universe where someone insisted that CONFIG_HYPFS=n had to be tested before being security supported. >>> >>> Hope that makes sense. :-) >> >> Yes, it does, thanks for the clarification. I can see what you describe >> as a valid perspective to take, but really in my request to Jürgen I >> took another: Now that we have Kconfig, additions of larger bodies of >> code (possibly also just in terms of binary size) should imo generally >> be questioned whether they want/need to be built for everyone. I.e. it >> is not to be left to people being worried about binary sizes to arrange >> for things to not be built, but for people contributing new but not >> entirely essential code to consider making it option from the very >> beginning. > > I think that’s a reasonable position to take, but needs to be balanced on the amount of work that this would actually require. If it only requires adding a handful of #ifdef’s and maybe making a few stubs, then yes, asking the submitter to make the change makes sense. But if it requires three dozen #ifdef’s throughout the code and a fairly major architectural change, then I think it’s reasonable for a submitter to push back. > > I don’t really understand why Juergen thinks adding CONFIG_HYPFS would cause a lot of code churn; my argumentation here is based on the assumption that his assessment is correct. The main problem I'm seeing is the setting of runtime parameters. This will need a complete different set of macros for defining those parameters, split across multiple patches. And I'm fairly sure I'll need to touch each custom_runtime_param handling function, too, in order not to add dead code. Juergen
diff --git a/.gitignore b/.gitignore index fd5610718d..bc8e053ccb 100644 --- a/.gitignore +++ b/.gitignore @@ -297,6 +297,8 @@ xen/arch/*/efi/boot.c xen/arch/*/efi/compat.c xen/arch/*/efi/efi.h xen/arch/*/efi/runtime.c +xen/common/config_data.S +xen/common/config.gz xen/include/headers*.chk xen/include/asm xen/include/asm-*/asm-offsets.h diff --git a/docs/misc/hypfs-paths.pandoc b/docs/misc/hypfs-paths.pandoc index e392feff27..f134c505d6 100644 --- a/docs/misc/hypfs-paths.pandoc +++ b/docs/misc/hypfs-paths.pandoc @@ -133,6 +133,10 @@ Information about the compile domain. The compiler used to build Xen. +#### /buildinfo/config = STRING [CONFIG_HYPFS_CONFIG] + +The contents of the `xen/.config` file at the time of the hypervisor build. + #### /buildinfo/version/ A directory containing version information of the hypervisor. diff --git a/xen/common/Kconfig b/xen/common/Kconfig index a6914fcae9..c3303c8dfe 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -353,6 +353,16 @@ config DOM0_MEM Leave empty if you are not sure what to specify. +config HYPFS_CONFIG + bool "Provide hypervisor .config via hypfs entry" + default y + ---help--- + When enabled the contents of the .config file used to build the + hypervisor are provided via the hypfs entry /buildinfo/config. + + Disable this option in case you want to spare some memory or you + want to hide the .config contents from dom0. + config TRACEBUFFER bool "Enable tracing infrastructure" if EXPERT = "y" default y diff --git a/xen/common/Makefile b/xen/common/Makefile index 022e5f7ca3..3ae9f3420b 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -1,6 +1,7 @@ obj-$(CONFIG_ARGO) += argo.o obj-y += bitmap.o obj-y += bsearch.o +obj-$(CONFIG_HYPFS_CONFIG) += config_data.o obj-$(CONFIG_CORE_PARKING) += core_parking.o obj-y += cpu.o obj-$(CONFIG_DEBUG_TRACE) += debugtrace.o @@ -73,3 +74,14 @@ obj-$(CONFIG_UBSAN) += ubsan/ obj-$(CONFIG_NEEDS_LIBELF) += libelf/ obj-$(CONFIG_HAS_DEVICE_TREE) += libfdt/ + +config.gz: ../.config + gzip -c $< >$@ + +config_data.o: config.gz + +config_data.S: $(XEN_ROOT)/xen/tools/binfile + $(XEN_ROOT)/xen/tools/binfile $@ config.gz xen_config_data + +clean:: + rm -f config_data.S config.gz 2>/dev/null diff --git a/xen/common/kernel.c b/xen/common/kernel.c index da6e4b4444..4b7bc28afb 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -389,6 +389,16 @@ static HYPFS_STRING_INIT(compile_date, "compile_date"); static HYPFS_STRING_INIT(compile_domain, "compile_domain"); static HYPFS_STRING_INIT(extra, "extra"); +#ifdef CONFIG_HYPFS_CONFIG +static struct hypfs_entry_leaf config = { + .e.type = XEN_HYPFS_TYPE_STRING, + .e.encoding = XEN_HYPFS_ENC_GZIP, + .e.name = "config", + .e.read = hypfs_read_leaf, + .content = &xen_config_data +}; +#endif + static int __init buildinfo_init(void) { hypfs_add_dir(&hypfs_root, &buildinfo, true); @@ -414,6 +424,11 @@ static int __init buildinfo_init(void) hypfs_add_leaf(&version, &major, true); hypfs_add_leaf(&version, &minor, true); +#ifdef CONFIG_HYPFS_CONFIG + config.e.size = xen_config_data_size; + hypfs_add_leaf(&buildinfo, &config, true); +#endif + return 0; } __initcall(buildinfo_init); diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h index 548b64da9f..02e3281f52 100644 --- a/xen/include/xen/kernel.h +++ b/xen/include/xen/kernel.h @@ -100,5 +100,8 @@ extern enum system_state { bool_t is_active_kernel_text(unsigned long addr); +extern const char xen_config_data; +extern const unsigned int xen_config_data_size; + #endif /* _LINUX_KERNEL_H */
Add the /buildinfo/config entry to the hypervisor filesystem. This entry contains the .config file used to build the hypervisor. Signed-off-by: Juergen Gross <jgross@suse.com> --- V3: - store data in gzip format - use binfile mechanism to create data file - move code to kernel.c V6: - add config item for the /buildinfo/config (Jan Beulich) - make config related variables const in kernel.h (Jan Beulich) V7: - update doc (Jan Beulich) - use "rm -f" in Makefile (Jan Beulich) --- .gitignore | 2 ++ docs/misc/hypfs-paths.pandoc | 4 ++++ xen/common/Kconfig | 10 ++++++++++ xen/common/Makefile | 12 ++++++++++++ xen/common/kernel.c | 15 +++++++++++++++ xen/include/xen/kernel.h | 3 +++ 6 files changed, 46 insertions(+)