Re: [Xen-devel] [PATCH V3 1/2] x86/vm_event: added hvm/vm_event.{h, c}

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Fri, May 5, 2017 at 10:47 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 05.05.17 at 16:42, <tamas@xxxxxxxxxxxxx> wrote:
>> On Thu, May 4, 2017 at 11:37 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>> On 04.05.17 at 17:17, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>> On 05/04/17 17:57, Tamas K Lengyel wrote:
>>>>> On Thu, May 4, 2017 at 5:22 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>>>>> On 04.05.17 at 11:14, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>>>> On 05/04/17 12:11, Jan Beulich wrote:
>>>>>>>>>>> On 04.05.17 at 11:00, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>>>>>> Created arch/x86/hvm/vm_event.c and include/asm-x86/hvm/vm_event.h,
>>>>>>>>> where HVM-specific vm_event-related code will live. This cleans up
>>>>>>>>> hvm_do_resume() and ensures that the vm_event maintainers are
>>>>>>>>> responsible for changes to that code.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>>>>>>>>> Acked-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
>>>>>>>>
>>>>>>>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>>>>> albeit I wonder ...
>>>>>>>>
>>>>>>>>> +void hvm_vm_event_do_resume(struct vcpu *v)
>>>>>>>>> +{
>>>>>>>>> +    struct monitor_write_data *w;
>>>>>>>>> +
>>>>>>>>> +    if ( likely(!v->arch.vm_event) )
>>>>>>>>> +        return;
>>>>>>>>
>>>>>>>> ... whether this now wouldn't better be an ASSERT().
>>>>>>>
>>>>>>> I have no objections (can this be done on commit or should I re-send 
>>>>>>> V4?).
>>>>>>
>>>>>> Let's first see what Tamas thinks. If he agrees, I see not problem
>>>>>> doing the adjustment while committing.
>>>>>
>>>>> I'm not quite sure how converting that to an ASSERT would work. It
>>>>> looks fine to me as is tbh.
>>>>
>>>> I think Jan means that, since currently the only caller is
>>>> hvm_do_resume() where there's already that check now (to avoid the
>>>> call), we could here simply replace the if() with
>>>> ASSERT(v->arch.vm_event). I could be wrong. :)
>>>
>>> You aren't - that's precisely my reasoning.
>>
>> So if we are changing this to an ASSERT here then a check needs to be
>> added on the caller site. That would work for me.
>
> I don't follow - the reason I did ask for converting the if() here
> was because (upon my request) a check in the caller has been
> added (or actually, is being kept from the original code instead
> of deleting it) in this version.

If there is already a check in the caller, then just go ahead and
convert this to an ASSERT.

Tamas

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