Re: [Qemu-devel] [PATCH v2] qemu-kvm/pci-assign: 64 bits bar emulation

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 2012-12-20 at 01:52 +0000, Hao, Xudong wrote:
> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Thursday, December 20, 2012 12:06 AM
> > To: Hao, Xudong
> > Cc: qemu-devel@nongnu.org; mtosatti@redhat.com; gleb@redhat.com;
> > kvm@vger.kernel.org
> > Subject: Re: [PATCH v2] qemu-kvm/pci-assign: 64 bits bar emulation
> > 
> > On Wed, 2012-12-19 at 16:47 +0800, Xudong Hao wrote:
> > > Enable 64 bits bar emulation.
> > >
> > > v2 changes from v1:
> > > - Change 0lx% to 0x%016 when print a 64 bit variable.
> > >
> > > Test pass with the current seabios which already support 64bit pci bars.
> > >
> > > Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> > > ---
> > >  hw/kvm/pci-assign.c |   22 ++++++++++++++--------
> > >  1 files changed, 14 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/hw/kvm/pci-assign.c b/hw/kvm/pci-assign.c
> > > index 7a0998c..fb58ca9 100644
> > > --- a/hw/kvm/pci-assign.c
> > > +++ b/hw/kvm/pci-assign.c
> > > @@ -46,6 +46,7 @@
> > >  #define IORESOURCE_IRQ      0x00000400
> > >  #define IORESOURCE_DMA      0x00000800
> > >  #define IORESOURCE_PREFETCH 0x00002000  /* No side effects */
> > > +#define IORESOURCE_MEM_64   0x00100000
> > >
> > >  //#define DEVICE_ASSIGNMENT_DEBUG
> > >
> > > @@ -442,9 +443,13 @@ static int assigned_dev_register_regions(PCIRegion
> > *io_regions,
> > >
> > >          /* handle memory io regions */
> > >          if (cur_region->type & IORESOURCE_MEM) {
> > > -            int t = cur_region->type & IORESOURCE_PREFETCH
> > > -                ? PCI_BASE_ADDRESS_MEM_PREFETCH
> > > -                : PCI_BASE_ADDRESS_SPACE_MEMORY;
> > > +            int t = PCI_BASE_ADDRESS_SPACE_MEMORY;
> > > +            if (cur_region->type & IORESOURCE_PREFETCH) {
> > > +                t |= PCI_BASE_ADDRESS_MEM_PREFETCH;
> > > +            }
> > > +            if (cur_region->type & IORESOURCE_MEM_64) {
> > > +                t |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> > > +            }
> > >
> > >              /* map physical memory */
> > >              pci_dev->v_addrs[i].u.r_virtbase = mmap(NULL,
> > cur_region->size,
> > > @@ -468,10 +473,10 @@ static int
> > assigned_dev_register_regions(PCIRegion *io_regions,
> > >                  (cur_region->base_addr & 0xFFF);
> > >
> > >              if (cur_region->size & 0xFFF) {
> > > -                error_report("PCI region %d at address 0x%" PRIx64 "
> > has "
> > > -                             "size 0x%" PRIx64 ", which is not a
> > multiple of "
> > > -                             "4K.  You might experience some
> > performance hit "
> > > -                             "due to that.",
> > > +                error_report("PCI region %d at address 0x%016" PRIx64
> > " has "
> > > +                             "size 0x%016" PRIx64 ", which is not a
> > multiple "
> > > +                             "of 4K.  You might experience some
> > performance "
> > > +                             "hit due to that.",
> > 
> > nit, these changes to %016 don't make sense.  If the size is not a
> > multiple of 4k, then it's less than 4k, so adding leading zeros is just
> > a waste.  Also, are BARs that small likely to be 64bit?  Seems like not,
> > so more unnecessary zeros.  Thanks,
> > 
> 
> Alex,
> You're right for this error_report print less than 4k. So your comments is removing 016 and just remain the original code is fine, right?

Yes, I'd just drop that chunk and leave the original error string.
Thanks,

Alex