On 10/18/2018 05:44 PM, Andrea Bolognani wrote: > On Thu, 2018-10-18 at 15:13 +0800, Yi Min Zhao wrote: >> 在 2018/10/17 下午10:30, Andrea Bolognani 写道: >>> On Tue, 2018-10-16 at 11:28 +0800, Yi Min Zhao wrote: >>>> I think this would make things complex. If either PCI address or >>>> zPCI address exists, we have to do more checks for calling >>>> virDomainPCIAddressReserveAddr(). And there are amounts of >>>> code calling ***IsWanted() to call ***ReserveNext***(). I think >>>> keeping them separately is better. >>> >>> Again, I might be missing something because I haven't actually tried >>> implementing any of this, but at least from the theoretical point of >>> view I don't see how keeping them separate would make things simpler: >>> if anything, it seems to me like it would make them more complicated >>> for the calling code because now you have to worry about the PCI >>> address extensions *in addition* to the PCI address itself. >> >> For example, during collection stage, checking both PCI address and >> extension address >> is requried, and still need to separately do some additional checks for >> PCI address if it >> is present, at last in reserving addr function we still check if PCI >> normal address or >> extension address exists to separately reserve present one. So that we >> have to do the >> check on the same condition repetively. If you don't have strong >> opposition, I want to >> send the new version ASAP. > > So I gave an half-hearted stab at implementing my own suggestion > today, and quite unsurprisingly I have gained more sympathy for your > argument in the process :) > > The main problem I see is that, as you noticed, we have a lot of > calls to IsWanted(), IsPresent(), ReserveAddr() and ReserveNextAddr() > where really we should be using EnsureAddr() pretty much all of the > time and hide most of the details from the drivers, which in turn > would make it easier to change them in a transparent manner. > > Another big problem, which I highlighted in the past, is that the > current API was not designed with the idea that PCI addresses could > have "parts" in mind, and so it's not nuanced enough: if I call > IsEmpty() on and address where the PCI part itself has been filled > in but the zPCI part hasn't, or vice versa, what should I get back? > The answer is probably that, after we've made sure those functions > are used as little as possible thanks to the changes outlined above, > we should replace them with better named alternatives. > > Of course it would be entirely unfair to ask you to perform such > a massive refactoring before your series can be considered for > inclusion, so at this point I'm okay with merging it and cleaning > up the pre-existing mess afterwards. > > There's still the question of whether Dan is now okay with the XML > structure as currently implemented; assuming that's the case, it > would be great if Laine could also take a quick look at the series > before it's pushed. As I said before, I think the current XML is the right variant. This is exactly how QEMU implements it (have a real classic pci bus naming scheme augmented with some additional data named uid/fid). So having an zcpi name space (instead of a pci one) would be wrong. Daniel, having said this, are you ok with the current variant? -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
