| Message ID | 20210616073007.5215-2-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: fix max_pfn handling for pv guests | expand |
On 16.06.2021 09:30, Juergen Gross wrote: > Xen PV guests are specifying the highest used PFN via the max_pfn > field in shared_info. This value is used by the Xen tools when saving > or migrating the guest. > > Unfortunately this field is misnamed, as in reality it is specifying > the number of pages (including any memory holes) of the guest, so it > is the highest used PFN + 1. Renaming isn't possible, as this is a > public Xen hypervisor interface which needs to be kept stable. > > The kernel will set the value correctly initially at boot time, but > when adding more pages (e.g. due to memory hotplug or ballooning) a > real PFN number is stored in max_pfn. This is done when expanding the > p2m array, and the PFN stored there is even possibly wrong, as it > should be the last possible PFN of the just added P2M frame, and not > one which led to the P2M expansion. > > Fix that by setting shared_info->max_pfn to the last possible PFN + 1. > > Fixes: 98dd166ea3a3c3 ("x86/xen/p2m: hint at the last populated P2M entry") > Cc: stable@vger.kernel.org > Signed-off-by: Juergen Gross <jgross@suse.com> The code change is fine, so Reviewed-by: Jan Beulich <jbeulich@suse.com> But I think even before the rename you would want to clarify the comment next to the variable's definition, to make clear what it really holds. Jan
On 16.06.21 11:52, Jan Beulich wrote: > On 16.06.2021 09:30, Juergen Gross wrote: >> Xen PV guests are specifying the highest used PFN via the max_pfn >> field in shared_info. This value is used by the Xen tools when saving >> or migrating the guest. >> >> Unfortunately this field is misnamed, as in reality it is specifying >> the number of pages (including any memory holes) of the guest, so it >> is the highest used PFN + 1. Renaming isn't possible, as this is a >> public Xen hypervisor interface which needs to be kept stable. >> >> The kernel will set the value correctly initially at boot time, but >> when adding more pages (e.g. due to memory hotplug or ballooning) a >> real PFN number is stored in max_pfn. This is done when expanding the >> p2m array, and the PFN stored there is even possibly wrong, as it >> should be the last possible PFN of the just added P2M frame, and not >> one which led to the P2M expansion. >> >> Fix that by setting shared_info->max_pfn to the last possible PFN + 1. >> >> Fixes: 98dd166ea3a3c3 ("x86/xen/p2m: hint at the last populated P2M entry") >> Cc: stable@vger.kernel.org >> Signed-off-by: Juergen Gross <jgross@suse.com> > > The code change is fine, so > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > But I think even before the rename you would want to clarify the comment > next to the variable's definition, to make clear what it really holds. It already says: "Number of valid entries in the p2m table(s) ..." What do you think is unclear about that? Or do you mean another variable? Juergen
On 16.06.2021 12:37, Juergen Gross wrote: > On 16.06.21 11:52, Jan Beulich wrote: >> On 16.06.2021 09:30, Juergen Gross wrote: >>> Xen PV guests are specifying the highest used PFN via the max_pfn >>> field in shared_info. This value is used by the Xen tools when saving >>> or migrating the guest. >>> >>> Unfortunately this field is misnamed, as in reality it is specifying >>> the number of pages (including any memory holes) of the guest, so it >>> is the highest used PFN + 1. Renaming isn't possible, as this is a >>> public Xen hypervisor interface which needs to be kept stable. >>> >>> The kernel will set the value correctly initially at boot time, but >>> when adding more pages (e.g. due to memory hotplug or ballooning) a >>> real PFN number is stored in max_pfn. This is done when expanding the >>> p2m array, and the PFN stored there is even possibly wrong, as it >>> should be the last possible PFN of the just added P2M frame, and not >>> one which led to the P2M expansion. >>> >>> Fix that by setting shared_info->max_pfn to the last possible PFN + 1. >>> >>> Fixes: 98dd166ea3a3c3 ("x86/xen/p2m: hint at the last populated P2M entry") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Juergen Gross <jgross@suse.com> >> >> The code change is fine, so >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> >> But I think even before the rename you would want to clarify the comment >> next to the variable's definition, to make clear what it really holds. > > It already says: "Number of valid entries in the p2m table(s) ..." > What do you think is unclear about that? Or do you mean another > variable? I mean the variable the value of which the patch corrects, i.e. xen_p2m_last_pfn. What I see in current source is /* * Hint at last populated PFN. * * Used to set HYPERVISOR_shared_info->arch.max_pfn so the toolstack * can avoid scanning the whole P2M (which may be sized to account for * hotplugged memory). */ static unsigned long xen_p2m_last_pfn; Jan
On 16.06.21 12:56, Jan Beulich wrote: > On 16.06.2021 12:37, Juergen Gross wrote: >> On 16.06.21 11:52, Jan Beulich wrote: >>> On 16.06.2021 09:30, Juergen Gross wrote: >>>> Xen PV guests are specifying the highest used PFN via the max_pfn >>>> field in shared_info. This value is used by the Xen tools when saving >>>> or migrating the guest. >>>> >>>> Unfortunately this field is misnamed, as in reality it is specifying >>>> the number of pages (including any memory holes) of the guest, so it >>>> is the highest used PFN + 1. Renaming isn't possible, as this is a >>>> public Xen hypervisor interface which needs to be kept stable. >>>> >>>> The kernel will set the value correctly initially at boot time, but >>>> when adding more pages (e.g. due to memory hotplug or ballooning) a >>>> real PFN number is stored in max_pfn. This is done when expanding the >>>> p2m array, and the PFN stored there is even possibly wrong, as it >>>> should be the last possible PFN of the just added P2M frame, and not >>>> one which led to the P2M expansion. >>>> >>>> Fix that by setting shared_info->max_pfn to the last possible PFN + 1. >>>> >>>> Fixes: 98dd166ea3a3c3 ("x86/xen/p2m: hint at the last populated P2M entry") >>>> Cc: stable@vger.kernel.org >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> >>> The code change is fine, so >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> >>> But I think even before the rename you would want to clarify the comment >>> next to the variable's definition, to make clear what it really holds. >> >> It already says: "Number of valid entries in the p2m table(s) ..." >> What do you think is unclear about that? Or do you mean another >> variable? > > I mean the variable the value of which the patch corrects, i.e. > xen_p2m_last_pfn. What I see in current source is > > /* > * Hint at last populated PFN. > * > * Used to set HYPERVISOR_shared_info->arch.max_pfn so the toolstack > * can avoid scanning the whole P2M (which may be sized to account for > * hotplugged memory). > */ > static unsigned long xen_p2m_last_pfn; Ah, okay. I think only changing the comment without renaming the variable isn't the way to go. In order to keep the to be backported patch small I'd rather do the comment adjustment and variable renaming in the followup patch. Juergen
diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c index ac06ca32e9ef..5e6e236977c7 100644 --- a/arch/x86/xen/p2m.c +++ b/arch/x86/xen/p2m.c @@ -618,8 +618,8 @@ int xen_alloc_p2m_entry(unsigned long pfn) } /* Expanded the p2m? */ - if (pfn > xen_p2m_last_pfn) { - xen_p2m_last_pfn = pfn; + if (pfn >= xen_p2m_last_pfn) { + xen_p2m_last_pfn = ALIGN(pfn + 1, P2M_PER_PAGE); HYPERVISOR_shared_info->arch.max_pfn = xen_p2m_last_pfn; }
Xen PV guests are specifying the highest used PFN via the max_pfn field in shared_info. This value is used by the Xen tools when saving or migrating the guest. Unfortunately this field is misnamed, as in reality it is specifying the number of pages (including any memory holes) of the guest, so it is the highest used PFN + 1. Renaming isn't possible, as this is a public Xen hypervisor interface which needs to be kept stable. The kernel will set the value correctly initially at boot time, but when adding more pages (e.g. due to memory hotplug or ballooning) a real PFN number is stored in max_pfn. This is done when expanding the p2m array, and the PFN stored there is even possibly wrong, as it should be the last possible PFN of the just added P2M frame, and not one which led to the P2M expansion. Fix that by setting shared_info->max_pfn to the last possible PFN + 1. Fixes: 98dd166ea3a3c3 ("x86/xen/p2m: hint at the last populated P2M entry") Cc: stable@vger.kernel.org Signed-off-by: Juergen Gross <jgross@suse.com> --- arch/x86/xen/p2m.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)