[Qemu-devel] Re: [PATCH 12/25] pci: 64bit bar support.

[Isaku Yamahata]

On Sun, Oct 04, 2009 at 12:26:46PM +0200, Michael S. Tsirkin wrote:
> On Sat, Oct 03, 2009 at 05:16:04AM +0900, Isaku Yamahata wrote:
> > implemented pci 64bit bar support.
> > 
> > Signed-off-by: Isaku Yamahata <address@hidden>
> > ---
> >  hw/pci.c |   41 ++++++++++++++++++++++++++++++++++++-----
> >  hw/pci.h |    9 +++++++++
> >  2 files changed, 45 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 510fad2..75af2cd 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -427,8 +427,13 @@ void pci_register_bar(PCIDevice *pci_dev, int 
> > region_num,
> >          addr = 0x10 + region_num * 4;
> >      }
> >      pci_set_long(pci_dev->config + addr, type);
> > -    pci_set_long(pci_dev->wmask + addr, wmask & 0xffffffff);
> > -    pci_set_long(pci_dev->cmask + addr, 0xffffffff);
> > +    if (pci_bar_is_64bit(r)) {
> > +        pci_set_quad(pci_dev->wmask + addr, wmask);
> > +        pci_set_quad(pci_dev->cmask + addr, ~0ULL);
> > +    } else {
> > +        pci_set_long(pci_dev->wmask + addr, wmask & 0xffffffff);
> > +        pci_set_long(pci_dev->cmask + addr, 0xffffffff);
> > +    }
> >  }
> >  
> >  static void pci_update_mappings(PCIDevice *d)
> > @@ -462,7 +467,11 @@ static void pci_update_mappings(PCIDevice *d)
> >                  }
> >              } else {
> >                  if (cmd & PCI_COMMAND_MEMORY) {
> > -                    new_addr = pci_get_long(d->config + config_ofs);
> > +                    if (pci_bar_is_64bit(r)) {
> > +                        new_addr = pci_get_quad(d->config + config_ofs);
> > +                    } else {
> > +                        new_addr = pci_get_long(d->config + config_ofs);
> > +                    }
> >                      /* the ROM slot has a specific enable bit */
> >                      if (i == PCI_ROM_SLOT && !(new_addr & 1))
> >                          goto no_mem_map;
> > @@ -477,7 +486,7 @@ static void pci_update_mappings(PCIDevice *d)
> >  
> >                          /* keep old behaviour
> >                           * without this, PC ide doesn't work well. */
> > -                        last_addr >= UINT32_MAX) {
> > +                        (!pci_bar_is_64bit(r) && last_addr >= UINT32_MAX)) 
> > {
> >                          new_addr = PCI_BAR_UNMAPPED;
> 
> I don't really understand what is this doing

I'll add a comment above the line in the patch.

In a word, it is a work around of qemu implementation limitation.
So far bar was only 32bit, just wrapping around check worked.
But now it's 64bit, explicit 4G check becomes needed.
OS writes all one bits to BAR register to detect its size
meaning that memory 32 bit bar is once set right below 4GB,
and then maps the area to where OS really want it to exist.
Since mapping memory space at 4G causes troubles,
pci_update_mapping() have to work around not to map right below 4G.


> >                      }
> >                  } else {
> > @@ -736,7 +745,29 @@ static void pci_info_device(PCIDevice *d)
> >                  monitor_printf(mon, "I/O at 0x%04"FMT_pcibus" 
> > [0x%04"FMT_pcibus"].\n",
> >                                 r->addr, r->addr + r->size - 1);
> >              } else {
> > -                monitor_printf(mon, "32 bit memory at 0x%08"FMT_pcibus" 
> > [0x%08"FMT_pcibus"].\n",
> > +                const char *type;
> > +                const char* prefetch;
> > +
> > +                switch (r->type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) {
> > +                case PCI_ADDRESS_SPACE_MEM:
> > +                    type = "32 bit";
> > +                    break;
> > +                case PCI_ADDRESS_SPACE_MEM_64:
> > +                    type = "64 bit";
> > +                    break;
> > +                default:
> > +                    type = "unknown";
> > +                    break;
> 
> this default is just dead code?
> How about type = is_64bit() ? "64" : "32"
> 
> > +                }
> > +
> > +                prefetch = "";
> > +                if (r->type & PCI_ADDRESS_SPACE_MEM_PREFETCH) {
> > +                    prefetch = " prefetchable";
> > +                }
> > +
> > +                monitor_printf(mon, "%s%s memory at "
> > +                               "0x%08"FMT_pcibus" [0x%08"FMT_pcibus"].\n",
> > +                               type, prefetch,
> >                                 r->addr, r->addr + r->size - 1);
> >              }
> >          }
> > diff --git a/hw/pci.h b/hw/pci.h
> > index c209f34..0455b30 100644
> > --- a/hw/pci.h
> > +++ b/hw/pci.h
> > @@ -84,6 +84,8 @@ typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
> >  
> >  #define PCI_ADDRESS_SPACE_MEM              0x00
> >  #define PCI_ADDRESS_SPACE_IO               0x01
> > +#define PCI_ADDRESS_SPACE_MEM_64        0x04    /* 64 bit address */
> 
> PCI_ADDRESS_MEM_TYPE_64 is a better name: it's a type of memory,
> not a separate address space.
> 
> Actually, we should probably just reuse macros from pci_regs.h
> and put them with rest of macros imported from there.
> The list of relevant macros (prefixes with // these that we don't use
> and probably shouldn't put in header):
> 
>       #define  PCI_BASE_ADDRESS_SPACE         0x01    /* 0 = memory, 1 = I/O 
> */
>       #define  PCI_BASE_ADDRESS_SPACE_IO      0x01
>       #define  PCI_BASE_ADDRESS_SPACE_MEMORY  0x00
>       //#define  PCI_BASE_ADDRESS_MEM_TYPE_MASK       0x06
>       //#define  PCI_BASE_ADDRESS_MEM_TYPE_32 0x00    /* 32 bit address */
>       //#define  PCI_BASE_ADDRESS_MEM_TYPE_1M 0x02    /* Below 1M [obsolete] 
> */
>       #define  PCI_BASE_ADDRESS_MEM_TYPE_64   0x04    /* 64 bit address */
>       #define  PCI_BASE_ADDRESS_MEM_PREFETCH  0x08    /* prefetchable? */
> 
> 
> > +#define PCI_ADDRESS_SPACE_MEM_TYPE_MASK 0x06
> 
> This one is used in pci_regs.h to include 64 bit an 1M.
> what do you use it for?
> It seems unnecessary.

Will drop unused ones.


> >  #define PCI_ADDRESS_SPACE_MEM_PREFETCH     0x08
> >  
> >  typedef struct PCIIORegion {
> > @@ -94,6 +96,13 @@ typedef struct PCIIORegion {
> >      PCIMapIORegionFunc *map_func;
> >  } PCIIORegion;
> >  
> > +static inline int pci_bar_is_64bit(const PCIIORegion *r)
> 
> Is it used outside pci.c? If no, do not put in pci.h
> 
> > +{
> > +    return !(r->type & PCI_ADDRESS_SPACE_IO) &&
> > +        ((r->type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) ==
> > +         PCI_ADDRESS_SPACE_MEM_64);
> 
> This is probably wrong: we should only care about the MEM_64 bit.
> 
> Why don't we just return (r->type & PCI_BASE_ADDRESS_MEM_TYPE_64)?
> And this function is simple enough to be open-coded then.

Will use PCI_ADDRESS_MEM_TYPE_64.
Since it's not a bit,
(>type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) == PCI_ADDRESS_SPACE_MEM_TYPE_64
makes sense.

> Nit: we don't need () around the == comparison.
> 
> > +}
> > +
> >  #define PCI_ROM_SLOT 6
> >  #define PCI_NUM_REGIONS 7
> >  
> 

-- 
yamahata