[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: |
Thu, 27 Sep 2012 01:21:38 +0000 |
> -----Original Message-----
> From: Stefano Stabellini [mailto:address@hidden
> Sent: Wednesday, September 26, 2012 6:57 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
>
> > 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.