subject:"\[PATCH v1\] kvm\: Make vcpu\->requests as 64 bit bitmap"

[PATCH v1] kvm: Make vcpu->requests as 64 bit bitmap

2015-12-24 Thread Andrey Smetanin

Currently on x86 arch we has already 32 requests defined
so the newer request bits can't be placed inside
vcpu->requests(unsigned long) inside x86 32 bit system.
But we are going to add a new request in x86 arch
for Hyper-V tsc page support.

To solve the problem the patch replaces vcpu->requests by
bitmap with 64 bit length and uses bitmap API.

The patch consists of:
* announce kvm_vcpu_has_requests() to check whether vcpu has
requests
* announce kvm_vcpu_requests() to get vcpu requests pointer
* announce kvm_clear_request() to clear particular vcpu request
* replace if (vcpu->requests) by if (kvm_vcpu_has_requests(vcpu))
* replace clear_bit(req, vcpu->requests) by
 kvm_clear_request(req, vcpu)

Signed-off-by: Andrey Smetanin 
CC: Paolo Bonzini 
CC: Gleb Natapov 
CC: James Hogan 
CC: Paolo Bonzini 
CC: Paul Burton 
CC: Ralf Baechle 
CC: Alexander Graf 
CC: Christian Borntraeger 
CC: Cornelia Huck 
CC: linux-m...@linux-mips.org
CC: kvm-ppc@vger.kernel.org
CC: linux-s...@vger.kernel.org
CC: Roman Kagan 
CC: Denis V. Lunev 
CC: qemu-de...@nongnu.org

---
 arch/mips/kvm/emulate.c   |  4 +---
 arch/powerpc/kvm/book3s_pr.c  |  2 +-
 arch/powerpc/kvm/book3s_pr_papr.c |  2 +-
 arch/powerpc/kvm/booke.c  |  6 +++---
 arch/powerpc/kvm/powerpc.c|  6 +++---
 arch/powerpc/kvm/trace.h  |  2 +-
 arch/s390/kvm/kvm-s390.c  |  4 ++--
 arch/x86/kvm/vmx.c|  2 +-
 arch/x86/kvm/x86.c| 14 +++---
 include/linux/kvm_host.h  | 27 ++-
 10 files changed, 42 insertions(+), 27 deletions(-)

diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c
index 41b1b09..14aebe8 100644
--- a/arch/mips/kvm/emulate.c
+++ b/arch/mips/kvm/emulate.c
@@ -774,10 +774,8 @@ enum emulation_result kvm_mips_emul_wait(struct kvm_vcpu 
*vcpu)
 * We we are runnable, then definitely go off to user space to
 * check if any I/O interrupts are pending.
 */
-   if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) {
-   clear_bit(KVM_REQ_UNHALT, >requests);
+   if (kvm_check_request(KVM_REQ_UNHALT, vcpu))
vcpu->run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN;
-   }
}
 
