diff mbox

[v2] qemu/xen: Add 64 bits big bar support on qemu

Message ID 1348544326-31334-1-git-send-email-xudong.hao@intel.com
State New
Headers show

Commit Message

Hao, Xudong Sept. 25, 2012, 3:38 a.m. UTC 
Changes from v1:
- Rebase to qemu upstream from qemu-xen

Currently it is assumed PCI device BAR access < 4G memory. If there is such a
device whose BAR size is larger than 4G, it must access > 4G memory address.
This patch enable the 64bits big BAR support on qemu.

Signed-off-by: Xudong Hao <xudong.hao@intel.com>
Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
---
 hw/xen_pt.c             |   16 ++++++++--------
 hw/xen_pt_config_init.c |   42 +++++++++++++++++++++++++++++-------------

Comments

Stefano Stabellini Sept. 25, 2012, 10:52 a.m. UTC | #1 
On Tue, 25 Sep 2012, Xudong Hao wrote:
> Changes from v1:
> - Rebase to qemu upstream from qemu-xen

Thanks. Please run scripts/checkpatch.pl on this patch, you'll find
some cody style issues that need to be fixed.


> Currently it is assumed PCI device BAR access < 4G memory. If there is such a
> device whose BAR size is larger than 4G, it must access > 4G memory address.
> This patch enable the 64bits big BAR support on qemu.
> 
> Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
> ---
>  hw/xen_pt.c             |   16 ++++++++--------
>  hw/xen_pt_config_init.c |   42 +++++++++++++++++++++++++++++-------------
> 
> diff --git a/hw/xen_pt.c b/hw/xen_pt.c
> index 307119a..2a8bcf3 100644
> --- a/hw/xen_pt.c
> +++ b/hw/xen_pt.c
> @@ -403,21 +403,21 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s)
>  
>          s->bases[i].access.u = r->base_addr;
>  
> -        if (r->type & XEN_HOST_PCI_REGION_TYPE_IO) {
> +        if (r->type & XEN_HOST_PCI_REGION_TYPE_IO)
>              type = PCI_BASE_ADDRESS_SPACE_IO;
> -        } else {
> +        else if (r->type & XEN_HOST_PCI_REGION_TYPE_MEM_64)
> +            type = PCI_BASE_ADDRESS_MEM_TYPE_64;
> +        else if (r->type & XEN_HOST_PCI_REGION_TYPE_PREFETCH)
> +            type |= PCI_BASE_ADDRESS_MEM_PREFETCH;
> +        else
>              type = PCI_BASE_ADDRESS_SPACE_MEMORY;
> -            if (r->type & XEN_HOST_PCI_REGION_TYPE_PREFETCH) {
> -                type |= PCI_BASE_ADDRESS_MEM_PREFETCH;
> -            }
> -        }

Aside from the cody style issue here, this changes the behavior for type
PCI_BASE_ADDRESS_SPACE_MEMORY. Before we could have:

type = PCI_BASE_ADDRESS_SPACE_MEMORY|PCI_BASE_ADDRESS_MEM_PREFETCH;

now we cannot anymore.


>          memory_region_init_io(&s->bar[i], &ops, &s->dev,
>                                "xen-pci-pt-bar", r->size);
>          pci_register_bar(&s->dev, i, type, &s->bar[i]);
>  
> -        XEN_PT_LOG(&s->dev, "IO region %i registered (size=0x%08"PRIx64
> -                   " base_addr=0x%08"PRIx64" type: %#x)\n",
> +        XEN_PT_LOG(&s->dev, "IO region %i registered (size=0x%lx"PRIx64
> +                   " base_addr=0x%lx"PRIx64" type: %#x)\n",
>                     i, r->size, r->base_addr, type);
>      }
>  
> diff --git a/hw/xen_pt_config_init.c b/hw/xen_pt_config_init.c
> index e524a40..5e7ca22 100644
> --- a/hw/xen_pt_config_init.c
> +++ b/hw/xen_pt_config_init.c
> @@ -342,6 +342,7 @@ static int xen_pt_cmd_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
>  #define XEN_PT_BAR_IO_RO_MASK     0x00000003  /* BAR ReadOnly mask(I/O) */
>  #define XEN_PT_BAR_IO_EMU_MASK    0xFFFFFFFC  /* BAR emul mask(I/O) */
>  
> +static uint64_t xen_pt_get_bar_size(PCIIORegion *r);

