On 11.12.2012, at 13:46, Cornelia Huck wrote:
> On Tue, 11 Dec 2012 11:22:04 +0100
> Alexander Graf <agraf@xxxxxxx> wrote:
>
>>
>> On 10.12.2012, at 11:09, Cornelia Huck wrote:
>>
>>> On Mon, 10 Dec 2012 08:33:10 +0100
>>> Alexander Graf <agraf@xxxxxxx> wrote:
>>>
>>>>
>>>> On 07.12.2012, at 13:30, Cornelia Huck wrote:
>>>>
>>>>> Add support for handling I/O interrupts (standard, subchannel-related
>>>>> ones and rudimentary adapter interrupts).
>>>>>
>>>>> The subchannel-identifying parameters are encoded into the interrupt
>>>>> type.
>>>>>
>>>>> I/O interrupts are floating, so they can't be injected on a specific
>>>>> vcpu.
>>>>>
>>>>> Reviewed-by: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
>>>>> Signed-off-by: Cornelia Huck <cornelia.huck@xxxxxxxxxx>
>>>>> ---
>>>>> Documentation/virtual/kvm/api.txt | 4 ++
>>>>> arch/s390/include/asm/kvm_host.h | 2 +
>>>>> arch/s390/kvm/interrupt.c | 115 ++++++++++++++++++++++++++++++++++++--
>>>>> include/uapi/linux/kvm.h | 6 ++
>>>>> 4 files changed, 122 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>>>>> index 6671fdc..e298a72 100644
>>>>> --- a/Documentation/virtual/kvm/api.txt
>>>>> +++ b/Documentation/virtual/kvm/api.txt
>>>>> @@ -2068,6 +2068,10 @@ KVM_S390_INT_VIRTIO (vm) - virtio external interrupt; external interrupt
>>>>> KVM_S390_INT_SERVICE (vm) - sclp external interrupt; sclp parameter in parm
>>>>> KVM_S390_INT_EMERGENCY (vcpu) - sigp emergency; source cpu in parm
>>>>> KVM_S390_INT_EXTERNAL_CALL (vcpu) - sigp external call; source cpu in parm
>>>>> +KVM_S390_INT_IO(ai,cssid,ssid,schid) (vm) - compound value to indicate an
>>>>> + I/O interrupt (ai - adapter interrupt; cssid,ssid,schid - subchannel);
>>>>> + I/O interruption parameters in parm (subchannel) and parm64 (intparm,
>>>>> + interruption subclass)
>>>>>
>>>>> Note that the vcpu ioctl is asynchronous to vcpu execution.
>>>>>
>>>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>>>> index b784154..e47f697 100644
>>>>> --- a/arch/s390/include/asm/kvm_host.h
>>>>> +++ b/arch/s390/include/asm/kvm_host.h
>>>>> @@ -76,6 +76,7 @@ struct kvm_s390_sie_block {
>>>>> __u64 epoch; /* 0x0038 */
>>>>> __u8 reserved40[4]; /* 0x0040 */
>>>>> #define LCTL_CR0 0x8000
>>>>> +#define LCTL_CR6 0x0200
>>>>> __u16 lctl; /* 0x0044 */
>>>>> __s16 icpua; /* 0x0046 */
>>>>> __u32 ictl; /* 0x0048 */
>>>>> @@ -127,6 +128,7 @@ struct kvm_vcpu_stat {
>>>>> u32 deliver_prefix_signal;
>>>>> u32 deliver_restart_signal;
>>>>> u32 deliver_program_int;
>>>>> + u32 deliver_io_int;
>>>>> u32 exit_wait_state;
>>>>> u32 instruction_stidp;
>>>>> u32 instruction_spx;
>>>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>>>> index c30615e..070ba22 100644
>>>>> --- a/arch/s390/kvm/interrupt.c
>>>>> +++ b/arch/s390/kvm/interrupt.c
>>>>> @@ -21,11 +21,26 @@
>>>>> #include "gaccess.h"
>>>>> #include "trace-s390.h"
>>>>>
>>>>> +#define IOINT_SCHID_MASK 0x0000ffff
>>>>> +#define IOINT_SSID_MASK 0x00030000
>>>>> +#define IOINT_CSSID_MASK 0x03fc0000
>>>>> +#define IOINT_AI_MASK 0x04000000
>>>>> +
>>>>> +static int is_ioint(u64 type)
>>>>> +{
>>>>> + return ((type & 0xfffe0000u) != 0xfffe0000u);
>>>>> +}
>>>>> +
>>>>> static int psw_extint_disabled(struct kvm_vcpu *vcpu)
>>>>> {
>>>>> return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_EXT);
>>>>> }
>>>>>
>>>>> +static int psw_ioint_disabled(struct kvm_vcpu *vcpu)
>>>>> +{
>>>>> + return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_IO);
>>>>> +}
>>>>> +
>>>>> static int psw_interrupts_disabled(struct kvm_vcpu *vcpu)
>>>>> {
>>>>> if ((vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PER) ||
>>>>> @@ -68,7 +83,18 @@ static int __interrupt_is_deliverable(struct kvm_vcpu *vcpu,
>>>>> case KVM_S390_RESTART:
>>>>> return 1;
>>>>> default:
>>>>> - BUG();
>>>>> + if (is_ioint(inti->type)) {
>>>>
>>>> Though I usually like if (...) { positive) } else { abort(); } coding style in general, it makes code quite hard to read when you are limited to 80 characters per line :)
>>>>
>>>> I think it'd really help readability if you instead would write
>>>>
>>>> if (!is_ioint(...)) {
>>>> BUG();
>>>> }
>>>>
>>>> and then continue without indent. That problem gets even more obvious further down the file.
>>>
>>> Hm, "bad state last" seems to parse, though.
>>
>> Fine with me. Extract the io bits into a separate function then :). Or maybe better even just use gcc switch magic:
>>
>> case 0x0 ... 0xfffdffff:
>
> What, magic numbers? ;)
Well, in code it would be
case KVM_S390_IOINT_MIN ... KVM_S390_IOINT_MAX:
I was just trying to make the point clear :).
> I'll check how this works out. Adding a #define for that range might
> even define better what INT_IO is.
Yup :)
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
