| Message ID | 1466437879-32182-8-git-send-email-okaya@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 20 Jun 2016 11:51:17 -0400 Sinan Kaya <okaya@codeaurora.org> wrote: > The code was allowing platform devices to be used without a supporting > VFIO reset driver. The hardware can be left in some inconsistent state > after a guest machine abort. > > The reset driver will put the hardware back to safe state and disable > interrupts before returning the control back to the host machine. > > Adding a new reset_required kernel module option to AMBA and platform > VFIO drivers with a default value of true. > > New requirements are: > 1. A reset function needs to be implemented by the corresponding driver > via DT/ACPI. > 2. The reset function needs to be discovered via DT/ACPI. > > The probe of the driver will fail if any of the above conditions are > not satisfied. > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > drivers/vfio/platform/vfio_amba.c | 5 +++++ > drivers/vfio/platform/vfio_platform.c | 5 +++++ > drivers/vfio/platform/vfio_platform_common.c | 24 ++++++++++++++++-------- > drivers/vfio/platform/vfio_platform_private.h | 1 + > 4 files changed, 27 insertions(+), 8 deletions(-) > > diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c > index a66479b..7585902 100644 > --- a/drivers/vfio/platform/vfio_amba.c > +++ b/drivers/vfio/platform/vfio_amba.c > @@ -23,6 +23,10 @@ > #define DRIVER_AUTHOR "Antonios Motakis <a.motakis@virtualopensystems.com>" > #define DRIVER_DESC "VFIO for AMBA devices - User Level meta-driver" > > +static bool reset_required = true; > +module_param(reset_required, bool, 0644); > +MODULE_PARM_DESC(reset_required, "override reset requirement (default: 1)"); > + > /* probing devices from the AMBA bus */ > > static struct resource *get_amba_resource(struct vfio_platform_device *vdev, > @@ -68,6 +72,7 @@ static int vfio_amba_probe(struct amba_device *adev, const struct amba_id *id) > vdev->get_resource = get_amba_resource; > vdev->get_irq = get_amba_irq; > vdev->parent_module = THIS_MODULE; > + vdev->reset_required = reset_required; > > ret = vfio_platform_probe_common(vdev, &adev->dev); > if (ret) { > diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c > index b1cc3a7..ef89146 100644 > --- a/drivers/vfio/platform/vfio_platform.c > +++ b/drivers/vfio/platform/vfio_platform.c > @@ -23,6 +23,10 @@ > #define DRIVER_AUTHOR "Antonios Motakis <a.motakis@virtualopensystems.com>" > #define DRIVER_DESC "VFIO for platform devices - User Level meta-driver" > > +static bool reset_required = true; > +module_param(reset_required, bool, 0644); > +MODULE_PARM_DESC(reset_required, "override reset requirement (default: 1)"); > + > /* probing devices from the linux platform bus */ > > static struct resource *get_platform_resource(struct vfio_platform_device *vdev, > @@ -66,6 +70,7 @@ static int vfio_platform_probe(struct platform_device *pdev) > vdev->get_resource = get_platform_resource; > vdev->get_irq = get_platform_irq; > vdev->parent_module = THIS_MODULE; > + vdev->reset_required = reset_required; Do you see value in making the global reset_required changeable, with the behavior of any given device dependent on the setting of this variable at the time of probe? It seems like a bit of a support issue to me. Also, we're breaking existing users if there are any with this change. Should we introduce a CONFIG option to set the default? I think we can get away with changing the default that way, but I'm not so sure otherwise. > > ret = vfio_platform_probe_common(vdev, &pdev->dev); > if (ret) > diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c > index 1d771a6..b9ffed4 100644 > --- a/drivers/vfio/platform/vfio_platform_common.c > +++ b/drivers/vfio/platform/vfio_platform_common.c > @@ -128,10 +128,10 @@ static bool vfio_platform_has_reset(struct vfio_platform_device *vdev) > return vdev->of_reset ? true : false; > } > > -static void vfio_platform_get_reset(struct vfio_platform_device *vdev) > +static int vfio_platform_get_reset(struct vfio_platform_device *vdev) > { > if (vdev->acpihid) > - return; > + return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT; > > vdev->of_reset = vfio_platform_lookup_reset(vdev->compat, > &vdev->reset_module); > @@ -140,6 +140,8 @@ static void vfio_platform_get_reset(struct vfio_platform_device *vdev) > vdev->of_reset = vfio_platform_lookup_reset(vdev->compat, > &vdev->reset_module); > } > + > + return vdev->of_reset ? 0 : -ENOENT; > } nit, this looks more like a: static bool vfio_platform_has_reset(...) ... return vfio_platform_acpi_has_reset() == 0; ... return vdev->of_reset != NULL > > static void vfio_platform_put_reset(struct vfio_platform_device *vdev) > @@ -663,6 +665,13 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, > > vdev->device = dev; > > + ret = vfio_platform_get_reset(vdev); > + if (ret && vdev->reset_required) { > + pr_err("vfio: no reset function found for device %s\n", > + vdev->name); > + return ret; > + } > + > group = iommu_group_get(dev); > if (!group) { > pr_err("VFIO: No IOMMU group for device %s\n", vdev->name); > @@ -670,16 +679,15 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, > } > > ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev); > - if (ret) { > - iommu_group_put(group); > - return ret; > - } > - > - vfio_platform_get_reset(vdev); > + if (ret) > + goto out; > > mutex_init(&vdev->igate); > > return 0; > +out: > + iommu_group_put(group); > + return ret; > } > EXPORT_SYMBOL_GPL(vfio_platform_probe_common); > > diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h > index ba9e4f8..68fbc00 100644 > --- a/drivers/vfio/platform/vfio_platform_private.h > +++ b/drivers/vfio/platform/vfio_platform_private.h > @@ -50,6 +50,7 @@ struct vfio_platform_region { > }; > > struct vfio_platform_device { > + bool reset_required; > struct vfio_platform_region *regions; > u32 num_regions; > struct vfio_platform_irq *irqs; -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/23/2016 2:59 PM, Alex Williamson wrote: >> > static struct resource *get_platform_resource(struct vfio_platform_device *vdev, >> > @@ -66,6 +70,7 @@ static int vfio_platform_probe(struct platform_device *pdev) >> > vdev->get_resource = get_platform_resource; >> > vdev->get_irq = get_platform_irq; >> > vdev->parent_module = THIS_MODULE; >> > + vdev->reset_required = reset_required; > > Do you see value in making the global reset_required changeable, with > the behavior of any given device dependent on the setting of this > variable at the time of probe? It seems like a bit of a support issue > to me. Also, we're breaking existing users if there are any with this > change. Should we introduce a CONFIG option to set the default? I > think we can get away with changing the default that way, but I'm not > so sure otherwise. > We have two groups of existing users. 1. AMBA based drivers 2. DT based drivers and now we are trying to add the ACPI based drivers in this series. The AMBA based drivers do not have reset function implemented. Based on previous conversation with Eric, these devices were mostly used for bringing up the VFIO framework and were not intended for production. If we want to maintain existing functionality, I can change reset_required to false by default for the AMBA based drivers. The DT based drivers all have reset functions implemented. They shouldn't be impacted by the reset_required flag. The reset_required flag is again useful for testing purposes when the reset driver is broken or the ACPI _RST method is missing. The previously agreed approach was to force the reset required by default for production environment and be able to clear it for testing purposes. When I was implementing HIDMA, I never realized that I needed a reset driver until Arnd told me during the review. We want to avoid this for the long term for DT and ACPI based implementations. The reset_required command line parameter would be useful if somebody suspects that the ACPI _RST implementation is broken or the DT based reset driver is broken or you quickly want to test the virtualization without having a reset driver ready yet. Let us know which way you want to go. I can also add a Kconfig option and set it by default. But then I have to recompile the kernel when I want to test without the reset stuff.
On 6/23/2016 2:59 PM, Alex Williamson wrote: >> -static void vfio_platform_get_reset(struct vfio_platform_device *vdev) >> > +static int vfio_platform_get_reset(struct vfio_platform_device *vdev) >> > { >> > if (vdev->acpihid) >> > - return; >> > + return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT; >> > >> > vdev->of_reset = vfio_platform_lookup_reset(vdev->compat, >> > &vdev->reset_module); >> > @@ -140,6 +140,8 @@ static void vfio_platform_get_reset(struct vfio_platform_device *vdev) >> > vdev->of_reset = vfio_platform_lookup_reset(vdev->compat, >> > &vdev->reset_module); >> > } >> > + >> > + return vdev->of_reset ? 0 : -ENOENT; >> > } > nit, this looks more like a: > > static bool vfio_platform_has_reset(...) > ... > return vfio_platform_acpi_has_reset() == 0; > > ... > > return vdev->of_reset != NULL > Sorry, I didn't understand this comment. The code has get and put functions for DT. These functions are not useful for ACPI. This is the reason for the above change. Can you be more specific?
On Wed, 13 Jul 2016 16:12:35 -0400 Sinan Kaya <okaya@codeaurora.org> wrote: > On 6/23/2016 2:59 PM, Alex Williamson wrote: > >> > static struct resource *get_platform_resource(struct vfio_platform_device *vdev, > >> > @@ -66,6 +70,7 @@ static int vfio_platform_probe(struct platform_device *pdev) > >> > vdev->get_resource = get_platform_resource; > >> > vdev->get_irq = get_platform_irq; > >> > vdev->parent_module = THIS_MODULE; > >> > + vdev->reset_required = reset_required; > > > > Do you see value in making the global reset_required changeable, with > > the behavior of any given device dependent on the setting of this > > variable at the time of probe? It seems like a bit of a support issue > > to me. Also, we're breaking existing users if there are any with this > > change. Should we introduce a CONFIG option to set the default? I > > think we can get away with changing the default that way, but I'm not > > so sure otherwise. > > > > We have two groups of existing users. > > 1. AMBA based drivers > 2. DT based drivers > > and now we are trying to add the ACPI based drivers in this series. > > The AMBA based drivers do not have reset function implemented. Based on > previous conversation with Eric, these devices were mostly used for > bringing up the VFIO framework and were not intended for production. > If we want to maintain existing functionality, I can change reset_required to > false by default for the AMBA based drivers. I think we need to consider them to be in production at this point, so probably better to make such a change. > The DT based drivers all have reset functions implemented. They shouldn't be > impacted by the reset_required flag. Ok, so we're fine there. > The reset_required flag is again useful for testing purposes when the reset > driver is broken or the ACPI _RST method is missing. I don't doubt that, but it doesn't need to be mode 644 for that, which allows changing the default dynamically. We could make it 444 so that it can be set at module load time and not modified. I just don't want to try to guess the state of that variable at the time the device was probed. > The previously agreed approach was to force the reset required by default > for production environment and be able to clear it for testing purposes. > When I was implementing HIDMA, I never realized that I needed a reset driver > until Arnd told me during the review. We want to avoid this for the long > term for DT and ACPI based implementations. I agree, but we don't need to make it dynamically changeable for that. Also, nothing prevents us from printing a warning when a device is probed w/o a reset function, it's just a matter of whether that causes a probe failure or a complain and continue. > The reset_required command line parameter would be useful if somebody suspects > that the ACPI _RST implementation is broken or the DT based reset driver is > broken or you quickly want to test the virtualization without having a reset > driver ready yet. > > Let us know which way you want to go. I can also add a Kconfig option and > set it by default. But then I have to recompile the kernel when I want to > test without the reset stuff. Seems like we don't need a Kconfig, but I don't see why the option needs to be settable except at module load time and we can complain either way to clue in developers and catch such things in testing. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 13 Jul 2016 16:48:32 -0400 Sinan Kaya <okaya@codeaurora.org> wrote: > On 6/23/2016 2:59 PM, Alex Williamson wrote: > >> -static void vfio_platform_get_reset(struct vfio_platform_device *vdev) > >> > +static int vfio_platform_get_reset(struct vfio_platform_device *vdev) > >> > { > >> > if (vdev->acpihid) > >> > - return; > >> > + return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT; > >> > > >> > vdev->of_reset = vfio_platform_lookup_reset(vdev->compat, > >> > &vdev->reset_module); > >> > @@ -140,6 +140,8 @@ static void vfio_platform_get_reset(struct vfio_platform_device *vdev) > >> > vdev->of_reset = vfio_platform_lookup_reset(vdev->compat, > >> > &vdev->reset_module); > >> > } > >> > + > >> > + return vdev->of_reset ? 0 : -ENOENT; > >> > } > > nit, this looks more like a: > > > > static bool vfio_platform_has_reset(...) > > ... > > return vfio_platform_acpi_has_reset() == 0; > > > > ... > > > > return vdev->of_reset != NULL > > > > Sorry, I didn't understand this comment. The code has get and put functions for DT. > These functions are not useful for ACPI. This is the reason for the above change. > > Can you be more specific? It was sort of cryptic, I'm not entirely sure I can make sense of it either. It think I was mainly suggesting that it looked more like a bool function so we could just return true/false, but we are actually setting the of_reset function as part of this, so there is a 'get' aspect. Feel free to ignore this comment. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/13/2016 4:55 PM, Alex Williamson wrote: > On Wed, 13 Jul 2016 16:12:35 -0400 > Sinan Kaya <okaya@codeaurora.org> wrote: > >> On 6/23/2016 2:59 PM, Alex Williamson wrote: >>>>> static struct resource *get_platform_resource(struct vfio_platform_device *vdev, >>>>> @@ -66,6 +70,7 @@ static int vfio_platform_probe(struct platform_device *pdev) >>>>> vdev->get_resource = get_platform_resource; >>>>> vdev->get_irq = get_platform_irq; >>>>> vdev->parent_module = THIS_MODULE; >>>>> + vdev->reset_required = reset_required; >>> >>> Do you see value in making the global reset_required changeable, with >>> the behavior of any given device dependent on the setting of this >>> variable at the time of probe? It seems like a bit of a support issue >>> to me. Also, we're breaking existing users if there are any with this >>> change. Should we introduce a CONFIG option to set the default? I >>> think we can get away with changing the default that way, but I'm not >>> so sure otherwise. >>> >> >> We have two groups of existing users. >> >> 1. AMBA based drivers >> 2. DT based drivers >> >> and now we are trying to add the ACPI based drivers in this series. >> >> The AMBA based drivers do not have reset function implemented. Based on >> previous conversation with Eric, these devices were mostly used for >> bringing up the VFIO framework and were not intended for production. >> If we want to maintain existing functionality, I can change reset_required to >> false by default for the AMBA based drivers. > > I think we need to consider them to be in production at this point, so > probably better to make such a change. > >> The DT based drivers all have reset functions implemented. They shouldn't be >> impacted by the reset_required flag. > > Ok, so we're fine there. > >> The reset_required flag is again useful for testing purposes when the reset >> driver is broken or the ACPI _RST method is missing. > > I don't doubt that, but it doesn't need to be mode 644 for that, which > allows changing the default dynamically. We could make it 444 so that > it can be set at module load time and not modified. I just don't want > to try to guess the state of that variable at the time the device was > probed. OK. I'll change it to 444. > >> The previously agreed approach was to force the reset required by default >> for production environment and be able to clear it for testing purposes. >> When I was implementing HIDMA, I never realized that I needed a reset driver >> until Arnd told me during the review. We want to avoid this for the long >> term for DT and ACPI based implementations. > > I agree, but we don't need to make it dynamically changeable for that. > Also, nothing prevents us from printing a warning when a device is > probed w/o a reset function, it's just a matter of whether that causes > a probe failure or a complain and continue. > >> The reset_required command line parameter would be useful if somebody suspects >> that the ACPI _RST implementation is broken or the DT based reset driver is >> broken or you quickly want to test the virtualization without having a reset >> driver ready yet. >> >> Let us know which way you want to go. I can also add a Kconfig option and >> set it by default. But then I have to recompile the kernel when I want to >> test without the reset stuff. > > Seems like we don't need a Kconfig, but I don't see why the option > needs to be settable except at module load time and we can complain > either way to clue in developers and catch such things in testing. > Thanks, OK. > > Alex >
diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c index a66479b..7585902 100644 --- a/drivers/vfio/platform/vfio_amba.c +++ b/drivers/vfio/platform/vfio_amba.c @@ -23,6 +23,10 @@ #define DRIVER_AUTHOR "Antonios Motakis <a.motakis@virtualopensystems.com>" #define DRIVER_DESC "VFIO for AMBA devices - User Level meta-driver" +static bool reset_required = true; +module_param(reset_required, bool, 0644); +MODULE_PARM_DESC(reset_required, "override reset requirement (default: 1)"); + /* probing devices from the AMBA bus */ static struct resource *get_amba_resource(struct vfio_platform_device *vdev, @@ -68,6 +72,7 @@ static int vfio_amba_probe(struct amba_device *adev, const struct amba_id *id) vdev->get_resource = get_amba_resource; vdev->get_irq = get_amba_irq; vdev->parent_module = THIS_MODULE; + vdev->reset_required = reset_required; ret = vfio_platform_probe_common(vdev, &adev->dev); if (ret) { diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c index b1cc3a7..ef89146 100644 --- a/drivers/vfio/platform/vfio_platform.c +++ b/drivers/vfio/platform/vfio_platform.c @@ -23,6 +23,10 @@ #define DRIVER_AUTHOR "Antonios Motakis <a.motakis@virtualopensystems.com>" #define DRIVER_DESC "VFIO for platform devices - User Level meta-driver" +static bool reset_required = true; +module_param(reset_required, bool, 0644); +MODULE_PARM_DESC(reset_required, "override reset requirement (default: 1)"); + /* probing devices from the linux platform bus */ static struct resource *get_platform_resource(struct vfio_platform_device *vdev, @@ -66,6 +70,7 @@ static int vfio_platform_probe(struct platform_device *pdev) vdev->get_resource = get_platform_resource; vdev->get_irq = get_platform_irq; vdev->parent_module = THIS_MODULE; + vdev->reset_required = reset_required; ret = vfio_platform_probe_common(vdev, &pdev->dev); if (ret) diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c index 1d771a6..b9ffed4 100644 --- a/drivers/vfio/platform/vfio_platform_common.c +++ b/drivers/vfio/platform/vfio_platform_common.c @@ -128,10 +128,10 @@ static bool vfio_platform_has_reset(struct vfio_platform_device *vdev) return vdev->of_reset ? true : false; } -static void vfio_platform_get_reset(struct vfio_platform_device *vdev) +static int vfio_platform_get_reset(struct vfio_platform_device *vdev) { if (vdev->acpihid) - return; + return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT; vdev->of_reset = vfio_platform_lookup_reset(vdev->compat, &vdev->reset_module); @@ -140,6 +140,8 @@ static void vfio_platform_get_reset(struct vfio_platform_device *vdev) vdev->of_reset = vfio_platform_lookup_reset(vdev->compat, &vdev->reset_module); } + + return vdev->of_reset ? 0 : -ENOENT; } static void vfio_platform_put_reset(struct vfio_platform_device *vdev) @@ -663,6 +665,13 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, vdev->device = dev; + ret = vfio_platform_get_reset(vdev); + if (ret && vdev->reset_required) { + pr_err("vfio: no reset function found for device %s\n", + vdev->name); + return ret; + } + group = iommu_group_get(dev); if (!group) { pr_err("VFIO: No IOMMU group for device %s\n", vdev->name); @@ -670,16 +679,15 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, } ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev); - if (ret) { - iommu_group_put(group); - return ret; - } - - vfio_platform_get_reset(vdev); + if (ret) + goto out; mutex_init(&vdev->igate); return 0; +out: + iommu_group_put(group); + return ret; } EXPORT_SYMBOL_GPL(vfio_platform_probe_common); diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h index ba9e4f8..68fbc00 100644 --- a/drivers/vfio/platform/vfio_platform_private.h +++ b/drivers/vfio/platform/vfio_platform_private.h @@ -50,6 +50,7 @@ struct vfio_platform_region { }; struct vfio_platform_device { + bool reset_required; struct vfio_platform_region *regions; u32 num_regions; struct vfio_platform_irq *irqs;
The code was allowing platform devices to be used without a supporting VFIO reset driver. The hardware can be left in some inconsistent state after a guest machine abort. The reset driver will put the hardware back to safe state and disable interrupts before returning the control back to the host machine. Adding a new reset_required kernel module option to AMBA and platform VFIO drivers with a default value of true. New requirements are: 1. A reset function needs to be implemented by the corresponding driver via DT/ACPI. 2. The reset function needs to be discovered via DT/ACPI. The probe of the driver will fail if any of the above conditions are not satisfied. Signed-off-by: Sinan Kaya <okaya@codeaurora.org> --- drivers/vfio/platform/vfio_amba.c | 5 +++++ drivers/vfio/platform/vfio_platform.c | 5 +++++ drivers/vfio/platform/vfio_platform_common.c | 24 ++++++++++++++++-------- drivers/vfio/platform/vfio_platform_private.h | 1 + 4 files changed, 27 insertions(+), 8 deletions(-)