Re: [Xen-devel] [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/11/16 15:01, Boris Ostrovsky wrote:
> On 11/09/2016 04:01 PM, Boris Ostrovsky wrote:
>> On 11/09/2016 02:58 PM, Andrew Cooper wrote:
>>> On 09/11/16 15:14, Boris Ostrovsky wrote:
>>>> On 11/09/2016 09:59 AM, Andrew Cooper wrote:
>>>>>> diff --git a/xen/include/public/hvm/ioreq.h 
>>>>>> b/xen/include/public/hvm/ioreq.h
>>>>>> index 2e5809b..e3fa704 100644
>>>>>> --- a/xen/include/public/hvm/ioreq.h
>>>>>> +++ b/xen/include/public/hvm/ioreq.h
>>>>>> @@ -24,6 +24,8 @@
>>>>>>  #ifndef _IOREQ_H_
>>>>>>  #define _IOREQ_H_
>>>>>>  
>>>>>> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
>>>>>> +
>>>>>>  #define IOREQ_READ      1
>>>>>>  #define IOREQ_WRITE     0
>>>>>>  
>>>>>> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
>>>>>>  #define ACPI_GPE0_BLK_ADDRESS        ACPI_GPE0_BLK_ADDRESS_V0
>>>>>>  #define ACPI_GPE0_BLK_LEN            ACPI_GPE0_BLK_LEN_V0
>>>>>>  
>>>>>> +#define ACPI_PM1A_EVT_BLK_LEN        0x04
>>>>>> +#define ACPI_PM1A_CNT_BLK_LEN        0x02
>>>>>> +#define ACPI_PM_TMR_BLK_LEN          0x04
>>>>>> +
>>>>>> +/* Location of online VCPU bitmap. */
>>>>>> +#define ACPI_CPU_MAP                 0xaf00
>>>>>> +#define ACPI_CPU_MAP_LEN             ((HVM_MAX_VCPUS / 8) + \
>>>>>> +                                      ((HVM_MAX_VCPUS & 7) ? 1 : 0))
>>>>>> +#if ACPI_CPU_MAP + ACPI_CPU_MAP_LEN >= ACPI_GPE0_BLK_ADDRESS_V1
>>>>>> +#error "ACPI_CPU_MAP is too big"
>>>>>> +#endif
>>>>> Why is this in ioreq.h?  It has nothing to do with ioreq's.
>>>>>
>>>>> The current ACPI bits in here are to do with the qemu ACPI interface,
>>>>> not the Xen ACPI interface.
>>>>>
>>>>> Also, please can we avoid hard-coding the location of the map in the
>>>>> hypervisor ABI.  These constants make it impossible to ever extend the
>>>>> number of HVM vcpus at a future date.
>>>> The first three logically belong here because corresponding blocks'
>>>> addresses are defined right above.
>>> They have no relationship to the ones above, other than their name.
>> They describe the same object --- for example
>> ACPI_PM1A_CNT_BLK_ADDRESS_V1 and (new) ACPI_PM1A_CNT_BLK_LEN describe
>> pm1a control.
>>
>> As far as definitions being there for qemu interface only ---
>> ACPI_PM1A_CNT_BLK_ADDRESS_V1, for example, is used only by hvmloader and
>> libacpi.
>>
>>
>>>> ACPI_CPU_MAP has to be seen by both the toolstack (libacpi) and the
>>>> hypervisor (and qemu as well, although it is defined as
>>>> PIIX4_CPU_HOTPLUG_IO_BASE).
>>>>
>>>> Where do you think it should go then?
>>> This highlights a reoccurring problem in Xen which desperately needs
>>> fixing, but still isn't high enough on my TODO list to tackle yet.
>>>
>>> There is no central registration of claims on domain resources.  This is
>>> the root cause of memory accounting problems for HVM guests.
>>>
>>>
>>> The way I planned to fix this was to have Xen maintain a registry of
>>> domains physical resources which ends up looking very much like
>>> /proc/io{mem,ports}.  There will be a hypercall interface for querying
>>> this information, and for a toolstack and device model to modify it.
>>>
>>> The key point is that Xen needs to be authoritative source of
>>> information pertaining to layout, rather than the current fiasco we have
>>> of the toolstack, qemu and hvmloader all thinking they know and control
>>> what's going on.  This fixes several current unknowns which have caused
>>> real problems, such as whether a domain was told about certain RMRRs
>>> when it booted, or how many PXEROMs qemu tried to fit into the physmap.
>>>
>>> This information (eventually, when I get Xen-level migration v2 sorted)
>>> needs to move at the head of the migration stream.
>>>
>>> The way I would envisage this working is that on domain create, Xen
>>> makes a blank map indicating that all space is free.  By selecting
>>> X86_EMUL_APCI_*, Xen takes out an allocation when it wires up the ioport
>>> handler.
>>>
>>> Later, when constructing the ACPI tables, the toolstack reads the
>>> current ioport allocations and can see exactly which ports are reserved
>>> for what.
>>>
>>>
>>> Now, I understand that lumbering you with this work as a prerequisite
>>> would be unfair.
>>>
>>> Therefore, I will accept an alternative of hiding all these definitions
>>> behind __XEN_TOOLS__ so the longterm fix can be introduced in a
>>> compatible manner in the future.
>> __XEN_TOOLS__ or (__XEN__ || __XEN_TOOLS__) ? Because both the toolstack
>> and the hypervisor want to see them.

(__XEN__ || __XEN_TOOLS__) is fine.

>>
>>
>>> That said, I am still certain that they shouldn't live in ioreq.h, as
>>> they have nothing to do with Qemu.
>> None of the existing files looks (to me) much better in terms of being
>> more appropriate. include/public/arch-x86/xen.h?
> Andrew, ping on these two questions.

Sorry for letting this slip through the cracks.

Leave them here for now.  xen.h is not a better place for them to live.

~Andrew

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