Re: [PATCH 7/7] KVM: assigned dev: Big endian support for MSI-X MMIO

[Sheng Yang <sheng@linux.intel.com>]

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 <sheng@linux.intel.com>
> 
> 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)) {