[1/2] qemu-kvm/cpuid: fix a emulation of guest physical address space

Submitted by Hao, Xudong on Nov. 2, 2012, 5:38 a.m.

Details

Message ID 1351834702-25937-1-git-send-email-xudong.hao@intel.com
State New
Headers show

Commit Message

Hao, Xudong Nov. 2, 2012, 5:38 a.m.
For 64 bit processor, emulate 40 bits physical address if the host physical
address space >= 40bits, else guest physical is same as host.

Signed-off-by: Xudong Hao <xudong.hao@intel.com>
---
 target-i386/cpu.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

Comments

Jan Kiszka Nov. 3, 2012, 10:54 a.m.
On 2012-11-02 06:38, Xudong Hao wrote:
> For 64 bit processor, emulate 40 bits physical address if the host physical
> address space >= 40bits, else guest physical is same as host.
> 
> Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> ---
>  target-i386/cpu.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 423e009..3a78881 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1584,7 +1584,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          if (env->cpuid_ext2_features & CPUID_EXT2_LM) {
>              /* 64 bit processor */
>  /* XXX: The physical address space is limited to 42 bits in exec.c. */
> -            *eax = 0x00003028;	/* 48 bits virtual, 40 bits physical */
> +/* XXX: 40 bits physical if host physical address space >= 40 bits */
> +            uint32_t a, b, c, d;
> +            host_cpuid(0x80000008, 0, &a, &b, &c, &d);
> +            *eax = a < 0x00003028 ? a : 0x00003028;

This variation will not only affect -cpu host, right? That can create
problems when migrating between hosts with different address widths, and
then we will need some control knob to adjust what it reported to the guest.

Jan

>          } else {
>              if (env->cpuid_features & CPUID_PSE36)
>                  *eax = 0x00000024; /* 36 bits physical */
>
Hao, Xudong Nov. 4, 2012, 12:15 p.m.
> -----Original Message-----
> From: Jan Kiszka [mailto:jan.kiszka@web.de]
> Sent: Saturday, November 03, 2012 6:55 PM
> To: Hao, Xudong
> Cc: qemu-devel@nongnu.org; avi@redhat.com; kvm@vger.kernel.org
> Subject: Re: [PATCH 1/2] qemu-kvm/cpuid: fix a emulation of guest physical
> address space
> 
> On 2012-11-02 06:38, Xudong Hao wrote:
> > For 64 bit processor, emulate 40 bits physical address if the host physical
> > address space >= 40bits, else guest physical is same as host.
> >
> > Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> > ---
> >  target-i386/cpu.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> >
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 423e009..3a78881 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1584,7 +1584,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t
> index, uint32_t count,
> >          if (env->cpuid_ext2_features & CPUID_EXT2_LM) {
> >              /* 64 bit processor */
> >  /* XXX: The physical address space is limited to 42 bits in exec.c. */
> > -            *eax = 0x00003028;	/* 48 bits virtual, 40 bits physical */
> > +/* XXX: 40 bits physical if host physical address space >= 40 bits */
> > +            uint32_t a, b, c, d;
> > +            host_cpuid(0x80000008, 0, &a, &b, &c, &d);
> > +            *eax = a < 0x00003028 ? a : 0x00003028;
> 
> This variation will not only affect -cpu host, right? That can create
> problems when migrating between hosts with different address widths, and
> then we will need some control knob to adjust what it reported to the guest.
> 

Oh, I did not consider migrating to different platform(addr widths).
But I think the fixed value 40 bits may cause problem: in VT-d case, when a host support GAW < 40 bits, and qemu emulate 40 bits guest physical address space, will bring bug on:

drivers/iommu/intel-iommu.c
static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
                      unsigned long pfn, int target_level)
{
    int addr_width = agaw_to_width(domain->agaw) - VTD_PAGE_SHIFT;
...
    BUG_ON(!domain->pgd);
    BUG_ON(addr_width < BITS_PER_LONG && pfn >> addr_width);

> Jan
> 
> >          } else {
> >              if (env->cpuid_features & CPUID_PSE36)
> >                  *eax = 0x00000024; /* 36 bits physical */
> >
>
Jan Kiszka Nov. 4, 2012, 12:54 p.m.
On 2012-11-04 13:15, Hao, Xudong wrote:
>> -----Original Message-----
>> From: Jan Kiszka [mailto:jan.kiszka@web.de]
>> Sent: Saturday, November 03, 2012 6:55 PM
>> To: Hao, Xudong
>> Cc: qemu-devel@nongnu.org; avi@redhat.com; kvm@vger.kernel.org
>> Subject: Re: [PATCH 1/2] qemu-kvm/cpuid: fix a emulation of guest physical
>> address space
>>
>> On 2012-11-02 06:38, Xudong Hao wrote:
>>> For 64 bit processor, emulate 40 bits physical address if the host physical
>>> address space >= 40bits, else guest physical is same as host.
>>>
>>> Signed-off-by: Xudong Hao <xudong.hao@intel.com>
>>> ---
>>>  target-i386/cpu.c |    5 ++++-
>>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>>> index 423e009..3a78881 100644
>>> --- a/target-i386/cpu.c
>>> +++ b/target-i386/cpu.c
>>> @@ -1584,7 +1584,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t
>> index, uint32_t count,
>>>          if (env->cpuid_ext2_features & CPUID_EXT2_LM) {
>>>              /* 64 bit processor */
>>>  /* XXX: The physical address space is limited to 42 bits in exec.c. */
>>> -            *eax = 0x00003028;	/* 48 bits virtual, 40 bits physical */
>>> +/* XXX: 40 bits physical if host physical address space >= 40 bits */
>>> +            uint32_t a, b, c, d;
>>> +            host_cpuid(0x80000008, 0, &a, &b, &c, &d);
>>> +            *eax = a < 0x00003028 ? a : 0x00003028;
>>
>> This variation will not only affect -cpu host, right? That can create
>> problems when migrating between hosts with different address widths, and
>> then we will need some control knob to adjust what it reported to the guest.
>>
> 
> Oh, I did not consider migrating to different platform(addr widths).
> But I think the fixed value 40 bits may cause problem: in VT-d case, when a host support GAW < 40 bits, and qemu emulate 40 bits guest physical address space, will bring bug on:
> 
> drivers/iommu/intel-iommu.c
> static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
>                       unsigned long pfn, int target_level)
> {
>     int addr_width = agaw_to_width(domain->agaw) - VTD_PAGE_SHIFT;
> ...
>     BUG_ON(!domain->pgd);
>     BUG_ON(addr_width < BITS_PER_LONG && pfn >> addr_width);
> 

Does it mean that buggy or malicious user space can trigger a kernel
bug? Then this must be fixed of course.

Jan
Hao, Xudong Nov. 5, 2012, 2:42 a.m.
> -----Original Message-----
> From: Jan Kiszka [mailto:jan.kiszka@web.de]
> Sent: Sunday, November 04, 2012 8:55 PM
> To: Hao, Xudong
> Cc: qemu-devel@nongnu.org; avi@redhat.com; kvm@vger.kernel.org
> Subject: Re: [PATCH 1/2] qemu-kvm/cpuid: fix a emulation of guest physical
> address space
> 
> On 2012-11-04 13:15, Hao, Xudong wrote:
> >> -----Original Message-----
> >> From: Jan Kiszka [mailto:jan.kiszka@web.de]
> >> Sent: Saturday, November 03, 2012 6:55 PM
> >> To: Hao, Xudong
> >> Cc: qemu-devel@nongnu.org; avi@redhat.com; kvm@vger.kernel.org
> >> Subject: Re: [PATCH 1/2] qemu-kvm/cpuid: fix a emulation of guest physical
> >> address space
> >>
> >> On 2012-11-02 06:38, Xudong Hao wrote:
> >>> For 64 bit processor, emulate 40 bits physical address if the host physical
> >>> address space >= 40bits, else guest physical is same as host.
> >>>
> >>> Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> >>> ---
> >>>  target-i386/cpu.c |    5 ++++-
> >>>  1 files changed, 4 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> >>> index 423e009..3a78881 100644
> >>> --- a/target-i386/cpu.c
> >>> +++ b/target-i386/cpu.c
> >>> @@ -1584,7 +1584,10 @@ void cpu_x86_cpuid(CPUX86State *env,
> uint32_t
> >> index, uint32_t count,
> >>>          if (env->cpuid_ext2_features & CPUID_EXT2_LM) {
> >>>              /* 64 bit processor */
> >>>  /* XXX: The physical address space is limited to 42 bits in exec.c. */
> >>> -            *eax = 0x00003028;	/* 48 bits virtual, 40 bits physical
> */
> >>> +/* XXX: 40 bits physical if host physical address space >= 40 bits */
> >>> +            uint32_t a, b, c, d;
> >>> +            host_cpuid(0x80000008, 0, &a, &b, &c, &d);
> >>> +            *eax = a < 0x00003028 ? a : 0x00003028;
> >>
> >> This variation will not only affect -cpu host, right? That can create
> >> problems when migrating between hosts with different address widths, and
> >> then we will need some control knob to adjust what it reported to the guest.
> >>
> >
> > Oh, I did not consider migrating to different platform(addr widths).
> > But I think the fixed value 40 bits may cause problem: in VT-d case, when a
> host support GAW < 40 bits, and qemu emulate 40 bits guest physical address
> space, will bring bug on:
> >
> > drivers/iommu/intel-iommu.c
> > static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
> >                       unsigned long pfn, int target_level)
> > {
> >     int addr_width = agaw_to_width(domain->agaw) - VTD_PAGE_SHIFT;
> > ...
> >     BUG_ON(!domain->pgd);
> >     BUG_ON(addr_width < BITS_PER_LONG && pfn >> addr_width);
> >
> 
> Does it mean that buggy or malicious user space can trigger a kernel
> bug? Then this must be fixed of course.
> 
Probably yes, when guest RAM is large enough or allocate MMIO to very high address.

Jan, I'm not familiar the migration, do you have interest to add the migration part fixing?
Jan Kiszka Nov. 5, 2012, 6:22 a.m.
On 2012-11-05 03:42, Hao, Xudong wrote:
>> -----Original Message-----
>> From: Jan Kiszka [mailto:jan.kiszka@web.de]
>> Sent: Sunday, November 04, 2012 8:55 PM
>> To: Hao, Xudong
>> Cc: qemu-devel@nongnu.org; avi@redhat.com; kvm@vger.kernel.org
>> Subject: Re: [PATCH 1/2] qemu-kvm/cpuid: fix a emulation of guest physical
>> address space
>>
>> On 2012-11-04 13:15, Hao, Xudong wrote:
>>>> -----Original Message-----
>>>> From: Jan Kiszka [mailto:jan.kiszka@web.de]
>>>> Sent: Saturday, November 03, 2012 6:55 PM
>>>> To: Hao, Xudong
>>>> Cc: qemu-devel@nongnu.org; avi@redhat.com; kvm@vger.kernel.org
>>>> Subject: Re: [PATCH 1/2] qemu-kvm/cpuid: fix a emulation of guest physical
>>>> address space
>>>>
>>>> On 2012-11-02 06:38, Xudong Hao wrote:
>>>>> For 64 bit processor, emulate 40 bits physical address if the host physical
>>>>> address space >= 40bits, else guest physical is same as host.
>>>>>
>>>>> Signed-off-by: Xudong Hao <xudong.hao@intel.com>
>>>>> ---
>>>>>  target-i386/cpu.c |    5 ++++-
>>>>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>>>>> index 423e009..3a78881 100644
>>>>> --- a/target-i386/cpu.c
>>>>> +++ b/target-i386/cpu.c
>>>>> @@ -1584,7 +1584,10 @@ void cpu_x86_cpuid(CPUX86State *env,
>> uint32_t
>>>> index, uint32_t count,
>>>>>          if (env->cpuid_ext2_features & CPUID_EXT2_LM) {
>>>>>              /* 64 bit processor */
>>>>>  /* XXX: The physical address space is limited to 42 bits in exec.c. */
>>>>> -            *eax = 0x00003028;	/* 48 bits virtual, 40 bits physical
>> */
>>>>> +/* XXX: 40 bits physical if host physical address space >= 40 bits */
>>>>> +            uint32_t a, b, c, d;
>>>>> +            host_cpuid(0x80000008, 0, &a, &b, &c, &d);
>>>>> +            *eax = a < 0x00003028 ? a : 0x00003028;
>>>>
>>>> This variation will not only affect -cpu host, right? That can create
>>>> problems when migrating between hosts with different address widths, and
>>>> then we will need some control knob to adjust what it reported to the guest.
>>>>
>>>
>>> Oh, I did not consider migrating to different platform(addr widths).
>>> But I think the fixed value 40 bits may cause problem: in VT-d case, when a
>> host support GAW < 40 bits, and qemu emulate 40 bits guest physical address
>> space, will bring bug on:
>>>
>>> drivers/iommu/intel-iommu.c
>>> static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
>>>                       unsigned long pfn, int target_level)
>>> {
>>>     int addr_width = agaw_to_width(domain->agaw) - VTD_PAGE_SHIFT;
>>> ...
>>>     BUG_ON(!domain->pgd);
>>>     BUG_ON(addr_width < BITS_PER_LONG && pfn >> addr_width);
>>>
>>
>> Does it mean that buggy or malicious user space can trigger a kernel
>> bug? Then this must be fixed of course.
>>
> Probably yes, when guest RAM is large enough or allocate MMIO to very high address.

...and those things are under user space control. If you have an idea
how to trigger this, please give it a try. This is an availability issue
as untrusted user space could bring down the whole system.

> 
> Jan, I'm not familiar the migration, do you have interest to add the migration part fixing?
> 

I'm not up to date with what is going on in the context of CPU feature
configuration, CC'ing folks who reworked this recently.

In any case, the general pattern is: make this configurable (=> CPU
feature flag) and then possibly also adjust it for compat QEMU machine
types.

Jan
Eduardo Habkost Nov. 8, 2012, 4:40 p.m.
On Mon, Nov 05, 2012 at 07:22:55AM +0100, Jan Kiszka wrote:
> On 2012-11-05 03:42, Hao, Xudong wrote:
> >> -----Original Message-----
> >> From: Jan Kiszka [mailto:jan.kiszka@web.de]
> >> Sent: Sunday, November 04, 2012 8:55 PM
> >> To: Hao, Xudong
> >> Cc: qemu-devel@nongnu.org; avi@redhat.com; kvm@vger.kernel.org
> >> Subject: Re: [PATCH 1/2] qemu-kvm/cpuid: fix a emulation of guest physical
> >> address space
> >>
> >> On 2012-11-04 13:15, Hao, Xudong wrote:
> >>>> -----Original Message-----
> >>>> From: Jan Kiszka [mailto:jan.kiszka@web.de]
> >>>> Sent: Saturday, November 03, 2012 6:55 PM
> >>>> To: Hao, Xudong
> >>>> Cc: qemu-devel@nongnu.org; avi@redhat.com; kvm@vger.kernel.org
> >>>> Subject: Re: [PATCH 1/2] qemu-kvm/cpuid: fix a emulation of guest physical
> >>>> address space
> >>>>
> >>>> On 2012-11-02 06:38, Xudong Hao wrote:
> >>>>> For 64 bit processor, emulate 40 bits physical address if the host physical
> >>>>> address space >= 40bits, else guest physical is same as host.
> >>>>>
> >>>>> Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> >>>>> ---
> >>>>>  target-i386/cpu.c |    5 ++++-
> >>>>>  1 files changed, 4 insertions(+), 1 deletions(-)
> >>>>>
> >>>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> >>>>> index 423e009..3a78881 100644
> >>>>> --- a/target-i386/cpu.c
> >>>>> +++ b/target-i386/cpu.c
> >>>>> @@ -1584,7 +1584,10 @@ void cpu_x86_cpuid(CPUX86State *env,
> >> uint32_t
> >>>> index, uint32_t count,
> >>>>>          if (env->cpuid_ext2_features & CPUID_EXT2_LM) {
> >>>>>              /* 64 bit processor */
> >>>>>  /* XXX: The physical address space is limited to 42 bits in exec.c. */
> >>>>> -            *eax = 0x00003028;	/* 48 bits virtual, 40 bits physical
> >> */
> >>>>> +/* XXX: 40 bits physical if host physical address space >= 40 bits */
> >>>>> +            uint32_t a, b, c, d;
> >>>>> +            host_cpuid(0x80000008, 0, &a, &b, &c, &d);
> >>>>> +            *eax = a < 0x00003028 ? a : 0x00003028;
> >>>>
> >>>> This variation will not only affect -cpu host, right? That can create
> >>>> problems when migrating between hosts with different address widths, and
> >>>> then we will need some control knob to adjust what it reported to the guest.
> >>>>
> >>>
> >>> Oh, I did not consider migrating to different platform(addr widths).
> >>> But I think the fixed value 40 bits may cause problem: in VT-d case, when a
> >> host support GAW < 40 bits, and qemu emulate 40 bits guest physical address
> >> space, will bring bug on:
> >>>
> >>> drivers/iommu/intel-iommu.c
> >>> static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
> >>>                       unsigned long pfn, int target_level)
> >>> {
> >>>     int addr_width = agaw_to_width(domain->agaw) - VTD_PAGE_SHIFT;
> >>> ...
> >>>     BUG_ON(!domain->pgd);
> >>>     BUG_ON(addr_width < BITS_PER_LONG && pfn >> addr_width);
> >>>
> >>
> >> Does it mean that buggy or malicious user space can trigger a kernel
> >> bug? Then this must be fixed of course.
> >>
> > Probably yes, when guest RAM is large enough or allocate MMIO to very high address.
> 
> ...and those things are under user space control. If you have an idea
> how to trigger this, please give it a try. This is an availability issue
> as untrusted user space could bring down the whole system.
> 
> > 
> > Jan, I'm not familiar the migration, do you have interest to add the migration part fixing?
> > 
> 
> I'm not up to date with what is going on in the context of CPU feature
> configuration, CC'ing folks who reworked this recently.
> 
> In any case, the general pattern is: make this configurable (=> CPU
> feature flag) and then possibly also adjust it for compat QEMU machine
> types.

We can't automatically expose data derived from host capabilities to the
guest automatically, as this breaks live migration. This is probably
better handled by adding a new property to the X86CPU class.

If you really want to, you can add a "host" or "auto" mode, too, for
users that don't care about live migration. But that mode can't be
enabled by default (but it could be enabled by -cpu host).

Patch hide | download patch | download mbox

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 423e009..3a78881 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1584,7 +1584,10 @@  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         if (env->cpuid_ext2_features & CPUID_EXT2_LM) {
             /* 64 bit processor */
 /* XXX: The physical address space is limited to 42 bits in exec.c. */
-            *eax = 0x00003028;	/* 48 bits virtual, 40 bits physical */
+/* XXX: 40 bits physical if host physical address space >= 40 bits */
+            uint32_t a, b, c, d;
+            host_cpuid(0x80000008, 0, &a, &b, &c, &d);
+            *eax = a < 0x00003028 ? a : 0x00003028;
         } else {
             if (env->cpuid_features & CPUID_PSE36)
                 *eax = 0x00000024; /* 36 bits physical */