libxc: remove stale error check for domain size in xc_sr_save_x86_hvm.c

PATCH In review Juergen Gross 5 participants 9 messages Sep 26, 2017 – Oct 27, 2017

Share & tools

Juergen Gross

author

2017-09-26 12:03 UTC #0 lore raw

PATCH v2 1/1 libxc: remove stale error check for domain size in xc_sr_save_x86_hvm.c

Long ago domains to be saved were limited to 1TB size due to the migration stream v1 limitations which used a 32 bit value for the PFN and the frame type (4 bits) leaving only 28 bits for the PFN.

Migration stream V2 uses a 64 bit value for this purpose, so there is no need to refuse saving (or migrating) domains larger than 1 TB.

For 32 bit toolstacks there is still a size limit, as domains larger than about 1TB will lead to an exhausted virtual address space of the saving process. So keep the test for 32 bit, but don't base it on the page type macros. As a migration could lead to the situation where a 32 bit toolstack would have to handle such a large domain (in case the sending side is 64 bit) the same test should be added for restoring a domain.

Signed-off-by Juergen Gross <[email protected]>

---
I know we have code freeze. Nevertheless I'd like to request an exception and get this into 4.10, as the impact of the patch is very local and of low risk.

V2:
- keep test for 32 bit toolstack (Andrew Cooper)
- test against size value instead of not page type mask
- add related test on the receive side, too

---
 tools/libxc/xc_sr_restore_x86_hvm.c | 9 +++++++++
 tools/libxc/xc_sr_save_x86_hvm.c    | 5 ++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

tools/libxc/xc_sr_restore_x86_hvm.c +9 -0

index 1dca85354a..227c48553e 100644
@@ -148,6 +148,15 @@ static int x86_hvm_setup(struct xc_sr_context *ctx)
               ctx->restore.guest_page_size);
         return -1;
     }
+#ifdef __i386__
+    /* Very large domains (> 1TB) will exhaust virtual address space. */
+    if ( ctx->restore.p2m_size > 0x0fffffff )
+    {
+        errno = E2BIG;
+        PERROR("Cannot restore this big a guest");
+        return -1;
+    }
+#endif
 
     return 0;
 }

tools/libxc/xc_sr_save_x86_hvm.c +4 -1

index e17bb59146..97a8c49807 100644
@@ -148,12 +148,15 @@ static int x86_hvm_setup(struct xc_sr_context *ctx)
         PERROR("Unable to obtain the guest p2m size");
         return -1;
     }
-    if ( nr_pfns > ~XEN_DOMCTL_PFINFO_LTAB_MASK )
+#ifdef __i386__
+    /* Very large domains (> 1TB) will exhaust virtual address space. */
+    if ( nr_pfns > 0x0fffffff )
     {
         errno = E2BIG;
         PERROR("Cannot save this big a guest");
         return -1;
     }
+#endif
 
     ctx->save.p2m_size = nr_pfns;

Signature · 7 lines

2.12.3


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Wei Liu

2017-09-27 14:37 UTC #1 lore raw

Quoted 15 lines · Juergen Gross

Long ago domains to be saved were limited to 1TB size due to the migration stream v1 limitations which used a 32 bit value for the PFN and the frame type (4 bits) leaving only 28 bits for the PFN.
Migration stream V2 uses a 64 bit value for this purpose, so there is no need to refuse saving (or migrating) domains larger than 1 TB.
For 32 bit toolstacks there is still a size limit, as domains larger than about 1TB will lead to an exhausted virtual address space of the saving process. So keep the test for 32 bit, but don't base it on the page type macros. As a migration could lead to the situation where a 32 bit toolstack would have to handle such a large domain (in case the sending side is 64 bit) the same test should be added for restoring a domain.

Quoted 1 line

Signed-off-by Juergen Gross <[email protected]>

I will leave this to Andrew.

I don't really have an opinion here.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Julien Grall

2017-10-06 13:31 UTC #2 lore raw

Hi,

Quoted 16 lines · Wei Liu

Quoted 15 lines · Juergen Gross
Long ago domains to be saved were limited to 1TB size due to the migration stream v1 limitations which used a 32 bit value for the PFN and the frame type (4 bits) leaving only 28 bits for the PFN.
Migration stream V2 uses a 64 bit value for this purpose, so there is no need to refuse saving (or migrating) domains larger than 1 TB.
For 32 bit toolstacks there is still a size limit, as domains larger than about 1TB will lead to an exhausted virtual address space of the saving process. So keep the test for 32 bit, but don't base it on the page type macros. As a migration could lead to the situation where a 32 bit toolstack would have to handle such a large domain (in case the sending side is 64 bit) the same test should be added for restoring a domain.

