Re: [External] Re: [PATCH v3 2/2] misc: pvpanic: introduce module parameter 'events'

[zhenwei pi <pizhenwei@bytedance.com>]

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