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

["Xu, Quan" <quan.xu@xxxxxxxxx>]

On May 24, 2016 4:22 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 18.05.16 at 10:08, <quan.xu@xxxxxxxxx> wrote:
> > --- a/xen/arch/x86/acpi/power.c
> > +++ b/xen/arch/x86/acpi/power.c
> > @@ -43,36 +43,68 @@ struct acpi_sleep_info acpi_sinfo;
> >
> >  void do_suspend_lowlevel(void);
> >
> > +enum dev_power_saved
> > +{
> > +    SAVED_NONE,    /* None of device power saved */
> > +    SAVED_CONSOLE, /* Device power of console saved */
> > +    SAVED_TIME,    /* Device power of time saved */
> > +    SAVED_I8259A,  /* Device power of I8259A saved */
> > +    SAVED_IOAPIC,  /* Device power of IOAPIC saved */
> > +    SAVED_IOMMU,   /* Device power of IOMMU saved */
> > +    SAVED_LAPIC,   /* Device power of LAPIC saved */
> > +};
> 
> Please avoid comments saying nothing substantially different than the code
> they accompany, and also not helping the reader to understand the
> commented code.
>

I'll drop these comments in v6.

 
> >  static int device_power_down(void)
> >  {
> > -    console_suspend();
> > +    if ( console_suspend() )
> > +        return SAVED_CONSOLE;
> 
> I said so on the previous round, and I need to repeat it now: If
> console_suspend() fails, you saved _nothing_.
> 

Ah, we may have some different views for SAVED_*, which I mean has been saved 
and we are no need to resume.

e.g. if console_suspend() fails, I did return SAVED_CONSOLE, reading my patch 
again, and I really resume nothing at all.

device_power_up()
{
     ...
    +    case SAVED_CONSOLE:
    +        break;
    ...
}


I know we can also propagate SAVED_NONE for console_suspend() failure, then we 
need adjust device_power_up() relevantly.


> > -    time_suspend();
> > +    if ( time_suspend() )
> > +        return SAVED_TIME;
> >
> > -    i8259A_suspend();
> > +    if ( i8259A_suspend() )
> > +        return SAVED_I8259A;
> >
> > +    /* ioapic_suspend cannot fail */
> >      ioapic_suspend();
> >
> > -    iommu_suspend();
> > +    if ( iommu_suspend() )
> > +        return SAVED_IOMMU;
> >
> > -    lapic_suspend();
> > +    if ( lapic_suspend() )
> > +        return SAVED_LAPIC;
> >
> >      return 0;
> 
> And this silently means SAVED_NONE, whereas here you saved everything.
> Yielding clearly bogus code ...
>


 '0' is just on success here.  Look at the condition where we call 
device_power_up():

+        if ( error > 0 )
+            device_power_up(error);

Then, it is not bogus code.
 

> > -static void device_power_up(void)
> > +static void device_power_up(enum dev_power_saved saved)
> >  {
> > -    lapic_resume();
> > -
> > -    iommu_resume();
> > -
> > -    ioapic_resume();
> > -
> > -    i8259A_resume();
> > -
> > -    time_resume();
> > -
> > -    console_resume();
> > +    switch ( saved )
> > +    {
> > +    case SAVED_NONE:
> > +        lapic_resume();
> 
> ... here and ...
> 
> > @@ -196,7 +232,7 @@ static int enter_state(u32 state)
> >      write_cr4(cr4 & ~X86_CR4_MCE);
> >      write_efer(read_efer());
> >
> > -    device_power_up();
> > +    device_power_up(SAVED_NONE);
> 
> ... here.

Then we need to call all of *_resume(). I think the logic is correct, but the 
SAVED_*  may be not what you suggested.

> 
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -541,20 +541,28 @@ static int iommu_flush_iotlb_psi(
> >      return status;
> >  }
> >
> > -static void iommu_flush_all(void)
> > +static int __must_check iommu_flush_all(void)
> >  {
> >      struct acpi_drhd_unit *drhd;
> >      struct iommu *iommu;
> >      int flush_dev_iotlb;
> > +    int rc = 0;
> >
> >      flush_all_cache();
> >      for_each_drhd_unit ( drhd )
> >      {
> > +        int ret;
> > +
> >          iommu = drhd->iommu;
> >          iommu_flush_context_global(iommu, 0);
> >          flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
> > -        iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
> > +        ret = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
> > +
> > +        if ( unlikely(ret) )
> > +            rc = ret;
> 
> Same as for earlier patches - "if ( unlikely(!rc) )" please.
> 


Yes,


> Also, having come here - did I miss iommu_flush_iotlb_global() gaining a
> __must_check annotation somewhere? 

I will add __must_check annotation to iommu_flush_iotlb_global().

> And the struct iommu_flush pointers
> and handlers? And, by analogy, iommu_flush_context_*()?
> 

I am better only add __must_check annotation to flush->iotlb and handlers,
but leaving flush->context/handers and  iommu_flush_context_*() as are in 
current patch set..
the coming patch set will fix them.

Quan

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