[PATCH v3] KVM: Specify byte order for KVM_EXIT_MMIO

[PATCH v3] KVM: Specify byte order for KVM_EXIT_MMIO

[Christoffer Dall]

The KVM API documentation is not clear about the semantics of the data
field on the mmio struct on the kvm_run struct.

This has become problematic when supporting ARM guests on big-endian
host systems with guests of both endianness types, because it is unclear
how the data should be exported to user space.

This should not break with existing implementations as all supported
existing implementations of known user space applications (QEMU and
kvmtools for virtio) only support default endianness of the
architectures on the host side.

Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
Changes [v2 - v3]:
 - Change the semantics and wordings as per Scott's, Avi's, and Ben's
   suggestions.

Changes [v1 - v2]:
 - s/host kernel should/host user space should/

 Documentation/virtual/kvm/api.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 366bf4b..e11f09d 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2565,6 +2565,10 @@ executed a memory-mapped I/O instruction which could not be satisfied
 by kvm.  The 'data' member contains the written data if 'is_write' is
 true, and should be filled by application code otherwise.
 
+The 'data' member contains, in its first 'len' bytes, the value as it would
+appear if the VCPU performed a load or store of the appropriate width directly
+to the byte array.
+
 NOTE: For KVM_EXIT_IO, KVM_EXIT_MMIO, KVM_EXIT_OSI, KVM_EXIT_DCR,
       KVM_EXIT_PAPR and KVM_EXIT_EPR the corresponding
 operations are complete (and guest state is consistent) only after userspace
-- 
1.8.5.2

Re: [PATCH v3] KVM: Specify byte order for KVM_EXIT_MMIO

[Alexander Graf]

On 01/28/2014 05:28 PM, Christoffer Dall wrote:
> The KVM API documentation is not clear about the semantics of the data
> field on the mmio struct on the kvm_run struct.
>
> This has become problematic when supporting ARM guests on big-endian
> host systems with guests of both endianness types, because it is unclear
> how the data should be exported to user space.
>
> This should not break with existing implementations as all supported
> existing implementations of known user space applications (QEMU and
> kvmtools for virtio) only support default endianness of the
> architectures on the host side.
>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

Acked-by: Alexander Graf <agraf@suse.de>


Alex

Re: [PATCH v3] KVM: Specify byte order for KVM_EXIT_MMIO

[Peter Maydell]

On 28 January 2014 16:28, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 366bf4b..e11f09d 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2565,6 +2565,10 @@ executed a memory-mapped I/O instruction which could not be satisfied
>  by kvm.  The 'data' member contains the written data if 'is_write' is
>  true, and should be filled by application code otherwise.
>
> +The 'data' member contains, in its first 'len' bytes, the value as it would
> +appear if the VCPU performed a load or store of the appropriate width directly
> +to the byte array.

I don't think you need my acked-by as such (though if you want it you
have it), but since I was one of the people arguing in this thread I guess
I should say specifically that I'm happy with this wording.

thanks
-- PMM

Re: [PATCH v3] KVM: Specify byte order for KVM_EXIT_MMIO

[Christoffer Dall]

On Sun, Feb 02, 2014 at 12:50:47PM +0000, Peter Maydell wrote:
> On 28 January 2014 16:28, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > index 366bf4b..e11f09d 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -2565,6 +2565,10 @@ executed a memory-mapped I/O instruction which could not be satisfied
> >  by kvm.  The 'data' member contains the written data if 'is_write' is
> >  true, and should be filled by application code otherwise.
> >
> > +The 'data' member contains, in its first 'len' bytes, the value as it would
> > +appear if the VCPU performed a load or store of the appropriate width directly
> > +to the byte array.
> 
> I don't think you need my acked-by as such (though if you want it you
> have it), but since I was one of the people arguing in this thread I guess
> I should say specifically that I'm happy with this wording.
> 
Paolo, Gleb,

Will you apply this one directly (assuming you also agree) or do you
want me to include it as part of a pull request?

Thanks,
-Christoffer

Re: [PATCH v3] KVM: Specify byte order for KVM_EXIT_MMIO

[Christoffer Dall]

