The HVM flavor of the hypercall handlers exists only when GRANT_TABLE is
enabled, while surrogate shim variants exist only for the purpose of PV.
(Also scratch out the Arm variant in that case; what exactly is used in
that cell of the new table row doesn't really matter.)

Fixes: 8523851dbc49 ("xen/x86: call hypercall handlers via generated macro")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>

--- a/xen/include/hypercall-defs.c
+++ b/xen/include/hypercall-defs.c
@@ -241,8 +241,10 @@ event_channel_op_compat            do
 xen_version                        compat   do       compat   do       do
 console_io                         do       do       do       do       do
 physdev_op_compat                  compat   do       -        -        dep
-#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM)
+#if defined(CONFIG_GRANT_TABLE)
 grant_table_op                     compat   do       hvm      hvm      do
+#elif defined(CONFIG_PV_SHIM)
+grant_table_op                     compat   do       -        -        -
 #endif
 vm_assist                          do       do       do       do       do
 update_va_mapping_otherdomain      compat   do       -        -        -

On 01/12/2022 15:57, Jan Beulich wrote:
> The HVM flavor of the hypercall handlers exists only when GRANT_TABLE is
> enabled, while surrogate shim variants exist only for the purpose of PV.
> (Also scratch out the Arm variant in that case; what exactly is used in
> that cell of the new table row doesn't really matter.)
>
> Fixes: 8523851dbc49 ("xen/x86: call hypercall handlers via generated macro")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks for investigating.

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thoughts about inclusion into 4.17?  This is a build time regression vs
4.16.

On 01.12.2022 17:01, Andrew Cooper wrote:
> On 01/12/2022 15:57, Jan Beulich wrote:
>> The HVM flavor of the hypercall handlers exists only when GRANT_TABLE is
>> enabled, while surrogate shim variants exist only for the purpose of PV.
>> (Also scratch out the Arm variant in that case; what exactly is used in
>> that cell of the new table row doesn't really matter.)
>>
>> Fixes: 8523851dbc49 ("xen/x86: call hypercall handlers via generated macro")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Thanks for investigating.
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

> Thoughts about inclusion into 4.17?  This is a build time regression vs
> 4.16.

I thought this was odd enough a configuration, but since you ask, let me
forward the question to Henry.

Jan

Hi Jan and Andrew,

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Subject: [4.17?] Re: [PATCH] x86/HVM+shim: fix build
> when !CONFIG_GRANT_TABLE
> 
> On 01.12.2022 17:01, Andrew Cooper wrote:
> > On 01/12/2022 15:57, Jan Beulich wrote:
> >> The HVM flavor of the hypercall handlers exists only when GRANT_TABLE
> is
> >> enabled, while surrogate shim variants exist only for the purpose of PV.
> >> (Also scratch out the Arm variant in that case; what exactly is used in
> >> that cell of the new table row doesn't really matter.)
> >>
> >> Fixes: 8523851dbc49 ("xen/x86: call hypercall handlers via generated
> macro")
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >
> > Thanks for investigating.
> >
> > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Thanks.
> 
> > Thoughts about inclusion into 4.17?  This is a build time regression vs
> > 4.16.
> 
> I thought this was odd enough a configuration, but since you ask, let me
> forward the question to Henry.

I think a build time regression should be fixed. We cannot assume users
will not use this configuration (as Jan pointed out in IRC). So for 4.17:

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

But if OSSTest is exploded after merging this patch, I would like to request
a revert since we don't have too much time left for the due date. Hopefully
everyone is fine with that.

Kind regards,
Henry

> 
> Jan

On 01/12/2022 16:14, Henry Wang wrote:
>>> Thanks for investigating.
>>>
>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Thanks.
>>
>>> Thoughts about inclusion into 4.17?  This is a build time regression vs
>>> 4.16.
>> I thought this was odd enough a configuration, but since you ask, let me
>> forward the question to Henry.
> I think a build time regression should be fixed. We cannot assume users
> will not use this configuration (as Jan pointed out in IRC). So for 4.17:
>
> Release-acked-by: Henry Wang <Henry.Wang@arm.com>
>
> But if OSSTest is exploded after merging this patch, I would like to request
> a revert since we don't have too much time left for the due date. Hopefully
> everyone is fine with that.

It is very unlikely that people are going have a configuration like this
in production.

But, the 4.17 branch does have Gitlab CI running on it, including
randconf tests, which provably do spot the error occasionally.

The (IMO better) justification to take it into 4.17 at this point is to
fix a CI failure.

~Andrew

Hi Andrew,

> -----Original Message-----
> From: Andrew Cooper <Andrew.Cooper3@citrix.com>
> Subject: Re: [4.17?] Re: [PATCH] x86/HVM+shim: fix build
> when !CONFIG_GRANT_TABLE
> 
> On 01/12/2022 16:14, Henry Wang wrote:
> >>> Thanks for investigating.
> >>>
> >>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> Thanks.
> >>
> >>> Thoughts about inclusion into 4.17?  This is a build time regression vs
> >>> 4.16.
> >> I thought this was odd enough a configuration, but since you ask, let me
> >> forward the question to Henry.
> > I think a build time regression should be fixed. We cannot assume users
> > will not use this configuration (as Jan pointed out in IRC). So for 4.17:
> >
> > Release-acked-by: Henry Wang <Henry.Wang@arm.com>
> >
> > But if OSSTest is exploded after merging this patch, I would like to request
> > a revert since we don't have too much time left for the due date. Hopefully
> > everyone is fine with that.
> 
> It is very unlikely that people are going have a configuration like this
> in production.
> 
> But, the 4.17 branch does have Gitlab CI running on it, including
> randconf tests, which provably do spot the error occasionally.
> 
> The (IMO better) justification to take it into 4.17 at this point is to
> fix a CI failure.

Good point, my release ack is still valid :)

Kind regards,
Henry

> 
> ~Andrew