xen-dom-mgmt: do not left domain paused on creation #86

firscity · 2024-05-13T10:43:19Z

Some hacky logic were added previously on domain creation - only Domain-D is unpaused after domain_create(). This looks like hack, that definitely should be removed from mainline library.

lorc

I agree, this was hack. I am fine with this change:

Reviewed-by: Volodymyr Babchuk <[email protected]>

But, could you please add -p parameter to xu create command in case someone wants to create domain in paused state? I believe it was needed for PV drivers (or vchan?) debugging.

Deedone · 2024-05-13T10:52:26Z

Reviewed-by: Mykyta Poturai <[email protected]>

firscity · 2024-05-13T10:56:51Z

Thanks for review

But, could you please add -p parameter to xu create command in case someone wants to create domain in paused state? I believe it was needed for PV drivers (or vchan?) debugging.

Yes, it is possible, but after changing domain_create() signature, unpause is currently called inside. Are you OK if it will be made separately after AoS release? Unfortunately, it will affect some other PRs.

lorc · 2024-05-13T11:20:17Z

Yes, it is possible, but after changing domain_create() signature, unpause is currently called inside. Are you OK if it will be made separately after AoS release? Unfortunately, it will affect some other PRs.

Well, I am not sure that we need additional parameter. This can be flag in a domain configuration structure.

xakep-amatop · 2024-05-13T11:20:17Z

Acked-by: Mykola Kvach <[email protected]>

GrygiriiS · 2024-05-14T08:33:14Z

xen-dom-mgmt/src/xen-dom-mgmt.c

@@ -742,14 +742,10 @@ int domain_create(struct xen_domain_cfg *domcfg, uint32_t domid)
 		goto stop_domain_console;
 	}

-	if (domid == DOMID_DOMD) {


Seems you can drop DOMID_DOMD now

Thanks, done.

GrygiriiS · 2024-05-14T09:04:05Z

And API documentation seems need to be updated to mention that domain up and running

GrygiriiS · 2024-05-14T10:22:33Z

Yes, it is possible, but after changing domain_create() signature, unpause is currently called inside. Are you OK if it will be made separately after AoS release? Unfortunately, it will affect some other PRs.

Well, I am not sure that we need additional parameter. This can be flag in a domain configuration structure.

I'd probably agree with @lorc here. This patch will unconditionally change current DomU start sequence from:

domain_create
domain_post_create
domain_unpause

--- to ---

domain_create
domain_unpause
domain_post_create

If it's safe - ok then. if some doubts - It'd better to add "f_noautostart" in domcfg with default f_noautostart=false. All prods now expect DomD started from static dom_cfg (and use only one static domcfg), so f_noautostart will be "false" and no changes in prods will be needed.

firscity

Thanks everyone for review and suggestions!

And API documentation seems need to be updated to mention that domain up and running

This hack wasn't even mentioned in function description :)

to
domain_create
domain_unpause
domain_post_create

AFAIK, this is correct sequence and should not be a problem, maybe @lorc can correct me.

Since last push:

removed #define DOMID_DOMD
added pausing flag to domain config
added optional -p parameter for xu create

firscity · 2024-05-14T21:39:32Z

xen-dom-mgmt/src/xen-dom-mgmt.c

@@ -742,14 +742,10 @@ int domain_create(struct xen_domain_cfg *domcfg, uint32_t domid)
 		goto stop_domain_console;
 	}

-	if (domid == DOMID_DOMD) {


Thanks, done.

GrygiriiS · 2024-05-15T06:59:58Z

Please, the commit message for last patch "xen-shell-cmd: add optional paramenters for pausing domain on creation" should contain cmd run/test example.

And seems some spelling issues.

firscity · 2024-05-15T09:59:54Z

Please, the commit message for last patch "xen-shell-cmd: add optional paramenters for pausing domain on creation" should contain cmd run/test example.

Currently any of our product do not use config_name in create command. To provide valuable output for command run advanced environment required. I tested flag functionality (set and successfully triggers domain creation logic) and updated help section for xu command.

And seems some spelling issues.

Can you please clarify?

lorc

s/paramenters/parameters

Configure spell checker already :)

Acked-by: Volodymyr Babchuk <[email protected]>

GrygiriiS · 2024-05-15T14:18:13Z

Please, the commit message for last patch "xen-shell-cmd: add optional paramenters for pausing domain on creation" should contain cmd run/test example.

Currently any of our product do not use config_name in create command. To provide valuable output for command run advanced environment required. I tested flag functionality (set and successfully triggers domain creation logic) and updated help section for xu command.

Then drop it. If you can't test it without magic. And I can't test it - then drop it from this PR, please?