there is just one user of xen_pt_get_bar_size, maybe you can just
move the implementation here


>  static XenPTBarFlag xen_pt_bar_reg_parse(XenPCIPassthroughState *s,
>                                           XenPTRegInfo *reg)
>  {
> @@ -366,7 +367,7 @@ static XenPTBarFlag xen_pt_bar_reg_parse(XenPCIPassthroughState *s,
>  
>      /* check unused BAR */
>      r = &d->io_regions[index];
> -    if (r->size == 0) {
> +    if (!xen_pt_get_bar_size(r)) {
>          return XEN_PT_BAR_FLAG_UNUSED;
>      }
>  
> @@ -383,6 +384,24 @@ static XenPTBarFlag xen_pt_bar_reg_parse(XenPCIPassthroughState *s,
>      }
>  }
>  
> +static bool is_64bit_bar(PCIIORegion *r)
> +{
> +    return !!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64);
> +}
> +
> +static uint64_t xen_pt_get_bar_size(PCIIORegion *r)
> +{
> +    if (is_64bit_bar(r))
> +    {
> +        uint64_t size64;
> +        size64 = (r + 1)->size; 
> +        size64 <<= 32; 
> +        size64 += r->size;
> +        return size64; 
> +    }
> +    return r->size; 
> +}
> +
>  static inline uint32_t base_address_with_flags(XenHostPCIIORegion *hr)
>  {
>      if (hr->type & XEN_HOST_PCI_REGION_TYPE_IO) {
> @@ -481,7 +500,10 @@ static int xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
>      switch (s->bases[index].bar_flag) {
>      case XEN_PT_BAR_FLAG_MEM:
>          bar_emu_mask = XEN_PT_BAR_MEM_EMU_MASK;
> -        bar_ro_mask = XEN_PT_BAR_MEM_RO_MASK | (r_size - 1);
> +        if (!r_size)
> +            bar_ro_mask = XEN_PT_BAR_ALLF;
> +        else
> +            bar_ro_mask = XEN_PT_BAR_MEM_RO_MASK | (r_size - 1);
>          break;

Is this an actual mistake everywhere?
There is at least another instance of

bar_ro_mask = something | (r_size - 1);

under the XEN_PT_BAR_FLAG_IO case.
Or is it only a problem with 64 bit bars? If so, could you please
explain why?


>      case XEN_PT_BAR_FLAG_IO:
>          bar_emu_mask = XEN_PT_BAR_IO_EMU_MASK;
> @@ -489,7 +511,10 @@ static int xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
>          break;
>      case XEN_PT_BAR_FLAG_UPPER:
>          bar_emu_mask = XEN_PT_BAR_ALLF;
> -        bar_ro_mask = 0;    /* all upper 32bit are R/W */
> +        if (!r_size)
> +            bar_ro_mask = 0;
> +        else
> +            bar_ro_mask = r_size - 1;
>          break;

bar_ro_mask = r_size ? r_size - 1 : 0;


>      default:
>          break;
> @@ -501,22 +526,13 @@ static int xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
>  
>      /* check whether we need to update the virtual region address or not */
>      switch (s->bases[index].bar_flag) {
> +    case XEN_PT_BAR_FLAG_UPPER:
>      case XEN_PT_BAR_FLAG_MEM:
>          /* nothing to do */
>          break;
>      case XEN_PT_BAR_FLAG_IO:
>          /* nothing to do */
>          break;
> -    case XEN_PT_BAR_FLAG_UPPER:
> -        if (cfg_entry->data) {
> -            if (cfg_entry->data != (XEN_PT_BAR_ALLF & ~bar_ro_mask)) {
> -                XEN_PT_WARN(d, "Guest attempt to set high MMIO Base Address. "
> -                            "Ignore mapping. "
> -                            "(offset: 0x%02x, high address: 0x%08x)\n",
> -                            reg->offset, cfg_entry->data);
> -            }
> -        }
> -        break;
>      default:
>          break;
>      }
> -- 
> 1.5.5
>
Hao, Xudong Sept. 26, 2012, 8:24 a.m. UTC | #2 
> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Tuesday, September 25, 2012 6:52 PM
> To: Hao, Xudong
> Cc: Stefano Stabellini; xen-devel@lists.xen.org; qemu-devel@nongnu.org;
> Zhang, Xiantao
> Subject: Re: [PATCH v2] qemu/xen: Add 64 bits big bar support on qemu
> 
> On Tue, 25 Sep 2012, Xudong Hao wrote:
> > Changes from v1:
> > - Rebase to qemu upstream from qemu-xen
> 
> Thanks. Please run scripts/checkpatch.pl on this patch, you'll find
> some cody style issues that need to be fixed.
> 
OK, will use this scripts to check code style.

> 
> > Currently it is assumed PCI device BAR access < 4G memory. If there is such a
> > device whose BAR size is larger than 4G, it must access > 4G memory
> address.
> > This patch enable the 64bits big BAR support on qemu.
> >
> > Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> > Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
> > ---
> >  hw/xen_pt.c             |   16 ++++++++--------
> >  hw/xen_pt_config_init.c |   42
> +++++++++++++++++++++++++++++-------------
> >
> > diff --git a/hw/xen_pt.c b/hw/xen_pt.c
> > index 307119a..2a8bcf3 100644
> > --- a/hw/xen_pt.c
> > +++ b/hw/xen_pt.c
> > @@ -403,21 +403,21 @@ static int
> xen_pt_register_regions(XenPCIPassthroughState *s)
> >
> >          s->bases[i].access.u = r->base_addr;
> >
> > -        if (r->type & XEN_HOST_PCI_REGION_TYPE_IO) {
> > +        if (r->type & XEN_HOST_PCI_REGION_TYPE_IO)
> >              type = PCI_BASE_ADDRESS_SPACE_IO;
> > -        } else {
> > +        else if (r->type & XEN_HOST_PCI_REGION_TYPE_MEM_64)
> > +            type = PCI_BASE_ADDRESS_MEM_TYPE_64;
> > +        else if (r->type & XEN_HOST_PCI_REGION_TYPE_PREFETCH)
> > +            type |= PCI_BASE_ADDRESS_MEM_PREFETCH;
> > +        else
> >              type = PCI_BASE_ADDRESS_SPACE_MEMORY;
> > -            if (r->type & XEN_HOST_PCI_REGION_TYPE_PREFETCH) {
> > -                type |= PCI_BASE_ADDRESS_MEM_PREFETCH;
> > -            }
> > -        }
> 
> Aside from the cody style issue here, this changes the behavior for type
> PCI_BASE_ADDRESS_SPACE_MEMORY. Before we could have:
> 
> type =
> PCI_BASE_ADDRESS_SPACE_MEMORY|PCI_BASE_ADDRESS_MEM_PREFETCH;
> 
> now we cannot anymore.
> 

Will change to:
-        if (r->type & XEN_HOST_PCI_REGION_TYPE_IO) {
+        if (r->type & XEN_HOST_PCI_REGION_TYPE_IO)
             type = PCI_BASE_ADDRESS_SPACE_IO;
-        } else {
+        else
             type = PCI_BASE_ADDRESS_SPACE_MEMORY;
-            if (r->type & XEN_HOST_PCI_REGION_TYPE_PREFETCH) {
+            if (r->type & XEN_HOST_PCI_REGION_TYPE_MEM_64)
+                type |= PCI_BASE_ADDRESS_MEM_TYPE_64;
+           else if (r->type & XEN_HOST_PCI_REGION_TYPE_PREFETCH)
                 type |= PCI_BASE_ADDRESS_MEM_PREFETCH;
-            }
-        }

> 
> >          memory_region_init_io(&s->bar[i], &ops, &s->dev,
> >                                "xen-pci-pt-bar", r->size);
> >          pci_register_bar(&s->dev, i, type, &s->bar[i]);
> >
> > -        XEN_PT_LOG(&s->dev, "IO region %i registered
> (size=0x%08"PRIx64
> > -                   " base_addr=0x%08"PRIx64" type: %#x)\n",
> > +        XEN_PT_LOG(&s->dev, "IO region %i registered
> (size=0x%lx"PRIx64
> > +                   " base_addr=0x%lx"PRIx64" type: %#x)\n",
> >                     i, r->size, r->base_addr, type);
> >      }
> >
> > diff --git a/hw/xen_pt_config_init.c b/hw/xen_pt_config_init.c
> > index e524a40..5e7ca22 100644
> > --- a/hw/xen_pt_config_init.c
> > +++ b/hw/xen_pt_config_init.c
> > @@ -342,6 +342,7 @@ static int
> xen_pt_cmd_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
> >  #define XEN_PT_BAR_IO_RO_MASK     0x00000003  /* BAR ReadOnly
> mask(I/O) */
> >  #define XEN_PT_BAR_IO_EMU_MASK    0xFFFFFFFC  /* BAR emul
> mask(I/O) */
> >
> > +static uint64_t xen_pt_get_bar_size(PCIIORegion *r);
> 
> there is just one user of xen_pt_get_bar_size, maybe you can just
> move the implementation here
> 

Ok, thanks.

> 
> >  static XenPTBarFlag xen_pt_bar_reg_parse(XenPCIPassthroughState *s,
> >                                           XenPTRegInfo *reg)
> >  {
> > @@ -366,7 +367,7 @@ static XenPTBarFlag
> xen_pt_bar_reg_parse(XenPCIPassthroughState *s,
> >
> >      /* check unused BAR */
> >      r = &d->io_regions[index];
> > -    if (r->size == 0) {
> > +    if (!xen_pt_get_bar_size(r)) {
> >          return XEN_PT_BAR_FLAG_UNUSED;
> >      }
> >
> > @@ -383,6 +384,24 @@ static XenPTBarFlag
> xen_pt_bar_reg_parse(XenPCIPassthroughState *s,
> >      }
> >  }
> >
> > +static bool is_64bit_bar(PCIIORegion *r)
> > +{
> > +    return !!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64);
> > +}
> > +
> > +static uint64_t xen_pt_get_bar_size(PCIIORegion *r)
> > +{
> > +    if (is_64bit_bar(r))
> > +    {
> > +        uint64_t size64;
> > +        size64 = (r + 1)->size;
> > +        size64 <<= 32;
> > +        size64 += r->size;
> > +        return size64;
> > +    }
> > +    return r->size;
> > +}
> > +
> >  static inline uint32_t base_address_with_flags(XenHostPCIIORegion *hr)
> >  {
> >      if (hr->type & XEN_HOST_PCI_REGION_TYPE_IO) {
> > @@ -481,7 +500,10 @@ static int
> xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
> >      switch (s->bases[index].bar_flag) {
> >      case XEN_PT_BAR_FLAG_MEM:
> >          bar_emu_mask = XEN_PT_BAR_MEM_EMU_MASK;
> > -        bar_ro_mask = XEN_PT_BAR_MEM_RO_MASK | (r_size - 1);
> > +        if (!r_size)
> > +            bar_ro_mask = XEN_PT_BAR_ALLF;
> > +        else
> > +            bar_ro_mask = XEN_PT_BAR_MEM_RO_MASK | (r_size - 1);
> >          break;
> 
> Is this an actual mistake everywhere?

It's low 32bit mask for 64 bit bars.

> There is at least another instance of
> 
> bar_ro_mask = something | (r_size - 1);
> 
> under the XEN_PT_BAR_FLAG_IO case.

Bar_ro_mask under XEN_PT_BAR_FLAG_IO already be done, I don't change IO case.

> Or is it only a problem with 64 bit bars? If so, could you please
> explain why?
> 
> 
> >      case XEN_PT_BAR_FLAG_IO:
> >          bar_emu_mask = XEN_PT_BAR_IO_EMU_MASK;
> > @@ -489,7 +511,10 @@ static int
> xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
> >          break;
> >      case XEN_PT_BAR_FLAG_UPPER:
> >          bar_emu_mask = XEN_PT_BAR_ALLF;
> > -        bar_ro_mask = 0;    /* all upper 32bit are R/W */
> > +        if (!r_size)
> > +            bar_ro_mask = 0;
> > +        else
> > +            bar_ro_mask = r_size - 1;
> >          break;
> 
> bar_ro_mask = r_size ? r_size - 1 : 0;
> 

Great. :)
Stefano Stabellini Sept. 26, 2012, 10:57 a.m. UTC | #3 
On Wed, 26 Sep 2012, Hao, Xudong wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: Tuesday, September 25, 2012 6:52 PM
> > To: Hao, Xudong
> > Cc: Stefano Stabellini; xen-devel@lists.xen.org; qemu-devel@nongnu.org;
> > Zhang, Xiantao
> > Subject: Re: [PATCH v2] qemu/xen: Add 64 bits big bar support on qemu
> > 
> > On Tue, 25 Sep 2012, Xudong Hao wrote:
> > > Changes from v1:
> > > - Rebase to qemu upstream from qemu-xen
> > 
> > Thanks. Please run scripts/checkpatch.pl on this patch, you'll find
> > some cody style issues that need to be fixed.
> > 
> OK, will use this scripts to check code style.
> 
> > 
> > > Currently it is assumed PCI device BAR access < 4G memory. If there is such a
> > > device whose BAR size is larger than 4G, it must access > 4G memory
> > address.
> > > This patch enable the 64bits big BAR support on qemu.
> > >
> > > Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> > > Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
> > > ---
> > >  hw/xen_pt.c             |   16 ++++++++--------
> > >  hw/xen_pt_config_init.c |   42
> > +++++++++++++++++++++++++++++-------------
> > >
> > > diff --git a/hw/xen_pt.c b/hw/xen_pt.c
> > > index 307119a..2a8bcf3 100644
> > > --- a/hw/xen_pt.c
> > > +++ b/hw/xen_pt.c
> > > @@ -403,21 +403,21 @@ static int
> > xen_pt_register_regions(XenPCIPassthroughState *s)
> > >
> > >          s->bases[i].access.u = r->base_addr;
> > >
> > > -        if (r->type & XEN_HOST_PCI_REGION_TYPE_IO) {
> > > +        if (r->type & XEN_HOST_PCI_REGION_TYPE_IO)
> > >              type = PCI_BASE_ADDRESS_SPACE_IO;
> > > -        } else {
> > > +        else if (r->type & XEN_HOST_PCI_REGION_TYPE_MEM_64)
> > > +            type = PCI_BASE_ADDRESS_MEM_TYPE_64;
> > > +        else if (r->type & XEN_HOST_PCI_REGION_TYPE_PREFETCH)
> > > +            type |= PCI_BASE_ADDRESS_MEM_PREFETCH;
> > > +        else
> > >              type = PCI_BASE_ADDRESS_SPACE_MEMORY;
> > > -            if (r->type & XEN_HOST_PCI_REGION_TYPE_PREFETCH) {
> > > -                type |= PCI_BASE_ADDRESS_MEM_PREFETCH;
> > > -            }
> > > -        }
> > 
> > Aside from the cody style issue here, this changes the behavior for type
> > PCI_BASE_ADDRESS_SPACE_MEMORY. Before we could have:
> > 
> > type =
> > PCI_BASE_ADDRESS_SPACE_MEMORY|PCI_BASE_ADDRESS_MEM_PREFETCH;
> > 
> > now we cannot anymore.
> > 
> 
> Will change to:
> -        if (r->type & XEN_HOST_PCI_REGION_TYPE_IO) {
> +        if (r->type & XEN_HOST_PCI_REGION_TYPE_IO)
>              type = PCI_BASE_ADDRESS_SPACE_IO;
> -        } else {
> +        else
>              type = PCI_BASE_ADDRESS_SPACE_MEMORY;
> -            if (r->type & XEN_HOST_PCI_REGION_TYPE_PREFETCH) {
> +            if (r->type & XEN_HOST_PCI_REGION_TYPE_MEM_64)
> +                type |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> +           else if (r->type & XEN_HOST_PCI_REGION_TYPE_PREFETCH)
>                  type |= PCI_BASE_ADDRESS_MEM_PREFETCH;
> -            }
> -        }

Isn't it possible that both XEN_HOST_PCI_REGION_TYPE_MEM_64 and
XEN_HOST_PCI_REGION_TYPE_PREFETCH are set? It doesn't look like this can
cover that case.
The following seems to be what you are looking for:


if (r->type & XEN_HOST_PCI_REGION_TYPE_IO) {
    type = PCI_BASE_ADDRESS_SPACE_IO;
} else {
    type = PCI_BASE_ADDRESS_SPACE_MEMORY;
    if (r->type & XEN_HOST_PCI_REGION_TYPE_PREFETCH) {
        type |= PCI_BASE_ADDRESS_MEM_PREFETCH;
    }
    if (r->type & XEN_HOST_PCI_REGION_TYPE_MEM_64) {
        type |= PCI_BASE_ADDRESS_MEM_TYPE_64;
    }
}


> > >  static XenPTBarFlag xen_pt_bar_reg_parse(XenPCIPassthroughState *s,
> > >                                           XenPTRegInfo *reg)
> > >  {
> > > @@ -366,7 +367,7 @@ static XenPTBarFlag
> > xen_pt_bar_reg_parse(XenPCIPassthroughState *s,
> > >
> > >      /* check unused BAR */
> > >      r = &d->io_regions[index];
> > > -    if (r->size == 0) {
> > > +    if (!xen_pt_get_bar_size(r)) {
> > >          return XEN_PT_BAR_FLAG_UNUSED;
> > >      }
> > >
> > > @@ -383,6 +384,24 @@ static XenPTBarFlag
> > xen_pt_bar_reg_parse(XenPCIPassthroughState *s,
> > >      }
> > >  }
> > >
> > > +static bool is_64bit_bar(PCIIORegion *r)
> > > +{
> > > +    return !!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64);
> > > +}
> > > +
> > > +static uint64_t xen_pt_get_bar_size(PCIIORegion *r)
> > > +{
> > > +    if (is_64bit_bar(r))
> > > +    {
> > > +        uint64_t size64;
> > > +        size64 = (r + 1)->size;
> > > +        size64 <<= 32;
> > > +        size64 += r->size;
> > > +        return size64;
> > > +    }
> > > +    return r->size;
> > > +}
> > > +
> > >  static inline uint32_t base_address_with_flags(XenHostPCIIORegion *hr)
> > >  {
> > >      if (hr->type & XEN_HOST_PCI_REGION_TYPE_IO) {
> > > @@ -481,7 +500,10 @@ static int
> > xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
> > >      switch (s->bases[index].bar_flag) {
> > >      case XEN_PT_BAR_FLAG_MEM:
> > >          bar_emu_mask = XEN_PT_BAR_MEM_EMU_MASK;
> > > -        bar_ro_mask = XEN_PT_BAR_MEM_RO_MASK | (r_size - 1);
> > > +        if (!r_size)
> > > +            bar_ro_mask = XEN_PT_BAR_ALLF;
> > > +        else
> > > +            bar_ro_mask = XEN_PT_BAR_MEM_RO_MASK | (r_size - 1);
> > >          break;
> > 
> > Is this an actual mistake everywhere?
> 
> It's low 32bit mask for 64 bit bars.

I see. It is a good idea to add a line of comment in the code saying
that.
Hao, Xudong Sept. 27, 2012, 1:21 a.m. UTC | #4 
> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Wednesday, September 26, 2012 6:57 PM
> To: Hao, Xudong
> Cc: Stefano Stabellini; xen-devel@lists.xen.org; qemu-devel@nongnu.org;
> Zhang, Xiantao
> Subject: RE: [PATCH v2] qemu/xen: Add 64 bits big bar support on qemu
> 
> > Will change to:
> > -        if (r->type & XEN_HOST_PCI_REGION_TYPE_IO) {
> > +        if (r->type & XEN_HOST_PCI_REGION_TYPE_IO)
> >              type = PCI_BASE_ADDRESS_SPACE_IO;
> > -        } else {
> > +        else
> >              type = PCI_BASE_ADDRESS_SPACE_MEMORY;
> > -            if (r->type & XEN_HOST_PCI_REGION_TYPE_PREFETCH) {
> > +            if (r->type & XEN_HOST_PCI_REGION_TYPE_MEM_64)
> > +                type |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> > +           else if (r->type & XEN_HOST_PCI_REGION_TYPE_PREFETCH)
> >                  type |= PCI_BASE_ADDRESS_MEM_PREFETCH;
> > -            }
> > -        }
> 
> Isn't it possible that both XEN_HOST_PCI_REGION_TYPE_MEM_64 and
> XEN_HOST_PCI_REGION_TYPE_PREFETCH are set? It doesn't look like this can
> cover that case.
It's possible. Thanks comments.

> The following seems to be what you are looking for:
> 
> 
> if (r->type & XEN_HOST_PCI_REGION_TYPE_IO) {
>     type = PCI_BASE_ADDRESS_SPACE_IO;
> } else {
>     type = PCI_BASE_ADDRESS_SPACE_MEMORY;
>     if (r->type & XEN_HOST_PCI_REGION_TYPE_PREFETCH) {
>         type |= PCI_BASE_ADDRESS_MEM_PREFETCH;
>     }
>     if (r->type & XEN_HOST_PCI_REGION_TYPE_MEM_64) {
>         type |= PCI_BASE_ADDRESS_MEM_TYPE_64;
>     }
> }
> 
> 
> > > >  static XenPTBarFlag xen_pt_bar_reg_parse(XenPCIPassthroughState
> *s,
> > > >                                           XenPTRegInfo *reg)
> > > >  {
> > > > @@ -366,7 +367,7 @@ static XenPTBarFlag
> > > xen_pt_bar_reg_parse(XenPCIPassthroughState *s,
> > > >
> > > >      /* check unused BAR */
> > > >      r = &d->io_regions[index];
> > > > -    if (r->size == 0) {
> > > > +    if (!xen_pt_get_bar_size(r)) {
> > > >          return XEN_PT_BAR_FLAG_UNUSED;
> > > >      }
> > > >
> > > > @@ -383,6 +384,24 @@ static XenPTBarFlag
> > > xen_pt_bar_reg_parse(XenPCIPassthroughState *s,
> > > >      }
> > > >  }
> > > >
> > > > +static bool is_64bit_bar(PCIIORegion *r)
> > > > +{
> > > > +    return !!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64);
> > > > +}
> > > > +
> > > > +static uint64_t xen_pt_get_bar_size(PCIIORegion *r)
> > > > +{
> > > > +    if (is_64bit_bar(r))
> > > > +    {
> > > > +        uint64_t size64;
> > > > +        size64 = (r + 1)->size;
> > > > +        size64 <<= 32;
> > > > +        size64 += r->size;
> > > > +        return size64;
> > > > +    }
> > > > +    return r->size;
> > > > +}
> > > > +
> > > >  static inline uint32_t base_address_with_flags(XenHostPCIIORegion
> *hr)
> > > >  {
> > > >      if (hr->type & XEN_HOST_PCI_REGION_TYPE_IO) {
> > > > @@ -481,7 +500,10 @@ static int
> > > xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
> > > >      switch (s->bases[index].bar_flag) {
> > > >      case XEN_PT_BAR_FLAG_MEM:
> > > >          bar_emu_mask = XEN_PT_BAR_MEM_EMU_MASK;
> > > > -        bar_ro_mask = XEN_PT_BAR_MEM_RO_MASK | (r_size - 1);
> > > > +        if (!r_size)
> > > > +            bar_ro_mask = XEN_PT_BAR_ALLF;
> > > > +        else
> > > > +            bar_ro_mask = XEN_PT_BAR_MEM_RO_MASK | (r_size -
> 1);
> > > >          break;
> > >
> > > Is this an actual mistake everywhere?
> >
> > It's low 32bit mask for 64 bit bars.
> 
> I see. It is a good idea to add a line of comment in the code saying
> that.

OK, I'll add code comment.
diff mbox

Patch

diff --git a/hw/xen_pt.c b/hw/xen_pt.c
index 307119a..2a8bcf3 100644
--- a/hw/xen_pt.c
+++ b/hw/xen_pt.c
@@ -403,21 +403,21 @@  static int xen_pt_register_regions(XenPCIPassthroughState *s)
 
         s->bases[i].access.u = r->base_addr;
 
-        if (r->type & XEN_HOST_PCI_REGION_TYPE_IO) {
+        if (r->type & XEN_HOST_PCI_REGION_TYPE_IO)
             type = PCI_BASE_ADDRESS_SPACE_IO;
-        } else {
+        else if (r->type & XEN_HOST_PCI_REGION_TYPE_MEM_64)
+            type = PCI_BASE_ADDRESS_MEM_TYPE_64;
+        else if (r->type & XEN_HOST_PCI_REGION_TYPE_PREFETCH)
+            type |= PCI_BASE_ADDRESS_MEM_PREFETCH;
+        else
             type = PCI_BASE_ADDRESS_SPACE_MEMORY;
-            if (r->type & XEN_HOST_PCI_REGION_TYPE_PREFETCH) {
-                type |= PCI_BASE_ADDRESS_MEM_PREFETCH;
-            }
-        }
 
         memory_region_init_io(&s->bar[i], &ops, &s->dev,
                               "xen-pci-pt-bar", r->size);
         pci_register_bar(&s->dev, i, type, &s->bar[i]);
 
-        XEN_PT_LOG(&s->dev, "IO region %i registered (size=0x%08"PRIx64
-                   " base_addr=0x%08"PRIx64" type: %#x)\n",
+        XEN_PT_LOG(&s->dev, "IO region %i registered (size=0x%lx"PRIx64
+                   " base_addr=0x%lx"PRIx64" type: %#x)\n",
                    i, r->size, r->base_addr, type);
     }
 
diff --git a/hw/xen_pt_config_init.c b/hw/xen_pt_config_init.c
index e524a40..5e7ca22 100644
--- a/hw/xen_pt_config_init.c
+++ b/hw/xen_pt_config_init.c
@@ -342,6 +342,7 @@  static int xen_pt_cmd_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
 #define XEN_PT_BAR_IO_RO_MASK     0x00000003  /* BAR ReadOnly mask(I/O) */
 #define XEN_PT_BAR_IO_EMU_MASK    0xFFFFFFFC  /* BAR emul mask(I/O) */
 
+static uint64_t xen_pt_get_bar_size(PCIIORegion *r);
 static XenPTBarFlag xen_pt_bar_reg_parse(XenPCIPassthroughState *s,
                                          XenPTRegInfo *reg)
 {
@@ -366,7 +367,7 @@  static XenPTBarFlag xen_pt_bar_reg_parse(XenPCIPassthroughState *s,
 
     /* check unused BAR */
     r = &d->io_regions[index];
-    if (r->size == 0) {
+    if (!xen_pt_get_bar_size(r)) {
         return XEN_PT_BAR_FLAG_UNUSED;
     }
 
@@ -383,6 +384,24 @@  static XenPTBarFlag xen_pt_bar_reg_parse(XenPCIPassthroughState *s,
     }
 }
 
+static bool is_64bit_bar(PCIIORegion *r)
+{
+    return !!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64);
+}
+
+static uint64_t xen_pt_get_bar_size(PCIIORegion *r)
+{
+    if (is_64bit_bar(r))
+    {
+        uint64_t size64;
+        size64 = (r + 1)->size; 
+        size64 <<= 32; 
+        size64 += r->size;
+        return size64; 
+    }
+    return r->size; 
+}
+
 static inline uint32_t base_address_with_flags(XenHostPCIIORegion *hr)
 {
     if (hr->type & XEN_HOST_PCI_REGION_TYPE_IO) {
@@ -481,7 +500,10 @@  static int xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
     switch (s->bases[index].bar_flag) {
     case XEN_PT_BAR_FLAG_MEM:
         bar_emu_mask = XEN_PT_BAR_MEM_EMU_MASK;
-        bar_ro_mask = XEN_PT_BAR_MEM_RO_MASK | (r_size - 1);
+        if (!r_size)
+            bar_ro_mask = XEN_PT_BAR_ALLF;
+        else
+            bar_ro_mask = XEN_PT_BAR_MEM_RO_MASK | (r_size - 1);
         break;
     case XEN_PT_BAR_FLAG_IO:
         bar_emu_mask = XEN_PT_BAR_IO_EMU_MASK;
@@ -489,7 +511,10 @@  static int xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
         break;
     case XEN_PT_BAR_FLAG_UPPER:
         bar_emu_mask = XEN_PT_BAR_ALLF;
-        bar_ro_mask = 0;    /* all upper 32bit are R/W */
+        if (!r_size)
+            bar_ro_mask = 0;
+        else
+            bar_ro_mask = r_size - 1;
         break;
     default:
         break;
@@ -501,22 +526,13 @@  static int xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
 
     /* check whether we need to update the virtual region address or not */
     switch (s->bases[index].bar_flag) {
+    case XEN_PT_BAR_FLAG_UPPER:
     case XEN_PT_BAR_FLAG_MEM:
         /* nothing to do */
         break;
     case XEN_PT_BAR_FLAG_IO:
         /* nothing to do */
         break;
-    case XEN_PT_BAR_FLAG_UPPER:
-        if (cfg_entry->data) {
-            if (cfg_entry->data != (XEN_PT_BAR_ALLF & ~bar_ro_mask)) {
-                XEN_PT_WARN(d, "Guest attempt to set high MMIO Base Address. "
-                            "Ignore mapping. "
-                            "(offset: 0x%02x, high address: 0x%08x)\n",
-                            reg->offset, cfg_entry->data);
-            }
-        }
-        break;
     default:
         break;
     }