lkml.org 
[lkml]   [2021]   [Jan]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [External] Re: [PATCH v3 2/2] misc: pvpanic: introduce module parameter 'events'
From
Date
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

\
 
 \ /
  Last update: 2021-01-10 04:15    [W:2.357 / U:0.588 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site