Re: [PATCH v4 07/16] powerpc/pseries/vas: Define VAS/NXGZIP HCALLs and structs

[Nicholas Piggin <npiggin@gmail.com>]

Excerpts from Haren Myneni's message of June 4, 2021 11:30 am:
> On Thu, 2021-06-03 at 14:47 +1000, Nicholas Piggin wrote:
>> Excerpts from Haren Myneni's message of May 21, 2021 7:34 pm:
>> > This patch adds HCALLs and other definitions. Also define structs
>> > that are used in VAS implementation on powerVM.
>> > 
>> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
>> > ---
>> >  arch/powerpc/include/asm/hvcall.h    |   7 ++
>> >  arch/powerpc/include/asm/vas.h       |  32 ++++++++
>> >  arch/powerpc/platforms/pseries/vas.h | 110
>> > +++++++++++++++++++++++++++
>> >  3 files changed, 149 insertions(+)
>> >  create mode 100644 arch/powerpc/platforms/pseries/vas.h
>> > 
>> > diff --git a/arch/powerpc/include/asm/hvcall.h
>> > b/arch/powerpc/include/asm/hvcall.h
>> > index e3b29eda8074..7c3418d1b5e9 100644
>> > --- a/arch/powerpc/include/asm/hvcall.h
>> > +++ b/arch/powerpc/include/asm/hvcall.h
>> > @@ -294,6 +294,13 @@
>> >  #define H_RESIZE_HPT_COMMIT	0x370
>> >  #define H_REGISTER_PROC_TBL	0x37C
>> >  #define H_SIGNAL_SYS_RESET	0x380
>> > +#define H_ALLOCATE_VAS_WINDOW	0x388
>> > +#define H_MODIFY_VAS_WINDOW	0x38C
>> > +#define H_DEALLOCATE_VAS_WINDOW	0x390
>> > +#define H_QUERY_VAS_WINDOW	0x394
>> > +#define H_QUERY_VAS_CAPABILITIES	0x398
>> > +#define H_QUERY_NX_CAPABILITIES	0x39C
>> > +#define H_GET_NX_FAULT		0x3A0
>> >  #define H_INT_GET_SOURCE_INFO   0x3A8
>> >  #define H_INT_SET_SOURCE_CONFIG 0x3AC
>> >  #define H_INT_GET_SOURCE_CONFIG 0x3B0
>> > diff --git a/arch/powerpc/include/asm/vas.h
>> > b/arch/powerpc/include/asm/vas.h
>> > index 49bfb5be896d..371f62d99174 100644
>> > --- a/arch/powerpc/include/asm/vas.h
>> > +++ b/arch/powerpc/include/asm/vas.h
>> > @@ -181,6 +181,7 @@ struct vas_tx_win_attr {
>> >  	bool rx_win_ord_mode;
>> >  };
>> >  
>> > +#ifdef CONFIG_PPC_POWERNV
>> >  /*
>> >   * Helper to map a chip id to VAS id.
>> >   * For POWER9, this is a 1:1 mapping. In the future this maybe a
>> > 1:N
>> > @@ -248,6 +249,37 @@ void vas_win_paste_addr(struct vas_window
>> > *window, u64 *addr,
>> >  int vas_register_api_powernv(struct module *mod, enum vas_cop_type
>> > cop_type,
>> >  			     const char *name);
>> >  void vas_unregister_api_powernv(void);
>> > +#endif
>> > +
>> > +#ifdef CONFIG_PPC_PSERIES
>> > +
>> > +/* VAS Capabilities */
>> > +#define VAS_GZIP_QOS_FEAT	0x1
>> > +#define VAS_GZIP_DEF_FEAT	0x2
>> > +#define VAS_GZIP_QOS_FEAT_BIT	PPC_BIT(VAS_GZIP_QOS_FEAT) /*
>> > Bit 1 */
>> > +#define VAS_GZIP_DEF_FEAT_BIT	PPC_BIT(VAS_GZIP_DEF_FEAT) /*
>> > Bit 2 */
>> > +
>> > +/* NX Capabilities */
>> > +#define VAS_NX_GZIP_FEAT	0x1
>> > +#define VAS_NX_GZIP_FEAT_BIT	PPC_BIT(VAS_NX_GZIP_FEAT) /*
>> > Bit 1 */
>> > +#define VAS_DESCR_LEN		8
>> > +
>> > +/*
>> > + * These structs are used to retrieve overall VAS capabilities
>> > that
>> > + * the hypervisor provides.
>> > + */
>> > +struct hv_vas_all_caps {
>> 
>> ...
>> 
>> > +
>> > +/*
>> > + * Use to get feature specific capabilities from the
>> > + * hypervisor.
>> > + */
>> > +struct hv_vas_ct_caps {
>> 
>> ...
>> 
>> > +/*
>> > + * To get window information from the hypervisor.
>> > + */
>> > +struct hv_vas_win_lpar {
>> 
>> Are any of these names coming from PAPR? If not, then typically we
>> don't 
>> use hv_ kind of prefixes for something we got from the hypervisor,
>> but
>> rather something that's hypervisor privileged or specific information
>> about the hypervisor perhaps (which is not the same as what the 
>> hypervisor may or may not advertise to the guest).
>> 
>> So if these are all capabilities and features the hypervisor
>> advertises 
>> to the LPAR, I think the hv_ should be dropped.
> 
> The hypervisor advertises the available capabalities and the LPAR
> passes buffer to HCALLs to retrieve these capabilties / features (in
> BE). 
> 
> I was using *_be in the previous version. So can I use like 'struct
> vas_ct_caps_be'  
> 
>> 
>> Otherwise seems okay. I would be careful of the "lpar" name. I think 
>> it's okay in this situation where the hypervisor advertises features
>> to 
>> the partition, but in other parts of the vas code you call it
>> pseries_
>> but you also have some lpar_ bits. So aside from interacting with
>> PAPR
>> APIs, it would be safe to consistently use pseries for your driver
>> and
>> type names.
> 
> I can use 'struct hv_vas_win_pseries' or 'struct vas_win_pseries_be'

I'm actually wrong about hv_ prefix now I look a bit further, there are
other structures that do use it for similar hcalls, sorry about that.

I'm not sure I like it too much but it is what it is. I would say don't
worry about churning your hv_ names for now. It's probably better / more 
used than _be postfix. Changing conventions could be a later 
powerpc-wide series if it's that important.

Thanks,
Nick