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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 5/14/24 13:48, Julien Grall wrote:
> Hi Stewart,
> 
> On 14/05/2024 15:33, Stewart Hildebrand wrote:
>> From: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
>>
>> Guest can try to read config space using different access sizes: 8,
>> 16, 32, 64 bits. We need to take this into account when we are
>> returning an error back to MMIO handler, otherwise it is possible to
>> provide more data than requested: i.e. guest issues LDRB instruction
>> to read one byte, but we are writing 0xFFFFFFFFFFFFFFFF in the target
>> register.
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> 
> With one remark below:
> 
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

Thanks!

> 
>> ---
>> v9->10:
>> * New patch in v10.
>> ---
>>   xen/arch/arm/vpci.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
>> index 348ba0fbc860..aaf9d9120c3d 100644
>> --- a/xen/arch/arm/vpci.c
>> +++ b/xen/arch/arm/vpci.c
>> @@ -41,6 +41,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t 
>> *info,
>>   {
>>       struct pci_host_bridge *bridge = p;
>>       pci_sbdf_t sbdf;
>> +    const uint8_t access_size = (1 << info->dabt.size) * 8;

I'd like to add a U suffix to the 1 to make it consistent with the
remaining occurrences in this file.

>> +    const uint64_t access_mask = GENMASK_ULL(access_size - 1, 0);
>>       /* data is needed to prevent a pointer cast on 32bit */
>>       unsigned long data;
>>   @@ -48,7 +50,7 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t 
>> *info,
>>         if ( !vpci_sbdf_from_gpa(v->domain, bridge, info->gpa, &sbdf) )
>>       {
>> -        *r = ~0UL;
>> +        *r = access_mask;
> 
> The name 'access_mask' is a bit confusing. I would not expect such value
> for be returned to the guest. What about 'invalid'?

That sounds good, I've made the change in my local tree.

> 
> Also can you confirm whether patches #4 and #5 be committed without
> the rest of the series?

Patch #4: no, it uses a constant defined in patch #2 ("vpci: add initial
support for virtual PCI bus topology").

Patch #5: conceptually, yes, but patch #3 ("xen/arm: translate virtual
PCI bus topology for guests") also modifies vpci_mmio_read(), so there
are rebase conflicts to resolve in both patches #3 and #5. Thinking more
about it, patch #5 falls more into the category of a fix than a feature,
so it probably should have been in the beginning of the series anyway.
Alright, I've reordered it and resolved the rebase conflicts in my local
tree.

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;
+    const uint64_t invalid = GENMASK_ULL(access_size - 1, 0);
     /* data is needed to prevent a pointer cast on 32bit */
     unsigned long data;
 
@@ -39,7 +41,7 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
         return 1;
     }
 
-    *r = ~0ul;
+    *r = invalid;
 
     return 0;
 }

The patch ("xen/arm: translate virtual PCI bus topology for guests")
will then introduce a new use of the "invalid" variable.