Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling
On 05/28/2013 07:12:32 PM, Alexey Kardashevskiy wrote: On 05/29/2013 09:35 AM, Scott Wood wrote: On 05/28/2013 06:30:40 PM, Alexey Kardashevskiy wrote: @@ -939,6 +940,9 @@ struct kvm_s390_ucas_mapping { #define KVM_GET_DEVICE_ATTR _IOW(KVMIO, 0xe2, struct kvm_device_attr) #define KVM_HAS_DEVICE_ATTR _IOW(KVMIO, 0xe3, struct kvm_device_attr) +/* ioctl for SPAPR TCE IOMMU */ +#define KVM_CREATE_SPAPR_TCE_IOMMU _IOW(KVMIO, 0xe4, struct kvm_create_spapr_tce_iommu) Shouldn't this go under the vm ioctl section? The KVM_CREATE_SPAPR_TCE_IOMMU ioctl (the version for emulated devices) is in this section so I decided to keep them together. Wrong? You decided to keep KVM_CREATE_SPAPR_TCE_IOMMU together with KVM_CREATE_SPAPR_TCE_IOMMU? Yes. Sigh. That's the same thing repeated. There's only one IOCTL. Nothing is being kept together. Sorry, I meant this ioctl - KVM_CREATE_SPAPR_TCE. But you didn't put it in the same section as KVM_CREATE_SPAPR_TCE. 0xe0 begins a different section. -Scott -- To unsubscribe from this list: send the line unsubscribe kvm-ppc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling
On 05/30/2013 06:05 AM, Scott Wood wrote: On 05/28/2013 07:12:32 PM, Alexey Kardashevskiy wrote: On 05/29/2013 09:35 AM, Scott Wood wrote: On 05/28/2013 06:30:40 PM, Alexey Kardashevskiy wrote: @@ -939,6 +940,9 @@ struct kvm_s390_ucas_mapping { #define KVM_GET_DEVICE_ATTR _IOW(KVMIO, 0xe2, struct kvm_device_attr) #define KVM_HAS_DEVICE_ATTR _IOW(KVMIO, 0xe3, struct kvm_device_attr) +/* ioctl for SPAPR TCE IOMMU */ +#define KVM_CREATE_SPAPR_TCE_IOMMU _IOW(KVMIO, 0xe4, struct kvm_create_spapr_tce_iommu) Shouldn't this go under the vm ioctl section? The KVM_CREATE_SPAPR_TCE_IOMMU ioctl (the version for emulated devices) is in this section so I decided to keep them together. Wrong? You decided to keep KVM_CREATE_SPAPR_TCE_IOMMU together with KVM_CREATE_SPAPR_TCE_IOMMU? Yes. Sigh. That's the same thing repeated. There's only one IOCTL. Nothing is being kept together. Sorry, I meant this ioctl - KVM_CREATE_SPAPR_TCE. But you didn't put it in the same section as KVM_CREATE_SPAPR_TCE. 0xe0 begins a different section. It is not really obvious that there are sections as no comment defines those :) But yes, makes sense to move it up a bit and change the code to 0xad. -- Alexey -- To unsubscribe from this list: send the line unsubscribe kvm-ppc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling
On 05/29/2013 06:10:33 PM, Alexey Kardashevskiy wrote: On 05/30/2013 06:05 AM, Scott Wood wrote: On 05/28/2013 07:12:32 PM, Alexey Kardashevskiy wrote: On 05/29/2013 09:35 AM, Scott Wood wrote: On 05/28/2013 06:30:40 PM, Alexey Kardashevskiy wrote: @@ -939,6 +940,9 @@ struct kvm_s390_ucas_mapping { #define KVM_GET_DEVICE_ATTR _IOW(KVMIO, 0xe2, struct kvm_device_attr) #define KVM_HAS_DEVICE_ATTR _IOW(KVMIO, 0xe3, struct kvm_device_attr) +/* ioctl for SPAPR TCE IOMMU */ +#define KVM_CREATE_SPAPR_TCE_IOMMU _IOW(KVMIO, 0xe4, struct kvm_create_spapr_tce_iommu) Shouldn't this go under the vm ioctl section? The KVM_CREATE_SPAPR_TCE_IOMMU ioctl (the version for emulated devices) is in this section so I decided to keep them together. Wrong? You decided to keep KVM_CREATE_SPAPR_TCE_IOMMU together with KVM_CREATE_SPAPR_TCE_IOMMU? Yes. Sigh. That's the same thing repeated. There's only one IOCTL. Nothing is being kept together. Sorry, I meant this ioctl - KVM_CREATE_SPAPR_TCE. But you didn't put it in the same section as KVM_CREATE_SPAPR_TCE. 0xe0 begins a different section. It is not really obvious that there are sections as no comment defines those :) There is a comment /* ioctls for fds returned by KVM_CREATE_DEVICE */ Putting KVM_CREATE_DEVICE in there was mainly to avoid dealing with the ioctl number conflict mess in the vm-ioctl section, but at least that one is related to the device control API. :-) But yes, makes sense to move it up a bit and change the code to 0xad. 0xad is KVM_KVMCLOCK_CTRL -Scott -- To unsubscribe from this list: send the line unsubscribe kvm-ppc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling
On 05/30/2013 09:14 AM, Scott Wood wrote: On 05/29/2013 06:10:33 PM, Alexey Kardashevskiy wrote: On 05/30/2013 06:05 AM, Scott Wood wrote: On 05/28/2013 07:12:32 PM, Alexey Kardashevskiy wrote: On 05/29/2013 09:35 AM, Scott Wood wrote: On 05/28/2013 06:30:40 PM, Alexey Kardashevskiy wrote: @@ -939,6 +940,9 @@ struct kvm_s390_ucas_mapping { #define KVM_GET_DEVICE_ATTR _IOW(KVMIO, 0xe2, struct kvm_device_attr) #define KVM_HAS_DEVICE_ATTR _IOW(KVMIO, 0xe3, struct kvm_device_attr) +/* ioctl for SPAPR TCE IOMMU */ +#define KVM_CREATE_SPAPR_TCE_IOMMU _IOW(KVMIO, 0xe4, struct kvm_create_spapr_tce_iommu) Shouldn't this go under the vm ioctl section? The KVM_CREATE_SPAPR_TCE_IOMMU ioctl (the version for emulated devices) is in this section so I decided to keep them together. Wrong? You decided to keep KVM_CREATE_SPAPR_TCE_IOMMU together with KVM_CREATE_SPAPR_TCE_IOMMU? Yes. Sigh. That's the same thing repeated. There's only one IOCTL. Nothing is being kept together. Sorry, I meant this ioctl - KVM_CREATE_SPAPR_TCE. But you didn't put it in the same section as KVM_CREATE_SPAPR_TCE. 0xe0 begins a different section. It is not really obvious that there are sections as no comment defines those :) There is a comment /* ioctls for fds returned by KVM_CREATE_DEVICE */ Putting KVM_CREATE_DEVICE in there was mainly to avoid dealing with the ioctl number conflict mess in the vm-ioctl section, but at least that one is related to the device control API. :-) But yes, makes sense to move it up a bit and change the code to 0xad. 0xad is KVM_KVMCLOCK_CTRL That's it. I am _completely_ confused now. No system whatsoever :( What rule should I use in order to choose the number for my new ioctl? :) -- Alexey -- To unsubscribe from this list: send the line unsubscribe kvm-ppc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: PPC: Book3S: Add support for H_IPOLL and H_XIRR_X in XICS emulation
On 05/28/2013 07:41:18 PM, Benjamin Herrenschmidt wrote: On Tue, 2013-05-28 at 12:41 -0500, Scott Wood wrote: I believe Alex is staying far away from e-mail on his vacation. He's asked me to fill in for him while he's gone. The patch itself seems reasonable (though I don't know much about XICS, and do have one question...), but I'll leave it up to Gleb/Marcelo/Ben if it should go in for 3.10 and via which tree. I understand the desire to not have an incomplete ABI in a released version, but Linus is already grumbling about how much went into rc3, and you say the hcalls aren't currently used... Are they likely to be used in any timeframe in which we'd reasonably care about 3.10? Yes. I'd like to have them in. Their implementation is actually fairly trivial and they cannot be emulated by qemu if the rest of the XICS is in the kernel, so it's a problem. OK. Does it make more sense for you to take it as Paul suggested, or for Gleb or Marcelo to pick it up directly? + /* These requests don't have real-mode implementations at present */ + switch (req) { + case H_XIRR_X: + res = kvmppc_h_xirr(vcpu); + kvmppc_set_gpr(vcpu, 4, res); + kvmppc_set_gpr(vcpu, 5, get_tb()); + return rc; + case H_IPOLL: + rc = kvmppc_h_ipoll(vcpu, kvmppc_get_gpr(vcpu, 4)); + return rc; + } + /* Check for real mode returning too hard */ if (xics-real_mode) return kvmppc_xics_rm_complete(vcpu, req); Could you explain what's going on here relative to kvmppc_xics_rm_complete()? What does returning too hard mean, and why must rm_action not be checked for these hcalls? This is related to how we handle some hcalls in real mode as a fast path. The real-mode stuff cannot handle cases that require for example a re-emit of the interrupt, a reject, etc... so in some cases, it returns H_TOO_HARD which causes KVM to exit and try to handle the hcall again in kernel virtual mode. When doing so as the result of a XICS hcall, it sets a bit mask of tasks to handle in virtual mode (because it will have already partially done the operation, it cannot just re-play the whole hcall). So when real-mode is supported we must not just call the normal virtual mode version of the hcalls, we instead go to kvmppc_xics_rm_complete() to handle those tasks. However, for those 2 missing hcalls, we have no real mode implementation at all (we didn't bother, we will do that later if needed, it's purely a performance issue). So we need to fully handle them in virtual mode, and we know there will be no tasks to handle in rm_complete. Then rm_action should always be 0 for these hcalls, right? So there's no correctness reason to keep the hcalls in separate switch statements. You shave off a few cycles checking rm_action, at the cost of needing to change kvmppc_xics_hcall() if a real-mode version of these hcalls is ever done. -Scott -- To unsubscribe from this list: send the line unsubscribe kvm-ppc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: PPC: Book3S: Add support for H_IPOLL and H_XIRR_X in XICS emulation
On Wed, 2013-05-29 at 18:38 -0500, Scott Wood wrote: Yes. I'd like to have them in. Their implementation is actually fairly trivial and they cannot be emulated by qemu if the rest of the XICS is in the kernel, so it's a problem. OK. Does it make more sense for you to take it as Paul suggested, or for Gleb or Marcelo to pick it up directly? I'll take it. Then rm_action should always be 0 for these hcalls, right? So there's no correctness reason to keep the hcalls in separate switch statements. You shave off a few cycles checking rm_action, at the cost of needing to change kvmppc_xics_hcall() if a real-mode version of these hcalls is ever done. No, because rm_action will also be 0 if the hcall was fully done in real mode (which can happen, that's our fast path), in which case we do *NOT* want to to be re-done in virtual mode. That's why we always return whether rm_action is 0 or not when real-mode is enabled. Cheers, Ben. -- To unsubscribe from this list: send the line unsubscribe kvm-ppc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: PPC: Book3S: Add support for H_IPOLL and H_XIRR_X in XICS emulation
On 05/29/2013 06:57:32 PM, Benjamin Herrenschmidt wrote: On Wed, 2013-05-29 at 18:38 -0500, Scott Wood wrote: Yes. I'd like to have them in. Their implementation is actually fairly trivial and they cannot be emulated by qemu if the rest of the XICS is in the kernel, so it's a problem. OK. Does it make more sense for you to take it as Paul suggested, or for Gleb or Marcelo to pick it up directly? I'll take it. Acked-by: Scott Wood scottw...@freescale.com Then rm_action should always be 0 for these hcalls, right? So there's no correctness reason to keep the hcalls in separate switch statements. You shave off a few cycles checking rm_action, at the cost of needing to change kvmppc_xics_hcall() if a real-mode version of these hcalls is ever done. No, because rm_action will also be 0 if the hcall was fully done in real mode (which can happen, that's our fast path), in which case we do *NOT* want to to be re-done in virtual mode. That's why we always return whether rm_action is 0 or not when real-mode is enabled. Oh, I misread the code and thought the decision to return was based on the return value of kvmppc_xics_rm_complete. Sorry about that. :-( -Scott -- To unsubscribe from this list: send the line unsubscribe kvm-ppc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html