[PATCH] x86/hvm: Extend HVM cpuid leaf with vcpu id

[PATCH] x86/hvm: Extend HVM cpuid leaf with vcpu id

[Paul Durrant]

To perform certain hypercalls HVM guests need to use Xen's idea of
vcpu id, which may well not match the guest OS idea of CPU id.
This patch adds vcpu id to the HVM cpuid leaf allowing the guest
to build a mapping.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/hvm/hvm.c              |    4 ++++
 xen/include/public/arch-x86/cpuid.h |    5 +++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 78f519d..d9a5706 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4189,6 +4189,10 @@ void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
          * foreign pages) has valid IOMMU entries.
          */
         *eax |= XEN_HVM_CPUID_IOMMU_MAPPINGS;
+
+        /* Indicate presence of vcpu id and set it in ebx */
+        *eax |= XEN_HVM_CPUID_VCPU_ID_PRESENT;
+        *ebx = current->vcpu_id;
     }
 }
 
diff --git a/xen/include/public/arch-x86/cpuid.h b/xen/include/public/arch-x86/cpuid.h
index 6005dfe..8ccb6e1 100644
--- a/xen/include/public/arch-x86/cpuid.h
+++ b/xen/include/public/arch-x86/cpuid.h
@@ -76,13 +76,14 @@
 /*
  * Leaf 5 (0x40000x04)
  * HVM-specific features
+ * EAX: Features
+ * EBX: VCPU ID
  */
-
-/* EAX Features */
 #define XEN_HVM_CPUID_APIC_ACCESS_VIRT (1u << 0) /* Virtualized APIC registers */
 #define XEN_HVM_CPUID_X2APIC_VIRT      (1u << 1) /* Virtualized x2APIC accesses */
 /* Memory mapped from other domains has valid IOMMU entries */
 #define XEN_HVM_CPUID_IOMMU_MAPPINGS   (1u << 2)
+#define XEN_HVM_CPUID_VCPU_ID_PRESENT  (1u << 3) /* vcpu is present in EBX */
 
 #define XEN_CPUID_MAX_NUM_LEAVES 4
 
-- 
1.7.10.4

Re: [PATCH] x86/hvm: Extend HVM cpuid leaf with vcpu id

[Andrew Cooper]

On 06/11/14 15:07, Paul Durrant wrote:
> To perform certain hypercalls HVM guests need to use Xen's idea of
> vcpu id, which may well not match the guest OS idea of CPU id.
> This patch adds vcpu id to the HVM cpuid leaf allowing the guest
> to build a mapping.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/arch/x86/hvm/hvm.c              |    4 ++++
>  xen/include/public/arch-x86/cpuid.h |    5 +++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 78f519d..d9a5706 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4189,6 +4189,10 @@ void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
>           * foreign pages) has valid IOMMU entries.
>           */
>          *eax |= XEN_HVM_CPUID_IOMMU_MAPPINGS;
> +
> +        /* Indicate presence of vcpu id and set it in ebx */
> +        *eax |= XEN_HVM_CPUID_VCPU_ID_PRESENT;
> +        *ebx = current->vcpu_id;
>      }
>  }
>  
> diff --git a/xen/include/public/arch-x86/cpuid.h b/xen/include/public/arch-x86/cpuid.h
> index 6005dfe..8ccb6e1 100644
> --- a/xen/include/public/arch-x86/cpuid.h
> +++ b/xen/include/public/arch-x86/cpuid.h
> @@ -76,13 +76,14 @@
>  /*
>   * Leaf 5 (0x40000x04)
>   * HVM-specific features
> + * EAX: Features
> + * EBX: VCPU ID

Probably want "iff EAX & VCPU_ID_PRESENT" in this comment.

>   */
> -
> -/* EAX Features */

Spurious delete?

