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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

> @@ -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).

> @@ -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.

> --- /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.

> +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)?

> --- 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?

Jan

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