Patchwork [4/7] savevm: Add VMSTATE_ helpers for target_phys_addr_t

login
register
mail settings
Submitter David Gibson
Date Oct. 9, 2012, 4:53 a.m.
Message ID <1349758412-5559-5-git-send-email-david@gibson.dropbear.id.au>
Download mbox | patch
Permalink /patch/190217/
State New
Headers show

Comments

David Gibson - Oct. 9, 2012, 4:53 a.m.
The savevm code contains VMSTATE_ helpers for a number of commonly used
types, but not for target_phys_addr_t.  This patch fixes that deficiency
implementing VMSTATE_TPA helpers in terms of VMSTATE_UINT32 or
VMSTATE_UINT64 helpers as appropriate.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 targphys.h |   11 +++++++++++
 1 file changed, 11 insertions(+)
Peter Maydell - Oct. 9, 2012, 8:03 a.m.
On 9 October 2012 05:53, David Gibson <david@gibson.dropbear.id.au> wrote:
> The savevm code contains VMSTATE_ helpers for a number of commonly used
> types, but not for target_phys_addr_t.  This patch fixes that deficiency
> implementing VMSTATE_TPA helpers in terms of VMSTATE_UINT32 or
> VMSTATE_UINT64 helpers as appropriate.

(This comment is out of date...)

Traditionally we have deliberately not had any vmstate helpers
for target_phys_addr_t because the meaning of that type is "a
type wide enough to hold the widest address in use in the system".
vmstate fields, on the other hand, are for the state of a specific
device model, and a particular device's registers are always a
fixed width. Even if a pci card model is plugged into a system
with 64 bit PCI addresses, if the chip on the card itself only
has 32 bit wide registers controlling DMA those registers need
to be uint32_t. So if you think you need to put target_phys_addr_t
into a vmstate the chances are you don't. I think this logic
should still hold even now we have made target_phys_addr_t
64 bits for every platform.

What was the problem this patch is trying to fix?

-- PMM
David Gibson - Oct. 9, 2012, 12:53 p.m.
On Tue, Oct 09, 2012 at 09:03:21AM +0100, Peter Maydell wrote:
> On 9 October 2012 05:53, David Gibson <david@gibson.dropbear.id.au> wrote:
> > The savevm code contains VMSTATE_ helpers for a number of commonly used
> > types, but not for target_phys_addr_t.  This patch fixes that deficiency
> > implementing VMSTATE_TPA helpers in terms of VMSTATE_UINT32 or
> > VMSTATE_UINT64 helpers as appropriate.
> 
> (This comment is out of date...)

Ah, yes.

> Traditionally we have deliberately not had any vmstate helpers
> for target_phys_addr_t because the meaning of that type is "a
> type wide enough to hold the widest address in use in the system".
> vmstate fields, on the other hand, are for the state of a specific
> device model, and a particular device's registers are always a
> fixed width. Even if a pci card model is plugged into a system
> with 64 bit PCI addresses, if the chip on the card itself only
> has 32 bit wide registers controlling DMA those registers need
> to be uint32_t. So if you think you need to put target_phys_addr_t
> into a vmstate the chances are you don't. I think this logic
> should still hold even now we have made target_phys_addr_t
> 64 bits for every platform.
> 
> What was the problem this patch is trying to fix?

Well, the place I've used this (in patches yet to be posted) is saving
the state of the pseries machine itself.  Specifically, I use
VMSTATE_TPA_EQUAL to sanity check that the restored machine has the
same ram size as the saved machine.  In this case the variable is qemu
internal information and represents a ram size, so I think
target_phys_addr_t is the correct type here.
Peter Maydell - Oct. 9, 2012, 4:24 p.m.
On 9 October 2012 13:53, David Gibson <dwg@au1.ibm.com> wrote:
> Well, the place I've used this (in patches yet to be posted) is saving
> the state of the pseries machine itself.  Specifically, I use
> VMSTATE_TPA_EQUAL to sanity check that the restored machine has the
> same ram size as the saved machine.

This doesn't sound like the sort of check that should be in
a specific machine model -- you should be able to rely on
common qemu code to handle this. (If you can't then we need
a check in the common code...)

-- PMM
David Gibson - Oct. 10, 2012, 12:17 a.m.
On Tue, Oct 09, 2012 at 05:24:29PM +0100, Peter Maydell wrote:
> On 9 October 2012 13:53, David Gibson <dwg@au1.ibm.com> wrote:
> > Well, the place I've used this (in patches yet to be posted) is saving
> > the state of the pseries machine itself.  Specifically, I use
> > VMSTATE_TPA_EQUAL to sanity check that the restored machine has the
> > same ram size as the saved machine.
> 
> This doesn't sound like the sort of check that should be in
> a specific machine model -- you should be able to rely on
> common qemu code to handle this. (If you can't then we need
> a check in the common code...)

