* [linux-sunxi] [PATCH 3/5] Input: add driver for Ilitek ili2139 touch IC
From: Hans de Goede @ 2016-10-12 14:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161011132149.LZ40KToV@smtp1h.mail.yandex.net>
Hi,
On 10/11/2016 12:21 PM, Icenowy Zheng wrote:
>
> 2016?10?11? ??5:37? Hans de Goede <hdegoede@redhat.com>???
>>
>> Hi,
>
> I have a request: could you please test this driver on your E708 Q1? (According to the info you published on github, your E708 has also ili)
I'm afraid the touchscreen on my E708 Q1 is broken (does not even work in android),
so I cannot test this.
>
>>
>> On 10/11/2016 02:33 AM, Icenowy Zheng wrote:
>>> This driver adds support for Ilitek ili2139 touch IC, which is used in
>>> several Colorfly tablets (for example, Colorfly E708 Q1, which is an
>>> Allwinner A31s tablet with mainline kernel support).
>>>
>>> Theortically it may support more Ilitek touch ICs, however, only ili2139
>>> is used in any mainlined device.
>>>
>>> It supports device tree enumeration, with screen resolution and axis
>>> quirks configurable.
>>>
>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>>> ---
>>> drivers/input/touchscreen/Kconfig | 14 ++
>>> drivers/input/touchscreen/Makefile | 1 +
>>> drivers/input/touchscreen/ili2139.c | 320 ++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 335 insertions(+)
>>> create mode 100644 drivers/input/touchscreen/ili2139.c
>>>
>>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>>> index 5079813..bb4d9d2 100644
>>> --- a/drivers/input/touchscreen/Kconfig
>>> +++ b/drivers/input/touchscreen/Kconfig
>>> @@ -348,6 +348,20 @@ config TOUCHSCREEN_ILI210X
>>> To compile this driver as a module, choose M here: the
>>> module will be called ili210x.
>>>
>>> +config TOUCHSCREEN_ILI2139
>>> + tristate "Ilitek ILI2139 based touchscreen"
>>> + depends on I2C
>>> + depends on OF
>>> + help
>>> + Say Y here if you have a ILI2139 based touchscreen
>>> + controller. Such kind of chipsets can be found in several
>>> + Colorfly tablets.
>>> +
>>> + If unsure, say N.
>>> +
>>> + To compile this driver as a module, choose M here; the
>>> + module will be called ili2139.
>>> +
>>> config TOUCHSCREEN_IPROC
>>> tristate "IPROC touch panel driver support"
>>> depends on ARCH_BCM_IPROC || COMPILE_TEST
>>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>>> index 81b8645..930b5e2 100644
>>> --- a/drivers/input/touchscreen/Makefile
>>> +++ b/drivers/input/touchscreen/Makefile
>>> @@ -40,6 +40,7 @@ obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
>>> obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
>>> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix.o
>>> obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
>>> +obj-$(CONFIG_TOUCHSCREEN_ILI2139) += ili2139.o
>>> obj-$(CONFIG_TOUCHSCREEN_IMX6UL_TSC) += imx6ul_tsc.o
>>> obj-$(CONFIG_TOUCHSCREEN_INEXIO) += inexio.o
>>> obj-$(CONFIG_TOUCHSCREEN_INTEL_MID) += intel-mid-touch.o
>>> diff --git a/drivers/input/touchscreen/ili2139.c b/drivers/input/touchscreen/ili2139.c
>>> new file mode 100644
>>> index 0000000..65c2dea
>>> --- /dev/null
>>> +++ b/drivers/input/touchscreen/ili2139.c
>>> @@ -0,0 +1,320 @@
>>> +/* -------------------------------------------------------------------------
>>> + * Copyright (C) 2016, Icenowy Zheng <icenowy@aosc.xyz>
>>> + *
>>> + * Derived from:
>>> + * ili210x.c
>>> + * Copyright (C) Olivier Sobrie <olivier@sobrie.be>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + * -------------------------------------------------------------------------
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/input.h>
>>> +#include <linux/input/mt.h>
>>> +#include <linux/input/touchscreen.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/workqueue.h>
>>> +
>>> +#define DEFAULT_POLL_PERIOD 20
>>> +
>>> +#define MAX_TOUCHES 10
>>> +#define COMPATIBLE_TOUCHES 2
>>> +
>>> +/* Touchscreen commands */
>>> +#define REG_TOUCHDATA 0x10
>>> +#define REG_TOUCHSUBDATA 0x11
>>> +#define REG_PANEL_INFO 0x20
>>> +#define REG_FIRMWARE_VERSION 0x40
>>> +#define REG_PROTO_VERSION 0x42
>>> +
>>> +#define SUBDATA_STATUS_TOUCH_POINT 0x80
>>> +#define SUBDATA_STATUS_RELEASE_POINT 0x00
>>> +
>>> +struct finger {
>>> + u8 x_low;
>>> + u8 x_high;
>>> + u8 y_low;
>>> + u8 y_high;
>>> +} __packed;
>>> +
>>> +struct touchdata {
>>> + u8 length;
>>> + struct finger finger[COMPATIBLE_TOUCHES];
>>> +} __packed;
>>> +
>>> +struct touch_subdata {
>>> + u8 status;
>>> + struct finger finger;
>>> +} __packed;
>>> +
>>> +struct panel_info {
>>> + struct finger finger_max;
>>> + u8 xchannel_num;
>>> + u8 ychannel_num;
>>> +} __packed;
>>> +
>>> +struct firmware_version {
>>> + u8 id;
>>> + u8 major;
>>> + u8 minor;
>>> +} __packed;
>>> +
>>> +struct ili2139 {
>>> + struct i2c_client *client;
>>> + struct input_dev *input;
>>> + unsigned int poll_period;
>>> + struct delayed_work dwork;
>>> + struct touchscreen_properties prop;
>>> + int slots[MAX_TOUCHES];
>>> + int ids[MAX_TOUCHES];
>>> + struct input_mt_pos pos[MAX_TOUCHES];
>>> +};
>>> +
>>> +static int ili2139_read_reg(struct i2c_client *client, u8 reg, void *buf,
>>> + size_t len)
>>> +{
>>> + struct i2c_msg msg[2] = {
>>> + {
>>> + .addr = client->addr,
>>> + .flags = 0,
>>> + .len = 1,
>>> + .buf = ®,
>>> + },
>>> + {
>>> + .addr = client->addr,
>>> + .flags = I2C_M_RD,
>>> + .len = len,
>>> + .buf = buf,
>>> + }
>>> + };
>>> +
>>> + if (i2c_transfer(client->adapter, msg, 2) != 2) {
>>> + dev_err(&client->dev, "i2c transfer failed\n");
>>> + return -EIO;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>> This just i2c_smbus_read_i2c_block_data, please use that instead.
>>
>>> +static void ili2139_work(struct work_struct *work)
>>> +{
>>> + int id;
>>> + struct ili2139 *priv = container_of(work, struct ili2139,
>>> + dwork.work);
>>> + struct i2c_client *client = priv->client;
>>> + struct touchdata touchdata;
>>> + struct touch_subdata subdata;
>>> + int error;
>>> +
>>> + error = ili2139_read_reg(client, REG_TOUCHDATA,
>>> + &touchdata, sizeof(touchdata));
>>> + if (error) {
>>> + dev_err(&client->dev,
>>> + "Unable to get touchdata, err = %d\n", error);
>>> + return;
>>> + }
>>> +
>>> + for (id = 0; id < touchdata.length; id++) {
>>> + error = ili2139_read_reg(client, REG_TOUCHSUBDATA, &subdata,
>>> + sizeof(subdata));
>>> + if (error) {
>>> + dev_err(&client->dev,
>>> + "Unable to get touch subdata, err = %d\n",
>>> + error);
>>> + return;
>>> + }
>>> +
>>> + priv->ids[id] = subdata.status & 0x3F;
>>> +
>>> + /* The sequence changed in the v2 subdata protocol. */
>>> + touchscreen_set_mt_pos(&priv->pos[id], &priv->prop,
>>> + (subdata.finger.x_high | (subdata.finger.x_low << 8)),
>>> + (subdata.finger.y_high | (subdata.finger.y_low << 8)));
>>> + }
>>> +
>>> + input_mt_assign_slots(priv->input, priv->slots, priv->pos,
>>> + touchdata.length, 0);
>>> +
>>> + for (id = 0; id < touchdata.length; id++) {
>>> + input_mt_slot(priv->input, priv->slots[id]);
>>> + input_mt_report_slot_state(priv->input, MT_TOOL_FINGER,
>>> + subdata.status &
>>> + SUBDATA_STATUS_TOUCH_POINT);
>>> + input_report_abs(priv->input, ABS_MT_POSITION_X,
>>> + priv->pos[id].x);
>>> + input_report_abs(priv->input, ABS_MT_POSITION_Y,
>>> + priv->pos[id].y);
>>> + }
>>> +
>>> + input_mt_sync_frame(priv->input);
>>> + input_sync(priv->input);
>>> +
>>> + schedule_delayed_work(&priv->dwork,
>>> + msecs_to_jiffies(priv->poll_period));
>>
>> If the irq is working properly there should be no need for this,
>> can you try with this schedule call removed ?
>
> Thanks. I'm glad to know this.
> The driver that I modelled after is too old and unmaintained, and even nearly not usable now (as it requires platform data)
>
>>
>>> +}
>>> +
>>> +static irqreturn_t ili2139_irq(int irq, void *irq_data)
>>> +{
>>> + struct ili2139 *priv = irq_data;
>>> +
>>> + schedule_delayed_work(&priv->dwork, 0);
>>> +
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int ili2139_i2c_probe(struct i2c_client *client,
>>> + const struct i2c_device_id *id)
>>> +{
>>> + struct device *dev = &client->dev;
>>> + struct ili2139 *priv;
>>> + struct input_dev *input;
>>> + struct panel_info panel;
>>> + struct firmware_version firmware;
>>> + int xmax, ymax;
>>> + int error;
>>> +
>>> + dev_dbg(dev, "Probing for ILI2139 I2C Touschreen driver");
>>> +
>>> + if (client->irq <= 0) {
>>> + dev_err(dev, "No IRQ!\n");
>>> + return -ENODEV;
>>> + }
>>> +
>>> + /* Get firmware version */
>>> + error = ili2139_read_reg(client, REG_FIRMWARE_VERSION,
>>> + &firmware, sizeof(firmware));
>>> + if (error) {
>>> + dev_err(dev, "Failed to get firmware version, err: %d\n",
>>> + error);
>>> + return error;
>>> + }
>>> +
>>> + /* get panel info */
>>> + error = ili2139_read_reg(client, REG_PANEL_INFO, &panel, sizeof(panel));
>>> + if (error) {
>>> + dev_err(dev, "Failed to get panel information, err: %d\n",
>>> + error);
>>> + return error;
>>> + }
>>> +
>>> + xmax = panel.finger_max.x_low | (panel.finger_max.x_high << 8);
>>> + ymax = panel.finger_max.y_low | (panel.finger_max.y_high << 8);
>>> +
>>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> + input = devm_input_allocate_device(dev);
>>> + if (!priv || !input)
>>> + return -ENOMEM;
>>> +
>>> + priv->client = client;
>>> + priv->input = input;
>>> + priv->poll_period = DEFAULT_POLL_PERIOD;
>>> + INIT_DELAYED_WORK(&priv->dwork, ili2139_work);
>>> +
>>> + /* Setup input device */
>>> + input->name = "ILI2139 Touchscreen";
>>> + input->id.bustype = BUS_I2C;
>>> + input->dev.parent = dev;
>>> +
>>> + __set_bit(EV_SYN, input->evbit);
>>> + __set_bit(EV_KEY, input->evbit);
>>> + __set_bit(EV_ABS, input->evbit);
>>> +
>>> + /* Multi touch */
>>> + input_mt_init_slots(input, MAX_TOUCHES, INPUT_MT_DIRECT |
>>> + INPUT_MT_DROP_UNUSED | INPUT_MT_TRACK);
>>> + input_set_abs_params(input, ABS_MT_POSITION_X, 0, xmax, 0, 0);
>>> + input_set_abs_params(input, ABS_MT_POSITION_Y, 0, ymax, 0, 0);
>>> +
>>> + touchscreen_parse_properties(input, true, &priv->prop);
>>> +
>>> + input_set_drvdata(input, priv);
>>> + i2c_set_clientdata(client, priv);
>>> +
>>> + error = devm_request_irq(dev, client->irq, ili2139_irq,
>>> + IRQF_TRIGGER_FALLING, client->name, priv);
>>
>> If things work with the re-scheduleing of the delayed work
>> from the work removed, then you can use request_threaded_irq here,
>> pass in ili2139_irq as the threaded handler (and NULL as the non threaded
>> handler) and do all the i2c reading directly in ili2139_irq without needing
>> to use any work struct at all.
>
> I'll test it, and remember request_threaded_irq function. I've seen the usage of the function in silead.c.
Good :)
Regards,
Hans
>
>>
>> Regards,
>>
>> Hans
>>
>>
>>> + if (error) {
>>> + dev_err(dev, "Unable to request touchscreen IRQ, err: %d\n",
>>> + error);
>>> + return error;
>>> + }
>>> +
>>> + error = input_register_device(priv->input);
>>> + if (error) {
>>> + dev_err(dev, "Cannot register input device, err: %d\n", error);
>>> + return error;
>>> + }
>>> +
>>> + device_init_wakeup(&client->dev, 1);
>>> +
>>> + dev_dbg(dev,
>>> + "ILI2139 initialized (IRQ: %d), firmware version %d.%d.%d",
>>> + client->irq, firmware.id, firmware.major, firmware.minor);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ili2139_i2c_remove(struct i2c_client *client)
>>> +{
>>> + struct ili2139 *priv = i2c_get_clientdata(client);
>>> +
>>> + cancel_delayed_work_sync(&priv->dwork);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int __maybe_unused ili2139_i2c_suspend(struct device *dev)
>>> +{
>>> + struct i2c_client *client = to_i2c_client(dev);
>>> +
>>> + if (device_may_wakeup(&client->dev))
>>> + enable_irq_wake(client->irq);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int __maybe_unused ili2139_i2c_resume(struct device *dev)
>>> +{
>>> + struct i2c_client *client = to_i2c_client(dev);
>>> +
>>> + if (device_may_wakeup(&client->dev))
>>> + disable_irq_wake(client->irq);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static SIMPLE_DEV_PM_OPS(ili2139_i2c_pm,
>>> + ili2139_i2c_suspend, ili2139_i2c_resume);
>>> +
>>> +static const struct i2c_device_id ili2139_i2c_id[] = {
>>> + { "ili2139", 0 },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, ili2139_i2c_id);
>>> +
>>> +static struct i2c_driver ili2139_ts_driver = {
>>> + .driver = {
>>> + .name = "ili2139_i2c",
>>> + .pm = &ili2139_i2c_pm,
>>> + },
>>> + .id_table = ili2139_i2c_id,
>>> + .probe = ili2139_i2c_probe,
>>> + .remove = ili2139_i2c_remove,
>>> +};
>>> +
>>> +module_i2c_driver(ili2139_ts_driver);
>>> +
>>> +MODULE_AUTHOR("Olivier Sobrie <olivier@sobrie.be>");
>>> +MODULE_DESCRIPTION("ILI2139 I2C Touchscreen Driver");
>>> +MODULE_LICENSE("GPL");
>>>
^ permalink raw reply
* [bug report] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver
From: Mason @ 2016-10-12 14:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161012113229.GA16447@mwanda>
On 12/10/2016 13:32, Dan Carpenter wrote:
> Hello Tai Nguyen,
>
> The patch 832c927d119b: "perf: xgene: Add APM X-Gene SoC Performance
> Monitoring Unit driver" from Jul 15, 2016, leads to the following
> static checker warning:
>
> drivers/perf/xgene_pmu.c:1014 acpi_get_pmu_hw_inf()
> warn: '&res' isn't an ERR_PTR
>
> drivers/perf/xgene_pmu.c
> 992 static struct
> 993 xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
> 994 struct acpi_device *adev, u32 type)
> 995 {
> 996 struct device *dev = xgene_pmu->dev;
> 997 struct list_head resource_list;
> 998 struct xgene_pmu_dev_ctx *ctx;
> 999 const union acpi_object *obj;
> 1000 struct hw_pmu_info *inf;
> 1001 void __iomem *dev_csr;
> 1002 struct resource res;
> 1003 int enable_bit;
> 1004 int rc;
> 1005
> 1006 ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> 1007 if (!ctx)
> 1008 return NULL;
> 1009
> 1010 INIT_LIST_HEAD(&resource_list);
> 1011 rc = acpi_dev_get_resources(adev, &resource_list,
> 1012 acpi_pmu_dev_add_resource, &res);
> 1013 acpi_dev_free_resource_list(&resource_list);
> 1014 if (rc < 0 || IS_ERR(&res)) {
> ^^^^
> Obviously this is a stack address and not an error pointer. I'm not
> sure what was intended here.
Reminds me of 287980e49ffc0f6d911601e7e352a812ed27768e
("remove lots of IS_ERR_VALUE abuses")
Regards.
^ permalink raw reply
* Question: Re: [PATCH] drm/bridge: analogix: protect power when get_modes or detect
From: Sean Paul @ 2016-10-12 14:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <57FE0EF5.7010207@rock-chips.com>
On Wed, Oct 12, 2016 at 6:22 AM, Mark yao <mark.yao@rock-chips.com> wrote:
>
> I'm not familiar with the analogix driver, maybe use a power reference count
> would better then direct power on/off analogix_dp.
>
> Does anyone has the idea to protect detect and get_modes context?
>
I'm not sure a reference count is going to help here. The common
pattern is to call detect() followed by get_modes() and then modeset.
However, it's not guaranteed that any one of those functions will be
called after the other. So, if you leave things on after detect or
get_modes, you might be wasting power (or worse).
I recently ran into this exact problem with a panel we're using. Check
out "0b8b059a7: drm/bridge: analogix_dp: Ensure the panel is properly
prepared/unprepared". Perhaps you can piggyback on that function to
add your pm_runtime and plat_data callbacks (since using dpms_mode
might be racey).
Sean
> I found many other connector driver also direct access register on detect or
> get_modes, no problem for it?
>
> On 2016?10?12? 18:00, Mark Yao wrote:
>
> The drm callback ->detect and ->get_modes seems is not power safe,
> they may be called when device is power off, do register access on
> detect or get_modes will cause system die.
>
> Here is the path call ->detect before analogix_dp power on
> [<ffffff800843babc>] analogix_dp_detect+0x44/0xdc
> [<ffffff80083fd840>]
> drm_helper_probe_single_connector_modes_merge_bits+0xe8/0x41c
> [<ffffff80083fdb84>] drm_helper_probe_single_connector_modes+0x10/0x18
> [<ffffff8008418d24>] drm_mode_getconnector+0xf4/0x304
> [<ffffff800840cff0>] drm_ioctl+0x23c/0x390
> [<ffffff80081a8adc>] do_vfs_ioctl+0x4b8/0x58c
> [<ffffff80081a8c10>] SyS_ioctl+0x60/0x88
>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Cc: "Ville Syrj?l?" <ville.syrjala@linux.intel.com>
>
> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> ---
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 28
> ++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index efac8ab..09dece2 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1062,6 +1062,13 @@ int analogix_dp_get_modes(struct drm_connector
> *connector)
> return 0;
> }
>
> + if (dp->dpms_mode != DRM_MODE_DPMS_ON) {
> + pm_runtime_get_sync(dp->dev);
> +
> + if (dp->plat_data->power_on)
> + dp->plat_data->power_on(dp->plat_data);
> + }
> +
> if (analogix_dp_handle_edid(dp) == 0) {
> drm_mode_connector_update_edid_property(&dp->connector, edid);
> num_modes += drm_add_edid_modes(&dp->connector, edid);
> @@ -1073,6 +1080,13 @@ int analogix_dp_get_modes(struct drm_connector
> *connector)
> if (dp->plat_data->get_modes)
> num_modes += dp->plat_data->get_modes(dp->plat_data, connector);
>
> + if (dp->dpms_mode != DRM_MODE_DPMS_ON) {
> + if (dp->plat_data->power_off)
> + dp->plat_data->power_off(dp->plat_data);
> +
> + pm_runtime_put_sync(dp->dev);
> + }
> +
> ret = analogix_dp_prepare_panel(dp, false, false);
> if (ret)
> DRM_ERROR("Failed to unprepare panel (%d)\n", ret);
> @@ -1106,9 +1120,23 @@ analogix_dp_detect(struct drm_connector *connector,
> bool force)
> return connector_status_disconnected;
> }
>
> + if (dp->dpms_mode != DRM_MODE_DPMS_ON) {
> + pm_runtime_get_sync(dp->dev);
> +
> + if (dp->plat_data->power_on)
> + dp->plat_data->power_on(dp->plat_data);
> + }
> +
> if (!analogix_dp_detect_hpd(dp))
> status = connector_status_connected;
>
> + if (dp->dpms_mode != DRM_MODE_DPMS_ON) {
> + if (dp->plat_data->power_off)
> + dp->plat_data->power_off(dp->plat_data);
> +
> + pm_runtime_put_sync(dp->dev);
> + }
> +
> ret = analogix_dp_prepare_panel(dp, false, false);
> if (ret)
> DRM_ERROR("Failed to unprepare panel (%d)\n", ret);
>
>
>
> --
> ?ark Yao
^ permalink raw reply
* [PATCH 2/3] arm64: hw_breakpoint: Handle inexact watchpoint addresses
From: Pavel Labath @ 2016-10-12 13:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAHB_GupR4sJtAGYdRwvCwu7891KM8TEByZCgqMCrYrdt=cJuYw@mail.gmail.com>
Hello Pratyush,
I am sorry about the delay. I have finally had a chance to try out
your changes today. Response inline.
On 8 October 2016 at 06:10, Pratyush Anand <panand@redhat.com> wrote:
> On Fri, Oct 7, 2016 at 10:54 PM, Pavel Labath <labath@google.com> wrote:
>> The thing is, I have observed different behavior here depending on the
>> exact hardware used. I don't have the exact parameters with me now,
>> but I can look it up next week.
>>
>> The thing is that the spec is imprecise about what exact address the
>> hardware can report for the watchpoint hit. I presume that is
>> deliberate to give some leeway to implementers. The spec says the
>> address can be anywhere in the range from the lowest memory address
>> accessed by the instruction to the highest address watched by the
>> watchpoint,
>
> I think, my patches should be able to take care of the above condition.
Unfortunately, the patch does not solve the problem for my hardware,
because of the leeway you give in watchpoint_handler is not big
enough. It does work however, if I change the line
> if (addr + 7 < val + lens || addr > val + lene)
to
> if (addr + 15 < val + lens || addr > val + lene)
I do not think we can assume that address reported by the hardware
will be at most 7 bytes off from the address we put the watchpoint at.
There is nothing in the spec that guarantees that, and it does not
seem to be enough for some hardware. In fact, I am not sure we can
assume 15 is enough either, but maybe it can do for now, until we can
find hardware that does not work with that (I haven't yet tried what
happens it the watchpoint is triggered by cache management
instructions, which can access much larger blocks of memory).
For reference, the hardware in question is:
> Processor : AArch64 Processor rev 0 (aarch64)
> processor : 0
> min_vddcx : 400000
> min_vddmx : 490000
> BogoMIPS : 38.00
> Features : fp asimd evtstrm aes pmull sha1 sha2 crc32
> CPU implementer : 0x51
> CPU architecture: 8
> CPU variant : 0x2
> CPU part : 0x201
> CPU revision : 0
> CPU param : 300 468 468 621 939 299 445 445 621 1077
> Hardware : Qualcomm Technologies, Inc MSM8996pro
And this is how it behaves:
The output of the test app triggering the watchpoint (I have set a
single-byte watchpoint at 555556705f)
>
> Writing to: 555556705f, size: 1
> Writing to: 555556705e, size: 2
> Writing to: 555556705c, size: 4
> Writing to: 5555567058, size: 8
> Writing to: 5555567050, size: 16
> Writing to: 5555567040, size: 32
The addresses received by the kernel:
[ 251.812166] c1 3780 hw-breakpoint: watchpoint_handler: addr:
555556705f, val+lens: 555556705f, val+lene: 555556705f
[ 251.820341] c1 3781 hw-breakpoint: watchpoint_handler: addr:
555556705e, val+lens: 555556705f, val+lene: 555556705f
[ 251.825572] c0 3782 hw-breakpoint: watchpoint_handler: addr:
555556705c, val+lens: 555556705f, val+lene: 555556705f
[ 251.831085] c0 3783 hw-breakpoint: watchpoint_handler: addr:
5555567058, val+lens: 555556705f, val+lene: 555556705f
[ 251.835804] c0 3784 hw-breakpoint: watchpoint_handler: addr:
5555567050, val+lens: 555556705f, val+lene: 555556705f
[ 251.841350] c0 3785 hw-breakpoint: watchpoint_handler: addr:
5555567050, val+lens: 555556705f, val+lene: 555556705f
Note that for the case of 16 and 32-byte access it returns the address
5555567050 -- this is why the "+15" is sufficient for me.
The other thing I am not so sure about in your patch is that it has
potential to mis-attribute the watchpoint hit if we have two
watchpoints close together. For example, if I have first watchpoint on
0x1008-0x100f and a second one on 0x1000-0x1007, *and* the application
writes one byte to 0x1004, then your code will still attribute the hit
to the first watchpoint, even though it was not really triggered. This
is the reason I implemented my fix as a two-stage process, first
looking for exact hits, and then falling back to the nearest one. That
said, I don't know enough about the codebase to say if this is a real
problem.
On the plus side, I like the fact that we can watch arbitrary memory
regions now, and the feature is working perfectly. :)
My proposal would be to combine the two patches - take the byte mask
handling code from yours, and the hit-attribution code from my patch.
What do you think?
regards,
pavel
^ permalink raw reply
* [PATCH v14 16/16] vfio/type1: Introduce MSI_RESV capability
From: Eric Auger @ 2016-10-12 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476278544-3397-1-git-send-email-eric.auger@redhat.com>
This patch allows the user-space to retrieve the MSI reserved region
requirements, if any. The implementation is based on capability chains,
now also added to VFIO_IOMMU_GET_INFO.
The returned info comprises the size and the alignment requirements
In case the userspace must provide the IOVA aperture, we currently report
a size/alignment based on all the doorbells registered by the host kernel.
This may exceed the actual needs.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
v13 -> v14:
- new capability struct
- change the padding in vfio_iommu_type1_info
v11 -> v12:
- msi_doorbell_pages was renamed msi_doorbell_calc_pages
v9 -> v10:
- move cap_offset after iova_pgsizes
- replace __u64 alignment by __u32 order
- introduce __u32 flags in vfio_iommu_type1_info_cap_msi_geometry and
fix alignment
- call msi-doorbell API to compute the size/alignment
v8 -> v9:
- use iommu_msi_supported flag instead of programmable
- replace IOMMU_INFO_REQUIRE_MSI_MAP flag by a more sophisticated
capability chain, reporting the MSI geometry
v7 -> v8:
- use iommu_domain_msi_geometry
v6 -> v7:
- remove the computation of the number of IOVA pages to be provisionned.
This number depends on the domain/group/device topology which can
dynamically change. Let's rely instead rely on an arbitrary max depending
on the system
v4 -> v5:
- move msi_info and ret declaration within the conditional code
v3 -> v4:
- replace former vfio_domains_require_msi_mapping by
more complex computation of MSI mapping requirements, especially the
number of pages to be provided by the user-space.
- reword patch title
RFC v1 -> v1:
- derived from
[RFC PATCH 3/6] vfio: Extend iommu-info to return MSIs automap state
- renamed allow_msi_reconfig into require_msi_mapping
- fixed VFIO_IOMMU_GET_INFO
---
drivers/vfio/vfio_iommu_type1.c | 67 ++++++++++++++++++++++++++++++++++++++++-
include/uapi/linux/vfio.h | 20 +++++++++++-
2 files changed, 85 insertions(+), 2 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index c18ba9d..6775da3 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1147,6 +1147,46 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
return ret;
}
+static int msi_resv_caps(struct vfio_iommu *iommu, struct vfio_info_cap *caps)
+{
+ struct iommu_domain_msi_resv msi_resv = {.size = 0, .alignment = 0};
+ struct vfio_iommu_type1_info_cap_msi_resv *cap;
+ struct vfio_info_cap_header *header;
+ struct iommu_domain_msi_resv iter;
+ struct vfio_domain *d;
+
+ mutex_lock(&iommu->lock);
+
+ list_for_each_entry(d, &iommu->domain_list, next) {
+ if (iommu_domain_get_attr(d->domain,
+ DOMAIN_ATTR_MSI_RESV, &iter))
+ continue;
+ if (iter.size > msi_resv.size) {
+ msi_resv.size = iter.size;
+ msi_resv.alignment = iter.alignment;
+ }
+ }
+
+ if (!msi_resv.size)
+ return 0;
+
+ mutex_unlock(&iommu->lock);
+
+ header = vfio_info_cap_add(caps, sizeof(*cap),
+ VFIO_IOMMU_TYPE1_INFO_CAP_MSI_RESV, 1);
+
+ if (IS_ERR(header))
+ return PTR_ERR(header);
+
+ cap = container_of(header, struct vfio_iommu_type1_info_cap_msi_resv,
+ header);
+
+ cap->alignment = msi_resv.alignment;
+ cap->size = msi_resv.size;
+
+ return 0;
+}
+
static long vfio_iommu_type1_ioctl(void *iommu_data,
unsigned int cmd, unsigned long arg)
{
@@ -1168,8 +1208,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
}
} else if (cmd == VFIO_IOMMU_GET_INFO) {
struct vfio_iommu_type1_info info;
+ struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
+ int ret;
- minsz = offsetofend(struct vfio_iommu_type1_info, iova_pgsizes);
+ minsz = offsetofend(struct vfio_iommu_type1_info, cap_offset);
if (copy_from_user(&info, (void __user *)arg, minsz))
return -EFAULT;
@@ -1181,6 +1223,29 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
info.iova_pgsizes = vfio_pgsize_bitmap(iommu);
+ ret = msi_resv_caps(iommu, &caps);
+ if (ret)
+ return ret;
+
+ if (caps.size) {
+ info.flags |= VFIO_IOMMU_INFO_CAPS;
+ if (info.argsz < sizeof(info) + caps.size) {
+ info.argsz = sizeof(info) + caps.size;
+ info.cap_offset = 0;
+ } else {
+ vfio_info_cap_shift(&caps, sizeof(info));
+ if (copy_to_user((void __user *)arg +
+ sizeof(info), caps.buf,
+ caps.size)) {
+ kfree(caps.buf);
+ return -EFAULT;
+ }
+ info.cap_offset = sizeof(info);
+ }
+
+ kfree(caps.buf);
+ }
+
return copy_to_user((void __user *)arg, &info, minsz) ?
-EFAULT : 0;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 4a9dbc2..e34a9a6 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -488,7 +488,23 @@ struct vfio_iommu_type1_info {
__u32 argsz;
__u32 flags;
#define VFIO_IOMMU_INFO_PGSIZES (1 << 0) /* supported page sizes info */
- __u64 iova_pgsizes; /* Bitmap of supported page sizes */
+#define VFIO_IOMMU_INFO_CAPS (1 << 1) /* Info supports caps */
+ __u64 iova_pgsizes; /* Bitmap of supported page sizes */
+ __u32 cap_offset; /* Offset within info struct of first cap */
+ __u32 __resv;
+};
+
+/*
+ * The MSI_RESV capability allows to report the MSI reserved IOVA requirements:
+ * In case this capability is supported, the userspace must provide an IOVA
+ * window characterized by @size and @alignment using VFIO_IOMMU_MAP_DMA with
+ * RESERVED_MSI_IOVA flag.
+ */
+#define VFIO_IOMMU_TYPE1_INFO_CAP_MSI_RESV 1
+struct vfio_iommu_type1_info_cap_msi_resv {
+ struct vfio_info_cap_header header;
+ __u64 size; /* requested IOVA aperture size in bytes */
+ __u64 alignment; /* requested byte alignment of the window */
};
#define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
@@ -503,6 +519,8 @@ struct vfio_iommu_type1_info {
* IOVA region that will be used on some platforms to map the host MSI frames.
* In that specific case, vaddr is ignored. Once registered, an MSI reserved
* IOVA region stays until the container is closed.
+ * The requirement for provisioning such reserved IOVA range can be checked by
+ * checking the VFIO_IOMMU_TYPE1_INFO_CAP_MSI_RESV capability.
*/
struct vfio_iommu_type1_dma_map {
__u32 argsz;
--
1.9.1
^ permalink raw reply related
* [PATCH v14 15/16] iommu/arm-smmu: Do not advertise IOMMU_CAP_INTR_REMAP
From: Eric Auger @ 2016-10-12 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476278544-3397-1-git-send-email-eric.auger@redhat.com>
Do not advertise IOMMU_CAP_INTR_REMAP for arm-smmu(-v3). Indeed the
irq_remapping capability is abstracted on irqchip side for ARM as
opposed to Intel IOMMU featuring IRQ remapping HW.
So to check IRQ remapping capability, the msi domain needs to be
checked instead.
This commit affects platform and PCIe device assignment use cases
on any platform featuring an unsafe MSI controller (currently the
ARM GICv2m). For those platforms the VFIO module must be loaded with
allow_unsafe_interrupts set to 1.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
v9 -> v10:
- reword the commit message (allow_unsafe_interrupts)
---
drivers/iommu/arm-smmu-v3.c | 3 ++-
drivers/iommu/arm-smmu.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 572cad8..d71a955 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1371,7 +1371,8 @@ static bool arm_smmu_capable(enum iommu_cap cap)
case IOMMU_CAP_CACHE_COHERENCY:
return true;
case IOMMU_CAP_INTR_REMAP:
- return true; /* MSIs are just memory writes */
+ /* interrupt translation handled at MSI controller level */
+ return false;
case IOMMU_CAP_NOEXEC:
return true;
default:
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index ae20b9c..becad89 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1361,7 +1361,8 @@ static bool arm_smmu_capable(enum iommu_cap cap)
*/
return true;
case IOMMU_CAP_INTR_REMAP:
- return true; /* MSIs are just memory writes */
+ /* interrupt translation handled at MSI controller level */
+ return false;
case IOMMU_CAP_NOEXEC:
return true;
default:
--
1.9.1
^ permalink raw reply related
* [PATCH v14 14/16] vfio/type1: Check doorbell safety
From: Eric Auger @ 2016-10-12 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476278544-3397-1-git-send-email-eric.auger@redhat.com>
On x86 IRQ remapping is abstracted by the IOMMU. On ARM this is abstracted
by the msi controller.
Since we currently have no way to detect whether the MSI controller is
upstream or downstream to the IOMMU we rely on the MSI doorbell information
registered by the interrupt controllers. In case at least one doorbell
does not implement proper isolation, we state the assignment is unsafe
with regard to interrupts. This is a coarse assessment but should allow to
wait for a better system description.
At this point ARM sMMU still advertises IOMMU_CAP_INTR_REMAP. This is
removed in next patch.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
v13 -> v15:
- check vfio_msi_resv before checking whether msi doorbell is safe
v9 -> v10:
- coarse safety assessment based on MSI doorbell info
v3 -> v4:
- rename vfio_msi_parent_irq_remapping_capable into vfio_safe_irq_domain
and irq_remapping into safe_irq_domains
v2 -> v3:
- protect vfio_msi_parent_irq_remapping_capable with
CONFIG_GENERIC_MSI_IRQ_DOMAIN
---
drivers/vfio/vfio_iommu_type1.c | 30 +++++++++++++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index e0c97ef..c18ba9d 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -442,6 +442,29 @@ static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
}
/**
+ * vfio_msi_resv - Return whether any VFIO iommu domain requires
+ * MSI mapping
+ *
+ * @iommu: vfio iommu handle
+ *
+ * Return: true of MSI mapping is needed, false otherwise
+ */
+static bool vfio_msi_resv(struct vfio_iommu *iommu)
+{
+ struct iommu_domain_msi_resv msi_resv;
+ struct vfio_domain *d;
+ int ret;
+
+ list_for_each_entry(d, &iommu->domain_list, next) {
+ ret = iommu_domain_get_attr(d->domain, DOMAIN_ATTR_MSI_RESV,
+ &msi_resv);
+ if (!ret)
+ return true;
+ }
+ return false;
+}
+
+/**
* vfio_set_msi_aperture - Sets the msi aperture on all domains
* requesting MSI mapping
*
@@ -945,8 +968,13 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
INIT_LIST_HEAD(&domain->group_list);
list_add(&group->next, &domain->group_list);
+ /*
+ * to advertise safe interrupts either the IOMMU or the MSI controllers
+ * must support IRQ remapping (aka. interrupt translation)
+ */
if (!allow_unsafe_interrupts &&
- !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) {
+ (!iommu_capable(bus, IOMMU_CAP_INTR_REMAP) &&
+ !(vfio_msi_resv(iommu) && iommu_msi_doorbell_safe()))) {
pr_warn("%s: No interrupt remapping support. Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
__func__);
ret = -EPERM;
--
1.9.1
^ permalink raw reply related
* [PATCH v14 13/16] vfio: Allow reserved msi iova registration
From: Eric Auger @ 2016-10-12 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476278544-3397-1-git-send-email-eric.auger@redhat.com>
The user is allowed to register a reserved MSI IOVA range by using the
DMA MAP API and setting the new flag: VFIO_DMA_MAP_FLAG_MSI_RESERVED_IOVA.
This region is stored in the vfio_dma rb tree. At that point the iova
range is not mapped to any target address yet. The host kernel will use
those iova when needed, typically when MSIs are allocated.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
---
v13 -> v14:
- in vfio_set_msi_aperture, unfold in case of failure
- Get DOMAIN_ATTR_MSI_RESV attribute to decide whether to set the MSI
aperture
v12 -> v13:
- use iommu_get_dma_msi_region_cookie
v9 -> v10
- use VFIO_IOVA_RESERVED_MSI enum value
v7 -> v8:
- use iommu_msi_set_aperture function. There is no notion of
unregistration anymore since the reserved msi slot remains
until the container gets closed.
v6 -> v7:
- use iommu_free_reserved_iova_domain
- convey prot attributes downto dma-reserved-iommu iova domain creation
- reserved bindings teardown now performed on iommu domain destruction
- rename VFIO_DMA_MAP_FLAG_MSI_RESERVED_IOVA into
VFIO_DMA_MAP_FLAG_RESERVED_MSI_IOVA
- change title
- pass the protection attribute to dma-reserved-iommu API
v3 -> v4:
- use iommu_alloc/free_reserved_iova_domain exported by dma-reserved-iommu
- protect vfio_register_reserved_iova_range implementation with
CONFIG_IOMMU_DMA_RESERVED
- handle unregistration by user-space and on vfio_iommu_type1 release
v1 -> v2:
- set returned value according to alloc_reserved_iova_domain result
- free the iova domains in case any error occurs
RFC v1 -> v1:
- takes into account Alex comments, based on
[RFC PATCH 1/6] vfio: Add interface for add/del reserved iova region:
- use the existing dma map/unmap ioctl interface with a flag to register
a reserved IOVA range. A single reserved iova region is allowed.
---
drivers/vfio/vfio_iommu_type1.c | 97 ++++++++++++++++++++++++++++++++++++++++-
include/uapi/linux/vfio.h | 10 ++++-
2 files changed, 105 insertions(+), 2 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 2108e2e..e0c97ef 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -441,6 +441,40 @@ static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
vfio_lock_acct(-unlocked);
}
+/**
+ * vfio_set_msi_aperture - Sets the msi aperture on all domains
+ * requesting MSI mapping
+ *
+ * @iommu: vfio iommu handle
+ * @iova: MSI window iova base address
+ * @size: size of the MSI reserved iova window
+ *
+ * Return: 0 if the MSI reserved region was set on at least one domain,
+ * negative value on failure
+ */
+static int vfio_set_msi_aperture(struct vfio_iommu *iommu,
+ dma_addr_t iova, size_t size)
+{
+ struct iommu_domain_msi_resv msi_resv;
+ struct vfio_domain *d;
+ int ret = -EINVAL;
+
+ list_for_each_entry(d, &iommu->domain_list, next) {
+ if (iommu_domain_get_attr(d->domain, DOMAIN_ATTR_MSI_RESV,
+ &msi_resv))
+ continue;
+ ret = iommu_get_dma_msi_region_cookie(d->domain, iova, size);
+ if (ret)
+ goto unfold;
+ }
+ return 0;
+unfold:
+ list_for_each_entry(d, &iommu->domain_list, next)
+ iommu_put_dma_cookie(d->domain);
+
+ return ret;
+}
+
static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
{
vfio_unmap_unpin(iommu, dma);
@@ -690,6 +724,63 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
return ret;
}
+static int vfio_register_msi_range(struct vfio_iommu *iommu,
+ struct vfio_iommu_type1_dma_map *map)
+{
+ dma_addr_t iova = map->iova;
+ size_t size = map->size;
+ int ret = 0;
+ struct vfio_dma *dma;
+ unsigned long order;
+ uint64_t mask;
+
+ /* Verify that none of our __u64 fields overflow */
+ if (map->size != size || map->iova != iova)
+ return -EINVAL;
+
+ order = __ffs(vfio_pgsize_bitmap(iommu));
+ mask = ((uint64_t)1 << order) - 1;
+
+ WARN_ON(mask & PAGE_MASK);
+
+ if (!size || (size | iova) & mask)
+ return -EINVAL;
+
+ /* Don't allow IOVA address wrap */
+ if (iova + size - 1 < iova)
+ return -EINVAL;
+
+ mutex_lock(&iommu->lock);
+
+ if (vfio_find_dma(iommu, iova, size, VFIO_IOVA_ANY)) {
+ ret = -EEXIST;
+ goto unlock;
+ }
+
+ dma = kzalloc(sizeof(*dma), GFP_KERNEL);
+ if (!dma) {
+ ret = -ENOMEM;
+ goto unlock;
+ }
+
+ dma->iova = iova;
+ dma->size = size;
+ dma->type = VFIO_IOVA_RESERVED_MSI;
+
+ ret = vfio_set_msi_aperture(iommu, iova, size);
+ if (ret)
+ goto free_unlock;
+
+ vfio_link_dma(iommu, dma);
+ goto unlock;
+
+free_unlock:
+ kfree(dma);
+unlock:
+ mutex_unlock(&iommu->lock);
+ return ret;
+}
+
static int vfio_bus_type(struct device *dev, void *data)
{
struct bus_type **bus = data;
@@ -1068,7 +1159,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
} else if (cmd == VFIO_IOMMU_MAP_DMA) {
struct vfio_iommu_type1_dma_map map;
uint32_t mask = VFIO_DMA_MAP_FLAG_READ |
- VFIO_DMA_MAP_FLAG_WRITE;
+ VFIO_DMA_MAP_FLAG_WRITE |
+ VFIO_DMA_MAP_FLAG_RESERVED_MSI_IOVA;
minsz = offsetofend(struct vfio_iommu_type1_dma_map, size);
@@ -1078,6 +1170,9 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
if (map.argsz < minsz || map.flags & ~mask)
return -EINVAL;
+ if (map.flags & VFIO_DMA_MAP_FLAG_RESERVED_MSI_IOVA)
+ return vfio_register_msi_range(iommu, &map);
+
return vfio_dma_do_map(iommu, &map);
} else if (cmd == VFIO_IOMMU_UNMAP_DMA) {
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 255a211..4a9dbc2 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -498,12 +498,19 @@ struct vfio_iommu_type1_info {
*
* Map process virtual addresses to IO virtual addresses using the
* provided struct vfio_dma_map. Caller sets argsz. READ &/ WRITE required.
+ *
+ * In case RESERVED_MSI_IOVA flag is set, the API only aims@registering an
+ * IOVA region that will be used on some platforms to map the host MSI frames.
+ * In that specific case, vaddr is ignored. Once registered, an MSI reserved
+ * IOVA region stays until the container is closed.
*/
struct vfio_iommu_type1_dma_map {
__u32 argsz;
__u32 flags;
#define VFIO_DMA_MAP_FLAG_READ (1 << 0) /* readable from device */
#define VFIO_DMA_MAP_FLAG_WRITE (1 << 1) /* writable from device */
+/* reserved iova for MSI vectors*/
+#define VFIO_DMA_MAP_FLAG_RESERVED_MSI_IOVA (1 << 2)
__u64 vaddr; /* Process virtual address */
__u64 iova; /* IO virtual address */
__u64 size; /* Size of mapping (bytes) */
@@ -519,7 +526,8 @@ struct vfio_iommu_type1_dma_map {
* Caller sets argsz. The actual unmapped size is returned in the size
* field. No guarantee is made to the user that arbitrary unmaps of iova
* or size different from those used in the original mapping call will
- * succeed.
+ * succeed. Once registered, an MSI region cannot be unmapped and stays
+ * until the container is closed.
*/
struct vfio_iommu_type1_dma_unmap {
__u32 argsz;
--
1.9.1
^ permalink raw reply related
* [PATCH v14 12/16] vfio/type1: Handle unmap/unpin and replay for VFIO_IOVA_RESERVED slots
From: Eric Auger @ 2016-10-12 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476278544-3397-1-git-send-email-eric.auger@redhat.com>
Before allowing the end-user to create VFIO_IOVA_RESERVED dma slots,
let's implement the expected behavior for removal and replay.
As opposed to user dma slots, reserved IOVAs are not systematically bound
to PAs and PAs are not pinned. VFIO just initializes the IOVA "aperture".
IOVAs are allocated outside of the VFIO framework, by the MSI layer which
is responsible to free and unmap them. The MSI mapping resources are freed
by the IOMMU driver on domain destruction.
On the creation of a new domain, the "replay" of a reserved slot simply
needs to set the MSI aperture on the new domain.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
v13 -> v14:
- make iommu_get_dma_msi_region_cookie's failure not passable
- remove useless "select IOMMU_DMA" causing cyclic dependency
- set the MSI region only if needed
v12 -> v13:
- use dma-iommu iommu_get_dma_msi_region_cookie
v9 -> v10:
- replay of a reserved slot sets the MSI aperture on the new domain
- use VFIO_IOVA_RESERVED_MSI enum value instead of VFIO_IOVA_RESERVED
v7 -> v8:
- do no destroy anything anymore, just bypass unmap/unpin and iommu_map
on replay
---
drivers/vfio/vfio_iommu_type1.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 1f120f9..2108e2e 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -36,6 +36,7 @@
#include <linux/uaccess.h>
#include <linux/vfio.h>
#include <linux/workqueue.h>
+#include <linux/dma-iommu.h>
#define DRIVER_VERSION "0.2"
#define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>"
@@ -386,7 +387,7 @@ static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
struct vfio_domain *domain, *d;
long unlocked = 0;
- if (!dma->size)
+ if (!dma->size || dma->type != VFIO_IOVA_USER)
return;
/*
* We use the IOMMU to track the physical addresses, otherwise we'd
@@ -717,12 +718,24 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
return -EINVAL;
for (; n; n = rb_next(n)) {
+ struct iommu_domain_msi_resv msi_resv;
struct vfio_dma *dma;
dma_addr_t iova;
dma = rb_entry(n, struct vfio_dma, node);
iova = dma->iova;
+ if ((dma->type == VFIO_IOVA_RESERVED_MSI) &&
+ (!iommu_domain_get_attr(domain->domain,
+ DOMAIN_ATTR_MSI_RESV,
+ &msi_resv))) {
+ ret = iommu_get_dma_msi_region_cookie(domain->domain,
+ dma->iova,
+ dma->size);
+ if (ret)
+ return ret;
+ }
+
while (iova < dma->iova + dma->size) {
phys_addr_t phys = iommu_iova_to_phys(d->domain, iova);
size_t size;
--
1.9.1
^ permalink raw reply related
* [PATCH v14 11/16] vfio/type1: Implement recursive vfio_find_dma_from_node
From: Eric Auger @ 2016-10-12 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476278544-3397-1-git-send-email-eric.auger@redhat.com>
This patch handles the case where a node is encountered, matching
@start and @size arguments but not matching the @type argument.
In that case, we need to skip that node and pursue the search in the
node's leaves. In case @start is inferior to the node's base, we
resume the search on the left leaf. If the recursive search on the left
leaves did not produce any match, we search the right leaves recursively.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Acked-by: Alex Williamson <alex.williamson@redhat.com>
---
v10: creation
---
drivers/vfio/vfio_iommu_type1.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 1bd16ff..1f120f9 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -125,7 +125,17 @@ static struct vfio_dma *vfio_find_dma_from_node(struct rb_node *top,
if (type == VFIO_IOVA_ANY || dma->type == type)
return dma;
- return NULL;
+ /* restart 2 searches skipping the current node */
+ if (start < dma->iova) {
+ dma = vfio_find_dma_from_node(node->rb_left, start,
+ size, type);
+ if (dma)
+ return dma;
+ }
+ if (start + size > dma->iova + dma->size)
+ dma = vfio_find_dma_from_node(node->rb_right, start,
+ size, type);
+ return dma;
}
/**
--
1.9.1
^ permalink raw reply related
* [PATCH v14 10/16] vfio/type1: vfio_find_dma accepting a type argument
From: Eric Auger @ 2016-10-12 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476278544-3397-1-git-send-email-eric.auger@redhat.com>
In our RB-tree we get prepared to insert slots of different types
(USER and RESERVED). It becomes useful to be able to search for dma
slots of a specific type or any type.
This patch introduces vfio_find_dma_from_node which starts the
search from a given node and stops on the first node that matches
the @start and @size parameters. If this node also matches the
@type parameter, the node is returned else NULL is returned.
At the moment we only have USER SLOTS so the type will always match.
In a separate patch, this function will be enhanced to pursue the
search recursively in case a node with a different type is
encountered.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
v13 -> v14:
- remove top_node variable
---
drivers/vfio/vfio_iommu_type1.c | 52 +++++++++++++++++++++++++++++++++--------
1 file changed, 42 insertions(+), 10 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index a9f8b93..1bd16ff 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -94,25 +94,55 @@ struct vfio_group {
* into DMA'ble space using the IOMMU
*/
-static struct vfio_dma *vfio_find_dma(struct vfio_iommu *iommu,
- dma_addr_t start, size_t size)
+/**
+ * vfio_find_dma_from_node: looks for a dma slot intersecting a window
+ * from a given rb tree node
+ * @top: top rb tree node where the search starts (including this node)
+ * @start: window start
+ * @size: window size
+ * @type: window type
+ */
+static struct vfio_dma *vfio_find_dma_from_node(struct rb_node *top,
+ dma_addr_t start, size_t size,
+ enum vfio_iova_type type)
{
- struct rb_node *node = iommu->dma_list.rb_node;
+ struct rb_node *node = top;
+ struct vfio_dma *dma;
while (node) {
- struct vfio_dma *dma = rb_entry(node, struct vfio_dma, node);
-
+ dma = rb_entry(node, struct vfio_dma, node);
if (start + size <= dma->iova)
node = node->rb_left;
else if (start >= dma->iova + dma->size)
node = node->rb_right;
else
- return dma;
+ break;
}
+ if (!node)
+ return NULL;
+
+ /* a dma slot intersects our window, check the type also matches */
+ if (type == VFIO_IOVA_ANY || dma->type == type)
+ return dma;
return NULL;
}
+/**
+ * vfio_find_dma: find a dma slot intersecting a given window
+ * @iommu: vfio iommu handle
+ * @start: window base iova
+ * @size: window size
+ * @type: window type
+ */
+static struct vfio_dma *vfio_find_dma(struct vfio_iommu *iommu,
+ dma_addr_t start, size_t size,
+ enum vfio_iova_type type)
+{
+ return vfio_find_dma_from_node(iommu->dma_list.rb_node,
+ start, size, type);
+}
+
static void vfio_link_dma(struct vfio_iommu *iommu, struct vfio_dma *new)
{
struct rb_node **link = &iommu->dma_list.rb_node, *parent = NULL;
@@ -484,19 +514,21 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
* mappings within the range.
*/
if (iommu->v2) {
- dma = vfio_find_dma(iommu, unmap->iova, 0);
+ dma = vfio_find_dma(iommu, unmap->iova, 0, VFIO_IOVA_USER);
if (dma && dma->iova != unmap->iova) {
ret = -EINVAL;
goto unlock;
}
- dma = vfio_find_dma(iommu, unmap->iova + unmap->size - 1, 0);
+ dma = vfio_find_dma(iommu, unmap->iova + unmap->size - 1, 0,
+ VFIO_IOVA_USER);
if (dma && dma->iova + dma->size != unmap->iova + unmap->size) {
ret = -EINVAL;
goto unlock;
}
}
- while ((dma = vfio_find_dma(iommu, unmap->iova, unmap->size))) {
+ while ((dma = vfio_find_dma(iommu, unmap->iova, unmap->size,
+ VFIO_IOVA_USER))) {
if (!iommu->v2 && unmap->iova > dma->iova)
break;
unmapped += dma->size;
@@ -600,7 +632,7 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
mutex_lock(&iommu->lock);
- if (vfio_find_dma(iommu, iova, size)) {
+ if (vfio_find_dma(iommu, iova, size, VFIO_IOVA_ANY)) {
mutex_unlock(&iommu->lock);
return -EEXIST;
}
--
1.9.1
^ permalink raw reply related
* [PATCH v14 09/16] vfio: Introduce a vfio_dma type field
From: Eric Auger @ 2016-10-12 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476278544-3397-1-git-send-email-eric.auger@redhat.com>
We introduce a vfio_dma type since we will need to discriminate
different types of dma slots:
- VFIO_IOVA_USER: IOVA region used to map user vaddr
- VFIO_IOVA_RESERVED_MSI: IOVA region reserved to map MSI doorbells
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Acked-by: Alex Williamson <alex.williamson@redhat.com>
---
v9 -> v10:
- renamed VFIO_IOVA_RESERVED into VFIO_IOVA_RESERVED_MSI
- explicitly set type to VFIO_IOVA_USER on dma_map
v6 -> v7:
- add VFIO_IOVA_ANY
- do not introduce yet any VFIO_IOVA_RESERVED handling
---
drivers/vfio/vfio_iommu_type1.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 2ba1942..a9f8b93 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -53,6 +53,12 @@ module_param_named(disable_hugepages,
MODULE_PARM_DESC(disable_hugepages,
"Disable VFIO IOMMU support for IOMMU hugepages.");
+enum vfio_iova_type {
+ VFIO_IOVA_USER = 0, /* standard IOVA used to map user vaddr */
+ VFIO_IOVA_RESERVED_MSI, /* reserved to map MSI doorbells */
+ VFIO_IOVA_ANY, /* matches any IOVA type */
+};
+
struct vfio_iommu {
struct list_head domain_list;
struct mutex lock;
@@ -75,6 +81,7 @@ struct vfio_dma {
unsigned long vaddr; /* Process virtual addr */
size_t size; /* Map size (bytes) */
int prot; /* IOMMU_READ/WRITE */
+ enum vfio_iova_type type; /* type of IOVA */
};
struct vfio_group {
@@ -607,6 +614,7 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
dma->iova = iova;
dma->vaddr = vaddr;
dma->prot = prot;
+ dma->type = VFIO_IOVA_USER;
/* Insert zero-sized and grow as we map chunks of it */
vfio_link_dma(iommu, dma);
--
1.9.1
^ permalink raw reply related
* [PATCH v14 08/16] irqchip/gicv3-its: Register the MSI doorbell
From: Eric Auger @ 2016-10-12 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476278544-3397-1-git-send-email-eric.auger@redhat.com>
This patch registers the ITS global doorbell. Registered information
are needed to set up the KVM passthrough use case.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
v13 -> v14:
- use iommu_msi_doorbell_alloc/free
v12 -> v13:
- use new doorbell registration prototype
v11 -> v12:
- use new irq_get_msi_doorbell_info name
- simplify error handling
v10 -> v11:
- adapt to new doorbell registration API and implement msi_doorbell_info
---
drivers/irqchip/irq-gic-v3-its.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 98ff669..b42e006 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -86,6 +86,7 @@ struct its_node {
u32 ite_size;
u32 device_ids;
int numa_node;
+ struct iommu_msi_doorbell_info *doorbell_info;
};
#define ITS_ITT_ALIGN SZ_256
@@ -1717,6 +1718,7 @@ static int __init its_probe(struct device_node *node,
if (of_property_read_bool(node, "msi-controller")) {
struct msi_domain_info *info;
+ phys_addr_t translater;
info = kzalloc(sizeof(*info), GFP_KERNEL);
if (!info) {
@@ -1724,10 +1726,21 @@ static int __init its_probe(struct device_node *node,
goto out_free_tables;
}
+ translater = its->phys_base + GITS_TRANSLATER;
+ its->doorbell_info =
+ iommu_msi_doorbell_alloc(translater, sizeof(u32), true);
+
+ if (IS_ERR(its->doorbell_info)) {
+ kfree(info);
+ goto out_free_tables;
+ }
+
+
inner_domain = irq_domain_add_tree(node, &its_domain_ops, its);
if (!inner_domain) {
err = -ENOMEM;
kfree(info);
+ iommu_msi_doorbell_free(its->doorbell_info);
goto out_free_tables;
}
--
1.9.1
^ permalink raw reply related
* [PATCH v14 07/16] irqchip/gic-v2m: Register the MSI doorbell
From: Eric Auger @ 2016-10-12 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476278544-3397-1-git-send-email-eric.auger@redhat.com>
Register the GIC V2M global doorbell. The registered information
are used to set up the KVM passthrough use case.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
v13 -> v14:
- use iommu_msi_doorbell_alloc/free
v12 -> v13:
- use new msi doorbell registration prototype
- remove iommu protection attributes
- add unregistration in teardown
v11 -> v12:
- use irq_get_msi_doorbell_info new name
- simplify error handling
v10 -> v11:
- use the new registration API and re-implement the msi_doorbell_info
ops
v9 -> v10:
- introduce the registration concept in place of msi_doorbell_info
callback
v8 -> v9:
- use global_doorbell instead of percpu_doorbells
v7 -> v8:
- gicv2m_msi_doorbell_info does not return a pointer to const
- remove spurious !v2m check
- add IOMMU_MMIO flag
v7: creation
---
drivers/irqchip/irq-gic-v2m.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
index 863e073..33acfe0 100644
--- a/drivers/irqchip/irq-gic-v2m.c
+++ b/drivers/irqchip/irq-gic-v2m.c
@@ -70,6 +70,7 @@ struct v2m_data {
u32 spi_offset; /* offset to be subtracted from SPI number */
unsigned long *bm; /* MSI vector bitmap */
u32 flags; /* v2m flags for specific implementation */
+ struct iommu_msi_doorbell_info *doorbell_info; /* MSI doorbell */
};
static void gicv2m_mask_msi_irq(struct irq_data *d)
@@ -254,6 +255,7 @@ static void gicv2m_teardown(void)
struct v2m_data *v2m, *tmp;
list_for_each_entry_safe(v2m, tmp, &v2m_nodes, entry) {
+ iommu_msi_doorbell_free(v2m->doorbell_info);
list_del(&v2m->entry);
kfree(v2m->bm);
iounmap(v2m->base);
@@ -370,12 +372,18 @@ static int __init gicv2m_init_one(struct fwnode_handle *fwnode,
goto err_iounmap;
}
+ v2m->doorbell_info = iommu_msi_doorbell_alloc(v2m->res.start,
+ sizeof(u32), false);
+ if (IS_ERR(v2m->doorbell_info))
+ goto err_free_bm;
+
list_add_tail(&v2m->entry, &v2m_nodes);
pr_info("range%pR, SPI[%d:%d]\n", res,
v2m->spi_start, (v2m->spi_start + v2m->nr_spis - 1));
return 0;
-
+err_free_bm:
+ kfree(v2m->bm);
err_iounmap:
iounmap(v2m->base);
err_free_v2m:
--
1.9.1
^ permalink raw reply related
* [PATCH v14 06/16] iommu/arm-smmu: Implement domain_get_attr for DOMAIN_ATTR_MSI_RESV
From: Eric Auger @ 2016-10-12 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476278544-3397-1-git-send-email-eric.auger@redhat.com>
ARM smmu and smmu-v3 translate MSI transactions so their driver
are must implement domain_get_attr for DOMAIN_ATTR_MSI_RESV.
This allows to retrieve the size and alignment requirements of
the MSI reserved IOVA window.
Also IOMMU_DMA gets selected since it exposes the API to map the
MSIs.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
drivers/iommu/Kconfig | 4 ++--
drivers/iommu/arm-smmu-v3.c | 7 +++++++
drivers/iommu/arm-smmu.c | 7 +++++++
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 8ee54d7..f5e5e4b 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -297,7 +297,7 @@ config SPAPR_TCE_IOMMU
config ARM_SMMU
bool "ARM Ltd. System MMU (SMMU) Support"
depends on (ARM64 || ARM) && MMU
- select IOMMU_API
+ select IOMMU_DMA
select IOMMU_IO_PGTABLE_LPAE
select ARM_DMA_USE_IOMMU if ARM
help
@@ -310,7 +310,7 @@ config ARM_SMMU
config ARM_SMMU_V3
bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support"
depends on ARM64
- select IOMMU_API
+ select IOMMU_DMA
select IOMMU_IO_PGTABLE_LPAE
select GENERIC_MSI_IRQ_DOMAIN
help
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 15c01c3..572cad8 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1559,6 +1559,9 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain)
if (ret < 0)
free_io_pgtable_ops(pgtbl_ops);
+ if (domain->type == IOMMU_DOMAIN_UNMANAGED)
+ iommu_calc_msi_resv(domain);
+
return ret;
}
@@ -1840,6 +1843,10 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
case DOMAIN_ATTR_NESTING:
*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
return 0;
+ case DOMAIN_ATTR_MSI_RESV:
+ *(struct iommu_domain_msi_resv *)data =
+ smmu_domain->domain.msi_resv;
+ return 0;
default:
return -ENODEV;
}
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index ac4aab9..ae20b9c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -943,6 +943,9 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
domain->geometry.aperture_end = (1UL << ias) - 1;
domain->geometry.force_aperture = true;
+ if (domain->type == IOMMU_DOMAIN_UNMANAGED)
+ iommu_calc_msi_resv(domain);
+
/* Initialise the context bank with our page table cfg */
arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
@@ -1486,6 +1489,10 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
case DOMAIN_ATTR_NESTING:
*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
return 0;
+ case DOMAIN_ATTR_MSI_RESV:
+ *(struct iommu_domain_msi_resv *)data =
+ smmu_domain->domain.msi_resv;
+ return 0;
default:
return -ENODEV;
}
--
1.9.1
^ permalink raw reply related
* [PATCH v14 05/16] iommu/dma: Introduce iommu_calc_msi_resv
From: Eric Auger @ 2016-10-12 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476278544-3397-1-git-send-email-eric.auger@redhat.com>
iommu_calc_msi_resv() sum up the number of iommu pages of the lowest
order supported by the iommu domain requested to map all the registered
doorbells. This function will allow to dimension the intermediate
physical address (IPA) aperture requested to map the MSI doorbells.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
v13 -> v14
- name and proto changed, moved to dma-iommu
---
drivers/iommu/dma-iommu.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++
include/linux/dma-iommu.h | 11 +++++++-
2 files changed, 80 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index d8a7d86..3a4b73b 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -830,3 +830,73 @@ bool iommu_msi_doorbell_safe(void)
return !nb_unsafe_doorbells;
}
EXPORT_SYMBOL_GPL(iommu_msi_doorbell_safe);
+
+/**
+ * calc_region_reqs - compute the number of pages requested to map a region
+ *
+ * @addr: physical base address of the region
+ * @size: size of the region
+ * @order: the page order
+ *
+ * Return: the number of requested pages to map this region
+ */
+static int calc_region_reqs(phys_addr_t addr, size_t size, unsigned int order)
+{
+ phys_addr_t offset, granule;
+ unsigned int nb_pages;
+
+ granule = (uint64_t)(1 << order);
+ offset = addr & (granule - 1);
+ size = ALIGN(size + offset, granule);
+ nb_pages = size >> order;
+
+ return nb_pages;
+}
+
+/**
+ * calc_dbinfo_reqs - compute the number of pages requested to map a given
+ * MSI doorbell
+ *
+ * @dbi: doorbell info descriptor
+ * @order: page order
+ *
+ * Return: the number of requested pages to map this doorbell
+ */
+static int calc_dbinfo_reqs(struct iommu_msi_doorbell_info *dbi,
+ unsigned int order)
+{
+ int ret = 0;
+
+ if (!dbi->doorbell_is_percpu) {
+ ret = calc_region_reqs(dbi->global_doorbell, dbi->size, order);
+ } else {
+ phys_addr_t __percpu *pbase;
+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ pbase = per_cpu_ptr(dbi->percpu_doorbells, cpu);
+ ret += calc_region_reqs(*pbase, dbi->size, order);
+ }
+ }
+ return ret;
+}
+
+int iommu_calc_msi_resv(struct iommu_domain *domain)
+{
+ unsigned long order = __ffs(domain->pgsize_bitmap);
+ struct iommu_msi_doorbell *db;
+ phys_addr_t granule;
+ int size = 0;
+
+ mutex_lock(&iommu_msi_doorbell_mutex);
+ list_for_each_entry(db, &iommu_msi_doorbell_list, next)
+ size += calc_dbinfo_reqs(&db->info, order);
+
+ mutex_unlock(&iommu_msi_doorbell_mutex);
+
+ granule = (uint64_t)(1 << order);
+ domain->msi_resv.size = size * granule;
+ domain->msi_resv.alignment = granule;
+
+ return 0;
+}
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 9640a27..95875c8 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -97,6 +97,16 @@ void iommu_msi_doorbell_free(struct iommu_msi_doorbell_info *db);
*/
bool iommu_msi_doorbell_safe(void);
+/**
+ * iommu_calc_msi_resv - compute the number of pages of the lowest order
+ * supported by @domain, requested to map all the registered doorbells.
+ *
+ * @domain: iommu_domain
+ * @msi_resv: MSI reserved window requirements
+ *
+ */
+int iommu_calc_msi_resv(struct iommu_domain *domain);
+
#else
struct iommu_domain;
@@ -139,7 +149,6 @@ static inline bool iommu_msi_doorbell_safe(void)
{
return false;
}
-
#endif /* CONFIG_IOMMU_DMA */
#endif /* __KERNEL__ */
#endif /* __DMA_IOMMU_H */
--
1.9.1
^ permalink raw reply related
* [PATCH v14 04/16] iommu/dma: MSI doorbell alloc/free
From: Eric Auger @ 2016-10-12 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476278544-3397-1-git-send-email-eric.auger@redhat.com>
We introduce the capability to (un)register MSI doorbells.
A doorbell region is characterized by its physical address base, size,
and whether it its safe (ie. it implements IRQ remapping). A doorbell
can be per-cpu or global. We currently only care about global doorbells.
A function returns whether all registered doorbells are safe.
MSI controllers likely to work along with IOMMU that translate MSI
transaction must register their doorbells to allow device assignment
with MSI support. Otherwise the MSI transactions will cause IOMMU faults.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
v13 -> v14:
- previously in msi-doorbell.h/c
---
drivers/iommu/dma-iommu.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++
include/linux/dma-iommu.h | 41 ++++++++++++++++++++++++++
2 files changed, 116 insertions(+)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index d45f9a0..d8a7d86 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -43,6 +43,38 @@ struct iommu_dma_cookie {
spinlock_t msi_lock;
};
+/**
+ * struct iommu_msi_doorbell_info - MSI doorbell region descriptor
+ * @percpu_doorbells: per cpu doorbell base address
+ * @global_doorbell: base address of the doorbell
+ * @doorbell_is_percpu: is the doorbell per cpu or global?
+ * @safe: true if irq remapping is implemented
+ * @size: size of the doorbell
+ */
+struct iommu_msi_doorbell_info {
+ union {
+ phys_addr_t __percpu *percpu_doorbells;
+ phys_addr_t global_doorbell;
+ };
+ bool doorbell_is_percpu;
+ bool safe;
+ size_t size;
+};
+
+struct iommu_msi_doorbell {
+ struct iommu_msi_doorbell_info info;
+ struct list_head next;
+};
+
+/* list of registered MSI doorbells */
+static LIST_HEAD(iommu_msi_doorbell_list);
+
+/* counts the number of unsafe registered doorbells */
+static uint nb_unsafe_doorbells;
+
+/* protects the list and nb_unsafe_doorbells */
+static DEFINE_MUTEX(iommu_msi_doorbell_mutex);
+
static inline struct iova_domain *cookie_iovad(struct iommu_domain *domain)
{
return &((struct iommu_dma_cookie *)domain->iova_cookie)->iovad;
@@ -755,3 +787,46 @@ int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
return 0;
}
EXPORT_SYMBOL(iommu_get_dma_msi_region_cookie);
+
+struct iommu_msi_doorbell_info *
+iommu_msi_doorbell_alloc(phys_addr_t base, size_t size, bool safe)
+{
+ struct iommu_msi_doorbell *db;
+
+ db = kzalloc(sizeof(*db), GFP_KERNEL);
+ if (!db)
+ return ERR_PTR(-ENOMEM);
+
+ db->info.global_doorbell = base;
+ db->info.size = size;
+ db->info.safe = safe;
+
+ mutex_lock(&iommu_msi_doorbell_mutex);
+ list_add(&db->next, &iommu_msi_doorbell_list);
+ if (!db->info.safe)
+ nb_unsafe_doorbells++;
+ mutex_unlock(&iommu_msi_doorbell_mutex);
+ return &db->info;
+}
+EXPORT_SYMBOL_GPL(iommu_msi_doorbell_alloc);
+
+void iommu_msi_doorbell_free(struct iommu_msi_doorbell_info *dbinfo)
+{
+ struct iommu_msi_doorbell *db;
+
+ db = container_of(dbinfo, struct iommu_msi_doorbell, info);
+
+ mutex_lock(&iommu_msi_doorbell_mutex);
+ list_del(&db->next);
+ if (!db->info.safe)
+ nb_unsafe_doorbells--;
+ mutex_unlock(&iommu_msi_doorbell_mutex);
+ kfree(db);
+}
+EXPORT_SYMBOL_GPL(iommu_msi_doorbell_free);
+
+bool iommu_msi_doorbell_safe(void)
+{
+ return !nb_unsafe_doorbells;
+}
+EXPORT_SYMBOL_GPL(iommu_msi_doorbell_safe);
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 05ab5b4..9640a27 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -19,6 +19,8 @@
#ifdef __KERNEL__
#include <asm/errno.h>
+struct iommu_msi_doorbell_info;
+
#ifdef CONFIG_IOMMU_DMA
#include <linux/iommu.h>
#include <linux/msi.h>
@@ -70,6 +72,31 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg);
int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
dma_addr_t base, u64 size);
+/**
+ * iommu_msi_doorbell_alloc - allocate a global doorbell
+ * @base: physical base address of the doorbell
+ * @size: size of the doorbell
+ * @safe: true is irq_remapping implemented for this doorbell
+ *
+ * Return: the newly allocated doorbell info or a pointer converted error
+ */
+struct iommu_msi_doorbell_info *
+iommu_msi_doorbell_alloc(phys_addr_t base, size_t size, bool safe);
+
+/**
+ * iommu_msi_doorbell_free - free a global doorbell
+ * @db: doorbell info to free
+ */
+void iommu_msi_doorbell_free(struct iommu_msi_doorbell_info *db);
+
+/**
+ * iommu_msi_doorbell_safe - return whether all registered doorbells are safe
+ *
+ * Safe doorbells are those which implement irq remapping
+ * Return: true if all doorbells are safe, false otherwise
+ */
+bool iommu_msi_doorbell_safe(void);
+
#else
struct iommu_domain;
@@ -99,6 +126,20 @@ static inline int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
return -ENODEV;
}
+static inline struct iommu_msi_doorbell_info *
+iommu_msi_doorbell_alloc(phys_addr_t base, size_t size, bool safe)
+{
+ return NULL;
+}
+
+static inline void
+iommu_msi_doorbell_free(struct msi_doorbell_info *db) {}
+
+static inline bool iommu_msi_doorbell_safe(void)
+{
+ return false;
+}
+
#endif /* CONFIG_IOMMU_DMA */
#endif /* __KERNEL__ */
#endif /* __DMA_IOMMU_H */
--
1.9.1
^ permalink raw reply related
* [PATCH v14 03/16] iommu/dma: Allow MSI-only cookies
From: Eric Auger @ 2016-10-12 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476278544-3397-1-git-send-email-eric.auger@redhat.com>
From: Robin Murphy <robin.murphy@arm.com>
IOMMU domain users such as VFIO face a similar problem to DMA API ops
with regard to mapping MSI messages in systems where the MSI write is
subject to IOMMU translation. With the relevant infrastructure now in
place for managed DMA domains, it's actually really simple for other
users to piggyback off that and reap the benefits without giving up
their own IOVA management, and without having to reinvent their own
wheel in the MSI layer.
Allow such users to opt into automatic MSI remapping by dedicating a
region of their IOVA space to a managed cookie.
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
v13 -> v14:
- restore reserve_iova for iova >= base + size
v1 -> v13 incorpration:
- compared to Robin's version
- add NULL last param to iommu_dma_init_domain
- set the msi_geometry aperture
- I removed
if (base < U64_MAX - size)
reserve_iova(iovad, iova_pfn(iovad, base + size), ULONG_MAX);
don't get why we would reserve something out of the scope of the iova domain?
what do I miss?
---
drivers/iommu/dma-iommu.c | 39 +++++++++++++++++++++++++++++++++++++++
include/linux/dma-iommu.h | 9 +++++++++
2 files changed, 48 insertions(+)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index c5ab866..d45f9a0 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -716,3 +716,42 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
msg->address_lo += lower_32_bits(msi_page->iova);
}
}
+
+/**
+ * iommu_get_dma_msi_region_cookie - Configure a domain for MSI remapping only
+ * @domain: IOMMU domain to prepare
+ * @base: Base address of IOVA region to use as the MSI remapping aperture
+ * @size: Size of the desired MSI aperture
+ *
+ * Users who manage their own IOVA allocation and do not want DMA API support,
+ * but would still like to take advantage of automatic MSI remapping, can use
+ * this to initialise their own domain appropriately.
+ */
+int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
+ dma_addr_t base, u64 size)
+{
+ struct iommu_dma_cookie *cookie;
+ struct iova_domain *iovad;
+ int ret;
+
+ if (domain->type == IOMMU_DOMAIN_DMA)
+ return -EINVAL;
+
+ ret = iommu_get_dma_cookie(domain);
+ if (ret)
+ return ret;
+
+ ret = iommu_dma_init_domain(domain, base, size, NULL);
+ if (ret) {
+ iommu_put_dma_cookie(domain);
+ return ret;
+ }
+
+ cookie = domain->iova_cookie;
+ iovad = &cookie->iovad;
+ if (base < U64_MAX - size)
+ reserve_iova(iovad, iova_pfn(iovad, base + size), ULONG_MAX);
+
+ return 0;
+}
+EXPORT_SYMBOL(iommu_get_dma_msi_region_cookie);
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 32c5890..05ab5b4 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -67,6 +67,9 @@ int iommu_dma_mapping_error(struct device *dev, dma_addr_t dma_addr);
/* The DMA API isn't _quite_ the whole story, though... */
void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg);
+int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
+ dma_addr_t base, u64 size);
+
#else
struct iommu_domain;
@@ -90,6 +93,12 @@ static inline void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
{
}
+static inline int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
+ dma_addr_t base, u64 size)
+{
+ return -ENODEV;
+}
+
#endif /* CONFIG_IOMMU_DMA */
#endif /* __KERNEL__ */
#endif /* __DMA_IOMMU_H */
--
1.9.1
^ permalink raw reply related
* [PATCH v14 02/16] iommu: Introduce DOMAIN_ATTR_MSI_RESV
From: Eric Auger @ 2016-10-12 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476278544-3397-1-git-send-email-eric.auger@redhat.com>
Introduce a new DOMAIN_ATTR_MSI_RESV domain attribute and associated
iommu_domain msi_resv field. It comprises the size and alignment
of the IOVA reserved window dedicated to MSI mapping. This attribute
only is supported when MSI must be IOMMU mapped.
This is the case on ARM.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Suggested-by: Alex Williamson <alex.williamson@redhat.com>
---
v13 -> v14:
- new msi_resv type and name
v12 -> v13:
- reword the commit message
v8 -> v9:
- rename programmable into iommu_msi_supported
- add iommu_domain_msi_aperture_valid
v8: creation
- deprecates DOMAIN_ATTR_MSI_MAPPING flag
---
include/linux/iommu.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 436dc21..aaeb598 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -52,6 +52,12 @@ struct iommu_domain_geometry {
bool force_aperture; /* DMA only allowed in mappable range? */
};
+/* MSI reserved IOVA window requirements */
+struct iommu_domain_msi_resv {
+ size_t size; /* size in bytes */
+ size_t alignment; /* byte alignment */
+};
+
/* Domain feature flags */
#define __IOMMU_DOMAIN_PAGING (1U << 0) /* Support for iommu_map/unmap */
#define __IOMMU_DOMAIN_DMA_API (1U << 1) /* Domain for use in DMA-API
@@ -83,6 +89,7 @@ struct iommu_domain {
iommu_fault_handler_t handler;
void *handler_token;
struct iommu_domain_geometry geometry;
+ struct iommu_domain_msi_resv msi_resv;
void *iova_cookie;
};
@@ -108,6 +115,7 @@ enum iommu_cap {
enum iommu_attr {
DOMAIN_ATTR_GEOMETRY,
+ DOMAIN_ATTR_MSI_RESV,
DOMAIN_ATTR_PAGING,
DOMAIN_ATTR_WINDOWS,
DOMAIN_ATTR_FSL_PAMU_STASH,
--
1.9.1
^ permalink raw reply related
* [PATCH v14 01/16] iommu/iova: fix __alloc_and_insert_iova_range
From: Eric Auger @ 2016-10-12 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476278544-3397-1-git-send-email-eric.auger@redhat.com>
Fix the size check within start_pfn and limit_pfn.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
the issue was observed when playing with 1 page iova domain with
higher iova reserved.
---
drivers/iommu/iova.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index e23001b..ee29dbf 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -147,7 +147,7 @@ move_left:
if (!curr) {
if (size_aligned)
pad_size = iova_get_pad_size(size, limit_pfn);
- if ((iovad->start_pfn + size + pad_size) > limit_pfn) {
+ if ((iovad->start_pfn + size + pad_size - 1) > limit_pfn) {
spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
return -ENOMEM;
}
--
1.9.1
^ permalink raw reply related
* [PATCH v14 00/16] KVM PCIe/MSI passthrough on ARM/ARM64
From: Eric Auger @ 2016-10-12 13:22 UTC (permalink / raw)
To: linux-arm-kernel
This is the second respin on top of Robin's series [1], addressing Alex' comments.
Major changes are:
- MSI-doorbell API now is moved to DMA IOMMU API following Alex suggestion
to put all API pieces at the same place (so eventually in the IOMMU
subsystem)
- new iommu_domain_msi_resv struct and accessor through DOMAIN_ATTR_MSI_RESV
domain with mirror VFIO capability
- more robustness I think in the VFIO layer
- added "iommu/iova: fix __alloc_and_insert_iova_range" since with the current
code I failed allocating an IOVA page in a single page domain with upper part
reserved
IOVA range exclusion will be handled in a separate series
The priority really is to discuss and freeze the API and especially the MSI
doorbell's handling. Do we agree to put that in DMA IOMMU?
Note: the size computation does not take into account possible page overlaps
between doorbells but it would add quite a lot of complexity i think.
Tested on AMD Overdrive (single GICv2m frame) with I350 VF assignment.
dependency:
the series depends on Robin's generic-v7 branch:
[1] [PATCH v7 00/22] Generic DT bindings for PCI IOMMUs and ARM SMMU
http://www.spinics.net/lists/arm-kernel/msg531110.html
Best Regards
Eric
Git: complete series available at
https://github.com/eauger/linux/tree/generic-v7-pcie-passthru-v14
the above branch includes a temporary patch to work around a ThunderX pci
bus reset crash (which I think unrelated to this series):
"vfio: pci: HACK! workaround thunderx pci_try_reset_bus crash"
Do not take this one for other platforms.
Eric Auger (15):
iommu/iova: fix __alloc_and_insert_iova_range
iommu: Introduce DOMAIN_ATTR_MSI_RESV
iommu/dma: MSI doorbell alloc/free
iommu/dma: Introduce iommu_calc_msi_resv
iommu/arm-smmu: Implement domain_get_attr for DOMAIN_ATTR_MSI_RESV
irqchip/gic-v2m: Register the MSI doorbell
irqchip/gicv3-its: Register the MSI doorbell
vfio: Introduce a vfio_dma type field
vfio/type1: vfio_find_dma accepting a type argument
vfio/type1: Implement recursive vfio_find_dma_from_node
vfio/type1: Handle unmap/unpin and replay for VFIO_IOVA_RESERVED slots
vfio: Allow reserved msi iova registration
vfio/type1: Check doorbell safety
iommu/arm-smmu: Do not advertise IOMMU_CAP_INTR_REMAP
vfio/type1: Introduce MSI_RESV capability
Robin Murphy (1):
iommu/dma: Allow MSI-only cookies
drivers/iommu/Kconfig | 4 +-
drivers/iommu/arm-smmu-v3.c | 10 +-
drivers/iommu/arm-smmu.c | 10 +-
drivers/iommu/dma-iommu.c | 184 ++++++++++++++++++++++++++
drivers/iommu/iova.c | 2 +-
drivers/irqchip/irq-gic-v2m.c | 10 +-
drivers/irqchip/irq-gic-v3-its.c | 13 ++
drivers/vfio/vfio_iommu_type1.c | 279 +++++++++++++++++++++++++++++++++++++--
include/linux/dma-iommu.h | 59 +++++++++
include/linux/iommu.h | 8 ++
include/uapi/linux/vfio.h | 30 ++++-
11 files changed, 587 insertions(+), 22 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
From: Adrian Hunter @ 2016-10-12 13:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4009a66b-1e09-0780-b7c1-128ff1401b5f@marvell.com>
On 12/10/16 14:58, Ziji Hu wrote:
> Hi Adrian,
>
> Thank you very much for your review.
> I will firstly fix the typo.
>
> On 2016/10/11 20:37, Adrian Hunter wrote:
>> On 07/10/16 18:22, Gregory CLEMENT wrote:
>>> From: Ziji Hu <huziji@marvell.com>
>>>
>>> Add Xenon eMMC/SD/SDIO host controller core functionality.
>>> Add Xenon specific intialization process.
>>> Add Xenon specific mmc_host_ops APIs.
>>> Add Xenon specific register definitions.
>>>
>>> Add CONFIG_MMC_SDHCI_XENON support in drivers/mmc/host/Kconfig.
>>>
>>> Marvell Xenon SDHC conforms to SD Physical Layer Specification
>>> Version 3.01 and is designed according to the guidelines provided
>>> in the SD Host Controller Standard Specification Version 3.00.
>>>
>>> Signed-off-by: Hu Ziji <huziji@marvell.com>
>>> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>
>> I looked at a couple of things but you need to sort out the issues with
>> card_candidate before going further.
>>
> Understood.
> I will improve the card_candidate. Please help check the details in below.
>
>>> ---
> <snip>
>>> +
>>> +static int xenon_emmc_signal_voltage_switch(struct mmc_host *mmc,
>>> + struct mmc_ios *ios)
>>> +{
>>> + unsigned char voltage = ios->signal_voltage;
>>> +
>>> + if ((voltage == MMC_SIGNAL_VOLTAGE_330) ||
>>> + (voltage == MMC_SIGNAL_VOLTAGE_180))
>>> + return __emmc_signal_voltage_switch(mmc, voltage);
>>> +
>>> + dev_err(mmc_dev(mmc), "Unsupported signal voltage: %d\n",
>>> + voltage);
>>> + return -EINVAL;
>>> +}
>>> +
>>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>>> + struct mmc_ios *ios)
>>> +{
>>> + struct sdhci_host *host = mmc_priv(mmc);
>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>> +
>>> + /*
>>> + * Before SD/SDIO set signal voltage, SD bus clock should be
>>> + * disabled. However, sdhci_set_clock will also disable the Internal
>>> + * clock in mmc_set_signal_voltage().
>>> + * If Internal clock is disabled, the 3.3V/1.8V bit can not be updated.
>>> + * Thus here manually enable internal clock.
>>> + *
>>> + * After switch completes, it is unnecessary to disable internal clock,
>>> + * since keeping internal clock active obeys SD spec.
>>> + */
>>> + enable_xenon_internal_clk(host);
>>> +
>>> + if (priv->card_candidate) {
>>
>> mmc_power_up() calls __mmc_set_signal_voltage() calls
>> host->ops->start_signal_voltage_switch so priv->card_candidate could be an
>> invalid reference to an old card.
>>
>> So that's not going to work if the card changes - not only for removable
>> cards but even for eMMC if init fails and retries.
>>
> As you point out, this piece of code have defects, even though it actually works on Marvell multiple platforms, unless eMMC card is removable.
>
> I can add a property to explicitly indicate eMMC type in DTS.
> Then card_candidate access can be removed here.
> Does it sounds more reasonable to you?
Sure
>
>>> + if (mmc_card_mmc(priv->card_candidate))
>>> + return xenon_emmc_signal_voltage_switch(mmc, ios);
>>
>> So if all you need to know is whether it is a eMMC, why can't DT tell you?
>>
> I can add an eMMC type property in DTS, to remove the card_candidate access here.
>
>>> + }
>>> +
>>> + return sdhci_start_signal_voltage_switch(mmc, ios);
>>> +}
>>> +
>>> +/*
>>> + * After determining which slot is used for SDIO,
>>> + * some additional task is required.
>>> + */
>>> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
>>> +{
>>> + struct sdhci_host *host = mmc_priv(mmc);
>>> + u32 reg;
>>> + u8 slot_idx;
>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>> +
>>> + /* Link the card for delay adjustment */
>>> + priv->card_candidate = card;
>>
>> You really need a better way to get the card. I suggest you take up the
>> issue with Ulf. One possibility is to have mmc core set host->card = card
>> much earlier.
>>
> Could you please tell me if any issue related to card_candidate still exists, after the card_candidate is removed from xenon_start_signal_voltage_switch() in above?
> It seems that when init_card is called, the structure card has already been updated and stable in MMC/SD/SDIO initialization sequence.
> May I keep it here?
It works by accident rather than by design. We can do better.
>
>>> + /* Set tuning functionality of this slot */
>>> + xenon_slot_tuning_setup(host);
>>> +
>>> + slot_idx = priv->slot_idx;
>>> + if (!mmc_card_sdio(card)) {
>>> + /* Re-enable the Auto-CMD12 cap flag. */
>>> + host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
>>> + host->flags |= SDHCI_AUTO_CMD12;
>>> +
>>> + /* Clear SDIO Card Inserted indication */
>>> + reg = sdhci_readl(host, SDHC_SYS_CFG_INFO);
>>> + reg &= ~(1 << (slot_idx + SLOT_TYPE_SDIO_SHIFT));
>>> + sdhci_writel(host, reg, SDHC_SYS_CFG_INFO);
>>> +
>>> + if (mmc_card_mmc(card)) {
>>> + mmc->caps |= MMC_CAP_NONREMOVABLE;
>>> + if (!(host->quirks2 & SDHCI_QUIRK2_NO_1_8_V))
>>> + mmc->caps |= MMC_CAP_1_8V_DDR;
>>> + /*
>>> + * Force to clear BUS_TEST to
>>> + * skip bus_test_pre and bus_test_post
>>> + */
>>> + mmc->caps &= ~MMC_CAP_BUS_WIDTH_TEST;
>>> + mmc->caps2 |= MMC_CAP2_HC_ERASE_SZ |
>>> + MMC_CAP2_PACKED_CMD;
>>> + if (mmc->caps & MMC_CAP_8_BIT_DATA)
>>> + mmc->caps2 |= MMC_CAP2_HS400_1_8V;
>>> + }
>>> + } else {
>>> + /*
>>> + * Delete the Auto-CMD12 cap flag.
>>> + * Otherwise, when sending multi-block CMD53,
>>> + * Driver will set Transfer Mode Register to enable Auto CMD12.
>>> + * However, SDIO device cannot recognize CMD12.
>>> + * Thus SDHC will time-out for waiting for CMD12 response.
>>> + */
>>> + host->quirks &= ~SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
>>> + host->flags &= ~SDHCI_AUTO_CMD12;
>>
>> sdhci_set_transfer_mode() won't enable auto-CMD12 for CMD53 anyway, so is
>> this needed?
>>
> In Xenon driver, Auto-CMD12 flag is set to enable full support to Auto-CMD feature, both Auto-CMD12 and Auto-CMD23.
> As a result, when Xenon SDHC slot can both support SD and SDIO, Auto-CMD12 is disabled when SDIO card is inserted, and renabled when SD is inserted.
>
> I recheck the sdhci code to set Auto-CMD bit in Transfer Mode register, in sdhci_set_transfer_mode():
> if (mmc_op_multi(cmd->opcode) || data->blocks > 1)
> As you can see, as long as it is CMD18/CMD25 OR there are multiple data blocks, Auto-CMD field will be set.
> CMD53 doesn't have CMD23. Thus Auto-CMD12 is selected since Auto-CMD12 flag is set.
> Thus I have to clear Auto-CMD12 to avoid issuing Auto-CMD12 in SDIO transfer.
The code is:
if (mmc_op_multi(cmd->opcode) || data->blocks > 1) {
mode = SDHCI_TRNS_BLK_CNT_EN | SDHCI_TRNS_MULTI;
/*
* If we are sending CMD23, CMD12 never gets sent
* on successful completion (so no Auto-CMD12).
*/
if (sdhci_auto_cmd12(host, cmd->mrq) &&
(cmd->opcode != SD_IO_RW_EXTENDED))
mode |= SDHCI_TRNS_AUTO_CMD12;
else if (cmd->mrq->sbc && (host->flags & SDHCI_AUTO_CMD23)) {
mode |= SDHCI_TRNS_AUTO_CMD23;
sdhci_writel(host, cmd->mrq->sbc->arg, SDHCI_ARGUMENT2);
}
}
You can see the check for SD_IO_RW_EXTENDED which is CMD53.
>
> I just meet a similar issue in RPMB.
> When Auto-CMD12 flag is set, eMMC RPMB access will trigger Auto-CMD12, since CMD25 is in use.
> It will cause RPMB access failed.
Can you explain more about the RPMB issue. Doesn't it use CMD23, so CMD12
wouldn't be used - auto or manually.
>
> One possible solution is to drop Auto-CMD12 support and use Auto-CMD23 only, in Xenon driver.
> May I know you opinion, please?
I don't use auto-CMD12 because I don't know if it provides any benefit and
sdhci does not seem to have implemented Auto CMD12 Error Recovery, although
I have never looked at it closely.
^ permalink raw reply
* [PATCH 2/2] power/reset: at91-poweroff: timely shitdown LPDDR memories
From: Jean-Jacques Hiblot @ 2016-10-12 12:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161007163427.11454-3-alexandre.belloni@free-electrons.com>
2016-10-07 18:34 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> LPDDR memories can only handle up to 400 uncontrolled power off. Ensure the
> proper power off sequence is used before shutting down the platform.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
> drivers/power/reset/at91-poweroff.c | 52 +++++++++++++++++++++++++++++++-
> drivers/power/reset/at91-sama5d2_shdwc.c | 48 ++++++++++++++++++++++++++++-
> 2 files changed, 98 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/power/reset/at91-poweroff.c b/drivers/power/reset/at91-poweroff.c
> index e9e24df35f26..bf97390e6cd7 100644
> --- a/drivers/power/reset/at91-poweroff.c
> +++ b/drivers/power/reset/at91-poweroff.c
> @@ -14,9 +14,12 @@
> #include <linux/io.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/of_address.h>
> #include <linux/platform_device.h>
> #include <linux/printk.h>
>
> +#include <soc/at91/at91sam9_ddrsdr.h>
> +
> #define AT91_SHDW_CR 0x00 /* Shut Down Control Register */
> #define AT91_SHDW_SHDW BIT(0) /* Shut Down command */
> #define AT91_SHDW_KEY (0xa5 << 24) /* KEY Password */
> @@ -50,6 +53,7 @@ static const char *shdwc_wakeup_modes[] = {
>
> static void __iomem *at91_shdwc_base;
> static struct clk *sclk;
> +static void __iomem *mpddrc_base;
>
> static void __init at91_wakeup_status(void)
> {
> @@ -73,6 +77,28 @@ static void at91_poweroff(void)
> writel(AT91_SHDW_KEY | AT91_SHDW_SHDW, at91_shdwc_base + AT91_SHDW_CR);
> }
>
> +static void at91_lpddr_poweroff(void)
> +{
> + asm volatile(
> + /* Align to cache lines */
> + ".balign 32\n\t"
> +
> + " ldr r6, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t"
At first sight, it looks useless. I assume it's used to preload the
TLB before the LPDDR is turned off.
A comment to explain why this line is useful would prevent its removal.
> +
> + /* Power down SDRAM0 */
> + " str %1, [%0, #" __stringify(AT91_DDRSDRC_LPR) "]\n\t"
> + /* Shutdown CPU */
> + " str %3, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t"
> +
> + " b .\n\t"
> + :
> + : "r" (mpddrc_base),
> + "r" cpu_to_le32(AT91_DDRSDRC_LPDDR2_PWOFF),
> + "r" (at91_shdwc_base),
> + "r" cpu_to_le32(AT91_SHDW_KEY | AT91_SHDW_SHDW)
> + : "r0");
> +}
> +
> static int at91_poweroff_get_wakeup_mode(struct device_node *np)
> {
> const char *pm;
> @@ -124,6 +150,8 @@ static void at91_poweroff_dt_set_wakeup_mode(struct platform_device *pdev)
> static int __init at91_poweroff_probe(struct platform_device *pdev)
> {
> struct resource *res;
> + struct device_node *np;
> + u32 ddr_type;
> int ret;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -150,12 +178,29 @@ static int __init at91_poweroff_probe(struct platform_device *pdev)
>
> pm_power_off = at91_poweroff;
>
> + np = of_find_compatible_node(NULL, NULL, "atmel,sama5d3-ddramc");
> + if (!np)
> + return 0;
> +
> + mpddrc_base = of_iomap(np, 0);
> + of_node_put(np);
> +
> + if (!mpddrc_base)
> + return 0;
> +
> + ddr_type = readl(mpddrc_base + AT91_DDRSDRC_MDR) & AT91_DDRSDRC_MD;
> + if ((ddr_type == AT91_DDRSDRC_MD_LPDDR2) ||
> + (ddr_type == AT91_DDRSDRC_MD_LPDDR3))
Souldn't there be something like "pm_power_off = at91_lpddr_poweroff;" here ?
Jean-Jacques
> + else
> + iounmap(mpddrc_base);
> +
> return 0;
> }
>
> static int __exit at91_poweroff_remove(struct platform_device *pdev)
> {
> - if (pm_power_off == at91_poweroff)
> + if (pm_power_off == at91_poweroff ||
> + pm_power_off == at91_lpddr_poweroff)
> pm_power_off = NULL;
>
> clk_disable_unprepare(sclk);
> @@ -163,6 +208,11 @@ static int __exit at91_poweroff_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static const struct of_device_id at91_ramc_of_match[] = {
> + { .compatible = "atmel,sama5d3-ddramc", },
> + { /* sentinel */ }
> +};
> +
> static const struct of_device_id at91_poweroff_of_match[] = {
> { .compatible = "atmel,at91sam9260-shdwc", },
> { .compatible = "atmel,at91sam9rl-shdwc", },
> diff --git a/drivers/power/reset/at91-sama5d2_shdwc.c b/drivers/power/reset/at91-sama5d2_shdwc.c
> index 8a5ac9706c9c..5736f360b374 100644
> --- a/drivers/power/reset/at91-sama5d2_shdwc.c
> +++ b/drivers/power/reset/at91-sama5d2_shdwc.c
> @@ -22,9 +22,12 @@
> #include <linux/io.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/of_address.h>
> #include <linux/platform_device.h>
> #include <linux/printk.h>
>
> +#include <soc/at91/at91sam9_ddrsdr.h>
> +
> #define SLOW_CLOCK_FREQ 32768
>
> #define AT91_SHDW_CR 0x00 /* Shut Down Control Register */
> @@ -75,6 +78,7 @@ struct shdwc {
> */
> static struct shdwc *at91_shdwc;
> static struct clk *sclk;
> +static void __iomem *mpddrc_base;
>
> static const unsigned long long sdwc_dbc_period[] = {
> 0, 3, 32, 512, 4096, 32768,
> @@ -108,6 +112,28 @@ static void at91_poweroff(void)
> at91_shdwc->at91_shdwc_base + AT91_SHDW_CR);
> }
>
> +static void at91_lpddr_poweroff(void)
> +{
> + asm volatile(
> + /* Align to cache lines */
> + ".balign 32\n\t"
> +
> + " ldr r6, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t"
> +
> + /* Power down SDRAM0 */
> + " str %1, [%0, #" __stringify(AT91_DDRSDRC_LPR) "]\n\t"
> + /* Shutdown CPU */
> + " str %3, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t"
> +
> + " b .\n\t"
> + :
> + : "r" (mpddrc_base),
> + "r" cpu_to_le32(AT91_DDRSDRC_LPDDR2_PWOFF),
> + "r" (at91_shdwc->at91_shdwc_base),
> + "r" cpu_to_le32(AT91_SHDW_KEY | AT91_SHDW_SHDW)
> + : "r0");
> +}
> +
> static u32 at91_shdwc_debouncer_value(struct platform_device *pdev,
> u32 in_period_us)
> {
> @@ -212,6 +238,8 @@ static int __init at91_shdwc_probe(struct platform_device *pdev)
> {
> struct resource *res;
> const struct of_device_id *match;
> + struct device_node *np;
> + u32 ddr_type;
> int ret;
>
> if (!pdev->dev.of_node)
> @@ -249,6 +277,23 @@ static int __init at91_shdwc_probe(struct platform_device *pdev)
>
> pm_power_off = at91_poweroff;
>
> + np = of_find_compatible_node(NULL, NULL, "atmel,sama5d3-ddramc");
> + if (!np)
> + return 0;
> +
> + mpddrc_base = of_iomap(np, 0);
> + of_node_put(np);
> +
> + if (!mpddrc_base)
> + return 0;
> +
> + ddr_type = readl(mpddrc_base + AT91_DDRSDRC_MDR) & AT91_DDRSDRC_MD;
> + if ((ddr_type == AT91_DDRSDRC_MD_LPDDR2) ||
> + (ddr_type == AT91_DDRSDRC_MD_LPDDR3))
> + pm_power_off = at91_lpddr_poweroff;
> + else
> + iounmap(mpddrc_base);
> +
> return 0;
> }
>
> @@ -256,7 +301,8 @@ static int __exit at91_shdwc_remove(struct platform_device *pdev)
> {
> struct shdwc *shdw = platform_get_drvdata(pdev);
>
> - if (pm_power_off == at91_poweroff)
> + if (pm_power_off == at91_poweroff ||
> + pm_power_off == at91_lpddr_poweroff)
> pm_power_off = NULL;
>
> /* Reset values to disable wake-up features */
> --
> 2.9.3
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [RFC PATCH 05/11] pci: rename *host* directory to *controller*
From: Christoph Hellwig @ 2016-10-12 12:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1473829927-20466-6-git-send-email-kishon@ti.com>
This is a big and painful change. I'd suggest to either drop it for
now or convince Bjorn to take it as a scripted renamed just after -rc1.
^ permalink raw reply
* [RFC PATCH 02/11] pci: endpoint: introduce configfs entry for configuring EP functions
From: Christoph Hellwig @ 2016-10-12 12:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1473829927-20466-3-git-send-email-kishon@ti.com>
On Wed, Sep 14, 2016 at 10:41:58AM +0530, Kishon Vijay Abraham I wrote:
> diff --git a/drivers/pci/endpoint/Kconfig b/drivers/pci/endpoint/Kconfig
> index a6d827c..f1dd206 100644
> --- a/drivers/pci/endpoint/Kconfig
> +++ b/drivers/pci/endpoint/Kconfig
> @@ -13,7 +13,9 @@ config PCI_ENDPOINT
>
> Enabling this option will build the endpoint library, which
> includes endpoint controller library and endpoint function
> - library.
> + library. This will also enable the configfs entry required to
> + configure the endpoint function and used to bind the
> + function with a endpoint controller.
>
> If in doubt, say "N" to disable Endpoint support.
This needs to grow a
depends on CONFIGFS_FS
> +/**
> + * pci-ep-cfs.c - configfs to configure the PCI endpoint
Please don't use the file name in the top of the file comment, it's
only bound to get out of date..
> +struct pci_epf_info {
> + struct config_item pci_epf;
> + struct pci_epf *epf;
> +};
Any reason not to simply embedd the config_item into the pci_epf structure?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox