Re: [PATCH v14 5/5] arm/vpci: honor access size when returning an error

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 5/15/24 02:32, Jan Beulich wrote:
> On 14.05.2024 22:31, Stewart Hildebrand wrote:
>> Here's what the patch ("arm/vpci: honor access size when returning an
>> error") now looks like based on staging:
>>
>> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
>> index 3bc4bb55082a..31e9e1d20751 100644
>> --- a/xen/arch/arm/vpci.c
>> +++ b/xen/arch/arm/vpci.c
>> @@ -29,6 +29,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t 
>> *info,
>>  {
>>      struct pci_host_bridge *bridge = p;
>>      pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
>> +    const uint8_t access_size = (1U << info->dabt.size) * 8;
> 
> And why exactly uint8_t here, rather than unsigned int? See ./CODING_STYLE.

I'll change to unsigned int.

> 
>> +    const uint64_t invalid = GENMASK_ULL(access_size - 1, 0);
> 
> I'm not entirely convinced of uint64_t here either, but I'd view this as
> more borderline than the uint8_t above. As per ...
> 
>> @@ -39,7 +41,7 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t 
>> *info,
>>          return 1;
>>      }
>>  
>> -    *r = ~0ul;
>> +    *r = invalid;
> 
> ... the original rhs here, unsigned long (or perhaps register_t) would seem
> more appropriate, but I have no idea whether on Arm32 info->dabt.size can
> end up being 3.

Well, it depends which spec we look at. In the ARMv7 ARM, 3 is reserved.
But in the ARMv8 ARM, in the aarch32 section, 3 appears to be valid...

Anyway, since the value returned in r can only be as wide as register_t
due to the way our mmio handlers are implemented, I'll change to
register_t for now to match the lhs.