[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 1/3] Add vmware_hw to xl.cfg



On 09/02/14 03:28, Jan Beulich wrote:
On 01.09.14 at 17:33, <dslutz@xxxxxxxxxxx> wrote:
If non-zero then
   Return VMware's cpuid leaves.
   Not doing the hardcoded IRQ9 on PIIX4 ACPI PM.
   Force use of VMware's VGA in QEMU.

The support of hypervisor cpuid leaves has not been agreed to.

MicroSoft Hyper-V (AKA viridian) currently must be at 0x40000000.

VMware currently must be at 0x40000000.

KVM currently must be at 0x40000000 (from Seabios).

Seabios will find xen at 0x40000000, 0x40000100, 0x40000200 ..
0x40010000.

http://download.microsoft.com/download/F/B/0/FB0D01A3-8E3A-4F5F-AA59-08C8026D3B8
A/requirements-for-implementing-microsoft-hypervisor-interface.docx

http://kb.vmware.com/selfservice/microsites/search.do?language=en_US&cmd=disp
layKC&externalId=1009458

http://lwn.net/Articles/301888/
   Attempted to get this cleaned up.

So based on this, I picked the order:

0x40000000 is viridian, vmware or xen
0x40000100 is vmware or xen
0x40000200 is xen
Is there really a point in enabling both Viridian and VMware extensions
at the same time?

Not that I know of (and I do not want to say there there is no code
out there that can work with both).  Instead of an error or warning
I went with what xen is currently doing and that seabios was happy
to find xen at 0x40000200.

If the consensus is to ignore, or report an error or warning I will go that
way.  For now I am not planning on changing.

@@ -149,8 +152,11 @@ void pci_setup(void)
              pci_writew(devfn, 0x20, 0x0000); /* No smb bus IO enable */
              pci_writew(devfn, 0xd2, 0x0000); /* No smb bus IO enable */
              pci_writew(devfn, 0x22, 0x0000);
-            pci_writew(devfn, 0x3c, 0x0009); /* Hardcoded IRQ9 */
-            pci_writew(devfn, 0x3d, 0x0001);
+            if ( !vmware_hw )
+            {
+                pci_writew(devfn, 0x3c, 0x0009); /* Hardcoded IRQ9 */
+                pci_writew(devfn, 0x3d, 0x0001);
+            }
This needs an explanation (it is merely being mentioned in the
description).

Ok, how does this comment sound:

/*
 * When looking more like VMware, let the guest pick the
 * PCI_INTERRUPT_LINE (0x3c) and the PCI_INTERRUPT_PIN(3d)
 * instead of them being hardcoded.  This allows for example
 * lscpci in the guest to match for "PIIX4 ACPI PM" what
 * one gets on VMware.
 */


I also plan to add this to the comment message:

The "not doing the hardcoded IRQ9 on PIIX4 ACPI PM" is only one of
many places where VMware's pci config space is different.  There are
many that also need this kind of change in QEMU.  That part is
outside the scope of this patch.



@@ -5692,6 +5695,22 @@ long do_hvm_op(unsigned long op, 
XEN_GUEST_HANDLE_PARAM(void) arg)
break;
              }
+            case HVM_PARAM_VMWARE_HW:
+                /*
+                 * This should only ever be set non-zero one time by
+                 * the tools and is read only by the guest.
+                 */
+                if ( d == current->domain )
+                {
+                    rc = -EPERM;
+                    break;
+                }
+                if ( d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW] )
+                {
+                    rc = -EEXIST;
+                    break;
+                }
+                break;
Can you make this similar to Viridian, returning success when setting
the value to what it already is.

Yes  (but that patch is not there yet).

--- /dev/null
+++ b/xen/arch/x86/hvm/vmware.c
@@ -0,0 +1,85 @@
+/*
+ * arch/x86/hvm/vmware.c
+ *
+ * Copyright (C) 2012 Verizon Corporation
+ *
+ * This file is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License Version 2 (GPLv2)
+ * as published by the Free Software Foundation.
+ *
+ * This file is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details. <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/sched.h>
+#include <xen/version.h>
+#include <xen/perfc.h>
+#include <xen/hypercall.h>
+#include <xen/domain_page.h>
+#include <asm/paging.h>
+#include <asm/p2m.h>
+#include <asm/apic.h>
+#include <asm/hvm/support.h>
+
+#include <public/xen.h>
+#include <public/sched.h>
+#include <public/hvm/params.h>
+
+#include <asm/hvm/hvm.h>
+#include <asm/hvm/vlapic.h>
+#include <asm/hvm/vmware.h>
Please keep all asm/ includes together. Also, are all above includes
really needed? The list looks more like having got copied from
somewhere else - e.g. I can't see a need for xen/perfc.h.

Yes they were copied.  Will work out a smaller set.

+int cpuid_vmware_leaves(uint32_t idx, uint32_t sub_idx,
+                        uint32_t *eax, uint32_t *ebx,
+                        uint32_t *ecx, uint32_t *edx)
+{
You don't seem to be using sub_idx: Is this a mistake, or are you
passing the value in just in case of future extensions (in which
case I'd be slightly in favor of not passing it for now)?

I was just copying cpuid_hypervisor_leaves() and missed that
it was not needed.  Will remove.

--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -29,6 +29,7 @@
  #include <asm/hvm/io.h>
  #include <xen/hvm/iommu.h>
  #include <asm/hvm/viridian.h>
+#include <asm/hvm/vmware.h>
This being th only change to this file - why?

I was just following how viridian.h is done.

When I had it in just hvm.c, I remember other files not building. This
is no longer the case and so, this file is no longer changed.

   -Don Slutz


Jan


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.