Re: [Qemu-devel] [RESEND PATCH v3 6/8] acpi: Add hardware implementation for memory hot unplug.

[Tang Chen]

Hi Igor,

On 09/04/2014 10:20 PM, Igor Mammedov wrote:

> ......
> +
> +        acpi_handle_ost_event(mdev);
> _OST is optional and OSPM doesn't have to call it at all,
> it was already discussed on list and using _OST for device removal
> was evaluated as not usable.
> We use _OST here as supplementary status information for management tools
> and no more /i.e. as LEDs on real hardware/.
>
> See "Figure 6-37 Device Ejection Flow Example Using _OST." in ACPI 5.1 spec
> and note below it.

OK, I agree we should not make memory hotplug process depend on _OST.

But we do need to handle one problem:   When guest OS failed to remove 
device,
we should not clear QEmu data of that device.

As you and ACPI spec said, Platform (which is QEmu here) should reply _OST
rised by guest OS except things such as LEDs on real hardware. But _OST is
the only way I can find to get status from guest OS.

So, let's do it in the following way:
1. Make device hotplug process independent from _OST for the 
compatibility reason.

2. For OSPMs that support _OST, QEmu use _OST to catch error from guest OS.
    May be report an error message only, and stop QEmu from remove 
related data.
    (Please see/review patch-set:
https://www.mail-archive.com/qemu-devel%40nongnu.org/msg253025.html)

3. For OSPMs that do not support _OST, do not handle guest OS error for now.

How do you think ?

Thanks.

> >           break;
> > -    case 0x14:
> > +    case 0x14: /* set is_* fields */
> >           mdev = &mem_st->devs[mem_st->selector];
> > +
> >           if (data & 2) { /* clear insert event */
> >               mdev->is_inserting  = false;
> >               trace_mhp_acpi_clear_insert_evt(mem_st->selector);
> > +        } else if (data & 4) { /* request removal of device */
> > +            mdev->is_enabled = false;
> device tear-down should be initiated here, when OSPM calls _EJ0 method
> and when we return from this function it should be destroyed.
>
> >           }
> > +
> > +        break;
> > +    default:
> >           break;
> >       }
> > -
> >   }
> >   static const MemoryRegionOps acpi_memory_hotplug_ops = {
> >       .read = acpi_memory_hotplug_read,
> > @@ -198,6 +250,7 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, 
> > MemHotplugState *mem_st,
> >   void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState 
> > *mem_st,
> >                              DeviceState *dev, Error **errp)
> >   {
> > +    MemStatus *mdev;
> >       Error *local_err = NULL;
> >       int slot = object_property_get_int(OBJECT(dev), "slot", &local_err);
> >   
> > @@ -215,6 +268,9 @@ void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
> >           return;
> >       }
> >   
> > +    mdev = &mem_st->devs[slot];
> > +    mdev->is_removing = true;
> > +
> >       /* do ACPI magic */
> >       ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
> >       acpi_update_sci(ar, irq);
> > diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> > index 1f678b4..e105e45 100644
> > --- a/include/hw/acpi/acpi.h
> > +++ b/include/hw/acpi/acpi.h
> > @@ -91,6 +91,20 @@
> >   /* PM2_CNT */
> >   #define ACPI_BITMASK_ARB_DISABLE                0x0001
> >   
> > +/* OST_EVENT */
> > +#define ACPI_NOTIFY_EJECT_REQUEST               0x03
> > +#define ACPI_OSPM_EJECT                         0x103
> > +
> > +/* OST_STATUS */
> > +#define ACPI_SUCCESS                            0x0
> > +#define ACPI_FAILURE                            0x1
> > +#define ACPI_UNRECOGNIZED_NOTIFY                0x2
> > +#define ACPI_EJECT_NOT_SUPPORTED                0x80
> > +#define ACPI_EJECT_DEVICE_IN_USE                0x81
> > +#define ACPI_EJECT_DEVICE_BUSY                  0x82
> > +#define ACPI_EJECT_DEPENDENCY_BUSY              0x83
> > +#define ACPI_EJECT_IN_PROGRESS                  0x84
> > +
> >   /* structs */
> >   typedef struct ACPIPMTimer ACPIPMTimer;
> >   typedef struct ACPIPM1EVT ACPIPM1EVT;
> > diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
> > index fc6b868..fe41268 100644
> > --- a/include/hw/acpi/memory_hotplug.h
> > +++ b/include/hw/acpi/memory_hotplug.h
> > @@ -11,6 +11,7 @@ typedef struct MemStatus {
> >       DeviceState *dimm;
> >       bool is_enabled;
> >       bool is_inserting;
> > +    bool is_removing;
> >       uint32_t ost_event;
> >       uint32_t ost_status;
> >   } MemStatus;
> .