Re: [Xen-devel] [PATCH v4 09/10] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.05.16 at 10:54, <quan.xu@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -43,36 +43,71 @@ struct acpi_sleep_info acpi_sinfo;
>  
>  void do_suspend_lowlevel(void);
>  
> +enum dev_power_type
> +{
> +    /* Use for all of device power type */

No sure what this comment is meant to say. Also if you mean it to
apply to just TYPE_ALL, it would probably better be placed on that
same line. In any event it is missing a full stop.

> +    TYPE_ALL,
> +    TYPE_CONSOLE,
> +    TYPE_TIME,
> +    TYPE_I8259A,
> +    TYPE_IOAPIC,
> +    TYPE_IOMMU,
> +    TYPE_LAPIC,
> +    /* Use for error */

Same here, but ...

> +    TYPE_UNKNOWN

... this isn't used anywhere anyway, and hence should be left out.
Errors would better be reported via -E... values anyway.

>  static int device_power_down(void)
>  {
> -    console_suspend();
> +    if ( console_suspend() )
> +        return TYPE_CONSOLE;

This (together with the resume side) makes me guess that the use
of TYPE_ as a prefix confused not just me, but also you: Here you
want to tell the caller that everything _prior_ to console suspend
(which happens to be nothing in this specific case) needs to be
undone. Yet below you use TYPE_CONSOLE as an indication that
console_resume() needs to be called.

> -    time_suspend();
> +    if ( time_suspend() )
> +        return TYPE_TIME;
>  
> -    i8259A_suspend();
> +    if ( i8259A_suspend() )
> +        return TYPE_I8259A;
>  
> +    /* ioapic_suspend should never fail */
>      ioapic_suspend();

The comment is bogus: "should" means it can in theory. Yet the
function having void return type means it just cannot fail.

> -static void device_power_up(void)
> +static void device_power_up(enum dev_power_type type)
>  {
> -    lapic_resume();
> -
> -    iommu_resume();
> -
> -    ioapic_resume();
> -
> -    i8259A_resume();
> -
> -    time_resume();
> -
> -    console_resume();
> +    switch ( type )
> +    {
> +    case TYPE_ALL:
> +        /* fall through */
> +    case TYPE_LAPIC:

No need for a fall-through comment between immediately adjacent
case labels.

> @@ -169,6 +204,7 @@ static int enter_state(u32 state)
>      {
>          printk(XENLOG_ERR "Some devices failed to power down.");
>          system_state = SYS_STATE_resume;
> +        device_power_up(error);

Either error's and device_power_down()'s return type get changed
to enum dev_power_type, or this needs a "error > 0" conditional.

> @@ -1267,7 +1273,9 @@ static void __hwdom_init intel_iommu_hwdom_init(struct 
> domain *d)
>      setup_hwdom_pci_devices(d, setup_hwdom_device);
>      setup_hwdom_rmrr(d);
>  
> -    iommu_flush_all();
> +    if ( iommu_flush_all() )
> +        printk(XENLOG_WARNING VTDPREFIX
> +               " intel_iommu_hwdom_init: IOMMU flush all failed.\n");

As said (and I think a number of times) before: At least when you
already hold the error value in your hands, please also log it. Also
personally I'm opposed to including function names in non-debug
log messages, but I'll leave that decision to the VT-d maintainers
here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel