[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] qemu/xen: Add 64 bits big bar support on qem
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] [PATCH v2] qemu/xen: Add 64 bits big bar support on qemu |
Date: |
Wed, 26 Sep 2012 11:57:10 +0100 |
User-agent: |
Alpine 2.02 (DEB 1266 2009-07-14) |
On Wed, 26 Sep 2012, Hao, Xudong wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:address@hidden
> > Sent: Tuesday, September 25, 2012 6:52 PM
> > To: Hao, Xudong
> > Cc: Stefano Stabellini; address@hidden; address@hidden;
> > 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 <address@hidden>
> > > Signed-off-by: Xiantao Zhang <address@hidden>
> > > ---
> > > 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.