| Message ID | 1434024527-13869-2-git-send-email-eric.auger@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2015-06-11 at 14:08 +0200, Eric Auger wrote: > This patch introduces the vfio_platform_reset_combo struct that > stores all the information useful to handle the reset modality: > compat string, name of the reset function, name of the module that > implements the reset function. A lookup table of such structures > is added, currently containing a single sentinel element. A new > type field is added in vfio_platform_device to store what kind of > reset is associated to the device, if any. The commit log no longer matches the code. The only other thing I'm struggling with in this series is that in 0/4 you suggest that the reset modules can be in-kernel or external, but we're making a static list here, so there's really no support for random, user-provided reset modules. So are we missing the mark on the requirements? One way I thought you could achieve your requirement would be if we did away with the lookup table and looked for the module and function using a pre-defined transform on the compat ID. For instance, a compat ID of "calxeda,hb-xgmac" would automatically request a module named "vfio-platform-reset-calxedahb-xgmac" and look for a symbol of the same name for the reset function (I wonder if we can actually have a module and symbol of the same name). It seems fairly safe since an external module would need to be explicitly placed in the search path for the userspace module loader. Otherwise the table would need to become a list, the external module would need to be manually loaded, and the module_init() would need to register an entry on that list. Thanks, Alex > Signed-off-by: Eric Auger <eric.auger@linaro.org> > > --- > > v2 -> v3: > - add const in struct vfio_platform_reset_combo > - remove enum vfio_platform_reset_type > > v2: creation > --- > drivers/vfio/platform/vfio_platform_common.c | 3 +++ > drivers/vfio/platform/vfio_platform_private.h | 6 ++++++ > 2 files changed, 9 insertions(+) > > diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c > index abcff7a..611597e 100644 > --- a/drivers/vfio/platform/vfio_platform_common.c > +++ b/drivers/vfio/platform/vfio_platform_common.c > @@ -25,6 +25,9 @@ > > static DEFINE_MUTEX(driver_lock); > > +static const struct vfio_platform_reset_combo reset_lookup_table[] = { > +}; > + > static int vfio_platform_regions_init(struct vfio_platform_device *vdev) > { > int cnt = 0, i; > diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h > index 5d31e04..9e37b9f 100644 > --- a/drivers/vfio/platform/vfio_platform_private.h > +++ b/drivers/vfio/platform/vfio_platform_private.h > @@ -69,6 +69,12 @@ struct vfio_platform_device { > int (*get_irq)(struct vfio_platform_device *vdev, int i); > }; > > +struct vfio_platform_reset_combo { > + const char *compat; > + const char *reset_function_name; > + const char *module_name; > +}; > + > extern int vfio_platform_probe_common(struct vfio_platform_device *vdev, > struct device *dev); > extern struct vfio_platform_device *vfio_platform_remove_common
Hi Alex, On 06/11/2015 11:11 PM, Alex Williamson wrote: > On Thu, 2015-06-11 at 14:08 +0200, Eric Auger wrote: >> This patch introduces the vfio_platform_reset_combo struct that >> stores all the information useful to handle the reset modality: >> compat string, name of the reset function, name of the module that >> implements the reset function. A lookup table of such structures >> is added, currently containing a single sentinel element. A new >> type field is added in vfio_platform_device to store what kind of >> reset is associated to the device, if any. > > The commit log no longer matches the code. yes indeed missed that. > > The only other thing I'm struggling with in this series is that in 0/4 > you suggest that the reset modules can be in-kernel or external, but > we're making a static list here, so there's really no support for > random, user-provided reset modules. So are we missing the mark on the > requirements? Well personally I do not have a MUST HAVE requirement for that feature. My main requirement was to find a way to stop DMA/IRQ transfers programmed by a previous VM potentially jeopardizing the integrity of a second VM. > > One way I thought you could achieve your requirement would be if we did > away with the lookup table and looked for the module and function using > a pre-defined transform on the compat ID. For instance, a compat ID of > "calxeda,hb-xgmac" would automatically request a module named > "vfio-platform-reset-calxedahb-xgmac" and look for a symbol of the same > name for the reset function (I wonder if we can actually have a module > and symbol of the same name). I just tried and it works It seems fairly safe since an external > module would need to be explicitly placed in the search path for the > userspace module loader. we talked together with Christoffer about such a technique at the very beginning and he was not very fond of it, advising an approach using a defined API. > > Otherwise the table would need to become a list, the external module > would need to be manually loaded, and the module_init() would need to > register an entry on that list. Isn't it what I attempted to do in v2 or do you mean something else? Having a whitelist brings the benefit to know which devices really are used with vfio-platform (besides some devices may not need any reset module). I don't know yet whether this is something that may slow down the adoption. Personally I would keep it as is for now and see how it gets used and whether users complain, ... If you agree I will respin just reworking the commit message. Best Regards Eric Thanks, > > Alex > >> Signed-off-by: Eric Auger <eric.auger@linaro.org> >> >> --- >> >> v2 -> v3: >> - add const in struct vfio_platform_reset_combo >> - remove enum vfio_platform_reset_type >> >> v2: creation >> --- >> drivers/vfio/platform/vfio_platform_common.c | 3 +++ >> drivers/vfio/platform/vfio_platform_private.h | 6 ++++++ >> 2 files changed, 9 insertions(+) >> >> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c >> index abcff7a..611597e 100644 >> --- a/drivers/vfio/platform/vfio_platform_common.c >> +++ b/drivers/vfio/platform/vfio_platform_common.c >> @@ -25,6 +25,9 @@ >> >> static DEFINE_MUTEX(driver_lock); >> >> +static const struct vfio_platform_reset_combo reset_lookup_table[] = { >> +}; >> + >> static int vfio_platform_regions_init(struct vfio_platform_device *vdev) >> { >> int cnt = 0, i; >> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h >> index 5d31e04..9e37b9f 100644 >> --- a/drivers/vfio/platform/vfio_platform_private.h >> +++ b/drivers/vfio/platform/vfio_platform_private.h >> @@ -69,6 +69,12 @@ struct vfio_platform_device { >> int (*get_irq)(struct vfio_platform_device *vdev, int i); >> }; >> >> +struct vfio_platform_reset_combo { >> + const char *compat; >> + const char *reset_function_name; >> + const char *module_name; >> +}; >> + >> extern int vfio_platform_probe_common(struct vfio_platform_device *vdev, >> struct device *dev); >> extern struct vfio_platform_device *vfio_platform_remove_common > > >
On Fri, 2015-06-12 at 15:41 +0200, Eric Auger wrote: > Hi Alex, > On 06/11/2015 11:11 PM, Alex Williamson wrote: > > On Thu, 2015-06-11 at 14:08 +0200, Eric Auger wrote: > >> This patch introduces the vfio_platform_reset_combo struct that > >> stores all the information useful to handle the reset modality: > >> compat string, name of the reset function, name of the module that > >> implements the reset function. A lookup table of such structures > >> is added, currently containing a single sentinel element. A new > >> type field is added in vfio_platform_device to store what kind of > >> reset is associated to the device, if any. > > > > The commit log no longer matches the code. > yes indeed missed that. > > > > The only other thing I'm struggling with in this series is that in 0/4 > > you suggest that the reset modules can be in-kernel or external, but > > we're making a static list here, so there's really no support for > > random, user-provided reset modules. So are we missing the mark on the > > requirements? > > Well personally I do not have a MUST HAVE requirement for that feature. > My main requirement was to find a way to stop DMA/IRQ transfers > programmed by a previous VM potentially jeopardizing the integrity of a > second VM. > > > > One way I thought you could achieve your requirement would be if we did > > away with the lookup table and looked for the module and function using > > a pre-defined transform on the compat ID. For instance, a compat ID of > > "calxeda,hb-xgmac" would automatically request a module named > > "vfio-platform-reset-calxedahb-xgmac" and look for a symbol of the same > > name for the reset function (I wonder if we can actually have a module > > and symbol of the same name). > I just tried and it works > It seems fairly safe since an external > > module would need to be explicitly placed in the search path for the > > userspace module loader. > we talked together with Christoffer about such a technique at the very > beginning and he was not very fond of it, advising an approach using a > defined API. > > > > Otherwise the table would need to become a list, the external module > > would need to be manually loaded, and the module_init() would need to > > register an entry on that list. > Isn't it what I attempted to do in v2 or do you mean something else? IIRC, v2 required all modules to be manually loaded, which I think was bound to be a support issue. In-kernel modules should definitely have the benefit of auto-loading, but if we want to support external modules, I'm just hypothesizing that we either need to standardize naming so that we can ask for them or require registration. In-kernel modules could still effectively be pre-registered and requested on-demand. > Having a whitelist brings the benefit to know which devices really are > used with vfio-platform (besides some devices may not need any reset > module). I don't know yet whether this is something that may slow down > the adoption. > > Personally I would keep it as is for now and see how it gets used and > whether users complain, ... > > If you agree I will respin just reworking the commit message. If there's really no external module requirement, then your current approach is fine. We're not creating anything here that can't later be modified. Thanks, Alex > >> --- > >> > >> v2 -> v3: > >> - add const in struct vfio_platform_reset_combo > >> - remove enum vfio_platform_reset_type > >> > >> v2: creation > >> --- > >> drivers/vfio/platform/vfio_platform_common.c | 3 +++ > >> drivers/vfio/platform/vfio_platform_private.h | 6 ++++++ > >> 2 files changed, 9 insertions(+) > >> > >> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c > >> index abcff7a..611597e 100644 > >> --- a/drivers/vfio/platform/vfio_platform_common.c > >> +++ b/drivers/vfio/platform/vfio_platform_common.c > >> @@ -25,6 +25,9 @@ > >> > >> static DEFINE_MUTEX(driver_lock); > >> > >> +static const struct vfio_platform_reset_combo reset_lookup_table[] = { > >> +}; > >> + > >> static int vfio_platform_regions_init(struct vfio_platform_device *vdev) > >> { > >> int cnt = 0, i; > >> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h > >> index 5d31e04..9e37b9f 100644 > >> --- a/drivers/vfio/platform/vfio_platform_private.h > >> +++ b/drivers/vfio/platform/vfio_platform_private.h > >> @@ -69,6 +69,12 @@ struct vfio_platform_device { > >> int (*get_irq)(struct vfio_platform_device *vdev, int i); > >> }; > >> > >> +struct vfio_platform_reset_combo { > >> + const char *compat; > >> + const char *reset_function_name; > >> + const char *module_name; > >> +}; > >> + > >> extern int vfio_platform_probe_common(struct vfio_platform_device *vdev, > >> struct device *dev); > >> extern struct vfio_platform_device *vfio_platform_remove_common > > > > > > >
Hi Alex, On 06/12/2015 04:19 PM, Alex Williamson wrote: > On Fri, 2015-06-12 at 15:41 +0200, Eric Auger wrote: >> Hi Alex, >> On 06/11/2015 11:11 PM, Alex Williamson wrote: >>> On Thu, 2015-06-11 at 14:08 +0200, Eric Auger wrote: >>>> This patch introduces the vfio_platform_reset_combo struct that >>>> stores all the information useful to handle the reset modality: >>>> compat string, name of the reset function, name of the module that >>>> implements the reset function. A lookup table of such structures >>>> is added, currently containing a single sentinel element. A new >>>> type field is added in vfio_platform_device to store what kind of >>>> reset is associated to the device, if any. >>> >>> The commit log no longer matches the code. >> yes indeed missed that. >>> >>> The only other thing I'm struggling with in this series is that in 0/4 >>> you suggest that the reset modules can be in-kernel or external, but >>> we're making a static list here, so there's really no support for >>> random, user-provided reset modules. So are we missing the mark on the >>> requirements? >> >> Well personally I do not have a MUST HAVE requirement for that feature. >> My main requirement was to find a way to stop DMA/IRQ transfers >> programmed by a previous VM potentially jeopardizing the integrity of a >> second VM. >>> >>> One way I thought you could achieve your requirement would be if we did >>> away with the lookup table and looked for the module and function using >>> a pre-defined transform on the compat ID. For instance, a compat ID of >>> "calxeda,hb-xgmac" would automatically request a module named >>> "vfio-platform-reset-calxedahb-xgmac" and look for a symbol of the same >>> name for the reset function (I wonder if we can actually have a module >>> and symbol of the same name). >> I just tried and it works >> It seems fairly safe since an external >>> module would need to be explicitly placed in the search path for the >>> userspace module loader. >> we talked together with Christoffer about such a technique at the very >> beginning and he was not very fond of it, advising an approach using a >> defined API. >>> >>> Otherwise the table would need to become a list, the external module >>> would need to be manually loaded, and the module_init() would need to >>> register an entry on that list. >> Isn't it what I attempted to do in v2 or do you mean something else? > > IIRC, v2 required all modules to be manually loaded, which I think was > bound to be a support issue. In-kernel modules should definitely have > the benefit of auto-loading, yes you're right! thanks for your clarification. but if we want to support external modules, > I'm just hypothesizing that we either need to standardize naming so that > we can ask for them or require registration. In-kernel modules could > still effectively be pre-registered and requested on-demand. >> Having a whitelist brings the benefit to know which devices really are >> used with vfio-platform (besides some devices may not need any reset >> module). I don't know yet whether this is something that may slow down >> the adoption. >> >> Personally I would keep it as is for now and see how it gets used and >> whether users complain, ... >> >> If you agree I will respin just reworking the commit message. > > If there's really no external module requirement, then your current > approach is fine. We're not creating anything here that can't later be > modified. Thanks, OK. I will send v4 with updated commit messages early next week. Have a nice week end. Eric > > Alex > > >>>> --- >>>> >>>> v2 -> v3: >>>> - add const in struct vfio_platform_reset_combo >>>> - remove enum vfio_platform_reset_type >>>> >>>> v2: creation >>>> --- >>>> drivers/vfio/platform/vfio_platform_common.c | 3 +++ >>>> drivers/vfio/platform/vfio_platform_private.h | 6 ++++++ >>>> 2 files changed, 9 insertions(+) >>>> >>>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c >>>> index abcff7a..611597e 100644 >>>> --- a/drivers/vfio/platform/vfio_platform_common.c >>>> +++ b/drivers/vfio/platform/vfio_platform_common.c >>>> @@ -25,6 +25,9 @@ >>>> >>>> static DEFINE_MUTEX(driver_lock); >>>> >>>> +static const struct vfio_platform_reset_combo reset_lookup_table[] = { >>>> +}; >>>> + >>>> static int vfio_platform_regions_init(struct vfio_platform_device *vdev) >>>> { >>>> int cnt = 0, i; >>>> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h >>>> index 5d31e04..9e37b9f 100644 >>>> --- a/drivers/vfio/platform/vfio_platform_private.h >>>> +++ b/drivers/vfio/platform/vfio_platform_private.h >>>> @@ -69,6 +69,12 @@ struct vfio_platform_device { >>>> int (*get_irq)(struct vfio_platform_device *vdev, int i); >>>> }; >>>> >>>> +struct vfio_platform_reset_combo { >>>> + const char *compat; >>>> + const char *reset_function_name; >>>> + const char *module_name; >>>> +}; >>>> + >>>> extern int vfio_platform_probe_common(struct vfio_platform_device *vdev, >>>> struct device *dev); >>>> extern struct vfio_platform_device *vfio_platform_remove_common >>> >>> >>> >> > > >
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c index abcff7a..611597e 100644 --- a/drivers/vfio/platform/vfio_platform_common.c +++ b/drivers/vfio/platform/vfio_platform_common.c @@ -25,6 +25,9 @@ static DEFINE_MUTEX(driver_lock); +static const struct vfio_platform_reset_combo reset_lookup_table[] = { +}; + static int vfio_platform_regions_init(struct vfio_platform_device *vdev) { int cnt = 0, i; diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h index 5d31e04..9e37b9f 100644 --- a/drivers/vfio/platform/vfio_platform_private.h +++ b/drivers/vfio/platform/vfio_platform_private.h @@ -69,6 +69,12 @@ struct vfio_platform_device { int (*get_irq)(struct vfio_platform_device *vdev, int i); }; +struct vfio_platform_reset_combo { + const char *compat; + const char *reset_function_name; + const char *module_name; +}; + extern int vfio_platform_probe_common(struct vfio_platform_device *vdev, struct device *dev); extern struct vfio_platform_device *vfio_platform_remove_common
This patch introduces the vfio_platform_reset_combo struct that stores all the information useful to handle the reset modality: compat string, name of the reset function, name of the module that implements the reset function. A lookup table of such structures is added, currently containing a single sentinel element. A new type field is added in vfio_platform_device to store what kind of reset is associated to the device, if any. Signed-off-by: Eric Auger <eric.auger@linaro.org> --- v2 -> v3: - add const in struct vfio_platform_reset_combo - remove enum vfio_platform_reset_type v2: creation --- drivers/vfio/platform/vfio_platform_common.c | 3 +++ drivers/vfio/platform/vfio_platform_private.h | 6 ++++++ 2 files changed, 9 insertions(+)