Re: [PATCH] vfio: platform: Fix using devices in PM Domains
Hi Eric, On Fri, Apr 13, 2018 at 11:44 AM, Auger Eric wrote: > On 13/04/18 11:19, Geert Uytterhoeven wrote: >> On Fri, Apr 13, 2018 at 11:14 AM, Auger Eric wrote: >>> On 11/04/18 11:24, Geert Uytterhoeven wrote: If a device is part of a PM Domain (e.g. power and/or clock domain), its power state is managed using Runtime PM. Without Runtime PM, the device may not be powered up, causing subtle failures, crashes, or system lock-ups when the device is accessed by the guest. >>> the device may not be powered up/clcoked or power/clock may be switched >>> off while the guest uses it. Fix this by adding Runtime PM support, powering the device when the VFIO device is opened by the guest. Note that while more fine-grained power management could be implemented on the guest side, if exported, this would be inherently unsafe, as abusing it may kill the whole system. >>> >>> Please can you elaborate on this remark please? >> >> If power-management of the device would be delegated to the guest, and the >> guest forgets to enable device power before accessing the device's registers, >> this could lock up the system, and thus disturb both the host and other >> guests. > > Wouldn't you need to assign another device or use para-virt to allow the > guest to perform this power management control? I think you can remove > this paragraph from the commit message. Yes, that needs additional exporting of power-management (cfr. the "if exported"). I just wanted to mention the caveat, as eventually, someone is going to suggest doing this ;-) But if you think the comment should be removed, I can of course do so. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH] vfio: platform: Fix using devices in PM Domains
Hi Geert, On 13/04/18 11:19, Geert Uytterhoeven wrote: > Hi Eric, > > On Fri, Apr 13, 2018 at 11:14 AM, Auger Eric wrote: >> On 11/04/18 11:24, Geert Uytterhoeven wrote: >>> If a device is part of a PM Domain (e.g. power and/or clock domain), its >>> power state is managed using Runtime PM. Without Runtime PM, the device >>> may not be powered up, causing subtle failures, crashes, or system >>> lock-ups when the device is accessed by the guest. >> the device may not be powered up/clcoked or power/clock may be switched >> off while the guest uses it. >>> >>> Fix this by adding Runtime PM support, powering the device when the VFIO >>> device is opened by the guest. >>> >>> Note that while more fine-grained power management could be implemented >>> on the guest side, if exported, this would be inherently unsafe, as >>> abusing it may kill the whole system. >> >> Please can you elaborate on this remark please? > > If power-management of the device would be delegated to the guest, and the > guest forgets to enable device power before accessing the device's registers, > this could lock up the system, and thus disturb both the host and other > guests. Wouldn't you need to assign another device or use para-virt to allow the guest to perform this power management control? I think you can remove this paragraph from the commit message. Thanks Eric > > Gr{oetje,eeting}s, > > Geert >
Re: [PATCH] vfio: platform: Fix using devices in PM Domains
Hi Eric, On Fri, Apr 13, 2018 at 11:14 AM, Auger Eric wrote: > On 11/04/18 11:24, Geert Uytterhoeven wrote: >> If a device is part of a PM Domain (e.g. power and/or clock domain), its >> power state is managed using Runtime PM. Without Runtime PM, the device >> may not be powered up, causing subtle failures, crashes, or system >> lock-ups when the device is accessed by the guest. > the device may not be powered up/clcoked or power/clock may be switched > off while the guest uses it. >> >> Fix this by adding Runtime PM support, powering the device when the VFIO >> device is opened by the guest. >> >> Note that while more fine-grained power management could be implemented >> on the guest side, if exported, this would be inherently unsafe, as >> abusing it may kill the whole system. > > Please can you elaborate on this remark please? If power-management of the device would be delegated to the guest, and the guest forgets to enable device power before accessing the device's registers, this could lock up the system, and thus disturb both the host and other guests. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH] vfio: platform: Fix using devices in PM Domains
Hi Geert, On 11/04/18 11:24, Geert Uytterhoeven wrote: > If a device is part of a PM Domain (e.g. power and/or clock domain), its > power state is managed using Runtime PM. Without Runtime PM, the device > may not be powered up, causing subtle failures, crashes, or system > lock-ups when the device is accessed by the guest. the device may not be powered up/clcoked or power/clock may be switched off while the guest uses it. > > Fix this by adding Runtime PM support, powering the device when the VFIO > device is opened by the guest. > > Note that while more fine-grained power management could be implemented > on the guest side, if exported, this would be inherently unsafe, as > abusing it may kill the whole system. Please can you elaborate on this remark please? Thanks Eric > > Signed-off-by: Geert Uytterhoeven > --- > This depends on "[PATCH v3 2/2] vfio: platform: Add generic DT reset > support" due to a small contextual change (addition of "#include > "). > > drivers/vfio/platform/vfio_platform_common.c | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/drivers/vfio/platform/vfio_platform_common.c > b/drivers/vfio/platform/vfio_platform_common.c > index ef9b9e3220ebe939..4db0a143992c3353 100644 > --- a/drivers/vfio/platform/vfio_platform_common.c > +++ b/drivers/vfio/platform/vfio_platform_common.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -249,6 +250,8 @@ static void vfio_platform_release(void *device_data) > const char *extra_dbg = NULL; > int ret; > > + pm_runtime_put(vdev->device); > + > ret = vfio_platform_call_reset(vdev, &extra_dbg); > if (ret && vdev->reset_required) { > dev_warn(vdev->device, "reset driver is required and > reset call failed in release (%d) %s\n", > @@ -291,6 +294,10 @@ static int vfio_platform_open(void *device_data) >ret, extra_dbg ? extra_dbg : ""); > goto err_rst; > } > + > + ret = pm_runtime_get_sync(vdev->device); > + if (ret < 0) > + goto err_rst; > } > > vdev->refcnt++; > @@ -706,6 +713,7 @@ int vfio_platform_probe_common(struct > vfio_platform_device *vdev, > > mutex_init(&vdev->igate); > > + pm_runtime_enable(vdev->device); > return 0; > > put_iommu: > @@ -723,6 +731,7 @@ struct vfio_platform_device > *vfio_platform_remove_common(struct device *dev) > vdev = vfio_del_group_dev(dev); > > if (vdev) { > + pm_runtime_disable(vdev->device); > vfio_platform_put_reset(vdev); > vfio_iommu_group_put(dev->iommu_group, dev); > } >
Re: [PATCH] vfio: platform: Fix using devices in PM Domains
On Wed, Apr 11, 2018 at 11:24:08AM +0200, Geert Uytterhoeven wrote: > If a device is part of a PM Domain (e.g. power and/or clock domain), its > power state is managed using Runtime PM. Without Runtime PM, the device > may not be powered up, causing subtle failures, crashes, or system > lock-ups when the device is accessed by the guest. > > Fix this by adding Runtime PM support, powering the device when the VFIO > device is opened by the guest. > > Note that while more fine-grained power management could be implemented > on the guest side, if exported, this would be inherently unsafe, as > abusing it may kill the whole system. > > Signed-off-by: Geert Uytterhoeven Reviewed-by: Simon Horman
[PATCH] vfio: platform: Fix using devices in PM Domains
If a device is part of a PM Domain (e.g. power and/or clock domain), its power state is managed using Runtime PM. Without Runtime PM, the device may not be powered up, causing subtle failures, crashes, or system lock-ups when the device is accessed by the guest. Fix this by adding Runtime PM support, powering the device when the VFIO device is opened by the guest. Note that while more fine-grained power management could be implemented on the guest side, if exported, this would be inherently unsafe, as abusing it may kill the whole system. Signed-off-by: Geert Uytterhoeven --- This depends on "[PATCH v3 2/2] vfio: platform: Add generic DT reset support" due to a small contextual change (addition of "#include "). drivers/vfio/platform/vfio_platform_common.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c index ef9b9e3220ebe939..4db0a143992c3353 100644 --- a/drivers/vfio/platform/vfio_platform_common.c +++ b/drivers/vfio/platform/vfio_platform_common.c @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -249,6 +250,8 @@ static void vfio_platform_release(void *device_data) const char *extra_dbg = NULL; int ret; + pm_runtime_put(vdev->device); + ret = vfio_platform_call_reset(vdev, &extra_dbg); if (ret && vdev->reset_required) { dev_warn(vdev->device, "reset driver is required and reset call failed in release (%d) %s\n", @@ -291,6 +294,10 @@ static int vfio_platform_open(void *device_data) ret, extra_dbg ? extra_dbg : ""); goto err_rst; } + + ret = pm_runtime_get_sync(vdev->device); + if (ret < 0) + goto err_rst; } vdev->refcnt++; @@ -706,6 +713,7 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, mutex_init(&vdev->igate); + pm_runtime_enable(vdev->device); return 0; put_iommu: @@ -723,6 +731,7 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev) vdev = vfio_del_group_dev(dev); if (vdev) { + pm_runtime_disable(vdev->device); vfio_platform_put_reset(vdev); vfio_iommu_group_put(dev->iommu_group, dev); } -- 2.7.4
Re: [PATCH] vfio: platform: Fix using devices in PM Domains
On Wed, Apr 11, 2018 at 11:15 AM, Geert Uytterhoeven wrote: > If a device is part of a PM Domain (e.g. power and/or clock domain), its > power state is managed using Runtime PM. Without Runtime PM, the device > may not be powered up, causing subtle failures, crashes, or system > lock-ups when the device is accessed by the guest. > > Fix this by adding Runtime PM support, powering the device when the VFIO > device is opened by the guest. > > Note that while more fine-grained power management could be implemented > on the guest side, if exported, this would be inherently unsafe, as > abusing it may kill the whole system. > > Signed-off-by: Geert Uytterhoeven Please ignore, this was sent accidentally with another series. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
[PATCH] vfio: platform: Fix using devices in PM Domains
If a device is part of a PM Domain (e.g. power and/or clock domain), its power state is managed using Runtime PM. Without Runtime PM, the device may not be powered up, causing subtle failures, crashes, or system lock-ups when the device is accessed by the guest. Fix this by adding Runtime PM support, powering the device when the VFIO device is opened by the guest. Note that while more fine-grained power management could be implemented on the guest side, if exported, this would be inherently unsafe, as abusing it may kill the whole system. Signed-off-by: Geert Uytterhoeven --- drivers/vfio/platform/vfio_platform_common.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c index 3c13327b2777f8ec..9d46fce2446daebe 100644 --- a/drivers/vfio/platform/vfio_platform_common.c +++ b/drivers/vfio/platform/vfio_platform_common.c @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -255,6 +256,8 @@ static void vfio_platform_release(void *device_data) const char *extra_dbg = NULL; int ret; + pm_runtime_put(vdev->device); + ret = vfio_platform_call_reset(vdev, &extra_dbg); if (ret && vdev->reset_required) { dev_warn(vdev->device, "reset driver is required and reset call failed in release (%d) %s\n", @@ -297,6 +300,10 @@ static int vfio_platform_open(void *device_data) ret, extra_dbg ? extra_dbg : ""); goto err_rst; } + + ret = pm_runtime_get_sync(vdev->device); + if (ret < 0) + goto err_rst; } vdev->refcnt++; @@ -712,6 +719,7 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, mutex_init(&vdev->igate); + pm_runtime_enable(vdev->device); return 0; put_iommu: @@ -729,6 +737,7 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev) vdev = vfio_del_group_dev(dev); if (vdev) { + pm_runtime_disable(vdev->device); vfio_platform_put_reset(vdev); vfio_iommu_group_put(dev->iommu_group, dev); } -- 2.7.4