The current AP bus implementation periodically polls the AP configuration
to detect changes. When the AP configuration is dynamically changed via the
SE or an SCLP instruction, the changes will not be reflected to sysfs until
the next time the AP configuration is polled. The CHSC architecture
provides a Store Event Information (SEI)command to make notification of an
AP configuration change. This patch introduces a handler to process
notification from the CHSC SEI command BY immediately kickING off an AP bus
scan.
Signed-off-by: Tony Krowiak <[email protected]>
Reviewed-by: Halil Pasic <[email protected]>
---
arch/s390/include/asm/ap.h | 12 ++++++++++++
drivers/s390/cio/chsc.c | 12 ++++++++++++
drivers/s390/cio/chsc.h | 1 +
drivers/s390/crypto/ap_bus.c | 12 ++++++++++++
4 files changed, 37 insertions(+)
diff --git a/arch/s390/include/asm/ap.h b/arch/s390/include/asm/ap.h
index 1a6a7092d942..c778593d509f 100644
--- a/arch/s390/include/asm/ap.h
+++ b/arch/s390/include/asm/ap.h
@@ -360,4 +360,16 @@ static inline struct ap_queue_status ap_dqap(ap_qid_t qid,
return reg1;
}
+/*
+ * Interface to tell the AP bus code that a configuration
+ * change has happened. The bus code should at least do
+ * an ap bus resource rescan.
+ */
+#if IS_ENABLED(CONFIG_ZCRYPT)
+void ap_bus_cfg_chg(void);
+#else
+#error "no CONFIG_ZCRYPT"
+static void ap_bus_cfg_chg(void){};
+#endif
+
#endif /* _ASM_S390_AP_H_ */
diff --git a/drivers/s390/cio/chsc.c b/drivers/s390/cio/chsc.c
index a0baee25134c..dccccc337078 100644
--- a/drivers/s390/cio/chsc.c
+++ b/drivers/s390/cio/chsc.c
@@ -586,6 +586,15 @@ static void chsc_process_sei_scm_avail(struct chsc_sei_nt0_area *sei_area)
" failed (rc=%d).\n", ret);
}
+static void chsc_process_sei_ap_cfg_chg(struct chsc_sei_nt0_area *sei_area)
+{
+ CIO_CRW_EVENT(3, "chsc: ap config changed\n");
+ if (sei_area->rs != 5)
+ return;
+
+ ap_bus_cfg_chg();
+}
+
static void chsc_process_sei_nt2(struct chsc_sei_nt2_area *sei_area)
{
switch (sei_area->cc) {
@@ -612,6 +621,9 @@ static void chsc_process_sei_nt0(struct chsc_sei_nt0_area *sei_area)
case 2: /* i/o resource accessibility */
chsc_process_sei_res_acc(sei_area);
break;
+ case 3: /* ap config changed */
+ chsc_process_sei_ap_cfg_chg(sei_area);
+ break;
case 7: /* channel-path-availability information */
chsc_process_sei_chp_avail(sei_area);
break;
diff --git a/drivers/s390/cio/chsc.h b/drivers/s390/cio/chsc.h
index 78aba8d94eec..5651066c46e3 100644
--- a/drivers/s390/cio/chsc.h
+++ b/drivers/s390/cio/chsc.h
@@ -9,6 +9,7 @@
#include <asm/chsc.h>
#include <asm/schid.h>
#include <asm/qdio.h>
+#include <asm/ap.h>
#define CHSC_SDA_OC_MSS 0x2
diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
index 48ea0004a56d..94f621783d6b 100644
--- a/drivers/s390/crypto/ap_bus.c
+++ b/drivers/s390/crypto/ap_bus.c
@@ -35,6 +35,7 @@
#include <linux/mod_devicetable.h>
#include <linux/debugfs.h>
#include <linux/ctype.h>
+#include <asm/crw.h>
#include "ap_bus.h"
#include "ap_debug.h"
@@ -860,6 +861,17 @@ void ap_bus_force_rescan(void)
EXPORT_SYMBOL(ap_bus_force_rescan);
/*
+* A config change has happened, Force an ap bus rescan.
+*/
+void ap_bus_cfg_chg(void)
+{
+ AP_DBF(DBF_INFO, "%s config change, forcing bus rescan\n", __func__);
+
+ ap_bus_force_rescan();
+}
+EXPORT_SYMBOL(ap_bus_cfg_chg);
+
+/*
* hex2bitmap() - parse hex mask string and set bitmap.
* Valid strings are "0x012345678" with at least one valid hex number.
* Rest of the bitmap to the right is padded with 0. No spaces allowed
--
2.7.4
On Wed, 30 Jan 2019, Tony Krowiak wrote:
> +#if IS_ENABLED(CONFIG_ZCRYPT)
> +void ap_bus_cfg_chg(void);
> +#else
> +#error "no CONFIG_ZCRYPT"
^
I don't think that's the right thing to do here.
> +++ b/drivers/s390/cio/chsc.h
> @@ -9,6 +9,7 @@
> #include <asm/chsc.h>
> #include <asm/schid.h>
> #include <asm/qdio.h>
> +#include <asm/ap.h>
This should be moved to chsc.c
> +++ b/drivers/s390/crypto/ap_bus.c
> @@ -35,6 +35,7 @@
> #include <linux/mod_devicetable.h>
> #include <linux/debugfs.h>
> #include <linux/ctype.h>
> +#include <asm/crw.h>
This is not needed here.
> /*
> +* A config change has happened, Force an ap bus rescan.
> +*/
> +void ap_bus_cfg_chg(void)
> +{
> + AP_DBF(DBF_INFO, "%s config change, forcing bus rescan\n", __func__);
> +
> + ap_bus_force_rescan();
> +}
> +EXPORT_SYMBOL(ap_bus_cfg_chg);
There is no need for the export symbol - you don't call that function
from module code.
As an unrelated question, just to be sure: ap_bus.c is compiled as
built-in even with ZCRYPT=m, right?
Reviewed-by: Sebastian Ott <[email protected]>
Regards,
Sebastian
On 30/01/2019 18:48, Tony Krowiak wrote:
> The current AP bus implementation periodically polls the AP configuration
> to detect changes. When the AP configuration is dynamically changed via the
> SE or an SCLP instruction, the changes will not be reflected to sysfs until
> the next time the AP configuration is polled. The CHSC architecture
> provides a Store Event Information (SEI)command to make notification of an
> AP configuration change. This patch introduces a handler to process
> notification from the CHSC SEI command BY immediately kickING off an AP bus
> scan.
>
> Signed-off-by: Tony Krowiak <[email protected]>
> Reviewed-by: Halil Pasic <[email protected]>
> ---
> arch/s390/include/asm/ap.h | 12 ++++++++++++
> drivers/s390/cio/chsc.c | 12 ++++++++++++
> drivers/s390/cio/chsc.h | 1 +
> drivers/s390/crypto/ap_bus.c | 12 ++++++++++++
> 4 files changed, 37 insertions(+)
>
> diff --git a/arch/s390/include/asm/ap.h b/arch/s390/include/asm/ap.h
> index 1a6a7092d942..c778593d509f 100644
> --- a/arch/s390/include/asm/ap.h
> +++ b/arch/s390/include/asm/ap.h
> @@ -360,4 +360,16 @@ static inline struct ap_queue_status ap_dqap(ap_qid_t qid,
> return reg1;
> }
>
> +/*
> + * Interface to tell the AP bus code that a configuration
> + * change has happened. The bus code should at least do
> + * an ap bus resource rescan.
> + */
> +#if IS_ENABLED(CONFIG_ZCRYPT)
> +void ap_bus_cfg_chg(void);
> +#else
> +#error "no CONFIG_ZCRYPT"
> +static void ap_bus_cfg_chg(void){};
> +#endif
> +
> #endif /* _ASM_S390_AP_H_ */
> diff --git a/drivers/s390/cio/chsc.c b/drivers/s390/cio/chsc.c
> index a0baee25134c..dccccc337078 100644
> --- a/drivers/s390/cio/chsc.c
> +++ b/drivers/s390/cio/chsc.c
> @@ -586,6 +586,15 @@ static void chsc_process_sei_scm_avail(struct chsc_sei_nt0_area *sei_area)
> " failed (rc=%d).\n", ret);
> }
>
> +static void chsc_process_sei_ap_cfg_chg(struct chsc_sei_nt0_area *sei_area)
> +{
> + CIO_CRW_EVENT(3, "chsc: ap config changed\n");
> + if (sei_area->rs != 5)
Why just return if the rs code is 5?
The event content code concerns the AP configuration change.
Should the source of the reporting of this change hold us from
rescanning the available AP?
Regards,
Pierre
> + return;
> +
> + ap_bus_cfg_chg();
> +}
> +
> static void chsc_process_sei_nt2(struct chsc_sei_nt2_area *sei_area)
> {
> switch (sei_area->cc) {
> @@ -612,6 +621,9 @@ static void chsc_process_sei_nt0(struct chsc_sei_nt0_area *sei_area)
> case 2: /* i/o resource accessibility */
> chsc_process_sei_res_acc(sei_area);
> break;
> + case 3: /* ap config changed */
> + chsc_process_sei_ap_cfg_chg(sei_area);
> + break;
> case 7: /* channel-path-availability information */
> chsc_process_sei_chp_avail(sei_area);
> break;
> diff --git a/drivers/s390/cio/chsc.h b/drivers/s390/cio/chsc.h
> index 78aba8d94eec..5651066c46e3 100644
> --- a/drivers/s390/cio/chsc.h
> +++ b/drivers/s390/cio/chsc.h
> @@ -9,6 +9,7 @@
> #include <asm/chsc.h>
> #include <asm/schid.h>
> #include <asm/qdio.h>
> +#include <asm/ap.h>
>
> #define CHSC_SDA_OC_MSS 0x2
>
> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
> index 48ea0004a56d..94f621783d6b 100644
> --- a/drivers/s390/crypto/ap_bus.c
> +++ b/drivers/s390/crypto/ap_bus.c
> @@ -35,6 +35,7 @@
> #include <linux/mod_devicetable.h>
> #include <linux/debugfs.h>
> #include <linux/ctype.h>
> +#include <asm/crw.h>
>
> #include "ap_bus.h"
> #include "ap_debug.h"
> @@ -860,6 +861,17 @@ void ap_bus_force_rescan(void)
> EXPORT_SYMBOL(ap_bus_force_rescan);
>
> /*
> +* A config change has happened, Force an ap bus rescan.
> +*/
> +void ap_bus_cfg_chg(void)
> +{
> + AP_DBF(DBF_INFO, "%s config change, forcing bus rescan\n", __func__);
> +
> + ap_bus_force_rescan();
> +}
> +EXPORT_SYMBOL(ap_bus_cfg_chg);
> +
> +/*
> * hex2bitmap() - parse hex mask string and set bitmap.
> * Valid strings are "0x012345678" with at least one valid hex number.
> * Rest of the bitmap to the right is padded with 0. No spaces allowed
>
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
On 30/01/2019 18:48, Tony Krowiak wrote:
> The current AP bus implementation periodically polls the AP configuration
> to detect changes. When the AP configuration is dynamically changed via the
> SE or an SCLP instruction, the changes will not be reflected to sysfs until
> the next time the AP configuration is polled. The CHSC architecture
> provides a Store Event Information (SEI)command to make notification of an
> AP configuration change. This patch introduces a handler to process
> notification from the CHSC SEI command BY immediately kickING off an AP bus
> scan.
>
> Signed-off-by: Tony Krowiak <[email protected]>
> Reviewed-by: Halil Pasic <[email protected]>
> ---
> arch/s390/include/asm/ap.h | 12 ++++++++++++
> drivers/s390/cio/chsc.c | 12 ++++++++++++
> drivers/s390/cio/chsc.h | 1 +
> drivers/s390/crypto/ap_bus.c | 12 ++++++++++++
> 4 files changed, 37 insertions(+)
>
> diff --git a/arch/s390/include/asm/ap.h b/arch/s390/include/asm/ap.h
> index 1a6a7092d942..c778593d509f 100644
> --- a/arch/s390/include/asm/ap.h
> +++ b/arch/s390/include/asm/ap.h
> @@ -360,4 +360,16 @@ static inline struct ap_queue_status ap_dqap(ap_qid_t qid,
> return reg1;
> }
>
> +/*
> + * Interface to tell the AP bus code that a configuration
> + * change has happened. The bus code should at least do
> + * an ap bus resource rescan.
> + */
> +#if IS_ENABLED(CONFIG_ZCRYPT)
> +void ap_bus_cfg_chg(void);
> +#else
> +#error "no CONFIG_ZCRYPT"
> +static void ap_bus_cfg_chg(void){};
> +#endif
> +
> #endif /* _ASM_S390_AP_H_ */
> diff --git a/drivers/s390/cio/chsc.c b/drivers/s390/cio/chsc.c
> index a0baee25134c..dccccc337078 100644
> --- a/drivers/s390/cio/chsc.c
> +++ b/drivers/s390/cio/chsc.c
> @@ -586,6 +586,15 @@ static void chsc_process_sei_scm_avail(struct chsc_sei_nt0_area *sei_area)
> " failed (rc=%d).\n", ret);
> }
>
> +static void chsc_process_sei_ap_cfg_chg(struct chsc_sei_nt0_area *sei_area)
> +{
> + CIO_CRW_EVENT(3, "chsc: ap config changed\n");
> + if (sei_area->rs != 5)
Why just return if the rs code is 5?
The event content code concerns the AP configuration change.
Should the source of the reporting of this change hold us from
rescanning the available AP?
Regards,
Pierre
> + return;
> +
> + ap_bus_cfg_chg();
> +}
> +
> static void chsc_process_sei_nt2(struct chsc_sei_nt2_area *sei_area)
> {
> switch (sei_area->cc) {
> @@ -612,6 +621,9 @@ static void chsc_process_sei_nt0(struct chsc_sei_nt0_area *sei_area)
> case 2: /* i/o resource accessibility */
> chsc_process_sei_res_acc(sei_area);
> break;
> + case 3: /* ap config changed */
> + chsc_process_sei_ap_cfg_chg(sei_area);
> + break;
> case 7: /* channel-path-availability information */
> chsc_process_sei_chp_avail(sei_area);
> break;
> diff --git a/drivers/s390/cio/chsc.h b/drivers/s390/cio/chsc.h
> index 78aba8d94eec..5651066c46e3 100644
> --- a/drivers/s390/cio/chsc.h
> +++ b/drivers/s390/cio/chsc.h
> @@ -9,6 +9,7 @@
> #include <asm/chsc.h>
> #include <asm/schid.h>
> #include <asm/qdio.h>
> +#include <asm/ap.h>
>
> #define CHSC_SDA_OC_MSS 0x2
>
> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
> index 48ea0004a56d..94f621783d6b 100644
> --- a/drivers/s390/crypto/ap_bus.c
> +++ b/drivers/s390/crypto/ap_bus.c
> @@ -35,6 +35,7 @@
> #include <linux/mod_devicetable.h>
> #include <linux/debugfs.h>
> #include <linux/ctype.h>
> +#include <asm/crw.h>
>
> #include "ap_bus.h"
> #include "ap_debug.h"
> @@ -860,6 +861,17 @@ void ap_bus_force_rescan(void)
> EXPORT_SYMBOL(ap_bus_force_rescan);
>
> /*
> +* A config change has happened, Force an ap bus rescan.
> +*/
> +void ap_bus_cfg_chg(void)
> +{
> + AP_DBF(DBF_INFO, "%s config change, forcing bus rescan\n", __func__);
> +
> + ap_bus_force_rescan();
> +}
> +EXPORT_SYMBOL(ap_bus_cfg_chg);
> +
> +/*
> * hex2bitmap() - parse hex mask string and set bitmap.
> * Valid strings are "0x012345678" with at least one valid hex number.
> * Rest of the bitmap to the right is padded with 0. No spaces allowed
>
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
On Wed, 30 Jan 2019 12:48:46 -0500
Tony Krowiak <[email protected]> wrote:
> The current AP bus implementation periodically polls the AP configuration
> to detect changes. When the AP configuration is dynamically changed via the
> SE or an SCLP instruction, the changes will not be reflected to sysfs until
> the next time the AP configuration is polled. The CHSC architecture
> provides a Store Event Information (SEI)command to make notification of an
missing blank ---------------------------^
> AP configuration change. This patch introduces a handler to process
> notification from the CHSC SEI command BY immediately kickING off an AP bus
s/BY/by/
s/kickING/kicking/
> scan.
Scan-after-event sounds useful, yeah.
Two questions:
- Does the event cover _any_ change to the AP configuration, or can the
periodic scan detect changes that are not signaled?
- Do we want to generate such an event in QEMU on plugging/unplugging
the vfio-ap device?
>
> Signed-off-by: Tony Krowiak <[email protected]>
> Reviewed-by: Halil Pasic <[email protected]>
> ---
> arch/s390/include/asm/ap.h | 12 ++++++++++++
> drivers/s390/cio/chsc.c | 12 ++++++++++++
> drivers/s390/cio/chsc.h | 1 +
> drivers/s390/crypto/ap_bus.c | 12 ++++++++++++
> 4 files changed, 37 insertions(+)
>
> diff --git a/arch/s390/include/asm/ap.h b/arch/s390/include/asm/ap.h
> index 1a6a7092d942..c778593d509f 100644
> --- a/arch/s390/include/asm/ap.h
> +++ b/arch/s390/include/asm/ap.h
> @@ -360,4 +360,16 @@ static inline struct ap_queue_status ap_dqap(ap_qid_t qid,
> return reg1;
> }
>
> +/*
> + * Interface to tell the AP bus code that a configuration
> + * change has happened. The bus code should at least do
> + * an ap bus resource rescan.
> + */
> +#if IS_ENABLED(CONFIG_ZCRYPT)
> +void ap_bus_cfg_chg(void);
> +#else
> +#error "no CONFIG_ZCRYPT"
That #error looks like a development debugging leftover.
> +static void ap_bus_cfg_chg(void){};
> +#endif
> +
> #endif /* _ASM_S390_AP_H_ */
> diff --git a/drivers/s390/cio/chsc.c b/drivers/s390/cio/chsc.c
> index a0baee25134c..dccccc337078 100644
> --- a/drivers/s390/cio/chsc.c
> +++ b/drivers/s390/cio/chsc.c
> @@ -586,6 +586,15 @@ static void chsc_process_sei_scm_avail(struct chsc_sei_nt0_area *sei_area)
> " failed (rc=%d).\n", ret);
> }
>
> +static void chsc_process_sei_ap_cfg_chg(struct chsc_sei_nt0_area *sei_area)
> +{
> + CIO_CRW_EVENT(3, "chsc: ap config changed\n");
> + if (sei_area->rs != 5)
> + return;
I'm guessing that a reporting source of 5 means ap, right? (The code is
silent on all those magic rs values :/)
If so, should the debug logging be moved after the check?
> +
> + ap_bus_cfg_chg();
> +}
> +
> static void chsc_process_sei_nt2(struct chsc_sei_nt2_area *sei_area)
> {
> switch (sei_area->cc) {
> @@ -612,6 +621,9 @@ static void chsc_process_sei_nt0(struct chsc_sei_nt0_area *sei_area)
> case 2: /* i/o resource accessibility */
> chsc_process_sei_res_acc(sei_area);
> break;
> + case 3: /* ap config changed */
> + chsc_process_sei_ap_cfg_chg(sei_area);
> + break;
> case 7: /* channel-path-availability information */
> chsc_process_sei_chp_avail(sei_area);
> break;
> diff --git a/drivers/s390/cio/chsc.h b/drivers/s390/cio/chsc.h
> index 78aba8d94eec..5651066c46e3 100644
> --- a/drivers/s390/cio/chsc.h
> +++ b/drivers/s390/cio/chsc.h
> @@ -9,6 +9,7 @@
> #include <asm/chsc.h>
> #include <asm/schid.h>
> #include <asm/qdio.h>
> +#include <asm/ap.h>
I would probably include this in chsc.c instead.
>
> #define CHSC_SDA_OC_MSS 0x2
>
> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
> index 48ea0004a56d..94f621783d6b 100644
> --- a/drivers/s390/crypto/ap_bus.c
> +++ b/drivers/s390/crypto/ap_bus.c
> @@ -35,6 +35,7 @@
> #include <linux/mod_devicetable.h>
> #include <linux/debugfs.h>
> #include <linux/ctype.h>
> +#include <asm/crw.h>
Why are you adding this #include?
>
> #include "ap_bus.h"
> #include "ap_debug.h"
> @@ -860,6 +861,17 @@ void ap_bus_force_rescan(void)
> EXPORT_SYMBOL(ap_bus_force_rescan);
>
> /*
> +* A config change has happened, Force an ap bus rescan.
s/Force/force/
> +*/
> +void ap_bus_cfg_chg(void)
> +{
> + AP_DBF(DBF_INFO, "%s config change, forcing bus rescan\n", __func__);
> +
> + ap_bus_force_rescan();
> +}
> +EXPORT_SYMBOL(ap_bus_cfg_chg);
> +
> +/*
> * hex2bitmap() - parse hex mask string and set bitmap.
> * Valid strings are "0x012345678" with at least one valid hex number.
> * Rest of the bitmap to the right is padded with 0. No spaces allowed
On 1/30/19 1:32 PM, Sebastian Ott wrote:
> On Wed, 30 Jan 2019, Tony Krowiak wrote:
>> +#if IS_ENABLED(CONFIG_ZCRYPT)
>> +void ap_bus_cfg_chg(void);
>> +#else
>> +#error "no CONFIG_ZCRYPT"
> ^
> I don't think that's the right thing to do here.
I'd like to leave it. If somebody edits .config
and sets CONFIG_ZCRYPT=n, then the build will
fail. The preprocessor error above tells them
why.
>
>
>> +++ b/drivers/s390/cio/chsc.h
>> @@ -9,6 +9,7 @@
>> #include <asm/chsc.h>
>> #include <asm/schid.h>
>> #include <asm/qdio.h>
>> +#include <asm/ap.h>
>
> This should be moved to chsc.c
I can do that, but I'm curious why.
>
>
>> +++ b/drivers/s390/crypto/ap_bus.c
>> @@ -35,6 +35,7 @@
>> #include <linux/mod_devicetable.h>
>> #include <linux/debugfs.h>
>> #include <linux/ctype.h>
>> +#include <asm/crw.h>
>
> This is not needed here.
I'll remove it.
>
>
>> /*
>> +* A config change has happened, Force an ap bus rescan.
>> +*/
>> +void ap_bus_cfg_chg(void)
>> +{
>> + AP_DBF(DBF_INFO, "%s config change, forcing bus rescan\n", __func__);
>> +
>> + ap_bus_force_rescan();
>> +}
>> +EXPORT_SYMBOL(ap_bus_cfg_chg);
>
> There is no need for the export symbol - you don't call that function
> from module code.
> As an unrelated question, just to be sure: ap_bus.c is compiled as
> built-in even with ZCRYPT=m, right?
No. If you edit .config and set CONFIG_ZCRYPT=m, ap_bus.c will be built
into the zcrypt.ko module. Through some other magic, the zcrypt module
is loaded when linux boots.
>
> Reviewed-by: Sebastian Ott <[email protected]>
>
> Regards,
> Sebastian
>
On 1/31/19 4:09 AM, Pierre Morel wrote:
> On 30/01/2019 18:48, Tony Krowiak wrote:
>> The current AP bus implementation periodically polls the AP configuration
>> to detect changes. When the AP configuration is dynamically changed
>> via the
>> SE or an SCLP instruction, the changes will not be reflected to sysfs
>> until
>> the next time the AP configuration is polled. The CHSC architecture
>> provides a Store Event Information (SEI)command to make notification
>> of an
>> AP configuration change. This patch introduces a handler to process
>> notification from the CHSC SEI command BY immediately kickING off an
>> AP bus
>> scan.
>>
>> Signed-off-by: Tony Krowiak <[email protected]>
>> Reviewed-by: Halil Pasic <[email protected]>
>> ---
>> arch/s390/include/asm/ap.h | 12 ++++++++++++
>> drivers/s390/cio/chsc.c | 12 ++++++++++++
>> drivers/s390/cio/chsc.h | 1 +
>> drivers/s390/crypto/ap_bus.c | 12 ++++++++++++
>> 4 files changed, 37 insertions(+)
>>
>> diff --git a/arch/s390/include/asm/ap.h b/arch/s390/include/asm/ap.h
>> index 1a6a7092d942..c778593d509f 100644
>> --- a/arch/s390/include/asm/ap.h
>> +++ b/arch/s390/include/asm/ap.h
>> @@ -360,4 +360,16 @@ static inline struct ap_queue_status
>> ap_dqap(ap_qid_t qid,
>> return reg1;
>> }
>> +/*
>> + * Interface to tell the AP bus code that a configuration
>> + * change has happened. The bus code should at least do
>> + * an ap bus resource rescan.
>> + */
>> +#if IS_ENABLED(CONFIG_ZCRYPT)
>> +void ap_bus_cfg_chg(void);
>> +#else
>> +#error "no CONFIG_ZCRYPT"
>> +static void ap_bus_cfg_chg(void){};
>> +#endif
>> +
>> #endif /* _ASM_S390_AP_H_ */
>> diff --git a/drivers/s390/cio/chsc.c b/drivers/s390/cio/chsc.c
>> index a0baee25134c..dccccc337078 100644
>> --- a/drivers/s390/cio/chsc.c
>> +++ b/drivers/s390/cio/chsc.c
>> @@ -586,6 +586,15 @@ static void chsc_process_sei_scm_avail(struct
>> chsc_sei_nt0_area *sei_area)
>> " failed (rc=%d).\n", ret);
>> }
>> +static void chsc_process_sei_ap_cfg_chg(struct chsc_sei_nt0_area
>> *sei_area)
>> +{
>> + CIO_CRW_EVENT(3, "chsc: ap config changed\n");
>> + if (sei_area->rs != 5)
>
> Why just return if the rs code is 5?
>
> The event content code concerns the AP configuration change.
>
> Should the source of the reporting of this change hold us from
> rescanning the available AP?
Why rescan if the RS code does not indicate the AP configuration
changed?
>
>
> Regards,
> Pierre
>
>
>> + return;
>> +
>> + ap_bus_cfg_chg();
>> +}
>> +
>> static void chsc_process_sei_nt2(struct chsc_sei_nt2_area *sei_area)
>> {
>> switch (sei_area->cc) {
>> @@ -612,6 +621,9 @@ static void chsc_process_sei_nt0(struct
>> chsc_sei_nt0_area *sei_area)
>> case 2: /* i/o resource accessibility */
>> chsc_process_sei_res_acc(sei_area);
>> break;
>> + case 3: /* ap config changed */
>> + chsc_process_sei_ap_cfg_chg(sei_area);
>> + break;
>> case 7: /* channel-path-availability information */
>> chsc_process_sei_chp_avail(sei_area);
>> break;
>> diff --git a/drivers/s390/cio/chsc.h b/drivers/s390/cio/chsc.h
>> index 78aba8d94eec..5651066c46e3 100644
>> --- a/drivers/s390/cio/chsc.h
>> +++ b/drivers/s390/cio/chsc.h
>> @@ -9,6 +9,7 @@
>> #include <asm/chsc.h>
>> #include <asm/schid.h>
>> #include <asm/qdio.h>
>> +#include <asm/ap.h>
>> #define CHSC_SDA_OC_MSS 0x2
>> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
>> index 48ea0004a56d..94f621783d6b 100644
>> --- a/drivers/s390/crypto/ap_bus.c
>> +++ b/drivers/s390/crypto/ap_bus.c
>> @@ -35,6 +35,7 @@
>> #include <linux/mod_devicetable.h>
>> #include <linux/debugfs.h>
>> #include <linux/ctype.h>
>> +#include <asm/crw.h>
>> #include "ap_bus.h"
>> #include "ap_debug.h"
>> @@ -860,6 +861,17 @@ void ap_bus_force_rescan(void)
>> EXPORT_SYMBOL(ap_bus_force_rescan);
>> /*
>> +* A config change has happened, Force an ap bus rescan.
>> +*/
>> +void ap_bus_cfg_chg(void)
>> +{
>> + AP_DBF(DBF_INFO, "%s config change, forcing bus rescan\n",
>> __func__);
>> +
>> + ap_bus_force_rescan();
>> +}
>> +EXPORT_SYMBOL(ap_bus_cfg_chg);
>> +
>> +/*
>> * hex2bitmap() - parse hex mask string and set bitmap.
>> * Valid strings are "0x012345678" with at least one valid hex number.
>> * Rest of the bitmap to the right is padded with 0. No spaces allowed
>>
>
>
On 1/31/19 4:55 AM, Cornelia Huck wrote:
> On Wed, 30 Jan 2019 12:48:46 -0500
> Tony Krowiak <[email protected]> wrote:
>
>> The current AP bus implementation periodically polls the AP configuration
>> to detect changes. When the AP configuration is dynamically changed via the
>> SE or an SCLP instruction, the changes will not be reflected to sysfs until
>> the next time the AP configuration is polled. The CHSC architecture
>> provides a Store Event Information (SEI)command to make notification of an
>
> missing blank ---------------------------^
I'll insert it.
>
>> AP configuration change. This patch introduces a handler to process
>> notification from the CHSC SEI command BY immediately kickING off an AP bus
>
> s/BY/by/
>
> s/kickING/kicking/
Will fix both.
>
>> scan.
>
> Scan-after-event sounds useful, yeah.
sure
>
> Two questions:
> - Does the event cover _any_ change to the AP configuration, or can the
> periodic scan detect changes that are not signaled?
It can detect any change, such as a change to the CRYCB masks.
> - Do we want to generate such an event in QEMU on plugging/unplugging
> the vfio-ap device?
We've discussed this quite a bit internally and decided not to implement
that at this time. We will address it as a future enhancement.
>
>>
>> Signed-off-by: Tony Krowiak <[email protected]>
>> Reviewed-by: Halil Pasic <[email protected]>
>> ---
>> arch/s390/include/asm/ap.h | 12 ++++++++++++
>> drivers/s390/cio/chsc.c | 12 ++++++++++++
>> drivers/s390/cio/chsc.h | 1 +
>> drivers/s390/crypto/ap_bus.c | 12 ++++++++++++
>> 4 files changed, 37 insertions(+)
>>
>> diff --git a/arch/s390/include/asm/ap.h b/arch/s390/include/asm/ap.h
>> index 1a6a7092d942..c778593d509f 100644
>> --- a/arch/s390/include/asm/ap.h
>> +++ b/arch/s390/include/asm/ap.h
>> @@ -360,4 +360,16 @@ static inline struct ap_queue_status ap_dqap(ap_qid_t qid,
>> return reg1;
>> }
>>
>> +/*
>> + * Interface to tell the AP bus code that a configuration
>> + * change has happened. The bus code should at least do
>> + * an ap bus resource rescan.
>> + */
>> +#if IS_ENABLED(CONFIG_ZCRYPT)
>> +void ap_bus_cfg_chg(void);
>> +#else
>> +#error "no CONFIG_ZCRYPT"
>
> That #error looks like a development debugging leftover.
>
>> +static void ap_bus_cfg_chg(void){};
>> +#endif
>> +
>> #endif /* _ASM_S390_AP_H_ */
>> diff --git a/drivers/s390/cio/chsc.c b/drivers/s390/cio/chsc.c
>> index a0baee25134c..dccccc337078 100644
>> --- a/drivers/s390/cio/chsc.c
>> +++ b/drivers/s390/cio/chsc.c
>> @@ -586,6 +586,15 @@ static void chsc_process_sei_scm_avail(struct chsc_sei_nt0_area *sei_area)
>> " failed (rc=%d).\n", ret);
>> }
>>
>> +static void chsc_process_sei_ap_cfg_chg(struct chsc_sei_nt0_area *sei_area)
>> +{
>> + CIO_CRW_EVENT(3, "chsc: ap config changed\n");
>> + if (sei_area->rs != 5)
>> + return;
>
> I'm guessing that a reporting source of 5 means ap, right? (The code is
> silent on all those magic rs values :/)
The 5 indicates the accessibility of one or more adjunct processors has
changed. The reason this gets called is because the CC sent with the
instruction indicates the AP configuration has changed, so the reporting
belongs where it is. There is only one RS associated with it.
>
> If so, should the debug logging be moved after the check?
covered in the response above.
>
>> +
>> + ap_bus_cfg_chg();
>> +}
>> +
>> static void chsc_process_sei_nt2(struct chsc_sei_nt2_area *sei_area)
>> {
>> switch (sei_area->cc) {
>> @@ -612,6 +621,9 @@ static void chsc_process_sei_nt0(struct chsc_sei_nt0_area *sei_area)
>> case 2: /* i/o resource accessibility */
>> chsc_process_sei_res_acc(sei_area);
>> break;
>> + case 3: /* ap config changed */
>> + chsc_process_sei_ap_cfg_chg(sei_area);
>> + break;
>> case 7: /* channel-path-availability information */
>> chsc_process_sei_chp_avail(sei_area);
>> break;
>> diff --git a/drivers/s390/cio/chsc.h b/drivers/s390/cio/chsc.h
>> index 78aba8d94eec..5651066c46e3 100644
>> --- a/drivers/s390/cio/chsc.h
>> +++ b/drivers/s390/cio/chsc.h
>> @@ -9,6 +9,7 @@
>> #include <asm/chsc.h>
>> #include <asm/schid.h>
>> #include <asm/qdio.h>
>> +#include <asm/ap.h>
>
> I would probably include this in chsc.c instead.
Already done per Sebastian's comment.
>
>>
>> #define CHSC_SDA_OC_MSS 0x2
>>
>> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
>> index 48ea0004a56d..94f621783d6b 100644
>> --- a/drivers/s390/crypto/ap_bus.c
>> +++ b/drivers/s390/crypto/ap_bus.c
>> @@ -35,6 +35,7 @@
>> #include <linux/mod_devicetable.h>
>> #include <linux/debugfs.h>
>> #include <linux/ctype.h>
>> +#include <asm/crw.h>
>
> Why are you adding this #include?
Already removed per Sebastian's comment.
>
>>
>> #include "ap_bus.h"
>> #include "ap_debug.h"
>> @@ -860,6 +861,17 @@ void ap_bus_force_rescan(void)
>> EXPORT_SYMBOL(ap_bus_force_rescan);
>>
>> /*
>> +* A config change has happened, Force an ap bus rescan.
>
> s/Force/force/
Will do.
>
>> +*/
>> +void ap_bus_cfg_chg(void)
>> +{
>> + AP_DBF(DBF_INFO, "%s config change, forcing bus rescan\n", __func__);
>> +
>> + ap_bus_force_rescan();
>> +}
>> +EXPORT_SYMBOL(ap_bus_cfg_chg);
>> +
>> +/*
>> * hex2bitmap() - parse hex mask string and set bitmap.
>> * Valid strings are "0x012345678" with at least one valid hex number.
>> * Rest of the bitmap to the right is padded with 0. No spaces allowed
>
On Thu, Jan 31, 2019 at 06:28:39PM -0500, Tony Krowiak wrote:
> On 1/30/19 1:32 PM, Sebastian Ott wrote:
> >On Wed, 30 Jan 2019, Tony Krowiak wrote:
> >>+#if IS_ENABLED(CONFIG_ZCRYPT)
> >>+void ap_bus_cfg_chg(void);
> >>+#else
> >>+#error "no CONFIG_ZCRYPT"
> > ^
> >I don't think that's the right thing to do here.
>
> I'd like to leave it. If somebody edits .config
> and sets CONFIG_ZCRYPT=n, then the build will
> fail. The preprocessor error above tells them
> why.
No, the kernel build should never fail if a config option is not set.
Also the above should be "#ifdef CONFIG_ZCRYPT".
In addition (this isn't quoted unfortunately) the alternative function
in the header file is missing the "inline" attribute. Can you please
add that too?
static inline void ap_bus_cfg_chg(void) { }
> >>+* A config change has happened, Force an ap bus rescan.
> >>+*/
> >>+void ap_bus_cfg_chg(void)
> >>+{
> >>+ AP_DBF(DBF_INFO, "%s config change, forcing bus rescan\n", __func__);
> >>+
> >>+ ap_bus_force_rescan();
> >>+}
> >>+EXPORT_SYMBOL(ap_bus_cfg_chg);
> >
> >There is no need for the export symbol - you don't call that function
> >from module code.
> >As an unrelated question, just to be sure: ap_bus.c is compiled as
> >built-in even with ZCRYPT=m, right?
>
> No. If you edit .config and set CONFIG_ZCRYPT=m, ap_bus.c will be built
> into the zcrypt.ko module. Through some other magic, the zcrypt module
> is loaded when linux boots.
If that happens, then we have a build problem that needs to be
fixed. What exactly are you doing to get the ap code linked into the
zcrypt module?
On Fri, 1 Feb 2019 10:01:59 +0100
Heiko Carstens <[email protected]> wrote:
> On Thu, Jan 31, 2019 at 06:28:39PM -0500, Tony Krowiak wrote:
> > On 1/30/19 1:32 PM, Sebastian Ott wrote:
> > >On Wed, 30 Jan 2019, Tony Krowiak wrote:
> > >>+#if IS_ENABLED(CONFIG_ZCRYPT)
> > >>+void ap_bus_cfg_chg(void);
> > >>+#else
> > >>+#error "no CONFIG_ZCRYPT"
> > > ^
> > >I don't think that's the right thing to do here.
> >
> > I'd like to leave it. If somebody edits .config
> > and sets CONFIG_ZCRYPT=n, then the build will
> > fail. The preprocessor error above tells them
> > why.
>
> No, the kernel build should never fail if a config option is not set.
> Also the above should be "#ifdef CONFIG_ZCRYPT".
>
> In addition (this isn't quoted unfortunately) the alternative function
> in the header file is missing the "inline" attribute. Can you please
> add that too?
>
> static inline void ap_bus_cfg_chg(void) { }
>
> > >>+* A config change has happened, Force an ap bus rescan.
> > >>+*/
> > >>+void ap_bus_cfg_chg(void)
> > >>+{
> > >>+ AP_DBF(DBF_INFO, "%s config change, forcing bus rescan\n", __func__);
> > >>+
> > >>+ ap_bus_force_rescan();
> > >>+}
> > >>+EXPORT_SYMBOL(ap_bus_cfg_chg);
> > >
> > >There is no need for the export symbol - you don't call that function
> > >from module code.
> > >As an unrelated question, just to be sure: ap_bus.c is compiled as
> > >built-in even with ZCRYPT=m, right?
> >
> > No. If you edit .config and set CONFIG_ZCRYPT=m, ap_bus.c will be built
> > into the zcrypt.ko module. Through some other magic, the zcrypt module
> > is loaded when linux boots.
>
> If that happens, then we have a build problem that needs to be
> fixed. What exactly are you doing to get the ap code linked into the
> zcrypt module?
Current upstream code:
ap-objs := ap_bus.o ap_card.o ap_queue.o
obj-$(subst m,y,$(CONFIG_ZCRYPT)) += ap.o
The ap_bus.o file is either not build at all or it is linked into the
main kernel image. If ap_bus.o is build then it is guaranteed that
CONFIG_ZCRYPT is either "m" or "y".
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Fri, Feb 01, 2019 at 10:01:59AM +0100, Heiko Carstens wrote:
> On Thu, Jan 31, 2019 at 06:28:39PM -0500, Tony Krowiak wrote:
> > On 1/30/19 1:32 PM, Sebastian Ott wrote:
> > >On Wed, 30 Jan 2019, Tony Krowiak wrote:
> > >>+#if IS_ENABLED(CONFIG_ZCRYPT)
> > >>+void ap_bus_cfg_chg(void);
...
> Also the above should be "#ifdef CONFIG_ZCRYPT".
Please ignore that part of my comment.
I always forget the CONFIG_ZCRYPT_MODULE option.
On Thu, 31 Jan 2019 18:50:57 -0500
Tony Krowiak <[email protected]> wrote:
> On 1/31/19 4:55 AM, Cornelia Huck wrote:
> > On Wed, 30 Jan 2019 12:48:46 -0500
> > Tony Krowiak <[email protected]> wrote:
> > Two questions:
> > - Does the event cover _any_ change to the AP configuration, or can the
> > periodic scan detect changes that are not signaled?
>
> It can detect any change, such as a change to the CRYCB masks.
Nice. I suppose we can not rely on those messages being generated,
though, and therefore need to keep the periodic scan...
>
> > - Do we want to generate such an event in QEMU on plugging/unplugging
> > the vfio-ap device?
>
> We've discussed this quite a bit internally and decided not to implement
> that at this time. We will address it as a future enhancement.
Ok, but I think it would be nice to have.
> >> diff --git a/drivers/s390/cio/chsc.c b/drivers/s390/cio/chsc.c
> >> index a0baee25134c..dccccc337078 100644
> >> --- a/drivers/s390/cio/chsc.c
> >> +++ b/drivers/s390/cio/chsc.c
> >> @@ -586,6 +586,15 @@ static void chsc_process_sei_scm_avail(struct chsc_sei_nt0_area *sei_area)
> >> " failed (rc=%d).\n", ret);
> >> }
> >>
> >> +static void chsc_process_sei_ap_cfg_chg(struct chsc_sei_nt0_area *sei_area)
> >> +{
> >> + CIO_CRW_EVENT(3, "chsc: ap config changed\n");
> >> + if (sei_area->rs != 5)
> >> + return;
> >
> > I'm guessing that a reporting source of 5 means ap, right? (The code is
> > silent on all those magic rs values :/)
>
> The 5 indicates the accessibility of one or more adjunct processors has
> changed. The reason this gets called is because the CC sent with the
> instruction indicates the AP configuration has changed, so the reporting
> belongs where it is. There is only one RS associated with it.
So if we'd ever get there anything but rs == 5, it would be a hardware
or hypervisor bug? Then the code makes sense, I guess.
>
> >
> > If so, should the debug logging be moved after the check?
>
> covered in the response above.
>
> >
> >> +
> >> + ap_bus_cfg_chg();
> >> +}
> >> +
On 2/1/19 8:05 AM, Heiko Carstens wrote:
> On Fri, Feb 01, 2019 at 10:01:59AM +0100, Heiko Carstens wrote:
>> On Thu, Jan 31, 2019 at 06:28:39PM -0500, Tony Krowiak wrote:
>>> On 1/30/19 1:32 PM, Sebastian Ott wrote:
>>>> On Wed, 30 Jan 2019, Tony Krowiak wrote:
>>>>> +#if IS_ENABLED(CONFIG_ZCRYPT)
>>>>> +void ap_bus_cfg_chg(void);
> ...
>> Also the above should be "#ifdef CONFIG_ZCRYPT".
Okay
>
> Please ignore that part of my comment.
> I always forget the CONFIG_ZCRYPT_MODULE option.
>
On 2/1/19 4:01 AM, Heiko Carstens wrote:
> On Thu, Jan 31, 2019 at 06:28:39PM -0500, Tony Krowiak wrote:
>> On 1/30/19 1:32 PM, Sebastian Ott wrote:
>>> On Wed, 30 Jan 2019, Tony Krowiak wrote:
>>>> +#if IS_ENABLED(CONFIG_ZCRYPT)
>>>> +void ap_bus_cfg_chg(void);
>>>> +#else
>>>> +#error "no CONFIG_ZCRYPT"
>>> ^
>>> I don't think that's the right thing to do here.
>>
>> I'd like to leave it. If somebody edits .config
>> and sets CONFIG_ZCRYPT=n, then the build will
>> fail. The preprocessor error above tells them
>> why.
>
> No, the kernel build should never fail if a config option is not set.
> Also the above should be "#ifdef CONFIG_ZCRYPT".
Will do.
>
> In addition (this isn't quoted unfortunately) the alternative function
> in the header file is missing the "inline" attribute. Can you please
> add that too?
I can.
>
> static inline void ap_bus_cfg_chg(void) { }
>
>>>> +* A config change has happened, Force an ap bus rescan.
>>>> +*/
>>>> +void ap_bus_cfg_chg(void)
>>>> +{
>>>> + AP_DBF(DBF_INFO, "%s config change, forcing bus rescan\n", __func__);
>>>> +
>>>> + ap_bus_force_rescan();
>>>> +}
>>>> +EXPORT_SYMBOL(ap_bus_cfg_chg);
>>>
>>> There is no need for the export symbol - you don't call that function
>> >from module code.
>>> As an unrelated question, just to be sure: ap_bus.c is compiled as
>>> built-in even with ZCRYPT=m, right?
>>
>> No. If you edit .config and set CONFIG_ZCRYPT=m, ap_bus.c will be built
>> into the zcrypt.ko module. Through some other magic, the zcrypt module
>> is loaded when linux boots.
>
> If that happens, then we have a build problem that needs to be
> fixed. What exactly are you doing to get the ap code linked into the
> zcrypt module?
To tell you the truth, I don't know. The build configuration precedes my
creating this patch by many years. I only discovered these anomalies by
playing with the CONFIG_ZCRYPT option in response to Sebastian's
comments. I will have to take this up with our internal IBM maintainer.
>
On 2/1/19 9:35 AM, Cornelia Huck wrote:
> On Thu, 31 Jan 2019 18:50:57 -0500
> Tony Krowiak <[email protected]> wrote:
>
>> On 1/31/19 4:55 AM, Cornelia Huck wrote:
>>> On Wed, 30 Jan 2019 12:48:46 -0500
>>> Tony Krowiak <[email protected]> wrote:
>
>>> Two questions:
>>> - Does the event cover _any_ change to the AP configuration, or can the
>>> periodic scan detect changes that are not signaled?
>>
>> It can detect any change, such as a change to the CRYCB masks.
>
> Nice. I suppose we can not rely on those messages being generated,
> though, and therefore need to keep the periodic scan...
I don't know how the CRYCB can be changed dynamically on the host, but
hot plug for a guest changes it dynamically. Down the road, we may
send a CHSC SEI AP Configuration event to let the guest know. I don't
know if there may be other AP config changes that can occur without this
event being posted, so it is probably a good idea to keep the scan. It
certainly doesn't hurt anything to do so.
>
>>
>>> - Do we want to generate such an event in QEMU on plugging/unplugging
>>> the vfio-ap device?
>>
>> We've discussed this quite a bit internally and decided not to implement
>> that at this time. We will address it as a future enhancement.
>
> Ok, but I think it would be nice to have.
Duly noted, but that discussion is outside of scope for this patch.
>
>>>> diff --git a/drivers/s390/cio/chsc.c b/drivers/s390/cio/chsc.c
>>>> index a0baee25134c..dccccc337078 100644
>>>> --- a/drivers/s390/cio/chsc.c
>>>> +++ b/drivers/s390/cio/chsc.c
>>>> @@ -586,6 +586,15 @@ static void chsc_process_sei_scm_avail(struct chsc_sei_nt0_area *sei_area)
>>>> " failed (rc=%d).\n", ret);
>>>> }
>>>>
>>>> +static void chsc_process_sei_ap_cfg_chg(struct chsc_sei_nt0_area *sei_area)
>>>> +{
>>>> + CIO_CRW_EVENT(3, "chsc: ap config changed\n");
>>>> + if (sei_area->rs != 5)
>>>> + return;
>>>
>>> I'm guessing that a reporting source of 5 means ap, right? (The code is
>>> silent on all those magic rs values :/)
>>
>> The 5 indicates the accessibility of one or more adjunct processors has
>> changed. The reason this gets called is because the CC sent with the
>> instruction indicates the AP configuration has changed, so the reporting
>> belongs where it is. There is only one RS associated with it.
>
> So if we'd ever get there anything but rs == 5, it would be a hardware
> or hypervisor bug? Then the code makes sense, I guess.
I have no idea if that is possible, but this follows the architecture.
>
>>
>>>
>>> If so, should the debug logging be moved after the check?
>>
>> covered in the response above.
>>
>>>
>>>> +
>>>> + ap_bus_cfg_chg();
>>>> +}
>>>> +
>
Hi Tony,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.0-rc4 next-20190201]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Tony-Krowiak/zcrypt-handle-AP-Info-notification-from-CHSC-SEI-command/20190202-184856
config: s390-alldefconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.2.0 make.cross ARCH=s390
All errors (new ones prefixed by >>):
In file included from arch/s390/kvm/kvm-s390.c:46:
>> arch/s390/include/asm/ap.h:371:2: error: #error "no CONFIG_ZCRYPT"
#error "no CONFIG_ZCRYPT"
^~~~~
In file included from arch/s390/kvm/kvm-s390.c:46:
arch/s390/include/asm/ap.h:372:13: warning: 'ap_bus_cfg_chg' defined but not used [-Wunused-function]
static void ap_bus_cfg_chg(void){};
^~~~~~~~~~~~~~
vim +371 arch/s390/include/asm/ap.h
362
363 /*
364 * Interface to tell the AP bus code that a configuration
365 * change has happened. The bus code should at least do
366 * an ap bus resource rescan.
367 */
368 #if IS_ENABLED(CONFIG_ZCRYPT)
369 void ap_bus_cfg_chg(void);
370 #else
> 371 #error "no CONFIG_ZCRYPT"
372 static void ap_bus_cfg_chg(void){};
373 #endif
374
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Thu, 31 Jan 2019, Tony Krowiak wrote:
> On 1/30/19 1:32 PM, Sebastian Ott wrote:
> > On Wed, 30 Jan 2019, Tony Krowiak wrote:
> >> +++ b/drivers/s390/cio/chsc.h
> >> @@ -9,6 +9,7 @@
> >> #include <asm/chsc.h>
> >> #include <asm/schid.h>
> >> #include <asm/qdio.h>
> >> +#include <asm/ap.h>
> >
> > This should be moved to chsc.c
>
> I can do that, but I'm curious why.
You only need to include ap.h because of the ap_bus_cfg_chg definition and
you only use that in chsc.c . Other users of chsc.h don't call that
function.
Regards,
Sebastian
On 01.02.19 16:38, Tony Krowiak wrote:
> On 2/1/19 4:01 AM, Heiko Carstens wrote:
>> On Thu, Jan 31, 2019 at 06:28:39PM -0500, Tony Krowiak wrote:
>>> On 1/30/19 1:32 PM, Sebastian Ott wrote:
>>>> On Wed, 30 Jan 2019, Tony Krowiak wrote:
>>>>> +#if IS_ENABLED(CONFIG_ZCRYPT)
>>>>> +void ap_bus_cfg_chg(void);
>>>>> +#else
>>>>> +#error "no CONFIG_ZCRYPT"
>>>> ^
>>>> I don't think that's the right thing to do here.
>>>
>>> I'd like to leave it. If somebody edits .config
>>> and sets CONFIG_ZCRYPT=n, then the build will
>>> fail. The preprocessor error above tells them
>>> why.
>>
>> No, the kernel build should never fail if a config option is not set.
>> Also the above should be "#ifdef CONFIG_ZCRYPT".
>
> Will do.
>
>>
>> In addition (this isn't quoted unfortunately) the alternative function
>> in the header file is missing the "inline" attribute. Can you please
>> add that too?
>
> I can.
>
>>
>> static inline void ap_bus_cfg_chg(void) { }
>>
>>>>> +* A config change has happened, Force an ap bus rescan.
>>>>> +*/
>>>>> +void ap_bus_cfg_chg(void)
>>>>> +{
>>>>> + AP_DBF(DBF_INFO, "%s config change, forcing bus rescan\n", __func__);
>>>>> +
>>>>> + ap_bus_force_rescan();
>>>>> +}
>>>>> +EXPORT_SYMBOL(ap_bus_cfg_chg);
>>>>
>>>> There is no need for the export symbol - you don't call that function
>>> >from module code.
>>>> As an unrelated question, just to be sure: ap_bus.c is compiled as
>>>> built-in even with ZCRYPT=m, right?
>>>
>>> No. If you edit .config and set CONFIG_ZCRYPT=m, ap_bus.c will be built
>>> into the zcrypt.ko module. Through some other magic, the zcrypt module
>>> is loaded when linux boots.
>>
>> If that happens, then we have a build problem that needs to be
>> fixed. What exactly are you doing to get the ap code linked into the
>> zcrypt module?
>
> To tell you the truth, I don't know. The build configuration precedes my
> creating this patch by many years. I only discovered these anomalies by
> playing with the CONFIG_ZCRYPT option in response to Sebastian's
> comments. I will have to take this up with our internal IBM maintainer.
>
>>
>
As Martin already pointed out the ap bus code is statically build into the kernel
with this line in the Makefile:
obj-$(subst m,y,$(CONFIG_ZCRYPT)) += ap.o
So CONFIG_ZCRYPT may have the value 'm' or 'y'
and this is also the reason why a simple #ifdef CONFIG_ZCRYPT does not
work, instead #if IS_ENABLED(CONFIG_ZCRYPT) has to be used.
Only the ap stuff (which is: ap_bus.o, ap_card.o and ap_queue.o) is statically
build into the kernel. The zcrypt kernel module is loaded as a dependency
when a crypto card is detected. So detecting a CEX4 forces kernel module
zcrypt_cex4.ko which forces the zcrypt.ko to insert into the kernel.
So all global functions in ap_bus.c are available when CONFIG_ZCRYPT is
enabled. However, I thought the symbol still needs an EXPORT_SYMBOL()
statement. Otherwise I should throw out all these statements.
Harald
On 01.02.19 15:35, Cornelia Huck wrote:
> On Thu, 31 Jan 2019 18:50:57 -0500
> Tony Krowiak <[email protected]> wrote:
>
>> On 1/31/19 4:55 AM, Cornelia Huck wrote:
>>> On Wed, 30 Jan 2019 12:48:46 -0500
>>> Tony Krowiak <[email protected]> wrote:
>>> Two questions:
>>> - Does the event cover _any_ change to the AP configuration, or can the
>>> periodic scan detect changes that are not signaled?
>> It can detect any change, such as a change to the CRYCB masks.
> Nice. I suppose we can not rely on those messages being generated,
> though, and therefore need to keep the periodic scan...
As you wrote, I am not sure if the ap bus code can rely on this to
cover all changes. For kvm guests I think it is currently not working
as there is no such notification generated at all. So I'd like to
have the periodic scan in place.
>
>>> - Do we want to generate such an event in QEMU on plugging/unplugging
>>> the vfio-ap device?
>> We've discussed this quite a bit internally and decided not to implement
>> that at this time. We will address it as a future enhancement.
> Ok, but I think it would be nice to have.
>
>>>> diff --git a/drivers/s390/cio/chsc.c b/drivers/s390/cio/chsc.c
>>>> index a0baee25134c..dccccc337078 100644
>>>> --- a/drivers/s390/cio/chsc.c
>>>> +++ b/drivers/s390/cio/chsc.c
>>>> @@ -586,6 +586,15 @@ static void chsc_process_sei_scm_avail(struct chsc_sei_nt0_area *sei_area)
>>>> " failed (rc=%d).\n", ret);
>>>> }
>>>>
>>>> +static void chsc_process_sei_ap_cfg_chg(struct chsc_sei_nt0_area *sei_area)
>>>> +{
>>>> + CIO_CRW_EVENT(3, "chsc: ap config changed\n");
>>>> + if (sei_area->rs != 5)
>>>> + return;
>>> I'm guessing that a reporting source of 5 means ap, right? (The code is
>>> silent on all those magic rs values :/)
>> The 5 indicates the accessibility of one or more adjunct processors has
>> changed. The reason this gets called is because the CC sent with the
>> instruction indicates the AP configuration has changed, so the reporting
>> belongs where it is. There is only one RS associated with it.
> So if we'd ever get there anything but rs == 5, it would be a hardware
> or hypervisor bug? Then the code makes sense, I guess.
>
>>> If so, should the debug logging be moved after the check?
>> covered in the response above.
>>
>>>
>>>> +
>>>> + ap_bus_cfg_chg();
>>>> +}
>>>> +
On Mon, 4 Feb 2019 11:15:25 +0100
Harald Freudenberger <[email protected]> wrote:
> On 01.02.19 15:35, Cornelia Huck wrote:
> > On Thu, 31 Jan 2019 18:50:57 -0500
> > Tony Krowiak <[email protected]> wrote:
> >
> >> On 1/31/19 4:55 AM, Cornelia Huck wrote:
> >>> On Wed, 30 Jan 2019 12:48:46 -0500
> >>> Tony Krowiak <[email protected]> wrote:
> >>> Two questions:
> >>> - Does the event cover _any_ change to the AP configuration, or can the
> >>> periodic scan detect changes that are not signaled?
> >> It can detect any change, such as a change to the CRYCB masks.
> > Nice. I suppose we can not rely on those messages being generated,
> > though, and therefore need to keep the periodic scan...
> As you wrote, I am not sure if the ap bus code can rely on this to
> cover all changes. For kvm guests I think it is currently not working
> as there is no such notification generated at all. So I'd like to
> have the periodic scan in place.
Yes, and there are bound to be QEMU versions without that support in
active use even if this is implemented in the future. But a quicker
reaction to changes on real hardware still sounds like a win to me :)
On 2/4/19 5:06 AM, Harald Freudenberger wrote:
> On 01.02.19 16:38, Tony Krowiak wrote:
>> On 2/1/19 4:01 AM, Heiko Carstens wrote:
>>> On Thu, Jan 31, 2019 at 06:28:39PM -0500, Tony Krowiak wrote:
>>>> On 1/30/19 1:32 PM, Sebastian Ott wrote:
>>>>> On Wed, 30 Jan 2019, Tony Krowiak wrote:
>>>>>> +#if IS_ENABLED(CONFIG_ZCRYPT)
>>>>>> +void ap_bus_cfg_chg(void);
>>>>>> +#else
>>>>>> +#error "no CONFIG_ZCRYPT"
>>>>> ^
>>>>> I don't think that's the right thing to do here.
>>>>
>>>> I'd like to leave it. If somebody edits .config
>>>> and sets CONFIG_ZCRYPT=n, then the build will
>>>> fail. The preprocessor error above tells them
>>>> why.
>>>
>>> No, the kernel build should never fail if a config option is not set.
>>> Also the above should be "#ifdef CONFIG_ZCRYPT".
>>
>> Will do.
>>
>>>
>>> In addition (this isn't quoted unfortunately) the alternative function
>>> in the header file is missing the "inline" attribute. Can you please
>>> add that too?
>>
>> I can.
>>
>>>
>>> static inline void ap_bus_cfg_chg(void) { }
>>>
>>>>>> +* A config change has happened, Force an ap bus rescan.
>>>>>> +*/
>>>>>> +void ap_bus_cfg_chg(void)
>>>>>> +{
>>>>>> + AP_DBF(DBF_INFO, "%s config change, forcing bus rescan\n", __func__);
>>>>>> +
>>>>>> + ap_bus_force_rescan();
>>>>>> +}
>>>>>> +EXPORT_SYMBOL(ap_bus_cfg_chg);
>>>>>
>>>>> There is no need for the export symbol - you don't call that function
>>>> >from module code.
>>>>> As an unrelated question, just to be sure: ap_bus.c is compiled as
>>>>> built-in even with ZCRYPT=m, right?
>>>>
>>>> No. If you edit .config and set CONFIG_ZCRYPT=m, ap_bus.c will be built
>>>> into the zcrypt.ko module. Through some other magic, the zcrypt module
>>>> is loaded when linux boots.
>>>
>>> If that happens, then we have a build problem that needs to be
>>> fixed. What exactly are you doing to get the ap code linked into the
>>> zcrypt module?
>>
>> To tell you the truth, I don't know. The build configuration precedes my
>> creating this patch by many years. I only discovered these anomalies by
>> playing with the CONFIG_ZCRYPT option in response to Sebastian's
>> comments. I will have to take this up with our internal IBM maintainer.
>>
>>>
>>
> As Martin already pointed out the ap bus code is statically build into the kernel
> with this line in the Makefile:
> obj-$(subst m,y,$(CONFIG_ZCRYPT)) += ap.o
> So CONFIG_ZCRYPT may have the value 'm' or 'y'
> and this is also the reason why a simple #ifdef CONFIG_ZCRYPT does not
> work, instead #if IS_ENABLED(CONFIG_ZCRYPT) has to be used.
>
> Only the ap stuff (which is: ap_bus.o, ap_card.o and ap_queue.o) is statically
> build into the kernel. The zcrypt kernel module is loaded as a dependency
> when a crypto card is detected. So detecting a CEX4 forces kernel module
> zcrypt_cex4.ko which forces the zcrypt.ko to insert into the kernel.
> So all global functions in ap_bus.c are available when CONFIG_ZCRYPT is
> enabled. However, I thought the symbol still needs an EXPORT_SYMBOL()
> statement. Otherwise I should throw out all these statements.
>
> Harald
Thank you for explaining that Harald.
Tony K
>
On 2/4/19 5:01 AM, Sebastian Ott wrote:
> On Thu, 31 Jan 2019, Tony Krowiak wrote:
>> On 1/30/19 1:32 PM, Sebastian Ott wrote:
>>> On Wed, 30 Jan 2019, Tony Krowiak wrote:
>>>> +++ b/drivers/s390/cio/chsc.h
>>>> @@ -9,6 +9,7 @@
>>>> #include <asm/chsc.h>
>>>> #include <asm/schid.h>
>>>> #include <asm/qdio.h>
>>>> +#include <asm/ap.h>
>>>
>>> This should be moved to chsc.c
>>
>> I can do that, but I'm curious why.
>
> You only need to include ap.h because of the ap_bus_cfg_chg definition and
> you only use that in chsc.c . Other users of chsc.h don't call that
> function.
>
> Regards,
> Sebastian
Thanks Sebastian, I've made the change and it will be in v2 when I post
it.
>
On 2/4/19 5:15 AM, Harald Freudenberger wrote:
> On 01.02.19 15:35, Cornelia Huck wrote:
>> On Thu, 31 Jan 2019 18:50:57 -0500
>> Tony Krowiak <[email protected]> wrote:
>>
>>> On 1/31/19 4:55 AM, Cornelia Huck wrote:
>>>> On Wed, 30 Jan 2019 12:48:46 -0500
>>>> Tony Krowiak <[email protected]> wrote:
>>>> Two questions:
>>>> - Does the event cover _any_ change to the AP configuration, or can the
>>>> periodic scan detect changes that are not signaled?
>>> It can detect any change, such as a change to the CRYCB masks.
>> Nice. I suppose we can not rely on those messages being generated,
>> though, and therefore need to keep the periodic scan...
> As you wrote, I am not sure if the ap bus code can rely on this to
> cover all changes. For kvm guests I think it is currently not working
> as there is no such notification generated at all. So I'd like to
> have the periodic scan in place.
I verified that a dynamic change to the CRYCB (via the SE) is not
signaled by CHSC Adjunct Processor changed notification. Apparently only
configuration changes are signaled, so we will definitely need to keep
the scan unless somebody knows otherwise.
Tony K
>>
>>>> - Do we want to generate such an event in QEMU on plugging/unplugging
>>>> the vfio-ap device?
>>> We've discussed this quite a bit internally and decided not to implement
>>> that at this time. We will address it as a future enhancement.
>> Ok, but I think it would be nice to have.
>>
>>>>> diff --git a/drivers/s390/cio/chsc.c b/drivers/s390/cio/chsc.c
>>>>> index a0baee25134c..dccccc337078 100644
>>>>> --- a/drivers/s390/cio/chsc.c
>>>>> +++ b/drivers/s390/cio/chsc.c
>>>>> @@ -586,6 +586,15 @@ static void chsc_process_sei_scm_avail(struct chsc_sei_nt0_area *sei_area)
>>>>> " failed (rc=%d).\n", ret);
>>>>> }
>>>>>
>>>>> +static void chsc_process_sei_ap_cfg_chg(struct chsc_sei_nt0_area *sei_area)
>>>>> +{
>>>>> + CIO_CRW_EVENT(3, "chsc: ap config changed\n");
>>>>> + if (sei_area->rs != 5)
>>>>> + return;
>>>> I'm guessing that a reporting source of 5 means ap, right? (The code is
>>>> silent on all those magic rs values :/)
>>> The 5 indicates the accessibility of one or more adjunct processors has
>>> changed. The reason this gets called is because the CC sent with the
>>> instruction indicates the AP configuration has changed, so the reporting
>>> belongs where it is. There is only one RS associated with it.
>> So if we'd ever get there anything but rs == 5, it would be a hardware
>> or hypervisor bug? Then the code makes sense, I guess.
>>
>>>> If so, should the debug logging be moved after the check?
>>> covered in the response above.
>>>
>>>>
>>>>> +
>>>>> + ap_bus_cfg_chg();
>>>>> +}
>>>>> +
>
On 30.01.19 19:32, Sebastian Ott wrote:
> On Wed, 30 Jan 2019, Tony Krowiak wrote:
>> +#if IS_ENABLED(CONFIG_ZCRYPT)
>> +void ap_bus_cfg_chg(void);
>> +#else
>> +#error "no CONFIG_ZCRYPT"
> ^
> I don't think that's the right thing to do here.
>
>
>> +++ b/drivers/s390/cio/chsc.h
>> @@ -9,6 +9,7 @@
>> #include <asm/chsc.h>
>> #include <asm/schid.h>
>> #include <asm/qdio.h>
>> +#include <asm/ap.h>
> This should be moved to chsc.c
>
>
>> +++ b/drivers/s390/crypto/ap_bus.c
>> @@ -35,6 +35,7 @@
>> #include <linux/mod_devicetable.h>
>> #include <linux/debugfs.h>
>> #include <linux/ctype.h>
>> +#include <asm/crw.h>
> This is not needed here.
>
>
>> /*
>> +* A config change has happened, Force an ap bus rescan.
>> +*/
>> +void ap_bus_cfg_chg(void)
>> +{
>> + AP_DBF(DBF_INFO, "%s config change, forcing bus rescan\n", __func__);
>> +
>> + ap_bus_force_rescan();
>> +}
>> +EXPORT_SYMBOL(ap_bus_cfg_chg);
> There is no need for the export symbol - you don't call that function
> from module code.
That's what I have learned now: You don't need to export a symbol
as long as the symbol is only called in static code parts of the kernel.
But you need to export it when it is intended to be used by code
which sits in a kernel module. So now the big question:
How does a provider of a function in the kernel know, if the caller is in static
code or in module code ? And ... maybe this may even change over
the time. So my recommendation is to always export the symbol with
the EXPORT_SYMBOL macro. This way you don't need to change the
code providing a function when the caller code changes or additional
code uses the symbol.
Other opinions ?
> As an unrelated question, just to be sure: ap_bus.c is compiled as
> built-in even with ZCRYPT=m, right?
>
> Reviewed-by: Sebastian Ott <[email protected]>
>
> Regards,
> Sebastian
On Thu, 21 Feb 2019 11:42:25 +0100
Harald Freudenberger <[email protected]> wrote:
> On 30.01.19 19:32, Sebastian Ott wrote:
> > On Wed, 30 Jan 2019, Tony Krowiak wrote:
> >> /*
> >> +* A config change has happened, Force an ap bus rescan.
> >> +*/
> >> +void ap_bus_cfg_chg(void)
> >> +{
> >> + AP_DBF(DBF_INFO, "%s config change, forcing bus rescan\n", __func__);
> >> +
> >> + ap_bus_force_rescan();
> >> +}
> >> +EXPORT_SYMBOL(ap_bus_cfg_chg);
> > There is no need for the export symbol - you don't call that function
> > from module code.
> That's what I have learned now: You don't need to export a symbol
> as long as the symbol is only called in static code parts of the kernel.
> But you need to export it when it is intended to be used by code
> which sits in a kernel module. So now the big question:
> How does a provider of a function in the kernel know, if the caller is in static
> code or in module code ? And ... maybe this may even change over
> the time. So my recommendation is to always export the symbol with
> the EXPORT_SYMBOL macro. This way you don't need to change the
> code providing a function when the caller code changes or additional
> code uses the symbol.
>
> Other opinions ?
Well, if you know it will be called from module code in upcoming
patches, export it. If not, I consider it the choice of the maintainer.
You can easily add the export later on, if needed, anyway, and I don't
consider changing the code a problem.
In this particular case, both exporting and not exporting looked like
reasonable choices to me.
On Thu, Feb 21, 2019 at 01:12:40PM +0100, Cornelia Huck wrote:
> On Thu, 21 Feb 2019 11:42:25 +0100
> Harald Freudenberger <[email protected]> wrote:
>
> > On 30.01.19 19:32, Sebastian Ott wrote:
> > > On Wed, 30 Jan 2019, Tony Krowiak wrote:
>
> > >> /*
> > >> +* A config change has happened, Force an ap bus rescan.
> > >> +*/
> > >> +void ap_bus_cfg_chg(void)
> > >> +{
> > >> + AP_DBF(DBF_INFO, "%s config change, forcing bus rescan\n", __func__);
> > >> +
> > >> + ap_bus_force_rescan();
> > >> +}
> > >> +EXPORT_SYMBOL(ap_bus_cfg_chg);
> > > There is no need for the export symbol - you don't call that function
> > > from module code.
> > That's what I have learned now: You don't need to export a symbol
> > as long as the symbol is only called in static code parts of the kernel.
> > But you need to export it when it is intended to be used by code
> > which sits in a kernel module. So now the big question:
> > How does a provider of a function in the kernel know, if the caller is in static
> > code or in module code ? And ... maybe this may even change over
> > the time. So my recommendation is to always export the symbol with
> > the EXPORT_SYMBOL macro. This way you don't need to change the
> > code providing a function when the caller code changes or additional
> > code uses the symbol.
> >
> > Other opinions ?
>
> Well, if you know it will be called from module code in upcoming
> patches, export it. If not, I consider it the choice of the maintainer.
> You can easily add the export later on, if needed, anyway, and I don't
> consider changing the code a problem.
>
> In this particular case, both exporting and not exporting looked like
> reasonable choices to me.
The number of exported symbols should be as small as possible. Each
exported symbol eats up extra memory for meta information, plus I
really do not want to deal with semi-automated bots sending patches to
remove not needed exported symbols.
You can check usage of exported symbols with "make export_report".