From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [patch 4/4] KVM: allow direct access to PMTimer port
Date: Sun, 25 May 2008 13:09:26 -0300
Message-ID: <20080525160926.GA16012@dmt>
References: <20080524234342.983197667@localhost.localdomain> <20080525000036.710955693@localhost.localdomain> <483939B8.5080304@qumranet.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Chris Wright <chrisw@redhat.com>, kvm-devel <kvm@vger.kernel.org>,
	Len Brown <lenb@kernel.org>
To: Avi Kivity <avi@qumranet.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([66.187.233.31]:56131 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752512AbYEYQKI (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 25 May 2008 12:10:08 -0400
Content-Disposition: inline
In-Reply-To: <483939B8.5080304@qumranet.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Sun, May 25, 2008 at 01:04:40PM +0300, Avi Kivity wrote:
> >@@ -424,6 +425,10 @@ static __init int svm_hardware_setup(void)
> > 	iopm_va = page_address(iopm_pages);
> > 	memset(iopm_va, 0xff, PAGE_SIZE * (1 << IOPM_ALLOC_ORDER));
> > 	clear_bit(0x80, iopm_va); /* allow direct access to PC debug port */
> >+	if (pmtmr_ioport)
> >+		for (r = 0; r < 4; r++)
> >+			clear_bit(pmtmr_ioport+r, iopm_va);
> >+            
> >  
> 
> What if the port conflicts with a virtualized port?

Userspace registers the port range in QEMU even if it can be accessed
directly, so if there is a conflict it can be handled there.

> >+	if (pmtmr_ioport)
> >+		for (r = 0; r < 4; r++)
> >+			clear_bit(pmtmr_ioport+r, va);
> >+
> >  
> 
> Instead of duplicating the code, how about an kvm_x86_ops interface?

Yes sounds much better.

> >+	case KVM_GET_PMTIMER: {
> >+		struct kvm_pmtimer pmtmr;
> >+		r = -ENODEV;
> >+		if (!pmtmr_ioport)
> >+			goto out;
> >+		pmtmr.val = inl(pmtmr_ioport);
> >+		r = -EFAULT;
> >+		if (copy_to_user(argp, &pmtmr, sizeof pmtmr))
> >+			goto out;
> >+		r = 0;
> >+		break;
> >+	}
> > 	default:
> >  
> 
> This definitely doesn't belong in kvm.  /dev/pmtimer?

Think so.