Re: [GIT PULL 3/8] KVM: s390: fix get_all_floating_irqs
On Tue, Mar 31, 2015 at 03:01:58PM +0200, Christian Borntraeger wrote: From: Jens Freimann jf...@linux.vnet.ibm.com This fixes a bug introduced with commit c05c4186bbe4 (KVM: s390: add floating irq controller). get_all_floating_irqs() does copy_to_user() while holding a spin lock. Let's fix this by filling a temporary buffer first and copy it to userspace after giving up the lock. Cc: sta...@vger.kernel.org # 3.18+: 69a8d4562638 KVM: s390: no need to hold... Reviewed-by: David Hildenbrand d...@linux.vnet.ibm.com Signed-off-by: Jens Freimann jf...@linux.vnet.ibm.com Signed-off-by: Christian Borntraeger borntrae...@de.ibm.com Acked-by: Cornelia Huck cornelia.h...@de.ibm.com ... -static int get_all_floating_irqs(struct kvm *kvm, __u8 *buf, __u64 len) +static int get_all_floating_irqs(struct kvm *kvm, __user u8 *usrbuf, u64 len) fwiw, this is probably the only place within the kernel where we see __user u8 * instead of u8 __user *. This is odd within this whole patch. + if (copy_to_user((void __user *) usrbuf, The cast shouldn't be necessary at all... -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 14/21] KVM: s390: clear the pfault queue if user space sets the invalid token
On Thu, Jan 15, 2015 at 02:43:27PM +0100, Christian Borntraeger wrote:
From: David Hildenbrand d...@linux.vnet.ibm.com
We need a way to clear the async pfault queue from user space (e.g.
for resets and SIGP SET ARCHITECTURE).
This patch simply clears the queue as soon as user space sets the
invalid pfault token. The definition of the invalid token is moved
to uapi.
Signed-off-by: David Hildenbrand d...@linux.vnet.ibm.com
Acked-by: Cornelia Huck cornelia.h...@de.ibm.com
Signed-off-by: Christian Borntraeger borntrae...@de.ibm.com
---
arch/s390/include/asm/kvm_host.h | 1 -
arch/s390/include/uapi/asm/kvm.h | 3 +++
arch/s390/kvm/kvm-s390.c | 4
3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/s390/include/asm/kvm_host.h
b/arch/s390/include/asm/kvm_host.h
index 4de479e..b617052 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -469,7 +469,6 @@ struct kvm_vcpu_arch {
};
struct gmap *gmap;
struct kvm_guestdbg_info_arch guestdbg;
-#define KVM_S390_PFAULT_TOKEN_INVALID(-1UL)
unsigned long pfault_token;
unsigned long pfault_select;
unsigned long pfault_compare;
diff --git a/arch/s390/include/uapi/asm/kvm.h
b/arch/s390/include/uapi/asm/kvm.h
index 9c01159..ac3000d 100644
--- a/arch/s390/include/uapi/asm/kvm.h
+++ b/arch/s390/include/uapi/asm/kvm.h
@@ -108,6 +108,9 @@ struct kvm_guest_debug_arch {
struct kvm_hw_breakpoint __user *hw_bp;
};
+/* for KVM_SYNC_PFAULT and KVM_REG_S390_PFTOKEN */
+#define KVM_S390_PFAULT_TOKEN_INVALID0xUL
uapi is for both 64 bit and (theoretically) also 32 bit. So this
should be 0xfff...ffULL instead of UL to prevent a different compat
ABI.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL 10/11] KVM: s390: handle pending local interrupts via bitmap
On Fri, Nov 28, 2014 at 02:25:38PM +0100, Christian Borntraeger wrote:
+static int __must_check __deliver_mchk_floating(struct kvm_vcpu *vcpu,
+struct kvm_s390_interrupt_info *inti)
+{
+ struct kvm_s390_mchk_info *mchk = inti-mchk;
+ int rc;
+
+ VCPU_EVENT(vcpu, 4, interrupt: machine check mcic=%llx,
+mchk-mcic);
+ trace_kvm_s390_deliver_interrupt(vcpu-vcpu_id, KVM_S390_MCHK,
+ mchk-cr14, mchk-mcic);
+
+ rc = kvm_s390_vcpu_store_status(vcpu, KVM_S390_STORE_STATUS_PREFIXED);
+ rc |= put_guest_lc(vcpu, mchk-mcic,
+ (u64 __user *) __LC_MCCK_CODE);
+ rc |= put_guest_lc(vcpu, mchk-failing_storage_address,
+ (u64 __user *) __LC_MCCK_FAIL_STOR_ADDR);
+ rc |= write_guest_lc(vcpu, __LC_PSW_SAVE_AREA,
+ mchk-fixed_logout, sizeof(mchk-fixed_logout));
+ rc |= write_guest_lc(vcpu, __LC_MCK_OLD_PSW,
+ vcpu-arch.sie_block-gpsw, sizeof(psw_t));
+ rc |= read_guest_lc(vcpu, __LC_MCK_NEW_PSW,
+ vcpu-arch.sie_block-gpsw, sizeof(psw_t));
+ return rc;
+}
FWIW, rc handling seems to be a bit fragile.
The usual return statement for such a pattern is
return rc ? -EWHATEVER : 0;
so we don't get random or'ed return values.
-static int __inject_prog_irq(struct kvm_vcpu *vcpu,
- struct kvm_s390_interrupt_info *inti)
+static int __inject_prog(struct kvm_vcpu *vcpu, struct kvm_s390_irq *irq)
{
struct kvm_s390_local_interrupt *li = vcpu-arch.local_int;
- list_add(inti-list, li-list);
- atomic_set(li-active, 1);
+ li-irq.pgm = irq-u.pgm;
+ __set_bit(IRQ_PEND_PROG, li-pending_irqs);
^
+static int __inject_pfault_init(struct kvm_vcpu *vcpu, struct kvm_s390_irq
*irq)
{
struct kvm_s390_local_interrupt *li = vcpu-arch.local_int;
- inti-ext.ext_params2 = s390int-parm64;
- list_add_tail(inti-list, li-list);
- atomic_set(li-active, 1);
+ VCPU_EVENT(vcpu, 3, inject: external irq params:%x, params2:%llx,
+irq-u.ext.ext_params, irq-u.ext.ext_params2);
+ trace_kvm_s390_inject_vcpu(vcpu-vcpu_id, KVM_S390_INT_PFAULT_INIT,
+irq-u.ext.ext_params,
+irq-u.ext.ext_params2, 2);
+
+ li-irq.ext = irq-u.ext;
+ set_bit(IRQ_PEND_PFAULT_INIT, li-pending_irqs);
^^^
You're using atomic and non-atomic bitops all over the place on the same
object(s). It would be very good to have some consistency here.
And as far as I remember the non-atomic variant is good enough anyway.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL 1/2] KVM: s390/facilities: allow TOD-CLOCK steering facility bit
On Wed, Oct 01, 2014 at 08:27:38PM +0200, Christian Borntraeger wrote: On 10/01/2014 04:17 PM, Alexander Graf wrote: On 01.10.14 16:02, Christian Borntraeger wrote: There is nothing to do for KVM to support TOD-CLOCK steering. Signed-off-by: Christian Borntraeger borntrae...@de.ibm.com Reviewed-by: David Hildenbrand d...@linux.vnet.ibm.com --- arch/s390/kvm/kvm-s390.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index 56a411c..0d5aa88 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -1786,7 +1786,7 @@ static int __init kvm_s390_init(void) return -ENOMEM; } memcpy(vfacilities, S390_lowcore.stfle_fac_list, 16); - vfacilities[0] = 0xff82fff3f4fc2000UL; + vfacilities[0] = 0xff82fffbf47c2000UL; Can we please convert this into something readable soon? :) It will be sooner when you send patches ;-) The facility numbers are documented in the POP (chapter 4 last page) in IBM notation (bit0 is the MSB) It probably makes sense to do this for the non-KVM part as well. When you grep for test_facility under arch/s390 there are lots of numerical value. These numbers _are_ a wart and were the source of a couple of bugs e.g. in our ALS code already. However converting these bitfields to something readable doesn't seem to be easy, since I'd like to have variable size array initializers which set the bits depening on the symbolic name (e.g. set bits 19, 20 and 139 and automatically choose the correct size of the array): ..something like INIT_FACILITY_ARRAY(FAC19, FAC20, FAC139) And of course this should work for asm code as well. Hmm, maybe we can find somebody that wants to increase the patch counter? If you think this is trivial, please send a patch which does this. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC][patch 3/6] KVM: s390: Add GISA support
On Thu, Sep 04, 2014 at 12:52:26PM +0200, frank.blasc...@de.ibm.com wrote:
+void kvm_s390_gisa_register_alert(struct kvm *kvm, u32 gisc)
+{
+ int bito = BITS_PER_BYTE * 7 + gisc;
+
+ set_bit(bito ^ (BITS_PER_LONG - 1), kvm-arch.iam);
+}
Just a very minor nit: you could also use set_bit_inv() friends.
+static inline u64 kvm_s390_get_base_disp_rxy(struct kvm_vcpu *vcpu)
+{
+ u32 x2 = (vcpu-arch.sie_block-ipa 0x000f);
+ u32 base2 = vcpu-arch.sie_block-ipb 28;
+ u32 disp2 = ((vcpu-arch.sie_block-ipb 0x0fff) 16) +
+ ((vcpu-arch.sie_block-ipb 0xff00) 4);
+
+ return (base2 ? vcpu-run-s.regs.gprs[base2] : 0) +
+ (x2 ? vcpu-run-s.regs.gprs[x2] : 0) + (u64)disp2;
+}
Not very readable ;) However.. for the RXY instruction format the 20 bit
displacement is usually signed and not unsigned like your code seems to
treat it.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL 1/6] KVM: s390: Handle MVPG partial execution interception
On Tue, Apr 29, 2014 at 03:36:43PM +0200, Christian Borntraeger wrote:
+static int handle_mvpg_pei(struct kvm_vcpu *vcpu)
+{
+ unsigned long hostaddr, srcaddr, dstaddr;
+ psw_t *psw = vcpu-arch.sie_block-gpsw;
+ struct mm_struct *mm = current-mm;
+ int reg1, reg2, rc;
+
+ kvm_s390_get_regs_rre(vcpu, reg1, reg2);
+ srcaddr = kvm_s390_real_to_abs(vcpu, vcpu-run-s.regs.gprs[reg2]);
+ dstaddr = kvm_s390_real_to_abs(vcpu, vcpu-run-s.regs.gprs[reg1]);
+
+ /* Make sure that the source is paged-in */
+ hostaddr = gmap_fault(srcaddr, vcpu-arch.gmap);
+ if (IS_ERR_VALUE(hostaddr))
+ return kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING);
FWIW (and nothing that should keep this code from going upstream),
this is not entirely correct, since gmap_fault() may return -ENOMEM.
So a host out-of-memory situation will incorrectly result in a guest
addressing exception, which is most likely not what we want.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/5] KVM: s390: adapter interrupt sources
On Mon, Mar 17, 2014 at 07:11:37PM +0100, Cornelia Huck wrote: Add a new interface to register/deregister sources of adapter interrupts identified by an unique id via the flic. Adapters may also be maskable and carry a list of pinned pages. These adapters will be used by irq routing later. Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com --- [...] +#define MAX_S390_IO_ADAPTERS (MAX_ISC + 1) * 8 Subtle possible bug alert ;) Please add braces around (MAX_ISC + 1) * 8. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] KVM changes for 3.12
On Wed, Sep 04, 2013 at 06:08:08PM -0700, Linus Torvalds wrote: On Tue, Sep 3, 2013 at 5:10 AM, Gleb Natapov g...@redhat.com wrote: This pull request adds tlb_gather_mmu() caller in S390 code, but 2b047252 in your tree added another parameter to the function, so the patch bellow have to be applied during merge to resolve the conflicts. The patch was used in linux-next for awhile. Hmm. Fine. Except: /* Reallocate the page tables with pgstes */ mm-context.has_pgste = 1; - tlb_gather_mmu(tlb, mm, 0); + tlb_gather_mmu(tlb, mm, 0, TASK_SIZE); page_table_realloc(tlb, mm, 0, TASK_SIZE); tlb_finish_mmu(tlb, 0, -1); up_write(mm-mmap_sem); Realistically, the begin/end arguments to tlb_gather_mmu() and tlb_finish_mmu() should match. In fact, I considered getting rid of the ones to tlb_finish_mmu() because they are kind of pointless these days (but didn't, because I wanted to keep the patches minimal). And in your case they don't. Which implies a certain amount of confusion. Actually they do match in our internal version of the merge conflict. It was just a copy-paste error from me when sending the merge resolution patch. Since the fix contained two changes lines within the same hunk it was hard to get right.. oh well.. :) Thanks for fixing it! -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: s390: add floating irq controller
On Wed, Jul 31, 2013 at 11:08:15AM +0200, Cornelia Huck wrote:
On Mon, 29 Jul 2013 15:59:53 +0200
Jens Freimann jf...@linux.vnet.ibm.com wrote:
This patch adds a floating irq controller as a kvm_device.
It will be necesary for migration of floating interrupts as well
as for hardening the reset code by allowing user space to explicitly
remove all pending floating interrupts.
Signed-off-by: Jens Freimann jf...@linux.vnet.ibm.com
---
arch/s390/include/uapi/asm/kvm.h | 5 +
arch/s390/kvm/interrupt.c| 210
+++
arch/s390/kvm/kvm-s390.c | 1 +
include/linux/kvm_host.h | 1 +
include/uapi/linux/kvm.h | 1 +
virt/kvm/kvm_main.c | 3 +
6 files changed, 181 insertions(+), 40 deletions(-)
+static int flic_set_attr(struct kvm_device *dev, struct kvm_device_attr
*attr)
+{
+ int r;
+ struct kvm_s390_irq *s390irq;
+ struct kvm_s390_interrupt_info *inti;
+
+ switch (attr-group) {
+ case KVM_DEV_FLIC_ENQUEUE:
+ inti = kzalloc(sizeof(*inti), GFP_KERNEL);
+ if (!inti) {
+ r = -ENOMEM;
+ goto out;
+ }
+ s390irq = kzalloc(sizeof(*s390irq), GFP_KERNEL);
+ if (!s390irq) {
+ r = -ENOMEM;
+ goto out_free_inti;
+ }
+ if (copy_from_user(s390irq, (u64 __user *)attr-addr,
+ sizeof(s390irq))) {
+ r = -EFAULT;
+ goto out_free_s390irq;
+ }
Can't you just copy into inti-irq here? You'd get rid of allocating
two structures that way.
+ switch(s390irq-type) {
+ case KVM_S390_INT_VIRTIO:
+ case KVM_S390_INT_SERVICE:
+ case KVM_S390_INT_IO_MIN...KVM_S390_INT_IO_MAX:
+ case KVM_S390_MCHK:
+ inti-irq = *s390irq;
+ __inject_vm(dev-kvm, inti);
+ default:
+ r = -EINVAL;
+ }
+ break;
And, due to missing break statement, it will always return -EINVAL ;)
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/2] KVM: s390: add floating irq controller
On Fri, Jul 26, 2013 at 06:47:59PM +0200, Jens Freimann wrote:
This patch adds a floating irq controller as a kvm_device.
It will be necesary for migration of floating interrupts as well
as for hardening the reset code by allowing user space to explicitly
remove all pending floating interrupts.
Signed-off-by: Jens Freimann jf...@linux.vnet.ibm.com
[...]
+static void clear_floating_interrupts(struct kvm *kvm)
+{
+ struct kvm_s390_float_interrupt *fi = kvm-arch.float_int;
+ struct kvm_s390_interrupt_info *n, *inti = NULL;
+
+ if (atomic_read(fi-active)) {
+ spin_lock_bh(fi-lock);
+ list_for_each_entry_safe(inti, n, fi-list, list) {
+ list_del(inti-list);
+ kfree(inti);
+ }
+ atomic_set(fi-active, 0);
+ spin_unlock_bh(fi-lock);
+ }
+}
FWIW, unrelated to your patch, since it used to be always like this:
the sematics of fi-active atomic_t seem to be a bit odd. It only
gets set while the fi-lock spinlock is held and might be read also
while not holding the spinlock.
Either it's racy, the spinlock is not needed at all, or it should
be held everytime.
Besides that the meaning of the active value seems to be the same
as !list_empty()... so you could get rid of it.
Just a comment ;)
+static int flic_set_attr(struct kvm_device *dev, struct kvm_device_attr
*attr)
+{
+ int r;
+
+ switch (attr-group) {
+ case KVM_DEV_FLIC_ENQUEUE: {
+ struct kvm_s390_irq *s390irq;
+ struct kvm_s390_interrupt_info *inti;
+ inti = kzalloc(sizeof(*inti), GFP_KERNEL);
+ if (!inti)
+ return -ENOMEM;
+ s390irq = kzalloc(sizeof(*s390irq), GFP_KERNEL);
+ if (!s390irq)
+ return -ENOMEM;
+ if (copy_from_user(s390irq, (u64 __user *)attr-addr,
+ sizeof(s390irq)))
+ return -EFAULT;
User space triggerable memory leak.
+ inti-irq = *s390irq;
+ __inject_vm(dev-kvm, inti);
I think you must check the irq type, otherwise the kernel may crash if it
tries to deliver the interrupt and it doesn't match any of the known irqs.
(or remove the BUG() in the deliver function, or both ;)
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] KVM: s390: Add support for machine checks.
On Fri, Dec 07, 2012 at 01:30:22PM +0100, Cornelia Huck wrote:
+ rc = put_guest_u64(vcpu, __LC_MCCK_CODE, inti-mchk.mcic);
+ if (rc == -EFAULT)
+ exception = 1;
+
+ rc = copy_to_guest(vcpu, __LC_MCK_OLD_PSW,
+vcpu-arch.sie_block-gpsw, sizeof(psw_t));
+ if (rc == -EFAULT)
+ exception = 1;
Please don't add more explicit -EFAULT checks on guest access paths. Just
make this like normal user space accesses. That is return code != 0 means
an error occured:
rc = put_guest_u64(vcpu, __LC_MCCK_CODE, inti-mchk.mcic);
if (rc)
exception = 1;
In fact, with the current kvm gaccess code it's even broken, since on error
the guest access functions may return also -ENOMEM instead of -EFAULT, which
would be ignored by your code.
I addressed that with a patch when trying to clean up the guest access
functions. Maybe the patch below should be merged anyway. Christian?
===
From db05454b6f3f49a7a10f5a1e546917303cf94532 Mon Sep 17 00:00:00 2001
From: Heiko Carstens heiko.carst...@de.ibm.com
Date: Mon, 10 Sep 2012 16:36:23 +0200
Subject: s390/kvm,gaccess: fix guest access return code handling
Guest access functions like copy_to/from_guest() call __guestaddr_to_user()
which in turn call gmap_fault() in order to translate a guest address to a
user space address.
In error case __guest_addr_to_user() returns either -EFAULT or -ENOMEM.
The copy_to/from_guest functions just pass these return values down to the
callers.
The -ENOMEM case however is problematic since there are several places
which access guest memory like:
rc = copy_to_guest(...);
if (rc == -EFAULT)
error_handling();
So in case of -ENOMEM the code assumes that the guest memory access
succeeded even though it failed.
This can cause guest data or state corruption.
If __guestaddr_to_user() returns -ENOMEM the meaning is that a valid user
space mapping exists, but there was not enough memory available when trying
to build the guest mapping. In other words an out-of-memory situation
occured.
For normal user space accesses an out-of-memory situation causes the page
fault handler to map -ENOMEM to -EFAULT (see fixup code in do_no_context()).
We need to do exactly the same for the kvm gaccess functions.
So __guestaddr_to_user() should just map all error codes to -EFAULT.
Signed-off-by: Heiko Carstens heiko.carst...@de.ibm.com
---
arch/s390/kvm/gaccess.h | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/s390/kvm/gaccess.h b/arch/s390/kvm/gaccess.h
index 4703f12..84d01dd 100644
--- a/arch/s390/kvm/gaccess.h
+++ b/arch/s390/kvm/gaccess.h
@@ -22,13 +22,16 @@ static inline void __user *__guestaddr_to_user(struct
kvm_vcpu *vcpu,
unsigned long guestaddr)
{
unsigned long prefix = vcpu-arch.sie_block-prefix;
+ unsigned long uaddress;
if (guestaddr 2 * PAGE_SIZE)
guestaddr += prefix;
else if ((guestaddr = prefix) (guestaddr prefix + 2 * PAGE_SIZE))
guestaddr -= prefix;
-
- return (void __user *) gmap_fault(guestaddr, vcpu-arch.gmap);
+ uaddress = gmap_fault(guestaddr, vcpu-arch.gmap);
+ if (IS_ERR_VALUE(uaddress))
+ uaddress = -EFAULT;
+ return (void __user *)uaddress;
}
static inline int get_guest_u64(struct kvm_vcpu *vcpu, unsigned long guestaddr,
--
1.7.12.4
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] KVM: s390: Add support for machine checks.
On Wed, Dec 19, 2012 at 11:20:18AM +0100, Christian Borntraeger wrote: On 19/12/12 10:44, Heiko Carstens wrote: On Fri, Dec 07, 2012 at 01:30:22PM +0100, Cornelia Huck wrote: + rc = put_guest_u64(vcpu, __LC_MCCK_CODE, inti-mchk.mcic); + if (rc == -EFAULT) + exception = 1; + + rc = copy_to_guest(vcpu, __LC_MCK_OLD_PSW, + vcpu-arch.sie_block-gpsw, sizeof(psw_t)); + if (rc == -EFAULT) + exception = 1; Please don't add more explicit -EFAULT checks on guest access paths. Just make this like normal user space accesses. That is return code != 0 means an error occured: rc = put_guest_u64(vcpu, __LC_MCCK_CODE, inti-mchk.mcic); if (rc) exception = 1; In fact, with the current kvm gaccess code it's even broken, since on error the guest access functions may return also -ENOMEM instead of -EFAULT, which would be ignored by your code. I addressed that with a patch when trying to clean up the guest access functions. Maybe the patch below should be merged anyway. Christian? The whole guest memory access of KVM needs to be reworked to work properly in those corner cases. I have this on my todo list as one of things for next year with lots of open questions that I dont want to answer before xmas. what about in kernel intercepts? (shall we then return EFAULT for the KVM_RUN ioctl, shall we kill the guest?.) We actually need to test the address for validity via the memslots (and not via return value of copy_from/to_user) all across the s390 code. I really want to avoid mixing this effort with the virtio-ccw patches. So my proposal is to apply your patch below and keep Conny's patch as is. Ok? Sure. :) -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/7] s390/kvm: Add support for machine checks.
On Tue, Sep 04, 2012 at 05:13:25PM +0200, Cornelia Huck wrote:
Just some quick comments:
[...]
int kvm_s390_inject_program_int(struct kvm_vcpu *vcpu, u16 code)
{
struct kvm_s390_local_interrupt *li = vcpu-arch.local_int;
@@ -648,6 +747,12 @@ int kvm_s390_inject_vm(struct kvm *kvm,
case KVM_S390_INT_EMERGENCY:
kfree(inti);
return -EINVAL;
+ case KVM_S390_MCHK:
+ VM_EVENT(kvm, 5, inject: machine check parm64:%llx,
+ s390int-parm64);
+ inti-type = s390int-type;
+ inti-mchk.mcic = s390int-parm64;
+ break;
The kvm_s390_interrupt struct seems to be inappropriate to pass machine check
data around.
E.g. if you want to inject an uncorrectable storage error, because the host
failed to swap in a page, you must also pass a failing storage address which
doesn't fit into this structure.
Just something you should consider. ;)
+static int handle_lpswe(struct kvm_vcpu *vcpu)
+{
+ int base2 = vcpu-arch.sie_block-ipb 28;
+ int disp2 = ((vcpu-arch.sie_block-ipb 0x0fff) 16);
Sooner or later we need helper functions which extract the significant parts
of an instruction.
Maybay something like insn_[type]_get_base2(...) or simply structures like
struct insn_[type], which allow to easily access parts of an instruction.
+ u64 addr;
+ u64 new_psw[2];
psw_t?
+
+ addr = disp2;
+ if (base2)
+ addr += vcpu-run-s.regs.gprs[base2];
+
+ if (addr 7) {
+ kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+ goto out;
+ }
+
+ if (copy_from_guest(vcpu, new_psw, addr, sizeof(*new_psw))) {
I assume that should be sizeof(new_psw). Did that ever work?!
+ if ((vcpu-arch.sie_block-gpsw.mask 0xb80800fe7fff) ||
+ (((vcpu-arch.sie_block-gpsw.mask 0x00011000) ==
+ 0x1000)
+ (vcpu-arch.sie_block-gpsw.addr 0x8000)) ||
+ (!(vcpu-arch.sie_block-gpsw.mask 0x00018000)
+ (vcpu-arch.sie_block-gpsw.addr 0xfff0)) ||
+ ((vcpu-arch.sie_block-gpsw.mask 0x00011000) ==
+ 0x0001)) {
This is not very readable...
Please make use of the PSW defines in ptrace.h and add new ones if needed.
Also please make use of (move) the PSW32 defines in compat.h.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] s390/kvm: Handle hosts not supporting s390-virtio.
On Thu, Aug 09, 2012 at 12:41:57PM +0200, Cornelia Huck wrote:
On Thu, 09 Aug 2012 13:03:57 +0300
Avi Kivity a...@redhat.com wrote:
On 08/07/2012 05:52 PM, Cornelia Huck wrote:
Running under a kvm host does not necessarily imply the presence of
a page mapped above the main memory with the virtio information;
however, the code includes a hard coded access to that page.
Instead, check for the presence of the page and exit gracefully
before we hit an addressing exception if it does not exist.
/*
* Init function for virtio
* devices are in a single page above top of normal mem
@@ -443,6 +458,12 @@ static int __init kvm_devices_init(void)
}
kvm_devices = (void *) real_memory_size;
+ if (test_devices_support() 0) {
+ vmem_remove_mapping(real_memory_size, PAGE_SIZE);
+ root_device_unregister(kvm_root);
+ /* No error. */
+ return 0;
+ }
Cleaner to defer root_device_register() until after the mapping has been
verified.
OK, will reorder.
Please use the lura instruction to figure out if the guest real page
even exists _before_ creating the mapping.
That way you don't need all the code afterwards.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] kvm-s390: provide standard guest registers via kvm_run
On Tue, Jan 03, 2012 at 04:50:58PM +0100, Christian Borntraeger wrote:
struct kvm_sync_rw_regs {
+ __u64 gprs[16];/* general purpose registers */
+ s390_fp_regs fpregs;
+ unsigned int acrs[16];
So, if I got this right, userspace is allowed to map this shared read/write
with the kernel? If that's the case then...
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 0badc9f..a7b7ae0 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -269,9 +269,9 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
{
save_fp_regs(vcpu-arch.host_fpregs);
save_access_regs(vcpu-arch.host_acrs);
- vcpu-arch.guest_fpregs.fpc = FPC_VALID_MASK;
- restore_fp_regs(vcpu-arch.guest_fpregs);
- restore_access_regs(vcpu-arch.guest_acrs);
+ vcpu-run-sync_rw.fpregs.fpc = FPC_VALID_MASK;
+ restore_fp_regs(vcpu-run-sync_rw.fpregs);
...this is broken, since userspace can update the floating point control
register contents after the kernel has masked out the offending bits but
before the register is actually loaded.
Which in turn could cause a kernel crash, hm?
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/3] kvm-s390: provide general purpose registers via kvm_run
On Thu, Dec 22, 2011 at 12:56:49PM +0100, Christian Borntraeger wrote:
From: Christian Borntraeger borntrae...@de.ibm.com
The general purpose registers are often necessary to handle SIE exits.
Avoid additional ioctls by providing the guest registers in the r/w
section of the kvm_run structure.
Signed-off-by: Christian Borntraeger borntrae...@de.ibm.com
---
struct sync_rw_regs {
+ __u64 gprs[16]; /* general purpose registers */
FWIW, you should be able to get rid of guest_gprs[] from the
kvm_vcpu_arch structure.
Index: b/arch/s390/include/asm/kvm_host.h
===
Btw. you may want to set QUILT_NO_DIFF_INDEX to get rid of the
Index lines junk :)
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 10/12] [PATCH] kvm-s390: storage key interface
On Thu, Dec 15, 2011 at 11:28:03AM +0100, Carsten Otte wrote:
New version below. Changes:
- __pmdp_for_addr and ptep_for_addr now take a vma as argument
- check if a vma exists has moved to gmap_fault and kvm_s390_keyop
- kvm_s390_keyop verifies that a vma is writable so that it's safe to
set the SWC bit
oh.. cool.
[...]
+ spin_lock(current-mm-page_table_lock);
+ pgste = pgste_get_lock(ptep);
+
+ switch (kop-operation) {
+ case KVM_S390_KEYOP_SSKE:
+ if (!(vma-vm_flags (VM_WRITE | VM_MAYWRITE))) {
+ r = -EACCES;
+ break;
+ }
Why again is this needed? Or put in other words: what prevents a guest to
change the storage key contents via sske of a page that is mapped read-only
into the guest address space?
As far as I can see: nothing. Interestingly I could -in theory- do some nice
stuff like:
- map a file from a read-only filesystem (which doesn't have a writepage
aops function) into guest address space
- let the guest set the change bit in the storage key of a page that belongs
to that file mapping via sske
- watch the fun that happens when the host tries to write the page back
But of course I could be totally wrong ;)
This doesn't have to do anything with your patch, it's just that I think
you shouldn't check if the vma is writable or not. It doesn't matter.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 10/12] [PATCH] kvm-s390: storage key interface
On Thu, Dec 15, 2011 at 05:49:19PM +0100, Christian Borntraeger wrote: On 15/12/11 17:11, Heiko Carstens wrote: Why again is this needed? Or put in other words: what prevents a guest to change the storage key contents via sske of a page that is mapped read-only into the guest address space? As far as I can see: nothing. Interestingly I could -in theory- do some nice stuff like: - map a file from a read-only filesystem (which doesn't have a writepage aops function) into guest address space - let the guest set the change bit in the storage key of a page that belongs to that file mapping via sske - watch the fun that happens when the host tries to write the page back Huh? The guest itself can neither set the dirty bit of the real storage key nor set the dirty bit the host change bit of the pgste via guest SSKE. The transition 0-1 will only be done in the guest change bit of the pgste. (Otherwise we would not have a separate guest/host view of change/referenced) Yeah, I had a major braino.. This interface here is for userspace (to change the guest storage key on behalf of the guest, e.g. for life guest relocation). Since we might have to touch the real storage key and this is host code millicode will not protect us from doing stupid things like it does for guest code, we better check before we touch the real storage key. Or did I misread your question? No, you did not. However, I still think it's wrong to have an early exit if the vma is not writable. Since the guest can set the guest change bit, but it is is not possible with this interface, but I can see now that the purpose was to avoid an overindication of the change bit. oh well... -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 10/12] [PATCH] kvm-s390: storage key interface
On Sat, Dec 10, 2011 at 01:35:39PM +0100, Carsten Otte wrote:
This patch introduces an interface to access the guest visible
storage keys. It supports three operations that model the behavior
that SSKE/ISKE/RRBE instructions would have if they were issued by
the guest. These instructions are all documented in the z architecture
principles of operation book.
Signed-off-by: Carsten Otte co...@de.ibm.com
[...]
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -112,13 +112,115 @@ void kvm_arch_exit(void)
{
}
+static long kvm_s390_keyop(struct kvm_s390_keyop *kop)
+{
+ unsigned long addr = kop-user_addr;
+ pte_t *ptep;
+ pgste_t pgste;
+ int r;
+ unsigned long skey;
+ unsigned long bits;
+
+ /* make sure this process is a hypervisor */
+ r = -EINVAL;
+ if (!mm_has_pgste(current-mm))
+ goto out;
+
+ r = -EFAULT;
+ if (addr = PGDIR_SIZE)
+ goto out;
+
+ spin_lock(current-mm-page_table_lock);
+ ptep = ptep_for_addr(addr);
Locking is broken; following order is possible:
kvm_s390_keyop()- spin_lock(current-mm-page_table_lock)
- ptep_for_addr() - down_read(current-mm-mmap_sem)
--- Bug 1, we might schedule here
- __pmdp_for_addr()
- __pte_alloc()- spin_lock(mm-page_table_lock)
--- Bug 2, deadlock
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 10/12] [PATCH] kvm-s390: storage key interface
On Fri, Dec 09, 2011 at 12:23:36PM +0100, Carsten Otte wrote:
This patch introduces an interface to access the guest visible
storage keys. It supports three operations that model the behavior
that SSKE/ISKE/RRBE instructions would have if they were issued by
the guest. These instructions are all documented in the z architecture
principles of operation book.
Signed-off-by: Carsten Otte co...@de.ibm.com
[...]
+static long kvm_s390_keyop(struct kvm_s390_keyop *kop)
+{
+ unsigned long addr = kop-user_addr;
+ pte_t *ptep;
+ pgste_t pgste;
+ int r;
+ unsigned long skey;
+ unsigned long bits;
+
+ /* make sure this process is a hypervisor */
+ r = -EINVAL;
+ if (!mm_has_pgste(current-mm))
+ goto out;
+
+ r = -ENXIO;
+ if (addr = PGDIR_SIZE)
+ goto out;
imho this should be -EFAULT.
+ spin_lock(current-mm-page_table_lock);
+ ptep = ptep_for_addr(addr);
+ if (!ptep)
+ goto out_unlock;
ptep is a pointer and may contain an error code, like you implemented it
below. Therefore you need to check for IS_ERR() here.
+static pmd_t *__pmdp_for_addr(struct mm_struct *mm, unsigned long addr)
+{
+ struct vm_area_struct *vma;
+ pgd_t *pgd;
+ pud_t *pud;
+ pmd_t *pmd;
+
+ vma = find_vma(mm, addr);
+ if (!vma)
+ return ERR_PTR(-EINVAL);
-EFAULT imho.
Also, why is this check good enough? As far as I remember find_vma() only
guarantees that addr vma_end, (if vma != NULL), but it does not guarantee
that addr = vma_start.
- vma = find_vma(mm, vmaddr);
- if (!vma || vma-vm_start vmaddr)
- return -EFAULT;
... you used to check for that and also used the proper return code, btw.
Or is there a different reason why the above code is correct?
+pte_t *ptep_for_addr(unsigned long addr)
+{
+ pmd_t *pmd;
+ pte_t *rc;
Would you mind renaming rc into pte?
+
+ down_read(current-mm-mmap_sem);
+
+ pmd = __pmdp_for_addr(current-mm, addr);
+ if (IS_ERR(pmd)) {
+ rc = (pte_t *)pmd;
+ goto up_out;
+ }
+
+ rc = pte_offset(pmd, addr);
+up_out:
+ up_read(current-mm-mmap_sem);
+ return rc;
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 10/12] [PATCH] kvm-s390: storage key interface
On Fri, Dec 09, 2011 at 01:49:35PM +0100, Carsten Otte wrote: This patch introduces an interface to access the guest visible storage keys. It supports three operations that model the behavior that SSKE/ISKE/RRBE instructions would have if they were issued by the guest. These instructions are all documented in the z architecture principles of operation book. Signed-off-by: Carsten Otte co...@de.ibm.com --- [...] + spin_lock(current-mm-page_table_lock); + ptep = ptep_for_addr(addr); + if (!ptep) + goto out_unlock; FWIW, this is also a bit odd: if the guest would perform a storage key operation on such an address it would succeed. If the host will do it, it will fail (which doesn't match your description above). No? -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] kvm: add missing void __user * cast to access_ok() call
From: Heiko Carstens heiko.carst...@de.ibm.com fa3d315a KVM: Validate userspace_addr of memslot when registered introduced this new warning onn s390: kvm_main.c: In function '__kvm_set_memory_region': kvm_main.c:654:7: warning: passing argument 1 of '__access_ok' makes pointer from integer without a cast arch/s390/include/asm/uaccess.h:53:19: note: expected 'const void *' but argument is of type '__u64' Add the missing cast to get rid of it again... Cc: Takuya Yoshikawa yoshikawa.tak...@oss.ntt.co.jp Signed-off-by: Heiko Carstens heiko.carst...@de.ibm.com --- virt/kvm/kvm_main.c |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -651,7 +651,8 @@ int __kvm_set_memory_region(struct kvm * /* We can read the guest memory with __xxx_user() later on. */ if (user_alloc ((mem-userspace_addr (PAGE_SIZE - 1)) || -!access_ok(VERIFY_WRITE, mem-userspace_addr, mem-memory_size))) +!access_ok(VERIFY_WRITE, (void __user *)mem-userspace_addr, + mem-memory_size))) goto out; if (mem-slot = KVM_MEMORY_SLOTS + KVM_PRIVATE_MEM_SLOTS) goto out; -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] KVM: fix build warning within __kvm_set_memory_region() on s390
From: Heiko Carstens heiko.carst...@de.ibm.com Get rid of this warning: CC arch/s390/kvm/../../../virt/kvm/kvm_main.o arch/s390/kvm/../../../virt/kvm/kvm_main.c:596:12: warning: 'kvm_create_dirty_bitmap' defined but not used The only caller of the function is within a !CONFIG_S390 section, so add the same ifdef around kvm_create_dirty_bitmap() as well. Signed-off-by: Heiko Carstens heiko.carst...@de.ibm.com --- virt/kvm/kvm_main.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index f29abeb..a762720 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -588,6 +588,7 @@ static int kvm_vm_release(struct inode *inode, struct file *filp) return 0; } +#ifndef CONFIG_S390 /* * Allocation size is twice as large as the actual dirty bitmap size. * This makes it possible to do double buffering: see x86's @@ -608,6 +609,7 @@ static int kvm_create_dirty_bitmap(struct kvm_memory_slot *memslot) memslot-dirty_bitmap_head = memslot-dirty_bitmap; return 0; } +#endif /* !CONFIG_S390 */ /* * Allocate some memory and give it an address in the guest physical address -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] S390: Add virtio hotplug add support
On Tue, Aug 24, 2010 at 03:48:51PM +0200, Alexander Graf wrote:
+static void hotplug_devices(struct work_struct *dummy)
+{
+ unsigned int i;
+ struct kvm_device_desc *d;
+ struct device *dev;
+
+ for (i = 0; i PAGE_SIZE; i += desc_size(d)) {
This should be
for (i = 0; i + desc_size(d) = PAGE_SIZE; i += desc_size(d)) {
otherwise you might have memory accesses beyond the device page...
+ d = kvm_devices + i;
+
+ /* end of list */
+ if (d-type == 0)
+ break;
...even if that should not happen if everything works.
But let's be paranoid.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] S390: Add virtio hotplug add support
On Wed, Aug 25, 2010 at 10:20:03AM +0200, Alexander Graf wrote:
On 25.08.2010, at 10:16, Heiko Carstens wrote:
On Tue, Aug 24, 2010 at 03:48:51PM +0200, Alexander Graf wrote:
+static void hotplug_devices(struct work_struct *dummy)
+{
+ unsigned int i;
+ struct kvm_device_desc *d;
+ struct device *dev;
+
+ for (i = 0; i PAGE_SIZE; i += desc_size(d)) {
This should be
for (i = 0; i + desc_size(d) = PAGE_SIZE; i += desc_size(d)) {
otherwise you might have memory accesses beyond the device page...
Oh, this is a simple copypaste from the original search method.
Is d valid in the first part of the loop already?
No, you would need to initialize it with kvm_devices if you change
the loop.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] S390: Add virtio hotplug add support
On Wed, Aug 25, 2010 at 10:34:29AM +0200, Alexander Graf wrote:
On 25.08.2010, at 10:35, Heiko Carstens wrote:
On Wed, Aug 25, 2010 at 10:20:03AM +0200, Alexander Graf wrote:
On 25.08.2010, at 10:16, Heiko Carstens wrote:
On Tue, Aug 24, 2010 at 03:48:51PM +0200, Alexander Graf wrote:
+static void hotplug_devices(struct work_struct *dummy)
+{
+unsigned int i;
+struct kvm_device_desc *d;
+struct device *dev;
+
+for (i = 0; i PAGE_SIZE; i += desc_size(d)) {
This should be
for (i = 0; i + desc_size(d) = PAGE_SIZE; i += desc_size(d)) {
otherwise you might have memory accesses beyond the device page...
Oh, this is a simple copypaste from the original search method.
Is d valid in the first part of the loop already?
No, you would need to initialize it with kvm_devices if you change
the loop.
But even then it would take the size of the current entry, not the next
one we're actually looking for, no? Maybe it'd be easier to just convert
this into a while loop and explicitly open code everything.
Yes indeed. It's going to be quite ugly if you want to make sure that at
no time you're going to access memory beyond the device page. Eeek.. :)
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] kvm-s390: fix potential array overrun in intercept handling
- if (code 3 || code 0x48) + if (code 3 || (code 2) = ARRAY_SIZE(intercept_funcs)) return -ENOTSUPP; Not that it matters for this patch, but -ENOTSUPP should not leak to userspace. Not sure if it does somewhere, but it is used all over the place within arch/s390/kvm... Use -EOPNOTSUPP or something similar instead. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] kvm: get rid of unused label warning
From: Heiko Carstens heiko.carst...@de.ibm.com arch/s390/kvm/../../../virt/kvm/kvm_main.c: In function 'kvm_create_vm': arch/s390/kvm/../../../virt/kvm/kvm_main.c:409: warning: label 'out_err' defined but not used Signed-off-by: Heiko Carstens heiko.carst...@de.ibm.com --- Adds even more ifdefery... virt/kvm/kvm_main.c |3 +++ 1 file changed, 3 insertions(+) --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -406,8 +406,11 @@ static struct kvm *kvm_create_vm(void) out: return kvm; +#if defined(KVM_COALESCED_MMIO_PAGE_OFFSET) || \ +(defined(CONFIG_MMU_NOTIFIER) defined(KVM_ARCH_WANT_MMU_NOTIFIER)) out_err: hardware_disable_all(); +#endif out_err_nodisable: kfree(kvm); return ERR_PTR(r); -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] kvm: fix compile warnings on s390
From: Heiko Carstens heiko.carst...@de.ibm.com
CC arch/s390/kvm/../../../virt/kvm/kvm_main.o
arch/s390/kvm/../../../virt/kvm/kvm_main.c: In function
'__kvm_set_memory_region':
arch/s390/kvm/../../../virt/kvm/kvm_main.c:485: warning: unused variable 'j'
arch/s390/kvm/../../../virt/kvm/kvm_main.c:484: warning: unused variable
'lpages'
arch/s390/kvm/../../../virt/kvm/kvm_main.c:483: warning: unused variable 'ugfn'
Cc: Carsten Otte co...@de.ibm.com
Signed-off-by: Heiko Carstens heiko.carst...@de.ibm.com
---
Patch applies on top of linux-next.
virt/kvm/kvm_main.c |8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
Index: linux-next/virt/kvm/kvm_main.c
===
--- linux-next.orig/virt/kvm/kvm_main.c
+++ linux-next/virt/kvm/kvm_main.c
@@ -480,9 +480,8 @@ int __kvm_set_memory_region(struct kvm *
{
int r;
gfn_t base_gfn;
- unsigned long npages, ugfn;
- int lpages;
- unsigned long i, j;
+ unsigned long npages;
+ unsigned long i;
struct kvm_memory_slot *memslot;
struct kvm_memory_slot old, new;
@@ -560,6 +559,9 @@ int __kvm_set_memory_region(struct kvm *
goto skip_lpage;
for (i = 0; i KVM_NR_PAGE_SIZES - 1; ++i) {
+ unsigned long ugfn;
+ unsigned long j;
+ int lpages;
int level = i + 2;
/* Avoid unused variable warning if no large pages */
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] show hypervisor information on sysfs
On Tue, 10 Feb 2009 11:20:23 -0200 Glauber Costa glom...@redhat.com wrote: Anyway, just wanted to make you aware of what might come next. Why wouldn't it be extensible? So far, I've only added an attribute. But we can easily add others, and directories with even more attributes if we must. Oh, sure it's easily extensible, the question is if the first attribute you now add will be at the right place. That is: is all information about the 1st level hypervisor supposed to be found in /sys/hypervisor or should it be in a subdirectory? E.g. /sys/hypervisor/level1 or something like that. But adding your attribute won't hurt. The worst case is that it gets duplicated. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] show hypervisor information on sysfs
On Fri, 6 Feb 2009 10:46:57 -0500
Glauber Costa glom...@redhat.com wrote:
It is useful to easily grab information about whether or not
we're running on top of a hypervisor. And in case affirmative,
which one.
This patch shows it in /sys/hypervisor (and as a site effect, allow
it to be directly selectable).
Not really an objection, but is this really easily extensible? Next
thing people are going to ask: how many cpus does my hypervisor have?
Is my hypervisor running itself within a hypervisor and how many cpus
does that have? etc.
Maybe some directory structure for each hypervisor level would be nice?
Just to get a feeling for what will come up next: on s390 we have the
file /proc/sysinfo which contains a lot of information about all
underlying hypervisors:
Manufacturer: IBM
Type: 2097
Model:724 E26
Sequence Code:0003C03F
Plant:02
Model Capacity: 724 1748
Model Perm. Capacity: 724 1748
Model Temp. Capacity: 724 1748
CPUs Total: 28
CPUs Configured: 24
CPUs Standby: 0
CPUs Reserved:4
Capability: 904 1350
Adjustment 02-way:61500 61500
Adjustment 03-way:59260 59500
Adjustment 04-way:57480 57300
Adjustment 05-way:55800 55700
Adjustment 06-way:54700 54300
Adjustment 07-way:53600 53100
Adjustment 08-way:52500 52040
Adjustment 09-way:51200 51000
Adjustment 10-way:50100 5
Adjustment 11-way:49000 49020
Adjustment 12-way:48000 48120
Adjustment 13-way:47200 47300
Adjustment 14-way:46400 46500
Adjustment 15-way:45600 45800
Adjustment 16-way:44800 45200
Adjustment 17-way:44300 44680
Adjustment 18-way:43900 44120
Adjustment 19-way:43500 43640
Adjustment 20-way:43100 43200
Adjustment 21-way:42600 42740
Adjustment 22-way:42300 42360
Adjustment 23-way:42000 41980
Adjustment 24-way:41700 41600
Adjustment 25-way:41400 41240
Adjustment 26-way:41100 40960
Adjustment 27-way:40800 40680
Adjustment 28-way:40400 40360
Secondary Capability: 904
LPAR Number: 47
LPAR Characteristics: Shared
LPAR Name:H42LP45
LPAR Adjustment: 500
LPAR CPUs Total: 16
LPAR CPUs Configured: 12
LPAR CPUs Standby:4
LPAR CPUs Reserved: 0
LPAR CPUs Dedicated: 0
LPAR CPUs Shared: 12
VM00 Name:H4245004
VM00 Control Program: z/VM5.3.0
VM00 Adjustment: 250
VM00 CPUs Total: 3
VM00 CPUs Configured: 3
VM00 CPUs Standby:0
VM00 CPUs Reserved: 0
Besides a lot of other stuff this tells you that my guest is running on
28 cpu machine (24 cpus active), that my guest is running in logical
partition 47, which has 16 cpus of which 12 are used. Within that
logical partition my hypervisor is running (z/VM 5.3.0). And finally it
tells you my guest (H4245004) has three cpus (cpus that you're going to
see in /proc/cpuinfo).
Oh, and before somebody asks: yes, we do have an instruction to get all
of this information ;)
Anyway, just wanted to make you aware of what might come next.
int __init hypervisor_init(void)
{
+ int ret;
hypervisor_kobj = kobject_create_and_add(hypervisor, NULL);
if (!hypervisor_kobj)
return -ENOMEM;
+
+ ret = sysfs_create_file(hypervisor_kobj, hyper_name_attr.attr);
+ if (ret) {
+ printk(KERN_WARNING could not create hyper_name file\n);
+ return ret;
+ }
+
return 0;
}
why not simply:
return sysfs_create_file(hypervisor_kobj, hyper_name_attr.attr);
If it fails the sysfs code hopefully will dump something on the
console. We shouldn't add a printk for each and every sysfs_create_file
that might fail. It's just overkill.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] kvm-s390: Fix printk on SIGP set arch
On Thu, 22 Jan 2009 10:27:38 +0100 Christian Borntraeger borntrae...@de.ibm.com wrote: KVM on s390 does not support the ESA/390 architecture. We refuse to change the architecture mode and print a warning. While testing a crashme for kvm, I spotted two problems with the printk: o A malicious can flood host dmesg o There was no newline at the end of the printk This patch fixes both problems. [...] - printk(KERN_WARNING kvm: request to switch to ESA/390 mode - not supported); + if (printk_ratelimit()) + printk(KERN_WARNING kvm: request to switch to ESA/390 + mode not supported\n); Why don't you just remove the printk? IMHO it's rather pointless. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] kvm-s390: Fix printk on SIGP set arch
On Thu, 22 Jan 2009 12:26:11 +0100 Carsten Otte co...@de.ibm.com wrote: Am Thu, 22 Jan 2009 12:17:07 +0100 schrieb heica...@linux.vnet.ibm.com: Why don't you just remove the printk? IMHO it's rather pointless. It is'nt: It infoms the user why his guest is going to crash even though it has performed an operation that is perfectly legal according to the spec. If you have hundreds of guests running, how do you get the connection from this message to a specific user process? Informing a user process that it did something that isn't allowed or supported is usually done by returning an appropriate return code. Also, if you have one evil process and hit the prink_limit the messages for all other processes will likely be lost anyway. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
