Switch 'requests' to be explicitly 64-bit and update BUILD_BUG_ON check to
use the size of "requests" instead of the hard-coded '32'.
That gives us a bit more room again for arch-specific requests as we
already ran out of space for x86 due to the hard-coded check.
Cc: Paolo Bonzini <[email protected]>
Cc: Radim Krčmář <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: KarimAllah Ahmed <[email protected]>
---
include/linux/kvm_host.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 6930c63..fe4f46b 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -129,7 +129,7 @@ static inline bool is_error_page(struct page *page)
#define KVM_REQUEST_ARCH_BASE 8
#define KVM_ARCH_REQ_FLAGS(nr, flags) ({ \
- BUILD_BUG_ON((unsigned)(nr) >= 32 - KVM_REQUEST_ARCH_BASE); \
+ BUILD_BUG_ON((unsigned)(nr) >= (sizeof(((struct kvm_vcpu *)0)->requests) * 8) - KVM_REQUEST_ARCH_BASE); \
(unsigned)(((nr) + KVM_REQUEST_ARCH_BASE) | (flags)); \
})
#define KVM_ARCH_REQ(nr) KVM_ARCH_REQ_FLAGS(nr, 0)
@@ -223,7 +223,7 @@ struct kvm_vcpu {
int vcpu_id;
int srcu_idx;
int mode;
- unsigned long requests;
+ u64 requests;
unsigned long guest_debug;
int pre_pcpu;
@@ -1122,7 +1122,7 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
* caller. Paired with the smp_mb__after_atomic in kvm_check_request.
*/
smp_wmb();
- set_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
+ set_bit(req & KVM_REQUEST_MASK, (void *)&vcpu->requests);
}
static inline bool kvm_request_pending(struct kvm_vcpu *vcpu)
@@ -1132,12 +1132,12 @@ static inline bool kvm_request_pending(struct kvm_vcpu *vcpu)
static inline bool kvm_test_request(int req, struct kvm_vcpu *vcpu)
{
- return test_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
+ return test_bit(req & KVM_REQUEST_MASK, (void *)&vcpu->requests);
}
static inline void kvm_clear_request(int req, struct kvm_vcpu *vcpu)
{
- clear_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
+ clear_bit(req & KVM_REQUEST_MASK, (void *)&vcpu->requests);
}
static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
--
2.7.4
On Sun, 2018-04-15 at 00:26 +0200, KarimAllah Ahmed wrote:
> Switch 'requests' to be explicitly 64-bit and update BUILD_BUG_ON check to
> use the size of "requests" instead of the hard-coded '32'.
>
> That gives us a bit more room again for arch-specific requests as we
> already ran out of space for x86 due to the hard-coded check.
>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Radim Krčmář <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: KarimAllah Ahmed <[email protected]>
> ---
> include/linux/kvm_host.h | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 6930c63..fe4f46b 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -129,7 +129,7 @@ static inline bool is_error_page(struct page *page)
> #define KVM_REQUEST_ARCH_BASE 8
>
> #define KVM_ARCH_REQ_FLAGS(nr, flags) ({ \
> - BUILD_BUG_ON((unsigned)(nr) >= 32 - KVM_REQUEST_ARCH_BASE); \
> + BUILD_BUG_ON((unsigned)(nr) >= (sizeof(((struct kvm_vcpu *)0)->requests) * 8) - KVM_REQUEST_ARCH_BASE); \
While looking at a completely unrelated code I realize that there
is FIELD_SIZEOF that is doing exactly what I did. Will use it in v2.
> (unsigned)(((nr) + KVM_REQUEST_ARCH_BASE) | (flags)); \
> })
> #define KVM_ARCH_REQ(nr) KVM_ARCH_REQ_FLAGS(nr, 0)
> @@ -223,7 +223,7 @@ struct kvm_vcpu {
> int vcpu_id;
> int srcu_idx;
> int mode;
> - unsigned long requests;
> + u64 requests;
> unsigned long guest_debug;
>
> int pre_pcpu;
> @@ -1122,7 +1122,7 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
> * caller. Paired with the smp_mb__after_atomic in kvm_check_request.
> */
> smp_wmb();
> - set_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
> + set_bit(req & KVM_REQUEST_MASK, (void *)&vcpu->requests);
> }
>
> static inline bool kvm_request_pending(struct kvm_vcpu *vcpu)
> @@ -1132,12 +1132,12 @@ static inline bool kvm_request_pending(struct kvm_vcpu *vcpu)
>
> static inline bool kvm_test_request(int req, struct kvm_vcpu *vcpu)
> {
> - return test_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
> + return test_bit(req & KVM_REQUEST_MASK, (void *)&vcpu->requests);
> }
>
> static inline void kvm_clear_request(int req, struct kvm_vcpu *vcpu)
> {
> - clear_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
> + clear_bit(req & KVM_REQUEST_MASK, (void *)&vcpu->requests);
> }
>
> static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
On Sun, Apr 15, 2018 at 6:12 AM, Raslan, KarimAllah <[email protected]> wrote:
> On Sun, 2018-04-15 at 00:26 +0200, KarimAllah Ahmed wrote:
>> Switch 'requests' to be explicitly 64-bit and update BUILD_BUG_ON check to
>> use the size of "requests" instead of the hard-coded '32'.
>>
>> That gives us a bit more room again for arch-specific requests as we
>> already ran out of space for x86 due to the hard-coded check.
>>
>> Cc: Paolo Bonzini <[email protected]>
>> Cc: Radim Krčmář <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: KarimAllah Ahmed <[email protected]>
I like it!
Reviewed-by: Jim Mattson <[email protected]>
On 15/04/2018 00:26, KarimAllah Ahmed wrote:
> Switch 'requests' to be explicitly 64-bit and update BUILD_BUG_ON check to
> use the size of "requests" instead of the hard-coded '32'.
>
> That gives us a bit more room again for arch-specific requests as we
> already ran out of space for x86 due to the hard-coded check.
>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Radim Krčmář <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: KarimAllah Ahmed <[email protected]>
I'm afraid architectures like ARM 32 need this to be conditional (using
Kconfig).
Thanks,
Paolo
> ---
> include/linux/kvm_host.h | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 6930c63..fe4f46b 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -129,7 +129,7 @@ static inline bool is_error_page(struct page *page)
> #define KVM_REQUEST_ARCH_BASE 8
>
> #define KVM_ARCH_REQ_FLAGS(nr, flags) ({ \
> - BUILD_BUG_ON((unsigned)(nr) >= 32 - KVM_REQUEST_ARCH_BASE); \
> + BUILD_BUG_ON((unsigned)(nr) >= (sizeof(((struct kvm_vcpu *)0)->requests) * 8) - KVM_REQUEST_ARCH_BASE); \
> (unsigned)(((nr) + KVM_REQUEST_ARCH_BASE) | (flags)); \
> })
> #define KVM_ARCH_REQ(nr) KVM_ARCH_REQ_FLAGS(nr, 0)
> @@ -223,7 +223,7 @@ struct kvm_vcpu {
> int vcpu_id;
> int srcu_idx;
> int mode;
> - unsigned long requests;
> + u64 requests;
> unsigned long guest_debug;
>
> int pre_pcpu;
> @@ -1122,7 +1122,7 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
> * caller. Paired with the smp_mb__after_atomic in kvm_check_request.
> */
> smp_wmb();
> - set_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
> + set_bit(req & KVM_REQUEST_MASK, (void *)&vcpu->requests);
> }
>
> static inline bool kvm_request_pending(struct kvm_vcpu *vcpu)
> @@ -1132,12 +1132,12 @@ static inline bool kvm_request_pending(struct kvm_vcpu *vcpu)
>
> static inline bool kvm_test_request(int req, struct kvm_vcpu *vcpu)
> {
> - return test_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
> + return test_bit(req & KVM_REQUEST_MASK, (void *)&vcpu->requests);
> }
>
> static inline void kvm_clear_request(int req, struct kvm_vcpu *vcpu)
> {
> - clear_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
> + clear_bit(req & KVM_REQUEST_MASK, (void *)&vcpu->requests);
> }
>
> static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
>
On Mon, 2018-04-16 at 18:28 +0200, Paolo Bonzini wrote:
> On 15/04/2018 00:26, KarimAllah Ahmed wrote:
> >
> > Switch 'requests' to be explicitly 64-bit and update BUILD_BUG_ON check to
> > use the size of "requests" instead of the hard-coded '32'.
> >
> > That gives us a bit more room again for arch-specific requests as we
> > already ran out of space for x86 due to the hard-coded check.
> >
> > Cc: Paolo Bonzini <[email protected]>
> > Cc: Radim Krčmář <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: KarimAllah Ahmed <[email protected]>
>
> I'm afraid architectures like ARM 32 need this to be conditional (using
> Kconfig).
Why would using a 64-bit 'requests' be a problem for ARM32? Are you
concerned about performance here or is there some symantic problem?
>
> Thanks,
>
> Paolo
>
> >
> > ---
> > include/linux/kvm_host.h | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 6930c63..fe4f46b 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -129,7 +129,7 @@ static inline bool is_error_page(struct page *page)
> > #define KVM_REQUEST_ARCH_BASE 8
> >
> > #define KVM_ARCH_REQ_FLAGS(nr, flags) ({ \
> > - BUILD_BUG_ON((unsigned)(nr) >= 32 - KVM_REQUEST_ARCH_BASE); \
> > + BUILD_BUG_ON((unsigned)(nr) >= (sizeof(((struct kvm_vcpu *)0)->requests) * 8) - KVM_REQUEST_ARCH_BASE); \
> > (unsigned)(((nr) + KVM_REQUEST_ARCH_BASE) | (flags)); \
> > })
> > #define KVM_ARCH_REQ(nr) KVM_ARCH_REQ_FLAGS(nr, 0)
> > @@ -223,7 +223,7 @@ struct kvm_vcpu {
> > int vcpu_id;
> > int srcu_idx;
> > int mode;
> > - unsigned long requests;
> > + u64 requests;
> > unsigned long guest_debug;
> >
> > int pre_pcpu;
> > @@ -1122,7 +1122,7 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
> > * caller. Paired with the smp_mb__after_atomic in kvm_check_request.
> > */
> > smp_wmb();
> > - set_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
> > + set_bit(req & KVM_REQUEST_MASK, (void *)&vcpu->requests);
> > }
> >
> > static inline bool kvm_request_pending(struct kvm_vcpu *vcpu)
> > @@ -1132,12 +1132,12 @@ static inline bool kvm_request_pending(struct kvm_vcpu *vcpu)
> >
> > static inline bool kvm_test_request(int req, struct kvm_vcpu *vcpu)
> > {
> > - return test_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
> > + return test_bit(req & KVM_REQUEST_MASK, (void *)&vcpu->requests);
> > }
> >
> > static inline void kvm_clear_request(int req, struct kvm_vcpu *vcpu)
> > {
> > - clear_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
> > + clear_bit(req & KVM_REQUEST_MASK, (void *)&vcpu->requests);
> > }
> >
> > static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
> >
>
>
Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
On 22/05/2018 17:42, Raslan, KarimAllah wrote:
> On Mon, 2018-04-16 at 18:28 +0200, Paolo Bonzini wrote:
>> On 15/04/2018 00:26, KarimAllah Ahmed wrote:
>>>
>>> Switch 'requests' to be explicitly 64-bit and update BUILD_BUG_ON check to
>>> use the size of "requests" instead of the hard-coded '32'.
>>>
>>> That gives us a bit more room again for arch-specific requests as we
>>> already ran out of space for x86 due to the hard-coded check.
>>>
>>> Cc: Paolo Bonzini <[email protected]>
>>> Cc: Radim Krčmář <[email protected]>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Signed-off-by: KarimAllah Ahmed <[email protected]>
>>
>> I'm afraid architectures like ARM 32 need this to be conditional (using
>> Kconfig).
>
> Why would using a 64-bit 'requests' be a problem for ARM32? Are you
> concerned about performance here or is there some symantic problem?
They don't support atomics on double-word data.
Paolo
On Tue, 2018-05-22 at 17:47 +0200, Paolo Bonzini wrote:
> On 22/05/2018 17:42, Raslan, KarimAllah wrote:
> >
> > On Mon, 2018-04-16 at 18:28 +0200, Paolo Bonzini wrote:
> > >
> > > On 15/04/2018 00:26, KarimAllah Ahmed wrote:
> > > >
> > > >
> > > > Switch 'requests' to be explicitly 64-bit and update BUILD_BUG_ON check to
> > > > use the size of "requests" instead of the hard-coded '32'.
> > > >
> > > > That gives us a bit more room again for arch-specific requests as we
> > > > already ran out of space for x86 due to the hard-coded check.
> > > >
> > > > Cc: Paolo Bonzini <[email protected]>
> > > > Cc: Radim Krčmář <[email protected]>
> > > > Cc: [email protected]
> > > > Cc: [email protected]
> > > > Signed-off-by: KarimAllah Ahmed <[email protected]>
> > >
> > > I'm afraid architectures like ARM 32 need this to be conditional (using
> > > Kconfig).
> >
> > Why would using a 64-bit 'requests' be a problem for ARM32? Are you
> > concerned about performance here or is there some symantic problem?
>
> They don't support atomics on double-word data.
But they support atomics on single words. Of which there are two.
We don't need atomic updates of the whole 64-bit quantity (a là
cmpxchg). Do we strictly need this to be atomic accross the 64-bit?
Looking at the use cases for "requests":
kvm_clear_request
kvm_test_request
kvm_request_pending
kvm_check_request
... and all of them would still work if the atomicity is only at the
word level, right?
>
> Paolo
>
Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
On 05/07/2018 14:22, Raslan, KarimAllah wrote:
>> They don't support atomics on double-word data.
> But they support atomics on single words. Of which there are two.
> We don't need atomic updates of the whole 64-bit quantity (a là
> cmpxchg). Do we strictly need this to be atomic accross the 64-bit?
>
> Looking at the use cases for "requests":
>
> kvm_clear_request
> kvm_test_request
> kvm_request_pending
> kvm_check_request
>
> ... and all of them would still work if the atomicity is only at the
> word level, right?
>
Good point! Then yes, requests can be made 64 bit wide.
Paolo
On Sun, Apr 15, 2018 at 12:26:44AM +0200, KarimAllah Ahmed wrote:
> Switch 'requests' to be explicitly 64-bit and update BUILD_BUG_ON check to
> use the size of "requests" instead of the hard-coded '32'.
>
> That gives us a bit more room again for arch-specific requests as we
> already ran out of space for x86 due to the hard-coded check.
>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Radim Krčmář <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: KarimAllah Ahmed <[email protected]>
> ---
> include/linux/kvm_host.h | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 6930c63..fe4f46b 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -129,7 +129,7 @@ static inline bool is_error_page(struct page *page)
> #define KVM_REQUEST_ARCH_BASE 8
>
> #define KVM_ARCH_REQ_FLAGS(nr, flags) ({ \
> - BUILD_BUG_ON((unsigned)(nr) >= 32 - KVM_REQUEST_ARCH_BASE); \
> + BUILD_BUG_ON((unsigned)(nr) >= (sizeof(((struct kvm_vcpu *)0)->requests) * 8) - KVM_REQUEST_ARCH_BASE); \
> (unsigned)(((nr) + KVM_REQUEST_ARCH_BASE) | (flags)); \
> })
> #define KVM_ARCH_REQ(nr) KVM_ARCH_REQ_FLAGS(nr, 0)
> @@ -223,7 +223,7 @@ struct kvm_vcpu {
> int vcpu_id;
> int srcu_idx;
> int mode;
> - unsigned long requests;
> + u64 requests;
The usual thing to do for bitmaps is something like:
#define KVM_REQUEST_NR (KVM_REQUEST_ARCH_BASE + KVM_REQUEST_ARCH_NR)
unsigned long requests[BITS_TO_LONGS(NR_KVM_REQUESTS)];
> unsigned long guest_debug;
>
> int pre_pcpu;
> @@ -1122,7 +1122,7 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
> * caller. Paired with the smp_mb__after_atomic in kvm_check_request.
> */
> smp_wmb();
> - set_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
> + set_bit(req & KVM_REQUEST_MASK, (void *)&vcpu->requests);
... which wouldn't require a void cast to make the bit API functions
happy (as these expect a pointer to the first unsigned long in the
bitmap).
Thanks,
Mark.
On Thu, 2018-07-05 at 14:51 +0100, Mark Rutland wrote:
> On Sun, Apr 15, 2018 at 12:26:44AM +0200, KarimAllah Ahmed wrote:
> >
> > Switch 'requests' to be explicitly 64-bit and update BUILD_BUG_ON check to
> > use the size of "requests" instead of the hard-coded '32'.
> >
> > That gives us a bit more room again for arch-specific requests as we
> > already ran out of space for x86 due to the hard-coded check.
> >
> > Cc: Paolo Bonzini <[email protected]>
> > Cc: Radim Krčmář <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: KarimAllah Ahmed <[email protected]>
> > ---
> > include/linux/kvm_host.h | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 6930c63..fe4f46b 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -129,7 +129,7 @@ static inline bool is_error_page(struct page *page)
> > #define KVM_REQUEST_ARCH_BASE 8
> >
> > #define KVM_ARCH_REQ_FLAGS(nr, flags) ({ \
> > - BUILD_BUG_ON((unsigned)(nr) >= 32 - KVM_REQUEST_ARCH_BASE); \
> > + BUILD_BUG_ON((unsigned)(nr) >= (sizeof(((struct kvm_vcpu *)0)->requests) * 8) - KVM_REQUEST_ARCH_BASE); \
> > (unsigned)(((nr) + KVM_REQUEST_ARCH_BASE) | (flags)); \
> > })
> > #define KVM_ARCH_REQ(nr) KVM_ARCH_REQ_FLAGS(nr, 0)
> > @@ -223,7 +223,7 @@ struct kvm_vcpu {
> > int vcpu_id;
> > int srcu_idx;
> > int mode;
> > - unsigned long requests;
> > + u64 requests;
>
> The usual thing to do for bitmaps is something like:
>
> #define KVM_REQUEST_NR (KVM_REQUEST_ARCH_BASE + KVM_REQUEST_ARCH_NR)
>
> unsigned long requests[BITS_TO_LONGS(NR_KVM_REQUESTS)];
>
> >
> > unsigned long guest_debug;
> >
> > int pre_pcpu;
> > @@ -1122,7 +1122,7 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
> > * caller. Paired with the smp_mb__after_atomic in kvm_check_request.
> > */
> > smp_wmb();
> > - set_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
> > + set_bit(req & KVM_REQUEST_MASK, (void *)&vcpu->requests);
>
> ... which wouldn't require a void cast to make the bit API functions
> happy (as these expect a pointer to the first unsigned long in the
> bitmap).
Ah, right! Good point.
Paolo,
Would you prefer to switch "requests" to a bitmap and
update kvm_request_pending to handle this?
>
> Thanks,
> Mark.
>
Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B