>  #define XEN_HVM_CPUID_APIC_ACCESS_VIRT (1u << 0) /* Virtualized APIC registers */
>  #define XEN_HVM_CPUID_X2APIC_VIRT      (1u << 1) /* Virtualized x2APIC accesses */
>  /* Memory mapped from other domains has valid IOMMU entries */
>  #define XEN_HVM_CPUID_IOMMU_MAPPINGS   (1u << 2)
> +#define XEN_HVM_CPUID_VCPU_ID_PRESENT  (1u << 3) /* vcpu is present in EBX */

vcpu id?

~Andrew

>  
>  #define XEN_CPUID_MAX_NUM_LEAVES 4
>  

Re: [PATCH] x86/hvm: Extend HVM cpuid leaf with vcpu id

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 06 November 2014 15:14
> To: Paul Durrant; xen-devel@lists.xen.org
> Cc: Keir (Xen.org); Jan Beulich
> Subject: Re: [Xen-devel] [PATCH] x86/hvm: Extend HVM cpuid leaf with vcpu
> id
> 
> On 06/11/14 15:07, Paul Durrant wrote:
> > To perform certain hypercalls HVM guests need to use Xen's idea of
> > vcpu id, which may well not match the guest OS idea of CPU id.
> > This patch adds vcpu id to the HVM cpuid leaf allowing the guest
> > to build a mapping.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Cc: Keir Fraser <keir@xen.org>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > ---
> >  xen/arch/x86/hvm/hvm.c              |    4 ++++
> >  xen/include/public/arch-x86/cpuid.h |    5 +++--
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index 78f519d..d9a5706 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -4189,6 +4189,10 @@ void hvm_hypervisor_cpuid_leaf(uint32_t
> sub_idx,
> >           * foreign pages) has valid IOMMU entries.
> >           */
> >          *eax |= XEN_HVM_CPUID_IOMMU_MAPPINGS;
> > +
> > +        /* Indicate presence of vcpu id and set it in ebx */
> > +        *eax |= XEN_HVM_CPUID_VCPU_ID_PRESENT;
> > +        *ebx = current->vcpu_id;
> >      }
> >  }
> >
> > diff --git a/xen/include/public/arch-x86/cpuid.h b/xen/include/public/arch-
> x86/cpuid.h
> > index 6005dfe..8ccb6e1 100644
> > --- a/xen/include/public/arch-x86/cpuid.h
> > +++ b/xen/include/public/arch-x86/cpuid.h
> > @@ -76,13 +76,14 @@
> >  /*
> >   * Leaf 5 (0x40000x04)
> >   * HVM-specific features
> > + * EAX: Features
> > + * EBX: VCPU ID
> 
> Probably want "iff EAX & VCPU_ID_PRESENT" in this comment.
> 

Yes, I guess.

> >   */
> > -
> > -/* EAX Features */
> 
> Spurious delete?
> 

Nope - it moved up into the above block.

> >  #define XEN_HVM_CPUID_APIC_ACCESS_VIRT (1u << 0) /* Virtualized
> APIC registers */
> >  #define XEN_HVM_CPUID_X2APIC_VIRT      (1u << 1) /* Virtualized x2APIC
> accesses */
> >  /* Memory mapped from other domains has valid IOMMU entries */
> >  #define XEN_HVM_CPUID_IOMMU_MAPPINGS   (1u << 2)
> > +#define XEN_HVM_CPUID_VCPU_ID_PRESENT  (1u << 3) /* vcpu is
> present in EBX */
> 
> vcpu id?

Yes. Typo.

  Paul

> 
> ~Andrew
> 
> >
> >  #define XEN_CPUID_MAX_NUM_LEAVES 4
> >

Re: [PATCH] x86/hvm: Extend HVM cpuid leaf with vcpu id

[Andrew Cooper]

