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

[zhenwei pi]

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