return EMULATE_DONE;
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index 64891b0..e975279 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -349,7 +349,7 @@ static void kvmppc_set_msr_pr(struct kvm_vcpu *vcpu, u64 
msr)
if (msr & MSR_POW) {
if (!vcpu->arch.pending_exceptions) {
kvm_vcpu_block(vcpu);
-   clear_bit(KVM_REQ_UNHALT, >requests);
+   kvm_clear_request(KVM_REQ_UNHALT, vcpu));
vcpu->stat.halt_wakeup++;
 
/* Unset POW bit after we woke up */
diff --git a/arch/powerpc/kvm/book3s_pr_papr.c 
b/arch/powerpc/kvm/book3s_pr_papr.c
index f2c75a1..60cf393 100644
--- a/arch/powerpc/kvm/book3s_pr_papr.c
+++ b/arch/powerpc/kvm/book3s_pr_papr.c
@@ -309,7 +309,7 @@ int kvmppc_h_pr(struct kvm_vcpu *vcpu, unsigned long cmd)
case H_CEDE:
kvmppc_set_msr_fast(vcpu, kvmppc_get_msr(vcpu) | MSR_EE);
kvm_vcpu_block(vcpu);
-   clear_bit(KVM_REQ_UNHALT, >requests);
+   kvm_clear_request(KVM_REQ_UNHALT, vcpu);
vcpu->stat.halt_wakeup++;
return EMULATE_DONE;
case H_LOGICAL_CI_LOAD:
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index fd58751..6bed382 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -574,7 +574,7 @@ static void arm_next_watchdog(struct kvm_vcpu *vcpu)
 * userspace, so clear the KVM_REQ_WATCHDOG request.
 */
if ((vcpu->arch.tsr & (TSR_ENW | TSR_WIS)) != (TSR_ENW | TSR_WIS))
-   clear_bit(KVM_REQ_WATCHDOG, >requests);
+   kvm_clear_request(KVM_REQ_WATCHDOG, vcpu);
 
spin_lock_irqsave(>arch.wdt_lock, flags);
nr_jiffies = watchdog_next_timeout(vcpu);
@@ -677,7 +677,7 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
 
kvmppc_core_check_exceptions(vcpu);
 
-   if (vcpu->requests) {
+   if (kvm_vcpu_has_requests(vcpu)) {
/* Exception delivery raised request; start over */
return 1;
}
@@ -685,7 +685,7 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
if (vcpu->arch.shared->msr & MSR_WE) {
local_irq_enable();
kvm_vcpu_block(vcpu);
-   clear_bit(KVM_REQ_UNHALT, >requests);
+

Re: [PATCH v1] kvm: Make vcpu->requests as 64 bit bitmap

2015-12-24 Thread James Hogan

Hi Andrey,

On Thu, Dec 24, 2015 at 12:30:26PM +0300, Andrey Smetanin wrote:
> Currently on x86 arch we has already 32 requests defined
> so the newer request bits can't be placed inside
> vcpu->requests(unsigned long) inside x86 32 bit system.
> But we are going to add a new request in x86 arch
> for Hyper-V tsc page support.
> 
> To solve the problem the patch replaces vcpu->requests by
> bitmap with 64 bit length and uses bitmap API.
> 
> The patch consists of:
> * announce kvm_vcpu_has_requests() to check whether vcpu has
> requests
> * announce kvm_vcpu_requests() to get vcpu requests pointer
> * announce kvm_clear_request() to clear particular vcpu request
> * replace if (vcpu->requests) by if (kvm_vcpu_has_requests(vcpu))
> * replace clear_bit(req, vcpu->requests) by
>  kvm_clear_request(req, vcpu)
> 
> Signed-off-by: Andrey Smetanin 
> CC: Paolo Bonzini 
> CC: Gleb Natapov 
> CC: James Hogan 
> CC: Paolo Bonzini 
> CC: Paul Burton 
> CC: Ralf Baechle 
> CC: Alexander Graf 
> CC: Christian Borntraeger 
> CC: Cornelia Huck 
> CC: linux-m...@linux-mips.org
> CC: kvm-ppc@vger.kernel.org
> CC: linux-s...@vger.kernel.org
> CC: Roman Kagan 
> CC: Denis V. Lunev 
> CC: qemu-de...@nongnu.org

For MIPS KVM bit:
Acked-by: James Hogan 

Thanks
James

> 
> ---
>  arch/mips/kvm/emulate.c   |  4 +---
>  arch/powerpc/kvm/book3s_pr.c  |  2 +-
>  arch/powerpc/kvm/book3s_pr_papr.c |  2 +-
>  arch/powerpc/kvm/booke.c  |  6 +++---
>  arch/powerpc/kvm/powerpc.c|  6 +++---
>  arch/powerpc/kvm/trace.h  |  2 +-
>  arch/s390/kvm/kvm-s390.c  |  4 ++--
>  arch/x86/kvm/vmx.c|  2 +-
>  arch/x86/kvm/x86.c| 14 +++---
>  include/linux/kvm_host.h  | 27 ++-
>  10 files changed, 42 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c
> index 41b1b09..14aebe8 100644
> --- a/arch/mips/kvm/emulate.c
> +++ b/arch/mips/kvm/emulate.c
> @@ -774,10 +774,8 @@ enum emulation_result kvm_mips_emul_wait(struct kvm_vcpu 
> *vcpu)
>* We we are runnable, then definitely go off to user space to
>* check if any I/O interrupts are pending.
>*/
> - if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) {
> - clear_bit(KVM_REQ_UNHALT, >requests);
> + if (kvm_check_request(KVM_REQ_UNHALT, vcpu))
>   vcpu->run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN;
> - }
>   }
>  
>   return EMULATE_DONE;
> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
> index 64891b0..e975279 100644
> --- a/arch/powerpc/kvm/book3s_pr.c
> +++ b/arch/powerpc/kvm/book3s_pr.c
> @@ -349,7 +349,7 @@ static void kvmppc_set_msr_pr(struct kvm_vcpu *vcpu, u64 
> msr)
>   if (msr & MSR_POW) {
>   if (!vcpu->arch.pending_exceptions) {
>   kvm_vcpu_block(vcpu);
> - clear_bit(KVM_REQ_UNHALT, >requests);
> + kvm_clear_request(KVM_REQ_UNHALT, vcpu));
>   vcpu->stat.halt_wakeup++;
>  
>   /* Unset POW bit after we woke up */
> diff --git a/arch/powerpc/kvm/book3s_pr_papr.c 
> b/arch/powerpc/kvm/book3s_pr_papr.c
> index f2c75a1..60cf393 100644
> --- a/arch/powerpc/kvm/book3s_pr_papr.c
> +++ b/arch/powerpc/kvm/book3s_pr_papr.c
> @@ -309,7 +309,7 @@ int kvmppc_h_pr(struct kvm_vcpu *vcpu, unsigned long cmd)
>   case H_CEDE:
>   kvmppc_set_msr_fast(vcpu, kvmppc_get_msr(vcpu) | MSR_EE);
>   kvm_vcpu_block(vcpu);
> - clear_bit(KVM_REQ_UNHALT, >requests);
> + kvm_clear_request(KVM_REQ_UNHALT, vcpu);
>   vcpu->stat.halt_wakeup++;
>   return EMULATE_DONE;
>   case H_LOGICAL_CI_LOAD:
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index fd58751..6bed382 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -574,7 +574,7 @@ static void arm_next_watchdog(struct kvm_vcpu *vcpu)
>* userspace, so clear the KVM_REQ_WATCHDOG request.
>*/
>   if ((vcpu->arch.tsr & (TSR_ENW | TSR_WIS)) != (TSR_ENW | TSR_WIS))
> - clear_bit(KVM_REQ_WATCHDOG, >requests);
> + kvm_clear_request(KVM_REQ_WATCHDOG, vcpu);
>  
>   spin_lock_irqsave(>arch.wdt_lock, flags);
>   nr_jiffies = watchdog_next_timeout(vcpu);
> @@ -677,7 +677,7 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
>  
>   kvmppc_core_check_exceptions(vcpu);
>  
> - if (vcpu->requests) {
> + if (kvm_vcpu_has_requests(vcpu)) {
>   /* Exception delivery raised request; start

Re: [PATCH v1] kvm: Make vcpu->requests as 64 bit bitmap

2015-12-24 Thread Andrey Smetanin




On 12/24/2015 02:14 PM, Roman Kagan wrote:

On Thu, Dec 24, 2015 at 12:30:26PM +0300, Andrey Smetanin wrote:

Currently on x86 arch we has already 32 requests defined
so the newer request bits can't be placed inside
vcpu->requests(unsigned long) inside x86 32 bit system.
But we are going to add a new request in x86 arch
for Hyper-V tsc page support.

To solve the problem the patch replaces vcpu->requests by
bitmap with 64 bit length and uses bitmap API.

The patch consists of:
* announce kvm_vcpu_has_requests() to check whether vcpu has
requests
* announce kvm_vcpu_requests() to get vcpu requests pointer


I think that if abstracting out the implementation of the request
container is what you're after, you'd better not define this function;
accesses to the request map should be through your accessor functions.


Good idea! I'll rework this patch and will send V2.



* announce kvm_clear_request() to clear particular vcpu request
* replace if (vcpu->requests) by if (kvm_vcpu_has_requests(vcpu))
* replace clear_bit(req, vcpu->requests) by
  kvm_clear_request(req, vcpu)


Apparently one accessor is missing: test the presence of a request
without clearing it from the bitmap (because kvm_check_request() clears
it).

agree



diff --git a/arch/powerpc/kvm/trace.h b/arch/powerpc/kvm/trace.h
index 2e0e67e..4015b88 100644
--- a/arch/powerpc/kvm/trace.h
+++ b/arch/powerpc/kvm/trace.h
@@ -109,7 +109,7 @@ TRACE_EVENT(kvm_check_requests,

TP_fast_assign(
__entry->cpu_nr  = vcpu->vcpu_id;
-   __entry->requests= vcpu->requests;
+   __entry->requests= (__u32)kvm_vcpu_requests(vcpu)[0];
),


This doesn't make sense, to expose only subset of the requests.  BTW I
don't see this event in Linus tree, nor in linux-next,

I found it here:
arch/powerpc/kvm/powerpc.c:104: trace_kvm_check_requests(vcpu);
>so I'm not quite

sure why it's formed this way; I guess the interesting part is the
request number and the return value (i.e. whether it's set), not the
whole bitmap.

No, the code actually dumps part of first 32 bit of bitmap:
TP_printk("vcpu=%x requests=%x",
__entry->cpu_nr, __entry->requests)

So, for this corner case we can make __entry->requests as unsigned long 
variable and make the API helper to get first unsigned long from 
vcpu->requests bitmap. Next print part of bitmap this way:

 TP_printk("vcpu=0x%x requests=0x%lx",
__entry->cpu_nr, __entry->requests)
(as unsigned long).

POWERPC guys what do you think about such approach? Or could
we even drop trace_kvm_check_requests() at all. Does 
trace_kvm_check_requests() still useful for you?



--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5957,7 +5957,7 @@ static int handle_invalid_guest_state(struct kvm_vcpu 
*vcpu)
if (intr_window_requested && vmx_interrupt_allowed(vcpu))
return handle_interrupt_window(>vcpu);

-   if (test_bit(KVM_REQ_EVENT, >requests))
+   if (test_bit(KVM_REQ_EVENT, kvm_vcpu_requests(vcpu)))


Here you'd rather want that function to test for the request without
clearing it.

agree, i'll provide such function.



--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2117,7 +2117,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)

if (ka->boot_vcpu_runs_old_kvmclock != tmp)
set_bit(KVM_REQ_MASTERCLOCK_UPDATE,
-   >requests);
+   kvm_vcpu_requests(vcpu));


This should have been kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);


agree

@@ -8048,7 +8048,7 @@ static inline bool kvm_vcpu_has_events(struct kvm_vcpu 
*vcpu)
if (atomic_read(>arch.nmi_queued))
return true;

-   if (test_bit(KVM_REQ_SMI, >requests))
+   if (test_bit(KVM_REQ_SMI, kvm_vcpu_requests(vcpu)))


Again the test-only accessor is due here.

agree



--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -146,6 +146,8 @@ static inline bool is_error_page(struct page *page)
  #define KVM_REQ_HV_EXIT   30
  #define KVM_REQ_HV_STIMER 31

+#define KVM_REQ_MAX   64
+
  #define KVM_USERSPACE_IRQ_SOURCE_ID   0
  #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID  1

@@ -233,7 +235,7 @@ struct kvm_vcpu {
int vcpu_id;
int srcu_idx;
int mode;
-   unsigned long requests;
+   DECLARE_BITMAP(requests, KVM_REQ_MAX);
unsigned long guest_debug;

int pre_pcpu;
@@ -286,6 +288,16 @@ struct kvm_vcpu {
struct kvm_vcpu_arch arch;
  };

+static inline bool kvm_vcpu_has_requests(struct kvm_vcpu *vcpu)
+{
+   return !bitmap_empty(vcpu->requests, KVM_REQ_MAX);
+}
+
+static inline ulong *kvm_vcpu_requests(struct kvm_vcpu *vcpu)
+{
+   return (ulong *)vcpu->requests;
+}


As I wrote above, I believe this function doesn't belong in the API.

agree,

Re: [PATCH v1] kvm: Make vcpu->requests as 64 bit bitmap

2015-12-24 Thread Roman Kagan

On Thu, Dec 24, 2015 at 12:30:26PM +0300, Andrey Smetanin wrote:
> Currently on x86 arch we has already 32 requests defined
> so the newer request bits can't be placed inside
> vcpu->requests(unsigned long) inside x86 32 bit system.
> But we are going to add a new request in x86 arch
> for Hyper-V tsc page support.
> 
> To solve the problem the patch replaces vcpu->requests by
> bitmap with 64 bit length and uses bitmap API.
> 
> The patch consists of:
> * announce kvm_vcpu_has_requests() to check whether vcpu has
> requests
> * announce kvm_vcpu_requests() to get vcpu requests pointer

I think that if abstracting out the implementation of the request
container is what you're after, you'd better not define this function;
accesses to the request map should be through your accessor functions.

> * announce kvm_clear_request() to clear particular vcpu request
> * replace if (vcpu->requests) by if (kvm_vcpu_has_requests(vcpu))
> * replace clear_bit(req, vcpu->requests) by
>  kvm_clear_request(req, vcpu)

Apparently one accessor is missing: test the presence of a request
without clearing it from the bitmap (because kvm_check_request() clears
it).

> diff --git a/arch/powerpc/kvm/trace.h b/arch/powerpc/kvm/trace.h
> index 2e0e67e..4015b88 100644
> --- a/arch/powerpc/kvm/trace.h
> +++ b/arch/powerpc/kvm/trace.h
> @@ -109,7 +109,7 @@ TRACE_EVENT(kvm_check_requests,
>  
>   TP_fast_assign(
>   __entry->cpu_nr = vcpu->vcpu_id;
> - __entry->requests   = vcpu->requests;
> + __entry->requests   = (__u32)kvm_vcpu_requests(vcpu)[0];
>   ),

This doesn't make sense, to expose only subset of the requests.  BTW I
don't see this event in Linus tree, nor in linux-next, so I'm not quite
sure why it's formed this way; I guess the interesting part is the
request number and the return value (i.e. whether it's set), not the
whole bitmap.

> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5957,7 +5957,7 @@ static int handle_invalid_guest_state(struct kvm_vcpu 
> *vcpu)
>   if (intr_window_requested && vmx_interrupt_allowed(vcpu))
>   return handle_interrupt_window(>vcpu);
>  
> - if (test_bit(KVM_REQ_EVENT, >requests))
> + if (test_bit(KVM_REQ_EVENT, kvm_vcpu_requests(vcpu)))

Here you'd rather want that function to test for the request without
clearing it.

> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2117,7 +2117,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
>  
>   if (ka->boot_vcpu_runs_old_kvmclock != tmp)
>   set_bit(KVM_REQ_MASTERCLOCK_UPDATE,
> - >requests);
> + kvm_vcpu_requests(vcpu));

This should have been kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);

> @@ -8048,7 +8048,7 @@ static inline bool kvm_vcpu_has_events(struct kvm_vcpu 
> *vcpu)
>   if (atomic_read(>arch.nmi_queued))
>   return true;
>  
> - if (test_bit(KVM_REQ_SMI, >requests))
> + if (test_bit(KVM_REQ_SMI, kvm_vcpu_requests(vcpu)))

Again the test-only accessor is due here.

> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -146,6 +146,8 @@ static inline bool is_error_page(struct page *page)
>  #define KVM_REQ_HV_EXIT   30
>  #define KVM_REQ_HV_STIMER 31
>  
> +#define KVM_REQ_MAX   64
> +
>  #define KVM_USERSPACE_IRQ_SOURCE_ID  0
>  #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID 1
>  
> @@ -233,7 +235,7 @@ struct kvm_vcpu {
>   int vcpu_id;
>   int srcu_idx;
>   int mode;
> - unsigned long requests;
> + DECLARE_BITMAP(requests, KVM_REQ_MAX);
>   unsigned long guest_debug;
>  
>   int pre_pcpu;
> @@ -286,6 +288,16 @@ struct kvm_vcpu {
>   struct kvm_vcpu_arch arch;
>  };
>  
> +static inline bool kvm_vcpu_has_requests(struct kvm_vcpu *vcpu)
> +{
> + return !bitmap_empty(vcpu->requests, KVM_REQ_MAX);
> +}
> +
> +static inline ulong *kvm_vcpu_requests(struct kvm_vcpu *vcpu)
> +{
> + return (ulong *)vcpu->requests;
> +}

As I wrote above, I believe this function doesn't belong in the API.

> +
>  static inline int kvm_vcpu_exiting_guest_mode(struct kvm_vcpu *vcpu)
>  {
>   return cmpxchg(>mode, IN_GUEST_MODE, EXITING_GUEST_MODE);
> @@ -1002,7 +1014,7 @@ static inline bool kvm_is_error_gpa(struct kvm *kvm, 
> gpa_t gpa)
>  
>  static inline void kvm_migrate_timers(struct kvm_vcpu *vcpu)
>  {
> - set_bit(KVM_REQ_MIGRATE_TIMER, >requests);
> + set_bit(KVM_REQ_MIGRATE_TIMER, kvm_vcpu_requests(vcpu));

kvm_make_request(KVM_REQ_MIGRATE_TIMER, vcpu);

> @@ -1118,19 +1130,24 @@ static inline bool kvm_vcpu_compatible(struct 
> kvm_vcpu *vcpu) { return true; }
>  
>  static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
>  {
> - set_bit(req, >requests);
> + set_bit(req, kvm_vcpu_requests(vcpu));
>  }
>  
>  static inline

[PATCH v1] kvm: Make vcpu->requests as 64 bit bitmap

Re: [PATCH v1] kvm: Make vcpu->requests as 64 bit bitmap

Re: [PATCH v1] kvm: Make vcpu->requests as 64 bit bitmap

Re: [PATCH v1] kvm: Make vcpu->requests as 64 bit bitmap

4 matches

Site Navigation

Mail list logo

Footer information