Re: [Xen-devel] [v7][PATCH 00/16] Fix RMRR
On 16.07.15 at 11:27, george.dun...@eu.citrix.com wrote: On Thu, Jul 16, 2015 at 9:26 AM, Jan Beulich jbeul...@suse.com wrote: On 16.07.15 at 10:13, tiejun.c...@intel.com wrote: It looks like most of the libxl/libxc patches have been acked. It seems to me that most of the hypervisor patches (1-3, 14-15) are either ready to go in or pretty close. Now that I looked over v8 I have to admit that if I was a tools maintainer I wouldn't want to see some of the tools patches in with just an ack, but without any review. I'm somewhat confused at this point. Acked-by: is often used by the maintainer of the affected code when that maintainer neither contributed to nor forwarded the patch. It is a record that the acker has at least reviewed the patch and has indicated acceptance. Does this imply this is already reviewed? No, that would be expressed by Reviewed-by. Acked-by merely means no objection by the maintainer for the change to go in. Sorry I'm trying to dig into this. If nobody would like to take a look at this, so isn't this the associated maintainer's responsibility to review finally? In this case isn't Acked-by fine enough? Acked-by is good enough for a patch to go in, yes. Note that I didn't make this a requirement (as I'm not the maintainer), I just said that if I was the maintainer, I would for at least some of the tools patches. There does seem to be a disconnect between how Reviewed-by and Acked-by are used on the tools side vs the hypervisor side. (We just stumbled across this in an internal discussion about commit stats actually.) But in any case, it's the maintainers' responsibility to determine if something has had sufficient review, and it's their responsibility not to give an Ack unless they really mean As far as I'm concerned, this is ready to go in. The fact that there were Acks on the toolstack side ought to mean that this judgement had already been made. Hmm, that's a different view than I take: To me Reviewed-by implies Acked-by, but not the other way around. And I view it as the committer's responsibility to ensure a patch has all necessary acks, but not the maintainer to give an ack only when reviews were done. In the end I'm afraid the current model of tags may not be suitable to express everything we need, or the lack of a formal description somewhere leaves too much room for mismatching interpretations. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 00/16] Fix RMRR
On Thu, Jul 16, 2015 at 10:44 AM, Jan Beulich jbeul...@suse.com wrote: On 16.07.15 at 11:27, george.dun...@eu.citrix.com wrote: On Thu, Jul 16, 2015 at 9:26 AM, Jan Beulich jbeul...@suse.com wrote: On 16.07.15 at 10:13, tiejun.c...@intel.com wrote: It looks like most of the libxl/libxc patches have been acked. It seems to me that most of the hypervisor patches (1-3, 14-15) are either ready to go in or pretty close. Now that I looked over v8 I have to admit that if I was a tools maintainer I wouldn't want to see some of the tools patches in with just an ack, but without any review. I'm somewhat confused at this point. Acked-by: is often used by the maintainer of the affected code when that maintainer neither contributed to nor forwarded the patch. It is a record that the acker has at least reviewed the patch and has indicated acceptance. Does this imply this is already reviewed? No, that would be expressed by Reviewed-by. Acked-by merely means no objection by the maintainer for the change to go in. Sorry I'm trying to dig into this. If nobody would like to take a look at this, so isn't this the associated maintainer's responsibility to review finally? In this case isn't Acked-by fine enough? Acked-by is good enough for a patch to go in, yes. Note that I didn't make this a requirement (as I'm not the maintainer), I just said that if I was the maintainer, I would for at least some of the tools patches. There does seem to be a disconnect between how Reviewed-by and Acked-by are used on the tools side vs the hypervisor side. (We just stumbled across this in an internal discussion about commit stats actually.) But in any case, it's the maintainers' responsibility to determine if something has had sufficient review, and it's their responsibility not to give an Ack unless they really mean As far as I'm concerned, this is ready to go in. The fact that there were Acks on the toolstack side ought to mean that this judgement had already been made. Hmm, that's a different view than I take: To me Reviewed-by implies Acked-by, but not the other way around. And I view it as the committer's responsibility to ensure a patch has all necessary acks, but not the maintainer to give an ack only when reviews were done. I'm not debating the meaning of Reviewed-by and Acked-by in general; I'm responding to your questioning earlier whether the tools side was really ready to go in. What I'm saying is, when the maintainer gives an Acked-by, they should have already either 1) reviewed the code themselves and determined it's ready to go in (although perhaps in this case a Reviewed-by would be preferred), or 2) have looked through the patch and determined based on their trust of the other reviewers, on their own previous review of the code, or their confidence in the author of the patch, that the patch is ready to go in. So your question shouldn't be, Should we check this in given that it's only been Acked? It should be, Why did you Ack it when there has apparently been no review? And the answer turns out to be, Because we tend to use Acked-by even when we have reviewed it. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 00/16] Fix RMRR
On 16 Jul 2015, at 09:30, Ian Campbell ian.campb...@citrix.com wrote: On Thu, 2015-07-16 at 09:08 +0100, Jan Beulich wrote: Does this imply this is already reviewed? No, that would be expressed by Reviewed-by. Acked-by merely means no objection by the maintainer for the change to go in. For my part I, perhaps wrongly, use Acked-by for both. If I haven't actually carefully reviewed the change I will usually say so, e.g. I see XXX has reviewed this already, so that's fine by me or something similar (which I admit gets lost once it becomes just the tags). I can't speak for Ian or Wei (now CCd) but Ian at least I think operates similarly. Ian. We shouldn't have an argument about this now. I wrote some scripts yesterday to track Acked-by and Reviewed-by and it is clear that these two tags are not consistently used at the moment. Maybe something to pick up in 2 weeks Regards Lars ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 00/16] Fix RMRR
On 16.07.15 at 10:03, tiejun.c...@intel.com wrote: On 2015/7/16 15:55, Jan Beulich wrote: On 10.07.15 at 16:50, george.dun...@eu.citrix.com wrote: On Thu, Jul 9, 2015 at 6:33 AM, Tiejun Chen tiejun.c...@intel.com wrote: v7: It looks like most of the libxl/libxc patches have been acked. It seems to me that most of the hypervisor patches (1-3, 14-15) are either ready to go in or pretty close. Now that I looked over v8 I have to admit that if I was a tools maintainer I wouldn't want to see some of the tools patches in with just an ack, but without any review. I'm somewhat confused at this point. Acked-by: is often used by the maintainer of the affected code when that maintainer neither contributed to nor forwarded the patch. It is a record that the acker has at least reviewed the patch and has indicated acceptance. Does this imply this is already reviewed? No, that would be expressed by Reviewed-by. Acked-by merely means no objection by the maintainer for the change to go in. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 00/16] Fix RMRR
On 2015/7/16 15:55, Jan Beulich wrote: On 10.07.15 at 16:50, george.dun...@eu.citrix.com wrote: On Thu, Jul 9, 2015 at 6:33 AM, Tiejun Chen tiejun.c...@intel.com wrote: v7: It looks like most of the libxl/libxc patches have been acked. It seems to me that most of the hypervisor patches (1-3, 14-15) are either ready to go in or pretty close. Now that I looked over v8 I have to admit that if I was a tools maintainer I wouldn't want to see some of the tools patches in with just an ack, but without any review. I'm somewhat confused at this point. Acked-by: is often used by the maintainer of the affected code when that maintainer neither contributed to nor forwarded the patch. It is a record that the acker has at least reviewed the patch and has indicated acceptance. Does this imply this is already reviewed? Thanks Tiejun ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 00/16] Fix RMRR
On Thu, 2015-07-16 at 09:08 +0100, Jan Beulich wrote: On 16.07.15 at 10:03, tiejun.c...@intel.com wrote: On 2015/7/16 15:55, Jan Beulich wrote: On 10.07.15 at 16:50, george.dun...@eu.citrix.com wrote: On Thu, Jul 9, 2015 at 6:33 AM, Tiejun Chen tiejun.c...@intel.com wrote: v7: It looks like most of the libxl/libxc patches have been acked. It seems to me that most of the hypervisor patches (1-3, 14-15) are either ready to go in or pretty close. Now that I looked over v8 I have to admit that if I was a tools maintainer I wouldn't want to see some of the tools patches in with just an ack, but without any review. I'm somewhat confused at this point. Acked-by: is often used by the maintainer of the affected code when that maintainer neither contributed to nor forwarded the patch. It is a record that the acker has at least reviewed the patch and has indicated acceptance. Does this imply this is already reviewed? No, that would be expressed by Reviewed-by. Acked-by merely means no objection by the maintainer for the change to go in. For my part I, perhaps wrongly, use Acked-by for both. If I haven't actually carefully reviewed the change I will usually say so, e.g. I see XXX has reviewed this already, so that's fine by me or something similar (which I admit gets lost once it becomes just the tags). I can't speak for Ian or Wei (now CCd) but Ian at least I think operates similarly. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 00/16] Fix RMRR
It looks like most of the libxl/libxc patches have been acked. It seems to me that most of the hypervisor patches (1-3, 14-15) are either ready to go in or pretty close. Now that I looked over v8 I have to admit that if I was a tools maintainer I wouldn't want to see some of the tools patches in with just an ack, but without any review. I'm somewhat confused at this point. Acked-by: is often used by the maintainer of the affected code when that maintainer neither contributed to nor forwarded the patch. It is a record that the acker has at least reviewed the patch and has indicated acceptance. Does this imply this is already reviewed? No, that would be expressed by Reviewed-by. Acked-by merely means no objection by the maintainer for the change to go in. Sorry I'm trying to dig into this. If nobody would like to take a look at this, so isn't this the associated maintainer's responsibility to review finally? In this case isn't Acked-by fine enough? Or you still want to us add two lines explicitly, Reviewed-by: A Acked-by: A Thanks Tiejun ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 00/16] Fix RMRR
On 16.07.15 at 10:13, tiejun.c...@intel.com wrote: It looks like most of the libxl/libxc patches have been acked. It seems to me that most of the hypervisor patches (1-3, 14-15) are either ready to go in or pretty close. Now that I looked over v8 I have to admit that if I was a tools maintainer I wouldn't want to see some of the tools patches in with just an ack, but without any review. I'm somewhat confused at this point. Acked-by: is often used by the maintainer of the affected code when that maintainer neither contributed to nor forwarded the patch. It is a record that the acker has at least reviewed the patch and has indicated acceptance. Does this imply this is already reviewed? No, that would be expressed by Reviewed-by. Acked-by merely means no objection by the maintainer for the change to go in. Sorry I'm trying to dig into this. If nobody would like to take a look at this, so isn't this the associated maintainer's responsibility to review finally? In this case isn't Acked-by fine enough? Acked-by is good enough for a patch to go in, yes. Note that I didn't make this a requirement (as I'm not the maintainer), I just said that if I was the maintainer, I would for at least some of the tools patches. And no, it is not the maintainer's role to do a review if no-one else did - that's why (s)he can ack it as an alternative, implying (s)he trusts the author without further review. Or you still want to us add two lines explicitly, Reviewed-by: A Acked-by: A We generally take Reviewed-by as a superset of Acked-by, so two tags by the same person are not needed. And (as said elsewhere recently) ack-s by non-maintainers generally don't count much. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 00/16] Fix RMRR
On Thu, Jul 16, 2015 at 09:30:54AM +0100, Ian Campbell wrote: On Thu, 2015-07-16 at 09:08 +0100, Jan Beulich wrote: On 16.07.15 at 10:03, tiejun.c...@intel.com wrote: On 2015/7/16 15:55, Jan Beulich wrote: On 10.07.15 at 16:50, george.dun...@eu.citrix.com wrote: On Thu, Jul 9, 2015 at 6:33 AM, Tiejun Chen tiejun.c...@intel.com wrote: v7: It looks like most of the libxl/libxc patches have been acked. It seems to me that most of the hypervisor patches (1-3, 14-15) are either ready to go in or pretty close. Now that I looked over v8 I have to admit that if I was a tools maintainer I wouldn't want to see some of the tools patches in with just an ack, but without any review. I'm somewhat confused at this point. Acked-by: is often used by the maintainer of the affected code when that maintainer neither contributed to nor forwarded the patch. It is a record that the acker has at least reviewed the patch and has indicated acceptance. Does this imply this is already reviewed? No, that would be expressed by Reviewed-by. Acked-by merely means no objection by the maintainer for the change to go in. For my part I, perhaps wrongly, use Acked-by for both. If I haven't actually carefully reviewed the change I will usually say so, e.g. I see XXX has reviewed this already, so that's fine by me or something similar (which I admit gets lost once it becomes just the tags). I can't speak for Ian or Wei (now CCd) but Ian at least I think operates similarly. I do the same. I will explicitly say that if I give my ack without looking at the code. I use Reviewed-by when the component is not maintained by me. Wei. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 00/16] Fix RMRR
On 10.07.15 at 16:50, george.dun...@eu.citrix.com wrote: On Thu, Jul 9, 2015 at 6:33 AM, Tiejun Chen tiejun.c...@intel.com wrote: v7: It looks like most of the libxl/libxc patches have been acked. It seems to me that most of the hypervisor patches (1-3, 14-15) are either ready to go in or pretty close. The main thing I think we're missing is the hvmloader stuff (5-7). Is that right? I looked through it for subsets of patches we could usefully check in, but it looks like the device MMIO range placement in the hvmloader patches are pretty crucial for proper functioning, even if we're not doing anything in particular with the memory layout (a la strategy=host). Yeah, putting in the hypervisor bits would make little sense without the hvmloader stuff going in at (about) the same time. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 00/16] Fix RMRR
On Thu, Jul 9, 2015 at 6:33 AM, Tiejun Chen tiejun.c...@intel.com wrote: v7: It looks like most of the libxl/libxc patches have been acked. It seems to me that most of the hypervisor patches (1-3, 14-15) are either ready to go in or pretty close. The main thing I think we're missing is the hvmloader stuff (5-7). Is that right? I looked through it for subsets of patches we could usefully check in, but it looks like the device MMIO range placement in the hvmloader patches are pretty crucial for proper functioning, even if we're not doing anything in particular with the memory layout (a la strategy=host). -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [v7][PATCH 00/16] Fix RMRR
v7: * Need to rename some parameters: In the xl rdm config parsing, `reserve=' should be `policy='. In the xl pci config parsing, `rdm_reserve=' should be `rdm_policy='. The type `libxl_rdm_reserve_flag' should be `libxl_rdm_policy'. The field name `reserve' in `libxl_rdm_reserve' should be `policy'. * Just sync with the fallout of renaming parameters above. Note I also mask patch #10 Acked by Wei Liu, Ian Jackson and Ian Campbell. ( If I'm wrong just let me know at this point. ) And as we discussed I'd further improve something as next step after this round of review. v6: * Inside patch #01, add a comments to the nr_entries field inside xen_reserved_device_memory_map. Note this is from Jan. * Inside patch #10, we need rename something to make our policy reasonable type - strategy none - ignore and based on our discussion, we won't expose ignore in xl level and just keep that as a default, and then sync docs and the patch head description * Inside patch #10, we fix some code stypes and especially we refine libxl__xc_device_get_rdm() * Inside patch #16, we need to sync those renames introduced by patch #10. v5: * Fold our original patch #2 and #3 as this new, and here introduce a new, clear_identity_p2m_entry, which can wrapper guest_physmap_remove_page(). And we use this to clean our identity mapping. * Just leave one bit XEN_DOMCTL_DEV_RDM_RELAXED as our policy flag, so now 0 means strict and 1 means relaxed, and also make DT device ignore the flag field simply. And then correct all associated code comments. * Just make sure the per-device plicy always override the global policy, and so cleanup some associated comments and the patch head description. * Improve some descriptions in doc. * Make all rdm variables specific to .hvm * Inside patch #6, we're trying to rename that field, is_64bar, inside struct bars with flag, and then extend to also indicate if this bar is already allocated. * Inside patch 11, Rename xc_device_get_rdm() with libxl__xc_device_get_rdm(), and then replace malloc() with libxl__malloc(), and finally cleanup this fallout. libxl__xc_device_get_rdm() should return proper libxl error code, ERROR_FAIL. Then instead, the allocated RDM entries would be returned with an out parameter. * The original patch #13 is sent out separately since actually this is not related to RMRR. v4: * Change one condition inside patch #2, xen/x86/p2m: introduce set_identity_p2m_entry, if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm ) to make sure we just catch our requirement. * Inside patch #3, xen/vtd: create RMRR mapping, Instead of intel_iommu_unmap_page(), we should use guest_physmap_remove_page() to unmap rmrr mapping correctly. And drop iommu_map_page() since actually ept_set_entry() can do this internally. * Inside patch #4, xen/passthrough: extend hypercall to support rdm reservation policy, add code comments to describer why we fix to set a policy flag in some cases like adding a device to hwdomain, and removing a device from user domain. And fix one judging condition domctl-u.assign_device.flag == XEN_DOMCTL_DEV_NO_RDM - domctl-u.assign_device.flag != XEN_DOMCTL_DEV_NO_RDM Additionally, also add to range check the flag passed to make future extensions possible (and to avoid ambiguity on what out of range values would mean). * Inside patch #6, hvmloader: get guest memory map into memory_map[], we move some codes related to e820 to that specific file, e820.c, and consolidate printf()+BUG() and BUG_ON(), and also avoid another fixed width type for the parameter of get_mem_mapping_layout() * Inside patch #7, hvmloader/pci: skip reserved ranges We have to re-design this as follows: #1. Goal MMIO region should exclude all reserved device memory #2. Requirements #2.1 Still need to make sure MMIO region is fit all pci devices as before #2.2 Accommodate the not aligned reserved memory regions If I'm missing something let me know. #3. How to #3.1 Address #2.1 We need to either of populating more RAM, or of expanding more highmem. But we should know just 64bit-bar can work with highmem, and as you mentioned we also should avoid expanding highmem as possible. So my implementation is to allocate 32bit-bar and 64bit-bar orderly. 1. The first allocation round just to 32bit-bar If we can finish allocating all 32bit-bar, we just go to allocate 64bit-bar with all remaining resources including low pci memory. If not, we need to calculate how much RAM should be populated to allocate the remaining 32bit-bars, then populate sufficient RAM as exp_mem_resource to go to the second allocation round 2. 2. The second allocation round to the remaining 32bit-bar We should can finish allocating all 32bit-bar in theory, then go to the third allocation round 3. 3. The third allocation round to 64bit-bar We'll try to first allocate from the remaining low
