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 > > 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: # 3.18+: 69a8d4562638 KVM: s390: no need to > hold... > > Reviewed-by: David Hildenbrand > Signed-off-by: Jens Freimann > Signed-off-by: Christian Borntraeger > Acked-by: Cornelia Huck ... > -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 > > 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 > Acked-by: Cornelia Huck > Signed-off-by: Christian Borntraeger > --- > 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 > >> Reviewed-by: David Hildenbrand > >> --- > >> 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, ®1, ®2); > + 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 > --- [...] > +#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 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 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 > > --- > > 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 [...] > +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 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 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 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 --- 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 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 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 > > 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 > --- > 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 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 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(¤t->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 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 [...] > --- 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(¤t->mm->page_table_lock); > + ptep = ptep_for_addr(addr); Locking is broken; following order is possible: kvm_s390_keyop()- spin_lock(¤t->mm->page_table_lock) -> ptep_for_addr() - down_read(¤t->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 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 > --- [...] > + spin_lock(¤t->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
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 [...] > +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(¤t->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(¤t->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(¤t->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
[PATCH] kvm: add missing void __user * cast to access_ok() call
From: Heiko Carstens 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 Signed-off-by: Heiko Carstens --- 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 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 --- 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 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 copy&paste 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: [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 copy&paste 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 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: [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 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 --- 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 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 Signed-off-by: Heiko Carstens --- 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 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 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 12:26:11 +0100 Carsten Otte 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
Re: [PATCH 1/3] kvm-s390: Fix printk on SIGP set arch
On Thu, 22 Jan 2009 10:27:38 +0100 Christian Borntraeger 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