| Message ID | 1352774820-22804-9-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On 13.11.2012, at 03:46, David Gibson wrote: > Currently the ppcemb_tlb_t struct, used on a number of embedded ppc models > to represent a TLB entry contains a hwaddr. That works reasonably for now, > but is troublesome for saving the state, which we'll want to do in future. > hwaddr is a large enough type to contain a physical address for any > supported machine - and can thus, in theory at least, vary depending on > what machines are enabled other than the one we're actually using right > now (though in fact it doesn't for ppc). This makes it unsuitable for > describing in vmstate. > > This patch therefore changes ppcemb_tlb_t to use a fixed 64-bit integer > which we know is sufficient for all the machines which use this structure. hwaddr is always defined to 64bit by now. Alex > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > target-ppc/cpu.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h > index 5f1dc8b..742d4f8 100644 > --- a/target-ppc/cpu.h > +++ b/target-ppc/cpu.h > @@ -355,7 +355,7 @@ struct ppc6xx_tlb_t { > > typedef struct ppcemb_tlb_t ppcemb_tlb_t; > struct ppcemb_tlb_t { > - hwaddr RPN; > + uint64_t RPN; > target_ulong EPN; > target_ulong PID; > target_ulong size; > -- > 1.7.10.4 >
On Mon, Nov 19, 2012 at 05:26:45PM +0100, Alexander Graf wrote: > > On 13.11.2012, at 03:46, David Gibson wrote: > > > Currently the ppcemb_tlb_t struct, used on a number of embedded ppc models > > to represent a TLB entry contains a hwaddr. That works reasonably for now, > > but is troublesome for saving the state, which we'll want to do in future. > > hwaddr is a large enough type to contain a physical address for any > > supported machine - and can thus, in theory at least, vary depending on > > what machines are enabled other than the one we're actually using right > > now (though in fact it doesn't for ppc). This makes it unsuitable for > > describing in vmstate. > > > > This patch therefore changes ppcemb_tlb_t to use a fixed 64-bit integer > > which we know is sufficient for all the machines which use this structure. > > hwaddr is always defined to 64bit by now. I know, but there aren't state save helpers for hwaddr, and there are objections to creating them.
On 19.11.2012, at 23:48, David Gibson wrote: > On Mon, Nov 19, 2012 at 05:26:45PM +0100, Alexander Graf wrote: >> >> On 13.11.2012, at 03:46, David Gibson wrote: >> >>> Currently the ppcemb_tlb_t struct, used on a number of embedded ppc models >>> to represent a TLB entry contains a hwaddr. That works reasonably for now, >>> but is troublesome for saving the state, which we'll want to do in future. >>> hwaddr is a large enough type to contain a physical address for any >>> supported machine - and can thus, in theory at least, vary depending on >>> what machines are enabled other than the one we're actually using right >>> now (though in fact it doesn't for ppc). This makes it unsuitable for >>> describing in vmstate. >>> >>> This patch therefore changes ppcemb_tlb_t to use a fixed 64-bit integer >>> which we know is sufficient for all the machines which use this structure. >> >> hwaddr is always defined to 64bit by now. > > I know, but there aren't state save helpers for hwaddr, and there are > objections to creating them. Sure, but you can just use the 64bit save helpers now that hwaddr == uint64_t, no? The reason I'm reluctant is that this is not the only case where we have an hwaddr field in the CPUState struct. Either we convert all the hwaddr fields that we potentially want to savevm/loadvm or we don't convert any :). Alex
On 20 November 2012 09:29, Alexander Graf <agraf@suse.de> wrote: > On 19.11.2012, at 23:48, David Gibson wrote: >> On Mon, Nov 19, 2012 at 05:26:45PM +0100, Alexander Graf wrote: >>> On 13.11.2012, at 03:46, David Gibson wrote: >>>> This patch therefore changes ppcemb_tlb_t to use a fixed 64-bit integer >>>> which we know is sufficient for all the machines which use this structure. >>> >>> hwaddr is always defined to 64bit by now. >> >> I know, but there aren't state save helpers for hwaddr, and there are >> objections to creating them. (previous discussion on this point: https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg01456.html ) > Sure, but you can just use the 64bit save helpers now that hwaddr == uint64_t, no? That would be one approach. I'm a bit sceptical about putting hwaddr fields in CPU state, though -- it's suggestive that something's not modelled right. hwaddr is conceptually "big enough for the biggest bus in the system", and no single component should have internal state whose size depends on that. -- PMM
On 20.11.2012, at 10:53, Peter Maydell wrote: > On 20 November 2012 09:29, Alexander Graf <agraf@suse.de> wrote: >> On 19.11.2012, at 23:48, David Gibson wrote: >>> On Mon, Nov 19, 2012 at 05:26:45PM +0100, Alexander Graf wrote: >>>> On 13.11.2012, at 03:46, David Gibson wrote: >>>>> This patch therefore changes ppcemb_tlb_t to use a fixed 64-bit integer >>>>> which we know is sufficient for all the machines which use this structure. >>>> >>>> hwaddr is always defined to 64bit by now. >>> >>> I know, but there aren't state save helpers for hwaddr, and there are >>> objections to creating them. > > (previous discussion on this point: > https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg01456.html ) > >> Sure, but you can just use the 64bit save helpers now that hwaddr == uint64_t, no? > > That would be one approach. I'm a bit sceptical about putting hwaddr > fields in CPU state, though -- it's suggestive that something's > not modelled right. hwaddr is conceptually "big enough for the > biggest bus in the system", and no single component should have > internal state whose size depends on that. *shrug* I'm more than happy to get a patch that just converts all the hwaddr fields in CPUState to uint64_t. Alex
On Tue, Nov 20, 2012 at 10:55:50AM +0100, Alexander Graf wrote: > > On 20.11.2012, at 10:53, Peter Maydell wrote: > > > On 20 November 2012 09:29, Alexander Graf <agraf@suse.de> wrote: > >> On 19.11.2012, at 23:48, David Gibson wrote: > >>> On Mon, Nov 19, 2012 at 05:26:45PM +0100, Alexander Graf wrote: > >>>> On 13.11.2012, at 03:46, David Gibson wrote: > >>>>> This patch therefore changes ppcemb_tlb_t to use a fixed 64-bit integer > >>>>> which we know is sufficient for all the machines which use this structure. > >>>> > >>>> hwaddr is always defined to 64bit by now. > >>> > >>> I know, but there aren't state save helpers for hwaddr, and there are > >>> objections to creating them. > > > > (previous discussion on this point: > > https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg01456.html ) > > > >> Sure, but you can just use the 64bit save helpers now that hwaddr == uint64_t, no? > > > > That would be one approach. I'm a bit sceptical about putting hwaddr > > fields in CPU state, though -- it's suggestive that something's > > not modelled right. hwaddr is conceptually "big enough for the > > biggest bus in the system", and no single component should have > > internal state whose size depends on that. Right, that's the reason I was given for not adding VMSTATE helpers for hwaddr too. But more directly, as long as hwaddr is a different type from uint64_t, to me that at least admits the possibility that it could be changed again some day. And if we're using a uint64_t based VMSTATE helper on a type that could change, that could go badly wrong. Basically it's a subtle and ungreppable dependency on the fact that a hwaddr is actually a uint64_t, which seems like a bad idea. > *shrug* I'm more than happy to get a patch that just converts all > *the hwaddr fields in CPUState to uint64_t. So.. does that mean you'll apply this one or not?
On 21.11.2012, at 02:14, David Gibson wrote: > On Tue, Nov 20, 2012 at 10:55:50AM +0100, Alexander Graf wrote: >> >> On 20.11.2012, at 10:53, Peter Maydell wrote: >> >>> On 20 November 2012 09:29, Alexander Graf <agraf@suse.de> wrote: >>>> On 19.11.2012, at 23:48, David Gibson wrote: >>>>> On Mon, Nov 19, 2012 at 05:26:45PM +0100, Alexander Graf wrote: >>>>>> On 13.11.2012, at 03:46, David Gibson wrote: >>>>>>> This patch therefore changes ppcemb_tlb_t to use a fixed 64-bit integer >>>>>>> which we know is sufficient for all the machines which use this structure. >>>>>> >>>>>> hwaddr is always defined to 64bit by now. >>>>> >>>>> I know, but there aren't state save helpers for hwaddr, and there are >>>>> objections to creating them. >>> >>> (previous discussion on this point: >>> https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg01456.html ) >>> >>>> Sure, but you can just use the 64bit save helpers now that hwaddr == uint64_t, no? >>> >>> That would be one approach. I'm a bit sceptical about putting hwaddr >>> fields in CPU state, though -- it's suggestive that something's >>> not modelled right. hwaddr is conceptually "big enough for the >>> biggest bus in the system", and no single component should have >>> internal state whose size depends on that. > > Right, that's the reason I was given for not adding VMSTATE helpers > for hwaddr too. > > But more directly, as long as hwaddr is a different type from > uint64_t, to me that at least admits the possibility that it could be > changed again some day. And if we're using a uint64_t based VMSTATE > helper on a type that could change, that could go badly wrong. > Basically it's a subtle and ungreppable dependency on the fact that a > hwaddr is actually a uint64_t, which seems like a bad idea. > >> *shrug* I'm more than happy to get a patch that just converts all >> *the hwaddr fields in CPUState to uint64_t. > > So.. does that mean you'll apply this one or not? It means I'll wait for one that converts more than just this one field :). According to the above rationale, there shouldn't be any hwaddr fields in CPUState, right? Alex
On Wed, Nov 21, 2012 at 02:48:07AM +0100, Alexander Graf wrote: > > On 21.11.2012, at 02:14, David Gibson wrote: > > > On Tue, Nov 20, 2012 at 10:55:50AM +0100, Alexander Graf wrote: > >> > >> On 20.11.2012, at 10:53, Peter Maydell wrote: > >> > >>> On 20 November 2012 09:29, Alexander Graf <agraf@suse.de> wrote: > >>>> On 19.11.2012, at 23:48, David Gibson wrote: > >>>>> On Mon, Nov 19, 2012 at 05:26:45PM +0100, Alexander Graf wrote: > >>>>>> On 13.11.2012, at 03:46, David Gibson wrote: > >>>>>>> This patch therefore changes ppcemb_tlb_t to use a fixed 64-bit integer > >>>>>>> which we know is sufficient for all the machines which use this structure. > >>>>>> > >>>>>> hwaddr is always defined to 64bit by now. > >>>>> > >>>>> I know, but there aren't state save helpers for hwaddr, and there are > >>>>> objections to creating them. > >>> > >>> (previous discussion on this point: > >>> https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg01456.html ) > >>> > >>>> Sure, but you can just use the 64bit save helpers now that hwaddr == uint64_t, no? > >>> > >>> That would be one approach. I'm a bit sceptical about putting hwaddr > >>> fields in CPU state, though -- it's suggestive that something's > >>> not modelled right. hwaddr is conceptually "big enough for the > >>> biggest bus in the system", and no single component should have > >>> internal state whose size depends on that. > > > > Right, that's the reason I was given for not adding VMSTATE helpers > > for hwaddr too. > > > > But more directly, as long as hwaddr is a different type from > > uint64_t, to me that at least admits the possibility that it could be > > changed again some day. And if we're using a uint64_t based VMSTATE > > helper on a type that could change, that could go badly wrong. > > Basically it's a subtle and ungreppable dependency on the fact that a > > hwaddr is actually a uint64_t, which seems like a bad idea. > > > >> *shrug* I'm more than happy to get a patch that just converts all > >> *the hwaddr fields in CPUState to uint64_t. > > > > So.. does that mean you'll apply this one or not? > > It means I'll wait for one that converts more than just this one > field :). According to the above rationale, there shouldn't be any > hwaddr fields in CPUState, right? Grah. Why is that qemu people always seem to insist on not fixing something that needs fixing unless everything else that needs fixing is done at the same time. In any case, I don't think that's strictly correct. The point is that fields which represent architected CPU state should never be hwaddr, but it's at least possible that it could be appropriate for some anciliary data.
On 21.11.2012, at 02:56, David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Nov 21, 2012 at 02:48:07AM +0100, Alexander Graf wrote: >> >> On 21.11.2012, at 02:14, David Gibson wrote: >> >>> On Tue, Nov 20, 2012 at 10:55:50AM +0100, Alexander Graf wrote: >>>> >>>> On 20.11.2012, at 10:53, Peter Maydell wrote: >>>> >>>>> On 20 November 2012 09:29, Alexander Graf <agraf@suse.de> wrote: >>>>>> On 19.11.2012, at 23:48, David Gibson wrote: >>>>>>> On Mon, Nov 19, 2012 at 05:26:45PM +0100, Alexander Graf wrote: >>>>>>>> On 13.11.2012, at 03:46, David Gibson wrote: >>>>>>>>> This patch therefore changes ppcemb_tlb_t to use a fixed 64-bit integer >>>>>>>>> which we know is sufficient for all the machines which use this structure. >>>>>>>> >>>>>>>> hwaddr is always defined to 64bit by now. >>>>>>> >>>>>>> I know, but there aren't state save helpers for hwaddr, and there are >>>>>>> objections to creating them. >>>>> >>>>> (previous discussion on this point: >>>>> https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg01456.html ) >>>>> >>>>>> Sure, but you can just use the 64bit save helpers now that hwaddr == uint64_t, no? >>>>> >>>>> That would be one approach. I'm a bit sceptical about putting hwaddr >>>>> fields in CPU state, though -- it's suggestive that something's >>>>> not modelled right. hwaddr is conceptually "big enough for the >>>>> biggest bus in the system", and no single component should have >>>>> internal state whose size depends on that. >>> >>> Right, that's the reason I was given for not adding VMSTATE helpers >>> for hwaddr too. >>> >>> But more directly, as long as hwaddr is a different type from >>> uint64_t, to me that at least admits the possibility that it could be >>> changed again some day. And if we're using a uint64_t based VMSTATE >>> helper on a type that could change, that could go badly wrong. >>> Basically it's a subtle and ungreppable dependency on the fact that a >>> hwaddr is actually a uint64_t, which seems like a bad idea. >>> >>>> *shrug* I'm more than happy to get a patch that just converts all >>>> *the hwaddr fields in CPUState to uint64_t. >>> >>> So.. does that mean you'll apply this one or not? >> >> It means I'll wait for one that converts more than just this one >> field :). According to the above rationale, there shouldn't be any >> hwaddr fields in CPUState, right? > > Grah. Why is that qemu people always seem to insist on not fixing > something that needs fixing unless everything else that needs fixing > is done at the same time. Because that's the only way to keep the code consistent. > > In any case, I don't think that's strictly correct. The point is that > fields which represent architected CPU state should never be hwaddr, > but it's at least possible that it could be appropriate for some > anciliary data. Yup. Please double-check all fields. I wouldn't be surprised if there are more fields that you would want to save/restore that are hwaddr. Alex > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h index 5f1dc8b..742d4f8 100644 --- a/target-ppc/cpu.h +++ b/target-ppc/cpu.h @@ -355,7 +355,7 @@ struct ppc6xx_tlb_t { typedef struct ppcemb_tlb_t ppcemb_tlb_t; struct ppcemb_tlb_t { - hwaddr RPN; + uint64_t RPN; target_ulong EPN; target_ulong PID; target_ulong size;
Currently the ppcemb_tlb_t struct, used on a number of embedded ppc models to represent a TLB entry contains a hwaddr. That works reasonably for now, but is troublesome for saving the state, which we'll want to do in future. hwaddr is a large enough type to contain a physical address for any supported machine - and can thus, in theory at least, vary depending on what machines are enabled other than the one we're actually using right now (though in fact it doesn't for ppc). This makes it unsuitable for describing in vmstate. This patch therefore changes ppcemb_tlb_t to use a fixed 64-bit integer which we know is sufficient for all the machines which use this structure. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- target-ppc/cpu.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)