Re: [PATCH v2 4/4] x86: adjustments to guest handle treatment

[Julien Grall <julien@xxxxxxx>]

Hi,

On 22/04/2020 10:32, Jan Beulich wrote:
> On 22.04.2020 10:17, Julien Grall wrote:
> > On 21/04/2020 10:13, Jan Beulich wrote:
> > > First of all avoid excessive conversions. copy_{from,to}_guest(), for
> > > example, work fine with all of XEN_GUEST_HANDLE{,_64,_PARAM}().
> > >
> > > Further
> > > - do_physdev_op_compat() didn't use the param form for its parameter,
> > > - {hap,shadow}_track_dirty_vram() wrongly used the param form,
> > > - compat processor Px logic failed to check compatibility of native and
> > >     compat structures not further converted.
> > >
> > > As this eliminates all users of guest_handle_from_param() and as there's
> > > no real need to allow for conversions in both directions, drop the
> > > macros as well.
> > I was kind of expecting both guest_handle_from_param() and
> > guest_handle_to_param() to be dropped together. May I ask why
> > you still need guest_handle_to_param()?
> There are three (iirc) uses left which I don't really see how
> to sensibly replace. Take a look at the different callers of
> x86's vcpumask_to_pcpumask(), for example.
Oh, const_guest_handle_from_ptr() is returning a GUEST_HANDLE_PARAM. 
This is a bit odd but fair enough.
> > > --- a/xen/include/xen/acpi.h
> > > +++ b/xen/include/xen/acpi.h
> > > @@ -184,8 +184,8 @@ static inline unsigned int acpi_get_csub
> > >    static inline void acpi_set_csubstate_limit(unsigned int new_limit) { 
> > > return; }
> > >    #endif
> > >    -#ifdef XEN_GUEST_HANDLE_PARAM
> > > -int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE_PARAM(uint32));
> > > +#ifdef XEN_GUEST_HANDLE
> > > +int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE(uint32));
> > >    #endif
> > Do we really need to keep the #ifdef here?
> I think so, yes, or else the original one wouldn't have been
> needed either. (Consider the header getting included without
> any of the public headers having got included first.) Dropping
> (if it was possible) this would be an orthogonal change imo.
Fair point.

Cheers,

--
Julien Grall