Re: [Xen-devel] [PATCH v7] x86/emulate: Send vm_event from emulate

From: Jan Beulich <JBeulich@suse.com>
To: Alexandru Stefan ISAILA <aisaila@bitdefender.com>
Cc: Petre Ovidiu PIRCALABU <ppircalabu@bitdefender.com>,
	"tamas@tklengyel.com" <tamas@tklengyel.com>,
	"wl@xen.org" <wl@xen.org>,
	"rcojocaru@bitdefender.com" <rcojocaru@bitdefender.com>,
	"george.dunlap@eu.citrix.com" <george.dunlap@eu.citrix.com>,
	"andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>,
	"paul.durrant@citrix.com" <paul.durrant@citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"roger.pau@citrix.com" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v7] x86/emulate: Send vm_event from emulate
Date: Tue, 30 Jul 2019 13:27:00 +0000	[thread overview]
Message-ID: <60651ad5-f0bd-caad-77f6-5f8fb71a8319@suse.com> (raw)
In-Reply-To: <1bdaee7b-a01f-7be8-426f-fe6c678c2859@bitdefender.com>

On 30.07.2019 14:21, Alexandru Stefan ISAILA wrote:
> 
>>>>>> @@ -629,6 +697,14 @@ static void *hvmemul_map_linear_addr(
>>>>>>        
>>>>>>                    ASSERT(p2mt == p2m_ram_logdirty || !p2m_is_readonly(p2mt));
>>>>>>                }
>>>>>> +
>>>>>> +        if ( curr->arch.vm_event &&
>>>>>> +            curr->arch.vm_event->send_event &&
>>>>>> +            hvm_emulate_send_vm_event(addr, gfn, pfec) )
>>>>>> +        {
>>>>>> +            err = ERR_PTR(~X86EMUL_RETRY);
>>>>>> +            goto out;
>>>>>> +        }
>>>>>
>>>>> Did you notice that there's an immediate exit from the loop only
>>>>> in case the linear -> physical translation fails? This is
>>>>> relevant for page fault delivery correctness for accesses
>>>>> crossing page boundaries. I think you want to use
>>>>> update_map_err() and drop the "goto out". I can't really make up
>>>>
>>>> By update_map_err() are you saying to have the err var assigned and then
>>>> drop "goto out"? If so how do I keep the err from my access violation
>>>> without exiting from the loop?
>>>
>>> Counter question: Why do you _need_ to keep "your" value of err?
>>> If, just as an example, there's going to be a #PF on the other
>>> half of the access, then "your" access violation is of no interest
>>> at all.
>>
>> You are right, there is no need to keep the "goto out" here. It was just
>> for optimization in the idea that there is no need to do further steps
>> but I can drop the "goto out" and the code will work the same.
>>
> 
> There is a problem with dropping the "goto out". If everything goes fine
> then it will return the mapping and I don't want that. This can be
> stopped by checking if ( err ) after the loop and it is not null then
> goto out. And going with this idea I can init *err = NULL and drop the
> err = NULL from hvmemul_map_linear_addr(). Is this ok for the next version?

I'd prefer to see the code to decide. If you want this settled before
sending the next full version, then please send at least the resulting
patch hunk(s).

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel