Re: [PATCH RFC v12 07/21] pvh: Disable unneeded features of HVM containers

[George Dunlap <george.dunlap@eu.citrix.com>]

On 18/09/13 15:18, Jan Beulich wrote:
>>>> On 13.09.13 at 18:25, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> @@ -368,6 +375,7 @@ void hvm_do_resume(struct vcpu *v)
>>           }
>>       }
>>   
>> +  check_inject_trap:
> Labels are commonly indented by just one space.

Ack -- sorry, emacs its own ideas about how they should be indented.  I 
try to catch these but I don't always notice.

>
>> @@ -521,6 +529,7 @@ int hvm_domain_initialise(struct domain *d)
>>           return -EINVAL;
>>       }
>>   
>> +    /* PVH: pbut_lock and uc_lock unused, but won't hurt */
>>       spin_lock_init(&d->arch.hvm_domain.pbuf_lock);
>>       spin_lock_init(&d->arch.hvm_domain.irq_lock);
>>       spin_lock_init(&d->arch.hvm_domain.uc_lock);
> Typo (pbuf_lock). But the comment is pretty pointless anyway.
> And if any future patch starts using either of the mentioned
> locks, it'll likely forget to update the comment (and hence it get
> stale).
>
>> --- a/xen/arch/x86/hvm/io.c
>> +++ b/xen/arch/x86/hvm/io.c
>> @@ -175,6 +175,10 @@ int handle_mmio(void)
>>       struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io;
>>       int rc;
>>   
>> +    /* No MMIO for PVH vcpus */
>> +    if ( is_pvh_vcpu(curr) )
>> +        return 0;
>> +
> So why does hvm_pio() not get adjusted similarly?

At the moment I don't think hvm_pio() will be called for PVH guests.  IO 
instructions for PVH domains are handled by the PV io emulation engine.  
The check here is to make sure that Xen doesn't try to do MMIO for PVH 
domains on an EPT violation.  Mukesh had an "if(!is_pvh_domain)" in the 
EPT handler before calling hvm_hap_nested_page_fault(), which not only 
filtered out MMIO, but also any other reason the fault may have happened 
(e.g., paging or a mem event).

I put the check in handle_mmio() instead of hvm_hap_nested_page_fault() 
just in case there was another path to that function.  I suppose the 
same logic applies to hvm_pio(). We could put either an ASSERT or just a 
check that returns failure.  (I would lean toward an ASSERT.)

  -George