On 06/11/14 15:16, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper
>> Sent: 06 November 2014 15:14
>> To: Paul Durrant; xen-devel@lists.xen.org
>> Cc: Keir (Xen.org); Jan Beulich
>> Subject: Re: [Xen-devel] [PATCH] x86/hvm: Extend HVM cpuid leaf with vcpu
>> id
>>
>> On 06/11/14 15:07, Paul Durrant wrote:
>>> To perform certain hypercalls HVM guests need to use Xen's idea of
>>> vcpu id, which may well not match the guest OS idea of CPU id.
>>> This patch adds vcpu id to the HVM cpuid leaf allowing the guest
>>> to build a mapping.
>>>
>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>>> Cc: Keir Fraser <keir@xen.org>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> ---
>>>  xen/arch/x86/hvm/hvm.c              |    4 ++++
>>>  xen/include/public/arch-x86/cpuid.h |    5 +++--
>>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index 78f519d..d9a5706 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -4189,6 +4189,10 @@ void hvm_hypervisor_cpuid_leaf(uint32_t
>> sub_idx,
>>>           * foreign pages) has valid IOMMU entries.
>>>           */
>>>          *eax |= XEN_HVM_CPUID_IOMMU_MAPPINGS;
>>> +
>>> +        /* Indicate presence of vcpu id and set it in ebx */
>>> +        *eax |= XEN_HVM_CPUID_VCPU_ID_PRESENT;
>>> +        *ebx = current->vcpu_id;
>>>      }
>>>  }
>>>
>>> diff --git a/xen/include/public/arch-x86/cpuid.h b/xen/include/public/arch-
>> x86/cpuid.h
>>> index 6005dfe..8ccb6e1 100644
>>> --- a/xen/include/public/arch-x86/cpuid.h
>>> +++ b/xen/include/public/arch-x86/cpuid.h
>>> @@ -76,13 +76,14 @@
>>>  /*
>>>   * Leaf 5 (0x40000x04)
>>>   * HVM-specific features
>>> + * EAX: Features
>>> + * EBX: VCPU ID
>> Probably want "iff EAX & VCPU_ID_PRESENT" in this comment.
>>
> Yes, I guess.
>
>>>   */
>>> -
>>> -/* EAX Features */
>> Spurious delete?
>>
> Nope - it moved up into the above block.

Ah - I see now.  That looks ok.

With the other changes, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

~Andrew

Re: [PATCH] x86/hvm: Extend HVM cpuid leaf with vcpu id

[Konrad Rzeszutek Wilk]

On Thu, Nov 06, 2014 at 03:07:10PM +0000, Paul Durrant wrote:
> To perform certain hypercalls HVM guests need to use Xen's idea of

What are those 'certain'?

> vcpu id, which may well not match the guest OS idea of CPU id.

Can you elaborate more on the use case please? And why
only restrict this to HVM guests? Why not PV?

> This patch adds vcpu id to the HVM cpuid leaf allowing the guest
> to build a mapping.

Don't we have existing hypercalls to get this? Ah VCPUOP_get_physid
is what I was thinking of and that does not seem to be what
you want?
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/arch/x86/hvm/hvm.c              |    4 ++++
>  xen/include/public/arch-x86/cpuid.h |    5 +++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 78f519d..d9a5706 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4189,6 +4189,10 @@ void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
>           * foreign pages) has valid IOMMU entries.
>           */
>          *eax |= XEN_HVM_CPUID_IOMMU_MAPPINGS;
> +
> +        /* Indicate presence of vcpu id and set it in ebx */
> +        *eax |= XEN_HVM_CPUID_VCPU_ID_PRESENT;
> +        *ebx = current->vcpu_id;
>      }
>  }
>  
> diff --git a/xen/include/public/arch-x86/cpuid.h b/xen/include/public/arch-x86/cpuid.h
> index 6005dfe..8ccb6e1 100644
> --- a/xen/include/public/arch-x86/cpuid.h
> +++ b/xen/include/public/arch-x86/cpuid.h
> @@ -76,13 +76,14 @@
>  /*
>   * Leaf 5 (0x40000x04)
>   * HVM-specific features
> + * EAX: Features
> + * EBX: VCPU ID
>   */
> -
> -/* EAX Features */
>  #define XEN_HVM_CPUID_APIC_ACCESS_VIRT (1u << 0) /* Virtualized APIC registers */
>  #define XEN_HVM_CPUID_X2APIC_VIRT      (1u << 1) /* Virtualized x2APIC accesses */
>  /* Memory mapped from other domains has valid IOMMU entries */
>  #define XEN_HVM_CPUID_IOMMU_MAPPINGS   (1u << 2)
> +#define XEN_HVM_CPUID_VCPU_ID_PRESENT  (1u << 3) /* vcpu is present in EBX */
>  
>  #define XEN_CPUID_MAX_NUM_LEAVES 4
>  
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] x86/hvm: Extend HVM cpuid leaf with vcpu id

