| Message ID | 1349758412-5559-5-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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.
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?
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
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.
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
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(+)