Quoted 5 lines

Quoted 1 line
Signed-off-by Juergen Gross <[email protected]>
I will leave this to Andrew.
I don't really have an opinion here.

I will wait Andrew feedback before giving a release ack on this patch.

Cheers,

Signature · 6 lines

Julien Grall

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Juergen Gross

author

2017-10-23 10:20 UTC #3 lore raw

Quoted 19 lines · Julien Grall

Hi,
Quoted 16 lines · Wei Liu
Quoted 15 lines · Juergen Gross
Long ago domains to be saved were limited to 1TB size due to the migration stream v1 limitations which used a 32 bit value for the PFN and the frame type (4 bits) leaving only 28 bits for the PFN.
Migration stream V2 uses a 64 bit value for this purpose, so there is no need to refuse saving (or migrating) domains larger than 1 TB.
For 32 bit toolstacks there is still a size limit, as domains larger than about 1TB will lead to an exhausted virtual address space of the saving process. So keep the test for 32 bit, but don't base it on the page type macros. As a migration could lead to the situation where a 32 bit toolstack would have to handle such a large domain (in case the sending side is 64 bit) the same test should be added for restoring a domain.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
Signed-off-by Juergen Gross <[email protected]>
I will leave this to Andrew.
I don't really have an opinion here.
I will wait Andrew feedback before giving a release ack on this patch.

Andrew?

Juergen

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Andrew Cooper

2017-10-24 10:09 UTC #4 lore raw

Quoted 20 lines · Juergen Gross

Quoted 19 lines · Julien Grall
Hi,
Quoted 16 lines · Wei Liu
Quoted 15 lines · Juergen Gross
Long ago domains to be saved were limited to 1TB size due to the migration stream v1 limitations which used a 32 bit value for the PFN and the frame type (4 bits) leaving only 28 bits for the PFN.
Migration stream V2 uses a 64 bit value for this purpose, so there is no need to refuse saving (or migrating) domains larger than 1 TB.
For 32 bit toolstacks there is still a size limit, as domains larger than about 1TB will lead to an exhausted virtual address space of the saving process. So keep the test for 32 bit, but don't base it on the page type macros. As a migration could lead to the situation where a 32 bit toolstack would have to handle such a large domain (in case the sending side is 64 bit) the same test should be added for restoring a domain.

Quoted 7 lines

Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Juergen Gross <[email protected]>
I will leave this to Andrew.
I don't really have an opinion here.
I will wait Andrew feedback before giving a release ack on this patch.
Andrew?

Sorry - this completely fell off my radar.

This is probably fine overall.

One area which is now more important for vendors to take care over is preventing migration of VMs from swapping dom0 to death. There are a number of large structures allocated (only O(n) with the size of the VM), and this will get worse as/when steps are taken to address the ballooning issues. The best way is probably to limit the number of concurrent migrations which can be performed.

~Andrew

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Juergen Gross

author

2017-10-26 07:31 UTC #5 lore raw

Quoted 24 lines · Andrew Cooper

Quoted 23 lines · Juergen Gross
Quoted 22 lines · Julien Grall
Hi,
Quoted 19 lines · Wei Liu
Quoted 15 lines · Juergen Gross
Long ago domains to be saved were limited to 1TB size due to the migration stream v1 limitations which used a 32 bit value for the PFN and the frame type (4 bits) leaving only 28 bits for the PFN.
Migration stream V2 uses a 64 bit value for this purpose, so there is no need to refuse saving (or migrating) domains larger than 1 TB.
For 32 bit toolstacks there is still a size limit, as domains larger than about 1TB will lead to an exhausted virtual address space of the saving process. So keep the test for 32 bit, but don't base it on the page type macros. As a migration could lead to the situation where a 32 bit toolstack would have to handle such a large domain (in case the sending side is 64 bit) the same test should be added for restoring a domain.
Quoted 1 line
Signed-off-by Juergen Gross <[email protected]>
I will leave this to Andrew.

Quoted 8 lines

Quoted 4 lines
Quoted 3 lines
Quoted 1 line
I don't really have an opinion here.
I will wait Andrew feedback before giving a release ack on this patch.
Andrew?
Sorry - this completely fell off my radar.
This is probably fine overall.

Julien, are you fine with the patch now?

Juergen

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Julien Grall

2017-10-26 08:15 UTC #6 lore raw

Hi Juergen,