[Andrew Cooper]

On 06/11/2014 19:32, Konrad Rzeszutek Wilk wrote:
> On Thu, Nov 06, 2014 at 03:07:10PM +0000, Paul Durrant wrote:
>> To perform certain hypercalls HVM guests need to use Xen's idea of
> What are those 'certain'?

Some HVM hypercalls take a vcpu_id as a parameter.  See the
functionally-unrelated-but-companion patch "x86/hvm: Add per-vcpu evtchn
upcalls".

HVM guests currently have no way of obtaining a xen vcpu_id for a given
vcpu, and therefore have no way of passing an appropriate parameter to
the hypercalls.

As it currently happens, HVM guests can (and sadly do) take their APIC
id, divide by 2, and end up with a number which happens to match the Xen
vcpu_id.  This only works because Xen's allocation of APIC IDs is wrong
on AMD hardware and Intel Hardware with hyperthreading, and is an issue
I plan to fix in the not too distant future.  (It is one of the quagmire
of issues relating to cpuid policies)

>
>> vcpu id, which may well not match the guest OS idea of CPU id.
> Can you elaborate more on the use case please? And why
> only restrict this to HVM guests? Why not PV?

PV guests unconditionally know their vcpu_id's right from the start, and
indeed need them to bring up APs.  HVM guests currently only know APIC IDs.

>
>> This patch adds vcpu id to the HVM cpuid leaf allowing the guest
>> to build a mapping.
> Don't we have existing hypercalls to get this? Ah VCPUOP_get_physid
> is what I was thinking of and that does not seem to be what
> you want?

This hypercall is only valid for pinned vcpus (i.e. only dom0 with
opt_dom0_vcpus_pin), and gives back the underlying APIC ID, and the ACPI
processor ID for that APIC ID.

The APIC ID can be obtained from cpuid as the vcpus are pinned, and the
ACPI ID can be obtained from the ACPI tables, as the domain is dom0. 
Ergo, this hypercall is quite redundant, has nothing at all to do with
the problem Paul is trying to solve, and appears buggy as it hands back
two 64bit values, shifted 32bits apart, in a 64bit integer.

~Andrew

Re: [PATCH] x86/hvm: Extend HVM cpuid leaf with vcpu id

[Matt Wilson]

On Thu, Nov 06, 2014 at 09:27:59PM +0000, Andrew Cooper wrote:
> On 06/11/2014 19:32, Konrad Rzeszutek Wilk wrote:
> > On Thu, Nov 06, 2014 at 03:07:10PM +0000, Paul Durrant wrote:
> >> To perform certain hypercalls HVM guests need to use Xen's idea of
> > What are those 'certain'?
> 
> Some HVM hypercalls take a vcpu_id as a parameter.  See the
> functionally-unrelated-but-companion patch "x86/hvm: Add per-vcpu evtchn
> upcalls".
> 
> HVM guests currently have no way of obtaining a xen vcpu_id for a given
> vcpu, and therefore have no way of passing an appropriate parameter to
> the hypercalls.

Sorry for chiming in late...

That's not strictly true. You can use the processor index in ACPI.

> As it currently happens, HVM guests can (and sadly do) take their APIC
> id, divide by 2, and end up with a number which happens to match the Xen
> vcpu_id.  This only works because Xen's allocation of APIC IDs is wrong
> on AMD hardware and Intel Hardware with hyperthreading, and is an issue
> I plan to fix in the not too distant future.  (It is one of the quagmire
> of issues relating to cpuid policies)

This is absolutely the wrong thing to do. If a guest OS is doing this,
it is doing the wrong thing. FreeBSD went down this erroneous path...

