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

[Igor Mammedov]

On Tue, 16 Sep 2014 18:12:02 +0800
Tang Chen <address@hidden> wrote:

> 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 ?
I'd keep _OST handling in ACPI write handler only for notification purposes
only. This way management tools/end user will see guest OS error if supported
, otherwise we do not care.

Make '_OST unplug handler' reset removing bit in case of error, so that guest
won't see DIMM stuck in continuous removing state when following hotplug events
happen.


> 
> 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;
> > .
> >
>