| Message ID | 1383905236-32741-3-git-send-email-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Il 08/11/2013 11:07, Marc Zyngier ha scritto: > Do the necessary byteswap when host and guest have different > views of the universe. Actually, the only case we need to take > care of is when the guest is BE. All the other cases are naturally > handled. > > Also be careful about endianness when the data is being memcopy-ed > from/to the run buffer. > > Acked-by: Christoffer Dall <christoffer.dall@linaro.org> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm/include/asm/kvm_emulate.h | 41 +++++++++++++++++ > arch/arm/kvm/mmio.c | 86 +++++++++++++++++++++++++++++++----- > arch/arm64/include/asm/kvm_emulate.h | 48 ++++++++++++++++++++ > 3 files changed, 164 insertions(+), 11 deletions(-) Christoffer and Marc, please coordinate so that arm+arm64 patch go only through one person. The conflict between your two pull requests was not really necessary. I'm pulling both anyway. Paolo > diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h > index a464e8d..8a6be05 100644 > --- a/arch/arm/include/asm/kvm_emulate.h > +++ b/arch/arm/include/asm/kvm_emulate.h > @@ -157,4 +157,45 @@ static inline u32 kvm_vcpu_hvc_get_imm(struct kvm_vcpu *vcpu) > return kvm_vcpu_get_hsr(vcpu) & HSR_HVC_IMM_MASK; > } > > +static inline bool kvm_vcpu_is_be(struct kvm_vcpu *vcpu) > +{ > + return !!(*vcpu_cpsr(vcpu) & PSR_E_BIT); > +} > + > +static inline unsigned long vcpu_data_guest_to_host(struct kvm_vcpu *vcpu, > + unsigned long data, > + unsigned int len) > +{ > + if (kvm_vcpu_is_be(vcpu)) { > + switch (len) { > + case 1: > + return data & 0xff; > + case 2: > + return be16_to_cpu(data & 0xffff); > + default: > + return be32_to_cpu(data); > + } > + } > + > + return data; /* Leave LE untouched */ > +} > + > +static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu, > + unsigned long data, > + unsigned int len) > +{ > + if (kvm_vcpu_is_be(vcpu)) { > + switch (len) { > + case 1: > + return data & 0xff; > + case 2: > + return cpu_to_be16(data & 0xffff); > + default: > + return cpu_to_be32(data); > + } > + } > + > + return data; /* Leave LE untouched */ > +} > + > #endif /* __ARM_KVM_EMULATE_H__ */ > diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c > index 0c25d94..4cb5a93 100644 > --- a/arch/arm/kvm/mmio.c > +++ b/arch/arm/kvm/mmio.c > @@ -23,6 +23,68 @@ > > #include "trace.h" > > +static void mmio_write_buf(char *buf, unsigned int len, unsigned long data) > +{ > + void *datap = NULL; > + union { > + u8 byte; > + u16 hword; > + u32 word; > + u64 dword; > + } tmp; > + > + switch (len) { > + case 1: > + tmp.byte = data; > + datap = &tmp.byte; > + break; > + case 2: > + tmp.hword = data; > + datap = &tmp.hword; > + break; > + case 4: > + tmp.word = data; > + datap = &tmp.word; > + break; > + case 8: > + tmp.dword = data; > + datap = &tmp.dword; > + break; > + } > + > + memcpy(buf, datap, len); > +} > + > +static unsigned long mmio_read_buf(char *buf, unsigned int len) > +{ > + unsigned long data = 0; > + union { > + u16 hword; > + u32 word; > + u64 dword; > + } tmp; > + > + switch (len) { > + case 1: > + data = buf[0]; > + break; > + case 2: > + memcpy(&tmp.hword, buf, len); > + data = tmp.hword; > + break; > + case 4: > + memcpy(&tmp.word, buf, len); > + data = tmp.word; > + break; > + case 8: > + memcpy(&tmp.dword, buf, len); > + data = tmp.dword; > + break; > + } > + > + return data; > +} > + > /** > * kvm_handle_mmio_return -- Handle MMIO loads after user space emulation > * @vcpu: The VCPU pointer > @@ -33,28 +95,27 @@ > */ > int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) > { > - unsigned long *dest; > + unsigned long data; > unsigned int len; > int mask; > > if (!run->mmio.is_write) { > - dest = vcpu_reg(vcpu, vcpu->arch.mmio_decode.rt); > - *dest = 0; > - > len = run->mmio.len; > if (len > sizeof(unsigned long)) > return -EINVAL; > > - memcpy(dest, run->mmio.data, len); > - > - trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr, > - *((u64 *)run->mmio.data)); > + data = mmio_read_buf(run->mmio.data, len); > > if (vcpu->arch.mmio_decode.sign_extend && > len < sizeof(unsigned long)) { > mask = 1U << ((len * 8) - 1); > - *dest = (*dest ^ mask) - mask; > + data = (data ^ mask) - mask; > } > + > + trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr, > + data); > + data = vcpu_data_host_to_guest(vcpu, data, len); > + *vcpu_reg(vcpu, vcpu->arch.mmio_decode.rt) = data; > } > > return 0; > @@ -105,6 +166,7 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run, > phys_addr_t fault_ipa) > { > struct kvm_exit_mmio mmio; > + unsigned long data; > unsigned long rt; > int ret; > > @@ -125,13 +187,15 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run, > } > > rt = vcpu->arch.mmio_decode.rt; > + data = vcpu_data_guest_to_host(vcpu, *vcpu_reg(vcpu, rt), mmio.len); > + > trace_kvm_mmio((mmio.is_write) ? KVM_TRACE_MMIO_WRITE : > KVM_TRACE_MMIO_READ_UNSATISFIED, > mmio.len, fault_ipa, > - (mmio.is_write) ? *vcpu_reg(vcpu, rt) : 0); > + (mmio.is_write) ? data : 0); > > if (mmio.is_write) > - memcpy(mmio.data, vcpu_reg(vcpu, rt), mmio.len); > + mmio_write_buf(mmio.data, mmio.len, data); > > if (vgic_handle_mmio(vcpu, run, &mmio)) > return 1; > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index eec0738..b016577 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -177,4 +177,52 @@ static inline u8 kvm_vcpu_trap_get_fault(const struct kvm_vcpu *vcpu) > return kvm_vcpu_get_hsr(vcpu) & ESR_EL2_FSC_TYPE; > } > > +static inline bool kvm_vcpu_is_be(struct kvm_vcpu *vcpu) > +{ > + if (vcpu_mode_is_32bit(vcpu)) > + return !!(*vcpu_cpsr(vcpu) & COMPAT_PSR_E_BIT); > + > + return !!(vcpu_sys_reg(vcpu, SCTLR_EL1) & (1 << 25)); > +} > + > +static inline unsigned long vcpu_data_guest_to_host(struct kvm_vcpu *vcpu, > + unsigned long data, > + unsigned int len) > +{ > + if (kvm_vcpu_is_be(vcpu)) { > + switch (len) { > + case 1: > + return data & 0xff; > + case 2: > + return be16_to_cpu(data & 0xffff); > + case 4: > + return be32_to_cpu(data & 0xffffffff); > + default: > + return be64_to_cpu(data); > + } > + } > + > + return data; /* Leave LE untouched */ > +} > + > +static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu, > + unsigned long data, > + unsigned int len) > +{ > + if (kvm_vcpu_is_be(vcpu)) { > + switch (len) { > + case 1: > + return data & 0xff; > + case 2: > + return cpu_to_be16(data & 0xffff); > + case 4: > + return cpu_to_be32(data & 0xffffffff); > + default: > + return cpu_to_be64(data); > + } > + } > + > + return data; /* Leave LE untouched */ > +} > + > #endif /* __ARM64_KVM_EMULATE_H__ */ > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 November 2013 03:04, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 08/11/2013 11:07, Marc Zyngier ha scritto: >> Do the necessary byteswap when host and guest have different >> views of the universe. Actually, the only case we need to take >> care of is when the guest is BE. All the other cases are naturally >> handled. >> >> Also be careful about endianness when the data is being memcopy-ed >> from/to the run buffer. >> >> Acked-by: Christoffer Dall <christoffer.dall@linaro.org> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> arch/arm/include/asm/kvm_emulate.h | 41 +++++++++++++++++ >> arch/arm/kvm/mmio.c | 86 +++++++++++++++++++++++++++++++----- >> arch/arm64/include/asm/kvm_emulate.h | 48 ++++++++++++++++++++ >> 3 files changed, 164 insertions(+), 11 deletions(-) > > Christoffer and Marc, please coordinate so that arm+arm64 patch go only > through one person. The conflict between your two pull requests was not > really necessary. > I don't think the same patch was in both pull requests, right? Is what you're saying that any patch that touches arm and arm64 should always come from one of us so that you reduce the chance of a merge conflict? There would still be the case where I carry those arm/arm64 patches but th arm64 changes conflict with those in Marc's tree, no? -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 11/11/2013 15:56, Christoffer Dall ha scritto: >> > Christoffer and Marc, please coordinate so that arm+arm64 patch go only >> > through one person. The conflict between your two pull requests was not >> > really necessary. >> > > I don't think the same patch was in both pull requests, right? No, this patch conflicted with "arm/arm64: KVM: PSCI: use MPIDR to identify a target CPU" from your tree. Both touched arch/arm/include/asm/kvm_emulate.h and arch/arm64/include/asm/kvm_emulate.h > Is what you're saying that any patch that touches arm and arm64 should > always come from one of us so that you reduce the chance of a merge > conflict? Yes---note that it doesn't have to be *always* the same person; whatever works best for you probably works best for me too. In this case all of them were written by Marc, it would have been even simpler for you to just give your Acked-by instead of picking up "arm/arm64: KVM: PSCI: use MPIDR to identify a target CPU". > There would still be the case where I carry those arm/arm64 > patches but th arm64 changes conflict with those in Marc's tree, no? Yes, that can still happen. Conflicts are not bad, only inconsistencies are. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 November 2013 07:10, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 11/11/2013 15:56, Christoffer Dall ha scritto: >>> > Christoffer and Marc, please coordinate so that arm+arm64 patch go only >>> > through one person. The conflict between your two pull requests was not >>> > really necessary. >>> > >> I don't think the same patch was in both pull requests, right? > > No, this patch conflicted with "arm/arm64: KVM: PSCI: use MPIDR to > identify a target CPU" from your tree. Both touched > arch/arm/include/asm/kvm_emulate.h and arch/arm64/include/asm/kvm_emulate.h > >> Is what you're saying that any patch that touches arm and arm64 should >> always come from one of us so that you reduce the chance of a merge >> conflict? > > Yes---note that it doesn't have to be *always* the same person; whatever > works best for you probably works best for me too. In this case all of > them were written by Marc, it would have been even simpler for you to > just give your Acked-by instead of picking up "arm/arm64: KVM: PSCI: use > MPIDR to identify a target CPU". I don't think it would have made much sense - that patch was part of a series that was touching mainly arch/arm/kvm/* and therefore I included it in my pull. It would have been strange to have a kvm-arm-next tree that included 75% of the functionality because Marc happens to have another patch that touches arch/arm and arch/arm64 and have two untestable trees until the merge window... > >> There would still be the case where I carry those arm/arm64 >> patches but the arm64 changes conflict with those in Marc's tree, no? > > Yes, that can still happen. Conflicts are not bad, only inconsistencies > are. > Not sure what you mean and where we could be more consistent to make life easier for you. You say it should always come from the same person, but not necessary always from the same person? Note: I have no problem giving my ack to patches or follow any procedure that makes it easier, but I thought these pull requests were quite clean (albeit a bit late). -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 11/11/2013 16:49, Christoffer Dall ha scritto: > I don't think it would have made much sense - that patch was part of a > series that was touching mainly arch/arm/kvm/* and therefore I > included it in my pull. It would have been strange to have a > kvm-arm-next tree that included 75% of the functionality because Marc > happens to have another patch that touches arch/arm and arch/arm64 and > have two untestable trees until the merge window... Yes, I found the original series now at http://thread.gmane.org/gmane.linux.ports.arm.kernel/274722/. BTW, why did the arm/arm64 patch move from patch 1 in Marc's post to patch 4 here? Also, the description says "this also requires a patch to kvmtool so the generated DT matches the expectations of the guest (posted separately)". Does this apply to QEMU as well? If so, can you please point me to the QEMU patch? How does this patch affect guest ABI, and is guest ABI not yet considered stable for KVM ARM? Sorry for the question storm. :) >>> There would still be the case where I carry those arm/arm64 >>> patches but the arm64 changes conflict with those in Marc's tree, no? >> >> Yes, that can still happen. Conflicts are not bad, only inconsistencies >> are. > > Not sure what you mean and where we could be more consistent to make > life easier for you. You say it should always come from the same > person, but not necessary always from the same person? > > Note: I have no problem giving my ack to patches or follow any > procedure that makes it easier, but I thought these pull requests were > quite clean (albeit a bit late). The pull requests were clean and my life wasn't complicated much... On the other hand I'm trying to understand if there's something that can be improved because the conflict surprised me. Right now, in fact, it's not even entirely clear to me why ARM and ARM64 have separate maintainers. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 November 2013 17:56, Paolo Bonzini <pbonzini@redhat.com> wrote: > BTW, why did the arm/arm64 patch move from patch 1 in Marc's post to > patch 4 here? Also, the description says "this also requires a patch to > kvmtool so the generated DT matches the expectations of the guest > (posted separately)". Does this apply to QEMU as well? If so, can you > please point me to the QEMU patch? How does this patch affect guest > ABI, and is guest ABI not yet considered stable for KVM ARM? QEMU currently insists on 4 CPUs max for A15, so the question doesn't come up in that case. -- PMM -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 11/11/2013 19:03, Peter Maydell ha scritto: >> How does this patch affect guest >> ABI, and is guest ABI not yet considered stable for KVM ARM? > > QEMU currently insists on 4 CPUs max for A15, so the > question doesn't come up in that case. Still, does the guest ABI not matter only for kvmtool, or also for QEMU? Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Paolo, On 11/11/13 17:56, Paolo Bonzini wrote: > Il 11/11/2013 16:49, Christoffer Dall ha scritto: >> I don't think it would have made much sense - that patch was part of a >> series that was touching mainly arch/arm/kvm/* and therefore I >> included it in my pull. It would have been strange to have a >> kvm-arm-next tree that included 75% of the functionality because Marc >> happens to have another patch that touches arch/arm and arch/arm64 and >> have two untestable trees until the merge window... > > Yes, I found the original series now at > http://thread.gmane.org/gmane.linux.ports.arm.kernel/274722/. > > BTW, why did the arm/arm64 patch move from patch 1 in Marc's post to > patch 4 here? Also, the description says "this also requires a patch to > kvmtool so the generated DT matches the expectations of the guest > (posted separately)". Does this apply to QEMU as well? If so, can you > please point me to the QEMU patch? How does this patch affect guest > ABI, and is guest ABI not yet considered stable for KVM ARM? > > Sorry for the question storm. :) There is no QEMU patch so far, as Peter (CC-ed) doesn't want to support such a configuration and prefers to stick to a setups for which an actual platform exist in QEMU. This doesn't really change the ABI: - Configurations with more than 4 CPUs were rejected until now (KVM only dealt with a single cluster), and are now accepted - CPUs 4 to 7 are part of a second cluster, which is reflected in the MPIDR register, just like on HW. - The kvmtool patch only deals with DT generation, which was buggy and didn't deal properly with multiple clusters. So anything that was working before still is, and things that were wrongly advertised as working are now fixed. All in all, this patch series is more a set of bug-fixes than anything else. >>>> There would still be the case where I carry those arm/arm64 >>>> patches but the arm64 changes conflict with those in Marc's tree, no? >>> >>> Yes, that can still happen. Conflicts are not bad, only inconsistencies >>> are. >> >> Not sure what you mean and where we could be more consistent to make >> life easier for you. You say it should always come from the same >> person, but not necessary always from the same person? >> >> Note: I have no problem giving my ack to patches or follow any >> procedure that makes it easier, but I thought these pull requests were >> quite clean (albeit a bit late). > > The pull requests were clean and my life wasn't complicated much... On > the other hand I'm trying to understand if there's something that can be > improved because the conflict surprised me. Right now, in fact, it's > not even entirely clear to me why ARM and ARM64 have separate maintainers. Mostly because arm64 was developed and merged before any kind of useful documentation was publicly available. As I've written most of the code, it was only logical that I'd assume responsibility for it. Christoffer and I are actually working quite well together, and I don't think there is much to improve, short of sharing a common git tree. And to be perfectly clear, I wouldn't mind if we were written down as co-maintainers for both ports... Cheers, M.
On 11 November 2013 09:56, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 11/11/2013 16:49, Christoffer Dall ha scritto: >> I don't think it would have made much sense - that patch was part of a >> series that was touching mainly arch/arm/kvm/* and therefore I >> included it in my pull. It would have been strange to have a >> kvm-arm-next tree that included 75% of the functionality because Marc >> happens to have another patch that touches arch/arm and arch/arm64 and >> have two untestable trees until the merge window... > > Yes, I found the original series now at > http://thread.gmane.org/gmane.linux.ports.arm.kernel/274722/. > > BTW, why did the arm/arm64 patch move from patch 1 in Marc's post to > patch 4 here? hmm, probably I just goofed something up when exporting the mbox from mutt - it made sense for me the psci part was the last one as well I guess, but I can't say I applied my brain to the reordering. > Also, the description says "this also requires a patch to > kvmtool so the generated DT matches the expectations of the guest > (posted separately)". Does this apply to QEMU as well? If so, can you > please point me to the QEMU patch? How does this patch affect guest > ABI, and is guest ABI not yet considered stable for KVM ARM? > > Sorry for the question storm. :) > >>>> There would still be the case where I carry those arm/arm64 >>>> patches but the arm64 changes conflict with those in Marc's tree, no? >>> >>> Yes, that can still happen. Conflicts are not bad, only inconsistencies >>> are. >> >> Not sure what you mean and where we could be more consistent to make >> life easier for you. You say it should always come from the same >> person, but not necessary always from the same person? >> >> Note: I have no problem giving my ack to patches or follow any >> procedure that makes it easier, but I thought these pull requests were >> quite clean (albeit a bit late). > > The pull requests were clean and my life wasn't complicated much... On > the other hand I'm trying to understand if there's something that can be > improved because the conflict surprised me. Right now, in fact, it's > not even entirely clear to me why ARM and ARM64 have separate maintainers. > Well, KVM/ARM was my thing originally, I was, and am, the maintainer when it was merged into Linux, then Marc started doing a lot of work on there, and he did the ARM64 port and then we ended up this way. -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 November 2013 10:26, Marc Zyngier <marc.zyngier@arm.com> wrote: > Paolo, > > On 11/11/13 17:56, Paolo Bonzini wrote: >> Il 11/11/2013 16:49, Christoffer Dall ha scritto: >>> I don't think it would have made much sense - that patch was part of a >>> series that was touching mainly arch/arm/kvm/* and therefore I >>> included it in my pull. It would have been strange to have a >>> kvm-arm-next tree that included 75% of the functionality because Marc >>> happens to have another patch that touches arch/arm and arch/arm64 and >>> have two untestable trees until the merge window... >> >> Yes, I found the original series now at >> http://thread.gmane.org/gmane.linux.ports.arm.kernel/274722/. >> >> BTW, why did the arm/arm64 patch move from patch 1 in Marc's post to >> patch 4 here? Also, the description says "this also requires a patch to >> kvmtool so the generated DT matches the expectations of the guest >> (posted separately)". Does this apply to QEMU as well? If so, can you >> please point me to the QEMU patch? How does this patch affect guest >> ABI, and is guest ABI not yet considered stable for KVM ARM? >> >> Sorry for the question storm. :) > > There is no QEMU patch so far, as Peter (CC-ed) doesn't want to support > such a configuration and prefers to stick to a setups for which an > actual platform exist in QEMU. > > This doesn't really change the ABI: > - Configurations with more than 4 CPUs were rejected until now (KVM only > dealt with a single cluster), and are now accepted > - CPUs 4 to 7 are part of a second cluster, which is reflected in the > MPIDR register, just like on HW. > - The kvmtool patch only deals with DT generation, which was buggy and > didn't deal properly with multiple clusters. > > So anything that was working before still is, and things that were > wrongly advertised as working are now fixed. All in all, this patch > series is more a set of bug-fixes than anything else. > >>>>> There would still be the case where I carry those arm/arm64 >>>>> patches but the arm64 changes conflict with those in Marc's tree, no? >>>> >>>> Yes, that can still happen. Conflicts are not bad, only inconsistencies >>>> are. >>> >>> Not sure what you mean and where we could be more consistent to make >>> life easier for you. You say it should always come from the same >>> person, but not necessary always from the same person? >>> >>> Note: I have no problem giving my ack to patches or follow any >>> procedure that makes it easier, but I thought these pull requests were >>> quite clean (albeit a bit late). >> >> The pull requests were clean and my life wasn't complicated much... On >> the other hand I'm trying to understand if there's something that can be >> improved because the conflict surprised me. Right now, in fact, it's >> not even entirely clear to me why ARM and ARM64 have separate maintainers. > > Mostly because arm64 was developed and merged before any kind of useful > documentation was publicly available. As I've written most of the code, > it was only logical that I'd assume responsibility for it. > > Christoffer and I are actually working quite well together, and I don't > think there is much to improve, short of sharing a common git tree. And > to be perfectly clear, I wouldn't mind if we were written down as > co-maintainers for both ports... > I completely agree, I feel the collaboration works well too, and we can change the git workflow a bit and list us both as co-maintainers if required. It seems this is how it effectively works today, I don't merge anything unless Marc gives his ack and I believe it's the other way around too. -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 11/11/2013 19:26, Marc Zyngier ha scritto: >> > The pull requests were clean and my life wasn't complicated much... On >> > the other hand I'm trying to understand if there's something that can be >> > improved because the conflict surprised me. Right now, in fact, it's >> > not even entirely clear to me why ARM and ARM64 have separate maintainers. > Mostly because arm64 was developed and merged before any kind of useful > documentation was publicly available. As I've written most of the code, > it was only logical that I'd assume responsibility for it. That was my understanding as well. > Christoffer and I are actually working quite well together, and I don't > think there is much to improve, short of sharing a common git tree. And > to be perfectly clear, I wouldn't mind if we were written down as > co-maintainers for both ports... Then go for it. :) Send a patch to MAINTAINERS, get an Acked-by from Christoffer and I'll apply it. Gleb and I share the git tree and hand it off "formally" by email every 1 or 2 weeks to the other person. After the email is sent, the sender should no longer push to the shared tree. This however is by no means the only way to proceed, having separate trees and sending separate pull requests works well too. I would not mind the occasional conflict, and I'd be hardly surprised. Thanks, Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 11, 2013 at 07:41:30PM +0100, Paolo Bonzini wrote: > Il 11/11/2013 19:26, Marc Zyngier ha scritto: > >> > The pull requests were clean and my life wasn't complicated much... On > >> > the other hand I'm trying to understand if there's something that can be > >> > improved because the conflict surprised me. Right now, in fact, it's > >> > not even entirely clear to me why ARM and ARM64 have separate maintainers. > > Mostly because arm64 was developed and merged before any kind of useful > > documentation was publicly available. As I've written most of the code, > > it was only logical that I'd assume responsibility for it. > > That was my understanding as well. > > > Christoffer and I are actually working quite well together, and I don't > > think there is much to improve, short of sharing a common git tree. And > > to be perfectly clear, I wouldn't mind if we were written down as > > co-maintainers for both ports... > > Then go for it. :) Send a patch to MAINTAINERS, get an Acked-by from > Christoffer and I'll apply it. > > Gleb and I share the git tree and hand it off "formally" by email every > 1 or 2 weeks to the other person. After the email is sent, the sender > should no longer push to the shared tree. This however is by no means > the only way to proceed, having separate trees and sending separate pull > requests works well too. I would not mind the occasional conflict, and > I'd be hardly surprised. I'd cast my vote (if I have one) towards the sharing a tree method. For those of us scrambling to get caught up with kvmarm, a reduction in the number of trees and branches we need to track would be a welcome change. drew -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/11/13 09:41, Andrew Jones wrote: > On Mon, Nov 11, 2013 at 07:41:30PM +0100, Paolo Bonzini wrote: >> Il 11/11/2013 19:26, Marc Zyngier ha scritto: >>>>> The pull requests were clean and my life wasn't complicated much... On >>>>> the other hand I'm trying to understand if there's something that can be >>>>> improved because the conflict surprised me. Right now, in fact, it's >>>>> not even entirely clear to me why ARM and ARM64 have separate maintainers. >>> Mostly because arm64 was developed and merged before any kind of useful >>> documentation was publicly available. As I've written most of the code, >>> it was only logical that I'd assume responsibility for it. >> >> That was my understanding as well. >> >>> Christoffer and I are actually working quite well together, and I don't >>> think there is much to improve, short of sharing a common git tree. And >>> to be perfectly clear, I wouldn't mind if we were written down as >>> co-maintainers for both ports... >> >> Then go for it. :) Send a patch to MAINTAINERS, get an Acked-by from >> Christoffer and I'll apply it. >> >> Gleb and I share the git tree and hand it off "formally" by email every >> 1 or 2 weeks to the other person. After the email is sent, the sender >> should no longer push to the shared tree. This however is by no means >> the only way to proceed, having separate trees and sending separate pull >> requests works well too. I would not mind the occasional conflict, and >> I'd be hardly surprised. > > I'd cast my vote (if I have one) towards the sharing a tree method. For > those of us scrambling to get caught up with kvmarm, a reduction in the > number of trees and branches we need to track would be a welcome change. Not sure what the benefit would be. We'd go from two trees with respectively x and y branches, to a single tree with x+y branches. Christoffer and I tend to work on separate topics, we track what the other does, and we make sure we don't overlap. And if we do, we shove the related patches in the same branch. Overall, whether or not we switch to co-maintainership, I don't expect our workflow to change much. M.
Il 12/11/2013 11:03, Marc Zyngier ha scritto: >> > >> > I'd cast my vote (if I have one) towards the sharing a tree method. For >> > those of us scrambling to get caught up with kvmarm, a reduction in the >> > number of trees and branches we need to track would be a welcome change. > Not sure what the benefit would be. We'd go from two trees with > respectively x and y branches, to a single tree with x+y branches. > > Christoffer and I tend to work on separate topics, we track what the > other does, and we make sure we don't overlap. And if we do, we shove > the related patches in the same branch. Overall, whether or not we > switch to co-maintainership, I don't expect our workflow to change much. Yes, I think your workflow is fine as is. Andrew, with two co-maintainers Christoffer and Marc would probably send more frequent pull requests. You're probably better off sending them patches based on kvm/next directly. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 November 2013 02:07, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 12/11/2013 11:03, Marc Zyngier ha scritto: >>> > >>> > I'd cast my vote (if I have one) towards the sharing a tree method. For >>> > those of us scrambling to get caught up with kvmarm, a reduction in the >>> > number of trees and branches we need to track would be a welcome change. >> Not sure what the benefit would be. We'd go from two trees with >> respectively x and y branches, to a single tree with x+y branches. >> >> Christoffer and I tend to work on separate topics, we track what the >> other does, and we make sure we don't overlap. And if we do, we shove >> the related patches in the same branch. Overall, whether or not we >> switch to co-maintainership, I don't expect our workflow to change much. > > Yes, I think your workflow is fine as is. > > Andrew, with two co-maintainers Christoffer and Marc would probably send > more frequent pull requests. You're probably better off sending them > patches based on kvm/next directly. > Or just ask us if you're working on a feature, and you want to know where to get the latest sources or what to send pull requests against. Usually, if it's 32-bit stuff, it's against kvm-arm-next in my tree, which is equivalent to kvm/next workflow wise. -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h index a464e8d..8a6be05 100644 --- a/arch/arm/include/asm/kvm_emulate.h +++ b/arch/arm/include/asm/kvm_emulate.h @@ -157,4 +157,45 @@ static inline u32 kvm_vcpu_hvc_get_imm(struct kvm_vcpu *vcpu) return kvm_vcpu_get_hsr(vcpu) & HSR_HVC_IMM_MASK; } +static inline bool kvm_vcpu_is_be(struct kvm_vcpu *vcpu) +{ + return !!(*vcpu_cpsr(vcpu) & PSR_E_BIT); +} + +static inline unsigned long vcpu_data_guest_to_host(struct kvm_vcpu *vcpu, + unsigned long data, + unsigned int len) +{ + if (kvm_vcpu_is_be(vcpu)) { + switch (len) { + case 1: + return data & 0xff; + case 2: + return be16_to_cpu(data & 0xffff); + default: + return be32_to_cpu(data); + } + } + + return data; /* Leave LE untouched */ +} + +static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu, + unsigned long data, + unsigned int len) +{ + if (kvm_vcpu_is_be(vcpu)) { + switch (len) { + case 1: + return data & 0xff; + case 2: + return cpu_to_be16(data & 0xffff); + default: + return cpu_to_be32(data); + } + } + + return data; /* Leave LE untouched */ +} + #endif /* __ARM_KVM_EMULATE_H__ */ diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c index 0c25d94..4cb5a93 100644 --- a/arch/arm/kvm/mmio.c +++ b/arch/arm/kvm/mmio.c @@ -23,6 +23,68 @@ #include "trace.h" +static void mmio_write_buf(char *buf, unsigned int len, unsigned long data) +{ + void *datap = NULL; + union { + u8 byte; + u16 hword; + u32 word; + u64 dword; + } tmp; + + switch (len) { + case 1: + tmp.byte = data; + datap = &tmp.byte; + break; + case 2: + tmp.hword = data; + datap = &tmp.hword; + break; + case 4: + tmp.word = data; + datap = &tmp.word; + break; + case 8: + tmp.dword = data; + datap = &tmp.dword; + break; + } + + memcpy(buf, datap, len); +} + +static unsigned long mmio_read_buf(char *buf, unsigned int len) +{ + unsigned long data = 0; + union { + u16 hword; + u32 word; + u64 dword; + } tmp; + + switch (len) { + case 1: + data = buf[0]; + break; + case 2: + memcpy(&tmp.hword, buf, len); + data = tmp.hword; + break; + case 4: + memcpy(&tmp.word, buf, len); + data = tmp.word; + break; + case 8: + memcpy(&tmp.dword, buf, len); + data = tmp.dword; + break; + } + + return data; +} + /** * kvm_handle_mmio_return -- Handle MMIO loads after user space emulation * @vcpu: The VCPU pointer @@ -33,28 +95,27 @@ */ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) { - unsigned long *dest; + unsigned long data; unsigned int len; int mask; if (!run->mmio.is_write) { - dest = vcpu_reg(vcpu, vcpu->arch.mmio_decode.rt); - *dest = 0; - len = run->mmio.len; if (len > sizeof(unsigned long)) return -EINVAL; - memcpy(dest, run->mmio.data, len); - - trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr, - *((u64 *)run->mmio.data)); + data = mmio_read_buf(run->mmio.data, len); if (vcpu->arch.mmio_decode.sign_extend && len < sizeof(unsigned long)) { mask = 1U << ((len * 8) - 1); - *dest = (*dest ^ mask) - mask; + data = (data ^ mask) - mask; } + + trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr, + data); + data = vcpu_data_host_to_guest(vcpu, data, len); + *vcpu_reg(vcpu, vcpu->arch.mmio_decode.rt) = data; } return 0; @@ -105,6 +166,7 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run, phys_addr_t fault_ipa) { struct kvm_exit_mmio mmio; + unsigned long data; unsigned long rt; int ret; @@ -125,13 +187,15 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run, } rt = vcpu->arch.mmio_decode.rt; + data = vcpu_data_guest_to_host(vcpu, *vcpu_reg(vcpu, rt), mmio.len); + trace_kvm_mmio((mmio.is_write) ? KVM_TRACE_MMIO_WRITE : KVM_TRACE_MMIO_READ_UNSATISFIED, mmio.len, fault_ipa, - (mmio.is_write) ? *vcpu_reg(vcpu, rt) : 0); + (mmio.is_write) ? data : 0); if (mmio.is_write) - memcpy(mmio.data, vcpu_reg(vcpu, rt), mmio.len); + mmio_write_buf(mmio.data, mmio.len, data); if (vgic_handle_mmio(vcpu, run, &mmio)) return 1; diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h index eec0738..b016577 100644 --- a/arch/arm64/include/asm/kvm_emulate.h +++ b/arch/arm64/include/asm/kvm_emulate.h @@ -177,4 +177,52 @@ static inline u8 kvm_vcpu_trap_get_fault(const struct kvm_vcpu *vcpu) return kvm_vcpu_get_hsr(vcpu) & ESR_EL2_FSC_TYPE; } +static inline bool kvm_vcpu_is_be(struct kvm_vcpu *vcpu) +{ + if (vcpu_mode_is_32bit(vcpu)) + return !!(*vcpu_cpsr(vcpu) & COMPAT_PSR_E_BIT); + + return !!(vcpu_sys_reg(vcpu, SCTLR_EL1) & (1 << 25)); +} + +static inline unsigned long vcpu_data_guest_to_host(struct kvm_vcpu *vcpu, + unsigned long data, + unsigned int len) +{ + if (kvm_vcpu_is_be(vcpu)) { + switch (len) { + case 1: + return data & 0xff; + case 2: + return be16_to_cpu(data & 0xffff); + case 4: + return be32_to_cpu(data & 0xffffffff); + default: + return be64_to_cpu(data); + } + } + + return data; /* Leave LE untouched */ +} + +static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu, + unsigned long data, + unsigned int len) +{ + if (kvm_vcpu_is_be(vcpu)) { + switch (len) { + case 1: + return data & 0xff; + case 2: + return cpu_to_be16(data & 0xffff); + case 4: + return cpu_to_be32(data & 0xffffffff); + default: + return cpu_to_be64(data); + } + } + + return data; /* Leave LE untouched */ +} + #endif /* __ARM64_KVM_EMULATE_H__ */