diff mbox

[v3] KVM: Specify byte order for KVM_EXIT_MMIO

Message ID 1390926522-18267-1-git-send-email-christoffer.dall@linaro.org
State New, archived
Headers show

Commit Message

Christoffer Dall Jan. 28, 2014, 4:28 p.m. UTC
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(+)

Comments

Alexander Graf Jan. 29, 2014, 2:31 p.m. UTC | #1
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

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Maydell Feb. 2, 2014, 12:50 p.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoffer Dall Feb. 2, 2014, 9:02 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoffer Dall March 14, 2014, 4:27 a.m. UTC | #4
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,
Marc Zyngier March 17, 2014, 7 p.m. UTC | #5
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.
Paolo Bonzini March 29, 2014, 2:45 p.m. UTC | #6
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
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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