[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: |
Hao, Xudong |
Subject: |
Re: [Qemu-devel] [PATCH v2] qemu/xen: Add 64 bits big bar support on qemu |
Date: |
Wed, 26 Sep 2012 08:24:32 +0000 |
> -----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;
- }
- }
>
> > 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. :)