| Message ID | 1427814488-28467-5-git-send-email-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
> This commit adds a stub function to support the KVM_SET_GUEST_DEBUG > ioctl. Currently any operation flag will return EINVAL. Actual Well it won't return -EINVAL if you push in KVM_GUESTDBG_ENABLE or 0. "Any unsupported flag will return -EINVAL. For now, only KVM_GUESTDBG_ENABLE is supported, although it won't have any effects." > functionality will be added with further patches. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>. > > --- > v2 > - simplified form of the ioctl (stuff will go into setup_debug) > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > index b112efc..06c5064 100644 > --- a/Documentation/virtual/kvm/api.txt > +++ b/Documentation/virtual/kvm/api.txt > @@ -2604,7 +2604,7 @@ handled. > 4.87 KVM_SET_GUEST_DEBUG > > Capability: KVM_CAP_SET_GUEST_DEBUG > -Architectures: x86, s390, ppc > +Architectures: x86, s390, ppc, arm64 > Type: vcpu ioctl > Parameters: struct kvm_guest_debug (in) > Returns: 0 on success; -1 on error > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > index 5560f74..445933d 100644 > --- a/arch/arm/kvm/arm.c > +++ b/arch/arm/kvm/arm.c > @@ -183,6 +183,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) > case KVM_CAP_ARM_PSCI: > case KVM_CAP_ARM_PSCI_0_2: > case KVM_CAP_READONLY_MEM: > + case KVM_CAP_SET_GUEST_DEBUG: > r = 1; > break; > case KVM_CAP_COALESCED_MMIO: > @@ -303,10 +304,21 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu) > kvm_arm_set_running_vcpu(NULL); > } > > +#define KVM_GUESTDBG_VALID (KVM_GUESTDBG_ENABLE) That makes me rather think that it is another flag. We(s390x) use VALID_GUESTDBG_FLAGS, what about that or KVM_GUESTDBG_VALID_MASK? > + > int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, > struct kvm_guest_debug *dbg) > { > - return -EINVAL; > + if (dbg->control & KVM_GUESTDBG_ENABLE) { > + if (dbg->control & ~KVM_GUESTDBG_VALID) > + return -EINVAL; I'd move that check directly to the start of the function and bail out on any unsupported flag. > + > + vcpu->guest_debug = dbg->control; > + } else { > + /* If not enabled clear all flags */ > + vcpu->guest_debug = 0; > + } > + return 0; > } > > David
On Wed, Apr 01, 2015 at 05:55:29PM +0200, David Hildenbrand wrote: > > This commit adds a stub function to support the KVM_SET_GUEST_DEBUG > > ioctl. Currently any operation flag will return EINVAL. Actual > > Well it won't return -EINVAL if you push in KVM_GUESTDBG_ENABLE or 0. > > "Any unsupported flag will return -EINVAL. For now, only KVM_GUESTDBG_ENABLE is > supported, although it won't have any effects." > > > functionality will be added with further patches. > > > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>. > > > > --- > > v2 > > - simplified form of the ioctl (stuff will go into setup_debug) > > > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > > index b112efc..06c5064 100644 > > --- a/Documentation/virtual/kvm/api.txt > > +++ b/Documentation/virtual/kvm/api.txt > > @@ -2604,7 +2604,7 @@ handled. > > 4.87 KVM_SET_GUEST_DEBUG > > > > Capability: KVM_CAP_SET_GUEST_DEBUG > > -Architectures: x86, s390, ppc > > +Architectures: x86, s390, ppc, arm64 > > Type: vcpu ioctl > > Parameters: struct kvm_guest_debug (in) > > Returns: 0 on success; -1 on error > > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > > index 5560f74..445933d 100644 > > --- a/arch/arm/kvm/arm.c > > +++ b/arch/arm/kvm/arm.c > > @@ -183,6 +183,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) > > case KVM_CAP_ARM_PSCI: > > case KVM_CAP_ARM_PSCI_0_2: > > case KVM_CAP_READONLY_MEM: > > + case KVM_CAP_SET_GUEST_DEBUG: > > r = 1; > > break; > > case KVM_CAP_COALESCED_MMIO: > > @@ -303,10 +304,21 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu) > > kvm_arm_set_running_vcpu(NULL); > > } > > > > +#define KVM_GUESTDBG_VALID (KVM_GUESTDBG_ENABLE) > > That makes me rather think that it is another flag. > > We(s390x) use VALID_GUESTDBG_FLAGS, what about that or KVM_GUESTDBG_VALID_MASK? > > > + > > int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, > > struct kvm_guest_debug *dbg) > > { > > - return -EINVAL; > > + if (dbg->control & KVM_GUESTDBG_ENABLE) { > > + if (dbg->control & ~KVM_GUESTDBG_VALID) > > + return -EINVAL; > > I'd move that check directly to the start of the function and bail out on any > unsupported flag. > > > + > > + vcpu->guest_debug = dbg->control; > > + } else { > > + /* If not enabled clear all flags */ > > + vcpu->guest_debug = 0; > > + } > > + return 0; > > } > > > > > > David > I don't see any follow-up from Alex on this, so I feel the need to "+1" all David's comments here. drew
Andrew Jones <drjones@redhat.com> writes: > On Wed, Apr 01, 2015 at 05:55:29PM +0200, David Hildenbrand wrote: >> > This commit adds a stub function to support the KVM_SET_GUEST_DEBUG >> > ioctl. Currently any operation flag will return EINVAL. Actual >> >> Well it won't return -EINVAL if you push in KVM_GUESTDBG_ENABLE or 0. >> >> "Any unsupported flag will return -EINVAL. For now, only KVM_GUESTDBG_ENABLE is >> supported, although it won't have any effects." >> >> > functionality will be added with further patches. >> > >> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>. >> > >> > --- >> > v2 >> > - simplified form of the ioctl (stuff will go into setup_debug) >> > >> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt >> > index b112efc..06c5064 100644 >> > --- a/Documentation/virtual/kvm/api.txt >> > +++ b/Documentation/virtual/kvm/api.txt >> > @@ -2604,7 +2604,7 @@ handled. >> > 4.87 KVM_SET_GUEST_DEBUG >> > >> > Capability: KVM_CAP_SET_GUEST_DEBUG >> > -Architectures: x86, s390, ppc >> > +Architectures: x86, s390, ppc, arm64 >> > Type: vcpu ioctl >> > Parameters: struct kvm_guest_debug (in) >> > Returns: 0 on success; -1 on error >> > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c >> > index 5560f74..445933d 100644 >> > --- a/arch/arm/kvm/arm.c >> > +++ b/arch/arm/kvm/arm.c >> > @@ -183,6 +183,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) >> > case KVM_CAP_ARM_PSCI: >> > case KVM_CAP_ARM_PSCI_0_2: >> > case KVM_CAP_READONLY_MEM: >> > + case KVM_CAP_SET_GUEST_DEBUG: >> > r = 1; >> > break; >> > case KVM_CAP_COALESCED_MMIO: >> > @@ -303,10 +304,21 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu) >> > kvm_arm_set_running_vcpu(NULL); >> > } >> > >> > +#define KVM_GUESTDBG_VALID (KVM_GUESTDBG_ENABLE) >> >> That makes me rather think that it is another flag. >> >> We(s390x) use VALID_GUESTDBG_FLAGS, what about that or KVM_GUESTDBG_VALID_MASK? >> >> > + >> > int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, >> > struct kvm_guest_debug *dbg) >> > { >> > - return -EINVAL; >> > + if (dbg->control & KVM_GUESTDBG_ENABLE) { >> > + if (dbg->control & ~KVM_GUESTDBG_VALID) >> > + return -EINVAL; >> >> I'd move that check directly to the start of the function and bail out on any >> unsupported flag. >> >> > + >> > + vcpu->guest_debug = dbg->control; >> > + } else { >> > + /* If not enabled clear all flags */ >> > + vcpu->guest_debug = 0; >> > + } >> > + return 0; >> > } >> > >> > >> >> David >> > > I don't see any follow-up from Alex on this, so I feel the need to > "+1" all David's comments here. Yeah they make sense. Will do.
On Tue, Mar 31, 2015 at 04:08:02PM +0100, Alex Bennée wrote: > This commit adds a stub function to support the KVM_SET_GUEST_DEBUG > ioctl. Currently any operation flag will return EINVAL. Actual > functionality will be added with further patches. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>. > > --- > v2 > - simplified form of the ioctl (stuff will go into setup_debug) > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > index b112efc..06c5064 100644 > --- a/Documentation/virtual/kvm/api.txt > +++ b/Documentation/virtual/kvm/api.txt > @@ -2604,7 +2604,7 @@ handled. > 4.87 KVM_SET_GUEST_DEBUG > > Capability: KVM_CAP_SET_GUEST_DEBUG > -Architectures: x86, s390, ppc > +Architectures: x86, s390, ppc, arm64 > Type: vcpu ioctl > Parameters: struct kvm_guest_debug (in) > Returns: 0 on success; -1 on error > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > index 5560f74..445933d 100644 > --- a/arch/arm/kvm/arm.c > +++ b/arch/arm/kvm/arm.c > @@ -183,6 +183,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) > case KVM_CAP_ARM_PSCI: > case KVM_CAP_ARM_PSCI_0_2: > case KVM_CAP_READONLY_MEM: > + case KVM_CAP_SET_GUEST_DEBUG: > r = 1; > break; shouldn't you wait with advertising this capability until you've implemented support for it? Thanks, -Christoffer
> On Tue, Mar 31, 2015 at 04:08:02PM +0100, Alex Bennée wrote: > > This commit adds a stub function to support the KVM_SET_GUEST_DEBUG > > ioctl. Currently any operation flag will return EINVAL. Actual > > functionality will be added with further patches. > > > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>. > > > > --- > > v2 > > - simplified form of the ioctl (stuff will go into setup_debug) > > > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > > index b112efc..06c5064 100644 > > --- a/Documentation/virtual/kvm/api.txt > > +++ b/Documentation/virtual/kvm/api.txt > > @@ -2604,7 +2604,7 @@ handled. > > 4.87 KVM_SET_GUEST_DEBUG > > > > Capability: KVM_CAP_SET_GUEST_DEBUG > > -Architectures: x86, s390, ppc > > +Architectures: x86, s390, ppc, arm64 > > Type: vcpu ioctl > > Parameters: struct kvm_guest_debug (in) > > Returns: 0 on success; -1 on error > > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > > index 5560f74..445933d 100644 > > --- a/arch/arm/kvm/arm.c > > +++ b/arch/arm/kvm/arm.c > > @@ -183,6 +183,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) > > case KVM_CAP_ARM_PSCI: > > case KVM_CAP_ARM_PSCI_0_2: > > case KVM_CAP_READONLY_MEM: > > + case KVM_CAP_SET_GUEST_DEBUG: > > r = 1; > > break; > > shouldn't you wait with advertising this capability until you've > implemented support for it? > I think this would work for now, however it's not very practical - in the end one has to sense which debug flags are actually supported. Question is if he wants to add initial support and extend functionality and flags with each patch or enable the whole set of features in one shot at the end. Doing the latter seems more practicable to me (especially as the debug features are added in the same patch series). David
David Hildenbrand <dahi@linux.vnet.ibm.com> writes: >> On Tue, Mar 31, 2015 at 04:08:02PM +0100, Alex Bennée wrote: >> > This commit adds a stub function to support the KVM_SET_GUEST_DEBUG >> > ioctl. Currently any operation flag will return EINVAL. Actual >> > functionality will be added with further patches. >> > >> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>. >> > >> > --- >> > v2 >> > - simplified form of the ioctl (stuff will go into setup_debug) >> > >> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt >> > index b112efc..06c5064 100644 >> > --- a/Documentation/virtual/kvm/api.txt >> > +++ b/Documentation/virtual/kvm/api.txt >> > @@ -2604,7 +2604,7 @@ handled. >> > 4.87 KVM_SET_GUEST_DEBUG >> > >> > Capability: KVM_CAP_SET_GUEST_DEBUG >> > -Architectures: x86, s390, ppc >> > +Architectures: x86, s390, ppc, arm64 >> > Type: vcpu ioctl >> > Parameters: struct kvm_guest_debug (in) >> > Returns: 0 on success; -1 on error >> > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c >> > index 5560f74..445933d 100644 >> > --- a/arch/arm/kvm/arm.c >> > +++ b/arch/arm/kvm/arm.c >> > @@ -183,6 +183,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) >> > case KVM_CAP_ARM_PSCI: >> > case KVM_CAP_ARM_PSCI_0_2: >> > case KVM_CAP_READONLY_MEM: >> > + case KVM_CAP_SET_GUEST_DEBUG: >> > r = 1; >> > break; >> >> shouldn't you wait with advertising this capability until you've >> implemented support for it? >> > > I think this would work for now, however it's not very practical > - in the end one has to sense which debug flags are actually supported. > > Question is if he wants to add initial support and extend functionality and > flags with each patch or enable the whole set of features in one shot at the > end. This is what in effect I was doing. Testing each feature in turn and ensuring it failed gracefully when kernel features where not present (both missing the capability or returning EINVAL). > Doing the latter seems more practicable to me (especially as the debug features > are added in the same patch series). Well in practice the whole series will go in together so this is only really relevant to what happens when you bisect the series. > > David
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt index b112efc..06c5064 100644 --- a/Documentation/virtual/kvm/api.txt +++ b/Documentation/virtual/kvm/api.txt @@ -2604,7 +2604,7 @@ handled. 4.87 KVM_SET_GUEST_DEBUG Capability: KVM_CAP_SET_GUEST_DEBUG -Architectures: x86, s390, ppc +Architectures: x86, s390, ppc, arm64 Type: vcpu ioctl Parameters: struct kvm_guest_debug (in) Returns: 0 on success; -1 on error diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index 5560f74..445933d 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -183,6 +183,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) case KVM_CAP_ARM_PSCI: case KVM_CAP_ARM_PSCI_0_2: case KVM_CAP_READONLY_MEM: + case KVM_CAP_SET_GUEST_DEBUG: r = 1; break; case KVM_CAP_COALESCED_MMIO: @@ -303,10 +304,21 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu) kvm_arm_set_running_vcpu(NULL); } +#define KVM_GUESTDBG_VALID (KVM_GUESTDBG_ENABLE) + int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, struct kvm_guest_debug *dbg) { - return -EINVAL; + if (dbg->control & KVM_GUESTDBG_ENABLE) { + if (dbg->control & ~KVM_GUESTDBG_VALID) + return -EINVAL; + + vcpu->guest_debug = dbg->control; + } else { + /* If not enabled clear all flags */ + vcpu->guest_debug = 0; + } + return 0; }
This commit adds a stub function to support the KVM_SET_GUEST_DEBUG ioctl. Currently any operation flag will return EINVAL. Actual functionality will be added with further patches. Signed-off-by: Alex Bennée <alex.bennee@linaro.org>. --- v2 - simplified form of the ioctl (stuff will go into setup_debug)