See http://lists.xen.org/archives/html/xen-devel/2013-05/msg03156.html

> >
> >> vcpu id, which may well not match the guest OS idea of CPU id.
> > Can you elaborate more on the use case please? And why
> > only restrict this to HVM guests? Why not PV?
> 
> PV guests unconditionally know their vcpu_id's right from the start, and
> indeed need them to bring up APs.  HVM guests currently only know APIC IDs.

No, don't assume anything about APIC IDs mapping to Xen virtual
processor index. This will break things on EC2 instances that expose
proper CPU topology through CPUID, and this does *not* follow the
"index * 2" formula.

> >> This patch adds vcpu id to the HVM cpuid leaf allowing the guest
> >> to build a mapping.
> > Don't we have existing hypercalls to get this? Ah VCPUOP_get_physid
> > is what I was thinking of and that does not seem to be what
> > you want?
> 
> This hypercall is only valid for pinned vcpus (i.e. only dom0 with
> opt_dom0_vcpus_pin), and gives back the underlying APIC ID, and the ACPI
> processor ID for that APIC ID.
> 
> The APIC ID can be obtained from cpuid as the vcpus are pinned, and the
> ACPI ID can be obtained from the ACPI tables, as the domain is dom0. 
> Ergo, this hypercall is quite redundant, has nothing at all to do with
> the problem Paul is trying to solve, and appears buggy as it hands back
> two 64bit values, shifted 32bits apart, in a 64bit integer.

And the Xen vCPU index can be determined by the processor object IDs
in DSDT.

--msw

Re: [PATCH] x86/hvm: Extend HVM cpuid leaf with vcpu id

[Jan Beulich]

>>> On 10.12.14 at 07:00, <msw@linux.com> wrote:
> On Thu, Nov 06, 2014 at 09:27:59PM +0000, Andrew Cooper wrote:
>> On 06/11/2014 19:32, Konrad Rzeszutek Wilk wrote:
>> > On Thu, Nov 06, 2014 at 03:07:10PM +0000, Paul Durrant wrote:
>> >> To perform certain hypercalls HVM guests need to use Xen's idea of
>> > What are those 'certain'?
>> 
>> Some HVM hypercalls take a vcpu_id as a parameter.  See the
>> functionally-unrelated-but-companion patch "x86/hvm: Add per-vcpu evtchn
>> upcalls".
>> 
>> HVM guests currently have no way of obtaining a xen vcpu_id for a given
>> vcpu, and therefore have no way of passing an appropriate parameter to
>> the hypercalls.
> 
> Sorry for chiming in late...
> 
> That's not strictly true. You can use the processor index in ACPI.

Not really, no. The ACPI tables get created by hvmloader, without
regard to Xen vCPU numbering afaict.

>> >> vcpu id, which may well not match the guest OS idea of CPU id.
>> > Can you elaborate more on the use case please? And why
>> > only restrict this to HVM guests? Why not PV?
>> 
>> PV guests unconditionally know their vcpu_id's right from the start, and
>> indeed need them to bring up APs.  HVM guests currently only know APIC IDs.
> 
> No, don't assume anything about APIC IDs mapping to Xen virtual
> processor index. This will break things on EC2 instances that expose
> proper CPU topology through CPUID, and this does *not* follow the
> "index * 2" formula.

Hmm, you say "no" first, but the rest of your statement seems to
agree with what Andrew was saying...

>> >> This patch adds vcpu id to the HVM cpuid leaf allowing the guest
>> >> to build a mapping.
>> > Don't we have existing hypercalls to get this? Ah VCPUOP_get_physid
>> > is what I was thinking of and that does not seem to be what
>> > you want?
>> 
>> This hypercall is only valid for pinned vcpus (i.e. only dom0 with
>> opt_dom0_vcpus_pin), and gives back the underlying APIC ID, and the ACPI
>> processor ID for that APIC ID.
>> 
>> The APIC ID can be obtained from cpuid as the vcpus are pinned, and the
>> ACPI ID can be obtained from the ACPI tables, as the domain is dom0. 
>> Ergo, this hypercall is quite redundant, has nothing at all to do with
>> the problem Paul is trying to solve, and appears buggy as it hands back
>> two 64bit values, shifted 32bits apart, in a 64bit integer.
> 
> And the Xen vCPU index can be determined by the processor object IDs
> in DSDT.

