Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
>>> On 29.01.19 at 16:02, wrote: > On Thu, Dec 13, 2018 at 07:47:59AM -0700, Jan Beulich wrote: >> >>> On 13.12.18 at 15:20, wrote: >> > On Thu, Dec 13, 2018 at 03:17:05AM -0700, Jan Beulich wrote: >> >> >>> On 13.12.18 at 10:14, wrote: >> >> > On Thu, Dec 13, 2018 at 12:45:07AM -0700, Jan Beulich wrote: >> >> >> >>> On 12.12.18 at 18:05, wrote: >> >> >> > On Wed, Dec 12, 2018 at 09:15:09AM -0700, Jan Beulich wrote: >> >> >> >> The MMIO side of things of course still remains unclear. >> >> >> > >> >> >> > Right, for the MMIO and the handling of grant and foreign mappings >> >> >> > it's >> >> >> > not clear how we want to proceed. >> >> >> > >> >> >> > Maybe account for all host RAM (total_pages) plus MMIO BARs? >> >> >> >> >> >> Well, I thought we've already settled on it being impossible to >> >> >> account for all MMIO BARs at this point. >> >> > >> >> > Well, I could iterate over all the registered PCI devices and size >> >> > the BARs (without VF BARs at least initially). This is quite >> >> > cumbersome, my other option would be using max_page and hope that >> >> > there are enough holes to make up for BAR MMIO regions. >> >> >> >> Well, maybe we could live with this for now. I certainly would >> >> prefer to have a 3rd opinion though, as I continue to feel uneasy >> >> with this rather imprecise estimation (i.e. I'd much prefer a more >> >> dynamic / on-demand approach). >> > >> > I agree it's not a perfect solution, but I think what's currently done >> > is even worse, and we already had bug reports of users seeing Xen >> > panic at PVH Dom0 build time if no dom0_mem parameter is specified. >> > >> > Would you be OK with using max_page then? >> >> I'm not going to say yes or no here without having seen a (qualified) >> 3rd opinion. > > I would like to get this fixed. The current code for accounting the > memory required for the paging structures is wrong, I've received > several reports (most of them privately) of the dom0 builder running > out of memory, thus leading to a panic. > > I'm open to suggestions. I'm afraid I'm the wrong addressee: I can't provide the asked for 3rd opinion. And if others think going from one often breaking solution to another, less often (but still) breaking one is a good idea, I won't block a change along those lines from going in. But it is kind of hard for me to understand why we would do such, instead of eliminating the problem altogether. Granted this may involve more intrusive a change. In any event PVH Dom0 is still experimental, and hence requiring people to use dom0_mem= for the time being does not seem overly unfriendly to me. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
On Thu, Dec 13, 2018 at 07:47:59AM -0700, Jan Beulich wrote: > >>> On 13.12.18 at 15:20, wrote: > > On Thu, Dec 13, 2018 at 03:17:05AM -0700, Jan Beulich wrote: > >> >>> On 13.12.18 at 10:14, wrote: > >> > On Thu, Dec 13, 2018 at 12:45:07AM -0700, Jan Beulich wrote: > >> >> >>> On 12.12.18 at 18:05, wrote: > >> >> > On Wed, Dec 12, 2018 at 09:15:09AM -0700, Jan Beulich wrote: > >> >> >> The MMIO side of things of course still remains unclear. > >> >> > > >> >> > Right, for the MMIO and the handling of grant and foreign mappings > >> >> > it's > >> >> > not clear how we want to proceed. > >> >> > > >> >> > Maybe account for all host RAM (total_pages) plus MMIO BARs? > >> >> > >> >> Well, I thought we've already settled on it being impossible to > >> >> account for all MMIO BARs at this point. > >> > > >> > Well, I could iterate over all the registered PCI devices and size > >> > the BARs (without VF BARs at least initially). This is quite > >> > cumbersome, my other option would be using max_page and hope that > >> > there are enough holes to make up for BAR MMIO regions. > >> > >> Well, maybe we could live with this for now. I certainly would > >> prefer to have a 3rd opinion though, as I continue to feel uneasy > >> with this rather imprecise estimation (i.e. I'd much prefer a more > >> dynamic / on-demand approach). > > > > I agree it's not a perfect solution, but I think what's currently done > > is even worse, and we already had bug reports of users seeing Xen > > panic at PVH Dom0 build time if no dom0_mem parameter is specified. > > > > Would you be OK with using max_page then? > > I'm not going to say yes or no here without having seen a (qualified) > 3rd opinion. I would like to get this fixed. The current code for accounting the memory required for the paging structures is wrong, I've received several reports (most of them privately) of the dom0 builder running out of memory, thus leading to a panic. I'm open to suggestions. Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
>>> On 13.12.18 at 15:20, wrote: > On Thu, Dec 13, 2018 at 03:17:05AM -0700, Jan Beulich wrote: >> >>> On 13.12.18 at 10:14, wrote: >> > On Thu, Dec 13, 2018 at 12:45:07AM -0700, Jan Beulich wrote: >> >> >>> On 12.12.18 at 18:05, wrote: >> >> > On Wed, Dec 12, 2018 at 09:15:09AM -0700, Jan Beulich wrote: >> >> >> The MMIO side of things of course still remains unclear. >> >> > >> >> > Right, for the MMIO and the handling of grant and foreign mappings it's >> >> > not clear how we want to proceed. >> >> > >> >> > Maybe account for all host RAM (total_pages) plus MMIO BARs? >> >> >> >> Well, I thought we've already settled on it being impossible to >> >> account for all MMIO BARs at this point. >> > >> > Well, I could iterate over all the registered PCI devices and size >> > the BARs (without VF BARs at least initially). This is quite >> > cumbersome, my other option would be using max_page and hope that >> > there are enough holes to make up for BAR MMIO regions. >> >> Well, maybe we could live with this for now. I certainly would >> prefer to have a 3rd opinion though, as I continue to feel uneasy >> with this rather imprecise estimation (i.e. I'd much prefer a more >> dynamic / on-demand approach). > > I agree it's not a perfect solution, but I think what's currently done > is even worse, and we already had bug reports of users seeing Xen > panic at PVH Dom0 build time if no dom0_mem parameter is specified. > > Would you be OK with using max_page then? I'm not going to say yes or no here without having seen a (qualified) 3rd opinion. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
On Thu, Dec 13, 2018 at 03:17:05AM -0700, Jan Beulich wrote: > >>> On 13.12.18 at 10:14, wrote: > > On Thu, Dec 13, 2018 at 12:45:07AM -0700, Jan Beulich wrote: > >> >>> On 12.12.18 at 18:05, wrote: > >> > On Wed, Dec 12, 2018 at 09:15:09AM -0700, Jan Beulich wrote: > >> >> The MMIO side of things of course still remains unclear. > >> > > >> > Right, for the MMIO and the handling of grant and foreign mappings it's > >> > not clear how we want to proceed. > >> > > >> > Maybe account for all host RAM (total_pages) plus MMIO BARs? > >> > >> Well, I thought we've already settled on it being impossible to > >> account for all MMIO BARs at this point. > > > > Well, I could iterate over all the registered PCI devices and size > > the BARs (without VF BARs at least initially). This is quite > > cumbersome, my other option would be using max_page and hope that > > there are enough holes to make up for BAR MMIO regions. > > Well, maybe we could live with this for now. I certainly would > prefer to have a 3rd opinion though, as I continue to feel uneasy > with this rather imprecise estimation (i.e. I'd much prefer a more > dynamic / on-demand approach). I agree it's not a perfect solution, but I think what's currently done is even worse, and we already had bug reports of users seeing Xen panic at PVH Dom0 build time if no dom0_mem parameter is specified. Would you be OK with using max_page then? This is the less complex option to implement ATM, and BAR sizing can be added later together with a more dynamic p2m memory management. Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
>>> On 13.12.18 at 10:14, wrote: > On Thu, Dec 13, 2018 at 12:45:07AM -0700, Jan Beulich wrote: >> >>> On 12.12.18 at 18:05, wrote: >> > On Wed, Dec 12, 2018 at 09:15:09AM -0700, Jan Beulich wrote: >> >> The MMIO side of things of course still remains unclear. >> > >> > Right, for the MMIO and the handling of grant and foreign mappings it's >> > not clear how we want to proceed. >> > >> > Maybe account for all host RAM (total_pages) plus MMIO BARs? >> >> Well, I thought we've already settled on it being impossible to >> account for all MMIO BARs at this point. > > Well, I could iterate over all the registered PCI devices and size > the BARs (without VF BARs at least initially). This is quite > cumbersome, my other option would be using max_page and hope that > there are enough holes to make up for BAR MMIO regions. Well, maybe we could live with this for now. I certainly would prefer to have a 3rd opinion though, as I continue to feel uneasy with this rather imprecise estimation (i.e. I'd much prefer a more dynamic / on-demand approach). Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
On Thu, Dec 13, 2018 at 12:45:07AM -0700, Jan Beulich wrote: > >>> On 12.12.18 at 18:05, wrote: > > On Wed, Dec 12, 2018 at 09:15:09AM -0700, Jan Beulich wrote: > >> The MMIO side of things of course still remains unclear. > > > > Right, for the MMIO and the handling of grant and foreign mappings it's > > not clear how we want to proceed. > > > > Maybe account for all host RAM (total_pages) plus MMIO BARs? > > Well, I thought we've already settled on it being impossible to > account for all MMIO BARs at this point. Well, I could iterate over all the registered PCI devices and size the BARs (without VF BARs at least initially). This is quite cumbersome, my other option would be using max_page and hope that there are enough holes to make up for BAR MMIO regions. Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
>>> On 12.12.18 at 18:05, wrote: > On Wed, Dec 12, 2018 at 09:15:09AM -0700, Jan Beulich wrote: >> The MMIO side of things of course still remains unclear. > > Right, for the MMIO and the handling of grant and foreign mappings it's > not clear how we want to proceed. > > Maybe account for all host RAM (total_pages) plus MMIO BARs? Well, I thought we've already settled on it being impossible to account for all MMIO BARs at this point. >> What I don't understand in any case though is >> "PAGE_SIZE << PAGE_ORDER_4K". This is x86 code - why not >> just PAGE_SIZE? > > Oh, I've done it like that because this is related to p2m code, which > uses this way to get the page size. IIRC you told me to use this for > things like pvh_setup_e820. I don't mind switching to just PAGE_SIZE. Oh, I see. It's fine either way then. My general way of thinking here is that outside of x86 code we better use these PAGE_ORDER_* values, while in x86 specific code I don't see the point. But indeed the p2m code is littered with them. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
On Wed, Dec 12, 2018 at 09:15:09AM -0700, Jan Beulich wrote: > >>> On 12.12.18 at 16:56, wrote: > > On Wed, Dec 12, 2018 at 03:32:53AM -0700, Jan Beulich wrote: > >> >>> On 12.12.18 at 11:04, wrote: > >> > You mentioned there's some code (for PV?) to calculate the size of the > >> > page tables but I'm having trouble finding it (mainly because I'm not > >> > that familiar with PV), could you point me to it? > >> > >> In dom0_construct_pv() you'll find a loop starting with > >> "for ( nr_pt_pages = 2; ; nr_pt_pages++ )". It's not the neatest, > >> but at least we've never had reports of failure. > > > > That seems quite complicated, what about using the formula below: > > > > /* > > * Approximate the memory required for the HAP/IOMMU page tables by > > * pessimistically assuming every guest page will use a p2m page table > > * entry. > > */ > > return DIV_ROUND_UP(( > > /* Account for one entry in the L1 per page. */ > > nr_pages + > > /* Account for one entry in the L2 per 512 pages. */ > > DIV_ROUND_UP(nr_pages, 512) + > > /* Account for one entry in the L3 per 512^2 pages. */ > > DIV_ROUND_UP(nr_pages, 512 * 512) + > > /* Account for one entry in the L4 per 512^3 pages. */ > > DIV_ROUND_UP(nr_pages, 512 * 512 * 512) + > > ) * 8, PAGE_SIZE << PAGE_ORDER_4K); > > > > That takes into account higher level page table structures. > > That's a fair approximation without 2M and 1G pages available. I'm > unconvinced we want to over-estimate this heavily in the more > common case of large page mappings being available. Otoh this > provides enough resources to later also deal with shattering of > large pages. > > The MMIO side of things of course still remains unclear. Right, for the MMIO and the handling of grant and foreign mappings it's not clear how we want to proceed. Maybe account for all host RAM (total_pages) plus MMIO BARs? > What I don't understand in any case though is > "PAGE_SIZE << PAGE_ORDER_4K". This is x86 code - why not > just PAGE_SIZE? Oh, I've done it like that because this is related to p2m code, which uses this way to get the page size. IIRC you told me to use this for things like pvh_setup_e820. I don't mind switching to just PAGE_SIZE. Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
>>> On 12.12.18 at 16:56, wrote: > On Wed, Dec 12, 2018 at 03:32:53AM -0700, Jan Beulich wrote: >> >>> On 12.12.18 at 11:04, wrote: >> > You mentioned there's some code (for PV?) to calculate the size of the >> > page tables but I'm having trouble finding it (mainly because I'm not >> > that familiar with PV), could you point me to it? >> >> In dom0_construct_pv() you'll find a loop starting with >> "for ( nr_pt_pages = 2; ; nr_pt_pages++ )". It's not the neatest, >> but at least we've never had reports of failure. > > That seems quite complicated, what about using the formula below: > > /* > * Approximate the memory required for the HAP/IOMMU page tables by > * pessimistically assuming every guest page will use a p2m page table > * entry. > */ > return DIV_ROUND_UP(( > /* Account for one entry in the L1 per page. */ > nr_pages + > /* Account for one entry in the L2 per 512 pages. */ > DIV_ROUND_UP(nr_pages, 512) + > /* Account for one entry in the L3 per 512^2 pages. */ > DIV_ROUND_UP(nr_pages, 512 * 512) + > /* Account for one entry in the L4 per 512^3 pages. */ > DIV_ROUND_UP(nr_pages, 512 * 512 * 512) + > ) * 8, PAGE_SIZE << PAGE_ORDER_4K); > > That takes into account higher level page table structures. That's a fair approximation without 2M and 1G pages available. I'm unconvinced we want to over-estimate this heavily in the more common case of large page mappings being available. Otoh this provides enough resources to later also deal with shattering of large pages. The MMIO side of things of course still remains unclear. What I don't understand in any case though is "PAGE_SIZE << PAGE_ORDER_4K". This is x86 code - why not just PAGE_SIZE? Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
On Wed, Dec 12, 2018 at 03:32:53AM -0700, Jan Beulich wrote: > >>> On 12.12.18 at 11:04, wrote: > > On Wed, Dec 12, 2018 at 02:53:26AM -0700, Jan Beulich wrote: > >> >>> On 12.12.18 at 10:14, wrote: > >> > On Tue, Dec 11, 2018 at 08:33:08AM -0700, Jan Beulich wrote: > >> >> >>> On 11.12.18 at 16:19, wrote: > >> >> > On Tue, Dec 11, 2018 at 08:08:51AM -0700, Jan Beulich wrote: > >> >> >> >>> On 05.12.18 at 15:54, wrote: > >> >> >> > To note it's calculating the approximate amount of memory required > >> >> >> > by > >> >> >> > shadow paging. > >> >> >> > >> >> >> I don't understand this logic, and ... > >> >> >> > >> >> >> > @@ -325,7 +325,7 @@ unsigned long __init dom0_compute_nr_pages( > >> >> >> > break; > >> >> >> > > >> >> >> > /* Reserve memory for shadow or HAP. */ > >> >> >> > -avail -= dom0_paging_pages(d, nr_pages); > >> >> >> > +avail -= dom0_shadow_pages(d, nr_pages); > >> >> >> > } > >> >> >> > >> >> >> ... the comment here (and lack of conditional restricting the > >> >> >> code to shadow mode) appear to support me: Have you > >> >> >> been mislead by the function having a comment referring > >> >> >> to libxl_get_required_shadow_memory()? I think if anything > >> >> >> that libxl function would want to be renamed (to replace > >> >> >> "shadow" by something more generic in its name). > >> >> > > >> >> > But the logic in dom0_shadow_pages to calculate the size of the paging > >> >> > memory pool is specifically for shadow AFAICT, I don't think HAP needs > >> >> > to take the number of vCPUs into account, since there's only a > >> >> > single p2m for the whole domain. OTOH shadow needs to take the number > >> >> > of vCPUs into account because each one will have a different shadow. > >> >> > >> >> Yes, the vCPU count aspect is indeed shadow specific. However, > >> >> as said in reply to the other patch, the calculation here was at > >> >> least supposed to also take into account the P2M part of the > >> >> needed allocations. Yet the P2M part ought to be similar between > >> >> both modes. > >> >> > >> >> > Note that patch 2 in this series adds a function to calculate the size > >> >> > of the paging memory pool for HAP, and a conditional is added to the > >> >> > expression above that takes into account whether shadow or HAP is in > >> >> > use when subtracting from the amount of available memory. > >> >> > >> >> Well, assuming we can settle on what shape patch 2 should take > >> >> I can see the point in doing the rename here, but then with an > >> >> adjusted description: Especially in light of the code comment still > >> >> visible above you'll want to point out that the rename is in > >> >> preparation of splitting the calculations. Since I question the split, > >> >> though, the rename (in a separate patch) is questionable to me > >> >> too. If we used uniform P2M calculations and added just shadow's > >> >> per-vCPU extra on top, no rename in a separate patch would > >> >> seem warranted. > >> > > >> > The current calculations in dom0_paging_pages assume 1 page is needed > >> > for each 1MB of guest memory for the p2m, do you think this is OK? > >> > (and suitable to be used for HAP/IOMMU page tables also) > >> > >> Well, 1 page per 1Mb means the same as your current 8 bytes > >> per page times 2 (for separate P2M and IOMMU tables), afaict. > > > > I was planning to use 1 page per 1Mb for the p2m, and then 1 page per > > 1Mb for the IOMMU, so 16 bytes per page. > > Well, that's (as said for patch 2) quite a bit of an over-estimate, > but then again reserving a little too much is perhaps better than > reserving too little. > > > You mentioned there's some code (for PV?) to calculate the size of the > > page tables but I'm having trouble finding it (mainly because I'm not > > that familiar with PV), could you point me to it? > > In dom0_construct_pv() you'll find a loop starting with > "for ( nr_pt_pages = 2; ; nr_pt_pages++ )". It's not the neatest, > but at least we've never had reports of failure. That seems quite complicated, what about using the formula below: /* * Approximate the memory required for the HAP/IOMMU page tables by * pessimistically assuming every guest page will use a p2m page table * entry. */ return DIV_ROUND_UP(( /* Account for one entry in the L1 per page. */ nr_pages + /* Account for one entry in the L2 per 512 pages. */ DIV_ROUND_UP(nr_pages, 512) + /* Account for one entry in the L3 per 512^2 pages. */ DIV_ROUND_UP(nr_pages, 512 * 512) + /* Account for one entry in the L4 per 512^3 pages. */ DIV_ROUND_UP(nr_pages, 512 * 512 * 512) + ) * 8, PAGE_SIZE << PAGE_ORDER_4K); That takes into account higher level page table structures. Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
>>> On 12.12.18 at 11:04, wrote: > On Wed, Dec 12, 2018 at 02:53:26AM -0700, Jan Beulich wrote: >> >>> On 12.12.18 at 10:14, wrote: >> > On Tue, Dec 11, 2018 at 08:33:08AM -0700, Jan Beulich wrote: >> >> >>> On 11.12.18 at 16:19, wrote: >> >> > On Tue, Dec 11, 2018 at 08:08:51AM -0700, Jan Beulich wrote: >> >> >> >>> On 05.12.18 at 15:54, wrote: >> >> >> > To note it's calculating the approximate amount of memory required by >> >> >> > shadow paging. >> >> >> >> >> >> I don't understand this logic, and ... >> >> >> >> >> >> > @@ -325,7 +325,7 @@ unsigned long __init dom0_compute_nr_pages( >> >> >> > break; >> >> >> > >> >> >> > /* Reserve memory for shadow or HAP. */ >> >> >> > -avail -= dom0_paging_pages(d, nr_pages); >> >> >> > +avail -= dom0_shadow_pages(d, nr_pages); >> >> >> > } >> >> >> >> >> >> ... the comment here (and lack of conditional restricting the >> >> >> code to shadow mode) appear to support me: Have you >> >> >> been mislead by the function having a comment referring >> >> >> to libxl_get_required_shadow_memory()? I think if anything >> >> >> that libxl function would want to be renamed (to replace >> >> >> "shadow" by something more generic in its name). >> >> > >> >> > But the logic in dom0_shadow_pages to calculate the size of the paging >> >> > memory pool is specifically for shadow AFAICT, I don't think HAP needs >> >> > to take the number of vCPUs into account, since there's only a >> >> > single p2m for the whole domain. OTOH shadow needs to take the number >> >> > of vCPUs into account because each one will have a different shadow. >> >> >> >> Yes, the vCPU count aspect is indeed shadow specific. However, >> >> as said in reply to the other patch, the calculation here was at >> >> least supposed to also take into account the P2M part of the >> >> needed allocations. Yet the P2M part ought to be similar between >> >> both modes. >> >> >> >> > Note that patch 2 in this series adds a function to calculate the size >> >> > of the paging memory pool for HAP, and a conditional is added to the >> >> > expression above that takes into account whether shadow or HAP is in >> >> > use when subtracting from the amount of available memory. >> >> >> >> Well, assuming we can settle on what shape patch 2 should take >> >> I can see the point in doing the rename here, but then with an >> >> adjusted description: Especially in light of the code comment still >> >> visible above you'll want to point out that the rename is in >> >> preparation of splitting the calculations. Since I question the split, >> >> though, the rename (in a separate patch) is questionable to me >> >> too. If we used uniform P2M calculations and added just shadow's >> >> per-vCPU extra on top, no rename in a separate patch would >> >> seem warranted. >> > >> > The current calculations in dom0_paging_pages assume 1 page is needed >> > for each 1MB of guest memory for the p2m, do you think this is OK? >> > (and suitable to be used for HAP/IOMMU page tables also) >> >> Well, 1 page per 1Mb means the same as your current 8 bytes >> per page times 2 (for separate P2M and IOMMU tables), afaict. > > I was planning to use 1 page per 1Mb for the p2m, and then 1 page per > 1Mb for the IOMMU, so 16 bytes per page. Well, that's (as said for patch 2) quite a bit of an over-estimate, but then again reserving a little too much is perhaps better than reserving too little. > You mentioned there's some code (for PV?) to calculate the size of the > page tables but I'm having trouble finding it (mainly because I'm not > that familiar with PV), could you point me to it? In dom0_construct_pv() you'll find a loop starting with "for ( nr_pt_pages = 2; ; nr_pt_pages++ )". It's not the neatest, but at least we've never had reports of failure. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
On Wed, Dec 12, 2018 at 02:53:26AM -0700, Jan Beulich wrote: > >>> On 12.12.18 at 10:14, wrote: > > On Tue, Dec 11, 2018 at 08:33:08AM -0700, Jan Beulich wrote: > >> >>> On 11.12.18 at 16:19, wrote: > >> > On Tue, Dec 11, 2018 at 08:08:51AM -0700, Jan Beulich wrote: > >> >> >>> On 05.12.18 at 15:54, wrote: > >> >> > To note it's calculating the approximate amount of memory required by > >> >> > shadow paging. > >> >> > >> >> I don't understand this logic, and ... > >> >> > >> >> > @@ -325,7 +325,7 @@ unsigned long __init dom0_compute_nr_pages( > >> >> > break; > >> >> > > >> >> > /* Reserve memory for shadow or HAP. */ > >> >> > -avail -= dom0_paging_pages(d, nr_pages); > >> >> > +avail -= dom0_shadow_pages(d, nr_pages); > >> >> > } > >> >> > >> >> ... the comment here (and lack of conditional restricting the > >> >> code to shadow mode) appear to support me: Have you > >> >> been mislead by the function having a comment referring > >> >> to libxl_get_required_shadow_memory()? I think if anything > >> >> that libxl function would want to be renamed (to replace > >> >> "shadow" by something more generic in its name). > >> > > >> > But the logic in dom0_shadow_pages to calculate the size of the paging > >> > memory pool is specifically for shadow AFAICT, I don't think HAP needs > >> > to take the number of vCPUs into account, since there's only a > >> > single p2m for the whole domain. OTOH shadow needs to take the number > >> > of vCPUs into account because each one will have a different shadow. > >> > >> Yes, the vCPU count aspect is indeed shadow specific. However, > >> as said in reply to the other patch, the calculation here was at > >> least supposed to also take into account the P2M part of the > >> needed allocations. Yet the P2M part ought to be similar between > >> both modes. > >> > >> > Note that patch 2 in this series adds a function to calculate the size > >> > of the paging memory pool for HAP, and a conditional is added to the > >> > expression above that takes into account whether shadow or HAP is in > >> > use when subtracting from the amount of available memory. > >> > >> Well, assuming we can settle on what shape patch 2 should take > >> I can see the point in doing the rename here, but then with an > >> adjusted description: Especially in light of the code comment still > >> visible above you'll want to point out that the rename is in > >> preparation of splitting the calculations. Since I question the split, > >> though, the rename (in a separate patch) is questionable to me > >> too. If we used uniform P2M calculations and added just shadow's > >> per-vCPU extra on top, no rename in a separate patch would > >> seem warranted. > > > > The current calculations in dom0_paging_pages assume 1 page is needed > > for each 1MB of guest memory for the p2m, do you think this is OK? > > (and suitable to be used for HAP/IOMMU page tables also) > > Well, 1 page per 1Mb means the same as your current 8 bytes > per page times 2 (for separate P2M and IOMMU tables), afaict. I was planning to use 1 page per 1Mb for the p2m, and then 1 page per 1Mb for the IOMMU, so 16 bytes per page. You mentioned there's some code (for PV?) to calculate the size of the page tables but I'm having trouble finding it (mainly because I'm not that familiar with PV), could you point me to it? Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
>>> On 12.12.18 at 10:14, wrote: > On Tue, Dec 11, 2018 at 08:33:08AM -0700, Jan Beulich wrote: >> >>> On 11.12.18 at 16:19, wrote: >> > On Tue, Dec 11, 2018 at 08:08:51AM -0700, Jan Beulich wrote: >> >> >>> On 05.12.18 at 15:54, wrote: >> >> > To note it's calculating the approximate amount of memory required by >> >> > shadow paging. >> >> >> >> I don't understand this logic, and ... >> >> >> >> > @@ -325,7 +325,7 @@ unsigned long __init dom0_compute_nr_pages( >> >> > break; >> >> > >> >> > /* Reserve memory for shadow or HAP. */ >> >> > -avail -= dom0_paging_pages(d, nr_pages); >> >> > +avail -= dom0_shadow_pages(d, nr_pages); >> >> > } >> >> >> >> ... the comment here (and lack of conditional restricting the >> >> code to shadow mode) appear to support me: Have you >> >> been mislead by the function having a comment referring >> >> to libxl_get_required_shadow_memory()? I think if anything >> >> that libxl function would want to be renamed (to replace >> >> "shadow" by something more generic in its name). >> > >> > But the logic in dom0_shadow_pages to calculate the size of the paging >> > memory pool is specifically for shadow AFAICT, I don't think HAP needs >> > to take the number of vCPUs into account, since there's only a >> > single p2m for the whole domain. OTOH shadow needs to take the number >> > of vCPUs into account because each one will have a different shadow. >> >> Yes, the vCPU count aspect is indeed shadow specific. However, >> as said in reply to the other patch, the calculation here was at >> least supposed to also take into account the P2M part of the >> needed allocations. Yet the P2M part ought to be similar between >> both modes. >> >> > Note that patch 2 in this series adds a function to calculate the size >> > of the paging memory pool for HAP, and a conditional is added to the >> > expression above that takes into account whether shadow or HAP is in >> > use when subtracting from the amount of available memory. >> >> Well, assuming we can settle on what shape patch 2 should take >> I can see the point in doing the rename here, but then with an >> adjusted description: Especially in light of the code comment still >> visible above you'll want to point out that the rename is in >> preparation of splitting the calculations. Since I question the split, >> though, the rename (in a separate patch) is questionable to me >> too. If we used uniform P2M calculations and added just shadow's >> per-vCPU extra on top, no rename in a separate patch would >> seem warranted. > > The current calculations in dom0_paging_pages assume 1 page is needed > for each 1MB of guest memory for the p2m, do you think this is OK? > (and suitable to be used for HAP/IOMMU page tables also) Well, 1 page per 1Mb means the same as your current 8 bytes per page times 2 (for separate P2M and IOMMU tables), afaict. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
On Tue, Dec 11, 2018 at 08:33:08AM -0700, Jan Beulich wrote: > >>> On 11.12.18 at 16:19, wrote: > > On Tue, Dec 11, 2018 at 08:08:51AM -0700, Jan Beulich wrote: > >> >>> On 05.12.18 at 15:54, wrote: > >> > To note it's calculating the approximate amount of memory required by > >> > shadow paging. > >> > >> I don't understand this logic, and ... > >> > >> > @@ -325,7 +325,7 @@ unsigned long __init dom0_compute_nr_pages( > >> > break; > >> > > >> > /* Reserve memory for shadow or HAP. */ > >> > -avail -= dom0_paging_pages(d, nr_pages); > >> > +avail -= dom0_shadow_pages(d, nr_pages); > >> > } > >> > >> ... the comment here (and lack of conditional restricting the > >> code to shadow mode) appear to support me: Have you > >> been mislead by the function having a comment referring > >> to libxl_get_required_shadow_memory()? I think if anything > >> that libxl function would want to be renamed (to replace > >> "shadow" by something more generic in its name). > > > > But the logic in dom0_shadow_pages to calculate the size of the paging > > memory pool is specifically for shadow AFAICT, I don't think HAP needs > > to take the number of vCPUs into account, since there's only a > > single p2m for the whole domain. OTOH shadow needs to take the number > > of vCPUs into account because each one will have a different shadow. > > Yes, the vCPU count aspect is indeed shadow specific. However, > as said in reply to the other patch, the calculation here was at > least supposed to also take into account the P2M part of the > needed allocations. Yet the P2M part ought to be similar between > both modes. > > > Note that patch 2 in this series adds a function to calculate the size > > of the paging memory pool for HAP, and a conditional is added to the > > expression above that takes into account whether shadow or HAP is in > > use when subtracting from the amount of available memory. > > Well, assuming we can settle on what shape patch 2 should take > I can see the point in doing the rename here, but then with an > adjusted description: Especially in light of the code comment still > visible above you'll want to point out that the rename is in > preparation of splitting the calculations. Since I question the split, > though, the rename (in a separate patch) is questionable to me > too. If we used uniform P2M calculations and added just shadow's > per-vCPU extra on top, no rename in a separate patch would > seem warranted. The current calculations in dom0_paging_pages assume 1 page is needed for each 1MB of guest memory for the p2m, do you think this is OK? (and suitable to be used for HAP/IOMMU page tables also) Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
>>> On 11.12.18 at 16:19, wrote: > On Tue, Dec 11, 2018 at 08:08:51AM -0700, Jan Beulich wrote: >> >>> On 05.12.18 at 15:54, wrote: >> > To note it's calculating the approximate amount of memory required by >> > shadow paging. >> >> I don't understand this logic, and ... >> >> > @@ -325,7 +325,7 @@ unsigned long __init dom0_compute_nr_pages( >> > break; >> > >> > /* Reserve memory for shadow or HAP. */ >> > -avail -= dom0_paging_pages(d, nr_pages); >> > +avail -= dom0_shadow_pages(d, nr_pages); >> > } >> >> ... the comment here (and lack of conditional restricting the >> code to shadow mode) appear to support me: Have you >> been mislead by the function having a comment referring >> to libxl_get_required_shadow_memory()? I think if anything >> that libxl function would want to be renamed (to replace >> "shadow" by something more generic in its name). > > But the logic in dom0_shadow_pages to calculate the size of the paging > memory pool is specifically for shadow AFAICT, I don't think HAP needs > to take the number of vCPUs into account, since there's only a > single p2m for the whole domain. OTOH shadow needs to take the number > of vCPUs into account because each one will have a different shadow. Yes, the vCPU count aspect is indeed shadow specific. However, as said in reply to the other patch, the calculation here was at least supposed to also take into account the P2M part of the needed allocations. Yet the P2M part ought to be similar between both modes. > Note that patch 2 in this series adds a function to calculate the size > of the paging memory pool for HAP, and a conditional is added to the > expression above that takes into account whether shadow or HAP is in > use when subtracting from the amount of available memory. Well, assuming we can settle on what shape patch 2 should take I can see the point in doing the rename here, but then with an adjusted description: Especially in light of the code comment still visible above you'll want to point out that the rename is in preparation of splitting the calculations. Since I question the split, though, the rename (in a separate patch) is questionable to me too. If we used uniform P2M calculations and added just shadow's per-vCPU extra on top, no rename in a separate patch would seem warranted. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
On Tue, Dec 11, 2018 at 08:08:51AM -0700, Jan Beulich wrote: > >>> On 05.12.18 at 15:54, wrote: > > To note it's calculating the approximate amount of memory required by > > shadow paging. > > I don't understand this logic, and ... > > > @@ -325,7 +325,7 @@ unsigned long __init dom0_compute_nr_pages( > > break; > > > > /* Reserve memory for shadow or HAP. */ > > -avail -= dom0_paging_pages(d, nr_pages); > > +avail -= dom0_shadow_pages(d, nr_pages); > > } > > ... the comment here (and lack of conditional restricting the > code to shadow mode) appear to support me: Have you > been mislead by the function having a comment referring > to libxl_get_required_shadow_memory()? I think if anything > that libxl function would want to be renamed (to replace > "shadow" by something more generic in its name). But the logic in dom0_shadow_pages to calculate the size of the paging memory pool is specifically for shadow AFAICT, I don't think HAP needs to take the number of vCPUs into account, since there's only a single p2m for the whole domain. OTOH shadow needs to take the number of vCPUs into account because each one will have a different shadow. Note that patch 2 in this series adds a function to calculate the size of the paging memory pool for HAP, and a conditional is added to the expression above that takes into account whether shadow or HAP is in use when subtracting from the amount of available memory. Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
>>> On 05.12.18 at 15:54, wrote: > To note it's calculating the approximate amount of memory required by > shadow paging. I don't understand this logic, and ... > @@ -325,7 +325,7 @@ unsigned long __init dom0_compute_nr_pages( > break; > > /* Reserve memory for shadow or HAP. */ > -avail -= dom0_paging_pages(d, nr_pages); > +avail -= dom0_shadow_pages(d, nr_pages); > } ... the comment here (and lack of conditional restricting the code to shadow mode) appear to support me: Have you been mislead by the function having a comment referring to libxl_get_required_shadow_memory()? I think if anything that libxl function would want to be renamed (to replace "shadow" by something more generic in its name). Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function
On Wed, Dec 05, 2018 at 03:54:59PM +0100, Roger Pau Monne wrote: > To note it's calculating the approximate amount of memory required by > shadow paging. > > No functional change. > > Signed-off-by: Roger Pau Monné Reviewed-by: Wei Liu ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
