From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sheng Yang Subject: Re: [PATCH 7/7] KVM: assigned dev: Big endian support for MSI-X MMIO Date: Fri, 12 Nov 2010 10:47:06 +0800 Message-ID: <201011121047.06833.sheng@linux.intel.com> References: <1289461620-7055-1-git-send-email-sheng@linux.intel.com> <1289461620-7055-8-git-send-email-sheng@linux.intel.com> <20101111161221.GK25692@redhat.com> Mime-Version: 1.0 Content-Type: Text/Plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: Avi Kivity , Marcelo Tosatti , kvm@vger.kernel.org To: "Michael S. Tsirkin" Return-path: Received: from mga02.intel.com ([134.134.136.20]:33063 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753787Ab0KLCqz (ORCPT ); Thu, 11 Nov 2010 21:46:55 -0500 In-Reply-To: <20101111161221.GK25692@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Friday 12 November 2010 00:12:21 Michael S. Tsirkin wrote: > On Thu, Nov 11, 2010 at 03:47:00PM +0800, Sheng Yang wrote: > > Add marco for big-endian machine.(Untested!) > > > > Signed-off-by: Sheng Yang > > I presume this is tested at the same level as the previous patch? > So you want to fold this into the previous patch. Of course tested on Intel's platform, but it didn't make sense for big-endian patch... I'd like to make it separate still, because I can't test it properly. > > Also, please build with sparse (C=1) to find some endian-ness issues. Sure. (Have it run with the patches, nothing new found...) > > > --- > > > > virt/kvm/assigned-dev.c | 7 ++++--- > > 1 files changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c > > index 3010d7d..15b5f74 100644 > > --- a/virt/kvm/assigned-dev.c > > +++ b/virt/kvm/assigned-dev.c > > @@ -848,9 +848,9 @@ static int msix_mmio_read(struct kvm_io_device *this, > > gpa_t addr, int len, > > > > idx = (addr - adev->msix_mmio_base) / PCI_MSIX_ENTRY_SIZE; > > if ((addr % PCI_MSIX_ENTRY_SIZE) == > > > > PCI_MSIX_ENTRY_VECTOR_CTRL) > > > > - *(unsigned long *)val = > > + *(unsigned long *)val = le32_to_cpu( > > This should be cpu_to_le32. And val cast to __le32. I think we can just cast val to __le32 here? The result should be big-endian. No, no, I suppose all parameter of msix_mmio_read/write should be little-endian because it's being written into PCI MSI-X table? > > > test_bit(idx, adev->msix_mask_bitmap) ? > > > > - PCI_MSIX_ENTRY_CTRL_MASKBIT : 0; > > + PCI_MSIX_ENTRY_CTRL_MASKBIT : 0); > > > > else > > > > r = -EOPNOTSUPP; > > > > goto out; > > In this function, > entry must be __le32 too, and fix up endian-ness where you fill it in. > > > @@ -869,6 +869,7 @@ static int msix_mmio_read(struct kvm_io_device *this, > > gpa_t addr, int len, > > > > adev->msix_mask_bitmap); > > > > memcpy(val, &entry[addr % PCI_MSIX_ENTRY_SIZE / sizeof *entry], len); > > > > + *(unsigned long *)val = le32_to_cpu(*(unsigned long *)val); > > > > out: > > mutex_unlock(&adev->kvm->lock); > > return r; > > > > @@ -881,7 +882,7 @@ static int msix_mmio_write(struct kvm_io_device > > *this, gpa_t addr, int len, > > > > container_of(this, struct kvm_assigned_dev_kernel, > > > > msix_mmio_dev); > > > > int idx, r = 0; > > > > - unsigned long new_val = *(unsigned long *)val; > > + unsigned long new_val = cpu_to_le32(*(unsigned long *)val); > > __le32 Should it be unsigned long new_val = le32_to_cpu(*(unsigned long *)val); *val should be little endian I guess? Or Michael, could you provide a patch for this? I really don't think I can guarantee correctness of this patch... -- regards Yang, Sheng > > > mutex_lock(&adev->kvm->lock); > > if (!msix_mmio_in_range(adev, addr, len)) {