I hope you don't have code doing so, as such an association isn't
pinned down anywhere in the ABI afaik.

Jan

Re: [PATCH] x86/hvm: Extend HVM cpuid leaf with vcpu id

[Matt Wilson]

On Wed, Dec 10, 2014 at 10:29:46AM +0000, Jan Beulich wrote:
> >>> On 10.12.14 at 07:00, <msw@linux.com> wrote:
> > On Thu, Nov 06, 2014 at 09:27:59PM +0000, Andrew Cooper wrote:
> >> On 06/11/2014 19:32, Konrad Rzeszutek Wilk wrote:
> >> > On Thu, Nov 06, 2014 at 03:07:10PM +0000, Paul Durrant wrote:
> >> >> To perform certain hypercalls HVM guests need to use Xen's idea of
> >> > What are those 'certain'?
> >> 
> >> Some HVM hypercalls take a vcpu_id as a parameter.  See the
> >> functionally-unrelated-but-companion patch "x86/hvm: Add per-vcpu evtchn
> >> upcalls".
> >> 
> >> HVM guests currently have no way of obtaining a xen vcpu_id for a given
> >> vcpu, and therefore have no way of passing an appropriate parameter to
> >> the hypercalls.
> > 
> > Sorry for chiming in late...
> > 
> > That's not strictly true. You can use the processor index in ACPI.
> 
> Not really, no. The ACPI tables get created by hvmloader, without
> regard to Xen vCPU numbering afaict.

The ACPI tables created by hvmloader must take Xen vCPU numbering into
regard, at last as a consideration as both hvmloader and the
hypervisor share this "vcpu_id * 2" math for assigning APIC IDs.

> >> >> vcpu id, which may well not match the guest OS idea of CPU id.
> >> > Can you elaborate more on the use case please? And why
> >> > only restrict this to HVM guests? Why not PV?
> >> 
> >> PV guests unconditionally know their vcpu_id's right from the start, and
> >> indeed need them to bring up APs.  HVM guests currently only know APIC IDs.
> > 
> > No, don't assume anything about APIC IDs mapping to Xen virtual
> > processor index. This will break things on EC2 instances that expose
> > proper CPU topology through CPUID, and this does *not* follow the
> > "index * 2" formula.
> 
> Hmm, you say "no" first, but the rest of your statement seems to
> agree with what Andrew was saying...

I say "no" because the guest has more to work on than APIC IDs. It has
ACPI processor object IDs. 

> >> >> This patch adds vcpu id to the HVM cpuid leaf allowing the guest
> >> >> to build a mapping.
> >> > Don't we have existing hypercalls to get this? Ah VCPUOP_get_physid
> >> > is what I was thinking of and that does not seem to be what
> >> > you want?
> >> 
> >> This hypercall is only valid for pinned vcpus (i.e. only dom0 with
> >> opt_dom0_vcpus_pin), and gives back the underlying APIC ID, and the ACPI
> >> processor ID for that APIC ID.
> >> 
> >> The APIC ID can be obtained from cpuid as the vcpus are pinned, and the
> >> ACPI ID can be obtained from the ACPI tables, as the domain is dom0. 
> >> Ergo, this hypercall is quite redundant, has nothing at all to do with
> >> the problem Paul is trying to solve, and appears buggy as it hands back
> >> two 64bit values, shifted 32bits apart, in a 64bit integer.
> > 
> > And the Xen vCPU index can be determined by the processor object IDs
> > in DSDT.
> 
> I hope you don't have code doing so, as such an association isn't
> pinned down anywhere in the ABI afaik.

