On Tue, Jun 07, 2016 at 10:00:42PM +0200, Alexander Gordeev wrote:
> On Tue, Jun 07, 2016 at 04:08:22PM +0200, Andrew Jones wrote:
> > On Tue, Jun 07, 2016 at 12:33:54PM +0200, Alexander Gordeev wrote:
> > > On Tue, Jun 07, 2016 at 09:03:18AM +0200, Andrew Jones wrote:
> > > > On Tue, Jun 07, 2016 at 08:38:55AM +0200, Alexander Gordeev wrote:
> > > > > On Mon, Jun 06, 2016 at 04:12:03PM +0200, Andrew Jones wrote:
> > > > > > > +phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
> > > > > > > {
> > > > > > > uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> > > > > > > + phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
> > > > > > > + phys_addr_t mask = (int32_t)pci_bar_mask(bar);
> > > > > > >
> > > > > > > - if (bar & PCI_BASE_ADDRESS_SPACE_IO)
> > > > > > > - return bar & PCI_BASE_ADDRESS_IO_MASK;
> > > > > > > - else
> > > > > > > - return bar & PCI_BASE_ADDRESS_MEM_MASK;
> > > > > > > + if (pci_bar_is64(dev, bar_num)) {
> > > > > > > + uint32_t size_high = pci_bar_size32(dev, bar_num + 1);
> > > > > > > + size = ((phys_addr_t)size_high << 32) | (uint32_t)size;
> > > > > > > + }
> > > > > > > +
> > > > > > > + return (~(size & mask)) + 1;
> > > > > >
> > > > > > All this casting of size and mask is pointless. Please rework it
> > > > > > similar to what I did above.
> > > > >
> > > > > This is the most compact and straight variant I was able to come up with:
> > > > >
> > > > > uint32_t bar = pci_bar_get(dev, bar_num);
> > > > > phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
> > > >
> > > > But this is wrong. If your 32-bit size was 0x80000000, then you now
> > > > say it's 0xffffffff80000000.
> > >
> > > Hmm.. I am either terribly missing the point or we are on different
> > > pages.
> >
> > Let's make sure I know what the function is supposed to do.
> >
> > Based on your comment explaining how to get the PCI bar size,
> > it sounds like we should do these steps; let's assume a device
> > needs size=0x200000, and is 32-bit
>
> I am not quite sure what barsz=0x00000000 at read-write1s-dance
barsz is initialized to zero in the sequence in order to start the
variable descriptions with something, but it's not necessary to do
so with real code.
> phase is, but otherwise it seems correct to me (well may be a nit -
> the 32-bit variant denoted 64-bit mmio).
Not sure what you mean, but it doesn't matter for this :-)
>
> > read-write1s-dance barsz=0x00000000
> > readl barsz=0xffe0000c
> > mask barsz=0xffe00000
> >
> > barsz = ~barsz + 1 barsz=0x00200000
> >
> > Now let's say it's 64-bit
> >
> > read-write1s-dance barsz_lo=0x00000000
> > readl barsz_lo=0xffe0000c
> > mask barsz_lo=0xffe00000
> > read-write1s-dance barsz_hi=0x00000000
> > readl barsz_hi=0xffffffff
> > <upper 32-bit mask is 0xffffffff, so don't bother masking at all>
> >
> > barsz = ~((phys_addr_t)barsz_hi << 32 | barsz_lo) + 1 (barsz=0x200000)
> >
> > So you need 5 functions
> >
> > uint32_t get_mask(int bar)
> > {
> > ...
> > }
> >
> > uint32_t read_dance(int bar)
> > {
> > read-write1s-dance
> > return readl
> > }
> >
> > uint32_t get_size32(int bar)
> > {
> > uint32_t size = read_dance(bar) & get_mask(bar);
> > return ~size + 1;
> > }
> >
> > uint64_t get_size64(int bar)
> > {
> > uint64_t size = read_dance(bar) & get_mask(bar);
> > uint64_t size_hi = read_dance(bar + 1);
> >
> > size |= size_hi << 32;
> > return ~size + 1;
> > }
> >
> > uint64_t get_size(int bar)
> > {
> > return bar_size(bar) == BAR64 ? get_size64(bar) : get_size32(bar);
> > }
>
> If the above is a pseudo code or you want me to rework using these
> functions?
Yes and no. You can do what you like, but pci_bar_size32 should
return a size (like my get_size32), not a 2's complement of size,
otherwise the name is wrong.
> If not, below is how it could look like.
>
> Also, looking at your get_size() I noticed a bug in my version :-)
> when read_dance() is followed by readl() returns zero - that means no
> (more) BAR(s).
>
> phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
> {
> uint32_t size;
> uint32_t bar;
>
> size = pci_bar_size32(dev, bar_num);
> if (!size)
> return 0;
Should callers ever call pci_bar_size on a bar_num that will result
in zero, "no more BARs"? If not, then you can assert here. Otherwise
will all callers know what zero means, and check for it?
>
> bar = pci_bar_get(dev, bar_num);
> size &= pci_bar_mask(bar);
>
> if (pci_bar_is64(dev, bar_num)) {
> phys_addr_t size64 = pci_bar_size32(dev, bar_num + 1);
> size64 = (size64 << 32) | size;
>
> return ~size64 + 1;
> } else {
> return ~size + 1;
> }
> }
>
Otherwise I think it's right.
Thanks,
drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