On Tue, Jan 28, 2014 at 08:28:42AM -0800, Christoffer Dall wrote:
> The KVM API documentation is not clear about the semantics of the data
> field on the mmio struct on the kvm_run struct.
> 
> This has become problematic when supporting ARM guests on big-endian
> host systems with guests of both endianness types, because it is unclear
> how the data should be exported to user space.
> 
> This should not break with existing implementations as all supported
> existing implementations of known user space applications (QEMU and
> kvmtools for virtio) only support default endianness of the
> architectures on the host side.
> 
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> Changes [v2 - v3]:
>  - Change the semantics and wordings as per Scott's, Avi's, and Ben's
>    suggestions.
> 
> Changes [v1 - v2]:
>  - s/host kernel should/host user space should/
> 
>  Documentation/virtual/kvm/api.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 366bf4b..e11f09d 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2565,6 +2565,10 @@ executed a memory-mapped I/O instruction which could not be satisfied
>  by kvm.  The 'data' member contains the written data if 'is_write' is
>  true, and should be filled by application code otherwise.
>  
> +The 'data' member contains, in its first 'len' bytes, the value as it would
> +appear if the VCPU performed a load or store of the appropriate width directly
> +to the byte array.
> +
>  NOTE: For KVM_EXIT_IO, KVM_EXIT_MMIO, KVM_EXIT_OSI, KVM_EXIT_DCR,
>        KVM_EXIT_PAPR and KVM_EXIT_EPR the corresponding
>  operations are complete (and guest state is consistent) only after userspace
> -- 
> 1.8.5.2
> 

Any more comments on this one, or can it be applied?

Thanks,
-- 
Christoffer

Re: [PATCH v3] KVM: Specify byte order for KVM_EXIT_MMIO

[Marc Zyngier]

On Fri, Mar 14 2014 at  4:27:56 am GMT, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Tue, Jan 28, 2014 at 08:28:42AM -0800, Christoffer Dall wrote:
>> The KVM API documentation is not clear about the semantics of the data
>> field on the mmio struct on the kvm_run struct.
>> 
>> This has become problematic when supporting ARM guests on big-endian
>> host systems with guests of both endianness types, because it is unclear
>> how the data should be exported to user space.
>> 
>> This should not break with existing implementations as all supported
>> existing implementations of known user space applications (QEMU and
>> kvmtools for virtio) only support default endianness of the
>> architectures on the host side.
>> 
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>> ---
>> Changes [v2 - v3]:
>>  - Change the semantics and wordings as per Scott's, Avi's, and Ben's
>>    suggestions.
>> 
>> Changes [v1 - v2]:
>>  - s/host kernel should/host user space should/
>> 
>>  Documentation/virtual/kvm/api.txt | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index 366bf4b..e11f09d 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -2565,6 +2565,10 @@ executed a memory-mapped I/O instruction which could not be satisfied
>>  by kvm.  The 'data' member contains the written data if 'is_write' is
>>  true, and should be filled by application code otherwise.
>>  
>> +The 'data' member contains, in its first 'len' bytes, the value as it would
>> +appear if the VCPU performed a load or store of the appropriate width directly
>> +to the byte array.
>> +
>>  NOTE: For KVM_EXIT_IO, KVM_EXIT_MMIO, KVM_EXIT_OSI, KVM_EXIT_DCR,
>>        KVM_EXIT_PAPR and KVM_EXIT_EPR the corresponding
>>  operations are complete (and guest state is consistent) only after userspace
>> -- 
>> 1.8.5.2
>> 
>
> Any more comments on this one, or can it be applied?

I think we should go ahead and apply this. Having dark corners in the
ABI is not a very good thing, and bi-endianness seems to be quite
fashionnable these days... ;-)

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH v3] KVM: Specify byte order for KVM_EXIT_MMIO

[Paolo Bonzini]

Il 28/01/2014 17:28, Christoffer Dall ha scritto:
> The KVM API documentation is not clear about the semantics of the data
> field on the mmio struct on the kvm_run struct.
>
> This has become problematic when supporting ARM guests on big-endian
> host systems with guests of both endianness types, because it is unclear
> how the data should be exported to user space.
>
> This should not break with existing implementations as all supported
> existing implementations of known user space applications (QEMU and
> kvmtools for virtio) only support default endianness of the
> architectures on the host side.
>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> Changes [v2 - v3]:
>  - Change the semantics and wordings as per Scott's, Avi's, and Ben's
>    suggestions.
>
> Changes [v1 - v2]:
>  - s/host kernel should/host user space should/
>
>  Documentation/virtual/kvm/api.txt | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 366bf4b..e11f09d 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2565,6 +2565,10 @@ executed a memory-mapped I/O instruction which could not be satisfied
>  by kvm.  The 'data' member contains the written data if 'is_write' is
>  true, and should be filled by application code otherwise.
>
> +The 'data' member contains, in its first 'len' bytes, the value as it would
> +appear if the VCPU performed a load or store of the appropriate width directly
> +to the byte array.
> +
>  NOTE: For KVM_EXIT_IO, KVM_EXIT_MMIO, KVM_EXIT_OSI, KVM_EXIT_DCR,
>        KVM_EXIT_PAPR and KVM_EXIT_EPR the corresponding
>  operations are complete (and guest state is consistent) only after userspace
>

Applied, thanks!

Paolo