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

[Don Slutz <dslutz@verizon.com>]

On 09/02/14 03:28, Jan Beulich wrote:
>>>> On 01.09.14 at 17:33, <dslutz@verizon.com> 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