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

From: Jan Beulich <JBeulich@suse.com>
To: Alexandru Stefan ISAILA <aisaila@bitdefender.com>
Cc: Wei Liu <wl@xen.org>,
	"kevin.tian@intel.com" <kevin.tian@intel.com>,
	"tamas@tklengyel.com" <tamas@tklengyel.com>,
	"jun.nakajima@intel.com" <jun.nakajima@intel.com>,
	"rcojocaru@bitdefender.com" <rcojocaru@bitdefender.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Tim Deegan <tim@xen.org>, PaulDurrant <Paul.Durrant@citrix.com>,
	"suravee.suthikulpanit@amd.com" <suravee.suthikulpanit@amd.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	"brian.woods@amd.com" <brian.woods@amd.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v5] x86/emulate: Send vm_event from emulate
Date: Mon, 1 Jul 2019 14:55:14 +0000	[thread overview]
Message-ID: <3fa5932d-174c-9b57-3cb6-aab4eb6a5238@suse.com> (raw)
In-Reply-To: <0b12721c-8820-7227-1079-0bd1f5587287@bitdefender.com>

On 01.07.2019 16:45, Alexandru Stefan ISAILA wrote:
> On 01.07.2019 16:13, Jan Beulich wrote:
>>>>> On 04.06.19 at 13:49, <aisaila@bitdefender.com> wrote:
>>> +    if ( !send_event || !pfec )
>>> +        return false;
>>
>> I think I've said before that the !pfec part need an explanation (in
>> a comment). Without such an explanation I'm inclined to say it
>> should be deleted. If otoh this is simply mean to be a shortcut,
>> then you should really just check the two bits you actually care
>> about further down.
> 
> The pfec check is done because I encountered pfec 0 in tests. It could
> save some work if pfec == 0 when calling the function. Is there
> a must in having this check removed? If so then it can be done. The
> send_event will be checked before calling the function as you said.

It's not a requirement for it to be removed, _if_ there's a good
reason for it to be there. I don't, however, see how pfec=0 could
be a problem - afaict it would return false a few lines further
down in that case.

>>> +    if ( !req.u.mem_access.flags )
>>> +        return false; /* no violation */
>>
>> How is the "false" here (I think this is the one the description talks
>> about) matching up with the various other ones in the function?
> 
> There should be no event if there is no access violation. So in this
> case the emulation is continued as expected.

But this doesn't answer my question: You use "false" as return value
to indicate different things. Only the one here means "no access
violation".

>>> @@ -615,6 +669,13 @@ static void *hvmemul_map_linear_addr(
>>>    
>>>            if ( pfec & PFEC_write_access )
>>>            {
>>> +            if ( hvm_emulate_send_vm_event(addr, gfn, pfec,
>>> +                                           hvmemul_ctxt->send_event) )
>>> +            {
>>> +                err = ERR_PTR(~X86EMUL_RETRY);
>>> +                goto out;
>>> +            }
>>
>> How come this sits only on the write path?
> 
> We are interested only for the write access on this path. This also
> ensures that pfec is set.

I'm sorry, but the event sending should not be tailored to what you
need or want. Or if so (i.e. if agreed upon among the VM event
maintainers) then this restriction should be clearly spelled out.

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