Re: [Xen-devel] [PATCH v2 1/3] x86 / vmx: make apic_access_mfn type-safe

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 23 January 2020 13:30
> To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Jun Nakajima <jun.nakajima@xxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Subject: Re: [PATCH v2 1/3] x86 / vmx: make apic_access_mfn type-safe
> 
> On 23.01.2020 14:09, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of
> Jan
> >> Beulich
> >> Sent: 23 January 2020 12:45
> >> To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>
> >> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Andrew
> Cooper
> >> <andrew.cooper3@xxxxxxxxxx>; Jun Nakajima <jun.nakajima@xxxxxxxxx>;
> xen-
> >> devel@xxxxxxxxxxxxxxxxxxxx; Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >> Subject: Re: [Xen-devel] [PATCH v2 1/3] x86 / vmx: make apic_access_mfn
> >> type-safe
> >>
> >> On 23.01.2020 13:21, Paul Durrant wrote:
> >>> Use mfn_t rather than unsigned long and change previous tests against
> 0
> >> to
> >>> tests against INVALID_MFN (also introducing initialization to that
> >> value).
> >>>
> >>> Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
> >>> Acked-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> >>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>
> >> No, this isn't what the R-b was given for.
> >
> > Oh, sorry, I misunderstood; I thought the R-b was good as long as
> idempotency was ensured.
> >
> >>
> >>> v2:
> >>>  - Set apic_access_mfn to INVALID_MFN in vmx_free_vlapic_mapping() to
> >> make
> >>>    the function idempotent
> >>
> >> Andrew had suggested to use 0 instead of INVALID_MFN. I don't see
> >> how you achieved idempotency with this adjustment. Aiui
> >> vmx_free_vlapic_mapping() is supposed to also run correctly if
> >> vmx_alloc_vlapic_mapping() was never called.
> >
> > It will. vmx_domain_initialise() will set apic_access_mfn to INVALID_MFN
> > so vmx_free_vlapic_mapping() will do nothing.
> 
> I'm sorry, it was implied that it also needs to work if
> vmx_domain_initialise() was never called. Andrew's goal after
> all is, aiui, to be able to call "destroy" functions on error
> paths irrespective of how far "create" had managed to progress.
> 

Oh, I see. That implication was not at all obvious to me. I thought he was just 
after being able to 'destroy' as many times as it took to finish, in which case 
our choice for the value of INVALID_MFN is indeed unfortunate. If that's the 
goal I'll switch to use _mfn(0) as a comparator.

  Paul

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