I think that both Linux and FreeBSD are using the enumeration of CPUs
in ACPI tables to derive the vCPU ID needed for HYPERVISOR_vcpu_op
hypercalls. For Linux, we use smp_processor_id() to get the vCPU
ID. This, as far as I know, will enumerate as CPUs are presented in
ACPI tables (where CPU 0, the boot CPU, will return 0 for
smp_processor_id() and so on.).

--msw

Re: [PATCH] x86/hvm: Extend HVM cpuid leaf with vcpu id

[Andrew Cooper]

On 10/12/14 06:00, Matt Wilson wrote:
> On Thu, Nov 06, 2014 at 09:27:59PM +0000, Andrew Cooper wrote:
>> On 06/11/2014 19:32, Konrad Rzeszutek Wilk wrote:
>>> On Thu, Nov 06, 2014 at 03:07:10PM +0000, Paul Durrant wrote:
>>>> To perform certain hypercalls HVM guests need to use Xen's idea of
>>> What are those 'certain'?
>> Some HVM hypercalls take a vcpu_id as a parameter.  See the
>> functionally-unrelated-but-companion patch "x86/hvm: Add per-vcpu evtchn
>> upcalls".
>>
>> HVM guests currently have no way of obtaining a xen vcpu_id for a given
>> vcpu, and therefore have no way of passing an appropriate parameter to
>> the hypercalls.
> Sorry for chiming in late...
>
> That's not strictly true. You can use the processor index in ACPI.

The LAPIC objects in the MADT/APIC table?  That is still constructed
with the broken LAPIC_ID = 2 * processor_id logic.

>
>> As it currently happens, HVM guests can (and sadly do) take their APIC
>> id, divide by 2, and end up with a number which happens to match the Xen
>> vcpu_id.  This only works because Xen's allocation of APIC IDs is wrong
>> on AMD hardware and Intel Hardware with hyperthreading, and is an issue
>> I plan to fix in the not too distant future.  (It is one of the quagmire
>> of issues relating to cpuid policies)
> This is absolutely the wrong thing to do. If a guest OS is doing this,
> it is doing the wrong thing. FreeBSD went down this erroneous path...
>
> See http://lists.xen.org/archives/html/xen-devel/2013-05/msg03156.html
>
>>>> vcpu id, which may well not match the guest OS idea of CPU id.
>>> Can you elaborate more on the use case please? And why
>>> only restrict this to HVM guests? Why not PV?
>> PV guests unconditionally know their vcpu_id's right from the start, and
>> indeed need them to bring up APs.  HVM guests currently only know APIC IDs.
> No, don't assume anything about APIC IDs mapping to Xen virtual
> processor index. This will break things on EC2 instances that expose
> proper CPU topology through CPUID, and this does *not* follow the
> "index * 2" formula.
>
>>>> This patch adds vcpu id to the HVM cpuid leaf allowing the guest
>>>> to build a mapping.
>>> Don't we have existing hypercalls to get this? Ah VCPUOP_get_physid
>>> is what I was thinking of and that does not seem to be what
>>> you want?
>> This hypercall is only valid for pinned vcpus (i.e. only dom0 with
>> opt_dom0_vcpus_pin), and gives back the underlying APIC ID, and the ACPI
>> processor ID for that APIC ID.
>>
>> The APIC ID can be obtained from cpuid as the vcpus are pinned, and the
>> ACPI ID can be obtained from the ACPI tables, as the domain is dom0. 
>> Ergo, this hypercall is quite redundant, has nothing at all to do with
>> the problem Paul is trying to solve, and appears buggy as it hands back
>> two 64bit values, shifted 32bits apart, in a 64bit integer.
> And the Xen vCPU index can be determined by the processor object IDs
> in DSDT.

The processor object IDs have no requirement in the spec to be in order,
or contiguous.  A DSDT processor object refers back to the MADT, which
in turn still has the broken logic.

Also, any solution using ACPI tables is going to be an issue for PVH guests.

~Andrew

Re: [PATCH] x86/hvm: Extend HVM cpuid leaf with vcpu id

[Matt Wilson]

