On Tue, Jun 07, 2016 at 12:33:54PM +0200, Alexander Gordeev wrote: > > > phys_addr_t mask = (int32_t)pci_bar_mask(bar); > > > > It might be OK to do that here, on a mask, but even if it is, then I > > don't like it, because it's too subtle (like I said for the casting > > in pci_bar_addr in my last reply). I don't like that it requires us to > > know that masking bit 31 in a 32-bit mask means we also want to mask > > 63..32. That should at least be in a comment somewhere. > > Yes, but it is not an arbitrary mask, it is an alignment mask. > We unconditionally want to mask 63..32, 31th and even lower. Seems like making pci_bar_mask() return phys_addr_t rather than uint32_t would make more sense. > > > if (pci_bar_is64(dev, bar_num)) > > > size |= (phys_addr_t)pci_bar_size32(dev, bar_num + 1) << 32; > > > > > > return (~(size & mask)) + 1; > > > > > > The casting is needed to avoid putting explicitly all 1s into higher > > > bits of size and/or mask. Otherwise (~(size & mask)) + 1 expression would > > > not bring correct results. I really struggle to make something better > > > readable. > > > > It's not just about readability, it's about correctness. You shouldn't > > use 32-bit sizes/masks with 64-bit data this way. Like I said in the > > last reply, you should rework it like I showed you, operate on the > > 32-bit data with the 32-bit mask/size, and then eventually construct > > 64-bit data. > > It is all about those last +1 and shifting bits from lower to upper > word of 64-bit value. My hope was to off-load it to the compiler :) > > > 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
