Re: [Xen-devel] [PATCH] pvh: Fix regression caused by assumption that HVM paths MUST use io-backend device.

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Konrad Rzeszutek Wilk wrote on 2014-02-05:
> On Wed, Feb 05, 2014 at 02:35:51PM +0000, George Dunlap wrote:
>> On 02/04/2014 04:42 PM, Konrad Rzeszutek Wilk wrote:
>>> On Tue, Feb 04, 2014 at 03:46:48PM +0000, Jan Beulich wrote:
>>>>>>> On 04.02.14 at 16:32, Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx> wrote:
>>>>> On Tue, Feb 04, 2014 at 03:02:44PM +0000, Jan Beulich wrote:
>>>>>> Wasn't it that Mukesh's patch simply was yours with the two
>>>>>> get_ioreq()s folded by using a local variable?
>>>>> Yes. As so
>>>> Thanks. Except that ...
>>>> 
>>>>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>>>>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>>>>> @@ -1394,13 +1394,13 @@ void nvmx_switch_guest(void)
>>>>>      struct vcpu *v = current;
>>>>>      struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
>>>>>      struct cpu_user_regs *regs = guest_cpu_user_regs();
>>>>> -
>>>>> +    ioreq_t *p = get_ioreq(v);
>>>> ... you don't want to drop the blank line, and naming the new
>>>> variable "ioreq" would seem preferable.
>>>> 
>>>>>      /*
>>>>>       * a pending IO emualtion may still no finished. In this case,
>>>>>       * no virtual vmswith is allowed. Or else, the following IO
>>>>>       * emulation will handled in a wrong VCPU context.
>>>>>       */
>>>>> -    if ( get_ioreq(v)->state != STATE_IOREQ_NONE )
>>>>> +    if ( p && p->state != STATE_IOREQ_NONE )
>>>> And, as said before, I'd think "!p ||" instead of "p &&" would be
>>>> the right thing here. Yang, Jun?
>>> I have two patches - one the simpler one that is pretty
>>> straightfoward and the one you suggested. Either one fixes PVH
>>> guests. I also did bootup tests with HVM guests to make sure they worked.
>>> 
>>> Attached and inline.
>> 

Sorry for the late response. I just back from Chinese new year holiday.

>> But they do different things -- one does "ioreq && ioreq->state..."
> 
> Correct.
>> and the other does "!ioreq || ioreq->state...".  The first one is
>> incorrect, AFAICT.
> 
> Both of them fix the hypervisor blowing up with any PVH guest.

Both of fixings are right to me.
The only concern is that what we want to do here:
"ioreq && ioreq->state..." will only allow the VCPU that supporting IO request 
emulation mechanism to continue nested check which current means HVM VCPU.
And "!ioreq || ioreq->state..." will check the VCPU that doesn't support the IO 
request emulation mechanism only which current means PVH VCPU.

The purpose of my original patch only wants to allow the HVM VCPU that doesn't 
has pending IO request to continue nested check. Not use it to distinguish 
whether it is HVM or PVH. So here I prefer to only allow HVM VCPU goes to here 
as Jan mentioned before that non-HVM domain should never call nested related 
function at all unless it also supports nested.

Best regards,
Yang


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