Messages in this thread |  | | Subject | Re: [External] Re: [PATCH v3 2/2] misc: pvpanic: introduce module parameter 'events' | From | zhenwei pi <> | Date | Sun, 10 Jan 2021 11:10:43 +0800 |
| |
On 1/9/21 7:31 PM, Greg KH wrote: > On Fri, Jan 08, 2021 at 04:26:17PM +0100, Paolo Bonzini wrote: >> On 08/01/21 16:15, Greg KH wrote: >>> On Fri, Jan 08, 2021 at 04:04:24PM +0100, Paolo Bonzini wrote: >>>> On 08/01/21 15:07, Greg KH wrote: >>>>>> static void __iomem *base; >>>>>> +static unsigned int events = PVPANIC_PANICKED | PVPANIC_CRASH_LOADED; >>>>>> +module_param(events, uint, 0644); >>>>>> +MODULE_PARM_DESC(events, "set event limitation of pvpanic device"); >>>>> I do not understand you wanting a module parameter as well as a sysfs >>>>> file. Why is this needed? Why are you spreading this information out >>>>> across different apis and locations? >>>> >>>> It can be useful to disable some functionality, for example in case you want >>>> to fake running on an older virtualization host. This can be done for >>>> debugging reasons, or to keep uniform handling across a fleet that is >>>> running different versions of QEMU. >>> >>> And where is this all going to be documented? >> >> I don't disagree. >> >>> And what's wrong with just making the sysfs attribute writable? >> >> Isn't it harder to configure it at boot? Also the sysfs attribute added by >> patch 1 is documenting what is supported by the device, while the module >> parameter can be set to any value (you can think of the module parameter as >> of a "what to log" option, except the logging happens on another machine). > > But the module parameter is global, and not device specific. > > And yes, it would be harder to configure this at boot, is this something > that is required? If so, please document that somewhere. > >> Therefore, if you make the sysfs attribute writable, you would actually need >> _two_ attributes, one for the in-use capabilities and one for the device >> capabilities. And sysfs files are runtime values, which is different >> concept than 0444 module parameters (which are more like just >> configuration). > > That's not the module parameter mode setting in this patch :( > >> So you would have to decide whether it's valid to write 2 >> to the in-use capabilities file when the device capabilities are "1", and I >> don't really have a good answer for that. >> >> Also considering that there will not be more than one copy of this device >> (it doesn't make sense as they would all do exactly the same thing), in this >> case a module parameter really seems to be the simplest way to configure it. > > So you never can have more than one of these in the system at one time? > Because if this ever becomes not true, the module parameter is a mess... > > thanks, > > greg k-h >
What about adding _two_ device attribute: capability (0444): detect from device which the hypervisor really supports.
events (0644): root user could enable/disable feature(s) from guest side.
-- zhenwei pi
|  |