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

[Tang Chen <tangchen@cn.fujitsu.com>]

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