Hm, true.  Very well, I'll drop it, and the VMSTATE_TPA patch as
well.
David Gibson - Oct. 11, 2012, 1:57 a.m.
On Wed, Oct 10, 2012 at 11:17:31AM +1100, David Gibson wrote:
> On Tue, Oct 09, 2012 at 05:24:29PM +0100, Peter Maydell wrote:
> > On 9 October 2012 13:53, David Gibson <dwg@au1.ibm.com> wrote:
> > > Well, the place I've used this (in patches yet to be posted) is saving
> > > the state of the pseries machine itself.  Specifically, I use
> > > VMSTATE_TPA_EQUAL to sanity check that the restored machine has the
> > > same ram size as the saved machine.
> > 
> > This doesn't sound like the sort of check that should be in
> > a specific machine model -- you should be able to rely on
> > common qemu code to handle this. (If you can't then we need
> > a check in the common code...)
> 
> Hm, true.  Very well, I'll drop it, and the VMSTATE_TPA patch as
> well.

Actually, turns out I had another use of these helpers.  That was to
store the real page address from the ppcmeb_tlb_t structure.  That
structure is used to represent TLB entries on a number of different
embedded chips, which don't all have the same physical bus width.  So
target_phys_addr_t does seem like the correct type there.

Obviously I could change the type to a fixed uint64_t, but I'm not
sure if that's a better idea than bringing in the VMSTATE_TPA
helpers.  Advice?
Peter Maydell - Oct. 11, 2012, 3:07 p.m.
On 11 October 2012 02:57, David Gibson <dwg@au1.ibm.com> wrote:
> Actually, turns out I had another use of these helpers.  That was to
> store the real page address from the ppcmeb_tlb_t structure.  That
> structure is used to represent TLB entries on a number of different
> embedded chips, which don't all have the same physical bus width.  So
> target_phys_addr_t does seem like the correct type there.

> Obviously I could change the type to a fixed uint64_t, but I'm not
> sure if that's a better idea than bringing in the VMSTATE_TPA
> helpers.  Advice?

PPC has had 64 bit target_phys_addr_t even before the recent change.
Either these various CPUs all actually have physical hardware
TLB configs which hold the full 64 bits (use uint64_t) or they
have physical configs which vary between them (in which case
you're presumably mismodelling some of them) and you need to
use different types or alternatively always use uint64_t and
do masking on writes to the fields or something.

If you're happy to continue to bake the assumption into your
code that target_phys_addr_t is 64 bits, you can just use
VMSTATE_UINT64 which (I think) should work OK when t_p_a_t
really is 64 bits, and will give a helpful compile failure
if the assumption is ever untrue...

Basically I think that to the extent that we contemplate
t_p_a_t as distinct from "64 bit uint" it's a bad idea to put
it into vmstate because it means potential migration
compatibility breaks if it changes.

-- PMM
David Gibson - Oct. 12, 2012, 12:35 a.m.
On Thu, Oct 11, 2012 at 04:07:24PM +0100, Peter Maydell wrote:
> On 11 October 2012 02:57, David Gibson <dwg@au1.ibm.com> wrote:
> > Actually, turns out I had another use of these helpers.  That was to
> > store the real page address from the ppcmeb_tlb_t structure.  That
> > structure is used to represent TLB entries on a number of different
> > embedded chips, which don't all have the same physical bus width.  So
> > target_phys_addr_t does seem like the correct type there.
> 
> > Obviously I could change the type to a fixed uint64_t, but I'm not
> > sure if that's a better idea than bringing in the VMSTATE_TPA
> > helpers.  Advice?
> 
> PPC has had 64 bit target_phys_addr_t even before the recent change.
> Either these various CPUs all actually have physical hardware
> TLB configs which hold the full 64 bits (use uint64_t) or they
> have physical configs which vary between them (in which case
> you're presumably mismodelling some of them) and you need to
> use different types or alternatively always use uint64_t and
> do masking on writes to the fields or something.

Masking will be done in the handling of the instructions that access
the TLB.  They do have different physical configs, but they also have
different access methods, the data type as it is is sufficient for
both of them.

> If you're happy to continue to bake the assumption into your
> code that target_phys_addr_t is 64 bits, you can just use
> VMSTATE_UINT64 which (I think) should work OK when t_p_a_t
> really is 64 bits, and will give a helpful compile failure
> if the assumption is ever untrue...
> 
> Basically I think that to the extent that we contemplate
> t_p_a_t as distinct from "64 bit uint" it's a bad idea to put
> it into vmstate because it means potential migration
> compatibility breaks if it changes.

Hm, yeah, that I'll grant you.  Ok, I'll remove it.

Patch

diff --git a/targphys.h b/targphys.h
index 08cade9..a1c20b4 100644
--- a/targphys.h
+++ b/targphys.h
@@ -17,4 +17,15 @@  typedef uint64_t target_phys_addr_t;
 #define TARGET_PRIxPHYS PRIx64
 #define TARGET_PRIXPHYS PRIX64
 
+#define VMSTATE_TPA_V(_f, _s, _v) \
+    VMSTATE_UINT64_V(_f, _s, _v)
+
+#define VMSTATE_TPA_EQUAL_V(_f, _s, _v) \
+    VMSTATE_UINT64_EQUAL_V(_f, _s, _v)
+
+#define VMSTATE_TPA(_f, _s) \
+    VMSTATE_TPA_V(_f, _s, 0)
+#define VMSTATE_TPA_EQUAL(_f, _s) \
+    VMSTATE_TPA_EQUAL_V(_f, _s, 0)
+
 #endif