Re: [PATCH v1 2/2] xen/arm: Make has_vpci depend on CONFIG_HAS_VPCI

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 6/25/23 08:45, Julien Grall wrote:
> Hi,
> 
> On 20/06/2023 16:29, Stewart Hildebrand wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> VPCI is disabled on ARM. Make it depend on CONFIG_HAS_VPCI to test the PCI
>> passthrough support.
>>
>> While here, remove the comment on the preceding line.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
>> ---
>> There are two downstreams [1] [2] that have independently made a version this
>> change, each with different Signed-off-by's. I simply picked one at random 
>> for
>> the Author: field, and added both Signed-off-by lines. Please let me know if
>> there are any objections.
>>
>> downstream->v1:
>> * change to IS_ENABLED(CONFIG_HAS_VPCI) instead of hardcoded to true
>> * remove the comment on the preceding line
>>
>> [1] 
>> https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/27be1729ce8128dbe37275ce7946b2fbd2e5a382
>> [2] 
>> https://github.com/xen-troops/xen/commit/bf12185e6fb2e31db0d8e6ea9ccd8a02abadec17
>> ---
>>   xen/arch/arm/include/asm/domain.h | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/domain.h 
>> b/xen/arch/arm/include/asm/domain.h
>> index 99e798ffff68..6e016b00bae1 100644
>> --- a/xen/arch/arm/include/asm/domain.h
>> +++ b/xen/arch/arm/include/asm/domain.h
>> @@ -298,8 +298,7 @@ static inline void arch_vcpu_block(struct vcpu *v) {}
>>
>>   #define arch_vm_assist_valid_mask(d) (1UL << 
>> VMASST_TYPE_runstate_update_flag)
>>
>> -/* vPCI is not available on Arm */
>> -#define has_vpci(d)    ({ (void)(d); false; })
>> +#define has_vpci(d)    ({ (void)(d); IS_ENABLED(CONFIG_HAS_VPCI); })
> 
> This will enable vPCI for all the domains. However, in the cover letter,
> you seemed to suggest that guest support is not there. So shouldn't this
> be "is_harware_domain(d)"? Or d->arch.has_vcpi?

Right, I mentioned in the SMMU series discussion [3] that it will only work in 
dom0 / hardware domain (unless additional vPCI series [4] is applied).

So, making it depend on is_hardware_domain makes sense for now:

#define has_vpci(d) ({ IS_ENABLED(CONFIG_HAS_VPCI) && is_hardware_domain(d); })

However, the is_hardware_domain check should be removed when the vPCI series 
[4] is merged.

[3] https://lists.xenproject.org/archives/html/xen-devel/2023-06/msg01135.html
[4] https://lists.xenproject.org/archives/html/xen-devel/2023-06/msg00863.html