| Message ID | 1550152269-6317-3-git-send-email-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: ap: ioctl definitions for AP Queue Interrupt Control | expand |
On Thu, 14 Feb 2019 14:51:02 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > This patch adds interception code for the PQAP instructions, > and a callback inside the KVM arch structure for s390. > > If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ s/drivers/driver/ > instructions, the driver will initialize the callback inside > the kvm_arch structure to be called when the interception of a > PQAP instruction occurs. > > If the callback is not initialized, the code still returns > -EOPNOTSUPP to let userland handle the instruction as it used to. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com> > --- > arch/s390/include/asm/kvm_host.h | 1 + > arch/s390/kvm/priv.c | 50 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 51 insertions(+) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index c5f5156..49cc8b0 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -719,6 +719,7 @@ struct kvm_s390_cpu_model { > > struct kvm_s390_crypto { > struct kvm_s390_crypto_cb *crycb; > + int (*pqap_hook)(struct kvm_vcpu *vcpu); > __u32 crycbd; > __u8 aes_kw; > __u8 dea_kw; > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c > index 8679bd7..72fdc21 100644 > --- a/arch/s390/kvm/priv.c > +++ b/arch/s390/kvm/priv.c > @@ -27,6 +27,7 @@ > #include <asm/io.h> > #include <asm/ptrace.h> > #include <asm/sclp.h> > +#include <asm/ap.h> > #include "gaccess.h" > #include "kvm-s390.h" > #include "trace.h" > @@ -592,6 +593,53 @@ static int handle_io_inst(struct kvm_vcpu *vcpu) > } > } > > +/* > + * handle_pqap: Handling pqap interception > + * @vcpu: the vcpu having issue the pqap instruction s/issue/issued/ > + * > + * This callback only handles PQAP/AQIC instruction and Here you only talk about PQAP/AQIC, what about PQAP/TAPQ mentioned in the patch description? > + * calls a dedicated callback for this instruction if > + * a driver did register one in the CRYPTO satellite of the > + * SIE block. > + * > + * Do not change the behavior if, return -EOPNOTSUPP if: > + * - the hook is not used do not change the behavior. The hook is not used? Or not set? (I don't think you need to repeat "do not change the behavior".) > + * - AP instructions are not available or not available to the guest > + * - the instruction is not PQAP with function code indicating > + * AQIC do not change the previous behavior. > + * > + * For PQAP/AQIC instruction, verify privilege and specifications > + * > + * return the value returned by the callback. > + */ > +static int handle_pqap(struct kvm_vcpu *vcpu) > +{ > + uint8_t fc; > + > + /* Verify that the hook callback is registered */ > + if (!vcpu->kvm->arch.crypto.pqap_hook) > + return -EOPNOTSUPP; > + /* Verify that the AP instruction are available */ > + if (!ap_instructions_available()) > + return -EOPNOTSUPP; > + /* Verify that the guest is allowed to use AP instructions */ > + if (!(vcpu->arch.sie_block->eca & ECA_APIE)) > + return -EOPNOTSUPP; > + /* Verify that the function code is AQIC */ > + fc = vcpu->run->s.regs.gprs[0] >> 24; > + if (fc != 0x03) > + return -EOPNOTSUPP; > + > + /* PQAP instructions are allowed for guest kernel only */ > + if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) > + return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP); > + /* AQIC instruction is allowed only if facility 65 is available */ > + if (!test_kvm_facility(vcpu->kvm, 65)) > + return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION); > + /* All right, call the callback */ > + return vcpu->kvm->arch.crypto.pqap_hook(vcpu); Can that callback also return -EOPNOTSUPP to order to drop to user space? > +} > + > static int handle_stfl(struct kvm_vcpu *vcpu) > { > int rc; > @@ -878,6 +926,8 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu) > return handle_sthyi(vcpu); > case 0x7d: > return handle_stsi(vcpu); > + case 0xaf: > + return handle_pqap(vcpu); > case 0xb1: > return handle_stfl(vcpu); > case 0xb2:
On 14/02/2019 16:54, Cornelia Huck wrote: > On Thu, 14 Feb 2019 14:51:02 +0100 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> This patch adds interception code for the PQAP instructions, >> and a callback inside the KVM arch structure for s390. >> >> If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ > > s/drivers/driver/ thanks. OK > ... >> #include "kvm-s390.h" >> #include "trace.h" >> @@ -592,6 +593,53 @@ static int handle_io_inst(struct kvm_vcpu *vcpu) >> } >> } >> >> +/* >> + * handle_pqap: Handling pqap interception >> + * @vcpu: the vcpu having issue the pqap instruction > > s/issue/issued/ OK. thanks. > >> + * >> + * This callback only handles PQAP/AQIC instruction and > > Here you only talk about PQAP/AQIC, what about PQAP/TAPQ mentioned in > the patch description? I can add "for now" or "in this patch" or suppress the reference to PAPQ/TAPQ > >> + * calls a dedicated callback for this instruction if >> + * a driver did register one in the CRYPTO satellite of the >> + * SIE block. >> + * >> + * Do not change the behavior if, return -EOPNOTSUPP if: >> + * - the hook is not used do not change the behavior. > > The hook is not used? Or not set? I think "is not set" is better. > (I don't think you need to repeat "do > not change the behavior".) OK > >> + * - AP instructions are not available or not available to the guest >> + * - the instruction is not PQAP with function code indicating >> + * AQIC do not change the previous behavior. >> + * >> + * For PQAP/AQIC instruction, verify privilege and specifications >> + * >> + * return the value returned by the callback. >> + */ >> +static int handle_pqap(struct kvm_vcpu *vcpu) >> +{ >> + uint8_t fc; >> + >> + /* Verify that the hook callback is registered */ >> + if (!vcpu->kvm->arch.crypto.pqap_hook) >> + return -EOPNOTSUPP; >> + /* Verify that the AP instruction are available */ >> + if (!ap_instructions_available()) >> + return -EOPNOTSUPP; >> + /* Verify that the guest is allowed to use AP instructions */ >> + if (!(vcpu->arch.sie_block->eca & ECA_APIE)) >> + return -EOPNOTSUPP; >> + /* Verify that the function code is AQIC */ >> + fc = vcpu->run->s.regs.gprs[0] >> 24; >> + if (fc != 0x03) >> + return -EOPNOTSUPP; >> + >> + /* PQAP instructions are allowed for guest kernel only */ >> + if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) >> + return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP); >> + /* AQIC instruction is allowed only if facility 65 is available */ >> + if (!test_kvm_facility(vcpu->kvm, 65)) >> + return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION); >> + /* All right, call the callback */ >> + return vcpu->kvm->arch.crypto.pqap_hook(vcpu); > > Can that callback also return -EOPNOTSUPP to order to drop to user > space? Yes. Why not? > >> +} >> + >> static int handle_stfl(struct kvm_vcpu *vcpu) >> { >> int rc; >> @@ -878,6 +926,8 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu) >> return handle_sthyi(vcpu); >> case 0x7d: >> return handle_stsi(vcpu); >> + case 0xaf: >> + return handle_pqap(vcpu); >> case 0xb1: >> return handle_stfl(vcpu); >> case 0xb2: >
On Thu, 14 Feb 2019 17:45:06 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > On 14/02/2019 16:54, Cornelia Huck wrote: > > On Thu, 14 Feb 2019 14:51:02 +0100 > > Pierre Morel <pmorel@linux.ibm.com> wrote: > > > >> This patch adds interception code for the PQAP instructions, > >> and a callback inside the KVM arch structure for s390. > >> > >> If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ > > > > s/drivers/driver/ > > thanks. OK > > >> + * > >> + * This callback only handles PQAP/AQIC instruction and > > > > Here you only talk about PQAP/AQIC, what about PQAP/TAPQ mentioned in > > the patch description? > > I can add "for now" or "in this patch" or suppress the reference to > PAPQ/TAPQ I'd just add a note to the patch description that this patch only handles PQAP/AQCI and that handling PQAP/TAPQ is something for a follow-on patch. > > > > >> + * calls a dedicated callback for this instruction if > >> + * a driver did register one in the CRYPTO satellite of the > >> + * SIE block. > >> + * > >> + * Do not change the behavior if, return -EOPNOTSUPP if: > >> + * - the hook is not used do not change the behavior. > > > > The hook is not used? Or not set? > > I think "is not set" is better. Ok. > > > (I don't think you need to repeat "do > > not change the behavior".) > > OK > > > > >> + * - AP instructions are not available or not available to the guest > >> + * - the instruction is not PQAP with function code indicating > >> + * AQIC do not change the previous behavior. > >> + * > >> + * For PQAP/AQIC instruction, verify privilege and specifications > >> + * > >> + * return the value returned by the callback. > >> + */ > >> +static int handle_pqap(struct kvm_vcpu *vcpu) > >> +{ > >> + uint8_t fc; > >> + > >> + /* Verify that the hook callback is registered */ > >> + if (!vcpu->kvm->arch.crypto.pqap_hook) > >> + return -EOPNOTSUPP; > >> + /* Verify that the AP instruction are available */ > >> + if (!ap_instructions_available()) > >> + return -EOPNOTSUPP; > >> + /* Verify that the guest is allowed to use AP instructions */ > >> + if (!(vcpu->arch.sie_block->eca & ECA_APIE)) > >> + return -EOPNOTSUPP; > >> + /* Verify that the function code is AQIC */ > >> + fc = vcpu->run->s.regs.gprs[0] >> 24; > >> + if (fc != 0x03) > >> + return -EOPNOTSUPP; > >> + > >> + /* PQAP instructions are allowed for guest kernel only */ > >> + if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) > >> + return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP); > >> + /* AQIC instruction is allowed only if facility 65 is available */ > >> + if (!test_kvm_facility(vcpu->kvm, 65)) > >> + return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION); > >> + /* All right, call the callback */ > >> + return vcpu->kvm->arch.crypto.pqap_hook(vcpu); > > > > Can that callback also return -EOPNOTSUPP to order to drop to user > > space? > > Yes. > Why not? Maybe also mention that in the function description?
On 15/02/2019 10:26, Cornelia Huck wrote: > On Thu, 14 Feb 2019 17:45:06 +0100 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> On 14/02/2019 16:54, Cornelia Huck wrote: >>> On Thu, 14 Feb 2019 14:51:02 +0100 >>> Pierre Morel <pmorel@linux.ibm.com> wrote: >>> >>>> This patch adds interception code for the PQAP instructions, >>>> and a callback inside the KVM arch structure for s390. >>>> >>>> If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ >>> >>> s/drivers/driver/ >> >> thanks. OK >> > >>>> + * >>>> + * This callback only handles PQAP/AQIC instruction and >>> >>> Here you only talk about PQAP/AQIC, what about PQAP/TAPQ mentioned in >>> the patch description? >> >> I can add "for now" or "in this patch" or suppress the reference to >> PAPQ/TAPQ > > I'd just add a note to the patch description that this patch only > handles PQAP/AQCI and that handling PQAP/TAPQ is something for a > follow-on patch. OK, I will clear this. Thanks > >> >>> ...snip... >>>> + /* PQAP instructions are allowed for guest kernel only */ >>>> + if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) >>>> + return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP); >>>> + /* AQIC instruction is allowed only if facility 65 is available */ >>>> + if (!test_kvm_facility(vcpu->kvm, 65)) >>>> + return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION); >>>> + /* All right, call the callback */ >>>> + return vcpu->kvm->arch.crypto.pqap_hook(vcpu); >>> >>> Can that callback also return -EOPNOTSUPP to order to drop to user >>> space? >> >> Yes. >> Why not? > > Maybe also mention that in the function description? > Will do thanks. Regards, Pierre
On 2/14/19 8:51 AM, Pierre Morel wrote: > This patch adds interception code for the PQAP instructions, > and a callback inside the KVM arch structure for s390. > > If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ > instructions, the driver will initialize the callback inside > the kvm_arch structure to be called when the interception of a > PQAP instruction occurs. > > If the callback is not initialized, the code still returns > -EOPNOTSUPP to let userland handle the instruction as it used to. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com> > --- > arch/s390/include/asm/kvm_host.h | 1 + > arch/s390/kvm/priv.c | 50 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 51 insertions(+) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index c5f5156..49cc8b0 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -719,6 +719,7 @@ struct kvm_s390_cpu_model { > > struct kvm_s390_crypto { > struct kvm_s390_crypto_cb *crycb; > + int (*pqap_hook)(struct kvm_vcpu *vcpu); > __u32 crycbd; > __u8 aes_kw; > __u8 dea_kw; > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c > index 8679bd7..72fdc21 100644 > --- a/arch/s390/kvm/priv.c > +++ b/arch/s390/kvm/priv.c > @@ -27,6 +27,7 @@ > #include <asm/io.h> > #include <asm/ptrace.h> > #include <asm/sclp.h> > +#include <asm/ap.h> > #include "gaccess.h" > #include "kvm-s390.h" > #include "trace.h" > @@ -592,6 +593,53 @@ static int handle_io_inst(struct kvm_vcpu *vcpu) > } > } > > +/* > + * handle_pqap: Handling pqap interception > + * @vcpu: the vcpu having issue the pqap instruction > + * > + * This callback only handles PQAP/AQIC instruction and > + * calls a dedicated callback for this instruction if > + * a driver did register one in the CRYPTO satellite of the > + * SIE block. > + * > + * Do not change the behavior if, return -EOPNOTSUPP if: > + * - the hook is not used do not change the behavior. > + * - AP instructions are not available or not available to the guest > + * - the instruction is not PQAP with function code indicating > + * AQIC do not change the previous behavior. > + * > + * For PQAP/AQIC instruction, verify privilege and specifications > + * > + * return the value returned by the callback. > + */ > +static int handle_pqap(struct kvm_vcpu *vcpu) > +{ > + uint8_t fc; > + > + /* Verify that the hook callback is registered */ > + if (!vcpu->kvm->arch.crypto.pqap_hook) > + return -EOPNOTSUPP; > + /* Verify that the AP instruction are available */ > + if (!ap_instructions_available()) > + return -EOPNOTSUPP; > + /* Verify that the guest is allowed to use AP instructions */ > + if (!(vcpu->arch.sie_block->eca & ECA_APIE)) > + return -EOPNOTSUPP; > + /* Verify that the function code is AQIC */ > + fc = vcpu->run->s.regs.gprs[0] >> 24; > + if (fc != 0x03) > + return -EOPNOTSUPP; This does not belong here. Function code 3 is one of 7 function codes that can be sent with the PQAP instruction. This belongs in the PQAP hook code. > + > + /* PQAP instructions are allowed for guest kernel only */ > + if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) > + return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP); > + /* AQIC instruction is allowed only if facility 65 is available */ > + if (!test_kvm_facility(vcpu->kvm, 65)) > + return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION); > + /* All right, call the callback */ > + return vcpu->kvm->arch.crypto.pqap_hook(vcpu); > +} > + > static int handle_stfl(struct kvm_vcpu *vcpu) > { > int rc; > @@ -878,6 +926,8 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu) > return handle_sthyi(vcpu); > case 0x7d: > return handle_stsi(vcpu); > + case 0xaf: > + return handle_pqap(vcpu); > case 0xb1: > return handle_stfl(vcpu); > case 0xb2: >
On 15/02/2019 23:02, Tony Krowiak wrote: > On 2/14/19 8:51 AM, Pierre Morel wrote: >> This patch adds interception code for the PQAP instructions, >> and a callback inside the KVM arch structure for s390. >> >> If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ >> instructions, the driver will initialize the callback inside >> the kvm_arch structure to be called when the interception of a >> PQAP instruction occurs. >> >> If the callback is not initialized, the code still returns >> -EOPNOTSUPP to let userland handle the instruction as it used to. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com> >> --- >> arch/s390/include/asm/kvm_host.h | 1 + >> arch/s390/kvm/priv.c | 50 >> ++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 51 insertions(+) >> >> diff --git a/arch/s390/include/asm/kvm_host.h >> b/arch/s390/include/asm/kvm_host.h >> index c5f5156..49cc8b0 100644 >> --- a/arch/s390/include/asm/kvm_host.h >> +++ b/arch/s390/include/asm/kvm_host.h >> @@ -719,6 +719,7 @@ struct kvm_s390_cpu_model { >> struct kvm_s390_crypto { >> struct kvm_s390_crypto_cb *crycb; >> + int (*pqap_hook)(struct kvm_vcpu *vcpu); >> __u32 crycbd; >> __u8 aes_kw; >> __u8 dea_kw; >> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c >> index 8679bd7..72fdc21 100644 >> --- a/arch/s390/kvm/priv.c >> +++ b/arch/s390/kvm/priv.c >> @@ -27,6 +27,7 @@ >> #include <asm/io.h> >> #include <asm/ptrace.h> >> #include <asm/sclp.h> >> +#include <asm/ap.h> >> #include "gaccess.h" >> #include "kvm-s390.h" >> #include "trace.h" >> @@ -592,6 +593,53 @@ static int handle_io_inst(struct kvm_vcpu *vcpu) >> } >> } >> +/* >> + * handle_pqap: Handling pqap interception >> + * @vcpu: the vcpu having issue the pqap instruction >> + * >> + * This callback only handles PQAP/AQIC instruction and >> + * calls a dedicated callback for this instruction if >> + * a driver did register one in the CRYPTO satellite of the >> + * SIE block. >> + * >> + * Do not change the behavior if, return -EOPNOTSUPP if: >> + * - the hook is not used do not change the behavior. >> + * - AP instructions are not available or not available to the guest >> + * - the instruction is not PQAP with function code indicating >> + * AQIC do not change the previous behavior. >> + * >> + * For PQAP/AQIC instruction, verify privilege and specifications >> + * >> + * return the value returned by the callback. >> + */ >> +static int handle_pqap(struct kvm_vcpu *vcpu) >> +{ >> + uint8_t fc; >> + >> + /* Verify that the hook callback is registered */ >> + if (!vcpu->kvm->arch.crypto.pqap_hook) >> + return -EOPNOTSUPP; >> + /* Verify that the AP instruction are available */ >> + if (!ap_instructions_available()) >> + return -EOPNOTSUPP; >> + /* Verify that the guest is allowed to use AP instructions */ >> + if (!(vcpu->arch.sie_block->eca & ECA_APIE)) >> + return -EOPNOTSUPP; >> + /* Verify that the function code is AQIC */ >> + fc = vcpu->run->s.regs.gprs[0] >> 24; >> + if (fc != 0x03) >> + return -EOPNOTSUPP; > > This does not belong here. Function code 3 is one of 7 function codes > that can be sent with the PQAP instruction. This belongs in the PQAP > hook code. On one hand, effectively I would prefer to put the code in the VFIO driver code. On the other hand, doing this would lead to export the code for test_kvm_facility() and kvm_s390_inject_program_int() from the kvm-s390.h I choose not to export these functions from the KVM code. Would like opinion from KVM maintainers? > >> + >> + /* PQAP instructions are allowed for guest kernel only */ >> + if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) >> + return kvm_s390_inject_program_intkvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP); >> + /* AQIC instruction is allowed only if facility 65 is available */ >> + if (!test_kvm_facility(vcpu->kvm, 65)) >> + return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION); >> + /* All right, call the callback */ >> + return vcpu->kvm->arch.crypto.pqap_hook(vcpu); >> +} >> + >> static int handle_stfl(struct kvm_vcpu *vcpu) >> { >> int rc; >> @@ -878,6 +926,8 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu) >> return handle_sthyi(vcpu); >> case 0x7d: >> return handle_stsi(vcpu); >> + case 0xaf: >> + return handle_pqap(vcpu); >> case 0xb1: >> return handle_stfl(vcpu); >> case 0xb2: >> >
On Mon, 18 Feb 2019 19:29:10 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > On 15/02/2019 23:02, Tony Krowiak wrote: > > On 2/14/19 8:51 AM, Pierre Morel wrote: > >> +/* > >> + * handle_pqap: Handling pqap interception > >> + * @vcpu: the vcpu having issue the pqap instruction > >> + * > >> + * This callback only handles PQAP/AQIC instruction and > >> + * calls a dedicated callback for this instruction if > >> + * a driver did register one in the CRYPTO satellite of the > >> + * SIE block. > >> + * > >> + * Do not change the behavior if, return -EOPNOTSUPP if: > >> + * - the hook is not used do not change the behavior. > >> + * - AP instructions are not available or not available to the guest > >> + * - the instruction is not PQAP with function code indicating > >> + * AQIC do not change the previous behavior. > >> + * > >> + * For PQAP/AQIC instruction, verify privilege and specifications > >> + * > >> + * return the value returned by the callback. > >> + */ > >> +static int handle_pqap(struct kvm_vcpu *vcpu) > >> +{ > >> + uint8_t fc; > >> + > >> + /* Verify that the hook callback is registered */ > >> + if (!vcpu->kvm->arch.crypto.pqap_hook) > >> + return -EOPNOTSUPP; > >> + /* Verify that the AP instruction are available */ > >> + if (!ap_instructions_available()) > >> + return -EOPNOTSUPP; > >> + /* Verify that the guest is allowed to use AP instructions */ > >> + if (!(vcpu->arch.sie_block->eca & ECA_APIE)) > >> + return -EOPNOTSUPP; > >> + /* Verify that the function code is AQIC */ > >> + fc = vcpu->run->s.regs.gprs[0] >> 24; > >> + if (fc != 0x03) > >> + return -EOPNOTSUPP; > > > > This does not belong here. Function code 3 is one of 7 function codes > > that can be sent with the PQAP instruction. This belongs in the PQAP > > hook code. > > On one hand, effectively I would prefer to put the code in the VFIO > driver code. > On the other hand, doing this would lead to export the code for > test_kvm_facility() and kvm_s390_inject_program_int() from the kvm-s390.h > > I choose not to export these functions from the KVM code. > > Would like opinion from KVM maintainers? Looking at this (and without access to the specification...), I think the check for problem state makes sense in here (if this applies to all PQAP functions equally, which seems likely). The check for the facility makes more sense in the handler. You can probably still inject the specification exception here if you use a clever return code. Another option: Provide a way to register a callback per function code; this allows you to still do the check here and extend it later for other function codes (which will probably be indicated by another facility). > > > > >> + > >> + /* PQAP instructions are allowed for guest kernel only */ > >> + if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) > >> + return kvm_s390_inject_program_intkvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP); > >> + /* AQIC instruction is allowed only if facility 65 is available */ > >> + if (!test_kvm_facility(vcpu->kvm, 65)) > >> + return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION); > >> + /* All right, call the callback */ > >> + return vcpu->kvm->arch.crypto.pqap_hook(vcpu); > >> +} > >> + > >> static int handle_stfl(struct kvm_vcpu *vcpu) > >> { > >> int rc; > >> @@ -878,6 +926,8 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu) > >> return handle_sthyi(vcpu); > >> case 0x7d: > >> return handle_stsi(vcpu); > >> + case 0xaf: > >> + return handle_pqap(vcpu); > >> case 0xb1: > >> return handle_stfl(vcpu); > >> case 0xb2: > >> > > > >
On 18/02/2019 23:42, Cornelia Huck wrote: > On Mon, 18 Feb 2019 19:29:10 +0100 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> On 15/02/2019 23:02, Tony Krowiak wrote: >>> On 2/14/19 8:51 AM, Pierre Morel wrote: > >>>> +/* >>>> + * handle_pqap: Handling pqap interception >>>> + * @vcpu: the vcpu having issue the pqap instruction >>>> + * >>>> + * This callback only handles PQAP/AQIC instruction and >>>> + * calls a dedicated callback for this instruction if >>>> + * a driver did register one in the CRYPTO satellite of the >>>> + * SIE block. >>>> + * >>>> + * Do not change the behavior if, return -EOPNOTSUPP if: >>>> + * - the hook is not used do not change the behavior. >>>> + * - AP instructions are not available or not available to the guest >>>> + * - the instruction is not PQAP with function code indicating >>>> + * AQIC do not change the previous behavior. >>>> + * >>>> + * For PQAP/AQIC instruction, verify privilege and specifications >>>> + * >>>> + * return the value returned by the callback. >>>> + */ >>>> +static int handle_pqap(struct kvm_vcpu *vcpu) >>>> +{ >>>> + uint8_t fc; >>>> + >>>> + /* Verify that the hook callback is registered */ >>>> + if (!vcpu->kvm->arch.crypto.pqap_hook) >>>> + return -EOPNOTSUPP; >>>> + /* Verify that the AP instruction are available */ >>>> + if (!ap_instructions_available()) >>>> + return -EOPNOTSUPP; >>>> + /* Verify that the guest is allowed to use AP instructions */ >>>> + if (!(vcpu->arch.sie_block->eca & ECA_APIE)) >>>> + return -EOPNOTSUPP; >>>> + /* Verify that the function code is AQIC */ >>>> + fc = vcpu->run->s.regs.gprs[0] >> 24; >>>> + if (fc != 0x03) >>>> + return -EOPNOTSUPP; >>> >>> This does not belong here. Function code 3 is one of 7 function codes >>> that can be sent with the PQAP instruction. This belongs in the PQAP >>> hook code. >> >> On one hand, effectively I would prefer to put the code in the VFIO >> driver code. >> On the other hand, doing this would lead to export the code for >> test_kvm_facility() and kvm_s390_inject_program_int() from the kvm-s390.h >> >> I choose not to export these functions from the KVM code. >> >> Would like opinion from KVM maintainers? > > Looking at this (and without access to the specification...), I think > the check for problem state makes sense in here (if this applies to all > PQAP functions equally, which seems likely). The check for the facility > makes more sense in the handler. You can probably still inject the > specification exception here if you use a clever return code. > If there is no objection on exporting the KVM functions... I can do this. > Another option: Provide a way to register a callback per function code; > this allows you to still do the check here and extend it later for > other function codes (which will probably be indicated by another > facility). I like this option even better. Regards, Pierre
On 2/19/19 2:50 PM, Pierre Morel wrote: > On 18/02/2019 23:42, Cornelia Huck wrote: >> On Mon, 18 Feb 2019 19:29:10 +0100 >> Pierre Morel <pmorel@linux.ibm.com> wrote: >> >>> On 15/02/2019 23:02, Tony Krowiak wrote: >>>> On 2/14/19 8:51 AM, Pierre Morel wrote: >> >>>>> +/* >>>>> + * handle_pqap: Handling pqap interception >>>>> + * @vcpu: the vcpu having issue the pqap instruction >>>>> + * >>>>> + * This callback only handles PQAP/AQIC instruction and >>>>> + * calls a dedicated callback for this instruction if >>>>> + * a driver did register one in the CRYPTO satellite of the >>>>> + * SIE block. >>>>> + * >>>>> + * Do not change the behavior if, return -EOPNOTSUPP if: >>>>> + * - the hook is not used do not change the behavior. >>>>> + * - AP instructions are not available or not available to the guest >>>>> + * - the instruction is not PQAP with function code indicating >>>>> + * AQIC do not change the previous behavior. >>>>> + * >>>>> + * For PQAP/AQIC instruction, verify privilege and specifications >>>>> + * >>>>> + * return the value returned by the callback. >>>>> + */ >>>>> +static int handle_pqap(struct kvm_vcpu *vcpu) >>>>> +{ >>>>> + uint8_t fc; >>>>> + >>>>> + /* Verify that the hook callback is registered */ >>>>> + if (!vcpu->kvm->arch.crypto.pqap_hook) >>>>> + return -EOPNOTSUPP; >>>>> + /* Verify that the AP instruction are available */ >>>>> + if (!ap_instructions_available()) >>>>> + return -EOPNOTSUPP; >>>>> + /* Verify that the guest is allowed to use AP instructions */ >>>>> + if (!(vcpu->arch.sie_block->eca & ECA_APIE)) >>>>> + return -EOPNOTSUPP; >>>>> + /* Verify that the function code is AQIC */ >>>>> + fc = vcpu->run->s.regs.gprs[0] >> 24; >>>>> + if (fc != 0x03) >>>>> + return -EOPNOTSUPP; >>>> >>>> This does not belong here. Function code 3 is one of 7 function codes >>>> that can be sent with the PQAP instruction. This belongs in the PQAP >>>> hook code. >>> >>> On one hand, effectively I would prefer to put the code in the VFIO >>> driver code. >>> On the other hand, doing this would lead to export the code for >>> test_kvm_facility() and kvm_s390_inject_program_int() from the >>> kvm-s390.h >>> >>> I choose not to export these functions from the KVM code. >>> >>> Would like opinion from KVM maintainers? >> >> Looking at this (and without access to the specification...), I think >> the check for problem state makes sense in here (if this applies to all >> PQAP functions equally, which seems likely). The check for the facility >> makes more sense in the handler. You can probably still inject the >> specification exception here if you use a clever return code. >> > > If there is no objection on exporting the KVM functions... I can do this. I do not understand why you would have to export KVM functions to place the check for FC 0x03 in the pqap hook? What am I missing here? Maybe you misunderstood my comment? The following is the only code I suggested you move: + fc = vcpu->run->s.regs.gprs[0] >> 24; + if (fc != 0x03) + return -EOPNOTSUPP; What does that have to do with test_kvm_facility() and kvm_s390_inject_program_int()? Please explain. > >> Another option: Provide a way to register a callback per function code; >> this allows you to still do the check here and extend it later for >> other function codes (which will probably be indicated by another >> facility). > > I like this option even better. > > Regards, > Pierre > >
On 2/19/19 2:50 PM, Pierre Morel wrote: > On 18/02/2019 23:42, Cornelia Huck wrote: >> On Mon, 18 Feb 2019 19:29:10 +0100 >> Pierre Morel <pmorel@linux.ibm.com> wrote: >> >>> On 15/02/2019 23:02, Tony Krowiak wrote: >>>> On 2/14/19 8:51 AM, Pierre Morel wrote: >> >>>>> +/* >>>>> + * handle_pqap: Handling pqap interception >>>>> + * @vcpu: the vcpu having issue the pqap instruction >>>>> + * >>>>> + * This callback only handles PQAP/AQIC instruction and >>>>> + * calls a dedicated callback for this instruction if >>>>> + * a driver did register one in the CRYPTO satellite of the >>>>> + * SIE block. >>>>> + * >>>>> + * Do not change the behavior if, return -EOPNOTSUPP if: >>>>> + * - the hook is not used do not change the behavior. >>>>> + * - AP instructions are not available or not available to the guest >>>>> + * - the instruction is not PQAP with function code indicating >>>>> + * AQIC do not change the previous behavior. >>>>> + * >>>>> + * For PQAP/AQIC instruction, verify privilege and specifications >>>>> + * >>>>> + * return the value returned by the callback. >>>>> + */ >>>>> +static int handle_pqap(struct kvm_vcpu *vcpu) >>>>> +{ >>>>> + uint8_t fc; >>>>> + >>>>> + /* Verify that the hook callback is registered */ >>>>> + if (!vcpu->kvm->arch.crypto.pqap_hook) >>>>> + return -EOPNOTSUPP; >>>>> + /* Verify that the AP instruction are available */ >>>>> + if (!ap_instructions_available()) >>>>> + return -EOPNOTSUPP; >>>>> + /* Verify that the guest is allowed to use AP instructions */ >>>>> + if (!(vcpu->arch.sie_block->eca & ECA_APIE)) >>>>> + return -EOPNOTSUPP; >>>>> + /* Verify that the function code is AQIC */ >>>>> + fc = vcpu->run->s.regs.gprs[0] >> 24; >>>>> + if (fc != 0x03) >>>>> + return -EOPNOTSUPP; >>>> >>>> This does not belong here. Function code 3 is one of 7 function codes >>>> that can be sent with the PQAP instruction. This belongs in the PQAP >>>> hook code. >>> >>> On one hand, effectively I would prefer to put the code in the VFIO >>> driver code. >>> On the other hand, doing this would lead to export the code for >>> test_kvm_facility() and kvm_s390_inject_program_int() from the >>> kvm-s390.h >>> >>> I choose not to export these functions from the KVM code. >>> >>> Would like opinion from KVM maintainers? >> >> Looking at this (and without access to the specification...), I think >> the check for problem state makes sense in here (if this applies to all >> PQAP functions equally, which seems likely). The check for the facility >> makes more sense in the handler. You can probably still inject the >> specification exception here if you use a clever return code. >> > > If there is no objection on exporting the KVM functions... I can do this. I think I understand where you are coming from. In looking back at the original patch, I see there are checks using the test_kvm_facility and kvm_s390_inject_program_int functions placed after your check for fc != 0x03. You clearly misunderstood what I was asking you to do. I was suggesting that ONLY the check for 'fc != 0x03' be done in the hook. I was NOT suggesting the instructions following the check for fc != 0x03 be done in the hook, so there is no need to export any KVM functions. > >> Another option: Provide a way to register a callback per function code; >> this allows you to still do the check here and extend it later for >> other function codes (which will probably be indicated by another >> facility). > > I like this option even better. > > Regards, > Pierre > >
On 19/02/2019 23:36, Tony Krowiak wrote: > On 2/19/19 2:50 PM, Pierre Morel wrote: >> On 18/02/2019 23:42, Cornelia Huck wrote: >>> On Mon, 18 Feb 2019 19:29:10 +0100 >>> Pierre Morel <pmorel@linux.ibm.com> wrote: >>> >>>> On 15/02/2019 23:02, Tony Krowiak wrote: >>>>> On 2/14/19 8:51 AM, Pierre Morel wrote: >>> >>>>>> +/* >>>>>> + * handle_pqap: Handling pqap interception >>>>>> + * @vcpu: the vcpu having issue the pqap instruction >>>>>> + * >>>>>> + * This callback only handles PQAP/AQIC instruction and >>>>>> + * calls a dedicated callback for this instruction if >>>>>> + * a driver did register one in the CRYPTO satellite of the >>>>>> + * SIE block. >>>>>> + * >>>>>> + * Do not change the behavior if, return -EOPNOTSUPP if: >>>>>> + * - the hook is not used do not change the behavior. >>>>>> + * - AP instructions are not available or not available to the guest >>>>>> + * - the instruction is not PQAP with function code indicating >>>>>> + * AQIC do not change the previous behavior. >>>>>> + * >>>>>> + * For PQAP/AQIC instruction, verify privilege and specifications >>>>>> + * >>>>>> + * return the value returned by the callback. >>>>>> + */ >>>>>> +static int handle_pqap(struct kvm_vcpu *vcpu) >>>>>> +{ >>>>>> + uint8_t fc; >>>>>> + >>>>>> + /* Verify that the hook callback is registered */ >>>>>> + if (!vcpu->kvm->arch.crypto.pqap_hook) >>>>>> + return -EOPNOTSUPP; >>>>>> + /* Verify that the AP instruction are available */ >>>>>> + if (!ap_instructions_available()) >>>>>> + return -EOPNOTSUPP; >>>>>> + /* Verify that the guest is allowed to use AP instructions */ >>>>>> + if (!(vcpu->arch.sie_block->eca & ECA_APIE)) >>>>>> + return -EOPNOTSUPP; >>>>>> + /* Verify that the function code is AQIC */ >>>>>> + fc = vcpu->run->s.regs.gprs[0] >> 24; >>>>>> + if (fc != 0x03) >>>>>> + return -EOPNOTSUPP; >>>>> >>>>> This does not belong here. Function code 3 is one of 7 function codes >>>>> that can be sent with the PQAP instruction. This belongs in the PQAP >>>>> hook code. >>>> >>>> On one hand, effectively I would prefer to put the code in the VFIO >>>> driver code. >>>> On the other hand, doing this would lead to export the code for >>>> test_kvm_facility() and kvm_s390_inject_program_int() from the >>>> kvm-s390.h >>>> >>>> I choose not to export these functions from the KVM code. >>>> >>>> Would like opinion from KVM maintainers? >>> >>> Looking at this (and without access to the specification...), I think >>> the check for problem state makes sense in here (if this applies to all >>> PQAP functions equally, which seems likely). The check for the facility >>> makes more sense in the handler. You can probably still inject the >>> specification exception here if you use a clever return code. >>> >> >> If there is no objection on exporting the KVM functions... I can do this. > > I do not understand why you would have to export KVM functions to place > the check for FC 0x03 in the pqap hook? What am I missing here? Maybe > you misunderstood my comment? No I did not but in between I discovered an error in the handling of the interception of PQAP/AQIC. QEMU and KVM can both accept PQAP/AQIC even if the vfio_ap driver is not loaded. However now that the guest officially get the PQAP/AQIC instruction we need to handle the specification and operation exceptions inside KVM _before_ testing and even calling the driver hook. I will make the changes in the next iteration. Regards, Pierre
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index c5f5156..49cc8b0 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -719,6 +719,7 @@ struct kvm_s390_cpu_model { struct kvm_s390_crypto { struct kvm_s390_crypto_cb *crycb; + int (*pqap_hook)(struct kvm_vcpu *vcpu); __u32 crycbd; __u8 aes_kw; __u8 dea_kw; diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c index 8679bd7..72fdc21 100644 --- a/arch/s390/kvm/priv.c +++ b/arch/s390/kvm/priv.c @@ -27,6 +27,7 @@ #include <asm/io.h> #include <asm/ptrace.h> #include <asm/sclp.h> +#include <asm/ap.h> #include "gaccess.h" #include "kvm-s390.h" #include "trace.h" @@ -592,6 +593,53 @@ static int handle_io_inst(struct kvm_vcpu *vcpu) } } +/* + * handle_pqap: Handling pqap interception + * @vcpu: the vcpu having issue the pqap instruction + * + * This callback only handles PQAP/AQIC instruction and + * calls a dedicated callback for this instruction if + * a driver did register one in the CRYPTO satellite of the + * SIE block. + * + * Do not change the behavior if, return -EOPNOTSUPP if: + * - the hook is not used do not change the behavior. + * - AP instructions are not available or not available to the guest + * - the instruction is not PQAP with function code indicating + * AQIC do not change the previous behavior. + * + * For PQAP/AQIC instruction, verify privilege and specifications + * + * return the value returned by the callback. + */ +static int handle_pqap(struct kvm_vcpu *vcpu) +{ + uint8_t fc; + + /* Verify that the hook callback is registered */ + if (!vcpu->kvm->arch.crypto.pqap_hook) + return -EOPNOTSUPP; + /* Verify that the AP instruction are available */ + if (!ap_instructions_available()) + return -EOPNOTSUPP; + /* Verify that the guest is allowed to use AP instructions */ + if (!(vcpu->arch.sie_block->eca & ECA_APIE)) + return -EOPNOTSUPP; + /* Verify that the function code is AQIC */ + fc = vcpu->run->s.regs.gprs[0] >> 24; + if (fc != 0x03) + return -EOPNOTSUPP; + + /* PQAP instructions are allowed for guest kernel only */ + if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) + return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP); + /* AQIC instruction is allowed only if facility 65 is available */ + if (!test_kvm_facility(vcpu->kvm, 65)) + return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION); + /* All right, call the callback */ + return vcpu->kvm->arch.crypto.pqap_hook(vcpu); +} + static int handle_stfl(struct kvm_vcpu *vcpu) { int rc; @@ -878,6 +926,8 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu) return handle_sthyi(vcpu); case 0x7d: return handle_stsi(vcpu); + case 0xaf: + return handle_pqap(vcpu); case 0xb1: return handle_stfl(vcpu); case 0xb2: