RE: [PATCH 2/3] drivers:hv: Export the API to invoke a hypercall on Hyper-V

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx]
> Sent: Monday, September 21, 2015 1:45 PM
> To: KY Srinivasan <kys@xxxxxxxxxxxxx>
> Cc: olaf@xxxxxxxxx; jasowang@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> Jake Oshins <jakeo@xxxxxxxxxxxxx>; apw@xxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/3] drivers:hv: Export the API to invoke a hypercall on
> Hyper-V
> 
> On Mon, Sep 21, 2015 at 05:36:06PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx]
> > > Sent: Monday, September 21, 2015 9:41 AM
> > > To: KY Srinivasan <kys@xxxxxxxxxxxxx>
> > > Cc: linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx;
> > > olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx; vkuznets@xxxxxxxxxx;
> > > jasowang@xxxxxxxxxx; Jake Oshins <jakeo@xxxxxxxxxxxxx>
> > > Subject: Re: [PATCH 2/3] drivers:hv: Export the API to invoke a hypercall
> on
> > > Hyper-V
> > >
> > > On Mon, Sep 21, 2015 at 04:22:01PM +0000, KY Srinivasan wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx]
> > > > > Sent: Sunday, September 20, 2015 10:28 PM
> > > > > To: KY Srinivasan <kys@xxxxxxxxxxxxx>
> > > > > Cc: linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx;
> > > > > olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx; vkuznets@xxxxxxxxxx;
> > > > > jasowang@xxxxxxxxxx; Jake Oshins <jakeo@xxxxxxxxxxxxx>
> > > > > Subject: Re: [PATCH 2/3] drivers:hv: Export the API to invoke a
> hypercall
> > > on
> > > > > Hyper-V
> > > > >
> > > > > On Tue, Sep 15, 2015 at 06:26:48PM -0700, K. Y. Srinivasan wrote:
> > > > > > From: Jake Oshins <jakeo@xxxxxxxxxxxxx>
> > > > > >
> > > > > > This patch exposes the function that hv_vmbus.ko uses to make
> > > hypercalls.
> > > > > This
> > > > > > is necessary for retargeting an interrupt when it is given a new
> affinity.
> > > > > >
> > > > > > Since we are exporting this API, rename the API as it will be visible
> > > outside
> > > > > > the hv.c file.
> > > > > >
> > > > > > Signed-off-by: Jake Oshins <jakeo@xxxxxxxxxxxxx>
> > > > > > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> > > > > > ---
> > > > > >  drivers/hv/hv.c        |    9 +++++----
> > > > > >  include/linux/hyperv.h |    1 +
> > > > > >  2 files changed, 6 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> > > > > > index 6341be8..a7b6c6a 100644
> > > > > > --- a/drivers/hv/hv.c
> > > > > > +++ b/drivers/hv/hv.c
> > > > > > @@ -89,9 +89,9 @@ static int query_hypervisor_info(void)
> > > > > >  }
> > > > > >
> > > > > >  /*
> > > > > > - * do_hypercall- Invoke the specified hypercall
> > > > > > + * hv_do_hypercall- Invoke the specified hypercall
> > > > > >   */
> > > > > > -static u64 do_hypercall(u64 control, void *input, void *output)
> > > > > > +u64 hv_do_hypercall(u64 control, void *input, void *output)
> > > > > >  {
> > > > > >  	u64 input_address = (input) ? virt_to_phys(input) : 0;
> > > > > >  	u64 output_address = (output) ? virt_to_phys(output) : 0;
> > > > > > @@ -132,6 +132,7 @@ static u64 do_hypercall(u64 control, void
> *input,
> > > > > void *output)
> > > > > >  	return hv_status_lo | ((u64)hv_status_hi << 32);
> > > > > >  #endif /* !x86_64 */
> > > > > >  }
> > > > > > +EXPORT_SYMBOL_GPL(hv_do_hypercall);
> > > > > >
> > > > > >  #ifdef CONFIG_X86_64
> > > > > >  static cycle_t read_hv_clock_tsc(struct clocksource *arg)
> > > > > > @@ -329,7 +330,7 @@ int hv_post_message(union
> hv_connection_id
> > > > > connection_id,
> > > > > >  	aligned_msg->payload_size = payload_size;
> > > > > >  	memcpy((void *)aligned_msg->payload, payload,
> payload_size);
> > > > > >
> > > > > > -	status = do_hypercall(HVCALL_POST_MESSAGE,
> aligned_msg, NULL)
> > > > > > +	status = hv_do_hypercall(HVCALL_POST_MESSAGE,
> aligned_msg,
> > > > > NULL)
> > > > > >  		& 0xFFFF;
> > > > > >
> > > > > >  	put_cpu();
> > > > > > @@ -347,7 +348,7 @@ u16 hv_signal_event(void *con_id)
> > > > > >  {
> > > > > >  	u16 status;
> > > > > >
> > > > > > -	status = (do_hypercall(HVCALL_SIGNAL_EVENT, con_id,
> NULL) &
> > > > > 0xFFFF);
> > > > > > +	status = (hv_do_hypercall(HVCALL_SIGNAL_EVENT, con_id,
> NULL) &
> > > > > 0xFFFF);
> > > > >
> > > > > What's with the crazy () around a function call?
> > > >
> > > > I will address this and resend.
> > > > >
> > > > > And why are you passing a 64bit return value into a 16bit status value?
> > > > > That seems like something is broken.
> > > >
> > > > The low order 16 bits have the valid status codes we are interested in.
> > >
> > > Then why even return a 64bit number if no one uses it?
> >
> > The hypervisor ABI specifies a 64 bit return value.
> 
> Then just fricken return a 32bit value, don't force everyone who calls
> it to do themselves...

Sorry for the confusion. The generic hypercall primitive will return a 64 bit value
that includes the status bits and other information. As we export the ability to invoke
the hypercall, we should preserve the 64 bits of the return value as the potential
consumers of this hypercall interface may care about the other information
in the return value.

Currently, we have a couple of wrappers built on the hypercall primitive that just
need to return the status value and don't care about other bits in the return value. I will
cleanup the code in these wrappers functions (two to be precise).

Regards,

K. Y
 
> 
> greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/