Quoted 27 lines · Juergen Gross

Quoted 26 lines · Andrew Cooper
Quoted 25 lines · Juergen Gross
Quoted 24 lines · Julien Grall
Hi,
Quoted 20 lines · Wei Liu
Quoted 15 lines · Juergen Gross
Long ago domains to be saved were limited to 1TB size due to the migration stream v1 limitations which used a 32 bit value for the PFN and the frame type (4 bits) leaving only 28 bits for the PFN.
Migration stream V2 uses a 64 bit value for this purpose, so there is no need to refuse saving (or migrating) domains larger than 1 TB.
For 32 bit toolstacks there is still a size limit, as domains larger than about 1TB will lead to an exhausted virtual address space of the saving process. So keep the test for 32 bit, but don't base it on the page type macros. As a migration could lead to the situation where a 32 bit toolstack would have to handle such a large domain (in case the sending side is 64 bit) the same test should be added for restoring a domain.
Quoted 1 line
Signed-off-by Juergen Gross <[email protected]>
I will leave this to Andrew.
I don't really have an opinion here.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Quoted 1 line
I will wait Andrew feedback before giving a release ack on this patch.
Andrew?
Sorry - this completely fell off my radar.
This is probably fine overall.
Julien, are you fine with the patch now?

I don't see any formal ack on this patch. Can we get one?

Cheers,

Signature · 6 lines

Julien Grall

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Wei Liu

Acked-by

2017-10-26 11:03 UTC #7 lore raw

Quoted 33 lines · Julien Grall

Hi Juergen,
Quoted 30 lines · Juergen Gross
Quoted 29 lines · Andrew Cooper
Quoted 27 lines · Juergen Gross
Quoted 19 lines · Julien Grall
Hi,
Quoted 16 lines · Wei Liu
Quoted 15 lines · Juergen Gross
Long ago domains to be saved were limited to 1TB size due to the migration stream v1 limitations which used a 32 bit value for the PFN and the frame type (4 bits) leaving only 28 bits for the PFN.
Migration stream V2 uses a 64 bit value for this purpose, so there is no need to refuse saving (or migrating) domains larger than 1 TB.
For 32 bit toolstacks there is still a size limit, as domains larger than about 1TB will lead to an exhausted virtual address space of the saving process. So keep the test for 32 bit, but don't base it on the page type macros. As a migration could lead to the situation where a 32 bit toolstack would have to handle such a large domain (in case the sending side is 64 bit) the same test should be added for restoring a domain.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Juergen Gross <[email protected]>
I will leave this to Andrew.
I don't really have an opinion here.
I will wait Andrew feedback before giving a release ack on this patch.
Andrew?

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Sorry - this completely fell off my radar.
This is probably fine overall.
Julien, are you fine with the patch now?
I don't see any formal ack on this patch. Can we get one?

Acked-by Wei Liu <[email protected]>

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Julien Grall

2017-10-27 10:54 UTC #8 lore raw

Hi,

Quoted 36 lines · Wei Liu

Quoted 35 lines · Julien Grall
Hi Juergen,
Quoted 32 lines · Juergen Gross
Quoted 31 lines · Andrew Cooper
Quoted 20 lines · Juergen Gross
Quoted 19 lines · Julien Grall
Hi,
Quoted 16 lines · Wei Liu
Quoted 15 lines · Juergen Gross
Long ago domains to be saved were limited to 1TB size due to the migration stream v1 limitations which used a 32 bit value for the PFN and the frame type (4 bits) leaving only 28 bits for the PFN.
Migration stream V2 uses a 64 bit value for this purpose, so there is no need to refuse saving (or migrating) domains larger than 1 TB.
For 32 bit toolstacks there is still a size limit, as domains larger than about 1TB will lead to an exhausted virtual address space of the saving process. So keep the test for 32 bit, but don't base it on the page type macros. As a migration could lead to the situation where a 32 bit toolstack would have to handle such a large domain (in case the sending side is 64 bit) the same test should be added for restoring a domain.
Quoted 7 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Juergen Gross <[email protected]>
I will leave this to Andrew.
I don't really have an opinion here.
I will wait Andrew feedback before giving a release ack on this patch.
Andrew?
Sorry - this completely fell off my radar.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Quoted 1 line
This is probably fine overall.
Julien, are you fine with the patch now?
I don't see any formal ack on this patch. Can we get one?
Acked-by Wei Liu <[email protected]>

Release-acked-by Julien Grall <[email protected]>

Cheers,

Signature · 6 lines

Julien Grall

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel