Re: [RFC PATCH v1 07/25] hw/xen: Implement core serialize/deserialize methods for xenstore_impl
On 07/03/2023 16:59, Paul Durrant wrote: On 07/03/2023 16:52, David Woodhouse wrote: On Tue, 2023-03-07 at 16:39 +, Paul Durrant wrote: On 07/03/2023 16:33, David Woodhouse wrote: On Thu, 2023-03-02 at 15:34 +, David Woodhouse wrote: From: David Woodhouse In fact I think we want to only serialize the contents of the domain's path in /local/domain/${domid} and leave the rest to be recreated? Will defer to Paul for that. Signed-off-by: David Woodhouse Paul, your Reviewed-by: on this one is conspicuous in its absence. I mentioned migration in the cover letter — this much is working fine, but it's the PV back ends that don't yet work. I'd quite like to merge the basic serialization/deserialization of XenStore itself, with the unit tests. The patch is basically ok, I just think the serialization should be limited to the guest nodes... filtering out those not owned by xen_domid would probably work for that. Yeah, so let's just do this (as part of this patch #7) for now: --- a/hw/i386/kvm/xen_xenstore.c +++ b/hw/i386/kvm/xen_xenstore.c @@ -235,6 +235,7 @@ static int xen_xenstore_post_load(void *opaque, int ver) static const VMStateDescription xen_xenstore_vmstate = { .name = "xen_xenstore", + .unmigratable = 1, /* The PV back ends don't migrate yet */ .version_id = 1, .minimum_version_id = 1, .needed = xen_xenstore_is_needed, It means we can't migrate guests even if they're only using fully emulated devices... but I think that's a reasonable limitation until we implement it fully. Ok. With that added... Revieweed-by: Paul Durrant Typoed, sorry... Reviewed-by: Paul Durrant
Re: [RFC PATCH v1 07/25] hw/xen: Implement core serialize/deserialize methods for xenstore_impl
On 07/03/2023 16:52, David Woodhouse wrote: On Tue, 2023-03-07 at 16:39 +, Paul Durrant wrote: On 07/03/2023 16:33, David Woodhouse wrote: On Thu, 2023-03-02 at 15:34 +, David Woodhouse wrote: From: David Woodhouse In fact I think we want to only serialize the contents of the domain's path in /local/domain/${domid} and leave the rest to be recreated? Will defer to Paul for that. Signed-off-by: David Woodhouse Paul, your Reviewed-by: on this one is conspicuous in its absence. I mentioned migration in the cover letter — this much is working fine, but it's the PV back ends that don't yet work. I'd quite like to merge the basic serialization/deserialization of XenStore itself, with the unit tests. The patch is basically ok, I just think the serialization should be limited to the guest nodes... filtering out those not owned by xen_domid would probably work for that. Yeah, so let's just do this (as part of this patch #7) for now: --- a/hw/i386/kvm/xen_xenstore.c +++ b/hw/i386/kvm/xen_xenstore.c @@ -235,6 +235,7 @@ static int xen_xenstore_post_load(void *opaque, int ver) static const VMStateDescription xen_xenstore_vmstate = { .name = "xen_xenstore", +.unmigratable = 1, /* The PV back ends don't migrate yet */ .version_id = 1, .minimum_version_id = 1, .needed = xen_xenstore_is_needed, It means we can't migrate guests even if they're only using fully emulated devices... but I think that's a reasonable limitation until we implement it fully. Ok. With that added... Revieweed-by: Paul Durrant
Re: [RFC PATCH v1 07/25] hw/xen: Implement core serialize/deserialize methods for xenstore_impl
On Tue, 2023-03-07 at 16:39 +, Paul Durrant wrote: > On 07/03/2023 16:33, David Woodhouse wrote: > > On Thu, 2023-03-02 at 15:34 +, David Woodhouse wrote: > > > From: David Woodhouse > > > > > > In fact I think we want to only serialize the contents of the domain's > > > path in /local/domain/${domid} and leave the rest to be recreated? Will > > > defer to Paul for that. > > > > > > Signed-off-by: David Woodhouse > > > > Paul, your Reviewed-by: on this one is conspicuous in its absence. I > > mentioned migration in the cover letter — this much is working fine, > > but it's the PV back ends that don't yet work. > > > > I'd quite like to merge the basic serialization/deserialization of > > XenStore itself, with the unit tests. > > The patch is basically ok, I just think the serialization should be > limited to the guest nodes... filtering out those not owned by xen_domid > would probably work for that. Yeah, so let's just do this (as part of this patch #7) for now: --- a/hw/i386/kvm/xen_xenstore.c +++ b/hw/i386/kvm/xen_xenstore.c @@ -235,6 +235,7 @@ static int xen_xenstore_post_load(void *opaque, int ver) static const VMStateDescription xen_xenstore_vmstate = { .name = "xen_xenstore", +.unmigratable = 1, /* The PV back ends don't migrate yet */ .version_id = 1, .minimum_version_id = 1, .needed = xen_xenstore_is_needed, It means we can't migrate guests even if they're only using fully emulated devices... but I think that's a reasonable limitation until we implement it fully. smime.p7s Description: S/MIME cryptographic signature
Re: [RFC PATCH v1 07/25] hw/xen: Implement core serialize/deserialize methods for xenstore_impl
On 07/03/2023 16:39, Paul Durrant wrote: On 07/03/2023 16:33, David Woodhouse wrote: On Thu, 2023-03-02 at 15:34 +, David Woodhouse wrote: From: David Woodhouse In fact I think we want to only serialize the contents of the domain's path in /local/domain/${domid} and leave the rest to be recreated? Will defer to Paul for that. Signed-off-by: David Woodhouse Paul, your Reviewed-by: on this one is conspicuous in its absence. I mentioned migration in the cover letter — this much is working fine, but it's the PV back ends that don't yet work. I'd quite like to merge the basic serialization/deserialization of XenStore itself, with the unit tests. The patch is basically ok, I just think the serialization should be limited to the guest nodes... filtering out those not owned by xen_domid would probably work for that. Perhaps we can also set TYPE_XEN_DEVICE or TYPE_XEN_BLOCK_DEVICE to be unmigratable? Ideally I think we want TYPE_XEN_DEVICE to be unmigratable by default *unless* the specific device class (including net and other as we port them from XenLegacyDevice) says otherwise. Yes, that sounds like a good idea. Is there a way to do that? Not something I've looked into. I'll go look now. Maybe calling migrate_add_blocker() in the realize method is the right way to achieve this? Paul
Re: [RFC PATCH v1 07/25] hw/xen: Implement core serialize/deserialize methods for xenstore_impl
On 07/03/2023 16:33, David Woodhouse wrote: On Thu, 2023-03-02 at 15:34 +, David Woodhouse wrote: From: David Woodhouse In fact I think we want to only serialize the contents of the domain's path in /local/domain/${domid} and leave the rest to be recreated? Will defer to Paul for that. Signed-off-by: David Woodhouse Paul, your Reviewed-by: on this one is conspicuous in its absence. I mentioned migration in the cover letter — this much is working fine, but it's the PV back ends that don't yet work. I'd quite like to merge the basic serialization/deserialization of XenStore itself, with the unit tests. The patch is basically ok, I just think the serialization should be limited to the guest nodes... filtering out those not owned by xen_domid would probably work for that. Perhaps we can also set TYPE_XEN_DEVICE or TYPE_XEN_BLOCK_DEVICE to be unmigratable? Ideally I think we want TYPE_XEN_DEVICE to be unmigratable by default *unless* the specific device class (including net and other as we port them from XenLegacyDevice) says otherwise. Yes, that sounds like a good idea. Is there a way to do that? Not something I've looked into. I'll go look now. Paul
Re: [RFC PATCH v1 07/25] hw/xen: Implement core serialize/deserialize methods for xenstore_impl
On Thu, 2023-03-02 at 15:34 +, David Woodhouse wrote: > From: David Woodhouse > > In fact I think we want to only serialize the contents of the domain's > path in /local/domain/${domid} and leave the rest to be recreated? Will > defer to Paul for that. > > Signed-off-by: David Woodhouse Paul, your Reviewed-by: on this one is conspicuous in its absence. I mentioned migration in the cover letter — this much is working fine, but it's the PV back ends that don't yet work. I'd quite like to merge the basic serialization/deserialization of XenStore itself, with the unit tests. Perhaps we can also set TYPE_XEN_DEVICE or TYPE_XEN_BLOCK_DEVICE to be unmigratable? Ideally I think we want TYPE_XEN_DEVICE to be unmigratable by default *unless* the specific device class (including net and other as we port them from XenLegacyDevice) says otherwise. Is there a way to do that? smime.p7s Description: S/MIME cryptographic signature
Re: [RFC PATCH v1 07/25] hw/xen: Implement core serialize/deserialize methods for xenstore_impl
On 02/03/2023 15:34, David Woodhouse wrote: From: David Woodhouse In fact I think we want to only serialize the contents of the domain's path in /local/domain/${domid} and leave the rest to be recreated? Will defer to Paul for that. Yes. Assuming backends have properly implemented save/restore code then they should be able to re-create their xenstore content. Also, it's generally convenient for them to start back in 'InitWait' state the drive their state machine back to 'Connected' so that grant refs can be mapped, event channels bound etc. along the way. Signed-off-by: David Woodhouse --- hw/i386/kvm/xen_xenstore.c | 25 +- hw/i386/kvm/xenstore_impl.c | 574 +++- hw/i386/kvm/xenstore_impl.h | 5 + tests/unit/test-xs-node.c | 236 ++- 4 files changed, 824 insertions(+), 16 deletions(-) The code LGTM though; just need to limit the saved tree to '/local/domain/' Paul