On Wed, Dec 10, 2014 at 10:36:18AM +0000, Andrew Cooper wrote:
> On 10/12/14 06:00, Matt Wilson wrote:
> >
> > That's not strictly true. You can use the processor index in ACPI.
> 
> The LAPIC objects in the MADT/APIC table?  That is still constructed
> with the broken LAPIC_ID = 2 * processor_id logic.

Have you checked on an EC2 instance (for example: C3, I2, R3)?

> > And the Xen vCPU index can be determined by the processor object IDs
> > in DSDT.
> 
> The processor object IDs have no requirement in the spec to be in order,
> or contiguous.

True - this is a weak heuristic today, so having a CPUID based
approach would be more robust. (To be clear, I'm not arguing against
the proposed patch - I just want to make sure that the justification
doesn't give people the wrong impression about the functional way to
derive Xen vCPU ID without it.)

> A DSDT processor object refers back to the MADT, which in turn still
> has the broken logic.

See above. :-)

> Also, any solution using ACPI tables is going to be an issue for PVH guests.

PVH guests have the same facilities available to them for vCPU
enumeration as PV guests, no?

--msw

Re: [PATCH] x86/hvm: Extend HVM cpuid leaf with vcpu id

[Andrew Cooper]

On 11/12/14 18:19, Matt Wilson wrote:
> On Wed, Dec 10, 2014 at 10:36:18AM +0000, Andrew Cooper wrote:
>> On 10/12/14 06:00, Matt Wilson wrote:
>>> That's not strictly true. You can use the processor index in ACPI.
>> The LAPIC objects in the MADT/APIC table?  That is still constructed
>> with the broken LAPIC_ID = 2 * processor_id logic.
> Have you checked on an EC2 instance (for example: C3, I2, R3)?

I meant in terms of upstream code.  I will believe you when you say that
those instances do it differently.

My point was that the processor index is only as accurate as hvmloader
makes it, and upstream, this relies on the broken logic.

Even if hvmloader is altered and does things differently, it is still
bogus to draw any inference of vcpu_id from processor id.

~Andrew

Re: [PATCH] x86/hvm: Extend HVM cpuid leaf with vcpu id

[Matt Wilson]

On Thu, Dec 11, 2014 at 07:00:55PM +0000, Andrew Cooper wrote:
> On 11/12/14 18:19, Matt Wilson wrote:
> > On Wed, Dec 10, 2014 at 10:36:18AM +0000, Andrew Cooper wrote:
> >> On 10/12/14 06:00, Matt Wilson wrote:
> >>> That's not strictly true. You can use the processor index in ACPI.
> >> The LAPIC objects in the MADT/APIC table?  That is still constructed
> >> with the broken LAPIC_ID = 2 * processor_id logic.
> > Have you checked on an EC2 instance (for example: C3, I2, R3)?
> 
> I meant in terms of upstream code.  I will believe you when you say that
> those instances do it differently.
> 
> My point was that the processor index is only as accurate as hvmloader
> makes it, and upstream, this relies on the broken logic.
> 
> Even if hvmloader is altered and does things differently, it is still
> bogus to draw any inference of vcpu_id from processor id.

hvmloader accurately sets the processor_id in the MADT LAPIC tables to
the correct vcpu_id value (by chance, though things would not work if
this was not so).

hvmloader/acpi/build.c:
>    for ( i = 0; i < nr_processor_objects; i++ )
>    {
>        memset(lapic, 0, sizeof(*lapic));
>        lapic->type    = ACPI_PROCESSOR_LOCAL_APIC;
>        lapic->length  = sizeof(*lapic);
>        /* Processor ID must match processor-object IDs in the DSDT. */
>-->     lapic->acpi_processor_id = i;
>        lapic->apic_id = LAPIC_ID(i);
>        lapic->flags = ((i < hvm_info->nr_vcpus) &&
>                        test_bit(i, hvm_info->vcpu_online)
>                        ? ACPI_LOCAL_APIC_ENABLED : 0);
>        lapic++;
>    }

See also hvmloader/config.h:
> #define LAPIC_ID(vcpu_id)   ((vcpu_id) * 2)

See how Keir called this "vcpu_id"?

--msw