Re: [Xen-devel] Re: Paravirtualizing bits of acpi access

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Monday 23 March 2009, Jeremy Fitzhardinge wrote:
> Rafael J. Wysocki wrote:
> > On Monday 23 March 2009, Tian, Kevin wrote:
> >   
> >> And then Xen jumps in to finish remaining steps. From this angle,
> >> Xen is not a completely new platform and, well, S3 is more like a
> >> 'S1' type from dom0's p.o.v with a different trigger method. Then is
> >> it overkilled to introduce a new set of ops with 99% content 
> >> duplicated?
> >>     
> >
> > IMO, no, it isn't.
> 
> Hm.  Well, lets take acpi_suspend_enter() as a specific example.  The 
> Xen change here is:
> 
> @@ -240,11 +240,20 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
>  		barrier();
>  		status = acpi_enter_sleep_state(acpi_state);
>  		break;
>  
>  	case ACPI_STATE_S3:
> -		do_suspend_lowlevel();
> +		if (!xen_pv_domain())
> +			do_suspend_lowlevel();
> +		else {
> +			/*
> +			 * Xen will save and restore CPU context, so
> +			 * we can skip that and just go straight to
> +			 * the suspend.
> +			 */
> +			acpi_enter_sleep_state(acpi_state);
> +		}
>  		break;
>  	}

Hmm, in that case it may be more appropriate to modify
do_suspend_lowlevel().  Have you considered doing that?

>  	/* If ACPI is not enabled by the BIOS, we need to enable it here. */
>  	if (set_sci_en_on_resume)
> 
> 
> Which is, functionally, adding one if() and a new line of code, in the 
> middle of a ~70 line function.
> 
> Are you suggesting that it would be best to copy this whole function so 
> that I can put one line of Xen-specific code in the middle, rather than 
> just making this change?

No.

> Some other functions, the Xen vs. non-Xen changes are larger; 
> acpi_sleep_prepare() could reasonably have a Xen-specific variant 
> because a big chunk of it is setting up the wakeup vector (which is 
> unnecessary under Xen), and the rest can be easily pulled into common 
> code.  But unfortunately acpi_sleep_prepare isn't itself an operation, 
> and is only called at the bottom of 2-3 level deep callchains.
> 
> I think that rather than having a separate xen-acpi 
> platform_suspend_ops, it would make more sense to have a acpi_ops within 
> acpi/sleep.c and handle the differences that way.  I'll see how it turns 
> out.

Yes, that may be better.

Thanks,
Rafael