Otherwise, provide full cmd line test sequence results:

xu create - without "p"
xstat stat - to show dom is unpaused
[xu console] - to prove dom is working
xu destroy
xu create - with "p"
xstat stat - to show dom is paused
[xu console] - to prove dom is paused
xu unpause - to run dom
xstat stat - to show dom is unpaused
[xu console] - to prove dom is working
xu destroy

And seems some spelling issues.

Can you please clarify?

You have "paramenters" in commit subject.

GrygiriiS · 2024-05-15T14:25:21Z

For patches 1 & 2: Reviewed-by: Grygorii Strashko <[email protected]>

Some hacky logic were added previously on domain creation - only Domain-D is unpaused after domain_create(). This looks like hack, that definitely should be removed from mainline library. Signed-off-by: Dmytro Firsov <[email protected]> Acked-by: Volodymyr Babchuk <[email protected]> Reviewed-by: Grygorii Strashko <[email protected]>

Some users may want to left domain in paused state after creation. Add flag for domain config that allows to configure it before passing it to domain_create(). Signed-off-by: Dmytro Firsov <[email protected]> Acked-by: Volodymyr Babchuk <[email protected]> Reviewed-by: Grygorii Strashko <[email protected]>

Previously f_paused flag was introduced to domain configuration struct. Add optional parameter for "xu create" command to left created domain paused after creation. Signed-off-by: Dmytro Firsov <[email protected]> Acked-by: Volodymyr Babchuk <[email protected]>

GrygiriiS · 2024-05-16T09:09:58Z

FYI. Last patch works not as expected. It updates dom-cfg->f_paused and this configuration can't be undone without reboot.

cfg->f_paused = false
xu create rpi_5_domu    --> unpaused
xu create rpi_5_domu -p --> paused cfg->f_paused=true
xu create rpi_5_domu    --> paused cfg->f_paused=true
xu create rpi_5_domu -p --> paused cfg->f_paused=true
xu create rpi_5_domu    --> paused cfg->f_paused=true

firscity requested review from lorc, Deedone, LKomaryanskiy and xakep-amatop May 13, 2024 10:43

lorc approved these changes May 13, 2024

View reviewed changes

xakep-amatop approved these changes May 13, 2024

View reviewed changes

GrygiriiS reviewed May 14, 2024

View reviewed changes

firscity force-pushed the pausing_fix branch from 054a530 to 36de39d Compare May 14, 2024 21:37

firscity commented May 14, 2024

View reviewed changes

lorc approved these changes May 15, 2024

View reviewed changes

firscity force-pushed the pausing_fix branch from 36de39d to 5345755 Compare May 15, 2024 14:19

firscity added 3 commits May 15, 2024 17:30

firscity force-pushed the pausing_fix branch from 5345755 to 24b6194 Compare May 15, 2024 14:30

firscity merged commit 7c39b56 into xen-troops:main May 15, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xen-dom-mgmt: do not left domain paused on creation #86

xen-dom-mgmt: do not left domain paused on creation #86

firscity commented May 13, 2024

lorc left a comment

Deedone commented May 13, 2024

firscity commented May 13, 2024

lorc commented May 13, 2024

xakep-amatop commented May 13, 2024

GrygiriiS May 14, 2024

firscity May 14, 2024

GrygiriiS commented May 14, 2024

GrygiriiS commented May 14, 2024

firscity left a comment

firscity May 14, 2024

GrygiriiS commented May 15, 2024

firscity commented May 15, 2024

lorc left a comment

GrygiriiS commented May 15, 2024

GrygiriiS commented May 15, 2024

GrygiriiS commented May 16, 2024

xen-dom-mgmt: do not left domain paused on creation #86

xen-dom-mgmt: do not left domain paused on creation #86

Conversation

firscity commented May 13, 2024

lorc left a comment

Choose a reason for hiding this comment

Deedone commented May 13, 2024

firscity commented May 13, 2024

lorc commented May 13, 2024

xakep-amatop commented May 13, 2024

GrygiriiS May 14, 2024

Choose a reason for hiding this comment

firscity May 14, 2024

Choose a reason for hiding this comment

GrygiriiS commented May 14, 2024

GrygiriiS commented May 14, 2024

firscity left a comment

Choose a reason for hiding this comment

firscity May 14, 2024

Choose a reason for hiding this comment

GrygiriiS commented May 15, 2024

firscity commented May 15, 2024

lorc left a comment

Choose a reason for hiding this comment

GrygiriiS commented May 15, 2024

GrygiriiS commented May 15, 2024

GrygiriiS commented May 16, 2024