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 +++++++++++++++++++++++++++++-------------
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;
}
--
1.5.5
Stefano Stabellini
2012-Sep-25 10:52 UTC
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-xenThanks. 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
2012-Sep-26 08:24 UTC
Re: [PATCH v2] qemu/xen: Add 64 bits big bar support on qemu
> -----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
2012-Sep-26 10:57 UTC
Re: [PATCH v2] qemu/xen: Add 64 bits big bar support on qemu
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
2012-Sep-27 01:21 UTC
Re: [PATCH v2] qemu/xen: Add 64 bits big bar support on qemu
> -----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.