Re: [Xen-devel] [PATCH v4 07/15] pvh/acpi: Install handlers for ACPI-related PVH IO accesses

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 12/01/2016 11:32 AM, Jan Beulich wrote:
>>>> On 29.11.16 at 16:33, <boris.ostrovsky@xxxxxxxxxx> wrote:
>> +void hvm_acpi_init(struct domain *d)
>> +{
>> +    if ( has_acpi_dm_ff(d) )
>> +        return;
>> +
>> +    register_portio_handler(d, XEN_ACPI_CPU_MAP,
>> +                            XEN_ACPI_CPU_MAP_LEN, acpi_guest_access);
>> +    register_portio_handler(d, ACPI_GPE0_BLK_ADDRESS_V1,
>> +                            sizeof (d->arch.hvm_domain.acpi.gpe0_sts) +
>> +                            sizeof (d->arch.hvm_domain.acpi.gpe0_en),
> Keyword or not, we don't put spaces between sizeof and the
> opening parenthesis.
>
>> +                            acpi_guest_access);
>> +    register_portio_handler(d, ACPI_PM1A_EVT_BLK_ADDRESS_V1,
>> +                            sizeof (d->arch.hvm_domain.acpi.pm1a_sts) +
>> +                            sizeof (d->arch.hvm_domain.acpi.pm1a_en),
>> +                            acpi_guest_access);
> Does it really result in most legible / maintainable code (in
> subsequent patches) if all three use the same handler?

I can split them into 2 --- one for pm1a/gpe0 and the other for CPU map.
They are indeed handled differently.


>
>> --- a/xen/include/public/arch-x86/hvm/save.h
>> +++ b/xen/include/public/arch-x86/hvm/save.h
>> @@ -532,6 +532,8 @@ struct hvm_hw_acpi {
>>      uint32_t tmr_val;   /* PM_TMR_BLK.TMR_VAL: 32bit free-running counter */
>>      uint16_t pm1a_sts;  /* PM1a_EVT_BLK.PM1a_STS: status register */
>>      uint16_t pm1a_en;   /* PM1a_EVT_BLK.PM1a_EN: enable register */
>> +    uint16_t gpe0_sts;
>> +    uint16_t gpe0_en;
>>  };
> Don't you need to create compat handling for the case where a
> smaller struct arrives during migration? Or do we zero extend
> structures nowadays without extra code being needed (assuming
> zero extension is what we want in that case in the first place)?

I thought that the fact that new fields are added at the end would make
this safe. And I assumed we will always read into a newly-allocated
hvm_domain, so those registers would be zero.

But being more explicit about this is probably better, I'll add
DECLARE_HVM_SAVE_TYPE_COMPAT.

Andrew, will this interfere with your hypervisor migration v2 plan?


-boris


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